refactor(agent): unify regular and upgrade step execution - #407
refactor(agent): unify regular and upgrade step execution#407rice-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 centralizes command-step options, defaults, execution, fingerprinting, serialization, and environment resolution in shared helpers. Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent/go/internal/step/step.go (1)
74-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the
OnHostdefault when decoding.
newStepOptionsdefaultsOnHosttotrue.Decodecreates zero-value steps, andapplyStepDefaultsdoes not set boolean fields. A payload withouton_hosttherefore runs as non-host, while the equivalent constructed step runs in the host chroot.Initialize both decode targets with
OnHost: truebefore unmarshalling. JSON will still preserve an explicitfalse. Add a regression test for omitted and expliciton_host.Proposed fix
if probe.UpgradeStep { - var u UpgradeStep + u := UpgradeStep{OnHost: true} if err := json.Unmarshal(data, &u); err != nil { return nil, fmt.Errorf("decode upgrade step: %w", err) } @@ - var regular RegularStep + regular := RegularStep{OnHost: true} if err := json.Unmarshal(data, ®ular); err != nil { return nil, fmt.Errorf("decode step: %w", err) }🤖 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/step/step.go` around lines 74 - 90, Update Decode’s UpgradeStep and RegularStep initialization to set OnHost: true before json.Unmarshal, allowing an explicit JSON false to override the default. Add regression coverage for both omitted on_host (true) and explicit on_host:false (false), while preserving existing validation and default application.
🤖 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/step/shared.go`:
- Around line 204-238: Update step fingerprint generation around stepFingerprint
and its callers to include the runtime version pair used by
RegularStep.WithVersions and UpgradeStep.Run. Add the versions to the serialized
fingerprint payload and pass them from both Fingerprint implementations,
preserving distinct completion flags for different transitions to the same
package version.
---
Outside diff comments:
In `@agent/go/internal/step/step.go`:
- Around line 74-90: Update Decode’s UpgradeStep and RegularStep initialization
to set OnHost: true before json.Unmarshal, allowing an explicit JSON false to
override the default. Add regression coverage for both omitted on_host (true)
and explicit on_host:false (false), while preserving existing validation and
default application.
🪄 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: e303c3d0-0d57-426b-9b36-0c7b32377165
📒 Files selected for processing (11)
agent/go/internal/config/validate_test.goagent/go/internal/execution/execution.goagent/go/internal/step/regular_step.goagent/go/internal/step/regular_step_test.goagent/go/internal/step/shared.goagent/go/internal/step/shared_linux_test.goagent/go/internal/step/shared_test.goagent/go/internal/step/step.goagent/go/internal/step/step_test.goagent/go/internal/step/upgrade_step.goagent/go/internal/step/upgrade_step_test.go
92c0586 to
6105106
Compare
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/step/shared.go`:
- Around line 251-274: Update decoding in the step types and the applyDefaults
flow so omitted on_host fields retain the constructor default of true while
explicitly decoded false values remain false. Track on_host field presence
during Decode before applying defaults, initialize OnHost only when absent, and
add decode coverage for both step types covering omitted and explicit false
values.
🪄 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: ca6fe0ef-fee7-4c02-a3c2-3d361f7f22ef
📒 Files selected for processing (11)
agent/go/internal/config/validate_test.goagent/go/internal/execution/execution.goagent/go/internal/step/regular_step.goagent/go/internal/step/regular_step_test.goagent/go/internal/step/shared.goagent/go/internal/step/shared_linux_test.goagent/go/internal/step/shared_test.goagent/go/internal/step/step.goagent/go/internal/step/step_test.goagent/go/internal/step/upgrade_step.goagent/go/internal/step/upgrade_step_test.go
| func applyStepDefaults( | ||
| name *string, | ||
| path string, | ||
| arguments *[]string, | ||
| returncodes *[]command.ExitCode, | ||
| environment *map[string]string, | ||
| idempotence *Idempotence, | ||
| ) { | ||
| if *name == "" { | ||
| *name = path | ||
| } | ||
| if len(*arguments) == 0 { | ||
| *arguments = []string{} | ||
| } | ||
| if len(*returncodes) == 0 { | ||
| *returncodes = []command.ExitCode{command.SuccessExitCode} | ||
| } | ||
| if *environment == nil { | ||
| *environment = map[string]string{} | ||
| } | ||
| if *idempotence == "" { | ||
| *idempotence = Auto | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the OnHost default during decoding.
Line 63 makes true the constructor default. Decode calls applyDefaults at Lines 79 and 90 in agent/go/internal/step/step.go, but this helper never initializes OnHost. A payload that omits on_host therefore selects the non-host path at Line 151 instead of chroot execution.
Track on_host field presence during decoding. Set OnHost to true only when the field is absent. Preserve an explicit false. Add decode coverage for both step types.
🤖 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/step/shared.go` around lines 251 - 274, Update decoding in
the step types and the applyDefaults flow so omitted on_host fields retain the
constructor default of true while explicitly decoded false values remain false.
Track on_host field presence during Decode before applying defaults, initialize
OnHost only when absent, and add decode coverage for both step types covering
omitted and explicit false values.
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/step/upgrade_step_test.go`:
- Around line 116-144: Make this test platform-independent by removing the
/bin/sh script and using the existing
prepareStepTestExecutable/runStepTestHelper mechanism. Extend the inspect action
in runStepTestHelper to print PREVIOUS_VERSION and CURRENT_VERSION, then
construct the upgrade step with that helper and assert its arguments and
environment output while preserving the existing encoding assertions.
In `@agent/go/internal/step/upgrade_step.go`:
- Around line 84-102: In the Run method of UpgradeStep, extend the validation
after the nil check on s.versions to also verify that both s.versions.previous
and s.versions.current are non-empty strings. Return execution.StatusFailed with
an appropriate error message if either version string is empty, rejecting the
step before it proceeds to runStep. This ensures empty version values are caught
defensively rather than being passed as arguments or environment variables to
the script.
🪄 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: 9890055d-0c42-42a4-bfdc-7eb382a1d013
📒 Files selected for processing (11)
agent/go/internal/config/validate_test.goagent/go/internal/execution/execution.goagent/go/internal/step/regular_step.goagent/go/internal/step/regular_step_test.goagent/go/internal/step/shared.goagent/go/internal/step/shared_linux_test.goagent/go/internal/step/shared_test.goagent/go/internal/step/step.goagent/go/internal/step/step_test.goagent/go/internal/step/upgrade_step.goagent/go/internal/step/upgrade_step_test.go
| func (s UpgradeStep) Run(ctx context.Context, config execution.Config) (execution.Status, error) { | ||
| if err := s.Validate(); err != nil { | ||
| return execution.StatusFailed, fmt.Errorf("upgrade step validation failed: %w", err) | ||
| } | ||
| return u.RegularStep.Run(ctx, config) | ||
| if s.versions == nil { | ||
| return execution.StatusFailed, errors.New("running upgrade step: versions were not provided") | ||
| } | ||
| s.applyDefaults() | ||
| return runStep( | ||
| ctx, | ||
| config, | ||
| s.ScriptPath, | ||
| []string{s.versions.previous, s.versions.current}, | ||
| s.Returncodes, | ||
| s.OnHost, | ||
| s.Env, | ||
| s.versions, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every WithVersions call site and the values it supplies.
rg -n -C 10 --type go 'WithVersions\(' agent/go
# Find how the orchestration derives previous and current package versions.
rg -n -C 8 --type go -i 'previousVersion|currentVersion|PreviousVersion|unknown' agent/go --glob '!**/*_test.go'Repository: NVIDIA/nodewright
Length of output: 25863
🏁 Script executed:
# Find where upgrade steps have WithVersions called on them
rg -n -C 5 'WithVersions\(' agent/go --type go --glob '!**/*_test.go' | head -80
# Find the orchestration code that manages upgrade steps
fd -e go -path '*/agent/go/*' | xargs grep -l 'UpgradeStep' | grep -v test
# Look for where upgrade step versions come from
rg -n -C 3 'upgrade.*step' agent/go --type go -i | grep -i 'version\|history'Repository: NVIDIA/nodewright
Length of output: 4648
🏁 Script executed:
# Find the stage execution code that calls WithVersions with versions from history
rg -n 'WithVersions' agent/go --type go -B 5 -A 2 | grep -v test
# Look for code that reads from history and uses upgrade steps
rg -n -C 8 'history\.' agent/go/internal --type go | grep -A 8 -B 8 'WithVersions\|\.Read\|Versions'
# Search for the main orchestration entry point
fd -e go -path '*/agent/go/internal/*' ! -path '*/test*' | xargs grep -l 'Execute\|Run.*stage' | head -5Repository: NVIDIA/nodewright
Length of output: 2591
🏁 Script executed:
# Search for where WithVersions is actually called in non-test code
rg -n 'WithVersions\(' agent/go -A 3 -B 3 --glob '!*_test.go'
# Look for the stage execution logic
fd -e go -path '*/agent/go/internal/*' ! -name '*_test.go' -type f | xargs grep -l 'stage\|Stage' | head -10Repository: NVIDIA/nodewright
Length of output: 1929
🏁 Script executed:
# Search more broadly for WithVersions calls
rg 'WithVersions' agent/go -n | grep -v 'func.*WithVersions'
# Look at what files are in the internal directory
ls -la agent/go/internal/
# Search for the executor or orchestration that uses steps
find agent/go/internal -name '*.go' ! -name '*_test.go' -type f | xargs grep -l 'Execute\|ForEach\|Stages'Repository: NVIDIA/nodewright
Length of output: 1635
🏁 Script executed:
# Look at the stage package structure
ls -la agent/go/internal/stage/
# Look for Execute or main orchestration logic in stage
cat agent/go/internal/stage/*.go | head -200
# Search for any file that processes steps with versions
rg 'step\.' agent/go/internal/stage -A 3 -B 3 -nRepository: NVIDIA/nodewright
Length of output: 5799
🏁 Script executed:
# Look for the main agent file and executor
find agent/go -name '*.go' ! -name '*_test.go' -type f | xargs grep -l 'Execute\|Main'
# Look for the agent package structure
ls -la agent/go/
# Find where upgrade stage execution happens
rg -n 'Upgrade\|upgrade' agent/go --glob '!*_test.go' | grep -i 'execute\|run\|withversion' | head -20Repository: NVIDIA/nodewright
Length of output: 738
🏁 Script executed:
# Look at the cmd directory for the main entry point
ls -la agent/go/cmd/
# Check if there's a main file
find agent/go/cmd -name '*.go' -type f | head -10
# Look for executor or orchestration logic by searching for "step" usage
rg -n '\.WithVersions\|ForEach\|for.*step' agent/go --glob '!*_test.go' -A 3 -B 3Repository: NVIDIA/nodewright
Length of output: 349
🏁 Script executed:
# Look at the main entry point
cat agent/go/cmd/agent/main.go
# Check if there's Python code that calls the Go agent
find . -name '*.py' -type f | xargs grep -l 'WithVersions\|upgrade.*step' 2>/dev/null | head -5
# Search for Python code related to upgrade
find . -path '*/agent/python*' -name '*.py' -type f | head -10Repository: NVIDIA/nodewright
Length of output: 1497
🏁 Script executed:
# Look at the Python controller that orchestrates the agent
cat ./agent/skyhook-agent/src/skyhook_agent/controller.py
# Look at how Python calls upgrade steps
rg -n 'WithVersions\|upgrade' ./agent/skyhook-agent/src -A 3 -B 3
# Check step.py for how versions are handled
cat ./agent/skyhook-agent/src/skyhook_agent/step.pyRepository: NVIDIA/nodewright
Length of output: 37486
🏁 Script executed:
# Look at the specific function that handles versions for upgrade steps
sed -n '200,250p' ./agent/skyhook-agent/src/skyhook_agent/controller.py
# Look at the history data structure and defaults
sed -n '160,195p' ./agent/skyhook-agent/src/skyhook_agent/controller.pyRepository: NVIDIA/nodewright
Length of output: 4196
🏁 Script executed:
# Get the specific function that handles versions for upgrade steps
sed -n '500,570p' ./agent/skyhook-agent/src/skyhook_agent/controller.pyRepository: NVIDIA/nodewright
Length of output: 3322
🏁 Script executed:
# Find the get_or_update_history function which supplies versions to upgrade steps
grep -n "def get_or_update_history" ./agent/skyhook-agent/src/skyhook_agent/controller.py
# Show that function
sed -n '465,510p' ./agent/skyhook-agent/src/skyhook_agent/controller.pyRepository: NVIDIA/nodewright
Length of output: 2219
🏁 Script executed:
# Show the get_or_update_history function in full
sed -n '376,430p' ./agent/skyhook-agent/src/skyhook_agent/controller.pyRepository: NVIDIA/nodewright
Length of output: 2907
🏁 Script executed:
# Show the rest of the get_or_update_history function
sed -n '430,445p' ./agent/skyhook-agent/src/skyhook_agent/controller.pyRepository: NVIDIA/nodewright
Length of output: 915
🏁 Script executed:
# Check the exact behavior when JSON is corrupted - does it re-initialize?
sed -n '405,420p' ./agent/skyhook-agent/src/skyhook_agent/controller.pyRepository: NVIDIA/nodewright
Length of output: 916
Run method should validate that version strings are not empty.
The guard at line 88 only rejects a nil versions pointer. The WithVersions method accepts any string values without validation. Empty strings would be passed to the script as arguments and set as PREVIOUS_VERSION and CURRENT_VERSION environment variables.
The Python orchestration (agent/skyhook-agent/src/skyhook_agent/controller.py, get_or_update_history function) always supplies non-nil, non-empty strings: either actual version strings from the history file, the string "unknown" when no history exists, or the configured package version. However, the Go implementation should enforce this invariant defensively by rejecting empty strings, matching the pattern used by the history package itself (which converts empty CurrentVersion to "unknown").
🤖 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/step/upgrade_step.go` around lines 84 - 102, In the Run
method of UpgradeStep, extend the validation after the nil check on s.versions
to also verify that both s.versions.previous and s.versions.current are
non-empty strings. Return execution.StatusFailed with an appropriate error
message if either version string is empty, rejecting the step before it proceeds
to runStep. This ensures empty version values are caught defensively rather than
being passed as arguments or environment variables to the script.
6105106 to
cf0ce60
Compare
cf0ce60 to
84eb7e9
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 (4)
agent/go/internal/step/upgrade_step.go (1)
84-102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Runaccepts empty version strings.The guard at Line 88 rejects only a nil
versionspointer.WithVersionsperforms no validation. Empty strings then reach the script as the first two arguments and asPREVIOUS_VERSIONandCURRENT_VERSION. The step script cannot distinguish an empty version from a missing one.Reject empty
previousorcurrentvalues before callingrunStep.🛡️ Proposed guard
if s.versions == nil { return execution.StatusFailed, errors.New("running upgrade step: versions were not provided") } + if s.versions.previous == "" || s.versions.current == "" { + return execution.StatusFailed, errors.New( + "running upgrade step: previous and current versions must not be empty", + ) + }🤖 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/step/upgrade_step.go` around lines 84 - 102, Update UpgradeStep.Run’s versions validation to reject nil versions or empty versions.previous/current values before applyDefaults and runStep; return StatusFailed with an appropriate validation error while preserving the existing behavior for valid versions.agent/go/internal/step/upgrade_step_test.go (1)
116-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test depends on
/bin/shand carries no build constraint.The test writes a script with a
#!/bin/shshebang and runs it withWithOnHost(false), so the interpreter must exist on the machine that runs the suite. Every other execution test in this package re-execs the compiled test binary throughprepareStepTestExecutable, which keeps the suite portable.Extend the
inspectaction inrunStepTestHelperinagent/go/internal/step/regular_step_test.goto printPREVIOUS_VERSIONandCURRENT_VERSION, then build the upgrade step from the helper executable and assert against that output.🤖 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/step/upgrade_step_test.go` around lines 116 - 144, Replace the shell-script executable setup in the upgrade test with the compiled helper produced by prepareStepTestExecutable via the inspect action in runStepTestHelper. Extend that inspect action in runStepTestHelper to print PREVIOUS_VERSION and CURRENT_VERSION, then build the upgrade step with the helper executable and preserve assertions for arguments, environment values, validation, and encoded arguments.agent/go/internal/step/shared.go (2)
204-238: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
stepFingerprintstill excludes the runtime version pair.
UpgradeStep.Fingerprintpassess.Arguments, which the no-arguments invariant keeps empty.RegularStep.WithVersionsalso leaves the fingerprint inputs unchanged. Two different transitions to the same target version therefore produce the same digest, so any completion-flag or dedup consumer keyed on the fingerprint can skip a step that ran for a different version pair.If this omission is intentional, state the reason in a comment on
stepFingerprint.🤖 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/step/shared.go` around lines 204 - 238, Update stepFingerprint and its callers, including UpgradeStep.Fingerprint and RegularStep.WithVersions, to include the runtime version pair in the serialized fingerprint payload so transitions to the same target from different source versions produce distinct digests. If the pair is intentionally excluded, document that decision directly in stepFingerprint instead.
251-274: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
applyStepDefaultsstill does not preserve theOnHostconstructor default.Line 63 sets
onHost: truefor constructed steps.Decodeinagent/go/internal/step/step.go(Lines 79 and 90) callsapplyDefaults, which routes toapplyStepDefaults. This helper does not touchOnHost. A wire payload that omitson_hosttherefore decodes tofalseand selects the mounted-root path at Line 151 instead of chroot execution.Track presence of the
on_hostkey during decoding. SetOnHosttotrueonly when the key is absent. Preserve an explicitfalse. Add decode coverage for both step types.🤖 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/step/shared.go` around lines 251 - 274, Update the Decode/applyDefaults flow in step.go and applyStepDefaults so decoding tracks whether the on_host key was present; default OnHost to true only when absent, while preserving an explicit false value. Add decode coverage for both supported step types covering omitted and explicitly false on_host values.
🤖 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/step/shared.go`:
- Around line 204-238: Update stepFingerprint and its callers, including
UpgradeStep.Fingerprint and RegularStep.WithVersions, to include the runtime
version pair in the serialized fingerprint payload so transitions to the same
target from different source versions produce distinct digests. If the pair is
intentionally excluded, document that decision directly in stepFingerprint
instead.
- Around line 251-274: Update the Decode/applyDefaults flow in step.go and
applyStepDefaults so decoding tracks whether the on_host key was present;
default OnHost to true only when absent, while preserving an explicit false
value. Add decode coverage for both supported step types covering omitted and
explicitly false on_host values.
In `@agent/go/internal/step/upgrade_step_test.go`:
- Around line 116-144: Replace the shell-script executable setup in the upgrade
test with the compiled helper produced by prepareStepTestExecutable via the
inspect action in runStepTestHelper. Extend that inspect action in
runStepTestHelper to print PREVIOUS_VERSION and CURRENT_VERSION, then build the
upgrade step with the helper executable and preserve assertions for arguments,
environment values, validation, and encoded arguments.
In `@agent/go/internal/step/upgrade_step.go`:
- Around line 84-102: Update UpgradeStep.Run’s versions validation to reject nil
versions or empty versions.previous/current values before applyDefaults and
runStep; return StatusFailed with an appropriate validation error while
preserving the existing behavior for valid versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 76d070e4-9976-41ac-8613-54ae3ed71914
📒 Files selected for processing (11)
agent/go/internal/config/validate_test.goagent/go/internal/execution/execution.goagent/go/internal/step/regular_step.goagent/go/internal/step/regular_step_test.goagent/go/internal/step/shared.goagent/go/internal/step/shared_linux_test.goagent/go/internal/step/shared_test.goagent/go/internal/step/step.goagent/go/internal/step/step_test.goagent/go/internal/step/upgrade_step.goagent/go/internal/step/upgrade_step_test.go
84eb7e9 to
3d1b12a
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 (3)
agent/go/internal/step/shared.go (2)
251-274: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
applyStepDefaultsdoes not restore theOnHostdefault.Line 63 sets
onHost: truefor constructor paths.applyStepDefaultsnever initializesOnHost, andDecodeinagent/go/internal/step/step.gocallsapplyDefaultson a zero-valued struct. A payload that omitson_hosttherefore runs the non-host branch at Line 151 instead of chroot execution. Track field presence during decoding and setOnHosttotrueonly whenon_hostis absent.🤖 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/step/shared.go` around lines 251 - 274, The decoding/defaulting flow around applyStepDefaults and Decode must preserve the constructor default OnHost=true when the on_host field is omitted. Track whether on_host was present during decoding, then set OnHost to true only when absent while preserving explicitly supplied true or false values.
204-238: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
stepFingerprintstill omits the runtime version pair.
UpgradeStep.Runbuilds its command arguments froms.versions, andrunStepinjectsPREVIOUS_VERSIONandCURRENT_VERSIONinto the environment. Neither value reachesstepFingerprint, so two different version transitions to the same package version produce the same digest. Downstream completion-flag paths keyed by fingerprint can then skip a step for the wrong version pair.🤖 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/step/shared.go` around lines 204 - 238, Update stepFingerprint and its callers to include the runtime version pair in the fingerprint payload, using the same versions represented by UpgradeStep.Run arguments and runStep’s PREVIOUS_VERSION and CURRENT_VERSION environment values. Ensure distinct version transitions produce distinct digests and preserve the existing JSON hashing behavior.agent/go/internal/step/upgrade_step.go (1)
84-102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Runaccepts empty version strings.The guard at Line 88 rejects only a nil
versionspointer.WithVersionsperforms no validation, so empty strings pass through as the first two script arguments and asPREVIOUS_VERSIONandCURRENT_VERSION. Reject empty values before execution.🛡️ Proposed guard
if s.versions == nil { return execution.StatusFailed, errors.New("running upgrade step: versions were not provided") } + if s.versions.previous == "" || s.versions.current == "" { + return execution.StatusFailed, errors.New("running upgrade step: versions must not be empty") + }🤖 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/step/upgrade_step.go` around lines 84 - 102, Update UpgradeStep.Run to validate both s.versions.previous and s.versions.current are non-empty after the nil check and before applyDefaults or runStep; return execution.StatusFailed with an appropriate validation error when either value is empty, while preserving execution for valid versions.
🤖 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/step/shared.go`:
- Around line 251-274: The decoding/defaulting flow around applyStepDefaults and
Decode must preserve the constructor default OnHost=true when the on_host field
is omitted. Track whether on_host was present during decoding, then set OnHost
to true only when absent while preserving explicitly supplied true or false
values.
- Around line 204-238: Update stepFingerprint and its callers to include the
runtime version pair in the fingerprint payload, using the same versions
represented by UpgradeStep.Run arguments and runStep’s PREVIOUS_VERSION and
CURRENT_VERSION environment values. Ensure distinct version transitions produce
distinct digests and preserve the existing JSON hashing behavior.
In `@agent/go/internal/step/upgrade_step.go`:
- Around line 84-102: Update UpgradeStep.Run to validate both
s.versions.previous and s.versions.current are non-empty after the nil check and
before applyDefaults or runStep; return execution.StatusFailed with an
appropriate validation error when either value is empty, while preserving
execution for valid versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 8272699f-93ca-44db-9297-9531d29d1b00
📒 Files selected for processing (11)
agent/go/internal/config/validate_test.goagent/go/internal/execution/execution.goagent/go/internal/step/regular_step.goagent/go/internal/step/regular_step_test.goagent/go/internal/step/shared.goagent/go/internal/step/shared_linux_test.goagent/go/internal/step/shared_test.goagent/go/internal/step/step.goagent/go/internal/step/step_test.goagent/go/internal/step/upgrade_step.goagent/go/internal/step/upgrade_step_test.go
3d1b12a to
183eec6
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. |
183eec6 to
4d68da6
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent/go/internal/step/step.go (1)
74-91: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the default
OnHostvalue for omitted fields.An omitted
on_hostfield and an explicitfalseboth decode tofalse. This bypasses the constructor default oftrue. The shared runner then uses the non-chroot path.Track
on_hostfield presence during decoding. SetOnHosttotrueonly when the field is absent. Preserve explicitfalse. Add decode coverage for both step types.🤖 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/step/step.go` around lines 74 - 91, Update the step decoding flow around UpgradeStep and RegularStep to track whether the on_host field was present, setting OnHost to true only when omitted while preserving an explicit false. Apply this behavior consistently to both step types and add decode coverage for omitted and explicitly false on_host values.
♻️ Duplicate comments (2)
agent/go/internal/step/shared.go (1)
204-238: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude runtime versions in the step fingerprint.
Runtime versions change command execution. The current fingerprint cannot distinguish version transitions that use the same configured step.
agent/go/internal/step/shared.go#L204-L238: serialize explicit previous and current runtime-version fields in the fingerprint payload.agent/go/internal/step/regular_step.go#L91-L94: passs.versionsto the shared fingerprint helper.agent/go/internal/step/upgrade_step.go#L104-L107: passs.versionsto the shared fingerprint helper.Add tests that confirm distinct version pairs produce distinct fingerprints.
🤖 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/step/shared.go` around lines 204 - 238, Extend stepFingerprint in agent/go/internal/step/shared.go (lines 204-238) to serialize explicit previous and current runtime-version fields in its payload. Update callers in agent/go/internal/step/regular_step.go (lines 91-94) and agent/go/internal/step/upgrade_step.go (lines 104-107) to pass s.versions to the helper, and add tests verifying distinct version pairs produce distinct fingerprints.agent/go/internal/step/upgrade_step.go (1)
88-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject empty runtime versions before execution.
The nil check accepts
WithVersions("", ""). The command then receives empty positional arguments and emptyPREVIOUS_VERSIONandCURRENT_VERSIONvalues.Reject an empty previous or current version before calling
runStep. Add coverage for each invalid value.🤖 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/step/upgrade_step.go` around lines 88 - 100, Update the version validation in the upgrade step before runStep so it rejects both empty s.versions.previous and empty s.versions.current, returning the existing failed status with an appropriate error. Preserve valid version execution and add coverage for each empty value.
🤖 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.
Outside diff comments:
In `@agent/go/internal/step/step.go`:
- Around line 74-91: Update the step decoding flow around UpgradeStep and
RegularStep to track whether the on_host field was present, setting OnHost to
true only when omitted while preserving an explicit false. Apply this behavior
consistently to both step types and add decode coverage for omitted and
explicitly false on_host values.
---
Duplicate comments:
In `@agent/go/internal/step/shared.go`:
- Around line 204-238: Extend stepFingerprint in
agent/go/internal/step/shared.go (lines 204-238) to serialize explicit previous
and current runtime-version fields in its payload. Update callers in
agent/go/internal/step/regular_step.go (lines 91-94) and
agent/go/internal/step/upgrade_step.go (lines 104-107) to pass s.versions to the
helper, and add tests verifying distinct version pairs produce distinct
fingerprints.
In `@agent/go/internal/step/upgrade_step.go`:
- Around line 88-100: Update the version validation in the upgrade step before
runStep so it rejects both empty s.versions.previous and empty
s.versions.current, returning the existing failed status with an appropriate
error. Preserve valid version execution and add coverage for each empty value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 64cd8913-2cd4-4065-a7b7-c849dc5000ad
📒 Files selected for processing (11)
agent/go/internal/config/validate_test.goagent/go/internal/execution/execution.goagent/go/internal/step/regular_step.goagent/go/internal/step/regular_step_test.goagent/go/internal/step/shared.goagent/go/internal/step/shared_linux_test.goagent/go/internal/step/shared_test.goagent/go/internal/step/step.goagent/go/internal/step/step_test.goagent/go/internal/step/upgrade_step.goagent/go/internal/step/upgrade_step_test.go
4d68da6 to
1b1ffc8
Compare
1b1ffc8 to
a5b9a9b
Compare
Signed-off-by: Riley Rice <rrice@nvidia.com>
a5b9a9b to
608c14e
Compare
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 instructed to refute it. Reviewed against this PR's actual base (0a02c4f5, the agent-hostfs-219 branch), not main, so defects already present in that base are out of scope.
Outcome: twelve candidate findings were raised. Four were refuted during cross-review, four reached consensus and were then adversarially verified, and of those one survived. Six remain contested and need a human call. The integration pass verified fourteen change-list items and found no broken consumers.
Two of the strongest-looking round-1 findings were refuted on the same grounds, worth stating because it is a point in this PR's favour: the non-host execution path is not newly exposed, it was previously broken (the base pointed a container-namespace step at a host-absolute path that does not exist inside a distroless container), and the version strings that appeared to be injectable are constrained by an anchored semver pattern in skyhook-agent-schema.json and so cannot contain the env: prefix that would be needed.
No code was executed at any point in this review; every claim is from static reading at the pinned commit.
Verified with no issues
The integration pass confirmed each of these individually, which is worth recording since the de-embedding could plausibly have broken any of them:
- The step fingerprint is byte-stable. The pinned flag-filename hash in
flags_test.go:80is unchanged, so existing completion flags on nodes stay valid. - The emitted JSON is byte-identical for both step kinds despite
UpgradeStepno longer embeddingRegularStep. CURRENT_VERSION/PREVIOUS_VERSIONmatch the shipping Python agent in name, value semantics, and argument order (controller.py:434-438).- No type outside
internal/stepimplementsstep.Step, and no composite literalUpgradeStep{RegularStep: ...}survives anywhere.
Contested — raised but not cross-evaluated
agent/README.md:23 — this file still carries the sentence "A Config composes the host root mount, the child-visible step and package directories…", which is verbatim the wording this PR rewrote in agent/go/internal/execution/execution.go:22 to "the host root mount, step and package paths within that host". agent/README.md is untouched by this commit, so the repo now describes execution.Config two contradictory ways. One reviewer read the untouched file as out of scope; the counter-argument is that the contradiction is created by this commit, and this project treats same-PR doc updates as a blocking requirement for behaviour changes. Your call, but it is a one-line fix.
Open questions
These could not be settled without running code, which this review deliberately does not do:
- Does
regular_step_test.go:196pass on macOS? It assertsworking-directory=against a lexically joinedGinkgoT().TempDir(), but the child's environment is replaced wholesale soPWDis absent andos.Getwdfalls back to the syscall, which on macOS resolves/var/…to/private/var/…. Presumably fine on Linux CI. - Does
golangci-lintacceptencodeStep(value any, …)and the six-pointer-parameterapplyStepDefaultsunderagent/go/.golangci.yml? - Worth deciding before the first caller lands:
history.Versionsdeclares its fields in the orderCurrent, Previous, whileStep.WithVersions(previous, current string)takes them in the opposite order. Both are barestrings, so an orchestrator written ass.WithVersions(v.Current, v.Previous)compiles cleanly and silently swaps them, handing the package author's upgrade script its arguments backwards. Nothing consumes this pairing yet, so it is not a defect today, but it is the most likely place the Go agent will diverge fromcontroller.py:434-438. A named struct, or matching the field order, would remove the hazard entirely. - Relatedly:
UpgradeStep.Runhard-fails when versions are absent, butRegularStep.Rundoes not, while the Python agent sets the version variables on every step in the upgrade stage (controller.py:617-619), not just upgrade steps. An orchestrator that only callsWithVersionson values type-asserting toUpgradeStepwould give regular upgrade-stage steps empty version env with no error. Is the asymmetry intended?
| ) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| value := NewRegularStep( | ||
| status, err := runStep( |
There was a problem hiding this comment.
This rewrite drops the only coverage that pinned RegularStep.Run forwarding s.OnHost in the true direction.
The base built a real step and went through the public API:
value := NewRegularStep("step-helper", WithArguments(...))
status, err := value.Run(context.Background(), config)which exercised the default OnHost=true. The head calls the unexported runStep with onHost hard-coded as a literal true, so RegularStep is no longer in the path at all.
An exhaustive sweep of the package confirms nothing else covers it: every .Run( on a step value in regular_step_test.go (lines 176, 219, 239, 249, 261, 288, 304) passes WithOnHost(false); the two NewUpgradeStep("upgrade.sh") specs default to OnHost=true but return early on the Validate/nil-versions guards before reaching runStep; shared_test.go passes false; and no test outside internal/step calls Step.Run at all. So no public Run reaches runStep with onHost=true.
Concretely: hard-coding false in RegularStep.Run would now pass the entire suite. On the base that would have failed this spec, because without the non-host hostfs.Resolve the step would have exec'd the nonexistent /steps/step-helper.
This is live coverage, not theoretical — agent-go-ci.yaml runs the job in an ubuntu:latest container as root, so the chroot spec actually executes.
Two things narrow it: the opposite regression (hard-coding true) is caught by the new regular_step_test.go:195 spec via its step-root=<mountedStepRoot> assertion, and the applyDefaults-clobber variant is impossible because applyStepDefaults takes no onHost parameter. Test-only; no production behaviour change. Keeping one public-API call on the chroot path would close it.
| environment = maps.Clone(environment) | ||
| } | ||
| if versions != nil { | ||
| environment[previousVersionEnv] = versions.previous |
There was a problem hiding this comment.
Contested — the reviewers split on this one.
The configured environment is cloned just above, then PREVIOUS_VERSION and CURRENT_VERSION are written unconditionally. A package whose config legitimately sets either key runs with values it did not declare, with no error, warning, or validation. There is no reserved-name check in the config loader, and the schema accepts arbitrary env keys.
For treating it as a defect: a package author has no way to discover the collision. Their declared value silently disappears at runtime, and the failure would surface as inexplicable behaviour inside their script rather than as a config error. Rejecting the collision at validation time would be cheap and would fail loudly at the right layer.
Against: this is exactly what the shipping Python agent does (controller.py:434-435 assigns both keys with no collision check), and the same unconditional-override pattern already applies to STEP_ROOT and SKYHOOK_DIR two lines below, both in this file and in the base. Rejecting a CURRENT_VERSION collision while silently stomping a STEP_ROOT collision would arguably be the less consistent design.
If you keep the current behaviour, the reserved names are worth documenting somewhere a package author will actually look.
| environment[previousVersionEnv] = versions.previous | ||
| environment[currentVersionEnv] = versions.current | ||
| } | ||
| environment["STEP_ROOT"] = stepRoot |
There was a problem hiding this comment.
Contested — raised late in the review, so it carries only one reviewer's position.
STEP_ROOT and SKYHOOK_DIR now carry values from different namespaces depending on on_host. For an on-host step they stay host-absolute, matching the Python agent's docstring (controller.py:290-291, "The path on the host to the root directory of all the steps"). For a non-host step they are rewritten to <rootMount>/… just above and exported here, as the new spec at regular_step_test.go:227-229 asserts.
Within each namespace that is arguably correct, and it is what makes the non-host path work at all. The concern is that it is a package-author-visible contract with no documentation: a script that records $STEP_ROOT or $SKYHOOK_DIR into host state, or passes either to something that later runs on the host, gets a container-prefixed path that is meaningless there.
Neither agent/README.md nor the on_host description in agent/go/internal/config/schemas/v1/step-schema.json (currently just "Run the step on the host or inside the agent") notes the split.
| // UpgradeStep runs during the Upgrade and UpgradeCheck modes. | ||
| type UpgradeStep struct { | ||
| RegularStep | ||
| Name string `json:"name"` |
There was a problem hiding this comment.
Contested — the reviewers split on this one.
Dropping the embedded RegularStep leaves two hand-maintained copies of the same nine exported fields, their JSON tags, the unexported versions field, and applyDefaults. Nothing in the type system links them, and both feed the same Decode path.
The reason this is worth more than a style note is that the test intended to guard it has a hole. upgrade_step_test.go:196-209 byte-compares NewRegularStep("shared.sh").Encode() against NewUpgradeStep("shared.sh").Encode(). But under default construction Env is an empty map and RequiresInterrupt is false, and both fields carry omitempty — so neither key appears in either payload. The comparison cannot see them.
Concretely: renaming UpgradeStep's Env tag to json:"environment,omitempty", or deleting RequiresInterrupt from UpgradeStep entirely, leaves the whole suite green while silently dropping those fields on the upgrade branch of Decode. A package's step env or interrupt requirement would be lost at runtime with no signal.
Against: the fields and tags do match today, so this is a future-maintenance risk rather than a present defect.
If the duplication stays, extending that byte-comparison to a step constructed with a non-empty Env and RequiresInterrupt: true would close the hole cheaply.
| IdempotenceMode Idempotence `json:"idempotence"` | ||
| UpgradeStep bool `json:"upgrade_step"` | ||
| Env map[string]string `json:"env,omitempty"` | ||
| RequiresInterrupt bool `json:"requires_interrupt,omitempty"` |
There was a problem hiding this comment.
Contested — the reviewers split on this one.
The base carried a comment immediately above this field recording that RequiresInterrupt round-trips as requires_interrupt, is emitted only when true, and "stays schema-valid because the step schema permits extra properties." It is gone, and the field now stands unexplained here and on UpgradeStep.
The invariant is real, non-obvious, and owned by a different file: agent/go/internal/config/schemas/v1/step-schema.json lists requires_interrupt in neither properties nor required, and omits additionalProperties: false. The field's schema-validity rests entirely on that omission. A future change that tightens the schema, or that "tidies up" the omitempty, has no in-code signal that the two are coupled.
Against: no behaviour changed; this is a deleted explanation rather than a code bug.
This PR deletes a substantial number of explanatory comments. Most of them genuinely were narration and are better gone. This one, and the OnHost seeding comment, are the two that documented constraints living outside the file they annotated.
| ) | ||
|
|
||
| func newStepOptions(path string, opts ...Option) stepOptions { | ||
| value := stepOptions{onHost: true} |
There was a problem hiding this comment.
Contested — the reviewers split on this one.
The base carried two paired comments explaining this seeding: one here ("OnHost is seeded true here because Go's bool has no 'absent' state, so applyDefaults can't tell zero apart from 'explicit false'; WithOnHost(false) overrides this initial value") and a matching note on applyDefaults saying OnHost is intentionally not defaulted. Both are gone, leaving a bare stepOptions{onHost: true} here and an applyStepDefaults that silently has no onHost parameter.
The asymmetry now looks like an oversight, and the obvious "fix" is harmful: adding an onHost default to applyStepDefaults would break WithOnHost(false), because both RegularStep.Run and UpgradeStep.Run call applyDefaults on every run and would flip an explicitly-false OnHost back to true. That silently promotes a container-scoped step to chrooted host execution as root.
Against: no behaviour changed, and existing specs still pin both the explicit-false and default-true cases, so the mistake would be caught.
Given the failure mode, a one-line note on why onHost is absent from applyStepDefaults seems worth keeping.
2 participants
Description
Consolidates regular and upgrade steps onto one command-execution implementation while keeping their wire types and stage-specific behavior explicit.
Depends on #406.
Part of #219
Checklist
git commit -s) per the DCO.