refactor(space): drop the goal-owner fallback agent - #4497
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Review status
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![hyperneo-ai-test[bot]](https://avatars.githubusercontent.com/in/4875581?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A goal with no owner row now resolves to no_recipient instead of routing to a stand-in agent. The fallback existed because every space was created with a coordinator, so there was always somebody to ping; spaces no longer pre-create an agent, and picking an arbitrary active agent as the goal's reviewer is a guess rather than a guarantee. Removes the fallback_agent decision arm, getFallbackAgent on both agent repositories, and the coordinator default in create_goal's owner resolution. Goal outcome wakes and outcome claims now admit the resolved owner only: a degraded owner no longer hands authority to another agent. GoalDetailPanel drops the fallback branch and renders the unowned state it already had.
69a6b24 to
553d6ad
Compare
lsm
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
There was a problem hiding this comment.
Approved.
Engine claude (glm-5.3-flash) · Reviewed commit 553d6ad · Trigger: push
Anchored outside the PR's changes — usually pre-existing; blocking severities here still gate, and defect provenance is the reviewer's judgment, not a verified claim.
docs/rpc-message-inventory.md:358— P2 — RPC inventory still lists removed coordinator_fallback arm
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 553d6ad218
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The roadmap and the launcher RFC both described a coordinator fallback for goals with no usable owner. That fallback no longer exists.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ece56862f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The MCP description told agents they could discover notifications they are the authorized fallback for. Discovery is owner-only, and that text goes straight to the model. Also reconciles the MC2/MC3 minimal-core sections of the roadmap, which still required the fallback that MC1 no longer has.
goal-owner-resolution no longer has a coordinator_fallback arm.
|
Followed up on Devin's stranded-notification finding in #4508 — owner assignment now re-runs the space's pending outcome redelivery, covering both the RPC and MCP assignment surfaces. |
There was a problem hiding this comment.
Approved.
Engine claude (glm-5.3-flash) · Reviewed commit 717fded · Trigger: push
Anchored outside the PR's changes — usually pre-existing; blocking severities here still gate, and defect provenance is the reviewer's judgment, not a verified claim.
docs/rpc-message-inventory.md:358— P2 — Inventory still lists coordinator_fallback in the union
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 717fded95d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (resolution?.action !== 'resolved') { | ||
| log.warn(`Goal outcome wake has no active owner for notification "${notification.id}"`); | ||
| return false; |
There was a problem hiding this comment.
When owner resolution is degraded or no_recipient, this now drops the wake, and goal-service.ts:722 also denies agent claims; however, the runtime-injected packages/prompts/src/agents/long-horizon/owner-review-contract.md:15 and built-in coordinator prompt at packages/prompts/src/agents/long-horizon/coordinator.md:4 still tell the Space Manager that these wakes arrive and are claimable. Coordinator sessions will therefore pursue a recovery path that no longer exists, so both agent-facing contracts and their template test need to be updated with this behavior change.
Useful? React with 👍 / 👎.
| conflicts: SpaceGoalOwnerCandidate[]; | ||
| } | ||
| | { action: 'coordinator_fallback'; coordinatorAgentId: string } | ||
| | { action: 'no_recipient' }; |
There was a problem hiding this comment.
The audited RPC inventory still documents SpaceGoalOwnerResolution for spaceGoal.assignOwner as including coordinator_fallback at docs/rpc-message-inventory.md:358. After this deletion neither the shared response type nor the handler can emit that arm, so the repository's stated client/server contract is now inaccurate and should be updated alongside the wire-type change.
Useful? React with 👍 / 👎.
| if (resolution?.action !== 'resolved') { | ||
| log.warn(`Goal outcome wake has no active owner for notification "${notification.id}"`); | ||
| return false; |
There was a problem hiding this comment.
If a task completes while its goal is unowned, this leaves the notification pending but stops delivery; assigning a real owner later through either ownership path only publishes spaceGoal.ownerChanged (space-goal-handlers.ts:159-160 and space-agent-tools.ts:1515-1522) and no backend subscriber retries the notification. Recovery otherwise runs only at daemon startup or Space resume, while inactivity watchdog configurations default to disabled, so the newly assigned owner receives no outcome wake during normal operation. This does not require restoring the rejected coordinator fallback—re-drive the pending notifications when an actual owner becomes resolved.
Useful? React with 👍 / 👎.
| if (resolution?.action !== 'resolved') { | ||
| log.warn(`Goal outcome wake has no active owner for notification "${notification.id}"`); | ||
| return false; |
There was a problem hiding this comment.
Keeping ownerless notifications pending means the same rows remain eligible forever, but both recovery queries in space-goal-outcome-notification-repository.ts:57-74 return only the oldest 1,000 pending rows. Once 1,000 ownerless or degraded-owner outcomes accumulate globally or in one Space, every startup/resume recovery pass retries that permanently undeliverable prefix and never reaches newer notifications that do have active owners, so those later wakes cannot recover after an interrupted inline delivery. Recovery needs to paginate or otherwise skip/terminalize the no-recipient rows.
Useful? React with 👍 / 👎.
| } | ||
| const coordinator = config.longHorizonAgentRepo.getCoordinator(spaceId); | ||
| return coordinator?.id ?? null; | ||
| return null; |
There was a problem hiding this comment.
When owner_agent_id is omitted by a Space-chat or human caller without myAgentId, this changed branch now returns null, but CreateGoalSchema still tells callers at space-agent-tool-schemas.ts:663-669 that the field defaults to the coordinator. That schema description is exposed through both the MCP tool and the action registry, so callers relying on it will omit the field and unintentionally create an unowned goal—which under the new routing behavior receives no outcome wakes. Update the argument contract to describe the new unowned default.
Useful? React with 👍 / 👎.
…argets Editing the coordinator prompt broke two tests that CI caught and local verification did not, because only the router test files were re-run. The golden hash guards every extracted prompt byte-for-byte, so an intentional edit has to update it. The other test asserted the instructions contain 'fallback reviewer' -- a duty deleted in #4497. It kept passing because the prompt still described the behaviour, so it was checking that two stale documents agreed rather than that the system did anything. It now asserts the phrase is absent. The not-found reason claimed "the target is declared but no live session received the message". For a retired handle that is false, and it is read by a model deciding whether to wait -- the exact signal that stalls a blocked worker. It now says the target is not reachable: no live session, or not a routable address.
* refactor(space): stop routing messages to the coordinator Removes the four sender-side paths that delivered to the coordinator when there was no specific recipient. Spaces no longer pre-create an agent, so electing one as the default recipient is a guess. decideGenericAddressRouting no longer special-cases the space-manager handle. @Space-Manager and @coordinator now fall through to the ordinary handle path, so they reach a real agent holding that handle or report not found, exactly like any other handle. The deliverToCoordinator arm is gone, deliverSpaceAgentWithFallback loses its coordinator retry and becomes a passthrough (inlined), the injector no longer flags coordinator targets with a null reply-to, and a node message to space-agent with no reply route now reports not found instead of falling back to the Space chat session. Tests that pinned the fallback assert not-found; tests that used the coordinator only as a leg that queues successfully take an authorized session reply route instead, keeping their original subject. * fix(space): hard-fail the space-manager handles instead of routing them Falling through to the ordinary handle path did not remove the coordinator route. listActors synthesizes a coordinator actor for every space, so the messaging facade resolved @Space-Manager straight back to the Space chat session -- and translateLegacyNodeTarget rewrites a reply-route-less space-agent into exactly that handle. decideGenericAddressRouting now reports notFound for those handles. Also drops the agent-kind arm of deliverSingleTarget, unreachable once the router stopped constructing coordinator targets, and corrects the design doc's promise of a Coordinator fallback for space-agent. * fix(space): stop advertising the coordinator handle to models Two surfaces still told agents the handle worked. The unsupported-target hint listed @Space-Manager among valid generic targets, and the long-horizon coordinator prompt said the @coordinator handle still routed. The prompt also claimed the Space Manager is the fallback reviewer for goals with no usable owner. That stopped being true when the goal-owner fallback was deleted; a goal with no owner now sends its outcome to nobody. Both strings reach a model directly, so a stale capability claim there is worse than a stale comment. * fix(space): repair the prompt goldens and stop misreporting retired targets Editing the coordinator prompt broke two tests that CI caught and local verification did not, because only the router test files were re-run. The golden hash guards every extracted prompt byte-for-byte, so an intentional edit has to update it. The other test asserted the instructions contain 'fallback reviewer' -- a duty deleted in #4497. It kept passing because the prompt still described the behaviour, so it was checking that two stale documents agreed rather than that the system did anything. It now asserts the phrase is absent. The not-found reason claimed "the target is declared but no live session received the message". For a retired handle that is false, and it is read by a model deciding whether to wait -- the exact signal that stalls a blocked worker. It now says the target is not reachable: no live session, or not a routable address. * refactor(space): remove the space-agent escalation route There is no default Space recipient, so nothing should tell a worker to escalate to one. Creator-based routing replaces this later; for now the task itself is the signal a human acts on. Six surfaces named the route and each had to go: - list_peers advertised "Use space-agent to escalate blockers" - translateLegacyNodeTarget rewrote a reply-route-less space-agent into @Space-Manager; it now resolves the authorized session or throws - normalizeReplyTargetHandle collapsed both 'space-agent' and every coordinator alias onto @Space-Manager, and space-agent-tools fed that into every outbound envelope as an explicit replyTargetHandle -- so the reply instruction on every space-agent-to-worker message pointed at a handle that now hard-fails - the envelope default did the same for senders with no explicit handle - the task-agent rejection strings advised the dead handle - four workflow prompts escalated and then waited The prompts now record the blocker artifact they were already writing and stop, instead of waiting for a reply nobody will send. coder-only loses self-service re-approval after a post-approval fix push: it stops, and a human re-approves on GitHub and resumes. Alias collapsing is gone too, so @coordinator normalizes to @coordinator -- an ordinary handle that resolves or fails, matching what the routing gates already decided rather than being quietly rewritten underneath them. * fix(space): reject the reserved coordinator role, not just its handles @ROLE:coordinator has kind 'role', so the handle-only guard let it fall through to the messaging facade and reach the same synthetic actor the handle guard blocks. Safe to reject outright: roles ['coordinator', 'space-agent'] appear only on the synthetic coordinatorActor. Real agents carry actor-role:<handle> prefixed roles, so nothing legitimate holds a bare coordinator role. @ROLE:space-agent stays routable -- that one IS shared by real agents.
1 participant
Drops the goal-owner fallback. A goal with no owner row now resolves to
no_recipient.The fallback existed because every space was created with a coordinator, so there was always somebody to ping. Spaces no longer pre-create an agent, and picking an arbitrary active agent as a goal's reviewer is a guess, not a guarantee — an unrelated agent would receive outcome wakes for a goal it has nothing to do with.
Changes
fallback_agentdecision arm removed;applyFallbackAgentGatebecomesapplyNoRecipientGatecoordinator_fallbackremoved from the sharedSpaceGoalOwnerResolutionuniongetFallbackAgentdropped from both agent repositoriescreate_goalno longer assigns the coordinator when the caller has no agent identity — the goal is created unownedGoalDetailPaneldrops the fallback branch and renders the unowned state it already hadBehaviour change
Goals with no owner row stop receiving outcome wakes; the notification stays pending and logs one warning at startup recovery. Nothing else breaks — human approval never went through an agent (
resolveCompletionActorreturns early for non-MCP callers), and agent-created goals still get their creator as owner.+17/−92 prod, +62/−154 test.
Verification
Negative-controlled three ways, each failing exactly the intended test and no others:
resolveClaimAuthorizedAgentIds→denies the named owner when the resolution is degradeddrops a degraded-owner wake instead of routing it elsewhere,drops a no-recipient wakeresolveCreateGoalOwnerId→create_goal leaves the goal unowned when the caller has no agent identity