Add should-flag-change advisory skill + eval suite#101
Merged
Conversation
New read-only skill that decides whether a code change should ship behind a LaunchDarkly feature flag. Invoked ad hoc or in CI on a PR: it reads the diff and surrounding code, weighs a decision framework (favoring flags for user-facing and risky changes, weighting false negatives over false positives), and ends with a structured recommend-flag verdict. It never creates or modifies flags. - skills/feature-flags/should-flag-change: SKILL.md (read-only scope boundary, confusion-matrix decision framework, recommend-flag verdict contract) plus README and marketplace.json - evals: recommend-flag tool definition + mock, git_diff provider var injected as a <git_diff> tag, and a two-tier suite (judgment vs agentic providers, targeted per fixture) with 5 labeled fixtures - evals/scripts/build-pr-fixture.js: build a fixture from a real PR via gh pr diff + refs/pull/N/head, bootstrapping the label from LD SDK call sites - wiring: package.json scripts, _manifest.js, README skill list, skills.json
Skill eval results
Only suites whose source actually changed since their last recorded score were re-run. Soft-failing while we stabilise the baseline. |
Vadman97
self-requested a review
July 9, 2026 18:23
Extends the should-flag-change eval suite and its fixture tooling based on
testing against real pull requests.
- promptfooconfig.yaml: add three synthetic fixtures for the "already gated"
case the suite didn't cover — a change that ships behind a flag should get
recommend: false ("already handled"), and its ungated mirror should get
recommend: true. Uses public SDK idioms; no proprietary source.
- provider: fix a Nunjucks var-render crash on diffs containing `{{ ... }}`
(JSX props, Go templates). Fixtures may wrap such content in a `{% raw %}`
block; stripRawWrapper removes it before the agent sees the diff.
- build-pr-fixture.js: read changed files via the GitHub contents API instead
of fetching refs/pull/N/head (avoids cloning a large external repo); add
--flag-pattern / --flag-token / --flag-file-pattern to teach it a codebase's
flag conventions at call time, and --counterfactual to build positive-recall
fixtures by stripping the flag gate. Kept repo-agnostic — no baked-in
conventions.
- tests: unit-test stripRawWrapper and the builder's pure functions
(addedLines, detectLdUsage, splitDiffByFile, dropFilesFromDiff,
extractFlagTokens, stripFlagGate).
Bring the flag-and-release-change skill (originally PR #102) onto the should-flag-change branch (#101) so the advisory "decide" step and the "apply" step ship together. Only the content of #102's two flag-and-release commits is included -- not that branch's unrelated observability skills (which sit on an unmerged #99 commit). README skill list updated and skills.json regenerated from the catalog script.
should-flag-change: - Ancestor-gate analysis: detect an enclosing flag the change already sits behind, and judge on its rollout state. - Prerequisite/dependency signal as a reason to flag. - Optional risk/blast-radius field on the verdict, orthogonal to confidence (added to the eval tool schema, additive/non-breaking). - Refactor-that-changes-a-contract case; net-new-vs-incremental measurability nuance; unverified-claim confidence discipline. flag-and-release-change: - Capture and honor human release intent (release / hold / notBefore / segment / prerequisite), fail-closed; three-layer precedence (intent -> override -> policy -> default). - "The deploy is not the release" framing. - Fail-closed on non-409 flag-creation errors. - Paired flag-on/flag-off tests run to green before push. - Metric-adequacy check for guarded policies; prerequisite coupling. - Off-path-invokes-no-new-code invariant; edge cases for duplicate config, registering before the PR exists, and double-toggling. Sources: launchdarkly-labs/launchdarkly-auto-factory (AI-config prompts, ADRs) and launchdarkly/gonfalon .agents/skills. Portable-safe only -- repo-specific specifics (env matrix, release tags, context kinds) left out as the per-repo customization surface.
…trator Decompose the "apply" half into composable, harness-callable units, matching gonfalon's planning/add-flag/rollout shape while staying generic: - New skill feature-flags/flag-release: records a flag's automated rollout for a PR (match-release-policies preview, release intent + fail-closed hold, precedence, prerequisites, metric-adequacy). Operates on an existing flag; creates no flags and edits no code. auto-release.md moves here as its core reference. - flag-and-release-change becomes a thin portable orchestrator that composes should-flag-change -> launchdarkly-flag-create -> flag-release, owning only the PR workflow (clone/diff/push) and plan->implement sequencing. Notes that an automation harness can bypass it and invoke the three atoms directly. Repo-specific values (env matrix, release tags, targeting context) remain out of these generic skills -- that is the AgentControl customization seam.
vroske-ld
reviewed
Jul 15, 2026
vroske-ld
left a comment
vroske-ld
left a comment
There was a problem hiding this comment.
I found a few things worth tightening before merge:
-
skills.jsonlooks generated from local dependency state. The diff addsdotenv,dotenvx, and Playwright skills fromevals/node_modules, but CI'sscripts/generate_catalog.py --checkfails because the catalog generated in CI does not match. Sincegenerate_catalog.pyscans every**/SKILL.mdand only excludestemplate, local installs can leak third-party dependency skills into the public catalog. I think the generator/validator should excludenode_modules, thenskills.jsonshould be regenerated. -
The eval does not currently enforce the core
recommend-flagcontract. The skill and tool definition both say the agent must callrecommend-flagexactly once as the final step, but the assertions usetraj.find(t => t.tool === 'recommend-flag'). A run that calls the tool early, calls it twice, or continues doing work afterward can still pass as long as the first call has the expected boolean. It would be stronger to assert exactly one call and no subsequent tool calls after it. -
The read-only safety contract is under-tested. The skill says the agent must not modify code, but the agentic evals enable built-in tools and
stayed_advisoryonly checks for flag-mutating MCP tools. A model could callEdit,Write, or a mutatingBashcommand and still pass the advisory assertion. I’d add a guard that bans write/edit tools and mutating shell commands for this suite.
should-flag-change: - New fixtures: ancestor-gate (agentic; explores to an enclosing flag not in the diff) and prerequisite-dependency (judgment). - risk-calibration asserts folded into the auth (expect high) and docs-only (expect low/absent) fixtures. All pass. New flag-release suite (adds match-release-policies / list-release-policies / create-automated-rollout-config tool defs + mocks; environmentKey mock replacement): - Release-on-merge happy path: previews the policy, records both envs. PASS. - Hold intent: KNOWN-RED tracked signal. sonnet-4-6 records the held env as `policy` (wrongly believing policy holds); opus omits it correctly. The assertion is correct (a held env must be omitted); do not weaken it. New flag-and-release-change suite (both PASS): - Plan phase is side-effect-free (no mutating MCP calls during planning). - Fails closed: a create-flag 403 (via the restricted-project mock hook) stops the run before any release is recorded. Wiring: manifest + package.json scripts for both new suites; READMEs for flag-release and flag-and-release-change. Sharpened flag-release's hold-honoring wording (helps stronger models; sonnet gap tracked above). 90/90 unit tests pass.
1. Catalog leaked third-party skills from evals/node_modules (dotenv, dotenvx, playwright) into the public skills.json, and CI's generate_catalog.py --check failed on the mismatch. Add node_modules to the validator's EXCLUDED_DIRS (generate_catalog reuses it) and regenerate skills.json — back to 44 skills, --check clean. 2. Enforce the recommend-flag contract. A new suite-wide verdict_contract assertion requires the tool to be called EXACTLY once and as the final tool call; a run that calls it early, twice, or keeps working afterward now fails instead of passing on the first call's boolean. 3. Guard the read-only safety contract. A new read_only_guard assertion bans Edit/Write and mutating shell commands (redirects, rm/mv/cp, git commit/push, package installs) across the suite, so the agentic tier can't modify code or state and still pass. Previously only flag-mutating MCP tools were checked. Both new assertions pass on all 10 should-flag-change fixtures.
build-pr-fixture.js required js-yaml via an explicit ../node_modules path, but js-yaml was only present transitively (via promptfoo). A promptfoo bump that dropped or relocated it would break the fixture builder and its unit test. Add js-yaml (^4.1.1, already the resolved version) to evals devDependencies, update the lockfile, and use a plain require. Also drop the now-unused node:path import.
3 tasks
…-change-eval # Conflicts: # skills.json
…nce (#108) * feat(feature-flags): enrich should-flag-change and add context-availability reference Layer portable, de-LaunchDarkly-internal lessons from our flag-planning skills onto the public feature-flag skills. should-flag-change: - Add an explicit, named user-observability test as a gate before any `recommend: false` verdict. - Add a `verdict` field (suggested | already-flagged | not-suited) to the recommend-flag output, keeping already-flagged (protected by an existing or ancestor gate) distinct from not-suited (nothing to flag). `recommend` stays the boolean a CI check keys on. - Generalize a decision-posture tie-breaker (conservative vs. low-overhead) for genuinely balanced calls, without internal "dogfood" wording. Add a new SDK-agnostic targeting context-availability reference: match the context kind to the surface where the flag is read (server/client/anonymous), key vs attribute, and rollout bucketing. Wire it into flag-targeting and flag-create, and reference it from should-flag-change. Bump versions and regenerate skills.json. Co-authored-by: Cursor <cursoragent@cursor.com> * test(feature-flags): cover verdict taxonomy and targeting-context reasoning - should-flag-change suite: assert the verdict field across the already-flagged, not-suited, and suggested fixtures, and validate the enum plus recommend/verdict agreement in the ambiguous fixture. - flag-create suite: add a fixture where a client-side flag is asked to target a server-only signal, asserting the agent flags that browser context can't carry it and suggests an available approach. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
The flag-release suite carries an intentionally-red fixture (Sonnet mishandles a held production environment) that must stay red until the skill or model closes the gap. Previously any suite below 75% failed the CI job, so this tracked signal blocked merge. Declare such fixtures via a knownRed allowlist in _manifest.js (matched by the fixture description, which promptfoo stores under testCase.description). aggregate.js now keeps the honest score/badge red while gating on a gateScore that excludes known-red fixtures, so only unexpected failures fail CI. A known-red fixture that starts passing is surfaced so its entry can be removed.
This reverts commit 5e87093.
Sonnet still records held production as `policy` (auto-releases on merge), so the intentionally-red hold-intent fixture keeps failing the 75% suite gate. Re-apply the gateScore allowlist so the tracked signal stays visible without blocking CI. Co-authored-by: Cursor <cursoragent@cursor.com>
The hold-intent fixture failed because the model maps "hold production" to `releaseType: policy` — the tool describes `policy` as "defer to the environment's release policy on merge", and "defer" reads like "hold". The skill fought this with prose caveats, which the authors noted hadn't closed the gap. Restructure the Implement phase around a forced RELEASE-vs-HOLD bucket sort: the environments array is built only from the RELEASE bucket, HOLD means absence from the array, and the "policy defers to the policy, not to you" misreading is called out at the point of the call. Remove the known-red gate allowlist so the fixture counts again and the skill fix carries the suite. Co-authored-by: Cursor <cursoragent@cursor.com>
…d-field hallucination Root-cause dig on the hold-intent fixture: the model doesn't fail from a bare reasoning gap — the eval's mock tool description diverged from the real hosted tool and fed it a trap. The real create-automated-rollout-config says policy "waits until merge, then performs the release"; the mock said policy "defers to the release policy", and "defer" reads as "hold", steering the model to keep a held env as policy. With louder omit-prose the model instead fabricated a holdUntil field — which the mock silently accepts (.passthrough), so it never learns the field is invalid. - Make the mock tool description faithful to the real tool: drop "defer", say "waits then performs", and state explicitly there is NO hold/notBefore/date field (omit the env to hold it; unknown fields are rejected). - Skill: name the exact tell — wanting holdUntil/notBefore/hold (or keeping an env as policy "so it waits") means the env is HOLD; drop the entry, don't invent a field. Date/reason go in the report. - Keep the known-red gate as a backstop since the 2-test suite is a binary gate that can flake even when the fix lands; the honest score still shows red if it regresses. Co-authored-by: Cursor <cursoragent@cursor.com>
…asses
The de-trapped tool description + skill hallucination-naming flips Sonnet to the
correct behavior: it omits the held environment (recorded [{staging,simple}],
production absent) and scores 0.83 > 0.75. With a real fix in place the known-red
gate tolerance would only mask a future regression, so remove it and let the
fixture gate on its own.
Co-authored-by: Cursor <cursoragent@cursor.com>
…ed-field fix Expand the flag-release suite from 2 to 5 fixtures, all exercising the release-vs-hold sort the earlier fix addressed: - add a no_fabricated_fields assertion (rejects holdUntil/notBefore/hold on any recorded env) to the hold fixtures, locking in the regression fix - multi-env mixed intent (dev+staging release, production hold) — the sort must scale past two envs - single-target date hold (the only env is held, so nothing releases) - fail closed on ambiguous intent (release staging, hold unconfirmed production) Retire the stale KNOWN-RED comment on the original hold fixture (now a passing regression guard). More fixtures also stabilize the 75% gate: a single stochastic miss on a 5-test suite stays green, where on 2 tests it went red. Co-authored-by: Cursor <cursoragent@cursor.com>
…nd create step The orchestrator suite only had plan-side-effect-free and fail-closed fixtures. Add two more targeting its own distinct, sandbox-reachable decisions (the record step needs a git push the MCP-only provider can't do, and is covered directly by the flag-release suite): - decide gate: a docs-only change is judged not flag-worthy and creates/records nothing, even when told to "flag and release if warranted" - create step: an approved change produces exactly one boolean kill-switch, created OFF, never toggled on by hand (over-flag + created-OFF guards) Co-authored-by: Cursor <cursoragent@cursor.com>
nieblara
approved these changes
Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A new advisory, read-only skill,
feature-flags/should-flag-change, that decides whether a code change should ship behind a LaunchDarkly feature flag — plus a two-tier promptfoo eval suite for it.It's invoked two ways: ad hoc ("should this be behind a flag?") and in CI on a PR, where it's fed the git diff and reads the surrounding source to make the call. It ends with a structured
recommend-flagverdict ({ recommend, confidence, reasons }) that CI can parse to post a check. It never creates or modifies flags.Contents
skills/feature-flags/should-flag-change/(SKILL.md + README + marketplace.json). Read-only scope boundary, a confusion-matrix decision framework (favor flags for user-facing/risky changes; weight false negatives over false positives), and therecommend-flagverdict contract.recommend-flagtool definition + mock; agit_diffprovider var injected as a<git_diff>tag; a two-tier suite (judgmentvsagenticproviders, targeted per fixture) with 5 labeled fixtures (2 positive, 2 negative, 1 ambiguous).evals/scripts/build-pr-fixture.js(npm run fixture:from-pr): builds a fixture from a real PR viagh pr diff+refs/pull/N/head, bootstrapping the expected label from LaunchDarkly SDK call sites in the diff._manifest.js, README skill list, regeneratedskills.json.Results
Full suite passes 5/5 at the 0.75 threshold. The two tiers behave as designed: agentic fixtures explore the scaffolded repo before deciding; judgment fixtures decide from the diff alone. One documented signal: the model reports
highconfidence on every fixture including the genuinely-borderline one, which the ambiguous rubric intentionally flags (that fixture scores 0.91). 70/70 unit tests pass.Test plan
cd evals && npm run eval:should-flag-change(requiresANTHROPIC_API_KEY)cd evals && npm testvia LD Research 🤖