fix(review): align optional finding IDs with candidate-causal admission

[decode2]

Pre-flight Checklist

• I searched open and closed issues for reviewer finding IDs, optional IDs, canonicalization, candidate-causal admission, proof references, frozen candidates, and changed-path manifests.
• I understand that no PR should be opened without maintainer approval.

Bug Description

Native reviewer capture accepts facadeFinding.id as optional (json:"id,omitempty"), but candidate-causal admission requires repository-derived finding IDs before canonical lens-result processing assigns deterministic IDs to omitted values.

For a valid candidate-causal finding that omits id, the native pipeline can derive an empty candidate-causal ID from the decoded provider result, then canonicalize the same finding to R1-001, R2-001, R3-001, or R4-001. AdmitArtifact compares the pre-canonical verified ID set with the post-canonical expected ID set and rejects the result as out of scope:

reviewer artifact admission out_of_scope: candidate-causal findings are not proven by repository-derived changed-line evidence

The public decoder therefore permits a shape that native semantic admission cannot successfully process for candidate-causal findings.

This is distinct from intended proof-scope validation. AdmitArtifact correctly rejects proof_refs that cite repository paths outside the frozen changed_path_manifest; callers should keep candidate proof inside the frozen candidate rather than weaken that check.

Steps to Reproduce

1. Start a negotiated native review with a frozen candidate containing one changed line and select any review lens.
2. Obtain the provider-owned artifact subject, candidate diff, and changed-path manifest through capture preflight.
3. Submit a reviewer result with completed provider-owned inspection and one candidate-causal CRITICAL finding whose location and proof reference point to the changed candidate line.
4. Omit only the finding id; keep evidence_class: "deterministic" and causal_disposition: "introduced".
5. Capture the result through gentle-ai review capture-result.
6. Observe that decoding succeeds because facadeFinding.ID is optional, canonicalization assigns a deterministic lens ID, but candidate-causal ID verification no longer matches and admission rejects the result.

Expected Behavior

The native boundary should have one coherent ownership rule for finding IDs:

• Either require every non-empty finding ID during decode/admission before repository-derived candidate-causal verification, with the canonical selected-lens prefix and uniqueness checks; or
• Canonicalize deterministic IDs before deriving and comparing repository-owned candidate-causal ID sets, while preserving the exact canonical payload and all existing admission checks.

Missing IDs must not be silently accepted by the schema and then rejected only because canonicalization occurs after causal verification.

Actual Behavior

facadeFinding.ID uses omitempty, canonicalLensResult deterministically supplies a missing ID, and AdmitArtifact compares candidate-causal IDs against the canonicalized result. A schema-valid reviewer payload is therefore unreplayable even though its location and proof are fully inside the frozen candidate.

Environment

• Gentle AI: 1.49.1-0.20260722122850-8881b60b01c7
• Operating System: Linux
• Agent / Client: OpenCode
• Shell: bash
• Affected Area: native review capture and artifact admission

Source Evidence

• internal/cli/review_facade.go: facadeFinding.ID string json:"id,omitempty".
• internal/reviewtransaction/transaction.go: canonicalLensResult assigns <lens-prefix>-<index> when finding.ID == "".
• internal/cli/review_artifact.go: repository-derived CandidateCausalFindingIDs are passed into admission.
• internal/reviewtransaction/artifact_admission.go: admission canonicalizes the lens result, derives candidate-causal IDs from canonical findings, and compares them with the previously verified IDs.

Acceptance Criteria

• A focused test covers a candidate-causal finding with omitted id and proves the chosen ownership rule deterministically.
• Native schema/decoder behavior and semantic admission agree about whether id is required.
• Stable IDs remain bound to the selected lens and unique within the result.
• Candidate-causal changed-line verification remains fail-closed.
• Out-of-manifest locations, proof references, and evidence remain rejected.
• No adapter synthesizes hashes, strips evidence, or rewrites semantic findings to bypass native admission.

Related Issues

• fix(review): reject unsupported candidate-causal blockers #1317 established native rejection of unsupported candidate-causal blockers and explains why frozen-scope validation must remain strict.
• fix(review): expose immutable candidate diff and changed-path manifest from START #1454 delivered immutable candidate diff/manifest context.
• fix(opencode): enrich reviewer output with provider-owned capture subject #1689 covers provider-owned capture-envelope enrichment in the OpenCode adapter, not optional semantic finding IDs.

