feat(approvals): credentials are the company lead's job and deduped#45
Conversation
Credential requests to the board are now the company lead's responsibility. A reporting agent (one with a manager) gets a 403 pointing it to its CEO, on both create and resubmit, so subordinates route credential needs through the lead instead of going to the board directly. A duplicate pending credential request for the same env key is rejected with 409, as a server safety net on top of the lead owning dedup. The capability-request guide is now role-aware: reporting agents are told to route credential needs through their CEO, and the lead is told it owns acquisition and must reuse existing secrets / avoid duplicate requests.
|
Warning Review limit reached
Next review available in: 49 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCredential request creation and resubmission routes now enforce that only company leads (agents with Lead-aware credential request enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/routes/approvals.ts (1)
646-660: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRe-check pending envKey conflicts on resubmit.
A revision-requested credential can be resubmitted with an
envKeythat already has another pending request. This path only validates shape beforesvc.resubmit, so it can still create duplicate pending credential approvals.Proposed fix
- if (req.body.payload) { - requestCredentialSchema.parse(req.body.payload); - } + const credentialPayload = requestCredentialSchema.parse(req.body.payload ?? existing.payload); + const pending = await svc.list(existing.companyId, "pending"); + const duplicate = pending.find( + (a) => + a.id !== existing.id && + a.type === "request_credential" && + (a.payload as { envKey?: string } | null)?.envKey === credentialPayload.envKey, + ); + if (duplicate) { + res.status(409).json({ + error: `A pending credential request for ${credentialPayload.envKey} already exists (${duplicate.id}). Resolve it instead of resubmitting a duplicate.`, + }); + return; + }🤖 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 `@server/src/routes/approvals.ts` around lines 646 - 660, The resubmit path in approvals handling currently only validates the payload shape before calling svc.resubmit, so it can allow duplicate pending credential approvals for the same envKey. Add a re-check in the resubmission flow around normalizedPayload and approval resubmission logic to detect any existing pending request with the same envKey for the same company/credential and reject it before calling svc.resubmit. Reuse the same envKey conflict validation used elsewhere in this route or related approval helpers so the behavior stays consistent for hire_agent and other credential types.
🤖 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 `@server/src/routes/approvals.ts`:
- Around line 184-195: The duplicate credential check in approvals flow is
currently a non-atomic list-then-create race, so move the uniqueness enforcement
into the service/DB layer used by the approvals route (around svc.list and
svc.create). Add a transaction, row lock, or partial unique constraint keyed by
envKey for pending request_credential approvals, and have the create path
surface conflicts so the route can still return 409 for duplicates instead of
relying on the pre-check. Use the approvals route and the svc methods that
create pending credential requests as the main points to update.
- Around line 168-173: The lead-check in the approvals flow is using
body-controlled `requestedByAgentId`, which allows an agent actor to spoof a
lead and bypass the subordinate restriction. Update the `request_credential`
path in `approvals.ts` to use the authenticated agent from `req.actor.agentId`
for any agent-based lead/subordinate validation, and explicitly reject requests
when `approvalInput.requestedByAgentId` does not match the authenticated agent.
Keep the existing `agentsSvc.getById` lookup, but base the permission decision
on `req.actor` (agent vs board) rather than the request body, and ensure the
company access/actor permission check is enforced before allowing the action.
---
Outside diff comments:
In `@server/src/routes/approvals.ts`:
- Around line 646-660: The resubmit path in approvals handling currently only
validates the payload shape before calling svc.resubmit, so it can allow
duplicate pending credential approvals for the same envKey. Add a re-check in
the resubmission flow around normalizedPayload and approval resubmission logic
to detect any existing pending request with the same envKey for the same
company/credential and reject it before calling svc.resubmit. Reuse the same
envKey conflict validation used elsewhere in this route or related approval
helpers so the behavior stays consistent for hire_agent and other credential
types.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55cf0c09-ea2a-4f0d-a31e-475ed5ea106d
📒 Files selected for processing (5)
server/src/__tests__/approval-routes-idempotency.test.tsserver/src/__tests__/capability-requests.test.tsserver/src/routes/approvals.tsserver/src/services/capability-requests.tsserver/src/services/heartbeat.ts
| const pending = await svc.list(companyId, "pending"); | ||
| const duplicate = pending.find( | ||
| (a) => | ||
| a.type === "request_credential" && | ||
| (a.payload as { envKey?: string } | null)?.envKey === credPayload.envKey, | ||
| ); | ||
| if (duplicate) { | ||
| res.status(409).json({ | ||
| error: `A pending credential request for ${credPayload.envKey} already exists (${duplicate.id}). Resolve or resubmit it instead of creating a duplicate.`, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make duplicate credential detection atomic.
svc.list(...) followed later by svc.create(...) is a check-then-insert race; two lead requests can both observe no pending request and create duplicates for the same envKey. Move this into the service/DB layer with a transaction, lock, or partial unique constraint, then map conflicts to 409.
🤖 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 `@server/src/routes/approvals.ts` around lines 184 - 195, The duplicate
credential check in approvals flow is currently a non-atomic list-then-create
race, so move the uniqueness enforcement into the service/DB layer used by the
approvals route (around svc.list and svc.create). Add a transaction, row lock,
or partial unique constraint keyed by envKey for pending request_credential
approvals, and have the create path surface conflicts so the route can still
return 409 for duplicates instead of relying on the pre-check. Use the approvals
route and the svc methods that create pending credential requests as the main
points to update.
There was a problem hiding this comment.
Taking this one with caution and deferring. The duplicate check is an application-level safety net on top of the lead owning dedup; the race needs two near-simultaneous requests from the same lead for the same envKey, and the failure mode is benign (two pending requests, the board rejects one). A fully atomic guard would need a partial unique index on (company_id, payload->>'envKey') where type='request_credential' and status='pending', which we can add if duplicates are ever observed in practice.
There was a problem hiding this comment.
@antoinekm, that makes sense — thanks for the context. I agree this is a reasonable defer given the current risk profile and the benign failure mode.
If you want, I can open a small follow-up issue to track the DB-level hardening later, with the partial unique index approach you outlined.
✏️ Learnings added
Learnt from: antoinekm
Repo: antoinekm/atelier PR: 45
File: server/src/routes/approvals.ts:183-194
Timestamp: 2026-06-30T11:23:16.528Z
Learning: In `server/src/routes/approvals.ts`, duplicate detection for pending `request_credential` approvals is intentionally an application-level safety net for now rather than an atomic database constraint. The stronger future hardening, if duplicates are observed in practice, would be a partial unique index on `(company_id, payload->>'envKey')` where `type = 'request_credential'` and `status = 'pending'`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
requestedByAgentId is body-controlled, so a reporting agent could set it to a lead agent and bypass the subordinate block. Check req.actor.agentId (the authenticated agent) instead, and add a regression test for the forged-id case.
1 participant
What
Operating-model change for credential requests, enforced server-side.
1. Credentials are the company lead's job
A reporting agent (one with a
reportsTomanager) can no longer request a credential from the board directly.POST /companies/:id/approvalsandPOST /approvals/:id/resubmitreturn 403 for a subordinate, pointing it to its CEO. The lead (no manager) requests and provisions credentials for the company. This stops sub-agents (e.g. a CTO) from going straight to the human board.2. No duplicate pending requests
A new
request_credentialwhoseenvKeyalready has a pending request is rejected with 409 (pointing at the existing one). The lead owns dedup; this is the server safety net so duplicates cannot pile up (we just saw a CTO open two identical GITHUB_TOKEN requests).3. Role-aware capability guide
renderCapabilityRequestGuidenow takes{ isLead }. Reporting agents are told to route credential needs through their CEO; the lead is told it owns acquisition and must reuse existing secrets / avoid duplicate requests. Wired from heartbeat viaagent.reportsTo == null.Builds on the prior creation-time schema validation (#44).
Tests
Server suite green (20/20 across the two touched suites), typecheck clean.
Summary by CodeRabbit
New Features
Bug Fixes