Skip to content

feat(chat): add transcript section markers, gate suggest_followup on human provenance, refuse over-capacity forks - #7163

Open
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:feat/transcript-section-markers
Open

feat(chat): add transcript section markers, gate suggest_followup on human provenance, refuse over-capacity forks#7163
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:feat/transcript-section-markers

Conversation

@rnoack1

@rnoack1 rnoack1 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

A long-lived chat session is sometimes not one conversation but a sequence of
independent units of work handled one at a time: review this item, finish it,
move to the next. The transcript is a single unbroken scroll, so when unit N+1
starts the reader is looking at the tail of unit N, and nothing in the record
marks where one ended and the next began.

That boundary is information only the writer holds. A client reading the rows
afterwards would have to infer it — from a timestamp gap, a content pattern, a
turn count — and every such heuristic is wrong on the cases that matter (one item
that took four turns; two items handled back to back in one). It is not a
rendering preference a client can compute; it is an assertion about the work.

There is no divider row in the transcript today for any purpose. Every collapse
affordance is intra-turn (CollapsibleToolGroup, TurnBlock, ToolCallLine),
and EarlierMessagesBar is a server-side pager for history not yet in the
client, not a fold of loaded rows.

Why it matters

Without a way to mark the seam, the only ways to get a clean working surface are
to scroll past everything or open a new session — and a new session per item
loses the sequence, floods the session list, and pays session-start cost per
item. reset-conversation does not help: it deliberately leaves the view
untouched, and it answers 409 on four guards for a caller that is, by
construction, mid-turn.

This is Phase 1 of the design of record in
docs/request-for-change/rfc-transcript-section-markers.md, which argues the
shape and records why four cheaper designs were rejected. Phase 1 ships the
event and the rule and is useful alone: a labelled divider helps in a long
session with no collapse behaviour at all. Collapse is Phase 2 and is
deliberately absent here.

Before you upgrade — two capabilities are WITHDRAWN

Both are on paths unrelated to section markers, and both remove something that worked
before. Neither has a config escape hatch (verified: zero override keys exist).

1. Agent-initiated follow-up cards now require human provenance. suggest_followup
joins _USER_ORIGIN_DIRECTIVES (src/kiro_crew/dashboard/session_directive_apply.py:103-107;
it is absent from that set on the base). A cron or sub-agent turn riding a slot with an
open dashboard tab could post a follow-up card before this change; after it, that call is
refused. Signal: a WARNING line in the gateway log and a record in the security audit
stream — the card simply never appears, so an automation that posted cards becomes a
silent no-op from the caller's side. Recovery: none in config. A follow-up card has to
originate from a human-driven turn; automations that relied on posting one must deliver
their result another way (send_message, or a notification).

2. Forking an over-capacity conversation now fails instead of truncating. A
whole-conversation fork of a session past _MAX_SLOT_MESSAGES (10,000 rows) previously
answered success while silently dropping the oldest rows; it now answers 400 with code
fork_corpus_too_large. Signal: the 400 and its code, surfaced in the UI as the
fork-failure banner (see Screenshots). Recovery: fork at a message
(at_message_index / at_message_id) to copy a slice that fits — the refusal message
names this. The body carries error, code and max_messages — the enforced row
ceiling, which the banner interpolates so no localized copy restates it. There is
still no computed retry-at index to read.

Anyone bisecting a broken cron-followup workflow, or an automated whole-fork that used to
return 200, should start here.

Behaviour change beyond the feature

Three independently revertible changes ride this one commit, and that coupling is
a cost rather than a design.
They are (1) the section-marker feature itself, (2) the
suggest_followup human-provenance withdrawal, and (3) the fork over-capacity refusal
together with the report argument its refusal copy needs. None depends on another's code, so any
one of them could be reverted alone — but reverting THIS COMMIT reverts all three, which
means a regression in the marker rendering and a rollback of the fork data-loss fix share
one fate. The only shared surface is _USER_ORIGIN_DIRECTIVES, introduced here and
carrying both section_marker and suggest_followup.

A directive dropped because its call's identity could not be verified now says so in
the tool result, and that applies to every provenance-gated directive rather than only
to markers.
The identity gate refuses a directive whose call does not carry a trusted
_meta.kiro server name — a forged marker, and equally an ACP backend that emits no such
identity at all. Before this change that refusal was operator-visible only: a structured
denial row plus a warning naming the observed and expected server, the tool, the session
and the parked-record depth. The agent, meanwhile, saw the tool's own deliberately
non-committal confirmation and had no way to tell a dropped directive from an applied
one, so a backend that cannot carry directives looked identical to one that can. The
result now carries a bracketed [Not applied: …] note instead, mirroring the note the
native sub-agent refusal already appended. The gate itself is unchanged — silence toward
a genuinely foreign MCP server is the security property, and the note's wording states
only that identity could not be verified.

Every producer of a suggest_followup directive was enumerated to confirm no legitimate
headless flow relied on the inherited-tab path. The tool handler
(mcp_tools/control.py::suggest_followup) is the only emitter; the sole programmatic
call_tool("suggest_followup") in the tree is a test. No bundled skill (0 of 61), app
(0 of 496) or task-runner step names it, against a positive control that a sibling
directive autonudge_stop appears in 4 bundled skills. The tool's published restriction
already read "cron, and subagent contexts are rejected" before this change, so the
inherited-tab path was an enforcement gap against its own stated contract rather than a
supported route. The agent-facing bullet in the shipped config/prompt.md promised only
"Dashboard sessions only", and now states the headless refusal like its three siblings;
a guard reads the gate membership off the applier so a directive added to it later must
say so too.

A fork of an over-capacity conversation now refuses instead of silently losing its oldest
rows, and that is a behaviour change on a path markers need not be involved in at all.

A destination slot trims from the FRONT at _MAX_SLOT_MESSAGES (10,000), and during the copy
loop nothing has reached disk yet, so persisted_trim is 0 and every evicted row is
unrecoverable. Before this change the handler answered success anyway. It now answers 400
with code fork_corpus_too_large, and the error names the alternative that still works:
forking at a message (at_message_index / at_message_id) copies a head or tail slice that
fits. The refusal carries the message, the code and the static max_messages ceiling: an earlier round also returned a computed at_message_index_that_fits, and that field was CUT before merge because it had a single consumer and would have been a permanent addition to the wire body. test_the_over_capacity_refusal_carries_no_retry_parameter pins its absence, so the 400 body is exactly error plus code. Markers make this reachable at lower turn counts because they are copied rows too, but
any conversation over the ceiling was already affected, including one with zero markers.

Also widened, and outside this feature: a provenance refusal now logs a gateway
WARNING, and it does so for all four provenance-gated directives — set_project,
reset_conversation, section_marker and suggest_followup — because the log sits
in the shared gate rather than per directive. For the two pre-existing directives,
set_project and reset_conversation, the SEL audit row was previously the SOLE
record of a refusal; they now emit an operator-visible WARNING as well. Nothing else
about their gating changes.

A provenance refusal now also leaves a line in the gateway log, so an operator whose cron or sub-agent follow-up cards stopped appearing can find out why from the log they actually read rather than only from the security audit stream. It covers ALL FOUR directives behind that gate, which means set_project and reset_conversation — outside this feature — also gain a gateway-log line they did not have. That is a deliberate widening: scoping it to the two directives this change gates bought an extra branch and left the other two with the security audit stream as their only record of the same refusal, which is the operator-cannot-see-why gap the line exists to close. Pinned by test_every_gated_directive_leaves_a_refusal_record.

