fix(dashboard): name every field GET /api/agents ships, instead of spreading the record

[chenmingwei23]

Problem / Motivation

GET /api/agents built every roster row with a dataclass spread:

{"name": name, "scope": "global", **dataclasses.asdict(agent_cfg)}

So the endpoint's response contract was not a list of fields anyone chose -- it was "every field KiroCrewAgentConfig has today, plus every field anyone adds tomorrow", automatically. The project-scope rows had a second copy of the same spread (base = dataclasses.asdict(KiroCrewAgentConfig())), so the two sources could also drift into different key sets.

handlers/members.py made the opposite call for GET /api/members and wrote down why:

Explicit allowlist -- never a dataclass spread. The response is a network-boundary contract: spreading AgentConfig would ship every future field (including a credential-shaped one) to the roster endpoint automatically.

The defect is the pattern, not any single field. Measured, not inferred -- the response shipped 14 keys (12 record fields + handler-added name and scope), and none of the 12 is a credential. The bug is that nothing stood between a field being added to the record and that field reaching the browser.

Why it matters

Two things make the direction matter more than the field list of the day:

1. The next field decides itself. Whoever adds a field to KiroCrewAgentConfig -- for a scheduler knob, a resolved path, a capability hint -- ships it to every caller of this endpoint without ever opening agents.py. Nobody reviews a decision that was never made.
2. The caller set is wider than the dashboard. This handler carries neither _require_owner nor _deny_app_caller, unlike /api/members (which 404s app-token callers) and unlike POST /api/agents/sync (owner-gated). Two shipped app manifests declare /api/agents in their api allowlist (static/dist/apps/agent-worlds/app.json, static/dist/apps/demo-app/app.json), and the app-kit docs and federated-app RFC use it as their worked example. So installed third-party apps are real, present callers.

Per the issue's framing: this changes the severity, not the fix. The endpoint is not anonymous, and a narrower reading -- "only the owner's browser ever sees this" -- would still not justify keeping the spread, because the spread is what makes the next field's exposure unreviewed.

What is sensitive here (naming fields, never values)

• No credential field exists on this record. All 12 fields are str/float; there is no token, secret, or key field. Nothing in this PR, its tests, or this description contains a real secret value.
• No host filesystem path leaves through this row. workspace and memory_store are names of workspaces / memory_stores config entries. The actual directory lives in WorkspaceConfig.dir, which this endpoint does not ship, and still does not.
• telegram_account is the one field naming an external messaging-account binding. It is deprecated and inert (nothing reads it; it is preserved only so an existing config is not rewritten). Withheld.
• description and triggers are free text an agent can write through config.json, and description is additionally package-controlled (agent sync copies it off a discovered spec). Both fields are kept -- the picker renders description and select_crew routing depends on triggers -- and both are scrubbed on the way out, with the write-side rule below keeping that scrub from destroying the stored value.

What changed (motivation -> approach -> change)

Symptom -- the response ships whatever the record holds, in whatever shape it holds it. Cause -- the row was built by spreading the record, so both which fields ship and what their values are went unexamined. Change -- two independent halves at one seam:

Key half. One _agent_roster_row() allowlist that names each shipped field, used by both row sources, so the whole response has exactly one key set and adding a field is opt-in:

name  scope  kiro_agent  workspace  memory_store  model
reasoning_effort  description  triggers  source  session_color

That converts "everything unless someone remembers to exclude it" into "nothing unless someone adds it" -- the direction that stays correct as the record grows.

Value half: mask, plus a write rule that recognises the mask. Neither half is correct alone.

Read half -- every record value goes through _roster_mask. A value the redactors would alter (credential- or exfiltration-URL-shaped text), or a non-string the loader let through, is replaced wholesale by _SENSITIVE_MASK -- the sentinel _masked_config_dict already uses for the same job on GET /api/config/kirocrew. Benign content is byte-identical. They are all untrusted: an agent can edit config.json directly, and _do_agents_sync copies description straight off a discovered agent spec (cfg.agents[disc.name] = KiroCrewAgentConfig(..., description=disc.description, ...)), so a third-party package controls that string.

Write half -- api_kirocrew_agent_update drops any body field carrying the mask, treating it as unchanged:

body = {key: val for key, val in body.items() if not _carries_mask(val)}

Without it the read half destroys stored config: this response feeds a read-modify-write (openEdit seeds the agents-page edit sheet from a roster row, saveEdit sends every field on every save so '' can clear a pin), so the mask would be persisted over the operator's original. This is the remedy _masked_config_dict's docstring prescribes verbatim: "if one is ever added it MUST treat _SENSITIVE_MASK as 'unchanged' and keep the stored value".

A fixed sentinel rather than redacting in place is the load-bearing choice, and it is the one thing in this PR that took the longest to get right. An in-place scrub makes the browser's view a function of the stored value, so the write rule has to recognise it by recomputing that transform -- which breaks two ways a sentinel does not:

• Redaction-chain drift. fix(dashboard): redact crew record strings at both roster serializers #8465 wraps this same response in redact_record_strings, whose order differs from _redact_external's. A recomputed-equality rule stops matching and silently persists the redacted text. An exact sentinel survives, because scrubbing ******** leaves it unchanged. This dissolves the fix(dashboard): redact crew record strings at both roster serializers #8465 collision earlier revisions of this description escalated as needing a human.
• Stale-view skew. If the stored value changes between the GET and the PUT (an agent editing config.json, a second dashboard tab), a recomputed rule compares the old view against the NEW value, fails to match, and writes [REDACTED ...] into the config as though the operator had typed it. The sentinel does not depend on the stored value at all. test_a_stale_view_cannot_corrupt_the_config pins this.

Two consequences, named rather than buried:

• A value containing one credential-shaped token is masked entirely, so the owner loses the benign remainder rather than seeing it partially redacted. Same trade _masked_config_dict already makes; the price of a view that cannot be mistaken for content.
• Accepted residual: an operator cannot store the mask string itself (eight U+2022 bullets). Identical to the config endpoint's residual.

Masking a non-string rather than coercing it to "" is what lets the write rule preserve it -- an echoed "" would read as a genuine edit and overwrite the stored value, which was a real defect in an earlier revision of this PR.

Why the rule is keyed on the backend, not the client. An earlier revision exempted the seven fields the agents page happens to send today. That encoded a claim about the CLIENT which this side could not enforce -- and it was already wrong: api_kirocrew_agent_update accepts nine record fields including description and source, the two exempted on the grounds that no write path accepted them.

name is the single exemption, and only for the owner. It travels in the URL rather than the body, so the write rule structurally cannot protect it, and masking it would make /api/agents/{name} unaddressable for edit and delete (it also keys the usage sort). More than addressable: renaming is the remediation for a credential-shaped name, so masking it from the owner would remove the only route out of the bad state. A non-owner, an app token, and every project row get it masked. scope is a handler literal and is never touched. dict[str, str] is now true rather than aspirational.

The caller class is the OWNER predicate, not an app-token check. bool(request.get("app", "")) asks "is this an app?", and a non-owner DASHBOARD session answers no -- an allow-listed messaging user running !dashboard holds a dashboard token with app == "". redact is therefore not is_owner_dashboard_request(request), the same predicate _require_owner resolves to for this module's mutating routes, and it fails CLOSED when there is no state to resolve an owner against.

