Skip to content

tests: drive the latch through the engine, not around it - #24

Merged
ualtinok merged 1 commit into
cortexkit:masterfrom
legion-works:fix/engine-driven-latch-test
Aug 30, 2026
Merged

tests: drive the latch through the engine, not around it#24
ualtinok merged 1 commit into
cortexkit:masterfrom
legion-works:fix/engine-driven-latch-test

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

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.status reports stale_pending: false on a record that has left Active — 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.

v1  store.invalidate(...)                     unversioned, direct
v2  store.invalidate_if_version_reported(...)  version-fenced, still direct
v3  a real failing refresh through RefreshEngine

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 get sees it and forces a refresh, the provider refuses. The engine latches the record and writes the refresh_failed observation no direct store call produces.

Two details that are load-bearing and easy to lose later

The fixture returns InvalidGrant specifically. 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 get passes force_refresh: false. The refresh has to be driven by the stale_pending mark the report left behind. Forcing it would make the test pass on a path no consumer takes.

Neither is asserted — both are mutation-proved:

revert the production fix        RED on the pin's own message
adapter InvalidGrant -> Transport RED at the engine step: RefreshFailed, record still Active

The second is what makes the InvalidGrant choice a finding rather than a detail.

Heads-up: master does not currently resolve --locked

Independent of this PR, and I checked it on a clean detached origin/master before assuming my rebase caused it:

cargo metadata --locked --offline   ->   cannot update the lock file
cargo metadata --offline            ->   wants to REMOVE exactly two lines
                                          "reqwest", "rusqlite"  from subc-core

Those are the two lines 12a1b4c added. The lock now describes a subc-core that declares reqwest and rusqlite; the sibling revision published to the fleet (20029b3d) declares neither. They appear in 7e4a705a and later.

subconscious 20029b3d   subc-core 0.12.0   reqwest ✗  rusqlite ✗   <- current fleet pin
subconscious 7e4a705a   subc-core 0.12.0   reqwest ✓  rusqlite ✓
subconscious 989ddb27   subc-core 0.12.0   reqwest ✓  rusqlite ✓   <- resolves
subconscious 44e03c27   subc-core 0.13.0                            <- version no longer matches

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 --locked failure that looks like their own tree is dirty — it is not, and git status stays clean throughout.

Gated at 989ddb27: exit 0, nine real-daemon e2e arms executing.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with 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

  • The prior test called the store directly, reproducing the outcome without running the engine's failure path.
  • Now the fixture returns InvalidGrant (the only terminal provider verdict) and the get passes force_refresh: false, so the stale_pending mark drives the refresh.
  • Both details are mutation-tested: reverting the production fix turns the pin red, and swapping the error to Transport fails at the engine step.

Heads-up for reviewers

  • cargo metadata --locked fails on master because the lock file was refreshed against a newer subconscious than the fleet pin; the lockfile currently resolves only until the version bump.

Written for commit 05373ab. Summary will update on new commits.

Review in cubic

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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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
ualtinok merged commit d83d671 into cortexkit:master Aug 30, 2026
4 of 6 checks passed
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants