Skip to content

[codex] Add escalation routing for out-of-bound actions#641

Merged
furukama merged 9 commits intomainfrom
codex/issue-587-escalation-routing
Apr 28, 2026
Merged

[codex] Add escalation routing for out-of-bound actions#641
furukama merged 9 commits intomainfrom
codex/issue-587-escalation-routing

Conversation

@furukama
Copy link
Copy Markdown
Contributor

@furukama furukama commented Apr 28, 2026

Summary

Closes #587
Adding escalation routing for actions that exceed an agent's configured autonomy level.

  • Adds per-agent escalationTarget configuration with channel and recipient, including runtime normalization, agent registry support, JSON config command handling, and SQLite persistence via schema v25.
  • Threads escalation targets through gateway execution, host/container runners, container approval evaluation, pending approvals, and tool execution audit data.
  • Pauses out-of-bound actions by promoting them to explicit approval requests, and enriches escalation prompts with the proposed action and classifier reasoning.
  • Routes pending approval notifications to the configured target channel/recipient when available, while keeping the originating action paused until approval resolves.
  • Emits escalation.decision audit events for escalation routes.

Validation

  • npm run lint
  • tsc --noEmit --noUnusedLocals --noUnusedParameters -p container/tsconfig.json
  • vitest run tests/approval-policy.test.ts tests/audit-events.test.ts tests/agent-registry.test.ts tests/hybridai-skills-command.test.ts
  • vitest run --configLoader runner --config vitest.integration.config.ts tests/config-reload.integration.test.ts
  • git diff --check

Notes

A full npm run test:unit was attempted. The new DB fixture issue it exposed was fixed. The remaining reproduced failure was tests/plugin-manager.singleton.test.ts timing out while loading this machine's configured external plugins from the real runtime home, which appears unrelated to this change.

@furukama furukama marked this pull request as ready for review April 28, 2026 15:41
Copilot AI review requested due to automatic review settings April 28, 2026 15:41
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds per-agent escalation routing so out-of-bound tool actions (relative to configured autonomy) are paused and promoted into explicit approval requests, with notifications routed to a configured channel/recipient and new audit coverage for escalation decisions.

Changes:

  • Introduces per-agent escalationTarget config (normalization, registry + CLI JSON updates, SQLite persistence via schema v25).
  • Threads escalation targets through gateway → runners → container approval evaluation → pending approvals + notifications.
  • Adds escalation.decision structured audit events and enriches approval prompts with proposed action + classifier reasoning.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/hybridai-skills-command.test.ts Isolates runtime HOME/DATA_DIR for tests to avoid leaking local machine config.
tests/config-reload.integration.test.ts Adds runtime-config normalization test for per-agent escalation targets.
tests/audit-events.test.ts Verifies escalation.decision audit event payload + metadata threading.
tests/approval-policy.test.ts Covers autonomy out-of-bound escalation behavior + prompt enrichment.
tests/agent-registry.test.ts Ensures escalation targets normalize + persist through registry/SQLite migration v25.
src/types/execution.ts Adds EscalationTarget type and threads it into ToolExecution + PendingApproval.
src/types/container.ts Extends ContainerInput with escalationTarget.
src/memory/db.ts Bumps schema to v25 and persists agents.escalation_target JSON.
src/infra/host-runner.ts Passes escalation target into container input; parses it from approval progress.
src/infra/container-runner.ts Passes escalation target into container input; parses it from approval progress.
src/gateway/text-channel-commands.ts Stores pending approvals under escalation recipient user id when present.
src/gateway/gateway.ts Adds routing helpers; suppresses in-channel approval UI when routed; supports proactive channel override.
src/gateway/gateway-types.ts Extends proactive message callback payload with optional channelId.
src/gateway/gateway-chat-service.ts Routes escalation approvals via proactive callback; passes resolved agent escalation target into execution.
src/gateway/fullauto-runtime.ts Updates proactive message payload type to include optional channelId.
src/gateway/chat-approval.ts Threads escalationTarget through extracted gateway approval events.
src/config/runtime-config.ts Normalizes/clones per-agent escalation target in runtime config.
src/audit/audit-events.ts Emits new escalation.decision audit event for non-none routes.
src/agents/agent-types.ts Adds escalation target type + normalize/clone/equals helpers.
src/agents/agent-runtime-config.ts Includes escalation target equality in agent config diffing.
src/agents/agent-registry.ts Normalizes/defaults/fingerprints/escalation target and syncs it to DB.
src/agents/agent-config-command.ts Adds CLI JSON config support/validation for escalationTarget.
src/agent/executor-types.ts Threads escalation target through executor request.
container/src/types.ts Adds EscalationTarget and threads it through container types.
container/src/index.ts Passes escalation target into approval evaluation and tool execution metadata.
container/src/approval-policy.ts Implements out-of-bound-by-autonomy escalation + prompt enrichment + target threading.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/gateway/gateway-chat-service.ts Outdated
Comment thread src/infra/host-runner.ts Outdated
Comment thread src/infra/container-runner.ts Outdated
Comment thread tests/hybridai-skills-command.test.ts
@furukama
Copy link
Copy Markdown
Contributor Author

