test(daemon): prove the protected preview stays redacted - #1815
Conversation
`tracedecay_configuration_protected_preview` is the dry run a host calls before it commits a protected setting, and nothing covered its host-facing contract end to end. Drive the tool through MCP `tools/call` on the production composition and assert what a caller actually receives: a plan bound to the supplied revision, the redacted per-setting digests, the operation digest echoed as the preview digest, the five-minute validity window, and a preview id the host can apply. The rule identity and the denied capability must never appear in the rendered answer, a stale revision must be a typed `configuration.conflict`, an empty capability set a typed `configuration.invalid_request`, and no preview may advance the configuration revision. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0d26c7050
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let unbind = ProtectedChange::UnbindSource { | ||
| binding_id: SourceBindingId::new(ABSENT_BINDING_ID).expect("binding identity"), |
There was a problem hiding this comment.
This constructs an UnbindSource for an explicitly absent binding and then requires the preview to succeed. Such a plan can never be applied: ConfigurationSnapshotV1::apply_protected_change returns ProtectedChangeSnapshotError::Stale when the binding is absent, while introducing the binding first would change the pinned base revision. The test therefore blesses a non-actionable preview rather than exercising a real second protected-change journey; seed a binding before previewing its removal, or assert that this request is refused.
AGENTS.md reference: AGENTS.md:L9-L12
Useful? React with 👍 / 👎.
| "access_rule_upsert", | ||
| before_digest.as_str(), | ||
| &access_digest, | ||
| &[ACCESS_RULE_ID, DENIED_CAPABILITY], |
There was a problem hiding this comment.
The submitted access rule embeds project_id in its AuthorityRef, but the leak check only searches for the rule ID and denied capability. A response that accidentally exposed the target project identity anywhere outside the exactly compared redacted_changes object would still pass, despite the ProtectedChangePlan contract explicitly forbidding target identities. Include project_id.as_str() among the hidden values so this journey actually covers the full sensitive input it sends.
AGENTS.md reference: AGENTS.md:L9-L12
Useful? React with 👍 / 👎.
1 participant
Supersedes #1615 (the journey-test half of it). #1615 is left open for the owner to close.
What this is
tracedecay_configuration_protected_previewis the dry run a host calls before it commits a protected configuration change, and nothing covered its host-facing contract end to end. This adds one journey test that drives the tool through MCPtools/callon the production project composition and asserts what a caller actually receives:base_revision_id);redacted_changescarries only the setting key, the operation, and the before/after digests — never the rule identity or the denied capability;preview_idis theplan_idthe host later applies;expires_at - created_at);UpsertAccessRule,UnbindSource) produce two different digests, so the digest is evidence rather than a constant;expected_revisionis a typedconfiguration.conflictwithretry: after_revalidate, not a committed setting;configuration.invalid_requestwithretry: never;What this is not
#1615 also carried an unexplained production change in
code_index_scheduler/serving.rs, test-authority grafts in two scheduler tests, two assertion changes weaker than master's, and byte-identical fixture churn. None of that is here.The
serving.rschange was evaluated separately and rejected: it re-collapses a text-build admission refusal intoRetrievalPortError::BudgetExceeded, which is exactly what commit 0ae9fad ("shrink text builds to available memory headroom", #1782) deliberately undid — "Reservation failures now retain their typed detail instead of collapsing into BudgetExceeded." Its stated rationale (thatAuthorityUnavailablemakes the successor never retry) does not hold on master:advance_text_serving_with_controlmarks text serving failed only forCapabilityManifestRejected | GenerationMismatch | IncompatibleProjection | Contract, so both variants are retried identically, while at the executorBudgetExceededmaps to a terminalTimedOut/InternalandAuthorityUnavailablemaps to the reason the code documents as the retryable one. The reclassification would therefore make transient reservation pressure look less retryable, not more, and it drops the byte-level diagnostic #1782 added.Verification
Run in a clean worktree on
origin/masterwith its ownCARGO_TARGET_DIR:cargo test -p tracedecay --features tracedecay/test-helpers --lib -- configuration_protected_previewcargo clippy --workspace --all-targets --locked -- -D warningscargo fmt --all -- --checknode scripts/lint-commit-range.mjs origin/master HEAD🤖 Generated with Claude Code