Skip to content

feat(board): governed agent self-tagging via a chat_tag session directive + [BOARD] context line - #7779

Open
jeeshofone wants to merge 1 commit into
kirodotdev:mainfrom
jeeshofone:feat/7774-board-self-tagging
Open

feat(board): governed agent self-tagging via a chat_tag session directive + [BOARD] context line#7779
jeeshofone wants to merge 1 commit into
kirodotdev:mainfrom
jeeshofone:feat/7774-board-self-tagging

Conversation

@jeeshofone

@jeeshofone jeeshofone commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

A user managing many concurrent sessions has no way to see at a glance what is pending on them versus what agents are still working. Agents cannot participate in board organization at all: no MCP tool can set a workflow-state tag, and an agent cannot even read its own session's tags. The board only stays truthful through manual grooming.

Why it matters

Handoff visibility is the core value of a session board: an agent that finishes its work should be able to tag its own session Review on its last action, so filtering by Review answers "what needs me" across dozens of sessions. Without an agent write path, workflow tags rot the moment the user stops hand-maintaining them.

What changed (motivation → approach → change)

Goal: let agents move their own session between workflow states — under governance, because the board is also the human's tool. Approach chosen (over a backend rules engine and over unrestricted tooling): agents own the judgment of when to move; a fixed backend authorization layer owns who may move what. "Turn ended with nothing actionable ⇒ Review" style inference was deliberately rejected — the agent must explicitly declare the transition. Design discussed in #7774.

