Repository navigation
feat(codegraph-adapter): SDK-backed provider reads (#574 S5) - #597
Conversation
CodeGraphCliProvider keeps the CLI only to build the index (sync) and reads buildContext, findSymbols and impact through one read-only session per read. buildContext takes its file set from findRelevantContext and its query nodes from searchNodes (C6). The explore, query, node and impact CLI reads and their parsers (parseExploreSymbols, parseNodeTrail, trailEntries, normalizeCliNode) are removed. Refs #574
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
repo-harness acceptance claim |
Ancienttwo
left a comment
There was a problem hiding this comment.
repo-harness acceptance review
Round: 1
Subject: f14b6a8
Tier: high
Verdict: FAIL
Reviewer: codex (gpt-6.1-terra)
Model relation: different-family
Cross-family review of S5 at f14b6a8 against 6efd32a. The provider removes its CLI reads and text parsers, opens a read-only session per operation and closes it in finally; the real-index tests cover the main branches and the frozen fixture is unchanged. The impact edge semantics depart from the design, and two failures escape the error mapping.
NITs
None.
|
repo-harness acceptance record
Model relation: different-family Findings:
|
|
repo-harness acceptance claim |
Ancienttwo
left a comment
There was a problem hiding this comment.
repo-harness acceptance review
Round: 1
Subject: f14b6a8
Tier: high
Verdict: PASS
Reviewer: pi (claude-opus-5-5)
Model relation: same-family
Fresh same-family review (Pi, Opus 5.5) of S5. No CLI read remains; one session per read, closed in finally (open/close counts tested); C6 implemented as decided with deterministic output (three-run probe identical); error mapping correct, nothing retried or opened writable; tests real and budget matches numstat; frozen fixture unchanged. P3 notes only.
NITs
- F1: cli-provider.ts:56 CALL_EDGE_KINDS is calls/instantiates, while the design summary says calls/references; keep the code (NormalizedEdge has no references kind) and record a design amendment on #574.
- F2: codegraph-adapter.test.ts:544,576-578 the ambiguous-name and findSymbols tests never pin the union or tie-break order; assert the exact arrays.
- F3: cli-provider.ts:158 openCodeGraphReadSession sits outside the try, so a raw CodeGraphProviderUnavailableError escapes where readiness maps it to codegraph-unavailable.
- F4: cli-provider.ts:166-170 a throw while the index moved is reported as non-retryable index-open-failed; check unchangedSinceOpen() in the catch and report build-in-progress (G4).
- F5: codegraph-adapter.test.ts:592-603 the read-only refusal is fault-injected; findRelevantContext makes no heal write in 1.6.2, so the PR body's segment-vocab heal wording overstates it.
|
repo-harness acceptance record
Model relation: same-family Findings:
|
Preserve the S5 round-2 worker fixes for cross-machine continuation. Updated-head review remains pending.
|
repo-harness acceptance claim |
Ancienttwo
left a comment
There was a problem hiding this comment.
repo-harness acceptance review
Round: 2
Subject: d59c8f8
Tier: high
Verdict: PASS
Reviewer: pi (gpt-6-astra)
Model relation: different-family
Round-2 fix-delta and sibling review: round-1 F1 and F2 are resolved. The approved #574 amendment limits callers/callees to calls and instantiates; the new real-index type-reference assertion retains both affected files with empty call arrays. Provider-load errors map to the readiness FailureError envelope, close failures cannot replace mapped read errors, and a moved index remains retryable even when the read throws. Exact union/search ordering assertions strengthen coverage, and the read-only write test now accurately describes fault injection. All checks reported by gh pr checks 597 pass on the confirmed subject; the PR records 120 passing adapter tests and a clean typecheck on this content. No passing test/typecheck was rerun. No P0-P2 finding; one nonblocking error-path coverage note. S6 impact-port deletion remains outside this round.
NITs
- N1: Error-boundary coverage siblings (packages/local-runtime/codegraph-adapter/test/codegraph-adapter.test.ts:480-505): the new combined read/close-throw case exercises preservation of index-open-failed, but leaves successful-read/close-throw, moved-index/close-throw (with either a successful or throwing read), and post-open revision-check throws unpinned. The read/close case also overrides the fixture close counter and expects zero, so it cannot detect omission of the close attempt. In the same refusal table, add those branches and count close attempts before throwing: expect index-open-failed after a good read or an unprovable revision, preserve retryable code-facts-not-ready after a proven moved index, assert sanitized messages and one close attempt. The current implementation handles these branches on inspection; this is a regression-coverage improvement, not a blocking behavior defect.
|
repo-harness acceptance record
Model relation: different-family Findings:
|
|
repo-harness acceptance claim |
Ancienttwo
left a comment
There was a problem hiding this comment.
repo-harness acceptance review
Round: 2
Subject: d59c8f8
Tier: high
Verdict: PASS
Reviewer: claude (claude-opus)
Model relation: same-family
Delta check f14b6a8..d59c8f8: no new defects or regressions. read() now maps provider-load failure to the readiness gate's code-facts-unavailable shape, keeps the read's own error over a failing close, and reports a throw while the index moved as retryable build-in-progress (G4). All 5 round-1 P3s are fixed: F1 doc comment cites the #574 amendment and a real-index type-reference case pins affectedPaths without callers (test:597); F2 exact arrays pin the findSymbols tie-break (test:575) and caller order (test:615); F3 open is inside try (cli-provider.ts:169) with a provider-unavailable test; F4 catch consults indexMoved, covered by the 'read throws while the index moves' case; F5 test comment and PR body now say fault-injected. GPT F1/F2 are addressed by the same changes. CI green on the subject (gh pr checks 597).
NITs
- F1: cli-provider.ts:200-202 codeGraphUnavailable() restates the FailureError that repository-code-facts.ts:211 builds inline for the same codegraph-unavailable refusal; two constructions of one contract can drift. Export one helper from the readiness module and use it at both sites.
- F2: cli-provider.ts:183 the branch where close() throws after a good read (index-open-failed) has no test case; the refusal table at test:493 covers only close failing after a failed read. One more row would pin it.
|
repo-harness acceptance record
Model relation: same-family Findings:
|
|
Joint decision (orchestrator + advisor), 2026-11-14, on closing the round-1 threads of #597 after the machine switch.
Then: mark ready, merge with Seat: orchestrator t366 |
|
Review-bot reference: no findings at f14b6a8: no action Seat: orchestrator t366 |
Summary
Refs #574
Tier: High (GPT plus Opus review). S5 of ADR-0058: SDK-backed provider reads. Base:
main6efd32a (S1 #589, S1b #596).Goal
CodeGraphCliProvideruses the CodeGraph CLI only to build the index (syncundercodeGraphChildEnv).buildContext,findSymbolsandgetImpactRadiusread through one S1 read session per read (openCodeGraphReadSession, read-only, closed after settling the read). No read starts a CodeGraph process, and no read opens the index writable.Changes (
codegraph-adapter/src/cli-provider.tsonly)CodeGraphCliIndexBuilderkeepssync. The provider composes it with SDK reads.runCodeGraphClidelegates to one sharedrunCodeGraphInvocationand keeps its signature (S2 still uses it).explore,query,node,impact),parseExploreSymbols,parseNodeTrail,trailEntries,normalizeCliNode,syntheticSymbolIdand thefindSymbolsfallback insidebuildContext. There is no CLI path, no snapshot path and no mode branch.buildContext(C6). The file set comes fromfindRelevantContext(query): entry-point files first, then node order, capped atmaxSymbols. The query nodes come fromsearchNodes. Symbols are the non-import query nodes, followed by the index's file nodes for the relevant files (getNodesInFiles). Import edges are unchanged: the same scopedimport <paths>search withkinds: ["import"], resolved byimportEdgesFromQueryNodes. The digest and evidence bind the relevant-file list where they used to bind theexploremarkdown. The evidence summary is nowObserved by CodeGraph for task: ….findSymbols.searchNodes(query, { limit, kinds })in a total order: hand-written files before generated ones (generatedFilePredicate, as the CLI ranks them), then score, then file path, start line and id. Equal scores therefore never reorder.symbolIdresolves to the node with that id. Otherwise it resolves to every node whose exact qualified name or exact name matches. An unknown symbol gives an empty result, and an ambiguous name gives the union over its definitions.affectedPathslists the unique, sorted files ofgetImpactRadius(id, depth)for each definition.callersandcalleesare the directcalls/instantiatesedges of the definitions, with CodeGraph node ids on both ends.index-missing→AC_CODE_FACTS_UNAVAILABLE/index-missingAC_CODE_FACTS_UNAVAILABLE/index-open-failedAC_CODE_FACTS_NOT_READY/code-facts-not-ready(index-incomplete, retryable)index-open-failed, with no provider text in it. It is never retried, and never with a writable open.unchangedSinceOpen()is false, design G4) is refused like a build in progress.index.ts,read-session.ts,real-index.ts,read-provider-fixture.tsandfailure.tsare untouched. The class keeps the nameCodeGraphCliProviderand its constructor(root, binary, readProvider?). A rename would touchindex.ts(S4) andruntime-daemon, so it is left for S6 if wanted.C6 before/after (same fixture as the new real-index tests)
buildContext("change the cart total pricing", { maxSymbols: 8, changedPaths: ["src/checkout/cart.ts"] })on the original five-file fixture (the final real-index fixture also includes the type-reference pair) (leaf.ts←tax.ts←cart.ts, plus the tworenderdefinitions):explore+query)function cartTotal,file cart.ts,file leaf.ts,file tax.tscart.ts → tax.ts,tax.ts → leaf.tsObserved by codegraph explore for task: …Observed by CodeGraph for task: …sha256:3c427fa5…f11582sha256:594ad299…6eaefThe symbol set matches on this fixture, but the digest changes once because it now binds the structured file set instead of the markdown. When a query has few search hits, the relevant context adds files:
buildContext("basePrice", { maxSymbols: 4 })givesbasePrice,leaf.ts,tax.ts,cart.ts, which the new test pins.Impact, same fixture,
getImpactRadius("render", 1):codegraph.6c225064b482 → "render"). The CLI trail picked one definition.affectedPathswas[page.ts, page.ts, widget.ts, widget.ts].showPage → render()andshowWidget → Widget::render, with node ids.affectedPathsis[src/views/page.ts, src/views/widget.ts].Release note (0.15.0)
buildTaskContextreads CodeGraph in-process: files come fromfindRelevantContextand query nodes fromsearchNodes, with nocodegraph explore, so the code-context digest, the explorer's observed views and practice matches change once. Impact callers and callees are typed call edges with CodeGraph node ids, and an ambiguous name gives the union over its definitions.Tests (one case per branch)
codegraph-adapter.test.ts, a new real-index describe (indexed once byindexCodeGraphFixture):buildContextreturns the query node, then the relevant files in relevance order, with the exact[kind, name, path, range]list and the import edge. A second call returns a deep-equal result.findSymbolswithkinds: ["method"]gives onlyWidget.render, and with["function"]onlyrender().leaf.ts,tax.ts, and at depth 2 addscart.ts. The caller is thetaxedPrice → basePricecall edge. A node id resolves like its name, and callees are pinned too.rendergives the union of both callers and both files. The qualifiedWidget::rendernarrows it to the method.findRelevantContext, and SQLite refuses it (attempt to write a readonly database, asserted).FailureErrorAC_CODE_FACTS_UNAVAILABLE/index-open-failedwith no provider text.CodeGraph.open/openSyncrecords exactly[["openSync", { readOnly: true }]], so there is no retry and no writable open. The handle closed once.Session refusals, on the fake provider: no index, open fails, build lock and revision moves. Each case asserts its code, reason, retryable flag and the handles opened and closed.
Changed assertions, each justified
extracts import edges … scoped by changed paths: the reads moved from the fake CLI to an injected provider. The argv checks (-k import, last query has the changed path andimport, not the task, not the escapes) became the same checks on the lastsearchNodescall (kinds: ["import"], same query text checks). The edge and delta assertions are unchanged.extracts callers and callees from the CodeGraph node trailandresolves callers and callees to empty arrays when the CLI node trail has no recognizable calls. They pinned the deleted text-trail parser (duplicate collapse,+28 more, synthetic ids). Their behaviors are now covered on a real index by the impact, unknown and ambiguous cases above.refactor-baseline.test.ts: thenormalized-import-edgesfreeze now reads from an injected provider answering the same import dump, with scores in dump order so the search keeps the CLI's order. The frozen fixture and its digest are unchanged. The fake CLI loses the non-importqueryandexplorebranches, which nothing calls now.Test budget (
git diff --numstat origin/main)Previous verification on f14b6a8 (Bun 1.4.4, macOS arm64, Node v24.22.1)
bun run typecheck: cleanbun test packages/local-runtime/codegraph-adapter: 118 pass, 0 failbun test --timeout 60000 packages/surfaces/cli/test/code-facts-not-ready.test.ts packages/local-runtime/runtime-daemon: 871 pass, 0 failbun test packages/core/context-compiler packages/core/application: 31 passbun run test:codegraph-selector-node: verifiednode scripts/package-boundary-audit.mjsandnode scripts/production-mock-reachability-audit.mjs: passCurrent handoff and verification
Draft, not accepted for merge. The owner requested that the uncommitted round-2 work be saved to this PR for another machine. Current content is preserved unchanged from the S5 writer; commit
d59c8f83cead0f1dcf311d281b2b2ba1745e426b. See handoff and approved design amendment.On this content, Codex reran
bun run typecheck && bun test --timeout 60000 packages/local-runtime/codegraph-adapterusing Bun 1.4.4: 120 pass, 0 fail, exit 0.git diff --checkpassed. The worker also reported 902 pass/0 fail across 903 affected tests and a verified Node integration before stopping for provider credit exhaustion; those are prior pane evidence, not new-head CI receipts.The old f14b6a8 CI passed, but the GPT round-1 acceptance was FAIL and the Opus round-1 record was PASS with P3 findings. Neither old review nor old CI establishes acceptance of d59c8f8. Updated-head round-2 review and required CI remain pending. Review the changed assertions explicitly for tests bent to fit implementation; no new PASS is claimed here.
Changed assertions in the current addition:
Risk
affectedPathsstill covers them through the impact radius.buildContextawaitsfindRelevantContextwhile the session is open, and the handle is closed after the awaited read settles.Rollback
Revert this PR. There is no data, schema or ledger change.