# Residual Review Findings

**Branch:** `feat/computer-admin-crud-backend`
**HEAD SHA:** 52f39af4857cf61823a6f6c1e964d91dbe38cb9b
**Run:** ce-code-review mode:autofix — 20260512-053843-dc999154
**Reviewers:** correctness, testing, maintainability, project-standards, security, adversarial, reliability, api-contract, data-migrations, kieran-typescript

`safe_auto` fixes applied automatically (4): `FailureReason` derived from `ProvisionResult` (3-reviewer agreement: KT-002/M1/maintainability), dead intermediate `templateId` removed (KT-005), `_parent: any` → `unknown` (KT-004), `typeof` guard added to `isAlreadyActiveConflict` before unknown cast (KT-001).

All remaining findings below are `gated_auto` or `manual` and require human judgment.

## P1 — Reliability

### REL-001: Template DB query is outside `try/catch` — "never throws" contract is technically broken
**File:** `packages/api/src/lib/computers/provision.ts:107-129`
**Severity:** High · **Confidence:** 90% · **Reviewers:** reliability (also flagged by adversarial)
**Issue:** The platform-default template `db.select()` runs before the `try/catch` block at line 132. A DB connection failure, pool exhaustion, or query timeout would propagate out of `provisionComputerForMember`, violating the "never throws" contract documented in the JSDoc and every call-site comment. The five call-site catches are defense-in-depth; the function itself does not honor the contract.
**Suggested fix:** Wrap the entire function body (template resolution + `createComputerCore` call) in a single top-level `try/catch`; on a top-level DB error, call `safeWriteActivityLog` with `reason: 'unknown'` and return `{ status: 'failed', reason: 'unknown' }`.

### REL-002 / M2 / correctness-002: `classifyValidationError` matches on message substrings rather than `extensions.code`
**File:** `packages/api/src/lib/computers/provision.ts:177-186`
**Severity:** High · **Confidence:** 75–90% · **Reviewers:** reliability + maintainability + correctness (3-reviewer agreement)
**Issue:** `classifyValidationError` matches `message.includes("owner user not found")` and `message.includes("template not found")` — both strings live in `shared.ts` (`requireTenantUser`, `requireComputerTemplate`). Rewording either message silently collapses the classification to `unknown`, producing misleading `activity_log` rows. The `GraphQLError`s already carry typed `extensions.code` values (`NOT_FOUND`, `BAD_USER_INPUT`).
**Suggested fix:** Switch the classifier to read `err.extensions?.code` and map to `user_not_in_tenant` / `template_not_found`. Update the existing `failed:no_default_template` activity_log test to also cover both validation-error reasons.

## P1 — Adversarial / Cascade

### adv-bootstrap-claim-ordering: Cognito update failure strands user with invisible Computer
**File:** `packages/api/src/graphql/resolvers/core/bootstrapUser.mutation.ts:94-106`
**Severity:** Medium · **Confidence:** 70% · **Reviewer:** adversarial
**Issue:** In the claim path, the sequence is (1) insert tenant member, (2) auto-provision Computer, (3) update Cognito `custom:tenant_id`. If step 3 fails (Cognito control-plane degraded), the user has a Computer in DB but no `tenant_id` claim in their JWT — admin shows nothing for them on next login. Recovery requires manual Cognito attribute fix.
**Suggested fix:** Either reorder (Cognito update before Computer provision) or capture a queue of "needs Cognito repair" events. Brainstorm doesn't specify recovery; leave to a follow-up.

### adv-rest-addmember-status-case-bypass: `member.status === "active"` is case-sensitive
**File:** `packages/api/src/handlers/tenants.ts:~388`
**Severity:** Medium · **Confidence:** 80% · **Reviewer:** adversarial
**Issue:** The REST `addMember` handler accepts arbitrary `body.status`. We gate provisioning on `member.status === "active"` (lowercase). A caller passing `"ACTIVE"` (uppercase) bypasses the gate — the member is created as active but the helper never fires.
**Suggested fix:** Normalize before comparison: `if (String(member.status).toLowerCase() === "active") { ... }`. Add a unit test.

