Skip to content

fix: make required team hooks cross-platform#2217

Open
neallee2012 wants to merge 5 commits into
garrytan:mainfrom
neallee2012:neallee2012-cross-platform-gstack-hooks
Open

fix: make required team hooks cross-platform#2217
neallee2012 wants to merge 5 commits into
garrytan:mainfrom
neallee2012:neallee2012-cross-platform-gstack-hooks

Conversation

@neallee2012

Copy link
Copy Markdown

Summary

Required team mode currently generates a Bash hook and registers it with Bash environment-variable syntax. Windows and Copilot hook runners can select a different shell, turning an intentional policy denial into Denied by preToolUse hook ... (hook errored).

This PR makes required-mode enforcement shell-neutral:

  • Generate .claude/hooks/check-gstack.cjs with an explicit CommonJS extension and a Node 18+ implementation.
  • Register one node -e launcher that reads CLAUDE_PROJECT_DIR through process.env; no shell field or Bash, PowerShell, or cmd environment expansion is used.
  • Return neutral {} when the global install is present. The hook never emits permissionDecision: "allow", so normal permission prompts remain intact.
  • Return structured PreToolUse deny JSON with exit code 0 when gstack is absent, its install cannot be verified, the project directory is blank or stale, or the hook cannot be loaded. These are policy denials, not hook execution errors.
  • Resolve home directories through HOME, then USERPROFILE, then os.homedir().

Legacy required-mode projects migrate cleanly:

  • Matching Bash registrations are replaced and duplicate gstack handlers are removed without touching unrelated hooks.
  • The obsolete .claude/hooks/check-gstack.sh file is removed only after the CommonJS hook and settings registration succeed.
  • If Git tracks the legacy file, the generated git add command includes its deletion so the suggested migration commit leaves no unstaged change.

Test plan

  • bun test --timeout 30000 test\team-mode.test.ts twice: 32 passed, 0 failed, 114 assertions per run.
  • Default-shell execution covers neutral success, intentional denial, blank project context, and nonexistent or incomplete project paths.
  • Windows PowerShell execution covers blank project context, stale project paths, and USERPROFILE home fallback.
  • Injected filesystem verification failures remain structured deny decisions with exit code 0.
  • Legacy migration executes the exact printed git add command, commits it, and verifies a clean worktree while preserving an unrelated hook.
  • bash -n bin/gstack-team-init.
  • git diff --check.

neallee2012 and others added 4 commits July 10, 2026 19:56
Generate one Node CommonJS enforcement hook, fail closed when project context or the global install cannot be verified, and cover cmd and PowerShell execution paths.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Catch project hook resolution and load failures in the cross-shell Node launcher, returning a sanitized structured denial instead of a module-loader error.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@trunk-io

trunk-io Bot commented Jul 10, 2026

Copy link
Copy Markdown

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

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

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@time-attack

Copy link
Copy Markdown
Contributor

@16francej — recommend closing this PR as superseded by #2229. It offers useful portability and migration groundwork, but omits Copilot response details and includes unrelated files; #2229 provides the narrower cross-host implementation. Credit #2217 for the CommonJS hook, shell-neutral launcher, fail-closed errors, and legacy migration, while leaving its unrelated files out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants