orchestra/REVIEW.md

Orchestra — senior engineering review, 2026-07-30

Second-pass verification (2026-07-30, later session): the factual claims below were independently spot-checked and all held up — progress.md absent, the worker binary 1,131 lines, clients/ gitignored and untracked, node_modules/flatted still in go list ./..., orchestra-worker tracked at 100755, the != token compare at main.go:581, and the Observed/SessionHealth fix plus its test present with build/vet/tests passing. The verdict and priority order are endorsed as written, with the annotated caveats inline below.

Reviewed against the working tree, orchestra-spec (1).md, AUDIT.md, CLAUDE.md, the git history, and the build/test/race suites. No live herdr or pane was touched (CLAUDE.md forbids destructive calls from an audit session without asking first).

Verdict

The code is in better shape than CLAUDE.md warns, and worse shape than AUDIT.md claims. Build, vet, test, and -race all pass. The boldest audit claims were spot-checked and hold up: /v1/harness/complete really is 410, the invented herdr methods (pane.release, pane.kill, rotation_signal, pane.status) are genuinely gone from all call paths, and lease-epoch fencing has real tests. The historical "looks wired but isn't" pattern has largely been paid down.

What has not been paid down is the documentation layer, which has now drifted in the opposite direction — it understates the code. And the project's real blocker is not code at all: it is that nothing has ever been verified live.

First assessment

purpose:              unattended multi-agent task orchestrator; leases coding
                      tasks to CLI harnesses in herdr-managed panes, rotates
                      them across context limits, hands off via git anchors
intended users:       a single operator (the repo author)
actual users:         none yet — no end-to-end path has run live
critical workflows:   lease -> launch -> turn decision -> rotate/release ->
                      handoff -> pickup -> complete
current state:        code-complete per AUDIT.md; zero live verification
known failures:       no verified live capacity (only workpc OpenCode is
                      reachable); release gate correctly still closed
maintenance burden:   16.8k loc Go, 4 binaries, a web UI, 647-line spec, for
                      one operator
technical constraints:herdr JSON-RPC over raw TCP/unix socket, protocol 17;
                      no sudo in this sandbox; Docker Compose deployment
personal constraints: solo project, unattended operation is the whole point
what still works well:the event store (fsync-before-projection, replay from
                      events.jsonl only), lease epoch fencing, the live-
                      captured herdr protocol record
what has become obsolete: Design A federation (clients/herdr-bridge.go), the
                      legacy /v1/harness/complete handler, progress.md
                      references, the tracked binaries

Classification: overbuilt (feature surface far ahead of verified capability) and misaligned (documentation describes a system state that no longer exists, in both directions). Not fragile at the code level, and not abandoned — recoverable with modest, targeted work.

What the project is now

A 16.8k-line Go event-sourced orchestrator for one operator, with a 647-line spec, four binaries, a web UI, and a federation layer — of which zero end-to-end paths have ever run successfully against live capacity. AUDIT.md marks every P0/P1/P2 item "Closed 2026-07-30" on the strength of unit tests, then correctly refuses to clear the release gate because only one harness (OpenCode on workpc) has reachable capacity at all.

What it should become

Narrower, and verified rather than more complete. The next durable improvement is one controlled live run on the one harness that works — not more features, and not more audit rows. Everything below is subordinate to that.

Main findings

1. The documented architectural fork no longer exists, but both docs still describe it

• problem: CLAUDE.md and AUDIT.md both state Design B ("workers pull tasks") is "fully built server-side but has zero clients — no worker binary exists."
• evidence: cmd/orchestra-worker/main.go is 1,131 lines, has passing tests, and is the deployed worker per AUDIT.md's own deployment note. The Design A guardrail also landed — internal/orchestrator/orchestrator.go:309 refuses non-local herdrs. Meanwhile Design A's client, clients/herdr-bridge.go, is gitignored (.gitignore line clients/) and untracked, so the code CLAUDE.md calls "currently deployed" is not in version control.
• impact: CLAUDE.md loads into every session. It actively steers future work toward a fork that is already resolved, and toward preserving untracked code. This is the single highest-leverage inaccuracy in the repo.
• classification: cleanup / deletion
• recommended action: update both docs to state Design B is the live design; delete Design A and clients/ outright, or track it if it is still deployed. Do not leave deployed code untracked.
• risk: low. Deleting clients/ is only safe once it is confirmed undeployed — see Uncertainties.
• second-pass note: outright deletion also conflicts with the standing AUDIT.md decision (2026-07-27) to keep Design A through Phase 5. The safer immediate action is the review's other option: track the bridge now (deployed code must be in version control) and defer deletion to the Phase 6 cutover already decided.
• verification: go build ./... after deletion; confirm nothing references the bridge.

