fix-swarm: fail a task whose diff contains dark code - #97
Conversation
A fix-swarm task commissioned to close four scoped action-plan items came
back with those four fixes plus roughly 340 lines of changes nobody
requested -- dark code: an embeddings rewrite, a streaming-envelope
rework, and a new mixed-tool-call-ownership path that touched a live,
deliberately preserved disagreement.
Every existing check passed. The verify command was green, the summary was
well-formed, and every file was inside the ownership list, because the
extra work landed in files the task legitimately owned. The kit had no way
to distinguish "the fix I asked for" from "that fix plus opinions", so the
diff exported clean and the scope creep was only caught by reading all 340
lines by hand.
Ownership lists bound WHERE a worker may write. They say nothing about WHY
a given hunk exists. This adds that second question.
New optional --item-ids flag: the task declares the item IDs it was
commissioned to close, and any diff hunk citing none of them fails as
dark_code. The ID may appear anywhere in the hunk, including a context
line, so a worker's inline comment naming the item it is closing satisfies
it -- no new syntax, no annotation format to learn. The failure names each
offending file and hunk header so the operator can go straight to it.
Opt-in by construction: an empty list disables the check, so kits used by
projects with no item-ID convention behave exactly as before. The kit
manifest passes the flag and the worker spec states the convention -- a
check the worker is not told about is a trap rather than a guardrail.
Deliberately NOT in this PR, to keep it to one concern:
- a --max-diff-lines ceiling, the natural companion, is a separate change;
- templates/repo-feature has the same gap but produces no diff today, so
wiring it there means adding diff export first -- also separate.
Proof: tests/test_fix_swarm_item_ids.py covers a hunk citing a declared
ID, a dark-code hunk being reported with its file and header, the opt-in
empty case, several declared IDs, and the ID appearing in a context line.
All five fail against unfixed main (the function does not exist) and pass
with this change.
End to end against a real repo, one commissioned fix plus one unrequested
rewrite staged together:
upstream today PASS [fix_contract]: exported ... 2 file(s) exit 0
with --item-ids R17-P0.1 FAIL [dark_code]: ... embeddings.py @@ exit 1
Full suite: 258 tests, one failure -- test_deliverables
.test_runner_harvests_when_task_passes -- which fails identically on
pristine main and is fixed separately in NateBJones-Projects#96.
391f4e0 to
5b0a61a
Compare
|
Real-world evidence for the The incident: a repo-feature lane specced as strictly additive (new procedures + new tests) came back as an 8,896-line patch that rewrote 1,434 existing lines across core files the task never mentioned. Every substance grep passed — the requested features were in there, wrapped in an unrequested restructuring. Classic dark code in this PR's sense; it was caught only by a human reading the diffstat at integration time. What we run now: our shared check script enforces two budgets before exporting the worktree patch, both env-overridable per task:
On breach the check prints a corrective, not just a number — "this task is additive: restore what you removed and ADD code only; no drive-by improvements" — so the retry prompt carries the instruction the worker actually needs. Result: in a five-lane UI run this week, the same model that produced the rewrite stayed additive on every existing-file lane (largest diff +50/−13) and the budgets never fired. The one first-attempt failure mode we still saw was "no changes made", which the failure-text injection also rescued. Two takeaways for the flag design: (1) |
A fix-swarm task commissioned to close four scoped action-plan items came back with those four fixes plus roughly 340 lines of changes nobody requested, "Dark Code" — an embeddings rewrite, a streaming-envelope rework, and a new mixed-tool-call-ownership path that touched a live, deliberately preserved disagreement.
Every existing check passed. The verify command was green, the summary was well-formed, and every file was inside the ownership list — because the extra work landed in files the task legitimately owned. The kit had no way to tell "the fix I asked for" from "that fix plus opinions", so the patch exported clean and the scope creep was caught only by reading all 340 lines by hand.
Ownership lists bound where a worker may write. They say nothing about why a given hunk exists. This adds the second question.
What it does
A new optional
--item-idsflag: the task declares the item IDs it was commissioned to close, and any diff hunk citing none of them fails asdark_code.The ID may appear anywhere in the hunk, including a context line, so a worker's inline comment naming the item it is closing satisfies it — no new syntax and no annotation format to learn. The failure names each offending file and hunk header, so the operator goes straight to it instead of re-reading the diff.
Opt-in by construction: an empty list disables the check, so kits used by projects with no item-ID convention behave exactly as before. The kit manifest passes the flag and the worker spec states the convention — a check the worker is never told about is a trap, not a guardrail.
End to end, against a real repo
One commissioned fix (citing its item) and one unrequested rewrite, staged together:
Deliberately not in this PR
Keeping it to one concern:
--max-diff-lines, the natural companion ceiling, is a separate change.templates/repo-featurehas the same gap, but produces no diff today — wiring it there means adding diff export first, also separate.Proof
tests/test_fix_swarm_item_ids.pycovers a hunk citing a declared ID, a dark-code hunk being reported with its file and hunk header, the opt-in empty case, several declared IDs, and the ID appearing in a context line rather than a changed line. All five fail against unfixedmain(the function does not exist) and pass with this change.Full suite: 258 tests, one failure —
test_deliverables.test_runner_harvests_when_task_passes— which fails identically on pristinemainand is fixed separately in #96.Scope: one flag, one kit, additions only apart from two wiring lines. It inspects the worker's output, not the worker — a check, in the sense the trust model already uses.