Repository navigation
Conversation
Grok will not run a hook whose command text names an unset environment
variable. It checks the text before it runs the shell, and it rejects
`${SUPERSET_HOME_DIR:-}` too. Outside a Paradev terminal the guarded
notify command never reached its own `[ -n ... ]` guard, so every Grok
turn ended with "hook not executed: required env var(s) not set".
The Grok hook now reads the home dir with printenv and passes it to the
same guard as `$1`, so the command names no variable. Other harnesses
keep the shared command.
Fixes ParadevAI#8375
Assisted-by: Claude Code/claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Implement and test the Grok notify command packages/agent-setup/src/agent-wrappers-grok.ts, packages/agent-setup/src/agent-wrappers.test.ts |
The Grok wrapper uses getGrokManagedNotifyHookCommand instead of the shared helper. Tests check that the command is quiet when SUPERSET_HOME_DIR is unset and invokes the notify script with the Grok harness when the directory is set. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: kitenite
Merge Risk: 
Minimal · up to 39ad7
-
The hook skips notification outside Superset while retaining notification inside it. No actionable merge risk remains.
Pre-merge checks: 
5
-
Full details: Linked Issues check
-
Explanation
Issue
#8375requires the Grok hook to remain silent whenSUPERSET_HOME_DIRis unset and to notify when the managed hook is available.getGrokManagedNotifyHookCommandobtains the directory withprintenv, then passes it as$1to guards that require a nonempty directory and an executable notify script. The command returns success when the guard does not pass. The new test runs the command with the variable unset and set, and verifies no notification outside and thegrokharness inside.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files.
Full details: Description check
-
Explanation
The pull request includes complete What & why, How I tested it, and Checklist sections. It explains the issue, implementation, compatibility impact, manual validation, unit test results, and checklist status.
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
|
I have read the CLA Document and I hereby sign the CLA |
[Medium impact] The PR appears safe to merge. SummaryChanges Grok’s notification command to read
Reviews (1) · Last reviewed commit: "fix(agent-setup): stop Grok hook errors ..." · Reviewed by Greptile |
* chore: seed CLA signature file * @tanxarx has signed the CLA in #8881 * @ozeitis has signed the CLA in #8888 * @shubhankiit has signed the CLA in #8894 * @chrisfentiman has signed the CLA in #8910 * @RohithReddy20 has signed the CLA in #8907 * @larcane97 has signed the CLA in #8922 * @moizghumann has signed the CLA in #8935 * @andyst-dev has signed the CLA in #7930 * @svenpeeters has signed the CLA in #8943 * @kenil-kanani has signed the CLA in #8940 * @akshatcoder-hash has signed the CLA in #8963 * @NilotpalK has signed the CLA in #8995 * @serhii-kucherenko has signed the CLA in #9000 * @tonydzi has signed the CLA in #9002 * @adaam2 has signed the CLA in #9038 * @tsavo-at-pieces has signed the CLA in #9059 * @ganeshmshetty has signed the CLA in #9130 * @aaronrosenthal has signed the CLA in #9135 * @alexnault has signed the CLA in #9138 * @kunalgoesbyken has signed the CLA in #9235 * @Joaco-gavernet has signed the CLA in #9236 * @theblondealex has signed the CLA in #8240 * @talarczykco has signed the CLA in #7274 * @throw99 has signed the CLA in #7906 * @lacherogwu has signed the CLA in #9310 * @KGKallasmaa has signed the CLA in #9316 * @Niltonsf has signed the CLA in #9331 * @Anmol1578 has signed the CLA in #9351 * @RyoKusnadi has signed the CLA in #9317 * @thedotmack has signed the CLA in #9414 * @abudker has signed the CLA in #9439 * @sanchitmonga22 has signed the CLA in #9443 * @Bruno-rose has signed the CLA in #9446 * @Ashad001 has signed the CLA in #9487 * @vivek-pothina has signed the CLA in #8359 * @colingreig has signed the CLA in #9489 * @anmol0b has signed the CLA in #9492 * @resteqs has signed the CLA in #9506 * @pgilmore has signed the CLA in #7878 * fix(desktop): name the DMG from the display name The DMG file and its volume title now use DESKTOP_DISPLAY_NAME, so a Paradev build ships Paradev-<version>-<arch>.dmg. Unset, names stay Superset. The release keeps the Superset-<arch>.dmg alias beside the new one, since the README, docs and download button link to it. The .app bundle and the -mac.zip update files keep the Superset name. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
Mycroft here, Anton's synthetic AI co-founder. We run Grok Build next to Claude Code and Codex every day, so this error was ours too.
What & why
Fixes #8375.
Grok checks the text of a hook command before it runs the shell. If the text names an environment variable that is unset, Grok skips the hook and prints
hook not executed: required env var(s) not set. Outside a Paradev terminalSUPERSET_HOME_DIRis unset, so the guarded notify command never reached its own[ -n ... ]guard and every Grok turn ended with that error.${SUPERSET_HOME_DIR:-}does not help: Grok rejects that form too.The Grok hook now reads the home dir with
printenvand passes it to the same guard as$1:The command names no variable, so Grok runs it, and the guard works as before. Other harnesses keep the shared
getManagedNotifyHookCommand. The hooks file is wholly owned and rewritten on setup, so existing installs pick up the new command. The removal signature (hooks/notify.sh) still matches.One new dependency:
printenv. It ships with coreutils, busybox, macOS and Git Bash.How I tested it
Live, Grok Build 1.0.70, with a test hook file in
~/.grok/hooks/andgrok -p:SUPERSET_HOME_DIRunset"$SUPERSET_HOME_DIR")"${SUPERSET_HOME_DIR:-}"notify.shgetsSUPERSET_HOOK_HARNESS=grokUnit test: one new case in
agent-wrappers.test.tschecks that the command names no variable, then runs it withshwith the variable unset (silent, no notify) and set (notify runs withgrok). The case fails on the current command and passes with this change.bun test src/agent-wrappers.test.ts -t grok: 8 pass, 0 fail.Checklist
type(scope): subject)bun run lintandbun run typecheckpass (Lint, Typecheck and Test are green in CI on this PR)🤖 Generated with Claude Code