feat(session-control): let a cron dispatch the sessions it creates - #8335
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound admission-plus-fence design, but the late fail-closed flip contradicts the spec it ships with and likely refuses the PR's own headline workflow. Watch
Suggestions
[DESIGN-REVIEWED] 71e544e |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All checks complete. Here is my review. First-Principles-Verdict: CONCERNS Three already-merged fixes ride along undeclared, and the shipped spec still documents the fail-open residual the branch's final commit closed. What this change shipsIntent: let a scheduled cron job open and drive its own worker sessions so a morning dispatch can fan tasks out in parallel — an ADDITION (
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 71e544e |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBoth candidates fall. Candidate 1: the only writer of a I traced the new machinery myself: No findings. [OPUS-REVIEWED] 71e544e Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
8c5e3c2 to
a0c88b5
Compare
ai-review-dispositionGPT: App-owned crons bypass app isolation ( The finding is correct and the mechanism is exactly as stated. Fixed rather than reverted, because the derived-but-ignored identity is the defect, not the exemption: Two details worth flagging for the re-review:
Tests: 6 new cases in
|
a0c88b5 to
552ab5b
Compare
ai-review-dispositionGPT: cron authority is identified only by slot-key prefix / created child is an unfenced deputy -- ACCEPTED and FIXED in 552ab5b. The escalation chain is exactly as described, and I reproduced each link against the source rather than taking it on trust:
Fixed by making the fence follow AUTHORITY rather than spelling. Two properties worth checking in the re-review, because they are what make this a fix rather than a patch:
This tightens the MEMBER path too, and deliberately so: the same deputy hole existed there before this PR, since a member's worker also carries a Tests: 7 new cases in I did not take the alternative you offered (revert cron admission), because the deputy hole is a property of the fence's identity test rather than of admitting a cron: it was already reachable through a member, and reverting would have left it. |
552ab5b to
f0bbdc6
Compare
ai-review-dispositionBoth findings on 552ab5b ACCEPTED and FIXED in f0bbdc6. 1. Sticky attendance disables the ownership fence (
|
ba3fac0 to
716ed46
Compare
ai-review-dispositionGPT: generic app-created crons are treated as user-owned -- ACCEPTED and FIXED in 716ed46. Opus is clean on ba3fac0. Verified before accepting, because four straight correct findings do not make a fifth correct by default. Fixed by checking both spellings, with the second delegating to One residual is accepted rather than closed, and I would rather state it than have it found. When A note on the shape of this round, addressed to whoever reviews nextThis is the fourth consecutive blocking finding of the same class: a security decision keyed on one spelling of an identity while another spelling carries the same authority. In order -- the ownership fence keyed on slot-key prefix while a created child carried the same authority under a Every one was real and every fix was local, but the sequence is evidence about the surface rather than about the individual defects: this module decides authority from several partial spellings of "who is this", and enumerating them one review round at a time converges slowly and cannot be shown to have terminated. I have flagged the scope question to the repository owner rather than deciding it myself. The options as I see them: continue converging here; or split the app-authority-through-cron dimension into its own change, land the cron-dispatch fence (which the first three rounds have made solid and which the remaining findings do not touch), and audit the identity surface as one piece with its own tests. Standing dispositionsAll three current non-review reds are infrastructure, none touched by this diff (four files: one Python module, one spec, two test files):
|
ai-review-disposition -- finding ACCEPTED, fix NOT in this head. Handing the design decision to the repository owner.GPT on 716ed46: cron exemption bypasses channel and closed-app containment. Accepted as real. Verified: I am not patching this one, and the reason is the shape of the last five rounds rather than the difficulty of the patch. Why this stops here instead of convergingFive consecutive blocking findings, all one class: a security decision keyed on one spelling of an identity while another spelling carries the same authority.
Every finding was real and every fix was correct. But round 5's hole is inside the code round 4 added, which is the signal worth acting on: each patch introduces the next partial spelling, and a blocklist of spellings cannot be shown to have terminated. Continuing costs another round per spelling with no bound on how many remain. The fix I believe is right, and why it is not mine to landInvert the test. Instead of enumerating ways a cron's owner might be untrustworthy, require positive proof that it IS trustworthy: the cron's authority is only as good as its owner's, so admit a cron caller only when its That terminates the sequence by construction. Two reasons I am not landing it unilaterally:
What is solid in this head, for whoever picks it upRounds 1-3 addressed the cron-dispatch fence itself and none of the later findings has touched it: the Open finding on this head, un-fixed and therefore listed here for the handoff: the one above. Nothing else is outstanding from either reviewer -- Opus is clean as of ba3fac0. Standing dispositions (all infrastructure, none from this diff)
|
… resolved An app records a cron's authority only in session_key via mcp_cron.cron_add (no created_by). If the app then closes its session, the owning slot is gone and _app_owned_cron_refusal could not read _app, so it fell through and allowed the cron to create/control a persistent non-app session -- an app confinement escape (anchor backend-security-controls). Fail closed on an unresolvable nonempty owning_key, matching the missing-job and unreadable-registry cases. A live non-app owner is still allowed; a live app owner is still refused.
UX Review (Fable 5) — ✅ PASSUX-level review of All evidence reviewed. The user-facing surface here is small: a disclosure-identity fix in UX-Verdict: PASS Fixes a real annoyance — expanding one tool row no longer expands its same-tick siblings — with no visible surface changed or moved. [UX-REVIEWED] 71e544e |
|
Disposition on the fail-closed change in this PR, recorded so the tradeoff is not silent. GPT's blocking finding was REAL and is fixed here: This reverses the tradeoff the earlier docstring had taken on purpose, so the docstring was It is a stopgap, not the resolution. The durable fix is to record Tests: |
… at run start
A cron's cron-{job_id} dashboard slot was created only by the result
injection, which runs after a turn completes. During a brand-new job's
FIRST run the tab therefore did not exist: session-control caller
identity resolves by walking live slot links (caller_slot_key matching
the presented cron:{job_id}), so every verb refused caller_unidentified
— on exactly the run a person watches after creating a job (kirodotdev#8335 made
crons legitimate session-control callers). The dashboard-surface
registry had the same first-run hole for sub-agent event routing,
completion injection, and widget/question/approval delivery, and the
executor's silent/dedup delivery paths gate on has_slot, so a silent
job's first result never reached its tab either. From run 2 onward the
previous delivery's slot masked all of it.
Fix: ensure_cron_slot pre-creates the tab at run start for a job that
will get one at delivery anyway (persistent_session and not
hide_in_chat), placed after the fire-time gate in the executor
callback. The link/hydration invariant the issue names moves with it:
_bind_cron_slot is now the single shared core for BOTH creator paths,
linking and hydrating (prefetch_cron_history, off-loop) in the same
step, so the injection's unlink guard stays an idempotent no-op
whichever path created the slot. Ineligible jobs (per-run session,
hidden) keep the fail-closed no-tab/no-identity/no-dispatch contract.
Decided explicitly, as kirodotdev#8336 asks: a first run that starts and then
fails now leaves an empty tab where none appeared before. Gating the
pre-create on the run reaching injection would recreate the hole this
fixes — identity must exist DURING the run.
Fixes kirodotdev#8336
… at run start (#9030) * fix(cron): give a job's first run its tab — and its caller identity — at run start A cron's cron-{job_id} dashboard slot was created only by the result injection, which runs after a turn completes. During a brand-new job's FIRST run the tab therefore did not exist: session-control caller identity resolves by walking live slot links (caller_slot_key matching the presented cron:{job_id}), so every verb refused caller_unidentified — on exactly the run a person watches after creating a job (#8335 made crons legitimate session-control callers). The dashboard-surface registry had the same first-run hole for sub-agent event routing, completion injection, and widget/question/approval delivery, and the executor's silent/dedup delivery paths gate on has_slot, so a silent job's first result never reached its tab either. From run 2 onward the previous delivery's slot masked all of it. Fix: ensure_cron_slot pre-creates the tab at run start for a job that will get one at delivery anyway (persistent_session and not hide_in_chat), placed after the fire-time gate in the executor callback. The link/hydration invariant the issue names moves with it: _bind_cron_slot is now the single shared core for BOTH creator paths, linking and hydrating (prefetch_cron_history, off-loop) in the same step, so the injection's unlink guard stays an idempotent no-op whichever path created the slot. Ineligible jobs (per-run session, hidden) keep the fail-closed no-tab/no-identity/no-dispatch contract. Decided explicitly, as #8336 asks: a first run that starts and then fails now leaves an empty tab where none appeared before. Gating the pre-create on the run reaching injection would recreate the hole this fixes — identity must exist DURING the run. Fixes #8336 * fix(cron): make the first-run tab pre-create best-effort — never consume the run it serves The pre-create added for #8336 awaited ensure_cron_slot bare in the pre-dispatch window. A review lane convicted the chain: _execute clears run_never_started before the callback and its except arm never re-arms it, so a transcript/store failure (or the wake deadline cancelling AT the await) propagated out of a window where the retention marker was down — and the delete site then consumed a delete_after_run one-shot, the default at-scheduled shape, for a run that never dispatched. _pre_create_cron_slot now wraps the await in the fire-time gate's proven contract: run_never_started armed for exactly the duration of the await (CancelledError escapes except Exception with the marker standing, so the one-shot is retained), ordinary failures contained with a warning (the run proceeds without the pre-created tab — losing first-run identity for that run only, which is the status quo the feature improves on), and a linear clear before dispatch so a healthy one-shot is still consumed. record_failure() deliberately not called: an unminted tab is not a defect of the job. Four pins in TestPreCreateGuard mirror cron.py's delete_owed expression verbatim: raise-contained, cancel-retains-one-shot, healthy bind through the guard, ineligible job untouched (no new deny path). Owning doc updated with the best-effort contract.
What is the problem?
A cron job cannot use session control, and the refusal does not depend on which agent the job runs as. A job mapped to
kirocrew-conductor, an agent installed specifically for session control with nofs_writeand nocode, was refused exactly like a job running as the default agent, because the gate reads the slot's NAME.Three refusals fired, all keyed on the
cron-slot-key prefix or on a field a cron borrowed:create_sessionunattended_caller(UNATTENDED_SLOT_PREFIXES)_refuse_ineligible_creatorlinked_session_caller, becauseinject_cron_result_to_dashboardsetslinked_session_key = cron:<job_id>authorize_targetunattended_caller, the one that gatessession_send/read/stopagent.session_controldid not reach any of them: the config gate sits above the prefix check. "View last result" was not a way around it either, becauseapi_cron_to_chatreuses the samecron-<job_id>slot.Why this issue matters to the user
The blocked workflow is a morning dispatch: a 06:30 job enumerates the tasks due today, the user replies "work on 1, 2 and 6", and each task should get its own session so the three run in parallel with separate context. The fan-out needs
session_createplussession_send, and both were refused.Beyond that one workflow, the gate was checking the wrong property and its stated reason was already contradicted.
Wrong property. Capability is bounded per agent already:
@kirocrew-dashboardis an opt-in per-agent MCP server, deliberately absent from the default agent's spec, and_install_conductor_agent()mounting it IS the explicit assignment. That layer is fail-closed by construction, since an agent without the mount never sees the verbs. The prefix check added a gate on top that cannot tell a session-control agent from a general-purpose one.Contradicted reason. The comment said a cron must not "type into the user's live conversations unattended", and named
send_messageas the supported alternative. Butsend_message(session="origin")resolves the originating dashboard slot and, when it is idle, callsspawn_guarded_turn->_run_chat. That is an unattended scheduled job starting a turn in the user's live conversation, through a documented path. What actually separates the two is SCOPE:_resolve_session_targetaccepts only the literal"origin"and rejects arbitrary slot keys, so a cron can talk back to its owner and nothing else.Scope is a defensible line. It was not the line the code stated, and it does not justify refusing
create_sessionat all: a session the cron just created is empty, so there is no third party's turn to interrupt and nothing to clobber.How our fix solves it
The chain runs symptom, then stated reason, then real invariant, then the mechanism that already expresses it.
The real invariant is "a scheduled job must not reach the user's own sessions", and this repo already has a fence for exactly that shape.
authorize_targetrefuses a crew member on any slot it did not create (_created_by, 403), and that fence is precisely why_MEMBER_DASHBOARD_GRANTSmay auto-approve the write verbs while_CONDUCTOR_DASHBOARD_GRANTSwithholds them; the tuple comments state the reasoning. So:create_sessionandauthorize_targetadmit acron-caller, and the fence binds it. Both admissions and the fence read one predicate (_caller_is_ownership_fenced) so they cannot drift apart. Fail-closed on an unowned slot, which is what an ownerless rehydrate looks like.workflow-stays refused. It is minted only once its originating tab is gone, so there is no owning session to fence it to. Membership ofUNATTENDED_SLOT_PREFIXESis now the fail direction for any prefix added later: a new unattended surface is refused as a source until it is given a fence of its own.cron:<job_id>link is exempt from the caller-side channel-link refusals. Those exist for links that republish to a Slack or Telegram audience; a cron link names the job's own run transcript and republishes to nobody. Both caller-side sites are exempted together, keeping_refuse_ineligible_creatoran exact mirror ofauthorize_target's caller half as its docstring requires. The TARGET-side refusal is untouched.unattended_targetstands. A cron drives its own children, never another job's tab.SlotOrigin.CRON, notUSER. This is the one place the change would otherwise open something. A cron's own slot is tagged CRON so its output stays outside theslots:userWS scope ("a USER label would expose it to any app holdingslots:user"), and the trust model states the same rule from the other side: inferring USER for a background caller "put cron output insideslots:user". A USER-labelled child would hand a cron that exposure by the route of creating a session and writing there. Nothing is lost, because only app tokens are filtered by origin (_serialize_for_clientreturns the unfiltered payload to a dashboard user), so the child stays in the sidebar exactly as a cron tab does.Runaway creation needed no new work. The existing guards were written for this caller:
create_rate_limited(5-minute window),slot_cap_reached, andcreator_slot_cap_reachedkeyed on the caller so each job gets its own share, whose comment already reasons about "an automated creator looping on it".What tests we did
New
test/test_cron_session_control.py, 19 tests against REAL slot objects (the suite's own doctrine, since the guards readlinked_session_key/_created_by/_originoff the production class and a permissive double would let a dead guard look alive):workflow-caller still getsunattended_caller; the global switch still refuses a cron withsession_control_disabled.not_creatoron one it did not, getsunattended_targeton another job's tab, and fails closed on an unowned slot.slack:link on the same slot still getslinked_session_caller, and an app-scoped cron tab still getsapp_scoped_caller, so the exemption widens one refusal rather than the set.SlotOrigin.CRON; an ordinary caller's child is stillSlotOrigin.USER.Updated
test_scheduled_caller_cannot_control_anyone, which asserted the old contract, to assert the fence instead, and addedtest_workflow_caller_cannot_control_anyonebeside it so the surviving refusal keeps a test of its own.238 tests green across
test_cron_session_control.py,test_session_control.py,test_member_session_control.py,test_session_control_boundaries.py,test_queue_drain_revalidation.pyandtest_session_pulse_session_count.py. black, isort, flake8 and mypy clean on the changed files.The spec (
docs/system-specs/modules/session-control.md) moves with the code: two refusal-table rows and a new "Cron callers" section stating the admission, the fence, the link exemption, the origin rule, and the per-agent capability layer the prefix could not see.Any other suggestions on the work
Two things I found while doing this and deliberately did not fold in.
A cron's caller identity resolves only through its live tab, and that tab is minted after the first run's result is injected.
caller_slot_keywalks live slots, and the only creator site for acron-<job_id>slot is the post-run injection path. So a brand-new job's FIRST run has no tab, resolves to no caller, and is refusedcaller_unidentified: the capability lands from its second run onward. Jobs withpersistent_session=Falseorhide_in_chat=Truenever get a tab and so never become eligible, which is coherent and fail-closed, but the first-run gap is a bad first impression for exactly the person testing a new job. Fixing it means ensuring the tab exists at run start, which has to movehydrate_slot_from_historyalong with it, because the injection hydrates underif not slot.linked_session_keyand pre-linking the slot without moving the hydration would silently skip it. That belongs in its own PR against the cron delivery path, not bundled with an authorization change. Filed separately.Approval, not authorization, is the remaining step for a hands-off fan-out.
_CONDUCTOR_DASHBOARD_GRANTSwithholdssession_send, correctly, because a conductor agent also runs in dashboard sessions where no ownership fence applies. A cron whose dispatch must run without an approval prompt needs the write verbs in its own agent'sallowedTools. That is the existing per-agent extension point and needs no code change, so it is documented in the spec rather than widened here.Closes #8332