Allowed Commands#89
Conversation
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed PR #89 (alex/allowed_commands2 → main). The PR diff contains only changes to two skill markdown files (.claude/skills/code-review/SKILL.md and .claude/skills/review-fix-loop/SKILL.md). These are documentation/tooling improvements to the review-fix-loop and code-review skills.
However, the PR description defines a SPECS section requiring implementation of allowedCommands config, allowAllCommands config, and CLI flags. None of these specs are implemented in the diff.
Spec Coverage
| Spec | Implemented | Location | Notes |
|---|---|---|---|
Implement allowedCommands config for rshell and CLI (--allowed-command) |
No | — | No implementation found in diff |
If allowedCommands is falsy, no commands are allowed |
No | — | No implementation found in diff |
Implement allowAllCommands / --allow-all-commands for testing convenience |
No | — | No implementation found in diff |
Findings Summary
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
PR description | All three SPECS are unimplemented — PR contains only skill doc changes |
Skill Documentation Changes (what IS in the diff)
The actual changes are reasonable improvements to skill files:
- code-review/SKILL.md: Adds a new "Step 2: Verify specs implementation" section that checks PR descriptions for SPECS and validates they are implemented. Good addition.
- review-fix-loop/SKILL.md: Increases iteration limit from 10 to 30, adds iteration tracking in task subjects, requires 5 consecutive Step 3 successes before completion, and enriches @codex review comments with specs instructions.
These skill changes are fine on their own, but this PR's stated purpose (per the SPECS) is to implement allowedCommands — which is entirely missing.
Positive Observations
- The skill documentation changes are well-structured and internally consistent
- The specs verification step in the code-review skill is a good process improvement
Overall Assessment: Needs major rework — the core feature described in SPECS is not implemented.
Note: This review would be REQUEST_CHANGES but GitHub does not allow requesting changes on your own PR. Treating this as REQUEST_CHANGES.
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
|
On it |
|
Iteration 1 self-review result: REQUEST_CHANGES
|
![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: 55b819be15
ℹ️ 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".
Summary
PR / Commit
Testing
|
…flags Implement the three SPECS from the PR description: 1. AllowedCommands config and CLI --allowed-commands flag: restricts which commands (builtins or external) the interpreter may execute. Only commands in the list are permitted; others are rejected with "command not allowed" (exit 127). 2. Falsy-means-no-commands: when AllowedCommands is not set and AllowAllCommands is false, no commands are allowed by default. 3. AllowAllCommands config and CLI --allow-all-commands flag: bypasses the AllowedCommands restriction for testing convenience. Updates all existing Go tests to use AllowAllCommands() where needed. The YAML scenario test harness defaults to allow-all when the field is omitted, preserving backward compatibility for existing 1998 scenarios. Adds 6 new YAML scenario tests and 4 new CLI tests for the feature. Updates README.md and SHELL_FEATURES.md documentation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
All three SPECS are now implemented in commit efcb5f6:
Changes span 29 files: core implementation in |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary (Self-Review — Iteration 2)
Reviewed the full diff for the allowedCommands feature. The implementation is clean, secure, and well-tested. Would APPROVE but cannot approve own PR.
Spec Coverage
| Spec | Implemented | Location | Notes |
|---|---|---|---|
allowedCommands config + CLI --allowed-commands |
Yes | interp/api.go:418, cmd/rshell/main.go:103 |
Fully implemented |
If allowedCommands is falsy, no commands allowed |
Yes | interp/runner_exec.go:247 |
nil map lookup returns false; blocks all |
allowAllCommands / --allow-all-commands |
Yes | interp/api.go:432, cmd/rshell/main.go:104 |
Fully implemented |
All specs are implemented.
Security Analysis
- Command gate placement: The
allowedCommandscheck inr.call()(runner_exec.go:247) is positioned before both the builtin lookup and the exec handler call. All command execution flows through this single gate. No bypass paths exist. - Subshell inheritance:
subshell()copiesrunnerConfigwhich includes bothallowedCommandsandallowAllCommands, so restrictions are properly inherited by pipes and subshells. - Command substitution: Blocked at AST validation level, so cannot be used to bypass command restrictions.
- Function declarations: Blocked at AST validation level.
- Default-deny: When neither
AllowedCommandsnorAllowAllCommandsis set,allowedCommandsis nil andallowAllCommandsis false, so!false && !nil[name]evaluates totrue, blocking all commands. Correct default-deny behavior.
Test Coverage
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
| Allowed command runs | allowed_command_runs.yaml | TestAllowedCommandsFlag | Covered |
| Disallowed command blocked | disallowed_command_blocked.yaml | TestAllowedCommandsBlocksOther | Covered |
| No commands allowed (default) | no_commands_allowed.yaml | TestDefaultNoCommandsAllowed | Covered |
| AllowAllCommands bypass | allow_all_commands.yaml | TestAllowAllCommandsFlag | Covered |
| Multiple allowed commands | multiple_allowed_commands.yaml | — | Covered |
| Execution continues after blocked cmd | allowed_after_blocked.yaml | — | Covered |
Coverage is adequate.
Findings
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
tests/scenarios_test.go:191 |
Test harness ignores allow_all_commands when allowed_commands is also set |
| 2 | |
interp/api.go:424 |
AllowedCommands accepts empty-string and path-containing names without validation |
Both findings are low-priority and non-blocking.
Positive Observations
- The single-gate pattern in
r.call()is the correct approach — easy to audit, impossible to accidentally bypass. - Using
map[string]boolgives O(1) lookups and naturally handles nil (blocks all) vs empty (blocks all) vs populated (allows listed). - Backward compatibility in the test harness (defaulting to AllowAllCommands when the field is omitted) is well-designed.
- Good documentation updates in README.md and SHELL_FEATURES.md.
- All new scenarios correctly use
skip_assert_against_bash: truesince this is intentional divergence from bash.
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
|
Iteration 2 self-review result: APPROVE (COMMENT)
|
- Reorder else-if chain in test harness so allow_all_commands takes priority over allowed_commands, matching the runtime behavior in runner_exec.go. - Reject empty strings in AllowedCommands with an error to prevent confusing silent no-ops. - Document that command names are matched exactly against args[0]. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efcb5f6833
ℹ️ 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".
… failures When Go's fuzz engine cancels workers after -fuzztime expires, fuzz functions using context.Background() don't receive the cancellation signal. This causes the fuzz coordinator to report "context deadline exceeded" as a test failure. Fix by deriving per-iteration contexts from t.Context() so fuzz engine cancellation propagates to runner.Run(). Also add t.Context().Err() guards in differential fuzz tests to bail out early when the fuzz engine stops, preventing false comparison failures from partial output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary (Self-Review, Iteration 3)
Reviewed all 46 changed files. This is a self-review (cannot APPROVE own PR).
Reviewed:
- Core implementation (
interp/api.go,interp/runner_exec.go) - CLI integration (
cmd/rshell/main.go,cmd/rshell/main_test.go) - Test harness updates (
tests/scenarios_test.go,builtins/testutil/testutil.go) - 6 new scenario tests for allowed commands
- ~20 existing test files updated to use
AllowAllCommands() - Fuzz test context handling fixes (
t.Context()migration) - Documentation updates (
README.md,SHELL_FEATURES.md)
Spec Coverage
| Spec | Implemented | Location | Notes |
|---|---|---|---|
allowedCommands config for Runner |
Yes | interp/api.go:422-434 |
AllowedCommands() RunnerOption with map-based lookup |
CLI --allowed-commands flag |
Yes | cmd/rshell/main.go:106 |
Comma-separated list, properly parsed |
If allowedCommands is falsy, no commands allowed |
Yes | interp/runner_exec.go:247 |
nil map + false allowAll = all blocked |
allowAllCommands / --allow-all-commands |
Yes | interp/api.go:439-444, main.go:107 |
Bypasses allowedCommands check |
Security Analysis
- Command gate placement: The allowed commands check in
call()(line 247) executes before bothbuiltins.Lookup()andr.exec(), ensuring no command can run unchecked. - Single entry point:
r.call()is the only call site for command execution. - Subshell inheritance:
allowedCommandsandallowAllCommandsare inrunnerConfig(immutable), inherited by subshells. No bypass possible. - Input validation: Empty strings in
AllowedCommandsare rejected at construction time. - Map lookup on nil:
r.allowedCommands[name]on a nil map returnsfalsein Go — correct deny-by-default.
Test Coverage
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
| Allowed command runs | allowed_command_runs.yaml |
TestAllowedCommandsFlag |
Covered |
| Disallowed command blocked | disallowed_command_blocked.yaml |
TestAllowedCommandsBlocksOther |
Covered |
| No commands allowed (default) | no_commands_allowed.yaml |
TestDefaultNoCommandsAllowed |
Covered |
| Allow all commands | allow_all_commands.yaml |
TestAllowAllCommandsFlag |
Covered |
| Multiple allowed commands | multiple_allowed_commands.yaml |
— | Covered |
| Execution continues after blocked cmd | allowed_after_blocked.yaml |
— | Covered |
| Empty command name rejected | — | (constructor validation) | Covered |
Verdict: No findings (P0: 0, P1: 0, P2: 0, P3: 0)
The implementation is secure, correct, well-tested, and properly documented. Would APPROVE if this were not a self-review.
Reviewed by code-review skill, iteration 3
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
|
Iteration 3 self-review result: APPROVE
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9adf7295dd
ℹ️ 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".
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary (Iteration 4)
Result: APPROVE (submitted as COMMENT due to GitHub self-review restriction) — 0 findings.
Reviewed the full diff (44 files) implementing the allowedCommands / allowAllCommands feature.
Spec Coverage
| Spec | Implemented | Location | Notes |
|---|---|---|---|
allowedCommands config for runner + CLI --allowed-commands |
Yes | interp/api.go:422-433, cmd/rshell/main.go:106 |
Fully implemented |
If allowedCommands is falsy, no commands are allowed |
Yes | interp/runner_exec.go:247 |
nil map lookup returns false |
allowAllCommands / --allow-all-commands for testing convenience |
Yes | interp/api.go:439-443, cmd/rshell/main.go:107 |
Bypasses check correctly |
Security Analysis
- Single chokepoint: The allowed-commands check at
runner_exec.go:247is the sole gate before both builtin dispatch (builtins.Lookup) and external exec (r.exec). No bypass paths exist. - Subshell inheritance:
subshell()copiesrunnerConfigwhich includes bothallowedCommandsandallowAllCommands— subshells, pipes, and command substitution all inherit the restriction. - Empty string validation:
AllowedCommandsrejects empty strings in the name list, preventing accidental wildcard entries. - Default-deny: When neither
AllowedCommandsnorAllowAllCommandsis set,allowedCommandsisnilandallowAllCommandsisfalse, so the check!r.allowAllCommands && !r.allowedCommands[name]correctly blocks all commands. - No filesystem bypass: No new
os.Open/os.Stat/os.ReadFilecalls were introduced outside the sandbox.
Test Coverage
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
| Allowed command runs | allowed_command_runs.yaml |
TestAllowedCommandsFlag |
Covered |
| Disallowed command blocked (exit 127) | disallowed_command_blocked.yaml |
TestAllowedCommandsBlocksOther |
Covered |
| Multiple allowed commands | multiple_allowed_commands.yaml |
— | Covered |
| No commands allowed (default) | no_commands_allowed.yaml |
TestDefaultNoCommandsAllowed |
Covered |
allow_all_commands bypasses check |
allow_all_commands.yaml |
TestAllowAllCommandsFlag |
Covered |
| Execution continues after blocked command | allowed_after_blocked.yaml |
— | Covered |
| Empty string rejected | — | Validated via AllowedCommands option |
Covered |
Backward compat (existing tests use AllowAllCommands) |
All existing scenario/Go tests updated | — | Covered |
Coverage: Adequate — all new/changed code paths are covered.
Positive Observations
- Clean separation of
runnerConfig(immutable) vsrunnerState(mutable) makes it easy to verify that command restrictions can't be mutated at runtime. - The test harness three-way logic (
AllowAllCommandsexplicit true / explicit false / nil default) correctly maintains backward compatibility while enabling the new feature. - Documentation updated in both
README.mdandSHELL_FEATURES.md. - All scenario tests correctly use
skip_assert_against_bash: truesince this feature intentionally diverges from bash behavior.
| # | Priority | File | Finding |
|---|---|---|---|
| — | — | — | No findings |
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
|
Iteration 4 self-review result: APPROVE
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9adf7295dd
ℹ️ 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".
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary (Iteration 5 — Stability Check)
Reviewed: PR #89 — allowedCommands config for the restricted shell interpreter.
Overall assessment: safe to merge — no findings. (COMMENT instead of APPROVE due to GitHub self-review restriction.)
SPECS Coverage
| Spec | Implemented | Location | Notes |
|---|---|---|---|
allowedCommands config for rshell and CLI (--allowed-commands) |
Yes | interp/api.go:422-434, cmd/rshell/main.go:106 |
Fully implemented — AllowedCommands([]string) RunnerOption + CLI flag |
If allowedCommands is falsy, no commands are allowed |
Yes | interp/runner_exec.go:247 |
nil map lookup returns false; exit code 127 + stderr message |
allowAllCommands / --allow-all-commands for testing convenience |
Yes | interp/api.go:439-444, cmd/rshell/main.go:107 |
Bypasses allowedCommands check entirely |
Existing tests use allowAllCommands: true for backward compat |
Yes | tests/scenarios_test.go:183-194, builtins/testutil/testutil.go:60 |
Test harness defaults to AllowAllCommands when allow_all_commands is omitted |
Security
- Single enforcement point at
runner_exec.go:247— thecall()function is the sole entry for both builtins and external commands. The check!r.allowAllCommands && !r.allowedCommands[name]is correct: nil map lookup safely returns false. - Empty string validation —
AllowedCommandsrejects empty strings at construction time (api.go:426-428). - No bypass vectors —
allowedCommandsandallowAllCommandslive in immutablerunnerConfig, set only duringNew(). Subshells inherit viarunnerConfigcopy. Scripts cannot mutate these fields. - Exit code 127 matches bash convention for "command not found".
Test Coverage
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
| Command in allowedCommands runs | allowed_command_runs.yaml |
TestAllowedCommandsFlag |
Covered |
| Command not in allowedCommands blocked | disallowed_command_blocked.yaml |
TestAllowedCommandsBlocksOther |
Covered |
| No commands allowed (falsy) | no_commands_allowed.yaml |
TestDefaultNoCommandsAllowed |
Covered |
allowAllCommands permits any command |
allow_all_commands.yaml |
TestAllowAllCommandsFlag |
Covered |
| Multiple allowed commands | multiple_allowed_commands.yaml |
— | Covered |
| Non-fatal: execution continues after block | allowed_after_blocked.yaml |
— | Covered |
| Empty command name rejected | — | inline test | Covered |
CLI --allowed-commands flag |
— | TestAllowedCommandsFlag |
Covered |
CLI --allow-all-commands flag |
— | TestAllowAllCommandsFlag |
Covered |
| Help output shows new flags | — | TestHelp |
Covered |
Positive Observations
- Clean separation of immutable config (
runnerConfig) and mutable state (runnerState) ensures the allow-list cannot be tampered with at runtime. - The test harness three-way logic (
allow_all_commands: true/false/ omitted) maintains backward compatibility elegantly using*bool. - All allowed_commands scenario tests correctly set
skip_assert_against_bash: truesince this is intentional divergence from bash behavior. - Documentation (README.md, SHELL_FEATURES.md) updated to reflect the new feature.
No P0, P1, P2, or P3 findings. This is consistent with iterations 3 and 4 (both APPROVE with 0 findings). The implementation is stable.
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
|
Iteration 5 self-review result: APPROVE
|
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary (Iteration 6)
Stability check — no code changes since iteration 2. Reviewed the full diff (44 files, ~584 changed lines).
Overall Assessment: APPROVE — safe to merge
(Submitted as COMMENT because GitHub does not allow self-approval.)
No P0, P1, P2, or P3 findings.
| Priority | Count |
|---|---|
| P0 (critical) | 0 |
| P1 (urgent) | 0 |
| P2 (normal) | 0 |
| P3 (low) | 0 |
Spec Coverage
| Spec | Implemented | Location | Notes |
|---|---|---|---|
allowedCommands config for rshell and CLI (--allowed-commands) |
Yes | interp/api.go:422-434, cmd/rshell/main.go:106 |
Map-based lookup, exact name match, empty string rejected |
If allowedCommands is falsy, no commands are allowed |
Yes | interp/runner_exec.go:247-251 |
nil map lookup returns false; default is deny-all |
allowAllCommands / --allow-all-commands for testing convenience |
Yes | interp/api.go:439-444, cmd/rshell/main.go:107 |
Bypasses allowedCommands check |
Existing tests updated to use allowAllCommands: true |
Yes | All test files | testutil, interp tests, CLI tests, scenario harness default |
Security Analysis
- Command allowlist enforcement is correctly placed in
call()before both builtin lookup and external exec, ensuring no bypass path exists. - The check
!r.allowAllCommands && !r.allowedCommands[name]is safe: a nil map lookup returnsfalsein Go, so the default (no options set) blocks all commands. allowedCommandsandallowAllCommandsare inrunnerConfig(immutable after construction) — no mutation path from script execution.subshell()copiesrunnerConfigby value, so child shells inherit the same restrictions.- Exit code 127 for blocked commands matches bash convention for "command not found".
- Empty command name validation in
AllowedCommands()prevents accidental wildcard entries.
Test Coverage: Adequate
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
| Allowed command runs | allowed_command_runs.yaml |
TestAllowedCommandsFlag |
Covered |
| Disallowed command blocked (exit 127) | disallowed_command_blocked.yaml |
TestAllowedCommandsBlocksOther |
Covered |
| Multiple allowed commands | multiple_allowed_commands.yaml |
— | Covered |
| No commands allowed (explicit false) | no_commands_allowed.yaml |
TestDefaultNoCommandsAllowed |
Covered |
| Allow all commands bypass | allow_all_commands.yaml |
TestAllowAllCommandsFlag |
Covered |
| Execution continues after blocked cmd | allowed_after_blocked.yaml |
— | Covered |
| Empty command name rejected | — | (constructor error) | Covered |
| CLI flags | — | TestAllowedCommandsFlag, TestAllowAllCommandsFlag, TestHelp |
Covered |
| Default deny (no flags) | — | TestDefaultNoCommandsAllowed |
Covered |
Positive Observations
- Clean separation: allowedCommands is a security config field in
runnerConfig, not mutable state - Test harness backward-compatibility logic (
*boolforAllowAllCommandswith nil = allow-all default) is well-designed - Empty string validation in
AllowedCommands()is a good defensive measure - All existing tests updated consistently — no test was missed
This is the 4th consecutive APPROVE with 0 findings. The implementation is solid.
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9adf7295dd
ℹ️ 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".
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary (Self-Review — Iteration 7)
Stability check — reviewed all 46 changed files. No code changes since iteration 2. Previous 4 self-reviews were APPROVE with 0 findings.
Specs Coverage
| Spec | Implemented | Location | Notes |
|---|---|---|---|
allowedCommands config for rshell and CLI (--allowed-commands) |
Yes | interp/api.go:422-434, cmd/rshell/main.go:56-57,106 |
Fully implemented as RunnerOption + CLI flag |
If allowedCommands is falsy, no commands are allowed |
Yes | interp/runner_exec.go:247 |
nil map lookup returns false; enforced before both builtin and exec paths |
allowAllCommands / --allow-all-commands for testing convenience |
Yes | interp/api.go:439-444, cmd/rshell/main.go:107 |
Bypasses allowedCommands check entirely |
Security
- Command enforcement is correctly placed in
call()(runner_exec.go:247) — the single entry point for all command execution. The check runs before bothbuiltins.Lookupandr.exec, so there is no bypass path. - Subshells inherit restrictions via
runnerConfigcopy insubshell()(api.go:453). - Empty command names are rejected at construction time (api.go:427).
- No new imports of unsafe packages.
Test Coverage
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
| Allowed command runs | allowed_command_runs.yaml |
TestAllowedCommandsFlag |
Covered |
| Disallowed command blocked | disallowed_command_blocked.yaml |
TestAllowedCommandsBlocksOther |
Covered |
| Multiple allowed commands | multiple_allowed_commands.yaml |
— | Covered |
| Allow all commands | allow_all_commands.yaml |
TestAllowAllCommandsFlag |
Covered |
| No commands allowed (default) | no_commands_allowed.yaml |
TestDefaultNoCommandsAllowed |
Covered |
| Execution continues after blocked cmd | allowed_after_blocked.yaml |
— | Covered |
| Empty string rejection | — | (validated at construction) | Covered |
| CLI --help includes new flags | — | TestHelp |
Covered |
| Backward compat (existing tests use AllowAllCommands) | All existing tests | All existing tests | Covered |
Findings
No findings. P0: 0, P1: 0, P2: 0, P3: 0.
Positive Observations
- Clean separation between config (immutable
runnerConfig) and state (mutablerunnerState) ensures command restrictions cannot be modified at runtime. - Test harness backward compatibility is well-handled with the
*booltype forAllowAllCommandsin scenario struct. - Documentation (README.md, SHELL_FEATURES.md) updated.
Verdict: APPROVE (submitted as COMMENT because GitHub blocks self-approval).
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
|
Iteration 7 self-review result: APPROVE
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6604064e21
ℹ️ 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".
Review-Fix Loop Summary
Iteration log
Final state
Key changes made
Remaining issues
|
|
Summary
Testing
|
AlexandreYang
requested review from
astuyve,
julesmcrt,
matt-dz and
thieman
as code owners
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eca3ca3841
ℹ️ 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".
| // names and vice versa. Empty strings are rejected. | ||
| // | ||
| // When not set (default), no commands are allowed unless [AllowAllCommands] is | ||
| // used. |
There was a problem hiding this comment.
How do we imagine we'll extend this if we allow executing external binaries at some point? We might have a few categories of command we can execute then:
- rshell builtins
- host shell (sh, bash, zsh, whatever?) builtins
- external binaries by name (discovered through PATH)
- external binaries by path
Would it make sense to add a namespace to the allowlist to start differentiating these? So instead of having tail we'd have something like rshell:tail?
There was a problem hiding this comment.
hum, interesting, maybe we can add that, but later, when we have the need for it?
we might also need to handle cases where builtin / host command shadowing the other one.
thieman
left a comment
thieman
left a comment
There was a problem hiding this comment.
One thought here otherwise LGTM
# Conflicts: # builtins/tests/echo/echo_fuzz_test.go
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-devflow-36fce6
bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-mergequeue-cf854d
bot
merged commit 8913075
into
main
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e8924f6df
ℹ️ 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".
| } else if len(sc.Input.AllowedCommands) > 0 { | ||
| opts = append(opts, interp.AllowedCommands(sc.Input.AllowedCommands)) | ||
| } else if sc.Input.AllowAllCommands == nil { | ||
| // Default: allow all commands for backward compatibility with | ||
| // existing scenarios that predate the allowedCommands feature. |
There was a problem hiding this comment.
This branch treats allowed_commands: [] the same as an omitted field, because len(sc.Input.AllowedCommands) > 0 is false and the fallback then enables interp.AllowAllCommands(). With gopkg.in/yaml.v3 (imported in this file), an explicit empty list unmarshals as a non-nil empty slice, so this case is distinguishable; as written, scenarios that explicitly request an empty allowlist will silently run with all commands enabled and can miss command-gating regressions.
Useful? React with 👍 / 👎.
The Allowed Commands feature (merged in #89) requires tests to explicitly opt in to command execution via interp.AllowAllCommands(). Our ip test helpers were written before this feature and did not include it, causing all ip tests to fail with exit code 127 ("command not found"). Add interp.AllowAllCommands() to: - builtins/tests/ip/helpers_test.go (runScriptCtx base options) - interp/builtin_ip_pentest_test.go (runScriptWithCtx) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
SPECS
allowedCommandsconfig for the rshell and CLI (--allowed-commands). The value is a list of command names (builtin or host/external command). There is no need for backward compatibility. Existing tests can be updated to useallowAllCommands: true(see below) to keep existing "allow all" behaviour.allowedCommandsis falsy, no commands are allowedallowAllCommands / --allow-all-commandsthat will allow any command, and will ignoreallowedCommandsconfig.Extra notes: