feat(agent): complete Go agent orchestration and entrypoint - #379
feat(agent): complete Go agent orchestration and entrypoint#379rice-riley wants to merge 1 commit into
Conversation
github-actions
Bot
added
doc
|
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 a Go agent runtime with CLI argument parsing, environment configuration, validation, filesystem preparation, lifecycle step execution, interrupt handling, cancellation, logging, flags, and history updates. The CLI now handles Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 7
🤖 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/cmd/agent/main.go`:
- Around line 34-35: Rename the local variable nodewright_agent to idiomatic
mixedCaps naming, such as nodewrightAgent, and update its Run invocation
accordingly; leave the surrounding agent.New and execution flow unchanged.
In `@agent/go/internal/agent/agent.go`:
- Around line 249-263: Update normalizeRuntime to default stateRoot and logRoot
when they are empty, using the documented default root values before returning
the normalized runtimeConfig. Keep the existing dataDir, stdout, stderr, and
logger normalization unchanged so runRequest receives complete roots for
flags.NewLayout.
- Around line 241-247: Update envBool to parse environment values with
strconv.ParseBool instead of comparing only against defaultTrueValue, accepting
standard boolean forms while rejecting invalid values such as whitespace-padded
or arbitrary strings. When parsing fails, emit a warning and return the provided
fallback value so misconfigured default-true variables remain enabled rather
than silently becoming false.
In `@agent/go/internal/agent/interrupt.go`:
- Around line 107-155: The interrupt execution flow duplicates closeLog error
wrapping and errors.Join handling across failure branches. Extract a local
closeLogErr helper near closeLog that closes the log and returns the
consistently wrapped error, then reuse it in the log-retention and
execution-configuration failure paths (and the third matching branch) while
preserving the existing joined failure status.
In `@agent/go/internal/agent/package_test.go`:
- Around line 32-42: Update the “copies the container resolver into the mounted
root” test to check whether /etc/resolv.conf exists before calling
copyResolverConfig; skip the spec when the source file is absent, while
preserving the existing copy and content assertions when it is available.
In `@agent/go/internal/step/shared.go`:
- Around line 173-179: Update the environment initialization in runStep so a nil
input becomes an allocated writable map before assigning version and directory
values. Preserve cloning for non-nil environments and keep the existing
assignments to previousVersionEnv, currentVersionEnv, STEP_ROOT, and SKYHOOK_DIR
unchanged.
In `@agent/README.md`:
- Around line 74-80: Update the environment-variable documentation in the listed
README section to distinguish required variables from optional variables with
documented defaults: keep OVERLAY_FRAMEWORK_VERSION and SKYHOOK_RESOURCE_ID
required, and identify SKYHOOK_DATA_DIR, SKYHOOK_ROOT_DIR, and SKYHOOK_LOG_DIR
as defaulted. Correct the typos in “environment,” “this the,” and
“configuration” while preserving the documented defaults and descriptions.
🪄 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: e70aee63-f105-40f2-ac45-92205647c2b6
⛔ Files ignored due to path filters (1)
agent/go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
agent/README.mdagent/go/.mockery.yamlagent/go/Makefileagent/go/cmd/agent/main.goagent/go/deps.mkagent/go/go.modagent/go/internal/agent/agent.goagent/go/internal/agent/agent_test.goagent/go/internal/agent/interrupt.goagent/go/internal/agent/interrupt_test.goagent/go/internal/agent/package.goagent/go/internal/agent/package_test.goagent/go/internal/agent/request_test.goagent/go/internal/agent/runtime_test.goagent/go/internal/agent/steps.goagent/go/internal/agent/steps_test.goagent/go/internal/config/mock/SchemaValidator.goagent/go/internal/config/validate_test.goagent/go/internal/execution/execution.goagent/go/internal/flags/flags.goagent/go/internal/flags/flags_test.goagent/go/internal/history/history.goagent/go/internal/history/history_test.goagent/go/internal/history/mock/Store.goagent/go/internal/hostfs/copy.goagent/go/internal/hostfs/copy_test.goagent/go/internal/hostfs/hostfs.goagent/go/internal/hostfs/hostfs_test.goagent/go/internal/interrupts/mock/Interrupt.goagent/go/internal/step/mock/Step.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
💤 Files with no reviewable changes (2)
- agent/go/internal/history/history_test.go
- agent/go/internal/history/history.go
f7b3432 to
5a86acf
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/agent/agent_test.go`:
- Around line 265-279: Extend the cancellation tests around New().Run with a
multi-step stage that cancels the context while the first step is executing.
Assert the active step aborts and the subsequent step does not start, covering
the per-iteration cancellation guard in steps.go rather than only validateRun’s
pre-flight ctx.Err() check.
- Around line 287-309: The test around orchestrator{}.runRequest assumes
/etc/resolv.conf exists when runtime.copyResolver is true. Make the
resolver-copy verification hermetic by injecting a controlled resolver source
through the available configuration path, or conditionally skip the resolver
assertion when the host file is absent, while preserving the existing success
and other file assertions.
In `@agent/go/internal/agent/interrupt.go`:
- Around line 181-199: Update configFromResourceID to parse resource IDs with an
unambiguous fixed prefix or encoding so package names containing underscores
remain intact, while still extracting the package version correctly. Preserve
validatePathComponent checks and the existing error behavior for malformed IDs.
In `@agent/go/internal/agent/steps_test.go`:
- Around line 129-142: The always-run completion-flag test only verifies
execution and must also verify the mandated warning. In the test setup around
runSteps, capture logger output with a bytes.Buffer instead of discarding it,
then assert the buffer contains the warning emitted when a completion flag
exists while runtime.alwaysRunStep is enabled.
In `@agent/go/internal/flags/flags.go`:
- Around line 136-150: Extract the shared path-resolution and validation
sequence from fileStore.Remove, Write, and Check into a small helper method.
Have each method call the helper and preserve the existing wrapped error context
and returned validated path, keeping their file-operation behavior unchanged.
In `@agent/go/internal/hostfs/hostfs.go`:
- Around line 225-263: Extract the shared temp-file write, explicit permission
adjustment, rename, and cleanup flow from writeFile and copyRegularFile into one
rooted-write helper. Ensure the helper calls root.Chmod with the requested mode
after creating the file so both paths produce identical permissions despite
umask masking, then update both callers to use it while preserving their
existing error and atomic-replacement behavior.
In `@agent/go/Makefile`:
- Around line 38-41: Update the license-fmt target in the agent/go Makefile so
format_license.py scopes changed-file processing to agent/go rather than the
repository root. Keep repo-wide formatting out of this component target; if
needed, place it in the top-level Makefile target instead.
In `@agent/README.md`:
- Line 87: Update the SKYHOOK_NODE_ORDER description in the README to insert
“is” after the variable name, so the sentence begins with “SKYHOOK_NODE_ORDER is
a zero-indexed monotonic position” while preserving the remaining wording.
- Line 65: Update the environment-variable description in the README to refer to
the agent rather than the controller, while preserving the existing meaning and
surrounding documentation.
🪄 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: e7f7d403-1151-43cc-8774-ae7116bd5861
⛔ Files ignored due to path filters (1)
agent/go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
agent/README.mdagent/go/.mockery.yamlagent/go/Makefileagent/go/cmd/agent/main.goagent/go/deps.mkagent/go/go.modagent/go/internal/agent/agent.goagent/go/internal/agent/agent_test.goagent/go/internal/agent/interrupt.goagent/go/internal/agent/interrupt_test.goagent/go/internal/agent/package.goagent/go/internal/agent/package_test.goagent/go/internal/agent/steps.goagent/go/internal/agent/steps_test.goagent/go/internal/config/mock/SchemaValidator.goagent/go/internal/config/validate_test.goagent/go/internal/execution/execution.goagent/go/internal/flags/flags.goagent/go/internal/flags/flags_test.goagent/go/internal/history/history.goagent/go/internal/history/history_test.goagent/go/internal/history/mock/Store.goagent/go/internal/hostfs/copy.goagent/go/internal/hostfs/copy_test.goagent/go/internal/hostfs/hostfs.goagent/go/internal/hostfs/hostfs_test.goagent/go/internal/interrupts/mock/Interrupt.goagent/go/internal/step/mock/Step.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
💤 Files with no reviewable changes (2)
- agent/go/internal/history/history_test.go
- agent/go/internal/history/history.go
5a86acf to
82de1d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/deps.mk`:
- Around line 58-60: Update the addlicense target to validate that ADDLICENSE is
an executable installed binary and that its reported version matches
ADDLICENSE_VERSION; reinstall it via the existing go install command whenever
validation fails, including for stale or incorrect versions.
In `@agent/go/internal/agent/agent_test.go`:
- Around line 488-501: Update writeCancellationPackageFixture so the shell
script written for the apply step replaces the infinite no-op busy loop with
sleep 3600, while preserving the existing marker-file behavior and script
permissions.
In `@agent/go/internal/agent/package_test.go`:
- Around line 67-78: Add unit-test coverage in the existing package tests for
the remaining branches of prepareHost and ensurePackageData: reject an
ExpectedConfigFiles path escaping the configmaps directory, reject an expected
config path that is not a regular file, and cover ensurePackageData with an
existing non-directory copyRoot, a missing dataDir, and legacy node-files
copying. Use the existing Ginkgo/Gomega helpers and assert the relevant errors
and filesystem outcomes.
In `@agent/go/internal/agent/package.go`:
- Line 41: Align the path argument order between ensurePackageData and
prepareHost by using the same leading-parameter order in both functions, then
update the prepareHost call in agent.go and its specifications in
package_test.go to match. Keep the resulting order consistent so copyRoot and
rootMount cannot be accidentally swapped.
In `@agent/go/internal/agent/steps_test.go`:
- Around line 252-278: Update the test around runSteps to replace the discarded
runtime.stdout writer with a buffer, then assert that the captured stdout
contains the same output written to the retained log file. Preserve the existing
success and log-directory assertions, ensuring the test verifies both
destinations when runtime.writeLogs is enabled.
In `@agent/go/internal/config/validate_test.go`:
- Around line 173-178: Update the test setup around Loader.Load and the
validator mock to use malformed JSON instead of validConfigJSON for both the
Validate expectation and the load input. Keep the sentinel validation error and
Once expectation, ensuring the test fails if decoding occurs before
SchemaValidator.Validate.
In `@agent/go/internal/flags/flags.go`:
- Around line 159-164: Restore atomic publication for the state-file write in
the flags flow by writing through a temporary file under the same root and
renaming it into place only after a successful write, using a rooted
atomic-replace helper shared by flags and control files. Update the relevant
marker-writing path around hostfs.WriteFile, and add a failure-path test proving
a failed marker write leaves no completion flag that causes a later Check to
skip the step.
In `@agent/go/internal/flags/layout.go`:
- Around line 155-162: The collision allocation in CreateLogFile must preserve
suffix ordering when lower suffixes were deleted: avoid reusing a lower suffix
while a higher suffix for the same timestamp remains, and update CleanupOldLogs
in logs.go so retention does not rely solely on the reusable suffix to determine
same-timestamp age. Add a regression test covering cleanup, creating another log
at the same timestamp, and verifying the new log is retained. Update
agent/go/internal/flags/layout.go lines 155-162 and
agent/go/internal/flags/logs.go lines 82-90; add the test in the appropriate
affected test file.
In `@agent/go/internal/history/history.go`:
- Around line 157-160: Replace the direct hostfs.WriteFile call in the history
ledger save method with a rooted atomic-write operation that writes and syncs a
temporary regular file, renames it only after successful completion, and syncs
the containing directory. Preserve the existing error wrapping and add a
failure-path test verifying the prior ledger remains readable when replacement
fails.
In `@agent/go/Makefile`:
- Around line 54-56: Update the license validation loop in license-header-check
to remove the generated-source exemption based on the “Code generated ... DO NOT
EDIT.” marker. Ensure every file from license_files is checked for the
SPDX-License-Identifier: Apache-2.0 header.
🪄 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: 5ef277fc-96ce-49b5-8f87-b9b605500d8c
⛔ Files ignored due to path filters (1)
agent/go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
agent/README.mdagent/go/.mockery.yamlagent/go/Makefileagent/go/cmd/agent/main.goagent/go/deps.mkagent/go/go.modagent/go/internal/agent/agent.goagent/go/internal/agent/agent_test.goagent/go/internal/agent/interrupt.goagent/go/internal/agent/interrupt_test.goagent/go/internal/agent/package.goagent/go/internal/agent/package_test.goagent/go/internal/agent/steps.goagent/go/internal/agent/steps_test.goagent/go/internal/config/mock/SchemaValidator.goagent/go/internal/config/validate_test.goagent/go/internal/execution/execution.goagent/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/history/mock/Store.goagent/go/internal/hostfs/copy.goagent/go/internal/hostfs/copy_test.goagent/go/internal/hostfs/hostfs.goagent/go/internal/hostfs/hostfs_test.goagent/go/internal/interrupts/mock/Interrupt.goagent/go/internal/step/mock/Step.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
| .PHONY: addlicense | ||
| addlicense: $(LOCALBIN) ## Download addlicense locally if necessary. | ||
| test -s $(ADDLICENSE) || GOBIN=$(LOCALBIN) go install github.com/google/addlicense@$(ADDLICENSE_VERSION) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '23,24p;52,60p' agent/go/deps.mk
fd -HI -t f '^addlicense$' . -E .git | while IFS= read -r binary; do
if test -x "$binary"; then
printf 'executable: %s\n' "$binary"
else
printf 'not executable: %s\n' "$binary"
fi
doneRepository: NVIDIA/nodewright
Length of output: 648
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- deps.mk context ---'
cat -n agent/go/deps.mk | sed -n '1,90p'
printf '%s\n' '--- addlicense references ---'
rg -n --glob '!**/.git/**' '\b(addlicense|ADDLICENSE|ADDLICENSE_VERSION)\b' agent/go Makefile .github 2>/dev/null || true
printf '%s\n' '--- local tool definitions ---'
rg -n --glob '!**/.git/**' 'LOCALBIN|MOCKERY_VERSION|ADDLICENSE_VERSION' . | head -120Repository: NVIDIA/nodewright
Length of output: 11534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
import stat
deps = Path("agent/go/deps.mk").read_text()
makefile = Path("agent/go/Makefile").read_text()
target = re.search(
r"^addlicense:.*\n(?P<recipe>\s+test -s \$\(ADDLICENSE\).*?)$",
deps,
re.MULTILINE,
).group("recipe")
assert "test -s $(ADDLICENSE)" in target
assert "ADDLICENSE_VERSION" in target
assert "test -x" not in target
assert "version" not in target
license_use = re.search(r"^\s*\$\(license_files\).*?\$\(ADDLICENSE\)", makefile, re.MULTILINE)
assert license_use
candidate = Path("agent/go/Makefile")
print(f"nonempty_candidate={candidate.stat().st_size > 0}")
print(f"executable_candidate=bool(mode & stat.S_IXUSR)={bool(candidate.stat().st_mode & stat.S_IXUSR)}")
print("addlicense_target_accepts_nonempty_nonexecutable=true")
print("addlicense_target_checks_declared_version=false")
print("license_targets_execute_ADDLICENSE=true")
PYRepository: NVIDIA/nodewright
Length of output: 374
Validate the installed addlicense binary.
test -s $(ADDLICENSE) accepts any nonempty path, including non-executable files. The license targets then fail when they execute it. The check also accepts stale versions and bypasses ADDLICENSE_VERSION. Require an executable binary that matches the declared version, or reinstall it.
🤖 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/deps.mk` around lines 58 - 60, Update the addlicense target to
validate that ADDLICENSE is an executable installed binary and that its reported
version matches ADDLICENSE_VERSION; reinstall it via the existing go install
command whenever validation fails, including for stale or incorrect versions.
| rootOverlayDirName = "root_dir" | ||
| ) | ||
|
|
||
| func ensurePackageData(rootMount, copyRoot, dataDir string) error { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Align the path parameter order across the package preparation functions.
ensurePackageData takes (rootMount, copyRoot, dataDir). prepareHost takes (copyRoot, rootMount, cfg). Both leading parameters are string, so the compiler cannot detect a swapped call. A swapped call to prepareHost would copy the host root overlay into the package directory instead of onto the host. Use the same order in both functions.
♻️ Proposed change
-func prepareHost(copyRoot, rootMount string, cfg config.Config) error {
+func prepareHost(rootMount, copyRoot string, cfg config.Config) error {Update the caller in agent/go/internal/agent/agent.go and the spec in agent/go/internal/agent/package_test.go at Line 67 and Line 72 to match.
Also applies to: 79-79
🤖 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/agent/package.go` at line 41, Align the path argument order
between ensurePackageData and prepareHost by using the same leading-parameter
order in both functions, then update the prepareHost call in agent.go and its
specifications in package_test.go to match. Keep the resulting order consistent
so copyRoot and rootMount cannot be accidentally swapped.
| It("composes command output with a retained log file", func() { | ||
| runtime.writeLogs = true | ||
| output := "step output\n" | ||
| value := newMockStep("apply.sh") | ||
| value.EXPECT(). | ||
| Run(mock.Anything, mock.Anything). | ||
| Run(func(_ context.Context, cfg execution.Config) { | ||
| _, err := io.WriteString(cfg.Stdout(), output) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }). | ||
| Return(execution.StatusSuccess, nil). | ||
| Once() | ||
| cfg.Modes[stage.Apply] = []step.Step{value} | ||
|
|
||
| status, err := runSteps( | ||
| context.Background(), req, runtime, layout, cfg, flagStore, historyStore, | ||
| ) | ||
|
|
||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(status).To(Equal(execution.StatusSuccess)) | ||
| entries, err := os.ReadDir(filepath.Join(layout.LogDir(), cfg.PackageName, cfg.PackageVersion)) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(entries).To(HaveLen(1)) | ||
| data, err := os.ReadFile(filepath.Join(layout.LogDir(), cfg.PackageName, cfg.PackageVersion, entries[0].Name())) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(string(data)).To(Equal(output)) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Also assert that step output reaches runtime.stdout.
This spec sets runtime.stdout to io.Discard, so it verifies only the retained log file. The contract requires both behaviors: output streams to stdout and stderr, and, when retention is enabled, output is also written under the log directory. Replace io.Discard with a buffer and assert the same content in both destinations.
As per coding guidelines, "step and interrupt output must stream to stdout/stderr and, when enabled, also be written under SKYHOOK_LOG_DIR."
♻️ Proposed change
It("composes command output with a retained log file", func() {
runtime.writeLogs = true
+ streamed := &bytes.Buffer{}
+ runtime.stdout = streamed
output := "step output\n" Expect(err).NotTo(HaveOccurred())
Expect(string(data)).To(Equal(output))
+ Expect(streamed.String()).To(Equal(output))
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| It("composes command output with a retained log file", func() { | |
| runtime.writeLogs = true | |
| output := "step output\n" | |
| value := newMockStep("apply.sh") | |
| value.EXPECT(). | |
| Run(mock.Anything, mock.Anything). | |
| Run(func(_ context.Context, cfg execution.Config) { | |
| _, err := io.WriteString(cfg.Stdout(), output) | |
| Expect(err).NotTo(HaveOccurred()) | |
| }). | |
| Return(execution.StatusSuccess, nil). | |
| Once() | |
| cfg.Modes[stage.Apply] = []step.Step{value} | |
| status, err := runSteps( | |
| context.Background(), req, runtime, layout, cfg, flagStore, historyStore, | |
| ) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(status).To(Equal(execution.StatusSuccess)) | |
| entries, err := os.ReadDir(filepath.Join(layout.LogDir(), cfg.PackageName, cfg.PackageVersion)) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(entries).To(HaveLen(1)) | |
| data, err := os.ReadFile(filepath.Join(layout.LogDir(), cfg.PackageName, cfg.PackageVersion, entries[0].Name())) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(string(data)).To(Equal(output)) | |
| }) | |
| It("composes command output with a retained log file", func() { | |
| runtime.writeLogs = true | |
| streamed := &bytes.Buffer{} | |
| runtime.stdout = streamed | |
| output := "step output\n" | |
| value := newMockStep("apply.sh") | |
| value.EXPECT(). | |
| Run(mock.Anything, mock.Anything). | |
| Run(func(_ context.Context, cfg execution.Config) { | |
| _, err := io.WriteString(cfg.Stdout(), output) | |
| Expect(err).NotTo(HaveOccurred()) | |
| }). | |
| Return(execution.StatusSuccess, nil). | |
| Once() | |
| cfg.Modes[stage.Apply] = []step.Step{value} | |
| status, err := runSteps( | |
| context.Background(), req, runtime, layout, cfg, flagStore, historyStore, | |
| ) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(status).To(Equal(execution.StatusSuccess)) | |
| entries, err := os.ReadDir(filepath.Join(layout.LogDir(), cfg.PackageName, cfg.PackageVersion)) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(entries).To(HaveLen(1)) | |
| data, err := os.ReadFile(filepath.Join(layout.LogDir(), cfg.PackageName, cfg.PackageVersion, entries[0].Name())) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(string(data)).To(Equal(output)) | |
| Expect(streamed.String()).To(Equal(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/agent/steps_test.go` around lines 252 - 278, Update the
test around runSteps to replace the discarded runtime.stdout writer with a
buffer, then assert that the captured stdout contains the same output written to
the retained log file. Preserve the existing success and log-directory
assertions, ensuring the test verifies both destinations when runtime.writeLogs
is enabled.
Source: Coding guidelines
| validator := configmock.NewMockSchemaValidator(GinkgoT()) | ||
| validator.EXPECT(). | ||
| Validate([]byte(validConfigJSON), schema.V1). | ||
| Return(sentinel). | ||
| Once() | ||
| loader := &Loader{validator: validator} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use malformed input to verify validation ordering.
At Line 175, validConfigJSON parses successfully. If Loader.Load decodes before calling Validate, the mock still returns sentinel, so this test passes. Use malformed JSON in both calls to prove that validation occurs before decoding.
Proposed test update
It("surfaces the injected validator's error without parsing the document", func() {
+ input := []byte("{")
sentinel := errors.New("boom from fake validator")
validator := configmock.NewMockSchemaValidator(GinkgoT())
validator.EXPECT().
- Validate([]byte(validConfigJSON), schema.V1).
+ Validate(input, schema.V1).
Return(sentinel).
Once()
loader := &Loader{validator: validator}
- _, err := loader.Load([]byte(validConfigJSON), GinkgoT().TempDir(), nil)
+ _, err := loader.Load(input, GinkgoT().TempDir(), nil)
Expect(err).To(MatchError(sentinel))
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| validator := configmock.NewMockSchemaValidator(GinkgoT()) | |
| validator.EXPECT(). | |
| Validate([]byte(validConfigJSON), schema.V1). | |
| Return(sentinel). | |
| Once() | |
| loader := &Loader{validator: validator} | |
| It("surfaces the injected validator's error without parsing the document", func() { | |
| input := []byte("{") | |
| sentinel := errors.New("boom from fake validator") | |
| validator := configmock.NewMockSchemaValidator(GinkgoT()) | |
| validator.EXPECT(). | |
| Validate(input, schema.V1). | |
| Return(sentinel). | |
| Once() | |
| loader := &Loader{validator: validator} | |
| _, err := loader.Load(input, GinkgoT().TempDir(), nil) | |
| Expect(err).To(MatchError(sentinel)) | |
| }) |
🤖 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/config/validate_test.go` around lines 173 - 178, Update the
test setup around Loader.Load and the validator mock to use malformed JSON
instead of validConfigJSON for both the Validate expectation and the load input.
Keep the sentinel validation error and Once expectation, ensuring the test fails
if decoding occurs before SchemaValidator.Validate.
| @wrong=$$($(license_files) | while read -r f; do \ | ||
| grep -qE '^.{1,2} Code generated .* DO NOT EDIT\.$$' "$$f" && continue; \ | ||
| head -20 "$$f" | grep -q 'SPDX-License-Identifier: Apache-2.0' || echo " $$f"; \ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- agent/go/Makefile ---'
cat -n agent/go/Makefile | sed -n '1,90p'
printf '%s\n' '--- related license checks ---'
rg -n -C 4 'license-header-check|license_files|addlicense|Code generated|SPDX-License-Identifier' agent/go Makefile .github 2>/dev/null | sed -n '1,240p'
printf '%s\n' '--- source files considered by the Makefile ---'
rg -n '^[[:space:]]*(license_files|GO_FILES|.*FILES).*=' agent/go/MakefileRepository: NVIDIA/nodewright
Length of output: 20676
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import subprocess, re
files = subprocess.check_output(
["git", "ls-files", "--", "*.go", "*.py", "*.sh", "*.yaml", "*.yml",
"Dockerfile", "*.Dockerfile"], text=True
).splitlines()
generated = []
missing_apache = []
for name in files:
text = Path(name).read_text(errors="replace")
first20 = "\n".join(text.splitlines()[:20])
is_generated = any(
re.search(r"^.{1,2} Code generated .* DO NOT EDIT\.$", line)
for line in text.splitlines()
)
has_apache = "SPDX-License-Identifier: Apache-2.0" in first20
if is_generated:
generated.append((name, has_apache))
if not has_apache:
missing_apache.append((name, is_generated))
print(f"tracked license candidates: {len(files)}")
print(f"generated candidates: {len(generated)}")
for name, has_apache in generated:
print(f"generated {'Apache-2.0' if has_apache else 'NON-Apache/missing'}: {name}")
print(f"missing Apache in first 20 lines: {len(missing_apache)}")
for name, is_generated in missing_apache:
print(f"missing {'generated' if is_generated else 'non-generated'}: {name}")
PY
printf '%s\n' '--- repository policy references ---'
rg -n -i -C 3 'generated.*(license|SPDX)|SPDX.*generated|license.*generated|Apache-2\.0.*header|license header' \
README.md CONTRIBUTING.md LICENSE scripts agent .github 2>/dev/null | sed -n '1,240p'
printf '%s\n' '--- workflow invocation context ---'
rg -n -C 4 'license-header-check|make .*lint|working-directory:.*agent/go|cd agent/go' .github agent 2>/dev/null | sed -n '1,240p'Repository: NVIDIA/nodewright
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact license_files output from repository root ---'
git ls-files -- '*.go' '*.py' '*.sh' '*.yaml' '*.yml' 'Dockerfile' '*.Dockerfile' |
awk 'BEGIN {n=0} {n++; if ($0 ~ /^agent\/go\//) agent++; if ($0 ~ /(^|\/)vendor\//) vendor++} END {printf "total=%d agent/go=%d vendor=%d\n", n, agent, vendor}'
printf '%s\n' '--- exact license_files output from agent/go ---'
(
cd agent/go
git ls-files -- '*.go' '*.py' '*.sh' '*.yaml' '*.yml' 'Dockerfile' '*.Dockerfile' |
awk 'BEGIN {n=0} {n++; if ($0 ~ /^agent\/go\//) agent++; if ($0 ~ /(^|\/)vendor\//) vendor++} END {printf "total=%d agent/go=%d vendor=%d\n", n, agent, vendor}'
)
printf '%s\n' '--- generated files in agent/go candidates ---'
python3 - <<'PY'
from pathlib import Path
import subprocess, re
for cwd in (Path('.'), Path('agent/go')):
files = subprocess.check_output(
["git", "-C", str(cwd), "ls-files", "--", "*.go", "*.py", "*.sh",
"*.yaml", "*.yml", "Dockerfile", "*.Dockerfile"], text=True
).splitlines()
generated = []
missing = []
for name in files:
path = cwd / name
if not path.is_file():
continue
lines = path.read_text(errors="replace").splitlines()
is_generated = any(re.search(r"^.{1,2} Code generated .* DO NOT EDIT\.$", line)
for line in lines)
has_apache = "SPDX-License-Identifier: Apache-2.0" in "\n".join(lines[:20])
if is_generated:
generated.append((name, has_apache))
if not has_apache:
missing.append(name)
print(f"cwd={cwd}: candidates={len(files)}, generated={len(generated)}, generated_missing_apache={len(missing)}")
print(" examples:", ", ".join(f"{n} ({'Apache' if a else 'missing'})" for n,a in generated[:8]))
print(" missing examples:", ", ".join(missing[:8]))
PY
printf '%s\n' '--- workflow context only ---'
rg -n -C 5 'license-header-check|make .*lint|working-directory:.*agent/go|cd agent/go' .github agent 2>/dev/null |
grep -v -E 'vendor|generated' | sed -n '1,180p'Repository: NVIDIA/nodewright
Length of output: 5054
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- dependency version and license template ---'
cat -n agent/go/deps.mk | sed -n '1,42p'
cat -n scripts/license-header.tmpl | sed -n '1,80p'
printf '%s\n' '--- all generated agent/go files and first header lines ---'
python3 - <<'PY'
from pathlib import Path
import subprocess, re
files = subprocess.check_output(
["git", "ls-files", "--", "*.go", "*.py", "*.sh", "*.yaml", "*.yml",
"Dockerfile", "*.Dockerfile"], cwd="agent/go", text=True
).splitlines()
for name in files:
path = Path("agent/go") / name
lines = path.read_text(errors="replace").splitlines()
if any(re.search(r"^.{1,2} Code generated .* DO NOT EDIT\.$", line)
for line in lines):
print(f"{name}:")
for i, line in enumerate(lines[:5], 1):
print(f" {i}: {line}")
PY
printf '%s\n' '--- license policy references excluding vendored files ---'
rg -n -i -C 3 'generated.*(license|SPDX)|SPDX.*generated|license.*generated|Apache-2\.0.*header|license header' \
README.md CONTRIBUTING.md LICENSE scripts agent/go .github 2>/dev/null |
grep -v '/vendor/' | sed -n '1,220p'Repository: NVIDIA/nodewright
Length of output: 8406
🌐 Web query:
Google addlicense -check accepts any license marker SPDX behavior
💡 Result:
The Google addlicense tool does not automatically accept any arbitrary SPDX license identifier provided by a user [1][2]. The tool manages license templates through a internal mapping system [1][3]. Its behavior regarding SPDX identifiers is as follows: 1. Predefined Licenses: It has built-in support for specific licenses (Apache-2.0, MIT, BSD, MPL-2.0) [1][3]. When the -s flag is used, it appends an SPDX identifier to these recognized licenses, or uses a dedicated SPDX-only template if -s=only is specified [1][2]. 2. Unknown Licenses: If a user specifies a license type that is not in the internal licenseTemplate map, the tool will return an error (e.g., unknown license: "...") unless the -s (or -s=only) flag is used [1][2]. 3. SPDX-Only Mode: If the -s=only flag is enabled, the tool will use a generic SPDX template (tmplSPDX) even if the provided license type is unknown [1][2]. In this mode, the tool essentially treats the provided license argument as a string to be used for the SPDXID field [1][3]. Essentially, the tool does not validate that a provided string is a valid SPDX identifier against an external database; it simply trusts the string provided to the license argument or, when using -s=only with an unknown license, uses that argument as the identifier in the generated header [1][3]. Top results: [1][3][2][4]
Citations:
- 1: https://github.com/google/addlicense/blob/master/tmpl.go
- 2: https://github.com/google/addlicense/blob/499ed7f2/tmpl_test.go
- 3: https://deepwiki.com/google/addlicense/3.1-license-template-system
- 4: https://pkg.go.dev/github.com/google/addlicense
🌐 Web query:
github.com/google/addlicense v1.2.0 check flag source license marker check
💡 Result:
In the google/addlicense tool (version 1.2.0), the -check flag enables a check-only mode where the tool verifies the presence of license headers in source files [1][2]. When this flag is used, the tool does not modify any files; instead, it exits with a non-zero status code if it detects any files that are missing a valid license header [1][3]. The tool identifies whether a file already contains a license header through several methods: 1. Keyword Detection: It scans for terms such as "copyright," "mozilla public," or "spdx-license-identifier" [3]. 2. Header Analysis: It checks for existing license text and specifically validates the copyright holder if the -c flag is provided [4]. A recent update ensures that when -c is specified, the tool verifies that the copyright holder in the file matches the expected value, rather than merely checking for the presence of any license header [4]. 3. Special Handling: The tool is designed to preserve specific file structures, such as shebang lines, and it automatically ignores empty files during its checks [4][3]. In version 1.2.0, the tool continues to support recursive scanning of directories and can be configured with various flags—such as -ignore for file exclusion, -s for SPDX identifiers, and -l for license types—to refine how these headers are detected and verified [1][5][3].
Citations:
- 1: https://github.com/google/addlicense
- 2: https://pkg.go.dev/github.com/google/addlicense
- 3: https://deepwiki.com/google/addlicense
- 4: fix: ensure --check validates copyright holder with -c google/addlicense#172
- 5: https://github.com/google/addlicense/blob/499ed7f2/README.md
Keep generated source in Apache SPDX validation.
addlicense -check detects a generic license header, not specifically SPDX-License-Identifier: Apache-2.0. Remove the generated-source exemption so license-header-check enforces its stated Apache-2.0 requirement.
🤖 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/Makefile` around lines 54 - 56, Update the license validation loop
in license-header-check to remove the generated-source exemption based on the
“Code generated ... DO NOT EDIT.” marker. Ensure every file from license_files
is checked for the SPDX-License-Identifier: Apache-2.0 header.
Source: Coding guidelines
82de1d8 to
9a4aa8c
Compare
rice-riley
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
agent/go/internal/agent/agent_test.go (1)
488-501: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the busy loop with
sleep 3600.The
applyfixture script spins withwhile :; do :; done. The spec keeps this process alive for up to five seconds, so it consumes a full CPU core during the run.sleep 3600produces the same blocking behavior without the CPU cost, and the marker file assertions stay valid.♻️ Proposed change
- []byte("#!/bin/sh\n: > \"$NODEWRIGHT_AGENT_TEST_MARKER_DIR/first-started\"\nwhile :; do :; done\n: > \"$NODEWRIGHT_AGENT_TEST_MARKER_DIR/first-finished\"\n"), + []byte("#!/bin/sh\n: > \"$NODEWRIGHT_AGENT_TEST_MARKER_DIR/first-started\"\nsleep 3600\n: > \"$NODEWRIGHT_AGENT_TEST_MARKER_DIR/first-finished\"\n"),🤖 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/agent/agent_test.go` around lines 488 - 501, Update the apply script created by writeCancellationPackageFixture to replace the CPU-intensive infinite while loop with sleep 3600, preserving the existing marker creation and blocking behavior.agent/go/internal/agent/package.go (1)
41-41: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAlign the path parameter order between
ensurePackageDataandprepareHost.
ensurePackageDatatakes(rootMount, copyRoot, dataDir).prepareHosttakes(copyRoot, rootMount, cfg). Both leading parameters arestring, so a swapped call compiles. Use the same order in both functions and update the caller inagent/go/internal/agent/agent.goat Line 366 and the specs inagent/go/internal/agent/package_test.goat Lines 67 and 72.Also applies to: 79-79
🤖 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/agent/package.go` at line 41, Align the parameter order of ensurePackageData with prepareHost by using copyRoot before rootMount, then update the call in prepareHost’s caller and the affected package tests to pass arguments in that order. Preserve the existing behavior and parameter names while ensuring all call sites match the unified order.agent/go/internal/agent/steps_test.go (1)
252-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert that step output reaches
runtime.stdout.The spec sets
runtime.stdouttoio.Discard, so it verifies only the retained log file. The contract requires both destinations: output streams to stdout and stderr, and, when retention is enabled, output is also written under the log directory. Use a buffer and assert the same content in both places.💚 Proposed change
It("composes command output with a retained log file", func() { runtime.writeLogs = true + streamed := &bytes.Buffer{} + runtime.stdout = streamed output := "step output\n"Expect(err).NotTo(HaveOccurred()) Expect(string(data)).To(Equal(output)) + Expect(streamed.String()).To(Equal(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/agent/steps_test.go` around lines 252 - 278, Update the “composes command output with a retained log file” test to replace the discarded runtime stdout with a buffer, then assert that the buffer contains the same step output as the retained log file. Preserve the existing log-directory assertions and use the test’s runtime stdout configuration to verify both destinations.agent/go/internal/agent/package_test.go (1)
51-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the remaining
prepareHostbranches and forensurePackageData.This spec covers the overlay copy, one present expected-config file, and one missing expected-config file. Three branches of
agent/go/internal/agent/package.gostay untested:
- Line 88:
prepareHostrejects a non-localExpectedConfigFilesentry, such as../escape.conf. This is a containment check on package-supplied input.- Line 99:
prepareHostrejects an expected config file that is not a regular file.- Lines 41-64:
ensurePackageDatais not exercised. The relevant cases are a non-directorycopyRoot, a missingdataDir, and the legacy node-files copy.Based on learnings, code changes should include unit tests, followed by running tests and formatting before submitting a merge request.
🤖 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/agent/package_test.go` around lines 51 - 78, Extend the package test coverage around prepareHost and ensurePackageData. Add cases verifying prepareHost rejects path-traversing ExpectedConfigFiles entries such as ../escape.conf and rejects expected config paths that are not regular files; add ensurePackageData cases for a non-directory copyRoot, a missing dataDir, and copying legacy node files. Use the existing test symbols and assert each expected error or resulting filesystem state, then run formatting and tests.Source: Learnings
🤖 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/history/history.go`:
- Around line 35-39: Update the Go agent’s runSteps flow to set
OVERLAY_FRAMEWORK_VERSION to the required {package name}-{version} value before
executing steps, using the existing previous/current version data passed by
runSteps. Ensure the environment variable is available to history management
without changing the existing version arguments or history constants.
In `@agent/go/internal/step/upgrade_step.go`:
- Around line 104-107: Update UpgradeStep.Fingerprint to include a step-type
discriminator in the value passed to stepFingerprint, matching the corresponding
RegularStep fingerprint behavior while preserving all existing execution inputs.
Add a test proving identical inputs for RegularStep and UpgradeStep produce
different fingerprints and cannot share the same idempotency marker.
---
Duplicate comments:
In `@agent/go/internal/agent/agent_test.go`:
- Around line 488-501: Update the apply script created by
writeCancellationPackageFixture to replace the CPU-intensive infinite while loop
with sleep 3600, preserving the existing marker creation and blocking behavior.
In `@agent/go/internal/agent/package_test.go`:
- Around line 51-78: Extend the package test coverage around prepareHost and
ensurePackageData. Add cases verifying prepareHost rejects path-traversing
ExpectedConfigFiles entries such as ../escape.conf and rejects expected config
paths that are not regular files; add ensurePackageData cases for a
non-directory copyRoot, a missing dataDir, and copying legacy node files. Use
the existing test symbols and assert each expected error or resulting filesystem
state, then run formatting and tests.
In `@agent/go/internal/agent/package.go`:
- Line 41: Align the parameter order of ensurePackageData with prepareHost by
using copyRoot before rootMount, then update the call in prepareHost’s caller
and the affected package tests to pass arguments in that order. Preserve the
existing behavior and parameter names while ensuring all call sites match the
unified order.
In `@agent/go/internal/agent/steps_test.go`:
- Around line 252-278: Update the “composes command output with a retained log
file” test to replace the discarded runtime stdout with a buffer, then assert
that the buffer contains the same step output as the retained log file. Preserve
the existing log-directory assertions and use the test’s runtime stdout
configuration to verify both destinations.
🪄 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: 0a2a2490-6208-4590-82c3-9fae9ec5eb1f
⛔ Files ignored due to path filters (1)
agent/go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
agent/README.mdagent/go/.mockery.yamlagent/go/Makefileagent/go/cmd/agent/main.goagent/go/deps.mkagent/go/go.modagent/go/internal/agent/agent.goagent/go/internal/agent/agent_test.goagent/go/internal/agent/interrupt.goagent/go/internal/agent/interrupt_test.goagent/go/internal/agent/package.goagent/go/internal/agent/package_test.goagent/go/internal/agent/steps.goagent/go/internal/agent/steps_test.goagent/go/internal/config/mock/SchemaValidator.goagent/go/internal/config/validate_test.goagent/go/internal/execution/execution.goagent/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/history/mock/Store.goagent/go/internal/hostfs/copy.goagent/go/internal/hostfs/copy_test.goagent/go/internal/hostfs/hostfs.goagent/go/internal/hostfs/hostfs_test.goagent/go/internal/interrupts/mock/Interrupt.goagent/go/internal/step/mock/Step.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
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (4)
agent/go/internal/agent/agent_test.go (1)
488-501: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the busy loop with
sleep 3600.The
applyfixture script spins withwhile :; do :; done. The spec keeps this process alive for up to five seconds, so it consumes a full CPU core during the run.sleep 3600produces the same blocking behavior without the CPU cost, and the marker file assertions stay valid.♻️ Proposed change
- []byte("#!/bin/sh\n: > \"$NODEWRIGHT_AGENT_TEST_MARKER_DIR/first-started\"\nwhile :; do :; done\n: > \"$NODEWRIGHT_AGENT_TEST_MARKER_DIR/first-finished\"\n"), + []byte("#!/bin/sh\n: > \"$NODEWRIGHT_AGENT_TEST_MARKER_DIR/first-started\"\nsleep 3600\n: > \"$NODEWRIGHT_AGENT_TEST_MARKER_DIR/first-finished\"\n"),🤖 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/agent/agent_test.go` around lines 488 - 501, Update the apply script created by writeCancellationPackageFixture to replace the CPU-intensive infinite while loop with sleep 3600, preserving the existing marker creation and blocking behavior.agent/go/internal/agent/package.go (1)
41-41: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAlign the path parameter order between
ensurePackageDataandprepareHost.
ensurePackageDatatakes(rootMount, copyRoot, dataDir).prepareHosttakes(copyRoot, rootMount, cfg). Both leading parameters arestring, so a swapped call compiles. Use the same order in both functions and update the caller inagent/go/internal/agent/agent.goat Line 366 and the specs inagent/go/internal/agent/package_test.goat Lines 67 and 72.Also applies to: 79-79
🤖 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/agent/package.go` at line 41, Align the parameter order of ensurePackageData with prepareHost by using copyRoot before rootMount, then update the call in prepareHost’s caller and the affected package tests to pass arguments in that order. Preserve the existing behavior and parameter names while ensuring all call sites match the unified order.agent/go/internal/agent/steps_test.go (1)
252-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert that step output reaches
runtime.stdout.The spec sets
runtime.stdouttoio.Discard, so it verifies only the retained log file. The contract requires both destinations: output streams to stdout and stderr, and, when retention is enabled, output is also written under the log directory. Use a buffer and assert the same content in both places.💚 Proposed change
It("composes command output with a retained log file", func() { runtime.writeLogs = true + streamed := &bytes.Buffer{} + runtime.stdout = streamed output := "step output\n"Expect(err).NotTo(HaveOccurred()) Expect(string(data)).To(Equal(output)) + Expect(streamed.String()).To(Equal(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/agent/steps_test.go` around lines 252 - 278, Update the “composes command output with a retained log file” test to replace the discarded runtime stdout with a buffer, then assert that the buffer contains the same step output as the retained log file. Preserve the existing log-directory assertions and use the test’s runtime stdout configuration to verify both destinations.agent/go/internal/agent/package_test.go (1)
51-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the remaining
prepareHostbranches and forensurePackageData.This spec covers the overlay copy, one present expected-config file, and one missing expected-config file. Three branches of
agent/go/internal/agent/package.gostay untested:
- Line 88:
prepareHostrejects a non-localExpectedConfigFilesentry, such as../escape.conf. This is a containment check on package-supplied input.- Line 99:
prepareHostrejects an expected config file that is not a regular file.- Lines 41-64:
ensurePackageDatais not exercised. The relevant cases are a non-directorycopyRoot, a missingdataDir, and the legacy node-files copy.Based on learnings, code changes should include unit tests, followed by running tests and formatting before submitting a merge request.
🤖 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/agent/package_test.go` around lines 51 - 78, Extend the package test coverage around prepareHost and ensurePackageData. Add cases verifying prepareHost rejects path-traversing ExpectedConfigFiles entries such as ../escape.conf and rejects expected config paths that are not regular files; add ensurePackageData cases for a non-directory copyRoot, a missing dataDir, and copying legacy node files. Use the existing test symbols and assert each expected error or resulting filesystem state, then run formatting and tests.Source: Learnings
🤖 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/history/history.go`:
- Around line 35-39: Update the Go agent’s runSteps flow to set
OVERLAY_FRAMEWORK_VERSION to the required {package name}-{version} value before
executing steps, using the existing previous/current version data passed by
runSteps. Ensure the environment variable is available to history management
without changing the existing version arguments or history constants.
In `@agent/go/internal/step/upgrade_step.go`:
- Around line 104-107: Update UpgradeStep.Fingerprint to include a step-type
discriminator in the value passed to stepFingerprint, matching the corresponding
RegularStep fingerprint behavior while preserving all existing execution inputs.
Add a test proving identical inputs for RegularStep and UpgradeStep produce
different fingerprints and cannot share the same idempotency marker.
---
Duplicate comments:
In `@agent/go/internal/agent/agent_test.go`:
- Around line 488-501: Update the apply script created by
writeCancellationPackageFixture to replace the CPU-intensive infinite while loop
with sleep 3600, preserving the existing marker creation and blocking behavior.
In `@agent/go/internal/agent/package_test.go`:
- Around line 51-78: Extend the package test coverage around prepareHost and
ensurePackageData. Add cases verifying prepareHost rejects path-traversing
ExpectedConfigFiles entries such as ../escape.conf and rejects expected config
paths that are not regular files; add ensurePackageData cases for a
non-directory copyRoot, a missing dataDir, and copying legacy node files. Use
the existing test symbols and assert each expected error or resulting filesystem
state, then run formatting and tests.
In `@agent/go/internal/agent/package.go`:
- Line 41: Align the parameter order of ensurePackageData with prepareHost by
using copyRoot before rootMount, then update the call in prepareHost’s caller
and the affected package tests to pass arguments in that order. Preserve the
existing behavior and parameter names while ensuring all call sites match the
unified order.
In `@agent/go/internal/agent/steps_test.go`:
- Around line 252-278: Update the “composes command output with a retained log
file” test to replace the discarded runtime stdout with a buffer, then assert
that the buffer contains the same step output as the retained log file. Preserve
the existing log-directory assertions and use the test’s runtime stdout
configuration to verify both destinations.
🪄 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: 0a2a2490-6208-4590-82c3-9fae9ec5eb1f
⛔ Files ignored due to path filters (1)
agent/go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
agent/README.mdagent/go/.mockery.yamlagent/go/Makefileagent/go/cmd/agent/main.goagent/go/deps.mkagent/go/go.modagent/go/internal/agent/agent.goagent/go/internal/agent/agent_test.goagent/go/internal/agent/interrupt.goagent/go/internal/agent/interrupt_test.goagent/go/internal/agent/package.goagent/go/internal/agent/package_test.goagent/go/internal/agent/steps.goagent/go/internal/agent/steps_test.goagent/go/internal/config/mock/SchemaValidator.goagent/go/internal/config/validate_test.goagent/go/internal/execution/execution.goagent/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/history/mock/Store.goagent/go/internal/hostfs/copy.goagent/go/internal/hostfs/copy_test.goagent/go/internal/hostfs/hostfs.goagent/go/internal/hostfs/hostfs_test.goagent/go/internal/interrupts/mock/Interrupt.goagent/go/internal/step/mock/Step.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
🛑 Comments failed to post (2)
agent/go/internal/history/history.go (1)
35-39: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Find remaining producers of the version environment and upgrade arguments. rg -n -C4 'OVERLAY_FRAMEWORK_VERSION|OVERLAY_PREVIOUS|UpgradeArguments|WithVersions' --type=goRepository: NVIDIA/nodewright
Length of output: 155
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- candidate files ---' fd -i 'history|version|upgrade|orches' agent/go printf '%s\n' '--- history outline ---' ast-grep outline agent/go/internal/history/history.go --match '$_' --view compact || true printf '%s\n' '--- history source ---' cat -n agent/go/internal/history/history.go printf '%s\n' '--- version-related identifiers and literals ---' rg -n -C3 'OVERLAY|UpgradeArguments|WithVersions|Versions|upgrade|history' agent/go --glob '*.go' || true printf '%s\n' '--- repository diff summary ---' git diff --statRepository: NVIDIA/nodewright
Length of output: 50373
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- upgrade step implementation ---' cat -n agent/go/internal/step/upgrade_step.go printf '%s\n' '--- shared step execution ---' cat -n agent/go/internal/step/shared.go printf '%s\n' '--- orchestration version flow ---' sed -n '250,430p' agent/go/internal/agent/agent.go printf '%s\n' '--- all framework/version environment references ---' rg -n -C3 'OVERLAY_FRAMEWORK_VERSION|PREVIOUS_VERSION|CURRENT_VERSION|WithVersions|Versions\{' . --glob '!agent/go/internal/step/*_test.go' --glob '!agent/go/internal/history/*_test.go' printf '%s\n' '--- configuration fields ---' rg -n -C3 'PackageName|PackageVersion' agent/go/internal/config agent/go/internal/agent --glob '*.go'Repository: NVIDIA/nodewright
Length of output: 48588
Set
OVERLAY_FRAMEWORK_VERSIONin the Go agent.
runStepsalready passes previous and current versions. No Go code setsOVERLAY_FRAMEWORK_VERSIONto{package name}-{version}, which history management requires.🤖 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/history/history.go` around lines 35 - 39, Update the Go agent’s runSteps flow to set OVERLAY_FRAMEWORK_VERSION to the required {package name}-{version} value before executing steps, using the existing previous/current version data passed by runSteps. Ensure the environment variable is available to history management without changing the existing version arguments or history constants.Source: Learnings
agent/go/internal/step/upgrade_step.go (1)
104-107: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Determine whether step completion flags are keyed by Fingerprint. set -euo pipefail rg -n --type=go -C 10 'func .*fileStore\) Path\(' agent/go/internal/flags rg -n --type=go -C 4 'Fingerprint\(\)' agent/goRepository: NVIDIA/nodewright
Length of output: 6378
🏁 Script executed:
#!/bin/bash set -euo pipefail for f in agent/go/internal/step/step.go \ agent/go/internal/step/regular_step.go \ agent/go/internal/step/upgrade_step.go \ agent/go/internal/step/fingerprint.go \ agent/go/internal/flags/flags.go; do if [ -f "$f" ]; then echo "===== $f =====" wc -l "$f" ast-grep outline "$f" || true fi done echo "===== step fingerprint definitions/usages =====" rg -n --type=go -C 8 'func stepFingerprint|stepFingerprint\(' agent/go/internal/step agent/go/internal/flags agent/go/internal/agent echo "===== flag path and completion call sites =====" sed -n '105,145p' agent/go/internal/flags/flags.go rg -n --type=go -C 8 '\.Path\(.*step|flags\.(Get|Set|Has|Path)|Store\.(Get|Set|Path)|Flag' agent/go/internal/agent agent/go/internal/flagsRepository: NVIDIA/nodewright
Length of output: 48409
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "===== fingerprint helper =====" sed -n '204,270p' agent/go/internal/step/shared.go echo "===== step execution and flag checks =====" sed -n '165,245p' agent/go/internal/agent/steps.go echo "===== regular and upgrade constructors/defaults =====" sed -n '29,115p' agent/go/internal/step/regular_step.go sed -n '31,140p' agent/go/internal/step/upgrade_step.go echo "===== stage assignment and upgrade-step decoding/configuration =====" rg -n --type=go -C 5 'UpgradeStep|NewUpgradeStep|NewRegularStep|Modes\[|stage\.Upgrade' agent/go/internal/config agent/go/internal/step agent/go/internal/agentRepository: NVIDIA/nodewright
Length of output: 50373
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' import hashlib import json import os # This models the fields passed by both Fingerprint methods and the filename # construction in fileStore.Path. It does not import or execute repository code. path = "upgrade.sh" arguments = [] returncodes = [0] environment = {} on_host = True payload = { "path": path, "arguments": arguments, "returncodes": returncodes, "environment": environment, "on_host": on_host, } digest = hashlib.sha256( json.dumps(payload, sort_keys=True, separators=(",", ":")).encode() ).hexdigest() regular = digest upgrade = digest regular_filename = os.path.basename(path) + "-" + regular + ".flag" upgrade_filename = os.path.basename(path) + "-" + upgrade + ".flag" print("regular fingerprint:", regular) print("upgrade fingerprint:", upgrade) print("fingerprints equal:", regular == upgrade) print("regular flag filename:", regular_filename) print("upgrade flag filename:", upgrade_filename) print("flag filenames equal:", regular_filename == upgrade_filename) PYRepository: NVIDIA/nodewright
Length of output: 587
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' import hashlib import json import os path = "upgrade.sh" arguments = [] returncodes = [0] environment = {} on_host = True payload = { "path": path, "arguments": arguments, "returncodes": returncodes, "environment": environment, "on_host": on_host, } digest = hashlib.sha256( json.dumps(payload, sort_keys=True, separators=(",", ":")).hexdigest() ).hexdigest() regular_filename = os.path.basename(path) + "-" + digest + ".flag" upgrade_filename = os.path.basename(path) + "-" + digest + ".flag" print("fingerprints equal:", digest == digest) print("flag filenames equal:", regular_filename == upgrade_filename) PYRepository: NVIDIA/nodewright
Length of output: 287
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "===== runSteps loop and flag decision =====" sed -n '44,165p' agent/go/internal/agent/steps.go echo "===== flag Check implementation and decision =====" sed -n '168,214p' agent/go/internal/flags/flags.go echo "===== package/version inputs to flag store =====" rg -n --type=go -C 6 'PackageVersion|NewStore\(|WithVersions|stage\.Upgrade|StageAlwaysRuns' agent/go/internal/agent agent/go/internal/flags agent/go/internal/config agent/go/internal/runtime echo "===== uniqueness and placement validation =====" rg -n --type=go -C 8 'duplicate|unique|same path|requireUpgradeStepPlacement|expectedCheckName|UpgradeStep' agent/go/internal/config/validate.goRepository: NVIDIA/nodewright
Length of output: 39423
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' # Read-only verifier of the relevant policy encoded in the inspected source. allowed_upgrade_stages = {"upgrade", "upgrade-check"} always_run_stages = {"config", "uninstall", "upgrade"} for stage in ["apply", "upgrade", "upgrade-check"]: upgrade_allowed = stage in allowed_upgrade_stages stage_forces_run = stage in always_run_stages print(stage, "upgrade_allowed=", upgrade_allowed, "flag_can_skip=", not stage_forces_run) PYRepository: NVIDIA/nodewright
Length of output: 311
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' allowed_upgrade_stages = {"upgrade", "upgrade-check"} always_run_stages = {"config", "uninstall", "upgrade"} for stage in ["apply", "upgrade", "upgrade-check"]: upgrade_allowed = stage in allowed_upgrade_stages stage_forces_run = stage in always_run_stages print(stage, "upgrade_allowed=", upgrade_allowed, "flag_can_skip=", not stage_forces_run) PYRepository: NVIDIA/nodewright
Length of output: 311
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "===== stage definitions and execution dispatch =====" rg -n --type=go -C 8 'var All|const \(|ApplyCheck|UpgradeCheck|runSteps\(|stage\.Apply|stage\.Upgrade' agent/go/internal/stage agent/go/internal/agent echo "===== validation rules for duplicate paths or cross-stage identity =====" rg -n --type=go -C 6 'Path\(\)|path.*duplicate|duplicate.*path|same.*path|map\[.*Path|requireApplyChecks|expectedCheckName' agent/go/internal/config echo "===== flag namespace construction =====" sed -n '80,125p' agent/go/internal/flags/flags.goRepository: NVIDIA/nodewright
Length of output: 39850
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' # Model the relevant policy from the inspected source. def decide(flag_exists, always_run, stage, idempotence="auto"): if not flag_exists: return True, "flag-missing" if always_run: return True, "always-run" if stage in {"config", "uninstall", "upgrade"}: return True, "stage-always-runs" if idempotence == "disabled": return True, "idempotence-disabled" return False, "already-completed" print("upgrade stage with shared marker:", decide(True, False, "upgrade")) print("apply stage with shared marker:", decide(True, False, "apply")) print("upgrade-check stage execution:", "flag check is bypassed because it is a check stage") PYRepository: NVIDIA/nodewright
Length of output: 360
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' def decide(flag_exists, always_run, stage, idempotence="auto"): if not flag_exists: return True, "flag-missing" if always_run: return True, "always-run" if stage in {"config", "uninstall", "upgrade"}: return True, "stage-always-runs" if idempotence == "disabled": return True, "idempotence-disabled" return False, "already-completed" print("upgrade stage with shared marker:", decide(True, False, "upgrade")) print("apply stage with shared marker:", decide(True, False, "apply")) print("upgrade-check stage execution:", "flag check is bypassed because it is a check stage") PYRepository: NVIDIA/nodewright
Length of output: 360
Include the step type in the fingerprint.
RegularStepandUpgradeStepproduce the same fingerprint for identical inputs, andfileStore.Pathuses that fingerprint without the step type or stage. A shared marker can cause a later idempotentRegularStepinstage.Applyto be skipped after anUpgradeStepwrites it. Add the discriminator and a cross-type collision 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/step/upgrade_step.go` around lines 104 - 107, Update UpgradeStep.Fingerprint to include a step-type discriminator in the value passed to stepFingerprint, matching the corresponding RegularStep fingerprint behavior while preserving all existing execution inputs. Add a test proving identical inputs for RegularStep and UpgradeStep produce different fingerprints and cannot share the same idempotency marker.
9a4aa8c to
9d34850
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
agent/go/internal/agent/package_test.go (1)
51-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a spec for the non-local
ExpectedConfigFilesbranch.This spec covers the overlay copy, one present expected-config file, and one missing expected-config file. The containment check at
agent/go/internal/agent/package.goLine 88 stays uncovered. That check validates package-supplied input, so a regression there would allow reads outside the configmaps directory.💚 Proposed additional spec
It("rejects an expected config file outside the configmaps directory", func() { root := GinkgoT().TempDir() copyRoot := filepath.Join(root, "package") Expect(os.MkdirAll(filepath.Join(copyRoot, configMapsDirName), 0o755)).To(Succeed()) err := prepareHost(copyRoot, root, config.Config{ ExpectedConfigFiles: []string{filepath.Join("..", "escape.conf")}, }) Expect(err).To(MatchError(ContainSubstring("must be relative to the configmaps directory"))) })As per coding guidelines, "Code changes should include unit tests".
🤖 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/agent/package_test.go` around lines 51 - 78, Add a unit test alongside the existing prepareHost specs covering a non-local ExpectedConfigFiles entry such as a parent-directory traversal path. Create the configmaps directory, call prepareHost with that entry, and assert the error contains “must be relative to the configmaps directory,” exercising the containment validation in prepareHost.Source: Coding guidelines
🤖 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/agent/interrupt.go`:
- Around line 166-199: Move completion-marker creation for non-NodeRestartType
interrupts in the execution flow around value.Run so it occurs immediately after
successful execution, before closeLogErr or CleanupOldLogs failures are
returned; preserve failure reporting and avoid marking unsuccessful runs
complete. Add a regression test that forces log finalization to fail, then
verifies the subsequent invocation does not call Run, ensuring one marker is
written per interrupt type and resource ID.
In `@agent/go/internal/agent/steps.go`:
- Around line 203-227: Update the step execution flow around value.Run and log
closing so log retention always runs after runtime or close errors, rather than
returning immediately. Close the log, resolve the log pattern, and call
flags.CleanupOldLogs before returning, joining any retention failure with the
existing runErr and closeErr; preserve the current error context and failed
status. Add a regression test covering repeated runErr results and verifying old
logs are cleaned up.
- Around line 252-270: The check-stage flow must clear any existing
*_ALL_CHECKED marker before running current checks, so stale success state
cannot survive a failure. Update the relevant check execution function around
the checkResultsFlagName and completed-check flag writes to remove the marker
before checks begin, retain recreation only after all checks pass, and add a
regression test covering a successful run followed by a failed run.
---
Duplicate comments:
In `@agent/go/internal/agent/package_test.go`:
- Around line 51-78: Add a unit test alongside the existing prepareHost specs
covering a non-local ExpectedConfigFiles entry such as a parent-directory
traversal path. Create the configmaps directory, call prepareHost with that
entry, and assert the error contains “must be relative to the configmaps
directory,” exercising the containment validation in prepareHost.
🪄 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: 5f306cba-b719-463c-aad6-dace908798bc
📒 Files selected for processing (10)
agent/README.mdagent/go/cmd/agent/main.goagent/go/internal/agent/agent.goagent/go/internal/agent/agent_test.goagent/go/internal/agent/interrupt.goagent/go/internal/agent/interrupt_test.goagent/go/internal/agent/package.goagent/go/internal/agent/package_test.goagent/go/internal/agent/steps.goagent/go/internal/agent/steps_test.go
| status, runErr := value.Run(ctx, runConfig) | ||
| closeErr := closeLogErr() | ||
| var cleanupErr error | ||
| if runtime.writeLogs { | ||
| cleanupErr = flags.CleanupOldLogs(logFiles, flags.DefaultLogRetention) | ||
| } | ||
| if runErr != nil || closeErr != nil || cleanupErr != nil { | ||
| if runErr != nil { | ||
| runErr = fmt.Errorf("running interrupt %q: %w", interruptType, runErr) | ||
| } | ||
| if cleanupErr != nil { | ||
| cleanupErr = fmt.Errorf("cleaning old interrupt logs: %w", cleanupErr) | ||
| } | ||
| return execution.StatusFailed, errors.Join( | ||
| runErr, | ||
| closeErr, | ||
| cleanupErr, | ||
| ) | ||
| } | ||
| if status != execution.StatusSuccess { | ||
| return execution.StatusFailed, nil | ||
| } | ||
|
|
||
| if interruptType != interrupts.NodeRestartType { | ||
| if err := hostfs.CreateFile( | ||
| req.rootMount, | ||
| completeMarker, | ||
| []byte(time.Now().UTC().Format(time.RFC3339Nano)), | ||
| markerFileMode, | ||
| ); err != nil { | ||
| return execution.StatusFailed, fmt.Errorf("marking interrupt complete: %w", err) | ||
| } | ||
| } | ||
| return execution.StatusSuccess, nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Write the ordinary completion marker before post-run log failures return.
For a non-NodeRestartType interrupt, value.Run can return execution.StatusSuccess with no error, but closeLogErr or flags.CleanupOldLogs can return before completeMarker is written. The next invocation then executes the completed interrupt again.
Write the completion marker after successful interrupt execution and before reporting post-run log failures. Add a regression test that forces a log-finalization failure and verifies that the next invocation does not call Run.
As per coding guidelines, the orchestration layer must write one completion marker per interrupt type and resource ID after successful execution.
🤖 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/agent/interrupt.go` around lines 166 - 199, Move
completion-marker creation for non-NodeRestartType interrupts in the execution
flow around value.Run so it occurs immediately after successful execution,
before closeLogErr or CleanupOldLogs failures are returned; preserve failure
reporting and avoid marking unsuccessful runs complete. Add a regression test
that forces log finalization to fail, then verifies the subsequent invocation
does not call Run, ensuring one marker is written per interrupt type and
resource ID.
Source: Coding guidelines
| if runErr != nil || closeErr != nil { | ||
| if runErr != nil { | ||
| runErr = fmt.Errorf("running step %q: %w", value.Path(), runErr) | ||
| } | ||
| if closeErr != nil { | ||
| closeErr = fmt.Errorf("closing log for step %q: %w", value.Path(), closeErr) | ||
| } | ||
| return execution.StatusFailed, errors.Join( | ||
| runErr, | ||
| closeErr, | ||
| ) | ||
| } | ||
| if runtime.writeLogs { | ||
| logFiles, err := layout.LogFilePattern(cfg, value.Path()) | ||
| if err != nil { | ||
| return execution.StatusFailed, fmt.Errorf( | ||
| "resolving log retention for step %q: %w", | ||
| value.Path(), | ||
| err, | ||
| ) | ||
| } | ||
| if err := flags.CleanupOldLogs(logFiles, flags.DefaultLogRetention); err != nil { | ||
| return execution.StatusFailed, fmt.Errorf("cleaning old logs for step %q: %w", value.Path(), err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Run log retention after runtime errors.
CreateLogFile creates a log before value.Run. If value.Run returns runErr, this branch returns before CleanupOldLogs runs. A repeated runtime error then retains one new host log per invocation without a retention pass.
Close the log, run retention, and join any retention error before returning the step error. Add a regression test for repeated runErr results.
🤖 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/agent/steps.go` around lines 203 - 227, Update the step
execution flow around value.Run and log closing so log retention always runs
after runtime or close errors, rather than returning immediately. Close the log,
resolve the log pattern, and call flags.CleanupOldLogs before returning, joining
any retention failure with the existing runErr and closeErr; preserve the
current error context and failed status. Add a regression test covering repeated
runErr results and verifying old logs are cleaned up.
| if err := flagStore.Write( | ||
| filepath.Join(layout.FlagDir(), checkResultsFlagName), | ||
| []byte(strings.Join(lines, "\n")), | ||
| ); err != nil { | ||
| return execution.StatusFailed, fmt.Errorf("writing check results for stage %q: %w", currentStage, err) | ||
| } | ||
| if failed { | ||
| return execution.StatusFailed, nil | ||
| } | ||
| if err := flagStore.Write( | ||
| filepath.Join(layout.FlagDir(), string(currentStage)+"_ALL_CHECKED"), | ||
| nil, | ||
| ); err != nil { | ||
| return execution.StatusFailed, fmt.Errorf( | ||
| "writing completed-check flag for stage %q: %w", | ||
| currentStage, | ||
| err, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear an existing all-checked marker before each check run.
A passing run creates *_ALL_CHECKED. A later failed run returns at Line 258 without removing that existing marker. A prior success can therefore remain visible after the current checks fail.
Remove the marker before executing the check stage. Recreate it only after every current check succeeds. Add a success-then-failure regression 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/agent/steps.go` around lines 252 - 270, The check-stage
flow must clear any existing *_ALL_CHECKED marker before running current checks,
so stale success state cannot survive a failure. Update the relevant check
execution function around the checkResultsFlagName and completed-check flag
writes to remove the marker before checks begin, retain recreation only after
all checks pass, and add a regression test covering a successful run followed by
a failed run.
9d34850 to
a9b4f91
Compare
a9b4f91 to
c10a958
Compare
825f656 to
f9d6fef
Compare
f9d6fef to
d03e584
Compare
lockwobr
left a comment
lockwobr
left a comment
There was a problem hiding this comment.
Cross-review summary
Automated multi-reviewer analysis of d03e5844, cross-reviewed to consensus with adversarial verification of every confirmed finding. Nothing was run against a host or a cluster and no tests were executed; every claim comes from reading the pinned commit alongside the Python agent it replaces.
No CI ran on this commit. .github/workflows/agent-go-ci.yaml triggers only on pull_request targeting main, and this PR targets agent-mocks-219, so no Go build, test, vet or lint executed. The sole reported check is an automated review. That is worth fixing independently of anything below, because a stacked branch is exactly where a rewrite of this size wants a test signal.
Findings fall into two groups. Confirmed ones reached agreement across independent reviewers and then survived a fresh reviewer whose only job was to refute them. Unadjudicated ones carry a single reporter's position or a genuine reviewer split, noted per item. Two further findings were dropped after verification, and are not reproduced here.
The theme
Every confirmed finding is the same class: a divergence from the Python agent's on-disk or environment contract. Individually each is small. Together they mean cross-agent state continuity is not preserved: interrupt completion markers, step completion flags, symlink policy and boolean env parsing all changed shape. A node that has run the Python agent and then runs the Go agent does not recognise its own prior work.
That may well be deliberate, and the argument for it is reasonable: the Go agent is not the container entrypoint yet (containers/agent.Dockerfile still runs python -m skyhook_agent.controller), so nothing regresses today. But the drop-in-replacement framing implies a rolling image swap, and under a rolling swap these become live at once. Whichever way it goes, it should be a stated decision rather than an emergent one, because the failure modes are silent: a second reboot on an already-interrupted node, or a package that wedges on every stage.
Inline comments carry the findings that anchor to changed lines. Two more follow, both in files this PR does not touch but does put on the execution path.
Unadjudicated: reviewers split
agent/go/internal/interrupts/node_restart.go:62 - reboot success is not verified
One reviewer raised this and another disagreed, so treat it as an open technical question rather than a defect.
The claim: node_restart returns StatusSuccess on an ordinary exit-0 from reboot, which bypasses the boot-ID verification this PR adds. :51-55 return success only for the SIGTERM-during-shutdown case; :59-61 fail on signal or non-zero; :62 is the plain exit-0 path.
On systemd hosts reboot returns 0 as soon as the shutdown job is enqueued. In that window the agent exits 0, the init container completes, and the operator treats the interrupt stage as done with no verified reboot. Because a completed init container is not re-run, the retained .pending marker is never consumed and the boot-ID check never fires. interrupt.go:160-163 sets retainPendingMarker = true precisely so completion is decided "by comparing host boot IDs on the next invocation" - but that next invocation has to happen.
Two things would settle it:
- Is there a guarantee across supported host images that
rebootinside the chroot never returns 0 before shutdown terminates the init container? This is the hinge for severity, and it was not found in the pinned docs. - Does any later invocation (interrupt-check, post-interrupt) actually re-run the
node_restartpath after the node returns? If it does,prepareNodeRestartMarkereventually promotes the pending marker and the exposure narrows to the window before the operator advances the stage.
agent/go/internal/step/shared.go:184 - step scripts are never chmodded executable
Single reporter, not cross-evaluated.
shared.go:184-191 builds the step command with no command.WithPermissions(...). command.go:98 defaults Permissions to 0, and both runners only chmod when it is non-zero (command_runner.go:36, chroot_command_runner.go:54). The Python agent chmods +x on every normal step: chroot_exec.py:62-64 runs os.chmod(cmds[0], ... S_IXGRP|S_IXUSR|S_IXOTH) whenever no_chmod is false, and run_step calls _run without it (controller.py:255, invoked at :330-337). Interrupts are consistent between the two.
So any package whose step scripts lack the execute bit in the container image installs fine today and starts failing with a permission error on apply/config/upgrade/post-interrupt once the agent image switches. Silent behavior removal rather than a documented tightening.
Open questions
- Should
agent-go-cirun for PRs targeting stacked branches? As written it only fires againstmain, so this entire rewrite merged with no Go test signal. - Was the
strconv.ParseBoolwidening intentional?"1","t","T"are true in Go and false in Python, independent of the fallback issue in the inline comment. If intentional it belongs inagent/CHANGELOG.mdand the README as a documented break. - Was dropping the separate stderr log file intentional? Python writes
<step>-<ts>.log.erralongside the stdout log and prefixes lines with[out]/[err]plus per-line timestamps (controller.py:160-201); the Go path MultiWriters both streams into one raw.log. No fixture asserts on it, so nothing breaks today. - Nothing prunes
interrupts/flags/<resourceID>/. The.pendingmarker is retained once node-restart execution begins and is only consumed by a later invocation for the same resource ID. Across generations those directories accumulate on the host, and neither the operator nor the agent removes them. - Can
os.Rootsee host procfs?boot_idis read through the mounted host root. The interrupt pod mounts hostPath/withMountPropagation: HostToContainer, so/root/procshould be host procfs, but no lane could verify the submount is visible to a rooted read without executing something. If it is not, node-restart completion never promotes. - Interrupt log filenames also change (
interrupts/<type>-<ts>.logversusinterrupts/<type>_<i>-<ts>.log). No consumer reads host log filenames, so this is noted rather than raised. - Log timestamps gain nanosecond precision.
CleanupOldLogsparses both formats so retention survives, but external tooling matching the old exact shape will not.
| interruptsDirName, | ||
| interruptFlagsDirName, | ||
| runtime.resourceID, | ||
| string(interruptType)+".complete", |
There was a problem hiding this comment.
Confirmed finding, highest severity here. The completion marker name does not match the agent that ships today, so a completed interrupt is invisible after an agent swap.
This builds <stateRoot>/interrupts/flags/<resourceID>/<type>.complete and line 73 gates re-execution on that exact name. The Python agent builds the same directory (controller.py:492) but names the file per interrupt command index:
interrupt_id = f"{interrupt._type()}_{i}" # controller.py:503
... f"{interrupt_dir}/{interrupt_id}.complete" # controller.py:484So the on-disk names are node_restart_0.complete, service_restart_0.complete, service_restart_1.complete. Only NoOp uses the bare type (controller.py:497) - which is why k8s-tests/operator-agent/interrupt/chainsaw-test.yaml:46 asserts no_op.complete and passes, and would not catch this.
SKYHOOK_RESOURCE_ID is <name>-<uid>-<generation>_<pkg>_<version>, stable across pod restarts, so this is not a race. A node that already rebooted under the Python agent presents no node_restart_0.complete that this code looks for, and the reboot interrupt runs a second time on an already-interrupted node. Same class for service_restart and restart_all_services, which additionally lose Python's per-command resume granularity.
The reboot path is the one case that always spans two agent invocations for a single resource ID, so it is the most likely to be hit in practice.
One reviewer refuted a version of this on the grounds that no Go agent image exists yet, which is true and is why nothing regresses today. It does not change the analysis for the moment the entrypoint switches. If the intent is that hosts start from a clean state format, that is defensible - but then it needs to be stated, and probably needs the old markers cleared rather than ignored.
| time.Now().UTC().Format(time.RFC3339Nano), | ||
| configuredStep.Idempotence() == step.Disabled, | ||
| ) | ||
| if _, err := flagStore.Mark(configuredStep, message); err != nil { |
There was a problem hiding this comment.
Confirmed finding. Step completion flags use a different naming scheme than the Python agent, so every step re-runs on hosts carrying Python-era flags.
This line (flagStore.Mark) and line 88 (flagStore.Check) are the first code paths that actually exercise flags.Store. flags.go:118 names the file:
base(step.Path()) + "-" + <sha256 fingerprint> + ".flag"
The Python agent names it <step.path>_<base64(f"{arguments}_{returncodes}")> with no suffix (controller.py:246), under the identical directory <stateRoot>/flags/<pkg>/<version>/.
Same directory, no overlap in filenames. When a live cluster's agent image switches, no existing flag matches, so every previously-completed apply and post-interrupt step re-runs once on every node. (Config, uninstall and upgrade always re-run anyway, so those are unaffected.) How much that hurts depends on how genuinely idempotent the packaged steps are - but avoiding exactly this re-run is what the flag is for.
The naming code in flags.go predates this PR. This PR is what puts it on the execution path, which is why it surfaces here.
| } | ||
|
|
||
| func prepareHost(copyRoot, rootMount string, cfg config.Config) error { | ||
| if err := hostfs.CopyTreeIfExists( |
There was a problem hiding this comment.
Confirmed finding, plus a second unadjudicated mechanism on the same line. Both are consequences of the same symlink policy, but they fail at different points and need separate fixes.
1. Symlinks in the source tree (confirmed). prepareHost here and ensurePackageData (lines 52, 60) route every copy through hostfs.CopyTreeIfExists, and hostfs.CopyTree rejects any symlink outright:
// agent/go/internal/hostfs/copy.go:72-74
return fmt.Errorf("copy source %q is a symbolic link", path)The reference implementation uses shutil.copytree(..., dirs_exist_ok=True) with the default symlinks=False (controller.py:557,561,575), which dereferences and copies the target's contents. So a package that ships a symlink anywhere under root_dir/ - a common way to alias a config file - installs cleanly under the Python agent and fails every apply/config/upgrade/post-interrupt stage under the Go agent with copying package root overlay: ... is a symbolic link, wedging the package on the node. The same applies to legacy package data copied from SKYHOOK_DATA_DIR. Note the operator populates the host copy dir with cp -r, which preserves symlinks.
2. Symlinks in the destination path (unadjudicated, single reporter). Distinct and arguably worse. The per-entry work goes through hostfs.ensureDirectories (hostfs.go:335-345) and inspect (hostfs.go:348-360), both of which Lstat every path component and return path component %q is a symbolic link.
On every usr-merged distro - Ubuntu 20.04+, Debian 12, RHEL 8+ - /bin, /sbin, /lib and /lib64 are symlinks into /usr. Individual files such as /etc/localtime are symlinks too. So a root_dir overlay targeting a unit file under /lib/systemd/system, a binary under /bin or /sbin, or a replacement /etc/localtime - all documented uses - fails with copying package root overlay: path component "lib" is a symbolic link on essentially every mainstream host.
If the strictness is deliberate hardening rather than an oversight, the destination case still needs an answer: there is no way to write to /lib on a usr-merged system without traversing a symlink.
| return value | ||
| } | ||
|
|
||
| func envBool(name string, fallback bool, logger *slog.Logger) bool { |
There was a problem hiding this comment.
Confirmed finding. envBool returns the caller's default when strconv.ParseBool fails, and lines 227-228 pass fallback=true for both SKYHOOK_AGENT_WRITE_LOGS and COPY_RESOLV.
The Python agent evaluates both as os.getenv(...).lower() == "true" (controller.py:74 and :680), so any value other than "true" yields false - including the empty string, "yes", "off", or a typo. agent/skyhook-agent/tests/test_controller.py:1471 asserts that behavior.
So the two contracts disagree in opposite directions on the same input. A user who disabled host log retention or resolv.conf copying with a value the Python agent accepted as false silently gets the feature re-enabled under the Go agent: host log files reappear, and the container's /etc/resolv.conf is written to the host. The only trace is a warning log.
Worth noting that agent_test.go:167-180 codifies the divergence by asserting COPY_RESOLV=" true " yields copyResolver=true, so this reads as intentional. If it is, the safer shape is to keep the permissive parse but make the fallback false for these two variables, so an unparseable value never turns a feature on. Either way it belongs in the changelog: silently re-enabling a host-mutating behavior on upgrade is the kind of break that is hard to diagnose from the node.
| if err != nil { | ||
| return fmt.Errorf("resolving host resolver path: %w", err) | ||
| } | ||
| if err := hostfs.CopyFile(rootMount, source, destination); err != nil { |
There was a problem hiding this comment.
Unadjudicated, single reporter. Lower severity than the sibling comment above, but the same root cause.
copyResolverConfig calls hostfs.CopyFile(rootMount, "/etc/resolv.conf", <rootMount>/etc/resolv.conf), and copyRegularFile rejects a symlinked destination (copy.go:126-128); os.Root would refuse the traversal regardless. On systemd-resolved hosts /etc/resolv.conf is a symlink to ../run/systemd/resolve/stub-resolv.conf. Python uses shutil.copyfile (controller.py:682), which opens through the symlink and succeeds.
agent.go:228 defaults copyResolver to true and agent.go:338-342 runs it before anything else, so on a mainstream host this aborts every stage, including uninstall, with copying resolver configuration: ... is a symbolic link.
Kept at minor only because the operator injects COPY_RESOLV="false" for both package and interrupt pods, so the supported path never reaches it. What does reach it is the manual and legacy invocation forms this PR documents in agent/README.md, where the documented default is enabled.
d03e584 to
84a5f0e
Compare
84a5f0e to
bd12a2e
Compare
Signed-off-by: Riley Rice <rrice@nvidia.com>
bd12a2e to
11f9d05
Compare
2 participants
Description
Completes the final orchestration for the Go agent rewrite and activates it through the production entrypoint.
Depends on #408.
Closes #219
Checklist
git commit -s) per the DCO.