feat(ps): add --proc-path parameter to CLI and API#127
Conversation
…roc filesystem path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nstruction Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ult logic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AlexandreYang
requested review from
astuyve,
julesmcrt,
matt-dz and
thieman
as code owners
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds a --proc-path CLI flag and a ProcPath(path string) RunnerOption API option that lets operators redirect the ps builtin to a custom proc filesystem path (default: /proc). The change threads the new value cleanly through runnerConfig → CallContext → all procinfo functions on all platforms.
Overall assessment: safe to merge with minor documentation gaps.
No security vulnerabilities introduced. The ProcPath value is set exclusively by the Go host at construction time and is immutable during script execution — a running script has no way to influence it. The direct os.* calls in procinfo_linux.go pre-existed on main and are explicitly exempt via the builtins/internal/ location.
Findings Summary
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
SHELL_FEATURES.md |
ProcPath RunnerOption missing from docs |
| 2 | |
builtins/ps/ps_procpath_linux_test.go |
GetSession (default ps with no flags) not tested with fake proc path |
| 3 |  |
builtins/ps/ps_procpath_linux_test.go:77 |
pid parameter of writeFakeProc is silently ignored (_ = pid) |
Findings
P2 — ProcPath RunnerOption not documented
Category: Documentation
Location: SHELL_FEATURES.md (not in diff — needs separate change)
SHELL_FEATURES.md lists all RunnerOptions at lines 98–100:
- ✅ AllowedCommands — ...
- ✅ AllowAllCommands — ...
- ✅ AllowedPaths filesystem sandboxing — ...
The new ProcPath option is absent. Per AGENTS.md: "README.md and SHELL_FEATURES.md must be kept up to date with the implementation."
Remediation: Add to SHELL_FEATURES.md in the RunnerOptions section:
- ✅ ProcPath — overrides the proc filesystem path used by `ps` (default `/proc`; Linux-only; useful for testing/container environments)
P2 — GetSession (default ps) not covered by fake-proc tests
Category: Test Coverage
Location: builtins/ps/ps_procpath_linux_test.go
The test file covers:
ps -e→ListAll(TestProcPathFakeProc, TestProcPathFakeProcFullFormat)ps -p 1→GetByPIDs(TestProcPathFakeProcByPID)
But the default invocation ps (no flags) routes through GetSession, which on Linux re-reads per-PID stat files from procPath to resolve the session ID. There is no test verifying that GetSession reads from the custom procPath rather than the real /proc.
Remediation: Add a TestProcPathFakeProcSession that calls runScriptWithProcPath(t, "ps", procPath) with a fake proc and verifies it neither crashes nor reads the real /proc.
P3 — pid parameter of writeFakeProc is ignored
Category: Code Quality
Location: builtins/ps/ps_procpath_linux_test.go:77–104
The helper accepts a pid int parameter but always writes files under subdirectory "1" and suppresses the parameter with _ = pid. A caller passing a different value would get a misleading result.
Remediation: Either remove the pid parameter and document the hardcoded PID 1, or use it to construct the correct subdirectory and stat content.
Positive Observations
ProcPathis correctly placed inrunnerConfig(immutable after construction) — no running script can modify it.resolveProcPathis cleanly extracted into the platform-neutralprocinfo.go, avoiding duplication across platform files.- The
filepath.Joinmigration removes the fragilefmt.Sprintf("/proc/%d/stat", pid)pattern. - All four platform implementations (Linux, Darwin, Windows, other) are updated consistently.
- The
writeFakeProctest helper provides genuine isolation from/procon the test host. TestProcPathEmptyUsesDefaultandTestProcPathNonexistentDirErrorscover the two most important boundary cases.
|
@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 1 self-review result: COMMENT — 3 findings (2×P2, 1×P3) Summary:
No P0/P1 blockers. Safe to merge with minor doc and test gaps fixed. |
![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: eb9b7c8dd7
ℹ️ 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".
…roc, add session test, update docs - Introduce builtins.ProcProvider (proc_provider.go): wraps procinfo path in an opaque struct so callers cannot override the configured proc path. Addresses thieman's concern that passing procPath as a plain string param allowed any builtin to bypass the sandbox-configured path. - Replace CallContext.ProcPath string with CallContext.Proc *ProcProvider; update runner_exec.go to construct ProcProvider from r.procPath. - Update ps.go to call callCtx.Proc.ListAll/GetSession/GetByPIDs instead of passing procPath directly to procinfo functions. - Fix writeFakeProc to use the pid parameter instead of always writing to directory "1" (P3 self-comment). - Add TestProcPathFakeProcSession to cover the bare ps → GetSession code path with a fake proc directory (P2 self-comment). - Add ProcPath to SHELL_FEATURES.md RunnerOptions section (P2 review body). - Add path/filepath.Join and strconv.Itoa to allowedsymbols allowlists for the procinfo package; these were used in procinfo_linux.go but missing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
[Claude Sonnet 4.6] Addressed review feedback from @AlexandreYang:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d09c26bcb0
ℹ️ 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".
Add a base-path accessibility check at the start of getByPIDs so that a bad --proc-path is reported as an error by ps -p, consistent with how ps -e already fails via os.ReadDir in listAll. Also add a test TestProcPathNonexistentDirErrorsByPID covering this path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@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: a789762c07
ℹ️ 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".
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed PR #127 — feat(ps): add --proc-path parameter to CLI and API.
The overall design is solid: ProcProvider cleanly abstracts the fixed proc path, the public API (ProcPath option) is well-documented, platform stubs ignore the arg correctly, and subshell inheritance works automatically since procPath is in runnerConfig (value-copied). Test coverage for the new API is good.
Assessment: needs fixes — one P1 CI break, one P3 nit.
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
builtins/internal/procinfo/procinfo_linux.go:109 |
os.Stat not in allowed-symbols allowlist — CI failing |
| 2 | |
interp/runner_exec.go:313 |
NewProcProvider allocated on every builtin call |
Test coverage
| Code path | Test | Status |
|---|---|---|
getByPIDs valid path |
TestProcPathFakeProcByPID |
Covered |
getByPIDs nonexistent path |
TestProcPathNonexistentDirErrorsByPID |
Covered |
listAll valid path |
TestProcPathFakeProc |
Covered |
listAll nonexistent path |
TestProcPathNonexistentDirErrors |
Covered |
getSession valid path |
TestProcPathFakeProcSession |
Covered |
ProcPath("") defaults to /proc |
TestProcPathEmptyUsesDefault |
Covered |
CLI --proc-path flag |
TestProcPathFlagNonexistentDir |
Covered |
--proc-path in --help output |
TestProcPathFlagInHelp |
Covered |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aff70bb0fe
ℹ️ 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 review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e73d031cc
ℹ️ 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".
Self-review (iteration 1)Result: COMMENT
|
Codex review (iteration 1)Findings: 1×P1, 3×P2 P1 — P2 — P2 — P2 — |
…use testify in ps test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…x review comments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… in review-fix-loop Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Code Review Summary
Reviewed: feat(ps): add --proc-path parameter to CLI and API
Overall assessment: safe to merge. This PR introduces a --proc-path CLI flag and ProcPath RunnerOption to override the proc filesystem root used by the ps builtin. The design is clean and the implementation is correct.
Security
- ProcPath is operator-controlled only. The path is fixed at
New()time and stored inrunnerConfig— scripts cannot influence it. This is explicitly documented inREADME.md. - No path injection via PID components. All sub-paths under
procPathare constructed withfilepath.Join(procPath, strconv.Itoa(pid), filename)wherepidis always a validated positive integer (fromos.ReadDirdirectory names orparsePIDs). No injection vector. - Sandbox bypass is intentional and documented.
procinfodirectly usesos.ReadFile/os.ReadDir(bypassingAllowedPaths), which is appropriate for a virtual read-only filesystem. This is already permitted by theallowedsymbolsallowlist and clearly documented inREADME.md. getByPIDsStat guard is correctly placed. Without the upfrontos.Statcheck, a nonexistentprocPathwould silently produce empty results (becausereadProc'sErrNotExistreturn is silently skipped as "process no longer exists"). The guard prevents this silent failure.
Correctness
- The
ProcProviderwrapper correctly fixes the path at construction time, preventing any override by script-level callers. NewProcProviderandresolveProcPathboth normalize empty string →DefaultProcPath. This double-normalization is redundant but not incorrect.- Subshells inherit
procviarunnerConfigvalue copy — correct, sinceProcProvideris immutable after construction.
Test Coverage
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
listAll with nonexistent procPath (ps -e) |
— | TestProcPathNonexistentDirErrors |
Covered |
getByPIDs with nonexistent procPath (ps -p) |
— | TestProcPathNonexistentDirErrorsByPID |
Covered |
listAll with file (not dir) as procPath |
— | TestProcPathNotADirErrors_ListAll |
Covered |
getByPIDs with file (not dir) as procPath |
— | TestProcPathNotADirErrors_ByPID |
Covered |
Empty ProcPath falls back to /proc |
— | TestProcPathEmptyUsesDefault |
Covered |
listAll with fake proc tree (ps -e) |
— | TestProcPathFakeProc |
Covered |
getByPIDs with fake proc tree (ps -p) |
— | TestProcPathFakeProcByPID |
Covered |
getSession with fake proc tree (bare ps) |
— | TestProcPathFakeProcSession |
Covered |
getSession with nonexistent procPath (bare ps) |
— | — | Missing (P3) |
CLI --proc-path flag nonexistent dir |
— | TestProcPathFlagNonexistentDir |
Covered |
CLI --proc-path flag with fake proc tree |
— | TestProcPathFlagFakeProc |
Covered |
--proc-path appears in --help output |
— | TestProcPathFlagInHelp |
Covered |
Positive Observations
- Excellent use of the
ProcProviderwrapper to enforce immutability — the path cannot be changed afterNew(), and scripts have zero influence over it. - The
allowedsymbolsallowlist additions (os.Stat,path/filepath.Join,strconv.Itoa) are minimal and correctly justified. - Consistent use of
filepath.Jointhroughoutprocinfo_linux.goreplaces error-pronefmt.Sprintfwith embedded/separators. - Documentation is updated in both
README.mdandSHELL_FEATURES.md.
Findings
| # | Priority | File | Finding |
|---|---|---|---|
| 1 | |
interp/api.go:493 |
ProcPath godoc missing Linux-only effectiveness note |
| 2 | |
builtins/internal/procinfo/procinfo_linux.go:131 |
Error message uses %s/%d string format instead of filepath.Join |
| 3 | |
builtins/ps/ps_procpath_linux_test.go |
Missing test: bare ps (GetSession) with nonexistent procPath |
Self-review (iteration 2)Findings: 3×P3 P3 — interp/api.go:493: ProcPath godoc missing note that it is Linux-only |
Codex review (iteration 2)Findings: 2×P1, 1×P2 P1 — cmd/rshell/main.go:112, procinfo_linux.go:27,140,242,261: P1 — review-fix-loop/SKILL.md:102,286,314: self-review posts GitHub threads but loop has no mechanism to resolve them; Step 3 requires zero unresolved threads → loop can't converge. Will fix: remove the unresolved-threads gate from Step 3. P2 — code-review/SKILL.md:29, review-fix-loop/SKILL.md:110: SPECS section removed. Intentional: removed as prompt injection security fix; not restoring. |
…te review-fix-loop to use RULES.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gate from review-fix-loop Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2fc858ac3
ℹ️ 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".
…eview Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
Self-review (iteration 1/1)Findings: 1×P2 P2 — builtins/ps/ps_procpath_linux_test.go: |
Codex review (iteration 1/1)Findings: 1×P1 (disputed), 1×P2 P1 (disputed) — cmd/rshell/main.go:112, interp/api.go:492: Codex flags that P2 — builtins/internal/procinfo/procinfo_linux.go:70,85: bare |
…ID-namespace caveat - Add line-count assertion to TestProcPathFakeProcSession to verify the real /proc is not read (only the header line should appear). - Add comment to getSession on Linux explaining that bare ps returns empty results when procPath is from a foreign PID namespace. - Extend ProcPath godoc to document the same PID-namespace caveat. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review-Fix Loop Summary
Iteration log
Final state
Fixes applied (iter 1)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95acdeec6c
ℹ️ 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".
| @@ -1,336 +0,0 @@ | |||
| --- | |||
There was a problem hiding this comment.
Note: Removing skills/instructions related to using PR comment. This is a public repo and it's likely not very safe to rely on PR comments since it could be a prompt injection attack vector.
3 participants
Summary
--proc-pathCLI flag (default:/proc) that configures the proc filesystem path used by thepsbuiltinProcPath(path string) RunnerOptionto the interpreter API for programmatic useProcPaththroughrunnerConfig→CallContext→procinfofunctions on all platformsprocinfo_linux.goto use proper multi-segmentfilepath.Joininstead offmt.Sprintfwith embedded separatorsTest plan
go test ./...passespsworks as before when--proc-pathis not set--proc-path /custom/procis accepted by the CLI without error🤖 Generated with Claude Code