feat(agent): subagent lifecycle v1 (foreground-only)#287
Merged
Conversation
Astro-Han
added
enhancement
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds session metadata columns and a SubagentRun subsystem: persisted subtask parts with lifecycle/status/events, concurrency and row-level guards, prompt interruption wiring, two agent tools (agent_list, agent_output), a migration snapshot, and tests exercising lifecycle and guards. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as AgentTool
participant Run as SubagentRun
participant Session as Session.Service
participant DB as Database
participant Ops as AgentPromptOps
Agent->>Run: reserveSlot(parentSessionID)
alt slot available
Agent->>Run: start(startInput)
Run->>Session: create/update subtask part (status="running")
Session->>DB: INSERT/UPDATE subtask row
Agent->>Ops: prompt(childSession)
Ops->>Ops: execute subagent work
alt interrupted
Ops-->>Agent: wasInterrupted(childSession)
Agent->>Run: finalize(childID, status="canceled_by_user", partial_result)
else completed
Agent->>Run: finalize(childID, status="completed")
else failed
Agent->>Run: finalize(childID, status="failed", error)
end
Run->>Session: recordEvent/updatePart(final fields)
Session->>DB: UPDATE subtask row
Agent->>Run: releaseSlot(parentSessionID)
else too many active
Agent->>Run: recordRejected(parentSessionID, error="too_many_active")
Run->>Session: create failed subtask row
Session->>DB: INSERT subtask row
end
sequenceDiagram
participant User as Client
participant List as agent_list Tool
participant Query as SubagentRun.Service
participant DB as Database
User->>List: execute(status, limit)
List->>Query: list(parentSessionID, filter)
Query->>DB: SELECT subtasks WHERE parent_session_id=?
DB-->>Query: rows
Query->>Query: format entries (status, latest, elapsed)
Query-->>List: formatted output
List-->>User: { title, metadata, output }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a robust subagent lifecycle management system. Key changes include the introduction of a SubagentRun service to manage subagent states and slot reservations (limiting active subagents to 5 per parent), and the addition of agent_list and agent_output tools for monitoring and retrieving subagent results. The agent tool has been significantly refactored to utilize these new services, improving the handling of task completion, failures, and user cancellations. Database schemas and session models were updated to support these features. Review feedback suggests improving debuggability in the agent_output tool by replacing generic error messages with more specific ones for different failure conditions.
Lays the groundwork for subagent lifecycle tracking (#283). Adds two nullable Session columns plus the corresponding Info/CreateInput/Interface fields so the agent tool can mark child sessions as agent-tool-created and record the requested subagent type.
…Type Wires the new Session lifecycle fields (#283) through the agent tool's sessions.create call so dispatched subagents are identifiable as agent-tool-created and tagged with the requested subagent type.
Adds the SubtaskEvent discriminated union and extends SubtaskPart with identity (tool_call_id / parent_session_id / parent_message_id / subagent_session_id), lifecycle (status / started_at / updated_at / ended_at / consumed_at), progress (last_activity / recent_events), and result (result_summary / result_text / partial_result / error) fields required by #283. Every new field is optional or defaulted so legacy SubtaskPart rows decode as terminal-completed without rewrites. Existing literal writers in prompt.ts and the prompt-effect test get explicit status/recent_events defaults to satisfy the inferred output shape.
Introduces the SubagentRun service that owns subagent lifecycle writes (#283). Slot reservation is gated by a per-parent semaphore at MAX_ACTIVE=5; each tool_call_id has its own row-level semaphore for serialized lifecycle mutations. start/finalize/patchSession/recordActivity/setConsumed are implemented; recordEvent/recordRejected/findLatestBySessionID/list remain explicit Effect.die stubs filled in by Tasks 5/11/12/13.
Wires per-Instance interruptedSessions tracking through SessionPrompt's ops factory so AgentTool.execute can deterministically distinguish a child runner abort from a natural completion or model failure (#283). The set is reset at the start of every prompt() call and added to inside loop()'s onInterrupt branch, so the read happens after ops.prompt resolves without racing ctx.abort.aborted. Test stubOps gains an optional interruptedSessions hook for tests that need to simulate abort.
Implements recordEvent with append-and-evict semantics: the ring is capped at 20 entries; when full, the oldest non-lifecycle (progress) event is removed. Lifecycle events (started, completed, completed_empty, canceled_by_user, failed, consumed) stay pinned so the rendered timeline always shows the run's beginning and outcome regardless of intermediate tool churn (#283).
Adds a Context.Reference (SubagentRunWriterContext) that is true only inside SubagentRun service writers. Session.updatePart consults it on SubtaskPart writes and dies with SubagentRunGuardViolation when an outsider tries to mutate lifecycle fields. The reference lives in its own module so both writers and the guard can observe it without a circular Layer dep, deviating from the spec's Layer-wrapper sketch but preserving the API-level enforcement intent (#283).
…t error catch Restructures AgentTool.execute around the SubagentRun service (#283): - Cap-rejection short-circuits before reserveSlot, returning a synthesized failed output via recordRejected (Task 11 will fill in that writer). - Effect.scoped wraps the rest of the body so releaseSlot and the parent abort listener detach via Effect.addFinalizer on every exit path, closing the prior reserveSlot/acquireUseRelease interrupt window. - start/patchSession/finalize replace ad-hoc lifecycle bookkeeping; an outer Effect.catch finalizes a started SubtaskPart to `failed` so pre-prompt errors can never strand the row in `running`. - A pre-aborted check finalizes canceled_by_user without invoking ops.prompt. - finalizeAfter latches `interrupted = wasInterrupted || ctx.abort.aborted` synchronously at handler entry so a late parent abort during cleanup of a real model failure cannot misclassify it. ToolRegistry layer requirements grow to include SubagentRun.Service; the test layer composers in prompt-effect.test.ts and snapshot-tool-race.test.ts provide it. Three older agent.test.ts cases (resume + canAgent shaping) are skipped with `it.live.skip` and updated by Tasks 8/9. synthesizeOutput, truncateHead, and readLastCompletedAssistantText are minimal stubs; Task 10 replaces them with the structured-output + partial result implementation.
The agent permission deny rule is now unconditional regardless of whether the dispatching subagent has canAgent permission. Combined with the already-unconditional `agent: false` entry in the prompt-time tools map, this prevents the v1 non-goal of nested subagent dispatch (#283). Removes the unused canAgent local; updates the matching agent.test.ts permission and tools assertions and rescues the previously skipped case.
Replaces the skipped resume tests with three positive/negative cases that match the new agent.ts contract (#283): a SubagentRun-created child resumes successfully, a missing session id fails fast, a plain child session (no createdByAgentTool) is rejected, and a subagent_type mismatch fails. The "creates a child anyway when missing" legacy case is left skipped — Task 13's agent_output covers the lookup semantics for the remaining paths.
Replaces the Task 7 stubs with the production implementation (#283): synthesizeOutput preserves the exact 5-line `<subagent_result>` format on the success path so existing prompt teaching still parses, while non-completed statuses surface a `status: <status>` header (and a sanitized `error: ...` line for failed runs) before the wrapper. Sanitization strips stack traces, host paths, and JSON envelopes and caps the message at 200 chars. readLastCompletedAssistantText reads the most recent stable assistant text from the child session for partial_result on cancel and returns null when no completed text exists, avoiding leaked half-tokens. TextPart completion is keyed off `time.end !== undefined`.
When the parent already has 5 active subagents, AgentTool.execute now delegates to SubagentRun.recordRejected which writes a fully-formed SubtaskPart in `failed` state with `error.kind = too_many_active` and recent_events seeded with [started, failed]. The model sees a synthesized output via synthesizeOutput's failed branch instead of a thrown error, keeping the cap path inside the lifecycle vocabulary (#283).
Adds the agent_list model-callable tool plus the SubagentRun.list and collectSubtaskParts helpers it relies on. List filters by lifecycle status (running / completed / completed_empty / failed / canceled_by_user) and the two synthetic filters all_active (running plus unread terminal) and all. Rows are returned newest-first; legacy SubtaskParts decoded without tool_call_id stay visible with elapsed=- and label=(legacy) so older runs remain reachable (#283).
Adds the agent_output model-callable tool. Accepts exactly one of subagent_session_id or tool_call_id (XOR enforced via Zod refine), verifies the row's parent_session_id matches the caller and that the child session was created by the agent tool, then formats either the result text (status: result) or a transcript preview (status: transcript) plus event count and last_activity. Reading a row in any terminal status calls SubagentRun.setConsumed once so future agent_list runs in all_active mode skip it. SubagentRun.findLatestBySessionID powers the subagent_session_id lookup branch (#283).
Wires the two new model-callable inspection tools into the builtin tool list right after `agent`, so they are reachable from any agent that already has agent dispatch enabled (#283).
Drives the SubagentRun service through completed, completed_empty, failed (execution_error), failed (too_many_active via recordRejected), and canceled_by_user terminal states end-to-end (#283), and exercises findLatestBySessionID + list filters across running/completed/all.
Drives 5 reservations + 5 row writes through the service, asserts the 6th reservation hits TooManyActive, then interleaves reads with releases. If the per-row mutex (start, finalize, etc.) ever called back into reserveSlot the loop would hang on iteration 6, so passing this test confirms the documented lock-ordering invariant (slot acquired before row, never the reverse) holds in code (#283).
Allow setup() body Effect to surface NotFound from svc.read; runner uses Effect.orDie to bubble unexpected failures so tests still see them as test failures instead of swallowing them as undefined behavior.
…row catch
Three review fixes to SubagentRun:
- recent_events ring now enforces a hard cap of 20 after the lifecycle-priority eviction pass. Previously, if all 20 entries were lifecycle events the inner while loop would break (no progress event to evict) and the array kept growing past 20, which violates SubtaskEvent.array().max(20) on next decode.
- New readByToolCallID(parentID, toolCallID) falls back to scanning persisted SubtaskParts when the in-memory tool_call_id index misses, so agent_output { tool_call_id } keeps working across host restarts; on a hit it backfills the cache so subsequent reads stay fast.
- The shared read-or-skip shell is extracted as withExistingPart and the readPart catch is narrowed from Effect.catch to the typed Effect.catchTag("NotFound", ...) so storage failures surface as defects instead of being swallowed as missing rows.
Also drops the unused Bus.Service requirement from layer.
Two review fixes to AgentTool.execute: - finalizeAfter no longer ORs ctx.abort.aborted into interrupted. A parent abort that lands AFTER ops.prompt's natural success but BEFORE the tap callback runs would otherwise relabel a completed run as canceled_by_user and lose the result_text. ops.wasInterrupted is the authoritative signal because it is set by SessionRunState.onInterrupt firing on the child runner; the pre-aborted short-circuit upstream already handles aborts that landed before ops.prompt was invoked at all. - The success-path tap now wraps finalizeAfter in Effect.catchCause so a transient finalize failure cannot propagate to the next pipe step and re-finalize as kind: err, rebranding a completed run as failed. finalize is already idempotent against repeat calls. Also narrows the outer catch of Session.read to the typed NotFound tag so storage failures surface as defects instead of being swallowed.
…Schema Combines two changes that arrived together during rebase onto the upstream tool-framework-Schema migration (PR #270 / #23244): Review fixes (originally a separate fix(agent_output) commit, folded in because the same files needed Schema migration in the same hunk): - agent_output: switches the tool_call_id lookup branch to readByToolCallID so the call survives a host restart by falling back to persisted parts. - agent_output: formatTranscript now mirrors formatResult's fallback chain (result_text then partial_result), so canceled_by_user rows still surface their preserved partial under detail=transcript. Schema migration: - Both tools port their parameters from z.object(...) to Schema.Struct + Schema.Literals + Schema.optional + Schema.withDecodingDefault, matching codesearch.ts / webfetch.ts. - agent_output: the XOR validation (subagent_session_id ⊕ tool_call_id) moves from Zod's .refine(...) to an imperative check at the entry of execute() — Effect Schema 4 beta does not yet expose a friendly filter combinator and the imperative form keeps the error message clear.
Astro-Han
force-pushed
the
claude/subagent-lifecycle-v1
branch
from
4a614b8 to
26fcac1
Compare
Astro-Han
commented
Apr 28, 2026
Owner
Author
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Review: subagent lifecycle v1
Overall this is a solid, well-tested PR. The scoped 8-step flow, single-writer guard, and explicit terminal states are good architectural choices. Below are issues found at various severity levels.
P0 — Must fix before merge
1. agent_output swallows ALL errors as "not found" via blanket Effect.catch
agent_output.ts lines 75-78 use .pipe(Effect.catch(() => Effect.succeed(null))) on both readByToolCallID and findLatestBySessionID. This means a storage I/O error, a JSON parse failure, or any other defect gets silently converted to "subagent not found", which is a false-negative that misleads the model. Narrow the catch to Effect.catchTag("NotFound", ...) so real failures surface as defects.
2. finalizeAfter reads the row AFTER checking wasInterrupted, creating a race
In agent.ts, finalizeAfter does const interrupted = ops.wasInterrupted(nextSession.id) first, then const current = yield* subagentRun.read(ctx.callID!). If the child runner completes naturally, then the parent aborts, then wasInterrupted returns true (because onInterrupt fired), but between that and the read, another fiber finalizes the row to completed. The current.status !== "running" guard at line 391 will skip the finalize, but the interrupted variable was already computed as true. This is benign because the early-return prevents the wrong path from running, but the logic is fragile: the read should happen first, then wasInterrupted should be consulted only if the row is still running. Reorder so read happens before wasInterrupted.
3. recordEvent sorts by at but at can collide, making sort order non-deterministic
In subagent-run.ts line 257, merged.sort((a, b) => a.at - b.at) can produce unstable ordering when two events share the same timestamp (e.g. started and failed in recordRejected both use Date.now()). Add a secondary sort key (array index) to preserve insertion order.
4. releaseSlot can underflow activeCounts without warning
subagent-run.ts line 143: if (current > 0) activeCounts.set(parentID, current - 1). If a bug ever causes releaseSlot to be called more times than reserveSlot, the count silently stays at 0 instead of going negative. An underflow would mask a logic error. Consider asserting current > 0 and dying if not, or at least logging a warning.
P1 — Should fix before merge
5. errorMessage can leak sensitive JSON payloads before sanitization
In agent.ts, errorMessage stringifies arbitrary unknowns with JSON.stringify(e). If the error is a model response containing API keys, tokens, or PII, this gets passed straight into sanitizeErrorMessage, which only strips stacks/paths/JSON envelopes — it does NOT strip the raw JSON content inside the envelope. The JSON_ENVELOPE_RE regex removes {...} blocks, but if JSON.stringify(e) produces a flat string without braces (e.g. "sensitive_token_here"), it passes through untouched. Consider redacting known sensitive patterns or truncating before JSON.stringify.
6. findLatestBySessionID does a full table scan of all parent messages
subagent-run.ts lines 383-393: collectSubtaskParts loads ALL messages in the parent session, then ALL parts of each message, then filters. For a long-running parent session with many messages, this is O(n*m) and gets worse with every subagent dispatch. Add an index or at least a message-time bound to limit the scan.
7. agent_output does not validate subagent_session_id belongs to the parent before calling findLatestBySessionID
agent_output.ts line 77 calls findLatestBySessionID(ctx.sessionID, params.subagent_session_id! as SessionID). The findLatestBySessionID implementation scans the parent's messages and filters by subagent_session_id, so it implicitly enforces parent ownership. But if a caller passes a subagent_session_id that belongs to a DIFFERENT parent's child, the scan simply returns nothing and the tool reports "not found". This is correct behavior but the error message is misleading — it says "not found or not accessible" but the real issue is cross-parent access. Consider a clearer error.
8. synthesizeOutput for failed status does not include subagent_session_id in the resume hint
In agent.ts, the failed branch at line 119 produces header = \status: failed\nerror: ...`. The resumeHintis still computed (line 84-86) but is only included whenchildIDis truthy. ForrecordRejected(cap rejection),childIDisundefinedso no resume hint appears — this is correct. But for afailedrun that DID create a child session (e.g. model error afterstart+patchSession), the resume hint IS included, yet the status header says "failed" with no guidance that the session can be resumed. Consider adding a note like "(session still resumable)" to the failed header when childID` is present.
9. MAX_ACTIVE = 5 is hardcoded with no config hook
subagent-run.ts line 94. The PR description says "caps parallel dispatch at 5 per parent" but there's no way to tune this. At minimum, document why 5 was chosen (token budget? UI clutter? memory?). Better: make it a config value with a sensible default.
10. agent_list formatElapsed shows 0s for negative or zero durations
agent-list.ts line 9: if (ms <= 0) return "0s". A negative duration means started_at is in the future (clock skew), which is a real bug. Returning "0s" silently hides the skew. Return something like "-" or "clock skew" to signal the anomaly.
P2 — Nice to have
11. SubtaskEvent discriminated union lacks a details field for extensibility
message-v2.ts: Events like tool_started have tool and label, but failed only has kind. If v1.1 wants to add retry_count, duration_ms, or stack_trace, the union shape needs to change. Add an optional details?: Record<string, unknown> to each variant now to avoid a migration later.
12. recent_events ring sorts after every append, which is O(n log n) per event
subagent-run.ts line 257 sorts the entire array after every recordEvent. With 20 items this is trivial, but if the cap ever increases, this becomes wasteful. Since events are append-only and mostly chronological, a simple push + splice at the front is O(n) and avoids the sort. Not a real perf issue at n=20, but the sort is unnecessary complexity.
13. agent_output formatTranscript truncates at 1000 chars with no indication of total length
agent-output.ts line 37: body.slice(0, 1000) + "…(truncated)". The model doesn't know how much was cut. Include the original length: …(truncated, 2345 chars total).
14. lifecycleFieldsChanged uses JSON.stringify for value comparison, which is brittle
subagent-run-context.ts line 55: JSON.stringify(existing[k]) !== JSON.stringify(next[k]). This fails for objects with different key orders ({a:1,b:2} vs {b:2,a:1} stringify differently), and for undefined values (JSON.stringify skips them). Use a deep-equal utility or at least document this limitation.
15. session.ts updatePart guard does a getPart lookup for EVERY subtask part write, even writer writes
session.ts lines 83-98: The guard checks !isWriter first, but the getPart call and lifecycleFieldsChanged computation happen inside the if (!isWriter) block — that's correct. However, the Effect.catch(() => Effect.succeed(undefined)) on getPart means a storage error during the guard check is silently swallowed, causing the guard to pass (because existing is undefined and lifecycleFieldsChanged returns false for first writes with default values). A storage error should die, not let the write through.
16. AgentTool.execute outer Effect.catch uses Effect.fail(error) which loses the original Cause
agent.ts line 466: return yield* Effect.fail(error). This reconstructs the error as a plain failure, losing any Cause metadata (stack, annotations, parallel errors). Use Effect.failCause to preserve the original cause.
P3 — Nitpicks / style
17. agent.ts makeReadLastCompletedAssistantText reads up to 5 messages but only needs 1
The function scans up to 5 messages looking for the most recent assistant text. Since messages are ordered, reading just the last 1-2 messages would suffice. The limit: 5 is arbitrary and wastes I/O.
18. subagent-run.ts collectSubtaskParts is duplicated logic between findLatestBySessionID, list, and readByToolCallID
All three methods call collectSubtaskParts which does a full parent message scan. Extract a cached/indexed approach or at least memoize within a single Effect scope.
19. agent_output.ts NOT_FOUND error is constructed eagerly on every call
Line 71 creates new Error(...) before knowing if it's needed. Move inside the if (!row) branch.
20. subagent-run.ts withExistingPart silently no-ops when the row is missing
Line 164-178: If readPart returns NotFound, the mutation is skipped silently. For finalize, setConsumed, and recordEvent, a missing row is a real bug (e.g. finalize called on a non-existent tool_call_id). Consider making the no-op behavior explicit per-mutator rather than universal.
21. agent-list.ts formatRow uses p.subagent_session_id ?? "-" but legacy rows may have undefined session ID
For legacy rows, showing "-" is fine, but the statusLabel logic at line 23 treats consumed_at as the unread marker. Legacy rows have no consumed_at, so they always show as (unread) even if they were completed long ago. Consider defaulting consumed_at for legacy decoded rows.
22. SubagentRunGuardViolation is thrown via Effect.die but carries no stack trace
session.ts line 94-95: Effect.die(new SubagentRunGuardViolation(...)). Effect.die creates a defect, but the SubagentRunGuardViolation class doesn't extend Error, so standard stack traces won't be captured. Make it extends Error.
23. agent.ts finalizeAfter has a comment saying "Do NOT also OR with ctx.abort.aborted" but the code doesn't OR it anymore
The comment at lines 378-384 is a good explanation of WHY the fix was made, but it reads like a commit message embedded in source. Trim or move to a docstring.
24. agent.ts synthesizeOutput completed_empty branch wraps an empty string with <subagent_result> tags
Line 112: body = wrapper("") produces <subagent_result>\n\n</subagent_result>. For an empty result, the model sees an empty wrapper. Consider omitting the wrapper entirely for completed_empty.
25. subagent-run.ts recordRejected uses Date.now() for both started_at and ended_at, making the run appear instantaneous
This is semantically correct (the rejection is immediate), but the recent_events array contains both started and failed at the same timestamp. The recordEvent sort will order them arbitrarily (see P0 #3). Ensure deterministic ordering.
26. agent_output.ts formatResult for failed returns error.message but sanitizeErrorMessage was already applied in synthesizeOutput
The failed branch in formatResult reads part.error?.message, which was already sanitized when finalize stored it. But agent_output is also callable by the model directly, and the model might want the raw error. The double-sanitization is fine, but there's no way to get the raw error kind+message pair. Consider whether agent_output should ever expose unsanitized errors.
27. subagent-run.ts Interface has recordActivity but it's never called in this PR
The method exists but has no callers in the diff. Either remove it or add a TODO comment explaining when v1.1 will use it.
28. agent.ts Effect.scoped + Effect.addFinalizer pattern is elegant but the inner Effect.gen at line 287 is 150+ lines deep
Consider extracting the scoped body into a named function for readability. The nesting makes it hard to trace where Effect.scoped ends.
29. message-v2.ts SubtaskPart schema has .meta({ ref: "SubtaskPart" }) but the new fields aren't reflected in any TS type-level constraints
The SubtaskPart type is inferred from Zod, but the satisfies MessageV2.SubtaskPart usage in subagent-run.ts (lines 203, 315) is a runtime assertion, not compile-time. The SubtaskPart type should be exported and used as the return type for start and recordRejected.
30. prompt.ts interruptedSessions is a Set<SessionID> but SessionID is a branded string; the Set uses reference equality
This is fine for strings, but if SessionID ever becomes an object type, the Set lookup breaks. Document that SessionID must remain a string brand.
Astro-Han
commented
Apr 28, 2026
Astro-Han
commented
Apr 28, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/opencode/src/tool/agent-output.ts (1)
95-100:⚠️ Potential issue | 🟠 MajorDon't mask child-session read defects as an access miss.
Line 99 converts every
sessions.get()cause intonull, so storage/decode failures become"subagent not found or not accessible from this parent". Only the missing-session case should collapse tonull; everything else should keep failing.Suggested fix
-import { Effect, Schema } from "effect" +import { Cause, Effect, Schema } from "effect" import * as Tool from "./tool" import { Session } from "../session" import type { SessionID } from "../session/schema" import { SubagentRun } from "../session/subagent-run" import type { MessageV2 } from "../session/message-v2" +import { NotFoundError } from "../storage/db"const child = yield* sessions .get(row.subagent_session_id as SessionID) - .pipe(Effect.catchCause(() => Effect.succeed(null))) + .pipe( + Effect.catchCause((cause) => { + const err = Cause.squash(cause) + return err instanceof NotFoundError ? Effect.succeed(null) : Effect.failCause(cause) + }), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/tool/agent-output.ts` around lines 95 - 100, The current Effect.catchCause on sessions.get(row.subagent_session_id) indiscriminately maps all causes to null, hiding real defects; change it so only the "not found" cause becomes Effect.succeed(null) while all other causes are propagated unchanged. Concretely, replace the unconditional Effect.catchCause(() => Effect.succeed(null)) with a catchCause handler that inspects the Cause (or error) returned by sessions.get and returns Effect.succeed(null) only when the cause represents a missing session (NotFoundError / missing-session case), and otherwise re-raises the original cause (e.g., by returning Effect.failCause(cause) or rethrowing) so storage/decoding failures still surface; keep the surrounding logic that checks child and child.createdByAgentTool.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/session/session.ts`:
- Around line 518-537: The guard treats newly-replayed/copied subtask parts as
illegal because it dies when existing is undefined; fix by only enforcing
lifecycleFieldsChanged/SubagentRunGuardViolation for true mutations (i.e., when
an existing part is present) or when a dedicated replay flag is not set. Update
the block that reads existing via getPart (inside the SubagentRunWriterContext
check) to skip the lifecycleFieldsChanged check and subsequent Effect.die if
existing is undefined (or if a Session.fork/replay marker context is active), so
Session.fork() replayed parts (which yield undefined existing) are not treated
as illegal mutations; reference SubagentRunWriterContext, getPart,
lifecycleFieldsChanged, SubagentRunGuardViolation, Session.fork(), and
updatePart() when making the change.
In `@packages/opencode/src/session/subagent-run.ts`:
- Around line 217-223: The patchSession updater currently overwrites
subagent_session_id unconditionally; change it so within withExistingPart (used
by patchSession) you inspect existing.subagent_session_id and: if undefined, set
it to the provided sessionID and update updated_at; if equal to the provided
sessionID, no-op and return Effect.asVoid; if different, fail fast (return an
Effect failure/error) to prevent re-pointing the row. Reference symbols:
patchSession, withExistingPart, session.updatePart, subagent_session_id, and any
callers that rely on findLatestBySessionID.
- Around line 104-108: rowLocks and partsByToolCall are keyed only by
tool_call_id but readByToolCallID shows tool_call_ids are scoped to a parent
session, so use a composite key of (parent_session_id, tool_call_id) for both
maps to avoid cross-session collisions; update the declarations for rowLocks and
partsByToolCall to use a composite key (e.g. string concat or tuple) and replace
every access site (reads/writes/ deletes) — including places that call
readByToolCallID and all mutators patchSession, recordEvent, finalize,
setConsumed — to construct and use the identical composite key, and adjust types
for Semaphore.Semaphore and the partsByToolCall value accordingly so lookups
always include the parent session id.
In `@packages/opencode/src/tool/agent-output.ts`:
- Around line 17-29: The formatResult function currently falls through for
p.status === "canceled_by_user" and returns only partial_result, losing the
terminal state; update formatResult (MessageV2.SubtaskPart handling) to detect
the "canceled_by_user" status and return a string that begins with a status
header like `status: canceled_by_user` (and optionally `latest: ...` or error
info as appropriate) followed by the existing partial_result or result_text so
callers see the explicit canceled lifecycle state while preserving the body.
In `@packages/opencode/src/tool/agent.ts`:
- Around line 41-52: PATH_RE currently only covers POSIX roots and misses
Windows paths; update the PATH_RE used by sanitizeErrorMessage so it also
matches Windows drive paths (e.g., C:\Users\alice\...) and UNC paths (e.g.,
\\server\share\path). Modify the regex for PATH_RE to include alternatives for
drive-letter backslash sequences and UNC backslash sequences (remember to escape
backslashes in the pattern), keep the global flag, and leave the rest of
sanitizeErrorMessage (STACK_RE, JSON_ENVELOPE_RE, SANITIZE_LIMIT) unchanged so
all matched Windows paths are replaced with "<path>" like POSIX paths.
- Around line 210-217: The current resume lookup swallows all failures from
sessions.get(SessionID.make(...)) and turns them into undefined, causing
storage/decoding errors to surface as "subagent_session_id not found"; change
the catch so only the genuine "not found" case is suppressed and all other
causes are rethrown. Replace the Effect.catchCause(...) around sessions.get with
a selective catcher (e.g., Effect.catchSomeCause or an equivalent) that inspects
the Cause from sessions.get and returns Effect.succeed(undefined) only when the
cause indicates a missing session/key, otherwise re-throws the original cause so
storage/decoding errors propagate instead of being treated as not-found in the
subagent_session_id handling.
---
Duplicate comments:
In `@packages/opencode/src/tool/agent-output.ts`:
- Around line 95-100: The current Effect.catchCause on
sessions.get(row.subagent_session_id) indiscriminately maps all causes to null,
hiding real defects; change it so only the "not found" cause becomes
Effect.succeed(null) while all other causes are propagated unchanged.
Concretely, replace the unconditional Effect.catchCause(() =>
Effect.succeed(null)) with a catchCause handler that inspects the Cause (or
error) returned by sessions.get and returns Effect.succeed(null) only when the
cause represents a missing session (NotFoundError / missing-session case), and
otherwise re-raises the original cause (e.g., by returning
Effect.failCause(cause) or rethrowing) so storage/decoding failures still
surface; keep the surrounding logic that checks child and
child.createdByAgentTool.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 63ca90bb-a34d-4294-9e2e-5974b1963c4f
📒 Files selected for processing (7)
packages/opencode/src/session/prompt.tspackages/opencode/src/session/session.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.tspackages/opencode/src/tool/agent-list.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/tool/agent.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
packages/opencode/**/*.ts
📄 CodeRabbit inference engine (packages/opencode/AGENTS.md)
packages/opencode/**/*.ts: UseEffect.gen(function* () { ... })for Effect composition
UseEffect.fn("Domain.method")for named/traced effects andEffect.fnUntracedfor internal helpers; these accept pipeable operators as extra arguments to avoid unnecessary outer.pipe()wrappers
UseEffect.callbackfor callback-based APIs
PreferDateTime.nowAsDateovernew Date(yield* Clock.currentTimeMillis)when you need aDatein Effect code
UseSchema.Classfor multi-field data in Effect schemas
Use branded schemas (Schema.brand) for single-value types in Effect
UseSchema.TaggedErrorClassfor typed errors in Effect schemas
UseSchema.Defectinstead ofunknownfor defect-like causes in Effect code
InEffect.gen/Effect.fn, preferyield* new MyError(...)overyield* Effect.fail(new MyError(...))for direct early-failure branches
UsemakeRuntimefromsrc/effect/run-service.tsfor all services; it returns{ runPromise, runFork, runCallback }backed by a sharedmemoMapthat deduplicates layers
UseInstanceStatefromsrc/effect/instance-state.tsfor per-directory or per-project state that needs per-instance cleanup; do work directly in theInstanceState.makeclosure whereScopedCachehandles run-once semantics
UseEffect.addFinalizerorEffect.acquireReleaseinside theInstanceState.makeclosure for cleanup (subscriptions, process teardown, etc.)
UseEffect.forkScopedinside theInstanceState.makeclosure for background stream consumers — the fiber is interrupted when the instance is disposed
PreferFileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O in Effect services
PreferChildProcessSpawner.ChildProcessSpawnerwithChildProcess.make(...)instead of custom process wrappers in Effect services
PreferHttpClient.HttpClientinstead of rawfetchin Effect services
PreferPath.Path,Config,Clock, andDateTimeservices when those concerns are already inside Effect code
For backgroun...
Files:
packages/opencode/src/session/prompt.tspackages/opencode/src/tool/agent-list.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/agent.tspackages/opencode/src/session/subagent-run.ts
🧠 Learnings (48)
📓 Common learnings
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/test/session/subagent-lifecycle-integration.test.ts:41-47
Timestamp: 2026-04-28T11:24:35.312Z
Learning: In `packages/opencode/test/session/subagent-lifecycle-integration.test.ts` (Astro-Han/pawwork, PR `#287`), the test suite intentionally uses a manual `run` helper (`Effect.runPromise(program.pipe(Effect.provide(Layer.mergeAll(SubagentRun.defaultLayer, Session.defaultLayer)), Effect.orDie))`) and raw `bun:test` test cases rather than the `testEffect(...)` harness. All 27 lifecycle tests pass with this pattern. Migration to `testEffect` + `it.live(...)` is deferred to a dedicated test-harness sweep PR to avoid fixture drift before merge. Do NOT re-flag the manual `Effect.runPromise` runner in this file as needing harness migration until that sweep PR lands.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/agent.ts:23-27
Timestamp: 2026-04-28T04:38:11.771Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/tool/agent.ts`), the `subagent_session_id` field in the `Parameters` schema accepts any `Schema.String` rather than a branded `SessionID`. This is inherited upstream behavior (adopted in PR `#270`, an upstream-sync graft of upstream PR `#23244`). The fix — validating `subagent_session_id` as a `SessionID` brand up front so malformed/typo'd IDs fail explicitly rather than silently forking a new subagent session — is intentionally deferred to a follow-up PR or upstream report. Do NOT re-flag this as a blocking issue in PR `#270` or in future upstream-sync PRs that carry the same schema; flag it only in a PawWork-authored PR that touches `agent.ts` parameter validation.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/tool/agent-list.ts:19-28
Timestamp: 2026-04-28T11:24:26.012Z
Learning: In `packages/opencode/src/tool/agent-list.ts` (Astro-Han/pawwork, PR `#287`), the `formatElapsed` function intentionally does NOT handle sub-second durations (1–999 ms) with a `<1s` branch. Sub-second elapsed times are not a real case for LLM-driven subagent dispatch because session creation alone involves multiple round trips plus a model turn. Do NOT suggest adding a `<1s` early-return to `formatElapsed` in future reviews.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 247
File: packages/ui/src/components/message-part.tsx:1322-1324
Timestamp: 2026-04-26T16:34:57.130Z
Learning: In Astro-Han/pawwork (`packages/ui/src/components/message-part.tsx`), the `taskId` createMemo and `childSessionId` createMemo both intentionally read only from `partMetadata().sessionId` (populated post-execution), not from `input.task_id` / `input.subagent_session_id`. This has always been the case — the original code never read the input field either. Adding an `input.subagent_session_id` fallback would be a new capability, not a bug fix. Do NOT flag the absence of this fallback as a regression in PR `#247` or future PRs unless there is a concrete case where metadata is not populated.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:108-169
Timestamp: 2026-04-27T10:33:12.228Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/session/prompt.ts` and `packages/opencode/src/session/processor.ts`, PR `#264`), the loop-gate race condition between `buildLoopContext()` and `recordSyntheticBlock`/`recordSyntheticStop` is intentionally handled via idempotence guards (re-check sigKey presence / `hasStopped` inside the record helpers) rather than a full per-parent `Effect.Mutex`. Threading a `Map<MessageID, Mutex>` through the processor was considered too large a surface change for this edge case; the residual TOCTOU window only produces extra synthetic parts with no behavioral drift on the "turn ends" contract. A code comment documents the trade-off and points to a full-mutex follow-up if the race is observed in practice. Do NOT re-flag the absence of a per-parent mutex as a blocking issue in future reviews.
📚 Learning: 2026-04-24T03:51:56.211Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 206
File: packages/app/e2e/prompt/prompt-footer-focus.spec.ts:131-143
Timestamp: 2026-04-24T03:51:56.211Z
Learning: In Astro-Han/pawwork E2E tests (packages/app/e2e/fixtures.ts), `project.prompt(text)` internally calls `trackSession(next.sessionID, active.directory)` after the prompt submission is observed and the active session is resolved. Tests that obtain a `sessionID` from `project.prompt()` do NOT need to call `project.trackSession(sessionID)` manually — it is already registered for teardown. Calling it again would be duplicate ownership.
Applied to files:
packages/opencode/src/session/prompt.tspackages/opencode/src/tool/agent.ts
📚 Learning: 2026-04-27T12:59:49.844Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/test/session/prompt-effect.test.ts:0-0
Timestamp: 2026-04-27T12:59:49.844Z
Learning: In `packages/opencode/test/session/prompt-effect.test.ts` and `packages/opencode/src/session/diagnostics.ts` (PR `#264`), the recovery reminder copy differs between signature kinds: the input-repeat variant says "repeated the same tool input 3 times" (uses a literal count), while the target-repeat variant says "failed against the same target multiple times" (uses "multiple times" with no count). Assertions that check for injected reminder text in LLM inputs must accept both phrasings when a scenario produces both `input:` and `target:` signatures (e.g., `read` tool with a `filePath` parameter). Do NOT narrow the assertion to only the input-variant phrasing.
Applied to files:
packages/opencode/src/session/prompt.ts
📚 Learning: 2026-04-27T10:33:08.974Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:108-169
Timestamp: 2026-04-27T10:33:08.974Z
Learning: For Astro-Han/pawwork session processing code under packages/opencode/src/session/ (e.g., prompt.ts and processor.ts), this specific loop-gate TOCTOU/race between buildLoopContext() and recordSyntheticBlock/recordSyntheticStop is intentionally handled via idempotence guards (re-check sigKey presence and hasStopped inside the record helpers). Do not treat the absence of a per-parent Effect.Mutex as a blocking review issue for this known edge case; the residual window should only create extra synthetic parts and must not change the "turn ends" contract. Respect the existing code comment documenting the trade-off, and only escalate to a full-mutex approach if the race is observed in practice.
Applied to files:
packages/opencode/src/session/prompt.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/session.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-26T16:34:57.130Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 247
File: packages/ui/src/components/message-part.tsx:1322-1324
Timestamp: 2026-04-26T16:34:57.130Z
Learning: In Astro-Han/pawwork (`packages/ui/src/components/message-part.tsx`), the `taskId` createMemo and `childSessionId` createMemo both intentionally read only from `partMetadata().sessionId` (populated post-execution), not from `input.task_id` / `input.subagent_session_id`. This has always been the case — the original code never read the input field either. Adding an `input.subagent_session_id` fallback would be a new capability, not a bug fix. Do NOT flag the absence of this fallback as a regression in PR `#247` or future PRs unless there is a concrete case where metadata is not populated.
Applied to files:
packages/opencode/src/session/prompt.tspackages/opencode/src/tool/agent-list.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/agent.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T11:24:35.312Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/test/session/subagent-lifecycle-integration.test.ts:41-47
Timestamp: 2026-04-28T11:24:35.312Z
Learning: In `packages/opencode/test/session/subagent-lifecycle-integration.test.ts` (Astro-Han/pawwork, PR `#287`), the test suite intentionally uses a manual `run` helper (`Effect.runPromise(program.pipe(Effect.provide(Layer.mergeAll(SubagentRun.defaultLayer, Session.defaultLayer)), Effect.orDie))`) and raw `bun:test` test cases rather than the `testEffect(...)` harness. All 27 lifecycle tests pass with this pattern. Migration to `testEffect` + `it.live(...)` is deferred to a dedicated test-harness sweep PR to avoid fixture drift before merge. Do NOT re-flag the manual `Effect.runPromise` runner in this file as needing harness migration until that sweep PR lands.
Applied to files:
packages/opencode/src/session/prompt.tspackages/opencode/src/tool/agent-list.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/agent.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T04:38:11.771Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/agent.ts:23-27
Timestamp: 2026-04-28T04:38:11.771Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/tool/agent.ts`), the `subagent_session_id` field in the `Parameters` schema accepts any `Schema.String` rather than a branded `SessionID`. This is inherited upstream behavior (adopted in PR `#270`, an upstream-sync graft of upstream PR `#23244`). The fix — validating `subagent_session_id` as a `SessionID` brand up front so malformed/typo'd IDs fail explicitly rather than silently forking a new subagent session — is intentionally deferred to a follow-up PR or upstream report. Do NOT re-flag this as a blocking issue in PR `#270` or in future upstream-sync PRs that carry the same schema; flag it only in a PawWork-authored PR that touches `agent.ts` parameter validation.
Applied to files:
packages/opencode/src/session/prompt.tspackages/opencode/src/tool/agent-list.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/agent.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T11:24:26.012Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/tool/agent-list.ts:19-28
Timestamp: 2026-04-28T11:24:26.012Z
Learning: In `packages/opencode/src/tool/agent-list.ts` (Astro-Han/pawwork, PR `#287`), the `formatElapsed` function intentionally does NOT handle sub-second durations (1–999 ms) with a `<1s` branch. Sub-second elapsed times are not a real case for LLM-driven subagent dispatch because session creation alone involves multiple round trips plus a model turn. Do NOT suggest adding a `<1s` early-return to `formatElapsed` in future reviews.
Applied to files:
packages/opencode/src/session/prompt.tspackages/opencode/src/tool/agent-list.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/tool/agent.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T05:36:22.128Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/session/compaction.ts:169-191
Timestamp: 2026-04-28T05:36:22.128Z
Learning: In `packages/opencode/src/session/compaction.ts` (Astro-Han/pawwork), the internal helper `splitTurn` intentionally returns a raw `Effect.gen(...)` rather than being wrapped with `Effect.fnUntraced`. Converting `splitTurn` (and similar plain-Effect.gen internal helpers in this file, e.g. `turns`, `completedCompactions`, `buildPrompt`) to `Effect.fnUntraced` is a repo-wide convention sweep deferred to a dedicated follow-up PR. The same sweep covers `tool/tool.ts` and other helpers flagged in the same sync. Do NOT re-flag the absence of `Effect.fnUntraced` on `splitTurn` or other internal helpers in `compaction.ts` during upstream-sync PRs; flag it only in the dedicated convention-sweep PR.
Applied to files:
packages/opencode/src/session/prompt.tspackages/opencode/src/tool/agent-list.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/agent.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-23T15:25:31.118Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts:5-55
Timestamp: 2026-04-23T15:25:31.118Z
Learning: In Astro-Han/pawwork E2E tests, reaching a real "running" session state requires the `project` fixture (for model bootstrap) plus `llm.wait(1)` orchestration. The bare `sdk` fixture used by `withSession` does not trigger an LLM call even with `agent: "build"` + `system` prompt set via `sdk.session.promptAsync`, so simulating running state is not lightweight in the current test infrastructure.
Applied to files:
packages/opencode/src/session/prompt.ts
📚 Learning: 2026-04-27T08:58:00.665Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:531-538
Timestamp: 2026-04-27T08:58:00.665Z
Learning: When using Effect (e.g., `yield*` with `Effect`-style generator yielding), only use `yield* new SomeErrorClass(...)` if `SomeErrorClass` extends `Schema.TaggedErrorClass` (i.e., it implements Effect’s Yieldable interface). For plain `Error` subclasses (like `BlockedLoopError` / `LoopStopError`) or inline `new Error(...)` values, they are not yieldable and must be wrapped as `yield* Effect.fail(new PlainError(...))`. Do not recommend changing `yield* Effect.fail(new SomePlainError(...))` to `yield* new SomePlainError(...)` unless the error class extends `Schema.TaggedErrorClass`.
Applied to files:
packages/opencode/src/session/prompt.tspackages/opencode/src/tool/agent-list.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/agent.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : For background loops or scheduled tasks, use `Effect.repeat` or `Effect.schedule` with `Effect.forkScoped` in the layer definition
Applied to files:
packages/opencode/src/session/prompt.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-27T10:32:59.274Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/diagnostics.ts:245-252
Timestamp: 2026-04-27T10:32:59.274Z
Learning: In pawwork’s session diagnostics/loop error handling (e.g., packages/opencode/src/session/diagnostics.ts), keep `loopLastError` stored as raw/unscrubbed error text in loop metadata. The intended trust boundary for sensitive-data scrubbing is the renderer: `LoopRenderer.render` must apply `scrubErrorText` before any user/model-facing output. Do not recommend scrubbing `loopLastError` at the `observeToolError` call site, because failed tool parts already store raw errors in `state.error` (pre-existing leak) and scrubbing only `loopLastError` would be lossy without closing the gap. If export-side hardening is required, implement a single sanitizer pass in the export layer over both `state.error` and the loop/metadata fields, rather than sanitizing only `loopLastError` earlier in the storage path.
Applied to files:
packages/opencode/src/session/prompt.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/session.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T11:24:23.345Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/session/subagent-run.ts:8-16
Timestamp: 2026-04-28T11:24:23.345Z
Learning: In this repo’s service-internal code (e.g., session modules), follow the established error-class convention: use plain classes with a `readonly _tag` discriminant and/or `NamedError.create` (zod-based) for service-internal errors. Do NOT request one-off conversions of such plain/service-internal errors to `Schema.TaggedErrorClass`; that migration is intentionally deferred until the dedicated repo-wide error-class sweep PR lands.
Applied to files:
packages/opencode/src/session/prompt.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/session.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T02:24:27.738Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 280
File: packages/opencode/src/tool/todo.ts:7-22
Timestamp: 2026-04-28T02:24:27.738Z
Learning: In `packages/opencode/src/tool/todo.ts` (Astro-Han/pawwork, PR `#280`), the `parameters` constant for `TodoWriteTool` intentionally duplicates the `Todo.Info` fields inline rather than referencing `Todo.Info` or `zodObject(Todo.Info)`. After the Effect Schema migration in this PR, `Todo.Info` is an Effect Schema, so `zodObject(Todo.Info)` returns `z.ZodObject<any>` — which erases per-field types that `Tool.define`'s `execute()` inference depends on. The inline Zod schema is a deliberate temporary measure; tool schemas will migrate to Effect Schema as a separate slice per `specs/effect/schema.md`. Do NOT suggest replacing the inline schema with `z.array(Todo.Info)` or `zodObject(Todo.Info)` until that migration lands.
Applied to files:
packages/opencode/src/tool/agent-list.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.ts
📚 Learning: 2026-04-27T10:33:12.228Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:108-169
Timestamp: 2026-04-27T10:33:12.228Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/session/prompt.ts` and `packages/opencode/src/session/processor.ts`, PR `#264`), the loop-gate race condition between `buildLoopContext()` and `recordSyntheticBlock`/`recordSyntheticStop` is intentionally handled via idempotence guards (re-check sigKey presence / `hasStopped` inside the record helpers) rather than a full per-parent `Effect.Mutex`. Threading a `Map<MessageID, Mutex>` through the processor was considered too large a surface change for this edge case; the residual TOCTOU window only produces extra synthetic parts with no behavioral drift on the "turn ends" contract. A code comment documents the trade-off and points to a full-mutex follow-up if the race is observed in practice. Do NOT re-flag the absence of a per-parent mutex as a blocking issue in future reviews.
Applied to files:
packages/opencode/src/tool/agent-list.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/tool/agent.ts
📚 Learning: 2026-04-28T11:24:23.345Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/session/subagent-run.ts:8-16
Timestamp: 2026-04-28T11:24:23.345Z
Learning: In Astro-Han/pawwork, the established pattern for service-internal errors (e.g., in `packages/opencode/src/session/subagent-run.ts`) is plain classes (with a `readonly _tag` discriminant) or `NamedError.create` (zod-based), NOT `Schema.TaggedErrorClass`. Converting individual modules to `Schema.TaggedErrorClass` one-off would diverge from the surrounding code. The migration to `Schema.TaggedErrorClass` for service-internal errors is intentionally deferred to a dedicated repo-wide error-class sweep PR. Do NOT flag plain error classes in `subagent-run.ts` or other service-internal modules as needing `Schema.TaggedErrorClass` conversion until that sweep PR lands.
Applied to files:
packages/opencode/src/tool/agent-list.tspackages/opencode/src/tool/agent-output.ts
📚 Learning: 2026-04-28T04:56:21.338Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/tool-define.test.ts:2-9
Timestamp: 2026-04-28T04:56:21.338Z
Learning: In `packages/opencode/test/tool/tool-define.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a `ManagedRuntime.make(Layer.mergeAll(Truncate.defaultLayer, Agent.defaultLayer))` wrapper and raw `bun:test` `test(...)` cases rather than the `testEffect(...)` harness. This code was adopted wholesale from upstream via the graft strategy (per `project_upstream_strategy.md`). Migrating to `testEffect` is deferred to a follow-up PR so as not to mix refactor + harness-migration intents and to avoid drifting the diff from the upstream baseline needed for clean future grafts. Do NOT re-flag the `ManagedRuntime.make` usage in this file as needing harness migration until that follow-up lands.
Applied to files:
packages/opencode/src/tool/agent-list.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/agent.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T06:47:20.342Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/question.ts:6-16
Timestamp: 2026-04-28T06:47:20.342Z
Learning: In Astro-Han/pawwork, `packages/opencode/src/tool/question.ts` is a permanent PawWork carve-out: `Question.Prompt` (defined in `packages/opencode/src/question/index.ts`) remains a Zod schema, NOT an Effect Schema. The `ts-expect-error` boundary in `tool/registry.ts` marks the Zod ↔ Effect Schema seam. The dropped `packages/opencode/test/tool/question.test.ts` covered the old Zod-mixed surface. Schema-bound regression tests for this tool are intentionally deferred until `question/index.ts` migrates to Effect Schema in its own dedicated PR. Do NOT request restoring question tool tests in upstream-sync PRs; flag it only in the PR that migrates `question/index.ts` to Effect Schema.
Applied to files:
packages/opencode/src/tool/agent-list.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/tool/agent.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T04:56:21.528Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/tool.ts:34-42
Timestamp: 2026-04-28T04:56:21.528Z
Learning: In `packages/opencode/src/tool/tool.ts` (Astro-Han/pawwork, PR `#270`), the `Def.execute` signature uses `ctx: Context` (without the `M` generic) instead of `ctx: Context<M>`, meaning `ctx.metadata(...)` is not checked against the same `M` as `ExecuteResult<M>`. This is inherited upstream behavior adopted via the graft strategy in PR `#270` (an upstream-sync PR per `project_upstream_strategy.md`). Fixing it here would mix refactor + bugfix intents and drift the diff from the upstream baseline. The fix is deferred to a dedicated follow-up PR or upstream report. Do NOT re-flag the missing `Context<M>` generic in `Def.execute` during upstream-sync PRs; only flag it in a PawWork-authored PR that directly touches `tool.ts` type signatures.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/agent.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T04:38:11.727Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/edit.ts:35-45
Timestamp: 2026-04-28T04:38:11.727Z
Learning: In `packages/opencode/src/tool/edit.ts` (Astro-Han/pawwork, PR `#270`), the module-scoped `Map<string, Semaphore.Semaphore>` (`locks`) and `lock(filePath)` helper are intentionally left as a global, never-cleaned registry. PR `#270` is a pure upstream-sync graft and mixing in a bugfix would drift the diff from the upstream baseline. The fix (moving per-file semaphores into `InstanceState`/`ScopedCache` for per-instance lifecycle management) is tracked as a follow-up to land in its own PR or be reported upstream. Do NOT re-flag the absence of InstanceState wiring for the lock map in this file until the follow-up PR is opened.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-25T12:52:49.735Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 234
File: packages/opencode/src/share/session.ts:27-27
Timestamp: 2026-04-25T12:52:49.735Z
Learning: In `packages/opencode/src/share/session.ts`, the local `ensureEnabled` closure is intentionally duplicated from `ShareRuntime.ensureEnabled` in `runtime.ts`. Using the shared `ShareRuntime.ensureEnabled` effect directly would leak `CloudShareGate` back into the `R` (requirement) type of `share`, `unshare`, and `create`, because the shared effect re-yields the service rather than capturing an already-resolved gate reference. The current pattern resolves `gate` once at the top of the outer `Effect.gen` and then closes over it in a local `ensureEnabled`, keeping the inner effects' requirement types clean. A comment in the file points to `runtime.ts` to make the intentional duplication discoverable. The real fix would be refactoring `ensureEnabled` to accept a gate parameter, but that change is larger than the DRY benefit warrants. Do not flag this duplication as a maintenance issue.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `Schema.Defect` instead of `unknown` for defect-like causes in Effect code
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/agent.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : In `Effect.gen` / `Effect.fn`, prefer `yield* new MyError(...)` over `yield* Effect.fail(new MyError(...))` for direct early-failure branches
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/agent.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-24T06:50:05.142Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 211
File: packages/opencode/src/provider/models.ts:113-179
Timestamp: 2026-04-24T06:50:05.142Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/provider/models.ts`), `PublishModel` and `PublishProvider` intentionally duplicate fields from `Model` and `Provider` with looser optionality. The split is a deliberate behavior boundary: `PublishModel`/`PublishProvider` (with `.passthrough()`) are used for lenient publish-time validation of incoming catalogs, while `Model`/`Provider` are used for strict runtime normalization. Do not flag this duplication as a maintenance burden or suggest extracting a shared base schema — the explicit separation is intentional and tied to the models-refresh reliability path introduced in PR `#211`.
Applied to files:
packages/opencode/src/session/subagent-run-context.ts
📚 Learning: 2026-04-28T06:51:54.812Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/todo.ts:9-18
Timestamp: 2026-04-28T06:51:54.812Z
Learning: In `packages/opencode/src/tool/todo.ts` (Astro-Han/pawwork), `TodoItem.status` and `TodoItem.priority` are intentionally declared as plain `Schema.String` rather than closed literal unions. This matches the upstream opencode baseline (`dev:packages/opencode/src/tool/todo.ts`). The tightening — `Schema.Literals(["pending","in_progress","completed","cancelled"])` for `status` and `Schema.Literals(["high","medium","low"])` for `priority` — is tracked as a follow-up under the harness/tool-set-v1 series (issue `#129`) to land either as part of a tool-schema tightening sweep or upstream-first. Do NOT re-flag the free-form strings for these fields in upstream-sync PRs; flag it only in a PawWork-authored PR or the dedicated sweep that touches `TodoItem` schema.
Applied to files:
packages/opencode/src/session/subagent-run-context.ts
📚 Learning: 2026-04-27T11:19:24.963Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 271
File: packages/opencode/test/tool/websearch-auth.test.ts:0-0
Timestamp: 2026-04-27T11:19:24.963Z
Learning: In `packages/opencode/test/tool/websearch-auth.test.ts` (Astro-Han/pawwork), the tests intentionally use a small local `runWith` runner with raw `bun:test` and `Effect.runPromise` rather than the `testEffect` harness. Each test case injects a custom in-memory `Auth.Service` layer; switching to `testEffect` would be style-only churn without changing risk coverage. Do not flag these tests as needing harness migration.
Applied to files:
packages/opencode/src/session/subagent-run-context.ts
📚 Learning: 2026-04-28T04:38:21.935Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/file/ripgrep.test.ts:9-10
Timestamp: 2026-04-28T04:38:21.935Z
Learning: In `packages/opencode/test/file/ripgrep.test.ts` (Astro-Han/pawwork, PR `#270`), the tests intentionally use a local `run` helper (`effect.pipe(Effect.provide(Ripgrep.defaultLayer), Effect.runPromise)`) rather than the `testEffect(...)` harness. This file was adopted from upstream wholesale as part of an upstream-sync graft; migrating to `testEffect` + `it.live(...)` was deferred to a separate follow-up PR to avoid mixing refactor and harness-migration intents within the sync. Do NOT re-flag the local `run` wrapper as needing harness migration until that follow-up lands.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.tspackages/opencode/src/tool/agent.ts
📚 Learning: 2026-04-27T08:27:43.672Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/diagnostics.ts:255-279
Timestamp: 2026-04-27T08:27:43.672Z
Learning: In `packages/opencode/src/session/diagnostics.ts` (PR `#264`, issue `#229`), target-level loop detection is intentionally tool-scoped. The `real` filter includes `r.tool === input.tool` and target signature keys are formatted as `target:${tool}:${targetHash}`. Cross-tool retries on the same URL/path/query are considered legitimate exploration (e.g., webfetch failed → switch to a different tool), NOT a stuck loop. Only same-tool + same-target accumulation counts toward block/stop. Do NOT suggest removing the tool scope from target signature keys or the `real` filter.
Applied to files:
packages/opencode/src/tool/agent-output.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `Schema.TaggedErrorClass` for typed errors in Effect schemas
Applied to files:
packages/opencode/src/tool/agent-output.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `Effect.cached` when multiple concurrent callers should share a single in-flight computation rather than storing `Fiber | undefined` or `Promise | undefined` manually
Applied to files:
packages/opencode/src/tool/agent-output.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Define `const it = testEffect(...)` near the top of the test file and keep the test body inside `Effect.gen(function* () { ... })`. Yield services directly with `yield* MyService.Service` or `yield* MyTool`.
Applied to files:
packages/opencode/src/tool/agent-output.tspackages/opencode/src/session/session.ts
📚 Learning: 2026-04-28T01:24:29.645Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 280
File: packages/opencode/src/session/projectors.ts:0-0
Timestamp: 2026-04-28T01:24:29.645Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/session/projectors.ts`), `SyncEvent.Definition.schema` preserves the literal Zod type through `z.infer<Def["schema"]>`, so `data.info` inside a `SyncEvent.project(Session.Event.Created, ...)` callback is already typed as `Session.Info` by the TypeScript compiler. No `as Session.Info` cast or `Session.Info.parse()` call is needed at this persistence boundary — a typecheck-clean removal of any such cast is sufficient.
Applied to files:
packages/opencode/src/session/session.ts
📚 Learning: 2026-04-28T03:46:07.505Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 280
File: packages/opencode/src/config/config.ts:148-155
Timestamp: 2026-04-28T03:46:07.505Z
Learning: In `packages/opencode/src/config/config.ts` (Astro-Han/pawwork), `Config.Info` is an Effect Schema with a `.zod` accessor derived via `withStatics`. There are zero callers of `Config.Info.parse`, `Config.Info.safeParse`, `Config.parse`, or `Config.safeParse` in the codebase. The `.zod` accessor pattern (`Config.Info.zod`) is the documented PawWork convention for all Hono validators and Zod-style decoders. Do NOT suggest adding `.parse`/`.safeParse` compatibility shims to `Config.Info` — they would be dead surface area with no consumer.
Applied to files:
packages/opencode/src/session/session.ts
📚 Learning: 2026-04-25T12:52:35.631Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 234
File: packages/desktop-electron/src/main/ipc.ts:238-263
Timestamp: 2026-04-25T12:52:35.631Z
Learning: In Astro-Han/pawwork (`packages/desktop-electron/src/main/ipc.ts`), `deps.getServerReadyData()` (backed by `serverReady.promise` in `index.ts`) resolves once at server startup and remains settled; it is not expected to reject in practice. Do not flag the absence of a try-catch around it in the `export-session` IPC handler — the network/fetch layer in `server-client.ts` already has a 10-second AbortController timeout and returns a typed `{ok: false, error}` payload, covering the real failure modes.
Applied to files:
packages/opencode/src/session/session.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-27T11:18:47.332Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 271
File: packages/opencode/test/tool/mcp-exa.test.ts:1-186
Timestamp: 2026-04-27T11:18:47.332Z
Learning: In `packages/opencode/test/tool/mcp-exa.test.ts` (Astro-Han/pawwork), the tests intentionally use raw `bun:test` async cases with `Effect.runPromise(...)` and per-case `HttpClient.make(...)` fakes rather than the `testEffect(...)` harness. The maintainer has explicitly decided not to migrate, because the HttpClient fake wiring is itself the behavior under test and switching to `testEffect` would be style churn without changing risk coverage. Do not flag these tests as needing harness migration.
Applied to files:
packages/opencode/src/session/session.tspackages/opencode/src/tool/agent.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `Effect.forkScoped` inside the `InstanceState.make` closure for background stream consumers — the fiber is interrupted when the instance is disposed
Applied to files:
packages/opencode/src/session/session.ts
📚 Learning: 2026-04-27T11:18:47.596Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 271
File: packages/opencode/test/agent/agent.test.ts:440-447
Timestamp: 2026-04-27T11:18:47.596Z
Learning: In `packages/opencode/test/agent/agent.test.ts` (Astro-Han/pawwork), all agent-permission tests intentionally use the manual `tmpdir()` + `Instance.provide(...)` pattern. Do not flag individual tests in this file for conversion to `provideTmpdirInstance(...)` or `provideInstance(...)`; a full harness migration would be a separate PR if the pattern ever needs to change.
Applied to files:
packages/opencode/src/tool/agent.ts
📚 Learning: 2026-04-25T12:52:39.403Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 234
File: packages/opencode/src/session/export.ts:75-88
Timestamp: 2026-04-25T12:52:39.403Z
Learning: In `packages/opencode/src/session/export.ts` (Astro-Han/pawwork), the `extractReasonFromCause` function intentionally uses a lightweight cast-based inspection of the Cause `reasons` array instead of `Cause.failures()` / `Cause.defects()` utilities. This is a deliberate choice: the function is diagnostic-only (used to populate a best-effort reason string for export diagnostics), and the maintainer explicitly prefers not to add coupling to Effect's Cause API surface for this purpose. Do not flag this pattern as fragile or suggest replacing it with Cause utilities.
Applied to files:
packages/opencode/src/tool/agent.ts
📚 Learning: 2026-04-28T07:27:59.666Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/file/ripgrep.ts:364-368
Timestamp: 2026-04-28T07:27:59.666Z
Learning: In `packages/opencode/src/file/ripgrep.ts` (Astro-Han/pawwork, PR `#270`), both `files()` and `search()` do not short-circuit on an already-aborted `AbortSignal` before `spawner.spawn(...)`. The `check(input.cwd)` effect before spawn already provides a natural cancellation point for interrupted fibers. Upstream `dev:packages/opencode/src/file/ripgrep.ts` has identical ordering with no pre-spawn `signal.aborted` guard; adding one would diverge from the graft baseline. The fix (explicit pre-spawn `signal.aborted` check) is deferred to a dedicated PawWork ripgrep cancellation pass or upstream PR. Do NOT re-flag the missing pre-spawn cancellation short-circuit in `files()`/`search()` in upstream-sync PRs.
Applied to files:
packages/opencode/src/tool/agent.ts
📚 Learning: 2026-04-27T10:33:02.677Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/diagnostics.ts:245-252
Timestamp: 2026-04-27T10:33:02.677Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/session/diagnostics.ts`, PR `#264`), `loopLastError` is stored as raw (unscrubbed) error text in loop metadata intentionally. The trust boundary for sensitive data scrubbing is the renderer (`LoopRenderer.render` already applies `scrubErrorText` before any user/model-facing output), not the storage layer. Failed tool parts also already store raw errors in `state.error` (PR `#204` default behavior), so scrubbing only `loopLastError` would not close that pre-existing leak and would add lossy storage. If export-side hardening is needed, the correct approach is a single sanitizer pass over both `state.error` and metadata in the export layer. Do NOT suggest scrubbing `loopLastError` at the `observeToolError` call site.
Applied to files:
packages/opencode/src/tool/agent.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `Effect.fn("Domain.method")` for named/traced effects and `Effect.fnUntraced` for internal helpers; these accept pipeable operators as extra arguments to avoid unnecessary outer `.pipe()` wrappers
Applied to files:
packages/opencode/src/tool/agent.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `makeRuntime` from `src/effect/run-service.ts` for all services; it returns `{ runPromise, runFork, runCallback }` backed by a shared `memoMap` that deduplicates layers
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `Schema.Class` for multi-field data in Effect schemas
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T07:27:41.971Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/file/ripgrep.ts:29-118
Timestamp: 2026-04-28T07:27:41.971Z
Learning: In `packages/opencode/src/file/ripgrep.ts` (Astro-Han/pawwork), the internal Effect schemas `TimeStats`, `Stats`, `PathText`, `Begin`, `End`, `Summary`, and the exported `SearchMatch`/`Match`/`Result` union are all declared as `Schema.Struct` — matching upstream `dev:packages/opencode/src/file/ripgrep.ts` lines 60-118. PR `#270` is an upstream-sync graft per `project_upstream_strategy.md`. Converting these to `Schema.Class` would diverge from the upstream baseline and create recurring merge friction on future syncs. The `Schema.Class` convention applies to PawWork-authored schemas, not to upstream-grafted shapes in this file. Do NOT re-flag the use of `Schema.Struct` for these schemas in upstream-sync PRs; flag it only in a PR that simultaneously converts the upstream side, or in a dedicated PawWork carve-out PR for ripgrep schemas.
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use branded schemas (`Schema.brand`) for single-value types in Effect
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `Effect.addFinalizer` or `Effect.acquireRelease` inside the `InstanceState.make` closure for cleanup (subscriptions, process teardown, etc.)
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-27T11:18:47.298Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 271
File: packages/opencode/test/tool/websearch.test.ts:21-78
Timestamp: 2026-04-27T11:18:47.298Z
Learning: In `packages/opencode/test/tool/websearch.test.ts`, the tests intentionally use manual `Effect.runPromise` with explicit `Effect.provide(...)` chains (including `Layer.succeed(Auth.Service, ...)`, `Layer.succeed(HttpClient.HttpClient, http)`, `WebSearchAuth.layer`, `Truncate.defaultLayer`, and `Agent.defaultLayer`) rather than the `testEffect(...)` harness. This is by design: the fake Auth and HTTP recovery-metadata layers must be explicitly injected and kept visible/scoped at the test site. Do NOT suggest migrating these tests to `testEffect` or removing the manual layer provides.
Applied to files:
packages/opencode/src/session/subagent-run.ts
The lifecycle guard policed the very first persistence of a SubtaskPart, which blocked Session.fork() — fork replays historical parts via updatePart() outside any SubagentRunWriterContext, and a forked session that contained a completed/failed/canceled subagent now died on SubagentRunGuardViolation. Trust on creation: only mutations of an existing row are policed. lifecycleFieldsChanged loses its first-write branch (callers no longer pass undefined). New regression test asserts a non-writer can persist a SubtaskPart with full lifecycle values when no row exists yet.
…lt mode formatResult fell through to the generic body for canceled_by_user rows, returning only partial_result. The model could not distinguish a cancellation from a normal completion. Canceled rows now lead with "status: canceled_by_user" and surface the partial body (or "(no partial output)") so the lifecycle state is explicit.
- PATH_RE now also covers Windows drive paths (C:\Users\...) and UNC paths (\\server\share\...) so error.message persisted in SubtaskPart.error never leaks usernames or local layout on Windows. - Resume lookup uses Cause.squash to swallow only NotFoundError; other defects (storage I/O, schema decode) re-raise via failCause instead of being masked as "subagent_session_id not found". - Export sanitizeErrorMessage and add invariant-style tests so any future leak vector (including Cygwin / WSL / new envelopes) trips the suite. Assertions verify the output never contains the original sensitive substrings, not the literal replacement format.
Re-pointing subagent_session_id on a row that already names a different child would corrupt findLatestBySessionID and ownership-based lookups. patchSession now no-ops when the same id is provided again and dies when a different id is already stored, so any retry / re-entrant path that tries to overwrite the binding surfaces as a defect instead of silently mutating ownership.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/session/subagent-run-context.ts`:
- Around line 13-25: The LIFECYCLE_KEYS constant is missing immutable subtask
linkage fields, allowing unauthorized rewrites via Session.updatePart(...);
update the LIFECYCLE_KEYS array to include "tool_call_id", "parent_session_id",
"parent_message_id", and "subagent_session_id" so lifecycleFieldsChanged() will
guard those fields too and prevent repointing child/parent sessions.
In `@packages/opencode/src/session/subagent-run.ts`:
- Around line 391-400: findLatestBySessionID currently scans persisted history
via collectSubtaskParts and returns a row but does not repopulate the in-memory
index partsByToolCall, so later setConsumed(row.tool_call_id) /
withExistingPart() can't find it after a restart; modify findLatestBySessionID
so that before returning matches[0] it inserts that row into partsByToolCall
keyed by row.tool_call_id (the same key used by withExistingPart/setConsumed) so
the in-memory lookup is repopulated for subsequent consumed-marking logic.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 5374501e-239a-4b7a-b2c4-c7898322da7b
📒 Files selected for processing (7)
packages/opencode/src/session/session.tspackages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.tspackages/opencode/src/tool/agent-output.tspackages/opencode/src/tool/agent.tspackages/opencode/test/session/subagent-run.test.tspackages/opencode/test/tool/agent.test.ts
LIFECYCLE_KEYS now also covers tool_call_id, parent_session_id, parent_message_id, and subagent_session_id. Without these, a non-writer path that already holds an existing SubtaskPart could re-point the row to a different child or parent via Session.updatePart(), bypassing the single-writer guarantee. SubagentRun's own writers stay unaffected: start() / recordRejected() set the fields on first write (always allowed) and patchSession() runs inside SubagentRunWriterContext.
After a host restart, agent_output can still locate a row by subagent_session_id via the persisted-part scan, but subsequent mutators (setConsumed, recordEvent, finalize) hit withExistingPart's NotFound no-op because partsByToolCall has no entry. The advertised "consumed on first read" behavior therefore silently failed in the subagent_session_id lookup mode. findLatestBySessionID now repopulates partsByToolCall before returning, mirroring readByToolCallID's fallback.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/session/subagent-run.ts`:
- Around line 21-31: StartInput and RejectedInput include a dead field
subagent_type that neither SubagentRun.start() nor SubagentRun.recordRejected()
use; remove subagent_type from the DTOs (StartInput, RejectedInput) so the type
no longer implies this service performs session tagging, or alternatively
implement the missing tagging write inside SubagentRun (add logic in start() and
recordRejected() to persist the subagent_type to the session/tag store) so the
API responsibility matches behavior; update any code referencing
StartInput.subagent_type or RejectedInput.subagent_type accordingly (search for
StartInput, RejectedInput, SubagentRun, start, recordRejected) and adjust
tests/types to reflect the removal or the new persistence behavior.
- Line 447: Remove the redundant namespace self-reexport statement `export * as
SubagentRun from "./subagent-run"` from the module; simply delete that line (the
module "./subagent-run" already exports the needed symbols and all consumers
import SubagentRun directly), then run the build/tests to ensure no code
references the namespace re-export remain.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 6f81713a-7c47-4f1e-9b5a-745dd84bbe99
📒 Files selected for processing (2)
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
packages/opencode/**/*.ts
📄 CodeRabbit inference engine (packages/opencode/AGENTS.md)
packages/opencode/**/*.ts: UseEffect.gen(function* () { ... })for Effect composition
UseEffect.fn("Domain.method")for named/traced effects andEffect.fnUntracedfor internal helpers; these accept pipeable operators as extra arguments to avoid unnecessary outer.pipe()wrappers
UseEffect.callbackfor callback-based APIs
PreferDateTime.nowAsDateovernew Date(yield* Clock.currentTimeMillis)when you need aDatein Effect code
UseSchema.Classfor multi-field data in Effect schemas
Use branded schemas (Schema.brand) for single-value types in Effect
UseSchema.TaggedErrorClassfor typed errors in Effect schemas
UseSchema.Defectinstead ofunknownfor defect-like causes in Effect code
InEffect.gen/Effect.fn, preferyield* new MyError(...)overyield* Effect.fail(new MyError(...))for direct early-failure branches
UsemakeRuntimefromsrc/effect/run-service.tsfor all services; it returns{ runPromise, runFork, runCallback }backed by a sharedmemoMapthat deduplicates layers
UseInstanceStatefromsrc/effect/instance-state.tsfor per-directory or per-project state that needs per-instance cleanup; do work directly in theInstanceState.makeclosure whereScopedCachehandles run-once semantics
UseEffect.addFinalizerorEffect.acquireReleaseinside theInstanceState.makeclosure for cleanup (subscriptions, process teardown, etc.)
UseEffect.forkScopedinside theInstanceState.makeclosure for background stream consumers — the fiber is interrupted when the instance is disposed
PreferFileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O in Effect services
PreferChildProcessSpawner.ChildProcessSpawnerwithChildProcess.make(...)instead of custom process wrappers in Effect services
PreferHttpClient.HttpClientinstead of rawfetchin Effect services
PreferPath.Path,Config,Clock, andDateTimeservices when those concerns are already inside Effect code
For backgroun...
Files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
🧠 Learnings (31)
📓 Common learnings
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/agent.ts:23-27
Timestamp: 2026-04-28T04:38:11.771Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/tool/agent.ts`), the `subagent_session_id` field in the `Parameters` schema accepts any `Schema.String` rather than a branded `SessionID`. This is inherited upstream behavior (adopted in PR `#270`, an upstream-sync graft of upstream PR `#23244`). The fix — validating `subagent_session_id` as a `SessionID` brand up front so malformed/typo'd IDs fail explicitly rather than silently forking a new subagent session — is intentionally deferred to a follow-up PR or upstream report. Do NOT re-flag this as a blocking issue in PR `#270` or in future upstream-sync PRs that carry the same schema; flag it only in a PawWork-authored PR that touches `agent.ts` parameter validation.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/test/session/subagent-lifecycle-integration.test.ts:41-47
Timestamp: 2026-04-28T11:24:35.312Z
Learning: In `packages/opencode/test/session/subagent-lifecycle-integration.test.ts` (Astro-Han/pawwork, PR `#287`), the test suite intentionally uses a manual `run` helper (`Effect.runPromise(program.pipe(Effect.provide(Layer.mergeAll(SubagentRun.defaultLayer, Session.defaultLayer)), Effect.orDie))`) and raw `bun:test` test cases rather than the `testEffect(...)` harness. All 27 lifecycle tests pass with this pattern. Migration to `testEffect` + `it.live(...)` is deferred to a dedicated test-harness sweep PR to avoid fixture drift before merge. Do NOT re-flag the manual `Effect.runPromise` runner in this file as needing harness migration until that sweep PR lands.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/session/session.ts:518-542
Timestamp: 2026-04-28T12:01:16.559Z
Learning: In `packages/opencode/src/session/session.ts` (Astro-Han/pawwork, PR `#287`), the `updatePart` subtask guard intentionally allows first writes (where `existing === undefined`) to pass through without calling `lifecycleFieldsChanged`. This is required for `Session.fork()`, migration, and import paths that replay historical `SubtaskPart` rows via `updatePart()` outside any `SubagentRunWriterContext`. Only mutations of an already-persisted row (`existing` is defined) are policed. Do NOT suggest adding a lifecycle check on first-write in this guard.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/session/subagent-run.ts:104-108
Timestamp: 2026-04-28T12:01:13.981Z
Learning: In `packages/opencode/src/session/subagent-run.ts` (Astro-Han/pawwork, PR `#287`), `rowLocks` and `partsByToolCall` are intentionally keyed by `tool_call_id` alone (not a composite `(parent_session_id, tool_call_id)` key). `tool_call_id` is AI-SDK-generated as `call_<nanoid>`, making it globally unique across sessions in practice. `readByToolCallID(parentID, toolCallID)` takes `parentID` solely as an access-control/ownership guard, not because IDs repeat across parents. Composite-keying would add unnecessary API surface per YAGNI for v1; revisit only if AI SDK moves off nanoid-based call IDs. Do NOT re-flag the single-key maps in this file.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/tool/agent-list.ts:19-28
Timestamp: 2026-04-28T11:24:26.012Z
Learning: In `packages/opencode/src/tool/agent-list.ts` (Astro-Han/pawwork, PR `#287`), the `formatElapsed` function intentionally does NOT handle sub-second durations (1–999 ms) with a `<1s` branch. Sub-second elapsed times are not a real case for LLM-driven subagent dispatch because session creation alone involves multiple round trips plus a model turn. Do NOT suggest adding a `<1s` early-return to `formatElapsed` in future reviews.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 247
File: packages/ui/src/components/message-part.tsx:1322-1324
Timestamp: 2026-04-26T16:34:57.130Z
Learning: In Astro-Han/pawwork (`packages/ui/src/components/message-part.tsx`), the `taskId` createMemo and `childSessionId` createMemo both intentionally read only from `partMetadata().sessionId` (populated post-execution), not from `input.task_id` / `input.subagent_session_id`. This has always been the case — the original code never read the input field either. Adding an `input.subagent_session_id` fallback would be a new capability, not a bug fix. Do NOT flag the absence of this fallback as a regression in PR `#247` or future PRs unless there is a concrete case where metadata is not populated.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:108-169
Timestamp: 2026-04-27T10:33:12.228Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/session/prompt.ts` and `packages/opencode/src/session/processor.ts`, PR `#264`), the loop-gate race condition between `buildLoopContext()` and `recordSyntheticBlock`/`recordSyntheticStop` is intentionally handled via idempotence guards (re-check sigKey presence / `hasStopped` inside the record helpers) rather than a full per-parent `Effect.Mutex`. Threading a `Map<MessageID, Mutex>` through the processor was considered too large a surface change for this edge case; the residual TOCTOU window only produces extra synthetic parts with no behavioral drift on the "turn ends" contract. A code comment documents the trade-off and points to a full-mutex follow-up if the race is observed in practice. Do NOT re-flag the absence of a per-parent mutex as a blocking issue in future reviews.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts:5-55
Timestamp: 2026-04-23T15:25:31.118Z
Learning: In Astro-Han/pawwork E2E tests, reaching a real "running" session state requires the `project` fixture (for model bootstrap) plus `llm.wait(1)` orchestration. The bare `sdk` fixture used by `withSession` does not trigger an LLM call even with `agent: "build"` + `system` prompt set via `sdk.session.promptAsync`, so simulating running state is not lightweight in the current test infrastructure.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/session/subagent-run.ts:8-16
Timestamp: 2026-04-28T11:24:23.345Z
Learning: In Astro-Han/pawwork, the established pattern for service-internal errors (e.g., in `packages/opencode/src/session/subagent-run.ts`) is plain classes (with a `readonly _tag` discriminant) or `NamedError.create` (zod-based), NOT `Schema.TaggedErrorClass`. Converting individual modules to `Schema.TaggedErrorClass` one-off would diverge from the surrounding code. The migration to `Schema.TaggedErrorClass` for service-internal errors is intentionally deferred to a dedicated repo-wide error-class sweep PR. Do NOT flag plain error classes in `subagent-run.ts` or other service-internal modules as needing `Schema.TaggedErrorClass` conversion until that sweep PR lands.
📚 Learning: 2026-04-28T12:01:16.559Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/session/session.ts:518-542
Timestamp: 2026-04-28T12:01:16.559Z
Learning: In `packages/opencode/src/session/session.ts` (Astro-Han/pawwork, PR `#287`), the `updatePart` subtask guard intentionally allows first writes (where `existing === undefined`) to pass through without calling `lifecycleFieldsChanged`. This is required for `Session.fork()`, migration, and import paths that replay historical `SubtaskPart` rows via `updatePart()` outside any `SubagentRunWriterContext`. Only mutations of an already-persisted row (`existing` is defined) are policed. Do NOT suggest adding a lifecycle check on first-write in this guard.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T11:24:35.312Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/test/session/subagent-lifecycle-integration.test.ts:41-47
Timestamp: 2026-04-28T11:24:35.312Z
Learning: In `packages/opencode/test/session/subagent-lifecycle-integration.test.ts` (Astro-Han/pawwork, PR `#287`), the test suite intentionally uses a manual `run` helper (`Effect.runPromise(program.pipe(Effect.provide(Layer.mergeAll(SubagentRun.defaultLayer, Session.defaultLayer)), Effect.orDie))`) and raw `bun:test` test cases rather than the `testEffect(...)` harness. All 27 lifecycle tests pass with this pattern. Migration to `testEffect` + `it.live(...)` is deferred to a dedicated test-harness sweep PR to avoid fixture drift before merge. Do NOT re-flag the manual `Effect.runPromise` runner in this file as needing harness migration until that sweep PR lands.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T04:38:11.771Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/agent.ts:23-27
Timestamp: 2026-04-28T04:38:11.771Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/tool/agent.ts`), the `subagent_session_id` field in the `Parameters` schema accepts any `Schema.String` rather than a branded `SessionID`. This is inherited upstream behavior (adopted in PR `#270`, an upstream-sync graft of upstream PR `#23244`). The fix — validating `subagent_session_id` as a `SessionID` brand up front so malformed/typo'd IDs fail explicitly rather than silently forking a new subagent session — is intentionally deferred to a follow-up PR or upstream report. Do NOT re-flag this as a blocking issue in PR `#270` or in future upstream-sync PRs that carry the same schema; flag it only in a PawWork-authored PR that touches `agent.ts` parameter validation.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T12:01:13.981Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/session/subagent-run.ts:104-108
Timestamp: 2026-04-28T12:01:13.981Z
Learning: In `packages/opencode/src/session/subagent-run.ts` (Astro-Han/pawwork, PR `#287`), `rowLocks` and `partsByToolCall` are intentionally keyed by `tool_call_id` alone (not a composite `(parent_session_id, tool_call_id)` key). `tool_call_id` is AI-SDK-generated as `call_<nanoid>`, making it globally unique across sessions in practice. `readByToolCallID(parentID, toolCallID)` takes `parentID` solely as an access-control/ownership guard, not because IDs repeat across parents. Composite-keying would add unnecessary API surface per YAGNI for v1; revisit only if AI SDK moves off nanoid-based call IDs. Do NOT re-flag the single-key maps in this file.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T11:24:23.345Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/session/subagent-run.ts:8-16
Timestamp: 2026-04-28T11:24:23.345Z
Learning: In this repo’s service-internal code (e.g., session modules), follow the established error-class convention: use plain classes with a `readonly _tag` discriminant and/or `NamedError.create` (zod-based) for service-internal errors. Do NOT request one-off conversions of such plain/service-internal errors to `Schema.TaggedErrorClass`; that migration is intentionally deferred until the dedicated repo-wide error-class sweep PR lands.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-27T10:33:08.974Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:108-169
Timestamp: 2026-04-27T10:33:08.974Z
Learning: For Astro-Han/pawwork session processing code under packages/opencode/src/session/ (e.g., prompt.ts and processor.ts), this specific loop-gate TOCTOU/race between buildLoopContext() and recordSyntheticBlock/recordSyntheticStop is intentionally handled via idempotence guards (re-check sigKey presence and hasStopped inside the record helpers). Do not treat the absence of a per-parent Effect.Mutex as a blocking review issue for this known edge case; the residual window should only create extra synthetic parts and must not change the "turn ends" contract. Respect the existing code comment documenting the trade-off, and only escalate to a full-mutex approach if the race is observed in practice.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T11:24:26.012Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/tool/agent-list.ts:19-28
Timestamp: 2026-04-28T11:24:26.012Z
Learning: In `packages/opencode/src/tool/agent-list.ts` (Astro-Han/pawwork, PR `#287`), the `formatElapsed` function intentionally does NOT handle sub-second durations (1–999 ms) with a `<1s` branch. Sub-second elapsed times are not a real case for LLM-driven subagent dispatch because session creation alone involves multiple round trips plus a model turn. Do NOT suggest adding a `<1s` early-return to `formatElapsed` in future reviews.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-25T12:52:49.735Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 234
File: packages/opencode/src/share/session.ts:27-27
Timestamp: 2026-04-25T12:52:49.735Z
Learning: In `packages/opencode/src/share/session.ts`, the local `ensureEnabled` closure is intentionally duplicated from `ShareRuntime.ensureEnabled` in `runtime.ts`. Using the shared `ShareRuntime.ensureEnabled` effect directly would leak `CloudShareGate` back into the `R` (requirement) type of `share`, `unshare`, and `create`, because the shared effect re-yields the service rather than capturing an already-resolved gate reference. The current pattern resolves `gate` once at the top of the outer `Effect.gen` and then closes over it in a local `ensureEnabled`, keeping the inner effects' requirement types clean. A comment in the file points to `runtime.ts` to make the intentional duplication discoverable. The real fix would be refactoring `ensureEnabled` to accept a gate parameter, but that change is larger than the DRY benefit warrants. Do not flag this duplication as a maintenance issue.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T04:38:11.727Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/edit.ts:35-45
Timestamp: 2026-04-28T04:38:11.727Z
Learning: In `packages/opencode/src/tool/edit.ts` (Astro-Han/pawwork, PR `#270`), the module-scoped `Map<string, Semaphore.Semaphore>` (`locks`) and `lock(filePath)` helper are intentionally left as a global, never-cleaned registry. PR `#270` is a pure upstream-sync graft and mixing in a bugfix would drift the diff from the upstream baseline. The fix (moving per-file semaphores into `InstanceState`/`ScopedCache` for per-instance lifecycle management) is tracked as a follow-up to land in its own PR or be reported upstream. Do NOT re-flag the absence of InstanceState wiring for the lock map in this file until the follow-up PR is opened.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-26T16:34:57.130Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 247
File: packages/ui/src/components/message-part.tsx:1322-1324
Timestamp: 2026-04-26T16:34:57.130Z
Learning: In Astro-Han/pawwork (`packages/ui/src/components/message-part.tsx`), the `taskId` createMemo and `childSessionId` createMemo both intentionally read only from `partMetadata().sessionId` (populated post-execution), not from `input.task_id` / `input.subagent_session_id`. This has always been the case — the original code never read the input field either. Adding an `input.subagent_session_id` fallback would be a new capability, not a bug fix. Do NOT flag the absence of this fallback as a regression in PR `#247` or future PRs unless there is a concrete case where metadata is not populated.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-27T08:58:00.665Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:531-538
Timestamp: 2026-04-27T08:58:00.665Z
Learning: When using Effect (e.g., `yield*` with `Effect`-style generator yielding), only use `yield* new SomeErrorClass(...)` if `SomeErrorClass` extends `Schema.TaggedErrorClass` (i.e., it implements Effect’s Yieldable interface). For plain `Error` subclasses (like `BlockedLoopError` / `LoopStopError`) or inline `new Error(...)` values, they are not yieldable and must be wrapped as `yield* Effect.fail(new PlainError(...))`. Do not recommend changing `yield* Effect.fail(new SomePlainError(...))` to `yield* new SomePlainError(...)` unless the error class extends `Schema.TaggedErrorClass`.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `Schema.Defect` instead of `unknown` for defect-like causes in Effect code
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : In `Effect.gen` / `Effect.fn`, prefer `yield* new MyError(...)` over `yield* Effect.fail(new MyError(...))` for direct early-failure branches
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-24T06:50:05.142Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 211
File: packages/opencode/src/provider/models.ts:113-179
Timestamp: 2026-04-24T06:50:05.142Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/provider/models.ts`), `PublishModel` and `PublishProvider` intentionally duplicate fields from `Model` and `Provider` with looser optionality. The split is a deliberate behavior boundary: `PublishModel`/`PublishProvider` (with `.passthrough()`) are used for lenient publish-time validation of incoming catalogs, while `Model`/`Provider` are used for strict runtime normalization. Do not flag this duplication as a maintenance burden or suggest extracting a shared base schema — the explicit separation is intentional and tied to the models-refresh reliability path introduced in PR `#211`.
Applied to files:
packages/opencode/src/session/subagent-run-context.ts
📚 Learning: 2026-04-28T05:36:22.128Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/session/compaction.ts:169-191
Timestamp: 2026-04-28T05:36:22.128Z
Learning: In `packages/opencode/src/session/compaction.ts` (Astro-Han/pawwork), the internal helper `splitTurn` intentionally returns a raw `Effect.gen(...)` rather than being wrapped with `Effect.fnUntraced`. Converting `splitTurn` (and similar plain-Effect.gen internal helpers in this file, e.g. `turns`, `completedCompactions`, `buildPrompt`) to `Effect.fnUntraced` is a repo-wide convention sweep deferred to a dedicated follow-up PR. The same sweep covers `tool/tool.ts` and other helpers flagged in the same sync. Do NOT re-flag the absence of `Effect.fnUntraced` on `splitTurn` or other internal helpers in `compaction.ts` during upstream-sync PRs; flag it only in the dedicated convention-sweep PR.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T06:51:54.812Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/todo.ts:9-18
Timestamp: 2026-04-28T06:51:54.812Z
Learning: In `packages/opencode/src/tool/todo.ts` (Astro-Han/pawwork), `TodoItem.status` and `TodoItem.priority` are intentionally declared as plain `Schema.String` rather than closed literal unions. This matches the upstream opencode baseline (`dev:packages/opencode/src/tool/todo.ts`). The tightening — `Schema.Literals(["pending","in_progress","completed","cancelled"])` for `status` and `Schema.Literals(["high","medium","low"])` for `priority` — is tracked as a follow-up under the harness/tool-set-v1 series (issue `#129`) to land either as part of a tool-schema tightening sweep or upstream-first. Do NOT re-flag the free-form strings for these fields in upstream-sync PRs; flag it only in a PawWork-authored PR or the dedicated sweep that touches `TodoItem` schema.
Applied to files:
packages/opencode/src/session/subagent-run-context.ts
📚 Learning: 2026-04-28T04:56:21.528Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/tool.ts:34-42
Timestamp: 2026-04-28T04:56:21.528Z
Learning: In `packages/opencode/src/tool/tool.ts` (Astro-Han/pawwork, PR `#270`), the `Def.execute` signature uses `ctx: Context` (without the `M` generic) instead of `ctx: Context<M>`, meaning `ctx.metadata(...)` is not checked against the same `M` as `ExecuteResult<M>`. This is inherited upstream behavior adopted via the graft strategy in PR `#270` (an upstream-sync PR per `project_upstream_strategy.md`). Fixing it here would mix refactor + bugfix intents and drift the diff from the upstream baseline. The fix is deferred to a dedicated follow-up PR or upstream report. Do NOT re-flag the missing `Context<M>` generic in `Def.execute` during upstream-sync PRs; only flag it in a PawWork-authored PR that directly touches `tool.ts` type signatures.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-27T08:27:43.672Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/diagnostics.ts:255-279
Timestamp: 2026-04-27T08:27:43.672Z
Learning: In `packages/opencode/src/session/diagnostics.ts` (PR `#264`, issue `#229`), target-level loop detection is intentionally tool-scoped. The `real` filter includes `r.tool === input.tool` and target signature keys are formatted as `target:${tool}:${targetHash}`. Cross-tool retries on the same URL/path/query are considered legitimate exploration (e.g., webfetch failed → switch to a different tool), NOT a stuck loop. Only same-tool + same-target accumulation counts toward block/stop. Do NOT suggest removing the tool scope from target signature keys or the `real` filter.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-27T10:32:59.274Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/diagnostics.ts:245-252
Timestamp: 2026-04-27T10:32:59.274Z
Learning: In pawwork’s session diagnostics/loop error handling (e.g., packages/opencode/src/session/diagnostics.ts), keep `loopLastError` stored as raw/unscrubbed error text in loop metadata. The intended trust boundary for sensitive-data scrubbing is the renderer: `LoopRenderer.render` must apply `scrubErrorText` before any user/model-facing output. Do not recommend scrubbing `loopLastError` at the `observeToolError` call site, because failed tool parts already store raw errors in `state.error` (pre-existing leak) and scrubbing only `loopLastError` would be lossy without closing the gap. If export-side hardening is required, implement a single sanitizer pass in the export layer over both `state.error` and the loop/metadata fields, rather than sanitizing only `loopLastError` earlier in the storage path.
Applied to files:
packages/opencode/src/session/subagent-run-context.tspackages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T04:56:21.338Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/tool-define.test.ts:2-9
Timestamp: 2026-04-28T04:56:21.338Z
Learning: In `packages/opencode/test/tool/tool-define.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a `ManagedRuntime.make(Layer.mergeAll(Truncate.defaultLayer, Agent.defaultLayer))` wrapper and raw `bun:test` `test(...)` cases rather than the `testEffect(...)` harness. This code was adopted wholesale from upstream via the graft strategy (per `project_upstream_strategy.md`). Migrating to `testEffect` is deferred to a follow-up PR so as not to mix refactor + harness-migration intents and to avoid drifting the diff from the upstream baseline needed for clean future grafts. Do NOT re-flag the `ManagedRuntime.make` usage in this file as needing harness migration until that follow-up lands.
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `makeRuntime` from `src/effect/run-service.ts` for all services; it returns `{ runPromise, runFork, runCallback }` backed by a shared `memoMap` that deduplicates layers
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `Schema.TaggedErrorClass` for typed errors in Effect schemas
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `Schema.Class` for multi-field data in Effect schemas
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T07:27:41.971Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/file/ripgrep.ts:29-118
Timestamp: 2026-04-28T07:27:41.971Z
Learning: In `packages/opencode/src/file/ripgrep.ts` (Astro-Han/pawwork), the internal Effect schemas `TimeStats`, `Stats`, `PathText`, `Begin`, `End`, `Summary`, and the exported `SearchMatch`/`Match`/`Result` union are all declared as `Schema.Struct` — matching upstream `dev:packages/opencode/src/file/ripgrep.ts` lines 60-118. PR `#270` is an upstream-sync graft per `project_upstream_strategy.md`. Converting these to `Schema.Class` would diverge from the upstream baseline and create recurring merge friction on future syncs. The `Schema.Class` convention applies to PawWork-authored schemas, not to upstream-grafted shapes in this file. Do NOT re-flag the use of `Schema.Struct` for these schemas in upstream-sync PRs; flag it only in a PR that simultaneously converts the upstream side, or in a dedicated PawWork carve-out PR for ripgrep schemas.
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use branded schemas (`Schema.brand`) for single-value types in Effect
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `Effect.addFinalizer` or `Effect.acquireRelease` inside the `InstanceState.make` closure for cleanup (subscriptions, process teardown, etc.)
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T06:47:20.342Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/question.ts:6-16
Timestamp: 2026-04-28T06:47:20.342Z
Learning: In Astro-Han/pawwork, `packages/opencode/src/tool/question.ts` is a permanent PawWork carve-out: `Question.Prompt` (defined in `packages/opencode/src/question/index.ts`) remains a Zod schema, NOT an Effect Schema. The `ts-expect-error` boundary in `tool/registry.ts` marks the Zod ↔ Effect Schema seam. The dropped `packages/opencode/test/tool/question.test.ts` covered the old Zod-mixed surface. Schema-bound regression tests for this tool are intentionally deferred until `question/index.ts` migrates to Effect Schema in its own dedicated PR. Do NOT request restoring question tool tests in upstream-sync PRs; flag it only in the PR that migrates `question/index.ts` to Effect Schema.
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-28T05:36:25.456Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/grep.test.ts:16-22
Timestamp: 2026-04-28T05:36:25.456Z
Learning: In `packages/opencode/test/tool/grep.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a manual `ManagedRuntime.make(Layer.mergeAll(...))` setup and raw `bun:test` test cases rather than the `testEffect(...)` harness. The migration to `testEffect` + `it.live(...)` + `provideTmpdirInstance(...)` is a whole-file rewrite deferred to a dedicated sweep PR that will migrate all tool tests still on the manual ManagedRuntime pattern together. Do NOT re-flag the ManagedRuntime setup in this file as needing harness migration until that sweep PR lands.
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-27T11:18:47.298Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 271
File: packages/opencode/test/tool/websearch.test.ts:21-78
Timestamp: 2026-04-27T11:18:47.298Z
Learning: In `packages/opencode/test/tool/websearch.test.ts`, the tests intentionally use manual `Effect.runPromise` with explicit `Effect.provide(...)` chains (including `Layer.succeed(Auth.Service, ...)`, `Layer.succeed(HttpClient.HttpClient, http)`, `WebSearchAuth.layer`, `Truncate.defaultLayer`, and `Agent.defaultLayer`) rather than the `testEffect(...)` harness. This is by design: the fake Auth and HTTP recovery-metadata layers must be explicitly injected and kept visible/scoped at the test site. Do NOT suggest migrating these tests to `testEffect` or removing the manual layer provides.
Applied to files:
packages/opencode/src/session/subagent-run.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : For background loops or scheduled tasks, use `Effect.repeat` or `Effect.schedule` with `Effect.forkScoped` in the layer definition
Applied to files:
packages/opencode/src/session/subagent-run.ts
…ectedInput
Neither start() nor recordRejected() reads input.subagent_type — the
SubtaskPart schema does not carry it, and child-session tagging is
already done by sessions.create({ subagentType }) in agent.ts. The
field made the SubagentRun API imply responsibility for tagging that
this layer never performed. Remove it from the DTOs and from the two
agent.ts call sites; tests no longer pass it.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/opencode/test/session/subagent-run.test.ts (1)
38-58: 🧹 Nitpick | 🔵 TrivialCollapse the repeated ad hoc runtime setup into the test harness.
This file keeps re-building the same
tmpdir+Instance.provide+Effect.runPromise(Layer.mergeAll(...))scaffolding in each case. Moving the suite totestEffect(...)plus the fixture helpers would remove a lot of duplicated wiring and keep these Effect-service tests aligned with the repo-standard harness.As per coding guidelines: "Use
testEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows." and "Prefer Effect-aware helpers fromfixture/fixture.tsover building manual runtimes in tests: usetmpdirScoped()for scoped temp directories,provideInstance(dir)(effect)for low-level binding without directory creation,provideTmpdirInstance(...)for single temp instance binding, orprovideTmpdirServer(...)for tests that also need the test LLM server."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/session/subagent-run.test.ts` around lines 38 - 58, The test "rejects more than 5 concurrent reservations per parent" repeats manual runtime setup (tmpdir + Instance.provide + Effect.runPromise + Layer.mergeAll); replace that scaffolding by converting the test to use the repo test harness: rewrite the test to run under testEffect(...) and use tmpdirScoped() to provide a temporary directory and one of the fixture helpers (provideTmpdirInstance or provideTmpdirServer / provideInstance) to bind the Instance and layers instead of calling Instance.provide and Effect.runPromise with Layer.mergeAll; keep the existing logic around SubagentRun.Service reserveSlot/releaseSlot but wire the environment via the provided fixtures so the test no longer constructs its own runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/session/subagent-run.ts`:
- Around line 270-340: The part update paths don't persist lifecycle events:
modify finalize (function finalize) to append an appropriate terminal event
object (e.g., { type: "completed"|"completed_empty"|"failed"|"canceled_by_user",
at: Date.now(), kind?: ... } based on the status argument) into
existing.recent_events when calling session.updatePart; similarly, modify
setConsumed (function setConsumed) to append a { type: "consumed", at:
Date.now() } event when setting consumed_at. Use the existing.recent_events
array from the part (avoid duplicating events if already present) and update
updated_at/ended_at fields as you already do so the persisted part contains the
full transition history. Ensure you reference the same shapes used in
recordRejected (which already sets recent_events) for consistency.
In `@packages/opencode/src/tool/agent.ts`:
- Around line 85-98: The backscan in makeReadLastCompletedAssistantText stops
after sessions.messages(..., limit: 5) which can miss earlier assistant
completions; update the implementation to page through messages (or remove the
fixed small limit) until an assistant text part with isTextPartCompleted(...) is
found or there are no more messages, by repeatedly calling sessions.messages
with appropriate pagination parameters and scanning each batch in reverse order
(refer to makeReadLastCompletedAssistantText, sessions.messages, and
isTextPartCompleted).
- Around line 390-393: The finalizeOk handler currently swallows errors from
finalizeAfter by using Effect.catchCause(() => Effect.void), which lets
execution proceed to subagentRun.read even if subagentRun.finalize failed;
remove that catch and let the failure propagate (or map the cause to an
Effect.fail) so a failed subagentRun.finalize prevents further processing.
Update the finalizeOk arrow function (and/or finalizeAfter usage) so that errors
from subagentRun.finalize are not caught and discarded but returned as a failing
Effect, ensuring subsequent calls like subagentRun.read only run on successful
finalization.
---
Duplicate comments:
In `@packages/opencode/test/session/subagent-run.test.ts`:
- Around line 38-58: The test "rejects more than 5 concurrent reservations per
parent" repeats manual runtime setup (tmpdir + Instance.provide +
Effect.runPromise + Layer.mergeAll); replace that scaffolding by converting the
test to use the repo test harness: rewrite the test to run under testEffect(...)
and use tmpdirScoped() to provide a temporary directory and one of the fixture
helpers (provideTmpdirInstance or provideTmpdirServer / provideInstance) to bind
the Instance and layers instead of calling Instance.provide and
Effect.runPromise with Layer.mergeAll; keep the existing logic around
SubagentRun.Service reserveSlot/releaseSlot but wire the environment via the
provided fixtures so the test no longer constructs its own runtime.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 2dc3e8ee-dc33-4f09-8c2f-ba7e4ee97f8d
📒 Files selected for processing (4)
packages/opencode/src/session/subagent-run.tspackages/opencode/src/tool/agent.tspackages/opencode/test/session/subagent-lifecycle-integration.test.tspackages/opencode/test/session/subagent-run.test.ts
recent_events used to stop at "started" for any row not produced by recordRejected — finalize and setConsumed updated only the status / consumed_at fields without appending a matching event. That left the ring useless for reconstructing the actual transition sequence (started → completed / failed / canceled_by_user → consumed). Hoist the eviction + stable-sort logic into a shared appendLifecycleEvent helper and use it from recordEvent, finalize, and setConsumed. finalize maps TerminalStatus to the matching SubtaskEvent variant (failed carries the error.kind); setConsumed appends a "consumed" event when the consumed_at flip lands.
…nd split finalize paths - makeReadLastCompletedAssistantText drops the limit:5 backscan and scans the full child message history. Tool-heavy children can push the last completed assistant text past five messages, which used to surface as partial_result: null for canceled_by_user even though stable text existed. Cancellation is a rare path so the extra read cost is acceptable. - Replace the tap+catch finalize handling with Effect.matchCauseEffect. The previous wrapper swallowed ok-finalize failures via catchCause, which let read() see a stale "running" row and synthesizeOutput render a defective success. Each path now runs its matching finalize exactly once: success → ok-finalize (failures propagate to outer catch); failure → err-finalize as best-effort cleanup, then re-raise the original cause via failCause.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Astro-Han
commented
Apr 28, 2026
Owner
Author
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Additional review pass focused on current head, edge cases, regressions, and cross-module impact.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/session/subagent-run.ts`:
- Around line 411-415: The current sort in the collectSubtaskParts result only
orders by started_at, which can tie and leave selection non-deterministic;
update the comparator used where matches are sorted (used by
findLatestBySessionID and list logic) to add a deterministic tiebreaker chain
such as comparing started_at descending then created_at (or another monotonic
timestamp) descending and finally a unique id/sequence descending to guarantee a
stable, newest-first order; apply the same change to both places that call
matches.sort(...) so the latest subtask is always chosen deterministically
(refer to collectSubtaskParts, findLatestBySessionID, list, matches, and match).
In `@packages/opencode/src/tool/agent.ts`:
- Around line 428-443: The catchCause block currently passes the raw Cause to
errorMessage, producing unhelpful JSON; update that handler inside the
Effect.catchCause (the generator that reads subagentRun.read(ctx.callID!) and
calls subagentRun.finalize) to call Cause.squash(cause) and pass the squashed
value into errorMessage before storing it (i.e., replace errorMessage(cause)
with errorMessage(Cause.squash(cause))) so the persisted failure message matches
other paths that use Cause.squash.
In `@packages/opencode/test/session/subagent-run.test.ts`:
- Around line 349-353: The test currently only asserts that Session.updatePart()
failed; update it to assert the specific guard defect by unwrapping the Effect
exit and checking that the failure is a Defect of type/name
SubagentRunGuardViolation and that its payload includes the expected
tool_call_id from the part; locate the call to session.updatePart(...) and
replace the generic expect(exit._tag).toBe("Failure") with assertions that the
exit represents a Failure whose cause is the SubagentRunGuardViolation defect
(matching the defect/tag name) and that the defect object contains tool_call_id
=== part.tool_call_id.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: f9628382-c411-4673-8185-9512cb668dc6
📒 Files selected for processing (4)
packages/opencode/src/session/subagent-run.tspackages/opencode/src/tool/agent.tspackages/opencode/test/session/subagent-lifecycle-integration.test.tspackages/opencode/test/session/subagent-run.test.ts
MessageV2.parts() and Session.getPart() return raw row.data with a
type cast — they never run the SubtaskPart zod schema, so its
.default("completed") / .default([]) annotations are not applied to
rows persisted before this PR. Legacy subtask rows therefore reached
agent_list / agent_output as `status === undefined`, rendering as
"undefined (unread)" and breaking the advertised backward-compat path.
Add a hydrateSubtask coercion helper and apply it in collectSubtaskParts
(used by list / findLatestBySessionID / readByToolCallID) and in
readPart (used by every per-row mutator), so old rows surface with
status: "completed" and recent_events: [] like new ones.
last_activity: - Nothing in v1 ever wrote last_activity (recordActivity was already removed earlier; no caller wires child tool/model events). agent_list and agent_output were reading last_activity?.label as the "what is it doing" label, which always fell back to "-" / result_summary in practice. YAGNI: remove the schema field, the readers, and its mention in LIFECYCLE_KEYS. Progress reporting comes back as a v1.1 feature with a real writer. - agent_output's running-status header surfaces result_summary instead of an empty placeholder; agent_list's row activity falls back to result_summary or "(legacy)". agent_output sessions.get cleanup: - Switch the catchCause to suppress only NotFoundError (squash the cause + instanceof check), re-raising any other cause via failCause. Same selective-catch pattern that 6bf8a1f applied to the resume lookup; storage / schema-decode failures are no longer masked as a clean "subagent inaccessible" response.
Note that ctx.metadata (set in step 4) keeps subagent_session_id visible to the parent for resume even when the tool result lands as \`error\` — answers a recurring review question about why model failure does not surface a structured status: failed via synthesizeOutput. The existing test contract (b50815f) deliberately expects tool state = error + metadata.sessionId on child failure.
findLatestBySessionID and list both sorted only on started_at. Two rows started in the same millisecond — possible under burst dispatch of multiple subagents from one parent — kept whichever the scan saw first, so agent_output could surface a stale row and agent_list could return the older one as "latest". Pair each part with its scan index and break the started_at tie on index descending, mirroring the deterministic ordering recordEvent uses for SubtaskEvents that share Date.now().
errorMessage(cause) fell through to JSON.stringify on the raw Cause
object, producing {"_tag":"Fail",...} which JSON_ENVELOPE_RE then
collapsed to "<json>" — uninformative for the persisted
SubtaskPart.error.message. Use Cause.squash to extract the underlying
error before sanitizing, matching what 6bf8a1f and the err-path
finalize already do.
The guard test only checked exit._tag === "Failure", which would still pass if updatePart started failing for an unrelated reason in a future refactor. Inspect the Cause's Die reasons, find the SubagentRunGuardViolation defect, and assert its tool_call_id matches the row being mutated. The test now actually pins the contract this PR introduces.
This was referenced Apr 30, 2026
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
Implements subagent lifecycle v1 (foreground-only) per #283. The agent tool now exposes explicit terminal states (completed / completed_empty / failed / canceled_by_user), caps parallel dispatch at 5 per parent, and ships two new model-callable inspection tools (
agent_list,agent_output) so a parent agent can ask "is my subagent still running, what is it doing, what did it return".Why
Today the agent tool returns an empty
<subagent_result>on cancel and silently swallows model failures, with no way for the dispatching agent to introspect or recover. This change makes lifecycle explicit, queryable, and bounded so common subagent failure modes stop being invisible. Background execution (run_in_background, notification injection, abandoned sweep, nested subagents) is intentionally deferred to v1.1.Related Issue
#283 (v5 spec, comment id 4332342274).
Architecture
SubtaskPart(packages/opencode/src/session/message-v2.ts) extended with all lifecycle / progress / result fields. Every new field is.optional()or.default(...)so legacy rows decode as terminal-completed without rewrites.Sessioncolumnscreated_by_agent_tool+subagent_typemark child sessions as agent-tool-created and tag the subagent type.SubagentRunservice (packages/opencode/src/session/subagent-run.ts) is the single writer for lifecycle fields. Parent slot reservation is gated bySemaphore.makeUnsafe(1)per parent at MAX_ACTIVE=5; per-row mutex serializes lifecycle writes.Context.Reference<boolean>insubagent-run-context.ts.Session.updatePartrejects non-writer SubtaskPart writes that mutate lifecycle fields withSubagentRunGuardViolation.AgentTool.executerestructured into a scoped 8-step flow withEffect.addFinalizerforreleaseSlot+ listener cleanup on every exit path, and an outerEffect.catchthat finalizes a started SubtaskPart tofailedso pre-prompt errors cannot strand the row inrunning.AgentPromptOps.wasInterrupted(sessionID)query (set bySessionRunState.onInterruptfiring on the child runner) deterministically distinguishes child-runner abort from natural completion or model failure.agent_listfilters by lifecycle status (running / completed / completed_empty / failed / canceled_by_user / all_active / all).agent_outputaccepts XOR(subagent_session_id,tool_call_id), validates parent ownership andcreatedByAgentTool, and marks terminal rows consumed on first read.Spec deviations
Two implementable mechanisms in this branch differ slightly from the literal spec text; both preserve the spec's intent:
Context.Reference+ inline check, not a Layer wrapper. The spec sketch was a circular layer dep; the Reference lives in its own module so both writers and the guard observe it without circularity.AgentPromptOps.wasInterrupted(sessionID)query, not a subscribe API. The currentSessionRunStateexposes no such API; the per-InstanceSetwritten fromloop()'sonInterruptarg gives equivalent semantics with smaller surface.Not in this PR (deferred / known limitations)
packages/app/is mid-rewrite and does not yet renderSubtaskPartat all. Lifecycle data is queryable viaagent_list/agent_output; the visual card is a follow-up.reserveSlotcap accuracy after host restart. In-memoryactiveCountsresets on restart; persistedrunningrows are not re-counted. Strandedrunningrows belong to the v1.1 abandoned-sweep work.run_in_background,agent_stop,block=true, abandoned, notification injection, nested subagents). All v1.1.How To Verify
```bash
bun --cwd packages/opencode run typecheck
bun --cwd packages/opencode test test/session/subagent-run.test.ts
test/session/subagent-lifecycle-integration.test.ts
test/tool/agent.test.ts
test/session/prompt-effect.test.ts
```
Targeted suites cover: cap rejection + FIFO contention, lock-ordering invariant, all five terminal states end-to-end, ring-eviction with lifecycle pinning + hard-20 cap, single-writer guard violation, resume validation (parent / createdByAgentTool / subagent_type), backward-compat decode of legacy SubtaskPart rows.
Repo-wide `turbo test:ci` left to PR CI.
Checklist
Summary by CodeRabbit
New Features
Tests
Chores