From b0cacd05ee3adbb0e420871616f5f72dce71c115 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Tue, 28 Jul 2026 17:13:29 +0100 Subject: [PATCH] fix(ci): stop the review agent rejecting its own graph-backed reviews (#2731) * fix(ci): stop the review agent rejecting its own graph-backed reviews The context-evidence gate only counted a `context` call when the call itself passed `file_path` equal to a changed path. The review skill teaches plain `context({name})`, so 17 of the 26 review-agent run failures were complete, graph-backed reviews thrown away after full model spend, with no log line saying which invariant failed. Prove the evidence from the result instead: `status=found` plus a `symbol.filePath` inside the repo-scoped changed-path set. Every other check stays exactly as it was - strict JSON, orchestrator-only turns, result ordering, duplicate tool-id rejection - and the `repo` argument still selects the head or the merge-base path set. Same failure inventory, smaller classes: - rejection now logs why (in-scope, out-of-scope, sidechain, unresolved and off-path counts plus up to three sanitized paths), and the envelope error names the message count and first-message shape - Glob/Grep leave the tool set: they were enabled through `--tools` but never allow-listed, so every lane call was denied and burned turns - both pinned `npm ci` installs retry three times; one registry ECONNRESET killed a whole run - the prompt matches the new contract and asks for the structured body even when the analysis is incomplete Co-Authored-By: Claude Opus 5 (1M context) * fix(skills): mirror the review-skill tool-set change into the shipped copies The npm package, Claude plugin, and Cursor integration ship byte-identical copies of .claude/skills/gitnexus-review, and the drift guard compares them. Dropping Glob/Grep from the lane frontmatter and the SKILL.md sentence only landed in the canonical tree. Co-Authored-By: Claude Opus 5 (1M context) * fix(ci): stop one junk context result discarding a proven review Tri-review of this PR found that the previous commit fixed one spurious rejection and created another. Widening evidence candidacy from "the call that named a changed path" to "every orchestrator context call" also widened the *strict-parse* surface: `contextResultProvesChangedPath` throws rather than returning false, so a single malformed payload anywhere in the transcript now discarded a review that an earlier call had already proven. The MCP makes that reachable without any misbehaving model - `GITNEXUS_MCP_DEFAULT_MAX_TOKENS=12000` truncates any context payload over ~48 KB mid-JSON and appends a marker - and it also destroyed docs-only runs that the `no_indexable_changed_symbols` mode exempts. Reproduced by running the workflow's own embedded script on both trees: a proving evidence call followed by one truncated exploratory call gave `failure_code: null` on the base and `invalid_execution_transcript` on the head; it is `null` again here. - payload-shape failures are caught and counted (`malformedResults`) instead of thrown; transcript-structural invariants (envelope, tool shapes, duplicate ids, empty tool_result) still fail closed - diagnostics gained the reasons they were blind to: errored results, results that arrived out of order or via a sidechain, unanswered in-scope calls, and malformed payloads. A rejection can no longer print an in-scope call with every reason at zero - a deletion-only PR no longer registers head-scoped candidates that can never be satisfied: an empty eligible set is out of scope, not a result "outside the changed paths" - the mandatory-body prompt clause now pairs with a required `complete` boolean. An incomplete analysis publishes its partial body labelled `incomplete_analysis` instead of passing as an accepted review - `Agent(a,b,c)` is split into six separate `Agent(x)` rules: the pinned base action parses allowedTools with `.flatMap((v) => v.split(","))` (parse-sdk-options.ts at 3553f843), which shattered the grouped rule into `Agent(ci-correctness-lens`, four bare names, and `ci-critic-lens)` before the SDK saw it. Pre-existing and unproven at runtime, but the split form is correct under either reading and lets the header's dispatch canary actually prove something Co-Authored-By: Claude Opus 5 (1M context) * fix(ci): require a line range for context evidence The tri-review's adversarial lane executed `context({name: 'AGENTS.md'})` and had the result accepted: the gate checked only that the resolved filePath was in the changed set, so a bare File node passed for a review of that file's contents. The trusted prescan already defines an indexable symbol as one with startLine and endLine, so require the same here. Pre-existing rather than introduced by this branch, but it is the same "what counts as proof" surface the rest of this PR tightens. Co-Authored-By: Claude Opus 5 (1M context) * fix(ci): close the remaining tri-review findings Addresses every finding the tri-review left open after 0432214d and 1d9f2d75, across both engines. Reliability and maintainability (Codex ce, ce-reliability, ce-maintainability): - both pinned `npm ci` installs now call one shared `.github/scripts/npm-ci-retry.sh` instead of two near-identical 12-line blocks that differed only in a label - each attempt runs under `timeout` (default 600s, overridable), so a slow registry can no longer crowd the model review out of the job's budget - the helper distinguishes a timeout kill (124) from an npm rejection in its log Test coverage (ce-testing, Codex swarm P3, ce-security, swarm test-ci): - the retry helper is now exercised behaviourally with a stub npm: first-try success runs once, two failures recover on the third, three failures exit 1 - a non-string `symbol.filePath` is a clean reject, not a type error - an adversarial resolved path (ESC, newline, `::set-output`, RTL override) is proven sanitized before it reaches the job log - the envelope error's shape string is asserted - an in-scope call whose result never arrives is counted, not silent - install flags that keep the runtime inert (`--ignore-scripts`, `--prefix`, the lock-bound registry) are asserted against the helper they moved into Correctness and clarity (risk-architect, ce-standards): - the prompt now tells the model to prefer the uid form or pass file_path when a bare name could resolve into an unchanged file, which was the narrower off-path failure mode the gate rewrite left behind - `contextResultProvesChangedPath` -> `contextResultProvesEligiblePath`, matching the set-membership contract its sibling was renamed for - the transcript fixture's default no longer carries a `file_path` the gate ignores, which implied the opposite of the contract - SKILL.md says "file reads" rather than naming a CLI-specific tool, per the CLI-neutrality rule in AGENTS.md; mirrored to all three shipped copies - the interactive-swarm README notes the CI lanes are narrower Publisher (ce-reliability residual, pre-existing): - the publish job no longer gates the whole job on authorization, so a request rejected at normalization no longer strands the "review in progress" marker on the PR forever. Publication stays authorization-gated at the step; only the marker cleanup is unconditional. Co-Authored-By: Claude Opus 5 (1M context) * fix(ci): ship the install helper executable The extracted helper was committed 100644, so the workflow's direct invocation would have failed on the runner with permission denied - a break introduced by the extraction itself, invisible to every existing assertion. Set the mode and pin it with a test. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Gergo Magyar Co-authored-by: Claude Opus 5 (1M context) --- .claude/README-gitnexus-reviewer-swarm.md | 2 + .claude/skills/gitnexus-review/SKILL.md | 3 +- .../ci-personas/ci-adversarial-lens.md | 2 +- .../ci-personas/ci-blast-radius-lens.md | 2 +- .../ci-personas/ci-correctness-lens.md | 2 +- .../ci-personas/ci-coverage-lens.md | 2 +- .../ci-personas/ci-critic-lens.md | 2 +- .../ci-personas/ci-security-lens.md | 2 +- .github/scripts/npm-ci-retry.sh | 40 ++ .github/workflows/gitnexus-review-agent.yml | 258 +++++++--- .../skills/gitnexus-review/SKILL.md | 3 +- .../ci-personas/ci-adversarial-lens.md | 2 +- .../ci-personas/ci-blast-radius-lens.md | 2 +- .../ci-personas/ci-correctness-lens.md | 2 +- .../ci-personas/ci-coverage-lens.md | 2 +- .../ci-personas/ci-critic-lens.md | 2 +- .../ci-personas/ci-security-lens.md | 2 +- .../skills/gitnexus-review/SKILL.md | 3 +- .../ci-personas/ci-adversarial-lens.md | 2 +- .../ci-personas/ci-blast-radius-lens.md | 2 +- .../ci-personas/ci-correctness-lens.md | 2 +- .../ci-personas/ci-coverage-lens.md | 2 +- .../ci-personas/ci-critic-lens.md | 2 +- .../ci-personas/ci-security-lens.md | 2 +- gitnexus/skills/gitnexus-review/SKILL.md | 3 +- .../ci-personas/ci-adversarial-lens.md | 2 +- .../ci-personas/ci-blast-radius-lens.md | 2 +- .../ci-personas/ci-correctness-lens.md | 2 +- .../ci-personas/ci-coverage-lens.md | 2 +- .../ci-personas/ci-critic-lens.md | 2 +- .../ci-personas/ci-security-lens.md | 2 +- .../test/unit/review-agent-workflow.test.ts | 458 ++++++++++++++++-- 32 files changed, 677 insertions(+), 141 deletions(-) create mode 100755 .github/scripts/npm-ci-retry.sh diff --git a/.claude/README-gitnexus-reviewer-swarm.md b/.claude/README-gitnexus-reviewer-swarm.md index 4706a2cb1..07cb5228f 100644 --- a/.claude/README-gitnexus-reviewer-swarm.md +++ b/.claude/README-gitnexus-reviewer-swarm.md @@ -29,6 +29,8 @@ lanes on Sonnet. - **Read-only.** Tools limited to Read/Grep/Glob/Bash, and every persona enforces an explicit permitted/prohibited Bash list. No agent edits files, commits, or posts. + This is the interactive swarm; the CI review agent's `ci-personas/` lanes are + narrower still — file reads plus the safe graph tools, no Grep/Glob/Bash. - **Evidence-grounded**; **missing visibility becomes verification work**; **manually invoked.** ## Editing diff --git a/.claude/skills/gitnexus-review/SKILL.md b/.claude/skills/gitnexus-review/SKILL.md index 90fe12396..9eabc426c 100644 --- a/.claude/skills/gitnexus-review/SKILL.md +++ b/.claude/skills/gitnexus-review/SKILL.md @@ -181,8 +181,7 @@ dropping anything without a concrete failing scenario. ### Swarm lanes Six dispatchable lane definitions ship with this skill in `ci-personas/` — -read-only reviewers restricted to Read/Glob/Grep plus the safe graph -tools. Five are finder lanes: `ci-correctness-lens`, `ci-security-lens`, +read-only reviewers restricted to file reads plus the safe graph tools. Five are finder lanes: `ci-correctness-lens`, `ci-security-lens`, `ci-blast-radius-lens`, `ci-coverage-lens`, and `ci-adversarial-lens` (which assumes the change is broken and constructs reachable failure scenarios the pattern checks miss). They carry the verification diff --git a/.claude/skills/gitnexus-review/ci-personas/ci-adversarial-lens.md b/.claude/skills/gitnexus-review/ci-personas/ci-adversarial-lens.md index c7d620afc..84d526cd2 100644 --- a/.claude/skills/gitnexus-review/ci-personas/ci-adversarial-lens.md +++ b/.claude/skills/gitnexus-review/ci-personas/ci-adversarial-lens.md @@ -1,7 +1,7 @@ --- name: ci-adversarial-lens description: CI review swarm lane. Assumes the change is broken and constructs concrete failure scenarios — races, hostile inputs, state corruption, abuse of new surfaces — verified against source and the GitNexus graph. Read-only; reports findings only. -tools: Read, Glob, Grep, mcp__gitnexus__query, mcp__gitnexus__context, mcp__gitnexus__impact, mcp__gitnexus__explain, mcp__gitnexus__pdg_query, mcp__gitnexus__trace, mcp__gitnexus__list_repos +tools: Read, mcp__gitnexus__query, mcp__gitnexus__context, mcp__gitnexus__impact, mcp__gitnexus__explain, mcp__gitnexus__pdg_query, mcp__gitnexus__trace, mcp__gitnexus__list_repos maxTurns: 12 --- diff --git a/.claude/skills/gitnexus-review/ci-personas/ci-blast-radius-lens.md b/.claude/skills/gitnexus-review/ci-personas/ci-blast-radius-lens.md index 65cf04771..e014d39dc 100644 --- a/.claude/skills/gitnexus-review/ci-personas/ci-blast-radius-lens.md +++ b/.claude/skills/gitnexus-review/ci-personas/ci-blast-radius-lens.md @@ -1,7 +1,7 @@ --- name: ci-blast-radius-lens description: CI review swarm lane. Maps a PR's blast radius — dependents outside the diff, API/route surface, schema and version constants, compatibility breaks — from the GitNexus graph. Read-only; reports findings only. -tools: Read, Glob, Grep, mcp__gitnexus__impact, mcp__gitnexus__api_impact, mcp__gitnexus__route_map, mcp__gitnexus__context, mcp__gitnexus__query, mcp__gitnexus__shape_check, mcp__gitnexus__tool_map, mcp__gitnexus__list_repos +tools: Read, mcp__gitnexus__impact, mcp__gitnexus__api_impact, mcp__gitnexus__route_map, mcp__gitnexus__context, mcp__gitnexus__query, mcp__gitnexus__shape_check, mcp__gitnexus__tool_map, mcp__gitnexus__list_repos maxTurns: 12 --- diff --git a/.claude/skills/gitnexus-review/ci-personas/ci-correctness-lens.md b/.claude/skills/gitnexus-review/ci-personas/ci-correctness-lens.md index 8de542079..1c2ef5ed9 100644 --- a/.claude/skills/gitnexus-review/ci-personas/ci-correctness-lens.md +++ b/.claude/skills/gitnexus-review/ci-personas/ci-correctness-lens.md @@ -1,7 +1,7 @@ --- name: ci-correctness-lens description: CI review swarm lane. Hunts logic errors, edge cases, contract breaks, and state bugs in the changed symbols of a PR, grounded in the GitNexus graph. Read-only; reports findings only. -tools: Read, Glob, Grep, mcp__gitnexus__query, mcp__gitnexus__context, mcp__gitnexus__impact, mcp__gitnexus__pdg_query, mcp__gitnexus__trace, mcp__gitnexus__list_repos +tools: Read, mcp__gitnexus__query, mcp__gitnexus__context, mcp__gitnexus__impact, mcp__gitnexus__pdg_query, mcp__gitnexus__trace, mcp__gitnexus__list_repos maxTurns: 12 --- diff --git a/.claude/skills/gitnexus-review/ci-personas/ci-coverage-lens.md b/.claude/skills/gitnexus-review/ci-personas/ci-coverage-lens.md index 55667ae91..faf2192f6 100644 --- a/.claude/skills/gitnexus-review/ci-personas/ci-coverage-lens.md +++ b/.claude/skills/gitnexus-review/ci-personas/ci-coverage-lens.md @@ -1,7 +1,7 @@ --- name: ci-coverage-lens description: CI review swarm lane. Judges whether a PR's changed behavior is actually tested — missing cases, weak assertions, stale baselines, drift guards — using the GitNexus graph's test linkage. Read-only; reports findings only. -tools: Read, Glob, Grep, mcp__gitnexus__query, mcp__gitnexus__context, mcp__gitnexus__impact, mcp__gitnexus__check, mcp__gitnexus__list_repos +tools: Read, mcp__gitnexus__query, mcp__gitnexus__context, mcp__gitnexus__impact, mcp__gitnexus__check, mcp__gitnexus__list_repos maxTurns: 12 --- diff --git a/.claude/skills/gitnexus-review/ci-personas/ci-critic-lens.md b/.claude/skills/gitnexus-review/ci-personas/ci-critic-lens.md index 4bd5017b0..d610f8f94 100644 --- a/.claude/skills/gitnexus-review/ci-personas/ci-critic-lens.md +++ b/.claude/skills/gitnexus-review/ci-personas/ci-critic-lens.md @@ -1,7 +1,7 @@ --- name: ci-critic-lens description: CI review swarm gate. Audits the orchestrator's draft review before publication — every finding anchored and concrete, severities calibrated, sections and verdict wording conformant, no generic filler. Returns PASS or a defect list; never rewrites the review. -tools: Read, Glob, Grep, mcp__gitnexus__context, mcp__gitnexus__query, mcp__gitnexus__list_repos +tools: Read, mcp__gitnexus__context, mcp__gitnexus__query, mcp__gitnexus__list_repos maxTurns: 6 --- diff --git a/.claude/skills/gitnexus-review/ci-personas/ci-security-lens.md b/.claude/skills/gitnexus-review/ci-personas/ci-security-lens.md index 5e643a6f9..e98180464 100644 --- a/.claude/skills/gitnexus-review/ci-personas/ci-security-lens.md +++ b/.claude/skills/gitnexus-review/ci-personas/ci-security-lens.md @@ -1,7 +1,7 @@ --- name: ci-security-lens description: CI review swarm lane. Audits a PR's changed trust boundaries — input handling, injection, unsafe parsing, secrets, workflow/config risk — with GitNexus taint and dependence evidence. Read-only; reports findings only. -tools: Read, Glob, Grep, mcp__gitnexus__query, mcp__gitnexus__context, mcp__gitnexus__explain, mcp__gitnexus__pdg_query, mcp__gitnexus__impact, mcp__gitnexus__list_repos +tools: Read, mcp__gitnexus__query, mcp__gitnexus__context, mcp__gitnexus__explain, mcp__gitnexus__pdg_query, mcp__gitnexus__impact, mcp__gitnexus__list_repos maxTurns: 12 --- diff --git a/.github/scripts/npm-ci-retry.sh b/.github/scripts/npm-ci-retry.sh new file mode 100755 index 000000000..58a3ae462 --- /dev/null +++ b/.github/scripts/npm-ci-retry.sh @@ -0,0 +1,40 @@ +#!/usr/bin/env bash +# Install a lock-pinned runtime, retrying only what a transient registry fault +# can change. `npm ci` re-creates node_modules from the committed lockfile and +# re-verifies every SHA-512 integrity on each attempt, so a retry can only +# reproduce the identical tree — never a different one. Each attempt is bounded +# so a hung registry cannot eat the job budget the model review needs. +# +# Usage: npm-ci-retry.sh