The hazard is closed at creation, not at one read site

GPT 5.6 asked for _roster_mask(name) on every caller including the owner. Declined, with reasons in a PR comment; what ships instead refuses a credential-shaped name where it comes to exist:

• _name_would_be_masked (agents.py:2648) is keyed on _roster_mask itself, not a second detector, so the create rule and the read rule cannot drift: what the roster would mask is exactly what creation refuses.
• api_kirocrew_agents_create (agents.py:3187) returns 400 credential_shaped_name, and does NOT echo the rejected name -- reflecting it into a body and thence the request log is the disclosure being prevented.
• PUT cannot rename (the name comes from match_info; there is no new_name), so creation is the only user-facing way in.

Masking a read closes one of N surfaces, measurably: _do_agents_sync prints a crew name into a log line at agents.py:2902, which no response mask touches.

What this does NOT cover, stated because the boundary is real. A crew already in config.json is not retroactively renamed or refused; a name hand-written into config.json bypasses the check (there is no load-time rejection, and refusing to load a config would take the dashboard down over a naming problem); and _do_agents_sync writes cfg.agents[disc.name] at agents.py:2910 from a discovered spec without this check, those names being package-controlled rather than user-pasted. Closed for new crews created through the API; open for those three.

The alternative First Principles asked to see weighed -- scrub=redact for every field, deleting the write rule entirely, since app tokens cannot reach the owner-gated PUT -- is genuinely simpler and was the r4 design. It is rejected because it leaves package-controlled description reaching the owner unmasked, contradicting the control discover.py:53 records for exactly that class of string, and GPT 5.6 blocked it on 5aa800a0d for that reason. The trade is: one silent-no-op semantic on PUT (a field echoed back unchanged) against owner-visible unredacted third-party text. This PR takes the first; a maintainer who prefers the second can have it by deleting _carries_mask and passing redact through, and the tests name which ones would need to go.

Review history, since the value half changed five times

Every revision was driven by a finding, and three corrected errors of mine:

1. r1 -- allowlist only, values untouched. GPT blocked: agent-writable free text reaches the browser unredacted.
2. r2 -- unconditional in-place scrub, no write rule. Design and GPT convicted it on the same mechanism (the scrubbed value round-trips through saveEdit and overwrites stored config); First Principles noted a value-half asymmetry with /api/members.
3. r3 -- scrub removed, arguing it closed nothing because /api/config/kirocrew serves the same records unmasked. Wrong for app tokens, the caller GPT had named: app-token scope cannot reach that endpoint.
4. r4 -- split per caller class. Design found the field the split still exempted (name for apps); GPT found description is package-controlled.
5. r5 -- per-field rule keyed on the frontend's payload. Design showed the guard was backend-side convention for a client-side invariant; checking the PUT handler showed the exemption was already wrong.
6. r6 -- uniform in-place scrub plus a write rule keyed on the backend's accepted fields. I had declined that rule twice claiming it belonged to Crew record strings (avatar traits, description, triggers) reach dashboard JSON without credential redaction #8447's chokepoint; wrong, since /api/members has no write path. GPT went green here.
7. r7 (current) -- Design's suggested sentinel replaces the recomputed predicate, killing both its stale-view and chain-drift failure modes.

The ratchet caught a live base change, which is what it is for

avatar landed on main hours after this branch's base was cut, and the omission ratchet
went red naming the decision instead of just failing:

KiroCrewAgentConfig grew ['avatar']. GET /api/agents is an explicit allowlist (#8454),
so decide deliberately: add the field to _agent_roster_row AND ROSTER_ROW_KEYS if the
dashboard needs it, or to WITHHELD_RECORD_FIELDS if it must not leave the process.

Decision: avatar SHIPS. AgentSelector.tsx declares avatar?: unknown commented
"verbatim from the backend", and main's roster still ships it through the asdict spread
this PR replaces -- so the field reaches the dashboard today and withholding it would
regress a live feature rather than narrow a disclosure. What it holds was read before
choosing: a dict, never bytes and never a data URI (so no payload-size change), either
{"kind":"ghost","traits":{...}} or {"kind":"image","v":<int>,"file":"<16-hex>.<ext>"},
and no host path -- the picture's bytes live under the data home's agent-fenced
run/avatars/ dir and file is a digest-named basename.

Shape allowlist, with masking confined to traits values. _roster_avatar runs
_safe_avatar (the config's own validator, so junk collapses to {}) and then masks only
the traits values. kind, v and file pass through as the pinned shapes they are:

• Mask kind and the dashboard can no longer tell a ghost from an uploaded picture.
• Mask file and the per-crew avatar endpoint resolves nothing -- the image silently
  breaks. file is pinned by _AVATAR_FILE_PIN_RE to <16-hex>.<ext>, and a value
  constrained by a regex is safer than a masked one: the pin refuses a bad value, where
  masking destroys a good one.
• traits values are the only user-authored strings, so they take the same _roster_mask
  as every other roster string. The renderer resolves an unrecognized trait to absent, so a
  masked trait degrades that axis instead of breaking the face.

A blanket mask would have shipped, through the mask, the exact regression that withholding
the field would have shipped through the allowlist.

Verified in both directions, because a masking-only test passes just as well against a
blanket. Over the wire from the registered handler: {"kind":"image","v":17,"file": "0123456789abcdef.png"} intact, while a traits.eyes carrying AKIA... comes back as the
sentinel and a bool trait is untouched.

Honest limit: because _safe_avatar already pins every non-traits leaf to a shape the
redactors do not alter, a blanket mask behaves the SAME as the targeted one today -- a
mutation that masks every leaf does not redden the suite. The targeted rule is chosen for
intent and for the day that pin loosens, not because a live defect separates them.

A ratchet whose probe rebuilds the class can manufacture failures

The same push produced a second red -- TypeError: non-default argument 'avatar' follows default argument -- and it was the probe's, not the record's. avatar uses
default_factory=dict (a mutable default must), so f.default is dataclasses.MISSING, and
the probe rebuilt every field as field(default=f.default), turning MISSING into no
default. A non-default field after defaulted ones is a hard error at class-creation time.

Determined by construction rather than by reading: on clean origin/main with no
scaffolding, KiroCrewAgentConfig() imports and instantiates fine (13 fields, avatar == {}); the same probe reproduces the TypeError against that class; preserving
default_factory builds it. Nothing to file against main. The general lesson, now
recorded in the probe itself: a ratchet that REBUILDS the class it inspects can report a
defect it created.

Consumer inventory (what I checked, and what a positive would have looked like)

Every field I dropped, I looked for as a bare identifier across all of website/src (.ts + .tsx, production and test). A positive would have been any occurrence at all: a property read (a.telegram_account), a destructure, an index (row['telegram_account']), a test fixture, or a declaration in the row's TS interface. Result -- 0 occurrences each:

Field	Occurrences in website/src	Verdict
watchdog_tool_stall_suspect_secs	0	dropped
watchdog_tool_stall_hard_cap_secs	0	dropped
telegram_account	0	dropped

Corroborated structurally rather than by grep alone: the row's own TS interface KiroCrewAgent (website/src/components/AgentSelector.tsx:12) declares exactly 10 fields and does not declare any of the three, so a consumer reading one would already be a type error. I also checked the paths a grep on field names cannot see:

• Every kirocrewAgents() call site -- KiroCrewAgentsPage.tsx:698, AgentsPage.tsx:469, MembersPage.tsx:221, and the shared useAgents hook. None casts the row to any or a widened type.
• The edit-sheet round-trip, the real regression risk. KiroCrewAgentsPage.tsx populates its editor from a roster row (editingAgent = agents.find(...)), so a dropped field could in principle be written back as empty. It reads exactly these off it -- kiro_agent, workspace, memory_store, model, reasoning_effort, description, triggers, source, session_color -- all allowlisted, and its AgentUpdatePayload names 7 explicit fields with no spread. Nothing I dropped can reach a write.
• The whole row handed to a provider function -- AgentSelector.tsx:308 passes the row into provider.resolveAgentTemplate(a). Its parameter type AgentBinding declares only name / kiro_agent? / workspace? / memory_store? and the implementation returns agent.kiro_agent || agent.name. All allowlisted.
• scope is KEPT despite having no production consumer -- the only reference is a test fixture (useAgents.projectScope.test.ts:49) and it is not in the TS interface. It is not unexplained, though: the handler adds it deliberately as Discover project-level Kiro agents (.kiro/agents/) everywhere, not just in Slack #1684's project-scope tag and documents it. A handler-authored, documented field is not the same as a record field arriving by accident, so it stays.
• The two shipped apps that declare /api/agents ARE in the repo, and I had been searching the wrong tree. My grep scope above was website/src; their sources live under website/public/apps/*/ui/index.mjs, which that scope never covered -- so "zero consumers" was, for them, an unmeasured claim dressed as a measured one. Corrected: across website/public/apps/ and docs/app-kit/, the three dropped fields have 0 hits, demo-app/ui/index.mjs:19 calls api.get('/api/agents'), and the only roster field either app reads is name (3 occurrences, no other field). agent-worlds declares the route in its manifest without calling it. So the conclusion is unchanged and now actually measured. The remaining honest limit is narrower than before: an app installed from outside this repo cannot be inspected from here, which is why the allowlist is 11 fields and not 4 -- I removed only fields with a zero-consumer result and no interface declaration and no plausible render (two backend scheduling knobs and one inert deprecated binding), and kept anything ambiguous.

No frontend change is needed and none is included: this only removes keys nothing reads, and no fixture sets them. Every retained value ships byte-identical to what is stored.

Interaction with #8447 / PR #8465

#8454's acceptance sketch asks to keep the redact_record_strings pass. That chokepoint does not exist on main -- it arrives with #8447's fix, open as PR #8465 -- so this PR cannot keep it, and does not remove or bypass it.

The two are textually disjoint -- #8465 wraps the finished list at the return plus one import, this PR changes only row construction ~25 lines above -- so whichever lands second needs no reconciliation, and #8465's pass will scrub the allowlisted values this PR narrowed the row to.

One thing #8465 should know, surfaced by this PR's round 2: wrapping this response in a redaction pass inherits the read-modify-write hazard described above. saveEdit returns seven of these fields unconditionally, so a scrubbed triggers will be persisted over the operator's original on the next save. That is not an argument against the chokepoint -- it is the right place for it -- but the chokepoint needs the write-path "unchanged" rule that _masked_config_dict's docstring prescribes. Raised here rather than on #8465 so I am not editing another author's PR; Design Review made the same point ("whichever lands first should own the answer").

Tests

test/test_agents_roster_contract.py, 27 tests.

Key half

1. test_global_row_ships_exactly_the_allowlist / test_project_row_ships_the_same_key_set -- endpoint-level, both row sources. The fixture sets every record field, giving each withheld one a distinctive non-default value so asserting its absence proves the allowlist rather than a falsy default, and pins that the two sources share one key set.
2. test_record_field_added_later_is_not_shipped_by_omission -- the ratchet the issue asks for. Every dataclasses.fields(KiroCrewAgentConfig) name must be allowlisted or explicitly withheld, so adding a field to the record fails this test with a message telling the author to decide. It guards its own guard: the withheld names must be real fields and the allowlist must not claim a field that no longer exists.
3. test_an_attribute_the_allowlist_does_not_name_is_dropped -- behavioral, not a literal comparison: a synthetic record carrying an extra attribute serializes to the same key set.
4. test_app_token_caller_gets_the_same_keys_with_scrubbed_values -- endpoint-level through a middleware setting request["app"], so the caller wiring is covered, not just the row function's keyword.

Value half, read side

5. test_no_record_field_ships_a_credential_shaped_value (both callers) and test_a_non_string_is_masked_not_emptied (parametrized over the five fields the loader does not coerce) -- nothing unshowable leaves as content, and a malformed value is masked rather than emptied so the write rule can preserve it.
6. test_every_value_is_a_string, test_benign_content_is_never_altered (both callers), test_owner_keeps_name_addressable_but_app_token_does_not_need_it, test_both_caller_classes_ship_the_same_key_set.

Value half, write side

7. test_end_to_end_read_then_write_preserves_the_stored_value -- over HTTP: GET the roster, assert the value arrived masked, echo the row back the way saveEdit does (every field, always), assert the stored value is byte-identical afterwards. The round-trip defect as an executable test rather than an argument.
8. test_a_stale_view_cannot_corrupt_the_config -- rewrites the stored value between the GET and the PUT, then echoes the stale masked row. Pins the failure mode a recomputed-equality predicate had.
9. test_a_real_edit_still_writes_through -- the counterweight: the rule must not swallow an actual change.
10. test_the_sentinel_is_the_one_the_config_endpoint_uses -- the mask is core._SENSITIVE_MASK, not a private copy, so drift cannot silently break the round-trip rule. test_real_content_is_not_treated_as_the_mask -- "", plain text and a dict are not the mask.

Mutation-verified eleven ways, one per distinct defect: dropping the write-side filter reddens 2 (the round-trip and stale-view tests); dropping the read-side mask reddens 17; replacing the sentinel predicate with a recomputed/substring one reddens 16; coercing a non-string to "" instead of masking reddens 10; leaving name unmasked for app tokens reddens 1; reinstating the dataclasses.asdict spread reddens 6; narrowing the write rule from containment to exact equality reddens 2 (an operator appending to a rendered mask); swapping the owner predicate back for the app-token check reddens 2; removing the creation-time name check reddens 1; shipping the avatar dict without _safe_avatar reddens 1; dropping avatar from the row reddens 9.

Manual verification

N/A -- unit coverage sufficient, and deliberately so: the assertion is about the exact JSON key set and value shape at the HTTP boundary, which the endpoint-level tests measure directly through TestClient rather than approximate. There is no UI change to look at (three keys nothing renders were removed; every retained value is byte-identical), so a screenshot would show an unchanged page.

Gates run: repo black gate passed, flake8 clean, isort --check-only clean, mypy on agents.py clean (the 2 reported errors are pre-existing in src/kiro_crew/transcribe.py, untouched here). Tests: 121 passing across test_agents_roster_contract.py, test_api_agents_order.py, test_config_api.py and test_security_posture.py -- scoped to the touched files rather than the full suite. test_config_api.py's existing CRUD round-trips assert on name / kiro_agent / workspace / memory_store and still pass unchanged.

Related Issues

Fixes #8454
Refs #8447 (the field-generic redaction chokepoint; PR #8465)

Pattern harvest

Rule candidate: semgrep
Pattern: whole-record spread (**dataclasses.asdict(...)) inside a value returned to an HTTP response boundary -- makes the response contract "every field the record grows", so field exposure is opt-out and silent.

This one generalizes, and the census is small enough to state exactly. After this PR no **dataclasses.asdict spread remains in dashboard/handlers/. Two same-class sites remain, neither touched here:

• dashboard/handlers/autonudge.py:105 -- _serialize() does payload = asdict(loop) and then pops a field (payload.pop("monitor", None)). Same defect class in its purest form: exclusion-based serialization at a response boundary, where a field added to the record ships unless someone remembers to pop it. The strongest candidate for the same conversion. Evidence that it is a growing service record rather than a response DTO: NudgeLoop is declared in the service module (src/kiro_crew/autonudge.py:433) with 16 fields -- id, slot_key, message, idle_secs, max_cycles, cycle_count, active, last_fire_ts, created_ts, stop_sentinel_path, max_runtime_secs, gate, stopped_reason, approval_stalled, next_due_ts, monitor -- and stop_sentinel_path is a filesystem path reaching the browser, concretely the field class this PR's own rationale warns about. Reached through /api/autonudge listing plus three write responses, so four call sites inherit it.
• apps/builtins/meetings/backend/providers/tasks.py:197 -- **asdict(draft) into a ledger entry. Same shape, but a persistence boundary rather than a network one, so the consequence differs; a rule should classify it separately rather than lump it in.

A semgrep rule keyed on a dataclass spread reaching a web.json_response(...) value would have caught this endpoint and would catch autonudge.py. Left as a candidate rather than done here: writing the rule and converting a second endpoint are each their own change with their own consumer inventory, and this PR's whole point is that a response-contract change needs one.

Second candidate, from GPT 5.6 round 1: a dataclass field annotated str is not a guarantee at a serialization boundary, because the config loader enforces the annotation for only some fields. Any handler that types its output dict[str, str] from record attributes is asserting something the loader does not check. Lint/review-prompt rather than semgrep -- it needs the loader's per-field behavior, which is not local to the call site.

Third candidate, from Design Review round 2, and the most generalizable of the three: a read-side transform is unsafe on any response that feeds a read-modify-write. Redaction, masking and normalization are all read-side transforms, and this codebase has both shapes -- _masked_config_dict is safe only because no write endpoint accepts its output, and its docstring says so; round 1 of this PR added a transform to a response whose values the agents page writes straight back. The rule generalizes as: if a GET's values are ever POSTed/PUT back, either the transform must be absent or the write path must treat the transformed value as "unchanged". Review-prompt candidate, because deciding it needs the client's write behavior, which no backend-local rule can see -- and it is the class that would have caught this PR's own round-1 regression, #8465's coming one, and any future masked field on a settings surface.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) -- N/A: the behavior is documented in the handler docstrings and the pinning tests, and no spec describes this response's key set
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 2ab8bd64572e8615c05eb7c21210690ddd02ff50 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design review complete. The diff replaces an asdict spread on GET /api/agents with an explicit 11-key allowlist, masks agent-writable values behind the existing _SENSITIVE_MASK sentinel, pairs it with a write-side drop on update, and refuses credential-shaped names at both creation sources. I verified the sentinel/uniform-masking rationale holds (the in-flight #8465 redaction chain makes a recompute-based rule genuinely unsafe), that is_owner_dashboard_request is the same predicate the mutating routes use, and that the withheld fields have no frontend consumers. One asymmetry survives scrutiny: the create route accepts the same record fields as update but lacks the _carries_mask drop.

Design-Verdict: PASS

Allowlist plus fixed-sentinel masking mirrors the repo's established members/config patterns, with the write-side pair proven end-to-end and every cost named.

Suggestions

• Apply the same _carries_mask drop to api_kirocrew_agents_create (it accepts description, triggers, source, avatar from the body): a duplicate-from-roster-row flow would otherwise seed a new record with sentinel glyphs as content, and the update-route remedy doesn't cover it.

[DESIGN-REVIEWED] 2ab8bd6

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 2ab8bd64572e8615c05eb7c21210690ddd02ff50 — 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 evidence is gathered. Emitting the review.

First-Principles-Verdict: CONCERNS

The allowlist is the fix and it lands at cause level; the concern is one counted sibling read site the PR itself cites but leaves shipping the same strings raw.

What this change ships

Intent: stop GET /api/agents from shipping whatever fields KiroCrewAgentConfig grows, to callers wider than the owner's browser — a FIX.

1. Roster rows carry a named 12-key set; watchdog windows and telegram_account stop shipping — justified (mirrors the documented members.py rule; withheld fields have 0 consumers in website/src, grepped)
2. Project rows share that exact key set — justified
3. Credential/URL-shaped values arrive as the •••••••• sentinel for every caller, owner included — declared; owner half questioned below
4. Row name masked for apps, non-owner dashboard sessions, and all project rows — declared
5. PUT drops any body field carrying the mask, recursively — declared (write half of item 3)
6. POST /api/agents 400s a credential-shaped name — rides along, but cause-level
7. Agent sync skips a discovered credential-shaped name — rides along, but cause-level
8. Avatar ships shape-allowlisted, only traits masked — justified (AgentSelector.tsx:33 consumes it)
9. An owner edit that keeps-and-appends to the mask is silently dropped — declared cost of item 3
10. Non-string values in str fields ship as the mask — declared

No unfixed spread siblings: \*\*(dataclasses\.)?asdict across src/ yields 1 other hit, a local ledger write (meetings/backend/providers/tasks.py:197), not a response.

Watch

• One unfixed sibling read site. The non-owner dashboard session item 3 masks against can GET /api/config/kirocrew — its GET branch carries no owner gate (core.py:1571-1572) and _masked_config_dict masks only schema-sensitive values, which none of these 12 fields are — and read description/triggers raw. The PR quotes exactly this fact to justify the owner name exemption (agents.py:2807-2809), so the value masking closes one of two read sites for its own stated caller class. Count: 1 sibling.
• Owner-side value masking is what forces the entire write half. PUT is owner-gated (agent.update, agents.py:3452), so masking values only when redact=True would leave no caller who both receives a mask and can write, making _carries_mask, the PUT body filter, and the append-refusal/benign-remainder costs deletable. The counterweight is real — description is package-controlled and discover.py::_redact_external already scrubs publisher text shown to the owner — so this is a premise for a human to weigh, not a blocker.

[FIRST-PRINCIPLES-REVIEWED] 2ab8bd6

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 2ab8bd64572e8615c05eb7c21210690ddd02ff50 — this comment is updated in place on each push.

Review details

I have enough to complete the validation. Let me verify my reasoning on the two candidates against the code I've now opened.

Candidate 1 (owner-visible benign values wholesale-masked): _roster_mask returns the value verbatim unless _redact_external(value) != value. _redact_external only alters strings containing a credential shape (AKIA/xoxb/PEM), an exfiltration URL, or a token=…-style param. For an ordinary owner description this is byte-identical (verified in test_benign_content_is_never_altered). The only "wrong outcome" is that a description literally embedding a credential-shaped token renders as bullets for the owner — which is the explicitly documented, intentional trade mirroring _masked_config_dict. That is a design decision, not a defect; (c) does not resolve to an observable wrong outcome. Drops below 80.

Candidate 2 (legitimate names rejected): rejection requires _redact_external(name) != name, i.e. the name itself contains an AKIA/xoxb/PEM shape or a token=-style param. A normal slug ("oncall-triage", "crew-7") cannot trip redact_credentials (fixed prefixes) or _URL_SECRET_PARAM_RE (needs a <param>= construct). The candidate itself concedes (a) — a concrete legitimate input that trips it — was "not exhaustively enumerated." Without a concrete triggering input, (a) fails. Drops below 80.

Step 2: I checked the new redact line for a crash (request.app.get("state") is None or not is_owner_dashboard_request(request) short-circuits before the predicate's request.app["state"] subscript, so no KeyError), the _carries_mask recursion, the widened sort key (str(...)), and the allowlist/withheld split. Nothing grounds to a reachable, concrete defect on a changed line at 80+.

No findings.

[OPUS-REVIEWED] 2ab8bd6

Verdict parsed from the review's SHA-scoped output markers for commit 2ab8bd64572e8615c05eb7c21210690ddd02ff50.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 2ab8bd64572e8615c05eb7c21210690ddd02ff50: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 2ab8bd64572e8615c05eb7c21210690ddd02ff50 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 2ab8bd6

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 2ab8bd64572e8615c05eb7c21210690ddd02ff50: <one-sentence reason>

[chenmingwei23]

Disposition of the GPT 5.6 blocking finding: accepted and fixed in 03846c367

Fixed, not overridden. The finding is real, and checking it against the live loader made it narrower in one place and wider in four, so the fix is not quite the one prescribed.

What I verified before changing anything

GPT's remedy was "type-check these fields, then scrub them with _redact_external". I checked whether the type-check half was a no-op, since KiroCrewAgentConfig declares every one of these fields as str. It is not a no-op. Loading a config that puts an object under each field in turn, and reading back what the record actually holds:

field	loader result for a non-string input
kiro_agent	dict -- passes through verbatim
workspace	dict -- passes through verbatim
memory_store	dict -- passes through verbatim
description	dict -- passes through verbatim
source	dict -- passes through verbatim
model	'' -- defaulted
reasoning_effort	'' -- defaulted
triggers	'' -- defaulted
session_color	'' -- defaulted

Two consequences:

1. The finding is wider than the two fields it named. Five fields are uncoerced, not two.
2. triggers, one of the two fields it did name, is already loader-defaulted and so was never exposed to the type half. It is still agent-writable free text, so it needs the scrub -- just not the type guard.
3. This half landed on my diff specifically. I annotated _agent_roster_row as -> dict[str, str], and that annotation was false: a dict-valued description would have serialized into the response as a nested JSON object out of a row I documented as flat strings. mypy could not catch it because the dataclass declares str.

The fix

One _roster_str() applied to all nine allowlisted record values: a non-string fails closed to "", then the string goes through _redact_external -- the chain this module already imports and applies to text it does not control (credentials, then exfiltration URLs, then the lexical URL-parameter scrub, in that documented order).

Fail-closed to "" rather than to the field's declared default, deliberately: coercing a malformed workspace to "default" would silently bind the crew to a real workspace its config never named. "" is also the "unset / inherit" spelling all of these fields already document.

Two things I did NOT do, with reasons

• name stays verbatim. It is the row's identity: it addresses /api/agents/{name} for edit and delete, and it keys the usage sort. Scrubbing it would make an affected row unaddressable while closing nothing, because the same name reaches the browser through the config and CRUD surfaces regardless. Stated as a reasoned exclusion rather than an assumed-safe one: crew creation enforces only that the name is non-empty -- the shared _AGENT_NAME_RE grammar guards kiro_agent, not name -- so the value is not constrained to a credential-free shape.
• I did not build a second redaction chokepoint. Crew record strings (avatar traits, description, triggers) reach dashboard JSON without credential redaction #8447's fix (open as fix(dashboard): redact crew record strings at both roster serializers #8465) adds redact_record_strings in handlers/_shared.py and wraps this endpoint's finished list at the return. Creating my own copy of that helper would duplicate the core of someone else's in-flight PR and guarantee a conflict in a shared module. _roster_str instead sits inside _agent_roster_row, a function that exists only in this diff, and uses the redactor agents.py already imports -- so it remains textually disjoint from fix(dashboard): redact crew record strings at both roster serializers #8465 and the two compose (both chains are specified idempotent and byte-identical on clean text, so the later wrap re-scrubbing already-scrubbed values is defense in depth, which is what GET /api/agents ships the whole agent record via dataclass spread — convert to an explicit allowlist like /api/members #8454's acceptance sketch asks for).

Coverage

Three new tests, all mutation-verified against the committed fix:

• the non-string path, parametrized over the five uncoerced fields (removing the isinstance guard reddens 5)
• a credential-shaped value in description/triggers not surviving the serializer, plus benign text in the same fields passing through unchanged (removing the scrub reddens 1)
• name/scope verbatim, so the exclusion above is pinned as a decision rather than left to drift

Reinstating the original dataclass spread reddens 8 of 11. test_security_posture.py passes (53) -- agents.py is already a registered redaction call site, so no posture-registry change was needed. test_config_api.py + test_api_agents_order.py + this file: 52 passed. Black gate, flake8, isort clean; mypy clean on agents.py, and the dict[str, str] annotation is now true rather than aspirational.

[chenmingwei23]

Round 2 on ecba8c5fb: Design Review's finding is right, and it retires the round-1 fix

Design Review (CONCERNS on 03846c367) caught a real defect that I introduced in round 1, and it also invalidates the remedy I applied for GPT 5.6's round-1 block. Both are addressed by the same change: the scrub is gone; the type normalization stays.

Design Review's finding, confirmed against live code

Verbatim as described. openEdit(a) (KiroCrewAgentsPage.tsx:810) seeds the edit sheet from a roster row -- a.kiro_agent, a.workspace, a.memory_store, a.triggers, a.session_color, a.model, a.reasoning_effort -- and saveEdit (line ~892) sends all seven back unconditionally, with comments saying that is deliberate so '' can clear a pin. So "skip the unmodified field" is not available as a fix: it fights an intentional design decision.

Seven of the nine values I scrubbed round-trip. A scrub-altered value persists over the operator's original on the next save of an unrelated field. My PR body had called the round-trip "the real regression risk" and then only checked dropped fields, not scrubbed ones -- the review named that gap precisely, and it was mine.

The repo already states the rule I broke

_masked_config_dict in handlers/core.py masks sensitive values for GET /api/config/kirocrew, and its docstring says why that is safe:

Applied ONLY to the GET response -- never to the value cfg.to_dict() / cfg.save() serialize, since masking there would persist the sentinel and destroy the real secret. Safe here because no config write endpoint accepts sensitive fields; if one is ever added it MUST treat _SENSITIVE_MASK as "unchanged" and keep the stored value.

A read-side transform is safe only while no write path accepts its output. PUT /api/agents/{name} accepts mine and has no such rule, so round 1 violated an invariant this codebase had already written down.

And the scrub did not even close the exposure

Before removing it I checked whether it was buying anything worth a write-path rule. It was not. GET /api/config/kirocrew serves the same caller the same records, masking only schema-sensitive paths -- and no KiroCrewAgentConfig field is marked sensitive. Measured by loading a config with a credential-shaped string in agents.probe.description and agents.probe.triggers and rendering that endpoint's response body:

agents.probe present in /api/config/kirocrew response: True
  description -> 'see AKIA...EXAMPLE'      <- unmasked
  triggers    -> 'use AKIA...EXAMPLE'      <- unmasked

Neither route carries _require_owner or _deny_app_caller, so this is not an owner-vs-app distinction either: any caller that can reach /api/agents can reach /api/config/kirocrew. Scrubbing one of two endpoints that serve the same records to the same caller closes nothing, so round 1 paid a real corruption cost for no reduction in exposure.

What ecba8c5fb does

_roster_str keeps only the type enforcement: a non-string fails closed to "". That half has neither problem -- a non-string was never a usable value for any consumer, the alternative is shipping a nested JSON object out of a row contracted as flat strings, and -> dict[str, str] is a claim the loader does not enforce (five fields pass an object through verbatim). It is also not a data-loss risk in the way a scrub is: the pre-existing behavior was that the sheet rendered a dict into a text input and wrote that back.

The reasoning is in the function's docstring, including both arguments above, so the next reader does not helpfully add the scrub back. test_values_round_trip_verbatim pins it behaviorally: it asserts every written-back field is byte-identical to what is stored, using a credential-shaped probe precisely because that is the value a scrub would alter. Mutation-verified -- restoring the _redact_external call reddens exactly that test.

To GPT 5.6, on the round-1 block

Your premise is true: this endpoint hands agent-writable free text to the browser. I am not overriding it and have used no /ai-review override. But the remedy at this seam is refused on evidence, not preference -- it is ineffective (the measurement above) and destructive (the round-trip above), and the second point is an invariant the codebase states in its own docstring.

Redacting these records is #8447's subject, which is exactly where #8454 left it, and #8465 is already open to do it at one chokepoint covering both roster endpoints. That PR will inherit this same round-trip hazard -- it wraps this response with redact_record_strings, so a scrubbed triggers will round-trip through the same saveEdit -- and it is the right place to solve it, because the write-path "unchanged" rule belongs with the chokepoint rather than being duplicated per endpoint. Design Review's note says the same thing: whichever lands first should own the answer. This PR is the contract half and deliberately does not take that on.

If a maintainer would rather this PR carry the redaction anyway, the honest cost is a write-path change to api_kirocrew_agent_update that treats "value equals the scrubbed form of what is stored" as unchanged -- which will then need reconciling with #8465's different chain. I would rather not build a sentinel convention here that #8447 has to redesign.

Board

Gates on ecba8c5fb: 52 tests passing across test_agents_roster_contract.py (11), test_api_agents_order.py and test_config_api.py; black gate, flake8, isort clean; mypy clean on agents.py. Mutation checks: reinstating the dataclass spread reddens 8 of 11, removing the isinstance guard reddens 5, adding a scrub back reddens 1.

[chenmingwei23]

First Principles round 2, and a note on where GPT 5.6 actually landed

GPT 5.6's r2 finding was the same defect Design Review found, which is worth stating plainly because I had read the two lanes as pulling in opposite directions and they were not. Its r1 block asked for redaction; its r2 block (03846c367, "Redacted editable values overwrite stored configuration -- credential-shaped trigger -> roster redaction -> editor's full-field PUT -> stored trigger becomes the redaction marker") convicted the redaction I added for r1, on the same read-modify-write mechanism. ecba8c5fb removes that scrub, so both lanes' r2 position is what shipped. GPT's r3 run has not produced a completed verdict yet (its own comment carries a stale notice), so its current verdict binds a dead sha.

First Principles: the /api/members value-half asymmetry -- resolved by ecba8c5fb, and it was a fair hit on r2

Confirmed: members.py:144-147 ships kiro_agent, workspace, memory_store and model verbatim from the same record. On r2 this PR scrubbed exactly those four at /api/agents while its sibling shipped them raw, and the description did not admit to the asymmetry. That was a real gap, and it is inherent to fixing a value-half endpoint-locally rather than at a chokepoint.

With the scrub removed, both roster endpoints ship record values verbatim and both are covered by #8465's single pass over the same records. The remaining difference between them is the key half -- which is what this PR exists to fix, and it now matches members.py's documented allowlist rule. PR body updated to say this rather than leave it implied.

First Principles: the autonudge.py harvest entry -- I disagree, with evidence

The review classes autonudge.py:105 and kiro_prerequisite.py:40 as serializing "purpose-built DTOs -- not siblings". For kiro_prerequisite.py I have no quarrel. For autonudge.py the census says otherwise:

NudgeLoop is declared in the service module (src/kiro_crew/autonudge.py:433), not beside the handler, and carries 16 fields:

id, slot_key, message, idle_secs, max_cycles, cycle_count, active,
last_fire_ts, created_ts, stop_sentinel_path, max_runtime_secs, gate,
stopped_reason, approval_stalled, next_due_ts, monitor

Those are loop-scheduler state, not a response shape: stop_sentinel_path is a filesystem path that reaches the browser, which is concretely the field class this PR's rationale is about. And _serialize spreads the record and then pops monitor -- exclusion-based, so a 17th field added for a scheduler reason ships unless someone remembers to pop it. It is reached from the /api/autonudge listing plus three write responses, so four call sites inherit it.

A DTO is a type whose fields exist because the response needs them; this one's fields exist because the service needs them, and the response is a spread of it. I have left it in Pattern harvest as a candidate with that evidence attached, and deliberately have NOT converted it here -- it is a different endpoint with its own consumers, and this PR's whole argument is that a response-contract change needs its own inventory.

Unchanged

No code change this round: the members.py point was already answered by ecba8c5fb, and the autonudge point is a harvest classification, not a defect in this diff. Head stays ecba8c5fb; the r3 review lanes are still running.

[chenmingwei23]

Round 3 on 5aa800a0d: I withdraw a claim I made, and both GPT findings are fixed

GPT 5.6 raised two blocking findings on ecba8c5fb. Both are correct. The first one disproves an argument I made in my own previous two comments, so that goes first.

Withdrawn: "a scrub here closes nothing"

I argued twice that redacting this response was pointless because GET /api/config/kirocrew serves the same caller the same records unmasked. That is true for the dashboard owner and false for an app token, which is precisely the caller GPT's finding names ("App tokens receive unredacted credential-bearing free text", and its prescribed fix said app-token roster responses -- I read it as blanket in round 1 and did not re-read it in round 2).

App-token scope is deny-by-default and per path (token_auth.py::app_token_path_allowed):

• _APP_TOKEN_IMPLICIT_ALLOW is {"/api/ws"} -- that is the entire implicit grant.
• A manifest api entry matches by prefix at a path boundary (_api_pattern_matches: /api/agents matches /api/agents and /api/agents/..., not /api/agentsx).

So an app granted /api/agents -- which two shipped manifests declare -- cannot reach /api/config/kirocrew. For that caller /api/agents is not one of two doors to these values; it is the only one, and scrubbing it genuinely reduces exposure rather than relocating it. My "closes nothing" reasoning was scoped to the owner and I generalised it without checking the app path. That was the error.

Both findings fixed by splitting on caller class, not by trading one for the other

The two findings look opposed only until you notice they belong to different callers, and that the split is provable rather than convenient:

	dashboard owner	app token
Can write these values back?	Yes -- openEdit seeds the edit sheet from a roster row, saveEdit returns 7 of them unconditionally	No -- PUT/POST/DELETE on the agent routes are all _require_owner-gated
Other route to the same values?	Yes -- /api/config/kirocrew, unmasked	No -- out of app-token scope (above)
Therefore	values verbatim	values scrubbed

_roster_value(value, *, redact) implements exactly that, and redact = bool(request.get("app", "")) -- the same predicate members.py::_deny_app_caller uses. Only values differ; the key set is identical, which a test pins directly, so this is not a second response contract.

That disposes of both findings without reintroducing the other's defect:

• Finding A (app tokens receive unredacted free text) -- fixed. The app path applies _redact_external (credentials, then exfiltration URLs, then the lexical URL-parameter scrub), which is your prescribed remedy, scoped to the caller you named.
• Finding B (empty normalization causes configuration data loss) -- fixed, and you were right that r3 still had it. The owner path no longer coerces at all: a non-string is reported as stored, so nothing new can be PUT over it. The row is now typed dict[str, object] rather than dict[str, str], which is honest about the loader instead of asserting what the record declares. Your suggested remedy was to reject a malformed writable binding before rendering it editable; I did not do that, because rejecting a stored value is the config layer's call and doing it in a serializer would be a second place that decides what a valid binding is. Not coercing achieves the stated goal -- no roster-originated write can destroy the original -- with no new authority in this handler. Happy to be told otherwise.

Coverage

19 tests in test_agents_roster_contract.py, including an endpoint-level app-token case that goes through a middleware setting request["app"], so the caller wiring is covered and not just the row function's keyword.

Four mutations, each pinning one distinct defect:

mutation	reddens	pins
owner path also scrubbed	6	the r2 regression (GPT r2 + Design r2)
app path gets raw values	7	GPT r3 finding A
owner non-string coerced to ""	5	GPT r3 finding B
reinstate the dataclasses.asdict spread	5	the original #8454 defect

Gates on 5aa800a0d: 60 tests passing across test_agents_roster_contract.py, test_api_agents_order.py, test_config_api.py; test_security_posture.py 53 passing (agents.py is already a registered redaction call site, so its omission ratchet needs no change); black gate, flake8, isort clean; mypy clean on agents.py.

Unrelated CI reds, for the record

Backend Tests (Windows) (3) and Build Windows Installer (x64) are base-owned, not from this branch. The failures are test/test_playwright_cli_installer.py hitting AttributeError: module 'os' has no attribute 'killpg' at test/installer_test_helpers.py:56 (plus a pwsh timeout) -- os.killpg does not exist on Windows, that line is unchanged on origin/main, and this branch touches two files, neither installer-related. That shard reported 2 failed / 18952 passed.

[chenmingwei23]

Round 4 on d898e2190: both findings accepted, and they converge on one rule

GPT 5.6 and Design Review each landed one finding on 5aa800a0d. Both are right, and taken together they replace my per-RESPONSE split with a per-FIELD one that is sharper than either revision before it.

The rule

Scrub a value exactly where no write path accepts the result.

That is not a new invention -- it is the invariant _masked_config_dict's own docstring states for GET /api/config/kirocrew ("safe here because no config write endpoint accepts sensitive fields; if one is ever added it MUST treat _SENSITIVE_MASK as 'unchanged'"). Applied literally here it decides every case, including the two just raised.

_EDITOR_WRITTEN_BACK names the seven fields saveEdit returns unconditionally. Everything else has no write path, so scrubbing it costs nothing.

field	owner	app token
kiro_agent, workspace, memory_store, triggers, model, reasoning_effort, session_color	verbatim -- saveEdit writes these back	scrubbed
description, source	scrubbed -- absent from AgentUpdatePayload	scrubbed
name	verbatim -- addresses /api/agents/{name}	scrubbed
scope	handler literal, never touched	handler literal

GPT: "Owner responses bypass mandatory credential redaction" -- accepted

The finding named description and a provenance I had not checked, and it holds. _do_agents_sync does cfg.agents[disc.name] = KiroCrewAgentConfig(..., description=disc.description, ...) -- the string is copied straight off a discovered agent spec, so a third-party package controls it. That is exactly the class _redact_external exists for ("Any skills.sh publisher -- or, via the capability seam, any edition package manager -- controls these fields"). My previous revisions treated every record value as operator-typed text; for description that was wrong.

What makes this cheap to fix is a fact I had missed while arguing about round-trips: description is not in AgentUpdatePayload. saveEdit sends seven fields and that is not one of them. So scrubbing it for the owner destroys nothing, and the objection that killed the r2 unconditional scrub simply does not apply to it. source is in the same position. Both are now scrubbed on the owner path.

One part of the prescribed fix I have not taken: "and preserve masked editable values on PUT". The editable fields stay verbatim for the owner instead, because scrubbing them is the defect this same lane blocked in r2 -- "Credential-shaped trigger -> roster redaction -> editor's full-field PUT -> stored trigger becomes the redaction marker" (03846c367). Adding the write-path sentinel that would make scrubbing them safe means minting a masked-value convention on api_kirocrew_agent_update, which is #8447's chokepoint work and would need reconciling with #8465's differently-ordered chain. Scrubbing what is not written back reaches the same end state for every field that can be reached, with no new authority in this handler and no convention for #8447 to redesign.

Design: "the app-token scrub exempts name" -- accepted, and correctly reasoned

Right, and the reasoning is the part worth acknowledging: my addressability argument for name is an owner argument. The /api/agents/{name} edit and delete routes are _require_owner-gated, so an app token cannot address anything with that string, and name is agent-writable text like any other -- crew creation enforces only that it is non-empty (_AGENT_NAME_RE guards kiro_agent, not name). It was the one remaining unscrubbed path to a caller the scrub exists for. name is now scrubbed on the redact=True path only.

The cost is named rather than hidden: an app that reads a roster name and feeds it to another route sees the scrubbed form. That differs from the stored name only when the name contains credential- or URL-shaped text, and a test pins that ordinary names (kirocrew) pass through byte-identical on both paths.

Design: merge-order hazard with #8465 -- agreed, and it needs a human

Nothing I can do in this PR enforces it, which is the point of the finding. #8465's unconditional redact_record_strings wrap over this response reproduces the r2 defect: it would scrub triggers, which saveEdit writes back. Both PRs are green independently and wrong together. I have raised it on this PR rather than editing #8465, and I am not sequencing merges. A maintainer should decide the order, and whichever lands second needs the per-field rule above applied to the chokepoint -- the chokepoint is still the right home for record redaction, it just cannot be unconditional over a response whose values the editor writes back.

Coverage

24 tests. New in this round:

• test_written_back_set_matches_the_frontend_payload -- pins _EDITOR_WRITTEN_BACK against AgentUpdatePayload's seven names AND against the shipped record, so if the frontend makes another field editable this test fails rather than the operator's config being destroyed on the next save. That is the drift this whole rule depends on.
• test_owner_gets_non_editable_fields_scrubbed, test_app_token_also_scrubs_name, test_benign_content_is_never_altered_on_either_path.

Five mutations, each pinning one distinct defect:

mutation	reddens	pins
scrub written-back fields for the owner	4	the r2 defect (GPT r2 + Design r2)
app path gets written-back fields raw	4	GPT r3 finding A
name unscrubbed for app tokens	2	Design r4
coerce a written-back non-string	3	GPT r3 finding B
reinstate the dataclasses.asdict spread	5	the original #8454 defect

Gates on d898e2190: 118 tests passing across test_agents_roster_contract.py, test_api_agents_order.py, test_config_api.py and test_security_posture.py; black gate, flake8, isort clean; mypy clean on agents.py. The two Windows reds remain base-owned (os.killpg at test/installer_test_helpers.py:56, unchanged on origin/main; this branch touches two non-installer files).

[chenmingwei23]

Declining GPT 5.6's prescribed fix, and closing the hazard at its source instead

GPT's concern, stated first, because it is legitimate. A credential can be pasted as a crew name, a POST stores it, and a later GET returns it. GPT's finding on 00c9f6ec6 asks for _roster_mask(name) on every caller, owner included, so that no read of the roster can return such a name in the clear. The underlying worry -- a secret ends up somewhere it can be read -- is real and I am not disputing it.

Why I am not applying that fix.

name is the row's only handle. website/src/components/AgentSelector.tsx:127 dispatches onChange(a.name), and PUT/DELETE /api/agents/{name} both 404 on a name absent from cfg.agents. Masked for the owner, a crew cannot be selected, cannot be told apart from a sibling that also masks, and -- the part that matters -- cannot be renamed. Renaming is the remediation for a credential-shaped name, so masking it from the owner removes the only route out of the bad state and makes it permanent.

The owner is also not a disclosure surface for a string the owner typed. The person reading the name is the person who wrote it, and they can already read those same names unmasked one hop away at GET /api/config/kirocrew, where they are the agents map's KEYS and _masked_config_dict masks only schema-sensitive VALUES, never keys.

Masking one read closes one of N. This is measurable in this very file: _do_agents_sync logs "syncing agent %r ..." at agents.py:2902, printing a crew name straight into the log. Nothing about masking a JSON response touches that line, or telemetry, or an exception message. A stored credential-shaped name leaks through every surface that prints a crew name, and this endpoint controls exactly one of them.

What I implemented instead. A credential-shaped name is refused where it comes to exist:

• _name_would_be_masked (agents.py:2648) is keyed on _roster_mask itself, not on a second detector, so the create rule and the read rule cannot drift: anything the roster would mask is exactly what creation refuses.
• The check runs in api_kirocrew_agents_create (agents.py:3187) and returns 400 credential_shaped_name. The rejected name is deliberately NOT echoed into the response, because reflecting it into a body and from there into the request log is the disclosure being prevented.
• PUT cannot rename (the name comes from match_info, there is no new_name field), so creation is the only user-facing way in and there is no second hole to guard.

What this does NOT cover. Stating it plainly, because the hazard is closed only for new names arriving through that route:

1. A crew ALREADY in config.json is not retroactively renamed or refused. It keeps working and the owner keeps reading its name verbatim -- deliberately, since that is what makes renaming possible.
2. A name written into config.json by hand bypasses the check entirely. There is no load-time rejection, and I did not add one: refusing to load a config would take the dashboard down over a naming problem.
3. _do_agents_sync writes cfg.agents[disc.name] at agents.py:2910 from a discovered agent spec without this check. Those names are package-controlled rather than user-pasted, so the paste hazard does not apply, but the check does not run there either.

So: closed for new crews created through the API, open for the three cases above. The non-owner and app-token halves of the roster mask remain in place and are unchanged by this -- a caller who is not the author still gets a masked name.

If a reviewer thinks case 1 or 2 needs closing too, the honest next step is a separate change (a startup audit that reports credential-shaped names, or a rename-assist flow), not a mask that makes the existing ones unfixable.

[bolichen97]

Tech Lead review: APPROVE

Verified against the code at 2ab8bd6, not the description.

Allowlist is complete. KiroCrewAgentConfig has exactly 13 fields (config/sections.py:2816-2905). The row ships 10 of them plus handler-added name/scope; the drop set is exactly the 3 claimed (watchdog_tool_stall_suspect_secs, watchdog_tool_stall_hard_cap_secs, telegram_account). No 4th field is silently missing.

No frontend regression. KiroCrewAgent (AgentSelector.tsx:12-31) declares exactly the 11 shipped fields and nothing dropped. AgentUpdatePayload (KiroCrewAgentsPage.tsx:54-70) sends 8 fields on save, all allowlisted — so the edit-sheet round trip cannot write an empty over a dropped field. The three dropped fields have zero consumers in website/: their only readers are acp/session_handle.py:244-248 and config/loader.py, which read the config record directly rather than the API response.

The create-time 400 does not misfire on real names. I ran the actual redactors (redact_credentials → redact_exfiltration_urls → _URL_SECRET_PARAM_RE) against 30 candidate names. api-token-manager, prod-key-rotation, secret-santa, my_password_helper, ghp-triage, AKIA-audit, sk-reviewer all pass through untouched; only keyword=value forms (auth=x, sig=abc) and genuine credential shapes (AKIAIOSFODNN7EXAMPLE, xoxb-…) are refused. The usability cost is real but confined to names nobody chooses.

Value half holds. The sentinel is the canonical core._SENSITIVE_MASK (core.py:108), not a copy. _carries_mask is recursive, which is load-bearing rather than defensive: avatar is on the save payload, so an echoed masked trait would otherwise be persisted one level down. The owner predicate fails closed — request.app.get("state") is None short-circuits before is_owner_dashboard_request can KeyError on its request.app["state"] subscript (source_providers.py:6586).

No residual leak in the kept fields. avatar.file is fullmatched by ^[0-9a-f]{16}\.(?:png|jpg|webp)$ (sections.py:490), so no path component and no traversal. workspace/memory_store are config-entry names; WorkspaceConfig.dir is not shipped and still is not.

Tests are real, not vacuous. 30 tests (the description says 27 — stale count, immaterial). Key assertions are exact set equality (set(row) == ROSTER_ROW_KEYS), the fixture gives every withheld field a distinctive non-default value so absence proves the allowlist rather than a falsy default, and the omission ratchet subtracts both sets from dataclasses.fields() and guards its own guard in both directions. The read-then-write tests go over HTTP through the registered handler via TestClient(TestServer(...)).

Findings. GPT 5.6, Opus 4.8 and Design all PASS on the current head. First Principles is CONCERNS, which is advisory and says so itself ("a premise for a human to weigh, not a blocker"); its one Watch item — that a non-owner dashboard session can still read description/triggers raw from GET /api/config/kirocrew — is a pre-existing sibling surface this PR neither creates nor worsens, and the PR names it explicitly.

Direction is right. This is strictly stronger than the members.py precedent it cites, which allowlists keys but ships values raw. Scope is clean: 3 files, nothing extraneous, one commit, Pattern harvest present. No AGENTS.md security invariant is weakened, and the refusal log line deliberately withholds the offending name.

Merging.