AP-13: Retire repo-local ADMIN_TOKEN /admin/* path#89
Conversation
Remove legacy admin contract routes, API handlers, CLI admin verbs, and ADMIN_TOKEN bootstrap secrets. Add non-production smoke harness routes and update smokes, OpenAPI, workflows, and operator docs to use WorkOS/operator paths plus SMOKE_HARNESS_SECRET or production smoke API keys. Co-authored-by: Isaac Suttell <isaac@isaacsuttell.com>
Co-authored-by: Isaac Suttell <isaac@isaacsuttell.com>
AP-13 Ops: Retire repo-local ADMIN_TOKEN admin path
Context
Source docs
ScopeRemove the repo-local Out of scopeDo not remove the path before the migration plan and replacement smokes prove coverage. DependenciesBlocked by the legacy admin route migration plan. Implementation notesThis project is pre-launch; when behavior changes, remove old paths outright rather than adding compatibility shims. AcceptanceNo runtime route requires VerificationRun focused admin/operator tests, Remote Cursor handoffStart by reading |
|
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 (3)
WalkthroughThis PR removes the repo-local ADMIN_TOKEN surface and /admin/* routes and CLI verbs, replaces admin-token auth with SMOKE_HARNESS_SECRET for non-production test endpoints and a hosted production smoke API key, updates runtime principal/auth types and rate-limit/scope logic, removes admin helpers from the API worker, converts tests and scripts to shared smoke-harness helpers, updates the API client to drop admin APIs, and revises documentation and CI workflows to the new smoke-harness and WorkOS operator flows. sequenceDiagram
participant CLI
participant SmokeHarness
participant APIWorker
participant DB
participant R2
CLI->>SmokeHarness: POST /__test__/provision-smoke (SMOKE_HARNESS_SECRET)
SmokeHarness->>APIWorker: provision request with harness auth
APIWorker->>DB: create workspace and one-time api key
APIWorker-->>SmokeHarness: return workspace id and api key secret
CLI->>APIWorker: publish using Authorization: Bearer <api-key>
APIWorker->>R2: store artifact bytes
SmokeHarness->>APIWorker: POST /__test__/delete-artifact (harness auth)
APIWorker->>R2: delete object and write denylist key
APIWorker->>DB: scheduled cleanup via /__test__/run-cleanup or scheduled handler
Possibly related issues
Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
CI failed with ReferenceError: key is not defined after admin path removal. Also drop stale adminEnv default in runCliJson. Co-authored-by: Isaac Suttell <isaac@isaacsuttell.com>
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-89.isaac-a46.workers.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
scripts/deploy-pr-preview.mjs (1)
304-306: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUnused
hmacBase64Urlfunction.This function appears to be dead code left over from the removed admin-token hashing logic. Consider removing it for cleaner maintenance.
🧹 Proposed cleanup
-function hmacBase64Url(value, secret) { - return createHmac("sha256", secret).update(value).digest("base64url"); -}🤖 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 `@scripts/deploy-pr-preview.mjs` around lines 304 - 306, Remove the dead helper function hmacBase64Url from scripts/deploy-pr-preview.mjs: locate the function declaration function hmacBase64Url(value, secret) and delete it (and any related imports like createHmac if now unused), after confirming there are no remaining references to hmacBase64Url elsewhere in the codebase; ensure the file still lints/compiles after removing the unused function and clean up any now-unused imports.scripts/smoke-hosted.mjs (2)
61-66:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUndefined variable
keywill cause runtime error.Line 65 references
key.secret, butkeyis not defined in this file. This appears to be a leftover from the removed admin flow. The variable should beprovisioned.apiKeySecret.🐛 Proposed fix
if (target !== "production") { await assertArtifactRateLimitFires(published.view_url); await assertBytesPurgedAfterDelete(published); await assertBytesPurgedAfterExpiry(userEnv); - await assertActorRateLimitFires(key.secret); + await assertActorRateLimitFires(provisioned.apiKeySecret); }🤖 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 `@scripts/smoke-hosted.mjs` around lines 61 - 66, The test references an undefined variable key when calling assertActorRateLimitFires, causing a runtime error; replace the use of key.secret with the correct provisioned.apiKeySecret value (i.e., call assertActorRateLimitFires(provisioned.apiKeySecret)) so the function uses the provisioned API key secret; update the call site in the block that checks non-production targets where assertArtifactRateLimitFires, assertBytesPurgedAfterDelete, assertBytesPurgedAfterExpiry, and assertActorRateLimitFires are invoked.
131-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUndefined variable
adminEnvwill cause runtime error.The default parameter
adminEnvon line 131 is not defined anywhere in the file. It was likely removed as part of the admin-token retirement but this reference remains. Since all call sites passuserEnvexplicitly, this may not be hit, but it's a latent bug.🐛 Proposed fix
-async function runCliJson(args, commandEnv = adminEnv) { +async function runCliJson(args, commandEnv = userEnv) {Note: If
userEnvis not in scope at function definition time, you may need to make this parameter required or restructure the code.🤖 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 `@scripts/smoke-hosted.mjs` at line 131, The function runCliJson currently uses an undefined default parameter adminEnv which will throw at runtime; remove the adminEnv default and either (A) set the default to userEnv if userEnv is guaranteed to be in scope at definition time, or (B) make the second parameter required (no default) and update callers to pass userEnv explicitly; update the function signature for runCliJson and any call sites accordingly, referencing the runCliJson function and the adminEnv/userEnv symbols to locate the change.docs/specs/phases.md (1)
33-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify Phase 1 admin deliverables post-AP-13 retirement.
Line 33 still lists "repo-local admin CLI" as a Phase 1 deliverable, while line 25 explicitly notes the legacy
ADMIN_TOKENwas retired in AP-13. This creates potential confusion about the current Phase 1 deliverable set.Consider either removing the admin CLI reference or adding a clarifying note that it was later retired.
📝 Suggested revision
-- Admin: internal REST APIs plus repo-local admin CLI. +- Operator: WorkOS-based web operator routes (legacy repo-local admin CLI retired in AP-13).Or add a clarifying note:
-- Admin: internal REST APIs plus repo-local admin CLI. +- Operator: internal REST APIs, web operator routes, and WorkOS admin auth (repo-local admin CLI later retired in AP-13).🤖 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/specs/phases.md` at line 33, The line "Admin: internal REST APIs plus repo-local admin CLI." is inconsistent with AP-13 retirement of the legacy ADMIN_TOKEN; either remove the "repo-local admin CLI" phrase or append a short clarifying note that the repo-local admin CLI (and ADMIN_TOKEN) was retired in AP-13 and is no longer a Phase 1 deliverable so readers aren’t confused—update the sentence that contains "Admin: internal REST APIs plus repo-local admin CLI." accordingly.docs/specs/mvp.md (1)
160-160:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale admin CLI reference in retention section.
This line still references "admin CLI" for triggering cleanup, but the admin CLI was removed in AP-13. Per the updated acceptance criteria (docs/specs/acceptance.md line 53), manual cleanup now uses the non-production harness endpoint
POST /__test__/run-cleanup.📝 Suggested revision
-Cleanup runs in the API Worker scheduled handler and can also be triggered through the admin CLI. +Cleanup runs in the API Worker scheduled handler and can also be triggered through the non-production smoke harness endpoint.🤖 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/specs/mvp.md` at line 160, Replace the stale phrase "admin CLI" in the sentence that begins "Cleanup runs in the API Worker scheduled handler..." with the correct manual trigger: the non-production harness endpoint POST /__test__/run-cleanup, so the sentence reads that cleanup runs on the scheduled API Worker handler and can also be triggered via the non-production harness endpoint POST /__test__/run-cleanup per the updated acceptance criteria.
🤖 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/cli/README.md`:
- Around line 65-70: Update the broken relative links in the CLI README: replace
the `../docs/...` targets used on the lines showing `agent-paste login` (ADR
0068 link) and the admin operations spec link (used in the paragraph after the
command table) with the correct two-level-up path `../../docs/...` so the ADR
and admin spec resolve correctly from apps/cli/README.md.
In `@scripts/local-mvp-server.mjs`:
- Around line 11-12: The environment variable precedence for smokeHarnessSecret
is inverted compared to smoke-harness.mjs; update the assignment of the
smokeHarnessSecret constant to read process.env.AGENT_PASTE_SMOKE_HARNESS_SECRET
first, then process.env.SMOKE_HARNESS_SECRET, and keep the same default
"local-smoke-harness-secret" so both modules use the same precedence when both
vars are set.
In `@scripts/README.md`:
- Around line 109-110: Update the runbooks that currently mention only
AGENT_PASTE_PR_SMOKE_HARNESS_SECRET so they explicitly state that
scripts/smoke-hosted.mjs also accepts AGENT_PASTE_PREVIEW_SMOKE_HARNESS_SECRET
and will fall back to that value if the PR-specific variable is not set;
reference both environment variable names (AGENT_PASTE_PR_SMOKE_HARNESS_SECRET
and AGENT_PASTE_PREVIEW_SMOKE_HARNESS_SECRET) and the scripts/smoke-hosted.mjs
behavior, and mirror the README example showing
AGENT_PASTE_PREVIEW_SMOKE_HARNESS_SECRET for pnpm smoke:preview to avoid
operator confusion.
In `@scripts/smoke-harness.mjs`:
- Around line 68-70: The error messages for failed fetches (e.g., the throw in
the force-expire response check and other functions runSmokeCleanup,
deleteSmokeArtifact, listR2Keys, fetchDenylistKey) only include status codes;
change them to include the response body for consistency with
provisionSmokeWorkspace by awaiting response.text() (or response.json() where
appropriate) and appending that body to the thrown Error message (e.g., throw
new Error(`... returned ${response.status}: ${body}`)) so failures include both
status and body for easier debugging.
---
Outside diff comments:
In `@docs/specs/mvp.md`:
- Line 160: Replace the stale phrase "admin CLI" in the sentence that begins
"Cleanup runs in the API Worker scheduled handler..." with the correct manual
trigger: the non-production harness endpoint POST /__test__/run-cleanup, so the
sentence reads that cleanup runs on the scheduled API Worker handler and can
also be triggered via the non-production harness endpoint POST
/__test__/run-cleanup per the updated acceptance criteria.
In `@docs/specs/phases.md`:
- Line 33: The line "Admin: internal REST APIs plus repo-local admin CLI." is
inconsistent with AP-13 retirement of the legacy ADMIN_TOKEN; either remove the
"repo-local admin CLI" phrase or append a short clarifying note that the
repo-local admin CLI (and ADMIN_TOKEN) was retired in AP-13 and is no longer a
Phase 1 deliverable so readers aren’t confused—update the sentence that contains
"Admin: internal REST APIs plus repo-local admin CLI." accordingly.
In `@scripts/deploy-pr-preview.mjs`:
- Around line 304-306: Remove the dead helper function hmacBase64Url from
scripts/deploy-pr-preview.mjs: locate the function declaration function
hmacBase64Url(value, secret) and delete it (and any related imports like
createHmac if now unused), after confirming there are no remaining references to
hmacBase64Url elsewhere in the codebase; ensure the file still lints/compiles
after removing the unused function and clean up any now-unused imports.
In `@scripts/smoke-hosted.mjs`:
- Around line 61-66: The test references an undefined variable key when calling
assertActorRateLimitFires, causing a runtime error; replace the use of
key.secret with the correct provisioned.apiKeySecret value (i.e., call
assertActorRateLimitFires(provisioned.apiKeySecret)) so the function uses the
provisioned API key secret; update the call site in the block that checks
non-production targets where assertArtifactRateLimitFires,
assertBytesPurgedAfterDelete, assertBytesPurgedAfterExpiry, and
assertActorRateLimitFires are invoked.
- Line 131: The function runCliJson currently uses an undefined default
parameter adminEnv which will throw at runtime; remove the adminEnv default and
either (A) set the default to userEnv if userEnv is guaranteed to be in scope at
definition time, or (B) make the second parameter required (no default) and
update callers to pass userEnv explicitly; update the function signature for
runCliJson and any call sites accordingly, referencing the runCliJson function
and the adminEnv/userEnv symbols to locate the change.
🪄 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: fd074002-3e8e-46dd-a6b6-690ac6ee1328
📒 Files selected for processing (55)
.env.example.github/workflows/deploy-production.yml.github/workflows/pr-preview.ymlapps/api/.dev.vars.exampleapps/api/src/error-envelope.test.tsapps/api/src/index.test.tsapps/api/src/index.tsapps/api/src/local-mvp.test.tsapps/cli/README.mdapps/cli/src/index.tsapps/cli/test/index.test.tsapps/cli/test/local.test.tsdocs/ops/ap-12-migration-plan.mddocs/ops/bootstrap-hosting-checklist.mddocs/ops/first-deploy.mddocs/ops/project-status.mddocs/ops/runbook-logpush.mddocs/ops/runbook-rotation.mddocs/ops/runbook-workos.mddocs/ops/status/changelog.mddocs/ops/status/coverage.mddocs/ops/status/hosted-ops.mddocs/ops/status/phase-backlog.mddocs/ops/worker-runtime-todo.mddocs/specs/acceptance.mddocs/specs/admin.mddocs/specs/api.mddocs/specs/features.mddocs/specs/local-dev.mddocs/specs/mvp.mddocs/specs/phases.mdexamples/local-harness/README.mdpackage.jsonpackages/api-client/src/index.tspackages/api-client/test/client.test.tspackages/auth/src/index.tspackages/contracts/openapi/api.jsonpackages/contracts/src/mvp-contracts.test.tspackages/contracts/src/openapi/api.tspackages/contracts/src/openapi/shared.tspackages/contracts/src/routes.tspackages/worker-runtime/src/index.tspackages/worker-runtime/src/principal.tspackages/worker-runtime/src/rate-limit.test.tspackages/worker-runtime/src/rate-limit.tspackages/worker-runtime/src/registrar.tsscripts/README.mdscripts/bootstrap-secrets.mjsscripts/deploy-pr-preview.mjsscripts/lighthouse-dashboard-a11y.mjsscripts/local-mvp-server.mjsscripts/smoke-harness.mjsscripts/smoke-hosted.mjsscripts/smoke-local-mvp.mjsscripts/smoke-web-api.mjs
💤 Files with no reviewable changes (7)
- package.json
- packages/worker-runtime/src/index.ts
- packages/worker-runtime/src/registrar.ts
- packages/worker-runtime/src/rate-limit.ts
- packages/contracts/src/mvp-contracts.test.ts
- packages/worker-runtime/src/rate-limit.test.ts
- packages/contracts/src/openapi/api.ts
- Fix relative doc links in apps/cli/README.md - Use smokeHarnessSecretFromEnv() in local-mvp-server for consistent precedence - Include response bodies in smoke-harness error messages - Document preview vs PR harness secret fallback in scripts README and runbooks Co-authored-by: Isaac Suttell <isaac@isaacsuttell.com>
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-89.isaac-a46.workers.dev |
Remove unused hmacBase64Url from deploy-pr-preview, require explicit CLI env in smoke-hosted runCliJson, and clarify phases/mvp docs after admin path retirement. Other review items were already fixed in 88a1082. Co-authored-by: Isaac Suttell <isaac@isaacsuttell.com>
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-89.isaac-a46.workers.dev |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/cli/README.md`:
- Line 65: The README table entry for the CLI command `agent-paste login`
incorrectly links to ADR 0068 (web AuthKit); update the link to point to ADR
0060 which documents the CLI loopback login flow. Locate the table row
containing `agent-paste login` in apps/cli/README.md and replace the ADR
reference `0068`/URL with the ADR 0060 reference/URL so the CLI login command
points to the correct documentation.
In `@docs/specs/mvp.md`:
- Line 224: The MVP acceptance bullets are contradictory about auth/surface:
reconcile references to "agent-paste login" and the "/v1/web/*" surface with the
declared API-key-only public auth and deferred OAuth/dashboard; either remove or
mark "agent-paste login" and "/v1/web/*" as non-MVP (future work) or update the
auth scope to include them. Edit the MVP list so the entry about provisioning a
workspace/API key via "agent-paste login" explicitly states it is
non-production/future if OAuth/dashboard is deferred, and either delete or
reword the "/v1/web/*" acceptance to match API-key-only public auth (e.g., state
only API-key endpoints are in MVP). Ensure the symbols "agent-paste login" and
"/v1/web/*" are updated consistently across the document.
In `@docs/specs/phases.md`:
- Line 56: Phase 2's exit criteria currently includes the "web dashboard, WorkOS
operator routes, and `agent-paste login` with Codex assistance", which overlaps
with Phase 3; remove that phrase from the Phase 2 exit criteria and instead add
a clear statement in the Phase 3 scope that it is responsible for delivering the
web dashboard, WorkOS operator routes, and `agent-paste login` with Codex
assistance so the milestone boundaries are distinct and testable (update any
neighboring sentences that imply those surfaces are done in Phase 2 to avoid
contradiction).
🪄 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: 7c131784-262d-499e-86fb-929c2144f952
📒 Files selected for processing (11)
apps/cli/README.mddocs/ops/first-deploy.mddocs/ops/runbook-rotation.mddocs/ops/runbook-workos.mddocs/specs/mvp.mddocs/specs/phases.mdscripts/README.mdscripts/deploy-pr-preview.mjsscripts/local-mvp-server.mjsscripts/smoke-harness.mjsscripts/smoke-hosted.mjs
💤 Files with no reviewable changes (1)
- scripts/deploy-pr-preview.mjs
Point CLI login at ADR 0060, mark login and /v1/web/* as Phase 3 in mvp.md, and keep Phase 2 exit criteria operational without dashboard overlap.
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-89.isaac-a46.workers.dev |
|
agent-paste PR preview resources were cleaned up. The pr-preview-${context.issue.number} environment is left in place; remove it from the GitHub UI if desired. |
2 participants
CodeRabbit follow-up
Addressed the remaining actionable review items in the latest commit. Several items were already landed in
88a1082(CLI README../../docs/...links,smokeHarnessSecretFromEnv()inlocal-mvp-server.mjs, harness secret fallback docs inscripts/README.md, and response bodies insmoke-harness.mjserrors).This commit:
hmacBase64Urlfromscripts/deploy-pr-preview.mjs(leftover fromADMIN_TOKEN_HASHbootstrap).commandEnvforrunCliJsoninscripts/smoke-hosted.mjs(no staleadminEnv/ implicit default).docs/specs/phases.mdanddocs/specs/mvp.mdto drop repo-local admin CLI acceptance language.Already fixed earlier (
0eac26c):assertActorRateLimitFires(provisioned.apiKeySecret)— thekey.secretReferenceErrorfrom the first preview smoke failure.pnpm verifypassed locally on the branch head.Linear Issue: AP-13
Summary by CodeRabbit
Breaking Changes
New Features
Documentation