Repository navigation
refactor(codegraph-adapter): remove snapshots (#574 S4) - #599
Ancienttwo wants to merge 3 commits into
Conversation
|
repo-harness acceptance claim |
Ancienttwo
left a comment
There was a problem hiding this comment.
repo-harness acceptance review
Round: 1
Subject: 735b9dd
Tier: high
Verdict: FAIL
Reviewer: pi (gpt-6-astra)
Model relation: different-family
FAIL: readiness and fact queries use two read-only sessions instead of the required single session, and the deleted provider-configuration test loses coverage of still-supported source enumeration/freshness behavior. Confirmed subject and green CI with gh pr checks 599. Reviewed all ten deletion-table rows, ADR-0058, joint decisions and the scoped diff. Snapshot files/exports and three failure reasons are retired; code-facts-read-not-ready remains; PR release note and CLI envelopes agree; JD-4 keeps ref-impact verbs separate; S2-owned regions are untouched. One targeted disposable-HOME probe confirmed two opens and two closes; no passing suite or typecheck was rerun.
NITs
None.
| case "open": { | ||
| // Readiness opens its own session inside this one. An unchanged revision after the read | ||
| // (design G4) proves that both sessions read one index state. | ||
| const facts = inspectRepositoryCodeFacts(root); |
There was a problem hiding this comment.
P2 GK599-1
withReadyCodeGraphIndex opens an outer session, then inspectRepositoryCodeFacts(root) opens and closes a second session in repository-code-facts.ts before read(opened.session.graph) queries the first handle. This contradicts the acceptance requirement that readiness and the read run in one read-only session, and the one-handle claim in the updated runbook. A targeted probe using the existing fakeReadProvider under a disposable HOME returned opensAtCallback=2, closesAtCallback=1, and final open=2/close=2. Thus cleanup succeeds on the probed success path, but readiness is not evaluated on the queried handle. The cited G4 test at read-session.test.ts:118 exercises withCodeGraphReadSession/inspectRepositoryCodeFacts/assertRepositoryCodeFactsReady, never this new helper; it does not pin the helper's post-query revision rejection or lifetime branches. Both flows --no-sync and ref impact share this helper.
Fix: Evaluate compatibility and freshness on opened.session and reuse the existing readiness mapping without opening another handle. Add focused coverage of this helper showing one open/close for a successful read, no query on failed readiness, code-facts-read-not-ready when revision changes during the query or a writer lock appears, and closure on readiness rejection and callback failure. Keep the shared fix for both callers.
| @@ -185,195 +184,6 @@ function builtinSelectorModel(sinkName: string): NativeModel { | |||
| // shared by the import-graph and exact-selector assertions: spawning it per assertion made the | |||
| // neighbouring adapter suite time out under parallel test execution. | |||
| describe("capability documentation projection inputs from the code index", () => { | |||
There was a problem hiding this comment.
P2 GK599-2
The removed 'private source mirror preserves provider includeIgnored, include and extension overrides' case covered more than mirroring: it built facts from an opted-in ignored nested repository and an included custom-extension source, asserted both names were readable, then added external-projects/embedded/new.ts and asserted the readiness gate refused the stale index. These source-enumeration/freshness behaviors remain supported and are explicitly promised by docs/runbooks/ref-impact.md:39-40. The deletion table calls this retired with the mirror and cites read-session.test.ts:141 and :268; the first uses fakeReadProvider and the second uses ordinary src/*.ts files with no provider configuration. Neither pins includeIgnored/include/extensions behavior, and the retained test/script search has no replacement configuration fixture. This drops coverage of behavior that still exists, contrary to the review acceptance contract.
Fix: Port the still-applicable configuration assertions to the ready-index read path using a real provider fixture: ready reads must expose the opted-in ignored and custom-extension declarations, and adding a source under the configured ignored population must make the read fail as code-facts-read-not-ready. Retire only mirror-specific and old filter-refusal assertions, update the deletion-to-retained coverage table, and keep S2-owned regions untouched.
|
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: 735b9dd
Tier: high
Verdict: FAIL
Reviewer: claude (claude-opus)
Model relation: same-family
The snapshot path is fully retired: no re-export, no fallback, no stale help, and the three reasons are removed consistently from contracts, both envelope fixtures and the PR-body 0.15.0 note, with code-facts-read-not-ready kept. JD-2, JD-3 and JD-4 are met. ready-index-read runs readiness and the read inside one read-only session, and the session closes in a finally. Two families block: (1) the new ref-impact runbook says core.fsmonitor is not executed, but ls-files and check-ignore (both Decision 3 commands) do run a configured fsmonitor hook, which I proved in a disposable HOME; (2) two coverage-table rows point at tests that do not pin the claimed behavior. withReadyCodeGraphIndex's moved-revision and build-in-progress branches are untested, and freshness over provider include, includeIgnored and extension settings lost its only test. CI on the subject is green.
NITs
- P3-retryable-split: ready-index-read.ts:30-34: when the inner inspectRepositoryCodeFacts detects a live build or a moved revision, it returns not-ready with index-incomplete. That is rethrown as code-facts-read-not-ready with the policy default retryable:false, because inspectRepositoryCodeFacts drops the tier's retryable flag. The same condition caught by the outer session (indexMoving) is retryable:true. Consider using the tier result's retryable.
- P3-runbook-reasons: docs/runbooks/ref-impact.md:43-45 lists the no-sync reasons as index-missing, index-open-failed or code-facts-read-not-ready. withReadyCodeGraphIndex also returns code-facts-unavailable when the provider cannot load.
- P3-duplicate-git-spy: The PR adds a shared test/git-spy.ts, but read-session.test.ts:292 keeps its own private gitSpy with the same mechanism. Consolidate in a later cleanup (S6).
| The read runs only the git commands ADR-0058 allows: `ls-files`, `rev-parse`, | ||
| `check-ignore` and `config --get core.excludesFile`, plus the `rev-parse`, | ||
| `ls-tree`, `cat-file` and `log` reads of the immutable refs. It never runs `git | ||
| status` or `git diff`, so configured clean/process filters, `core.fsmonitor` and |
There was a problem hiding this comment.
P2 F1-fsmonitor-claim
The runbook says the allowed git commands mean configured clean/process filters, core.fsmonitor and index hooks are not executed. That is false for fsmonitor. Probe in a disposable HOME (git 2.50.1, GIT_CONFIG_NOSYSTEM=1) with core.fsmonitor set to a marker script: git ls-files --cached --others --exclude-standard -z (the exact listGitVisibleFiles call, git-adapter/src/index.ts:63) ran the hook once, and git check-ignore ran it again. rev-parse did not. Git's index read (tweak_fsmonitor/add_fsmonitor) calls the hook when the index has no FSMN extension. Because ls-files never writes the index, this happens on every read. Sibling: the PR-body coverage row for the deleted 'runs no repository or configured hook' test says the Decision 3 commands trigger no hooks. That holds for post-index-change and hooksPath, but not for the fsmonitor hook. ADR-0058 Consequences accepts that fsmonitor repositories now get facts. It does not claim the hook stays unexecuted.
Fix: Correct the runbook sentence. Filters, status/diff and index-writing hooks do not run, but ls-files and check-ignore invoke a configured core.fsmonitor hook as ordinary git reads do. Also correct the coverage-table hook row. Suppressing fsmonitor with -c core.fsmonitor=false would be a product change outside ADR-0058 Decision 3. If wanted, take that to the orchestrator.
| } | ||
| if (facts.state === "not-ready") throw new FailureError("AC_CODE_FACTS_UNAVAILABLE", "code-facts-read-not-ready", { issues: facts.issues }); | ||
| const result = read(opened.session.graph); | ||
| if (!opened.session.unchangedSinceOpen()) throw indexMoving(); |
There was a problem hiding this comment.
P2 F2-coverage-table-gaps
Two rows in the deleted-assertion table name retained tests that do not pin the behavior. (a) The 'reads Git objects / code-facts-snapshot-changed' row cites read-session G4 for an index that moves during the read. G4 (read-session.test.ts:118) exercises session.unchangedSinceOpen() and inspectRepositoryCodeFacts/assertRepositoryCodeFactsReady. It never calls withReadyCodeGraphIndex. The only callers are the Node integration's ready and stale cases. So the post-read check at ready-index-read.ts:37, the build-in-progress branch at :25 (both mapping to code-facts-read-not-ready with retryable:true) and the non-FailureError -> code-facts-unavailable mapping have no test. Deleting line 37 fails nothing. withReadyCodeGraphIndex accepts no provider, so the fakeReadProvider revisionMoves fixture cannot reach it. (b) The 'private source mirror preserves provider includeIgnored, include and extension overrides' row is marked retired and covered by read-session freshness tests. The deleted test also asserted that a new source inside an includeIgnored directory (configured in codegraph.json) makes the read fail. That freshness behavior still exists (scanSourceFiles -> provider scanDirectory, read-session.ts:105/283). No remaining test sets codegraph.json include, includeIgnored or extensions (grep over adapter, daemon, CLI tests and the integration script finds none). 'an edit, a new source and a deletion are counted' uses only src/ under default settings.
Fix: Pass an optional { provider } through withReadyCodeGraphIndex to withCodeGraphReadSession and inspectRepositoryCodeFacts, as both already accept. Then add one read-session case that asserts revisionMoves gives code-facts-read-not-ready, retryable:true, and that open equals close. Extend the real-index freshness case, or the flows no-sync stale case, with a codegraph.json includeIgnored directory, and assert that a new file there makes the read not-ready.
|
repo-harness acceptance record
Model relation: same-family Findings:
|
Summary
删除 CodeGraph snapshot copy / child reader 路径,让 ref impact 和
flows check --no-sync通过 ADR-0058 read-only session 读取已就绪的 index,并删除三个已退役的 snapshot failure reasons。Refs #574。Tier: High。Draft,等待 review。Head
735b9ddeff45413901040ab1050e8ec463e72214,base6efd32ad1e2448e7f4e1dd9b69d6dcd23c5c9284。按 JD-1 本 PR 最后合并,合并前由 coordinator 安排 merge-only sync。Changes
snapshot.ts、snapshot-query.ts及index.ts的 snapshot re-exports。ready-index-read.ts:一个 read-only session 内检查 readiness 再读取,revision 在读取期间变化时按 not-ready 失败;ref-impact-facts 和 flow-selector-check 改走这里。failure.ts删除code-facts-snapshot-changed、code-facts-snapshot-limit-exceeded、code-facts-snapshot-layout-unsupported,contracts 与 CLI failure-envelope fixture 同步。flowshelp(main.ts的case "flows"块)改为描述 ADR-0058 read-only session,不再提 disposable private snapshots 和 unsupported-layout / over-limit snapshots。S2 的
packagedCodeGraphRuntimeDigestexport 与 S2 名下的capability-projection-inputs.test.ts区域(import-graph cases、seedWorkspace/fakeCli、combined import/selector、selector-fanout、evidenceFor 六个 built-in cases)本 PR 未改;冲突在本 PR 的 merge-only sync 中处理。Decisions resolved
见 joint decision:
test/real-index.ts保留私有codeGraphNodeRuntime()(+15/-4),不为测试保留 production export。flowshelp 在本 PR 修正(commit 735b9dd),只改case "flows"区域。test/git-spy.ts的 CodeGraph read allow-list 固定为ls-files、rev-parse、check-ignore、config --get core.excludesFile;ref impact 自己的ls-tree/cat-file/log只在 ref-impact 测试里以调用参数传入,不进入该 allow-list。flows 测试不传额外 verb。ref-impact 测试标题原写 "no config",已改成 "no ... config beyond core.excludesFile" 与断言一致;无断言改动。Deleted assertions → retained coverage
private snapshots refuse the literal filter driver %s(capability-projection-inputs)a configured clean filter still gets evidence…:marker 不出现 + git spy 无 filter 相关命令;flow-selector-checkno-sync … readygit spyprivate snapshots allow inert filter configuration and reject effective filters(capability-projection-inputs).git/index、不跑 filtera fresh index is ready; the read changes neither the database nor .git/index, and runs no git status or diffprivate snapshot reads real facts without changing original SQLite sidecars (live WAL)(capability-projection-inputs)readyIndexReads: closed-and-live-wal-preserved;flow-selector-checkno-sync preserves original checkout and index for *(ADR-0058 Decision 4 允许的 shm/空 WAL 除外)private snapshot reads Git objects through a read-only alternate…(capability-projection-inputs)code-facts-snapshot-changedreal CLI binds CodeGraph ranges to Git blobs without touching the source index覆盖;读取期间 index 变化由 read-session G4a revision that moves between reads…覆盖private snapshot runs no repository or configured hook…(capability-projection-inputs)private snapshot limit stops copying…(capability-projection-inputs)code-facts-snapshot-limit-exceededprivate source mirror preserves provider includeIgnored, include and extension overrides(capability-projection-inputs)freshness() is CodeGraph's change test与an edit, a new source and a deletion are counted覆盖private snapshot readiness mapping (C2):index-open-failedthrough the snapshot (read-session)index-open-failedunopenable状态;read-sessiona read-only open that throws is index-open-failed、a linked .codegraph is index-open-failedforeign期望code-facts-snapshot-layout-unsupported.codegraph失败关闭index-open-failedsnapshotLimit字段(contracts、CLI)Verification (head 735b9dd)
本机运行(load average 约 96,后降到 27):
bun run typecheck:clean。node scripts/codegraph-selector-node-integration.mjs:status: verified。node scripts/package-boundary-audit.mjs:passed。bun test packages/local-runtime/runtime-daemon/test/flow-selector-check.test.ts packages/surfaces/cli/test/ref-impact.test.ts packages/local-runtime/codegraph-adapter packages/contracts/test/failure-envelope.test.ts packages/surfaces/cli/test/failure-envelope.test.ts:第一次 159 pass / 4 fail,第二次失败的是另一组测试;全部是超时(spawnSync ETIMEDOUT、测试超时 5s/60s 后杀掉子进程),没有 assertion failure,归类为本机负载问题。以本 head 的 CI 结果为准。Test changes and budget
git diff --numstat 6efd32ad...HEAD:tests 127 added / 224 deleted,product 116 added / 595 deleted(src + scripts),ratio 1.09:1。docs 22 added / 29 deleted 另计。新增 test 文件 1:packages/local-runtime/codegraph-adapter/test/git-spy.ts。Release note (0.15.0)
删除 code-facts-snapshot-changed、code-facts-snapshot-limit-exceeded、code-facts-snapshot-layout-unsupported 三个 failure reasons,属于已批准的 contracts breaking change;code-facts-read-not-ready 保留。
Risk / Rollback
不再提供 snapshot isolation:读取直接打开仓库自身 index(只读),依赖 ADR-0058 Decision 3/4 的 git 命令与 sidecar 约束,由上面的 git spy 和 sidecar 断言覆盖。Rollback: revert 本 PR;无 ledger/schema/data migration。