feat: wire runCommand into mutation routes#4
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a transactional command framework (packages/commands) implementing durable idempotency with in-flight claiming, replay, and stale takeover; migrates idempotency_records to allow NULL workspace_id and a composite unique index; updates LocalRepository and PostgresRepository to accept AdminActor and idempotencyKey and delegate admin mutations through runCommand; requires Idempotency-Key in API admin endpoints and maps IdempotencyInFlightError to 409 responses; and adds upload handler error handling, tests, migration script changes, and docs updates. Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API Handler
participant Normalize as adminActor
participant Wrapper as runIdempotent/runCommand
participant DB as Database
Client->>API: POST with Idempotency-Key
API->>Normalize: normalize ApiActor -> AdminActor
API->>Wrapper: call db method with { actor, idempotencyKey, ... }
Wrapper->>DB: INSERT idempotency_records (in_flight)
alt Claimed
Wrapper->>DB: execute mutation + INSERT operation_events
DB-->>Wrapper: UPDATE idempotency_records -> completed
Wrapper-->>API: return result (isReplay: false)
else Existing
Wrapper->>DB: SELECT ... FOR UPDATE
alt completed
DB-->>Wrapper: return result_json
Wrapper-->>API: return cached result (isReplay: true)
else in_flight fresh
DB-->>Wrapper: indicate in_flight -> throw IdempotencyInFlightError
Wrapper-->>API: 409 idempotency_in_flight
end
end
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Every POST/PUT/DELETE in api and upload now flows through the shared runCommand wrapper from packages/commands. The wrapper claims an idempotency_records row, executes the handler in the same transaction, persists result_json, and writes the audit event. Replay returns the cached body; an in-flight collision returns 409. A new migration (0002_idempotency_admin_ops) drops the workspace_id FK and adds a NULLS NOT DISTINCT unique index so admin-without-workspace mutations (workspace create, cleanup) can claim records cleanly. Closes backlog item 1 in docs/ops/project-status.md; advances ADR 0004, 0022, and 0035 from Partial to Done. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- adminActor now throws on unexpected actor types instead of silently mapping non-system actors to admin. - createApiKey defers generateApiKey until inside the idempotent path (both LocalRepository and PostgresRepository) so replays skip the unused crypto work. - Clarify backlog wording so item 9 (PR preview exercise) is not read as a deployment blocker. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reorder idempotency-key validation to run before database availability checks in all admin handlers. Eliminate redundant mustApiKey call in PostgresRepository.revokeApiKey by reusing the initially fetched row.
isuttell
force-pushed
the
feat/idempotent-mutations
branch
from
aa56598 to
f27eee3
Compare
The rebase onto origin/main silently regressed the @vitest/coverage-v8 catalog and importer entries that PR #3 added. Frozen-lockfile install on CI failed because the root package.json still referenced the catalog spec.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/package.json`:
- Around line 25-29: The package manifest's "dependencies" were updated for the
workspace packages (e.g., "`@agent-paste/auth`", "`@agent-paste/commands`",
"`@agent-paste/db`"), but the pnpm lockfile is out of date causing CI to fail;
regenerate the lockfile by running pnpm install (or pnpm install
--lockfile-only) in the repo root to update pnpm-lock.yaml and commit the
updated pnpm-lock.yaml alongside the package.json changes so that pnpm install
--frozen-lockfile passes in CI.
In `@apps/api/src/index.ts`:
- Around line 266-268: The call to runCleanupAndDeny currently hard-codes the
actor as { type: "admin", id: actor.id }, losing the original actor type; change
this to pass the verified actor object (or wrap via adminActor(actor) if that
helper exists) so the exact actor.type and id are preserved when calling
runCleanupAndDeny from runIdempotent — i.e., replace the hard-coded object with
the actual actor/adminActor(actor) parameter and keep the other args (dryRun,
batchSize, idempotencyKey) unchanged.
In `@docs/ops/project-status.md`:
- Line 167: Multiple backlog subsection headings (e.g., "### 1. Enforce native
rate-limit bindings" and the other headings noted) are missing the required
blank line below them causing MD022 markdownlint warnings; fix by inserting a
single blank line immediately after each of those "###" headings (the ones at
the listed occurrences) so every heading is followed by an empty line before the
next paragraph or list.
In `@packages/commands/src/index.ts`:
- Around line 58-60: The runCommand flow must require a transactional executor:
update the logic in runCommand to check that the passed-in SqlExecutor has a
defined transaction method (SqlExecutor.transaction) and immediately throw an
error if it is absent rather than falling back to non-transactional execution;
ensure the code path that writes the idempotency "in_flight" row and later
updates completed/audit always runs inside executor.transaction by invoking
executor.transaction(...) and propagating the thrown error so partial side
effects cannot leave the idempotency row stuck in_flight (refer to the
runCommand handler and the SqlExecutor.transaction symbol to locate where to add
the guard and wrap the operation).
In `@packages/db/migrations/0002_idempotency_admin_ops.sql`:
- Around line 9-17: The ALTER sequence is incorrect: drop the primary key
constraint idempotency_records_pkey on table idempotency_records first, then run
ALTER COLUMN workspace_id DROP NOT NULL to allow nulls, and finally create the
replacement unique index using NULLS NOT DISTINCT (or the equivalent expression)
so the composite uniqueness on (actor, operation, key, workspace_id) behaves as
intended; reference idempotency_records, idempotency_records_pkey, and
workspace_id when making the reorder and adding the new unique index.
In `@packages/db/src/index.ts`:
- Line 165: The idempotency keys passed to runIdempotent (e.g., the call that
builds "admin.workspace.create:${input.actor.id}:${input.idempotencyKey}") omit
input.actor.type and thus collide across actor types; update the key builder to
include input.actor.type (for example
"admin.workspace.create:${input.actor.type}:${input.actor.id}:${input.idempotencyKey}")
or extract this into a small helper function (e.g., buildIdempotencyKey(action,
input)) and replace the current usages; apply the same change/helper to the
other affected calls createApiKey, revokeApiKey, runCleanup, and deleteArtifact
so the in-memory keys match the Postgres path that uses both actor.type and
actor.id.
In `@packages/db/src/schema.ts`:
- Around line 164-170: Replace the plain unique index usage: change the call
site that currently uses
uniqueIndex("idempotency_records_unique").on(...table.workspaceId,
table.actorType, table.actorId, table.operation, table.idempotencyKey) to use
the Drizzle unique API and nullsNotDistinct:
unique("idempotency_records_unique").on(...same columns).nullsNotDistinct();
also update imports to remove or keep uniqueIndex and add unique (and
nullsNotDistinct is chained on the unique result) from drizzle-orm/pg-core so
the schema matches the migration's `NULLS NOT DISTINCT` behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 11aeb712-ea7c-4725-ae87-d61f5ac0046e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
apps/api/package.jsonapps/api/src/index.test.tsapps/api/src/index.tsapps/upload/package.jsonapps/upload/src/index.tsdocs/ops/project-status.mdpackages/commands/src/index.test.tspackages/commands/src/index.tspackages/db/migrations/0002_idempotency_admin_ops.sqlpackages/db/package.jsonpackages/db/src/index.test.tspackages/db/src/index.tspackages/db/src/schema.tsscripts/local-mvp-server.mjs
| - Done: every POST/PUT/DELETE route in `api` and `upload` is wrapped via `runCommand`; idempotency keys are honored from `Idempotency-Key` header; replay of the same idempotency key returns the original result; vitest covers replay for at least workspace-create, api-key-create, artifact-delete. | ||
|
|
||
| ### 2. Enforce native rate-limit bindings | ||
| ### 1. Enforce native rate-limit bindings |
There was a problem hiding this comment.
Fix MD022 heading spacing in backlog subsections.
These headings are missing the required blank line below them, which is triggering markdownlint warnings.
Also applies to: 172-172, 177-177, 182-182, 187-187, 192-192, 197-197, 202-202, 207-207, 212-212, 217-217, 222-222, 227-227, 232-232
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 167-167: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/ops/project-status.md` at line 167, Multiple backlog subsection headings
(e.g., "### 1. Enforce native rate-limit bindings" and the other headings noted)
are missing the required blank line below them causing MD022 markdownlint
warnings; fix by inserting a single blank line immediately after each of those
"###" headings (the ones at the listed occurrences) so every heading is followed
by an empty line before the next paragraph or list.
The migrate runner was hardcoded to apply only 0001_mvp_postgres.sql, so the new 0002_idempotency_admin_ops.sql was never running on PR preview Neon branches. Switch to enumerating the migrations directory in lex order so future migrations land automatically. All current migrations are idempotent (if exists / if not exists), so re-running 0001 on already-migrated branches is safe.
Postgres refuses to drop NOT NULL on a column that's part of a primary key (error 42P16). Reorder 0002 to drop the composite PK first, then the FK and NOT NULL, then recreate the unique index with NULLS NOT DISTINCT.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/scripts/migrate.mjs`:
- Around line 16-21: The migration loop currently applies every SQL file
unconditionally (iterating over files, resolving migrationsDir, reading file and
calling sql.unsafe) which forces idempotent SQL and offers no resume on failure;
modify the script to create and use a migration tracking table (e.g.,
schema_migrations) at startup, query that table to skip already-applied
filenames, run each migration inside a transaction (or ensure atomicity), and
after a successful sql.unsafe insert a record for the filename into
schema_migrations before writing the "Applied" message so failed runs can resume
and already-applied migrations are not re-executed.
- Line 11: The migrationsDir currently uses resolve("migrations") which is
anchored to process.cwd(); change it to resolve relative to the script file
using import.meta.url so the path points to the package's migrations folder
regardless of where the script is run. Replace the current migrationsDir
assignment (the resolve("migrations") use) with a resolution based on
import.meta.url (e.g., convert import.meta.url to a directory path with
fileURLToPath or use new URL and then path.resolve) so migrationsDir references
the script's directory + "migrations".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cd761d8f-8413-4232-b887-b2749b49ec58
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/db/scripts/migrate.mjs
|
|
||
| const migrationPath = resolve("migrations/0001_mvp_postgres.sql"); | ||
| const sqlText = await readFile(migrationPath, "utf8"); | ||
| const migrationsDir = resolve("migrations"); |
There was a problem hiding this comment.
Anchor migrations directory to script location, not CWD.
The resolve("migrations") call resolves relative to process.cwd(), which depends on where the script is invoked from rather than where the script file lives. This can fail if the script is run from a different directory (e.g., node packages/db/scripts/migrate.mjs from repo root would look for <repo-root>/migrations instead of <repo-root>/packages/db/migrations).
📂 Proposed fix to use import.meta.url
+import { fileURLToPath } from "node:url";
+import { dirname } from "node:path";
+
+const __filename = fileURLToPath(import.meta.url);
+const __dirname = dirname(__filename);
+
-const migrationsDir = resolve("migrations");
+const migrationsDir = resolve(__dirname, "..", "migrations");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const migrationsDir = resolve("migrations"); | |
| import { fileURLToPath } from "node:url"; | |
| import { dirname } from "node:path"; | |
| const __filename = fileURLToPath(import.meta.url); | |
| const __dirname = dirname(__filename); | |
| const migrationsDir = resolve(__dirname, "..", "migrations"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/scripts/migrate.mjs` at line 11, The migrationsDir currently uses
resolve("migrations") which is anchored to process.cwd(); change it to resolve
relative to the script file using import.meta.url so the path points to the
package's migrations folder regardless of where the script is run. Replace the
current migrationsDir assignment (the resolve("migrations") use) with a
resolution based on import.meta.url (e.g., convert import.meta.url to a
directory path with fileURLToPath or use new URL and then path.resolve) so
migrationsDir references the script's directory + "migrations".
Comment on lines
+16
to
+21
| for (const file of files) { | ||
| const path = resolve(migrationsDir, file); | ||
| const sqlText = await readFile(path, "utf8"); | ||
| await sql.unsafe(sqlText); | ||
| process.stdout.write(`Applied ${path}\n`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Ensure migrations are idempotent and consider tracking applied migrations.
The script re-runs all migrations every time without tracking which have been applied. This pattern requires all migration SQL to be idempotent (using CREATE TABLE IF NOT EXISTS, CREATE INDEX IF NOT EXISTS, etc.). If a migration fails mid-stream, there's no resume capability—subsequent runs will attempt to re-apply all migrations from the beginning.
Consider adding a migration tracking table (e.g., schema_migrations) to record which migrations have been successfully applied, or document that all migrations must be written to be safely re-runnable. Production-grade migration tools like Flyway and Liquibase use tracking tables to enable incremental application and rollback support.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/scripts/migrate.mjs` around lines 16 - 21, The migration loop
currently applies every SQL file unconditionally (iterating over files,
resolving migrationsDir, reading file and calling sql.unsafe) which forces
idempotent SQL and offers no resume on failure; modify the script to create and
use a migration tracking table (e.g., schema_migrations) at startup, query that
table to skip already-applied filenames, run each migration inside a transaction
(or ensure atomicity), and after a successful sql.unsafe insert a record for the
filename into schema_migrations before writing the "Applied" message so failed
runs can resume and already-applied migrations are not re-executed.
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-4.isaac-a46.workers.dev |
- apps/api: use adminActor helper for cleanup so system tokens record correct actor.type. - packages/commands, packages/db: make SqlExecutor.transaction required; drop silent fallback that ran handlers without a transaction. createPostgresHttpExecutor now throws since HTTP cannot span transactions. - packages/db: include actor.type in LocalRepository admin idempotency cache keys so admin+system with the same id cannot collide. - packages/db/schema: switch idempotency_records unique to unique(...).nullsNotDistinct() to reflect 0002 migration.
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-4.isaac-a46.workers.dev |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/src/index.ts (1)
458-468:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
batchSizein the local cleanup implementation.
LocalRepository.runCleanupnow acceptsbatchSize, butrunCleanupSyncstill processes every expired artifact/session. That makes the local path diverge fromPostgresRepository.runCleanupInternaland can expire far more records than the configured cap during local smoke/manual preview.Suggested fix
- private runCleanupSync(input: { actor: AdminActor; dryRun: boolean; now: string }) { + private runCleanupSync(input: { actor: AdminActor; dryRun: boolean; batchSize?: number; now: string }) { + const limit = input.batchSize ?? 100; + const nowMs = new Date(input.now).getTime(); + const expiredArtifactsToProcess = [...this.artifacts.values()] + .filter((artifact) => artifact.status === "active" && new Date(artifact.expires_at).getTime() <= nowMs) + .sort((left, right) => left.expires_at.localeCompare(right.expires_at)) + .slice(0, limit); + const expiredSessionsToProcess = [...this.uploadSessions.values()] + .filter((session) => session.status === "pending" && new Date(session.expires_at).getTime() <= nowMs) + .sort((left, right) => left.expires_at.localeCompare(right.expires_at)) + .slice(0, limit); + let expiredArtifacts = 0; let expiredUploadSessions = 0; - for (const artifact of this.artifacts.values()) { - if (artifact.status === "active" && new Date(artifact.expires_at).getTime() <= new Date(input.now).getTime()) { - expiredArtifacts += 1; - if (!input.dryRun) { - artifact.status = "expired"; - artifact.deleted_at = input.now; - artifact.delete_reason = "expired"; - } - } + for (const artifact of expiredArtifactsToProcess) { + expiredArtifacts += 1; + if (!input.dryRun) { + artifact.status = "expired"; + artifact.deleted_at = input.now; + artifact.delete_reason = "expired"; + artifact.updated_at = input.now; + } } - for (const session of this.uploadSessions.values()) { - if (session.status === "pending" && new Date(session.expires_at).getTime() <= new Date(input.now).getTime()) { - expiredUploadSessions += 1; - if (!input.dryRun) { - session.status = "expired"; - } - } + for (const session of expiredSessionsToProcess) { + expiredUploadSessions += 1; + if (!input.dryRun) { + session.status = "expired"; + } }Also applies to: 471-516
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/index.ts` around lines 458 - 468, The local cleanup path ignores batchSize—update runCleanup to pass input.batchSize into runCleanupSync and modify LocalRepository.runCleanup / runCleanupSync (and any helper called from lines 471-516) to accept an optional batchSize parameter and enforce it when collecting/deleting expired artifacts/sessions (i.e., stop processing once batchSize records are removed or returned in a single run). Ensure the idempotent key logic remains unchanged while threading the batchSize through the calls so the local implementation mirrors PostgresRepository.runCleanupInternal's capping behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/db/src/index.ts`:
- Around line 458-468: The local cleanup path ignores batchSize—update
runCleanup to pass input.batchSize into runCleanupSync and modify
LocalRepository.runCleanup / runCleanupSync (and any helper called from lines
471-516) to accept an optional batchSize parameter and enforce it when
collecting/deleting expired artifacts/sessions (i.e., stop processing once
batchSize records are removed or returned in a single run). Ensure the
idempotent key logic remains unchanged while threading the batchSize through the
calls so the local implementation mirrors
PostgresRepository.runCleanupInternal's capping behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 708505d2-4454-43b3-baed-e881d3eef084
📒 Files selected for processing (4)
apps/api/src/index.tspackages/commands/src/index.tspackages/db/src/index.tspackages/db/src/schema.ts
Mirror PostgresRepository.runCleanupInternal: order expired artifacts/sessions by expires_at, slice to batchSize (default 100), and set artifact.updated_at when expiring. Prevents the local path from blowing past the cap during smoke and manual preview runs.
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-4.isaac-a46.workers.dev |
This was referenced May 22, 2026
2 tasks
4 tasks
This was referenced May 23, 2026
isuttell
added a commit
that referenced
this pull request
May 24, 2026
#4) (#46) * feat(web): wire dashboard routes to live data, mutations, and error toasts Replace the EmptyState/no-loader fallbacks across the authed dashboard with real loaders against the existing /v1/web/* endpoints, wire the previously dead mutations, and surface api error envelopes as toasts. - dashboard: loads GET /v1/web/workspace + recent artifacts + recent audit in parallel; renders a usage-policy card, recent tables, and a first-run key card gated on default_key_first_run (secret held in component state only). - artifacts: index rows link to detail; detail surfaces entrypoint, file count, size, and pinned/lockdown badges. - keys: KeyCreateForm (POST /v1/web/keys) reveals the one-time secret in state; KeysTable Revoke (POST /v1/web/keys/{id}/revoke) refreshes via router invalidate. Both go through createServerFn -> apiFetch with a generated Idempotency-Key. - settings: SettingsForm saves name + auto-deletion days (PATCH /v1/web/settings). - audit: reads ?request_id= and highlights the matching row. - toasts: ToastProvider/useToast mounted in _authed plus an errorToast helper; failures show code + message + a link to /audit?request_id=<id>. - Access Links and the operator lockdown list stay EmptyState (endpoints deferred / Phase 4). Adds Vitest coverage for the toast mechanism, first-run card, key create/revoke, settings save, and dashboard cards. pnpm verify green (70 tasks). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(web): address CodeRabbit review on dashboard wiring - ToastProvider: clear pending auto-dismiss timers on unmount - SettingsForm: reject auto-deletion days outside 1..90 client-side - audit: mark the request_id-matched row with aria-current - test: cover the out-of-range auto-deletion guard Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(web): server-side input validation and semantic status tones Address CodeRabbit round 2: - web-mutations: validate createKey/revokeKey/saveSettings inputs against the canonical contract schemas (CreateApiKeyRequest, ApiKeyId, UpdateWebSettingsRequest) so malformed payloads never reach the API - artifact-status: map Expired->warning and Deleted->destructive badge tones, shared by the dashboard and artifacts index - FirstRunKeyCard: clarify the missing-secret copy - keys: memoize the refresh callback Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(web): reuse artifactStatusTone in detail view, drop dead fallback Address CodeRabbit round 3: - artifacts detail route now uses the shared artifactStatusTone helper - remove the unreachable neutral fallback (the tone map is exhaustive over the WebArtifactStatus enum) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(web): address CodeRabbit PR review on dashboard wiring - SettingsForm: sync local name/days state to the persisted server response after save so the form stops showing stale (untrimmed/unclamped) input. - Artifacts index + detail: render "Untitled" when an artifact title is empty. - revokeKeyFn: read data?.apiKeyId defensively so a malformed server-fn payload yields a 400 validation envelope instead of a TypeError. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
isuttell
added a commit
that referenced
this pull request
May 24, 2026
Snapshot now points at ad85175 (#46); record the operator lockdown (#45) and web loader-wiring (#46) merges in Recently Completed; strike backlog #4 as done; update the Phase 3 (~55%), web.md, ADR 0055 rows. Remaining Phase 3 code work is CLI login (#5) and smoke:web (#6), both gated on the WorkOS/Access click-ops in backlog item #1. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
4 tasks
This was referenced May 26, 2026
This was referenced May 29, 2026
isuttell
added a commit
that referenced
this pull request
Jun 14, 2026
…st-login provisioning, /v tests Cursor Bugbot (blocking, Medium) + first-pass review findings: - **Revision-link retry duplicates (Bugbot).** createAndMintAccessLink's salted-key retry ran for both make_public and create_revision_link. Share links dedupe on create (findActiveShareLink), so the retry is idempotent; revision links do NOT dedupe, so a create-ok/mint-fail retry inserted a second revision link for the same revision. Scope the retry to `type:'share'` only; revision links return the original failure. Test asserts create_revision_link does not retry. - **/v first-login provisioning (finding #1).** /v is a standalone handoff route, not under _authed, so it never provisioned the workspace member. A brand-new user signing in directly via a private link had a valid token but no member row, so the owner-scoped artifact read missed and showed the empty state. Provision via webSessionQuery in the loader before the artifact read. - **/v route tests (finding #4).** New apps/web/test/v-route.test.tsx pins the gate: unauthed → redirect + no artifact read; authed → provision-then-read in order; chromeless viewer renders; redirect path works. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
isuttell
added a commit
that referenced
this pull request
Jun 14, 2026
…ic, unified scopes (#525) * feat(publish): make publish content-only and private-first; one private_url, explicit make_public Publish and add_revision are now content-only on every surface (CLI, MCP, REST). They take no visibility input and return exactly one link, `private_url`, the login-walled clean viewer at `/v/<artifactId>` for a Workspace Member. There is no `share` input and no `shared` output anywhere; the misleading viewer_url/ shared flip is gone. Going public is a separate, explicit, revocable step: `make_public` (MCP) / `agent-paste make-public` (CLI), with `revoke_access_link` as the go-private verb. The `/artifacts/<id>` console is never returned to an agent or user. Private Link is a plain `_authed` route, not an access_links row: nothing to sign, nothing to leak, nothing to revoke. The public `/al/<publicId>` resolve gate is untouched, so existing Share Links keep resolving. A new shared ArtifactLiveViewer backs both the console and the clean `/v` viewer. make_public now reuses an Artifact's one active (non-revoked, unexpired) Share Link instead of minting a duplicate, so an Artifact has at most one live Share Link — making the "mints or reuses the one Share Link" contract true in code, not just in copy. revision links are never deduped. Unify the scope vocabulary: one set `read`/`publish`/`admin` shared verbatim by API and MCP. The old MCP-only `write`/`share` names are gone, and the mcpScopesToApiScopes/apiScopesToMcpScopes translation layer is deleted (MCP scopes are the member's stored API scopes verbatim, per ADR 0079). The four agent access-link routes require `publish` (managing your own Artifact's public access is content authority); `admin` is dashboard-only and no MCP tool needs it. web.accessLinks.lockdown.* stay `admin`. Supersedes ADR 0085 (publish-returns-one-viewer-url, which made the link flip and surfaced the lie); amends ADR 0084 (output shape) and ADR 0079 (scope vocabulary unified). Adds ADR 0086. Specs are updated as the source of truth (api/web/cli/ephemeral-publish, CONTEXT vocab, docs/mcp.md, apex agent docs). OpenAPI goldens regenerated. Early-alpha break: CLI and MCP ship in lockstep; the deployed MCP server `instructions` text still teaches `share:true` and updates on deploy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(web): clean viewer header links to /dashboard, not the banned /artifacts console The /v clean viewer's own header Wordmark linked to /artifacts/$artifactId — the dashboard console page that is explicitly never to be handed to a user. Point it at /dashboard (brand home) instead. The console stays reachable only from the dashboard's own artifact list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(mcp): make_public self-heals when a replayed create points mint at a revoked link If a client reuses the same idempotency key across a revoke (make_public → revoke_access_link → make_public with the same key), the create step replays the now-revoked link from its idempotency record and mint fails on the dead link. Real MCP clients use monotonic JSON-RPC ids and the CLI uses random UUIDs so neither collides, but the failure mode is sharp: a revoke must never lock you out of going public again. createAndMintAccessLink now retries once on a salted idempotency key when the first create→mint fails. The salted create runs the command fresh, so for a share link it reuses the artifact's active link (the DB-layer findActiveShareLink path) or mints a new one. The happy path never retries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: state the Private Link permanence guarantee explicitly We kept tripping over what the link guarantees, so write it down where the specs are the source of truth. The Private Link (`/v/<artifactId>`) is permanent and stable: derived only from the Artifact id with no token/signature/expiry, and add_revision republishes into the same id, so it never changes across revisions and live-updates to the latest Published Revision. It is always private (member only; publish never grants public access) and stops resolving only when the Artifact is deleted or swept by Auto Deletion — the expires_at in the publish response is the Artifact's content lifetime, not a link expiry. Going public is the separate, revocable Share Link. Documented in CONTEXT.md (Private Link + Share Link vocab), docs/specs/api.md (publish behavior), and ADR 0086 (decision trail). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(mcp,web): address Cursor review — revision-link retry dup, /v first-login provisioning, /v tests Cursor Bugbot (blocking, Medium) + first-pass review findings: - **Revision-link retry duplicates (Bugbot).** createAndMintAccessLink's salted-key retry ran for both make_public and create_revision_link. Share links dedupe on create (findActiveShareLink), so the retry is idempotent; revision links do NOT dedupe, so a create-ok/mint-fail retry inserted a second revision link for the same revision. Scope the retry to `type:'share'` only; revision links return the original failure. Test asserts create_revision_link does not retry. - **/v first-login provisioning (finding #1).** /v is a standalone handoff route, not under _authed, so it never provisioned the workspace member. A brand-new user signing in directly via a private link had a valid token but no member row, so the owner-scoped artifact read missed and showed the empty state. Provision via webSessionQuery in the loader before the artifact read. - **/v route tests (finding #4).** New apps/web/test/v-route.test.tsx pins the gate: unauthed → redirect + no artifact read; authed → provision-then-read in order; chromeless viewer renders; redirect path works. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
5 tasks
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
1 participant
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
Closes backlog item #1 in
docs/ops/project-status.md: wires therunCommandtransactional wrapper (ADR 0035) into every admin POST/PUT/DELETE handler in theapianduploadworkers, so retries with the sameIdempotency-Keyreplay the original result instead of double-executing. Drives ADR 0022 (idempotent mutations) anddocs/specs/api.mdto Done.Changes
packages/commands: addrunCommandorchestrator +IdempotencyInFlightError; idempotency_records claim → replay-completed | re-run-stale | 409-in-flight; audit events written inside the same tx.apps/api: every admin handler (createWorkspace,createApiKey,revokeApiKey,deleteArtifact,cleanup) now requiresIdempotency-Keyheader and routes throughrunIdempotent→ 409 on in-flight.apps/upload: upload-session create routes through the same wrapper.packages/db: new migration0002_idempotency_admin_ops.sqladdsNULLS NOT DISTINCTunique constraint for admin-without-workspace operations;PostgresRepositoryandLocalRepositoryadoptrunAdminCommand+ key the in-memory idempotency cache by actor/operation/key.packages/commands,packages/db,apps/api,apps/upload.Risk: MEDIUM
packages/commands,packages/db(schema + handlers), local harness.(workspace_id, actor_type, actor_id, operation)to prevent cross-tenant collisions; no new auth surface.INSERT ... ON CONFLICT DO NOTHINGper mutation; replays short-circuit handler work.Idempotency-Keywith400 invalid_idempotency_key. Documented indocs/specs/api.md.Test plan
pnpm verify→ 49/49 green locally.pnpm smoke:local→ green (workspace/key create, publish, agent-view, delete, content denylist)./admin/cleanup/runtwice with the sameIdempotency-Keyagainst preview env and confirm the second call returns the originalexpired_artifact_idspayload.CodeRabbit notes
298886d,aa56598). Third pass was rate-limited by the local CodeRabbit CLI; remaining biome warnings on non-null assertions are intentional (guarded by explicitdb?.xxxchecks immediately above).Summary by CodeRabbit
New Features
Documentation
Tests
Chores