Skip to content

feat(codegraph-adapter): SDK-backed provider reads (#574 S5) - #597

Merged
Ancienttwo merged 2 commits into
mainfrom
feat/574-s5-sdk-provider
Nov 14, 2026
Merged

Ancienttwo merged 2 commits into
mainfrom
feat/574-s5-sdk-provider

Conversation

@Ancienttwo

@Ancienttwo Ancienttwo commented Nov 13, 2026 •

Copy link
Copy Markdown
Owner

Summary

Refs #574

Tier: High (GPT plus Opus review). S5 of ADR-0058: SDK-backed provider reads. Base: main 6efd32a (S1 #589, S1b #596).

Goal

CodeGraphCliProvider uses the CodeGraph CLI only to build the index (sync under codeGraphChildEnv). buildContext, findSymbols and getImpactRadius read 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.ts only)

  • Split. A private CodeGraphCliIndexBuilder keeps sync. The provider composes it with SDK reads. runCodeGraphCli delegates to one shared runCodeGraphInvocation and keeps its signature (S2 still uses it).
  • Deleted: every CLI read (explore, query, node, impact), parseExploreSymbols, parseNodeTrail, trailEntries, normalizeCliNode, syntheticSymbolId and the findSymbols fallback inside buildContext. There is no CLI path, no snapshot path and no mode branch.
  • buildContext (C6). The file set comes from findRelevantContext(query): entry-point files first, then node order, capped at maxSymbols. The query nodes come from searchNodes. 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 scoped import <paths> search with kinds: ["import"], resolved by importEdgesFromQueryNodes. The digest and evidence bind the relevant-file list where they used to bind the explore markdown. The evidence summary is now Observed 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.
  • Impact. symbolId resolves 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. affectedPaths lists the unique, sorted files of getImpactRadius(id, depth) for each definition. callers and callees are the direct calls/instantiates edges of the definitions, with CodeGraph node ids on both ends.
  • Failures. Each read opens one session, and nothing is retried. Provider-load exceptions map to AC_CODE_FACTS_UNAVAILABLE / code-facts-unavailable, matching readiness. A close failure preserves an existing mapped read error; after a successful read it becomes index-open-failed. A throw while the index moved is retryable.
    • index-missing → AC_CODE_FACTS_UNAVAILABLE/index-missing
    • open failure → AC_CODE_FACTS_UNAVAILABLE/index-open-failed
    • live build lock → AC_CODE_FACTS_NOT_READY/code-facts-not-ready (index-incomplete, retryable)
    • Any throw inside an SDK call, including a fault-injected write that the read-only handle refuses, is a read failure: index-open-failed, with no provider text in it. It is never retried, and never with a writable open.
    • An index that moved during the read (unchangedSinceOpen() is false, design G4) is refused like a build in progress.

index.ts, read-session.ts, real-index.ts, read-provider-fixture.ts and failure.ts are untouched. The class keeps the name CodeGraphCliProvider and its constructor (root, binary, readProvider?). A rename would touch index.ts (S4) and runtime-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 two render definitions):

before (main, CLI explore + query) after (SDK)
symbols function cartTotal, file cart.ts, file leaf.ts, file tax.ts same four, same ids
edges cart.ts → tax.ts, tax.ts → leaf.ts same
evidence summary Observed by codegraph explore for task: … Observed by CodeGraph for task: …
digest sha256:3c427fa5…f11582 sha256:594ad299…6eaef

The 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 }) gives basePrice, leaf.ts, tax.ts, cart.ts, which the new test pins.

Impact, same fixture, getImpactRadius("render", 1):

  • before: one caller with a synthetic id (codegraph.6c225064b482 → "render"). The CLI trail picked one definition. affectedPaths was [page.ts, page.ts, widget.ts, widget.ts].
  • after: two callers, showPage → render() and showWidget → Widget::render, with node ids. affectedPaths is [src/views/page.ts, src/views/widget.ts].

