feat(index): add authenticated semantic execution facts - #633
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds authenticated execution facts, queue/job/event channels, persistence evidence, immutable query indexes, and artifact-identity caching. Retrieval validates source and graph evidence. Release governance records beta.4 publication and phase ordering. ChangesSemantic execution indexing
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TypeScriptIndexer
participant ExecutionCollector
participant CanonicalGraph
participant QueryIndex
participant RetrieveContext
TypeScriptIndexer->>ExecutionCollector: collect execution facts and channels
ExecutionCollector->>CanonicalGraph: write encoded facts and graph edges
CanonicalGraph->>QueryIndex: load and validate canonical index
QueryIndex->>RetrieveContext: provide immutable operation and channel maps
RetrieveContext->>RetrieveContext: authenticate source excerpts and relationships
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/core-reset/scorecard.md (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTwo summaries overstate what is missing for the
#632candidate. Both sites group "candidate" together with the genuinely pending CI/review/merge/publication receipts, butdocs/core-reset/removal-manifest.yml'ssemantic-execution-index-632item already records concrete interim candidate data (149 focused tests passed, graph/channel counts, indexing-median ratio, warm-retrieval p95) and its own notes say those receipts "are recorded above," listing only CI, independent review, zero-thread, merge, and publication as absent.
docs/core-reset/scorecard.md#L46-53: change the Evidence cell from "no candidate, CI, review or merge receipt yet" to match line 206's more precise "No candidate final receipt, CI result, review result, merge commit... is claimed here."docs/roadmap.md#L166-170: reword "Candidate, CI, review, merge, and publication receipts remain pending" so it does not imply that no candidate measurement exists yet, consistent with the recordedcandidate.local_verificationdata in the manifest.🤖 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 `@docs/core-reset/scorecard.md` at line 1, Update the `#632` evidence summary in scorecard.md to distinguish the existing interim candidate data from the missing candidate final, CI, review, merge, and publication receipts, matching the precise wording used near the manifest reference. Reword the corresponding roadmap entry so it states that the candidate final receipt and remaining CI/review/merge/publication receipts are pending without implying that candidate measurements are absent; preserve the documented manifest data and scope the changes to these two summaries.
🧹 Nitpick comments (5)
src/adapters/typescript/execution.ts (2)
1968-1984: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrecompute the sort keys in
sortEdges.The comparator calls
JSON.stringify(left)andJSON.stringify(right)on every comparison, so serialization runs O(n log n) times over all execution edges. Serialize each edge once, then sort on the precomputed key.♻️ Proposed change
- return [...retained, ...structuralRoutes.values()].sort((left, right) => compareText(JSON.stringify(left), JSON.stringify(right))); + return [...retained, ...structuralRoutes.values()] + .map((edge) => [JSON.stringify(edge), edge] as const) + .sort(([left], [right]) => compareText(left, right)) + .map(([, edge]) => edge);🤖 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 1968 - 1984, Update sortEdges to precompute each edge’s JSON serialization once and retain it alongside the edge, including when selecting structuralRoutes representatives; use those cached strings for both compareText calls and return the edges in the existing sorted order.
151-167: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the scanner in
structuralText. Use a module-local scanner and callsetLanguageVariant(sf.languageVariant)andsetText(node.getText(sf))before each scan. KeepboundedTextunchanged; its loop is capped atMAX_TEXT_BYTES(256 by default and 96 for switch arms), so it is not quadratic in input length. Do not remove every trailing\uFFFD, because valid replacement characters can occur at the truncation boundary.🤖 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 151 - 167, Update structuralText to reuse a module-local TypeScript scanner instead of creating one per call; before scanning, call setLanguageVariant(sf.languageVariant) and setText(node.getText(sf)). Keep boundedText unchanged, including its existing truncation behavior and valid trailing replacement characters.src/domain/index/model.ts (1)
386-403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the shared scalar validators instead of re-implementing them in the query layer.
safeInt,validText,orderCmp,SHA256andMAX_TEXTare duplicated insrc/domain/query/index-status.ts(lines 45-46, 72-85, 230-236). The copies have already diverged: thesafeIntinindex-status.tsomits theObject.is(value, -0)rejection that this version applies. Both files validate the same wire values, so the two definitions must stay identical.Export these helpers from
model.tsand import them inindex-status.ts.♻️ Proposed export surface
-function safeInt(value: unknown, minimum = 0): value is number { +export function safeInt(value: unknown, minimum = 0): value is number { return typeof value === 'number' && Number.isSafeInteger(value) && !Object.is(value, -0) && value >= minimum } -function validText(value: unknown, maxBytes = MAX_TEXT): value is string { +export function validText(value: unknown, maxBytes = MAX_TEXT): value is string {🤖 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/index/model.ts` around lines 386 - 403, Export the shared validators and constants safeInt, validText, orderCmp, SHA256, and MAX_TEXT from model.ts, then remove their duplicate definitions in index-status.ts and import the exported symbols there. Ensure query validation uses the model implementations, including safeInt’s -0 rejection, while preserving existing behavior.tests/unit/query-index-execution-validation.test.ts (1)
515-523: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the integrity subject so each case reaches its intended validator.
The assertion checks only
state: 'corrupt'. Three cases therefore pass through an earlier check and leave the validator they name uncovered:
- 'a missing job parent' (lines 487-494) changes
parent_channel_id, sochannelFromrecomputes a differentindexChannelIdand validation fails atcanonical channel node. Thecanonical job parent channelbranch is never reached.- 'an unscoped event' (lines 507-514) deletes
scope, which triggers the same id mismatch instead ofcanonical event channel scope.- 'reversed publish endpoints' (lines 496-500) adds an edge with no
evidence, soedgeProoffails atcanonical channel evidenceinstead of thepublishes_toendpoint check.Add an expected
subjectper case, and give the reversed-endpoint case the authenticatedchannelEvidencepayload so it reaches the endpoint check.💚 Proposed change
{ name: 'reversed publish endpoints', + subject: 'canonical publishes_to endpoints', mutate: ({ graph, runId, jobId }: Fixture) => { - graph.addEdge(jobId, runId, { relation: 'publishes_to' }) + graph.addEdge(jobId, runId, { + relation: 'publishes_to', + ...channelEvidenceFor(graph, runId), + }) }, }, - ])('rejects $name as corrupt after re-signing', ({ mutate }) => { + ])('rejects $name as corrupt after re-signing', ({ mutate, subject }) => { const value = fixture() mutate(value) resign(value.graph) expect(inspectQueryIndex(value.graph)).toMatchObject({ state: 'corrupt', + subject, }) })Export the evidence payload from
fixture()so a case can reuse it, and set the expected subject for the job-parent and event-scope cases to the value they intend to exercise.🤖 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 515 - 523, Update the parameterized corruption test around fixture() and inspectQueryIndex() to assert each case’s expected subject, including the canonical job parent channel and canonical event channel scope cases. Export or expose fixture()’s authenticated channelEvidence so the reversed publish endpoints mutation reuses it when adding the edge, allowing validation to reach the publishes_to endpoint check.src/domain/query/index-status.ts (1)
68-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the offending node or edge identifier in the integrity subject.
inspectQueryIndexreturnserror.messageas the corrupt subject. Everyfail()call passes a fixed category string, so an operator seescanonical operation control referencewithout the owner, fact, channel, or edge identity. Diagnosing a corrupt artifact then requires re-deriving the failure by hand.Accept an optional detail argument and append it to the subject.
♻️ Proposed change
-function fail(subject: string): never { - throw new QueryIndexIntegrityError(subject) +function fail(subject: string, detail?: string): never { + throw new QueryIndexIntegrityError(detail ? `${subject}: ${detail}` : subject) }Then pass the identity at each call site, for example
fail('canonical operation control reference', fact.id).🤖 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 68 - 70, Update fail and every call site in inspectQueryIndex to accept and pass the offending node or edge identifier as an optional detail, appending it to the integrity subject while preserving the existing category text. Use the relevant identities such as fact.id, owner, channel, or edge identifier so QueryIndexIntegrityError and inspectQueryIndex expose the specific corrupt artifact.
🤖 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 `@docs/core-reset/scorecard.md`:
- Around line 46-53: Update the Semantic execution index `#632` Evidence cell in
the scorecard table to say “no candidate final receipt” instead of “no candidate
receipt,” while preserving the existing CI, review, and merge wording and all
other content.
In `@docs/roadmap.md`:
- Around line 166-170: Revise the receipt statement in the “semantic execution
index `#632`” roadmap entry so it acknowledges that candidate measurements are
already recorded, while keeping exact-head CI, independent review, merge, and
publication receipts pending and prohibited from being inferred from local work.
In `@src/adapters/typescript/execution.ts`:
- Around line 1930-1967: Update attachFacts so every encodeIndexBodyFactTable or
decodeIndexBodyFactTable failure is converted into the same per-owner error
diagnostic and omission behavior as IndexBodyFactBoundsError. Remove the rethrow
for non-bounds errors, preserve the diagnostic context for symbol.name and
symbol.range, and ensure one invalid owner does not abort
buildCanonicalTypeScriptIndex.
- Around line 449-470: Update callableOwner so binary-assigned closures are only
indexed when declSymbol(node, file, ctx) resolves to a symbol whose range
exactly matches the function node’s range; otherwise return null instead of
treating the enclosing callable as the owner. Add regression coverage for
property-assigned closures such as this.handler = (job) => ..., verifying their
effects and parameter mappings are not attributed to the enclosing method.
---
Outside diff comments:
In `@docs/core-reset/scorecard.md`:
- Line 1: Update the `#632` evidence summary in scorecard.md to distinguish the
existing interim candidate data from the missing candidate final, CI, review,
merge, and publication receipts, matching the precise wording used near the
manifest reference. Reword the corresponding roadmap entry so it states that the
candidate final receipt and remaining CI/review/merge/publication receipts are
pending without implying that candidate measurements are absent; preserve the
documented manifest data and scope the changes to these two summaries.
---
Nitpick comments:
In `@src/adapters/typescript/execution.ts`:
- Around line 1968-1984: Update sortEdges to precompute each edge’s JSON
serialization once and retain it alongside the edge, including when selecting
structuralRoutes representatives; use those cached strings for both compareText
calls and return the edges in the existing sorted order.
- Around line 151-167: Update structuralText to reuse a module-local TypeScript
scanner instead of creating one per call; before scanning, call
setLanguageVariant(sf.languageVariant) and setText(node.getText(sf)). Keep
boundedText unchanged, including its existing truncation behavior and valid
trailing replacement characters.
In `@src/domain/index/model.ts`:
- Around line 386-403: Export the shared validators and constants safeInt,
validText, orderCmp, SHA256, and MAX_TEXT from model.ts, then remove their
duplicate definitions in index-status.ts and import the exported symbols there.
Ensure query validation uses the model implementations, including safeInt’s -0
rejection, while preserving existing behavior.
In `@src/domain/query/index-status.ts`:
- Around line 68-70: Update fail and every call site in inspectQueryIndex to
accept and pass the offending node or edge identifier as an optional detail,
appending it to the integrity subject while preserving the existing category
text. Use the relevant identities such as fact.id, owner, channel, or edge
identifier so QueryIndexIntegrityError and inspectQueryIndex expose the specific
corrupt artifact.
In `@tests/unit/query-index-execution-validation.test.ts`:
- Around line 515-523: Update the parameterized corruption test around fixture()
and inspectQueryIndex() to assert each case’s expected subject, including the
canonical job parent channel and canonical event channel scope cases. Export or
expose fixture()’s authenticated channelEvidence so the reversed publish
endpoints mutation reuses it when adding the edge, allowing validation to reach
the publishes_to endpoint check.
🪄 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: 9762ec6b-fa32-4ae3-9bd9-5201f2094241
📒 Files selected for processing (21)
docs/core-reset/removal-manifest.ymldocs/core-reset/scorecard.mddocs/roadmap.mdpackage.jsonsrc/adapters/filesystem/graph-artifact.tssrc/adapters/mcp/server.tssrc/adapters/typescript/execution.tssrc/adapters/typescript/index.tssrc/application/retrieve-context.tssrc/domain/index/build-state.tssrc/domain/index/model.tssrc/domain/query/index-status.tssrc/domain/query/rank.tstests/unit/canonical-index-execution-hardening.test.tstests/unit/canonical-index-execution.test.tstests/unit/core-reset-governance.test.tstests/unit/graph-artifact.test.tstests/unit/query-index-execution-validation.test.tstests/unit/retrieve-context.test.tstests/unit/retrieve-evidence-skeleton-adversarial.test.tstests/unit/retrieve-evidence-skeleton-regression.test.ts
💤 Files with no reviewable changes (1)
- package.json
|
Corrective head |
|
Exact-head stop receipt
Fresh independent review reproduced nine false-proof classes on the exact head:
The prior binary-assigned deferred-closure and malformed-owner codec corrections are retained, but they do not clear these blockers. No candidate pass, merge, publication, release, Registry metadata, tag, or |
Corrective candidate receiptThis receipt supersedes stopped heads
Frozen source:
Package gate:
Real GoValidate gate:
Retrieval compatibility:
Local verification:
Still mandatory and pending on this exact head:
No merge is authorized unless every gate passes. This receipt authorizes no npm publication, GitHub Release, Registry metadata, tag, provider activity, or |
mohanagy
left a comment
mohanagy
left a comment
There was a problem hiding this comment.
Independent exact-head review for bff482c6fef28217d18021561038b406599c990a / tree 4a4710e56db949b2082270c1f3cb4d8ec8b3da27: NO BLOCKER.
The review covered the complete frozen implementation and governance diff, including lexical-capture/public-schema preservation after byte compaction, authenticated control/order/value facts, alias invalidation, persistence receiver proof, wrapper bounds, channel identity, switch/try/Promise semantics, stale-value removal, package/source ceilings, and the real GoValidate topology. The 51→42 change removes exactly nine duplicate outer-callsite projections; no unique producer→channel→consumer obligation is lost. Exact source, package, graph, retrieval, performance, and governance receipts match the candidate.
Local runner caveat is recorded without overclaim: default forks completed 79 files / 707 passing tests; the sole unstarted retrieve-context file passed 54/54 alone. A clean single-invocation full suite remains mandatory in hosted CI. Merge remains blocked on all six exact-head CI jobs, completed CodeRabbit review, and zero unresolved threads.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/canonical-index-execution-review-regressions.test.ts (1)
236-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the
armhelper.The helper calls
control.findthree times and then casts the frame. A single lookup with a narrowing check gives the same result and removes the cast.♻️ Proposed refactor
- const arm = (name: string): string | undefined => - byName.get(name)?.control.find((frame) => - frame.kind === 'branch')?.kind === 'branch' - ? (byName.get(name)!.control.find((frame) => - frame.kind === 'branch') as { arm: string }).arm - : undefined + const arm = (name: string): string | undefined => { + const frame = byName.get(name)?.control + .find((candidate) => candidate.kind === 'branch') + return frame?.kind === 'branch' ? frame.arm : undefined + }🤖 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 236 - 241, Update the arm helper to perform one control.find lookup for the requested name, narrow the returned frame by checking its kind, and return its arm directly when it is a branch; otherwise return undefined. Remove the repeated byName.get/control.find calls and the type cast.
🤖 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 `@docs/core-reset/removal-manifest.yml`:
- Around line 2233-2241: Update the verification list for the
semantic-execution-index-632 cohort to include both modified retrieval test
files, alongside the existing entries, so the manifest covers every changed
test.
In `@tests/unit/core-reset-governance.test.ts`:
- Line 2129: Update the git diff argument in the relevant test to use
--full-index instead of --abbrev=7, and replace the expected
EVIDENCE_SKELETON_RETRIEVAL_DIFF_SHA256 digest with
753ae098393b52f2c57ea4ffdc282ef19a28c0bb8e81698816af2c1f38df7570.
---
Nitpick comments:
In `@tests/unit/canonical-index-execution-review-regressions.test.ts`:
- Around line 236-241: Update the arm helper to perform one control.find lookup
for the requested name, narrow the returned frame by checking its kind, and
return its arm directly when it is a branch; otherwise return undefined. Remove
the repeated byName.get/control.find calls and the type cast.
🪄 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: b9117487-4f92-41c0-b6af-0217fba5b29c
📒 Files selected for processing (10)
docs/core-reset/removal-manifest.ymldocs/core-reset/scorecard.mddocs/roadmap.mdsrc/adapters/typescript/execution.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/core-reset-governance.test.tstests/unit/query-index-execution-validation.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/roadmap.md
- tests/unit/canonical-index-execution-hardening.test.ts
- docs/core-reset/scorecard.md
- src/domain/query/index-status.ts
- src/domain/index/model.ts
CodeRabbit correction candidate receiptThis head supersedes
The sole correction adds the two modified retrieval-evidence test files to #632’s verification list and records the exact focused cohort as 196 passing tests. Those added files pass 53/53; the governance contract passes 18/18. Production is byte-identical to the prior reviewed candidate:
CodeRabbit’s other new suggestion would rewrite the already accepted #625 historical diff-hash format from its recorded Run No publication, release, Registry metadata, tag, provider activity, or |
mohanagy
left a comment
mohanagy
left a comment
There was a problem hiding this comment.
Independent exact-head correction review for c9d2c6f730c5e4e426d6f2f4b1aa4c0623bb4067 / tree c46b93c85c415ec0036fbe5a5fe2cee06abc1e99: NO BLOCKER.
The delta changes only the #632 verification manifest and matching governance expectation. The newly enumerated adversarial and regression files pass 25 and 28 cases respectively, making the exact cohort 143 + 53 = 196. Every test path changed from protected base is now covered, plus the relevant stdio test. Production, package, graph, performance, retrieval, and source hashes remain byte-identical to the prior reviewed candidate.
The separate CodeRabbit suggestion to change completed #625 from its recorded --abbrev=7 diff representation to --full-index is correctly rejected as an immutable historical-receipt rewrite outside #632; the historical hash still reproduces. Merge remains blocked on all six CI jobs, a completed CodeRabbit review of this exact two-file delta rather than its current rate-limit skip, and zero unresolved threads.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Protected-
|
1 participant
Summary
madar.retrievev1 response; [P0][Retrieval]: Plan obligations and return strict answer-ready workflow dossiers #630 owns the v2 retrieval cutoverTesting
npm run test:run— exact-head hosted Ubuntu/Node 22 coverage suite in run30622792952; local split proved 761 unique tests without claiming a single clean local invocation on the busy Darwin hostnpm run typechecknpm run buildnpm pack --dry-run30622792952git diff --checkCore Reset contract
semantic-execution-index-632mainsrc/adapters/typescript/execution.tspath within the four-file program ceilingReset scope checks
src/Exact candidate
nextat9043320cfa08370e5cdd3911bfb9283005aa9912f51d6e75e3b806dec6caf9ff0be43fc2ab5713fcc9d2c6f730c5e4e426d6f2f4b1aa4c0623bb4067c46b93c85c415ec0036fbe5a5fe2cee06abc1e99next, nevermainMeasurements
+3,667/-187, net+3,480against the+3,500ceiling9f0c66e663f703afbb9a5e68f6037f9e211cba58sha512-3yYpFxnym0r9DF66IfS8w1MI01DMLU+hX6uQi6aQoBQvbeu6jHn8j059N6ml3MwMx3wvlj41Y3yAMIX3D2N2Aw==1.229171887466374835 publishes_to,7 consumed_by); the removed nine edges were duplicate outer-callsite projections0.594314079422382777270a6f0330a3ce85fbc42b90e7a3e99f8bf37776f6e65f5da8aad1bad3caaf87b4ef75473834708b20f1d2580b31470a710d797d7bdf55eee1d0876827a173Superseded stop disposition
Stopped heads
9fe3c244…,c977de03…, andf4ae6440…did not merge. The exact current head retains the prior corrections and adds dedicated generic regressions for all ninef4ae6440…false-proof classes: mutable Map channel identity, dead-tail reachability, computed credential targets, wrapper persistence multiplicity, switch fallthrough, nested secret taint, reassigned injected queues, mutated Promise inputs, and reassigned typed EventEmitter scope. The durable exact-head receipts are on #632.CodeRabbit completed substantive review on the byte-identical production candidate and its valid governance-cohort omission was corrected in this head. A genuine post-correction CodeRabbit rerun remains required; a summary refresh or temporary rate-limit status is not treated as completed review.
Checklist
Related issues
Closes #632
Related: #629
Successor after merge: #630
Summary by CodeRabbit
New Features
Bug Fixes
Documentation