### adv-invite-existing-member-no-reprovision: Re-invite of an archived-Computer user never re-provisions
**Files:**
- `packages/api/src/graphql/resolvers/core/inviteMember.mutation.ts` (existing-member branch)
- `packages/api/src/handlers/tenants.ts` (REST invite existing-member branch)

**Severity:** Medium · **Confidence:** 80% · **Reviewer:** adversarial
**Issue:** The existing-member early-return paths return success without calling `provisionComputerForMember`. If a user's Computer was archived between invites, re-inviting them returns success with no Computer in place. Brainstorm R16 says "re-provision" is one recovery path — the wording is ambiguous about whether re-invite should trigger it.
**Suggested fix:** Product decision required. Either (a) call helper unconditionally and rely on `skipped:already_active` idempotency, (b) keep current behavior and document re-provision-via-manual-create as the only recovery path, or (c) detect archived-Computer state on the early-return branch and create a new one.

### adv-activity-log-fk-swallowed: FK violation on bad tenantId destroys the audit signal
**File:** `packages/api/src/lib/computers/provision.ts:208-237` (`safeWriteActivityLog`)
**Severity:** Medium · **Confidence:** 75% · **Reviewer:** adversarial
**Issue:** `safeWriteActivityLog` wraps `db.insert(activityLog)` in `try/catch` and swallows on error. If `tenant_id` is bogus or the foreign-key target is missing, the FK violation produces no audit row — exactly the case where backfill audit is most needed.
**Suggested fix:** Surface failed audit writes via a structured `console.error` with a stable error code (e.g., `[provision:audit-write-failed]`) so log-based monitoring catches them.

## P2 — Data migrations

### DM-001: Seed migration lacks `BEGIN/COMMIT` transaction wrap
**File:** `packages/database-pg/drizzle/0085_seed_thinkwork_computer_default_template.sql`
**Severity:** Low · **Confidence:** 75% · **Reviewer:** data-migrations
**Issue:** The migration runs `INSERT ... WHERE NOT EXISTS` followed by `CREATE OR REPLACE VIEW`. Other 008x migrations (e.g., `0084_artifacts_favorited_at.sql`) wrap DDL/DML in `BEGIN; SET LOCAL lock_timeout = '5s'; ... COMMIT;`. A crash between the two statements leaves the marker view absent with the data row present — the drift gate would then report MISSING despite the row existing.
**Suggested fix:** Wrap in `BEGIN/COMMIT` with `SET LOCAL lock_timeout` and `SET LOCAL statement_timeout` matching the 0084 precedent.

### DM-002: No rollback companion for 0085
**File:** missing `packages/database-pg/drizzle/0085_seed_thinkwork_computer_default_template_rollback.sql`
**Severity:** Low · **Confidence:** 75% · **Reviewer:** data-migrations
**Issue:** 0080–0084 each ship with a paired `_rollback.sql`. 0085 creates a permanent view and a data row but has no rollback script.
**Suggested fix:** Add `0085_seed_thinkwork_computer_default_template_rollback.sql` containing `DROP VIEW IF EXISTS public.view_thinkwork_computer_default_template_seeded;` and `DELETE FROM agent_templates WHERE slug = 'thinkwork-computer-default' AND tenant_id IS NULL AND template_kind = 'computer';`.

## P2 — Maintainability

### M3 / KT-003: `ProvisionInput.templateId` is an unused extension point; `args.input` typed `Record<string, any>`
**Files:** `packages/api/src/lib/computers/provision.ts:62-66`, `packages/api/src/graphql/resolvers/computers/createComputer.mutation.ts:16`
**Severity:** Low · **Confidence:** 75% · **Reviewer:** maintainability + kieran-typescript
**Issue:** `ProvisionInput.templateId` is documented "not used by v1 callers" and all five sites omit it. `createComputer.mutation.ts:16` types `args.input` as `Record<string, any>` even though `CreateComputerCoreInput` from the same import covers every field accessed.
**Suggested fix:** Either remove `templateId` from `ProvisionInput` until a real override use case lands, or move it to a `provisionForTenantOverride` helper. Type `args.input` as `CreateComputerCoreInput` (existing import already in scope).

## P2 — Testing gaps

### T1: Five call-site wirings have zero integration tests
**Files:** `addTenantMember`, `inviteMember`, `bootstrapUser` (both paths), `tenants.ts` (2 REST sites)
**Severity:** Medium · **Confidence:** 90% · **Reviewer:** testing
**Issue:** The plan listed integration tests for each resolver wiring; they were not written in this run. The 12 helper unit tests cover the helper's discriminated union, but no test exercises the resolver→helper handoff against the real DB-mock layer.
**Suggested fix:** Add integration tests for each call site mocking `provisionComputerForMember` and asserting (a) the helper is called with correct args, (b) a `failed:` return does not break the primary mutation. These are a clean next-PR addition.

### T2: `inviteMember-computer-claim.test.ts` does not mock the new provision module
**File:** pre-existing test file
**Severity:** Medium · **Confidence:** 85% · **Reviewer:** testing
**Issue:** The pre-existing test for `inviteMember`'s Computer-claim flow runs `inviteMemberCore` with a mocked DB but does not mock `../lib/computers/provision.js`. After this PR, the provision module runs its real code path against the test's mocked DB selectRowsQueue, silently draining queued rows. The test passes only by accident (provision failure is swallowed).
**Suggested fix:** Add `vi.mock("../../../lib/computers/provision.js", () => ({ provisionComputerForMember: vi.fn().mockResolvedValue({ status: "skipped", reason: "already_active" }) }));` to the test file.

### T3: `classifyValidationError` non-`unknown` branches untested
**File:** `packages/api/src/lib/computers/provision.test.ts`
**Severity:** Low · **Confidence:** 80% · **Reviewer:** testing + maintainability
**Issue:** The unit tests for `provisionComputerForMember` exercise the `unknown` fallback path but no test pins `user_not_in_tenant` or `template_not_found` classifications. Combined with the brittle message-substring matching (REL-002), a future reword in `shared.ts` would silently degrade audit fidelity.

## P3 — Lower-impact observations

- **correctness-001** — `inviteMember` GraphQL: apikey callers pass `ctx.auth.principalId` directly as `invokerUserId` without validating it's a `users.id`. Risk: malformed `activity_log` rows if a non-UUID principal id is used.
- **correctness-003** — `bootstrapUser` existing-user fast path (lines 32-49) does not call the helper, so users who pre-date the seed migration never get retro-provisioned on next login. Brainstorm acknowledges manual backfill is the path for these; surfacing for documentation.
- **DM-003** — Seed migration hardcodes `model = 'us.anthropic.claude-sonnet-4-20250514-v1:0'`. No runtime availability check; if Bedrock deprecates this profile, provisioning succeeds at DB-level but agent invocations fail later.
- **adv-seed-deletion-replay** — Deleting and re-applying the seed produces a new UUID for the platform-default row; existing Computers' `template_id` FK still points at the old UUID. Operational concern, not introduced by this PR.

## Security review verdict

`security` reviewer returned zero P0/P1 findings. The authz boundaries are intact across all five provisioning call sites; `createComputerCore` is only exposed via the admin-gated `createComputer` mutation and the server-internal helper; `requireTenantUser` tenant-scopes the owner argument inside `createComputerCore`.

Residual security risks documented (not P0/P1):
- `createComputerCore` is `export`ed and ungated by design — a future caller could import it without an authz gate. Worth a lint rule or stronger naming (`__internal_createComputerCore`).
- `isAlreadyActiveConflict` maps any 23505 (regardless of constraint) to `skipped:already_active`. A slug-collision 23505 would silently no-op without an `activity_log` row.

## Project standards verdict

`project-standards` reviewer returned zero findings. pnpm-only usage, `-- creates:` marker convention, codegen across consumers, hand-rolled migration regime — all verified compliant with `CLAUDE.md` / `AGENTS.md`.

## API contract verdict

`api-contract` reviewer returned zero P1+ findings. New `computerTemplates` query is purely additive; signature changes to `inviteMemberCore` / `tenants.ts` `addMember` / `inviteMember` are private; `createComputer` GraphQL contract is unchanged.

## Run artifact

Full per-reviewer JSON: `/tmp/compound-engineering/ce-code-review/20260512-053843-dc999154/`