[dnlrsls]

Correction after checking the native schema and replay contracts: the mismatch is valid, but requiring an explicit id on every review capture-result input is not the correct universal ownership rule. The public reviewer schema permits omitted IDs, and native canonicalization already assigns deterministic lens-bound IDs. Requiring IDs at capture would break compatible reviewer payloads and preserved-result replay.

Please select the second model in the issue: canonicalize the reviewer result once, before repository-derived causal classification, then preserve that exact canonical result and its IDs through admission, re-admission, replay, finalize, ledger, receipts, and gates. Raw reviewer bytes and their digest must remain bound and immutable, while adapters must not synthesize or rewrite semantic finding IDs.

Please cover omitted IDs on candidate-causal and non-causal findings, mixed explicit/implicit IDs, wrong lens prefixes, duplicates or generated-ID collisions, deterministic replay of preserved raw payloads, and zero slot or authority mutation on rejection. With that ownership rule fixed and the existing frozen-scope checks preserved, this should be ready to approve as a medium-priority bug.

[ftorga]

The review correctly identifies that this issue must choose one finding-ID ownership model. However, requiring every reviewer to provide an ID at review capture-result is not compatible with all shipped reviewer contracts.

Ledger-oriented agent assets require {LENS}-{NNN} IDs, but the native Judgment Day result contract and the currently installed OpenCode review agents allow only location, severity, claim, evidence_class, causal_disposition, and proof_refs. Those contracts reject additional finding fields. The preserved four-lens results from lineage review-28f51b71c1bf50cb therefore contain valid findings without IDs. Making IDs mandatory at capture would render those results unreplayable and break existing managed reviewers.

Proposed ownership rule

The native provider should own deterministic fallback IDs. This matches the existing canonicalLensResult behavior, which already assigns missing IDs. The defect is ordering: canonicalization currently happens after repository-derived causal verification.

The native pipeline should instead be:

strict reviewer decode
→ selected-lens canonicalization
→ assign or validate finding IDs
→ provider causal classification
→ canonical payload and result hash
→ artifact admission
→ replay/finalize/receipt/gates

Rules:

• A missing ID is assigned deterministically as {LENS}-{NNN} by the native provider.
• A valid explicit ID is preserved.
• A malformed, duplicate, unstable, or cross-lens ID is rejected before slot consumption or authority mutation.
• Adapters and plugins never synthesize IDs.
• The exact same canonical findings and IDs are used for causal classification, hashing, admission, replay, finalize, receipts, and gates.
• Legacy/internal callers retain compatibility through the same canonicalization boundary.

Updated acceptance criteria

• Native capture canonicalizes deterministic finding IDs before repository-derived causal verification.
• Omitted IDs are accepted only for contracts where the native provider owns fallback IDs.
• Explicit IDs must match the selected-lens prefix and be unique.
• Malformed, duplicate, and cross-lens IDs fail before publication or authority mutation.
• Candidate-causal verification uses the same canonical IDs persisted in the admitted artifact.
• Replay and response-loss recovery reproduce the same IDs, canonical payload, result hash, and admission digest.
• Out-of-manifest locations, proof references, and unsupported evidence remain rejected.
• No reviewer adapter authors hashes, manifests, causal classifications, or provider metadata.

Required tests

• Omitted ID on candidate-causal and non-causal findings.
• Valid explicit selected-lens IDs.
• Wrong-lens prefixes.
• Duplicate and malformed IDs.
• Multiple omitted findings with deterministic ordering.
• Replay, concurrent capture, and response-loss recovery.
• Zero slot, budget, or authority mutation on rejection.
• Stable IDs across capture, causal classification, finalize, receipts, and gates.

If maintainers prefer reviewer-owned IDs instead, every shipped reviewer contract, including Judgment Day and installed OpenCode agents, must be migrated atomically with explicit capability/version negotiation. That is a broader compatibility change and should not be implied by this narrow bug fix.

This issue should remain limited to finding-ID ownership and canonicalization order. #1757 consumes these stable IDs for provider-owned causal classification, while #1689 remains responsible only for reviewer-result capture enrichment.

