mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
fix(secrets): delete a secret that a binding proposal referenced (#20193)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents can propose a secret binding. A person approves or rejects it on a review card. > - Each binding proposal row refers to one source: a secret or a pending secret proposal. A check constraint enforces this. > - The secret reference used `ON DELETE SET NULL`. When a person deletes the secret, Postgres sets the reference to null. The check constraint then rejects the row. > - So a person cannot delete any secret that a binding proposal ever referenced. The API returns 500. > - This pull request deletes the proposal rows together with the secret, as the other source reference already does. Before the delete, it rejects pending proposals so their cards close with a reason. > - The benefit is that secret deletion works again and no review card points at a removed proposal. ## Linked Issues or Issue Description Refs #15644 (related: block secret deletion while bindings still exist). **What happened** `DELETE /api/secrets/:id` returned 500 for a secret that a binding proposal referenced. The server log showed `new row for relation "company_secret_proposals" violates check constraint "company_secret_proposals_shape_check"`. This happened for pending, rejected and approved proposals. It also happened after the person removed the agent's binding. **Expected behavior** The person can delete the secret. Proposals that referenced it go with it. A pending review card closes as rejected and tells why. **Steps to reproduce** 1. An agent proposes a binding for an existing company secret (`POST /api/agents/me/secret-proposals` with `secretId`). 2. Approve or reject the proposal, or leave it pending. 3. Delete the secret with `DELETE /api/secrets/:id`. 4. The response is 500 and the secret stays. ## What Changed - `packages/db`: `company_secret_proposals.secret_id` now uses `ON DELETE CASCADE`, the same as `secret_proposal_id`. Migration `0452_secret_proposal_secret_cascade.sql` replaces the foreign key (`DROP CONSTRAINT IF EXISTS`, then `ADD CONSTRAINT`). - `server/src/services/secrets.ts`: `remove` accepts an optional `beforeDelete` hook. The hook runs after the secret is marked deleted and before its row is removed. - `server/src/services/secret-proposals.ts`: new `rejectPendingBindingsForSecret`. It rejects pending binding proposals for a secret with a reason and returns them. This closes their review cards and writes the normal activity entries. `createBinding` now holds the secret row `FOR SHARE` in its insert transaction and refuses a deleted secret. So no new pending proposal can start after the delete marks the secret deleted. - `server/src/routes/secrets.ts`: the delete route rejects pending proposals in `beforeDelete`. For each one it sends the normal rejection notice and wake (`notifySecretProposalResolution`), so the proposing agent continues. - `server/src/__tests__/secret-proposals-routes.test.ts`: a regression test deletes a secret with a pending, rejected and approved binding proposal through the route. It checks the secret and proposal are gone, the card status is correct, and only the pending case sends one rejection notice and one wake. A second test checks that a binding proposal for a secret that the delete already marked deleted fails with 404. ## Verification - Red first: the new test failed on `origin/master` with the shape-check violation for all three cases. - `cd server && vitest run --config vitest.config.ts src/__tests__/secret-proposals-routes.test.ts`: 47 passed. - `vitest run src/__tests__/chat-run-milestone-governed-work.test.ts src/__tests__/agent-secrets-routes.test.ts src/__tests__/secrets-service.test.ts`: 112 passed. - `tsc --noEmit` for `server` and `packages/db`: pass. `pnpm --filter @paperclipai/db run check:migrations`: pass. - Found by the `secrets.secret-proposal` chat evaluation. Its cleanup deletes the fixture secret and failed on this error in runs 42563093640 and 42571127911. ## Risks - The migration only replaces one foreign key. It does not rewrite rows. Existing rows already have a non-null `secret_id` or `secret_proposal_id`, so nothing else changes. - Deleting a secret now also deletes its binding proposal rows. The activity log keeps the proposal history (`secret.proposal.*` and `secret.deleted`). Secret-kind proposal rows that created the secret keep `created_secret_id ON DELETE SET NULL`, so their history stays. - Other paths that delete secrets (company deletion, user secret cleanup) now cascade instead of failing on the same constraint. They do not reject pending cards first. ## Model Used Anthropic Claude Opus 5.5 (`claude-opus-5-5`), used with tool use and code execution in Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
d7d0f96f3f
commit
d2fe85e57c
9 files changed
+428
-9
No files matched your search
@@ -0,0 +1,3 @@
|
|||||||
|
ALTER TABLE "company_secret_proposals" DROP CONSTRAINT IF EXISTS "company_secret_proposals_secret_id_company_secrets_id_fk";
|
||||||
|
--> statement-breakpoint
|
||||||
|
ALTER TABLE "company_secret_proposals" ADD CONSTRAINT "company_secret_proposals_secret_id_company_secrets_id_fk" FOREIGN KEY ("secret_id") REFERENCES "public"."company_secrets"("id") ON DELETE cascade ON UPDATE no action;
|
||||||
+287
-3
@@ -1,6 +1,6 @@
|
|||||||
{
|
{
|
||||||
"id": "76f06e29-53a8-4ecc-809a-d52b6bbb6520",
|
"id": "64e8be1e-e0cb-4cd8-85e0-0fdcce5eafde",
|
||||||
"prevId": "782688f1-7843-4a93-8ea3-2cec362d269e",
|
"prevId": "b5699366-77c2-48c8-b7dc-658a75de7658",
|
||||||
"version": "7",
|
"version": "7",
|
||||||
"dialect": "postgresql",
|
"dialect": "postgresql",
|
||||||
"tables": {
|
"tables": {
|
||||||
@@ -4102,6 +4102,28 @@
|
|||||||
"method": "btree",
|
"method": "btree",
|
||||||
"with": {}
|
"with": {}
|
||||||
},
|
},
|
||||||
|
"agent_wakeup_requests_issue_blocker_stalled_uq": {
|
||||||
|
"name": "agent_wakeup_requests_issue_blocker_stalled_uq",
|
||||||
|
"columns": [
|
||||||
|
{
|
||||||
|
"expression": "company_id",
|
||||||
|
"isExpression": false,
|
||||||
|
"asc": true,
|
||||||
|
"nulls": "last"
|
||||||
|
},
|
||||||
|
{
|
||||||
|
"expression": "idempotency_key",
|
||||||
|
"isExpression": false,
|
||||||
|
"asc": true,
|
||||||
|
"nulls": "last"
|
||||||
|
}
|
||||||
|
],
|
||||||
|
"isUnique": true,
|
||||||
|
"where": "\"agent_wakeup_requests\".\"idempotency_key\" LIKE 'issue_blocker_stalled:%' AND \"agent_wakeup_requests\".\"status\" IN ('queued', 'deferred_issue_execution', 'claimed', 'coalesced', 'completed')",
|
||||||
|
"concurrently": true,
|
||||||
|
"method": "btree",
|
||||||
|
"with": {}
|
||||||
|
},
|
||||||
"agent_wakeup_requests_company_payload_issue_idx": {
|
"agent_wakeup_requests_company_payload_issue_idx": {
|
||||||
"name": "agent_wakeup_requests_company_payload_issue_idx",
|
"name": "agent_wakeup_requests_company_payload_issue_idx",
|
||||||
"columns": [
|
"columns": [
|
||||||
@@ -4252,6 +4274,18 @@
|
|||||||
"primaryKey": false,
|
"primaryKey": false,
|
||||||
"notNull": false
|
"notNull": false
|
||||||
},
|
},
|
||||||
|
"harness_verified_digest": {
|
||||||
|
"name": "harness_verified_digest",
|
||||||
|
"type": "text",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": false
|
||||||
|
},
|
||||||
|
"harness_verified_at": {
|
||||||
|
"name": "harness_verified_at",
|
||||||
|
"type": "timestamp with time zone",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": false
|
||||||
|
},
|
||||||
"reports_to": {
|
"reports_to": {
|
||||||
"name": "reports_to",
|
"name": "reports_to",
|
||||||
"type": "uuid",
|
"type": "uuid",
|
||||||
@@ -7848,6 +7882,150 @@
|
|||||||
},
|
},
|
||||||
"isRLSEnabled": false
|
"isRLSEnabled": false
|
||||||
},
|
},
|
||||||
|
"public.budget_resume_claims": {
|
||||||
|
"name": "budget_resume_claims",
|
||||||
|
"schema": "",
|
||||||
|
"columns": {
|
||||||
|
"run_id": {
|
||||||
|
"name": "run_id",
|
||||||
|
"type": "uuid",
|
||||||
|
"primaryKey": true,
|
||||||
|
"notNull": true
|
||||||
|
},
|
||||||
|
"company_id": {
|
||||||
|
"name": "company_id",
|
||||||
|
"type": "uuid",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": true
|
||||||
|
},
|
||||||
|
"incident_id": {
|
||||||
|
"name": "incident_id",
|
||||||
|
"type": "uuid",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": true
|
||||||
|
},
|
||||||
|
"actor_user_id": {
|
||||||
|
"name": "actor_user_id",
|
||||||
|
"type": "text",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": true
|
||||||
|
},
|
||||||
|
"actor_source": {
|
||||||
|
"name": "actor_source",
|
||||||
|
"type": "text",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": false
|
||||||
|
},
|
||||||
|
"state": {
|
||||||
|
"name": "state",
|
||||||
|
"type": "text",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": true,
|
||||||
|
"default": "'pending'"
|
||||||
|
},
|
||||||
|
"claimed_at": {
|
||||||
|
"name": "claimed_at",
|
||||||
|
"type": "timestamp with time zone",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": false
|
||||||
|
},
|
||||||
|
"settled_at": {
|
||||||
|
"name": "settled_at",
|
||||||
|
"type": "timestamp with time zone",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": false
|
||||||
|
},
|
||||||
|
"reason": {
|
||||||
|
"name": "reason",
|
||||||
|
"type": "text",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": false
|
||||||
|
},
|
||||||
|
"request_id": {
|
||||||
|
"name": "request_id",
|
||||||
|
"type": "uuid",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": false
|
||||||
|
},
|
||||||
|
"created_at": {
|
||||||
|
"name": "created_at",
|
||||||
|
"type": "timestamp with time zone",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": true,
|
||||||
|
"default": "now()"
|
||||||
|
}
|
||||||
|
},
|
||||||
|
"indexes": {
|
||||||
|
"budget_resume_claims_pending_idx": {
|
||||||
|
"name": "budget_resume_claims_pending_idx",
|
||||||
|
"columns": [
|
||||||
|
{
|
||||||
|
"expression": "company_id",
|
||||||
|
"isExpression": false,
|
||||||
|
"asc": true,
|
||||||
|
"nulls": "last"
|
||||||
|
},
|
||||||
|
{
|
||||||
|
"expression": "run_id",
|
||||||
|
"isExpression": false,
|
||||||
|
"asc": true,
|
||||||
|
"nulls": "last"
|
||||||
|
}
|
||||||
|
],
|
||||||
|
"isUnique": false,
|
||||||
|
"where": "\"budget_resume_claims\".\"state\" = 'pending'",
|
||||||
|
"concurrently": false,
|
||||||
|
"method": "btree",
|
||||||
|
"with": {}
|
||||||
|
}
|
||||||
|
},
|
||||||
|
"foreignKeys": {
|
||||||
|
"budget_resume_claims_run_id_heartbeat_runs_id_fk": {
|
||||||
|
"name": "budget_resume_claims_run_id_heartbeat_runs_id_fk",
|
||||||
|
"tableFrom": "budget_resume_claims",
|
||||||
|
"tableTo": "heartbeat_runs",
|
||||||
|
"columnsFrom": [
|
||||||
|
"run_id"
|
||||||
|
],
|
||||||
|
"columnsTo": [
|
||||||
|
"id"
|
||||||
|
],
|
||||||
|
"onDelete": "cascade",
|
||||||
|
"onUpdate": "no action"
|
||||||
|
},
|
||||||
|
"budget_resume_claims_company_id_companies_id_fk": {
|
||||||
|
"name": "budget_resume_claims_company_id_companies_id_fk",
|
||||||
|
"tableFrom": "budget_resume_claims",
|
||||||
|
"tableTo": "companies",
|
||||||
|
"columnsFrom": [
|
||||||
|
"company_id"
|
||||||
|
],
|
||||||
|
"columnsTo": [
|
||||||
|
"id"
|
||||||
|
],
|
||||||
|
"onDelete": "cascade",
|
||||||
|
"onUpdate": "no action"
|
||||||
|
},
|
||||||
|
"budget_resume_claims_incident_id_budget_incidents_id_fk": {
|
||||||
|
"name": "budget_resume_claims_incident_id_budget_incidents_id_fk",
|
||||||
|
"tableFrom": "budget_resume_claims",
|
||||||
|
"tableTo": "budget_incidents",
|
||||||
|
"columnsFrom": [
|
||||||
|
"incident_id"
|
||||||
|
],
|
||||||
|
"columnsTo": [
|
||||||
|
"id"
|
||||||
|
],
|
||||||
|
"onDelete": "cascade",
|
||||||
|
"onUpdate": "no action"
|
||||||
|
}
|
||||||
|
},
|
||||||
|
"compositePrimaryKeys": {},
|
||||||
|
"uniqueConstraints": {},
|
||||||
|
"policies": {},
|
||||||
|
"checkConstraints": {},
|
||||||
|
"isRLSEnabled": false
|
||||||
|
},
|
||||||
"public.built_in_managed_resources": {
|
"public.built_in_managed_resources": {
|
||||||
"name": "built_in_managed_resources",
|
"name": "built_in_managed_resources",
|
||||||
"schema": "",
|
"schema": "",
|
||||||
@@ -17221,7 +17399,7 @@
|
|||||||
"columnsTo": [
|
"columnsTo": [
|
||||||
"id"
|
"id"
|
||||||
],
|
],
|
||||||
"onDelete": "set null",
|
"onDelete": "cascade",
|
||||||
"onUpdate": "no action"
|
"onUpdate": "no action"
|
||||||
},
|
},
|
||||||
"company_secret_proposals_secret_proposal_id_company_secret_proposals_id_fk": {
|
"company_secret_proposals_secret_proposal_id_company_secret_proposals_id_fk": {
|
||||||
@@ -21965,6 +22143,112 @@
|
|||||||
"checkConstraints": {},
|
"checkConstraints": {},
|
||||||
"isRLSEnabled": false
|
"isRLSEnabled": false
|
||||||
},
|
},
|
||||||
|
"public.controller_instances": {
|
||||||
|
"name": "controller_instances",
|
||||||
|
"schema": "",
|
||||||
|
"columns": {
|
||||||
|
"boot_id": {
|
||||||
|
"name": "boot_id",
|
||||||
|
"type": "text",
|
||||||
|
"primaryKey": true,
|
||||||
|
"notNull": true
|
||||||
|
},
|
||||||
|
"epoch": {
|
||||||
|
"name": "epoch",
|
||||||
|
"type": "bigserial",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": true
|
||||||
|
},
|
||||||
|
"process_space": {
|
||||||
|
"name": "process_space",
|
||||||
|
"type": "text",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": true
|
||||||
|
},
|
||||||
|
"pid": {
|
||||||
|
"name": "pid",
|
||||||
|
"type": "integer",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": true
|
||||||
|
},
|
||||||
|
"started_at": {
|
||||||
|
"name": "started_at",
|
||||||
|
"type": "timestamp with time zone",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": true,
|
||||||
|
"default": "now()"
|
||||||
|
},
|
||||||
|
"renewed_at": {
|
||||||
|
"name": "renewed_at",
|
||||||
|
"type": "timestamp with time zone",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": true,
|
||||||
|
"default": "now()"
|
||||||
|
},
|
||||||
|
"lease_expires_at": {
|
||||||
|
"name": "lease_expires_at",
|
||||||
|
"type": "timestamp with time zone",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": true
|
||||||
|
},
|
||||||
|
"fenced_at": {
|
||||||
|
"name": "fenced_at",
|
||||||
|
"type": "timestamp with time zone",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": false
|
||||||
|
},
|
||||||
|
"fenced_by_epoch": {
|
||||||
|
"name": "fenced_by_epoch",
|
||||||
|
"type": "bigint",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": false
|
||||||
|
},
|
||||||
|
"stopped_at": {
|
||||||
|
"name": "stopped_at",
|
||||||
|
"type": "timestamp with time zone",
|
||||||
|
"primaryKey": false,
|
||||||
|
"notNull": false
|
||||||
|
}
|
||||||
|
},
|
||||||
|
"indexes": {
|
||||||
|
"controller_instances_epoch_idx": {
|
||||||
|
"name": "controller_instances_epoch_idx",
|
||||||
|
"columns": [
|
||||||
|
{
|
||||||
|
"expression": "epoch",
|
||||||
|
"isExpression": false,
|
||||||
|
"asc": true,
|
||||||
|
"nulls": "last"
|
||||||
|
}
|
||||||
|
],
|
||||||
|
"isUnique": true,
|
||||||
|
"concurrently": false,
|
||||||
|
"method": "btree",
|
||||||
|
"with": {}
|
||||||
|
},
|
||||||
|
"controller_instances_lease_idx": {
|
||||||
|
"name": "controller_instances_lease_idx",
|
||||||
|
"columns": [
|
||||||
|
{
|
||||||
|
"expression": "lease_expires_at",
|
||||||
|
"isExpression": false,
|
||||||
|
"asc": true,
|
||||||
|
"nulls": "last"
|
||||||
|
}
|
||||||
|
],
|
||||||
|
"isUnique": false,
|
||||||
|
"concurrently": false,
|
||||||
|
"method": "btree",
|
||||||
|
"with": {}
|
||||||
|
}
|
||||||
|
},
|
||||||
|
"foreignKeys": {},
|
||||||
|
"compositePrimaryKeys": {},
|
||||||
|
"uniqueConstraints": {},
|
||||||
|
"policies": {},
|
||||||
|
"checkConstraints": {},
|
||||||
|
"isRLSEnabled": false
|
||||||
|
},
|
||||||
"public.cost_events": {
|
"public.cost_events": {
|
||||||
"name": "cost_events",
|
"name": "cost_events",
|
||||||
"schema": "",
|
"schema": "",
|
||||||
+7
@@ -3151,6 +3151,13 @@
|
|||||||
"when": 1794185816322,
|
"when": 1794185816322,
|
||||||
"tag": "0451_budget_resume_claims",
|
"tag": "0451_budget_resume_claims",
|
||||||
"breakpoints": true
|
"breakpoints": true
|
||||||
|
},
|
||||||
|
{
|
||||||
|
"idx": 452,
|
||||||
|
"version": "7",
|
||||||
|
"when": 1794205579544,
|
||||||
|
"tag": "0452_secret_proposal_secret_cascade",
|
||||||
|
"breakpoints": true
|
||||||
}
|
}
|
||||||
]
|
]
|
||||||
}
|
}
|
||||||
@@ -21,7 +21,9 @@ export const companySecretProposals = pgTable(
|
|||||||
valueCiphertext: jsonb("value_ciphertext").$type<Record<string, unknown> | null>(),
|
valueCiphertext: jsonb("value_ciphertext").$type<Record<string, unknown> | null>(),
|
||||||
valueFingerprintSha256: text("value_fingerprint_sha256"),
|
valueFingerprintSha256: text("value_fingerprint_sha256"),
|
||||||
valueLength: integer("value_length"),
|
valueLength: integer("value_length"),
|
||||||
secretId: uuid("secret_id").references(() => companySecrets.id, { onDelete: "set null" }),
|
// Binding proposals require exactly one source reference (shape check), so
|
||||||
|
// they go with the secret they reference, like secretProposalId below.
|
||||||
|
secretId: uuid("secret_id").references(() => companySecrets.id, { onDelete: "cascade" }),
|
||||||
secretProposalId: uuid("secret_proposal_id").references((): AnyPgColumn => companySecretProposals.id, { onDelete: "cascade" }),
|
secretProposalId: uuid("secret_proposal_id").references((): AnyPgColumn => companySecretProposals.id, { onDelete: "cascade" }),
|
||||||
targetType: text("target_type"),
|
targetType: text("target_type"),
|
||||||
targetId: uuid("target_id").references(() => agents.id, { onDelete: "cascade" }),
|
targetId: uuid("target_id").references(() => agents.id, { onDelete: "cascade" }),
|
||||||
|
|||||||
@@ -865,6 +865,73 @@ describeEmbeddedPostgres("secret proposal routes", () => {
|
|||||||
]);
|
]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it.each(["rejected", "approved", "pending"] as const)("deletes a secret whose %s binding proposal referenced it", async (outcome) => {
|
||||||
|
const fixture = await seedRun();
|
||||||
|
const secret = await secretService(db).create(fixture.companyId, {
|
||||||
|
name: "dev/deploy/publishing",
|
||||||
|
key: "DEPLOY_PUBLISHING",
|
||||||
|
provider: "local_encrypted",
|
||||||
|
value: "publishing-secret",
|
||||||
|
});
|
||||||
|
const proposals = createSecretProposalsService(db);
|
||||||
|
const proposal = await proposals.createBinding({ companyId: fixture.companyId, heartbeatRunId: fixture.heartbeatRunId }, {
|
||||||
|
secretId: secret.id,
|
||||||
|
configPath: "env.PUBLISHING_API_KEY",
|
||||||
|
justification: "Publish the site",
|
||||||
|
bindingTargetPolicy: "self_and_reports",
|
||||||
|
});
|
||||||
|
if (outcome === "rejected") await proposals.transition(fixture.companyId, proposal.id, "rejected", { resolvedByUserId: "board-user" });
|
||||||
|
if (outcome === "approved") {
|
||||||
|
await proposals.approve(fixture.companyId, proposal.id, { resolvedByUserId: "board-user" });
|
||||||
|
await db.delete(companySecretBindings).where(eq(companySecretBindings.secretId, secret.id));
|
||||||
|
}
|
||||||
|
|
||||||
|
// The proposal's secret reference used ON DELETE SET NULL, which the binding
|
||||||
|
// shape check rejects, so deleting the secret failed with a 500.
|
||||||
|
await db.update(issues).set({ assigneeAgentId: fixture.agentId }).where(eq(issues.id, fixture.issueId));
|
||||||
|
const wakeup = vi.fn().mockResolvedValue(null);
|
||||||
|
const persistedIssues = issueService(db);
|
||||||
|
const addComment = vi.fn().mockResolvedValue(undefined);
|
||||||
|
const deleted = await request(createBoardApp(fixture, {
|
||||||
|
heartbeat: { wakeup },
|
||||||
|
issues: { getById: persistedIssues.getById, addComment },
|
||||||
|
})).delete(`/api/secrets/${secret.id}`);
|
||||||
|
expect(deleted.status).toBe(200);
|
||||||
|
// Only a pending proposal is rejected by the delete; it gets the normal rejection signal.
|
||||||
|
expect(addComment).toHaveBeenCalledTimes(outcome === "pending" ? 1 : 0);
|
||||||
|
if (outcome === "pending") {
|
||||||
|
expect(addComment.mock.calls[0]?.[1]).toContain("Status: **rejected**");
|
||||||
|
expect(addComment.mock.calls[0]?.[1]).toContain("The referenced secret was deleted.");
|
||||||
|
}
|
||||||
|
expect(wakeup).toHaveBeenCalledTimes(outcome === "pending" ? 1 : 0);
|
||||||
|
expect(await db.select().from(companySecrets).where(eq(companySecrets.id, secret.id))).toEqual([]);
|
||||||
|
expect(await db.select().from(companySecretProposals).where(eq(companySecretProposals.id, proposal.id))).toEqual([]);
|
||||||
|
// A pending review card closes as rejected instead of pointing at a deleted proposal.
|
||||||
|
const [card] = await db.select().from(issueThreadInteractions).where(eq(issueThreadInteractions.id, proposal.interactionId!));
|
||||||
|
expect(card?.status).toBe(outcome === "approved" ? "accepted" : "rejected");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("refuses a new binding proposal for a secret that a delete already marked deleted", async () => {
|
||||||
|
const fixture = await seedRun();
|
||||||
|
const secret = await secretService(db).create(fixture.companyId, {
|
||||||
|
name: "dev/deploy/deleting",
|
||||||
|
key: "DEPLOY_DELETING",
|
||||||
|
provider: "local_encrypted",
|
||||||
|
value: "deleting-secret",
|
||||||
|
});
|
||||||
|
let proposalDuringDelete: unknown;
|
||||||
|
await secretService(db).remove(secret.id, {
|
||||||
|
beforeDelete: async () => {
|
||||||
|
proposalDuringDelete = await createSecretProposalsService(db).createBinding(
|
||||||
|
{ companyId: fixture.companyId, heartbeatRunId: fixture.heartbeatRunId },
|
||||||
|
{ secretId: secret.id, configPath: "env.LATE_KEY", justification: "Late request", bindingTargetPolicy: "self_and_reports" },
|
||||||
|
).catch((error: unknown) => error);
|
||||||
|
},
|
||||||
|
});
|
||||||
|
expect(proposalDuringDelete).toMatchObject({ status: 404 });
|
||||||
|
expect(await db.select().from(companySecretProposals)).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
it("proposes and approves a binding by resolving the proposer's own sourceConfigPath", async () => {
|
it("proposes and approves a binding by resolving the proposer's own sourceConfigPath", async () => {
|
||||||
const fixture = await seedRun();
|
const fixture = await seedRun();
|
||||||
const sourceSecret = await secretService(db).create(fixture.companyId, {
|
const sourceSecret = await secretService(db).create(fixture.companyId, {
|
||||||
|
|||||||
@@ -1039,6 +1039,7 @@ describe("secret routes", () => {
|
|||||||
expect(res.body).toEqual({ ok: true });
|
expect(res.body).toEqual({ ok: true });
|
||||||
expect(mockSecretService.remove).toHaveBeenCalledWith(
|
expect(mockSecretService.remove).toHaveBeenCalledWith(
|
||||||
"33333333-3333-4333-8333-333333333333",
|
"33333333-3333-4333-8333-333333333333",
|
||||||
|
{ beforeDelete: expect.any(Function) },
|
||||||
);
|
);
|
||||||
expect(mockLogActivity).toHaveBeenCalledWith(
|
expect(mockLogActivity).toHaveBeenCalledWith(
|
||||||
expect.anything(),
|
expect.anything(),
|
||||||
|
|||||||
@@ -1115,7 +1115,17 @@ export function secretRoutes(db: Db, deps: SecretRoutesDeps = {}) {
|
|||||||
);
|
);
|
||||||
if (!existing) return;
|
if (!existing) return;
|
||||||
|
|
||||||
const removed = await svc.remove(id);
|
const resolvedByUserId = req.actor.userId ?? "board";
|
||||||
|
const reason = "The referenced secret was deleted.";
|
||||||
|
const removed = await svc.remove(id, {
|
||||||
|
beforeDelete: async (secret) => {
|
||||||
|
const rejected = await proposals.rejectPendingBindingsForSecret(secret.companyId, secret.id, { resolvedByUserId, reason });
|
||||||
|
// Same signal as an ordinary rejection, so the proposing agent continues.
|
||||||
|
for (const proposal of rejected) {
|
||||||
|
await notifySecretProposalResolution({ proposal, status: "rejected", userId: resolvedByUserId, reason, issues, heartbeat });
|
||||||
|
}
|
||||||
|
},
|
||||||
|
});
|
||||||
if (!removed) {
|
if (!removed) {
|
||||||
res.status(404).json({ error: "Secret not found" });
|
res.status(404).json({ error: "Secret not found" });
|
||||||
return;
|
return;
|
||||||
|
|||||||
@@ -446,6 +446,13 @@ export function createSecretProposalsService(db: Db) {
|
|||||||
return createWithinQuota(
|
return createWithinQuota(
|
||||||
{ companyId: context.companyId, agentId: run.agentId, runId: run.id, issueId: originIssueId },
|
{ companyId: context.companyId, agentId: run.agentId, runId: run.id, issueId: originIssueId },
|
||||||
async (txDb) => {
|
async (txDb) => {
|
||||||
|
if (resolvedSecretId) {
|
||||||
|
// Hold the secret against a concurrent delete: the delete marks it
|
||||||
|
// deleted first, then settles the proposals that reference it.
|
||||||
|
const live = await txDb.select({ status: companySecrets.status }).from(companySecrets)
|
||||||
|
.where(eq(companySecrets.id, resolvedSecretId)).for("share").then((rows) => rows[0] ?? null);
|
||||||
|
if (!live || live.status === "deleted") throw notFound("Secret not found");
|
||||||
|
}
|
||||||
if (input.secretProposalId) {
|
if (input.secretProposalId) {
|
||||||
const dependency = await txDb.select().from(companySecretProposals).where(and(
|
const dependency = await txDb.select().from(companySecretProposals).where(and(
|
||||||
eq(companySecretProposals.id, input.secretProposalId),
|
eq(companySecretProposals.id, input.secretProposalId),
|
||||||
@@ -915,5 +922,35 @@ export function createSecretProposalsService(db: Db) {
|
|||||||
return row?.at == null ? null : new Date(row.at).getTime();
|
return row?.at == null ? null : new Date(row.at).getTime();
|
||||||
}
|
}
|
||||||
|
|
||||||
return { getById, view: enrich, createSecret, createBinding, listForAgent, listForBoard, assertBindingSnapshotCurrent, approve, transition, sweepExpired, sweepExpiredForCompany, expiryDeadlines, nextExpiryAt };
|
/**
|
||||||
|
* Deleting a secret also deletes the binding proposals that reference it.
|
||||||
|
* Settle pending ones first, so their review cards close as rejected with
|
||||||
|
* the reason instead of pointing at a proposal that no longer exists. Call
|
||||||
|
* it after the secret is marked deleted: createBinding holds the secret row
|
||||||
|
* and refuses a deleted one, so no new pending proposal can follow.
|
||||||
|
*/
|
||||||
|
async function rejectPendingBindingsForSecret(companyId: string, secretId: string, input: { resolvedByUserId?: string | null; reason: string }) {
|
||||||
|
const pending = await db.select().from(companySecretProposals).where(and(
|
||||||
|
eq(companySecretProposals.companyId, companyId),
|
||||||
|
eq(companySecretProposals.secretId, secretId),
|
||||||
|
eq(companySecretProposals.kind, "binding"),
|
||||||
|
eq(companySecretProposals.status, "pending"),
|
||||||
|
));
|
||||||
|
const rejected: Proposal[] = [];
|
||||||
|
for (const row of pending) {
|
||||||
|
try {
|
||||||
|
await transition(companyId, row.id, "rejected", {
|
||||||
|
resolvedByUserId: input.resolvedByUserId ?? null,
|
||||||
|
reason: input.reason,
|
||||||
|
});
|
||||||
|
rejected.push(row);
|
||||||
|
} catch (error) {
|
||||||
|
// Resolved concurrently; the secret deletion still removes the row.
|
||||||
|
if (!(error instanceof HttpError) || error.status !== 409) throw error;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return rejected;
|
||||||
|
}
|
||||||
|
|
||||||
|
return { getById, view: enrich, createSecret, createBinding, listForAgent, listForBoard, assertBindingSnapshotCurrent, approve, transition, rejectPendingBindingsForSecret, sweepExpired, sweepExpiredForCompany, expiryDeadlines, nextExpiryAt };
|
||||||
}
|
}
|
||||||
@@ -970,6 +970,11 @@ function assertSelectableProviderConfig(config: {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
export type RemoveSecretOptions = {
|
||||||
|
/** Runs after the secret is marked deleted and before its row is removed. */
|
||||||
|
beforeDelete?: (secret: typeof companySecrets.$inferSelect) => Promise<void>;
|
||||||
|
};
|
||||||
|
|
||||||
export function secretService(db: Db | DbTransaction) {
|
export function secretService(db: Db | DbTransaction) {
|
||||||
const authorization = authorizationService(db);
|
const authorization = authorizationService(db);
|
||||||
|
|
||||||
@@ -3401,18 +3406,18 @@ export function secretService(db: Db | DbTransaction) {
|
|||||||
// `removeSecretUnlocked` re-reads the secret under the lock, so a delete
|
// `removeSecretUnlocked` re-reads the secret under the lock, so a delete
|
||||||
// that lost a race between the pre-check and the lock still sees current
|
// that lost a race between the pre-check and the lock still sees current
|
||||||
// state, not the pre-check's stale read.
|
// state, not the pre-check's stale read.
|
||||||
async function removeSecretInternal(secretId: string) {
|
async function removeSecretInternal(secretId: string, options: RemoveSecretOptions = {}) {
|
||||||
const preCheckSecret = await getById(secretId);
|
const preCheckSecret = await getById(secretId);
|
||||||
if (!preCheckSecret) return null;
|
if (!preCheckSecret) return null;
|
||||||
return withAccountHomeSecretMutationLock(undefined, preCheckSecret.companyId, () =>
|
return withAccountHomeSecretMutationLock(undefined, preCheckSecret.companyId, () =>
|
||||||
removeSecretUnlocked(secretId),
|
removeSecretUnlocked(secretId, options),
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
// The body of `removeSecretInternal` above, unchanged. Extracted to a named
|
// The body of `removeSecretInternal` above, unchanged. Extracted to a named
|
||||||
// function so that wrapper can hold the lock across this whole delete
|
// function so that wrapper can hold the lock across this whole delete
|
||||||
// sequence without duplicating it.
|
// sequence without duplicating it.
|
||||||
async function removeSecretUnlocked(secretId: string) {
|
async function removeSecretUnlocked(secretId: string, options: RemoveSecretOptions = {}) {
|
||||||
const secret = await getById(secretId);
|
const secret = await getById(secretId);
|
||||||
if (!secret) return null;
|
if (!secret) return null;
|
||||||
const versionRow = await getSecretVersion(secret.id, secret.latestVersion);
|
const versionRow = await getSecretVersion(secret.id, secret.latestVersion);
|
||||||
@@ -3430,6 +3435,9 @@ export function secretService(db: Db | DbTransaction) {
|
|||||||
})
|
})
|
||||||
.where(eq(companySecrets.id, secretId));
|
.where(eq(companySecrets.id, secretId));
|
||||||
}
|
}
|
||||||
|
// New references now see the deleted status, so the caller can settle
|
||||||
|
// every reference that exists before the row and its cascades go.
|
||||||
|
await options.beforeDelete?.(secret);
|
||||||
const providerConfig = secret.providerConfigId
|
const providerConfig = secret.providerConfigId
|
||||||
? await getProviderConfigById(secret.providerConfigId)
|
? await getProviderConfigById(secret.providerConfigId)
|
||||||
: null;
|
: null;
|
||||||
|
|||||||
Reference in new issue
Block a user