fix(code-review, fd3): close the defects the console-* runs kept repeating - #14
Conversation
…en CI A toolchain report pinned the main checkout's absolute path as every command's cwd, so CI runners graded the root branch while reporting a stacked one, and a runner that sed-fixed a type error returned passed=true on an uncommitted tree.
… one output Two runs burned their context polling instead of ending the turn, one of them killing a working security lens with TaskStop; seven tailored conventions notes turned the brief into per-lens instructions; and a Scanner that spawned its own agent overwrote its own findings.
Six of twelve runs published arithmetic that added up over categories that did not: handoffs counted as merged into bullets that were never rendered, "0 boy-scout" over a block holding three of them, and primary findings dropping out of the merge uncounted entirely.
…ut by risk Five runs put a behaviour-changing fix in front of the user unexamined — a guard moved onto a DTO, a payload bounded ahead of redaction, a client split that dropped a submit guard — and the batched boy-scout bucket is what carried the worst of them.
… checkout The root branch's worktree was the main checkout, so every status flip dirtied the tree under test and the user's own uncommitted work rode along in the verdict.
All five review agents of one run invoked start-cr inside a workflow step, where no Agent tool exists; three degraded to a single inline pass without saying so.
… and access widening Runs rewrote files with sed and formatted whole repositories, lost their approved list to a compaction mid-walk, and had no rule for a private key sitting in prod state or for a contract permission quietly widened to org-wide read.
…iff already carries Runs skipped .mjs build scripts as tooling, graded a tests-e2e/ suite as source, and left the spec lens off on a branch whose own SPEC.md was in the diff.
…eam is itself git tracks a pushed feature branch against origin/feature, so the committed diff is empty and a review of the whole branch reported nothing to review.
…s that name what they share A run stopped on a blocked claim the spec had already assigned an owner, bundled a CODEOWNERS- guarded file into a feature branch, and serialised two same-branch tasks that shared nothing.
…s rewrote A pass returned ready while carrying blocked claims the split then refused, and a pass that edited the spec closed the loop on a document nothing re-read.
…able-cell prose A run lost its pre-command constraints and its carried-over question numbers to a compaction, and wrote spec tables whose cells had grown into paragraphs.
…e close A run composed a repair from a grep-shaped guess, asked its repair agent to validate alongside the workflow, and ended without saying where any branch's worktree lived.
… research/ The eval gate reads any other .md the grilling writes as a spec written before confirmation, and an unplaced ledger is also a file the next skill cannot find.
A fenced block reads as source to the merge step; the eval caught the scanner emitting one.
… verdict The eval suite caught both regressions from this batch: validate handed up gaps the spec already owned, and split grew an operational task per phase boundary where the spec names no gate.
… what a finding names
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 37539347 | Triggered | Generic Terraform Variable Secret | ab36f9a | plugins/code-review/evals/fixtures/iac-exposure.tf | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Nine compiled Python bytecode files ship in this PR, four of them for modules that have no source in the repository, inside a published marketplace plugin.
Reviewed changes — two independent plugins hardened in one pass after repeated console-* runs surfaced the same defects.
code-review: Scanner protocol tightened incommands/start-cr.md(waiting is ending your turn, truncated-<result>detection,Editmeans the tool), two new security rulesiac-exposureandaccess-widening(42 → 44 severity rows), scope widened to.mts/.cts/.mjs/.cjswith a CI-workflow skip sentence, analternate-base payload inscripts/get_changes.pyplus the plugin's first pytest suite, and evals 16–19 with four new fixtures.fd3:ready/blockedverdict semantics invalidate-spec, declared-gap and protected-path cut rules insplit-to-tasks, and CI-verdict integrity checks inworkflows/implement-run.js/repair-run.js—CI_RESULTnow requiresbranchanddirty, andciFaultdiscards a verdict from the wrong branch or a dirty tree.- Four new fd3 eval scenarios with three new fixtures (
gap-rollout-spec,ownerless-gap-payments-spec,protected-path-rollout-spec), wired intoreset-sandboxes.sh. - Neither plugin's
versionis bumped and bothCHANGELOG.mdfiles carry## [Unreleased]entries — correct for a feature PR under this repo's release script.
🚨 Compiled Python bytecode is committed, including bytecode for source that does not exist
Commit 2c89a3f added nine .pyc files (~265 KB) that ship inside the published code-review marketplace plugin. Four of them are bytecode for dispatch.py, pre_llm_pass.py, test_dispatch.py and test_pre_llm_pass.py — modules with no source anywhere in this repository. plugins/code-review/scripts/ contains only get_changes.py, and scripts/tests/ only test_get_changes.py. Users installing this plugin receive opaque binaries they cannot read or audit against any source.
Technical details
# Committed `__pycache__` bytecode, four files with no corresponding source
## Affected sites
- `plugins/code-review/scripts/__pycache__/dispatch.cpython-312.pyc` (35739 B)
- `plugins/code-review/scripts/__pycache__/dispatch.cpython-313.pyc` (35454 B)
- `plugins/code-review/scripts/__pycache__/pre_llm_pass.cpython-312.pyc` (24666 B)
- `plugins/code-review/scripts/__pycache__/pre_llm_pass.cpython-313.pyc` (25358 B)
- `plugins/code-review/scripts/tests/__pycache__/test_dispatch.cpython-312-pytest-9.0.3.pyc`
- `plugins/code-review/scripts/tests/__pycache__/test_dispatch.cpython-313.pyc`
- `plugins/code-review/scripts/tests/__pycache__/test_get_changes.cpython-314-pytest-9.1.1.pyc`
- `plugins/code-review/scripts/tests/__pycache__/test_pre_llm_pass.cpython-312-pytest-9.0.3.pyc`
- `plugins/code-review/scripts/tests/__pycache__/test_pre_llm_pass.cpython-313.pyc`
- `.gitignore` — has no `__pycache__/` and no `*.pyc` entry
## Required outcome
No compiled bytecode is tracked in the repository, and re-running the Python
tests locally cannot reintroduce it.
## Suggested approach
- `git rm -r --cached plugins/code-review/scripts/__pycache__ plugins/code-review/scripts/tests/__pycache__`
- Add `__pycache__/` and `*.pyc` to `.gitignore`.
## Open questions for the human
- `dispatch.py` and `pre_llm_pass.py` have no source here. Were they local
experiments, or are they meant to land and were left out of the commit?
If the former, the bytecode is pure accident; if the latter, the PR is
missing files.⚠️ validate-spec can no longer produce the ready + blocked verdict that split-to-tasks and the new fixture were taught to depend on
This PR rewrites both sides of the same contract in opposite directions.
validate-spec/SKILL.md:37-38 defines the buckets: deferred is "the spec declares the gap and names both an owner and a placement", blocked is "nothing settles it and the spec names no owner and no placement". The new section at :294-295 then says "The verdict is ready only when every claim is verified or deferred", and :302-303 says "A gap the spec already declares with an owner and a placement is deferred on sight". Taken together, a blocked claim is by definition ownerless, and an ownerless blocked claim forces not ready (:297-298). ready with blocked > 0 is unreachable.
But split-to-tasks/SKILL.md:46-50, rewritten in the same PR, asserts the opposite: "A ready verdict may carry a blocked claim when validation recorded it as a declared gap: the fact is unresolved and the spec names who resolves it." And the new fixture pins that state as its contract — gap-rollout-spec/spec/gap-rollout-spec.md:218 reads Verdict: ready — claims: 1 verified / 0 deferred / 1 blocked, while that same spec's section 7 (:161-165) declares the gap with "Owner: the platform team. Placement: a gate before phase 2", and :223 records it as "declared as a gap in section 7 with the platform team as owner and a gate before phase 2 as placement". By validate-spec's own definition that claim is deferred. gap-rollout-spec/DEFECTS.md:15 nonetheless calls the combination "load-bearing" and forbids the obvious repair.
The other two fixtures show the settled convention holds elsewhere: gap-payments-spec/DEFECTS.md:15 records the same owned-gap shape as deferred, and the new ownerless-gap-payments-spec correctly drives an unowned gap to blocked and not ready. gap-rollout-spec is the one that inverts it — so split-declared-gap is an end-to-end scenario whose precondition validate-spec can never actually emit.
Technical details
# `ready` + `blocked` is defined as unreachable by one skill and required by two others
## Affected sites
- `plugins/fd3/skills/validate-spec/SKILL.md:37-38` — bucket definitions
- `plugins/fd3/skills/validate-spec/SKILL.md:293-300` — "`ready` only when every claim is `verified` or `deferred`"
- `plugins/fd3/skills/validate-spec/SKILL.md:302-303` — owned+placed gap is "`deferred` on sight"
- `plugins/fd3/skills/split-to-tasks/SKILL.md:43-50` — precondition accepts blocked claims that are declared gaps
- `plugins/fd3/evals/fixtures/gap-rollout-spec/spec/gap-rollout-spec.md:161-165` — the gap, with owner and placement
- `plugins/fd3/evals/fixtures/gap-rollout-spec/spec/gap-rollout-spec.md:218,223` — the pinned verdict line and its claim row
- `plugins/fd3/evals/fixtures/gap-rollout-spec/DEFECTS.md:9-19` — declares the combination load-bearing
- `plugins/fd3/evals/lib/checks/split-declared-gap.mjs` — asserts `1 blocked` is quoted back
- `plugins/fd3/evals/fixtures/gap-payments-spec/DEFECTS.md:15` — the sibling fixture uses `deferred` for the same shape
## Required outcome
One vocabulary for a declared gap, used consistently by `validate-spec`,
`split-to-tasks` and every fixture, so that the verdict line
`split-to-tasks` is told to accept is a verdict line `validate-spec` can
actually write.
## Suggested approach
Two coherent resolutions; pick one and sweep it:
1. **A declared gap is `deferred`** (matches the bucket definitions and
`gap-payments-spec`). Then `split-to-tasks` reads the `deferred` bucket
for the operational task, its precondition drops the blocked-claim
language, and `gap-rollout-spec`'s verdict line becomes
`1 verified / 1 deferred / 0 blocked` — with the spec's own line count
rewritten to match, since `224` is pinned and verified by `wc -l`.
2. **A declared gap is `blocked` with an owner.** Then
`validate-spec:37-38` must stop defining `blocked` as ownerless, and
`:294-295` must read "`ready` when every claim is `verified`, `deferred`,
or a `blocked` claim that names an owner and a placement".
Resolution 1 is the smaller sweep and is already what two shipped fixtures
encode.
## Open questions for the human
- Was `gap-rollout-spec` authored against an intended-but-unwritten
`validate-spec` rule, or against the pre-PR wording?⚠️ The scope-mix fixture sits in the one directory name that overrides the classification it exists to test
promptfooconfig.yaml:785 points eval-19 at plugins/code-review/evals/fixtures/scope-mix, and its first rubric requires that build.mjs and legacy-report.cjs be "reviewed as source files". But references/scope.md:56-57 — the authoritative file-kind list, which this PR edits — opens the test kind with "any file under one of these directories: __tests__/, test/, tests/, spec/, e2e/, cypress/, fixtures/, …". Every file in the fixture is under fixtures/, so a scanner faithfully applying scope.md classifies all of them as test.
This matters because eval-19 is the only guard on the .mjs/.cjs-as-source rule this PR adds at scope.md:16-17. Either the grader marks a correct test classification as a failure, or it is lenient and the eval passes without exercising the rule. Either way the new rule has no reliable guard.
Technical details
# eval-19's fixture path makes every file in it `test` kind under the rule being tested
## Affected sites
- `plugins/code-review/evals/promptfooconfig.yaml:782-804` — eval-19 and its four asserts
- `plugins/code-review/evals/promptfooconfig.yaml:785` — `file: plugins/code-review/evals/fixtures/scope-mix`
- `plugins/code-review/references/scope.md:56-57` — `test` kind includes anything under `fixtures/`
- `plugins/code-review/references/scope.md:16-17` — the `.mjs`/`.cjs`-as-source rule eval-19 is meant to guard
- `plugins/code-review/evals/fixtures/scope-mix/build.mjs`
- `plugins/code-review/evals/fixtures/scope-mix/legacy-report.cjs`
## Required outcome
eval-19 fails when the `.mjs`/`.cjs`-as-source rule regresses and passes
when it holds, with no dependence on how leniently the grader reads
"reviewed as source files".
## Suggested approach
The other eval fixtures are single files whose own names carry no kind
signal, so the `fixtures/` parent never mattered before; a directory
fixture is the first case where it does. Either:
- give eval-19 a fixture root outside `fixtures/` (the config already
resolves `path:` values relative to itself, so the move is local), or
- make the rubric assert the classification the plugin should actually
reach for a file at this path, rather than "source".
## Open questions for the human
- Is `fixtures/` in the `test`-kind list intended to be this broad — it
also captures every other plugin's eval fixture tree — or was it meant
for application-level fixture directories only?ℹ️ Neither the new Python test suite nor the code-review evals run in CI
scripts/tests/test_get_changes.py is this repo's first pytest suite, and it is a real one — it builds a bare clone and a tracking working clone per test rather than mocking git. Nothing runs it. .github/workflows/lint.yml runs shellcheck over *.sh and nothing else; .github/workflows/fd3-evals.yml is scoped to plugins/fd3/**. The four new code-review evals are likewise local-only.
The fd3 side is not the gap — fd3-evals.yml:34 runs a smoke group of one scenario per skill, and the four new fd3 scenarios are additional per-skill variants, so leaving that filter untouched is correct.
Technical details
# The `get_changes.py` regression suite has no CI job
## Affected sites
- `plugins/code-review/scripts/tests/test_get_changes.py` — new, 87 lines, never executed by CI
- `.github/workflows/lint.yml:16` — `find . -name '*.sh' … | xargs shellcheck`, the only lint job
- `.github/workflows/fd3-evals.yml:8-13` — `paths:` is `plugins/fd3/**`, `scripts/run-evals.sh`, `package.json`
## Required outcome
A change to `get_changes.py` that breaks base resolution or the
`alternate` payload fails a check on the pull request.
## Suggested approach
A job gated on `plugins/code-review/scripts/**` that installs `pytest` and
runs it against `plugins/code-review/scripts/tests/`. The suite shells out
to real `git` and sets `user.email`/`user.name` per repository itself, so a
stock `ubuntu-latest` runner should suffice.
## Open questions for the human
- Is the absence deliberate — Python being a one-script exception in a
repo whose stated script preference is Bash > Node.js > Python?ℹ️ Nitpicks
-
README.md:82-84and:126-128— the README still enumerates the security lens as five categories ("Secrets in source, injection sinks, missing access checks, unvalidated boundaries, and insecure settings"), omitting the two this PR adds.severity.md:71was correctly updated to "the six security rules" (six of the seven gradehigh;unvalidated-boundaryismedium), so only the README prose lags. -
.claude-plugin/plugin.json:4and.claude-plugin/marketplace.json— the shared description carries the same five-category security list and says "a spec lens via--spec <path>", which this PR widens to also activate from a spec-shaped file in the diff. The two files agree with each other, so the marketplace mirror is intact; both are just stale. Neither file is in this diff. -
docs/adr/0002-active-lens-set-and-standards.md:11and:15-16— still read "specis active only when the user passed--spec <local path>" and "the change and the--specflag decide". The ADR's Consequences section was amended in this PR for the 42 → 44 change (:72-73), so the same amend-in-place convention would cover the spec-lens gate. -
scripts/tests/test_get_changes.py:75—test_no_alternate_when_the_resolved_base_already_sees_the_changeopens withassert "alternate" in out. The second phase does assert its absence, but the name describes only half the test.
Claude Opus ｜ 𝕏
Nine __pycache__ files landed with the code-review scripts, four of them for local modules that were never committed.
…ords validate-spec buckets an owned, placed gap as deferred, so a ready verdict carrying a blocked claim was unreachable; split-to-tasks and the gap-rollout-spec fixture now use the same vocabulary.
|
@pullfrog thanks for the review. Every finding is addressed in 981333b..3cd82d9:
Separately, the |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the eight commits since the prior pullfrog review at 0174deb. Each closes a finding from that review; I verified every one against the working tree rather than against the commit messages.
-
Untracked the committed Python bytecode —
git ls-files -- '*.pyc' '*__pycache__*'now returns empty, and.gitignorecarries__pycache__/and*.pyc. The open question is answered too:dispatch.pyandpre_llm_pass.pywere local experiments, so no source was missing from the PR. -
Made
ready+blockedreachable by retiring it — resolution 1, swept end to end. A declared gap is now adeferredclaim everywhere:split-to-tasks/SKILL.md:43-51requires the verdict line to count noblockedclaim and turns eachdeferredgap into an operational task,gap-rollout-spec.md:218and itsDEFECTS.md:13read1 verified / 1 deferred / 0 blocked, andsplit-declared-gap.mjs:34asserts/1 deferred/.validate-spec/SKILL.mdneeded no edit — it already defineddeferredas the owned-and-placed bucket, which is what the sweep aligned everything else to. The pinned224still equalswc -lon the spec, and the two counted claim rows at:222-223match the new counts. -
Moved the
scope-mixfixture out offixtures/— now atplugins/code-review/evals/scope-mix/, whose ancestors match none ofscope.md'stest-kind directory clauses, sobuild.mjsandlegacy-report.cjsclassify assourceand eval-19 can actually fail when that rule regresses.tests-e2e/checkout.spec.tsstill lands ontest(the directory name containse2e, and the filename matches*.spec.*),notes.txtand.github/workflows/ci.ymlstill skip, so all four asserts match whatscope.mdmechanically produces. No stale reference to the old path remains. -
Added a pytest job for
get_changes.py—code-review-scripts.ymlis gated onplugins/code-review/scripts/**plus its own path. I ranpython -m pytest plugins/code-review/scripts/testson a stock runner: 3 passed, no git identity configuration needed, so the workflow's premise holds. -
Swept the stale docs and the prompt — README,
plugin.jsonandmarketplace.jsonnow list all seven security rules and the spec-file offer (the two descriptions remain byte-identical to each other), ADR 0002's Decision paragraph is amended in place for the spec-lens gate,prompts/security.txtno longer assumes a single path, and the alternate-base test is renamed to cover both of its phases. The count vocabulary still reconciles across sites:severity.mdcarries 44 rows and itshighparagraph says "the six security rules", correctly counting the high-severity subset of the seven —unvalidated-boundarystaysmedium.
On the scanner alert: evals/fixtures/iac-exposure.tf:12 is a synthetic literal (Sup3rS3cret-prod!) sitting alongside a deliberately public allUsers IAM binding and an exported admin password — it is the planted defect security · iac-exposure has to catch, so the alert is a true positive on pattern and a false positive on risk. Suppressing it at the path rather than at the value keeps the fixture readable as the contract it is.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Re-reviewed the 8 commits added since the last review (0174deb → 3cd82d9). Every finding from the prior review is closed, and I verified each fix rather than taking the commit message for it.
Prior findings, and what closes them
| Finding | Commit | Verification |
|---|---|---|
.pyc files committed |
981333b |
.gitignore covers __pycache__/ and *.pyc; all nine tracked files deleted, and no __pycache__ path remains in the tree |
| No CI for the new pytest suite | be32219 |
code-review-scripts.yml path filter covers plugins/code-review/scripts/**, which contains tests/; SCRIPT resolves via parents[1], so the repo-root invocation finds get_changes.py. The workflow's claim that no git config is needed holds — the origin_repo fixture sets user.email/user.name in both the seed and the work clone before any commit |
eval-19 fixture classified as test |
bd62040 |
Checked against the widened rule in scope.md: no ancestor of evals/scope-mix/ contains e2e or ends in -test/-tests, so the relocation actually changes the classification — while tests-e2e/ inside it still resolves to test |
security.txt phrased for a single file |
28ff654 |
Now "read every file in {{file}} … each path is a separate file", so eval-18's two-path file var no longer depends on the scanner splitting the string |
| Test name contradicted its first assertion | 61d3017 |
Renamed to test_alternate_appears_until_the_resolved_base_sees_the_change, which covers both phases |
| Docs lagged the two new security rules | 3198e35 |
README, CONTEXT, ADR-0002, plugin.json and marketplace.json all carry iac-exposure / access-widening and the spec-lens offer, with the two descriptions byte-identical |
The two net-new commits
577e350(fd3-evals.yml:ANTHROPIC_API_KEY→CLAUDE_CODE_OAUTH_TOKEN) — I checked this one specifically, since a wrong swap would silently break the only eval CI. Nothing downstream readsANTHROPIC_API_KEY: all three fd3 promptfoo configs useanthropic:claude-agent-sdkwithapiKeyRequired: false,run-evals.shnever references it, and the only other mentions are comments in the code-review evals, which this workflow does not run. The token is already an established secret in this repo (pullfrog.yml). Sound.3cd82d9(fd3blocked→deferredre-vocabularisation) — the sweep is complete and self-consistent across all five restating sites:split-to-tasks/SKILL.md(precondition + theready-counts-no-blocked-claim line),validate-spec/SKILL.md,split-declared-gap.mjs(/1 deferred/), thegap-rollout-specfixture and itsDEFECTS.md, and the CHANGELOG. The fixture stays coherent end to end: it declares the gap with owner platform team and placement a gate before phase 2, its verdict line readsready — 1 verified / 1 deferred / 0 blocked — spec 224 lines,DEFECTS.mdpins the same string, and the spec is still 224 lines after the edit — so the checker's/platform team/iand/1 deferred/assertions both have something to match.
One thing I looked at and cleared rather than raising: the new deferred (owner and placement) / blocked (neither) definitions leave a half-case — a gap with an owner but no placement — in neither bucket. validate-spec/SKILL.md:289-290 catches it ("a claim you cannot settle before reporting becomes blocked"), so it is not a gap in the logic. That text is also base-PR content, not part of this delta.
Both inline threads from the previous review were already answered and resolved by the author.
Claude Opus ｜ 𝕏
The rollout fixtures had API-1 write through an internal ledger endpoint that nothing built or cited, and a split run rightly stopped on that coverage gap.
… cite The fixture's prerequisites, apply mechanism and delivery path pointed at a manifest, a /metrics endpoint, a CI deploy and queue wiring the repository never had, so a faithful validation raised them next to the five planted defects.
There was a problem hiding this comment.
ℹ️ No blocking issues — one fixture-consistency nit inline, plus a coverage note.
Reviewed changes — the two commits since the prior pullfrog review at 3cd82d9. Both are fd3 eval-fixture work; no plugin logic, no code-review files, no CHANGELOG or version surface is touched.
- Gave API-1's ledger write an endpoint a work item actually builds (
92e2838) — API-2's contract and its section 6 work item now coverPOST /ledger/entriesacross all five rollout specs. Previously API-1 wrote "via the ledger service's internal write endpoint", which no element defined, leaving the frozen split's API-1 → API-2depends-onedge unmotivated by anything in the spec.rollout-spec/DEFECTS.md:16-18records why that endpoint is now load-bearing, and the fixtures that defer to it by reference (gap-,protected-path-, and the two derived ones) inherit the note correctly. - Backed
defective-payments-spec's clean claims with the files they cite (8acc50d) — addsserver.ts,http/merchantAuth.ts,metrics.ts,db.ts,orders/repository.ts,queue/poller.ts, the orders migration,deploy/manifest.yaml,.github/workflows/deploy.ymlandfixtures/charge.json, and makesqueue/worker.ts'spostreally call the merchant endpoint so a failing merchant is reachable. - Verified the fixture contracts rather than trusting the commit messages — every
DEFECTS.mdline pin still resolves (charge.tsexactly 30 lines withgetIdempotencyKeyat 16 andproviderTimeoutMsat 27;idempotency.tsnew Mapat 3, functions at 5 and 9;enqueue.tsenqueueWebhookstill at 8 afterdrainQueuewas appended below it;worker.tscap at 3, retry loop at 6). All five pinned spec line counts are unchanged (216 / 224 / 226 / 207 / 237), the threesplit-shared.mjssentinels are still present in every rollout variant, anddiffconfirmsorphan-andunvalidated-rollout-specare still byte-exactly their documented derivation ofrollout-specdespite being edited in place rather than regenerated.reset-sandboxes.shusesrsync -a "fixtures/<f>/" "$dest/", so the new.github/dot-directory does reach the sandbox.
ℹ️ Four of the five fixtures 92e2838 rewrote have no CI coverage, and the PR's live-eval evidence predates both commits
fd3-evals.yml:34 runs the smoke filter validate-defective|split-baseline|write-missing-input|grill-round|build-spec-gate. That covers the two riskiest edits in this delta — defective-payments-spec (via validate-defective-spec) and rollout-spec (via split-baseline) — but not gap-rollout-spec, protected-path-rollout-spec, orphan-rollout-spec or unvalidated-rollout-spec, whose spec bodies 92e2838 also rewrote. The PR body's "both suites were run live and every failure triaged against a baseline worktree at 72c4451" describes the tree as of 3cd82d9, so the four local-only scenarios consuming those fixtures have not been exercised since. I checked the pins and the derivations statically and they hold, but an element-contract rewrite is exactly the kind of change that moves an LLM-graded split.
Technical details
# Fixture edits landed outside the CI smoke group without a matching local run
## Affected sites
- `.github/workflows/fd3-evals.yml:34` — smoke filter covers `validate-defective` and `split-baseline` only
- `plugins/fd3/evals/fixtures/gap-rollout-spec/spec/gap-rollout-spec.md` — API-2 contract rewritten; consumed by `split-declared-gap`
- `plugins/fd3/evals/fixtures/protected-path-rollout-spec/spec/protected-path-spec.md` — same rewrite; consumed by `split-protected-path`
- `plugins/fd3/evals/fixtures/orphan-rollout-spec/spec/rollout-spec.md` — same rewrite; consumed by `split-orphan-element`
- `plugins/fd3/evals/fixtures/unvalidated-rollout-spec/spec/rollout-spec.md` — same rewrite; consumed by `split-unvalidated-precondition`
## Required outcome
Confidence that the four non-smoke split scenarios still pass against the rewritten
API-2 contract, before this branch merges.
## Suggested approach
- `scripts/run-evals.sh fd3 "split-declared-gap|split-protected-path|split-orphan-element|split-unvalidated-precondition"`
and triage from `plugins/fd3/evals/.results/latest.json`.
- Do not widen the smoke filter for this — per the suite's own convention it is one
scenario per skill, and these are additional `split-to-tasks` variants.
## Open questions for the human
- Were these four re-run locally after `92e2838`? If so this is already closed and
needs nothing.Claude Opus ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No blocking issues in this delta — one carry-back finding inline, plus a nit.
Reviewed changes — the four fd3 commits since the prior pullfrog review at 8acc50d: 7 insertions, 4 deletions, all prose in three files plus one fixture line. No code-review files, no version surface.
-
Made the split reply carry all six columns (
bdd125d) —split-to-tasks/SKILL.md:296now says the conversation gets "the same table — all six columns,elementsincluded". I checked the count rather than trusting it: the enumeration two lines up is(slug, repository, branch, phase, depends-on, elements), andsplit-baseline.mjs:15-25'shasReportTablerequires one header row matching all six ofelements,slug,repo,branch,phaseanddepends-on.split-baselineis in thefd3-evals.ymlsmoke filter, so this fix lands with a guard that can actually fail. -
Stopped
grill-topicholding a whole round for a lookup (819532d) —grill-topic/SKILL.md:80gains "Never hold a round back to keep it whole: post the unblocked questions and name the blocked numbers on the round's opening line." That agrees with the pre-existing:43("a blocked question keeps its number and stays out of the round's numbered items; name it on the line that opens the round") rather than competing with it — the defect was the model holding the round despite:43, so restating it at the dispatch site is the right shape of fix. -
Sent the merchant key in the defective spec's API-1 probe (
38688e3) — closes theserver.ts:13thread, which I've replied to and resolved.$MERCHANT_API_KEYresolves end to end:merchantAuth.ts:5readsprocess.env.MERCHANT_API_KEYanddeploy/manifest.yaml:28sets it, which is exactly the envDEFECTS.md's "backs every claim" paragraph already promises. One line replaced by one, so the spec's line count and everyDEFECTS.mdline pin are untouched. -
Logged both fd3 fixes under
[Unreleased]→Changed(b58c137) — both bullets match what the commits actually do, and neither plugin'sversionmoved, which is correct for./scripts/release.sh.
ℹ️ Nitpicks
plugins/fd3/skills/split-to-tasks/SKILL.md:296— the edit left the line at 137 characters, where every other prose line in the file sits between 86 and 98. Re-wrapping the paragraph keeps future diffs on this section line-scoped instead of rewriting the whole sentence each time.
Claude Opus ｜ 𝕏
|
@pullfrog local run of the four non-smoke split scenarios against the rewritten fixtures (92e2838, 38688e3):
|
|
Thanks — the diagnosis holds up on inspection, and it pointed at one site the fix missed. On the run results. All three commits are on the branch ( What the fix missed.
Same defect, same skill family. Its step 1 is "Read the graph: parse task frontmatter", so the first action is necessarily a tool call and the instruction to withhold it is straightforwardly wrong. It is also the more dangerous of the two, because there is no Pushed Checked the rest for the same shape:
|
…y too 2ad21ea fixed split-to-tasks; implement-tasks carried the identical wording and has no eval scenario that would catch it.
There was a problem hiding this comment.
ℹ️ No blocking issues in this delta — one sibling-site suggestion inline.
Reviewed changes — the single commit since the prior pullfrog review at 38688e3: 2ad21ea, 7 insertions / 4 deletions across two files, prose only. No code-review files, no eval surface, no version surface.
-
Bound the split's opening checklist to a tool call in the same reply (
2ad21ea) —split-to-tasks/SKILL.md:62-66replaces "Post this checklist as your first message in the run, before any tool call" with "Open your first reply with this checklist … and make the first tool call in that same reply. A reply that only announces the checklist, or only posts it, ends the turn with nothing done." The rest of the paragraph (repost in full before every user interaction and at the close) is carried over unchanged, and the new wording does not collide with the Precondition's stop-before-step-1 path — evaluating that precondition is itself a read, so the first reply still has a tool call to make. -
Logged it under
[Unreleased]→Changed(plugins/fd3/CHANGELOG.md:16-17) — the bullet matches what the commit does, sits in the right section, and neither plugin'sversionmoved, which is correct for./scripts/release.sh. -
Checked what guards it — nothing does. No fd3 eval check or prompt references the workflow checklist or first-message shape (
grep -i "checklist|first message"overplugins/fd3/evalsis empty), so this fix lands unguarded. Not raised as a finding: a deterministic fixture for turn shape is the same infeasible thing that was cleared forgrill-topic:80in the prior review.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the single commit pushed while the previous review was being written (2ad21ea → 2977b86), which closes that review's only finding.
-
Mirrored the checklist rule into
implement-tasks—implement-tasks/SKILL.md:36-40now carries the same four sentences assplit-to-tasks/SKILL.md:62-66("Open your first reply with this checklist … and make the first tool call in that same reply. A reply that only announces the checklist, or only posts it, ends the turn with nothing done"). I diffed the two paragraphs on disk rather than from the range-diff: the only divergence is each skill's own interaction list — "each report round" here, "the report" there — which is correct, sinceimplement-tasksreports per wave. Its step 1 (Read the graph) opens with tool calls, so the rule has something to bind to on turn one. -
Widened the
[Unreleased]bullet to name both skills —plugins/fd3/CHANGELOG.md:16-17reads "split-to-tasksandimplement-tasksmake their first tool call in the same reply as the opening checklist", which restores the joint framing the released entry at:273already used for this rule. No version moved, correct for./scripts/release.sh. -
Confirmed the sweep is complete —
split-to-tasksandimplement-tasksare the only two fd3 skills that state a workflow checklist at all;grill-topic,validate-specandwrite-spechave no such paragraph, so there is no third site left stale.
Claude Opus ｜ 𝕏
…pec gate check The check read only the final message, so a run that put round 1 to the user through the tool, then ended waiting on a lookup, failed as if no grilling ran.
There was a problem hiding this comment.
ℹ️ No blocking issues in this delta — one narrowing inline, plus a scope question on the sibling checks.
Reviewed changes — the single commit since the prior pullfrog review at 2977b86: dec9ad5, one file, plugins/fd3/evals/lib/checks/build-spec-gate.mjs. No skill prose, no fixture, no CHANGELOG or version surface.
-
Let the gate check see a round that went out through the question tool — the assert previously demanded a numbered question in the model's final message. With
ask_user_question: behavior: first_option(promptfooconfig.yaml:17-18) a round is auto-answered and the run continues, so the final message is a later artifact and the grilling half reads as never having run. The check now also accepts a matchingAskUserQuestionentry from the provider's tool telemetry. -
Verified the provider contract rather than trusting the shape —
node_modulesis not installed here, so I checked it against promptfoo's published docs.metadata.toolCallsis real onanthropic:claude-agent-sdkand each entry carries exactlyid,name,input,output,is_error,parentToolUseId; the docs' own example iscontext.providerResponse?.metadata?.toolCalls || []filtered ont.name, which is the pattern used here verbatim. The(output, context)signature andcontext.providerResponseare likewise documented forfile://*.mjsjavascript asserts.evals/CLAUDE.md:43-44's warning is aboutmetadata.skillCalls, a different field that stays empty for plugin skills — it does not apply totoolCalls, which is populated for every tool call in the session. -
Confirmed the widening cannot leak past the gate — only the "did the grilling half run" assert moved. The gate itself (
specFiles.length === 0anddiff.modified.length === 0) is untouched and still on-disk. Accepting anAskUserQuestioncall as evidence is safe in this scenario specifically:commands/build-spec.md:12makes the user's confirmation the gate between stages, so if stage 2 had started the on-disk assert would fail anyway. -
The regex refactor is behaviour-preserving —
output.match(/…/gm).length >= 1becameNUMBERED.test(output)with the pattern hoisted to a module const. It has nogflag, so there is nolastIndexcarry-over between the two.test()call sites.
ℹ️ Two sibling checks still assume a round can only reach the final message
The premise this commit establishes — a round can leave through AskUserQuestion, get auto-answered, and put nothing numbered in the final message — holds identically for grill-round-shape.mjs:7-8 and grill-numbered-questions.mjs:7-8, which both scan output alone with the same regex.
grill-round-shape is the sharper of the two, because the contradiction is inside one test: its llm-rubric at promptfooconfig.yaml:163-167 says "A single question-tool round is acceptable", while the deterministic assert four lines above fails that same run for having "fewer than 2 numbered questions". It is also the one that runs in CI — fd3-evals.yml:35's filter matches grill-round-shape but not grill-numbered-questions.
Both are pre-existing on main, so this is a scope question for you rather than a regression in this delta: is the tool path something the whole grill group should tolerate, or is build-spec-gate special because it only needs to know the half ran?
Technical details
# The AskUserQuestion blind spot was closed in one of three checks
## Affected sites
- `plugins/fd3/evals/lib/checks/grill-round-shape.mjs:7-8` — `output.match(…)` only; requires `>= 2` numbered questions
- `plugins/fd3/evals/promptfooconfig.yaml:163-167` — the same test's rubric sanctions "a single question-tool round"
- `plugins/fd3/evals/lib/checks/grill-numbered-questions.mjs:7-8` — `output.split('\n').findIndex(…)`, same assumption
- `.github/workflows/fd3-evals.yml:35` — `grill-round` matches `grill-round-shape`, so that one fails CI on a tool round
- `plugins/fd3/evals/lib/checks/build-spec-gate.mjs:10-14` — the only site swept by this commit
## Required outcome
One decision about the question-tool path, applied consistently across the grill
group — either every check that grades "a round happened" reads the tool
telemetry, or `grill-round-shape`'s rubric stops sanctioning a shape its own
deterministic assert rejects.
## Suggested approach (optional)
If the answer is "tolerate it everywhere", the detection is worth a named helper in
`lib/helpers.mjs` (e.g. `askedQuestions(context)` returning the flattened
`AskUserQuestion` payloads) rather than a third copy of the `toolCalls` walk.
## Open questions for the human
- Is `grill-round-shape` meant to enforce that the round is *printed* as markdown
rather than delegated to the tool? If so its rubric is the thing to fix, not the
assert.ℹ️ Nitpicks
plugins/fd3/evals/CLAUDE.md:41-44— the Assertions section still reads "Assert on artifacts on disk vialib/helpers.mjs…, never on skill telemetry". This commit makesbuild-spec-gatethe suite's first assert to read provider telemetry, and the stated reason for the rule (metadata.skillCallsbeing empty for plugin skills) does not apply totoolCalls. A contributor reading that path-scoped rules file literally would flag this pattern as against house rules; one clause naming the exception would settle it.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No blocking issues in this delta — one sweep-completion note inline.
Reviewed changes — the two commits since the prior pullfrog review at dec9ad5, both in plugins/fd3/evals/lib/checks/. No skill prose, no fixture, no CHANGELOG or version surface.
-
Took the
build-spec-gatenarrowing (ac6e253) —askedis now a bare.some((call) => call.name === 'AskUserQuestion')instead of matchingNUMBEREDagainst the tool payload'squestionfield, closing the residual false negative for a tool round the model did not self-number.NUMBEREDis still live at:13viaNUMBERED.test(output), so the hoisted const has not become dead. The added comment ("Numbering isgrill-numbered-questions' concern; here the call itself is the evidence") states the division of labour accurately:grill-numbered-questions.mjs:7-8andgrill-round-shape.mjs:7-8are the two checks that genuinely grade numbering shape, and neither is weakened by this. -
Documented why the prior-conversation assert grades placement only (
d5d9dc4) — I checked the claim rather than the comment.prompts/grill-session-files.txtis the single line/fd3:grill-topic notes/topic.md, andgrill-topic/SKILL.md:19conditions the record on "whatever was established before this skill was invoked". With a bare command nothing was established, so an emptypriorarray is the correct outcome andprior.every(...)returningtrueon it is intended, not vacuous. That settles the open question from the earlier review on this line.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the single commit since the prior pullfrog review at d5d9dc4: 26056cf, 11 insertions / 9 deletions across three files, all under plugins/fd3/evals/lib/. It closes that review's only finding. No skill prose, no fixture, no config, no CHANGELOG or version surface.
-
Extracted the tool-round liveness check into a shared helper —
helpers.mjs:166-168gainsaskedThroughTool(context), byte-identical in behaviour to the walkac6e253had inlined inbuild-spec-gate(context?.providerResponse?.metadata?.toolCalls || []filtered oncall.name === 'AskUserQuestion'). Both call sites now readNUMBERED.test(output) || h.askedThroughTool(context)against the same failure message.NUMBEREDis still live atbuild-spec-gate.mjs:9, so the hoisted const did not become dead. -
Widened
grill-session-filesto the same shape — the signature is now(output, context)and:9-10accepts a round that left through the question tool. I checked the regex rewrite is behaviour-preserving rather than assuming it:output.match(/…/gm) || []with.length >= 1and/…/m.test(output)agree on exactly the same inputs, and the literal is constructed fresh per call either way, so droppinggcarries nolastIndexrisk. The widening also cannot mask a real failure — liveness is its ownc.check, and the ledger/placement asserts at:16-32are untouched and still graded on disk, so a run that delegates its round but never writesnotes/question-ledger.mdstill fails on the assert that actually matters. -
Confirmed the sweep stops in the right place —
grill-round-shape.mjs:7-8requires>= 2numbered items andgrill-numbered-questions.mjs:7-16grades where the first numbered item sits relative to the preamble. Both grade numbering as their subject, not as a liveness precondition, so leaving them strict is coherent with the comment atbuild-spec-gate.mjs:8and with the answer given in the resolved thread.grepover the tree confirmsaskedThroughTool/toolCallsnow appear in exactly the three expected places and nowhere else. -
Checked for stale references to the old assert — the failure string changed from "no numbered round of questions" to "no round of questions" in both files. Nothing else in the repository quotes either string; no
DEFECTS.md, rubric orCLAUDE.mddepends on it.grill-session-filesis wired astype: javascript/file://lib/checks/grill-session-files.mjs(promptfooconfig.yaml:186-188), which is the signature form that receivescontext, and itsmax_budget_usd: 8.0is the budget the grill group already carries forfirst_optionauto-answered rounds — the exact scenario this fix unblocks.
Review 8's thread on grill-session-files.mjs:9-10 was already replied to and resolved by the author, so there was nothing to retire this run.
Claude Opus ｜ 𝕏
1 participant
Why
An analysis of the
code-reviewandfd3runs executed in~/vsf/console-*on the currentplugin version (12 CR runs and one fd3 chain; older runs excluded by the install date of HEAD
72c4451) surfaced defects that recurred across runs rather than one-off misbehaviour. This branchcloses them in the plugins and adds eval scenarios so they cannot come back unnoticed.
What changed
code-review
of its own, and its final message is its whole output; the conventions note is one byte-identical
suppress-only text in every brief.
Reconciliationcounters are bound to the rendered blocks they check against, and aP primary droppedterm was added.Edit, theformatter runs only on files the review touched, boy-scout extras are sorted by risk.
securityrules —iac-exposureandaccess-widening(44 rows in the master table).get_changes.pyreports analternatebase, so a branch pushed to its own remote counterpart nolonger reads as "nothing to review" (first pytest suite for that script).
.mjs/.cjs/.mts/.cts,e2edirectories,.txtskipped, CI workflowsskipped with a security sentence);
start-croffers thespeclens when the diff carries aspec-shaped file.
commentsR4 resolves a spec-id-shaped token against the code before stripping it;Not flaggedis required whenever a look-alike was cleared.
fd3
split-to-tasksaccepts areadyverdict carrying a declared gap (owner + placement) andturns it into an operational task; an ownerless blocked claim still stops the split.
CODEOWNERS, branch protection, required review) is cut into a delivery taskon its own branch, so one external approval no longer holds a whole landing unit.
validate-specreturnsreadyonly when every claim isverifiedordeferred, sends itsreport together with any question batch, and edits only what a finding of that pass names.
commit — and a runner that edits its way to green no longer produces a pass.
grill-topickeeps a question ledger and writes pre-command facts to the research directory, soa compaction no longer costs the session's bookkeeping.
Evals
Both suites were run live and every failure triaged against a baseline worktree at
72c4451, whichseparates a regression from a standing failure. Result: all standing failures traced to fixtures,
none to a rule — each was fixed by tightening the fixture, never by lowering an assert.
Eight new scenarios guard this round's defects:
eval-16iac-exposure recall,eval-17access-widening over a unified diff (new scannertrack),
eval-18a guard whose weak tests must route totests· test-fidelity rather than asecurity high,
eval-19scope classification over a mixed tree.split-declared-gap,split-protected-path,validate-ownerless-gap,grill-session-files,with three new fixtures and their
DEFECTS.mdcontracts.One harness false positive was fixed along the way:
split-shared.checkBoundariesfroze "DB-1 hasno
depends-on" fromrollout-spec, which a fixture whose build order puts CI-1 first legitimatelybreaks. It now takes
{ precedesDb }, validated offline against the saved run artifacts.Not in this PR
No version bumps — both CHANGELOGs carry their entries under
[Unreleased]; releasing stays with./scripts/release.sh.