Repository navigation
fix(training-agent): allow retired GCS account replacement - #8173
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean migration + application-layer defense-in-depth for training GCS account replacement.
Checked:
- No Critical/High/Medium findings reported.
- Migration 622 ordering installs the BEFORE INSERT trigger before dropping the old
host_accountsUNIQUE constraint, so there is no unguarded rollout window (migrations run on startup in prod — judged as production-facing). - Idempotency, per-principal advisory locking with bounded lock/statement timeouts, and fail-closed client-release on rollback failure all verified.
- Exact-generation retirement proof matches the always-generation-1 authorize/grant flow.
- No changeset required: operational/training-agent code, not published protocol surface (
static/schemas/source/**, reference docs, dist artifacts untouched).
High-risk flag is true only due to a newly-added migration file (server/src/db/migrations/** — (added)). Per the arbiter rules, a new file matching a high-risk glob is normal scaffolding and not escalation-worthy on the flag alone; no (deleted)/(modified) sensitive-path finding exists. gated_paths is false, no no-auto-approve team match, no prior decision. Rows 1–8 do not fire → row 9 approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — server-only canary-account migration with no blocking findings.
This PR adds migration 622 (BEFORE INSERT trigger on host_accounts) and reworks reserveTrainingGcsAccount() to enforce one active/unfinished training-GCS canary account per principal via a shared advisory-lock key, with retired-binding tombstones and generation-based retirement proof.
Checks confirmed by the reviewer:
- Dropping the
principal_idunique constraint breaks no read path (reads key onaccount_idPK / join byaccount_id+destination_ref). ON CONFLICT (account_id)remains valid post-migration.- Explicit lock/statement timeouts on both paths; no hang exposure.
- Errors rethrown (no warn-only swallowing); correct pool-poison release on rollback-failure.
- Integration tests run against real Postgres with a localhost/adcp_test guard, covering the fence (23514), serialization, exact repair, exact-generation revocation, immutable-fact takeover, tombstone-byte preservation, and rollback cleanliness.
Not a protocol-surface change (no static/schemas/source/**, docs, or dist artifacts touched), so no changeset is required. high_risk is true only because the migration is a newly added file matching server/src/db/migrations/** — a new-file scaffolding signal, not a destructive change; it does not warrant escalation on the flag alone. No critical/high/medium findings. gated_paths is false and review_decision is APPROVED. Rows 1–8 do not fire; outcome is row 9 approve. Consistent with the prior clean pass.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean rebase, no blocking findings.
This PR is a pure rebase of the account-reservation migration: renamed from slot 622 to 623 to avoid colliding with main's new 622_compliance_heartbeat_claim_ownership.sql, with a corresponding bump to its RAISE EXCEPTION message. The reviewer confirmed no stale 622 references remain and that migration ordering between the two files is independent. The prior pass on head 720c6a9 was a clean approve with no findings, and nothing re-surfaces here.
Checked:
- Migration rename (622→623) is mechanical; no logic change beyond the exception message text matching the new slot.
high_riskis true only because the new migration file matchesserver/src/db/migrations/**, but the reason is(added)— a new file is inherently low-risk scaffolding, not an escalation trigger. No(deleted)or(modified)sensitive-path concern.- No protocol surface touched (
static/schemas/source/**,docs/reference/**, dist artifacts untouched), so no changeset/spec-drift/oneOf obligations apply. Migration-only + accompanying integration test. - No no-auto-approve team match;
gated_pathsis false.
Decision table: rows 1–8 do not fire (no critical/high/medium findings, no deleted sensitive file, no medium-on-modified-high-risk, prior decision was approve, no team gate, zero medium findings). Falls through to row 9 → approve.
A failed private GCS canary leaves an immutable account binding. The unique organization constraint then prevents a replacement even after its destination is revoked. Permit a fresh account only when every prior account has an exact saved grant with durable revocation evidence and no current authorization.
Keep one active or unfinished account per organization. Application reservations serialize by principal and preserve immutable source facts. Migration 623 installs the database reservation guard before removing the old unique constraint, so older runtimes using INSERT ... ON CONFLICT DO NOTHING retain the same fence during rolling deployment. Retired bindings, grants, and authorization tombstones remain intact; retired destinations cannot be reused.
Validation: 21 focused checks pass, including nine real PostgreSQL controls for concurrent current and older-runtime inserts, unfinished setup, exact-generation fencing, newer live authorizations, original history preservation, immutable facts, and rollback, plus 12 existing API authorization controls. Restoring the original unique constraint reproduces the replacement failure. Normal mandatory commit hooks passed: 12,579 server tests, 1,113 root tests, guards, and both typechecks. Two HTTP teardown test files timed out during the parallel run and passed the hook’s automatic isolated rerun. The authoritative storyboard matrix runs in CI.
Integration: includes current main and the published 3.2.4 release records from merged #8153. The account reservation migration is now 623, preserving main’s independent migration 622 for compliance heartbeat ownership. The fresh PostgreSQL reservation/API controls and migration replay pass after integration; the published compliance manifest is current. A new live canary still requires explicit approval for a fresh time-bound access grant after the repairs are deployed; the original failed attempt remains unqualified.
Open workspace in Conductor