fix(config): mask agent description/triggers in config endpoint

[bolichen97]

Fixes #8717.

Problem

GET /api/config/kirocrew returned every agent's description and triggers verbatim. _masked_config_dict (src/kiro_crew/dashboard/handlers/core.py) masks only values the schema marks sensitive; those two are writable by the agent itself and by an installed package and are not schema-sensitive, so the schema-driven walk never reached them.

Underneath that sat a second, sharper defect. description was the last free-text field on an agent record without the isinstance(..., str) guard its siblings model and triggers already carry in KiroCrewConfig.load(). The schema validator notices a type mismatch there, logs it, and keeps the value on the stated grounds that its consumer validates it — and for this field no consumer did. So a config.json holding

{"agents": {"x": {"description": {"k": "AKIAIOSFODNN7EXAMPLE"}}}}

loaded with description as a dict on a field declared str, and any redaction guarding on isinstance(val, str) skipped it and passed the nested credential straight to the browser. config.json is hand- **and agent-**writable, so that input is reachable rather than hypothetical. Reproduced before the fix (CREDENTIAL LEAKS TO BROWSER? True) and after (False).

Fix

Two layers, one per commit.

At the load boundary (config/loader.py) — coerce a non-str description to "", exactly as model and triggers already are. This closes the hole for every reader at once rather than once per endpoint, and it is the layer no other open PR on #8717 touches.

At the view boundary (handlers/core.py) — mask description/triggers on every agents.<name> record of the copy.deepcopy(cfg.to_dict()) view, deny-by-default:

• The guard is not isinstance(val, str) and val. That is the fail-open shape the backend-security-controls rule names outright ("never write if x and y and z guards where a falsy value silently skips the check"), and here it would defeat the redaction it performs — a value that is not a string is precisely the untrusted one. Everything but an empty string is masked.
• "" alone renders unchanged, so the UI shows no "set (hidden)" placeholder for a field nobody set, and an absent key is no longer conjured into one.
• The view must hold on its own rather than inherit the loader's guarantee from a distance, so its tests build the record on the dataclass directly, bypassing the coercion.

Mask cannot be persisted — verified, not assumed. PUT /api/config/kirocrew accepts only body["agent"] (the singular agent section); PATCH is a strict _EDITABLE_CONFIG allowlist that contains no agents.* path at all. So there is no route by which the sentinel is read back and written over the real value. Agent descriptions are still edited through PATCH /api/agents/<name>, which reads its value from the roster route, so editing is unaffected.

Scope, stated honestly

An earlier revision of this description claimed #8472 "already masks this same class of strings on the roster path". That was false and is retracted here and in the code comments it had reached: #8472 is open and unmerged, and handlers/agents.py still ships {"name": ..., **dataclasses.asdict(agent_cfg)}, so GET /api/agents returns both fields verbatim today. This PR does not close that route and does not depend on it being closed — each route owns its own response. Unifying both behind one rule, and settling whether that rule should be an unconditional mask or a redact_credentials-style scrub that preserves benign text, is #8717's remaining work, not something this PR decides.

The issue's final paragraph (agent-sync credential-shaped-name surfacing / gateway logging / install-refusal surface) is labeled "NOT part of this issue" and is untouched.

Pattern harvest

Rule candidate: semgrep
Pattern: a redaction, masking, or scrub step whose guard is isinstance(x, str) (or and-chained with a truthiness test) and whose false branch omits the value instead of redacting it.

This is one instance of a two-part class, and both parts recurred here:

1. A type-narrowing guard inside a security control fails open. isinstance in a validator is a check; inside a redactor it is a bypass, because the untrusted input is exactly the one that fails the narrowing. The rule should flag a masking assignment reachable only from the isinstance(..., str) true-branch, which is mechanically detectable — the enclosing function names a mask/redact sentinel and the else-branch has no write.
2. A "kept for its consumer to validate" value with no consumer. The schema validator's own message promises downstream validation; model and triggers honor it, description did not. Not semgrep-shaped, but it is checkable: every field the validator type-warns about should have a coercion at the load site. Worth an AGENTS.md line so the next field added to a record inherits the guard by convention rather than by whoever remembers.

