refactor: redesign gateway and runtime middleware chain

[furukama]

Summary

• add the shared middleware contract and ordered chain execution model
• migrate gateway pre-agent work into session, media, memory, prompt assembly, and audit middleware
• extract gateway post-agent work into afterAgent middleware for tool analysis, side effects, completion, plugin lifecycle, and response finalization
• replace container runtime extensions with beforeTool/afterTool middleware and explicit tool decisions
• add middleware chain and gateway/container regression coverage

Testing

• npm run typecheck
• npm run lint
• npm --prefix container run lint
• ./node_modules/.bin/vitest run tests/middleware-chain.test.ts tests/container-runtime-middleware.test.ts tests/gateway-service.audit.test.ts tests/gateway-service.audio-transcription.test.ts tests/gateway-service.plugins.test.ts tests/gateway-service.memory-citations.test.ts tests/gateway-service.skill-observation.test.ts tests/gateway-service.fullauto.test.ts

[Copilot]

Pull request overview

This PR refactors the gateway and container runtime execution flow to use a shared, ordered middleware-chain model, splitting “pre-agent” and “post-agent” responsibilities into explicit middleware phases.

Changes:

• Introduces a shared middleware contract (Middleware, phases, results, tool decisions) and an ordered MiddlewareChain executor with timeouts.
• Refactors handleGatewayMessage() to run gateway turn preparation and post-processing through a dedicated middleware chain (session/media/memory/prompt/audit + afterAgent processing).
• Updates the container runtime tool execution path to use before/after tool middleware hooks (including loop detection + security checks) and adds regression tests.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/middleware-chain.test.ts	Adds unit coverage for ordered state updates, halting, and tool argument modification/denial behavior.
tests/container-runtime-middleware.test.ts	Adds regression tests for container tool-deny patterns (bash exfiltration + loop guard).
src/middleware/types.ts	Defines the shared middleware types/phases and tool decision contract.
src/middleware/chain.ts	Implements ordered middleware execution with timeout handling and result application.
src/gateway/gateway-service.ts	Refactors gateway turn handling into beforeAgent/afterAgent middleware stages.
container/src/index.ts	Routes tool execution through container middleware hooks and uses rewritten args when applicable.
container/src/extensions.ts	Replaces runtime “extensions” with middleware + middleware chain, including security and loop-detection middleware.

---

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

[Copilot]

The emitted before_tool_call event sets modified only when decision.action === 'continue', so it will incorrectly report modified: false for the primary mutation path (decision.action === 'modify'). Update this flag to reflect actual argument rewrites (e.g., treat modify as modified, and/or compare original vs final args regardless of action).

Suggested change

	modified:
	decision.action === 'continue' &&
	JSON.stringify(args) !== JSON.stringify(ctx.toolArgs),
	modified: JSON.stringify(args) !== JSON.stringify(ctx.toolArgs),

[Copilot]

This test imports getRuntimeConfig() from src/config/runtime-config, which initializes runtime config on module load (creates/reads config file, starts a watcher unless HYBRIDCLAW_DISABLE_CONFIG_WATCHER is set). Unlike most gateway tests, it doesn’t call setupGatewayTest().setupHome(), so it can write to a developer’s real HOME and keep file watchers open. Consider using setupGatewayTest (or stubbing HOME / disabling the watcher) and/or mocking getRuntimeConfig() to keep the unit test hermetic.

[Copilot]

registerActiveGatewayRequest() is called before the middleware chain runs, but SessionMiddleware can mutate req.sessionId (auto-reset, getOrCreateSession id fixup, workspace reset). That means the active request gets registered/released under the old sessionId, so interruptGatewaySessionExecution()/abort routing won’t work for the actual session and the old map entry may leak. Move registration until after the final sessionId is known (or re-register when sessionId changes) and ensure the abort signal passed to downstream middleware/agent uses the correctly registered entry.

[furukama]

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

[claude]

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

---

