Repository navigation
test(cli): cover daemon and local-read contracts - #4394
ScriptedAlchemy wants to merge 9 commits into
Conversation
|
There was a problem hiding this comment.
🔍 Devin Review: 2 flags
Not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
Observe the daemon and CLI proxy lifetimes under scoped child ownership, drain and join the listener, and cover listener cleanup during unwinding. Assert no storage creation for cold init and reads with accounting enabled, then verify stopped-daemon reads against the same private endpoint. Clarify optional service installation and foreground startup terminals. Refs #4344
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Pushed the reviewed follow-up in 2c2bd60 (refs #4344; builds on merged #4348).
The original proxy was installed after daemon startup, so daemon-side pricing requests could escape the network assertion. The observer now configures the daemon, init, and each CLI child before startup; owns a scoped listener; reaps children before stop; drains pending accepts; and joins before checking the count. A real loopback regression covers counting and cleanup on panic.
Cold init and read checks now pin the fixture endpoint, use canonical child ownership and short Unix homes, enable normal global accounting, and verify absence of the canonical identity marker, profile database, project shards, daemon authority, and repository-local store. The stopped-enrolled read case remains covered, with an endpoint probe after each command. README and user-guide instructions now consistently describe optional service installation and foreground startup in another terminal.
Validation: pinned rustfmt, git diff --check, and commitlint passed; independent source review completed. No local native run was claimed for these revised tests. Bazel CI on this commit must establish compilation and behavior. The PR body now explicitly distinguishes the original implementation's direct-artifact evidence from this follow-up.
Use the canonical asynchronous SQL reader for persisted scope lookup and CAS admission while preserving the foreground priority and read budget. Exercise real native-task CAS, readback, and stale-revision conflict requests and assert that scope-set reads do not park native workers.
Part of #4344; #4348 covers README.md and SECURITY.md.
Root cause
The user guide still described persistent service installation as the normal daemon prerequisite and retained wording that could be read as query-triggered freshness work, even after the shipped contract made the running daemon mandatory and service installation optional. The production behavior also lacked focused journey tests proving the typed no-daemon refusal/no-write boundary and that bundled pricing/native usage reads stay network-free.
Change
docs/USER-GUIDE.mdwith the merged docs(security): clarify daemon authority and outbound operations #4348 contract: a running daemon is required,daemon install-serviceis optional,daemon runis the foreground alternative, and MCP/read paths do not perform an implicit refresh. The existing Privacy and Network section remains the authority for GitHub, semantic/configured-remote effects and local-only bundled pricing/native usage reads.code_index_scheduler_unavailablerefusal before any init/index write;cost,gain, andcurrent-countercompleting with zero connections to a controlled loopback HTTP proxy.Existing versus added coverage
Existing
init_accepts_relative_current_directoryandinit_wait_holds_until_first_generation_is_readyalready initialize through the shared fixture that executestracedecay daemon run; no duplicate foreground-daemon test was added. Existing status/tool tests covered individual no-daemon states, while the new combined read test adds the explicit no-start/no-store assertion. The no-daemon init/no-write and controlled-listener network assertions are new.Review follow-up
Falsifiability evidence for the original implementation
Before commit, the prior owner temporarily broke each guarded production behavior in one local build, ran the compiled Bazel test artifact, then reverted the breaks:
daemon_connect_down; the typed-refusal test failed;0instead of daemon-unreachable69; the status/query test failed;ureqpricing request produced1accepted proxy connection; the local-only test failed.After reverting, each exact test reported
1 passed; 0 failed. These direct artifact runs are retained only as falsifiability evidence for the original implementation, not as final Bazel test evidence. They were not rerun for the revised observer, cold-read assertions, or cleanup regression in the review follow-up.Original-head validation
Normal configuration, with no timeout increase:
Blocked before test execution: crate-universe
cargo-bazel splicetimed out (exit 48, elapsed722.961s; no libtest count). After the final rebase, a repeat again reached no test and the Bazel server terminated withSocket closed(exit 37).Blocked before compilation by the same cached crate-universe splice timeout (
exit 48). No budget was raised and no direct-binary run is claimed as final validation.Validation of the review follow-up
Follow-up commit:
2c2bd60ce79300f20b1735529986ff9b2ad3c568.The revised three-file change passes the pinned Rust formatter,
git diff --check, and commitlint. The final source and test changes received independent review. No local native build, direct-binary test run, timeout increase, or budget increase was performed for this follow-up. Bazel CI on the published follow-up head must establish compilation and native behavioral results; the original implementation's direct-artifact evidence above does not validate the revised tests.CI and shutdown follow-up
The follow-up commits address failures exposed by the PR test runs:
The shutdown repairs are in bf193ab2, 7fa37f88, and 99810d70. Regressions cover exact-root retirement isolation, real MCP settlement during harness shutdown, and a real authenticated socket whose client is aborted and joined while its reservation writer is paused.
Validation: pinned Rust formatting, whitespace checks, commitlint, and independent source/API/lifetime review passed. Native regression execution is pending Bazel CI on the published head. No assertion, deadline, admission budget, or CI gate was weakened. The confirmed ownership defects do not by themselves establish the cause of every earlier suite timeout.