Record a removal before retiring what the person owned

[zopeVaibhav]

What this changes

A removal whose second half fails is now recorded, and asking again finishes it.

peopleStore.revoke did two things: a transaction writing the deny-list row and deleting the person's
sessions, then the retirement of what they owned, against the vault and the broker. Only the first is
guaranteed, and the second throws on purpose rather than reporting an ending that did not happen — so
the throw travelled past the route's next line, which is the one that writes person.access_revoked.
The person was out of the deployment and nothing said anybody had removed them. Removing them again
answered 200 without retrying, because the state had already changed, and retireConnectionsFor has
no other caller: the credentials and composio_connections rows stayed standing with nothing able to
reach them.

The two halves are now two methods. revoke is the part that must stick, the route records the
removal as soon as it has, and retireOwned runs after that row on every request that asks for a
removal — including one for somebody already removed, which is what turns "try it again" into a
recovery instead of a no-op. On a person whose retirement did succeed it finds nothing: the vault rows
are already marked revoked and the connection rows are already gone, so it writes nothing and returns.

The retirement's failure still reaches the administrator as a 500. That is deliberate — answering 200
over a retirement that did not happen is the outcome the plugin store's own comment sets out to avoid.
What changed is that the trail no longer loses the removal on the way past.

Where it runs

• New state that outlives a request? None. One store method split into two, and one call moved.
• What happens on the second replica? The same. Both halves are Postgres writes and broker calls
  keyed by user id; two administrators removing the same person concurrently both write a deny-list
  row under onConflictDoNothing and both retire, and the second retirement finds nothing left.
• Anything serialised? The deny list and the session deletion are one transaction, as before. The
  retirement is idempotent by the state it reads rather than by a lock: a credential already marked
  revoked is skipped and a connection row already deleted is not found.
• Anything fanned out to a browser? No.
• New listener, port, or schedule? No.

One case is worth naming because this change reaches it. When the broker refuses part-way through a
person's apps, the ones already withdrawn at the vendor have no audit row written and no connection
row deleted, because both come after the loop that threw. On main that state is permanent — the
retry answers 200 and does nothing, which is the bug this fixes. Here the retry converges: the rows go
down and the access ends. The cost is that the apps withdrawn on the first pass answer false on the
second, so their rows record that the vendor was not asked when it was. That is a defect in
retireConnectionsFor rather than in this diff, and it wants a catch per app that records the answer
that app actually got; it is worth its own change. This PR moves that case from permanently stuck to
converged-with-three-imprecise-rows, which is the direction, not the destination.

Boundary and audit

• Every acting call still goes through the gateway: resolve, decide, audit, then act.
• New refusals and new failures each write a row. No new refusal; the existing
  person.access_revoked row is now written for removals that previously wrote none, and the
  retirement's own mcp.account_disconnected rows are unchanged.
• Nothing new is trusted from the client. The person comes from the path as before.

Changelog

• CHANGELOG.md, under Unreleased.

Proof

server/tests/offboarding-retirement.integration.test.ts is new. It drives the real route over the
real store against a migrated database, with a retirer that throws the first time, and asserts what has
committed when it does: the deny-list row is there, the sessions are gone, person.access_revoked is
on the trail, and the second request retries the retirement and writes no second row.

$ bun test server/tests/offboarding-retirement.integration.test.ts
 1 pass
 0 fail

The same file on main, in a detached worktree:

$ bun test server/tests/offboarding-retirement.integration.test.ts
(fail) a broker that will not answer still leaves the removal on the trail, and asking again finishes it

expect(received).toEqual(expected)
- [ "person.access_revoked" ]
+ []

 0 pass
 1 fail

It fails on the trail assertion, after the 500, the deny-list row and the session deletion have all
been confirmed — which is the bug and nothing else.

server/tests/people-routes.test.ts gains retireOwned in its stub and asserts the route now calls
it. The whole server suite is green on this branch: 3239 pass, 5 skip, 0 fail across 181 files, plus
typecheck, lint and format:check.

No surface changed, so there is nothing to screenshot.

Closes #573.

[Hotragn]

Reviewing this because the audit-ordering seam is where I have been working, and a removal that loses its own row is the clearest case of it. The diagnosis is right and the split is the right split. Two things I verified, and one consequence I think this PR makes reachable.

Verified

The bug is where you say it is. On main, app.ts:769-783:

if (person.revoked !== revoked) {
  if (revoked) await peopleStore.revoke(userId, context.var.actor.id);   // throws
  else await peopleStore.restore(userId);
  await recordPersonEvent(auditStore, context, "person.access_revoked", person, {});
}

The throw travels past the recordPersonEvent line. The deny-list row and the session deletion are committed in their own transaction inside revoke, so the person is out and nothing says anybody removed them.

And the fix's placement is better than the body sells it. Putting retireOwned after the person.revoked !== revoked block means a retry on somebody already removed skips revoke and skips recordPersonEvent — so the recovery runs the retirement without writing a second person.access_revoked row. One removal, one row, however many attempts it took. That is the property I would have gone looking for first, and it falls out of where you put the call rather than needing a guard.

The idempotency claim is the load-bearing one and it holds. retireConnectionsFor skips already-retired vault rows explicitly (store.ts:6604, if (credential.revokedAt) continue;) with a comment saying retiring twice should be quiet, and the brokered half reads composioConnections fresh each time, so after a success the second call finds nothing. You are not relying on new behaviour; you are relying on behaviour that was already written for exactly this and had no caller that could reach it.

The thing I would want addressed: a partial broker failure now writes a false row

Your test throws from the retirer, so the first attempt does nothing and the retry does all of it. The interesting case is a throw inside it. store.ts:6672-6712:

const vendorRevocationRequested = new Map<string, boolean>();
for (const connection of brokered) {
  vendorRevocationRequested.set(
    connection.toolkit,
    broker ? await broker.revoke({ userId, toolkit: connection.toolkit }) : false,
  );                                          // ← BrokerRefusalError
}
await database.delete(composioConnections).where(...);
for (const connection of brokered) { /* writes mcp.account_disconnected */ }

ComposioBroker.revoke throws BrokerRefusalError (composio-adapter.ts:3683 and the throws below it), so with five brokered apps and a refusal on the fourth:

1. Apps one to three were deleted at Composio with revoke_on_delete, and each returned true.
2. The delete and the whole audit loop are after the throw, so no mcp.account_disconnected row is written for any of them, and the composio_connections rows all stand.
3. The retry re-reads all five rows and calls revoke again. For one to three the account is already gone, so revoke answers false — "nothing to withdraw", which is what it is documented to mean.
4. Five rows go down, three of them saying vendorRevocationRequested: false.

Those three say the vendor was not asked. The vendor was asked, and complied. And that field's own docblock is unusually explicit that this is the one distinction it exists to carry:

THE ANSWER IS WHAT WAS ASKED FOR, not whether the call threw. false here means there was nothing to withdraw, which is what the audit trail's vendorRevocationRequested is for: a reader has to be able to tell an account this deployment acted on from one that outlives it somewhere else.

So an auditor asking "did we tear up their Google grant" reads no about three apps where the answer is yes.

I want to be fair about whose bug this is: it is not in your diff. It is in retireConnectionsFor, and it predates you. But on main it is unreachable — a retry answered 200 without retrying, which is the bug you are fixing — and this PR makes the retry the documented recovery path. So the PR turns a latent wrong row into one an administrator following your changelog line will produce.

Two ways out, and I do not think it should hold the PR:

• Smallest: move the delete and the audit loop inside the per-app loop, so each app's row goes down with the answer it actually got. Costs the "read before anything is deleted" property the docblock argues for — though that argument is about needing the app names, and the loop already holds them.
• Cleanest, and probably its own PR: catch per app, record the row with the answer you got, and rethrow after the loop. Then a partial failure records exactly what happened for each app and still 500s.

