kami/orchestra
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

v3 workflow: intent, phases, review, submission, enforcement, burn-in

Browse Source 
The v3 stack, previously an uncommitted working tree, plus this session's two
units and the burn-in instrument. This commit is the burn-in build identity:
coordinator and worker must both report this revision before a task is created.

Workflow (earlier sessions, uncommitted until now): human decision events and
reduction, source cursors and reconcile-before-launch, turn-boundary
reconciliation, internal/agentctx as the single renderer, ace-fca phases with
sealed artifacts, the trajectory gate, bounded grilling, independent review,
task pr enforcement, and human review reflection.

Capability restrictions at the agent boundary: an authz.Agent surface at
GatedWrite may ask and may not act. It also fixes two bugs the unit exposed --
gated surfaces could not reach the two endpoints written for them, and
RequestHumanDecision would block an unowned task while rejecting a question
from the session that did own it.

Turn-boundary reconcile-failure escalation: a streak of consecutive failures
asks the session to hand off, fenced on the lease epoch, with reconcile_failure
as a real handoff reason. The worker was dropping the coordinator's verdict on
the floor; it now acts on it.

Burn-in: herdr.WriteLaunchContext dumps the exact agentctx.Build result to
<worktree>/.orchestra/launch.md at every launch, local and federated. BURNIN.md
is the runbook. deploy/build.sh stamps both binaries from one commit.

go build, go vet and go test ./... pass, 20 packages.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
kami
2026-08-26 18:31:20 +04:00
parent 97a9c65302
commit 7f12c7fc37
78 changed files with 16417 additions and 352 deletions
Download Patch File Download Diff File Expand all files Collapse all files
REVIEW.md
+299
View File
@@ -0,0 +1,299 @@
# 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.