From d1f07e5d19b7158e4dd92cd69e055df9689469fa Mon Sep 17 00:00:00 2001 From: Rafael Prado Date: Sun, 2 Aug 2026 21:21:01 -0300 Subject: [PATCH 1/2] feat(protocol): carry a bounce's feedback in the execute envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bounced` told a worker its task was coming back but not what was asked: SKILL.md still sent it to `gh issue view --comments` to fetch the thread and judge by eye which comments were new. The program holds both halves of that derivation — §6 compares the live comment total against the record's frozen anchor — and threw the anchor away after computing the boolean. This is the residual of the design review's c1, which shipped the verdict and kept the index. `acquire_next` now returns the anchor it already had in hand, and next-action cuts the thread on it — paying the extra read only when `bounced` is true, so a fresh task costs nothing. The cut answers two questions, not one. WHERE the range starts is positional: the anchor is a comment TOTAL and every comment counts toward it (§6), so nothing may be filtered before the slice without sliding the index. WHAT inside that range is the ask excludes the machine's own comments — by the time this runs the claim's own comment is already on the thread, and an unfiltered tail handed every bounced task back its own "Claimed this task" line as though the operator had written it. A marker is exactly the signal that says worker-authored (§4), and filtering after the cut moves no index. The two empties stay distinct: `[]` says the read landed and found nothing past the anchor (the stale-anchor case §6 repairs), while an absent key says the read failed and the agent still owes the thread a look of its own. Collapsing them would tell a worker with a failed read that its operator asked for nothing. Co-Authored-By: Claude Opus 5 --- skills/unleash/SKILL.md | 18 +++--- skills/unleash/kraken/__init__.py | 5 +- skills/unleash/kraken/claim.py | 34 ++++++---- skills/unleash/kraken/next_action.py | 91 +++++++++++++++++++++++---- skills/unleash/kraken/render.py | 10 +++ tests/conformance/test_next_action.py | 27 ++++++++ tests/unit/test_kraken.py | 78 ++++++++++++++++++++++- 7 files changed, 229 insertions(+), 34 deletions(-) diff --git a/skills/unleash/SKILL.md b/skills/unleash/SKILL.md index e144720..2fdba90 100644 --- a/skills/unleash/SKILL.md +++ b/skills/unleash/SKILL.md @@ -75,6 +75,10 @@ An `execute` envelope carries everything you need, so you never fetch the task a - **`bounced`** — `true` when the task is coming *back* to you rather than arriving fresh. The program derives it; you never have to notice it yourself. What to do about it is the next section. +- **`feedback`** — on a bounce, the comments that arrived since the last transition, + already cut for you: **that is the ask**. An empty list means the thread moved but + left nothing past the anchor. The key is *absent* only when the read failed — the one + case where you go read the thread yourself. - **`pr`** — present only when an earlier turn on this task already delivered. It is where that work lives: **continue on that branch and update that PR** rather than opening a second one ([`DELIVERY.md`](DELIVERY.md)). @@ -120,13 +124,13 @@ that leaves the claim ref standing and the task reads as held until the lease ex ## What is yours: the judgment -**On `bounced: true`, read the thread before the Goal** (`gh -R OWNER/tasks issue view - --comments`). The task is coming back: any comment I leave puts a held task in the -queue again — from `awaiting-merge` exactly as from `needs-decision` (PROTOCOL.md §6) — -so **the newest feedback is the ask** and the Goal is only background. If it asks for -nothing actionable ("nice, thanks", "merging tomorrow"), do **not** invent rework — run -`then.escalate` saying the delivery still stands and you cannot tell what I want changed. -A question on the thread is the honest answer to a comment nobody can act on. +**On `bounced: true`, read `feedback` before the Goal.** The task is coming back: any +comment I leave puts a held task in the queue again — from `awaiting-merge` exactly as +from `needs-decision` (PROTOCOL.md §6) — so **the newest feedback is the ask** and the +Goal is only background. If it asks for nothing actionable ("nice, thanks", "merging +tomorrow"), or the list came back empty, do **not** invent rework — run `then.escalate` +saying the delivery still stands and you cannot tell what I want changed. A question on +the thread is the honest answer to a comment nobody can act on. Then, inside an `execute`: diff --git a/skills/unleash/kraken/__init__.py b/skills/unleash/kraken/__init__.py index 6596fb4..1f6191d 100644 --- a/skills/unleash/kraken/__init__.py +++ b/skills/unleash/kraken/__init__.py @@ -102,8 +102,9 @@ ) from .next_action import ( NEXT_ACTIONS, NEXT_ACTION_EXIT, NextAction, NextActionEnvelope, - cmd_next_action, issue_is_finished, lease_block, next_action, - next_action_envelope, resume_verdict, task_brief, then_commands + cmd_next_action, feedback_since, issue_is_finished, lease_block, + next_action, next_action_envelope, resume_verdict, task_brief, + then_commands ) from .watch import ( WATCH_MAX_FAILURES, WATCH_WARN_EVERY, cmd_watch, snapshot_state, diff --git a/skills/unleash/kraken/claim.py b/skills/unleash/kraken/claim.py index 46d0118..8645784 100644 --- a/skills/unleash/kraken/claim.py +++ b/skills/unleash/kraken/claim.py @@ -450,17 +450,26 @@ def acquire_next(api: Api, project: str, worker: Worker, one-task-at-a-time guard, §6 reconcile, then guard + CAS down the candidate list — as DATA rather than as printed output: returns `(exit_code, won)` where `won` is - `{"issue", "title", "body", "bounced", "pr"}` on EXIT_OK, `{"issue"}` naming - the already-held claim on EXIT_NOT_CLEAR, and None on every other outcome. - - `bounced` and `pr` come off the state record this read already carried, and - they are here because the record is READ HERE and nowhere else afterwards: - a first claim writes none (§3.1), so a caller that wanted either would have - to fetch what this function had in hand and threw away. `bounced` is §6's - requeue derivation — two integers, exact — so no reader downstream has to - re-derive "is this task coming back" from the thread, and `pr` is where the - last delivery went, so rework continues on that branch instead of opening a - second PR beside it. + `{"issue", "title", "body", "bounced", "pr", "anchor"}` on EXIT_OK, + `{"issue"}` naming the already-held claim on EXIT_NOT_CLEAR, and None on + every other outcome. + + `bounced`, `pr` and `anchor` come off the state record this read already + carried, and they are here because the record is READ HERE and nowhere else + afterwards: a first claim writes none (§3.1), so a caller that wanted any of + them would have to fetch what this function had in hand and threw away. + `bounced` is §6's requeue derivation — two integers, exact — so no reader + downstream has to re-derive "is this task coming back" from the thread, and + `pr` is where the last delivery went, so rework continues on that branch + instead of opening a second PR beside it. + + `anchor` is the OTHER half of that derivation: the comment total frozen into + the record, which is the index the new comments start at. `bounced` says the + thread moved on; `anchor` says WHERE, and a reader holding both can cut the + feedback out of the thread instead of judging by eye which comments are new. + Carried as the integer rather than as the comments themselves because this + function is the deterministic claim loop and reading a thread is not part of + it — `next-action` pays for that read, and only when `bounced` is true. `claim-next` and `next-action` are both thin renderings of this, which is what keeps the deterministic claim loop from drifting between them: there is @@ -540,7 +549,8 @@ def acquire_next(api: Api, project: str, worker: Worker, return (EXIT_OK, {"issue": cand.number, "title": cand.title, "body": cand.body, "bounced": record.requeued(cand.task.comment_total), - "pr": record.pr}) + "pr": record.pr, + "anchor": record.comments}) if rc == EXIT_TRANSPORT: # State is now ambiguous — do NOT move on to another candidate while # a write of ours may have half-landed. Re-check before any retry. diff --git a/skills/unleash/kraken/next_action.py b/skills/unleash/kraken/next_action.py index 25e6b85..fc61fe9 100644 --- a/skills/unleash/kraken/next_action.py +++ b/skills/unleash/kraken/next_action.py @@ -8,10 +8,11 @@ import os from .contract import ( - ClaimRecord, ENTRYPOINT, EXIT_LOST, EXIT_NONE, EXIT_NOT_CLEAR, EXIT_OK, - EXIT_TRANSPORT, EXIT_UNKNOWN_PROJECT, Envelope, Epoch, Gen, Issue, Json, - Repo, Worker, diag, diagnostics_on_stderr + ClaimRecord, CommentRecord, ENTRYPOINT, EXIT_LOST, EXIT_NONE, + EXIT_NOT_CLEAR, EXIT_OK, EXIT_TRANSPORT, EXIT_UNKNOWN_PROJECT, Envelope, + Epoch, Gen, Issue, Json, Repo, Worker, diag, diagnostics_on_stderr ) +from .comments import parse_marker from .transport import Api, comment_total_of from .lease import ( Lease, UNREADABLE_LEASE, clear_claim_state, format_iso, @@ -107,6 +108,38 @@ def lease_block( } +def feedback_since(api: Api, issue: Issue, anchor: int, + ) -> list[CommentRecord] | None: + """The comments that arrived AFTER the state record was written — §6's + requeue derivation as the words that caused it, not as a boolean the agent + then goes back to the thread to interpret. Returns the records in server + order, or None on transport failure. + + Two steps, and they answer different questions. WHERE the new comments start + is positional: the anchor is a comment TOTAL and `comment_records` returns + the whole thread in that same server order, so the tail past that index is + the range §6 derived its verdict from — every comment counts there (§6), so + no filter may run before the cut without sliding it. + + WHAT of that range is the ask is a different question, and the machine's own + comments are not it. A marker is exactly the structural signal that says + "worker-authored" (§4), and by the time this runs the claim's own comment is + already on the thread — so an unfiltered tail would hand every bounced task + back its own "Claimed this task" line as though the operator had written it. + Filtering AFTER the cut costs the range nothing: the index is already fixed. + + A thread that SHRANK below its anchor yields an empty list rather than a + slice from the end — which is honest, and is the c13 case: the anchor is + stale, §6's re-anchor repair is what fixes it, and a reader must not invent + feedback to fill the gap. Empty is still an answer, and the caller reports + it as one; None is the absence that means "the read did not land".""" + records = api.comment_records(issue) + if records is None: + return None + return [c for c in records[max(0, int(anchor)):] + if parse_marker(c.get("body") or "") is None] + + def task_brief(title: str, body: str) -> Json: """The task as the agent needs it: the issue-form sections split out, plus the raw body for a hand-written issue that carries no headings. An empty or @@ -152,6 +185,7 @@ def build(self, action: str, *, resumed: bool | None = None, bounced: bool | None = None, pr: str | None = None, + feedback: list[CommentRecord] | None = None, brief: Json | None = None, lease: Json | None = None, reason: str | None = None, @@ -159,14 +193,24 @@ def build(self, action: str, *, holding: Json | None = None) -> Envelope: """The one JSON shape next-action emits. - `bounced` and `pr` are what the state record knows about this task's - PAST, carried because the program already computed both and the agent - would otherwise have to rediscover them from the thread — `bounced` by - reading comments and judging whose they are, `pr` by hunting the earlier - delivery in the body. `bounced` rides every execute, false included: "no, - this is a fresh task" is an answer, and an absent key would read as an - older program that could not tell. `pr` is omitted when there is none, - the same rule the markers follow — no delivery is not an empty delivery. + `bounced`, `pr` and `feedback` are what the state record knows about this + task's PAST, carried because the program already computed them and the + agent would otherwise have to rediscover them from the thread — + `bounced` by reading comments and judging whose they are, `pr` by hunting + the earlier delivery in the body. `bounced` rides every execute, false + included: "no, this is a fresh task" is an answer, and an absent key + would read as an older program that could not tell. `pr` is omitted when + there is none, the same rule the markers follow — no delivery is not an + empty delivery. + + `feedback` is what `bounced` is ABOUT: the comments past the record's + anchor, so the ask arrives with the verdict instead of costing a fetch + and an eyeball judgment of which comments are new. It rides a bounced + execute only — a fresh task has no thread to cut — and its two absences + are distinct, which is why it is omitted rather than emitted empty on + failure: `[]` says the read landed and found nothing past the anchor (a + stale anchor, §6's repair case), while an ABSENT key says the read did + not land and the agent owes the thread a look of its own. `holding` is `{"repo", "issue"}` naming a claim this worker must resolve, and it exists because a `blocked` claim may live in a **different repo** @@ -188,6 +232,8 @@ def build(self, action: str, *, env["bounced"] = bool(bounced) if pr: env["pr"] = pr + if feedback is not None: + env["feedback"] = list(feedback) if reason: env["reason"] = reason if detail: @@ -445,13 +491,32 @@ def _resumed(self, issue: Issue, detail: Json, issue_obj: Json, # and still carries the anchor the live total ran past. Every resume of # this task therefore reports the same `bounced` its acquisition did — # which is what the agent needs, since it is rework for its whole turn. + # And because the anchor is the same, so is the cut: a resumed bounce + # carries the same feedback the acquisition did, rather than a shorter + # tail that would silently drop what the agent is meant to answer. + bounced = state.requeued(comment_total_of(issue_obj)) return self.envelope.answer( "execute", issue=issue, resumed=True, - bounced=state.requeued(comment_total_of(issue_obj)), pr=state.pr, + bounced=bounced, pr=state.pr, + feedback=self._feedback(issue, bounced, state.comments), brief=task_brief(issue_obj.get("title") or "", issue_obj.get("body") or ""), lease=lease) + def _feedback(self, issue: Issue, bounced: bool, + anchor: int) -> list[CommentRecord] | None: + """The comments this bounce is about, or None when there is nothing to + cut or the cut could not be read. + + Gated on `bounced` so the common case — a fresh task, no thread — pays + nothing: this is the one read `next-action` adds beyond what the claim + loop already did, and it is owed only when the program has just told the + agent the thread moved on. A failed read degrades to the older behaviour + (the agent goes to the thread itself), never to a wrong cut.""" + if not bounced: + return None + return feedback_since(self.api, issue, anchor) + # --- acquiring the next task --------------------------------------------- def acquire(self) -> tuple[int, Envelope]: @@ -462,6 +527,8 @@ def acquire(self) -> tuple[int, Envelope]: return self.envelope.answer( "execute", issue=won["issue"], resumed=False, bounced=won["bounced"], pr=won["pr"], + feedback=self._feedback(won["issue"], won["bounced"], + won["anchor"]), brief=task_brief(won["title"], won["body"]), lease=lease_block(self.now, self.now, self.ttl, source="estimated")) diff --git a/skills/unleash/kraken/render.py b/skills/unleash/kraken/render.py index 25ec307..20a4709 100644 --- a/skills/unleash/kraken/render.py +++ b/skills/unleash/kraken/render.py @@ -27,6 +27,16 @@ def render_next_action(env: Envelope) -> None: print(f" {env['detail']}") if env.get("pr"): print(f" pr: {env['pr']} (continue on this branch — do not open a second)") + # Only on a bounce, and as a COUNT: the bodies are the JSON's business, and + # a console line that dumped a thread would bury the verdict it sits under. + # An absent key on a bounced envelope is worth saying out loud — it is the + # one case where the agent still owes the thread a read of its own. + if env.get("bounced"): + feedback = env.get("feedback") + if feedback is None: + print(" feedback: unread — check the thread yourself") + else: + print(f" feedback: {len(feedback)} comment(s) past the anchor") brief = env.get("brief") if brief: print(f" title: {brief['title']}") diff --git a/tests/conformance/test_next_action.py b/tests/conformance/test_next_action.py index ac0aa71..dc8731b 100644 --- a/tests/conformance/test_next_action.py +++ b/tests/conformance/test_next_action.py @@ -122,6 +122,17 @@ def test_a_bounced_task_arrives_flagged_and_carrying_its_pr(self): "the earlier delivery's PR was dropped; a worker " "without it opens a second, orphan PR") + # The bounce arrives with the words that caused it. `bounced` alone still + # cost a `gh issue view --comments` and an eyeball judgment of which + # comments were new — the program holds the anchor, so the cut is a list + # slice and belongs here, not in the agent's head. + self.assertIn("feedback", env, + "a bounce must carry the comments it is about") + self.assertEqual([c["body"].strip() for c in env["feedback"]], + ["almost — the pagination is off by one, please fix"], + "the feedback must be exactly what arrived past the " + "anchor — no earlier comment, nothing missing") + # And it survives the claim: taking the task writes no record (§3.1), so # every resume of it reports the same verdict its acquisition did. A # bounce that evaporated on the first `next-action` after the claim @@ -130,6 +141,22 @@ def test_a_bounced_task_arrives_flagged_and_carrying_its_pr(self): self.assertTrue(again["resumed"], "setup: expected a resume") self.assertTrue(again["bounced"], "the bounce did not survive the claim") self.assertEqual(again["pr"], pr, "the resume dropped the delivery URL") + self.assertEqual([c["body"].strip() for c in again["feedback"]], + ["almost — the pagination is off by one, please fix"], + "the resume must cut from the SAME anchor — a shorter " + "tail would drop what the agent is meant to answer") + + def test_a_fresh_task_carries_no_feedback(self): + """No bounce, no thread to cut, no read paid for. `feedback` is owed by + a bounce and nothing else.""" + self.mk_issue(7, "a fresh task", "kraken-task", "project:app") + self.mk_body(7, "### Goal\nship it") + _r, env = self.envelope("acme/tasks", "app", "w1") + self.assertEqual(env["action"], "execute", "setup claim failed") + self.assertFalse(env["bounced"], "setup: expected a fresh claim") + self.assertNotIn("feedback", env, + "a fresh task must carry no feedback key — an empty " + "one reads as an operator who said nothing") def test_stolen_lease_is_abandoned_without_writing(self): self.mk_issue(7, "task", "kraken-task", "project:app") diff --git a/tests/unit/test_kraken.py b/tests/unit/test_kraken.py index 219b533..ecfb64f 100644 --- a/tests/unit/test_kraken.py +++ b/tests/unit/test_kraken.py @@ -822,7 +822,7 @@ def test_claims_first_startable(self): self.assertEqual(rc, kraken.EXIT_OK) self.assertEqual(self.attempted, [7]) self.assertEqual(won, {"issue": 7, "title": "oldest", "body": "body-7", - "bounced": False, "pr": None}) + "bounced": False, "pr": None, "anchor": 0}) def test_the_won_payload_carries_the_records_bounce_and_pr(self): """A task with no record is a fresh one; a task whose held record the @@ -841,6 +841,9 @@ def test_the_won_payload_carries_the_records_bounce_and_pr(self): "a comment past the record's anchor is a bounce (§6)") self.assertEqual(won["pr"], "https://github.com/o/r/pull/3", "the delivery the task is coming back from was dropped") + self.assertEqual(won["anchor"], 4, + "the anchor is where the new comments start — without " + "it a reader knows the thread moved but not from where") def test_a_task_whose_record_still_holds_is_not_bounced(self): # Same record, nothing said since: the anchor equals the live total, so @@ -2275,6 +2278,61 @@ def test_claim_in_another_repo_blocks(self): "the blocking claim is not named") +class FeedbackSinceTests(unittest.TestCase): + """The cut §6's requeue derivation implies: `bounced` says the thread moved + on, the anchor says from where, and this is the tail between them. Judgment + is the model's — deciding what the feedback ASKS FOR — but finding which + comments are new is arithmetic, and arithmetic is the program's.""" + + THREAD = [{"body": "goal notes", "createdAt": "t0"}, + {"body": "assumptions", "createdAt": "t1"}, + {"body": "delivered", "createdAt": "t2"}, + {"body": "please rename the flag", "createdAt": "t3"}] + + def _api(self, records=None): + thread = self.THREAD if records is None else records + return FakeApi("acme/tasks", comment_records=lambda i: thread) + + def test_the_cut_starts_at_the_anchor(self): + got = kraken.feedback_since(self._api(), 12, 3) + self.assertEqual([c["body"] for c in got], ["please rename the flag"], + "the ask is exactly what arrived past the anchor") + + def test_the_cut_is_positional_and_counts_from_the_total(self): + """WHERE the range starts is the anchor, which is a comment TOTAL: every + comment counts toward it (§6), so nothing may be filtered out BEFORE the + slice or the index slides by one per skipped comment.""" + got = kraken.feedback_since(self._api(), 12, 1) + self.assertEqual([c["body"] for c in got], + ["assumptions", "delivered", "please rename the flag"], + "the cut must start exactly at the anchor index") + + def test_the_machines_own_comments_are_not_the_ask(self): + """By the time this runs the claim's own comment is on the thread. A + marker is what says "worker-authored" (§4), so an unfiltered tail would + hand a bounced task its own "Claimed this task" line as the operator's + ask — and the agent would do rework against a machine's boilerplate.""" + thread = self.THREAD + [ + {"body": kraken.compose_comment("w1", "Claimed this task.", + {"type": "claim", "worker": "w1"}), + "createdAt": "t4"}] + got = kraken.feedback_since(self._api(thread), 12, 3) + self.assertEqual([c["body"] for c in got], ["please rename the flag"], + "a machine comment past the anchor read as the ask") + + def test_an_anchor_past_the_thread_yields_nothing(self): + """The c13 case: the thread SHRANK below its anchor. An empty cut is + honest — §6's re-anchor repair is what fixes the anchor, and a reader + must never invent feedback by slicing from the end.""" + self.assertEqual(kraken.feedback_since(self._api(), 12, 99), [], + "a stale anchor must not produce feedback") + + def test_a_failed_read_is_none_never_an_empty_cut(self): + api = FakeApi("acme/tasks", comment_records=lambda i: None) + self.assertIsNone(kraken.feedback_since(api, 12, 0), + "a failed read must not pose as 'nothing was said'") + + class NextActionEnvelopeTests(unittest.TestCase): """The envelope is the contract the agent reads, so its shape is pinned.""" @@ -2320,6 +2378,24 @@ def test_blocked_names_the_held_claim_and_targets_it(self): "then.%s targets the drained repo, not the held " "claim" % name) + def test_feedback_absent_and_empty_mean_different_things(self): + """The two absences are not interchangeable, so the envelope keeps them + apart: `[]` is "the read landed and the thread has nothing past the + anchor" (the stale-anchor case), while an ABSENT key is "the read did + not land" — the one case where the agent still owes the thread a look. + Collapsing them would tell a worker with a failed read that its + operator asked for nothing.""" + empty = kraken.next_action_envelope( + "execute", "acme/tasks", "env-1", issue=12, bounced=True, + feedback=[]) + self.assertEqual(empty["feedback"], [], + "an empty cut is an answer and must be emitted") + unread = kraken.next_action_envelope( + "execute", "acme/tasks", "env-1", issue=12, bounced=True, + feedback=None) + self.assertNotIn("feedback", unread, + "an unread thread must not pose as an empty one") + def test_every_action_has_an_exit_code(self): self.assertEqual(set(kraken.NEXT_ACTIONS), set(kraken.NEXT_ACTION_EXIT), "an action without an exit code is unreachable from a " From 9d94595f4da085547fafb7b5c7967a3c9d3f1d2b Mon Sep 17 00:00:00 2001 From: Rafael Prado Date: Sun, 2 Aug 2026 21:21:13 -0300 Subject: [PATCH 2/2] fix(protocol): refresh the state anchor after a validation comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PROTOCOL.md §3.1 has asked for this since protocol/9: a machine-authored comment posted without a lease SHOULD refresh the record's `comments` field, so its own comment does not read as an operator reply (§6). `cmd_validate` never did — the spec was right and the implementation lagged. The cost lands on the operator. §6 derives a requeue from `total > record.comments` and counts every comment, so a validation comment on a held task makes it read as bounced: the next worker claims rework nobody asked for, finds an automated nag on the thread, and — correctly, per the skill — escalates it back as a question. That is one more needs-decision entry to answer, generated by the program, paid for in the model's judgment. `re_anchored`, not `moved_to`: the anchor moves and nothing else does, so the hold, the worker, the expiry count and the delivery URL all stand. A refresh that lifted the hold would hand an answered task back to the queue. The record is read before the count because that is the cheap order for this caller: the validator fires on new and edited queue entries, and a task nobody has put down has no record at all — the common case exits having paid one ref read rather than the issue fetch as well. A failed read or write reports transport rather than swallowing the error: the comment has landed by then, so silence would leave precisely the stale anchor this exists to prevent. Co-Authored-By: Claude Opus 5 --- skills/unleash/kraken/workflow.py | 53 +++++++++++- tests/unit/test_workflow_commands.py | 117 +++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 1 deletion(-) diff --git a/skills/unleash/kraken/workflow.py b/skills/unleash/kraken/workflow.py index d99a0c3..c62d159 100644 --- a/skills/unleash/kraken/workflow.py +++ b/skills/unleash/kraken/workflow.py @@ -16,6 +16,7 @@ from .refs import Refs from .queue import is_empty_section, section_body from .render import render_init +from .state import States # --- subcommand: init -------------------------------------------------------- # The bootstrap `init`, single-sourced here so the skill and the program never @@ -204,7 +205,9 @@ def cmd_validate(args: argparse.Namespace) -> int: a compliant task gets none (no noise on the happy path, and the same exit once the operator fixes what was flagged). Debounced: a re-run whose missing set is unchanged posts no duplicate. Informs only — never holds, closes, or - relabels. Exit 0 on a clean run, 20 on gh/transport failure.""" + relabels; the one ref it writes is the §3.1 anchor refresh its own comment + owes (`_refresh_anchor`), which changes no state. Exit 0 on a clean run, 20 + on gh/transport failure.""" api, issue = args.api, args.issue labels = api.issue_label_names(issue) @@ -248,10 +251,58 @@ def cmd_validate(args: argparse.Namespace) -> int: if not api.post_comment(issue, body_to_post): print(f"validate: gh-failure stage=comment issue={issue}", file=sys.stderr) return EXIT_TRANSPORT + + rc = _refresh_anchor(api, issue) + if rc != EXIT_OK: + return rc print(f"validate: #{issue} flagged (missing: project/Goal/Acceptance as listed)") return EXIT_OK +def _refresh_anchor(api, issue) -> int: + """Move the state record's comment anchor past the comment this pass just + posted — the PROTOCOL.md §3.1 SHOULD that any program-authored comment on a + task's thread owes. + + Without it the validator's own comment is indistinguishable from an + operator's reply: §6 derives a requeue from `total > record.comments` and + counts every comment, so a held task the validator touches reads as bounced. + The next worker then claims rework nobody asked for, finds an automated + nag on the thread, and — correctly, per the skill — escalates it back as a + question. The noise costs the operator a `needs-decision` entry; the fix is + to stop generating it, since the count is arithmetic the program has. + + `re_anchored` is exactly the right write and no more: the anchor moves, the + hold does not lift, the state, worker, expiry count and PR all stand. Only a + task that HAS a record is touched — an absent record reads as queued (§3.1) + and holds nothing, so there is no anchor to move and writing one would + invent state this pass has no business creating. + + A failed read or write is reported as transport rather than swallowed: the + comment has landed by now, so a silent failure leaves precisely the stale + anchor this exists to prevent, and the run should say so. + + The record is read BEFORE the count, which is the cheap order for this + caller: the validator fires on new and edited queue entries, and a task that + has never been put down has no record at all — the overwhelmingly common + case exits here having paid one ref read, never the issue fetch as well.""" + states = States(api) + record = states.of(issue) + if record.unknown: + print(f"validate: gh-failure stage=record issue={issue}", file=sys.stderr) + return EXIT_TRANSPORT + if not record.recorded: + return EXIT_OK + total = api.comment_count(issue) + if total is None: + print(f"validate: gh-failure stage=anchor issue={issue}", file=sys.stderr) + return EXIT_TRANSPORT + if not states.write(issue, record.re_anchored(total)): + print(f"validate: gh-failure stage=record issue={issue}", file=sys.stderr) + return EXIT_TRANSPORT + return EXIT_OK + + def is_identity_label(name: str) -> bool: """A label cleanup MUST preserve on a closed task: the task-type label (kraken-task) and its project routing label (project:). Everything else diff --git a/tests/unit/test_workflow_commands.py b/tests/unit/test_workflow_commands.py index 75b5865..dc53ab6 100644 --- a/tests/unit/test_workflow_commands.py +++ b/tests/unit/test_workflow_commands.py @@ -480,6 +480,9 @@ def _api(self, **methods): "post_comment": lambda issue, body: ( self.posts.append((issue, body)) or True), "comment_records": lambda issue: [], + # No state ref: the default queue entry has never been put down, so + # the §3.1 anchor refresh finds no record and writes nothing. + "paginated": lambda path, **kw: [], } defaults.update(methods) return FakeApi("acme/tasks", **defaults) @@ -539,6 +542,120 @@ def test_transport_failure_on_labels_is_twenty(self): rc = kraken.cmd_validate(args) self.assertEqual(rc, kraken.EXIT_TRANSPORT) + # --- the §3.1 anchor refresh this pass's own comment owes ---------------- + + def _flagged(self, record=None, total=9, **methods): + """Run a validate pass over a task that WILL be flagged (no project + label), against a state ref that resolves to `record` through the REAL + `States.of` path — one matching-refs read, one batched commit read. + `record=None` is a task that has none. Ref writes land in `self.writes`; + returns the exit code.""" + self.writes = [] + + def request(verb, path, body=None): + self.writes.append((verb, path, body)) + # The commit create is the one write whose RESPONSE is read back + # (Refs.commit wants the sha it then points the ref at). + return (201, json.dumps({"sha": "new-sha"})) + + def paginated(path, **kw): + if "matching-refs" in path and record is not None: + return [{"ref": "refs/kraken/state/1", "object": {"sha": "s1"}}] + return [] + + defaults = { + "issue_label_names": lambda i: ["kraken-task"], + "issue_body": lambda i: self.GOOD, + "paginated": paginated, + "aliased": lambda fields: { + "c0": {"committedDate": "2026-07-01T09:00:00Z", + "message": kraken.make_marker(record.payload())}} + if record is not None else (lambda fields: {}), + "comment_count": lambda issue: total, + "request": request, + } + defaults.update(methods) + args = SimpleNamespace(repo="acme/tasks", issue="1", + api=self._api(**defaults)) + with redirect_stdout(StringIO()), redirect_stderr(StringIO()): + return kraken.cmd_validate(args) + + def _written_record(self): + """The state marker this run committed, decoded — or None if it wrote + no commit at all.""" + for _verb, path, body in self.writes: + if path.endswith("/git/commits"): + return kraken.parse_state_commit((body or {}).get("message", "")) + return None + + def test_its_own_comment_does_not_read_as_an_operator_reply(self): + """THE point of the refresh (PROTOCOL.md §3.1). §6 derives a requeue + from `total > record.comments` and counts EVERY comment, so a validation + comment left un-anchored makes a held task read as bounced: the next + worker claims rework nobody asked for, finds an automated nag, and + escalates it back as a question the operator then has to answer.""" + held = kraken.TaskState(state="awaiting-merge", worker="w0", + comments=4, recorded=True, + pr="https://github.com/o/r/pull/3") + rc = self._flagged(held, total=5) + self.assertEqual(rc, kraken.EXIT_OK) + self.assertEqual(len(self.posts), 1, "the flag itself still posts") + written = self._written_record() + self.assertIsNotNone(written, "the anchor was never refreshed") + self.assertEqual(written.comments, 5, + "the anchor must clear this pass's own comment") + self.assertFalse(written.requeued(5), + "after the refresh the task must NOT read as bounced") + + def test_the_refresh_moves_the_anchor_and_nothing_else(self): + """`re_anchored`, not `moved_to`: validate informs, so the hold, the + worker, the expiry count and the delivery URL all stand. A refresh that + lifted the hold would hand an answered task back to the queue.""" + held = kraken.TaskState(state="needs-decision", worker="w0", comments=4, + expiries=2, recorded=True, + pr="https://github.com/o/r/pull/3") + self._flagged(held, total=5) + written = self._written_record() + self.assertEqual(written.state, "needs-decision", "the hold was lifted") + self.assertEqual(written.worker, "w0") + self.assertEqual(written.expiries, 2, "the expiry count is cumulative") + self.assertEqual(written.pr, "https://github.com/o/r/pull/3") + + def test_a_task_with_no_record_is_not_given_one(self): + """The common case: a new queue entry has never been put down, so there + is no anchor to move and no record this pass may invent.""" + rc = self._flagged(None) + self.assertEqual(rc, kraken.EXIT_OK) + self.assertEqual(len(self.posts), 1, "the flag itself still posts") + self.assertIsNone(self._written_record(), + "a task with no record must not be given one") + + def test_a_compliant_task_pays_for_no_refresh(self): + """No comment, no anchor to clear. The refresh is owed by the POST, so + the happy path must not read the record at all.""" + reads = [] + rc = self._flagged( + None, + issue_label_names=lambda i: ["kraken-task", "project:app"], + paginated=lambda path, **kw: reads.append(path) or []) + self.assertEqual(rc, kraken.EXIT_OK) + self.assertEqual(self.posts, [], "a compliant task is not flagged") + self.assertEqual( + [p for p in reads if "matching-refs" in p], [], + "a compliant task must not pay for a state-ref read") + + def test_an_unreadable_record_is_transport_not_a_silent_stale_anchor(self): + held = kraken.TaskState(state="awaiting-merge", worker="w0", + comments=4, recorded=True) + rc = self._flagged(held, paginated=lambda path, **kw: None) + self.assertEqual(rc, kraken.EXIT_TRANSPORT) + + def test_an_unreadable_count_is_transport(self): + held = kraken.TaskState(state="awaiting-merge", worker="w0", + comments=4, recorded=True) + rc = self._flagged(held, comment_count=lambda issue: None) + self.assertEqual(rc, kraken.EXIT_TRANSPORT) + # --- cleanup-closed: the identity-label rule --------------------------------