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.

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.