[dnlrsls]

The proposed provider-owned canonicalization model resolves the ownership and ordering concern. Two scope clarifications remain before approval.

Please update the issue body to replace the current either/or alternatives with this single rule. Under the current reviewer/v1 schema, omitted IDs should be valid and receive deterministic provider-owned fallback IDs. The criterion “only for contracts where the provider owns fallback IDs” is not enforceable at the current boundary because capture carries no per-reviewer ownership capability. A future reviewer-owned-ID model should require an explicitly negotiated schema or capability version rather than implicit contract detection.

Please also narrow the compatibility rationale: current OpenCode overlays request stable IDs and the native schema allows the id field, so the repository evidence does not establish that OpenCode universally rejects IDs. The optional reviewer/v1 schema and preserved-result replay are sufficient reasons to retain omitted-ID support. With those points reflected in the issue body, this is ready for approval as a medium-priority bug.

[ftorga]

I cannot edit the issue body because the contributor token does not have repository permission, so here is the exact requested narrowing for an author or maintainer to apply.

Replacement: Expected Behavior

The reviewer/v1 native boundary has one finding-ID ownership rule:

1. Strictly decode the reviewer payload while preserving its exact raw bytes and digest.
2. Canonicalize the selected-lens result exactly once before repository-derived causal verification.
3. Assign deterministic provider-owned {LENS}-{NNN} fallback IDs to findings that omit id.
4. Preserve valid explicit IDs.
5. Reject malformed, duplicate, cross-lens, or generated-ID-colliding explicit IDs before slot consumption or authority mutation.
6. Use the exact same canonical findings and IDs for causal classification, canonical payload hashing, admission, re-admission, replay, finalize, ledger projection, receipts, and gates.

Omitted IDs remain valid under reviewer/v1. A future reviewer-owned-ID model must use an explicitly negotiated schema or capability version rather than implicit reviewer-contract detection.

Replacement: Acceptance Criteria

• Omitted IDs are valid under reviewer/v1 and receive deterministic provider-owned fallback IDs.
• Canonicalization occurs before repository-derived causal verification.
• Valid explicit selected-lens IDs are preserved.
• Malformed, duplicate, cross-lens, and generated-ID-colliding IDs fail before publication or authority mutation.
• Mixed explicit and implicit IDs produce deterministic, collision-free identities.
• Candidate-causal verification uses the same canonical IDs persisted in the admitted artifact.
• Raw reviewer bytes and their digest remain immutable and bound to the admitted result.
• Replay and response-loss recovery reproduce the same IDs, canonical payload, result hash, and admission digest.
• Rejection leaves the artifact slot, authority revision, correction budget, and reviewer execution count unchanged.
• Existing frozen-scope location, proof-reference, evidence, subject, manifest, and CAS checks remain fail-closed.
• Adapters do not synthesize or rewrite finding IDs.
• Any future reviewer-owned-ID rule requires an explicitly negotiated schema or capability version.

Required Tests

• Omitted IDs on candidate-causal and non-causal findings.
• Valid explicit selected-lens IDs.
• Wrong-lens prefixes, duplicate IDs, and malformed IDs.
• Mixed explicit and implicit IDs.
• Explicit IDs colliding with generated fallback IDs.
• Multiple omitted findings with deterministic ordering.
• Preserved reviewer/v1 payload replay with omitted IDs.
• Concurrent capture and response-loss recovery.
• Zero reviewer rerun, slot consumption, budget change, or authority mutation on rejection.
• Stable IDs across capture, causal classification, finalize, receipts, and gates.

[ftorga]

A corrected replacement issue has been created as #1761 because the current contributor cannot edit this issue body and the requested single ownership rule could not be applied here.

#1761 incorporates the latest review feedback: one provider-owned reviewer/v1 fallback-ID model, canonicalization before causal verification, explicit collision handling, immutable raw replay, and capability-versioning for any future reviewer-owned-ID contract.

If #1761 is accepted, please close this issue as superseded to avoid parallel implementations.

[ftorga]

#1699 remains the canonical valid issue for finding-ID ownership and canonicalization ordering. The replacement #1761 was closed to avoid parallel tracking. Follow-up implementation and review should continue here.