Commit Graph

2 Commits

Author SHA1 Message Date
Nicky Leach 44694328a3
fix(issues): make DELETE /api/issues/:id succeed for issues with dependents (#11331)
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work
> - The server provides issue APIs and the database stores issue child
rows
> - The issue delete endpoint removes the parent issue before dependent
rows
> - Several issue foreign keys had no delete policy, so PostgreSQL
returned a foreign-key error
> - This pull request adds safe cascade and set-null policies and a
clear conflict response
> - The benefit is reliable issue deletion with a useful error when a
restricted audit row still blocks deletion

## Linked Issues or Issue Description

Fixes #7728
Fixes #4660
Fixes #7991
Fixes #4627
Fixes #5086

**What happened?**

`DELETE /api/issues/:id` returned HTTP 500 when dependent comments,
thread interactions, read states, inbox archives, feedback votes, or
ledger rows referenced the issue. The database raised SQLSTATE 23503
because several foreign keys had no delete policy.

**Expected behavior**

The endpoint must remove dependent rows that have no meaning without the
issue. It must keep ledger rows with a null issue reference. It must
return HTTP 409 when a restricted decision audit row still references
the issue.

**Steps to reproduce**

1. Create an issue.
2. Add a comment or thread interaction that references the issue.
3. Send `DELETE /api/issues/:id`.
4. Observe the HTTP 500 response.

**Paperclip version or commit**

Commit `1f8f456f8340823fe2bd891ae8933d942f190b7b`.

**Deployment mode**

Local dev with embedded PGlite or external PostgreSQL.

## What Changed

- Add `CASCADE` to five issue child foreign keys.
- Add `SET NULL` to the finance and cost event issue foreign keys.
- Keep decision audit references restricted.
- Map SQLSTATE 23503 from the issue delete service to HTTP 409.
- Add migration 0217 for the seven changed tables.
- Add regression tests for cascade deletion and restricted decision
references.

## Verification

- Run `pnpm --filter @paperclipai/db typecheck`.
- Run `pnpm --filter @paperclipai/server typecheck`.
- Run `npx vitest run src/__tests__/issue-remove-cascade.test.ts` from
`server/`.
- The regression test applies migration 0217 to a fresh embedded
PostgreSQL database.

## Risks

- Migration 0217 changes only seven foreign keys that reference
`issues.id`.
- Cascade deletion removes child rows that cannot exist without the
parent issue.
- Set-null preserves finance and cost ledger rows.
- Decision audit rows remain protected, so the endpoint can return HTTP
409.

## Model Used

Codex, based on GPT-5, with tool use and code-review support. The
implementation author used an AI coding agent. This PR handoff uses the
same model family to validate the commit and manage the pull request.

## 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
- [ ] I will address all Greptile and reviewer comments before
requesting merge

---------

Co-authored-by: Paperclip <noreply@paperclip.ing>
2026-08-13 12:02:15 -07:00
LeonSGP 6a4e2e1b8c
fix(routes): return 409 for routine checkout conflicts (#3790)
## Thinking Path

> - Paperclip orchestrates AI agents and relies on issue checkout as the
core task-claiming primitive
> - The issue checkout route is the HTTP boundary that translates
service and database outcomes into agent-usable API responses
> - Routine-linked issues are protected by the partial unique index
`issues_open_routine_execution_uq`, which covers only rows whose
`execution_run_id` is set
> - `svc.checkout` sets `execution_run_id`, so a concurrent claim moves
the row into that index and can raise a 23505 mid-request
> - Unhandled, that surfaces as a 500 and crashes the agent run instead
of being a recoverable conflict
> - Drizzle wraps driver failures in its own `Failed query: ...` error,
so the Postgres error carrying `code` and the constraint name is
reachable only through `cause`
> - This pull request translates that violation into a 409 at the
checkout route, detecting it through the cause chain the way
`isReviewPathRecoveryIdempotencyConflict` already does
> - The benefit is that agents handle routine execution contention
through the normal heartbeat conflict path instead of failing on an
internal server error

## Linked Issues or Issue Description

Fixes #3660

Related pull requests found while searching for duplicates:

- #3699 — an earlier attempt at this same route-level fix, closed
unmerged. Same shape, and its check has the flat-error bug described
under Verification.
- #3633 — related work on postgres.js `constraint_name` handling in
conflict detection.
- #5662 — covers the adoption path (`assertCheckoutOwner`) that this
pull request does not.

## What Changed

- Added `server/src/db-errors.ts` with `isUniqueViolation(error,
constraintName?)`, which walks the `cause` chain (depth-capped) and
accepts the postgres.js `constraint_name`, the node-postgres
`constraint`, or the driver message as evidence of SQLSTATE 23505.
- Wrapped `svc.checkout()` in `POST /issues/:id/checkout` with a narrow
try/catch that uses that helper to return **409 Conflict** for
`issues_open_routine_execution_uq`, and rethrows every other error
unchanged.
- Added `server/src/__tests__/db-errors.test.ts` covering the wrapped
and bare error shapes, both constraint field names, the message
fallback, non-matching constraints, non-unique-violation codes, and a
self-referential cause chain.

## Verification

- The new unit test includes the wrapped case `{ cause: { code: "23505",
constraint_name: ... } }` that a flat `error.code` check fails, so it is
a real regression guard rather than a restatement of the implementation.
- The wrapped shape is what this codebase observes in practice:
`server/src/__tests__/plugin-tenant-isolation.test.ts` asserts
`cause?.code === "23505"` against embedded Postgres,
`packages/db/src/pipelines-schema.test.ts` asserts that constraint
failures throw `Failed query`, and
`server/src/services/recovery/review-path-recovery.ts` walks the same
chain.
- CI (verify, e2e, policy) exercises this change against current master
through the pull request merge ref.
- Not verified locally: no monorepo install or typecheck was run in this
environment.

## Risks

- Low. One route gains a catch that matches a single constraint and
rethrows all other errors, so no unrelated failure can be swallowed.
- The 409 body `{ error: ... }` matches the other 409 responses this
route already returns.
- Scope limit: this covers the checkout route only. The adoption path
reached through `assertCheckoutOwner` (heartbeat, plugins, and pipelines
routes) can still surface the same violation as a 500; #5662 targets
that path.
- `isUniqueViolation` is new and intentionally generic. Existing flat
23505 checks elsewhere in the server are left untouched by this pull
request.

## Model Used

- Original change: OpenAI Codex, GPT-5-class tool-using coding agent in
the Codex CLI environment; exact backend model revision is not exposed
in that runtime.
- Follow-up revision (cause-chain detection plus tests): Anthropic
Claude Opus 5 (`claude-opus-5`), tool-using coding agent with extended
thinking and code execution.

## 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)
- [ ] 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
- [ ] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [ ] 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

---------

Co-authored-by: Andrew Aymeloglu <aaymeloglu@gmail.com>
2026-08-09 20:20:09 -05:00