d47b4da655d5769abd5d34fdbdd088716797a7f2
1025 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Devin Foley
|
d47b4da655 |
Auto-build bundled plugins on install (#8254)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Plugins extend the server with worker/UI surfaces, and bundled local plugins under `packages/plugins/**` ship as TS source — their compiled `dist/` is not checked in > - On a fresh checkout, installing a bundled local plugin via the in-app **Install** button failed because `paperclipPlugin.manifest` points at `./dist/manifest.js`, which does not exist until the package is built > - The error surfaces as `Package … does not appear to be a Paperclip plugin (no manifest found)`, which is misleading — the manifest is real, the dist is just missing — and forces every contributor to run `pnpm --filter … build` by hand before the bundled-plugin installer works at all > - This pull request teaches the install path to detect that case and run the package's build (plus standalone runtime bootstrap for plugins outside the root workspace) before manifest resolution, gated by a kill switch and a bounded timeout > - The benefit is bundled plugins like `@paperclipai/plugin-workspace-diff` install in one click on a fresh checkout, with a clear error message and manual fallback when the autobuild itself fails ## Linked Issues or Issue Description No existing GitHub issue. Underlying bug, following the bug-report template: **What happened?** Installing a bundled local plugin from a fresh checkout fails with `Package @paperclipai/plugin-workspace-diff at packages/plugins/plugin-workspace-diff does not appear to be a Paperclip plugin (no manifest found)`. The manifest is declared in `package.json` (`paperclipPlugin.manifest = ./dist/manifest.js`) but `dist/` is not built/committed, so the loader cannot find it. **Expected behavior** Clicking **Install** on a bundled plugin builds it if needed and registers it, without a manual build step. **Steps to reproduce** 1. Fresh checkout of `master` 2. Start the server, open Plugin Manager 3. Click **Install** next to `@paperclipai/plugin-workspace-diff` 4. Observe the "no manifest found" failure **Scope** Same failure mode affects every bundled plugin without a checked-in `dist/` (`plugin-llm-wiki`, examples, sandbox-provider plugins, etc.). ## What Changed - `server/src/services/plugin-loader.ts`: added `ensureLocalPluginBuilt(packageRoot, pkgJson)` — when the package lives under `packages/plugins/**` and its declared paperclipPlugin entrypoints (`manifest`, `worker`, `ui`) are missing, run `pnpm --filter <name> build` (and a standalone runtime-deps bootstrap for plugins outside the root pnpm workspace) before manifest resolution - `server/src/routes/plugins.ts`: invoke the autobuild from the local-path install path; surface a `hasBuiltEntrypoints` boolean on the `AvailableBundledPlugin` listing; invalidate the bundled-plugins cache after a successful install so a freshly built plugin no longer reports `hasBuiltEntrypoints: false` - `ui/src/api/plugins.ts` + `ui/src/pages/PluginManager.tsx`: type and consume `hasBuiltEntrypoints` so the installer can show that an autobuild will run on install - `server/src/__tests__/plugin-install-autobuild.test.ts`: new suite — 9 tests covering success, kill-switch, build failure, timeout, manifest still missing after build, standalone variant, and the existing `plugin-routes-authz` listing assertion - `doc/plugins/LOCAL_PLUGIN_DEVELOPMENT.md`: documents the autobuild, the `PAPERCLIP_DISABLE_PLUGIN_AUTOBUILD=1` kill switch, and the manual fallback command - Detect the autobuild timeout via the child-process `killed` flag rather than string-matching the error message, so the "after timing out" context is actually emitted Knobs: - `PAPERCLIP_DISABLE_PLUGIN_AUTOBUILD=1` — skip autobuild entirely; restore prior behavior - Build timeout: 120s, with a clear error that points at the manual `pnpm --filter <name> build` recovery command ## Verification - `cd server && pnpm vitest run src/__tests__/plugin-install-autobuild.test.ts src/__tests__/plugin-routes-authz.test.ts` → 44/44 pass - End-to-end on a clean checkout: `rm -rf packages/plugins/plugin-workspace-diff/dist`, invoke `ensureLocalPluginBuilt()` against the real package, all declared entrypoints (`dist/manifest.js`, `dist/worker.js`, `dist/ui/index.js`) regenerated. The original `no manifest found` symptom no longer reproduces. ## Risks Low. The autobuild only fires when (a) the package sits under `packages/plugins/**`, (b) at least one declared entrypoint is missing, and (c) the kill switch is not set. In a packaged production server the `packages/plugins/**` path does not exist on disk, so the helper short-circuits and never shells out to `pnpm`. Failures from the spawned build are surfaced as an install error with the exact manual command to retry, so the worst-case is the same UX as before plus a clearer message. ## Model Used Claude Opus 4.7 (claude-opus-4-7), extended thinking enabled, tool use (filesystem + bash). ## 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 - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
Devin Foley
|
f3e01c63bd |
fix(environments): partial unique index to dedup managed sandbox rows (#8247)
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work.
> - The `server/services/environments.ts` module lazily provisions a
managed Kubernetes sandbox environment for each company on first
heartbeat. Idempotency relies on `ensureKubernetesEnvironment` returning
the single managed row per company.
> - The `(company_id, driver)` unique index in the `environments` schema
is partial on `driver='local'` only, so two concurrent callers (e.g.
simultaneous first heartbeats from a freshly synced tenant) can both
insert a `driver='sandbox'` row before either sees the other.
> - The function tried to converge after the race by re-reading, picking
the oldest managed row as winner, and deleting the loser. Under
autocommit + read-committed, each post-insert SELECT is a fresh snapshot
— A may not see B's row, B may not see A's, so both pick their own and
neither deletes. Two rows survive.
> - The same race fired in CI as `expected 2 to be 1` at
`environment-service.test.ts:293`, gating multiple unrelated PRs on
retries.
> - This pull request encodes the operator-level invariant ("at most one
Paperclip-managed sandbox row per company") at the DB layer with a
partial unique index, then switches `ensureKubernetesEnvironment` to
`INSERT … ON CONFLICT DO NOTHING` keyed on that index. Losers re-read
the surviving row.
> - The benefit is the race is impossible by construction — no
application-side convergence loop, no test flake, and any future
`ensureXyzSandboxEnvironment` that sets `managedByPaperclip=true`
inherits the invariant for free.
## Linked Issues or Issue Description
Paperclip issue: PAPA-783 — implement managed-sandbox dedup fix (phase 2
of the approved plan on the parent flake-investigation issue).
This is the phase-2 fix for the flaky `environmentService > deduplicates
concurrent managed Kubernetes environment creation` test introduced by
`4ad94d0bd` (PR #7938). Failing CI runs since then on at least PRs
#7595, #8233, #8215, #8212. The plan was reviewed and approved on the
parent issue before implementation.
Closely related (not duplicates):
- PR #7938 — introduced the test and the in-process convergence loop
being replaced here.
- PR #7595, PR #8233, PR #8215, PR #8212 — downstream PRs affected by
the flake; one of them will be rebased onto this fix as the acceptance
gate.
## What Changed
- `packages/db/src/schema/environments.ts`: added
`environments_company_managed_sandbox_idx`, a partial unique index on
`(company_id) WHERE driver='sandbox' AND
(metadata->>'managedByPaperclip')::boolean = true`. The umbrella
`managedByPaperclip` predicate covers any current or future
Paperclip-managed sandbox flavor without needing a new index per
provider.
- `packages/db/src/migrations/0102_managed_sandbox_dedup_index.sql`:
one-shot dedup `DELETE` keeping the oldest managed-sandbox row per
`company_id` (scoped to `driver='sandbox' AND managedByPaperclip=true`),
`RAISE NOTICE` if any duplicates were removed, then `CREATE UNIQUE INDEX
IF NOT EXISTS environments_company_managed_sandbox_idx`. `CONCURRENTLY`
is omitted because the codebase's migration runner wraps each file in a
transaction (see `applyPendingMigrationsManually`); the table holds 1–3
rows per company, so the short lock is acceptable and consistent with
every other migration in the repo.
- `server/src/services/environments.ts`: `ensureKubernetesEnvironment`
now uses `INSERT … ON CONFLICT DO NOTHING` keyed on the new index. On
conflict it re-reads the surviving managed-sandbox row and returns it.
Drops the post-insert convergence (re-read by `createdAt ASC, id ASC`,
delete the loser) and the trailing comment that flagged "until a partial
unique index is added via migration" as the proper long-term fix.
- Unused `asc` import removed from
`server/src/services/environments.ts`.
## Verification
Local (matches the success criteria in the issue body):
```
$ cd server
$ passes=0; for i in $(seq 1 20); do
pnpm vitest run src/__tests__/environment-service.test.ts -t "deduplicates concurrent" \
&& passes=$((passes+1)) || break
done; echo "$passes/20"
20/20
$ passes=0; for i in $(seq 1 10); do
pnpm vitest run src/__tests__/environment-service.test.ts \
&& passes=$((passes+1)) || break
done; echo "$passes/10"
10/10
```
Adversarial fan-out stress (temporarily bumped `Array.from({ length: 8
}, …)` to `length: 32` on both dedup tests; reverted before commit):
```
$ # both dedup tests fan-out-of-32, 10 iterations
10/10
```
Two ensure paths exist in the parent plan, but only
`ensureKubernetesEnvironment` is on `master`.
`ensureManagedSandboxEnvironment` (referenced by the approved plan as
commit `dce9a9622`) lives only on an unmerged feature branch, not
master. The plan's helper-extraction and symmetric dedup test for that
path are deferred to whichever PR lands the second ensure path — it
inherits the same DB invariant by setting `managedByPaperclip=true`.
Discrepancy was flagged on the issue thread before implementation.
Typecheck:
```
$ pnpm -C server typecheck
ok
```
## Risks
Low risk.
- **Migration safety.** `IF NOT EXISTS` on the index makes the migration
idempotent. The dedup `DELETE` is bounded to rows matching the
managed-sandbox predicate; in production this should be a no-op (no race
has been reported in the wild — only in CI). On dev/CI DBs that already
accumulated duplicates, the migration emits a `NOTICE` reporting the
count.
- **No `CONCURRENTLY`.** The migration runner wraps each `.sql` file in
a transaction, which is incompatible with `CREATE INDEX CONCURRENTLY`.
The `environments` table holds 1–3 rows per company and the row count is
bounded by company count; the short ACCESS EXCLUSIVE during `CREATE
UNIQUE INDEX` is acceptable here and matches every other index migration
in the repo.
- **Predicate scope.** The partial index predicate matches exactly the
rows that `ensureKubernetesEnvironment` writes (`driver='sandbox'` with
`metadata.managedByPaperclip=true`). Tenant-created sandbox envs (via
`svc.create`) do not set this marker and are not covered — no false
positives, no surprise constraint violations on unrelated inserts.
## Model Used
Claude (Anthropic), `claude-opus-4-7`. Tool use: code edit + bash +
filesystem search; no extended-thinking mode.
## 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
- [ ] 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>
|
||
|
Dotta
|
6a684a5053 |
fix(adapters): pass Hermes custom providers as args (#8231)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agent adapters translate Paperclip configuration into the CLI/runtime flags needed by each provider. > - Hermes custom providers are represented as `custom:*` values, but the adapter registry was not passing that provider value through as Hermes CLI arguments. > - The fix is isolated to adapter argument construction and its regression tests. > - This pull request extracts only the Hermes custom-provider pass-through fix onto `origin/master`. > - The benefit is a small adapter PR that can merge independently from UI, skills catalog, and migration work. ## Linked Issues or Issue Description No GitHub issue exists for this branch split. Internal source task: [PAP-11234](/PAP/issues/PAP-11234). Problem/motivation: - Hermes `custom:*` providers need to reach the Hermes process as `--provider <value>`. - The adapter should preserve existing auth injection and avoid adding duplicate provider flags when the user already supplied one. Proposed solution: - Detect Hermes `custom:*` provider values in adapter registry argument construction. - Add `--provider <custom value>` unless an explicit provider arg already exists. - Cover both spaced and equals-style existing provider args in regression tests. Related PR search: - Found related Hermes adapter PRs such as #7544 and #3027, but none duplicates this specific custom-provider arg pass-through behavior. Roadmap alignment: - Checked `ROADMAP.md`; no duplicate planned item was found for this adapter fix. ## What Changed - Updated Hermes adapter registration argument construction to pass `custom:*` providers through `extraArgs` as `--provider <value>`. - Preserved existing auth injection behavior. - Avoided duplicate provider arguments when `--provider value` or `--provider=value` is already present. - Added adapter registry regression coverage. ## Verification - `CI=true NODE_ENV=development pnpm install --frozen-lockfile --prefer-offline` - `pnpm exec vitest server/src/__tests__/adapter-registry.test.ts --run` — 1 file, 20 tests passed. ## Risks - Low risk; change is scoped to Hermes adapter argument construction. - Review should confirm the provider flag syntax matches current Hermes CLI expectations. > 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 Codex via Paperclip `codex_local` / CodexCoder, GPT-5-class coding model with tool use and shell execution. Exact runtime snapshot and context-window setting were not exposed by the Paperclip run context. ## 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 available from the run context) - [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; no UI change) - [x] I have updated relevant documentation to reflect my changes (N/A; no docs behavior changed) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green (pending on draft PR) - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups (pending follow-up loop) - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
Jannes Stubbemann
|
362c30ccdc |
feat(server): opt-in OpenTelemetry auto-instrumentation (#3735)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - Production self-hosters increasingly expect telemetry out of the box — Jaeger, Tempo, Honeycomb, Datadog, Grafana Cloud, Dynatrace all speak OTLP > - Today there is no OpenTelemetry bootstrap in the server, so operators who want traces have to patch their fork or run a sidecar that captures only HTTP-level info > - An opt-in bootstrap that costs nothing when disabled is the minimum-viable surface for this audience > - The OpenTelemetry packages are heavyweight enough that we don't want them in the default dependency graph — they should load only when the operator configures an OTLP endpoint > - This pull request adds a self-contained `server/src/instrumentation.ts` that dynamically imports the OTel SDK and starts it when `OTEL_EXPORTER_OTLP_ENDPOINT` is set, and is a complete no-op otherwise ## Linked Issues or Issue Description No existing issue covers this directly — feature-gap description following the feature-request template: **Problem or motivation** Production self-hosters increasingly expect telemetry out of the box — Jaeger, Tempo, Honeycomb, Datadog, Grafana Cloud, Dynatrace all speak OTLP — but the server has no OpenTelemetry bootstrap. Operators who want traces today must patch their fork or run a sidecar that captures only HTTP-level information. **Proposed solution** An opt-in OTel bootstrap gated on `OTEL_EXPORTER_OTLP_ENDPOINT`, loaded via dynamic `import()` only when configured, so the heavyweight OTel packages stay out of the default dependency graph. **Alternatives considered** Related open PRs found during the duplicate-PR search approach observability differently: #4894 adds OTLP instrumentation to Paperclip core unconditionally, and #3752 proposes an observability plugin. Not duplicates — different layering: this PR keeps the default install dependency-free via opt-in dynamic import. ## What Changed - New `server/src/instrumentation.ts` — opt-in OpenTelemetry auto-instrumentation. Gated on `OTEL_EXPORTER_OTLP_ENDPOINT`. Respects the standard OTel env vars (`OTEL_SERVICE_NAME`, `OTEL_SERVICE_VERSION`, `OTEL_EXPORTER_OTLP_ENDPOINT`). Skips the fs/dns/net auto-instrumentations (too chatty). `sdk.start()` is wrapped in try/catch so a bad endpoint or missing native bindings doesn't crash the server. `process.once("SIGTERM" / "SIGINT", …)` for clean shutdown on the first signal only. OTel packages are loaded via dynamic `import()` so they are true optional runtime dependencies — no entries in `package.json`, no lockfile churn. - `server/src/index.ts` — import `./instrumentation.js` as the very first statement so auto-instrumentation can patch `http` / `express` / `pg` before they are evaluated by downstream modules. ## Verification - `OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4317 pnpm start` after `pnpm install @opentelemetry/{sdk-node,auto-instrumentations-node,exporter-trace-otlp-grpc,resources,semantic-conventions}` in `server/` — traces show up in the configured collector; HTTP, Express, and Postgres spans are populated. - `OTEL_EXPORTER_OTLP_ENDPOINT` unset — server starts with no OTel-shaped output in logs, no behavior change. - `OTEL_EXPORTER_OTLP_ENDPOINT=…` set but packages not installed — single `console.warn` at startup telling the operator which packages to install. ## Risks Low. No behavior change unless the env var is set. The bootstrap never throws into the caller; every failure path ends in `console.warn` / `console.error` and falls through to non-traced operation. ## Model Used Claude Opus 4.6 (1M context), extended thinking mode. ## 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] Tests run locally and pass - [x] CI green - [x] Greptile review addressed --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Jannes Stubbemann
|
937fe62d10 |
feat(server): TRUST_PROXY supports CIDR list + named subnets (supersedes #3729) (#5872)
## Thinking Path
> - Paperclip orchestrates AI agents for zero-human companies
> - Express looks at incoming `X-Forwarded-For` headers only when
`app.set("trust proxy", …)` says it should, and uses that resolved
client IP downstream for rate-limiting, audit logging, and any
auth/abuse signal that ties back to source IP
> - The original PR #3729 added `TRUST_PROXY` accepting only `"true"` or
a positive integer, which forces operators to pick between two unsafe
defaults: hop-count (brittle if topology changes) or boolean-true (any
client can spoof `X-Forwarded-For` and bypass rate-limits or pollute
audit logs)
> - `trust proxy: true` is one of the most common Express
misconfigurations and trivially exploitable for IP-spoofing-based
rate-limit bypass; the safest config — trust only the LB's actual CIDR
or only loopback — was unreachable with the previous parser
> - This pull request replaces the parser with full Express 5 support —
unset / `false` / `0` (Express default), positive integer hop count,
comma-separated CIDR list, named subnets (`loopback`, `linklocal`,
`uniquelocal`) — and emits a startup error naming the offending token on
invalid input
> - The benefit is that operators can now trust *only* their actual
ingress and close the spoofing window without leaking client-IP
integrity to downstream layers, while preserving every
previously-working config as a strict superset
## Linked Issues or Issue Description
Refs #1690 — login returns 500 behind a reverse proxy because Express
`trust proxy` is not enabled; this PR ships the configuration surface
(`TRUST_PROXY` with CIDR lists and named subnets) that lets operators
enable it safely. It does not change the default, so #1690 still
requires the operator to set `TRUST_PROXY` — hence Refs, not Fixes.
No other existing issue covers this directly — remaining problem
described in-PR:
- The original `TRUST_PROXY` parser (PR #3729, which this PR supersedes)
accepted only `"true"` or a hop count, forcing operators to choose
between brittle hop-counting and the spoofable `trust proxy: true`.
- `trust proxy: true` lets any client spoof `X-Forwarded-For` and bypass
rate limits or pollute audit logs; the safest config — trusting only the
LB's actual CIDR or only loopback — was unreachable with the previous
parser.
Duplicate-PR search: #1854 / #1714 are earlier minimal trust-proxy
enablement PRs; this PR supersedes #3729 and generalizes beyond a
boolean enable (CIDR lists + named subnets).
## What Changed
- **`server/src/middleware/trust-proxy.ts`** — new helper exposing
`parseTrustProxyEnv` (testable) and `applyTrustProxy(app)` (one-call
boot wiring). Surface:
- Unset / `""` / `false` / `0` → no `app.set("trust proxy", …)` (Express
default: trust nothing).
- `true` → `app.set("trust proxy", true)`. Documented as unsafe in
untrusted-LB deployments.
- Positive integer (e.g. `"2"`) → hop count. Strict parse: rejects
`"01"`, leading/trailing whitespace.
- Comma-separated list of CIDRs and/or named subnets (e.g.
`"loopback,uniquelocal,10.0.0.0/8,fd00::/8"`) → array passed to
`app.set("trust proxy", [...])`.
- Anything else → startup error naming the offending token.
- **`server/src/app.ts`** — one import + one call to
`applyTrustProxy(app)`.
- **`server/src/__tests__/trust-proxy.test.ts`** — 12 cases: unset,
`"true"`, `"0"`, `"2"`, `"01"` rejected, `" 2 "` rejected, `"loopback"`,
`"loopback,uniquelocal"`, `"10.0.0.0/8"`, `"10.0.0.0/8,fd00::/8"`,
`"bogus"` rejected (error names the bad token), mixed-list with one bad
token rejected (error names the offending token specifically).
## Verification
- `pnpm --filter @paperclipai/server run typecheck` — clean.
- `npx vitest run trust-proxy` — 12/12 pass.
## Risks
- **No new required env vars.** Unset means default Express behavior
(trust nothing). Pure superset of #3729's surface — anything that worked
under #3729 still works here.
- **Strict parse.** `"01"` and `" 2 "` are rejected on purpose so
configuration mistakes surface at startup, not as silently-degraded
auth/rate-limit behavior. The error message names the offending token.
- **No runtime cost** — the parse runs once at boot. The downstream
`trust proxy` setting is internal to Express.
- Single-tenant local-first deploys unaffected by default.
## Model Used
Claude Opus 4.7 (1M context), extended thinking mode.
## 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 — not in conflict with planned core work
- [x] Tests run locally and pass (`trust-proxy` 12/12)
- [x] Added boundary cases (leading-zero, whitespace, unknown token,
mixed-list-with-bad-token)
- [x] No UI changes
- [x] Documented risks above
- [x] Will address all Greptile and reviewer comments before merge
Closes #3729.
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Jannes Stubbemann
|
3701be76fa |
fix: read-only agent config/skill endpoints should not require agents:create (#3725)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - Access to agents, skills, and configurations is governed by a per-company permission system > - `agents:create` is a mutation-tier permission that controls who can create or modify agents > - `assertCanReadConfigurations` delegates to `assertCanCreateAgentsForCompany`, effectively requiring `agents:create` just to *read* agent configs, skills, and config revisions > - That's a permission regression: any company member without `agents:create` hits 403 on the Skills tab, agent config pages, and revision history — but those responses are already secret-redacted > - This pull request loosens the read gate to company membership only, while keeping every mutation-adjacent gate at `agents:create` ## Linked Issues or Issue Description No existing issue covers this directly — problem described in-PR following the bug-report template: **What happened** `assertCanReadConfigurations` delegates to `assertCanCreateAgentsForCompany`, effectively requiring the mutation-tier `agents:create` permission just to *read* agent configs, skills, and config revisions. Any company member without `agents:create` hits 403 on the Skills tab, agent config pages, and revision history — even though those responses are already secret-redacted (`redactAgentConfiguration`, `redactConfigRevision`). **Expected behavior** Read-only configuration/skill/revision endpoints are readable by any company member; only mutation-adjacent endpoints require `agents:create`. **Steps to reproduce** As a company member without `agents:create`, open the Skills tab or an agent config page (or `GET` the config/skill/revision endpoints) — the request fails with 403. ## What Changed - `server/src/routes/agents.ts`: - `assertCanReadConfigurations` now requires company membership only (plus the existing agent-key cross-company check). Previously it required `agents:create`. - `actorCanReadConfigurationsForCompany` (the boolean twin, used by `GET /agents/:id` to decide whether to return a restricted detail) now uses the standard try/catch-around-`assertCompanyAccess` pattern. - `POST /companies/:companyId/adapters/:type/test-environment` is not a pure read (it exercises adapter secrets) and now calls `assertCanCreateAgentsForCompany` directly instead of going through `assertCanReadConfigurations`. Behavior for this endpoint is unchanged. ## Verification - Existing tests pass. - Manual: log in as a company member without an `agents:create` grant. Visit the Skills tab on an agent and the agent configuration panel — both load. Try to edit the agent — blocked, as before. - Manual: POST to `/companies/:companyId/adapters/:type/test-environment` as the same user — still 403. ## Risks Low. The only behavior change is on read endpoints whose responses were already redacted (\`redactAgentConfiguration\`, \`redactConfigRevision\`). No secret escapes anywhere. ## Model Used Claude Opus 4.6 (1M context), extended thinking mode. ## 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] Tests run locally and pass - [x] CI green - [x] Greptile review addressed Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Jannes Stubbemann
|
c21f70ef1c |
fix: skip gosu when already running as target user (#2908)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - The reference container image must be deployable on both Docker Compose (where it starts as root and `gosu`'s a `USER_UID`/`USER_GID` switch) and Kubernetes (where the pod is typically constrained by PodSecurity) > - The Kubernetes operator (paperclipinc/paperclip-operator#45) sets `runAsNonRoot: true`, `runAsUser: 1000`, `allowPrivilegeEscalation: false`, and `drop: ALL` capabilities by default — the unconditional `usermod` + `gosu` flow in the entrypoint requires root + `CAP_SETUID` / `CAP_SETGID`, making the image undeployable on any cluster enforcing baseline or restricted PodSecurity > - Without root, neither the user remap nor `gosu` can ever succeed — so the fix is a runtime branch: non-root starts `exec` the command directly (warning if the runtime UID/GID differs from the requested one), while root starts keep the existing `usermod`+`gosu` flow > - This also covers platforms that assign arbitrary UIDs (OpenShift restricted SCC), which previously crashed with a cryptic `usermod: Permission denied` > - The benefit is one image that works for both deployment shapes with no operator-side workaround — pure superset, no breaking change ## Linked Issues or Issue Description Refs paperclipinc/paperclip-operator#45 (cross-repo) — the operator's default pod security context (`runAsNonRoot: true`, `runAsUser: 1000`, `allowPrivilegeEscalation: false`, `drop: ALL`) is blocked by this entrypoint behavior. The operator shipped a stopgap (paperclipinc/paperclip-operator#46 lets the CRD override the security context); this PR is the image-side fix that makes the secure defaults work out of the box. Supersedes #2904 (v1 of this branch). No in-repo issue covers this directly — problem described in-PR following the bug-report template: **What happened** The entrypoint unconditionally runs `usermod`/`groupmod`/`chown` + `exec gosu node`, which requires root plus `CAP_SETUID` / `CAP_SETGID` — any non-root start crashes (`gosu` cannot drop privileges; a mismatched UID dies earlier at `usermod: Permission denied`), making the reference image undeployable on clusters enforcing baseline or restricted PodSecurity. **Expected behavior** A non-root container `exec`s the command directly (with a clear warning if its UID/GID differs from the requested `USER_UID`/`USER_GID`, since a remap is impossible without root). The existing root + `usermod` + `gosu` flow is preserved for Docker Compose, where the container starts as root and switches to the requested UID/GID. **Deployment mode** Kubernetes with baseline/restricted PodSecurity and OpenShift-style arbitrary-UID platforms (failing cases); Docker Compose root-start (must keep working). ## What Changed - **`scripts/docker-entrypoint.sh`** — branch on the runtime UID: - **Non-root start** → `exec "$@"` directly. If the runtime UID/GID differs from `USER_UID`/`USER_GID`, print a one-line warning to stderr first (the remap is impossible without root; the warning keeps volume-permission mismatches diagnosable instead of failing cryptically inside `usermod`). - **Root start** → unchanged: `usermod`/`groupmod` remap when requested, `chown` of `/paperclip` when a remap happened, then `exec gosu node "$@"`. ## Verification **Automated:** `server/src/__tests__/docker-entrypoint.test.ts` runs the real entrypoint with `id`/`usermod`/`groupmod`/`chown`/`gosu` stubbed via PATH and asserts all five privilege branches (root+defaults, root+remap, non-root match, arbitrary non-root UID, GID mismatch) — runs in the regular server suite, no Docker needed. **Manual (Docker):** ran the entrypoint on `node:lts-trixie-slim` (the image's actual base) across the full matrix, with `gosu` stubbed to a marker: - [x] Root start, defaults → no remap, `gosu node` invoked (Docker Compose flow unchanged) - [x] Root start, `USER_UID=1001`/`USER_GID=1001` → `Updating node UID/GID to 1001` + `gosu node` (remap flow unchanged) - [x] Non-root `--user 1000:1000` (the operator's `runAsUser: 1000` shape) → silent direct exec, command runs as 1000:1000 - [x] Non-root `--user 1234:1234` (arbitrary UID) → warning `running unprivileged as 1234:1234; cannot remap to requested 1000:1000`, then direct exec (previously: crash) - [x] Non-root `--user 1000:1001` (GID mismatch) → warning, then direct exec - [x] Baseline check: master's entrypoint fails for any non-root start (gosu/usermod require root) End-to-end cluster verification under restricted PodSecurity exercises the same branch as the `--user 1000:1000` case above; the operator repo's deploy is the natural place for that smoke test once this ships in an image tag. ## Risks - **Backward-compatible.** Docker Compose / root-entrypoint path is byte-for-byte the same flow — `usermod`/`gosu` runs whenever the container starts as root. - **Behavior change only for previously-broken starts.** Non-root containers used to crash; they now run. The only observable difference for a *working* deployment is none. - **Mismatched non-root UID/GID warns instead of failing.** Deliberate: the remap is impossible without root, and arbitrary-UID platforms (OpenShift) rely on group-writable volumes; a hard fail would keep them broken. The stderr warning preserves diagnosability. - **No new env vars, no API surface.** Pure entrypoint behavior change gated on the runtime UID. - **Restricted PodSecurity ready.** The non-root branch needs no Linux capabilities — works under `drop: ALL`. ## Model Used Claude Opus 4.6; rebase, non-root generalization, and verification matrix 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 — not in conflict with planned core work (agent-runtime sandbox images use `tini`, no gosu — unaffected) - [x] Tests run locally and pass (new `docker-entrypoint.test.ts` covering all five privilege branches, plus a manual Docker matrix on the real base image; see Verification) - [x] No UI changes - [x] Documented risks above - [x] Will address all Greptile and reviewer comments before merge Unblocks the default (non-overridden) security context of paperclipinc/paperclip-operator#45 / #46. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
Jannes Stubbemann
|
05bcd3ce84 |
feat(security): plugin tables get company_id FK for tenant isolation (#5865)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - The plugin subsystem persists state into four tables (`plugin_entities`, `plugin_job_runs`, `plugin_logs`, `plugin_webhook_deliveries`) and those rows currently have no notion of an owning tenant — so company-deletion doesn't cascade plugin state, and operators have no way to query "what does this plugin own for company X?" > - The fix is one thin slice of tenant-isolation hygiene that doesn't change any external API: add a nullable `company_id` FK with `ON DELETE CASCADE` to the four plugin tables, index it, and scope the `plugin_entities` external-id uniqueness per-tenant > - The benefit is plugin-row tenant attribution + cascade cleanup, with zero impact on single-tenant local-first deploys (`NULL` continues to mean instance-scope) > **Rebase note (scope narrowed):** This PR originally also hardened the schedulers (`heartbeat.tickTimers` / `resumeQueuedRuns` / `enqueueWakeup` and `routines.tickScheduledTriggers`) to skip archived companies. That half has since landed on `master` via #7478 (`93206f73`, "Stop archived companies from waking agents") with a stricter implementation (`status != 'active'` plus a skipped-request audit row). On rebase those changes were dropped as redundant — `heartbeat.ts` and `routines.ts` are now identical to `master`, and the scheduler-specific tests were removed. **This PR is now DB-only.** ## Linked Issues or Issue Description No existing issue covers this directly — problem described in-PR: - Four plugin tables (`plugin_entities`, `plugin_job_runs`, `plugin_logs`, `plugin_webhook_deliveries`) persist rows with no notion of an owning tenant. - Company deletion therefore does not cascade plugin state, and operators have no way to query "what does this plugin own for company X?" - One thin slice of tenant-isolation hygiene fixes this without changing any external API: a nullable `company_id` FK with `ON DELETE CASCADE`, an index, and per-tenant scoping of the `plugin_entities` external-id uniqueness. - Part of the multi-tenant hardening initiative alongside #3967 (cross-tenant 404 oracle) and #5864 (per-company JWT keys). ## What Changed **Schema (`packages/db/src/schema/plugin_*.ts`):** - Nullable `companyId` FK with `onDelete: "cascade"` added to `plugin_entities`, `plugin_job_runs`, `plugin_logs`, `plugin_webhook_deliveries`. - A btree index on each new `company_id` column (`<table>_company_idx`). - `plugin_entities_external_idx` rescoped from `(plugin_id, entity_type, external_id)` to `(company_id, plugin_id, entity_type, external_id)` and switched to `UNIQUE … NULLS NOT DISTINCT` so instance-scope rows (`company_id IS NULL`) keep their dedup guarantee while tenants get their own namespace. **Migration:** - `0095_plugin_company_id_tenant_isolation.sql` — 14 statements: 4 column adds + 4 FK constraints (`ON DELETE CASCADE`) + 4 indexes + drop/recreate of the external-id unique constraint. - Journal entry + regenerated `0095_snapshot.json`. **Tests:** - `server/src/__tests__/plugin-tenant-isolation.test.ts` — `NULL` preserves backward compat; `CASCADE` on company delete across all four tables; per-tenant external-id namespacing; NULL-NULL collision rejected (`NULLS NOT DISTINCT`). ## Verification - `pnpm --filter @paperclipai/db run check:migrations` — pass. - `pnpm --filter @paperclipai/db typecheck` (`tsc`) — pass. - `vitest run plugin-tenant-isolation` — **4/4 pass** (embedded Postgres applies `0095` and exercises cascade + NULLS NOT DISTINCT). ## Notes - **Clean snapshot, no drift.** The earlier revision of this PR shipped a ~17.6k-line meta snapshot that was almost entirely pre-existing drift. On rebase the migration was renumbered (the old `0090_brainy_darkhawk` collided with `master`'s `0090_resource_memberships … 0094`) and regenerated from the current `master` baseline via `drizzle-kit generate`. The result is a 14-statement migration containing **only** the plugin-table changes — no unrelated drift. - **Backward-compatible.** `NULL company_id` continues to mean instance-scope (cron jobs, public webhooks). No new env vars, no API surface change. Single-tenant local-first deploys unaffected. ## Risks - **Migration is additive and nullable** — `0095` adds nullable `company_id` columns, FK constraints, and indexes; existing rows stay valid (`NULL` keeps meaning instance-scope) and no backfill is required. - **`ON DELETE CASCADE` is a behavioral change**: deleting a company now removes its plugin rows across all four tables. Intended (it is the point of the PR), but operators relying on plugin rows surviving company deletion would be affected. Covered by the cascade tests. - **Uniqueness semantics change on `plugin_entities`**: the external-id constraint is rescoped per-tenant and switched to `UNIQUE … NULLS NOT DISTINCT`, so two instance-scope rows (`company_id IS NULL`) with the same external id are now rejected instead of coexisting. Covered by the NULL-NULL collision test. - **No API surface change, no new env vars.** Single-tenant local-first deploys unaffected. (Section added retroactively to match the PR template; distilled from the What Changed / Notes sections above.) ## Model Used Same authoring setup as #5864 (same series, same day): Claude Opus 4.7 (1M context), extended thinking mode. (Section added retroactively.) ## Checklist - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] Tests run locally and pass (plugin-tenant-isolation 4/4) - [x] `check:migrations` + db typecheck pass - [x] No UI changes - [x] Migration carries only the intended changes (no snapshot drift) - [x] Scheduler half dropped as superseded by #7478 Part of the multi-tenant hardening initiative — see also #3967 (cross-tenant 404 oracle) and #5864 (per-company JWT keys). --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Devin Foley <devin@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
Aron Prins
|
a5b3cc98b0 |
fix(server): cache Intl.DateTimeFormat per timezone in cron minute-stepper (#8033) (#8034)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The routine scheduler lets agents and users run work on cron schedules; every 30s a tick computes each trigger's next occurrence > - `computeNextRun`/`nextCronTickInTimeZone` find that occurrence by stepping forward one minute at a time (capped at 2.6M iterations), constructing a fresh `Intl.DateTimeFormat` on every step — ~1ms of ICU work each > - Sparse schedules (monthly ≈ 43k steps ≈ 40s) and never-matching crons (the #7529 midnight bug → full 2.6M steps ≈ 45 min) block the Node event loop for the whole scan, every tick > - In production this pegs the server at 100% CPU, health checks time out, and Paperclip Desktop repeatedly shows "the embedded server is no longer responding" (diagnosed via a CPU sample: 74% of samples inside `Builtin_DateTimeFormatConstructor`) > - This pull request caches the formatter per timezone, since `Intl.DateTimeFormat` instances are immutable and reusable > - The benefit is each minute-step pays only `formatToParts` (~1µs): a 43k-step scan drops from ~40s of blocked event loop to under a second, and the server stays responsive while routines are active ## Linked Issues or Issue Description - Fixes: #8033 - Refs #7529 — a never-matching midnight cron forces the minute-stepper through its full 2.6M-iteration cap, which is the worst-case trigger for this perf bug; the two compound - Refs #7922 — in-flight fix for #7529 touching the same function (`getZonedMinuteParts`); the changes are compatible (this PR changes formatter construction, that PR changes hour normalization) ## What Changed - `server/src/services/routines.ts`: added a per-timezone `Intl.DateTimeFormat` cache (`getZonedMinuteFormatter`) used by `getZonedMinuteParts` and `assertTimeZone`, replacing per-call construction - Exported `nextCronTickInTimeZone` so the behavior is testable (same export PR #7922 makes) - Added `server/src/services/routines-formatter-cache.test.ts`: verifies a sparse monthly cron resolves to the correct next occurrence across a DST-bearing timezone, and asserts at most one formatter construction for a ~43k-minute-step scan (and zero on a warm cache) ## Verification - `npx vitest run server/src/services/routines-formatter-cache.test.ts` — 2 tests pass - `pnpm --filter @paperclipai/server exec tsc --noEmit` — clean - Live validation: applied the same patch to the server bundled in Paperclip Desktop 3.2.9, which was hitting 99.3% CPU with 5s health-check timeouts within 60s of boot on a real workload; after the patch, CPU idles at 0–3% across scheduler ticks and `/api/health` answers in ~1ms (observed over multiple 30s ticks) ## Risks - Low risk. `Intl.DateTimeFormat` instances are immutable and safe to reuse; the cache key is the timezone string, and entries are small and bounded by the number of distinct timezones in use - Invalid timezones still throw in the constructor before anything is cached, so `assertTimeZone` semantics are unchanged - Does not change cron matching semantics; minute-stepping remains (replacing it with cron-field arithmetic is noted in #8033 as a follow-up) ## Model Used - Claude (Anthropic), model ID `claude-fable-5` (Fable 5), via Claude Code CLI with extended thinking and tool use (profiling, patching, and live verification performed by the model under user supervision) ## 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 (N/A — server-only change) - [ ] I have updated relevant documentation to reflect my changes (N/A — internal perf fix, no doc surface) - [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 (pending review) - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Devin Foley
|
3fbab2e6db |
fix: resolve orphan-sweep null-assignee filter regression (#8018)
> Resubmits #5925 by @digitalflanker-ux (rebased onto current `master`; original commit authorship preserved). ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The issues list API powers orphan-sweep and board inbox views that filter by assignee > - `assigneeAgentId=null` is a valid query-string sentinel for "unassigned issues" > - A regression caused that sentinel to throw 500 instead of filtering correctly > - This pull request restores null-sentinel parsing in the route and service layers > - The benefit is reliable orphan-sweep and unassigned-issue queries without server errors ## Linked Issues or Issue Description Refs #5891 (paired fix — land together) **Bug:** `GET /api/companies/:id/issues?assigneeAgentId=null` returned HTTP 500. Expected: HTTP 200 with only unassigned issues. Malformed UUIDs should return 4xx, not 500. ## What Changed - Parse `assigneeAgentId=null` in the issues list route and pass a JS `null` filter to the service - Handle malformed assignee IDs with HTTP 422 in the route layer - Extend `issueService.list` to treat `assigneeAgentId: null` as `IS NULL` SQL filter - Add route-level and service-level regression tests ## Verification - `pnpm exec vitest run server/src/__tests__/issue-list-assignee-filter-routes.test.ts server/src/__tests__/issues-service.test.ts` - result: 2 files passed, 79 tests passed ## Risks Low risk — scoped to query-parameter parsing and list filtering; no schema or API contract changes beyond fixing the regression. ## Model Used None — human-authored original fix by @digitalflanker-ux; rebased and test-harness adjustments by Paperclip cluster cleanup. ## 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 - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge ## Cross-references and status (maintainer) - Pairs with #5891 — both fix the `assigneeAgentId=null` issues-list regression and should land together. - Supersedes #5925 (fork branch could not be force-pushed; this is the operator-mergeable resubmission). --------- Co-authored-by: openclaw <digitalflanker@gmail.com> Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
roy493
|
d9ea1bf9e1 |
fix: skip same-run self-comments (Path A heartbeat-reopen + Path B implicit-todo move) (#4973)
## Thinking Path - Paperclip treats issue comments as both communication and wake signals, so comment attribution affects whether completed work reopens. - The bug lived in two independent paths: deferred comment wake promotion in `heartbeat.ts`, and implicit reopen-on-comment logic in `routes/issues.ts`. - Both paths need the same core rule: a comment from the same run that just closed the issue must not look like a fresh human follow-up. - Deferred wake batches also need one extra safeguard: if a batch mixes a same-run self-comment with a real human comment, the human follow-up must still reopen the issue. ## What Changed - `server/src/services/heartbeat.ts` now suppresses deferred reopen only when every referenced comment in the batch was created by the closing run. - `server/src/routes/issues.ts` now passes `actorRunId`, `checkoutRunId`, and `executionRunId` into `shouldImplicitlyMoveCommentedIssueToTodo`, and skips the implicit move when the comment came from the run that already owns the issue. - `server/src/__tests__/heartbeat-comment-wake-batching.test.ts` adds coverage for both Path A cases: same-run self-comment stays closed, while a mixed self-comment plus human-comment batch still reopens. - `server/src/__tests__/issue-comment-reopen-routes.test.ts` covers the same-run guard on both POST and PATCH comment paths, plus the negative case where a different run still reopens. ## Verification ```bash pnpm --filter @paperclipai/server exec vitest run src/__tests__/issue-comment-reopen-routes.test.ts pnpm --filter @paperclipai/server exec vitest run src/__tests__/heartbeat-comment-wake-batching.test.ts -t "self-authored by the closing run|mixes self-authored and human comments" pnpm --filter @paperclipai/server typecheck ``` ## Risks - Low risk. Both changes are additive guards and preserve existing behavior for comments that do not originate from the owning run. - The deferred-wake change now uses all-self semantics, which is the key correctness detail for mixed batches. - Full CI is still the authoritative validation for the broader heartbeat integration surface. ## Model Used - OpenAI Codex, GPT-5-based coding agent (`codex_local` adapter in Paperclip). ## 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 — N/A, server-only - [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 ## Cross-references and status (maintainer) Refs #6601 Refs #3980 --------- Co-authored-by: Paperclip <paperclip@users.noreply.github.com> |
||
|
Sherman Lye
|
fecc41d4fd |
fix(recovery): skip stranded-issue recovery when pending wake interaction exists (#4854)
## Thinking Path > - Paperclip is the open source control plane people use to manage AI agents and their work. > - Recovery logic is part of that control plane because it decides when agent work is truly stranded versus intentionally waiting. > - Issues can pause on human-gated thread interactions such as `request_confirmation`, `ask_user_questions`, and `suggest_tasks`. > - `reconcileStrandedAssignedIssues()` was treating some of those waiting issues as stranded because it did not check for pending wake-style interactions. > - That mismatch created false-positive recovery cascades on work that was correctly paused for human input. > - This pull request adds the missing guard and locks it in with focused regression coverage. > - The benefit is safer recovery behavior: real stranded work is still recovered, while human-gated work stays stable and inspectable. ## Linked Issues or Issue Description - Refs #7403 - Searched open GitHub PRs/issues for the same recovery-interaction bug before merge prep; no duplicate open PRs found. ## What Changed - Added `hasPendingWakeInteraction(companyId, issueId)` in `server/src/services/recovery/service.ts` to detect pending thread interactions with continuation policies `wake_assignee` and `wake_assignee_on_accept`. - Inserted that guard into `reconcileStrandedAssignedIssues()` immediately after the active-execution-path check so human-gated issues are skipped instead of escalated. - Added a parameterized regression test in `server/src/__tests__/heartbeat-process-recovery.test.ts` that covers both continuation-policy values and verifies recovery does not fire. - Appended the maintainer cross-reference section required by merge prep. ## Verification - `pnpm exec vitest run --project @paperclipai/server server/src/__tests__/heartbeat-process-recovery.test.ts -t "skips stranded recovery when a pending" --pool=forks --isolate` - Greptile Summary comment on the latest head reports `Confidence Score: 5/5`. - Remote Paperclip CI is running on head `4702684c213b5018e6918cb6176e7ef40f440ebf`. ## Risks - Low risk. The production change is a read-only early exit in an existing recovery sweep. - The main behavioral shift is intentional: issues with pending wake-style interactions will no longer enter stranded recovery until the human gate clears. - If there is a hidden interaction state we should also treat as waiting, it would need an explicit follow-up rather than falling through this guard. > Checked `ROADMAP.md`; this is a focused bug fix, not overlapping roadmap feature work. ## Model Used - Original PR authoring: Claude Code using Claude Opus 4.6. - Merge prep, rebase, verification, PR-body repair, and Greptile follow-up: OpenAI Codex via the Paperclip ACPX local adapter (exact model ID not exposed in this workspace), with tool use and code execution. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [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 - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] 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) Refs #7403 Co-authored-by: Sherman Lye <user@example.com> |
||
|
Ismaël O.
|
3b7c42be86 |
fix(openclaw-gateway): complete and stabilize OpenClaw Gateway integration (#2322)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The `openclaw_gateway` adapter is how operators wire Paperclip agents to an OpenClaw gateway over WebSocket > - The adapter UI previously only exposed a handful of config fields in edit mode; many timeout / auth / session-routing knobs were unreachable through the form > - The serializer also forgot to inject the configured `authToken` into the `x-openclaw-token` header, and the server-side execute path lacked retries on transient gateway errors and an `OPENCLAW_TOKEN` env fallback > - This pull request exposes the full set of config fields in both create and edit modes, fixes the serializer, hardens the server-side execute path, and pins the existing default request timeouts (120s / 120000ms) — see the dedicated commit and the new unit tests > - The benefit is operators can configure and reconfigure an `openclaw_gateway` agent end-to-end through the UI, with no silent change to the defaults documented in the adapter README and `doc/ONBOARDING_AND_TEST_PLAN.md` ## Linked Issues or Issue Description Closes #414 Closes #1901 Closes #2309 ## What Changed - **UI**: Removed the `!isCreate` guard so all `openclaw_gateway` config fields are visible in both create and edit modes (`authToken`, `agentId`, `sessionKeyStrategy`, `sessionKey`, `timeoutSec`, `waitTimeoutMs`, `disableDeviceAuth`, `autoPairOnFirstConnect`, `role`, `scopes`, `paperclipApiUrl`, `headersJson`, `payloadTemplate`, `runtimeServices`). - **Serialization** (`packages/adapters/openclaw-gateway/src/ui/build-config.ts`): inject `authToken` into headers as `x-openclaw-token`; apply safe defaults on create (`timeoutSec=120`, `waitTimeoutMs=120000`, `sessionKeyStrategy="issue"`, `role="operator"`, `scopes=["operator.admin"]`). - **Backend** (`packages/adapters/openclaw-gateway/src/server/execute.ts`): add `OPENCLAW_TOKEN` env-var fallback for `authToken`, retry logic (max 2 retries with backoff for transient gateway errors), session-key prefix `agent:{agentId}:{sessionId}` when `agentId` is configured. - **Defaults restoration** (dedicated commit): an earlier revision of this PR lowered the default request timeouts to `60` / `30000`. The current branch restores the historical `timeoutSec=120` / `waitTimeoutMs=120000` defaults that match the values documented in `packages/adapters/openclaw-gateway/src/index.ts`, `src/server/execute.ts` on master, and the worked example in `doc/ONBOARDING_AND_TEST_PLAN.md`. - **Tests** (new): `packages/adapters/openclaw-gateway/src/ui/build-config.test.ts` pins the documented timeout and identity defaults so the silent-halve regression cannot recur. ## Verification - `pnpm --filter @paperclipai/adapter-openclaw-gateway typecheck` - `pnpm typecheck` (root) - Manual: create a new `openclaw_gateway` agent — all fields visible, defaults populate as documented. - Manual: edit an existing `openclaw_gateway` agent — every field round-trips correctly and saves. - Manual: unset `authToken` in the form and set `OPENCLAW_TOKEN` env var — adapter picks up the env-var fallback. - Manual: simulate a transient gateway error — execute retries up to 2 times with backoff before failing. ## Risks - Low risk. Surface area is one adapter, behind explicit operator configuration. The defaults change in this PR is a restoration of values that already exist on master and in the adapter docs, so no production agent sees a behavioral shift relative to the prior release. Field exposure in edit mode is purely additive — existing values are preserved on save. ## Model Used - Provider/model: Claude (Anthropic) — `claude-opus-4-7` - Mode: standard tool use, no extended thinking - Capability notes: code execution + repository file edits via Claude Code ## Cross-references and status (maintainer) Closes #414 Closes #1901 Closes #2309 ## 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 - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip Bot <bot@paperclip.dev> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Devin Foley <devin@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
Vasu Yadav
|
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> |
||
|
David Bezar
|
01e59c074a |
fix(watchdog): suppress repeat alerts when source issue is blocked or evaluation board-closed (#5942)
## Thinking Path
> - Paperclip orchestrates AI agents for zero-human companies; the
stale-active-run watchdog monitors agent heartbeat runs for extended
output silence and fires evaluation issues to alert the responsible
manager
> - The watchdog uses a unique index on open evaluation issues to
prevent duplicate open issues per run, but this only prevents two
*simultaneous* open issues — not sequential ones created after each
closure
> - When a board reviewer closes an alert as done directly (without
recording a watchdog decision), the dismissed_false_positive guard is
bypassed and `findOpenStaleRunEvaluation` returns null on the next scan
— causing a new alert to fire every 30 minutes until the run terminates
> - The previous fix also removed `blockedByIssueIds` mutation from
`ensureSourceIssueBlockedByStaleEvaluation` to break the
block→silence→new-alert loop, but that left no idempotency guard on the
source-issue escalation comment, so every critical scan re-appended the
comment to the source-issue thread
> - Additionally, runs whose source issue is already `blocked` (agent is
correctly idle, waiting on a human action) should never generate alerts
at all — silence is expected in that state
> - This PR fixes all three gaps in `createOrUpdateStaleRunEvaluation` /
`ensureSourceIssueCommentedForStaleEvaluation`: (1) skip when source
issue is `blocked`, (2) auto-record a dismissed_false_positive decision
when a closed evaluation exists with no prior watchdog decision, (3) add
an activity-log-backed idempotency guard so the source-issue escalation
comment fires exactly once per (sourceIssue, evaluationIssue) pair
across scan cycles and process restarts
> - The benefit is that agents correctly paused waiting on board-gated
blockers no longer generate repeated false-positive noise tickets,
board-closed evaluations are permanently suppressed without requiring a
second interaction, and source-issue threads no longer get spammed with
duplicate escalation comments
## What Changed
- `server/src/services/recovery/service.ts`:
- Added `blocked` source-issue guard: `if (sourceIssue?.status ===
"blocked") return { kind: "skipped" }` — idle output is expected when
the source issue is blocked
- Added `findClosedStaleRunEvaluation()` — queries for `done` evaluation
issues for a given run, ordered by most recent update (scoped to `done`
only so system-cancelled evaluations don't permanently suppress alerts)
- Added `hasDismissedFalsePositiveDecision()` — queries for an existing
dismissed_false_positive watchdog decision record
- Added closed-evaluation auto-dismiss: when a prior evaluation was
closed `done` on the board without any watchdog decision, auto-inserts a
dismissed_false_positive record so future scans skip via the existing
guard. The check-then-insert runs inside a transaction guarded by a
per-(company, run) `pg_advisory_xact_lock` so two overlapping scans
cannot both observe `hasAnyDecision = false` and both insert duplicate
rows
- Removed `blockedByIssueIds` mutation from the escalation path and
renamed `ensureSourceIssueBlockedByStaleEvaluation` →
`ensureSourceIssueCommentedForStaleEvaluation` to reflect that the
function now only adds a comment + activity log (no state mutation) —
evaluation issues are observability-only and adding them as hard
blockers created a self-amplifying loop (blocked→silent→new
alert→blocked again)
- Added activity-log-backed idempotency guard at the top of
`ensureSourceIssueCommentedForStaleEvaluation`: query the activity log
for a `heartbeat.output_stale_escalated` row with the same (sourceIssue,
evaluationIssue) pair and return false when one is present. The single
activity-log row written on the first successful escalation is the
suppression record for all later scans, surviving process restarts
- `server/src/__tests__/heartbeat-active-run-output-watchdog.test.ts`:
- Added: "emits the source-issue escalation comment only once across
repeated critical scans" (covers the comment-spam regression path)
- Added: "skips ticket creation when the source issue is blocked"
- Added: "suppresses repeat alerts when evaluation is closed on the
board without a watchdog decision"
- Added: "still allows re-arm after continue decision even when
evaluation is board-closed" (exception path: if any watchdog decision
exists, human opted in to lifecycle — honour it)
## Verification
```sh
pnpm exec vitest run server/src/__tests__/heartbeat-active-run-output-watchdog.test.ts
```
- All 18 watchdog tests pass locally
- Regression: source-issue escalation comment emits exactly once across
repeated critical scans
- Blocked source → no evaluation created (result.created === 0,
result.skipped === 1)
- Board-closed evaluation + no decisions → auto-records
dismissed_false_positive; second scan creates nothing
- Board-closed evaluation + continue decision → second scan still
creates (re-arm preserved)
## Risks
- **Low risk.** The blocked-status guard is a pure early-return that
adds no state mutation. The auto-dismiss path only inserts a record when
no decisions exist — it cannot fire for runs where a human has opted in
to the watchdog lifecycle via snooze/continue. Removing
`blockedByIssueIds` from the critical-escalation path is safe because
evaluation issues are already parented under the source issue.
- The `dismissed_false_positive` auto-insert is now race-safe under
concurrent scans via `pg_advisory_xact_lock` keyed on `(companyId,
runId)` so the check-then-insert pair is serialized without requiring a
schema change.
- `findClosedStaleRunEvaluation` is scoped to `done` only (not
`cancelled`) so system code paths that cancel evaluation issues cannot
permanently suppress future watchdog alerts for the same run.
## Model Used
- Provider: Anthropic
- Model: Claude Sonnet 4.6 (`claude-sonnet-4-6`) for original change;
Claude Opus 4.7 (`claude-opus-4-7`) for follow-up review fixes
- Context: full repo read with tool use, running as SADE agent in
Paperclip Claude Code
- Mode: agentic code analysis + targeted edit
## 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 (no UI changes)
- [x] I have considered and documented any risks above
- [x] I will address all Greptile and reviewer comments before
requesting merge
## Cross-references and status (maintainer)
- Closes #4937
- Closes #5207
- Closes #5767
- Closes #5949
Co-authored-by: Paperclip <noreply@paperclip.ing>
|
||
|
Vladimir Balko
|
deef1f479d |
fix(heartbeat): release execution lock on cross-agent reassignment (#5110)
## Thinking Path
> - Paperclip orchestrates AI agents for zero-human companies
> - Each issue can hold an execution lock via `issues.execution_run_id`,
so concurrent wakes for the same task either coalesce into the active
run or wait deferred
> - When the issue is reassigned to a *different* agent (e.g. board
operator changes `assigneeAgentId` from Coder → Reviewer + flips
`status` to `in_review`), the new assignee's wake is correctly sent down
the assignment-wakeup path
> - But the lookup `activeExecutionRun` still finds the previous holder
run as long as it is in `{queued, running, scheduled_retry}` — and
`enqueueAssignmentWakeup` falls through to the deferred-wake branch when
the holder agent does not match the new assignee
> - The trouble is the **queued** holder for the old assignee will never
start (the issue's status / target now belongs to someone else, the
relevant assignment trigger was the original one), so the lock is never
released, the deferred wake is never promoted, and the new assignee
silently never wakes
> - This pull request detects that situation right next to the existing
`cancelStaleScheduledRetry` cleanup: if `activeExecutionRun.status !==
'running'` AND the holder agent differs from `issue.assigneeAgentId`,
cancel the holder run, release the lock, and proceed with a normal
queued wake instead of deferring
> - The benefit is hand-offs across agents become reliable — no more
silent stalls that operators have to unstick by manually cancelling a
queued run
## Linked Issues or Issue Description
- Closes #4058
## What Changed
- One new check in `reapOrphanedRuns()`'s peer function — the
`enqueueAssignmentWakeup` defer-detection block in
`server/src/services/heartbeat.ts` (around the lock-resolution code
immediately following `cancelStaleScheduledRetry`):
- If `activeExecutionRun` exists, its `status !== 'running'`, and
`activeExecutionRun.agentId !== issue.assigneeAgentId`, mark the holder
run as `cancelled` with errorCode `lock_released_on_reassignment`,
cancel its corresponding wakeup request if any, and null
`activeExecutionRun` so the lock-clear branch right below proceeds to
release `executionRunId` / `executionAgentNameKey` / `executionLockedAt`
and the wake gets enqueued normally.
- `running` runs still defer (legitimate concurrency).
- Same-agent queued/scheduled holders still defer (legitimate coalesce).
- Total +37 lines, no API change, no schema change.
## Verification
```sh
# Existing reaper tests still pass — exercises the lock-resolution path
pnpm exec vitest run server/src/__tests__/heartbeat-process-recovery.test.ts --no-coverage
# expected: Tests 39 passed (39)
# New regression test for the cross-agent lock-release race
pnpm exec vitest run server/src/__tests__/heartbeat-lock-release-on-reassignment.test.ts --no-coverage
```
Manual reproduction (matches an incident we hit running a small Coder +
Reviewer company):
1. Coder pickup heartbeat schedule fires; paperclip queues a Coder run
and pre-allocates the lock by recording `issues.execution_run_id =
<queued-coder-run-id>` for the pickup issue.
2. The Coder run sits in `queued` because the agent's slot is busy
elsewhere (`maxConcurrentRuns: 1`).
3. Operator (or CEO) PATCHes the issue: `assigneeAgentId: <coder>` →
`<reviewer>` together with `status: in_progress` → `in_review`.
4. Paperclip creates the Reviewer assignment wakeup, but stores it as
`deferred_issue_execution` because `activeExecutionRun` is the queued
Coder run.
5. **Before this PR**: Reviewer never wakes; the deferred wakeup waits
for the queued Coder lock holder which never starts (the issue is no
longer the Coder's). Operator has to `POST
/api/heartbeat-runs/<queued-coder>/cancel` manually to unstick the
chain.
6. **After this PR**: paperclip recognizes the holder is non-running and
belongs to a now-foreign agent, cancels it inline, releases the lock,
and queues the Reviewer wake normally — Reviewer wakes on the next
heartbeat tick.
## Risks
- **Low**. The new branch only fires when both conditions are true:
- The holder run is **not** `running` — `running` runs still defer (we
never interrupt active work).
- `activeExecutionRun.agentId` is different from the issue's *current*
`assigneeAgentId` — i.e. the assignee was just changed, the old holder
is bound to the prior owner.
- The cancel uses errorCode `lock_released_on_reassignment` so operators
can grep for it; the corresponding wakeup is also cancelled in the same
transaction so we do not leave an orphan wakeup request.
- No DB schema change, no public API change, no UI change.
- Sits next to the existing `cancelStaleScheduledRetry` cleanup pattern,
so the behavior is locally consistent with how stale schedule retries
are already cleared.
## Model Used
- Claude Opus 4.7 (`claude-opus-4-7`), 1M-context build, extended
thinking + tool use enabled. Used to trace the lock-acquire / defer /
promote paths in `heartbeat.ts` from the live incident, design the
minimal-blast-radius fix next to `cancelStaleScheduledRetry`, and
produce this PR description.
## 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 (39 in the directly
affected suite, plus the new regression test)
- [x] I have added or updated tests where applicable
- [ ] If this change affects the UI, I have included before/after
screenshots — N/A (server-side wakeup routing)
- [x] I have updated relevant documentation to reflect my changes —
in-line code comment explains the new branch
- [x] I have considered and documented any risks above
- [x] I will address all Greptile and reviewer comments before
requesting merge
## Cross-references and status (maintainer)
- `Closes #4058`
### Maintainer-added changes on top of the original commit
A second commit was added on top of @vbalko-claimate's original to pin
the cancel `UPDATE` for the queued/scheduled holder to the exact
non-running status read just above it. Without that predicate, a worker
that flipped the holder from `queued` → `running` between the `SELECT`
and the `UPDATE` could have its freshly-claimed `running` row silently
clobbered to `cancelled`. The new commit also gates the wakeup-request
cancellation and the `activeExecutionRun = null` assignment on a
non-empty `RETURNING`, so neither fires when the predicate misses. A
dedicated regression test
(`heartbeat-lock-release-on-reassignment.test.ts`) covers both paths:
the legitimate-running-holder defer case and the queued→running race.
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Devin Foley <devin@devinfoley.com>
|
||
|
alcylu
|
9e81067678 |
fix: clear stale executionRunId on release, reassignment, and checkout (#2482)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Issues are the unit of agent assignment; each assignment queues a heartbeat run, and the agent claims ownership via a `checkout()` that sets `checkoutRunId` and `executionRunId` on the issue row. > - When a queued run never starts (crash, deploy, lost heartbeat) or a different run picks up the work, the issue is left with a stale `executionRunId` pointing at a terminal/missing run. > - The next checkout attempt fails with "Issue checkout conflict" because the fast-path `UPDATE` requires `executionRunId` to be null or equal to the requester's run id, so the row is permanently locked until an admin clears the column by hand. > - This pull request closes that lifecycle gap in three places — `release()` and `update()` clear the execution lock fields alongside the existing `checkoutRunId` clear, and `checkout()` gains a guarded stale-`executionRunId` adoption path that mirrors the existing `adoptStaleCheckoutRun` pattern. > - The benefit is that assignment-triggered issues self-heal after a lost run instead of paging an admin to unlock them, while the adoption path keeps the caller's `expectedStatuses` guard, preserves any pending `assigneeUserId`, and preserves the original `startedAt` for issues already `in_progress`. ## Linked Issues or Issue Description - Closes #759 - Closes #1015 - Closes #1276 - Closes #1298 - Closes #2265 - Closes #2661 - Closes #2964 - Closes #3559 - Closes #4033 - Closes #4131 ## What Changed - `server/src/services/issues.ts` — `release()` now clears `executionRunId`, `executionAgentNameKey`, and `executionLockedAt` alongside `checkoutRunId`. - `server/src/services/issues.ts` — `update()` clears the same execution-lock fields on status change (away from `in_progress`) and on assignee change. - `server/src/services/issues.ts` — `checkout()` gains a stale `executionRunId` adoption block that runs only when the row's `executionRunId` points at a terminal/missing heartbeat run, the caller's `expectedStatuses` still hold, and the requester is either the existing assignee or the assignee is null. The `SET` clause preserves `assigneeUserId` and only resets `startedAt` when the issue was not already `in_progress` (matches `adoptStaleCheckoutRun` semantics). - `server/src/__tests__/issues-service.test.ts` — two regression tests covering the new adoption guards: (1) checkout refuses to promote a `done` issue when `done` is not in `expectedStatuses`, even with a lingering `executionRunId` pointer; (2) checkout adoption of a stale `checkoutRunId` preserves the issue's `assigneeUserId`. ## Verification - `vitest run src/__tests__/issues-service.test.ts` — 75/75 tests pass, including the two new regression tests. - `tsc --noEmit` clean. - Manual repro of the original stuck-lock case: queue a run, mark the heartbeat run terminal without releasing the issue, attempt a new checkout — the adoption path now succeeds with the caller's `expectedStatuses` guard intact instead of returning a checkout conflict. ## Risks - Low risk. The `release()` and `update()` changes are additive field clears alongside the existing `checkoutRunId` clear and follow the same conditions. The `checkout()` adoption block is gated by the same status / assignee / expected-statuses constraints as the fast-path `UPDATE` and only fires when the prior run is verifiably terminal via `isTerminalOrMissingHeartbeatRun()`. No migration. No public API change. ## Model Used - Claude Opus 4.7 (`claude-opus-4-7`), extended-thinking mode, tool-use enabled (file reads, edits, shell, gh CLI). Used to address review feedback on the original commit by Allen Lu (`alcylu`); follow-up fix commit preserves the `expectedStatuses` guard, `assigneeUserId`, and `startedAt` and adds regression tests. ## 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 (N/A — server-only) - [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 #759 - Closes #1015 - Closes #1276 - Closes #1298 - Closes #2265 - Closes #2661 - Closes #2964 - Closes #3559 - Closes #4033 - Closes #4131 --------- Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Devin Foley <devin@paperclip.ing> |
||
|
NiViGaHo
|
7945c70396 |
fix(issues): reopen-guard for assignee self-comment on terminal issue (AKS-1563) (#4346)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Issues are the unit of agent work, and a "done" issue should stay done unless something explicit reopens it > - The implicit-reopen path (human comment on a terminal issue) already keeps agents from reopening their own issues via `shouldImplicitlyMoveCommentedIssueToTodo`, but the explicit `reopen: true` path was not similarly guarded > - That gap lets the assignee agent reopen its own `done`/`cancelled` issue just by posting a log-style comment with `reopen: true` — the same "log lines are not reopen signals" semantics that the implicit path already encodes > - This pull request adds a focused `isAssigneeSelfCommentOnTerminalIssue` guard applied at both `PATCH /issues/:id` and `POST /issues/:id/comments`, forcing `effectiveMoveToTodoRequested = false` when the actor is an agent commenting on its own terminal issue without `resume: true` > - The benefit is a single, narrow invariant: only an explicit `resume: true` (or a different-agent / human commenter) reopens a terminal issue — assignee self-comments stay communicative ## Linked Issues or Issue Description Refs #3980 Refs #3935 Refs #6601 ## What Changed - Adds `isAssigneeSelfCommentOnTerminalIssue` helper in `server/src/routes/issues.ts` next to the existing `shouldImplicitlyMoveCommentedIssueToTodo` - Applies the guard at both comment entry paths (`PATCH /issues/:id` with a `comment` body and `POST /issues/:id/comments`) so `effectiveMoveToTodoRequested` is forced to `false` when actor is an agent and matches the **current** assignee of a `done`/`cancelled` issue — even if `reopen: true` was sent explicitly - PATCH path compares against `existing.assigneeAgentId` (not `requestedAssigneeAgentId`), so a different agent that PATCHes a terminal issue with `{ comment, reopen: true, assigneeAgentId: <self> }` still reopens as today - The `resume: true` explicit-resume path is preserved verbatim — the guard short-circuits on `resumeRequested` - Existing external-caller paths (different agent / human user commenting on terminal) are unchanged and still reopen - New unit tests in `server/src/__tests__/issue-comment-reopen-routes.test.ts`: - `does not reopen via POST comment+reopen when the assignee agent is the actor on a done issue` - `does not reopen via POST comment+reopen when the assignee agent is the actor on a cancelled issue` - `does not reopen via PATCH comment+reopen when the assignee agent is the actor on a done issue` - `still reopens a done issue via PATCH when a different agent reassigns to self with reopen=true` ## Verification - [x] `vitest run src/__tests__/issue-comment-reopen-routes.test.ts` — 65/65 pass locally (4 new + 61 existing) - [x] `tsc --noEmit` — no new errors in changed files - [x] Manual trace: explicit-resume path (`resume: true`) still reopens because the guard short-circuits on `resumeRequested` ## Risks Low. The guard is a single short-circuit before the existing reopen decision and only fires when actor is an agent commenting on its own `done`/`cancelled` issue without `resume: true`. The `resume: true` path is unchanged, and the PATCH comparison uses the current assignee so cross-agent takeover with `reopen: true` continues to reopen. ## Model Used - Provider: Anthropic - Model: Claude Opus 4.7 (`claude-opus-4-7`) - Mode: extended thinking + tool use (Claude Code agent harness) ## 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 — server-only) - [x] I have updated relevant documentation to reflect my changes (no docs touch the reopen guard) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --- ## Cross-references and status (maintainer) Rebased on current `master`. The implicit-reopen case is already handled upstream by the user-actor branch of `shouldImplicitlyMoveCommentedIssueToTodo`; this PR adds the matching guard for the explicit `reopen: true` path. The PATCH-path guard compares against `existing.assigneeAgentId` so cross-agent reassignment + reopen still reopens. Refs #3980 Refs #3935 Refs #6601 Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> |
||
|
Dale Carman
|
d782c4cd53 |
fix(heartbeat): prevent zombie run coalescing and ensure startup reap completes before timer ticks (#1731)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - Agents run in heartbeats — short execution windows triggered by the heartbeat service > - The heartbeat service coalesces overlapping wakeups: if a run for an agent is already active, a new wakeup merges into it rather than creating a duplicate > - But when the server restarts, in-progress runs are left in `"running"` status in the database — their child processes are gone, but the DB rows persist as orphans > - The startup `reapOrphanedRuns()` was fired as a `void` promise — the timer interval started immediately in parallel, so the first timer tick could coalesce a new wakeup into an orphaned "running" row before the reap had a chance to remove it > - Once coalesced, the orphan's `updatedAt` refreshed, making the reaper skip it as "not old enough" — a zombie run that prevents the agent from ever waking again > - This PR fixes both the coalescing guard (do not coalesce into a zombie) and the startup ordering (await reap before starting the timer), eliminating the death spiral ## What Changed - **`server/src/index.ts`** — `startServer` now `await`s `reapOrphanedRuns()` (with one retry) before calling `setInterval`. Timer ticks cannot start until orphaned runs are cleaned up. - **`server/src/services/heartbeat.ts`** — Added two exported pure functions: - `isZombieRun(run, tracked)` — returns `true` if a run is `"running"` in the DB but has no live entry in the in-memory `runningProcesses` Map - `filterZombieCoalesceTarget(target, tracked)` — returns `null` if the coalesce candidate is a zombie, letting the wakeup fall through to create a new queued run instead - Both coalescing call sites now use `filterZombieCoalesceTarget` before deciding to coalesce - **`server/src/__tests__/heartbeat-zombie-guard.test.ts`** — 8 new behavioral tests covering `isZombieRun` and `filterZombieCoalesceTarget`, including the critical zombie scenario, legitimate live runs, queued runs (must never be filtered), and null pass-through ## Verification ```bash # Run the new tests pnpm test:run ``` Manual reproduction (before fix): 1. Start an agent on a timer heartbeat 2. Kill the server mid-run (child process dies, DB row stays `"running"`) 3. Restart the server 4. Observe: agent never wakes again — subsequent wakeups coalesce into the dead run, refreshing `updatedAt`, keeping it alive forever After fix: startup reap clears the orphan before the timer starts; subsequent wakeups create fresh queued runs. The one pre-existing test failure (`worktree helpers > copies shared git hooks`) is unrelated — it fails on `upstream/master` as well due to a `pnpm install` failure in the test environment. ## Risks - **Startup latency**: `await reapOrphanedRuns()` adds a small delay before the timer starts. In practice this is a fast DB query. The retry adds at most one extra attempt on transient failure. - **Behavior change**: Wakeups that previously coalesced into zombie runs will now create new queued runs instead. This is the correct behavior — the zombie was preventing any forward progress. - **Queued runs unaffected**: `isZombieRun` only flags `"running"` status. Queued runs pass through `filterZombieCoalesceTarget` unchanged (covered by tests). ## Model Used - OpenAI GPT-5 via a Codex-style terminal coding agent with tool use and git/gh access. Exact hosted alias is not exposed in this environment. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [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 ## Cross-references and status (maintainer) Refs #3168 Refs #4174 Refs #4697 Refs #6399 Related PRs checked: #4075, #4705, #5232, #6952 - [x] I have searched GitHub for duplicate or related PRs and linked them above --------- Co-authored-by: Devin Foley <devin@paperclip.ing> |
||
|
sunghere
|
130219c0be |
fix(recovery): exempt stranded escalation when assignee shows recent visible progress (#5213)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - The recovery service watches `in_progress` agent-assigned issues every 30s and creates "Recover stalled issue …" child issues when execution looks stranded > - The `isRepeatedProductiveContinuationRecovery` branch escalates after just **two consecutive productive continuation runs** — fine for genuinely stuck agents that loop without doing anything, but a false positive for batch workflows that legitimately advance every heartbeat (e.g. multi-frame image generation that produces 1–2 frames + an attachment per heartbeat) > - In production this fired ~95 times for a 19-character batch run, burning a recovery owner heartbeat each time > - This pull request adds a "recent visible progress" exemption: if the assignee posted a comment or any attachment within the exemption window (default 30 min, env-tunable, 60s floor), skip the escalation and let the normal continuation-retry path enqueue the next wake > - The benefit is one platform tweak unblocks all current and future batch workflows without weakening the genuinely-stuck case — agents that go silent still escalate after the window elapses ## What Changed - `server/src/services/recovery/service.ts` - new `STRANDED_RECENT_PROGRESS_EXEMPTION_MS` constant (default 30 min, override via env, floored at 60s) - new `hasRecentVisibleProgress(companyId, issueId, assigneeAgentId, windowMs)` helper — single parallel query against `issue_comments` (filtered by `authorAgentId`) + `issue_attachments`, both using existing indexes - in `reconcileStrandedAssignedIssues`, the `isRepeatedProductiveContinuationRecovery` branch now consults the helper before escalating; on exemption it falls through to the existing continuation-retry enqueue path - new `recentProgressExempted` counter on the reconcile result, surfaced in the periodic recovery log via the existing `...reconciled` spread - `server/src/__tests__/heartbeat-process-recovery.test.ts` - new test: recent agent comment → no escalation, continuation re-queued, `recentProgressExempted: 1` - new test: stale (24h-old) agent comment → escalation still fires as before ## Verification - `pnpm typecheck` — green across the workspace - `pnpm exec vitest run server/src/__tests__/heartbeat-process-recovery.test.ts` — 39/39 pass (37 pre-existing + 2 new) - Smoke after deploy: confirm Image Spec multi-frame generation no longer creates `Recover stalled issue …` child issues per heartbeat ## Risks - **Behavioral shift, low blast radius.** A genuinely-stuck agent that posts cosmetic comments every <30 min would now escalate later instead of immediately. Mitigated by: - Window is configurable via `STRANDED_RECENT_PROGRESS_EXEMPTION_MS` - Other escalation paths are untouched (failed/cancelled/timed_out runs still escalate immediately, paused-tree handling unchanged, recovery-issue-on-recovery guard unchanged) - Periodic recovery log now reports `recentProgressExempted` so a runaway exemption is visible in operations - No DB migration required — both `issue_comments` and `issue_attachments` queries use existing indexes - Backward compatible: pre-existing test "blocks stranded in-progress work after a productive continuation retry was already used" still passes unchanged because no comment is seeded → no exemption → escalates ## Model Used - Claude Opus 4.7 (`claude-opus-4-7`), extended thinking, tool use enabled. Investigation, change, tests, and PR body all human-supervised through a Paperclip agent heartbeat. ## 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 — N/A, server-only - [ ] I have updated relevant documentation to reflect my changes — no docs touched the previous behavior; the env knob is self-documenting via the comment in `service.ts` - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge ## Cross-references and status (maintainer) Rebased onto current `master`. No duplicate PRs absorbed. Refs #6072 — related open report in the same `reconcileStrandedAssignedIssues` / `isRepeatedProductiveContinuationRecovery` family (stale productive-continuation evidence). This PR does not fix #6072, but the recent-visible-progress exemption added here shrinks the false-positive surface in that branch and the new `recentProgressExempted` counter gives operators visibility into the broader escalation path. Co-authored-by: sunghere <sunghere@users.noreply.github.com> Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
Lempkey
|
e1e2cef928 |
fix(issues): accept array-form ?status= filter and stop crashing on repeated keys (#4628) (#4890)
## Thinking Path
> - Paperclip orchestrates AI agents for zero-human companies
> - Boards, agents, and the public REST API all read issue lists via
`GET /api/companies/:cid/issues`, with `?status=` as the most-common
filter
> - Express's default `qs` parser binds repeated keys to a `string[]` —
the conventional URL form `?status=todo&status=in_progress` is therefore
valid input
> - The service layer treated `filters.status` as a string and called
`.split(",")` unconditionally, returning HTTP 500 with `TypeError:
filters.status.split is not a function`. The same buggy pattern lived at
a second call site in the same file
> - This PR adds a small `parseStatusFilter` helper that normalizes all
four shapes the route can receive, routes both service-layer call sites
through it, and widens the `IssueFilters.status` type so the contract
stops lying about runtime reality
> - The benefit is a passive 500 disappears for any client (curl, board,
agent code) that builds `?status=` with array-style binding, and the
type system now forces every future caller to handle both shapes
correctly
## Linked Issues or Issue Description
- Refs #4628
- Closes #4084
- Related earlier attempt: #1964
## What Changed
- **`server/src/services/issues.ts`** — Added exported helper
`parseStatusFilter(input: string | readonly string[] | undefined):
string[]` that normalizes single strings, CSV
(`?status=todo,in_progress`), array (`?status=todo&status=in_progress`),
and mixed array+CSV; trims and filters empties. Widened
`IssueFilters.status` from `string` to `string | readonly string[]`.
Replaced inline `.split` call sites in `list()`, blocked-count
filtering, `count()`, and `countUnreadTouchedByUser()` with
helper-driven branching.
- **`server/src/routes/issues.ts`** — Replaced dishonest
`req.query.status` casts with `string | string[] | undefined` at both
issue-list and blocked-count entry points so the route contract matches
Express `qs` runtime behavior.
- **`server/src/__tests__/parse-status-filter.test.ts`** (new) — 10 unit
cases: undefined, empty string, single, CSV, array, mixed array+CSV,
whitespace trim, trailing/extra commas, no-mutation guarantee, hostile
non-string entry guard.
- **`server/src/__tests__/issues-list-query-parsing.test.ts`** (new) — 5
supertest cases against a minimal Express app whose handler mirrors the
route cast/forwarding pattern: single, CSV, repeated-key array, mixed
array+CSV, and no `?status` param.
- **`server/src/__tests__/issues-service.test.ts`** — Added
embedded-Postgres service coverage for array-form status filters through
`list`, `count`, and `countUnreadTouchedByUser` on current master.
**Why service-layer, not route-layer:** the bug is the service contract.
Fixing only at the route would leave other service-layer call sites
latent, keep `IssueFilters.status` inaccurate, and let future internal
callers reintroduce the same crash. Widening the type is the forcing
function that prevents recurrence.
**Why `parseStatusFilter` is exported, not file-local:** the helper has
direct unit coverage and keeps the normalization logic colocated with
its only current call sites.
## Coordination with prior work
- Supersedes **#4084** (thanks to @adlai88 for the original
report-and-fix). This PR additionally fixes the extra current-master
service call sites, widens `IssueFilters.status` so the type contract is
honest, replaces the incorrect route casts, and ships direct regression
coverage.
- **#1964** bundles unrelated route/service changes; this PR keeps scope
tight per CONTRIBUTING.md's one-PR-one-change guidance.
## Out-of-scope finding
While verifying all query-string status parsing sites, I found a sibling
bug in `server/src/services/execution-workspaces.ts:409` reachable from
`routes/execution-workspaces.ts:48`, where repeated `?status=` keys can
still hit the same `.split(",")` assumption. I left that out of this PR
to keep the review surface small.
## Verification
```bash
pnpm --filter @paperclipai/server exec vitest run \
src/__tests__/parse-status-filter.test.ts \
src/__tests__/issues-list-query-parsing.test.ts \
src/__tests__/issues-service.test.ts \
--testNamePattern='parseStatusFilter|issue list status query parsing|accepts array-form status filters in list and count|excludes plugin operation issues from unread inbox counts'
```
Result on the rebased head: `3` files passed, `17` tests passed, `72`
skipped.
GitHub CI on PR `#4890` is green on the rebased head
`805731d3270783d0b80b33ee1dccdc6771febef6`, including `verify`,
`Typecheck + Release Registry`, `Build`, `e2e`, general tests,
serialized suites, Socket checks, Snyk, and `Greptile Review`.
Local workspace typecheck commands still encounter unrelated
current-master baseline errors under `packages/plugins/sdk` and
`server/src/services/company-skills.ts`; no failures were produced from
the `issues` files changed in this PR.
## Risks
- **Type widening blast radius:** `IssueFilters.status` widens from
`string` to `string | readonly string[]`. Any direct caller that still
assumes `.split()` on the input now gets a useful typecheck failure
instead of a latent runtime crash.
- **Behavior change:** `?status=todo,in_progress&status=done` previously
returned HTTP 500; it now returns HTTP 200 with the union of matching
statuses. Single-string and CSV behavior remain unchanged.
- **No migration. No breaking API changes. No new deps. No UI changes.**
> For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and
discuss it in `#dev` before opening the PR. — Confirmed: this is a bug
fix, not a feature. ROADMAP.md grep showed no overlap.
## Model Used
- **Claude Opus 4.7** (Anthropic), `claude-opus-4-7`, 1M context,
extended thinking. Used for problem scoping, implementation, and test
authoring; the final rebasing, PR prep, and verification updates were
handled in the maintainer workflow.
## 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
- [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 #4084
---------
Co-authored-by: Devin Foley <devin@paperclip.ing>
|
||
|
Jannes Stubbemann
|
70357b961f |
feat(security): per-company JWT signing keys for multi-tenant isolation (#5864)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - Agents authenticate to the server with a JWT signed by the deployment's master secret > - In a multi-tenant deployment, all agents from every tenant are signed with the *same* key, so a leak (CI/staging dump, hostile contractor with infra access, supply-chain) lets the attacker mint tokens for *any* tenant > - The same master secret also issued tokens with a 48-hour TTL, giving any leaked token a two-day window of validity even after rotation > - This pull request derives a per-company signing key via `HMAC-SHA256(master, "jwt:<companyId>")` and reduces the default TTL to 1h; the verifier tries the per-company key first and falls back to the master secret only for tokens issued before this change so no agent gets locked out on deploy > - The benefit is multi-tenant key isolation (a leak of one company's derived key cannot forge tokens for another) and a tighter blast-radius on any leaked token, with zero local-first impact (single-tenant deploys derive their one company's key the same way and continue to work unchanged) ## Linked Issues or Issue Description Refs #5288 — a separate key-hygiene finding in the same module (`agent-auth-jwt.ts` falls back to `BETTER_AUTH_SECRET` as the JWT signing secret). Related agent-JWT trust-model concern, but not fixed by this PR — the master-secret fallback selection is unchanged here. No existing issue covers this PR's problem directly — described in-PR: - In a multi-tenant deployment, agents from every tenant get JWTs signed with the *same* master key, so a single leak (CI/staging dump, hostile contractor, supply chain) lets the attacker mint tokens for *any* tenant. - The same master secret issued tokens with a 48-hour TTL, giving any leaked token a two-day validity window even after rotation. - Fix: derive a per-company signing key via `HMAC-SHA256(master, "jwt:<companyId>")` and reduce the default TTL to 1h, with a master-secret verification fallback so pre-existing tokens are not locked out on deploy. ## What Changed - **`server/src/agent-auth-jwt.ts`** - New `deriveCompanySigningKey(masterSecret, companyId)` — `HMAC-SHA256` with domain-separated input (`jwt:<companyId>`) so the master secret can be safely reused for other HMAC purposes in the future without cross-protocol risk. - `signAgentJwt` always signs with the derived per-company key. - `verifyAgentJwt` reads `company_id` from the token's (untrusted) claim payload, looks up the candidate derived key, and verifies. If that fails AND a master secret is set, it falls back to verifying with the raw master secret — pre-existing tokens validate until they expire. Verification still fails if the signature doesn't bind. - Default TTL: `60 * 60 * 48` → `60 * 60`. Existing `PAPERCLIP_AGENT_JWT_TTL_SECONDS` override still wins. - **`PAPERCLIP_AGENT_JWT_DISABLE_LEGACY_FALLBACK`** (optional, default off) — operators set this ~one TTL after deploying to sunset the master-secret verification fallback entirely, closing the window in which a leaked master secret could forge arbitrary-`exp` tokens for any tenant. - **`server/src/__tests__/agent-auth-jwt.test.ts`** (6 new cases) - Per-company isolation via tamper: token for company A fails when verified for company B. - Legacy-token verification path: tokens signed with the raw master secret still verify. - Default TTL is 1h. - Legacy fallback toggle: master-secret tokens accepted when unset, rejected when enabled, and per-company tokens unaffected either way. ## Verification - `pnpm --filter @paperclipai/server run typecheck` — clean. - `npx vitest run agent-auth-jwt` — 11/11 pass (6 new + 5 existing). - Manual: token signed for company A under per-company key fails when verified against company B's derived key. ## Risks - **Backward-compatible verification**, so no agent gets locked out on deploy — but operators relying on hot-swapping the master secret should note that pre-existing tokens *will* keep validating against the master key until their TTL elapses, unless `PAPERCLIP_AGENT_JWT_DISABLE_LEGACY_FALLBACK=true` is set to end the fallback window explicitly. - **TTL reduction is a default, not a hard cap.** Operators who relied on the 48h window can override via env. If 1h is too aggressive for upstream taste, happy to gate the change behind an env var. - **No new required env vars.** Single-tenant local-first deploys derive one company's key the same way and behave identically to today. - **Domain-separated HMAC input** (`jwt:<companyId>`) means the master secret can be safely reused for other future HMAC purposes without cross-protocol risk. ## Model Used Claude Opus 4.7 (1M context), extended thinking mode; rebase + legacy-fallback sunset documentation 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 (`agent-auth-jwt` 11/11) - [x] Added per-company-isolation, legacy-fallback, and TTL-default tests - [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 #3967 (cross-tenant 404 oracle) and #5865 (plugin tables `company_id`). --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Jannes Stubbemann
|
606e74d11f |
cloud_tenant: company-scoped tenants, never instance-admin (#7525)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies, and a single server instance can host many companies. > - The auth middleware (`server/src/middleware/auth.ts`) supports a `cloud_tenant` mode where a trusted hosting proxy injects per-request identity headers, designed originally for one-deployment-per-tenant setups. > - In that original setup, granting every cloud tenant the `instance_admin` role was harmless; on a **shared, multi-tenant pool** it means any paying tenant is admin of the whole instance and can reach every other tenant's data. > - A tenant only needs to own its own company — which it already gets via the company membership the same code path upserts — so instance-level admin is never appropriate for `cloud_tenant` actors. > - This PR removes the `instance_admin` grant from the cloud-tenant path and pins `isInstanceAdmin: false` on the resolved actor. > - Greptile review then surfaced a follow-up gap: deployments that ran the pre-hardening build still have stale `instance_admin` rows in `instance_user_roles`, which other lookups (BetterAuth session path, board API keys, and the authorization service's own DB re-check) would still honor. > - The follow-up commit closes that gap by purging stale rows at the cloud-tenant auth boundary and by teaching the authorization service that `cloud_tenant` actors are never instance admins. > - The benefit is that shared-pool hosting becomes structurally safe: tenants are company-scoped owners, never instance admins — including on deployments upgrading from the older behavior. ## Linked Issues - Refs #966 — managed SaaS multi-tenant hosting is the deployment shape this hardening protects. - Refs #5015 — same problem space: instance-admin-scoped credentials are too broad for multi-company instances; tenants need company-scoped access. Neither issue is fully closed by this PR; it removes the instance-admin grant from the `cloud_tenant` trusted-header path specifically. ## What Changed - `server/src/middleware/auth.ts` - Removed the `instanceUserRoles` insert that granted every cloud tenant `instance_admin`; `resolveCloudTenantActor` now returns `isInstanceAdmin: false` (was `true`). - `resolveCloudTenantActor` now **deletes** any stale `instance_admin` row for the authenticated tenant user on every trusted-header request, so grants left behind by pre-hardening deployments are purged at the source (closes the Greptile P2: stale rows could otherwise re-elevate the user via the BetterAuth session path, board API keys, or the authorization service). - The function is `export`ed so it can be unit-tested directly. - `server/src/services/authorization.ts` - `authorizationService` previously re-checked `instanceUserRoles` from the DB regardless of the actor flag, which would have elevated even hardened `cloud_tenant` actors while a stale row lingered. Actors with `source === "cloud_tenant"` are now never elevated to instance admin; other board actors keep the existing lookup. - `server/src/services/authorization.ts` + `server/src/middleware/auth.ts` (follow-up commit `dc57a71c7`) - CI on the merge ref surfaced that elevation removal alone strands real cloud tenant users: board actors only ever reached `issue:read` / `issue:mutate` through instance-admin elevation (`permissionForAction` maps both to no grant key). `decide()` now grants `cloud_tenant` actors with an **active membership in the resource company** the same read surface as a same-company agent (`agent:read`, `company_scope:read`, `issue:read`, `project:read`) plus `issue:mutate` for non-viewer members — cross-company access stays denied (new `allow_company_member` reason). - `resolveCloudTenantActor` seeds the standard role-default permission grants (`ensureHumanRoleDefaultGrants`) so granted actions (e.g. `tasks:assign`, `agents:create` for owners) work without elevation. - Master-side route tests that stubbed cloud tenant actors with `isInstanceAdmin: true` now seed a real membership and assert under the hardened contract (`issue-identifier-routes`, `multilingual-issues-routes`, `issue-comment-redaction`). - Tests - `server/src/middleware/cloud-tenant-actor.test.ts` (new): cloud tenant is never instance-admin, is scoped to exactly the one company from its stack, still upserts user/company/membership, purges stale `instance_admin` rows, returns null without the server token, and maps non-owner stack roles without elevating. - `server/src/__tests__/auth-session-route.test.ts`: end-to-end middleware regression — a user with a stale `instance_admin` row stops being elevated via the session path once they authenticate through the cloud-tenant path (with a control assertion showing the pre-purge elevation). - `server/src/__tests__/authorization-service.test.ts` (embedded Postgres): a `cloud_tenant` actor with a stale `instance_admin` row in the real DB cannot cross company boundaries, while a `session` actor with the same row still resolves `allow_instance_admin`. ## Verification Run from the repo root after `pnpm install --frozen-lockfile`: ```bash cd server npx vitest run src/middleware/cloud-tenant-actor.test.ts src/__tests__/auth-session-route.test.ts # 9 tests passed npx vitest run src/__tests__/authorization-service.test.ts # 16 tests passed (embedded Postgres) pnpm typecheck # clean ``` Also ran the broader auth-related suites locally (`auth-routes`, `authz-company-access`, `better-auth`, `adapter-routes-authz`, `express5-auth-wildcard`): 8 files, 58 tests, all passing. ## Risks - **This touches authentication and authorization paths directly.** Mistakes here are security bugs in both directions; review accordingly. - **Behavioral change for existing `cloud_tenant` deployments:** tenants that previously (incorrectly) had instance-admin lose it — including the ability to see/manage other companies on the instance. This is the intended hardening, but any single-tenant deployment that relied on the cloud-tenant identity for instance administration must provision a separate admin identity. - **The purge is destructive by design:** if an operator's instance-admin identity is *also* provisioned through the cloud-tenant headers (same user id), its `instance_admin` row will be deleted on the next trusted-header request. Operators should hold admin through a non-cloud-tenant identity. - **Residual gap (documented, not fixed here):** a deployment that ran the old cloud_tenant build and then *disabled* cloud-tenant mode keeps stale rows until the affected user re-authenticates through the cloud path. A data migration was considered and deliberately avoided: there is no reliable SQL predicate for "cloud-tenant-provisioned user" (no source column), so a migration risks deleting legitimate admins. - No schema or migration changes; no UI changes. ## Model Used - Claude Fable 5 (claude-fable-5, 1M context), extended thinking + tool use, via Claude Code — this revision; original PR authored in an earlier Claude Code session. ## 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 (none duplicate this; related issues Refs #966 / #5015 are linked in the issue section) - [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 (no UI changes) - [x] I have updated relevant documentation to reflect my changes (no existing docs reference `cloud_tenant` mode) - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Harshit Khemani
|
d7f2f88323 |
fix(server): allow board members the null-mapped visibility actions agents already get (#7890) (#7935)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The authorization service (`server/src/services/authorization.ts`) decides every actor's actions; `permissionForAction()` intentionally maps read/visibility actions (`agent:read`, `issue:read`, `project:read`, `company_scope:read`, `runtime:manage`, `secrets:read`) to `null`, meaning "no explicit database grant required" > - The board-actor path's `if (!permissionKey) return deny(deny_unsupported_action)` guard caught those null-mapped actions *before* any membership-based evaluation, contradicting the intentional null mapping > - Result (#7890): board users with active company membership see "You have no agents" on the Dashboard — `filterAgentsForActor()` drops every agent because `access.decide({action: "agent:read"})` denies > - This pull request allows exactly those six actions for board users with an active company membership, mirroring the agent actor path's standard-trust policy so board and agent actors behave consistently > - The benefit is board members can actually see their company's agents, issues, and projects, while everything else (including `agent:wake` and `issue:mutate`, which have no board analog today) keeps its existing deny ## Linked Issues or Issue Description Fixes #7890 ## What Changed - `server/src/services/authorization.ts`: inside the board path's null-`permissionKey` branch, the six null-mapped visibility actions (`agent:read`, `company_scope:read`, `issue:read`, `project:read`, `runtime:manage`, `secrets:read`) now resolve via `getActiveMembership` — active membership → `allow` with the pre-existing `allow_simple_company_member` reason; no membership → `deny_missing_membership`. All other null-mapped actions (`agent:wake`, `issue:mutate`) keep `deny_unsupported_action`. - `server/src/__tests__/authorization-service.test.ts`: three regression tests in the existing embedded-postgres suite — member allowed the visibility actions, non-member denied with `deny_missing_membership`, and `agent:wake`/`issue:mutate` still denied. ## Verification - `npx vitest run server/src/__tests__/authorization-service.test.ts` → 20 passed (17 pre-existing + 3 new) against embedded postgres. - `pnpm --filter @paperclipai/server typecheck` → clean. - Policy rationale: the agent actor path's standard-trust branch already allows these same six actions company-wide (`allow_company_agent`); this PR gives board members the identical set, per the issue's note that the null mapping means "no explicit grant needed". `agent:wake` is self-only for agents and `issue:mutate` is assignee-gated — neither has a board semantic today (no route invokes them for board actors), so both intentionally keep the unsupported-action deny. ## Risks - This is authorization code, so reviewed conservatively: the change only affects the board (session user) path, only for actions that returned `null` from `permissionForAction()`, and only flips deny→allow when an **active** company membership exists. Instance admins and `local_implicit` boards were already allowed via earlier short-circuits. - Viewer members keep the four read-only visibility actions but are denied `runtime:manage` and `secrets:read` (`deny_missing_grant`), matching the `tasks:assign` viewer carve-out in the same board block (added in review follow-up 55f3b40). ## Model Used - Claude Fable 5 (`claude-fable-5`, Anthropic) via Claude Code, agentic mode with tool use (subagent implementation + independent adversarial review subagent), extended thinking 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 (none found for #7890) - [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 — server-only; the UI symptom is "no agents" with no styling change) - [x] I have updated relevant documentation to reflect my changes (N/A — no docs describe the board permission mapping) - [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: Claude Fable 5 <noreply@anthropic.com> |
||
|
ymmot
|
7058d7b6c3 |
fix: auto-complete approved review comments (#5839)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Issue lifecycle and review handoff rely on a comment-driven "approve" gesture from the active reviewer to transition `in_review` → `done` > - The original auto-completion path matched approval markers loosely and split the comment insert from the status transition, which let `NOT APPROVED` close issues and let a 422-on-status-change leave an orphan comment behind > - That broke the safety expectation that a rejection comment can never auto-complete an issue, and that observable state (comment+status) cannot diverge from intended state > - This pull request tightens the approval regex against negated phrasings and wraps the comment insert + status transition + execution decision in one transaction so a failed transition rolls the comment back > - The benefit is that reviewers can post negated phrasings safely, and any failure in the auto-approval transition leaves the thread unchanged instead of in a half-applied state ## Linked Issues or Issue Description ### What happened? The comment-driven auto-approval path in `routes/issues.ts` had two latent safety bugs surfaced during review: 1. The approval-detection regex matched negated phrasings such as `NOT APPROVED`, `NOT APPROVED.`, `Do not approve`, `Not approving this`, so a reviewer comment intended as a rejection could auto-complete the issue. 2. The auto-approval insert + status transition + execution decision were not atomic. If the post-comment status update returned 422 (`unprocessable`), the persisted approval comment was left behind without the corresponding state change, leaving the thread half-applied. ### Expected behavior - Negated approval phrasings (`NOT APPROVED`, `not approved.`, `I do not approve`, `not approving this`, etc.) must never trigger auto-completion. Positive controls (`Approved`, `LGTM, approved`) must continue to trigger it. - A failed status transition must roll back the corresponding approval comment so observable state and intended state never diverge. ### Steps to reproduce 1. Open an `in_review` issue assigned to a reviewer. 2. As the reviewer, post `NOT APPROVED` as a comment. 3. Prior to this fix: the issue auto-transitions to `done`. After this fix: the issue stays `in_review`, the comment lands, and no transition fires. 4. Separately, induce a 422 on the post-approval status update (e.g. concurrent delete). Prior to this fix: the approval comment is persisted but the issue stays `in_review`. After this fix: the comment is rolled back along with the failed transition. ### Paperclip version or commit Branch tip `ca60f00276` at the time of this submission. Targets `master`. ### Deployment mode Affects both hosted and self-hosted deployments. Behavior is server-side only. ## What Changed - Tightened the review-marker approval regex in `server/src/routes/issues.ts` so it rejects negated phrasings while preserving positive controls. - Required structured `kind: review` / `decision: approved` metadata adjacent to the markdown approval marker (rejects blank-separated structured approval and mismatched actor kinds). - Wrapped the auto-approval comment insert, status transition, and execution decision in a single drizzle transaction in `server/src/routes/issues.ts`, threading the transaction handle through `addComment` in `server/src/services/issues.ts` so a concurrent delete or 422 transition rolls back the comment. - Added a dedicated activity log entry for the post-approval status transition and skipped stale `issue_commented` wakes that arrive after the auto-approval transition. - Added 61 regression tests in `server/src/__tests__/issue-comment-reopen-routes.test.ts` covering negated phrasings, positive controls, structured-metadata adjacency, actor-kind matching, atomic rollback on transition failure, and stale-wake suppression. ## Verification - `cd server && npx vitest run src/__tests__/issue-comment-reopen-routes.test.ts` → 61/61 passing locally. - Typecheck on changed files passes locally. - All required CI checks expected to be green on this branch tip. ## Risks - Low risk. Changes are localized to the comment-driven auto-approval path; existing `addComment` callers are unaffected (the new transaction handle parameter is optional and defaults to the top-level `db`). - The transaction wrapper changes observable timing very slightly (single tx vs. two-step), but the only externally visible effect is atomicity — failures now leave no orphan state. - Regex tightening is opt-out safe (positive controls still match) but if a reviewer in the wild used a creative phrasing not covered by the regression set, they may need to repost as `Approved`. ## Model Used - Provider: Anthropic - Model: Claude Opus 4.7 (`claude-opus-4-7`) - Capabilities: extended thinking, tool use, code execution ## Checklist - [x] I searched for similar open/closed PRs and confirmed this is not a duplicate - [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 either linked existing issues or 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] I have considered and documented any risks above --------- Co-authored-by: Tommy <tommy@Mac-mini-Anton.local> Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Devin Foley <devin@paperclip.ing> |
||
|
Devin Foley
|
bb7978327e |
fix(logger): redact passwords and tokens from HTTP error log lines (#8013)
Resubmits #5820 by @echokos. The original PR's head fork could not accept maintainer edits (organization-owned fork without cross-org maintainer-edit access), so we've resubmitted the commits here with original authorship preserved. Thanks to @echokos for the contribution. --- ## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies; the server is an Express app with structured HTTP logging via pino-http. > - The middleware in `server/src/middleware/logger.ts` defines a `customProps` hook that attaches request context (`req.body` / `req.params` / `req.query`) to every 4xx/5xx log entry so operators can diagnose failed requests. > - That hook copies the body verbatim. Better Auth's `POST /api/auth/sign-in/email` carries an `{ email, password }` body — on a wrong-password attempt the request lands in the 4xx branch and the plaintext password is written to `~/.paperclip/logs/server.log`. > - Two existing issues raise this (#3072 plaintext-password leak, #4759 similar concerns) and neither has a fix. > - Same exposure surface applies to sign-up, reset-password, API key creation, and any endpoint that accepts a credential in the body and can return 4xx. > - This pull request introduces a small `redactSensitive` walker that returns a shallow copy of the input with values for known credential-shaped keys replaced with `[REDACTED]`, and applies it at every body/params/query log site in `customProps`. > - The benefit is that operators can keep diagnostic logging on without their disk silently accumulating user passwords and bearer tokens. ## What Changed - `server/src/middleware/redact-sensitive.ts` (new): depth-capped, case-insensitive walker. Sensitive keys covered: `password`, `currentPassword`, `newPassword`, `passwordConfirmation`, `passwordConfirm`, `confirmPassword` (+ snake_case variants), `secret`, `client_secret`, `access_token`, `refresh_token`, `id_token`, `auth_token`, `session_token`, `api_key`, `authorization`, `private_key`. Bare `token` deliberately not in the list — pagination cursors and CSRF tokens are not credentials (per Greptile review). - `server/src/middleware/logger.ts`: wraps the six log sites in `customProps` (3 ctx-path + 3 fallback-path) with `redactSensitive`. - `server/src/__tests__/redact-sensitive.test.ts` (new): covers plaintext password, case-insensitive matching, multiple credential keys, nested objects/arrays, bare `token` left untouched, primitives untouched, cycle safety. - Depth-cap returns `undefined` (field absent from log line) rather than a sentinel string, per Greptile review. ## Verification - `pnpm --filter @paperclipai/server test redact-sensitive` should run the new test file green. - Manual: tail `~/.paperclip/logs/server.log`, hit `POST /api/auth/sign-in/email` with a deliberately wrong password, confirm the logged `reqBody.password` reads `[REDACTED]` (not the plaintext) and the surrounding fields still appear for diagnosis. ## Risks Low. The walker only rewrites values at the log-emit boundary — the actual `req.body` object handed to downstream handlers is unchanged because `redactSensitive` returns a new object. Standard log fields (email, route path, status code) remain visible. The sensitive-key list is conservative enough that the only risk is over-redacting a non-credential field that happens to share a name with a known credential; the bare `token` carve-out in this revision addresses the most obvious such case. ## Model Used - Claude Opus 4.7 (`claude-opus-4-7`), Anthropic, extended thinking mode, working through Claude Code CLI. ## 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 - [ ] I have run tests locally and they pass (no `node_modules` in my disposable PR-prep checkout; CI vitest will exercise) - [x] I have added or updated tests where applicable - [ ] If this change affects the UI, I have included before/after screenshots (N/A — server-only) - [ ] I have updated relevant documentation to reflect my changes (no doc surface affected) - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge Closes #3072. Refs #4759. --- ## Original Context `customProps` in the HTTP logger copies `req.body` / `req.params` / `req.query` verbatim into 4xx/5xx log entries. Better Auth's wrong-password flow therefore writes: ``` {"reqBody":{"email":"…","password":"founding6gomez6croaking"},"msg":"POST /api/auth/sign-in/email 401"} ``` …to disk. This PR rewrites credential-shaped values to `[REDACTED]` at that boundary. --- ## Cross-references and status (maintainer) Closes #5820 Closes #3095 Closes #4760 Closes #4886 --------- Co-authored-by: Aurora <aurora@majorimpact.com> Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
AyeletMorris-ShieldFC
|
d2ef767712 |
fix(heartbeat): clear orphan execution locks on every issue when a run finalizes (#4318)
> **Note (rebase, 2026-06-11):** this PR was rebased onto current `master` again after #6008 (`Clear stale checkoutRunId on run finalization and add backstop sweeper`) landed. See "What Changed" below for how the previous narrow per-issue checkoutRunId clear from #6008 is now subsumed by a single bulk-update pass over every sibling that still references the finalizing run, with the two columns cleared in separate, scoped UPDATEs so retry pointers are not clobbered. ## Thinking Path > - Paperclip orchestrates AI-agent companies; issue execution ownership is gated by `executionRunId` / `executionAgentNameKey` / `executionLockedAt`, and any checkout whose run doesn't match the stored `executionRunId` is rejected with 409 "Issue run ownership conflict" > - In production, a running company silently got stuck: multiple in-progress issues ended up with `executionRunId` pointing at heartbeat runs that had already finalized hours earlier, so every new agent checkout returned 409 and the issues stayed marked blocked forever > - Root cause: `releaseIssueExecutionAndPromote` only resolved and cleared the execution lock on one issue per finalizing run (the run's `contextSnapshot.issueId`, or `rows[0]` when no context issue existed), but `enqueueWakeup`'s "legacy run" fallback can stamp the same `run.id` onto sibling issues' `execution_run_id`, so the siblings were left as orphans > - #4258 shipped a *reactive* fix for this bug class in `issueService`: `clearExecutionRunIfTerminal` now self-heals a stale execution lock on the next ownership-gated access (`checkout`, `assertCheckoutOwner`, `release`) to each affected issue, and `release` now unconditionally clears the three execution-lock fields > - #6008 shipped the *symmetric* fix for the `checkoutRunId` column (per-issue self-heal in `releaseIssueExecutionAndPromote`, `clearCheckoutRunIfTerminal` helper, and a backstop sweeper) > - This PR adds the *proactive* half at the point of run finalization, and generalizes #6008's per-issue checkoutRunId clear to every sibling that still references the finalizing run. After this PR + #4258 + #6008, orphan locks are cleared at the moment the run ends (across both execution and checkout columns, on every affected sibling), not only on the next access attempt to one of them ## Linked Issues or Issue Description - Closes #4194 - Closes #201 - Closes #3904 ## What Changed - **`server/src/services/heartbeat.ts` — `releaseIssueExecutionAndPromote`:** lock the context issue (when set) **and** every issue still referencing the finalizing run via either `execution_run_id` or `checkout_run_id`, under a single `SELECT ... FOR UPDATE ORDER BY id` (deterministic lock acquisition across concurrent finalizations). Then issue two scoped bulk `UPDATE`s in the same transaction: - one clears `executionRunId` / `executionAgentNameKey` / `executionLockedAt` on every issue whose `executionRunId` still matches this run, - the other clears `checkoutRunId` on every issue whose `checkoutRunId` still matches this run. The split avoids clobbering a retry's `executionRunId` pointer: in the codex-transient-upstream and process-loss retry paths, `executionRunId` is moved from this run to the retry run before `releaseIssueExecutionAndPromote` runs, while `checkoutRunId` is left pinned at the failed run. A single combined `UPDATE` with an `OR` predicate would null the retry's `executionRunId` in that case — these two scoped UPDATEs do not. The deferred-wake promotion contract is preserved: pick the run's context issue when present, else the first candidate (matching the legacy `rows[0]` selection under the new ordering). Recovery-agent fields added by a concurrent master change (`taskKey`, `recoveryAgent`, `recoverySessionBefore`, `recoveryAgentNameKey`, and the extra `assigneeAgentId`/`assigneeUserId` columns used downstream for `issueNeedsImmediateRecovery`) are fully preserved through the merge. The workspace-validation-failed recovery-comment path added by master is also preserved on the primary issue. - **`server/src/__tests__/execution-lock-orphan-cleanup.test.ts` (new, 6 tests):** multi-issue cleanup on finalize (2 issues); higher fan-out (4 issues) exercising the bulk `UPDATE` path; finalization of a run without a `contextSnapshot.issueId`; cross-company isolation under a pathologically cross-tenant `executionRunId`; unrelated-run locks are never touched by a sibling run's finalization; and a dedicated test for the `checkoutRunId` bulk-clear path that proves the split-UPDATE invariant by seeding a sibling whose `executionRunId` already points at a retry run while `checkoutRunId` is still pinned at the finalizing run — the test asserts the retry pointer is preserved and the checkout column is cleared. **Not in this PR:** `server/src/services/issues.ts` is intentionally unchanged. The release-side changes from the previous revision of this PR are fully subsumed by #4258 (`clearExecutionRunIfTerminal` plus unconditional clear in `release`) and #6008 (`clearCheckoutRunIfTerminal`). The per-issue checkoutRunId clear added in `releaseIssueExecutionAndPromote` by #6008 is replaced by the bulk path here, which strictly widens coverage from "primary issue only" to "every sibling that still references this run". ## Verification - `pnpm install --frozen-lockfile` — clean - `pnpm typecheck` (server workspace) — passes on the rebased branch - Focused suite (8 files, 147 tests — `execution-lock-orphan-cleanup` (6), `heartbeat-run-log`, `heartbeat-run-summary`, `issues-checkout-wakeup`, `issue-execution-policy-routes`, `issue-agent-mutation-ownership-routes`, `issues-service`, and `issue-stale-execution-lock-routes`): **147/147 pass**. - `heartbeat-process-recovery.test.ts` (52 tests): 51 pass; the one failure (`queues exactly one retry when the recorded local pid is dead`) reproduces verbatim on raw `master` with the PR's changes reverted, so it is a pre-existing flake (also noted by the earlier CI-retrigger commit on this branch). - Regression evidence: reverting `server/src/services/heartbeat.ts` to `master` while keeping the 6 new tests causes 5 of them to fail (the finalization-cleanup tests, including the new checkoutRunId-pointer-preservation test; the "unrelated-run locks never touched" test passes either way — that's its purpose as a negative control); restoring the fix returns to 6/6 green. - `pnpm-lock.yaml` untouched; no migration required; no public API shape change. Repro-ability of the original production symptom: ``` # Seed an issue with executionRunId pointing at a finalized run # (matches what enqueueWakeup's legacy-run fallback can produce) UPDATE issues SET execution_run_id = '<finalized-run-id>', execution_agent_name_key = 'ceo', execution_locked_at = NOW() WHERE id = '<issue-id>'; # Any subsequent svc.checkout against this issue 409s until the # lock is cleared. Before #4258, the lock stayed forever. After # #4258, it self-heals on next ownership-gated access. After this # PR, it's cleared at the moment the run finalizes so an untouched # sibling issue doesn't rely on a later access to recover. ``` ## Risks - **Same bug class exists in two adjacent, untouched code paths in this file** — `enqueueProcessLossRetry` repoints `executionRunId` only for the context issue (not siblings stamped with the failed run id), and `enqueueMissingIssueCommentRetry` locks all matching issues under `FOR UPDATE` but only updates the first row returned. Filed separately as #4319 with exact line refs so this PR can stay narrow. - **No `activity_log` entry is emitted for secondary orphan issues** whose locks are cleared by the new bulk UPDATEs — only the single primary issue retains its existing promotion event stream. Callers that audit lock transitions purely via `activity_log` may see orphan issues flip to `execution_run_id = null` (or `checkout_run_id = null`) without a matching event. Easy to add a batched log line in a follow-up if audit completeness matters; the #6008 backstop sweeper already emits `issue.stale_lock_cleared` for the catch-up path so this is mainly an observability nicety for the proactive path. - **UI polling shift** — `ui/src/pages/IssueDetail.tsx` and `ui/src/lib/issueActiveRun.ts` key off `execution_run_id` for active-run polling; clearing orphan locks at finalize (rather than waiting for next access as in #4258's flow) means the "executing" UI state falls back slightly faster when the underlying run has genuinely finalized. Observable, but an improvement over showing stuck state. - **Lockfile and manifests untouched**; no migration required; no public API shape change. ## Model Used - **Provider/model:** Anthropic Claude Opus 4.7 (the model this session is running on, per Cursor IDE system context) - **Harness:** Cursor IDE - **Capabilities used:** extended/reasoning thinking mode; filesystem and shell tool use; parallel subagent orchestration for a three-lens readonly self-review (correctness+concurrency, regression blast radius, test adequacy+style) before the initial push; targeted rebase conflict resolution after #4258 landed and again after #6008 landed, combining all three branches' changes to `releaseIssueExecutionAndPromote` - **Context:** full repository plus live access to the running Paperclip instance that exhibited the bug (the instance was unblocked via a targeted DB intervention before this code fix was authored; the live observation drove the root-cause analysis) ## 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 (the roadmap contains no references to heartbeat execution-lock management or `releaseIssueExecutionAndPromote`) - [x] I have searched GitHub for duplicate or related PRs and linked them above (#4258 and #6008 are the closest prior art and are explicitly cross-linked in the thinking path and "What Changed" sections; no other open or merged PR touches `releaseIssueExecutionAndPromote`) - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template — see "Linked Issues or Issue Description" above - [x] I have run tests locally and they pass (typecheck workspace-wide; 147/147 in the focused suite including #4258's and #6008's new tests) - [x] I have added or updated tests where applicable (6 regression tests; 5 of them provably fail on `master` without the code change) - [ ] If this change affects the UI, I have included before/after screenshots — *n/a, this is a server-only change; any UI polling effect is documented under Risks* - [ ] I have updated relevant documentation to reflect my changes — *n/a, no user-facing or API docs reference `releaseIssueExecutionAndPromote`* - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green (the prior `verify` flake is unrelated to this PR's change path and reproduces on unrelated branches; documented above and in follow-up #4328) - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups (the only test-coverage gap Greptile flagged on the latest review — the `checkoutRunId` bulk-clear branch — is now covered by the new 6th test in this revision) - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Devin Foley <devin@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
scotttong
|
6f9801a46b |
feat(ui): NUX rework behind enableConferenceRoomChat experimental flag — capsule onboarding, conference-room chat, unified composer (#8000)
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - The first-run experience (onboarding wizard) and the chat surfaces
(conference-room/board chat, task threads, composers) are the product's
front door — they decide whether a new operator understands "hire
agents, give them work, review results" in the first five minutes
> - Today those surfaces feel ticket-y and form-like: the wizard is a
static multi-step form that ends in an anticlimactic "Launch" screen,
the task composer and board chat behave differently from each other, and
agent-feed issue quicklooks misbehave (multiple flyouts open at once,
cards jump on hover)
> - We wanted to iterate toward a conversational, team-centric NUX — but
without risking the workflows of everyone already running Paperclip
> - This PR reworks the NUX behind a new default-OFF
`enableConferenceRoomChat` experimental flag: a capsule-motif onboarding
wizard that builds your team as you answer, a conference-room chat
surface, one shared ChatComposer across surfaces, brand-accurate status
chips, and feed-quicklook fixes — with the pre-existing UI
fork-and-frozen as `*Classic` components that flag-OFF users keep
> - The benefit is a complete, testable modern NUX that anyone can opt
into from Settings → Experimental, with zero default behavior change and
a clean path to either graduate or drop the experiment
## Linked Issues or Issue Description
No pre-existing GitHub issue — feature description per
`feature_request.yml`:
- **Problem / motivation:** Paperclip's onboarding wizard and chat
surfaces grew up as separate ticket-centric forms. New users get a
form-filling experience rather than the feeling of standing up a team;
the board chat and task threads use different composers with different
affordances; the agent feed's issue quicklook can stack multiple
popovers and shifts cards on hover.
- **Proposed solution:** A coherent NUX experiment behind one
experimental flag (`enableConferenceRoomChat`, Settings → Experimental,
default OFF): capsule onboarding wizard with an evolving team capsule,
conference-room chat, unified `ChatComposer`, team-centric copy, brand
status chips, quicklook single-flight fix. Flag-OFF users get the exact
pre-experiment UI via frozen `*Classic` forks, verified by an on/off
parity test matrix.
- **Alternatives considered:** (a) incremental unflagged restyling —
rejected: the changes interlock across surfaces and would drip risk into
every release; (b) a separate app shell / route for the new NUX —
rejected: too much divergence, the flag + classic-fork pattern keeps the
diff reviewable and reversible.
- **Roadmap alignment:** `ROADMAP.md` lists **CEO Chat** ("a
lighter-weight way to talk to leadership agents... should still resolve
to real work objects"). This experiment is groundwork in that direction
(conference-room chat resolves to issues/tasks via the same composer
used in task threads) and does not change the core task-and-comments
model.
Related PRs found in the dedup search (same area, none duplicate this
work — they target the classic wizard, which this PR intentionally
leaves intact and mergeable):
- #5385 — Coach-driven onboarding: conversational entry +
agent-companies package import
- #5378 — Onboarding wizard: reusable adapter picker + probe card
- #6636 — ui(onboarding): friendly error surface + retry for the wizard
- #7005 — fix(onboarding): explicitly await first-task wake
- #2616 — fix: restore workspace directory config in onboarding wizard
## What Changed
- **Experimental flag plumbing** — `enableConferenceRoomChat` in shared
types/validators, server instance-settings service + API, Settings →
Experimental card with explicit enable/disable copy
- **Onboarding wizard** — classic wizard forked and frozen
(`OnboardingWizardClassic`); flag-ON variant is a 5-step capsule wizard
with a persistent evolving `AgentCapsule` (gradient/glow motif),
team-centric reframed copy, and a typing-dots intro (hardened with
fake-timer tests)
- **Conference-room chat** — flag-ON board-chat surface with agent
bubble name/icon headers and copy/vote/timestamp action rows
(`AgentBubbleActionRow`)
- **Unified composer** — shared `ChatComposer` adopted across surfaces;
translucent surface + scroll-mask removal; "Agent mode"/"Plan mode"
relabels; no-assignee confirmation `AlertDialog` (new
`ui/alert-dialog.tsx` primitive); `@task` reference picker +
linkification in mentions
- **Agent feed** — single-flight issue-quicklook store (one popover at a
time), flyouts open to the left, removed hover translate-y jitter
- **Status chips** — brand-accurate task status chips behind the flag
(light/dark, 1px borders per paperclip.ing/brand)
- **Tests** — flag on/off parity matrix across IssueDetail,
NewIssueDialog, Sidebar, wizard, gate components; component tests for
all new pieces
- **Merge with `master`** — one conflict in
`ui/src/components/IssueChatThread.tsx`, resolved by keeping master's
new `AssigneeChip`/`HandoffWakeRow`/`RunStatusBadge` components inside
the flag-gated metadata-row chrome (details in commit `21a5642a`);
post-merge fixes: vitest 4 mock typing in `MarkdownEditor.test.tsx`,
flag hook made safe for provider-less mounts (master's new isolated
component tests)
- **Branch hygiene** — internal design wireframes/mockups stripped
before the PR (they live in the Paperclip issue threads)
- No user-facing documentation changes required: the flag is
intentionally experimental and self-described in the Settings card; no
existing docs reference the affected surfaces
## Verification
- `pnpm run typecheck` — green across the workspace (ui, server, shared,
plugins)
- Full UI suite (`vitest run` in `ui/`, clean worktree at this HEAD):
**1593/1595 passing, 223/224 files** — the 2 remaining failures are in
`src/components/artifacts/ArtifactCard.test.tsx` and **fail identically
on pristine `origin/master`** (pre-existing upstream, unrelated to this
branch)
- Full server suite (`vitest run` in `server/`, same clean worktree):
results in PR checks; flag plumbing covered by instance-settings tests
- Targeted post-merge resolution check: `IssueChatThread`,
`IssueChatThreadSystemNotice`, `IssueDetail`, `Sidebar`,
`ConferenceRoomChatGate`, `OnboardingWizardVariant`, `NewIssueDialog`,
`InstanceExperimentalSettings`, `MarkdownEditor` — 172/172 passing
- Manual walkthrough: flag OFF (default) → onboarding wizard, task
thread, board chat, composer all render the classic UI; flag ON via
Settings → Experimental → capsule wizard, conference-room chat, unified
composer, status chips active
- Screenshots: see below
**Flag on/off screenshots** (committed on this branch under
`screenshots/PR-8000-*`):
| Surface | Flag OFF (classic, default) | Flag ON (experimental) |
| --- | --- | --- |
| Settings → Experimental | 
| 
|
| Task thread | 
| 
|
| Home / nav | 
| 
|
| Conference Room (flag-ON only surface) | — | 
|
Capsule onboarding wizard walkthrough screenshots (flag ON) are attached
to the Paperclip design/implementation threads; the wizard requires a
fresh instance so it is captured via the e2e harness
(`tests/e2e/nux-phase4-screenshots.spec.ts`).
## Risks
- **Large surface, but gated:** all new behavior sits behind
`enableConferenceRoomChat`, default OFF; flag-OFF rendering is locked by
frozen `*Classic` forks plus an on/off parity test suite
- **Classic forks are frozen at the fork point (`e3aada1d`):** master
features added to the live thread component after that point (assignee
handoff chips, run status badge, composer mention coach) render in the
flag-ON path; the flag-OFF task thread keeps the fork-point behavior
until the experiment graduates (forks deleted) or is dropped (forks
restored as canonical). Called out for reviewer attention.
- **Merge-conflict resolution in `IssueChatThread.tsx`** (commit
`21a5642a`) deserves reviewer eyes: master's new handoff/run-status
components were kept; the base toast-style no-assignee flow remains
replaced by the AlertDialog flow introduced on this branch
- Schema/server changes are additive (one optional boolean instance
setting); no migrations of existing data
## Model Used
- Claude (Anthropic) via Claude Code running in the Paperclip agent
harness (agent: ClaudeCoder)
- Branch implemented across multiple agent sessions on Claude Opus-class
models with extended thinking + tool use (file edits, shell, Playwright
screenshots); merge/PR session model ID as reported by the harness:
`claude-fable-5` (Claude Code CLI)
- All code was agent-authored and board-reviewed through Paperclip issue
threads (plans, wireframes, confirmations) before merging
## 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 (none
required — experimental flag, self-documenting Settings card; noted
above)
- [x] I have considered and documented any risks above
- [x] All Paperclip CI gates are green (run 3 on `8af3041a`: all 16
gates SUCCESS, incl. e2e and all 4 serialized-suite shards)
- [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
(re-review verdict: Confidence 5/5, “Safe to merge”; all 4 round-1
findings fixed + confirmed resolved; both summary notes addressed in
`8af3041a`)
- [x] I will address all Greptile and reviewer comments before
requesting merge
---------
Co-authored-by: Paperclip <noreply@paperclip.ing>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
Dotta
|
1413729a06 |
Build the Skills Store (#7990)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents increasingly depend on reusable skills, so the control plane needs a first-class way to browse, inspect, install, version, and attach those skills. > - The old skills surface was mostly operational plumbing; it did not give operators a store-like discovery flow, canonical detail URLs, rich source/version context, or creation paths. > - The backend also needed stronger contracts around company skill metadata, versions, install counts, runtime materialization, and adapter skill preferences. > - This pull request builds the Skills Store foundation across DB, shared contracts, server routes/services, UI, and Storybook. > - The benefit is a more inspectable, operator-friendly skill workflow that still preserves company-scoped control-plane boundaries and agent runtime behavior. ## Linked Issues or Issue Description No GitHub issue exists for this Paperclip work item. Paperclip task refs: PAP-10846 and PAP-10921. Feature request: Paperclip operators need a single Skills Store experience where company skills can be discovered, inspected, created, versioned, installed, and attached to agents without relying on scattered operational screens or implicit runtime state. Related PR search: - Searched GitHub for `Skills Store`, `company skills`, and `skill detail`. - Found several open skills-related PRs such as #7809 and #4409, but no duplicate PR for this end-to-end Skills Store branch. ## What Changed - Added the Skills Store backend foundation: company skill schema fields, migrations, shared types/validators, and expanded server skill routes/services. - Added skill discovery, category navigation, canonical skill detail routes, tabs, source attribution, version snapshots/diffs, install count backfill, and creation flows. - Updated agent skill preference handling so version selections survive runtime mention injection and runtime skill materialization honors pinned versions. - Preserved unversioned skill assignments as live/current selections instead of silently pinning them to the current version at assignment time. - Added focused regression coverage for company skill routes/services, route helpers, UI behavior, skill version diffs, and runtime skill version pins. - Added Storybook coverage for Skills Store discovery/detail states and updated the main layout navigation. - Addressed Greptile findings around version creation races, soft-deleted comments, fork metadata scoping, GitHub skill directory fallback, runtime snapshot materialization, shared runtime skill-selection helpers, and version-assignment semantics. ## Verification - `pnpm exec vitest run server/src/__tests__/heartbeat-project-env.test.ts server/src/__tests__/heartbeat-runtime-skills.test.ts` - `pnpm exec vitest run packages/shared/src/validators/company-skill.test.ts` - `pnpm exec vitest run server/src/__tests__/company-portability.test.ts server/src/__tests__/company-skills-service.test.ts` - `pnpm exec vitest run cli/src/__tests__/company-import-export-e2e.test.ts` - `pnpm exec vitest run server/src/__tests__/agent-skills-routes.test.ts` - `pnpm exec vitest run server/src/__tests__/openapi-routes.test.ts` - `pnpm exec vitest run server/src/__tests__/company-skills-service.test.ts server/src/__tests__/company-skills-routes.test.ts server/src/__tests__/heartbeat-runtime-skills.test.ts` - `pnpm exec vitest run server/src/__tests__/company-skills-service.test.ts` - `pnpm exec vitest run ui/src/components/IssueProperties.test.tsx -t "edits existing custom assignee model options from the properties pane"` - `pnpm --filter @paperclipai/server typecheck` - GitHub checks are green on `0823957a2`: Build, Canary Dry Run, General tests, Typecheck + Release Registry, serialized server suites, e2e, policy/review, Socket, Snyk, and aggregate `verify`. - Greptile Review succeeded on `0823957a2` with `40 files reviewed, 0 comments added`; GitHub unresolved review threads: 0. Not run in this heartbeat: - Browser screenshot capture for the UI changes. This PR intentionally omits screenshots per the Paperclip task direction not to add design screenshots/images. ## Risks - Broad feature branch touching DB, shared contracts, server, and UI; reviewers should still scan merge conflicts carefully if `master` moves again before landing. - Skill version/runtime behavior is sensitive: pinned skill versions must stay pinned while default selections should continue following the current version. - UI polish should get normal reviewer/browser attention before merge because this PR includes a large Skills Store surface and screenshots were intentionally omitted. > 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-based coding agent with tool use and local command 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 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 (intentionally omitted per PAP-10921 direction) - [ ] 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> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Jannes Stubbemann
|
4ad94d0bde |
feat(server): kubernetes execution integration for sandbox-provider plugins (stage 2/3) (#7938)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The execution subsystem runs those agents in environments (local, ssh, sandbox), and sandbox-provider plugins let an environment materialize per-run sandboxes > - Stage 1 (#5790) contributed a first-party Kubernetes sandbox-provider plugin, but the server core has no way to adopt it operationally: no per-run adapter selection, no way to force an instance onto sandboxed execution, no declarative adapter/model configuration, and the plugin must be installed by hand > - Without this, a multi-tenant or security-conscious deployment cannot guarantee that agent runs never execute on the host, and a single environment cannot serve agents with different harnesses > - This pull request adds the server + SDK integration: per-run adapterType on the lease protocol, an env-gated forced-Kubernetes execution policy with provisioning and a per-run allowlist guard, a declarative adapter registry and model list, in-cluster env passthrough for sandbox plugin workers, fail-safe auto-install of the bundled plugin, and the matching UI affordance > - The benefit is that sandbox-provider plugins become fully usable for Kubernetes execution: operators configure everything via environment variables and GitOps, while self-hosters who set none of the variables see exactly the behavior they have today ## Linked Issues or Issue Description Refs #5790 (stage 1 of 3: the Kubernetes sandbox-provider plugin package). No existing issue. Feature description: the server core lacks the integration seams to operate a sandbox-provider plugin as the mandatory execution path of an instance. This PR is stage 2 of 3 of the staged Kubernetes contribution; stage 3 will contribute the agent runtime images and their build pipeline. ## What Changed One line per piece: - `packages/plugins/sdk/protocol.ts`: optional `adapterType` on `PluginEnvironmentAcquireLeaseParams` so a provider can select the runtime image per run; existing providers simply ignore it - `server/services/environment-runtime.ts` + `environment-run-orchestrator.ts`: thread the agent's adapter type into both lease-acquiring drivers, including the heartbeat path (the two call sites have historically drifted, hence the pinned test) - `server/services/environments.ts`: `ensureKubernetesEnvironment` / `findKubernetesEnvironment`, an idempotent managed Kubernetes environment per company, identified by a metadata marker and refreshed (not recreated) on config change; `timeoutMs` rides on the config for slow cold-start leases - `server/services/execution-allowlist.ts`: pure (driver, provider, policy) -> allow/deny guard; `executionMode=kubernetes` only allows the kubernetes sandbox provider - `server/services/execution-policy-bootstrap.ts` + startup hook in `server/index.ts`: parse `PAPERCLIP_EXECUTION_MODE` / `PAPERCLIP_K8S_*`, persist `executionMode` into instance general settings, and provision the managed environment for every company; fails loud on misconfiguration - `server/services/heartbeat.ts`: when the policy forces Kubernetes, pin run selection to the managed environment (also overriding any persisted workspace environment id), refuse to fall back to local, and re-check the actually acquired environment against the allowlist as defense in depth - `server/services/adapter-registry-bootstrap.ts` + shared `AdapterRegistryEntry` type/validator: declarative `PAPERCLIP_ADAPTERS` registry (inline JSON or file) that reconciles adapter availability at startup and rides on the Kubernetes environment config - `server/services/adapter-models-env.ts` + `adapters/registry.ts`: `PAPERCLIP_ADAPTER_MODELS` lets an operator declare picker model lists the server cannot CLI-discover - `server/services/plugin-loader.ts`: pass `KUBERNETES_SERVICE_HOST/PORT(_HTTPS)` through to plugin workers that register environment drivers, so in-cluster API clients can be constructed; all other host env stays stripped - `server/app.ts`: fail-safe auto-install of the bundled kubernetes plugin at boot; no-ops when the bundle is absent and never blocks startup on error - `packages/shared` types/validators: `InstanceExecutionMode` on general settings (optional, strict schema) - `ui/lib/forced-kubernetes-environment.ts` + `AgentConfigForm`: when the policy is active, show a read-only Kubernetes environment instead of the environment picker and default new agents onto the managed environment - Tests for every new module plus the adapterType pin in `heartbeat-plugin-environment` and the managed-environment lifecycle in `environment-service` Everything is gated: with `PAPERCLIP_EXECUTION_MODE`, `PAPERCLIP_ADAPTERS`, and `PAPERCLIP_ADAPTER_MODELS` unset (and no bundled plugin present), every code path reduces to current behavior. The per-run `adapterType` is an optional SDK parameter that existing providers ignore. ## Verification - `cd server && npx tsc --noEmit`: clean (0 errors); `ui` typecheck also clean - Targeted suites all green (11 files, 90 tests): `npx vitest run server/src/__tests__/heartbeat-plugin-environment.test.ts server/src/__tests__/environment-service.test.ts server/src/__tests__/environment-runtime.test.ts server/src/__tests__/environment-run-orchestrator.test.ts server/src/__tests__/plugin-database.test.ts server/src/services/execution-policy-bootstrap.test.ts server/src/services/execution-allowlist.test.ts server/src/services/adapter-registry-bootstrap.test.ts server/src/services/adapter-registry-bootstrap.reconcile.test.ts server/src/services/adapter-models-env.test.ts packages/shared/src/validators/adapter-registry.test.ts` - `npx vitest run ui/src/components/AgentConfigForm.test.ts`: green (6 tests) - Full `npx vitest run server/src/__tests__`: 2323 passed, 1 skipped; the only failures (heartbeat-process-recovery pid-retry, workspace-runtime symbolic-ref/git tests) reproduce identically on pristine `master` in the same environment, so they are machine-environment issues unrelated to this change; `server-startup-feedback-export` needed its `services/index.js` mock extended with the new export and is green - This integration has been running in production on a hosted multi-tenant deployment, where it executes agent runs across five different harnesses through the stage 1 plugin ## Risks - Low for existing deployments: every behavior is env-gated and the defaults preserve current semantics; the auto-install block is wrapped fail-safe and skips silently when the plugin bundle is absent - `executionMode` is a new optional field on a strict zod schema; absent input normalizes exactly as before - The forced policy intentionally fails runs loudly (rather than falling back to local) when no managed Kubernetes environment exists; this only affects instances that explicitly set `PAPERCLIP_EXECUTION_MODE=kubernetes` ## Model Used Claude Opus 4.8 (claude-opus-4-8, 1M context), extended thinking, agentic tool use via Claude Code. ## UI screenshots The UI change is a new read-only "Execution" section in `AgentConfigForm`, shown only when the instance execution policy forces Kubernetes (`executionMode=kubernetes`); there is no "before" state for it (the section did not exist, and instances without the forced policy render the existing picker unchanged). Captured from the new Storybook stories added in this PR (`Product/Agent Management`): Managed Kubernetes environment present (read-only display, no local/SSH picker):  No managed environment available yet (warning notice, no silent local fallback):  ## 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 - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Devin Foley
|
c139d6c025 |
fix(codex-local): omit default model so codex CLI picks per auth mode (#7971)
## Thinking Path > - Paperclip orchestrates AI agents through pluggable local adapters; codex_local wraps OpenAI's `codex` CLI. > - The codex_local adapter declares a hard-coded `DEFAULT_CODEX_LOCAL_MODEL = "gpt-5.3-codex"` and multiple Paperclip consumers (UI build-config, server route, OnboardingWizard, NewAgent form, AgentConfigForm) fall back to it when the operator doesn't pick a model. > - That model — and every `*-codex` model plus the older `gpt-5/5.1/5.2` lines — is API-key-only. Codex CLI rejects them on ChatGPT subscription auth with "The 'gpt-5.3-codex' model is not supported when using Codex with a ChatGPT account." > - Every codex_local agent created through the default onboarding path inherits this pin and breaks on its first heartbeat for any user authed via `codex login` (ChatGPT). > - claude_local already takes the right shape: its build-config only sets `adapterConfig.model` when the operator actually picked one, and falls through to whatever default `claude` CLI uses. > - Codex CLI's own default is auth-mode-aware. ChatGPT-subscription accounts get `gpt-5.5`; API-key accounts get the codex-tuned default. A Paperclip-side pin masks this and downgrades whichever group it wasn't built for. > - This PR makes codex_local match claude_local's shape: omit `adapterConfig.model` when the user picks "default," and let the CLI choose. Subscription users stop breaking; API-key users stop getting downgraded. > - The benefit is auth-mode-correct defaults with no Paperclip-side hard pin, plus future-proofing: when OpenAI bumps the CLI default we inherit it for free. ## What Changed - `packages/adapters/codex-local/src/ui/build-config.ts` — only set `adapterConfig.model` when the operator picked one (parity with `packages/adapters/claude-local/src/ui/build-config.ts`). - `server/src/routes/agents.ts` — drop the codex_local-specific `next.model = DEFAULT_CODEX_LOCAL_MODEL` fallback in `applyCreateDefaultsByAdapterType`. Bypass-sandbox default is left in place (security posture, not a model choice). - `ui/src/pages/NewAgent.tsx`, `ui/src/components/AgentConfigForm.tsx`, `ui/src/components/OnboardingWizard.tsx` — stop pre-populating the model field with `DEFAULT_CODEX_LOCAL_MODEL` when the user selects the Codex adapter. Other adapters' defaults (gemini_local, cursor, opencode_local) are unchanged. - `DEFAULT_CODEX_LOCAL_MODEL` is preserved as an exported constant for downstream consumers / plugin authors who want to opt in to a pin; we just stop forcing it on operators who didn't ask for one. - Test: assert `buildCodexLocalConfig` omits `model` when input is blank. ## Verification - `pnpm exec vitest run packages/adapters/codex-local/src/ui/build-config.test.ts packages/adapters/codex-local/src/server/codex-args.test.ts server/src/__tests__/adapter-registry.test.ts server/src/__tests__/heartbeat-model-profile.test.ts server/src/__tests__/agent-permissions-routes.test.ts` → 74/74 passing - `pnpm exec vitest run ui/src/lib/duplicate-agent-payload.test.ts ui/src/lib/acpx-model-filter.test.ts` → passing - `pnpm tsc --noEmit -p .` → clean - Live: I separately verified live during initial investigation that on ChatGPT-subscription auth, `gpt-5.3-codex` is rejected and `gpt-5.5` is what Codex CLI picks by default. Omitting model lets the CLI handle that. ## Risks - Telemetry: any sink that reads `adapterConfig.model` for cost attribution will now see the empty/omitted case more often. The CLI emits the actually-used model in its event stream; downstream telemetry should already read from there for accuracy, but worth a check. - Operator UX: "default" now means "whatever the CLI picks" instead of a Paperclip-known model. The selectable catalog still includes `gpt-5.5`, `gpt-5.4`, `gpt-5.3-codex`, etc. for operators who want to pin explicitly. - Existing agents are unaffected — their `adapterConfig.model` is already set; this only changes the *new-agent* default flow. ## Related work - Depends on: an open catalog-add PR adding `gpt-5.5` to the selectable model list and to `CODEX_LOCAL_FAST_MODE_SUPPORTED_MODELS`. Operators who want to switch to `gpt-5.5` explicitly need that PR merged first; this PR is the structural change that makes "default" mean "let the CLI choose." - Closes #5371 — codex_local default model selection persists `gpt-5.3-codex` instead of adapter default (this PR is the exact fix #5371 proposes). - Related: #5132 (opencode-local: hire-time default model fails on ChatGPT-OAuth accounts) — same problem shape on a sibling adapter; not fixed here but worth tracking for a parallel. - Related: #5939 (codex_local adapter hardcodes `gpt-5.3-codex-spark` validation, fails on ChatGPT OAuth accounts regardless of configured model) — separate validation-path bug; not fixed here. ## Model Used Claude (Sonnet-class), running inside Paperclip as a claude_local executor. ## 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 - [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> |
||
|
LIXin Ye
|
9a48d92104 |
Add GPT-5.5 to Codex local model options (#5575)
## Related work This PR is the cleanest "add `gpt-5.5` to the codex-local catalog" change open against master. Several other PRs propose the same catalog/fast-mode update; they should close as duplicates once this lands: - #4646 — Add Codex gpt-5.5 model option - #6044 — feat(codex-local): add gpt-5.5 to model catalog, default reasoning to medium, cheap profile xhigh - #6045 — feat(codex-local): add gpt-5.5 to model catalog, default medium reasoning, xhigh cheap profile - #6595 — feat(adapters): add new Codex models (gpt-5.5, gpt-5.4-mini, gpt-5.3-codex, gpt-5.2) Related issues this enables (catalog-level surface area): - #5371 — codex_local default model selection persists `gpt-5.3-codex` instead of adapter default. This PR makes `gpt-5.5` selectable in the dropdown; a separate follow-up changes the *default* behavior so users who don't pick a model are subscription-compatible. - #5132 — opencode-local: hire-time default model fails on ChatGPT-OAuth accounts. Sibling adapter, same problem shape; not fixed here but worth tracking as a parallel for the opencode side. --- ## Thinking Path > - Paperclip orchestrates AI agents through adapter-backed local and remote runtimes. > - The `codex_local` adapter declares built-in model options that feed the server model list and, in turn, the agent configuration UI dropdown. > - GPT-5.5 is available in newer Codex environments but was missing from Paperclip's fallback `codex_local` model list. > - Operators could still type a manual model ID, but the default dropdown made the supported path look unavailable. > - Codex fast mode support is declared separately, so adding GPT-5.5 to the visible list should also include it in the supported fast-mode set. > - This pull request adds GPT-5.5 to the built-in Codex local model options and updates focused tests around argument generation and adapter model listing. > - The benefit is a clearer default setup path for agents using GPT-5.5 without changing existing defaults or migrations. ## What Changed - Added `gpt-5.5` to the `codex_local` fallback model list. - Added `gpt-5.5` to `CODEX_LOCAL_FAST_MODE_SUPPORTED_MODELS`. - Updated Codex argument tests to cover GPT-5.5 fast mode and preserve manual-model fast mode behavior. - Updated adapter model listing tests to assert the Codex fallback list includes GPT-5.5. ## Verification - `pnpm exec vitest run packages/adapters/codex-local/src/server/codex-args.test.ts server/src/__tests__/adapter-models.test.ts` - `git diff --check` - UI note: this is a dropdown data-source change rather than a layout/component change; the adapter model listing test covers the list consumed by the UI. ## Risks - Low risk. This only extends a static fallback model list and fast-mode allowlist. - Existing defaults remain unchanged (`gpt-5.3-codex`). - If a local Codex CLI does not support `gpt-5.5`, selecting it will still fail at execution time the same way any unavailable manual model would. > 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 desktop coding agent, GPT-5-family model. The exact backing model ID was not exposed by the local runtime; the session used shell, Git, test execution, and GitHub CLI tool access. ## 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 - [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: apple <apple@appledeMacBook-Pro.local> Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Devin Foley <devin@paperclip.ing> |
||
|
Dotta
|
11a64819f9 |
Keep agent-created follow-ups in run workspace
Reviewed and merged for PAP-10871/PAP-10873.\n\nVerification:\n- pnpm vitest run server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts\n- git diff --check origin/master...HEAD\n- GitHub PR checks green before merge |
||
|
Sherif Kozman
|
b8fb81dee9 |
fix(gemini-local): treat token-overflow as a fresh-session signal (#4932)
## Thinking Path The same 2026-04-30 audit that produced PR #4118 (`Invalid session` regex extension) and the ENOTFOUND classifier (#4931) identified a third stuck-session pattern: **13 failures in 7 days, all on a single agent (Ernest)**, with stderr matching: ``` _ApiError: {"error":{"code":400,"message":"The input token count exceeds the maximum number of tokens allowed 1048576","status":"INVALID_ARGUMENT"}} at ChatCompressionService.compress ``` The root cause is that gemini-cli's `ChatCompressionService` blew the 1M token context limit **during its compression step itself**. Resuming the same session ID will hit the same wall on the next attempt — the session is effectively dead the same way it is when "Invalid session identifier" fires (PR #4118). ## What Changed Extends the `isGeminiUnknownSessionError` regex in `parse.ts` with two phrases: - `exceeds\s+the\s+maximum\s+number\s+of\s+tokens` - `input\s+token\s+count\s+exceeds` Both trigger the **existing** fresh-session retry path in `execute.ts:596` — no new code path. Same extension pattern as PR #4118. ## Verification - `npx vitest run --project @paperclipai/adapter-gemini-local` → 14/14 pass (11 in `parse.test.ts` + 3 existing in `execute.remote.test.ts`) - 2 new tests cover the token-overflow patterns - `pnpm --filter @paperclipai/adapter-gemini-local typecheck` → clean - Audit query against `heartbeat_runs.stderr_excerpt` confirms regex matches all 13 occurrences ## Stacking This PR is stacked on top of #4931 (the ENOTFOUND classifier) which adds the `parse.test.ts` file. If #4931 merges first, this PR's diff is just the regex + 2 tests. If this PR is reviewed first, please merge #4931 first to avoid touching the same test scaffolding twice. ## Risks - **Low.** Single-line regex extension. No new code paths. - The session-reset path is well-trodden (PR #4118 in flight). - If a non-Gemini caller produces a stderr containing "exceeds the maximum number of tokens" by coincidence, they would trigger one unnecessary fresh-session retry. Not plausible in the gemini-cli output context where this stderr is sourced. ## Model Used Claude Opus 4.7 (1M context), Anthropic SDK via Claude Code CLI. ## Checklist - [x] Thinking path traces from audit data to single-line regex change - [x] Model specified - [x] No duplicate of planned core work - [x] Tests pass locally - [x] Tests added (2 new) - [x] N/A — server-side regex - [x] Internal pattern; no docs change - [x] Risks documented - [x] Will address Greptile + reviewer comments before merge - [x] I searched the GitHub PR list for similar PRs and confirmed this is not a duplicate (related: #4118 covers the "Invalid session identifier" regex; this PR extends the same regex with token-overflow phrases) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Devin Foley <devin@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
Nir Arazi
|
b853ce5183 |
Fix heartbeat task-session reuse when agent model changes (#4195)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies. > - Heartbeats wake agents and resume prior adapter task sessions so work is continuous. > - A persisted task session can contain adapter-specific state (for Codex, a resumable thread/session) created under the agent's then-current model. > - When an operator changes an agent's configured model, the next run should not blindly reuse a session created under a different model — context window, capabilities, and prompt assumptions may differ. > - The existing wake reset logic handles wake reasons (forceFreshSession, comment wakes, etc.) but not model drift between current agent config and persisted task-session metadata. > - This pull request adds model-aware task-session reset and persists the configured model into task-session metadata. > - The benefit is that heartbeat runs reliably honor the current agent model configuration and avoid stale session/model mismatches. ## Linked Issues or Issue Description **What happened?** After an operator changes an agent's configured model (for example, swapping a Codex agent from one model variant to another), the heartbeat reuses the persisted adapter task session that was created under the previous model. The new model never takes effect on resume — the run continues on the prior session and prior model assumptions. **Expected behavior** A model change in agent configuration should invalidate the persisted task session for that agent and force a fresh session start on the next run, so the configured model is the one actually used. **Steps to reproduce** 1. Run an agent with model `A` so it persists an adapter task session under model `A`. 2. Change the agent's configured model to `B`. 3. Trigger a heartbeat for the same issue/agent. 4. Observe: the run resumes the prior task session (still under model `A`) instead of starting fresh under model `B`. ## What Changed - Added task-session model metadata support in heartbeat session handling via `__paperclipConfiguredModel`. - Persisted the current configured adapter model into `agent_task_sessions.sessionParamsJson` whenever heartbeat upserts task-session state. - Added `shouldResetTaskSessionForModelChange(...)` to explicitly detect model drift between current config and persisted session metadata. - Updated run startup logic to force a fresh session when model drift is detected, with a clear reason message in runtime warnings. - Strips the internal `__paperclipConfiguredModel` key from `sessionParamsJson` before it is forwarded to adapters so the metadata stays internal. - Added focused tests in `server/src/__tests__/heartbeat-workspace-session.test.ts` covering model-drift reset behavior, non-reset cases, and the strip helper. ## Verification - `pnpm --filter @paperclipai/server test src/__tests__/heartbeat-workspace-session.test.ts` - `pnpm --filter @paperclipai/server typecheck` ## Risks Low. Sessions without persisted model metadata are not reset (backward compatible). The model key is namespaced (`__paperclip...`) to avoid colliding with adapter-forwarded params. Drift detection only fires when both current config and persisted metadata are present and differ. ## Model Used Claude (Opus 4.6) — used to design the metadata persistence, add the drift detection helper, and write unit coverage. ## 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 — no UI changes) - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green (in progress) - [ ] 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> Co-authored-by: Devin Foley <devin@paperclip.ing> |
||
|
Nicolás Rodrigues
|
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> |
||
|
NyDamon
|
0713dfa41f |
fix: validate session ID as UUID before --resume + error diagnostics (DLD-889) (#1742)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies. > - The Claude-local adapter uses `claude --resume <session-id>` to continue prior sessions; the `--resume` value MUST be a UUID per Claude's CLI contract. > - Paperclip internally uses session IDs prefixed with `ses_` (not UUIDs); these get passed straight through to `--resume` and crash the run. > - On top of the crash, when the underlying error path triggers a secret-decryption failure or heartbeat setup failure, the diagnostics are too thin to tell key-mismatch from other failures, and the heartbeat error code is mis-classified as `adapter_failed` instead of `setup_failed`. > - This PR validates `runtimeSessionId` against a UUID regex before letting `canResumeSession` become true, adds `not a valid UUID` to Claude's own retry-error regex, improves AES-256-GCM decryption diagnostics in the local encrypted provider, and re-classifies pre-adapter setup failures. > - The benefit is that Paperclip session IDs are detected and skipped gracefully (logged, no crash), legitimate Claude UUID-rejection errors are treated as retriable, and operators can diagnose decryption/setup failures from the run log. ## Linked Issues or Issue Description **What happened?** The `claude-local` adapter passes Paperclip's internal session identifiers (e.g. `ses_…`) straight to `claude --resume <session-id>`. Because Claude's CLI requires the `--resume` argument to be a UUID, the run crashes with a `not a valid UUID` error. When the surrounding code path also hits a secret-decryption failure, the heartbeat reports it as `adapter_failed`, hiding the real `setup_failed` cause and making diagnosis hard. **Expected behavior** Non-UUID session IDs should be detected before `--resume` is called, the run should fall back to a fresh session with a clear log line, and any decryption / setup failure should be reported with enough detail (and the correct error code) for an operator to tell what failed. **Steps to reproduce** 1. Have a persisted task session whose ID is not a UUID (Paperclip-issued `ses_…` form). 2. Trigger a heartbeat that resumes that session via the `claude-local` adapter. 3. Observe: the adapter crashes with a UUID-validation error; if the path also involves a decryption failure, the heartbeat surfaces `adapter_failed` instead of `setup_failed`. ## What Changed - `packages/adapters/claude-local/src/server/execute.ts`: Validates `runtimeSessionId` against a UUID regex before setting `canResumeSession`; non-UUID IDs are logged and skipped gracefully. Guards the cwd-mismatch log block on `isValidUuid` so it does not fire for non-UUID session IDs. - `packages/adapters/claude-local/src/server/parse.ts`: Adds `not a valid UUID` to the session-error retry regex so Claude's own UUID rejection is treated as a retriable error. - `server/src/services/secrets/local-encrypted-provider.ts`: Wraps AES-256-GCM decryption in try/catch and re-throws with a key fingerprint hint to aid key-mismatch diagnosis. - `server/src/services/heartbeat.ts`: Corrects the outer-catch `errorCode` from `adapter_failed` to `setup_failed` for pre-adapter setup failures. - `AGENTS.md`: Adds task/PR/CI governance sections (10–13) and expands the Definition of Done. ## Verification - `pnpm --filter @paperclipai/adapter-claude-local test` covers UUID validation and the parse retry regex. - `pnpm --filter @paperclipai/server test src/services/secrets` covers decryption diagnostics. - `pnpm --filter @paperclipai/server typecheck` ## Risks Low. UUID validation is strictly additive (non-UUIDs that previously crashed now log and skip). Decryption diagnostics only fire on failure paths. The `setup_failed` error code change is a clearer classification, not a behavior change. ## Model Used Claude (Opus 4.6) — used to identify the UUID-validation root cause, mirror existing parse patterns, and re-classify the heartbeat setup error code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have 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 — no UI changes) - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green (in progress) - [ ] 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: CTO Agent <cto@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Devin Foley <devin@paperclip.ing> |
||
|
Dotta
|
67b22d872f |
[codex] Clarify interrupt handoffs and scoped wake semantics (#7855)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The issue thread is the operator surface where comments, assignee changes, pauses, resumes, and wakeups turn human intent into agent execution. > - Interrupting a live run and handing work to another assignee needs clear semantics so the product does not accidentally keep work alive, wake the wrong participant, or hide why an agent stopped. > - Comment-driven wakes also need strict boundaries so closed, blocked, and dependency-driven work only resumes when there is real actionable input. > - This pull request codifies the interrupt handoff contract, implements backend scheduling behavior, and gives the UI clearer handoff/pause language. > - The benefit is a more inspectable and predictable task lifecycle for both operators and agents. ## Linked Issues or Issue Description Paperclip issue: `PAP-10664` / `PAP-10751`. Problem: interrupting or reassigning live agent work could be ambiguous in the UI and backend. Operators needed clearer feedback about whether a handoff wakes an agent, what pause/cancel affects, and when comments should revive execution. The backend also needed stronger tests around comment wake boundaries, retry supersession, and structured agent mention dispatch. Related GitHub PR search found broad workflow-adjacent PRs #5082, #6359, and #4083, but no exact duplicate for this head branch or interrupt-handoff scope. ## What Changed - Added an interrupt handoff semantics document covering destination behavior, wake expectations, and live-run interruption states. - Implemented backend interrupt handoff behavior and comment wake/reopen handling in issue routes/services and heartbeat scheduling. - Hardened structured agent mention dispatch so mentions resolve through the intended dispatch path. - Added UI helpers and components for handoff chips, wake rows, interrupt banners, pause-affects summaries, and composer guidance. - Updated the issue properties assignee picker and issue chat/composer surfaces to make interrupt/reassign behavior clearer. - Added backend, UI utility, component, and Storybook coverage for the new behavior. - Stabilized the new UI component tests with a local `flushSync`-backed act helper matching existing repo practice in this dependency set. - Addressed Greptile feedback by threading historical run `errorCode` through issue-run data and operator-interrupted chat labels. - Addressed Greptile's cancel ordering concern by terminating/deleting in-memory heartbeat processes before cancellation status persistence, with regression coverage for DB update failure. ## Verification - `git diff --check $(git merge-base HEAD origin/master)..HEAD` - `pnpm --filter @paperclipai/ui exec vitest run src/lib/interrupt-handoff.test.ts src/lib/issue-chat-messages.test.ts src/components/IssueProperties.test.tsx src/components/interrupt-handoff/InterruptHandoffViews.test.tsx --no-file-parallelism --maxWorkers=1` — 4 files / 91 tests passed before the Greptile follow-ups. - `pnpm run preflight:workspace-links && pnpm exec vitest run server/src/__tests__/heartbeat-process-recovery.test.ts server/src/__tests__/heartbeat-retry-scheduling.test.ts server/src/__tests__/issue-comment-reopen-routes.test.ts server/src/__tests__/issue-tree-control-service.test.ts server/src/__tests__/issue-update-comment-wakeup-routes.test.ts server/src/__tests__/issues-service.test.ts --no-file-parallelism --maxWorkers=1` — 6 files / 191 tests passed before the Greptile follow-ups. - `pnpm --filter @paperclipai/ui exec vitest run src/lib/issue-chat-messages.test.ts --no-file-parallelism --maxWorkers=1` — 1 file / 24 tests passed after the historical `errorCode` follow-up. - `pnpm exec vitest run server/src/__tests__/activity-service.test.ts server/src/__tests__/activity-routes.test.ts --no-file-parallelism --maxWorkers=1` — 2 files / 11 tests passed after the historical `errorCode` follow-up. - `pnpm exec vitest run server/src/__tests__/heartbeat-process-recovery.test.ts --no-file-parallelism --maxWorkers=1` — 1 file / 52 tests passed after the cancel ordering follow-up. - Greptile is green for head `272647636287d034bab8d981eaf5305865aa0f96`; the old inline P2 is resolved/outdated. - GitHub Actions, Socket, security-review, and Greptile checks are green for head `272647636287d034bab8d981eaf5305865aa0f96`. The external `security/snyk (cryppadotta)` status was still pending at `https://app.snyk.io/org/cryppadotta/pr-checks/85b3e8f4-04e1-4f8e-9362-899c8148c23c` after a bounded wait. ## Risks - Medium: changes touch issue comments, wake scheduling, and live-run interruption semantics, so regressions could affect when agents resume or stay stopped. - Medium: UI copy and state grouping for assignee changes may need reviewer tuning after product review. - Low migration risk: no database schema migration is included. - The branch was created before the latest `origin/master` commits; reviewers should confirm CI merge-base behavior and resolve any merge conflicts if GitHub reports them. > 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-based coding agent, tool use and local command execution enabled. Exact hosted model build and context window were not exposed by the runtime. ## 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 Screenshot note: this PR includes Storybook coverage for the new interrupt handoff UI states rather than captured before/after browser screenshots in this PR-creation heartbeat. --------- Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
xidui
|
5d315ab778 |
Defer same-issue forceFreshSession wakes into follow-up runs (#4080)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The heartbeat service governs how agent wake events get queued, deferred, or folded into the currently-running adapter run > - `forceFreshSession: true` wakes on a same-agent/same-issue path get silently folded into the active run, so callers can never request a true cold-start follow-up > - This breaks phased workflows that need to drop a poisoned session and restart cleanly on the same issue without bouncing to another agent > - This PR extracts the existing same-issue follow-up decision into `shouldDeferFollowupWakeForSameIssue` and extends it to also defer `forceFreshSession: true` wakes into a follow-up run boundary > - The benefit is that `forceFreshSession` now behaves as documented: it actually starts a fresh session, even when the wake targets the same agent/issue/runtime that is currently executing ## Linked Issues or Issue Description **What happened?** A wake event posted with `forceFreshSession: true` against an issue whose current adapter run is still `running` on the same execution agent is silently coalesced into that in-flight run instead of starting a cold session. Callers that explicitly request a fresh-session reset see no behavior change until the run naturally completes. **Expected behavior** `forceFreshSession: true` should always force a fresh session start, even when the wake targets the same agent/issue that is currently executing. The wake should defer into a follow-up run boundary if the current run is still in-flight. **Steps to reproduce** 1. Start an adapter run for some issue. 2. While the run is still `running`, post a wake event for the same issue/agent with `forceFreshSession: true`. 3. Observe: the active run continues without resetting the session; the fresh-session signal is dropped. ## What Changed - Extracted same-issue follow-up decision into exported helper `shouldDeferFollowupWakeForSameIssue` in `server/src/services/heartbeat.ts` - Extended that helper so `forceFreshSession: true` (not only `wakeCommentId`) defers into a follow-up run when the current run is still `running` for the same execution agent - Added stickiness to `mergeCoalescedContextSnapshot`: if either side of a wake-merge has `forceFreshSession: true`, the merged snapshot keeps it set so it is not silently dropped while queued wakes coalesce - Added five unit tests in `heartbeat-workspace-session.test.ts` covering each decision branch of the helper ## Verification - `pnpm --filter @paperclipai/server test src/__tests__/heartbeat-workspace-session.test.ts` - `pnpm --filter @paperclipai/server typecheck` ## Risks Low. Behavior change only affects the narrow case where a same-agent/same-issue wake carries `forceFreshSession: true` while the active run is still `running`. Other wake paths (cross-agent, queued/failed runs) are untouched. The helper extraction is a pure refactor preserving the prior comment-wake deferral. ## Model Used Claude (Opus 4.7) — extended thinking enabled, used to extract the helper, extend the deferral condition to cover `forceFreshSession`, and write unit coverage. ## 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 — no UI changes) - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green (in progress) - [ ] 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: Devin Foley <devin@paperclip.ing> |
||
|
Danial Jawaid
|
8ee3987d12 |
adapter-claude-local: recover from poisoned previous_message_id 400 (detect + clearSession) (#5972)
## Thinking Path
> - Paperclip's `claude_local` adapter persists Claude Code session
jsonls under `~/.claude/projects/…/{sessionId}.jsonl` and resumes them
on the next heartbeat
> - When Claude Code injects `<synthetic>` placeholder assistant
messages (after rate-limit, max-turn exhaustion, or transient-upstream
failures) those placeholders get UUID-format `message.id`s rather than
`msg_…`-format ids
> - On the next `--resume`, Claude Code passes that UUID as
`previous_message_id` and Anthropic's API rejects it with a 400:
``diagnostics.previous_message_id: must be the `id` from a prior
/v1/messages response (starts with `msg_`)``
> - The adapter had a session-rotation fallback only for "unknown
session" errors, so the poisoned session was `--resume`-d indefinitely
and the agent flipped between `idle` and `error` every heartbeat
> - Even worse, the *result* event of the failing run still carried a
`session_id`, and the adapter was persisting that id into the
issue-scoped session store (`agentTaskSessions`). So even after we
detected the 400, every subsequent continuation re-loaded the same
poisoned id and hit the same 400 again — the issue was permanently
stranded
> - We observed this on multiple agents in our deployment; the only
manual fix was to rename the `.jsonl`, which is not a viable long-term
workaround
> - This PR detects the 400, runs the same session-rotation fallback the
unknown-session path uses **and** stops persisting the poisoned id, so
the next attempt starts genuinely fresh
## Linked Issues or Issue Description
No external GitHub issue is linked. Describing the problem inline
following the bug-report template:
**What happened:** `claude_local` agents flipped between `idle` and
`error` on every heartbeat because the persisted session jsonl carried a
synthetic UUID `previous_message_id` (from `<synthetic>` assistant
placeholders injected after rate-limit/max-turn/upstream errors).
Anthropic's API rejected every `--resume` with a 400:
``diagnostics.previous_message_id: must be the `id` from a prior
/v1/messages response (starts with `msg_`)``.
**Expected behavior:** When the persisted session is poisoned and
unrecoverable, the adapter should rotate to a fresh session — the same
fallback path already used for unknown-session errors — and stop
re-persisting the poisoned `session_id`.
**Actual behavior:** The session-rotation fallback only matched the
"unknown session" pattern, so the poisoned session was `--resume`-d
forever. The result event of the failing run still carried `session_id`,
which was being persisted into `agentTaskSessions`, so every subsequent
continuation reloaded the same poisoned id and hit the same 400.
**Reproduction:** Inject any flow that causes Claude Code to emit a
`<synthetic>` placeholder (rate-limit, max-turn exhaustion, transient
upstream failure). The next `--resume` will fail with the 400 and the
agent will not self-recover.
**Scope of fix:** Add a `previous_message_id` 400 detector; route it
through the existing unknown-session fallback; drop the poisoned
`sessionId` and emit `clearSession: true` so the heartbeat service wipes
the persisted row; best-effort delete the local poisoned `.jsonl`.
## What Changed
Two commits:
1. **`adapter-claude-local: auto-rotate session on previous_message_id
400 (synthetic-msg poisoning)`** — detector + execute-time rotation
2. **`adapter-claude-local: guard against persisting poisoned
sessionId`** — validate-before-persist + `clearSession`
Combined diff:
- `parse.ts`: new `isClaudePoisonedPreviousMessageIdError(parsed)`
matching ``/diagnostics\.previous_message_id.*starts with `msg_`/i``
against `parsed.result` and `extractClaudeErrorMessages(parsed)`
- `parse.ts`: `isClaudeTransientUpstreamError()` excludes the new error
from transient classification so it isn't masked as retryable upstream
noise
- `execute.ts`: expand the resume-fallback branch so it triggers on both
`isClaudeUnknownSessionError` and the new
`isClaudePoisonedPreviousMessageIdError`, with a distinct log line
(`"returned a poisoned message-id"` vs `"is unavailable"`)
- `execute.ts`: for local (non-remote) execution targets, best-effort
delete the poisoned `~/.claude/projects/.../{sessionId}.jsonl` before
retrying so the file can't be accidentally resumed by an out-of-band
caller. The `fs.unlink` and follow-up log call are in separate try/catch
blocks so a closed log stream cannot mask a successful unlink (and vice
versa)
- `execute.ts` / `toAdapterResult`: when a result carries the poisoned
400, **drop** `sessionId`/`sessionParams`/`sessionDisplayId` (return
`null`) and emit `clearSession: true` so the heartbeat service's
`resolveNextSessionState` wipes the persisted row. The result also
surfaces `errorCode: "claude_poisoned_previous_message_id"` for
observability
- `docs/adapters/claude-local.md`: runbook entry — symptom,
auto-recovery flow, on-call checklist
- Tests:
- 4 new `parse.test.ts` cases covering positive detection in `result`
and `errors[]`, negative cases, and non-transient classification
- 3 new `claude-local-execute.test.ts` cases: (a) fresh run reports the
poisoned error → sessionId dropped + `clearSession: true`; (b) recovery
retry also reports the poisoned error → same guards apply; (c)
session-rotation success on retry
## Verification
```bash
pnpm --filter @paperclipai/adapter-claude-local exec vitest run src/server/parse.test.ts
pnpm --filter @paperclipai/server exec vitest run src/__tests__/claude-local-execute.test.ts
```
Both suites green locally. This patch is also currently running as a
hot-patch over the published `2026.513.0` adapter on the reporting
deployment — sessions that previously looped indefinitely now
self-recover on the first heartbeat after the 400 surfaces.
## Risks
- Low risk. The detector is conservative (regex over `result` +
`errors[]` only) and the rotation reuses the existing unknown-session
fallback path
- The local-only `fs.unlink` of the poisoned `.jsonl` is wrapped in
`try/catch` and ignored on failure — strictly an optimization; the
server-side session clear is the authoritative reset
- Remote execution targets (`executionTargetIsRemote`) skip the disk
cleanup because the file lives on a remote host that we can't safely
reach from the adapter
- The `clearSession: true` + nulled session fields path is a no-op on
healthy runs; it only fires when the new detector matches, so existing
successful continuations are unaffected
- No DB schema changes, no public API changes, no new dependencies
## Model Used
- Provider: Anthropic Claude
- Model: `claude-opus-4-7` (Opus 4.7)
- Context window: 1M
- Capabilities: extended reasoning, tool use, code execution
- Role: implemented the detector, expanded the fallback branch, added
the persist-guard + `clearSession`, wrote the unit + integration tests,
validated locally, and applied the equivalent hot-patch to the deployed
`2026.513.0` install while this PR is in 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 similar or duplicate PRs and linked
them — closed #2295, #2361, #3572, #5438 as duplicates of this canonical
fix; complementary fixes #4838 (heartbeat_timer reset) and #4932 (gemini
context-overflow rotation) target different code paths
- [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, adapter-only change
- [x] I have updated relevant documentation
(`docs/adapters/claude-local.md` runbook entry)
- [x] I have considered and documented any risks above
- [x] I will address all Greptile and reviewer comments before
requesting merge
---------
Co-authored-by: Danial Jawaid <danial.jawaid@gmail.com>
Co-authored-by: Paperclip <noreply@paperclip.ing>
Co-authored-by: Devin Foley <devin@paperclip.ing>
|
||
|
Dotta
|
468edd8b22 |
Add workspace file viewer and artifact links (#7681)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agent work is issue-centered, and reviewers often need to inspect files, artifacts, and path references produced during that work. > - Before this branch, workspace-relative paths and artifact file references were not first-class inspectable objects in the board UI. > - Safe file viewing needs shared resource contracts, server-side workspace boundary checks, and UI that opens files without exposing arbitrary host paths. > - The workspace file viewer branch needed to stay as one active PR and be rebased onto current `paperclipai/paperclip:master` for review. > - This pull request adds the workspace file resource API, issue-page file viewer and browser, markdown file-reference links, and artifact file chips. > - The benefit is that board users can inspect relevant files from issue context while preserving workspace boundaries and auditability. ## Linked Issues or Issue Description No public GitHub issue exists for this branch. Internal Paperclip issues: `PAP-1953`, `PAP-10539`, `PAP-10733`. Problem / motivation: - Board users need to open workspace-relative files mentioned by agents or attached as work-product metadata without switching to a terminal. - The UI needs to support both direct file-path opening and workspace browsing/searching from an issue page. - The server must enforce company access, workspace boundaries, size limits, rate limits, and safe audit logging. Related PR: - Prior closed attempt: #4442 - Single active PR for this branch: #7681 ## What Changed - Added shared workspace file resource types, validators, and workspace-file `resourceRef` metadata validation for work products. - Added server routes/services for resolving, listing, and previewing workspace-relative files with access checks, scan caps, list-specific limits, and audit logging. - Added the issue file viewer provider, sheet, workspace browser, command-palette action, markdown workspace-file autolinks, and artifact file chips. - Updated issue workspace UI and stories/tests for file browsing and workspace file opening. - Rebased the branch onto current `paperclipai/paperclip:master` and updated the existing single PR branch. - Addressed current-head Greptile follow-ups by applying `offset` consistently across search/recent/changed file listings, restoring stopped-service port ownership checks before auto-port reuse, and stabilizing the workspace browser pagination test. ## Verification Current local verification after rebase to `public/master`: - `pnpm exec vitest run packages/shared/src/work-product.test.ts server/src/__tests__/file-resources.test.ts server/src/__tests__/instance-settings-routes.test.ts server/src/__tests__/instance-settings-service.test.ts server/src/__tests__/workspace-runtime.test.ts ui/src/components/FileViewerSheet.test.tsx ui/src/components/FileViewerSheet.copy.test.tsx ui/src/components/WorkspaceFileBrowser.test.tsx ui/src/components/WorkspaceFileMarkdownBody.test.tsx ui/src/context/FileViewerContext.test.ts ui/src/lib/remark-workspace-file-refs.test.ts ui/src/lib/workspace-file-parser.test.ts ui/src/components/IssueWorkspaceCard.test.tsx` - 13 files passed, 197 tests passed. - `pnpm -r --filter @paperclipai/shared --filter @paperclipai/server --filter @paperclipai/ui typecheck` - passed. - `pnpm exec vitest run ui/src/components/WorkspaceFileBrowser.test.tsx` - 1 file passed, 25 tests passed. - `pnpm exec vitest run server/src/__tests__/file-resources.test.ts server/src/__tests__/workspace-runtime.test.ts` - 2 files passed, 90 tests passed. - `pnpm -r --filter @paperclipai/server typecheck` - passed. - Confirmed branch is `0` behind and `46` ahead of current `public/master` after rebase and follow-up commits. - Confirmed the PR diff does not include `pnpm-lock.yaml`. - Confirmed the PR diff does not include `.github/workflows` changes. - Searched GitHub for duplicate or related workspace file viewer PRs/issues; #4442 is the prior closed attempt and this PR is the single active PR for the branch. - No screenshots were committed; the task explicitly asked not to add design screenshots or images unless they were part of the work. Current remote verification on head `a698a7bc10137baf7d25bd5722e1d6e0343387c1`: - Greptile Review - success, 64 files reviewed, 0 comments added, no unresolved Greptile review threads. - PR workflow `verify` - success. - Typecheck + Release Registry, General tests, workspace test shards, serialized server suites, Build, Canary Dry Run, e2e, Socket, and Snyk - success. - `security-review` - neutral, with output saying a draft advisory was filed for maintainer review and is not a merge block. - `commitperclip PR Review / review` - cancelled after the security gate detected flags and timed out while creating/reviewing the advisory. I reran it once and it cancelled the same way; no actionable code/test failure was exposed in the job logs. ## Risks - This is a broad UI/server feature PR, so review needs to pay attention to route authorization, workspace boundary handling, and markdown autolink false positives. - Workspace browsing intentionally caps list results and scan depth; very large workspaces may require users to refine search terms. - Remote workspace preview remains unavailable until remote file-access support is implemented. - The neutral commitperclip security-review advisory needs maintainer review, but the check output says it is not a merge block. > 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 in a Paperclip/Codex local tool-use environment, medium reasoning, with shell/GitHub CLI tool use for branch inspection, verification, rebase, PR update, Greptile review, and CI inspection. ## 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 - [ ] 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> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> |
||
|
Reasonofmoon
|
a0f7d3daba |
Reset task session on timer-driven wakes (PF-4) (#4838)
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - Each agent is woken via the heartbeat scheduler — `heartbeat_timer` for periodic interval wakes, `issue_assigned` / `execution_*` / `issue_commented` for event-driven wakes > - The heartbeat reuses the prior task session by default; only specific wake reasons trigger a fresh session via `shouldResetTaskSessionForWake` (assignment, review, approval, changes-requested) or explicit `forceFreshSession` > - In CEO run `292a5fd1`, repeated context compaction warnings appeared near the 64k threshold for the long-lived manager session — symptomatic of repeated `heartbeat_timer` wakes accumulating low-value "checked, nothing new" inbox-scan traces inside one ever-growing session > - PF-4 in the 2026-04-16 hangeul-school operational issue set asks for a compaction-aware session freshness policy: "manager sessions can rotate before low-value compaction pressure accumulates" and "repeated timer wakes do not indefinitely bloat the same session" > - This pull request adds `wakeReason === "heartbeat_timer"` to both `shouldResetTaskSessionForWake` and `describeSessionResetReason`, so each interval wake starts fresh and the run log explicitly records why. Event-driven wakes (`issue_commented`, `transient_failure_retry`, etc.) keep their existing reuse behavior. > - The benefit is that timer wakes — which are exploratory and carry no continuation state — stop bloating long-lived manager sessions. Compaction pressure that previously accumulated across N timer wakes is now bounded to a single interval's worth of context. ## Linked Issues or Issue Description No external GitHub issue is linked. Describing the problem inline following the bug-report template: **What happened:** Long-lived manager/CEO agent sessions hit the 64k context-compaction threshold after many `heartbeat_timer` wakes accumulated low-value inbox-scan traces inside one ever-growing task session. Reproduced in CEO run `292a5fd1`. **Expected behavior:** Periodic timer wakes — which carry no continuation state — should not indefinitely bloat the same session. The heartbeat should rotate sessions on timer wakes the way it already does on assignment/review/approval/changes-requested wakes. **Actual behavior:** `shouldResetTaskSessionForWake` only reset on `issue_assigned`, `execution_review_requested`, `execution_approval_requested`, `execution_changes_requested`, or explicit `forceFreshSession`. `heartbeat_timer` reused the prior session indefinitely, causing compaction pressure. **Scope of fix:** Add `heartbeat_timer` to the reset list and to `describeSessionResetReason` so the run log records why. Event-driven wakes keep their existing reuse behavior. ## What Changed - `shouldResetTaskSessionForWake` (`server/src/services/heartbeat.ts`) now also returns `true` when `wakeReason === "heartbeat_timer"`. The existing reset reasons (`issue_assigned`, `execution_review_requested`, `execution_approval_requested`, `execution_changes_requested`, `forceFreshSession`) are unchanged. - `describeSessionResetReason` returns a paired explanation `"wake reason is heartbeat_timer (timer-driven wake starts fresh)"` so run logs make session reset behavior legible. - `describeSessionResetReason` was promoted from internal to `export` so the paired contract can be unit-tested directly alongside `shouldResetTaskSessionForWake`. This is the only API surface change in this PR. Wake reasons whose reuse behavior is intentionally **unchanged**: - `issue_commented` — the comment is the reason to engage; continuation context matters - `issue_comment_mentioned` — same rationale - `transient_failure_retry` — resuming a previously-failed run; want continuity - `process_lost_retry` — resuming after process loss; want continuity - `missing_issue_comment`, recovery reasons — out of scope; can be revisited as follow-ups if observed bloat shows up ## Verification ```bash cd server pnpm vitest run src/__tests__/heartbeat-timer-wake-session-reset-pf4.test.ts # 12/12 pass pnpm vitest run \ src/__tests__/heartbeat-stale-queue-invalidation.test.ts \ src/__tests__/heartbeat-process-recovery.test.ts \ src/__tests__/heartbeat-comment-wake-batching.test.ts # 48/48 adjacent heartbeat tests pass ``` The 12 new tests assert: 1. `shouldResetTaskSessionForWake` resets on `heartbeat_timer` 2. `shouldResetTaskSessionForWake` still resets on the four existing reasons 3. `forceFreshSession === true` still triggers reset 4. `issue_commented`, `transient_failure_retry`, unknown reasons, and null/undefined context do **not** trigger reset 5. `describeSessionResetReason` describes `heartbeat_timer` explicitly so logs are legible 6. `describeSessionResetReason` keeps the exact wording for the four existing reasons 7. `describeSessionResetReason` returns the `forceFreshSession` message 8. `describeSessionResetReason` returns `null` for non-resetting reasons 9. **Parity invariant**: the two functions agree on every input — `describeSessionResetReason(ctx)` is non-null iff `shouldResetTaskSessionForWake(ctx)` returns true. This locks the pair so future changes to one must update the other. ## Risks - **Low–medium.** This changes behavior for every `heartbeat_timer` wake on every agent: the prior task session is no longer reused. - For **manager / CEO agents** (the documented case): this is the intended improvement. Timer wakes carry no continuation state for these roles. - For **worker agents** that may have used timer wakes to resume in-flight work: any genuine continuation should already be triggered by issue/execution wake reasons (which still reuse) or by an active checkout being resumed via `process_lost_retry` / `transient_failure_retry`. Timer wakes themselves do not create checkouts. - If a deployment relied on timer wakes to preserve mid-task context — which is fragile by design — the right path is to switch to a non-timer wake reason or accept the reset. The PR doesn't add a new opt-out flag because the goal is to bound session size; introducing an opt-out would re-open the bloat path this PR is closing. - No schema or API surface change beyond exporting `describeSessionResetReason`. No migration. No client-visible API change. ## Model Used Claude Opus 4.7 (1M context), model ID `claude-opus-4-7[1m]`. Used in interactive Claude Code session with extended reasoning, tool use (Read/Edit/Write/Bash), and verification gates between exploration → fix → tests → push. ## 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 the open PR list for similar/duplicate work — distinct from #4080 (force-fresh follow-up wake — codex/general) and #4195 (codex session reset on model change); this PR specifically targets the `heartbeat_timer` reuse path - [x] I have run tests locally and they pass (12 new + 48 adjacent = 60 tests, no regressions) - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots — N/A, server-only change - [x] I have updated relevant documentation to reflect my changes — none needed; the new export carries clear semantics and the run log message is self-explanatory - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Irene <irene@users.noreply.github.com> Co-authored-by: Devin Foley <devin@devinfoley.com> Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Devin Foley <devin@paperclip.ing> |
||
|
Dotta
|
393e6f5e68 |
Add Claude Fable 5 and Mythos 5 to the model selector (#7826)
## Summary Adds the newly released Claude models from the [models overview](https://platform.claude.com/docs/en/about-claude/models/overview) to the `claude_local` adapter's model selector: - **Claude Fable 5** (`claude-fable-5`) — generally available as of 2026-06-09, Anthropic's most capable widely-released model. - **Claude Mythos 5** (`claude-mythos-5`) — limited availability (Project Glasswing). **Opus 4.8 stays first in the list so it remains the default selection** — per the request, the new flagship models are *offered* but not defaulted (not Fable, not Mythos). ## Changes - `packages/adapters/claude-local/src/index.ts` — add `claude-fable-5` and `claude-mythos-5` to the adapter model list, right after `claude-opus-4-8`. - `packages/adapters/claude-local/src/server/models.ts` — add the Fable 5 Bedrock identifier (`us.anthropic.claude-fable-5-v1`) to the Bedrock fallback list. Mythos 5 is limited-availability on Bedrock, so it's intentionally left out of that fallback. - `server/src/__tests__/adapter-models.test.ts` — assert the new models are present and that `claude-opus-4-8` remains first (the default). These flow through the single `claudeModels` source, so they also appear in the ACPX combined list (`registry.ts` prefixes them with `Claude:`) and are recognized by the ACPX Claude model filter. The UI selector reads models dynamically from the adapter, so no UI changes are needed. ## Testing - `npx vitest run src/__tests__/adapter-models.test.ts` — 13 passed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Dotta
|
0a2230b2ec |
[codex] Guard document comment wake boundaries (#7766)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The execution control plane uses issue comments, assignments, monitors, blockers, and interactions to decide when agent-owned work should wake and run. > - Top-level issue comments are actionable issue-thread feedback for the assignee, but document-scoped comments are review context unless they are converted into an explicit routing primitive. > - Document annotation comments were still wired into the same `issue_commented` wake path as top-level issue comments. > - That made document activity capable of waking an assignee and looking like an execution path even when no issue-level handoff happened. > - This pull request narrows the wake boundary so document annotation activity stays document-scoped while normal issue comments continue waking the assignee. > - The benefit is fewer spurious wakeups and clearer non-terminal issue liveness semantics. ## Linked Issues or Issue Description Internal Paperclip work: [PAP-10613](/PAP/issues/PAP-10613), [PAP-10640](/PAP/issues/PAP-10640) Problem description: - Document annotation thread creation and annotation comments were treated as assignee wake sources. - Document-scoped activity should remain visible as document/review context, but should not by itself act as a queued issue wake, monitor, approval, interaction response, blocker, or terminal disposition. - Top-level issue comments should still wake the assignee on agent-assigned, non-terminal issues. Related PR search performed: - Found related prior document annotation work: #6733. - Found related prior issue-comment wake work and revert context: #7678, #7765. - No existing PR for `PAP-10613-why-is-this-task-not-running`. ## What Changed - Removed the document annotation comment assignee wake helper from issue routes. - Kept document annotation reference sync and activity logging intact. - Documented the distinction between top-level issue comments and document-scoped comments in `doc/execution-semantics.md`. - Added route tests proving document/document annotation activity does not wake the assignee. - Added route coverage proving top-level board issue comments still wake the assignee. ## Verification - `pnpm exec vitest run server/src/__tests__/document-annotation-routes.test.ts server/src/__tests__/issue-update-comment-wakeup-routes.test.ts` — 2 files passed, 9 tests passed. - `pnpm --filter @paperclipai/server typecheck` — passed. - `git status -sb` — clean branch tracking `origin/PAP-10613-why-is-this-task-not-running`. ## Risks - Low to moderate behavior change: document annotation comments no longer wake the issue assignee automatically. - Operators who want document feedback to route work must use an explicit primitive such as assignment, issue-thread comment, agent mention, issue-thread interaction, approval, blocker, or delegated follow-up. - No database migration or public API shape 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 Codex, GPT-5-based coding agent with shell/tool use enabled. Exact hosted runtime model identifier beyond GPT-5 was not exposed in this session. ## 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 - [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> |
||
|
Dotta
|
7fb40264f8 |
[codex] Revert PR #7678 (#7765)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Issue comment wake handoffs are part of the control-plane execution loop that decides when agents resume work after comments and issue updates. > - PR #7678 changed that wake handoff behavior in server issue routes, heartbeat context, and related tests. > - The change broke an important workflow after merge, so the safest immediate fix is to restore the pre-#7678 wake behavior. > - This pull request reverts the wake-handoff behavior from PR #7678 while keeping narrow review-requested safeguards that prevent known runtime/test regressions. > - The benefit is that Paperclip returns to the last known working wake behavior without reintroducing avoidable UUID skill lookup and annotation-resolution test gaps. ## Linked Issues or Issue Description Refs: #7678 Bug context: - What happened: PR #7678 was reported to have broken an important Paperclip workflow after it merged. - Expected behavior: Paperclip should preserve the prior issue comment wake handoff behavior until a corrected change is ready. - Steps to reproduce: Use the workflow affected by PR #7678's issue comment wake handoff changes. - Paperclip version/commit: `master` after merge commit `4da79a88c67e54084d40bd18cada5ee5c8be23da`. - Deployment mode: Paperclip control-plane server behavior. ## What Changed - Reverted merge commit `4da79a88c67e54084d40bd18cada5ee5c8be23da` from PR #7678 to restore pre-#7678 wake-handoff behavior. - Preserved the safe accepted-plan routing check so `parseObject(...)` is not used as a boolean. - Preserved UUID filtering for run-scoped skill mentions so legacy non-UUID skill IDs do not reach a Postgres UUID lookup. - Restored the annotation thread-resolution test guard that verifies resolving a thread does not wake the assignee. ## Verification - `pnpm run preflight:workspace-links && NODE_ENV=test PAPERCLIP_HOME=/tmp/... PAPERCLIP_INSTANCE_ID=pap10614-revert TMPDIR=/tmp/... pnpm exec vitest run --project @paperclipai/server --no-file-parallelism --maxWorkers=1 server/src/__tests__/document-annotation-routes.test.ts server/src/__tests__/heartbeat-project-env.test.ts server/src/__tests__/heartbeat-accepted-plan-workspace-refresh.test.ts server/src/__tests__/heartbeat-context-summary.test.ts` - Result: 4 test files passed, 26 tests passed. - Earlier targeted revert verification also passed: 4 test files, 50 tests. ## Risks - This intentionally restores behavior from before PR #7678, so intended wake-handoff improvements from that PR are removed. - The PR is no longer a byte-for-byte revert because Greptile identified two narrow safeguards worth preserving. - Low migration risk: no schema or dependency changes are included. - Follow-up work may still be needed to reintroduce the desired wake handoff behavior without the regression. ## Model Used OpenAI Codex, GPT-5 coding agent in this Paperclip heartbeat, with shell/tool execution and repository write access. ## 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 - [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 |
||
|
Dotta
|
76c88e5855 |
[codex] Move instance settings under company settings (#7680)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Operators manage both company-scoped configuration and instance-level runtime/admin settings from the board UI > - Instance settings previously lived as their own top-level sidebar area, separate from the company settings context operators already use > - That split made settings navigation feel heavier and made instance configuration less discoverable from the settings tab > - This pull request moves instance settings under company settings while preserving the existing instance settings routes and plugin/admin surfaces > - The benefit is a smaller primary sidebar and a more coherent settings hierarchy for operators ## Linked Issues or Issue Description - Refs #338 - Internal: PAP-10491, PAP-10538 ## What Changed - Moved instance settings navigation under the company settings area. - Added route helpers and sidebar entries for nested instance settings paths. - Updated plugin/admin settings routes to use the company settings instance scope. - Preserved legacy instance-settings bookmarks through compatibility redirects that keep the active company prefix. - Updated focused UI and plugin tests for the new navigation shape. - Stabilized the process-loss retry test that was failing the serialized server shard in CI. - Rebased the branch onto current `paperclipai/paperclip` `master` and pushed the current head. ## Verification - `pnpm exec vitest run ui/src/components/CompanySettingsSidebar.test.tsx ui/src/components/access/CompanySettingsNav.test.tsx ui/src/lib/instance-settings.test.ts ui/src/components/InstanceSidebar.test.tsx ui/src/components/Layout.test.tsx ui/src/components/SidebarAccountMenu.test.tsx ui/src/pages/PluginPage.test.tsx ui/src/plugins/bridge.test.ts packages/shared/src/validators/plugin.test.ts` - `pnpm exec vitest run ui/src/lib/instance-settings.test.ts ui/src/components/CompanySettingsSidebar.test.tsx ui/src/components/access/CompanySettingsNav.test.tsx ui/src/components/Layout.test.tsx ui/src/plugins/bridge.test.ts` - `pnpm exec vitest run server/src/__tests__/heartbeat-process-recovery.test.ts -t "queues exactly one retry when the recorded local pid is dead"` - `pnpm test:run:serialized -- --shard-index 0 --shard-count 4` - GitHub PR checks are green on head `fe7b0955169dcae55cbe10889c1876a70ab0b80c`, including `verify`, `General tests (server)`, all serialized server shards, build, e2e, policy, security checks, and Greptile. - Confirmed the PR diff does not include `pnpm-lock.yaml` or `.github/workflows` changes. ## Risks - Medium UI/navigation risk: instance settings links are intentionally moving under company settings, so stale external bookmarks to legacy paths rely on the compatibility routing in this branch. - Low test-only risk from the CI stabilization commit: it makes the recovery assertion select the actual retry run by `retryOfRunId` instead of whichever non-original run appears first. - No database migrations. - No dependency lockfile or workflow 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 coding agent based on GPT-5, with shell/tool execution in a local repository worktree. Exact context window was not exposed by the runtime. ## 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 --------- Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
Nour Eddine Hamaidi
|
823c2b115a |
feat(adapters): allow external overrides of built-ins (#7394)
## Thinking Path > - Paperclip orchestrates AI agents through server-side adapters. > - Some adapters are bundled as built-ins, while external adapter plugins can provide newer or organization-specific implementations. > - The adapter registry already supports external plugins overriding a built-in type while keeping the built-in available as fallback. > - The hot-install API still rejected built-in adapter types before registration, so plugin installation did not match registry behavior. > - That blocked users from installing an external adapter update for a built-in adapter type such as `hermes_local`. > - This pull request removes the hot-install conflict guard and keeps the existing fallback lifecycle intact. > - The benefit is consistent adapter override behavior across startup registration, hot install, pause/resume, and removal. Fixes #7395 ## What Changed - Allows `POST /api/adapters/install` to register an external adapter whose type matches a built-in adapter. - Keeps built-in adapters protected from deletion unless there is an external plugin record for that adapter type. - Tightens the install route so `requiresRestart` is only reported on a true reinstall (existing external plugin record), not on a first-time override of a built-in adapter type. - Adds route coverage for installing a built-in type override, pausing back to the built-in implementation, deleting the override, and restoring the built-in adapter. ## Verification - `pnpm --filter @paperclipai/server exec vitest run src/__tests__/adapter-routes.test.ts src/__tests__/adapter-registry.test.ts` - `pnpm --filter @paperclipai/server typecheck` - GitHub Actions passed for server tests, typecheck, build, serialized server suites, e2e, canary dry run, Socket, Snyk, Greptile, and policy checks on the prior pushed commit before the follow-up review fix. ## Risks - Low risk: this only changes the hot-install/removal lifecycle for external plugins targeting a built-in adapter type. - Built-in adapters remain protected when no external plugin record exists. - The existing registry fallback behavior restores the built-in adapter when an override is paused or removed. > 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.5 via Hermes Agent for the initial implementation and verification (terminal/file/GitHub tool use). - Anthropic Claude Opus 4.7 (claude-opus-4-7) via Paperclip Claude adapter for the Greptile-feedback follow-up commit (extended-thinking reasoning, terminal/file/GitHub tool use). ## 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 confirmed none exist for this hot-install override fix - [x] I have linked the existing issue with `Fixes #7395` - [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] 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: HenkDz <henkdz@users.noreply.github.com> Co-authored-by: Devin Foley <devin@devinfoley.com> Co-authored-by: Paperclip <noreply@paperclip.ing> |
||
|
Dotta
|
2d1b531a49 |
[codex] Add clear-error agent action (#7695)
## Thinking Path > - Paperclip is the open source control plane people use to manage AI agents for work. > - Agent runtime state is surfaced in both the server API and the board UI so operators can tell whether an agent is idle, running, paused, or in error. > - When an agent is already in `error`, the existing pause/resume action slot is not useful because there is no running work to pause. > - Operators need a direct, audited recovery path that clears the stale error state only for agents in the same company. > - This pull request adds a company-scoped clear-error mutation, exposes the shared API contract, and wires the board action cluster to show Clear error in the pause/resume slot for errored agents. > - The benefit is that operators can recover CEO/CTO-style errored agents without resorting to database edits or unrelated session reset actions. ## Linked Issues or Issue Description Refs #4021 Paperclip issue: PAP-10515 — right now the CEO and CTO agents are in error state, but there is no way to clear the error; they appear otherwise fine. ## What Changed - Added shared constants, API path, and agent status type support for a company-scoped clear-error action. - Added the server service and route to clear an agent from `error` back to `idle`, with company access enforcement and activity logging. - Added OpenAPI/docs coverage for the clear-error endpoint. - Added backend coverage for service behavior and cross-tenant authorization. - Updated the board agent action cluster to show a red-tinted Clear error button only when `agent.status === "error"`. - Updated agent properties to show a red active last-error indicator only while the agent is currently errored. - Added UI component tests for the error-state action and the non-error pause/resume behavior. ## Verification Local: - `pnpm exec vitest run server/src/__tests__/agents-service-clear-error.test.ts server/src/__tests__/agent-cross-tenant-authz-routes.test.ts ui/src/components/AgentActionButtons.test.tsx` - `pnpm --filter @paperclipai/ui typecheck` - `pnpm exec vitest run server/src/__tests__/openapi-routes.test.ts` PR checks: - Main Paperclip workflow is green on `a7378e584d50594e7bd507a1a02985bfaaa5abf8`. - Greptile is 5/5 with no files requiring special attention and no new comments on the latest review. - `commitperclip PR Review` is still red because its security-gate step canceled after filing a draft advisory; the linked `security-review` check is neutral and says the draft advisory is not a merge block. Visual artifact: -  ## Risks Low to medium risk. The mutation is intentionally narrow, but reviewers should check that clearing `lastError`/`lastRunError` and returning to `idle` is the desired recovery semantics for every adapter state. The remaining red check is from the external commitperclip security-review workflow, not from the code/test workflow for this PR. > 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-family coding model, tool-assisted with local shell, git, GitHub CLI, and targeted Vitest 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 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 - [ ] 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 |
||
|
Dotta
|
4da79a88c6 |
[codex] Refine issue comment wake handoffs (#7678)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The heartbeat and issue-comment routes decide when an assigned agent wakes up and what context it receives. > - Passive comments and annotation notes can currently wake assignees even when no actionable state changed. > - Accepted planning confirmations also need to preserve recent plan comments so child-issue creation does not lose board/user constraints. > - Runtime skill mentions should only send UUID ids into database lookups, because legacy slug-like ids are not valid runtime skill ids. > - This pull request tightens those wake and handoff rules in one server-side branch. > - The benefit is fewer noisy agent wakeups and better accepted-plan continuation context without changing the task model. ## Linked Issues or Issue Description Internal Paperclip task: [PAP-10535](/PAP/issues/PAP-10535). Problem or motivation: Passive comments and annotation notes could wake the current assignee even when no actionable state changed, and accepted plan continuations needed recent plan comments preserved in the wake handoff. Runtime skill mentions also needed to ignore non-UUID ids before database lookup. Proposed solution: Tighten server-side wake routing so passive comments do not wake assignees unless they reopen the issue, preserve mention-targeted wakeups, include recent non-deleted plan comments in accepted confirmation wake payloads, and guard runtime skill mention lookup to UUID-like ids. Alternatives considered: Leaving passive assignee wakeups in place was rejected because it keeps generating noisy non-actionable heartbeats. Treating every skill mention-like token as a runtime skill id was rejected because legacy slug-like ids are not valid runtime skill ids. Roadmap alignment: This aligns with the V1 control-plane heartbeat contract by making wakeups more intentional and preserving handoff context for approved plans. This PR was split from the local `master` branch on June 7, 2026. It covers server-side heartbeat and comment-wakeup behavior only. I searched GitHub for duplicate/related PRs; the results were broader heartbeat/run PRs, not this exact passive-comment and accepted-plan handoff change. ## What Changed - Filter runtime skill mention extraction so only UUID-like skill ids are looked up. - Stop ordinary issue comments and document annotation comments from waking the current assignee unless the comment reopens the issue. - Keep mention-targeted wakeups intact while removing passive assignee wakeups. - Include recent non-deleted issue comments in accepted-plan confirmation wake payloads and task markdown. - Updated focused server tests for the new wakeup and accepted-plan behavior. ## Verification - `git diff --check origin/master..HEAD` - `NODE_ENV=test pnpm exec vitest run server/src/__tests__/heartbeat-project-env.test.ts server/src/__tests__/document-annotation-routes.test.ts server/src/__tests__/issue-comment-reopen-routes.test.ts server/src/__tests__/issue-update-comment-wakeup-routes.test.ts server/src/__tests__/heartbeat-context-summary.test.ts server/src/__tests__/issue-comment-redaction.test.ts` - `NODE_ENV=test pnpm exec vitest run server/src/__tests__/heartbeat-accepted-plan-workspace-refresh.test.ts server/src/__tests__/issue-comment-redaction.test.ts server/src/__tests__/heartbeat-context-summary.test.ts` - `pnpm --filter /server typecheck` - PR checks green on head `a379a0264d384510ff8ac4a47fb1e44d7b556f68` - Greptile rerun green on head `a379a0264d384510ff8ac4a47fb1e44d7b556f68`: 9 files reviewed, 0 comments added, 0 unresolved review threads ## Risks - Medium behavioral risk: agents will no longer wake for passive comments unless mentioned or unless the comment reopens/resumes the issue. That is intentional, but any workflow relying on passive assignee comment wakeups should use explicit mentions or structured resume paths. - Low migration risk: 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 coding agent based on GPT-5, with shell, git, GitHub CLI, and local test execution. Exact hosted model variant and context-window size were not exposed by the runtime. ## 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 - [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> |
||
|
Aron Prins
|
8b85fdfa3c |
fix(cli): send X-Paperclip-Run-Id so agents can mutate their issues via the CLI (#7642)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents act on issues through the `paperclipai` CLI as well as the HTTP API; the server gates agent-authenticated **mutations** of an in-progress issue (checkout, release, interactions, PATCH, attachment upload) behind the `X-Paperclip-Run-Id` header (`requireAgentRunId` / `assertAgentIssueMutationAllowed`). > - The CLI's HTTP client (`client/http.ts`) already supports sending that header, but `resolveCommandContext` never populated `runId`, so there was no way to provide it — every agent-authenticated mutation via the CLI failed with `401 Agent run id required`. > - Separately, `issue attachment:upload` hand-rolls its own multipart `fetch` (bypassing the JSON client), so it never forwarded the run-id at all, and its `inferContentTypeFromPath` couldn't produce `text/html` and appended `; charset=utf-8` to `md`/`txt` — which fails the server's exact-match content-type allowlist (`422 Unsupported attachment content type`). > - This PR lets the CLI send `X-Paperclip-Run-Id` from a new global `--run-id` flag (falling back to `$PAPERCLIP_RUN_ID`), and fixes `attachment:upload` to forward the run-id and emit server-allowed bare MIME types. > - The benefit is that an embodied agent can drive the full issue lifecycle (checkout → work → disposition → upload deliverable) entirely through the official CLI, instead of dropping to raw HTTP. ## Linked Issues or Issue Description No issue exactly covers the CLI **send** side, so describing it here (bug path). Related: - `Refs #2063` — "Sub-agents cannot post comments on subtickets — Agent run id required" (same error string; that report focuses on the server gate, this PR fixes the CLI not sending the header for agent mutations). - `Refs #1199` — injects `X-Paperclip-Run-Id` on the **http adapter's** outbound request (server side). This PR is the complementary **CLI client** side. **Bug (per `bug_report.yml`):** - **What happened:** Running agent-authenticated CLI mutations (`issue checkout` / `issue update` on an in-progress issue / `issue attachment:upload`) returns `401 Agent run id required`, even with `--run-id`/`$PAPERCLIP_RUN_ID` set; `attachment:upload` of an HTML/markdown deliverable additionally returns `422 Unsupported attachment content type`. - **Expected:** The CLI forwards the agent run-id so the server authorizes the mutation, and uploads use a content-type the server accepts. - **Steps to reproduce:** As an agent token, `paperclipai issue checkout <id> --agent-id <id>` then `paperclipai issue update <id> --status done` (→ 401); `paperclipai issue attachment:upload <id> ./report.html` (→ 401, then 422 once run-id is wired). - **Deployment mode:** local_trusted (applies to all modes — server-side gate is mode-independent). ## What Changed - `cli/src/commands/client/common.ts`: resolve `runId` in `resolveCommandContext` from a new global `--run-id` flag, falling back to `$PAPERCLIP_RUN_ID`, so the existing HTTP client sends `X-Paperclip-Run-Id`; thread `runId` into the attachment-upload path; align `inferContentTypeFromPath` with the server's `DEFAULT_ALLOWED_TYPES` (add `html`/`htm`/`csv`/`zip`/`mp4`/`m4v`/`webm`/`mov`/`qt`, drop the `; charset` suffix). - `cli/src/commands/client/issue.ts`: pass `ctx.api.runId` into `uploadAttachment` and send the `X-Paperclip-Run-Id` header on the hand-rolled multipart request (matching what the JSON client injects automatically). - Tests: CLI asserts `attachment:upload` forwards `x-paperclip-run-id` + the inferred bare MIME type, and that `inferContentTypeFromPath` covers the allowed types; a server test locks the contract that an in-progress checkout owner without a run-id is rejected `401` on attachment upload. ## Verification ```bash # CLI tests (no DB) node_modules/.bin/vitest run \ cli/src/__tests__/common.test.ts \ cli/src/__tests__/issue-subresources.test.ts # → 2 files, 13 tests passed # Server contract test (embedded postgres) node_modules/.bin/vitest run \ server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts # → 37 tests passed ``` Manual: with an agent token and a valid `$PAPERCLIP_RUN_ID`, `issue checkout` / `issue update --status done` / `issue attachment:upload ./report.html` now succeed where they previously returned 401/422. ## Risks Low. Additive only: - `--run-id` is a new optional flag; behavior is unchanged when it (and `$PAPERCLIP_RUN_ID`) are unset — the header is simply omitted as before. - The content-type map only **widens** the allowed set to match the server's existing allowlist and removes a suffix the server already rejected, so no previously-accepted upload changes type. - No schema/migration changes; no server behavior changes (the server test only documents the existing gate). ## Model Used Claude Opus 4.8 (1M context window), via Claude Code (tool use / agentic file edits + local test execution). Extended reasoning 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 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 (CLI-only) - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green — pending CI run on this PR - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
