fix(desktop): reap stale notify.sh paths from in-repo dev worktrees#3698
Conversation
Dev setup writes SUPERSET_HOME_DIR=<worktree>/superset-dev-data (no leading dot), which didn't match the managed-hook regex, so stale notify.sh entries from deleted dev worktrees stuck around in ~/.codex/hooks.json. Widen the pattern to also recognize `superset-dev-data/` and add a regression test.
📝 WalkthroughWalkthroughThe managed-hook command matcher now recognizes hook paths in both standard Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes a gap in the stale-hook reaper: dev worktrees set Confidence Score: 5/5Safe to merge — minimal, well-targeted regex fix with a direct regression test and no correctness or security concerns. The change is a one-line regex update with a clear motivation, a good explanatory comment, and a purpose-built test. The new superset-dev-data alternative is specific enough (combined with the existing /hooks/${scriptName} guard) to avoid false positives. No existing tests are affected and no P0/P1 findings were identified. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/agent-setup/agent-wrappers-common.ts | Widened SUPERSET_MANAGED_HOOK_PATH_PATTERN regex to also match /superset-dev-data/ path segments alongside the existing /.superset/ variants, with a clear explanatory comment. |
| apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts | Added regression test covering the in-repo .worktrees//superset-dev-data/ layout, verifying stale paths are replaced across all three hook event types (SessionStart, UserPromptSubmit, Stop). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["hooks.json entry read\n(command string)"] --> B{"normalized.includes\n'/hooks/scriptName'?"}
B -- No --> C["return false\n(not managed)"]
B -- Yes --> D{"SUPERSET_MANAGED_HOOK_PATH_PATTERN\n.test(normalized)?"}
D -- No --> C
D -- Yes --> E["return true\n(managed → eligible for reaping)"]
subgraph Pattern [Regex alternation]
F["/.superset/\nor /.superset-suffix/\n(original)"]
G["/superset-dev-data/\n(new — dev worktree layout)"]
end
D -.-> Pattern
Reviews (1): Last reviewed commit: "fix(desktop): reap stale notify.sh paths..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/agent-setup/agent-wrappers-common.ts (1)
9-13: Minor coupling tosteps.shconstant.The literal
superset-dev-datahere mirrors the directory name set by.superset/lib/setup/steps.sh(SUPERSET_HOME_DIR=$PWD/superset-dev-data). If that constant is ever renamed on the shell side, this regex will silently stop reaping stale entries again. Consider a short code comment pointing to the exactsteps.shline (or a shared constant/name comment block) so future renames stay in sync.No action required for this PR — the pre-gate on
/hooks/${scriptName}already keeps the false-positive surface minimal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers-common.ts` around lines 9 - 13, The regex constant SUPERSET_MANAGED_HOOK_PATH_PATTERN embeds the literal "superset-dev-data" which mirrors SUPERSET_HOME_DIR in the shell setup; add a short clarifying comment next to SUPERSET_MANAGED_HOOK_PATH_PATTERN that references the exact shell variable (SUPERSET_HOME_DIR) and the steps.sh location/line where it's defined (or note to keep these in sync / consider a shared constant), so future rename of SUPERSET_HOME_DIR in .superset/lib/setup/steps.sh is obvious to maintainers.apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts (1)
1196-1259: Good regression coverage.The fixture path contains no
/.superset/segment, so it can only match via the newsuperset-dev-databranch — exactly the scenario from the PR description. Assertions cover all three managed Codex events.Optional: for parity with the neighboring
replaces stale Codex hook commands from old superset pathstest, you could writecontentback tocodexHooksPathand re-invokegetCodexGlobalHooksJsonContentto assert idempotency on the dev-worktree layout too. Not required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts` around lines 1196 - 1259, Add an idempotency check to the test by writing the returned content back to the same Codex hooks file and re-calling getCodexGlobalHooksJsonContent to ensure the function is stable for dev-worktree layouts; specifically, after computing content (and after the existing assertions) write content to the codexHooksPath file and call getCodexGlobalHooksJsonContent(currentHookPath) again, asserting the new return equals the first content; reference the test's codexHooksPath variable and the getCodexGlobalHooksJsonContent function when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers-common.ts`:
- Around line 9-13: The regex constant SUPERSET_MANAGED_HOOK_PATH_PATTERN embeds
the literal "superset-dev-data" which mirrors SUPERSET_HOME_DIR in the shell
setup; add a short clarifying comment next to SUPERSET_MANAGED_HOOK_PATH_PATTERN
that references the exact shell variable (SUPERSET_HOME_DIR) and the steps.sh
location/line where it's defined (or note to keep these in sync / consider a
shared constant), so future rename of SUPERSET_HOME_DIR in
.superset/lib/setup/steps.sh is obvious to maintainers.
In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts`:
- Around line 1196-1259: Add an idempotency check to the test by writing the
returned content back to the same Codex hooks file and re-calling
getCodexGlobalHooksJsonContent to ensure the function is stable for dev-worktree
layouts; specifically, after computing content (and after the existing
assertions) write content to the codexHooksPath file and call
getCodexGlobalHooksJsonContent(currentHookPath) again, asserting the new return
equals the first content; reference the test's codexHooksPath variable and the
getCodexGlobalHooksJsonContent function when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d9b00a8-e672-499f-acf2-b381e12a990a
📒 Files selected for processing (2)
apps/desktop/src/main/lib/agent-setup/agent-wrappers-common.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
.superset/lib/setup/steps.sh) pointsSUPERSET_HOME_DIRat$PWD/superset-dev-data— without a leading dot — so the managed-hook regex/\.superset(?:-[^/'"\s\\]+)?\//never matched those paths.notify.shpath stayed in~/.codex/hooks.jsonforever, even though the file was gone.superset-dev-data/so the reaper can swap in the current hook path. Added a regression test covering the in-repo.worktrees/<name>/superset-dev-data/layout.Test plan
bun test apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.tsSummary by cubic
Fixes stale
notify.shentries in~/.codex/hooks.jsonwhen deleting in-repo dev worktrees by recognizingsuperset-dev-datapaths used by the dev setup. On desktop startup, the reaper now replaces old paths with the current hook path./.superset.../and/superset-dev-data/paths..worktrees/<name>/superset-dev-data/to ensure stale paths are removed and replaced.Written for commit 12b4319. Summary will update on new commits.
Summary by CodeRabbit