From a1a33cf0502efc8045f4090d7467e7d23903868d Mon Sep 17 00:00:00 2001 From: Randalix Date: Thu, 15 Oct 2026 12:17:45 +0200 Subject: [PATCH] fix(agent-inbox): review round 1 - codeman agent post: accept text that starts with "-" (markdown from a file), --json and -h stay options. - DELETE /api/sessions/:id on a persisted-only row drops the inbox a detach kept. - inbox --wait: wait.ended is exit 3 only when the session list no longer has the session; a restart or detach ending the wait is exit 2 (wait again). - A read whose client hung up during ?wait= marks nothing seen. - InboxPostSchema bound is 4x the byte cap, so every oversize message gets the store's "post a file path" answer. - Store: restore sorts oldest first; the TTL sweep checks every message, not only the first. - Docs: admins can act as any session of the receiver's owner (now tested for admin and non-admin); a peek with ?wait= does not block; exit codes. - Tests: partial PUT /api/settings keeps persistence on. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/api-reference.md | 14 ++++-- docs/architecture-invariants.md | 4 +- .../codeman/skills/codeman/reference/verbs.md | 8 +++- skills/codeman/reference/verbs.md | 8 +++- src/cli-agent.ts | 30 ++++++++++-- src/web/agent-inbox.ts | 13 ++++-- src/web/routes/inbox-routes.ts | 11 +++-- src/web/routes/session-routes.ts | 4 ++ src/web/schemas.ts | 9 +++- src/web/server.ts | 5 +- test/agent-inbox.test.ts | 16 +++++++ test/cli-agent.test.ts | 46 +++++++++++++++++-- test/routes/inbox-routes.test.ts | 30 +++++++++++- test/routes/session-routes.test.ts | 12 +++++ ...system-routes-settings-partial-put.test.ts | 30 +++++++++++- 15 files changed, 209 insertions(+), 31 deletions(-) diff --git a/docs/api-reference.md b/docs/api-reference.md index 8ac38050..1c335683 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -591,7 +591,7 @@ request returns. The capture runs synchronous tmux calls on the server; the ## The `codeman agent` CLI (client over these endpoints) -`codeman agent ls|spawn|send|wait|read|interrupt|rm|post|inbox|ack` (`src/cli-agent.ts`) is the command-line client for the endpoints above, for agents the skill does not reach (a pi or shell session, a machine without `jq`). It adds no route: `spawn` is `POST /api/v1/quick-start` (+ `wait-output` on the mode's `capabilities.composerReadyMark` from the CLI registry, where it declares one), `send` is `POST …/input` with `clientId`+`seq` (and `wait`/`waitTimeout` for `--wait` / `--until `; `delivered:false` without `duplicate` and `wait.ended` both exit 3 — the CLI never reports a dead worker as done), `wait` is `GET …/wait` (`--until`) or `GET …/wait-output` (`--match`, `from=buffer` by default), `read` is `GET …/last-response` or `GET …/terminal?tail=`, `interrupt` is `POST …/input` with a bare `\u001b`, `rm` is `DELETE …/sessions/:id`, and the inbox verbs map one to one onto the [agent inbox](#agent-inbox) routes (`post` → `POST …/inbox`, `inbox [id] [--wait]` → `GET …/inbox[?wait=]`, own inbox by default, `ack ` → `POST …/inbox/ack`); `ls` adds an `INBOX` column from `GET /api/v1/agent-inbox/summary` and drops it when that route does not answer. A fire-and-forget `send` to a sleeping wake-on-LAN host reads the route's `buffered` (own line, exit 0) and `dropped` (exit 1: the chunk is gone). An id may be the 8-character form `ls` prints, resolved through `GET /api/v1/sessions`; anything shorter refuses before any request, the same floor as `PARENT_SESSION_ID_MIN_PREFIX`. Every call carries `X-Codeman-Parent-Session`; only `spawn`'s quick-start carries `X-Codeman-Agent-Origin: codeman-agent-cli` (the agent-scratch label must never reach a request that cannot create the case directory). Basic auth comes from `CODEMAN_PASSWORD` or the data dir's `.env`. Server-side error codes are shown verbatim (`INVALID_INPUT: until=stop …` on a hook-less mode is not hidden); exit codes are `0` ok, `1` error, `2` timeout, `3` the session exited, `4` refused by a client-side guard. See the README section "`codeman agent`" for the guards and `test/cli-agent.test.ts` for the pinned behaviour. +`codeman agent ls|spawn|send|wait|read|interrupt|rm|post|inbox|ack` (`src/cli-agent.ts`) is the command-line client for the endpoints above, for agents the skill does not reach (a pi or shell session, a machine without `jq`). It adds no route: `spawn` is `POST /api/v1/quick-start` (+ `wait-output` on the mode's `capabilities.composerReadyMark` from the CLI registry, where it declares one), `send` is `POST …/input` with `clientId`+`seq` (and `wait`/`waitTimeout` for `--wait` / `--until `; `delivered:false` without `duplicate` and `wait.ended` both exit 3 — the CLI never reports a dead worker as done), `wait` is `GET …/wait` (`--until`) or `GET …/wait-output` (`--match`, `from=buffer` by default), `read` is `GET …/last-response` or `GET …/terminal?tail=`, `interrupt` is `POST …/input` with a bare `\u001b`, `rm` is `DELETE …/sessions/:id`, and the inbox verbs map one to one onto the [agent inbox](#agent-inbox) routes (`post` → `POST …/inbox`, `inbox [id] [--wait]` → `GET …/inbox[?wait=]`, own inbox by default, `ack ` → `POST …/inbox/ack`); `ls` adds an `INBOX` column from `GET /api/v1/agent-inbox/summary` and drops it when that route does not answer. A fire-and-forget `send` to a sleeping wake-on-LAN host reads the route's `buffered` (own line, exit 0) and `dropped` (exit 1: the chunk is gone). An id may be the 8-character form `ls` prints, resolved through `GET /api/v1/sessions`; anything shorter refuses before any request, the same floor as `PARENT_SESSION_ID_MIN_PREFIX`. Every call carries `X-Codeman-Parent-Session`; only `spawn`'s quick-start carries `X-Codeman-Agent-Origin: codeman-agent-cli` (the agent-scratch label must never reach a request that cannot create the case directory). Basic auth comes from `CODEMAN_PASSWORD` or the data dir's `.env`. Server-side error codes are shown verbatim (`INVALID_INPUT: until=stop …` on a hook-less mode is not hidden); exit codes are `0` ok, `1` error, `2` timeout, `3` the session exited, `4` refused by a client-side guard (for `inbox --wait`, `3` only when `GET /api/v1/sessions` no longer lists the session; a wait the server ended otherwise, on a restart or a detach, is `2`). `post` takes text that starts with `-` as it is (a markdown file often does); text that is exactly an option (`--json`, `-h`) goes after `--`. See the README section "`codeman agent`" for the guards and `test/cli-agent.test.ts` for the pinned behaviour. `codeman mcp` (`src/mcp-server.ts`) is a Model Context Protocol server on stdio over the same endpoints, for MCP clients: `list_sessions` (`GET /api/v1/sessions`), `start_session` (`POST /api/v1/quick-start` with `X-Codeman-Agent-Origin: codeman-mcp`, then `wait-output` on the mode's ready mark), `send_prompt` (`POST …/input` with `wait`/`waitTimeout`), `wait_for_session` (`GET …/wait` or `…/wait-output`), `read_output` (`GET …/last-response` or `…/terminal?tail=`), `interrupt_session` and `delete_session`. It adds no route. The URL comes from `CODEMAN_API_URL` or `--url`, never a guess; prompting, interrupting and deleting are limited to sessions the process started (or the calling session's own children) unless it runs with `--allow-any-session`. See the README section "MCP server" and `test/mcp-server.test.ts`. @@ -619,7 +619,9 @@ request, with the same resolver as session lineage: a live session the caller can see, with the same owner as the receiving session, matched by exact id or a unique prefix of at least 8 characters. Anything else counts as no session. The body cannot name a sender (`InboxPostSchema` is strict). In multi-user mode -that means a caller cannot claim to be another owner's session. In single-user +that means a caller cannot claim to be another owner's session. Admins are the +exception, as everywhere else: an admin can see every session, so an admin's +request can act as any session of the receiver's owner. In single-user mode every local process holds the same credentials, so the header is a hint the client gives about itself, not proof. @@ -639,7 +641,13 @@ pending does not spin. The value is clamped to the agent-wait bounds like waiter counts against the same per-session, per-owner and total caps (`409 SESSION_BUSY` / `429 RATE_LIMITED`). `wait` has the shape of the other wait results plus `posted` (a post released it). A session deleted during the wait -answers `wait.ended: true` with an empty inbox. +answers `wait.ended: true` with an empty inbox. A detach or a server shutdown +ends the wait with `ended: true` as well, so check the session list before +treating the session as gone (`codeman agent inbox --wait` does: exit 3 only +when the list lacks it, exit 2 otherwise). A client that hangs up during the wait +marks nothing seen. A **peek** with `?wait=` returns at once for as long as the +receiver has unseen mail, since only the receiver's read clears it; to watch +whether a worker has read its mail, poll the summary instead. **Limits.** 200 messages, 16384 bytes (UTF-8) per message, 512 KiB of text per inbox. A message over the per-message cap is `400 INVALID_INPUT`. A post that diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 6cde5a00..b1d062fa 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -694,13 +694,13 @@ Frontend `readmymind-ui.js`: header 🧠 marker-hidden (`btn-readmymind--hidden` ⚠️ **Read until ack.** A read removes nothing; the same messages come back until the receiver acks them BY ID (`POST …/inbox/ack`, idempotent: an id already gone is not an error). Acking by id rather than "everything I read" is what keeps a message posted between the read and the ack from being lost. -⚠️ **Seen and ack belong to the receiver.** The caller is whatever `resolveParentSessionId()` makes of `X-Codeman-Parent-Session` (live session, same owner as the receiver), never a body field (`InboxPostSchema` is strict). Only a read whose caller IS the receiver marks messages seen; anyone else's read is a peek. An ack from anyone else is a 403, not a silent no-op, so a coordinator never believes it cleared a worker's mail. In multi-user mode this stops a caller from naming another owner's session; in single-user mode every local process holds the same credentials, so the header is the client's own claim about itself, which is all the seen/peek split needs. +⚠️ **Seen and ack belong to the receiver.** The caller is whatever `resolveParentSessionId()` makes of `X-Codeman-Parent-Session` (live session, same owner as the receiver), never a body field (`InboxPostSchema` is strict). Only a read whose caller IS the receiver marks messages seen; anyone else's read is a peek. An ack from anyone else is a 403, not a silent no-op, so a coordinator never believes it cleared a worker's mail. In multi-user mode this stops a non-admin caller from naming another owner's session (an admin passes `canAccessOwned` for every session, so an admin can act as any session of the receiver's owner); in single-user mode every local process holds the same credentials, so the header is the client's own claim about itself, which is all the seen/peek split needs. ⚠️ **`?wait=` parks while nothing is UNSEEN**, not while nothing is pending: a receiver holding a seen-but-unacked message would otherwise get an immediate answer on every loop and spin. Inbox waiters are the registry's third waiter kind (`waitForInbox`/`notifyInbox`), so they count against the same per-session/owner/total caps and are ended by `cancelAll` like signal and output waiters. `notifyInbox` runs AFTER the store, so a woken reader finds the message. `parked` in the summary counts receiver waiters only. ⚠️ **Caps refuse, they never drop.** 16 KiB per message (UTF-8 bytes), 512 KiB and 200 messages per inbox; a full inbox is 409 CONFLICT and nothing is stored. Dropping the oldest would lose exactly the message a slow receiver has not handled yet. TTL is 12 h from each message's own `createdAt`, swept lazily, and a restore keeps `createdAt`, so a restart does not restart the clock. -⚠️ **Lifetime follows the session, not the pane.** `_doCleanupSession` drops the inbox only when `killMux` is set (a real delete); a detach or server shutdown keeps it. Persistence (`agentInboxPersistEnabled`, SYNCED, default OFF) writes `agent-inbox.json` via `dataPath()`, 0600 through tmp+rename, debounced and flushed on stop; off means no file, and switching it off deletes the snapshot while memory stays. It is never fed to `/api/search`, and `sensitive-path.ts` keeps the file viewer off it. Boot loads the snapshot before `restoreMuxSessions`, but prunes inboxes of sessions that no longer exist only in `finalizeRestoredState`'s SUCCESS path: a failed restore must not delete mail for sessions that are about to come back. Tests: `test/agent-inbox.test.ts`, `test/agent-inbox-persistence.test.ts`, `test/routes/inbox-routes.test.ts`, `test/agent-inbox-server-wiring.test.ts`, the inbox block in `test/session-wait-registry.test.ts`. +⚠️ **Lifetime follows the session, not the pane.** `_doCleanupSession` drops the inbox only when `killMux` is set (a real delete); a detach or server shutdown keeps it, and `DELETE /api/sessions/:id` on a row that is no longer live (the persisted-only branch in `session-routes.ts`) drops what the detach kept. A detach also ends a parked read with `ended: true`, which is why `codeman agent inbox --wait` asks the session list before it exits 3. Persistence (`agentInboxPersistEnabled`, SYNCED, default OFF) writes `agent-inbox.json` via `dataPath()`, 0600 through tmp+rename, debounced and flushed on stop; off means no file, and switching it off deletes the snapshot while memory stays. It is never fed to `/api/search`, and `sensitive-path.ts` keeps the file viewer off it. Boot loads the snapshot before `restoreMuxSessions`, but prunes inboxes of sessions that no longer exist only in `finalizeRestoredState`'s SUCCESS path: a failed restore must not delete mail for sessions that are about to come back. Tests: `test/agent-inbox.test.ts`, `test/agent-inbox-persistence.test.ts`, `test/routes/inbox-routes.test.ts`, `test/agent-inbox-server-wiring.test.ts`, the inbox block in `test/session-wait-registry.test.ts`. ### Voice dictation via Claude diff --git a/plugins/codeman/skills/codeman/reference/verbs.md b/plugins/codeman/skills/codeman/reference/verbs.md index 5896d8b0..65af5c65 100644 --- a/plugins/codeman/skills/codeman/reference/verbs.md +++ b/plugins/codeman/skills/codeman/reference/verbs.md @@ -795,10 +795,14 @@ Rules that matter: changes nothing for it. Only the receiver can ack (403 otherwise). - **`--wait` waits for something new.** It returns at once when there is unseen mail and otherwise blocks until a post arrives; seen but unacked messages come back with it - and do not end the wait. Timeout is exit 2, the session vanishing is exit 3. + and do not end the wait. Timeout is exit 2, and so is a wait the server cut short + (a restart): loop again. Exit 3 means the session list no longer has the session. + `--wait` on someone else's inbox returns at once while they have unseen mail (only + their read clears it); check `ls` instead. - **The sender is resolved, not claimed.** `from` is the session the header resolves to, or `null` (the user). In single-user mode that is a hint, since every local process - holds the same credentials. + holds the same credentials. In multi-user mode it is checked against the owner; + an admin can still act as any session of the receiver's owner. - **Full means refused.** 200 messages or 512 KiB per inbox, 16 KiB per message. A full inbox answers 409 and stores nothing; the oldest is never dropped. Messages expire 12 hours after they were posted. diff --git a/skills/codeman/reference/verbs.md b/skills/codeman/reference/verbs.md index 5896d8b0..65af5c65 100644 --- a/skills/codeman/reference/verbs.md +++ b/skills/codeman/reference/verbs.md @@ -795,10 +795,14 @@ Rules that matter: changes nothing for it. Only the receiver can ack (403 otherwise). - **`--wait` waits for something new.** It returns at once when there is unseen mail and otherwise blocks until a post arrives; seen but unacked messages come back with it - and do not end the wait. Timeout is exit 2, the session vanishing is exit 3. + and do not end the wait. Timeout is exit 2, and so is a wait the server cut short + (a restart): loop again. Exit 3 means the session list no longer has the session. + `--wait` on someone else's inbox returns at once while they have unseen mail (only + their read clears it); check `ls` instead. - **The sender is resolved, not claimed.** `from` is the session the header resolves to, or `null` (the user). In single-user mode that is a hint, since every local process - holds the same credentials. + holds the same credentials. In multi-user mode it is checked against the owner; + an admin can still act as any session of the receiver's owner. - **Full means refused.** 200 messages or 512 KiB per inbox, 16 KiB per message. A full inbox answers 409 and stores nothing; the oldest is never dropped. Messages expire 12 hours after they were posted. diff --git a/src/cli-agent.ts b/src/cli-agent.ts index cf0d76fd..8bd51e96 100644 --- a/src/cli-agent.ts +++ b/src/cli-agent.ts @@ -890,6 +890,17 @@ export interface InboxReadOptions { waitMs?: number; } +/** True only when the session list was read and does not contain `id`. */ +async function sessionIsGone(deps: AgentDeps, id: string): Promise { + try { + const res = await deps.request(deps.ctx, { method: 'GET', path: '/api/v1/sessions' }); + if (!res.json?.success) return false; + return !((res.json.data as SessionRow[] | undefined) ?? []).some((s) => s.id === id); + } catch { + return false; + } +} + /** Relative age for humans: `12s`, `4m`, `3h`. */ function age(createdAt: number, now: number): string { const s = Math.max(0, Math.round((now - createdAt) / 1000)); @@ -901,8 +912,8 @@ function age(createdAt: number, now: number): string { /** * `agent inbox` — read this session's inbox (or peek at another's). Removes nothing: * a message stays until `agent ack `. Only reading your OWN inbox marks messages - * seen. Exit 0 with mail, 2 when a `--wait` ran out with nothing unseen, 3 when the - * session went away during the wait. + * seen. Exit 0 with mail, 2 when a `--wait` ran out with nothing unseen or the server + * cut it short (restart, detach), 3 when the session is gone from the list. */ export async function agentInboxRead(deps: AgentDeps, options: InboxReadOptions): Promise { const target = await resolveSessionId(deps, options.id ?? deps.ctx.selfId); @@ -924,7 +935,12 @@ export async function agentInboxRead(deps: AgentDeps, options: InboxReadOptions) } | undefined; const messages = data?.messages ?? []; - const code = data?.wait?.ended ? EXIT.dead : data?.wait?.timedOut ? EXIT.timeout : EXIT.ok; + // `ended` is also what a server restart or a detach answers. Exit 3 tells a receiver + // loop to stop, so it needs the session list to confirm the session is gone; while + // it is still listed (or the list cannot be had, as during a restart) the wait was + // only cut short, which is exit 2: loop again. + const gone = data?.wait?.ended ? await sessionIsGone(deps, target.id) : false; + const code = gone ? EXIT.dead : data?.wait?.timedOut || data?.wait?.ended ? EXIT.timeout : EXIT.ok; if (deps.json) { emitJson(deps, data ?? {}); return code; @@ -936,8 +952,10 @@ export async function agentInboxRead(deps: AgentDeps, options: InboxReadOptions) deps.io.out(palette.muted(`── ${m.id} from ${sender}, ${age(m.createdAt, now)} ago, `) + tag); deps.io.out(m.text); } - if (data?.wait?.ended) { + if (gone) { deps.io.err(palette.err(`${GLYPH.fail} the session went away during the wait`)); + } else if (data?.wait?.ended) { + deps.io.err(palette.muted('(the server ended the wait early, e.g. a restart; exit 2, wait again)')); } else if (messages.length === 0) { deps.io.err(palette.muted(data?.wait?.timedOut ? '(no mail; wait timed out, exit 2)' : '(no mail)')); } else if (data?.receiver) { @@ -1165,6 +1183,10 @@ export function registerAgentCommands(program: Command): Command { "Leave a message in another session's inbox (ONE quoted argument; multi-line is fine). Nothing is typed into its pane: it reads with `agent inbox`" ) .option('--json', 'Machine-readable output') + // A message is often a file (`"$(cat task.md)"`) and markdown starts with `- ` or + // `---` often enough; without this commander takes it for an unknown option. + // `--json` and `-h` are still options: text that is exactly one goes after `--`. + .allowUnknownOption() .action((id: string, words: string[], options: { json?: boolean }) => run(Boolean(options.json), (deps) => { const message = postTextFromArgs(words); diff --git a/src/web/agent-inbox.ts b/src/web/agent-inbox.ts index 7b5822e2..ae0013db 100644 --- a/src/web/agent-inbox.ts +++ b/src/web/agent-inbox.ts @@ -228,8 +228,12 @@ export class AgentInbox { if (!Array.isArray(raw)) continue; const queue: StoredMessage[] = []; let bytes = 0; - for (const entry of raw) { - if (!isStoredMessage(entry) || entry.createdAt <= cutoff) continue; + // Oldest first whatever order the file has, so the caps keep the oldest mail, + // which is the rule for posts too. + const entries = raw + .filter((entry): entry is StoredMessage => isStoredMessage(entry) && entry.createdAt > cutoff) + .sort((a, b) => a.createdAt - b.createdAt); + for (const entry of entries) { const size = Buffer.byteLength(entry.text, 'utf8'); if (queue.length >= MAX_MESSAGES_PER_INBOX || bytes + size > MAX_INBOX_BYTES) break; queue.push({ ...entry }); @@ -253,8 +257,9 @@ export class AgentInbox { const queue = this.inboxes.get(sessionId); if (!queue) return []; const cutoff = this.now() - MESSAGE_TTL_MS; - if (queue.length === 0 || queue[0].createdAt > cutoff) return queue; - // Oldest first, so the expired ones are a prefix. + // Every message, not only queue[0]: a clock stepping back can put an older + // createdAt behind a newer one. At most 200 entries, so the scan is cheap. + if (!queue.some((m) => m.createdAt <= cutoff)) return queue; const kept = queue.filter((m) => m.createdAt > cutoff); this.store(sessionId, kept); this.onChange?.(); diff --git a/src/web/routes/inbox-routes.ts b/src/web/routes/inbox-routes.ts index 4d19c67e..ff5285e5 100644 --- a/src/web/routes/inbox-routes.ts +++ b/src/web/routes/inbox-routes.ts @@ -8,8 +8,10 @@ * the caller can see that has the same owner as the receiver (exact id, or a unique * prefix of at least 8). Anything else is "no session": a post is from the user, and a * read is a peek. What that buys: in multi-user mode a caller cannot name another - * owner's session. In single-user mode every local process holds the same - * credentials, so the header is a hint the client sets about itself. + * owner's session (an admin, who can see every session, can act as any session of + * the receiver's owner, as admins can everywhere else). In single-user mode every + * local process holds the same credentials, so the header is a hint the client sets + * about itself. * * - `POST /api/sessions/:id/inbox` stores a message. `from` is the resolved caller or * null. A full inbox is 409 CONFLICT; the oldest message is never dropped. @@ -135,8 +137,9 @@ export function registerInboxRoutes(app: FastifyInstance, ctx: SessionPort): voi } // Read AFTER the wait: this is what the caller gets, so it must be current. A wait - // that ended because the session was deleted reads an empty inbox. - const read = agentInbox.read(id, { receiver }); + // that ended because the session was deleted reads an empty inbox. A client that + // hung up never sees this answer, so its read must not mark anything seen. + const read = agentInbox.read(id, { receiver: receiver && !wait?.aborted }); return { success: true, data: { sessionId: id, receiver, ...read, ...(wait ? { wait } : {}) } }; }); diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index dfa81fea..21c9a02f 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -119,6 +119,7 @@ import { abortOnClientHangUp, } from '../route-helpers.js'; import { buildAgentCaseMarker, writeAgentCaseMarker } from '../../agent-case-marker.js'; +import { agentInbox } from '../agent-inbox.js'; import { canUsernameRunPrivilegedCommands, resolveClaudeModeForUsername } from '../../user-store.js'; import { clampEnvOverridesForOwner } from '../../session-env-clamp.js'; import { enabledClis, getCli } from '../../config/cli-registry/registry.js'; @@ -1680,6 +1681,9 @@ export function registerSessionRoutes( // below re-looks-up the record by id, so the returned SessionState is unused here. findPersistedSessionOrFail(ctx.store, id, req); ctx.store.demoteOrRemoveSession(id); + // A detached session kept its agent inbox (server.ts _doCleanupSession drops it + // only on killMux); retiring the row is the delete, so the mail goes now. + agentInbox.drop(id); // Mirrors the broadcast at the tail of the live-session cleanup path // (_doCleanupSession in server.ts) — without it, other open tabs keep // showing the retired row until their next unrelated fetch. diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 2a815163..a240e51e 100644 --- a/src/web/schemas.ts +++ b/src/web/schemas.ts @@ -1847,12 +1847,17 @@ export const SessionWaitOutputQuerySchema = z.object({ /** * Body for `POST /api/sessions/:id/inbox`. The byte cap (`MAX_MESSAGE_BYTES`) is the * store's, checked there in UTF-8 bytes; this bound is only the outer fence so a - * multi-megabyte body never reaches it. Multi-line text is fine: it is stored and + * multi-megabyte body never reaches it. It is 4× the byte cap in characters (a UTF-8 + * character is at most 4 bytes), so every over-size message gets the store's answer, + * which tells the sender what to do instead. Multi-line text is fine: it is stored and * handed back as data, never typed. */ export const InboxPostSchema = z .object({ - text: z.string().min(1, 'text must not be empty').max(16_384), + text: z + .string() + .min(1, 'text must not be empty') + .max(4 * 16_384), }) .strict(); diff --git a/src/web/server.ts b/src/web/server.ts index 6b6408bc..bad6b2f6 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -1691,8 +1691,9 @@ export class WebServer extends EventEmitter { // detached a few lines above, before `session.stop()`, so on a delete the // session's own exit event never reaches the registry. // The agent inbox goes with a DELETED session only. killMux=false is shutdown or - // detach, and a detached session comes back to the mail posted while it was away. - // Before cancelAll, so a reader released by it reads an empty inbox. + // detach: mail posted before that is still there when the session is re-attached + // (while detached, the routes answer 404, so nothing new arrives). Before + // cancelAll, so a reader released by it reads an empty inbox. if (killMux) agentInbox.drop(sessionId); sessionWaits.notifySignal(sessionId, 'exit'); sessionWaits.cancelAll(sessionId); diff --git a/test/agent-inbox.test.ts b/test/agent-inbox.test.ts index a611a37f..8fa576b5 100644 --- a/test/agent-inbox.test.ts +++ b/test/agent-inbox.test.ts @@ -161,6 +161,15 @@ describe('AgentInbox', () => { expect(new AgentInbox(() => now).restore(snap)).toBe(0); }); + it('sweeps an expired message that is not first (clock stepped back)', () => { + const late = post('posted first, stamped later'); + now -= 1000; // the clock steps back + const early = post('posted second, stamped earlier'); + now += MESSAGE_TTL_MS + 1000 - (late.createdAt - early.createdAt) / 2; + // `early` is past the TTL, `late` is not, and `early` sits behind `late`. + expect(inbox.read(RX, { receiver: false }).messages.map((m) => m.id)).toEqual([late.id]); + }); + it('expiry fires onChange so the snapshot catches up', () => { post('m'); let changes = 0; @@ -248,6 +257,13 @@ describe('AgentInbox', () => { ).toBe(1); }); + it('restores oldest first whatever the file order, so the caps keep the oldest', () => { + const msg = (id: string, createdAt: number) => ({ id, from: null, text: 'x', createdAt, seenAt: null }); + const restored = new AgentInbox(() => now); + restored.restore({ version: 1, inboxes: { s: [msg('new', now - 10), msg('old', now - 20)] } }); + expect(restored.read('s', { receiver: false }).messages.map((m) => m.id)).toEqual(['old', 'new']); + }); + it('holds the count cap on restore', () => { const many = Array.from({ length: MAX_MESSAGES_PER_INBOX + 5 }, (_, i) => ({ id: `m${i}`, diff --git a/test/cli-agent.test.ts b/test/cli-agent.test.ts index 55ad117e..5ec13474 100644 --- a/test/cli-agent.test.ts +++ b/test/cli-agent.test.ts @@ -714,12 +714,32 @@ describe('agent inbox verbs', () => { expect(deps.err.join('')).toMatch(/peek: nothing was marked seen/); }); - it('inbox --wait passes the budget and maps timeout to exit 2, a vanished session to exit 3', async () => { + it('inbox --wait passes the budget and maps timeout to exit 2', async () => { const timedOut = fakeDeps([ok({ receiver: true, messages: [], pending: 0, unseen: 0, wait: { timedOut: true } })]); expect(await agentInboxRead(timedOut, { waitMs: 30_000 })).toBe(EXIT.timeout); expect(timedOut.calls[0]).toMatchObject({ query: { wait: 30_000 }, timeoutMs: 40_000 }); - const ended = fakeDeps([ok({ receiver: true, messages: [], pending: 0, unseen: 0, wait: { ended: true } })]); - expect(await agentInboxRead(ended, { waitMs: 30_000 })).toBe(EXIT.dead); + }); + + describe('a wait the server ended', () => { + const ended = ok({ receiver: true, messages: [], pending: 0, unseen: 0, wait: { ended: true } }); + + it('is exit 3 only when the session list confirms the session is gone', async () => { + const deps = fakeDeps([ended, ok([{ id: OTHER }])]); + expect(await agentInboxRead(deps, { waitMs: 30_000 })).toBe(EXIT.dead); + expect(deps.calls[1]).toMatchObject({ method: 'GET', path: '/api/v1/sessions' }); + expect(deps.err.join('')).toMatch(/went away/); + }); + + it('is exit 2 (wait again) while the session is still listed: a restart or detach', async () => { + const deps = fakeDeps([ended, ok([{ id: SELF }])]); + expect(await agentInboxRead(deps, { waitMs: 30_000 })).toBe(EXIT.timeout); + expect(deps.err.join('')).toMatch(/ended the wait early/); + }); + + it('is exit 2 when the list cannot be had (the server is still coming back)', async () => { + const deps = fakeDeps([ended, apiError(503, 'INTERNAL_ERROR', 'restarting')]); + expect(await agentInboxRead(deps, { waitMs: 30_000 })).toBe(EXIT.timeout); + }); }); it('ack posts the ids to this session’s own inbox', async () => { @@ -767,6 +787,26 @@ describe('help texts carry the traps the skill documents', () => { expect(sub('send').description()).toContain('send -- "- fix the bug"'); }); + it('post takes text that starts with "-" (markdown from a file) without --', async () => { + const program = new Command(); + program.exitOverride(); + const post = registerAgentCommands(program).commands.find((c) => c.name() === 'post')!; + const got: Array<{ words: string[]; json: boolean }> = []; + post.action((_id: string, words: string[], o: { json?: boolean }) => { + got.push({ words, json: Boolean(o.json) }); + }); + for (const text of ['- [ ] item\nnext', '---\ntitle: a', '-v is great']) { + await program.parseAsync(['node', 'codeman', 'agent', 'post', 'abcdefgh', text]); + } + await program.parseAsync(['node', 'codeman', 'agent', 'post', 'abcdefgh', 'hi', '--json']); + expect(got).toEqual([ + { words: ['- [ ] item\nnext'], json: false }, + { words: ['---\ntitle: a'], json: false }, + { words: ['-v is great'], json: false }, + { words: ['hi'], json: true }, + ]); + }); + it('rm does not claim a lineage check it does not make', () => { expect(sub('rm').description()).toMatch(/^Delete any session except this one/); }); diff --git a/test/routes/inbox-routes.test.ts b/test/routes/inbox-routes.test.ts index b37b3b1f..4661ef1e 100644 --- a/test/routes/inbox-routes.test.ts +++ b/test/routes/inbox-routes.test.ts @@ -101,6 +101,28 @@ describe('agent inbox routes', () => { expect(res.statusCode).toBe(400); }); + it('multi-user, non-admin: a session the caller cannot see never resolves as the sender', async () => { + process.env.CODEMAN_MULTIUSER = '1'; + currentUser = { username: 'alice', role: 'user' }; + (h.ctx.sessions.get(RX) as unknown as { owner: string }).owner = 'alice'; + (h.ctx.sessions.get(TX) as unknown as { owner: string }).owner = 'bob'; + (h.ctx.sessions.get(OTHER) as unknown as { owner: string }).owner = 'alice'; + expect(data(await post(RX, 'x', TX)).message.from).toBeNull(); + expect(data(await post(RX, 'y', OTHER)).message.from).toBe(OTHER); + // Nor can alice read as bob's session or ack for it: bob's inbox is a 404 to her. + expect((await read(TX, TX)).statusCode).toBe(404); + expect((await ack(TX, ['m'], TX)).statusCode).toBe(404); + }); + + it('multi-user, admin: may act as any session of the receiver’s owner (admins see everything)', async () => { + process.env.CODEMAN_MULTIUSER = '1'; + currentUser = { username: 'root', role: 'admin' }; + (h.ctx.sessions.get(RX) as unknown as { owner: string }).owner = 'bob'; + (h.ctx.sessions.get(TX) as unknown as { owner: string }).owner = 'bob'; + expect(data(await post(RX, 'x', TX)).message.from).toBe(TX); + expect(data(await read(RX, RX)).receiver).toBe(true); + }); + it('multi-user: a session of another owner never resolves as the sender', async () => { process.env.CODEMAN_MULTIUSER = '1'; currentUser = { username: 'alice', role: 'admin' }; @@ -128,8 +150,12 @@ describe('agent inbox routes', () => { expect(data(await read(RX)).messages[0].id).toBe(first); }); - it('a message over the byte cap is 400', async () => { - expect((await post(RX, 'é'.repeat(8193))).statusCode).toBe(400); + it('a message over the byte cap is 400 with the store’s hint, multi-byte or not', async () => { + for (const text of ['é'.repeat(8193), 'x'.repeat(16_385)]) { + const res = await post(RX, text); + expect(res.statusCode).toBe(400); + expect(JSON.parse(res.body).error).toMatch(/Write it to a file in the workspace and post the path/); + } }); it('404 for an unknown session', async () => { diff --git a/test/routes/session-routes.test.ts b/test/routes/session-routes.test.ts index 1d6eaf8b..c9777769 100644 --- a/test/routes/session-routes.test.ts +++ b/test/routes/session-routes.test.ts @@ -26,6 +26,7 @@ import { createMockRouteContext, type MockRouteContext } from '../mocks/index.js import { installRouteErrorHandler } from '../../src/web/route-error-handler.js'; import { ApiErrorCode, httpStatusForErrorCode } from '../../src/types.js'; import { Session } from '../../src/session.js'; +import { agentInbox } from '../../src/web/agent-inbox.js'; // Mock execFile so the send-key route's `tmux` invocation is observable (not run for real). const { execFile } = vi.hoisted(() => ({ execFile: vi.fn() })); @@ -985,6 +986,17 @@ describe('session-routes', () => { expect(harness.ctx.broadcast).toHaveBeenCalledWith('session:deleted', { id: 'ghost-session' }); }); + it('a persisted-only delete drops the agent inbox the detach kept', async () => { + // Detach (killMux=false) keeps the inbox; retiring the row afterwards is the delete. + agentInbox.resetForTests(); + agentInbox.post('ghost-session', null, 'mail from before the detach'); + vi.mocked(harness.ctx.store.getSession).mockReturnValueOnce({ id: 'ghost-session', owner: undefined } as never); + const res = await harness.app.inject({ method: 'DELETE', url: '/api/sessions/ghost-session' }); + expect(res.statusCode).toBe(200); + expect(agentInbox.pendingCount('ghost-session')).toBe(0); + agentInbox.resetForTests(); + }); + it('404s a persisted-only session id the state store does not recognize either', async () => { vi.mocked(harness.ctx.store.getSession).mockReturnValueOnce(null); const res = await harness.app.inject({ diff --git a/test/routes/system-routes-settings-partial-put.test.ts b/test/routes/system-routes-settings-partial-put.test.ts index b6876843..416b1d64 100644 --- a/test/routes/system-routes-settings-partial-put.test.ts +++ b/test/routes/system-routes-settings-partial-put.test.ts @@ -47,13 +47,15 @@ const { EXISTING_SETTINGS, subagentWatcher, imageWatcher, workflowRunWatcher } = }; }; return { - // Persisted settings.json for these tests: two watchers ON, subagent tracking OFF. + // Persisted settings.json for these tests: two watchers ON, subagent tracking OFF, + // agent inbox snapshot ON. EXISTING_SETTINGS: { subagentTrackingEnabled: false, imageWatcherEnabled: true, showUltracodeAgents: true, promptCacheKeepWarm: false, promptCacheKeepWarmMaxHours: 2, + agentInboxPersistEnabled: true, }, subagentWatcher: makeWatcher(false), imageWatcher: makeWatcher(true), @@ -61,6 +63,9 @@ const { EXISTING_SETTINGS, subagentWatcher, imageWatcher, workflowRunWatcher } = }; }); +// setEnabled(false) deletes the snapshot file, so a wrong default here loses mail. +const { agentInboxPersistence } = vi.hoisted(() => ({ agentInboxPersistence: { setEnabled: vi.fn() } })); + vi.mock('node:fs/promises', () => ({ default: { readFile: vi.fn(async () => JSON.stringify(EXISTING_SETTINGS)), @@ -76,6 +81,7 @@ vi.mock('node:fs', async (importOriginal) => { vi.mock('../../src/subagent-watcher.js', () => ({ subagentWatcher })); vi.mock('../../src/image-watcher.js', () => ({ imageWatcher })); vi.mock('../../src/workflow-run-watcher.js', () => ({ workflowRunWatcher })); +vi.mock('../../src/web/agent-inbox-persistence.js', () => ({ agentInboxPersistence })); describe('PUT /api/settings — partial body must not reset service toggles', () => { let harness: RouteTestHarness; @@ -87,6 +93,7 @@ describe('PUT /api/settings — partial body must not reset service toggles', () w.stop.mockClear(); w.__resetRunning(); // running state, not just call records — see makeWatcher } + agentInboxPersistence.setEnabled.mockClear(); }); afterEach(async () => { @@ -167,4 +174,25 @@ describe('PUT /api/settings — partial body must not reset service toggles', () expect((await put({ promptCacheKeepWarm: true })).statusCode).toBe(200); expect(harness.ctx.keepWarm.rescheduleAll).toHaveBeenCalledTimes(2); }); + + it('keeps the agent inbox snapshot on when the body omits its key', async () => { + const res = await harness.app.inject({ + method: 'PUT', + url: '/api/settings', + payload: { showPlanUsageLimits: true }, + }); + expect(res.statusCode).toBe(200); + expect(agentInboxPersistence.setEnabled).toHaveBeenCalledWith(true); + expect(agentInboxPersistence.setEnabled).not.toHaveBeenCalledWith(false); + }); + + it('switches the agent inbox snapshot off when the body says so', async () => { + const res = await harness.app.inject({ + method: 'PUT', + url: '/api/settings', + payload: { agentInboxPersistEnabled: false }, + }); + expect(res.statusCode).toBe(200); + expect(agentInboxPersistence.setEnabled).toHaveBeenCalledWith(false); + }); });