The root held four Markdown files that were not in any git repo and were being read as current. CLAUDE_SIDECAR_ISOLATION.md is the one that prompted this: it describes, in the present tense, an agent that dies whenever officer restarts. That was true when it was written and has not been true for two days. Rather than delete analysis that version control was not holding, the two substantial ones moved into platform/docs/ with headers that say what has since happened: - claude-sidecar-isolation.md — stages 0-2 are done and running (R1, R2, R4, R5 all satisfied); stages 3-5 are the only live part. - sidecar-audit-2026-07.md — a snapshot audit, largely executed. Email, pty, music and vnc have been done since; claude, opencode and the cross-cutting notes are still open. The vault stays off-limits. MUSIC_IMAGES_SPEC.md is deleted outright: the sidecar serves /image and /poster, so the spec is the feature. The workspace root now holds one Markdown file, CLAUDE.md, which is where cross-cutting operational reality belongs. Also corrected, in the same pass: - root CLAUDE.md listed email as "the big one, and untouched" and pty stage 4 as outstanding; both are done. It now names what actually remains (claude stages 3-5, the terminal orphan leak) and what landed. - docs/sidecar-topology.md said "nothing built yet". Two sidecars now serve their own transport; what has NOT happened is the part the document is about — fixed ports, the shared table, .env toggles — so it says exactly that rather than implying the design is underway. Migration order updated: pty and email went first, not music. - docs/navigation-audit.md is cited as authoritative but still planned work on Projects, a feature since deleted. A status header marks H3 void, H1/H2 done and H4 the one open item, so nobody follows it into a directory that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1378 lines
93 KiB
Markdown
1378 lines
93 KiB
Markdown
> **Historical — a snapshot audit, largely executed.**
|
||
> Two-pass audit from 2026-07-30. Most of what it recommends has since been done: email moved into its
|
||
> sidecar entirely (store, routes, accounts, resync and both syncs), pty became a byte relay onto the
|
||
> sidecar's own listener, music's cliamp pipeline moved out of the platform, vnc stopped reading the
|
||
> owner's password, and every HTTP sidecar now shares one proxy factory. Still open: **claude** (stages
|
||
> 3–5), **opencode**, the frontend-speaks-slskd smell, and the cross-cutting notes on the protocol and
|
||
> the registry. **The vault is off-limits by standing instruction.**
|
||
> Read it for the reasoning and the line counts, not as a description of today.
|
||
|
||
# Sidecar architecture — the rule, and where we currently break it
|
||
|
||
Written 2026-07-30, after an audit of the Soulseek work. This is a findings document, not a plan:
|
||
nothing here has been changed. Discussion pending.
|
||
|
||
## The rule
|
||
|
||
> The main Officer process is a **thin auth proxy, forever**. Each sidecar owns everything related to
|
||
> its job. The custom UI exists so the upstream service (slskd) stays unforked while everything
|
||
> custom is implemented on top of it.
|
||
|
||
Three consequences worth stating explicitly, because the audit turned on the third one:
|
||
|
||
1. The platform holds **no credentials** for the upstream service, and no knowledge of its API.
|
||
2. The platform's router for a sidecar has **no routes of its own** — it authenticates, injects
|
||
`X-Officer-User`, forwards, and returns the response untouched.
|
||
3. **The sidecar, not the browser, owns the upstream contract.** A thin proxy that forwards raw
|
||
upstream calls to a frontend that knows the upstream's URLs and JSON shapes has moved the
|
||
coupling rather than removed it. This is the part we are currently getting wrong.
|
||
|
||
## What is compliant (verified)
|
||
|
||
`src/servers/api/slskd/` is **70 lines total** and does exactly the two things it should:
|
||
|
||
| File | Lines | Role |
|
||
|---|---|---|
|
||
| `router.ts` | 51 | `all('/*')` catch-all. Forwards subpath + query + body, injects `X-Officer-User`, streams the response back. No routes of its own. |
|
||
| `sidecar-server.ts` | 19 | Remembers the port the sidecar reports on connect (`slskd:server`). Nothing else. |
|
||
|
||
- `SLSKD_URL` / `SLSKD_API_KEY` are read in **exactly one file**: `src/servers/sidecar/slskd/upstream.ts`.
|
||
The platform never sees either.
|
||
- `hono.ts` contains 3 slskd lines: two imports and `protectedRouter.route('/slskd', slskdRouter)`.
|
||
- `protocol.ts` contains 1: the `slskd:server` port message.
|
||
- No platform-server file imports any `soulseek*` query from `officerdb`.
|
||
|
||
Today's three commits (`8032c8b`, `b7b91a2`, `dea9ee2`) touched **zero** platform-server files.
|
||
|
||
### Not a violation, but worth naming
|
||
|
||
The `soulseek_*` tables live in the shared `officer_db` package (`schema/soulseek.ts`,
|
||
`queries/soulseek.ts`) rather than in the sidecar. Only the sidecar reads them — this was a
|
||
deliberate call (one database, schema isolated in its own file, `soulseek_` prefix) and it stands.
|
||
The cost to remember: `bun db:push` diffs the *whole* schema, which is why soulseek DDL is
|
||
hand-applied.
|
||
|
||
## The smell: the frontend speaks slskd
|
||
|
||
**37 raw `/slskd/api/v0/…` calls from React, against 10 `/slskd/_officer/…` calls.**
|
||
|
||
| File | Raw slskd calls |
|
||
|---|---|
|
||
| `SoulseekTransfers.tsx` | 8 |
|
||
| `SoulseekRooms.tsx` | 6 |
|
||
| `SoulseekChat.tsx` | 5 |
|
||
| `SoulseekDashboard.tsx` | 4 |
|
||
| `SearchView.tsx` | 4 |
|
||
| `SoulseekSystem.tsx` | 3 |
|
||
| `SearchResults.tsx` | 3 |
|
||
| `useSoulseekUser.ts` | 2 |
|
||
| `SoulseekUploads.tsx` | 2 |
|
||
|
||
The proxy stays thin, so rule (1) and (2) hold. Rule (3) does not: `shared.ts` defines slskd's wire
|
||
types (`SlskdUserStatus`, `SlskdUserInfo`, `SlskdTransferUser`, …), and the panels know slskd's URL
|
||
shapes, field names and failure modes. If slskd renames a field, the break lands in React.
|
||
|
||
Three places where it's not just a pass-through call but **domain logic that belongs in the sidecar**:
|
||
|
||
### 1. `useSoulseekUser.ts` — introduced today (`b7b91a2`)
|
||
|
||
Fans out to `/users/<u>/status` and `/users/<u>/info`, then reconciles in the browser: both
|
||
rejected → "couldn't reach them"; one rejected → show the half we got. That "what does a partially
|
||
reachable peer look like" decision is Soulseek domain knowledge living in a React hook.
|
||
|
||
Target shape: `GET /_officer/users/<u>` in the sidecar does both calls, merges, and returns one
|
||
Officer-shaped peer. The hook becomes a single fetch that doesn't know slskd exists.
|
||
|
||
### 2. `SoulseekDashboard.tsx`
|
||
|
||
Four parallel calls (`/application`, `/transfers/downloads`, `/transfers/uploads`, `/searches`)
|
||
aggregated client-side into dashboard figures. The aggregation is the product; slskd just supplies
|
||
inputs. Belongs behind one `/_officer/dashboard`.
|
||
|
||
### 3. `SoulseekTransfers.tsx`
|
||
|
||
Eight call sites, including state rollups across users and multi-delete fan-outs
|
||
(`Promise.all` over per-file `DELETE`s). Bulk operations over an upstream that has no bulk endpoint
|
||
is precisely the "custom on top of unforked slskd" the sidecar exists for — and doing it from the
|
||
browser means a closed tab is a half-finished operation.
|
||
|
||
The remaining panels (`Rooms`, `Chat`, `SearchView`, `SearchResults`, `Uploads`, `System`) are
|
||
mostly genuine 1:1 pass-throughs. They still leak slskd's shapes into the UI, but there's no
|
||
composition logic to move — lower priority, and arguably fine to leave until they grow one.
|
||
|
||
## Where the line actually is
|
||
|
||
Worth agreeing on before any refactor, since "everything through `/_officer/*`" and "proxy raw
|
||
where it's 1:1" are both defensible:
|
||
|
||
- **Sidecar route** when the answer involves more than one upstream call, any merging or
|
||
reconciliation, any Officer-only data (Postgres), any bulk operation, or any decision about what
|
||
a partial failure means.
|
||
- **Raw proxy** when it is one upstream call, forwarded, rendered — no composition of any kind.
|
||
|
||
Under that line, the three files above are the work. The other six are a naming/typing question
|
||
(does the UI import `SlskdRoom`, or an Officer type?) rather than an architecture one.
|
||
|
||
## Reference: the compliant pieces of the Soulseek sidecar
|
||
|
||
`src/servers/sidecar/slskd/` — what "the sidecar owns its job" already looks like:
|
||
|
||
| File | Role |
|
||
|---|---|
|
||
| `index.ts` | Reverse proxy to slskd on a random loopback port; documents the whole `/api/slskd/*` contract; reports its port to the platform. |
|
||
| `upstream.ts` | The only holder of `SLSKD_URL` / `SLSKD_API_KEY`. |
|
||
| `officer.ts` | The `/_officer/*` routes — favourites, browse snapshots, tree levels, filtered search, downloads. Features slskd has no concept of. |
|
||
| `browse.ts` | Multi-minute background share-tree fetch, tree built at ingest. Outlives any request. |
|
||
| `download.ts` | Expands a browsed folder into a file list from cache and enqueues it. `MAX_ENQUEUE` guard. |
|
||
|
||
Other sidecars for comparison: `claude`, `email`, `music`, `opencode`, `vault`, `vnc`.
|
||
|
||
---
|
||
|
||
# Pass 1 — backend / API code
|
||
|
||
One section per sidecar: what logic still runs in the main `officer` process. Frontend is deliberately
|
||
excluded here; it gets its own pass below. Every claim carries a `file:line` so nothing has to be
|
||
re-derived later.
|
||
|
||
The eight sidecars, from `ecosystem.config.cjs`:
|
||
|
||
| PM2 process | Entry point |
|
||
|---|---|
|
||
| `officer-claude` | `src/servers/sidecar/claude/index.ts` |
|
||
| `officer-opencode` | `src/servers/sidecar/opencode/index.ts` |
|
||
| `officer-email` | `src/servers/sidecar/email/index.ts` |
|
||
| `officer-pty` | `src/servers/api/terminal/pty-sidecar.mjs` ← note the path |
|
||
| `officer-vnc` | `src/servers/sidecar/vnc/index.ts` |
|
||
| `officer-music` | `src/servers/sidecar/music/index.ts` |
|
||
| `officer-vault` | `src/servers/sidecar/vault/index.ts` |
|
||
| `officer-slskd` | `src/servers/sidecar/slskd/index.ts` |
|
||
|
||
## music — partially compliant
|
||
|
||
`/api/music/*` is a near-exact clone of the slskd proxy and is **clean**. The violation is the
|
||
**cliamp audio subsystem**: the same domain — playing local audio to the browser — implemented
|
||
entirely in the main process, with no sidecar owning any of it.
|
||
|
||
| Surface | Lines |
|
||
|---|---|
|
||
| Compliant proxy: `api/music/router.ts` + `api/music/sidecar-server.ts` | 88 |
|
||
| `hono.ts` (3) + `protocol.ts` (1) | 4 |
|
||
| `api/cliamp/websocket.ts` | 201 |
|
||
| `api/cliamp/audio-ws.ts` | 91 |
|
||
| `server.tsx` PulseAudio bootstrap | 46 |
|
||
| `api/cliamp/asoundrc` + `server.tsx` cliamp wiring | 18 |
|
||
| **Audio domain in the main process** | **~356** |
|
||
|
||
1. **cliamp player process management** — `api/cliamp/websocket.ts:1-201`. Locates the `cliamp` binary
|
||
by probing `Bun.which` plus three GOPATH candidates (`:48-66`), resolves and traversal-validates the
|
||
requested file against the owner's real home (`:40-46`, `:85-98`), shell-escapes it (`:68`, `:102`),
|
||
then spawns `script -qfc '<cliamp> <file>' /dev/null` to fake a PTY (`:106-113`) with
|
||
`PULSE_SINK: 'virtual_out'` and `ALSA_CONFIG_PATH` injected. Pumps stdout/stderr into JSON frames
|
||
(`:124-160`), forwards `{type:'input'}` to stdin (`:173-185`), kills the child on close (`:187-198`).
|
||
Child processes are held in a module-level `Map` (`:22`).
|
||
*Belongs in* `sidecar/music/`, which already runs its own loopback HTTP server
|
||
(`sidecar/music/index.ts:140`). *Obstacle:* a browser-held WebSocket with bidirectional keystroke
|
||
traffic — but the relay pattern already exists twice (`server.tsx:164-228` for dev-server,
|
||
`server.tsx:323-326` for vault).
|
||
2. **PulseAudio host-daemon bootstrap** — `server.tsx:391-436`. A startup IIFE that locates
|
||
`pulseaudio`/`pactl`, runs `pulseaudio --start -D` if the daemon is down (`:401-411`), then greps
|
||
`pactl list short sinks` and loads `module-null-sink sink_name=virtual_out` if absent (`:414-435`).
|
||
Runs unconditionally at every boot even if nobody opens the player.
|
||
*Belongs in* the music sidecar's startup. *Obstacle:* none technical — same host, `pactl` works
|
||
identically. Must move together with (1) and (3), since the sink must exist before they start.
|
||
3. **Host audio capture → browser PCM** — `api/cliamp/audio-ws.ts:1-91`. Spawns
|
||
`parec --format=s16le --rate=44100 --channels=2 -d virtual_out.monitor` (`:28-33`) and pushes each
|
||
chunk to the browser as a binary frame (`:44-73`). Hardcoded format, sample rate, channel count and
|
||
monitor device name — pipeline domain knowledge. *Obstacle:* continuous binary PCM, so a relay hop
|
||
costs a copy per chunk.
|
||
4. **ALSA config shipped inside the API tree** — `api/cliamp/asoundrc:1-9`, passed via
|
||
`ALSA_CONFIG_PATH` (`websocket.ts:8`, `:112`). Upstream config in the thin-proxy process. Moves for
|
||
free — it's resolved by `__dirname`.
|
||
5. **Music-domain param in the WS envelope** — `server.tsx:253`, `:265`, `WSData.files` at `:53`.
|
||
`upgradeWs` plumbs a `files` query param that exists only for this feature. The JWT verify +
|
||
blacklist check in `upgradeWs` (`:231-272`) is legitimate platform work; the feature-specific
|
||
parameter is not.
|
||
6. **Route names leaking into the proxy** — `api/music/router.ts:29-36`. The catch-all special-cases
|
||
`/reindex` and `/reindex/stream` by name to call `server.timeout(req, 1800)`. Minor, but the slskd
|
||
reference router is uniform for every path. Simple fix: extend the timeout for the whole prefix.
|
||
7. **Done — the scope list is generic now** (2026-07-30, `17b1da2`) — `_middlewares/origin-validation.ts`.
|
||
The finding was that the music app's origin was allowlisted by hand and its scope named the feature,
|
||
with `NON_OWNER_PATHS = ['/api/auth', '/api/music']` backstopping a music-only account class. None of
|
||
it belonged in the sidecar — this is authorisation, i.e. the platform's actual job, and an origin is
|
||
**not a credential** — so the only cleanup available was making the scope list generic instead of
|
||
naming music. That is what landed:
|
||
|
||
**Any `OFFICER_<APP>_ORIGIN` in the environment is an app origin, and its rule is derived from its own
|
||
name** — `/api/auth` (it has to sign in) plus `/api/<slug>`. So `OFFICER_MUSIC_ORIGIN` (renamed from
|
||
`MUSIC_APP_ORIGIN`) gets `/api/music` without being mentioned in code, and adding an app is adding an
|
||
env var. Two declared exceptions, because they are not that convention: `OWNER_ONLY_APPS` (`APP` — the
|
||
main app is the whole platform rather than one feature, so full access but owner-only) and
|
||
`APP_SCOPE_OVERRIDES` (`TAIL` → `/api/vpn`, the one app whose name and API surface differ).
|
||
|
||
`NON_OWNER_PATHS` stays hand-written, and should not be mistaken for the music app's rule even though
|
||
the value matches: it is the **account** backstop, enforced whatever Origin a caller claims or omits,
|
||
and it is the airtight half of the pair. The origin rules are defence in depth on top of it.
|
||
|
||
Verified clean: no music credential or library path in the main process (`MUSIC_ROOT` exists only in
|
||
`sidecar/music/indexer.ts:24`); all range/206/`Content-Range` handling is sidecar-side
|
||
(`stream-audio.ts:77-91`); `queries/music.ts` (231 lines) has exactly one consumer,
|
||
`sidecar/music/index.ts:25-39`.
|
||
|
||
**Adjacent, flagged for the file-browser pass rather than counted here:**
|
||
`api/file-browser/router.ts` does real audio work in the main process — `/audio-meta` shells out to
|
||
`ffprobe` twice for ID3 tags and USLT/SYLT lyrics sniffing (`:459-523`), `/extract-audio` transcodes
|
||
video→mp3 with `ffmpeg -codec:a libmp3lame` (`:909-942`), `/audio-tracks` probes streams (`:407-455`),
|
||
`ensureAudioRemux` remuxes for range-seekable playback (`:80-132`). Same kind of work the music
|
||
sidecar owns, but it belongs to the file-browser contract.
|
||
|
||
## vault — partially compliant, and the placement is security-relevant
|
||
|
||
The transport proxy is right; the platform owns the entire Vaultwarden **auth/session/key-custody**
|
||
domain. This is the worst offender of the eight, and the one where placement has real consequences.
|
||
|
||
| Surface | Lines |
|
||
|---|---|
|
||
| `api/vault/router.ts` | 169 |
|
||
| `api/vault/websocket.ts` | 164 |
|
||
| `api/vault/broker.ts` | 79 |
|
||
| `api/vault/token-store.ts` | 34 |
|
||
| `api/vault/proxy-util.ts` | 29 |
|
||
| `api/vault/sidecar-server.ts` | 24 |
|
||
| **`api/vault/` total** | **499** |
|
||
| `hono.ts` (7) + `server.tsx` (8) + `protocol.ts` (2) + auth handlers (8) | 25 |
|
||
| `officerdb` vault layer the platform calls: `queries/vault.ts` + `crypto.ts` + `schema/vault.ts` | 176 |
|
||
| **Total** | **~702** (~525 excluding the shared DB package) |
|
||
|
||
For scale: the sidecar itself is 295 lines and is a genuine dumb pass-through
|
||
(`sidecar/vault/index.ts:153-183` streams bodies verbatim; `upstream.ts:12-22` is the only reader of
|
||
`VAULTWARDEN_URL`).
|
||
|
||
Two structural notes before the findings:
|
||
- `api/vault/router.ts:123` *is* an `all('/*')` catch-all, but it is not thin — it **replaces** the
|
||
`Authorization` header with a platform-held upstream credential (`:137-141`) and implements
|
||
401-refresh-retry (`:158-165`).
|
||
- It does **not** inject `X-Officer-User` (contrast `api/slskd/router.ts:34`,
|
||
`api/music/router.ts:50`). The vault sidecar receives no user identity at all — only a bearer token.
|
||
Everything below follows from that one inversion.
|
||
|
||
1. **Vaultwarden token broker in the main process** — `api/vault/broker.ts:1-79` +
|
||
`api/vault/router.ts:52-104`. `broker.ts:46-57` builds an OAuth `grant_type=password` form
|
||
(`username=email`, `password=authHash`) and POSTs it to `/identity/connect/token` (`:15-20`);
|
||
`:60-66` does the `refresh_token` grant. `router.ts:52-104` is a hand-written `POST /session/login`
|
||
that parses the body, calls `passwordGrant`, extracts the token pair, persists it and assembles the
|
||
crypto-material response. Pure Vaultwarden protocol knowledge, including its version-dependent
|
||
casing quirks (`broker.ts:69-73`) and inconsistent error-message locations (`:27-31`).
|
||
**Security-relevant:** the master-password-derived `authHash` (`router.ts:57`, `broker.ts:50`)
|
||
transits main-process memory, and the main process is what mints and holds the upstream token set.
|
||
"Platform holds no upstream credentials" is violated directly here.
|
||
2. **Token custody + refresh in the main process** — `api/vault/token-store.ts:1-34`, called at
|
||
`router.ts:137`, `:158-165`, `websocket.ts:73`. Loads the decrypted token set from Postgres, applies
|
||
a 60s expiry skew (`:8`, `:30`), re-derives and persists a new pair on expiry (`:11-24`).
|
||
This is the direct analogue of the slskd API key — which the platform deliberately never sees.
|
||
**Security-relevant:** plaintext access + refresh tokens exist as JS strings in the main process on
|
||
every proxied request.
|
||
3. **Notifications WebSocket proxied twice, with token injection** — `api/vault/websocket.ts:1-164`.
|
||
`injectToken` (`:45-52`) rewrites the SignalR query string: drops `token`, sets `access_token=<vw
|
||
token>`. `open` (`:55-118`) verifies the platform JWT, fetches the upstream token, dials
|
||
`ws://127.0.0.1:<sidecarPort>` and runs a full buffered bidirectional pipe — **which the sidecar
|
||
already implements** (`sidecar/vault/index.ts:45-114`, `:143-151`). Frames are relayed twice.
|
||
*Obstacle:* Bun requires a synchronous upgrade, hence the deferred validation at `:60-70`; that
|
||
pattern stays, the token lookup at `:73` should not.
|
||
4. **The platform is the vault's key escrow** — `api/vault/router.ts:107-120`. `PUT /unlock-key`
|
||
persists a `wrappedKey` (`:111`); `GET /unlock-key` hands it back to any owner session (`:117-119`).
|
||
This is the protector key that unwraps the on-device wrapped user key — i.e. what opens the vault
|
||
without the master password (`schema/vault.ts:22-25`).
|
||
**Security-relevant, sharpest one:** combined with (2), one compromised main process yields both the
|
||
transport token and the key material that decrypts the vault, and the read side is a plain endpoint
|
||
gated only by the platform session.
|
||
5. **Vault at-rest crypto executes in the main process** — `officerdb/src/crypto.ts:1-41`, called from
|
||
`queries/vault.ts:22-23,34-35,67-68,83,88`. Derives an AES-256-GCM key as
|
||
`SHA-256(VAULT_STORE_KEY)` (`:12-20`) and runs `createCipheriv`/`createDecipheriv` (`:23-39`).
|
||
Because the vault router imports `officerdb` (`router.ts:10`, `token-store.ts:1`), all of this runs
|
||
inside `officer`. *Obstacle:* `crypto.ts` lives in the shared package, so it is importable from
|
||
anywhere; moving it means moving the vault queries out of the shared package or enforcing a
|
||
sidecar-only import boundary. No config obstacle — both processes read the same `.env`.
|
||
6. **Vault tables are read/written by the platform, not the sidecar** — `queries/vault.ts:18-101`,
|
||
`schema/vault.ts:10-32`, exported globally at `officerdb/src/index.ts:130-138`. Every current caller
|
||
is main-process: `router.ts:78,111,117,159`, `token-store.ts:17,28`, plus (7). The vault sidecar
|
||
imports **no** DB module at all. Exact inverse of the intended ownership.
|
||
7. **Auth flows reach into vault storage directly** — `api/auth/signout.ts:10`,
|
||
`revoke-handler.ts:18-19`, `panic-handler.ts:15-16`. Signout deletes the token row; distress and
|
||
panic also burn the protector key. The *policy* is platform-level; the *mechanism* — direct DELETEs
|
||
against the sidecar's tables — is not. All three are already best-effort `.catch(() => {})`, so
|
||
failure semantics wouldn't worsen behind a sidecar call.
|
||
8. **Dead weight** — `router.ts:38-48` is a hand-written `GET /_health` passthrough the catch-all
|
||
would cover for free. `proxy-util.ts:1-29` is a byte-for-byte duplicate of the sidecar's
|
||
`stripHopByHop`/`redactPath` (`sidecar/vault/upstream.ts:33-58`), identical `HOP_BY_HOP` set and
|
||
`REDACT` regex. The redaction exists only because the main process handles token-bearing query
|
||
strings — fix (2) and it's unnecessary.
|
||
9. **Mounted outside the protected tree** — `hono.ts:73-77`. `route('/api/vault', …)` sits outside
|
||
`protectedRouter`, so the router re-implements its own stack (`router.ts:31-33`: `originMiddleware`,
|
||
`userMiddleware`, `ownerGate`). *Real constraint, probably why:* it deliberately avoids
|
||
`bodyParser()` so bodies stream (`router.ts:14`), and `protectedRouter` would inherit it from
|
||
`hono.ts:88` and buffer vault attachments.
|
||
10. **Stale comments on a security boundary** — `hono.ts:73-76` and
|
||
`origin-validation.ts:36-39,61-64` both claim vault requests "carry their own Bitwarden bearer
|
||
token, not a platform session JWT" and that `userMiddleware` would 401 them. Untrue since
|
||
`router.ts:32-33` requires a valid platform JWT *and* owner status on every request. Also,
|
||
`VAULT_AUTH_SPEC.md` (cited at `router.ts:12`, `schema/vault.ts:4`) and
|
||
`BITWARDEN_SIDECAR_PROMPT.md` (cited at `sidecar/vault/upstream.ts:3`) **do not exist** in the repo.
|
||
Not logic, but exactly the drift that makes someone loosen a gate by mistake.
|
||
|
||
Verified clean: `protocol.ts:61-62` carries only `{type:'vault:server', port}` — no secrets cross the
|
||
registration socket. **No other platform code and no other sidecar reads vault secrets** — grepped
|
||
every `officerdb` vault export across `src/servers` and `src/databases`; the only consumers are
|
||
`api/vault/*` and the three auth handlers. No MCP tool, queue job, or agent path pulls vault items.
|
||
User-key derivation is genuinely client-side (the platform only relays `Kdf*` params at
|
||
`router.ts:96-101`) — the one credential decision that is correctly placed.
|
||
|
||
*Shortest path to compliance (inferred, not attempted):* move `broker.ts`, `token-store.ts`,
|
||
`/session/login`, `/unlock-key`, the vault queries and `crypto.ts` into `sidecar/vault/`; have the
|
||
router inject `X-Officer-User` instead of `Authorization`; reduce `websocket.ts` to
|
||
origin-check + verify + upgrade + dumb pipe; delete `/_health` and `proxy-util.ts`; replace the three
|
||
auth-handler DB calls with a sidecar call. Result: a slskd-sized router plus `sidecar-server.ts`, and
|
||
no plaintext token or key buffer anywhere in `officer`.
|
||
|
||
## claude — not compliant. ~2,700–3,050 platform lines against slskd's 70
|
||
|
||
The largest violation after email, and the one with the worst consequences, because it is the reason a
|
||
`pm2 restart officer` kills a running agent. **The process-topology half of this has its own document
|
||
— `CLAUDE_SIDECAR_ISOLATION.md`.** This section is the inventory of domain logic; that one is the fix
|
||
for survivability. They overlap deliberately.
|
||
|
||
There is **no `/api/claude` mount, no proxy router, and no `X-Officer-User` anywhere on this path.**
|
||
Nothing here is shaped like slskd. The platform does not forward to the claude sidecar; it *drives* it,
|
||
over a typed RPC vocabulary, and interprets everything that comes back.
|
||
|
||
A structural fact worth stating before the list, because it inverts the usual reading: **the sidecar
|
||
imports its own event type from the platform.** `sidecar/protocol.ts:1` and
|
||
`sidecar/claude/user-instance.ts:121` both import `ChatEvent` from `../../api/chat/types`. The
|
||
dependency arrow points the wrong way — the sidecar is a client of the platform's domain model rather
|
||
than the owner of it. Every other item below is downstream of that.
|
||
|
||
1. **`api/chat/websocket.ts:1-636` — the turn loop.** The single biggest concentration. The default
|
||
model is decided here (`DEFAULT_MODEL = 'claude-code'`, `:16`), the harness is chosen by string
|
||
test (`isClaudeModel`, `:20`), the prompt is constructed here (`:336-338`), cwd is resolved here
|
||
(`:42-82`), and `:407-435` encodes the first-turn-vs-subsequent-turn distinction (whether to pass
|
||
a resume id) — a fact about the Claude CLI's contract living in the platform. `:564-589` owns stop
|
||
semantics. `:443-518` is shared with opencode.
|
||
2. **`api/chat/websocket.ts:168-312` — a ChatEvent state machine.** 145 lines interpreting the
|
||
sidecar's event stream: mutating tool-call records in place (`:247-254`), accruing cost
|
||
(`:279-282`), and persisting via `appendChatEvent` (`:103-116`) with replay at `:612-629`. This is
|
||
the machinery that makes a restart lossy — it is the durable writer, and it sits on the far side
|
||
of the socket from the process producing the events.
|
||
3. **`api/chat/claude-sessions.ts:1-361` — a reimplementation of Claude's transcript format.** The
|
||
platform reads and *writes* `~/.claude/projects/<slug>/<uuid>.jsonl` directly: the slug encoding
|
||
(`:39`), the entry schema (`:69-78`), content-block decoding (`:143-227`), listing (`:350-361`),
|
||
delete-by-unlink (`:253-258`), a 32KB `readSync` plus a `"cwd":"…"` regex to recover a session's
|
||
directory (`:294-306`), and — the sharpest example — **rename implemented by appending a
|
||
`{type:'summary'}` entry and reconstructing `leafUuid`** (`:265-287`). The platform is writing
|
||
into another program's private on-disk format. If the CLI changes that format, `officer` breaks.
|
||
4. **`api/tasks/pipeline-executor.ts:98-249, 585-586` — a second, independent interpreter.** Its own
|
||
`refreshProxyToken()` POSTing to `127.0.0.1:${ANTHROPIC_PROXY_PORT}/refresh` (`:98-105`), its own
|
||
model branch (`:126-129`), its own ChatEvent interpretation (`:151-201`), its own prompt builder
|
||
(`:257-268`), and `:585-586` silently coerces any model to `claude-code`. Two divergent
|
||
implementations of the same protocol is the cost of not having a sidecar boundary.
|
||
5. **`sidecar-registry.ts:198-274` — the platform spawns the agent.** `ensureClaudeSidecar` /
|
||
`spawnAndWaitForRegistration`: a per-email `Bun.spawn` of `user-instance.ts` with
|
||
`stdout: 'inherit', stderr: 'inherit'` (`:240-241`), a `claudeProcs` Map, a `claudeSpawnWaiters`
|
||
Map, a 15s timeout and a 50ms registration poll (`:259-267`). Plus the claude verbs at `:306-358`
|
||
and a broadcast fallback at `:339-346`. `:88-90` uses `capabilities.includes('proxy')` as a
|
||
stand-in for "is this the claude sidecar", which is only true by accident of naming.
|
||
6. **`generate-container-context.ts:135-182` (+ `:50-133`) — the platform writes the CLI's config.**
|
||
It authors `~/.claude/settings.json`: a `Stop` hook curling
|
||
`localhost:5000/api/hooks/claude-done` (`:144-153`), `defaultMode: 'bypassPermissions'` and
|
||
`skipDangerousModePermissionPrompt: true` (`:155-156`), and pre-seeded `hasTrustDialogAccepted`
|
||
(`:163-179`). Called from `users/provision.ts:28-38`. Two notes: the hook points at the platform,
|
||
so it fails during exactly the restart window that matters; and the permission posture is a
|
||
deliberate documented choice (`platform/CLAUDE.md`: agents run unsandboxed as the owner) that is
|
||
being *implemented in the wrong process*, not a mistake.
|
||
7. **`api/activity/router.ts:1-191` — the platform walks the agent's scratch tree.** Reads
|
||
`/tmp/claude-<uid>/<encoded-cwd>/tasks/<id>.output` (`:24-61`, keyed on
|
||
`startsWith('claude-')` at `:34`) and tails it over SSE (`:116-191`). Another private layout the
|
||
platform has learned. (`/announce`, `:103-113`, is unrelated and fine.)
|
||
8. **`api/chat/session-manager.ts:1-152` — a mirror of sidecar state** in the platform (`:29-32`),
|
||
with its own GC (`:126-138`) and teardown (`:83-88`). Two copies of the same truth.
|
||
9. **`api/chat/chat.ts:1-123` — session CRUD** (`:33-85`) plus hardcoded provider names (`:88-97`).
|
||
(`/chat/stt`, `:100-123`, is Whisper — a different domain that happens to live here.)
|
||
10. **`api/chat/list-models.ts:5-9` — the model catalogue is hardcoded in the platform:**
|
||
`claude-code/opus|sonnet|haiku` with context windows, max tokens and reasoning flags. This is the
|
||
sidecar's knowledge by definition; it changes when the CLI changes, not when Officer does.
|
||
11. **`api/server-settings/claude-code.ts:1-80` — installing and authenticating the CLI from the
|
||
platform:** `curl … install.sh | bash` (`:24`), `claude auth login` (`:45-47`),
|
||
`auth status` (`:56-61`), `--version` (`:9, :71`).
|
||
12. **`channels/send-claude-code.ts:1-68` — a third execution path**, running the SDK in-process, with
|
||
a subscription-lifetime rule at `:53-58`.
|
||
13. **`sidecar/protocol.ts:14-19, 34-43, 68-103` — nine claude message types in the shared protocol,**
|
||
including `ClaudeState` (`:70-73`, which exposes `proxySecret` and the whole session map) and
|
||
`ClaudeSpawnParams` (`:77-96`). Domain vocabulary in what should be transport.
|
||
14. **Smaller, still real:** `channels/send-and-await.ts:6, 53, 75-78`;
|
||
`hono.ts:79-86` — the `/api/hooks/claude-done` endpoint is **unauthenticated** (deliberate, since
|
||
the hook has no credential, but it means an unauthenticated local caller can nudge chat state);
|
||
`hono.ts:119`; `api/chat/retention.ts:1-22` (started from `bootstrap.ts:17`);
|
||
`queries/chat-events.ts:1-29` (read only by the platform);
|
||
`mcp-tool-server.ts:1-296`; `api/chat/logger.ts:1-62`.
|
||
|
||
**Dead code on this path, verified unimported (≈193 lines):** `api/task-logger.ts:1-101`; and
|
||
`api/anthropic-proxy.ts:1-92`, which reads `~/.claude/.credentials.json` (`:6, 18-27`) and mints
|
||
`sk-ant-api03-<uuid>` keys (`:10`) — the live copy is `sidecar/claude/proxy.ts:135`. A stale second
|
||
implementation of the credential path is worth deleting on security grounds alone, not just tidiness.
|
||
|
||
*What the sidecar already has right:* the Anthropic proxy genuinely lives in the PM2-managed sidecar
|
||
(`sidecar/claude/index.ts:20`), so the platform never holds an API key at rest, and `ANTHROPIC_BASE_URL`
|
||
points at the sidecar (`sidecar-registry.ts:234`). The credential path is roughly correct. It is the
|
||
process topology, the transport direction and the domain logic that are not.
|
||
|
||
## email — not compliant, and it is the exact inverse of the pattern
|
||
|
||
The worst of the eight by volume, and the only one where the arrow points backwards end to end:
|
||
**≈3,238 platform lines** (2,875 of them in seven files) against a **314-line sidecar** — and the
|
||
sidecar *imports platform code back out* (`sidecar/email/email-idle.ts:3` imports
|
||
`../../api/email/resync`). There is no proxy router, no `email:server` port event, and no forwarding of
|
||
any kind. `emailRouter` implements 18 concrete endpoints itself.
|
||
|
||
Read plainly: the sidecar is a cron/IDLE trigger, and the platform is the mail client.
|
||
|
||
1. **`api/email/email-db.ts:1-746` — the entire mail store lives in the platform.** SQLite schema
|
||
(`:9-55`), migrations with a `thread_id` backfill (`:167-217`), an FTS5 index (`:223-251`), a
|
||
hand-written **Gmail-style query parser** (`:253-349`), threading (`:81-132`), and — the part that
|
||
is unambiguously protocol knowledge — RFC-2047 decoding, quoted-printable, base64, charset
|
||
conversion, multipart walking and attachment extraction (`:496-746`). `openEmailDb` (`:134-154`)
|
||
carries a documented race at `:138-139`.
|
||
2. **`queue/handlers/gmail-sync.ts:1-712` — Gmail sync runs in the main process.** Gmail REST calls
|
||
(`:103-213`), label mapping (`:65-96`, `:329-396`), an IMAP first-sync path (`:398-546`), OAuth
|
||
refresh (`:586-608`). It runs in `officer` because `server.tsx:385` calls `initQueue()` →
|
||
`queue/init.ts:6` → `handlers/index.ts:1-2`. So a full mailbox sync competes with request serving,
|
||
and dies on restart.
|
||
3. **`queue/handlers/email-sync.ts:1-381` — the IMAP sync, same placement.** `imapflow` (`:118`), a
|
||
label map (`:48-74`), UIDVALIDITY tracking, a ten-attempt reconnect loop (`:134-335`), a
|
||
Message-Id hash (`:40-44`), OAuth refresh (`:78-107`), and dock mutation (`:353-377`). A
|
||
ten-attempt reconnect loop is precisely the kind of long-lived stateful work sidecars exist for.
|
||
4. **`api/email/email.ts:1-414` — sending and reading.** SMTP send (`:16-69`) where the SMTP host is
|
||
derived by **string-munging the IMAP host** (`:24`); an SSE hub (`:94-146`); conversation
|
||
collapsing (`:167-202`); thread fetch (`:246-285`); row shaping (`:204-228`); attachment
|
||
extraction (`:287-318`); contacts (`:72-92`); stats (`:362-414`).
|
||
**Two unparameterised SQL interpolations:** `labels LIKE '%${folder}%'` at `:174` and `:366`.
|
||
The `folder` value reaches those lines from request input. Worth checking before anything else in
|
||
this section — it is the one item here that is a defect rather than a placement problem.
|
||
5. **`api/email/resync.ts:1-309` — shared mutable coordination across a process boundary that cannot
|
||
work.** `gmailResync` (`:43-63`), `imapResync` (`:148-264`), `resolveImapAuth` (`:118-146`),
|
||
`refreshCredentials` (`:21-41`), and `performResync` (`:276-284`) which coalesces concurrent
|
||
resyncs through an **in-process Map**. It is imported by both `sidecar/email/email-cron.ts:2` and
|
||
`email-idle.ts:3` *and* by `accounts.ts:159` — i.e. by two different processes. Each gets its own
|
||
copy of the Map, so the coalescing silently does nothing across the boundary. This is what
|
||
"importing platform code back out" costs.
|
||
6. **`api/email/accounts.ts:1-259` — account setup does live IMAP.** Validation by real connection on
|
||
create (`:87-121`), provider dispatch (`:180`), `resolveAuth` (`:231-259`), stale-status
|
||
reconciliation (`:38-85`), and **raw credentials written into queue job metadata** (`:198`,
|
||
`:200`) — so mail passwords land in a Postgres job row.
|
||
7. **Three channel handlers duplicate ~218 lines of email logic each:**
|
||
`channels/telegram/handler.ts:48-119` (`:53`, `:87`, enqueue at `:65`),
|
||
`channels/discord/handler.ts:47-122` (`:53`, `:89`),
|
||
`channels/whatsapp/handler.ts:52-121` (`:57`, `:91`). Same work, three times, none of it in the
|
||
sidecar.
|
||
8. **`api/integrations/google-auth.ts:1-61` + `integrations.ts:162-194` — a second Gmail client.**
|
||
Token exchange (`:6-33`), refresh-and-persist (`:38-61`), and at `integrations.ts:162-194` an
|
||
independent Gmail REST proxy. `:15-19` requests the `https://mail.google.com/` scope — full
|
||
mailbox access — and `:307-317` mutates the dock.
|
||
9. **`api/chat/websocket.ts:57-70` — `resolveEmailCwd` mkdirs email account directories** from inside
|
||
the chat socket. Cross-domain reach: the chat path owns email's filesystem layout.
|
||
10. **`data-path.ts:44-50` — the platform owns the mail store's paths**, with a second consumer at
|
||
`sidecar/claude/user-instance.ts:40, 75` (the `email_db` MCP tool). So three processes agree on a
|
||
layout by convention rather than by asking one owner.
|
||
11. **Glue (routing only, would survive a proxy):** `hono.ts:34` + `:114`; `server.tsx:20` + `:85-89`;
|
||
`protocol.ts:50-51` carries only `email:new`, which is correctly transport-shaped.
|
||
12. **`src/servers/sidecar/email-cron.ts` — 92 dead lines**, imported by nothing (the live one is
|
||
`sidecar/email/email-cron.ts`).
|
||
|
||
*Shortest path (inferred):* this one is a rewrite, not a move. The realistic first step is not
|
||
relocating `email-db.ts` — it is deleting the duplicate clients (items 7 and 8) and moving the two
|
||
queue handlers (items 2 and 3) into the sidecar so sync stops dying with `officer`. The store itself
|
||
can follow later, behind a proxy router.
|
||
|
||
## opencode — not compliant. ≈792 platform lines (≈1,424 counting the credential file)
|
||
|
||
Structurally the same failure as claude, at a third of the size — and with the largest proportion of
|
||
**dead code** of any sidecar: roughly 330 lines that can be deleted outright, because the sidecar
|
||
already does the same job. That makes this the cheapest of the non-compliant surfaces to improve.
|
||
|
||
1. **`api/chat/opencode/client.ts:1-208` — a full HTTP + SSE client in the platform**, including a
|
||
hand-rolled SSE parser (`:48-96`), REST wrappers (`:98-167`), wire shapes (`:171-195`), the
|
||
`metadata.officer` convention (`:179-182`), and a comment pinning it to **"opencode 1.17.9"**
|
||
(`:170`) — a version dependency in `officer`'s source. **`:16-96` and `:108-139` are dead.**
|
||
2. **`api/chat/opencode/event-mapper.ts:1-140` — entirely dead.** The sidecar already maps events at
|
||
`sidecar/opencode/runner.ts:150-205`. This is a stale duplicate of a sidecar concern.
|
||
3. **`api/server-settings/chat-providers.ts:1-632` — provider credentials handled in the platform.**
|
||
Reads and writes `~/.pi/agent/auth.json` (`:10`, `:334-346`) and `models.json` (`:11`, `:61-73`),
|
||
exposes API keys over GET/PUT (`:384-406`), and calls `api.anthropic.com`, `api.openai.com` and
|
||
`opencode.ai/zen/v1/models` with raw keys (`:444-471`). `PROVIDERS` at `:353-367`;
|
||
`invalidateModelCache()` at `:284, 293, 328, 404`; `:474-551` is stale `pi` install logic.
|
||
**Open question before moving any of it:** `AGENT_CONFIG_DIR = ~/.pi/agent` (`data-path.ts:23-24`)
|
||
may be dead config — I did not verify that the current opencode binary reads that path at all. If
|
||
it doesn't, a large part of this file is elaborate no-op and should be deleted rather than moved.
|
||
4. **`api/chat/opencode-sessions.ts:1-113` — transcript reconstruction** (`:50-82`), filtering on
|
||
`metadata.officer.cwd` (`:21`), epoch→ISO conversion (`:29-30`), and a synthesised
|
||
`opencode/<modelID>` model string (`:84`). Same shape as `claude-sessions.ts`: the platform reading
|
||
another program's session store.
|
||
5. **`api/chat/websocket.ts:443-518` (+ `:6, 18-20, 345-348, 570-584`) — `handleOpenCodeChat`**,
|
||
including deriving a session title from the prompt (`:490`) and selecting the harness by string
|
||
test.
|
||
6. **`channels/send-opencode.ts:29-66` — the terminal-event set (`:33-37`) and a resume policy keyed
|
||
on the `ses_` id prefix (`:42-45`).** Protocol knowledge encoded as a string prefix, in the
|
||
platform.
|
||
7. **`api/chat/list-models.ts:2, 11-59` — fetches `/config/providers` and then invents capability
|
||
metadata for the results (`:42-45`).**
|
||
8. **`api/chat/chat.ts:13-19, 44, 56-57, 68, 80-81, 91` — CRUD dispatch on `isOpenCodeSessionId`**
|
||
(`opencode-sessions.ts:112`, a `startsWith('ses_')` test).
|
||
9. **`sidecar-registry.ts:360-392` — typed opencode RPC**, same anti-pattern as the claude verbs.
|
||
10. **`sidecar/protocol.ts:20-22, 52-58, 105-113` — CLI flag mapping in the shared protocol**
|
||
(`--dir`, `--model`, `--session`). The platform's wire format encodes the sidecar's command line.
|
||
11. **More dead code:** `api/chat/opencode/server-manager.ts:7-23` (`isServerHealthy`, unused);
|
||
`api/chat/opencode/state.ts:13-15` (`clearOpenCodeSession`, unused). `state.ts:1-15` otherwise
|
||
holds a sessionKey→`ses_` Map — sidecar state mirrored in the platform again.
|
||
|
||
**The one compliant piece:** `api/chat/opencode/sidecar-server.ts:12-26` memoises the port the sidecar
|
||
reports — the slskd `sidecar-server.ts` pattern, already present and working. The proxy that should sit
|
||
on top of it was never written. (`:19-21` adds a session subscription, which is domain logic that
|
||
doesn't belong in a port memo.)
|
||
|
||
Verified clean: **no `OPENCODE_*` env var, no binary path, and no CLI spawn in the main process** — the
|
||
process boundary itself is right here, unlike claude. It is only the data and protocol knowledge that
|
||
leaked.
|
||
|
||
## pty — not compliant. 202 platform lines (228 with protocol) against a 73-line reference
|
||
|
||
The smallest violation in absolute terms and the **least compliant in proportion**: essentially the
|
||
entire platform-side file is sidecar logic. It is also the most tractable, because the fix template
|
||
already exists in the same codebase.
|
||
|
||
1. **`api/terminal/websocket.ts:1-169` — the whole file.** Broken out:
|
||
- **`:59-68` — the platform builds the `PtyInitConfig`:** `process.env.SHELL ?? '/bin/zsh'`,
|
||
`args: ['-i']`, and `host: true` **hardcoded** at `:61`. That last one means the sidecar's own
|
||
sandbox branch (`pty-sidecar.mjs:138-151`) is unreachable — dead code kept alive only by a
|
||
constant on the wrong side of the boundary.
|
||
- **`:38-43` — `resolveCwd` tilde expansion** in the platform.
|
||
- **`:71-89` — per-session fan-out.** The platform subscribes to a **global** `pty:output` /
|
||
`pty:exit` stream, filters by `msg.sessionId === sessionId`, and rewraps each frame. This is the
|
||
hard part of any fix, and **the template already exists** at `server.tsx:164-228`
|
||
(`devServerWebsocket` — a real byte relay).
|
||
- **`:106-143` — inbound translation**, including `:135` **synthesizing a shell command**:
|
||
`` `cd ${JSON.stringify(msg.path)}\r` ``. The platform is typing into the user's shell.
|
||
- **`:145-153` — detach-not-kill on disconnect.** Correct policy, and worth preserving: this is
|
||
why terminals survive a browser reload. Note that **`pty:close` has no sender anywhere in the
|
||
platform**, so there is currently no kill path at all.
|
||
- **`:51-54`, `:97-103` — ANSI-framed error text** composed in the platform.
|
||
- **`:23, 25-28, 30-36` — a duplicated session Map, `nextId` and `sendOutput`.**
|
||
- **`:158-169` — `broadcastPanelRefresh`**, a Claude concern parked in the terminal module; its
|
||
only caller is `hono.ts:83` (the `claude-done` hook).
|
||
2. **A placement anomaly worth fixing on its own:** `ecosystem.config.cjs:27-32` is the only sidecar
|
||
**outside `src/servers/sidecar/`**, the only **`.mjs`**, and the only one run by **`node`** rather
|
||
than `bun`. It has zero relative imports (`pty-sidecar.mjs:13-19, 31`) and does **not** use
|
||
`sidecar/connect.ts` — it carries **66 duplicated lines** (`:232-297`) including a byte-identical
|
||
backoff table (`:37` vs `connect.ts:22`). Its types are JSDoc (`:39`), so `protocol.ts:145-156` is
|
||
unenforced against it. The actual blocker to moving it is mundane: sibling `templates/` files on
|
||
disk (`:27-29, 61, 66, 71` — `.zshrc`, `.tmux.conf`, `starship-officer.toml`, and an unused
|
||
`.zshenv`). So a `git mv`, not a rewrite. *(Inferred: the `.mjs`/node choice is probably a
|
||
node-pty native-addon workaround — corroborated by the comment at `api/cliamp/websocket.ts:100`.)*
|
||
3. **Every PTY byte transits the main process, double-JSON-encoded.** Plus terminal-specific query
|
||
parsing in the shared upgrade handler (`server.tsx:248-252`, `WSData:51-52`) and wiring at `:7, 38,
|
||
143, 234, 335`. Auth at `:236-246` is correct. Identity ships **inside the payload** as
|
||
`userLabel` / `sessionId` (`websocket.ts:56, 65`) instead of as `X-Officer-User`.
|
||
4. **`sidecar-registry.ts:395-407` plus PTY types threaded through generic plumbing** at
|
||
`:12-13, 35, 93, 125, 153, 171, 180, 194`. One subtlety to preserve: the 30s
|
||
`sendPtyCommandAsync` timeout at `websocket.ts:96` is load-bearing backpressure for `pty:ready`.
|
||
5. **`hono.ts` has no terminal router at all** — `terminal` appears only at `:42`. There is nothing to
|
||
thin down; there is something to create.
|
||
6. **`data-path.ts` is bypassed:** terminal uses `process.env.HOME!` (`websocket.ts:63-64`) rather
|
||
than `getOwnerHomeDir` (`data-path.ts:34`), unlike the eight other host-executing surfaces. Same
|
||
result on this machine (`HOME_DIR` is set and equals `HOME`), divergent anywhere it isn't.
|
||
|
||
*Shortest path (inferred):* `git mv` the sidecar into `src/servers/sidecar/pty/` with its templates,
|
||
switch it to `connect.ts`, move the `PtyInitConfig` construction and cwd resolution into it, and
|
||
replace `websocket.ts` with the `devServerWebsocket` relay shape. The detach-on-disconnect policy moves
|
||
with it.
|
||
|
||
## vnc — not compliant. ~239 platform lines, 170 of them in `api/desktop/`
|
||
|
||
Structurally closer to compliant than most, and blocked on one concrete missing piece rather than a
|
||
pile of leaked logic.
|
||
|
||
1. **`api/desktop/websocket.ts:1-122` — session lifecycle plus a raw RFB TCP tunnel.** `startVnc`
|
||
(`:21-25`), `Bun.connect` to the VNC port (`:35-37`), both pump directions (`:41`, `:103`), a
|
||
`pendingMessages` buffer for pre-connect frames (`:9, 73-76, 97-100`), and close code `4004`
|
||
(`:30, 67, 87`).
|
||
**The blocker:** the vnc sidecar **exposes no HTTP or WS listener at all** — there is no
|
||
`Bun.serve` anywhere in `sidecar/vnc/`, and no `vnc:server` port event, unlike `slskd:server`
|
||
(`protocol.ts:64`), `vault:server` (`:62`) and `music:server` (`:60`). So the platform cannot
|
||
forward to it even in principle; the TCP tunnel exists because there is nothing else to talk to.
|
||
Adding a listener is the prerequisite for everything else here.
|
||
2. **`sidecar-registry.ts:409-439` — five VNC verbs.** `stopVnc` (`:427`) has **no callers**.
|
||
3. **`sidecar/protocol.ts:23-28, 44-49, 115-129` — four commands and five events**, with
|
||
`VncStartParams` (`:117-121`) and `VncSessionInfo` (`:123-129`) exposing `display` and `pid` to the
|
||
platform — internal process detail crossing the boundary.
|
||
4. **`api/desktop/rest.ts:11-29` — a `/vnc-password` fallback ladder** in the platform, when the
|
||
sidecar is already idempotent about this (`sidecar/vnc/index.ts:28-36` → `vnc-manager.ts:57`).
|
||
`:31-37` also invents its own response envelope.
|
||
5. **`api/desktop/vnc-config.ts:1-10` — the main process reads `~/.vnc/password` in plaintext**
|
||
(`:4, 7, 9`). The same layout is duplicated at `vnc-manager.ts:58-60`, so the sidecar already
|
||
knows it. This is the one security-relevant item on this path: a credential read that has no
|
||
reason to happen in `officer`.
|
||
6. **`server.tsx:309-312` — `/novnc/*` maps the request pathname onto `public/<pathname>` with no
|
||
auth and no traversal guard.** Flagging as observed, not diagnosed — whether it is exploitable
|
||
depends on how `pathname` is normalised before it gets there, which I did not trace. Worth a
|
||
deliberate look given it is the one unauthenticated static path in the file.
|
||
7. Wiring at `server.tsx:13, 45, 149, 234, 339` is fine, and **`hono.ts:37, 122` is already
|
||
reference-shaped** (two lines).
|
||
|
||
*Adjacent, and its own domain rather than a vnc violation:* the browser relay —
|
||
`server.tsx:369, 371` plus `api/browser/relay.ts` (677 lines), `api/browser/router.ts` (198, including
|
||
`Bun.spawn(['zip', …])` at `:26-30`), `cdp.ts` (99) and `relay-auth.ts` (42); and
|
||
`api/scrape/scrape.ts:9-19, 49+` launches chromium in-process. Noted for a future pass; not counted
|
||
above.
|
||
|
||
## Cross-cutting: the shared infrastructure
|
||
|
||
The per-sidecar sections above are symptoms. These are the four things that let the same mistake happen
|
||
eight times.
|
||
|
||
### 1. The proxy-size table
|
||
|
||
Sorted by how far each is from the reference. This is the whole audit in one view:
|
||
|
||
| sidecar | platform lines | verdict |
|
||
|---|---:|---|
|
||
| slskd | 70 | ✅ reference |
|
||
| music | 88 | ✅ compliant (the cliamp subsystem beside it is not) |
|
||
| pty | 169 | ✗ ~all of it is sidecar logic |
|
||
| vnc | 170 | ✗ blocked on a missing listener |
|
||
| vault | 499 | ◐ partial, security-relevant |
|
||
| opencode | 478 | ✗ (~330 lines deletable as dead) |
|
||
| claude | ~2,300 | ✗ |
|
||
| email | ~2,875 | ✗ no proxy exists at all |
|
||
|
||
`hono.ts` mounts **36 routers. Three are thin sidecar proxies** — `:106` (music), `:107` (slskd), and
|
||
`:77` (vault, mounted *outside* `protectedRouter`).
|
||
|
||
For contrast, sidecar-side LOC: music 1,630 · claude 1,523 · slskd 653 · opencode 427 · vnc 326 ·
|
||
email 314 · vault 295. Note the inversion on email: 314 sidecar lines to 2,875 platform lines.
|
||
|
||
And `api/*` by size, which shows where the mass actually sits: tasks 2,399 · chat 2,227 · email 1,782 ·
|
||
file-browser 1,465 · server-settings 1,452 · browser 1,105 · auth 607 · system-monitor 500 · vault 499
|
||
· … · desktop 170 · terminal 169 · music 88 · slskd 70.
|
||
|
||
### 2. The protocol is not a transport
|
||
|
||
`sidecar/protocol.ts` is a **closed union of ~34 message types: 7 transport, 25+ domain.** Every new
|
||
sidecar capability requires editing a shared platform file — which is why domain knowledge keeps
|
||
landing there (CLI flags, `display`/`pid`, `proxySecret`, spawn params).
|
||
|
||
Two specific consequences:
|
||
|
||
- **`protocol.ts:1` imports `ChatEvent` and `MessageCost` from `../api/chat/types`.** The transport
|
||
layer depends on one feature's domain model.
|
||
- **Nine `queue:*` message types are undeclared**, passed through by a `msg.type.startsWith('queue:')`
|
||
string test (`server.tsx:80`, `sidecar/email/index.ts:31, 38`). So the "closed" union is already
|
||
being bypassed where it was inconvenient — evidence that the closed shape is the wrong shape.
|
||
|
||
By contrast `registration-protocol.ts` (16 lines: `name` + `capabilities: string[]`) is genuinely
|
||
generic. The registration handshake got this right; the command channel did not.
|
||
|
||
### 3. Ten WebSocket providers, and only three are tunnels
|
||
|
||
`platform/CLAUDE.md` says eight; it is **ten** (add `vault` and `sidecar`). Of those, only **`vault`,
|
||
`desktop` and `dev-server`** are byte tunnels. **`task-runner`, `pipeline`, `cliamp` and `cliamp-audio`,
|
||
plus the browser relay, spawn and manage child processes inside the auth proxy — ~2,200 lines.** So the
|
||
"thin proxy" rule is violated by roughly as much code in the WS layer as in all the routers combined.
|
||
|
||
`api/desktop/` is the right model to copy (an opaque byte pump). `dev-server` is the right model for the
|
||
relay itself — including the detail that it defers JWT verification into `open()`
|
||
(`server.tsx:168-186`) because Bun's upgrade must be synchronous. Anyone building a claude or pty relay
|
||
will hit that same constraint.
|
||
|
||
Port discovery across all of them depends on **ES-module side-effect imports** (`hono.ts:27, 28, 29`,
|
||
`server.tsx:19`) — a sidecar's port is remembered because a module happened to be imported. Fragile,
|
||
and invisible at the call site.
|
||
|
||
### 4. Three copies of the queue engine, two of them dead
|
||
|
||
- `queue/init.ts` — 290 lines, **live**
|
||
- `queue/engine.ts` + `queue/index.ts` — 283 lines, **dead**
|
||
- `sidecar/queue-runner.ts` — 285 lines, **dead**
|
||
|
||
`queue/init.ts:8` comments that it was "moved from sidecar/queue-runner.ts" — i.e. **work migrated from
|
||
a sidecar back into the main process**, the opposite direction to the rule. The comments at
|
||
`bootstrap.ts:4, 15` are false. And **both live job kinds are email-domain and run in `officer`**, which
|
||
is item 2 of the email section arriving from a different direction.
|
||
|
||
### 5. Registry bugs that will bite during any migration
|
||
|
||
- **`unregisterSidecar` (`sidecar-registry.ts:80-85`) rejects the entire global pending-command map
|
||
when *any single* sidecar disconnects.** So restarting `officer-music` fails in-flight claude, pty and
|
||
vault commands. This will look like random unrelated breakage the moment sidecars restart
|
||
independently — which is the entire goal.
|
||
- **`:88-90` treats `capabilities.includes('proxy')` as "is this claude"** — true only by accident of
|
||
the naming confusion documented in `CLAUDE_SIDECAR_ISOLATION.md`.
|
||
|
||
### 6. What the database says (the clearest signal in the audit)
|
||
|
||
Table ownership tracks compliance exactly:
|
||
|
||
- **`queries/music.ts` and `queries/soulseek.ts` — read only by their sidecars.** Exemplary, and it is
|
||
no coincidence these are the two compliant proxies.
|
||
- **`queries/vault.ts` — platform-only**, including the three auth handlers
|
||
(`signout.ts`, `revoke-handler.ts`, `panic-handler.ts`).
|
||
- **`queries/chat-events.ts` — platform-only**, which is exactly what makes an `officer` restart lossy.
|
||
- **`queries/email-accounts.ts` — split**, with `api/chat/websocket.ts:11, 61-62` reaching across
|
||
domains into it.
|
||
|
||
**A useful rule falls out of this:** *if a table is read by exactly one sidecar and nothing else, that
|
||
sidecar is probably compliant. If the platform reads it, the platform probably owns logic it shouldn't.*
|
||
Cheaper to check than reading 3,000 lines.
|
||
|
||
---
|
||
|
||
**End of Pass 1 (backend / API).** Pass 2 covers the frontend — how each sidecar's UI talks to it,
|
||
mirroring the slskd findings at the top of this document.
|
||
|
||
# Pass 2 — frontend code
|
||
|
||
Same rule, applied one layer out. The question here is not "what logic runs in `officer`" but **"does
|
||
the browser know things only the sidecar should know?"** — upstream URL shapes, wire formats, session-id
|
||
conventions, retry and reconnect policy, capability catalogues.
|
||
|
||
The slskd case at the top of this document is the template: **37 raw `/slskd/api/v0/…` calls against 10
|
||
`/slskd/_officer/…` calls**, meaning the browser is a second client of the upstream API rather than a
|
||
client of Officer. Each section below asks the same question of one sidecar.
|
||
|
||
Ordered most-coupled first, not alphabetically — so the ordering differs from Pass 1 deliberately.
|
||
|
||
## claude / chat — the frontend is better than the backend
|
||
|
||
An unexpected result, and the most important single finding in Pass 2: **the chat frontend already
|
||
implements the reconnect-and-replay contract that the backend cannot honour.** Where the slskd browser
|
||
code is a second client of an upstream API, the chat browser code is a well-behaved client of Officer.
|
||
The leakage here is vocabulary, not architecture.
|
||
|
||
Main files: `officerdev/src/hooks/useChat.ts` (384), `hooks/src/useChatWebSocket.ts` (84),
|
||
`apps/Chat/types.ts` (75), `state/src/useClaudeSessions.ts` (97), `state/src/useModels.ts` (97),
|
||
`Chat/components/ModelSelector.tsx` (170), `ChatHistory/ChatDetailPanel.tsx` (156),
|
||
`ChatHistory/SessionList.tsx` (210), `Chat/EmbeddableChat/useEmbeddableChat.ts` (274),
|
||
`Chat/components/MessageBubble.tsx` (204).
|
||
|
||
### Reconnect and replay — verified working, and it changes the isolation plan
|
||
|
||
`hooks/src/useChatWebSocket.ts:46-58` auto-reconnects with linear-capped backoff
|
||
(`Math.min(5000, 300 * retry)`), resetting the counter on a successful open (`:33`). And the browser
|
||
already tracks a **sequence cursor**:
|
||
|
||
```ts
|
||
// useChat.ts:145-146
|
||
const seq = (data as { seq?: number }).seq;
|
||
if (typeof seq === 'number' && seq > cursorRef.current) cursorRef.current = seq;
|
||
```
|
||
|
||
```ts
|
||
// useChat.ts:282-285 — on every (re)connect once OPEN
|
||
const sid = sessionIdRef.current;
|
||
if (sid) sendRef.current({ type: 'resume-cursor', sessionId: sid, cursor: cursorRef.current });
|
||
```
|
||
|
||
The comment at `useChatWebSocket.ts:6-8` states the intent exactly: re-bind to the session and replay
|
||
events missed while briefly disconnected. On top of that, `sync:messages` rebuilds full transcript
|
||
state including re-arming a mid-stream buffer (`useChat.ts:209-237`, `:233-236`), and there is real
|
||
disconnected UI — a red "Disconnected" indicator (`ChatDetailPanel.tsx:38-52`), send disabled
|
||
(`InputArea.tsx:84`), model switching locked (`ModelSelector.tsx:67`).
|
||
|
||
**So `seq` + `resume-cursor` already exist end to end.** Pass 1 found the matching backend half at
|
||
`chat/websocket.ts:612-629` (`getChatEventsSince`). The protocol is not missing; the *writer* is simply
|
||
on the wrong side of the socket. That makes the durability stage of `CLAUDE_SIDECAR_ISOLATION.md`
|
||
substantially smaller than I estimated — a relocation, not a new mechanism.
|
||
|
||
One real gap: **a silent replay gap is undetectable by the browser.** `resume-cursor` is only sent when
|
||
`sessionIdRef.current` is already set, no `sync:messages` is forced on reconnect, and nothing validates
|
||
that the server actually had events from `cursor` onward. If the server's retained range were shorter
|
||
than the gap, the UI would show a seamless conversation with a hole in it. Worth a sequence-continuity
|
||
check when the writer moves.
|
||
|
||
### What the browser knows that it shouldn't
|
||
|
||
1. **Hardcoded model ids and provider names.** `state/src/useSettings.ts:86-87, 99` hardcodes
|
||
`'claude-code'` three times as a default. `state/src/useModels.ts:68, 96` branches access policy on
|
||
`m.provider === 'claude-code'` — twice. `Chat/components/ModelSelector.tsx:7-23` carries a
|
||
`PROVIDER_DISPLAY` map of 15 provider ids.
|
||
2. **A fully-qualified Claude model id, in an unrelated app.**
|
||
`FileBrowser/FileBrowserApp/components/TaskRunnerModal.tsx:1362` —
|
||
`useState(settings.tasks.defaultModel ?? 'claude-haiku-4-5-20251001')`. A dated upstream model
|
||
string, in the task runner. This one silently goes stale.
|
||
3. **The CLI invocation string is in the browser.** `apps/Terminal/index.tsx:32-33` —
|
||
`command="claude --dangerously-skip-permissions"`, `statePrefix="claude-code"`. The browser decides
|
||
how the agent binary is invoked, including its permission flag. *(The unsandboxed posture is
|
||
deliberate per `platform/CLAUDE.md`; the objection is only to where the decision lives — the
|
||
browser is the furthest possible place from the sidecar that owns it.)*
|
||
4. **Capability metadata crosses to the client.** `Chat/types.ts:11-19` types `contextWindow`,
|
||
`maxTokens` and `reasoning?`, and `ModelSelector.tsx:116` branches the UI on `reasoning`. The
|
||
browser doesn't compute these, so this is acceptable *if* they come from the sidecar — but Pass 1
|
||
found them hardcoded in the platform at `api/chat/list-models.ts:5-9`, so today the numbers
|
||
originate two layers away from the thing they describe.
|
||
5. **Claude CLI session conventions are documented in the browser.**
|
||
`state/src/useClaudeSessions.ts:8-9` comments that the id *is* the transcript filename;
|
||
`SessionList.tsx:10-11` explains that clicking a session continues it "via --resume"; `:15` types
|
||
`harness?: 'claude' | 'opencode'`. And the magic string **`'general_chat_sessions'`** — Claude's
|
||
own default directory bucket — appears as a literal in `PwdSelector.tsx:7, 22, 62`,
|
||
`ChatDetailPanel.tsx:86, 101` and `useClaudeSessions.ts:42`.
|
||
6. **`GET /chat/models` returns `hostHome`** (`useModels.ts:38`) — a host filesystem path handed to
|
||
the browser.
|
||
7. **Route balance: 7 chat-domain routes against ~6 generic ones.** `/chat/models`, `/chat/pwds`,
|
||
`/chat/sessions`, `/chat/sessions/:id` (with `?before=&limit=` pagination straight against the
|
||
transcript store), `/chat/sessions/:id/title`, `/api/chat/ws`. Against generic `/scrape`,
|
||
`/upload`, `/user/settings`, `/server-settings/chat-providers/*`, `/tasks/:id`. Unlike slskd these
|
||
are all Officer-owned paths — nothing bypasses the platform — but they are shaped around Claude's
|
||
session model rather than a neutral conversation resource.
|
||
8. **The event state machine is duplicated a third time — in the browser.** `Chat/types.ts:43-55`
|
||
declares a 12-member `ServerMessage` union and `useChat.ts:148-277` switches on all of it,
|
||
assembling deltas, tracking tool calls by `toolCallId`, and handling
|
||
`task:started` / `task:notification`. Pass 1 found the same interpretation in
|
||
`chat/websocket.ts:168-312` and again in `pipeline-executor.ts:151-201`. **Three
|
||
implementations.** The browser's copy is legitimate — it renders — but it means the wire format has
|
||
three consumers to keep in step.
|
||
|
||
### Verified clean
|
||
|
||
- **No JSONL parsing, no `leafUuid`, no `~/.claude` path construction in the browser.** Those stop at
|
||
the backend. The browser knows the concepts by name only.
|
||
- **No pricing table or cost formula client-side.** `MessageBubble.tsx:163-170` sums
|
||
`inputTokens + outputTokens` and formats a `totalUSD` it was handed (`Chat/types.ts:5-9`). Correct
|
||
placement.
|
||
- **Lifecycle control is two logical verbs, not process control:** `stop` (turn) and `disconnect`
|
||
(session) — `useChat.ts:352-360`. No PIDs, no process assumptions. This is exactly the abstraction a
|
||
relay-based architecture needs, and it already exists.
|
||
|
||
## email — routes are compliant, payloads and realtime are not
|
||
|
||
The mirror image of chat: **every one of the 20 API paths is Officer-shaped** — there is no
|
||
`/imap/uid/…` anywhere — but the request *bodies* carry IMAP configuration, the compose path builds
|
||
MIME, and the realtime channel cannot recover from a restart at all.
|
||
|
||
There is no windowed panel app; the email UI is screen-level under
|
||
`officer-web/Screens/Dashboard/Email/**` plus account setup in `Settings/`. ~2,451 lines total:
|
||
`EmailAccounts.tsx` (512), `Compose.tsx` (393), `EmailList.tsx` (377), `SMTPSection.tsx` (292),
|
||
`GoogleOAuthConfig.tsx` (288), `EmailReader.tsx` (224), `GoogleAccount.tsx` (98, **dead** — commented
|
||
out of `IntegrationsSettings/index.tsx:11, 71-79`), `EmailScreen.tsx` (60), `types/email.ts` (29).
|
||
|
||
1. **The browser owns IMAP connection parameters.** `EmailAccounts.tsx:21-37` holds `imapHost`,
|
||
`imapPort` (default `'993'`) and `imapSecure` as first-class client state; `:82` hardcodes the
|
||
Gmail preset `imap.gmail.com:993`; `:98-107` and `:124-133` POST `imapHost`/`imapPort`/`imapSecure`
|
||
/`authType`. **Provider detection is a hostname string comparison in the browser** (`:123`):
|
||
`const provider = form.imapHost === 'imap.gmail.com' ? 'gmail' : 'imap';`. Provider presets are
|
||
the sidecar's knowledge by definition.
|
||
2. **MIME construction in the browser.** `Compose.tsx:218-236` clones the editor DOM, rewrites each
|
||
inline `<img>` to `cid:inline-N`, and ships the parts — the browser authoring the `cid:` URI scheme
|
||
of a `multipart/related` message. Its own comment admits the contract is positional ("backend
|
||
matches by order"), which is a fragile coupling in both directions. Inline-vs-attachment is decided
|
||
by `f.type.startsWith('image/')` (`:176`, `:195`).
|
||
3. **No HTML sanitisation of message bodies.** `EmailReader.tsx:19-53` writes server-supplied HTML
|
||
directly into an iframe via `doc.write(...)`, with `sandbox="allow-same-origin"` and no
|
||
`allow-scripts` (`:52`). No DOMPurify or equivalent anywhere. So the **iframe sandbox is the only
|
||
thing preventing script execution from hostile mail** — it does hold, since scripts aren't
|
||
allowed, but it is a single un-defended layer, and `allow-same-origin` without `allow-scripts` is a
|
||
combination worth a deliberate second look rather than an accident.
|
||
4. **Two plaintext secrets are fetched into the browser and cached.**
|
||
- `SMTPSection.tsx:39` — `GET /server-settings/smtp` returns the SMTP `password` (and Resend
|
||
`apiKey`); `:63` writes it into state; `:208-213` binds it to a `type="password"` input. It lives
|
||
in the React Query cache under `['SMTP_CONFIG']` for the life of the tab.
|
||
- `GoogleOAuthConfig.tsx:197-210` — `GET /integrations/google/config` returns `clientSecret` in
|
||
plaintext; held in `useState` (`:184`), shown at `:255-262`.
|
||
|
||
Neither is a mail credential *the sidecar owns*, and both are the owner's own secrets on the
|
||
owner's own machine — but "GET returns the secret so the form can prefill" is the pattern worth
|
||
changing, since a write-only field would work identically.
|
||
5. **The session bearer token is passed in a URL.** `EmailList.tsx:110-112` builds
|
||
`new EventSource('/api/email/events?token=' + …)` from `localStorage`. Unavoidable for `EventSource`
|
||
(it can't set headers), but it puts the JWT into browser history and any proxy access log. Worth
|
||
noting that the chat socket does the same at `useChat.ts:121-123`.
|
||
6. **The realtime channel cannot survive a restart — explicit, not inferred.**
|
||
`EmailList.tsx:109-125` opens the SSE stream, expects `{ type: 'new-mail' }`, invalidates three
|
||
query keys, and closes on unmount. There is **no `es.onerror`, no backoff, no reconnect, and no
|
||
`Last-Event-ID` handling.** And the server never sends an `id:` field — Pass 1's
|
||
`api/email/email.ts:101` emits only `data: {"type":"new-mail"}` — so even the browser's *native*
|
||
`EventSource` retry cannot request replay. Any `new-mail` event emitted during a restart is lost
|
||
silently until the next event arrives or the user hits Sync manually (`:134-154`).
|
||
**Direct contrast with chat, in the same codebase: one channel has cursor-based replay, the other
|
||
has nothing.** That gap is the template for what to fix, and chat is the template to copy.
|
||
7. **Retry and polling policy decided in the browser, duplicated.** A fixed 5,000 ms account poll
|
||
while any account is `syncing`/`queued` — `EmailList.tsx:157-167`, duplicated verbatim at
|
||
`EmailAccounts.tsx:65-70`. Search debounce 300 ms (`EmailList.tsx:63-66`), contact autocomplete
|
||
180 ms (`Compose.tsx:28`). No backoff anywhere; a failed sync gets one `toast.error`.
|
||
8. **A client-side folder heuristic.** `EmailList.tsx:95-105`: if `inbox` returns zero but an `all`
|
||
probe finds mail, the browser silently switches folder. Business logic about mailbox state, decided
|
||
in the UI.
|
||
9. **Gmail knowledge, mostly as copy rather than behaviour.** `GoogleOAuthConfig.tsx:15-18` lists the
|
||
OAuth scopes (`gmail.readonly`, `calendar.readonly`) and `:104-111`, `:116` explain Google's
|
||
"restricted" classification — documentation text, not requests the browser makes.
|
||
`EmailScreen.tsx:13` puts Gmail-vs-local-DB routing into an **agent system prompt**
|
||
("Do not use the Gmail integration for questions about existing emails"), which is domain routing
|
||
logic expressed as English in the frontend. `Compose.tsx:241` knows Gmail files its own sent copy.
|
||
|
||
### Verified clean
|
||
|
||
- **Threading is server-side.** The browser receives `EmailThread` pre-grouped (`EmailReader.tsx:130-134`,
|
||
`types/email.ts:24-29`) and consumes precomputed `threadCount`/`threadUnread`. No participant
|
||
computation, no subject normalisation, no dedup. (`Compose.tsx:385-393` does `Re:` prefixing for a
|
||
reply draft — cosmetic.)
|
||
- **Folder names are Officer's, not IMAP's.** `EmailList.tsx:35-41` is a fixed
|
||
`inbox/all/sent/spam/trash` list passed as `folder=`; no special-use flag mapping.
|
||
- **No charset, quoted-printable, base64 or RFC-2047 decoding in the browser** — it receives decoded
|
||
`text`/`html`/`snippet`. Reading is compliant; only composing leaks.
|
||
- **No Gmail label ids and no Gmail query syntax constructed client-side.** The search box passes `q=`
|
||
through untouched (`EmailList.tsx:83-85`); `:309`'s placeholder only *hints* at the syntax.
|
||
- **Mail credentials are write-only.** The password is POSTed at `EmailAccounts.tsx:132` and never read
|
||
back — `GET /email/accounts` returns no credential field. OAuth tokens never reach the browser at
|
||
all: `:88-107` either redirects the page to `/api/integrations/google/authorize` or POSTs
|
||
`credentials: { userIntegrationId: true }`, a boolean. This is the right shape, and it is worth
|
||
noting that the *account* credential path is stricter than the *settings* ones in item 4.
|
||
- **No Message-Id handling** — and `Compose.tsx:382-384` documents the absence, noting `m.id` is a local
|
||
hash and that threading currently leans on `Re:` + participants.
|
||
|
||
## opencode — the most compliant frontend of the eight
|
||
|
||
Genuinely surprising given Pass 1 found ≈792 non-compliant *backend* lines. **The string `opencode`
|
||
appears in exactly four frontend files, and only one of those is logic.** Everything the backend leaks
|
||
— the `ses_` prefix, the `opencode/<modelID>` id shape, `metadata.officer`, `auth.json`,
|
||
`models.json`, the version pin — stops at the server. Verified by exhaustive grep: **zero frontend hits
|
||
for `ses_`, `opencode/`, `metadata.officer`, `auth.json`, `models.json`.**
|
||
|
||
The four mentions: `state/src/useClaudeSessions.ts:15` (`harness?: 'claude' | 'opencode'`),
|
||
`ChatHistory/SessionList.tsx:153-156` (renders an "OpenCode" badge),
|
||
`Chat/components/ModelSelector.tsx:22` (`opencode: 'OpenCode Zen'` display label), and
|
||
`Chat/EmbeddableChat/useEmbeddableChat.ts:108-109` — a **comment only**, explaining that `cwd` is sent
|
||
every turn because OpenCode rebuilds its working directory each turn. The code isn't gated on harness;
|
||
it just always sends `cwd`.
|
||
|
||
1. **Harness selection is not opencode-aware at all — it is `claude-code`-aware.** The only string
|
||
test is a Claude one, and everything else falls through to the opencode path:
|
||
`state/src/useModels.ts:68` (`if (m.provider === 'claude-code') return true;`) and `:96`. Both are
|
||
**access-policy bypasses**: Claude models always pass the admin policy filter; every
|
||
opencode-routed provider is subject to it. Worth knowing that's the actual semantic, since it isn't
|
||
obvious from the code.
|
||
2. **Session UI is shared, not forked.** `SessionList.tsx` / `ChatDetailPanel.tsx` serve both
|
||
harnesses off the same `/chat/pwds`, `/chat/sessions`, `/chat/sessions/:id`,
|
||
`/chat/sessions/:id/title` routes, and the browser treats `id` as an **opaque string** — no format
|
||
validation, no `ses_` regex, no branch on id shape. `ChatDetailPanel.tsx:96-98` resume/pagination is
|
||
harness-agnostic.
|
||
3. **The WebSocket machinery is fully shared with no opencode branch.**
|
||
`useChat.ts:121-123` builds one URL unconditionally; `hooks/src/useChatWebSocket.ts` takes only
|
||
`{url, onMessage, onOpen}`; the 12-member `ServerMessage` union (`Chat/types.ts:34-46`) has zero
|
||
opencode-specific variants. So the reconnect-and-replay work described in the chat section above
|
||
benefits both harnesses for free.
|
||
4. **API keys are never echoed to the browser — verified against the server.**
|
||
`AIHarnessesSection.tsx:57` GETs `/server-settings/chat-providers/api-keys`, but
|
||
`api/server-settings/chat-providers.ts:384-391` masks server-side first
|
||
(`value.slice(0,3) + '...' + value.slice(-3)`), and the browser uses the result only as a
|
||
placeholder (`AIHarnessesSection.tsx:467`). A freshly typed key lives transiently in
|
||
`keyInputs` state (`:74`) and is **deleted after the PUT** (`:121-125`). Never in `localStorage`,
|
||
`sessionStorage`, or the query cache. Local-provider config returns the auth *type* only, never key
|
||
material. **This is the pattern the email settings surface (Pass 2, email item 4) should copy.**
|
||
5. **No hardcoded model catalogue.** `state/src/useModels.ts:30-51` fetches everything from
|
||
`/chat/models`. The only hardcoded data is display-name maps — `ModelSelector.tsx:7-23` (14 pairs)
|
||
and a near-duplicate at `AISettings.tsx:19-34` (13 pairs, falling back to server-supplied
|
||
`providerNames` via `useModels.ts:18-20`). Two overlapping copies of a label map is a small
|
||
duplication, not a boundary violation.
|
||
6. **Naming debt, not coupling.** The shared hook is called `useClaudeSessions`, and `:9` comments
|
||
`id: string; // Claude session uuid (= transcript filename)` while that field also carries
|
||
opencode's `ses_…` ids; `:15` treats absent harness as claude; `:58` calls the store "Claude's own
|
||
transcript store". Accurate when written, misleading now.
|
||
7. **No dead opencode frontend code.** Verified importers for all four files — `ModelSelector.tsx`
|
||
(via `InputArea.tsx:92`, `ChatLauncher.tsx:107`, `TaskRunnerModal.tsx:202`), `SessionList.tsx`
|
||
(routed at `App.tsx:38-40`), `useClaudeSessions.ts`, `useEmbeddableChat.ts`. All reachable.
|
||
|
||
**One thing the frontend displays that isn't real, and the cause is in the backend.**
|
||
`api/chat/list-models.ts:24` stubs *every* opencode-routed model with constant metadata —
|
||
`contextWindow: 200000, maxTokens: 8192, reasoning: false, images: true`, with the comment "metadata is
|
||
left at neutral defaults for now". The browser faithfully renders these (`ModelSelector.tsx:116`
|
||
branches the thinking toggle on `reasoning`). So the capability numbers shown to the user for opencode
|
||
models are placeholders, and `reasoning: false` will suppress the thinking toggle for models that do
|
||
support it. A backend defect, surfaced by a compliant frontend.
|
||
|
||
## terminal / pty — the browser reconnects, and then loses the session anyway
|
||
|
||
The mirror of the backend result. Pass 1 called pty the least compliant *backend* surface; the frontend
|
||
is mostly well-behaved, has real reconnect logic, and yet contains **one bug that defeats the entire
|
||
detach-not-kill design.**
|
||
|
||
All terminal UI funnels through one component. `Terminal/Terminal.tsx` (329) holds every piece of logic;
|
||
`CommandTerminalWrapper.tsx` (59), `TerminalWrapper.tsx` (57), `HostTerminalWrapper.tsx` (50),
|
||
`Terminal/index.tsx` (76), `Headers.tsx` (79). Reused by `FileBrowser/CliampPanel.tsx` (36) and
|
||
`Settings/SystemSettings.tsx` (151). Verified: **no code editor, dev-server or task-runner panel embeds
|
||
a PTY** — the apparent matches were lucide icons and an unrelated `isTerminal(status)`.
|
||
|
||
### The session-orphaning bug
|
||
|
||
The backend deliberately detaches rather than kills, so a shell outlives a dropped socket. The browser
|
||
throws that away on any ordinary unmount:
|
||
|
||
```ts
|
||
// CommandTerminalWrapper.tsx:32-39 — and identically TerminalWrapper.tsx:35-42,
|
||
// HostTerminalWrapper.tsx:28-35
|
||
useEffect(() => {
|
||
return () => {
|
||
setTerminalsRef.current((prev) => {
|
||
const { [panelId]: _, ...rest } = prev;
|
||
return rest;
|
||
});
|
||
};
|
||
}, [panelId]);
|
||
```
|
||
|
||
That cleanup deletes the `panelId → sessionId` mapping when the panel closes, the workspace layout
|
||
changes, or a client-side route change unmounts the wrapper. The next mount calls
|
||
`crypto.randomUUID()` again (`CommandTerminalWrapper.tsx:28`, `TerminalWrapper.tsx:30`,
|
||
`HostTerminalWrapper.tsx:24`) and connects with a **new** session id — leaving the previous detached
|
||
shell running server-side with no reference to it, ever. Combined with Pass 1's finding that
|
||
**`pty:close` has no sender anywhere**, there is no kill path either: those shells accumulate.
|
||
|
||
So: reconnect works for network drops and tab backgrounding; **closing a panel silently leaks a shell.**
|
||
|
||
### Reconnect — implemented, and independent of the chat implementation
|
||
|
||
```ts
|
||
// Terminal.tsx:141-145
|
||
const MAX_RECONNECT_ATTEMPTS = 5;
|
||
const RECONNECT_DELAYS = [1000, 2000, 3000, 5000, 5000];
|
||
```
|
||
|
||
`handleClose` (`:235-254`) reschedules `connect` unless the process exited or the effect was disposed,
|
||
gives up after 5 attempts, then reports disconnected (`:249-253`); the counter resets on a successful
|
||
open (`:173`). There is also a **visibility-triggered reconnect** (`:272-280`) that resets the counter
|
||
and reconnects immediately when the tab is refocused. Because `connect()` rebuilds the URL from the same
|
||
`sessionId` prop (`:162`), every retry re-presents the same id — re-attachment is correct by
|
||
construction.
|
||
|
||
Note this is a **third, unrelated backoff implementation**: a 5-entry table here, `Math.min(5000, 300 *
|
||
retry)` in `useChatWebSocket.ts:46-58`, and a byte-identical copy of the sidecar table duplicated
|
||
between `pty-sidecar.mjs:37` and `connect.ts:22` (Pass 1). Four backoff policies, no shared helper.
|
||
|
||
**Scrollback survives, but the browser doesn't know it.** There is no replay message type at all. The
|
||
sidecar keeps a capped 50KB buffer (`pty-sidecar.mjs:36`, `BUFFER_MAX`) and re-emits it on re-init
|
||
(`:99-101`); the bridge forwards it as an ordinary `output` frame
|
||
(`api/terminal/websocket.ts:71-79`), and `Terminal.tsx:181` `term.write()`s it indistinguishably from
|
||
live output. No dedup, no historical marker. It works, passively. *(INFERRED: survival across a hard
|
||
page reload depends on React cleanup not running during navigation teardown — standard behaviour, but
|
||
not verified against `pagehide` here.)*
|
||
|
||
Session ids are **chosen by the browser** and persisted server-side through `useDashboardState` →
|
||
`GET/PATCH /dashboards` (React Query key `['DASHBOARD_STATE']`, `staleTime: Infinity`), so they survive
|
||
a reload. Key derivation is inconsistent across the three wrappers: `CommandTerminalWrapper.tsx:19` and
|
||
`HostTerminalWrapper.tsx:12` template `dashboardId` directly, while `TerminalWrapper.tsx:12-18`
|
||
regex-matches `ws-layout-(.+)` / `proj-layout-(.+)` to build its key. `SystemSettings.tsx:92` opts out
|
||
with an ephemeral `` `run-cmd-${Date.now()}` `` in local state — deliberate for a one-shot panel.
|
||
|
||
### Shell knowledge in the browser
|
||
|
||
1. **The browser composes shell commands by string concatenation, unescaped.**
|
||
```ts
|
||
// Terminal.tsx:187-190
|
||
const wrapped = onCommandDoneRef.current
|
||
? `${commandRef.current}; echo "${EXIT_MARKER}$?__"`
|
||
: commandRef.current;
|
||
ws.send(JSON.stringify({ type: 'input', data: wrapped + '\r' }));
|
||
```
|
||
That assumes a POSIX shell (`;`, `$?`, `echo`) and does not escape `command`. Same pattern at
|
||
`CommandTerminalWrapper.tsx:46-47`, which also does **tilde/home expansion in the browser** and
|
||
builds `cd <path> && <command>`:
|
||
```ts
|
||
const cwdPath = cwd && cwd !== '~' ? (cwd.startsWith('~') ? cwd : `~/${cwd.replace(/^\//, '')}`) : null;
|
||
const fullCommand = cwdPath ? `cd ${cwdPath} && ${command}` : command;
|
||
```
|
||
The inputs are the app's own hardcoded commands and a cwd from the workspace, not free user text —
|
||
so this is a correctness and placement problem rather than an injection hole today. It becomes one
|
||
the moment a command string is user-supplied.
|
||
2. **A sentinel protocol invented in the browser.** `Terminal.tsx:151`
|
||
(`EXIT_MARKER = '__OFFICER_EXIT_'`), a hand-rolled ANSI stripper at `:152-153` (CSI plus
|
||
BEL-terminated OSC only — misses ST-terminated OSC), and echo detection by
|
||
`l.includes(commandRef.current!.slice(0, 20))` at `:212`. Command completion is inferred by
|
||
scraping terminal output for a marker the browser injected.
|
||
3. **Hardcoded CLI invocations in the bundle:** `Terminal/index.tsx:13` (`tmux`), `:17` (`nvim`), and
|
||
`:32` — `command="claude --dangerously-skip-permissions"`. Each is then wrapped with the `cd` prefix
|
||
and the exit-marker suffix above and typed into the pty as keystrokes.
|
||
4. No hardcoded shell **path** in the browser — `/bin/zsh` lives at
|
||
`api/terminal/websocket.ts:62` (Pass 1, item 1a). Default cwd `'~'` comes from
|
||
`WorkspaceContext.ts:34`.
|
||
|
||
### Two cross-checks that confirm Pass 1 findings
|
||
|
||
- **The backend's `cwd` handler is unreachable.** Pass 1 flagged
|
||
`api/terminal/websocket.ts:129-138` for synthesizing `` `cd ${JSON.stringify(msg.path)}\r` ``.
|
||
Repo-wide grep finds **zero** frontend senders of `{type:'cwd'}` — the browser does its own `cd`
|
||
composition instead (item 1 above). So that branch is dead, and the capability it implements is
|
||
duplicated in the client.
|
||
- **`detached` is dead in the other direction.** `Terminal.tsx:225-226` handles a `'detached'` message
|
||
and writes `[Session taken over]`, but **no backend code ever emits it** — the only `detached` in
|
||
`src/servers` is an unrelated field at `activity/router.ts:98`. The frontend author modelled a
|
||
takeover concept the server never implemented.
|
||
|
||
Also dead: **`officerdev/terminal-host`** (`Terminal/index.tsx:47-54`, `HostTerminalWrapper.tsx`) is
|
||
registered `availableOnPanel: false` and referenced by no route, layout or panel map.
|
||
|
||
### Ongoing resize is not implemented
|
||
|
||
`fitAddon.fit()` is called exactly once per `connect()` (`Terminal.tsx:158`). There is **no
|
||
`ResizeObserver` and no `window` resize listener** in the file. The browser is the authority on
|
||
dimensions — it creates the terminal at a fixed 80×24 (`:120-121`), fits after layout settles, then
|
||
sends `cols`/`rows` both as query params (`:59-60`) and as an initial `resize` message (`:175`), and the
|
||
server never overrides them. **Consequence: resizing the window or dragging a panel splitter does not
|
||
re-fit or notify the server until the next reconnect.** Not an architecture violation — a real bug, and
|
||
the one item in this section a user would notice daily.
|
||
|
||
### Verified clean
|
||
|
||
- **No PIDs, no tmux internals, no process-lifetime or kill semantics in the browser.** `exit` carries
|
||
no code and the backend sends none (`api/terminal/websocket.ts:84`), so there is nothing to leak.
|
||
- **The browser never generates ANSI for rendering** — `term.write(msg.data)` (`:182`) hands raw bytes
|
||
to xterm.js. The only parsing is the sentinel stripper above.
|
||
- **`CliampPanel.tsx:25, 35` reuses the same component against `/api/cliamp/ws`** with a different
|
||
`wsPath` — evidence the client really is a generic PTY-over-WS view, agnostic to whether a shell or
|
||
mpv is on the far end. The env the cliamp sidecar sets (`api/cliamp/websocket.ts:112`) is never
|
||
surfaced to the browser.
|
||
|
||
## music — the compliant frontend reference
|
||
|
||
**12 routes, all Officer-owned `/music/…`, zero upstream-shaped calls.** Put beside slskd's 37-vs-10,
|
||
this is what the rule looks like when it's followed on both sides. Unsurprising given Pass 1 found the
|
||
music backend proxy compliant at 88 lines — and the pairing is the point, see the cross-cutting note
|
||
below.
|
||
|
||
Files: `MusicPlayer/MusicPlayerHost.tsx` (370), `apps/Music/MusicDetail.tsx` (363),
|
||
`MusicPlayer/gapless-engine.ts` (325), `apps/Music/MusicBrowser.tsx` (233),
|
||
`widgets/MusicPlayer/index.tsx` (200), `apps/Music/FavoritesView.tsx` (188),
|
||
`apps/Music/shared.ts` (123), `useMusicFavorites.ts` (52), `useMusicPlayer.ts` (47),
|
||
`MusicScreen.tsx` (44), `MusicHeart.tsx` (43).
|
||
|
||
Routes: `/music/manifest`, `/music/meta`, `/music/cover`, `/music/discography`, `/music/reindex`,
|
||
`/music/favorites` (GET/POST/DELETE), `/music/now-playing` (GET/PUT/DELETE), `/music/stream`. Plus the
|
||
generic `/file-browser/ls` for directory walking — deliberate per `MUSIC_API.md:11-13`, and the right
|
||
call: no music-specific listing endpoint was invented.
|
||
|
||
Documented but **never called by the frontend**: `/music/reindex/status`, `/music/reindex/stream`,
|
||
`/music/poster`, `/music/lyrics`.
|
||
|
||
What little leaks:
|
||
|
||
1. **`MUSIC_ROOT = 'Music'` hardcoded twice** — `shared.ts:4` and `widgets/MusicPlayer/index.tsx:12`. A
|
||
home-relative prefix convention, not a filesystem path (no absolute path, no env var in the
|
||
browser). Mild.
|
||
2. **The client re-sorts what the sidecar returns**, and says why: `shared.ts:63` comments that
|
||
"meta.json is in ffprobe/readdir order (arbitrary)", so `sortTracks` (`:65-75`) re-orders by track
|
||
tag. **The browser is compensating for the sidecar's output format.** The correct fix is in the
|
||
indexer, not the client.
|
||
3. **`AUDIO_EXT` duplicated verbatim** — `shared.ts:96` and `widgets/MusicPlayer/index.tsx:19` — a
|
||
9-extension allowlist used to filter raw `file-browser/ls` results. Plus an album-name convention
|
||
regex (`ALBUM_NAME_RE`, `shared.ts:12`) for `"[year] Album Name"`.
|
||
4. **A playback design decision worth surfacing, though not a compliance issue.**
|
||
`gapless-engine.ts:86-89` does a plain `fetch(url)` → `arrayBuffer()` → `decodeAudioData()` with
|
||
**no `Range` header**, downloading and decoding each whole file to PCM. Its own comment
|
||
(`:9-11`) puts a ~10-minute track at **≈200 MB in memory**, bounded only by `CACHE_MAX = 3`
|
||
(`:26`). So the byte-range support the API offers is never exercised by this path, and
|
||
`X-Audio-Duration` is ignored in favour of `AudioBuffer.duration` (`:199`, `:231`). A deliberate
|
||
trade for sample-accurate gapless transitions — just an expensive one.
|
||
|
||
Verified clean: **no codec, container, MIME, remux or MediaSource logic anywhere** (grepped —
|
||
zero hits); no cover-art filename convention (always the opaque `/music/cover?path=`, with `meta.cover`
|
||
used only as a boolean); no tag-format knowledge; no reindex internals (`POST /music/reindex` is a
|
||
black-box trigger and the status/stream routes are never read).
|
||
|
||
## cliamp — the browser hardcodes the sidecar's capture format
|
||
|
||
Two sockets, and they behave very differently. The control channel is exemplary; the audio channel is
|
||
the most tightly coupled thing in Pass 2.
|
||
|
||
`FileBrowser/AudioStreamPlayer.tsx` (164), `FileBrowser/CliampPanel.tsx` (36),
|
||
`FileBrowser/pcm-worklet-processor.js` (41, **dead** — zero importers; `AudioStreamPlayer.tsx:19-58`
|
||
inlines its own copy of the same worklet as a string and loads it via `Blob`/`createObjectURL` at `:60`).
|
||
|
||
### The control channel is genuinely opaque
|
||
|
||
`useFileBrowserApp.ts:443-446` sets `?play=<path>`; `CliampPanel.tsx:25` builds
|
||
`/api/cliamp/ws?files=<path>`; `:35` renders the **generic `TerminalView`**. That's the entire
|
||
integration. Verified: **the browser has no idea mpv is on the other end** — no keybinding table, no
|
||
command syntax, no output parsing. Keystrokes are forwarded byte-for-byte
|
||
(`Terminal.tsx:265-269`) and output goes straight to xterm's generic ANSI parser (`:182`).
|
||
|
||
The consequence is a UI observation rather than a violation: **there are no transport controls.** No
|
||
play/pause, seek or volume — `CliampPanelHeader` (`CliampPanel.tsx:7-19`) shows only the filename split
|
||
off the URL (`:10`). The user reads mpv's own status line as rendered raw. That is the honest cost of a
|
||
fully opaque tunnel, and worth deciding deliberately rather than by default.
|
||
|
||
### The audio channel hardcodes the wire format on both sides independently
|
||
|
||
The sidecar spawns `parec --format=s16le --rate=44100 --channels=2` (`api/cliamp/audio-ws.ts:29`) and
|
||
pipes raw bytes with **no framing and no header** (`:57`). The browser hardcodes the matching
|
||
assumptions, with **no negotiation or handshake of any kind**:
|
||
|
||
```ts
|
||
// AudioStreamPlayer.tsx:9-10
|
||
const SAMPLE_RATE = 44100;
|
||
const CHANNELS = 2;
|
||
```
|
||
|
||
- `s16le` is implied by `new Int16Array(ev.data)` (`:108`) and `int16[i] / 32768` (`:111`).
|
||
- `new AudioContext({ sampleRate: SAMPLE_RATE })` (`:76`) — the browser **forces** its output rate to
|
||
match rather than resampling.
|
||
- Ring buffer capped at `SAMPLE_RATE * CHANNELS * 2` samples (`:30`) ≈ 4 seconds.
|
||
- Graph: WS binary → Int16→Float32 on the main thread → `postMessage` → `AudioWorkletNode`
|
||
(`pcm-processor`, deinterleaving at `:42-48`) → `GainNode` (mute) → destination.
|
||
|
||
Change the `parec` flags in the sidecar and the browser plays noise. Two files, no shared constant, no
|
||
version marker. This is the clearest single instance in Pass 2 of a sidecar's internal format being
|
||
duplicated into the client — and cliamp is precisely the subsystem Pass 1 found **has no sidecar at
|
||
all**.
|
||
|
||
**The audio socket has no reconnect.** `AudioStreamPlayer.tsx:121-123` handles `close` by flipping a UI
|
||
flag and nothing else; the effect only reconnects if the `wsUrl` prop identity changes (`:143`) or the
|
||
component remounts. A dropped capture socket stays dead until the panel is closed and reopened. The
|
||
control socket, by contrast, inherits `TerminalView`'s full backoff — so in the same panel, the terminal
|
||
comes back and the audio doesn't.
|
||
|
||
Also: the music and cliamp paths share no code despite both ending in Web Audio.
|
||
|
||
## vault — there is no vault frontend
|
||
|
||
Worth stating plainly rather than padding: **zero lines of client code.** Six grep passes across
|
||
`src/workspaces/officerdev/src/apps/**`, `src/apps/officer-web/**` and the shared workspaces for
|
||
`vault|bitwarden|cipher|kdf|PBKDF2|Argon2|crypto.subtle|masterPassword|unlock|/api/vault|TOTP` produced
|
||
only false positives: `helpers/slug.ts:25` (the word "vault" in a slug word list), a base64 audio blob,
|
||
and `components/ui/input-otp.tsx` (the generic shadcn primitive, matched on the substring in
|
||
`InputOTP`). `AppRegistry.tsx:22` lists every windowed app — FileBrowser, Terminal, CodeEditor, Chat,
|
||
FileViewer, Dashboards, Projects, ChatHistory, Preview, Widgets, Desktop, Music, Soulseek,
|
||
SystemMonitor — and there is no Vault entry. Nothing in `Settings/**`, nothing in the sidebar, nothing
|
||
in `TODO.md`.
|
||
|
||
So: no client-side crypto, no key custody, no decrypted-secret handling, no lock state machine, no
|
||
`/api/vault` calls, no WebSocket client. Nothing to assess.
|
||
|
||
**But this reframes a Pass 1 finding, and it matters.** Pass 1 credited the vault with getting one
|
||
thing right: "user-key derivation is genuinely client-side (the platform only relays `Kdf*` params at
|
||
`router.ts:96-101`)". Since Officer has no vault client, **the client doing that derivation is a
|
||
third-party Bitwarden app.** Which means the platform-side login broker, token store, WebSocket token
|
||
injection and at-rest crypto documented in Pass 1 exist to serve **external clients**, not Officer's own
|
||
UI. That is a materially different security story than "our own frontend needs this", and it should be
|
||
settled before any of that code is moved: who is actually meant to talk to the vault?
|
||
|
||
---
|
||
|
||
# Cross-cutting findings from Pass 2
|
||
|
||
## 1. The inverse correlation — and the mechanism behind it
|
||
|
||
The single most useful thing in this pass. Ranked by frontend compliance:
|
||
|
||
| sidecar | backend verdict (Pass 1) | frontend verdict (Pass 2) |
|
||
|---|---|---|
|
||
| slskd | ✅ compliant, 70 lines | ✗ **worst** — 37 raw upstream calls vs 10 Officer routes |
|
||
| music | ✅ compliant, 88 lines | ✅ 12 routes, all Officer-owned |
|
||
| opencode | ✗ ≈792 lines | ✅ **best** — 4 mentions, 1 of them logic |
|
||
| claude | ✗ ≈2,300 lines | ✅ mostly clean; vocabulary leaks only |
|
||
| email | ✗ ≈2,875 lines, no proxy | ◐ routes clean, payloads and realtime not |
|
||
| pty | ✗ ~all of 169 lines | ◐ clean boundary, but shell composition + a session leak |
|
||
| vnc | ✗ blocked on a missing listener | ✅ 149 lines, one route, no internals |
|
||
| vault | ◐ partial | — no frontend exists |
|
||
|
||
**A thin proxy alone does not produce a clean frontend — it relocates the problem.** When the platform
|
||
translates (claude, opencode, email), the browser receives Officer-shaped data and stays clean at the
|
||
cost of thousands of platform lines. When the platform is a dumb pass-through (slskd), the browser has
|
||
to speak the upstream API itself. Music is the only case that escapes the trade, and it escapes it
|
||
because **the sidecar exposes Officer-shaped routes** — the `/_officer/*` namespace — rather than only
|
||
proxying the upstream one.
|
||
|
||
So the rule as stated ("main server is a thin proxy") is necessary but not sufficient. The complete
|
||
version is: *the sidecar owns the contract the browser consumes.* Thinning a router without adding
|
||
`/_officer/*` routes to the sidecar just moves domain logic from the platform into the browser, which is
|
||
strictly worse — it is further from the data and unversioned.
|
||
|
||
## 2. Resilience is inconsistent, and that is the frontend's real problem
|
||
|
||
Pass 1 found an architecture problem. Pass 2 mostly finds a **resilience** problem — and it is
|
||
per-socket rather than systemic:
|
||
|
||
| channel | reconnect | replay |
|
||
|---|---|---|
|
||
| chat WS | ✅ `min(5000, 300 × retry)` | ✅ `seq` + `resume-cursor` (best in repo) |
|
||
| terminal / cliamp control WS | ✅ 5-entry table + visibility trigger | ◐ passive 50 KB sidecar buffer; browser unaware |
|
||
| cliamp audio WS | ✗ none | — n/a (live capture) |
|
||
| desktop / VNC WS | ✗ **none at all** | — n/a |
|
||
| email SSE | ✗ none, and no `id:` field server-side | ✗ so native `EventSource` retry can't replay either |
|
||
|
||
Two channels have thought-through recovery; three have none. The chat implementation is the template
|
||
and it already works — see the isolation document, where this finding shortened the plan.
|
||
|
||
**Four independent backoff implementations, no shared helper:** `useChatWebSocket.ts:46-58`,
|
||
`Terminal.tsx:141-145`, and the byte-identical pair `pty-sidecar.mjs:37` / `connect.ts:22` (Pass 1).
|
||
|
||
## 3. The bearer token travels in URLs on every socket
|
||
|
||
`useChat.ts:121-123` (chat WS), `Terminal.tsx:53-55` (terminal WS), `DesktopView.tsx:13-17` (VNC WS),
|
||
`EmailList.tsx:110-112` (email SSE) all read `localStorage.getItem('BEARER_TOKEN')` and append
|
||
`?token=…`. Unavoidable for `EventSource` and awkward for `WebSocket` (neither can set headers), and
|
||
this is the owner's own machine behind their own HTTPS proxy — but it does put a 30-day JWT into browser
|
||
history and any proxy access log, on four separate paths. If it's ever worth fixing, a short-lived
|
||
single-use ticket exchanged for the socket is the usual shape.
|
||
|
||
Related, from Pass 1: `server.tsx:308-311` serves `/novnc/*` with **no auth**, which Pass 2 confirms is
|
||
how the vendored noVNC client (`public/novnc/rfb.js`, 3,415 lines + ~40 modules) is loaded, via a
|
||
memoized dynamic `import()` (`DesktopView.tsx:28-37`).
|
||
|
||
## 4. Credential handling in the browser — one clear right answer, two wrong ones
|
||
|
||
- ✅ **VNC password**: fetched once into a local closure variable (`DesktopView.tsx:55`), passed to RFB
|
||
(`:83-85`, `:104-106`), never in state, storage, the query cache, or a URL.
|
||
- ✅ **Provider API keys**: masked server-side before the GET
|
||
(`api/server-settings/chat-providers.ts:384-391`), used only as a placeholder
|
||
(`AIHarnessesSection.tsx:467`), and the typed value is deleted from state after the PUT (`:121-125`).
|
||
- ✅ **Mail account passwords / OAuth**: write-only; the account list returns no credential, and OAuth
|
||
tokens never reach the browser (`EmailAccounts.tsx:88-107`).
|
||
- ✗ **SMTP password / Resend API key**: `GET /server-settings/smtp` returns them in plaintext and they
|
||
live in the React Query cache under `['SMTP_CONFIG']` for the life of the tab
|
||
(`SMTPSection.tsx:39, 63, 208-213`).
|
||
- ✗ **Google OAuth client secret**: returned in plaintext to prefill a form
|
||
(`GoogleOAuthConfig.tsx:197-210, 184, 255-262`).
|
||
|
||
The masking pattern in the chat-providers route is the fix for both failures, and it already exists in
|
||
this codebase.
|
||
|
||
## 5. Dead frontend code, verified by grepping for importers
|
||
|
||
- `FileBrowser/pcm-worklet-processor.js` (41) — zero importers; the worklet is inlined instead.
|
||
- `Settings/IntegrationsSettings/GoogleAccount.tsx` (98) — commented out of
|
||
`IntegrationsSettings/index.tsx:11, 71-79`.
|
||
- `officerdev/terminal-host` (`Terminal/index.tsx:47-54` + `HostTerminalWrapper.tsx`) — registered
|
||
`availableOnPanel: false`, referenced by no route, layout or panel map.
|
||
- `Terminal.tsx:225-226` — handles a `'detached'` message **no backend ever sends**.
|
||
- `GET /desktop/vnc-status` (`api/desktop/rest.ts:31-38`) — defined, never called.
|
||
- `api/terminal/websocket.ts:129-138` — the `cwd` handler; **no frontend sender exists** (the browser
|
||
composes its own `cd`), confirming Pass 1's flag from the other direction.
|
||
|
||
## 6. Duplicated constants across the frontend
|
||
|
||
`PROVIDER_DISPLAY` twice (`ModelSelector.tsx:7-23`, 14 entries; `AISettings.tsx:19-34`, 13 entries);
|
||
`AUDIO_EXT` twice (`shared.ts:96`, `widgets/MusicPlayer/index.tsx:19`); `MUSIC_ROOT` twice
|
||
(`shared.ts:4`, `widgets/MusicPlayer/index.tsx:12`); the PCM worklet twice (one copy dead). Small, but
|
||
each is a place where two files must be changed together and nothing enforces it.
|
||
|
||
## 7. Three bugs a user would notice, none of them architectural
|
||
|
||
Recorded because they surfaced during the audit, not because they're in scope:
|
||
|
||
1. **Closing a terminal panel leaks a shell.** `CommandTerminalWrapper.tsx:32-39` (and the two sibling
|
||
wrappers) delete the `panelId → sessionId` mapping on unmount, so the next mount generates a new
|
||
uuid and the detached shell becomes unreachable — with no kill path, since `pty:close` has no sender.
|
||
2. **Terminals don't re-fit after a resize.** `fitAddon.fit()` runs once per `connect()`
|
||
(`Terminal.tsx:158`); there is no `ResizeObserver` or window listener, so dragging a splitter leaves
|
||
the pty on stale dimensions until the next reconnect.
|
||
3. **opencode model capabilities shown to the user are placeholder constants.**
|
||
`api/chat/list-models.ts:24` stubs every opencode model at `contextWindow: 200000, maxTokens: 8192,
|
||
reasoning: false`, and `ModelSelector.tsx:116` hides the thinking toggle based on that `false`.
|
||
|
||
---
|
||
|
||
**End of Pass 2.** Both passes are complete. Nothing in either document has been changed in code.
|