Skip to content

feat(coding-agent): add codex app-server mode#104

Open
code-yeongyu wants to merge 58 commits into
mainfrom
feat/codex-app-server-mode
Open

feat(coding-agent): add codex app-server mode#104
code-yeongyu wants to merge 58 commits into
mainfrom
feat/codex-app-server-mode

Conversation

@code-yeongyu

@code-yeongyu code-yeongyu commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Summary

Adds a real senpi app-server mode that serves pi's own agent sessions over the Codex app-server protocol, replacing the dead builtin extension path with a source-owned CLI/server mode. The branch implements stdio, WebSocket, and WS-over-UDS transports; multi-thread session routing; turn steering/interrupts; approval bridging; daemon lifecycle commands; docs; and conformance QA including the real codex-query.ts client workflow.

The old false-positive pi-codex-app-server extension is removed. The new path is validated with mock/faux providers only, uses QA ports in the allowed 18990-18999 range, and does not depend on the codex binary at runtime.

Changes

  • Added app-server protocol facade/generated types, JSON-RPC framing, method registry, app-server mode wiring, and CLI entrypoints.
  • Added thread registry/lifecycle, turn log/control, notification routing, event projection, approval request handling, and model/server-info methods.
  • Added stdio, TCP WebSocket, WS-over-UDS, daemon supervisor/probe/process handling, and QA conformance scripts.
  • Fixed required adjacent blockers: cwd-keyed extension-loader cache, stdout ownership for app-server mode, QA port allocation, loaded-list contract alignment, and oversized test/support files.
  • Removed the dead builtin pi-codex-app-server extension and stale flag/tests.
  • Added docs for app-server usage, daemon/launchd setup, SDK notes, README entries, and changelog coverage.

QA / Evidence

  • Final F1 plan compliance: .omo/evidence/final-f1-plan-compliance-rerun-2.md PASS.
  • Final F2 code quality: .omo/evidence/final-f2-code-quality-rerun.md PASS; included root npm run check, app-server vitest, full coding-agent vitest, static scans, and TUI no-diff checks.
  • Final F3 real manual QA: .omo/evidence/final-f3-real-manual-qa.md PASS; copied evidence to local-ignore/qa-evidence/20260702-app-server-mode/.
  • Final F4 scope fidelity: .omo/evidence/final-f4-scope-fidelity-rerun-2.md PASS.
  • Review-work gate: goal/constraint, hands-on QA, code quality, security, and context mining all passed after targeted fixes; reports under .omo/evidence/global-review/.
  • Runtime debugging audit: .omo/evidence/debugging-audit/final-runtime-debugging-audit.md PASS; ruled out loaded-list shape regression, disallowed QA port allocation, silent leftover listener/processes, and stale branch execution.
  • Latest live surface checks after review fixes:
    • cd packages/coding-agent && npm run qa:app-server PASS with handshake, multiclient, approval, and real-client probes.
    • cd packages/coding-agent && npx tsx ../../node_modules/vitest/dist/cli.js --run test/suite/app-server-protocol.test.ts PASS.
    • root npm run check PASS.

Risks

  • The new app-server surface is broad; coverage is mitigated by per-todo TDD, final full app-server/full coding-agent vitest, live conformance QA, and the real-client workflow.
  • Several legacy files remain oversized with explicit SIZE_OK rationale after app-server-specific extraction; review-work approved this as a watch item, not a blocker.
  • Security review found only low hardening notes around token-file verification and stricter bind-host validation; no critical/high blockers.
  • No real provider calls were used in tests or QA by design; mock/faux provider paths cover protocol and agent-loop behavior without paid tokens.

Secret safety

No raw tokens, auth headers, cookies, API keys, launchd environments, or private credential dumps are included in this PR body. Evidence paths refer to local gitignored artifacts and sanitized transcripts; QA used scratch agent dirs and fake/mock providers.


Summary by cubic

Adds a real senpi app-server mode that serves Pi agent sessions over the Codex app-server protocol via stdio, WebSocket, and WS-over-UDS, plus a managed daemon for background listeners. Vendors a hermetic protocol golden pinned to codex-cli 0.142.5 and removes the old pi-codex-app-server extension.

  • New Features

    • App-server listeners: stdio://, ws://host:port, and unix:// (WS-over-UDS) with JSON-RPC and NDJSON framing, plus WS bearer auth (token file is auto-managed).
    • Multi-thread session routing with thread lifecycle and turn control (start, steer, interrupt) and an approval bridge to the permission system.
    • Daemon commands: senpi app-server daemon <start|stop|status|restart> with probing, token, and log management; conformance QA scripts and docs included; protocol types vendored and pinned to 0.142.5 with a regenerate script, no runtime codex dependency.
  • Migration

    • Remove any flags for the old pi-codex-app-server extension; use senpi app-server --listen <url> instead.
    • Update integrations to connect over WS/UDS or embed via stdio; send Authorization: Bearer <token> using the token file printed on startup.
    • Only when regenerating protocol types, run scripts/generate-app-server-protocol.sh (requires the codex CLI; pinned to 0.142.5); normal installs do not.

