[Core Reset] Return strict obligation-driven retrieval dossiers - #635
Conversation
📝 WalkthroughWalkthroughThe pull request replaces legacy retrieval with a version 2 obligation-driven workflow. It adds authenticated evidence hydration, structured answer dossiers, bounded proof validation, updated parity tooling, runtime fixtures, benchmarks, documentation, and Core Reset governance records. ChangesRetrieval v2
Estimated code review effort: 5 (Critical) | ~120 minutes 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/pipeline/workers/db-sync.worker.ts (1)
9-25: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftProvide a runtime report repository.
declare const reportRepositorycreates no runtime binding. When validation succeeds, Line 24 dereferences an undeclared identifier and fails thedb-sync-queuejob before persistence.Inject a configured repository into
DbSyncWorker, then pass it tosaveStructuredReport.🤖 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/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/pipeline/workers/db-sync.worker.ts` around lines 9 - 25, The saveStructuredReport flow currently relies on the non-runtime reportRepository declaration. Update DbSyncWorker to receive a configured MongoRepository<StoredReport>, then pass that repository into saveStructuredReport and use the injected instance for update; remove the undeclared global dependency while preserving the existing validation and return behavior.
🧹 Nitpick comments (11)
tests/unit/core-reset-governance.test.ts (3)
742-749: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport the missing path instead of a bare boolean.
gitPathExistsreturns onlytrueorfalse, and both call sites collapse a whole path list into one boolean. When the assertion fails, the report states only thatfalsewas received. A maintainer cannot tell which path is absent at the recorded revision.git cat-file -ealso writes itsfatal:message to the inherited stderr on every negative result, which adds noise to the test output.
tests/unit/core-reset-governance.test.ts#L742-L749: pass{ stdio: 'ignore' }toexecFileSyncso the negative case stays silent.tests/unit/core-reset-governance.test.ts#L5465-L5466: assert the filtered list of missing paths, for exampleexpect(EVIDENCE_REPLACEMENTS.filter((path) => !gitPathExists(EVIDENCE_IMPLEMENTATION, path))).toEqual([]).tests/unit/core-reset-governance.test.ts#L5943-L5944: apply the same filtered-list assertion.🤖 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/core-reset-governance.test.ts` around lines 742 - 749, Update gitPathExists in tests/unit/core-reset-governance.test.ts:742-749 to pass { stdio: 'ignore' } to execFileSync, keeping missing-path checks silent. At tests/unit/core-reset-governance.test.ts:5465-5466 and :5943-5944, replace boolean assertions with filtered-list assertions that collect paths for which gitPathExists returns false and expect the missing-path list to equal []; this ensures failures identify absent paths.
2119-2152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the candidate measurements against the recorded ceilings.
The test pins
delivery_limits,npm_package_budget, and the candidate measurements as independent literals. No assertion links them. If a later edit lowers a ceiling or raises a measurement, this test still passes while the manifest records a candidate that breaks its own gate. Add relational assertions so the governance gate is machine-checked.♻️ Proposed additional assertions
const limits = obligationRetrieval.delivery_limits const budget = obligationRetrieval.npm_package_budget const candidate = obligationRetrieval.candidate expect(candidate.replacement_measurement.emitted_bytes) .toBeLessThanOrEqual(limits.replacement_emitted_bytes_max) expect(candidate.replacement_measurement.source_loc) .toBeLessThanOrEqual(limits.replacement_source_loc_max) expect(candidate.source_measurement.production_typescript_loc) .toBeLessThanOrEqual(limits.total_production_loc_max) expect(candidate.source_measurement.net).toBeLessThanOrEqual(limits.net_production_loc_max) expect(candidate.package_measurement.files).toBeLessThanOrEqual(budget.files_max) expect(candidate.package_measurement.packed_bytes).toBeLessThanOrEqual(budget.packed_bytes_max) expect(candidate.package_measurement.unpacked_bytes) .toBeLessThanOrEqual(budget.unpacked_bytes_max) expect(limits.replacement_emitted_bytes_max) .toBe(candidate.amendment.replacement_emitted_bytes_max) expect(budget.unpacked_bytes_max).toBe(candidate.amendment.npm_unpacked_bytes_max)🤖 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/core-reset-governance.test.ts` around lines 2119 - 2152, Add relational governance assertions after constructing the obligation retrieval candidate, linking candidate replacement, source, and package measurements to their corresponding delivery limits and npm budget ceilings. Also assert that the amendment ceiling fields match limits.replacement_emitted_bytes_max and budget.unpacked_bytes_max, using obligationRetrieval, delivery_limits, npm_package_budget, candidate, and amendment.
1400-1409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared
manifest.currentexpectation. Six tests repeat the samemanifest.currentshape and the samein_progressidentity assertion. Each phase transition requires six identical edits, and this PR performs exactly that edit six times. A drift in one site is easy to miss. Extract one helper, for exampleexpectCurrentPhase(manifest, { withPackage: true }), that builds the object fromSEMANTIC_EXECUTION_INDEX_ID,OBLIGATION_RETRIEVAL_ID,OBLIGATION_RETRIEVAL_BASE,OBLIGATION_RETRIEVAL_SOURCE, andOBLIGATION_RETRIEVAL_PACKAGE, and asserts the solein_progressitem.
tests/unit/core-reset-governance.test.ts#L1400-L1409: define the helper and call it here with the package fields included.tests/unit/core-reset-governance.test.ts#L2687-L2696: call the helper without the package fields.tests/unit/core-reset-governance.test.ts#L3551-L3560: call the helper with the package fields.tests/unit/core-reset-governance.test.ts#L4065-L4070: call the helper without the package fields.tests/unit/core-reset-governance.test.ts#L4319-L4324: call the helper without the package fields.tests/unit/core-reset-governance.test.ts#L4950-L4962: call the helper with the package fields and the snapshot scope.🤖 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/core-reset-governance.test.ts` around lines 1400 - 1409, Repeated manifest.current expectations should be centralized to prevent drift. In tests/unit/core-reset-governance.test.ts#L1400-L1409, define an expectCurrentPhase helper that builds the shared expectation from SEMANTIC_EXECUTION_INDEX_ID, OBLIGATION_RETRIEVAL_ID, OBLIGATION_RETRIEVAL_BASE, and OBLIGATION_RETRIEVAL_SOURCE, includes package fields when requested, and asserts the sole in_progress item; replace the duplicated expectations at `#L1400-L1409` and `#L3551-L3560` with package fields, `#L2687-L2696`, `#L4065-L4070`, and `#L4319-L4324` without package fields, and `#L4950-L4962` with package fields plus snapshot scope.tools/eval/core-reset/benchmark.mjs (1)
43-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMetric ceilings duplicate
.github/scripts/verify-packed-retrieval-parity.mjs.Lines 46-54 repeat the same numeric ceilings (
selected_files <= 12,authenticated_excerpts <= 25,root_candidates <= 3,initial_candidates <= 32,explored_nodes <= 512,causal_hops <= 24,recovery_passes <= 2,recovery_frontier_nodes <= 64) also present in.github/scripts/verify-packed-retrieval-parity.mjs(lines 542-551). Extract these shared ceilings into one exported constants module used by both scripts, so a future ceiling change cannot update one gate but not the other.🤖 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 `@tools/eval/core-reset/benchmark.mjs` around lines 43 - 67, Extract the shared retrieval metric ceilings currently asserted in retrieve into an exported constants module, then import and reuse those constants in both benchmark.mjs and verify-packed-retrieval-parity.mjs. Replace the duplicated numeric thresholds for selected_files, authenticated_excerpts, root_candidates, initial_candidates, explored_nodes, causal_hops, recovery_passes, and recovery_frontier_nodes while preserving the existing assertions and behavior..github/scripts/verify-packed-retrieval-parity.mjs (1)
517-553: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMetric ceilings duplicate
tools/eval/core-reset/benchmark.mjs.Lines 542-551 repeat the same numeric ceilings (
selected_files,authenticated_excerpts,root_candidates,initial_candidates,explored_nodes,causal_hops,recovery_passes,recovery_frontier_nodes) that also appear intools/eval/core-reset/benchmark.mjs(lines 47-54). Extract these shared ceilings into one exported constants module. Two independent copies of the same gate thresholds can drift silently when one file changes and the other does not.The channel-link proof sequence check itself (Lines 523-539) is correct.
🤖 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 @.github/scripts/verify-packed-retrieval-parity.mjs around lines 517 - 553, Extract the shared metric ceilings for selected_files, authenticated_excerpts, root_candidates, initial_candidates, explored_nodes, causal_hops, recovery_passes, and recovery_frontier_nodes into one exported constants module, then import and reuse that module in both the verification flow around the packed-flow gate and tools/eval/core-reset/benchmark.mjs. Remove the duplicated numeric literals while preserving the existing threshold checks and leave the channel-link proof validation unchanged.src/application/evidence-hydrator.ts (1)
102-134: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMint entity aliases from a monotonic counter, not from
ents.size.
entderives the alias ase${ents.size}at Line 105, and theopsloop derivese${ents.size}at Line 176.factdeletes a speculative owner entry at Line 124, which lowersents.sizeand frees an alias for reuse.This is safe only because the delete at Line 124 runs immediately after the matching
ent(owner)at Line 123, so the removed entry is always the most recently minted one. If any insertion is ever added between those two statements, two live entries receive the sameeNalias, andretrieve-context.tspackthen binds proofs, links, and obligations to the wrong entity with no error.Use a counter that never decreases so the alias space cannot collide.
🛡️ Suggested change
cuts = new Map<string, HydratedExcerpt>(), refs = new Map<string, HydratedProof>(), used = new Set<string>() + let aliases = 0 @@ - const a = node(id), ref = `e${ents.size}`, ch = i.channels_by_id.get(id) + const a = node(id), ref = `e${aliases++}`, ch = i.channels_by_id.get(id)Apply the same
aliases++mint at Line 176.🤖 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/application/evidence-hydrator.ts` around lines 102 - 134, Replace all entity alias generation based on ents.size with a monotonic aliases counter, incrementing it whenever a new entity alias is minted in ent and in the ops loop. Ensure speculative deletion in fact cannot cause alias reuse, while preserving existing entity references and serialization behavior.tests/unit/evidence-hydrator.test.ts (1)
470-476: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the source-text ordering assertion.
These lines read
evidence-hydrator.tsand compare the character offsets of two source strings. The assertion breaks on any reformat or rename insideload, and it proves nothing about behavior. The guarantee that matters is already asserted at Lines 467-469: a symlink that escapes the indexed root yields{ state: 'unavailable', subject: path }.🧹 Suggested removal
expect(hydrateEvidence(value.index, value.targets)).toEqual({ state: 'unavailable', subject: path, }) - const implementation = readFileSync( - new URL('../../src/application/evidence-hydrator.ts', import.meta.url), - 'utf8', - ) - expect(implementation.indexOf('const rel = relative(root, file)')) - .toBeLessThan(implementation.indexOf('buf = readFileSync(file)')) })If the intent is to pin containment checks ahead of the read, add a behavioral case instead: point the symlink at a path inside the root and assert the result stays authenticated.
🤖 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/evidence-hydrator.test.ts` around lines 470 - 476, Remove the source-text ordering assertion that reads evidence-hydrator.ts and compares the positions of `const rel = relative(root, file)` and `buf = readFileSync(file)`. Keep the existing behavioral assertion for symlinks escaping the indexed root; do not replace it with another source-inspection check.tests/unit/retrieve-context-proof-eviction.test.ts (1)
164-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the fake index the fields
packcan read.
{ state: 'ready' } as neverworks today only because no test supplies a control group.packreadsindex.operation_by_id.get(id)for every group that has acontrollerOperationId. If a later test adds such a group, the call throws a TypeError,retrieveContextcatches it at thedossier packinghandler, and the test reportscorruptinstead of the real cause.Build the stub once with the collections
packtouches.♻️ Suggested stub
+const readyIndex = { + state: 'ready', operation_by_id: new Map(), operations_by_owner: new Map(), + channels_by_id: new Map(), +} as never +Then pass
readyIndexat eachretrieveContextcall site in this file.🤖 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/retrieve-context-proof-eviction.test.ts` around lines 164 - 166, Define a shared readyIndex stub in retrieve-context-proof-eviction.test.ts with the collections and fields pack accesses, including operation_by_id, then replace the `{ state: 'ready' } as never` argument at every retrieveContext call site with readyIndex. Preserve the existing ready-state behavior while ensuring control groups can be packed without TypeError.src/domain/query/types.ts (1)
27-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the shared type aliases descriptive names.
This file is the public retrieval contract. The aliases
L,M,FS,EF,MK,SF,Tag,DR,PR,OR, andWRare used across many exported types, so a reader must resolve ten one-letter symbols to understandWorkflowSelection,AnswerDossier, orRetrieveContextResult. Rename them to intent-revealing names. The change is local to this module because the aliases are not exported.♻️ Suggested renames
-type L<T> = readonly T[] -type M<T> = ReadonlyMap<string, T> -type FS = 'stale' | 'unavailable' | 'corrupt' -type EF = { state: FS; subject: string } +type List<T> = readonly T[] +type Index<T> = ReadonlyMap<string, T> +type FailureState = 'stale' | 'unavailable' | 'corrupt' +type EvidenceFailure = { state: FailureState; subject: string }Also applies to: 92-99
🤖 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/types.ts` around lines 27 - 51, Rename the local aliases L, M, FS, EF, and MK to descriptive, intent-revealing names, and apply the same treatment to the additional local aliases SF, Tag, DR, PR, OR, and WR in this module. Update every reference within exported retrieval contracts such as RetrieveState and RetrieveMetrics while preserving the existing types and public API.tests/unit/query-workflow.test.ts (1)
30-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the obligation literals instead of casting the whole plan.
The
as QueryPlancast at Line 43 suppresses checking of the object literals above it. Thekindvalues widen tostring, so a future change toObligationKindor toQueryObligationwill not fail compilation in this suite. Annotate the array so the fixtures stay bound to the contract.♻️ Suggested typing
-function plan(intent: QueryPlan['intent'], subject = 'idea report'): QueryPlan { - const obligations = intent === 'workflow' ? [ +function plan(intent: QueryPlan['intent'], subject = 'idea report'): QueryPlan { + const obligations: readonly QueryObligation[] = intent === 'workflow' ? [ { id: 'o1', kind: 'subject', target: subject, mandatory: true }, @@ - return { intent, subject, terms: subject.split(' ').sort(), obligations } as QueryPlan + return { intent, subject, terms: subject.split(' ').sort(), obligations } }Add
QueryObligationto the type import from../../src/domain/query/types.js.🤖 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-workflow.test.ts` around lines 30 - 44, Update the test fixture’s type import to include QueryObligation, annotate the obligations array in plan with QueryObligation[], and remove the whole-object as QueryPlan cast so each obligation literal is checked against the contract.src/application/retrieve-context.ts (1)
71-76: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCompute
serialized_tokensby bounded convergence.
sealcurrently estimates the final count from the count withserialized_tokens: 0. This does not guarantee that the stored value equalscountTokens(json(out)). Measure the complete serialized result after each assignment, stop only when the value stabilizes, and handle capped or oscillating sequences conservatively so budget checks cannot undercount.🤖 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/application/retrieve-context.ts` around lines 71 - 76, Update seal to compute out.metrics.serialized_tokens through bounded iterative convergence: assign a candidate, measure countTokens(json(out)) including that assignment, and repeat until the stored value equals the measured value. Apply a maximum iteration bound and conservatively retain the highest observed count if the sequence oscillates or fails to stabilize, ensuring budget checks never undercount.
🤖 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 2570-2582: Qualify the package_ceiling_change constraint in the
manifest’s constraints block to explicitly permit only the already recorded
owner amendment raising npm_unpacked_bytes_max from 640,000 to 655,000, while
continuing to forbid any additional package ceiling changes. Preserve the
existing npm_package_budget.unpacked_bytes_max value and all other constraints.
In `@src/domain/query/workflow.ts`:
- Around line 202-215: Replace the two per-arc arcs.some scans inside the
channel-arc filter with lookup maps built from the pre-filter arcs array: index
arcs by source symbol and by (from, to, kind), then use constant-time lookups to
evaluate the redundant condition. Preserve the existing fact checks, hidden-edge
updates, filtering semantics, and final sort.
In
`@tests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/pipeline/api/queue-registry.service.ts`:
- Around line 1-2: Declare bullmq, `@nestjs/common`, and typeorm in the fixture
workspace package.json dependencies or devDependencies so the workspace resolves
correctly. Apply this dependency fix for the affected imports in
tests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/pipeline/api/queue-registry.service.ts
(lines 1-2),
tests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/ideas/interface/http/idea-generation.controller.ts
(line 1), and
tests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/pipeline/workers/db-sync.worker.ts
(lines 1-2); alternatively exclude the fixture from dependency resolution.
- Around line 43-48: Update QueueRegistryService.registerWorker to retain each
created Worker, and retain the four queue instances created by the service. Add
or use the owning lifecycle teardown hook to close every retained worker and
queue, ensuring all BullMQ resources are released during shutdown.
In `@tests/unit/retrieve-context.test.ts`:
- Around line 267-274: Remove the wall-clock sampling and p95 latency assertion
from the test containing the retrieveContext loop, and delete the now-unused
performance import. Rename the test so its title describes correctness or
convergence rather than a latency requirement, while preserving the existing
retrieveContext correctness assertions.
In `@tools/eval/core-reset/verify-isolation.mjs`:
- Line 267: Update the package budget overage message to use a neutral “selected
ceilings” label instead of “active ceilings,” covering both the active-phase and
evaluationPackageBudget cases while preserving the existing measurements and
limits.
---
Outside diff comments:
In
`@tests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/pipeline/workers/db-sync.worker.ts`:
- Around line 9-25: The saveStructuredReport flow currently relies on the
non-runtime reportRepository declaration. Update DbSyncWorker to receive a
configured MongoRepository<StoredReport>, then pass that repository into
saveStructuredReport and use the injected instance for update; remove the
undeclared global dependency while preserving the existing validation and return
behavior.
---
Nitpick comments:
In @.github/scripts/verify-packed-retrieval-parity.mjs:
- Around line 517-553: Extract the shared metric ceilings for selected_files,
authenticated_excerpts, root_candidates, initial_candidates, explored_nodes,
causal_hops, recovery_passes, and recovery_frontier_nodes into one exported
constants module, then import and reuse that module in both the verification
flow around the packed-flow gate and tools/eval/core-reset/benchmark.mjs. Remove
the duplicated numeric literals while preserving the existing threshold checks
and leave the channel-link proof validation unchanged.
In `@src/application/evidence-hydrator.ts`:
- Around line 102-134: Replace all entity alias generation based on ents.size
with a monotonic aliases counter, incrementing it whenever a new entity alias is
minted in ent and in the ops loop. Ensure speculative deletion in fact cannot
cause alias reuse, while preserving existing entity references and serialization
behavior.
In `@src/application/retrieve-context.ts`:
- Around line 71-76: Update seal to compute out.metrics.serialized_tokens
through bounded iterative convergence: assign a candidate, measure
countTokens(json(out)) including that assignment, and repeat until the stored
value equals the measured value. Apply a maximum iteration bound and
conservatively retain the highest observed count if the sequence oscillates or
fails to stabilize, ensuring budget checks never undercount.
In `@src/domain/query/types.ts`:
- Around line 27-51: Rename the local aliases L, M, FS, EF, and MK to
descriptive, intent-revealing names, and apply the same treatment to the
additional local aliases SF, Tag, DR, PR, OR, and WR in this module. Update
every reference within exported retrieval contracts such as RetrieveState and
RetrieveMetrics while preserving the existing types and public API.
In `@tests/unit/core-reset-governance.test.ts`:
- Around line 742-749: Update gitPathExists in
tests/unit/core-reset-governance.test.ts:742-749 to pass { stdio: 'ignore' } to
execFileSync, keeping missing-path checks silent. At
tests/unit/core-reset-governance.test.ts:5465-5466 and :5943-5944, replace
boolean assertions with filtered-list assertions that collect paths for which
gitPathExists returns false and expect the missing-path list to equal []; this
ensures failures identify absent paths.
- Around line 2119-2152: Add relational governance assertions after constructing
the obligation retrieval candidate, linking candidate replacement, source, and
package measurements to their corresponding delivery limits and npm budget
ceilings. Also assert that the amendment ceiling fields match
limits.replacement_emitted_bytes_max and budget.unpacked_bytes_max, using
obligationRetrieval, delivery_limits, npm_package_budget, candidate, and
amendment.
- Around line 1400-1409: Repeated manifest.current expectations should be
centralized to prevent drift. In
tests/unit/core-reset-governance.test.ts#L1400-L1409, define an
expectCurrentPhase helper that builds the shared expectation from
SEMANTIC_EXECUTION_INDEX_ID, OBLIGATION_RETRIEVAL_ID, OBLIGATION_RETRIEVAL_BASE,
and OBLIGATION_RETRIEVAL_SOURCE, includes package fields when requested, and
asserts the sole in_progress item; replace the duplicated expectations at
`#L1400-L1409` and `#L3551-L3560` with package fields, `#L2687-L2696`, `#L4065-L4070`,
and `#L4319-L4324` without package fields, and `#L4950-L4962` with package fields
plus snapshot scope.
In `@tests/unit/evidence-hydrator.test.ts`:
- Around line 470-476: Remove the source-text ordering assertion that reads
evidence-hydrator.ts and compares the positions of `const rel = relative(root,
file)` and `buf = readFileSync(file)`. Keep the existing behavioral assertion
for symlinks escaping the indexed root; do not replace it with another
source-inspection check.
In `@tests/unit/query-workflow.test.ts`:
- Around line 30-44: Update the test fixture’s type import to include
QueryObligation, annotate the obligations array in plan with QueryObligation[],
and remove the whole-object as QueryPlan cast so each obligation literal is
checked against the contract.
In `@tests/unit/retrieve-context-proof-eviction.test.ts`:
- Around line 164-166: Define a shared readyIndex stub in
retrieve-context-proof-eviction.test.ts with the collections and fields pack
accesses, including operation_by_id, then replace the `{ state: 'ready' } as
never` argument at every retrieveContext call site with readyIndex. Preserve the
existing ready-state behavior while ensuring control groups can be packed
without TypeError.
In `@tools/eval/core-reset/benchmark.mjs`:
- Around line 43-67: Extract the shared retrieval metric ceilings currently
asserted in retrieve into an exported constants module, then import and reuse
those constants in both benchmark.mjs and verify-packed-retrieval-parity.mjs.
Replace the duplicated numeric thresholds for selected_files,
authenticated_excerpts, root_candidates, initial_candidates, explored_nodes,
causal_hops, recovery_passes, and recovery_frontier_nodes while preserving the
existing assertions and behavior.
🪄 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: 500f7f8d-fe4a-491e-b5d7-2e21773535df
📒 Files selected for processing (46)
.github/scripts/verify-packed-retrieval-parity.mjsdocs/core-reset/removal-manifest.ymldocs/core-reset/scorecard.mddocs/designs/2026-07-19-core-reset.mddocs/roadmap.mdsrc/adapters/mcp/protocol.tssrc/application/evidence-hydrator.tssrc/application/retrieve-context.tssrc/domain/query/plan.tssrc/domain/query/rank.tssrc/domain/query/slice.tssrc/domain/query/traverse.tssrc/domain/query/types.tssrc/domain/query/workflow.tstests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/ideas/interface/http/idea-generation.controller.tstests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/pipeline/api/pipeline-trigger.service.tstests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/pipeline/api/queue-registry.service.tstests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/pipeline/assembly/assembly.worker.tstests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/pipeline/workers/db-sync.worker.tstests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/pipeline/workers/orchestrator.worker.tstests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/planning/planner.service.tstests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/reports/assembly.service.tstests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/research/research-agent.service.tstests/fixtures/pack-quality/runtime-generation-explain-report-flow/workspace/src/modules/research/workers/section-research.worker.tstests/unit/benchmark-quality.test.tstests/unit/benchmark-runtime-proof.test.tstests/unit/benchmark-suite-isolation-docs.test.tstests/unit/benchmark.test.tstests/unit/core-reset-governance.test.tstests/unit/evidence-hydrator.test.tstests/unit/query-index-execution-validation.test.tstests/unit/query-plan.test.tstests/unit/query-workflow.test.tstests/unit/retrieve-context-proof-eviction.test.tstests/unit/retrieve-context.test.tstests/unit/retrieve-evidence-skeleton-adversarial.test.tstests/unit/retrieve-evidence-skeleton-regression.test.tstests/unit/retrieve-v2-contract-gaps.test.tstests/unit/sample-workspace.test.tstests/unit/stdio-server.test.tstests/unit/try-command.test.tstools/eval/core-reset/benchmark.mjstools/eval/core-reset/isolation-support.mjstools/eval/core-reset/verify-isolation.mjstools/eval/lib/infrastructure/benchmark/quality.tstools/eval/lib/infrastructure/benchmark/questions.ts
💤 Files with no reviewable changes (5)
- src/domain/query/traverse.ts
- src/domain/query/slice.ts
- src/domain/query/rank.ts
- tests/unit/retrieve-evidence-skeleton-regression.test.ts
- tests/unit/retrieve-evidence-skeleton-adversarial.test.ts
Exact-head local validation and independent-review receiptCandidate head: Local validation passed:
Fresh independent exact-head review used separate Standards and Spec axes and reports no P0/P1 blocker. Earlier documentation reviewers' findings were corrected before this commit, then re-reviewed with no blocker. The two remaining old fixture comments were answered on the exact head: the frozen 18-file corpus is intentionally static, excluded from dependency resolution/typechecking, never executed, and attested byte-for-byte. Both threads are now resolved. Hosted exact-head CI and CodeRabbit are still pending. This receipt does not represent them as complete and does not authorize npm publication, a release, Registry publication, a tag, or |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
examples/mcp-tool-examples.md (1)
40-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the metrics key order with the canonical serialization.
The document presents this JSON as canonical.
stat()insrc/application/retrieve-context.ts(lines 44-75) emitsrecovery_passes, thenrecovery_frontier_nodes, thenalternate_seeds. The example placesrecovery_passeslast. The key set is correct, only the order differs.♻️ Proposed fix
"causal_hops": 0, + "recovery_passes": 0, "recovery_frontier_nodes": 0, - "alternate_seeds": 0, - "recovery_passes": 0 + "alternate_seeds": 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 `@examples/mcp-tool-examples.md` around lines 40 - 55, Reorder the metrics keys in the JSON example to match the canonical order emitted by stat(): place recovery_passes first, followed by recovery_frontier_nodes and alternate_seeds, while leaving all values and other keys unchanged.tools/eval/core-reset/verify-isolation.mjs (1)
291-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFail with a clear message when the
#630manifest item is absent.Lines 293-300 guard
obligationRetrievalwith optional chaining. Line 301 then readsobligationRetrieval.delivery_limitsdirectly. The earlier assertion protects that access today, so the behavior is correct. However, if the manifest lacks the item, lines 117-127 already readdistoutputs from an empty source list and the failure message never names the missing manifest item. An explicit precondition makes the failure self-describing.♻️ Proposed precondition
+assert( + obligationRetrieval?.delivery_limits !== undefined, + "removal manifest is missing the obligation-driven-retrieval-630 item", +) assert( replacementSources.length === 3🤖 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 `@tools/eval/core-reset/verify-isolation.mjs` around lines 291 - 306, The verification flow should explicitly assert that the `#630` obligation retrieval and its candidate manifest item exist before reading replacement measurements or delivery_limits. Add a clear precondition near the existing `#630` assertions, referencing obligationRetrieval, so a missing manifest item fails with a self-describing message instead of relying on downstream access.docs/integrations/agent-orchestration.md (1)
60-64: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a freshness rule before reusing dossier evidence.
A ready dossier is authenticated against the accepted graph. After a worker changes an indexed file, the old dossier can contain stale hashes or ranges. Require
madar generate .and a freshretrievebefore workers or reviewers use dossier claims for changed files. Keep continuation reuse only when no indexed source changed.🤖 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/integrations/agent-orchestration.md` around lines 60 - 64, Update the dossier reuse guidance in the lead agent, implementation worker, and reviewer workflow to require running madar generate . followed by a fresh retrieve whenever a worker changes an indexed file before relying on dossier claims for those files. Preserve continuation-context reuse only when no indexed source has changed.
🤖 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 @.github/workflows/ci.yml:
- Around line 75-78: Update the CI workflow step invoking
tools/eval/core-reset/benchmark.mjs so hosted GitHub Actions runs do not enforce
the environment-dependent warm retrieval p95 latency assertion. Preserve the
benchmark’s correctness and determinism assertions in CI, while restricting the
latency gate to the local reference protocol or disabling it via an explicit CI
environment flag.
In `@docs/concepts/pipelines.md`:
- Line 33: Update the retrieval-bound descriptions in docs/concepts/pipelines.md
at lines 33-33 and docs/reference/cli-and-mcp.md at lines 51-51 to include
recovery_frontier_nodes: 64 and alternate_seeds: 3, or explicitly mark each list
as non-exhaustive.
In `@docs/reference/cli-and-mcp.md`:
- Line 51: Update the bounds summary near the `question` and `budget`
documentation to include `recovery_frontier_nodes: 64` and `alternate_seeds: 3`,
matching the values documented in `docs/mcp-response-shape.md`; alternatively,
explicitly label the list as non-exhaustive.
- Line 59: Update the retrieve row’s non-ready payload description to say it
returns an exact non-ready state with its missing requirements, reason, or
failures, covering the distinct payload fields for unsupported, stale,
unavailable, and corrupt results.
In `@src/domain/query/plan.ts`:
- Around line 190-191: Update the qualified-name extraction in the plan
construction logic so the regex preserves identifiers with multiple
dot-separated segments, such as api.client.fetch, rather than truncating them
after the first separator. Keep the existing names mapping and precedence
behavior intact while ensuring all qualified segments remain in each captured
name.
In `@tests/unit/why-madar-doc.test.ts`:
- Line 30: Update the retired-term assertion in why-madar-doc.test.ts to reject
the distinctive multi-word phrase used by the retired v1 terminology instead of
the generic word “boundaries”; follow the existing phrase-based assertions on
lines 28–29, or use the quoted `"boundaries"` form only if the retired term is a
JSON field.
In `@tools/eval/core-reset/benchmark.mjs`:
- Around line 264-268: Update the controller validation in the benchmark checks
around group.controller so it accepts the selector formats emitted by
retrieve-context.ts, including single indexes, contiguous ranges such as 0-3,
and dot-separated indexes such as 0.2.5. Keep validating the controller
identifier with controls.has, and replace the Number(ordinal) safe-integer
assertion with shape validation that rejects malformed or empty selectors.
---
Nitpick comments:
In `@docs/integrations/agent-orchestration.md`:
- Around line 60-64: Update the dossier reuse guidance in the lead agent,
implementation worker, and reviewer workflow to require running madar generate .
followed by a fresh retrieve whenever a worker changes an indexed file before
relying on dossier claims for those files. Preserve continuation-context reuse
only when no indexed source has changed.
In `@examples/mcp-tool-examples.md`:
- Around line 40-55: Reorder the metrics keys in the JSON example to match the
canonical order emitted by stat(): place recovery_passes first, followed by
recovery_frontier_nodes and alternate_seeds, while leaving all values and other
keys unchanged.
In `@tools/eval/core-reset/verify-isolation.mjs`:
- Around line 291-306: The verification flow should explicitly assert that the
`#630` obligation retrieval and its candidate manifest item exist before reading
replacement measurements or delivery_limits. Add a clear precondition near the
existing `#630` assertions, referencing obligationRetrieval, so a missing manifest
item fails with a self-describing message instead of relying on downstream
access.
🪄 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: 24b67374-333f-4ad3-a5eb-97c6c57b1863
📒 Files selected for processing (37)
.github/workflows/ci.ymlREADME.mddocs/agent-governance.mddocs/concepts/pipelines.mddocs/core-reset/removal-manifest.ymldocs/core-reset/scorecard.mddocs/designs/2026-07-19-core-reset.mddocs/indexing-completeness.mddocs/integrations/agent-orchestration.mddocs/language-capability-matrix.mddocs/mcp-response-shape.mddocs/proof-workflows.mddocs/reference/cli-and-mcp.mddocs/roadmap.mddocs/security/mcp-threat-model.mddocs/tutorials/agent-quickstarts.mddocs/tutorials/getting-started.mddocs/tutorials/sample-workspace.mdexamples/mcp-tool-examples.mdexamples/why-madar.mdsrc/application/evidence-hydrator.tssrc/application/retrieve-context.tssrc/domain/query/plan.tssrc/domain/query/types.tssrc/domain/query/workflow.tstests/unit/agent-governance-doc.test.tstests/unit/canonical-index-execution-review-regressions.test.tstests/unit/core-reset-governance.test.tstests/unit/evidence-hydrator.test.tstests/unit/mcp-response-shape-doc.test.tstests/unit/query-plan.test.tstests/unit/query-workflow.test.tstests/unit/retrieve-context-proof-eviction.test.tstests/unit/retrieve-context.test.tstests/unit/why-madar-doc.test.tstools/eval/core-reset/benchmark.mjstools/eval/core-reset/verify-isolation.mjs
💤 Files with no reviewable changes (1)
- tests/unit/evidence-hydrator.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/unit/retrieve-context-proof-eviction.test.ts
- src/application/evidence-hydrator.ts
- docs/roadmap.md
- docs/designs/2026-07-19-core-reset.md
- docs/core-reset/removal-manifest.yml
- src/application/retrieve-context.ts
- src/domain/query/types.ts
- tests/unit/retrieve-context.test.ts
- docs/core-reset/scorecard.md
Exact-head merge-gate receiptCandidate head: All required gates pass on this exact head:
The live remote |
1 participant
Summary
madar.retrievev2 dossiers only when every mandatory stage, adjacent async handoff, terminal action, and proof is completeIssue: #630
Owner amendment: #630 (comment)
Protected base:
c88823ecbeb6da6284cf74ecbd304e9315ffd4faCandidate head:
5ec3fab427985e3385a94cff1f4ef08b292c3ad2Candidate tree:
5db67cbe19a9479409192558ea40b0ac8e3add78Exact local gates
PasswordPolicyparent label, above the frozen >=90% CI threshold76340caade75454a96e546117c55128e1a69d15720dc60d1a800f5ceb497169377add32848cfd6f94be700dabe78efabf2bc3ed9; integritysha512-mj2bYY6JbNS8iIxWnq0bZSuNqdkQWlb3bLbob0wAodCxpT6iuuYFc75hhuZM7ijesm9rx94mo5iZ8VoKEsEr+g==; artifact SHA-2561db61f9760fc933de44faa34543d97775548ac8e96fbe40cbae03623de20164aMerge gates
Do not merge until this exact head has all six protected CI jobs green, fresh independent exact-head review reports no blocker, and zero review threads remain.
Exact-head CodeRabbit succeeded; all six protected CI jobs passed in run 30727068191; three independent exact-head reviews found no P0/P1 blocker; and all 13 review threads are resolved.
This PR targets protected
next, nevermain. It does not authorize npm publication, a GitHub Release, Registry publication, a tag, or anymainaction.Summary by CodeRabbit
ready,incomplete,unsupported,stale,unavailable, andcorruptresponse states.