feat: enhance agent runner

[pikann]

Summary

Fixes 15 findings from a full code review of the services/agent-runner migration (services/ai-agent Python → Go/Goose), covering correctness bugs, concurrency races, a path-safety gap, and one efficiency issue — plus new documentation for the service. Every fix ships with a regression test; the concurrency fixes are additionally verified under go test -race, and each of those tests was confirmed to actually catch its bug (temporarily reverted the fix locally, watched the test fail, restored it).

Correctness

• MCP tool calls failed auth for nearly every project-scoped conversation. buildMCPServers sent PACA_ACTOR_USER_ID from trigger.ActorMemberID instead of trigger.ActorUserID — the former is set on every project-scoped trigger and gets rejected by services/api's verifyAgentIdentity (which only accepts an actor-user-id claim for a global-scope agent), breaking get_task, clone_repository, and every other MCP tool call during normal project chat.
• Global chat agents were told they were scoped to a nonexistent project. buildInitialMessage unconditionally rendered You are working inside project \00000000-0000-0000-0000-000000000000`` for global-chat conversations instead of the intended "you are a global agent" framing.
• Gemini- and DeepSeek-configured agents couldn't start at all. resolveProviderEnv passed Paca's llm_provider value straight through as GOOSE_PROVIDER, but Goose registers Gemini as "google" and DeepSeek as "custom_deepseek" — verified directly against block/goose's source (a public docs page for Goose turned out to be wrong about this). cohere has no Goose provider at all; left mapped (with a comment explaining why) so it fails with a clear "unknown provider" error instead of silently misrouting through the OpenAI fallback.
• A skill-load failure left conversations stuck forever with no visible error. BundledSkills.Load could fail before the conversation was ever marked running, and this service's Valkey consumer has no redelivery mechanism, so the conversation just sat there. Reordered so running is written first; a load failure now marks the conversation failed with the underlying error.
• Diff cards showed cumulative, not incremental, changes. Editing the same file twice in one turn always diffed against turn-start git HEAD, so the second edit's diff card showed both edits combined. Now tracks a per-turn baseline per file.
• Dropped the "no human is watching" framing for automation-triggered conversations, restored from the old Python implementation.
• clone_repository recursively force-deleted an agent-supplied targetDir with no validation — a task that got the agent to pass /, /home, or /etc would wipe it out. Now refuses a short list of protected top-level directories.

Concurrency safety

Root cause: nothing prevented two triggers for the same conversation_id from running Handle() concurrently, which enabled three related races:

• Silent event loss — event_index is allocated once per turn and incremented in-memory afterward; two concurrent turns could allocate the same index, and InsertEvent's ON CONFLICT DO NOTHING silently dropped the loser's events.
• A turn could become uncancellable — registry.Conversations.Register/Unregister had no ownership check, so a paused turn's deferred Unregister could delete a newer turn's live cancel entry.
• A chat sandbox could be torn down mid-turn, racing the idle reaper or a stop control message against a turn that had just started resuming it.

Fixed at the root: internal/messaging.Consumer now serializes trigger handling per conversation_id (different conversations still run concurrently). Layered with defense-in-depth: Register now returns an ownership token Unregister must match, Handle() registers in-flight before reading the paused sandbox, and TeardownPausedChatSandbox re-checks InFlight.IsRegistered before popping.

Also fixed a real goroutine/Redis-subscription leak in the ACP bridge: acpbridge.Registry.Register overwrote the connections map with no reference to the previous entry, so a same-process reconnect left the old connection's forwarder goroutine running forever.

Efficiency

• sandbox.Manager.ensureImage called docker.ImageList (enumerating every image on the host) on every single conversation start, even though the image is pinned for the process's lifetime. Now caches "confirmed present" after the first check.

Documentation