A fork failure now carries a structured report on the notice that already existed, rather than
getting a surface of its own.
To be exact about the before-state: fork failures ALREADY rendered
through ErrorNotice on the base — ChatPage.tsx routed both handleFork paths through
showActionError, and the base carries no live alert() call at all. The gap was the SIGNATURE, not
the slot: showActionError took (message, title?) only, so it had no parameter that could carry the
structured report from the error journal (endpoint, status, backend code). An earlier round of this
PR answered that with a second, dedicated fork-error notice; that was a duplicate surface, since the
page already stacks upload-error, sid-error, action-error and pin-error with identical styling,
dismiss and hand-off behaviour. It is gone. showActionError now takes an optional third report
argument and the one shared notice renders it, so the fork path adds a capability to an existing
surface instead of re-spelling it. Net effect: one error surface where there were two, one
forkError state and one slot-scoped clearing effect removed — the shared notice already clears on
slot switch — and the direction-specific copy (the over-capacity refusal naming the direction that
copies fewer rows) is unchanged. The twelve existing showActionError call sites are untouched,
because the new argument is optional.

Provider availability

The row and the path that writes it are provider-agnostic. The signal that ADMITS a
marker is not, and that limit is worth stating plainly rather than implying parity.

General. The event the applier hangs off is EVENT_TOOL_RESULT, defined in the
transport-neutral ACP layer at src/kiro_crew/acp/types.py:46 and imported by the
provider base contract at src/kiro_crew/providers/base.py:28. The applier
src/kiro_crew/dashboard/session_directive_apply.py carries no provider branch (zero
kiro-cli references), and the persisted row is plain role / content / meta, so
any provider that surfaces a tool result reaches the same code.

The limit. The anti-forgery gate honours a directive only when the call was
observed as MCP-served, keyed on _meta.kiro.toolName with _meta.kiro.mcpServerName
set (src/kiro_crew/session_directive.py:30-40). That identity arrives on kiro-cli's
out-of-band _meta channel and the gate FAILS CLOSED when it is absent, so on a
provider exposing no equivalent channel a marker is IGNORED rather than mis-applied.
The mechanism is general today; the trust signal is kiro-cli-specific. Generalising it
means giving other providers a provenance channel of their own, which is a larger
change than this PR and is deliberately not attempted here.

What changed

A new transcript row type that renders as a labelled horizontal rule.

A new role, section_marker, carrying its label in meta. Not a cls on an
existing role: the dashboard serializer persists cls only when
role == "system" while ConversationLog.append persists any truthy cls, so a
cls-carried label is silently dropped on one of the two write paths. meta
persists for every role. content carries a human-readable — End of: … —
string as the compatibility surface, so an older client degrades legibly rather
than showing a blank row. The role stays out of RECALL_ROLES (never enters a
model's prompt), _QUESTION_RETIRING_ROLES (must not retire a pending question
card) and _PROMPT_ROLES (must not make a session look freshly asked-of); all
three exclude it by default and tests pin that none gains it.

An MCP tool and session directive section_marker(label?),
wired the way reset_conversation is: the stateless tool validates and returns
an encoded directive, and the session-aware consumer decodes it at the
tool-result event and applies it against its own slot, so cross-session
misattribution is unrepresentable. label is capped at 120 chars — a structural
row must not become a route for smuggling a paragraph of prose. Dashboard-only,
because the marker is a rendering and a messaging channel has no transcript
surface to draw on.

The append is deferred to a turn boundary, reusing /note's existing hold.
This is the part worth reading the code for. A turn builds its prompt with
exclude_last_n=1 so the current turn's user message is not fed back as history,
and that exclusion is a raw positional slice applied before role filtering. So
a row appended after the current-turn user row becomes the physical tail, absorbs
the exclusion, and the user's message survives the slice and is replayed —
sending the request twice. Keeping the role out of RECALL_ROLES does not
rescue this: membership governs whether the row itself is replayed, never which
row the positional slice removes. /note defers its visible line for exactly
this reason, so a marker rides slot._deferred_notes and the seams that already
drain it rather than introducing a third notion of "held". Deferral is arguably
correct anyway: a marker held to the turn's end lands after the closing message
for the item just finished and before anything for the next one. The honest
limit, stated in the tool description: a marker separates turns, so several
emitted in one turn clump at that turn's end.

flush_deferred_notes gained an optional per-entry role/meta, so a /note
entry keeps its exact prior shape while a marker supplies its own. noteSession
is stamped only on note rows — it is the surviving half of the /note wire
contract (isNoteRow returns true for any row carrying it, because cls does
not survive the write path for a non-system role), so stamping it on a marker
would make that predicate call the marker a note.

A renderer registered in both frontend paths, sharing one component. The two
transcript surfaces disagree on the unknown-role fallback: ChatPage's local
role chain falls through to an assistant markdown bubble, while the SDK path
resolves to undefined and draws nothing. Miss one and the row is silently wrong
on that surface — a stray bubble, or an invisible row. Both now dispatch to the
same SectionMarkerRow, so they cannot drift in appearance. EVERY break also draws the
row's own time. An unlabelled one needs it because they all render the same fixed caption,
and a labelled one needs it too: the same label is emitted twice whenever one unit of work
is marked more than once (an agent retry), so the caption cannot separate those either.
The time is the only field that differs in both cases.

Also updates the RFC's front matter (draftin-progress) and its row in the
directory index, which the index's own "Keeping this honest" section requires in
the implementing PR.

It also gives section_marker a capability bullet in src/kiro_crew/config/prompt.md, the
agent prompt that ships in the wheel. Nine of the ten DIRECTIVE_TOOLS members already had
one and this did not, so the agent was never told the tool exists — a discoverability gap
rather than a doc nit, since availability is what that file conveys. A new membership-only
assertion in test_directive_tools_doc_parity.py pins it, so the next directive tool added
cannot miss its bullet silently; the bullet's wording is deliberately not pinned.

Headless producers are refused even when a dashboard tab is open. A cron turn can
run on a user's slot and a sub-agent can share its parent's, so the surface gates are
not sufficient on their own: they ask whether a tab or user surface exists, which a
headless producer riding a user's slot satisfies without a human having asked for
anything. section_marker therefore joins _USER_ORIGIN_DIRECTIVES, which demands
authenticated-human provenance for the turn on top of whichever surface gate already
applies — so a marker cannot be written into a human's transcript on the strength of a
tab it merely inherited.

Also tightened: suggest_followup now requires human provenance. This is a
behaviour change to a pre-existing tool, called out here rather than left to the
diff, because a reviewer or anyone bisecting a broken cron-followup workflow gets
no signal from the section_marker framing above.

What changes. _USER_ORIGIN_DIRECTIVES is created by this PR and carries
suggest_followup alongside section_marker. Before this PR the provenance gate
applied only to _USER_SURFACE_DIRECTIVES (set_project, reset_conversation),
and suggest_followup was admitted on has_dashboard_surface alone. A headless
cron or sub-agent turn running on a session with an INHERITED dashboard tab could
previously post a follow-up card into that session; after this merge it is refused.

