feat(pwd): add pwd builtin with -L/-P symlink resolution#208
feat(pwd): add pwd builtin with -L/-P symlink resolution#208
Conversation
Adds the POSIX pwd builtin to print the absolute pathname of the current working directory. Supports -L (logical, default) and -P (physical), last-wins semantics when both are given, and --help. Symlink resolution for -P is sandbox-safe: walks the path through callCtx.LstatFile and ReadlinkFile, caps expansions at the Linux ELOOP limit (40 hops), and treats components above AllowedPaths as opaque pass-through so -P stays useful when the cwd lives at the sandbox root. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
On Windows, paths constructed from `string(filepath.Separator)` alone yield `\foo`, which filepath.IsAbs rejects — Windows requires a drive letter. Introduce an `absRoot` helper that returns "/" on Unix and `C:\` on Windows, and use it everywhere the internal tests need an absolute root. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A relative symlink target like "./real" uses forward slashes, but the walking loop in resolveSymlinks splits on filepath.Separator — `\` on Windows. Without normalization the target is treated as one opaque component and the resolved path ended up containing a literal "./" segment (e.g. `C:\./real` instead of `C:\real`). filepath.Clean collapses "." segments and converts "/" to "\" on Windows, so the splice produces a correctly-tokenized rest for the resolver to walk. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Windows pwd output uses backslash separators, so the existing `stdout_contains: ["/"]` assertion fails on the windows-latest runner. Add `stdout_contains_windows: ["\\\\"]` to every pwd scenario that asserts a substring of the printed path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
![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: 9c93caf2f2
ℹ️ 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".
Two bugs surfaced by the Codex review: 1. pflag.FlagSet.Visit walks set flags in lexicographical (or declaration) order, never command-line order. The previous pickPhysical helper therefore picked the wrong mode whenever `-P -L` appeared in that order — for a symlinked cwd, `pwd -P -L` printed the physical path even though POSIX (and bash) say -L wins. 2. pflag.BoolP accepts explicit values like `--physical=false`. The handler used to treat any visited mode flag as true, so `pwd --physical=false` silently selected physical mode. GNU `/bin/pwd --physical=false` rejects the argument; we should too. Switch -L/-P to a shared boolSeqFlag (matching the head builtin's boolSeqFlag pattern). The flag's Set increments a shared counter and records the new value in pos, so we can compare pos fields after parsing to determine the actual command-line order. Set also rejects any value other than the NoOptDefVal "true", which makes `--physical=false` and friends fail with "option doesn't allow an argument". Add regression tests with a real symlinked cwd (the previous tests used a non-symlink temp dir, so the wrong mode was hidden), and internal tests asserting that explicit values are rejected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ba4c37883
ℹ️ 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".
Address two more findings from the Codex review: 1. P2: Container-style sandboxes mount the host filesystem at a prefix (e.g. /mnt/host) and on-disk symlinks often store host-absolute targets (e.g. /var/log/pods/app) without that prefix. The previous pwd -P resolver took the readlink string at face value and tried to walk it relative to root, so for a cwd reached via such a symlink the printed path was unreachable through the sandbox — `cat $(pwd)` would fail even though the file is allowed. Expose the runner's host prefix on CallContext and apply it to absolute symlink targets that don't already start with it. The resolved path now matches what the sandbox itself accepts. 2. P3: pwd --physical=true was silently accepted because pflag passed the literal "true" both for the bare flag (via NoOptDefVal="true") and for the explicit `=true` form, so boolSeqFlag.Set could not distinguish them. Switch NoOptDefVal to a private sentinel string so any explicit value — including "true" — fails Set's equality check and is rejected as "option doesn't allow an argument", matching GNU /bin/pwd. Tests: add unix tests for the HostPrefix translation (with and without double-prefix avoidance) and internal tests asserting that --logical=true / --physical=true are now rejected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3cd881015
ℹ️ 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 resolveSymlinks can return context.Canceled or context.DeadlineExceeded when the run is interrupted mid-walk, but the handler treated every non-loop error as a sandbox-miss and fell back to printing the logical path with exit 0. RULES.md requires graceful handling of cancellation, and reporting success with a stale path is misleading. Add a context-cancel check between the loop-error branch and the silent fallback: if ctx.Err() is set, exit 1 without writing anything. The sandbox-miss best-effort case (resolver returned a non-loop, non-cancel error) keeps its logical-path fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 775b92ee5b
ℹ️ 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 hp := callCtx.HostPrefix(); hp != "" && !strings.HasPrefix(cleanedTarget, hp+string(filepath.Separator)) && cleanedTarget != hp { | ||
| cleanedTarget = filepath.Join(hp, cleanedTarget) |
There was a problem hiding this comment.
When callers configure HostPrefix with a trailing separator (for example HostPrefix("/mnt/host/")), this code compares the raw prefix against a cleaned symlink target. Absolute symlink targets that already include the cleaned prefix then fail the HasPrefix check and get prefixed again, so pwd -P can print paths like /mnt/host/mnt/host/real even though the sandbox itself normalizes the prefix. Cleaning hp before the comparison/join keeps pwd -P consistent with AllowedPaths resolution.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 775b92ee5b
ℹ️ 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 hp := callCtx.HostPrefix(); hp != "" && !strings.HasPrefix(cleanedTarget, hp+string(filepath.Separator)) && cleanedTarget != hp { | ||
| cleanedTarget = filepath.Join(hp, cleanedTarget) |
There was a problem hiding this comment.
When HostPrefix is configured with a trailing slash or ./.. components, the sandbox normalizes it via SetHostPrefix, but pwd receives the raw value and uses it in this prefix check. In a container setup where the symlink target already includes the cleaned prefix, for example HostPrefix("/mnt/host/") and target /mnt/host/var, hp + separator becomes /mnt/host//, so the check misses and filepath.Join produces /mnt/host/mnt/host/var; pwd -P then prints a non-existent double-prefixed path. Clean hp, or return the sandbox's normalized prefix, before comparing/joining.
Useful? React with 👍 / 👎.
Codex flagged that interp.HostPrefix("/mnt/host/") (trailing slash)
breaks pwd -P: the sandbox normalizes the prefix via filepath.Clean
in SetHostPrefix, but my CallContext callback returned the runner's
raw r.hostPrefix. The unnormalized value defeats the HasPrefix
check ("/mnt/host//" never matches a clean target) and filepath.Join
then doubles the prefix, producing paths like
"/mnt/host/mnt/host/var".
Switch both CallContext.HostPrefix sites to call r.sandbox.HostPrefix(),
which already returns the filepath.Clean'd value. The raw runner
field stays as-is (still consumed by SetHostPrefix during init).
Add a regression test that passes HostPrefix with a trailing slash
and verifies the resolved path is not double-prefixed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 869e4caf85
ℹ️ 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".
| } | ||
|
|
||
| candidate := joinPath(out, comp) | ||
| info, err := callCtx.LstatFile(ctx, candidate) |
There was a problem hiding this comment.
When the working directory is itself under an AllowedPaths root that is a symlink (for example launching from /tmp/link and passing --allowed-paths /tmp/link), pwd -P still prints the logical /tmp/link path. The sandbox opens roots through os.OpenRoot, so this LstatFile call sees the opened root's target directory rather than the symlink path and the branch below treats it as non-symlink; this makes the documented physical mode fail in the common -p "$PWD" configuration after entering a directory via a symlink.
Useful? React with 👍 / 👎.
Codex flagged that when an AllowedPaths root is itself a symlink (e.g.
--allowed-paths /tmp/link with /tmp/link -> /tmp/real), `pwd -P` still
prints the logical /tmp/link path. os.Root follows the root symlink
at OpenRoot time, so the per-component LstatFile walk in
resolveSymlinks observes only the target dir and treats the root as
a non-symlink — defeating physical resolution in the common
"pwd -P after entering via a symlink" case.
Add a sandbox-level helper that records each root's canonical
(filepath.EvalSymlinks-resolved) path at New() time and exposes it
via Sandbox.CanonicalizeRootPrefix. Plumb a CanonicalizeRootPrefix
callback through CallContext, and apply it in pwd's -P branch after
the per-component walk completes. Roots that aren't symlinks are
no-ops; the helper only translates configured-form prefixes that
differ from their canonical form.
Test coverage:
- TestPwdPhysicalSymlinkedSandboxRoot: regression for the
/tmp/link → /tmp/real case Codex described.
- TestPwdPhysicalNonSymlinkedSandboxRootUnchanged: control case.
- canonicalTempDir helper used everywhere -P tests previously
passed because t.TempDir() returns canonical form on Linux but
a /var/folders symlink on macOS.
Allowlist updates: filepath.EvalSymlinks added to allowedpaths
allowlist (the sandbox is allowed to inspect the host filesystem
to record canonical roots).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a786636223
ℹ️ 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".
| pwd --no-such-flag | ||
| expect: | ||
| stdout: "" | ||
| stderr_contains: ["pwd:", "unknown flag"] |
There was a problem hiding this comment.
The repo instructions in AGENTS.md say scenario tests should use expect.stderr when possible; this new flag-error scenario uses stderr_contains even though the diagnostic is deterministic (pwd: unknown flag: --no-such-flag). Please switch this (and the analogous pwd -x / pwd --version cases) to an exact stderr: |+ block so regressions in the emitted message are caught.
Useful? React with 👍 / 👎.
Codex flagged that AGENTS.md mandates expect.stderr over stderr_contains when the diagnostic is deterministic. The three pwd flag-error scenarios (unknown long flag, unknown short flag, --version) emit fixed pflag-formatted messages, so a regression in the wording would slip past the substring assertion. Switch them to exact stderr blocks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Accidentally committed a runtime lock file from the local loop scheduler in 937cc1b; add it to .gitignore and remove from the index so it doesn't recur. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
julesmcrt
requested review from
astuyve,
matt-dz,
thieman and
val06
as code owners
2 participants
Summary
pwdbuiltin with-L(logical, default),-P(physical, resolves symlinks), and--helpflags. Last-wins semantics when-L/-Pboth appear (matches POSIX/bash).-Pwalks the absolute path throughcallCtx.LstatFile/ReadlinkFileso it never bypassesAllowedPaths. Caps expansions at the LinuxELOOPlimit (40 hops). Cycles error with exit 1; sandbox-blocked components pass through opaquely so-Pstays useful when the cwd is the sandbox root..github/workflows/fuzz.yml. 94% line coverage onpwd.go.Test plan
go test ./builtins/pwd/...— pass, 94% coveragego test ./...— all packages passgo test -run Fuzz -count=1 ./builtins/pwd/— seed corpus passesgo test -fuzz=FuzzPwdArgs -fuzztime=20s— local fuzz run, no findings beyond the filtered upstream parser quirksgo run ./cmd/rshell --allow-all-commands -c 'help pwd'— help output appears correctlygo run ./cmd/rshell --allow-all-commands --allowed-paths /tmp -c 'pwd; pwd -L; pwd -P'— manual smoke test through a/tmpsymlink confirms-Presolves and-Lpreserves🤖 Generated with Claude Code