fix(skills): symlink node_modules in Codex mirror instead of deep-copying#2733
fix(skills): symlink node_modules in Codex mirror instead of deep-copying#2733VXNCXNX wants to merge 2 commits into
Conversation
…ying
The Codex skills mirror deep-copied each skill into ~/.agents/skills with
fs.cp({ recursive: true, dereference: true }) and no filter, so a skill's
node_modules was duplicated on every startup, every 30-min tick, and after
any skill change. A dependency-heavy skill (~1.1 GB, ~700 MB node_modules)
became a ~1.4 GB copy, most of it duplicated deps.
- Add copySkillDirLinkingNodeModules(): skip the skill's top-level
node_modules during the copy (fs.cp filter) and symlink it to the source
instead. Deps still resolve through the symlink, so mirrored skills stay
runnable, with no duplication, no transient disk peak, and no per-startup
I/O churn. Used by mirrorUserSkillsToCodex and syncCodexSkills.
- Add POSTHOG_DISABLE_CODEX_MIRROR=1 opt-out for users who don't use Codex;
gates the startup calls and early-returns both copy paths.
Tests: node_modules becomes a symlink and a dep resolves through it; a skill
without node_modules is mirrored unchanged; the opt-out no-ops both paths.
Generated-By: PostHog Code
Task-Id: 212331c1-dba9-4b85-b5a3-be8416d292d6
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/workspace-server/src/services/posthog-plugin/codex-mirror.ts:49-55
Redundant `rm` before symlink creation — the `filter` passed to `fs.cp` already ensures `destNodeModules` is never written, and all callers remove `dest` before invoking this function (the stated contract). The extra `rm` is always a no-op and adds noise without adding safety.
```suggestion
if (fs.existsSync(srcNodeModules)) {
const destNodeModules = path.join(dest, "node_modules");
// "junction" so this also works on Windows without elevated permissions;
// the type arg is ignored on POSIX. Target is absolute, as junctions require.
await fs.promises.symlink(srcNodeModules, destNodeModules, "junction");
}
```
### Issue 2 of 2
packages/workspace-server/src/services/posthog-plugin/codex-mirror.test.ts:140-155
**Env-var save/restore repeated across test files**
The identical `prev = process.env[key]` / `try { set } finally { restore }` block appears here and again in `update-skills-saga.test.ts` (lines 55–71). Per the "OnceAndOnlyOnce" simplicity rule, a small shared helper — e.g. `withEnvVar(key, value, fn)` — would keep the restoration logic in one place and make both opt-out tests shorter.
Reviews (1): Last reviewed commit: "fix(skills): symlink node_modules in Cod..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (fs.existsSync(srcNodeModules)) { | ||
| const destNodeModules = path.join(dest, "node_modules"); | ||
| await fs.promises.rm(destNodeModules, { recursive: true, force: true }); | ||
| // "junction" so this also works on Windows without elevated permissions; | ||
| // the type arg is ignored on POSIX. Target is absolute, as junctions require. | ||
| await fs.promises.symlink(srcNodeModules, destNodeModules, "junction"); | ||
| } |
There was a problem hiding this comment.
rm before symlink creation — the filter passed to fs.cp already ensures destNodeModules is never written, and all callers remove dest before invoking this function (the stated contract). The extra rm is always a no-op and adds noise without adding safety.
| if (fs.existsSync(srcNodeModules)) { | |
| const destNodeModules = path.join(dest, "node_modules"); | |
| await fs.promises.rm(destNodeModules, { recursive: true, force: true }); | |
| // "junction" so this also works on Windows without elevated permissions; | |
| // the type arg is ignored on POSIX. Target is absolute, as junctions require. | |
| await fs.promises.symlink(srcNodeModules, destNodeModules, "junction"); | |
| } | |
| if (fs.existsSync(srcNodeModules)) { | |
| const destNodeModules = path.join(dest, "node_modules"); | |
| // "junction" so this also works on Windows without elevated permissions; | |
| // the type arg is ignored on POSIX. Target is absolute, as junctions require. | |
| await fs.promises.symlink(srcNodeModules, destNodeModules, "junction"); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/workspace-server/src/services/posthog-plugin/codex-mirror.ts
Line: 49-55
Comment:
Redundant `rm` before symlink creation — the `filter` passed to `fs.cp` already ensures `destNodeModules` is never written, and all callers remove `dest` before invoking this function (the stated contract). The extra `rm` is always a no-op and adds noise without adding safety.
```suggestion
if (fs.existsSync(srcNodeModules)) {
const destNodeModules = path.join(dest, "node_modules");
// "junction" so this also works on Windows without elevated permissions;
// the type arg is ignored on POSIX. Target is absolute, as junctions require.
await fs.promises.symlink(srcNodeModules, destNodeModules, "junction");
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| it("does nothing when POSTHOG_DISABLE_CODEX_MIRROR=1", async () => { | ||
| await createSkill(userDir, "alpha"); | ||
| const prev = process.env.POSTHOG_DISABLE_CODEX_MIRROR; | ||
| process.env.POSTHOG_DISABLE_CODEX_MIRROR = "1"; | ||
| try { | ||
| await mirrorUserSkillsToCodex(userDir, codexDir); | ||
| } finally { | ||
| if (prev === undefined) { | ||
| delete process.env.POSTHOG_DISABLE_CODEX_MIRROR; | ||
| } else { | ||
| process.env.POSTHOG_DISABLE_CODEX_MIRROR = prev; | ||
| } | ||
| } | ||
|
|
||
| expect(existsSync(path.join(codexDir, "alpha"))).toBe(false); | ||
| }); |
There was a problem hiding this comment.
The identical prev = process.env[key] / try { set } finally { restore } block appears here and again in update-skills-saga.test.ts (lines 55–71). Per the "OnceAndOnlyOnce" simplicity rule, a small shared helper — e.g. withEnvVar(key, value, fn) — would keep the restoration logic in one place and make both opt-out tests shorter.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/workspace-server/src/services/posthog-plugin/codex-mirror.test.ts
Line: 140-155
Comment:
**Env-var save/restore repeated across test files**
The identical `prev = process.env[key]` / `try { set } finally { restore }` block appears here and again in `update-skills-saga.test.ts` (lines 55–71). Per the "OnceAndOnlyOnce" simplicity rule, a small shared helper — e.g. `withEnvVar(key, value, fn)` — would keep the restoration logic in one place and make both opt-out tests shorter.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30c01f1c49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await fs.promises.rm(destNodeModules, { recursive: true, force: true }); | ||
| // "junction" so this also works on Windows without elevated permissions; | ||
| // the type arg is ignored on POSIX. Target is absolute, as junctions require. | ||
| await fs.promises.symlink(srcNodeModules, destNodeModules, "junction"); |
There was a problem hiding this comment.
When the target home directory is on a filesystem or policy that rejects symlinks/junctions, this throws after the skill has already been copied without node_modules. syncCodexSkills then swallows the error and leaves a mirrored skill missing its dependencies, while mirrorUserSkillsToCodex deletes/drops the user-skill mirror; before this change the deep copy path still worked in that environment. Please fall back to copying node_modules or clean up the partial destination when symlink creation fails.
Useful? React with 👍 / 👎.
- Fall back to copying node_modules when symlink/junction creation is rejected (restricted filesystem or sandbox policy), so a mirrored skill never ends up without its dependencies. Preserves the pre-change deep-copy behavior in those environments. - Extract the env save/restore boilerplate in the opt-out tests into a shared withEnvVar() helper (test-helpers.ts), used by both test files. - Drop the dead rm before the happy-path symlink (the fs.cp filter never writes node_modules and callers remove dest first). Generated-By: PostHog Code Task-Id: 212331c1-dba9-4b85-b5a3-be8416d292d6
|
Thanks for the review. Addressed all three points in
|
1 participant
Summary
On every app launch (and again every 30 minutes, and after any skill create/edit/install),
PosthogPluginServicere-mirrors all user skills into~/.agents/skills. The copy isfs.cp(..., { recursive: true, dereference: true })with no filter, so each skill'snode_modulesis re-duplicated from scratch every single time the app starts, not copied once.For a dependency-heavy skill (~1.1 GB with ~700 MB
node_modules) that means a ~1.4 GB copy under~/.agents/skills(most of it duplicated deps), rewritten on every startup. The recurring disk churn and startup I/O is the real cost here, and there's currently no way to opt out of the mirror.Changes
copySkillDirLinkingNodeModules(): skips the skill's top-levelnode_modulesduring the copy (via anfs.cpfilter), then symlinks it to the source. Dependencies still resolve through the symlink, so mirrored skills stay runnable, with no duplication, no transient disk peak, and no per-startup I/O churn. Used by bothmirrorUserSkillsToCodexandsyncCodexSkills.POSTHOG_DISABLE_CODEX_MIRROR=1env opt-out for users who don't use Codex; gates the startup calls (with a log line) plus early-returns in both copy paths.Tests
packages/workspace-servervitest, all green:node_modulesbecomes a symlink and a dependency resolves through itnode_modulesis mirrored unchangedmirrorUserSkillsToCodex+syncCodexSkills)pnpm --filter @posthog/workspace-server testpasses;tsc --noEmitclean; biome clean.Notes
Playwright browser binaries live in the shared
~/Library/Caches/ms-playwright, outsidenode_modules, so they are unaffected.Possible follow-up (happy to do if wanted): surface the opt-out as a Settings toggle backed by
IWorkspaceSettings(aSettingRow/Switch, mirroring the existing "Keep awake while agents work" pattern). Left out here to keep this focused, since the placement and default feel like a product call for the team.Created with PostHog Code