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

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

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