feat(find): add --help, -type b/c, -perm, -quit; fix access TOCTOU#104
feat(find): add --help, -type b/c, -perm, -quit; fix access TOCTOU#104
Conversation
matt-dz
changed the title
matt-dz
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
Self-Review — Iteration 1
Overall assessment: needs minor fixes (no P0, 1×P1, 3×P2, 1×P3)
This is a well-structured PR that adds 21 GNU find predicates with proper platform-specific stat helpers, sandbox-safe imports, and good test coverage. The core architecture (expr/eval/match separation, iterative stack-based walk) is maintained cleanly. All new predicates go through the sandbox's CallContext interface — no direct filesystem access.
Summary Table
| # | Priority | File | Finding |
|---|---|---|---|
| 1 | P1 | eval_predicates.go:152-179 |
-samefile re-stats reference file on every entry (O(N) stat amplification) |
| 2 | P2 | eval_predicates.go:88-98 |
-user/-group error for unknown names printed on every file visit |
| 3 | P2 | eval_predicates.go:228-232 |
-ls uses 1K blocks; GNU find uses 512-byte blocks |
| 4 | P2 | eval_predicates.go |
-printf missing commonly used directives %P, %H, %k |
| 5 | P3 | expr.go:640 |
goto doneWho in parseSymbolicMode is non-idiomatic Go |
Positive Observations
- All filesystem access goes through
CallContext(sandbox-safe) -readable/-writable/-executablecorrectly useAccessFile— no rawos.Access-permparsing handles octal and symbolic modes with proper validation-quitpropagation through AND/OR/NOT is correct-depthpost-order traversal handles starting dir correctly viaisStartDirflag- Platform-specific files use proper build tags
- Overflow protection constants follow the
maxMtimeN/maxMminNpattern evalTimeInDays/evalTimeInMinutescorrectly extract shared logic
|
@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 — 5 findings (1×P1, 3×P2, 1×P3). Key issues: -samefile O(N) stat amplification (P1), -user/-group error spam (P2), -ls block size mismatch (P2), -printf missing directives (P2). |
![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: 516aed27f2
ℹ️ 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 this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
matt-dz
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
Self-Review — Iteration 2
APPROVE (no findings) — all 5 findings from iteration 1 addressed. Caches are per-walk (no cross-invocation leaks). Windows tests fixed with stdout_windows.
|
Iteration 2 self-review result: APPROVE (no findings) — all iteration 1 findings addressed. No new issues introduced by the fixes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5531bf1376
ℹ️ 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
Fixes applied across all iterations
Deferred to follow-up
🤖 Generated with Claude Code |
|
@codex conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe7df3ff90
ℹ️ 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".
matt-dz
changed the title
|
@codex conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87f4b726dc
ℹ️ 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 conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26c0bc85b8
ℹ️ 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 conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d84c9773d
ℹ️ 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 conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6c223d8b9
ℹ️ 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 conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80e3e4b007
ℹ️ 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 conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5ea737dd8
ℹ️ 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 --help to the find builtin (required by RULES.md for every command) and extend -type to support block (b) and character (c) device types for GNU find parity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cover fileTypeChar, matchType, and parseTypePredicate with unit tests, plus a scenario test validating parser acceptance of -type b, c, and b,c. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add permission (-readable, -writable, -executable, -perm), time (-atime, -amin, -ctime, -cmin), ownership (-user, -group, -uid, -gid, -nouser, -nogroup), link (-links, -inum, -samefile), action (-ls, -printf, -quit), and global option (-daystart, -depth, -mount/-xdev) predicates. All are read-only and safe for the AI agent sandbox. Includes platform-specific stat helpers (Linux/Darwin/Windows), unit tests for parser and matchPerm, and 14 scenario tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add symbols required by the new find predicates to both the global ceiling and find per-command allowlist: io/fs.FileMode, os/user lookups, syscall.Stat_t, syscall.Win32FileAttributeData, time.Date, time.Unix, strconv.FormatUint, and others. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
No YAML scenarios reference skip_if_root after removing the root-sensitive readable and test_write_denied scenarios. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfbe097394
ℹ️ 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".
Duplicate of test_x_executable (which also covers missing files). Its stdout_windows expectation was stale after the Windows execute-deny fix in 1ac97c4. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TestAccessExecAlwaysSucceedsWindows asserted NoError but the implementation (1ac97c4) returns ErrPermission for execute checks. Rename to TestAccessExecAlwaysDeniedWindows and assert ErrorIs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ 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". |
matt-dz
changed the title
matt-dz
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
Effective Go Review — PR #104
[Claude Opus 4.6] Reviewed 13 changed Go files (1688 added lines) against Effective Go and 100 Go Mistakes.
Overall assessment: ✅ All clear — no P0/P1 findings.
Positive observations
- Excellent TOCTOU prevention in
accessCheck— open-first withO_NONBLOCK, fstat for metadata from the same inode - Proper
errors.Is()usage throughout - Well-structured guard clauses (e.g.,
if !checkReadearly return) - Comprehensive doc comments on complex functions
- Clean separation of root/non-root paths in
effectiveHasPerm - Correct quit-before-implicit-print evaluation order matching GNU find
Minor observations (P3, not blocking)
| # | Priority | File | Finding | Rule |
|---|---|---|---|---|
| 1 |  |
portable_unix.go:58 |
f.Close() discards error |
#54 Defer errors |
| 2 | |
portable_windows.go:76 |
f.Close() discards error |
#54 Defer errors |
Both are on read-only file descriptors opened for permission probing — close errors are benign (no unflushed data). Acceptable trade-off for the access-check hot path.
🤖 Generated with Claude Code
Read-only fds rarely fail on close, but Effective Go (#54) requires checking. Capture and return close errors instead of discarding them. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0402635107
ℹ️ 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".
…ype-bc # Conflicts: # LICENSE-3rdparty.csv # allowedsymbols/symbols_builtins.go # go.mod
accessCheck discarded checkWrite, so Access(..., 0x02) always succeeded — even for read-only files. On Windows, FILE_ATTRIBUTE_READONLY clears write bits in Mode().Perm(), making mode-bit inspection reliable. Capture checkWrite and deny when the owner write bit is clear. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review |
The merge from main replaced our symbols_builtins.go, dropping find-specific symbols (io/fs.FileMode, ModeCharDevice, ModeDevice, ModeSetuid, ModeSetgid, ModeSticky, strconv.ParseUint, strings.Split). Re-add them to both per-command and global lists. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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". |
matt-dz
requested review from
AlexandreYang,
astuyve,
julesmcrt and
thieman
as code owners
2 participants
Summary
--helpflag triggers usage output, including when it appears after path operandsu=rwx,g=rx, copy-bitsg=u, conditional executeX, setuid/setgid/stickys/t, all three operators=/+/-)-print(matches GNU find)-perminsteadO_NONBLOCKfor single-inode permission checks; eliminates Stat→OpenFile double resolutiongolang.org/x/systo LICENSE-3rdparty.csvtest_exec_check,test_write_denied,readable_*)Test plan
go test ./... -count=1 -timeout 120s— full suite passesRSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s— GNU find parity verified for-quitimplicit-print behaviorGOOS=windows go build ./...— cross-compiles cleanlygo test ./allowedsymbols/ -run TestAllowedSymbols -count=1— symbol allowlist passesRSHELL_COMPLIANCE_TEST=1 go test ./tests/ -run TestCompliance -v— compliance passesgofmt -l .— clean🤖 Generated with Claude Code