Why it belongs here, and why it is intended. The tool already promised this. On
the base, mcp_tools/control.py reads "Restrictions: dashboard sessions only
(Slack, cron, and subagent contexts are rejected)", and the surface-only gate did
not enforce that promise for a headless turn with an inherited tab. So the
tightening makes the documented contract true rather than adding a new
restriction. Closing the surface-versus-provenance gap for section_marker while
leaving the sibling that makes the same promise unenforced would leave
_DASHBOARD_ONLY_DIRECTIVES internally inconsistent. Covered by
test_mcp_followup_dispatch.py, and described in session.md and
followup-suggestions.md.

ask_question, the third member of that set, is deliberately NOT included: its
description promises a degradation off the dashboard (an [OPTIONS:] steer)
rather than a refusal, so its gate stays surface-based.

The label is scrubbed by both redactors before the length cap, not after. Ordering
matters and is deliberate: truncating first can cut a credential mid-pattern so the
regex no longer matches, leaving a partial secret in both the content and meta
surfaces. Redacting first replaces it whole. Redaction can lengthen a value (a 20-char
key becomes a 22-char tag), so the cap is applied to the scrubbed string and still
binds. content and meta['label'] are both derived from that one scrubbed value, so
the drawn field carries the same guarantee as the fallback rather than a weaker one.

Tests

New test/test_section_marker_directive.py (28 tests) plus
website/src/test/SectionMarkerRow.test.tsx, covering the RFC's Phase 1 exit
criteria:

  • The row renders as a labelled rule in both paths — the shared component's
    two hairline rules and role="separator", the SDK registry resolving to its
    own entry (not undrawn, not undefined), and ChatPage dispatching on the
    role and drawing the same component.
  • The persisted row carries its label under meta, asserted against the real
    flush_deferred_notes on a real slot rather than a stand-in — the applier
    tests use a fake, which would share a blind spot with the code if the real
    flush dropped role or meta.
  • A marker requested while a turn is in flight is held, not appended, and lands
    only when a seam drains the hold. An idle slot gets it immediately, since there
    is no positional tail to steal.
  • build_session_replay and _recall_rows return no marker content, with the
    surrounding user/assistant rows as a positive control so a replay that returned
    nothing at all cannot pass.
  • The eleventh marker in one turn is refused, naming the same
    deferred_notes_full condition /note's 429 body does. The directive path has
    no HTTP response to carry a status code — the route is Phase 4 — so the
    refusal is a string.
  • Negative controls: a /note entry still writes as an inject with
    reconcile-note and its noteSession stamp; a 120-char label is accepted so
    the 121-char rejection is about the cap and not about long labels; a label is
    rendered as text, never as markup.

Reverting slot_buffers.py alone makes the two marker-flush assertions fail for
their intended reasons ('inject' == 'section_marker', and the noteSession
stamp) while the /note control still passes, so the flush test detects this
change specifically rather than any change.

Backend: 341 passed across the touched surface (the new file plus the directive,
note-deferral, flush-safety, replay and driver-directive suites). isort,
flake8 and mypy clean; the five touched files are already outside black's
baseline on unmodified main, and the lines added here are black-clean.

The frontend suite runs in this worktree, and the tests named above were
executed against the change. The parity contract in
chatRolesParity.contract.test.ts was additionally traced against its own
extractor at source — it scrapes .role [!=]== '<literal>' from ChatPage.tsx,
which this change deliberately does NOT add: the row is drawn by the shared
renderer registry, so ChatPage.tsx carries no section_marker dispatch at all
and SectionMarkerRow.test.tsx asserts that absence. The registry claims the
role, so both the missing-role and orphaned-role assertions hold.

Screenshots

The fork-failure banner is captured separately by
website/scripts/capture-fork-failure-banner.mjs, whose scene
(website/capture/fork-failure-banner.tsx) mounts the real ErrorNotice and reads its
copy from the live i18n catalog, laid out between a transcript tail and a composer so
the frame shows where the banner lands. Six frames cover both copies: the
over-capacity refusal in each fork direction (the advice differs — an EARLIER message
for a head fork, a LATER one for a tail fork) and the generic failure, in both themes.
The runner asserts one role="alert" and one agent hand-off per scene and prints the
banner-to-composer gap (16px) before shooting, so a scene that did not render writes no
frame rather than a misleading one.

Over-capacity head fork refusal, dark theme

Over-capacity head fork refusal, light theme

Over-capacity tail fork refusal, dark theme

Over-capacity tail fork refusal, light theme

Generic fork failure, dark theme

Generic fork failure, light theme

Captured from the shipped SectionMarkerRow rendered with the app stylesheet via
website/scripts/capture-transcript-section-markers.mjs (Playwright, both themes,
deviceScaleFactor 2). The harness at website/capture/transcript-section-markers.tsx
mounts the real component — the same one ChatPage and the SDK message registry
dispatch to — and the capture script asserts four marker rows are present with the
expected label before shooting, so a frame cannot photograph an empty container.

Each frame shows the four states a reviewer needs to judge: a labelled break between
two units of work, an unlabelled break, which has only its time to go on, a label at the 120-char schema cap, and that
same label in a narrow column, where it wraps between the rules instead of
overflowing.

Dark

Section marker row, dark theme

Light

Section marker row, light theme

@rnoack1
rnoack1 requested a review from a team August 31, 2026 01:35
@rnoack1
rnoack1 requested a review from a team as a code owner August 31, 2026 01:35
@rnoack1
rnoack1 requested a review from bolichen97 August 31, 2026 01:35
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@rnoack1
rnoack1 force-pushed the feat/transcript-section-markers branch 2 times, most recently from eb21253 to 5af90ad Compare August 31, 2026 01:39
@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 Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] 184d3c7

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Well-designed feature, but three independently revertible behavior changes — one a capability withdrawal, one an API-breaking refusal — share one commit's fate.

Watch

  • The PR itself concedes the bundling: "Three independently revertible changes ride this one commit, and that coupling is a cost rather than a design." The fork over-capacity refusal (plus its ErrorNotice report plumbing, i18n keys, capture harness) touches no marker code and is fully separable, so a regression in marker rendering forces a revert that also rolls back a data-loss fix and re-opens the silently-truncating 200 — against the repo's own one-logical-change-per-commit rule. The suggest_followup gating is defensibly coupled (it shares _USER_ORIGIN_DIRECTIVES, introduced here); the fork refusal is not.
    Clears when: the fork refusal + error-report changes move to their own PR, or a maintainer explicitly accepts the shared revert fate.

