feat: file new sessions into a sidebar folder at creation (#6118)

[CrysisDeu]

Summary

session_create had no folder argument, so a caller that wanted a session filed in a sidebar folder had to make two calls (session_create then chat_folder_move_session). The window between the two was a real defect path: a folder deleted in between left the session created but unfiled — PR #6109 hardened the goal-conductor skill's instructions around it but explicitly could not close that case, and both the Design Review and First Principles lanes there named the missing field as the root cause.

This PR adds an optional folder to session_create (a folder id or /-separated human path, resolved with chat_folder_create's parent semantics — missing segments created behind the existing tree-shaping gate) and files the slot as part of creation, then deletes the instruction-layer workaround.

Closes #6118

What changed

• validation.py — SESSION_CREATE_SCHEMA gains folder, bounded like every other folder reference (_ARTIFACT_FOLDER_REF_MAX).
• mcp_dashboard.py — session_create resolves the reference through _refuse_tree_shaping_if_unverifiable + _ensure_chat_folder_path (the same gate folder creation already rides, not a second authorization path) and posts the resolved folder_id to the create route.
• dashboard/session_control.py — create_session confirms folder existence read-only under the folder-store lock (read_folders) as the last suspension before the re-gate, assigns folder_id inside the same synchronous window that configures the slot, holds suspend_slots_push across the whole allocation-to-persist span (the slot's first broadcast frame already shows it filed; a slot whose birth write fails is never broadcast), carries the placement in the persist-at-birth metadata, and applies the move path's Model-B un-hide only after the filing has landed.
• dashboard/chat_persistence.py — a forced save of a message-less slot now merges folder_id into an existing metadata line. Birth metadata made empty sessions durable, which made the save path's empty-window early return newly consequential: without the merge, moving/unfiling an empty newborn (or deleting its folder) would not persist, and a restart would resurrect the birth placement. No line is ever invented for a plain empty tab.
• goal-conductor/SKILL.md — the chat_folder_create precondition step and the separate chat_folder_move_session step are deleted; dispatch passes folder at session_create.
• Docs — docs/system-specs/modules/session-control.md updated in the same commit.

Failure semantics (decision, per the issue)

An unresolvable or mid-create-deleted folder refuses the whole create (folder_not_found): the caller asked for a session filed in this folder, "created but unfiled" would silently honor half of that, and no session exists yet so refusal loses nothing — matching the move path's posture. Path segments already created by the mkdir-p walk persist on a refused create and are reported in the error (created folder path: …), the same partial-report posture chat_folder_create takes, since folder deletion is deliberately not a capability this server has.

Authorization adds no new path: app-scoped callers are refused before any folder handling (app_scoped_caller, strictly stronger than the move path's app-ownership rule at this entry point), and unverifiable/delegated callers are refused by the tree-shaping gate plus the session-control strict-identity check.

Pre-push review

Two blind model-pinned lanes (mirroring the CI reviewers) ran before the PR: both blocked on real defects, all fixed with pinning tests —

1. Broadcast atomicity: get_or_create_slot broadcasts on a leading edge, so the new slot could render unfiled for a frame → allocation-to-persist now runs under suspend_slots_push (the move path's own pattern).
2. Refusal side effects: the Model-B un-hide ran before the re-gate, so a refused create could durably un-hide a folder the user hid → existence check is now read-only (read_folders); un-hide runs only after the filing lands.
3. Empty-newborn durability: see chat_persistence.py above.

One advisory rebutted with evidence: the success message's folder label resolves correctly for freshly created segments because _ensure_chat_folder_path appends created rows into the folder list in place before the path render.

Testing

• Schema: id form, path form, omitted default, over-length rejection.
• Endpoint: filed at birth + metadata round-trip; unknown folder refused with nothing allocated; folder deleted mid-create refused under the lock; refused create leaves a hidden folder hidden; filing un-hides like a move; app-scoped caller refused before folder handling; empty-newborn move/unfile survives restart; no metadata line invented for a plain empty tab.
• Structural pins: folder confirmation is the last suspension before the re-gate; filing inside the synchronous configuration window; un-hide after persist; allocation inside suspend_slots_push.
• MCP: one POST carrying the resolved id; mkdir-p segment creation under the gate's verified key; unresolvable folder refuses before the create POST; no gate consulted when folder is omitted.
• Local gates: isort, flake8, mypy (0 issues, 1113 files), black gate (3 graduated baseline entries pruned per the gate's instruction), targeted pytest 361 passed across session-control, MCP folders, conductor, persistence suites.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] af94bb7

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Root-cause fix at the right seam: filing becomes part of creation under the existing tree-shaping gate, and the instruction-layer workaround is deleted rather than kept alongside.

Suggestions

• _fresh_fields() in chat_persistence.py hand-mirrors the full save's metadata enumeration ("Mirrors the FULL save's slot-owned enumeration"); the drift guard keyed to SLOT_OWNED_META_KEYS makes divergence loud, but its manually-maintained excluded set is itself driftable — a follow-up extracting one shared slot-metadata builder used by both save paths would remove the drift class structurally instead of by test.

[DESIGN-REVIEWED] af94bb7

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of af94bb7f53e40910b65e2c6344a83ab4626b5618 — 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 verified. The review contract's output follows.

First-Principles-Verdict: CONCERNS

The feature is a clean cause-level fix; the MCP-side app-scope pre-check is a second copy of the endpoint's rule guarding a residue this PR accepts one branch below.

What this change ships

Intent: let a caller create a session already filed in a sidebar folder, closing the create-then-move window — an ADDITION built as the root-cause fix a prior review named.

1. session_create takes folder (id or path) and files at creation — justified, cause-level (session_create should accept a folder so filing is atomic with creation #6118).
2. Missing path segments are created mkdir-p during the create — justified; reuses _ensure_chat_folder_path.
3. An unresolvable or mid-delete folder refuses the whole create — justified; matches the move posture.
4. The new slot is never observable unfiled (broadcast + persist atomic) — justified, test-pinned.
5. A refused create leaves a hidden folder hidden; un-hide after persist — justified.
6. Metadata changes to an empty newborn now survive a restart — justified general fix of a pre-existing gap.
7. Goal-conductor drops the precondition and move steps; one fewer approval per item — justified deletion.
8. App-scoped callers refused in the MCP layer before folder resolution — duplicate of session_control.py:525.
9. session_create output now redacted, naming folder label and created segments — undeclared (redaction half), but derived from existing folder-name redaction tests.
10. ~1,500-line reformat of test_mcp_dashboard_folders.py plus baseline prune — rides along.

Watch

• The app-scope pre-check (mcp_dashboard.py:922-936) restates the endpoint's eligibility rule (grep app_scoped_caller: authoritative at session_control.py:525,1121; this is the second home, plus an extra /api/chat/slots GET per folder-bearing create). Its zero option is the posture the PR itself takes at fld_err and _ensure_chat_folder_path already documents: "created names come back even alongside an error, so a partial mkdir -p is reported rather than silently left behind" — segments an app can legitimately create anyway, reported in made_note.
• _fresh_fields() and the full save's meta_line block (chat_persistence.py:2819-2978) are two hand-maintained enumerations of slot-owned fields; the falsy-vs-absent semantics genuinely differ and the SLOT_OWNED_META_KEYS drift test pins it, so this is accepted — but a field added to one and not the other is now this file's likeliest future defect.

Subtractions

• Delete the _caller_app_scope pre-check block (mcp_dashboard.py:922-936) — the endpoint's app_scoped_caller refusal already reports through the existing error return, made_note included.
• Defer the test_mcp_dashboard_folders.py reformat and its baseline prune to their own commit, per AGENTS.md's own instruction ("do it in its own commit") — it is over half this diff and none of the feature.

[FIRST-PRINCIPLES-REVIEWED] af94bb7

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Both candidates fail falsification.

Candidate 1 claims folder_label shows the raw folder id for a just-created leaf because chat_folders is a pre-mkdir -p snapshot. But _walk_chat_folder_segments does folders.append(made) (mcp_dashboard.py:607) — it mutates the same chat_folders list object passed into _ensure_chat_folder_path. By the time line 955 runs, the newly-minted leaf row is in chat_folders, so _chat_folder_paths(chat_folders).get(fld_id, fld_id) resolves the human path. The premise ("the new leaf id is absent from the map") is false.

Candidate 2 is self-described as latency, not correctness: suspend_slots_push coalesces and flushes other sessions' updates on block exit — nothing is lost. That is not a crash, data loss, corruption, or removed guard, and the widened span is a deliberate design choice (the slot's first broadcast frame must already show it filed). No observable wrong outcome (c) beyond deferred-but-delivered UI frames.

No grounded Step-2 additions survived scrutiny; the empty-window merge branch is guarded under update_metadata_if's single lock and fails closed as documented.

No findings.

[OPUS-REVIEWED] af94bb7

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

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

[CrysisDeu]

/ai-review override gpt e7cfc1c: The folder-delete-during-birth-persist interleave is the move path's own accepted residual, not a new defect: the delete sweep unfiles the published newborn exactly as it unfiles a just-moved session (deletion is the user's newer intent), the create still returns a live persisted session, and closing it would require holding the folder-store lock across file IO on the event loop; the F2 finding (app-scoped segment creation) IS fixed in this head.

[github-actions]

Human judgment recorded

@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for e7cfc1c67d0b32ef98192ffa54f7cccdbcb3673b.

The folder-delete-during-birth-persist interleave is the move path's own accepted residual, not a new defect: the delete sweep unfiles the published newborn exactly as it unfiles a just-moved session (deletion is the user's newer intent), the create still returns a live persisted session, and closing it would require holding the folder-store lock across file IO on the event loop; the F2 finding (app-scoped segment creation) IS fixed in this head.

This decision applies only to this commit. A new push requires a new judgment.

[github-actions]

Human judgment recorded, but the following reviewer lane(s) could not be re-run automatically: GPT 5.6. Re-run the lane's latest workflow run for e7cfc1c67d0b32ef98192ffa54f7cccdbcb3673b manually from the Actions tab.

[CrysisDeu]

/ai-review override gpt d157416: Same disposition as the prior head (rebase onto post-#6109 main only): the folder-delete-during-birth-persist interleave is the move path's own accepted residual — the delete sweep unfiles the published newborn exactly as it unfiles a just-moved session, deletion being the user's newer intent, and closing it would require holding the folder-store lock across file IO on the event loop.

[github-actions]

AI-review override not recorded: d1574168fa05449d54baed46559864adcf8f63cb is not the current PR head. Re-run the command with e7cfc1c67d0b32ef98192ffa54f7cccdbcb3673b.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

Drive-to-green update — head 7994f9ed4 (was 9f7dec505)

Root cause of everything red on the prior head: the branch's merge with origin/main resolved chat_persistence.py by taking main's rewrite wholesale, silently dropping the feature's update_metadata_if hunk — at the old HEAD, _save_slot_to_history returned early on an empty window and update_metadata_if had no call site. That one dropped hunk explains the shard-3 pinning-test failure (test_moving_an_empty_newborn_before_its_first_message_survives_a_restart), the downstream Coverage Gate red, and the blocking GPT/Opus/Design/First-Principles verdicts, which each name this mechanism.

Changes in this push (single commit, rebased onto current main, CONFLICTING → MERGEABLE):

1. Rebased the feature commit alone onto current main, discarding the bad merge commit. Conflicts resolved as main-structure + PR-logic: union of the state import block (kept main's stop_retry import + the branch's _safe_folder_tree), and main's newer docstring/semantics for test_a_trimmed_transcript_invents_no_cursor_on_an_empty_window in the branch's black-clean formatting.
2. Restored the dropped empty-window forced-save merge in _save_slot_to_history, ported onto main's rewritten file (if not window: now merges into an existing metadata line via update_metadata_if before returning). Red-before proven: the pinning test fails on main's version, passes with the restore.
3. Widened the merge from folder_id-only to every slot-owned field the force/closed save paths persist through it (tags, pin, pinned title + provenance, mode, artifact binding, reasoning effort, colors, project/agent/workspace identity fields, monotonic once-flags, closed/closed_at). Three local review rounds (GPT gpt-5.6-sol + verifiers) each showed a folder-only merge silently drops an acknowledged mutation on a message-less newborn after restart — e.g. tags assigned, a mode switch, an artifact binding, or a close (which would otherwise resurrect the dismissed tab as open). _fresh_fields now mirrors the full save's meta_line enumeration; clearable fields are written explicitly-empty since a merge cannot delete keys and rehydrate treats falsy as cleared.
4. Fields are read under the history lock at write time, not snapshotted before it: the merge dict is populated inside the update_metadata_if guard (which runs under the cross-process lock), so two concurrent force-saves of the same empty slot cannot commit a stale aggregate out of order (a tag save reverting a just-committed pin).
5. Tests added (all red-before-proven): tag/pin/mode/artifact mutations on an empty newborn survive a restart incl. explicit clears; closing an empty newborn persists closed/closed_at; a lock-time-read race test; and a drift guard keyed to SLOT_OWNED_META_KEYS pinning parity between the empty-window merge and the full save.

Local gates: isort / flake8 / mypy / black(changed files) all clean; full backend suite: 0 failures attributable to this diff (95 host-env failures reproduce byte-identically on pristine main — userns EPERM / home-dir classification family). Local model-pinned reviewers: Opus claude-opus-4.8 NO BLOCKING FINDINGS; GPT gpt-5.6-sol two blockers fixed as above; final verification round: VERIFIED.

Authorship: single commit preserving Zezhen Xu (@CrysisDeu) as author (noreply identity), with a Co-authored-by: Kiro Crew trailer.

Note on the reviewed birth-write race (folder deleted while the birth persist is in flight): left as the code's documented accepted residual — the dangling folder_id reads as "(unfiled)" everywhere (sidebar maps only known folder ids, history grouping routes unknown ids to Unfiled, resume clears a missing folder under the locked existence check), matching the move-vs-delete posture. No data loss or boundary crossing.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

• Failed metadata merges are reported as durable — span=1ce50744f47d — disposition: fixed in af94bb7f5.

state.conversation_log.update_metadata_if(...) followed by unconditional return True — empty newborn close + transient metadata read failure -> close save reports success -> restart resurrects the closed tab.

Verified real: update_metadata_if fails closed on an unreadable record without invoking the guard, and flush_slot_now skips message-less slots, so the close path's exception-based rollback was the only safety and we swallowed the failure. Fix: the empty-window merge now distinguishes the two False outcomes by whether the guard ran — the by-design skip (guard ran, record empty: a line-less plain tab) stays silent; an unreadable record (guard never invoked) raises OSError, so best_effort savers log + mark the slot dirty and archival savers (close, best_effort=False) roll back and keep the slot instead of reporting a durable write. Regression test test_an_unreadable_record_fails_the_empty_window_merge_loudly is red-before-proven (fails on the prior head's code, passes at af94bb7f5) and also pins the silent by-design skip.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

• MCP-side app-scope pre-check restates the endpoint's rule — disposition: rebutted (kept as shipped).

Delete the _caller_app_scope pre-check block (mcp_dashboard.py:922-936) — the endpoint's app_scoped_caller refusal already reports through the existing error return, made_note included.

The pre-check is not a pure duplicate: it refuses an app-scoped caller before _ensure_chat_folder_path runs, so a caller that will certainly be refused never mutates the folder tree at all — with only the endpoint check, a refused app-scoped create can still mkdir-p folder segments as a side effect (reported in made_note, but created nonetheless). Whether that fail-fast-before-side-effects posture is worth the second home (plus one extra GET per folder-bearing create) is the author's shipped design choice, and this drive's mandate is to restore and green the PR, not to redesign it. If maintainers prefer the single-home rule, it is a clean follow-up subtraction.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

• Two hand-maintained enumerations of slot-owned fields — disposition: rebutted (accepted risk, mechanically pinned).

_fresh_fields() and the full save's meta_line block (chat_persistence.py:2819-2978) are two hand-maintained enumerations of slot-owned fields; ... a field added to one and not the other is now this file's likeliest future defect.

Acknowledged as the review itself notes: the falsy-vs-absent semantics genuinely differ (a merge cannot delete keys; the reconstructive full save expresses "cleared" as absence), so the two shapes cannot share one literal enumeration without changing the full save's on-disk format — out of scope for a drive that restores a dropped hunk. The drift risk is exactly what test_the_empty_window_merge_mirrors_the_full_saves_slot_owned_fields exists for: it is keyed to SLOT_OWNED_META_KEYS, so a field added to the full save without a merge counterpart fails CI rather than regressing silently.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

• Defer the test_mcp_dashboard_folders.py reformat + baseline prune to its own commit — disposition: rebutted (splitting now would break the single-commit invariant this drive operates under).

Defer the test_mcp_dashboard_folders.py reformat and its baseline prune to their own commit, per AGENTS.md's own instruction ("do it in its own commit") — it is over half this diff and none of the feature.

The reformat came with the original feature commit (the author black-formatted the file they were substantially extending and pruned it from .github/black-baseline.txt accordingly); the diff-scoped black gate requires a changed non-baselined file to be fully black-clean, so the reformat and the feature edits to this file are coupled — a separate commit would still have to land under this PR, and repo convention (PR Hygiene) drives toward one commit per PR. Verified pure reflow: git diff --ignore-all-space on the file shows formatting-only changes outside the feature's own test additions. If maintainers want it split at merge time, the reflow hunks are cleanly separable.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

✅ Review-ready at head af94bb7f53e40910b65e2c6344a83ab4626b5618

• PR Readiness: passed · mergeable · single commit (author Zezhen Xu preserved, Co-authored-by: Kiro Crew)
• All five review lanes green: GPT 5.6 ✅ · Opus 4.8 ✅ · Design ✅ · UX ✅ · First Principles 🟡 CONCERNS (advisory — each concern answered in individual disposition comments above)
• 0 failing checks · 0 unresolved threads
• Drive summary: rebased the feature commit onto current main (dropping the merge that had silently discarded the chat_persistence.py forced-save hunk), restored and hardened that hunk (full slot-owned field parity, lock-time field evaluation, loud failure on unreadable records), two GPT blocking findings fixed with red-before-proven tests, one runner-cancellation flake cleared by rerun.

Auto-merge is not armed (drive-to-green never merges) — ready for maintainer review and merge. Closes #6118 on merge.

[bolichen97]

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 #6237 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 #6237: REBASE. The op-shaped create must carry PR #6146's folder argument and the skill must keep the single-call filing, or a shipped atomic-filing fix is reverted. Files: src/kiro_crew/builtin_skills/goal-conductor/SKILL.md.
• PR #4623 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #4623: CONTINUE_DEVELOPMENT. Main covers the headline close/force behaviour completely, so the PR must be rescoped and retitled to its real delta (the empty rewrite skip) rather than closed: that delta is absent from main and unpinned by any test there. Files: src/kiro_crew/dashboard/chat_persistence.py.

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