Skip to content

fix(cron): boot PlatformContext in script cron child launcher (#6431) - #6433

Merged
iamwhatever merged 1 commit into
mainfrom
fix/6431-cron-platform-context-boot
Aug 28, 2026
Merged

fix(cron): boot PlatformContext in script cron child launcher (#6431)#6433
iamwhatever merged 1 commit into
mainfrom
fix/6431-cron-platform-context-boot

Conversation

@bolichen97

@bolichen97 bolichen97 commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #6431. Script cron children now initialize PlatformContext before 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 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 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 the sys.path strip and before the ScriptContext import. Both positions matter:

  • After the strip — booting before it would let a stray sibling module in the launcher's temp dir shadow the stdlib for the boot itself, which is the failure the strip exists to prevent.
  • Before the ScriptContext import — 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 seam platform/context.py:404 documents: "callers that drive a process/worker without boot_platform should 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 preamble
  • test/test_cron_child_boot_platform.py (new) — 6 tests, all driving the real run_script_sandboxed:
    • two pinning the preamble's boot presence and its ordering against both neighbours
    • test_user_code_sees_a_booted_platform — end-to-end child asserts bootstrap._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. _BOOTED is set only by a real boot_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

  • Mutation-verified: reverting the three preamble lines turns 6 of 6 tests red; restoring makes all 6 green.
  • 216 passed / 1 skipped across test_cron_child_boot_platform.py + test_cron_script.py + test_cron_script_more_coverage.py — no regressions.
  • The module runs standalone under --noconftest (no hypothesis dependency).
  • black / subprocess-encoding / lockdown gates pass; isort 6.0.0, flake8 7.1.0, mypy 1.14.1 clean.

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 called boot_platform themselves without ever invoking the launcher. Both groups were removed after the First Principles review named them; see the disposition comment.

@bolichen97
bolichen97 requested a review from a team as a code owner August 27, 2026 23:51
@bolichen97
bolichen97 requested a review from Zedmor August 27, 2026 23:51
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 28, 2026
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

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

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] b34a549

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

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of b34a5499848c1a1f5923abaaf11962fc9da522a3 — 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. The three cited boot call sites exist as described (plus a fourth variant in slack/gateway.py:10497 passing an existing cfg), the seam doc at platform/context.py:404 says exactly what's quoted ("the lazy default is a fallback, not the intended boot path"), the gatewayd carve-out is documented in-repo at mcp_gateway/app_call.py:217-225 as a deliberate separate change, and I found no other unbooted child launcher (the sandbox/validation -I -S shims never touch platform code; dev_fleet and file_explorer servers boot at their own entry points). The production diff is three lines at the cause level of the reported defect, and the test file drives the real launcher.

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 ships

Intent: make script crons work (and fail attributably) on non-standalone hosts by installing the platform context before user code runs — a FIX.

  1. Enterprise/companion cron children now fail at launch with a composition error, not mid-script — justified (the reported defect, Script cron children do not initialize PlatformContext #6431).
  2. Cron failures are attributed to platform composition instead of the user's script — justified, declared.
  3. Every cron child now boots the platform (config load + compose) at start, all profiles — justified; platform/context.py:404 names the lazy default "a fallback, not the intended boot path", and the fix reuses the identical two-line boot already at cli.py:2352, dev_fleet/server.py:6059, file_explorer/server.py:1196 (grepped boot_platform( — 3 identical sites, 1 cfg-passing variant) rather than a second spelling.
  4. New test module (6 tests) pinning the boot's presence, ordering, and end-to-end effect — justified; all drive the real run_script_sandboxed, and the description's own pruning removed the earlier library-re-proving tests.

Sibling count after this change: 1 unbooted worker remains (gatewayd), documented in-repo at mcp_gateway/app_call.py:217-225 as a deliberate separate change — accepted-and-deferred, already recorded.

[FIRST-PRINCIPLES-REVIEWED] b34a549

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] b34a549

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

False positive or not applicable? A repository writer can comment:
/ai-review override fable b34a5499848c1a1f5923abaaf11962fc9da522a3: <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 Aug 28, 2026
@bolichen97
bolichen97 force-pushed the fix/6431-cron-platform-context-boot branch from 421284c to 9af728a Compare August 28, 2026 04:02
@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 Aug 28, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (24cb5e76c) and fixed the three blocking gate failures. Head is now 9af728ab2, still a single commit.

1. Black gatetest/test_cron_child_boot_platform.py was not black-formatted. Formatted with the CI-pinned black==26.3.1 (local 26.5.1 produces a different result, so version drift matters here).

2. Subprocess-encoding gate — the three subprocess.run(..., text=True) calls in TestSubprocessRegression had no encoding=, so they decoded the child with the Windows ANSI code page. This gate never ran in CI because the black step failed first. Fixed by routing all three through **UTF8_TEXT.

While fixing that I found two further defects in the same three blocks, so they were consolidated into one _run_child() helper:

  • Hardcoded host path. Each block pinned site_packages = "/opt/toolchains/.pyenv/versions/3.12.13/lib/python3.12/site-packages" — a path that exists only on the machine the tests were written on, and never on a CI runner. _run_child now derives PYTHONPATH from the running interpreter's sys.path, so the child resolves kiro_crew from whatever install the test executes against.
  • : as the path separator. f"{src_dir}:{site_packages}" is wrong on Windows, where Backend Tests also run. Now os.pathsep.join(...).

3. Frontend Lint (jscpd) — not this PR's doing. The two clones jscpd reported were in website/scripts/capture-chatpane-*.mjs and capture-hero-art-proxy.mjs, neither touched here; #6445 fixed the split-pane harness duplication on main at 01:28Z. The rebase picks that up.

Also removed six unused imports (tempfile, SimpleNamespace, MagicMock, patch, PlatformContext, PROFILE_ENTERPRISE) that flake8 flagged — again a step CI never reached. Rather than dropping PROFILE_ENTERPRISE/PROFILE_STANDALONE, the in-process monkeypatch.setenv("KIROCREW_PROFILE", ...) calls now use the constants instead of bare string literals.

Verification on 9af728ab2

  • black gate, subprocess-encoding gate, lockdown-before-publish: pass
  • isort 6.0.0, flake8 7.1.0, mypy 1.14.1 (1130 files): clean
  • test_cron_child_boot_platform.py + test_cron_script.py + test_cron_script_more_coverage.py: 218 passed, 1 skipped
  • mutation-verified the fix is load-bearing: reverting the three preamble lines in cron_script.py turns test_launcher_contains_bootstrap_import and test_boot_platform_after_syspath_before_scriptcontext red; restoring makes 8/8 green again

The fix itself is unchanged and still needed — main has no boot_platform in the cron child launcher preamble, and #6431 is still open.

@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 Aug 28, 2026
@bolichen97
bolichen97 force-pushed the fix/6431-cron-platform-context-boot branch from 9af728a to 2da038b Compare August 28, 2026 04:42
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Aug 28, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

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.

  • **- **Watch: all three subprocess tests write their own child that calls boot_platform directly — none executes the launcher preamble this PR changed, so all three pass with the fix reverted.** — Confirmed. Those three spawned a hand-written script; run_script_sandboxed` was never called, so the preamble was never on the path.
  • - **Subtractions: drop test_boot_is_idempotent / test_standalone_boot_installs_context / test_enterprise_profile_without_boot_raises — pinned by test_cpp_wiring_standalone.py:42, test_platform_context.py:220, test_security.py:3202.` — All three dropped. They re-proved library behavior that already has an owner.

The file is now 6 tests, all driving the real run_script_sandboxed, and the test count went from 8 to 6 while the diff shrank from +379 to +220.

  • Preamble ordering (2) — unchanged in intent, but the launcher is now captured by intercepting wrap_argv, the last seam before the spawn, so the string under assertion is the one this call actually built rather than a re-derived copy.
  • test_user_code_sees_a_booted_platform — runs a real cron script end to end and asserts bootstrap._BOOTED is True inside the child. This is the discriminator the old tests lacked: 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. _BOOTED is set only by a real boot_platform, never by the lazy default.
  • test_boot_failure_precedes_user_code — the issue's actual shape. Under a non-standalone profile it asserts composition fails in the preamble, before the script body. Two things worth naming:
    • The profile is injected into the CHILD env via _clean_cron_env, not monkeypatch.setenv. Setting KIROCREW_PROFILE=enterprise on the test process makes the PARENT's own current_context() fail closed before it ever spawns anything.
    • The sentinel is a FILE, not a stderr line. Without the fix the child exits 0 (the launcher's except Exception catches the mid-script failure) and its stderr is discarded — so a stderr sentinel would be absent in both directions and the assertion would have passed against the bug. This was a weak-assertion trap I hit while writing it.
  • test_launcher_is_profile_independent — new, parametrized standalone/enterprise. Guards against a future "only boot when it looks necessary" narrowing, which would reintroduce the bug for every profile the predicate failed to name.

Mutation-verified on 2da038b8c: reverting the three preamble lines in cron_script.py now turns 6 of 6 red (was 2 of 8); restoring makes all 6 green.

Also carried forward from the previous revision: the three subprocess.run(..., text=True) calls that decoded with the Windows ANSI code page, a hardcoded /opt/toolchains/.pyenv/... site-packages path that exists on no CI runner, and : used as the path separator on a suite that also runs on Windows — all gone with the tests that contained them.

Local floor on 2da038b8c: black / subprocess-encoding / lockdown gates pass; isort 6.0.0, flake8 7.1.0, mypy 1.14.1 (1131 files) clean; 216 passed / 1 skipped across test_cron_child_boot_platform.py + test_cron_script.py + test_cron_script_more_coverage.py; the module still runs standalone under --noconftest.

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
@bolichen97
bolichen97 force-pushed the fix/6431-cron-platform-context-boot branch from 2da038b to b34a549 Compare August 28, 2026 05:01
@bolichen97

Copy link
Copy Markdown
Collaborator Author

The rewritten end-to-end tests were host-dependent and failed on CI. Fixed on b34a54998.

What broke. Backend Tests shard 1 went red on both 3.10 and Windows — the same shard on two platforms, so a content failure, not a flake. Root cause:

❌ Cron could not run in an OS sandbox: Sandbox backend unavailable and allow_unsandboxed_exec is not set. Probe detail: unshare(CLONE_NEWNS) failed with errno 1 (EPERM) [Linux runner] / not Linux [Windows runner]

wrap_argv fails closed when the host has no OS sandbox backend, so run_script_sandboxed returned the sandbox-unavailable error and never spawned a child at all. Both end-to-end cases assert on child behavior, so they had nothing to observe.

Why it passed locally. This dev host does have a working backend — I probed it directly:

  • local wrap_argv: OK (sandbox available) — so the child really spawned here, and the tests were green for a reason that does not hold on any CI runner (Linux runners forbid unprivileged unshare, Windows and macOS 26 have no backend at all).

That is the same defect class as the hardcoded /opt/toolchains/.pyenv/... path in the first revision: a test whose outcome depends on host configuration rather than on the code under test. I had swapped one host dependency for another.

Fix. Added a _spawns_real_child fixture on the two spawning cases, mirroring the pattern test_cron_script.py already uses for exactly this reason (TestRunScriptSandboxed._passthrough_sandbox): bypass the wrap_argv wrap and put src/ on PYTHONPATH so the fresh interpreter can import kiro_crew from a non-packaged checkout. The four non-spawning cases already patched wrap_argv themselves, so they were never host-dependent.

Verified host-independent, not just green. Installed an autouse fixture that makes wrap_argv raise SandboxUnavailableError — simulating a CI runner — and re-ran: 6 passed. So the fixture, not the host's sandbox, is what makes these tests run.

Mutation re-verified on b34a54998: reverting the three preamble lines still turns 6 of 6 red; restoring makes all 6 green. Local floor clean (black / subprocess-encoding / lockdown gates, isort 6.0.0, flake8 7.1.0, mypy 1.14.1 over 1131 files, 216 passed / 1 skipped, and the module still runs under --noconftest).

Not mine: the Windows shard also failed test_acp_runtime.py::test_get_rss_tree_mb_real_process (assert 1105.55 >= 1105.69) — an RSS-comparison race that lives on main at test/test_acp_runtime.py:1277, untouched by this PR and unrelated to it.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision labels Aug 28, 2026
@github-actions github-actions Bot added the readiness: passed Eligible automated validation passed for the current revision label Aug 28, 2026
@iamwhatever
iamwhatever merged commit 2371ff1 into main Aug 28, 2026
67 checks passed
@iamwhatever
iamwhatever deleted the fix/6431-cron-platform-context-boot branch August 28, 2026 06:03
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 28, 2026
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.

Script cron children do not initialize PlatformContext

3 participants