The marker design itself is sound: RFC-backed with rejected alternatives, positive fail-closed provenance (headless producers refused even with an inherited tab, matching the tool's published contract rather than deleting a pinned decision), reuse of /note's existing hold instead of a third "held" notion, additive/optional wire fields on fork and transfer, and one shared renderer closing the two surfaces' divergent unknown-role fallbacks. The stage-execution refusal honestly prefers rejecting a call over mis-placing the row.

[DESIGN-REVIEWED] 184d3c7

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

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

UX-Verdict: CONCERNS

The surface holds up on the diff — but every new pixel (marker row, fork banner) exists only as fork-side binaries no first-time reader has seen.

Evidence gaps

  • SectionMarkerRow — labelled, unlabelled, 120-char cap, and narrow-wrap states in both themes: the PR-added temp-screenshots/transcript-section-markers/section-markers-{dark,light}.png are binary markers in this lane and no blind read ran. Push the branch to this repository to run the blind-read lane.
  • Fork over-capacity banner — head/tail refusal copy plus generic fallback, both themes: temp-screenshots/fork-failure-banner/*.png, same gap; whether "only the first 10,000 fit" reads cold is unestablished.

Watch

  • Unlabelled markers all render the identical "End of section" caption, distinguished only by a small muted timestamp; two adjacent unlabelled breaks in one scroll may read as duplicates. Low frequency × mild friction; if the blind read stumbles here, consider the turn's date in the stamp when breaks span days.

[UX-REVIEWED] 184d3c7

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

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

Premise-level review of 184d3c7ccf1a37c25aa235091bee8a5c76a21ecd 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 evidence is gathered and verified against the base tree. Final review:

First-Principles-Verdict: CONCERNS

One undeclared rider: sanitize_restored_deferred_notes now redacts every restored /note's content, a behaviour change on a path neither markers nor the description reach.

Not justified as shipped

  1. Restored /note rows now pass through redact() — rides along: not marker-scoped, absent from the description, alters a plain note's own text after a restart.

What this change ships

Intent: let a reader of a long one-item-at-a-time session see where each unit of work ended — an ADDITION (RFC Phase 1), plus two declared behaviour withdrawals.

  1. A labelled rule row via the new section_marker tool, held to the turn boundary — justified
  2. Markers refused during multi-stage plans and for headless callers even with an open tab — justified
  3. Forks and instance transfers carry markers with the turns they separated — justified
  4. suggest_followup withdrawn for cron/sub-agent turns riding a human's slot — justified
  5. Whole-fork of a >10,000-row conversation answers 400 instead of silently dropping oldest rows — justified
  6. Fork-failure banner gains direction-specific copy interpolating the wire max_messages — justified
  7. Identity-gate drops now tell the agent [Not applied: …], reusing the existing note mechanism — justified
  8. Provenance refusals now log a gateway WARNING for all four gated directives — justified
  9. Restored deferred /note content is now redacted and role-validated off disk — rides along (see above)
  10. Capture harnesses and screenshots under temp-screenshots/ — justified

Watch

  • Item 9 reaches plain notes, not only markers: a user's /note containing credential-shaped text comes back altered after a gateway restart. The boundary claim (tampered metadata line) is real, but the scope is wider than the feature needs. Clears when: the scrub is confirmed intended for /note rows too, or is scoped to role == SECTION_MARKER_ROLE entries.
  • The description itself: "reverting THIS COMMIT reverts all three… a regression in the marker rendering and a rollback of the fork data-loss fix share one fate." AGENTS.md's "one logical change per commit, at most two commits per PR" leaves room to decouple; both withdrawals are severable (only suggest_followup's set membership and the chat_fork guard). Clears when: items 4–5 land as their own commit/PR, or a maintainer accepts the shared revert fate.

Subtractions

  • Defer the suggest_followup entry in _USER_ORIGIN_DIRECTIVES (session_directive_apply.py) and the fork_corpus_too_large refusal (chat_fork.py) to their own PR — the author's own analysis shows neither depends on marker code.
  • Shrink the restore-path redact(content) in slot_buffers.py::sanitize_restored_deferred_notes to marker entries, restoring /note rows byte-identical as before.

[FIRST-PRINCIPLES-REVIEWED] 184d3c7

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 184d3c7

@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 Aug 31, 2026
@rnoack1
rnoack1 force-pushed the feat/transcript-section-markers branch from 5af90ad to 1f961ec Compare August 31, 2026 03:24
@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 Aug 31, 2026
@rnoack1
rnoack1 force-pushed the feat/transcript-section-markers branch from 1f961ec to 8e58499 Compare August 31, 2026 04:36
@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 Aug 31, 2026
@rnoack1
rnoack1 force-pushed the feat/transcript-section-markers branch from 8e58499 to 480177e Compare August 31, 2026 05:27
@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 readiness: checking Automated validation is still running labels Aug 31, 2026
@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 1, 2026

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Seems a big design, and also have conflicts with ongoing "crew manager" works which also do summary on session content but showing the "section view" on the right side panels. It is not clear to me how these markers will be commonly available for all providers, not only for kiro-cli.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Description / code mismatch

The Description adds an eighth session directive, but the three places that enumerate DIRECTIVE_TOOLS — including the copy shipped inside the package — still list seven and do not name it.

1. New directive lands as the 8th DIRECTIVE_TOOLS member while three specs and one shipped in-package doc still enumerate seven

The Description says

An MCP tool and session directive section_marker(label?, collapse_earlier?), wired the way reset_conversation is: the stateless tool validates and returns an encoded directive, and the session-aware consumer decodes it at the tool-result event and applies it against its own slot

The code doessection_marker joins DIRECTIVE_TOOLS as its eighth member, and no doc that enumerates that set moves with it:

  • docs/system-specs/modules/session.md:1030 still opens "Seven session-bound MCP tools" and lists seven names.
  • docs/architecture/mcp.md:635-636 and docs/architecture/mcp.md:946-947 each enumerate the directive tools without section_marker.
  • src/kiro_crew/docs/messaging-transport.md:123 — the copy bundled into the wheel and read by users — carries the same seven-name list.

Risk — AGENTS.md § Specification management requires the spec to move in the SAME commit when an API or documented behaviour changes, and the "Read before you touch" table routes MCP-tool changes to docs/architecture/mcp.md and session/slot changes to docs/system-specs/modules/session.md. The precedent is explicit rather than inferred: commit e2fc1dc0b bumped session.md's count Six→Seven and updated the same enumerations when reset_conversation landed, so the repo maintains these lists as current DIRECTIVE_TOOLS membership, not as frozen history. Docs Lint only checks index links, so nothing reports the drift; the next reader of any of these four lists concludes section_marker is not a session directive. docs/architecture/mcp.md self-describes kiro_crew.mcp_tools.build_tool_list() as the authoritative source, which makes this a stale mirror rather than a contradicted contract — and a stale mirror in a doc that ships to users is still what a reader trusts.

Required change — In this commit, add section_marker to the name list and the count at docs/system-specs/modules/session.md:1030, to both DIRECTIVE_TOOLS enumerations at docs/architecture/mcp.md:634-636 and :945-947, and to the list at src/kiro_crew/docs/messaging-transport.md:123. Naming its dashboard-only, human-provenance, and not-during-stage-execution gates where the other directives' gates are described is desirable but not required — the tool description in control.py and the RFC already carry them.

@rnoack1

rnoack1 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

All four enumerations are updated as of b62f64434c577566852c40585cc17140c4942c8f.

The count reads Ten rather than Eight. #5184 added monitor_watch and monitor_stop to DIRECTIVE_TOOLS after you reviewed, so rebasing composed those in alongside section_marker. The frozenset has 10 members now and each list below matches it.

  • docs/system-specs/modules/session.md:1074 opens "Ten session-bound MCP tools" and the list ends reset_conversation, section_marker.
  • docs/architecture/mcp.md:637, under Session-bound directives: autonudge_stop, set_project, reset_conversation, section_marker.
  • docs/architecture/mcp.md:950: suggest_followup, reset_conversation and section_marker.
  • src/kiro_crew/docs/messaging-transport.md:123, the copy bundled into the wheel, carries the same ten names including section_marker.

Line numbers have shifted from the ones you cited, since the branch was rebased a few times in between. monitor_inspect is deliberately not in these lists; it isn't a member of DIRECTIVE_TOOLS.

@rnoack1

rnoack1 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

All four enumerations are current as of bed43888876d26cf362ed2eddbff63d2c83a1012.

The count reads Ten rather than Eight because monitor_watch and monitor_stop joined DIRECTIVE_TOOLS after you reviewed. The frozenset has 10 members now, and each list below matches it:

  • docs/system-specs/modules/session.md:1074 opens "Ten session-bound MCP tools" and the list ends reset_conversation, section_marker.
  • docs/architecture/mcp.md:637, under Session-bound directives: reset_conversation, section_marker.
  • docs/architecture/mcp.md:950: suggest_followup, reset_conversation and section_marker.
  • src/kiro_crew/docs/messaging-transport.md:123, the copy bundled into the wheel, carries the same ten names including section_marker.

Line numbers have shifted from the ones you cited, since the branch has been rebased several times since your review. monitor_inspect is deliberately absent from these lists; it isn't a member of DIRECTIVE_TOOLS.

This supersedes my earlier note, which named a head that has since been replaced.

@rnoack1

rnoack1 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

All four required enumerations are updated at head ab5cbf856dbc39c858dfc2e69cca9d00a3c2694c. The count reads Ten rather than the Eight you'd expect, because monitor_watch and monitor_stop joined DIRECTIVE_TOOLS upstream after you reviewed, so section_marker is its tenth member.

Each site as it reads now:

  1. docs/system-specs/modules/session.md:1074 opens "Ten session-bound MCP tools" and lists all ten names including section_marker. No occurrence of "Seven session-bound" remains in the file.
  2. docs/architecture/mcp.md:637 ends its enumeration with reset_conversation, section_marker.
  3. docs/architecture/mcp.md:950 reads suggest_followup, reset_conversation and section_marker.
  4. src/kiro_crew/docs/messaging-transport.md:123, the copy bundled into the wheel, carries all ten names and cites session_directive.DIRECTIVE_TOOLS.

The line numbers differ from the ones in your review because the base advanced since you wrote it. Content is what I verified, at the head above.

One item from your list is still open: naming the dashboard-only, human-provenance, and not-during-stage-execution gates where the other directives' gates are described. You marked that desirable but not required, so the scoping call is yours rather than mine to assume. All three gates do exist in code if you want them documented: session_directive_apply.py:102 defines _USER_ORIGIN_DIRECTIVES, :226 is the user-surface and producer-provenance check that consumes it, and :965 is the _in_stage_execution gate.

Two corrections to the description you quoted, both already applied: it no longer spells the tool section_marker(label?, collapse_earlier?), since collapse_earlier was dropped and it now reads section_marker(label?); and the rendered divider reads "End of: " rather than the "Section: ..." form the earlier text described, built at session_directive_apply.py:963.

@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 #6823 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 #6823: MERGE_DISCUSSION. Additive on both sides; worth coordinating the close-time flush's payload semantics, but neither blocks the other. Files: src/kiro_crew/dashboard/slot_buffers.py, src/kiro_crew/dashboard/state.py.
  • This PR is OVERLAPPING with PR #4904. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7163: REBASE. Parallel new-role additions following the same precedent; independent user goals, no shared behaviour to reconcile beyond adjacent insertion points. Files: website/src/app-sdk/messageRenderers.tsx.
  • This PR is OVERLAPPING with PR #6813. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7163: REBASE. Adjacent work on the same deferred-note/pending-context structures with no conflicting semantics; 6813's budget accounting already tolerates a context-less entry. Files: src/kiro_crew/dashboard/state.py, src/kiro_crew/dashboard/slot_buffers.py.
  • This PR is OVERLAPPING with PR #7255. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7163: REBASE. Ordering-sensitive but complementary: a chat-page decomposition versus a new transcript row. Whichever merges second must relocate the section_marker branch, its import, and the three ChatPage.tsx source assertions to useChatPageTranscriptController.tsx. Neither should be closed. Files: website/src/pages/ChatPage.tsx, website/src/test/chatRolesParity.contract.test.ts, website/src/test/SectionMarkerRow.test.tsx.
  • This PR is OVERLAPPING with PR #7777. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7163: REBASE. Same file and same design policy for a capped display string, different features; both should land. Files: src/kiro_crew/dashboard/session_directive_apply.py, src/kiro_crew/validation.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
  • This PR is OVERLAPPING with PR #7779. 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.

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

@rnoack1

rnoack1 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

All four enumerations now name section_marker, at head dc4e573dfe011e5046d3a22fe3a2c118fd54af12.

Your line numbers had drifted, so here's what I found against what you quoted:

You quoted Found at Now reads
docs/system-specs/modules/session.md:1030 :1074 "Ten session-bound MCP tools", all ten named
docs/architecture/mcp.md:635-636 :683-686 all ten named
docs/architecture/mcp.md:946-947 :997-999 all ten named
src/kiro_crew/docs/messaging-transport.md:123 :123 all ten named

One correction to the count. Your review said seven going to eight, which was right against 356ef9d9a. monitor_watch and monitor_stop have landed on main since, so DIRECTIVE_TOOLS at src/kiro_crew/session_directive.py:57 now has ten members and the docs say Ten. Writing Eight would have introduced a fresh error.

Control, since a count claim is easy to get wrong: section_marker and reset_conversation occur exactly the same number of times in each of the three files (2 / 2 / 1). reset_conversation is the precedent you cited from e2fc1dc0b, so the new directive is enumerated everywhere its precedent is. A fabricated tool name returns zero in all three, and Seven session-bound returns zero repo-wide.

Your fifth item, the one you marked desirable rather than required, is also in. docs/system-specs/modules/session.md:1084 names the human-provenance gate (set_project, reset_conversation, section_marker, suggest_followup), the _in_stage_execution refusal with the reason it exists, and why ask_question sits outside that set.

Head moved from 790568e8c to dc4e573df for a message-only re-roll of an unrelated Windows shard failure. Tree object and patch-id are identical between the two, so the evidence above reads the same at either.

@rnoack1

rnoack1 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Re-verified at the current head f5cafa22ea62b3549afba003fa1e614c69cbb4fb. My earlier replies named shas that have since been rebased away, so here are all four sites as they read now.

Site Line Reads
docs/system-specs/modules/session.md 1074 "Ten session-bound MCP tools", then all ten named, section_marker last
docs/architecture/mcp.md 683-686 the DIRECTIVE_TOOLS enumeration, ending reset_conversation, section_marker
docs/architecture/mcp.md 997-999 the second enumeration, ending suggest_followup, reset_conversation and section_marker
src/kiro_crew/docs/messaging-transport.md 123 the wheel-bundled copy, all ten named against session_directive.DIRECTIVE_TOOLS

On the count, which is the part worth flagging. Rather than carry "Ten" forward from my earlier reply I re-derived it from the registry: DIRECTIVE_TOOLS at src/kiro_crew/session_directive.py:57 holds ten distinct members. Your review said eighth member, which was right when you wrote it. monitor_watch and monitor_stop have landed on main since, so ten is now correct and the docs say Ten. Worth saying explicitly because writing Eight to match the original review would have introduced a fresh error.

Controls, since a count or an absence is a claim about the query rather than about the file. In each of the three files section_marker and reset_conversation occur the same number of times (3 and 3 in session.md, 2 and 2 in mcp.md, 1 and 1 in messaging-transport.md), so the new entry sits at parity with the directive that set the precedent. A fabricated tool name returns zero in all three, and a token certain to be present returns non-zero, so the search discriminates in both directions. Matching is word-boundary rather than substring, so no longer sibling token can mask or manufacture a hit.

Your fifth item, the one you marked desirable rather than required, is in. docs/system-specs/modules/session.md:1084 names the four directives that require structural user-turn provenance, says which injected turn kinds are refused and why, records that section_marker is additionally refused while a multi-stage plan is executing, and explains why ask_question sits outside that set.

One thing I did not change, so it is visible rather than silent: the parenthetical at src/kiro_crew/dashboard/chat_runner.py:7437-7439 lists six tools as an example of a session-bound tool. It omits monitor_watch, monitor_stop and reset_conversation as well, all three predating this branch, so it was already partial before this change and it is an inline comment rather than a spec or the shipped doc. Happy to make it exhaustive if you would rather it were.

@rnoack1

rnoack1 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

All four enumerations name section_marker at head 2181502ae0e4d7cd7b4338ac858070328a243161. Line numbers below are live, since the ones in your review drifted across several rebases.

Surface Line at head Reads
docs/system-specs/modules/session.md 1074 opens "Ten session-bound MCP tools", then all ten names
docs/architecture/mcp.md 683-686 the DIRECTIVE_TOOLS enumeration, ending reset_conversation, section_marker
docs/architecture/mcp.md 997-999 the second enumeration, ending suggest_followup, reset_conversation and section_marker
src/kiro_crew/docs/messaging-transport.md 123 the copy bundled into the wheel, all ten names against session_directive.DIRECTIVE_TOOLS

One correction worth stating, because following your review literally would now introduce a fresh error. You described section_marker as the eighth member and the docs as saying "Seven", which was right against 356ef9d9a. monitor_watch and monitor_stop have landed on main since, so DIRECTIVE_TOOLS at src/kiro_crew/session_directive.py:57 holds ten members and the prose says Ten. I re-derived that from the registry rather than incrementing your count.

On completeness, since four cited coordinates are your list rather than proof of the full set. I ran a word-boundary census over every .md, .py, .ts and .tsx file in the repo, counting each of the ten member names and every count word. Fourteen files name five or more members; the four above plus session_directive.py, validation.py, mcp_tools/control.py, session_directive_apply.py and test_session_directive.py all carry the full ten. The word "Seven" survives in eight places and not one is a directive count: they are an audit-lane tally, a security-controls count, a call-site count, a documented-members note, and a test fixture literally named "Seven". A fabricated tool name returns zero everywhere, and the same grep finds "Ten" at session.md:1074, so it discriminates in both directions.

The remaining files that name only a subset are not enumerations of this set. learn-cron-dashboard.md and messaging.md both omit reset_conversation as well, which predates this branch, so neither ever listed the registry; the three test files exercise particular tools rather than enumerating them; and chat_runner.py:7437-7439 is a parenthetical example that also omits monitor_watch, monitor_stop and reset_conversation, so it was already partial before this change. Happy to make that one exhaustive if you would prefer it.

Nothing was changed for this reply. Every site was already correct at this head, so the fix landed in an earlier revision rather than in a new commit.

@rnoack1

rnoack1 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

The required change is in the branch. Verified at head c95df6b5e0eecd3e0aba1f2e8f003c3b47ef5f2c by reading each file:

your coordinate at this head
docs/system-specs/modules/session.md:1030 — "Seven session-bound MCP tools" docs/system-specs/modules/session.md:1333 — opens "Ten session-bound MCP tools" and names all ten, section_marker last
docs/architecture/mcp.md:635-636 docs/architecture/mcp.md:683-686DIRECTIVE_TOOLS list ends `reset_conversation`, `section_marker`
docs/architecture/mcp.md:946-947 docs/architecture/mcp.md:999-1002 — same list, ends `reset_conversation` and `section_marker`
src/kiro_crew/docs/messaging-transport.md:123 (wheel-bundled) src/kiro_crew/docs/messaging-transport.md:123 — list ends `reset_conversation`, `section_marker`

Your line numbers moved because the spec grew; the content is at the coordinates above.

One correction worth flagging, because it changes what the lists say rather than whether they were updated. The review asks for section_marker as the eighth member and for the count to go seven → eight. At this head DIRECTIVE_TOOLS has ten members and section_marker is the tenth:

monitor_start, monitor_watch, monitor_update, monitor_stop, autonudge_stop,
set_project, suggest_followup, ask_question, reset_conversation, section_marker

monitor_watch and monitor_stop are not part of this change — they landed on the base branch after the review was written, which is why the count is ten. Writing "eighth" would have introduced a new error into all four lists, so they carry ten.

On the precedent: agreed, and it is the reason the count sentence itself moved rather than just the name lists.

The optional item is also done — the three gates specific to this directive (dashboard-only, human provenance, not during multi-stage plan execution) are named in the spec's session-directive section alongside the other directives' gates, plus a deferral subsection covering which flush mode a seam is in.

@rnoack1

rnoack1 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

The required change is in the code. Re-verified at head 7040ef8e0, which is a new head since my
earlier replies — the branch was rebased to resolve a conflict, so I re-read all four surfaces on the
resolved tree rather than assuming they carried over.

All four enumerations now list the full set, counted by span:

  • docs/system-specs/modules/session.md:1335 — opens "Ten session-bound MCP tools" and names all ten.
  • docs/architecture/mcp.md:683-690 — 10/10.
  • docs/architecture/mcp.md:999-1006 — 10/10.
  • src/kiro_crew/docs/messaging-transport.md:123 — the in-wheel copy — 10/10.

"Seven session-bound" now appears in 0 files, against a positive control that finds the "Ten
session-bound" line, so that is a measured absence rather than a failed grep.

One correction to the wording, so the lists stay accurate: the review asks for section_marker
as the eighth member, but monitor_watch and monitor_stop landed upstream after the review was
written, so it is the tenth. DIRECTIVE_TOOLS has ten members at
src/kiro_crew/session_directive.py:57-70. Writing "eighth" would have introduced a fresh error, so
the counts read "Ten".

On the quoted description line — it named section_marker(label?, collapse_earlier?). That was
wrong and is already gone; the description now reads section_marker(label?). Phase 1 ships no
collapse field by design (docs/request-for-change/rfc-transcript-section-markers.md:333), the
payload carries only label, and the schema is closed, so passing collapse_earlier is rejected —
pinned both ways in test/test_section_marker_directive.py:75 and :91.

On the wiring claim in that same quote, which holds at source on both sides: reset_conversation
and section_marker are adjacent members of the same DIRECTIVE_TOOLS set
(src/kiro_crew/session_directive.py:67 and :68), so both encode through encode() at :152 and
decode through decode() at :309; both are then dispatched as adjacent branches of the same apply
switch (src/kiro_crew/dashboard/session_directive_apply.py:286 and :288), which
src/kiro_crew/dashboard/chat_runner.py calls from its EVENT_TOOL_RESULT handler at :7725. They
differ only in gating, not in the pattern: section_marker is in _DASHBOARD_ONLY_DIRECTIVES
(:86) where reset_conversation is in _USER_SURFACE_DIRECTIVES (:87).

@rnoack1

rnoack1 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Verified at head d82c12e36. All four lists you named already carry the directive, so there is
nothing left to edit — here is what I read on each, counted by span rather than per line (a
single-line count reports the two mcp.md blocks as partial and is misleading):

  • docs/system-specs/modules/session.md:1335 — names all ten members.
  • docs/architecture/mcp.md:683-690 — all ten.
  • docs/architecture/mcp.md:999-1006 — all ten.
  • src/kiro_crew/docs/messaging-transport.md:123 — the in-wheel copy — all ten.

Seven session-bound now appears in 0 files, against a positive control that finds
session-bound MCP tools in three, so that is a measured absence rather than a failed search.

Two corrections to the coordinates, so the record is accurate. The lists live further down than
the line numbers in the review (session.md:1030:1335, mcp.md:635-636/:946-947
:683-690/:999-1006); the files are right, the offsets drifted. And it is the tenth member,
not the eighth — monitor_watch and monitor_stop landed after the review was written, so
DIRECTIVE_TOOLS has ten members at src/kiro_crew/session_directive.py:57-70.

One part of the required change was met differently, and I want to be explicit rather than let it
pass silently.
You asked for the name list and the count. The list is done; the count word is
gonesession.md:1335 now opens "The session-bound MCP tools" with no numeral. That was not
an oversight: a reviewer applied AGENTS.md's own rule that a restated count goes stale silently and
should not be written in prose, so the count was deleted rather than incremented, together with the
test that policed it. The drift you identified cannot recur, because there is no longer a number to
fall behind. If you would rather the count came back, say so and I will restore it — I am flagging
the divergence, not arguing the point.

I also swept for a fifth enumeration in case one was missed. There is none. Six tracked files
mention DIRECTIVE_TOOLS or "session-bound MCP tools": the four above, plus
src/kiro_crew/session_directive.py (the definition) and
src/kiro_crew/dashboard/directive_queue.py, which imports the set and tests membership at
:106 and :219 rather than restating it, so it cannot go stale. Two near-misses I checked and
ruled out: docs/system-specs/modules/learn-cron-dashboard.md:1213 cites three tools as examples of
a class and never says DIRECTIVE_TOOLS, and chat_runner.py's "session-bound" hits are refusal
prose, not a list.

On the optional item — naming the dashboard-only, human-provenance and not-during-stage-execution
gates beside the other directives' gates — I left it. session.md:1343-1352 documents gates as
class-wide properties (the forgery gate, sub-agent refusal, SEL audit) rather than per directive, so
there is no per-directive list to add a row to; the specific gate lives at
src/kiro_crew/dashboard/session_directive_apply.py:86. Happy to add a sentence if you want it in
the spec.

@rnoack1

rnoack1 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

All four enumerations now name every member, verified at head b605489662d2958aaa4a5457764313d0b8b6ff58. Line numbers below are the ones I read at this head, not the ones in the review — the file has moved since it was written, which is why the cited coordinates look unfixed.

The three specs and the shipped in-package doc (10 of 10 names each, counted by span rather than per line, since one list wraps):

  • docs/system-specs/modules/session.md:1335 — "The session-bound MCP tools — monitor_start, monitor_watch, monitor_update, monitor_stop, autonudge_stop, set_project, suggest_followup, ask_question, reset_conversation, section_marker".
  • docs/architecture/mcp.md:684 — first enumeration, all ten.
  • docs/architecture/mcp.md:1000 — second enumeration, all ten.
  • src/kiro_crew/docs/messaging-transport.md:123 — the wheel-bundled copy, all ten.

Seven session-bound now appears in 0 files, against a positive control finding session-bound MCP in 4. The review's session.md:1030 currently renders a row of an ASCII sequence diagram, so that coordinate no longer points at the list.

Two corrections to the premise, both checkable at this head.

It is the tenth member, not the eighth: src/kiro_crew/session_directive.py:57-70 holds ten names. monitor_watch and monitor_stop landed after the review was written, so the set grew by two independently of this change.

The signature quoted from the description, section_marker(label?, collapse_earlier?), does not describe the shipped tool — it takes label? only. Every occurrence of collapse_earlier in the tree documents its absence: docs/request-for-change/rfc-transcript-section-markers.md:333 records that Phase 1 ships no collapse field, and test/test_section_marker_directive.py:76 and :98 pin that the closed schema rejects it.

One part of the required change is deliberately not literal, and it is worth your call. The ask was to add the name "and the count". There is no count at this head: the restated numeral was removed rather than incremented, because a hand-maintained count next to a hand-maintained list is a second thing to forget, and it goes stale silently. What replaces it is a parity test, test/test_directive_tools_doc_parity.py, which reads membership off DIRECTIVE_TOOLS and fails when an enumeration block omits a member — including in the bundled copy, resolved through the installed package rather than the repo path. So the drift you identified cannot recur unnoticed, but the numeral you asked for is gone rather than updated. If you would rather have the count back, say so and I will restore it alongside the test.

@rnoack1

rnoack1 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

All four enumerations you named are addressed at head 6740990e5. Reporting per site, since three moved and one no longer exists.

Two directive tools have landed on the base since your review, so the set is now ten members rather than eight — section_marker is the tenth.

1. docs/system-specs/modules/session.md — the enumeration is now at :1455 (it drifted from the :1030 you cited). It opens The session-bound MCP tools — and lists all ten names including section_marker. It carries no count word, so there is no Seven/Eight left to bump; the sentence no longer states a number.

2. docs/architecture/mcp.md:683 — enumerates all ten, section_marker included.

3. docs/architecture/mcp.md:1000-1002 — enumerates all ten, section_marker included, immediately above the session_directive.DIRECTIVE_TOOLS reference at :1003. This is the site you cited as :945-947.

4. src/kiro_crew/docs/messaging-transport.md — this file no longer exists. It was deleted on the base by the docs refresh that landed after your review, and it is absent from the base tip, from this branch, and from the built package. I checked whether the enumeration had moved to another bundled doc rather than assuming it had gone: scanning every src/kiro_crew/docs/*.md on the base tip for the ten member names, none carries the set (positive control: a term I know is present in that directory matches two files). So there is no successor in-package list to update. An earlier revision of this branch did update that file before the deletion landed.

On your point that nothing reports this drift: test/test_directive_tools_doc_parity.py now asserts two properties against DIRECTIVE_TOOLS — membership in the two enumerating docs, and that any spelled count adjacent to an enumeration equals the real set size. The second is deliberately silent when no numeral is present, which is today's state, so it forbids a wrong count without mandating one. It is mutation-verified: reinserting Seven session-bound MCP tools into the spec takes the test from pass to fail, and restoring the file byte-identically returns it to pass.

@rnoack1

rnoack1 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

All four enumerations are addressed at head ba476f5d9c. Per site, since three moved and one no longer exists.

First, the count: two more directive tools landed on the base after your review, so the set is now ten members rather than eight, and section_marker is the tenth.

1. docs/system-specs/modules/session.md — the enumeration is now at :1455 (it drifted from the :1030 you cited). It reads The session-bound MCP tools — followed by all ten names, section_marker included. It carries no count word at all, so there is no Seven/Eight left to bump — seven session-bound matches 0 times in the file.

2. docs/architecture/mcp.md:686 — the enumeration ends `reset_conversation`, `section_marker`.

3. docs/architecture/mcp.md:1002 — the enumeration ends and `suggest_followup`, `reset_conversation` and `section_marker, immediately above the session_directive.DIRECTIVE_TOOLS reference. This is the site you cited as :945-947.

4. src/kiro_crew/docs/messaging-transport.md — this file no longer exists. It was deleted on the base by 42873e4034 ("docs: refresh prompts, skills and documentation against the shipped code"), which landed after your review, and it is absent from the base tip as well as from this branch.

I checked whether that enumeration had simply moved to another wheel-bundled doc rather than assuming it was gone. Across all 42 files in src/kiro_crew/docs/, no file contains even three of the ten member names, and there is no spelled count beside a directive list anywhere in that directory. The discriminating measurement is reset_conversation, which has shipped as a directive tool since well before this branch: it appears in 0 of the 42 in-package docs, while a control term (session) appears in 35 of them — so the query reaches those files, and the package simply ships no DIRECTIVE_TOOLS enumeration for any member, old or new. There is therefore no successor list to update.

On your point that nothing reports this drift: test/test_directive_tools_doc_parity.py now asserts two properties against DIRECTIVE_TOOLS — that both enumerating docs name every member, and that any spelled count adjacent to an enumeration equals the real set size. The second stays silent when no numeral is present, which is today's state, so it forbids a wrong count without mandating one. It is mutation-verified: reinserting Seven session-bound MCP tools into the spec takes the test from pass to fail, and restoring the file byte-identically returns it to pass.

@rnoack1

rnoack1 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

All four sites you named are addressed at head 865039f45296a550b1ac7ea8387f58bf5aa171be. The line numbers have moved since your review, so here is what each one reads now:

  • docs/system-specs/modules/session.md:1455 — the sentence no longer opens with a count at all. It reads "The session-bound MCP tools" and enumerates all ten, section_marker included. Seven session-bound now matches zero times in that file, against a control of two matches for session-bound, so the search does reach the text.
  • docs/architecture/mcp.md:686 — the first enumeration ends `reset_conversation`, `section_marker`.
  • docs/architecture/mcp.md:1002 — the second ends and `suggest_followup`, `reset_conversation` and `section_marker`.
  • src/kiro_crew/docs/messaging-transport.md — this file is absent at this head and also absent on the base branch; it was removed upstream, so there is no list left in it to update. Nothing succeeded it either: reset_conversation appears in none of the 42 files under src/kiro_crew/docs/, so no in-package doc there enumerates the set any more.

On the part of your finding about the copy shipped inside the package — that is now covered by src/kiro_crew/config/prompt.md:74, which ships in the wheel and gives section_marker its own capability bullet next to reset_conversation. That file was still missing it when you reviewed, and it mattered more than the specs did: it is the surface that tells the agent the tool exists at all, so without the bullet the tool was unreachable rather than merely undocumented.

One correction to the framing rather than to the finding: the set holds ten members now, not eight. monitor_watch and monitor_stop landed upstream between your review and this head, so section_marker is the tenth rather than the eighth.

@rnoack1

rnoack1 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

All four coordinates you named are addressed at head 41352b9d4c0f523904a7ae987937aa88829bdc22. The line numbers have moved with the rebases since your review, so here is what each site reads now:

  • docs/system-specs/modules/session.md — the enumeration is now at :1454. The sentence no longer carries a count word: it opens "The session-bound MCP tools" and lists every member, section_marker included. Seven session-bound matches zero times in that file, against a control of two matches for session-bound, and section_marker appears four times.
  • docs/architecture/mcp.md — first enumeration now at :686, ending `reset_conversation`, `section_marker`.
  • docs/architecture/mcp.md — second enumeration now at :1002, ending and `suggest_followup`, `reset_conversation` and `section_marker`.
  • src/kiro_crew/docs/messaging-transport.md — this file no longer exists, at this head or on the base branch. It was removed upstream in the docs refresh, and nothing succeeded it: reset_conversation appears in none of the 42 files under src/kiro_crew/docs/, so there is no in-wheel enumeration left at that path to update. The wheel-bundled copy your finding is about is covered by src/kiro_crew/config/prompt.md:74, which names section_marker.

One correction to the framing rather than to the finding: the set holds ten members at this head, not seven — monitor_watch and monitor_stop landed upstream between your review and now — so section_marker is the tenth rather than the eighth. The precedent you cited is the one being followed: the count word was removed rather than bumped, so the sentence cannot go stale against the set again.

Also addressed since your review, in the same spirit: the over-capacity fork refusal no longer restates the row ceiling in localized copy. The server sends the enforced limit and every catalog interpolates it, so the number cannot drift out of step with the constant the backend enforces.

@rnoack1

rnoack1 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Re-verified at head 445cdab98566feba78bff1d06aa5e8979d1985a8.

DIRECTIVE_TOOLS has ten members, with section_marker among them — src/kiro_crew/session_directive.py:57.

Every enumeration of the set now names it:

  • docs/system-specs/modules/session.md:1464 — lists all ten individually: monitor_start, monitor_watch, monitor_update, monitor_stop, autonudge_stop, set_project, suggest_followup, ask_question, reset_conversation, section_marker.
  • docs/architecture/mcp.md:698 — the line reads "reset_conversation, section_marker".
  • docs/architecture/mcp.md:1014 — "and suggest_followup, reset_conversation and section_marker".
  • src/kiro_crew/config/prompt.md:74 — the in-package copy, which describes section_marker in the tool list.

The "Seven session-bound MCP tool" count word is gone from all of them; grepping the three specs plus the in-package doc for seven returns zero, with reset_conversation as a positive control confirming the query reaches each file.

The fourth file you named, src/kiro_crew/docs/messaging-transport.md, is not present in the tree; none of the 43 sibling documents in that directory enumerate the set.

…human provenance

A caller can mark the seam between two units of work; the dashboard draws it as a labelled rule.
An in-flight turn defers the append to its boundary (exclude_last_n=1 slices positionally); a multi-stage plan refuses it.
@bolichen97

Copy link
Copy Markdown
Collaborator

@rnoack1 Thanks for this. The audit read the PR at 207765e (audited at 207765e); your head has moved to 184d3c7, so please re-check anything you pushed after that. Nothing of this feature exists on main, so it is not superseded. It does share lines with three open PRs.

#7779 (@jeeshofone, chat_tag) adds a second new session directive through the identical seam: DIRECTIVE_TOOLS in src/kiro_crew/session_directive.py, a schema in src/kiro_crew/validation.py, a handler in src/kiro_crew/mcp_tools/control.py, an elif arm in src/kiro_crew/dashboard/session_directive_apply.py, and the same two enumerations in docs/architecture/mcp.md and docs/system-specs/modules/session.md. Yours goes further and refactors the gate, so _USER_SURFACE_DIRECTIVES becomes the source of a wider _USER_ORIGIN_DIRECTIVES. Suggestion: land this PR first because it owns that refactor, then #7779 rebases and states explicitly which set chat_tag belongs to and which refusal string it gets.

#8696 (@chenmingwei23) is complementary, since it changes what _emit_directive vouches for rather than adding a directive. The catch is that it keys a coverage assertion on DIRECTIVE_TOOLS membership, so section_marker needs a matching entry there. #8696 is in merge conflict and behind main, so this PR is the stronger branch, and #8696 should pick up the new member during its rebase.

#9156 (@buluoray) collides on one line in src/kiro_crew/dashboard/chat_runner.py: it rewrites the _flush_file_changes(slot) call that your marker flush is inserted after. Trivial rebase for whoever merges second.

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) readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants