From d65ab862611a3cb5a18252a693f205b3ac7b9040 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Sat, 29 Aug 2026 17:59:18 +0200 Subject: [PATCH 1/2] status: stop lying about the next get's cost; pin the whole wire frame Three defects on the credential.status surface were found by an automated reviewer after the stale_pending field shipped in 8d3d81c and bc87553. All three are fixed here; the field is now a real latency predictor on the records it claims to describe, and the test for the wire shape pins the full on-wire frame rather than the inner body alone. The first defect was a live one. None of the seven `UPDATE credentials SET state = ...` paths in credentials-core::store clear the stale_pending column, so a record marked stale by a consumer 401 and then latched to needs_reauth (the path taken when the forced refresh fails) still carries stale_pending = 1 on the column. status published the column value as a latency prediction: stale_pending=true said "next get pays seconds" while the next get failed fast with needs_reauth and never touched the network. Measured on this deployment 2026-08-27: every four hours, for roughly a five-minute window, until the re-seal wrote state='active', stale_pending=0 in one statement and hid the lie. The fix lives at the read surface, not the store: when a handle resolved and the record state is Active, publish the real meta.stale_pending; when the state is anything else, publish false -- the next get performs no upstream exchange, so the prediction must say so. Absent is unchanged on the resolve-fail and meta-unreadable arms; that semantic ("this path could not see the record") must not move. A new test pins the non-Active case. The state is constructed through the production paths -- public report_auth_failure sets the mark on a refreshable id, then store.invalidate does exactly what the engine does when its forced refresh fails -- so the test is a real reading of the buggy state, not a hand-staged copy of it. Mutation-checked both ways: reverting the fix turns the assertion red with "left: Some(true), right: Some(false)", and the precondition that meta.stale_pending survives the state flip is asserted first so the test's failure mode names the live shape rather than the diff. The second defect was a test-only one. The existing status_publishes_the_stale_mark_without_calling_the_credential_unhealthy test seeded apikey:active (non-refreshable) and called store.mark_stale_if_version_reported directly with the observation kind "consumer_report_stale" -- a string only the refreshable arm of report_auth_failure emits. The public route branches on refreshability and the non-refreshable arm INVALIDATES rather than marks, so the production path cannot produce the state the test was constructing; the test was passing against a copy of the mark with no assertion behind it. The fix seeds oauth:stub (refreshable per default_refresh_adapter) and drives the mark through the public report_auth_failure route, the only call that can ever set the marker on a real handle. The three assertions (mark visible without calling get, ready stays true, last_error_code stays None) keep their original intent and meaning. Mutation-checked by inverting the refreshable arm in report_auth_failure to call invalidate_if_version_reported: the test goes red with "left: Some(false), right: Some(true)" and the precondition that meta.stale_pending is set fails too. Restore returns green. The third defect was a comment-vs-coverage gap. The test pinned only the inner error body `{"error":{"code":"not_found","class":"permanent"}}` while the comment claimed it was "the exact frame captured from a live daemon". The transport layer wraps that body in `{"result": ...}` inside handle_read_request, and renaming the outer key left the test green while every consumer decoder that routes on `result` broke. The pin is now extended to the full on-wire frame `{"result":{"error":{"code":"not_found","class":"permanent"}}}` and written as a single readable JSON literal so the byte sequence can be quoted verbatim into the consumer's fixture rather than re-derived from a producer. The producer is still the real GetOutcome::Err serialization, then wrapped with the same key the route builder uses; a reconstruction would only pin a reconstruction. Two specific diagnostics precede the equality check (the outer wrapper vanished; the class field vanished from the body), so the broad assert_eq! panic names the shape rather than asking the reader to diff two blobs. Mutation-checked by renaming the wrapper key to result_mutated (RED with the wrapper diagnostic), then changing the class value (RED with the broad drift message showing the exact difference). Restore returns green. Gate: 441 tests, exit 0, nine real-daemon e2e arms executing. --- crates/credentials-module/src/main.rs | 183 ++++++++++++++++-- crates/credentials-module/src/read_surface.rs | 125 ++++++++---- 2 files changed, 254 insertions(+), 54 deletions(-) diff --git a/crates/credentials-module/src/main.rs b/crates/credentials-module/src/main.rs index e0ee6b7..d14ae90 100644 --- a/crates/credentials-module/src/main.rs +++ b/crates/credentials-module/src/main.rs @@ -1466,8 +1466,9 @@ mod tests { use cortexkit_store::{Isolation, StorageBackend}; use credentials_core::audit::{AuditCtx, AuditOp, AuditRecord}; use credentials_core::key::{MasterKey, MASTER_KEY_LEN}; + use credentials_core::oauth::OAuthCredential; use credentials_core::record::{CredentialKind, VaultRecord}; - use credentials_core::store::GrantOperation; + use credentials_core::store::{GrantOperation, RecordState}; use read_surface::ReadSurface; fn tmp_surface(seed: u8) -> Arc { @@ -4401,17 +4402,44 @@ mod tests { /// exchange. A consumer sizing a startup bound cannot get that from `ready`, because /// `ready` is genuinely TRUE -- the mark exists so the next get refreshes rather than /// refusing. + /// + /// The fixture is `oauth:stub` -- refreshable per `default_refresh_adapter` -- and the + /// mark is driven through the PUBLIC `report_auth_failure` route, the only call that + /// can ever set the marker on a real handle. The previous version seeded + /// `apikey:active` and called `store.mark_stale_if_version_reported` directly, which + /// constructs a state (non-refreshable + Active + `stale_pending = 1`) the production + /// path cannot produce: the public route branches on refreshability and the + /// non-refreshable arm INVALIDATES rather than marks, so the test was passing against + /// a hand-staged copy of the mark with no assertion behind it. #[tokio::test] async fn status_publishes_the_stale_mark_without_calling_the_credential_unhealthy() { let (surface, store, _db) = tmp_surface_with_store(16); + store + .create( + "oauth:stub", + &VaultRecord::new_oauth( + "test", + "stub", + OAuthCredential { + access_token: "locally-valid".into(), + refresh_token: "rt".into(), + expires_at_ms: Some(i64::MAX), + token_url: "https://example.invalid/token".into(), + client_id: None, + scopes: Vec::new(), + }, + b"locally-valid".to_vec(), + ), + ) + .expect("seed refreshable credential"); let handle = credentials_core::store::mint_handle().expect("mint handle"); store .put_handle_hash( &handle.hash, - "apikey:active", + "oauth:stub", AuditCtx::admin(AuditOp::MintHandle), ) - .expect("put handle"); + .expect("bind handle"); let clean = surface .status( @@ -4428,22 +4456,22 @@ mod tests { ); assert!(clean.ready, "precondition: the record starts healthy"); - // Exactly what a consumer's 401 report does, at the version it was served. - let served = clean - .record_version - .expect("resolved handle reports version"); - store - .mark_stale_if_version_reported( - "apikey:active", - served, - AuditCtx::admin(AuditOp::ReportAuthFailure), - credentials_core::store::AuthObservation { - kind: "consumer_report_stale", - provider_status: Some(401), - detail: None, + // Exactly what a consumer's 401 report does: the public route sees a refreshable + // id and chooses the stale arm, so the record stays Active and `stale_pending` + // flips to 1. Going through `report_auth_failure` rather than the store method is + // the point -- the version-gated invalidate arm on the non-refreshable path is the + // shape that has to be bypassed for a hand-staged mark to be possible. + surface + .report_auth_failure( + 1, + &read_surface::ReportAuthFailureParams { + handle: handle.raw.clone(), + provider_status: 401, + record_version: 1, }, ) - .expect("mark stale"); + .await + .expect("report accepted"); let marked = surface .status( @@ -4493,6 +4521,127 @@ mod tests { ); } + /// The stale-pending mark must NOT advertise an upstream exchange on a record the + /// next `get` will refuse out of hand. + /// + /// Pins the second half of the field's contract: it is a LATENCY PREDICTOR, not a + /// claim that anything is happening. A consumer reading `stale_pending: true` + /// concludes the next get is going to spend seconds on a token exchange -- the very + /// reason this field exists -- and will SKIP the credential in a startup warm bound. + /// Skipping is the only safe behaviour when the mark is true, because the alternative + /// is paying the exchange that the mark warned about. + /// + /// The construction reproduces the live shape on this deployment every four hours, + /// measured 2026-08-27: a consumer 401 marks a refreshable record stale, the forced + /// refresh fails, the engine latches the record to `needs_reauth`, and `stale_pending` + /// is left at 1 because none of the seven `UPDATE credentials SET state = ...` paths + /// in `credentials-core::store` clear the column. The mark is then a five-minute lie: + /// `stale_pending: true` says "next get pays seconds" while the next get fails fast + /// with `needs_reauth` without touching the network. + /// + /// The state is constructed through the production paths (public `report_auth_failure` + /// sets the mark, the same `store.invalidate` the engine uses after a failed refresh + /// flips the state), so the test is a real reading of the buggy state rather than a + /// hand-staged copy of it. A pure store-level construction would pass without ever + /// proving the public route is part of the path that creates it. + #[tokio::test] + async fn status_does_not_publish_a_stale_pending_mark_on_a_non_active_record() { + let (surface, store, _db) = tmp_surface_with_store(17); + store + .create( + "oauth:needs_reauth_after_stale", + &VaultRecord::new_oauth( + "test", + "stub", + OAuthCredential { + access_token: "locally-valid".into(), + refresh_token: "rt".into(), + expires_at_ms: Some(i64::MAX), + token_url: "https://example.invalid/token".into(), + client_id: None, + scopes: Vec::new(), + }, + b"locally-valid".to_vec(), + ), + ) + .expect("seed refreshable credential"); + let raw = credentials_core::store::mint_handle().expect("mint handle"); + store + .put_handle_hash( + &raw.hash, + "oauth:needs_reauth_after_stale", + AuditCtx::admin(AuditOp::MintHandle), + ) + .expect("bind handle"); + + // Production step 1: a consumer reports a 401 on the served version. The public + // route is refreshable, so it MARKS STALE rather than invalidating; the record + // stays Active and `stale_pending` becomes 1. + surface + .report_auth_failure( + 11, + &read_surface::ReportAuthFailureParams { + handle: raw.raw.clone(), + provider_status: 401, + record_version: 1, + }, + ) + .await + .expect("report accepted"); + + // Production step 2: a forced refresh then fails and the engine latches the record + // to `needs_reauth`. The store call below is exactly what the engine reaches for + // at the failure site; the column `stale_pending` is deliberately not touched by + // any of the seven state-update paths, which is the bug we are pinning here. + store + .invalidate("oauth:needs_reauth_after_stale") + .expect("engine-style invalidate after failed refresh"); + + // 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. + let meta = store.meta("oauth:needs_reauth_after_stale").expect("meta"); + assert_eq!( + meta.state, + RecordState::NeedsReauth, + "precondition: the construction must leave the record latched" + ); + assert!( + meta.stale_pending, + "precondition: the bug is exactly that stale_pending survives a state flip" + ); + + // The pin. Non-Active state => next get performs no upstream exchange, so the + // field is FALSE regardless of the column. Absent is reserved for "this path + // could not see the record" and must NOT be used here -- a defaulted false on a + // known record would be a defensible reading, an absent one would be a missing + // field that looks like a wire-drift to a consumer. + let got = surface + .status( + 11, + &crate::read_surface::StatusParams { + handle: Some(raw.raw), + }, + ) + .await; + assert_eq!( + got.stale_pending, + Some(false), + "non-Active state must publish the real (false) prediction, not the column's \ + stale value -- a consumer skipping the credential on stale_pending=true \ + would be skipping a credential whose next get refuses without an exchange" + ); + assert!( + !got.ready, + "a latched record is not ready -- the rest of the contract is unchanged" + ); + assert_eq!( + got.last_error_code, + Some(read_surface::ReadError::NeedsReauth), + "a needs_reauth record must name the reason" + ); + } + #[tokio::test] async fn status_handle_probe_runs_the_limiter() { let (surface, store, _db) = tmp_surface_with_store(15); diff --git a/crates/credentials-module/src/read_surface.rs b/crates/credentials-module/src/read_surface.rs index 8ed8f0a..6aefcae 100644 --- a/crates/credentials-module/src/read_surface.rs +++ b/crates/credentials-module/src/read_surface.rs @@ -1208,24 +1208,44 @@ impl ReadSurface { match self.engine.store().resolve_handle(handle) { Ok(credential_id) => match self.engine.store().meta(&credential_id) { - Ok(meta) => StatusResult { - // A fenced-out daemon is not ready even for an Active credential. - ready: !fenced_out - && matches!(meta.state, credentials_core::store::RecordState::Active), - // Deliberately NOT folded into `ready`: a stale-marked record is - // still usable, it is merely expensive on the next read. - stale_pending: Some(meta.stale_pending), - last_error_code: match meta.state { - credentials_core::store::RecordState::NeedsReauth - | credentials_core::store::RecordState::Retired => { - Some(ReadError::NeedsReauth) - } - credentials_core::store::RecordState::Corrupt => Some(ReadError::Corrupt), - credentials_core::store::RecordState::Active => None, - }, - lease_held, - record_version: Some(meta.record_version), - }, + Ok(meta) => { + // The field is a LATENCY PREDICTOR for the next get, and the + // prediction only has meaning when the next get will actually run. + // None of the seven `UPDATE credentials SET state = ...` paths in the + // store clear `stale_pending`, so a record that was marked stale by a + // consumer 401 and then latched to `needs_reauth` (or quarantined) by + // a failed refresh still carries `stale_pending = 1` on the column. + // Publishing that as the prediction would say "next get pays seconds" + // for a call that fails fast without an upstream exchange -- measured + // live on 2026-08-27, every four hours for ~five minutes, until the + // re-seal writes state = 'active' and stale_pending = 0 together. + // + // Non-Active => the next get performs no upstream exchange => FALSE. + // Absent stays reserved for "this path could not see the record" and + // is unchanged on the resolve and meta-fail arms below. + let is_active = + matches!(meta.state, credentials_core::store::RecordState::Active); + StatusResult { + // A fenced-out daemon is not ready even for an Active credential. + ready: !fenced_out && is_active, + // Deliberately NOT folded into `ready`: a stale-marked record is + // still usable, it is merely expensive on the next read. Published + // only when Active; see the comment above for the bug this gates. + stale_pending: Some(if is_active { meta.stale_pending } else { false }), + last_error_code: match meta.state { + credentials_core::store::RecordState::NeedsReauth + | credentials_core::store::RecordState::Retired => { + Some(ReadError::NeedsReauth) + } + credentials_core::store::RecordState::Corrupt => { + Some(ReadError::Corrupt) + } + credentials_core::store::RecordState::Active => None, + }, + lease_held, + record_version: Some(meta.record_version), + } + } // Meta unreadable: absent, not false. Reporting "no repair pending" for // a record we could not read would be an assertion with no basis. Err(_) => StatusResult { @@ -1482,53 +1502,84 @@ mod error_class_tests { /// Golden conformance for the FRAME SHAPE, which the class-string test above does /// not cover and cannot: it pins the four `class` values while saying nothing about /// the envelope they arrive in. Rename `class` to `error_class`, move the error a - /// level, or drop `class` from the body entirely, and that test stays green while - /// every consumer breaks. + /// level, drop `class` from the body, rename the outer `result` key, or wrap the + /// body in a second envelope, and the class-string test stays green while every + /// consumer breaks. /// /// WHY THIS EXISTS AT ALL: a consumer typed a decoder from the published contract, /// parsed a real error frame SUCCESSFULLY, and silently discarded `class` — serde /// drops unknown fields without complaint, so a decoder that ignores the field it /// was told to branch on looks identical to one that honours it. They then branched /// on `code` through a closed enum, which turns the first added code into a parse - /// failure rather than an unknown-code branch. Neither is reachable from this side; - /// what IS reachable is guaranteeing the bytes never move under them. + /// failure rather than an unknown-code branch. The outer `result` wrapper is the + /// field they actually depend on for routing their decoder to the body, and neither + /// their fixture nor this test pinned it before — it had two owners and no + /// assertion. Neither is reachable from this side; what IS reachable is guaranteeing + /// the bytes never move under them. /// - /// Serialized through the REAL producer type rather than a hand-built `json!`, so - /// this pins what the wire actually carries. A reconstruction would only pin the - /// reconstruction — the frame could drift and this would still pass. + /// Serialized through the REAL producer type rather than a hand-built `json!`, then + /// wrapped with the same `result` key `handle_read_request` puts around every route + /// reply — so this pins the full on-wire frame `{"result":{"error":{...}}}`, not just + /// the inner body. A reconstruction would only pin the reconstruction — the frame + /// could drift and this would still pass. /// - /// The literal is the exact frame captured from a live daemon and handed to that - /// consumer, who pinned it in their tree. Both directions now go red on drift. + /// The literal below is the on-wire frame captured from a live daemon and handed to + /// that consumer, who pinned it in their tree. Both directions now go red on drift. + /// Written as a single JSON literal so the byte sequence can be quoted verbatim into + /// the consumer's fixture rather than re-derived from a producer. #[test] fn error_frame_shape_is_pinned() { - let frame = GetOutcome::Err { + let inner = GetOutcome::Err { error: ErrorBody { code: ReadError::NotFound, class: ErrorClass::Permanent, }, }; - let got: serde_json::Value = - serde_json::to_value(&frame).expect("serialize the error outcome"); + let inner_value = serde_json::to_value(&inner).expect("serialize the error outcome"); + let got = serde_json::json!({ "result": inner_value }); // ORDER IS LOAD-BEARING, and this is the second version. Written with the - // equality first, the specific check below never ran: `assert_eq!` panics on any + // equality first, the specific checks below never ran: `assert_eq!` panics on any // difference, so dropping `class` reported "the frame shape drifted" and left the - // reader to diff two blobs. The diagnostic existed only for the case it could not - // reach. A cheap, specific assertion must precede a broad one that subsumes it, - // or it is decoration. + // reader to diff two blobs. The diagnostics existed only for the cases they could + // not reach. Cheap, specific assertions must precede a broad one that subsumes + // them, or they are decoration. + assert!( + got.get("result").is_some(), + "the outer `result` wrapper vanished — every route reply in `handle_read_request` \ + is wrapped in `{{\"result\": ...}}`, so a consumer that decodes straight into \ + the inner shape would start receiving a different envelope than the one their \ + fixture pinned" + ); + assert!( + got["result"].get("error").is_some(), + "the `error` body vanished from inside the wrapper — consumers route on this key" + ); assert!( - got["error"].get("class").is_some(), + got["result"]["error"].get("class").is_some(), "`class` vanished from the error body — the contract's branch-on-class rule \ becomes unfollowable and consumers silently fall back to branching on `code`" ); + // The full on-wire frame, written as a single JSON literal so it is quotable + // verbatim into a consumer's fixture. Keys and order are part of the contract — + // serde serializes structs in field-declaration order, so `error`/`code`/`class` + // appear in the order written here, and the route builder adds `result` last. + // Renaming any of these keys, reordering them, or nesting deeper than this is a + // contract change, not a refactor. let want = serde_json::json!({ - "error": { "code": "not_found", "class": "permanent" } + "result": { + "error": { + "code": "not_found", + "class": "permanent" + } + } }); assert_eq!( got, want, - "the error frame shape drifted — consumers branch on these exact keys" + "the error frame shape drifted — consumers route on the outer `result` key, \ + branch on the inner `class`, and pin both. The whole frame is the contract." ); } From b8474c3ed41acb66f8e07df3cbaec4769fd9f057 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Sat, 29 Aug 2026 18:09:30 +0200 Subject: [PATCH 2/2] status: pin the on-wire BYTES, not only the frame shape The shape pin compares `serde_json::Value`s, so it is order-independent. That is the right guarantee for a shape pin -- a key moving should not turn it red -- but it means the test is green for either field order, and a consumer holding a byte-string fixture is not covered by it. The wire order is not this struct's declaration order. `ErrorBody` declares `code` then `class`; the wire emits `class` then `code`, because the reply is built through a `serde_json::Value` and `serde_json::Map` is a `BTreeMap` unless the `preserve_order` feature is on, so keys ship alphabetically. Confirmed against the running daemon from both sides of the wire, and reproduced in isolation: the same struct serialized directly yields `code` first. So the current byte order is ACCIDENTAL. It holds only while the reply goes through a `Value`; serializing the struct straight to bytes would flip it with nothing to notice. This assertion converts that accident into a decision someone has to make deliberately. Found the way these things get found. The first consumer of this surface was handed a canonical literal to quote verbatim into their fixture. It was transcribed from the struct declaration rather than read off the wire, so it did not match production -- and the shape pin could not have caught that, because it was written not to. Deserialization does not care about key order; a byte-comparing fixture or a frame digest does. The consumer caught it on their first live probe, from a different process, against a claim I had just published. Mutation-checked: flipping the pinned order turns it red naming the byte change specifically, and the mutation script asserts its target text is present before editing, because the previous attempt silently matched nothing and reported a pass. Gate: exit 0, nine real-daemon e2e arms executing. --- crates/credentials-module/src/read_surface.rs | 35 +++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/crates/credentials-module/src/read_surface.rs b/crates/credentials-module/src/read_surface.rs index 6aefcae..50aaad8 100644 --- a/crates/credentials-module/src/read_surface.rs +++ b/crates/credentials-module/src/read_surface.rs @@ -1570,8 +1570,8 @@ mod error_class_tests { let want = serde_json::json!({ "result": { "error": { - "code": "not_found", - "class": "permanent" + "class": "permanent", + "code": "not_found" } } }); @@ -1581,6 +1581,37 @@ mod error_class_tests { "the error frame shape drifted — consumers route on the outer `result` key, \ branch on the inner `class`, and pin both. The whole frame is the contract." ); + + // THE BYTES, pinned separately, because the assertion above cannot see them. + // + // `Value` equality is order-independent, which is correct for a SHAPE pin — a key + // moving should not turn this red. But it means the test above is green for either + // field order, and a consumer holding a byte-string fixture is not covered by it. + // + // The wire order is NOT this struct's declaration order. `ErrorBody` declares + // `code` then `class`; the wire emits `class` then `code`, because the reply is + // built through a `serde_json::Value` and `serde_json::Map` is a `BTreeMap` unless + // the `preserve_order` feature is on — so keys ship alphabetically. Verified + // against the running daemon from both sides of the wire, and reproduced in + // isolation: the same struct serialized directly yields `code` first. + // + // That makes the current byte order ACCIDENTAL — it holds only while the reply + // goes through a `Value`. Serializing the struct straight to bytes would flip it + // with nothing to notice, so this assertion is what converts the accident into a + // decision someone has to make deliberately. + // + // WHY IT IS WORTH A TEST AT ALL: the first consumer of this surface was handed a + // literal transcribed from the struct declaration and told to quote it verbatim. + // It did not match production. Deserialization did not care; a byte-comparing + // fixture or a frame digest would have. The canonical bytes now live where someone + // reading this test will copy the right ones. + assert_eq!( + serde_json::to_string(&got).expect("serialize the pinned frame"), + r#"{"result":{"error":{"class":"permanent","code":"not_found"}}}"#, + "the on-wire BYTES changed. Deserializing consumers are unaffected; any \ + consumer holding a byte-string fixture or hashing a frame is not. If this is \ + intentional, the canonical literal published to consumers has to move with it." + ); } /// An UNMAPPED store error degrades to a TRANSIENT code, never a permanent one.