refactor(agent): centralize rooted host filesystem operations - #406
refactor(agent): centralize rooted host filesystem operations#406rice-riley wants to merge 1 commit into
Conversation
github-actions
Bot
added
component/operator
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds rooted, symlink-safe Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
fe8a945 to
629de6c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent/go/internal/flags/flags.go`:
- Line 65: Integrate flags.Store into the agent entrypoint lifecycle: create the
store with flags.NewStore, record completion flags around execution, support
explicit uninstall by removing the relevant step flags, and ensure finalizer
cleanup removes any remaining flags. Replace the placeholder Hello, World! flow
while preserving the existing step lifecycle and using Store/Remove
consistently.
In `@agent/go/internal/flags/layout.go`:
- Around line 155-162: The CreateLogFile collision loop must avoid reusing lower
suffixes: in agent/go/internal/flags/layout.go lines 155-162, determine an index
above the highest existing collision index for the timestamp while preserving
exclusive creation retries for concurrent writers. In
agent/go/internal/flags/logs_test.go lines 76-97, add coverage that creates
another same-timestamp file after retention cleanup, runs cleanup again, and
verifies the newly created path remains.
In `@agent/go/internal/hostfs/copy.go`:
- Around line 51-89: Update CopyTree to provide tree-level atomicity by copying
into a staging directory under the destination’s parent, then renaming the
completed staging tree into place only after filepath.WalkDir succeeds; remove
the staging directory on failure and preserve the existing rooted file
operations and error context. If the surrounding API intentionally permits
partial application instead, add a CopyTree doc comment explicitly documenting
that failure can leave a partial destination for callers to handle.
- Around line 32-47: Update CopyTreeIfExists to use os.Lstat when inspecting
source, reject a source-root symlink consistently with CopyTree’s nested-entry
behavior, and preserve the existing missing-source and validation errors.
Simplify the function tail by returning CopyTree(rootMount, source, destination)
directly instead of wrapping it in an if block.
- Around line 101-104: Remove the early os.Stat/os.SameFile check in the
surrounding copy flow so every destination reaches copyRegularFile, which
already uses Lstat and rejects symlinks correctly. Add a copy_test.go case where
the destination symlink points directly to the source, asserting the symlink is
rejected rather than treated as an already-copied file.
In `@agent/go/internal/hostfs/hostfs_test.go`:
- Around line 110-121: Extend the RenameFile specs in the existing test block to
cover replacing an existing regular destination: create newPath with different
contents, rename oldPath onto it, and assert the destination contains the source
contents. Add a separate case with newPath created as a directory and assert
RenameFile rejects the non-regular destination.
In `@agent/go/internal/hostfs/hostfs.go`:
- Around line 327-349: Directory creation does not preserve requested
permissions when the process umask is restrictive. In
agent/go/internal/hostfs/hostfs.go:327-349, update ensureDirectories to call
root.Chmod(current, mode) after a successful root.Mkdir and before the second
Lstat, while leaving already-existing directories unchanged; in
agent/go/internal/hostfs/copy.go:107-118, remove the duplicated
creating-directory error wrapping; in
agent/go/internal/hostfs/copy_test.go:31-47, add a nested directory with mode
0o701, apply umask 0o077, and assert CopyTree preserves that mode.
- Around line 194-203: Replace path-based permission updates with
descriptor-based chmod in the current file-writing flow: use file.Chmod(mode)
instead of root.Chmod(relative, mode), while preserving the existing cleanup and
joined-error behavior as appropriate. Apply the same change in writeRootedFile,
using its open file descriptor and simplifying its failure path consistently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 752c393c-320a-4f6f-a66a-74651a78d982
📒 Files selected for processing (12)
agent/go/internal/flags/flags.goagent/go/internal/flags/flags_test.goagent/go/internal/flags/layout.goagent/go/internal/flags/layout_test.goagent/go/internal/flags/logs.goagent/go/internal/flags/logs_test.goagent/go/internal/history/history.goagent/go/internal/history/history_test.goagent/go/internal/hostfs/copy.goagent/go/internal/hostfs/copy_test.goagent/go/internal/hostfs/hostfs.goagent/go/internal/hostfs/hostfs_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent/go/internal/flags/logs.go`:
- Around line 119-121: Update the suffix validation around strconv.Atoi in the
log filename parsing flow to require the portion after the leading hyphen to
contain only ASCII digits, rejecting values such as -+1 before classification.
Preserve the existing checks for the leading hyphen and positive index so
CleanupOldLogs ignores malformed filenames.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 64713de7-920a-45d0-8389-f67e183a2337
📒 Files selected for processing (12)
agent/go/internal/flags/flags.goagent/go/internal/flags/flags_test.goagent/go/internal/flags/layout.goagent/go/internal/flags/layout_test.goagent/go/internal/flags/logs.goagent/go/internal/flags/logs_test.goagent/go/internal/history/history.goagent/go/internal/history/history_test.goagent/go/internal/hostfs/copy.goagent/go/internal/hostfs/copy_test.goagent/go/internal/hostfs/hostfs.goagent/go/internal/hostfs/hostfs_test.go
629de6c to
2736b44
Compare
2736b44 to
46c2bc3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
agent/go/internal/hostfs/copy.go (2)
32-47: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject a symlinked source root, and simplify the tail.
os.Stat(source)follows symbolic links, so a symlinkedsourceis accepted and reports the target directory.CopyTreerejects every symlink inside the tree at Line 72, andcopyRegularFilerejects a symlinked destination. The source root is the only unchecked position. Useos.Lstatand reject a symlink there. If following a symlinked source root is intentional, state that in the doc comment.Lines 43-46 wrap
CopyTreewithout adding context. Return the call directly.🐛 Proposed fix
func CopyTreeIfExists(rootMount, source, destination string) error { - info, err := os.Stat(source) + info, err := os.Lstat(source) if errors.Is(err, fs.ErrNotExist) { return nil } if err != nil { return fmt.Errorf("stating copy source %q: %w", source, err) } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("copy source %q is a symbolic link", source) + } if !info.IsDir() { return fmt.Errorf("copy source %q is not a directory", source) } - if err := CopyTree(rootMount, source, destination); err != nil { - return err - } - return nil + return CopyTree(rootMount, source, destination) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/copy.go` around lines 32 - 47, Update CopyTreeIfExists to use os.Lstat so the source root itself is not followed, and reject it when info.Mode() identifies a symbolic link before the directory check. Keep the existing missing-source and non-directory errors, and return CopyTree(rootMount, source, destination) directly instead of wrapping the call in an unnecessary conditional.
101-104: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe early same-file return in
CopyFilebypasses the destination symlink rejection, and no test detects it.os.Stat(destination)follows symbolic links, so a destination symlink that points atsourcereports the same file andCopyFilereturns nil beforecopyRegularFileruns itsos.Lstatrejection.
agent/go/internal/hostfs/copy.go#L101-L104: delete theos.Stat/os.SameFileearly return and callcopyRegularFiledirectly, which already compares the same file withos.Lstat.agent/go/internal/hostfs/copy_test.go#L57-L72: add a case where the destination symlink points atsource, and assertCopyFilereports the symbolic link instead of succeeding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/copy.go` around lines 101 - 104, Remove the os.Stat/os.SameFile early return from CopyFile so it always delegates to copyRegularFile, which uses os.Lstat to reject destination symlinks. In agent/go/internal/hostfs/copy_test.go lines 57-72, add coverage for a destination symlink pointing to source and assert CopyFile returns the symbolic-link error instead of succeeding.agent/go/internal/hostfs/hostfs.go (1)
327-349: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDirectory modes are reduced by the process umask, and no test detects it.
ensureDirectoriesnever applies the requestedmodeafterroot.Mkdir, so every rooted directory getsmode &^ umask.CopyTreeforwards source directory permissions throughmakeDirectory, so it does not preserve directory modes either.
agent/go/internal/hostfs/hostfs.go#L327-L349: callroot.Chmod(current, mode)after a successfulroot.Mkdir(current, mode), before the secondLstat. Leave an already-existing directory unchanged and document that.agent/go/internal/hostfs/copy_test.go#L31-L47: set mode0o701onfilepath.Join(source, "nested"), set a umask of0o077, and assert the copied directory mode afterCopyTree.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/hostfs.go` around lines 327 - 349, Update ensureDirectories in agent/go/internal/hostfs/hostfs.go:327-349 to call root.Chmod(current, mode) immediately after a newly successful root.Mkdir and before the second Lstat, while leaving already-existing directories unchanged and documenting that behavior. In agent/go/internal/hostfs/copy_test.go:31-47, assign mode 0o701 to source/nested, set umask 0o077, and assert CopyTree preserves the copied directory mode.agent/go/internal/hostfs/hostfs_test.go (1)
110-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the documented replacement behavior of
RenameFile.
RenameFiledocuments that it replaces an existing regular destination, and it rejects a non-regular destination. This spec renames only onto a free path. Add a case that createsnewPathfirst and asserts the contents after the rename. Add a case wherenewPathis a directory and assert the rejection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/hostfs_test.go` around lines 110 - 121, The RenameFile test coverage only exercises renaming to a free destination. Extend the RenameFile specs with a case that pre-creates newPath as a regular file and verifies the rename succeeds with the source contents replacing it, plus a case that creates newPath as a directory and asserts RenameFile rejects it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent/go/internal/hostfs/copy_test.go`:
- Around line 31-47: The CopyTree directory-copy test should also verify source
directory permissions are preserved. In the spec around CopyTree, set
source/nested to a distinctive mode such as 0o701, apply a 0o077 umask, and
assert the destination/nested directory has the expected mode, while retaining
the existing file-content and file-mode assertions.
- Around line 57-72: Extend the “rejects symbolic links at copy destinations”
test around CopyFile to make destination point directly to source, then assert
CopyFile returns the symbolic-link rejection error rather than nil. Preserve the
existing outside-file protection assertion and ensure the case exercises the
early same-file path identified in copyRegularFile handling.
---
Duplicate comments:
In `@agent/go/internal/hostfs/copy.go`:
- Around line 32-47: Update CopyTreeIfExists to use os.Lstat so the source root
itself is not followed, and reject it when info.Mode() identifies a symbolic
link before the directory check. Keep the existing missing-source and
non-directory errors, and return CopyTree(rootMount, source, destination)
directly instead of wrapping the call in an unnecessary conditional.
- Around line 101-104: Remove the os.Stat/os.SameFile early return from CopyFile
so it always delegates to copyRegularFile, which uses os.Lstat to reject
destination symlinks. In agent/go/internal/hostfs/copy_test.go lines 57-72, add
coverage for a destination symlink pointing to source and assert CopyFile
returns the symbolic-link error instead of succeeding.
In `@agent/go/internal/hostfs/hostfs_test.go`:
- Around line 110-121: The RenameFile test coverage only exercises renaming to a
free destination. Extend the RenameFile specs with a case that pre-creates
newPath as a regular file and verifies the rename succeeds with the source
contents replacing it, plus a case that creates newPath as a directory and
asserts RenameFile rejects it.
In `@agent/go/internal/hostfs/hostfs.go`:
- Around line 327-349: Update ensureDirectories in
agent/go/internal/hostfs/hostfs.go:327-349 to call root.Chmod(current, mode)
immediately after a newly successful root.Mkdir and before the second Lstat,
while leaving already-existing directories unchanged and documenting that
behavior. In agent/go/internal/hostfs/copy_test.go:31-47, assign mode 0o701 to
source/nested, set umask 0o077, and assert CopyTree preserves the copied
directory mode.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4f4a9e9c-9b47-44db-a9f0-c52f89d7ecf0
📒 Files selected for processing (12)
agent/go/internal/flags/flags.goagent/go/internal/flags/flags_test.goagent/go/internal/flags/layout.goagent/go/internal/flags/layout_test.goagent/go/internal/flags/logs.goagent/go/internal/flags/logs_test.goagent/go/internal/history/history.goagent/go/internal/history/history_test.goagent/go/internal/hostfs/copy.goagent/go/internal/hostfs/copy_test.goagent/go/internal/hostfs/hostfs.goagent/go/internal/hostfs/hostfs_test.go
46c2bc3 to
bfc89e4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
♻️ Duplicate comments (7)
agent/go/internal/flags/logs.go (1)
119-121: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-digit collision suffixes.
Line 119 accepts
-+1becausestrconv.Atoiaccepts+1. This violates the required-Nfilename shape and can classify a malformed file as a removable log.Verify that every character after the leading
-is an ASCII digit before callingstrconv.Atoi.Proposed fix
- index, err := strconv.Atoi(strings.TrimPrefix(suffix, "-")) - if err != nil || !strings.HasPrefix(suffix, "-") || index < 1 { + if !strings.HasPrefix(suffix, "-") { + return time.Time{}, 0, false + } + digits := suffix[1:] + if digits == "" || strings.Trim(digits, "0123456789") != "" { + return time.Time{}, 0, false + } + index, err := strconv.Atoi(digits) + if err != nil || index < 1 { return time.Time{}, 0, false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/flags/logs.go` around lines 119 - 121, Update the suffix parsing logic around strconv.Atoi to validate that suffix contains a leading hyphen followed only by ASCII digits before conversion; reject values such as -+1 and preserve the existing invalid-result behavior for malformed or non-positive indices.agent/go/internal/flags/layout.go (1)
155-162: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not reuse collision suffixes after retention.
CreateLogFilerestarts at suffix zero, but cleanup treats larger suffixes as newer. A new same-timestamp base file can therefore be deleted while retained-N.logfiles survive.
agent/go/internal/flags/layout.go#L155-L162: start allocation above the highest existing suffix for the timestamp and retain exclusive-create retries.agent/go/internal/flags/layout_test.go#L111-L122: add a test that creates a same-timestamp file after retention and verifies it survives another cleanup.agent/go/internal/flags/logs_test.go#L76-L97: add the same post-retention regression scenario to the cleanup behavior test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/flags/layout.go` around lines 155 - 162, Prevent CreateLogFile from reusing retained collision suffixes by determining the highest existing suffix for the same timestamp, starting allocation above it, and preserving exclusive-create retries in agent/go/internal/flags/layout.go:155-162. Add the post-retention same-timestamp regression scenario and verify the new base file survives cleanup in agent/go/internal/flags/layout_test.go:111-122 and agent/go/internal/flags/logs_test.go:76-97.agent/go/internal/hostfs/hostfs.go (1)
327-349: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDirectory modes are still reduced by the process umask.
ensureDirectoriescallsroot.Mkdir(current, mode)and never appliesmodeafterwards. The created directory ends up withmode &^ umask. File paths in this package avoid the problem becausewriteRootedFilecallsChmodafter creation.makeDirectoryincopy.goforwards source directory permissions here, soCopyTreedoes not preserve directory modes.Call
root.Chmod(current, mode)after a successfulroot.Mkdirand before the secondLstat. Leave an already-existing directory unchanged, and state that in the doc comment.🐛 Proposed fix
if errors.Is(err, fs.ErrNotExist) { if err := root.Mkdir(current, mode); err != nil && !errors.Is(err, fs.ErrExist) { return fmt.Errorf("creating directory %q: %w", current, err) + } else if err == nil { + if err := root.Chmod(current, mode); err != nil { + return fmt.Errorf("setting permissions on directory %q: %w", current, err) + } } info, err = root.Lstat(current) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/hostfs.go` around lines 327 - 349, Update ensureDirectories to call root.Chmod(current, mode) immediately after a successful root.Mkdir and before the follow-up Lstat, while leaving already-existing directories unchanged. Add or update its doc comment to state that existing directories retain their current mode.agent/go/internal/hostfs/copy.go (2)
93-105: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CopyFilestill resolves the source and destination through symlink-following stats.Two problems remain in this function:
- Line 101 calls
os.Stat(destination), which follows symbolic links. Ifdestinationis a symlink that points atsource,os.SameFilereports the same file andCopyFilereturns nil. The symlink rejection incopyRegularFileat Line 135 never runs, and the caller believes the copy succeeded.copyRegularFilealready performs the same-file comparison at Lines 133-144 withos.Lstat, so this early return is redundant. Delete it.- Line 94 calls
os.Stat(source), which also follows symbolic links. A symlinkedsourceis accepted and copied as its target, whileCopyTreerejects every symlink in the source tree at Line 72. Useos.Lstatand reject a symlinked source, or document thatCopyFilefollows a symlinked source on purpose.🐛 Proposed fix
func CopyFile(rootMount, source, destination string) error { - info, err := os.Stat(source) + info, err := os.Lstat(source) if err != nil { return fmt.Errorf("stating copy source %q: %w", source, err) } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("copy source %q is a symbolic link", source) + } if !info.Mode().IsRegular() { return fmt.Errorf("copy source %q is not a regular file", source) } - if destinationInfo, err := os.Stat(destination); err == nil && os.SameFile(info, destinationInfo) { - return nil - } return copyRegularFile(rootMount, source, destination, info.Mode().Perm()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/copy.go` around lines 93 - 105, Update CopyFile to remove the early destination os.Stat/os.SameFile return so copyRegularFile remains responsible for same-file and symlink handling. Change source metadata lookup to os.Lstat and reject symbolic-link sources before validating regular-file status, matching CopyTree’s source-tree behavior.
32-47: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
CopyTreeIfExistskeeps the symlink-following source check and the redundant tail.
os.Stat(source)follows symbolic links, so a symlinked source directory passes theIsDircheck here.CopyTreethen rejects it at Line 72 with an unrelated message, becausefilepath.WalkDirlstats the root. Reject the symlink here so the error names the real cause. Lines 43-46 add nothing to theCopyTreeresult; return the call directly.♻️ Proposed change
- info, err := os.Stat(source) + info, err := os.Lstat(source) if errors.Is(err, fs.ErrNotExist) { return nil } if err != nil { return fmt.Errorf("stating copy source %q: %w", source, err) } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("copy source %q is a symbolic link", source) + } if !info.IsDir() { return fmt.Errorf("copy source %q is not a directory", source) } - if err := CopyTree(rootMount, source, destination); err != nil { - return err - } - return nil + return CopyTree(rootMount, source, destination)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/copy.go` around lines 32 - 47, Update CopyTreeIfExists to use an lstat-style check that does not follow symbolic links, rejecting a symlinked source before the IsDir validation with an error identifying the symlink cause. Simplify the final CopyTree invocation by returning it directly instead of wrapping it in a redundant conditional and tail return.agent/go/internal/hostfs/copy_test.go (2)
57-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover a destination symlink that points at the source.
This symlink points at a different file, so
CopyFilereachescopyRegularFileand the rejection fires. If the destination symlink points atsource, the earlyos.Statcomparison incopy.goat Line 101 treats the two paths as the same file and returns nil. Add that case and assert the rejection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/copy_test.go` around lines 57 - 72, Extend the “rejects symbolic links at copy destinations” test to create a destination symlink targeting source and invoke CopyFile with it, covering the os.Stat comparison in copyRegularFile’s copy flow. Assert that CopyFile returns an error containing “is a symbolic link” rather than succeeding, while preserving the existing outside-target case.
31-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the copied directory mode.
This spec asserts the copied file mode but not the copied directory mode.
CopyTreeforwards the source directory permissions tomakeDirectory, andensureDirectoriesnever applies them, so the directory mode is currently lost to the umask. Set a distinctive mode such as0o701onfilepath.Join(source, "nested"), set a umask of0o077, and assert the destination directory mode. That assertion pins theensureDirectoriesfix requested inagent/go/internal/hostfs/hostfs.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/copy_test.go` around lines 31 - 47, Extend the CopyTree test to set the source nested directory mode to 0o701, apply a 0o077 umask during the copy, and assert the destination nested directory retains mode 0o701. Use the existing CopyTree and ensureDirectories flow so the test verifies directory permissions are applied rather than masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@agent/go/internal/flags/layout.go`:
- Around line 155-162: Prevent CreateLogFile from reusing retained collision
suffixes by determining the highest existing suffix for the same timestamp,
starting allocation above it, and preserving exclusive-create retries in
agent/go/internal/flags/layout.go:155-162. Add the post-retention same-timestamp
regression scenario and verify the new base file survives cleanup in
agent/go/internal/flags/layout_test.go:111-122 and
agent/go/internal/flags/logs_test.go:76-97.
In `@agent/go/internal/flags/logs.go`:
- Around line 119-121: Update the suffix parsing logic around strconv.Atoi to
validate that suffix contains a leading hyphen followed only by ASCII digits
before conversion; reject values such as -+1 and preserve the existing
invalid-result behavior for malformed or non-positive indices.
In `@agent/go/internal/hostfs/copy_test.go`:
- Around line 57-72: Extend the “rejects symbolic links at copy destinations”
test to create a destination symlink targeting source and invoke CopyFile with
it, covering the os.Stat comparison in copyRegularFile’s copy flow. Assert that
CopyFile returns an error containing “is a symbolic link” rather than
succeeding, while preserving the existing outside-target case.
- Around line 31-47: Extend the CopyTree test to set the source nested directory
mode to 0o701, apply a 0o077 umask during the copy, and assert the destination
nested directory retains mode 0o701. Use the existing CopyTree and
ensureDirectories flow so the test verifies directory permissions are applied
rather than masked.
In `@agent/go/internal/hostfs/copy.go`:
- Around line 93-105: Update CopyFile to remove the early destination
os.Stat/os.SameFile return so copyRegularFile remains responsible for same-file
and symlink handling. Change source metadata lookup to os.Lstat and reject
symbolic-link sources before validating regular-file status, matching CopyTree’s
source-tree behavior.
- Around line 32-47: Update CopyTreeIfExists to use an lstat-style check that
does not follow symbolic links, rejecting a symlinked source before the IsDir
validation with an error identifying the symlink cause. Simplify the final
CopyTree invocation by returning it directly instead of wrapping it in a
redundant conditional and tail return.
In `@agent/go/internal/hostfs/hostfs.go`:
- Around line 327-349: Update ensureDirectories to call root.Chmod(current,
mode) immediately after a successful root.Mkdir and before the follow-up Lstat,
while leaving already-existing directories unchanged. Add or update its doc
comment to state that existing directories retain their current mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b0d4305f-50cf-4c0b-9efe-57205f22123b
📒 Files selected for processing (12)
agent/go/internal/flags/flags.goagent/go/internal/flags/flags_test.goagent/go/internal/flags/layout.goagent/go/internal/flags/layout_test.goagent/go/internal/flags/logs.goagent/go/internal/flags/logs_test.goagent/go/internal/history/history.goagent/go/internal/history/history_test.goagent/go/internal/hostfs/copy.goagent/go/internal/hostfs/copy_test.goagent/go/internal/hostfs/hostfs.goagent/go/internal/hostfs/hostfs_test.go
bfc89e4 to
c95d519
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
c95d519 to
41719db
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
agent/go/internal/flags/logs.go (1)
119-121: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-digit collision suffixes.
strconv.Atoiaccepts+1. The parser therefore accepts-+1and cleanup can delete a malformed filename. Require every character after the first-to be an ASCII digit before converting it.Proposed fix
- index, err := strconv.Atoi(strings.TrimPrefix(suffix, "-")) - if err != nil || !strings.HasPrefix(suffix, "-") || index < 1 { + if !strings.HasPrefix(suffix, "-") { + return time.Time{}, 0, false + } + digits := suffix[1:] + if digits == "" || strings.Trim(digits, "0123456789") != "" { + return time.Time{}, 0, false + } + index, err := strconv.Atoi(digits) + if err != nil || index < 1 { return time.Time{}, 0, false }#!/bin/bash set -euo pipefail sed -n '106,124p' agent/go/internal/flags/logs.go rg -n -C 3 --glob '*_test.go' 'not-a-timestamp|-\\+1|parseLogTimestamp' agent/go/internal/flags🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/flags/logs.go` around lines 119 - 121, Update the suffix validation in the log timestamp parser around strconv.Atoi to require the substring after the leading "-" to contain only ASCII digits before conversion; reject values such as "-+1" while preserving the existing checks for the leading hyphen and positive index.agent/go/internal/flags/layout.go (1)
155-162: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep collision indexes monotonic across retention.
CreateLogFilereuses the first free suffix, butCleanupOldLogstreats higher suffixes as newer. A file created after cleanup can receive a lower suffix and be deleted as the oldest file.
agent/go/internal/flags/layout.go#L155-L162: select an index greater than the highest existing suffix for the timestamp while retaining exclusive-create retries.agent/go/internal/flags/layout_test.go#L111-L122: add a creation test after retention removes lower suffixes.agent/go/internal/flags/logs_test.go#L76-L97: verify the recreated file remains after the next cleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/flags/layout.go` around lines 155 - 162, The collision index in CreateLogFile must remain greater than the highest existing suffix for the timestamp, rather than reusing the first free suffix, while preserving exclusive-create retries. Update agent/go/internal/flags/layout.go lines 155-162 accordingly; add the post-retention creation case in agent/go/internal/flags/layout_test.go lines 111-122; and verify in agent/go/internal/flags/logs_test.go lines 76-97 that the recreated file survives the next CleanupOldLogs run.agent/go/internal/hostfs/copy.go (2)
101-104: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winA destination symlink that points at the source bypasses symlink rejection.
CopyFilecallsos.Stat(destination), which follows symbolic links, so such a destination is reported as the same file and the function returns nil beforecopyRegularFilecan reject the link.
agent/go/internal/hostfs/copy.go#L101-L104: delete theos.Stat/os.SameFileearly return, becausecopyRegularFilealready compares files withos.Lstatat Lines 133-144.agent/go/internal/hostfs/copy_test.go#L57-L72: add a case wheredestinationis a symlink that points atsource, and assert the symbolic-link rejection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/copy.go` around lines 101 - 104, The destination symlink bypasses rejection because CopyFile performs an os.Stat/os.SameFile early return; remove that check so copyRegularFile handles destination comparisons via os.Lstat and rejects symlinks. In agent/go/internal/hostfs/copy.go lines 101-104, delete the early return; in agent/go/internal/hostfs/copy_test.go lines 57-72, add coverage for a destination symlink targeting the source and assert symbolic-link rejection.
32-47: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject a symlinked source root for a uniform contract.
os.Stat(source)follows symbolic links, so a symlinkedsourceis accepted and reports the target directory.CopyTreerejects every symlink inside the tree at Line 72, andcopyRegularFilerejects a symlinked destination at Line 135. The source root is the only unchecked position. Useos.Lstatand reject a symlink there. If following a symlinked source root is intentional, state that in the doc comment.Lines 43-46 also wrap
CopyTreewithout adding context. Return the call directly.♻️ Proposed change
- info, err := os.Stat(source) + info, err := os.Lstat(source) if errors.Is(err, fs.ErrNotExist) { return nil } if err != nil { return fmt.Errorf("stating copy source %q: %w", source, err) } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("copy source %q is a symbolic link", source) + } if !info.IsDir() { return fmt.Errorf("copy source %q is not a directory", source) } - if err := CopyTree(rootMount, source, destination); err != nil { - return err - } - return nil + return CopyTree(rootMount, source, destination)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/copy.go` around lines 32 - 47, Update CopyTreeIfExists to use os.Lstat so the source root itself is not followed, and reject it when info.Mode() identifies a symbolic link before the directory check. Return CopyTree(rootMount, source, destination) directly instead of wrapping it in an error-only conditional; preserve the existing missing-source and other validation errors.agent/go/internal/hostfs/hostfs.go (1)
327-349: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequested directory modes are lost, and no test detects it.
ensureDirectoriesnever appliesmodeafterroot.Mkdir, so every created directory getsmode &^ umask.CopyTreeforwards source directory permissions throughmakeDirectory, soCopyTreedoes not preserve directory modes.
agent/go/internal/hostfs/hostfs.go#L327-L349: callroot.Chmod(current, mode)after a successfulroot.Mkdir, only for the directory this call created, and document that an already-existing directory keeps its mode.agent/go/internal/hostfs/copy_test.go#L31-L47: set mode0o701onfilepath.Join(source, "nested"), set a umask of0o077, and assert the mode offilepath.Join(destination, "nested")afterCopyTree.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/hostfs.go` around lines 327 - 349, Update ensureDirectories in agent/go/internal/hostfs/hostfs.go (lines 327-349) to call root.Chmod(current, mode) only after this invocation successfully creates the directory, and document that existing directories retain their current mode. Add coverage in agent/go/internal/hostfs/copy_test.go (lines 31-47) by setting nested to mode 0o701, applying umask 0o077, and asserting the copied destination/nested mode after CopyTree.agent/go/internal/hostfs/copy_test.go (2)
31-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the copied directory mode.
This spec asserts the file mode but not the directory mode.
CopyTreeforwards the source directory permissions tomakeDirectory, andensureDirectoriesnever applies them, so a restrictive umask silently reduces the copied directory mode. Set a distinctive mode such as0o701onfilepath.Join(source, "nested"), set a umask of0o077, and assert the mode offilepath.Join(destination, "nested")afterCopyTree.This is the test gap for the
ensureDirectoriesdefect inagent/go/internal/hostfs/hostfs.go. See the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/copy_test.go` around lines 31 - 47, Extend the “copies directory contents and replaces existing regular files” test to set the source nested directory mode to 0o701, apply a 0o077 umask during the copy, and assert the destination nested directory retains mode 0o701 after CopyTree. Restore the original umask after the assertion and keep the existing file-content and file-mode checks unchanged.
57-72: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCover a destination symlink that points at the source.
This symlink points at a different file, so
CopyFilereachescopyRegularFileand the rejection fires. If the destination symlink points atsource, the early return inagent/go/internal/hostfs/copy.goat Line 101 treats the paths as the same file and returns nil. Add that case and assert the rejection. See the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/go/internal/hostfs/copy_test.go` around lines 57 - 72, Extend the “rejects symbolic links at copy destinations” test to create a destination symlink targeting source, then call CopyFile and assert it returns an error containing “is a symbolic link” rather than succeeding through the same-file early return. Keep the existing outside-target case and content-preservation assertions intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent/go/internal/flags/flags_test.go`:
- Around line 130-137: Add failure-path specs for Remove covering an
out-of-store path rejected by validatePath and a non-regular target rejected by
hostfs.RemoveFile, matching the existing Check and Write test patterns while
preserving the current idempotent removal case.
In `@agent/go/internal/flags/layout_test.go`:
- Around line 111-122: Extend the log-path collision tests around CreateLogFile
to cover retention: create multiple logs with the same timestamp, run retention
to remove lower-suffix files, create another log with that timestamp, then run
retention again and assert the newly created file remains. Reuse the existing
retention configuration and verify the behavior through the resulting file paths
or filesystem state.
In `@agent/go/internal/flags/logs_test.go`:
- Around line 76-97: The collision test should cover index reuse across cleanup
cycles. Extend the existing “keeps the newest files when timestamps collide”
test after the first CleanupOldLogs call by creating another log with the same
timestamp, running cleanup again, and asserting the newly created path still
exists while retention removes only the expected older file.
---
Duplicate comments:
In `@agent/go/internal/flags/layout.go`:
- Around line 155-162: The collision index in CreateLogFile must remain greater
than the highest existing suffix for the timestamp, rather than reusing the
first free suffix, while preserving exclusive-create retries. Update
agent/go/internal/flags/layout.go lines 155-162 accordingly; add the
post-retention creation case in agent/go/internal/flags/layout_test.go lines
111-122; and verify in agent/go/internal/flags/logs_test.go lines 76-97 that the
recreated file survives the next CleanupOldLogs run.
In `@agent/go/internal/flags/logs.go`:
- Around line 119-121: Update the suffix validation in the log timestamp parser
around strconv.Atoi to require the substring after the leading "-" to contain
only ASCII digits before conversion; reject values such as "-+1" while
preserving the existing checks for the leading hyphen and positive index.
In `@agent/go/internal/hostfs/copy_test.go`:
- Around line 31-47: Extend the “copies directory contents and replaces existing
regular files” test to set the source nested directory mode to 0o701, apply a
0o077 umask during the copy, and assert the destination nested directory retains
mode 0o701 after CopyTree. Restore the original umask after the assertion and
keep the existing file-content and file-mode checks unchanged.
- Around line 57-72: Extend the “rejects symbolic links at copy destinations”
test to create a destination symlink targeting source, then call CopyFile and
assert it returns an error containing “is a symbolic link” rather than
succeeding through the same-file early return. Keep the existing outside-target
case and content-preservation assertions intact.
In `@agent/go/internal/hostfs/copy.go`:
- Around line 101-104: The destination symlink bypasses rejection because
CopyFile performs an os.Stat/os.SameFile early return; remove that check so
copyRegularFile handles destination comparisons via os.Lstat and rejects
symlinks. In agent/go/internal/hostfs/copy.go lines 101-104, delete the early
return; in agent/go/internal/hostfs/copy_test.go lines 57-72, add coverage for a
destination symlink targeting the source and assert symbolic-link rejection.
- Around line 32-47: Update CopyTreeIfExists to use os.Lstat so the source root
itself is not followed, and reject it when info.Mode() identifies a symbolic
link before the directory check. Return CopyTree(rootMount, source, destination)
directly instead of wrapping it in an error-only conditional; preserve the
existing missing-source and other validation errors.
In `@agent/go/internal/hostfs/hostfs.go`:
- Around line 327-349: Update ensureDirectories in
agent/go/internal/hostfs/hostfs.go (lines 327-349) to call root.Chmod(current,
mode) only after this invocation successfully creates the directory, and
document that existing directories retain their current mode. Add coverage in
agent/go/internal/hostfs/copy_test.go (lines 31-47) by setting nested to mode
0o701, applying umask 0o077, and asserting the copied destination/nested mode
after CopyTree.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 098ef28b-dcdf-4751-b31e-4e9e80b475a1
📒 Files selected for processing (12)
agent/go/internal/flags/flags.goagent/go/internal/flags/flags_test.goagent/go/internal/flags/layout.goagent/go/internal/flags/layout_test.goagent/go/internal/flags/logs.goagent/go/internal/flags/logs_test.goagent/go/internal/history/history.goagent/go/internal/history/history_test.goagent/go/internal/hostfs/copy.goagent/go/internal/hostfs/copy_test.goagent/go/internal/hostfs/hostfs.goagent/go/internal/hostfs/hostfs_test.go
41719db to
93bf205
Compare
93bf205 to
fa8f210
Compare
Signed-off-by: Riley Rice <rrice@nvidia.com>
fa8f210 to
0a02c4f
Compare
ayuskauskas
left a comment
ayuskauskas
left a comment
There was a problem hiding this comment.
agent/go/internal/history/history.go — Versions.Environment() and Versions.UpgradeArguments() removed without visible call sites
Both exported methods and the CurrentVersionEnv / PreviousVersionEnv constants are removed. The test file removes the assertions that used them, but these are exported symbols — if the main agent startup code passes them as environment variables to step scripts (likely, given the names), that code would fail to compile. The PR description says "Depends on #405" which suggests #405 handles the callers, but that should be confirmed before this merges. If any call site is left dangling, nothing in this diff would surface it.
agent/go/internal/flags/layout.go — unbounded collision loop in CreateLogFile
The for ; ; collision++ loop retries on fs.ErrExist until it finds a free slot. nextLogCollision seeds the starting index from a directory listing so in the happy path it succeeds on the first try or after a small number of races. But if the directory were somehow flooded with every possible collision variant, the loop spins indefinitely. A maxCollisions guard (e.g., 1000) with a meaningful error would prevent a stuck agent.
lockwobr
left a comment
lockwobr
left a comment
There was a problem hiding this comment.
Multi-reviewer cross-review
Three independent reviews of this commit plus a targeted integration-impact pass, cross-reviewed to a 2-of-3 consensus, with every confirmed finding sent to a fresh adversarial verifier that was instructed to refute it.
One of the three reviewer lanes was unavailable, so the bar for confirmation was raised rather than lowered: the two remaining lanes both had to agree.
Outcome: eight candidate findings were raised. Four reached consensus and were then refuted during adversarial verification, so they are not reported here. One survived verification. Four remain contested and need a human call. The integration pass checked 14 change-list items across the operator, chart and Python agent, and found no broken consumers.
No code was executed at any point in this review; every claim is from static reading at the pinned commit.
Contested — raised but not cross-evaluated
agent/go/internal/history/history.go — this commit also deletes exported API unrelated to its stated scope: Versions.Environment(), Versions.UpgradeArguments(), and the CurrentVersionEnv / PreviousVersionEnv constants. They have no non-test callers, so as dead-code hygiene this is defensible. The reason it is worth a second look is that these encode a contract the shipping Python agent still honours — agent/skyhook-agent/src/skyhook_agent/controller.py:434-435 sets CURRENT_VERSION / PREVIOUS_VERSION on step env — so whoever ports the step-execution path will have to re-add them. Either split this out or note it in the PR description.
Open questions
These could not be settled without running code, which this review deliberately does not do:
- Does
make -C agent/go lintcurrently pass?unparamandunusedare enabled inagent/go/.golangci.yml, and several new helpers (makeDirectory,CreateFile,Resolve, andwriteFile'sreplaceparameter) are reachable only from tests. - Do the new
.000000000timestamps and-<n>.logcollision names round-trip cleanly through the Python agent'scleanup_old_logsglob if a node is rolled back from the Go agent to the Python agent? Static reading suggests yes (the glob isprefix-*.log, sorted by mtime), but this wants a real mixed-version run. - Does the Go rewrite intend to restore the
CURRENT_VERSION/PREVIOUS_VERSIONstep-env contract before it executes steps? history.NewStore(rootMount, dir, ...)does not validate at construction thatdirlies beneathrootMount; a mismatched pair now fails later insidehostfswithmust be contained withinon the firstRecordorRead. There is no caller yet, so it is unclear whether the intended wiring always derivesdirfrom aLayout.
Worth recording
The durability regression this refactor was most at risk of introducing did not occur. writeRootedFile preserves the deleted writeAtomic's ordering (chmod, write, fsync file, close, rename, fsync parent directory) and additionally fsyncs on the create path, which the code it replaces never did. legacyLogTimeFormat also exactly matches the shipping Python agent's naming, so log reaping stays compatible across the transition.
| return fmt.Errorf("copy source %q is a symbolic link", path) | ||
| } | ||
| if info.IsDir() { | ||
| if err := makeDirectory(rootMount, target, info.Mode().Perm()); err != nil { |
There was a problem hiding this comment.
CopyTree applies the source root directory's permission bits to every destination ancestor it has to create.
filepath.WalkDir visits the source root first, so that callback has relative == "." and target = destination. This line then passes the source root's Perm() into makeDirectory, which forwards the full relative path (not filepath.Dir) to ensureDirectories. That helper walks every component and Mkdir+Chmods each missing one with that same mode.
Every other ancestor-creating path in this package uses directoryMode (0o755) instead — copyRegularFile on line 151, and WriteFile / CreateFileWriter / RenameFile in hostfs.go. makeDirectory is the sole outlier.
Concretely: copying a tree whose top-level mode is 0o700 into a destination whose ancestors do not yet exist creates those host directories 0o700, blocking traversal; a 0o777 source would widen them. Which mode they end up with depends on which call happened to create them first.
Not currently reachable — nothing outside tests calls CopyTree — so this is latent rather than live.
The test on line 42 does not cover it: it passes CopyTree(destination, source, destination) with rootMount == destination, so relative is ".", pathComponents(".") returns nil, and the ancestor loop never executes.
Related, and best fixed together (this one is contested — one reviewer raised it, it was not cross-evaluated): the mirror-image inconsistency is that ensureDirectories only chmods inside its freshly-created branch, so re-copying into an existing destination silently keeps stale directory modes, while file modes are always reapplied (copyRegularFile always writes a fresh temp with mode and renames over). A second CopyTree run therefore converges file permissions but not directory permissions. Worth deciding explicitly whether CopyTree owns destination directory modes, then making creation and re-run agree.
| // CopyFile copies a regular file and its permissions to a destination beneath | ||
| // rootMount. | ||
| func CopyFile(rootMount, source, destination string) error { | ||
| info, err := os.Stat(source) |
There was a problem hiding this comment.
Contested — the two reviewers split on this one, so it needs your call.
The factual observation is not in dispute: this uses os.Stat(source), which follows symlinks, so a symlink pointing at a regular file passes the IsRegular check below and is silently dereferenced and copied. CopyTreeIfExists and CopyTree do the opposite — both use lstat semantics and reject a symlinked source explicitly.
The case for it being a defect: the package documents itself as providing symlink-safe operations, and this leaves one exported surface with two contradictory symlink policies. A file reached via CopyTree is rejected; the same file reached via CopyFile is followed. There is also no test passing a symlinked source to CopyFile, whereas the symlinked-source case for CopyTreeIfExists is covered.
The case against: source is an unrooted, caller-supplied path that is explicitly outside the root in this package's own tests, and the body reads it with os.Open(source), which follows anyway. Following a symlinked source is ordinary cp behaviour and grants a caller nothing it could not get by passing the resolved path directly. CopyTree rejects symlinks because a walker has to decide recursion and loop semantics, not for containment reasons.
One narrower point that survives either way: the destination pre-check below uses unrooted os.Lstat(destination), resolving parent components through the OS rather than through the rooted inspect used further down. Its os.SameFile short-circuit can therefore return early and mask the symlinked-component error the rooted check would have raised.
If the intent is that everything in this package is fail-closed on symlinks, this should use os.Lstat and reject, matching its siblings. If the intent is that CopyFile is a thin cp, that is worth saying in the doc comment, because the package-level doc currently implies otherwise.
|
|
||
| // CopyTreeIfExists recursively copies source into a destination beneath | ||
| // rootMount. A missing source is treated as an empty tree. | ||
| func CopyTreeIfExists(rootMount, source, destination string) error { |
There was a problem hiding this comment.
Contested — the two reviewers split on this one.
At this commit, CopyTreeIfExists, CopyTree and CopyFile are referenced only from copy_test.go; Resolve and CreateFile likewise only from hostfs_test.go. A repo-wide grep for hostfs. outside vendor and tests returns only flags/flags.go, flags/layout.go, flags/logs.go and history/history.go — none of which touch the copy surface — and agent/go/cmd/agent/main.go is still a Hello, World! stub.
So roughly half the new exported surface has no production caller.
The case for flagging it: the repo's stated rule is to build only what was asked for and avoid preemptive abstractions. This matters beyond style here, because the two copy.go findings above are latent defects in code that no integration currently exercises, and nothing will catch them until a caller appears.
The case against: this is plainly a staged port of the Python agent's existing copy behaviour, so the abstractions are anticipated rather than invented, and "unused at the moment" is a scope and sequencing question rather than a behavioural bug.
If the copy surface is landing ahead of its caller deliberately, saying so in the PR description would settle it.
3 participants
Description
Centralizes host-mounted filesystem ownership behind a symlink-safe rooted I/O package and migrates the agent state stores to use it.
Depends on #405.
Part of #219
Checklist
git commit -s) per the DCO.