fix herdr launch safety and task context
This commit is contained in:
kami
@@ -38,6 +38,36 @@ tests. Two things are not yet proven:
|
||||
|
||||
`go build ./...`, `go vet ./...`, and `go test ./...` all pass.
|
||||
|
||||
### Follow-up live verification (2026-07-28)
|
||||
|
||||
The OpenCode launch path has now been exercised against workpc herdr through
|
||||
an isolated `test-e2e` worktree. The verified outcomes are deliberately
|
||||
narrow:
|
||||
|
||||
- Initial prompts use a bounded retry only after an explicit JSON-RPC
|
||||
rejection (herdr's short post-start readiness window). Transport timeouts
|
||||
and disconnects are never replayed. A pane is inspected first; blocked or
|
||||
permission-dialog panes are refused.
|
||||
- Herdr accepts task-scoped unique names (`oc-<task-id>`); active sessions
|
||||
are visible by that name in `agent.list`. Prompt routing uses that name,
|
||||
while pane reads remain pane-scoped.
|
||||
- The initial launch message carries the task title and description. This is
|
||||
necessary under Design A because homesrv cannot write `TASK.md` inside a
|
||||
workpc checkout. A live OpenCode run received the full instruction and,
|
||||
after explicit operator approval of two file edits, created the requested
|
||||
marker and report without touching other files.
|
||||
- The initial launch does **not** wait for the agent to become idle. Waiting
|
||||
converted an ordinary long-running first turn into a false `TaskBlocked`;
|
||||
a fresh run now remains `TaskLeased` while OpenCode is paused at its normal
|
||||
permission boundary.
|
||||
|
||||
This does not complete the cross-machine continuity design. `clients/herdr-
|
||||
bridge.go` is only a byte proxy; it is not a worker. The worker-side process
|
||||
described below remains required before a remote checkout can author
|
||||
`TASK.md`, derive a canonical handoff anchor, scratch-commit WIP, or safely
|
||||
release it. Until then, homesrv must not rotate or clean up a non-local
|
||||
worktree.
|
||||
|
||||
| Spec layer | State |
|
||||
|---|---|
|
||||
| L1 substrate (§3, §4) | Built and correct in the main path. |
|
||||
@@ -49,7 +79,156 @@ tests. Two things are not yet proven:
|
||||
|
||||
## Blocking defect currently open
|
||||
|
||||
### B13 — `agent.start` silently no-ops under back-to-back leases (found live 2026-07-28)
|
||||
### B14 — `agent.prompt` can duplicate a task prompt after an ambiguous wait timeout (found live 2026-07-28) — open
|
||||
|
||||
Live Claude E2E task `06FTF8CPH3K3DPN6XQA7G3WRQ8` (pane `wM:p1`) received
|
||||
the same initial `Begin Orchestra task ...` prompt twice. Claude completed a
|
||||
first turn inspecting the repository and asking for task context, then began
|
||||
working on the identical prompt again after the turn stopped. This was
|
||||
observed directly in the pane transcript; it is not merely a UI replay.
|
||||
|
||||
**Root cause:** `CLIAdapter.Lease` calls `Client.Prompt` with
|
||||
`wait.until=idle` and a 60-second wait. `Client.Call`, however, applies the
|
||||
client's default 10-second connection deadline when the caller context has
|
||||
no deadline. Once that local read deadline expires, `Prompt` treats the
|
||||
result as any other transient failure and retries `agent.prompt` with the
|
||||
same payload for up to `bootRetryWindow`. Herdr may already have accepted
|
||||
the first request, so this is an ambiguous outcome, not a safe retry.
|
||||
|
||||
**Consequence:** an agent can repeat work, waste context/quota, or overwrite
|
||||
its own in-progress changes. The initial task prompt is especially exposed
|
||||
because it deliberately waits for the agent to become idle.
|
||||
|
||||
**Additional live consequence — OpenCode permission bypass (2026-07-28):**
|
||||
the OpenCode E2E task `06FTF9Z8RP5FM4F9M2SKAQNFZ4` was leased to
|
||||
`workpc-opencode` in `/tmp/test-e2e-worktrees/06FTF9Z8RP5FM4F9M2SKAQNFZ4`.
|
||||
An operator directly observed OpenCode stop at the interactive permission
|
||||
dialog for its initial `git log --oneline -10 && git status --short` command,
|
||||
then advance and run that command without operator input. Workpc herdr's
|
||||
live log records Orchestra's `agent.prompt` calls at 07:37:09 (request IDs 6
|
||||
and 7), 07:37:10 (ID 8), and 07:37:20 (ID 9); the last two only ended when
|
||||
Orchestra's 10-second RPC deadline disconnected the client. Herdr labels
|
||||
each as `changes_ui=true`. The active OpenCode manifest contains only
|
||||
permission-*detection* rules and no auto-approval rule. The task later
|
||||
blocked at the next prompt (`~/.claude/RTK.md`) when no further prompt retry
|
||||
advanced it.
|
||||
|
||||
This does not prove which internal herdr action produced the equivalent of
|
||||
Enter, but it proves the safety boundary is broken: an Orchestra-originated
|
||||
UI-changing `agent.prompt` operation can advance an OpenCode permission
|
||||
dialog. Treat this as an authorization bypass, not merely duplicate work.
|
||||
|
||||
**Required fix:** make the RPC deadline at least the requested herdr wait
|
||||
(plus a small transport margin), and do not blindly resend `agent.prompt`
|
||||
after a timeout or other ambiguous post-write error. Record an observable
|
||||
failure instead, or use an idempotency/acknowledgement mechanism if herdr
|
||||
adds one. In addition, never issue `agent.prompt` while `agent_status` is
|
||||
`blocked` (or a pane read matches a permission dialog); treat the condition
|
||||
as requiring explicit operator approval. Do not run further unattended
|
||||
OpenCode E2E tasks until both controls are verified live.
|
||||
|
||||
### B15 — a coordinator-side block cannot reconcile a later live completion (found live 2026-07-28) — open
|
||||
|
||||
The same OpenCode E2E task `06FTF9Z8RP5FM4F9M2SKAQNFZ4` was marked
|
||||
`TaskBlocked` when the coordinator's `agent.prompt` RPC timed out, despite
|
||||
the agent having received the task. The operator later explicitly approved
|
||||
the one pending `~/.claude/RTK.md` read through herdr; OpenCode completed the
|
||||
verification and became `idle` in pane `wN:p1`. Orchestra still showed
|
||||
`state: "blocked"` because `Coordinator.Start` returned through `block()`
|
||||
and never persisted a session; its monitor therefore has no live pane to
|
||||
observe or reconcile.
|
||||
|
||||
**Consequence:** the event log can permanently claim a task is blocked when
|
||||
the leased harness has actually completed it. This is not merely stale UI:
|
||||
blocked tasks have no normal completion hook path and their receipt/quota
|
||||
record is lost unless an operator manually corrects the lifecycle.
|
||||
|
||||
**Required fix:** retain enough launch/session identity after an ambiguous
|
||||
lease failure to reconcile `agent.get`/completion evidence, or introduce a
|
||||
versioned `TaskCorrected` terminal-state workflow that records the original
|
||||
blocker and evidence without pretending it never happened. A live pane must
|
||||
not be left outside the coordinator's session map simply because prompt
|
||||
delivery timed out.
|
||||
|
||||
### B16 — hardcoded harness name makes each harness globally single-instance (found live 2026-07-28) — open
|
||||
|
||||
While starting the real OpenCode healthcheck task
|
||||
`06FTFDW22833F1CCB8K43Z8808`, Orchestra created worktree/pane `wP:p1` but
|
||||
herdr never attached an agent. A direct retry exposed the actual cause:
|
||||
`agent.start` was sent with `name: "opencode"`, which herdr rejected with
|
||||
`agent_name_taken` because the completed prior E2E agent in `wN:p1` already
|
||||
owned that name. This is not a herdr limit of one OpenCode process per
|
||||
machine: retrying the same `kind: "opencode"` in `wP:p1` with the valid,
|
||||
unique lowercase name `oc-06ftfdw22833f1cc` immediately started a second
|
||||
attached, idle OpenCode session while `wN:p1` remained intact.
|
||||
|
||||
**Root cause:** `Client.StartAgent` passes the harness kind as both
|
||||
`kind` and `name`; every OpenCode lease therefore competes for the same
|
||||
global agent-name slot. The same defect applies to Claude and Codex.
|
||||
|
||||
**Required fix:** keep `kind` as the configured harness, but derive `name`
|
||||
from a validated, bounded task/session identifier (lowercase letters,
|
||||
digits, `-`, `_`, maximum 32 characters) and persist it in `Session` for
|
||||
subsequent lifecycle operations. Add a contract test that starts two same-
|
||||
harness sessions in distinct panes and verifies both attach.
|
||||
|
||||
### B17 — opaque harnesses must not author canonical handoff anchors (found live 2026-07-28) — open
|
||||
|
||||
The real OpenCode healthcheck run showed that prompting an opaque agent to
|
||||
write the full `continuity.Handoff` schema is the wrong ownership boundary.
|
||||
The agent wrote JSON that looked plausible but used `anchor.sha` instead of
|
||||
`anchor.git_sha`, omitted required top-level fields by nesting them under
|
||||
`knowledge`, and could not correctly represent the uncommitted handoff file
|
||||
as a dirty-file hash (a file cannot contain the SHA-256 of its own final
|
||||
contents). Repeated corrective prompts made the handoff less reliable and
|
||||
turned Orchestra's protocol details into agent prompt lore.
|
||||
|
||||
**Required direction:** prompt a harness exactly once for a small semantic
|
||||
handoff/review report (what changed, validation evidence, remaining work,
|
||||
review findings). The worker that owns the checkout must then deterministically
|
||||
collect `HEAD`, branch, dirty paths and SHA-256 values, construct the canonical
|
||||
handoff artifact, and validate it before publication. Canonical handoff files
|
||||
are worker-owned protocol state and must be excluded from their own dirty-file
|
||||
list. Project-level commands and stable expectations (`build`, `test`, file
|
||||
ownership/invariants, etc.) belong in `AGENTS.md`/`CLAUDE.md`; only task-
|
||||
specific evidence belongs in the handoff.
|
||||
|
||||
This cannot be safely implemented in Design A by homesrv: its coordinator
|
||||
does not own the workpc checkout. Expand the bridge into a real worker-side
|
||||
continuity participant (the deferred Design B direction) so all Git-derived
|
||||
handoff facts are gathered and sealed on the machine that hosts the worktree.
|
||||
|
||||
**Live evidence (OpenCode healthcheck/review run, 2026-07-28):** task
|
||||
`06FTFDW22833F1CCB8K43Z8808` initially hit B13/B16: Orchestra created `wP:p1`
|
||||
but its hardcoded `name: "opencode"` conflicted with the prior E2E session.
|
||||
Starting the same `kind: "opencode"` with unique name `oc-06ftfdw22833f1cc`
|
||||
attached successfully without closing the prior pane. The implementation
|
||||
agent created executable `scripts/healthcheck.sh`, validated `bash -n`, its
|
||||
normal mode, and `--help`, then committed `672c123 Add scripts/healthcheck.sh`.
|
||||
It first authored a lookalike handoff (`anchor.sha`, incomplete `meta`, and
|
||||
all protocol fields under `knowledge`). A distinct reviewer in a second pane
|
||||
(`wP:p2`) independently repeated the checks, noticed the nesting error, and
|
||||
rewrote the document. Its claimed schema validation checked only JSON syntax
|
||||
and the presence of keys — it did **not** use `continuity.Decode` or the
|
||||
actual typed contract: `test` remains an array where `Handoff.Test` is a
|
||||
string, and `last_result` remains a string where `Handoff.LastResult` is a
|
||||
structured object. This is a concrete proof that a reviewing harness cannot
|
||||
be trusted to validate canonical protocol state by visual shape alone.
|
||||
|
||||
The task is deliberately still `TaskBlocked` from its original failed
|
||||
automatic attach even though its manually-started implementation/review panes
|
||||
are live, a second live confirmation of B15. The reviewer pane is left open
|
||||
for operator inspection; do not close or release it without approval.
|
||||
|
||||
**Related live observation — unexpected Claude launch explained:** Claude
|
||||
pane `wM:p1` did not start spontaneously. At 08:00:41Z the expired lease for
|
||||
task `06FTF8CPH3K3DPN6XQA7G3WRQ8` emitted `TaskReleased(reason:
|
||||
"lease_expired")`; the router re-leased it at 08:00:42Z, and workpc herdr
|
||||
attached Claude at 08:00:43Z. The renewed attempt then hit B14's prompt
|
||||
timeout and became blocked. This is expected retry behavior, but illustrates
|
||||
why lifecycle retries must be visible to operators.
|
||||
|
||||
### B13 — `agent.start` silently no-ops under back-to-back leases (found live 2026-07-28) — closed (code fix; not yet re-verified live)
|
||||
|
||||
Discovered while live-verifying the B12 fix below. B12 itself is confirmed
|
||||
fixed (see that entry), but re-testing it exposed a second, deeper defect
|
||||
@@ -88,10 +267,35 @@ which requires spinning up more real panes and wasn't done this pass (see
|
||||
`TaskBlocked`), this failure mode leaves the task leased indefinitely against
|
||||
a pane that will never produce a session, with no error surfaced anywhere —
|
||||
worse than B12 was, because nothing currently distinguishes "still booting,
|
||||
give it more time" from "silently dead, will never start." No fix attempted
|
||||
yet; a client-side retry (B12's approach) cannot fix this because the
|
||||
`agent.start` call that should have started the process already returned
|
||||
success.
|
||||
give it more time" from "silently dead, will never start." A client-side
|
||||
retry on `agent.start` itself (B12's approach) cannot fix this, since the
|
||||
call that should have started the process already returned success with no
|
||||
error to retry on.
|
||||
|
||||
**Fix:** since `agent.start`'s own return value can't be trusted, `StartAgent`
|
||||
(`internal/herdr/herdr.go`) no longer treats its success as the end of the
|
||||
story. After `agent.start` returns without error, it polls `pane.get({pane_id})`
|
||||
(new `paneAgentAttached` helper) until the pane reports a real attached agent
|
||||
(`agent` non-empty and `agent_status` present and not `"unknown"`), bounded by
|
||||
a new `agentAttachWindow` (90s, longer than `bootRetryWindow`'s 15s since a
|
||||
legitimate attach was observed live taking "well over a minute"). If the
|
||||
window elapses with no attach, `StartAgent` now returns an explicit error
|
||||
instead of a false success — this is exactly the "recorded/observable
|
||||
failure over silent `continue`" pattern this repo's CLAUDE.md calls for, and
|
||||
should surface as `TaskBlocked` through the same path B12's fix already
|
||||
proved reachable.
|
||||
|
||||
**Not yet done:** re-verified against a live herdr instance (the schema for
|
||||
`pane.get`'s result — `agent`/`agent_status` fields — was inferred from
|
||||
prose in this file's own B12/B13 narration of live `pane.get` output, not
|
||||
re-confirmed by a fresh probe of `192.168.1.105:9245`; `deploy/herdr-schema.json`
|
||||
has no entry for `pane.get`'s result shape). `go build`/`go vet`/`go test
|
||||
./...` all pass, but no test exercises `StartAgent`'s new polling loop
|
||||
directly (existing `internal/herdr` tests don't call `agent.start`/`pane.get`
|
||||
through the fake TCP listener at all). Next session should confirm the
|
||||
`pane.get` field names live before trusting this closes B13 operationally,
|
||||
and ideally fire a fresh back-to-back-lease test against workpc once that's
|
||||
confirmed.
|
||||
|
||||
**Side effect of this investigation — three live orphaned panes on workpc,
|
||||
left untouched on purpose:** `wD:p1` (has a real but abandoned `claude`
|
||||
|
||||
Reference in New Issue
Block a user