tests: drive the latch through the engine, not around it - #24
Merged
ualtinok merged 1 commit intoAug 30, 2026
Merged
Conversation
Third version of this test, and the reviewer was right twice. The test pins that `credential.status` reports `stale_pending: false` on a record that has left Active -- because the column is never cleared by any of the seven state-update paths, while the next `get` on a latched record fails fast without an upstream exchange. The field predicts cost; on a latched record the honest prediction is "cheap". What kept being wrong was how the test REACHED that state. Version one called `store.invalidate` directly. Version two swapped in `invalidate_if_version_reported`, the version-fenced call production uses -- closer, and still a direct store call. Both reproduced the outcome of a failed refresh rather than running one, so if the engine's failure arm ever also cleared `stale_pending`, the test would have stayed green while the state it pins became unreachable. That is precisely the defect this test was rewritten to fix in the first place, surviving two rewrites aimed at it. It now runs the real path: a consumer reports a 401, the mark lands, the next `get` sees it and forces a refresh, and the provider refuses. The engine latches the record and writes the `refresh_failed` observation that no direct store call produces. Two details are load-bearing and easy to get wrong later. The fixture adapter returns `InvalidGrant` specifically, because that is the only provider verdict the engine treats as terminal -- every other error takes the transient arm, clears the intent and leaves the record ACTIVE. And the `get` passes `force_refresh: false`, so the refresh is driven by the `stale_pending` mark the report left behind rather than by the test asking for one; forcing it would pass on a path no consumer takes. Both are mutation-proved rather than asserted. Reverting the production fix turns the pin red on its own message. Swapping the adapter's error to `Transport` turns it red at the engine step, reporting `RefreshFailed` and a record still Active, which is what makes the InvalidGrant choice a finding rather than a detail. Gate: exit 0, nine real-daemon e2e arms executing.
There was a problem hiding this comment.
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:4721">
P2: This test does not verify the `refresh_failed` observation promised by the new engine path. Assert the recorded event kind and `invalid_grant` detail so removal or misclassification of that diagnostic write fails the test.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // and both would have stayed green if the engine's failure arm changed, which is the | ||
| // whole defect they were written to pin. The engine also writes a `refresh_failed` | ||
| // observation here that no direct store call produces. | ||
| let err = surface |
There was a problem hiding this comment.
P2: This test does not verify the refresh_failed observation promised by the new engine path. Assert the recorded event kind and invalid_grant detail so removal or misclassification of that diagnostic write fails the test.
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 4721:
<comment>This test does not verify the `refresh_failed` observation promised by the new engine path. Assert the recorded event kind and `invalid_grant` detail so removal or misclassification of that diagnostic write fails the test.</comment>
<file context>
@@ -4671,18 +4711,38 @@ mod tests {
+ // and both would have stayed green if the engine's failure arm changed, which is the
+ // whole defect they were written to pin. The engine also writes a `refresh_failed`
+ // observation here that no direct store call produces.
+ let err = surface
+ .get(
+ 11,
</file context>
ualtinok
added a commit
that referenced
this pull request
Aug 30, 2026
12a1b4c refreshed this lock against a subconscious checkout that was AHEAD of the revision the fleet is pinned to. The result named a subc-core 0.12.0 that declares reqwest and rusqlite -- a combination that exists only at intermediate sibling revisions. Anyone building against the published pin got error: cannot update the lock file ... because --locked was passed which reads as the builder's own tree being dirty. It is not: git status stays clean throughout, so the diagnosis points away from the actual cause. Found by iceteaSA on #24, who checked a clean detached origin/master before assuming their rebase caused it -- the check that separates 'my branch is wrong' from 'master is wrong', and the one I did not run when I introduced it. The fix is forward, not a revert: the sibling has since moved to 0.13.0, my checkout is aligned with its origin/master, and the lock now names that. Delta read rather than assumed -- exactly one line, 0.12.0 -> 0.13.0, with the two dependency entries staying because 0.13.0 declares them. WHAT I GOT WRONG WHEN I SETTLED 12a1b4c: I reasoned that the sibling had gained deps without a version bump and concluded my lock was merely catching up. The direction was right and the frame was wrong -- a lock refreshed against a local sibling describes MY checkout, and that is only the fleet's reference if the two are aligned. Checking alignment costs one rev-parse and I did not do it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third version of this test. The reviewer was right twice, and the second time the fix was still wrong in the same way.
The test pins that
credential.statusreportsstale_pending: falseon a record that has left Active — the column is never cleared by any of the seven state-update paths, while the nextgeton a latched record fails fast without an upstream exchange. The field predicts cost; on a latched record the honest prediction is "cheap".What kept being wrong was how the test reached that state.
Both earlier versions reproduced the outcome of a failed refresh rather than running one. If the engine's failure arm ever also cleared
stale_pending, they would have stayed green while the state they pin became unreachable — which is exactly the defect this test was rewritten to fix in the first place, surviving two rewrites aimed at it.It now runs the path: a consumer reports a 401, the mark lands, the next
getsees it and forces a refresh, the provider refuses. The engine latches the record and writes therefresh_failedobservation no direct store call produces.Two details that are load-bearing and easy to lose later
The fixture returns
InvalidGrantspecifically. That is the only provider verdict the engine treats as terminal. Every other error takes the transient arm, clears the intent, and leaves the record Active — so a stub that merely fails cannot reproduce a latch.The
getpassesforce_refresh: false. The refresh has to be driven by thestale_pendingmark the report left behind. Forcing it would make the test pass on a path no consumer takes.Neither is asserted — both are mutation-proved:
The second is what makes the
InvalidGrantchoice a finding rather than a detail.Heads-up: master does not currently resolve
--lockedIndependent of this PR, and I checked it on a clean detached
origin/masterbefore assuming my rebase caused it:Those are the two lines
12a1b4cadded. The lock now describes asubc-corethat declaresreqwestandrusqlite; the sibling revision published to the fleet (20029b3d) declares neither. They appear in7e4a705aand later.So master's lock was refreshed against a subconscious newer than the one the fleet is pinned to. Anyone building against the published pin gets a
--lockedfailure that looks like their own tree is dirty — it is not, andgit statusstays clean throughout.Gated at
989ddb27: exit 0, nine real-daemon e2e arms executing.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Rewrites the credential latch test to drive the failure through the engine instead of calling the store directly, so it catches regressions in both the store and the engine's failure arm.
Why the test changed
InvalidGrant(the only terminal provider verdict) and thegetpassesforce_refresh: false, so thestale_pendingmark drives the refresh.Transportfails at the engine step.Heads-up for reviewers
cargo metadata --lockedfails on master because the lock file was refreshed against a newersubconsciousthan the fleet pin; the lockfile currently resolves only until the version bump.Written for commit 05373ab. Summary will update on new commits.