feat(protocol): tell the worker it is rework, and repair a stale anchor - #169
Merged
Merged
Conversation
Four statements drifted from the contract as protocol/8 and /9 landed. None changes behaviour; each is a place where a doc still describes a retired design. - SKILL.md's inline authorization block named only refs/kraken/claims/, omitting the state record protocol/9 made every terminal transition write. That block is the one copy that must stand on its own — SKILL.md hands it verbatim to a subagent precisely because that subagent never reads PROTOCOL.md — so it under-authorized the writes the skill goes on to order. - DELIVERY.md still described the requeue gesture as "comment and remove awaiting-merge". protocol/8 dropped the asymmetry and /9 §3.1 makes removing a held label by hand a no-op on a task that has a record: the comment alone requeues. DELIVERY.md had not been touched since before either revision. - PROTOCOL.md §12 documented four exit codes while the reference implementation also returns 3 (nothing startable) and 13 (unknown project). A driver written to §12 — which is what §12 exists to enable — reads an `idle` envelope as an unknown failure. Name both and fence them off as ergonomics, which §12 already allows above the wire contract. - AGENTS.md summarized PROTOCOL.md without the state record, protocol/9's headline. The file's version marker was updated in #166; this sentence was not. Also drop §3's label-derived reading of `queued`: since /9 the record decides, and `kraken-task` is queue membership rather than a state — the same section goes on to forbid the reading its own table implied. Prose only. No wire-contract change, so no PROTOCOL_VERSION bump and no HISTORY.md entry: these are clarifications, which the versioning rule amends in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Lift the "Doesn't this already exist" heading and entry above "Why not just use X?", so the section lands inside that entry's disclosure: the question a first-time reader arrives with is what is visible, and expanding it reveals the side-by-side that answers it rather than leaving the two to reference each other across the page. Also fix the "bellow" typo in the entry's prose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Implements the two `genuinely-better` findings of DESIGN-REVIEW-2026-08.md (c1,
c13), plus the two prose contradictions it verified on the way (c4). The review
itself is committed here as the record of what was checked and rejected.
Both findings are the same doctrine applied to the places that lagged behind:
mechanics belong to the program, and silence never lands on the operator.
## c1 — carry what the program already knows into the envelope
`acquire_next` held the full state record at claim time and returned
`{issue, title, body}`; the resume path read the record, used it for the
verdict and dropped it too. Two things the program had computed exactly were
then left for the model to rediscover:
- SKILL.md promised "an execute envelope carries everything you need, so you
never fetch the task again", then asked for one more fetch and a judgment
call to notice a task was coming back — while `TaskState.requeued` had
already answered it with two integers.
- DELIVERY.md had the model hunt the earlier branch and PR in `brief.body` and
the thread, though `record.pr` has been a first-class sticky field since
protocol/9. The console got that field; the worker never did, and a model
that misses it opens a second, orphan PR beside the first.
The won payload gains `bounced` and `pr`, and `next-action` emits both on
`execute` from the acquire path and the resume path alike. `bounced` rides
every execute, `false` included — an absent key would read as an older program
that could not tell — and `pr` is omitted when there is none, the rule the
markers already follow. A bounce survives the claim that answered it: a first
claim writes no record (§3.1), so every resume reports the same verdict its
acquisition did rather than telling the agent halfway through that its rework
is a fresh task.
Worker-side ergonomics (§12), so no wire-contract change and no bump.
The two skill paragraphs shrink to what is left for judgment: "on `bounced`,
read the thread — the newest feedback is the ask", and "continue on the
envelope's `pr`".
Independent follow-ups from the same finding:
- `contract boundary` prints PROTOCOL.md §11 verbatim, and SKILL.md's
subagent-handover paragraph points at it instead of asking the driver to
reproduce the prose. A contract field that produces nothing is a broken
install, not an empty answer, so it exits non-zero rather than let a caller
paste a blank where the authorization rules go.
- `main()` refuses a placeholder slug (`^OWNER/`, or anything still wearing
`<>`) before any read or write, so the guard triplicated across three
SKILL.md files becomes a pointer to a program error. Keyed on the positional
`repo`, because `contract` takes one as an option and defaults it to the doc
placeholder deliberately. The conformance fixtures used the literal
placeholder as their repo slug and now use `acme/*`, which the loop-release
test already did for exactly this reason.
## c13 — an anchor the thread can no longer reach
Requeue is strictly `total > record.comments`, and the derivation is memoryless
by design. Verified sequence: a task escalates with anchor N, the operator
deletes an old noise comment (N-1; held, correctly), then answers (N; `N > N`
is false, so held — wrong). The task sits in `needs-decision` indefinitely
while the operator believes they replied, and nobody is watching that silence
except them. That is the failure class protocol/8 was written to eliminate.
§6 gains a fourth reconciler rule: an open task whose held record's anchor
exceeds the live total is rewritten with `comments` = that total. Re-anchoring
only — the hold is not lifted, because a deleted comment is not an answer — so
the *next* reply requeues instead of being absorbed. Strictly additive, which
the versioning rule amends in place: no PROTOCOL_VERSION bump, no HISTORY entry.
One deviation from the review's recommendation, and it is load-bearing. The
rule keyed directly on the walk's count would fire on a GraphQL response that
failed to return `totalCount`: `_comment_total` floors that to 0, which is below
every anchor ever written, and the reconciler would re-anchor the whole queue to
zero and hand it all back on the next good response. So the walk PROPOSES and
the applier CONFIRMS over REST (`Api.comment_count`, which answers None rather
than 0) before writing — the same surface every transition anchors from, so the
number written is comparable to the ones transitions write. Two calls, only when
the rule fires.
Three docstrings claimed the low side fails *open* when it fails *closed*:
`_comment_total`, `comment_total_of` and `TaskState.requeued` all repeated the
inverted claim, most recently in 497d681's own message. Corrected, with the
asymmetry stated: the REST reader's None fails open because it is one task, the
walk's 0 fails closed because it is the whole queue on one bad response.
## c4 — two verified prose contradictions
- README said `init` "installs the task template and its four coordination
workflows", contradicting the same README ten lines later ("no workflows") and
init's actual behaviour: it *deletes* the retired ones
(`test_init_prunes_every_retired_workflow`).
- skills/status/SKILL.md documented a `legacy:` tag no code has emitted since
protocol/9 — the example line, its explanation, and two claims that the
console paginates comment threads, contradicted later in the same file and by
`status.py`, which reads no comment at all.
## One pre-existing flake, found by running the suite
`test_a_renewal_and_a_steal_cannot_both_win` failed once under full-suite load.
Reproduced on a clean tree at HEAD: the stub held *every* ref create at the CAS
barrier, so on the branch where the thief wins, its follow-up state-record write
(§3.1) waits alone for the barrier's full 30s timeout — a dead tie with the
client's own 30s HTTP timeout, which under load wins and reports a transport
failure the protocol never had. The barrier now covers claim refs only, which is
what it exists to synchronize. The test goes from ~32s to ~3.4s and the
conformance suite from 307s to 205s.
## Verification
286 unit, 133 conformance, skill-lint OK.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#168 is the squash-merge of this branch's own two docs commits, so it carries no content this branch does not already have — the merge is an ancestry link and the resulting tree is byte-identical to the commit before it. One conflict, in DELIVERY.md's "The branch" paragraph, because both sides edited the same lines relative to the merge base and the squash left no ancestry to match them up: - main — fixed the requeue gesture ("that comment alone requeues", dropping the retired "and removing awaiting-merge"), keeping the paragraph that has the worker rediscover its earlier PR from `brief.body` and the thread. - ours — the same gesture fix, plus c1: that rediscovery is replaced by reading `pr` off the execute envelope, which is where the program now puts it. Resolved as ours: it is a strict superset — the fix main brought is already in it, and reverting to the rediscovery paragraph would undo c1 in the one file that tells a worker where to push. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Implements the two
genuinely-betterfindings ofDESIGN-REVIEW-2026-08.md(c1and c13), the two prose contradictions it verified on the way (c4), and the
doc realignment already sitting on this branch. The review itself is committed
here as the record of what was checked — including the ten claims it rejected.
Both findings are the same doctrine applied to the two places that lagged behind
it: mechanics belong to the program, and silence never lands on the operator.
c1 — carry what the program already knows into the envelope
acquire_nextheld the full state record at claim time and returned{issue, title, body}; the resume path read the record, decided the verdict fromit, and dropped it too. So two things the program had computed exactly were left
for the model to rediscover:
SKILL.mdpromised "an execute envelope carrieseverything you need, so you never fetch the task again", then asked for one
more
gh issue view --commentsand a judgment call to notice a task was comingback — while
TaskState.requeued()had already answered it with two integers.DELIVERY.mdhad the model hunt the earlier branchand PR in
brief.bodyand the thread, thoughrecord.prhas been a first-classsticky field since protocol/9. The console got that field; the worker never did,
and a model that misses it opens a second, orphan PR beside the first.
The won payload gains
bouncedandpr, andnext-actionemits both onexecute— from the acquire path and the resume path alike.bouncedrides every execute,falseincluded: an absent key would read asan older program that could not tell.
pris omitted when there is none — the rule the markers already follow. Nodelivery is not an empty delivery.
(§3.1), so every resume reports the same verdict its acquisition did, rather
than telling the agent halfway through that its rework is a fresh task.
Worker-side ergonomics (§12): no wire-contract change, no
PROTOCOL_VERSIONbump.The two skill paragraphs shrink to what is actually left for judgment — "on
bounced, read the thread; the newest feedback is the ask; escalate if nothing isactionable", and "continue on the envelope's
pr".Independent follow-ups from the same finding:
contract boundaryprints PROTOCOL.md §11 verbatim, andSKILL.md'ssubagent-handover paragraph points at it instead of asking the driver model to
reproduce the prose from memory. A contract field that produces nothing is a
broken install, not an empty answer, so it exits non-zero — a caller piping this
into a prompt must fail loudly rather than paste a blank where the authorization
rules go.
main()refuses a placeholder slug (^OWNER/, or anything still wearing<>)before any read or write, so the guard triplicated across three
SKILL.mdfilesbecomes a pointer to a program error. Keyed on the positional
repo,because
contracttakes one as an option and defaults it to the doc placeholderon purpose.
c13 — an anchor the thread can no longer reach
Requeue is strictly
total > record.comments, and the derivation is memoryless bydesign. Verified sequence:
The task then sits in
needs-decisionindefinitely while the operator believesthey replied, and nobody is watching that silence except them. That is exactly the
failure class protocol/8 was written to eliminate.
§6 gains a fourth reconciler rule: an open task whose held record's anchor
exceeds the live comment total is rewritten with
comments= that total.Re-anchoring only — the hold is not lifted, because a deleted comment is not an
answer — so the next reply requeues instead of being absorbed. Strictly
additive, which the versioning rule amends in place: no
PROTOCOL_VERSIONbump andno
HISTORY.mdentry.One deviation from the review's recommendation, and it is load-bearing. The
rule keyed directly on the walk's count would fire on a GraphQL response that
failed to return
totalCount:_comment_totalfloors that to 0, which is belowevery anchor ever written, so the reconciler would re-anchor the whole queue to
zero and hand it all back on the next good response. So the walk proposes and the
applier confirms over REST (
Api.comment_count, which answersNonerather than0) before writing — the same surface every transition anchors from, so the numberwritten is comparable to the ones transitions write. Two extra calls, and only when
the rule actually fires.
Three docstrings claimed the low side fails open when it fails closed:
_comment_total,comment_total_ofandTaskState.requeuedall repeated theinverted claim — most recently in 497d681's own commit message. Corrected, with the
asymmetry now stated outright: the REST reader's
Nonefails open because it isone task; the walk's
0fails closed because it is the whole queue on one badresponse.
c4 — two verified prose contradictions
README.mdsaidinit"installs the task template and its four coordinationworkflows" — contradicting the same README ten lines later ("no workflows") and
init's actual behaviour: it deletes the retired ones
(
test_init_prunes_every_retired_workflow).skills/status/SKILL.mddocumented alegacy:tag no code has emitted sinceprotocol/9 — the example line, its explanation, and two claims that the console
paginates comment threads, contradicted later in the same file and by
status.py, which reads no comment at all.One pre-existing flake, found by running the suite
test_a_renewal_and_a_steal_cannot_both_winfailed once under full-suite load.Reproduced on a clean tree at
HEADbefore any of this landed: the stub heldevery ref create at the CAS barrier, so on the branch where the thief wins, its
follow-up state-record write (§3.1) waits alone for the barrier's full 30s timeout
— a dead tie with the client's own 30s HTTP timeout, which under load wins and
reports a transport failure the protocol never had. The barrier now covers claim
refs only, which is what it exists to synchronize.
Side effect: the test goes from ~32s to ~3.4s, and the conformance suite from 307s
to 205s.
Also on this branch
Two earlier docs commits, unchanged by the above:
docs: realign the worker docs with protocol/9— four statements that driftedas protocol/8 and /9 landed (the inline authorization block omitting the state
record, DELIVERY.md's retired "comment and remove the label" gesture, §12's
missing exit codes 3 and 13, AGENTS.md's summary).
docs(readme): make the comparison table the FAQ answer that asks for it.Not in scope
The remaining review items, deliberately left for their own PRs: the validation
anchor refresh (
cmd_validatehonoring §3.1's SHOULD),KRAKEN_UNDER_TESTin theconformance harness (c9), GraphQL
$variables(c6 extract), and the optionalitems — the unit-test split, the rate-limit-aware watcher backoff, the
doctorpreflight, the
--operatormention.Verification
New coverage, all in
tests/COVERAGE.md:test_a_bounced_task_arrives_flagged_and_carrying_its_prpins theenvelope end to end, including that the bounce survives the claim; the
acquire_nextpayload it comes off is pinned byClaimNextIterationTests;contract boundary,protocol_sectionand the slug guard by their own unitclasses.
test_a_deletion_buries_every_reply_until_the_anchor_is_repairedpinsthe bug first (the operator's answer absorbed by
4 > 5), then thatreapmoves the anchor without lifting the hold or recovering the reply it already
absorbed, then that the next reply is seen. Plus four planner cases, four
applier cases (including the REST confirmation refusing a proposal the walk
made), and an idempotence pass.
The conformance fixtures moved off the literal
OWNER/tasksplaceholder ontoacme/*— the slug guard now refuses the former, andtest_loop_fast_release.pyalready used a real-looking slug for exactly this reason. That rename is the bulk
of the diff's line count and is purely mechanical.
🤖 Generated with Claude Code