feat(review): spec lens, spec-source ladder, mode-scoped preflight in quality-gate - #3036
Conversation
… quality-gate Closes #2937 Lane D of the Pocock shipping-lifecycle absorption (#2933), implementing the design locked on #2937 after two-validator adversarial audit. - New 9th quality-gate mode, `spec`, in context/spec.md. It OWNS the finding-class enum (missing / scope-creep / wrong); self.md's fenced worker checklist degrades to a shallow divergence check that defers classification, and the pointer to the owning file sits in orchestrator-facing escalation text — not inside the subagent template, which a fresh-context read-only worker cannot act on. - C14 spec-source discovery ladder, with the three corrections the audit forced: item body text comes from the provider-mechanic read (the seam's normalized item object has no `body` field; get-item supplies identity and parent_id), a harvested bare `#N` is promoted to the qualified <provider>:<owner>/<repo>#<number> form before use, and the contract-slice rung keys on the topic slug rather than the lossy branch slug. The cross-plugin reach into work-items is presence-gated with an explicit seam-absent degradation, and item text is read under the item-content-trust boundary. Post-prune emptiness of the contract slice is recorded as a limit. - C15 preflight ported from fanout, mode-scoped (criteria is a reference mode and stays exempt) with the allowed-tools widening it needs to not stall headless. - C13: self.md's parallel-worker split tightened to keep-separate presentation, and "axis" recorded once in context/severity.md as severity/confidence — a perspective is a lens. - C12-C16 verdicts recorded in docs/upstream/aihero-shipping-course.md; container close-out review noted as split out to #3027. Collateral: README mode list, frontmatter description + argument-hint, three evals (ladder/body-read, skip-rather-than-fabricate, preflight mode scoping). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
|
Claude finished @kyle-sexton's task in 3m 43s —— View job Security review complete ✅Task list
Reviewed the diff ( IMPORTANT — harvested issue ref is shelled into
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84b66b5b65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Last security-reviewed head: |
|
Claude finished @kyle-sexton's task in 6m 20s —— View job Claude finished the CI code-review lane (
|
…, --repo scoping, ref validation Four findings from the PR's automated review lanes, each verified against the repo before acting: - P1 untracked-only (SKILL.md preflight). The port copied fanout's untracked-only STOP, but the two skills genuinely differ: quality-gate's Shared inputs hand untracked files to the reviewer directly (SKILL.md:27, self.md:30, per-slice.md:21), while fanout's surfaces get only the merge-base diff. Stopping made a new-module or new-test review report "nothing to review" about work that is plainly there. Now only a genuinely empty change set stops; the divergence from fanout is stated with its reason. Still never stages files. - P2 cross-plugin surface (spec.md rung 2). PLUGIN-PHILOSOPHY.md forbids discovering a sibling plugin's installation directory and requires a documented public seam; /work-items:track exposes no item-fetch action, so "call get-item" had no legal path in the normal installed layout. Rung 2 now prefers a public reader when a consumer exposes one and otherwise uses the provider mechanic, which is the operative path and works with no tracker plugin installed at all. Parent linkage degrades honestly rather than assuming an unreachable get-item. - P2 cross-repo read scoping (spec.md). The read supplied only the issue number, so a promoted cross-repo id would have read a same-numbered issue in the current repo. Now carries --repo "$owner/$repo". - Security lane, IMPORTANT: harvested refs reach a command argument, and commit/PR text is attacker-influenceable through a fork PR. Added explicit validation (number strictly numeric, owner/repo to a repo-name shape), drop-never-repair on failure, and discrete-argument passing. Noted that the item-content-trust boundary covers returned body text, not identifiers used to build commands, so this is its counterpart rather than a duplicate. Verdict doc and CHANGELOG corrected to match — including retracting the "first cross-plugin seam call in the marketplace" claim, which the philosophy doctrine rules out. New eval 11 covers the malformed/hostile ref drop; eval 8 updated for the provider-mechanic read and --repo scoping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
|
Claude finished @kyle-sexton's task in 1m 30s —— View job Security review complete ✅Task list
Per the CI security-review lane's relevance gate, this pass scopes to commits since the last-reviewed head ( Previously flagged IMPORTANT finding — fixedThe prior review's finding — a harvested issue ref shelled into
Also folded in: the reviewer's No new security-relevant findings in this diff
No outstanding or newly introduced security issues on this head. |
Review round 1 addressed — e457877All four findings (three from Codex, one IMPORTANT from the security lane) were verified against the repo before acting, and all four were real. The three inline threads carry per-finding detail and are resolved; this covers the security finding. Security — harvested ref reaches a command argument without a stated validation step. Accepted. The distinction the finding draws is the load-bearing one:
New eval case 11 ( One correction the round forced beyond the findings themselves. The Codex cross-plugin finding invalidated a claim in the design record, not just the implementation: Re-verified after the fixes: Generated by Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…ry the trust fence Round 2 of PR review. Three findings from the CI code-review lane plus one it deferred to the security lane; all four verified before acting. - allowed-tools was widened for the preflight but not for what the ported text and spec mode actually invoke. The "even after a fetch" phrasing came from fanout, which has no allowlist at all; quality-gate does, and had no git fetch entry — so the exact recovery the bullet describes needed a permission prompt, which is the headless stall the widening was supposed to prevent. Adds git fetch, git remote get-url, gh pr view, gh issue view (the PR-body harvest, issue-body read, and origin-remote resolution rung 2 needs). All read-only. - "this mode adds a class axis" undercut the vocabulary rule this same PR establishes: severity.md now reserves "axis" for severity/confidence, and the finding-class enum is neither that nor a lens. Now "finding-class dimension", with the reservation stated so it does not drift back. - Step 2 cited the item-content-trust boundary without carrying it. item-content-trust.md mandates that item-derived text interpolated into a subagent prompt sit inside the BEGIN/END QUOTED PR DATA fence, reused verbatim rather than reworded per surface, with the never-follow instruction restated after the closing marker. Spec mode dispatches a worker with tracker body text, so the fence now appears in full; a spec resolved from a local file is not item-derived and is exempted explicitly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
|
Claude finished @kyle-sexton's task in 3m 15s —— View job Security review complete ✅Task list
Per the CI security-review lane's relevance gate, this pass scopes to commits since the last-reviewed head ( Deferred item from the CI code-review lane — resolvedThe code-review lane's round-1 comment explicitly deferred one item to this lane: SUGGESTION — new
|
Security lane SUGGESTION (non-blocking), taken: Bash(git fetch:*) authorizes a fetch from any URL, which is the same outbound-request-to-an-arbitrary-host primitive as curl — and nothing in the allowlist restricted it to origin. Not exploitable today (no instruction builds a fetch argument from untrusted text; the origin/<baseRefName> case is API-sourced), but the blast radius was unused, so it should not be granted. Narrowed to Bash(git fetch origin) + Bash(git fetch origin:*), and applied the same narrowing to Bash(git ls-remote --symref:*) from the round-1 widening, which the finding named as carrying the same class of reach. Both are invoked against origin throughout this skill's diff-base ladder, so nothing documented loses a path. The bare and :* forms are both listed so an argument-less invocation still matches. gh pr view / gh issue view keep their wildcards: read-only against the bound forge, no arbitrary-host reach, and the issue read already passes regex-validated components as discrete arguments. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
Round 3 — security SUGGESTION taken, a2e0dfbNon-blocking, but the reasoning holds and the cost is a line, so I took it rather than deferring: Narrowed to
Running total across three rounds: 8 findings from three independent lanes (Codex ×3, code-review ×3, security ×2), all verified against the tree before acting, all resolved. Two of them corrected the design record rather than just the code — the cross-plugin seam claim and the "axis" vocabulary — and those corrections are reflected in Generated by Claude Code |
|
Claude finished @kyle-sexton's task in 54s —— View job Security review complete ✅Task list
Per the CI security-review lane's relevance gate, this pass scopes to commits since the last-reviewed head ( Previously flagged SUGGESTION — addressedThe prior lane pass suggested narrowing the unbounded
This removes the blast radius the suggestion identified — the grant can no longer authorize a fetch/ls-remote against an arbitrary attacker-chosen remote URL, only against the repo's own No new security-relevant findings in this diff
No outstanding or newly introduced security issues on this head. |
|
Claude finished @kyle-sexton's task in 1m 17s —— View job Code review skipped — PR already merged
I read
PR #3036 is MERGED as of this run (this comment's trigger fired on a Per the gate, I'm stopping here rather than posting inline findings against already-merged, already-reviewed code (this PR already went through two full rounds of the code-review and security-review lanes during review, both resolved per the thread above). No code changes were made. Nothing further to do unless a new PR or branch is opened. |
…e that claimed it did (#3038) Closes #3028 ## Summary `ship`'s macro-state snippet annotated its `get-item` call with `# body = the spec`, under a heading reading "Through the seam (coordination verbs; **no inline provider commands**)". Both halves were wrong, and together they were worse than either alone: a session following the skill's own snippet to read the container spec got **no spec text**, while the placement asserted the seam could do something it cannot. The normalized item object is `schema_version, id, title, state, assignees, labels, type, blocked_by_count, parent_id, url` — there is **no `body` field**, and `--body` exists only as a *write* parameter on `create-item`. ## Fix **Fixed at the source, not only at the reported call site.** `reference/tracker-seam.md`'s operation-routing table listed "single-item fetch" under Coordination with nothing said about the body — that omission is what let the assumption spread, and it demonstrably did: the same false premise was independently proposed in Lane D's first-draft design (#2937) and caught by the same audit that filed this issue. The table now: - marks single-item fetch as **identity/state/`parent_id` — not body** - lists **reading an item's body** under Provider mechanics - carries a paragraph stating the split outright: `get-item` remains authoritative for `parent_id` (how a slice reaches its container), body text is a provider-mechanic read (`gh issue view <n> --repo <owner>/<repo> --json body,title` on GitHub, the provider's REST equivalent otherwise), and a surface that shows a body read must label it as such Degradation is named rather than implied: provider mechanics run unbound, so the read survives a missing binding; and `local-markdown` stores item text as the file itself rather than having a body concept, so it is called out instead of papered over as parity. **Call sites corrected:** | Surface | What it claimed | |---|---| | `skills/ship/SKILL.md` | The reported bug. Snippet comment corrected, heading no longer implies the whole read is seam-only, and the provider-mechanic body read is shown as its own labelled step | | `skills/work/SKILL.md` | **Not in the issue's list** — its pass-by-reference step carried the identical premise: "fetch the container via the seam … and read its Brief body" | | `skills/decompose/SKILL.md` | "If an item, fetch full body and comments" left the mechanism to inference; now names the provider-mechanic read | | `skills/ship/evals/evals.json` | Asserted the container is read wholly "through seam verbs"; now separates identity/rollup (seam) from the Brief text (provider mechanic) | `planning:plan` was checked as the issue asks and carries no such assumption — no change. ## Verification - Repo-wide `grep -rn "get-item"` across `plugins/` and `docs/`, filtered to hits near body/spec/brief, now returns **only these corrections** — no surface still claims the seam returns a body - `check-skill.sh` on `ship`, `work`, `decompose` → **PASS, 0 errors** each. Warning counts are unchanged from baseline (1 / 2 / 2), verified by re-running against the stashed tree — all pre-existing (`no Gotchas surface`, soft line-count targets) - `ship/evals/evals.json` validated against `plugins/skill-quality/reference/evals.schema.json` → OK - `check-evals-quality.sh` → **PASS, 0 warnings** - `markdownlint-cli2` over `plugins/work-items/**` → 0 issues in 46 files - `check-skill-portability.sh` / `check-shell-portability.sh` vs `origin/main` → no unexcused findings - `check-changelog-parity.sh` (`--check`, `--check-bump`, `--check-order`), `validate-plugins.sh`, `validate-plugin-contracts.mjs`, `generate-catalog.mjs`, `generate-cheatsheet.mjs` → all clean / in sync Patch bump `0.36.1` → `0.36.2` with the matching CHANGELOG entry, per the issue's acceptance criteria. ## Related - Refs #2937 — Lane D, merged as #3036; its audit surfaced this bug, and its `spec` mode documents the same provider-mechanic body read on the `review` side. This item is the fix at the source - Refs #2933 — spec container for the wider effort --- _Generated by [Claude Code](https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Closes #2937
Summary
Lane D of the Pocock shipping-lifecycle absorption (#2933):
quality-gategains a ninth lens,spec, that judges a diff against its originating spec rather than on its own terms — plus thediscovery ladder that finds that spec, the preflight gate the skill never had, and the C12–C16
verdicts. Implements the design locked on #2937 after a two-validator adversarial audit; three of
five original answers were revised on evidence, and PR review then corrected two more — this body
describes the shipped state, not the original design.
The gap was real and already had a dangling consumer:
work-items:decomposeandwork-items:shipboth route container close-out to "the review plugin's spec-fidelity machinery," which did not
exist.
context/spec.mdis that machinery.Fix
specmode —plugins/review/skills/quality-gate/context/spec.md(new). Owns thefinding-class enum (
missing/scope-creep/wrong), requires every finding to quote the specline it is judged against, and judges in both directions so
scope-creepis reachable at all.scope-creepneeds a positive statement of bounded scope before unlisted behavior becomes adefect — a spec that never mentions a surface leaves the implementer's judgment intact.
Spec-source discovery ladder (C14) —
--spec <path|id>→ item refs harvested from branchcommits and PR body → the topic's contract slice → ask → skip with a note. The last rung is the
point: a fidelity verdict rendered without a spec is a fabrication, so a headless run with nothing
resolved stops and names the rungs it tried.
attacker-influenceable through a fork PR, and this rung turns text found in them into a command
argument: the number must be strictly numeric and an accompanying owner/repo must match a
repo-name shape, a ref that fails is dropped rather than repaired, and components are passed
as discrete arguments. The item-content-trust boundary governs returned body text and does not
cover an identifier used to build a command, so this is its counterpart, not a duplicate.
<provider>:<owner>/<repo>#<number>, and the read is scopedwith
--repoto that id's own repository — a bare number reads the current repo, which for across-repo ref is a different issue sharing a number.
sibling plugin.
PLUGIN-PHILOSOPHY.mdforbids discovering another plugin's installationdirectory, and no namespaced item-fetch action exists today, so the provider-mechanic read is the
operative path — which also means this rung works with no tracker plugin installed at all. Body
text was never a seam field regardless (the normalized item object carries no
body), and parentlinkage degrades honestly:
get-itemis authoritative forparent_idand is not reachable here,so a slice's container is best-effort or named directly with
--spec.Recorded limit: the contract slice is pruned before merge, so that rung goes empty post-merge —
which is exactly why the tracker item is the durable spec home.
item-content-trust.md's mandated verbatimBEGIN/END QUOTED PR DATAfence, with the never-follow instruction restated after the closingmarker; a spec resolved from a local file is not item-derived and is exempted.
Preflight (C15) — ported from
fanout, whichquality-gateentirely lacked. Mode-scoped:criteriais a reference mode that legitimately runs on a clean tree and stays exempt.Untracked-only is reviewable here, deliberately unlike
fanout: this skill hands untrackedfiles to the reviewer directly, so a new-module or new-test branch is a real change set, while
fanout's surfaces receive only the merge-base diff. Theallowed-toolsallowlist is widened withthe read verbs the gate and rung 2 need — including the
ghreads — with the two network-capablegrants (
git fetch,git ls-remote --symref) scoped tooriginrather than any URL.C13 — the originally proposed "never merge or rerank across axes" rule was withdrawn: it
would negate the normalization pipeline
fanoutexists to run, and its second scope isunimplementable against
quality-gate's one-lens-per-invocation rule. Landed instead:self.md'slarge-diff worker split tightened from "merge only after verification" to keep-separate
presentation, and the vocabulary recorded once in
context/severity.md—axismeansseverity/confidence; a review perspective is a
lens. Three incompatible senses were live.self.mdstops being a second SSOT. Its spec-conformance checklist item restated the samethree classes; the fenced worker checklist now keeps a shallow divergence-and-quote check and
defers classification, with the pointer to the owning file in orchestrator-facing escalation
text — not inside the subagent template, which is addressed to a fresh-context read-only worker
that cannot invoke a skill to follow it.
Verdicts — C12–C16 recorded in
docs/upstream/aihero-shipping-course.md(C12 + C14ADOPTED-corrected, C13 + C16 ALREADY-PRESENT, C15 PARTIAL), including the reasoned decision to keep
the CI code-review lane quality-only, the note that container close-out split out to #3027, and the
retraction of the design's "first cross-plugin seam call in the marketplace" framing, which the
plugin doctrine rules out.
Collateral:
plugins/review/README.mdmode list, frontmatterdescription+argument-hint,four new evals, version bump + CHANGELOG.
Verification
All run locally against this branch:
check-skill.sh --require-evals quality-gate→ PASS, 0 errors / 0 warnings (SKILL.md124/500 lines, description 410/1536 chars, all 9 base-ref trigger phrases preserved, broken-ref
and markdownlint checks clean)
plugins/review/tests/standards-binding.test.sh→ PASS=8 FAIL=0 — confirms the acceptancecriterion that standards discovery still resolves through the standards contract with no
fixed-filename regression
check-evals-quality.sh→ PASS; the new cases raise no warnings (the 3 reported arepre-existing on case id=7)
plugins/skill-quality/reference/evals.schema.json→ OK (11 cases)check-skill-portability.sh origin/main→ no unexcused coupling tokens in 27 skill filescheck-shell-portability.sh origin/main→ no unexcused GNU-only constructs in 32 filesmarkdownlint-cli2overplugins/review/**+docs/upstream/**→ 0 issues in 34 filescheck-changelog-parity.sh(all four modes),validate-plugins.sh,validate-plugin-contracts.mjs,generate-catalog.mjs,generate-cheatsheet.mjs→ all clean / in synccheck-skill-precompute-compose.shreports a violation on this SKILL.md; verified pre-existingby re-running against the stashed tree, and warn-only in CI.
Review rounds: 8 findings across three independent lanes (Codex ×3, CI code-review ×3, CI
security ×2), each verified against the tree before acting, all resolved — see the inline threads
and the two round-summary comments. Two of them corrected the design record rather than only the
code; those corrections are in the verdict doc and CHANGELOG, not just the implementation.
Related
blocked on it; the two
work-itemsroutes that currently name non-existent machinery are thatitem's acceptance criteria, deliberately left untouched here
work-items:ship;this PR corrects it on the
reviewside, that item fixes it at its sourceGenerated by Claude Code