Written for commit 666b6d0. Summary will update on new commits.

Review in cubic

Plan: .omo/plans/codex-app-server-mode.md
Plan: .omo/plans/codex-app-server-mode.md

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

40 issues found across 852 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/coding-agent/src/modes/app-server/index.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/index.ts:116">
P2: Shutdown can leave sockets/stdin open when turn interruption fails or times out because transport close only runs after the interrupt await succeeds. A try/finally around the interrupt step would keep transport teardown deterministic.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/threads/handlers.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/threads/handlers.ts:134">
P2: A `thread/read` request can keep a resumed session loaded with no subscribers, so idle cleanup never runs for read-only access. This happens because `read()` calls `attachThread()` but never schedules idle unload afterward.</violation>

<violation number="2" location="packages/coding-agent/src/modes/app-server/threads/handlers.ts:173">
P2: Unexpected client disconnects can bypass idle unload and leave thread sessions loaded longer than intended. The unsubscribe-triggered timer is not mirrored in the connection-removal path, so subscriber removal on disconnect does not start idle eviction.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/transports/unix-socket.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/transports/unix-socket.ts:45">
P2: Custom unix socket paths can be exposed to other local users when `--ws-auth` is omitted, because owner-only directory permissions are not enforced for non-default paths while auth defaults to off. Applying the same owner-only directory enforcement for any unauthenticated unix listener would reduce accidental local exposure.</violation>

<violation number="2" location="packages/coding-agent/src/modes/app-server/transports/unix-socket.ts:110">
P1: Startup can delete an unrelated file when the configured unix path already exists but is not a socket. The stale-socket cleanup path unlinks whatever exists at `socketPath` after a failed probe, so adding a file-type check before unlink would prevent data loss.</violation>
</file>

<file name="packages/coding-agent/src/core/bash-executor.ts">

<violation number="1" location="packages/coding-agent/src/core/bash-executor.ts:166">
P2: Execution failures can be misreported because cleanup is awaited inside the error path, so a temp-stream close failure can replace the real bash error. Preserving the original `err` while best-effort closing avoids masking the root cause during retries/debugging.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/rpc/registry.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/rpc/registry.ts:88">
P2: RPC callers can receive raw internal exception messages, which may leak implementation details or sensitive context from thrown errors. Returning a generic internal error message here (and logging details server-side) would avoid exposing internals to untrusted clients.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/threads/archive-state.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/threads/archive-state.ts:78">
P2: A single corrupted `.archived` sidecar can break `thread/list` for archived threads because `listArchivedThreads` aborts on the first parse/read error. Consider treating invalid archive sidecars as non-fatal (skip and continue) so one bad file does not take down listing for all threads.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/threads/turn-log.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/threads/turn-log.ts:73">
P2: Turn-log snapshots can still be mutated through nested `WireItem` fields because cloning is shallow. Using a deep clone here keeps logged turn history stable when callers reuse or mutate nested payload objects.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/rpc/envelope.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/rpc/envelope.ts:111">
P2: Malformed responses containing both `result` and `error` are currently treated as successful responses, so explicit error payloads can be silently ignored. Consider requiring these fields to be mutually exclusive during classification and marking mixed responses as protocol-invalid.</violation>
</file>

<file name="packages/coding-agent/scripts/qa-app-server/real-client.mjs">

<violation number="1" location="packages/coding-agent/scripts/qa-app-server/real-client.mjs:17">
P1: The real-client QA probe is machine-bound right now, so it fails before exercising app-server behavior on any environment without that exact username path. Using an env-configurable path (with optional HOME-based fallback) keeps this script runnable in CI and by other contributors.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/threads/projection.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/threads/projection.ts:28">
P1: Assistant/tool event projection appears non-functional because `EventProjector` is added but never wired into the thread turn runtime. That leaves app-server turns without projected assistant/tool item notifications and logged items from this projector path; wiring projector creation into session event handling would address it.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/transports/websocket-connection-handler.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/transports/websocket-connection-handler.ts:53">
P1: A client/socket error can crash the app-server because accepted websocket instances do not have an `error` listener. Adding an `error` handler (and terminating/cleanup) keeps transport faults isolated to that connection.</violation>