Release note (0.15.0)

  • buildTaskContext reads CodeGraph in-process: files come from findRelevantContext and query nodes from searchNodes, with no codegraph 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 by indexCodeGraphFixture):

  • buildContext returns 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.
  • findSymbols with kinds: ["method"] gives only Widget.render, and with ["function"] only render().
  • impact at depth 1 gives leaf.ts, tax.ts, and at depth 2 adds cart.ts. The caller is the taxedPrice → basePrice call edge. A node id resolves like its name, and callees are pinned too.
  • an unknown symbol gives an empty impact.
  • an ambiguous render gives the union of both callers and both files. The qualified Widget::render narrows it to the method.
  • read-only refusal:
    • The wrapped real handle makes a real write through the read-only database inside findRelevantContext, and SQLite refuses it (attempt to write a readonly database, asserted).
    • The result is a FailureError AC_CODE_FACTS_UNAVAILABLE/index-open-failed with no provider text.
    • A spy on the SDK's CodeGraph.open/openSync records 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 and import, not the task, not the escapes) became the same checks on the last searchNodes call (kinds: ["import"], same query text checks). The edge and delta assertions are unchanged.
  • Removed extracts callers and callees from the CodeGraph node trail and resolves 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: the normalized-import-edges freeze 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-import query and explore branches, which nothing calls now.

Test budget (git diff --numstat origin/main)

187 added product lines; 248 added test lines; ratio 1.33:1; 0 added test files.
Measured against 6efd32ad...HEAD; product roots: packages/**/src + scripts; test roots: packages/**/test.

Previous verification on f14b6a8 (Bun 1.4.4, macOS arm64, Node v24.22.1)

  • bun run typecheck: clean
  • bun test packages/local-runtime/codegraph-adapter: 118 pass, 0 fail
  • bun test --timeout 60000 packages/surfaces/cli/test/code-facts-not-ready.test.ts packages/local-runtime/runtime-daemon: 871 pass, 0 fail
  • bun test packages/core/context-compiler packages/core/application: 31 pass
  • bun run test:codegraph-selector-node: verified
  • node scripts/package-boundary-audit.mjs and node scripts/production-mock-reachability-audit.mjs: pass

Current 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-adapter using Bun 1.4.4: 120 pass, 0 fail, exit 0. git diff --check passed. 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:

  • Extend the existing session-refusal table with read-throws-while-index-moves and read-plus-close failure; retain code/reason/retryability and handle-count assertions.
  • Add a provider-load mapping test that requires the readiness failure envelope and no provider text.
  • Add a real-index type-reference case: affectedPaths includes the reference, callers/callees remain empty.
  • Replace ambiguous-name length/containment assertions with an exact ordered callers array; add the unfiltered findSymbols tie-break assertion. These strengthen order coverage.
  • Correct the fault-injection comment: pinned CodeGraph 1.6.2 findRelevantContext makes no heal write; the test deliberately injects the refused write.
  • The four S2 import-graph baseline cases remain untouched by this addition. Impact port removal stays in S6.

Risk

  • C6 changes the context output once (above). It is not on a timed path.
  • Callers and callees no longer count type references as calls, because the old text trail mixed them in. affectedPaths still covers them through the impact radius.
  • buildContext awaits findRelevantContext while 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.

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
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-14T03:05:55.541160Z d59c8f8 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@Ancienttwo

Copy link
Copy Markdown
Owner Author

repo-harness acceptance claim
Role: acceptor
Round: 1
Subject: f14b6a8
Tier: high
Agent: codex (gpt-6.1-terra), read-only

@Ancienttwo Ancienttwo left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Ancienttwo

Copy link
Copy Markdown
Owner Author

repo-harness acceptance record
Claim: #597 (comment)

Round Role Subject Verdict
1 codex (gpt-6.1-terra), read-only f14b6a8d2c44005e80f4a0e5d419c06c11a6555b FAIL: 1 P1, 1 P2

Model relation: different-family

Findings:

  • P1 F1 at packages/local-runtime/codegraph-adapter/src/cli-provider.ts:57: Callers and callees use calls and instantiates edges only, excluding references; the design summary specifies typed calls/references edges and names C6 as the only output change. No test shows a type reference stays in affectedPaths. -> Implement the agreed calls/references edge semantics, or record a decision; add a real-index type-reference case that checks callers/callees and affectedPaths.
  • P2 F2 at packages/local-runtime/codegraph-adapter/src/cli-provider.ts:158: openCodeGraphReadSession is called outside the error-mapping block and can throw CodeGraphProviderUnavailableError, which then escapes without AC_CODE_FACTS_UNAVAILABLE; a throw from session.close() at :175 can mask the mapped read failure. -> Map provider-load and close failures at the read boundary without retrying or leaking provider text, and cover the failure contract.

@Ancienttwo

