mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-09 16:02:36 +01:00
Nicky LeachandClaude Opus 5.5
913102870d
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - On a managed instance under `enableManagedSandboxOnly`, boot points the instance default at the managed sandbox when nothing deliberate is set > - That write read the default and wrote it later without a lock. Two overlapping boots, or a boot and an admin write, could overwrite a value the other had just committed > - A revert path also reset that stamped default whenever the mode was turned off, so a platform-filled default could change again later > - A planned plugin-environment default should fill only an unset default and never change a set one, which needs every instance's default to stay set > - This pull request makes the fill one locked transaction and removes the revert, so a filled default is an ordinary set value > - The benefit is a single, race-free rule: boot fills an unset default once, and nothing changes a set default afterwards ## Linked Issues or Issue Description Refs #19551 (the plugin-environment default that builds on this rule) **Subsystem affected** Server environments: the managed environment bootstrap and the instance settings route. **Problem or motivation** `applyManagedEnvironments` read the instance default, decided, and wrote it in separate steps with no lock, so a concurrent boot or admin write could be overwritten. When `enableManagedSandboxOnly` was not declared, a revert reset a default the platform had filled. That needed a `managedDefaultStamped` marker, plus a marker clear on every admin default write. A later boot could therefore change a default that had already been set. **Proposed solution** Fill the default in one transaction that locks the instance default row and rechecks it. Fill only an unset, local or deleted default. Remove the revert and its marker, so a filled default behaves like any other set value. **Alternatives considered** Keeping the revert and adding locks to it. A revert on a mode change would still move an existing instance's default later, which a fill-only plugin default cannot tolerate. **Roadmap alignment** This narrows existing managed-environment behaviour. It adds no product surface. ## What Changed - New `server/src/services/managed-instance-default.ts`: `fillUnsetInstanceDefault(db, environmentId)`. In one transaction it creates the settings row if needed, locks the instance default row (`FOR UPDATE`), and rechecks the current value. It writes only when the default is unset, names the local environment, or names a deleted environment. Any other set default is kept. - `server/src/services/managed-environments.ts`: - Under `enableManagedSandboxOnly`, the managed sandbox fill now uses that helper, through a new `fillInstanceDefault` test seam. - The mode-off revert and the `managedDefaultStamped` marker are removed. - The unused `instanceSettings` seam option is removed, along with the `getById`, `update` and `findManagedSandboxEnvironment` methods of the environments seam. - `server/src/routes/instance-settings.ts`: an explicit default write is a plain `update` again. The marker clear and its transaction are gone. - `doc/DEVELOPING.md`: the "Managed environments" section states the fill rule. Existing rows may still carry `managedDefaultStamped` in their metadata. Nothing reads it any more, and it is not part of the managed stock hash. ## Verification - `cd server && npx vitest run src/__tests__/managed-instance-default.test.ts` (embedded Postgres): - The fill writes an unset default, including on a fresh instance with no settings row. It also writes a default that names the local environment, or one whose environment was deleted. - It never changes a default set to an ssh, plugin or other sandbox environment, and leaves a default that already names the target alone. - Controlled overlap: an uncommitted admin default write makes the fill wait, and the fill then keeps the admin's value. This test fails without the row lock. - Two overlapping fills: exactly one writes. - Real boot pass: the default is filled under managed-sandbox-only and stays the same after a boot with the mode off. A set default stays on every boot. - `npx vitest run src/services/managed-environments.test.ts src/services/managed-environments-plugin-entries.test.ts`: the fill runs only under managed-sandbox-only, with the managed sandbox row. It never runs for a plugin environment. Nothing is written when the mode is off. A failed fill does not fail the ensure. - `npx vitest run src/__tests__/instance-settings-routes.test.ts`: an explicit default write is a plain update that touches no environment row. - `npx vitest run src/__tests__/managed-plugin-environments.test.ts src/__tests__/environment-routes.test.ts src/__tests__/server-startup-feedback-export.test.ts src/__tests__/instance-settings-service.test.ts`: pass. - `pnpm --filter @paperclipai/server typecheck` passes. - I did not run the full `pnpm test:run`, `pnpm -r typecheck` or `pnpm build`. CI covers them. ## Risks - Behaviour change: when `enableManagedSandboxOnly` is turned off for an instance, a default the platform filled with the managed sandbox now stays. Local execution is still allowed then, but it is no longer the default unless an admin picks it. - No schema change. Stale `managedDefaultStamped` keys stay in some row metadata and are ignored. - The fill holds the instance default row lock for one short transaction at boot. > 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 - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, through Claude Code with tool use (file edits, shell, test runs). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>