Skip to content

tests: assert the refresh_failed observation the comment promised #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
iceteaSA wants to merge 1 commit into from
Closed

tests: assert the refresh_failed observation the comment promised #25

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions crates/credentials-module/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4744,6 +4744,35 @@ mod tests {
"the forced refresh must fail through the engine: {err:?}"
);

// The observation, asserted rather than advertised. The comment above claims the
// engine writes a diagnostic no direct store call produces -- and until this
// assertion existed that was a claim in prose with nothing behind it, which is the
// same defect as a guard whose coverage is invisible from its green result. The
// audit chain records a generic `invalidate` here; only this row says the provider
// was reached and what it answered.
let events = store.recent_auth_events(10).expect("read events");
let refresh_failed: Vec<_> = events
.iter()
.filter(|e| e.kind == "refresh_failed")
.collect();
assert_eq!(
refresh_failed.len(),

@cubic-dev-ai cubic-dev-ai Bot Aug 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

1,
"the engine's invalid_grant arm must leave exactly one refresh_failed row; \
got {events:?}"
);
assert_eq!(
refresh_failed[0].credential_id, "oauth:needs_reauth_after_stale",
"the row must name the credential the refresh was attempted for"
);
assert_eq!(
refresh_failed[0].detail.as_deref(),
Some("invalid_grant"),
"the row must carry WHICH provider verdict was seen -- a `refresh_failed` with \
no detail cannot distinguish a terminal invalid_grant from a transient \
transport failure, and only the terminal one latches"
);

// Precondition checks: the construction actually reproduced the live shape, so a
// green fix can be trusted to mean the fix is real and not a different test
// passing for a different reason.
Expand Down
Loading