test(server): convert heartbeat suites to shared TRUNCATE CASCADE teardown - #201
Open
claudegoogl-sudo wants to merge 235 commits into
Open
test(server): convert heartbeat suites to shared TRUNCATE CASCADE teardown#201claudegoogl-sudo wants to merge 235 commits into
claudegoogl-sudo wants to merge 235 commits into
Conversation
* fix(PLA-98): require schema-qualified refs in ctx.db.query `validatePluginRuntimeQuery` previously only matched `schema.table` patterns to scope-check, which left unqualified table refs (e.g. `FROM agents`) entirely unchecked. Plugins with `database.namespace.read` could read every table in `public` via the connection's default `search_path`, bypassing the `coreReadTables` whitelist. Defense in depth: 1. Application layer: new `assertRuntimeQueryRefsQualified` walks the SQL, tracks paren scope (subquery vs. function call like `EXTRACT(... FROM x)`), and rejects any `FROM`/`JOIN` whose operand is not schema-qualified or a sub-`(SELECT|WITH ...)`. Documented as a regex stopgap pending PLA-94 F2. 2. Postgres layer: `pluginDatabaseService.query` and `.execute` now run inside a transaction with `SET LOCAL search_path TO <namespace>, pg_temp`, so any unqualified ref that escapes the validator resolves inside the plugin's schema instead of `public`. Tests: - Unit tests for the issue's repro queries (`SELECT * FROM agents`, nested `(SELECT count(*) FROM cost_events)`). - Unit tests proving EXTRACT, subquery, and LATERAL operands still pass. - Embedded-Postgres integration test asserting the application layer rejects the unqualified read before SQL is dispatched. Docs: plugin authoring guide and SDK README mark the `ctx.db` SQL surface as alpha and call out the qualified-ref + parameterized-input contracts. Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix(PLA-99): apply qualified-ref scan to validatePluginRuntimeExecute PLA-98 wired assertRuntimeQueryRefsQualified into validatePluginRuntimeQuery but left validatePluginRuntimeExecute relying only on extractQualifiedRefs, which never inspects subqueries. Writes such as UPDATE plugin_test.tbl SET x = (SELECT y FROM agents) WHERE id = $1 DELETE FROM plugin_test.tbl WHERE id IN (SELECT id FROM cost_events) passed application-layer validation today; only the PLA-98 search_path defense at the Postgres layer caught them. The layered-defense story in PLUGIN_AUTHORING_GUIDE.md and sdk/README.md claims application-layer rejection for both query and execute - this brings execute in line. - Parameterize assertRuntimeQueryRefsQualified with a caller label so the thrown message is correct for both ctx.db.query and ctx.db.execute. - Call assertRuntimeQueryRefsQualified(statement, "ctx.db.execute") in validatePluginRuntimeExecute after the keyword/banned checks. - Add unit tests covering the two repro statements (must throw) and a happy-path write with a fully-qualified subquery (must pass). Confirmed the new rejection tests fail against the pre-fix code. Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip CEO <ceo@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing>
…I routes (#5) Lock in the host's tenancy gate at server/src/routes/plugins.ts:1477 (assertCompanyAccess between companyResolution and worker dispatch). A future refactor that silently removes the gate would have been invisible until exploited; these tests fail in that case. Covers all four companyResolution modes the gate sits behind: - Agent for company A querying ?companyId=B (from: "query", GET) - Agent for company A posting { companyId: B } (from: "body", POST) - Board user with companyIds=[A] hitting ?companyId=B (from: "query") - Agent for company A targeting issue from company B (from: "issue") Each test asserts HTTP 403, the exact authz error message, and that workerManager.call is never invoked — i.e. the gate runs before any plugin code. The "issue" mode test also asserts assertCheckoutOwner is not called, isolating assertCompanyAccess from the layered enforceScopedApiCheckout "Issue not found" defense. No production code changes — the gate is verified to exist today. Verified locally: - All 12 tests pass against current code (8 prior + 4 new). - Commenting out assertCompanyAccess in plugins.ts:1477 fails the 4 new tests with status 200 (board) / 200 (issue) / etc., confirming the gate is what they protect. Co-authored-by: Paperclip CEO <ceo@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing>
…n mutations (#7) * PLA-40: redact secrets in plugin run-log chunks at append() boundary Apply redactSensitiveText() to event.chunk in RunLogStore.append() before NDJSON serialization, closing a defence-in-depth gap where a plugin that accidentally logs a resolved secret (GitHub PAT, OpenAI key, JWT, Bearer header, etc.) would persist plaintext to data/run-logs/*.ndjson. Adds an append()-level integration test that exercises the path end-to-end: writes a chunk containing a ghp_* token through a RunLogStore against a tmp RUN_LOG_BASE_PATH, reads back the NDJSON, and asserts the token is redacted while ts/stream/chunk schema is preserved (AC #2 + #3). Also adds a non-secret passthrough test. Mirrors the previously security-reviewed dist/-only patch onto master source so a rebuild cannot silently revert the protection. Co-Authored-By: Paperclip <noreply@paperclip.ing> * PLA-141: clear orphan checkoutRunId atomically and tolerate it on mutation Routine_execution issues stay `in_progress` between runs, so when a routine heartbeat finishes Paperclip's `releaseIssueExecutionAndPromote` was clearing only the execution-lock triple. The `checkoutRunId` was left pointing at the now-terminal `heartbeat_runs` row, which: - blocked subsequent mutations (PATCH/comment/release returned 500 because the assertCheckoutOwner path expected the run to still be running, and board users hit the wedge directly during the 5-minute adoption window), - poisoned the `long_active_duration` productivity heuristic into pinging "stuck for N hours" on issues that were actually idle. Three orphan rows were back-filled out of band (PLA-120, PLA-26, PLA-32); PLA-120 was force-closed. Changes: - `releaseIssueExecutionAndPromote` (heartbeat.ts) now clears `checkoutRunId`/`executionRunId` independently inside the same write when the completing run owns either side, so the two locks can never desync again. - `clearOrphanCheckoutLocksIfTerminal` helper (services/issues.ts) walks each lock column, takes a row lock + a stable-ordered run lock, and clears any side whose referenced run is terminal (or missing). Returns true if it cleared anything; safe to call on every mutation entry point. - The four mutation routes that were affected (PATCH `/issues/:id`, POST `/issues/:id/checkout`, POST `/issues/:id/release`, POST `/issues/:id/comments`) call the helper before their existing ownership/back-fill paths, so an orphan checkoutRunId is treated as unowned rather than as a 500. - `productivity-review.ts` now skips the `long_active_duration` flag when the `checkoutRunId` is terminal AND the latest assignee comment is older than the run's `finishedAt` — the common signature of a routine-completion orphan that nobody has actually been working on. - New regression test `issue-orphan-checkout-runid-routes.test.ts`: PATCH and addComment from the assignee both succeed when the lock columns point at terminal runs, the helper is a no-op when the runs are still active, and clears only the terminal side when one lock is live. Verified: pnpm vitest run server/src/__tests__/issue-orphan-checkout-runid-routes.test.ts -> 4 passed pnpm vitest run server/src/__tests__/issue-stale-execution-lock-routes.test.ts -> 3 passed pnpm vitest run server/src/__tests__/productivity-review-service.test.ts server/src/__tests__/routine-run-telemetry.test.ts -> 12 passed Co-Authored-By: Paperclip <noreply@paperclip.ing> * PLA-141: pin productivity-suppression and ownsCheckoutLock cleanup as unit tests Pre-merge tweaks requested in CTO sign-off (PLA-141 thread). Three small test changes, no production code changes: - `productivity-review-service.test.ts`: add the suppression-branch unit test for `productivity-review.ts:484-503` — terminal `checkoutRunId` + no later assignee comment must drop `long_active_duration` to zero reviews. Add a positive control where `checkoutRunId` points at a still-running run so the trigger is not suppressed (guards against the terminal-status gate widening accidentally to a typo regression). - `heartbeat-process-recovery.test.ts`: add the `ownsCheckoutLock` post-state unit test for `heartbeat.ts:6224-6245` — pre-clear the execution lock so only the `checkoutRunId` orphan is in play, then `cancelRun` and assert the row ends up with `checkoutRunId = NULL`. Direct coverage of the root-cause fix branch. - `heartbeat-process-recovery.test.ts`: update the existing paused-tree recovery assertion (`does not block paused-tree work…`) from `checkoutRunId === runId` to `toBeNull()`. The previous expectation was a pre-PLA-141 artifact: it asserted the orphan-checkout state we are now fixing on every reap. The pause hold is what gates further recovery, not a stale checkout lock — confirmed by the rest of that test (no recovery issues, no comments, status still `in_progress`). Verified: pnpm vitest run server/src/__tests__/productivity-review-service.test.ts -> 13 passed (was 11) pnpm vitest run server/src/__tests__/heartbeat-process-recovery.test.ts -> 38 passed (was 37, plus the corrected paused-tree assertion) pnpm vitest run server/src/__tests__/issue-orphan-checkout-runid-routes.test.ts server/src/__tests__/issue-stale-execution-lock-routes.test.ts server/src/__tests__/routine-run-telemetry.test.ts -> 8 passed (no behavioural change) pnpm vitest run server/src/__tests__/heartbeat-issue-liveness-escalation.test.ts server/src/__tests__/heartbeat-stale-queue-invalidation.test.ts server/src/__tests__/run-liveness.test.ts server/src/__tests__/issue-liveness.test.ts -> 37 passed (adjacent regression check) Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip CEO <ceo@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing>
Node's ESM loader keys cached modules by absolute specifier, so
`await import('/.../dist/manifest.js')` returned the original module
for the lifetime of the process even after a rebuild. Every
install/upgrade against the same path silently reused the stale
manifest, freezing tool/route/capability registration and the
DB-side `manifestJson` hydrated from the cached import.
Fix in `plugin-loader.ts`:
- Extract `loadManifestModule()` that resolves a file URL plus
`?mtime=<mtimeMs>` and dynamically imports that. Unchanged files
keep the same URL (cached) → no perf regression; changed files get
a fresh URL → fresh import.
- Route `loadManifestFromPath()` through it so install/upgrade and
registry persistence both see on-disk contents. `activatePlugin`
reads `plugin.manifestJson`, which is now refreshed by the install
/ update writes per AC #3.
Roll-in (validator):
- Reject `:` in `tool.name` at install/upgrade time with a clear
error pointing at the offending name and suggesting the bare-name
fix. Catches the original `cad:run_script` typo from PLA-58 before
the cache could be poisoned in the first place.
Tests (`plugin-loader.test.ts`):
- Regression test that writes a manifest, reads it, overwrites with
a new tool name + bumped mtime, and asserts the second read sees
the new name. Verified to fail without the cache-bust query
string and pass with it.
- Cached-when-unchanged test pins identity equality across two reads
of an unchanged file (no perf regression).
- Validator tests covering the ':' rejection (with offending-name
pointer) and the bare-name happy path.
Local-dev impact: no host restart required between iterations of a
plugin's `dist/manifest.js` — uninstall + reinstall (or upgrade)
observes the latest build immediately.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Apply redactSensitiveText() to event.chunk in RunLogStore.append() before NDJSON serialization, closing a defence-in-depth gap where a plugin that accidentally logs a resolved secret (GitHub PAT, OpenAI key, JWT, Bearer header, etc.) would persist plaintext to data/run-logs/*.ndjson. Adds an append()-level integration test that exercises the path end-to-end: writes a chunk containing a ghp_* token through a RunLogStore against a tmp RUN_LOG_BASE_PATH, reads back the NDJSON, and asserts the token is redacted while ts/stream/chunk schema is preserved (AC #2 + #3). Also adds a non-secret passthrough test. Mirrors the previously security-reviewed dist/-only patch onto master source so a rebuild cannot silently revert the protection. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…allowlist SE review on PR paperclipai#5019 flagged the colon denylist as a deny-over-anything-string — whitespace, control chars, path separators, and unicode lookalikes remained legal on `pluginToolDeclarationSchema.name`. Replace it with the same allowlist regex already used by `pluginEnvironmentDriverDeclarationSchema.driverKey`: /^[a-z0-9][a-z0-9._-]*$/ The allowlist subsumes the previous ':' denylist, so the manifest-level superRefine block at packages/shared/src/validators/plugin.ts:658-669 is deleted. The `cad:run_script` regression case from PLA-58 still fails validation (the colon is not in the allowlist), and the field path ("tools", i, "name") is still reported for UI highlighting. Tests: - Update the colon-rejection test to assert the allowlist contract (shape-focused message + name-targeted path). - Add a new test that rejects whitespace, uppercase, and path separators to lock the broader allowlist behavior in. - All `packages/shared` validator tests + plugin-loader/run-log-store tests pass. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…idget (#6) The host only mounts dashboardWidget in v1, but the manifest validator currently accepts the full PLUGIN_UI_SLOT_TYPES enum. A plugin shipping type: "sidebar" validates fine, installs, then silently never renders. Catch at the install boundary by refining ui.slots[].type to the v1 host-rendered subset and emitting a clear error that names the supported set (e.g. `Invalid slot type "sidebar". v1 supports: dashboardWidget`). PLUGIN_UI_SLOT_TYPES stays the canonical list of *planned* slot types for the host registry and future SDK versions; only the manifest schema narrows. The new const PLUGIN_UI_SLOT_TYPES_V1_SUPPORTED is the seam to expand as more slot types reach the v1 host-rendered floor. Adds focused validator tests covering the dashboardWidget happy path and each non-entity-scoped reserved slot type. Co-authored-by: Paperclip CEO <ceo@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing>
…ch can find the worker When a plugin's tools are registered through `pluginLoader → toolDispatcher.registerPluginTools → registry.registerPlugin`, the optional `pluginDbId` (the plugin row's DB UUID) was never threaded from the loader to the registry. The registry then defaulted `tool.pluginDbId` to the plugin **key** (e.g. `"platform.cad"`). `PluginToolRegistry.executeTool` calls `workerManager.isRunning(tool.pluginDbId)` to decide whether to dispatch. The worker manager is keyed by DB UUID, so the lookup with the plugin key returns `false` and every dispatch fails closed with `502 "worker for plugin '<key>' is not running"`, even when the worker is running. PLA-308 hit this on `cad:run_script` and `cad:export`. The fix is a 3-line plumbing change: - Extend `PluginToolDispatcher.registerPluginTools` with an optional `pluginDbId?: string` parameter (interface + factory implementation). Backwards-compatible: callers that pass two args still work. - At the only host call site in `plugin-loader.ts`, pass the DB UUID (`pluginId` in loader scope) as the third argument. `pluginKey` continues to be the namespacing key. The downstream `PluginToolRegistry.registerPlugin(pluginId, manifest, pluginDbId?)` already accepted and plumbed this argument through `addTool` to `tool.pluginDbId` — this PR just wires it end-to-end. Adds a focused regression test that registers a plugin via the dispatcher with a synthetic UUID and asserts `tool.pluginDbId` is the UUID (not the key). Confirmed: the new test fails on master and passes with this patch. Co-Authored-By: Paperclip <noreply@paperclip.ing>
fix(PLA-323): thread pluginDbId through registerPluginTools so dispatch can find the worker
PLA-159/40/163: manifest cache-bust + secret redaction + tool name allowlist
Stamp all 24 workspace package.json files for fork-build-4. Cumulative content: PLA-159 (manifest cache-bust), PLA-40 (run-log secret redaction), PLA-163 (plugin tool name allowlist), plus PLA-323 from fork-build-3 already on master. Refs: PLA-348 Co-Authored-By: Paperclip <noreply@paperclip.ing>
Re-pack of fork-build-4 content (PLA-159 / PLA-40 / PLA-163 cumulative on top of fork master) — fork-build-5 fixes the missing ui-dist/ packaging from fork-build-4 (server prepack: prepare:ui-dist did not run). Refs PLA-366 Co-Authored-By: Paperclip <noreply@paperclip.ing>
* feat(PLA-376): scaffold release-time manifest validation gate Mirrors the PLA-376 release-time validation gate from paperclip-plugin-cad into the create-paperclip-plugin scaffold so every new plugin inherits the gate by default. Same rationale: plugin-cad v0.1.1 shipped with `cad:run_script` tool names that the post-PLA-163 host validator rejects, the release passed CI before the operator's install attempt revealed the manifest was unloadable, and PLA-373 ask C — "make this not happen again" — applies to all future plugins, not just plugin-cad. What scaffolded plugins now get out of the box - `scripts/validate-manifest.mjs` — loads `dist/manifest.js` via `paperclipPlugin.manifest`, runs `pluginManifestV1Schema` from `@paperclipai/shared/validators/plugin`, and additionally enforces the PLA-163 tool-name allowlist regex (mirrored locally because the published shared package lags the fork validator on this rule). - `tests/validate-manifest.spec.ts` — vitest regression coverage that proves the gate catches `bad:name` and accepts the dot/hyphen/ underscore lowercase forms. - `.github/workflows/manifest-validate.yml` — PR + push gate that builds, runs the regression test, then runs the gate against the built manifest. - `package.json` — `validate:manifest` script and a `prepack` hook (`npm run build && npm run validate:manifest`) so `npm pack` / `npm publish` cannot succeed with a manifest the host will reject. Why @paperclipai/shared moves out of the conditional The gate imports `pluginManifestV1Schema` from `@paperclipai/shared/validators/plugin`, so every scaffolded plugin needs the dep — no longer optional. Workspace-internal scaffolds get `workspace:*`; out-of-workspace scaffolds get the packed-tarball `file:` dep that the previous code already produced. The `*` fallback case is unreachable because `packedSharedTarball` is non-null whenever `useWorkspaceSdk` is false (see initialization above). Verification - `npx tsc --noEmit` clean. - Smoke-scaffolded a plugin into `packages/plugins/examples/scaffold-pla376-test/` (tracked locally, not committed); generated tree includes `scripts/`, `tests/`, `.github/workflows/manifest-validate.yml`, and the package.json carries the new scripts and the workspace:* shared dep. Out of scope - The validator regex itself is not changed (PLA-376 explicit constraint). - Existing plugin-cad gate lives at `claudegoogl-sudo/paperclip-plugin-cad#pla-376/release-manifest-validate-gate` and is the reference implementation this scaffold mirrors. Co-Authored-By: Paperclip <noreply@paperclip.ing> * test(PLA-376): smoke-test the scaffolded manifest-validation gate in PR CI Addresses CTO sign-off conditional ([PLA-376](/PLA/issues/PLA-376)) — the fork-side `pr.yml` exists but never fired on PR #11 (empty rollup). Even when it does fire, the scaffold change in `packages/plugins/create-paperclip-plugin/src/index.ts` is a generator with no per-package vitest suite, so `pnpm test:run` only typechecks it. Without this step, no CI gate proves the scaffold actually emits the PLA-376 gate files / wiring before merge. What changed: - `scripts/smoke-create-paperclip-plugin.test.mjs` — a `node --test` smoke that: - imports the scaffold's built `dist/index.js` (after `pnpm build`), - calls `scaffoldPluginProject` into a tmp dir inside the repo so the `useWorkspaceSdk` branch is taken (no `pnpm pack` of SDK + shared), - asserts every PLA-376 gate file is emitted with the expected key contents (validate-manifest.mjs imports `pluginManifestV1Schema` and mirrors the PLA-163 `^[a-z0-9][a-z0-9._-]*$` regex; spec covers `bad:name`; workflow file present; package.json wires `validate:manifest` + `prepack` + `@paperclipai/shared` devDep), - cleans up `.tmp-scaffold-smoke` on exit. - `package.json` — `test:scaffold-create-paperclip-plugin` script (matches the existing `test:release-registry` pattern, same node --test runner). - `.github/workflows/pr.yml` — new `verify` step `Verify scaffold manifest-validation gate (PLA-376)` after `Build`, invoking the new script. Runs after `pnpm build` so the scaffold's `dist/index.js` exists. Verification (local): node --test scripts/smoke-create-paperclip-plugin.test.mjs # pass 1, fail 0, duration_ms ~70 Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Coder <noreply@paperclip.ing>
Stamp commit for fork-build-6. Cumulative content vs upstream 2026.428.0: - PLA-159: cache-bust plugin manifest dynamic import on mtime - PLA-40: redact secrets in plugin run-log chunks at append() boundary - PLA-163: tighten pluginToolDeclarationSchema.name to lowercase alnum allowlist - PLA-323: thread pluginDbId through registerPluginTools - PLA-376: scaffold release-time manifest validation gate - PLA-315: descendant-aware stranded-issue recovery sweep (NEW in fork-build-6) Refs PLA-321 PLA-399 Co-Authored-By: Paperclip <noreply@paperclip.ing>
…rballs (#12) `npm pack` does not apply `publishConfig` (only `npm publish` does), so the fork-build flow shipped tarballs whose inner `package.json` declared `exports → ./src/index.ts` and the host crashed on `import "@paperclipai/server"` at runtime (fork-build-1, 2026-05-08 ~16:11Z). fork-build-2 was unblocked by manually post-rewriting each tarball — fragile and unreproducible from source. Add `scripts/pack-public-packages.mjs` which: - Discovers every public workspace package (same logic as `release-package-map.mjs`). - For each package, deep-merges `publishConfig` into the top-level manifest, packs the tarball, and restores the source `package.json` (even on SIGINT). - Strips registry-only directives (`access`, `registry`, `tag`) from the published manifest, mirroring what `npm publish` does. - Skips the `paperclipai` CLI by default — `scripts/build-npm.sh` already generates a fully-replaced publishable CLI manifest. Diffed against the manually-fixed fork-build-2 tarballs: identical exports, main, types, bin, and files for all 12 overlapping packages (the only difference is a stray `access: public` field in fork-build-2 that the manual rewrite leaked but `npm publish` would have stripped). `scripts/PUBLISHCONFIG.md` documents why the fix lives here and the verification commands. `scripts/pack-public-packages.test.mjs` covers the merge contract: - promotes exports/main/types - drops access/registry/tag - handles missing publishConfig - handles bin overrides (mcp-server pattern) Verification: node --test scripts/pack-public-packages.test.mjs # 4/4 pass node scripts/pack-public-packages.mjs --out /tmp/out tar -xzOf /tmp/out/paperclipai-server-*.tgz package/package.json | jq '.exports' # {".":{"types":"./dist/index.d.ts","import":"./dist/index.js"}} Co-authored-by: Paperclip CEO <ceo@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing>
…als or wake_assignee interactions (PLA-407) (#14) ## CTO review — APPROVE All four acceptance criteria are met cleanly. **AC1 (pending board approval skip)** — `findPendingAssigneeBoardApproval` filters `issueApprovals` joined with `approvals` by `requestedByAgentId === assigneeAgentId` and status in `['pending','revision_requested']`. Skip path is non-destructive: no status flip, no recovery wake, no `stranded_issue_recovery` issue. Verified against the PLA-305/PLA-404 incident shape — the regression test reproduces it exactly. **AC2 (wake_assignee interaction skip)** — `findPendingAssigneeWakeInteraction` filters `issueThreadInteractions` by `status='pending'`, `continuationPolicy='wake_assignee'`, `createdByAgentId === assigneeAgentId`. Covers `request_confirmation`, `ask_user_questions`, and any future kind opting into `wake_assignee`. Scope correct. **AC3 (regression tests)** — Two tests in `heartbeat-process-recovery.test.ts` (one per skip path), both seed the candidate-stranded shape (in_progress, failed run, no live executionRunId, no in_progress children) plus the live waker, run the sweep, and assert: status preserved as `in_progress`, no `stranded_issue_recovery` issues opened, no `issue_continuation_needed` recovery wakes enqueued, and the audit row written. Tests exercise the canonical PLA-305/PLA-404 shape verbatim. **AC4 (telemetry)** — `logActivity` with `action: 'issue.recovery_skipped'`, `details.source: 'recovery.reconcile_stranded_assigned_issue_skipped'`, `details.reason` (`pending_board_approval` or `pending_wake_assignee_interaction`), and `approvalId` / `interactionId` (+ `kind`). Triage handle is structured. **Divergence accepted — telemetry source name uses single dot**: spec illustration was `recovery.reconcile_stranded_assigned_issue.skipped`; PR uses `_skipped` suffix instead. Verified against `server/src/redaction.ts:8` — `JWT_VALUE_RE = /^[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+(?:\.[A-Za-z0-9_-]+)?$/` redacts any 3-segment dot-separated token to `***REDACTED***`. The 2-segment underscore form preserves the discriminator and is consistent with the pre-existing `recovery.reconcile_stranded_assigned_issue` source naming. Sound call. **Position of skip gate** — between `hasActiveExecutionPath` and `isAutomaticRecoverySuppressedByPauseHold`, `in_progress`-only. Matches spec. `todo` branch unaffected. **Risk surface** — minimal. One extra round-trip per `in_progress` candidate; candidate set is small. Worst case: a stale `pending` row briefly suppresses a genuinely-stranded issue, but the audit row makes that observable. **Verification** — vitest 42 passed, recovery-classifiers 4 passed, tsc clean. PR template fully compliant. Merging.
Widens the v1 install-time slot allow-list to accept `page` in addition to
`dashboardWidget`. The page-slot render path is already wired end-to-end
(`PluginPage.tsx`, `App.tsx` mounts, `slots.tsx` dispatch, `PLUGIN_UI_SLOT_TYPES`).
Only the install-time validator floor blocked it.
Tests:
- new positive: page slot with valid routePath
- new positive: page slot without routePath (pluginId fallback)
- new negative: routePath on non-page slot still rejected
- new negative: page slot with non-slug routePath still rejected
- reserved-slot rejection table updated to drop "page" and assert the
new "v1 supports: dashboardWidget, page" message
No new capability — page slots reuse the same trusted same-origin host
contract per PLUGIN_SPEC.md §1. SecurityEngineer signed off on PLA-489
(AC #6 a/b/c). CTO LGTM on PR #15.
Refs: PLA-489, PLA-488, PLA-470
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Stamp version bump for fork-build-7. No source changes — same content as fork-build-6 plus the three commits merged to fork master since the fork-build-6 stamp commit (6f50e92): - a850931 feat(PLA-298): commit publishConfig pack-time merge for fork-build tarballs (#12) - 2e76ee1 fix(recovery): skip in_progress issues parked on pending board approvals or wake_assignee interactions (PLA-407) (#14) - b43a72b feat(PLA-489): add page to PLUGIN_UI_SLOT_TYPES_V1_SUPPORTED (#15) Refs PLA-490 PLA-489 Co-Authored-By: Paperclip <noreply@paperclip.ing>
Stamp version bump for fork-build-8. No source changes — same content as fork-build-7. Re-cut required because fb7 tarballs shipped @paperclipai/* deps as bare semver (not GitHub-Release URLs); operator install ETARGETed. fb8 re-applies the post-pack URL rewrite that fb1–fb6 carried as tribal knowledge, while a follow-up (PLA-497) folds the URL rewrite into scripts/pack-public-packages.mjs so this doesn't recur. Refs PLA-490
Squashing the 5 cherry-picked mock-drift fixes into one merge commit. All test-related verify steps green (Typecheck / Run tests / Build / Registry coverage / Scaffold manifest gate). The verify job conclusion is "failure" only because the final "Release canary dry run" step 403s on a GitHub Trust & Safety "account suspended" error fetching the fork repo — an infrastructure-level blocker tracked separately in PLA-549, not a code regression. Branch protection is off on this fork master (gh api .../branches/master/protection → 404). Admin-merging the test-fix cascade now so PLA-518's PR #17 can rebase onto a green-tests master once PLA-549 resolves the canary auth issue. Closes PLA-522 (test-fix objective). Refs PLA-549 (infra follow-up).
Make PluginHttpClient.fetch pass binary/multipart bodies end-to-end without String() corruption. - SDK: serializeFetchBody handles FormData/Uint8Array/ArrayBuffer/Buffer; base64-tagged in JSON-RPC envelope (utf8 default for legacy workers). - Host: decodeFetchBody mirrors the envelope; req.write receives Buffer for binary, string for text. No String(body) in path. - Tests: SDK vitest 6/6 (string utf8 + Uint8Array exact bytes + Buffer/ArrayBuffer + FormData multipart boundary + caller-supplied Content-Type preserved); QA E2E 3/3 via real http.createServer echo. - Scope: one logical change. klipper untouched (revert tracked on PLA-514). Refs: PLA-518, PLA-516. Co-Authored-By: Paperclip <noreply@paperclip.ing>
PLA-551: gate release-canary dry run on PR runs. No required checks; admin-merging because GitHub Actions queue appears stalled on this fork post-suspension (latest run 10:39Z). The fix itself is the one-line if: gate verified by yaml.safe_load locally.
…uthz (#26) * feat(PLA-574): SDK ctx.artifacts.fetch(attachmentId) helper Add worker-side SDK surface for fetching issue-attachment bytes from inside a tool handler. The worker sends only `{attachmentId, runId}` on the wire — agentId/companyId are host-derived from the dispatch-time runContext registry to keep the worker untrusted. The host returns base64-encoded bytes which the SDK decodes back to a Uint8Array along with `{filename, contentType, byteSize}` metadata. - protocol.ts: add ArtifactsFetchParams / ArtifactsFetchResult types - types.ts: add `artifacts: { fetch(attachmentId) }` to ToolRunContext - host-client-factory.ts: register `artifacts.fetch` as a capability-free host method (tool-dispatch implies attachment-read; host enforces dispatching-agent authorization) - worker-rpc-host.ts: inject `artifacts.fetch` into runCtx before calling the tool handler; reject empty attachmentId at the SDK boundary so no RPC fires - testing.ts: default `runCtx.artifacts` in the executeTool helper to a stub that throws unless the test passes its own mock Co-Authored-By: Paperclip <noreply@paperclip.ing> * feat(PLA-574): host-mediated artifacts.fetch handler with 7 security gates Add server-side handler for the `artifacts.fetch` worker RPC introduced by the SDK in the previous commit. All authorization decisions use the **dispatching agent's** identity (re-derived from an in-memory registry populated at executeTool dispatch) — the worker's JWT is never consulted, so a forged or replayed runId from another tenant fails closed. Seven security gates per SecurityEngineer checklist: 1. RunContext validation — deny-by-default when the composite key `(pluginDbId, runId)` is missing from the registry; no JWT fallback. 2. Dispatching-agent authorization — `dispatchingCompanyId` is read from the registry entry, never from worker-supplied params. 3. Single-resource shape — empty / non-string attachmentId rejected before any DB or storage call. 4. Dual-bucket rate limit — sliding-window 60/min per agent (global) plus 30/min per (agent, attachment-company) sub-bucket. 5. Six-field audit log on every call (allowed and denied) attributed to the dispatching tenant; includes outcome + deniedReason. 6. Typed errors — `runcontext_invalid | forbidden | not_found | rate_limited | too_large`; missing-attachment and cross-company access both collapse to `not_found` (no existence oracle). 7. No bytes / base64 in audit details — only byteSize is logged. Wiring: - plugin-run-context-registry.ts (new): in-memory registry with composite key, TTL sweep (.unref()'d), dispose() - plugin-artifacts-handler.ts (new): the seven-gate fetch handler - plugin-host-services.ts: build artifactsHandler when both storageService and runContextRegistry are supplied; expose via hostServices.artifacts - plugin-tool-registry.ts / -dispatcher.ts: register/deregister run context around each worker executeTool call - app.ts: wire registry into the dispatcher + host services and dispose on process exit Co-Authored-By: Paperclip <noreply@paperclip.ing> * test(PLA-574): SDK wire + 7-gate handler unit coverage Worker→host wire (artifacts-client.test.ts, 3 tests): - Worker sends only `{attachmentId, runId}` on the RPC (no agentId / companyId — trust boundary) - Base64 host response round-trips back to an exact Uint8Array with metadata (filename / contentType / byteSize) - Empty attachmentId throws at the SDK boundary before any RPC fires Host handler (plugin-artifacts-handler.test.ts, 12 tests) — one per gate plus a trust-boundary check that the same runId registered against a different plugin is invisible to ours, and a registry TTL test: - runcontext_invalid (no registry entry, no audit because no identity) - shape rejection (empty / null attachmentId) - happy path — base64 + metadata returned; audit shows six fields; audit details contain no base64 - cross-company → not_found (collapse) with denied audit - missing attachment → not_found (same shape as no-access) - global per-agent rate_limited audit - per-(agent, attachment-company) sub-bucket rate_limited audit - too_large via maxByteSize cap (prevents OOM via base64 inflation) - trust boundary: runId registered against another pluginDbId is invisible to our handler - deregister invalidates immediately (no stale-runContext window) - registry register/get composite key - registry TTL expiry between sweeps Co-Authored-By: Paperclip <noreply@paperclip.ing> * docs(PLA-574): document artifacts.fetch in PLUGIN_SPEC §13.11 Add reference section after §13.10 executeTool documenting: - Wire payload: `{attachmentId, runId}` only (agentId/companyId are host-derived from the dispatch-time runContext registry) - SDK return shape: `{bytes: Uint8Array, filename, contentType, byteSize}` decoded from the host's base64 response - Authorization semantics: company-match against the dispatching agent's tenant; missing-attachment and cross-company access both collapse to `not_found` (no existence oracle) - Rate limits: 60/min per agent (global) + 30/min per (agent, attachment-company) sub-bucket - Six-field audit log on every call (allowed and denied) attributed to the dispatching tenant - Capability requirement: none (tool-dispatch implies attachment-read) - Typed error table for runcontext_invalid | forbidden | not_found | rate_limited | too_large Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix(PLA-574): include toolName in artifacts.fetch audit details (B1) SecurityEngineer review of fork PR #26 flagged that the six-field audit schema documented in the coverage table was actually shipping only five fields — `toolName` was missing from `audit()` details despite the registered runContext entry carrying it. This is the lone blocker on the conditional security sign-off. Fix is a one-line pull-through: - audit() input type gains a required `toolName: string` - every call site reads `ctx.toolName` from the registry entry - happy-path test now asserts `details.toolName: "lookup-screenshot"` - Gate 4 cross-tenant deny test also asserts it (deny paths must carry toolName too — that's the field that lets a security analyst answer "which tool reached out for this attachment" without joining runId) This satisfies the six-field schema: attachmentId, attachmentCompanyId, dispatchingAgentId, dispatchingCompanyId, pluginInstanceId (pluginDbId), toolName Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
Stamp version bump for fork-build-9. Carries cumulative fork-master
commits since fb8:
- feat(PLA-574): SDK ctx.artifacts.fetch + host-mediated cross-tenant
authz handler (#26, 5fe77d1) — primary deliverable for this cut.
- ci(PLA-551): skip release-canary dry run on PR runs (#24, 11b4c0e)
- fix(PLA-518): SDK PluginHttpClient binary/FormData body support
(#17, 4387d34)
- fix(PLA-522): repair fork-master test-mock drift cascade (#22,
458cfd1)
PLA-498 codification (rewriteForkBuildDeps in pack-public-packages.mjs)
has not landed on fork master as of fb9, so this cut re-uses the one-off
URL-rewrite shim pattern (same as fb1-fb8). Once PLA-498 lands, fb10+
will use the codified path.
Refs PLA-587, PLA-574, PLA-585.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Hygiene-merge under CEO authorization on PLA-575 (option (a) approved in comment 44f2d244).
Red e2e on this PR is tracked separately in PLA-597 (fork e2e flakes — Postgres deadlock during signoff PATCH status:done + playwright install hang). e2e failure is in signoff-policy.spec.ts:333/372, no overlap with server/src/middleware/error-handler.ts. Hygiene-merge under CEO authorization on PLA-575.
CI on this PR: policy ✅, verify ✅, e2e ❌ (three distinct flakes across attempts 1/3/4, all outside diff surface).
Change:
- server/src/middleware/error-handler.ts: 4xx pass-through for numeric err.status/err.statusCode; dedicated 413 response shape ({error, code, limit}) + warn log with {route, method, contentLength, limit, type}; 5xx numeric still routed to existing telemetry attach.
- server/src/__tests__/error-handler.test.ts: extended to 5 unit cases.
- server/src/__tests__/error-handler-413-integration.test.ts: supertest + express.json({limit:"1kb"}) end-to-end.
PLA-573 acceptance §4 ("413 surfaces cleanly") satisfied once the next host fork-build (fb10) carries this commit.
…lock retry + runbook (#28) * fix(PLA-597): retry PATCH /issues/:id on transient Postgres deadlock The signoff-policy e2e (signoff-policy.spec.ts) intermittently 500s on `PATCH /api/issues/:id` with PostgresError 40P01 deadlock detected on `heartbeat_runs`. Two concurrent transactions — the issue PATCH and the heartbeat run-lifecycle path — take overlapping locks on the issue row and the heartbeat_runs row in different order, so Postgres aborts one. The rollback leaves no partial state, so a bounded retry from the app recovers cleanly. Adds services/pg-retry.ts: a `retryOnTransientPgError` helper that recognises 40P01 (deadlock_detected) and 40001 (serialization_failure), sleeps with exponential backoff + jitter, and rethrows after maxAttempts (default 4). Logs each retry at warn with the call site label so flakes remain visible in server.log. Wraps both branches of the PATCH /issues/:id transaction block in routes/issues.ts so the route is no-op when the transaction succeeds first try and bounded-retries when Postgres throws a transient error. Adds 9 unit tests covering retryable vs. non-retryable codes, retry success after N failures, and exhaustion. Co-Authored-By: Paperclip <noreply@paperclip.ing> * ci(PLA-597): split playwright install to bound apt-deps hangs `npx playwright install --with-deps chromium` runs the 167 MiB browser download and a `sudo apt-get install` of system deps as a single step. When apt/dpkg hangs (network blip, mirror flake) the combined step silently eats the full 30-minute e2e job budget — the Chrome zip is already on disk, but there is no signal in the logs and no per-step timeout to bound it. Split into two timeout-bounded steps in pr.yml and e2e.yml: - "Download Playwright browser (chromium)" — `playwright install chromium`, capped at 5 min - "Install Playwright system deps (chromium)" — `sudo playwright install-deps chromium`, capped at 10 min An apt hang now fails the deps step in ≤10 min with a clear name, so re-runs are quick and triage is unambiguous. No functional change to the e2e job. Co-Authored-By: Paperclip <noreply@paperclip.ing> * docs(PLA-597): document CI rerun policy for infra flakes PR #25's e2e check failed twice in a row with two unrelated infra shapes (Postgres deadlock, playwright apt-deps hang), neither caused by the PR's change surface. We had no written policy on when reruns are allowed vs. when an infra-shaped failure should be treated as a real bug. Adds a short subsection to CONTRIBUTING.md "Tests Must Pass": 1. A single rerun is allowed for infra-only failures; record the original failure on the PR thread first. 2. Two failures in a row of the same step = real failure; file a follow-up issue, do not retry a third time. 3. Never disable a check to land a PR. Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix(PLA-597): pin CI workers=1 + cache playwright browser + add runbook CTO directive on PLA-597 sharpened the bar from "bounded retry" to "deterministic test isolation" plus "pinned/cached browser install". This commit lands the three missing pieces; the retry helper from 856dfd6 stays as defence-in-depth. * tests/e2e/playwright.config.ts: `workers: process.env.CI ? 1 : undefined`. Cross-spec parallelism is what surfaces the issues <-> heartbeat_runs lock-order race. Pinning to one worker on CI makes spec files run strictly sequentially against the shared webServer. Local runs are unaffected; the suite stays fast for developers. * .github/workflows/{pr,e2e}.yml: add `actions/cache@v4` on ~/.cache/ms-playwright keyed by hashFiles('pnpm-lock.yaml'). The browser-download step is now `if: cache-hit != 'true'`, so the common path is a no-op and the network dependency on cdn.playwright.dev is removed entirely for cache hits. apt-deps install keeps its 10-min bounded timeout. * doc/runbooks/pla-597-e2e-flake.md: full runbook entry covering symptoms, both root causes (heartbeat_runs/issues lock-order race; bundled --with-deps step), the four landed fixes, verification gates, and a local-repro recipe for future regressions. Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
The first workflow_dispatch of `e2e.yml` against master @ `6ed85ef` (the
PLA-597 merge commit) failed in 5 seconds at the `pnpm/action-setup@v4`
step with:
Error: Multiple versions of pnpm specified:
- version 9 in the GitHub Action config with the key "version"
- version pnpm@9.15.4 in the package.json with the key "packageManager"
Remove one of these versions to avoid version mismatch errors
`package.json` has been pinned to `pnpm@9.15.4` via `packageManager` for
a while, and `pr.yml` does not pass an explicit `version:`. `e2e.yml`
still had `version: 9` from before the `packageManager` field landed,
and never hit this because the workflow is `workflow_dispatch`-only.
Drop the `version:` key so pnpm/action-setup@v4 reads `packageManager`,
matching `pr.yml`. No version change in practice — both pre/post resolve
to pnpm 9.15.4 — just removes the conflict that blocked the
PLA-597 3-green validation gate.
Co-authored-by: Paperclip <noreply@paperclip.ing>
…rk-build-10) (#27) Re-applies the PLA-39 stopgap that lets `claude_local` agent JWTs reach `POST /api/plugins/tools/execute` and `GET /api/plugins/tools`. The patch was applied to a live install in v2026.428.0 but did not survive the fork-build-9 rebase (PLA-43, the upstream PR companion, was cancelled). Two-part patch: 1. Actor-branch guard on both routes (mirrors PLA-39): - `POST /plugins/tools/execute`: agent path calls `assertCompanyAccess(req, body.runContext.companyId)` instead of `assertBoardOrgAccess`. - `GET /plugins/tools`: agent path scopes to JWT companyId. - Board callers keep the existing guard. 2. Body-shape adapter for v0.1.6 wake-comment clients: v0.1.5 sends `{tool, parameters, runContext}`; v0.1.6 sends `{name, parameters, runId}`. Route now accepts both, remapping `name → tool` and synthesising `runContext` from JWT claims (`projectId: "onboarding-fallback"`) for the v0.1.6 shape. Defence-in-depth unchanged: `validateToolRunContextScope` still cross-checks the run/agent/project tuple; `plugin-capability-validator` still requires `agent.tools.execute`. SE approved on PLA-595. Six new test cases in `plugin-routes-authz.test.ts` covering both shapes, cross-company 403, missing-runContext 400, and a board-no-access regression. 25/25 pass under `pnpm exec vitest run`. Empty `ci(PLA-594):` commit (fa958e6) rotates the head SHA to clear a GH Actions rerun-state hang on `Install Playwright`; CI infra flake analysis on PLA-601 (heartbeat FK + signoff deadlock signatures, both in tests with zero overlap with this diff). Refs PLA-39, PLA-576, PLA-585, PLA-593, PLA-594, PLA-595, PLA-601, DPR-130.
* fix(PLA-597): wrap pre-auth lock helpers in deadlock retry PLA-597 #28 wrapped the PATCH /issues/:id transaction block in `retryOnTransientPgError`, but the first post-merge gate dispatch on `776a972` still saw `signoff-policy.spec.ts:333` fail red with `PATCH /api/issues/<uuid> 500 — deadlock detected` and the test's `expect(changesRes.ok()).toBe(true)` missing. Crucially, the WebServer log shows the deadlock error but NO `label=patch_issue` retry trace — i.e. the retry never engaged. Root cause: every mutation route calls `clearOrphanCheckoutLocksIfTerminal` (and many call `clearExecutionRunIfTerminal`) BEFORE entering the wrapped transaction block. These helpers themselves run a `db.transaction` taking overlapping locks on `issues` then `heartbeat_runs`, so they hit the same 40P01 race against the heartbeat-run lifecycle — but the deadlock fires before any of the route's own retry can see it. Fix: wrap the helpers' transaction bodies in `retryOnTransientPgError` internally. All four mutation-route call sites (`routes/issues.ts:1932,2750,2826,3423` for clearOrphan, plus the cancel/release paths in services/issues.ts for clearExecutionRunIfTerminal) get retry-on-deadlock for free; no caller changes needed. Also bump `retryOnTransientPgError`'s default `maxAttempts` from 4 to 6 (≈1.6 s worst-case latency including jitter — still bounded well under any user-visible timeout) because the CI failure showed single-attempt-then-success was sometimes still losing the race against the sweeper. Test coverage: `pg-retry.test.ts` adds a 10th case asserting the default-attempts contract. Server typecheck baseline preserved (4 pre-existing errors in plugin-host-services.ts, unrelated). Route tests (environment-selection-route-guards, issue-comment-cancel-routes, issue-execution-policy-routes) pass — the helper wrap is transparent to existing mocked callers. Co-Authored-By: Paperclip <noreply@paperclip.ing> * docs(PLA-597): note wrap-at-helper pattern per CTO endorsement CTO endorsement of PR #31 (comment 47027caa) requested a one-liner in the runbook documenting why this fix's wrap layer differs from #28's, so the pattern sticks for future deadlock-retry work. Adds a Pattern Note to the "Fix landed" §1 explaining: - Wrap goes at the helper that opens db.transaction, not the route - Wrapping a layer above only retries the layer's own transaction - Updates the attempts default reference (4 → 6) to match shipped code Cheap to bundle pre-merge; no functional change. Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
…either posture list (#178) The `unqualified-mutation-security-posture-column` rule only ever guaranteed "the columns someone remembered to register". It shipped with the two `company_secret_bindings` columns that migration 0138 actually flattened, so every other posture column in the schema was silently unchecked -- the same absent-means-unprotected shape that let `full-table-mutation-large-table` miss 0138 in the first place. Two changes: one closes the gap, one keeps it closed. 1. Sweep the schema. Every candidate column is classified register / do-not-register, taking the registry from 2 pairs to 49 across 27 tables. Rejections record the reason they were rejected, so a later reviewer does not have to re-derive the analysis. `doc/SECURITY-POSTURE-COLUMN-SWEEP.md` carries the narrative; the lists are authoritative. 2. Make the classification total. `SECURITY_POSTURE_REJECTIONS` is machine-readable and reason-required, and `assertSchemaColumnsClassified` is wired into `runMigrationSafetyCheck`, so a column added to the schema later that lands in neither list fails the lint instead of quietly going unchecked. That closes the additions-side drift the existing resolvability check does not cover. What the sweep found, beyond the egress pair: - `NULL` is the permissive value far more often than `false` is. `revoked_at` is matched with `isNull(...)`, so an unqualified `SET revoked_at = NULL` resurrects every revoked agent key, board key, secret version and invite. `board_api_keys.expires_at` short-circuits on `!expiresAt`, so NULL means never expires. - Several checks are written as inequalities against the restrictive value: `membership_role !== "viewer"` passes for NULL, so clearing the column promotes every viewer to issue-mutate, `runtime:manage` and `secrets:read`. An empty `principal_permission_grants.scope` is read as unconstrained. - `source_trust` is the indirect-prompt-injection boundary -- NULL means trusted, and it is what keeps quarantined low-trust content out of higher-trust agents' prompt context. Registered on all five tables. - `agents.adapter_config` carries `dangerouslySkipPermissions`, which defaults to `true` when the key is absent, so clearing the object re-arms the bypass on agents that had been explicitly hardened. - `feedback_exports.status` is the sole gate on shipping a captured trace off-instance; the two consent flags that look like the control are records, not gates. - Four credential-hash columns of one shape: material whose hash is the control. `cli_auth_challenges.pending_key_hash` is the sharpest -- it is copied verbatim into `board_api_keys.key_hash` when a CLI challenge is approved, so flattening it makes the next approval mint an operator-scope board key whose token the attacker already holds. It has no unique index, so it is strictly more exposed than the column that was registered on its behalf. Every `reason` names the dangerous direction, because for most of these the permissive value is also the column default -- which is what makes an unqualified write look innocuous in review. Tests assert the properties that keep the registry honest: every pair names a column that exists in the drizzle schema (a typo does not fail loudly, it silently un-registers the column), every pair actually produces a finding, an unregistered column on a registered table still does not fire, and the totality check fails closed rather than passing vacuously on an empty schema or an empty rejection list. Each discriminates -- deleting the guard it defends makes it red. Two existing tests assumed every registry entry lived on `company_secret_bindings`; the multi-table TRUNCATE case now asserts one finding per table, which is the rule doing more work rather than less. The lint stays green: re-running it over all historical migrations surfaced no new findings, so nothing was baselined and no historical posture flatten needed escalating. Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…ping paperclip-dev-secret Implements the remediation decided in PLA-2132. ## Changes - AC1: Commented out BETTER_AUTH_SECRET in .env.example with generation recipe - AC2: Normalize empty/whitespace case in better-auth.ts (use trim() ||) - AC3: Reject known-weak values in authenticated mode via validateAuthSecretStrength() - AC4: Rewrite error message to recommend openssl rand -hex 32 - AC5: Surface strength check in paperclipai doctor - AC6: Fix three test fixtures with strong secrets - AC7: Add 14 regression tests for auth secret validation Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…lidation Reject known-weak BETTER_AUTH_SECRET in authenticated mode; stop shipping paperclip-dev-secret
* Harden boot migrations against out-of-band content swaps A production server re-ran migrations on boot because the installed package's migration files were swapped while the version string stayed identical, and nothing surfaced the mismatch. Close that path: - Preflight visibility: before applying anything, report the pending set with a sha256 per file, and log a loud WARN when a pending migration's file name is already recorded in __drizzle_migrations under a different content hash (the identity-drift signature of a swapped file). Add a --dry-run/--check mode to the migrate CLI that reports pending work and exits non-zero without applying. Drift is correlated by ordinal against the journal, so no schema change to __drizzle_migrations is needed; an orphaned-hash guard avoids false positives on reconcile/deletion paths. - Source-tree production guard: the default migrate/preflight connection now refuses a populated (production-shaped) cluster and fails closed unless PAPERCLIP_ALLOW_PROD_MIGRATE is explicitly set. - Apply audit: boot-time migrations write a durable row (count, per-file hash, binary version) to a dedicated audit table in the drizzle schema, best-effort so it never crashes an otherwise-migrated server. Tests exercise all three via embedded Postgres: drift fires on a swapped hash and stays silent on a legitimate deletion, the audit row is durable, and the source-tree guard refuses a populated cluster but allows an empty one and honors the opt-in. Co-Authored-By: Paperclip <noreply@paperclip.ing> * test(server): stub inspectMigrationPreflight in startup db mock The boot-migration preflight is imported and invoked by startServer, so the @paperclipai/db module mock in the startup wiring test must expose it or vitest fails the whole suite on the missing export. Co-Authored-By: Paperclip <noreply@paperclip.ing> * Harden migration drift detection with a separate file-identity table Address the security review on the boot-migration drift guard: - Drift detection no longer relies on ordinal correlation against the Drizzle journal, which a scrubbed or renumbered journal row could defeat. Each applied migration now binds its file name to the content hash it carried in a dedicated `drizzle.migration_file_identity` table, written in the same transaction as the journal entry and kept out of the dedup path. Drift becomes an exact per-name lookup that survives journal scrubs. - Distinguish "verified clean" from "cannot verify": a pending, previously applied file with no recorded identity (a cluster predating identity tracking) is surfaced as `unverifiable` and warned on, not folded into an empty-drift "clean" result. - Rescope the source-tree production guard: it is applied only from the `db:migrate` apply path (read-only `db:status`/`--dry-run` are ungated), discriminates on an explicitly configured external `postgres` target (never the embedded dev cluster), and requires `PAPERCLIP_ALLOW_PROD_MIGRATE` to name the exact target database rather than accepting a bare truthy value. - Record file identity and a durable audit row for the empty-database bootstrap path, which previously left no attribution. - Harden the audit writer: parameter-bind the payload (jsonb array, not a double-encoded string), prefer the on-disk package version over the environment and record any `PAPERCLIP_VERSION` override separately, and log rather than swallow a failed audit write. - Document the guardrails and opt-in semantics in doc/DATABASE.md. Regression test reproduces the incident: a content swap plus one scrubbed earlier journal row. This is swallowed by the old ordinal detector and is now detected via the identity table. Co-Authored-By: Paperclip <noreply@paperclip.ing> * Harden migration drift detection with a frozen identity watermark Add a `migration_identity_watermark` table that records the journal row count at the instant identity tracking begins. This replaces the live `count(*)` comparison used to classify pending files as previously-applied. The live count fails when a journal row is deleted (a re-apply or manual scrub): it shrinks, reclassifying the newest applied migration as "beyond the applied range" and hiding a swap of exactly the file most likely to be swapped. The watermark is frozen on first write, so migrations applied before identity tracking began are permanently unverifiable; migrations authored after tracking began are expected to carry an identity entry and are checked. A cluster with no identity table at all now reports every pending file as unverifiable rather than inferring anything from ordinals. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…oard_key writes (#177) * feat(activity-log): record board-credential provenance and alert on board_key writes activity_log could not distinguish an operator clicking in the dashboard from an agent presenting the operator's board API key: run_id is only set when the caller sends X-Paperclip-Run-Id, and details carried no auth source or credential id. actorMiddleware already resolves req.actor.source and req.actor.keyId; this simply stops dropping them before the log. - Add nullable activity_log.actor_source (credential class) and activity_log.actor_key_id (board API key id) columns (migration 0145, metadata-only ADD COLUMN, safe on the ~1.1M-row table). - Capture provenance centrally via an AsyncLocalStorage context set by a new middleware right after actorMiddleware, read in logActivity — no edits to the ~179 call sites. Background work (no request) records null, which is itself meaningful. - Emit a distinguishable warn log (event=board_key_authenticated_write) for every board_key-sourced write. The token value is never logged; keyId (a UUID) is the only credential id emitted. - Register the actor + provenance middleware pair through one shared registerActorContext helper used by both createApp and the AC5 regression test, so dropping the provenance registration turns the test red instead of letting production silently log NULL. The test drives real board-key and session HTTP requests through that chain and asserts distinct provenance rows, with two negative controls (provenance removed; run before actorMiddleware). - Not retroactive: pre-existing null-run_id rows stay unattributable by design. Co-Authored-By: Paperclip <noreply@paperclip.ing> * Apply PLA-2209 security-gate remediations (AC2 ruling) - Strip actorKeyId from publishLiveEvent payload (live-event stream is readable by any agent API key scoped to company; DB keeps the value) - Demote board-key log line from warn to info (board-key writes are routine at ~10/min/company, not anomalies) - Fix doc comments: actor_key_id holds board or agent API key id per actor_source, not board-only - Delete unused runWithActorProvenance (zero callers; new test doesn't use it) Co-Authored-By: Paperclip <noreply@paperclip.ing> * Add actor_source and actor_key_id to security-posture column registry These activity_log columns record credential provenance and are security controls: clearing them obscures which credential authenticated the write. Co-Authored-By: Paperclip <noreply@paperclip.ing> * Apply PLA-2201 security merge conditions C1-C3 C1: Remove actorSource/actorKeyId from live-events payload (broadcast to agents) - Dashboard reads provenance from GET /api/companies/:companyId/activity - Live events are for agents; DB row carries full provenance C2: Populate provenance on POST /api/companies/:companyId/activity - Add actorSource/actorKeyId from getActorProvenance() - Schema strips unknown keys (prevents forgery) C3: Add regression tests for C1, C2, and AC4 alert - Test live events exclude provenance fields - Test activity route writes provenance - Test schema strips actorSource (forgery prevention) - Test AC4 alert fires on board_key writes Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix(db): remove duplicate closing brace in security-posture-columns.ts The extra `},` at line 100 created a syntax error that broke the build. This was a typo from an earlier edit. Co-Authored-By: Paperclip <noreply@paperclip.ing> * Apply PLA-2209 security remediations to live-event payload Add actorSource to publishLiveEvent payload per PLA-2209 ruling. The payload now carries actorSource but NOT actorKeyId, satisfying least-privilege: the live-event stream is readable by any agent API key scoped to the company, while the DB row retains both fields for authenticated audit reads. Updated C1 regression test to expect actorSource in the payload. Log level already at info, doc comments already correct. Co-Authored-By: Paperclip <noreply@paperclip.ing> * Remove internal PLA ticket id from test comment Fork policy job rejects PLA-ids in code. Restate the security ruling context in plain English instead. Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
…-status-change async writes Root cause: heartbeatRunEvents (with NOT NULL FK to agents) is written to by the heartbeat service AFTER setRunStatus transitions the run to terminal. The test helper waitForHeartbeatIdle only polls heartbeatRuns.status for not-queued/not-running, so it returns immediately after the run transitions to failed/succeeded, but before async post-status-change side effects (e.g., appendRunEvent at heartbeat.ts:10288) complete. Fix: Add 150ms quiescence period after waitForHeartbeatIdle in test tearDown to allow all async side effects to complete before starting the ordered DELETE chain. This prevents the race where: 1. waitForHeartbeatIdle returns (run status = terminal) 2. Teardown deletes from heartbeatRunEvents 3. Heartbeat process writes to heartbeatRunEvents with agentId FK 4. Teardown attempts delete(agents) and fails FK constraint Files modified: - server/src/__tests__/heartbeat-workspace-branch-containment.test.ts: Add 150ms quiescence - server/src/__tests__/heartbeat-workspace-finalize-branch.test.ts: Add 150ms quiescence - server/src/__tests__/heartbeat-dependency-scheduling.test.ts: Increase quiescence from 50ms to 150ms Stability verification: All 12 tests across 3 files pass consistently. Sibling audit: - heartbeat-process-recovery.test.ts: Already has 100ms quiescence pattern (no change needed) - heartbeat-stale-queue-invalidation.test.ts: Uses TRUNCATE CASCADE with retry (robust) - heartbeat-worktree-suppression.test.ts: Uses TRUNCATE CASCADE via resetEmbeddedPostgresTestDatabase (robust) - issue-monitor-scheduler.test.ts: Custom waitForHeartbeatSideEffectsSettled with retry (no change needed) - heartbeat-archived-company-guard.test.ts: Tests archived companies don't run (no actual execution) - server-startup-feedback-export.test.ts: Not a heartbeat test (no DB tearDown) Fixes PLA-2319. Fork-target: claudegoogl-sudo/paperclip (upstream-PR freeze in effect per PLA-585). Co-Authored-By: Paperclip <noreply@paperclip.ing>
…rdown-PLA-2319 Fix flaky teardown in heartbeat tests (heartbeatRunEvents FK race on delete from agents)
…secret # Conflicts: # cli/src/checks/deployment-auth-check.ts # packages/shared/src/auth-secret.ts # server/src/__tests__/better-auth-secret.test.ts # server/src/__tests__/server-startup-feedback-export.test.ts # server/src/auth/better-auth.ts
…et-both-key-validation Fix FINDING-1 mirror bypass: validate BOTH auth signing keys
…ot warning for open sign-up, fix audit provenance logging (#190) * security: PLA-2235 fixes (A-C) - preserve egress allowlist across binding reconcile, emit boot warning for open sign-up on authenticated deployments, fix audit provenance logging - A (Medium): Preserve allowedEgress and egressAllowlistEnforced across binding reconcile in syncSecretRefsForTarget and writeBindings (syncEnvBindingsForTarget). Previously, delete+insert would reset these to schema defaults ([] and true), silently destroying operator decisions on config save. - B (Low): Emit boot warning when deploymentMode is "authenticated" and authDisableSignUp is false (open sign-up reachability concern). - C (Low): Fix audit provenance logging - unknown/unset actor sources must not default to "session". Only explicitly "session" sources log as session; all others log as their raw value or "unknown". Includes three regression tests per acceptance criteria: - AC-A: Two tests verifying egress allowlist preservation across both reconcile paths - AC-B: Test documenting boot warning behavior for open sign-up - AC-C: Test verifying unknown sources don't masquerade as session Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix: correct PLA-2235 test - use valid wildcard pattern and correct env binding field - Use *.internal.example instead of *.internal (wildcards need full TLD) - Use secretId field instead of key in env binding Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix: query by configPath instead of ID after reconcile (ID changes on delete+insert) Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix: import and from drizzle-orm for test queries Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix: revert accidental change to process.env.PAPERCLIP_RUNTIME_API_URL Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix: remove internal ticket IDs from test comments and names The fork's PLA-1836 source hygiene gate rejects PLA-xxxx references in public-repo diffs. Removed PLA-2235 from test descriptions to pass the policy check. Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix: type-safe audit provenance logging - use local_implicit fallback The actor source type only accepts specific union members, not arbitrary strings. Changed the fallback from "unknown" (not in type) to "local_implicit" (a valid, low-privilege source) with a type assertion to satisfy TypeScript. Updated test expectations accordingly. Security behavior unchanged: unset sources still don't log as "session", but now use a type-safe fallback. Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
Add section to CONTRIBUTING.md explaining that branches must be up to date with master before merge, with the command to update branches via gh CLI. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Extend the verify job to assert e2e and verify_serialized_server results. Both jobs only run when policy succeeds, so a skipped result means the gate itself failed and should fail the overall check. Change: - Add verify_serialized_server and e2e to verify.needs - Add plain equality assertions for both lanes in the verify step Co-Authored-By: Paperclip <noreply@paperclip.ing>
CI: gate e2e and verify_serialized_server in required verify check
…teral-secret gate Bind published compose/quadlet/docker-run ports to loopback, require an explicit POSTGRES_PASSWORD instead of shipping a known one, and close fail-open bypasses in the literal-secret compose checker (vacuous-pass on missing stacks, and marker honoured only in the comment portion). Adds tests covering the URL rule line-wide behaviour and the two gate-integrity fixes.
…history prune (#187) * fix(costs): denormalise run_identifier and project_id to survive run-history prune Migration 0143 relaxed cost_events.heartbeat_run_id to ON DELETE set null, which silently degrades two cost reports for windows reaching past the retention cutoff: 1. byAgent computes run counts as count(distinct heartbeat_run_id). NULL run ids (post-prune) read zero, underreporting run activity. 2. byProject joins on heartbeat_run_id to find project_id via activity_log. NULL run ids make the join miss, dropping the row entirely from totals. This commit adds a stable run_identifier column (TEXT, NOT NULL) that stores the run ID independently of the FK, and ensures projectId is populated at write time (heartbeat.ts already did this; the migration backfills existing rows). Queries now use these denormalised columns instead of the volatile FK reference. Changes: - Add run_identifier column to cost_events schema (TEXT, NOT NULL) - Migration 0145: add column, backfill from heartbeat_run_id, set NOT NULL - Migration 0145: backfill projectId from activity_log where resolvable - Update heartbeat.ts to populate run_identifier at write time - Update byAgent, byProvider, byBiller to count distinct run_identifier - Simplify byProject to use cost_events.projectId directly (no CTE/join) - Add test: run counts and project totals survive run prune Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(costs): make run_identifier nullable for backwards compatibility - Remove NOT NULL constraint from run_identifier schema definition - Update migration to skip SET NOT NULL step - Add missing eq import from drizzle-orm The NOT NULL constraint was breaking tests that insert cost_events directly via db.insert() rather than through costs.createEvent(). Making run_identifier nullable allows for historical rows where heartbeat_run_id was already NULL and provides flexibility for different code paths. The service layer still provides fallback logic: runIdentifier: data.runIdentifier ?? data.heartbeatRunId ?? randomUUID() Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix(db): register migration 0146 in journal The 0146 migration file was added but the journal entry was missing, causing the migration count check to fail with "journal has 144, files have 145". Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix(db): classify cost_events.run_identifier in security posture The schema-column classification guard requires every schema column to appear in either SECURITY_POSTURE_COLUMNS or SECURITY_POSTURE_REJECTIONS. Adding run_identifier to cost_events without classifying it would fail the build. Classify it as rejected with the same table-level reasoning as heartbeat_run_id: cost_events is per-invocation telemetry, not a security predicate. Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…rAllMocks (#194) Root cause: clearAllMocks clears call history but preserves mock implementations. Tests that override mock behavior (mockResolvedValueOnce, mockImplementation) leak those implementations to subsequent tests, causing order-dependent failures. Changed vi.clearAllMocks() to vi.resetAllMocks() in three confirmed flaky test files: - ui/src/components/AgentActionButtons.test.tsx - ui/src/pages/CompanyEnvironments.test.tsx - ui/src/pages/Secrets.render.test.tsx resetAllMocks clears both call history AND mock implementations, ensuring each test starts with fresh mock state. Verification: All 277 UI test files pass (2076 tests). Ran 5 iterations of affected tests to demonstrate determinism - all passed consistently. Co-authored-by: Paperclip <noreply@paperclip.ing>
…tdout (#195) When the CLI emits a clean structured result envelope, detectClaudeLoginRequired now scans only structured fields (parsed.result + CLI errors[]). The previous implementation also regex-scanned raw stdout/stderr, which for an agent run is the full stream-json transcript — so any tool result, file the agent read, or agent prose matching the auth regex would flip requiresLogin to true and classify a clean success (is_error:false, subtype:success, non-zero teardown exit) as claude_auth_required. That burned slot-hours recording completed work as failure and spawned retries that re-did the work. The !parsed branch keeps raw-text scanning because no other signal exists there. Audit of the two adjacent classifiers: - isClaudeUsageLimitResult: already scoped to parsed.result + parsed.errors[]. - isClaudeNoWorkResult: already scoped to parsed.total_cost_usd + parsed.num_turns. Neither takes stdout/stderr; both were unaffected and are unchanged. Adds two positive-control regression tests (parse.test.ts unit level + execute.classification.test.ts end-to-end through execute()) that fail if the fix is reverted, plus a genuine-auth-failure test confirming the structured detection path still flags auth text in parsed.result/errors[]. Retry policy is intentionally untouched. Co-authored-by: Paperclip <noreply@paperclip.ing>
… atomic rename
* fix(db): compress backups to temp file with ISIZE verification before atomic rename
Implements all acceptance criteria for backup integrity verification:
- AC1: Write gzip stream to ${backupFile}.partial, verify, then renameSync()
- AC2: Verify uncompressed size by reading gzip ISIZE (last 4 bytes, little-endian)
with fallback to streaming byte counter for files ≥4GB where ISIZE wraps
- AC3: On verification failure, keep raw .sql, write db-backup-to-s3.failure marker, throw
- AC4: Apply same temp-then-rename treatment to pg_dump path with content verification
- AC5: Exclude unverified archives, .partial files, and .sql files from retention keep slots
- AC6: Health check now reports uncompressed size and warns on ISIZE=0 archives
- AC7: Mirror AC5 in host-disk-janitor - unverified archives never occupy keep slots
- AC8: Add tests for empty/truncated detection, ≥4GB ISIZE wrap, and retention behavior
This prevents killed compression jobs from leaving final-named empty .gz files
that health checks report as "ok" and retention can promote into long-term slots.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
* fix(janitor): restore classifyBackups purity; export pruneOldBackups; cover AC7 unit tests
- classifyBackups no longer touches the filesystem. AC7 verification moves
to run(), which populates entry.verified from isArchiveVerified(). This
restores the original pure-function contract and unbreaks the 2 existing
classifyBackups unit tests that used synthetic entries with no backing
files. `verified === undefined` defaults to verified so callers and tests
that do not care about content checks keep working unchanged.
- readGzipIsize now guards against files smaller than the gzip trailer.
- pruneOldBackups is now exported and counts unverified archives it deletes
in its return value, so AC5 callers can observe the work.
- Health route's warning-code label map picks up database_backup_content_empty.
- New unit tests: classifyBackups AC7 keep-slot exclusion + undefined-verified
default; both verified by `node --test ./scripts/host-disk-janitor.test.mjs`
(34/34 pass) and `vitest run src/backup-lib.test.ts` (10/10 pass).
Co-Authored-By: Paperclip <noreply@paperclip.ing>
---------
Co-authored-by: Paperclip <noreply@paperclip.ing>
Anonymous webhook ingestion starves real provider traffic at 2 req/s. Add an optional auth: { type: "header-token", header, tokenDigestConfigKey } declaration, gated on a new webhooks.verify capability. The host holds HMAC-SHA256(key=salt, message=token) in instance config and compares it constant-time against the named header before the delivery insert and the worker RPC. A match bills the delivery to a separate 600/min budget with its own hit store; a mismatch falls through to the existing anonymous 120/min budget unchanged (fail-open semantics bound starvation without claiming to authenticate the caller).
Token mint uses crypto.randomBytes(16) -> 22-char base62 (128-bit floor); salt >=16 chars, unique per endpoint, host-minted. Verified tier exempt from the per-IP bucket (avoids starvation via shared loopback under unset TRUST_PROXY). Header-name validation, repeated-header-as-absent, log hygiene, and budget isolation all covered.
Security review: APPROVED by SecurityEngineer. Construction paragraph and code comment carry the correct length-extension reasoning (does not apply; concat ambiguity is the actual justification).
Co-Authored-By: Paperclip <noreply@paperclip.ing>
…x-rows prune (#196) The anonymous webhook ingestion path (POST /api/plugins/:pluginId/webhooks/:endpointKey) persists every inbound payload + headers as JSONB, and nothing deleted the rows afterwards. On a busy host the table grew without bound; the documented ingestion ceiling works out to ~169 GB/day, which an age-only retention cannot bound. This ships a batched, short-transaction prune that mirrors the existing run-history-retention pattern, with two distinct age bounds (success: 3 days junk; failed: 30 days incident audit trail) plus a load-bearing max-rows cap that evicts oldest-first when exceeded (success before failed). Pending rows are in-flight and never pruned. A new migration 0147 installs partial indexes that keep every prune predicate off a Seq Scan. Co-authored-by: Paperclip <noreply@paperclip.ing>
Remove the legacy raw-master verification fallback from verifyLocalAgentJwt and delete the PAPERCLIP_AGENT_JWT_DISABLE_LEGACY_FALLBACK escape hatch. Tokens signed with the shared master secret are now rejected under the default configuration; only signatures bound to the per-instance, per-company derived key validate. The fallback was instance-agnostic, so any token forged with the shared master secret would have validated across control-plane instances. Live verification confirms every agent JWT is now minted exclusively under the derived key with a 1h TTL, so the legacy token window has long since expired and the fallback can be removed without an opt-out. Tests: - reject raw-master-signed token under default config (regression) - keep per-company derived-key happy-path test green - keep cross-instance rejection test green Co-authored-by: Paperclip <noreply@paperclip.ing>
…198) Add a "Where merged-state verification happens" subsection under the existing Tests Must Pass heading. Explains that pre-merge verification is the PR workflow, while post-merge verification of the merged result is the verify_canary job in the Release workflow, which runs on every push to master. Lists exactly what verify_canary runs (release package-map check, pnpm -r typecheck, pnpm test:run, pnpm build) and calls out that pnpm test:run defaults to mode all, covering both the general groups and the serialized server suites — which is why there is no duplicate push-triggered test job on master. Also lists what it does NOT run (PR policy job diff-based static gates, release registry test coverage, upstream-sync idempotency, scaffold manifest-validation gate, e2e) so nobody assumes verify_canary mirrors PR CI. Closes with the P0/severity contract for a red Release run on master and the bisect-from-last-green attribution rule. A contributor asking "does anything test master after a merge, and where do I look when it breaks?" now finds the answer in CONTRIBUTING.md without reading a single workflow file. Co-authored-by: Paperclip <noreply@paperclip.ing>
claudegoogl-sudo
force-pushed
the
fix/activity-log-teardown-race
branch
from
August 6, 2026 16:15
7a852bf to
56bf9df
Compare
…rdown
An ordered per-table DELETE chain races background heartbeat writes that
land after the test function returns (fire-and-forget executeRun, worker
loops): deleting a parent table out from under a still-in-flight child
insert trips an FK violation and fails afterEach. The activity_log ->
agents FK is the identified symptom (`activity_log_agent_id_agents_id_fk`,
SQLSTATE 23503) but the same race affects every table pair in the chain.
TRUNCATE TABLE companies RESTART IDENTITY CASCADE from the companies root
clears the whole per-test dataset in one atomic statement, removing that
ordering race. The shared helper (reset-test-database.ts) is the only
teardown path these suites should use, so a new test file cannot hand-roll
a wrong order.
Changes:
- Migrate seven heartbeat suites that drive real background/fire-and-forget
heartbeat writes off the sleep + bounded-drain-loop + ordered-delete
teardown and onto resetEmbeddedPostgresTestDatabase. Each conversion
keeps an explicit delete for tables that aren't FK-chained to companies
(environments, instance_settings).
- heartbeat-issue-liveness-escalation was using a raw TRUNCATE without
RESTART IDENTITY and without the helper; migrate it to the helper for
consistency so future readers have one place to learn the teardown.
- Extend resetEmbeddedPostgresTestDatabase to wrap TRUNCATE in the shared
Postgres transient-error retry. heartbeat-process-recovery spawns real
background recovery work, which can deadlock (SQLSTATE 40P01) against
TRUNCATE's ACCESS EXCLUSIVE lock under contention; Postgres picks a
victim and rolls it back, so retrying is guaranteed to make progress.
Reuses services/pg-retry.ts rather than inventing a second mechanism.
- Add activity-log-fk-race-repro.test.ts as a deterministic regression
guard for the FK-race signature. Forces the late activity_log insert
that the broken teardown could not tolerate, and proves the fixed
teardown handles it. Two cases:
RED: ordered delete chain trips activity_log_agent_id_agents_id_fk
on a late insert.
GREEN: TRUNCATE ... CASCADE tolerates the same late insert.
Verification (run individually against each converted file):
- heartbeat-workspace-finalize-branch.test.ts: 3/3 PASS
- heartbeat-accepted-plan-workspace-refresh.test.ts: 4/4 PASS
- heartbeat-dependency-scheduling.test.ts: 6/6 PASS
- heartbeat-process-recovery.test.ts: 75/75 PASS
- heartbeat-responsible-user-invariant.test.ts: 6/6 PASS
- heartbeat-workspace-branch-containment.test.ts: 3/3 PASS
- heartbeat-issue-liveness-escalation.test.ts: 17/17 PASS
- activity-log-fk-race-repro.test.ts: 2/2 PASS (RED case proves the
old failure mode still reproduces, GREEN case proves the fix)
Co-Authored-By: Paperclip <noreply@paperclip.ing>
claudegoogl-sudo
force-pushed
the
fix/activity-log-teardown-race
branch
from
August 6, 2026 16:41
56bf9df to
234d0b4
Compare
claudegoogl-sudo
force-pushed
the
master
branch
from
August 24, 2026 07:46
ee93ee0 to
458e462
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
Linked Issues or Issue Description
Bug-fix path (no public GitHub issue). Underlying problem:
afterEachfailures in heartbeat suites with background writes. Error:delete from "agents"rejected with SQLSTATE23503, constraintactivity_log_agent_id_agents_id_fk. The failing job's log shows the FK violation landing inafterEachteardown, not in the test body.afterEachteardown should never trip an FK violation from a write that the test itself did not await.heartbeat-workspace-finalize-branch.test.ts,heartbeat-responsible-user-invariant.test.ts) in CI many times; eventually the race lands. The newactivity-log-fk-race-repro.test.tsin this PR reproduces the failure mode deterministically by forcing the late insert that the broken teardown could not tolerate.masterat8e9ad2918(and earlier).What Changed
packages/server/src/__tests__/helpers/reset-test-database.ts: extend the shared helper to wrapTRUNCATE TABLE companies RESTART IDENTITY CASCADEin the existingretryOnTransientPgError. TRUNCATE takes an ACCESS EXCLUSIVE lock across every cascaded table, and a concurrent background transaction (e.g. real recovery work spawned byheartbeat-process-recovery.test.ts) can deadlock against it (SQLSTATE40P01). Postgres picks a victim and rolls back, so retrying is guaranteed to make progress. Reusesservices/pg-retry.tsrather than inventing a second retry.heartbeat-workspace-finalize-branch.test.ts,heartbeat-accepted-plan-workspace-refresh.test.ts,heartbeat-dependency-scheduling.test.ts,heartbeat-process-recovery.test.ts,heartbeat-responsible-user-invariant.test.ts,heartbeat-workspace-branch-containment.test.ts: replace the sleep + bounded-drain-loop + ordered-delete teardown withresetEmbeddedPostgresTestDatabase. Each keeps an explicitdb.delete(environments)where applicable, sinceenvironmentsis a global table not FK-chained tocompanies.heartbeat-issue-liveness-escalation.test.ts: this suite already used a rawTRUNCATE TABLE "companies" CASCADEwithoutRESTART IDENTITYand without the helper. Migrate it to the shared helper for consistency, so a future reader has one place to learn the teardown.activity-log-fk-race-repro.test.ts: new deterministic regression guard. Forces the lateactivityLoginsert that the broken teardown could not tolerate, and proves the fixed teardown handles it. Two cases:REDreproduces the original FK violation against an ordered delete chain;GREENprovesTRUNCATE ... CASCADEtolerates the same late insert.Verification
RED-before / GREEN-after evidence was captured by the new
activity-log-fk-race-repro.test.ts:The RED case reproduces the exact failure signature:
delete from "agents"is rejected with SQLSTATE23503and constraintactivity_log_agent_id_agents_id_fkwhen a lateactivityLoginsert lands between the activityLog delete and the agents delete. The GREEN case runs the same late insert againstresetEmbeddedPostgresTestDatabaseand passes — the late insert either commits before the TRUNCATE (cleaned up atomically) or after (its own FK violation is swallowed inside the late-insert promise, not the test's afterEach).Local test commands (against each affected suite, run individually):
Result (per file): 3/3, 4/4, 6/6, 75/75, 6/6, 3/3, 17/17, 2/2 — 116/116 PASS.
Also note: while running
heartbeat-workspace-finalize-branch.test.tsandheartbeat-process-recovery.test.ts, the WARN log lineretrying postgres transaction after transient error {label: 'test_db_reset', attempt: 1, maxAttempts: 6, code: '40P01', delayMs: ...}appeared — i.e. the new deadlock-retry wrapper inresetEmbeddedPostgresTestDatabaseengaged exactly as designed when TRUNCATE collided with concurrent background work.Risks
services/pg-retry.tsdefaults (6 attempts × 25 ms × 2^(n-1) + jitter ≈ 1.6 s worst-case). If a deadlock truly cannot resolve in 6 attempts, the test fails loudly rather than silently — same posture as any other mutation route that uses this helper.waitForHeartbeatIdle/ sleep loops: the converted suites no longer poll for heartbeat idle or sleep 150 ms before teardown. The atomic TRUNCATE removes the ordering race that those waits were papering over. Background writes that land after TRUNCATE either fail their own FK (inside the background promise, swallowed by production code) or land before TRUNCATE (cleaned up atomically). Neither path can fail the test's afterEach.environmentstable: global, not FK-chained tocompanies, so each affected suite that touches environments retains an explicitdb.delete(environments)after the TRUNCATE. Per-file schema audit confirmed this is the only such hold-back table for these suites.Model Used
Claude Opus 4.6 (
claude-opus-4-6) via Claude Code, extended thinking + tool use.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template#NNN/github.com/paperclipai/paperclipURLs)docs/...,fix/...) and contains no internal Paperclip ticket id or instance-derived details