Secret hardening for HITL surfaces: serve-time re-redaction + raw-carrier strip - #679
Merged
Merged
Conversation
…action + raw-carrier strip The filter fix (943cd11) closed pattern gaps; this closes the architecture gaps that let a stale or missed redaction reach a human: - Serve-time re-redaction everywhere pending-call arguments leave the server: argumentsRedacted is computed once, at pause time, with the filter of that day - a pause stored before a filter improvement kept serving its old, leaky redaction forever. The approval-status summary, the detail=full snapshot, the MCP mirror and the Slack approval card now re-run SecretRedactionFilter over every served string. - The raw carriers never leave the server on the approver surface: detail=full returned the whole snapshot with only fingerprints stripped; argumentsRaw, chatTranscriptJson and traceSoFar rode along, each carrying the raw arguments the redaction beside them masked. sanitizePendingToolCallsForApprover strips all three (persisted document untouched; resume unaffected). - The tool trace records redacted arguments AND results from the start: the trace feeds task summaries, SSE, memory and the chat activity list. Execution and the model's own view keep the raw values. Six new tests incl. an end-to-end orchestrator pin that a credential embedded in tool arguments never survives into the trace.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…cret
${vault:...} is a POINTER to a secret, and the correct alternative to
writing one down. The key name it carries is ordinary configuration an
admin reads in the agent document anyway. Masking it cost real
information and bought nothing:
- on an approval card it hid WHICH credential a request uses, which is
exactly what an approver needs to judge it;
- it made every correct, vault-referencing request display a <REDACTED>
marker, training approvers to read that marker as normal - and that
marker is precisely the signal the Manager uses to warn that a request
embedded a secret LITERAL. A control that fires on the safe case is a
control people learn to ignore.
A resolved secret does not look like a vault reference (it is the raw
value, caught by the remaining rules), so nothing is weakened. The generic
key=value rule cannot match one either - its value class excludes braces.
Three tests pin that references stay legible: plain, inside JSON, and the
legacy ${eddivault:...} spelling.
pull Bot
pushed a commit
to Stars1233/EDDI
that referenced
this pull request
Aug 17, 2026
… live guard A four-reviewer audit of labsai#679-labsai#689 plus an end-to-end operator-path trace, then a second adversarial round over this fix itself. Everything confirmed is addressed here; docs/changelog.md carries the full account. - Cross-version placeholder stranding: dropPendingApprovalPlaceholder now recognises the previous builds' default wordings (legacy constant, suffix-less tool-named), so the first post-upgrade resume of an in-flight pause no longer renders [stale placeholder, answer]. Two upgrade-boundary tests simulate a pre-upgrade pause. - Self-conversation guard is now enforced on the LIVE path, including the mixed-batch pause branch the second review round caught (ungated calls execute before the pause is thrown and are never rechecked). Shared core extracted; same NOT_EXECUTED envelope and trace everywhere. - labsai#684 contract narrowed: failed results stay out of ApiCallsTask's cross-call template merge and out of the RAG system prompt; error bodies (and the status-message fallback) are redacted before reaching the model. Memory-side *Error keys unchanged. - Test-drive read-back: a blank returningFields entry means NO filter - [""] no longer nulls steps/outputs/properties out of the snapshot. - Generated tools: body $refs resolve one level (schemas namespace only), so descriptions name real fields - a guessed say-body bound to InputData defaults and silently sent an empty message; enum values and defaults now reach parameter descriptions (the environment typo->production trap). - padDataLines normalises bare CR; RFC 7615 headers join the credential response deny-list; labsai#688's shared-path stripping disclosed in changelog. 264 tests across the affected suites, including mutation-informed pins: same-tool ordinal drop, refused-mid-batch pairing, blank-filter recovery, redaction survival of the failure reason.
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.
Follow-up to the redaction-filter fix (943cd11) on this base branch — the filter fix closed the pattern gaps; this closes the architecture gaps that let a stale or missed redaction reach a human. Targets
chore/remove-agent-father(#672) since all HITL machinery lives there.Serve-time re-redaction
argumentsRedactedis computed once, at pause time, with whatever filter version existed then — a pause stored before a filter improvement kept serving its old, leaky redaction forever (exactly the observedsk-ant-…case). Every surface where pending-call arguments leave the server now re-runsSecretRedactionFilterat serve time: the approval-status summary (arguments + full request preview: uri, body, query params, headers), thedetail=fullsnapshot, the MCP mirror, and the Slack approval card.Raw carriers stripped from the approver surface
approval-status?detail=fullreturned the whole snapshot with only fingerprints stripped —argumentsRaw(unredacted by definition), the frozen LLM transcript (chatTranscriptJson) and the running trace (traceSoFar) rode along, each carrying the raw arguments the redaction beside them had masked. NewsanitizePendingToolCallsForApproverstrips all three and subsumes the fingerprint marker contract. The persisted document is untouched; resume reads from the store, not this view.The tool trace records redacted values from the start
ToolLoopRunnerstored the model's raw arguments (and raw tool results) in the trace — the same trace that feeds task summaries, SSEtask_complete, memory, and the chat activity list. Both now pass through the filter at record time. Execution and the model's own view keep the raw values — only the display record is filtered.Tests
Six new: five on the approver sanitizer (raw-carrier strip, stale-redaction re-redaction using an underscored key only the current filter catches, all four preview surfaces, fingerprint marker contract, null-safety at every level) and one end-to-end orchestrator test pinning that a credential embedded in tool arguments never survives into the trace.
The Manager-side counterpart (an
inlineCredentialescalation flag on the approval card) is a separate PR on EDDI-Manager.🤖 Generated with Claude Code
CI note
This PR targets
chore/remove-agent-father, andci.ymlonly triggers for PRs againstmain— so no CI runs here (only CodeRabbit registers, skipped for non-main bases). Validation instead: full compile plus 555 tests across every affected area (secrets, memory utilities, orchestrator/tool loop, streaming, Slack HITL) run green locally in an isolated worktree. Full CI covers this content once it lands on the #672 branch, which is a PR to main.