• New services/agent-runner/README.md — responsibilities, stack, source layout, local development (including the Docker-daemon/Postgres/Valkey prerequisite this service needs, unlike services/realtime's standalone dev loop), environment variables, testing, linting.
• Updated docs/ai-agent/agent-runner-service.md to document the per-conversation serialization guarantee, the registry ownership-token safety, the chat-sandbox teardown guard, the provider-id alias table, the clone_repository path-safety guard, and a previously-undocumented env var (PACA_MCP_DEV_SOURCE_DIR).

Test plan

• go build ./... and go vet ./... clean across services/agent-runner
• go test -race ./... clean, including new regression tests for every fix
• Regression tests confirmed to actually catch each bug (reverted the fix locally, watched the test fail, restored it)
• apps/mcp: bun run test (551 tests) and tsc --noEmit clean
• apps/web: bun run test (554 tests) and tsc --noEmit clean

[pullfrog]

✅ No new issues found.

Reviewed changes — Full initial review of all 23 files in commit bca8214 (agent-runner, apps/mcp, apps/web): read the entire diff front to back, traced every changed seam against the surrounding code, verified the third-party Goose provider claims against block/goose source, and ran the touched Go packages' unit tests (all pass under -race).

• Per-conversation trigger serialization — a refcounted per-conversation_id lock in the Valkey consumer guarantees two triggers for the same conversation never run Handler concurrently, closing the in-memory event_index increment and in-flight-registry races at their root; lock is acquired before the semaphore slot so queued triggers don't exhaust it.
• Token-owned in-flight registrations — Register/Unregister now hand back an ownership token so a stale deferred Unregister can't clear a newer turn's entry (pause/resume turn overlap case); TeardownPausedChatSandbox re-checks IsRegistered before popping so the idle reaper or a stop can't tear a resuming turn's sandbox out from under it.
• ACP bridge reconnect leak fix — Register now cancels, closes, and waits on a superseded same-process connection, so its forwarder/eviction-watcher goroutines and Redis Pub/Sub subscription actually exit instead of running forever against an orphaned connection.
• Bundled-skills failure surfaced as terminal — the conversation is marked running before skill load, so a transient services/api outage during BundledSkills.Load lands as a visible failed status + ack instead of an unacked message that is never actually redelivered (no XCLAIM in this consumer).
• Goose provider ID aliases — gemini→google and deepseek→custom_deepseek via an explicit alias table; verified against block/goose source (GOOGLE_PROVIDER_NAME, deepseek.json). Both previously failed to initialize silently for every conversation on those providers.
• Global-chat and automation prompt context — nil-project triggers get a global-agent context block instead of a bogus nil-UUID framing; automation-fired triggers get the "no human watching" note; PACA_ACTOR_USER_ID is now sourced from ActorUserID (was ActorMemberID, a member id mislabeled as a user id) so global-chat identity reaches apps/mcp.
• Turn-scoped diff baseline — a per-path baseline lets a second edit of the same file in one turn diff incrementally against the first edit's result rather than always against HEAD.
• Sandbox image-confirmation cache — ensureImage skips ImageList/ImagePull entirely for an already-confirmed pinned ref on subsequent sandbox starts.
• Realtime tail-cache pruning — useConversationEventWindow prunes the tail buffer down to events past the fetched window once a real fetch covers them, bounding a previously unbounded per-tab growth.
• clone_repository delete guard — assertSafeDeleteTarget refuses the recursive rm against a hand-picked set of top-level system directories regardless of what path the agent supplies, closing a prompt-injection crater the Python port it replaces never had.
• Docs — new services/agent-runner/README.md and expanded architecture doc, including the already-wired PACA_MCP_DEV_SOURCE_DIR.

Every load-bearing claim in the doc comments checks out against the code and — where it matters (provider IDs) — against upstream Goose source. The concurrency fixes are subtle and each carries a regression test that genuinely fails without the fix. Nothing to block or change.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes — Incremental delta since the prior review (commit bca8214): the new commit 479e0ca ("fix: resolve CI lint failures") is purely mechanical formatting — gofmt field alignment in convlock.go, goimports grouping in prompt.go, and a biome import-order fix in use-conversation-event-window.ts. No behavioral surface.

The original substantive review stands; nothing in this commit changes any of the previously-reviewed behavior.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

ℹ️ No critical issues — the new commit is correct and well-tested. One minor consolidation suggestion inline.

Reviewed changes — Incremental delta since the prior review (commit 479e0ca): commit ab390b0 ("feat: implement per-key locking mechanism and enhance concurrency safety in agent runner").

• Per-agent registerLocks in the ACP bridge — Register swapped the single process-wide registerMu for a convlock Locks keyed per agent_id, so one agent's potentially unbounded eviction wait (<-prev.done) can no longer stall every other agent's Register on this replica. Correct: presence, connection-map, and broadcast state are all strictly per-agent. Verifying test TestRegister_DifferentAgentsDoNotBlockOnEachOthersEviction genuinely blocks one agent's connector and asserts another's Register completes.
• resumeLock closes the Handle-vs-Teardown check-then-act race — Handle's registrar-of-flight + ChatSandboxes.Get now runs atomically, per conversation_id, with TeardownPausedChatSandbox's IsRegistered-check + Pop, so a stop/reaper can't tear down the sandbox a resuming turn just read. Held only across the two fast in-memory ops, never for the whole turn. The race test uses real registry.Conversations + chatsandbox.Registry and would fail without the lock.
• New generic internal/convlock package — per-key refcounted mutex used by both the handler and the bridge (see inline note).
• clone_repository hardening — /home/goose (the container user's home, parent of the default clone dir) added to the forbidden-delete set, with traversal test cases (/home/goose/../ → /home).

Verified: go build ./..., go vet, and go test -race all clean on the touched packages (convlock, handler, acpbridge, messaging). Nothing to block.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes — Incremental delta since the prior review (commit ab390b0): commit d641507 ("refactor: replace conversationLocks with convlock for improved trigger serialization") consolidates the duplicated per-key refcounted mutex.

• Deduplicated the per-key lock primitive — deleted internal/messaging/convlock.go (conversationLocks) and switched the messaging.Consumer to the shared convlock.Locks package (convlock.New() / Lock(trigger.ConversationID)), with doc comments updated to match. This directly implements the consolidation suggestion from the prior review.
• Behavior-preserving — convlock.Locks is byte-for-byte the same refcounted mutex conversationLocks was; the per-conversation_id serialization (lock acquired before the semaphore slot) and the regression test are unchanged. Verified the lock is still held only across the semaphore-acquire and handler call, never the whole run loop.

Verified locally: no lingering references to the old symbols, go build ./... and go vet ./... clean, and go test -race ./internal/messaging/... ./internal/convlock/... pass, including TestConsumer_SerializesTriggersForTheSameConversation. Consolidation done as suggested; nothing to block.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

ℹ️ No critical issues — the new preset instructions are well-formed and their ## Skill: … anchors genuinely resolve below the system prompt at runtime; two minor prompt-model notes inline.

Reviewed changes — Incremental delta since the prior Pullfrog review (commit d641507): the new commit 1eb96dee (single file, apps/web/src/lib/agent-api.ts). Each bundled LLM preset's defaultSystemPrompt now appends: "when you're assigned a task with nothing else said, skip skill-routing analysis and go straight to the <preset> skill (## Skill: <name> below)" — software-engineer → paca-do, code-reviewer & qa-engineer → paca-test, planner → paca-sprint, business-analyst → paca-clarify.

• Prompt text, no code changes — the create-agent dialog stores these strings as the agent's system_prompt, which agent-runner's buildInitialMessage emits first, followed by every enabled bundled skill as ## Skill: <name> sections. Confirmed against services/agent-runner/internal/executor/prompt.go:83-96 and services/api/internal/platform/bundledskills — all four referenced skills exist as agent-flavor entries (none CLIOnly), so nothing dangles.
• Consistent injection for every LLM conversation — the handler always prepends the bundled agent-flavor set to cfg.Skills (handler.go:135-152), so the referenced sections are always present below the system prompt regardless of per-agent skill customization.

Two optional refinements inline (both non-blocking observations, not defects).

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

The hard-wired "skip skill-routing analysis" directive sits directly above the always-injected bundled paca skill, whose Step 0 is written as a hard requirement ("You MUST locate the matching ## Skill: <name> section below and follow it", based on task status). For a task whose status the bundle's routing table would send to a different skill than the preset's fixed one (e.g. an in-review task that Step 0.5 maps to paca-test, but the software-engineer preset forces paca-do), the two prompt blocks now give the model explicitly conflicting orders. Usually harmless since the preset text is more specific, but wording the override explicitly ("this overrides the paca skill's routing step") would remove the ambiguity.

[pullfrog]

The planner preset's own defaultSystemPrompt is about task breakdown, create_task, types, and epics — persona-wise a fitter for the paca-breakdown / paca-epic sections — but the assigned-task path is hard-wired to paca-sprint (backlog planning). The bundled routing table only maps "to do / ready" tasks to paca-sprint; a differently-stated task would follow paca-do or paca-clarify. Since a planner is a persona, not a workflow, verify this mapping (or at least well-documented) so the preset routes towards the status it describes.