feat(autonudge): scrub loop text at every egress sink - #7996
Conversation
53ea23d to
850f32c
Compare
Audit note — this PR rehomes work from the closed #5999, and it collides with #7777The good news first. #5999 was closed by an overlap audit as redundant with #7777, and its closing comment listed work that was in #5999, not on Read the merge-base diff of #7996 (5,649 lines) end to end, its body.md, #5999's closing comment, #7777's diff, and checked every item against ITEM BY ITEM — all eight covered by #7996:
DISJOINTNESS PROVEN BOTH WAYS: zero harvest tokens in #7777's current head (all 12 probes = 0); zero banner tokens in #7996 ( The part that needs careThis PR and #7777 touch seven files in common, and one collision is semantic rather than textual:
From a repository-wide duplicate/overlap audit of every pull request open against |
4b3f1b1 to
d181a9c
Compare
d181a9c to
c7299bd
Compare
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of I have enough to render the design review. The change solves a real egress problem with a sound shared-definition scrub; the risks are bundling and the shape of the user-row sweep. Design-Verdict: CONCERNS Sound egress scrub, but four independent designs ship in one PR, and the ten-channel user-row rewrite is irreversible and unmarked. WatchBundling defeats revertability. The sink scrub, the ten-channel persist-time sweep ("a behaviour change for ORDINARY CHAT on all of them"), the two-phase Rewritten user rows are permanent and carry no marker. Scrubbing in the callers means SuggestionsThe quarantine sidecar's lock/move-aside/compact apparatus exists mainly so a downgraded build cannot arm a held row — but a downgrade merely regresses to today's baseline; the PR's own [DESIGN-REVIEWED] f717b15 |
UX Review (Fable 5, fork) — 🟡 CONCERNSUX-level review of I have everything I need: the full diff of the popover surface, the English strings, the capture harness, and the base component. Reconciling the added controls against available evidence (fork PR — no screenshots on disk, no blind read has run), here is the review. UX-Verdict: CONCERNS Well-reasoned overwrite/conflict gate, but every new control is unseen by any first-time reader, and the screenshots predate a focus-behavior change at HEAD. Watch
Evidence gaps
[UX-REVIEWED] f717b15 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All lenses are run and counts verified. Final review: First-Principles-Verdict: CONCERNS Three riders carry most of the permanent surface: a zero-consumer Not justified as shipped
What this change shipsInventory (10 items) — 5 justifiedIntent: stop credential-shaped loop text from leaving the backend unscrubbed at its read surfaces — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] f717b15 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsFINDING -- src/kiro_crew/dashboard/handlers/autonudge.py:170 -- |
c7299bd to
072ba97
Compare
5adff5e to
c08cf86
Compare
c08cf86 to
6acc32f
Compare
6acc32f to
489dc99
Compare
489dc99 to
4117cdf
Compare
4117cdf to
b5adecf
Compare
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
iamwhatever
left a comment
There was a problem hiding this comment.
There is one ongoing effort to unify all changes about auto-nudge, babysitting together. I will forward this PR to the owner so the work can be merged together.
|
@rnoack1 Thanks for this work. Audited at 495cfbf (the head has moved since, so please re-check anything below against your current branch). This PR and open #9052 (@rubencu) rewrite the same two statements in opposite directions, so a clean-looking conflict resolution would silently revert one side.
Also shared, mechanically only: Your side is further along: 64 files against 7, and #9052 is 131 commits behind on an older popover blob with a merge conflict. Suggested sequence: land this PR first after its own rebase, then #9052 re-derives both sites on top, keeping its conditional Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
|
@bolichen97 Re-checked every item from both audit notes at the current head Settled since the audit
Your two 2026-09-08 readings — both still accurate at this head
So your suggested sequence still holds from my side, and #9052 would re-derive both sites. Still open The PR body is stale exactly as you describe (2026-09-02, item 2) — this head does carry the Thanks for both audits. The base-state item was the right one to refuse to take on trust. |
Adds the denylist projection scrub, the message_redacted/message_ignored wire flags, the echo guard and 503 arms, store vetting with a sidecar, and the popover surface.
Problem / Motivation
A nudge loop's text fields are written by callers and then served back to readers on
three separate surfaces — the REST projection (
GET /api/autonudgeand the per-loopdetail, plus the
POST/PATCHechoes), theautonudge_stateWS broadcast, and thedashboard transcript row. Today
_serializeis a plainasdict, so whatever is in thestore leaves the backend verbatim.
That is fine for a loop armed through the API, because the arming authorizer already
redacts on the way in. It is not fine for the producers that never pass through it: a
hand-edited
autonudge.json, or an internalsvc.add. Base_loadalready credential-scrubsmessageandbanneron that path, so the gap is narrower than "nothing has ever scanned it": itscrubbed with the bare redactor pair rather than the active credential policy, so a composed host's
own patterns were skipped, and nothing scrubbed at the REST/WS sinks at all — so every other field
was served raw, and a value written to the store after load was served unscrubbed.
Riders, declared. Two changes here are independent of the redaction thesis and are named so a
bisect lands on something that mentions them. A non-list
loopsvalue in the store now refusesarming and writes instead of crashing the loader (it fixes a real base crash). And the popover
gained a viewport width clamp plus a confirmation gate for a goal that changed mid-edit — the latter
is a general lost-update guard, not a redaction concern, added because the AI review lane blocked on
a live update silently overwriting a newer goal.
Separately, the loader trusts fields it uses to ADDRESS a loop. A row whose
idorslot_keyis credential-shaped is armed and then served raw, because those two fieldsare deliberately exempt from the egress scrub — rewriting them would leave a row the
client cannot act on.
Why it matters
The transcript row and the WS broadcast are persisted and pushed to every connected
dashboard client, so an unscanned credential in a loop's text reaches every open browser
and stays in the session file. An input-side control does not discharge an output sink:
the sink is where the reader is.
The addressing case is worse in kind, because the value is exempt by design. Without a
guard at load there is no point downstream at which it is ever scrubbed.
What changed (motivation → approach → change)
Scrub at the sinks, from one shared definition.
scrub_loop_textandADDRESSING_FIELDSlive inautonudge.pyand are imported by the REST serializer andthe WS observer, so the two surfaces cannot disagree about what is credential-shaped.
_serializeis now a DENYLIST — every field is scrubbed unless named inADDRESSING_FIELDS(plus amonitorspecial case) — because an allowlist would silently miss the next free-text fieldadded to
NudgeLoop.Tell the client when the projection is masked. A scrubbed projection that a client
echoes back would overwrite the real instruction with its own mask. So the wire carries
message_redactedwhen the scrub changedmessage,message_ignoredwhen the backenddeclined to store a write, and
message_is_echoed_projectionrefuses aPATCHthat ismerely the masked copy handed back.
Refuse a store that cannot be vetted, rather than guessing. A row whose addressing
field is credential-shaped, non-printable or non-
stris held aside in a separateautonudge.quarantine.jsonsidecar while its siblings arm normally. The sidecar iswritten ADDITIVELY before the main store lands and compacted only once it has, so a
failed replacement cannot leave a repaired row in neither file. An unreadable sidecar
refuses every write in that process and is moved aside under a
.corrupt-<ts>name, sorecovery is a restart rather than a hand repair. On a host whose credential policy will
not compose, both authorizers answer 503 instead of persisting unvetted text.
Say so in the UI. The popover shows an amber notice when the served goal is masked,
and an explicit overwrite confirmation when the user edits masked text — saving that edit
would replace an instruction the server cannot return. Declining dismisses the gate and
keeps the typed text.
Breadth, stated plainly. Two parts of this change are wider than the nudge path. The
first is a persist-time
redact_via_contexton EVERY inbound user message a channel writesto the conversation log, because that persisted row is itself an egress served to dashboard
readers. It is added across TEN modules plus the shared
save_conversation_turnhelper, not three: thetransport_dispatch.pyofdiscord,telegram,webex,feishu,imessage,teams,wecom,weixin,whatsappandslack, pluseval/runner.py. That is a behaviour changefor ORDINARY CHAT on all of them, not only for nudges — a redaction false positive rewrites a
user's own transcript text at rest.
Slack is included rather than cited as precedent. An earlier draft of this description
claimed this change brought the other channels up to a posture Slack already had; that was
wrong and is withdrawn. Measured at the base commit,
slack/transport_dispatch.pycontained ZERO redactor calls and persisted its inbound user rows raw, so there was no
existing all-inbound posture anywhere to match. Slack's row is now scrubbed for
the PERSISTED copy only: the same
textstill reachesbuild_message, so the model promptis never redacted — the same split the nudge row already uses.
Scope of the sweep, and a correction. An earlier revision of this description claimed the
count was every append passing role
"user". That was FALSE and is corrected here: the countmissed the INDIRECT path.
save_conversation_turn(llm_helpers.py) appends a user row onbehalf of its callers, and its async wrapper is called from twelve Slack sites —
slack/handler.pyat seven,slack/transport_dispatch.pyat three,slack/gateway.pyat two— none of which name
appendthemselves, so a grep for the direct call could not see them.taskrunner.pywas missed the same way. The re-count at this head is in the sweep above:the scrub now sits in
save_conversation_turnitself, so ONE edit covers all twelve callers,plus
taskrunner.pyand the direct sites.What this still does NOT claim. The assistant halves need no call, because
_redact_at_write_boundary(history.py) already scrubs every role other thanuser. Itsuserexemption is the gap these calls close, and that docstring no longer asserts user textis stored verbatim — it said so while eleven callers had already stopped honouring it, which
made the stated invariant false; it now records where the rule actually lives. Centralising
the scrub INTO
ConversationLog.appendwould be the smaller, correct-by-construction shape,and it is deliberately NOT done here: the exemption also covers the dashboard's own
write-back, so flipping the default is a product-wide behaviour change rather than a fix to
this leak. A fourteenth persister that calls
conv_log.append(key, "user", text)directly wouldreopen the leak, so a TEST enforces the deferral rather than a promise:
TestEveryUserRowPersisterScrubspins the persister set bidirectionally and resolves everyuser-row call site to its enclosing function, requiring the redactor in that scope. A new
module fails the set pin; a new unscrubbed call site inside an already-listed module fails
the per-site arm. Both were mutation-controlled.
The
slot.append("user", …)writes indashboard/are a DIFFERENT surface — the dashboardslot, not the conversation log — and are untouched.
The reason for the sweep differs per channel:
binding_key_forhonours onlyslack:/discord:/webex:, so a telegram, feishu, imessage, teams, wecom, weixin orwhatsapp session never receives a nudge synthetic inbound at all, and its call covers
ordinary inbound text only. Separately, the autonudge log-line scrub drops two
exc_infotracebacks, trading a stack trace for keeping store-sourced values out of the log ring and
the
/api/logsstream.Two further riders ship here and are declared rather than implied.
A two-phase cycle claim. A fire previously incremented
cycle_countand persisted BEFOREdelivering, so a process that died inside
_on_fire-- which the code itself notes runs inlineand can take minutes -- reloaded a capped loop as spent and never ran its last action. The claim
is now recorded BESIDE the spent count as a disk-only
inflight_cyclekey: phase one persists theclaim with
cycle_countunchanged, phase two advances the count only once delivery is confirmed,and a refused turn clears the claim and re-arms. Restart semantics change: a claim marker that
outlived its process means delivery was never confirmed, so that cycle is OWED and runs again
rather than counting as spent.
inflight_cycleis not aNudgeLoopfield, so it never reaches aclient, and
cycle_countin memory still means DELIVERED cycles -- which is what the cap and thesix
gateway.pyrender sites read.The composed-host scrub now covers every persisted row BODY, not only user rows. The bare
redact_exfiltration_urls/redact_credentialspair skips a composed host's own patterns, sofixing that for user rows alone left the other halves on the bare pair.
history.py's non-userwrite boundary, the Slack nudge reply and announce rows, and the cron prompt/result/hydration rows
now route through
redact_via_contextas well -- model output carries credentials a companionrecognises and the baseline pair cannot see. Deliberate boundary:
cron_inject.py'sdisplay-name helper keeps the bare two-pass call, because its ORDER is pinned byte-for-byte by a
sibling test and it titles a slot rather than persisting a row. Bare-pair readers elsewhere in the
tree are out of scope for this PR and unchanged.
Tests
test/test_autonudge_egress.py(210 tests) locks in each half:text round-trips unrewritten — that last arm matters because a scrub that replaced
everything with a placeholder would satisfy the first two while destroying the surface.
cannot bypass either.
message_redacted/message_ignoredon the wire, and the echo guard refusing aresubmitted projection.
refusal does not destroy the store; a repaired row re-arms; a downgrade cannot delete
held rows; the sidecar's additive write survives a failed main-store replacement and is
compacted only after it lands; an unreadable sidecar refuses writes and is moved aside.
_on_fireleaves a capped loopable to fire its remaining cycle on restart, and a refused turn leaves no claim marker on disk.
Mutation-controlled -- returning the claim to
cycle_countreproduces the lost cycle.companion-only credential in an assistant row is rewritten. The control proves the bare pair
cannot see that shape, so neither arm can pass vacuously.
live loop's unsent edit, where closing would lose it; a second Escape closes.
website/src/test/AutoNudgeRedactedProjection.test.tsxcovers the popover: the notice,the overwrite confirmation, focus placement, and that declining preserves typed text.
Manual verification
Unit and component coverage carry the backend and the popover logic. Two honest caveats:
produced those frames asserts each state before it writes the PNG, so a frame cannot
document a state the component was not actually in.
test_autonudge.py::TestSentinelPathRepair::test_unnormalized_path_escaping_legacy_is_preservedfails on this branch. It is a PRE-EXISTING base-state failure, not caused by this
change: it fails identically on a clean checkout of the merge base (
assert '.kiro/crew'),which I verified in a throwaway worktree at that commit.
Screenshots / video
Captured with
website/scripts/capture-autonudge-redaction.mjs, which drives the isolatedentry
website/capture/autonudge-redaction.html. The fixture goal is FABRICATED — theseframes are permanent and outward-facing, and credential-shaped text is this PR's subject,
so the harness is never seeded from a real loop store. The only masked run shown is the
documented
aws-access-key-idexample placeholder.Each frame ASSERTS its state before writing, so it cannot document the wrong one.
The masked goal and the notice that explains it. Without the notice the user reads
[REDACTED: ...]in their own words with no explanation, which is the gap this closes.The overwrite confirm, armed. The question line renders, the two buttons sit BELOW the
action row (never in Save's position, so a double-click cannot land on the destructive
choice), and focus is on the safe choice — the ring is in the pixels, asserted through
:focus-visiblerather than claimed in prose.A save the backend answered with
message_ignored. The stored goal was kept, so thenotice says the goal text was left unchanged while the other settings were saved.
A save where the stored goal moved under the edit. The popover shows the newer
stored goal next to the text being typed, so the choice is made against both.
The keep-stored arm, and the gate re-arming. The first is the only path that saves settings while leaving the goal untouched; the second is what a second concurrent change looks like mid-confirm. Neither is reachable by hand without racing a websocket, so both are captured.
The same three states in the other theme
Related Issues
Carries forward the non-banner half of #5999, which was closed as superseded by #7777.
#7777 ships the
bannerfield alone; this PR ships the egress scrub, the wire flags, theecho guard, the 503 arms and the store vetting, none of which is on
mainor in #7777.The two are independent: this branch contains no
bannerfield and noMAX_BANNER_CHARS.