_{13 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

💡 Codex Review

Preserve completion flags when starting announce cleanup

startSubagentAnnounceCleanupFlow no longer forwards entry.expectsCompletionMessage (or entry.spawnMode) into runSubagentAnnounceFlow, so completion-mode runs are treated as regular announces. This bypasses completion-specific routing (including resolveSubagentCompletionOrigin/subagent_delivery_target handling) and changes delivery behavior from completion-direct semantics to generic queue-first behavior, which can delay or misroute completion updates for manual/session subagent workflows.

Good work consolidating these five fixes. I went through all 15 files carefully. The method: "send" removal is the right call and the BB message ID extraction improvements are solid. A few things to flag:

SSRF allowlist — looks safe, one nit

The buildBlueBubblesAttachmentSsrFPolicy approach is sound: it extracts only the hostname from the user-configured serverUrl and pins both allowedHostnames and hostnameAllowlist to that single host. This correctly scopes the SSRF bypass to the exact BB server the user already trusts in their config — it doesn't blanket-allow RFC 1918 ranges or anything like that. The try/catch returning undefined on malformed URLs is a nice defensive fallback.

One thing to verify: does the SSRF guard in fetchRemoteMedia also validate redirect targets against hostnameAllowlist? If BB sends a 3xx redirect to a different internal host, we want to make sure that's still blocked. The comment says "Keep redirects pinned to the same trusted host" but I want to confirm the consumer actually checks hostnameAllowlist on redirect resolution, not just on the initial request. If it does, this is good.

method: "send" removal — correct fix, love the warning comment

The // ⚠️ CRITICAL: DO NOT ADD A "send" PATH HERE comment is excellent given this has regressed 3+ times. The test changes are thorough — every assertion that previously checked sendSpy now correctly checks agentSpy, and the expected message content changed from pre-formatted "✅ Subagent main finished" headers to the raw "Result:" + LLM instruction pattern. This is the right architecture: let the parent LLM process and rephrase the result.

countPendingDescendantRuns — good addition, subtle semantics

The distinction between "active" (not ended) and "pending" (ended but cleanup not completed) is important for avoiding premature announcements. The logic:

const runEnded = typeof entry.endedAt === "number";
const cleanupCompleted = typeof entry.cleanupCompletedAt === "number";
if (!runEnded || !cleanupCompleted) {
  count += 1;
}

This counts a run as pending if it's either still running OR ended-but-not-cleaned-up. The new test in subagent-registry.nested.test.ts covers this correctly. Good.

Codex bot's P1: session-mode archival sweep

The Codex bot flagged that removing the spawnMode === "session" exemption from archiveAtMs means persistent session-mode subagents will now get swept. Is this intentional? If session-mode subagents are being removed entirely (which this PR suggests — spawnMode is stripped from SubagentRunRecord), then yes, they should be archived. But if anything still relies on long-lived session subagents surviving indefinitely, this is a regression. Can you confirm the session-mode concept is fully removed, including from the store layer? The Greptile bot noted subagent-registry.types.ts still exports the old type with spawnMode — if subagent-registry.store.ts references that stale type, you'll get a mismatch at the persistence boundary.

Codex bot's P1: subagent_ended hook no longer fires on kill

markSubagentRunTerminated no longer calls emitSubagentEndedHookOnce. If any plugins rely on the subagent_ended lifecycle hook for cleanup when a subagent is killed (not just when it completes normally), those plugins will silently stop receiving kill notifications. Is this intentional? If the hook infrastructure is being simplified, that's fine, but it should be documented as a breaking change for plugin authors.

Inbound meta changes — mostly good, one question

Promoting SenderName above SenderE164 in the sender field preference chain and including sender info for DMs (not just groups) is a nice UX improvement. The message_id change to prefer MessageSid and never inject both message_id + message_id_full saves tokens.

One question: resolveSenderLabel now receives an id parameter. Is the function signature updated to accept it? The diff only shows the caller side, not the callee. If resolveSenderLabel doesn't use id in its label construction, passing it is harmless but the test expects label: "Tyler (+15551234567)" which suggests it does use it as a fallback when e164 is missing. Just want to confirm the function body handles the new param.

readLatestSubagentOutput removal — simplification is welcome

The old readLatestSubagentOutput had a complex fallback chain: try readLatestAssistantReply, then fall back to chat.history parsing with extractSubagentOutputText / extractToolResultText / extractInlineTextContent. This PR collapses all of that to just readLatestAssistantReply. The test changes confirm that tool-role messages are no longer used as fallback output — they now produce "(no output)" instead. This is cleaner, but is a behavioral change: subagents that only produce tool output without a final assistant message will now report no output. Is that acceptable for all subagent types?

finalizeSubagentCleanup sync vs async

finalizeSubagentCleanup changed from async to sync, and the .then() call in startSubagentAnnounceCleanupFlow dropped the void prefix. This is fine since the function no longer awaits anything internally, but the emitCompletionEndedHookIfNeeded call that was previously awaited inside it is now gone entirely (related to the lifecycle hook removal above).

Overall: the core fixes (send-path removal, SSRF allowlist, BB message IDs, sender identity) are solid. The main concerns are the two items the Codex bot flagged (session-mode archival, killed-hook removal) plus confirming redirect validation in the SSRF guard. Happy to approve once those are addressed or confirmed intentional.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b7c8922bde

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 59ba16a810

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b564a0bfeb