feature/mini lilac - #20
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
WalkthroughMini Lilac adds a protocol package, SQLite-backed runtime, HTTP server, terminal UI, command-line bundle, provider/model integrations, web tools, configuration templates, secure OAuth storage, and extensive unit and integration tests. ChangesMini Lilac platform
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MiniLilacTUI
participant MiniLilacTransport
participant MiniLilacServer
participant SessionService
participant MiniLilacSqliteStore
MiniLilacTUI->>MiniLilacTransport: send chat message
MiniLilacTransport->>MiniLilacServer: POST /api/mini-lilac/chat
MiniLilacServer->>SessionService: startPrompt
SessionService->>MiniLilacSqliteStore: persist run and chunks
MiniLilacSqliteStore-->>SessionService: stored stream chunks
SessionService-->>MiniLilacServer: SSE runtime chunks
MiniLilacServer-->>MiniLilacTransport: SSE response
MiniLilacTransport-->>MiniLilacTUI: render transcript and cursor state
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e273a50ce8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/utils/codex-oauth.ts (1)
106-148: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider fsyncing the parent directory after rename for full crash durability.
The temp file is fsynced before rename, but the containing directory isn't fsynced afterward. On some filesystems, a rename's directory-entry update can survive a crash inconsistently without a directory fsync, leaving a narrow window where the atomic-rename guarantee is weaker than intended for this security-sensitive token store.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/utils/codex-oauth.ts` around lines 106 - 148, Update writeSecretFile after rename(temporaryPath, storagePath) to open the containing directory, fsync it, and close the directory handle so the rename is durable across crashes. Integrate directory-handle cleanup into the existing error handling and AggregateError flow without changing the temporary-file cleanup behavior.apps/mini-lilac-server/package.json (1)
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
devDependenciesinstead ofpeerDependenciesfortypescript.This package is
"private": trueand never consumed externally, so declaringtypescriptas a peer dependency is atypical;devDependenciesis the conventional choice for a localtypecheckscript in a non-published package.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mini-lilac-server/package.json` around lines 27 - 29, Move typescript from peerDependencies to devDependencies in the package manifest, preserving the existing version constraint so the local typecheck script continues using it without exposing it as a peer requirement.apps/mini-lilac-tui/src/controller.ts (1)
576-583: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid running both stream-specific
zodparses on every chunk.
miniLilacStreamCursorChunkSchemaandminiLilacSteeringChunkSchemadiscriminate ontype(data-streamCursor/data-steering), so gate the cursor/steering parses behindvalue.typechecks before dispatchingtext-deltachunks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mini-lilac-tui/src/controller.ts` around lines 576 - 583, Update the chunk handling flow around miniLilacStreamCursorChunkSchema and miniLilacSteeringChunkSchema to inspect value.type first, invoking only the matching schema parse for data-streamCursor or data-steering chunks. Preserve admitRun and appendReplayedSteering behavior for their respective types, and dispatch other chunks such as text-delta directly to renderer.handle without either stream-specific parse.apps/mini-lilac-tui/src/code-block-parsers.ts (1)
7-62: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin highlight query URLs to fixed refs. The grammar
wasmfiles are pinned to release tags, but thehighlights.scmquery URLs resolve against movingrefs/heads/masterbranches (nvim-treesitter and tree-sitter-python). Upstream query edits can drift out of sync with the pinned grammar version and silently break highlighting, and every parse becomes a runtime fetch against a mutable ref. Pin each query URL to a tag or commit SHA that matches the grammar version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mini-lilac-tui/src/code-block-parsers.ts` around lines 7 - 62, Pin every highlights.scm URL in the language configuration entries to an immutable tag or commit SHA matching its corresponding pinned wasm grammar version, replacing all refs/heads/master references while preserving each existing query source and language mapping.apps/mini-lilac-tui/src/startup.ts (1)
76-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider warning when resume ignores conflicting CLI bindings.
When resuming (
snapshot !== undefined), any--model/--profile/--reasoningCLI overrides are silently discarded in favor of the stored snapshot bindings (Lines 92-95, 109-112, 120-123). This is intentional (per the "preserves stored bindings" test) but gives no feedback to a user who explicitly passed a flag expecting it to apply.♻️ Proposed fix: surface a warning when resume overrides are ignored
if (options.session !== undefined) { // Resume state and canonical transcript are loaded before catalog selection. ({ snapshot, messages, todos, replayCursor } = await loadExistingSession( transport, options.session, options.cwd, )); + if (options.model !== undefined || options.profile !== undefined || options.reasoning !== undefined) { + io.write( + "Note: resumed session keeps its original model/profile/reasoning; --model/--profile/--reasoning are ignored.\n", + ); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mini-lilac-tui/src/startup.ts` around lines 76 - 123, When resuming an existing session in the startup binding-selection flow, warn the user whenever explicitly supplied options.model, options.profile, or options.reasoning differ from the corresponding stored snapshot values and will be ignored. Emit the warning through the existing I/O mechanism, while preserving the snapshot-authoritative assignments in the model, profile, and reasoning selection logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mini-lilac-server/config.example.yaml`:
- Line 1: Remove the stale 100-character comment from config.example.yaml; no
README or implementation changes are needed.
In `@apps/mini-lilac-server/src/server.ts`:
- Around line 104-141: Update withSseKeepAlive’s ReadableStream pump to track
consumer cancellation, set that state in cancel(), and guard
controller.enqueue(), controller.close(), and controller.error() calls from
pending reads or chunks after cancellation. Preserve reader cancellation and
keepalive cleanup while preventing late writes from producing unhandled
rejections.
In `@apps/mini-lilac-tui/src/cli.ts`:
- Line 13: Update TOKEN_ENV_VARS in the CLI configuration to include only
MINI_LILAC_TOKEN, removing the generic TOKEN fallback. Also revise the related
help text to state that MINI_LILAC_TOKEN is the sole environment variable used
for the client bearer token.
In `@packages/utils/codex-oauth.ts`:
- Around line 106-147: Update the catch block in writeSecretFile so the
single-failure path wraps the original error with a descriptive error that
identifies the token storagePath and write operation, while preserving the
original error as its cause. Keep the existing AggregateError behavior unchanged
when cleanupErrors is non-empty.
---
Nitpick comments:
In `@apps/mini-lilac-server/package.json`:
- Around line 27-29: Move typescript from peerDependencies to devDependencies in
the package manifest, preserving the existing version constraint so the local
typecheck script continues using it without exposing it as a peer requirement.
In `@apps/mini-lilac-tui/src/code-block-parsers.ts`:
- Around line 7-62: Pin every highlights.scm URL in the language configuration
entries to an immutable tag or commit SHA matching its corresponding pinned wasm
grammar version, replacing all refs/heads/master references while preserving
each existing query source and language mapping.
In `@apps/mini-lilac-tui/src/controller.ts`:
- Around line 576-583: Update the chunk handling flow around
miniLilacStreamCursorChunkSchema and miniLilacSteeringChunkSchema to inspect
value.type first, invoking only the matching schema parse for data-streamCursor
or data-steering chunks. Preserve admitRun and appendReplayedSteering behavior
for their respective types, and dispatch other chunks such as text-delta
directly to renderer.handle without either stream-specific parse.
In `@apps/mini-lilac-tui/src/startup.ts`:
- Around line 76-123: When resuming an existing session in the startup
binding-selection flow, warn the user whenever explicitly supplied
options.model, options.profile, or options.reasoning differ from the
corresponding stored snapshot values and will be ignored. Emit the warning
through the existing I/O mechanism, while preserving the snapshot-authoritative
assignments in the model, profile, and reasoning selection logic.
In `@packages/utils/codex-oauth.ts`:
- Around line 106-148: Update writeSecretFile after rename(temporaryPath,
storagePath) to open the containing directory, fsync it, and close the directory
handle so the rename is durable across crashes. Integrate directory-handle
cleanup into the existing error handling and AggregateError flow without
changing the temporary-file cleanup behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b181ad8-2615-4b0a-b904-d763c624f9f8
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (89)
DockerfilePROJECT.md__tests__/workspaces.test.tsapps/mini-lilac-server/.gitignoreapps/mini-lilac-server/README.mdapps/mini-lilac-server/auth.example.jsonapps/mini-lilac-server/config.example.yamlapps/mini-lilac-server/package.jsonapps/mini-lilac-server/providers.example.yamlapps/mini-lilac-server/src/main.tsapps/mini-lilac-server/src/server.tsapps/mini-lilac-server/tests/main.test.tsapps/mini-lilac-server/tests/server.test.tsapps/mini-lilac-server/tsconfig.jsonapps/mini-lilac-tui/.gitignoreapps/mini-lilac-tui/README.mdapps/mini-lilac-tui/bunfig.tomlapps/mini-lilac-tui/package.jsonapps/mini-lilac-tui/src/app.test.tsxapps/mini-lilac-tui/src/app.tsxapps/mini-lilac-tui/src/cli.tsapps/mini-lilac-tui/src/clipboard.tsapps/mini-lilac-tui/src/code-block-parsers.test.tsapps/mini-lilac-tui/src/code-block-parsers.tsapps/mini-lilac-tui/src/continuation.test.tsapps/mini-lilac-tui/src/continuation.tsapps/mini-lilac-tui/src/controller.test.tsapps/mini-lilac-tui/src/controller.tsapps/mini-lilac-tui/src/input-state.test.tsapps/mini-lilac-tui/src/input-state.tsapps/mini-lilac-tui/src/main.tsxapps/mini-lilac-tui/src/opentui-patch.test.tsxapps/mini-lilac-tui/src/palette.test.tsapps/mini-lilac-tui/src/palette.tsapps/mini-lilac-tui/src/preferences.test.tsapps/mini-lilac-tui/src/preferences.tsapps/mini-lilac-tui/src/preflight.test.tsapps/mini-lilac-tui/src/preflight.tsapps/mini-lilac-tui/src/presentation.test.tsapps/mini-lilac-tui/src/presentation.tsapps/mini-lilac-tui/src/render.test.tsapps/mini-lilac-tui/src/render.tsapps/mini-lilac-tui/src/startup.test.tsapps/mini-lilac-tui/src/startup.tsapps/mini-lilac-tui/src/theme.test.tsapps/mini-lilac-tui/src/theme.tsapps/mini-lilac-tui/src/transcript-buffer.test.tsapps/mini-lilac-tui/src/transcript-buffer.tsapps/mini-lilac-tui/tsconfig.jsonapps/mini-lilac/.gitignoreapps/mini-lilac/LICENSEapps/mini-lilac/README.mdapps/mini-lilac/build.tsapps/mini-lilac/package.jsonapps/mini-lilac/src/main.test.tsapps/mini-lilac/src/main.tsapps/mini-lilac/tsconfig.jsonpackage.jsonpackages/agent/auto-compaction.tspackages/agent/tests/auto-compaction.test.tspackages/mini-lilac-client/index.tspackages/mini-lilac-client/mini-lilac-transport.test.tspackages/mini-lilac-client/mini-lilac-transport.tspackages/mini-lilac-client/package.jsonpackages/mini-lilac-client/protocol.test.tspackages/mini-lilac-client/protocol.tspackages/mini-lilac-client/tsconfig.jsonpackages/mini-lilac-runtime/package.jsonpackages/mini-lilac-runtime/src/config.tspackages/mini-lilac-runtime/src/index.tspackages/mini-lilac-runtime/src/model-catalog.tspackages/mini-lilac-runtime/src/providers.tspackages/mini-lilac-runtime/src/session-service.tspackages/mini-lilac-runtime/src/skills.tspackages/mini-lilac-runtime/src/sqlite-store.tspackages/mini-lilac-runtime/src/web-search.tspackages/mini-lilac-runtime/src/webfetch.tspackages/mini-lilac-runtime/tests/config.test.tspackages/mini-lilac-runtime/tests/model-catalog.test.tspackages/mini-lilac-runtime/tests/providers.test.tspackages/mini-lilac-runtime/tests/session-runtime.test.tspackages/mini-lilac-runtime/tests/skills.test.tspackages/mini-lilac-runtime/tests/sqlite-store-todos.test.tspackages/mini-lilac-runtime/tests/web-search.test.tspackages/mini-lilac-runtime/tests/webfetch.test.tspackages/mini-lilac-runtime/tsconfig.jsonpackages/utils/codex-oauth.tspackages/utils/tests/codex-oauth.test.tspatches/@opentui%2Fcore@0.4.3.patch
| @@ -0,0 +1,79 @@ | |||
| configVersion: 1 | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg '(^|/)session-service\.(ts|js)$|config.example.yaml|README.md$' | sed -n '1,120p'
echo
echo "== search title/titleModel/title-generation related text =="
rg -n "titleModel|title|normalized first|50|100|first prompt" apps/mini-lilac-server -S || true
echo
echo "== config example near title model comments =="
if [ -f apps/mini-lilac-server/config.example.yaml ]; then
nl -ba apps/mini-lilac-server/config.example.yaml | sed -n '25,45p'
fi
echo
echo "== README title comments =="
if [ -f apps/mini-lilac-server/README.md ]; then
nl -ba apps/mini-lilac-server/README.md | sed -n '80,100p'
fi
echo
echo "== candidate session-service outline and relevant content =="
file="$(git ls-files | rg '(^|/)session-service\.ts$' | head -n1 || true)"
if [ -n "${file:-}" ]; then
echo "FILE=$file"
wc -l "$file"
ast-grep outline "$file" --view expanded | rg -n "title|session|Title|titleModel" || true
# Search precise lines around title logic
rg -n -C 4 "titleModel|title|normalize|normalized|first prompt|prompt" "$file" || true
fiRepository: stanley2058/lilac-mono
Length of output: 1434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect session-service.ts title generation area =="
FILE="packages/mini-lilac-runtime/src/src-session-service.ts"
if [ -f packages/mini-lilac-runtime/src/session-service.ts ]; then
FILE="packages/mini-lilac-runtime/src/session-service.ts"
fi
echo "FILE=$FILE"
wc -l "$FILE"
sed -n '1,260p' "$FILE"
echo
echo "== deterministic check for title generation constants in tracked files =="
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('.').rglob('*'):
if p.is_file() and '/.git/' not in str(p):
try:
s=p.read_text(errors='ignore')
except Exception:
continue
if 'normalized first' in s and ('titleModel' in s or 'title' in s):
print(p)
for i,line in enumerate(s.splitlines(),1):
if 'normalized first' in line or 'titleModel' in line or 'title' in line:
print(f"{i}: {line}")
PYRepository: stanley2058/lilac-mono
Length of output: 8073
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate generate title logic =="
rg -n -C 8 "normalized first|first 50|first 100|subarray\\(|slice\\(|substring\\(|title" packages/mini-lilac-runtime/src/session-service.ts || true
echo
echo "== focused scan over likely sections =="
wc -l packages/mini-lilac-runtime/src/session-service.ts
sed -n '2100,2450p' packages/mini-lilac-runtime/src/session-service.ts
sed -n '2450,2703p' packages/mini-lilac-runtime/src/session-service.ts
echo
echo "== narrow title-model references in session-service =="
rg -n -C 4 "titleModel|title:" packages/mini-lilac-runtime/src/session-service.ts || trueRepository: stanley2058/lilac-mono
Length of output: 37562
No documentation change needed. The README’s fallback title length matches the implementation; the 100-character text in config.example.yaml is an isolated stale comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/mini-lilac-server/config.example.yaml` at line 1, Remove the stale
100-character comment from config.example.yaml; no README or implementation
changes are needed.
| const body = new ReadableStream<Uint8Array>({ | ||
| start(controller) { | ||
| timer = setInterval(() => { | ||
| if (closed) return; | ||
| try { | ||
| controller.enqueue(encoder.encode(": keepalive\n\n")); | ||
| } catch { | ||
| close(); | ||
| } | ||
| }, intervalMs); | ||
| void (async () => { | ||
| try { | ||
| for (;;) { | ||
| const result = await reader.read(); | ||
| if (result.done) { | ||
| close(); | ||
| controller.close(); | ||
| return; | ||
| } | ||
| controller.enqueue(result.value); | ||
| } | ||
| } catch (error) { | ||
| close(); | ||
| controller.error(error); | ||
| } | ||
| })(); | ||
| }, | ||
| async cancel(reason) { | ||
| close(); | ||
| await reader.cancel(reason).catch(() => undefined); | ||
| }, | ||
| }); | ||
| return new Response(body, { | ||
| status: response.status, | ||
| statusText: response.statusText, | ||
| headers: response.headers, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat > /tmp/close-after-cancel.mjs <<'EOF'
let ctrl;
const rs = new ReadableStream({ start(c) { ctrl = c; } });
const reader = rs.getReader();
await reader.cancel("x");
try { ctrl.close(); console.log("close() did NOT throw"); }
catch (e) { console.log("close() threw:", e?.constructor?.name, e?.message); }
EOF
bun /tmp/close-after-cancel.mjsRepository: stanley2058/lilac-mono
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository file:"
fd -a 'server\.ts$' . | sed 's#^\./##' || true
echo
echo "Relevant server.ts lines if present:"
if [ -f app*/**/server.ts ] || [ -f apps/mini-lilac-server/src/server.ts ]; then
wc -l apps/mini-lilac-server/src/server.ts
sed -n '80,145p' apps/mini-lilac-server/src/server.ts | cat -n
fi
echo
echo "Search withSseKeepAlive/ctrl operations:"
rg -n "withSseKeepAlive|controller\.(enqueue|close|error)|async cancel|ReadableStream" apps/mini-lilac-server/src/server.ts apps 2>/dev/null | head -80Repository: stanley2058/lilac-mono
Length of output: 10691
🌐 Web query:
WHATWG Streams ReadableStreamDefaultController close after consumer canceller pending request done throws NotReadableError
💡 Result:
According to the WHATWG Streams Standard, calling ReadableStreamDefaultController.close does not throw a NotReadableError when there are pending read requests [1]. The close method on a ReadableStreamDefaultController behaves as follows: 1. It checks if the stream can be closed or enqueued using the ReadableStreamDefaultControllerCanCloseOrEnqueue algorithm [1]. If the stream's state is not readable (e.g., if it is already closed, errored, or in a state where it cannot accept further operations), it throws a TypeError exception, not a NotReadableError [1]. 2. When close is successfully called, the stream transitions to a closing state [1]. Any pending read requests from the consumer are not immediately rejected with an error; instead, they remain in the queue to be fulfilled by any previously enqueued chunks [1]. Once all enqueued chunks are consumed, the stream's state transitions to closed, and any remaining pending read requests are resolved with { value: undefined, done: true } [1]. There is no standard behavior defined in the WHATWG Streams specification where ReadableStreamDefaultController.close throws a "NotReadableError" [1]. If you are encountering a NotReadableError in your specific environment, it may be a custom error implementation or a platform-specific deviation from the standard, as standard stream errors typically use TypeError for invalid operations or DOMException for various operational failures [1]. Note that calling close on a stream that is not in a "readable" state will throw a TypeError [1]. [2][1]
Citations:
- 1: https://streams.spec.whatwg.org/
- 2: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultController/close
🌐 Web query:
Bun ReadableStream cancel ReadableStreamDefaultController close enqueue error after reader cancel throws closed stream
💡 Result:
In Bun (and web streams generally), attempting to enqueue() or close() a ReadableStreamDefaultController after the stream has been cancelled or closed will result in an error [1][2][3][4]. Specifically, calling controller.enqueue() on a stream that is already in a non-readable state (closed, cancelled, or errored) will throw a TypeError [1][3]. Recent updates in Bun have improved how the runtime handles these states, particularly for type: "direct" streams [5]. A significant fix involved setting an internal m_closed flag upon cancellation, ensuring that subsequent calls to controller.write() (or enqueue()) throw an error rather than silently buffering or behaving unpredictably [5]. If you are experiencing issues where errors occur during teardown—such as late-arriving chunks racing with a stream close—this is typical behavior for the Streams API [3]. The standard pattern to handle this involves: 1. Checking the stream state: While not always explicitly exposed, logic often relies on preventing enqueues after a close/cancel signal is triggered in your application code [1][4]. 2. Error Handling: Because enqueue() throws if the controller is invalid, you must wrap these calls in try-catch blocks if you are dealing with race conditions [6][3]. 3. Understanding cancel() vs close(): Calling ReadableStream.cancel() is an external request to stop the stream, while controller.close() is an internal request from the producer [2]. If controller.close() has already been called, subsequent enqueue() calls will fail [1][3]. If you find that cancel() is not firing in your specific context (e.g., in a Bun server response), ensure you are on a recent version of Bun, as several issues regarding stream cancellation, especially with AbortSignal and disconnected clients, were resolved in versions 1.1.27 and later [7][8][9].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultController/enqueue
- 2: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultController/close
- 3: ReadableStream controller crash: enqueue after close on LLM stream error (v1.2.x) mastra-ai/mastra#13107
- 4: ReadableStreamDefaultController: undocumented error conditions mdn/content#34597
- 5: streams: invoke underlyingSource.cancel() when a type:"direct" ReadableStream is cancelled oven-sh/bun#34854
- 6:
releasing lock of reader whose stream is still in readable statewhen cancelling a TransformStream oven-sh/bun#13816 - 7: ReadableStream cancel is not called oven-sh/bun#14390
- 8: https://bun.com/blog/bun-v1.1.27
- 9: Call cancel on ReadableStream when Bun.serve() response is aborted oven-sh/bun#13687
🌐 Web query:
MDN ReadableStreamDefaultController close throws TypeError stream is already close or closed
💡 Result:
Calling ReadableStreamDefaultController.close on a stream that is already closed, cancelled, or errored will throw a TypeError [1][2][3]. According to the Streams Standard, the close method is not idempotent [3]. It checks whether the stream is in a readable state and whether a close has already been requested [3]. If the stream is not in a state where it can be closed (i.e., it is already closed, cancelled, or has encountered an error), the method throws a TypeError exception [1][3]. This behavior is explicitly documented in the MDN Web Docs, which state that a TypeError is thrown if close is called when the stream is not readable [1]. This includes scenarios where: - The stream is already closed [1][3]. - The stream has been cancelled [1][3]. - The stream has errored [1][3]. - A close has already been requested by the underlying source but has not yet completed because there are still enqueued chunks remaining to be read [1][3]. If you need to ensure that a stream is closed without risking a TypeError, you should track the state of your stream externally or check the status before attempting to call close, though the controller itself is designed to strictly enforce these state transitions [3][4]. If you want to discard chunks and ensure the stream is closed regardless of its current state, you may alternatively use the cancel method on the ReadableStream or its reader, which is designed to be safe to call multiple times [1][3][5].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultController/close
- 2: https://github.com/mdn/content/blob/main/files/en-us/web/api/readablestreamdefaultcontroller/close/index.md
- 3: https://streams.spec.whatwg.org/
- 4: ReadableStreamDefaultController: undocumented error conditions mdn/content#34597
- 5: Close should be idempotent whatwg/streams#1136
🌐 Web query:
MDN ReadableStreamDefaultController error throws TypeError already closed cancelled
💡 Result:
When you encounter a TypeError related to ReadableStreamDefaultController—specifically messages indicating the controller is "already closed" or "cancelled"—it signifies that you are attempting to perform an operation (like enqueue or close) on a stream that is no longer in a "readable" state [1][2][3][4]. In the Streams API, a ReadableStream has an internal state (readable, closed, or errored) [5]. Methods like enqueue and close are only valid when the stream is currently "readable" [1][2]. If you attempt to use them after the stream has already been closed, cancelled, or transitioned to an errored state, the browser or runtime throws a TypeError [1][2][3][4]. Common causes for this error include: 1. Double-closing or double-enqueuing: Calling controller.close or controller.enqueue multiple times, often in asynchronous code where an initial closing operation has already finished [6]. 2. Race conditions: Attempting to manipulate the stream after a cancellation request has been processed or after an underlying source has already signaled completion [7][6]. 3. Improper AbortSignal handling: If you are using an AbortSignal to trigger stream cancellation, ensure your cleanup logic does not attempt to interact with the controller after the cancellation has taken effect [6]. To resolve or prevent these errors: - Check the state: While you cannot directly check the state (as it is internal) [5], you can wrap your calls in a try-catch block to handle or suppress the error gracefully if the stream might have already been closed by another part of your application [6]. - Use atomic operations: If possible, rely on reader.cancel instead of manually closing the controller from external logic. Reader cancellation atomicaly terminates the stream and propagates the signal to the underlying source, which is often cleaner and avoids conflicting state transitions [6]. - Centralize control: Ensure that your stream lifecycle management (closing, canceling, and erroring) is centralized so that multiple parts of your code do not attempt to finalize the stream independently [6]. Note that unlike close or enqueue, the error method is designed to be idempotent and safe to call even if the stream is not currently readable; it will simply do nothing if the stream is already in a non-readable state [8][9].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultController/enqueue
- 2: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultController/close
- 3: https://github.com/mdn/content/blob/main/files/en-us/web/api/readablestreamdefaultcontroller/close/index.md
- 4: https://github.com/mdn/content/blob/main/files/en-us/web/api/readablestreamdefaultcontroller/enqueue/index.md
- 5: https://streams.spec.whatwg.org/
- 6:
ReadableStreamcancel callback never invoked on client disconnect elysiajs/elysia#1768 - 7: WebSocketStream: cancel readable then close triggers uncaughtException ERR_INVALID_STATE (Controller is already closed) nodejs/undici#5103
- 8: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultController/error
- 9: Non readable stream should throw when errored while not readable nodejs/node#39756
🌐 Web query:
web streams ReadableStreamDefaultController error after cancel controller non readable
💡 Result:
When you call the error method on a ReadableStreamDefaultController, it does not throw an error if the stream is already in a non-readable state (such as cancelled, closed, or already errored) [1][2]. According to the official Streams specification, the error(e) method is designed to be idempotent regarding the stream's state [1]. When error is invoked: 1. It first checks the current state of the associated stream [1]. 2. If the stream is not "readable" (i.e., it is already closed, cancelled, or errored), the method simply returns without performing any action [1]. 3. It does not throw an exception, which allows developers to call it safely without needing to manually track the stream's state beforehand [1]. This behavior differs from other controller methods like enqueue or close, which typically throw a TypeError if called on a stream that is no longer readable [3][4][5]. Historically, some documentation (including older MDN pages) incorrectly suggested that error might throw if the stream was not readable, but this has been clarified in both the specification and updated documentation to reflect that it effectively performs a no-op in those scenarios [1].
Citations:
- 1: Non readable stream should throw when errored while not readable nodejs/node#39756
- 2: https://chromium.googlesource.com/chromium/src/+/c5620d27d54077c5cba04dc829b582d2ab63be95/third_party/blink/renderer/core/streams/readable_stream_default_controller.cc
- 3: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultController/close
- 4: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultController/enqueue
- 5: ReadableStreamDefaultController: undocumented error conditions mdn/content#34597
Guard late pump writes after consumer cancellation.
When the wrapped response stream is cancelled, pending chunks or a source read() done result can still call controller.close() or controller.enqueue(result.value), which throws on a non-readable controller. Track cancellation in withSseKeepAlive and skip those controller operations so stream disconnects do not create unhandled promise rejections.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/mini-lilac-server/src/server.ts` around lines 104 - 141, Update
withSseKeepAlive’s ReadableStream pump to track consumer cancellation, set that
state in cancel(), and guard controller.enqueue(), controller.close(), and
controller.error() calls from pending reads or chunks after cancellation.
Preserve reader cancellation and keepalive cleanup while preventing late writes
from producing unhandled rejections.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83b190f8b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mini-lilac/build.ts`:
- Around line 32-35: Replace the bundledSource text scan with a Bun
metafile-based check in the build configuration: enable metafile generation,
inspect output imports, and throw when any import is marked external and matches
`@opentui/core` or its subpaths. Remove the obsolete regex guard while preserving
the existing failure behavior for retained external Core imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b753386-5789-4bfd-87ee-84410c90040d
📒 Files selected for processing (13)
apps/mini-lilac-server/config.example.yamlapps/mini-lilac-server/src/main.tsapps/mini-lilac-server/tests/main.test.tsapps/mini-lilac-tui/README.mdapps/mini-lilac-tui/src/cli.test.tsapps/mini-lilac-tui/src/cli.tsapps/mini-lilac-tui/src/controller.tsapps/mini-lilac-tui/src/startup.test.tsapps/mini-lilac-tui/src/startup.tsapps/mini-lilac/README.mdapps/mini-lilac/build.tspackages/utils/codex-oauth.tspackages/utils/tests/codex-oauth.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/mini-lilac-server/config.example.yaml
- apps/mini-lilac/README.md
- packages/utils/tests/codex-oauth.test.ts
- apps/mini-lilac-tui/src/startup.test.ts
- apps/mini-lilac-tui/src/cli.ts
- apps/mini-lilac-tui/README.md
- packages/utils/codex-oauth.ts
- apps/mini-lilac-server/tests/main.test.ts
- apps/mini-lilac-tui/src/startup.ts
- apps/mini-lilac-server/src/main.ts
- apps/mini-lilac-tui/src/controller.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab1da8fca4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| trigger: "submit-message", | ||
| chatId: this.sessionId, | ||
| messageId: undefined, | ||
| messages: [...this.messages], |
There was a problem hiding this comment.
For long or resumed sessions, this.messages contains the entire canonical transcript, so every prompt retransmits all prior assistant output and every base64 image. The server validates this whole array but only uses its last user message in createMiniLilacServer, making cumulative upload and validation cost quadratic and eventually risking request-body rejection for image-heavy sessions; keep the full local transcript for rendering but send only the newly submitted user message for admission.
Useful? React with 👍 / 👎.
| if (!settled) { | ||
| logger.warn("agent operation did not settle after cancellation grace period", { |
There was a problem hiding this comment.
When a provider or tool ignores cancellation for more than five seconds, this branch only logs and then falls through to finalization, which clears the active actor even though the original agent.prompt() promise is still executing. A follow-up run can consequently start while the timed-out model/tool continues operating, and shutdown can close the store because that outstanding promise is no longer tracked; retain the run's active/close-blocking state until the underlying operation settles, even if the client-facing run is finalized earlier.
Useful? React with 👍 / 👎.
1 participant
add mini-lilac general agent:
Summary by CodeRabbit