tests: assert the refresh_failed observation the comment promised

[iceteaSA]

Follow-up to #24, from its review.

That PR's test comment says the engine "writes a refresh_failed observation here that no direct store call produces" — and that was the stated reason the test had to run the engine instead of reproducing its outcome. Nothing checked it.

A claim in prose with no assertion behind it is the same defect as a guard whose coverage is invisible from its green result. The test passed, the comment read as a guarantee, and deleting the observation outright would have kept it passing.

Why the detail matters as much as the row

The audit chain's entry at this point is a generic invalidate. It cannot distinguish a terminal invalid_grant from a transient transport failure, and only the terminal one latches. So this row is the only place that records that a provider was reached and what it answered.

Which makes a refresh_failed carrying the wrong detail worse than an absent one — it reads as diagnosed.

Both cases mutation-proved

The reviewer named removal; misclassification is the same hole and is covered too.

observation -> None            count drops to 0; failure prints the surviving
                               consumer_report_stale row

detail -> hardcoded "transport" row stays, fails on the detail instead — the arm
                               a count assertion alone would not catch

restored                        green

Verification

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

Pinned at subconscious 572e247d. Master's lock moved to subc-core 0.13.0 while this was in flight — 989ddb27 (which #24 was gated against) no longer resolves. The wire crates did not move with it:

989ddb27   core 0.12.0   proto 0.14.0   transport 0.5.1
44e03c27   core 0.13.0   proto 0.14.0   transport 0.5.1
572e247d   core 0.13.0   proto 0.14.0   transport 0.5.1

subc-core reaches this crate through dev-dependencies only (cargo tree -e normal gives zero hits), so the moving version never enters the shipped binary.

---

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

---

Summary by cubic

Adds assertions to the refresh-failed test so the engine's promised refresh_failed observation is actually verified. Previously the test passed even if that observation was removed or misclassified; now it checks that exactly one row is written, with the correct credential ID and a detail of invalid_grant.

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

[cubic-dev-ai]

1 issue found across 1 file

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:4759">
P3: The PR description and reviewer guidance say the test should stop relying on a guaranteed `refresh_failed` observation, but this added block asserts exactly one such row must exist. The flow is deterministic today (the InvalidGrant arm writes the row via `invalidate_if_version_reported`, not best-effort), so it is not flaky, but the implementation contradicts the stated intent. Reconcile the two: either keep the strict assertion and fix the description, or drop the `len()==1`/detail coupling so the test doesn't depend on the observation being written.</violation>
</file>

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

Re-trigger cubic</sub>

[cubic-dev-ai]

P3: The PR description and reviewer guidance say the test should stop relying on a guaranteed refresh_failed observation, but this added block asserts exactly one such row must exist. The flow is deterministic today (the InvalidGrant arm writes the row via invalidate_if_version_reported, not best-effort), so it is not flaky, but the implementation contradicts the stated intent. Reconcile the two: either keep the strict assertion and fix the description, or drop the len()==1/detail coupling so the test doesn't depend on the observation being written.

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 4759:

<comment>The PR description and reviewer guidance say the test should stop relying on a guaranteed `refresh_failed` observation, but this added block asserts exactly one such row must exist. The flow is deterministic today (the InvalidGrant arm writes the row via `invalidate_if_version_reported`, not best-effort), so it is not flaky, but the implementation contradicts the stated intent. Reconcile the two: either keep the strict assertion and fix the description, or drop the `len()==1`/detail coupling so the test doesn't depend on the observation being written.</comment>

<file context>
@@ -4744,6 +4744,35 @@ mod tests {
+            .filter(|e| e.kind == "refresh_failed")
+            .collect();
+        assert_eq!(
+            refresh_failed.len(),
+            1,
+            "the engine's invalid_grant arm must leave exactly one refresh_failed row; \
</file context>

[iceteaSA]

Landed as ae27bb0 on master — closing this.

Verified in the merged tree rather than by the commit title: the detail assertion is at main.rs:4791, so both arms the PR argued for are live. Thanks for taking it directly.