Skip to content

fix(cli): hydrate receiver inbound.db on approval-path destinations add#2510

Open
glifocat wants to merge 2 commits into
mainfrom
fix/2465-approval-destinations-inbound-sync
Open

fix(cli): hydrate receiver inbound.db on approval-path destinations add#2510
glifocat wants to merge 2 commits into
mainfrom
fix/2465-approval-destinations-inbound-sync

Conversation

@glifocat
Copy link
Copy Markdown
Collaborator

@glifocat glifocat commented May 16, 2026

Type of Change

  • Feature skill - adds a channel or integration (source code changes + SKILL.md)
  • Utility skill - adds a standalone tool (code files in .claude/skills/<name>/, no source changes)
  • Operational/container skill - adds a workflow or agent skill (SKILL.md only, no source changes)
  • Fix - bug fix or security fix to source code
  • Simplification - reduces or simplifies source code
  • Documentation - docs, README, or CONTRIBUTING changes only

Description

Fixes #2465 (related: #2464)

The bug

ncl destinations add (and its symmetric counterpart ncl destinations remove) require admin approval. When the approval handler executes, it INSERTs (or DELETEs) the row in the central agent_destinations table — but it did not project the change into the affected agent's per-session inbound.db.

The agent-runner reads its destination map from the per-session projection. Until the next container spawn (container-runner.ts:118-121 already hydrates on every wake via writeDestinations), the running agent sees a stale map, so the freshly-approved destination silently drops with unknown destination at send_message time. This is the projection-invariant called out at the top of src/modules/agent-to-agent/db/agent-destinations.ts.

The fix

A small helper projectDestinationsToSessions(agentGroupId) lives in src/cli/resources/destinations.ts and is called after both the add INSERT and the remove DELETE. It iterates getSessionsByAgentGroup(agentGroupId) and invokes writeDestinations(agentGroupId, session.id) for each — exactly the pattern container-runner uses on spawn. Failures are caught per-session and surfaced as a WARN log with session id + err (no silent best-effort).

The same handler is hit by both the direct-host call path and the approval-execution path, because dispatch.ts's cli_command approval handler re-enters dispatch() with caller: 'host' and lands on the same cmd.handler(parsed, ctx) line. So the fix at the handler level covers both surfaces.

Reviewer call-outs

(a) Earlier "restart doesn't fix it" report attributed to wrong-side restart. A 2026-05-13 thread suggested restarting the agent didn't pick up the new destination. After tracing it: container-runner.ts:118-121 already calls writeDestinations on every container spawn, and spawnContainer is the bottom of both restart branches in ncl groups restart (src/cli/resources/groups.ts:62-110). The earlier symptom was consistent with restarting the wrong side (the requester, not the receiver agent) — restarting the receiver agent's container has always rehydrated its inbound.db. No code change required there; flagging here so reviewers don't re-litigate.

(b) remove symmetry is the same bug class. A successful destinations remove left the local_name still resolvable inside the running container — the central row was gone but the projection still had it. The fix calls projectDestinationsToSessions after the DELETE too. Without that, a removed destination becomes a stale grant until the next container spawn.

(c) Helper lives in the resource file, not the agent-to-agent module. Two reasons:

  • The agent-to-agent module is optional (some installs don't have the table); putting host-orchestration glue inside it would mean importing it unconditionally from the CLI. The resource file already lives in src/cli/resources/ which is loaded by every host build, and guards on hasTable('agent_destinations') + a dynamic await import(...) keeps the module optional.
  • The helper is plumbing for the CLI surface specifically. writeDestinations is the right primitive to live in the agent-to-agent module; the loop over sessions + per-session error handling + log surface are CLI-handler concerns.

Verification

  • pnpm typecheck → 0 errors
  • pnpm test → 31 files, 328 tests, all passing
  • New destinations.test.ts invokes dispatch(..., { caller: 'host' }) — identical re-entry path to the approval handler in dispatch.ts:181-191 — with two active sessions on the source agent group, and asserts the projected destinations row lands in every session's inbound.db after add, and is cleared after remove. Two sessions catches the common regression shape where a fix only updates the "latest" session.

For Skills

  • SKILL.md contains instructions, not inline code (code goes in separate files)
  • SKILL.md is under 500 lines
  • I tested this skill on a fresh clone

(N/A — this is a source-code fix, not a skill.)

…dd/remove

The `destinations add` and `destinations remove` custom ops in the admin
CLI INSERT/DELETE rows in the central `agent_destinations` table, but
did not project the change into running sessions' `inbound.db`. The
agent-runner container reads its destination map from the per-session
projection, so until the next container spawn (`container-runner.ts`
hydrates on every wake), the running agent saw a stale map — explaining
the "dropped: unknown destination" symptom after a fresh `ncl
destinations add` even though the central row was clearly committed.

Same handler runs for both the direct-host path and the approval-execution
path because the `cli_command` approval handler in `dispatch.ts` re-enters
`dispatch()` as `caller: 'host'`, so the fix at the handler level covers
both surfaces.

Helper iterates over `getSessionsByAgentGroup(agentGroupId)` (every
active session for the affected agent), guarded by `hasTable('agent_destinations')`
and a lazy dynamic import of `writeDestinations` to keep the agent-to-agent
module optional. Per-session try/catch keeps one bad session from killing
the whole projection; failures are logged at WARN with session id + error.

Regression test invokes the dispatcher with `caller: 'host'` (the same
re-entry the approval handler uses after admin approves), with two active
sessions on the source agent group, and asserts the `destinations` row
lands in every session's inbound.db after `add` and is cleared after `remove`.

Fixes #2465
@glifocat glifocat requested a review from gavrielc as a code owner May 16, 2026 08:47
@glifocat glifocat added the cli label May 16, 2026
@glifocat glifocat requested a review from gabi-simons as a code owner May 16, 2026 08:47
@glifocat glifocat added the Type: Bug Something isn't working label May 16, 2026
@github-actions github-actions Bot added the PR: Fix Bug fix label May 16, 2026
@glifocat glifocat marked this pull request as draft May 16, 2026 08:49
@github-actions github-actions Bot added the follows-guidelines PR was created using the current contributing template label May 16, 2026
@glifocat glifocat marked this pull request as ready for review May 16, 2026 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli follows-guidelines PR was created using the current contributing template PR: Fix Bug fix Type: Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

destinations: approval-processed ncl destinations add doesn't populate receiver's session-local inbound.db (restart doesn't recover)

1 participant