fix: strip pasted code/diffs from user-prompt + tag tool-action metadata#34
Open
24601 wants to merge 5 commits into
Open
fix: strip pasted code/diffs from user-prompt + tag tool-action metadata#3424601 wants to merge 5 commits into
24601 wants to merge 5 commits into
Conversation
Stops the server-side fact extractor from reading assistant-authored
content as user prose. Two related issues:
1. user-prompt.ts queues the entire `prompt` for upload under userPeer.
Pastes (fenced code blocks, runs of unified-diff lines, long
path-bearing output) ride `role: "user"` per the Anthropic Messages
API but are not user-authored prose. The extractor reads them as
if they were and produces "<user> changed/wrote/added X" facts
from a pasted diff in an adversarial-review prompt.
2. post-tool-use.ts uploads aiPeer messages with no role discriminator.
In directional observationMode (and any deployment where aiPeer
cross-observes the user), these get folded into the user-peer's
representation as "<user> did X" — even though aiPeer authored
the action. unified mode reduces the blast radius but does not
eliminate it (MCP tools and queries can still operate in
directional scope).
Changes:
- src/hooks/user-prompt.ts: stripPastes() heuristic over fenced code
(```...```), 3+ consecutive unified-diff lines (^[+-] at line start),
and >200-char path-bearing lines. When anything is redacted, tag
the queued message with metadata.type = "user_paste_not_speech".
- src/hooks/post-tool-use.ts: aiPeer.message metadata now carries
type: "tool_action" and subject: "ai_action_on_user_behalf" so
server-side extraction can filter cross-peer attribution.
- src/cache.ts: QueuedMessage gains optional metadata field;
queueMessage() accepts an optional 5th metadata argument.
Backwards compatible — existing callers and queued messages
without metadata continue to work.
- src/hooks/session-end.ts: forward queued-message metadata into
userPeer.message uploads at session-end so server-side extraction
sees the role tags.
- plugins/honcho/scripts/test-strip-pastes.ts: standalone Bun script
exercising stripPastes() against 8 cases including the realistic
adversarial-review pattern that produced the original
"<user> changed buildOperatorPlan" misattribution. Run with:
bun run scripts/test-strip-pastes.ts
Repo currently has no test runner; this is a pragmatic regression
check intended to be rerun manually before releases that touch
user-prompt.ts.
- CHANGELOG.md: [Unreleased] entry.
Verified locally:
bunx tsc --noEmit -p tsconfig.json # clean
bun run scripts/test-strip-pastes.ts # 8/8 pass
WalkthroughClient-side prompt sanitization redacts fenced code, anchored unified-diff regions, and long path-like lines before queuing; redacted messages are tagged via message metadata. Queued messages may carry optional metadata and that metadata is forwarded at session end. Tool-use messages are annotated as AI-performed actions. Changes
Sequence DiagramsequenceDiagram
participant User
participant UserPromptHook as "User Prompt Hook"
participant Preprocessor as "stripPastes()"
participant Queue as "Message Queue (cache.ts)"
participant SessionEnd as "Session End Hook"
participant UserPeer as "User Peer"
participant Server
User->>UserPromptHook: submit prompt
UserPromptHook->>Preprocessor: sanitize (fenced code / diffs / paths)
Preprocessor-->>UserPromptHook: sanitized text + redacted flag
UserPromptHook->>Queue: queueMessage(content, metadata?)
Queue-->>UserPromptHook: stored (includes metadata)
Note over SessionEnd,Queue: at session end
SessionEnd->>Queue: retrieve queued messages
Queue-->>SessionEnd: messages with metadata
SessionEnd->>UserPeer: userPeer.message(chunks, metadata)
UserPeer->>Server: upload messages + metadata
Server->>Server: extract/filter/attribute based on metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/honcho/src/hooks/session-end.ts (1)
259-265: Spread order lets queued metadata clobberinstance_id/session_affinity.
...(msg.metadata || {})is applied after the explicit fields, so any future caller that happens to putinstance_idorsession_affinityinto the queued metadata would silently override the per-upload values computed here. Today onlytypeis stored, so no live bug — but inverting the order is cheap insurance and matches the intent ("queued metadata is additional, not authoritative").♻️ Suggested change
metadata: { + ...(msg.metadata || {}), instance_id: msg.instanceId || undefined, session_affinity: sessionName, - // Forward queued-message metadata (e.g. type: "user_paste_not_speech") - // so server-side extraction can filter pastes out of user attribution. - ...(msg.metadata || {}), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/honcho/src/hooks/session-end.ts` around lines 259 - 265, The metadata spread currently places ...(msg.metadata || {}) after the explicit fields so queued metadata can overwrite instance_id/session_affinity; change the order so ...(msg.metadata || {}) is spread first and then add instance_id: msg.instanceId || undefined and session_affinity: sessionName afterwards (referencing the metadata object construction, msg.metadata, msg.instanceId, sessionName, instance_id, and session_affinity) so the per-upload values always take precedence.plugins/honcho/scripts/test-strip-pastes.ts (1)
52-109: Add a markdown-bullet-list case to lock down the diff false-positive.The current cases miss the most likely real-world false positive for the
(?:^[+\-]…){3,}rule. Once the regex inuser-prompt.tsis tightened (see comment on that file), this case should pin the behavior so it doesn't regress.♻️ Suggested addition
{ name: "only a single +/- line (not a runs-of-3+ diff) — must NOT redact", input: "Note: + means added, - means removed.", expectRedacted: false, }, + { + name: "markdown bullet list (3+ items) — must NOT redact", + input: "Things to consider:\n- auth flow\n- rate limiting\n- error handling\nThoughts?", + expectRedacted: false, + mustContain: ["auth flow", "rate limiting", "error handling"], + mustNotContain: ["[diff removed]"], + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/honcho/scripts/test-strip-pastes.ts` around lines 52 - 109, Add a new test entry to the existing cases array (Case[]) to prevent the diff-detection regex from false-positive on markdown bullet lists: create a case named like "markdown bullet list — must NOT redact" with input containing multiple markdown bullets that start with "+" and "-" (e.g. "- item\n+ added item\n- removed item\n- another item") and set expectRedacted: false (no mustContain/mustNotContain needed or optionally ensure it does NOT contain "[diff removed]" or "[code block removed]"). Place this new object alongside the other cases in the cases array to lock down the intended behavior of the diff-detection logic used by the code that inspects user prompts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/honcho/src/hooks/user-prompt.ts`:
- Around line 91-95: The current diff-removal regex on variable out
(/^(?:^[+\-].*(?:\r?\n|$)){3,}/gm) falsely strips markdown bullet lists; change
the logic in the user-prompt hook to only run the 3+ +/- line replacement when
the text actually looks like a unified diff: compute looksLikeUnifiedDiff with
/^(?:@@|---\s+a\/|\+\+\+\s+b\/)/m.test(out) and wrap the out.replace(...) call
inside an if (looksLikeUnifiedDiff) block so regular bullet lists are preserved,
keep setting redacted = true when you do redact, and add a test case to
scripts/test-strip-pastes.ts covering the bullet-list scenario to prevent
regressions.
---
Nitpick comments:
In `@plugins/honcho/scripts/test-strip-pastes.ts`:
- Around line 52-109: Add a new test entry to the existing cases array (Case[])
to prevent the diff-detection regex from false-positive on markdown bullet
lists: create a case named like "markdown bullet list — must NOT redact" with
input containing multiple markdown bullets that start with "+" and "-" (e.g. "-
item\n+ added item\n- removed item\n- another item") and set expectRedacted:
false (no mustContain/mustNotContain needed or optionally ensure it does NOT
contain "[diff removed]" or "[code block removed]"). Place this new object
alongside the other cases in the cases array to lock down the intended behavior
of the diff-detection logic used by the code that inspects user prompts.
In `@plugins/honcho/src/hooks/session-end.ts`:
- Around line 259-265: The metadata spread currently places ...(msg.metadata ||
{}) after the explicit fields so queued metadata can overwrite
instance_id/session_affinity; change the order so ...(msg.metadata || {}) is
spread first and then add instance_id: msg.instanceId || undefined and
session_affinity: sessionName afterwards (referencing the metadata object
construction, msg.metadata, msg.instanceId, sessionName, instance_id, and
session_affinity) so the per-upload values always take precedence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1265ed1c-fedf-436b-b29c-299c7b4b9d1e
📒 Files selected for processing (6)
CHANGELOG.mdplugins/honcho/scripts/test-strip-pastes.tsplugins/honcho/src/cache.tsplugins/honcho/src/hooks/post-tool-use.tsplugins/honcho/src/hooks/session-end.tsplugins/honcho/src/hooks/user-prompt.ts
… spread Addresses CodeRabbit review feedback on PR plastic-labs#34. 1. user-prompt.ts: gate the +/- redaction on a real unified-diff anchor (`@@`, `--- a/`, or `+++ b/`). The previous unanchored regex matched any 3+ consecutive `+`/`-` lines, which false-positives on markdown bullet lists. With the anchor gate, drop the run-length requirement and redact every diff metadata + content line; collapse residue and prepend a single `[diff removed]` marker. Fenced ```diff blocks are already redacted by rule 1 above. 2. session-end.ts: spread `msg.metadata` BEFORE the explicit per-upload fields (instance_id, session_affinity) so per-upload values always take precedence. No live bug today (only `type` is queued), but the prior order let any future caller silently override session affinity. 3. scripts/test-strip-pastes.ts: lock down the new behavior with three added cases (markdown bullet list, mixed +/- bullet list, two raw unfenced unified-diff anchor variants) and update the previous anchorless +/- test to expect no redaction (ambiguous prose). Verification: bunx tsc --noEmit -p tsconfig.json # clean bun run scripts/test-strip-pastes.ts # 12/12 pass
There was a problem hiding this comment.
🧹 Nitpick comments (3)
plugins/honcho/scripts/test-strip-pastes.ts (3)
80-85: Tighten the long-path assertion.This case asserts the redaction marker appears but doesn't verify the original path/blob content is actually gone. Adding a
mustNotContainwould catch a regression where the marker is appended but the original content leaks through.♻️ Suggested addition
{ name: "long path-bearing line — must redact", input: "Look:\n" + "x ".repeat(110) + "/Users/foo/path/to/file.ts:123:error\nFix it.", expectRedacted: true, mustContain: ["[path/output removed]"], + mustNotContain: ["/Users/foo/path/to/file.ts"], },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/honcho/scripts/test-strip-pastes.ts` around lines 80 - 85, Update the test case named "long path-bearing line — must redact" to also assert the original path/blob is removed by adding a mustNotContain entry that includes the original path fragment (e.g. "/Users/foo/path/to/file.ts:123:error" or the full input substring) so the test checks both that the redaction marker (mustContain) is present and the unredacted path text (mustNotContain) does not appear; modify the test object with this new mustNotContain property alongside mustContain and expectRedacted.
16-51: DuplicatedstripPasteswill silently drift from production.This local copy is a verbatim duplicate of the production
stripPastes()inplugins/honcho/src/hooks/user-prompt.ts:71-126. The header comment (Lines 10-13) acknowledges the risk, but a duplicated reference implementation defeats the purpose of a regression test: if production is updated and this file isn't, the suite will keep passing against stale logic. Consider exportingstripPastesfromuser-prompt.tsand importing it here so the tests exercise the real function.♻️ Sketch: import the real implementation
In
plugins/honcho/src/hooks/user-prompt.ts, export the helper:-function stripPastes(prompt: string): { prompt: string; redacted: boolean } { +export function stripPastes(prompt: string): { prompt: string; redacted: boolean } {Then in this file:
-function stripPastes(prompt: string): { prompt: string; redacted: boolean } { - let redacted = false; - let out = prompt; - // ... duplicated body ... - return { prompt: out, redacted }; -} +import { stripPastes } from "../src/hooks/user-prompt";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/honcho/scripts/test-strip-pastes.ts` around lines 16 - 51, The test file duplicates the production stripPastes implementation, causing silent drift; instead export the canonical stripPastes from the production module and import it into this test. Modify plugins/honcho/src/hooks/user-prompt.ts to export the existing stripPastes function (or add a named export) and then replace the local stripPastes implementation in plugins/honcho/scripts/test-strip-pastes.ts with an import of that exported stripPastes, removing the duplicate code and keeping the test wired to the real function.
113-117: Test name references logic that doesn't exist.The name says "not a runs-of-3+ diff", but the current heuristic gates diff redaction on unified-diff anchors (
@@,--- a/,+++ b/), not on any "3+ run" rule. The input itself doesn't even start a line with+/-, so what's actually being verified is "no diff anchor → no redaction" — already covered by the cases at Lines 75-79 and 126-131. Either rename to reflect anchor-based gating or replace with a case that contributes new coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/honcho/scripts/test-strip-pastes.ts` around lines 113 - 117, The test case name "only a single +/- line (not a runs-of-3+ diff) — must NOT redact" is misleading because the redaction heuristic actually checks for unified-diff anchors (e.g., lines starting with "@@", "--- a/", "+++ b/") not "3+ run" logic; update this test in plugins/honcho/scripts/test-strip-pastes.ts by either renaming the test.name to reflect the real condition (e.g., "no unified-diff anchors — must NOT redact") or change the input to exercise a distinct "single-line +/-" scenario that actually differs from existing anchor-based tests (e.g., make input lines start with "+" and "-" and decide whether expectRedacted should be true/false per intended behavior), and ensure you update the test.name, input, and expectRedacted fields so the test provides unique coverage from the other cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/honcho/scripts/test-strip-pastes.ts`:
- Around line 80-85: Update the test case named "long path-bearing line — must
redact" to also assert the original path/blob is removed by adding a
mustNotContain entry that includes the original path fragment (e.g.
"/Users/foo/path/to/file.ts:123:error" or the full input substring) so the test
checks both that the redaction marker (mustContain) is present and the
unredacted path text (mustNotContain) does not appear; modify the test object
with this new mustNotContain property alongside mustContain and expectRedacted.
- Around line 16-51: The test file duplicates the production stripPastes
implementation, causing silent drift; instead export the canonical stripPastes
from the production module and import it into this test. Modify
plugins/honcho/src/hooks/user-prompt.ts to export the existing stripPastes
function (or add a named export) and then replace the local stripPastes
implementation in plugins/honcho/scripts/test-strip-pastes.ts with an import of
that exported stripPastes, removing the duplicate code and keeping the test
wired to the real function.
- Around line 113-117: The test case name "only a single +/- line (not a
runs-of-3+ diff) — must NOT redact" is misleading because the redaction
heuristic actually checks for unified-diff anchors (e.g., lines starting with
"@@", "--- a/", "+++ b/") not "3+ run" logic; update this test in
plugins/honcho/scripts/test-strip-pastes.ts by either renaming the test.name to
reflect the real condition (e.g., "no unified-diff anchors — must NOT redact")
or change the input to exercise a distinct "single-line +/-" scenario that
actually differs from existing anchor-based tests (e.g., make input lines start
with "+" and "-" and decide whether expectRedacted should be true/false per
intended behavior), and ensure you update the test.name, input, and
expectRedacted fields so the test provides unique coverage from the other cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faf0dadc-0cf0-4be6-8ad4-d8520c96d942
📒 Files selected for processing (3)
plugins/honcho/scripts/test-strip-pastes.tsplugins/honcho/src/hooks/session-end.tsplugins/honcho/src/hooks/user-prompt.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/honcho/src/hooks/session-end.ts
- plugins/honcho/src/hooks/user-prompt.ts
…, prose guard
Addresses adversarial-review feedback (Codex gpt-5.5 xhigh + Gemini
3.1 Pro + Cursor agent).
stripPastes() rewritten as three small helpers, each with stronger
guarantees and tests:
1. stripFencedBlocks(): scan-and-redact fenced code blocks.
- Supports 3+ backtick AND 3+ tilde fences (was: only triple
backtick non-greedy regex).
- Fails closed on EOF without a matching closer (the user almost
certainly pasted code without remembering to close the fence).
- Length-aware closer matching: a 4-backtick fence with an inner
triple-backtick block redacts the whole outer block in one go.
2. stripUnifiedDiffBlocks(): stateful, anchor-gated diff redactor.
- Enters "diff mode" only on a real anchor (@@, --- a/, +++ b/).
- Inside the block, redacts every diff-grammar line including
space-prefixed CONTEXT lines (closes the leak that Codex
reported: ` function buildOperatorPlan() {` was surviving the
previous +/-only filter).
- Exits the block on the first non-diff line, so a Markdown
bullet list that appears AFTER a diff in the same prompt
("- option A") is preserved.
3. looksLikeLongPathOutput(): tightened path heuristic.
- >200 chars + slash is no longer sufficient. We now require
either no internal whitespace OR a contiguous run of 3+
slash-separated identifiers. Long prose paragraphs that
mention a URL or a fraction are not redacted.
scripts/test-strip-pastes.ts: 17 cases now (was 12), adding:
- unclosed fence (fail-close guarantee)
- four-backtick fence with inner triple
- tilde fence
- long prose paragraph with URL/fraction (must NOT redact)
- raw unfenced unified diff with context lines (must redact)
- diff followed by Markdown bullet list (no cross-contamination)
17/17 pass. Type-check clean.
Findings deduplicated across reviewers; corresponding RESIDUAL RISK
items in the PR description still apply (dream/dialectic out of
scope; redaction loses forensic raw-paste record from Honcho's view).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/honcho/scripts/test-strip-pastes.ts (2)
144-148: Strengthen the leak assertion on the path-redaction case.This case verifies the marker is emitted but doesn't assert that the original path content is gone. A regression that emits the marker and leaves the line in place would still pass. Mirror the pattern used by the fenced-block / diff cases and add a
mustNotContainfor a distinctive substring of the path.✅ Proposed assertion
name: "long path-bearing line that looks like a stack trace — must redact", input: "Look:\n/Users/foo/" + "a/".repeat(110) + "file.ts:123\nFix it.", expectRedacted: true, mustContain: ["[path/output removed]"], + mustNotContain: ["file.ts:123"], },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/honcho/scripts/test-strip-pastes.ts` around lines 144 - 148, The test case named "long path-bearing line that looks like a stack trace — must redact" currently asserts only that the redaction marker appears; update that test object to also assert the original path is removed by adding a mustNotContain entry for a distinctive substring of the original path (for example "/Users/foo/" or "file.ts:123") so the test fails if the raw path remains even when the marker is present.
16-93: Test duplicates the production implementation; risks silent drift.
stripPastesand its three helpers here are a verbatim copy of the implementation inplugins/honcho/src/hooks/user-prompt.ts(lines 71–224 per context). The header comment on lines 10–13 acknowledges this — "if the source changes, this file should be updated in the same PR" — but that's a manual contract that future PRs can quietly break: a regression in production code will still produce 17/17 passing here because the test exercises its own inline copy. Consider exportingstripPastesfromuser-prompt.tsand importing it so this script tests the real function.♻️ Sketch of the refactor
In
plugins/honcho/src/hooks/user-prompt.ts, export the function (or a__test__namespace):-function stripPastes(prompt: string): { prompt: string; redacted: boolean } { +export function stripPastes(prompt: string): { prompt: string; redacted: boolean } {Then in this file, replace the inline copy with:
import { stripPastes } from "../src/hooks/user-prompt";If
user-prompt.tshas top-level side effects on import (hook registration, etc.), guard them or movestripPastesinto a sibling pure module (e.g.src/hooks/strip-pastes.ts) and import from there in both the hook and this script.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/honcho/scripts/test-strip-pastes.ts` around lines 16 - 93, The test file duplicates the production implementation of stripPastes (and helpers stripFencedBlocks, stripUnifiedDiffBlocks, looksLikeLongPathOutput), risking silent drift; refactor by moving/exporting stripPastes from the production module (e.g., export from the function in user-prompt.ts or extract into a new pure module like strip-pastes.ts) and replace the duplicate functions in plugins/honcho/scripts/test-strip-pastes.ts with an import of that exported stripPastes; if importing user-prompt.ts triggers hook side effects, either guard those side effects or place stripPastes in a sibling pure module and import that from both user-prompt.ts and the test script, then remove the inline copies (stripFencedBlocks, stripUnifiedDiffBlocks, looksLikeLongPathOutput) from the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/honcho/scripts/test-strip-pastes.ts`:
- Around line 183-187: The test case named "only a single +/- line (not a
runs-of-3+ diff) — must NOT redact" is stale; rename it to reflect the actual
anchor-gated heuristic (absence of diff anchors like "@@", "--- a/", "+++ b/")
so it isn't misleading. Locate the test object with the name string and update
the name to something like "no diff anchors (single +/- line) — must NOT redact"
or "single +/- line without @@/---/+++ anchors — must NOT redact" while keeping
input and expectRedacted unchanged.
---
Nitpick comments:
In `@plugins/honcho/scripts/test-strip-pastes.ts`:
- Around line 144-148: The test case named "long path-bearing line that looks
like a stack trace — must redact" currently asserts only that the redaction
marker appears; update that test object to also assert the original path is
removed by adding a mustNotContain entry for a distinctive substring of the
original path (for example "/Users/foo/" or "file.ts:123") so the test fails if
the raw path remains even when the marker is present.
- Around line 16-93: The test file duplicates the production implementation of
stripPastes (and helpers stripFencedBlocks, stripUnifiedDiffBlocks,
looksLikeLongPathOutput), risking silent drift; refactor by moving/exporting
stripPastes from the production module (e.g., export from the function in
user-prompt.ts or extract into a new pure module like strip-pastes.ts) and
replace the duplicate functions in plugins/honcho/scripts/test-strip-pastes.ts
with an import of that exported stripPastes; if importing user-prompt.ts
triggers hook side effects, either guard those side effects or place stripPastes
in a sibling pure module and import that from both user-prompt.ts and the test
script, then remove the inline copies (stripFencedBlocks,
stripUnifiedDiffBlocks, looksLikeLongPathOutput) from the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30efca98-c37b-4136-a8e1-7e806e5a720d
📒 Files selected for processing (2)
plugins/honcho/scripts/test-strip-pastes.tsplugins/honcho/src/hooks/user-prompt.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/honcho/src/hooks/user-prompt.ts
…rgence) Round 2 of adversarial review (Codex gpt-5.5 xhigh + Gemini 3.1 Pro + Cursor agent + Opus 4.7 xhigh + Pi w/ DeepSeek V4 Pro xhigh + DeepSeek direct API). 5/5 reviewers converged on the same critical findings, already addressed in b0e14ff and ee24fed. This commit closes the two remaining holes only Opus 4.7 + Pi caught: 1. Anchor gate too weak (Opus): single-line prose mentioning a diff- shaped token like "the path '+++ b/foo.ts' is stale" triggered the diff branch and self-redacted to "[diff removed]\n", deleting the user's entire prompt. Strengthened to require either: - a real hunk header (`@@ ... @@`), OR - a paired file-header pair (`--- a/...` immediately followed by `+++ b/...`) A single anchor-shaped line in prose no longer triggers. 2. Long URL prose paragraph false-positive (Pi+DeepSeek): paragraphs like "Based on the documentation at https://docs.example.com/api/v2/ authentication and the reference implementation at https://github. com/example/repo/blob/main/auth.py..." have 3+ slash-separated identifier runs and were redacted as `[path/output removed]`. looksLikeLongPathOutput() now also checks alphabetic word density: prose has 20+ word-like tokens at >200 chars; log dumps and stack traces are word-sparse. Lines with high word count are preserved even when they contain path-like substrings. Tests: 21/21 pass. Five new cases for the new behavior: - single-line prose mentioning '+++ b/foo.ts' must NOT redact - single-line prose mentioning '--- a/foo.ts' must NOT redact - user opinion bullets AFTER diff must survive - long URL-bearing prose paragraph must NOT redact Type-check clean. Reviewers also surfaced (deferred to follow-up): - per-chunk metadata re-eval in session-end.ts (large prompts where redacted content fits in one chunk but prose section spans several — currently all chunks inherit the user_paste_not_speech tag). Edge case; documented as residual risk. - shared schema doc for metadata.type strings (PR-34 emits, PR-619 reads — typo on either side silently disables filter). Worth a small docs/metadata-types.md follow-up. - raw unfenced code paste with no anchor and no path-trigger is the biggest remaining attack surface for misattribution. Future work: keyword-density classifier or LLM-based content classification at upload time.
The case name "(not a runs-of-3+ diff)" referenced a heuristic that no longer exists — the implementation is anchor-gated. Renamed to "stray +/- chars in prose (no diff anchor)" per CodeRabbit suggestion. Behavior unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/honcho/scripts/test-strip-pastes.ts (1)
16-98: Reduce sanitizer logic duplication to prevent drift.This block mirrors production logic from
plugins/honcho/src/hooks/user-prompt.tsverbatim. Keeping two implementations synchronized manually is brittle; a shared utility module (used by both hook and test script) would make regressions less likely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/honcho/scripts/test-strip-pastes.ts` around lines 16 - 98, The test script duplicates sanitizer logic from plugins/honcho/src/hooks/user-prompt.ts (functions stripPastes, stripFencedBlocks, stripUnifiedDiffBlocks, looksLikeLongPathOutput); extract that logic into a shared utility (e.g., plugins/honcho/src/utils/paste-sanitizer.ts) exporting those functions, replace the local implementations in plugins/honcho/scripts/test-strip-pastes.ts with imports from the shared module, and update the hook (user-prompt.ts) to import the same utilities so both use a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/honcho/scripts/test-strip-pastes.ts`:
- Around line 4-7: Update the usage comment in
plugins/honcho/scripts/test-strip-pastes.ts to clarify working directory: keep
the existing local invocation `bun run scripts/test-strip-pastes.ts` (for
running from plugins/honcho/) and add the repo-root invocation `bun run
plugins/honcho/scripts/test-strip-pastes.ts`, and state explicitly which command
to use depending on current directory so manual checks won't fail; reference the
script name test-strip-pastes.ts in the comment.
---
Nitpick comments:
In `@plugins/honcho/scripts/test-strip-pastes.ts`:
- Around line 16-98: The test script duplicates sanitizer logic from
plugins/honcho/src/hooks/user-prompt.ts (functions stripPastes,
stripFencedBlocks, stripUnifiedDiffBlocks, looksLikeLongPathOutput); extract
that logic into a shared utility (e.g.,
plugins/honcho/src/utils/paste-sanitizer.ts) exporting those functions, replace
the local implementations in plugins/honcho/scripts/test-strip-pastes.ts with
imports from the shared module, and update the hook (user-prompt.ts) to import
the same utilities so both use a single source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 446e233f-c179-4444-9ea2-50e2045643b8
📒 Files selected for processing (2)
plugins/honcho/scripts/test-strip-pastes.tsplugins/honcho/src/hooks/user-prompt.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/honcho/src/hooks/user-prompt.ts
Comment on lines
+4
to
+7
| * src/hooks/user-prompt.ts. Run with: | ||
| * | ||
| * bun run scripts/test-strip-pastes.ts | ||
| * |
There was a problem hiding this comment.
Clarify run location for the manual command.
On Line 6, bun run scripts/test-strip-pastes.ts works only if run from plugins/honcho/. Please document repo-root invocation too to avoid false “file not found” during manual release checks.
Suggested doc tweak
- * bun run scripts/test-strip-pastes.ts
+ * # from plugins/honcho/
+ * bun run scripts/test-strip-pastes.ts
+ * # from repo root
+ * bun run plugins/honcho/scripts/test-strip-pastes.ts📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * src/hooks/user-prompt.ts. Run with: | |
| * | |
| * bun run scripts/test-strip-pastes.ts | |
| * | |
| * src/hooks/user-prompt.ts. Run with: | |
| * | |
| * # from plugins/honcho/ | |
| * bun run scripts/test-strip-pastes.ts | |
| * # from repo root | |
| * bun run plugins/honcho/scripts/test-strip-pastes.ts | |
| * |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/honcho/scripts/test-strip-pastes.ts` around lines 4 - 7, Update the
usage comment in plugins/honcho/scripts/test-strip-pastes.ts to clarify working
directory: keep the existing local invocation `bun run
scripts/test-strip-pastes.ts` (for running from plugins/honcho/) and add the
repo-root invocation `bun run plugins/honcho/scripts/test-strip-pastes.ts`, and
state explicitly which command to use depending on current directory so manual
checks won't fail; reference the script name test-strip-pastes.ts in the
comment.
ichoosetoaccept
pushed a commit
to detailobsessed/claude-honcho
that referenced
this pull request
Jul 15, 2026
Everything on a role:"user" message is read by Honcho's fact extractor as
the user's own words, so a "review this diff" prompt with a pasted patch
minted durable misattributions ("the user changed buildOperatorPlan").
Add stripPastes(): redact fenced code blocks, runs of 3+ diff lines, and
long path-bearing output lines before upload; short path mentions and a
lone +/- line in prose are preserved. Tag the stored message
type:"user_paste_not_speech" when anything was redacted. Only the stored
copy is stripped — context retrieval still searches the full prompt.
Tool actions from the PostToolUse hook now carry type:"tool_action" /
subject:"ai_action_on_user_behalf" so directional/MCP scopes don't fold
the assistant's tool use into the user's own representation.
Ported from upstream plastic-labs#34.
ichoosetoaccept
pushed a commit
to detailobsessed/claude-honcho
that referenced
this pull request
Jul 15, 2026
Turn-path reliability and memory quality since 0.1.0: - Non-blocking Stop hook + workspace-scoped outbox drain (#15) - Strip pasted non-prose from user messages; tag tool actions (plastic-labs#34) - Inject prompt-matched conclusions instead of the stalest facts (plastic-labs#63)
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
user-prompt.tsandpost-tool-use.tsupload content to the Honcho message stream that the server-side fact extractor reads as user-authored prose, even when it isn't. This produces durable misattribution facts in the user-peer's representation — e.g.<user> changed buildOperatorPlanfrom a pasted diff in an adversarial-review prompt the user actually typed:The diff body rides
role: "user"per the Anthropic Messages API, so the extractor treats+/-lines as the user's statements about their own code.This PR strips pasted code/diffs/long-output from
userPeeruploads and tagsaiPeertool-action uploads with explicit role metadata.The bug, concretely
Two sites:
src/hooks/user-prompt.tsqueues the entirepromptstring for upload touserPeerat session-end. Pastes in user prompts (review-this-diff, fix-this-stack-trace, etc.) carryrole: "user"per the Messages API but are not user-authored prose. The extractor reads them as such.src/hooks/post-tool-use.tsuploadsaiPeer.message("[Tool] Edited file: ...")with metadata that has no role discriminator.unifiedobservation mode reduces blast radius but does not eliminate it — directional deployments and any MCP tool that operates in directional scope still fold these into the user's representation as<user> did X.The end result: durable cross-session memory contaminated with assistant-authored facts attributed to the user. We observed this in the wild after running with
directionalmode briefly — a sample of 50 conclusions on page 1 oflist_conclusionsshowed ~10 misattributions (extrapolated estimate: ~12K of ~60K conclusions in a heavily-used workspace).Fix
src/hooks/user-prompt.ts— strip pastes before queuingNew
stripPastes()helper redacts:If anything is redacted, the queued message is tagged with
metadata.type = "user_paste_not_speech"so server-side extraction can filter cross-peer attribution.The single-character
+/-line case (e.g."Note: + means added, - means removed.") is preserved — only runs of 3 or more consecutive prefixed lines are treated as a diff. Short path mentions in prose ("Edit /Users/foo/file.ts please") are also preserved.src/hooks/post-tool-use.ts— explicit role metadatasrc/cache.ts— optional metadata onQueuedMessageQueuedMessagegains an optionalmetadatafield.queueMessage()accepts an optional 5th argument that is forwarded touserPeer.message()at session-end. Backwards compatible — existing callers and queued messages without metadata continue to work.src/hooks/session-end.ts— forward queued metadataSpreads
msg.metadatainto theuserPeer.message()metadata block at upload time so server-side extraction sees the role tags.Verification
$ bunx tsc --noEmit -p tsconfig.json # clean $ bun run scripts/test-strip-pastes.ts PASS pure prose — must NOT redact PASS fenced code block — must redact + strip code PASS unified diff — must redact PASS long path-bearing line — must redact PASS short path-bearing line — must NOT redact PASS adversarial-review pattern (the original bug) — must redact code, preserve prose PASS empty prompt — must NOT redact PASS only a single +/- line (not a runs-of-3+ diff) — must NOT redact 8/8 passedThe repo currently has no test runner;
scripts/test-strip-pastes.tsis a pragmatic regression check intended to be rerun manually before releases that touchuser-prompt.ts. Happy to wire it into CI in a follow-up PR if desired (or to introducebun:testfor this and future tests).Backwards compatibility
queueMessage()signature change is purely additive (5th arg is optional). Single in-tree caller updated.QueuedMessageinterface change is purely additive (metadata?optional). Existing queue files on disk parse cleanly because the field is absent.Notes for reviewers
[code block removed],[diff removed],[path/output removed]) instead of the original paste content. That's intentional — the original content is not the user's prose and shouldn't be in the user-peer's record.metadata.contains_paste: truesofter marker would be a follow-up.metadata.subject = "ai_action_on_user_behalf"is a string convention. If you'd prefer a structured enum or a reserved namespace (honcho.role/honcho.subject), I'm happy to rename — just point me at the convention.saveMessages: truepolicy. A deeper-cut alternative would be to stop uploading raw user prompts entirely and only upload extracted facts — but that's a much bigger change and out of scope here.Summary by CodeRabbit
New Features
Tests