Code Review: KISS · YAGNI · DRY · Fail Fast · Secure by Default

• Gather context and read changed files
• Review src/middleware/types.ts and src/middleware/chain.ts
• Review src/gateway/gateway-service.ts
• Review container/src/extensions.ts and container/src/index.ts
• Review test files
• Post comprehensive review

---

The middleware chain abstraction is directionally right, but there are several violations of the principles you asked about. Here they are, ranked by severity.

---

DRY — Duplicate middleware system

The single biggest issue: container/src/extensions.ts re-implements the entire middleware subsystem from scratch — RuntimeMiddleware, RuntimeMiddlewareChain, RuntimeMiddlewarePhase, RuntimeToolDecision, withTimeout, resolveTimeoutMs, and the identical default timeout map — all duplicated from src/middleware/chain.ts and src/middleware/types.ts.

src/middleware/chain.ts:20-33  ←──┐  identical withTimeout
container/src/extensions.ts:134-147 ──┘

src/middleware/chain.ts:35-52  ←──┐  identical resolveTimeoutMs
container/src/extensions.ts:149-162 ──┘

src/middleware/types.ts:32-36  ←──┐  identical ToolDecision union
container/src/extensions.ts:36-40 ──┘

DEFAULT_MIDDLEWARE_TIMEOUTS_MS  ←──┐  identical values
DEFAULT_TIMEOUTS_MS ──────────────┘

The divergence exists for a real reason — the container's runPhase swallows errors while the gateway's propagates them — but that's a one-line behavioural difference that doesn't justify two full implementations. The shared types package exists; the container should import from it or from a shared utility, not redefine everything. Fix this →

---

YAGNI — Dead phases in the gateway chain

MiddlewarePhase declares six phases: beforeAgent, beforeModel, afterModel, beforeTool, afterTool, afterAgent. MiddlewareChain exposes runBeforeModel() and runAfterModel(). DEFAULT_MIDDLEWARE_TIMEOUTS_MS has entries for both. Yet no gateway middleware implements beforeModel or afterModel, and handleGatewayMessage never calls runBeforeModel() or runAfterModel().

GatewayCommandMiddlewareChain has the same problem from the other direction: it builds a chain of exactly one middleware (CommandSessionMiddleware). A chain of one is a function call with extra ceremony. Fix this →

---

KISS — GatewayMiddlewareState is a god object

GatewayMiddlewareState (gateway-service.ts:3785) has 35+ optional fields. Every middleware reads from and writes to this shared bag. The middleware ordering is therefore implicit: MemoryMiddleware must run after SessionMiddleware because it reads ctx.state.session, PromptAssemblyMiddleware must run after MemoryMiddleware because it reads ctx.state.history, and so on. None of this ordering is enforced by the type system — it's enforced by array position in buildGatewayMiddlewareChain.

If middleware order changes, you get a runtime throw new Error('Memory middleware requires session and agentId.') rather than a compile-time error. The guards inside each middleware are doing the job the type system should be doing. This design also makes it very hard to understand what any single middleware actually does without tracing all of its reads and writes through the shared state bag.

A simpler shape would be to thread explicit typed inputs and outputs (or narrow context subtypes) between phases rather than a single mutable bag. Fix this →

---

KISS — as never casts and the Exclude<MiddlewarePhase, 'beforeTool'> smell

chain.ts:92:

hook(current as never)

This is a type-system escape hatch that hides a real design problem: beforeTool returns ToolDecision instead of MiddlewareResult, so it can't go through the same runPhase path. The fix is to exclude beforeTool from the phase union at the type level and have a separate method signature — but the current code hides this with as never. The Exclude<MiddlewarePhase, 'beforeTool'> parameter on runPhase signals the smell but doesn't eliminate it. Fix this →

---

Fail Fast — registerActiveGatewayRequest called before SessionMiddleware mutates req.sessionId

gateway-service.ts:4968-4971:

