fix(cron): boot PlatformContext in script cron child launcher (#6431) - #6433
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Boots the child at the documented seam, matching the three existing call sites, with mutation-verified tests pinning the ordering that carries the rationale. [DESIGN-REVIEWED] b34a549 |
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: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All claims verified. The three cited boot call sites exist as described (plus a fourth variant in First-Principles-Verdict: PASS Boots the cron child through the same documented seam three sibling workers already use; every item is the fix or pins it. What this change shipsIntent: make script crons work (and fail attributably) on non-standalone hosts by installing the platform context before user code runs — a FIX.
Sibling count after this change: 1 unbooted worker remains (gatewayd), documented in-repo at [FIRST-PRINCIPLES-REVIEWED] b34a549 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
421284c to
9af728a
Compare
|
Rebased onto current 1. Black gate — 2. Subprocess-encoding gate — the three While fixing that I found two further defects in the same three blocks, so they were consolidated into one
3. Frontend Lint (jscpd) — not this PR's doing. The two clones jscpd reported were in Also removed six unused imports ( Verification on
The fix itself is unchanged and still needed — |
9af728a to
2da038b
Compare
|
Accepted in full and rewritten. The central finding was correct, and my own mutation run had already reproduced it before the review landed — reverting the three preamble lines turned only 2 of 8 tests red.
The file is now 6 tests, all driving the real
Mutation-verified on Also carried forward from the previous revision: the three Local floor on |
Script cron children execute in a fresh Python interpreter via the child launcher preamble in cron_script.py. The preamble previously imported ScriptContext and exec'd the user script but never installed a PlatformContext. Under a non-standalone (enterprise/companion) profile, the child's first ctx.call_tool() reached current_context() with an enterprise profile but no installed context, raising PlatformCompositionError before the MCP server could launch. Add boot_platform(KiroCrewConfig.load()) to the child launcher preamble after sys.path manipulation and before executing user code. This matches the pattern used in cli.py and dev_fleet/server.py. Tests validate: - Launcher string ordering (boot before ScriptContext import) - Negative control reproducing PlatformCompositionError without boot - Standalone child boot installs context cleanly - Subprocess regression with synthetic entry point
2da038b to
b34a549
Compare
|
The rewritten end-to-end tests were host-dependent and failed on CI. Fixed on What broke.
Why it passed locally. This dev host does have a working backend — I probed it directly:
That is the same defect class as the hardcoded Fix. Added a Verified host-independent, not just green. Installed an autouse fixture that makes Mutation re-verified on Not mine: the Windows shard also failed |
Summary
Fixes #6431. Script cron children now initialize
PlatformContextbefore executing user code.Problem
Script crons execute in a fresh Python interpreter built by the child launcher preamble in
src/kiro_crew/cron_script.py. The preamble importedScriptContextandexec'd the user script but never installed aPlatformContext. Under a non-standalone (enterprise/companion) profile, the child's firstctx.call_tool()reachedcurrent_context()with a non-standalone profile and no installed context, and failed closed before the MCP server launched.The failure was also misattributed: because composition was decided lazily at the first platform touch inside user code, the operator read a cron failure that looked like their script's fault.
Fix
Added
boot_platform(KiroCrewConfig.load())to the child launcher preamble, after thesys.pathstrip and before theScriptContextimport. Both positions matter:ScriptContextimport — booting later would leave the cron module's own import-time platform touches unbooted.This matches the three existing call sites of the identical two-line boot (
cli.py:2352,dev_fleet/server.py:6059,file_explorer/server.py:1196) and the seamplatform/context.py:404documents: "callers that drive a process/worker withoutboot_platformshould install a context first." The one remaining unbooted worker, gatewayd, documents itself as a deliberate separate change (mcp_gateway/app_call.py:217-225).Changes
src/kiro_crew/cron_script.py(+3) — boot the platform in the child launcher preambletest/test_cron_child_boot_platform.py(new) — 6 tests, all driving the realrun_script_sandboxed:test_user_code_sees_a_booted_platform— end-to-end child assertsbootstrap._BOOTED is True. This is the discriminator:current_context()lazily composes an all-defaults standalone context on first touch, so a standalone host works either way — which is why the bug survived._BOOTEDis set only by a realboot_platform, never by the lazy default.test_boot_failure_precedes_user_code— under a non-standalone profile, composition must fail in the preamble, before the script body. The sentinel is a file rather than a stderr line because without the fix the child exits 0 and its stderr is discarded, so a stderr sentinel would be absent in both directions.test_launcher_is_profile_independent— parametrized standalone/enterprise; guards against a future "only boot when it looks necessary" narrowing.Testing
test_cron_child_boot_platform.py+test_cron_script.py+test_cron_script_more_coverage.py— no regressions.--noconftest(no hypothesis dependency).An earlier revision of the test file shipped 8 tests, of which only 2 guarded the change — three re-proved platform-library behavior already owned by
test_platform_context.py/test_cpp_wiring_standalone.py/test_security.py, and three "subprocess regression" tests spawned hand-written children that calledboot_platformthemselves without ever invoking the launcher. Both groups were removed after the First Principles review named them; see the disposition comment.