fix(transfer): align controller route binding + prune stale handover targets - #217
Merged
Conversation
…te convention
The route `/api/v1/agents/{id}/transfer` uses path variable `{id}`,
but `AgentTransferController::transferPrincipal()` declared
`int $agentId, Request $request`. `Router.php:117` binds path
variables to controller parameters by NAME, so every transfer call
has been 500'ing since the route landed in `1b9e235` (the
principals migration, Aug 23). The unit test
`AgentTransferControllerTest` calls `$controller->transferPrincipal(1,
$request)` directly and never goes through the router — so the bug
was invisible until the operator tried to transfer an agent from
the settings page and got `LogicException: Required parameter
"agentId" has no matching route variable.` back as a 500.
Switch the controller to the convention used by
`AgentToolController`, `AgentOverrideController`, and
`AgentPictureController`: accept `Request $request` only, read the
id from `$request->attributes->get('id', 0)`. No behavior change
for valid ids; the service layer still 404s on a missing agent via
the existing `RuntimeException → notFound` translation in
`runTransfer()`.
Why not rename the route to `{agentId}`? Every other agent route in
`RouteDefinitions.php` uses `{id}` (`/api/v1/agents/{id}/tools/...`,
`/api/v1/agents/{id}/picture/...`, `/api/v1/agents/{id}/templates/...`,
etc.) — that variable name is the codebase convention. Renaming
this one would diverge for no benefit.
Tests:
- `AgentTransferControllerTest`: rewrite the six existing tests to
set the id via `Request::$attributes` (constructor arg 3) instead
of passing it as a method argument. Status-code assertions
unchanged.
- `RouterTest`: new `AttributesOnlyTestController` + test
`Router dispatches a route with {id} into a controller that only
declares Request $request` exercises the full Router → Controller
bind for a route shaped exactly like
`/api/v1/agents/{id}/transfer`. This is the test that would have
caught `1b9e235` — it asserts dispatch returns 200 and the
controller observes `attributes.id === 5`. Without the fix, this
test fails with `LogicException` from `Router::dispatch()`.
`composer format`: clean (785 files inspected).
`composer analyse`: clean (753 files, 0 errors).
`composer test:parallel`: 3583 passed (0 failed).
The per-agent override on HandoverTool stores `allowed_target_agents`
as an `int[]` of agent ids. It is keyed on `agent_id`, not
`principal_id`, so when an agent's `principal_id` changes via
transfer, the stored list still references the OLD principal's agents.
The tool-level `sharePrincipal()` gate at
`app/Tools/HandoverTool.php:260` already rejects stale targets at
runtime, but the LLM-facing tool definition still resolves the stored
ids into `"Name (#id)"` strings — meaning the model can only pick
from a stale name list and every handover silently fails until the
operator re-configures the targets.
Add `ToolConfigService::pruneAgentOverrideByPrincipal()`: read the
override, walk every multi-select setting with `resolveAs === 'agent'`
(HandoverTool's `allowed_target_agents` today; new tools that ship
the same setting shape will be picked up for free), resolve each
referenced agent's `principal_id` in a single `Agent::whereIn`, drop
the ones that don't share the new principal, and write the filtered
override back through the existing crypto + merge pipeline.
Hook into `AgentPrincipalService::transferAgent()` so the prune
runs every time `principal_id` changes. The new optional
`?ToolConfigServiceInterface` dependency stays unset in tests that
construct `AgentPrincipalService` without DI (e.g. the unit-level
transfer stubs), so the prune path is silently skipped there — the
runtime `sharePrincipal()` gate remains the final defence.
Layered intra-principal enforcement is unchanged:
1. Picker UI (`?principal_id=N`) — operator sees only same-principal
agents when configuring.
2. `isTargetOnAllowlist()` — defence against tampered payloads.
3. `sharePrincipal()` — runtime gate against principal mismatch.
The prune is purely a UX correctness fix: it keeps the LLM-facing
tool definition honest about what it can target so the model doesn't
have to pick-and-fail.
Tests:
- `tests/Unit/Tools/ToolConfigServiceTest.php`: four new unit tests
on `pruneAgentOverrideByPrincipal` — drops stale ids, no-op when
no override, keeps targets that already share the new principal,
no-op when no agent-id keys exist.
- `tests/Unit/Services/AgentPrincipalServiceTest.php`: new file with
two integration tests on `AgentPrincipalService::transferAgent` —
handover allowlist filters to new principal; non-handover overrides
survive untouched.
`composer format`: clean (786 files, 0 changed).
`composer analyse`: clean (754 files, 0 errors).
`composer test:parallel`: 3589 passed (0 failed).
…er-route-binding Folds the handover-allowlist prune (#218) into the transfer route-binding fix (#217) so this session lands as a single backend PR per CLAUDE.md. Both fixes touch the agent transfer flow and were originally split across two PRs by mistake — the route-binding bug was uncovered first and fixed immediately, then the allowlist prune followed once the transfer endpoint was working end-to-end. The user has now asked to collect them into one PR.
fabeat
changed the title
…sues)
SonarCloud flagged three issues on the prune method I added to
`ToolConfigService` in `13280d6`:
- php:S1448 (ToolConfigService): 21 methods > 20 authorised
- php:S1142 (ToolConfigService.php:502): prune method has 5 returns > 3
- php:S3776 (ToolConfigService.php:502): cognitive complexity 19 > 15
All three stem from the same method living on a class that was already
at the 20-method ceiling. Move the prune logic to
`AgentPrincipalService` (its only caller — the `transferAgent()`
path) and split the work into three private helpers:
- `loadExistingHandoverOverride(int): ?array` — row read +
empty-row-and-unwired-service null-sentinel (one of the early
returns goes through here)
- `collectAgentIdSettingKeys(string): list<string>` — schema walk
(the second early-return short-circuit)
- `filterOutCrossPrincipalTargets(array&, list, int): int` — DB
query + filter + counter
The public `pruneHandoverAllowlist()` drops to three returns and a
low cognitive complexity. `ToolConfigService` is back to its 20-method
limit (S1448 clear).
Tests:
- `tests/Unit/Tools/ToolConfigServiceTest.php`: removed the four
`pruneAgentOverrideByPrincipal` unit tests (method no longer
exists here).
- `tests/Unit/Services/AgentPrincipalServiceTest.php`: added the four
tests as a `describe('AgentPrincipalService::pruneHandoverAllowlist')`
block — same assertions, retargeted at the new home. The two
pre-existing integration tests (`transferAgent` happy path + no-op)
are unchanged and continue to cover the wiring.
No DI changes required — `AgentPrincipalService` already accepted
`?ToolConfigServiceInterface` and now uses it to read/write the
override (instead of forwarding to a helper method).
`composer format`: clean (786 files, 0 changed).
`composer analyse`: clean (754 files, 0 errors).
`composer test:parallel`: 3589 passed (0 failed; net 0 because the
four removed tests were re-added on the new class).
spora-light-review flagged three minor factual inaccuracies on the new `AgentPrincipalService` helpers, all behavioural no-ops: 1. `filterOutCrossPrincipalTargets` docblock claimed "Two short-circuits" — actually only the no-referenced-ids guard lives in this method; the schema-empty short-circuit is an earlier return in `pruneHandoverAllowlist`. Reworded to a single short-circuit and pointed at the common case it skips (the `Agent::whereIn` round-trip). 2. `transferAgent` said "the cost is at most one SELECT" for a no-op transfer. The prune path actually fires two SELECTs — `getRawAgentOverride` + `Agent::whereIn` — before short-circuiting on `$removed === 0`. Still bounded and cheap, but the claim was off. Updated to "two SELECTs at most" and called out exactly what they are. 3. `loadExistingHandoverOverride` docblock listed two null cases (no row, no service wired) but the method also returns null when a row exists with empty decoded settings. Added the third case so all three null paths are documented. Lint clean, tests pass (3589).
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two bugs surfaced together while testing the transfer endpoint end-to-end. Both fixed in this PR per
CLAUDE.md(one PR per repo + session):1.
POST /api/v1/agents/{id}/transferreturned 500The route was registered with path variable
{id}but the controller method declaredint $agentId. The Router binds path variables to controller parameters by name (Router.php:117), so the missing variable name tripped the dispatch and surfaced as a 500. Latent since1b9e235(the principals migration, Aug 23) — the existing unit test calls$controller->transferPrincipal(1, $request)directly and never exercises the full Router → Controller bind, so CI never caught it.Fix:
AgentTransferController::transferPrincipal()now acceptsRequest $requestonly and reads the id from$request->attributes->get('id', 0). Matches the existing convention inAgentToolController(line 42/69),AgentOverrideController, andAgentPictureController(line 56/125). Route path unchanged.2. After the transfer started working, handover settings went stale
HandoverTool's per-agent override (allowed_target_agents) is keyed onagent_id, not onprincipal_id. Whenprincipal_idchanges via transfer, the stored list still references the old principal's agents. The runtimesharePrincipal()gate atapp/Tools/HandoverTool.php:260already rejected stale targets at execute time (security is intact — a foreign agent can never be reached), but the LLM-facing tool definition still resolved the stored ids into"Name (#id)"strings, so the model was given a stale name list and every handover silently failed until the operator manually re-configured the targets.Fix: new
ToolConfigService::pruneAgentOverrideByPrincipal()reads the override, walks every multi-select setting withresolveAs === 'agent'(today that's justHandoverTool::allowed_target_agents, but the loop is generic for new tools with the same shape), resolves each referenced agent'sprincipal_idin a singleAgent::whereInquery, drops the ones that don't share the new principal, and writes the filtered override back through the existing crypto + merge pipeline. Hooked intoAgentPrincipalService::transferAgent()so the prune runs every timeprincipal_idchanges.Layered intra-principal enforcement (unchanged)
?principal_id=N) — operator sees only same-principal agents when configuring.isTargetOnAllowlist()— defence against tampered payloads.sharePrincipal()— runtime gate against principal mismatch.The prune sits between layers 1 and 3 — purely a UX correctness fix so the LLM picks from honest names.
Tests
For #1 (route binding)
tests/Unit/Http/AgentTransferControllerTest— six existing tests rewritten to set the id viaRequest::$attributes(constructor arg 3) instead of passing it as a method argument. Status-code assertions unchanged.tests/Feature/RouterTest— newAttributesOnlyTestController+ testRouter dispatches a route with {id} into a controller that only declares Request $request. Registers/api/v1/agents/{id}/transferexactly as production does. This is the test that would have caught1b9e235— pinning down the convention so any future controller that drifts back to a named$agentIdparameter fails fast in CI.For #2 (handover prune)
tests/Unit/Tools/ToolConfigServiceTest— four new unit tests onpruneAgentOverrideByPrincipal:tests/Unit/Services/AgentPrincipalServiceTest— new file with two integration tests onAgentPrincipalService::transferAgent:Verification
composer format— clean (786 files inspected, 0 changed)composer analyse— clean (754 files, 0 errors)composer test:parallel— 3589 passed, 0 failedRelease
No tag proposed — patch-level fixes to existing functionality. Can ride the next
v0.18.xrelease or fold into a dedicated patch.