mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-04 11:01:16 +01:00
a836109fb0ba5387fead3f7426287e4b3c8ba767
9
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 DottaandPaperclip
|
3a726e676f |
feat: enforce private task permissions across execution and data (#10633)
Enforce private task and project access across direct reads, search, execution, files, plugins, live delivery, and sharing mutations. Preserve downward-only sharing, current responsible-user authorization, and audited emergency access. Bind historical draft assets with migration 0314. Co-Authored-By: Paperclip <noreply@paperclip.ing> |
||
|
DottaandPaperclip
|
c9b93d7e8c |
fix: preserve terminal task owners during release (#14561)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Tasks record an assigned owner and separate checkout and execution locks. > - A completed task must retain its owner after execution ends. > - The release endpoint currently clears that owner when it clears the locks. > - This pull request preserves the assignee of Done and Cancelled tasks during release. > - Unfinished tasks keep the existing relinquishment behavior. > - The benefit is stable task attribution without retaining execution locks. ## Linked Issues or Issue Description **What happened?** An agent completed an assigned task, then called the release endpoint. The task stayed Done, but its assignee became null. The same defect affects Cancelled tasks. It caused the legacy Claude clarification/reuse and multiple-repository handoff E2E assertions to fail. **Expected behavior** Release must clear execution locks on terminal tasks and preserve their assignee, final status, and disposition timestamps. Release of unfinished tasks must still clear the agent assignee. Only In Progress work returns to Todo. **Steps to reproduce** 1. Create an assigned task with checkout and execution locks. 2. Complete or cancel the task. 3. Call `POST /api/issues/:id/release` as the assigned agent. 4. Read the saved task. Before this fix, its assignee is null. **Paperclip version or commit** Reproduced on master commit `d172197117a14b80a1eb2d2835a0e7cce2679656`. **Deployment mode** Local tests against real PostgreSQL through the production issue routes and services. This is a core lifecycle defect, independent of the agent adapter. Refs: #11689, #6899, #7769. These are related open release proposals. This is an independent fix limited to terminal task ownership. It does not include timer scheduling changes. ## What Changed - Preserve the current assignee when releasing Done or Cancelled tasks. - Keep all execution-lock cleanup and existing unfinished-task behavior. - Cover all seven task statuses through the release API and read back saved state. - Check disposition timestamps, activity attribution, and repeated board cleanup. - Update the API contract, agent reference, and CLI help. ## Verification - Red commit `ddaabb754`: the two terminal-owner regressions failed with `assigneeAgentId: null`; 12 other route tests passed. - Green: all 14 route tests pass, plus the existing successor-checkout race test (15 selected tests total). - Command: `pnpm --filter @paperclipai/server exec vitest run src/__tests__/issue-stale-execution-lock-routes.test.ts src/__tests__/issues-service.test.ts -t 'stale issue execution lock routes|does not let stale release clobber a successor checkout lock'`. - The local host has exhausted its SysV semaphore pool. The red/green runs used the existing test-provider hook to start disposable Docker PostgreSQL 17 instances. Routes, services, migrations, and assertions were unchanged. No database tests in the selected set were skipped. The other 134 tests were excluded by the name filter. - Capability contract and inventory drift checks pass. - `pnpm build` and `pnpm -r typecheck` pass. - The local `pnpm test:run` was interrupted after environment failures while the complete sharded CI suite ran in parallel: native PostgreSQL bootstrap fails under the host semaphore limit, and the large Git fixture hits macOS `ENAMETOOLONG`. A focused rerun confirmed these happen before the relevant assertions. The interrupted local run is not counted as a full pass. - Greptile completed on `1caeeb827e9cb658ddb71f16c2421ec20f80634e` with **5/5**, a successful check, and no review threads. - All CI gates pass on the current head: typecheck, build, general and serialized tests, Runner checks, browser E2E, release packaging, and security checks. Server shard 11 passed on one targeted retry; the first attempt had 836 passing tests but an unhandled workspace-runtime startup rejection caused by an existing timing window. All other successful jobs were reused. - No paid provider evaluations were run. ## Risks - A caller that used release to erase ownership from terminal work will now retain that owner. An explicit assignment update or the board force-release option with `clearAssignee=true` can still clear it. - No schema or migration changes. The transaction, company access, assignee/run checks, and activity log remain in place. ## Model Used - OpenAI GPT-6 through Codex, with tool use, code execution, and test debugging. The session does not expose a more specific backend model version or context-window size. - OpenAI `gpt-6-luna` assisted with read-only test discovery and review. ## 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 - [x] All Paperclip CI gates are green - [x] 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: Paperclip <noreply@paperclip.ing> |
||
|
DottaandPaperclip
|
f91a6e27c0 |
feat(issues): contain cross-issue agent side effects (#10837)
## Thinking Path > - Paperclip is the control plane that coordinates autonomous agent work. > - Agents need to collaborate on issues beyond their current assignment. > - Cross-issue comments and updates are useful, but an unbounded run can create cascading side effects. > - The control plane must preserve company-wide collaboration while containing each run's influence. > - Comment attribution must also show the responsible user and the acting agent in audits. > - This pull request adds run-bound cross-issue containment, attribution, and agent-class wake rules. > - The benefit is safer collaboration without restoring issue-assignee ownership restrictions. ## Linked Issues or Issue Description **What existing behavior does this improve?** Agent-authenticated issue comments, updates, reopen behavior, and assignee wake routing. **Subsystem affected** Cross-cutting: server routes and services, shared contracts, database schema and migration, and implementation documentation. **Current behavior** An authenticated agent can collaborate across company issues, but one heartbeat run has no per-run side-effect boundary. Comment records also do not persist the responsible user separately from the acting agent. **Proposed behavior** Require a valid heartbeat run for agent cross-issue comments and updates. Audit each attempt and cap a run at 20 cross-issue effects. Keep the cap in log-only mode until it automatically changes to enforcement at 2026-08-11 00:00 UTC. Preserve same-issue writes. Use agent-class wakes for agent comments. Keep same-run completion comments from reopening completed work. Record the responsible user on agent-authored comments and activity. **Reason and benefit** Agents can collaborate on other issues without an assignment gate, while each run has an atomic and inspectable side-effect limit. Operators can identify both the acting agent and the responsible user. **Breaking changes** After 2026-08-11 00:00 UTC, the twenty-first cross-issue comment or update from one heartbeat run returns a containment error. Agent cross-issue writes without valid run context are rejected. The migration is additive and backfills existing agent-authored comment attribution where the source data is available. ## What Changed - Added an atomic per-run counter for cross-issue agent comments and updates. - Added audit events for allowed and rejected cross-issue effects. - Added the automatic log-only to enforcement flip at 2026-08-11 00:00 UTC. - Added responsible-user attribution to agent-authored comments, activity records, shared types, and validators. - Added an additive migration and migration coverage for existing comments. - Updated reopen, resume, and wake behavior so agent comments create agent-class wakes and same-run completion comments remain inert. - Updated the implementation specification and regression coverage. ## Verification - `pnpm exec vitest run server/src/__tests__/cross-issue-influence-limit.test.ts server/src/__tests__/issue-comment-attribution-audit-routes.test.ts server/src/__tests__/issue-comment-reopen-routes.test.ts packages/db/src/issue-comment-on-behalf-migration.test.ts` — 97 tests passed. - `pnpm -r typecheck` — passed, including migration safety checks. - `pnpm test:run` — server batch: 3,364 passed and 2 skipped; UI batch: 3,504 passed. One unrelated CLI doctor test warned because this agent runtime injects static AWS credentials. - `env -u AWS_ACCESS_KEY_ID -u AWS_SECRET_ACCESS_KEY pnpm exec vitest run cli/src/__tests__/secrets.test.ts` — 8 tests passed and confirmed the CLI failure was ambient-environment sensitive. - `pnpm build` — passed. ## Risks - The fixed enforcement timestamp changes production behavior automatically on 2026-08-11 00:00 UTC. Audit logs before that time provide rollout visibility. - The per-run counter serializes on the heartbeat-run row. This prevents concurrent attempts from racing past the cap but adds a small lock scope for cross-issue writes. - Existing comments can only be backfilled when their acting run or agent attribution is recoverable. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used OpenAI GPT-5 in the Codex agent runtime. The runtime did not expose a context-window size. Reasoning, shell tools, code editing, and test execution were enabled. ## 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 - [x] All Paperclip CI gates are green - [x] 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: Paperclip <noreply@paperclip.ing> |
||
|
|
1ba79d82a5 |
fix(server): preserve terminal status on issue release (#7524)
Fixes #4206 ## Thinking Path > - Paperclip orchestrates AI agents on issues with checkout/release semantics for execution locks > - `POST /api/issues/:id/release` clears checkout and execution locks when a heartbeat ends without finishing the issue > - `issues.release()` unconditionally set `status: "todo"`, undoing terminal and waiting states (`done`, `cancelled`, `in_review`, `blocked`) set during the session > - Agents reported status drift after release (e.g. `in_review` → `todo`, `done` → `todo`), forcing manual PATCH recovery and risking silent stalls > - This pull request gates the `todo` re-queue to `in_progress` issues only and preserves all other statuses on release > - The benefit is lock cleanup without destroying workflow state agents already recorded ## Linked Issues or Issue Description - Fixes #4206 — `issues.release()` must not downgrade terminal/waiting statuses - Related internal incident: AIT-114 status drift on terminal issue release (AI Trading Council) ## What Changed - `server/src/services/issues.ts` — `releaseStatus` is `todo` only when `existing.status === "in_progress"`; otherwise preserves `existing.status` - `server/src/__tests__/issue-stale-execution-lock-routes.test.ts` — regression tests: release preserves done, cancelled, in_review, blocked keeps `done` and clears lock fields - `server/package.json` — patch bump `0.3.1` → `0.3.2` - `server/CHANGELOG.md` — documents the fix ## Verification ```sh pnpm --filter @paperclipai/server test issue-stale-execution-lock-routes ``` - 7/7 tests pass (parametrized done, cancelled, in_review, blocked) (includes new `preserves terminal status when releasing a done issue` and existing `in_progress` → `todo` on release) - CI: Build, Typecheck, serialized server suites, e2e, Canary Dry Run green on latest head `f31b55f` ## Risks Low risk. Behaviour change is intentional: non-`in_progress` releases no longer force `todo`. Agents that relied on release to re-queue `in_review`/`blocked` work must PATCH status explicitly (documented in agent lifecycle guidance). Rollback: revert this commit and redeploy `@paperclipai/server` 0.3.1. ## Model Used Anthropic Claude Opus 4.6 (extended thinking mode) — 200K context window, tool use enabled. Assisted implementation and PR packaging for AI Trading Council upstream port from local hotfix. ## 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 run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots (N/A) - [x] I have updated relevant documentation to reflect my changes (CHANGELOG) - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups (re-review requested on head `f31b55f`) - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: brandon <brandonburr@gmail.com> |
||
|
Jannes StubbemannandClaude Opus 4.7
|
7f2ed0ad90 |
security(server): close cross-tenant existence oracle (404 instead of 403) (#3967)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - In a multi-tenant deployment, route handlers that take a resource id (`issue`, `goal`, `project`, `approval`, etc.) look the resource up by id and then call `assertCompanyAccess` on its `companyId` — 404 if it doesn't exist, 403 if it exists in another tenant > - The split status codes are a classic *existence oracle*: any authenticated user can enumerate ids across tenants by probing for the 403/404 boundary, mapping out which issues, labels, approvals, etc. exist in other customers' tenants even when they cannot read the contents > - The right fix is a single uniform 404 for both "not found" and "found but cross-tenant", which collapses the oracle but still preserves write-path checks (active membership, viewer-readonly) for *authorized* tenants > - This pull request adds a non-throwing `hasCompanyAccess(req, companyId)` helper plus a `getAccessibleResource` wrapper that ~130 handlers across 14 route files now use, folding the access check into the existence check while still running `assertCompanyAccess` for authorized tenants so viewer-readonly / inactive-membership rejections fire unchanged on write paths > - The benefit is closing a multi-tenant information leak without breaking write-path security or single-tenant local-first behavior ## Linked Issues or Issue Description Refs #709 — asks for company-scope regression coverage across approval/activity/access routes, because a subtle route refactor could leak cross-tenant data; this PR hardens exactly those surfaces (uniform 404 across 14 route files including `approvals`, `activity`, `secrets`) and updates cross-tenant expectations in test files. It does not add the full coverage matrix #709 asks for — hence Refs, not Closes. No existing issue covers the oracle itself — described in-PR: - Route handlers returned 404 for "not found" but 403 for "exists in another tenant", a classic *existence oracle*: any authenticated user could enumerate ids across tenants by probing the 403/404 boundary. - That maps out which issues, labels, approvals, etc. exist in other customers' tenants even when their contents are unreadable. - Fix: a uniform 404 for both cases, while keeping write-path checks (active membership, viewer-readonly) for authorized tenants. ## What Changed - **`server/src/routes/authz.ts`** — new `hasCompanyAccess(req, companyId): boolean` helper alongside the existing `assertCompanyAccess`. Docstring spells out the two-step pattern (404 gate, then `assertCompanyAccess` for write-path checks). The helper mirrors `assertCompanyAccess`'s company-scope semantics exactly — in particular, signed-in instance admins do **not** get blanket access to companies they are not a member of (the repo's `authz-company-access` tests pin that behavior for `assertCompanyAccess`; an earlier draft of the helper accidentally widened it for reads). - **`getAccessibleResource(req, res, lookup, notFoundMessage)`** — the safe thing is now the easy thing. One helper wraps the whole pattern (uniform 404 for missing/cross-tenant, then `assertCompanyAccess` for write-path membership checks) and ~130 handlers across 14 route files use it: ```ts const goal = await getAccessibleResource(req, res, svc.getById(id), "Goal not found"); if (!goal) return; ``` Files: `activity`, `agents`, `approvals`, `assets`, `costs`, `environments`, `execution-workspaces`, `file-resources`, `goals`, `issue-tree-control`, `issues`, `projects`, `routines`, `secrets`. Handlers with bespoke not-found behavior (the legacy `200 []` contract, audit-logged denials in `file-resources`, null-returning authz helpers) compose `hasCompanyAccess` directly using the documented two-step pattern: ```ts // step 1: close the oracle (uniform 404 for both not-found and cross-tenant) if (!existing || !hasCompanyAccess(req, existing.companyId)) { res.status(404).json({ error: "Goal not found" }); return; } // step 2: enforce write-path membership checks for authorised tenants (no-op on GET) assertCompanyAccess(req, existing.companyId); ``` Routes where `companyId` comes from *request input* (`req.params.companyId`, `req.body.companyId`, e.g. in `companies.ts` and `plugins.ts`) deliberately retain plain `assertCompanyAccess` — there's no existence oracle to close because the companyId is an input, not a discovered value. - **Full-sweep coverage** — a scripted audit of every `assertCompanyAccess(req, <resource>.companyId)` call site in `server/src/routes/` found ~55 lookup-then-assert pairs the first pass missed; all are now gated. Notable ones: the `/secret-provider-configs/:id` CRUD routes, the agents instructions-bundle/config-revision/skills-sync routes (which check access via the `assertCanUpdateAgent` / `assertCanReadAgent` / `assertCanManageInstructionsPath` helpers), `POST /heartbeat-runs/:runId/watchdog-decisions`, `GET /issues/:id/cost-summary`, the environment + environment-lease GET routes, all six issue-tree-control routes, ~24 issue sub-resource routes (document annotations, interactions, approvals links, recovery actions, plan decompositions, lock/unlock), and the three workspace file-resource routes (these throw `notFound` instead of `forbidden` inside their audit-logging wrappers, so denied attempts are still activity-logged server-side while the client sees a uniform 404). - **Helpers made self-defending** — `assertCanUpdateAgent` / `assertCanReadAgent` / `assertCanManageInstructionsPath` (agents) and `assertCanManage{Project,Execution}WorkspaceRuntimeServices` throw `notFound` for cross-tenant resources before their `assertCompanyAccess` step, so a future caller that forgets the route-level gate still can't reopen the oracle. - **Pattern enforcement** — new `authz-existence-oracle-guard.test.ts` statically scans `server/src/routes/*.ts` and fails CI on any `assertCompanyAccess(req, <resource>.companyId)` call that is not preceded by a `hasCompanyAccess` gate, with an explicit allowlist (plus staleness check) for the request-input cases. New routes that regress to the 403/404 split fail the suite with a message pointing at the documented pattern. - **Tests** — cross-tenant expectations updated from 403→404 where routes are now gated; new `hasCompanyAccess` unit tests in `authz-company-access.test.ts` pin the instance-admin/local-implicit/agent/none semantics in lockstep with `assertCompanyAccess`; `write-path-membership.test.ts` (added in an earlier round) confirms viewer/inactive users are still rejected on writes. - **One legacy-contract preserve** — `GET /heartbeat-runs/:runId/issues` still returns `200 []` for both "doesn't exist" and "cross-tenant" so the legacy contract is preserved while the oracle stays closed. ## Verification - `pnpm run typecheck` — PASS. - `pnpm -F @paperclipai/server exec vitest run` — full server suite green locally apart from 4 pre-existing local-environment failures (`paperclip-skill-utils` ×2 and `workspace-runtime` ×1 are cwd/git-environment dependent — verified identical on a clean checkout of the base; `heartbeat-process-recovery` is the known macOS flake). - The new `authz-existence-oracle-guard` test sweeps `server/src/routes/*.ts` and confirms no remaining `assertCompanyAccess(resource.companyId)` site without a `hasCompanyAccess` gate; the only allowlisted holdouts take `companyId` from request input. ## Risks - **API contract narrowing.** Any client that specifically checked for `403` on cross-tenant access now sees `404`. This is a strict narrowing (one status instead of two for the same negative outcome) and matches what a client should expect for any id it can't access. - **Write-path checks preserved.** `assertCompanyAccess` still runs after the 404 gate on write routes, so viewer-readonly / inactive-membership rejections fire unchanged for legitimate users. - **Instance-admin scope unchanged.** `hasCompanyAccess` denies signed-in instance admins without an explicit membership, exactly like `assertCompanyAccess` (pinned by unit tests) — so the gate introduces no new read access for admins. - **Single-tenant local-first deploys** behave identically — the helper short-circuits to `true` for `local_implicit` sessions. - No new env vars, no deployment-mode switch. ## Model Used Claude Opus 4.7 (1M context), extended thinking mode; completeness sweep + instance-admin parity fix by Claude Fable 5 (1M context). ## Checklist - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] Thinking path traces from project context to this change - [x] Model used specified - [x] Checked ROADMAP.md — part of the multi-tenant hardening initiative - [x] Tests run locally and pass - [x] Added/updated cross-tenant 404 expectations across test files - [x] No UI changes - [x] Documented risks above - [x] Will address all Greptile and reviewer comments before merge Part of the multi-tenant hardening initiative — see also #5864 (per-company JWT keys) and #5865 (plugin tables `company_id`). --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
DottaandPaperclip
|
ef617bee5c |
[codex] Enforce backend execution release gates (#9089)
## Thinking Path > - Paperclip is the open source control plane people use to manage AI agents for work. > - Backend execution safety is part of the control plane contract: agents must stop at budget hard limits, stale execution paths must not create duplicate live work, and checkout ownership must remain authoritative. > - The recovery branch bundled these release-gate checks with broader unrelated work. > - Reviewers need a narrow PR that isolates only the backend safety behavior and regression coverage. > - This pull request keeps budget incident creation idempotent so repeated evaluation does not duplicate release-gate telemetry or approvals. > - It also adds focused coverage for idle timer skips, stale queued-run behavior, and live checkout conflict preservation. > - The benefit is a smaller, reviewable release-gate slice for budget hard stops, stale execution recovery, and ownership-safe issue mutation. ## Linked Issues or Issue Description Refs #8866 This PR extracts a focused backend safety slice from the closed broad recovery PR. The underlying problem is that release-gate behavior needs direct regression coverage before review: budget hard stops should not duplicate incidents/logging on repeated evaluation, timer wakes should respect the no-actionable-work skip policy, stale queued runs should remain invalidated, and active checkout ownership must survive conflicting checkout attempts without side effects. ## What Changed - Made budget incident creation report whether an incident was newly created, so soft/hard threshold activity logs are emitted once per incident window. - Added embedded Postgres budget release-gate tests covering soft incident idempotency, hard-stop pause/cancel behavior, budget override resume behavior, and telemetry redaction. - Added heartbeat coverage for skipping generic timer wakes when the agent opts into `skipTimerWhenNoActionableWork`, while preserving legacy/proactive timer behavior. - Added stale execution lock route coverage proving a conflicting checkout returns `409` without overwriting live checkout or execution ownership and without writing checkout activity. ## Verification - `./node_modules/.bin/vitest run server/src/__tests__/budgets-service.test.ts server/src/__tests__/heartbeat-process-recovery.test.ts server/src/__tests__/heartbeat-stale-queue-invalidation.test.ts server/src/__tests__/issue-stale-execution-lock-routes.test.ts --no-file-parallelism --maxWorkers=1` - First run: 3 files passed, 98 tests passed; `issue-stale-execution-lock-routes.test.ts` failed during import because the isolated worktree initially lacked dev dependency links for `supertest`. - `CI=true NODE_ENV=development pnpm install --frozen-lockfile --ignore-scripts` - Recreated worktree dev dependency links; emitted unrelated plugin SDK bin warnings because plugin SDK dist files were not built under `--ignore-scripts`. - `./node_modules/.bin/vitest run server/src/__tests__/issue-stale-execution-lock-routes.test.ts --no-file-parallelism --maxWorkers=1` - Passed: 1 file, 7 tests. - `git diff --check` - Passed. ## Risks Low to medium risk. The production code change is intentionally small and only suppresses duplicate threshold activity logging for already-open budget incidents, but it affects budget release-gate observability. The new tests use embedded Postgres and should catch regressions in budget hard stops, timer wake gating, stale queue invalidation, and checkout conflict preservation. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used OpenAI Codex, GPT-5 coding agent, tool-use enabled. ## 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 - [x] All Paperclip CI gates are green - [x] 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: Paperclip <noreply@paperclip.ing> |
||
|
Vasu YadavandDevin Foley
|
d7049e0cae |
fix(server): adopt stale checkout run ownership (#5413)
## Thinking Path > - Paperclip is a control plane for autonomous AI-agent companies. > - Issue checkout ownership is part of the execution-control layer that prevents two runs from mutating the same task at the same time. > - The current lock model should preserve `409` conflicts for live competing owners, but it should not strand the rightful assignee behind a stale terminal run. > - A same-agent follow-up run can encounter an existing `checkoutRunId` from a failed, timed-out, succeeded, or missing heartbeat run. > - In that case, the new run should safely adopt ownership instead of failing with an ownership conflict. > - This pull request makes stale checkout adoption transactional and keeps live checkout owners protected. > - The benefit is safer run recovery without weakening single-owner checkout semantics. ## Linked Issues or Issue Description - Fixes #5350 - Closes #1508 - Closes #1970 - Closes #2083 - Closes #3158 - Closes #3190 - Related stale-lock PRs reviewed during dedup search: #7536, #6658, #5660, #5442, #6223, #7048, #6824, #6799 ## What Changed - Updated issue checkout ownership recovery so the current assignee can adopt a stale terminal or missing checkout run. - Added row locking around stale checkout adoption to avoid races while replacing `checkoutRunId` / `executionRunId`. - Preserved `409` behavior when a different live checkout owner is still active. - Prevented terminal actor runs from reclaiming an unowned checkout lock after the newer eager stale-checkout clear path. - Fixed the stale checkout test fixture so same-assignee cases do not insert duplicate agent rows. - Added/kept focused coverage for stale checkout adoption and live-owner conflict behavior. - Fixes #5350. ## Verification - Focused tests: ```sh pnpm exec vitest run server/src/__tests__/issues-service.test.ts server/src/__tests__/issue-stale-execution-lock-routes.test.ts ``` Result: ```text 2 passed, 84 tests passed ``` - Server typecheck: ```sh pnpm --filter @paperclipai/server typecheck ``` Result: ```text passed ``` - Live curl smoke confirmed same-agent stale checkout adoption returns `200` instead of `409`. ```text old_run_status=succeeded checkout_http=200 patch_http=200 ``` The PATCH response showed `checkoutRunId` and `executionRunId` updated to the new run id. ### Live curl smoke result <img width="1498" height="570" alt="Live curl smoke showing stale checkout adoption returned 200" src="https://github.com/user-attachments/assets/4bf834de-e3cd-4495-ac5a-74767b439eeb" /> ### Server request log <img width="631" height="131" alt="Server logs showing heartbeat, checkout, and patch requests succeeded" src="https://github.com/user-attachments/assets/ceaaa403-110e-44e8-bac8-5d8506e79cc3" /> ## Risks - Low to medium risk: this touches issue execution lock ownership. - The behavioral shift is intentionally narrow: only the current assignee can adopt stale terminal or missing checkout ownership. - Live checkout owners remain protected with `409`. - No database migration or API contract change. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - OpenAI GPT-5.5 Codex coding agent with repository tool use, shell execution, code review, and local verification. ## 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 run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] If this change affects the UI, I have included before/after screenshots - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge ## Cross-references and status (maintainer) - Closes #1508 - Closes #1970 - Closes #2083 - Closes #3158 - Closes #3190 - Status: rebased onto current master; focused tests and server typecheck pass locally; all required CI is green; Greptile is 5/5; master drift verified. --------- Co-authored-by: Devin Foley <devin@paperclip.ing> |
||
|
|
f3db7b88ea |
Clear stale checkoutRunId on run finalization and add backstop sweeper (#6008)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The issue subsystem holds per-row lock columns (`checkoutRunId`, `executionRunId`, `executionAgentNameKey`, `executionLockedAt`) that gate checkout, ownership, and release > - When a heartbeat run terminates, `releaseIssueExecutionAndPromote` clears the execution-lock columns but stale checkout locks could remain attached to dead runs in edge paths > - The original fix closed the finalization, checkout, release, and sweeper paths, but PR CI exposed one more process-loss retry path where a queued retry advanced `executionRunId` while leaving `checkoutRunId` pinned to the failed run > - This pull request closes the asymmetry: terminal-run cleanup and process-loss retry recovery release dead checkout locks while preserving live execution ownership > - The benefit is permanent, automatic self-heal of stale lock columns and fewer false checkout 409s requiring board intervention > - Related upstream issue: #6007 ## Linked Issues or Issue Description Refs #6007. Duplicate/related PR search performed on 2026-06-10 with query `checkoutRunId process loss retry stale checkout lock repo:paperclipai/paperclip`. Related PRs found and reviewed for overlap: - #7727 `fix(heartbeat): atomically advance checkoutRunId on process-loss retry` - #7707 `test: cover same-agent stale checkout adoption` - #3068 `fix: clear checkoutRunId when releasing issue execution lock` ## What Changed - `server/src/services/heartbeat.ts` `releaseIssueExecutionAndPromote`: extend the per-issue update to also null `checkoutRunId` when it matches the terminating run id. WHERE clause scoped to `executionRunId = run.id OR checkoutRunId = run.id` for idempotence. - `server/src/services/heartbeat.ts` process-loss retry: when queuing the retry run, move `executionRunId` to the retry and clear the failed run's `checkoutRunId` so the dead run no longer owns checkout. - `server/src/services/issues.ts`: add `clearCheckoutRunIfTerminal` helper, symmetric to `clearExecutionRunIfTerminal`. No assignee/status precondition. Wired into `checkout`, `assertCheckoutOwner`, and `release`. Exported on the issue service. - `server/src/services/recovery/service.ts`: add `sweepStaleIssueLocks`. Scans `issues` where `checkoutRunId IS NOT NULL OR executionRunId IS NOT NULL`, joins each referenced run, and clears all lock columns on issues whose referenced runs are all terminal or missing. Emits one `issue.stale_lock_cleared` activity log row per cleared issue. - `server/src/services/heartbeat.ts`: re-export the sweeper on the heartbeat facade. - `server/src/index.ts`: invoke `sweepStaleIssueLocks` in both the startup recovery sequence and the periodic heartbeat timer chain. - Tests: route-level coverage of the new self-heal path on the next checkout attempt, service-level sweeper coverage, and heartbeat recovery assertions that terminal process-loss cleanup releases `checkoutRunId`. ## Verification ```bash pnpm --filter @paperclipai/server typecheck pnpm --filter @paperclipai/server exec vitest run \ src/__tests__/recovery-stale-issue-lock-sweep.test.ts \ src/__tests__/issue-stale-execution-lock-routes.test.ts NODE_ENV=test pnpm exec vitest run src/__tests__/heartbeat-process-recovery.test.ts -t "queues exactly one retry when the recorded local pid is dead|does not block paused-tree work when immediate continuation recovery is suppressed by the hold" NODE_ENV=test pnpm exec vitest run src/__tests__/heartbeat-process-recovery.test.ts ``` All listed local checks pass. The new and updated tests cover: - Run termination clears `checkoutRunId` when it points at the terminating run. - Process-loss retry clears the failed run's `checkoutRunId` while assigning `executionRunId` to the queued retry. - A different agent calling `POST /api/issues/:id/checkout` on an issue whose prior owner died self-heals via `clearCheckoutRunIfTerminal` and succeeds. - Sweeper clears stale lock columns for issues whose run row is terminal. - Sweeper leaves issues alone while the referenced run is still running. - Sweeper leaves issues alone when `executionRunId` is still running even if `checkoutRunId` is terminal. - Sweeper is idempotent; second pass clears nothing. Manual reproduction of the original bug shape: 1. Create an issue assigned to agent A, set `status='in_progress'`, `checkoutRunId=R1`, `executionRunId=null`, where `heartbeat_runs.status = 'failed'` for `R1`. 2. Reassign to agent B and move to `status='todo'`. 3. Before this PR: agent B `POST /checkout` returns `409 Issue checkout conflict` indefinitely. After this PR: succeeds, lock columns rewritten to agent B's current run id. ## Risks - Low. All clears are scoped by run id, so they only fire when the lock column unambiguously points at the terminating or terminal run. No schema change. No migration. No API surface change. - Behavioral shift: an issue that previously stayed `in_progress` with a dead `checkoutRunId` after run termination now self-heals. Downstream code that reads stale `checkoutRunId` as a proxy for recent run history should already be reading `executionRunId` or the `heartbeat_runs` table. - Sweeper cost: one indexed scan per recovery tick over rows where `checkoutRunId IS NOT NULL OR executionRunId IS NOT NULL` plus a single batched `heartbeatRuns` lookup per candidate. Negligible at expected cardinality; further bounded by the existing recovery cadence. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. This is a bug fix, not a feature. No roadmap overlap. ## Model Used - Claude (Anthropic), model ID `claude-opus-4-7`, extended-thinking off, tool use enabled. - OpenAI Codex, GPT-5-based coding agent, tool use enabled, used for the follow-up process-loss retry fix and PR body update. ## 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 run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] 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> Co-authored-by: Dotta <bippadotta@protonmail.com> |
||
|
Dotta
|
b69b563aa8 |
[codex] Fix stale issue execution run locks (#4258)
## Thinking Path > - Paperclip is a control plane for AI-agent companies, so issue checkout and execution ownership are core safety contracts. > - The affected subsystem is the issue service and route layer that gates agent writes by `checkoutRunId` and `executionRunId`. > - PAP-1982 exposed a stale-lock failure mode where a terminal heartbeat run could leave `executionRunId` pinned after checkout ownership had moved or been cleared. > - That stale execution lock could reject legitimate PATCH/comment/release requests from the rightful assignee after a harness restart. > - This pull request centralizes terminal-run cleanup, applies it before ownership-gated writes, and adds a board-only recovery endpoint for operator intervention. > - The benefit is that crashed or terminal runs no longer strand issues behind stale execution locks, while live execution locks still block conflicting writes. ## What Changed - Added `issueService.clearExecutionRunIfTerminal()` to atomically lock the issue/run rows and clear terminal or missing execution-run locks. - Reused stale execution-lock cleanup from checkout, `assertCheckoutOwner()`, and `release()`. - Allowed the same assigned agent/current run to adopt an unowned `in_progress` checkout after stale execution-lock cleanup. - Updated release to clear `executionRunId`, `executionAgentNameKey`, and `executionLockedAt`. - Added board-only `POST /api/issues/:id/admin/force-release` with company access checks, optional `clearAssignee=true`, and `issue.admin_force_release` audit logging. - Added embedded Postgres service tests and route integration tests for stale-lock recovery, release behavior, and admin force-release authorization/audit behavior. - Documented the new force-release API in `doc/SPEC-implementation.md`. ## Verification - `pnpm vitest run server/src/__tests__/issues-service.test.ts server/src/__tests__/issue-stale-execution-lock-routes.test.ts` passed. - `pnpm vitest run server/src/__tests__/issue-stale-execution-lock-routes.test.ts server/src/__tests__/approval-routes-idempotency.test.ts server/src/__tests__/issue-comment-reopen-routes.test.ts server/src/__tests__/issue-telemetry-routes.test.ts` passed. - `pnpm -r typecheck` passed. - `pnpm build` passed. - `git diff --check` passed. - `pnpm lint` could not run because this repo has no `lint` command. - Full `pnpm test:run` completed with 4 failures in existing route suites: `approval-routes-idempotency.test.ts` (2), `issue-comment-reopen-routes.test.ts` (1), and `issue-telemetry-routes.test.ts` (1). Those same files pass when run isolated and when run together with the new stale-lock route test, so this appears to be a whole-suite ordering/mock-isolation issue outside this patch path. ## Risks - Medium: this changes ownership-gated write behavior. The new adoption path is limited to the current run, the current assignee, `in_progress` issues, and rows with no checkout owner after terminal-lock cleanup. - Low: the admin force-release endpoint is board-only and company-scoped, but misuse can intentionally clear a live lock. It writes an audit event with prior lock IDs. - No schema or migration changes. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - OpenAI Codex, GPT-5 coding agent (`gpt-5`), agentic coding with terminal/tool use and local test 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) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge |