Update rshell help command output#213
Conversation
|
@codex review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eeacd0e55a
ℹ️ 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
There was a problem hiding this comment.
Self-review summary
Adds a feature-topic dimension to help (in addition to commands) and re-labels output from "builtins" to "commands". Code is small, contained to the help builtin and a new builtins/features.go registry, with no filesystem or external command access introduced. Sandbox integrity is unaffected — features and unsupported summaries are read-only static data, and the only new code path (builtins.Feature(name)) is a map lookup, so no path traversal, injection, or DoS vector.
Overall assessment: safe to merge after the P3 nits below. Only minor robustness/coverage suggestions.
Findings
Test coverage
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
help (no args, all allowed) |
unrestricted.yaml | TestHelpListsFeaturesAndUnsupportedSummary | Covered |
help (restricted) |
restricted.yaml | TestHelpRestrictedShowsOnlyAllowedInTable | Covered |
help --all |
restricted_all_flag.yaml / unrestricted_all_flag.yaml | TestHelpAllFlag* | Covered |
help --help |
help_flag.yaml | TestHelpFlagPrintsUsage | Covered |
help <feature> w/ Supported+Unsupported |
feature_detail.yaml | TestHelpShowsFeatureHelp | Covered |
help <feature> Notes-only (bash-divergences) |
— | — | Missing |
help unsupported (label not topic) |
unsupported_not_topic.yaml | TestUnsupportedIsSummaryNotTopic | Covered |
help <unknown> |
too_many_args.yaml | TestHelpUnknownTopicShowsError | Covered |
| Builtin-vs-feature name collision panic | — | TestCommandRegisterPanicsOnFeatureName | Covered |
Overall: Adequate, with one small gap (Notes-only feature display).
Positive observations
- Sandbox stays clean: no new filesystem access, no
ospackage functions introduced, no use ofos/exec,unsafe, ornet/http. - The feature/builtin name collision is detected at registration time via
panic, which fails loud at startup rather than at runtime. The init-timefeatureByNamemap and thesync.Once-guardedregisterBuiltins()make ordering safe. - Documentation (
README.md,SHELL_FEATURES.md) is kept in sync with the new behaviour, and the help builtin'sDescriptionfield is updated, satisfying the project rule about builtin metadata. - New scenario tests use
skip_assert_against_bash: truecorrectly (help is rshell-specific). printBulletSectioncorrectly skips empty sections, sobash-divergences(Notes-only) renders without orphan headers.
|
Iteration 1 — self-review: 5 findings, all P3 (no P0/P1/P2). No sandbox/security or correctness issues. Findings: shallow copy in |
- builtins/features.go: deep-copy returned FeatureMeta slices so callers cannot mutate the registry. Reword `commands` description to follow the consistent "...; no ..." pattern. Reword `bash-divergences` description to drop "including" since only one divergence is captured here. - builtins/help/help.go: document in the --all flag help text that --all is ignored when a topic argument is given. - builtins/builtins.go: add comment explaining the redundant feature-collision check in addToRegistry is defense-in-depth. - tests/scenarios/cmd/help: update existing scenarios to match the new feature descriptions and add feature_notes_only.yaml covering the Notes-only feature display path (help bash-divergences).
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Self-review summary (iteration 2)
Iteration 1 fixed all 5 P3 nits flagged previously. PR is in good shape — all CI green, all prior threads resolved. One small consistency finding remains:
Findings
| # | Priority | File | Finding |
|---|---|---|---|
| 1 | builtins/features.go:181-185 |
Feature(name) still returns a FeatureMeta with shared inner slices — Features() was deep-copied in iter 1 but the single-getter wasn't |
Test coverage
feature_notes_only.yaml was added in iter 1 covering the previously-uncovered Notes-only feature display path. Coverage now adequate.
Positive observations
- Iter 1 fixes are consistent with the original review feedback; no regressions introduced.
make fmt,go build, full test suite, and bash comparison all pass locally; CI green for the new commit.- Defense-in-depth comment in
addToRegistryclearly explains the redundancy.
|
Iteration 2 — self-review: 1 finding (P3). Inconsistency: |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ 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". |
Iter 1 deep-copied Features() so callers cannot mutate the registry. Apply the same treatment to the single-feature getter — featureByName returns the registry's FeatureMeta value whose Supported/Unsupported/Notes slices share backing arrays with featureRegistry.
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Self-review summary (iteration 3)
Iter 1 fixed the original 5 P3 nits. Iter 2 fixed the Feature(name) shared-slice consistency issue. The PR now reads cleanly — all sandbox/security invariants preserved, no filesystem access, no new external-command handling, all CI green.
Findings
No new findings from this self-review.
Note: outstanding Codex comment
A pre-existing Codex review comment (builtins/features.go:32) was not picked up by the loop's filter (the GitHub login is chatgpt-codex-connector without the [bot] suffix the filter uses). It points at the commands feature's "Command options that would … execute programs … are rejected." line — find -exec / -execdir are supported under AllowedCommands, so the wording is misleading. I'll address it in the next sub-step.
|
Iteration 3 — self-review: 0 new findings. Will address the pre-existing Codex comment on |
Codex flagged that "Command options that would ... execute programs ... are rejected." is misleading because find -exec / -execdir are supported predicates gated by AllowedCommands. Reword to keep the write-files / sandbox-bypass restriction but explicitly note that external program execution is allowed when the policy permits it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb81ec7def
ℹ️ 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".
Codex flagged that the project rule "Set exit code 0 and return. Help is not an error." (docs/RULES.md, Help Flag) was being violated — help --help printed usage but returned exit 1. Make it return 0 and update the package doc comment, scenario test, and Go test to match. Too-many-args still exits 1 (real misuse).
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Self-review summary (iteration 4)
All previously flagged findings (5 P3 from iter 1, 1 P3 from iter 2, 1 P3 + 1 P2 from Codex in iter 3) are addressed and resolved. The PR is fully clean — sandbox-safe, bash-compatible (where applicable), well-tested, well-documented.
Findings
No new findings.
Iteration recap
| Iter | New findings | Fixed |
|---|---|---|
| 1 | 5 P3 (self) | 5 |
| 2 | 1 P3 (self) | 1 |
| 3 | 1 P3 + 1 P2 (Codex) | 2 |
| 4 | 0 | — |
|
Iteration 4 — self-review: 0 findings. PR fully clean across all dimensions. |
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Self-review summary (iteration 5)
PR remains fully clean. No new findings; no fixes applied. This is the fifth and final consecutive clean iteration.
|
Iteration 5 — self-review: 0 findings. Final clean iteration. |
Review-Fix Loop Summary
Iteration log
Final state
Fixes applied across iterationsIter 1 (commit 505457e):
Iter 2 (commit cb81ec7):
Iter 3 (commits 36e6fd4, bd0f1f9):
Remaining issuesNone. |
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Iteration 1 self-review: no new findings; PR is clean.
|
Iteration 1 (run 2) — self-review: 0 findings. PR clean. |
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Iteration 2 self-review: no new findings; PR is clean.
|
Iteration 2 (run 2) — self-review: 0 findings. PR clean. |
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Iteration 3 self-review: no new findings; PR is clean.
|
Iteration 3 (run 2) — self-review: 0 findings. PR clean. |
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Iteration 4 self-review: no new findings; PR is clean.
|
Iteration 4 (run 2) — self-review: 0 findings. PR clean. |
|
@codex review this PR |
AlexandreYang
left a comment
There was a problem hiding this comment.
Iteration 5 self-review: no new findings; PR is clean.
|
Iteration 5 (run 2) — self-review: 0 findings. PR clean. Final iteration. |
Review-Fix Loop Summary (run 2)
Iteration log
Final state
Remaining issuesNone. (All actionable findings were addressed in the previous loop run; this run confirms the PR remains clean across 5 consecutive iterations.) |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ 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". |
Summary
Example output
help(all commands allowed)help variables(feature topic with Supported + Not supported)help bash-divergences(feature topic with only Notes)Restricted policy (
AllowedCommands=[rshell:echo, rshell:help])When the policy restricts commands, the
Commands:table only lists what's enabled and the rest collapses into a compactDisabled commands:line:Tests