fix(#6502): frame and sanitize validation feedback in agent prompt - #6510
Conversation
buildFeedbackPrompt injected validation output into the agent prompt as undelimited prose with no unicode sanitization. A prompt injection payload or obfuscation sequence in the validator output would arrive in the next iteration's user turn unmarked, at the same privilege level as the runner's own instructions. Three changes: 1. Fence the feedback inside <validation-output> tags with a preamble instructing the model to treat the enclosed content as data, not as instructions. Occurrences of the closing delimiter inside the feedback are escaped to prevent breakout. 2. Run the feedback through the existing Go UnicodeNormalizer (the same character classes as the PostToolUse hook chain's scan_text) to strip tag characters, bidi overrides, zero-width characters, null bytes, and ANSI/OSC escape sequences before they enter the prompt. This closes the gap where the sandbox hook path never sees runner-assembled prompt text. 3. Log when sanitization alters the feedback so a validator emitting escape sequences is visible in the run log rather than silently swallowed. Tests cover data framing, delimiter escape, unicode sanitization (dangerous chars stripped, CJK/accented text preserved), the all-sanitized-empty case, and iteration-1 byte-identity with DefaultAgentPrompt. Closes #6502
|
🤖 Review · Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
`security.NewUnicodeNormalizer().Scan` applies NFKC, so any validator output carrying fullwidth punctuation, a ligature or a vulgar fraction came back rewritten — `検証エラー：file ½` became `検証エラー:file 1⁄2` — and every such run raised a spurious "sanitization altered validation feedback" warning. Validation feedback routinely quotes file content the agent then edits, so handing it a normalized copy invites the agent to write the normalized form back. The PostToolUse chain made the same call for tool results (#6467): NFKC is used for detection, never for rewriting. Mirror it here — when the only finding is the compatibility class, keep the original bytes and report nothing. Mixed input (a zero-width, bidi, tag character, NUL or escape alongside compatibility text) still takes the sanitized copy. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Approving with one fix pushed on top (3ddf7d5b, fast-forward — your commit is untouched).
The change is right
Framing, delimiter escaping, the empty-after-sanitizing branch and the StepWarn when sanitizing fires are all what #6502 asked for, and reusing security.NewUnicodeNormalizer() is the correct instinct: one character-class policy for both the hook chain and the runner, no python dependency on the runner path. Six new tests, all passing, plus the full internal/cli package.
What I fixed
UnicodeNormalizer.Scan applies NFKC (internal/security/unicode.go:217), so any validator output containing compatibility characters came back rewritten. Reproduced:
in: 検証エラー：file 「設定」 が不正です ½ ｱｲｳ
out: 検証エラー:file 「設定」 が不正です 1⁄2 アイウ findings=1 ("fullwidth")
Two consequences: the agent is shown text that differs from what the validator produced — and validation feedback routinely quotes file content the agent then edits, so it can write the normalized form back into the repo — and every run with any non-ASCII output raised a spurious "sanitization altered validation feedback" warning, which trains people to ignore a warning that is supposed to mean something.
This is the same call the PostToolUse chain already made for tool results in #6467: NFKC is used for detection, never for rewriting, precisely because compatibility characters are content. sanitizeFeedbackUnicode now mirrors that — when the only finding is the compatibility class, the original bytes are kept and nothing is reported; mixed input (zero-width, bidi, tag character, NUL or escape alongside compatibility text) still takes the sanitized copy, which carries NFKC folding as the accepted cost of using this normalizer. Two regression tests cover both directions.
I also corrected the doc comment, which claimed parity with the hook chain — accurate now, not before.
Note for the release
This should land before the tag: fullsend-ai/agents#948 turns on feedback_mode: append for code and fix, and release.yml tags the agents repo with the same version, so the two would otherwise ship together with this path unframed.
|
🤖 Finished Review · ✅ Success · Started 1:35 AM UTC · Completed 1:46 AM UTC Commit: |
|
Review skipped — this PR is already merged. The Posted by fullsend post-review check |
|
🤖 Finished Retro · ✅ Success · Started 1:47 AM UTC · Completed 2:03 AM UTC Commit: |
Retro: PR #6510 — frame and sanitize validation feedbackOverall assessment: This was a well-executed workflow with fast turnaround (~24 min from PR creation to merge). The code agent produced a largely correct implementation on its first iteration, but a nuanced design decision about NFKC normalization required a human fix commit. No new proposals — the main improvement opportunity is already tracked. Timeline
What the human caught that the code agent missedThe issue body explicitly said to sanitize "matching the PostToolUse chain's character classes" and the dispatch comment referenced PR #6467 as the precedent. The code agent read both the Python PostToolUse chain and the Go Evidence for existing issues (no new proposals needed)
Review agent findings (post-merge, advisory)The review agent's 4 findings arrived after merge (review completed ~1 min after merge). Of note: the medium finding (delimiter-escape-gap — open delimiter not escaped, only close delimiter escaped) is a valid defense-in-depth observation that wasn't addressed before merge. The low finding about fullwidth characters returning original text was essentially flagging the intentional fix by waynesun09, making it a false positive in context. Agents repoAgent definitions resolved from |
1 participant
Summary
Hardens
buildFeedbackPromptagainst prompt injection and unicode obfuscation by fencing validation output inside<validation-output>tags with a "treat as data" preamble, sanitizing dangerous unicode characters (tag chars, bidi overrides, zero-width chars, ANSI/OSC escapes) via the existing GoUnicodeNormalizer, and logging when sanitization alters feedback.Changes
sanitizeFeedbackUnicodehelper that runs feedback throughsecurity.NewUnicodeNormalizer().Scan()— the same character classes the PostToolUse hook chain applies to tool resultsbuildFeedbackPromptto return(string, int)— the prompt and the number of sanitization findings<validation-output>/</validation-output>tags with a preamble instructing the model to treat the content as datarunAgentto log a warning when sanitization alters feedbacksanitizeFeedbackUnicodeunit testsTesting
TestBuildFeedbackPrompt_*andTestSanitizeFeedbackUnicodetests passbuildFeedbackPromptandsanitizeFeedbackUnicodego vetandgofmtcleanCloses #6502
Post-script verification
agent/6502-frame-sanitize-feedback)d31168a0ce5f0d0f2c10dbbb8ab9d78c8b066167..HEAD)