fix(index): bind shared consumer discriminators - #634
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR hardens semantic execution indexing with authenticated payload and switch-discriminant bindings, stricter fail-closed analysis, canonical graph validation, extensive regression tests, and updated corrective-candidate governance evidence. ChangesSemantic execution index correction
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TypeScriptAnalyzer
participant EffectResolver
participant QueryIndexValidator
participant RegressionTests
TypeScriptAnalyzer->>EffectResolver: collect facts and resolve payload-aware effects
EffectResolver->>QueryIndexValidator: emit channels, edges, and dispatch metadata
QueryIndexValidator->>RegressionTests: validate authenticated evidence and fail-closed cases
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
tests/unit/canonical-index-execution-review-regressions.test.ts (4)
3154-3215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare
selectorbefore the helpers that close over it.
mappingon Line 3154 readsselectoron Line 3162, andselectedPersistenceon Line 3186 callsmappingon Line 3204.selectoris declared withconston Line 3208. The current code works only because the first call site, Line 3222, runs after Line 3208.Any assertion added between Line 3216 and Line 3221 that calls
mappingorselectedPersistencethrowsReferenceError: Cannot access 'selector' before initialization. That failure names the temporal dead zone, not the assertion. Move theselectordeclaration abovemappingto remove the ordering dependency.♻️ Proposed reordering
+ const selector = { + kind: 'template', + parts: [ + { kind: 'parameter', position: 0 }, + { kind: 'literal', value: 'data' }, + { kind: 'literal', value: 'trigger' }, + ], + } const mapping = (built: ReturnType<typeof build>) => {- const selector = { - kind: 'template', - parts: [ - { kind: 'parameter', position: 0 }, - { kind: 'literal', value: 'data' }, - { kind: 'literal', value: 'trigger' }, - ], - } - const baseDispatch = dispatch(baseline)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/canonical-index-execution-review-regressions.test.ts` around lines 3154 - 3215, Move the const selector declaration above the mapping helper, before any helper that closes over it; keep mapping, dispatch, and selectedPersistence behavior unchanged while ensuring they can be called safely immediately after definition.
2647-2727: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert the payload assertions to a table and split the duplicate-witness scenario.
Lines 2647-2706 contain 60 sequential
expect(marked(name)).toEqual([...])calls. Vitest stops theitblock at the first failure, so one regression hides the remaining 59 results, and triage requires reading the 660-line fixture above. A table gives every case a name in the failure message and evaluates all rows.Lines 2707-2727 build a second fixture and assert duplicate case-arm behaviour plus
inspectQueryIndexreadiness. That scenario is independent of positional payload inference. Move it into its ownitblock so a failure names the scenario directly.♻️ Proposed table-driven assertions
- expect(marked('exact')).toEqual([1]) - expect(marked('observedExact')).toEqual([1]) - expect(marked('awaitedObservedExact')).toEqual([1]) - expect(marked('pendingObservedExact')).toEqual([]) + const expected: ReadonlyArray<readonly [string, number[]]> = [ + ['exact', [1]], + ['observedExact', [1]], + ['awaitedObservedExact', [1]], + ['pendingObservedExact', []], + // ...remaining cases + ] + for (const [name, positions] of expected) + expect(marked(name), name).toEqual(positions)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/canonical-index-execution-review-regressions.test.ts` around lines 2647 - 2727, Refactor the sequential marked(...) assertions in the existing test into a table-driven test that names each case and evaluates every payload expectation independently, preserving all current expected values. Move the duplicate-witness fixture, duplicateEdge assertions, and inspectQueryIndex readiness check into a separate it block with a scenario-specific name, leaving the payload regression test focused only on its table.
1743-1751: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the selector proof span.
statement_rangeintentionally spans the destructuring statement on fixture line 6 through the closing brace of theswitchon fixture line 12. Add a short comment beside the literal that records these fixture lines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/canonical-index-execution-review-regressions.test.ts` around lines 1743 - 1751, In the expectation for condition('exact'), add a short comment beside the statement_range literal documenting that the selector proof span covers fixture lines 6 through 12, from the destructuring statement to the switch’s closing brace.
2908-2916: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a positive Nest control and handle the decorator diagnostic.
symbol(nodes, 'Publisher.dispatch')andsymbol(nodes, 'outerNest')throw when either symbol is absent, so the test cannot pass with those symbols completely unanalyzed. However, the fixture has notsconfig.json, and default options omitexperimentalDecorators; TypeScript emits a parameter-decorator diagnostic. Add an unpatched Nest-shaped control, or assert method-body facts, to prove the monkey-patch path is exercised.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/canonical-index-execution-review-regressions.test.ts` around lines 2908 - 2916, Add a positive Nest-shaped control in the test fixture and explicitly handle the expected parameter-decorator diagnostic caused by missing experimentalDecorators configuration. Ensure the test validates analyzed method-body facts or guards symbol lookups for Publisher.dispatch and outerNest, proving the monkey-patch path is exercised without requiring unavailable symbols.tests/unit/canonical-index-execution-hardening.test.ts (1)
109-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the typed case-arm decoders between test files.
decodeTypedCaseArmandtypedCaseValuesare duplicated verbatim intests/unit/canonical-index-execution.test.ts(lines 139-169). The two copies already differ in parameter naming and in the frame cast on Line 136, which shows the drift starting. Move both helpers into a shared test helper module and import them in both files.The
rawFrame as Record<string, unknown>cast on Line 136 is not present in the sibling copy, so the frame type is already narrow enough to drop the cast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/canonical-index-execution-hardening.test.ts` around lines 109 - 143, Move decodeTypedCaseArm and typedCaseValues into a shared test-helper module, then import and use those shared helpers from both canonical-index-execution-hardening.test.ts and canonical-index-execution.test.ts. Remove both duplicated local implementations, preserving their existing behavior and signatures. In typedCaseValues, use the existing rawFrame type directly and remove the unnecessary `as Record<string, unknown>` cast.tests/unit/query-index-execution-validation.test.ts (2)
114-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the non-null assertion with an explicit guard.
If every operation of
runis a call fact,findreturnsundefinedand line 115 throws aTypeErrorwhen it reads.evidence. The other fixture lookups raise descriptive errors instead. Use the same pattern here.♻️ Proposed change
- const proof = marker?.matchCall === false - ? operations.find((operation) => operation.kind !== 'call')!.evidence - : marker ? call.evidence : operations[0]!.evidence + const other = operations.find((operation) => operation.kind !== 'call') + if (marker?.matchCall === false && !other) { + throw new Error('Execution validation fixture has no non-call operation') + } + const proof = marker?.matchCall === false + ? other!.evidence + : marker ? call.evidence : operations[0]!.evidence🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/query-index-execution-validation.test.ts` around lines 114 - 116, Update the proof lookup in the test around the marker?.matchCall branch to explicitly validate the result of finding a non-call operation before accessing evidence; when none exists, throw the same descriptive error pattern used by the other fixture lookups, while preserving the existing call and first-operation branches.
349-353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the accepted marker survives in the sealed graph.
The test proves that a valid marker does not corrupt the index. It does not prove that
dispatch_payload_argumentis preserved on the publish edge. A consumer of the query index reads that metadata, so assert its value directly.🧪 Proposed additional assertion
it('accepts a dispatch marker authenticated by one exact call fact', () => { const current = fixture(false, { edge: 'publish', value: 0 }) - expect(ready(inspectQueryIndex(current.graph))).toBeDefined() + const index = ready(inspectQueryIndex(current.graph)) expect(current.callArgumentCount).toBeGreaterThan(0) + const edge = index.graph.edgesBetween(current.runId, current.jobId) + .find((candidate) => candidate.attributes.relation === 'publishes_to') + expect(edge?.attributes.dispatch_payload_argument).toBe(0) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/query-index-execution-validation.test.ts` around lines 349 - 353, Extend the test around fixture and inspectQueryIndex in “accepts a dispatch marker authenticated by one exact call fact” to inspect the sealed graph’s publish edge and assert that its dispatch_payload_argument metadata equals current.callArgumentCount. Keep the existing readiness and positive-count assertions unchanged.src/domain/query/index-status.ts (1)
93-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReorder the owner narrowing before the payload lookup.
Line 102 uses
String(i)whileiis stillunknown. The finaltypeof i === 'string'check at line 107 makes the result fail closed, so there is no exploitable gap. Reading the code is easier if the string narrowing happens first and the lookup usesidirectly.♻️ Optional change
- const s = a[SF], i = a[EO], w = typeof i === 'string' ? n.get(i) : undefined + const s = a[SF], i = typeof a[EO] === 'string' ? a[EO] : null + const w = i === null ? undefined : n.get(i) @@ - && si(d) && (o.get(String(i)) ?? []).filter((x) => + && si(d) && i !== null && (o.get(i) ?? []).filter((x) => @@ - return typeof s === 'string' && f.has(s) && typeof i === 'string' + return typeof s === 'string' && f.has(s) && i !== null🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/query/index-status.ts` around lines 93 - 112, In function ep, narrow i to a string before computing the dispatch-payload lookup q, then use the narrowed i directly for o.get rather than String(i). Preserve the existing final validation and payload-matching behavior while making the owner lookup occur only after the type guard.src/adapters/typescript/execution.ts (1)
2013-2033: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the publish-edge dedup key is stable, then simplify the branch.
The dedup key omits
metadata.execution_owner_id, so two publish edges from different owners with identicalfrom,to,kind,source, andevidencecollapse into one. Theexecution_owner_idequals the publish source node forpublishes_toedges, sofromalready carries it. Verify that assumption holds for framework-decorator publish edges.The conflict branch is also easier to read if the payload key is removed once and reused.
♻️ Optional readability change
- if (!d) p.set(k, b) - else if (d.metadata?.dispatch_payload_argument !== b.metadata?.dispatch_payload_argument) { - const m = { ...(d.metadata ?? {}) }; delete m.dispatch_payload_argument - p.set(k, { ...d, metadata: m }) - } else if (ct(js(b), js(d)) < 0) p.set(k, b) + const pa = (x: IndexEdge): unknown => x.metadata?.dispatch_payload_argument + if (!d) p.set(k, b) + else if (pa(d) !== pa(b)) { + const m = { ...(d.metadata ?? {}) }; delete m.dispatch_payload_argument + p.set(k, { ...d, metadata: m }) + } else if (ct(js(b), js(d)) < 0) p.set(k, b)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/adapters/typescript/execution.ts` around lines 2013 - 2033, In sort, verify that framework-decorator publishes_to edges always set execution_owner_id to the publish source node, so the existing publish dedup key’s from component uniquely identifies the owner; update the key if that invariant does not hold. Simplify the PU conflict branch by removing metadata.dispatch_payload_argument once and reusing the resulting metadata when comparing or storing the edge.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/canonical-index-execution-review-regressions.test.ts`:
- Around line 535-539: Rename the event listener registration near the existing
events.on('conditional', ...) call to use the unique key 'conditional-event',
keeping the Worker key 'conditional' unchanged so consumes and hasConsumer
assertions target distinct channels. Update the Line 561 assertion loop to pass
its key as the assertion message, matching the existing loops around lines 546
and 578.
---
Nitpick comments:
In `@src/adapters/typescript/execution.ts`:
- Around line 2013-2033: In sort, verify that framework-decorator publishes_to
edges always set execution_owner_id to the publish source node, so the existing
publish dedup key’s from component uniquely identifies the owner; update the key
if that invariant does not hold. Simplify the PU conflict branch by removing
metadata.dispatch_payload_argument once and reusing the resulting metadata when
comparing or storing the edge.
In `@src/domain/query/index-status.ts`:
- Around line 93-112: In function ep, narrow i to a string before computing the
dispatch-payload lookup q, then use the narrowed i directly for o.get rather
than String(i). Preserve the existing final validation and payload-matching
behavior while making the owner lookup occur only after the type guard.
In `@tests/unit/canonical-index-execution-hardening.test.ts`:
- Around line 109-143: Move decodeTypedCaseArm and typedCaseValues into a shared
test-helper module, then import and use those shared helpers from both
canonical-index-execution-hardening.test.ts and
canonical-index-execution.test.ts. Remove both duplicated local implementations,
preserving their existing behavior and signatures. In typedCaseValues, use the
existing rawFrame type directly and remove the unnecessary `as Record<string,
unknown>` cast.
In `@tests/unit/canonical-index-execution-review-regressions.test.ts`:
- Around line 3154-3215: Move the const selector declaration above the mapping
helper, before any helper that closes over it; keep mapping, dispatch, and
selectedPersistence behavior unchanged while ensuring they can be called safely
immediately after definition.
- Around line 2647-2727: Refactor the sequential marked(...) assertions in the
existing test into a table-driven test that names each case and evaluates every
payload expectation independently, preserving all current expected values. Move
the duplicate-witness fixture, duplicateEdge assertions, and inspectQueryIndex
readiness check into a separate it block with a scenario-specific name, leaving
the payload regression test focused only on its table.
- Around line 1743-1751: In the expectation for condition('exact'), add a short
comment beside the statement_range literal documenting that the selector proof
span covers fixture lines 6 through 12, from the destructuring statement to the
switch’s closing brace.
- Around line 2908-2916: Add a positive Nest-shaped control in the test fixture
and explicitly handle the expected parameter-decorator diagnostic caused by
missing experimentalDecorators configuration. Ensure the test validates analyzed
method-body facts or guards symbol lookups for Publisher.dispatch and outerNest,
proving the monkey-patch path is exercised without requiring unavailable
symbols.
In `@tests/unit/query-index-execution-validation.test.ts`:
- Around line 114-116: Update the proof lookup in the test around the
marker?.matchCall branch to explicitly validate the result of finding a non-call
operation before accessing evidence; when none exists, throw the same
descriptive error pattern used by the other fixture lookups, while preserving
the existing call and first-operation branches.
- Around line 349-353: Extend the test around fixture and inspectQueryIndex in
“accepts a dispatch marker authenticated by one exact call fact” to inspect the
sealed graph’s publish edge and assert that its dispatch_payload_argument
metadata equals current.callArgumentCount. Keep the existing readiness and
positive-count assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b50491c8-19d4-4a68-932f-db50056d00a8
📒 Files selected for processing (14)
docs/core-reset/removal-manifest.ymldocs/core-reset/scorecard.mddocs/designs/2026-07-19-core-reset.mddocs/roadmap.mdsrc/adapters/typescript/execution.tssrc/domain/index/build-state.tssrc/domain/index/model.tssrc/domain/query/index-status.tstests/unit/canonical-index-execution-hardening.test.tstests/unit/canonical-index-execution-review-regressions.test.tstests/unit/canonical-index-execution.test.tstests/unit/core-reset-governance.test.tstests/unit/query-index-execution-validation.test.tstests/unit/update-index.test.ts
1 participant
Summary
Root cause
The merged index could separately prove a producer literal such as trigger: assembly_complete, the shared db-sync queue, a consumer switch, and persistence facts, but it did not bind the producer key and value to the local switch selector and exact case arm. That allowed durable writes owned by a sibling arm to appear eligible.
This correction stores a compact authenticated discriminator mapping tied to source excerpts and mutation state. Branch-owned facts are usable only when selector origin, discriminator property, literal value, and case ownership all resolve exactly.
Exact candidate
Frozen receipts
Verification
Merge gate
Merge remains blocked until this unchanged head has all six hosted CI jobs green, independent review remains blocker-free, and zero review threads remain. CodeRabbit is evaluated honestly: a skip, pending state, rate limit, or non-default-base limitation is not represented as an independent completed review.
No npm publication, GitHub Release, Registry metadata publication, tag, or main action is authorized.
Closes #632
Summary by CodeRabbit
New Features
Bug Fixes
Documentation