[PR #4442] [CLOSED] fix(evidence): accept legacy null coverage identity #6247

Closed
opened 2026-10-31 01:13:00 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4442
Author: @LuckVd
Created: 10/16/2026
Status: ❌ Closed

Base: main ← Head: fix/4436-legacy-null-relationship-coverage


📝 Commits (5)

  • af586f3 fix(evidence): accept legacy null coverage identity
  • f1dbaed docs(evidence): record the 6.6 evidence identity migration
  • 160f213 Merge remote-tracking branch 'origin/main' into fix/4436-legacy-null-relationship-coverage
  • 8be2287 fix(evidence): accept 6.5.0 legacy IDs in artifact comparison reprojection
  • db10e0b Merge remote-tracking branch 'origin/main' into fix/4436-legacy-null-relationship-coverage

📊 Changes

6 files changed (+157 additions, -10 deletions)

View changed files

📝 docs/index.md (+2 -1)
➕ docs/migration-6.6.md (+34 -0)
📝 src/application/artifacts/ArtifactComparisonEvidence.test.ts (+37 -0)
📝 src/application/artifacts/ArtifactComparisonEvidence.ts (+7 -2)
📝 src/domain/evidence.test.ts (+43 -0)
📝 src/domain/evidence.ts (+34 -7)

📄 Description

Description

Fixes #4436.

Records exported by rea-agents@6.5.0 with relationship_coverage: null changed
evidence_id after db8128dc0 flipped semanticProjection from omitting a null
coverage to hashing relationship_coverage: null
(src/domain/evidence.ts), so unmodified 6.5.0 bundles are rejected on main with
evidence_integrity_mismatch — exactly the records that do state the field
explicitly, while records missing the field keep the announced rejection.
This change keeps the current identity rule for everything producers emit, and
adds a verification-side fallback: when the current-rule ID does not match but
the record's relationship_coverage is explicitly null, the ID is re-checked
against the 6.5.0 projection (null coverage omitted). Records missing the field
are still rejected by the strict base schema, matching the announced breaking
change.
The same fallback now also covers the downstream artifact-comparison
re-projection, which rejected retained 6.5.0 summaries on the documented
compare_artifacts path even when the import-side check had accepted them.

Change

  • semanticProjection/computeEvidenceId take an optional
    { omitNullRelationshipCoverage } flag; when set and coverage is null, the
    field is left out of the hashed projection, reproducing the pre-db8128dc0
    (6.5.0) digest input. Default behavior (no flag) is byte-identical to before,
    so createEvidence/createImmutableEvidenceSteps keep producing current IDs.
  • The dual-projection identity check is extracted into an exported
    matchesEvidenceIdentity helper, shared by the parseEvidence gate
    (authenticatedEvidenceSchema, used by bundle import, compare, etc.) and the
    artifact-comparison path; the fallback only engages for explicit null and
    preserves the record's original evidence_id.
  • resolveArtifactComparisonEvidence re-projects the retained parent's summary
    through the current projection, so its exact-ID equality rejected retained
    6.5.0 summaries with EvidenceIntegrityError on the documented
    compare_artifacts path; it now uses the same dual-projection check.
  • Tamper surface unchanged: null→non-null edits fail both ID checks, and
    deleting the field fails the schema before the identity check.
  • Branch synced with main (merge; one import-conflict resolution in
    src/domain/evidence.test.ts, keeping both sides' tests).

Type of Change

  • 🐛 Bug fix

Testing

  • New test: accepts 6.5.0 Evidence identity when relationship coverage is explicitly null — builds a record whose ID is the 6.5.0-style digest
    (computed independently via digestCanonicalValue over the legacy
    projection), asserts parseEvidence accepts and preserves it, and asserts a
    record with the relationship_coverage key removed still throws.
  • New test: accepts a retained 6.5.0 summary whose legacy ID omits null coverage (ArtifactComparisonEvidence.test.ts) — reproduces the reported
    rejection on the public compare_artifacts flow (fails before the fix,
    passes after) and asserts the record is resolved to its retained parent with
    its original ID.
  • npx vitest run src/domain/evidence.test.ts — 28/28 passed; the original
    test fails on the merge-base (781d5751a) implementation, confirming it
    reproduces the reported issue.
  • npx vitest run src/application/artifacts src/server/runDerivedOperation.test.ts src/domain/evidence.test.ts src/domain/evidenceBundle.test.ts —
    49/49 passed across 4 files.
  • npm run check:fast (typecheck + lint via turbo) — 2/2 tasks pass.
  • Pre-commit oxfmt/oxlint hooks pass on all changed files.
  • Full-repository vitest suite intentionally not run locally
    (verification-scope SLA); left to CI. Targeted affected-scope checks above all
    pass. Known baseline failures on this machine (documented in internal notes,
    unrelated to this change): ciStaticArtifacts, closedStdout, runnerCancellation,
    packageImportAliases.

Checklist

  • git diff --check origin/main...HEAD passes
  • Changed source files are formatted
  • Targeted tests for the changed packages/components pass
  • Diff-scoped lint passes where applicable
  • Full-repository checks left to CI; scope and reason documented above
  • Self-reviewed the code
  • Added/updated tests covering the change
  • Breaking changes are clearly called out in the description above — none; producers keep emitting current IDs and the announced rejection of field-missing records is preserved

Follow-ups from review

  • Added docs/migration-6.6.md (linked from docs/index.md) per review
    feedback: it records the announced 6.6 relationship_coverage identity
    commitment, the verification-side acceptance of 6.5.0 explicit-null Evidence
    with its original ID, and the cross-generation rea compare behavior — a
    6.5.0 export and a 6.6 export of the same analysis keep different
    evidence_id values, so cross-bundle comparisons report unknown with the
    affected records unresolved rather than an unchanged pair. The release note's
    breaking-change entry can cite the guide.

  • Re-verified after the doc commit: npx vitest run src/domain/evidence.test.ts
    27/27 (EXIT=0); npx oxfmt --check on both changed files clean; pre-commit
    oxfmt hook passed.

  • Codex review (P2, on f1dbaedf4): handle legacy IDs during artifact-summary
    re-projection — addressed in this push via matchesEvidenceIdentity reuse in
    resolveArtifactComparisonEvidence, with a public-flow regression test.

  • Independent verification after the upstream merge (db10e0bd):
    vitest run src/domain/evidence.test.ts src/application/artifacts/ArtifactComparisonEvidence.test.ts — 35/35; npm run typecheck, changed-file oxlint/oxfmt --check, and git diff --check origin/main...HEAD all pass. The push hook's npm run check:fast also passed. The full repository suite was not rerun locally; CI is responsible for it.

Author readiness

Human owner: @LuckVd

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/4442 **Author:** [@LuckVd](https://github.com/LuckVd) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/4436-legacy-null-relationship-coverage` --- ### 📝 Commits (5) - [`af586f3`](https://github.com/morluto/rea/commit/af586f3c97ec2f16d446bc489a485289075105cc) fix(evidence): accept legacy null coverage identity - [`f1dbaed`](https://github.com/morluto/rea/commit/f1dbaedf412a2976c5e9a60aefbaab52d9738d3a) docs(evidence): record the 6.6 evidence identity migration - [`160f213`](https://github.com/morluto/rea/commit/160f2134a667d5063ac5fc62ed7247f995df0834) Merge remote-tracking branch 'origin/main' into fix/4436-legacy-null-relationship-coverage - [`8be2287`](https://github.com/morluto/rea/commit/8be228796b2952aff432c29ef53df6601abc94c1) fix(evidence): accept 6.5.0 legacy IDs in artifact comparison reprojection - [`db10e0b`](https://github.com/morluto/rea/commit/db10e0bd39a4e9f637f521f024b311e395aee045) Merge remote-tracking branch 'origin/main' into fix/4436-legacy-null-relationship-coverage ### 📊 Changes **6 files changed** (+157 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `docs/index.md` (+2 -1) ➕ `docs/migration-6.6.md` (+34 -0) 📝 `src/application/artifacts/ArtifactComparisonEvidence.test.ts` (+37 -0) 📝 `src/application/artifacts/ArtifactComparisonEvidence.ts` (+7 -2) 📝 `src/domain/evidence.test.ts` (+43 -0) 📝 `src/domain/evidence.ts` (+34 -7) </details> ### 📄 Description ## Description Fixes #4436. Records exported by rea-agents@6.5.0 with `relationship_coverage: null` changed `evidence_id` after db8128dc0 flipped `semanticProjection` from *omitting* a null coverage to hashing `relationship_coverage: null` (`src/domain/evidence.ts`), so unmodified 6.5.0 bundles are rejected on main with `evidence_integrity_mismatch` — exactly the records that *do* state the field explicitly, while records missing the field keep the announced rejection. This change keeps the current identity rule for everything producers emit, and adds a verification-side fallback: when the current-rule ID does not match but the record's `relationship_coverage` is explicitly `null`, the ID is re-checked against the 6.5.0 projection (null coverage omitted). Records missing the field are still rejected by the strict base schema, matching the announced breaking change. The same fallback now also covers the downstream artifact-comparison re-projection, which rejected retained 6.5.0 summaries on the documented `compare_artifacts` path even when the import-side check had accepted them. ### Change - `semanticProjection`/`computeEvidenceId` take an optional `{ omitNullRelationshipCoverage }` flag; when set and coverage is `null`, the field is left out of the hashed projection, reproducing the pre-db8128dc0 (6.5.0) digest input. Default behavior (no flag) is byte-identical to before, so `createEvidence`/`createImmutableEvidenceSteps` keep producing current IDs. - The dual-projection identity check is extracted into an exported `matchesEvidenceIdentity` helper, shared by the `parseEvidence` gate (`authenticatedEvidenceSchema`, used by bundle import, compare, etc.) and the artifact-comparison path; the fallback only engages for explicit `null` and preserves the record's original `evidence_id`. - `resolveArtifactComparisonEvidence` re-projects the retained parent's summary through the current projection, so its exact-ID equality rejected retained 6.5.0 summaries with `EvidenceIntegrityError` on the documented `compare_artifacts` path; it now uses the same dual-projection check. - Tamper surface unchanged: null→non-null edits fail both ID checks, and deleting the field fails the schema before the identity check. - Branch synced with `main` (merge; one import-conflict resolution in `src/domain/evidence.test.ts`, keeping both sides' tests). ### Type of Change - [x] 🐛 Bug fix ### Related Issue - Fixes #4436 ### Testing - New test: `accepts 6.5.0 Evidence identity when relationship coverage is explicitly null` — builds a record whose ID is the 6.5.0-style digest (computed independently via `digestCanonicalValue` over the legacy projection), asserts `parseEvidence` accepts and preserves it, and asserts a record with the `relationship_coverage` key removed still throws. - New test: `accepts a retained 6.5.0 summary whose legacy ID omits null coverage` (`ArtifactComparisonEvidence.test.ts`) — reproduces the reported rejection on the public `compare_artifacts` flow (fails before the fix, passes after) and asserts the record is resolved to its retained parent with its original ID. - `npx vitest run src/domain/evidence.test.ts` — 28/28 passed; the original test fails on the merge-base (`781d5751a`) implementation, confirming it reproduces the reported issue. - `npx vitest run src/application/artifacts src/server/runDerivedOperation.test.ts src/domain/evidence.test.ts src/domain/evidenceBundle.test.ts` — 49/49 passed across 4 files. - `npm run check:fast` (typecheck + lint via turbo) — 2/2 tasks pass. - Pre-commit oxfmt/oxlint hooks pass on all changed files. - Full-repository vitest suite intentionally not run locally (verification-scope SLA); left to CI. Targeted affected-scope checks above all pass. Known baseline failures on this machine (documented in internal notes, unrelated to this change): ciStaticArtifacts, closedStdout, runnerCancellation, packageImportAliases. ### Checklist - [x] `git diff --check origin/main...HEAD` passes - [x] Changed source files are formatted - [x] Targeted tests for the changed packages/components pass - [x] Diff-scoped lint passes where applicable - [x] Full-repository checks left to CI; scope and reason documented above - [x] Self-reviewed the code - [x] Added/updated tests covering the change - [ ] Breaking changes are clearly called out in the description above — none; producers keep emitting current IDs and the announced rejection of field-missing records is preserved ### Follow-ups from review - Added `docs/migration-6.6.md` (linked from `docs/index.md`) per review feedback: it records the announced 6.6 `relationship_coverage` identity commitment, the verification-side acceptance of 6.5.0 explicit-null Evidence with its original ID, and the cross-generation `rea compare` behavior — a 6.5.0 export and a 6.6 export of the same analysis keep different `evidence_id` values, so cross-bundle comparisons report `unknown` with the affected records unresolved rather than an unchanged pair. The release note's breaking-change entry can cite the guide. - Re-verified after the doc commit: `npx vitest run src/domain/evidence.test.ts` 27/27 (EXIT=0); `npx oxfmt --check` on both changed files clean; pre-commit oxfmt hook passed. - Codex review (P2, on `f1dbaedf4`): handle legacy IDs during artifact-summary re-projection — addressed in this push via `matchesEvidenceIdentity` reuse in `resolveArtifactComparisonEvidence`, with a public-flow regression test. - Independent verification after the upstream merge (`db10e0bd`): `vitest run src/domain/evidence.test.ts src/application/artifacts/ArtifactComparisonEvidence.test.ts` — 35/35; `npm run typecheck`, changed-file `oxlint`/`oxfmt --check`, and `git diff --check origin/main...HEAD` all pass. The push hook's `npm run check:fast` also passed. The full repository suite was not rerun locally; CI is responsible for it. ## Author readiness Human owner: @LuckVd - [ ] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [ ] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [ ] I declare this PR ready for initial maintainer review. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:13:00 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#6247
No description provided.