refactor(control-plane): one post-lease agent re-read instead of four copies - #1072
Merged
Conversation
… copies Four route files each carried their own `refreshMutationAgent` — the re-read an agent-scoped mutation takes after winning the move gate. The copies had already drifted: #1055 moved crons, integrations and the slack-install funnel onto placement identity and left `agents.ts` comparing the `daemonId` column, where two different member sets both read as null and compare equal. The rule moves to `http/mutation-agent.ts` and the four call sites bind to it, so `agents.ts` picks up the identity fence as well. Net -39 lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DaoSF8Lh8Ekbq3rjSgoQ7F
Contributor
There was a problem hiding this comment.
Approved. I verified the isolated checkout is the trusted synthetic merge whose parents are exactly base b17f981d49d929ca5a44bcbb9b8d66b0907dc31d and head a365557872b2ffdf4a6821b15152dd2c80de0cd6. The extracted helper preserves the existing org-scoped re-read and lastModifiedAt conflict fence while applying full placement identity uniformly, which closes the set-to-set movement gap in agents.ts without changing the corrected callers’ behavior.
Verification completed: control-plane typecheck passed; all 157 control-plane unit-test files and 1711 tests passed; focused ESLint and Prettier checks passed. No blocking findings.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
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
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.
Summary
Four route files each carried their own
refreshMutationAgent— the re-read an agent-scoped mutation takes after it wins the move gate. They move to onehttp/mutation-agent.ts. Net -39 lines, no new behavior except the drift correction below.Failure scenario
The copies had already diverged, which is what makes this worth doing rather than tidy.
#1055 corrected three of them from
daemonIdcolumn equality to placement identity (samePlacementRef) and left the fourth —agents.ts:1343— on the column. So onmaintoday:An agent on member set A, re-placed onto set B while a console edit holds the mutation lease, has
daemonId === nullon both sides. The columns compare equal, the fence passes, and the edit commits against a placement that has already changed. The three corrected copies would have refused it.It is a narrow window — it needs a re-placement between the caller's read and the lease — and set→set moves are not common yet, which is exactly why the divergence went unnoticed. The point is that one rule living in four places produced it in a single PR.
Fix
http/mutation-agent.tsholds the rule once:setplacement names no machine, so column equality both misses a re-placement onto another set and reads every set agent as unplaced;lastModifiedAt, so an edit that landed between the caller's read and the lease is a conflict.agents.ts,crons.tsandintegrations.tskeep a one-line binding so their call sites are untouched;slack-install.ts's inline copy becomes a call.samePlacementRefis no longer imported by any route file — it now has exactly one consumer.Test plan
New
src/http/mutation-agent.test.ts(unit, no Docker) covers the fence directly: unchanged row admitted and fenced on the observed row's own org; vanished row and editedlastModifiedAtrefused; machine placement moved to another daemon refused; set placement moved to a different set refused; unchanged set placement admitted; kind changes refused both directions.Mutation-checked: swapping
samePlacementRefback forcurrent.daemonId !== observed.daemonIdfails exactly the set→set case and nothing else.pnpm --filter @agentconnect.md/control-plane typecheck— cleanpnpm --filter @agentconnect.md/control-plane test:unit— 157 files, 1711 tests, all passingpnpm --filter @agentconnect.md/control-plane test:int— 89 files, 1363 tests, all passing (this sandbox blocks the Testcontainers image pull at the egress proxy, so the suite ran against a local PostgreSQL 16 cluster through a scratch global-setup with the identical migrate/seed/clone-per-pool contract; that scratch file is not part of the diff)pnpm lint,pnpm format:check— cleanFollow-up to #1055.
Generated by Claude Code