<violation number="2" location="packages/coding-agent/src/modes/app-server/transports/websocket-connection-handler.ts:60">
P2: A websocket send failure can turn into an unhandled promise rejection because the receive path is fire-and-forget. Attaching a local `.catch(...)` here keeps transport failures from bubbling as process-level unhandled rejections.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/transports/websocket-auth.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/transports/websocket-auth.ts:39">
P1: Websocket auth can fail open to a known empty credential when the token file is blank, so a client can authenticate with `Authorization: Bearer `. `readTokenFile()` currently returns empty strings; rejecting empty token files keeps this path fail-closed.</violation>
</file>

<file name="packages/coding-agent/test/helpers/rpc-hermetic.ts">

<violation number="1" location="packages/coding-agent/test/helpers/rpc-hermetic.ts:133">
P2: `close()` skips remaining cleanup on partial failure. If `fakeModelServer.close()` rejects (e.g., server already closed or socket error), `rmSync(sessionDir)` never runs, leaking temp directories. Wrap the body in try-finally or use `Promise.allSettled` to guarantee both resources are released.</violation>
</file>

<file name="packages/coding-agent/scripts/generate-app-server-protocol.sh">

<violation number="1" location="packages/coding-agent/scripts/generate-app-server-protocol.sh:9">
P2: Script fails when invoked as documented. The docs show running `packages/coding-agent/scripts/generate-app-server-protocol.sh` from the repo root, but the script uses the relative path `src/modes/app-server/protocol` (line 11) which resolves against the caller's CWD. From the repo root, that resolves to `<repo>/src/…` instead of `<repo>/packages/coding-agent/src/…`. Add a `cd "$(dirname "$0")/.."` guard so the script always runs from its package root regardless of how it's called.</violation>
</file>

<file name="packages/coding-agent/src/main.ts">

<violation number="1" location="packages/coding-agent/src/main.ts:117">
P2: The `resolveAppMode` fallback for `"app-server"` triggers false positives and routes to the wrong execution path. `handleAppServerCommand` already intercepts `app-server` subcommands before `parseArgs` (checking `args[0]`). When a positional argument happens to be exactly `"app-server"` — e.g., `senpi --print app-server` or `senpi --verbose app-server` — the guard matches, returning `"app-server"` mode. But `main` has no dispatch for that mode, so it falls through to `runPrintMode` instead. Consider removing the `resolveAppMode` check and instead making `handleAppServerCommand` scan all args for the subcommand, or route the app-server case properly in the mode dispatch.</violation>
</file>

<file name="packages/coding-agent/test/suite/app-server-mode-socket.ts">

<violation number="1" location="packages/coding-agent/test/suite/app-server-mode-socket.ts:125">
P2: Skipped messages consumed during the loop are lost when `read()` rejects before finding the target message (e.g., on WebSocket close or timeout mid-loop). This silently drops buffered protocol frames, which can cause subsequent `read()` calls to hang or return wrong data, making test failures harder to diagnose. Wrap the loop body in try/finally to always restore `skipped` messages.</violation>

<violation number="2" location="packages/coding-agent/test/suite/app-server-mode-socket.ts:139">
P2: Same message-loss bug as `readUntilResponse`. Wrap the loop in try/finally to restore `skipped` messages on any error path.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/threads/projection-message-items.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/threads/projection-message-items.ts:29">
P1: Later assistant messages can emit deltas onto a previous message item when a new message starts at the same content index without a fresh `text_start`. This comes from reusing completed map entries instead of creating a new item for the new message context.</violation>

