diff --git a/.github/workflows/gitnexus-review-agent.yml b/.github/workflows/gitnexus-review-agent.yml index e4c643aa9..039e772fb 100644 --- a/.github/workflows/gitnexus-review-agent.yml +++ b/.github/workflows/gitnexus-review-agent.yml @@ -1335,6 +1335,12 @@ jobs: Always end the run by returning that structured body field, even when a lane fails, a query comes back empty, or the analysis is incomplete — describe the gap inside the review instead of finishing without output. + Also return the boolean field complete: true only when you actually + finished the review you were asked for, and false whenever a lane + failed, a needed query never resolved, or you ran out of turns. A + false value still publishes the partial review, but labelled as + incomplete rather than accepted — never report true to make the run + look clean. claude_args: | --model claude-sonnet-5 --add-dir "${{ runner.temp }}/gitnexus-review-pr-target" @@ -1343,12 +1349,12 @@ jobs: --strict-mcp-config --mcp-config "${{ runner.temp }}/gitnexus-review-mcp.json" --tools "Read,Agent" - --allowedTools "Agent(ci-correctness-lens,ci-security-lens,ci-blast-radius-lens,ci-coverage-lens,ci-adversarial-lens,ci-critic-lens),Read(./**),Read(${{ runner.temp }}/gitnexus-review-pr-target/**),Read(${{ runner.temp }}/gitnexus-review-merge-base/**),mcp__gitnexus__list_repos,mcp__gitnexus__query,mcp__gitnexus__context,mcp__gitnexus__check,mcp__gitnexus__impact,mcp__gitnexus__explain,mcp__gitnexus__pdg_query,mcp__gitnexus__route_map,mcp__gitnexus__tool_map,mcp__gitnexus__shape_check,mcp__gitnexus__api_impact,mcp__gitnexus__trace" + --allowedTools "Agent(ci-correctness-lens),Agent(ci-security-lens),Agent(ci-blast-radius-lens),Agent(ci-coverage-lens),Agent(ci-adversarial-lens),Agent(ci-critic-lens),Read(./**),Read(${{ runner.temp }}/gitnexus-review-pr-target/**),Read(${{ runner.temp }}/gitnexus-review-merge-base/**),mcp__gitnexus__list_repos,mcp__gitnexus__query,mcp__gitnexus__context,mcp__gitnexus__check,mcp__gitnexus__impact,mcp__gitnexus__explain,mcp__gitnexus__pdg_query,mcp__gitnexus__route_map,mcp__gitnexus__tool_map,mcp__gitnexus__shape_check,mcp__gitnexus__api_impact,mcp__gitnexus__trace" --disallowedTools "Bash,Write,Edit,MultiEdit,NotebookEdit,WebFetch,WebSearch,Skill,Read(/proc/**),Read(/sys/**),Read(/dev/**),Read(${{ github.workspace }}/**),mcp__github,mcp__gitnexus__detect_changes,mcp__gitnexus__rename,mcp__gitnexus__cypher,mcp__gitnexus__group_list,mcp__gitnexus__group_sync" --permission-mode dontAsk --no-session-persistence --max-turns 150 - --json-schema '{"type":"object","properties":{"body":{"type":"string","maxLength":50000}},"required":["body"],"additionalProperties":false}' + --json-schema '{"type":"object","properties":{"body":{"type":"string","maxLength":50000},"complete":{"type":"boolean"}},"required":["body","complete"],"additionalProperties":false}' - name: Assemble bounded review artifact id: artifact @@ -1413,6 +1419,8 @@ jobs: index_failed: 'The review was not run because the exact-head graph index could not be built safely.', model_failed: 'The review agent did not produce a valid structured result.', invalid_model_output: 'The review agent returned an invalid structured result.', + incomplete_analysis: + 'The review agent reported that it could not complete this analysis, so the partial review below is published for diagnosis rather than accepted as a review.', invalid_execution_transcript: 'The review execution transcript failed strict validation, so no model review was accepted.', missing_graph_evidence: 'The review execution did not prove a successful GitNexus context result for a symbol in an exact changed file.', }; @@ -1674,13 +1682,16 @@ jobs: const headRepo = path.join(process.env.GITHUB_WORKSPACE, 'pr-target'); const baseRepo = path.join(process.env.RUNNER_TEMP, 'gitnexus-review-merge-base'); - if (!Object.hasOwn(input, 'repo') || input.repo === headRepo) { - return changedPathManifest.headPaths; - } - if (input.repo === baseRepo) { - return changedPathManifest.baseEvidencePaths; - } - return undefined; + // An empty set can never be satisfied (a deletion-only PR has no + // head paths), so such a call is out of scope rather than a + // candidate whose every result reads as "outside the changed paths". + const scoped = + !Object.hasOwn(input, 'repo') || input.repo === headRepo + ? changedPathManifest.headPaths + : input.repo === baseRepo + ? changedPathManifest.baseEvidencePaths + : undefined; + return scoped && scoped.size > 0 ? scoped : undefined; } function validateToolResultContent(content) { @@ -1712,6 +1723,14 @@ jobs: throw new Error('context tool result is not text'); } + // Payload-shape failures are NOT transcript corruption. Every + // orchestrator context call is a candidate now, so an ordinary + // exploratory call whose result the MCP truncated at + // GITNEXUS_MCP_DEFAULT_MAX_TOKENS (mid-JSON, marker appended) would + // otherwise throw and discard a review an earlier call already + // proved. This throws only what the caller converts into a counted + // non-evidence result; structural transcript invariants still throw + // hard from proveGraphReview. function contextResultProvesChangedPath(content, eligiblePaths, rejected) { const text = decodeTextToolResult(content).trim(); if (!text) throw new Error('context tool result is empty'); @@ -1784,7 +1803,11 @@ jobs: samples: [], sidechainCalls: 0, outOfScopeCalls: 0, + erroredResults: 0, + malformedResults: 0, + unusableResults: 0, }; + const answeredCalls = new Set(); const candidateCalls = new Map(); const successfulResults = new Map(); const seenToolCalls = new Set(); @@ -1886,14 +1909,25 @@ jobs: } seenToolResults.add(block.tool_use_id); const candidate = candidateCalls.get(block.tool_use_id); - if ( - !sidechain && - block.is_error !== true && - candidate && - messageIndex > candidate.messageIndex && - contextResultProvesChangedPath(block.content, candidate.eligiblePaths, rejected) - ) { - successfulResults.set(block.tool_use_id, messageIndex); + if (candidate && (sidechain || messageIndex <= candidate.messageIndex)) { + rejected.unusableResults += 1; + } else if (candidate && block.is_error === true) { + rejected.erroredResults += 1; + } else if (candidate) { + answeredCalls.add(block.tool_use_id); + let proved = false; + try { + proved = contextResultProvesChangedPath( + block.content, + candidate.eligiblePaths, + rejected, + ); + } catch { + // A malformed or truncated payload means this call is not + // the evidence call — never that the transcript is corrupt. + rejected.malformedResults += 1; + } + if (proved) successfulResults.set(block.tool_use_id, messageIndex); } } } @@ -1911,7 +1945,12 @@ jobs: `orchestrator context calls out of scope (no selector or unknown repo): ` + `${rejected.outOfScopeCalls}; ` + `sidechain context calls ignored: ${rejected.sidechainCalls}; ` + + `in-scope calls with no usable result: ` + + `${candidateCalls.size - answeredCalls.size}` + + ` (errored ${rejected.erroredResults}, out of order or sidechained ` + + `${rejected.unusableResults}); ` + `results that resolved nothing: ${rejected.unresolved}; ` + + `results too malformed or truncated to parse: ${rejected.malformedResults}; ` + `results outside the changed paths: ${rejected.offPath}` + (rejected.samples.length > 0 ? ` (${rejected.samples.join(', ')})` : ''), headHasIndexableSymbol: @@ -1987,22 +2026,32 @@ jobs: if ( !parsed || Array.isArray(parsed) || - Object.keys(parsed).length !== 1 || + Object.keys(parsed).length !== 2 || typeof parsed.body !== 'string' || - parsed.body.trim().length === 0 + parsed.body.trim().length === 0 || + typeof parsed.complete !== 'boolean' ) { throw new Error('structured output shape mismatch'); } - status = 'success'; - failureCode = 'none'; - graphEvidenceMode = { - mode: graphEvidence.hasContextEvidence - ? 'context' - : 'no_indexable_changed_symbols', - head_has_indexable_symbol: graphEvidence.headHasIndexableSymbol, - base_has_indexable_symbol: graphEvidence.baseHasIndexableSymbol, - }; - body = parsed.body; + // The prompt asks for a body even when the analysis could + // not finish, so completeness must be reported separately — + // otherwise a degraded run publishes as an accepted review. + if (parsed.complete) { + status = 'success'; + failureCode = 'none'; + graphEvidenceMode = { + mode: graphEvidence.hasContextEvidence + ? 'context' + : 'no_indexable_changed_symbols', + head_has_indexable_symbol: graphEvidence.headHasIndexableSymbol, + base_has_indexable_symbol: graphEvidence.baseHasIndexableSymbol, + }; + body = parsed.body; + } else { + failureCode = 'incomplete_analysis'; + body = `${failureMessages.incomplete_analysis}\n\n${parsed.body}`; + console.error('Review rejected: the model reported an incomplete analysis.'); + } } catch { failureCode = 'invalid_model_output'; body = failureMessages[failureCode]; diff --git a/gitnexus/test/unit/review-agent-workflow.test.ts b/gitnexus/test/unit/review-agent-workflow.test.ts index 9cf1daf48..59cbb1c89 100644 --- a/gitnexus/test/unit/review-agent-workflow.test.ts +++ b/gitnexus/test/unit/review-agent-workflow.test.ts @@ -326,6 +326,68 @@ function reviewTranscript({ ]; } +// Two orchestrator context calls in one turn: the first proves the evidence, +// the second is an ordinary exploratory call whose result may be junk. +function twoCallTranscript({ + firstResult, + secondResult, +}: { + firstResult: string; + secondResult: string; +}): Array> { + return [ + { + type: 'system', + subtype: 'init', + session_id: 'session-1', + uuid: '11111111-1111-4111-8111-111111111111', + }, + { + type: 'assistant', + parent_tool_use_id: null, + session_id: 'session-1', + uuid: '22222222-2222-4222-8222-222222222222', + message: { + role: 'assistant', + content: [ + { + type: 'tool_use', + id: 'tool-1', + name: 'mcp__gitnexus__context', + input: { name: 'statusCommand' }, + }, + { + type: 'tool_use', + id: 'tool-2', + name: 'mcp__gitnexus__context', + input: { name: 'bigHotSymbol' }, + }, + ], + }, + }, + { + type: 'user', + parent_tool_use_id: null, + session_id: 'session-1', + uuid: '33333333-3333-4333-8333-333333333333', + message: { + role: 'user', + content: [ + { type: 'tool_result', tool_use_id: 'tool-1', is_error: false, content: firstResult }, + { type: 'tool_result', tool_use_id: 'tool-2', is_error: false, content: secondResult }, + ], + }, + }, + { + type: 'result', + subtype: 'success', + is_error: false, + session_id: 'session-1', + uuid: '44444444-4444-4444-8444-444444444444', + }, + ]; +} + function reviewTranscriptWithoutTools(): Array> { return [ { @@ -362,7 +424,7 @@ function runArtifactScenario({ executionFileOutput, noIndexableChangedSymbols = false, rawTranscript = JSON.stringify(reviewTranscript()), - structuredOutput = JSON.stringify({ body: 'Accepted graph-backed review' }), + structuredOutput = JSON.stringify({ body: 'Accepted graph-backed review', complete: true }), }: ArtifactScenario = {}) { const script = embeddedNodeScript('analyze', 'Assemble bounded review artifact'); const runnerTemp = mkdtempSync(path.join(tmpdir(), 'gitnexus-review-artifact-')); @@ -1175,9 +1237,13 @@ describe('gitnexus review-agent workflow security contract', () => { // control-SHA personas; Agent is not bare-denied (deny beats allow), and // lane calls cannot satisfy the evidence gate. expect(analyze).toContain('--tools "Read,Agent"'); - expect(allowedTools).toContain( - 'Agent(ci-correctness-lens,ci-security-lens,ci-blast-radius-lens,ci-coverage-lens,ci-adversarial-lens,ci-critic-lens)', - ); + // One rule per persona, never a grouped Agent(a,b,c): the pinned base + // action parses allowedTools with `.flatMap((v) => v.split(","))`, which + // would shatter a grouped rule into `Agent(ci-correctness-lens`, bare + // names, and `ci-critic-lens)` before the SDK ever sees it. + expect(allowedToolRules).toContain('Agent(ci-correctness-lens)'); + expect(allowedToolRules).toContain('Agent(ci-critic-lens)'); + expect(allowedTools).not.toMatch(/Agent\([^)]*,/); expect(allowedTools).not.toContain('Task'); const disallowedTools = analyze.match(/--disallowedTools "([^"]+)"/)?.[1] ?? ''; const disallowedToolRules = disallowedTools.split(','); @@ -1218,12 +1284,12 @@ describe('gitnexus review-agent workflow security contract', () => { // parse time), so the canary is the acceptance gate for that. What a unit // test CAN pin is that the scoped allowlist, the persona filenames, and each // persona's frontmatter name are the same set — catching a rename or typo in - // any of the three without auth. + // any of the three without auth. Names are read one-per-rule because the + // pinned action splits allowedTools on commas. const analyze = jobBlock('analyze'); const allowed = analyze.match(/--allowedTools "([^"]+)"/)?.[1] ?? ''; - const allowlistNames = (allowed.match(/Agent\(([^)]+)\)/)?.[1] ?? '') - .split(',') - .map((name) => name.trim()) + const allowlistNames = [...allowed.matchAll(/Agent\(([^),]+)\)/g)] + .map((match) => match[1].trim()) .sort(); const personasDir = path.resolve( @@ -1618,7 +1684,11 @@ describe('gitnexus review-agent workflow security contract', () => { }), ), }); - expect(uidOnly.artifact.failure_code).toBeNull(); + expect(uidOnly.artifact).toMatchObject({ + status: 'success', + failure_code: null, + graph_evidence: { mode: 'context' }, + }); const noSelector = runArtifactScenario({ rawTranscript: JSON.stringify(reviewTranscript({ toolInput: { kind: 'Function' } })), @@ -1659,6 +1729,92 @@ describe('gitnexus review-agent workflow security contract', () => { expect(noCalls.stderr).toContain( 'orchestrator context calls out of scope (no selector or unknown repo): 0', ); + + // An in-scope call whose result errored must not read as "no calls made". + const erroredResult = runArtifactScenario({ + rawTranscript: JSON.stringify(reviewTranscript({ resultIsError: true })), + }); + expect(erroredResult.stderr).toContain('in-scope calls with no usable result: 1 (errored 1'); + + const unresolved = runArtifactScenario({ + rawTranscript: JSON.stringify( + reviewTranscript({ + toolResultContent: `${JSON.stringify({ error: "Symbol 'x' not found" })}\n\n---\n**Next:** retry.`, + }), + ), + }); + expect(unresolved.stderr).toContain('results that resolved nothing: 1'); + }); + + it('treats a malformed context payload as non-evidence, not as a corrupt transcript', () => { + // The MCP truncates any context payload over GITNEXUS_MCP_DEFAULT_MAX_TOKENS + // mid-JSON. Every orchestrator context call is a candidate, so throwing on a + // payload-shape failure would let one truncated exploratory call discard a + // review that an earlier call already proved. + const proved = contextResultContent(); + const truncated = `${JSON.stringify({ status: 'found', symbol: { uid: 'u' } }).slice(0, 30)}\n…`; + const provedThenTruncated = runArtifactScenario({ + rawTranscript: JSON.stringify( + twoCallTranscript({ firstResult: proved, secondResult: truncated }), + ), + }); + expect(provedThenTruncated.artifact).toMatchObject({ + status: 'success', + failure_code: null, + graph_evidence: { mode: 'context' }, + }); + + // With no proving call, the same truncated payload is counted, not thrown. + const truncatedOnly = runArtifactScenario({ + rawTranscript: JSON.stringify(reviewTranscript({ toolResultContent: truncated })), + }); + expect(truncatedOnly.artifact.failure_code).toBe('missing_graph_evidence'); + expect(truncatedOnly.stderr).toContain('results too malformed or truncated to parse: 1'); + + // Structural transcript invariants must still fail closed. + const structural = runArtifactScenario({ rawTranscript: '{not-json' }); + expect(structural.artifact.failure_code).toBe('invalid_execution_transcript'); + }); + + it('scopes a deletion-only PR to the merge-base set instead of an empty head set', () => { + const deletedPath = 'gitnexus/src/cli/deleted-command.ts'; + const headScopedCall = runArtifactScenario({ + basePaths: [deletedPath], + changedPaths: [], + rawTranscript: JSON.stringify( + reviewTranscript({ + toolInput: { name: 'deletedCommand' }, + toolResultContent: contextResultContent(deletedPath), + }), + ), + }); + // headPaths is empty, so the call can never be satisfied: report it as out + // of scope rather than as a result "outside the changed paths". + expect(headScopedCall.artifact.failure_code).toBe('missing_graph_evidence'); + expect(headScopedCall.stderr).toContain('orchestrator context calls in scope: 0'); + expect(headScopedCall.stderr).toContain( + 'orchestrator context calls out of scope (no selector or unknown repo): 1', + ); + expect(headScopedCall.stderr).toContain('results outside the changed paths: 0'); + }); + + it('publishes an incomplete analysis as a labelled failure, never as an accepted review', () => { + const incomplete = runArtifactScenario({ + structuredOutput: JSON.stringify({ body: 'Partial review, two lanes died', complete: false }), + }); + expect(incomplete.artifact).toMatchObject({ + status: 'failure', + failure_code: 'incomplete_analysis', + graph_evidence: null, + }); + expect(incomplete.artifact.body).toContain('could not complete this analysis'); + expect(incomplete.artifact.body).toContain('Partial review, two lanes died'); + expect(incomplete.stderr).toContain('the model reported an incomplete analysis'); + + const missingField = runArtifactScenario({ + structuredOutput: JSON.stringify({ body: 'No completeness signal' }), + }); + expect(missingField.artifact.failure_code).toBe('invalid_model_output'); }); it('accepts SDK text-block results with omitted is_error', () => { @@ -1697,13 +1853,17 @@ describe('gitnexus review-agent workflow security contract', () => { expect(wrongPath.artifact.failure_code).toBe('missing_graph_evidence'); }); - it('fails closed on malformed or empty context result content', () => { + it('separates malformed context payloads from structurally invalid tool results', () => { + // Payload shape is the MCP's business and can fail for benign reasons + // (truncation at the output budget), so it demotes one call to non-evidence. const malformed = runArtifactScenario({ rawTranscript: JSON.stringify(reviewTranscript({ toolResultContent: '{not-json' })), }); - expect(malformed.artifact.failure_code).toBe('invalid_execution_transcript'); - expect(malformed.stderr).toContain('context tool result is not strict JSON'); + expect(malformed.artifact.failure_code).toBe('missing_graph_evidence'); + expect(malformed.stderr).toContain('results too malformed or truncated to parse: 1'); + // An empty tool_result is a transcript-structural violation, not a payload + // shape, and still fails the whole run closed. const empty = runArtifactScenario({ rawTranscript: JSON.stringify(reviewTranscript({ toolResultContent: ' ' })), });