Either way it belongs to retireConnectionsFor, not here. I would be happy to send the second as a follow-up if you would rather keep this diff at the size it is — your call, it is your issue.

One small thing

offboarding-retirement.integration.test.ts reaches createApp's audit store through ...(Array.from({ length: 9 }) as never[]). It works, but it is nine positional holes that no longer mean anything to a reader, and anyone reordering createApp's parameters gets a test that fails somewhere unrelated to what it is testing. Not worth blocking on; worth a comment naming which parameter the padding is walking to.

Otherwise this reads right to me, and the integration test doing it over the real route against a migrated database is the right level for this — a stub would have proved the split and not the commit boundary, which is the whole question.

[zopeVaibhav]

Confirmed, and thank you for checking the claims rather than taking them.

One correction to the retry half of your account, in your favour. On the second attempt the configs for apps one to three are still this deployment's and still readable, so ours.length === 0 at composio-adapter.ts:3783 does not fire. The false arrives one listing further in: the accounts are gone, withdrawableAccounts returns nothing, and the method reaches its terminal return ids.length > 0 at composio-adapter.ts:3931. Configs intact, accounts withdrawn, answer false. Which makes the row wrong for exactly the reason you give, and makes the state harder to spot than "the config disappeared" would be.

I agree it is reachable because of this change. On main the retry answers 200 and does nothing, so nobody ever gets to a second pass; this PR makes the second pass the documented recovery, and the changelog line I wrote tells an administrator to take it. That is a fair thing to land on me even though the code is not mine.

I have added a paragraph to the PR body naming the case, since the body's idempotency sentence was written for the clean path and oversold it. It also says the part I think is worth keeping in view: on main a partial failure leaves those rows standing permanently, and here it converges. The three imprecise rows are the price of converging, not a new hole.

Please do send it, and I would take your second option over the first. A catch per app that records the answer that app actually got and rethrows after the loop keeps the revoke-before-delete order the docblock argues for, which the per-app delete would give up. Happy to review it.

On the positional padding in the test: you are right that it tells a reader nothing. The sibling people-routes.test.ts documents the same layout, which is why I left it alone rather than restating it. If you would rather it did not depend on counting holes at all, say so and I will restructure that part of the test — it is the parameter reorder you describe that would actually bite, not the reading.

[davidmckayv]

Code-verified clean; CI green on this sha.

[Hotragn]

Sent as #585, taking your second option — a catch per app that records the answer that app actually got, rethrown after the loop, so revoke-before-delete survives.

Thanks for the correction on where the false comes from. I have carried it into the PR body with attribution, because the mechanism you describe is the worse one: the configs stay intact and readable, so the only visible symptom is a boolean, where a missing config would at least have been obvious.

Two things beyond the shape we discussed, both of which fell out of writing it:

The delete had to be scoped too. Deleting by user id after a partial failure takes the rows of apps that were refused, and those are the rows your recovery needs. It now deletes only the toolkits that answered. That makes an offboarding the vendor refuses leaves the connection standing apply to the app it is about rather than to every app in the same act, which I think is what that test always meant.

A second test for the inverse risk. Holding refusals instead of throwing them is only correct if they are still thrown, so there is a test with the refusal on the first app asserting the act still fails and the second app is still asked. Collecting refusals quietly turning a failed offboarding into a reported one is the failure that fix could introduce, and it deserved its own assertion rather than being implied.

I also got a database up locally for this — pgvector/pgvector:pg17 with CI's credentials — so unlike my review these numbers are measured rather than reasoned: 2 fail on main with the fix reverted, 71 pass on the branch, and the whole server suite at 3236→3238 pass with the same 7 pre-existing failures either side.

On the positional padding: leave it. You are right that people-routes.test.ts documents the layout, and now that I have looked at it the thing that would bite is a parameter reorder, which would break loudly rather than silently. Not worth churning a merged test for.