Built (v1 of the issue's two-stage plan — self-only, tags-only):

  • chat_tag MCP session directive (mcp_tools/control.py, modeled on set_project; registered in DIRECTIVE_TOOLS; two-layer schema with a custom_validator enforcing at-least-one of set_state/add/remove). Applied in session_directive_apply.py through the existing tags_write_lock → validate_folder_tag_ids → save_slot_off_loop chokepoint, with fresh slot.tags reads inside the lock and SEL audit (chat.self_tag, allowed/denied).
  • Per-tag agent policy from a protected grants store (chat_tags.agent_tag_policy / agent_tag_grant, backed by the new dashboard/chat_tag_grants.py): add-remove | add-only | none, resolved from <data home>/trust/agent-tag-policy.json — the trust/ directory is an existing whole-directory keystone entry, so the agent's own file tools cannot read or write the policy source (GPT review finding: fields on agent-writable tags.json rows could be forged and survive restart). Rows are minted only by the authenticated dashboard tag CRUD (create mints the out-of-the-box add-remove for workflow-state tags; PATCH accepts an agent policy value; delete revokes), with a one-time trust-on-first-use seed at boot deriving rows from the pre-store vocabulary fields. The applier's status semantics (set_state eligibility, peer strip, no-status-through-add) also key on the store's recorded status bit. Everything fails closed to none/non-status. remove — including the implicit removal in set_state — requires add-remove.
  • Workflow mutual exclusivity: set_state strips every other status: True tag, keyed on the flag rather than a hardcoded id list so custom status tags participate; a human-only status peer is refused rather than silently stripped.
  • Named refusals (tag_policy_denied:<tag>, unknown_tag:<tag>, no_op) and a success result carrying the resulting tag list — the first agent-readable tag surface.
  • Surface gating: chat_tag added to _USER_SURFACE_DIRECTIVES, so cron/subagent turns are refused.
  • [BOARD] context line (context.py + chat_runner.py): one per-turn line showing the session's tags and which are agent-writable, resolved in chat_runner alongside folder_path so context.py stays free of dashboard/state imports.

Out of scope here (v2, tracked in #7774): folder moves, conductor-over-children, propose-confirm for human-placed state, move budgets/cooldowns.

Tests

New test/test_chat_tag_directive.py (plus test_session_directive.py additions):

  • set_state review replaces an existing workflow tag and returns resulting tags (mutual exclusivity).
  • A none-policy tag is refused tag_policy_denied; an add-only tag adds but refuses removal (asymmetric add/drop).
  • Unknown tag refused unknown_tag; repeated state is no_op.
  • Headless (cron/subagent) surface refused via _USER_SURFACE_DIRECTIVES.
  • Directive registration/forgery-gate coverage (DIRECTIVE_TOOLS).
  • Fixtures set state._tags_authoritative=True mirroring real boot.

Suites run locally: test_chat_tag_directive.py, test_session_directive.py, test_chat_slot_project.py, test_mcp_tool_registry.py, test_context_management.py — 179 passed; black gate clean.

Manual verification

N/A — unit coverage sufficient: the directive path, policy matrix, exclusivity, and surface gating are all exercised end-to-end at the applier; the [BOARD] line is a plain text injection covered by context tests.

Related Issues

Part of #7774 (v1; the issue stays open for v2 — folder moves and conductor scope).

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@jeeshofone
jeeshofone requested a review from a team as a code owner September 2, 2026 01:25
@jeeshofone
jeeshofone requested a review from patrigao September 2, 2026 01:25
@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
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

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

The trust-directory premise checks out (trust is a whole-directory sensitive entry in security/paths.py, and the new test pins the store path inside it). I've verified the routed dashboard spec (docs/system-specs/modules/learn-cron-dashboard.md) is not touched by the patch, while mcp.md and session.md are. Review follows.

Design-Verdict: CONCERNS

Sound governed design on existing seams, but the two-store commit protocol is hand-inlined three times with its safety ordering living only in comments.

Watch

  • The revoke-first / persist / mint-after / compensate sequence is reimplemented separately in create, PATCH, and DELETE (chat_tags.py), with the ordering invariant ("deletion must never outlive the authority it removes") stated only in comments. The next tag-mutating write path (bulk import, board edit) that misses one step reopens the exact stale-open-grant hazard this PR closes — an agent re-creates the id in agent-writable tags.json and inherits surviving authority.
    Clears when: the two-store transition is extracted into one shared helper (e.g. a grant_transition() the three handlers call), or a test pins that every tags.json write path routes through it.
  • The owning spec for dashboard handlers (learn-cron-dashboard.md, per AGENTS.md routing) is untouched, so the new persisted authorization store (trust/agent-tag-policy.json), its schema, its seeding asymmetry (fresh vs upgraded installs), and the dual status-bit authority (display in tags.json, enforcement in the grant store) exist only in module docstrings — and the PR description still claims the boot seed "deriv[es] rows from the pre-store vocabulary fields", which is the rejected, less-safe design the shipped code explicitly refuses.
    Clears when: the owning spec documents the grants store and commit protocol in this PR's commit, and the description's TOFU-seed sentence is corrected to code-constants-only.

Suggestions

  • The status-identity gap for pre-existing custom status tags on upgrade (no row → no peer-strip, PATCH refused with status_required) is a deliberate fail-closed cost; surface it in the dashboard (a one-line "re-confirm this tag's status" affordance) rather than leaving a bare 400 as the only signal.

[DESIGN-REVIEWED] 7ed4862

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

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

2 of 2 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/dashboard/chat_tag_grants.py:55 -- Grant policy resides in a sandbox-writable directory

_TRUST_SUBDIR = "trust"
Agent shell -> runtime-built path bypasses text gates -> overwrites grant store -> refresh_cache authorizes restricted tag mutations.
Anchor: backend-security-controls
Fix: store grants in a dedicated OS-hidden sandbox leaf.

BLOCKING -- src/kiro_crew/dashboard/chat_tags.py:291 -- Tag creation is not crash-atomic across vocabulary and grants

await asyncio.to_thread(mint_grant, ...)
Status-tag creation -> vocabulary write commits -> process exits before grant write -> restart retains a tag whose authorization state is missing.
Anchor: residual/crash-data-loss-corruption
Fix: add crash-recoverable transaction state spanning both writes.

[BLOCK-MERGE] 7ed4862
[GPT-REVIEWED] 7ed4862

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

I've confirmed the key facts. The adjudicable block is empty (0 findings); two fenced findings remain.

F1chat_tag_grants.py:55, grant store in trust/. The store's whole security premise is that trust/ is un-writable by the agent. But trust is listed in _CREW_SANDBOX_VISIBLE_LEAVES (sandbox.py:346) — deliberately read-write VISIBLE to sandboxed processes because verify_session_pid reads trust/sel_hmac.key and MCP servers append there in-sandbox. It is on the agent-tool text gate (_SENSITIVE_HOME_DIRS, security/paths.py:336) but NOT bind-masked at the OS layer. A prompt-injected agent running a shell/python -c open() bypasses the tool gate entirely — exactly the keystone doctrine's warning, and CLAUDE.md itself records sel_hmac.key (in trust/) as VISIBLE with "no OS fence." The module docstring's claim ("shell can neither read nor write it") is false. The reaching condition is the store's own core threat, not an extreme corner — no basis to FLAG.

F2chat_tags.py:291, non-atomic create. Confirmed ordering in create_tag_definition_off_loop (patch 716-743): vocabulary _write_tags_snapshot commits, THEN mint_grant. A crash in that window leaves the tag in tags.json with no grant row. On restart resolve_grant fails closed to ("none", False) (chat_tag_grants.py:499) — the tag returns human-only, i.e. LESS authority, the safe direction, not corruption. The vocabulary (user data: name/color) is fully intact. Recovery is a documented single authenticated dashboard PATCH re-mint (api_chat_tag_update grant transition; stated as "one dashboard click to re-mint," chat_tag_grants.py:300). Narrow crash window, fail-closed, visible, recoverable; remedy is a cross-store WAL/journal serving one rare path.

F1 harm: security-class (forgeable authorization / governance ceiling), condition trivially reachable → UPHOLD-FENCED.
F2 conditions: create-status branch (patch 720) commits vocab before mint; crash between the two to_thread calls; recovery at resolve_grant fail-closed (chat_tag_grants.py:499) + dashboard PATCH re-mint. Fail-closed & recoverable, outcome is reduced authority not corruption → FLAG.

[ADJUDICATION] 7ed4862352e460dfdcf45e1ec3fe06849e140534 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 7ed4862352e460dfdcf45e1ec3fe06849e140534

[ADJUDICATION-FENCED] 7ed4862352e460dfdcf45e1ec3fe06849e140534 fenced=2 flagged=1
UPHOLD-FENCED F1 src/kiro_crew/dashboard/chat_tag_grants.py:55 -- trust/ is read-write VISIBLE to sandboxed processes (sandbox.py:346), so a spawned shell open() writes the grant store past the tool gate — the store's core threat, not an extreme corner.
FLAG F2 src/kiro_crew/dashboard/chat_tags.py:291 -- create commits vocab then mints; a crash in that narrow window leaves the tag human-only (resolve_grant fails closed, chat_tag_grants.py:499), user data intact, recovered by one authenticated dashboard PATCH re-mint — fail-closed and recoverable, not corruption.
[GPT-ADJUDICATED-FENCED] 7ed4862352e460dfdcf45e1ec3fe06849e140534

🏷️ Fenced finding(s) machine-flagged as likely edge case

The security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)

  • F2 src/kiro_crew/dashboard/chat_tags.py:291 — create commits vocab then mints; a crash in that narrow window leaves the tag human-only (resolve_grant fails closed, chat_tag_grants.py:499), user data intact, recovered by one authenticated dashboard PATCH re-mint — fail-closed and recoverable, not corruption.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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

Premise-level review of 7ed4862352e460dfdcf45e1ec3fe06849e140534 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 verification is done. The claims in the change check out against the base tree (the trust/ keystone entry exists at security/paths.py:336, contains_injection is an existing shared helper, the comment rewordings are mandated by the documented comment-history gate), but two premise gaps survived: the new agent PATCH field has zero shipped consumers, and the PR description describes a vocabulary-derived upgrade seed that the shipped code explicitly deleted.

First-Principles-Verdict: CONCERNS

The per-tag policy knob ships with no way to set it — zero consumers of the agent PATCH field — and the described upgrade seed is not what ships.

Not justified as shipped

  • Item 3 — zero consumers: grepped website/src for the tags API; updateChatTag's body (website/src/api/client.ts:3135) is {name?, color?, order?, status?} — no agent, and the diff ships no frontend change. Only a hand-crafted authenticated PATCH can set a custom policy.
  • Item 4 — zero consumers: no shipped writer ever mints add-only (seed mints add-remove/none; create mints add-remove; the sole add-only source is the unconsumed field above).
  • Item 8 — undeclared: the description contradicts the diff (see Watch).

What this change ships

Intent: let an agent move its own session between board workflow states so a human scanning many sessions sees what needs them — ADDITION.

  1. Agent can set/add/remove its own session's board tags via a new chat_tag tool — justified
  2. Per-tag agent policy persisted in a new protected store trust/agent-tag-policy.json — justified
  3. Tag-edit API accepts a new agent policy value — zero consumers, no UI or caller ships it
  4. add-only policy variant — zero consumers, nothing shipped ever constructs it
  5. Non-boolean status on tag create/edit now rejected instead of coerced — undeclared, derived from the forgery boundary
  6. Status-tag create/edit/delete can now fail 500 on a grant-store failure — justified
  7. Per-turn [BOARD] line shows the model its tags and which are writable — justified
  8. Upgraded installs get every tag human-only; only fresh installs get agent-writable defaults — undeclared, description claims the opposite seed
  9. Cron and subagent turns are refused from retagging — justified
  10. Comment rewordings and black-baseline housekeeping in touched files — rides along (mandated by the comment-history and format gates)

Watch

  • The description says the boot seed "deriv[es] rows from the pre-store vocabulary fields"; the shipped seed_default_grants docstring says "file-derived seeding is gone" and upgrades seed an empty store. Shipped behavior is stricter (safer), but every upgraded user's workflow tags go dead until each tag is re-created or PATCHed raw — a migration the description doesn't state. Clears when: the description matches the shipped seed and names the upgrade remedy.
  • Items 3–4 share one root cause: the policy write path shipped ahead of any sender. Clears when: a counted consumer (UI or documented CLI) lands, or the field is deferred to v2 (feat: agent-driven board organization — governed self-tagging, per-tag agent policy, and a [BOARD] context line #7774).

Subtractions

  • Defer the agent value in api_chat_tag_update and the add-only member of _ROW_POLICIES to the v2 that ships a sender — 0 consumers today; fresh-install seeding plus create-time minting cover every reachable behavior, and the PATCH handler's grant-transition/status_required choreography shrinks with them.

[FIRST-PRINCIPLES-REVIEWED] 7ed4862

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 7ed4862

@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
@jeeshofone
jeeshofone force-pushed the feat/7774-board-self-tagging branch from 60ab955 to 5a114fe Compare September 2, 2026 03:02
@jeeshofone

Copy link
Copy Markdown
Contributor Author

Review round 1 — all lanes addressed in 5a114fe25dd2440bc9a48f4ba13b7f8a315e39ad

GPT 5.6 blocking #1 (chat_tags.py — invalid agent value falls through to a permissive default): FIXED. agent_tag_policy now fails closed: a present-but-invalid agent value (typo, wrong type) resolves to "none" unconditionally — never to the workflow default. Only an absent field takes the default. Regression tests cover a malformed value on a status tag and a non-string value.

GPT 5.6 blocking #2 / Opus advisory (session_directive_apply.py — add=["review"] bypasses mutual exclusivity): FIXED. The applier now refuses any workflow-state tag (status: True) supplied via add, before any mutation, with a teaching error: Error: status_tag_requires_set_state:<id>. set_state remains the only verb that can change board state, so its peer-strip invariant can't be bypassed. Chose refusal over silently routing through the exclusive path (Opus's alternative) because the directive's design is result-teaches-boundaries — a named refusal tells the model the sanctioned verb. The chat_tag tool schema's add description updated to match. Regression test: add=["review"] on a todo slot refuses and leaves exactly one state.

Design Review concern (default keyed on frozen id set vs described status: True flag): FIXED code-side. The absent-field default now keys on the tag's status: True flag — the same key the mutual-exclusivity logic uses — so the description, the #7774 spec, and both code sites agree, and custom status vocabularies get consistent treatment. Regression test: a custom status: True tag defaults add-remove; a bare well-known id without the flag defaults none.

First Principles concern (per-tag agent field / add-only ship without a product write path): ACKNOWLEDGED, no change. Advisory. The policy field is the governance contract for the v2 follow-up (folder moves, placement locks — staged per #7774), and shipping the chokepoint with the contract keyed in one helper is deliberate; shipping v2's writes without the policy in place would be the unsafe order.

Verified: test_chat_tag_directive.py (18) + test_chat_tags.py (84) + black gate, all green.

@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 2, 2026
@jeeshofone
jeeshofone force-pushed the feat/7774-board-self-tagging branch from 5a114fe to 6c7baf8 Compare September 2, 2026 03:59
@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
@jeeshofone

Copy link
Copy Markdown
Contributor Author

Review round 2 — GPT blockings addressed in 6c7baf852be227e4b6d7ac50a93c79f0545bfda8

Blocking #1 (set_state=review, remove=["review"] leaves the session stateless): FIXED, both halves. set_state now requires the requested tag to actually be a workflow state (status: True) — a plain label would have run the peer-strip in exchange for a non-state tag (Error: not_a_status_tag:<id>). And a remove naming set_state's own canonical id in the same call is refused before any mutation (Error: set_state_conflicts_with_remove:<id>), case-insensitively. Regression tests for both.

Blocking #2 (persist not pinned to the authorized transcript): FIXED. The applier now captures slot_history_key(slot) inside the tags write lock before mutating, passes it as expected_history_key to save_slot_off_loop, and on a refused save (rebind or delete-won — the two False cases per that function's contract) rolls the in-memory slot.tags back and returns Error: session_rebound instead of reporting success. This is the same pin the metadata endpoints in chat_handlers use. Regression test simulates the refused save and asserts rollback + no broadcast.

Verified: test_chat_tag_directive.py (21) + test_chat_tags.py (84) + black gate, green. Round ledger: r1 = invalid-policy fallback + add-bypass (fixed); r2 = set_state edge + persist pin (fixed) — both r2 findings are new families, not repeats.

@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 2, 2026
@jeeshofone
jeeshofone force-pushed the feat/7774-board-self-tagging branch from 6c7baf8 to f8026a0 Compare September 2, 2026 04:43
@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 2, 2026
@jeeshofone
jeeshofone force-pushed the feat/7774-board-self-tagging branch from f8026a0 to 3a88fc8 Compare September 2, 2026 05:35
@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 2, 2026
@jeeshofone

Copy link
Copy Markdown
Contributor Author

Round 9 disposition — head 61cbca57a7a78791fb66324e45e7466ed8044a1d (was 21da4e96e)

BLOCKING (session_directive_apply.py:717 — live-alias overwrite): FIXED

Valid NEW finding, distinct from the held tags.json family. Two live slots bound to the same transcript: chat_tag commits slot A's update, but slot B still holds the pre-update tags in memory, and B's next dirty flush persists those stale tags over the committed state — a real lost-update window.

Fix: after the successful pinned save, still inside tags_write_lock, the applier mirrors the applied tags onto every OTHER live slot whose slot_history_key matches the transcript captured at turn entry. Any later flush of an alias then writes the same (current) state. Per-alias failures degrade to a debug log rather than failing the committed update. Test: test_alias_slot_sharing_transcript_is_mirrored (alias mirrored, unrelated slot untouched).

FINDING (validation.py:1295 — _TAG_ID_RE rejects Unicode names): FIXED

Correct — the gate was ASCII-only while _board_safe_tag_name admits Unicode \w, so a display name like Révision was unreachable despite the resolver being able to match it. _TAG_ID_RE is now ^[\w][\w \-./]*$ (Python \w is Unicode), exactly the sanitizer's grammar. Tests: test_unicode_display_name_resolves (gate admits + resolver matches case-insensitively).

FINDING (function-local imports at :543): DECLINED — module contract

The module's docstring states the import discipline explicitly: imports here are DELIBERATELY function-local except the shared session/ownership contracts. sel is a genuine cycle, and the rest are deferred on purpose — they keep this module cheap to import from the turn loop's import graph, and call-time resolution is what lets tests (and runtime overrides) patch the SOURCE module and be observed; a module-scope from X import name would freeze a stale binding. save_slot_off_loop / chat_tags follow the same documented contract as the module's other deferred imports. Hoisting them would carve an inconsistent exception into a documented invariant for no behavioural gain.

31 directive tests + 123 adjacent tag/board tests green; black gate + isort + flake8 + mypy clean on all changed files.

The round-6 hold on the tags.json file-sourced-grants family and the flag-keyed vs id-keyed policy axis remain with the maintainer, unchanged.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Round 10 note — head 61cbca57a7a78791fb66324e45e7466ed8044a1d, no code change

GPT r10 re-asserts the tags.json file-sourced-grants finding — the same family held since r3, now its 7th consecutive round, with no new mechanism or argument. The r9 fixes (live-alias mirror, Unicode shape gate) were accepted; all four other lanes are green and CI is green on this head.

Position unchanged: the established mitigations stand (agent:"none" always wins; set_state refuses stripping non-add-remove peers; keystone-fenced policy file offered as a follow-up), and both open axes — this family and the flag-keyed vs id-keyed policy default — remain with the maintainer. I will implement whichever direction is chosen within one round.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

GPT blocking finding taken: the policy source is now protected from agent writes

The finding was correct and is fixed at the root rather than by reverting the mutation path: per-tag agent-write policy no longer lives in agent-writable tags.json at all.

What changed (819a65017):

  1. New protected grants storesrc/kiro_crew/dashboard/chat_tag_grants.py, persisting to <data home>/trust/agent-tag-policy.json. The trust/ directory is already a whole-directory keystone entry (shared with the SEL project key and the skill-trust store), so the agent's file tools and shell can neither read nor write it — the same placement computer_use.json and ops_mission_control_policy.json use for exactly this "the record IS the authorization" class. Reads fail closed (("none", False)) on unknown ids, malformed stores, malformed rows, or newer schemas.
  2. Resolution reads only the store. agent_tag_policy (and the new agent_tag_grant) consult the store by tag id; the tag dict's agent/status fields are never honored. The applier's status semantics — set_state eligibility, the mutual-exclusivity peer strip, the no-status-through-add rule — also key on the store's recorded status bit, since a forged status field would otherwise re-route those authorization decisions even with the policy axis protected.
  3. Writers are the authenticated dashboard CRUD only. Tag create mints the out-of-the-box add-remove grant for workflow-state tags; PATCH accepts an agent policy value (the authenticated replacement for the retired hand-edit) and handles status flips; delete revokes. Ordering is fail-closed by construction: revokes commit before the vocabulary write (a store failure aborts the PATCH), mints run after it (a store failure leaves the tag human-only).
  4. Trust-on-first-use migration. At boot, when the store does not exist, rows are derived once from the vocabulary's legacy fields — a pre-store hand-edited grant survives the upgrade; every later edit to tags.json changes nothing. Guarded on an authoritative vocabulary read.

Tests: the policy suite is rewritten against the store, including the finding's exact attack end-to-end (forged agent/status fields in the vocabulary with no store row → chat_tag refused on both the policy and status axes), TOFU one-shot semantics, mint/revoke round-trip, malformed-store and malformed-row fail-closed, and per-row damage isolation. 193 tests across the touched surface pass locally; full lint floor green.

The residual trust boundary is stated plainly: the store is enforced against the agent's tool surface by the keystone fence, not cryptographically — a process with unrestricted host access is outside this (and every) data-home control's threat model, same as the sibling keystone leaves.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Re-synced onto current main (0b17a532e, rebase of 819a65017 — no semantic change beyond the already-described grants-store commit). Conflicts were mechanical: the validation import list and the two directive-tool lists in the MCP architecture doc, both resolved by keeping main's new monitor-tool entries and re-adding chat_tag. Local floor re-verified post-rebase: 228 tests across the four touched suites (including the autonudge applier suite that guards the recent session_directive_apply.py churn), black/isort/flake8/mypy green.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

All five findings taken (da3272114)

Every finding in this round named a genuinely new mechanism against the grants store, and all five are fixed as asked:

  1. Seed promotes agent-controlled data (blocking, chat_tag_grants.py:227) — file-derived seeding is GONE. The boot seed now mints rows solely for the code-constant default workflow-state tag ids (seed_default_grants, fed from the class-level default vocabulary, never from tags.json). The migration cost is stated in the docstring: a pre-existing custom grant requires one authenticated PATCH to re-mint. The earlier disposition's "bounded by trust at rest" argument is withdrawn — laundering the file's contents through the upgrade was exactly the class of hole the store exists to close.
  2. agent: "none" erases status identity (blocking, chat_tags.py:413) — the store now records policy-none rows carrying the status bit; a PATCH to none mints such a row instead of revoking, and revocation is reserved for tag deletion and status removal. Regression test pins ("none", True) resolution.
  3. Failed narrowing retains broader authority (blocking, chat_tags.py:443) — every grant change now revokes the old row before the vocabulary persist and surfaces a replacement-mint failure as HTTP 500 instead of swallowing it; between revoke and mint the tag resolves closed, so a narrowing PATCH can never report success while add-remove silently survives.
  4. Store read on the event loop (blocking, chat_tag_grants.py:137) — the two async call sites (the chat_tag applier and the per-turn board context injection) now pre-warm the parse via await asyncio.to_thread(refresh_cache); the per-tag resolutions serve from the cache at one stat syscall each, the same trade-off the skill-trust reader documents.
  5. Truthy-string status (finding) — the store parser accepts the status bit only when it is an actual boolean (is True), and both CRUD handlers 400 a non-boolean status payload, since a coerced "false" would mint workflow-state identity and, through the default, agent authority.

231 tests across the four touched suites pass locally; black/isort/flake8/mypy green.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

GPT round disposition — all four blocking + three findings taken in 5dc30c4cb

  1. [BOARD] emits canonical tag IDs, never names (blocking, context.py:3174) — taken. chat_runner's board_tags resolution now yields (id, policy); the free-form name field (agent-writable prose) no longer reaches the trusted context rail. IDs are also the exact handles the chat_tag directive consumes, so the line got more useful, not less. _board_safe_tag_name stays applied at the injection point as defense in depth — an id hand-written into tags.json is still attacker-shaped input.

  2. Rejected PATCH leaves no partial mutation (blocking, chat_tags.py) — taken. The strict-boolean status validation moved above the write lock, before any field assignment: a PATCH carrying a valid rename plus an invalid status now rejects with nothing mutated in memory. Regression test test_update_tag_invalid_status_leaves_no_partial_mutation asserts the rename does not land behind the 400.

  3. Parse failure caches fail-closed empty rows (blocking, chat_tag_grants.py) — taken. _load_rows now caches (sig, {}) on a failed read/parse, so the off-thread refresh_cache pre-warm is effective for malformed stores too and resolver calls never re-read the file synchronously on the event loop. A rewrite changes the signature and re-reads. Regression test test_malformed_store_failure_is_cached patches read_text to raise after the first miss.

  4. Unhashable id no longer crashes after commit (blocking, session_directive_apply.py) — taken. Both name_by_id comprehensions (no-op READ path and post-commit result) filter to isinstance(id, str); malformed entries fall back to the raw tid string. Regression tests cover both paths with a list-valued id in the vocabulary.

  5. final_status strict boolean (finding, chat_tags.py:426) — taken: tag.get("status") is True, so a persisted "status": "false" string can no longer record workflow-state authority during an agent-policy PATCH. Regression test test_update_tag_string_status_row_not_promoted_by_agent_patch.

  6. Function-local imports (finding) — taken where clean, documented where deliberate: refresh_cache hoisted to module scope in chat_runner.py and seed_default_grants in state.py (the grants module depends only on atomic_write + config.paths, no cycle). In session_directive_apply.py the deferral is the module's documented policy (cheap turn-loop import graph + call-time symbol resolution so test patches are observed); the header's deferred-module list now names the tag modules explicitly.

  7. Docstring vs actual no-op result (finding) — taken: the docstring now describes the real behaviour (audited as no_op, answers with the current tag list).

Verification: 169 tests across the tag/board suites plus 67 state/body-guard tests pass; black gate (one graduated baseline entry pruned), isort, flake8, mypy, subprocess-encoding and feature-map gates all green.

@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 #5933 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 #5933: KEEP. No goal or code overlap, but a hard ordering dependency: three separate breakages in whichever direction these land. The _FakeState AttributeError is a test failure neither PR's CI can see today, and the force=True census is deliberately a design prompt rather than a number to bump, so the two authors should agree the order and who pays for the adaptation. Files: src/kiro_crew/dashboard/session_directive_apply.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
  • PR #7163 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 #7163: REBASE. Two independent directives added through the same seam. Purely additive on both sides; expect textual conflicts at five shared insertion points and nothing more. Files: src/kiro_crew/dashboard/session_directive_apply.py, src/kiro_crew/session_directive.py.
  • PR #7669 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 #7669: KEEP. Neither blocks the other and they do not conflict textually, but they must agree on two things: whether the agent self-tagging writer fires SessionLaneChanged (otherwise the event's documented writer table is wrong the day 7779 lands), and one convention for reading a tag's status flag. Files: src/kiro_crew/dashboard/session_directive_apply.py, src/kiro_crew/dashboard/chat_tags.py.
  • This PR is OVERLAPPING with PR #3469. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7779: MERGE_DISCUSSION. This PR is the surviving implementation of the closed PR #3469; no further deduplication is needed against it. Files: src/kiro_crew/dashboard/session_directive_apply.py.
  • This PR is OVERLAPPING with PR #7877. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7779: MERGE_DISCUSSION. Two competing designs for one capability. A maintainer should pick the canonical agent tag-write path; if 7779 lands, 7877's self-tag-chat skill and the PUT /slots/{slot}/tags entry in its allowlist should be re-pointed at chat_tag so the app keeps only what is genuinely its own (health sweep, auto-resume, reconciler, app page). Files: src/kiro_crew/apps/builtins/chat_status_tags/skills/self-tag-chat/SKILL.md, src/kiro_crew/validation.py.

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

@jeeshofone

Copy link
Copy Markdown
Contributor Author

GPT round disposition — all three blocking + the finding taken in 0a1154bbd

  1. Agent-writable status no longer feeds the mint (blocking, chat_tags.py:431) — taken. The status bit recorded with a grant mint now comes from the validated PATCH body when status is supplied, otherwise from the tag's EXISTING protected grant (read via an off-thread refresh + snapshot resolve) — never from tags.json. An agent forging a real boolean status: true in the file gains nothing from a subsequent agent-only PATCH. Regression test test_agent_patch_does_not_promote_forged_bool_status.

  2. Failed updates restore the prior grant (blocking, chat_tags.py:446) — taken. The pre-PATCH grant is captured before the up-front revoke; a vocabulary-persist or replacement-mint failure now re-mints it (best-effort, logged) before surfacing the 500, so a failed PATCH is a no-op on authority rather than a permanent revocation. The revoke-up-front ordering from the earlier round is preserved — between revoke and restore the tag still resolves closed. Regression test test_failed_mint_restores_prior_grant (mint fails on the new policy, prior ("add-remove", True) survives).

  3. Resolver is cache-only (blocking, chat_tag_grants.py:142) — taken. resolve_grant never touches the filesystem: it serves the immutable snapshot the last off-thread refresh (or authenticated write) installed, and fails closed to ("none", False) when no snapshot exists. Writers (mint/revoke/seed, already off-loop via to_thread) install the fresh snapshot directly instead of invalidating, so a signature miss can never become a synchronous read+parse on the gateway loop. Regression test test_resolver_never_reloads_the_store (file changed externally + read_text patched to raise: resolve serves the snapshot; with no snapshot it fails closed).

  4. Docstring wording (finding, chat_tags.py:63) — taken: "code-default workflow-state IDs."

Verification: 133 tag/board tests plus 106 lane/session/state tests pass; black gate, isort, flake8, mypy all green.

Note on the prior CI red: test_security_regex_linearity::test_long_nonshell_line_does_not_blow_up failed on the previous head — that is the known flake tracked in #8374 (filed before the push; this branch does not touch the security regexes). This push re-rolls it alongside the fixes.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Response to the 2026-09-04 relationship audit — position on each coordination item, from the #7779 side:

  1. fix(dashboard): snapshot the slot view, gate the persist on identity, reconcile post-commit #5933 (ordering dependency, session_directive_apply.py) — agreed the order should be explicit rather than discovered in CI. Proposal posted on fix(dashboard): snapshot the slot view, gate the persist on identity, reconcile post-commit #5933: land fix(dashboard): snapshot the slot view, gate the persist on identity, reconcile post-commit #5933 first and this PR pays the adaptation — the _FakeState test stubs here are ours to extend with whatever attributes the snapshot/identity-gated persist introduces, and the force=True census is ours to answer in the applier (which already pins the persist to the entry-time transcript key, so the identity-gate direction is one we want anyway). If feat(board): governed agent self-tagging via a chat_tag session directive + [BOARD] context line #7779 happens to land first, the same offer stands in reverse: ping here and the adaptation commit follows within a round.

  2. feat(hooks): fire SessionLaneChanged when a session's board lane changes #7669 (two conventions) — (a) Should the self-tagging writer fire SessionLaneChanged? Yes: once both land, a chat_tag set_state transition is a lane change and should emit the same event the human writers do, so the event's writer table stays true. Whichever PR lands second wires the applier call — same offer as above if that is us. (b) One convention for reading a tag's status flag: these PRs read it for different purposes and the conventions compose rather than conflict — feat(hooks): fire SessionLaneChanged when a session's board lane changes #7669 normalizes the tags.json field to a real boolean at load for display/event semantics; feat(board): governed agent self-tagging via a chat_tag session directive + [BOARD] context line #7779 treats that field as display-only and sources authority (what an agent may do, and what counts as workflow state for the agent path) exclusively from the protected grants store, strict-boolean at the parse boundary. With feat(hooks): fire SessionLaneChanged when a session's board lane changes #7669's load-time normalization in place, feat(board): governed agent self-tagging via a chat_tag session directive + [BOARD] context line #7779's is True reads become redundant-but-harmless defense in depth. Stated on feat(hooks): fire SessionLaneChanged when a session's board lane changes #7669 as well.

  3. feat(apps): add chat-status-tags builtin — SDLC + health tagging for dashboard chats #7877 (competing designs) — maintainer's pick, and this note is not a claim to it. feat(board): governed agent self-tagging via a chat_tag session directive + [BOARD] context line #7779's position for that discussion: the chat_tag directive is the governed single chokepoint (per-tag policy from a protected store, applier-enforced, SEL-audited, headless refused), and the audit's own recommendation — re-point feat(apps): add chat-status-tags builtin — SDLC + health tagging for dashboard chats #7877's self-tag skill and its PUT /slots/{slot}/tags allowlist entry at chat_tag if feat(board): governed agent self-tagging via a chat_tag session directive + [BOARD] context line #7779 lands — is the outcome we'd support, including helping with that re-point.

The 2026-09-02 audit's four carry-over asks from #3469 (name resolution + vocabulary-enumerating refusal, the v2 governed-target scope note on #7774, the advancement-only record on #3456) were all actioned that same day.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

GPT round disposition — all three blocking taken in 86695e866

  1. Identity captured before the refresh suspension point (blocking, session_directive_apply.py:819) — taken. authorized_history_key is now captured at function entry, BEFORE the off-thread grant refresh, so a rebind landing inside that await is caught by the in-lock recheck against the entry-time key instead of comparing the moved key against itself. Regression test test_rebind_during_grant_refresh_is_refused performs the rebind inside a patched refresh_cache and asserts the session_rebound refusal with nothing mutated.

  2. Failed mint rolls back the vocabulary too (blocking, chat_tags.py:497) — taken. The mint-failure path now restores the prior grant AND rolls the vocabulary back to the pre-PATCH snapshot in memory and on disk before surfacing the 500, so a failed PATCH is a no-op on both stores rather than a durable rename behind an error. The rollback write is best-effort with the memory rollback as the floor. Regression test test_failed_mint_rolls_back_vocabulary asserts both the live dict and tags.json on disk keep the old name.

  3. Deletion revokes before the vocabulary commit (blocking, chat_tags.py:563) — taken. The DELETE handler now captures the existing grant, revokes it FIRST, and aborts with 500 (nothing changed) if the revoke fails — closing the resurrect-by-recreating-the-id channel. A vocabulary-persist failure after the revoke re-mints the captured grant and rolls memory back. A crash between revoke and commit leaves the tag present but closed — fail-closed, never stale-open. The handler docstring's crash-atomicity note is updated to match. Regression tests test_delete_aborts_when_revoke_fails and test_delete_vocab_failure_restores_grant.

Verification: 137 tests across the tag/board suites pass; black gate, isort, flake8, mypy all green.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

GPT round disposition — all three blocking taken in cdb0362d6

  1. Injection screen runs on the sanitized value (blocking, context.py:293) — taken. _board_safe_tag_name now strips first and scans the string that actually ships: scanning the raw value let a punctuation-obfuscated payload (ig[no]re previous instru[ctions) pass, with the charset strip reconstructing the instruction on the trusted rail. Regression test test_obfuscated_injection_not_reconstructed_by_strip pins the exact bypass.

  2. Post-mirror re-save serializes past a queued stale flush (blocking, session_directive_apply.py:1030) — taken. After mirroring the applied tags onto live aliases, the applier issues a second confirmed save (same entry-time transcript pin): a dirty alias flush that captured pre-update tags before the mirror can no longer be the last write on the transcript. A failed re-save marks the slot dirty — memory is already mirrored, so the periodic flush reconverges from current state. Regression test test_post_mirror_resave_serializes_after_stale_flush asserts the second confirmed save.

  3. Missing store clears the snapshot (blocking, chat_tag_grants.py:137) — taken. A refresh observing a deleted/renamed store now clears _cache instead of leaving the old snapshot serving revoked grants; every resolve fails closed to ("none", False) until a store reappears. Regression test test_deleted_store_clears_the_cached_snapshot.

Verification: 157 tests across the tag/board suites plus the context board-line suite pass; black gate, isort, flake8, mypy all green.

Trajectory note (pre-commitment): this is the sixth consecutive blocking round on this PR. Each round so far has surfaced genuinely new mechanisms — mostly in compensation code the previous round added — and every finding has been implemented same-round. Finding 2 above is the second refinement of the alias-persistence family (after the earlier live-alias mirror round). If the next round blocks again within the alias-flush/persistence-interleaving family, I will treat it as same-family churn per the repo's deferred-finding discipline and request maintainer arbitration on that axis rather than another unilateral round; genuinely new different-family findings will still be fixed.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

GPT round disposition — finding 1 taken in 81f640b66; finding 2 held for maintainer arbitration per the pre-commitment in the previous disposition

1. Racing refresh vs authenticated write (blocking, chat_tag_grants.py:157) — TAKEN. Snapshot installs are now serialized behind a module lock, and a refresh re-verifies the on-disk signature at install time: a refresh that read the store before a concurrent write can no longer overwrite the writer's newer snapshot (the writer's install holds the same lock, so every interleaving resolves with the fresh snapshot last). Regression test test_stale_refresh_cannot_overwrite_newer_write drives the exact race — a revoke landing between a refresh's read and its install — and asserts the revoked grant stays revoked.

2. Durable cross-store recovery under ENOSPC (blocking, chat_tags.py:465) — HELD, requesting maintainer arbitration. This is the fourth consecutive round on the grant/vocabulary compensation axis (r4: restore the prior grant on failed mint; r5: roll back the vocabulary too, and revoke-before-delete with restore; r6: post-mirror re-save; r7: now a durable journal for the restore path itself). Each prior round was implemented; this one asks for a qualitatively different guarantee — recoverability when both stores fail to write, i.e. a write-ahead journal or equivalent transactional machinery spanning tags.json and the protected grants store.

Position held for the maintainer to rule on:

  • The failure mode under discussion is fail-closed, not fail-open. In every double-failure interleaving (ENOSPC after revoke, restore also failing), the tag resolves to ("none", False) — the agent LOSES authority it should have, never gains authority it shouldn't. The finding's cost is availability of a per-tag convenience grant, not a security boundary; the operator re-grants via one PATCH once the disk recovers.
  • Failed restores are not swallowed: both paths log at warning with the tag id and full traceback, and the request surfaces 500. What they do not do is persist a durable retry intent.
  • A cross-store journal is an architecture change disproportionate to the asset (a policy row reconstructible from one authenticated click), and it would itself need failure handling on the same full disk it exists to survive.

Two options for the maintainer to pick between — either will be implemented within one round of the ruling:
(a) Accept fail-closed + loudly-logged best-effort restore as the design floor for this store (a doc note recording the ENOSPC behaviour can be added), or
(b) Require durable recovery, in which case I'd propose the minimal shape: persist a pending-restore marker file next to the grants store before the revoke, cleared on success, replayed by the boot seed path.

CI is green on the new head and every other lane is clear; per the repo's deferred-finding discipline this held finding is recorded here rather than silently dropped.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

GPT round disposition — the blocking finding taken in f5dc8cbb4

Upgrade seeding no longer authorizes deleted default tags (blocking, state.py:6888) — taken, exactly as suggested. The boot seed now mints the code-default workflow-state grants ONLY on the boot that also seeds the default vocabulary (fresh install, no tags.json on disk); an upgraded install initializes an EMPTY grants store instead. A user who deleted planned/done before this feature ships can no longer have an agent restore the id in agent-writable tags.json and inherit the pre-granted authority after restart — on upgrade, every tag is human-only until an authenticated PATCH grants it. Note the deliberately rejected alternative: intersecting the defaults with the CURRENT tags.json contents would read the agent-writable file into the seeding decision, re-opening the laundering channel the r2 round closed — gating on the vocabulary-seed event keeps the seed constant-only.

Regression test test_upgrade_install_seeds_empty_store: a pre-existing vocabulary without the default tags boots to a store where neither the default ids nor the file's own status: true custom tag resolve writable.

Verification: 159 tests across the tag/board/state suites pass; black gate, isort, flake8, mypy all green.

The r7 held finding (durable cross-store recovery under ENOSPC) remains with the maintainer — the two options from the previous disposition stand.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

GPT round disposition — the blocking finding taken in e4ce17ceb

Failed grant creation no longer partially commits the tag (blocking, chat_tags.py:290) — taken, as suggested. A POST creating a status tag whose grant mint fails now rolls the create back in BOTH stores (in-memory vocabulary and the persisted tags.json) and re-raises; the handler surfaces it as 500 persist_failed. No more 201 for a tag that chat_tag refuses. This brings CREATE to the same transactional standard PATCH (r5: dual rollback) and DELETE (r5: revoke-first with restore) already meet. Regression test test_create_mint_failure_rolls_back_the_tag asserts both memory and disk carry no trace of the doomed tag behind the 500.

Completeness note for the next round: with this fix, every vocabulary/grant write pair in the module is now transactional in both directions — CREATE (mint fails → vocabulary rolled back), PATCH (persist or mint fails → grant restored + vocabulary rolled back), DELETE (revoke fails → abort; persist fails → grant re-minted). The one residual, by design, is the double-failure case (compensating write ALSO fails, e.g. ENOSPC), which is exactly the r7 finding held for maintainer arbitration — every such interleaving resolves fail-closed. If a further round identifies a pair outside this matrix, it is new; a finding inside the double-failure residual belongs to the held arbitration.

Verification: 143 tests across the tag suites pass; black gate, isort, flake8, mypy all green.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

GPT round disposition — the blocking finding and both findings taken in bf1e351b8

  1. Row cap no longer silently truncates (blocking, chat_tag_grants.py:120) — taken, with one refinement beyond the literal suggestion. Enforcement moved to both ends: mint_grant refuses a NEW row at or past the cap (updates to existing rows still work; the CRUD callers convert the raise into a 500 and roll their vocabulary write back per the transactional matrix), and the parser REJECTS an oversized document outright instead of truncating — the reader path fails closed to zero grants. The refinement: the WRITE path parses with allow_oversized=True, because a strictly-rejecting writer would freeze an oversized (hand-grown/corrupt) store permanently — even revoke couldn't shrink it back under the cap. So an oversized store resolves nothing until repaired, and revoke IS the repair path. Regression tests: test_oversized_store_fails_closed_never_truncates (whole store refused, no partial view) and test_mint_refuses_new_row_past_cap_but_allows_updates (new row raises at cap; update and revoke still work).

  2. Tool description qualified (finding, control.py:614) — taken: the chat_tag description now states that workflow states are agent-writable by default on a fresh install or when newly created as status tags, and that an upgraded install starts with every tag human-only until granted — matching the r8 seeding change.

  3. Stale comment fixed (finding, chat_tag_grants.py:184) — taken: the refresh docstring now says resolutions are entirely filesystem-free, matching the cache-only resolver.

Verification: 145 tests across the tag suites pass; black gate, isort, flake8, mypy all green.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Round disposition — Opus blocking and GPT's non-blocking finding both taken in 0dec06c89

Windows DACL on the grants store (Opus blocking, chat_tag_grants.py) — taken, exactly as suggested. The store now mirrors skill_trust's write discipline end to end: a _store_dir() helper that removes a planted link or junction (is_link_or_junction, not is_symlink — a Windows directory junction is not a symlink), then make_owner_only_dir + restrict_dir_to_owner; and _write_document writes with atomic_write(..., restrict_to_owner=True) — implying 0o600 on POSIX and applying a real owner-only ACL on Windows — replacing the mode=0o600 argument that was a documented no-op there. The obsolete _STORE_MODE constant is removed. This closes the cross-account forgery channel Opus identified: on Windows the authorization store no longer inherits the data-home DACL.

Stale comments (GPT non-blocking, chat_tag_grants.py:66) — taken: the row-cap comment now describes reject-whole (never truncation), and the last "stat syscall" phrasing was already corrected in the prior round.

Verification: 145 tests across the tag suites pass; black gate, isort, flake8, mypy all green. Note this fix is verified structurally on macOS (the POSIX path is behavior-identical: restrict_to_owner=True implies the same 0o600) — the Windows ACL branch rides the same platform_compat primitives skill_trust already ships and tests.

PR state after this round: GPT cleared on the previous head (ten rounds, all findings implemented or formally held); this push addresses Opus's first blocking round with the repo's own established pattern. The one open maintainer item remains the r7 ENOSPC-durability arbitration.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Round 11 disposition — invoking the round-6 pre-commitment: maintainer arbitration on the alias-flush axis

Blocking (session_directive_apply.py:1051 — rebind during the post-mirror re-save can leave stale tags durable): REAL, and the third refinement of one axis — HELD for maintainer arbitration per the pre-commitment.

The mechanism as I read it: a dirty alias flush queued BEFORE the mirror can land after our confirmed save; the post-mirror re-save exists to serialize behind it; if the requesting slot rebinds during that re-save's await, the reseal is correctly refused (expected_history_key mismatch) and the slot._dirty = True fallback marks a slot that now points at a different transcript — so nothing reconverges the original transcript on disk, and a restart inside that window restores stale tags. Memory on every live alias is already correct (the mirror ran); the exposure is durability-only and needs a triple race: queued pre-mirror flush + requester rebind mid-await + restart before any alias flushes.

This is the same axis three rounds running: live-alias mirror → post-mirror re-save → rebind-during-re-save. In round 6 I pre-committed publicly: another blocking round inside the alias-flush/persistence-interleaving family would go to maintainer arbitration rather than another unilateral compensation layer, because each layer here has spawned the next race. That is exactly what has happened, so I'm honoring it.

Two concrete options — I will implement whichever the maintainer picks within one round:

  • (a) Reseal through a surviving alias (GPT's literal suggestion). When the requester's reseal is refused, find a live alias still bound to authorized_history_key and force-save that slot instead; report success only on confirmation. Closes the window fully; adds one more branch to the compensation stack.
  • (b) Mark mirrored aliases dirty at mirror time. The mirror already corrects alias memory; marking those aliases _dirty makes the periodic flusher durably reconverge the transcript regardless of what happens to the requester — no new save path, and it also covers re-save failures from unrelated causes. Leaves a bounded window until the next periodic flush rather than closing it synchronously.

My recommendation is (b): it repairs by convergence through the existing flush machinery instead of adding a fourth synchronous compensation, which is how each of the last three races was created. But I'll take either.

Non-blocking (function-local imports at :806): record-keeping. These are the module's documented lazy-import policy (see the module header, added in round 3 when the same convention was raised); the applier's tag-module imports are deferred deliberately. No change.

PR state: Opus's Windows-DACL blocking round was fixed and cleared on this head; Design/FP/UX green. Open maintainer items are now two: the round-7 ENOSPC durability option pick, and this round's alias-axis option pick.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Maintainer request: allow-fork-workflow-change label — the flagged .github diff is a black-baseline deletion the black gate itself mandates

The Fork workflow-change guard (newly enforcing on this PR's re-run; it was skipped on the same head earlier today) flags one file: .github/black-baseline.txt, a single-line deletion — removing test/test_chat_tags.py.

That deletion is not optional: this PR formats test/test_chat_tags.py (it gained substantial new regression tests across the review rounds and was black-formatted in the process), and scripts/check_black_formatting.py explicitly requires that "a file in the baseline that has become clean must be removed from it" — the gate fails otherwise, and its --update-baseline mode only ever deletes lines. So the two gates conflict for any fork PR that formats a baselined file; the guard's label is the designed resolution.

The full .github diff is exactly:

-test/test_chat_tags.py

No workflow files, no CI logic, no other .github content is touched. Happy to have it inspected — it's the whole diff above.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

@bolichen97 Thanks for merging #8726 and #7628 earlier today — much appreciated!

When you have a moment, this PR is fully green (all five AI lanes cleared, GPT after 11 rounds) and is waiting only on three maintainer picks to move forward:

  1. Round 7 ENOSPC disposition — two options laid out in this comment; either is a one-round implementation on my side.
  2. Round 11 alias-flush disposition — options (a) reseal via surviving alias / (b) mark mirrored aliases dirty, recommendation and trade-offs in this comment.
  3. allow-fork-workflow-change label — the black gate forces a baseline deletion under .github/-adjacent scope, full diff shown in this comment; the fork CI can't pass without the label applied.

Happy to implement whichever way you pick on 1 and 2 within a round. No rush — just flagging it's decision-ready rather than work-in-progress.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Round 12 disposition (55a9e96e2) — the alias-flush axis, resolved by design instead of another layer

The round-6 pre-commitment sent this axis to arbitration; the arbitration ran as a six-seat cross-model design council (GPT-5.6 ×4 independent samples, Sol, Opus — each given the full round-9→10→11 history, the real code, and a mandate to find the change BOTH review lanes accept and that terminates the axis). The verdict was unanimous: neither option (a) nor keeping the round-10 layer — instead, convergence-by-flusher. Implemented exactly that:

The change. (1) The step-2 mirror now marks every mirrored alias _dirty alongside copying tags, and the requester is marked _dirty after its confirmed commit (the Opus seat's amendment — without it, the single-slot case reopens round 10). (2) The round-10 post-mirror re-save is DELETED, together with its resealed fallback — the site of the round-11 finding no longer exists. Net: −20/+10 lines, one save path removed, zero added.

Why this terminates the axis. Rounds 9→11 were one family: a synchronous write racing to land last, each new write adding the await the next finding exploited. The pinned step-1 commit already puts the correct state durably on disk; after this change there is NO post-commit await in the applier, so there is no interleaving of that family left to find. Durable reconvergence is now a single invariant — every live slot bound to the transcript holds correct memory and is dirty; the existing periodic flusher reconverges — instead of three compensations.

Walk-through of every named race. Queued pre-mirror stale flush lands after the commit → the dirty aliases re-flush correct memory; bounded, self-healing. Requester rebinds mid-await → no post-commit await exists; the pinned commit either confirmed or refused-and-rolled-back. ALL aliases gone → the step-1 commit is already the last write to that transcript (a rebound slot's flush is pinned by its own live history key and cannot touch it) — durable state correct, no action owed; note option (a) FAILS exactly here, having no alias to reseal through. Concurrent second update → same lock, serialized, last committer re-mirrors and re-dirties. Crash at any point → disk holds either the pre-change state (nothing claimed success) or the committed state; the ONLY residual is a stale queued flush landing post-commit and a crash before the next periodic flush tick — bounded by the flush interval, self-healing on restart's first flush, and never a loss of the committed write (it landed once already). That residual is the accepted trade, stated here in advance per the round-6 pre-commitment: no further synchronous compensation will be added on this axis; the levers, if the window is ever deemed too wide, are flush cadence or a shutdown flush hook, neither of which touches this seam.

Also taken — a latent bug the council caught on the REFUSAL path (session_rebound): a refused pin-save fires ONLY on rebind, yet the fallback marked that rebound slot dirty — its periodic flush would then persist the rolled-back tags onto the DIFFERENT transcript it now points at, leaking this session's tag set to an unrelated conversation. The dirty-mark is removed: nothing was committed, the original transcript is untouched on disk, no reconvergence is owed. Test pins the rebound slot as NOT dirty.

Mirror-failure logging raised from debug to warning (a persistently failing mirror is now the only path stale memory could survive; it should be visible).

Tests: the round-10 re-save test is REWRITTEN to the new contract (exactly ONE save — the pinned commit; aliases and requester dirty); rebound-rollback test extended with the not-dirty assertion; chat_tag suite 53/53, tag_session + board lanes 39/39; black gate, flake8 (CI scope), isort, mypy clean.

Open maintainer items are now ONE: the round-7 ENOSPC durability pick. The allow-fork-workflow-change label request also stands.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Round 13 disposition (813bc9277) — both security findings taken; also re-sync onto 2f9ed9724

F2 (fenced — agent-controlled IDs enter trusted context, context.py): TAKEN, in the terminal form. The round-12 guard was sanitize-then-screen — a blacklist, so instruction text phrased past the heuristic could still ride the trusted rail, exactly as flagged. _board_safe_tag_name is now an allowlist over the id grammar: a board handle is admitted only when it already IS a slug (lowercase alphanumeric runs joined by single -_./ separators, ≤48 chars) and is otherwise rejected WHOLE — never rewritten, so no strip can reconstruct a payload, and no heuristic needs to recognize an instruction: one cannot be spelled inside the admitted grammar (no spaces, no uppercase, no brackets/colons/backticks/newlines). This is "source displayed handles from protected data" in spirit — the line renders canonical tag IDS (the same strings chat_tag consumes), and ids that fail the grammar are dropped (the vocabulary loader independently prunes malformed ids). Tests rewritten to the allowlist contract, including the case the blacklist could not close: bare instruction PROSE with no punctuation at all is now rejected.

F1 (fenced — "authorization store remains agent-writable", chat_tag_grants.py): DISPUTED WITH EVIDENCE, and pinned by a new test. The store lives at <crew-home>/trust/agent-tag-policy.json, and the trust directory is whole-directory gated in the governance trust fence (security/paths.py) — the same keystone entry protecting denied_commands.json, security_policy.json, and the SEL HMAC signing key, blocking both reads and writes across every gated agent surface (file tools, the shell edit gate, artifacts, dashboard file I/O, knowledge indexing), with dual-resolution coverage of config_dir() and publish-artifact (atomic-write temp) coverage. Verified empirically: is_sensitive_path() returns True for both the store file and its directory. An agent that could nonetheless write this file could equally rewrite denied_commands.json — i.e., the threat reduces to "the platform's trust root is bypassable", which is a platform property, not a property of this PR, and moving the store OUT of the audited fence to a new bespoke location would weaken, not strengthen, it. What this PR now adds: TestGrantsStoreInsideTheTrustFence pins the store (file AND directory) inside the fence, so any future path change that exits the fence fails CI with this finding's rationale in the failure message. If the reviewer's model is a fence bypass, I'd ask for the concrete write path — that would be a critical platform issue worth filing on its own, independent of chat_tag.

Re-sync: rebased onto 2f9ed9724. Conflicts: context.py (main's _neutralize_reply_format_markers next to our sanitizer — union) and session_directive_apply.py (main's arming-directive machinery around _USER_SURFACE_DIRECTIVES — union, keeping our chat_tag entry). Main's new directive-invariant ratchet (test_directive_refusal_not_lost_marker_8635) requires a hostile-call row per directive tool — added the chat_tag row (unknown-tag refusal). The two test_context.py::TestContextBuilder failures on the previous run are inherited (fail on the detached main tree; tree byte-identical).

Suites: chat_tag + directive-invariant 93/93, context 95 passed/2 inherited; black gate, flake8 (CI scope), isort, mypy clean.

Open maintainer items unchanged: round-7 ENOSPC pick; allow-fork-workflow-change label.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Round 14 disposition (f2d622aad) — the status-default finding taken; F1 unchanged pending the requested write path; three CI lanes cleared

New blocking (chat_tags.py — upgrading a status tag records it as non-status): TAKEN, exactly as suggested. With no protected record to inherit from, defaulting the mint's status bit either way is wrong (True launders authority from agent-writable tags.json; False strips a status tag's identity and lets add bypass exclusive-peer stripping). The PATCH is now refused — 409-class 400 status_required — when a mint has no status in the body AND no protected row exists. One subtlety the suggestion surfaces: resolve_grant's ("none", False) default is deliberately indistinguishable from a minted none-row (policy resolution must fail closed either way), so the gate keys on a new has_grant_row — row EXISTENCE from the same cache snapshot — rather than the resolved tuple; a minted none-row still inherits its recorded bit. Tests: the two forged-status promotion tests now pin the refusal AND the explicit-status: False pass-through (the forged file bit never reaches the store); the mint-failure rollback tests pass the gate explicitly so they still exercise their seams.

F1 (grants store) — position unchanged from round 13: the store is inside the whole-directory governance trust fence (empirically pinned by TestGrantsStoreInsideTheTrustFence); a concrete agent write path to it would be a platform trust-root bypass worth its own critical filing, and relocating the store out of the audited fence weakens it. Standing by for that write path or maintainer arbitration.

Also this push: Inclusive Language lane — one added comment reworded (denylist). Comment-history ratchet — my added lines reworded to present-tense invariants, plus the file-total overage in context.py/chat_runner.py paid down by rewording seven pre-existing narration lines in files this PR touches (the lane bills main-side drift to whoever touches the file; see #9350 for the drift itself). Windows shard 3's test_security_conductor_skill_contract failure is inherited (byte-identical to main; #9363 tracks it).

Suites: chat_tags + chat_tag_directive + directive-invariant 185/185 (224 with tag_session/board lanes); black gate, flake8 (CI scope), isort, mypy, comment-history gate all green locally.

Open maintainer items: the allow-fork-workflow-change label; the round-7 ENOSPC pick; and now the F1 evidence question.

… + [BOARD] context line

An agent can now move its OWN session between workflow-state tags (and
add/remove ordinary tags) through a stateless chat_tag session directive,
under backend-enforced governance (issue kirodotdev#7774 v1):

- chat_tag MCP tool (control.py) modeled on set_project; registered in
  DIRECTIVE_TOOLS; applied via session_directive_apply through the
  existing tags_write_lock -> validate_folder_tag_ids ->
  save_slot_off_loop chokepoint, SEL-audited.
- Per-tag agent policy (agent: add-remove | add-only | none) resolved in
  chat_tags.agent_tag_policy; status:True tags default add-remove so the
  workflow set works with no vocabulary migration; user tags default
  human-only.
- set_state enforces workflow mutual exclusivity across all status:True
  tags; refuses (never silently strips) a human-only status peer.
- Named refusals: tag_policy_denied / unknown_tag / no_op; success result
  returns the resulting tag list (first agent-readable tag surface).
- Cron/subagent turns refused via _USER_SURFACE_DIRECTIVES.
- [BOARD] context line injected per turn (tags + agent-writable set),
  resolved in chat_runner alongside folder_path.

Part of kirodotdev#7774 (v1 of 2 — does not close the issue).
@jeeshofone

Copy link
Copy Markdown
Contributor Author

Round 15 disposition (7ed486235) — three of four taken; the store-location axis remains held

F2 (slug grammar admits ignore.previous.instructions): TAKEN — the round-14 allowlist's claim was wrong and this finding proves it. Separators read as spaces to a model, so instruction words joined by -_./ fit the grammar. The guard now normalizes every separator to a space and runs the injection screen on that rendering — the string as the model would read it — rejecting the handle whole on a hit. This composes the two prior forms instead of oscillating between them: the allowlist bounds the character space (nothing can be reconstructed, the round-13 failure mode), and the normalized screen catches instruction phrasing inside it. Tests pin the dotted/dashed/mixed forms rejected and ordinary slugs untouched.

F3 (rollback erases valid none/false rows): TAKEN, exactly as suggested — and it is the same existence-vs-default seam round 14 fixed at the gate, now fixed at the RESTORE sites. Both compensation paths (_restore_prev_grant on PATCH, the vocabulary-write rollback on DELETE) keyed "was there anything to restore" on prev_grant == ("none", False), which conflates a minted none/False row (protected status-identity state) with no row. Both now key on the captured has_grant_row existence bit and restore the exact tuple.

F4 (upgraded sessions retain two workflow states): TAKEN via the suggested none,true identity seeding. New seed_status_identity_rows, called at boot for the code-constant default workflow-state ids: rows minted as {"policy": "none", "status": True} — the identity bit constrains (peer exclusivity) and grants nothing, so a tag id restored into agent-writable tags.json inherits no authority; this threads the exact laundering concern that made whole-grant upgrade seeding wrong (that reasoning is unchanged and documented at the call site). Existing rows never touched; unreadable store fails closed rather than being papered over; idempotent (second boot is a no-op). Tests cover pre-existing-row preservation, identity-only rows, and idempotence.

F1 (store location): position unchanged — the store is inside the whole-directory governance trust fence, pinned by test since round 13; "sandbox-visible" has not yet come with a concrete write path that the fence's gated surfaces (file tools, shell edit gate, artifacts, dashboard I/O, knowledge indexing) permit. Standing by for that path or maintainer arbitration; relocating an authorization store out of the audited fence on assertion alone would weaken it.

Non-blocking (function-local imports): the module's documented lazy-import policy; no change.

Suites: 211/211 across chat_tags/chat_tag_directive/directive-invariant/tag_session; comment-history, black gate, flake8, isort, mypy clean.

@bolichen97

Copy link
Copy Markdown
Collaborator

@jeeshofone Thanks for this, and sorry for the wait. Reviewing it as part of a repo-wide open-PR relationship audit (audited at 55a9e96; your head has since moved to 7ed4862 and the file set is unchanged). Three open PRs touch it.

#7877 (@billygerhard) delivers the same end-user capability, an agent moving its own dashboard session between mutually exclusive workflow tags, through a different mechanism: a credentialed chat_status_tags_api tool whose allowlist includes PUT /slots/{slot}/tags, driven by a self-tag-chat skill where exclusivity and never-downgrade live in prose the model is asked to follow. This PR enforces those server side in _apply_chat_tag, with per-tag authorization from the protected store in src/kiro_crew/dashboard/chat_tag_grants.py. Neither contains the other, and only src/kiro_crew/validation.py is textually shared. We need to pick one canonical agent tag-write path; if it is this one, #7877 would drop its self-tag skill and the PUT /slots/{slot}/tags allowlist entry and keep its health sweep, reconciler and app page.

#8185 (@Pearcekieser) makes slot.tags_revision an invariant every slot-tag writer must rotate, in src/kiro_crew/dashboard/chat_tags.py. Your applier assigns slot.tags and mirrors tags onto alias slots without a bump, so whichever lands second owes one bump call in the applier and one in the alias mirror.

#8696 (@chenmingwei23) adds an exhaustive hostile-call table over DIRECTIVE_TOOLS. Whichever lands second must add the chat_tag row, or CI fails on a coverage assertion rather than on behaviour.

Separately, this branch is well behind main and currently conflicting, so please rebase before we judge merge state.

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

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) merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants