Skip to content

feat(autonudge): scrub loop text at every egress sink - #7996

Open
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:feat/autonudge-egress-hardening
Open

feat(autonudge): scrub loop text at every egress sink#7996
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:feat/autonudge-egress-hardening

Conversation

@rnoack1

@rnoack1 rnoack1 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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/autonudge and the per-loop
detail, plus the POST/PATCH echoes), the autonudge_state WS broadcast, and the
dashboard transcript row. Today _serialize is a plain asdict, so whatever is in the
store 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 internal svc.add. Base _load already credential-scrubs
message and banner on that path, so the gap is narrower than "nothing has ever scanned it": it
scrubbed 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 loops value in the store now refuses
arming 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 id or
slot_key is credential-shaped is armed and then served raw, because those two fields
are 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_text and
ADDRESSING_FIELDS live in autonudge.py and are imported by the REST serializer and
the WS observer, so the two surfaces cannot disagree about what is credential-shaped.
_serialize is now a DENYLIST — every field is scrubbed unless named in
ADDRESSING_FIELDS (plus a monitor special case) — because an allowlist would silently miss the next free-text field
added 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_redacted when the scrub changed message, message_ignored when the backend
declined to store a write, and message_is_echoed_projection refuses a PATCH that is
merely 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-str is held aside in a separate
autonudge.quarantine.json sidecar while its siblings arm normally. The sidecar is
written 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, so
recovery 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_context on EVERY inbound user message a channel writes
to 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_turn helper, not three: the
transport_dispatch.py of discord, telegram, webex, feishu, imessage, teams,
wecom, weixin, whatsapp and slack, plus eval/runner.py. That is a behaviour change
for 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.py
contained 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 text still reaches build_message, so the model prompt
is 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 count
missed the INDIRECT path. save_conversation_turn (llm_helpers.py) appends a user row on
behalf of its callers, and its async wrapper is called from twelve Slack sites —
slack/handler.py at seven, slack/transport_dispatch.py at three, slack/gateway.py at two
— none of which name append themselves, so a grep for the direct call could not see them.
taskrunner.py was missed the same way. The re-count at this head is in the sweep above:
the scrub now sits in save_conversation_turn itself, so ONE edit covers all twelve callers,
plus taskrunner.py and 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 than user. Its
user exemption is the gap these calls close, and that docstring no longer asserts user text
is 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.append would 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 would
reopen the leak, so a TEST enforces the deferral rather than a promise:
TestEveryUserRowPersisterScrubs pins the persister set bidirectionally and resolves every
user-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 in dashboard/ are a DIFFERENT surface — the dashboard
slot, not the conversation log — and are untouched.

The reason for the sweep differs per channel: binding_key_for honours only
slack:/discord:/webex:, so a telegram, feishu, imessage, teams, wecom, weixin or
whatsapp 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_info
tracebacks, trading a stack trace for keeping store-sourced values out of the log ring and
the /api/logs stream.

Two further riders ship here and are declared rather than implied.

A two-phase cycle claim. A fire previously incremented cycle_count and persisted BEFORE
delivering, so a process that died inside _on_fire -- which the code itself notes runs inline
and 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_cycle key: phase one persists the
claim with cycle_count unchanged, 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_cycle is not a NudgeLoop field, so it never reaches a
client, and cycle_count in memory still means DELIVERED cycles -- which is what the cap and the
six gateway.py render sites read.

