From 8d532cc4ef56ff7f7a3ef0ff921aca2922d87779 Mon Sep 17 00:00:00 2001 From: idy Date: Wed, 23 Sep 2026 00:37:22 +0800 Subject: [PATCH] fix: Revalidate stale Issue relationship blockers Incremental Issue-stage input carried only changed fields, so a previous blocker about unchanged native metadata (for example a wrong "parent: null" claim) could never clear. Always supply the current relationship fields and deterministic relationship checks, check a body `- Parent: #N` line against the native parent deterministically, and instruct the reviewer to drop previous blockers the current values contradict. Bump the Issue stage input version so cached evidence is revalidated once. Co-Authored-By: Claude Opus 5 --- .github/scripts/issue-review/common.mjs | 53 +++++++++++++++ .github/scripts/issue-review/test.mjs | 41 ++++++++++++ .github/scripts/pr-review/run.mjs | 7 ++ .github/scripts/pr-review/stages.mjs | 25 ++++++++ .github/scripts/pr-review/test.mjs | 85 ++++++++++++++++++++++++- README.md | 11 ++++ 6 files changed, 220 insertions(+), 2 deletions(-) diff --git a/.github/scripts/issue-review/common.mjs b/.github/scripts/issue-review/common.mjs index f318993..7bb495c 100644 --- a/.github/scripts/issue-review/common.mjs +++ b/.github/scripts/issue-review/common.mjs @@ -100,6 +100,22 @@ export function analyzeIssue(issue) { )); } + for (const check of issueRelationshipChecks(snapshot)) { + if (check.status === "pass") continue; + const declared = check.declared_parent_numbers + .map((number) => `#${number}`) + .join(", "); + blockers.push(check.native_parent_number == null + ? blocker( + "parent-relationship-missing", + `The Issue body declares parent ${declared}, but the Issue has no native parent.`, + ) + : blocker( + "parent-relationship-mismatch", + `The Issue body declares parent ${declared}, but its native parent is #${check.native_parent_number}.`, + )); + } + return { schema_version: ISSUE_REVIEW_SCHEMA_VERSION, snapshot, @@ -107,3 +123,40 @@ export function analyzeIssue(issue) { deterministic_blockers: blockers, }; } + +const PARENT_DECLARATION = + /^[ \t]*[-*][ \t]+Parent:[ \t]*(?:([A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+))?#(\d+)\b/gim; + +function declaredParents(snapshot) { + // Fenced examples are documentation, not a relationship declaration. + const body = snapshot.body.replace(/^[ \t]*(```|~~~)[\s\S]*?^[ \t]*\1[^\n]*$/gm, ""); + const numbers = new Set(); + for (const match of body.matchAll(PARENT_DECLARATION)) { + if ( + match[1] + && match[1].toLowerCase() !== snapshot.repository.toLowerCase() + ) { + continue; + } + numbers.add(Number(match[2])); + } + return [...numbers].sort((left, right) => left - right); +} + +// Native relationship facts the reviewer must not reinterpret: a body +// `- Parent: #N` line is compared with GitHub's native parent here instead of +// being left to a model that may misread (or keep re-emitting) the metadata. +export function issueRelationshipChecks(issue) { + const snapshot = issueSnapshot(issue); + const declared = declaredParents(snapshot); + if (declared.length === 0) return []; + const status = declared.length === 1 && declared[0] === snapshot.parent_number + ? "pass" + : "fail"; + return [{ + check: "body-parent-matches-native-parent", + status, + declared_parent_numbers: declared, + native_parent_number: snapshot.parent_number, + }]; +} diff --git a/.github/scripts/issue-review/test.mjs b/.github/scripts/issue-review/test.mjs index 2efc4a8..6253f95 100644 --- a/.github/scripts/issue-review/test.mjs +++ b/.github/scripts/issue-review/test.mjs @@ -3,6 +3,7 @@ import assert from "node:assert/strict"; import { analyzeIssue, + issueRelationshipChecks, issueSnapshot, issueSnapshotSha256, ISSUE_REVIEW_SCHEMA_VERSION, @@ -155,4 +156,44 @@ assert.deepEqual(analyzeIssue({ body: "Any project-defined Issue structure is accepted deterministically.", }).deterministic_blockers, []); +// A body `- Parent: #N` line is checked against the native parent +// deterministically instead of being left to the model. +const withParentLine = { ...valid, body: `- Parent: #1358\n\n${validBody}` }; +assert.deepEqual(issueRelationshipChecks(valid), []); +assert.deepEqual( + analyzeIssue({ ...withParentLine, parent_number: 1358 }).deterministic_blockers, + [], +); +assert.deepEqual(issueRelationshipChecks({ + ...withParentLine, + parent_number: 1358, +}), [{ + check: "body-parent-matches-native-parent", + status: "pass", + declared_parent_numbers: [1358], + native_parent_number: 1358, +}]); +assert.deepEqual( + analyzeIssue(withParentLine).deterministic_blockers.map((item) => item.code), + ["parent-relationship-missing"], +); +assert.deepEqual( + analyzeIssue({ ...withParentLine, parent_number: 7 }) + .deterministic_blockers.map((item) => item.code), + ["parent-relationship-mismatch"], +); +assert.deepEqual(analyzeIssue({ + ...valid, + body: `- Parent: GizClaw/example#1358\n\n${validBody}`, + parent_number: 1358, +}).deterministic_blockers, []); +assert.deepEqual(issueRelationshipChecks({ + ...valid, + body: `- Parent: other/repo#5\n\n${validBody}`, +}), []); +assert.deepEqual(issueRelationshipChecks({ + ...valid, + body: `\`\`\`markdown\n- Parent: #5\n\`\`\`\n\n${validBody}`, +}), []); + process.stdout.write("issue-review tests passed\n"); diff --git a/.github/scripts/pr-review/run.mjs b/.github/scripts/pr-review/run.mjs index 2e1c51c..98e43b6 100644 --- a/.github/scripts/pr-review/run.mjs +++ b/.github/scripts/pr-review/run.mjs @@ -13,8 +13,11 @@ import { usageFromSession, writeJson, } from "./common.mjs"; +import { issueRelationshipChecks } from "../issue-review/common.mjs"; import { emptyMetrics, + ISSUE_STAGE_INPUT_VERSION, + issueRelationships, snapshotDiff, stageIdentity, stageSha256, @@ -416,6 +419,7 @@ try { snapshot: { issue, trusted_base_sha: trustedBaseSha, + input_version: ISSUE_STAGE_INPUT_VERSION, }, policySha256: stagePolicySha("issue"), model, @@ -465,6 +469,8 @@ try { mode: issueMode, current_identity: issueIdentity, change: delta, + current_relationships: issueRelationships(issue), + relationship_checks: issueRelationshipChecks(issue), previous_result: resumableSession ? previousIssue?.result ?? null : null, deterministic_blockers: @@ -498,6 +504,7 @@ try { "Treat the trusted-base AGENTS.md hierarchy and the repository documents it designates as the authoritative project Issue contract. Apply additional trusted caller instructions where they do not contradict that repository contract. Do not impose a built-in title, Issue Type, section list, relationship format, or tracking-versus-implementation convention unless the trusted project policy or caller instructions require it.", "Assess repository fit and implementation readiness. Verify ownership and module boundaries, committed/generated surfaces, APIs or interfaces, runtime and lifecycle behavior, error handling and cleanup, platform differences, dependencies, and observable acceptance criteria whenever relevant. A competent implementer must be able to start without unresolved product or architecture decisions.", "If required evidence is absent or multiple designs remain plausible, return a blocker describing the missing decision or an Open Design Question. Do not invent product behavior, paths, APIs, storage formats, hardware behavior, migrations, or compatibility guarantees. Do not assess whether other Issues are present in the PR Close Set; deterministic PR linkage owns that decision. Do not review the PR body or code in this stage. Treat supplied deterministic blockers as already reported, and preserve only still-applicable previous model blockers when the input is incremental.", + "current_relationships holds the authoritative current native Issue Type, state, parent, sub-issues, and dependencies even when the change omits them, and relationship_checks holds deterministic results (a body `- Parent: #N` line against the native parent is owned by those checks; never report it yourself). Re-validate every previous blocker against current_relationships, relationship_checks, and the change before preserving it, and drop any previous blocker they contradict, such as a missing-parent claim when parent_number is set.", "Return only the JSON object required by the stage output schema.", ].join("\n"), }); diff --git a/.github/scripts/pr-review/stages.mjs b/.github/scripts/pr-review/stages.mjs index d455292..93871b3 100644 --- a/.github/scripts/pr-review/stages.mjs +++ b/.github/scripts/pr-review/stages.mjs @@ -116,3 +116,28 @@ export function totalMetrics(rows) { : totals.cached_input_tokens / totals.input_tokens; return totals; } + +// Bumped when the Issue stage input changes shape, so cached Issue evidence +// (which may hold a blocker the older input could never clear) is revalidated +// once as an incremental turn instead of being reused indefinitely. +export const ISSUE_STAGE_INPUT_VERSION = 2; + +export const ISSUE_RELATIONSHIP_FIELDS = Object.freeze([ + "issue_type", + "state", + "parent_number", + "sub_issue_numbers", + "sub_issues", + "blocked_by", + "blocking", +]); + +// A field-level delta omits unchanged metadata, so a previous blocker about a +// relationship that never changes could never be re-validated. Always supply +// the current values next to the delta. +export function issueRelationships(snapshot) { + return Object.fromEntries(ISSUE_RELATIONSHIP_FIELDS.map((field) => [ + field, + snapshot?.[field] ?? null, + ])); +} diff --git a/.github/scripts/pr-review/test.mjs b/.github/scripts/pr-review/test.mjs index ee6f8d7..aff4336 100644 --- a/.github/scripts/pr-review/test.mjs +++ b/.github/scripts/pr-review/test.mjs @@ -16,6 +16,7 @@ import { } from "./common.mjs"; import { emptyMetrics, + issueRelationships, snapshotDiff, stageIdentity, totalMetrics, @@ -75,6 +76,23 @@ assert.deepEqual( }], }, ); +{ + // A previous blocker about unchanged relationship metadata must be + // re-validatable: the delta omits parent_number, the current values do not. + const previous = { body: "Plan.", parent_number: 1358, sub_issue_numbers: [] }; + const current = { ...previous, body: "Plan.\nMore." }; + const delta = snapshotDiff(previous, current); + assert.deepEqual(delta.changes.map((change) => change.field), ["body"]); + assert.deepEqual(issueRelationships(current), { + issue_type: null, + state: null, + parent_number: 1358, + sub_issue_numbers: [], + sub_issues: null, + blocked_by: null, + blocking: null, + }); +} assert.deepEqual( stageIdentity({ stage: "pr", @@ -782,6 +800,7 @@ fs.appendFileSync(sessionFile, JSON.stringify({ const prompt = fs.readFileSync(0, "utf8"); if (process.env.FAKE_PROMPT) fs.writeFileSync(process.env.FAKE_PROMPT, prompt); const turn = path.basename(outputFile); +if (process.env.FAKE_PROMPTS) fs.writeFileSync(path.join(process.env.FAKE_PROMPTS, turn + ".prompt"), prompt); if (process.env.FAKE_TRACE) fs.appendFileSync(process.env.FAKE_TRACE, turn + "\\n"); const result = schemaFile.endsWith("stage-output-schema.json") ? { summary: "Fake stage review complete.", blockers: [] } @@ -1152,14 +1171,40 @@ fs.writeFileSync(outputFile, JSON.stringify(result)); ], ); + // Reproduce a stale full-review blocker: the previous result claims the + // Issue has no native parent although parent_number was already set and + // never changes. The incremental input must carry the current parent so the + // blocker can clear. + const ledgerFile = path.join(state, "review-ledger.json"); + const issueChangedPrompts = path.join(temporary, "issue-changed-prompts"); + fs.mkdirSync(issueChangedPrompts); + const staleLedger = JSON.parse(fs.readFileSync(ledgerFile, "utf8")); + const staleIssue = staleLedger.stage_evidence.issues["example/repo#1"]; + staleIssue.snapshot.parent_number = 9; + staleIssue.snapshot.body = `- Parent: #9\n\n${staleIssue.snapshot.body}`; + staleIssue.result.blockers = [{ + code: "missing-parent", + title: "Missing native parent", + body: "The supplied full Issue metadata has `parent: null`.", + }]; + fs.writeFileSync(ledgerFile, JSON.stringify(staleLedger)); const changedContext = JSON.parse(fs.readFileSync(contextFile, "utf8")); - changedContext.readiness.snapshot.linked_issues[0].snapshot.body += - "\n\nAcceptance detail changed."; + const changedIssue = changedContext.readiness.snapshot.linked_issues[0].snapshot; + changedIssue.parent_number = 9; + changedIssue.body = `- Parent: #9\n\n${changedIssue.body}` + + "\n\nAcceptance detail changed."; changedContext.readiness.snapshot.linked_issues[0].snapshot_sha256 = "issue-v2"; fs.writeFileSync( contextFile, `${JSON.stringify(changedContext, null, 2)}\n`, ); + const stageInputsDir = path.join( + state, + "generations", + latestGeneration.key, + "stage-inputs", + ); + const stageInputsBefore = fs.readdirSync(stageInputsDir); const issueChangedOutput = path.join(temporary, "issue-changed-output"); const issueChangedResult = spawnSync(process.execPath, [ path.join(path.dirname(new URL(import.meta.url).pathname), "run.mjs"), @@ -1189,9 +1234,45 @@ fs.writeFileSync(outputFile, JSON.stringify(result)); REVIEW_INSTRUCTIONS: "Review the diff.", ISSUE_REVIEW_INSTRUCTIONS: "Review the Issue.", PR_REVIEW_INSTRUCTIONS: "Review PR readiness.", + FAKE_PROMPTS: issueChangedPrompts, }, }); assert.equal(issueChangedResult.status, 0, issueChangedResult.stderr); + const issueStageInputName = fs.readdirSync(stageInputsDir).find((name) => ( + name.startsWith("issue-1-") && !stageInputsBefore.includes(name) + )); + const issueStageInput = JSON.parse(fs.readFileSync( + path.join(stageInputsDir, issueStageInputName), + "utf8", + )); + assert.equal(issueStageInput.mode, "incremental"); + assert.deepEqual( + issueStageInput.change.changes.map((change) => change.field), + ["body"], + ); + assert.equal(issueStageInput.current_relationships.parent_number, 9); + assert.deepEqual(issueStageInput.relationship_checks, [{ + check: "body-parent-matches-native-parent", + status: "pass", + declared_parent_numbers: [9], + native_parent_number: 9, + }]); + assert.equal( + issueStageInput.previous_result.blockers[0].code, + "missing-parent", + ); + const issuePrompt = fs.readFileSync(path.join( + issueChangedPrompts, + `stage-${issueStageInputName}.prompt`, + ), "utf8"); + assert.match(issuePrompt, /current_relationships holds the authoritative/); + assert.match(issuePrompt, /drop any previous blocker they contradict/); + // The fake reviewer returns no blockers, so the stale one clears. + const clearedLedger = JSON.parse(fs.readFileSync(ledgerFile, "utf8")); + assert.deepEqual( + clearedLedger.stage_evidence.issues["example/repo#1"].result.blockers, + [], + ); const issueChangedOutputs = fs.readFileSync(issueChangedOutput, "utf8"); assert.match(issueChangedOutputs, /^input_tokens=200$/m); const issueChangedUsageLine = issueChangedOutputs diff --git a/README.md b/README.md index a8d2666..e30cb8d 100644 --- a/README.md +++ b/README.md @@ -164,6 +164,9 @@ linkage requirements: - The PR has at least one same-repository native closing Issue from GraphQL `closingIssuesReferences`; text-only references do not count. - Issue and sub-issue snapshots must be complete. +- When an Issue body declares `- Parent: #N` (or `owner/repo#N` for the same + repository), GitHub's native parent must be that Issue. This is checked + deterministically, never left to the model. - The complete PR result follows the current Issue plan. Material deviations must be reflected in the Issue or disclosed and resolved in the PR. @@ -250,6 +253,14 @@ reviews; the previous code review is offered for preservation to incremental generations alone, so findings about an earlier range cannot outlive the diff that produced them. +Every Issue-stage turn, including an incremental one, also receives the +current native Issue Type, state, parent, sub-issues, and dependencies plus the +deterministic relationship checks. The reviewer re-validates previous blockers +against those current values and drops any they contradict, so a wrong blocker +about metadata that never changes cannot outlive its evidence. Changing that +input shape invalidates cached Issue evidence once, revalidating it as an +incremental turn. + An Issue edit revalidates plan conformance without resending an unchanged complete code diff. A runtime, model, or trusted-policy change intentionally invalidates incompatible session evidence and safely starts the affected