feat(command-safety): per-manager ALLOW_* env vars for package managers#24
Conversation
Previously the only escape from npm/yarn/pnpm/pip/python blocking was COMMAND_SAFETY_DISABLE_PACKAGE_MANAGERS=true (all-or-nothing) or a per-command --force-* bypass flag. Teams using mixed package managers had no middle ground. Add COMMAND_SAFETY_ALLOW_* vars checked at rule-load time so each manager can be individually unblocked: - COMMAND_SAFETY_ALLOW_NPM / ALLOW_NPX / ALLOW_YARN / ALLOW_PNPM - COMMAND_SAFETY_ALLOW_PIP (pip/pip3; python/python3 still blocked) - COMMAND_SAFETY_ALLOW_PYTHON (python/python3; pip still blocked) Set in ~/.zshrc.local before shell-config is sourced. The file-level COMMAND_SAFETY_DISABLE_PACKAGE_MANAGERS=true still works to disable all rules at once. 21 new regression tests cover each allow var and their independence from each other. Closes #2
Summary of ChangesHello, I'm Gemini Code Assist[^1]! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the command safety system by providing more granular control over package manager restrictions. Users can now opt-in to allow specific tools while maintaining the default blocking behavior for others, improving flexibility for specialized project requirements without compromising the overall safety policy. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Failed to generate code suggestions for PR |
|
Failed to generate code suggestions for PR |
1 similar comment
|
Failed to generate code suggestions for PR |
There was a problem hiding this comment.
Code Review
This pull request introduces fine-grained control over package manager safety rules by adding COMMAND_SAFETY_ALLOW_* environment variables. These flags allow users to selectively unblock specific tools like npm, yarn, or pip while keeping others restricted. The changes include updates to the rule definitions, documentation in the settings file, and a comprehensive suite of regression tests. Feedback suggests refactoring the new test file to use helper functions for checking blocked and unblocked states, which would reduce boilerplate and improve maintainability.
| # ============================================================================= | ||
| # Node managers | ||
| # ============================================================================= | ||
|
|
||
| @test "ALLOW_NPM: npm not blocked when COMMAND_SAFETY_ALLOW_NPM=true" { | ||
| run bash -c " | ||
| export COMMAND_SAFETY_ALLOW_NPM=true | ||
| $(_engine_with_rules) | ||
| _check_command_rules npm install lodash | ||
| " | ||
| [ "$status" -ne 1 ] | ||
| } | ||
|
|
||
| @test "ALLOW_NPM: yarn still blocked when only COMMAND_SAFETY_ALLOW_NPM=true" { | ||
| run bash -c " | ||
| export COMMAND_SAFETY_ALLOW_NPM=true | ||
| $(_engine_with_rules) | ||
| _check_command_rules yarn add lodash | ||
| " | ||
| [ "$status" -eq 1 ] | ||
| } | ||
|
|
||
| @test "ALLOW_NPM: pnpm still blocked when only COMMAND_SAFETY_ALLOW_NPM=true" { | ||
| run bash -c " | ||
| export COMMAND_SAFETY_ALLOW_NPM=true | ||
| $(_engine_with_rules) | ||
| _check_command_rules pnpm install lodash | ||
| " | ||
| [ "$status" -eq 1 ] | ||
| } | ||
|
|
||
| @test "ALLOW_NPX: npx not blocked when COMMAND_SAFETY_ALLOW_NPX=true" { | ||
| run bash -c " | ||
| export COMMAND_SAFETY_ALLOW_NPX=true | ||
| $(_engine_with_rules) | ||
| _check_command_rules npx create-react-app my-app | ||
| " | ||
| [ "$status" -ne 1 ] | ||
| } | ||
|
|
||
| @test "ALLOW_NPX: npm still blocked when only COMMAND_SAFETY_ALLOW_NPX=true" { | ||
| run bash -c " | ||
| export COMMAND_SAFETY_ALLOW_NPX=true | ||
| $(_engine_with_rules) | ||
| _check_command_rules npm install lodash | ||
| " | ||
| [ "$status" -eq 1 ] | ||
| } | ||
|
|
||
| @test "ALLOW_YARN: yarn not blocked when COMMAND_SAFETY_ALLOW_YARN=true" { | ||
| run bash -c " | ||
| export COMMAND_SAFETY_ALLOW_YARN=true | ||
| $(_engine_with_rules) | ||
| _check_command_rules yarn add lodash | ||
| " | ||
| [ "$status" -ne 1 ] | ||
| } | ||
|
|
||
| @test "ALLOW_YARN: npm still blocked when only COMMAND_SAFETY_ALLOW_YARN=true" { | ||
| run bash -c " | ||
| export COMMAND_SAFETY_ALLOW_YARN=true | ||
| $(_engine_with_rules) | ||
| _check_command_rules npm install lodash | ||
| " | ||
| [ "$status" -eq 1 ] | ||
| } | ||
|
|
||
| @test "ALLOW_PNPM: pnpm not blocked when COMMAND_SAFETY_ALLOW_PNPM=true" { | ||
| run bash -c " | ||
| export COMMAND_SAFETY_ALLOW_PNPM=true | ||
| $(_engine_with_rules) | ||
| _check_command_rules pnpm install lodash | ||
| " | ||
| [ "$status" -ne 1 ] | ||
| } | ||
|
|
||
| @test "ALLOW_PNPM: yarn still blocked when only COMMAND_SAFETY_ALLOW_PNPM=true" { | ||
| run bash -c " | ||
| export COMMAND_SAFETY_ALLOW_PNPM=true | ||
| $(_engine_with_rules) | ||
| _check_command_rules yarn add lodash | ||
| " | ||
| [ "$status" -eq 1 ] | ||
| } |
There was a problem hiding this comment.
These tests are great for ensuring correctness, but there's a lot of boilerplate repeated in each @test block. To improve maintainability and make the test suite easier to read and extend, consider using helper functions for the common test patterns.
For example, you could define _test_unblocked and _test_still_blocked helpers:
# Helper to check if a command is NOT blocked when its allow-var is set
_test_unblocked() {
local var_to_set="$1"; shift
local -a cmd_to_run=("$@")
run bash -c "
export $var_to_set=true
$(_engine_with_rules)
_check_command_rules ${cmd_to_run[@]}
"
[ "$status" -ne 1 ]
}
# Helper to check if a command IS blocked when another allow-var is set
_test_still_blocked() {
local var_to_set="$1"; shift
local -a cmd_to_run=("$@")
run bash -c "
export $var_to_set=true
$(_engine_with_rules)
_check_command_rules ${cmd_to_run[@]}
"
[ "$status" -eq 1 ]
}With these helpers (placed after _engine_with_rules), your tests become much more concise:
@test "ALLOW_NPM: npm not blocked when COMMAND_SAFETY_ALLOW_NPM=true" {
_test_unblocked COMMAND_SAFETY_ALLOW_NPM npm install lodash
}
@test "ALLOW_NPM: yarn still blocked when only COMMAND_SAFETY_ALLOW_NPM=true" {
_test_still_blocked COMMAND_SAFETY_ALLOW_NPM yarn add lodash
}
@test "ALLOW_NPM: pnpm still blocked when only COMMAND_SAFETY_ALLOW_NPM=true" {
_test_still_blocked COMMAND_SAFETY_ALLOW_NPM pnpm install lodash
}There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ef2b227f1
ℹ️ 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".
| if [[ "${COMMAND_SAFETY_ALLOW_PYTHON:-}" != "true" ]]; then | ||
|
|
||
| # Catches: python script.py, python -c "code", python -m module, piped python | ||
| _rule PYTHON_DIRECT cmd="python" \ |
There was a problem hiding this comment.
Exempt
python -m pip when ALLOW_PIP is enabled
When COMMAND_SAFETY_ALLOW_PIP=true is set, pip/pip3 are unblocked by skipping their rules, but python -m pip ... and python3 -m pip ... are still blocked because the catch-all PYTHON_DIRECT/PYTHON3_DIRECT rules still match every python/python3 invocation. This breaks a common pip workflow (often the only available one in managed environments) and makes the new per-manager allow flag incomplete for pip usage.
Useful? React with 👍 / 👎.
Summary
COMMAND_SAFETY_ALLOW_*env vars so each package manager can be individually unblocked without touching the othersCOMMAND_SAFETY_DISABLE_PACKAGE_MANAGERS=truestill works as an all-or-nothing kill switchNew vars
Set in
~/.zshrc.localbefore shell-config is sourced:Tests
21 new regression tests in
tests/regression/package-manager-allow-vars.batscover:Closes #2