test(desktop): isolate login-shell probe measurements - #7570
Conversation
Run the counter-owning discovery tests in bounded, exact-filtered libtest processes. Keep process-wide counting so auth workers remain covered, require a completed assertion body, and reject unisolated resets. Add parent-isolation and own-worker controls without changing production discovery. Signed-off-by: Pinky <5f5ab050ec58ae208332edd544ebf705221e24c1b86d82a6ca07038a7a8f6ac9@buzz.block.builderlab.xyz>
🔐 Codex Security Review
|
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Verdict
No actionable code blockers found in this test-isolation change. This is a review comment, not an approval.
Reviewed head 07f8e86c58a0aa5378537a3daa4610361fdbad55 against merge base 00209076c7a10d9e4a475466c313e8ebecf041f5; current PR base was 092c6a7277698bd373ccbc1d008fc1507094ae74. The intervening base changes do not touch desktop/src-tauri.
- Scope and measurements: all three changed files are test-only. Both original assertion bodies, the cold/repeated zero checks, and the forced positive control are preserved. The counter remains process-wide and attached to the real login-shell invocation path, including joined auth/worker threads.
- Subprocess boundary: exact libtest selection, child success, and a receipt emitted after the body prevent empty-selection and failed-child passes. The separate post-closure worker assertion detects omission of the closure invocation. Parent and child locks are process-local; the existing subprocess helper bounds runtime and output.
- Validation: source/call-path review and
git diff --checkpassed. On macOS, the existing default/system-keyring test artifact ran both isolated discovery tests, both new controls, and the missing-binary probe together with--exact --nocapture --test-threads=8: 5 passed, 0 failed. Checkout head, clean state, and binary SHA-256 were checked before/after. This was artifact-level validation, not a fresh build or an independent rerun of the author’s full CI/mutation campaign.
Runtime artifact: buzz_lib-827fc16670abf059, SHA-256 eea65f2beadbdcd6ec9c783e79f18896ffdc729d07dd99414892b3e050f57fe1.
Residual limits are accurately scoped in the PR: this isolates process state, not the filesystem, and does not strengthen the existing Unix ownership boundary for nested probe process groups. Those limitations do not warrant expanding this test-only repair. GitHub CI was still running at the review snapshot; no merge or deployment readiness is implied.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE
Reviewed: 092c6a7277698bd373ccbc1d008fc1507094ae74..07f8e86c58a0aa5378537a3daa4610361fdbad55 (exact head 07f8e86c58a0aa5378537a3daa4610361fdbad55)
Risk: medium — this is test-only code, but it controls whether process-global discovery measurements produce trustworthy CI results under parallel execution, bounded subprocesses, and failure paths.
Behavior/contracts traced: parent/worker libtest selection; marker and current-thread binding; recursion exclusion; reset isolation; completion receipt ordering; child exit/panic/early-return behavior; timeout/output/process-group bounds; real production probe instrumentation; full-suite interaction; platform gating; production-scope containment.
Findings: no unresolved author-actionable defect. The exact PR range changes only three Tauri test files, and the counter module remains #[cfg(test)]; production discovery/cache behavior is unchanged. The worker uses exact test selection, binds child recognition to both marker and current libtest name, executes the assertion closure before emitting its receipt, and requires successful child exit plus that receipt. Zero selection, rename/ignore, panic, nonzero exit, timeout, overflow, or skipped body therefore fail closed. The positive control still exercises the real production run_in_login_shell seam.
Author action: none.
Verification owner: CI owns the remaining exact-head Desktop Core and Smoke shard 4 completion. Reviewer/tooling owns any desired independent Windows runtime observation; the new isolation path is Unix-gated.
Validation at matching clean head:
- Both lanes independently passed the canonical full
just desktop-tauri-test: main library 3,175 passed, 19 ignored, with all workspace/integration/doc targets green. - Focused discovery/bounded-process slice passed 131/131.
- Causal mutations removing the assertion-closure invocation and removing the real production
record()seam each failed the intended worker control, then restoration returned to a clean exact head. cargo fmt --checkandgit diff --checkpassed.- Exact-head required checks had no failure at final refresh. Windows Rust and three smoke shards had completed green; Desktop Core and Smoke shard 4 remained pending.
Manual/native evidence: not applicable to a test-only subprocess-isolation change. No UI or production-auth behavior changed or was claimed.
Residual risk: the inherited marker is not cryptographically authenticated, though it must equal the exact current test name and no other repository/CI setter was found in the searched scope. The existing bounded-command helper cannot guarantee ownership of nested Unix processes that deliberately escape their process group; its return remains bounded and this PR does not alter that contract. Pending CI is a named gate confidence gap, not author rework.
jedwards27
left a comment
There was a problem hiding this comment.
Reviewed exact head 07f8e86c58a0aa5378537a3daa4610361fdbad55 against base 092c6a7277698bd373ccbc1d008fc1507094ae74.
No material defects found. The change is confined to three Tauri test files, with the probe module remaining #[cfg(test)]; production discovery/cache behavior is unchanged. The worker contract fails closed: it selects the exact current libtest name, recognizes child mode only when marker and current test name agree, runs single-threaded, and requires both successful process exit and a receipt emitted only after the assertion closure returns. Zero selection, panic, early failure, recursion, timeout, or bounded-output failure therefore cannot produce a pass.
The positive control reaches the real run_in_login_shell instrumentation seam, while the protected tests retain cold/repeated zero-probe assertions and the forced-path positive assertion. Existing bounded-command handling provides a 300-second deadline, aggregate 1 MiB output cap, child reaping, and Unix process-group cleanup. The inherited nested process-group escape limitation is unchanged and does not undermine bounded parent return.
Verification on the exact clean head:
- Two independent
just desktop-tauri-testruns passed; the main lib target reported 3,175 passed, 0 failed, 19 ignored, with all package targets/doc tests green. - A focused discovery slice passed 131/131.
- Two complementary mutations proved the controls are causal: skipping the child assertion body and removing the production probe record each made the intended control fail; both trees were restored clean.
git diff --checkand Tauri formatting passed.- Exact-head GitHub CI completed with no pending or failed checks, including Desktop Core, Windows Rust/build, macOS build, smoke shards, and integration gates.
Residual confidence gap: no reviewer independently exercised native Windows runtime behavior; the added isolation path is Unix-gated, while exact-head Windows CI is green. No author action requested.
* origin/main: Configure ACP session scope per agent (#7578) refactor(buzz-acp): point agents at buzz --help instead of a command table (#7586) feat(buzz-cli): render an agent-friendly command tree in --help (#7584) fix(avatars): scale agent squircles from normalized paths (#7307) fix(mobile): bind same-name mentions to exact selected identities (#7385) fix(desktop): isolate quota backoff and reuse channel discovery rosters (#6998) test(desktop): isolate login-shell probe measurements (#7570) feat(git): add default-branch management to relay and CLI (#7562) fix(acp): integrate the Buzz Pi adapter fork (#7552) fix(markdown): align mention chip wrapping (#7501) fix(relay): reject presence updates when Redis storage fails (#7532) fix(desktop): let inbox title and message author names truncate under narrow panes (#7550) fix(buzz-acp): report missing models without retrying (#7538) fix(desktop): require a Codex adapter with Astra support (#7427) fix(desktop): order unnamed roster members by full canonical npub (#7503) fix(mobile): standardize public-key identity display on npub (#7493) fix(desktop): npub identity controls across profile, agents, and workflows (#7489) fix(desktop): npub identity displays for mention, member, and workflow surfaces (#7495) fix(desktop): shared npub identity foundation (canonicalNpub, PubKey gate, strict parser) (#7488) fix(mobile): render push notification sender identity as npub (#7494) Signed-off-by: Sol <478bb5a31222ea2b28a3d1afb8b1d598940628f19c2a87efc3c4b822299eeec6@buzz.block.builderlab.xyz> # Conflicts: # desktop/src-tauri/src/commands/media_download.rs
Pinky, acting on Wes’s behalf.
Summary
Fix a pre-existing false-failure mechanism in the login-shell discovery tests, independently of the thread-window work.
Three test-only files, +103/-4. Base
00209076c7a10d9e4a475466c313e8ebecf041f5; head07f8e86c58a0aa5378537a3daa4610361fdbad55.Reproduction and causal checks
On the main-based source, cheap discovery alone counted 0. Running the existing missing-binary version test on another thread inside the measurement interval made the repeated-cheap assertion fail with 1. This reproduces interference, not the exact scheduling/caller behind the original full-suite failure. The counter measures
run_in_login_shellinvocations, not exact OS process counts.Mutation checks:
record()call fails the original forced positive control (0).Independent read-only review by Brain found no remaining blocking finding at the final head.
Validation at the final head
just ci: passed. Desktop Tauri: 3,175 passed, 19 ignored; desktop JS: 6,488 passed; mobile: 2,098 passed. Remaining Rust unit, formatting/lint/static, and desktop/web build lanes passed. An initial five-minute tool timeout interrupted compilation; the subsequent complete run used a longer deadline, not altered gates.TESTING.md: passed. Built releasebuzz-relay,buzz, andbuzz-adminfrom this head. Fresh isolated Postgres/Redis, additive migrations, dedicated loopback ports, disposable NIP-98 identity; health, create channel, signed root/reply send, exact-ID get, and thread retrieval passed. Relay and both owned containers stopped afterward. Dev auth-token mode follows the guide; not a production-auth or thread-window/native UI acceptance claim.Scope and limits
This is process/cache isolation, not a filesystem sandbox. The existing bounded-command helper bounds child runtime/output; its Unix process-group behavior does not guarantee ownership of nested probe groups. Existing individual probe deadlines remain unchanged.
No UI change, screenshots, thread-window commits, feature enablement, merge, or deployment included.
Originating conversation: buzz://message?channel=0b73400d-6922-4fe3-9b97-454da04d011b&id=984528c883faa8829971d19394870f0f2d543d5c1950565fd55ffddc1b8d8022