fix(session): settle native tools before compaction - #539
Conversation
LeXwDeX
left a comment
There was a problem hiding this comment.
Standards review for commit 8d99729, compared with 953ca98 using the three-dot diff.
Reviewed all five changed files, including the release notes and delivery binding, against the root/package/runtime/test AGENTS.md files, CONTEXT.md, the release-notes template, and the code-review smell baseline. This is an independent Standards review; I did not author the host change.
Hard findings: 0. The change keeps terminal-event buffering inside the native adapter's existing scoped stream. It does not move storage/session responsibilities or change runtime selection. The tests exercise the real processor and LLM path with an HTTP model fixture; the latency case tests ordering, and the parallel/cancellation cases synchronize on readiness signals.
Optional heuristic: possible Duplicated Code in packages/opencode/test/session/processor-effect.test.ts at lines 848–852, 927–931, and 982–986. The three cases repeat the 32K model and processor setup. A shared preparation helper could reduce future fixture drift. This is non-blocking, has no demonstrated correctness impact, and does not warrant expanding this fix.
The isolated checkout has no matching codebase graph project, so this review used the fixed diff and exact source. Formatting/type checks already covered by tooling were not repeated. This comment records source-review findings only; it does not assert that CI has passed or that a DAG workflow has run.
LeXwDeX
left a comment
There was a problem hiding this comment.
Independent Spec review: 0 actionable findings for issue #538 at commit 8d9972908c308da1836a004cebe27c7c23db1acc.
Fixed comparison: 953ca98999eb1120fe9bf99a32178ea709b4dc2d...8d9972908c308da1836a004cebe27c7c23db1acc. The complete diff includes the native adapter, focused tests, delivery binding, and v1.0.40 release notes.
- Slow-tool completion before automatic compaction: the adapter retains provider
step-finishandfinishevents until local tools settle and their queued results reach the processor. The processor regression checks both thecompactdecision and a persisted successful result with its original input. - Parallel tools and result identity: immediate fiber dispatch remains concurrent; all settlements and queued results precede the retained completion events. The stream test checks the start barrier and event order; the processor test checks both distinct call IDs, completed states, and outputs.
- Explicit cancellation: the existing abort signal remains connected. The interruption regression waits for tool startup, interrupts the processor fiber, and checks actual abort and the persisted interrupted error.
- Scope: no changes alter DCP policy, compaction thresholds, model limits, or timeout/grace settings. The issue explicitly includes the release notes, and the binding matches issue #538 / PR #539.
This is a Spec-axis source/diff review. I inspected the test assertions; I did not independently repeat the author's reported full run of 66 passing tests, one existing skip, and package typecheck. CI, SpecGit, and publication remain separate gates.
The real-model DAG has not run. Its startup was rejected by automatic approval review before process creation and remains pending explicit authorization. There is no DAG run ID, node output, or aggregate verdict; this COMMENT review does not represent DAG approval or completion of all required review gates.
LeXwDeX
left a comment
There was a problem hiding this comment.
Supplemental independent review for issue #540, commit 43894ec, compared with 8d99729.
Standards
Hard findings: 0. Actionable heuristic findings: 0. Both audit=false settings are scoped to automatically disposed temporary fixture directories. The real Npm service still loads those project settings through NpmConfig and runs Arborist.reify. No installer mock or global environment override was introduced. The other npm.test.ts changes only wrap existing long lines.
Spec
Findings: 0. The cache-reification test retains its entrypoint assertion; the omit test retains omit=dev and both production-present/development-absent assertions. The existing default five-second deadlines are unchanged. The diff adds only the issue binding and fixture/test-file changes: production npm defaults, runtime implementation, workflows, policies, and explicit security-audit gates are unchanged.
I inspected the exact isolated-checkout diff and configuration/install path. The available graph describes another checkout at an older generation, so it was not used as evidence of these changes. The author's reported six passing tests and package typecheck were not rerun for this source review. Required CI, DAG review/synthesis, and SpecGit acceptance remain separate gates; this comment does not assert their completion.
LeXwDeX
left a comment
There was a problem hiding this comment.
Supplemental independent Standards and Spec review for e03ad87, compared with 43894ec.
Standards
Hard findings: 0. Actionable heuristic findings: 0. The earlier optional Duplicated Code observation is resolved. nativeCompactionProcessor is a named Effect helper reused by all three native regressions; it binds services explicitly and performs the shared model/processor setup without introducing a new runtime abstraction.
Spec
Findings: 0. The delta is confined to packages/opencode/test/session/processor-effect.test.ts and satisfies issue #538's review-cleanup scope. The model still uses the same provider reference and 32K/4K limits. Passing msg.sessionID to processors.create is equivalent here because each message is constructed by assistant(chat.id, ...). The delayed tool, parallel readiness signals, explicit Fiber.interrupt action, assertions, timeouts, and real runtime path remain unchanged.
The optional setup-duplication item is closed by this review. I inspected the exact final-commit diff and existing fixture construction; I did not author this change or rerun checks already reported by its author. CI must evaluate the new head. DAG review/synthesis and SpecGit acceptance remain separate requirements; this comment does not assert that those gates have completed.
LeXwDeX
left a comment
There was a problem hiding this comment.
Independent merge-provenance review for 93959af. Verified its exact parents: reviewed feature head e03ad87 and dev 28f14d2.
Findings: 0. All 26 integrated files were individually compared by raw bytes, Git blob identity, and file mode; each exactly matches the dev parent. The five feature files (release notes, npm fixture tests, native runtime, and both native regression test files) exactly match the reviewed feature parent. The resolved .specgit.yaml also matches that feature parent and retains PR #539, issues #538/#540, and the delivery branch.
The complete diff against dev contains only those five previously reviewed files plus the binding. There are no unexpected paths, unresolved entries, or uncommitted changes. The merge introduces no hand-written behavior change.
This comment verifies the merge tree's provenance and closes the conflict-resolution review. Required CI, DAG review/synthesis, and SpecGit acceptance remain separate gates; it does not assert their completion.
Closes #538
Closes #540
Why
High provider usage could request automatic compaction while a native local tool was still running, aborting it before its result reached the processor. Local-file npm tests also waited on unrelated online audit traffic before reaching their existing assertions.
What changed
step-finishandfinishevents until concurrent local tools settle and their results reach the session processor. Preserve tool IDs, parallel execution and explicit cancellation.Evidence
Current head:
00695dab86358a34b5b756f30df07bccfbe4e309; parents are reviewed #539 head93959afa302a058fdb6210904dd07f6dfaa23185and dev8060765fccf3c3f7daf3b8b2f87f8cbe48b7a8f9.Independent Standards and Spec reviews found zero code blockers at
93959afa30. The three native implementation/test files are byte-identical at the current head. The two merge conflicts retain the #539 delivery binding and dev's exact npm test file; all other imported code is unchanged from dev.Current merged-tree validation:
git diff --checkpassed.All CI checks passed at
00695dab86: Typecheck, complete Test CI, and CodeQL. Core: 1225 passed / 6 skipped / 0 failed. Opencode: 4429 passed / 23 skipped / 1 todo / 0 failed. Generated client/SDK freshness passed; all three HttpAPI modes reported 230 passed with no failures, skips, missing or extra routes. Linux and Windows E2E passed. SpecGit 1.10.1 accepted this exact PR/head with exit 0; it will be rerun after this evidence update before merge. Earlier cancelled/superseded checks are not acceptance evidence. The user clarified that this Codex delivery uses subagents to carry out the review workflow. Independent Standards and Spec reviews, merge-resolution review and an independent synthesis checkpoint are complete, with zero code or merge blockers. The checkpoint binds the reviewed native files and dev imports to the current head; CI and SpecGit acceptance remain separate required gates. No external model endpoint is needed for this workflow.Checklist
specgit finish --jsonwith exit 0 before merging to dev.