fix: harden runtime lifecycle, storage boundaries, and release safety - #11
Merged
Conversation
Both providers armed one AbortController with two independent aborts — the 120s request timeout and the caller's signal — and both called the zero-arg abort(), so signal.reason was an identical anonymous AbortError either way. mapError had nothing to discriminate on and mapped both to 'aborted'. 'aborted' is the one LLMError that Agent.runInference bails on silently, by design: emitting inference_failed there would leave the agent 'errored' with unconsumed plugin tokens and loop resume_from_error <-> infer forever. That reasoning is correct for a real cancel. A stalled provider reaching the same branch is not: inference_started has already set status 'inferring', decide() has no branch for 'inferring' and falls through to 'idle', and nothing evicts an idle session — so the agent stays wedged for the process lifetime and a new user message will not unstick it. The event log keeps a dangling inference_started with no terminal event. A timedOut flag set in the setTimeout callback now separates the two. 'timeout' is already in isRetryableLLMError, so a stall retries instead of bailing; the 'aborted' path is untouched. config.timeout has no callers anywhere in the repo, so the 120s default is always live, and requests are non-streaming with max_tokens defaulting to 100_000 — responses over 120s are ordinary, not exotic. Also lifts the byte-identical mapError out of both providers into mapProviderError() in provider.ts, which is where the flag has to be read. Adds the timeout tests neither provider suite had. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
Nothing imports any of these — verified against the AST import graph with the
sdk's `~/*` alias resolved — and none is re-exported from an index.ts or named
in an exports map, so no consumer can reach them either:
sdk/src/lib/never.ts assertNever, called nowhere; the repo
throws inline instead
sdk/src/core/agents/communicator.ts createCommunicatorDefinition
sdk/src/plugins/filesystem/schema.ts DirectoryEntry
Dependencies with zero import sites:
sdk ws, @types/ws, @hono/zod-validator
standalone-server @roj-ai/client, @roj-ai/shared
client-react @roj-ai/sdk (devDep)
ws and @hono/zod-validator sat in sdk's *runtime* deps, so every consumer of
the flagship package installed both plus their trees. @types/ws is the fossil
showing ws was once real — Bun's and the browser's native WebSocket replaced
it. standalone-server references @roj-ai/client only inside comments; that one
comes back as a real import if the platform contract is ever typed rather than
mirrored by hand.
transport/src/platform/browser.ts also has no in-repo importer and is
deliberately kept — it is a published subpath entry point.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
packages/sdk/CLAUDE.md opened with "**Not linted by Biome** (excluded in root biome.json). Uses its own conventions." The root biome.json includes `**/*.ts` and excludes only node_modules and dist — biome lints 262 files under packages/sdk/src, 62% of the linted surface. The line licensed divergence that had already started to happen. shared/src/lib/ids.ts claimed "These are the canonical definitions — other packages import from here." They do not: @roj-ai/sdk declares its own SessionId/AgentId/ChatMessageId and, per sdk/src/index.ts, is the side the domain vocabulary is migrating to. The brands are structural so the two sets stay assignable, but nothing asserts they agree — the comment now says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
publish.yml triggers on a v* tag and ran lint + ts:build but never `bun run test`. Nothing else gated it either: no `needs:` on ci.yml, the branch is unprotected, rulesets are empty, and no package declares a prepare or prepublishOnly hook. ci.yml does test every push to main, so a tag cut from main has been tested — but nothing enforces that a tag is cut from main, and the one pipeline whose output reaches users was the one with no test step. The demo e2e was `describe.skip`, which also disabled 'server exposes platform REST surface' — a test that needs no API key and is the only coverage anywhere for standalone-server (1667 src lines, 0 test lines), @roj-ai/client (1368/0), and the platform REST shape. That one now runs; CI gains a test it never had. The build turn stays gated, but on LIVE_TESTS=1 like cache-live.test.ts and compaction-live.test.ts rather than on the presence of snapshots. Snapshots are keyed by a hash of the normalized InferenceRequest, so a preset change orphans them and replay hangs to the 120s idle timeout instead of failing fast — which is what the three committed under __snapshots__/app-builder/ now do. They need re-recording with a key before that gate can widen again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
Both local helpers fell out of their polling loop and returned `undefined` when
the target status never arrived. Two tests then asserted only
`events.length >= 1`, which the 'starting' event alone satisfies — so
'start → status_changed events (starting, ready)' and 'service with
autoStart: true → started on session creation' passed whether or not the
service ever became ready. They now assert `toContain('ready')`, and the
helpers throw with the statuses they actually saw.
This immediately exposes a real defect, fixed in the next commit: 'service that
exits immediately → status failed with error' now fails intermittently with
"saw [starting, ready]". Its assertions were already strong, so the silent
helper was not hiding it — the race is simply load-dependent and this machine
reproduces it about one run in three.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
matej21
force-pushed
the
refactor/audit-cleanup
branch
from
August 11, 2026 14:48
b4aad57 to
e70e1f9
Compare
matej21
changed the title
@roj-ai/cli had no `bin` and no shebang, so `npm i -g @roj-ai/cli` installed a package whose only purpose is to be run and gave you no way to run it — while its own --help and skills/roj/references/clients-and-cli.md both advertise `roj-cli`. The other three executables (platform-cli, sandbox-runtime, standalone-server) already do this correctly. @roj-ai/sdk shipped its compiled test suite: its tsconfig lacked the `src/**/*.test.ts` exclude that transport and debug both have. 256 test artifacts in dist, 5.94 MB unpacked -> 4.06 MB. The ./testing sub-export is unaffected — test-harness.ts is not a *.test.ts file — and every exports target still resolves. @roj-ai/sdk/package.json is resolved by three shipped code paths (sandbox-runtime/src/main.ts, platform-cli/src/build.ts) but was not in the exports map, which is ERR_PACKAGE_PATH_NOT_EXPORTED under Node. Both call sites are Bun-only in practice, and Bun resolves it — the one-line escape hatch costs nothing and removes the trap. client-react and debug pinned react and react-dom peers to the exact patch 19.2.4, so a consumer on any other React 19 patch got ERESOLVE for no technical reason, while lucide-react was "*" and would accept a future major that renames every icon export. Widened to ^19.0.0 and bounded to the 0.x line resolved in bun.lock (0.577.0). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
… and tests run.sh published in `packages/*` glob order, which puts cli and client ahead of the shared/sdk/transport they depend on. Versions move in lockstep, so during that window a consumer installing the new @roj-ai/client gets ETARGET for a @roj-ai/shared not yet on the registry, and a mid-loop failure leaves the release permanently half-shipped. 35 releases have already gone through it, which is also proof npm never validated it. Now driven by the same dependency order as scripts/ts-build.mjs. The four SIGINT/SIGTERM handlers used `void shutdown().then(() => process.exit(0))` with no catch, so a shutdown that rejects never reaches process.exit and the process hangs until something SIGKILLs it — which is the crash shape that orphans service processes and can tear an events.jsonl append. Now .catch().finally(), so the exit path cannot fail. ShellExecutor wrote to child.stdin with no 'error' listener. An unhandled 'error' on a Node writable is an uncaught exception, so a command that ignores stdin or exits first (EPIPE) could take down the agent process instead of failing the tool call through the Result channel execute() is built around. The transport send buffer dropped everything past 500 messages with no log, counter or error, and dropped the *newest* — keeping 500 stale notifications and discarding the fresh ones. It now drops the oldest and logs once per overflow episode plus a total on drain. The cap itself was right; the silence was not, and it left "the UI stopped updating" with no server-side trace. TestHarness never removed the /tmp scratch dir it exclusively owns — 204 call sites, one leak each, 2575 directories on this machine. Since @roj-ai/sdk/testing is published, every downstream user inherited it. shutdown() now removes it, and rpc.integration.test.ts (the one suite that constructed a harness without ever shutting it down) gained an afterEach. Leak per full run: 204 -> 5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
files.ts carried its own MIME_TYPES (33 entries), BINARY_EXTENSIONS (60), getMimeType and preventTraversal — all four already exported by plugins/filesystem/listing.ts, whose header says it was "Extracted from HTTP routes for reuse". The extraction happened; the original was never deleted. The tables were token-for-token identical, so this needs no parameters, just an import. It matters most for preventTraversal: that is the path-traversal guard on GET /:sessionId/files/* and /:sessionId/workspace/*, and hardening one copy left both public routes on the other. files.ts: 271 -> 131 lines. Result<T,E> existed three times with the same md5 (sdk, transport, shared). sdk now re-exports transport's — it already depends on it — so the ~58 in-package importers of `~/lib/utils/result.js` are untouched while the definition has one home. shared keeps its own copy on purpose: it declares no runtime dependency beyond zod and the whole client tier depends on it. That is now written down rather than implied. Four sleep() helpers, of which only core/agents/retry.ts's was cancellable — and the two that most needed cancelling (the retry loop in uploads/plugin.ts and the poll in pdf-preprocessor.ts) used the copies that were not. All four now use lib/utils/sleep.ts, which takes an optional signal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
… owner
ConsoleLogger, JsonLogger and FileLogger each carried their own
debug/info/warn/error — the four level forwarders plus the Error->context
flattening, verbatim in all three (one distinct body across three copies,
checked). They now extend an abstract BaseLogger and supply only log() and
child(); child() stays per-class because each returns its own type with its own
config. TeeLogger deliberately does not extend it — it fans out to other
loggers rather than writing to a sink, so it shares nothing but the interface.
The flattening is the part worth centralising: it is the only path by which a
stack trace reaches a structured log, so a sink that misses it drops the field
and nobody notices until they need the trace.
The HTTP routes hand-wrote `{ error: { type, message } }` inline — four copies
of the session 404 and four of the parse 400 across upload.ts and resources.ts —
so nothing enforced the envelope and a client parsing errors had no single
contract to code against. Both now come from transport/http/responses.ts, which
is also where the status codes and `type` strings are documented as wire
contract.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
…odule 41 lines, byte-identical in DashboardPage.tsx and AgentDetailPage.tsx. Both copies swallow the resume error with a bare comment, so a fix to that has to be applied twice — and the Dashboard copy is easy to miss, sitting between unrelated chart helpers. The rest of the overlap jscpd reports between these two pages, and between MailboxPage and UserChatPage, is deliberately left alone: the summary strips and table bodies differ per page (mailbox counts consumed/pending, chat counts user/agent/questions/answered) and a shared version would need five or more render callbacks. That part is model, not mechanism. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
postFile, PostFileArgs, PostFileResult and sha256Hex were hand-copied from
client/src/platform/rest-client.ts into platform-cli/src/resource.ts —
sha256Hex byte-identical, PostFileResult byte-identical, and the request
assembly differing only in whether `body` arrives as a Blob or as
{ buf, filename, mimeType }. That is one injected parameter, so platform-cli's
caller now builds the Blob and both sides share the definition.
This is a contract against a server that does not live in this repo: the
response shape is a hand-written interface on both ends, so a change to
/api/v1/files/upload would have been fixed on one side and left broken on the
other, surfacing as a runtime upload error rather than a type error.
platform-cli gains @roj-ai/client, which is cheap — it already pulls the entire
server SDK transitively through @roj-ai/sandbox-runtime, while @roj-ai/client
brings only @roj-ai/shared. client already precedes platform-cli in both
ts-build.mjs's ORDER and run.sh's PUBLISH_ORDER.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
Both packages import types from @roj-ai/sdk across 20 emitted .d.ts files but declared it only in devDependencies, and npm never installs a dependency's devDependencies. prepare-packages.mjs rewrites workspace:* in devDependencies too, so the published manifest looked fine — @roj-ai/shared@0.1.28 on npm carries exactly this shape. The failure is quiet rather than loud: every import is `import type`, so no emitted .js touches sdk, and skipLibCheck (on by default, and set in this repo and in roj-platform) suppresses the TS2307 entirely — the unresolvable types silently become `any`. Anyone who turns skipLibCheck off gets 12-15 errors from node_modules. Moving it to dependencies is the honest one-line statement of what the packages need. It does not fix the direction of the arrow — the client tier reaching up into the server SDK for its vocabulary is the architectural finding, and that is a separate decision about which package owns the branded IDs and event payloads. Also finishes packages/sdk/CLAUDE.md: the commands block listed four scripts the package does not define, the tree named main.ts and server.ts (neither exists) while omitting lib/, platform/, bun-platform/, file-store/ and image/, and the plugin count said 15+ where there are 22. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
Both were value-level, not type-shape artifacts, so the ESM module graph really did contain them. Four came from `getServices` (app.ts:49) — a runtime function that files.ts, resources.ts, rpc.ts and upload.ts all import, from the module that imports each of them back in order to mount them. `AppContext`/`AppEnv`/`AppServices` are type-only and were never the problem. All four now live in a new transport/http/context.ts that imports nothing from app.ts; app.ts re-exports them so `from './app.js'` keeps working for consumers. The fifth was workers/plugin.ts <-> workers/context.ts, and it was the worse one: `workerEvents` out of plugin.ts, `WorkerContextImpl` back out of context.ts, both value imports, with `workerEvents` produced by a top-level createEventsFactory call — so it depended on module initialisation order. Moving the events and the EmitEvent type into workers/state.ts also puts the plugin back on the convention mailbox, resources and uploads already follow. Verified with a value-import-only cycle detector over all 419 source files (type-only edges excluded, `~/*` resolved): 5 -> 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
… contract
platform-api.ts implemented 13 platform methods as
`Record<string, Handler>` with `input: any`, and referenced @roj-ai/client only
inside comments — the guarantee that "the platform client works unchanged
against the standalone server" rested entirely on prose.
The handler map is now `Partial<{ [M in PlatformMethodName]: Handler<M> }>` with
input and output drawn from the contract's own `MethodInput`/`MethodOutput`.
Partial is deliberate: bundles.*, sessions.publish, sessions.usage,
instances.archive and services.getUrl are unimplemented by design and still fall
through to `method_not_found` — now as an explicit gap rather than a silent one.
Wiring it up immediately produced three compile errors, i.e. three divergences
that had already shipped:
sessions.create returned { sessionId }, contract declares status too
sessions.list returned the raw manager payload, contract declares
{ id, presetId, status, createdAt } with createdAt as ISO
tokens.create returned { token: '' }, contract declares expiresAt too
Verified the link works by renaming a field on CreateSessionOutput: the
standalone build fails, which is the whole point.
Two supporting changes:
sessions.list's manager method declared `output: sessions: z.array(z.unknown())`
while the handler returns SessionMetadata[]. That lie is why the drift was
invisible, so the schema now says what it produces. `callManagerMethod` is still
typed `Result<unknown>` — the manager registry is untyped, unlike the plugin
method registry — so platform-api validates the payload with the now-exported
sessionMetadataSchema instead of asserting it. That also removes a pre-existing
`as` cast.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
Both providers abort one controller from two sources, so the first abort cause is now recorded and never overwritten by a later one. The abort is also honoured after the fetch resolves: a caller that cancels while the response body is still being read gets `aborted`, not the HTTP error or the parsed reply. The shared part lives in provider-request.ts — Anthropic and OpenRouter had identical copies.
… before it `withRetry` preferred `lastError` over `abortError` when it found the signal aborted. That was harmless while no provider ever produced a retryable error from an abort — but 2f007ed made a request timeout its own `LLMError`, so the loop now keeps a `timeout` in `lastError` and returns it when the caller cancels during the backoff. `Agent.runInference` bails silently only on 'aborted' (agent.ts:722). On anything else it emits `inference_failed` and lets onError notify the parent. So cancelling an agent whose request had just timed out — a shutdown, a user interrupt — produced an error event and a message to the parent for what was a clean cancel. An aborted signal says what happened regardless of how the attempt before it failed, so `abortError` now wins. The message stops claiming "before first attempt", which was only ever true for one of the two paths that reach it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
`child.on('close')` and the stdout/stderr listeners were attached ~190 lines
after the spawn, with two awaits in between: the /proc start-time read and the
pid-registry write. Neither Node nor Bun replays a 'close' or buffered stdio to
a listener attached after the child exited — confirmed on Bun 1.3.14 and
Node 24.
A service that died inside that window was left at whatever the readiness path
had set. With no readyPattern that is 'ready', reached via the immediate
markReady() at the end of startInternal. The result: no `failed` event, no
restart-policy evaluation, an entry that never leaves the map, and a preview URL
handed to the control plane for a process that is already gone. The faster the
failure, the more reliably it hit — a bad command or a missing binary is exactly
the case that exits in under a millisecond.
Re-checking `child.exitCode` after markReady() is not enough on its own. The
buffered output is gone with the 'close', so the `failed` notification cannot
say why and the fresh-port retry cannot fire for the fast EADDRINUSE crash it
exists for. And `exitCode` loses the race against a real spawn: still null often
enough that a service crashing instantly went on announcing `ready` first — 8
false `ready` transitions in 3s, measured against the real executor.
So the listeners attach immediately after spawn, ahead of the /proc read and the
pid-registry write, into collectors that the real handlers take over and replay.
The close handler is named and idempotent, which makes the replay safe against
the ordinary path firing too. `exitCode` no longer decides anything: a recorded
close is replayed after its output, and a child that is reaped but not yet
closed is left alone — 'close' waits for the stdio EOF, so firing early would
drop the tail of the very log the failure has to explain.
Found by the stricter wait helpers in the previous commit: 'service that exits
immediately -> status failed with error' failed about one run in three on a
loaded machine with "saw [starting, ready]".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
1fc690c excluded `*.test.ts` from sdk's tsconfig, which cleared the compiled copies out of `dist`. `files` also ships `src` — declarationMap and sourceMap point into it — so the sources kept going out untouched: 63 test files in every `@roj-ai/sdk` tarball and 5 in `@roj-ai/transport`. Measured on a prepared tarball, sdk is 860 KB → 726 KB and 287 → 224 src files. The new artifact check could not catch it: `findCompiledTests` walked only `dist`, and its pattern matched only compiled output, so `src/**/*.test.ts` was outside it twice over. It now walks the whole installed package and matches `.ts`/`.tsx` as well — dropping either `files` exclusion fails the release with the offending file list, which is how this should have surfaced the first time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189HdSxxQdTh6umQx7znSiM
Preprocessing now stops at every await once the signal fires — the image classifier, the vips resizer, the ZIP walk and the bounded-concurrency helper all check it instead of running to completion on a cancelled upload.
Overlapping signals, a shutdown that starts before startup finished, and a rejected close all had to stop racing each other. The lifecycle helper is shared from `@roj-ai/sdk/bun-platform`; sandbox-runtime and standalone-server both use it instead of keeping a copy each.
Migrate legacy metadata-only deletions before agent replay. Emit the legacy consumption tombstone atomically with new deletion requests so older runtimes cannot resurrect deleted upload content.
matej21
force-pushed
the
refactor/audit-cleanup
branch
from
August 18, 2026 13:19
532cd1a to
b0d4e06
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements the actionable findings from a repository-wide code-smell audit and the follow-up adversarial review.
It fixes runtime races that could wedge agents or lose cleanup, hardens upload and resource boundaries, makes standalone startup and shutdown deterministic, and rebuilds the npm release path around the artifacts that are actually published. It also removes dead code, duplicated definitions, import cycles, and invalid package metadata found during the audit.
What changed
Runtime and session lifecycle
LLM requests and retry behavior
Services and shell execution
Upload lifecycle and preprocessing
Resource and file boundaries
Standalone server
Release and package safety
origin/main.Audit cleanup
Verification
bun run lint— pass.bun run ts:build— pass.The 40 skipped tests require live services, API credentials, or recorded external responses.
Intentional limitations
fs.cpand arbitrarypostInjecthooks is not transactional. Inspection and extraction failures leave the target unchanged, but a promotion or hook failure can leave partial mutations.Out of scope
roj-platform.