<violation number="2" location="packages/coding-agent/src/modes/app-server/threads/projection-message-items.ts:55">
P1: Reasoning deltas can be attached to an old completed reasoning item when a new assistant message reuses the same content index. Recreating the item when the cached entry is completed keeps item IDs and turn-log entries aligned with the current message.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/server/approval-types.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/server/approval-types.ts:4">
P1: Command-approval responses that include policy-amendment decisions can be interpreted as cancellation instead of approval. This happens because `ApprovalDecision` excludes protocol-supported decision variants, so the response parser cannot represent them and drops into the cancel path; consider expanding the decision type/model to include all protocol variants and handling them explicitly.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/threads/projection-wire-items.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/threads/projection-wire-items.ts:24">
P2: MCP calls can be projected as `dynamicToolCall` when the server name contains `_`, so app-server clients lose MCP-specific fields (`server`, `tool`, MCP result/error shape) for those tools. This comes from an overly strict classifier regex; widening it to detect any non-empty `server__tool` form keeps routing consistent with `splitMcpName`.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/server/models.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/server/models.ts:75">
P1: `remoteControl/status/read` currently returns a payload that does not match the protocol schema, so conforming clients can fail decoding or treat the method as invalid. The handler should return a valid `RemoteControlStatusReadResponse` object (including required metadata fields and a supported status value).</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/daemon/probe.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/daemon/probe.ts:85">
P2: `pollProbe` can exceed its caller-provided timeout because each probe attempt always waits up to 2s. Using remaining time until `deadline` for each `probeListen` call keeps timeout behavior predictable for short deadlines and near-expiry loops.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/threads/wire-thread.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/threads/wire-thread.ts:99">
P2: Rehydrated `userMessage` items always have empty content, so clients reading turns cannot recover the original user input. Preserving `item.content` when it is an array keeps turn history consistent with live events.</violation>

<violation number="2" location="packages/coding-agent/src/modes/app-server/threads/wire-thread.ts:103">
P2: Reasoning items are reconstructed from the wrong field, so saved reasoning text is lost on `thread/read`/`thread/resume`. Mapping from `item.content` (and optional `item.summary`) preserves the generated schema shape.</violation>

<violation number="3" location="packages/coding-agent/src/modes/app-server/threads/wire-thread.ts:108">
P1: Turn history reconstruction drops structured tool/file/MCP items into generic `agentMessage`, so resumed/read threads lose item semantics needed by clients. Consider adding explicit mappings for the emitted wire item types instead of a generic fallback.</violation>
</file>

<file name="packages/coding-agent/test/helpers/rpc-fake-model.ts">

<violation number="1" location="packages/coding-agent/test/helpers/rpc-fake-model.ts:101">
P2: The request handler calls `JSON.parse(raw)` unprotected — a malformed body crashes the server and kills other tests using the same process since there's no error handler. Add a try-catch around the parse and return a 400 with an error body on parse failure so one flaky request doesn't take down the whole suite.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/threads/turn-runtime.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/threads/turn-runtime.ts:120">
P2: Mixed-input turns can fail even when the request matches the app-server schema, because `parseInput` throws on `image`, `localImage`, `skill`, and `mention`. Consider accepting these variants into `content` and only enforcing the existing text requirement separately, so protocol-valid payloads are not rejected early.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/threads/handler-params.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/threads/handler-params.ts:27">
P1: `connectionId()` accepts `RegistryConnection` but reads an `id` property that doesn't exist on that type. The interface only has `initialized` and `capabilities`. This will throw at runtime if the caller passes a bare `RegistryConnection` without an `id`, and TypeScript won't catch it. Either add `id: string` to `RegistryConnection` or widen the parameter type to something that includes `id` (e.g., `RegistryConnection & { id: string }`).</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/daemon.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/daemon.ts:82">
P2: Daemon start can never succeed with `--listen stdio://`, but this path is currently accepted and only fails after a 10s spawn/probe timeout. Validating daemon listen kind before start/restart would fail fast and avoid launching a detached process that cannot be health-checked.</violation>

<violation number="2" location="packages/coding-agent/src/modes/app-server/daemon.ts:163">
P1: A post-spawn file-write failure can leave a live detached daemon without pid/settings tracking, so later daemon commands report inconsistent state and may fail to manage that process. Wrapping state writes in a try/catch and terminating the spawned pid on failure keeps startup atomic.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/server/approval-bridge.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/server/approval-bridge.ts:46">
P2: Approval requests can remain pending without any client ever receiving them when a thread has subscribers that are not initialized yet. This check trusts a subscriber count rather than actual routed recipients, so treating only successfully delivered approvals as pending would avoid stuck waits.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/threads/list-handlers.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/threads/list-handlers.ts:27">
P1: thread/list pagination can skip results because archive filtering happens after page slicing. Filtering should happen before cursor/limit are applied (or the handler must repaginate filtered results) so nextCursor matches returned data.</violation>

<violation number="2" location="packages/coding-agent/src/modes/app-server/threads/list-handlers.ts:38">
P2: thread/loaded/list accepts negative limit values and can return an empty page with an unchanged cursor. Clamping limit to >= 0 avoids cursor stalls and infinite client pagination retries.</violation>
</file>

<file name="packages/coding-agent/src/modes/app-server/protocol/requests.ts">

<violation number="1" location="packages/coding-agent/src/modes/app-server/protocol/requests.ts:46">
P2: `ServerRequest` and `ServerNotification` use a flat `{ method, params?: JsonValue }` shape that loses per-method param typing, unlike `ClientRequest` which properly discriminates each method with a specific param type. The generated protocol already provides correctly typed discriminated unions for both types. Using `params?: JsonValue` forfeits compile-time validation: you can construct `{ method: "currentTime/read", params: {foo: 1} }` without error, when `CurrentTimeReadParams` only expects `{ threadId: string }`. Consider using a discriminated union matching the generated types' shape to catch param mismatches at compile time.</violation>
</file>

Note: This PR contains a large number of files. cubic only reviews up to 200 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.

Re-trigger cubic

Comment thread packages/coding-agent/test/suite/app-server-protocol.test.ts Outdated
}

function buildRemoteControlStatusReadResponse(): InactiveRemoteControlStatusReadResponse {
return { status: "inactive" };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: remoteControl/status/read currently returns a payload that does not match the protocol schema, so conforming clients can fail decoding or treat the method as invalid. The handler should return a valid RemoteControlStatusReadResponse object (including required metadata fields and a supported status value).

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/modes/app-server/server/models.ts, line 75:

<comment>`remoteControl/status/read` currently returns a payload that does not match the protocol schema, so conforming clients can fail decoding or treat the method as invalid. The handler should return a valid `RemoteControlStatusReadResponse` object (including required metadata fields and a supported status value).</comment>

<file context>
@@ -0,0 +1,87 @@
+}
+
+function buildRemoteControlStatusReadResponse(): InactiveRemoteControlStatusReadResponse {
+	return { status: "inactive" };
+}
+
</file context>

if (await probeLiveSocket(socketPath)) {
throw new AppServerUnixSocketListenError(`${socketPath}: address already in use by a live server.`);
}
await unlink(socketPath);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Startup can delete an unrelated file when the configured unix path already exists but is not a socket. The stale-socket cleanup path unlinks whatever exists at socketPath after a failed probe, so adding a file-type check before unlink would prevent data loss.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/modes/app-server/transports/unix-socket.ts, line 110:

<comment>Startup can delete an unrelated file when the configured unix path already exists but is not a socket. The stale-socket cleanup path unlinks whatever exists at `socketPath` after a failed probe, so adding a file-type check before unlink would prevent data loss.</comment>

<file context>
@@ -0,0 +1,154 @@
+	if (await probeLiveSocket(socketPath)) {
+		throw new AppServerUnixSocketListenError(`${socketPath}: address already in use by a live server.`);
+	}
+	await unlink(socketPath);
+}
+
</file context>

Comment thread packages/coding-agent/test/suite/app-server-protocol.test.ts
} from "./lib/env.mjs";
import { fail, httpStatus, pass } from "./lib/rpc.mjs";

const clientPath = "/Users/yeongyu/.agents/skills/use-codex-appserver/scripts/codex-query.ts";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The real-client QA probe is machine-bound right now, so it fails before exercising app-server behavior on any environment without that exact username path. Using an env-configurable path (with optional HOME-based fallback) keeps this script runnable in CI and by other contributors.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/scripts/qa-app-server/real-client.mjs, line 17:

<comment>The real-client QA probe is machine-bound right now, so it fails before exercising app-server behavior on any environment without that exact username path. Using an env-configurable path (with optional HOME-based fallback) keeps this script runnable in CI and by other contributors.</comment>

<file context>
@@ -0,0 +1,157 @@
+} from "./lib/env.mjs";
+import { fail, httpStatus, pass } from "./lib/rpc.mjs";
+
+const clientPath = "/Users/yeongyu/.agents/skills/use-codex-appserver/scripts/codex-query.ts";
+const transcript = [];
+const clientChildren = new Set();
</file context>

