feat(cron): allow assigning a Schedule-page folder when creating a cron

[andreyaurelien]

Problem / Motivation

Cron folders exist in the Schedule page, but no creation path can set one: the cron_add MCP tool has no folder parameter, the CLI has no flag, and app-manifest crons cannot carry a folder at all — worse, app disable / app enable deletes and recreates the jobs, so a folder set by hand on an app cron is lost at every reload. Folder organization is not reproducible on a fresh install.

Why it matters

Anyone who organizes more than a handful of crons has to re-file them by hand in the UI after every app reload or reinstall. Same class of gap as the app-cron timezone before #6020: the field exists on the live Job (CronService.add_job already accepts folder_id), but no creation surface transports it.

What changed (motivation → approach → change)

Goal: let every creation path name a folder, without breaking cron_folders.json's single-writer model — the dashboard state holds the canonical in-memory folder list and rewrites the file wholesale, so an out-of-band append can be silently clobbered by the next Schedule-page folder operation.

Approach: a shared read-only resolver (load_cron_folders / lookup_cron_folder_id in cron.py) for every surface; folder creation only from the MCP path, through POST /api/cron-folders — the dashboard's own endpoint, so the create runs under the same lock and lands in the same in-memory list as a Schedule-page create. This is the same loopback-with-internal-secret pattern the chat-folder MCP tools already use (_post("/api/chat/folders", ...) in mcp_dashboard.py).

What was built:

• MCP cron_add / cron_update: new optional folder argument (name or id). Resolution order: exact id, then case-insensitive name (refused when several folders share it); a missing name is created via the endpoint; an id-shaped miss is refused rather than created (ids are minted server-side — same contract as the chat-folder resolver). Resolved before add_job, in the same position as the model check, so a bad reference never strands an orphaned job. On cron_update, an explicit "" moves the job out of its folder.
• CLI kirocrew cron add --folder <name|id>: resolves existing folders only and exits with a clear message pointing to the Schedule page otherwise — the CLI may run with no gateway up, and creating by writing the file directly is exactly the clobber case above.
• App manifest: CronEntry.folder (a NAME, not an id — manifests are portable across installs and ids are minted per-machine). Propagated through _cron_defs_from_manifest and CronSDK (folder_id threaded into the single locked first save, like timezone in fix(apps): let an app set a cron job timezone and skip_dates at create #6020). Resolved at registration; an unresolved folder registers the job ungrouped with a logged warning rather than failing the app's enable (the job matters more than its grouping), and since enable re-registers from the manifest, the assignment is re-applied every cycle — which is what makes it survive the disable/enable delete-and-recreate.

Alternative considered and rejected: having non-dashboard surfaces create folders by appending to cron_folders.json under a file lock — the dashboard's next wholesale save from its in-memory list would silently erase the entry until a gateway restart.

• Auth allowlist: /api/cron-folders added to _MIXED_INTERNAL_API_PATHS in dashboard/server.py. token_auth's prefix match is path == p or path.startswith(p + "/"), so the existing /api/crons entry does not cover /api/cron-folders — without this the MCP create leg falls through to cookie auth and 403s with "Token required". Same trap the neighbouring /api/artifact-folders comment records; test_mcp_call_site_auth_coverage.py is what caught it.

Tests

test/test_cron_folder_on_create.py (24 tests):

• Resolver contract: id match, case-insensitive name, ambiguity refusal, unknown-name error, malformed/missing file degrading to "no folders", and the read-only guarantee (the file is never touched by a lookup).
• MCP resolver: existing references resolve with no HTTP call, a missing name POSTs to /api/cron-folders exactly once, a create failure is surfaced (not swallowed), id-shaped misses are refused.
• Manifest: folder round-trips to_dict/from_dict, is omitted when empty, degrades non-string values to empty, and is carried by _cron_defs_from_manifest.
• Registration end-to-end against a real CronService on an isolated store: an existing folder name lands as the job's folder_id; an unknown name registers the job ungrouped.
• CLI: --folder files the job into an existing folder; an unknown folder exits non-zero without creating the job.
• Schema acceptance for cron_add / cron_update, and the MCP tool declarations expose folder.

Updated: test_app_bridges.py (two exact-call assertions gain folder_id=""), test_cron_sdk.py (mock job gains the field), test_cli.py (six TestCronCli exact-call assertions gain folder_id=""), test_mcp_call_site_auth_coverage.py (mcp_cron.py added to _SOURCES — it now imports _post, so the guard must scan its call sites; that scan is what surfaced the missing /api/cron-folders allowlist entry).

Manual verification

Not performed against a live gateway: the POST-create path is exercised with the endpoint mocked, and the registration/CLI paths run against a real CronService on an isolated store. The one inference a live pass would replace with observation — that /api/cron-folders accepts the internal-secret handshake from the MCP process — is now enforced statically instead: test_mcp_call_site_auth_coverage.py resolves that call site's path and asserts it is in an internal allowlist, which is exactly the check that caught it missing.

Related Issues

Fixes #7248

Checklist

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

Contribution License Agreement

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

FINDING -- src/kiro_crew/apps/bridges.py:1603 -- "lookup_cron_folder_id" resolves IDs before names despite the manifest’s name-only contract, making folder assignment nonportable -> Fix: resolve manifest folders by name only.
[GPT-REVIEWED] af875bd

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of af875bde09c08289a038f795d0e871701212b3d3 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

Sound single-writer-respecting design; but the app-manifest leg quietly under-delivers the PR's own "reproducible on a fresh install" motivation.

Watch

• The manifest leg resolves against existing folders only, so on a fresh install an app's folder: "Radar" never materializes — the job registers ungrouped and the only signal is a gateway log warning ("an unresolved folder registers the job ungrouped with a logged warning"). The motivating claim "Folder organization is not reproducible on a fresh install" stays unmet for exactly the app-cron case that opens the description; app authors will adopt the field and users will report "folders don't work". Disclosed, and fixable compatibly later (names stay the contract), but a human should sign off on shipping the field with this gap.

Suggestions

• The MCP resolver decides "missing" from a disk snapshot but creates through the dashboard's in-memory endpoint; POST /api/cron-folders does not dedupe by name, so two concurrent cron_add calls with the same new name mint duplicate folders and every later name lookup is refused as ambiguous. A server-side resolve-or-create (name dedupe under the existing _cron_folders_lock) closes both the split-brain and the race.
• _read_cron_folders in cron.py is a second validity predicate over a file state.py already validates (with a differing order check); consider one shared predicate so the two readers can't drift on which entries exist.

[DESIGN-REVIEWED] af875bd

[github-actions]

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

Premise-level review of af875bde09c08289a038f795d0e871701212b3d3 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 claims verified against the base. Findings are settled; here is the review.

First-Principles-Verdict: CONCERNS

Every surface earns its place except load_cron_folders, a public function with zero consumers — and the hand-set-folder loss the description opens with survives the fix.

What this change ships

Intent: let a user's cron-folder organization be set at creation time and survive reinstalls — an ADDITION.

1. cron_add MCP tool takes a folder name/id; a missing name is auto-created — justified
2. cron_update can move a job between folders ("" ungroups) — declared, beyond the "when creating" title
3. kirocrew cron add --folder files into an existing folder or refuses — justified
4. App manifests name a folder per cron, re-applied on every enable — justified
5. Unknown manifest folder registers the job ungrouped with a warning — justified default
6. /api/cron-folders added to the internal-secret allowlist — justified; prefix match on /api/crons verifiably does not cover it
7. Shared resolver lookup_cron_folder_id — justified; one spelling for all three surfaces (chat-folder resolver reads API rows, not this file, so no duplicate)
8. Public load_cron_folders — zero consumers
9. folder_id threaded through CronSDK.add_job_if_absent_async — justified plumbing
10. Id-shaped miss refused, never created — justified (ids are server-minted)

Watch

• The defect the description leads with — "a folder set by hand on an app cron is lost at every reload" (remove_jobs_by_owner at bridges.py:1720 deletes it) — is only routed around: manifest-declared folders survive, hand-set ones still die. Cause-level fix (preserve job-level fields across recreate) is larger; say it's deferred.
• Move-between-folders ships on MCP cron_update but not CLI cron update (cli.py:1369) — 1 unfixed sibling surface.

Subtractions

• Delete load_cron_folders (cron.py): grepped src/ for it — 0 non-test consumers (only state.load_cron_folders, an unrelated method). Tests can call _read_cron_folders()[0].

[FIRST-PRINCIPLES-REVIEWED] af875bd

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

Both candidates are self-labeled low confidence, and both fail the falsification bar:

Candidate 1 (non-idempotent folder auto-create) is a concurrency race — its (a) is "two concurrent creates of the same not-yet-existing name," a "could," not a condition that occurs in practice on a single-writer path. It also depends on a pre-existing endpoint property (POST /api/cron-folders permits duplicate names by design, same as the dashboard UI), not on a defect the changed lines introduce. The create leg here merely surfaces it. Drop.

Candidate 2 (prefix entry exposes PATCH/DELETE to internal-secret callers) has no grounded (c): the internal secret is held only by trusted components, and the candidate itself concedes "there is no MCP tool that lets a steered agent hit the raw DELETE/PATCH route." No untrusted call path reaches those verbs, and it mirrors the deliberate /api/artifact-folders precedent. The concern is "wider surface than strictly needed" — speculative, not an observable wrong outcome. Drop.

Step 2: the new resolver correctly separates unreadable/ambiguous/missing outcomes, the MCP create leg is the only one that creates and it routes through the dashboard endpoint under the shared lock, id-shaped misses are refused, and every non-MCP surface (CLI, app manifest) is read-only and degrades safely. Nothing new rises to the 80+ bar.

No findings.

[OPUS-REVIEWED] af875bd

[andreyaurelien]

• fixed — synchronous folder lookup blocks the gateway event loop (src/kiro_crew/apps/bridges.py)

The finding holds. register_app_crons_with_service is a coroutine awaited on the gateway loop (app enable, gateway startup), and lookup_cron_folder_id reads and JSON-parses cron_folders.json synchronously, so a large or slow-to-read file stalls every request and the heartbeat with it.

Fixed in de56712f8e8158fd20844430f747643dc194b544 by moving the call off the loop:

folder_id, folder_err = await asyncio.to_thread(
    lookup_cron_folder_id, folder_ref
)

The other two call sites were checked and neither runs on an event loop, so both stay synchronous: src/kiro_crew/mcp_cron.py is a stdio JSON-RPC server with no asyncio at all (_call_tool and _resolve_cron_folder are plain functions in their own process), and the kirocrew cron add handler in src/kiro_crew/cli_commands.py is a synchronous CLI path. bridges.py was the only site awaited on the gateway loop.

Verified: scripts/check_sync_io_in_async.py exit 0, plus 1047 targeted tests green across test_cron_folder_on_create.py, test_app_bridges.py, test_cron_sdk.py, test_cli.py, test_mcp_call_site_auth_coverage.py, test_token_auth.py and test_validation.py.

[iamwhatever]

Same issue about duplicate code was raised in two reviews, so please fix them.

• Fold the match core into one function: have _resolve_cron_folder call lookup_cron_folder_id (returning a structured miss) and keep only the id-shape refusal and create leg local.
• Drop folder_id from CronSDK.add_job and add_job_async — grep of src/kiro_crew/apps/builtins shows 0 callers, and bridges consumes only add_job_if_absent_async (bridges.py:1595); keep it on the consumed method and _common_add_kwargs.

[andreyaurelien]

@iamwhatever both done in 791c42a0f, plus a rebase onto current main (the branch was 247 commits behind).

1. One match core. lookup_cron_folder_id now returns a CronFolderLookup NamedTuple (folder_id, error, missing), and _resolve_cron_folder calls it. The duplicated body — empty ref, exact id, case-insensitive name, ambiguous-name refusal — exists once, in cron.py. Only the two legs the read-only resolver deliberately lacks stayed in mcp_cron.py, and both hang off missing:

• an id-shaped ref that matched nothing is refused (ids are minted server-side);
• a missing name is created via POST /api/cron-folders.

missing is what makes that split safe: it is the only signal separating matched nothing from ambiguous name, so the create leg cannot fire on an ambiguity. Matching on the message prose would have been the alternative, and that is the version that eventually creates a third folder sharing a name. Two tests pin the semantics directly — an unknown ref is missing=True, an ambiguous one is missing=False.

The two existing callers moved to the named fields: cli_commands.py still exits on any error, and bridges.py still degrades to ungrouped with a warning.

2. folder_id dropped from CronSDK.add_job and add_job_async. Confirmed your grep independently: no builtin app calls either method, and bridges.py:1610 registers through add_job_if_absent_async only. It stays on that method and on _common_add_kwargs. Each docstring now says why the parameter is absent, so it does not get re-added as an oversight.

Gates on the rebased head: 234 passed / 2 skipped on the cron, SDK and bridges suites, flake8 and mypy clean on all six touched files. No behaviour change is intended on any surface — the folder a given reference resolves to is identical.

[andreyaurelien]

Disposition: fixed in af875bde0 (rebased onto current main, which the branch was 7 commits behind).

• Malformed folder data is mistaken for a creatable miss — src/kiro_crew/cron.py, span=d9648bc6ddff

except Exception: return [] / if not isinstance(loaded, list): return []
Malformed folder file -> cron_add(folder=...) -> POST creation -> dashboard overwrites prior contents.
Fix: Return a non-missing error for read/shape failures; reserve missing=True for a valid list with no match.

The finding holds and the reachable path was exactly as described. load_cron_folders() collapsed three distinct states into [] — store unreadable, store absent, store valid-but-empty — so missing=True fired on a corrupt file and _resolve_cron_folder took its create leg. The flag I added in the previous round separated ambiguous from missing but left unknown on the missing side, which is the case that costs data rather than just resolving oddly.

Fixed at the read, not at the branch, so the distinction cannot be lost again by a later caller. New _read_cron_folders() returns (folders, readable), with readable=False only on a read exception or a non-list JSON root. lookup_cron_folder_id turns that into an error carrying missing=False, so the create leg is unreachable while the folder set is unknown. Three states now separate, matching your fix exactly:

store state	outcome
file absent	readable, genuinely empty — a miss stays creatable
unreadable / non-list root	refusal, missing=False, never a create
malformed entries in a valid list	filtered out, store readable — the list itself was intelligible

load_cron_folders() keeps its exact signature and list contract, so its three other callers are untouched; only the resolver reads the second element.

Four tests added, one of which closes the hole that let this through: the pre-existing test_malformed_file_degrades_to_no_folders asserted an error was returned but never that missing was False, so it passed against the defect. The new ones pin missing=False for a {} root and for unparseable bytes, and missing=True for a genuinely absent file. 48 tests green across the cron and SDK suites, 230 across dashboard-folders and bridges, with flake8/isort/mypy clean on both touched files.

[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 #4199 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 #4199: REBASE. Not duplicate work -- 7249 extends the existing cron declaration, 4199 introduces a parallel one -- but the duplicated-yet-renamed field set is a public-contract question worth resolving before the recipes vocabulary is frozen. Files: src/kiro_crew/apps/manifest.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.

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