Repository navigation
fix(native-chat): preserve large structured command results - #17707
Conversation
|
Too many files changed for review (143 files, 100 file limit). Bypass the limit by tagging |
There was a problem hiding this comment.
ℹ️ No critical issues — one scaling note plus a scope question, both non-blocking.
Reviewed changes
Reviewed 142 files across three commits that replace the 1 MiB stdout string-accumulator cutoff with an incremental shared NDJSON framer, persist large command results as content-addressed journal blobs behind a byte/operation-accounted backpressure sink, reserve per-turn lifecycle capacity up front, and turn a proven provider exit into a generation-fenced host recovery event.
- Bounded non-fatal framing:
createIncrementalNdjsonFramer(16 MiB envelope) replaces the quadratic string accumulator; oversized/malformed records are classified from a bounded prefix and settle their pending request without killing a healthy provider. - Content-addressed result persistence:
item/completednow writes the complete payload as a journal blob with a 16 KiB inline head; the delta coalescer bounds retained streamed text with an explicit truncation marker. - Accounted sink backpressure: every queued closure is charged bytes/operations before enqueue, stdout pauses/resumes at watermarks, and a write failure converges on one bounded failed state.
- Reservation admission: per-dispatch/per-turn/per-item byte + append-slot tokens (rebuilt on reopen) prove a bounded terminal settlement even at quota saturation, including lifecycle-batch rows with idempotent settlement ids.
- Fenced exit recovery:
unexpected-exitvsrequested-close, acquisition-generation tokens, observed-exit lease release, and resume-capable-holder-gated reacquisition without redispatching unknown messages.
ℹ️ Out-of-scope root .npmrc deletion changes dependency hoisting
Commit 41c7a85 deletes the root .npmrc (shamefully-hoist=true, minimum-release-age=4320) in a PR otherwise dedicated to the native-chat result fix. shamefully-hoist=true changes where pnpm hoists packages — Electron apps commonly rely on it for native/unbundled module resolution at node_modules root — so removing it can alter build/runtime dependency layout on other machines or CI beyond what the summarized validation (tc:node, focused suites, a local Electron proof) covers. If this cleanup is intentional, confirm it's exercised by packaging CI; if not, it belongs in its own PR so a hoisting regression is bisectable.
Technical details
# Scope question: `.npmrc` deletion bundled with the fix
## Affected sites
- `.npmrc` (deleted) — `shamefully-hoist=true`, `minimum-release-age=4320`
## Required outcome
- Confirm dropping `shamefully-hoist` has no effect on Electron/desktop dependency resolution (packaging/runtime), or split the removal into a separate PR with its own validation.
## Open questions for the human
- Is `shamefully-hoist=true` actually stale under pnpm 10, or does the desktop build still rely on hoisted native modules?DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
| // so the peak check includes those bytes. | ||
| let physicalBytes = Math.max( | ||
| this.deps.physicalBytes(), | ||
| await journalDirectoryBytes(this.deps.journalDir) |
There was a problem hiding this comment.
Every append re-runs journalDirectoryBytes, a full recursive readdir + lstat over the journal directory including every retained blob. Now that this PR makes many large result blobs the normal case, each row write becomes O(total files) — paid again on every geometric streamed checkpoint. Consider maintaining the count incrementally from putJournalBlob/removeJournalBlob and only rescanning on open or when a concurrent opener is actually suspected, reserving the full directory walk for that once-per-open path.
There was a problem hiding this comment.
ℹ️ No new issues in this increment — one prior non-blocking observation remains open.
Reviewed changes
Reviewed the single new commit 624d5e3 since the prior pullfrog review — a narrow bugfix plus its regression test.
- Rebuilt append-slot enforcement:
JournalLifecycleCapacity.rebuildnow passesmaxAppendSlotsinto the tentative-turn reservation, closing a gap where a reopened unresolved submission could over-reserve an append slot past the session's reconstructed limit. - Regression coverage: a new test in
journal-lifecycle-capacity.test.tsprovesrebuildreturnsfalseand retains the first slot when two submission reservations need more append slots than are available. The assertion is exact (toBe(false)on the failure path plustoBe(1)onreservedAppendSlots), so it fails against the pre-fix code instead of merely passing alongside it.
No action requested on this commit. The earlier per-append journalDirectoryBytes traversal note and the bundled .npmrc hoisting question are unaffected and remain open for the author.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
|
Review status: land after fixes, with one review fix pushed.
No mobile-facing surface was touched; SSH/remote behavior is out of scope for this host-local change. |
|
Electron verification completed on the exact PR worktree using the rendered Orca UI and the load-balanced Codex backend.
Automated verification remains green for the native-chat/journal suite (140 tests), |
* fix(native-chat): preserve large structured command results (#17707) * fix(native-chat): preserve large structured command results * chore: place native chat validation artifacts under docs * chore: drop stale root package config * fix(native-chat): enforce rebuilt lifecycle append slots --------- Co-authored-by: Merge Sim <sim@local> * chore: omit native-chat reland planning docs * fix(native-chat): remove journal store import cycle * fix(native-chat): keep journal factory acyclic --------- Co-authored-by: Merge Sim <sim@local>
…#17707) * fix(native-chat): preserve large structured command results * chore: place native chat validation artifacts under docs * chore: drop stale root package config * fix(native-chat): enforce rebuilt lifecycle append slots --------- Co-authored-by: Merge Sim <sim@local>
…tablyai#17707)" (stablyai#17719) This reverts commit 17cede6.
…7720) * fix(native-chat): preserve large structured command results (stablyai#17707) * fix(native-chat): preserve large structured command results * chore: place native chat validation artifacts under docs * chore: drop stale root package config * fix(native-chat): enforce rebuilt lifecycle append slots --------- Co-authored-by: Merge Sim <sim@local> * chore: omit native-chat reland planning docs * fix(native-chat): remove journal store import cycle * fix(native-chat): keep journal factory acyclic --------- Co-authored-by: Merge Sim <sim@local>

Summary
Validation
Evidence: docs/native-chat-large-result-electron-evidence.md