Copy link
Copy Markdown
Owner Author

repo-harness acceptance claim
Role: acceptor
Round: 1
Subject: f14b6a8
Tier: high
Agent: pi (claude-opus-5-5), read-only

@Ancienttwo Ancienttwo left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Ancienttwo

Copy link
Copy Markdown
Owner Author

repo-harness acceptance record
Claim: #597 (comment)

Round Role Subject Verdict
1 pi (claude-opus-5-5), read-only f14b6a8d2c44005e80f4a0e5d419c06c11a6555b PASS: 5 P3

Model relation: same-family

Findings:

  • P3 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 codegraph-adapter: read the index only through the in-process read-only handle #574.
  • P3 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.
  • P3 F3: cli-provider.ts:158 openCodeGraphReadSession sits outside the try, so a raw CodeGraphProviderUnavailableError escapes where readiness maps it to codegraph-unavailable.
  • P3 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).
  • P3 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.

Preserve the S5 round-2 worker fixes for cross-machine continuation. Updated-head review remains pending.
@Ancienttwo

Copy link
Copy Markdown
Owner Author

repo-harness acceptance claim
Role: acceptor
Round: 2
Subject: d59c8f8
Tier: high
Agent: pi (gpt-6-astra), read-only

@Ancienttwo Ancienttwo left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Ancienttwo

Copy link
Copy Markdown
Owner Author

repo-harness acceptance record
Claim: #597 (comment)

Round Role Subject Verdict
2 pi (gpt-6-astra), read-only d59c8f83cead0f1dcf311d281b2b2ba1745e426b PASS: 1 P3

Model relation: different-family

Findings:

  • P3 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.

@Ancienttwo

Copy link
Copy Markdown
Owner Author

repo-harness acceptance claim
Role: acceptor
Round: 2
Subject: d59c8f8
Tier: high
Agent: claude (claude-opus), read-only

@Ancienttwo Ancienttwo left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Ancienttwo

Copy link
Copy Markdown
Owner Author

repo-harness acceptance record
Claim: #597 (comment)

Round Role Subject Verdict
2 claude (claude-opus), read-only d59c8f83cead0f1dcf311d281b2b2ba1745e426b PASS: 2 P3

Model relation: same-family

Findings:

  • P3 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.
  • P3 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.

@Ancienttwo

Copy link
Copy Markdown
Owner Author

Joint decision (orchestrator + advisor), 2026-11-14, on closing the round-1 threads of #597 after the machine switch.

  • Acceptance. Head d59c8f83 has two round-2 PASS rows from distinct claims: GPT (different-family, 597#issuecomment-6870578770) and Opus (597#issuecomment-6870590484). There is no FAIL row on this head. Required / CI is green (35/35).
  • Round-1 threads F1 and F2. The round-1 acceptor seat and the S5 head-owner seat were lost with the old machine (574#issuecomment-6870096533). The live round-2 GPT acceptor (t382) confirmed both fixes on d59c8f83. It wrote in its pane: "F1 resolved: The approved amendment specifies calls/instantiates edges. The new real-index test confirms type references remain in affectedPaths with empty callers/callees." and "F2 resolved: Provider-load failures receive the expected sanitized error; close failures preserve existing read errors; reads spanning an index change fail as retryable."
    The gatekeeper brief allows it one GitHub write, review publish. So the coordinator replies fixed d59c8f83 on each thread, quotes that confirmation, and resolves both threads. This applies to this PR only.
  • The three P3s go to S6, not S5: GPT N1, the error-boundary coverage siblings with close-attempt counting; Opus F1, one shared codegraph-unavailable FailureError helper for cli-provider.ts and repository-code-facts.ts; Opus F2, a close-throws-after-good-read row. Opus F2 and GPT N1 overlap.
  • Review bot. Marking the PR ready may trigger the Codex bot again on d59c8f83. Per acceptor.md §2.3 the merge does not wait for it. Anything it posts after the merge is triaged once.

Then: mark ready, merge with repo-harness publication merge (JD-1, S5 first).

Seat: orchestrator t366

@Ancienttwo

Copy link
Copy Markdown
Owner Author

Review-bot reference: no findings at f14b6a8: no action

Seat: orchestrator t366

@Ancienttwo
Ancienttwo marked this pull request as ready for review November 14, 2026 03:04
@Ancienttwo
Ancienttwo merged commit d63dfad into main Nov 14, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant