[review] e2e harness hygiene (secrets in artifacts, vacuous baseline, task cleanup on cancel) #72

New issue

Closed

opened 2026-07-07 13:52:35 +00:00 by robocub · 1 comment

robocub commented

2026-07-07 13:52:35 +00:00

Collaborator

#38 Tier-3 harness follow-ups: the generated config.toml (with appservice + puppet tokens) is written into the collectable artifacts tree (F6-2); await_baseline/voice_members_among pass vacuously when the guild is absent from cache (F6-3); and the detached serenity client tasks in the actors/probes aren't aborted on scenario cancellation (F6-4).

F6-2 — P2 / LOCAL / CONFIRMED

Location: crates/e2e/src/bridge_proc.rs:88 @ 968da57

Defect: BridgeUnderTest::start writes the generated config.toml — containing the plaintext appservice_token and every puppet bot token — into the scenario's artifacts tree (<artifacts>/<scenario>/bridge/config.toml), violating the harness's own rule (config.rs doc: secrets are 'never echoed into artifacts') and the repo-wide never-commit-secrets convention.

Failure scenario: A scenario runs (smoke.rs passes ctx.artifacts.join("bridge") as workdir); the operator or a future CI job collects/uploads the e2e-artifacts directory to share the report.json/bridge.log — the live Discord bot tokens and Matrix appservice token ship with it, granting anyone with the artifact full control of the puppet pool and appservice.

Verifier: Re-derived from code: generate_config (crates/e2e/src/bridge_proc.rs:29-46) embeds the plaintext appservice_token (line 39) and every puppet token (41-43); start() writes it to workdir/config.toml (87-88); smoke.rs:45 passes ctx.artifacts.join("bridge") as workdir, where ctx.artifacts = <artifacts_dir>/ (scenario.rs:78) — the same tree holding bridge.log and report.json. No code anywhere in the crate deletes or scrubs it after the run. This directly contradicts the crate's own documented invariant (config.rs:9-10: secrets are "never logged, never echoed into artifacts"), and the exposure vector is concrete: the testing design doc (docs/nether-voicebridge-testing-design.md D3, lines 392-398) plans a nightly CI workflow that will "always upload artifacts" with secrets supplied via Forgejo Actions secrets — uploading as written ships the tokens those secrets protect. No #[cfg(test)] test pins the artifacts location as intended (the existing test writes to temp_dir). The .gitignore (lines 13-16) acknowledges "generated configs (which embed tokens too)" in artifacts, but that only covers the git-commit vector and contradicts rather than supersedes config.rs's rule. Minor overstatement in the claim: the tokens are the dedicated harness puppet bots and test appservice as_token (config.rs:74-77 forbids prod tokens), not the prod pool — but a leaked as_token still grants namespace-wide masquerade on the live homeserver, so P2 stands.

Regression test sketch: Not required (P2), but if added: in bridge_proc.rs mod tests, a test that calls generate_config-based start-path plumbing indirectly is heavy (spawns a child); the lean regression is a unit test asserting the post-condition once fixed — e.g. after refactoring start() to place config.toml outside workdir (or delete it after spawn), assert that no file under a simulated artifacts workdir contains cfg.appservice_token or any puppet token after BridgeUnderTest teardown, using a stub bridge_bin (e.g. /bin/cat) so no real bridge is needed.

F6-3 — P2 / LIVE / CONFIRMED

Location: crates/e2e/src/observers.rs:221 @ 968da57

Defect: ConnCountProbe::voice_members_among returns an empty Vec when the guild is absent from the gateway cache, so await_baseline passes vacuously; connect() only sleeps a fixed 2 s after READY hoping GUILD_CREATE has arrived, with no verification that the guild is actually cached.

Failure scenario: On a slow gateway (or if the probe bot's GUILD_CREATE for the test guild arrives >2 s after READY), await_baseline's first poll hits the cache-miss branch, returns Ok immediately, and the scenario reports the 'no bridge bot lingers in voice' postcondition green even while puppet bots are still connected to the voice channel — a false PASS in the exact check the probe exists for.

Verifier: Fail-open on cache miss is real and reachable: connect() (observers.rs:207-209) only awaits READY then sleeps a fixed 2 s — despite its own doc comment (line 190-191) promising it resolves once "the guild's voice states are cached" — and serenity only caches a guild on its GUILD_CREATE dispatch, which follows READY and can lag >2 s. voice_members_among (lines 221-223) maps the resulting cache miss to an empty Vec, and await_baseline (lines 236-238) returns Ok(()) on the very first empty poll and never re-checks after success, so a late GUILD_CREATE cannot correct it. At the sole call site (scenarios/smoke.rs:133-140) this probe is the only Discord-side 'no bridge bot lingers in voice' postcondition, so the vacuous Ok is a false PASS of exactly the invariant the probe exists for (CLAUDE.md constraint #6: teardown returns connection count to baseline). No #[cfg(test)] test pins the empty-on-miss behavior as intended (tests cover only sender_class/token-decode/urlencode) and no doc declares it deliberate. Only the race frequency is live-dependent; the fail-open logic is deterministic from code, so CONFIRMED rather than UNCERTAIN_LIVE. P2: harness false-negative, not product code.

Regression test sketch: Not required (P2). If desired: in observers.rs tests, refactor voice_members_among to take Option<&Guild>-like input or make it return Result; assert that a None/absent guild yields an error (or a distinct 'cache not primed' state) instead of an empty Vec, and that await_baseline does not accept empty until the guild has been observed in cache at least once.

F6-4 — P2 / LIVE / CONFIRMED

Location: crates/e2e/src/actors/discord.rs:91 @ 968da57

Defect: DiscordSpeaker (and ConnCountProbe in observers.rs) spawn a detached serenity client task with no Drop/abort-on-cancel handling; shutdown() is only reachable by explicit call, so cancelling the scenario future leaks a live gateway session — including the speaker's active voice-channel membership.

Failure scenario: A scenario exceeds Profile::scenario_timeout(), so run_scenarios' tokio::time::timeout drops the scenario future mid-await (scenario.rs:84): speaker.shutdown() never runs, the spawned client.start() task keeps the speaker bot joined to the test voice channel for the rest of the suite run — every subsequent scenario sees a phantom 'human' in the channel, the bridge keeps the call open, and their quiescent-precondition/baseline checks fail with misleading errors.

Verifier: The failure scenario survives re-derivation from the actual code. (1) discord.rs:91 spawns the serenity client via tokio::spawn; the DiscordSpeaker has no Drop impl (none exists in the crate), dropping a JoinHandle detaches rather than aborts, and the spawned task owns the Client which holds its own Arc clone (voice_manager_arc, discord.rs:87) — so dropping the struct leaves the gateway session AND the live voice connection running; shard.shutdown_all() is only reachable via the explicit shutdown() (discord.rs:127). (2) scenario.rs:84 wraps each scenario in tokio::time::timeout (300s compressed / 900s real, config.rs:37-42); the smoke scenario's internal waits sum to ~500s worst case, so the outer timeout can fire mid-scenario, dropping the future in the wide window between speaker.join() (smoke.rs:96) and speaker.shutdown() (smoke.rs:127) — three 60-90s awaits sit inside it. (3) run_scenarios loops sequentially in-process (scenario.rs:77); the leaked speaker stays in the test voice channel, so the next scenario's fresh bridge (child is kill_on_drop, bridge_proc.rs:108, so it does NOT leak) mints a presence ghost for the phantom human and the 30s "quiescent precondition" (smoke.rs:82-86) fails with a misleading timeout; await_baseline fails likewise. Reachable today via repeated --scenario flags (not deduped, main.rs:69-71) or any future multi-scenario catalogue. Not pinned as intended by any test; the smoke.rs:54-55 comment ("never leaks a live child (and its Discord voice sessions)") shows the opposite intent — the Err path was covered but cancellation of harness-side actors was missed. ConnCountProbe (observers.rs:202/247) has the identical detached-task shape, though it leaks only a voiceless gateway session. P2 as claimed: harness-only blast radius, bounded to the suite-run process lifetime (Discord clears the voice state once the process exits and heartbeats stop).

Regression test sketch: Not required at P2, but if wanted: in crates/e2e/src/scenario.rs tests, add a scenario fn that sets a shared flag guard with an on-cancel-observable resource (e.g., a struct whose Drop records "cleaned") held across a pending await, run it under a tiny timeout via a Profile test hook, and assert the actor-style resource performed cleanup — this fails until DiscordSpeaker/ConnCountProbe gain Drop (abort JoinHandle + best-effort shard shutdown) or the runner passes a CancellationToken instead of hard-dropping the future.

Filed from an automated multi-agent code review of master @ 968da57 (8 scoped finders + adversarial verification). Every finding below was re-derived from the code by an independent verifier. Fixes for the confirmed local-verifiable defects (allocator anchor panic F1-1/F1-2/F1-3/F2-1/F2-2, capture-sink panic F2-7, config env-fallback F5-4, opt-out RMW race F5-2) already landed on branch refactor/review-and-split.

#38 Tier-3 harness follow-ups: the generated `config.toml` (with appservice + puppet tokens) is written into the collectable artifacts tree (F6-2); `await_baseline`/`voice_members_among` pass vacuously when the guild is absent from cache (F6-3); and the detached serenity client tasks in the actors/probes aren't aborted on scenario cancellation (F6-4). ### F6-2 — P2 / LOCAL / CONFIRMED **Location:** `crates/e2e/src/bridge_proc.rs:88` @ `968da57` **Defect:** BridgeUnderTest::start writes the generated config.toml — containing the plaintext appservice_token and every puppet bot token — into the scenario's artifacts tree (`<artifacts>/<scenario>/bridge/config.toml`), violating the harness's own rule (config.rs doc: secrets are 'never echoed into artifacts') and the repo-wide never-commit-secrets convention. **Failure scenario:** A scenario runs (smoke.rs passes `ctx.artifacts.join("bridge")` as workdir); the operator or a future CI job collects/uploads the e2e-artifacts directory to share the report.json/bridge.log — the live Discord bot tokens and Matrix appservice token ship with it, granting anyone with the artifact full control of the puppet pool and appservice. **Verifier:** Re-derived from code: generate_config (crates/e2e/src/bridge_proc.rs:29-46) embeds the plaintext appservice_token (line 39) and every puppet token (41-43); start() writes it to workdir/config.toml (87-88); smoke.rs:45 passes ctx.artifacts.join("bridge") as workdir, where ctx.artifacts = <artifacts_dir>/<scenario> (scenario.rs:78) — the same tree holding bridge.log and report.json. No code anywhere in the crate deletes or scrubs it after the run. This directly contradicts the crate's own documented invariant (config.rs:9-10: secrets are "never logged, never echoed into artifacts"), and the exposure vector is concrete: the testing design doc (docs/nether-voicebridge-testing-design.md D3, lines 392-398) plans a nightly CI workflow that will "always upload artifacts" with secrets supplied via Forgejo Actions secrets — uploading as written ships the tokens those secrets protect. No #[cfg(test)] test pins the artifacts location as intended (the existing test writes to temp_dir). The .gitignore (lines 13-16) acknowledges "generated configs (which embed tokens too)" in artifacts, but that only covers the git-commit vector and contradicts rather than supersedes config.rs's rule. Minor overstatement in the claim: the tokens are the dedicated harness puppet bots and test appservice as_token (config.rs:74-77 forbids prod tokens), not the prod pool — but a leaked as_token still grants namespace-wide masquerade on the live homeserver, so P2 stands. **Regression test sketch:** Not required (P2), but if added: in bridge_proc.rs `mod tests`, a test that calls generate_config-based start-path plumbing indirectly is heavy (spawns a child); the lean regression is a unit test asserting the post-condition once fixed — e.g. after refactoring start() to place config.toml outside workdir (or delete it after spawn), assert that no file under a simulated artifacts workdir contains cfg.appservice_token or any puppet token after BridgeUnderTest teardown, using a stub bridge_bin (e.g. /bin/cat) so no real bridge is needed. ### F6-3 — P2 / LIVE / CONFIRMED **Location:** `crates/e2e/src/observers.rs:221` @ `968da57` **Defect:** ConnCountProbe::voice_members_among returns an empty Vec when the guild is absent from the gateway cache, so await_baseline passes vacuously; connect() only sleeps a fixed 2 s after READY hoping GUILD_CREATE has arrived, with no verification that the guild is actually cached. **Failure scenario:** On a slow gateway (or if the probe bot's GUILD_CREATE for the test guild arrives >2 s after READY), await_baseline's first poll hits the cache-miss branch, returns Ok immediately, and the scenario reports the 'no bridge bot lingers in voice' postcondition green even while puppet bots are still connected to the voice channel — a false PASS in the exact check the probe exists for. **Verifier:** Fail-open on cache miss is real and reachable: connect() (observers.rs:207-209) only awaits READY then sleeps a fixed 2 s — despite its own doc comment (line 190-191) promising it resolves once "the guild's voice states are cached" — and serenity only caches a guild on its GUILD_CREATE dispatch, which follows READY and can lag >2 s. voice_members_among (lines 221-223) maps the resulting cache miss to an empty Vec, and await_baseline (lines 236-238) returns Ok(()) on the very first empty poll and never re-checks after success, so a late GUILD_CREATE cannot correct it. At the sole call site (scenarios/smoke.rs:133-140) this probe is the only Discord-side 'no bridge bot lingers in voice' postcondition, so the vacuous Ok is a false PASS of exactly the invariant the probe exists for (CLAUDE.md constraint #6: teardown returns connection count to baseline). No #[cfg(test)] test pins the empty-on-miss behavior as intended (tests cover only sender_class/token-decode/urlencode) and no doc declares it deliberate. Only the race frequency is live-dependent; the fail-open logic is deterministic from code, so CONFIRMED rather than UNCERTAIN_LIVE. P2: harness false-negative, not product code. **Regression test sketch:** Not required (P2). If desired: in observers.rs tests, refactor voice_members_among to take Option<&Guild>-like input or make it return Result; assert that a None/absent guild yields an error (or a distinct 'cache not primed' state) instead of an empty Vec, and that await_baseline does not accept empty until the guild has been observed in cache at least once. ### F6-4 — P2 / LIVE / CONFIRMED **Location:** `crates/e2e/src/actors/discord.rs:91` @ `968da57` **Defect:** DiscordSpeaker (and ConnCountProbe in observers.rs) spawn a detached serenity client task with no Drop/abort-on-cancel handling; shutdown() is only reachable by explicit call, so cancelling the scenario future leaks a live gateway session — including the speaker's active voice-channel membership. **Failure scenario:** A scenario exceeds Profile::scenario_timeout(), so run_scenarios' tokio::time::timeout drops the scenario future mid-await (scenario.rs:84): speaker.shutdown() never runs, the spawned client.start() task keeps the speaker bot joined to the test voice channel for the rest of the suite run — every subsequent scenario sees a phantom 'human' in the channel, the bridge keeps the call open, and their quiescent-precondition/baseline checks fail with misleading errors. **Verifier:** The failure scenario survives re-derivation from the actual code. (1) discord.rs:91 spawns the serenity client via tokio::spawn; the DiscordSpeaker has no Drop impl (none exists in the crate), dropping a JoinHandle detaches rather than aborts, and the spawned task owns the Client which holds its own Arc<Songbird> clone (voice_manager_arc, discord.rs:87) — so dropping the struct leaves the gateway session AND the live voice connection running; shard.shutdown_all() is only reachable via the explicit shutdown() (discord.rs:127). (2) scenario.rs:84 wraps each scenario in tokio::time::timeout (300s compressed / 900s real, config.rs:37-42); the smoke scenario's internal waits sum to ~500s worst case, so the outer timeout can fire mid-scenario, dropping the future in the wide window between speaker.join() (smoke.rs:96) and speaker.shutdown() (smoke.rs:127) — three 60-90s awaits sit inside it. (3) run_scenarios loops sequentially in-process (scenario.rs:77); the leaked speaker stays in the test voice channel, so the next scenario's fresh bridge (child is kill_on_drop, bridge_proc.rs:108, so it does NOT leak) mints a presence ghost for the phantom human and the 30s "quiescent precondition" (smoke.rs:82-86) fails with a misleading timeout; await_baseline fails likewise. Reachable today via repeated --scenario flags (not deduped, main.rs:69-71) or any future multi-scenario catalogue. Not pinned as intended by any test; the smoke.rs:54-55 comment ("never leaks a live child (and its Discord voice sessions)") shows the opposite intent — the Err path was covered but cancellation of harness-side actors was missed. ConnCountProbe (observers.rs:202/247) has the identical detached-task shape, though it leaks only a voiceless gateway session. P2 as claimed: harness-only blast radius, bounded to the suite-run process lifetime (Discord clears the voice state once the process exits and heartbeats stop). **Regression test sketch:** Not required at P2, but if wanted: in crates/e2e/src/scenario.rs tests, add a scenario fn that sets a shared flag guard with an on-cancel-observable resource (e.g., a struct whose Drop records "cleaned") held across a pending await, run it under a tiny timeout via a Profile test hook, and assert the actor-style resource performed cleanup — this fails until DiscordSpeaker/ConnCountProbe gain Drop (abort JoinHandle + best-effort shard shutdown) or the runner passes a CancellationToken instead of hard-dropping the future. --- *Filed from an automated multi-agent code review of `master` @ `968da57` (8 scoped finders + adversarial verification). Every finding below was re-derived from the code by an independent verifier. Fixes for the confirmed local-verifiable defects (allocator anchor panic F1-1/F1-2/F1-3/F2-1/F2-2, capture-sink panic F2-7, config env-fallback F5-4, opt-out RMW race F5-2) already landed on branch `refactor/review-and-split`.*