The composed-host scrub now covers every persisted row BODY, not only user rows. The bare
redact_exfiltration_urls/redact_credentials pair skips a composed host's own patterns, so
fixing that for user rows alone left the other halves on the bare pair. history.py's non-user
write boundary, the Slack nudge reply and announce rows, and the cron prompt/result/hydration rows
now route through redact_via_context as well -- model output carries credentials a companion
recognises and the baseline pair cannot see. Deliberate boundary: cron_inject.py's
display-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:

  • The denylist scrubs every text field, the addressing fields survive verbatim, and clean
    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.
  • The WS broadcast is scrubbed on the same definition as REST, and a non-string field
    cannot bypass either.
  • message_redacted / message_ignored on the wire, and the echo guard refusing a
    resubmitted projection.
  • Store vetting: a credential-shaped addressing field is held aside while siblings arm; a
    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.
  • The two-phase claim: a death between the claim persist and _on_fire leaves a capped loop
    able to fire its remaining cycle on restart, and a refused turn leaves no claim marker on disk.
    Mutation-controlled -- returning the claim to cycle_count reproduces the lost cycle.
  • Every persisted row body is scrubbed by the ACTIVE credential policy, not the bare pair: a
    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.
  • Escape: it closes for a new goal (the draft restores the text) and refuses VISIBLY once for a
    live loop's unsent edit, where closing would lose it; a second Escape closes.
  • website/src/test/AutoNudgeRedactedProjection.test.tsx covers 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:

  • The user-visible surface is photographed in the section above, and the harness that
    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_preserved
    fails 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 isolated
entry website/capture/autonudge-redaction.html. The fixture goal is FABRICATED — these
frames 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-id example 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.

Redacted goal at rest, dark

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-visible rather than claimed in prose.

Confirm gate armed, dark

A save the backend answered with message_ignored. The stored goal was kept, so the
notice says the goal text was left unchanged while the other settings were saved.

Ignored-fields notice, light

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.

Moved stored goal under an edit, dark

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.

Stored goal kept, other settings saved, light
Confirm gate re-armed after a second change, dark

The same three states in the other theme

Redacted goal at rest, light
Confirm gate armed, light
Ignored-fields notice, dark
Moved stored goal under an edit, light
Narrow viewport, dark
Narrow viewport, light

Related Issues

Carries forward the non-banner half of #5999, which was closed as superseded by #7777.
#7777 ships the banner field alone; this PR ships the egress scrub, the wire flags, the
echo guard, the 503 arms and the store vetting, none of which is on main or in #7777.
The two are independent: this branch contains no banner field and no MAX_BANNER_CHARS.

@rnoack1
rnoack1 requested a review from a team September 2, 2026 21:06
@rnoack1
rnoack1 requested a review from a team as a code owner September 2, 2026 21:06
@rnoack1
rnoack1 requested a review from patrigao September 2, 2026 21:06
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running labels Sep 2, 2026
@rnoack1
rnoack1 force-pushed the feat/autonudge-egress-hardening branch from 53ea23d to 850f32c Compare September 2, 2026 21:50
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Audit note — this PR rehomes work from the closed #5999, and it collides with #7777

The 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 main, and not in #7777 — work that would otherwise have been lost. This PR covers all of it. Verified item by item:

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 origin/main = 680baf9 (both PRs' actual base_sha).

ITEM BY ITEM — all eight covered by #7996:

  1. _serialize denylist egress scrub — _UNSCRUBBED_FIELDS = ADDRESSING_FIELDS (1073), out[key] = scrub_loop_text(value, field=key) (1134), monitor routed to _redact_monitor_value (1132), out["message_redacted"] (1139). Main still serves plain asdict + monitor-only redaction.

  2. scrub_loop_text + ADDRESSING_FIELDS (88, 164) — improved: homed in autonudge.py rather than the handler because a third caller (autonudge_authz.message_is_echoed_projection) would create an import cycle; the handler re-exports it so gateway.py keeps importing from there. Still one definition for REST + WS + authz. Main's only ADDRESSING_FIELDS is the unrelated _ADDRESSING_FIELDS in handlers_instances.py:89.

  3. _observer WS scrub — safe_message = scrub_loop_text(loop.message, field="message") (1374) + "message_redacted" (1387), plus the fire arm visible = redact_via_context(tagged) (1319). Main broadcasts "message": loop.message raw at gateway.py:5927.

  4. message_is_echoed_projection (863), _scrub_policy_unavailable (880) wired into both authorizers (on the arm path deliberately before the sentinel unlink), payload["message_ignored"] = True (1177).

  5. Popover: editsRedactedGoal (4766), two text-warn notices, 5 keys × 13 locales, AutoNudgeRedactedProjection.test.tsx at exactly 409 lines. Confirm label reworded ("Overwrite original goal") — same gate.

  6. Quarantine sidecar: _QUARANTINE_FILE (69), AutoNudgeStoreUnvetted (118), _quarantined (302), _load_refused (319), _read_quarantine_sidecar (645). The live GPT 5.6 BLOCKING finding is FIXED, not carried: an unreadable sidecar now arms nothing (338-345), pinned by test_a_non_object_sidecar_refuses_the_store_rather_than_arming, whose docstring names the finding and says the assertion was previously the opposite.

  7. security_posture.py inventory row (1217) + autonudge_state sentence (1240) + "autonudge.py" registration (1264).

  8. _load malformed-entry fix (544) + non-dict-row branch (540). Main still has the %r leak at autonudge.py:640; feat(autonudge): optional per-loop banner for the visible nudge row #7777 leaves that line untouched.

DISJOINTNESS PROVEN BOTH WAYS: zero harvest tokens in #7777's current head (all 12 probes = 0); zero banner tokens in #7996 (MAX_BANNER_CHARS/normalize_banner/banner_unsupported_for = 0), matching its body claim.

The part that needs care

This PR and #7777 touch seven files in common, and one collision is semantic rather than textual: slack/gateway.py::_fire_dashboard_nudge. A careless conflict resolution there silently drops either the egress scrub or the banner feature — the merge will look clean and one of the two behaviours will just be gone. Whichever lands second should re-derive that function rather than pick a side of the conflict.

  1. The cached the merge-base diff of feat(autonudge): optional per-loop banner for the visible nudge row #7777 is STALE — 17 files vs the current head's 20, missing src/kiro_crew/security_posture.py, test/test_autonudge_authz_cov80.py and test/test_slack_gateway_coverage.py. I re-fetched feat(autonudge): optional per-loop banner for the visible nudge row #7777's current patch over REST (GraphQL was rate-limited) to catch the security_posture.py collision; anyone re-running this from the cached file will miss it.

  2. feat(autonudge): scrub loop text at every egress sink #7996's body is stale: it says "I could not capture screenshots in this environment", but the head carries 6 PNGs under temp-screenshots/autonudge-redaction/ plus a capture harness (website/capture/autonudge-redaction.{html,tsx}, website/scripts/capture-autonudge-redaction.mjs) — the 9 files my local 24-section patch lacked. temp-screenshots/ is established convention (temp-screenshots/README.md and .github/workflows/cleanup-temp-screenshots.yml are both on main), so this is a body that no longer matches its diff, not a defect.

  3. Not verified: feat(autonudge): scrub loop text at every egress sink #7996's claim that test_autonudge.py::TestSentinelPathRepair::test_unnormalized_path_escaping_legacy_is_preserved is a pre-existing base-state failure. I did not run the suite.

  4. The 5 new i18n keys are inserted out of alphabetical order (confirm_overwrite_question after message_redacted_notice). No gate under website/scripts/ appears to enforce ordering, so cosmetic.

  5. I did not survey all ~280 other open PRs for overlap with feat(autonudge): scrub loop text at every egress sink #7996 — the duplication check was scoped to feat(autonudge): optional per-loop banner for the visible nudge row #7777 and origin/main as the task framed it. The harvest text itself asserts no split-b PR existed among the 280 open ones as of 2026-09-02.


From a repository-wide duplicate/overlap audit of every pull request open against main, re-run against the current tree (origin/main 680baf9448dc). This PR is not being closed — the note is informational. If the reading is wrong, please correct the reasoning rather than just the conclusion.

@rnoack1
rnoack1 force-pushed the feat/autonudge-egress-hardening branch 2 times, most recently from 4b3f1b1 to d181a9c Compare September 3, 2026 00:27
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 3, 2026
@rnoack1
rnoack1 force-pushed the feat/autonudge-egress-hardening branch from d181a9c to c7299bd Compare September 3, 2026 01:50
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of f717b1516040fd6290540f6168e604cf171bc221 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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.

Watch

Bundling defeats revertability. The sink scrub, the ten-channel persist-time sweep ("a behaviour change for ORDINARY CHAT on all of them"), the two-phase inflight_cycle claim, the expect_fingerprint concurrency gate, and the quarantine sidecar are five separable designs; a redaction false-positive regression on ordinary chat cannot be reverted without also reverting the lost-cycle fix and the lost-update guard. The repo's own rule is one logical change per commit, at most two commits per PR.
Clears when: the declared riders (channel sweep, cycle claim, fingerprint gate) land as separate PRs, or a human accepts the bundle explicitly.

Rewritten user rows are permanent and carry no marker. Scrubbing in the callers means _redact_at_write_boundary early-outs on role == "user", so a false-positive rewrite of a user's own text — the highest-stakes row, on ten channels — leaves no content_redacted flag, while every other role gets one; the doc itself calls this "the transparency cost." The two stated reasons for rejecting centralization contradict each other, and the test docstring's reason ("append also rehydrates a slot back into model context") is wrong — append is a writer, and resume reads the same scrubbed bytes under either shape; only the dashboard write-back exemption is real, and an opt-out parameter covers it.
Clears when: the user-row scrub moves into the write boundary with a dashboard opt-out (marker included), or the caller-side shape sets the marker on rewritten user rows.

Suggestions

The 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 _unparsed_rows carry-verbatim-unarmed shape covers the live-serving harm with none of that machinery.

[DESIGN-REVIEWED] f717b15

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of f717b1516040fd6290540f6168e604cf171bc221 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • Capture script and component disagree on the "safe choice": the effect focuses autonudge-dismiss-overwrite ("Cancel (save nothing)") while capture-autonudge-redaction.mjs asserts autonudge-decline-overwrite ("Keep stored goal") is focused — at HEAD frame 02 would fail its own assertion, so the committed 02 frames (sha ee5efa8, not HEAD f717b15) show a focus target the shipped code no longer produces. Re-run the capture at HEAD with the assertion pointed at the dismiss button.
  • Vertical overflow risk: the script's own comment says "the gate grew notice rows … until the confirm button fell below the fold," fixed by a 900px capture viewport. Real popovers on short laptop viewports get no such fix; with redacted notice + question + preview + three stacked buttons the destructive control can sit off-screen. Give the confirm stack a max-height/scroll like the preview's max-h-32.

Evidence gaps

  • Every new popover state — redacted-goal notice, the three-button confirm gate (Cancel / Keep stored goal / Overwrite), moved-goal preview, re-armed notice, ignored-fields and kept-goal notices, narrow clamp — exists only as fork-hosted screenshots not materialized here, and no blind read ran; push the branch to this repo to close it.
  • The two adjacent decline arms, "Cancel (save nothing)" vs "Keep stored goal (save other settings)", are exactly the pair a cold reader may not tell apart — only a blind read can settle it.
  • The escape-discard warning (autonudge-escape-warned, "Close again to discard your edit…") appears in no screenshot at any sha; the capture script never exercises it — add a frame.

[UX-REVIEWED] f717b15

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of f717b1516040fd6290540f6168e604cf171bc221 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All lenses are run and counts verified. Final review:

First-Principles-Verdict: CONCERNS

Three riders carry most of the permanent surface: a zero-consumer content_redacted row key, an unrelated inflight_cycle store change, and cross-process sidecar machinery.

Not justified as shipped

  • 4. Goal fingerprint / 409 stale-baseline — rides along: declared, but it is a general lost-update guard shipping permanent wire+store surface (goal_token, expect_fingerprint, a 409 code, two SKILL docs) beyond the redaction thesis.
  • 5. Quarantine sidecar — oversized: the _sidecar_transaction flock + _sidecar_seen ownership set defend a concurrent second writer, while _write_state still replaces autonudge.json last-writer-wins — the peer scenario it guards already loses live loops.
  • 7. Cycle claim/rollback (inflight_cycle) — rides along: a store-format change to cycle billing, independent of redaction; a failed claim persist now refuses delivery outright, a new availability trade the redaction fix doesn't need.
  • 8. content_redacted on transcript rows — zero consumers: grep for content_redacted finds no reader (the only hits are the unrelated file-stream API in dashboard/handlers/files.py); a persisted key only a test looks at.
  • 10. Reformat/comment sweep — rides along, undeclared: wholesale black reformat of eval/runner.py and test/test_history.py (both removed from .github/black-baseline.txt) plus issue-number comment deletions in five modules.

What this change ships

Inventory (10 items) — 5 justified

Intent: stop credential-shaped loop text from leaving the backend unscrubbed at its read surfaces — a FIX.

  1. Loop text is scrubbed on every REST response and the autonudge_state broadcast, one shared rule — justified
  2. Every channel's inbound user row is scrubbed before persisting (10 channels + shared helper + taskrunner + eval); deferral from ConversationLog.append stated and test-pinned — justified
  3. A masked goal is flagged (message_redacted), explained in the popover, and an echoed mask is dropped with message_ignored — justified
  4. Goal edits carry a fingerprint; stale ones answer 409, popover gained an overwrite-confirm gate — rides along (declared; non-trivial permanent surface)
  5. Credential-shaped id/slot_key rows are held in a new autonudge.quarantine.json (+.lock, .corrupt-<ts>) — oversized (peer-writer apparatus exceeds the main store's own guarantees)
  6. An unvettable store refuses arming and all writes (503s) instead of overwriting it — justified
  7. Cycles are claimed on disk before delivery and billed only when delivered — rides along (independent store-format change)
  8. Boundary-scrubbed rows persist content_redacted: true — zero consumers
  9. Store-sourced values in autonudge logs are repr-escaped and context-scrubbed; malformed rows carried, not deleted — justified
  10. Whole-file reformats and issue-number comment cleanups in six files — rides along, undeclared

Watch

  • Item 5: the sidecar lock + _sidecar_seen ownership licence protect never-armed, attacker-shaped rows against a second gateway process, a scenario in which _write_state's plain replace already clobbers healthy loops. Clears when: the cross-process half is dropped to the single-writer assumption the main store makes, or a two-writer deployment is named as supported.
  • Item 7: independent of the fix and it adds a refusal path on the fire loop (claim persist failure → no delivery). Clears when: it ships as its own change, or a linked report of a double-delivered/double-billed cycle grounds it in this one.
  • Item 4: the echo-overwrite defect is already closed by message_is_echoed_projection plus the popover dirty check; the fingerprint guards the residual two-window race at the cost of irrevocable API surface. Clears when: the popover remains its only client and the field is kept optional as shipped — confirmed by the author — or a second consumer lands.

Subtractions

  • Drop content_redacted from ConversationLog.append (history.py) — zero consumers counted (grep content_redacted); the write-boundary warning log already records the rewrite.
  • Drop _sidecar_transaction/_sidecar_seen in autonudge.py; keep additive-write-then-compact from self._quarantined alone — the peer it protects against is unsupported everywhere else in this store.
  • Defer _delivering_claim/rollback_undurable/inflight_cycle to a separate change.

[FIRST-PRINCIPLES-REVIEWED] f717b15

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed f717b1516040fd6290540f6168e604cf171bc221 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] f717b15

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed f717b1516040fd6290540f6168e604cf171bc221 via the fork AI-review pipeline; updated in place on each push.

Review details

FINDING -- src/kiro_crew/dashboard/handlers/autonudge.py:170 -- "continue" retains the duplicate raw goal_token wire field -> Fix: pop goal_token before continuing.
[GPT-REVIEWED] f717b15

@rnoack1
rnoack1 force-pushed the feat/autonudge-egress-hardening branch from c7299bd to 072ba97 Compare September 3, 2026 04:08
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@rnoack1
rnoack1 force-pushed the feat/autonudge-egress-hardening branch from 5adff5e to c08cf86 Compare September 3, 2026 10:40
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@rnoack1
rnoack1 force-pushed the feat/autonudge-egress-hardening branch from c08cf86 to 6acc32f Compare September 3, 2026 11:33
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@rnoack1
rnoack1 force-pushed the feat/autonudge-egress-hardening branch from 6acc32f to 489dc99 Compare September 3, 2026 12:48
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@rnoack1
rnoack1 force-pushed the feat/autonudge-egress-hardening branch from 489dc99 to 4117cdf Compare September 3, 2026 13:56
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@rnoack1
rnoack1 force-pushed the feat/autonudge-egress-hardening branch from 4117cdf to b5adecf Compare September 3, 2026 14:53
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • PR #7777 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7777: MERGE_DISCUSSION. Same author, same merge base (31b737e), both OPEN, 7 shared files, and hard textual conflicts in 4 of them (injected-messages.md, autonudge.py, autonudge_authz.py, slack/gateway.py) per git merge-tree. Goals differ, so neither should be closed — but they must be sequenced, and whichever lands second has to re-derive _fire_dashboard_nudge and drop its duplicate get_by_id and NON_EGRESS_REDACTION_MODULES entry rather than resolve the conflict by picking a side. Files: src/kiro_crew/slack/gateway.py, src/kiro_crew/autonudge.py, src/kiro_crew/security_posture.py.
  • This PR is OVERLAPPING with PR #5185. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7996: MERGE_DISCUSSION. A real but purely mechanical popover conflict with an unrelated goal. Whichever lands second resolves one file; nothing about either PR's behavior needs the other. Files: website/src/components/AutoNudgePopover.tsx, src/kiro_crew/autonudge.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

chenmingwei23
chenmingwei23 previously approved these changes Sep 7, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

@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.

  1. src/kiro_crew/slack/gateway.py, the autonudge_state loop_payload in GatewayOrchestrator._observer: this PR forwards stopped_reason only inside the is_structured_monitor_loop branch, as the base does. fix(dashboard): keep goal Save from restarting loops #9052 hoists stopped_reason into the common payload and deletes it from the structured branch, because its popover derives the restart decision from it.

  2. website/src/components/AutoNudgePopover.tsx, save(): this PR rebuilds the PATCH body as { idle_secs, max_cycles, active: true } with conditional message and expect_fingerprint, keeping active: true unconditional. That is exactly what fix(dashboard): keep goal Save from restarting loops #9052 makes conditional, so Save stops re-arming a loop that cannot resume.

Also shared, mechanically only: website/src/components/autoNudgeLoop.ts and website/src/test/AutoNudgePopover.test.tsx. The src/kiro_crew/autonudge.py contact is comment-only in #9052, and the Save/Start-loop label change is #9052's alone.

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 active: true and its stopped_reason placement. Whichever lands second should re-apply the other's semantics rather than keep its own side. Neither should be closed, the goals differ.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

@rnoack1

rnoack1 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@bolichen97 Re-checked every item from both audit notes at the current head
b01a134d5eb01e65c4037f4ece9e1323ed2cdedc, as you asked. Three are settled since you wrote, two of your
readings are still accurate here, and one is still open.

Settled since the audit

  • i18n key ordering (2026-09-02, item 4) — fixed. All 12 hand-edited catalogues now carry
    the popover block in alphabetical order: confirm_overwrite_question precedes
    message_redacted_notice, 28 keys in en.json. I checked ordering programmatically in every
    catalogue rather than by eye, with a control confirming a deliberately wrong order is caught.
  • The TestSentinelPathRepair::test_unnormalized_path_escaping_legacy_is_preserved claim
    (item 3) — you were right not to take it on trust, and it is now verified. I ran that test in
    a detached worktree at an unmodified upstream main tip
    (451761b8648b1aa5537b2d8ad88ce0e79eb96fcd) and it fails there with the same assertion, so
    the failure does not originate in this branch.
  • _fire_dashboard_nudge (your "part that needs care") — both behaviours survive at this
    head, so the semantic collision did not silently drop either side.
    src/kiro_crew/slack/gateway.py:6316-6631 holds the function, and that span contains both the
    egress scrub and the banner handling (17 banner references inside it).

Your two 2026-09-08 readings — both still accurate at this head

  1. stopped_reason is still forwarded only inside the structured-monitor branch:
    src/kiro_crew/slack/gateway.py:6843 is if is_structured_monitor_loop(loop): and :6848
    sets loop_payload["stopped_reason"] inside that branch.
  2. save() still sends active: true unconditionally:
    website/src/components/AutoNudgePopover.tsx:281 builds
    { idle_secs, max_cycles, active: true }, with message conditional at :283 and
    expect_fingerprint at :287.

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
capture harness and the frames. I have not edited the description yet because a body edit
cancels the in-flight checks on a fork; it needs doing while the tree is idle.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants