tests: fix six review findings, four of which were guards not guarding

[iceteaSA]

Six issues the reviewer raised on #20, #21 and #22 after they merged. I checked each against source rather than taking the read. All six are real, and four are the same shape as the defects those PRs were written to fix.

The gate's skip guard was a magic-string check wearing derivation clothes

#21 replaced an --ignored-keyed guard with one that "asks the source" whether a target can emit a skip notice. It greps case-sensitively for SKIPPING:

cli_admin.rs        SKIPPING=0   skipping(any case)=1    <- line 1585, eprintln!, lowercase
real_daemon_e2e.rs  SKIPPING=1   skipping(any case)=1

The probe stayed silent for cli_admin. Worse, run_expect's own downstream skip check greps case-sensitively too — so even with --nocapture, that arm's skip was invisible to the guard. I swapped one magic string for another and called the result derived.

Fixed by standardizing the convention, not widening the matcher. cli_admin.rs now emits the uppercase token, and its gate arm passes --nocapture so the newly-armed probe has something to see.

Widening the matcher was the tempting fix and it is wrong: it false-fires on that arm, whose skipping test is excluded by the test-name selector — something the probe cannot observe. An over-firing guard is not the conservative failure. The next person hits a red on an arm they know is fine, concludes the guard is broken, and deletes it rather than narrowing it.

The argument scan ran past the separator

A --test appearing after -- overwrote target, last-one-wins, and could aim the probe at a file with no skip path. Target capture now stops at --. --nocapture detection deliberately does not, because the real_daemon arm places it after the separator.

The frame pin did not exercise the route

#20 added a pin that hand-built json!({"result": inner}) and compared it against itself. Renaming the wrapper in handle_read_request would break every consumer while the test stayed green — the exact defect the pin exists to catch, one level in. It pinned my belief about my own route rather than the route's output.

Both now go through wrap_result. Mutating the wrapper inside the production helper turns the test red:

json!({ "result": value })  ->  json!({ "payload_envelope": value })
panicked: the outer `result` wrapper vanished — every route reply in
handle_read_request is wrapped in {"result": ...}

The non-Active test built its state off-path

It reached needs_reauth through the unversioned store.invalidate rather than the version-fenced call the engine makes when a refresh fails. Same resulting state today, which is why it passed. If the engine's path ever also cleared stale_pending, the test would have stayed green while production diverged.

Two NoHttp stubs became one

#[cfg(test)]-gated. Verified it stays out of the shipped artifact — zero occurrences in the release binary, against a control string that scores three.

Master interaction

01f76b1 adds scripts only; 004fc06 touches the manifest path. Neither overlaps these changes.

Verification

bash scripts/gate.sh exit 0, nine real-daemon e2e arms executing.

Mutation-checked, each red then restored green: wrapper rename, version fence, NoHttp symbol, guard without --nocapture, post-separator override, and a non-skipping target staying green. That last one is the control that matters — a guard which has only ever fired has shown it can trigger, not that it can distinguish.

One thing left open, deliberately

Eight route sites still hand-roll json!({ "result": ... }) rather than calling wrap_result. The get path this pin covers goes through the helper, so the finding is closed — but a helper one call site uses is a convention, not a seam. Worth folding the rest in, and it is a wider change than a review-fix PR should carry.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Fixes six review findings on the merged PRs #20–#22; four were guards that did not guard the same shape of defect those PRs were written to fix.

• The gate's skip guard now requires the uppercase SKIPPING token; cli_admin.rs emits it and its gate arm passes --nocapture so the probe can see the notice. Widening the matcher would have false-fired because the probe cannot observe test-name filters.
• run_expect target capture now stops at --, so a --test after the separator can no longer overwrite the target; --nocapture detection still reads past it.
• The frame pin now serializes through the production wrap_result helper instead of hand-building {"result": ...} and comparing against itself, so renaming the wrapper in handle_read_request turns the test red.
• The non-active test builds needs_reauth through the engine's version-fenced invalidate_if_version_reported rather than the unversioned store.invalidate.
• The two NoHttp stubs became one #[cfg(test)] helper in test_support.rs; verified absent from the release binary.

Left for a follow-up: eight route sites still hand-roll json!({ "result": ... }) instead of calling wrap_result; folding them in is a wider change than a review-fix PR should carry.

^{Written for commit b921061. Summary will update on new commits.}

[cubic-dev-ai]

1 issue found across 6 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/credentials-module/src/main.rs">

<violation number="1" location="crates/credentials-module/src/main.rs:4681">
P2: This test bypasses `RefreshEngine`: `report_auth_failure` only marks the record stale, then this direct store call manufactures `NeedsReauth`. Drive a failing refresh through `RefreshEngine` (including its observation), or the test remains green if the production failure path diverges.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: This test bypasses RefreshEngine: report_auth_failure only marks the record stale, then this direct store call manufactures NeedsReauth. Drive a failing refresh through RefreshEngine (including its observation), or the test remains green if the production failure path diverges.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/src/main.rs, line 4681:

<comment>This test bypasses `RefreshEngine`: `report_auth_failure` only marks the record stale, then this direct store call manufactures `NeedsReauth`. Drive a failing refresh through `RefreshEngine` (including its observation), or the test remains green if the production failure path diverges.</comment>

<file context>
@@ -4723,11 +4674,16 @@ mod tests {
+        // touched by any of the seven state-update paths, which is the bug pinned here.
         store
-            .invalidate("oauth:needs_reauth_after_stale")
+            .invalidate_if_version_reported(
+                "oauth:needs_reauth_after_stale",
+                1,
</file context>