Skip to content

feat(session-control): let a cron dispatch the sessions it creates - #8335

Merged
iamwhatever merged 2 commits into
mainfrom
feat/cron-session-control-8332
Sep 4, 2026
Merged

feat(session-control): let a cron dispatch the sessions it creates#8335
iamwhatever merged 2 commits into
mainfrom
feat/cron-session-control-8332

Conversation

@chenmingwei23

Copy link
Copy Markdown
Contributor

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 no fs_write and no code, 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:

gate code
create_session unattended_caller (UNATTENDED_SLOT_PREFIXES)
_refuse_ineligible_creator linked_session_caller, because inject_cron_result_to_dashboard sets linked_session_key = cron:<job_id>
authorize_target unattended_caller, the one that gates session_send / read / stop

agent.session_control did not reach any of them: the config gate sits above the prefix check. "View last result" was not a way around it either, because api_cron_to_chat reuses the same cron-<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_create plus session_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-dashboard is 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_message as the supported alternative. But send_message(session="origin") resolves the originating dashboard slot and, when it is idle, calls spawn_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_target accepts 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_session at 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_target refuses a crew member on any slot it did not create (_created_by, 403), and that fence is precisely why _MEMBER_DASHBOARD_GRANTS may auto-approve the write verbs while _CONDUCTOR_DASHBOARD_GRANTS withholds them; the tuple comments state the reasoning. So:

  1. create_session and authorize_target admit a cron- 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.
  2. workflow- stays refused. It is minted only once its originating tab is gone, so there is no owning session to fence it to. Membership of UNATTENDED_SLOT_PREFIXES is 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.
  3. A 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_creator an exact mirror of authorize_target's caller half as its docstring requires. The TARGET-side refusal is untouched.
  4. unattended_target stands. A cron drives its own children, never another job's tab.
  5. The global switch still gates a cron. Unlike a member it gets no bypass: the switch is the user's statement that agents may open and drive sessions at all, and a job running while they are asleep is the last caller that should be exempt from it.
  6. A session a cron creates is tagged SlotOrigin.CRON, not USER. This is the one place the change would otherwise open something. A cron's own slot is tagged CRON so its output stays outside the slots:user WS scope ("a USER label would expose it to any app holding slots:user"), and the trust model states the same rule from the other side: inferring USER for a background caller "put cron output inside slots: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_client returns 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, and creator_slot_cap_reached keyed 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 read linked_session_key / _created_by / _origin off the production class and a permissive double would let a dead guard look alive):

  • admission: a cron caller passes the unattended refusal and creates a session; a workflow- caller still gets unattended_caller; the global switch still refuses a cron with session_control_disabled.
  • the fence: a cron reaches a session it created, gets not_creator on one it did not, gets unattended_target on another job's tab, and fails closed on an unowned slot.
  • the link exemption: a cron link passes, a slack: link on the same slot still gets linked_session_caller, and an app-scoped cron tab still gets app_scoped_caller, so the exemption widens one refusal rather than the set.
  • origin: a cron caller's child is SlotOrigin.CRON; an ordinary caller's child is still SlotOrigin.USER.

Updated test_scheduled_caller_cannot_control_anyone, which asserted the old contract, to assert the fence instead, and added test_workflow_caller_cannot_control_anyone beside 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.py and test_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_key walks live slots, and the only creator site for a cron-<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 refused caller_unidentified: the capability lands from its second run onward. Jobs with persistent_session=False or hide_in_chat=True never 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 move hydrate_slot_from_history along with it, because the injection hydrates under if not slot.linked_session_key and 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_GRANTS withholds session_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's allowedTools. 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

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

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

  • Spec contradicts code on the fail direction. Commit 71e544e75 makes an unresolvable session_key owner refuse (cron_owner_unverifiable), but the spec still carries the opposite: "One residual is accepted rather than closed… the refusal returns nothing for it" (session-control.md:240), directly contradicting the refusal-table row ("an unverifiable owner fails closed") three sections above. The next reader of the residual paragraph will reason from the wrong invariant; the repo rule is spec-moves-in-the-same-commit. Fix the paragraph before merge.
  • Fail-closed on a closed authoring tab likely breaks the common agent-authored job. A cron created by asking the agent in chat gets session_key from cron_add; once that tab closes — the normal state for an unattended 06:30 job — every dispatch is refused. The code's recovery claim ("that caller can reopen a tab") is asserted, not demonstrated: nothing shown re-mints the recorded key. Verify that recovery path actually works, or the feature only functions while the authoring tab stays open.

