Require a token for the web UI and reconcile AUDIT.md
The UI is a full control plane: it can create tasks, release or complete them, and inject approval keystrokes into live panes. authz.HTTP did cover it (an absent surface header defaults to Web), but the gate is opt-in and the deployed env sets no tokens while binding all interfaces, so in practice it was reachable unauthenticated from the LAN. Setting the token alone did not work either: a browser cannot put a bearer token on a document load, so the UI would 401 on index.html. - ORCHESTRA_WEB_TOKEN is now mandatory; startup fails rather than silently serving an open control plane. - authz.Sessions issues random values stored SHA-256-hashed, with a TTL, so a leaked snapshot yields nothing usable. - POST /v1/ui/session verifies the token in constant time and returns it as an HttpOnly, SameSite=Strict, Secure cookie. This is a presentable form of the same credential, not a new authority. - HTTPWithSessions accepts that cookie in place of the bearer token, and only for the Web surface. The login endpoint and non-/v1/ GETs (the SPA shell) are exempt by necessity; every /v1/ control path stays gated. Note this is a breaking config change: .orchestra-config/orchestra.env sets no tokens, so the service will not start until it does, and setting a Web token newly gates the other /v1/ surfaces that default to Web. AUDIT.md is reconciled against the code rather than against itself. B14, B15 and B16 are closed with their evidence; B17 is closed on the worker path only; the stale claim that B13 was open is corrected. Adds the previously undocumented command channel and web UI, and files what that implementation pass surfaced: federated approvals emit no event (B19), the local capture revision is a timestamp rather than a change counter and can silently defeat approvals (B20), the command queue never prunes (B21), the ntfy token serves two unrelated purposes (S12), and a dead copy of the authorization policy sits in main.go (S13). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01535A3Y8RtkAi8wYuWhtkEd
This commit is contained in:
kami
@@ -22,18 +22,21 @@ document.
|
||||
The substrate (Layer 1) is solid. Layers 2–4 were originally shaped-but-not-
|
||||
wired; most of the blocking defects below are now closed, verified by
|
||||
reading the current code (not just by trusting this file) and by targeted
|
||||
tests. Two things are not yet proven:
|
||||
tests. Three things are not yet proven:
|
||||
|
||||
1. **A real end-to-end unattended run.** The first live attempt (2026-07-28)
|
||||
surfaced **B12** (fixed and confirmed live, see below), and re-verifying
|
||||
that fix surfaced **B13** (open) — `agent.start` can return success while
|
||||
silently never starting an agent, under back-to-back leases. One task
|
||||
(`wD:p1`) did make it all the way to a real attached `claude` session, so
|
||||
the lease path is provably reachable, but B13 means it's not yet reliable
|
||||
enough to call proven — occupancy/rotation/handoff/completion still
|
||||
haven't been exercised end-to-end against a session that's guaranteed to
|
||||
actually exist.
|
||||
2. **Cross-machine (federation) correctness.** Deliberately deferred — see
|
||||
1. **A real end-to-end unattended run.** The live attempts of 2026-07-28
|
||||
surfaced B12 through B17. All are now closed in code (B17 on the worker
|
||||
path only), but **B13 through B17 were each found live and none has been
|
||||
re-verified live** — occupancy/rotation/handoff/completion still haven't
|
||||
been exercised end-to-end against a session guaranteed to exist. Treat
|
||||
"closed" here as "the code no longer contains the defect," not as
|
||||
operational proof.
|
||||
2. **The operator surface was unauthenticated in the live deployment.**
|
||||
Fixed in code (**B18**: mandatory Web token, cookie sessions for the
|
||||
browser), but the fix is a breaking config change — the service will not
|
||||
start until `.orchestra-config/orchestra.env` sets
|
||||
`ORCHESTRA_WEB_TOKEN`. See B18 and the command-channel section.
|
||||
3. **Cross-machine (federation) correctness.** Deliberately deferred — see
|
||||
"The federation fork" below.
|
||||
|
||||
`go build ./...`, `go vet ./...`, and `go test ./...` all pass.
|
||||
@@ -71,15 +74,116 @@ worktree.
|
||||
| Spec layer | State |
|
||||
|---|---|
|
||||
| L1 substrate (§3, §4) | Built and correct in the main path. |
|
||||
| L2 harness (§5) | Occupancy, rotation, and completion all wired and reachable; B12's readiness race is fixed and confirmed live, but B13 (`agent.start` silently no-op'ing under back-to-back leases) still blocks reliable live verification of what's downstream. |
|
||||
| L2 harness (§5) | Occupancy, rotation, and completion all wired and reachable. B12 fixed and confirmed live; B13–B16 fixed in code but not re-verified live. Unique per-task agent names (B16) and ambiguity-safe prompting (B14) are the substantive changes. |
|
||||
| L3 continuity (§6) | Handoff schema, pickup validation, scratch branches, TASK.md are all wired into the live path (Phase 4 complete). |
|
||||
| L4 surfaces (§7) | Brief/standup/delivery real; quota has a post-hoc producer only (no live push feed yet); authz bypass closed. |
|
||||
| L4 surfaces (§7) | Brief/standup/delivery real; quota has a post-hoc producer only (no live push feed yet); B8's authz bypass closed, but the new web UI reintroduces an equivalent one (B18). |
|
||||
|
||||
---
|
||||
|
||||
## Blocking defect currently open
|
||||
## Blocking defects
|
||||
|
||||
### B14 — `agent.prompt` can duplicate a task prompt after an ambiguous wait timeout (found live 2026-07-28) — open
|
||||
B14, B15 and B16 are now closed by code and covered by tests; none has been
|
||||
re-verified against a live cross-machine run. B17 is closed on the worker
|
||||
path only and remains open for Design A. B18 (the unauthenticated web
|
||||
surface) is closed in code but **requires an env change before the service
|
||||
will start** — see B18 and "What's next".
|
||||
|
||||
Nothing is currently blocking in the sense B12/B13 were. The open defects
|
||||
B19–B21 and S12–S13, all found while implementing B18, are correctness and
|
||||
hygiene gaps in the new command channel rather than things that stop a task
|
||||
from running. The real remaining risk is evidential: B13 through B17 were
|
||||
each found live and none has been re-verified live.
|
||||
|
||||
### B18 — the web UI surface is unauthenticated (found by audit 2026-07-28, uncommitted working tree) — closed (code fix; requires an env change before restart)
|
||||
|
||||
`cmd/orchestra/main.go:205` mounts the browser read/write surface with no
|
||||
handler-local credential check:
|
||||
|
||||
```go
|
||||
mux.Handle("/v1/ui/", ui.Server{Store: s, Workers: workers, Coordinator: coordinator, ...}.Handler())
|
||||
```
|
||||
|
||||
Unlike the worker/harness endpoints on the same mux (`main.go:342, 372,
|
||||
467, 821, 840`), it performs no check of its own and stamps its events
|
||||
`Surface: string(authz.Web)` (`internal/ui/ui.go:222, 337, 368`).
|
||||
|
||||
**It is not, however, ungated in principle.** `authz.HTTP` wraps the entire
|
||||
mux (`main.go:1167`) and defaults a request with no `X-Orchestra-Surface`
|
||||
header to `Web`, so `/v1/ui/` is covered by `ORCHESTRA_WEB_TOKEN`. The
|
||||
defect is that this gate is opt-in — `authz.HTTP`'s own comment says
|
||||
"authentication is optional for local development," and `expected != ""` is
|
||||
the only thing standing between the LAN and the surface.
|
||||
|
||||
**The live deployment has opted out.** `.orchestra-config/orchestra.env`
|
||||
sets no `ORCHESTRA_*_TOKEN` at all, and the server binds `":"+port`
|
||||
(all interfaces, 9145). So on the deployed homesrv instance this surface is
|
||||
in fact reachable unauthenticated from the LAN.
|
||||
|
||||
**Consequence.** Unauthenticated reachable actions include `TaskCreated`,
|
||||
and per-task `release`, `block`, `complete` and `handoff`
|
||||
(`ui.go:345-361`). Most seriously, `grant_approval` / `deny_approval`
|
||||
(`ui.go:301`) terminate in `pane.send_text` against a live pane — so an
|
||||
unauthenticated caller can inject keystrokes into a running agent session on
|
||||
workpc. B14 was filed because Orchestra could *accidentally* advance a
|
||||
permission dialog; this surface allows it deliberately, from the network.
|
||||
|
||||
**Why simply setting `ORCHESTRA_WEB_TOKEN` does not fix it.**
|
||||
`mux.Handle("/", webui.Handler())` serves the SPA from the same listener,
|
||||
behind the same middleware. A browser cannot attach an `Authorization`
|
||||
header to a top-level document load, so setting the token returns 401 for
|
||||
`index.html` and the web UI stops loading entirely. Bearer-token auth and a
|
||||
browser-served SPA are incompatible as currently wired; that is likely why
|
||||
no token is set.
|
||||
|
||||
**Required fix (design decision, not a one-liner).** The browser needs a
|
||||
credential it can actually present on a document load — a login endpoint
|
||||
that exchanges the Web token for an `HttpOnly`, `SameSite=Strict` session
|
||||
cookie, with `authz.HTTP` accepting either that cookie or a bearer token for
|
||||
the `Web` surface. Alongside it: make the token mandatory rather than
|
||||
opt-in whenever the UI is mounted (refuse to start, rather than silently
|
||||
serving an open control plane), and bind to loopback by default so exposure
|
||||
is a deliberate configuration act. Approval commands in particular must not
|
||||
be reachable without it.
|
||||
|
||||
**Fix (2026-07-28):**
|
||||
|
||||
1. **The token is now mandatory** (`cmd/orchestra/main.go`): startup calls
|
||||
`log.Fatal` if `ORCHESTRA_WEB_TOKEN` is empty. Serving the control plane
|
||||
openly is no longer something a missing env var can cause silently.
|
||||
2. **`authz.Sessions`** (`internal/authz/authz.go`) issues 32 random bytes as
|
||||
a session value and stores only its SHA-256, so a leaked snapshot of the
|
||||
map yields nothing usable. Sessions expire (12h default) and are swept on
|
||||
each issue.
|
||||
3. **`POST /v1/ui/session`** verifies the Web token in constant time and sets
|
||||
the value as an `HttpOnly`, `SameSite=Strict`, `Secure` cookie. `Secure`
|
||||
is dropped only if `ORCHESTRA_UI_INSECURE_COOKIE` is set, which is
|
||||
required to reach the UI over plain HTTP.
|
||||
4. **`authz.HTTPWithSessions`** accepts that cookie *in place of* the bearer
|
||||
token, and only for the `Web` surface; the bearer comparison is also now
|
||||
constant-time. Two paths are exempt from the gate, both necessarily: the
|
||||
login endpoint (which performs its own check — gating it would make login
|
||||
unreachable) and non-`/v1/` GET/HEAD, i.e. the SPA shell and its static
|
||||
assets, which are not secrets. Every `/v1/` control path stays gated.
|
||||
|
||||
`TestWebSessionCookieGatesControlPathsOnly` asserts the whole shape: an
|
||||
uncredentialed control path is 401, the shell and login endpoint are
|
||||
reachable, a valid cookie authorizes, a forged one does not, and a cookie
|
||||
presented on the TUI surface is rejected. `TestSessionExpires` covers TTL.
|
||||
|
||||
**Operational precondition — this is a breaking config change.**
|
||||
`.orchestra-config/orchestra.env` currently sets no tokens, so
|
||||
`orchestra.service` will refuse to start until `ORCHESTRA_WEB_TOKEN` is
|
||||
added (plus `ORCHESTRA_UI_INSECURE_COOKIE=1` if the UI is served over plain
|
||||
HTTP). Setting a Web token also newly gates the other `/v1/` surfaces that
|
||||
default to `Web` — any existing unauthenticated client of this instance will
|
||||
begin getting 401s and needs the token too. Do not restart the service
|
||||
without making that env change first.
|
||||
|
||||
**Still open:** the listener binds all interfaces (`":"+port`); loopback-by-
|
||||
default was considered and deliberately not taken, so exposure remains a
|
||||
network-level concern.
|
||||
|
||||
### B14 — `agent.prompt` can duplicate a task prompt after an ambiguous wait timeout (found live 2026-07-28) — closed (code fix; not re-verified live)
|
||||
|
||||
Live Claude E2E task `06FTF8CPH3K3DPN6XQA7G3WRQ8` (pane `wM:p1`) received
|
||||
the same initial `Begin Orchestra task ...` prompt twice. Claude completed a
|
||||
@@ -127,7 +231,29 @@ adds one. In addition, never issue `agent.prompt` while `agent_status` is
|
||||
as requiring explicit operator approval. Do not run further unattended
|
||||
OpenCode E2E tasks until both controls are verified live.
|
||||
|
||||
### B15 — a coordinator-side block cannot reconcile a later live completion (found live 2026-07-28) — open
|
||||
**Fix (2026-07-28), in `Client.Prompt` (`internal/herdr/herdr.go:280`):**
|
||||
|
||||
1. The RPC deadline is now derived from the requested wait, not the client
|
||||
default: when the caller's context has no deadline within `wait + 5s`,
|
||||
`Prompt` installs one (`herdr.go:307-314`). A 60s `wait.until=idle` no
|
||||
longer expires against a 10s transport deadline.
|
||||
2. Retry is split by error class. A JSON-RPC protocol error is herdr's
|
||||
explicit rejection *before* it acted and stays retryable for the bounded
|
||||
boot window; a transport failure after the write is ambiguous and is
|
||||
never resent (`herdr.go:315-325`). Covered by
|
||||
`TestPromptDoesNotRetryAmbiguousDelivery`, which closes the connection
|
||||
mid-`agent.prompt` and asserts exactly one `agent.prompt` is issued.
|
||||
3. The authorization-bypass half is addressed by inspecting the pane before
|
||||
prompting: `Prompt` refuses a pane whose `agent_status` is `blocked`
|
||||
(`herdr.go:285`) and refuses any pane whose transcript matches
|
||||
`permissionPrompt` (`herdr.go:292`). Approval is now an explicit,
|
||||
separately authorized operation — see the command-channel section below.
|
||||
|
||||
Not re-verified live. The original live reproduction was cross-machine, and
|
||||
the "do not run further unattended OpenCode E2E tasks until both controls
|
||||
are verified live" instruction above still stands.
|
||||
|
||||
### B15 — a coordinator-side block cannot reconcile a later live completion (found live 2026-07-28) — closed (code fix; operator-mediated, not re-verified live)
|
||||
|
||||
The same OpenCode E2E task `06FTF9Z8RP5FM4F9M2SKAQNFZ4` was marked
|
||||
`TaskBlocked` when the coordinator's `agent.prompt` RPC timed out, despite
|
||||
@@ -150,7 +276,26 @@ blocker and evidence without pretending it never happened. A live pane must
|
||||
not be left outside the coordinator's session map simply because prompt
|
||||
delivery timed out.
|
||||
|
||||
### B16 — hardcoded harness name makes each harness globally single-instance (found live 2026-07-28) — open
|
||||
**Fix (2026-07-28), in `Coordinator.Start`
|
||||
(`internal/orchestrator/orchestrator.go:911-922`):** when the lease call
|
||||
fails but a pane was actually created, the session is completed with
|
||||
`HerdrID`, `TaskFileSHA` and `ConventionsHash` and persisted via
|
||||
`rememberSession` *before* `block()` records `TaskBlocked`. `Reconcile`
|
||||
(`orchestrator.go:421`) explicitly retains mappings for `StateBlocked` as
|
||||
well as `StateLeased`, so a restart does not orphan them either. The live
|
||||
pane therefore stays observable through `Capture` and the monitor.
|
||||
|
||||
**Scope limit — reconciliation is operator-mediated, not automatic.**
|
||||
Nothing promotes a blocked task to completed on its own; the router still
|
||||
never retries a blocked task. What exists now is the evidence and the
|
||||
surface: the retained session plus the `complete` / `release` / `handoff`
|
||||
actions on the UI task endpoint (`internal/ui/ui.go:345-361`), and the
|
||||
pre-existing versioned `TaskCorrected` workflow (S8) for recording the
|
||||
correction without pretending the block never happened. That satisfies the
|
||||
"must not be left outside the session map" requirement; it does not
|
||||
implement autonomous reconciliation, which remains unbuilt by choice.
|
||||
|
||||
### B16 — hardcoded harness name makes each harness globally single-instance (found live 2026-07-28) — closed (code fix; contract test only partially satisfied)
|
||||
|
||||
While starting the real OpenCode healthcheck task
|
||||
`06FTFDW22833F1CCB8K43Z8808`, Orchestra created worktree/pane `wP:p1` but
|
||||
@@ -172,7 +317,26 @@ digits, `-`, `_`, maximum 32 characters) and persist it in `Session` for
|
||||
subsequent lifecycle operations. Add a contract test that starts two same-
|
||||
harness sessions in distinct panes and verifies both attach.
|
||||
|
||||
### B17 — opaque harnesses must not author canonical handoff anchors (found live 2026-07-28) — open
|
||||
**Fix (2026-07-28):** `agentName(harness, taskID)`
|
||||
(`internal/herdr/herdr.go:452`) derives a bounded, validated name — a
|
||||
per-harness prefix (`oc`/`cl`/`cx`), the lowercased task ID with everything
|
||||
outside `[a-z0-9_-]` collapsed, capped at 32 characters. Over-long IDs keep
|
||||
a leading stem plus an 8-hex-character SHA-256 suffix, because plain
|
||||
truncation made distinct long task IDs collide in herdr's machine-global
|
||||
namespace. `StartAgent` passes it as `name` while `kind` stays the
|
||||
configured harness (`herdr.go:393`), persists it as `Session.AgentName`
|
||||
(`herdr.go:145, 445`), and binds it for prompt routing (`herdr.go:446`,
|
||||
`adapter.go:144`). Pane-scoped operations deliberately keep using
|
||||
`PaneID` — `AgentName` is only for agent-targeted calls (`adapter.go:531`).
|
||||
|
||||
**Requirement not fully met:** the requested contract test does not exist.
|
||||
What is covered is name derivation — `TestAgentNameIsBoundedAndValid` and
|
||||
`TestAgentNameLongIDsDoNotCollide` — plus `TestPromptDoesNotRetryAmbiguous
|
||||
Delivery` asserting the prompt target is the unique name. Nothing yet starts
|
||||
two same-harness sessions in distinct panes and verifies both attach, so the
|
||||
`agent_name_taken` failure mode itself is untested end-to-end.
|
||||
|
||||
### B17 — opaque harnesses must not author canonical handoff anchors (found live 2026-07-28) — closed on the worker path; open for Design A
|
||||
|
||||
The real OpenCode healthcheck run showed that prompting an opaque agent to
|
||||
write the full `continuity.Handoff` schema is the wrong ownership boundary.
|
||||
@@ -228,6 +392,22 @@ attached Claude at 08:00:43Z. The renewed attempt then hit B14's prompt
|
||||
timeout and became blocked. This is expected retry behavior, but illustrates
|
||||
why lifecycle retries must be visible to operators.
|
||||
|
||||
**Status (2026-07-28): satisfied on the worker path, unchanged for Design A.**
|
||||
`cmd/orchestra-worker` now implements the required ownership boundary. The
|
||||
harness is asked once for a bounded semantic report; `releaseReady`
|
||||
(`cmd/orchestra-worker/main.go:180`) then runs `CLIAdapter.Release` and
|
||||
`herdr.HeadSHA` **on the machine that hosts the worktree**, so `HEAD`,
|
||||
branch, dirty paths and SHA-256 values are collected locally and the
|
||||
canonical artifact is validated before publication. The handoff-provenance
|
||||
correction documented at the end of this file supplies the parsing and
|
||||
validation half.
|
||||
|
||||
The homesrv coordinator's Design A path is unchanged and still cannot
|
||||
satisfy this: it does not own the workpc checkout, so any Git-derived fact
|
||||
it seals is derived from the wrong filesystem. B17 is therefore only closed
|
||||
for tasks that actually run through a worker. The standing rule holds —
|
||||
homesrv must not rotate or clean up a non-local worktree.
|
||||
|
||||
### B13 — `agent.start` silently no-ops under back-to-back leases (found live 2026-07-28) — closed (code fix; not yet re-verified live)
|
||||
|
||||
Discovered while live-verifying the B12 fix below. B12 itself is confirmed
|
||||
@@ -380,6 +560,185 @@ the specific shell/agent-readiness race B12 was originally filed for, which
|
||||
|
||||
---
|
||||
|
||||
## Web UI and the capture / approval command channel (uncommitted, 2026-07-28)
|
||||
|
||||
Roughly 520 lines of Go across eight tracked files, plus the untracked
|
||||
`internal/ui`, `internal/webui` and `web/` trees, were added after the last
|
||||
commit and were previously undocumented here. Recording the design so the
|
||||
next session does not have to re-derive it.
|
||||
|
||||
**Capture/command model (`internal/federation/federation.go`).** Two new
|
||||
worker-scoped maps on `Registry`. `PutCapture` stores the latest pane text
|
||||
per (worker, task) and bumps a monotonic `Revision` only when the text or
|
||||
pane actually changes. `Queue` accepts only `grant_approval` /
|
||||
`deny_approval`, and requires a non-zero `CaptureRevision` — a command is
|
||||
bound to the exact capture an operator was looking at. `CompleteCommand`
|
||||
refuses to resolve a command twice.
|
||||
|
||||
**Worker side (`cmd/orchestra-worker/main.go`).** `publishCaptures` pushes
|
||||
recent pane text for every held session; `runCommands` polls
|
||||
`/v1/federation/commands` and, for each command, re-reads the pane,
|
||||
re-publishes it, and refuses with `stale` if the resulting revision differs
|
||||
from the one the command was issued against. `approvalResponse` decides the
|
||||
keystroke: it sends `y`/`n` only for a visible `[y/n]`/`(y/n)` prompt, and
|
||||
sends bare Enter only for OpenCode's fully-labelled `Allow once / Allow
|
||||
always / Reject … enter confirm` selector — where Enter is a bounded
|
||||
one-time grant. It refuses to *deny* through that selector, because doing so
|
||||
would require unobservable navigation. Unknown dialog layouts are rejected
|
||||
outright rather than guessed at. Both branches are covered by
|
||||
`TestApprovalResponseOpenCodeAllowOnce` and
|
||||
`TestWorkerApprovalCommandIsRevisionBoundAndAcknowledged`.
|
||||
|
||||
This is the right shape for B14's authorization half: approval becomes an
|
||||
explicit, revision-bound, separately-issued operation instead of a side
|
||||
effect of prompting, and it executes on the worker that owns the pane rather
|
||||
than over a coordinator-driven remote socket.
|
||||
|
||||
**Coordinator side.** `Coordinator.RespondApproval` and
|
||||
`herdr.ApprovalResponder` (`internal/herdr/adapter.go:602`) are the
|
||||
local-herdr equivalent, applying the same re-read-before-send rule via an
|
||||
`expectedCapture` comparison and the same y/n-only refusal.
|
||||
`Coordinator.RequestHandoff` exposes the handoff request as an operator
|
||||
action without releasing the pane.
|
||||
|
||||
**Known weaknesses of this channel.** The defects found in it are filed
|
||||
individually below — **B19** (federated approvals emit no event), **B20**
|
||||
(the local capture revision is a timestamp, not a change counter) and
|
||||
**B21** (`Registry.commands` never prunes). Two more, not separately filed:
|
||||
|
||||
- **Captures and commands are in-memory only.** `Registry.captures` and
|
||||
`Registry.commands` have no persistence, so a coordinator restart drops
|
||||
pending approvals silently and resets revision counters — which, with a
|
||||
reset counter, could let a stale command match a new capture. This is the
|
||||
other half of B21: the lifecycle forgets across restarts and never forgets
|
||||
within one.
|
||||
- **`RespondApproval` (coordinator path) has no test**, unlike its worker
|
||||
counterpart — and per B20 it is the path whose revision is meaningless,
|
||||
so its text-comparison guard is the only thing actually binding the
|
||||
decision to what the operator saw. That guard is untested.
|
||||
- **`web/` is a full Vite/React app whose build output is embedded** via
|
||||
`internal/webui`'s `go:embed assets/*`. The embedded assets are checked in
|
||||
as build output; there is no documented step tying `web/` sources to a
|
||||
rebuild of `internal/webui/assets`, so the two can silently diverge.
|
||||
|
||||
## Found while implementing B18 (2026-07-28) — not yet fixed
|
||||
|
||||
These were noticed while wiring the session gate. None is fixed; all were
|
||||
verified against the code rather than inferred.
|
||||
|
||||
### B19 — federated approvals leave no audit trail
|
||||
|
||||
`Server.action`'s `grant_approval` / `deny_approval` branch
|
||||
(`internal/ui/ui.go:301-341`) has two exits that are **not** symmetric. The
|
||||
local-coordinator path calls `RespondApproval` and then appends
|
||||
`ApprovalGranted` / `ApprovalDenied` to the store. The federated path calls
|
||||
`s.Workers.Queue(...)`, writes the command to the response, and **returns
|
||||
before appending any event**.
|
||||
|
||||
So the single most safety-critical operation in the system — injecting a
|
||||
keystroke that advances a permission dialog on another machine — is
|
||||
invisible in the event log precisely when it crosses a machine boundary.
|
||||
There is no record of who approved what, or that an approval happened at
|
||||
all. §7.1's authorization model assumes the event log is the record.
|
||||
|
||||
Fixing this needs a decision the local path did not have to make: the
|
||||
command is *queued*, not executed, so the honest event is a request at queue
|
||||
time plus a resolution when the worker acknowledges (`CompleteCommand`
|
||||
already carries `acknowledged` / `rejected` / `stale`). Emitting
|
||||
`ApprovalGranted` at queue time would claim an outcome that has not
|
||||
happened yet.
|
||||
|
||||
### B20 — the local capture revision is a timestamp, not a change counter
|
||||
|
||||
`Server.capture` (`ui.go:80`) fabricates `Revision:
|
||||
uint64(time.Now().UnixNano())` for the coordinator path. The federation
|
||||
path's revision is a real per-change counter (`Registry.PutCapture` bumps it
|
||||
only when pane text or pane ID actually changes), and the worker's staleness
|
||||
check depends on that meaning.
|
||||
|
||||
Two consequences:
|
||||
|
||||
1. The local revision changes on every read, so it conveys nothing about
|
||||
whether the pane changed. The local path is still safe, but only because
|
||||
`RespondApproval` re-reads and compares capture *text* — the revision it
|
||||
reports to the browser is decorative.
|
||||
2. If a task has both a live coordinator session and a published worker
|
||||
capture, `capture()` prefers the coordinator (`ui.go:77`) and returns a
|
||||
timestamp revision, which `action()` then passes to `Queue` as
|
||||
`CaptureRevision`. That value can never equal the worker's counter, so
|
||||
the worker resolves every such command `stale` and the approval silently
|
||||
never happens. The precedence between the two capture sources needs to be
|
||||
explicit rather than incidental.
|
||||
|
||||
### B21 — `Registry.commands` is append-only
|
||||
|
||||
`r.commands[worker]` is only ever appended to (`federation.go:119`);
|
||||
`CompleteCommand` flips a status in place and nothing is ever deleted.
|
||||
Resolved commands accumulate for the process lifetime, and `Commands()`
|
||||
rescans the entire history on every worker poll. Combined with the
|
||||
in-memory-only storage already noted, the lifecycle is wrong at both ends:
|
||||
it forgets across restarts and never forgets within one.
|
||||
|
||||
### S12 — `ORCHESTRA_NTFY_TOKEN` serves two unrelated purposes
|
||||
|
||||
The same env var is the ntfy *server* credential (`main.go:1153`,
|
||||
`delivery.Ntfy{Token: ...}`) and the authz *surface* credential for
|
||||
`authz.Ntfy` (`main.go:1165`). These are unrelated secrets with different
|
||||
trust boundaries: one is handed to a third-party notification server, the
|
||||
other authenticates callers to Orchestra. Setting the former silently makes
|
||||
it a valid inbound credential. They need separate variables.
|
||||
|
||||
### S13 — dead `auth()` middleware in `cmd/orchestra/main.go`
|
||||
|
||||
`func auth(next http.Handler)` (`main.go:1212`) is defined and never
|
||||
referenced; Go does not flag unused functions, so `go vet` stays quiet. It
|
||||
reimplements the notify-only rule that `authz.HTTP` already enforces. It is
|
||||
harmless today and a trap tomorrow — a second, divergent copy of the
|
||||
authorization policy sitting next to the real one. Delete it.
|
||||
|
||||
### Positive note — B8 is more strongly closed than it was
|
||||
|
||||
`authz.HTTP` downgrades a caller-declared `system` surface to `Web` before
|
||||
the token comparison. Previously that landed on a surface whose token was
|
||||
unset in production, i.e. no gate at all. With `ORCHESTRA_WEB_TOKEN` now
|
||||
mandatory, the downgrade lands on a genuinely gated surface, so the B8
|
||||
bypass is closed by construction rather than by the header check alone.
|
||||
|
||||
---
|
||||
|
||||
## What's next
|
||||
|
||||
In dependency order, not importance order:
|
||||
|
||||
1. **Apply the B18 env change** before any restart —
|
||||
`.orchestra-config/orchestra.env` needs `ORCHESTRA_WEB_TOKEN` (plus
|
||||
`ORCHESTRA_UI_INSECURE_COOKIE=1` for plain HTTP), and every existing
|
||||
unauthenticated `/v1/` client of this instance needs the token too. The
|
||||
service will not start otherwise. Nothing else can be tested live until
|
||||
this is done.
|
||||
2. **S13, then S12** — deleting dead policy code and splitting the conflated
|
||||
ntfy token are both small, local, and reduce the chance of the next
|
||||
auth change being made against the wrong copy.
|
||||
3. **B19** — decide the queued-approval event model and give federated
|
||||
approvals an audit trail. This is the largest correctness gap in the new
|
||||
command channel.
|
||||
4. **B20 and B21** — make the capture revision mean one thing, make the
|
||||
coordinator/worker precedence explicit, and give commands a retention
|
||||
policy. B20 in particular can silently defeat approvals.
|
||||
5. **Diagnose the flaky router test** (see open gaps). A nondeterministic
|
||||
assignment test is exactly the failure shape this codebase has hidden
|
||||
real defects behind before; do not paper over it with a retry.
|
||||
6. **Then, and only then, the live re-verification** that B13–B17 all still
|
||||
lack. Each was found live and closed on paper; the code fixes are
|
||||
unproven against a real cross-machine run, and that remains the single
|
||||
biggest gap between this document and reality.
|
||||
|
||||
Deliberately *not* next: building further on Design A's cross-machine calls,
|
||||
and closing B17 for the coordinator path. Both wait on the federation-fork
|
||||
decision below.
|
||||
|
||||
---
|
||||
|
||||
## Closed defects
|
||||
|
||||
Each entry is the flattened final state — design + what's verified — not the
|
||||
@@ -863,10 +1222,31 @@ implemented when nothing can reach it.
|
||||
|
||||
## Known open gaps (as of 2026-07-28)
|
||||
|
||||
- **B13** (above) — `agent.start` can silently no-op under back-to-back
|
||||
leases, with no error surfaced; blocks reliable live verification of
|
||||
everything downstream (occupancy, rotation, handoff, completion) against a
|
||||
real task, even though B12's readiness race is now fixed.
|
||||
Numbered defects are not repeated here — B19, B20, B21, S12 and S13 are in
|
||||
"Found while implementing B18" above, and the ordering across all of them is
|
||||
in "What's next". This list is the unnumbered residue: conditions that are
|
||||
known, accepted, or not actionable as a single fix.
|
||||
|
||||
- **B18's env change is not yet applied.** The code now refuses to start
|
||||
without `ORCHESTRA_WEB_TOKEN`, but `.orchestra-config/orchestra.env` does
|
||||
not set one — `orchestra.service` will fail to start until it does.
|
||||
- **The listener still binds all interfaces**; loopback-by-default was
|
||||
considered and not taken (B18).
|
||||
- **`TestAssignsByAffinityCapabilityAndConcurrency` is flaky** (`no task
|
||||
leased`, roughly 1 run in 10, reproduced with `-count=10`). Predates these
|
||||
changes — confirmed by stashing them — and is unrelated to authz. A
|
||||
nondeterministic router assignment test is exactly the kind of thing this
|
||||
codebase has been bitten by before; it should be diagnosed, not retried.
|
||||
- **B14/B15/B16 are code-fixed but not re-verified live**, and B17 is closed
|
||||
only for tasks that run through a worker. Each was found live, so a code
|
||||
fix plus unit tests is weaker evidence than the failure that produced it.
|
||||
In particular B16 still lacks the requested two-pane same-harness attach
|
||||
contract test, so its actual `agent_name_taken` failure mode is untested.
|
||||
- **B15's reconciliation is operator-mediated only.** Blocked tasks keep
|
||||
their session and are correctable through the UI, but nothing promotes
|
||||
them automatically and the router still never retries a blocked task.
|
||||
- **`web/` build output and `internal/webui/assets` can silently diverge**;
|
||||
no documented rebuild step ties them together.
|
||||
- **Cross-machine lease correctness proven at the primitive level only** —
|
||||
needs an actual homesrv/workpc pair over the real mesh; this repo cannot
|
||||
exercise that by itself.
|
||||
|
||||
Reference in New Issue
Block a user