fix(server): track OhMyPi child agent tasks from native tool updates - #18
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues affect live progress, terminal results, and task completion.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds native OhMyPi task-update parsing so delegated child agents appear in existing lifecycle and agent-panel flows.
Changes:
- Parses progress, status, result, and job snapshots.
- Emits deduplicated child-task lifecycle events.
- Adds mock-agent fixtures and driver coverage.
File summaries
| File | Summary | Review findings |
|---|---|---|
apps/server/src/provider/Layers/OhMyPiAdapter.ts |
Parses snapshots and emits child-task events. | Moderate (1 vote): nested task updates may be suppressed by ACP coalescing. Moderate (2 votes): terminal result summaries may be discarded. Moderate (1 vote): several terminal status variants are not recognized. Nit (1 vote): tool changes are missing from deduplication. |
apps/server/src/provider/Drivers/OhMyPiDriver.test.ts |
Tests lifecycle mapping and panel integration. | No final comments. |
apps/server/scripts/acp-mock-agent.ts |
Simulates native OhMyPi task updates. | No final comments. |
Review details
Suppressed comments (3)
apps/server/src/provider/Layers/OhMyPiAdapter.ts:961
- This hook only receives
ToolCallUpdatedevents that survive ACP's generic tool-update coalescer. That coalescer measures only top-level text fields, while these task snapshots live underrawOutput.details.progress/results/statusEvents/jobs, so repeated in-progress snapshots with unchanged tool detail are suppressed (up to the coalescing limit) and child status changes can remain invisible until the tool completes. Please route these native task updates through a snapshot-aware path or make the coalescer account for the nested task payload before relying on this hook for live progress.
yield* emitOhMyPiChildTaskEvents(ctx, event.toolCall);
apps/server/src/provider/Layers/OhMyPiAdapter.ts:257
childTaskStatusonly recognizescompleted,failed,aborted, andcancelled. Native task snapshots can also report terminal values such assuccess/succeeded,error,stopped, orkilled(the analogous task mapper handles these), and those currently fall through torunning, so the adapter emitstask.progressforever and never completes/stops the child. Normalize the status before mapping and cover these terminal variants in the driver test.
function childTaskStatus(
entry: OhMyPiTaskEntry,
): "pending" | "running" | "completed" | "failed" | "stopped" {
if (entry.aborted || entry.status === "aborted" || entry.status === "cancelled") return "stopped";
if (entry.status === "failed" || (entry.exitCode !== undefined && entry.exitCode !== 0))
return "failed";
if (entry.status === "completed" || entry.exitCode === 0) return "completed";
return entry.status === "pending" ? "pending" : "running";
apps/server/src/provider/Layers/OhMyPiAdapter.ts:510
currentToolis omitted from the dedup fingerprint even though it is emitted aslastToolNamebelow. Because the summary preferslastIntent, a task can switch tools while status, intent, and usage stay unchanged; the identical fingerprint then suppresses the update and leaves the agent panel showing a stale last tool.
entry.tokens ?? "",
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f8e7ab4bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues affect lifecycle cleanup, validation, status mapping, false-positive prevention, and event payload size.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
apps/server/src/provider/Layers/OhMyPiAdapter.ts:148
RuntimeTaskUsageis validated downstream withNonNegativeInt, but this helper accepts fractional values. A malformed native snapshot such astokens: 1.5ordurationMs: 2.5will therefore produce an invalidtypedUsagepayload and can fail when the runtime event is decoded or persisted, despite the parser being intended to be defensive. Restrict these numeric fields to finite integers before emitting them.
apps/server/src/provider/Layers/OhMyPiAdapter.ts:463
- This map retains one mutable state object for every child task ID and is never pruned until the entire provider session stops. Long-lived OhMyPi sessions that delegate repeatedly will accumulate titles, fingerprints, and terminal states indefinitely. Keep active tasks separate from a bounded terminal dedup/enrichment cache, or otherwise evict completed entries.
const existing = ctx.childTasks.get(stateKey);
const state: OhMyPiChildTaskState = existing ?? {
taskId,
turnId: ctx.activeTurnId,
toolUseId: toolCall.toolCallId,
title: candidateTitle,
role: candidateRole,
started: false,
terminal: false,
fingerprint: undefined,
};
ctx.childTasks.set(stateKey, state);
apps/server/src/provider/Layers/OhMyPiAdapter.ts:271
- This mapper treats every unrecognized status as
running, so native values such assuccess/succeeded,error,stopped, orkillednever reach the required terminalcompleted/failed/stoppedstates. The existing task lifecycle mapper handles these aliases (seeapps/server/src/provider/acp/XAiBackgroundTasks.ts:35-56); normalize the OhMyPi status before falling back to running, otherwise a finished child can remain active in the panel.
function childTaskStatus(
entry: OhMyPiTaskEntry,
): "pending" | "running" | "completed" | "failed" | "stopped" {
if (entry.aborted || entry.status === "aborted" || entry.status === "cancelled") return "stopped";
if (entry.status === "failed" || (entry.exitCode !== undefined && entry.exitCode !== 0))
return "failed";
if (entry.status === "completed" || entry.exitCode === 0) return "completed";
return entry.status === "pending" ? "pending" : "running";
apps/server/src/provider/Layers/OhMyPiAdapter.ts:223
- This promotes any tool output containing
details.jobswith atype: "task"entry, even when the enclosing tool is not OhMyPi's hub-wait envelope. An ordinary tool can therefore createtask.started/task.completedevents, contrary to the false-positive guard covered by this PR; gate this branch on the hub-wait discriminator (for exampledetails.op === "wait") or another equivalent native marker.
const jobValues = Array.isArray(details.jobs)
? details.jobs
.filter((value) => isUnknownRecord(value) && value.type === "task")
.map((value) => ({
...value,
id: nonEmptyString(value.agentUrlId) ?? value.id,
agent: nonEmptyString(value.type) ?? "task",
task: nonEmptyString(value.label),
output: nonEmptyString(value.resultText),
error: nonEmptyString(value.errorText),
status: value.status === "cancelled" ? "aborted" : value.status,
}))
: [];
apps/server/src/provider/Layers/OhMyPiAdapter.ts:736
- The callback forces every matching native update through the ACP event queue, but these snapshots keep child
output/stderrand progress arrays nested underrawOutput.details. The ACP raw-output bounding only limits top-level fields (content,stdout,stderr,output), so a busy batch can repeatedly send and persist large cumulative nested snapshots; bound/strip those nested fields before emission or keep a separate lightweight task-update path.
shouldEmitToolCallUpdate: (toolCall) =>
parseOhMyPiTaskSnapshot(toolCall.data.rawOutput) !== undefined,
apps/server/src/provider/Layers/OhMyPiAdapter.ts:883
- The new child-task map is never drained when
stopSessionInternalcloses the ACP session (lines 661-678), so children that were running when the provider is stopped receive notask.completedwithstatus: "stopped". This leaves persisted agent rows without a terminal lifecycle and can make them reappear as running after the session is resumed; the Claude adapter explicitly completes its live tasks during shutdown (apps/server/src/provider/Layers/ClaudeAdapter.ts:4099-4117). Emit stopped completions for non-terminal entries before removing the session.
childTasks: new Map(),
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7c21c9605
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
apps/server/src/provider/Layers/OhMyPiAdapter.ts:301
childTaskStatustreats every status outsidepending/completed/failed/aborted/cancelledasrunning, so native terminal values such assuccess/succeeded,error,stopped, orkillednever emittask.completedwhen no exit code is present. Normalize the provider's terminal aliases here (as the other native task mapper does) before falling back torunning.
apps/server/src/provider/Layers/OhMyPiAdapter.ts:492ctx.childTasksretains every completed child forever; onlystate.terminalchanges and no entry is ever removed or bounded. Long-lived sessions that delegate repeatedly will therefore accumulate one state and fingerprint per child, so please add a bounded/pruned terminal cache (while retaining enough state to deduplicate late result snapshots).
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7c25ce644
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue; | ||
| } | ||
| const terminal = status === "completed" || status === "failed" || status === "stopped"; | ||
| const terminalSummary = entry.abortReason ?? entry.error ?? entry.output ?? entry.stderr; |
There was a problem hiding this comment.
When a failed or stopped child result contains both output and stderr—for example, partial stdout followed by a nonzero exit and a diagnostic on stderr—this ordering selects output as the terminal summary. The client treats a failed completion's summary as its error, so the agent panel displays the partial output and hides the actual failure diagnostic. Select error/stderr ahead of output for failed and stopped statuses while retaining output precedence for successful completions.
Useful? React with 👍 / 👎.
- Parse TaskTool progress/results/statusEvents/jobs from ACP tool calls into task.started/progress/completed runtime events - Deduplicate updates via status fingerprints and prevent non-terminal regressions after completion - Add mock agent scenario and driver test covering lifecycles and panel model
e7c25ce to
a6c4c4d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues remain with unbounded child-task retention and incomplete terminal-status normalization.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
apps/server/src/provider/Layers/OhMyPiAdapter.ts:492
childTasksretains every observed child forever and is only discarded when the whole provider session stops. A long-lived session that repeatedly delegates work will therefore grow this map without bound even though terminal entries are no longer needed for shutdown; keep active tasks separate from bounded terminal/enrichment state, or otherwise prune completed IDs once late result enrichment is no longer possible.
ctx.childTasks.set(stateKey, state);
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
| if (entry.aborted || entry.status === "aborted" || entry.status === "cancelled") return "stopped"; | ||
| if (entry.status === "failed" || (entry.exitCode !== undefined && entry.exitCode !== 0)) | ||
| return "failed"; | ||
| if (entry.status === "completed" || entry.exitCode === 0) return "completed"; | ||
| return entry.status === "pending" ? "pending" : "running"; |
What Changed
OhMyPi subagent work now surfaces in the UI even when it flows through the provider's native TaskTool payloads rather than the agent-hub API. The adapter inspects
tool_call/tool_call_updateoutput for task-shaped progress and result snapshots, folds them into stable child-task lifecycles (task.started/task.progress/task.completed), and links them to the originating tool call.OhMyPiAdapterlogic: defensive parsing of progress entries,statusEvents, and job summaries; status mapping to completed/failed/stopped; fingerprint-based dedup so repeated updates don't re-emit; and result enrichment of a terminal snapshot's final output/usage.acp-mock-agent.tswith aT3_ACP_EMIT_OH_MY_PI_TASK_UPDATESmode reproducing the native batch-task update shapes.Why
OhMyPi child agents were invisible in the agent panel whenever the CLI reported them only via native tool updates, leaving users with no visibility into delegated work. Parsing this evidence at the adapter boundary (rather than trusting tool-call titles) keeps orchestration and the client side unchanged, and the strict entry parser plus fingerprint dedup avoids false positives and duplicate events.
UI Changes
Not applicable — no UI code changed. User-visible effect: OhMyPi child agents appear correctly in the existing agent panel for providers that only emit native task updates.
Checklist
Generated with GLM (glm-5.3-flash) via opencode.