feat(rskim-contract): add OutcomeReason + token fields to DecisionRecord (#342)#349
Open
dean0x wants to merge 6 commits into
Open
feat(rskim-contract): add OutcomeReason + token fields to DecisionRecord (#342)#349dean0x wants to merge 6 commits into
dean0x wants to merge 6 commits into
Conversation
…onRecord (#342) Adds the shared schema coordination step required by ADR-004 between #301 (this crate, schema owner) and #305 (persistence), unblocking #304's full 5→3 reason mapping in BlockRouter. Changes: - New OutcomeReason enum (Full/Degraded/Passthrough/FailedOpen/PolicyPassthrough) with #[non_exhaustive] + serde snake_case, cited to #342 + #305 in rustdoc - DecisionRecord gains `reason: OutcomeReason`, `tokens_in: Option<u64>`, `tokens_out: Option<u64>` (token fields skip_serializing_if=None) - Existing constructors stay backward-compatible: passthrough()→Passthrough, modified()→Full; no call-site changes required anywhere - New constructors: passthrough_with_reason(), modified_with_reason(), with_tokens() (builder-style, #[must_use]) - OutcomeReason re-exported from crate root alongside DecisionRecord - All 164 harness+proptest tests pass; clippy clean; fmt clean; Snyk clean Co-Authored-By: Claude <noreply@anthropic.com>
…solidate duplicates - outcome_reason_variants_are_distinct: replace 10 pairwise assert_ne! calls with compile-time array construction — a deleted variant causes a compile error, making the runtime checks redundant language-behavior tests. - token_fields_json_round_trip: drop the without-tokens half; that case is already asserted in decision_record_to_json_round_trips. - Remove existing_passthrough_ctor_reason_is_passthrough and existing_modified_ctor_reason_is_full; their assertions are exact duplicates of what decision_record_passthrough_fields and decision_record_modified_fields already assert. The additive-guarantee rationale is preserved as a comment in the base tests. - Remove restatement-only #342: prefixed comments inside assertion blocks. All 131 tests pass, no behavior changed.
Add discriminating #[should_panic] tests for the OutcomeReason family guards in DecisionRecord::passthrough_with_reason / modified_with_reason (#342). The debug_assert! family guards prevent a caller from pairing a wire Decision with a mismatched OutcomeReason (e.g. Decision::Modified + FailedOpen), which would silently violate the binding 5->3 mapping (the PF-006 silent-wrong-path class). They previously had no test: deleting either guard would still compile and pass the suite (PF-007 vacuous-coverage gap). The two new tests fail if either guard is removed. cargo nextest -p rskim-contract --all-features: green cargo clippy -p rskim-contract --all-targets --all-features -D warnings: clean cargo fmt -p rskim-contract --check: clean
…rt!, usize tokens, AC10, typed round-trips
Five confirmed review issues closed (ADR-001 / PF-002: surface and fix now):
1. Backward-/forward-compatible deserialization [medium x2]:
- Add `Default` impl for `OutcomeReason` (→ `Passthrough`), matching the
passthrough-default semantics documented in the rustdoc.
- Add `#[serde(default)]` on `DecisionRecord::reason` so pre-#342 records
(no `reason` key) deserialize successfully rather than failing with
`missing field "reason"`. Mirrors the already-resilient `tokens_in`/`tokens_out`
pattern. Asymmetry with sibling optional fields is now closed.
2. Family-consistency invariants in release builds [low]:
- Promote `debug_assert!` → `assert!` in `passthrough_with_reason` and
`modified_with_reason`. These are public module-boundary constructors; a
mismatched (Decision, OutcomeReason) pair produces a silently-inconsistent
record violating the 5→3 wire-vocabulary mapping (PF-006 silent-wrong-path
class). `assert!` fires in both debug and release builds (rust.md: assert!
at module boundaries).
3. PF-007 typed round-trip tests [medium]:
- Add five new discriminating tests: `legacy_record_deserializes_without_reason_or_tokens`
(the `#[serde(default)]` guard — deleting the attribute makes this fail),
`passthrough_record_typed_round_trip`, `modified_with_reason_degraded_typed_round_trip`,
`with_tokens_typed_round_trip`, and `outcome_reason_typed_deserialize_all_variants`.
- Use `Box::leak` / `static` literals to satisfy `Deserialize<'static>` (imposed
by `component: &'static str`). The `from_value` path was attempted first but
also requires `DeserializeOwned` + `'static` for the same reason.
4. Token-count type drift [medium]:
- Change `tokens_in`/`tokens_out` from `Option<u64>` to `Option<usize>` and
`with_tokens(u64, u64)` → `with_tokens(usize, usize)` to match
`rskim_tokens::Counter::count(&str) -> usize` and the `token_counter` closure
type documented in 304-plan.md:108,118. Eliminates the avoidable `as u64`
cast at every #304 call site; keeps consistency with sibling `bytes_in`/`bytes_out`
(also `usize`).
5. Rustdoc AC citation [low]:
- Change three "AC15" citations on token-field docs to "AC10" (the correct
AC mandating token counter exclusion from the gate, per 304-plan.md:288).
AC15 is the unrelated live-zone passthrough byte-identity criterion.
138/138 nextest + 14/14 doctests pass. clippy -p rskim-contract --all-targets -D warnings clean.
…y + doc correction Two review issues from #342: 1. [critical] is_sensitive_key (log.rs:128) used string slicing `key[key.len() - s.len()..]` for suffix matching. When `key` contains a multibyte UTF-8 code point and the byte offset lands inside that code point, Rust panics with 'byte index N is not a char boundary'. Fix: compare on byte slices via `key.as_bytes()` / `s.as_bytes()` — all suffix constants are pure ASCII so `eq_ignore_ascii_case` on byte slices is semantically identical but panic-free. Adds discriminating regression test `is_sensitive_key_multibyte_utf8_no_panic`. 2. [low] Rustdoc on `DecisionRecord::reason` attributed deserialization absence-tolerance to `skip_serializing_if`, which only governs the serialize direction. Corrected to explain that serde's built-in `Option` defaulting handles the deserialize direction for `tokens_in`/`tokens_out`, while `#[serde(default)]` explicitly handles it for the non-Option `reason` field. 142/142 tests pass; clippy -D warnings clean. Co-Authored-By: Claude <noreply@anthropic.com>
…unctions `cargo fmt -p rskim-contract -- --check` failed on Box::leak chains in passthrough_record_typed_round_trip, modified_with_reason_degraded_typed_round_trip, and with_tokens_typed_round_trip. rustfmt breaks the chained .expect() and .into_boxed_str() calls onto separate indented lines. Auto-applied by `cargo fmt -p rskim-contract`. 139/139 tests pass; clippy -D warnings clean. Refs #342 Co-Authored-By: Claude <noreply@anthropic.com>
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
1 participant
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.
Summary
OutcomeReasonenum (5 values:Full/Degraded/Passthrough/FailedOpen/PolicyPassthrough) torskim-contract, implementing the D2 decision from DECISIONS-NEEDED.mdreason: OutcomeReason+ optionaltokens_in/tokens_outfields toDecisionRecordas the shared schema coordination step between L3: Safety invariant contract & fail-open guardrail module #301 (schema owner) and L3: Proxy observability — analytics schema v4, per-provider/model savings accounting #305 (persistence), per ADR-004BlockRouterfrom authoring reason-level assertions (the "BLOCKED-PORTION" notes in AC3/AC19 of the 304 plan are now resolved)Changes
crates/rskim-contract/src/log.rs:OutcomeReasonenum —#[non_exhaustive], serde snake_case, full rustdoc citing L3: add reason + token fields to rskim_contract::DecisionRecord (schema coordination #301/#305) #342 + L3: Proxy observability — analytics schema v4, per-provider/model savings accounting #305 dependencyDecisionRecordgainsreason: OutcomeReason,tokens_in: Option<u64>,tokens_out: Option<u64>(token fields useskip_serializing_if = "Option::is_none")passthrough_with_reason(),modified_with_reason(),with_tokens()(builder-style#[must_use])passthrough()andmodified()stay signature-identical; they now setreason = Passthroughandreason = Fullrespectively — zero call-site changes requiredwith_unsanitized_request_id()(harness-only) derives reason from decision variantcrates/rskim-contract/src/lib.rs:OutcomeReasonfrom crate rootBreaking Changes
None. Additive only — all existing call sites compile unchanged. The
Decisionwire enum remains 2-variant (Modified/Passthrough);reasonis an additional JSON field.Reviewer Focus Areas
OutcomeReasonenum variants and their 5→3 mapping documentation (log.rs:198–260) — these must match the binding table in 304-plan.md §3passthrough_with_reason/modified_with_reasondebug assertions: passthrough-family variants must not be passed tomodified_with_reasonand vice versatokens_in/tokens_outare accounting-only: they must never appear in any accept/reject gate (AC15 in the 304 plan). Thewith_tokens()builder is the only way to set them, and it is called AFTER the byte gate, never beforeexisting_passthrough_ctor_reason_is_passthroughandexisting_modified_ctor_reason_is_fullpin the default-reason behavior so a future refactor can't silently change the defaultsTest Plan
cargo nextest run -p rskim-contract --features harness -j 4 --no-fail-fast→ 164/164 PASScargo test -p rskim-contract --doc→ 14/14 PASScargo clippy -p rskim-contract --all-targets -- -D warnings→ 0 warningscargo fmt -p rskim-contract -- --check→ cleanCloses #342