Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion skills/ce-code-review/references/finish-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 reconciled set includes a finding corroborated by an `adversarial-<provider>` 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:

Expand Down
1 change: 1 addition & 0 deletions skills/ce-code-review/references/subagent-template.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
27 changes: 27 additions & 0 deletions tests/skills/ce-code-review-synthetic-return.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
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 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 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", () => {
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/)
})
})