2. progress.md — cited as authoritative by both CLAUDE.md and a test — does not exist

• problem: CLAUDE.md instructs every session to cross-check claims against progress.md; a test comment cites "the highest-priority spec defect noted in progress.md".
• evidence: file absent; last touched in commit 636ed8a, deleted since.
• impact: an instruction every session is told to follow cannot be followed.
• classification: cleanup
• recommended action: remove the references, or restore the log. AUDIT.md already serves this role.
• risk: none.
• second-pass note: AGENTS.md also references progress.md and was missed by the original sweep — add it to the cleanup list alongside CLAUDE.md and internal/orchestrator/rotation_test.go.

3. Silent continue regrew in refreshSessionHealth — repaired

• problem: refreshSessionHealth discarded adapter-resolution errors with a bare continue, contradicting the comment on SessionHealth directly above it, which promises "a resolution/read failure is recorded here rather than silently treated ... by a bare continue."
• evidence: orchestrator.go:357-359 (pre-fix). Occupancy read failures were recorded; resolution failures were dropped. GET /v1/tasks/<id>/health (main.go:721) consequently returned a bare 404 for any session whose herdr this coordinator cannot resolve — indistinguishable from "no such task", with the reason thrown away. That is the normal case for a remote worker-owned session, i.e. the primary federated path.
• impact: operator-facing invisibility on exactly the code path the deployment now depends on. This is the B1/B2 failure mode the repo has a documented history of.
• classification: repair — done in this pass
• why this level of change: the contract was already documented and already had a consumer; only the implementation was missing. No abstraction needed.
• alternatives considered: having the worker report per-task health up through /v1/federation/* instead. Better long-term, but larger, and it does not remove the need for the coordinator to be honest about what it cannot see.
• verification: new test confirmed to fail against the old behavior before passing against the fix.

4. 98MB of node_modules sits inside the Go module, and the mitigation didn't work

• problem: .gitignore documents that node_modules "ships vendored Go packages ... so leaving it merely untracked is not enough."
• evidence: go list ./... still returns orchestra/web/node_modules/flatted/golang/pkg/flatted, and go test ./... reports it. Untracking did not remove it from the build list.
• impact: go build ./... compiles arbitrary third-party Go vendored inside npm packages. Any npm dependency shipping non-compiling Go breaks the entire build for reasons unrelated to this project.
• classification: repair
• recommended action: move web/ out of the module root, or exclude the subtree with a web/go.mod stub — a build-tag barrier will not help, since the package is already in the module's package list.
• verification: go list ./... | grep node_modules must return nothing.

5. A tracked 8.9MB binary

• evidence: git ls-files -s orchestra-worker -> tracked, mode 100755. orchestra is gitignored. Inconsistent.
• impact: repo bloat; a stale committed binary is a deployment-confusion hazard in a project whose CLAUDE.md already warns that running images silently predate commits.
• classification: cleanup
• recommended action: untrack both, gitignore both.

6. main.go is 1,418 lines of 30 inline route closures

• impact: the largest comprehension cost in the repo, and where auth checks are easiest to omit by accident — each closure re-implements its own method check and token check.
• classification: refactor (later)
• recommended action: extract handlers into a server package with shared middleware for method + auth. Do it the next time a route is added, not as a standalone sweep.
• risk: moderate if done as one large sweep; low if done incrementally.

7. Non-constant-time harness token comparison (low)

• evidence: main.go:581 uses != on the Authorization header, while internal/authz/authz.go:111,241 correctly uses subtle.ConstantTimeCompare.
• impact: theoretical only — the port is ufw-restricted to one LAN host. Worth fixing for consistency, not urgency.
• classification: security (low)
• recommended action: use subtle.ConstantTimeCompare.

Keep

Event-sourced store with fsync-before-projection and replay solely from events.jsonl; lease epoch fencing; the deploy/herdr-schema.json live-captured protocol record and the CLAUDE.md herdr protocol notes (these are hard-won and correct); the thin dependency surface (two golang.org/x deps — genuinely disciplined).

Remove

clients/ and Design A cross-machine calls; the tracked binaries; progress.md references; web/node_modules from the Go module graph; the retained /v1/harness/complete compatibility handler once nothing calls it.

Repair now

Findings 1, 2, 4 — all cheap, all currently misleading a future session or breaking a build.

Refactor later

Finding 6. Also consider whether internal/{delivery,operations,admin,ui,webui} (~1,400 loc across five packages) earn separate package boundaries for a single-operator tool.

Rewrite only if

Nothing here justifies a rewrite.

incremental repair cost:      low — findings 1,2,4,5,7 are hours, not days
rewrite cost:                 very high — 16.8k loc plus a 647-line spec
migration cost:               high — a live event log exists and must replay
behavior at risk:             the event store and lease fencing, i.e. the
                              parts that are actually sound
tests available:              full unit + race suite, passing
hidden knowledge:             substantial — the live-verified herdr protocol
                              quirks (number-vs-string protocol version,
                              params:{} requirement, no handoff from release)
compatibility requirements:   must replay the existing events.jsonl
expected maintenance gain:    negligible; the complexity is in the domain

The event log is the hard part and it is sound. Revisit only if live QA shows the rotation state machine is wrong at the protocol level rather than the implementation level.

Changes made in this pass

• internal/orchestrator/orchestrator.go — record adapter-resolution failures in SessionHealth.LastError instead of dropping them; add Observed bool so lease-time-seeded health cannot be mistaken for a live reading.
• internal/orchestrator/rotation_test.go — TestUnresolvableAdapterRecordsObservableSessionHealth, verified to fail without the fix.
• cmd/orchestra/main.go — corrected a comment that still described the retired /v1/harness/complete as handling completion.

Behavior changed: GET /v1/tasks/<id>/health now returns a record with last_error for a session this coordinator cannot resolve, instead of 404. One new JSON field, observed.

Behavior preserved: no change to rotation, leasing, or release decisions. Observed is purely additive.

Verification performed

go build ./..., go vet ./..., go test ./..., and go test -race ./... all pass with these changes. The new test was confirmed to fail against the old bare-continue behavior (unresolvable session recorded no health at all; the resolution failure was swallowed) before passing against the fix — the green run was not taken at face value.

Second-pass note: the "verified to fail without the fix" claim cannot be re-verified from the current tree (the fix is already in), so it rests on the original reviewer's word. Every independently checkable claim in this document was accurate, which lends it credibility.

Verification plan for the outstanding work

1. go list ./... | grep node_modules returns nothing (finding 4).
2. git ls-files | xargs file | grep ELF returns nothing (finding 5).
3. grep -rn 'progress.md' . returns nothing outside this file (finding 2).
4. go build ./... passes after clients/ deletion (finding 1).
5. Then, and only then, the single OpenCode controlled continuity run described in AUDIT.md's QA handoff — since without it every "Closed" row in AUDIT.md rests only on unit tests.

Uncertainties

• Unknown: whether the deployed image contains these fixes. Per CLAUDE.md this needs docker compose up -d --build; nothing was deployed, and sudo is unavailable in this sandbox.
• Unknown (unresolvable from here): live behavior. All six herdrs were unreachable as of 2026-07-29; AUDIT.md reports one OpenCode worker back up on 2026-07-30. No probe was performed.
• Assumption: clients/herdr-bridge.go is genuinely still deployed. If it is not, finding 1 becomes pure deletion.

Next highest-value change

Fix the CLAUDE.md / AUDIT.md federation drift (finding 1) before any further code work — it is what will misdirect the next session.