The near-miss is what makes it worth a rule: triggers was guarded only because select_crew calls .strip() on it, i.e. because a crash forced the issue. description had no crashing consumer, so the same missing guard stayed silent and became a redaction bypass instead. A class of bug that only surfaces when something happens to crash is one a linter should be finding.

Testing

• pytest test/test_config_agent_string_masking.py test/test_config_extra_sections.py → 12 passed (7 + 5).
• pytest test/test_config_loader.py → 492 passed.
• pytest test/test_config_api.py test/test_config_patch.py test/test_config_roundtrip.py test/test_config_schema.py test/test_config.py test/test_config_rmw_preserves_settings.py test/test_config_module_boundaries.py test/test_agent_config_merge_on_write.py → 240 passed, 1 skipped.
• Every module referencing _masked_config_dict (test_connections_ui_flag.py, test_telegram.py) plus test_dashboard_handlers_core_coverage.py → 481 passed.
• -k "select_crew or crew_select or agents_api or agent_roster or conductor" → 655 passed, 1 skipped (the loader change reaches crew selection).
• Gates clean: black gate, subprocess-encoding gate, flake8, isort, mypy --platform linux src/kiro_crew (1294 files), brand, harness-parity, changelog-history.
• New coverage pins both layers: a dict/list/nested-dict and a scalar (7, False) in either field mask rather than pass through, the credential bytes are absent from the serialized response, an absent key stays absent, and at the loader an object/number/list description collapses to "" with the credential gone from to_dict() entirely.

Files changed

• src/kiro_crew/config/loader.py — isinstance(..., str) coercion for description.
• src/kiro_crew/dashboard/handlers/core.py — deny-by-default masking loop; docstring rationale corrected.
• test/test_config_loader.py — loader coercion test beside the sibling triggers one.
• test/test_config_agent_string_masking.py — 7 tests across both layers.

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 3b3d0883531b964c2853b7c9520e4376fb681c46 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 3b3d088

Verdict parsed from the review's SHA-scoped output markers for commit 3b3d0883531b964c2853b7c9520e4376fb681c46.

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 3b3d0883531b964c2853b7c9520e4376fb681c46 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/config/loader.py:2525 -- “whose redaction recognizes only str” contradicts the new non-string masking loop -> Fix: qualify this as the schema-driven walk only.
[GPT-REVIEWED] 3b3d088

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Load-boundary coercion fixes the root type-confusion; the view mask is zero-cost defense-in-depth (no frontend consumer reads agents from this endpoint), and write-back was verified impossible.

Watch

• The masking half buys no confidentiality until the roster route is closed: GET /api/agents still spreads both fields verbatim (agents.py:2618) to the same browser audience. The PR scopes this out honestly, but the endpoint-by-endpoint mask should not become the pattern — the unified rule (scrub vs mask) deferred to Agent description/triggers leave unmasked from GET /api/config/kirocrew, so #8472's redaction is point coverage #8717 is where the real fix lives.

[DESIGN-REVIEWED] 3b3d088

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 3b3d0883531b964c2853b7c9520e4376fb681c46 — 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. Composing the review.

First-Principles-Verdict: CONCERNS

The loader guard is a point patch: five sibling unguarded string fields on the same record ferry the identical nested-credential payload to the same browser view.

What this change ships

Intent: stop agent-writable description/triggers text, including credential bytes hidden in them, from reaching the browser via the config endpoint — a FIX.

1. Config endpoint now shows "••••••••" for every non-empty agent description — justified
2. Same masking for agent triggers — justified
3. A non-string description in config.json collapses to "" at load, for every reader — justified, cause-level
4. Empty or absent description/triggers stay as-is, no phantom "set (hidden)" placeholder — justified
5. Docstring retracts a false "roster already masks this" claim — justified

Watch