const activeGatewayRequest = registerActiveGatewayRequest({
  sessionId: req.sessionId,   // ← registered under initial sessionId
  abortSignal: req.abortSignal,
});
// ...
preparedContext = await gatewayMiddlewareChain.runBeforeAgent(...)
// SessionMiddleware runs here and can reassign req.sessionId three times:
//   1. agent rebound → reboundSession.id
//   2. getOrCreateSession → session.id
//   3. workspace reset → rotated.session.id

interruptGatewaySessionExecution() will look up abort routing by the session ID it was registered under, but after SessionMiddleware runs the actual session ID may be different. The registered entry may never be cleaned up, and abort signals sent to the new session ID won't find the registered request. This is the correctness bug Copilot also flagged, and it's a fail-fast violation: the registration should happen after the session ID is finalized. Fix this →

---

Secure by Default — modified flag always false for modify decisions

container/src/extensions.ts:522-524 (also flagged by Copilot):

modified:
  decision.action === 'continue' &&
  JSON.stringify(args) !== JSON.stringify(ctx.toolArgs),

When a middleware returns { action: 'modify', args: ... }, the args are rewritten in ctx.toolArgs but modified is always false because the condition requires action === 'continue'. Audit/observability consumers of the before_tool_call event will never see modified: true on the primary mutation path. The fix (as Copilot suggested) is:

modified: JSON.stringify(args) !== JSON.stringify(ctx.toolArgs),

Fix this →

---

Secure by Default — parseArgs silently returns {} on invalid JSON

container/src/extensions.ts:419-429: If argsJson is malformed, parseArgs returns {}. The security middleware's bash check then tests String({}.command || '') === '' — which passes all pattern checks. The tool itself will subsequently receive empty args. This might be fine (the tool would likely fail cleanly), but it means a malformed args payload silently bypasses all content inspection. At minimum, the decision should be deny rather than a silent empty-args pass-through on parse failure. Fix this →

---

DRY — Repeated state-extraction boilerplate in every middleware

Every gateway middleware class starts with 6–10 lines of the same pattern:

const session = ctx.state.session;
const agentId = String(ctx.state.agentId || '').trim();
const model = String(ctx.state.model || '').trim();
const workspacePath = String(ctx.state.workspacePath || '').trim();

This is repeated across MemoryMiddleware, PromptAssemblyMiddleware, AuditMiddleware, ToolAnalysisMiddleware, and others. Small in isolation but it accumulates.

---

Test hermetics — middleware-chain.test.ts leaks real filesystem state

tests/middleware-chain.test.ts:3 imports getRuntimeConfig() directly, which reads/writes to ~/.hybridclaw/config.json and starts a file watcher unless HYBRIDCLAW_DISABLE_CONFIG_WATCHER is set. Unlike other gateway tests, it doesn't call setupGatewayTest().setupHome(). This can write to a developer's real HOME directory and leak open handles — exactly the issue Copilot flagged. Mock getRuntimeConfig() or stub HOME for this test. Fix this →

---

What's working well

• The beforeTool fail-safe in the container: a middleware error returns deny rather than allowing the call through. Correct secure-by-default behavior.
• Security middleware is unconditionally enabled (isEnabled: () => true). Good.
• runBeforeTool in MiddlewareChain correctly threads modified args through subsequent middleware (the modify action continues rather than short-circuiting), and only terminates early on deny/abort-turn. The semantics are right.
• Timeouts per phase with per-middleware overrides is a clean design.

---

Summary of issues by principle:

Principle	Issues
DRY	Entire middleware system duplicated in container; repeated state-extraction boilerplate
YAGNI	beforeModel/afterModel phases defined but unused in gateway; single-middleware command chain
KISS	GatewayMiddlewareState god object with implicit ordering; as never type cast
Fail Fast	registerActiveGatewayRequest uses pre-mutation sessionId
Secure by Default	modified flag always false for modify decisions; parseArgs({}) bypasses security checks