feat(agent): add filesystem flag and log helpers#292
Open
rice-riley wants to merge 2 commits into
Open
Conversation
github-actions
Bot
added
component/operator
Contributor
|
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:
📝 WalkthroughWalkthroughA new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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`:
- Around line 66-79: Wrap the bare errors returned from Path(), Mark(), and
Check() so callers can tell which step failed; instead of returning helper
errors directly from validatePackagePath, describeStep, fingerprint, and the
store checks, return contextual errors with fmt.Errorf("...: %w", err). Update
the affected logic in Path, Mark, and Check to preserve the original error while
adding operation-specific context for validation, fingerprinting, and store
lookup/write failures.
- Around line 100-113: The current containment check in Store.Write and the flag
validation in Store.Check are only lexical and can be bypassed by symlinked
directories or flag files under s.dir. Update both paths to reject symlink
traversal by resolving the final path safely and verifying each existing
component/file with symlink-aware checks before mkdir/write or stat/accept. Make
sure Write does not follow a symlinked parent or target when creating the flag,
and Check does not trust os.Stat alone to mark a symlink target as a valid
completion flag.
In `@agent/go/internal/flags/layout.go`:
- Around line 48-55: The Layout root handling in NewLayout and the directory
helpers like StateDir and LogDir currently accept caller-supplied roots that can
resolve outside RootMount, violating the mounted-host-root contract. Add
validation/sanitization in NewLayout (and/or a shared helper used by StateDir
and LogDir) to reject absolute or path-traversal roots such as ../ or ../../
that would escape RootMount, and ensure the returned paths are always confined
beneath RootMount.
- Around line 99-104: LogFilePattern() is currently turning the literal stepPath
into a filepath.Glob pattern, which breaks valid names containing glob
metacharacters and can match unrelated files. Update LogFilePattern() and the
related cleanup logic in the same area so retention uses a literal prefix/suffix
check derived from packageLogDir()/LogFilePath() instead of globbing the raw
step path. Keep the pattern generation safe for names with [], * and ? by
matching on the exact log filename shape rather than embedding the full stepPath
in a glob.
🪄 Autofix (Beta)
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: 1627a6cc-8033-416a-a0a0-311fe854d9cc
📒 Files selected for processing (4)
agent/go/internal/flags/flags.goagent/go/internal/flags/flags_test.goagent/go/internal/flags/layout.goagent/go/internal/flags/logs.go
rice-riley
force-pushed
the
agent-go-flags-215
branch
from
4be0fc0 to
b4af406
Compare
Contributor
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/flags.go`:
- Around line 323-329: The writeFlagFile flow deletes the existing flag before
the replacement is safely persisted, so a failed create/write/close can lose a
valid flag. Update writeFlagFile to write the new contents to a same-directory
temporary file first, ensure the write and close succeed, and only then
atomically swap it into place when replace is true; keep the existing flag
untouched until that final step succeeds.
🪄 Autofix (Beta)
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: 5853f46f-9940-43ec-83a9-bad4af5171cd
📒 Files selected for processing (4)
agent/go/internal/flags/flags.goagent/go/internal/flags/flags_test.goagent/go/internal/flags/layout.goagent/go/internal/flags/logs.go
rice-riley
force-pushed
the
agent-go-flags-215
branch
from
b4af406 to
004772c
Compare
Contributor
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 49-79: The log retention logic in logs.go is parsing the UTC
timestamp from each filename but the cleanup order in the candidate sorting
still uses ModTime(), which can keep the wrong files when mtimes differ or
match. Update the retention flow around the candidate slice and sort in the log
cleanup code to order files by the parsed timestamp extracted from the filename,
using path only as a tie-breaker if needed, and keep the existing regular-file
and parse validation intact.
🪄 Autofix (Beta)
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: 79de81e6-5b3f-41ae-b577-6ecc902b0fc3
📒 Files selected for processing (6)
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.go
rice-riley
force-pushed
the
agent-go-flags-215
branch
from
004772c to
9879b37
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/layout.go`:
- Around line 50-64: Wrap the helper failures in NewLayout, LogFilePath,
LogFilePattern, PrepareLogFile, and packageLogDir with operation-specific
context instead of returning the raw error. Update the error paths around
normalizeLayoutRoot and any other helper calls in those functions to use
fmt.Errorf with %w so callers can tell which step failed, while preserving the
original error for unwrapping. Keep the function names and their existing
behavior intact, only change the returned errors to include descriptive context.
In `@agent/go/internal/flags/logs_test.go`:
- Around line 47-48: The test is misleading because it uses os.Chtimes even
though CleanupOldLogs decides retention from the timestamp embedded in the
filename, not file mtime. Remove the Chtimes setup from the logs test and make
the fixture data reflect the filename-based timestamp behavior instead. Use the
CleanupOldLogs test case and the file creation loop in logs_test.go to locate
the affected setup, and keep the assertions focused on filename ordering rather
than modification times.
🪄 Autofix (Beta)
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: 6db033f8-0b36-4c36-907b-148334c5e32d
📒 Files selected for processing (6)
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.go
1 task
rice-riley
force-pushed
the
agent-go-flags-215
branch
from
9879b37 to
17c80ad
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Around line 193-247: The completion fingerprint in
describeStep/metadataFromRegularStep currently omits execution environment
fields, so changes to Env and OnHost can incorrectly reuse an old flag. Extend
stepMetadata to carry those fields, populate them in metadataFromRegularStep
from step.RegularStep, and include them in fingerprint’s JSON payload so the
hash changes whenever execution environment changes.
In `@agent/go/internal/flags/layout.go`:
- Around line 170-173: Update validatePathComponent in layout.go to explicitly
reject "." as an invalid single path component in addition to the existing
empty/local/base checks. Add a direct guard in validatePathComponent(name,
value) so package name/version inputs cannot be "." before they reach
filepath.Join and collapse into the parent namespace.
🪄 Autofix (Beta)
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: 5442dde7-96bc-4083-a693-5d3586d78438
📒 Files selected for processing (6)
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.go
Signed-off-by: Riley Rice <riceriley59@gmail.com>
rice-riley
force-pushed
the
agent-go-flags-215
branch
from
17c80ad to
425b625
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat(agent): add filesystem flag and log helpers
Summary
This PR adds the Go agent's host-path, completion-flag, log-path, and log-retention helpers in a new
internal/flagspackage.It is the filesystem/flags half of #215. Package history is intentionally handled in a separate PR so the persistence and corruption-recovery behavior can be reviewed independently.
Relates to #215
What changed
flags.Layoutfor resolving state, flag, history, copied-step, and log directories beneath the mounted host root.flags.Storefor:5by default).Go-native design choices
This package introduces two new conventions because the Go agent did not yet have an established filesystem seam:
Layoutreceives resolved roots from process configuration instead of reading environment variables inside helpers. This keeps path construction deterministic and easy to test.Storeowns flag I/O and returns aDecisionwith a typedReason. The eventual controller can decide how to log each outcome without coupling this low-level package to Python's print strings.Times are explicit inputs rather than hidden
time.Now()calls, filesystem failures retain operation and path context, and flag writes use private file permissions.Compatibility note
The completion marker is intentionally Go-native rather than byte-compatible with the Python agent. It hashes a canonical JSON representation of the step path, arguments, and accepted return codes with SHA-256, producing a path-safe hexadecimal filename.
As a result, completion flags created by the Python agent are not recognized by this implementation. The first Go-agent execution after cutover will treat those steps as incomplete and run them again. This is deliberate for the rewrite and avoids carrying Python list-
reprsemantics into the Go API.The host directory roots remain
/etc/skyhookand/var/log/skyhookby default, while allowing the caller to supply different resolved roots.Testing
make unit-tests— 142 specs passed, including 30 flags specsmake lint— 0 issuesmake vetDocumentation
No user documentation changes are included because these helpers are not wired into the agent executable yet. The controller/entrypoint integration will land separately and will document any user-visible behavior at that point.
Checklist
git commit -s) per the DCO.