• Counted siblings of the loader root cause ("validator keeps a type-mismatched value for a consumer that never validates"): kiro_agent, workspace, memory_store, source, telegram_account at src/kiro_crew/config/loader.py:2530-2532,2540,2552 take entry.get(...) with no isinstance guard. A dict-valued kiro_agent holding {"k": "AKIA..."} survives load, is neither schema-sensitive nor in the new mask loop, and renders verbatim in the same masked view — the exact reproduced defect, one field over. The general fix is the same one-line coercion, five times; it is not larger than this change. The comment's "the last free-text field to lack one" is true only under a framing the harm does not share.
• The view mask's zero option costs almost nothing today: the same authenticated client fetches both fields verbatim from GET /api/agents (handlers/agents.py:2618, **dataclasses.asdict(agent_cfg)). The author declares this and defers it to Agent description/triggers leave unmasked from GET /api/config/kirocrew, so #8472's redaction is point coverage #8717/fix(dashboard): name every field GET /api/agents ships, instead of spreading the record #8472; until the roster route closes, item 1-2's protection is per-route bookkeeping, not removed harm. I also verified no frontend consumer reads agents.<name>.description/triggers from this endpoint (grepped all 16 kirocrewConfig() call sites; the only agents renderer, KiroCrewCfgTab.tsx:181, shows name/kiro_agent/workspace/memory_store), so the mask breaks nobody — and blinds no one.

[FIRST-PRINCIPLES-REVIEWED] 3b3d088

[bolichen97]

Closing as superseded by #8775, which merged 2026-09-06 06:10Z (3b2d879) and closed #8717.

Why superseded rather than rebased

Both PRs rewrite the same function — _masked_config_dict in src/kiro_crew/dashboard/handlers/core.py — and the two masking policies are opposites, not two sizes of the same one:

	this PR (#8768)	merged #8775
what is masked	everything except "" (deny-by-default)	only a value _redact_external would alter (credential- or exfiltration-URL-shaped)
benign content	masked	byte-identical
field coverage	description, triggers	every unguarded str field of the record, pinned by an invariant test against the dataclass (_AGENT_UNTRUSTED_TEXT_FIELDS)
detector	a local rule	reuses the roster path's _redact_external, so the two rules cannot drift
record key	not handled	a credential-shaped agent name drops the record from the view, with default_agent / session.pool_agent references masked when they match

The rebase conflicts on exactly those two hunks. Resolving it in this PR's favour would narrow field coverage from "every unguarded str field" to two and reverse a reviewed, merged decision on benign-content fidelity, so main wins on the merits and there is no unclaimed hole left behind for this PR to cover.

Retitling and keeping this PR for the remainder was considered and rejected: the title, body, Fixes #8717 and all four review lanes here are about the masking policy that no longer exists, so a different change under this number would mislead a reader while still re-running every lane.

The one part main does not carry, and why it is not being carried forward either

The remaining commit adds an isinstance(..., str) guard for description at the load boundary (src/kiro_crew/config/loader.py). Main genuinely lacks it. It is still not being ported, because this PR's stated premise for it does not hold against current main.

The body says description "was the last free-text field on an agent record without the isinstance(..., str) guard its siblings model and triggers already carry". It is one of six equally unguarded declared-str fields on KiroCrewAgentConfig: kiro_agent, workspace, memory_store, description, source, telegram_account. Only model and triggers are guarded, and each was guarded for a specific crash — normalize_agent_model().strip(), and select_crew's roster filter c.triggers.strip() (src/kiro_crew/mcp_core.py:2092).

description has no such consumer. Its only readers are the two browser-facing views, and both already mask a non-string explicitly and independently: _mask_agent_free_text on this endpoint, and _roster_mask on the roster row, whose comment already names the loader gap outright ("the loader lets an object through five declared-str fields"). So a description-only load guard closes no reachable leak and prevents no crash, while leaving five identically-shaped siblings untouched — which is the same point-coverage shape #8717 was itself filed about.

Tracked instead as one item at the width the code actually has, with the two coherent end states written out: #9056.

No merge and no approval on this PR — closing only.