@claude Do a thorough review of this PR in terms of: KISS, YAGNI, DRY, Fail Fast, Secure by Default and performance/speed

@claude
Copy link
Copy Markdown

claude Bot commented Apr 28, 2026

Claude encountered an error —— View job


I'll analyze this and get back to you.

@furukama
Copy link
Copy Markdown
Contributor Author

@claude Do a thorough review of this PR in terms of: KISS, YAGNI, DRY, Fail Fast, Secure by Default and performance/speed

@claude
Copy link
Copy Markdown

claude Bot commented Apr 28, 2026

Claude finished @furukama's task in 2m 24s —— View job


PR Review: KISS, YAGNI, DRY, Fail Fast, Secure by Default, Performance

Tasks:

  • Gather context and understand request
  • Read key changed files
  • Analyze all dimensions
  • Post final review

Overall the feature is well-scoped and the threading is thorough. The issues below range from a silent behavior change to genuine duplication and one security concern worth discussing.


DRY — Significant violations

1. normalizeParsedEscalationTarget is copy-pasted identically in src/infra/host-runner.ts and src/infra/container-runner.ts — byte-for-byte the same function. Extract it to a shared utility (e.g. in src/types/execution.ts alongside EscalationTarget) and import it in both runners. Fix this →

2. EscalationTarget declared in two places in the main codebase. src/agents/agent-types.ts defines AgentEscalationTarget and src/types/execution.ts defines EscalationTarget — they are structurally identical ({ channel: string, recipient: string }). agent-types.ts could simply import and re-export the type from execution.ts (or vice versa), eliminating the second definition and the need for normalizeAgentEscalationTarget vs normalizeEscalationTarget to diverge. (The container's copy in container/src/types.ts is unavoidable given the separate build.)

3. Normalization logic implemented 5+ times across the codebase:

  • normalizeAgentEscalationTarget in agent-types.ts
  • normalizeEscalationTarget in gateway.ts (returns null)
  • normalizeParsedEscalationTarget in host-runner.ts
  • normalizeParsedEscalationTarget in container-runner.ts
  • normalizeEscalationTarget in container/src/approval-policy.ts
  • getPendingApprovalEscalationChannel in gateway-chat-service.ts (implicit)

All of them trim two strings and return falsy if either is empty. The only variation is the return type (null vs undefined). Consolidating these into one exported function in execution.ts (or a small shared util) would halve the surface area of this feature.

4. Approval-routing block triplicated in gateway.ts. Discord, Slack, and MS Teams each copy the pattern of computing approvalUserId, routedTarget, storedPrompt, flipping the sendApprovalNotification guard, and posting a formatRoutedApprovalNotice. The three blocks are ~30 lines each. A handlePendingApprovalRouting(params) helper extracted before this PR would make each integration a 2-line call; as-is the PR extends existing duplication instead of reducing it.


KISS — Moderate issues

5. Triple normalization per approval in gateway.ts. getApprovalRecipientUserId calls normalizeEscalationTarget, then approvalRoutesAwayFromChannel calls it again, then formatRoutedApprovalNotice calls it a third time — all on the same pendingApproval object. Compute it once at the top of the block and pass the result down.

6. processRequest now takes 7 positional parameters (including the new escalationTarget). This was pre-existing complexity, but this PR adds the 7th argument at 4 call sites and makes the function signature even harder to read. A params object would serve better here; at minimum the PR is an opportunity to note that the signature needs addressing.

7. Dead fallback in formatEscalationRouteNotice (gateway-chat-service.ts). The function returns approval.prompt when target is falsy, but its only caller (routeEscalationApproval) already returns early if targetChannel is empty — meaning escalationTarget is always present when this function is reached. The if (!target) return approval.prompt branch is unreachable.

8. Dead fallback in formatRoutedApprovalNotice (gateway.ts). Same pattern — the function is only called when routedTarget is truthy (already normalized non-null), then immediately calls normalizeEscalationTarget a second time internally. The if (!target) branch inside is unreachable.

9. cloneAgentEscalationTarget is unnecessary abstraction. AgentEscalationTarget is a flat 2-string object. cloneAgentEscalationTarget is literally value ? { ...value } : undefined. Unlike cloneAgentCv (which has nested optional fields), this helper adds no value and is a YAGNI function. Fix this →


Fail Fast — One silent behavior change

10. low-stakes-autonomous policy change is buried in this PR. The original logic:

if (stakes === 'high' && baseTier !== 'red') { baseTier = 'red'; }
else if (stakes === 'medium' && baseTier === 'green') { baseTier = 'yellow'; }

The new logic:

if (stakes !== 'low') { outOfBoundByAutonomy = true; baseTier = 'red'; }

For medium-stakes actions under low-stakes-autonomous: previously this produced a yellow tier (requiring confirmation but not a hard block); now it triggers outOfBoundByAutonomy = true and red tier, which will escalate. This is a policy tightening, and it's a meaningful behavior change that isn't called out in the PR description or tests. The test in approval-policy.test.ts covers the outOfBoundByAutonomy path but doesn't explicitly assert the old medium stakes → yellow path is gone. Worth an explicit callout, or at minimum a comment in the code explaining the intentional policy change. Fix this →

11. text-channel-commands.ts uses ?.recipient without normalization.

userId: pendingApproval.escalationTarget?.recipient || userId

This relies on the upstream runner having already validated and trimmed recipient. That's true on the happy path (runners call normalizeParsedEscalationTarget), but inconsistent with every other access site that either normalizes or guards. It should either use the already-exported normalizer or just trust the type (since parse-time validation already guarantees non-empty strings).


Secure by Default — One concern

12. No authorization check before routing approvals to an arbitrary channel. The escalationTarget.channel value flows from agent config through the container and is used directly as the destination for a proactive message in routeEscalationApproval. A misconfigured or deliberately crafted agent config could route approval notifications — which now include the proposed action text and classifier reasoning — to any channel the bot is in. There's no check that:

  • The requesting user/admin is authorized to configure escalation to that channel, or
  • The target channel has the appropriate trust level for receiving approval data.

For an internal-teams deployment this is likely fine, but for multi-tenant or untrusted-admin setups this is a confused-deputy / data-leak vector worth documenting explicitly. At minimum, consider logging the source agent ID alongside the target channel so the audit trail makes it traceable.


Performance/Speed — Minor

13. Triple normalizeEscalationTarget call per approval in gateway.ts (already noted under KISS — compute once).

14. formatEscalationRouteNotice creates a 2-element array just to .join('\n\n'). Template literal is simpler and avoids the allocation:

return `Escalation for ${target.recipient} on ${target.channel}.\n\n${approval.prompt}`;

What's done well

  • The normalizeEscalationTarget/normalizeParsedEscalationTarget guards in the runners (rejecting non-object, non-string fields) correctly address the Copilot review comment about unvalidated shape passthrough.
  • The outOfBoundByAutonomy flag cleanly separates the "exceeds autonomy" condition from tier logic, making the approval flow easier to follow.
  • Schema migration v25 uses the existing addColumnIfMissing helper correctly.
  • Test coverage is meaningful: normalization, round-trip registry/DB, audit event shape, and prompt enrichment are all exercised.
  • The onProactiveMessage warning log (also suggested by Copilot) is present and includes structured fields.

Summary: The feature works and is well-tested, but there are 3 DRY violations worth fixing before merge (identical runners function, duplicate type, 5× normalization logic), one silent policy change that needs callout, and one security consideration for multi-tenant deployments. The YAGNI/KISS issues (dead fallbacks, unnecessary clone helper) are low-risk cleanup.

@furukama furukama merged commit a70169e into main Apr 28, 2026
5 checks passed
@furukama furukama deleted the codex/issue-587-escalation-routing branch April 28, 2026 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Foundation F8.3] Escalation routing on out-of-bound actions

2 participants