feat: add OpenCode runtime provider (macOS/Linux draft) - #2077
Conversation
yuezengwu
left a comment
There was a problem hiding this comment.
Exact-head review at db3dee193bb0c91a267722e068bc974532f6ddb6: keep this draft and NOT QA READY. CI is green, and the third commit fixes the CodeQL regex finding, but the product contract still has blocking gaps:
OPENCODE_SUPPORTED_VERSIONstill rejects compatible 1.x releases such as 1.18.9; the bounded contract is>=1.18.7 <2.0.0, fail-closed for prerelease/unparseable values. The resolver also needs the official~/.opencode/binpath and a real Windows npm-global native-exe path shared by capability and handler.- The fixed
agent.first-tree, original MCP keys, and top-level permission are deep-merged with operator-global OpenCode config. Managed agent/MCP keys need caller-scoped stable namespaces so same-name global entries cannot form hybrid objects; empty managed sets must preserve unrelated global config. - Current Chat Context/output contract is still placed in
OPENCODE_CONFIG_CONTENT. It must be a one-shot prefix on the next messageful stdin turn, with preparation/queued input unable to consume it and abort/replay-safe retry restoring it. - JSONL success ignores
unknownCount; unknown events carrying a session id evade even that counter. Support knownreasoning, then fail closed on every other malformed/non-object/unknown non-empty stdout line, require one matching session id and exactly one non-tool-call terminal, and reject managed-agent warning/default fallback. - Consumed provider failures still emit only a plain error before
DeliveryToken.complete, soSessionManagerhas no standard provider event to persist a runtime failure notice before ACK. Use the existingProviderAttempt/provider-retry settlement chain and cover notice-post failure, unsafe effects, deterministic failure, and pre-effect replay. Any non-read-only tool event must set the effect fence regardless of its reported phase. - The default Windows supervisor still throws before every OpenCode invocation. That is a safe seam, not Windows product support. Formal Windows support requires the client-wide named Job authority described in Context Tree draft #851 and product-path drain integration.
Please add scoped follow-up commits and regression tests without amending or force-pushing the existing commits. Cross-link draft Context Tree PR https://github.com/agent-team-foundation/first-tree-context/pull/851 in this PR body.
yuezengwu
left a comment
There was a problem hiding this comment.
Blocking review — exact head fbbb6f4a0e29c9a110d5f2b3b2047eb84bcf16f7
I verified that the PR, remote branch, and detached review worktree all resolve to this head. The strict JSONL, general 1.x range, unsafe-tool detection, stdin-only prompt, and structured terminal-event changes are real improvements, but this head is still NOT QA READY and must not be frozen for the formal harness.
-
Provider retries are unbounded across the recovery path and ignore the policy delay.
settleFailure()increments the handler-closureproviderTurnFailureAttempt, immediately callstoken.retry(), then callsfailSessionForRecovery()whenever the stream produced activity (packages/client/src/handlers/opencode/index.ts:736-752).SessionManager.failSessionForRecovery()evicts that handler and creates a fresh one for redelivery, resetting the counter to zero. A repeatable pre-visible protocol/transport failure can therefore retry forever;settlement.decision.delayMsis also never observed. Use a bounded in-turn retry loop like Cursor, or persist the attempt across handler replacement, and add a realSessionManagerregression proving finite attempts, the prescribed delay, and terminal settlement after repeated stream-started failures. -
One-shot context and briefing custody still advance before delivery actually settles.
start()ignores the boolean returned byrunTurn()and unconditionally writes the briefing fingerprint (index.ts:1065-1080), so any safe retry advances the baseline. Separately, a terminal provider failure awaitstoken.complete()and then clearspendingChatContextPrompt/returnstrue(index.ts:755-758), butSessionManager.completeDeliveryTurn()converts a failed durable-notice post into inbox retry and resolves the token call normally. That redelivery has no one-shot prefix and its caller may already have advanced the fingerprint. Add an observable completion disposition (or equivalent message-ID custody) and test through the actual SessionManager: notice post fails → no ACK → the same message is redelivered with the one-shot prefix → the fingerprint remains unchanged until a real terminal settlement. -
Timeout/abort can replay an unsafe effect. After the child emits any write-capable tool event, the abort/timeout branch still unconditionally calls
token.retry()(index.ts:846-849) instead of settling fromstate.sawUnsafeTool. A completedbash/edit followed by a hung process will be replayed. Also, an exception after spawn (for example file cleanup) reaches the outer catch with a fabricated all-false state (index.ts:942-948), losing observed effects. Preserve the real turn state and route abort/timeout/cleanup failures through the same replay-safety settlement. Add completed/failed write-tool + timeout/abort tests that require durable terminal notice and no replay. -
The private projection still violates the isolation contract. The generated config retains a top-level
permissionblock (index.ts:136-142), so OpenCode deep-merges permissions into unrelated global agents; keep permissions only inside the managed agent. Large projections are written under the OS-widetmpdir()(index.ts:207), while draft Tree #851 requires private runtime-owned state inside the agent workspace; a daemon crash can also strand MCP headers there without an ownership/recovery sweep. In addition, bothbuildEnv()andprojectOpenCodeConfig()delete an inheritedOPENCODE_CONFIG(index.ts:193-194, 314-315), hiding provider-owned custom host configuration that OpenCode normally loads beforeOPENCODE_CONFIG_CONTENT. Preserve unrelated host config, put file-backed state in an owned workspace runtime area with crash cleanup, and test empty-MCP/global-custom-config preservation plus success/failure cleanup. -
The caller namespace is unique per handler, not stable per caller as requested.
randomUUID().slice(0, 12)(index.ts:281-282) makes the same First Tree agent acquire a different managed agent/MCP identity after every recovery/restart; the new test explicitly asserts that two handlers for the sameagent-1differ. Derive a full, sanitized/high-entropy stable scope from the First Tree agent identity (and chat/session only if that is the intended caller boundary). Test same caller across fresh handlers is stable and distinct callers do not collide. -
The empty-PATH Windows npm resolver test does not exercise the production resolver. The test injects
wellKnownDirs: () => [root], but the defaultwellKnownBinDirs(home)has no%APPDATA%\\npmcandidate. An actual Windows daemon with an empty service PATH still misses the normal global npmnode_modules/opencode-ai/bin/opencode.exe. Add the production APPDATA/npm candidate and a test using default candidate construction rather than a stub that supplies the answer. -
The version parser is not strict semver yet. It accepts invalid releases such as
01.18.7and treats1.18.7_suffixas1.18.7, despite the fail-closed stable-semver contract. The Client already depends onsemver; use a bounded strict parse/range check or reject leading-zero and trailing-token variants explicitly, with regressions.
Keep the PR draft. Please append scoped fixes (no amend/force-push), update tests for the actual SessionManager paths above, and return a new remote exact SHA before QA freeze.
yuezengwu
left a comment
There was a problem hiding this comment.
Blocking review — exact head d0eaff2078eb6d72373304bd4ca91fd1d1ef724b
I verified that the PR head, detached review worktree, and commit object all match this SHA. All repository checks are terminal green, the new CodeQL run is green, and alert #109's latest instance is fixed. The previous strict-semver, production Windows resolver, structured JSONL, unsafe-effect, stable caller namespace, failure-notice custody, and fresh-handler retry-delay findings are materially addressed.
This head is still NOT QA READY because two product-contract blockers remain:
-
The private-config cleanup follows a workspace-controlled ancestor symlink and can recursively delete or write outside the agent workspace.
prepareSession()builds a deterministic path below.first-tree-workspace/opencode-config, then calls recursivermSync()before anylstat/canonical containment check (packages/client/src/handlers/opencode/index.ts:1025-1032).projectOpenCodeConfig()later runsmkdirSync/chmodSync/mkdtempSyncthrough the same unchecked chain (index.ts:205-220).acquireAgentHome()only converges.first-tree-workspaceitself; an agent/tool can leaveopencode-configas a symlink. I reproduced with Node's actualrmSync: an intermediateopencode-config -> /outsidesymlink made the exact recursive call delete/outside/<scope>/sentinelwhile the symlink remained. Fail closed on every symlink/non-directory managed ancestor using canonical workspace containment and no-follow/opened-handle checks, and add an external-sentinel regression.The same deterministic per-agent/per-chat root also has no handler-generation ownership fence.
SessionManager.failSessionForRecovery()starts old-handler shutdown without awaiting it (packages/client/src/runtime/session-manager.ts:1245-1249); a replacement handler can prepare the same root, while the old handler's latershutdown()recursively removes that root (packages/client/src/handlers/opencode/index.ts:1228-1233). Use a stable caller parent plus a unique per-handler owned child (with lock/journaled stale cleanup), so an old generation can remove only its own state. Add a forced-order regression: replacement prepares a file-backed config first, then old shutdown completes, and the replacement config plus external sentinel must remain intact. -
The finite provider-retry window is still reset by legitimate redelivery regrouping and its state is unbounded.
deliveryAttemptKey()includes the entire timing-dependent fused message list (index.ts:317-319), whilerunQueued()drains every message currently present (index.ts:1048-1073). A failed fused turn[A,B]can redeliver as[A](or[A,B,C]) after handler recovery, producing a new key and restarting attempts at 1. The new tests only replay one message with an identical list, so they cannot prove finite attempts for the actual queue path. Anchor attempts to a stable delivery-head identity (prefer the coordinator/inbox entry identity), retain it across batch regrouping, and give abandoned entries bounded/owned cleanup rather than a process-global map that only deletes on later terminal success. Add a real SessionManager regression that changes the fused tail across fresh handlers and still reaches terminal settlement after exactly the policy limit.Also make the policy delay cancellation-aware. The production
retrySleepis a plain timer (index.ts:291-293, 768-773), so suspend/shutdown waits up to 15 seconds and the stale turn still callstoken.retry()/failSessionForRecovery()after cancellation. Tie the delay to the turn abort/generation and prove suspend during the delay produces no post-cancel custody mutation.
While tightening custody, please replace Promise<unknown> on DeliveryToken.complete / SessionContext.finishTurn with the documented Promise<DeliveryCompletionDisposition | void> union (packages/client/src/runtime/handler.ts:107-127). The current runtime checks the right literal, but unknown removes compile-time enforcement from the new settlement contract.
Keep the PR draft and append scoped commits without amend/force-push. Windows remains intentionally fail-closed and still requires the separate client-wide Job implementation before this can become a cross-platform acceptance candidate.
yuezengwu
left a comment
There was a problem hiding this comment.
Blocking review — exact head 6c1d87ae00deebed2295096dbfb001c224bd7df5
I verified that the PR head, detached worktree, and local commit object all match this SHA. The stable delivery-head key, unique handler generations, journal/lock structure, cancellation-aware delay, and typed completion result are substantive improvements. This head remains NOT QA READY for two correctness/security blockers, and its required CI run is currently red.
-
A live lease still hands an unchecked raw path to the file-backed projection, so replacing the handler child with a symlink after acquisition escapes the workspace.
acquireOpenCodePrivateConfigLease()validates the chain only while acquiring/closing and returnsruntimeRootas a string (packages/client/src/runtime/opencode-private-config.ts:54-129). Each oversized turn then passes that string toprojectOpenCodeConfig(), which directly callsmkdirSync/chmodSync/mkdtempSync/writeFileSyncwithout asking the lease to revalidate the handler child (packages/client/src/handlers/opencode/index.ts:210-241, 858-864). The OpenCode process and its allowed tools share the workspace and can list the generation name, removehandler-<id>, and replace it with a directory symlink between turns. The next large projection follows it and writes the private config outside the workspace;close()notices only after the escaped write.I reproduced the exact production filesystem sequence: create the leased handler directory, replace that leaf with
-> /outside, then run the same recursive mkdir/chmod/mkdtemp/write calls.opencode.jsonwas created under/outside/turn-*whileruntimeRootremained a symlink. Keep creation and cleanup behind lease methods (not a reusable path string), revalidate the full caller + generation chain immediately before every mutation, reject a replaced generation leaf, and use no-follow/opened-file identity checks for the config file. Add a production-path regression that acquires a lease, swaps the handler child to an external-sentinel symlink, forces file-backed projection through the handler, and proves fail-closed with no external write or deletion. The current tests only cover a symlink present before lease acquisition and inline-config replacement handlers. -
Canceling a retry delay erases the attempt window even though suspend/preemption deliberately retains the inbox delivery for recovery. The cancellation branch deletes
attemptKey(packages/client/src/handlers/opencode/index.ts:817-830), and the new test explicitly expects the map to become empty.SessionManageruses handler suspend/shutdown for concurrency preemption and canceled route transitions whileprepareEvictkeeps the work unacked. On resume/redelivery, the same delivery therefore restarts at attempt 1; repeatedly preempting during the delay defeats the finite 5s/15s/terminal policy. Cancellation must prevent post-cancel token/failSession mutations, but it must not forget an unsettled provider attempt.Retain the stable-head attempt across cancellation until that delivery actually settles or coordinator ownership proves it abandoned. The 30-minute TTL and 512-entry LRU also currently evict without consulting custody, so they can reset a still-unacked head; bind cleanup to the delivery coordinator/ledger (or an equivalent ownership callback) rather than wall-clock/LRU alone. Add a real SessionManager regression: fail attempt 1, preempt/suspend during its delay, redeliver the same head, and prove the next failure is attempt 2 and the sequence still terminally settles at the policy limit.
The exact-head CI run 30463028789 is also failed: Test Client & Web timed out in the existing managed-skills.test.ts target_backed_up case, and the aggregate Test gate failed. It appears unrelated/flaky, but this SHA is not remotely green; obtain a clean terminal rerun on the next candidate rather than treating the local full-client pass as the gate.
Keep the PR draft, append scoped commits without amend/force-push, and return a new remote exact SHA before QA freeze. The separate Windows client-wide Job implementation remains a formal platform blocker as already documented.
yuezengwu
left a comment
There was a problem hiding this comment.
Exact-head readback — ccf1cf074e7876094c54749dffe5b087754acb6e
I verified that the GitHub PR head, origin/feat/opencode-runtime-provider, and the clean detached review worktree all resolve to this SHA. The two blockers from the 6c1d87ae… review are materially addressed in the latest two commits:
- File-backed projection no longer receives a bare runtime path. The handler-generation lease owns materialization and cleanup, revalidates the caller/generation and per-turn directory identities around each path mutation, creates the config through an exclusive no-follow descriptor, and verifies the opened file identity. The real-handler generation-leaf replacement regression leaves the external sentinel untouched and fails closed.
- The provider-attempt ledger now retains the delivery-coordinator custody observer. TTL/capacity cleanup removes only coordinator-confirmed abandoned entries, and aborting the retry delay no longer deletes an unacked attempt. The real single-slot
SessionManagerregression preempts attempt 1 during the 5 s delay, resumes the same delivery head at attempt 2 / 15 s, and settles it terminally on attempt 3.
I found no new blocking correctness issue in the latest six-file scope. Local readback gates at this exact head:
- full
@first-tree/clientsuite: 162 files passed, 1 existing smoke file skipped; 1,751 tests passed, 3 skipped git diff --check: pass- exact-head CodeQL alert #109: latest instance remains
fixed, with no dismissal
This SHA must still not be frozen for formal QA. Since it was pushed, main advanced to f030d8e1d3468534055912bfc232d94c3d511ff4, and GitHub now reports the PR as conflicting. A three-way readback shows conflict markers in packages/client/src/index.ts and packages/shared/src/index.ts; the current head therefore has to change and all gates must be repeated on the reconciled candidate. The existing formal boundaries also remain: Windows product execution still fails closed pending client-wide Job authority, Context Tree PR #851 is draft, and product-path QA has not run.
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The current head materially closes the earlier delivery-custody and private-config isolation findings, but it still advertises OpenCode as runnable on Windows even though every Windows invocation is rejected, and the PR has left draft state while its stated design/QA gates remain open.
Risk level: A
- Path baseline: includes both
apps/cli/**andpackages/client/**-> A - Semantic lift: none; A is already the highest level
PR summary
- Author / repo: yuezengwu / agent-team-foundation/first-tree
- Problem: Allow an agent to use a host-authenticated OpenCode installation on macOS/Linux while preserving First Tree session continuity, delivery custody, private runtime projection, and process-drain guarantees.
- Approach: Adds OpenCode to shared/runtime/setup surfaces and implements a per-turn external JSONL handler with explicit session resume, caller-scoped config/MCP projection, managed Skills, provider retry settlement, and a POSIX supervisor seam; Windows intentionally fails closed pending Job Object authority.
- Impacted modules:
packages/client,packages/shared,packages/server,packages/web,apps/cli, andpackages/qa
Review findings
❌ 1. Windows clients advertise a provider that the product cannot run. probeOpenCodeCapability() returns available: true whenever the Windows executable exists (packages/client/src/runtime/capabilities/opencode.ts:19), and both Server runtime validation and Web provider selection treat that bit as runnable support. The default supervisor then rejects every Windows OpenCode process before spawn (packages/client/src/runtime/provider-process-supervisor.ts:40). This lets operators create or switch an agent onto a guaranteed-dead runtime. Gate advertised/selectable availability on platform execution support, or land the accepted Job Object supervisor in the same candidate, and cover the actual capability -> Server/Web selection path. [R5]
❌ 2. The PR is currently marked ready for review even though its own contract says it must remain draft and its durable Tree decision plus product-path QA are still unresolved. Restore draft status and keep it there until those stated release gates have a frozen candidate and terminal evidence.
packages/client/src/handlers/opencode/index.ts:1197). The original FIFO head and all later input can therefore remain held with no durable/chat-visible explanation of what is blocking progress. Keep the delivery unacked if that is the safety contract, but emit a durable, actionable resilience/error signal at a bounded cadence and cover the queued path (the start/resume retry path already has structured events). [R4]
Action taken
- Submitted request changes on exact head
ed2f62f8efd067930c37e015484d7ede92dd97b6.
|
Addressed the exact-head review in two additive commits (no history rewrite):
The PR is restored to draft and remains macOS/Linux-only pending the client-wide Windows Job Object authority. Focused validation at
|
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: comment
- Rationale: The synchronized exact head materially addresses all three findings from the prior review; no new code blocker was found in the two-commit delta, but the PR remains intentionally not QA-ready pending its stated Tree decision and product-path QA gates.
Risk level: A
- Path baseline: includes both
apps/cli/**andpackages/client/**-> A - Semantic lift: none; A is already the highest level
PR summary
- Author / repo: yuezengwu / agent-team-foundation/first-tree
- Problem: Allow an agent to use a host-authenticated OpenCode installation on macOS/Linux while preserving First Tree session continuity, delivery custody, private runtime projection, and process-drain guarantees.
- Approach: Adds OpenCode across shared/runtime/setup surfaces and implements a per-turn external JSONL handler with explicit resume, managed private projection, delivery settlement, and POSIX process supervision; unsupported Windows execution is now withheld from selectable capability advertising.
- Impacted modules:
packages/client,packages/shared,packages/server,packages/web,apps/cli, andpackages/qa
Review findings
✅ 1. ddc12e911 closes the Windows availability mismatch. Capability advertising and the default spawn path now share supportsDefaultProviderProcessSupervision; an installed Windows binary is retained for diagnostics but reported available: false. The added Server and Web regressions cover rejection during agent creation and omission from runtime switching.
✅ 2. The PR has been restored to draft, matching its body and the unresolved formal gates.
✅ 3. 456c54c76 closes the queued unsafe-discovery visibility gap. The handler emits a structured provider_retry_scheduled warning on the existing exponential cadence while leaving the FIFO delivery unacknowledged, and the production handler test covers both queued preflight locations.
Action taken
- Submitted a comment review on exact head
456c54c768caa46f620de4ea817bef4c019cf741; no approval was submitted.
Formal QA evidence — PASS
Frozen target
The result is PASS for this exact head and the scope below. Before task Windows remains unavailable and pre-spawn fail closed
This proves the current unsupported contract. It does not claim Windows Established queued session: durable warning, zero admission/ACK, FIFO recoveryBoth provider-preflight unsafe-discovery races were reproduced through the
Every observed payload used the existing
Throughout each blocked window:
After safety returned, the first-race head ( The new durable warning never became provider admission, a terminal notice, Session start/resume and lifecycle custody
Linux/macOS positive controls and adjacent boundaries
Performance, limitations, and cleanup
Cleanup completed: both Client daemons stopped gracefully; run-local This comment is the self-contained canonical GitHub evidence for the exact-head |
|
@baixiaohang,请对当前 exact head 两个冻结 release gate 均已完成:
本产品 head 自 formal QA 起未漂移;exact-head CI及三类CodeQL均成功。此前 PR现已转Ready,但不会由当前agent执行merge。请按 exact head提交最终 |
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: approve
- Rationale: The frozen exact head closes the prior review findings, passed independent product-path QA, and is now backed by the merged durable Context Tree decision; I found no remaining product-code blocker.
Risk level: A
- Path baseline: includes
packages/client/**andapps/cli/** - Semantic lift: none beyond the already-highest path baseline
Review findings
- ✅ Windows capability, Server/Web selection, and direct or stale invocation now consistently advertise unavailable and fail closed before spawn.
- ✅ Persistent unsafe managed-Skill discovery preserves unacknowledged FIFO custody while emitting bounded, actionable durable retry signals.
- ✅ Independent formal QA passed this exact product head across the Linux/macOS positive paths, Windows unavailable boundary, delivery recovery, lifecycle cleanup, and provider session continuity.
- ✅ Context Tree #851 was reconciled with the tested behavior, independently reviewed, approved, and merged.
⚠️ Windows OpenCode execution remains future work; the current explicit unavailable boundary is appropriate for this macOS/Linux release.
Action taken
- Approved exact head
456c54c768caa46f620de4ea817bef4c019cf741.
Summary
opencodeacross shared runtime schemas, Server invariants, daemon capability/reprobe, handler registration, CLI, and Web setup/config surfaces.opencode/skills, provider-native model passthrough, and DeliveryToken custodyopencode auth login) without reading, storing, or forwarding provider credentialspackages/qaacceptance caseThis PR does not modify the one-time Windows protocol harness in #2072 and does not modify the Context Tree.
Runtime contract
>=1.18.7 <2.0.0; prerelease, older, major-two, and unparseable versions fail closed~/.opencode/bininstall plus npm's package-native Windowsopencode.exe, never.cmd/shell:true; macOS/Linux advertise resolved installs, while Windows retains the resolved path only for diagnostics and reportsavailable: falseuntil the client-wide Job supervisor existsopencode --versionand the serial per-data-home DB readiness gate use the same process supervisor as turns--agentOPENCODE_CONFIG_CONTENT; large projections use a 0700/0600 runtime-owned file below the agent workspace. Each handler owns a unique generation below a stable caller parent guarded by a kernel lock and crash journal; the lease revalidates the caller/generation inode before every mutation and creates the config through an exclusive no-follow file descriptor. Symlink/identity replacement fails closed, and delayed shutdown can only remove the owning generation. A hostOPENCODE_CONFIGis preserved, and an oversized projection fails closed rather than replacing itopencode run --format json --auto; prompt content never enters argv, model is passed verbatim only when configured, and only stream-confirmed session IDs reach--sessionreasoningis explicitly ignored, unknown/malformed output fails closed, and success requires one matching session ID plus exactly one non-tool-callsterminal eventProviderAttempt/provider.retry:contract. Attempt state is keyed by the stable delivery head and survives fused-batch regrouping, fresh-handler recovery, and suspend/preempt cancellation while the delivery coordinator still owns an unacked row. TTL/capacity cleanup may forget only coordinator-confirmed abandoned state. Policy delays are generation/abort-aware, so suspend cannot mutate retry custody after cancellation. Credential/config and unsafe-effect failures produce a durable terminal notice before consumed ACK; unknown pre-effect failures retain replay custodyValidation
pnpm check— pass (only the repository's existing 16 warnings and 1 info)pnpm typecheck— pass, 11/11 taskspnpm --filter @first-tree/client test— 1,750/1,750 pass, 3 existing smoke tests skippedgit diff --check— passFormal gates and platform boundary
f71199102bc3019cdf6d0e1f112345cd483b11fc.Windows product execution is not implemented by this PR. Windows reports OpenCode unavailable and fails closed. Formal Windows execution acceptance remains future work that requires the owner-reviewed Job Object drain authority plus a client-wide non-breakaway, kill-on-close supervisor. The successful #2072 protocol harness is evidence for that future implementation, not product acceptance for this macOS/Linux PR.