mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-06 08:54:04 +01:00
* feat(ship): version-less repos, docs-audit record and caps, the frozen-scope paid case, and the review-loop follow-ups Follow-up to the review-loop convergence PR: Step 12 and the CHANGELOG step skip cleanly in a repository with no version source (receipt version "none"); the receipt records the docs audit outcome and the owner exception (DOCS_AUDIT_BLOCKED without one) and a policy may declare doc caps (gstack-ship-receipt docs-caps); docs/unattended.md names the READY evidence and the cross-family outside voice; the ship-frozen-scope-convergence behaviour case runs the generated Step 9.4 under review_scope frozen through the real binaries; carry probes git >= 2.39 (RECEIPT_GIT_TOO_OLD); defer passes the repository to gh repo view positionally and names its search limit; late defers count without being passes; reviewers diff from RANGE_BASE on pass 2+; the cap check runs before edits; --review-fanout batches specialists by free slots; read --check-prose compares quoted counters with the receipt (RECEIPT_PROSE_MISMATCH); checklist rule ids prereg-with-results and test-keyboard-count-loop; gstack-coauthor-trailers. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test: repair the CI reds the retarget onto main surfaced (test-only) Seven test files, no product change. The pull_request run at2d73faddfailed 5 Linux shards and 4 Windows shards: - test/ship-frozen-scope-actor.test.ts imported only test code, so the test-of-test ratchet refused it; the first case now recomputes the loop summary with summarizeReviewLoop/reviewLoopLine from lib/review-scope and asserts the exact REVIEW_LOOP line. Its three cases replay the bash workflow through bash -c and the shebang bins, so they are POSIX-only. - test/codex-host.test.ts: the new gate case is Claude-specific (33 -> 34). - test/paid-retry-supervision.test.ts: the gate census fixture now carries a trial record for the one planned behavior-kind gate case, the way test/ci-paid-coordination.test.ts already does. - test/review-fix-range.test.ts asserted the pre-#3487 DIFF_BASE sentence; it now asserts the ${RANGE_BASE:-merge-base} clause that carries the same contract (every reviewer reads the range from pass 2 on). - test/gstack-coauthor-trailers.test.ts: the fixture sets its own git identity; a CI runner has none to auto-detect ("Committer identity unknown"). - test/review-scope-followups.test.ts and test/gstack-ship-receipt-docs.test.ts: the gh and old-git stand-ins are sh scripts on a colon-joined PATH, which CreateProcess cannot run, so those three cases skip on win32. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes (pass 1) Twelve informational findings from the /ship pass-1 review, all below the blocking floor, fixed in place: - docs-caps reads docs.caps from origin/<base>'s policy (--base <ref>), never the branch's copy; an unavailable base policy refuses with POLICY_SOURCE_UNAVAILABLE instead of substituting DEFAULT_POLICY - receiptVersion returns "none" only when the stamp file is absent; other errors surface as undefined - checkReceiptProse groups counters by leaf key and uses word boundaries that cover hyphenated names, so shared leaf keys and hyphenated words no longer report RECEIPT_PROSE_MISMATCH - gstack-coauthor-trailers refuses a --range/--base starting with "-", prefixes origin/ when origin/<ref> resolves (slash-named branches), and passes --end-of-options to git log - REVIEW_SCOPE_KEY_VERSION and DOCS_CAPS_KEY_VERSION name the releases that first read their keys - the Step 11 late-defer row logs under skill review-late-defer so a tokenless review row never reads UNVERIFIED; summarizeReviewLoop counts both - the "already filed" literal is one constant shared by bin and renderer - receiptDocs (dead) removed - Step 14.5 clears .gstack/tmp/docs-audit.json on first entry - dispatched review prompts carry a literal diff-base sha (<diff-base>) instead of a shell variable the reviewer process never sees - the coauthor-trailers tests launch the bin through runScriptSync so the Windows lane can run them Tests extended for each change; SKILL.md, sections and goldens regenerated. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test: cover the carry patch-id refusal (Step 7 coverage gap) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test: derive the too-old review_scope minimum from the constant Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: keep co-author trailers in one trailer block gstack-coauthor-trailers --message appended the new Co-authored-by lines as a separate paragraph after existing trailers, so git interpret-trailers (and GitHub attribution) read only the last paragraph and the existing co-author lost credit. The new lines now join the existing trailer block; the test checks the result with git interpret-trailers --parse. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: let git place co-author trailers; name the docs key release gstack-coauthor-trailers --message now runs git interpret-trailers --in-place, so the appended lines join the existing trailer block whatever its shape (CRLF, comment lines, folded continuations) and the existing co-author keeps attribution; the interpret-trailers check in the test carries a timeout for the spawnSync tripwire. DOCS_CAPS_KEY_VERSION names the release after the newest CHANGELOG heading on main (1.91.258.0), and a test ties every unreleased policy key to that next release so a stamp elsewhere fails loudly. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: hold unreleased policy keys newer than any visible heading; pin trailer placement The CHANGELOG-heading test asked for the exact next release after the newest heading, which a depth-1 CI clone (no origin/main) computes from the branch alone, so it went red there. It now requires an unreleased key's version to be newer than every heading the checkout can see, which holds at depth 1, on the stamped train tree and on main; the ship-policy comment says who moves the constant when the stamp lands under another number. gstack-coauthor-trailers pins git interpret-trailers' placement on the command line (--no-divider so a --- body line is not a divider, --where end, --if-exists addIfDifferentNeighbor, trailer.separators=:, -- before the path) so a user's trailer.* config cannot replace or drop the existing co-author. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: describe the shipped contracts (Step 13 and 14.5) CHANGELOG [Unreleased]: docs-caps --base <base> reading origin/<base>'s policy, the review-late-defer row, the literal <diff-base> prompt rule, the coauthor-trailers placement and refusals, the policy key versions (review_scope 1.91.256.0 re-pin, docs.caps at the stamping release), the version-less conditions split between Step 12 and the receipt, and the carry refusal wording. docs/ship-policy.md, docs/review-policy.md and docs/unattended.md follow the code: docs-caps --base, the literal diff-base sha, the trailer block, the gstack-outside-voice run binding and the Step 11 row fields. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: scope the literal diff-base claim to the review-army prompts; name where the outside-voice cause lives Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(docsync): declare Step 14.5's own-tree commands to the fixture; exclude .gstack/tmp at the Entry The ship-docsync-completion and ship-docsync-current paid cases failed twice at775d9804: Step 14.5's continue path now runs gstack-ship-receipt docs-caps and writes .gstack/tmp/docs-audit.json, and the docsync fixture's closed interface declared neither, while its repository had no origin/main for docs-caps to read. The actor stopped correctly (blocked, no exception) and never reached the publication stand-in. - test/helpers/docsync-interface.ts: the three housekeeping commands (the shared exclude line, rm -f of the record, docs-caps --base main) and the record write are declared, byte for byte with the generated section; DOCS_AUDIT_RECORD_PATHS names what the kernel observer may see. - test/helpers/docsync-observer.ts: accepts exactly those commands and the record write; every neighbour (another base, --json, write --docs, another .gstack file) stays outside the interface. - test/helpers/docsync-fixture.ts: every scenario pushes main to the fixture remote, as Steps 0-14 would have. - ship/sections/documentation.md.tmpl: the Entry runs the shared .gstack/tmp exclude line before rm -f, so the record never surfaces as an untracked file when no earlier step added it; the caps paragraph tightened to hold the ship parity ratio at 1.7400. - test/docsync-command-grammar.test.ts: pins the literals against the generated section and the refusals around them. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(docsync): the receipt record is housekeeping for read-only audits and the fault cases too Pass 9's outside voice found two gaps in the fixture change (2524ec4): a read-only audit (ship-docsync-current) must still write .gstack/tmp/docs-audit.json, but docsToolFailures flagged every product write under readOnly before the record allowance; and the unseeded fault cases (missing-asset, legacy-completion) run the Entry too, so their kernel-write check saw .git/info/exclude as a forbidden write because only the lifecycle test named the housekeeping paths. - docsToolFailures: the record write is permitted under readOnly; the document and every other product file stay refused. - docsWriteFailures: DOCS_AUDIT_RECORD_PATHS are permitted for every caller, so the fault evaluator needs no per-scenario list; anything else under .git or .gstack stays forbidden. - test/docsync-command-grammar.test.ts pins both: the record under readOnly, the document refused there, the observed housekeeping paths accepted with an empty allowance and two neighbours refused. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test(docsync): accept the cap gate's ~/.claude/skills/gstack form the section prints (pass 9 Red Team note) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test(docsync): name the admitted composed line precisely (pass 9 maintainability note) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(docsync): install lib/ beside bin/ in the fixture so the declared cap gate can load (2524ec4rerun: module error, not a verdict) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(docsync): admit the Write tool's temp sibling of the receipt record (0230fc2rerun: both lifecycle cases red on it) The paid rerun at0230fc2cleared every earlier refusal and both cases then failed on one path: forbidden docs write: .gstack/tmp/docs-audit.json.tmp.<pid>.<12 hex>. Claude Code's Write and Edit tools land every file, new ones included, through that sibling plus fsync and rename, and the fixture's kernel observer proved such a landing for the document only (atomicTargets: [DOC_PATH]). The observer now treats the record's sibling as a transient file too, and docsWriteFailures admits it only as the cookie-paired MOVED_FROM source of a MOVED_TO on the record whose other events are create/modify/attrib/close_write and which is left in neither snapshot nor in changed. A lingering sibling, an unpaired move, a move onto another path or a sibling of the document moved onto the record stay forbidden (grammar test). A live check on this machine saw the second landing's create/modify/close_write/moved_from events admitted and a stray sibling refused. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
347 lines
20 KiB
TypeScript
347 lines
20 KiB
TypeScript
/**
|
||
* Review Army resolver — parallel specialist reviewers for /review
|
||
*
|
||
* Generates template prose that instructs Claude to:
|
||
* 1. Detect stack and scope (via gstack-diff-scope)
|
||
* 2. Select and dispatch specialist subagents in parallel
|
||
* 3. Collect, parse, merge, and deduplicate JSON findings
|
||
* 4. Feed merged findings into the existing Fix-First pipeline
|
||
*
|
||
* Shipped as Release 2 of the self-learning roadmap (SELF_LEARNING_V0.md).
|
||
*/
|
||
import type { TemplateContext } from './types';
|
||
import { CC_BACKGROUND_DEFAULT_SINCE, FOREGROUND_IF_AVAILABLE, BACKGROUND_RECOVERY } from './constants';
|
||
import { learningsCapture, LEARNINGS_VERDICT } from './learnings';
|
||
import { DEFENSIVE_REVIEW_FRAMING, FIXTURE_SUMMARY_MODE } from './defensive-review';
|
||
|
||
function generateSpecialistSelection(ctx: TemplateContext): string {
|
||
const isShip = ctx.skillName === 'ship';
|
||
const stepSel = isShip ? '9.1' : '4.5';
|
||
const stepMerge = isShip ? '9.2' : '4.6';
|
||
const nextStep = isShip ? 'Step 9.3 (cross-review dedup)' : 'Step 4.8 (adversarial review), then Step 5';
|
||
return `## Step ${stepSel}: Review Army — Specialist Dispatch
|
||
|
||
### Detect stack and scope
|
||
|
||
\`\`\`bash
|
||
_SM=$(${ctx.paths.binDir}/gstack-state-matrix detect --base <base>) || _SM=SCOPE_STATE_MACHINE=unknown; source <(echo "$_SM")
|
||
source <(${ctx.paths.binDir}/gstack-diff-scope <base> 2>/dev/null) || true
|
||
set | grep '^SCOPE_'
|
||
STACK=""
|
||
[ -f Gemfile ] && STACK="\${STACK}ruby "
|
||
[ -f package.json ] && STACK="\${STACK}node "
|
||
[ -f requirements.txt ] || [ -f pyproject.toml ] && STACK="\${STACK}python "
|
||
[ -f go.mod ] && STACK="\${STACK}go "
|
||
[ -f Cargo.toml ] && STACK="\${STACK}rust "
|
||
[ -f pubspec.yaml ] && STACK="\${STACK}dart "
|
||
echo "STACK: \${STACK:-unknown}"
|
||
DIFF_BASE=$(git merge-base origin/<base> HEAD)
|
||
DIFF_LINES=$(git diff "$DIFF_BASE" --numstat | awk '{n+=$1+$2} END {print n+0}')
|
||
echo "DIFF_LINES: $DIFF_LINES"
|
||
# Detect test framework
|
||
TEST_FW=""
|
||
{ [ -f jest.config.ts ] || [ -f jest.config.js ]; } && TEST_FW="jest"
|
||
[ -f vitest.config.ts ] && TEST_FW="vitest"
|
||
{ [ -f spec/spec_helper.rb ] || [ -f .rspec ]; } && TEST_FW="rspec"
|
||
{ [ -f pytest.ini ] || [ -f conftest.py ]; } && TEST_FW="pytest"
|
||
[ -f go.mod ] && TEST_FW="go-test"
|
||
[ -f pubspec.yaml ] && TEST_FW="dart-test" && grep -qsE '^ +sdk: *.?flutter' pubspec.yaml && TEST_FW="flutter-test"
|
||
echo "TEST_FW: \${TEST_FW:-unknown}"
|
||
\`\`\`
|
||
|
||
### Read specialist hit rates (adaptive gating)
|
||
|
||
\`\`\`bash
|
||
${ctx.paths.binDir}/gstack-specialist-stats 2>/dev/null || true
|
||
\`\`\`
|
||
|
||
### Select specialists
|
||
|
||
Select specialists from the scope signals above.
|
||
|
||
**Always-on (dispatch on every review with 50+ changed lines):**
|
||
1. **Testing** — read \`${ctx.paths.skillRoot}/review/specialists/testing.md\`
|
||
2. **Maintainability** — read \`${ctx.paths.skillRoot}/review/specialists/maintainability.md\`
|
||
|
||
**If DIFF_LINES < 50:** Skip all specialists except State Machines. Print: "Small diff ($DIFF_LINES lines) — specialists skipped." Continue to Step ${stepMerge} with the ${isShip ? 'core/design-lite' : 'core'} findings and an empty specialist list, then the parent's Exploratory QA step and ${nextStep}. Small diffs skip fan-out, never the parent-owned smoke probes. Core shared-code checks also remain required.
|
||
|
||
**Conditional (dispatch if the matching scope signal is true):**
|
||
3. **Security** — if SCOPE_AUTH=true, OR if SCOPE_BACKEND=true AND DIFF_LINES > 100. Read \`${ctx.paths.skillRoot}/review/specialists/security.md\`
|
||
4. **Performance** — if SCOPE_BACKEND=true OR SCOPE_FRONTEND=true. Read \`${ctx.paths.skillRoot}/review/specialists/performance.md\`
|
||
5. **Data Migration** — if SCOPE_MIGRATIONS=true. Read \`${ctx.paths.skillRoot}/review/specialists/data-migration.md\`
|
||
6. **API Contract** — if SCOPE_API=true. Read \`${ctx.paths.skillRoot}/review/specialists/api-contract.md\`
|
||
7. **Design** — if SCOPE_FRONTEND=true. Use the existing design review checklist at \`${ctx.paths.skillRoot}/review/design-checklist.md\` and run the mechanical pass at the top of that checklist (the user-installed design detector, when present) before the LLM items
|
||
8. **Simplification** — if DIFF_LINES > 100. Read \`${ctx.paths.skillRoot}/review/specialists/simplification.md\`. Advisory-only lens: hunts unrequested structure (hand-rolled stdlib, one-implementation abstractions, dependencies duplicating platform features), never coverage.
|
||
9. **State Machines** — if SCOPE_STATE_MACHINE is true or unknown: read \`${ctx.paths.skillRoot}/review/specialists/state-machines.md\`, run \`${ctx.paths.binDir}/gstack-state-matrix outfile\`, Read the file it prints, write the full output there, then run \`${ctx.paths.binDir}/gstack-state-matrix check <file>\` on it.
|
||
|
||
### Adaptive gating
|
||
|
||
After scope-based selection, apply adaptive gating based on specialist hit rates:
|
||
|
||
For each conditional specialist that passed scope gating, read the \`gstack-specialist-stats\` output:
|
||
- If tagged \`[GATE_CANDIDATE]\` (0 findings in 10+ dispatches): skip it. Print: "[specialist] auto-gated (0 findings in N reviews)."
|
||
- If tagged \`[NEVER_GATE]\`: always dispatch regardless of hit rate.
|
||
|
||
**Force flags:** \`--<specialist>\` (e.g. \`--simplification\`) or \`--all-specialists\` in the prompt force-includes it.
|
||
|
||
Print the selection:
|
||
"Dispatching N specialists: [names]. Skipped: [names] (scope not detected). Gated: [names] (0 findings in N+ reviews)."`;
|
||
}
|
||
|
||
/**
|
||
* The diff command every dispatched reviewer runs. In /ship the parent writes the
|
||
* base sha into the prompt (the merge base on pass 1, RANGE_BASE from pass 2 on):
|
||
* a reviewer is its own process and never sees the parent's shell variables, so a
|
||
* `$RANGE_BASE` reference there would silently fall back to the whole branch.
|
||
*/
|
||
function diffBase(ctx: TemplateContext): string {
|
||
return ctx.skillName === 'ship' ? 'DIFF_BASE=<diff-base>' : 'DIFF_BASE=$(git merge-base origin/<base> HEAD)';
|
||
}
|
||
|
||
function generateSpecialistDispatch(ctx: TemplateContext): string {
|
||
return `### Dispatch specialists in parallel
|
||
|
||
For each selected specialist, launch an independent subagent via the Agent tool.
|
||
**Launch ALL selected specialists in a single message** (multiple Agent tool calls)
|
||
so they run in parallel. Each subagent has fresh context — no prior review bias.
|
||
${ctx.skillName === 'ship' ? `
|
||
**Fan-out (\`--review-fanout auto|serial|parallel\` in the prompt; default \`auto\`).**
|
||
Under \`auto\`, when the host reports fewer free subagent slots than selected
|
||
specialists (Capy caps a thread at 8 running subagents, and waiting or idle ones hold
|
||
a slot), dispatch in batches of the free-slot count, at least one per batch, each
|
||
batch in one message, and start the next batch only after every reader of the
|
||
previous one has finished or is confirmed stopped; print \`Fan-out: N specialists in
|
||
M batches of K\`. A host that reports no slot count runs one parallel batch.
|
||
\`serial\` is batches of one; \`parallel\` is one batch regardless of slots (a refused
|
||
launch is a failed reader, never silently dropped coverage). Batching changes the
|
||
order, never the set: every selected specialist still runs before Step 9.2 merges.
|
||
` : ''}
|
||
**Each specialist subagent prompt:**
|
||
|
||
Each specialist prompt includes:
|
||
|
||
1. The specialist's checklist path from the selection above (the subagent reads it; never paste its content)
|
||
2. Stack context: "This is a {STACK} project."
|
||
3. Past learnings for this domain (if any exist):
|
||
|
||
\`\`\`bash
|
||
${learningsCapture(`${ctx.paths.binDir}/gstack-learnings-search --type pitfall --query "{specialist domain}" --limit 5`)}
|
||
${LEARNINGS_VERDICT}
|
||
\`\`\`
|
||
|
||
If learnings are found, include them: "Past learnings for this domain: {learnings}"
|
||
|
||
4. **Defensive framing (Security specialist and the Red Team below only).** Begin the prompt with:
|
||
"${DEFENSIVE_REVIEW_FRAMING}"
|
||
and replace its full-diff command with: "${FIXTURE_SUMMARY_MODE}"
|
||
Every other specialist, including Testing, reads the full diff with fixtures.
|
||
|
||
5. Instructions:
|
||
|
||
"You are a specialist code reviewer. Read the checklist at {checklist path}, then run
|
||
\`${diffBase(ctx)} && git diff "$DIFF_BASE"\` to get the ${ctx.skillName === 'ship' ? 'diff (`<diff-base>`: write the sha in before dispatch, the merge base on pass 1 and `RANGE_BASE` from pass 2 on; never a shell variable)' : 'full diff'}. Apply the checklist against the diff.
|
||
|
||
\`NO FINDINGS\` alone is the expected, complete result for a clean diff; never invent an item to show work. Otherwise output one JSON object per finding, on its own line:
|
||
{\\"severity\\":\\"CRITICAL|INFORMATIONAL\\",\\"confidence\\":N,\\"path\\":\\"file\\",\\"line\\":N,\\"category\\":\\"category\\",\\"summary\\":\\"description\\",\\"fix\\":\\"recommended fix\\",\\"fingerprint\\":\\"path:line:category\\",\\"specialist\\":\\"name\\"}
|
||
|
||
Required fields: severity, confidence, path, category, summary, specialist.
|
||
Optional: line, fix, fingerprint, evidence, test_stub, advisory, evidence_paths, helper_target, cause.
|
||
CRITICAL means a test that would fail, a production break, or a security or data-loss defect; everything else is INFORMATIONAL.
|
||
A defect the diff causes but that shows elsewhere (a caller broken by a changed signature, a consumer of a removed export) keeps the victim as \`path\` and names the changed line in \`cause\`: {\\"cause\\":{\\"path\\":\\"file\\",\\"line\\":N}}. Without it the finding reads as pre-existing code.
|
||
|
||
Leave extraction advice to the core shared-code check; report real defects in duplicated code independently, keep advisory metadata on structural advice, and never call a demonstrated defect advisory because a shared helper could fix it.
|
||
|
||
If you can write a test that would catch this issue, include it in the \`test_stub\` field.
|
||
Use the detected test framework ({TEST_FW}). Write a minimal skeleton — describe/it/test
|
||
blocks with clear intent. Skip test_stub for architectural or design-only findings.
|
||
|
||
Output nothing else: no preamble, no summary or commentary.
|
||
|
||
Stack context: {STACK}
|
||
Past learnings: {learnings or 'none'}"
|
||
|
||
**Subagent configuration:**
|
||
- Use \`subagent_type: "general-purpose"\`
|
||
- Pass ${FOREGROUND_IF_AVAILABLE} on every specialist Agent call — background is the default since ${CC_BACKGROUND_DEFAULT_SINCE}; omitting an available flag is not foreground. ${BACKGROUND_RECOVERY}
|
||
|
||
**Wait for readers before editing:**
|
||
- Confirm that each task has finished or is stopped. A timeout alone does not prove termination. If a reader or writer is still active, wait; if its state is unknown, inspect its task/process status. If you cannot confirm it stopped, use the parent's Fix-First stop path without edits.
|
||
- A failed task may be stopped without having completed its review. Record the failure and retain usable partial findings.
|
||
- Continue independent evidence collection after a terminal failure. Missing dispatched coverage remains incomplete, never completed or clean; successful peers cannot replace it.`;
|
||
}
|
||
|
||
function generateFindingsMerge(ctx: TemplateContext): string {
|
||
const isShip = ctx.skillName === 'ship';
|
||
const stepMerge = isShip ? '9.2' : '4.6';
|
||
const fixFirstRef = isShip ? 'Step 9.3 dedup, then Step 9.4 Fix-First' : 'Step 5 Fix-First';
|
||
const critPassRef = isShip ? 'the checklist pass (Step 9)' : 'the CRITICAL pass findings from Step 4';
|
||
const persistRef = isShip ? 'the review-log persist' : 'the review-log entry in Step 5.8';
|
||
return `### Step ${stepMerge}: Collect and merge findings
|
||
|
||
Follow these stages in order. Validate core and specialist findings alike, but keep
|
||
their source labels: specialist scoring is not the final review's defect count.
|
||
|
||
#### 1. Parse outputs
|
||
|
||
After specialist attempts settle, collect their outputs, tagged by actual source.
|
||
\`NO FINDINGS\` is a completed, clean result, never a failed reviewer. Otherwise parse
|
||
each JSON line and skip invalid lines. Missing or unusable output is incomplete
|
||
coverage, not an empty success. Keep each specialist's returned findings for activity stats.
|
||
|
||
#### 2. Validate severity
|
||
|
||
For core and specialist findings with \`"severity":"CRITICAL"\` and \`"advisory":true\`,
|
||
remove \`advisory\` and retain its \`CRITICAL\` severity. Treat these as defects before
|
||
identity, merging, counting, scoring or Fix-First. Never downgrade severity to make
|
||
advisory metadata consistent. Valid INFORMATIONAL advisories remain advisory in
|
||
every category, including simplification.
|
||
|
||
#### 3. Identify and merge
|
||
|
||
Partition defects and advisories BEFORE grouping by fingerprint. Never merge a
|
||
defect with advice, even on a supplied-hash collision. Neither higher-confidence
|
||
advice nor a prior skipped extraction may replace, downgrade or suppress a defect.
|
||
|
||
Compute identities for both core and specialist findings:
|
||
- Shared-code advice (category \`shared-libs\` or fingerprint prefix \`shared-libs:\`):
|
||
call installed \`sharedLibsFingerprint\` from \`${ctx.paths.skillRoot}/lib/review-evidence.ts\`
|
||
with \`evidence_paths\` and \`helper_target\` as literal JSON on stdin, as in the core pass;
|
||
never trust a supplied hash or generate one yourself. Missing/malformed metadata
|
||
cannot deduplicate or reuse a saved decision.
|
||
- Other findings: use supplied \`fingerprint\`, else \`{path}:{line}:{category}\`
|
||
or \`{path}:{category}\` when no line exists.
|
||
|
||
Within the specialist list, merge matching identities in the same partition: keep
|
||
the highest confidence and all source names. Confirmation by distinct specialists adds +1 (cap at 10) and \`MULTI-SPECIALIST CONFIRMED ({specialist1} + {specialist2})\`.
|
||
Core findings never earn a specialist confidence boost. Preserve \`advisory\`,
|
||
\`evidence_paths\` and \`helper_target\` through every merge.
|
||
|
||
#### 4. Lead judgment
|
||
|
||
Reviewers fill the space they are given. Before any merged finding counts, the
|
||
parent reads each one and drops or downgrades it on these grounds, saying so:
|
||
- **Nit gravity.** When a reviewer's whole return is style, naming and preference,
|
||
the code is probably fine; write that down and count nothing from it as a defect.
|
||
- **Hypothetical, not actual.** A defect names the call site or input that reaches
|
||
it. One that does not is a note until someone traces it.
|
||
- **Structure for a case that does not exist.** Advice to abstract, generalize or
|
||
guard a path nothing takes is advisory, never a defect.
|
||
- **A different design is not a defect.** "I would have done it differently" with
|
||
no failing case behind it is dropped.
|
||
- **Missing context.** A finding that assumes a rule the repository documents
|
||
otherwise is dropped with the pointer.
|
||
Agreement between reviewers that did not see each other's output is the strongest
|
||
signal; the \`MULTI-SPECIALIST CONFIRMED\` boost above records it. A reviewer that
|
||
returned \`NO FINDINGS\` was right until a traced defect says otherwise.
|
||
|
||
#### 5. Apply specialist confidence gates
|
||
|
||
- Confidence 7+: show normally in the findings output
|
||
- Confidence 5-6: show with caveat "Medium confidence — verify this is actually an issue"
|
||
- Confidence 3-4: move to appendix (suppress from main findings)
|
||
- Confidence 1-2: suppress entirely
|
||
|
||
Core findings keep the core Confidence Calibration gates.
|
||
|
||
#### 6. Score and present specialists
|
||
|
||
Only specialist findings enter this header and \`quality_score\`; core findings do not.
|
||
Use the merged NON-advisory specialist findings for both counts and score;
|
||
the header's N is X + Y, so advisory findings never add to it:
|
||
\`quality_score = max(0, 10 - (critical_count * 2 + informational_count * 0.5))\`
|
||
Cap at 10 and retain for ${persistRef}. These are not final unresolved-defect totals.
|
||
Print only this block: the stage 7 activity object and \`test_stub\` bodies are log and Fix-First data.
|
||
Validated \`"advisory": true\` findings from any source are excluded from score,
|
||
header, unresolved-defect totals and clean-status blockers. Show them separately;
|
||
they remain ASK-only, never auto-applied. Real defects follow normal Fix-First.
|
||
|
||
\`\`\`
|
||
SPECIALIST REVIEW: N findings (X critical, Y informational) from Z specialists
|
||
|
||
[For each finding, in order: CRITICAL first, then INFORMATIONAL, sorted by confidence descending;
|
||
advisory findings last, each rendered with an [ADVISORY] label in place of the severity]
|
||
[SEVERITY] (confidence: N/10, specialist: name) path:line — summary
|
||
Fix: recommended fix
|
||
[If MULTI-SPECIALIST CONFIRMED: show confirmation note]
|
||
|
||
PR Quality Score: X/10
|
||
\`\`\`
|
||
|
||
**Simplification footer (after the score line):**
|
||
- If the simplification specialist was dispatched and returned findings, sum
|
||
their \`lines_removable\` values and print: \`net: -N lines possible\` (omit
|
||
findings without the field from the sum).
|
||
- If it was dispatched and returned NO FINDINGS, print:
|
||
\`Simplification: lean already — nothing to cut.\`
|
||
- If it was not dispatched, print neither line.
|
||
|
||
Core shared-code savings never enter this footer; explain any overlap once in the core proposal.
|
||
|
||
#### 7. Save specialist activity
|
||
|
||
Compile a \`specialists\` object for ${persistRef}.
|
||
${isShip ? 'For each specialist' : 'For DIFF_LINES < 50, keep `specialists: {}`; do not manufacture per-specialist scope records. Otherwise record each considered specialist'} (testing, maintainability, security, performance, data-migration, api-contract, design, simplification, state-machines, red-team):
|
||
- If dispatched: \`{"dispatched": true, "findings": N, "critical": N, "informational": N}\`
|
||
- If skipped by scope: \`{"dispatched": false, "reason": "scope"}\`
|
||
- If skipped by gating: \`{"dispatched": false, "reason": "gated"}\`
|
||
- If not applicable (e.g., red-team not activated): omit from the object
|
||
|
||
Count only findings that specialist actually returned, before deduplication.
|
||
Advisory findings COUNT in the stats \`findings\` field, not its defect counts.
|
||
Include Design despite its different checklist. Preserve dispatch/failure status:
|
||
zero returned findings from a failed attempt is not a clean review.
|
||
|
||
#### 8. Hand off to Fix-First
|
||
|
||
Send these findings to ${fixFirstRef} alongside ${critPassRef}.
|
||
Consolidate equivalent shared-code advice under the core proposal, retaining all
|
||
sources and counting overlapping savings once. Keep actual specialist stats;
|
||
core-only advice must not create a specialist dispatch or finding.
|
||
Normal AUTO-FIX/ASK rules apply, with advice ASK-only. Missing coverage still blocks
|
||
completion. Advice never permits edits while readers are active or replaces a required review.`;
|
||
}
|
||
|
||
function generateRedTeam(ctx: TemplateContext): string {
|
||
const isShip = ctx.skillName === 'ship';
|
||
const stepMerge = isShip ? '9.2' : '4.6';
|
||
const fixFirstRef = isShip ? 'Step 9.3 dedup, then Step 9.4 Fix-First' : 'Step 5 Fix-First';
|
||
return `### Red Team dispatch (conditional)
|
||
|
||
**Activation:** Only if DIFF_LINES > 200 OR any specialist produced a CRITICAL finding.
|
||
|
||
If activated, dispatch one more subagent via the Agent tool (pass ${FOREGROUND_IF_AVAILABLE} — foreground; subagents default to background since ${CC_BACKGROUND_DEFAULT_SINCE}; ${BACKGROUND_RECOVERY})
|
||
|
||
The Red Team subagent receives:
|
||
1. The red-team checklist path \`${ctx.paths.skillRoot}/review/specialists/red-team.md\` (it reads the file)
|
||
2. The merged specialist findings from Step ${stepMerge}, one line each (so it knows what was already caught)
|
||
3. The git diff command
|
||
|
||
Prompt, after the defensive framing and fixture handling from item 4 of the specialist dispatch: "You are a red team reviewer. The code has already been reviewed by N specialists
|
||
who found the following issues: {merged findings summary}. Your job is to find what they
|
||
MISSED. Read the checklist at {red-team checklist path}, list changed files with \`${diffBase(ctx)} && git diff --name-status "$DIFF_BASE"\`, read them as that fixture handling says, and look for gaps.
|
||
Output findings as JSON objects (same schema as the specialists), or \`NO FINDINGS\` alone, the expected result when they missed nothing. Focus on cross-cutting
|
||
concerns, integration boundary issues, and failure modes that specialist checklists
|
||
don't cover."
|
||
|
||
Tag Red Team findings \`"specialist":"red-team"\`.
|
||
Add them to the original specialist outputs and rerun stages 1–8 of Step ${stepMerge}
|
||
before ${fixFirstRef}; do not boost or count the earlier findings twice.
|
||
|
||
If the Red Team returns NO FINDINGS, note: "Red Team review: no additional issues found."
|
||
If the Red Team fails or times out, confirm it stopped and record its review as incomplete, just as for other specialists. ${isShip ? "Return to the parent's Exploratory QA step, then dedup and persistence; Step 9.4 cannot certify missing dispatched coverage as completed or clean." : 'Continue independent Step 4.7 QA and Step 4.8 adversarial review; Step 5.8 cannot certify missing dispatched coverage as completed or clean.'}`;
|
||
}
|
||
|
||
export function generateReviewArmy(ctx: TemplateContext): string {
|
||
// Codex host: strip entirely — Codex should not run Review Army
|
||
if (ctx.host === 'codex') return '';
|
||
|
||
const sections = [
|
||
generateSpecialistSelection(ctx),
|
||
generateSpecialistDispatch(ctx),
|
||
generateFindingsMerge(ctx),
|
||
generateRedTeam(ctx),
|
||
];
|
||
|
||
return sections.join('\n\n---\n\n');
|
||
}
|