export function loadedThreadsResponse(requestParams: unknown, threads: ThreadRegistry): ThreadLoadedListResponse {
const params = objectValue(requestParams);
const cursor = decodeCursor(optionalString(params.cursor) ?? null);
const limit = optionalNumber(params.limit) ?? Number.POSITIVE_INFINITY;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: thread/loaded/list accepts negative limit values and can return an empty page with an unchanged cursor. Clamping limit to >= 0 avoids cursor stalls and infinite client pagination retries.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/modes/app-server/threads/list-handlers.ts, line 38:

<comment>thread/loaded/list accepts negative limit values and can return an empty page with an unchanged cursor. Clamping limit to >= 0 avoids cursor stalls and infinite client pagination retries.</comment>

<file context>
@@ -0,0 +1,74 @@
+export function loadedThreadsResponse(requestParams: unknown, threads: ThreadRegistry): ThreadLoadedListResponse {
+	const params = objectValue(requestParams);
+	const cursor = decodeCursor(optionalString(params.cursor) ?? null);
+	const limit = optionalNumber(params.limit) ?? Number.POSITIVE_INFINITY;
+	const ids = threads.listLoaded().map((thread) => thread.id);
+	const data = ids.slice(cursor, cursor + limit);
</file context>

@@ -0,0 +1,267 @@
import type {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Unexpected client disconnects can bypass idle unload and leave thread sessions loaded longer than intended. The unsubscribe-triggered timer is not mirrored in the connection-removal path, so subscriber removal on disconnect does not start idle eviction.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/modes/app-server/threads/handlers.ts, line 173:

<comment>Unexpected client disconnects can bypass idle unload and leave thread sessions loaded longer than intended. The unsubscribe-triggered timer is not mirrored in the connection-removal path, so subscriber removal on disconnect does not start idle eviction.</comment>

<file context>
@@ -0,0 +1,267 @@
+		return {};
+	}
+
+	private unsubscribe(connection: RegistryConnection, request: RpcRequest): ThreadUnsubscribeResponse {
+		const params = objectValue(request.params);
+		const threadId = requiredString(params.threadId, "threadId");
</file context>

validateSocketPath(socketPath);
await prepareSocketPath(
socketPath,
socketPath === defaultSocketPath && (options.auth === undefined || options.auth.kind === "off"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Custom unix socket paths can be exposed to other local users when --ws-auth is omitted, because owner-only directory permissions are not enforced for non-default paths while auth defaults to off. Applying the same owner-only directory enforcement for any unauthenticated unix listener would reduce accidental local exposure.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/modes/app-server/transports/unix-socket.ts, line 45:

<comment>Custom unix socket paths can be exposed to other local users when `--ws-auth` is omitted, because owner-only directory permissions are not enforced for non-default paths while auth defaults to off. Applying the same owner-only directory enforcement for any unauthenticated unix listener would reduce accidental local exposure.</comment>

<file context>
@@ -0,0 +1,154 @@
+	validateSocketPath(socketPath);
+	await prepareSocketPath(
+		socketPath,
+		socketPath === defaultSocketPath && (options.auth === undefined || options.auth.kind === "off"),
+	);
+
</file context>

const request = buildApprovalRequest(threadId, requestId, kind, payload);
return new Promise((resolve) => {
this.pending.set(requestId, { threadId, request, allowKey, resolve });
if (this.sendToThreadSubscribers(threadId, request) > 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Approval requests can remain pending without any client ever receiving them when a thread has subscribers that are not initialized yet. This check trusts a subscriber count rather than actual routed recipients, so treating only successfully delivered approvals as pending would avoid stuck waits.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/modes/app-server/server/approval-bridge.ts, line 46:

<comment>Approval requests can remain pending without any client ever receiving them when a thread has subscribers that are not initialized yet. This check trusts a subscriber count rather than actual routed recipients, so treating only successfully delivered approvals as pending would avoid stuck waits.</comment>

<file context>
@@ -0,0 +1,153 @@
+		const request = buildApprovalRequest(threadId, requestId, kind, payload);
+		return new Promise((resolve) => {
+			this.pending.set(requestId, { threadId, request, allowKey, resolve });
+			if (this.sendToThreadSubscribers(threadId, request) > 0) {
+				return;
+			}
</file context>

@@ -0,0 +1,267 @@
import type {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: A thread/read request can keep a resumed session loaded with no subscribers, so idle cleanup never runs for read-only access. This happens because read() calls attachThread() but never schedules idle unload afterward.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/modes/app-server/threads/handlers.ts, line 134:

<comment>A `thread/read` request can keep a resumed session loaded with no subscribers, so idle cleanup never runs for read-only access. This happens because `read()` calls `attachThread()` but never schedules idle unload afterward.</comment>

<file context>
@@ -0,0 +1,267 @@
+		return response;
+	}
+
+	private async read(request: RpcRequest): Promise<ThreadReadResponse> {
+		const params = objectValue(request.params);
+		const threadId = requiredString(params.threadId, "threadId");
</file context>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant