From b4830cb4537426851ba3a7a3e403002ca509f78b Mon Sep 17 00:00:00 2001 From: khsaurabh Date: Sat, 5 Sep 2026 14:19:57 +0530 Subject: [PATCH 1/3] docs(ce-code-review): synthetic returns carry independence_verified; interpreter-aware dependency evidence Follow-up to #1614 for the remaining halves of #1612: - finish-review.md: when the orchestrator serializes a synthetic reviewer return, it must copy independence_verified: true from the peer's on-disk artifact into the return entry - the findings-mechanics helper reads the field only from the return entry, so artifact-only carriage silently drops the peer from independent_reviewers. - subagent-template.md: new false-positive suppression class - dependency/ import findings must be verified under the project's own interpreter and the declared package's metadata (Requires-Dist), not a bare host interpreter import. --- .../references/finish-review.md | 2 +- .../references/subagent-template.md | 1 + .../ce-code-review-synthetic-return.test.ts | 26 +++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 tests/skills/ce-code-review-synthetic-return.test.ts diff --git a/skills/ce-code-review/references/finish-review.md b/skills/ce-code-review/references/finish-review.md index 04311e318..f5800d299 100644 --- a/skills/ce-code-review/references/finish-review.md +++ b/skills/ce-code-review/references/finish-review.md @@ -16,7 +16,7 @@ PY="$(for c in python3 python py; do command -v "$c" >/dev/null 2>&1 && "$c" -c Before the first helper run, load every available per-reviewer artifact and build a source-detail map keyed by reviewer plus the helper fingerprint: normalized `file`, string `line`, and whitespace-normalized lowercase `title`. The map owns each source finding's `why_it_matters` and `evidence`; compact returns are merge inputs, not final report objects. -Inspect the helper's `findings`, `pre_existing_findings`, and `suppressed_findings` for semantic duplicates that use different wording or nearby anchors, for the direct-dependency exception, and for settlement conflicts below. Merge only when candidates describe the same defect and fix path. If semantic reconciliation, direct-dependency reclassification, or settlement stamping changed the set, serialize every reconciled candidate from all three partitions as one valid synthetic reviewer return and run the helper again to restore deterministic gates, partitions, sort order, and numbering. Never ask the helper to decide semantic equivalence or settlement conflicts. When reconciling a semantic duplicate, carry its original source-map keys alongside the candidate in working memory so detail hydration does not depend on the rewritten title. The helper's deterministic `suppressed_findings` partition is not primary review output; after settlement reconciliation, inspect it for the soft-bucket route below, then discard the remainder while preserving `suppressed_by_confidence` counts. +Inspect the helper's `findings`, `pre_existing_findings`, and `suppressed_findings` for semantic duplicates that use different wording or nearby anchors, for the direct-dependency exception, and for settlement conflicts below. Merge only when candidates describe the same defect and fix path. If semantic reconciliation, direct-dependency reclassification, or settlement stamping changed the set, serialize every reconciled candidate from all three partitions as one valid synthetic reviewer return and run the helper again to restore deterministic gates, partitions, sort order, and numbering. When the synthetic return carries an `adversarial-` entry, copy `independence_verified: true` from that peer's on-disk artifact into the return entry itself: the helper reads the field only from the return entry, so leaving it in the artifact alone silently drops the peer from `independent_reviewers`. Never ask the helper to decide semantic equivalence or settlement conflicts. When reconciling a semantic duplicate, carry its original source-map keys alongside the candidate in working memory so detail hydration does not depend on the rewritten title. The helper's deterministic `suppressed_findings` partition is not primary review output; after settlement reconciliation, inspect it for the soft-bucket route below, then discard the remainder while preserving `suppressed_by_confidence` counts. Then apply only the judgment the helper cannot own: diff --git a/skills/ce-code-review/references/subagent-template.md b/skills/ce-code-review/references/subagent-template.md index efd9a903e..130033cef 100644 --- a/skills/ce-code-review/references/subagent-template.md +++ b/skills/ce-code-review/references/subagent-template.md @@ -135,6 +135,7 @@ False-positive categories to actively suppress. Do NOT emit a finding when any o - **Generic "consider adding" advice without a concrete failure mode.** If you cannot name what breaks, the finding is not actionable. Either find the failure mode or suppress. - **Issues with a relevant lint-ignore comment.** Code that carries an explicit lint disable comment for the rule you are about to flag (`eslint-disable-next-line no-unused-vars`, `# rubocop:disable Style/StringLiterals`, `# noqa: E501`, etc.) — suppress unless the suppression itself violates a project-standards rule that explicitly forbids disabling that lint for this code shape. The author already chose to suppress; re-flagging it via a different reviewer creates noise and ignores their decision. - **General code-quality concerns with no rule behind them.** "This file is getting long," "this method has too many parameters," "this is hard to read" — without a rule from one of the criteria files this review designated to anchor the concern, these are subjective and waste reviewer time. When a criteria file does state the limit, that is a project-standards finding; otherwise suppress. +- **Dependency or import mismatches verified under the wrong interpreter.** Before filing that a declared dependency is missing or an import is broken, run the project's documented command under the interpreter the project actually uses (its virtualenv, toolchain manager, or container), and check the declared package's own metadata (e.g. `Requires-Dist`) for transitive provision. A bare import failing under the host's default `python3` / `node` proves nothing about the project's environment - flag the invocation that used the wrong interpreter instead, if one exists. - **Speculative future-work concerns with no current signal.** "This might break under load," "what if the requirements change," "this could be hard to test later" — not findings unless the diff introduces concrete evidence the concern is reachable now. **Advisory observations — route to advisory autofix_class, do not force a decision.** If the honest answer to "what actually breaks if we do not fix this?" is "nothing breaks, but…", the finding is advisory. Set `autofix_class: advisory` and `confidence: 50` so synthesis routes the finding to a soft bucket rather than surfacing it as a primary action item. Do not suppress — the observation may have value; it just does not warrant user judgment. Typical advisory shapes: design asymmetry the PR improves but does not fully resolve, opportunity to consolidate two similar helpers when neither is broken, residual risk worth noting in the report. diff --git a/tests/skills/ce-code-review-synthetic-return.test.ts b/tests/skills/ce-code-review-synthetic-return.test.ts new file mode 100644 index 000000000..8c939954c --- /dev/null +++ b/tests/skills/ce-code-review-synthetic-return.test.ts @@ -0,0 +1,26 @@ +import { readFileSync } from "fs" +import path from "path" +import { describe, expect, test } from "bun:test" + +const REF_DIR = path.join(process.cwd(), "skills/ce-code-review/references") +const FINISH_BODY = readFileSync(path.join(REF_DIR, "finish-review.md"), "utf8") +const TEMPLATE_BODY = readFileSync(path.join(REF_DIR, "subagent-template.md"), "utf8") + +// Issue #1612 follow-ups (the halves #1614 did not cover): a synthetic or +// reconciled reviewer return must carry the peer's independence_verified in +// the return entry itself, and dependency/import findings must be verified +// under the project's own interpreter before filing. +describe("ce-code-review synthetic return and evidence discipline", () => { + test("finish-review requires copying independence_verified into the synthetic return entry", () => { + expect(FINISH_BODY).toMatch(/copy `independence_verified: true`.*on-disk artifact.*return entry/s) + expect(FINISH_BODY).toMatch(/helper reads the field only from the return entry/) + expect(FINISH_BODY).toMatch(/artifact alone/) + }) + + test("subagent-template requires interpreter-aware dependency verification before filing", () => { + expect(TEMPLATE_BODY).toMatch(/[Dd]ependency or import mismatches verified under the wrong interpreter/) + expect(TEMPLATE_BODY).toMatch(/interpreter the project actually uses/) + expect(TEMPLATE_BODY).toMatch(/Requires-Dist/) + expect(TEMPLATE_BODY).toMatch(/host's default/) + }) +}) From b7220dfc5fb76126d121844cc1c6f62d4b9167b0 Mon Sep 17 00:00:00 2001 From: khsaurabh Date: Sat, 5 Sep 2026 15:17:18 +0530 Subject: [PATCH 2/3] fix(ce-code-review): pin synthetic-return independence at the finding level Codex review on #1633 was right: findings-mechanics.py handles a synthesis return by reading independence only from each finding's independent_reviewers (intersected with reviewers); the return-level independence_verified flag is consulted only for returns whose reviewer starts with adversarial-. The prior wording told the reconciler to copy the flag onto the synthetic return entry, which the helper ignores. Restate the rule at the layer the helper actually reads: keep the verified peer in the finding's reviewers and add it to independent_reviewers only when the peer's on-disk artifact verifies independence. --- skills/ce-code-review/references/finish-review.md | 2 +- .../ce-code-review-synthetic-return.test.ts | 15 ++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/skills/ce-code-review/references/finish-review.md b/skills/ce-code-review/references/finish-review.md index f5800d299..1f2cc168d 100644 --- a/skills/ce-code-review/references/finish-review.md +++ b/skills/ce-code-review/references/finish-review.md @@ -16,7 +16,7 @@ PY="$(for c in python3 python py; do command -v "$c" >/dev/null 2>&1 && "$c" -c Before the first helper run, load every available per-reviewer artifact and build a source-detail map keyed by reviewer plus the helper fingerprint: normalized `file`, string `line`, and whitespace-normalized lowercase `title`. The map owns each source finding's `why_it_matters` and `evidence`; compact returns are merge inputs, not final report objects. -Inspect the helper's `findings`, `pre_existing_findings`, and `suppressed_findings` for semantic duplicates that use different wording or nearby anchors, for the direct-dependency exception, and for settlement conflicts below. Merge only when candidates describe the same defect and fix path. If semantic reconciliation, direct-dependency reclassification, or settlement stamping changed the set, serialize every reconciled candidate from all three partitions as one valid synthetic reviewer return and run the helper again to restore deterministic gates, partitions, sort order, and numbering. When the synthetic return carries an `adversarial-` entry, copy `independence_verified: true` from that peer's on-disk artifact into the return entry itself: the helper reads the field only from the return entry, so leaving it in the artifact alone silently drops the peer from `independent_reviewers`. Never ask the helper to decide semantic equivalence or settlement conflicts. When reconciling a semantic duplicate, carry its original source-map keys alongside the candidate in working memory so detail hydration does not depend on the rewritten title. The helper's deterministic `suppressed_findings` partition is not primary review output; after settlement reconciliation, inspect it for the soft-bucket route below, then discard the remainder while preserving `suppressed_by_confidence` counts. +Inspect the helper's `findings`, `pre_existing_findings`, and `suppressed_findings` for semantic duplicates that use different wording or nearby anchors, for the direct-dependency exception, and for settlement conflicts below. Merge only when candidates describe the same defect and fix path. If semantic reconciliation, direct-dependency reclassification, or settlement stamping changed the set, serialize every reconciled candidate from all three partitions as one valid synthetic reviewer return and run the helper again to restore deterministic gates, partitions, sort order, and numbering. When the reconciled set includes a finding corroborated by an `adversarial-` peer, keep that peer in the finding's `reviewers` and, only when the peer's on-disk artifact carries `independence_verified: true`, add it to the finding's `independent_reviewers`: the helper derives a synthetic return's independence solely from that finding-level list, so a return-level flag is ignored there and the peer silently stays non-independent without it. Never ask the helper to decide semantic equivalence or settlement conflicts. When reconciling a semantic duplicate, carry its original source-map keys alongside the candidate in working memory so detail hydration does not depend on the rewritten title. The helper's deterministic `suppressed_findings` partition is not primary review output; after settlement reconciliation, inspect it for the soft-bucket route below, then discard the remainder while preserving `suppressed_by_confidence` counts. Then apply only the judgment the helper cannot own: diff --git a/tests/skills/ce-code-review-synthetic-return.test.ts b/tests/skills/ce-code-review-synthetic-return.test.ts index 8c939954c..0b4b5ff27 100644 --- a/tests/skills/ce-code-review-synthetic-return.test.ts +++ b/tests/skills/ce-code-review-synthetic-return.test.ts @@ -7,14 +7,15 @@ const FINISH_BODY = readFileSync(path.join(REF_DIR, "finish-review.md"), "utf8") const TEMPLATE_BODY = readFileSync(path.join(REF_DIR, "subagent-template.md"), "utf8") // Issue #1612 follow-ups (the halves #1614 did not cover): a synthetic or -// reconciled reviewer return must carry the peer's independence_verified in -// the return entry itself, and dependency/import findings must be verified -// under the project's own interpreter before filing. +// reconciled reviewer return preserves a verified adversarial peer's +// independence at the finding level, and dependency/import findings must be +// verified under the project's own interpreter before filing. describe("ce-code-review synthetic return and evidence discipline", () => { - test("finish-review requires copying independence_verified into the synthetic return entry", () => { - expect(FINISH_BODY).toMatch(/copy `independence_verified: true`.*on-disk artifact.*return entry/s) - expect(FINISH_BODY).toMatch(/helper reads the field only from the return entry/) - expect(FINISH_BODY).toMatch(/artifact alone/) + test("finish-review requires finding-level independence preservation for verified adversarial peers", () => { + expect(FINISH_BODY).toMatch(/keep that peer in the finding's `reviewers`/) + expect(FINISH_BODY).toMatch(/`independence_verified: true`.*add it to the finding's `independent_reviewers`/s) + expect(FINISH_BODY).toMatch(/derives a synthetic return's independence solely from that finding-level list/) + expect(FINISH_BODY).toMatch(/return-level flag is ignored/) }) test("subagent-template requires interpreter-aware dependency verification before filing", () => { From 010f4ccf02680f254257901992810dc224c39a34 Mon Sep 17 00:00:00 2001 From: khsaurabh Date: Sat, 5 Sep 2026 17:59:49 +0530 Subject: [PATCH 3/3] chore: re-trigger CI (runner/test flake unrelated to diff)