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..50aaad8 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,115 @@ 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["error"].get("class").is_some(), + 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["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": { + "class": "permanent", + "code": "not_found" + } + } }); 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." + ); + + // 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." ); }