fix(cron-gate): don't defer on the classifier's own task-workstream-grouping-* - #2763
fix(cron-gate): don't defer on the classifier's own task-workstream-grouping-*#2763sonichi wants to merge 2 commits into
Conversation
…rouping-* task_workstreams.py queues a classifier task only while the core is IDLE, so every cron fire that follows an idle core sees one. It declares access_tier: owner (source: task-workstream-grouping, priority: low), which the tier filter cannot distinguish from a human DM, so the gate yields to a file the core itself produced — permanently, since going idle is the emit trigger. Observed 2026-08-09: pending-questions, sync-workspace and pr-flag each deferred three fires in a row with only a classifier task queued. Same failure and same fix as #2335 for cron-runner's task-cron-*; the legacy task-project-grouping-* name is excluded too, since task_workstreams.py still recognises it. Also makes the suite footer count instead of hardcoding "13/13" — 16 checks ran against that literal, so adding a case left the summary silently behind. Stand: Echo Act IV Pro
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
qingyun-wu
left a comment
There was a problem hiding this comment.
No blocking findings.
Reviewed scripts/cron-gate.sh and the new tests/cron-gate.test.sh cases. The filename exclusions line up with the classifier task IDs emitted by src/task_workstreams.py, preserve deferral for genuine owner tasks, and keep unknown-tier tasks fail-closed.
Tests run:
SUTANDO_PY=/Users/qingyun-air/.cache/codex-runtimes/codex-primary-runtime/dependencies/python/bin/python3 bash tests/cron-gate.test.shbash -n scripts/cron-gate.sh tests/cron-gate.test.sh
GitHub CI is passing, including tsc + tests (clean install) and shellcheck.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested: the functional fix checks out, but the exact-head repository comment policy still blocks merge.
Verified at b53405e4: the base gate lacks the classifier exclusion while task_workstreams.py emits owner-tier classifier tasks; SUTANDO_PY=… bash tests/cron-gate.test.sh passes 16/16, bash -n passes, the hardcoded-path scan is clean, and hosted CI plus CLA are green. Worst-case behavior is bounded to letting internal classifier files stop preempting owner-facing crons; genuine owner and unknown-tier files still defer.
Please trim the added production/test narration to the AGENTS.md:29 limit (at most two lines, non-obvious constraint only; no incident history). The PR body already preserves the useful evidence. After that cleanup, this is ready for re-review.
Drops the incident narration from the production and test comments; each added block is now at most two lines stating the constraint. Evidence stays in the PR body. No behaviour change — 16/16 still pass and the negative control against origin/main still fails with the deferral message. Stand: Echo Act IV Pro
|
Fixed at Every comment this PR adds is now at most two lines, constraint only: Dates, the three-deferral observation and the #2335 reference are gone from the code and remain in the PR body, as you said. No behaviour change: 16/16, One thing worth flagging, because it nearly went the other way. Re-running my negative control after the trim, it passed — which would have meant the new tests had gone vacuous. It hadn't. The control itself was invalid: Controlled against So the assertion still bites. The earlier control in the PR body was valid when I ran it (the fix was uncommitted then) — but anyone re-running it after the commit gets a false pass, so use the Re-requesting review. |
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Comment-policy fix looks good. The added production/test comments are within the two-line AGENTS constraint, and the cron-gate behavior remains scoped to excluding the classifier workstream/project grouping task files from external deferral.
Tests:
SUTANDO_PY=/Users/qingyun-air/.cache/codex-runtimes/codex-primary-runtime/dependencies/python/bin/python3 bash tests/cron-gate.test.sh(16/16)bash -n scripts/cron-gate.sh tests/cron-gate.test.sh- Hosted checks: all complete checks are passing;
tsc + tests (clean install)is still pending on a rerun at review time.
Reviewed by Qingyun's Personal Codex.
AGENTS.md:29 caps code comments at two lines, constraint only, no incident history. The block was five and carried the incident. Kept the part the code cannot state — that codex's trace shares this stdout and --stall watches it, so silencing is not available — and dropped the narration. The incident stays in the commit message and the test's module docstring, which is where it is checkable. Caught by Sutando-Pro, who had a change-request on #2763 for the same rule an hour earlier.
|
Adding production evidence measured on the live core today, since this PR is waiting on a second approval and the argument so far has been mostly mechanical. The classifier emits faster than a gated cron can find a gap. Five minutes is the Observed live three times in one session, each time with the classifier as the only thing in the queue: After draining that one task by hand, the same probe returns So the failure isn't "a cron is occasionally late." With emissions at the loop cadence, a gated cron's window to observe an empty queue is whatever time passes between the queue draining and the next idle-tick emission — and on this host that has been closing faster than the crons fire. The fix stays two |
|
Follow-up evidence: a live A/B against the real emitted artifact, with the negative control that makes it mean something. My earlier comment was observational (how often the gate deferred); this is the before/after CONTRIBUTING asks for. Fixture is not synthetic — it is an actual classifier task the running core emitted minutes ago, copied unmodified into both workspaces: Same fixture, same moment, the two gate versions: And the control, which is the half that matters — a genuine owner task must still stop the gate: So the change narrows the exclusion to the classifier's own emissions rather than opening the gate generally. Without that second run, "GATE OPEN" would be equally consistent with having broken the gate. One methodology note, because I nearly posted a false proof. My first attempt ran the PR's It opened because it saw no queue at all, not because of the exclusion. A passing result from an unverified resolution path is indistinguishable from a real one. The numbers above are from both gates reading a directory that demonstrably contains the fixture. |
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Confirmed independently — this is exactly the failure mode I hit live in this session on 2026-08-10: the workspace's task_workstreams.py classifier fires a self-emitted task-workstream-grouping-<ts>.txt with access_tier: owner every ~30s while the core is idle, and it kept looking indistinguishable from a real owner DM.
Verified at HEAD (aef7c1e), isolated worktree (not the shared checkout, which has unrelated uncommitted WIP on another branch):
bash tests/cron-gate.test.sh→ 16/16 pass, including both new cases (classifier task doesn't trigger deferral; a genuine owner task alongside a classifier task still defers).- Diff is a straight extension of the existing
task-cron-*exclusion pattern inowner_task_queued()— same shape, same place, no new logic paths. - Hosted CI green (tsc+tests, shellcheck, ruff, ban-list checks).
No blocking findings.
|
@cla-assistant check |
What
cron-gate.shdefers on the classifier's own emitted task, so gated crons starve. Same failure as #2335 (cron-runner'stask-cron-*), different emitter.Why it starves rather than just delays
task_workstreams.pyqueues a classifier task only while the core is idle (classifier_status()→if not core_is_idle(workspace): return "core-busy"). So the emit trigger is the state a cron fire arrives in. The file declares:access_tier: owneris what makes it indistinguishable from a human DM — the tier filter added in #2559 correctly ignoresteam/other/ambient, and this is none of those. So the gate yields to a file the core produced itself, and will do so on every fire that follows an idle core.Observed
Three consecutive fires, live, with only a classifier task in
tasks/:Run directly (gate bypassed) they all completed immediately, and
sync-workspacehad real work:pushed to host/Chis-MacBook-Pro/462490. So the deferral was costing backups, not just delaying a no-op.The fix
One
findpredicate, mirroring the existingtask-cron-*exclusion:task-project-grouping-*is the legacy name andtask_workstreams.pystill recognises it (LEGACY_CLASSIFIER_TASK_PREFIX), so excluding only the current name would leave the same hole on any workspace still carrying one.Tests
Three cases added, mirroring the
task-cron-*block including its owner-still-defers half. The classifier fixture is the real emitted body, not a baretouch, so theaccess_tier: ownerline is actually exercised — atouched file would pass for the wrong reason.Negative control — the new tests fail without the fix:
Drive-by, and why it's in this PR rather than a separate one
The footer was
echo "OK — 13/13 cron-gate tests passed"— a hardcoded literal. 16 checks now run against it, so it under-reported by three the moment I added a case, and would have kept reporting 13/13 forever. It is now$PASSED/$PASSED, counted inok(). I'd normally split an unrelated cleanup out, but a summary that cannot see the cases this PR adds is part of this PR's own evidence.Note for CI
tests/cron-gate.test.shis listed intests/shell-ci-known-failures.txt, so it runs on ubuntu but does not gate the build. The 16/16 above is a local macOS run; treat CI's result on that suite as non-authoritative either way.