Reconcile docs with reality; fix module graph, token compare, health #1
kami
Reference in New Issue
Block a user
Delete Branch "webui-and-audit-reconciliation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Acts on the 2026-07-30 senior review (
REVIEW.mdfindings 1, 2, 4, 5, 7). Onecommit,
56f5aac. Nothing was deployed and no live herdr or pane was touched.What changed
Docs (finding 1) —
CLAUDE.mdandAGENTS.mdboth claimed Design B "haszero clients — no worker binary exists".
cmd/orchestra-worker/main.gois thedeployed worker, and the non-local-herdr guardrail has landed in
Coordinator.adapterFor. Both sections rewritten;AUDIT.mdgains a matchingfederation-status record. The Phase 5 retention / Phase 6 deletion decision for
Design A is preserved, not flattened.
clients/un-ignored and tracked — including the.serviceunit and README,since tracking the bridge alone would leave its deployment unit untracked.
Design A is not deleted here.
progress.md(finding 2) — the file was deleted after636ed8a, yetCLAUDE.mdinstructed every session to cross-check against it. Referencesremoved from
CLAUDE.md,AGENTS.md,internal/orchestrator/rotation_test.go(comment only) and
deploy/hooks/orchestra-codex-poll.sh.web/go.mod(finding 4) — a module stub ends the parent package graph atthe directory boundary, so
go list ./...no longer yieldsweb/node_modules/flatted/golang/pkg/flatted. A build tag cannot work: thepackage is in the package list before tags are evaluated.
orchestra-worker(finding 5) — untracked (8.9MB, mode 100755, still ondisk); both binaries now gitignored.
Token compare (finding 7) —
cmd/orchestra/main.go:139,582usesubtle.ConstantTimeCompare, matching theauthz.goidiom. Thetoken != ""guard stays first, so an empty configured token still means auth-disabled
rather than auth-bypass.
Also included from the review pass:
orchestrator.gorecordsadapter-resolution failures in
SessionHealth.LastErrorinstead of droppingthem on a bare
continue, plus anObservedflag, with a covering test.GET /v1/tasks/<id>/healthnow returns a record withlast_errorwhere itpreviously returned a bare
404.Please look closely at
CLAUDE.md/AGENTS.mddiffs. Wholesale section rewrites, and thesefiles steer every future session.
clients/files. Tracking them rests on REVIEW.md's ownunverified assumption that the bridge is still deployed. If it is not, the
correct action was deletion, and this commit tracked dead code. Not
confirmable while the herdrs are down.
REVIEW.md inaccuracies found while doing this
Its second pass claimed every checkable fact held up. Four did not:
AUDIT.mdnever contained the false Design B claim —AGENTS.mdwas thereal second copy.
orchestrator.go:312, not:309.progress.mdsite list misseddeploy/hooks/orchestra-codex-poll.sh:6.orchestra-workerwas tracked;orchestrawas already gitignored.Finding 4's blast radius was also overstated:
Dockerfile.apibuilds./cmd/orchestraby explicit path and.dockerignorealready excludednode_modules, so this was local/CI-only, never a production-image break.Not addressed
!=secret compares remain ininternal/federation/federation.go:343,346,368— on the federated path thedeployment depends on. Tracked separately.
main.go's 30 inline route closures (finding 6) — deferred by the review.blocker: nothing here is verified against live capacity, and the deployed
image predates this commit until
docker compose up -d --buildruns.Verification
go build ./...,go vet ./...,go test ./...andgo list ./... | grep node_modulesall clean with every change appliedtogether — checked in combination, not per-change.
CLIAdapter.Occupancy called a.Usage(s.PaneID), but ClaudeUsage/CodexUsage/ OpenCodeUsage all take a filesystem path to session state, not a herdr pane id. Every call failed with "open <pane-id>: no such file", and Coordinator.rotate silently `continue`d past every failure, so occupancy always looked unmeasurable and rotation never fired. Add herdr.Session.SessionFile and CLIAdapter.resolveSessionFile: - claude: ClaudeSessionFile resolves the transcript by newest-mtime under Claude Code's own encoded project directory (~/.claude/projects/<abs-worktree-with-/-as-minus>/*.jsonl). This is the Phase-1 fallback; the Stop hook's transcript_path (Phase 2) is the authoritative source once wired. - codex: routes through the existing CodexActiveUsage sqlite/rollout discovery instead of the pane id. - opencode: resolution needs a live session id from the SSE/status API, not derivable from the worktree alone — refuses loudly with a pointer to AUDIT.md Phase 1 rather than guessing a path, per the spec's "verify against a live session before wiring any trigger" (§5.2.1). A missing/unreadable session file is now a hard error, not a silent zero-usage Usage{}. SessionHealth gained Occupancy/OccupancyError fields, populated every refreshSessionHealth tick, so GET /v1/tasks/{id}/health makes the number rotation decides on observable before trusting it. Tests: TestClaudeUsageIsLastTurnNotCumulative guards the exact trap named in §5.2.1 (large early-turn total, small last-turn usage -> low occupancy). TestClaudeSessionFileNewestByMtime and TestClaudeSessionFileMissingIsHardError cover the resolver. AUDIT.md B1. Live verification against a real Claude Code session (the spec's own acceptance bar for this phase) still needs to happen on a host with an actual session — not possible from this sandbox.S5: Store.Lease and Store.ExpireLeases both set Event.ID to the task id, so every TaskLeased/TaskReleased event for a given task collided on ID across every lease of that task — unsound for ApplyAdvisory or any future ID-based lookup. Both now call domain.NewID(). S6: Append's TaskCreated dedup path returned nil (success) without appending anything. main.go's handler then did `s.Events(0)[len(s.Events(0))-1]` and returned that — an unrelated event — with 201 Created, and every other Append caller (Gitea poll/webhook, JSONL ingest) had no way to distinguish "duplicate, as expected" from "genuinely appended". Add domain.ErrDuplicate, returned instead of nil on a duplicate (source, external_id). Add Store.TaskBySource to resolve the already-ingested task by that same dedup key. Update every caller: - main.go's POST /v1/tasks now returns 200 with the existing task on ErrDuplicate instead of fabricating a 201 with the wrong event. - provider.Gitea.Poll/IngestWebhook and provider.JSONL.Ingest treat ErrDuplicate as expected (already-seen issue/line), not a failure — without this, Gitea polling would have errored out of its loop on the first already-ingested issue in every batch, since Poll previously relied on the old nil-on-dup behavior to keep scanning. TestLeaseAndExpireEventIDsAreUnique and TestTaskBySourceResolvesDuplicate cover the store-level fixes; TestAppendReplayAndDeduplicate updated for the new error signal. AUDIT.md S5, S6.Verified against a live herdr instance (192.168.1.105:9245) that pane.kill, pane.release, and pane.rotation_signal never existed in the protocol, as AUDIT.md's B5 suspected. Real method list captured in deploy/herdr-schema.json. - Kill now calls the real pane.close({pane_id}). - RotationSignal interface/method/call-site deleted; no real equivalent exists. - Release now refuses loudly instead of calling a nonexistent method — the real pane.release_agent can't return a handoff_ref either way (herdr doesn't write handoffs, the agent does), so a real fix needs Phase 4 handoff production first. Also documents Phase 0 findings in AUDIT.md/progress.md, and adds CLAUDE.md/AGENTS.md with project-specific knowledge (herdr protocol facts, deployment topology, a currently-stuck live task, the federation fork) for future sessions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1rkJ2hBMybnJctPbcy4tTrotate() and TurnDecision now check the agent's own handoff for meta.reason=="manual" before evaluating occupancy/turn-boundary — per spec §5.3, that reason is itself the boundary signal ("a coherent unit finished and the next is independent"), so it bypasses both checks and releases immediately. Extracted the shared release-and-anchor-certify tail into Coordinator.finishRelease so the manual path gets the same anchor safety guarantee as the threshold path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1rkJ2hBMybnJctPbcy4tTNeither harness has a native Stop hook, so both poll for the newest session/rollout file and post to /v1/harness/{complete,turn} the same way Claude's Stop hook does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1rkJ2hBMybnJctPbcy4tTherdr hands a freshly created pane/agent back before it's actually ready, and rejects the very next call with a range of different transient errors ("not an available shell", "not an active named agent", "target ... not found") depending on timing. String-matching each wording as it turned up live proved unwinnable across three live redeploy-and-test rounds, so StartAgent and Prompt now retry any error for up to 15s (bounded by wall-clock time, not attempt count) rather than pattern-matching herdr's error text. Confirmed live against workpc: a fresh lease (wD:p1) now reaches a real attached claude session instead of failing before the agent starts. Live testing also exposed a second, separate defect (B13, documented in AUDIT.md, not fixed here): agent.start can return success while never actually starting an agent when two leases land close together, with no error for a retry to catch. Left three test panes on workpc untouched (wD:p1, wE:p1, wF:p1) pending manual cleanup, per the standing rule against destructive herdr calls without asking first. Also folds in the already-flattened AUDIT.md/progress.md merge that was staged ahead of this session's changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1rkJ2hBMybnJctPbcy4tT@@ -0,0 +1,534 @@# Orchestra — Token-Minimal Unattended Workflow Plandid you see this file?
@@ -0,0 +1,42 @@package mainthose probably should be deleted, because we now utilize "worker"s.
@@ -0,0 +1,66 @@{leave either this one or the jsonc variant.
@@ -0,0 +1,13 @@#!/bin/shcan be removed.
@@ -0,0 +1,84 @@#!/bin/shis this script, claude's and opencode one required?
@@ -0,0 +1,25 @@[Unit]can be removed, we use docker compose now.
@@ -0,0 +1,21 @@#!/usr/bin/env bashcan be removed too.
Review comments addressed —
97a9c65Per-thread replies aren't supported by this Gitea version (the replies endpoint 405s), so all seven are answered here.
2 —
TOKEN_MINIMAL_WORKFLOW_PLAN.md: did you see this file? No. It isn't in this PR's diff, and REVIEW.md never mentions it — norWEB_UI_PLAN.md. Its documentation sweep missed both root-level planning docs, which is a real gap in the review I was acting on. Left untouched: reconciling a 534-line forward-looking plan against AUDIT.md is its own task. It does corroborate the workers direction that comment 4 acts on.4 —
clients/should be deleted. Done. This reverses what the PR originally did (track it) and supersedes the 2026-07-27 AUDIT.md decision to keep Design A through Phase 5. All three docs now say deleted.6 — keep one config example. Kept the annotated
.jsonc(the oneregistry.go:88points at), deletedconfig.example.json. The.jsoncheader said 'Copy config.example.json, not this file' — broken advice once that file went away; rewritten.8 —
docker-api-entrypoint.shcan be removed. You were right and my first pass was wrong. I claimed it sourced the secrets; it does not. Env vars reach the container viaenv_file: .envincompose.yaml, and onlyconfig.jsoncis bind-mounted into/etc/orchestra/— so the script'sif [ -r /etc/orchestra/orchestra.env ]branch never fired. Deleted, withDockerfile.apinowENTRYPOINT ["/app/orchestra"]; line 17 already setsORCHESTRA_DATA/ORCHESTRA_PORT, so no behavior change. CLAUDE.md's claim about the entrypoint sourcing the env file was also wrong and is fixed.10 — are the hook scripts even used? By workers, I assume. Not by workers, and as it turns out not by anything. I first concluded their completion path was a live blocker; your observation that OpenCode QA finished with no 410s disproved that, and the reason is that
orchestra-workerowns completion end to end — it watches.orchestra/done, confirms viaAgentStatusthat the agent isn't busy, then posts through/v1/federation/*with lease epoch and expected version. The hooks used an older, separate convention (.orchestra-report.md) against/v1/harness/complete, already a 410 stub. So they were vestigial, not partly-needed. All three deleted, along with the 410 route, the unmountedharnessCompletionhandler, and its test — a green test against a handler no mux routed to, i.e. the exact pattern CLAUDE.md warns about./v1/harness/turnuntouched.12 —
orchestra.servicecan be removed. Deleted.orchestra-worker.servicekept — different unit, still current.14 —
redeploy.shcan be removed too. Deleted; itsudo installed to/usr/local/binand restarted the retired unit.Also corrected while in here
AGENTS.mdstill described a systemd deployment on homesrv as of 2026-07-27;AUDIT.md's H5 row still claimed a 'retained compatibility handler';deploy/DEPLOYMENT.mdstill namedredeploy.shas the deploy path;deploy/orchestra.env.examplestill citedEnvironmentFile=.Verification
go build ./...,go vet ./...,go test ./...pass after the deletions;go list ./...has nonode_modules. No live herdr or pane touched, nothing deployed — the running image still predates this commit until compose is rebuilt.A task that reached the router's MaxAttempts was permanently terminal. TaskReleased only ever increments Attempt, TaskCorrected could not touch it, and no HTTP route emitted a correction at all. The only way to work an exhausted issue again was to invent a second task for it, which defeats (source, external_id) dedupe and abandons the task's own history. POST /v1/tasks/{id}/retry, full-control surfaces only. It requires the task to be failed, unleased, and failed with reason retry_limit: restoring a retry budget is not an answer to a failure that was not the budget running out. The effect is one TaskCorrected naming that failure, setting state queued and attempt 0 and clearing next_retry_at, failure_class and last_error. Task id, source pair, goal, acceptance, decisions, work phase and artifact refs all stay, and the original failure events stay in the log. operation_id is required and makes the call idempotent, so a repeated request cannot reset an attempt that has since started running. This is RetryTask, not a generic correction endpoint: arbitrary task mutation over HTTP is a different and much larger authority. It also does not address F9, which is an operator releasing a lease someone else owns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>93338b7deployment and F32 f860de4370Two halves of the same failure, live on run 5. F38: the phase brief named .orchestra/research.json and described its contents in prose, never its schema. The agent guessed dead_ends as strings where the decoder wants {tried, why_failed} objects. The brief now carries the shape, and a test decodes each documented shape with the same function the worker uses, so a struct change that is not mirrored fails the build. F39: the local artifact check refused the request through recordError alone. answerRefusedPhase only ran on a coordinator 409, so a decode failure told the agent nothing. The session sat at a boundary rewriting nothing, which is the silent-loop shape the comment above that block warns about, reached by the one path with no delivery. Both local refusals now reach the agent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xsXyr5J1RACo71YeKG3PuF42, live on run 5's completion tail. The review agent wrote .orchestra/done, the worker recognised it, confirmed the agent idle, and then failed the result commit on every attempt: stage result: The following paths are ignored by one of your .gitignore files: .orchestra/done .orchestra carries a .gitignore of "*" (internal/herdr/adapter.go:132), so the marker is ignored, and git refuses an add whose pathspec names an ignored path. The exclusion now names the directory. Reproduced against git 2.55.0 in a scratch repo both ways, and the regression test uses the same constant the worker passes to git. The failure retried every five seconds for 22 minutes with the task stuck in review and nothing observable outside the journal, because each identical error overwrote the single last_error slot. That is F18, still open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVbaKucEYBjMqVeUgJUsc1The plan artifact was Changes{Target,Intent} plus three string lists, every entry capped at 500 single-line characters. That bound makes a specification impossible: a phase cannot carry a code block, a paragraph of reasoning, or a verification command with its own argument list. renderSealed then flattened what little survived through collapse(), so an implement session received a summary of a summary. plan.md replaces it. Markdown, 128 KiB, no per-line cap, sealed through the existing path under the existing PlanRef. The parser enforces the structure the brief states: required sections, phases numbered from 1 with no gaps, Files, Changes and Verification per phase, and at least one automated or manual check, because a phase nobody can verify can never be established as done. Automated entries are JSON argv arrays, so a pipe is a literal argument rather than an operator. Headings inside fenced blocks are content, so a plan may show markdown without parsing its own example. Citations resolve at seal time against the accepted research, on the coordinator, which is the only party holding ResearchRef. A plan resting on a finding nobody recorded fails on the planner while its session is still alive to be told. The plan now renders byte for byte into the implement launch, and a rotated successor receives the same complete document. That is the property the whole change exists for. collapse() stays for research findings, which really are short claims. DecodeStoredPlan reads pre-markdown refs and renders them into the same type, labelled, so nothing downstream branches on which era a plan came from. A legacy plan carries no phases, which is honest: the old artifact never named an executable unit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVbaKucEYBjMqVeUgJUsc1A detailed plan that nothing enforces is a document. This makes the phases executable: the implementer may write exactly one status, and every other status is a conclusion Orchestra reaches by running the plan's own commands. agent may request: ready_for_verification agent may not assert: verified, awaiting_manual_verification, failed, skipped The worker resolves commands from the coordinator, never from the request, so a request cannot smuggle in a command the planner did not write. They run as argv through exec with Dir set to the worktree, which is the quality gate's existing envelope and not a weaker one. There is no shell, so a pipe is a literal argument. Project policy decides executable reach. registry.Project.Verification matches argv positionally, and an absent policy refuses everything: a plan command is agent-authored, so inheriting the operator-authored gate's reach by default would be the wrong direction to fail in. A refused command is refused before anything runs, and the refusal names the project and the command so the planner learns its real reach. Two bindings make the record mean something later. PlanRef, so progress earned under plan A cannot survive into plan B. AtSHA, so "verified" does not outlive the code that made it true: a record whose commit has moved is retained as provenance and rendered as stale, never as a claim about the current tree. Both are the same failure this codebase already fixed for reviews, which bind to the commit they examined. Manual steps hold a phase at awaiting_manual_verification. The sign-off is an ordinary human decision whose subject carries the plan ref and the phase id, so a later "looks good" on an unrelated thread cannot satisfy a gate nobody was discussing. A plan sealed before plan.md declares no executable unit, and says so: the implement context states that phase progress is unavailable and the work continues under the old semantics. Inventing phases it never had would be worse than admitting it has none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVbaKucEYBjMqVeUgJUsc1An implementer that finds the plan contradicted by the code had two options, both bad: work around it silently, or improvise a different plan inside the phase meant to execute one. PlanMismatch is the third. The report carries an observation and nothing else. It may not propose a replacement plan, because writing the next plan is the planning phase's work. requested_action stays advisory: replan, research, or human_decision is a recommendation, and Orchestra decides. Staleness is checked before anything is recorded. A report names the plan ref and the commit it was written against, both filled by the worker from what it can verify rather than from what the agent asserted. A report against an older plan says nothing about the current one, and one against an older tree may already be fixed. Neither is replayed. The reducer keeps two things apart that are easy to conflate: mismatch recorded != plan superseded A plan stops being accepted only when a replacement is actually sealed, so an abandoned replan leaves the accepted plan and its verified progress intact. On a real re-seal the old ref moves to PlanHistory and its progress stops counting, while the verification events stay in the log as provenance. human_decision never reopens. It blocks with a packet stating what was observed and what it contradicts, and a human answer can resolve the contradiction without resealing anything: the plan, its progress and the phase all survive, and the answer outranks the plan where they differ. Turning every ambiguity into a replan would put the planner above the person who set the goal. The backward edge is Orchestra's alone. CanReopenPhase is separate from CanTransitionPhase, which every path validating an agent's request uses, so phase-request.json still refuses a move back. An agent asks by reporting a mismatch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVbaKucEYBjMqVeUgJUsc1View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.