# 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 ```text 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//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. ```text 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//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.