From e12e264f1ebce0b39ab881f055b486e100f80288 Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 01:20:22 -0700 Subject: [PATCH 01/14] Add make-hera-plan-living OpenSpec change (design + deltas + tasks) Living plan-DAG: required synchronous send-status (+failed role-status), gater re-arm + recovery notice, plan-mutation verbs (update/unblock/cancel), DAG-authoritative skill. Adopt + observed-reopen + wedge-notice cut per devil's-advocate. 3 phases, each its own PR. --- .../make-hera-plan-living/.openspec.yaml | 2 + .../changes/make-hera-plan-living/README.md | 3 + .../changes/make-hera-plan-living/design.md | 299 ++++++++++++++++++ .../changes/make-hera-plan-living/proposal.md | 113 +++++++ .../specs/hera-coordination/spec.md | 132 ++++++++ .../specs/hera-messaging/spec.md | 39 +++ .../specs/hera-view/spec.md | 43 +++ .../changes/make-hera-plan-living/tasks.md | 105 ++++++ 8 files changed, 736 insertions(+) create mode 100644 openspec/changes/make-hera-plan-living/.openspec.yaml create mode 100644 openspec/changes/make-hera-plan-living/README.md create mode 100644 openspec/changes/make-hera-plan-living/design.md create mode 100644 openspec/changes/make-hera-plan-living/proposal.md create mode 100644 openspec/changes/make-hera-plan-living/specs/hera-coordination/spec.md create mode 100644 openspec/changes/make-hera-plan-living/specs/hera-messaging/spec.md create mode 100644 openspec/changes/make-hera-plan-living/specs/hera-view/spec.md create mode 100644 openspec/changes/make-hera-plan-living/tasks.md diff --git a/openspec/changes/make-hera-plan-living/.openspec.yaml b/openspec/changes/make-hera-plan-living/.openspec.yaml new file mode 100644 index 00000000..6c351aca --- /dev/null +++ b/openspec/changes/make-hera-plan-living/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-06-21 diff --git a/openspec/changes/make-hera-plan-living/README.md b/openspec/changes/make-hera-plan-living/README.md new file mode 100644 index 00000000..a497543b --- /dev/null +++ b/openspec/changes/make-hera-plan-living/README.md @@ -0,0 +1,3 @@ +# make-hera-plan-living + +Make hera role-status + the plan-DAG a living, continuously-reconciled source of truth diff --git a/openspec/changes/make-hera-plan-living/design.md b/openspec/changes/make-hera-plan-living/design.md new file mode 100644 index 00000000..b5ce081b --- /dev/null +++ b/openspec/changes/make-hera-plan-living/design.md @@ -0,0 +1,299 @@ +## Context + +The hera plan-DAG (`add-hera-plan-substrate`) gave coordinators a durable plan: +**planned nodes** (bindingless worker roles) wired by **`hera_blocks` edges**, +materialized into live workers by `internal/heragater` once every blocker reaches +hera role-status `done`. Live dogfooding shows the DAG atrophies. Three confirmed +root causes (file:line anchors from a substrate read): + +1. **Role-status is decoupled from the message bus.** `hera_send` + (`internal/mcp/hera.go:99`) carries `to`/`body`/`tldr`/`in_reply_to` only — no + status. Role status changes *only* via an explicit `hera_status` call + (`internal/mcp/hera.go:723`), which also rolls a worker's task to `in_review` + (BUG-050, `RollHeraWorkerToReview`). The gater gates on + `HeraRoleStatusFor(blockerID).Status == done` + (`internal/heragater/heragater.go:241`). So a worker that says "I'm done" in + message prose stays `working`, and the DAG never advances. + +2. **The plan tools are create-only.** `hera_plan_node` / `hera_block` / + `hera_plan` (`internal/mcp/hera_plan.go`) only ADD. There is no verb to edit a + node, drop an edge, or cancel a node. `hera_blocks` has no `RemoveHeraBlock` + (edges only vanish via FK cascade on role delete). When reality diverges from + the authored plan the coordinator cannot reconcile it, so it abandons the DAG. + +3. **The gater never re-arms.** `holdAndPing` (`heragater.go:406`) already + dedups the held notice per `(node, blocker)` via `heldPings` — but that key is + set once and **never cleared**. A blocker that recovers then re-fails is never + re-reported, and a recovery is never announced. (The brief's "re-emits every + tick" was inaccurate; the real defect is the missing re-arm.) + +Intent is also ambiguous: the skill frames `hera_plan` as authoring-time, and the +harness `TaskCreate` system-reminder competes, so coordinators split state across +two trackers. + +This change makes role-status a trustworthy signal, makes the plan mutable, and +declares the DAG authoritative — in three independently-shippable phases. + +## Goals / Non-Goals + +**Goals:** + +- A worker finishing or failing reliably advances the DAG, without anyone + remembering a second call. +- A coordinator can reconcile a plan to reality: edit a node, drop an edge, cancel + a node — keeping the DAG a live mirror, not a stale authoring snapshot. +- The gater's held-state self-heals: a recovered blocker clears the hold and tells + the coordinator; a re-failure re-pings. +- The skill states unambiguously that the plan-DAG is the source of truth for a + bound coordinator. +- Message + rail + gater share one 1:1 status vocabulary. + +**Non-Goals:** + +- **No `hera_plan_adopt`.** Folding a manually-spawned/harness worker into a + planned slot is deferred — heaviest verb, most edge cases, and the Phase C + authority statement targets the same two-tracker root cause far more cheaply. + Revisit only if dogfooding shows docs + verbs are insufficient. +- **No activity-observed auto-reopen daemon watcher.** Considered, dropped: + redundant with the now-required send-status (reopen becomes structural), and it + adds a render-flap risk plus a direct conflict with self-reported `failed`. +- **No notice for an already-running node whose blocker reopens.** That case is + physics — a spawned worker cannot be un-spawned — so the notice would be + unactionable noise. +- **No declarative whole-plan re-submit/diff.** Narrow explicit mutation verbs are + chosen over a "re-send the desired plan and we reconcile" call: a diff the + coordinator's model must construct correctly is exactly the failure mode to + avoid. +- No web/SPA surface (the plan-DAG is TUI + MCP only). + +## Decisions + +### D1 — `hera_send` carries a required status (worker→coord), applied synchronously + +`hera_send` gains a `status` parameter. It is **required** when the resolved +sender role is a `worker` or `freelance` addressing a coordinator; optional/ignored +for a coordinator sender. Its value is the role-status enum (D2). When present the +status is applied to the sender's role **synchronously inside the send handler** +(via the same `UpsertHeraRoleStatus` path `hera_status` uses, including the +BUG-050 worker→in_review roll), *before* the call returns — it never rides the +best-effort `notify.Notifier` delivery path. + +- **Why required, not optional-with-default:** a default (`working`) lets a + forgetful agent silently report `working` forever, reintroducing the exact + staleness for the `done`/`failed` cases. Erroring on omission forces the + heartbeat. (Confirmed with Aaron.) +- **Why synchronous, not event-driven:** delivery is async, idle-gated, 5-minute- + deadline, soft-fail (`internal/notify`), so it can never be trusted for status. + Applying status in the handler decouples the authoritative state change from + best-effort delivery. +- **Alternatives considered:** (a) auto-infer "done-type" from message prose — + rejected, relies on NLP heuristics over the unreliable delivery path; (b) keep + status fully separate and only nag — rejected, "remember the second call" is the + failing behavior today. + +### D2 — Add `failed` as a fifth role-status (1:1 with the rail) + +The role-status enum (`internal/db/hera.go`, `hera_role_status.status` CHECK in +`schema.go:484`) grows from `idle|working|blocked|done` to add `failed`. The rail +gets a red ✕ glyph for it (`internal/tui/widget/rolestatusicon.go`), keeping +message + rail + gater 1:1 at five states. The send-status vocabulary (D1) is +exactly this enum. + +- **Why a real status, not inference:** today the gater *infers* failure (a + blocker whose session ended without `done` → + `blockerFailed`, `heragater.go:240`). Inference only catches crashes; it misses + "I tried and give up but I'm still alive," and it is late (waits for session + death). An explicit self-reported `failed` is timely and unambiguous. +- **Gater coupling:** `blockerOutcome` returns `blockerFailed` *explicitly* when + `HeraRoleStatusFor(blocker).Status == failed`, taking precedence over the + inference path. A `failed` worker is held + pinged like any failed blocker. +- **Task roll:** a worker reporting `failed` rolls its bound task to `in_review` + (so it leaves `in_progress` and surfaces for coordinator attention) but does + **not** stamp `ready_to_close` (a failed task is not ready to check off). This + is a sibling of `RollHeraWorkerToReview`; the rail then shows the red ✕ (role- + status `failed`) rather than the review ✓ (`ready_to_close` wins only when set). +- **Alternative considered:** strict 4-state (reuse today's rail) — rejected by + Aaron in favor of explicit defeat reporting, which is core to the "living" goal. + +### D3 — Reopen is structural (a consequence of D1), not a separate mechanism + +When a coordinator hands rework to a `done`/`failed` worker (its session is left +running after a finish), the worker engages and — because every worker→coord +message now *requires* a status (D1) — reports `working` on its next message by +enforcement. That self-report flips the role `done`/`failed → working`. No daemon +watcher, no coordinator status-write, no reliance on the worker *remembering* to +re-report (the requirement is the enforcement). + +- **Accepted trade-off:** a brief staleness window — between the coordinator's + "fix X" and the worker's next message, the role reads `done`/`failed`. Impact is + bounded: the gater ticks at minute granularity, and a dependent that would + materialize during the window has almost always already materialized on the + first `done` (the wedge is physics, D4). If dogfooding shows premature + materialization in this window, the deferred activity-observed reopen is the + enhancement. +- **Why not coordinator-writes-recipient-status:** breaks the "a role owns its + status" invariant; the gater would trust a status the worker never asserted — + the stale-status drift BUG-003 warned against. + +### D4 — Gater re-arm + one-time recovery notice; the materialization wedge is left as physics + +On each tick, after classifying planned nodes: + +- **Re-arm:** for any `(node, blocker)` key currently in `heldPings`, if the + blocker's outcome is no longer `blockerFailed` (it recovered to working/done, or + the edge/role vanished), **clear the key**. A later re-failure then re-pings + (the dedup re-arms). +- **Recovery notice:** when such a key clears *because the blocker recovered*, + emit a one-time "unblocked: node X's blocker Y recovered" message to the + coordinator (same `ping` seam as `holdAndPing`, sent FROM the held node's role + so the self-send guard never trips — `orchestration.md:61`). +- **The wedge stays:** a node that already materialized has left the planned set + permanently (`ListHeraPlannedNodes` filters `NOT EXISTS (binding)`), and a + running worker cannot be un-spawned. No notice (Non-Goals). Planned (not-yet- + materialized) dependents already re-wait correctly because the gater re-reads + current blocker status every tick. + +### D5 — Narrow plan-mutation verbs (planned-node scope) + +Three coordinator-only verbs (`internal/mcp/hera_plan.go`, guarded by the existing +`heroPlanCoordinatorGuard`): + +- **`hera_plan_node_update(cwd, name, [prompt], [project], [orchestrator])`** — + edit a *planned* node's prompt/project. Rejected once the node has materialized + (`HeraRoleHasBinding` true) — the prompt is already delivered. New store fn + `UpdateHeraPlannedNode` / `SetHeraRolePrompt`. +- **`hera_unblock(cwd, blocked, blocker, [orchestrator])`** — drop one + `hera_blocks` edge. New store fn `RemoveHeraBlock(blocked, blocker)`. Re-point = + unblock + block; no separate verb. Idempotent (dropping a missing edge is a + no-op success). +- **`hera_plan_node_cancel(cwd, name, [orchestrator])`** — move a planned node to + a single **cancelled** terminal state: it is kept in the DAG (renders grey ✕), + excluded from materialization, and its dependents proceed (it no longer gates + them). Cancelling a node that already materialized is rejected (use the task + lifecycle to stop a running worker). + +**Principle — verbs operate on the *plan*, not live workers.** A materialized node +is a running agent managed by the existing task lifecycle (stop/delete). The +mutation verbs touch planned nodes + edges only; this keeps "edit the plan" +cleanly separate from "kill an agent." + +**Cancelled representation:** a `cancelled_at` timestamp column on `hera_roles` +(mirrors the existing `archived_at`/`nuked_at`/`pinned_at` pattern, +`schema.go:446`). `ListHeraPlannedNodes` adds `AND cancelled_at IS NULL` so the +gater never materializes a cancelled node. The gater's `blockerOutcome` treats a +cancelled blocker as *no longer gating* (the dependent proceeds) — equivalently, +the dependent's edge to a cancelled blocker is ignored. Rendering keeps the role +(it is not archived/deleted) so planview can draw it grey ✕. + +- **Why a column, not delete:** Aaron chose "kept, visible" over hard delete — a + reconciled plan should show what was dropped, not silently shrink. One state + (cancelled), not two (cancelled + superseded): the "why" rides in the + coordinator's message; a second terminal state earned no distinct behavior. + +### D6 — The skill declares the DAG authoritative + +`.claude/skills/hera/SKILL.md` states firmly: with a live coordinator binding the +plan-DAG is the single source of truth; author and reconcile all worker activity +through `hera_plan*` (now including update/unblock/cancel); treat the harness +`TaskCreate` reminder as not applicable to coordinated work. Plus: document the +required send-status and the new verbs, and add a "keep the DAG reconciled" +standing order. + +- The harness `TaskCreate` system-reminder is Claude Code behavior we cannot + suppress, but the skill is the lever that tells coordinators where truth lives. + Risk is that docs alone under-change behavior — which argues *for* the verbs + (D5), not against the docs. + +## Phase breakdown + +Each phase ships as its own PR (the coordinator merges; squash-only on drn/argus). + +- **Phase A — status trust:** D1 (required synchronous send-status), D2 (`failed` + status + rail glyph + gater coupling + task roll), D4 (gater re-arm + recovery + notice). D3 is a consequence of D1, not separate code. Touches `mcp/hera.go`, + `hera/service.go`, `db/hera.go`+`schema.go`, `heragater`, `widget/rolestatusicon.go`. +- **Phase B — mutable plan:** D5 (three verbs + `RemoveHeraBlock` + + `SetHeraRolePrompt` + `cancelled_at` + planview cancelled rendering). Touches + `mcp/hera_plan.go`, `db/hera_plan.go`+`schema.go`, `tui/hera`+`tui/planview`, + README Reference (new MCP verbs). +- **Phase C — authority + docs:** D6 (skill) + gotcha updates + (`orchestration.md`, `dag-rendering.md`, `hera-view.md`, `messaging.md`) + + README Reference (send-status param). Docs only; no behavioral code. + +Phase B depends on Phase A only for the shared `cancelled`/status vocabulary in +rendering; the verbs themselves are independent. Phase C depends on A + B existing. + +## Data model changes + +- `hera_role_status.status` CHECK gains `'failed'` (`schema.go:484`). +- `hera_roles` gains `cancelled_at TEXT` + `idx_hera_roles_cancelled` + (`schema.go:446`), mirroring `archived_at`. +- No new tables. `hera_blocks` unchanged (edge-remove is a DELETE on the existing + table). +- Single-user, breaking-changes-OK: the `status` CHECK change is an additive enum + widening; the new column is nullable. Per repo policy a fresh schema is fine — + no migration shim. + +## Risks / Trade-offs + +- **[Breaking `hera_send` signature]** → required status is a hard break for any + caller. Acceptable: single-user, and the skill + tool description carry the new + contract; the error message on omission names the valid values. +- **[Reopen staleness window, D3]** → bounded by gater tick granularity and the + wedge; documented as accepted. Mitigation path (deferred observed-reopen) is + known. +- **[`failed` blocker held forever if coordinator ignores it]** → same as today's + failed-blocker behavior; the re-arm + recovery notice (D4) and the cancel verb + (D5) are the escape hatches (cancel the held node, or fix/unblock the blocker). +- **[Cancelled-blocker gate semantics]** → a dependent of a cancelled node + proceeds; if the dependent genuinely needed that output the coordinator made a + judgment call. Documented; the coordinator owns the decision. +- **[Synchronous status in send handler]** → must reuse the exact + `RollHeraWorkerToReview` invariants (worker-kind only, in_progress-gated, + idempotent, soft-fail) so it never clobbers human-set state. Pinned by tests. + +## Migration Plan + +No data migration. Schema is created fresh (repo policy). Phases land as separate +PRs; each is independently revertable. `SW_VERSION` untouched (no static assets). + +## Open Questions (for Plannotator review) + +- **OQ1:** Should a `freelance`→coord message also require status, or only + `worker`→coord? (Design assumes both worker and freelance, since both can be + gated; confirm.) +- **OQ2:** `failed` task roll — roll to `in_review` without `ready_to_close` + (design choice) vs. leave the task `in_progress` so rework needs no revive. + Design picks `in_review`/no-ready-to-close for "needs attention" semantics. +- **OQ3:** Recovery notice wording/threshold — emit only on failed→recovered, or + also on any held→unheld transition? Design emits only on the failed→recovered + clear. + +## Acceptance criteria (map to scenarios in the deltas) + +**D1 — required synchronous send-status:** +- it should reject a worker→coord `hera_send` with no `status` +- it should apply the sender's status synchronously before the send returns +- it should roll a worker's task to in_review when the sent status is `done` +- it should not require `status` from a coordinator sender + +**D2 — `failed` status:** +- it should accept `failed` as a role-status value +- it should render a worker with role-status `failed` as the red ✕ rail glyph +- it should hold + ping a dependent whose blocker reports `failed` +- it should roll a `failed` worker's task to in_review without ready_to_close + +**D4 — gater re-arm + recovery:** +- it should clear the held dedup when a held node's blocker stops being failed +- it should re-ping after a blocker recovers then re-fails +- it should emit a one-time "unblocked" notice when a held node's blocker recovers + +**D5 — mutation verbs:** +- it should edit a planned node's prompt +- it should reject editing a node that has materialized +- it should drop a blocking edge (and no-op on a missing edge) +- it should cancel a planned node so it never materializes and dependents proceed +- it should keep a cancelled node visible in the plan (not delete it) +- it should reject all three verbs from a non-coordinator caller + +**D6 — authority:** (docs; no testable scenario) diff --git a/openspec/changes/make-hera-plan-living/proposal.md b/openspec/changes/make-hera-plan-living/proposal.md new file mode 100644 index 00000000..af56fcc3 --- /dev/null +++ b/openspec/changes/make-hera-plan-living/proposal.md @@ -0,0 +1,113 @@ +# Make the hera plan-DAG a living source of truth + +## Why + +The hera plan-DAG (`add-hera-plan-substrate`, planned nodes + `hera_blocks` edges ++ the gater) atrophies in real use. Three root causes, confirmed by reading the +code: + +- **Role-status is decoupled from the message bus.** `hera_send` carries no + status; role status changes only via an explicit `hera_status` call. A worker + that says "I'm done" in prose stays `working`, so the gater (which gates on + role-status `done`) never advances the DAG. This is the root cause of the + "dead DAG." +- **The plan tools are create-only.** `hera_plan` / `hera_plan_node` / + `hera_block` only ADD. There is no verb to edit a node, cancel one, drop an + edge, or fold a manually-spawned worker into the plan. When reality diverges + the coordinator cannot reconcile the DAG, so it abandons it. +- **The gater never re-arms.** The held-ping dedup (`heldPings`) is set once and + never cleared, so a recovered-then-re-failed blocker is never re-reported and a + recovery is never announced. A materialized node also leaves the planned set + permanently, so a blocker that reopens after its dependent already spawned can + never re-gate it. + +Intent is also ambiguous: the skill frames `hera_plan` as authoring-time, and the +harness `TaskCreate` reminder competes, so coordinators split state across two +trackers and the DAG dies. + +## What Changes + +Three phases, each shipping as its own PR. + +### Phase A — make role-status trustworthy + +- **Required status on every worker→coord message.** `hera_send` gains a + `status` field that is **required** when the sender is a worker (or freelance) + addressing a coordinator, drawn from the role-status enum. The status is + applied **synchronously** when the send is processed — never via the + best-effort delivery path. (**BREAKING** for the `hera_send` tool signature.) +- **New `failed` role-status.** Add `failed` as a fifth role-status value with a + red ✕ rail glyph, so message + rail + gater stay 1:1. A worker self-declares + defeat instead of the gater inferring it from a dead session; the gater treats + a `failed` blocker as a held dependency explicitly. +- **Reopen is structural, not observed.** Because status is required on every + worker→coord message, a re-engaged worker reports `working` again on its next + message by enforcement — no separate daemon watcher, no reliance on the worker + remembering. (An activity-observed auto-reopen was considered and dropped as + redundant with the required status; see design.md.) +- **Gater re-arm + recovery notice.** Clear the held-ping dedup when a blocker's + outcome stops being `failed`; emit a one-time "unblocked" notice when a + previously-held node's blocker recovers. (A notice for an already-running + node whose blocker reopens was considered and dropped — that case is physics, + not actionable.) + +### Phase B — make the plan mutable + +- `hera_plan_node_update(name, [prompt], [project])` — edit a planned node; + rejected once it has materialized. +- `hera_unblock(blocked, blocker)` — drop one blocking edge (re-point = unblock + + block). +- `hera_plan_node_cancel(name)` — move a planned node to a single **cancelled** + terminal state (grey ✕): kept visible in the DAG, excluded from + materialization, dependents proceed. +- Supporting store + render work: `RemoveHeraBlock`, a node prompt-edit, a + cancelled marker, and planview rendering of cancelled nodes. + +_Deferred:_ `hera_plan_adopt` (fold an already-running worker into a planned-node +slot) was scoped out — it is the heaviest verb and targets the same two-tracker +root cause as the Phase C authority statement. Revisit only if dogfooding shows +the docs + mutation verbs are insufficient. + +### Phase C — declare the DAG authoritative + +- Update the in-repo hera skill: with a live coordinator binding the plan-DAG is + the single source of truth; reconcile through `hera_plan*` / adopt rather than + the harness task list; document the new verbs and the required send-status. +- Update the gotcha files (`orchestration.md`, `dag-rendering.md`, + `hera-view.md`, `messaging.md`). + +## Capabilities + +### New Capabilities + +_None._ The plan-DAG substrate lives in existing capabilities; this change +extends them. + +### Modified Capabilities + +- `hera-messaging`: `hera_send` gains the required worker→coord status field and + its synchronous-apply semantics. +- `hera-coordination`: the role-status enum gains `failed` (with explicit failure + gating); the structural reopen, the gater re-arm + recovery notice, the three + plan-mutation verbs (`hera_plan_node_update`, `hera_unblock`, + `hera_plan_node_cancel`), the cancelled-node store semantics, and the authority + statement are added. (The supporting store ops — `RemoveHeraBlock`, the node + prompt-edit, the `cancelled_at` marker — are specified within these + requirements rather than as a separate `data-persistence` delta.) +- `hera-view`: the rail gains a `failed` glyph and planview gains the cancelled + node rendering. + +## Impact + +- **Code:** `internal/mcp/hera.go` + `internal/mcp/hera_plan.go` (tool defs + + handlers), `internal/db/hera.go` + `internal/db/hera_plan.go` + `schema.go` + (status enum, cancelled marker, edge-remove, prompt-edit), `internal/heragater` + (re-arm + recovery notice), `internal/hera/service.go` (synchronous + status-on-send), `internal/tui/hera` + + `internal/tui/widget/rolestatusicon.go` (failed glyph, cancelled rendering). +- **Specs:** deltas for `hera-messaging`, `hera-coordination`, `hera-view`, + `mcp-server`, `data-persistence`. +- **Docs:** `.claude/skills/hera/SKILL.md`, the four gotcha files, README + Reference appendix (new MCP verbs + the `hera_send` status param), help modal + only if a TUI key changes (none planned). +- **No web/API surface change** beyond what already exists; TUI + MCP + daemon. diff --git a/openspec/changes/make-hera-plan-living/specs/hera-coordination/spec.md b/openspec/changes/make-hera-plan-living/specs/hera-coordination/spec.md new file mode 100644 index 00000000..436f1d9b --- /dev/null +++ b/openspec/changes/make-hera-plan-living/specs/hera-coordination/spec.md @@ -0,0 +1,132 @@ +## MODIFIED Requirements + +### Requirement: hera_status updates role status and rolls a finished worker + +The system SHALL, on `hera_status`, validate the status as one of idle/working/blocked/done/failed, upsert it on the caller's role, and mirror it to the `task_meta` "hera" namespace best-effort. When a WORKER role reports `done` the system SHALL roll its bound task to in_review and stamp `ready_to_close` via `RollHeraWorkerToReview` — the primary BUG-050 trigger for the idle-but-done case the exit hook misses. When a WORKER role reports `failed` the system SHALL roll its bound task to in_review WITHOUT stamping `ready_to_close` (a failed task is not ready to close — it needs coordinator attention), via the same in_progress-gated, worker-kind-only, idempotent, soft-fail helper family. Both rolls are worker-kind only, no-op unless the task is currently in_progress (so they never clobber a human-set in_review/complete and never auto-complete), leave the live session running, are idempotent, and are soft-fail (a failure never blocks the status update). + +Derived from: `internal/mcp/hera.go:643` (`toolHeraStatus`), `internal/mcp/hera.go:691` (BUG-050 worker roll), `internal/tui/hera/ops.go:193` (the same roll mirrored on the rail `s` key). + +#### Scenario: Invalid status is rejected + +- **WHEN** hera_status is called with a status other than idle/working/blocked/done/failed +- **THEN** the tool errors naming the valid values + +#### Scenario: Worker done rolls to in_review + +- **WHEN** a worker role reports status=done and its task is in_progress +- **THEN** the task is rolled to in_review and stamped ready_to_close, while the session keeps running + +#### Scenario: Worker failed rolls to in_review without ready_to_close + +- **WHEN** a worker role reports status=failed and its task is in_progress +- **THEN** the task is rolled to in_review but ready_to_close is NOT stamped, while the session keeps running + +#### Scenario: Done roll never clobbers a non-progress task + +- **WHEN** a worker reports done but its task is already in_review or complete +- **THEN** RollHeraWorkerToReview no-ops and the status update still succeeds + +## ADDED Requirements + +### Requirement: failed role-status and explicit failure gating + +The system SHALL support `failed` as a fifth hera role-status value alongside idle/working/blocked/done, persisted in the same `hera_role_status` store and accepted everywhere a role-status is accepted (`hera_status`, the `hera_send` status argument, and the rail step keys). The gater SHALL treat a blocker whose role-status is explicitly `failed` as a failed dependency directly — taking precedence over the session-death inference path — so a worker that self-declares defeat holds its dependents and pings the coordinator without waiting for the blocker's session to end. + +#### Scenario: failed is an accepted role-status + +- **WHEN** a role is set to status=failed via hera_status or the hera_send status argument +- **THEN** the value is persisted and read back as failed + +#### Scenario: A failed blocker holds its dependents + +- **WHEN** a planned node's blocker has role-status failed +- **THEN** the gater classifies the dependent as held and pings the coordinator, without waiting for the blocker's session to end + +### Requirement: Reopen via the required send-status + +The system SHALL flip a `done` or `failed` worker role back to `working` when the worker reports `working` again — which the required worker→coordinator send-status makes structural: a re-engaged worker (its session left running after a finish) reports `working` on its next message by enforcement, with no separate auto-reopen mechanism. A role's status is owned by that role; coordinators reopen a worker by sending it work, not by writing the worker's status. + +#### Scenario: A re-engaged worker reopens by self-report + +- **WHEN** a worker that previously reported done sends a later message with status=working +- **THEN** its role status flips from done back to working + +#### Scenario: Planned dependents re-wait when a blocker reopens + +- **WHEN** a blocker that was done returns to working before its dependent has materialized +- **THEN** the gater reads the current status and keeps the dependent planned (it does not materialize off the stale done) + +### Requirement: Gater held-state re-arm and recovery notice + +The system SHALL re-arm the gater's held-ping dedup so a held node is re-reported after its blocker recovers and re-fails, and SHALL announce a recovery. On each tick, for any `(node, blocker)` pair previously pinged as held, the system SHALL clear the dedup entry when the blocker's outcome is no longer failed (it recovered, or the edge/role vanished). When the entry clears specifically because the blocker recovered, the system SHALL emit exactly one "unblocked" notice to the coordinator (sent from the held node's own role so the self-send guard never trips). The system SHALL NOT emit any notice for an already-materialized node whose blocker later reopens (that node cannot be un-spawned). + +#### Scenario: Recovery clears the dedup and notifies once + +- **WHEN** a node was pinged as held behind a failed blocker and that blocker later recovers +- **THEN** the dedup entry is cleared and exactly one "unblocked" notice is sent to the coordinator + +#### Scenario: Re-failure after recovery re-pings + +- **WHEN** a blocker recovers (clearing the held dedup) and then fails again +- **THEN** the gater pings the coordinator again for the held node + +#### Scenario: No notice for an already-running node's reopened blocker + +- **WHEN** a node has already materialized and one of its blockers later reopens +- **THEN** no notice is emitted (the running worker cannot be un-spawned) + +### Requirement: Coordinator plan-mutation verbs + +The system SHALL provide three coordinator-only MCP verbs to reconcile a plan to reality, each guarded so a non-coordinator caller is rejected: + +- `hera_plan_node_update(cwd, name, [prompt], [project], [orchestrator])` — edit a PLANNED node's prompt and/or project. It SHALL be rejected when the named node has already materialized (holds a binding), since the prompt is already delivered. +- `hera_unblock(cwd, blocked, blocker, [orchestrator])` — remove one blocking edge between two roles in the orchestrator. Removing a non-existent edge is an idempotent no-op success. Re-pointing an edge is unblock + block; there is no separate re-point verb. +- `hera_plan_node_cancel(cwd, name, [orchestrator])` — move a PLANNED node to the cancelled terminal state (see the cancelled-node requirement). Cancelling a node that has already materialized SHALL be rejected (a running worker is stopped via the task lifecycle, not the plan). + +#### Scenario: Update edits a planned node's prompt + +- **WHEN** a coordinator calls hera_plan_node_update with a new prompt for a planned node +- **THEN** the node's stored prompt is updated and will be delivered when it materializes + +#### Scenario: Update is rejected for a materialized node + +- **WHEN** a coordinator calls hera_plan_node_update for a node that already holds a binding +- **THEN** the call is rejected because the prompt is already delivered + +#### Scenario: Unblock drops an edge and is idempotent + +- **WHEN** a coordinator calls hera_unblock for an existing edge, then again for the same pair +- **THEN** the first removes the edge and the second succeeds as a no-op + +#### Scenario: Mutation verbs reject non-coordinators + +- **WHEN** a worker or freelance role calls any of hera_plan_node_update, hera_unblock, or hera_plan_node_cancel +- **THEN** the call is rejected as coordinator-only + +### Requirement: Cancelled planned nodes are kept, gated out, and unblock dependents + +The system SHALL represent a cancelled planned node with a `cancelled_at` marker on the role rather than deleting it, so the node remains in the plan for visibility. A cancelled node SHALL be excluded from gater materialization (it never spawns), and SHALL no longer gate its dependents: a dependent blocked only by cancelled (and otherwise-satisfied) blockers SHALL be free to materialize. The role and its edges are NOT removed, so the plan view can still render the cancelled node distinctly. + +#### Scenario: A cancelled node never materializes + +- **WHEN** a planned node is cancelled +- **THEN** the gater excludes it from materialization on every subsequent tick + +#### Scenario: Dependents of a cancelled node proceed + +- **WHEN** a planned node's only unsatisfied blocker is cancelled +- **THEN** the gater no longer treats it as blocking and the dependent becomes eligible to materialize + +#### Scenario: A cancelled node is kept in the plan + +- **WHEN** a node is cancelled +- **THEN** its role and edges remain in the store (not deleted) so the plan view can render it + +### Requirement: The plan-DAG is the authoritative coordination tracker + +The system's coordinator guidance SHALL declare that, while a coordinator holds a live binding, the hera plan-DAG is the single source of truth for its multi-agent work: coordinators author and reconcile worker activity through the `hera_plan*` verbs (including update/unblock/cancel) and SHALL treat the harness task-creation reminder as not applicable to coordinated work. This guidance lives in the in-repo hera skill. + +#### Scenario: The skill declares the DAG authoritative + +- **WHEN** a coordinator with a live binding consults the hera skill +- **THEN** the skill instructs it to reconcile all worker activity through the plan-DAG verbs rather than the harness task list diff --git a/openspec/changes/make-hera-plan-living/specs/hera-messaging/spec.md b/openspec/changes/make-hera-plan-living/specs/hera-messaging/spec.md new file mode 100644 index 00000000..eb5ad037 --- /dev/null +++ b/openspec/changes/make-hera-plan-living/specs/hera-messaging/spec.md @@ -0,0 +1,39 @@ +## MODIFIED Requirements + +### Requirement: hera_send recipient resolution and defaults + +The system SHALL, on `hera_send`, require a non-empty body and tldr. It SHALL resolve the recipient from an explicit `to` role name within the sender's orchestrator, or default it: a worker/freelance sender with no `to` defaults to the orchestrator's active coordinator; a coordinator sender MUST supply an explicit `to`. An unknown `to` role SHALL fail naming the orchestrator. On success it returns the new message id, the recipient name, and the delivery mode. + +The system SHALL accept an optional `status` argument on `hera_send` whose value is a hera role-status (`idle`/`working`/`blocked`/`done`/`failed`). When the resolved SENDER role is a `worker` or `freelance`, `status` is REQUIRED: a worker/freelance `hera_send` with no `status` SHALL fail naming the valid values. A `coordinator` sender's `status` is optional and, if omitted, leaves the coordinator's status unchanged. When `status` is supplied the system SHALL apply it to the SENDER's role SYNCHRONOUSLY within the send handler — using the same upsert-and-roll path as `hera_status` (including the worker `done` → in_review / `failed` → in_review roll) — BEFORE the call returns, so the status change never depends on the best-effort doorbell delivery path. A status-apply failure is soft-fail and SHALL NOT block the message send. + +Derived from: `internal/mcp/hera.go:462` (`toolHeraSend`), `internal/mcp/hera.go:491` (recipient resolution + coordinator-must-supply-to). + +#### Scenario: Worker defaults to the coordinator + +- **WHEN** a worker calls hera_send with no `to` +- **THEN** the message is addressed to the orchestrator's active coordinator + +#### Scenario: Coordinator must address explicitly + +- **WHEN** a coordinator calls hera_send with no `to` +- **THEN** the call fails requiring an explicit recipient + +#### Scenario: Worker send requires a status + +- **WHEN** a worker or freelance role calls hera_send with no `status` +- **THEN** the call fails naming the valid status values and no message is sent + +#### Scenario: Sent status is applied synchronously + +- **WHEN** a worker calls hera_send with status=working +- **THEN** the sender's role status is set to working before the call returns, independent of whether the doorbell is delivered + +#### Scenario: Sent done status rolls the worker task + +- **WHEN** a worker calls hera_send with status=done and its task is in_progress +- **THEN** the task is rolled to in_review (and stamped ready_to_close) synchronously, exactly as hera_status(done) would + +#### Scenario: Coordinator send does not require status + +- **WHEN** a coordinator calls hera_send with an explicit `to` and no `status` +- **THEN** the message is sent and the coordinator's status is left unchanged diff --git a/openspec/changes/make-hera-plan-living/specs/hera-view/spec.md b/openspec/changes/make-hera-plan-living/specs/hera-view/spec.md new file mode 100644 index 00000000..a7e4d9ee --- /dev/null +++ b/openspec/changes/make-hera-plan-living/specs/hera-view/spec.md @@ -0,0 +1,43 @@ +## MODIFIED Requirements + +### Requirement: Status-icon precedence on role rows (area 3) + +The system SHALL choose a role row's status glyph by this precedence: (1) `ready_to_close` wins over everything with a distinct review glyph; otherwise (2) the hera role status when present — working, blocked, done, failed, or idle each map to a distinct glyph/style, with `failed` rendering a red ✕; otherwise (3) binding presence (`Live`) renders a "live" glyph; otherwise (4) an unbound/dimmed glyph. `ready_to_close` is read from the task-addressed `task_meta` "hera" namespace, not the hera tables. The status vocabulary (idle/working/blocked/done/failed) is shared 1:1 with the role-status enum so the rail, the plan view, and the gater never drift. + +Derived from: `internal/tui/hera/rail.go:514` (`statusIcon`), `internal/tui/hera/model.go:214` (`buildRoleView` reads `ready_to_close`). + +#### Scenario: ready_to_close overrides status + +- **WHEN** a role's bound task carries `meta:hera.ready_to_close=true` AND the role status is working +- **THEN** the row renders the review/ready glyph, not the working glyph + +#### Scenario: Hera role status drives the glyph + +- **WHEN** a role has a status row of `blocked` and is not ready_to_close +- **THEN** the row renders the needs-input/blocked glyph + +#### Scenario: failed renders the red cross glyph + +- **WHEN** a role has a status row of `failed` and is not ready_to_close +- **THEN** the row renders the red ✕ failed glyph distinct from the done glyph + +#### Scenario: Live-but-statusless role + +- **WHEN** a role holds a live binding but has no status row and is not ready_to_close +- **THEN** the row renders the in-review "live" glyph rather than the unbound glyph + +## ADDED Requirements + +### Requirement: Cancelled planned nodes render distinctly in the plan view + +The plan view SHALL render a cancelled planned node (a role carrying the `cancelled_at` marker) as a distinct grey ✕ node that remains visible in the DAG, rather than dropping it. The node keeps its position and edges so the coordinator can see what was reconciled out of the plan; it is visually distinguished from a live, planned (violet `○`), done (green `✓`), or failed (red `✕`) node. + +#### Scenario: A cancelled node stays visible and distinct + +- **WHEN** the plan view renders an orchestrator containing a cancelled planned node +- **THEN** the node is drawn as a distinct grey cancelled glyph in its DAG position, not omitted + +#### Scenario: Cancelled is distinct from failed + +- **WHEN** the plan view renders a cancelled node and a failed node +- **THEN** the two use distinct glyphs/styles (grey cancelled vs red failed) diff --git a/openspec/changes/make-hera-plan-living/tasks.md b/openspec/changes/make-hera-plan-living/tasks.md new file mode 100644 index 00000000..81429501 --- /dev/null +++ b/openspec/changes/make-hera-plan-living/tasks.md @@ -0,0 +1,105 @@ +# Tasks — make-hera-plan-living + +**Design doc:** `openspec/changes/make-hera-plan-living/design.md` + +Three phases, each shipping as its own PR (the coordinator merges; squash-only on +drn/argus). TDD throughout: write failing tests from the deltas first, then +implement. Verify each phase with targeted `go test` per package during dev and +the exact CI linter (`$(go env GOPATH)/bin/golangci-lint run +--new-from-rev=origin/master ./...` → 0 issues). Report each phase green to the +coordinator with branch + sha; do not push or open PRs directly. + +## 1. Phase A — Tests (status trust) + +Write failing tests from the `hera-messaging` and `hera-coordination` deltas. + +- [ ] 1.1 `internal/db`: tests that `hera_role_status` accepts and round-trips `failed` (schema CHECK widened) — `UpsertHeraRoleStatus`/`HeraRoleStatusFor`. +- [ ] 1.2 `internal/mcp` (hera_send): failing tests — worker→coord send with no `status` errors; `status` applied synchronously to the sender role before return; `status=done` rolls the worker task to in_review + ready_to_close; `status=failed` rolls to in_review WITHOUT ready_to_close; coordinator send needs no status. +- [ ] 1.3 `internal/mcp` (hera_status): failing tests — `failed` is accepted; invalid status names all five values; worker `failed` roll (in_review, no ready_to_close). +- [ ] 1.4 `internal/heragater`: failing tests — a blocker with role-status `failed` holds the dependent + pings (explicit, before session death); a planned dependent re-waits when a done blocker returns to working; held dedup clears on blocker recovery and re-pings on re-failure; exactly one "unblocked" notice on recovery; no notice for an already-materialized node whose blocker reopens. +- [ ] 1.5 `internal/tui/hera` + `internal/tui/widget`: failing test that a role with status `failed` renders the red ✕ glyph at the right precedence (below ready_to_close, distinct from done). +- [ ] 1.6 Confirm every Phase-A `it should X` acceptance criterion in design.md has a failing test (Prove-It). + +## 2. Phase A — failed role-status (schema + store + rail glyph) + +**Depends on:** Stage 1 + +- [ ] 2.1 `internal/db/schema.go`: widen the `hera_role_status.status` CHECK to include `'failed'`; add the `HeraStatusFailed` constant in `internal/db/hera.go`. +- [ ] 2.2 `internal/tui/widget/rolestatusicon.go`: add a `Failed` input + a red ✕ glyph case, placed below `ReadyToClose`/`NeedsInput` and distinct from `Done`; wire `roleStatusInputs` in `internal/tui/hera/rail.go` to set it from `role.Status == HeraStatusFailed`. +- [ ] 2.3 uxlog: log failed-status transitions where other status transitions are logged. + +## 3. Phase A — required synchronous send-status + +**Depends on:** Stage 2 + +- [ ] 3.1 `internal/mcp/hera.go`: add the `status` parameter to the `hera_send` tool def + handler; require it for worker/freelance senders (error naming the five values); ignore-or-accept for coordinators. +- [ ] 3.2 Apply the sent status synchronously in the handler via the shared upsert-and-roll path (reuse the `hera_status` logic / `RollHeraWorkerToReview` family) BEFORE returning; soft-fail so a roll error never blocks the send. +- [ ] 3.3 Add the `failed` worker roll sibling: roll to in_review WITHOUT `ready_to_close` (in_progress-gated, worker-kind-only, idempotent, soft-fail). Factor with `RollHeraWorkerToReview` so the invariants can't drift. +- [ ] 3.4 `internal/mcp/hera.go` (hera_status): accept `failed`; route the worker `failed` roll through the same sibling. +- [ ] 3.5 uxlog on the send-status apply (success + soft-fail). + +## 4. Phase A — gater failure gating, re-arm, recovery notice + +**Depends on:** Stage 2 + +- [ ] 4.1 `internal/heragater/heragater.go` `blockerOutcome`: return `blockerFailed` directly when `HeraRoleStatusFor(blocker).Status == failed`, BEFORE the session-death inference path. +- [ ] 4.2 Re-arm: each tick, for every `(node, blocker)` key in `heldPings`, clear it when the blocker's outcome is no longer `blockerFailed` (recovered, or edge/role gone). +- [ ] 4.3 Recovery notice: when a key clears because the blocker RECOVERED, emit exactly one "unblocked: node X's blocker Y recovered" message to the coordinator (FROM the held node's role, reusing the `ping` seam). +- [ ] 4.4 Confirm no notice path exists for an already-materialized node whose blocker reopens (assert via test). +- [ ] 4.5 uxlog on re-arm clears and recovery notices. +- [ ] 4.6 Full Phase-A verification: `go test ./internal/db/... ./internal/mcp/... ./internal/heragater/... ./internal/tui/...` green; linter 0 issues. Report Phase A green to coordinator. + +## 5. Phase B — Tests (mutation verbs) + +**Depends on:** Stage 4 + +Write failing tests from the `hera-coordination` (mutation-verb + cancelled) and +`hera-view` (cancelled rendering) deltas. + +- [ ] 5.1 `internal/db`: failing tests for `RemoveHeraBlock(blocked, blocker)` (removes one edge; missing edge is a no-op), `SetHeraRolePrompt`/`UpdateHeraPlannedNode` (edits prompt/project), and the `cancelled_at` column (`CancelHeraPlannedNode`; `ListHeraPlannedNodes` excludes cancelled; `ListHeraBlocks`/render still surface it). +- [ ] 5.2 `internal/heragater`: failing tests — a cancelled node never materializes; a dependent whose only unsatisfied blocker is cancelled becomes eligible. +- [ ] 5.3 `internal/mcp` (hera_plan.go): failing tests — `hera_plan_node_update` edits a planned node, rejects a materialized node; `hera_unblock` drops an edge idempotently; `hera_plan_node_cancel` cancels a planned node, rejects a materialized one; all three reject non-coordinator callers. +- [ ] 5.4 `internal/tui/planview` + `internal/tui/hera`: failing SimulationScreen/projection test — a cancelled node renders a distinct grey ✕, kept visible, distinct from failed. +- [ ] 5.5 Confirm every Phase-B `it should X` criterion has a failing test. + +## 6. Phase B — store additions (edge-remove, prompt-edit, cancelled marker) + +**Depends on:** Stage 5 + +- [ ] 6.1 `internal/db/schema.go`: add `cancelled_at TEXT` to `hera_roles` + `idx_hera_roles_cancelled`. +- [ ] 6.2 `internal/db/hera_plan.go`: `RemoveHeraBlock(blocked, blocker int64) error` (DELETE on `hera_blocks`, idempotent). +- [ ] 6.3 `internal/db/hera_plan.go` / `hera.go`: `SetHeraRolePrompt`/`UpdateHeraPlannedNode` (prompt + project edit, planned-only path). +- [ ] 6.4 `internal/db/hera_plan.go`: `CancelHeraPlannedNode` (stamp `cancelled_at`); extend `ListHeraPlannedNodes` with `AND cancelled_at IS NULL`. +- [ ] 6.5 `internal/heragater`: `blockerOutcome` treats a cancelled blocker as non-gating (dependent proceeds). +- [ ] 6.6 uxlog on each store mutation. + +## 7. Phase B — MCP mutation verbs + +**Depends on:** Stage 6 + +- [ ] 7.1 `internal/mcp/hera_plan.go`: `hera_plan_node_update` handler + tool def (coordinator-guarded; reject if materialized). +- [ ] 7.2 `internal/mcp/hera_plan.go`: `hera_unblock` handler + tool def (coordinator-guarded; idempotent). +- [ ] 7.3 `internal/mcp/hera_plan.go`: `hera_plan_node_cancel` handler + tool def (coordinator-guarded; reject if materialized). +- [ ] 7.4 Register the three verbs in `heraToolDefs` / the CallTool dispatcher. +- [ ] 7.5 uxlog on each verb. + +## 8. Phase B — planview cancelled rendering + +**Depends on:** Stage 6 + +- [ ] 8.1 `internal/tui/hera` projection (`heraPlanNodes`): map a `cancelled_at` role to a new `planview` cancelled state. +- [ ] 8.2 `internal/tui/planview`: render the cancelled state as a distinct grey ✕ (rune-slice iteration; full-rect; no Sync — follow dag-rendering.md). +- [ ] 8.3 README Reference appendix: add the three MCP verbs to the hera tool table. +- [ ] 8.4 Full Phase-B verification: targeted `go test` green; linter 0 issues. Report Phase B green to coordinator. + +## 9. Phase C — authority + docs + +**Depends on:** Stage 8 + +- [ ] 9.1 `.claude/skills/hera/SKILL.md`: state the plan-DAG is authoritative for a bound coordinator; document the required `hera_send` status; document `hera_plan_node_update` / `hera_unblock` / `hera_plan_node_cancel`; add a "keep the DAG reconciled" standing order; reframe the create-only language. +- [ ] 9.2 `context/knowledge/gotchas/orchestration.md`: status-on-send synchronous apply; `failed` status + explicit gating; re-arm + recovery notice; cancelled-node semantics. +- [ ] 9.3 `context/knowledge/gotchas/dag-rendering.md`: cancelled node grey ✕ rendering rules. +- [ ] 9.4 `context/knowledge/gotchas/hera-view.md`: failed glyph precedence; cancelled node projection. +- [ ] 9.5 `context/knowledge/gotchas/messaging.md`: required worker→coord send-status + synchronous apply + soft-fail. +- [ ] 9.6 README Reference: add the `hera_send` `status` parameter to the tool table. +- [ ] 9.7 Report Phase C green to coordinator. From 6530fc026b47ca8f6943d481c0ccf51d3f6eea63 Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 01:28:36 -0700 Subject: [PATCH 02/14] =?UTF-8?q?Phase=20A:=20add=20failed=20hera=20role-s?= =?UTF-8?q?tatus=20(enum,=20roll-without-ready-to-close,=20rail=20red=20?= =?UTF-8?q?=E2=9C=95)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - schema.go: widen hera_role_status.status CHECK to include 'failed' - db/hera.go: add HeraStatusFailed constant; refactor shared rollHeraWorkerToReviewInner (bool stampReadyToClose param); new RollHeraWorkerFailed (in_review, no ready_to_close stamp) - mcp/hera.go: toolHeraStatus accepts 'failed' (error names all 5 values); HeraStore interface adds RollHeraWorkerFailed; worker reporting 'failed' routes to RollHeraWorkerFailed (D2) - widget/rolestatusicon.go: RoleStatusInputs.Failed field + '✕' red case in RoleStatusIcon, precedence below NeedsInput above Done - tui/hera/rail.go: roleStatusInputs sets Failed from HeraStatusFailed - Tests: hera_failed_test.go (db + mcp), widget + rail tests updated/extended 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/db/hera.go | 68 +++++++---- internal/db/hera_failed_test.go | 127 +++++++++++++++++++++ internal/db/schema.go | 2 +- internal/mcp/hera.go | 20 +++- internal/mcp/hera_failed_test.go | 110 ++++++++++++++++++ internal/tui/hera/rail.go | 1 + internal/tui/hera/rail_test.go | 48 ++++++++ internal/tui/widget/rolestatusicon.go | 10 +- internal/tui/widget/rolestatusicon_test.go | 48 +++++++- 9 files changed, 406 insertions(+), 28 deletions(-) create mode 100644 internal/db/hera_failed_test.go create mode 100644 internal/mcp/hera_failed_test.go diff --git a/internal/db/hera.go b/internal/db/hera.go index 2012b970..01281228 100644 --- a/internal/db/hera.go +++ b/internal/db/hera.go @@ -124,6 +124,7 @@ const ( HeraStatusWorking HeraRoleStatusValue = "working" HeraStatusBlocked HeraRoleStatusValue = "blocked" HeraStatusDone HeraRoleStatusValue = "done" + HeraStatusFailed HeraRoleStatusValue = "failed" ) // HeraOrchestrator is one coordination group. ArchivedAt is non-nil for @@ -810,25 +811,18 @@ func (d *DB) ManagedTaskIDs() (map[string]bool, error) { return out, nil } -// RollHeraWorkerToReview implements the BUG-050 worker close-out roll: it moves -// a worker-bound task to in_review and stamps meta:hera.ready_to_close=true. It -// is the SINGLE shared helper behind BOTH close-out triggers — the session-exit -// hooks (backstop) and the hera_status("done") hook (primary path, since Claude -// workers finish their report and go idle rather than exiting) — so the two can -// never drift. +// rollHeraWorkerToReviewInner is the shared implementation behind +// RollHeraWorkerToReview and RollHeraWorkerFailed. Both roll a live worker's +// bound task from in_progress to in_review; they differ only in whether +// ready_to_close is stamped (done → stamp; failed → no stamp, the task is not +// ready to close). Invariants enforced here so neither call-site can drift: +// - worker-kind only (coordinators/freelance are no-ops) +// - no-op unless the task is currently in_progress (never clobbers human state) +// - DB status + meta only — the live session is never touched +// - idempotent (second call is a no-op; task is already in_review) // -// It acts ONLY when the task currently holds a live worker-kind binding AND is -// in StatusInProgress. It never auto-completes, never clobbers a human-set -// in_review/complete (the in_progress guard), and never touches the agent -// session (DB status + meta only — callers must not stop/restart). Returns -// (true, nil) when it flipped, (false, nil) on a no-op (not worker-bound, or not -// in_progress). Idempotent: a second call is a no-op because the task is no -// longer in_progress. -// -// SetStatus emits task.status_changed OUTSIDE the DB mutex (events.md); the -// ready_to_close stamp is best-effort soft-fail — a meta failure is logged and -// the flip still stands. -func (d *DB) RollHeraWorkerToReview(taskID string) (bool, error) { +// Returns (true, nil) when flipped, (false, nil) on any no-op path. +func (d *DB) rollHeraWorkerToReviewInner(taskID string, stampReadyToClose bool) (bool, error) { worker, err := d.TaskHoldsLiveHeraWorkerBinding(taskID) if err != nil { return false, err @@ -846,12 +840,46 @@ func (d *DB) RollHeraWorkerToReview(taskID string) (bool, error) { if err := d.SetStatus(taskID, model.StatusInReview); err != nil { return false, err } - if mErr := d.SetMeta(taskID, HeraMetaNamespace, HeraMetaKeyReadyToClose, "true"); mErr != nil { - slog.Warn("[hera] ready_to_close stamp failed (flip stands)", "task", taskID, "err", mErr) + if stampReadyToClose { + if mErr := d.SetMeta(taskID, HeraMetaNamespace, HeraMetaKeyReadyToClose, "true"); mErr != nil { + slog.Warn("[hera] ready_to_close stamp failed (flip stands)", "task", taskID, "err", mErr) + } } return true, nil } +// RollHeraWorkerToReview implements the BUG-050 worker close-out roll: it moves +// a worker-bound task to in_review and stamps meta:hera.ready_to_close=true. It +// is the SINGLE shared helper behind BOTH close-out triggers — the session-exit +// hooks (backstop) and the hera_status("done") hook (primary path, since Claude +// workers finish their report and go idle rather than exiting) — so the two can +// never drift. +// +// It acts ONLY when the task currently holds a live worker-kind binding AND is +// in StatusInProgress. It never auto-completes, never clobbers a human-set +// in_review/complete (the in_progress guard), and never touches the agent +// session (DB status + meta only — callers must not stop/restart). Returns +// (true, nil) when it flipped, (false, nil) on a no-op (not worker-bound, or not +// in_progress). Idempotent: a second call is a no-op because the task is no +// longer in_progress. +// +// SetStatus emits task.status_changed OUTSIDE the DB mutex (events.md); the +// ready_to_close stamp is best-effort soft-fail — a meta failure is logged and +// the flip still stands. +func (d *DB) RollHeraWorkerToReview(taskID string) (bool, error) { + return d.rollHeraWorkerToReviewInner(taskID, true) +} + +// RollHeraWorkerFailed rolls a failed worker's bound task to in_review WITHOUT +// stamping ready_to_close. A failed task surfaces for coordinator attention +// (in_review) but is NOT ready to check off — the coordinator must decide +// whether to retry, reassign, or close it. Shares all invariants with +// RollHeraWorkerToReview: worker-kind only, no-op unless in_progress, idempotent, +// soft-fail (the roll succeeds even if the meta write fails). +func (d *DB) RollHeraWorkerFailed(taskID string) (bool, error) { + return d.rollHeraWorkerToReviewInner(taskID, false) +} + // ClearHeraReadyToClose removes the meta:hera.ready_to_close mark on taskID — // the inverse of the stamp RollHeraWorkerToReview sets when a worker reaches // `done`. Stepping a worker's hera status back DOWN the ladder (out of `done`) diff --git a/internal/db/hera_failed_test.go b/internal/db/hera_failed_test.go new file mode 100644 index 00000000..e97ea732 --- /dev/null +++ b/internal/db/hera_failed_test.go @@ -0,0 +1,127 @@ +package db + +import ( + "testing" + + "github.com/drn/argus/internal/model" + "github.com/drn/argus/internal/testutil" +) + +// TestHeraRoleStatus_Failed verifies that "failed" round-trips through +// UpsertHeraRoleStatus / HeraRoleStatusFor and is accepted by the CHECK +// constraint (schema.go). +func TestHeraRoleStatus_Failed(t *testing.T) { + d := heraTestDB(t) + o := mkOrch(t, d, "o") + r := mkRole(t, d, o.ID, "w", HeraKindWorker) + + testutil.NoError(t, d.UpsertHeraRoleStatus(r.ID, HeraStatusFailed)) + got, err := d.HeraRoleStatusFor(r.ID) + testutil.NoError(t, err) + testutil.Equal(t, got.Status, HeraStatusFailed) + + // Overwrite back to working — idempotent round-trip. + testutil.NoError(t, d.UpsertHeraRoleStatus(r.ID, HeraStatusWorking)) + got2, err := d.HeraRoleStatusFor(r.ID) + testutil.NoError(t, err) + testutil.Equal(t, got2.Status, HeraStatusWorking) +} + +// TestRollHeraWorkerFailed covers the D2 invariants for the failed roll helper: +// - in_progress worker flips to in_review WITHOUT ready_to_close +// - non-in_progress is a no-op (idempotent) +// - non-worker (coordinator) is a no-op +// - no binding is a no-op +// - second call is a no-op (idempotent) +func TestRollHeraWorkerFailed(t *testing.T) { + setup := func(t *testing.T, status model.Status, kind HeraRoleKind, bind bool) (*DB, string) { + t.Helper() + d := heraTestDB(t) + task := &model.Task{Name: "t", Status: status, Project: "p"} + testutil.NoError(t, d.Add(task)) + if bind { + o := mkOrch(t, d, "o") + r := mkRole(t, d, o.ID, "r", kind) + mkBinding(t, d, r.ID, task.ID, "/wt/t") + } + return d, task.ID + } + + readyToClose := func(d *DB, id string) bool { + meta, _ := d.ListMeta(id, HeraMetaNamespace) + for _, e := range meta { + if e.Key == HeraMetaKeyReadyToClose { + return e.Value == "true" + } + } + return false + } + + t.Run("worker in_progress -> flips to in_review, no ready_to_close", func(t *testing.T) { + d, id := setup(t, model.StatusInProgress, HeraKindWorker, true) + flipped, err := d.RollHeraWorkerFailed(id) + testutil.NoError(t, err) + testutil.Equal(t, flipped, true) + + got, _ := d.Get(id) + testutil.Equal(t, got.Status, model.StatusInReview) + // D2 key invariant: ready_to_close MUST NOT be set for a failed roll. + testutil.Equal(t, readyToClose(d, id), false) + }) + + t.Run("idempotent: second call is a no-op (task already in_review)", func(t *testing.T) { + d, id := setup(t, model.StatusInProgress, HeraKindWorker, true) + _, _ = d.RollHeraWorkerFailed(id) + flipped, err := d.RollHeraWorkerFailed(id) + testutil.NoError(t, err) + testutil.Equal(t, flipped, false) + }) + + t.Run("non-worker (coordinator) -> no-op", func(t *testing.T) { + d, id := setup(t, model.StatusInProgress, HeraKindCoordinator, true) + flipped, err := d.RollHeraWorkerFailed(id) + testutil.NoError(t, err) + testutil.Equal(t, flipped, false) + got, _ := d.Get(id) + testutil.Equal(t, got.Status, model.StatusInProgress) + }) + + t.Run("no binding -> no-op", func(t *testing.T) { + d, id := setup(t, model.StatusInProgress, HeraKindWorker, false) + flipped, err := d.RollHeraWorkerFailed(id) + testutil.NoError(t, err) + testutil.Equal(t, flipped, false) + }) + + t.Run("already complete -> not clobbered", func(t *testing.T) { + d, id := setup(t, model.StatusComplete, HeraKindWorker, true) + flipped, err := d.RollHeraWorkerFailed(id) + testutil.NoError(t, err) + testutil.Equal(t, flipped, false) + got, _ := d.Get(id) + testutil.Equal(t, got.Status, model.StatusComplete) + }) + + t.Run("human-set in_review -> not re-flipped", func(t *testing.T) { + d, id := setup(t, model.StatusInReview, HeraKindWorker, true) + flipped, err := d.RollHeraWorkerFailed(id) + testutil.NoError(t, err) + testutil.Equal(t, flipped, false) + }) + + // Contrast with RollHeraWorkerToReview: the done roll DOES stamp ready_to_close. + t.Run("contrast: done roll stamps ready_to_close, failed roll does not", func(t *testing.T) { + d, id := setup(t, model.StatusInProgress, HeraKindWorker, true) + flipped, err := d.RollHeraWorkerToReview(id) + testutil.NoError(t, err) + testutil.Equal(t, flipped, true) + testutil.Equal(t, readyToClose(d, id), true) + + // Reset task status so we can test the failed path independently. + d2, id2 := setup(t, model.StatusInProgress, HeraKindWorker, true) + flipped2, err2 := d2.RollHeraWorkerFailed(id2) + testutil.NoError(t, err2) + testutil.Equal(t, flipped2, true) + testutil.Equal(t, readyToClose(d2, id2), false) + }) +} diff --git a/internal/db/schema.go b/internal/db/schema.go index 9d534961..981415ac 100644 --- a/internal/db/schema.go +++ b/internal/db/schema.go @@ -496,7 +496,7 @@ func (d *DB) createHeraTables() error { CREATE TABLE IF NOT EXISTS hera_role_status ( role_id INTEGER PRIMARY KEY REFERENCES hera_roles(id) ON DELETE CASCADE, - status TEXT NOT NULL CHECK (status IN ('idle','working','blocked','done')), + status TEXT NOT NULL CHECK (status IN ('idle','working','blocked','done','failed')), updated_at TEXT NOT NULL ); diff --git a/internal/mcp/hera.go b/internal/mcp/hera.go index b8ce8095..f40cf8c5 100644 --- a/internal/mcp/hera.go +++ b/internal/mcp/hera.go @@ -55,6 +55,10 @@ type HeraStore interface { // session-exit hooks; the hera_status("done") trigger calls it. No-op // unless the task is a live worker AND currently in_progress. RollHeraWorkerToReview(taskID string) (bool, error) + // RollHeraWorkerFailed rolls a failed worker's task to in_review WITHOUT + // stamping ready_to_close (D2, make-hera-plan-living). Called when a worker + // reports status="failed"; same invariants as RollHeraWorkerToReview. + RollHeraWorkerFailed(taskID string) (bool, error) } // heraToolDefs contains the 12 hera_* tool schemas. The first 9 are ported @@ -740,9 +744,9 @@ func (s *Server) toolHeraStatus(id interface{}, args json.RawMessage) *Response sv := db.HeraRoleStatusValue(p.Status) switch sv { - case db.HeraStatusIdle, db.HeraStatusWorking, db.HeraStatusBlocked, db.HeraStatusDone: + case db.HeraStatusIdle, db.HeraStatusWorking, db.HeraStatusBlocked, db.HeraStatusDone, db.HeraStatusFailed: default: - return toolError(id, fmt.Sprintf("invalid status %q; must be one of idle, working, blocked, done", p.Status)) + return toolError(id, fmt.Sprintf("invalid status %q; must be one of idle, working, blocked, done, failed", p.Status)) } caller, err := s.resolveCallerRole(p.Cwd, p.Orchestrator) @@ -776,6 +780,18 @@ func (s *Server) toolHeraStatus(id interface{}, args json.RawMessage) *Response } } + // D2 (make-hera-plan-living): a WORKER reporting status="failed" rolls its + // bound task to in_review WITHOUT stamping ready_to_close — the task needs + // coordinator attention but is not ready to check off. Same soft-fail / + // idempotent invariants as the done roll above. + if sv == db.HeraStatusFailed && caller.role.Kind == db.HeraKindWorker { + if flipped, rErr := s.heraStore.RollHeraWorkerFailed(caller.binding.ArgusTaskID); rErr != nil { + slog.Warn("[hera] status(failed): worker roll failed (status still updated)", "task_id", caller.binding.ArgusTaskID, "err", rErr) + } else if flipped { + slog.Info("[hera] status(failed): rolled worker task to in_review (no ready_to_close)", "task_id", caller.binding.ArgusTaskID, "role", caller.role.Name) + } + } + slog.Info("[hera] status ok", "role", caller.role.Name, "status", p.Status, "orch", caller.orch.Name) var b strings.Builder fmt.Fprintf(&b, "Status updated.\n\n") diff --git a/internal/mcp/hera_failed_test.go b/internal/mcp/hera_failed_test.go new file mode 100644 index 00000000..93ad588a --- /dev/null +++ b/internal/mcp/hera_failed_test.go @@ -0,0 +1,110 @@ +package mcp + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/drn/argus/internal/db" + "github.com/drn/argus/internal/model" + "github.com/drn/argus/internal/testutil" +) + +// --- hera_status("failed") D2 tests --- + +// TestHera_Status_Failed_Accepted verifies that "failed" is now a valid status +// value (previously the enum was idle|working|blocked|done only). +func TestHera_Status_Failed_Accepted(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "myorch", "/wt/coord") + worker := addHeraTestTask(t, d, "/wt/worker") + attachWorker(t, s, "myorch", worker.Worktree) + + cr := heraStatus(t, s, worker.Worktree, "failed") + testutil.Equal(t, cr.IsError, false) + testutil.Contains(t, cr.Content[0].Text, "**status**: failed") +} + +// TestHera_Status_Failed_InvalidEnumNamesAllFive verifies that the invalid-status +// error message now enumerates all five values including "failed". +func TestHera_Status_Failed_InvalidEnumNamesAllFive(t *testing.T) { + s, d := testHeraServer(t) + task := addHeraTestTask(t, d, "/wt/status-five") + + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_status", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd":%q,"status":"winning" + }`, task.Worktree)), + }) + testutil.NoError(t, respErr(resp)) + cr := callResult(t, resp) + testutil.Equal(t, cr.IsError, true) + testutil.Contains(t, cr.Content[0].Text, "failed") + testutil.Contains(t, cr.Content[0].Text, "done") + testutil.Contains(t, cr.Content[0].Text, "idle") +} + +// TestHera_Status_Failed_RollsWorkerToReview verifies that a worker reporting +// status="failed" gets its bound task rolled to in_review. +func TestHera_Status_Failed_RollsWorkerToReview(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "myorch", "/wt/coord") + worker := addHeraTestTask(t, d, "/wt/worker") + attachWorker(t, s, "myorch", worker.Worktree) + + cr := heraStatus(t, s, worker.Worktree, "failed") + testutil.Equal(t, cr.IsError, false) + + got, _ := d.Get(worker.ID) + testutil.Equal(t, got.Status, model.StatusInReview) +} + +// TestHera_Status_Failed_NoReadyToClose is the D2 key invariant: a worker +// reporting "failed" must NOT stamp ready_to_close — the task is not done, just +// failed and surfaced for coordinator attention. +func TestHera_Status_Failed_NoReadyToClose(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "myorch", "/wt/coord") + worker := addHeraTestTask(t, d, "/wt/worker") + attachWorker(t, s, "myorch", worker.Worktree) + + cr := heraStatus(t, s, worker.Worktree, "failed") + testutil.Equal(t, cr.IsError, false) + + meta, _ := d.ListMeta(worker.ID, db.HeraMetaNamespace) + for _, e := range meta { + if e.Key == db.HeraMetaKeyReadyToClose && e.Value == "true" { + t.Fatalf("ready_to_close was stamped for a failed worker (D2 invariant violated)") + } + } +} + +// TestHera_Status_Failed_CoordinatorUnchanged verifies coordinators are not +// rolled when they report "failed" (the roll is worker-kind only). +func TestHera_Status_Failed_CoordinatorUnchanged(t *testing.T) { + s, d := testHeraServer(t) + coord := seedCoordinator(t, s, d, "myorch", "/wt/coord") + + cr := heraStatus(t, s, coord.Worktree, "failed") + testutil.Equal(t, cr.IsError, false) + + got, _ := d.Get(coord.ID) + testutil.Equal(t, got.Status, model.StatusInProgress) // NOT rolled +} + +// TestHera_Status_Failed_DoesNotClobberComplete verifies idempotency when the +// task is already terminal (same guard as the done roll). +func TestHera_Status_Failed_DoesNotClobberComplete(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "myorch", "/wt/coord") + worker := addHeraTestTask(t, d, "/wt/worker") + attachWorker(t, s, "myorch", worker.Worktree) + testutil.NoError(t, d.SetStatus(worker.ID, model.StatusComplete)) + + cr := heraStatus(t, s, worker.Worktree, "failed") + testutil.Equal(t, cr.IsError, false) + + got, _ := d.Get(worker.ID) + testutil.Equal(t, got.Status, model.StatusComplete) // not clobbered +} diff --git a/internal/tui/hera/rail.go b/internal/tui/hera/rail.go index 7dc963c9..6a9d5a28 100644 --- a/internal/tui/hera/rail.go +++ b/internal/tui/hera/rail.go @@ -1501,6 +1501,7 @@ func roleStatusInputs(role *RoleView) widget.RoleStatusInputs { return widget.RoleStatusInputs{ ReadyToClose: role.ReadyToClose, NeedsInput: role.ShowsNeedsInput(), + Failed: role.HasStatus && role.Status == db.HeraStatusFailed, Done: role.HasStatus && role.Status == db.HeraStatusDone, Active: role.IsActive(), Idle: role.HasStatus && role.Status == db.HeraStatusIdle, diff --git a/internal/tui/hera/rail_test.go b/internal/tui/hera/rail_test.go index 4fd7e1ec..1eb3fdcf 100644 --- a/internal/tui/hera/rail_test.go +++ b/internal/tui/hera/rail_test.go @@ -946,6 +946,7 @@ func TestStatusIcon_StatusMapping(t *testing.T) { {db.HeraStatusBlocked}, {db.HeraStatusDone}, {db.HeraStatusIdle}, + {db.HeraStatusFailed}, } for _, c := range cases { // Each known status yields a non-zero glyph without panicking. @@ -1082,6 +1083,53 @@ func TestStatusIcon_NeedsInputSources(t *testing.T) { }) } +// TestStatusIcon_Failed pins the D2 (make-hera-plan-living) failed rail glyph. +// A role with status "failed" renders a red ✕ via roleStatusInputs → widget, +// placed below NeedsInput and above Done in precedence. +func TestStatusIcon_Failed(t *testing.T) { + t.Run("failed renders ✕", func(t *testing.T) { + icon, style := statusIcon(&RoleView{HasStatus: true, Status: db.HeraStatusFailed}, false, 0) + testutil.Equal(t, icon, '✕') + testutil.Equal(t, style, theme.StyleError) + }) + + t.Run("failed is distinct from done ✓", func(t *testing.T) { + failedIcon, _ := statusIcon(&RoleView{HasStatus: true, Status: db.HeraStatusFailed}, false, 0) + doneIcon, _ := statusIcon(&RoleView{HasStatus: true, Status: db.HeraStatusDone}, false, 0) + if failedIcon == doneIcon { + t.Fatalf("failed glyph %q must differ from done glyph %q", failedIcon, doneIcon) + } + }) + + t.Run("needs-input beats failed", func(t *testing.T) { + // ShowsNeedsInput uses NeedsInput || SubtreeNeedsInput. + icon, _ := statusIcon(&RoleView{ + HasStatus: true, Status: db.HeraStatusFailed, NeedsInput: true, + }, false, 0) + testutil.Equal(t, icon, theme.IconNeedsInput) + }) + + t.Run("ready_to_close beats failed", func(t *testing.T) { + icon, _ := statusIcon(&RoleView{ + HasStatus: true, Status: db.HeraStatusFailed, ReadyToClose: true, + }, false, 0) + testutil.Equal(t, icon, theme.IconReview) + }) + + t.Run("failed appears in TestStatusIcon_StatusMapping set", func(t *testing.T) { + // All five role-status values yield a non-zero glyph without panicking. + for _, sv := range []db.HeraRoleStatusValue{ + db.HeraStatusIdle, db.HeraStatusWorking, db.HeraStatusBlocked, + db.HeraStatusDone, db.HeraStatusFailed, + } { + icon, _ := statusIcon(&RoleView{HasStatus: true, Status: sv}, false, 0) + if icon == 0 { + t.Errorf("status %q produced zero glyph", sv) + } + } + }) +} + // TestRail_DrawDoesNotPanic exercises every drawRow branch against a real // SimulationScreen (the required SimulationScreen integration for new widget // rendering). It covers orchestrators, roles, freelance, archive, rules, and diff --git a/internal/tui/widget/rolestatusicon.go b/internal/tui/widget/rolestatusicon.go index ebf1344d..f908cca7 100644 --- a/internal/tui/widget/rolestatusicon.go +++ b/internal/tui/widget/rolestatusicon.go @@ -18,6 +18,10 @@ type RoleStatusInputs struct { // NeedsInput: the role's needs-input signal (own OR subtree rollup) — a worker // blocked on a user prompt. Ranks below ready_to_close, above done/active. NeedsInput bool + // Failed: the hera role status is "failed" (D2, make-hera-plan-living) — a + // worker that self-reported defeat. Ranks below NeedsInput, above Done. Renders + // a red ✕ distinct from the Done ✓ (a failed task is not done). + Failed bool // Done: the hera role status is "done". Done bool // Active: genuinely producing output (live binding + bound task in_progress) — @@ -35,7 +39,7 @@ type RoleStatusInputs struct { // (BUG-007). `frame` is the current spinner animation frame (the Active case // animates via SpinnerFrame); `dim` forces the dimmed style for archived // placement (the glyph never lies — only the style dims). Precedence: -// ready_to_close → needs-input → done → active(spinner) → idle → live → default. +// ready_to_close → needs-input → failed(red ✕) → done → active(spinner) → idle → live → default. func RoleStatusIcon(in RoleStatusInputs, dim bool, frame int) (rune, tcell.Style) { if in.ReadyToClose { st := tcell.StyleDefault.Foreground(theme.ColorComplete).Bold(true) @@ -49,6 +53,10 @@ func RoleStatusIcon(in RoleStatusInputs, dim bool, frame int) (rune, tcell.Style switch { case in.NeedsInput: glyph, style = theme.IconNeedsInput, theme.StyleNeedsInput + case in.Failed: + // D2 (make-hera-plan-living): explicit self-reported defeat — red ✕, + // distinct from the Done ✓ (a failed task is not done, not ready to close). + glyph, style = '✕', theme.StyleError case in.Done: glyph, style = '✓', theme.StyleComplete case in.Active: diff --git a/internal/tui/widget/rolestatusicon_test.go b/internal/tui/widget/rolestatusicon_test.go index 6b28c637..f0b4e140 100644 --- a/internal/tui/widget/rolestatusicon_test.go +++ b/internal/tui/widget/rolestatusicon_test.go @@ -9,8 +9,8 @@ import ( // TestRoleStatusIcon_Precedence pins the shared classifier's precedence + // vocabulary (BUG-007): the single source of truth the rail and the plan view -// both consume. ready_to_close → needs-input → done → active → idle → live → -// default. +// both consume. ready_to_close → needs-input → failed(red ✕) → done → active → +// idle → live → default. func TestRoleStatusIcon_Precedence(t *testing.T) { const frame = 0 cases := []struct { @@ -18,8 +18,9 @@ func TestRoleStatusIcon_Precedence(t *testing.T) { in RoleStatusInputs wantGlyph rune }{ - {"ready_to_close wins over all", RoleStatusInputs{ReadyToClose: true, NeedsInput: true, Done: true, Active: true}, theme.IconReview}, - {"needs-input over done/active", RoleStatusInputs{NeedsInput: true, Done: true, Active: true}, theme.IconNeedsInput}, + {"ready_to_close wins over all", RoleStatusInputs{ReadyToClose: true, NeedsInput: true, Failed: true, Done: true, Active: true}, theme.IconReview}, + {"needs-input over failed/done/active", RoleStatusInputs{NeedsInput: true, Failed: true, Done: true, Active: true}, theme.IconNeedsInput}, + {"failed over done/active", RoleStatusInputs{Failed: true, Done: true, Active: true}, '✕'}, {"done over active", RoleStatusInputs{Done: true, Active: true}, '✓'}, {"active → spinner frame", RoleStatusInputs{Active: true}, SpinnerFrame(frame)}, {"idle moon-outline", RoleStatusInputs{Idle: true, Live: true}, theme.IconMoonOutline}, @@ -34,6 +35,45 @@ func TestRoleStatusIcon_Precedence(t *testing.T) { } } +// TestRoleStatusIcon_Failed pins the D2 (make-hera-plan-living) failed glyph: +// a red ✕ distinct from the Done ✓, placed below NeedsInput and above Done in +// precedence. +func TestRoleStatusIcon_Failed(t *testing.T) { + t.Run("failed renders red ✕", func(t *testing.T) { + glyph, style := RoleStatusIcon(RoleStatusInputs{Failed: true}, false, 0) + testutil.Equal(t, glyph, '✕') + testutil.Equal(t, style, theme.StyleError) + }) + + t.Run("failed is distinct from done ✓", func(t *testing.T) { + failedGlyph, _ := RoleStatusIcon(RoleStatusInputs{Failed: true}, false, 0) + doneGlyph, _ := RoleStatusIcon(RoleStatusInputs{Done: true}, false, 0) + if failedGlyph == doneGlyph { + t.Fatalf("failed glyph %q must differ from done glyph %q", failedGlyph, doneGlyph) + } + }) + + t.Run("needs-input beats failed", func(t *testing.T) { + glyph, _ := RoleStatusIcon(RoleStatusInputs{NeedsInput: true, Failed: true}, false, 0) + testutil.Equal(t, glyph, theme.IconNeedsInput) + }) + + t.Run("ready_to_close beats failed", func(t *testing.T) { + glyph, _ := RoleStatusIcon(RoleStatusInputs{ReadyToClose: true, Failed: true}, false, 0) + testutil.Equal(t, glyph, theme.IconReview) + }) + + t.Run("failed beats done", func(t *testing.T) { + glyph, _ := RoleStatusIcon(RoleStatusInputs{Failed: true, Done: true}, false, 0) + testutil.Equal(t, glyph, '✕') + }) + + t.Run("dim forces dimmed style even for failed", func(t *testing.T) { + _, style := RoleStatusIcon(RoleStatusInputs{Failed: true}, true, 0) + testutil.Equal(t, style, theme.StyleDimmed) + }) +} + // TestRoleStatusIcon_DimForcesDimStyle: archived placement forces the dimmed // style (the glyph never lies — only the style dims). func TestRoleStatusIcon_DimForcesDimStyle(t *testing.T) { From dafed4cc8172beeed3d09ac8269e59f34868d1b2 Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 01:37:22 -0700 Subject: [PATCH 03/14] Phase A: required synchronous status on worker hera_send MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract applyRoleStatus helper (DRY): hera_send and hera_status now share one path for status validation, upsert, meta mirror, and worker task roll (done→in_review+ready_to_close, failed→in_review/no-ready_to_close) - hera_send gains optional status param; REQUIRED for worker/freelance senders: missing status returns a hard error naming all five valid values and sends nothing; status is applied synchronously before message send (D1); soft-fail on apply error never blocks the send; coordinator status remains optional - Update hera_send tool description and schema to document the requirement - Update all existing worker hera_send call sites in tests to include status - New hera_send_status_test.go: 7 tests covering worker-no-status rejection, freelance rejection, status=working applied synchronously, status=done rolls task to in_review+ready_to_close, status=failed rolls to in_review without ready_to_close, coordinator no-status succeeds, delivery-independent apply 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/mcp/hera.go | 119 +++++++++----- internal/mcp/hera_send_status_test.go | 218 ++++++++++++++++++++++++++ internal/mcp/hera_test.go | 28 ++-- 3 files changed, 314 insertions(+), 51 deletions(-) create mode 100644 internal/mcp/hera_send_status_test.go diff --git a/internal/mcp/hera.go b/internal/mcp/hera.go index f40cf8c5..aaf2b4f4 100644 --- a/internal/mcp/hera.go +++ b/internal/mcp/hera.go @@ -101,7 +101,7 @@ var heraToolDefs = []Tool{ }, { Name: "hera_send", - Description: "Send a message to another role in the same orchestrator. Workers/freelancers default to the coordinator when 'to' is omitted. Coordinators must supply an explicit 'to'.", + Description: "Send a message to another role in the same orchestrator. Workers/freelancers default to the coordinator when 'to' is omitted. Coordinators must supply an explicit 'to'. Worker/freelance senders MUST supply 'status' (one of idle/working/blocked/done/failed) — it is applied to the sender's role synchronously before the message is sent.", InputSchema: map[string]interface{}{ "type": "object", "properties": map[string]interface{}{ @@ -111,6 +111,7 @@ var heraToolDefs = []Tool{ "to": map[string]interface{}{"type": "string", "description": "(optional for worker/freelance, required for coordinator) Recipient role name within the same orchestrator"}, "in_reply_to": map[string]interface{}{"type": "integer", "description": "(optional) Message id this is a reply to"}, "orchestrator": map[string]interface{}{"type": "string", "description": "(required when the caller's argus task holds 2+ live bindings; optional when it holds exactly one) The orchestrator whose binding identifies the sender role for this call. The recipient is resolved within the same orchestrator."}, + "status": map[string]interface{}{"type": "string", "enum": []string{"idle", "working", "blocked", "done", "failed"}, "description": "REQUIRED for worker/freelance senders: the sender's current role status, applied synchronously before the message is sent. Optional for coordinator senders (omit to leave status unchanged)."}, }, "required": []string{"cwd", "body", "tldr"}, }, @@ -554,6 +555,7 @@ func (s *Server) toolHeraSend(id interface{}, args json.RawMessage) *Response { To string `json:"to"` InReplyTo *int64 `json:"in_reply_to"` Orchestrator string `json:"orchestrator"` + Status string `json:"status"` } json.Unmarshal(args, &p) //nolint:errcheck @@ -572,6 +574,37 @@ func (s *Server) toolHeraSend(id interface{}, args json.RawMessage) *Response { return toolError(id, err.Error()) } + // D1 (make-hera-plan-living): worker/freelance senders MUST supply a status. + // Enforce BEFORE recipient resolution so no message is sent on omission. + switch caller.role.Kind { + case db.HeraKindWorker, db.HeraKindFreelance: + if p.Status == "" { + return toolError(id, "status is required for worker/freelance senders; must be one of idle, working, blocked, done, failed") + } + } + + // Validate the status value when supplied (coordinator or worker/freelance). + var sv db.HeraRoleStatusValue + if p.Status != "" { + sv = db.HeraRoleStatusValue(p.Status) + switch sv { + case db.HeraStatusIdle, db.HeraStatusWorking, db.HeraStatusBlocked, db.HeraStatusDone, db.HeraStatusFailed: + default: + return toolError(id, fmt.Sprintf("invalid status %q; must be one of idle, working, blocked, done, failed", p.Status)) + } + } + + // Apply status SYNCHRONOUSLY before the message is sent (D1). This decouples + // the authoritative state change from the best-effort doorbell delivery path. + // Soft-fail: a status-apply error must NOT block the message send. + if p.Status != "" { + if applyErr := s.applyRoleStatus(caller, sv); applyErr != nil { + slog.Warn("[hera] send: status apply failed (proceeding with send)", "role", caller.role.Name, "status", p.Status, "err", applyErr) + } else { + slog.Info("[hera] send: status applied", "role", caller.role.Name, "status", p.Status) + } + } + // Resolve recipient. var toRole *db.HeraRole if p.To != "" { @@ -724,6 +757,52 @@ func (s *Server) toolHeraMarkRead(id interface{}, args json.RawMessage) *Respons return toolResult(id, fmt.Sprintf("Marked %d of %d message ID%s as read.", n, len(p.MessageIDs), plural(len(p.MessageIDs)))) } +// applyRoleStatus validates sv, upserts the role status, mirrors to task_meta, +// and performs the worker task roll (done→in_review+ready_to_close, +// failed→in_review/no-ready_to_close). This is the single shared path used by +// both toolHeraStatus and toolHeraSend — keeping the two callers identical so +// they can never drift (D1, make-hera-plan-living). +// +// caller must be fully resolved before this call. Returns the first hard error +// (upsert failure); the task-roll is always soft-fail (logged, not returned). +// Callers that need soft-fail semantics on the whole apply (hera_send) should +// log any returned error rather than blocking the message send. +func (s *Server) applyRoleStatus(caller *callerRoleResult, sv db.HeraRoleStatusValue) error { + if err := s.heraStore.UpsertHeraRoleStatus(caller.role.ID, sv); err != nil { + return fmt.Errorf("update status failed: %w", err) + } + + // Mirror to task_meta best-effort. + if metaErr := s.heraStore.SetMeta(caller.binding.ArgusTaskID, db.HeraMetaNamespace, db.HeraMetaKeyThreadStatus, string(sv)); metaErr != nil { + slog.Warn("[hera] meta mirror failed", "role", caller.role.Name, "task_id", caller.binding.ArgusTaskID, "err", metaErr) + } + + // BUG-050 PRIMARY trigger: a WORKER reporting status="done" rolls its bound + // task to in_review + stamps ready_to_close — the idle-but-done case the + // exit hook misses. Worker-kind ONLY; RollHeraWorkerToReview no-ops unless + // the task is in_progress (never clobbers human-set state). Soft-fail. + if sv == db.HeraStatusDone && caller.role.Kind == db.HeraKindWorker { + if flipped, rErr := s.heraStore.RollHeraWorkerToReview(caller.binding.ArgusTaskID); rErr != nil { + slog.Warn("[hera] apply-status(done): worker roll failed (status still updated)", "task_id", caller.binding.ArgusTaskID, "err", rErr) + } else if flipped { + slog.Info("[hera] apply-status(done): rolled worker task to in_review", "task_id", caller.binding.ArgusTaskID, "role", caller.role.Name) + } + } + + // D2 (make-hera-plan-living): a WORKER reporting status="failed" rolls its + // bound task to in_review WITHOUT stamping ready_to_close. Same soft-fail / + // idempotent invariants as the done roll above. + if sv == db.HeraStatusFailed && caller.role.Kind == db.HeraKindWorker { + if flipped, rErr := s.heraStore.RollHeraWorkerFailed(caller.binding.ArgusTaskID); rErr != nil { + slog.Warn("[hera] apply-status(failed): worker roll failed (status still updated)", "task_id", caller.binding.ArgusTaskID, "err", rErr) + } else if flipped { + slog.Info("[hera] apply-status(failed): rolled worker task to in_review (no ready_to_close)", "task_id", caller.binding.ArgusTaskID, "role", caller.role.Name) + } + } + + return nil +} + func (s *Server) toolHeraStatus(id interface{}, args json.RawMessage) *Response { if !s.heraEnabled() { return toolError(id, "hera not configured") @@ -754,42 +833,8 @@ func (s *Server) toolHeraStatus(id interface{}, args json.RawMessage) *Response return toolError(id, err.Error()) } - if err := s.heraStore.UpsertHeraRoleStatus(caller.role.ID, sv); err != nil { - return toolError(id, fmt.Sprintf("update status failed: %v", err)) - } - - // Mirror to task_meta best-effort. - if metaErr := s.heraStore.SetMeta(caller.binding.ArgusTaskID, db.HeraMetaNamespace, db.HeraMetaKeyThreadStatus, p.Status); metaErr != nil { - slog.Warn("[hera] meta mirror failed", "tool", "hera_status", "task_id", caller.binding.ArgusTaskID, "err", metaErr) - } - - // BUG-050 PRIMARY trigger: a WORKER reporting status="done" rolls its bound - // task to in_review + stamps ready_to_close — the idle-but-done case the - // exit hook misses (Claude workers finish their report and go idle, they - // don't exit). Worker-kind ONLY (coordinators/freelance just update status); - // RollHeraWorkerToReview itself no-ops unless the task is in_progress, so it - // never clobbers a human-set in_review/complete and never auto-completes. It - // touches DB status + meta only — the live session is left running. Failure - // is soft (logged, never surfaced) so the status update always succeeds; the - // call is idempotent (re-calling done is a no-op once flipped). - if sv == db.HeraStatusDone && caller.role.Kind == db.HeraKindWorker { - if flipped, rErr := s.heraStore.RollHeraWorkerToReview(caller.binding.ArgusTaskID); rErr != nil { - slog.Warn("[hera] status(done): worker roll failed (status still updated)", "task_id", caller.binding.ArgusTaskID, "err", rErr) - } else if flipped { - slog.Info("[hera] status(done): rolled worker task to in_review", "task_id", caller.binding.ArgusTaskID, "role", caller.role.Name) - } - } - - // D2 (make-hera-plan-living): a WORKER reporting status="failed" rolls its - // bound task to in_review WITHOUT stamping ready_to_close — the task needs - // coordinator attention but is not ready to check off. Same soft-fail / - // idempotent invariants as the done roll above. - if sv == db.HeraStatusFailed && caller.role.Kind == db.HeraKindWorker { - if flipped, rErr := s.heraStore.RollHeraWorkerFailed(caller.binding.ArgusTaskID); rErr != nil { - slog.Warn("[hera] status(failed): worker roll failed (status still updated)", "task_id", caller.binding.ArgusTaskID, "err", rErr) - } else if flipped { - slog.Info("[hera] status(failed): rolled worker task to in_review (no ready_to_close)", "task_id", caller.binding.ArgusTaskID, "role", caller.role.Name) - } + if err := s.applyRoleStatus(caller, sv); err != nil { + return toolError(id, err.Error()) } slog.Info("[hera] status ok", "role", caller.role.Name, "status", p.Status, "orch", caller.orch.Name) diff --git a/internal/mcp/hera_send_status_test.go b/internal/mcp/hera_send_status_test.go new file mode 100644 index 00000000..cec54eeb --- /dev/null +++ b/internal/mcp/hera_send_status_test.go @@ -0,0 +1,218 @@ +package mcp + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/drn/argus/internal/db" + "github.com/drn/argus/internal/model" + "github.com/drn/argus/internal/testutil" +) + +// --- hera_send required status (D1, make-hera-plan-living) --- + +// heraSend is a convenience wrapper that calls hera_send via the MCP tool and +// returns the ToolCallResult. Mirrors heraStatus in hera_test.go. +func heraSend(t *testing.T, s *Server, cwd, body, tldr, status, to string) ToolCallResult { + t.Helper() + args := map[string]interface{}{ + "cwd": cwd, + "body": body, + "tldr": tldr, + } + if status != "" { + args["status"] = status + } + if to != "" { + args["to"] = to + } + raw, err := json.Marshal(args) + testutil.NoError(t, err) + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_send", + Arguments: json.RawMessage(raw), + }) + testutil.NoError(t, respErr(resp)) + return callResult(t, resp) +} + +// TestHeraSend_Worker_NoStatus_Rejected verifies that a worker calling +// hera_send without a status gets a hard error naming all five valid values, +// and that no message is persisted. +func TestHeraSend_Worker_NoStatus_Rejected(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "myorch", "/wt/coord") + worker := addHeraTestTask(t, d, "/wt/worker") + attachWorker(t, s, "myorch", worker.Worktree) + + // Send with no status — must fail before any message is sent. + cr := heraSend(t, s, worker.Worktree, "report body", "done report", "", "") + testutil.Equal(t, cr.IsError, true) + // Error message must name all five valid values. + for _, v := range []string{"idle", "working", "blocked", "done", "failed"} { + testutil.Contains(t, cr.Content[0].Text, v) + } + + // Verify no message was persisted in the DB. The coordinator's inbox must be + // empty (we check via hera_inbox on the coordinator worktree). + coordInbox := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_inbox", + Arguments: json.RawMessage(`{"cwd":"/wt/coord"}`), + }) + testutil.NoError(t, respErr(coordInbox)) + inboxCR := callResult(t, coordInbox) + testutil.Equal(t, inboxCR.IsError, false) + testutil.Contains(t, inboxCR.Content[0].Text, "Inbox empty") +} + +// TestHeraSend_Freelance_NoStatus_Rejected verifies that a freelance role +// (same as worker) also requires status on hera_send. +func TestHeraSend_Freelance_NoStatus_Rejected(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "myorch", "/wt/coord") + + freelanceTask := addHeraTestTask(t, d, "/wt/freelance") + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_join", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd":%q,"orchestrator":"myorch","role_name":"fl","kind":"freelance" + }`, freelanceTask.Worktree)), + }) + testutil.Equal(t, callResult(t, resp).IsError, false) + + cr := heraSend(t, s, freelanceTask.Worktree, "freelance report", "report", "", "") + testutil.Equal(t, cr.IsError, true) + testutil.Contains(t, cr.Content[0].Text, "status is required") +} + +// TestHeraSend_Worker_StatusWorking_Applied verifies that a worker sending with +// status=working has its role status set to working synchronously, and the +// message is delivered. +func TestHeraSend_Worker_StatusWorking_Applied(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "myorch", "/wt/coord") + worker := addHeraTestTask(t, d, "/wt/worker") + attachWorker(t, s, "myorch", worker.Worktree) + + cr := heraSend(t, s, worker.Worktree, "still working", "progress", "working", "") + testutil.Equal(t, cr.IsError, false) + testutil.Contains(t, cr.Content[0].Text, "Message sent") + + // Role status must be "working" synchronously after the call. + orch, err := d.HeraOrchestratorByName("myorch") + testutil.NoError(t, err) + role, err := d.HeraRoleByName(orch.ID, "w1") + testutil.NoError(t, err) + rs, err := d.HeraRoleStatusFor(role.ID) + testutil.NoError(t, err) + testutil.Equal(t, rs.Status, db.HeraStatusWorking) +} + +// TestHeraSend_Worker_StatusDone_RollsToReview verifies that a worker sending +// with status=done rolls its bound argus task to in_review + stamps +// ready_to_close, exactly as hera_status(done) would. +func TestHeraSend_Worker_StatusDone_RollsToReview(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "myorch", "/wt/coord") + worker := addHeraTestTask(t, d, "/wt/worker") // StatusInProgress + attachWorker(t, s, "myorch", worker.Worktree) + + cr := heraSend(t, s, worker.Worktree, "all done", "done report", "done", "") + testutil.Equal(t, cr.IsError, false) + + // Task must have been rolled to in_review synchronously. + got, err := d.Get(worker.ID) + testutil.NoError(t, err) + testutil.Equal(t, got.Status, model.StatusInReview) + + // ready_to_close must be stamped. + meta, _ := d.ListMeta(worker.ID, db.HeraMetaNamespace) + found := false + for _, e := range meta { + if e.Key == db.HeraMetaKeyReadyToClose && e.Value == "true" { + found = true + } + } + testutil.Equal(t, found, true) +} + +// TestHeraSend_Worker_StatusFailed_RollsToReviewNoReadyToClose verifies that a +// worker sending with status=failed rolls its bound task to in_review WITHOUT +// stamping ready_to_close (D2 invariant). +func TestHeraSend_Worker_StatusFailed_RollsToReviewNoReadyToClose(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "myorch", "/wt/coord") + worker := addHeraTestTask(t, d, "/wt/worker") + attachWorker(t, s, "myorch", worker.Worktree) + + cr := heraSend(t, s, worker.Worktree, "hit a wall", "failed", "failed", "") + testutil.Equal(t, cr.IsError, false) + + got, err := d.Get(worker.ID) + testutil.NoError(t, err) + testutil.Equal(t, got.Status, model.StatusInReview) + + // ready_to_close must NOT be stamped. + meta, _ := d.ListMeta(worker.ID, db.HeraMetaNamespace) + for _, e := range meta { + if e.Key == db.HeraMetaKeyReadyToClose && e.Value == "true" { + t.Fatalf("ready_to_close was stamped for a failed worker (D2 invariant violated)") + } + } +} + +// TestHeraSend_Coordinator_NoStatus_Succeeds verifies that a coordinator sender +// with explicit "to" and no status successfully sends the message and leaves the +// coordinator's status unchanged. +func TestHeraSend_Coordinator_NoStatus_Succeeds(t *testing.T) { + s, d := testHeraServer(t) + coord := seedCoordinator(t, s, d, "myorch", "/wt/coord") + worker := addHeraTestTask(t, d, "/wt/worker") + attachWorker(t, s, "myorch", worker.Worktree) + + // Set coordinator to a known status first so we can verify it's unchanged. + cr0 := heraStatus(t, s, coord.Worktree, "working") + testutil.Equal(t, cr0.IsError, false) + + // Coordinator sends without status. + cr := heraSend(t, s, coord.Worktree, "task for you", "assignment", "", "w1") + testutil.Equal(t, cr.IsError, false) + testutil.Contains(t, cr.Content[0].Text, "Message sent") + testutil.Contains(t, cr.Content[0].Text, "**to**: w1") + + // Coordinator's status must still be "working" (unchanged). + orch, err := d.HeraOrchestratorByName("myorch") + testutil.NoError(t, err) + role, err := d.HeraRoleByName(orch.ID, "coord") + testutil.NoError(t, err) + rs, err := d.HeraRoleStatusFor(role.ID) + testutil.NoError(t, err) + testutil.Equal(t, rs.Status, db.HeraStatusWorking) +} + +// TestHeraSend_StatusApply_IndependentOfDelivery verifies that the status +// mutation is applied synchronously even when the notifier is nil (meaning +// doorbell delivery is skipped). This pins the D1 invariant that status never +// rides the best-effort delivery path. +// +// The testHeraServer wires hera.New(d, nil) — nil notifier — so delivery is +// always skipped in MCP tests, yet status must apply regardless. +func TestHeraSend_StatusApply_IndependentOfDelivery(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "myorch", "/wt/coord") + worker := addHeraTestTask(t, d, "/wt/worker") + attachWorker(t, s, "myorch", worker.Worktree) + + // nil notifier → delivery is deferred/dropped; status must still apply. + cr := heraSend(t, s, worker.Worktree, "body", "tldr", "blocked", "") + testutil.Equal(t, cr.IsError, false) + + orch, err := d.HeraOrchestratorByName("myorch") + testutil.NoError(t, err) + role, err := d.HeraRoleByName(orch.ID, "w1") + testutil.NoError(t, err) + rs, err := d.HeraRoleStatusFor(role.ID) + testutil.NoError(t, err) + testutil.Equal(t, rs.Status, db.HeraStatusBlocked) +} diff --git a/internal/mcp/hera_test.go b/internal/mcp/hera_test.go index 489a19f9..f76278b6 100644 --- a/internal/mcp/hera_test.go +++ b/internal/mcp/hera_test.go @@ -545,7 +545,7 @@ func TestHera_Send_DefaultRoute_WorkerToCoordinator(t *testing.T) { resp := doRequest(t, s, "tools/call", ToolCallParams{ Name: "hera_send", Arguments: json.RawMessage(fmt.Sprintf(`{ - "cwd":%q,"body":"hello coord","tldr":"greeting" + "cwd":%q,"body":"hello coord","tldr":"greeting","status":"working" }`, workerWt)), }) testutil.NoError(t, respErr(resp)) @@ -616,7 +616,7 @@ func TestHera_Inbox_MarksRead(t *testing.T) { doRequest(t, s, "tools/call", ToolCallParams{ Name: "hera_send", Arguments: json.RawMessage(fmt.Sprintf(`{ - "cwd":%q,"body":"status update","tldr":"update" + "cwd":%q,"body":"status update","tldr":"update","status":"working" }`, workerWt)), }) @@ -653,7 +653,7 @@ func TestHera_MarkRead(t *testing.T) { r := doRequest(t, s, "tools/call", ToolCallParams{ Name: "hera_send", Arguments: json.RawMessage(fmt.Sprintf(`{ - "cwd":%q,"body":"msg %d","tldr":%q + "cwd":%q,"body":"msg %d","tldr":%q,"status":"working" }`, workerWt, i, tldr)), }) cr := callResult(t, r) @@ -748,7 +748,7 @@ func TestHera_GetMessages_HappyPath(t *testing.T) { sendResp := doRequest(t, s, "tools/call", ToolCallParams{ Name: "hera_send", Arguments: json.RawMessage(fmt.Sprintf(`{ - "cwd":%q,"body":"hello coord","tldr":"hi" + "cwd":%q,"body":"hello coord","tldr":"hi","status":"working" }`, workerWt)), }) cr := callResult(t, sendResp) @@ -805,11 +805,11 @@ func TestHera_GetMessages_AccessRule(t *testing.T) { }`, workerTask2.Worktree)), }) - // Worker in orch2 sends to coord2. + // Worker in orch1 sends to coord1 (default route). sendResp := doRequest(t, s, "tools/call", ToolCallParams{ Name: "hera_send", Arguments: json.RawMessage(fmt.Sprintf(`{ - "cwd":%q,"body":"private","tldr":"private" + "cwd":%q,"body":"private","tldr":"private","status":"working" }`, workerWt1)), // worker1 in orch1 }) cr := callResult(t, sendResp) @@ -947,10 +947,10 @@ func TestHera_TreeUpdates_SubtreeRollup(t *testing.T) { Name: "hera_send", Arguments: json.RawMessage(fmt.Sprintf(`{"cwd":%q,"to":"w1","body":"root body","tldr":"root-tldr"}`, coordWt)), }))) - // Sub-orch message: sw → subcoord (default route). + // Sub-orch message: sw → subcoord (default route). Workers must supply status. subMsg := parseHeraMsgID(t, callResult(t, doRequest(t, s, "tools/call", ToolCallParams{ Name: "hera_send", - Arguments: json.RawMessage(fmt.Sprintf(`{"cwd":%q,"body":"sub body","tldr":"sub-tldr"}`, subWorkerWt)), + Arguments: json.RawMessage(fmt.Sprintf(`{"cwd":%q,"body":"sub body","tldr":"sub-tldr","status":"working"}`, subWorkerWt)), }))) resp := doRequest(t, s, "tools/call", ToolCallParams{ @@ -999,7 +999,7 @@ func TestHera_TreeUpdates_CursorAutoAdvanceAndExplicitSince(t *testing.T) { // A new message then appears. m2 := parseHeraMsgID(t, callResult(t, doRequest(t, s, "tools/call", ToolCallParams{ Name: "hera_send", - Arguments: json.RawMessage(fmt.Sprintf(`{"cwd":%q,"body":"b2","tldr":"t2"}`, subWorkerWt)), + Arguments: json.RawMessage(fmt.Sprintf(`{"cwd":%q,"body":"b2","tldr":"t2","status":"working"}`, subWorkerWt)), }))) // Explicit since=0 overrides the stored cursor (returns both) WITHOUT advancing it. @@ -1051,10 +1051,10 @@ func TestHera_GetMessages_SubtreeAccess(t *testing.T) { s, d := testHeraServer(t) coordWt, _, subWorkerWt := setupNestedOrch(t, s, d) - // Message inside the child orchestrator. + // Message inside the child orchestrator (worker must supply status). subMsg := parseHeraMsgID(t, callResult(t, doRequest(t, s, "tools/call", ToolCallParams{ Name: "hera_send", - Arguments: json.RawMessage(fmt.Sprintf(`{"cwd":%q,"body":"child secret","tldr":"child"}`, subWorkerWt)), + Arguments: json.RawMessage(fmt.Sprintf(`{"cwd":%q,"body":"child secret","tldr":"child","status":"working"}`, subWorkerWt)), }))) // Unrelated orchestrator + message. @@ -1070,7 +1070,7 @@ func TestHera_GetMessages_SubtreeAccess(t *testing.T) { }) otherMsg := parseHeraMsgID(t, callResult(t, doRequest(t, s, "tools/call", ToolCallParams{ Name: "hera_send", - Arguments: json.RawMessage(fmt.Sprintf(`{"cwd":%q,"body":"other secret","tldr":"other"}`, otherWorkerTask.Worktree)), + Arguments: json.RawMessage(fmt.Sprintf(`{"cwd":%q,"body":"other secret","tldr":"other","status":"working"}`, otherWorkerTask.Worktree)), }))) // Root coordinator fetches both: child accessible, unrelated denied. @@ -1155,12 +1155,12 @@ func TestHera_Join_ClaimMode_UnreadCount(t *testing.T) { }`, wt.Worktree)), }) - // Worker sends 2 messages. + // Worker sends 2 messages. Workers must supply status (D1). for i := 0; i < 2; i++ { doRequest(t, s, "tools/call", ToolCallParams{ Name: "hera_send", Arguments: json.RawMessage(fmt.Sprintf(`{ - "cwd":%q,"body":"msg","tldr":"t%d" + "cwd":%q,"body":"msg","tldr":"t%d","status":"working" }`, wt.Worktree, i)), }) } From 3c43b7e3aa557a6cb775ed9297a48360fc82117d Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 01:42:34 -0700 Subject: [PATCH 04/14] Phase A: gater explicit-failed gating + held-state re-arm + recovery notice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/heragater/heragater.go | 133 +++++++++++++++++++++- internal/heragater/heragater_test.go | 160 ++++++++++++++++++++++++++- 2 files changed, 290 insertions(+), 3 deletions(-) diff --git a/internal/heragater/heragater.go b/internal/heragater/heragater.go index 45586975..c5dc8495 100644 --- a/internal/heragater/heragater.go +++ b/internal/heragater/heragater.go @@ -175,7 +175,9 @@ func (w *Watcher) Tick() { w.logf("[heragater] list planned nodes: %v", err) return } + plannedByID := make(map[int64]*db.HeraRole, len(planned)) for _, node := range planned { + plannedByID[node.ID] = node state, failedBlocker := w.classify(node) switch state { case stateReady: @@ -186,6 +188,124 @@ func (w *Watcher) Tick() { w.holdAndPing(node, failedBlocker) } } + w.rearmHeldPings(plannedByID) +} + +// rearmHeldPings sweeps the held-ping dedup (D4) so a held node is re-reported +// after its blocker recovers then re-fails, and announces a recovery exactly +// once. For each (node, blocker) key currently marked: +// +// - If the NODE is no longer a planned node (it materialized — gained a binding +// — or was archived/removed), delete the key SILENTLY. An already-running +// node's reopened blocker is physics, not actionable (Non-Goals): no notice. +// - Else recompute the blocker's outcome. If it is still blockerFailed, keep the +// key (the hold stands). Otherwise clear it: +// - If it cleared because the blocker RECOVERED (the blocker role still exists +// and its outcome is now working or done), emit EXACTLY ONE "unblocked" +// notice to the coordinator, sent FROM the held node's own role (same ping +// seam holdAndPing uses, so the self-send guard never trips). +// - If it cleared because the edge/role VANISHED (the blocker role is gone, or +// the edge was removed so the blocker no longer gates this node), no notice. +// +// After a key is cleared, a later re-failure re-arms naturally: holdAndPing sets +// the key again the next time the node is held. +func (w *Watcher) rearmHeldPings(plannedByID map[int64]*db.HeraRole) { + w.mu.Lock() + keys := make([][2]int64, 0, len(w.heldPings)) + for k := range w.heldPings { + keys = append(keys, k) + } + w.mu.Unlock() + + for _, key := range keys { + nodeID, blockerID := key[0], key[1] + node, stillPlanned := plannedByID[nodeID] + if !stillPlanned { + // Materialized or removed — un-spawnable, no notice (Non-Goals). + w.clearHeldKey(key) + w.logf("[heragater] re-arm: cleared held key (node %d, blocker %d) — node no longer planned (silent)", nodeID, blockerID) + continue + } + if w.blockerOutcome(blockerID) == blockerFailed { + continue // hold stands; key kept + } + // No longer failed → clear. Distinguish recovered vs vanished: a RECOVERED + // blocker still gates this node (edge present) and its role still exists. + recovered := w.blockerRecovered(nodeID, blockerID) + w.clearHeldKey(key) + if recovered { + w.emitRecoveryNotice(node, blockerID) + continue + } + w.logf("[heragater] re-arm: cleared held key (node %d, blocker %d) — blocker edge/role vanished (no notice)", nodeID, blockerID) + } +} + +// blockerRecovered reports whether a cleared held key cleared because the blocker +// genuinely RECOVERED (its role still exists, it still gates the node via an +// extant edge, and its outcome is now working or done) versus the edge/role +// having VANISHED. Distinguishing the two is what gates the recovery notice: only +// a real recovery is actionable; a removed edge/role is not. +func (w *Watcher) blockerRecovered(nodeID, blockerID int64) bool { + // Role must still exist. + if _, err := w.db.HeraRole(blockerID); err != nil { + return false + } + // Edge must still gate this node. + blockerIDs, err := w.db.HeraBlockersOf(nodeID) + if err != nil { + return false + } + stillEdge := false + for _, bid := range blockerIDs { + if bid == blockerID { + stillEdge = true + break + } + } + if !stillEdge { + return false + } + switch w.blockerOutcome(blockerID) { + case blockerWorking, blockerDone: + return true + default: + return false + } +} + +// clearHeldKey deletes one (node, blocker) entry from the dedup map. +func (w *Watcher) clearHeldKey(key [2]int64) { + w.mu.Lock() + delete(w.heldPings, key) + w.mu.Unlock() +} + +// emitRecoveryNotice sends a one-time "unblocked" notice to the coordinator that +// a held node's blocker has recovered, sent FROM the held node's own role (same +// self-send-safe ping seam as holdAndPing). A delivery failure is logged but the +// key stays cleared — the hold is genuinely gone, and re-failure re-arms anyway; +// we do not resurrect the dedup just to retry an advisory recovery notice. +func (w *Watcher) emitRecoveryNotice(node *db.HeraRole, blockerID int64) { + coords, err := w.db.ListHeraRolesByKind(node.OrchestratorID, db.HeraKindCoordinator) + if err != nil || len(coords) == 0 { + w.logf("[heragater] recovery %d: no coordinator to notify: %v", node.ID, err) + return + } + blockerName := "" + if r, rErr := w.db.HeraRole(blockerID); rErr == nil { + blockerName = r.Name + } + body := "Planned node " + node.Name + " is UNBLOCKED: its previously-failed blocker " + blockerName + + " has recovered. It will materialize once all its blockers reach role-status done." + tldr := "unblocked: " + node.Name + " blocker " + blockerName + " recovered" + if w.ping != nil { + if pErr := w.ping(node.ID, coords[0].ID, body, tldr); pErr != nil { + w.logf("[heragater] recovery %d: notify coordinator failed (key stays cleared): %v", node.ID, pErr) + return + } + } + w.logf("[heragater] recovery: node %d (%s) unblocked — blocker %s recovered; notified coordinator", node.ID, node.Name, blockerName) } // classify computes a node's state from its blockers, returning the failing @@ -249,8 +369,17 @@ const ( // pending), and "failed" once its session has ended (task no longer in_progress // AND not pending, or the binding has been ended) without ever reaching done. func (w *Watcher) blockerOutcome(blockerID int64) blockerOutcome { - if st, err := w.db.HeraRoleStatusFor(blockerID); err == nil && st.Status == db.HeraStatusDone { - return blockerDone + if st, err := w.db.HeraRoleStatusFor(blockerID); err == nil { + switch st.Status { + case db.HeraStatusDone: + return blockerDone + case db.HeraStatusFailed: + // Explicit self-declared defeat (D2 gating half). A worker that reports + // `failed` holds its dependents IMMEDIATELY — no need to wait for its + // session to end. This takes precedence over the session-death inference + // path below (which only catches crashes / silent give-up). + return blockerFailed + } } // Not done. A COORDINATOR never reaches role-status done — its session is alive // for the whole orchestration (BUG-003). An alive coordinator blocker must NOT diff --git a/internal/heragater/heragater_test.go b/internal/heragater/heragater_test.go index 4eb18588..5133d140 100644 --- a/internal/heragater/heragater_test.go +++ b/internal/heragater/heragater_test.go @@ -27,7 +27,9 @@ type gaterFixture struct { } type ping struct { + from int64 coord int64 + body string tldr string } @@ -62,7 +64,7 @@ func newGaterFixture(t *testing.T) *gaterFixture { if f.pingFail { return errors.New("ping boom") } - f.pings = append(f.pings, ping{coord: coordRoleID, tldr: tldr}) + f.pings = append(f.pings, ping{from: fromRoleID, coord: coordRoleID, body: body, tldr: tldr}) return nil }, ) @@ -85,6 +87,12 @@ func (f *gaterFixture) pingCount() int { return len(f.pings) } +func (f *gaterFixture) lastPing() ping { + f.mu.Lock() + defer f.mu.Unlock() + return f.pings[len(f.pings)-1] +} + // seedCoord creates an orchestrator + a coordinator role+binding (so a coord // exists to be pinged) and returns the orchestrator id. func (f *gaterFixture) seedCoord(t *testing.T, orchName string) int64 { @@ -553,6 +561,156 @@ func TestGater_BlockerHavingNodeBaseUnchanged(t *testing.T) { testutil.Equal(t, f.matBranch[node.ID], "argus/1a") } +// TestGater_ExplicitFailedBlockerHoldsWithoutSessionEnd covers the D2 gating +// half: a blocker that self-declares role-status `failed` while its session is +// STILL LIVE (task in_progress, binding live) holds its dependent immediately — +// the gater does not wait for the session to end. This exercises the explicit +// failed branch in blockerOutcome, which takes precedence over the session-death +// inference path. +func TestGater_ExplicitFailedBlockerHoldsWithoutSessionEnd(t *testing.T) { + f := newGaterFixture(t) + orch := f.seedCoord(t, "orch") + // Live session (in_progress, live binding) but the worker self-reports failed. + failed := f.boundWorker(t, orch, "1a", model.StatusInProgress, db.HeraStatusFailed) + node := f.planned(t, orch, "2a") + testutil.NoError(t, f.d.AddHeraBlock(node.ID, failed.ID)) + + f.w.Tick() + + testutil.Equal(t, len(f.materialized()), 0) // held, not materialized + testutil.Equal(t, f.pingCount(), 1) + testutil.Equal(t, strings.Contains(f.lastPing().tldr, "held"), true) + // The blocker's session never ended — its task is still in_progress. + bt, err := f.d.HeraLiveBindingByRole(failed.ID) + testutil.NoError(t, err) + tk, err := f.d.Get(bt.ArgusTaskID) + testutil.NoError(t, err) + testutil.Equal(t, tk.Status, model.StatusInProgress) +} + +// TestGater_PlannedDependentReWaitsWhenBlockerReopens covers "Planned dependents +// re-wait when a blocker reopens": a done blocker returns to working before the +// dependent materialized; the gater reads the current status and keeps the +// dependent planned (it does not materialize off the stale done). +func TestGater_PlannedDependentReWaitsWhenBlockerReopens(t *testing.T) { + f := newGaterFixture(t) + orch := f.seedCoord(t, "orch") + blocker := f.boundWorker(t, orch, "1a", model.StatusInProgress, db.HeraStatusDone) + node := f.planned(t, orch, "2a") + testutil.NoError(t, f.d.AddHeraBlock(node.ID, blocker.ID)) + + // Blocker reopens to working BEFORE the dependent materializes. + testutil.NoError(t, f.d.UpsertHeraRoleStatus(blocker.ID, db.HeraStatusWorking)) + + f.w.Tick() + + testutil.Equal(t, len(f.materialized()), 0) // does not materialize off stale done + testutil.Equal(t, f.pingCount(), 0) // working blocker → planned, not held + + planned, err := f.d.ListHeraPlannedNodes() + testutil.NoError(t, err) + found := false + for _, p := range planned { + if p.ID == node.ID { + found = true + } + } + testutil.Equal(t, found, true) +} + +// TestGater_HeldDedupClearsOnRecoveryAndReArms covers the re-arm contract: a held +// node behind a failed blocker pings once; when the blocker recovers the dedup +// clears (and one recovery notice fires); a subsequent re-failure pings AGAIN. +func TestGater_HeldDedupClearsOnRecoveryAndReArms(t *testing.T) { + f := newGaterFixture(t) + orch := f.seedCoord(t, "orch") + blocker := f.boundWorker(t, orch, "1a", model.StatusInProgress, db.HeraStatusFailed) + node := f.planned(t, orch, "2a") + testutil.NoError(t, f.d.AddHeraBlock(node.ID, blocker.ID)) + + f.w.Tick() // held → ping #1 + testutil.Equal(t, f.pingCount(), 1) + testutil.Equal(t, strings.Contains(f.lastPing().tldr, "held"), true) + + // Recover: blocker flips back to working. The re-arm sweep clears the dedup and + // emits exactly one recovery notice. + testutil.NoError(t, f.d.UpsertHeraRoleStatus(blocker.ID, db.HeraStatusWorking)) + f.w.Tick() // recovery notice → ping #2 (unblocked) + testutil.Equal(t, f.pingCount(), 2) + testutil.Equal(t, strings.Contains(f.lastPing().tldr, "unblocked"), true) + + // A tick with no change emits nothing more (notice was one-time). + f.w.Tick() + testutil.Equal(t, f.pingCount(), 2) + + // Re-fail: the dedup re-armed, so this pings AGAIN (held). + testutil.NoError(t, f.d.UpsertHeraRoleStatus(blocker.ID, db.HeraStatusFailed)) + f.w.Tick() // held → ping #3 + testutil.Equal(t, f.pingCount(), 3) + testutil.Equal(t, strings.Contains(f.lastPing().tldr, "held"), true) +} + +// TestGater_RecoveryNoticeAddressedToCoordinatorFromNode covers "Recovery clears +// the dedup and notifies once": EXACTLY ONE unblocked notice, addressed to the +// coordinator role and sent FROM the held node's own role (self-send-safe). +func TestGater_RecoveryNoticeAddressedToCoordinatorFromNode(t *testing.T) { + f := newGaterFixture(t) + orch := f.seedCoord(t, "orch") + coord := f.coordRole(t, orch) + blocker := f.boundWorker(t, orch, "1a", model.StatusInProgress, db.HeraStatusFailed) + node := f.planned(t, orch, "2a") + testutil.NoError(t, f.d.AddHeraBlock(node.ID, blocker.ID)) + + f.w.Tick() // held ping + testutil.Equal(t, f.pingCount(), 1) + + // Recover. + testutil.NoError(t, f.d.UpsertHeraRoleStatus(blocker.ID, db.HeraStatusDone)) + f.w.Tick() + + testutil.Equal(t, f.pingCount(), 2) // exactly one recovery notice on top of the hold + recovery := f.lastPing() + testutil.Equal(t, strings.Contains(recovery.tldr, "unblocked"), true) + testutil.Equal(t, recovery.coord, coord.ID) // addressed to the coordinator + testutil.Equal(t, recovery.from, node.ID) // sent from the held node's own role + // Never to itself (self-send guard would reject from==to). + testutil.Equal(t, recovery.from != recovery.coord, true) +} + +// TestGater_NoRecoveryNoticeForMaterializedNode covers the Non-Goals case: a node +// that ALREADY MATERIALIZED (has a binding) whose blocker later reopens gets NO +// notice — a running worker cannot be un-spawned. The held key is cleared +// silently when the node leaves the planned set. +func TestGater_NoRecoveryNoticeForMaterializedNode(t *testing.T) { + f := newGaterFixture(t) + orch := f.seedCoord(t, "orch") + blocker := f.boundWorker(t, orch, "1a", model.StatusInProgress, db.HeraStatusFailed) + node := f.planned(t, orch, "2a") + testutil.NoError(t, f.d.AddHeraBlock(node.ID, blocker.ID)) + + f.w.Tick() // node held behind failed blocker → ping #1 + testutil.Equal(t, f.pingCount(), 1) + + // Materialize the node manually (give it a binding) so it leaves the planned + // set — simulating the coordinator force-spawning it past the hold. + tk := newRunningTask(node.Name) + testutil.NoError(t, f.d.Add(tk)) + _, err := f.d.CreateHeraBinding(db.CreateHeraBindingInput{ + RoleID: node.ID, OrchestratorID: node.OrchestratorID, + ArgusTaskID: tk.ID, WorktreePath: "/wt/" + node.Name, + }) + testutil.NoError(t, err) + + // Now flip the blocker around (recover then re-fail). Because the node is no + // longer planned, the re-arm sweep clears the key SILENTLY — zero new pings. + testutil.NoError(t, f.d.UpsertHeraRoleStatus(blocker.ID, db.HeraStatusWorking)) + f.w.Tick() + testutil.NoError(t, f.d.UpsertHeraRoleStatus(blocker.ID, db.HeraStatusFailed)) + f.w.Tick() + + testutil.Equal(t, f.pingCount(), 1) // no recovery notice, no re-held ping +} + func TestGater_StartStop(t *testing.T) { f := newGaterFixture(t) orch := f.seedCoord(t, "orch") From f38d371bf7e8c3136cf747f8fb77bba4f2aee1ae Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 01:46:21 -0700 Subject: [PATCH 05/14] Phase A: tick tasks.md (stages 1-4 complete) --- .../changes/make-hera-plan-living/tasks.md | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/openspec/changes/make-hera-plan-living/tasks.md b/openspec/changes/make-hera-plan-living/tasks.md index 81429501..16a7174e 100644 --- a/openspec/changes/make-hera-plan-living/tasks.md +++ b/openspec/changes/make-hera-plan-living/tasks.md @@ -13,41 +13,41 @@ coordinator with branch + sha; do not push or open PRs directly. Write failing tests from the `hera-messaging` and `hera-coordination` deltas. -- [ ] 1.1 `internal/db`: tests that `hera_role_status` accepts and round-trips `failed` (schema CHECK widened) — `UpsertHeraRoleStatus`/`HeraRoleStatusFor`. -- [ ] 1.2 `internal/mcp` (hera_send): failing tests — worker→coord send with no `status` errors; `status` applied synchronously to the sender role before return; `status=done` rolls the worker task to in_review + ready_to_close; `status=failed` rolls to in_review WITHOUT ready_to_close; coordinator send needs no status. -- [ ] 1.3 `internal/mcp` (hera_status): failing tests — `failed` is accepted; invalid status names all five values; worker `failed` roll (in_review, no ready_to_close). -- [ ] 1.4 `internal/heragater`: failing tests — a blocker with role-status `failed` holds the dependent + pings (explicit, before session death); a planned dependent re-waits when a done blocker returns to working; held dedup clears on blocker recovery and re-pings on re-failure; exactly one "unblocked" notice on recovery; no notice for an already-materialized node whose blocker reopens. -- [ ] 1.5 `internal/tui/hera` + `internal/tui/widget`: failing test that a role with status `failed` renders the red ✕ glyph at the right precedence (below ready_to_close, distinct from done). -- [ ] 1.6 Confirm every Phase-A `it should X` acceptance criterion in design.md has a failing test (Prove-It). +- [x] 1.1 `internal/db`: tests that `hera_role_status` accepts and round-trips `failed` (schema CHECK widened) — `UpsertHeraRoleStatus`/`HeraRoleStatusFor`. +- [x] 1.2 `internal/mcp` (hera_send): failing tests — worker→coord send with no `status` errors; `status` applied synchronously to the sender role before return; `status=done` rolls the worker task to in_review + ready_to_close; `status=failed` rolls to in_review WITHOUT ready_to_close; coordinator send needs no status. +- [x] 1.3 `internal/mcp` (hera_status): failing tests — `failed` is accepted; invalid status names all five values; worker `failed` roll (in_review, no ready_to_close). +- [x] 1.4 `internal/heragater`: failing tests — a blocker with role-status `failed` holds the dependent + pings (explicit, before session death); a planned dependent re-waits when a done blocker returns to working; held dedup clears on blocker recovery and re-pings on re-failure; exactly one "unblocked" notice on recovery; no notice for an already-materialized node whose blocker reopens. +- [x] 1.5 `internal/tui/hera` + `internal/tui/widget`: failing test that a role with status `failed` renders the red ✕ glyph at the right precedence (below ready_to_close, distinct from done). +- [x] 1.6 Confirm every Phase-A `it should X` acceptance criterion in design.md has a failing test (Prove-It). ## 2. Phase A — failed role-status (schema + store + rail glyph) **Depends on:** Stage 1 -- [ ] 2.1 `internal/db/schema.go`: widen the `hera_role_status.status` CHECK to include `'failed'`; add the `HeraStatusFailed` constant in `internal/db/hera.go`. -- [ ] 2.2 `internal/tui/widget/rolestatusicon.go`: add a `Failed` input + a red ✕ glyph case, placed below `ReadyToClose`/`NeedsInput` and distinct from `Done`; wire `roleStatusInputs` in `internal/tui/hera/rail.go` to set it from `role.Status == HeraStatusFailed`. -- [ ] 2.3 uxlog: log failed-status transitions where other status transitions are logged. +- [x] 2.1 `internal/db/schema.go`: widen the `hera_role_status.status` CHECK to include `'failed'`; add the `HeraStatusFailed` constant in `internal/db/hera.go`. +- [x] 2.2 `internal/tui/widget/rolestatusicon.go`: add a `Failed` input + a red ✕ glyph case, placed below `ReadyToClose`/`NeedsInput` and distinct from `Done`; wire `roleStatusInputs` in `internal/tui/hera/rail.go` to set it from `role.Status == HeraStatusFailed`. +- [x] 2.3 uxlog: log failed-status transitions where other status transitions are logged. ## 3. Phase A — required synchronous send-status **Depends on:** Stage 2 -- [ ] 3.1 `internal/mcp/hera.go`: add the `status` parameter to the `hera_send` tool def + handler; require it for worker/freelance senders (error naming the five values); ignore-or-accept for coordinators. -- [ ] 3.2 Apply the sent status synchronously in the handler via the shared upsert-and-roll path (reuse the `hera_status` logic / `RollHeraWorkerToReview` family) BEFORE returning; soft-fail so a roll error never blocks the send. -- [ ] 3.3 Add the `failed` worker roll sibling: roll to in_review WITHOUT `ready_to_close` (in_progress-gated, worker-kind-only, idempotent, soft-fail). Factor with `RollHeraWorkerToReview` so the invariants can't drift. -- [ ] 3.4 `internal/mcp/hera.go` (hera_status): accept `failed`; route the worker `failed` roll through the same sibling. -- [ ] 3.5 uxlog on the send-status apply (success + soft-fail). +- [x] 3.1 `internal/mcp/hera.go`: add the `status` parameter to the `hera_send` tool def + handler; require it for worker/freelance senders (error naming the five values); ignore-or-accept for coordinators. +- [x] 3.2 Apply the sent status synchronously in the handler via the shared upsert-and-roll path (reuse the `hera_status` logic / `RollHeraWorkerToReview` family) BEFORE returning; soft-fail so a roll error never blocks the send. +- [x] 3.3 Add the `failed` worker roll sibling: roll to in_review WITHOUT `ready_to_close` (in_progress-gated, worker-kind-only, idempotent, soft-fail). Factor with `RollHeraWorkerToReview` so the invariants can't drift. +- [x] 3.4 `internal/mcp/hera.go` (hera_status): accept `failed`; route the worker `failed` roll through the same sibling. +- [x] 3.5 uxlog on the send-status apply (success + soft-fail). ## 4. Phase A — gater failure gating, re-arm, recovery notice **Depends on:** Stage 2 -- [ ] 4.1 `internal/heragater/heragater.go` `blockerOutcome`: return `blockerFailed` directly when `HeraRoleStatusFor(blocker).Status == failed`, BEFORE the session-death inference path. -- [ ] 4.2 Re-arm: each tick, for every `(node, blocker)` key in `heldPings`, clear it when the blocker's outcome is no longer `blockerFailed` (recovered, or edge/role gone). -- [ ] 4.3 Recovery notice: when a key clears because the blocker RECOVERED, emit exactly one "unblocked: node X's blocker Y recovered" message to the coordinator (FROM the held node's role, reusing the `ping` seam). -- [ ] 4.4 Confirm no notice path exists for an already-materialized node whose blocker reopens (assert via test). -- [ ] 4.5 uxlog on re-arm clears and recovery notices. -- [ ] 4.6 Full Phase-A verification: `go test ./internal/db/... ./internal/mcp/... ./internal/heragater/... ./internal/tui/...` green; linter 0 issues. Report Phase A green to coordinator. +- [x] 4.1 `internal/heragater/heragater.go` `blockerOutcome`: return `blockerFailed` directly when `HeraRoleStatusFor(blocker).Status == failed`, BEFORE the session-death inference path. +- [x] 4.2 Re-arm: each tick, for every `(node, blocker)` key in `heldPings`, clear it when the blocker's outcome is no longer `blockerFailed` (recovered, or edge/role gone). +- [x] 4.3 Recovery notice: when a key clears because the blocker RECOVERED, emit exactly one "unblocked: node X's blocker Y recovered" message to the coordinator (FROM the held node's role, reusing the `ping` seam). +- [x] 4.4 Confirm no notice path exists for an already-materialized node whose blocker reopens (assert via test). +- [x] 4.5 uxlog on re-arm clears and recovery notices. +- [x] 4.6 Full Phase-A verification: `go test ./internal/db/... ./internal/mcp/... ./internal/heragater/... ./internal/tui/...` green; linter 0 issues. Report Phase A green to coordinator. ## 5. Phase B — Tests (mutation verbs) From 1352c4237cdcb767bd09ae2a26ef620b4c2b6b6f Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 01:53:35 -0700 Subject: [PATCH 06/14] Phase B: cancelled_at column + RemoveHeraBlock + planned-node edit + gater cancelled-is-non-gating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - schema.go: add cancelled_at TEXT + idx_hera_roles_cancelled to hera_roles; add idempotent ALTER TABLE hera_roles ADD COLUMN cancelled_at TEXT migration (same pattern as nuked_at/base_branch) so pre-existing DBs migrate automatically - hera.go: add CancelledAt *time.Time to HeraRole struct; update scanHeraRole to scan 11 columns (cancelled_at last); update all five SELECT column lists on hera_roles to include cancelled_at (ListHeraRoles, ListHeraRolesByKind, heraRoleByID, heraRoleByActiveName) - hera_plan.go: ListHeraPlannedNodes adds AND cancelled_at IS NULL; add RemoveHeraBlock (idempotent DELETE), UpdateHeraPlannedNode (prompt + optional project, preserves project on empty string), CancelHeraPlannedNode (COALESCE idempotent cancelled_at stamp); uxlog via slog on each mutation - heragater.go: blockerOutcome checks CancelledAt != nil FIRST, returning blockerDone — a cancelled planned blocker is treated as satisfied so its dependents can materialize; placed before the never-bound/working inference to prevent a cancelled never-bound node from blocking dependents forever Tests: 11 new db tests (RemoveHeraBlock x3, UpdateHeraPlannedNode x2, CancelHeraPlannedNode x4, migration round-trip); 3 new gater tests (CancelledNodeNeverMaterializes, DependentOfCancelledBlockerBecomesReady, CancelledBlockerAmongMultiple); migration test extended with cancelled_at column 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/db/hera.go | 17 ++- internal/db/hera_plan.go | 63 +++++++++- internal/db/hera_plan_test.go | 164 +++++++++++++++++++++++++++ internal/db/hera_test.go | 5 + internal/db/schema.go | 22 +++- internal/heragater/heragater.go | 9 ++ internal/heragater/heragater_test.go | 74 ++++++++++++ 7 files changed, 342 insertions(+), 12 deletions(-) diff --git a/internal/db/hera.go b/internal/db/hera.go index 01281228..50c54aa1 100644 --- a/internal/db/hera.go +++ b/internal/db/hera.go @@ -152,6 +152,9 @@ type HeraOrchestrator struct { // NodeKind is the plan-node discriminator (add-hera-subcoord-nodes): only // meaningful on planned roles (no binding ever); defaults to HeraNodeKindWorker // when the DB column is NULL or absent. +// CancelledAt (make-hera-plan-living) is non-nil when the coordinator has +// cancelled a planned node; the node is kept in the DB but excluded from +// materialization and treated as non-blocking by the gater. type HeraRole struct { ID int64 OrchestratorID int64 @@ -164,6 +167,7 @@ type HeraRole struct { ArchivedAt *time.Time PinnedAt *time.Time NukedAt *time.Time + CancelledAt *time.Time } // HeraBinding is one (role, argus task) incarnation. OrchestratorID is @@ -447,7 +451,7 @@ func (d *DB) ListHeraRoles(orchID int64, includeArchived bool) ([]*HeraRole, err // Nuked roles (Tier-2 EOL) are invisible to the rail-feeding list regardless // of includeArchived — recoverable only by id lookup (HeraRole). - query := `SELECT id, orchestrator_id, name, kind, argus_project, prompt, created_at, archived_at, pinned_at, nuked_at, node_kind + query := `SELECT id, orchestrator_id, name, kind, argus_project, prompt, created_at, archived_at, pinned_at, nuked_at, node_kind, cancelled_at FROM hera_roles WHERE orchestrator_id=? AND nuked_at IS NULL` if !includeArchived { query += ` AND archived_at IS NULL` @@ -478,7 +482,7 @@ func (d *DB) ListHeraRolesByKind(orchID int64, kind HeraRoleKind) ([]*HeraRole, defer d.mu.Unlock() rows, err := d.conn.Query( - `SELECT id, orchestrator_id, name, kind, argus_project, prompt, created_at, archived_at, pinned_at, nuked_at, node_kind + `SELECT id, orchestrator_id, name, kind, argus_project, prompt, created_at, archived_at, pinned_at, nuked_at, node_kind, cancelled_at FROM hera_roles WHERE orchestrator_id=? AND kind=? AND archived_at IS NULL ORDER BY name ASC`, orchID, string(kind)) if err != nil { @@ -586,14 +590,14 @@ func (d *DB) DeleteHeraRole(id int64) error { func (d *DB) heraRoleByID(id int64) (*HeraRole, error) { row := d.conn.QueryRow( - `SELECT id, orchestrator_id, name, kind, argus_project, prompt, created_at, archived_at, pinned_at, nuked_at, node_kind + `SELECT id, orchestrator_id, name, kind, argus_project, prompt, created_at, archived_at, pinned_at, nuked_at, node_kind, cancelled_at FROM hera_roles WHERE id=?`, id) return scanHeraRole(row) } func (d *DB) heraRoleByActiveName(orchID int64, name string) (*HeraRole, error) { row := d.conn.QueryRow( - `SELECT id, orchestrator_id, name, kind, argus_project, prompt, created_at, archived_at, pinned_at, nuked_at, node_kind + `SELECT id, orchestrator_id, name, kind, argus_project, prompt, created_at, archived_at, pinned_at, nuked_at, node_kind, cancelled_at FROM hera_roles WHERE orchestrator_id=? AND name=? AND archived_at IS NULL`, orchID, name) return scanHeraRole(row) } @@ -1231,9 +1235,9 @@ func scanHeraOrchestrator(s rowScanner) (*HeraOrchestrator, error) { func scanHeraRole(s rowScanner) (*HeraRole, error) { var r HeraRole var kind, createdAt string - var archivedAt, pinnedAt, nukedAt, nodeKind sql.NullString + var archivedAt, pinnedAt, nukedAt, nodeKind, cancelledAt sql.NullString if err := s.Scan(&r.ID, &r.OrchestratorID, &r.Name, &kind, &r.ArgusProject, &r.Prompt, - &createdAt, &archivedAt, &pinnedAt, &nukedAt, &nodeKind); err != nil { + &createdAt, &archivedAt, &pinnedAt, &nukedAt, &nodeKind, &cancelledAt); err != nil { if errors.Is(err, sql.ErrNoRows) { return nil, ErrHeraNotFound } @@ -1251,6 +1255,7 @@ func scanHeraRole(s rowScanner) (*HeraRole, error) { } else { r.NodeKind = HeraNodeKindWorker } + r.CancelledAt = nullTimePtr(cancelledAt) return &r, nil } diff --git a/internal/db/hera_plan.go b/internal/db/hera_plan.go index e83384e9..0369bbff 100644 --- a/internal/db/hera_plan.go +++ b/internal/db/hera_plan.go @@ -4,6 +4,7 @@ import ( "database/sql" "errors" "fmt" + "log/slog" "time" ) @@ -355,9 +356,9 @@ func (d *DB) ListHeraPlannedNodes() ([]*HeraRole, error) { defer d.mu.Unlock() rows, err := d.conn.Query( `SELECT r.id, r.orchestrator_id, r.name, r.kind, r.argus_project, r.prompt, - r.created_at, r.archived_at, r.pinned_at, r.nuked_at, r.node_kind + r.created_at, r.archived_at, r.pinned_at, r.nuked_at, r.node_kind, r.cancelled_at FROM hera_roles r - WHERE r.kind=? AND r.archived_at IS NULL + WHERE r.kind=? AND r.archived_at IS NULL AND r.cancelled_at IS NULL AND NOT EXISTS (SELECT 1 FROM hera_bindings b WHERE b.role_id = r.id) ORDER BY r.id ASC`, string(HeraKindWorker)) @@ -376,6 +377,64 @@ func (d *DB) ListHeraPlannedNodes() ([]*HeraRole, error) { return out, rows.Err() } +// RemoveHeraBlock removes the blocking edge (blockedRoleID waits on +// blockerRoleID). Idempotent: deleting a non-existent edge returns nil. +func (d *DB) RemoveHeraBlock(blockedRoleID, blockerRoleID int64) error { + d.mu.Lock() + defer d.mu.Unlock() + _, err := d.conn.Exec( + `DELETE FROM hera_blocks WHERE blocked_role_id=? AND blocker_role_id=?`, + blockedRoleID, blockerRoleID) + if err != nil { + return fmt.Errorf("remove hera block: %w", err) + } + // Idempotent: zero rows affected is not an error. + slog.Info(fmt.Sprintf("[hera plan] removed block edge blocked=%d blocker=%d", blockedRoleID, blockerRoleID)) + return nil +} + +// UpdateHeraPlannedNode updates the prompt and optionally the argus_project of a +// planned node. project is only updated when non-empty — an empty string +// preserves the existing value. The materialized-vs-planned guard belongs in the +// MCP layer; this function updates any role unconditionally. +func (d *DB) UpdateHeraPlannedNode(roleID int64, prompt, project string) error { + d.mu.Lock() + defer d.mu.Unlock() + var err error + if project != "" { + _, err = d.conn.Exec( + `UPDATE hera_roles SET prompt=?, argus_project=? WHERE id=?`, + prompt, project, roleID) + } else { + _, err = d.conn.Exec( + `UPDATE hera_roles SET prompt=? WHERE id=?`, + prompt, roleID) + } + if err != nil { + return fmt.Errorf("update hera planned node: %w", err) + } + slog.Info(fmt.Sprintf("[hera plan] updated planned node role=%d", roleID)) + return nil +} + +// CancelHeraPlannedNode stamps cancelled_at on a planned node. Idempotent: a +// node that is already cancelled is a no-op (cancelled_at is preserved via +// COALESCE). The node is kept in the DB for the plan view but excluded from +// materialization (ListHeraPlannedNodes adds AND cancelled_at IS NULL) and +// treated as non-blocking by the gater. +func (d *DB) CancelHeraPlannedNode(roleID int64) error { + d.mu.Lock() + defer d.mu.Unlock() + _, err := d.conn.Exec( + `UPDATE hera_roles SET cancelled_at=COALESCE(cancelled_at, ?) WHERE id=?`, + formatTime(time.Now()), roleID) + if err != nil { + return fmt.Errorf("cancel hera planned node: %w", err) + } + slog.Info(fmt.Sprintf("[hera plan] cancelled planned node role=%d", roleID)) + return nil +} + // HeraRoleHasBinding reports whether a role has EVER held a binding (live or // ended). The gater's idempotency guard: a node that already has a binding has // been (or is being) materialized and must never be materialized again. diff --git a/internal/db/hera_plan_test.go b/internal/db/hera_plan_test.go index d423d0f5..b903985b 100644 --- a/internal/db/hera_plan_test.go +++ b/internal/db/hera_plan_test.go @@ -499,3 +499,167 @@ func TestHeraRoleHasBinding(t *testing.T) { testutil.NoError(t, err) testutil.Equal(t, has, true) } + +// --- RemoveHeraBlock --- + +func TestRemoveHeraBlock_RemovesEdge(t *testing.T) { + d := testDB(t) + orch := planTestOrch(t, d, "orch") + a := plannedRole(t, d, orch, "a") + b := plannedRole(t, d, orch, "b") + testutil.NoError(t, d.AddHeraBlock(b.ID, a.ID)) + + // Verify edge exists. + blockers, err := d.HeraBlockersOf(b.ID) + testutil.NoError(t, err) + testutil.Equal(t, len(blockers), 1) + + // Remove it. + testutil.NoError(t, d.RemoveHeraBlock(b.ID, a.ID)) + + // Edge is gone. + after, err := d.HeraBlockersOf(b.ID) + testutil.NoError(t, err) + testutil.Equal(t, len(after), 0) +} + +func TestRemoveHeraBlock_IdempotentOnMissingEdge(t *testing.T) { + d := testDB(t) + orch := planTestOrch(t, d, "orch") + a := plannedRole(t, d, orch, "a") + b := plannedRole(t, d, orch, "b") + + // No edge was ever added — removing it must be a no-op (no error). + testutil.NoError(t, d.RemoveHeraBlock(b.ID, a.ID)) +} + +func TestRemoveHeraBlock_OnlyRemovesTargetEdge(t *testing.T) { + // When a node has two blockers, removing one must leave the other intact. + d := testDB(t) + orch := planTestOrch(t, d, "orch") + a := plannedRole(t, d, orch, "a") + b := plannedRole(t, d, orch, "b") + c := plannedRole(t, d, orch, "c") + testutil.NoError(t, d.AddHeraBlock(c.ID, a.ID)) + testutil.NoError(t, d.AddHeraBlock(c.ID, b.ID)) + + testutil.NoError(t, d.RemoveHeraBlock(c.ID, a.ID)) + + blockers, err := d.HeraBlockersOf(c.ID) + testutil.NoError(t, err) + testutil.DeepEqual(t, blockers, []int64{b.ID}) +} + +// --- UpdateHeraPlannedNode --- + +func TestUpdateHeraPlannedNode_UpdatesPromptAndProject(t *testing.T) { + d := testDB(t) + orch := planTestOrch(t, d, "orch") + r := plannedRole(t, d, orch, "w") + + testutil.NoError(t, d.UpdateHeraPlannedNode(r.ID, "new prompt", "new-proj")) + + got, err := d.HeraRole(r.ID) + testutil.NoError(t, err) + testutil.Equal(t, got.Prompt, "new prompt") + testutil.Equal(t, got.ArgusProject, "new-proj") +} + +func TestUpdateHeraPlannedNode_PreservesProjectOnEmpty(t *testing.T) { + // An empty project string must leave the existing project unchanged. + d := testDB(t) + orch := planTestOrch(t, d, "orch") + r := plannedRole(t, d, orch, "w") // argus_project="proj" from plannedRole helper + + testutil.NoError(t, d.UpdateHeraPlannedNode(r.ID, "new prompt", "")) + + got, err := d.HeraRole(r.ID) + testutil.NoError(t, err) + testutil.Equal(t, got.Prompt, "new prompt") + testutil.Equal(t, got.ArgusProject, "proj") // unchanged +} + +// --- CancelHeraPlannedNode --- + +func TestCancelHeraPlannedNode_StampsCancelledAt(t *testing.T) { + d := testDB(t) + orch := planTestOrch(t, d, "orch") + r := plannedRole(t, d, orch, "w") + testutil.Nil(t, r.CancelledAt) + + testutil.NoError(t, d.CancelHeraPlannedNode(r.ID)) + + got, err := d.HeraRole(r.ID) + testutil.NoError(t, err) + if got.CancelledAt == nil { + t.Fatal("expected CancelledAt to be set after cancel") + } +} + +func TestCancelHeraPlannedNode_Idempotent(t *testing.T) { + // A second cancel must preserve the original timestamp, not clobber it. + d := testDB(t) + orch := planTestOrch(t, d, "orch") + r := plannedRole(t, d, orch, "w") + + testutil.NoError(t, d.CancelHeraPlannedNode(r.ID)) + first, err := d.HeraRole(r.ID) + testutil.NoError(t, err) + if first.CancelledAt == nil { + t.Fatal("expected CancelledAt after first cancel") + } + + testutil.NoError(t, d.CancelHeraPlannedNode(r.ID)) + second, err := d.HeraRole(r.ID) + testutil.NoError(t, err) + if second.CancelledAt == nil { + t.Fatal("expected CancelledAt after second cancel") + } + + // COALESCE preserves the first timestamp — both reads return the same value. + if !first.CancelledAt.Equal(*second.CancelledAt) { + t.Fatalf("idempotent cancel changed timestamp: first=%v second=%v", first.CancelledAt, second.CancelledAt) + } +} + +func TestCancelHeraPlannedNode_ExcludedFromListHeraPlannedNodes(t *testing.T) { + // A cancelled node must NOT appear in ListHeraPlannedNodes — the gater must + // never attempt to materialize it. + d := testDB(t) + orch := planTestOrch(t, d, "orch") + active := plannedRole(t, d, orch, "active") + toCancel := plannedRole(t, d, orch, "cancelled") + + testutil.NoError(t, d.CancelHeraPlannedNode(toCancel.ID)) + + nodes, err := d.ListHeraPlannedNodes() + testutil.NoError(t, err) + testutil.Equal(t, len(nodes), 1) + testutil.Equal(t, nodes[0].ID, active.ID) +} + +func TestCancelHeraPlannedNode_StillVisibleViaByID(t *testing.T) { + // Cancelled nodes are kept in the DB (not deleted): HeraRole(id) and + // ListHeraBlocks still surface them (for the plan view). + d := testDB(t) + orch := planTestOrch(t, d, "orch") + dep := plannedRole(t, d, orch, "dep") + blocker := plannedRole(t, d, orch, "blocker") + testutil.NoError(t, d.AddHeraBlock(dep.ID, blocker.ID)) + testutil.NoError(t, d.CancelHeraPlannedNode(blocker.ID)) + + // By-id lookup still works. + got, err := d.HeraRole(blocker.ID) + testutil.NoError(t, err) + testutil.Equal(t, got.ID, blocker.ID) + if got.CancelledAt == nil { + t.Fatal("expected CancelledAt to be set") + } + + // ListHeraBlocks still surfaces the edge (plan view needs it to render the + // cancelled node in its position in the graph). + blocks, err := d.ListHeraBlocks(orch) + testutil.NoError(t, err) + testutil.Equal(t, len(blocks), 1) + testutil.Equal(t, blocks[0].BlockerRoleID, blocker.ID) +} diff --git a/internal/db/hera_test.go b/internal/db/hera_test.go index ce6f8c84..d375a6e1 100644 --- a/internal/db/hera_test.go +++ b/internal/db/hera_test.go @@ -73,6 +73,11 @@ func TestCreateHeraTables_MigratesPreNukedAtDB(t *testing.T) { // The legacy shape above omits the column, so this is a real regression guard. _, err = d.conn.Exec(`SELECT base_branch FROM hera_orchestrators`) testutil.NoError(t, err) + + // cancelled_at (make-hera-plan-living) is added by the same idempotent ALTER + // migration on a pre-existing hera_roles table that never had it. + _, err = d.conn.Exec(`SELECT cancelled_at FROM hera_roles`) + testutil.NoError(t, err) } // mkOrch creates an active orchestrator and fails the test on error. diff --git a/internal/db/schema.go b/internal/db/schema.go index 981415ac..14b40f40 100644 --- a/internal/db/schema.go +++ b/internal/db/schema.go @@ -424,6 +424,12 @@ func (d *DB) createHeraTables() error { // distinct coordinator agent. Idempotent — ignored when the column already // exists (e.g. fresh DBs where the CREATE TABLE below adds it inline). d.conn.Exec(`ALTER TABLE hera_roles ADD COLUMN node_kind TEXT`) //nolint:errcheck + // Idempotent ADD COLUMN migration for the role-level cancelled_at + // (make-hera-plan-living). Same pattern as nuked_at / base_branch above: + // additive, nullable TEXT, no backfill — existing rows read back NULL + // (active / not cancelled). Fails on a fresh DB (table not yet created) and + // is intentionally ignored; the CREATE TABLE below carries the column inline. + d.conn.Exec(`ALTER TABLE hera_roles ADD COLUMN cancelled_at TEXT`) //nolint:errcheck ddl := ` CREATE TABLE IF NOT EXISTS hera_orchestrators ( @@ -467,13 +473,21 @@ func (d *DB) createHeraTables() error { -- node_kind (add-hera-subcoord-nodes): plan-node discriminator. NULL or -- absent means leaf worker (default); "subcoord" means materialize as a -- distinct coordinator agent. Only meaningful on planned roles (no binding). - node_kind TEXT + node_kind TEXT, + -- cancelled_at (make-hera-plan-living): a planned node may be cancelled + -- by the coordinator (plan-mutation verb CancelHeraPlannedNode). A + -- cancelled node is kept in the DB (not deleted) so the plan view can + -- show it as cancelled, but it is excluded from ListHeraPlannedNodes so + -- the gater never materializes it, and it is treated as non-blocking + -- (satisfied) by the gater so its dependents can still proceed. + cancelled_at TEXT ); CREATE INDEX IF NOT EXISTS idx_hera_roles_kind ON hera_roles(orchestrator_id, kind); CREATE UNIQUE INDEX IF NOT EXISTS idx_hera_roles_active_name ON hera_roles(orchestrator_id, name) WHERE archived_at IS NULL; - CREATE INDEX IF NOT EXISTS idx_hera_roles_archived ON hera_roles(archived_at); - CREATE INDEX IF NOT EXISTS idx_hera_roles_pinned ON hera_roles(pinned_at); - CREATE INDEX IF NOT EXISTS idx_hera_roles_nuked ON hera_roles(nuked_at); + CREATE INDEX IF NOT EXISTS idx_hera_roles_archived ON hera_roles(archived_at); + CREATE INDEX IF NOT EXISTS idx_hera_roles_pinned ON hera_roles(pinned_at); + CREATE INDEX IF NOT EXISTS idx_hera_roles_nuked ON hera_roles(nuked_at); + CREATE INDEX IF NOT EXISTS idx_hera_roles_cancelled ON hera_roles(cancelled_at); CREATE TABLE IF NOT EXISTS hera_bindings ( id INTEGER PRIMARY KEY, diff --git a/internal/heragater/heragater.go b/internal/heragater/heragater.go index c5dc8495..af8ef183 100644 --- a/internal/heragater/heragater.go +++ b/internal/heragater/heragater.go @@ -369,6 +369,15 @@ const ( // pending), and "failed" once its session has ended (task no longer in_progress // AND not pending, or the binding has been ended) without ever reaching done. func (w *Watcher) blockerOutcome(blockerID int64) blockerOutcome { + // A cancelled planned node is treated as satisfied (non-blocking): its + // dependent can proceed even though the node was never materialized. This + // check runs first so a cancelled never-bound node is not mistakenly treated + // as still-working (the never-started path further below would return + // blockerWorking, but that would incorrectly block dependents forever). + if role, err := w.db.HeraRole(blockerID); err == nil && role.CancelledAt != nil { + return blockerDone + } + if st, err := w.db.HeraRoleStatusFor(blockerID); err == nil { switch st.Status { case db.HeraStatusDone: diff --git a/internal/heragater/heragater_test.go b/internal/heragater/heragater_test.go index 5133d140..2ada2d7b 100644 --- a/internal/heragater/heragater_test.go +++ b/internal/heragater/heragater_test.go @@ -588,6 +588,80 @@ func TestGater_ExplicitFailedBlockerHoldsWithoutSessionEnd(t *testing.T) { testutil.Equal(t, tk.Status, model.StatusInProgress) } +// --- Cancelled-node gater tests (make-hera-plan-living) --- + +// TestGater_CancelledNodeNeverMaterializes verifies that a cancelled planned node +// is excluded from the gater's planned set and is never materialized, even when +// it has no blockers and would otherwise be ready. +func TestGater_CancelledNodeNeverMaterializes(t *testing.T) { + f := newGaterFixture(t) + orch := f.seedCoord(t, "orch") + node := f.planned(t, orch, "1a") + + testutil.NoError(t, f.d.CancelHeraPlannedNode(node.ID)) + + f.w.Tick() + + // A cancelled node must never be spawned. + testutil.Equal(t, len(f.materialized()), 0) + testutil.Equal(t, f.pingCount(), 0) +} + +// TestGater_DependentOfCancelledBlockerBecomesReady verifies that a planned node +// whose ONLY unsatisfied blocker is cancelled is classified stateReady and +// materializes. The cancelled blocker is treated as "done" (non-blocking) so the +// dependent can proceed. +// +// The blocker is a live bound worker (in_progress, role-status working) so it +// keeps the dependent planned on the first tick. After the blocker role is +// cancelled, the dependent must materialize. +func TestGater_DependentOfCancelledBlockerBecomesReady(t *testing.T) { + f := newGaterFixture(t) + orch := f.seedCoord(t, "orch") + + // Use f.boundWorker so the blocker has a real task row in the DB. + blocker := f.boundWorker(t, orch, "1a", model.StatusInProgress, db.HeraStatusWorking) + node := f.planned(t, orch, "2a") + testutil.NoError(t, f.d.AddHeraBlock(node.ID, blocker.ID)) + + // Before cancel: node stays planned (blocker is still working). + f.w.Tick() + testutil.Equal(t, len(f.materialized()), 0) + testutil.Equal(t, f.pingCount(), 0) + + // Cancel the blocker role — its dependent's only blocker is now non-blocking. + testutil.NoError(t, f.d.CancelHeraPlannedNode(blocker.ID)) + + f.w.Tick() + + mat := f.materialized() + testutil.Equal(t, len(mat), 1) + testutil.Equal(t, mat[0].ID, node.ID) + testutil.Equal(t, f.pingCount(), 0) // no hold-ping — it was ready, not held +} + +// TestGater_CancelledBlockerAmongMultipleStillAllowsReady verifies that when a +// node has two blockers — one done and one cancelled — it is classified ready +// and materializes (both are treated as satisfied). +func TestGater_CancelledBlockerAmongMultipleStillAllowsReady(t *testing.T) { + f := newGaterFixture(t) + orch := f.seedCoord(t, "orch") + + done := f.boundWorker(t, orch, "1a", model.StatusInReview, db.HeraStatusDone) + cancelled := f.planned(t, orch, "1b") + node := f.planned(t, orch, "2a") + testutil.NoError(t, f.d.AddHeraBlock(node.ID, done.ID)) + testutil.NoError(t, f.d.AddHeraBlock(node.ID, cancelled.ID)) + + testutil.NoError(t, f.d.CancelHeraPlannedNode(cancelled.ID)) + + f.w.Tick() + + mat := f.materialized() + testutil.Equal(t, len(mat), 1) + testutil.Equal(t, mat[0].ID, node.ID) +} + // TestGater_PlannedDependentReWaitsWhenBlockerReopens covers "Planned dependents // re-wait when a blocker reopens": a done blocker returns to working before the // dependent materialized; the gater reads the current status and keeps the From fd7fa496ab8cad6438f92d21a43b3176d9c31cfd Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 02:00:54 -0700 Subject: [PATCH 07/14] Phase B: hera_plan_node_update / hera_unblock / hera_plan_node_cancel MCP verbs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three coordinator-only plan-mutation verbs (D5, make-hera-plan-living): - hera_plan_node_update: edits prompt/project on a planned node; rejects materialized nodes (HeraRoleHasBinding guard); requires at least one of prompt or project. - hera_unblock: drops a hera_blocks edge; idempotent on missing edges. - hera_plan_node_cancel: stamps cancelled_at; excludes from gater; keeps role visible for plan rendering; rejects materialized nodes. All three are guarded by heroPlanCoordinatorGuard (non-coordinator rejected). Adds HeraRoleHasBinding, UpdateHeraPlannedNode, CancelHeraPlannedNode, and RemoveHeraBlock to the HeraStore interface. Updates heraToolDefs (12→15) and the tools/list assertion. 11 new tests cover all scenarios from the spec. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/mcp/hera.go | 54 ++++++- internal/mcp/hera_plan.go | 152 ++++++++++++++++++++ internal/mcp/hera_plan_test.go | 253 +++++++++++++++++++++++++++++++++ internal/mcp/hera_test.go | 3 +- internal/mcp/server.go | 6 + 5 files changed, 465 insertions(+), 3 deletions(-) diff --git a/internal/mcp/hera.go b/internal/mcp/hera.go index aaf2b4f4..89b53aaa 100644 --- a/internal/mcp/hera.go +++ b/internal/mcp/hera.go @@ -36,6 +36,12 @@ type HeraStore interface { // by id (see db.HeraBlockSpec). CreateHeraPlan(orchID int64, nodes []db.HeraPlannedNodeSpec, edges []db.HeraBlockSpec) ([]*db.HeraRole, error) UniqueHeraRoleName(orchID int64, base string) (string, error) + // Plan-mutation verbs (make-hera-plan-living D5). Coordinator-only at the + // tool layer; the materialized-vs-planned guard lives in the MCP handlers. + HeraRoleHasBinding(roleID int64) (bool, error) + UpdateHeraPlannedNode(roleID int64, prompt, project string) error + CancelHeraPlannedNode(roleID int64) error + RemoveHeraBlock(blockedRoleID, blockerRoleID int64) error // Bindings HeraLiveBindingByTask(taskID string) (*db.HeraBinding, error) HeraLiveBindingByTaskAndOrchestrator(taskID string, orchID int64) (*db.HeraBinding, error) @@ -61,12 +67,14 @@ type HeraStore interface { RollHeraWorkerFailed(taskID string) (bool, error) } -// heraToolDefs contains the 12 hera_* tool schemas. The first 9 are ported +// heraToolDefs contains the 15 hera_* tool schemas. The first 9 are ported // verbatim from Hera's daemon.toolDefinitions() — same param names, // descriptions, and required lists as the external Hera daemon so agents have an -// identical surface when running natively. The last 3 (hera_plan_node / +// identical surface when running natively. The next 3 (hera_plan_node / // hera_block / hera_plan) are the native plan-DAG authoring tools // (add-hera-plan-substrate); they are coordinator-only like hera_spawn_worker. +// The last 3 (hera_plan_node_update / hera_unblock / hera_plan_node_cancel) are +// the plan-mutation verbs (make-hera-plan-living D5). var heraToolDefs = []Tool{ { Name: "hera_new_orchestrator", @@ -264,6 +272,48 @@ var heraToolDefs = []Tool{ "required": []string{"cwd", "nodes"}, }, }, + { + Name: "hera_plan_node_update", + Description: "Edit a PLANNED node's prompt and/or project. Coordinator-only. Rejected if the node has already materialized (prompt already delivered to a running worker). Requires at least one of prompt or project. Re-pointing a prompt before a node materializes lets a coordinator reconcile the plan to reality without cancelling and re-creating.", + InputSchema: map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "cwd": map[string]interface{}{"type": "string", "description": "Coordinator's worktree path (use $PWD)"}, + "name": map[string]interface{}{"type": "string", "description": "Name of the planned node to update"}, + "prompt": map[string]interface{}{"type": "string", "description": "(optional) New prompt to deliver when the node materializes. Preserves existing if omitted."}, + "project": map[string]interface{}{"type": "string", "description": "(optional) Override the argus project for the node. Preserves existing if omitted."}, + "orchestrator": map[string]interface{}{"type": "string", "description": "(optional) Disambiguates when the calling task holds multiple live coordinator bindings"}, + }, + "required": []string{"cwd", "name"}, + }, + }, + { + Name: "hera_unblock", + Description: "Remove a blocking edge between two roles in the orchestrator: `blocker` no longer gates `blocked`. Coordinator-only. Idempotent — removing a non-existent edge succeeds as a no-op. Re-pointing an edge is hera_unblock + hera_block; there is no separate re-point verb.", + InputSchema: map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "cwd": map[string]interface{}{"type": "string", "description": "Coordinator's worktree path (use $PWD)"}, + "blocked": map[string]interface{}{"type": "string", "description": "Name of the role that WAITS (the dependent)"}, + "blocker": map[string]interface{}{"type": "string", "description": "Name of the role whose blocking edge to remove"}, + "orchestrator": map[string]interface{}{"type": "string", "description": "(optional) Disambiguates when the calling task holds multiple live coordinator bindings"}, + }, + "required": []string{"cwd", "blocked", "blocker"}, + }, + }, + { + Name: "hera_plan_node_cancel", + Description: "Cancel a PLANNED node: stamps cancelled_at, excludes it from materialization, and unblocks its dependents (a cancelled node no longer gates them). Coordinator-only. The node is kept in the plan for visibility (renders as grey ✕). Rejected if the node has already materialized — stop a running worker via the task lifecycle instead.", + InputSchema: map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "cwd": map[string]interface{}{"type": "string", "description": "Coordinator's worktree path (use $PWD)"}, + "name": map[string]interface{}{"type": "string", "description": "Name of the planned node to cancel"}, + "orchestrator": map[string]interface{}{"type": "string", "description": "(optional) Disambiguates when the calling task holds multiple live coordinator bindings"}, + }, + "required": []string{"cwd", "name"}, + }, + }, } // callerRoleResult holds the resolved context for the calling agent's hera diff --git a/internal/mcp/hera_plan.go b/internal/mcp/hera_plan.go index d79dc9f3..7194ff2a 100644 --- a/internal/mcp/hera_plan.go +++ b/internal/mcp/hera_plan.go @@ -317,6 +317,158 @@ func (s *Server) resolvePlanEndpoint(id interface{}, caller *callerRoleResult, n return -1, role.ID, nil } +func (s *Server) toolHeraPlanNodeUpdate(id interface{}, args json.RawMessage) *Response { + if !s.heraEnabled() { + return toolError(id, "hera not configured") + } + var p struct { + Cwd string `json:"cwd"` + Name string `json:"name"` + Prompt string `json:"prompt"` + Project string `json:"project"` + Orchestrator string `json:"orchestrator"` + } + json.Unmarshal(args, &p) //nolint:errcheck + + if p.Cwd == "" { + return toolError(id, "cwd is required") + } + name := strings.TrimSpace(p.Name) + if name == "" { + return toolError(id, "name is required") + } + prompt := strings.TrimSpace(p.Prompt) + project := strings.TrimSpace(p.Project) + if prompt == "" && project == "" { + return toolError(id, "at least one of prompt or project is required") + } + + caller, errResp := s.heraPlanCoordinatorGuard(id, p.Cwd, p.Orchestrator) + if errResp != nil { + return errResp + } + + role, errResp := s.resolveOrchRole(id, caller.orch.ID, caller.orch.Name, name) + if errResp != nil { + return errResp + } + + has, err := s.heraStore.HeraRoleHasBinding(role.ID) + if err != nil { + return toolError(id, fmt.Sprintf("check materialization: %v", err)) + } + if has { + return toolError(id, fmt.Sprintf("role %q has already materialized (prompt already delivered); cannot update a running worker via the plan", name)) + } + + if err := s.heraStore.UpdateHeraPlannedNode(role.ID, prompt, project); err != nil { + return toolError(id, fmt.Sprintf("update planned node: %v", err)) + } + + slog.Info("[hera plan] plan_node_update ok", "orch", caller.orch.Name, "role", role.Name) + var b strings.Builder + fmt.Fprintf(&b, "Planned node updated.\n\n") + fmt.Fprintf(&b, "- **orchestrator**: %s\n", caller.orch.Name) + fmt.Fprintf(&b, "- **name**: %s\n", role.Name) + fmt.Fprintf(&b, "- **status**: planned (not yet materialized)\n") + return toolResult(id, b.String()) +} + +func (s *Server) toolHeraUnblock(id interface{}, args json.RawMessage) *Response { + if !s.heraEnabled() { + return toolError(id, "hera not configured") + } + var p struct { + Cwd string `json:"cwd"` + Blocked string `json:"blocked"` + Blocker string `json:"blocker"` + Orchestrator string `json:"orchestrator"` + } + json.Unmarshal(args, &p) //nolint:errcheck + + if p.Cwd == "" { + return toolError(id, "cwd is required") + } + if strings.TrimSpace(p.Blocked) == "" || strings.TrimSpace(p.Blocker) == "" { + return toolError(id, "blocked and blocker are both required") + } + + caller, errResp := s.heraPlanCoordinatorGuard(id, p.Cwd, p.Orchestrator) + if errResp != nil { + return errResp + } + + blockedRole, errResp := s.resolveOrchRole(id, caller.orch.ID, caller.orch.Name, p.Blocked) + if errResp != nil { + return errResp + } + blockerRole, errResp := s.resolveOrchRole(id, caller.orch.ID, caller.orch.Name, p.Blocker) + if errResp != nil { + return errResp + } + + if err := s.heraStore.RemoveHeraBlock(blockedRole.ID, blockerRole.ID); err != nil { + return toolError(id, fmt.Sprintf("remove block: %v", err)) + } + + slog.Info("[hera plan] unblock ok", "orch", caller.orch.Name, "blocked", blockedRole.Name, "blocker", blockerRole.Name) + var b strings.Builder + fmt.Fprintf(&b, "Blocking edge removed (idempotent — no-op if the edge did not exist).\n\n") + fmt.Fprintf(&b, "- **blocked**: %s\n", blockedRole.Name) + fmt.Fprintf(&b, "- **blocker**: %s\n", blockerRole.Name) + return toolResult(id, b.String()) +} + +func (s *Server) toolHeraPlanNodeCancel(id interface{}, args json.RawMessage) *Response { + if !s.heraEnabled() { + return toolError(id, "hera not configured") + } + var p struct { + Cwd string `json:"cwd"` + Name string `json:"name"` + Orchestrator string `json:"orchestrator"` + } + json.Unmarshal(args, &p) //nolint:errcheck + + if p.Cwd == "" { + return toolError(id, "cwd is required") + } + name := strings.TrimSpace(p.Name) + if name == "" { + return toolError(id, "name is required") + } + + caller, errResp := s.heraPlanCoordinatorGuard(id, p.Cwd, p.Orchestrator) + if errResp != nil { + return errResp + } + + role, errResp := s.resolveOrchRole(id, caller.orch.ID, caller.orch.Name, name) + if errResp != nil { + return errResp + } + + has, err := s.heraStore.HeraRoleHasBinding(role.ID) + if err != nil { + return toolError(id, fmt.Sprintf("check materialization: %v", err)) + } + if has { + return toolError(id, fmt.Sprintf("role %q has already materialized; stop the running worker via the task lifecycle instead", name)) + } + + if err := s.heraStore.CancelHeraPlannedNode(role.ID); err != nil { + return toolError(id, fmt.Sprintf("cancel planned node: %v", err)) + } + + slog.Info("[hera plan] plan_node_cancel ok", "orch", caller.orch.Name, "role", role.Name) + var b strings.Builder + fmt.Fprintf(&b, "Planned node cancelled.\n\n") + fmt.Fprintf(&b, "- **orchestrator**: %s\n", caller.orch.Name) + fmt.Fprintf(&b, "- **name**: %s\n", role.Name) + fmt.Fprintf(&b, "- **status**: cancelled (kept in plan for visibility; will not materialize; no longer gates dependents)\n") + return toolResult(id, b.String()) +} + // heraBlockErrMessage maps the store's block sentinels to agent-facing messages. func heraBlockErrMessage(err error) string { switch { diff --git a/internal/mcp/hera_plan_test.go b/internal/mcp/hera_plan_test.go index e7ac63fa..72d86bcf 100644 --- a/internal/mcp/hera_plan_test.go +++ b/internal/mcp/hera_plan_test.go @@ -238,6 +238,259 @@ func TestHeraPlan_NonCoordinatorRejected(t *testing.T) { testutil.Contains(t, cr.Content[0].Text, "only coordinators may author the plan") } +// --- hera_plan_node_update --- + +func TestHeraPlanNodeUpdate_EditsPrompt(t *testing.T) { + s, d := testHeraServer(t) + coord := seedCoordinator(t, s, d, "orch", "/wt/coord") + planNode(t, s, coord.Worktree, "1a-writer", "original prompt") + + orch, _ := d.HeraOrchestratorByName("orch") + role, _ := d.HeraRoleByName(orch.ID, "1a-writer") + testutil.Equal(t, role.Prompt, "original prompt") + + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_plan_node_update", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd": %q, "name": "1a-writer", "prompt": "revised prompt" + }`, coord.Worktree)), + }) + testutil.NoError(t, respErr(resp)) + cr := callResult(t, resp) + testutil.Equal(t, cr.IsError, false) + testutil.Contains(t, cr.Content[0].Text, "Planned node updated") + + updated, _ := d.HeraRoleByName(orch.ID, "1a-writer") + testutil.Equal(t, updated.Prompt, "revised prompt") +} + +func TestHeraPlanNodeUpdate_EditsProject(t *testing.T) { + s, d := testHeraServer(t) + coord := seedCoordinator(t, s, d, "orch", "/wt/coord") + planNode(t, s, coord.Worktree, "1a-writer", "some prompt") + + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_plan_node_update", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd": %q, "name": "1a-writer", "project": "new-project" + }`, coord.Worktree)), + }) + testutil.NoError(t, respErr(resp)) + cr := callResult(t, resp) + testutil.Equal(t, cr.IsError, false) + + orch, _ := d.HeraOrchestratorByName("orch") + updated, _ := d.HeraRoleByName(orch.ID, "1a-writer") + testutil.Equal(t, updated.ArgusProject, "new-project") +} + +func TestHeraPlanNodeUpdate_RejectsMaterialized(t *testing.T) { + s, d := testHeraServer(t) + coord := seedCoordinator(t, s, d, "orch", "/wt/coord") + // Attach a worker with a binding (simulates a materialized node). + orch, _ := d.HeraOrchestratorByName("orch") + workerTask := addHeraTestTask(t, d, "/wt/w1") + _, _, err := d.CreateHeraRoleWithBinding(db.CreateHeraRoleInput{ + OrchestratorID: orch.ID, + Name: "1a-live", + Kind: db.HeraKindWorker, + ArgusProject: "proj", + Prompt: "original", + }, workerTask.ID, "/wt/w1") + testutil.NoError(t, err) + + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_plan_node_update", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd": %q, "name": "1a-live", "prompt": "new prompt" + }`, coord.Worktree)), + }) + testutil.NoError(t, respErr(resp)) + cr := callResult(t, resp) + testutil.Equal(t, cr.IsError, true) + testutil.Contains(t, cr.Content[0].Text, "already materialized") +} + +func TestHeraPlanNodeUpdate_RejectsNonCoordinator(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "orch", "/wt/coord") + worker := attachWorkerTask(t, s, d, "orch", "/wt/w", "w1") + + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_plan_node_update", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd": %q, "name": "anything", "prompt": "p" + }`, worker.Worktree)), + }) + testutil.NoError(t, respErr(resp)) + cr := callResult(t, resp) + testutil.Equal(t, cr.IsError, true) + testutil.Contains(t, cr.Content[0].Text, "only coordinators may author the plan") +} + +func TestHeraPlanNodeUpdate_RejectsEmptyMutation(t *testing.T) { + s, d := testHeraServer(t) + coord := seedCoordinator(t, s, d, "orch", "/wt/coord") + planNode(t, s, coord.Worktree, "1a-writer", "original") + + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_plan_node_update", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd": %q, "name": "1a-writer" + }`, coord.Worktree)), + }) + testutil.NoError(t, respErr(resp)) + cr := callResult(t, resp) + testutil.Equal(t, cr.IsError, true) + testutil.Contains(t, cr.Content[0].Text, "at least one of prompt or project") +} + +// --- hera_unblock --- + +func TestHeraUnblock_DropsExistingEdge(t *testing.T) { + s, d := testHeraServer(t) + coord := seedCoordinator(t, s, d, "orch", "/wt/coord") + planNode(t, s, coord.Worktree, "a", "do a") + planNode(t, s, coord.Worktree, "b", "do b") + // Add block edge: b waits on a. + doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_block", + Arguments: json.RawMessage(fmt.Sprintf(`{"cwd": %q, "blocked": "b", "blocker": "a"}`, coord.Worktree)), + }) + + orch, _ := d.HeraOrchestratorByName("orch") + b, _ := d.HeraRoleByName(orch.ID, "b") + blockersBefore, _ := d.HeraBlockersOf(b.ID) + testutil.Equal(t, len(blockersBefore), 1) + + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_unblock", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd": %q, "blocked": "b", "blocker": "a" + }`, coord.Worktree)), + }) + testutil.NoError(t, respErr(resp)) + cr := callResult(t, resp) + testutil.Equal(t, cr.IsError, false) + testutil.Contains(t, cr.Content[0].Text, "Blocking edge removed") + + blockersAfter, _ := d.HeraBlockersOf(b.ID) + testutil.Equal(t, len(blockersAfter), 0) +} + +func TestHeraUnblock_IdempotentOnMissingEdge(t *testing.T) { + s, d := testHeraServer(t) + coord := seedCoordinator(t, s, d, "orch", "/wt/coord") + planNode(t, s, coord.Worktree, "a", "do a") + planNode(t, s, coord.Worktree, "b", "do b") + // No block edge added — unblock should still succeed. + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_unblock", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd": %q, "blocked": "b", "blocker": "a" + }`, coord.Worktree)), + }) + testutil.NoError(t, respErr(resp)) + cr := callResult(t, resp) + testutil.Equal(t, cr.IsError, false) +} + +func TestHeraUnblock_RejectsNonCoordinator(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "orch", "/wt/coord") + worker := attachWorkerTask(t, s, d, "orch", "/wt/w", "w1") + + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_unblock", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd": %q, "blocked": "b", "blocker": "a" + }`, worker.Worktree)), + }) + testutil.NoError(t, respErr(resp)) + cr := callResult(t, resp) + testutil.Equal(t, cr.IsError, true) + testutil.Contains(t, cr.Content[0].Text, "only coordinators may author the plan") +} + +// --- hera_plan_node_cancel --- + +func TestHeraPlanNodeCancel_CancelsPlannedNode(t *testing.T) { + s, d := testHeraServer(t) + coord := seedCoordinator(t, s, d, "orch", "/wt/coord") + planNode(t, s, coord.Worktree, "1a-worker", "do work") + + // Node is in the planned set before cancel. + plansBefore, _ := d.ListHeraPlannedNodes() + testutil.Equal(t, len(plansBefore), 1) + + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_plan_node_cancel", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd": %q, "name": "1a-worker" + }`, coord.Worktree)), + }) + testutil.NoError(t, respErr(resp)) + cr := callResult(t, resp) + testutil.Equal(t, cr.IsError, false) + testutil.Contains(t, cr.Content[0].Text, "Planned node cancelled") + + // After cancel the node must be excluded from the planned set (gater-side). + plansAfter, _ := d.ListHeraPlannedNodes() + testutil.Equal(t, len(plansAfter), 0) + + // But the role must still exist in the DB (kept for plan visibility). + orch, _ := d.HeraOrchestratorByName("orch") + role, err := d.HeraRoleByName(orch.ID, "1a-worker") + testutil.NoError(t, err) + if role.CancelledAt == nil { + t.Fatal("expected cancelled_at to be set after cancel") + } +} + +func TestHeraPlanNodeCancel_RejectsMaterialized(t *testing.T) { + s, d := testHeraServer(t) + coord := seedCoordinator(t, s, d, "orch", "/wt/coord") + // Attach a worker with a binding (simulates a materialized node). + orch, _ := d.HeraOrchestratorByName("orch") + workerTask := addHeraTestTask(t, d, "/wt/w2") + _, _, err := d.CreateHeraRoleWithBinding(db.CreateHeraRoleInput{ + OrchestratorID: orch.ID, + Name: "1b-live", + Kind: db.HeraKindWorker, + ArgusProject: "proj", + Prompt: "prompt", + }, workerTask.ID, "/wt/w2") + testutil.NoError(t, err) + + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_plan_node_cancel", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd": %q, "name": "1b-live" + }`, coord.Worktree)), + }) + testutil.NoError(t, respErr(resp)) + cr := callResult(t, resp) + testutil.Equal(t, cr.IsError, true) + testutil.Contains(t, cr.Content[0].Text, "already materialized") +} + +func TestHeraPlanNodeCancel_RejectsNonCoordinator(t *testing.T) { + s, d := testHeraServer(t) + seedCoordinator(t, s, d, "orch", "/wt/coord") + worker := attachWorkerTask(t, s, d, "orch", "/wt/w", "w1") + + resp := doRequest(t, s, "tools/call", ToolCallParams{ + Name: "hera_plan_node_cancel", + Arguments: json.RawMessage(fmt.Sprintf(`{ + "cwd": %q, "name": "anything" + }`, worker.Worktree)), + }) + testutil.NoError(t, respErr(resp)) + cr := callResult(t, resp) + testutil.Equal(t, cr.IsError, true) + testutil.Contains(t, cr.Content[0].Text, "only coordinators may author the plan") +} + // TestHeraPlan_EdgeReferencingExistingRole confirms an edge endpoint resolves // against a pre-existing orchestrator role (not just nodes in this call). func TestHeraPlan_EdgeReferencingExistingRole(t *testing.T) { diff --git a/internal/mcp/hera_test.go b/internal/mcp/hera_test.go index f76278b6..f7f24424 100644 --- a/internal/mcp/hera_test.go +++ b/internal/mcp/hera_test.go @@ -141,12 +141,13 @@ func TestToolsList_HeraOn(t *testing.T) { names[tool.Name] = true } - // All 12 hera tools must appear (9 ported + 3 plan-authoring). + // All 15 hera tools must appear (9 ported + 3 plan-authoring + 3 plan-mutation). for _, want := range []string{ "hera_new_orchestrator", "hera_join", "hera_send", "hera_inbox", "hera_mark_read", "hera_status", "hera_spawn_worker", "hera_tree_updates", "hera_get_messages", "hera_plan_node", "hera_block", "hera_plan", + "hera_plan_node_update", "hera_unblock", "hera_plan_node_cancel", } { if !names[want] { t.Errorf("hera tool missing from tools/list: %s", want) diff --git a/internal/mcp/server.go b/internal/mcp/server.go index ba33cd76..b372b54e 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -920,6 +920,12 @@ func (s *Server) handleToolsCall(req *Request) *Response { return s.toolHeraBlock(req.ID, params.Arguments) case "hera_plan": return s.toolHeraPlan(req.ID, params.Arguments) + case "hera_plan_node_update": + return s.toolHeraPlanNodeUpdate(req.ID, params.Arguments) + case "hera_unblock": + return s.toolHeraUnblock(req.ID, params.Arguments) + case "hera_plan_node_cancel": + return s.toolHeraPlanNodeCancel(req.ID, params.Arguments) default: // Plugin-registered tool? PR 4 — dispatch into the registry which // HTTP-POSTs to the plugin's callback_url and returns the response From 930df9bb133e4135d8b353151f01a62fc16dccfb Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 02:05:44 -0700 Subject: [PATCH 08/14] Phase B: fix project-only plan-node update wiping the prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UpdateHeraPlannedNode built a static SET clause for the project!='' branch that always included prompt=?, overwriting the stored prompt with empty when the caller only supplied a project. Now the SET clause is built dynamically: prompt=? and argus_project=? are appended only when the respective input is non-empty, so each field is preserved independently. New db-level tests assert: - project-only update preserves existing prompt (regression case) - both-fields update sets both Strengthened MCP-level TestHeraPlanNodeUpdate_EditsProject to assert the prompt survived the project-only edit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/db/hera_plan.go | 37 ++++++++++++++++++++++------------ internal/db/hera_plan_test.go | 30 +++++++++++++++++++++++++++ internal/mcp/hera_plan_test.go | 2 ++ 3 files changed, 56 insertions(+), 13 deletions(-) diff --git a/internal/db/hera_plan.go b/internal/db/hera_plan.go index 0369bbff..eafa80a2 100644 --- a/internal/db/hera_plan.go +++ b/internal/db/hera_plan.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "strings" "time" ) @@ -393,24 +394,34 @@ func (d *DB) RemoveHeraBlock(blockedRoleID, blockerRoleID int64) error { return nil } -// UpdateHeraPlannedNode updates the prompt and optionally the argus_project of a -// planned node. project is only updated when non-empty — an empty string -// preserves the existing value. The materialized-vs-planned guard belongs in the -// MCP layer; this function updates any role unconditionally. +// UpdateHeraPlannedNode updates the prompt and/or argus_project of a planned +// node. Each field is only updated when non-empty — an empty string preserves +// the existing value (so a project-only edit never wipes the prompt, and a +// prompt-only edit never wipes the project). Both empty is a no-op (the MCP +// layer rejects that case before calling here, but the store is safe either +// way). The materialized-vs-planned guard belongs in the MCP layer; this +// function updates any role unconditionally. func (d *DB) UpdateHeraPlannedNode(roleID int64, prompt, project string) error { d.mu.Lock() defer d.mu.Unlock() - var err error + setClauses := make([]string, 0, 2) + args := make([]interface{}, 0, 3) + if prompt != "" { + setClauses = append(setClauses, "prompt=?") + args = append(args, prompt) + } if project != "" { - _, err = d.conn.Exec( - `UPDATE hera_roles SET prompt=?, argus_project=? WHERE id=?`, - prompt, project, roleID) - } else { - _, err = d.conn.Exec( - `UPDATE hera_roles SET prompt=? WHERE id=?`, - prompt, roleID) + setClauses = append(setClauses, "argus_project=?") + args = append(args, project) } - if err != nil { + if len(setClauses) == 0 { + // Nothing to update — no-op. + return nil + } + args = append(args, roleID) + //nolint:gosec // G202: setClauses contains only hard-coded column=? literals; no user input is concatenated. + query := "UPDATE hera_roles SET " + strings.Join(setClauses, ", ") + " WHERE id=?" + if _, err := d.conn.Exec(query, args...); err != nil { return fmt.Errorf("update hera planned node: %w", err) } slog.Info(fmt.Sprintf("[hera plan] updated planned node role=%d", roleID)) diff --git a/internal/db/hera_plan_test.go b/internal/db/hera_plan_test.go index b903985b..f7841a24 100644 --- a/internal/db/hera_plan_test.go +++ b/internal/db/hera_plan_test.go @@ -579,6 +579,36 @@ func TestUpdateHeraPlannedNode_PreservesProjectOnEmpty(t *testing.T) { testutil.Equal(t, got.ArgusProject, "proj") // unchanged } +func TestUpdateHeraPlannedNode_PreservesPromptOnProjectOnly(t *testing.T) { + // A project-only update (empty prompt) must NOT wipe the existing prompt. + // This is the regression for the bug where the project-branch ran + // SET prompt=?, argus_project=? with the empty prompt, overwriting the stored value. + d := testDB(t) + orch := planTestOrch(t, d, "orch") + r := plannedRole(t, d, orch, "w") // prompt="do w", argus_project="proj" + + testutil.NoError(t, d.UpdateHeraPlannedNode(r.ID, "", "new-proj")) + + got, err := d.HeraRole(r.ID) + testutil.NoError(t, err) + testutil.Equal(t, got.Prompt, "do w") // must be unchanged + testutil.Equal(t, got.ArgusProject, "new-proj") // updated +} + +func TestUpdateHeraPlannedNode_BothFieldsUpdated(t *testing.T) { + // When both prompt and project are supplied, both are updated. + d := testDB(t) + orch := planTestOrch(t, d, "orch") + r := plannedRole(t, d, orch, "w") + + testutil.NoError(t, d.UpdateHeraPlannedNode(r.ID, "revised prompt", "revised-proj")) + + got, err := d.HeraRole(r.ID) + testutil.NoError(t, err) + testutil.Equal(t, got.Prompt, "revised prompt") + testutil.Equal(t, got.ArgusProject, "revised-proj") +} + // --- CancelHeraPlannedNode --- func TestCancelHeraPlannedNode_StampsCancelledAt(t *testing.T) { diff --git a/internal/mcp/hera_plan_test.go b/internal/mcp/hera_plan_test.go index 72d86bcf..f973427e 100644 --- a/internal/mcp/hera_plan_test.go +++ b/internal/mcp/hera_plan_test.go @@ -282,6 +282,8 @@ func TestHeraPlanNodeUpdate_EditsProject(t *testing.T) { orch, _ := d.HeraOrchestratorByName("orch") updated, _ := d.HeraRoleByName(orch.ID, "1a-writer") testutil.Equal(t, updated.ArgusProject, "new-project") + // Regression: a project-only edit must NOT wipe the existing prompt. + testutil.Equal(t, updated.Prompt, "some prompt") } func TestHeraPlanNodeUpdate_RejectsMaterialized(t *testing.T) { From 2194d3e55f33019bdbe917f63113984389a276aa Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 02:12:45 -0700 Subject: [PATCH 09/14] =?UTF-8?q?Phase=20B:=20render=20cancelled=20plan=20?= =?UTF-8?q?nodes=20(grey=20=E2=9C=95)=20+=20README=20verb=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add StateCancelled to planview.State enum with grey ✕ glyph (ColorDimmed) and distinct style from StateFailed (red ✕, bold). Updates all three switch statements (Glyph/Label/style) and the groupCounts legend list. - Add Cancelled bool to RoleView; set from role.CancelledAt != nil in buildRoleView alongside the existing Planned discriminator. - planNodeState: check Cancelled before Planned so cancelled nodes render StateCancelled (grey ✕) even when they were never materialized. - planNodeIcon: return nil for StateCancelled (State overlay handles rendering). - README: document hera_plan, hera_plan_node, hera_block, hera_plan_node_update, hera_unblock, hera_plan_node_cancel in the Reference MCP tools table. - Tests: render_test.go covers glyph/label/style distinctness + chip draw + groupCounts inclusion. plan_test.go covers cancelled projection to StateCancelled, node list retention (not dropped), Cancelled-wins-Planned priority, and nil Icon for cancelled nodes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- README.md | 3 + internal/tui/hera/model.go | 11 ++++ internal/tui/hera/plan.go | 17 ++++-- internal/tui/hera/plan_test.go | 85 ++++++++++++++++++++++++++++ internal/tui/planview/planview.go | 38 +++++++++++-- internal/tui/planview/render_test.go | 62 ++++++++++++++++++++ 6 files changed, 204 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 7e48e4b7..44a1cbfe 100644 --- a/README.md +++ b/README.md @@ -426,6 +426,9 @@ If the recipient has a live agent session the daemon also writes a single notifi | `hera_plan_node` | Author a single planned node under the caller's orchestrator (coordinator-only). Params: `name`, `kind` (`worker`\|`subcoord`, default `worker`), `prompt` (worker nodes) or `goal` (subcoord nodes — required; the goal handed to the spawned coordinator). A `subcoord` node materializes as a distinct coordinator agent with its own task, worktree, and child orchestrator. | | `hera_block` | Add a blocking edge: `blocked` waits until `blocker` reaches role-status `done`. Coordinator-only; both roles must be in the same orchestrator. No cycles. | | `hera_plan` | Author an entire plan graph in one call: a `nodes` array (each with `name`, `kind`, `prompt`/`goal`, optional `project`) and an `edges` array (`blocked`→`blocker` pairs). Coordinator-only; atomically creates all nodes then all edges. Supports mixed `kind` values in the same graph. | +| `hera_plan_node_update` | Edit a planned node's `prompt` and/or `project` before it materializes. Rejected after materialization. | +| `hera_unblock` | Remove a blocking edge between two roles. Idempotent. Re-pointing an edge is `hera_unblock` + `hera_block`. | +| `hera_plan_node_cancel` | Cancel a planned node: stamps `cancelled_at`, excludes it from materialization, unblocks dependents. Kept visible in the plan DAG as grey ✕. | **Schedule Management:** diff --git a/internal/tui/hera/model.go b/internal/tui/hera/model.go index 49cf53f3..3d07beaa 100644 --- a/internal/tui/hera/model.go +++ b/internal/tui/hera/model.go @@ -73,6 +73,13 @@ type RoleView struct { // NOT planned. Stage 2/3 populates this in buildRoleView. Planned bool + // Cancelled is true when the coordinator has explicitly cancelled this planned + // node (cancelled_at is set). A cancelled node is excluded from gater + // materialization but remains visible in the plan DAG, rendered as grey ✕ + // (StateCancelled). Cancelled wins over Planned in the plan projection + // (make-hera-plan-living B3). + Cancelled bool + // Prompt is the role's verbatim delivery prompt (persisted on the hera role at // plan/spawn time). The plan view's master-detail header shows its first line // as the node Description (D9). Empty when the role carries no prompt. @@ -887,5 +894,9 @@ func buildRoleView(r HeraReader, role *db.HeraRole, roleToBinding map[int64]*db. // A bound-but-finished role keeps Live==false but carries a BridgeTaskID, so it // is NOT planned — the gater never re-materializes it. rv.Planned = role.Kind == db.HeraKindWorker && !rv.Live && rv.BridgeTaskID == "" + // Cancelled discriminator (make-hera-plan-living B3): coordinator stamped + // cancelled_at on this planned node. Cancelled wins over Planned in the plan + // projection (renders grey ✕ instead of violet ○). + rv.Cancelled = role.CancelledAt != nil return rv } diff --git a/internal/tui/hera/plan.go b/internal/tui/hera/plan.go index de7bd4d8..974c3b82 100644 --- a/internal/tui/hera/plan.go +++ b/internal/tui/hera/plan.go @@ -112,11 +112,16 @@ func planNodeID(r *RoleView) string { return fmt.Sprintf("plan:%d", r.RoleID) } -// planNodeState maps a role to its render state (D7). A planned (never-bound) -// role is StatePlanned (violet ○). A live/finished role colours from its bound -// argus task status + result, with the {"failed":true} result winning over the -// workflow status (red ✕) — reusing coordTaskFailed (details.go), not a third copy. +// planNodeState maps a role to its render state (D7). Cancelled wins over all +// other discriminators — a cancelled planned node renders grey ✕ (StateCancelled) +// regardless of its other fields. A planned (never-bound) role is StatePlanned +// (violet ○). A live/finished role colours from its bound argus task status + +// result, with the {"failed":true} result winning over the workflow status (red ✕) +// — reusing coordTaskFailed (details.go), not a third copy. func planNodeState(r *RoleView) planview.State { + if r.Cancelled { + return planview.StateCancelled + } if r.Planned { return planview.StatePlanned } @@ -155,8 +160,8 @@ func planNodeState(r *RoleView) planview.State { // the spinner actually won, with zero duplication of the classifier's precedence // (and it tracks the active spinner style, which both sides read). See BUG-012. func planNodeIcon(r *RoleView, state planview.State) *planview.NodeIcon { - if state == planview.StatePlanned || state == planview.StateFailed { - return nil // ○ / ✕ overlays come from State (the rail has neither) + if state == planview.StatePlanned || state == planview.StateFailed || state == planview.StateCancelled { + return nil // ○ / ✕ / cancelled ✕ overlays come from State (the rail has no concept of these) } in := roleStatusInputs(r) glyph, style := widget.RoleStatusIcon(in, false, 0) diff --git a/internal/tui/hera/plan_test.go b/internal/tui/hera/plan_test.go index fe85c239..eaa94399 100644 --- a/internal/tui/hera/plan_test.go +++ b/internal/tui/hera/plan_test.go @@ -1,6 +1,7 @@ package hera import ( + "fmt" "testing" "github.com/drn/argus/internal/db" @@ -461,3 +462,87 @@ func TestRefresh_DifferentCoordinatorResetsPlanCursor(t *testing.T) { testutil.Equal(t, selectOrchByName(p, "orch-b"), true) testutil.Equal(t, pl.CursorPos().Stage, 0) } + +// --- Cancelled planned node rendering (make-hera-plan-living B3) --- + +// TestRoleViewCancelled_SetFromCancelledAt: a role whose CancelledAt is set +// projects Cancelled=true in the RoleView. +func TestRoleViewCancelled_SetFromCancelledAt(t *testing.T) { + d := memDB(t) + orch := seedOrch(t, d, "orch") + seedBoundRole(t, d, orch, "coord", db.HeraKindCoordinator, "t-coord") + r := seedPlannedRole(t, d, orch, "1a-to-cancel") + testutil.NoError(t, d.CancelHeraPlannedNode(r.ID)) + + ov := orchViewByName(t, d, "orch") + testutil.Equal(t, ov != nil, true) + found := false + for _, rv := range ov.Roles { + if rv.RoleID == r.ID { + found = true + testutil.Equal(t, rv.Cancelled, true) + } + } + testutil.Equal(t, found, true) +} + +// TestHeraPlanNodes_CancelledProjectsStateCancelled: a cancelled planned node +// (CancelledAt set) projects State=StateCancelled and remains in the node list +// (NOT omitted — it stays visible in the plan DAG as grey ✕). +func TestHeraPlanNodes_CancelledProjectsStateCancelled(t *testing.T) { + d := memDB(t) + orch := seedOrch(t, d, "orch") + seedBoundRole(t, d, orch, "coord", db.HeraKindCoordinator, "t-coord") + cancelled := seedPlannedRole(t, d, orch, "1a-cancelled") + active := seedPlannedRole(t, d, orch, "2a-active") + testutil.NoError(t, d.CancelHeraPlannedNode(cancelled.ID)) + + ov := orchViewByName(t, d, "orch") + testutil.Equal(t, ov != nil, true) + nodes, _ := heraPlanNodes(ov) + + // Both nodes appear (cancelled is visible, not dropped). + nc, okC := findNode(nodes, fmt.Sprintf("plan:%d", cancelled.ID)) + na, okA := findNode(nodes, fmt.Sprintf("plan:%d", active.ID)) + testutil.Equal(t, okC, true) + testutil.Equal(t, okA, true) + + // Cancelled node → StateCancelled; active planned node → StatePlanned. + testutil.Equal(t, nc.State, planview.StateCancelled) + testutil.Equal(t, na.State, planview.StatePlanned) +} + +// TestHeraPlanNodes_CancelledWinsOverPlanned: a node that is both planned +// (never materialized) AND cancelled projects StateCancelled, not StatePlanned. +// This pins the priority order: Cancelled > Planned. +func TestHeraPlanNodes_CancelledWinsOverPlanned(t *testing.T) { + d := memDB(t) + orch := seedOrch(t, d, "orch") + seedBoundRole(t, d, orch, "coord", db.HeraKindCoordinator, "t-coord") + r := seedPlannedRole(t, d, orch, "1a-cancel-planned") + testutil.NoError(t, d.CancelHeraPlannedNode(r.ID)) + + ov := orchViewByName(t, d, "orch") + testutil.Equal(t, ov != nil, true) + + // Confirm the RoleView carries Planned=true AND Cancelled=true (double flag). + for _, rv := range ov.Roles { + if rv.RoleID == r.ID { + testutil.Equal(t, rv.Planned, true) + testutil.Equal(t, rv.Cancelled, true) + } + } + + nodes, _ := heraPlanNodes(ov) + n, ok := findNode(nodes, fmt.Sprintf("plan:%d", r.ID)) + testutil.Equal(t, ok, true) + // Cancelled wins — renders StateCancelled, NOT StatePlanned. + testutil.Equal(t, n.State, planview.StateCancelled) +} + +// TestPlanNodeIcon_CancelledUsesStateOverlay: a cancelled node leaves Icon nil +// so the widget renders the State overlay (grey ✕) rather than an Icon glyph. +func TestPlanNodeIcon_CancelledUsesStateOverlay(t *testing.T) { + icon := projectWorkerIcon(t, RoleView{RoleID: 2, Name: "w-cancelled", Cancelled: true}) + testutil.Nil(t, icon) +} diff --git a/internal/tui/planview/planview.go b/internal/tui/planview/planview.go index 02693b7c..a10054a2 100644 --- a/internal/tui/planview/planview.go +++ b/internal/tui/planview/planview.go @@ -45,6 +45,11 @@ const ( StateFailed // StatePending is a live-but-not-yet-progressing node. StatePending + // StateCancelled is a planned node that was explicitly cancelled by the + // coordinator (grey ✕). It renders distinctly from StateFailed (red ✕) — + // same glyph, different colour. Cancelled nodes remain visible in the plan + // DAG but are excluded from materialization (make-hera-plan-living B3). + StateCancelled ) // Glyph returns the one-rune state indicator drawn inside a chip. @@ -58,6 +63,8 @@ func (s State) Glyph() rune { return '◔' case StateFailed: return '✕' + case StateCancelled: + return '✕' case StatePending: return '·' default: // StatePlanned @@ -77,6 +84,8 @@ func (s State) Label() string { return "in review" case StateFailed: return "failed" + case StateCancelled: + return "cancelled" case StatePending: return "pending" default: // StatePlanned @@ -97,6 +106,8 @@ func (s State) style() tcell.Style { return base.Foreground(theme.ColorInReview) case StateFailed: return base.Foreground(theme.ColorError).Bold(true) + case StateCancelled: + return base.Foreground(theme.ColorDimmed) case StatePending: return base.Foreground(theme.ColorPending) default: // StatePlanned — violet @@ -2072,6 +2083,21 @@ func put(screen tcell.Screen, clip clipRect, x, y int, s string, style tcell.Sty } } +// groupCounts renders the aggregate per-state counts for a collapsed group box, +// e.g. "3 ✓ · 2 ⟳ · 1 ○". States with a zero count are omitted; the order +// follows the State enum for stability. Retained as a flat-string helper (and +// for TestGroupCounts_IncludesCancelled); the live render path uses +// groupCountSegs for per-segment colour + spinner animation (BUG-011). +func groupCounts(g *Group) string { + var parts []string + for _, s := range []State{StateDone, StateInReview, StateWorking, StatePending, StateFailed, StateCancelled, StatePlanned} { + if c := g.Counts[s]; c > 0 { + parts = append(parts, fmt.Sprintf("%d %c", c, s.Glyph())) + } + } + return strings.Join(parts, " · ") +} + // countSeg is one styled run of the collapsed-group count line: either a // " " segment in its per-state colour or a dim " · " separator // (BUG-011). Painting per-segment (rather than a single flat string) is what @@ -2086,9 +2112,9 @@ type countSeg struct { // the compact State.Glyph() set (◔/⟳), it synthesises a widget.RoleStatusInputs // from the State and calls the SHARED classifier widget.RoleStatusIcon — the same // fn the rail's statusIcon and the node's planNodeIcon use — so the count can -// never drift from the rail. The two plan-only overlays the rail has no concept -// of (planned ○ / failed ✕) fall back to the State glyph. The working segment -// animates via the live spinner frame. +// never drift from the rail. The three plan-only overlays the rail has no concept +// of (planned ○ / failed ✕ / cancelled ✕) fall back to the State glyph. The +// working segment animates via the live spinner frame. // // Only the GLYPH comes from the classifier; the COLOUR is the caller's // State.style() (the per-state node-border colour). The classifier's @@ -2096,8 +2122,8 @@ type countSeg struct { // in_review cyan, so glyph and colour are sourced separately on purpose. func countSegGlyph(s State, frame int) rune { switch s { - case StatePlanned, StateFailed: - return s.Glyph() // ○ / ✕ overlays — the rail has neither + case StatePlanned, StateFailed, StateCancelled: + return s.Glyph() // ○ / ✕ / ✕ overlays — the rail has neither } var in widget.RoleStatusInputs switch s { @@ -2123,7 +2149,7 @@ func countSegGlyph(s State, frame int) rune { // the frame from w.animFrame each Draw (layout runs per Draw) so it animates. func (w *Widget) groupCountSegs(g *Group) []countSeg { var segs []countSeg - for _, s := range []State{StateDone, StateInReview, StateWorking, StatePending, StateFailed, StatePlanned} { + for _, s := range []State{StateDone, StateInReview, StateWorking, StatePending, StateFailed, StateCancelled, StatePlanned} { c := g.Counts[s] if c == 0 { continue diff --git a/internal/tui/planview/render_test.go b/internal/tui/planview/render_test.go index cd252cb6..d58f1472 100644 --- a/internal/tui/planview/render_test.go +++ b/internal/tui/planview/render_test.go @@ -625,6 +625,68 @@ func TestDraw_HScroll_NoIndicatorsWhenFits(t *testing.T) { testutil.Contains(t, out, "w1") } +// TestState_Glyph_Cancelled: StateCancelled carries the same ✕ glyph as +// StateFailed — the COLOUR is the discriminator (grey vs red). +func TestState_Glyph_Cancelled(t *testing.T) { + testutil.Equal(t, StateCancelled.Glyph(), '✕') +} + +// TestState_Label_Cancelled: StateCancelled labels itself "cancelled". +func TestState_Label_Cancelled(t *testing.T) { + testutil.Equal(t, StateCancelled.Label(), "cancelled") +} + +// TestState_Style_CancelledIsGrey_DistinctFromFailed: StateCancelled uses +// ColorDimmed (grey) and StateFailed uses ColorError (red) — same ✕ glyph, +// different colour, so they are visually distinct. +func TestState_Style_CancelledIsGrey_DistinctFromFailed(t *testing.T) { + cfg, _, _ := StateCancelled.style().Decompose() + ffg, _, _ := StateFailed.style().Decompose() + testutil.Equal(t, cfg, theme.ColorDimmed) + testutil.Equal(t, ffg, theme.ColorError) + testutil.Equal(t, cfg == ffg, false) +} + +// TestDraw_CancelledChipGlyphRendered: a node with State=StateCancelled renders +// the ✕ glyph and the node's short-id label, and is distinct from StateFailed +// (which uses a red bold style vs the dimmed grey of cancelled). +func TestDraw_CancelledChipGlyphRendered(t *testing.T) { + w := New() + w.SetData([]Node{ + {ID: "1a", Name: "1a-cancelled", State: StateCancelled}, + {ID: "2a", Name: "2a-failed", State: StateFailed}, + }, []Edge{{From: "1a", To: "2a"}}) + + sc := drawToSim(t, w, 60, 18) + cells, scw, _ := sc.GetContents() + + // Both nodes render a ✕ glyph. + cx, cy, okC := findStringCell(sc, "✕ 1a") + testutil.Equal(t, okC, true) + fx, fy, okF := findStringCell(sc, "✕ 2a") + testutil.Equal(t, okF, true) + + // The cancelled cell uses grey (ColorDimmed), not red. + cancelFG, _, _ := cells[cy*scw+cx].Style.Decompose() + testutil.Equal(t, cancelFG, theme.ColorDimmed) + + // The failed cell uses red (ColorError), not grey. + failFG, _, _ := cells[fy*scw+fx].Style.Decompose() + testutil.Equal(t, failFG, theme.ColorError) + + // They must be visually distinct — different colours. + testutil.Equal(t, cancelFG == failFG, false) +} + +// TestGroupCounts_IncludesCancelled: groupCounts includes a non-zero +// StateCancelled count as a grey ✕ entry in the aggregate line. +func TestGroupCounts_IncludesCancelled(t *testing.T) { + g := &Group{Counts: map[State]int{StateDone: 2, StateCancelled: 1}} + got := groupCounts(g) + testutil.Contains(t, got, "2 ✓") + testutil.Contains(t, got, "1 ✕") +} + func TestTruncateLabel_RuneAware(t *testing.T) { // A long name truncates to fallbackLabelRunes with an ellipsis, rune-counted. long := "verylongrolenamehere" From 4747708463b8d53420c4497a7cb77c0653f2a762 Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 02:17:44 -0700 Subject: [PATCH 10/14] Phase B: tick tasks.md (stages 5-8 complete) --- .../changes/make-hera-plan-living/tasks.md | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/openspec/changes/make-hera-plan-living/tasks.md b/openspec/changes/make-hera-plan-living/tasks.md index 16a7174e..758167a5 100644 --- a/openspec/changes/make-hera-plan-living/tasks.md +++ b/openspec/changes/make-hera-plan-living/tasks.md @@ -56,41 +56,41 @@ Write failing tests from the `hera-messaging` and `hera-coordination` deltas. Write failing tests from the `hera-coordination` (mutation-verb + cancelled) and `hera-view` (cancelled rendering) deltas. -- [ ] 5.1 `internal/db`: failing tests for `RemoveHeraBlock(blocked, blocker)` (removes one edge; missing edge is a no-op), `SetHeraRolePrompt`/`UpdateHeraPlannedNode` (edits prompt/project), and the `cancelled_at` column (`CancelHeraPlannedNode`; `ListHeraPlannedNodes` excludes cancelled; `ListHeraBlocks`/render still surface it). -- [ ] 5.2 `internal/heragater`: failing tests — a cancelled node never materializes; a dependent whose only unsatisfied blocker is cancelled becomes eligible. -- [ ] 5.3 `internal/mcp` (hera_plan.go): failing tests — `hera_plan_node_update` edits a planned node, rejects a materialized node; `hera_unblock` drops an edge idempotently; `hera_plan_node_cancel` cancels a planned node, rejects a materialized one; all three reject non-coordinator callers. -- [ ] 5.4 `internal/tui/planview` + `internal/tui/hera`: failing SimulationScreen/projection test — a cancelled node renders a distinct grey ✕, kept visible, distinct from failed. -- [ ] 5.5 Confirm every Phase-B `it should X` criterion has a failing test. +- [x] 5.1 `internal/db`: failing tests for `RemoveHeraBlock(blocked, blocker)` (removes one edge; missing edge is a no-op), `SetHeraRolePrompt`/`UpdateHeraPlannedNode` (edits prompt/project), and the `cancelled_at` column (`CancelHeraPlannedNode`; `ListHeraPlannedNodes` excludes cancelled; `ListHeraBlocks`/render still surface it). +- [x] 5.2 `internal/heragater`: failing tests — a cancelled node never materializes; a dependent whose only unsatisfied blocker is cancelled becomes eligible. +- [x] 5.3 `internal/mcp` (hera_plan.go): failing tests — `hera_plan_node_update` edits a planned node, rejects a materialized node; `hera_unblock` drops an edge idempotently; `hera_plan_node_cancel` cancels a planned node, rejects a materialized one; all three reject non-coordinator callers. +- [x] 5.4 `internal/tui/planview` + `internal/tui/hera`: failing SimulationScreen/projection test — a cancelled node renders a distinct grey ✕, kept visible, distinct from failed. +- [x] 5.5 Confirm every Phase-B `it should X` criterion has a failing test. ## 6. Phase B — store additions (edge-remove, prompt-edit, cancelled marker) **Depends on:** Stage 5 -- [ ] 6.1 `internal/db/schema.go`: add `cancelled_at TEXT` to `hera_roles` + `idx_hera_roles_cancelled`. -- [ ] 6.2 `internal/db/hera_plan.go`: `RemoveHeraBlock(blocked, blocker int64) error` (DELETE on `hera_blocks`, idempotent). -- [ ] 6.3 `internal/db/hera_plan.go` / `hera.go`: `SetHeraRolePrompt`/`UpdateHeraPlannedNode` (prompt + project edit, planned-only path). -- [ ] 6.4 `internal/db/hera_plan.go`: `CancelHeraPlannedNode` (stamp `cancelled_at`); extend `ListHeraPlannedNodes` with `AND cancelled_at IS NULL`. -- [ ] 6.5 `internal/heragater`: `blockerOutcome` treats a cancelled blocker as non-gating (dependent proceeds). -- [ ] 6.6 uxlog on each store mutation. +- [x] 6.1 `internal/db/schema.go`: add `cancelled_at TEXT` to `hera_roles` + `idx_hera_roles_cancelled`. +- [x] 6.2 `internal/db/hera_plan.go`: `RemoveHeraBlock(blocked, blocker int64) error` (DELETE on `hera_blocks`, idempotent). +- [x] 6.3 `internal/db/hera_plan.go` / `hera.go`: `SetHeraRolePrompt`/`UpdateHeraPlannedNode` (prompt + project edit, planned-only path). +- [x] 6.4 `internal/db/hera_plan.go`: `CancelHeraPlannedNode` (stamp `cancelled_at`); extend `ListHeraPlannedNodes` with `AND cancelled_at IS NULL`. +- [x] 6.5 `internal/heragater`: `blockerOutcome` treats a cancelled blocker as non-gating (dependent proceeds). +- [x] 6.6 uxlog on each store mutation. ## 7. Phase B — MCP mutation verbs **Depends on:** Stage 6 -- [ ] 7.1 `internal/mcp/hera_plan.go`: `hera_plan_node_update` handler + tool def (coordinator-guarded; reject if materialized). -- [ ] 7.2 `internal/mcp/hera_plan.go`: `hera_unblock` handler + tool def (coordinator-guarded; idempotent). -- [ ] 7.3 `internal/mcp/hera_plan.go`: `hera_plan_node_cancel` handler + tool def (coordinator-guarded; reject if materialized). -- [ ] 7.4 Register the three verbs in `heraToolDefs` / the CallTool dispatcher. -- [ ] 7.5 uxlog on each verb. +- [x] 7.1 `internal/mcp/hera_plan.go`: `hera_plan_node_update` handler + tool def (coordinator-guarded; reject if materialized). +- [x] 7.2 `internal/mcp/hera_plan.go`: `hera_unblock` handler + tool def (coordinator-guarded; idempotent). +- [x] 7.3 `internal/mcp/hera_plan.go`: `hera_plan_node_cancel` handler + tool def (coordinator-guarded; reject if materialized). +- [x] 7.4 Register the three verbs in `heraToolDefs` / the CallTool dispatcher. +- [x] 7.5 uxlog on each verb. ## 8. Phase B — planview cancelled rendering **Depends on:** Stage 6 -- [ ] 8.1 `internal/tui/hera` projection (`heraPlanNodes`): map a `cancelled_at` role to a new `planview` cancelled state. -- [ ] 8.2 `internal/tui/planview`: render the cancelled state as a distinct grey ✕ (rune-slice iteration; full-rect; no Sync — follow dag-rendering.md). -- [ ] 8.3 README Reference appendix: add the three MCP verbs to the hera tool table. -- [ ] 8.4 Full Phase-B verification: targeted `go test` green; linter 0 issues. Report Phase B green to coordinator. +- [x] 8.1 `internal/tui/hera` projection (`heraPlanNodes`): map a `cancelled_at` role to a new `planview` cancelled state. +- [x] 8.2 `internal/tui/planview`: render the cancelled state as a distinct grey ✕ (rune-slice iteration; full-rect; no Sync — follow dag-rendering.md). +- [x] 8.3 README Reference appendix: add the three MCP verbs to the hera tool table. +- [x] 8.4 Full Phase-B verification: targeted `go test` green; linter 0 issues. Report Phase B green to coordinator. ## 9. Phase C — authority + docs From df23570eedabb1f1a04624dd8896c523e1e6bf49 Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 02:23:04 -0700 Subject: [PATCH 11/14] Phase C: DAG-authoritative hera skill + gotcha/README docs for living plan-DAG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .claude/skills/hera/SKILL.md | 106 ++++++++++++++++----- README.md | 4 +- context/knowledge/gotchas/dag-rendering.md | 3 +- context/knowledge/gotchas/hera-view.md | 3 +- context/knowledge/gotchas/messaging.md | 5 + context/knowledge/gotchas/orchestration.md | 9 ++ 6 files changed, 100 insertions(+), 30 deletions(-) diff --git a/.claude/skills/hera/SKILL.md b/.claude/skills/hera/SKILL.md index 555d0919..b406599a 100644 --- a/.claude/skills/hera/SKILL.md +++ b/.claude/skills/hera/SKILL.md @@ -94,13 +94,16 @@ Arg names below are exact — do not invent others. The nine coordination tools ### Messaging (idle-gated bus) -- **`hera_send(cwd, body, tldr, [to], [in_reply_to], [orchestrator])`** — message another role **in the - same orchestrator**. `body` and `tldr` are **required**; `tldr` is a one-line summary ≤120 chars, - written from the recipient's perspective (see §5). Worker/freelance senders may omit `to` — it - default-routes to the orchestrator's coordinator. **Coordinators must supply an explicit `to`.** - `in_reply_to` threads a reply to a prior message id. Returns the `message_id`, recipient, and - delivery mode. Caps: 64 KiB body, 500 unread per recipient, 50 sends/min/sender. Cross-orchestrator - messaging is not possible — `to` always resolves within your own orchestrator. +- **`hera_send(cwd, body, tldr, status, [to], [in_reply_to], [orchestrator])`** — message another role + **in the same orchestrator**. `body` and `tldr` are **required**; `tldr` is a one-line summary ≤120 + chars, written from the recipient's perspective (see §5). **`status` is REQUIRED for worker/freelance + senders** (one of `idle`/`working`/`blocked`/`done`/`failed`) and is applied to the sender's role + **synchronously** before the message is sent — it is never delivered async. Omitting `status` as a + worker/freelance sender is an error; coordinator senders may omit it. Worker/freelance senders may + omit `to` — it default-routes to the orchestrator's coordinator. **Coordinators must supply an + explicit `to`.** `in_reply_to` threads a reply to a prior message id. Returns the `message_id`, + recipient, and delivery mode. Caps: 64 KiB body, 500 unread per recipient, 50 sends/min/sender. + Cross-orchestrator messaging is not possible — `to` always resolves within your own orchestrator. - **`hera_inbox(cwd, [orchestrator])`** — fetch all unread messages addressed to your role, oldest first. **Reading IS acknowledgment**: this both cancels pending pane deliveries AND marks the @@ -113,9 +116,12 @@ Arg names below are exact — do not invent others. The nine coordination tools ### Status / tree - **`hera_status(cwd, status, [orchestrator])`** — set your role status: `idle` | `working` | `blocked` - | `done`. Mirrored (best-effort) to argus `task_meta` so the coordinator sees it without asking. - **A `worker`-kind role reporting `status=done` also rolls its bound argus task to `in_review` and - stamps `ready_to_close`** (visible in the rail) — see §4. Coordinators/freelancers just update status. + | `done` | `failed`. Mirrored (best-effort) to argus `task_meta` so the coordinator sees it without + asking. **A `worker`-kind role reporting `status=done` also rolls its bound argus task to `in_review` + and stamps `ready_to_close`** (visible in the rail) — see §4. **A worker reporting `status=failed` + rolls its task to `in_review` WITHOUT `ready_to_close`** (needs-attention, not ready to check off). + The gater treats a `failed` blocker as explicitly failed (no need to wait for session death). + Coordinators/freelancers just update status. - **`hera_tree_updates(cwd, [orchestrator], [since])`** — scan the caller's orchestrator **subtree** (nested sub-orchestrators included) for messages since a cursor. Returns **TLDR-only subject lines — @@ -128,7 +134,12 @@ Arg names below are exact — do not invent others. The nine coordination tools recipient must live in it); inaccessible / missing ids get a per-id `error` field rather than a top-level error. -### Plan authoring — the plan-DAG (coordinator-only) +### Plan authoring + living reconciliation — the plan-DAG (coordinator-only) + +> **With a live coordinator binding, the plan-DAG is the single source of truth for all worker +> activity. Author every worker as a plan node; track progress through the DAG; reconcile the plan +> as work evolves. The harness `TaskCreate` system-reminder is not applicable to coordinated work — +> use `hera_plan_node` / `hera_plan` / `hera_spawn_worker`, never bare task creation.** Instead of spawning every worker immediately, a coordinator can lay out a **plan**: a set of **planned nodes** wired by **blocking edges**, and let the daemon's gater materialize each node into a @@ -136,6 +147,11 @@ live born-bound worker *automatically*, in dependency order. A planned node cost agent, worktree, or inbox until it materializes. The plan-DAG renders in the TUI's second tab (planned `○` → live-by-status), and you navigate it there. +The DAG is **living, not authoring-time**: update it as reality diverges from the plan. Use the +mutation verbs below to edit a node's prompt, drop a stale edge, or cancel a node you no longer need. + +**Plan authoring verbs:** + - **`hera_plan_node(cwd, name, prompt, [orchestrator], [project], [kind], [goal])`** — create ONE planned node (a worker role with no live agent/worktree/inbox yet). It materializes automatically once **all** its blockers reach role-status `done`. **Name with a stable short-id prefix** — number = @@ -164,7 +180,24 @@ agent, worktree, or inbox until it materializes. The plan-DAG renders in the TUI back the entire graph (no orphan nodes). This is the way to author a multi-stage plan at once rather than many `hera_plan_node` + `hera_block` calls. +**Plan mutation verbs (reconcile as you go — coordinator-only):** + +- **`hera_plan_node_update(cwd, name, [prompt], [project], [orchestrator])`** — edit a **planned** + node's prompt and/or project. Rejected once the node has materialized (the prompt was already + delivered to the worker). Use this when you discover the original spec needs revision before the + node spawns. + +- **`hera_unblock(cwd, blocked, blocker, [orchestrator])`** — drop one blocking edge. Idempotent + (dropping a missing edge is a no-op). To re-point an edge: `hera_unblock` (old blocker) then + `hera_block` (new blocker). + +- **`hera_plan_node_cancel(cwd, name, [orchestrator])`** — cancel a planned node: it will never + materialize, its dependents proceed (no longer gated on it), and it stays visible in the plan as a + grey ✕. Rejected if the node has already materialized (use the task lifecycle to stop a running + worker). Cancelled nodes are kept in the DAG to show what was dropped — not silently deleted. + **How materialization + branch-stacking works** (you don't drive it — the gater does, ~60s tick): + - A node with **no remaining blockers** materializes into a born-bound worker (same as `hera_spawn_worker` would produce). - **Non-root nodes stack automatically**: a materializing node is branched off its most-recently-`done` @@ -174,7 +207,16 @@ agent, worktree, or inbox until it materializes. The plan-DAG renders in the TUI branch stacks on it (pass `base_branch` to `hera_new_orchestrator` to override the root). - **Respond to check-ins promptly.** Each node check-ins on materialization via `hera_send`; you pull it from `hera_inbox` and reply (e.g. `"go"`). A node whose blocker genuinely **failed** is HELD and - pings you — decide whether to unblock, re-plan, or let it stay held. + pings you — decide whether to `hera_unblock`, `hera_plan_node_cancel` the held node, or dispatch a + replacement via `hera_spawn_worker`. + +**Standing order — keep the DAG reconciled:** + +- After every worker interaction, check whether the plan still reflects reality. If a worker's scope + changed, `hera_plan_node_update` its prompt. If an edge is no longer needed, `hera_unblock` it. If a + node was superseded, `hera_plan_node_cancel` it. +- A worker reopening (re-engaging on rework after `done`/`failed`) reports `working` on its next + `hera_send` by requirement — the DAG self-corrects. No coordinator action needed for a simple reopen. **Sub-coordinator nodes (`kind=subcoord`).** Use a subcoord node when a plan stage is itself a *sub-team* rather than a single unit of work — a chunk big enough to warrant its own coordinator and its own @@ -210,7 +252,9 @@ gater materializes it as a *distinct coordinator agent* when its blockers finish Use the **plan-DAG** (`hera_plan`, or `hera_plan_node` + `hera_block`) when work runs in **stages / dependency order** — author planned nodes wired by blocking edges and let the gater materialize them as their blockers finish (auto-stacking each stage's branch on the prior). Lay the whole graph out - with one `hera_plan` call; respond to each node's check-in via `hera_inbox`. + with one `hera_plan` call; respond to each node's check-in via `hera_inbox`. **Reconcile the DAG + as work evolves** — edit nodes, drop stale edges, cancel superseded nodes rather than abandoning the + plan. - **Worker node vs sub-coordinator node:** a plain plan node (`kind=worker`) is a single unit of work. Make it `kind=subcoord` (with a `goal`) only when the stage is a *sub-team* — large enough to deserve its own coordinator that plans and fans out its own workers. It's the declarative alternative to a @@ -220,11 +264,12 @@ gater materializes it as a *distinct coordinator agent* when its blockers finish doorbell line. - **Want whole-team state?** `hera_tree_updates(cwd=$PWD)`, then `hera_get_messages(ids=[…])` for the ones worth reading. -- **How completion flows back:** a worker finishing calls `hera_status(done)` (and usually a closing - `hera_send` to the coordinator). For a `worker`-kind role that rolls its argus task to `in_review` + - `ready_to_close`, which the coordinator sees in the rail without asking. The roll is idempotent and - only fires when the task is still `in_progress` — it never auto-completes or clobbers a human-set - status. The live session is left running. +- **How completion flows back:** a worker finishing sends a closing `hera_send(status="done", …)` — the + synchronous status apply rolls its task to `in_review` + `ready_to_close`, visible in the rail. A + worker that cannot complete sends `hera_send(status="failed", …)` — rolls to `in_review` WITHOUT + `ready_to_close` (needs attention, not ready to check off); the gater holds any dependent planned + nodes and pings you. Both rolls are idempotent and only fire when the task is still `in_progress`. + The live session is left running. - **Don't** use `hera_send` to talk to the human — the human reads the coordinator's own agent pane; the bus is role-to-role only. @@ -238,6 +283,10 @@ in the doorbell, returned by `hera_tree_updates`, and stored permanently. ## 6. Gotchas worth calling out +- **`hera_send` requires `status` for worker/freelance senders — omitting it is an error.** The status + is applied synchronously before the send completes; it never rides the async delivery bus. This means + every `hera_send` call doubles as a role-status heartbeat. There is no default; the error message on + omission names the valid values (`idle`/`working`/`blocked`/`done`/`failed`). - **Spawned workers default to the project's stale default branch, NOT the coordinator's branch.** `hera_spawn_worker`'s `branch` defaults to the *project* default (e.g. an old `master`/`main`), not the coordinator's current worktree branch. If the worker must build on the coordinator's (or a sibling's) @@ -280,13 +329,14 @@ You opened in a born-bound worker terminal: 1. `hera_join(cwd=$PWD)` → read your role name, mission (role prompt), and unread count. 2. `hera_status(cwd=$PWD, status="working")`. 3. Do the work in your worktree. If you hit a fork that needs the coordinator's call: - `hera_send(cwd=$PWD, body="", tldr="Need decision: X vs Y for the cart schema")` + `hera_send(cwd=$PWD, status="working", body="", tldr="Need decision: X vs Y for the cart schema")` (no `to` needed — default-routes to the coordinator), then check `hera_inbox(cwd=$PWD)` on the - doorbell for the answer. + doorbell for the answer. **Always supply `status` on every `hera_send` — it is required for + worker/freelance senders.** 4. Land your work (open a PR via iris, or leave commits for the coordinator to pull). -5. `hera_send(cwd=$PWD, body="", tldr="cart-api done, PR #47, tests green")` then - `hera_status(cwd=$PWD, status="done")` — which rolls your task to in_review + ready_to_close so the - coordinator sees you finished. +5. `hera_send(cwd=$PWD, status="done", body="", tldr="cart-api done, PR #47, tests green")` + — the synchronous status apply rolls your task to in_review + ready_to_close so the coordinator + sees you finished. If you cannot complete: `hera_send(cwd=$PWD, status="failed", body="", …)`. ### (c) Author a staged plan-DAG and let it self-materialize @@ -311,10 +361,14 @@ You are a coordinator and the work has clear stages (a seed, a parallel fan-out, `1a-seed` materializes first (rooted on your branch); `2a`/`2b` materialize in parallel once it's `done` (each stacked on `1a-seed`'s branch); `3a-final` waits for **both** and stacks on the latest. 3. Watch it fill in the second-tab plan-DAG (planned `○` → live). Respond to each node's check-in: - `hera_inbox(cwd=$PWD)` on the doorbell → reply `hera_send(to="", body="go", tldr="go")`. -4. If a node is HELD behind a genuinely failed blocker, the gater pings you — re-plan, unblock, or - re-dispatch. (A coordinator-as-blocker edge is rejected at authoring time, so you can't wedge the + `hera_inbox(cwd=$PWD)` on the doorbell → reply `hera_send(cwd=$PWD, to="", body="go", tldr="go")`. +4. If a node is HELD behind a genuinely failed blocker, the gater pings you — use `hera_unblock` to + drop the edge, `hera_plan_node_cancel` to cancel the held node, or `hera_spawn_worker` to dispatch + a replacement. (A coordinator-as-blocker edge is rejected at authoring time, so you can't wedge the graph on a never-`done` coordinator.) +5. **Reconcile as work unfolds.** If a worker's scope changed: `hera_plan_node_update` its prompt before + it materializes. If an edge is obsolete: `hera_unblock`. If a node was superseded: `hera_plan_node_cancel`. + Keep the DAG a live mirror of the actual plan. ### Worker promotion: becoming a sub-coordinator diff --git a/README.md b/README.md index 44a1cbfe..917567e5 100644 --- a/README.md +++ b/README.md @@ -417,10 +417,10 @@ If the recipient has a live agent session the daemon also writes a single notifi | `hera_new_orchestrator` | Bootstrap a new orchestrator and claim its `coordinator` role for the calling task. | | `hera_join` | Claim the calling task's existing role + unread count, or (with `role_name` + `kind`) attach a new `worker`/`freelance` role under an orchestrator. | | `hera_spawn_worker` | Spawn a born-bound worker task + session under the caller's orchestrator (caller must hold a live coordinator binding). Optional `model` picks the worker's model by task complexity (backend-scoped; empty = backend default). | -| `hera_send` | Send a role-addressed message; workers/freelancers default to the coordinator when `to` is omitted, coordinators must name a recipient. | +| `hera_send` | Send a role-addressed message. **`status` is required for worker/freelance senders** (`idle`/`working`/`blocked`/`done`/`failed`) and is applied synchronously before send. Workers/freelancers default to the coordinator when `to` is omitted; coordinators must name a recipient. | | `hera_inbox` | Fetch the caller role's unread messages (oldest first), cancel their pending pane deliveries, and mark them read. | | `hera_mark_read` | Mark a specific list of message IDs read and cancel their pending deliveries. | -| `hera_status` | Set the caller role's status (`idle`/`working`/`blocked`/`done`), mirrored to `task_meta`; a worker reporting `done` rolls its task to in-review. | +| `hera_status` | Set the caller role's status (`idle`/`working`/`blocked`/`done`/`failed`), mirrored to `task_meta`; `done` rolls the worker's task to in-review + `ready_to_close`; `failed` rolls to in-review without `ready_to_close`. | | `hera_tree_updates` | Scan the caller's orchestrator subtree for messages since a per-role cursor; returns TLDR subject lines only and auto-advances the cursor. | | `hera_get_messages` | Fetch full message bodies by ID (after `hera_tree_updates`), scoped to the caller's orchestrator subtree. | | `hera_plan_node` | Author a single planned node under the caller's orchestrator (coordinator-only). Params: `name`, `kind` (`worker`\|`subcoord`, default `worker`), `prompt` (worker nodes) or `goal` (subcoord nodes — required; the goal handed to the spawned coordinator). A `subcoord` node materializes as a distinct coordinator agent with its own task, worktree, and child orchestrator. | diff --git a/context/knowledge/gotchas/dag-rendering.md b/context/knowledge/gotchas/dag-rendering.md index 078ce5b6..4878ddb8 100644 --- a/context/knowledge/gotchas/dag-rendering.md +++ b/context/knowledge/gotchas/dag-rendering.md @@ -18,7 +18,8 @@ The `dagview` widget (`internal/tui/dagview/`) renders a layered top-down graph. ## planview node colour + state (the new consumer) - **Node colour comes from the bound task's argus status / result, NOT the hera role status.** `heraPlanNodes`/`heraPlanNodesWithBridge` stamp `planview.Node.State` from `RoleView.TaskStatus` + `RoleView.TaskResult` for a live node, and `StatePlanned` (violet `○`) for a never-bound planned role. A `{"failed":true}` result wins over the workflow status (red `✕`) via the shared `coordTaskFailed`. The rail's own status icons use the hera role status — the two are deliberately different signals. -- **Collapsed-group count segments use `widget.RoleStatusIcon` (1:1 with the rail), per-state colour, and an animated working spinner — never the compact `State.Glyph()` set (`◔`/`⟳`) (BUG-011).** `groupCountSegs` emits styled `countSeg`s: the glyph is resolved by `countSegGlyph(state, w.animFrame)`, which synthesises a `widget.RoleStatusInputs` from the State (done→`Done`✓, working→`Active`→live spinner, in_review→`ReadyToClose`→`IconReview 󰂼`, pending→`Idle`→moon) and calls the SAME shared classifier the rail/node use; planned/failed keep the `○`/`✕` State overlay. **Only the glyph comes from the classifier — the colour is `State.style()`** (done green, working amber, in_review **cyan** not the classifier's ready-to-close green, planned violet, failed red, pending dim); `·` separators and the role token stay dim. The working spinner animates because `groupCountSegs` runs during layout each Draw (after `w.animFrame = planSpinnerFrame()`), so the current frame is baked in per Draw. `layoutDashedBox` takes `[]countSeg` and paints the sub line segment-by-segment so each count keeps its own style. +- **Collapsed-group count segments use `widget.RoleStatusIcon` (1:1 with the rail), per-state colour, and an animated working spinner — never the compact `State.Glyph()` set (`◔`/`⟳`) (BUG-011).** `groupCountSegs` emits styled `countSeg`s: the glyph is resolved by `countSegGlyph(state, w.animFrame)`, which synthesises a `widget.RoleStatusInputs` from the State (done→`Done`✓, working→`Active`→live spinner, in_review→`ReadyToClose`→`IconReview 󰂼`, pending→`Idle`→moon) and calls the SAME shared classifier the rail/node use; planned/failed/cancelled keep the `○`/`✕` State overlay. **Only the glyph comes from the classifier — the colour is `State.style()`** (done green, working amber, in_review **cyan** not the classifier's ready-to-close green, planned violet, failed red, cancelled grey, pending dim); `·` separators and the role token stay dim. The working spinner animates because `groupCountSegs` runs during layout each Draw (after `w.animFrame = planSpinnerFrame()`), so the current frame is baked in per Draw. `layoutDashedBox` takes `[]countSeg` and paints the sub line segment-by-segment so each count keeps its own style. +- **Cancelled nodes render `StateCancelled` (grey ✕, `ColorDimmed`), distinct from failed (red ✕).** `planNodeState` checks `Cancelled` BEFORE `Planned` — a cancelled role is never treated as a plain planned node. `StateColour`/`stateStyle` for `StateCancelled` use `ColorDimmed` so the node is visually de-emphasised (kept visible but clearly terminal). Do not conflate `StateCancelled` with `StateFailed` (red, `ColorError`) — failed is a live-node argus task result; cancelled is a coordinator decision that a planned node will never materialize. ## Widget integration diff --git a/context/knowledge/gotchas/hera-view.md b/context/knowledge/gotchas/hera-view.md index 8a6f02b4..6afc95d8 100644 --- a/context/knowledge/gotchas/hera-view.md +++ b/context/knowledge/gotchas/hera-view.md @@ -66,7 +66,8 @@ M6a scaffolds the native Hera view: a `HeraPage` (rail | coordinator pane | agen - **`consumedSet` must equal what rendering actually PLACES, or the safety sweep flattens the difference to top-level roots.** This caused the second half of the under-nesting bug: an ARCHIVED bridging worker was hoisted into the collapsed per-coordinator Archive expando (never `appendWorkerRow`'d), yet `consumedSet` still consumed its child → child unplaced → safety-swept flat. Fix: `appendOrchWorkers` keeps an archived worker that `workerBridgeChild(...) != nil` IN PLACE (dimmed row) so its child nests; only archived LEAF workers fold into the expando. `SubtreeOrchIDs` has no parent-side archived-role filter, so this matches it. The archived worker ROW dims, but its live child subtree dims only from inherited `dim` or the child's OWN `Archived` (an active child under an archived bridge stays normal). Verified on a COPY of live `~/.argus/data.sql`: 7 rendered roots (was ~24), no migration. - **The bridging worker row keeps its PARENT worker selection context (conservative) for pane binding and most mutations — but Ctrl+D cascades the WHOLE nested subtree. The SAME cascade now also fires on a coordinator/orchestrator HEADER (BUG-017).** `Selection.BridgeChildOrchID` carries the child id (set when a role row's `collOrchID > 0`). `heraOpenDelete` routes BOTH the bridging-worker case (`heraCascadeDeleteFrom(BridgeChildOrchID)`) and the orchestrator-header case (`heraCascadeDeleteFrom(sel.Orch.ID)`) to the same cascade — `heraCascadeDeleteFrom(rootID)` (renamed from `heraCascadeDeleteSubtree`; takes ANY orch id). It walks `Model.BridgeSubtree(rootID)`, shows a confirm modal enumerating orchestrators/agents ARCHIVED and worktrees/branches RECLAIMED, and on confirm — per the archive-not-delete model above — ARCHIVES every live role (`RetireRole`, ending its binding) + the orchestrator (`ArchiveOrchestrator`), and reclaims+archives (`heraReclaimAndArchiveTask`) every task with NO live binding OUTSIDE the subtree. NOTHING is hard-deleted. `BridgeSubtree` already walks a TOP-LEVEL root (root orch + nested sub-coords via worker bridge AND `coordBridgeChildren`) — proven by `TestModel_BridgeSubtree` (`BridgeSubtree(R)` ⇒ `{R,C,G}`) — so NO model/rail change was needed. A task bound under a non-subtree parent is preserved (multi-binding safety — left fully alone). The `heraTaskBoundOutside(taskID, subtreeIDs)` predicate drives BOTH the count and the action, so the worktree count never undercounts an INTERNAL bridge task (bound under two subtree orchestrators) in a depth≥2 cascade; the reclaim decision per task is made BEFORE ending that role's binding so the multi-binding check sees full state. The subtree snapshot is point-in-time (modal-open) but every op re-validates live, so a stale pointer never archives the wrong row. A non-bridging ROLE row keeps the conservative single-role delete. (Bridging-row selection acts on the child, not the worker — Aaron's explicit call 2026-06-16.) - **Per-coordinator `Archive (N)` expando is distinct from the bottom Archive section.** `appendOrchWorkers` hoists a coordinator's ARCHIVED roles out of inline rendering into a per-coord expando (collapsed by default, `coordArchiveOpen[orchID]`, `railRow.archiveOwner`). The bottom Archive section (`collArchive`/`archiveCollapsed`) is for archived ROOT orchestrators. Both are `rrArchiveExpando`; `drawRow`/`ToggleCollapse` branch on `archiveOwner > 0`. -- **The rail spinner animates on REAL session activity (`RoleView.IsActive` = live binding + bound argus task `in_progress`), NOT the hera `working` role-status — BUG-003.** The hera role Status (`idle`/`working`/`blocked`/`done`) is a manual/MCP-set ladder value that NEVER reconciles down: it stays `working` after the session idles, stops, or dies. Binding the spinner to it (the original rail-parity #8 code) made stopped/idle/days-old roles animate. `statusIcon` now derives the spinner from `role.IsActive()` (`Live && TaskStatus == "in_progress"`), mirroring the plugin's `stateGlyph` (spinner on `in_progress`+running). Precedence: `ready_to_close` > `blocked`/`done` role-status > `IsActive()` spinner > `idle` role-status > `Live` (moon-stars) > unbound dimmed. **`blocked`/`done` outrank activity** (a deliberate "I'm blocked" assertion must show needs-input even while the task is still `in_progress`). Native has NO per-task idle/needs-input flag (the plugin sourced those from the API), so a live `in_progress` task sitting idle still counts active; the reported stale cases are excluded because they are not `Live` or no longer `in_progress`. `coordStatusLabel` (Details) applies the same honesty: stale `working` → `live` (binding alive) or `stopped` (binding gone), never `working`. Keep `statusIcon` pure (frame in, no `time.Now()` inside) so tests pass explicit frames; `Rail.animFrame` is recomputed from wall-clock each `Draw` (`spinnerFrame()` mirrors the task list). The spinner only advances while the app's spinner loop redraws (any session actively running). +- **The rail spinner animates on REAL session activity (`RoleView.IsActive` = live binding + bound argus task `in_progress`), NOT the hera `working` role-status — BUG-003.** The hera role Status (`idle`/`working`/`blocked`/`done`/`failed`) is a manual/MCP-set ladder value that NEVER reconciles down: it stays `working` after the session idles, stops, or dies. Binding the spinner to it (the original rail-parity #8 code) made stopped/idle/days-old roles animate. `statusIcon` now derives the spinner from `role.IsActive()` (`Live && TaskStatus == "in_progress"`), mirroring the plugin's `stateGlyph` (spinner on `in_progress`+running). Precedence: `ready_to_close` > `blocked`/`done`/`failed` role-status > `IsActive()` spinner > `idle` role-status > `Live` (moon-stars) > unbound dimmed. **`blocked`/`done`/`failed` outrank activity** (a deliberate assertion must show even while the task is still `in_progress`). **`failed` renders as red ✕ (distinct from `done` ✓ and `ready_to_close` ✓)** — its precedence is BELOW `ready_to_close`/needs-input but ABOVE `IsActive()`, so a failed worker shows red ✕ even while its session is still running. Native has NO per-task idle/needs-input flag (the plugin sourced those from the API), so a live `in_progress` task sitting idle still counts active; the reported stale cases are excluded because they are not `Live` or no longer `in_progress`. `coordStatusLabel` (Details) applies the same honesty: stale `working` → `live` (binding alive) or `stopped` (binding gone), never `working`. Keep `statusIcon` pure (frame in, no `time.Now()` inside) so tests pass explicit frames; `Rail.animFrame` is recomputed from wall-clock each `Draw` (`spinnerFrame()` mirrors the task list). The spinner only advances while the app's spinner loop redraws (any session actively running). +- **`RoleView.Cancelled` (from `hera_roles.cancelled_at IS NOT NULL`) projects to `planview.StateCancelled` in the plan widget.** `heraPlanNodesWithBridge` stamps `Node.State = StateCancelled` when `RoleView.Cancelled` is true; `planNodeState` checks `Cancelled` before `Planned` so a cancelled role never renders as a violet `○`. The rail itself does not render a cancelled glyph — `statusIcon` ignores `Cancelled` because a cancelled planned role has no binding and no task; it never appears as a rail row (only planned/live roles with bindings reach the rail). - **The PR cell on rail rows reads the same `prMeta` ("pr" namespace) the Details roster uses — `Rail.SetPRMeta` is called from `doRefresh`.** A managed role whose bound task has a non-empty `url` renders a right-aligned `PR` tag, reserving width so the name truncates instead of overwriting it. Best-effort; never fetched by the view. - **`buildRows`' safety sweep must NOT leak collapse/archive-HIDDEN children to the top — guard it with `structuralReach`.** Loop 2 (`for Active { if !placed[id] … appendOrch(depth 0) }`) exists to rescue true bridge-cycle orphans. But `placed` is render-time (it respects collapse/archive fold) while `consumed` is global, so a bridged child of a COLLAPSED parent (or behind a collapsed per-coordinator Archive expando) is `!placed` and gets re-placed flat at depth 0 — the rail under-nesting bug (a collapsed `sherlock-mvp` leaks `0e-team` et al. to the top). Fix: `structReach := r.structuralReach(canonical)` (every orch whose canonical-parent chain reaches a true root, fold ignored — mirrors EXACTLY what canonical placement nests), and rescue only `!placed && !structReach` (genuine cycle-orphans, i.e. a chain that loops without reaching a root). Collapse-hidden children then stay folded. **This bug only reproduces with the parent COLLAPSED** — expanded-fold renders look correct, so any offline rail repro MUST control fold state (the "live flat vs offline nested" paradox was fold state, not data/connection/binary). `f830308e` fixed only the archived-bridging-worker hoist, not this collapse leak. See `TestRail_CollapsedParentDoesNotLeak{Coord,WorkerBridged}Child`. - **A child reachable from MORE THAN ONE bridge-parent gets ONE deterministic, FOLD-INDEPENDENT canonical parent — `Model.canonicalParents()` — computed once per `buildRows`, so folding one parent can't migrate the subtree to the other.** The live quirk: a multi-bound coordinator session (task T) coordinates `update-argus` AND `native-hera-parity` AND is the `rail-debug` worker under `hera-rail`, so `native-hera-parity` was reachable via coordinator-spawn (`update-argus`) AND, transitively, the worker-bridge under `hera-rail`. Render used to nest it under whichever path it hit first (`placed`-guarded, first-reached-wins), so collapsing `hera-rail` relocated it to `update-argus` and re-expanding moved it back — bindings never changed, pure render artifact. The rule: **prefer a coordinator-spawn parent over a worker-bridge parent; among coordinator-spawn candidates the earliest coordinator role id wins (matching `coordBridgeParentOf`); among worker-bridge candidates the lowest orchestrator id wins.** The render then nests a child ONLY under its canonical parent: `appendOrchWorkers`' coord-spawn loop checks `canonical[child].coordSpawn && orchID==o.ID`, and `workerBridgeChild` searches for the canonical worker-child of the owner matching the worker's bridge task (NOT a `bridgeIndex` first-wins lookup — that first-wins ambiguity is what flipped under fold). **Coordinator-spawn + worker-bridge multi-parent always collapses to a CHAIN, never siblings**: a worker bridging the shared coordinator task T necessarily targets the clique ROOT (the earliest-coord-id orch sharing T), and the later coord-spawn children hang under it — so the deterministic structure is `hera-rail → rail-debug → update-argus → native-hera-parity`, not two competing parents. `canonicalParents` is purely a RENDER-placement decision: it does NOT change subtree membership, so `consumedSet`/`bridgeIndex`/`BridgeSubtree` (root selection + the Ctrl+D cascade) are untouched and still walk every bridge. `structuralReach` follows the canonical chain (above) so the two stay consistent. See `TestRail_MultiParentChildNestsDeterministically` (live shape) + `TestRail_MultiWorkerBridgeParentsPickLowestOrchID` (sibling worker-bridge form). diff --git a/context/knowledge/gotchas/messaging.md b/context/knowledge/gotchas/messaging.md index aaf30392..866f3893 100644 --- a/context/knowledge/gotchas/messaging.md +++ b/context/knowledge/gotchas/messaging.md @@ -135,6 +135,11 @@ true)` and `db.Delete(id)`; entrypoints that go through `db.Update` the typical "task replies within a few seconds" case where 500ms tick is plenty. +## Hera send-status (make-hera-plan-living) + +- **Worker/freelance `hera_send` REQUIRES a `status` parameter; the apply is SYNCHRONOUS and DECOUPLED from doorbell delivery.** The handler calls `applyRoleStatus` (the shared `hera_status` path) before any send/deliver work; it commits the role-status DB write and task roll synchronously in the send handler — it never rides `notify.Notifier` (best-effort, idle-gated). A failed `applyRoleStatus` is soft-failed (logged, send proceeds), so the message always lands even if the DB write fails transiently. +- **`failed` is a valid `hera_send` status (fifth value).** A worker reporting `status="failed"` rolls its task to `in_review` WITHOUT `ready_to_close` (needs attention). The gater's `blockerOutcome` treats a `failed` role-status as explicitly `blockerFailed` — without waiting for session death — and holds dependent planned nodes. + ## Hera message bus (M2) - **`hera_messages.read_at` is real NULL, never `''`.** The partial inbox index diff --git a/context/knowledge/gotchas/orchestration.md b/context/knowledge/gotchas/orchestration.md index 3228077d..1a540507 100644 --- a/context/knowledge/gotchas/orchestration.md +++ b/context/knowledge/gotchas/orchestration.md @@ -75,6 +75,15 @@ Hera orchestration + task-creation invariants that caused bugs when violated. - **The daemon MUST call `SetSubCoordMaterializer` before `heragater.Start`.** The gater has a defensive nil-seam fallback (falls back to the worker materializer), but that path logs `slog.Warn` with "MISCONFIGURATION" and produces a semantically-wrong agent — treat the warning as a bug, not a graceful degradation. - **`heragater.go`'s package doc and the substrate spec still say "worker-kind roles"; this is intentional staleness.** `node_kind=subcoord` is additive; the "worker default" framing remains accurate for every other node. The central archive pass (`openspec archive`) will reconcile the wording once the change is merged. +## Hera living plan-DAG (make-hera-plan-living) + +- **`hera_send` status is applied SYNCHRONOUSLY in the send handler, never via the async delivery bus.** `applyRoleStatus` (the shared path for both `hera_send` and `hera_status`) runs inside the send handler before the call returns; it is NOT enqueued on `notify.Notifier`. A worker's role-status update is therefore authoritative the instant `hera_send` returns, independent of whether the doorbell delivery ever fires. +- **`status` is REQUIRED for worker/freelance senders on `hera_send`; omitting it is a hard error.** The shared `applyRoleStatus` path is called only when `status` is present; the handler errors on omission with the full valid-values list. Optional/ignored for coordinator senders. +- **`failed` is a fifth role-status value (rail red ✕), distinct from `done`.** A worker reporting `failed` rolls its task to `in_review` WITHOUT `ready_to_close` (needs-attention, not ready to check off). The gater's `blockerOutcome` treats a blocker whose role-status is explicitly `failed` as `blockerFailed` — without waiting for session death. A worker that `done` stamps `ready_to_close`; `failed` does not. +- **The gater held-ping RE-ARMS when a failed blocker recovers.** When a `(node, blocker)` key in `heldPings` clears because the blocker's outcome is no longer `blockerFailed`, a one-time "unblocked" recovery notice fires to the coordinator (from the held node's role). A subsequent re-failure re-arms the dedup and re-pings. There is NO notice when a blocker reopens after an ALREADY-MATERIALIZED node (that case is physics — a spawned worker cannot be un-spawned). +- **Cancelled nodes: `cancelled_at` column, excluded from materialization, treated as blockerDone so dependents proceed.** `ListHeraPlannedNodes` filters `AND cancelled_at IS NULL`, so the gater never materializes a cancelled node. `blockerOutcome` returns `blockerDone` for a cancelled blocker (the dependent is unblocked). The role row is kept (not deleted) so planview renders it as grey ✕ (`StateCancelled`). Dependents of a cancelled node materialize as normal. +- **Reopen is structural, not a watcher.** A `done`/`failed` worker that re-engages is required to send `status="working"` on its next `hera_send` by enforcement — the required-status rule is the reopen mechanism. No observed-reopen daemon watcher was added. + ## Hera subtree TLDR roll-up (M5) - **The subtree (TLDR roll-up) and the orchestration tree share the SAME nesting graph now.** `SubtreeOrchIDs` walks the orchestrator *nesting* graph (multi-binding bridges) for message roll-up; `heraTreeNodes` (the Details-pane graph) walks the SAME nesting in-memory for rendering. Both derive from role bindings — there is no longer a separate `depends_on` dependency graph to conflate them with (it was retired). A message roll-up is still a distinct concern from the rendered tree, but they no longer use orthogonal data sources. From 90e8161cf2939bb171806e2a43b1eea508da244a Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 02:23:47 -0700 Subject: [PATCH 12/14] =?UTF-8?q?Phase=20C:=20tick=20tasks.md=20(stage=209?= =?UTF-8?q?=20complete)=20=E2=80=94=20all=2047=20tasks=20done?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- openspec/changes/make-hera-plan-living/tasks.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/openspec/changes/make-hera-plan-living/tasks.md b/openspec/changes/make-hera-plan-living/tasks.md index 758167a5..9eda4024 100644 --- a/openspec/changes/make-hera-plan-living/tasks.md +++ b/openspec/changes/make-hera-plan-living/tasks.md @@ -96,10 +96,10 @@ Write failing tests from the `hera-coordination` (mutation-verb + cancelled) and **Depends on:** Stage 8 -- [ ] 9.1 `.claude/skills/hera/SKILL.md`: state the plan-DAG is authoritative for a bound coordinator; document the required `hera_send` status; document `hera_plan_node_update` / `hera_unblock` / `hera_plan_node_cancel`; add a "keep the DAG reconciled" standing order; reframe the create-only language. -- [ ] 9.2 `context/knowledge/gotchas/orchestration.md`: status-on-send synchronous apply; `failed` status + explicit gating; re-arm + recovery notice; cancelled-node semantics. -- [ ] 9.3 `context/knowledge/gotchas/dag-rendering.md`: cancelled node grey ✕ rendering rules. -- [ ] 9.4 `context/knowledge/gotchas/hera-view.md`: failed glyph precedence; cancelled node projection. -- [ ] 9.5 `context/knowledge/gotchas/messaging.md`: required worker→coord send-status + synchronous apply + soft-fail. -- [ ] 9.6 README Reference: add the `hera_send` `status` parameter to the tool table. -- [ ] 9.7 Report Phase C green to coordinator. +- [x] 9.1 `.claude/skills/hera/SKILL.md`: state the plan-DAG is authoritative for a bound coordinator; document the required `hera_send` status; document `hera_plan_node_update` / `hera_unblock` / `hera_plan_node_cancel`; add a "keep the DAG reconciled" standing order; reframe the create-only language. +- [x] 9.2 `context/knowledge/gotchas/orchestration.md`: status-on-send synchronous apply; `failed` status + explicit gating; re-arm + recovery notice; cancelled-node semantics. +- [x] 9.3 `context/knowledge/gotchas/dag-rendering.md`: cancelled node grey ✕ rendering rules. +- [x] 9.4 `context/knowledge/gotchas/hera-view.md`: failed glyph precedence; cancelled node projection. +- [x] 9.5 `context/knowledge/gotchas/messaging.md`: required worker→coord send-status + synchronous apply + soft-fail. +- [x] 9.6 README Reference: add the `hera_send` `status` parameter to the tool table. +- [x] 9.7 Report Phase C green to coordinator. From 35d16d7a36fc444972f7b5d6789a25906aac1e96 Mon Sep 17 00:00:00 2001 From: Aaron Newton Date: Sun, 21 Jun 2026 02:47:44 -0700 Subject: [PATCH 13/14] Phase A: one-off migration script for failed role-status CHECK on existing DBs Rebuilds hera_role_status with the widened CHECK ('failed' added), preserving rows. Fresh DBs are already correct via schema.go CREATE; this is only for in-place upgrades of an existing ~/.argus/data.sql (SQLite can't ALTER a CHECK). --- script/migrate-hera-failed-status.sh | 76 ++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100755 script/migrate-hera-failed-status.sh diff --git a/script/migrate-hera-failed-status.sh b/script/migrate-hera-failed-status.sh new file mode 100755 index 00000000..dc933414 --- /dev/null +++ b/script/migrate-hera-failed-status.sh @@ -0,0 +1,76 @@ +#!/usr/bin/env bash +# One-off migration: widen the hera_role_status.status CHECK to accept the new +# `failed` role-status (make-hera-plan-living, Phase A / decision D2). +# +# WHY THIS IS NEEDED: schema.go's CREATE TABLE includes 'failed' in the CHECK, so +# a FRESH ~/.argus/data.sql is already correct and needs nothing. But CREATE TABLE +# IF NOT EXISTS is a no-op on a database that already has the table, and SQLite +# cannot ALTER a CHECK constraint. So an EXISTING DB keeps the old CHECK +# (idle|working|blocked|done); the first `failed` write fails the constraint and +# (via hera_send's soft-fail status-apply) silently no-ops. This rebuilds just the +# hera_role_status table with the widened CHECK, preserving all rows. +# +# Per CLAUDE.md ("breaking changes fine, no backwards-compat / no legacy migration +# code — write a ONE-OFF script for schema data moves"), this is that one-off +# script. Run it once against a live DB after deploying the Phase A binary; do NOT +# wire it into app startup. +# +# USAGE (stop the daemon first so it isn't holding the DB): +# script/migrate-hera-failed-status.sh # default ~/.argus/data.sql +# script/migrate-hera-failed-status.sh /path/to/data.sql +# +# Idempotent: safe to re-run. On a DB whose CHECK already allows 'failed' it just +# rebuilds the table identically. +set -euo pipefail + +DB="${1:-$HOME/.argus/data.sql}" + +if ! command -v sqlite3 >/dev/null 2>&1; then + echo "error: sqlite3 not found on PATH" >&2 + exit 1 +fi +if [ ! -f "$DB" ]; then + echo "error: database not found at $DB (a fresh DB needs no migration)" >&2 + exit 1 +fi + +BACKUP="${DB}.bak-$(date +%Y%m%d-%H%M%S)" +cp "$DB" "$BACKUP" +echo "Backed up $DB -> $BACKUP" + +# Rebuild hera_role_status with the widened CHECK, preserving rows. SQLite cannot +# ALTER a CHECK, so this is the create-new / copy / drop / rename dance. +sqlite3 "$DB" <<'SQL' +PRAGMA foreign_keys=OFF; +BEGIN; +CREATE TABLE hera_role_status_new ( + role_id INTEGER PRIMARY KEY REFERENCES hera_roles(id) ON DELETE CASCADE, + status TEXT NOT NULL CHECK (status IN ('idle','working','blocked','done','failed')), + updated_at TEXT NOT NULL +); +INSERT INTO hera_role_status_new (role_id, status, updated_at) + SELECT role_id, status, updated_at FROM hera_role_status; +DROP TABLE hera_role_status; +ALTER TABLE hera_role_status_new RENAME TO hera_role_status; +COMMIT; +PRAGMA foreign_keys=ON; +PRAGMA foreign_key_check; +SQL + +# Verify the widened CHECK now accepts 'failed' (uses a transaction it rolls back, +# so no test row is left behind). Requires a role row to reference; skip if none. +if sqlite3 "$DB" "SELECT 1 FROM hera_roles LIMIT 1;" | grep -q 1; then + RID="$(sqlite3 "$DB" "SELECT id FROM hera_roles LIMIT 1;")" + sqlite3 "$DB" < Date: Wed, 24 Jun 2026 21:18:05 -0700 Subject: [PATCH 14/14] Reconcile hera-view Status-icon delta onto DRN's reconciled base spec (#795 rebase) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DRN's archive pass rewrote the Status-icon precedence requirement (4-tier -> 6-tier + BUG-003 spinner-from-real-activity + new scenarios). Re-layer the failed red ✕ addition onto DRN's version instead of clobbering it with the stale 4-tier text. --- .../specs/hera-view/spec.md | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/openspec/changes/make-hera-plan-living/specs/hera-view/spec.md b/openspec/changes/make-hera-plan-living/specs/hera-view/spec.md index a7e4d9ee..5465b03d 100644 --- a/openspec/changes/make-hera-plan-living/specs/hera-view/spec.md +++ b/openspec/changes/make-hera-plan-living/specs/hera-view/spec.md @@ -2,24 +2,34 @@ ### Requirement: Status-icon precedence on role rows (area 3) -The system SHALL choose a role row's status glyph by this precedence: (1) `ready_to_close` wins over everything with a distinct review glyph; otherwise (2) the hera role status when present — working, blocked, done, failed, or idle each map to a distinct glyph/style, with `failed` rendering a red ✕; otherwise (3) binding presence (`Live`) renders a "live" glyph; otherwise (4) an unbound/dimmed glyph. `ready_to_close` is read from the task-addressed `task_meta` "hera" namespace, not the hera tables. The status vocabulary (idle/working/blocked/done/failed) is shared 1:1 with the role-status enum so the rail, the plan view, and the gater never drift. +The system SHALL choose a role row's status glyph by this precedence: (1) `ready_to_close` wins over everything with a distinct review glyph; otherwise (2) an operator/agent-set `blocked`, `done`, or `failed` hera role status renders its distinct static glyph (`failed` rendering a red ✕, distinct from `done`); otherwise (3) GENUINE activity (`RoleView.IsActive` — a live binding whose bound argus task is `in_progress`) renders the ACTIVE SPINNER's animated frame (see "Active agents animate a spinner glyph"); otherwise (4) an `idle` hera role status renders the static idle glyph; otherwise (5) binding presence (`Live`) renders a "live" glyph; otherwise (6) an unbound/dimmed glyph. The spinner is sourced from REAL session activity, never the stale `working` hera role status (BUG-003): a `working` role that is not genuinely active falls through to (5)/(6) and renders a static glyph. `ready_to_close` is read from the task-addressed `task_meta` "hera" namespace, not the hera tables. The status vocabulary (idle/working/blocked/done/failed) is shared 1:1 with the role-status enum so the rail, the plan view, and the gater never drift. -Derived from: `internal/tui/hera/rail.go:514` (`statusIcon`), `internal/tui/hera/model.go:214` (`buildRoleView` reads `ready_to_close`). +Derived from: `internal/tui/hera/rail.go` (`statusIcon`), `internal/tui/hera/model.go` (`RoleView.IsActive`, `buildRoleView` reads `ready_to_close`). #### Scenario: ready_to_close overrides status - **WHEN** a role's bound task carries `meta:hera.ready_to_close=true` AND the role status is working -- **THEN** the row renders the review/ready glyph, not the working glyph +- **THEN** the row renders the review/ready glyph, not the working spinner -#### Scenario: Hera role status drives the glyph +#### Scenario: Genuine activity renders the animated spinner -- **WHEN** a role has a status row of `blocked` and is not ready_to_close -- **THEN** the row renders the needs-input/blocked glyph +- **WHEN** a role holds a live binding whose bound argus task is `in_progress` and is not blocked/done/failed/ready_to_close +- **THEN** the row renders the active spinner's frame (animated), not a static glyph + +#### Scenario: Stale working role-status does not animate + +- **WHEN** a role's hera status is `working` but it is not genuinely active (no live binding, or its bound task is no longer `in_progress`) +- **THEN** the row renders a static glyph (live or dimmed-unbound), not the spinner + +#### Scenario: Blocked outranks activity + +- **WHEN** a role has a status row of `blocked` and is not ready_to_close (even while its bound task is still `in_progress`) +- **THEN** the row renders the needs-input/blocked glyph (static), not the spinner #### Scenario: failed renders the red cross glyph -- **WHEN** a role has a status row of `failed` and is not ready_to_close -- **THEN** the row renders the red ✕ failed glyph distinct from the done glyph +- **WHEN** a role has a status row of `failed` and is not ready_to_close (even while its bound task is still `in_progress`) +- **THEN** the row renders the red ✕ failed glyph (static), distinct from the done glyph, not the spinner #### Scenario: Live-but-statusless role