feat: Plan 9 PR D — billing + admin - #29
Merged
Merged
Conversation
CP — billing (fixed-tier Stripe, Decision 7): - POST /v1/billing/checkout-session (tenant-scoped, paid tier) and POST /v1/billing/portal-session (404 without a subscription). - POST /webhooks/stripe mounted with express.raw before the json parser; signature verification mandatory when STRIPE_WEBHOOK_SECRET is set; unsigned webhooks refused in production config; idempotent dedup via stripe_webhook_events (event.id PK); handles checkout.session.completed + customer.subscription.updated/deleted (upsert subscriptions with tenant_id/plan metadata carried through the checkout session). - BillingTransport interface (StripeBillingTransport + StubBillingTransport); no live Stripe in tests. Fixed tiers wired via STRIPE_PRICE_* env. CP — admin (platform staff, Decision 5): - GET /v1/tenants: staff (OIDC isStaff or admin:read API key) see all; members see their memberships; API key sees its own tenant. - GET /v1/admin/bootstrap-issuers + PATCH (Root.weight de-emphasis + reason + approved_by; issuers.trust_weight untouched per design). - GET /v1/admin/issuers/unverified (verification queue). - isPlatformStaff helper unifies OIDC staff + admin:read keys. - migration 014: UNIQUE (tenant_id, stripe_subscription_id) for webhook idempotency (justified per Decision 12). - integration coverage: checkout/portal/webhook (dedup, subscription upsert), admin authz/isolation, bootstrap PATCH, issuer queue. Dashboard: - admin view (/admin): graph health, tenants, bootstrap registry editor (stepwise Root.weight de-emphasis), issuer verification queue. - BillingLink (Stripe portal) in provider + agent-builder views. - vitest: admin panels (TenantList, GraphHealth, BootstrapEditor, IssuerVerificationQueue). Verification: CP tsc clean, 148 unit + 48 integration pass; dashboard typecheck clean, 53 vitest pass, vite build succeeds. Plan 9 complete after this PR; next is §13 step 14 (bootstrap cold-start seed).
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
WalkthroughAdded Stripe billing, tenant and issuer administration APIs, persistence, dashboard panels, billing links, route wiring, migrations, and integration tests. ChangesBilling and administration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant BillingRoute
participant BillingService
participant StripeTransport
participant SubscriptionRepository
Dashboard->>BillingRoute: request checkout or portal URL
BillingRoute->>BillingService: create session
BillingService->>StripeTransport: create Stripe session
StripeTransport-->>BillingService: return session URL
BillingService-->>BillingRoute: return URL
BillingRoute-->>Dashboard: return URL
StripeTransport->>BillingService: construct webhook event
BillingService->>SubscriptionRepository: persist subscription state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoPlan 9: Stripe billing + admin APIs + dashboard admin panels
AI Description
Diagram
High-Level Assessment
Files changed (29)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dashboard/src/pages/AgentBuilderHomePage.tsx (1)
102-139: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRender tenant billing without a selected principal.
Billing is scoped to the caller tenant, not to
selectedId. The current conditional hides billing when the tenant has no principals or when principal loading fails. Move the Billing panel outside theselectedIdconditional.🤖 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 `@dashboard/src/pages/AgentBuilderHomePage.tsx` around lines 102 - 139, Move the Billing section containing the BillingLink component outside the selectedId conditional in AgentBuilderHomePage, while keeping the keys, score, attestations, and issuer panels gated by selectedId. Ensure the billing panel renders for the caller tenant even when no principal is selected or principal loading fails.
🤖 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 `@control-plane/migrations/014_subscription_unique/migration.sql`:
- Around line 8-9: Update the subscription identity constraints around
subscription_tenant_stripe_unique: add uniqueness for stripe_subscription_id
globally, and if historical rows are retained, enforce one active subscription
per tenant with a partial unique constraint. Review every subscription upsert
and change its conflict target to match the selected constraint(s), preserving
the documented identity rules.
- Around line 8-9: Update migration 014_subscription_unique and the migration
runner to support this migration as non-transactional: check subscriptions for
duplicate tenant_id/stripe_subscription_id pairs, build the unique index with
CREATE UNIQUE INDEX CONCURRENTLY outside BEGIN/COMMIT, and record
_verilink_migrations only after successful completion. Preserve transactional
behavior for all other migrations.
In `@control-plane/src/__tests__/integration/billing-admin.test.ts`:
- Around line 101-141: Extend the billing webhook integration tests with two
cases covering signature enforcement: when STRIPE_WEBHOOK_SECRET is configured
but stripe-signature is missing, and when an unsigned webhook is delivered in
production. Assert HTTP 401 for both cases, while preserving the fixed event ID
in the existing idempotency test and relying on resetTestData isolation.
In `@control-plane/src/app.ts`:
- Around line 38-40: Move the Stripe webhook middleware and webhookStripeRouter
registration before the global apiLimiter application, or explicitly exempt
/webhooks/stripe from apiLimiter. Preserve express.raw({ type:
'application/json' }) before webhookStripeRouter so signature verification
continues receiving the raw body.
In `@control-plane/src/domains/billing/billingService.ts`:
- Line 105: Update the plan assignment in both billing branches to avoid
defaulting missing session metadata to “pro”. Resolve the plan from the Stripe
price ID when possible; otherwise store “unknown” and trigger the established
alerting path, ensuring absent metadata never grants Pro entitlements.
- Around line 92-132: The checkout.session.completed branch in applyEvent
overwrites newer subscription state with active status and a null
currentPeriodEnd. Preserve existing currentPeriodEnd and status on checkout
upserts, or add and propagate event timestamps through
subRepo.upsertSubscription so older events are ignored; ensure
customer.subscription.updated/deleted transitions remain order-safe.
- Around line 81-87: Remove the redundant try/catch around applyEvent and
markWebhookProcessed in the webhook processing flow, leaving both awaits
directly in sequence so errors propagate naturally and markWebhookProcessed
remains skipped when applyEvent fails.
- Around line 22-27: Validate that WEB_APP_URL is configured before constructing
Stripe checkout URLs, and fail fast with a clear error instead of using relative
fallbacks for successUrl and cancelUrl. Apply the same absolute-URL guard to
returnUrl in the surrounding billing flow, preserving the existing URL paths
when the base URL is valid.
In `@control-plane/src/domains/billing/billingTransport.ts`:
- Around line 80-93: Restrict StubBillingTransport selection in
createBillingTransport to an explicit test-only opt-in such as BILLING_STUB=1;
otherwise require config.stripe.secretKey and return StripeBillingTransport,
preventing staging or preview deployments from accepting unsigned webhook
payloads.
- Around line 21-26: Update the Stripe client initialization in
StripeBillingTransport’s constructor to pass the explicit apiVersion value
"2024-09-30.acacia" in the Stripe options, preserving the existing secretKey
usage.
- Around line 35-42: Update the Checkout Session creation in billingTransport.ts
to pass opts.metadata through subscription_data.metadata so Stripe copies
tenant_id to subscription events. In billingService.ts, when update or delete
events lack tenant_id, resolve the tenant using the stored
stripe_subscription_id instead of dropping the event; add coverage for
metadata-free deletion. Apply changes at billingTransport.ts lines 35-42 and
billingService.ts lines 111-115.
In `@control-plane/src/domains/billing/subscriptionRepository.ts`:
- Around line 15-23: Update getSubscriptionByTenant so the SQL query prioritizes
active subscriptions before ordering by created_at DESC, ensuring the returned
row is active when one exists while retaining the newest-row fallback for
tenants without an active subscription.
In `@control-plane/src/domains/billing/webhookDedup.ts`:
- Around line 14-19: Update the webhook persistence flow around the pool.query
INSERT so stripe_webhook_events.payload is subject to a defined retention or
redaction policy. Avoid storing the full PII-bearing Stripe event indefinitely
by either removing sensitive fields before JSON serialization or ensuring stored
payloads are automatically deleted after the approved retention period.
- Around line 10-21: Update claimWebhookEvent so an existing
stripe_webhook_events row is eligible for reinsertion when processed_at is NULL,
while preserving the conflict behavior for processed events. Ensure the SQL
conflict handling allows failed, unprocessed webhook events to be reclaimed and
still returns the correct boolean claim result.
In `@control-plane/src/routes/admin.ts`:
- Around line 56-76: Require at least one of current_weight or
de_emphasis_reason in the update handler before assigning approved_by or calling
updateBootstrapIssuer. Reject requests containing only principal_id with the
existing bad-request error mechanism, while preserving validation and updates
for either editable field.
In `@control-plane/src/routes/billing.ts`:
- Around line 39-48: Update the billing portal and checkout route handlers to
enforce the billing:write scope in addition to requireTenant(req), using the
existing scoped-auth authorization mechanism from auth middleware. Apply the
check consistently to both endpoints before invoking billingService methods,
while preserving their current success responses.
- Around line 24-32: Update the billing route handler’s request-body validation
alongside the existing tier check to require customer_email to be a string with
a valid email format before calling billingService.createCheckoutSession. Reject
missing, non-string, and malformed values with the route’s established
BAD_REQUEST AppError, while preserving the existing checkout-session arguments
for valid input.
In `@control-plane/src/routes/webhookStripe.ts`:
- Around line 23-29: Update the catch block around
billingService.handleWebhookEvent to emit a structured error log before
returning the AppError response, including the webhook event type and original
error details; preserve the existing response conversion and status handling.
In `@dashboard/src/pages/admin/BootstrapEditor.tsx`:
- Around line 72-79: Update the BootstrapEditor submission flow around apply and
the STEPS buttons so the current-weight option remains enabled when
de_emphasis_reason has changed, allowing a reason-only patch; retain the
disabled state when there are no unsaved changes or while busy. Add an
interaction test covering a reason-only update without changing current_weight.
In `@dashboard/src/pages/BillingLink.tsx`:
- Line 34: Update the conditional message element rendering {msg} in BillingLink
to include role="status", ensuring asynchronous billing errors and
no-subscription results are announced to assistive technology while preserving
the existing styling and conditional rendering.
---
Outside diff comments:
In `@dashboard/src/pages/AgentBuilderHomePage.tsx`:
- Around line 102-139: Move the Billing section containing the BillingLink
component outside the selectedId conditional in AgentBuilderHomePage, while
keeping the keys, score, attestations, and issuer panels gated by selectedId.
Ensure the billing panel renders for the caller tenant even when no principal is
selected or principal loading fails.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8534ef7-cecc-4723-9971-ad508a2b738b
📒 Files selected for processing (29)
control-plane/migrations/014_subscription_unique/migration.sqlcontrol-plane/src/__tests__/integration/billing-admin.test.tscontrol-plane/src/app.tscontrol-plane/src/domains/billing/billingService.tscontrol-plane/src/domains/billing/billingTransport.tscontrol-plane/src/domains/billing/subscriptionRepository.tscontrol-plane/src/domains/billing/tiers.tscontrol-plane/src/domains/billing/webhookDedup.tscontrol-plane/src/domains/bootstrap/bootstrapRepository.tscontrol-plane/src/domains/tenant/tenantRepository.tscontrol-plane/src/lib/platformStaff.test.tscontrol-plane/src/lib/platformStaff.tscontrol-plane/src/routes/admin.tscontrol-plane/src/routes/billing.tscontrol-plane/src/routes/tenants.tscontrol-plane/src/routes/webhookStripe.tscontrol-plane/src/testutil/testDb.tsdashboard/src/api/admin.tsdashboard/src/pages/AdminHomePage.tsxdashboard/src/pages/AgentBuilderHomePage.tsxdashboard/src/pages/BillingLink.tsxdashboard/src/pages/ProviderHomePage.tsxdashboard/src/pages/admin/BootstrapEditor.tsxdashboard/src/pages/admin/GraphHealth.tsxdashboard/src/pages/admin/IssuerVerificationQueue.tsxdashboard/src/pages/admin/TenantList.tsxdashboard/src/pages/admin/components.test.tsxdashboard/src/styles/index.cssdocs/superpowers/plans/HANDOVER.md
Comment on lines
+8
to
+9
| CREATE UNIQUE INDEX subscription_tenant_stripe_unique | ||
| ON subscriptions (tenant_id, stripe_subscription_id); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'CREATE TABLE subscriptions|stripe_subscription_id|INSERT INTO subscriptions|ON CONFLICT|getSubscriptionByTenant' \
control-planeRepository: Numeracode/verilink
Length of output: 24601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd /srv/storage/repo/VeriLink
printf '%s\n' '--- workspace ---'
git status --short
printf '%s\n' '--- branch ---'
git branch --show-current
printf '%s\n' '--- worktrees ---'
git worktree list
printf '%s\n' '--- commits ahead of origin/main ---'
git rev-list --count origin/main..HEAD
printf '%s\n' '--- stashes ---'
git stash list
printf '%s\n' '--- subscription files ---'
fd -t f . control-plane | rg 'subscription|billing|migration'
printf '%s\n' '--- relevant references ---'
rg -n -C 6 \
'CREATE TABLE subscriptions|stripe_subscription_id|INSERT INTO subscriptions|ON CONFLICT|getSubscriptionByTenant|status' \
control-plane/migrations control-plane/src/domains/billing control-plane/src/__tests__/integration/billing-admin.test.tsRepository: Numeracode/verilink
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- current repository ---'
pwd
printf '%s\n' '--- storage paths ---'
ls -ld /srv /srv/storage /srv/storage/repo /srv/storage/repo/VeriLink 2>&1 || true
printf '%s\n' '--- repository status ---'
git status --short
printf '%s\n' '--- branch ---'
git branch --show-current
printf '%s\n' '--- worktrees ---'
git worktree list
printf '%s\n' '--- commits ahead of origin/main ---'
git rev-list --count origin/main..HEAD
printf '%s\n' '--- stashes ---'
git stash listRepository: Numeracode/verilink
Length of output: 634
Enforce the documented subscription identities.
UNIQUE (tenant_id, stripe_subscription_id) permits the same Stripe subscription under multiple tenants and multiple subscription IDs for one tenant. getSubscriptionByTenant can hide these duplicates by returning only the newest row. Add a unique constraint on stripe_subscription_id. If history is retained, add a partial unique constraint for one active subscription per tenant. Update all subscription upserts to use the selected conflict target.
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 8-9: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
🤖 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 `@control-plane/migrations/014_subscription_unique/migration.sql` around lines
8 - 9, Update the subscription identity constraints around
subscription_tenant_stripe_unique: add uniqueness for stripe_subscription_id
globally, and if historical rows are retained, enforce one active subscription
per tenant with a partial unique constraint. Review every subscription upsert
and change its conflict target to match the selected constraint(s), preserving
the documented identity rules.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
cd /srv/storage/repo/VeriLink
printf '%s\n' '--- workspace ---'
git status --short --branch
printf '%s\n' '--- worktrees ---'
git worktree list
printf '%s\n' '--- branches ---'
git branch --show-current
git branch --format='%(refname:short)'
printf '%s\n' '--- ahead/behind origin/main ---'
git rev-list --left-right --count origin/main...HEAD
printf '%s\n' '--- stashes ---'
git stash list
printf '%s\n' '--- migration files ---'
git ls-files '*migration.sql' | tail -n 40
printf '%s\n' '--- target migration ---'
cat -n control-plane/migrations/014_subscription_unique/migration.sqlRepository: Numeracode/verilink
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- current directory ---'
pwd
printf '%s\n' '--- /srv ---'
if [ -d /srv ]; then find /srv -maxdepth 3 -type d -name .git -print; else echo '/srv is absent'; fi
printf '%s\n' '--- likely repository roots ---'
find /workspace /workspaces /repo /repos /root /tmp -maxdepth 4 -type d -name .git -print 2>/dev/null || true
printf '%s\n' '--- top-level entries ---'
find . -maxdepth 2 -type d -print | sort | head -n 80Repository: Numeracode/verilink
Length of output: 904
🏁 Script executed:
#!/bin/bash
set -eu
cd /home/jailuser/git
printf '%s\n' '--- workspace ---'
git status --short --branch
printf '%s\n' '--- worktrees ---'
git worktree list
printf '%s\n' '--- branches ---'
git branch --show-current
git branch --format='%(refname:short)'
printf '%s\n' '--- origin/main comparison ---'
if git show-ref --verify --quiet refs/remotes/origin/main; then
git rev-list --left-right --count origin/main...HEAD
else
echo 'origin/main is unavailable'
fi
printf '%s\n' '--- stashes ---'
git stash list
printf '%s\n' '--- migration inventory ---'
git ls-files 'control-plane/migrations/**' | sed -n '1,160p'
printf '%s\n' '--- target migration ---'
cat -n control-plane/migrations/014_subscription_unique/migration.sql
printf '%s\n' '--- migration configuration references ---'
rg -n --hidden -g '!vendor' -g '!node_modules' '(migrat|CREATE UNIQUE INDEX|CONCURRENTLY|subscriptions|stripe_subscription_id)' control-plane .github docs Makefile* 2>/dev/null | head -n 240Repository: Numeracode/verilink
Length of output: 31309
🏁 Script executed:
#!/bin/bash
set -eu
cd /home/jailuser/git
printf '%s\n' '--- migration runner ---'
cat -n control-plane/src/db/migrate.ts | sed -n '1,135p'
printf '%s\n' '--- subscriptions definition ---'
cat -n control-plane/migrations/006_audit/migration.sql | sed -n '1,75p'
printf '%s\n' '--- migration 013 index style ---'
cat -n control-plane/migrations/013_edge_nodes_api_key_unique/migration.sql | sed -n '75,110p'
printf '%s\n' '--- transaction and deployment references ---'
rg -n --hidden -g '!node_modules' -g '!vendor' '(BEGIN|COMMIT|ROLLBACK|transaction|migrate|migration|deploy|DATABASE_URL|webhook)' control-plane/src control-plane/package.json .github/workflows docs/superpowers/plans | head -n 260Repository: Numeracode/verilink
Length of output: 46408
🌐 Web query:
PostgreSQL CREATE INDEX documentation regular CREATE INDEX blocks writes ShareLock CREATE INDEX CONCURRENTLY cannot run inside a transaction block
💡 Result:
In PostgreSQL, the CREATE INDEX command exhibits different locking behaviors depending on whether the CONCURRENTLY option is used. Standard CREATE INDEX When executing a standard CREATE INDEX command, PostgreSQL locks the table against writes (INSERT, UPDATE, and DELETE operations) to ensure data consistency during the index build [1][2][3][4]. While the table remains accessible for read-only operations (SELECT), any concurrent attempt to modify the table's data will block until the index build is complete [1][2][3][4]. This process typically involves a single scan of the table and can hold the write lock for a significant duration, potentially causing performance issues in high-traffic production environments [1][2][3][4]. CREATE INDEX CONCURRENTLY The CONCURRENTLY option is designed to allow index creation without blocking writes to the table [1][2][3][4]. Instead of an exclusive write lock, it acquires a ShareUpdateExclusiveLock, which permits simultaneous SELECT, INSERT, UPDATE, and DELETE operations [1][5]. However, this method requires more total work, takes longer to complete, and must perform two scans of the table [1][2][3][4]. Constraint on Transaction Blocks A critical requirement for the CONCURRENTLY option is that it cannot be executed inside an explicit transaction block (e.g., within a BEGIN...COMMIT block) [5]. This restriction exists because a concurrently built index needs to wait for all existing transactions that could potentially modify the table to terminate before the build can finish [1][2][3][4]. If it were run inside a transaction, it would effectively be waiting for itself to finish, leading to a deadlock [5]. If the command is attempted inside a transaction, PostgreSQL will raise an error [5].
Citations:
- 1: https://www.postgresql.org/docs/current/sql-createindex.html
- 2: https://www.postgresql.org/docs/18/sql-createindex.html
- 3: https://www.postgresql.org/docs/17/sql-createindex.html
- 4: https://www.postgresql.org/docs/19/sql-createindex.html
- 5: https://alexstoica.com/blog/create-index-concurrently-locks
🏁 Script executed:
#!/bin/bash
set -eu
cd /home/jailuser/git
python3 - <<'PY'
from pathlib import Path
runner = Path("control-plane/src/db/migrate.ts").read_text()
migration = Path("control-plane/migrations/014_subscription_unique/migration.sql").read_text()
assert "await client.query('BEGIN');" in runner
assert "await client.query(migration.sql);" in runner
assert "await client.query('COMMIT');" in runner
assert "CREATE UNIQUE INDEX" in migration
print("migration_sql_is_executed_inside_runner_transaction: true")
print("concurrent_index_requires_runner_transaction_change: true")
print("regular_index_statement_present: true")
PY
printf '%s\n' '--- billing repository and webhook transaction context ---'
cat -n control-plane/src/domains/billing/subscriptionRepository.ts | sed -n '1,130p'
cat -n control-plane/src/routes/webhookStripe.ts | sed -n '1,230p'
printf '%s\n' '--- transaction helper ---'
cat -n control-plane/src/db/transaction.ts | sed -n '1,80p'Repository: Numeracode/verilink
Length of output: 4682
Use a non-transactional concurrent index build for production data.
If subscriptions can contain production data, run CREATE UNIQUE INDEX CONCURRENTLY outside an explicit transaction so webhook upserts are not blocked. The migration runner currently wraps every migration in BEGIN/COMMIT, so add a non-transactional path and record _verilink_migrations only after the index succeeds. Check for duplicate keys before building the index.
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 8-9: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
🤖 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 `@control-plane/migrations/014_subscription_unique/migration.sql` around lines
8 - 9, Update migration 014_subscription_unique and the migration runner to
support this migration as non-transactional: check subscriptions for duplicate
tenant_id/stripe_subscription_id pairs, build the unique index with CREATE
UNIQUE INDEX CONCURRENTLY outside BEGIN/COMMIT, and record _verilink_migrations
only after successful completion. Preserve transactional behavior for all other
migrations.
Source: Linters/SAST tools
Comment on lines
+24
to
+32
| const { tier, customer_email } = req.body as { tier?: string; customer_email?: string }; | ||
| if (!tier || typeof tier !== 'string') { | ||
| throw new AppError(CODES.BAD_REQUEST, 'tier is required'); | ||
| } | ||
| const { url } = await billingService.createCheckoutSession({ | ||
| tenantId, | ||
| tierId: tier, | ||
| customerEmail: customer_email, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate customer_email before forwarding it to Stripe.
The handler checks the type of tier but not customer_email. A non-string value reaches Stripe.checkout.sessions.create and produces an opaque upstream error. Reject non-string and malformed values in the route.
🛡️ Proposed validation
if (!tier || typeof tier !== 'string') {
throw new AppError(CODES.BAD_REQUEST, 'tier is required');
}
+ if (customer_email !== undefined && typeof customer_email !== 'string') {
+ throw new AppError(CODES.BAD_REQUEST, 'customer_email must be a string');
+ }🤖 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 `@control-plane/src/routes/billing.ts` around lines 24 - 32, Update the billing
route handler’s request-body validation alongside the existing tier check to
require customer_email to be a string with a valid email format before calling
billingService.createCheckoutSession. Reject missing, non-string, and malformed
values with the route’s established BAD_REQUEST AppError, while preserving the
existing checkout-session arguments for valid input.
Comment on lines
+39
to
+48
| router.post( | ||
| '/portal-session', | ||
| defineHandler({ | ||
| async handler(req, res) { | ||
| const tenantId = requireTenant(req); | ||
| const { url } = await billingService.createPortalSession({ tenantId }); | ||
| ok(res, { url }); | ||
| }, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require a billing scope for the portal and checkout endpoints.
Both routes accept any authenticated principal that has a tenantId. The auth middleware in control-plane/src/middleware/auth.ts accepts scoped API keys, and the integration test at control-plane/src/__tests__/integration/billing-admin.test.ts line 41 uses a key seeded with an empty scope list. A low-privilege integration key can therefore open the Stripe customer portal, where the holder can view invoices and cancel the subscription. Add a billing:write scope check.
🤖 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 `@control-plane/src/routes/billing.ts` around lines 39 - 48, Update the billing
portal and checkout route handlers to enforce the billing:write scope in
addition to requireTenant(req), using the existing scoped-auth authorization
mechanism from auth middleware. Apply the check consistently to both endpoints
before invoking billingService methods, while preserving their current success
responses.
Comment on lines
+23
to
+29
| try { | ||
| const result = await billingService.handleWebhookEvent(rawBody, signature); | ||
| ok(res, result); | ||
| } catch (err) { | ||
| const appErr = AppError.from(err); | ||
| return res.status(appErr.status).json(appErr.toResponse()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Log webhook failures before returning the error.
The catch block converts the error and returns it to Stripe. Nothing records the cause. Failed events then only appear as retries in the Stripe dashboard, with no local trace of the event id or the reason. Add a structured log line with the event type and error.
🤖 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 `@control-plane/src/routes/webhookStripe.ts` around lines 23 - 29, Update the
catch block around billingService.handleWebhookEvent to emit a structured error
log before returning the AppError response, including the webhook event type and
original error details; preserve the existing response conversion and status
handling.
Code Review by Qodo
1. Tenant keys get staff access
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+4
| process.env.DATABASE_URL ||= | ||
| 'postgresql://verilink:verilink@127.0.0.1:15432/verilink_test'; | ||
| process.env.API_KEY_HMAC_SECRET ||= 'test-hmac-secret-for-integration'; | ||
| process.env.STRIPE_PRICE_PRO ||= 'price_test_pro'; |
There was a problem hiding this comment.
1. Hardcoded db creds in test 📘 Rule violation ⛨ Security
The new integration test hardcodes a PostgreSQL connection string containing a username/password and also hardcodes API_KEY_HMAC_SECRET. This can be detected as credentials by gitleaks and risks leaking sensitive configuration patterns into the repo.
Agent Prompt
## Issue description
A new integration test hardcodes credentials/secrets (`DATABASE_URL` with an embedded password and `API_KEY_HMAC_SECRET`). This can violate secret-scanning policy and is risky to keep in source.
## Issue Context
The repo’s `.gitleaks.toml` does not appear to allowlist TypeScript test files, so hardcoded credentials in `*.test.ts` are more likely to be flagged.
## Fix Focus Areas
- control-plane/src/__tests__/integration/billing-admin.test.ts[1-4]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+8
to
+9
| CREATE UNIQUE INDEX subscription_tenant_stripe_unique | ||
| ON subscriptions (tenant_id, stripe_subscription_id); |
There was a problem hiding this comment.
2. Sql migration under control-plane 📘 Rule violation § Compliance
A new .sql migration file is added under control-plane/, which violates the component language restriction requiring TypeScript implementation files in control-plane. This weakens consistency of language boundaries for the component.
Agent Prompt
## Issue description
A new SQL migration was added under `control-plane/`, but the compliance rule restricts `control-plane` implementation language to TypeScript.
## Issue Context
If migrations are intended to be an exception, they should be moved to a clearly exempted location (or the component layout should be updated so migrations are not considered control-plane implementation files).
## Fix Focus Areas
- control-plane/migrations/014_subscription_unique/migration.sql[1-9]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+23
to
+25
| if (isPlatformStaff(user)) { | ||
| items = await tenantRepo.listTenants(); | ||
| } else if (user?.type === 'oidc' && user.userId) { |
There was a problem hiding this comment.
4. Tenant keys get staff access 🐞 Bug ⛨ Security
GET /v1/tenants treats admin:read API keys as platform staff and returns the full tenant list, and the new /v1/admin/* endpoints also use requireStaff (which allows admin:read API keys) while executing global unscoped queries. Because API keys are tenant-scoped and tenants can mint keys with admin:read, this enables cross-tenant admin data exposure (contradicting Plan 9’s “Admin — platform staff only”).
Agent Prompt
## Issue description
The new tenant/admin surfaces treat tenant-scoped API keys with `admin:read` as platform staff, allowing cross-tenant visibility (tenant enumeration and global admin queues/registry data).
## Issue Context
- `isPlatformStaff()` returns true for `type==='apikey'` with scope `admin:read`.
- `/v1/tenants` uses that to call `tenantRepo.listTenants()` (no tenant filter).
- `/v1/admin/bootstrap-issuers` and `/v1/admin/issuers/unverified` are behind `requireStaff`, which also grants access to `admin:read` API keys.
- `/v1/api-keys` allows creating keys with `admin:read`, and API keys are inherently tenant-bound (`authMiddleware` sets `tenantId`).
- Plan 9 decision doc describes Admin view as “platform staff only”.
## Fix Focus Areas
- control-plane/src/lib/platformStaff.ts[1-16]
- control-plane/src/routes/tenants.ts[12-36]
- control-plane/src/routes/admin.ts[11-14]
- control-plane/src/middleware/requireStaff.ts[5-18]
- control-plane/src/routes/apiKeys.ts[12-19]
### Implementation direction
Pick one consistent model and enforce it:
1) **Strict staff-only (recommended per Plan 9):**
- Introduce `requirePlatformStaff` that checks `req.user.isStaff === true` (OIDC platform_role), not API key scopes.
- Use it for `/v1/admin/*` and for the “all tenants” path in `/v1/tenants`.
- Keep `admin:read` API keys for *tenant-scoped* admin reads only (or remove the scope entirely if not needed).
2) **If `admin:read` API keys must remain staff-equivalent:**
- Prevent tenants from minting `admin:read` keys (gate it behind platform staff OIDC or a separate issuer process).
- Add explicit tests proving non-staff users cannot create staff-equivalent keys.
Also add an integration test that a non-staff API key cannot enumerate all tenants or access global admin endpoints.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+55
to
+59
| return this.client.webhooks.constructEvent( | ||
| rawBody, | ||
| signature, | ||
| config.stripe.webhookSecret || '' | ||
| ); |
There was a problem hiding this comment.
5. Unsigned webhook parsing fails 🐞 Bug ≡ Correctness
In non-production when STRIPE_WEBHOOK_SECRET is unset, handleWebhookEvent() allows missing signatures but still calls StripeBillingTransport.constructEvent() (selected whenever STRIPE_SECRET_KEY is set), which always attempts signature verification using an empty webhook secret. This makes webhook ingestion fail in dev/staging configurations that intend to accept unsigned events.
Agent Prompt
## Issue description
Non-production unsigned webhook acceptance is implemented in the handler, but the live Stripe transport still enforces signature verification with an empty secret, causing webhook ingestion to fail whenever `STRIPE_SECRET_KEY` is configured without `STRIPE_WEBHOOK_SECRET`.
## Issue Context
- `billingService.handleWebhookEvent()` permits missing signature when `config.stripe.webhookSecret` is empty and `nodeEnv !== 'production'`.
- `createBillingTransport()` selects `StripeBillingTransport` whenever `config.stripe.secretKey` is set.
- `StripeBillingTransport.constructEvent()` calls `stripe.webhooks.constructEvent(..., config.stripe.webhookSecret || '')`.
## Fix Focus Areas
- control-plane/src/domains/billing/billingService.ts[64-74]
- control-plane/src/domains/billing/billingTransport.ts[54-60]
- control-plane/src/domains/billing/billingTransport.ts[88-93]
### Implementation direction
- If `config.stripe.webhookSecret` is empty and non-prod, bypass Stripe signature verification and JSON-parse the event (similar to the stub transport), e.g.:
- in `handleWebhookEvent()`: `const event = config.stripe.webhookSecret ? billingTransport.constructEvent(...) : JSON.parse(rawBody.toString('utf8'))`
- or in `StripeBillingTransport.constructEvent()`: if secret missing, fall back to JSON parse.
- Keep the production refusal path unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+4
| process.env.DATABASE_URL ||= | ||
| 'postgresql://verilink:verilink@127.0.0.1:15432/verilink_test'; | ||
| process.env.API_KEY_HMAC_SECRET ||= 'test-hmac-secret-for-integration'; | ||
| process.env.STRIPE_PRICE_PRO ||= 'price_test_pro'; |
There was a problem hiding this comment.
6. Esm env defaults too late 🐞 Bug ☼ Reliability
The new integration test sets process.env.* before static imports, but in ESM those imports are evaluated before the module body runs, so transitive imports (notably config.ts) can freeze configuration using the pre-default environment. This can make the integration test flaky or environment-dependent.
Agent Prompt
## Issue description
In ESM, static imports are evaluated before this module’s top-level statements, so setting `process.env` at the top of the test file does not reliably happen before imported modules read and freeze configuration.
## Issue Context
`config.ts` reads from `process.env` and exports a frozen `config` object at module evaluation time. The integration test imports `startControlPlane`, `setupTestDb`, etc., which can transitively import `config.ts` before the env defaults are applied.
## Fix Focus Areas
- control-plane/src/__tests__/integration/billing-admin.test.ts[1-18]
- control-plane/src/config.ts[2-7]
- control-plane/src/config.ts[27-77]
### Implementation direction
- Move env defaulting into a separate preloaded setup file (preferred), or
- Replace static imports of modules that depend on config with dynamic imports performed after env is set, e.g. inside `before()`:
- `const { setupTestDb } = await import('../../testutil/testDb.js')`
- `const { startControlPlane } = await import('../../testutil/appHarness.js')`
- Mirror the pattern from the previously accepted fix for this exact ESM import-order issue.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
CP: - billingTransport: pin Stripe apiVersion; never silently stub in production (throw if STRIPE_SECRET_KEY unset in prod); pass metadata through subscription_data.metadata so customer.subscription.* events carry tenant_id. - billingService: default missing plan to "unknown" (reverse-lookup tier from priceId) instead of "pro"; require WEB_APP_URL (Stripe needs absolute URLs); checkout upsert preserves existing status/period on conflict (no clobber from a late checkout event); resolve tenant from stored sub id when update/delete events lack tenant_id; drop redundant try/catch around applyEvent. - subscriptionRepository: getSubscriptionByTenant prefers active rows; getSubscriptionByStripeId for metadata-free webhook resolution; upsert supports preserveOnConflict. - webhookDedup: reclaim events that failed before being marked processed. - webhookStripe: structured error log on handler failure. - billing route: validate customer_email format. - admin PATCH: 400 when neither current_weight nor de_emphasis_reason given. - app.ts: webhook mounted before apiLimiter/audit (exempt from rate limiting). - integration test: STRIPE_WEBHOOK_SECRET set + missing-signature 401 case; WEB_APP_URL configured; existing webhook tests send a signature header. Dashboard: - AgentBuilderHomePage: billing panel moved outside the selectedId gate. - BillingLink: role=status on the result message. - BootstrapEditor: "Save reason" button for reason-only patches (current-weight button stays enabled when the reason changed); +interaction test. Verification: CP tsc clean, 148 unit + 49 integration pass; dashboard typecheck clean, 54 vitest pass, vite build succeeds. Skipped (replied inline): migration CONCURRENTLY/partial-active-constraint (fresh table, transactional index is correct; per-tenant active constraint is out of scope/risky); stripe_webhook_events payload retention (ops policy, out of scope for this PR).
Collaborator
Author
|
@coderabbitai review Addressed the actionable findings in
Two findings intentionally skipped, with rationale:
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline --decorate 89411de
printf '\n=== changed files ===\n'
git diff-tree --no-commit-id --name-only -r 89411de
printf '\n=== source outlines ===\n'
for f in \
control-plane/src/domains/billing/billingTransport.ts \
control-plane/src/domains/billing/billingService.ts \
control-plane/src/domains/billing/subscriptionRepository.ts \
control-plane/src/domains/billing/webhookDedup.ts \
control-plane/src/routes/webhookStripe.ts \
control-plane/src/routes/billing.ts \
control-plane/src/routes/admin.ts \
control-plane/src/app.ts \
dashboard/src/pages/BillingLink.tsx \
dashboard/src/pages/admin/BootstrapEditor.tsx \
dashboard/src/pages/AgentBuilderHomePage.tsx \
control-plane/src/__tests__/integration/billing-admin.test.ts \
dashboard/src/pages/admin/components.test.tsx \
control-plane/migrations/014_subscription_unique/migration.sql
do
if [ -f "$f" ]; then
echo "=== $f ==="
ast-grep outline "$f" --items all 2>/dev/null || true
fi
doneLength of output: 11424 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_file() {
local f="$1"
echo "===== $f ====="
nl -ba "$f"
}
for f in \
control-plane/src/domains/billing/billingTransport.ts \
control-plane/src/domains/billing/billingService.ts \
control-plane/src/domains/billing/subscriptionRepository.ts \
control-plane/src/domains/billing/webhookDedup.ts \
control-plane/src/domains/billing/tiers.ts \
control-plane/src/routes/webhookStripe.ts \
control-plane/src/routes/billing.ts \
control-plane/src/routes/admin.ts \
control-plane/src/app.ts \
control-plane/src/__tests__/integration/billing-admin.test.ts \
dashboard/src/pages/AgentBuilderHomePage.tsx \
dashboard/src/pages/BillingLink.tsx \
dashboard/src/pages/admin/BootstrapEditor.tsx \
dashboard/src/pages/admin/components.test.tsx \
control-plane/migrations/014_subscription_unique/migration.sql
do
show_file "$f"
done
printf '\n===== subscriptions and webhook table definitions =====\n'
rg -n -i -C 8 \
'create table.*(subscriptions|stripe_webhook_events)|create.*index.*subscriptions|stripe_webhook_events|subscriptions\s*\(' \
control-plane/migrations
printf '\n===== middleware ordering and app route registration =====\n'
rg -n -C 4 \
'webhookStripeRouter|apiLimiter|auditMiddleware|express\.json|express\.raw' \
control-plane/src/app.ts
printf '\n===== subscription repository call sites =====\n'
rg -n -C 4 \
'\b(getSubscriptionByStripeId|getSubscriptionByTenant|upsertSubscription)\s*\(' \
control-plane/src --glob '*.ts'Length of output: 1926 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_file() {
local f="$1"
echo "===== $f ====="
awk '{ printf "%6d\t%s\n", NR, $0 }' "$f"
}
for f in \
control-plane/src/domains/billing/billingTransport.ts \
control-plane/src/domains/billing/billingService.ts \
control-plane/src/domains/billing/subscriptionRepository.ts \
control-plane/src/domains/billing/webhookDedup.ts \
control-plane/src/domains/billing/tiers.ts \
control-plane/src/routes/webhookStripe.ts \
control-plane/src/routes/billing.ts \
control-plane/src/routes/admin.ts \
control-plane/src/app.ts \
control-plane/src/__tests__/integration/billing-admin.test.ts \
dashboard/src/pages/AgentBuilderHomePage.tsx \
dashboard/src/pages/BillingLink.tsx \
dashboard/src/pages/admin/BootstrapEditor.tsx \
dashboard/src/pages/admin/components.test.tsx \
control-plane/migrations/014_subscription_unique/migration.sql
do
show_file "$f"
done
printf '\n===== subscriptions and webhook table definitions =====\n'
rg -n -i -C 8 \
'create table.*(subscriptions|stripe_webhook_events)|create.*index.*subscriptions|stripe_webhook_events|subscriptions\s*\(' \
control-plane/migrations
printf '\n===== middleware ordering and app route registration =====\n'
rg -n -C 4 \
'webhookStripeRouter|apiLimiter|auditMiddleware|express\.json|express\.raw' \
control-plane/src/app.ts
printf '\n===== subscription repository call sites =====\n'
rg -n -C 4 \
'\b(getSubscriptionByStripeId|getSubscriptionByTenant|upsertSubscription)\s*\(' \
control-plane/src --glob '*.ts'Length of output: 72500
One concurrency issue remains:
|
messagesgoel-blip
pushed a commit
that referenced
this pull request
Aug 7, 2026
The audit middleware writes async fire-and-forget on res.finish and can briefly hold row locks that TRUNCATE waits on; in slower CI this deadlocked resetTestData (reproduced on docs PR #30, flaky on PR D #29). Run the TRUNCATE under a short lock_timeout inside a transaction and retry a few times so an in-flight audit write can commit and release first. Also truncate audit_log explicitly. Test-only and bounded; verified with 3 clean full-suite runs.
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.
Plan 9 PR D — billing + admin (final Plan 9 slice)
@coderabbitai review
Implements the PR D slice of
docs/superpowers/plans/2026-08-01-plan-9-dashboard.md(PR A #26, PR B #27, PR C #28 merged). This completes Plan 9.Control plane — billing (fixed-tier Stripe, Decision 7)
POST /v1/billing/checkout-session(tenant-scoped, paid tier) andPOST /v1/billing/portal-session(404 without a subscription).POST /webhooks/stripemounted withexpress.rawbefore the json parser; signature verification mandatory whenSTRIPE_WEBHOOK_SECRETis set; unsigned webhooks refused in production config; idempotent dedup viastripe_webhook_events(event.id PK); handlescheckout.session.completed+customer.subscription.updated/deleted(upsertsubscriptionswithtenant_id/planmetadata carried through the checkout session).BillingTransportinterface (StripeBillingTransport+StubBillingTransport); no live Stripe in tests. Fixed tiers wired viaSTRIPE_PRICE_*env.Control plane — admin (platform staff, Decision 5)
GET /v1/tenants: staff (OIDCisStafforadmin:readAPI key) see all; OIDC members see their memberships; an API key sees its own tenant.GET/PATCH /v1/admin/bootstrap-issuers— Root.weight de-emphasis + reason +approved_by(issuers.trust_weightuntouched per design §4.5; full cold-start seed remains Plan 14).GET /v1/admin/issuers/unverified— issuer verification queue.isPlatformStaffhelper unifies OIDC staff +admin:readkeys (mirrorsrequireStaff).UNIQUE (tenant_id, stripe_subscription_id)for webhook idempotency (justified per Decision 12).Dashboard
/admin): graph health (reuses PR B's/v1/graph/summary), tenants, bootstrap registry editor (stepwise1.0→0.5→0.25→0Root.weight de-emphasis), issuer verification queue.Verification
tsc --noEmitclean; 148 unit + 48 integration tests pass.vite buildsucceeds.Note
Webhook integration tests post unsigned events in non-production (no
STRIPE_WEBHOOK_SECRET), which the handler accepts per Decision 7; the production refusal path is covered by thenodeEnv === 'production'guard. Plan 9 is complete after this PR; next is design §13 step 14 (bootstrap cold-start seed).Summary by CodeRabbit
New Features
Bug Fixes
Tests