fix(queue): refuse a bad task at add time instead of at dispatch - #59
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Stop the fleet queue's intake path costing the lead a round-trip per task. Three defects were hit repeatedly by the operator in one orchestration session on 2026-09-09, all of the same shape: something
queue.sh addalready knew, that onlyqueue.sh dispatchwas left to discover.(1)
add --brief-file <path>filled whichever of the brief's four scaffolded sections the file's own##headings named and left the rest holding the scaffold's placeholder.dispatchthen refused the task saying the BRIEF.md was 'unwritten' — false, a complete brief had been supplied — and named only the path, so the lead had to go read the file to discover the complaint was about two headings it never asked for. That sequence (add, dispatch, refused, grep, patch, dispatch again) ran three times in one session. The brief offered two fixes and asked me to argue rather than assume: either --brief-file supplies the WHOLE brief and the extra headings are not added, oraddrefuses at creation naming the unfilled sections. I chose the second and the reasoning is recorded in the code: BRIEF_SECTIONS' own comment says a section with nothing to say is filled in with 'None', which is a claim the worker can rely on unlike an absent heading, so dropping headings would make 'no hard constraints' and 'the lead forgot to think about hard constraints' indistinguishable to the worker. So handing in a file is now treated as a CLAIM to have written the brief:addrefuses one that leaves any section unwritten, names which, and creates nothing on disk (the brief is rendered before os.makedirs), so the repair is one edit and one re-run of the same command.addwith NO --brief-file is deliberately untouched: that is the 'scaffold it, I will write it' path and dispatch stays its backstop — with a refusal that now names the sections too.(2)
add --branch main --base mainwas accepted and then died at spawn with thurbox's own non-zero exit, leaving the task queued for the operator to hand-edit task.yaml.addhad both values all along, so it now refuses. The brief asked me to check whether the same argument applies to other values dispatch can only fail on later, naming 'a branch that already exists with divergent commits' as the obvious neighbour. It does, and more strongly than the brief supposed: queue.py's own branch_checkout docstring records that thurbox's--worktree-branch Xonly ever CREATES X and fails with 'a branch named X already exists'. So the real precondition is that the branch does not exist at all, and branch == base is merely its guaranteed instance (a base exists by definition). I implemented the general rule: branch == base is refused from the arguments alone (always available, including --host tasks), and any other already-existing branch is refused after asking the repo. A repo this machine cannot read — every --host task's — is not asked and still finds out at dispatch, unchanged. I first implemented this as 'already checked out in a worktree' and corrected it to 'exists' after reading that docstring.(3) Found by the lead while writing this task's own brief: dispatch's precondition was a substring grep for the placeholder comment anywhere in BRIEF.md, so a brief that QUOTED the placeholder to describe it was refused even though it was completely written — the queue could not carry a task about its own scaffold, and the quotation had to be cut out of this very brief to get the task dispatched. The brief explicitly required a structural fix (compare each scaffolded section against what the scaffold wrote) rather than choosing a rarer sentinel string. Done: new brief_sections() splits a rendered brief on its own headings (fence-aware, any '## ' ends a section, only one of the four opens one, so the scaffold's trailing '## Reporting back' does not leak into 'Done means'), and unfilled_sections() reports a section whose body IS the placeholder and nothing else.
Hard constraints from the brief, both honoured: the placeholder check is NOT weakened (it exists because a worker sent a scaffold has nothing to do; the complaint was about when it fires and what it says), and the scaffold's four section headings are unchanged because other briefs in the queue already use them.
Tests were written first, in scripts/queue-selftest.sh as the brief required, and each failed on the pre-fix code for its own reason before I touched queue.py — including the one the brief specifically asked for, a BRIEF.md that legitimately quotes the placeholder and must still dispatch. They are a new section 16d beside the existing 16a-c.
Deliberate collateral in the selftest and fleet-status-selftest fixtures, all of it consequence and not scope creep: several fixtures used --branch main --base main or pre-created their branches before
addran, which the new precondition correctly refuses. I reordered those (the shepherd's branches and test 18's now appear AFTER the adds, which is the order the real thing happens in —addrecords a branch that does not exist and the worker's own spawn creates it) and gave the push-task fixtures their own working branches, since the push publish method means 'commit onto the base branch', not 'work on it'. Test 16b/16c's fixtures deliberately handed in partial brief files; I filled them out so they still prove what they were written to prove (an unrecognised heading is content kept in place; a '## ' inside a fence is not a heading), and rewrote 16c's headingless-body case to assert the refusal names exactly the three sections the body did not fill — which is the same mapping claim, now readable in the message.One neighbour I found and deliberately did NOT fix, listed here rather than done:
--repopointing at a path that is not a git repository is the same shape of defect (accepted at add, fails at spawn), but roughly forty selftest call sites intentionally use fake repo paths like /tmp/repo-a, so refusing it would be a large blast radius in a file another worker is concurrently editing. It belongs in its own task.Docs updated to match: the fleet-queue skill (SKILL.md), queue.sh's usage header, and the affected docstrings. ./scripts/check.sh — the repo's whole gate — passes, and the branch is rebased on origin/main.
What Changed
queue.sh add --brief-filenow renders the brief before touching disk and refuses to create the task if any of the four scaffolded sections is still the placeholder, naming which sections are missing instead of lettingdispatchfail later with only a file path.addnow refuses a--branchthat can't get a worktree:--branchequal to--baseis rejected from the arguments alone, and any other branch already existing in a locally-readable repo is rejected after checking with git;--hosttasks (repo not locally readable) are left to fail at dispatch as before.dispatch's brief-completeness check is replaced with a structural, fence-aware section parser (brief_sections/unfilled_sections/brief_shortfall) that compares each scaffolded section's own content instead of substring-matching the placeholder text, so a brief that legitimately quotes the placeholder while describing it now dispatches; the refusal message also names the specific missing sections per task.SKILL.md,queue.sh's usage header, and related docstrings to document the newadd-time checks; reordered and extendedqueue-selftest.shandfleet-status-selftest.shfixtures (new section 16d, adjusted 16b/16c, branch/base ordering fixes) to match the new preconditions.Risk Assessment
✅ Low: The change adds two well-scoped, side-effect-free precondition checks to
queue.sh add(brief-section completeness via a structural, fence-aware section splitter, and branch-existence/branch==base) that mirror exactly whatdispatch/spawn would otherwise fail on later; the brief is rendered before any disk writes so a refusal creates nothing, the untouched no---brief-fileand non-existent-repo paths are preserved as documented, and the accompanying selftest changes exercise the new behavior through the real CLI (not source-content greps) with fixtures correctly reordered to match the new invariant.Testing
Baseline
./scripts/check.shalready passed; I additionally ran the focusedscripts/queue-selftest.shsuite (the smallest test surface covering this change), which exercises the realqueue.sh add/dispatchCLI end-to-end for all three fixed defects and passed entirely on the target commit, while the same test file failed exactly on those new assertions when run against the pre-fix base commit — demonstrating both that the intent is satisfied and that the tests are genuine regressions rather than tautologies. No issues found; no artifacts or transient files were left in the working tree.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
./scripts/check.sh./scripts/queue-selftest.shon target commit b60871c — full pass, including new section 16 (16a–16f) covering all three defects from the intent./scripts/queue-selftest.sh(target's copy) run against base commit e861cf3's queue.py/queue.sh in a throwaway git worktree — confirmed the new section-16 assertions fail there for the expected reasons, proving these are genuine regression tests✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.