Repository navigation
Conversation
|
|
![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: b41cf031d8
ℹ️ 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".
| content, err := fs.ReadFile(rootFS, path) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("reading %s: %w", path, err) |
There was a problem hiding this comment.
When the target contains a symlink to a directory, fs.WalkDir reports the symlink itself as a non-directory entry, so both walkers pass it here; fs.ReadFile then follows the link and returns an EISDIR error. Propagating that error makes both baseline create and baseline update fail on otherwise valid repositories containing directory symlinks, whereas these entries were previously skipped after the failed read. Detect and skip directory symlinks or other non-file entries before treating read failures as fatal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 40f9645.
fs.WalkDir exposed the directory symlink as a non-directory entry, and the new focused test failed on both create and update with reading target-link: ... is a directory before the fix.
Both walkers now skip entries with known non-regular mode bits (d.Type()&fs.ModeType != 0) before calling fs.ReadFile. Regular files still reach scanBaselineFile, so genuine read failures and scanner failures remain fail-closed. TestBaselineWalksSkipDirectorySymlinks creates an actual directory symlink and covers both baseline paths while asserting the scanner is not called.
Verification:
- focused baseline regressions: pass
go test ./internal/cli/cmd -count=1: pass- focused
-race: pass go vet ./internal/cli/cmd: passgo build: pass- bounded
go test ./... -count=1: all packages pass except the same two date-dependentinternal/ignorefailures previously reproduced unchanged at base commit 65f6eb9.
|
@codex address that feedback Please handle the directory-symlink/non-file false-positive identified in the review without weakening the original fail-closed repair. Preserve the intended distinction: routine non-file entries may be skipped when structurally expected, but genuine read/scan failures on files must remain loud. Add focused regression coverage for the symlink/non-file case, rerun the prior verification set, and keep unrelated behavior unchanged. |
|
To use Codex here, create an environment for this repo. |
Summary
Previously, either failure was returned as
nilfrom the walk callback, so the command could persist a baseline built from an unknown subset of files and still report success.Fixes #309.
Verification
go test ./internal/cli/cmd -count=1go test -race ./internal/cli/cmd -run 'TestBaselineWalksReturnScanErrors|TestScanBaselineFileReturnsReadErrors' -count=1go vet ./internal/cli/cmdgo build -o /private/tmp/v36-deputy-bin .go test ./... -count=1: all packages passed exceptinternal/ignore; its two date-dependent failures reproduce unchanged at base commit65f6eb9Implementation and regression tests were prepared with Codex and reviewed against the final diff.