Suggestions

  • The root ambiguity is that cron_add records no principal: stamping an owner tag (app: vs user) on the job at creation would let this gate answer without guessing, removing the fail-direction dilemma entirely — a follow-up on the cron-creation path.

[DESIGN-REVIEWED] 71e544e

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 71e544e750da30fc89dfb2d3fc3d63946ccc1c29 — 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 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 ships

Intent: let a scheduled cron job open and drive its own worker sessions so a morning dispatch can fan tasks out in parallel — an ADDITION (feat).

  1. A scheduled job can now open new sessions — justified (named blocked workflow, gate checked the wrong property)
  2. A job can send/read/stop only sessions it created itself — justified, reuses the existing _created_by member fence
  3. Background workflow- slots stay fully refused — justified (no owner to fence to)
  4. An app's own scheduled job is refused, ownership read from the job — justified (app-confinement boundary, derived)
  5. A cron whose authoring tab has closed is now refused — code contradicts the spec shipped beside it
  6. Sessions a cron opens stay outside apps' slots:user scope (CRON tag, transitive) — justified (trust-model rule)
  7. Spec gains a "Cron callers" section — carries a stale fail-open paragraph and an orphaned fragment
  8. CI ignores committed screenshot evidence when scoping tests — rides along, duplicate of main 39ae88b (fix(ci): exclude temp-screenshots evidence from the backend change bucket (#8027) #8438)
  9. Readiness sweep no longer cancels its own queued run — rides along, duplicate of main 3d75f84 (fix(ci): stop the readiness sweep cancelling its own queued successor (#8026) #8436)
  10. Each same-tick tool row expands independently in chat — rides along, duplicate of main dabd83e (fix(chat): give each same-tick tool row its own disclosure identity #8289)

Watch

Subtractions

  • Rebase onto main and drop the rider hunks: ci.yml, pr-readiness-sweep.yml, local-gate.py, run_scoped_tests.py, ChatPage.tsx, plus test_local_gate.py additions, test_pr_readiness_sweep_policy.py, and both toolDisclosureKey.* test files — all already shipped (39ae88b, 3d75f84, dabd83e).
  • Delete session-control.md:240-246 (the stale "residual is accepted" paragraph) and the dangling "Applied at both caller-side sites" fragment at lines 248-250; one sentence stating the fail-closed direction already exists in the refusal-table row at line 95.

[FIRST-PRINCIPLES-REVIEWED] 71e544e

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Both candidates fall. Candidate 1: the only writer of a cron:-prefixed linked_session_key is cron_inject.py:425, which writes it onto the cron slot itself (cron- key) — so no non-cron slot ever carries such a link, and any genuine cron caller is still caught by the _cron_caller branch of _caller_is_ownership_fenced. The exemption opens no path; (a) has no concrete input. Candidate 2 is a cosmetic HTTP-status difference consistent with the file's existing convention — not a defect, and status/consistency is not a class this pass reports.

I traced the new machinery myself: _app_owned_cron_refusal (both spellings, fail-closed on missing job / unreadable registry / closed authoring session, placed before _resolve_slot to avoid the existence oracle), the fence broadening to any _created_by-tagged caller (a tightening, purposeful), and the SlotOrigin.CRON propagation via _origin. No new grounded defect at the required bar.

No findings.

[OPUS-REVIEWED] 71e544e

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

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

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 71e544e

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

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/cron-session-control-8332 branch from 8c5e3c2 to a0c88b5 Compare September 4, 2026 01:23
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

ai-review-disposition

GPT: App-owned crons bypass app isolation (session_control.py:732) -- ACCEPTED and FIXED in a0c88b5.

The finding is correct and the mechanism is exactly as stated. _app is how every other isolation decision in these files recognises an app, but inject_cron_result_to_dashboard mints the cron tab with origin=SlotOrigin.CRON and NO app= argument, so an app's own scheduled job reaches this surface with _app == "". The exemption I added let it past the app_scoped_caller check beside it, and the child create_session mints is a persistent, sidebar-visible session with no app tag -- which is the confinement escape that refusal exists to prevent, reached through the app's cron instead of its session. Verified against apps/cron_sdk.py, which tags an app's jobs created_by = "app:{app_name}" (line 117).

Fixed rather than reverted, because the derived-but-ignored identity is the defect, not the exemption: _app_owned_cron_refusal reads created_by off the JOB, which is where app ownership is actually recorded, and refuses an app: owner with app_owned_cron_caller. A distinct code rather than reusing app_scoped_caller because mcp_dashboard renders that one with app-SESSION wording that would misdescribe a cron.

Two details worth flagging for the re-review:

  • Fail-closed on an unverifiable owner. A job the registry cannot produce, or a registry that raises, refuses with cron_owner_unverifiable. "Could not verify the owner" must not read as "has no owner", the same direction agent_unverifiable takes on its own unreadable input. Nothing legitimate is refused by it: a cron whose job is gone is not running.
  • Applied at BOTH caller-side sites (_refuse_ineligible_creator and authorize_target's caller half), so the two halves stay the exact mirrors _refuse_ineligible_creator's docstring requires, and scoped to cron callers so no other caller pays for the lookup (asserted by a test that makes the registry raise if consulted).

Tests: 6 new cases in test/test_cron_session_control.py covering app-owned refused on both paths, a user-owned (created_by = Slack user id) cron unaffected, an unfindable job, an unreadable registry, and the no-lookup-for-ordinary-callers scope. 244 green across the six related suites; black / isort / flake8 / mypy clean. The spec's refusal table and its "Cron callers" section carry the new rule.


Dependency Audit / Audit Production Dependencies is an infrastructure timeout, not a finding. The log reads production dependency audit failed closed: npm audit timed out after 120s for website/package-lock.json. This diff touches four files (one Python module, one spec, two test files) and no lockfile or manifest, so there is nothing here for the audit to have changed. Re-dispatched by this push rather than papered over with a .vulnerability-exceptions.json entry.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/cron-session-control-8332 branch from a0c88b5 to 552ab5b Compare September 4, 2026 02:24
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 4, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

ai-review-disposition

GPT: 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:

  1. create_session mints the child through get_or_create_slot(None, ...), so its key is a plain chat-N.
  2. The child inherits the caller's agent (agent.strip() or caller_slot.agent), which for a dispatching cron is a session-control-capable agent -- one that mounts @kirocrew-dashboard.
  3. _caller_is_ownership_fenced keyed on the prefix, so chat-N read as an ORDINARY caller.
  4. An ordinary caller with the switch on reaches any same-workspace persistent session. So: cron creates child, seeds it via session_send (permitted, it created it), child reads the user's sessions, cron reads the child's transcript via session_read_message (also permitted). The fence was bypassable in one hop.

Fixed by making the fence follow AUTHORITY rather than spelling. _caller_is_ownership_fenced now covers three populations: a member DM slot, a cron slot, and anything either of them created.

Two properties worth checking in the re-review, because they are what make this a fix rather than a patch:

  • No lineage walk, so it cannot fail open at depth or on a closed ancestor. _created_by is written at exactly ONE site (create_session, line 1042 -- verified by grep across dashboard/; the two other mentions are the rehydrate paths restoring it). A person's own tab and a fork reach get_or_create_slot directly and stay unattributed. So a non-empty _created_by means "an agent made this session" at ANY depth: a grandchild carries its parent's chat- key there and is fenced by the same test. A chain whose middle slot has been closed is not a fail-open hole, because no chain is consulted.
  • _human_seen releases a session a person is actually driving, which is the same escape hatch _ChatSlot.unattended uses. It is set only by a dashboard-user route with an empty request_app, so neither an app nor a cron can forge it. Without it, a human who opens a cron-created tab and asks it to stop a stuck session would be refused for lineage they had nothing to do with.

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 chat- key. Ordinary human-created conductors are unaffected (_created_by empty), which is pinned by a test.

Tests: 7 new cases in test/test_cron_session_control.py -- the child cannot reach a session it did not create, the child still reaches its own children, a grandchild is fenced, a human-driven agent session is released, and a human-created caller is not fenced. 249 green across the six related suites; black / isort / flake8 / mypy clean. The spec carries a new "The fence propagates to what a fenced caller creates" section.

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.

@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: checking Automated validation is still running labels Sep 4, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/cron-session-control-8332 branch from 552ab5b to f0bbdc6 Compare September 4, 2026 03:13
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

ai-review-disposition

Both findings on 552ab5b ACCEPTED and FIXED in f0bbdc6.

1. Sticky attendance disables the ownership fence (session_control.py:194)

Correct, and the chain is exactly as stated. _human_seen is monotonic and persisted, and it answers "has a human EVER driven this slot" -- which is the right question for an approval WINDOW (is someone around to click) and the wrong one for "whose authority is this turn". So: cron creates the child, the user types into it once out of curiosity, and from then on every cron-AUTHORED turn in that child runs unfenced. The creator gets its deputy back for the price of a glance at the tab.

Taken as offered: the exemption is removed, no substitute. There is no cheap correct version of it either -- knowing whether the CURRENT turn was human-authored is not something this predicate has access to, and the regression it was protecting (a person working in a cron-created tab keeps that session's reach rather than their own) is much smaller than the escalation. A test now pins the opposite of what I had written: _human_seen = True on an agent-created slot stays fenced.

My original reasoning for adding it was wrong in a specific way worth naming, since it is the kind of mistake that recurs: I reached for _ChatSlot.unattended's hatch because it looked like the same question, without checking that the two predicates are asked at different times. unattended is consulted when a tool is waiting for approval, where "a human has been here" really does predict "a human can answer". This one is consulted per turn, where it predicts nothing.

2. App-owned cron refused after target resolution (session_control.py:1328)

Also correct, and it is an existence oracle rather than merely untidy ordering: a caller allowed to touch NOTHING got target_not_found (404) for a session that does not exist and app_owned_cron_caller (403) for one that does, so it could enumerate the user's session keys and titles by the shape of the error.

Moved above _resolve_slot, where the unattended prefix gate already sits for the same reason. The refusal needs only state and caller_key, so nothing else had to move with it. Pinned by a test that asserts the SAME code for an existing and a nonexistent target -- its mutation guard is direct: put the refusal back below the resolution and the nonexistent case answers target_not_found again.

One thing I did NOT change, flagged rather than silently left: the pre-existing caller-side block below the resolution (app_scoped_caller, ephemeral_caller, linked_session_caller, mirrored_caller) has the same shape and the same oracle. Moving those alters refusal precedence for callers that exist today, which is a behaviour change this PR should not be making, so it is recorded in the spec next to the new ordering note instead. Happy to file it separately if you would rather it were tracked.

Unrelated red

CI / Backend Tests (Windows) (4) is a pre-existing timing flake, not this diff. test_snapshot_absent_vs_wrong_type::test_a_second_allocation_in_the_same_second_gets_its_own_directory needs both allocations inside one second; they landed on pre-restore-20260904T023330Z and ...023331Z, so the premise the assertion depends on did not hold. Nothing in this change touches snapshot or rollback directories. Dependency Audit remains the npm audit 120s timeout dispositioned earlier -- still no lockfile in this diff.

250 green across the six related suites; black / isort / flake8 / mypy clean. Spec updated for both changes.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/cron-session-control-8332 branch from ba3fac0 to 716ed46 Compare September 4, 2026 04:23
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

ai-review-disposition

GPT: 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. mcp_cron's cron_add (line ~1856) passes session_key=_authz_session_key() into svc.add_job and never passes created_by, so a job authored by an app-scoped session records its authority ONLY in session_key. Reading created_by alone let it through, exactly as described.

Fixed by checking both spellings, with the second delegating to _app on the owning slot rather than re-deriving app-ness, so there is one definition of "is this an app" and not a third. The owning slot is resolved through this module's own caller_slot_key rather than a removeprefix("dashboard:") -- chat_utils documents that the naive strip is wrong for every non-dashboard session key. Mutation-verified: disabling the session_key branch makes the new test fail with DID NOT RAISE. 253 green across the six related suites; black / isort / flake8 / mypy clean.

One residual is accepted rather than closed, and I would rather state it than have it found. When session_key names a session that is no longer open, its _app cannot be read and the refusal returns nothing. Refusing instead would disable dispatch for the ordinary case -- a user-created job whose authoring tab has since been closed, which is most of them -- so the fail-closed direction that is right for a missing JOB is wrong here. What bounds it is that the slot has to be gone: while an app's session is live, its jobs are refused. Recorded in the spec next to the rule.

A note on the shape of this round, addressed to whoever reviews next

This 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 chat- key; the app check keyed on _app while a cron tab never receives that tag; the origin tag keyed on the caller's prefix while a grandchild inherited the same agent; and now app ownership keyed on created_by while cron_add records it in session_key.

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 dispositions

All three current non-review reds are infrastructure, none touched by this diff (four files: one Python module, one spec, two test files):

  • Focus Cue Gate -- the gate crashed in its own temp-dir teardown: OSError: [Errno 39] Directory not empty: 'pack' inside shutil.rmtree. Run locally it reports 5 PRE-EXISTING elements and states "Not enforced here; only elements a change touches are gated". This diff touches no frontend file.
  • Dependency Audit -- npm audit timed out after 120s for website/package-lock.json, failed closed on the tool. No lockfile or manifest in this diff.
  • Backend Tests (Windows) (4) (earlier head) -- test_snapshot_absent_vs_wrong_type::test_a_second_allocation_in_the_same_second_gets_its_own_directory needs both allocations inside one second; they landed on ...023330Z and ...023331Z.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

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: session_create and session_send are both in CHANNEL_AGENT_BLOCKED_TOOLS, so a channel agent is deliberately forbidden from session control -- and a job it authors launders around that block exactly as an app's does. session_key for such a job names a channel session, which resolves to no dashboard slot when that thread has no open tab, so the check I added in the previous round returns None and admits it.

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 converging

Five consecutive blocking findings, all one class: a security decision keyed on one spelling of an identity while another spelling carries the same authority.

round keyed on authority that slipped through
1 slot-key prefix a created child under a chat- key
2 _app on the slot a cron tab never receives that tag
3 the caller's prefix a grandchild inheriting the same agent
4 created_by cron_add records it in session_key
5 session_key resolving to a dashboard slot a channel owner, or an app owner whose slot has closed

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 land

Invert 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 session_key resolves to a live session that would itself be allowed to create sessions. Reuse _refuse_ineligible_creator on the owning slot rather than re-deriving the conditions, so there is one definition and nothing left to enumerate -- a channel key, a closed slot, an app slot, an empty session_key, and any future principal spelling all fail one positive test.

That terminates the sequence by construction. Two reasons I am not landing it unilaterally:

  1. It changes the feature's contract, not just its guard. "A cron may dispatch only while its owning chat session is open and eligible" excludes jobs created by kirocrew cron add (no owning session at all) and jobs whose authoring tab has since been closed, until re-pointed with kirocrew cron adopt --session-of <live slot>. That is defensible -- arguably it is the right authority model, since the cron is borrowing a user session's reach -- but it decides who may use the feature, which is a product call.
  2. The reuse needs a small refactor to be safe. _app_owned_cron_refusal is wired into _refuse_ineligible_creator, so calling that gate on the owning slot recurses when a cron's owner is itself a cron tab. The non-cron half has to be extracted first.

What is solid in this head, for whoever picks it up

Rounds 1-3 addressed the cron-dispatch fence itself and none of the later findings has touched it: the created_by fence covering created descendants, the transitive SlotOrigin.CRON tag, the removal of the _human_seen release, and the refusal ordering that closed the existence oracle. 253 tests green across six suites, two of them mutation-verified; black / isort / flake8 / mypy clean; the spec moved with the code. The open item is exactly one axis: which principals may borrow a cron's dispatch authority.

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)

  • Dependency Audit -- npm audit timed out after 120s for website/package-lock.json. No lockfile or manifest in this diff.
  • Focus Cue Gate (earlier head) -- the gate crashed in its own temp-dir teardown: OSError: [Errno 39] Directory not empty: 'pack'.
  • Backend Tests (Windows) (4) (earlier head) -- test_a_second_allocation_in_the_same_second_gets_its_own_directory needs both allocations inside one second; they landed a second apart.

@chenmingwei23 chenmingwei23 reopened this Sep 4, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
… 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.
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

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

All evidence reviewed. The user-facing surface here is small: a disclosure-identity fix in ChatPage.tsx (each same-tick tool row now expands/collapses independently instead of in lockstep), plus backend cron session-control refusal strings that name the outcome and carry machine-readable codes. tool_call_id is persisted in tool-message meta (chat_runner.py:2222), so the fix holds on replayed history too, and disclosure state is in-memory useState, so re-keying loses nothing a user kept. New refusal copy ("a scheduled run can only control sessions it created itself") states what happened in the user's vocabulary. No new visual chrome, no moved controls, no new defaults facing a human.

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

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

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: _app_owned_cron_refusal short-circuited
on owning_slot is None and fell through to ALLOWED, so an app could author a cron through
mcp_cron's cron_add (which records the caller in session_key and never writes
created_by), close its session, and have the job dispatch a persistent non-app session that
this gate could no longer recognise. Reproduced before fixing: the new test asserted a refusal
and reported DID NOT RAISE against the prior head.

This reverses the tradeoff the earlier docstring had taken on purpose, so the docstring was
rewritten rather than left saying the opposite of what the code now does. The cost is stated
plainly there and here: a genuinely user-created dispatching cron whose authoring tab has since
been closed is refused too. That was accepted because the refusal is recoverable -- the operator
can reopen a tab -- whereas an app session minted outside its confinement cannot be undone.

It is a stopgap, not the resolution. The durable fix is to record created_by = "app:{app_name}"
on the job at cron_add time the way apps/cron_sdk.py already does, since that arm does not
depend on a live slot; the unresolvable-owner arm could then go back to allowing an ordinary
user's cron without reopening the escape. Tracked in #8449 with the known obstacle (mcp_cron
does not have the caller's app identity on hand at that point) written up.

Tests: test/test_cron_session_control.py 35 passed, test/test_session_control.py 143 passed.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@iamwhatever
iamwhatever merged commit 423181a into main Sep 4, 2026
64 checks passed
@iamwhatever
iamwhatever deleted the feat/cron-session-control-8332 branch September 4, 2026 16:09
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 4, 2026
javenciu added a commit to javenciu/KiroCrew that referenced this pull request Sep 6, 2026
… 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
bolichen97 pushed a commit that referenced this pull request Sep 7, 2026
… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cron cannot use session control, regardless of which agent the job runs as

2 participants