fix(dev): allow sandboxed npm to read the greptile tool store - #89645
fix(dev): allow sandboxed npm to read the greptile tool store#89645gantoine wants to merge 5 commits into
Conversation
The Seatbelt profile denies all reads under $HOME and never re-allowed ~/.config/posthog/tools, while on-activate wraps the greptile CLI npm install in bin/dev-sandbox (default on macOS). npm lstats and reads its --prefix, so the install failed with EPERM on every mac dev with the sandbox on, and hogli review fell back to manual install guidance. Read-allow only the tools subpath plus metadata on the ~/.config and ~/.config/posthog dir nodes for path canonicalization, matching the existing ~/.local -> ~/.local/share/uv pattern. The rest of ~/.config (gcloud, gh, op) stays denied. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🤖 CI report🚨 Trunk lane — universal laneThis PR is assigned to the universal lane. It cannot merge in parallel with other PRs, so it can take longer to merge. Ask dev-ex if you think this is wrong.
|
| File | Size | Δ vs base |
|---|---|---|
render-query/src/render-query/render-query.js |
21.69 MiB | 🔺 +5.9 KiB (+0.0%) |
toolbar/src/toolbar/debug/chunk-EventDebugMenu.js |
269.7 KiB | 🔺 +3.5 KiB (+1.3%) |
posthog-app/_parent/products/visual_review/frontend/scenes/VisualReviewFlakinessScene.js |
22.9 KiB | 🔺 +3.2 KiB (+16.4%) |
posthog-app/_parent/products/tracing/frontend/TracingScene.js |
100.1 KiB | 🔺 +2.9 KiB (+3.0%) |
posthog-app/_parent/products/metrics/frontend/MetricsScene.js |
63.9 KiB | 🔺 +1.7 KiB (+2.8%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.36 MiB · 22 files | no change | ███░░░░░░░ 30.3% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.78 MiB · 3,239 files | 🔺 +6.8 KiB (+0.1%) | █████████░ 90.4% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 306.5 KiB | ../node_modules/.pnpm/posthog-js@1.418.17/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 258.5 KiB | ../node_modules/.pnpm/posthog-js@1.418.17/node_modules/posthog-js/dist/module.js |
| 249.5 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 154.2 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.7 KiB | src/lib/api.ts |
| 95.2 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.25 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.25 MiB · 17 files | 🟢 -12 B (-0.0%) | ████░░░░░░ 39.3% of 5.72 MiB |
| Deferred (lazy) | 2.10 MiB · 33 files | 🔺 +3.5 KiB (+0.2%) | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 746.7 KiB | dist/toolbar/toolbar-app-RBPKVAJT.css |
| 582.8 KiB | dist/toolbar/chunk-chunk-KOF34GDY.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-PQM5GZBZ.js |
| 133.8 KiB | dist/toolbar/chunk-chunk-N62MBQ3D.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.3 KiB | dist/toolbar/toolbar-app-D67QTSQ7.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-Q4T66H6N.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-6FTFTGT4.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +414.3 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1442.67 MiB · 🔺 +414.3 KiB (+0.0%)
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
✅ Security review complete — 1 finding posted as a review (changes requested), in 8 min. Re-request a |
![posthog-security-review-bot[bot]](https://avatars.githubusercontent.com/in/4551195?s=60&v=4){kind=small}
…ging The tool store is an execute-later target: activation links its binary into the venv and hogli review runs it in the developer's shell, outside Seatbelt. The profile allowed sandboxed writes there, so a compromised dependency in a sandboxed dev service could replace the binary and run with the developer's full account on the next review. Deny sandboxed writes to ~/.config/posthog/tools, and to the ~/.config and ~/.config/posthog nodes so a rename cannot swap in a symlinked ancestor. Drop the store read allowance: npm now installs into a sandbox-writable staging dir and the unsandboxed activation shell publishes node_modules into the store. The self-test asserts the store and its ancestor nodes reject sandboxed writes and that entries inside ~/.config stay writable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![parameterai[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
The staging dir lived under TMPDIR, which every sandboxed dev service can write. A compromised dependency running concurrently with the install could swap the staged binary before the activation shell published it, reopening the execute-later escape the staging flow was meant to close. Drop the sandboxed npm run entirely: the trusted activation shell installs into the write-denied store with --ignore-scripts, so no package code runs at install time and the CLI runs unsandboxed at review time anyway. Every input npm reads is pinned away from sandboxed writers: the cache lives inside the store, the user config is disabled and the profile now write-denies ~/.npmrc (the ~/.gitconfig escape with npm as the trusted tool), and cwd plus a fresh package.json keep the project-config lookup inside the store. The self-test asserts the ~/.npmrc deny. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
…nstall The unsandboxed install resolved npm from a PATH that activation prepends with the venv bin, and $FLOX_ENV/bin reaches binaries through the repo's .flox/run symlink. Both are sandbox-writable, so a planted shim would run outside the sandbox with the developer's account. Resolve npm and flock with /usr/bin/readlink -f, refuse any result outside /nix/store, and pin PATH to the system dirs for the rest of the install. The resolved npm's shebang points at the store node, so node is never looked up via PATH. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
On macOS with the dev sandbox on (the default), the Greptile CLI never installs: every flox activation ends with "Greptile CLI install failed", and
hogli reviewreports the CLI as missing at PR-open time, so pre-PR reviews fall back.~/.config/posthog/tools/greptile/<version>inbin/dev-sandbox.$HOMEand never re-allows that store, and npm reads its--prefixpath.EPERM: lstat '~/.config'and leaves a half-created store that every activation retries.The store is also an execute-later target the profile left writable: activation links its binary into the venv, and
hogli reviewruns it in the developer's shell, outside Seatbelt. A compromised dependency in a sandboxed dev service could replace the binary and run with the developer's full account on the next review.Changes
hogli reviewfinds the binary.npm --ignore-scripts. No package code runs at install time, and the CLI runs unsandboxed at review time anyway, so sandboxing the install protected nothing. A sandboxed install was rejected twice: pointing npm at the store fails on the read deny, and a staging dir underTMPDIRgives a concurrently sandboxed dependency a window to swap the binary before publication./nix/store(PATH during activation contains sandbox-writable dirs, so a result outside the store is refused), the cache lives inside the store instead of the sandbox-writable~/.npm, the user config is disabled, and cwd plus a freshpackage.jsonkeep the project-config lookup inside the store.~/.config/posthog/tools, closing the binary-replacement route above.~/.configand~/.config/posthognodes, so a rename cannot swap in a symlinked ancestor and move the store out from under the deny. Entries inside~/.configstay writable.~/.npmrc: its registry, proxy, and cafile keys redirect where an unsandboxed npm downloads code from, the same escape class as the existing~/.gitconfigdeny.~/.configstay fully denied: the unsandboxed install removes the need for the store read allowance an earlier revision of this PR added.bin/dev-sandbox-selftestgains five checks: the store, both ancestor nodes, and~/.npmrcreject sandboxed writes, and an entry inside~/.configstays writable (the over-broad-deny control)..completestamp makes the next activation retry, and the retry now succeeds.How did you test this code?
bin/dev-sandboxfailed withEPERM: lstat '~/.config'.greptile@3.4.1, the binary runs,greptile configworks, and the private npm cache is removed./nix/store), and succeeds with the real flox npm.greptile@3.4.1has no install-time lifecycle scripts and no dependencies, so--ignore-scriptsloses nothing.rm -fof the store binary fails and the binary survives.bin/dev-sandbox-selftestpasses all checks locally, including the five new ones; CI runs the same suite on macOS when these files change.Automatic notifications
Docs update
None. The reviewing-before-pr skill's description of the store stays accurate.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
7669775e-3056-4dda-83de-5894430fef4e: no findings, confidence 5/5. Opened withno-greptileper the--checkgate.TMPDIRstaging dir was itself writable to concurrent sandboxed processes (fixed by removing sandboxed npm from the chain), and the unsandboxed npm was PATH-resolved from sandbox-writable dirs (fixed by resolving npm and flock into/nix/store).🤖 Generated with Claude Code