feat(agent): added Run() to Interrupt interface for agent go rewrite#327
feat(agent): added Run() to Interrupt interface for agent go rewrite#327rice-riley wants to merge 1 commit into
Conversation
Signed-off-by: Riley Rice <rrice@nvidia.com>
github-actions
Bot
added
component/operator
| - name: Run tests with coverage | ||
| working-directory: agent/go | ||
| run: make unit-tests merge-coverage | ||
| - name: Upload coverage artifact | ||
| if: hashFiles('agent/go/reporting/cover.out') != '' | ||
| uses: actions/upload-artifact@v5 | ||
| with: | ||
| name: coverage-agent-go-unit-tests | ||
| path: agent/go/reporting/cover.out | ||
| retention-days: 1 | ||
| if-no-files-found: ignore |
There was a problem hiding this comment.
will this conflict with the operator cover? or can we have 2 badges?
| var _ = Describe("chroot command execution", func() { | ||
| It("executes a command inside the target root", func() { | ||
| if os.Geteuid() != 0 { | ||
| Skip("chroot execution requires root privileges") |
There was a problem hiding this comment.
hmm, this could cause this to be skipped and not run, maybe this should be a fail.
| if runCommandTestHelper() { | ||
| return | ||
| } |
| type errorWriter struct { | ||
| err error | ||
| } | ||
|
|
||
| func (writer errorWriter) Write([]byte) (int, error) { | ||
| return 0, writer.err | ||
| } | ||
|
|
||
| func helperCommand(action string, valuesAndOptions ...any) Command { | ||
| values := make([]string, 0, len(valuesAndOptions)) | ||
| options := []CommandOption{} | ||
| for _, value := range valuesAndOptions { | ||
| switch typed := value.(type) { | ||
| case string: | ||
| values = append(values, typed) | ||
| case CommandOption: | ||
| options = append(options, typed) | ||
| default: | ||
| panic(fmt.Sprintf("unsupported helper command value %T", value)) | ||
| } | ||
| } | ||
| arguments := append([]string{"-test.run=^TestCommand$", "--", action}, values...) | ||
| options = append([]CommandOption{WithArguments(arguments...)}, options...) | ||
| return NewCommand(commandTestExecutable(), options...) | ||
| } | ||
|
|
||
| func commandTestExecutable() string { | ||
| executable, err := filepath.Abs(os.Args[0]) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| return executable | ||
| } | ||
|
|
||
| func runCommandTestHelper() bool { | ||
| separator := -1 | ||
| for index, argument := range os.Args { | ||
| if argument == "--" { | ||
| separator = index | ||
| break | ||
| } | ||
| } | ||
| if separator < 0 { | ||
| return false | ||
| } | ||
| if separator+1 >= len(os.Args) { | ||
| return false | ||
| } | ||
| action := os.Args[separator+1] | ||
| values := os.Args[separator+2:] | ||
| switch action { | ||
| case "inspect": | ||
| workingDirectory, err := os.Getwd() | ||
| if err != nil { | ||
| os.Exit(2) | ||
| } | ||
| _, _ = fmt.Fprintf(os.Stdout, "arguments=%s\n", strings.Join(values, ",")) | ||
| _, _ = fmt.Fprintf(os.Stdout, "environment=%s\n", os.Getenv("NODEWRIGHT_VALUE")) | ||
| _, _ = fmt.Fprintf(os.Stdout, "working-directory=%s\n", workingDirectory) | ||
| _, _ = fmt.Fprintln(os.Stderr, "helper-stderr") | ||
| os.Exit(0) | ||
| case "stdout": | ||
| _, _ = io.WriteString(os.Stdout, "output") | ||
| os.Exit(0) | ||
| case "exit": | ||
| code, err := strconv.Atoi(values[0]) | ||
| if err != nil { | ||
| os.Exit(2) | ||
| } | ||
| os.Exit(code) | ||
| case "signal": | ||
| _ = syscall.Kill(os.Getpid(), syscall.SIGTERM) | ||
| time.Sleep(time.Hour) | ||
| case "wait": | ||
| time.Sleep(time.Hour) | ||
| default: | ||
| _, _ = fmt.Fprintf(os.Stderr, "unknown helper action %q\n", action) | ||
| os.Exit(2) | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
I wonder if we should move to using mockery instead?
📝 WalkthroughWalkthroughThe Go agent adds shared execution configuration and status types, introduces process-group command execution with cancellation and result reporting, and migrates steps and interrupts to the shared runtime contract. Interrupts now use typed identifiers and execute through configured runners. Extensive unit and Linux integration tests cover command, step, interrupt, chroot, signal, cancellation, and serialization behavior. CI now generates, merges, and uploads Go coverage output. Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/agent-go-ci.yaml:
- Line 48: Update the actions/upload-artifact step in the workflow to reference
the approved immutable full commit SHA instead of the floating v5 tag, while
retaining an inline # v5 version hint.
- Around line 43-53: Add an explicit workflow-level permissions block to
agent-go-ci.yaml granting only contents: read. Place it alongside the workflow’s
top-level configuration so the existing checkout, test, and coverage artifact
upload steps retain the required access without broader GITHUB_TOKEN
permissions.
In `@agent/go/internal/command/process_test.go`:
- Around line 53-61: Update the cancellation test around NewRunner().Run and
helperCommand("wait") to synchronize cancellation with helper startup: make the
helper signal readiness once its process is running, wait for that signal, then
call cancel instead of using the fixed 100 ms timer. Preserve the existing
context.Canceled, SignalExitCode, and SIGKILL assertions.
In `@agent/go/internal/execution/execution.go`:
- Around line 29-44: Document the architectural execution contract introduced by
the Status type and Config struct, including the possible operation statuses and
the configuration fields used for filesystem and output composition. Update the
affected execution documentation in the same change, using the symbols Status,
StatusSuccess, StatusFailed, and Config as the documented API.
In `@agent/go/internal/interrupts/interrupts.go`:
- Around line 48-52: Update the architecture/operator documentation for the
Interrupt interface and its implementers to describe the migration from
command-only interrupts to runtime execution using execution.Config and
execution.Status, including the Run contract. Keep the documentation consistent
with the Type, Run, and Serialize methods exposed by Interrupt.
In `@agent/go/internal/interrupts/node_restart_test.go`:
- Around line 35-44: Update nodeRestartCompleted and its tests so a NodeRestart
result with exit code -15 is recognized as successful, while SIGKILL and
unrelated exit codes remain unsuccessful. Add an explicit compatibility
assertion for the unstructured -15 result alongside the existing structured
SIGTERM case.
In `@agent/go/internal/step/regular_step_test.go`:
- Around line 232-245: Update the missing-variable test around NewRegularStep
and Run to use environment variable names guaranteed not to exist in the parent
environment, or explicitly unset both variables before execution and restore
their original values with cleanup. Preserve the assertion that both missing
names are reported before command execution.
In `@agent/go/Makefile`:
- Around line 50-51: Update the Makefile test target and merge-coverage
dependency flow so merge-coverage always runs after unit-tests, preventing
parallel execution from reading incomplete profiles. Ensure
$(REPORTING)/cover.out is cleared or guarded before the go tool cover report,
while preserving the existing coverage reporting behavior.
🪄 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: 1b4f4880-b47f-4d54-82df-103bd9684122
📒 Files selected for processing (30)
.github/workflows/agent-go-ci.yamlagent/go/.gitignoreagent/go/Makefileagent/go/internal/command/chroot_command_runner_linux_test.goagent/go/internal/command/command_runner_test.goagent/go/internal/command/command_test.goagent/go/internal/command/process.goagent/go/internal/command/process_test.goagent/go/internal/execution/execution.goagent/go/internal/execution/execution_test.goagent/go/internal/interrupts/interrupts.goagent/go/internal/interrupts/interrupts_linux_test.goagent/go/internal/interrupts/interrupts_test.goagent/go/internal/interrupts/no_op.goagent/go/internal/interrupts/no_op_test.goagent/go/internal/interrupts/node_restart.goagent/go/internal/interrupts/node_restart_test.goagent/go/internal/interrupts/restart_all_services.goagent/go/internal/interrupts/restart_all_services_test.goagent/go/internal/interrupts/script_interrupt.goagent/go/internal/interrupts/script_interrupt_test.goagent/go/internal/interrupts/service_restart.goagent/go/internal/interrupts/service_restart_test.goagent/go/internal/step/regular_step.goagent/go/internal/step/regular_step_linux_test.goagent/go/internal/step/regular_step_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
| - name: Run tests with coverage | ||
| working-directory: agent/go | ||
| run: make unit-tests merge-coverage | ||
| - name: Upload coverage artifact | ||
| if: hashFiles('agent/go/reporting/cover.out') != '' | ||
| uses: actions/upload-artifact@v5 | ||
| with: | ||
| name: coverage-agent-go-unit-tests | ||
| path: agent/go/reporting/cover.out | ||
| retention-days: 1 | ||
| if-no-files-found: ignore |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' .github/workflows/agent-go-ci.yaml | cat -n
printf '\n--- permissions search ---\n'
rg -n '^\s*permissions:|actions/upload-artifact|id-token|pull-requests|contents:' .github/workflows -g '*.yaml' -g '*.yml'Repository: NVIDIA/nodewright
Length of output: 6060
🏁 Script executed:
sed -n '1,220p' .github/workflows/agent-go-ci.yaml | cat -nRepository: NVIDIA/nodewright
Length of output: 2702
Add explicit least-privilege permissions.
Set permissions: contents: read for this workflow so GITHUB_TOKEN stays scoped to the checkout/test/upload steps.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 33-53: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 48-48: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/agent-go-ci.yaml around lines 43 - 53, Add an explicit
workflow-level permissions block to agent-go-ci.yaml granting only contents:
read. Place it alongside the workflow’s top-level configuration so the existing
checkout, test, and coverage artifact upload steps retain the required access
without broader GITHUB_TOKEN permissions.
Source: Linters/SAST tools
| run: make unit-tests merge-coverage | ||
| - name: Upload coverage artifact | ||
| if: hashFiles('agent/go/reporting/cover.out') != '' | ||
| uses: actions/upload-artifact@v5 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Show the relevant workflow section with line numbers
sed -n '1,140p' .github/workflows/agent-go-ci.yaml | cat -n
# Look for other action pinning patterns in GitHub workflows
printf '\n--- workflow action refs ---\n'
rg -n "uses:\s+actions/|uses:\s+[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+@" .github/workflows -g '*.yaml' -g '*.yml'
# Look for any project policy around pinning
printf '\n--- pinning policy hints ---\n'
rg -n "pin.*action|immutable commit|upload-artifact|zizmor|pinned" . -g '!*node_modules*' -g '!*.lock'Repository: NVIDIA/nodewright
Length of output: 14428
Pin actions/upload-artifact to an immutable commit SHA in .github/workflows/agent-go-ci.yaml:48. Replace @v5 with the approved full SHA and keep # v5 as a version hint.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 48-48: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/agent-go-ci.yaml at line 48, Update the
actions/upload-artifact step in the workflow to reference the approved immutable
full commit SHA instead of the floating v5 tag, while retaining an inline # v5
version hint.
Source: Linters/SAST tools
| It("cancels the running process group through context", func() { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| time.AfterFunc(100*time.Millisecond, cancel) | ||
|
|
||
| result, err := NewRunner().Run(ctx, helperCommand("wait")) | ||
|
|
||
| Expect(errors.Is(err, context.Canceled)).To(BeTrue()) | ||
| Expect(result.ExitCode).To(Equal(SignalExitCode)) | ||
| Expect(result.Signal).To(Equal(os.Signal(syscall.SIGKILL))) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Synchronize cancellation with helper startup.
The 100 ms timer can fire before the child starts on a loaded runner, producing no ProcessState and failing the SIGKILL assertions. Have the helper signal readiness before calling cancel.
🤖 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/command/process_test.go` around lines 53 - 61, Update the
cancellation test around NewRunner().Run and helperCommand("wait") to
synchronize cancellation with helper startup: make the helper signal readiness
once its process is running, wait for that signal, then call cancel instead of
using the fixed 100 ms timer. Preserve the existing context.Canceled,
SignalExitCode, and SIGKILL assertions.
| // Status reports whether an agent operation satisfied its execution policy. | ||
| type Status string | ||
|
|
||
| const ( | ||
| StatusSuccess Status = "success" | ||
| StatusFailed Status = "failed" | ||
| ) | ||
|
|
||
| // Config contains the filesystem and output composition for an agent operation. | ||
| type Config struct { | ||
| rootMount string | ||
| stepRoot string | ||
| skyhookDir string | ||
| stdout io.Writer | ||
| stderr io.Writer | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document the new execution contract before merge.
The shared Status and Config contracts are architectural changes, but no affected documentation appears in the PR stack.
As per coding guidelines, “User-visible or architectural behavior changes must update the affected documentation in the same PR.” <coding_guidelines>
🤖 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/execution/execution.go` around lines 29 - 44, Document the
architectural execution contract introduced by the Status type and Config
struct, including the possible operation statuses and the configuration fields
used for filesystem and output composition. Update the affected execution
documentation in the same change, using the symbols Status, StatusSuccess,
StatusFailed, and Config as the documented API.
Source: Coding guidelines
| // Interrupt is the contract satisfied by every interrupt type. | ||
| type Interrupt interface { | ||
| Type() string | ||
| InterruptCmd() [][]string | ||
| Type() InterruptType | ||
| Run(context.Context, execution.Config) (execution.Status, error) | ||
| Serialize() ([]byte, error) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document the interrupt execution-contract migration.
This replaces command-only interrupts with runtime execution, configuration, and status semantics, but no affected documentation update is included. Update the architecture/operator documentation in this PR. As per coding guidelines, “User-visible or architectural behavior changes must update the affected documentation in the same PR.”
🤖 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/interrupts/interrupts.go` around lines 48 - 52, Update the
architecture/operator documentation for the Interrupt interface and its
implementers to describe the migration from command-only interrupts to runtime
execution using execution.Config and execution.Status, including the Run
contract. Keep the documentation consistent with the Type, Run, and Serialize
methods exposed by Interrupt.
Source: Coding guidelines
| It("recognizes only SIGTERM as reboot completion", func() { | ||
| Expect(nodeRestartCompleted(command.Result{ | ||
| ExitCode: command.SignalExitCode, | ||
| Signal: syscall.SIGTERM, | ||
| })).To(BeTrue()) | ||
| Expect(nodeRestartCompleted(command.Result{ | ||
| ExitCode: command.SignalExitCode, | ||
| Signal: syscall.SIGKILL, | ||
| })).To(BeFalse()) | ||
| Expect(nodeRestartCompleted(command.Result{ExitCode: 15})).To(BeFalse()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the required -15 restart completion result.
The objective explicitly requires NodeRestart exit code -15 to succeed, but this test only accepts structured SIGTERM. Add the compatibility case and update nodeRestartCompleted if necessary; otherwise an expected reboot termination may be retried as a failure.
Proposed test coverage
Expect(nodeRestartCompleted(command.Result{
ExitCode: command.SignalExitCode,
Signal: syscall.SIGTERM,
})).To(BeTrue())
+ Expect(nodeRestartCompleted(command.Result{
+ ExitCode: -15,
+ })).To(BeTrue())📝 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("recognizes only SIGTERM as reboot completion", func() { | |
| Expect(nodeRestartCompleted(command.Result{ | |
| ExitCode: command.SignalExitCode, | |
| Signal: syscall.SIGTERM, | |
| })).To(BeTrue()) | |
| Expect(nodeRestartCompleted(command.Result{ | |
| ExitCode: command.SignalExitCode, | |
| Signal: syscall.SIGKILL, | |
| })).To(BeFalse()) | |
| Expect(nodeRestartCompleted(command.Result{ExitCode: 15})).To(BeFalse()) | |
| It("recognizes only SIGTERM as reboot completion", func() { | |
| Expect(nodeRestartCompleted(command.Result{ | |
| ExitCode: command.SignalExitCode, | |
| Signal: syscall.SIGTERM, | |
| })).To(BeTrue()) | |
| Expect(nodeRestartCompleted(command.Result{ | |
| ExitCode: -15, | |
| })).To(BeTrue()) | |
| Expect(nodeRestartCompleted(command.Result{ | |
| ExitCode: command.SignalExitCode, | |
| Signal: syscall.SIGKILL, | |
| })).To(BeFalse()) | |
| Expect(nodeRestartCompleted(command.Result{ExitCode: 15})).To(BeFalse()) |
🤖 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/interrupts/node_restart_test.go` around lines 35 - 44,
Update nodeRestartCompleted and its tests so a NodeRestart result with exit code
-15 is recognized as successful, while SIGKILL and unrelated exit codes remain
unsuccessful. Add an explicit compatibility assertion for the unstructured -15
result alongside the existing structured SIGTERM case.
| It("reports all missing argument environment variables before execution", func() { | ||
| stepRoot, skyhookDir, executable := prepareStepTestExecutable() | ||
| value := NewRegularStep( | ||
| filepath.Base(executable), | ||
| WithOnHost(false), | ||
| WithArguments([]string{"env:NODEWRIGHT_MISSING_ONE", "env:NODEWRIGHT_MISSING_TWO"}), | ||
| ) | ||
|
|
||
| status, err := value.Run(context.Background(), newStepRunConfig(stepRoot, skyhookDir)) | ||
|
|
||
| Expect(status).To(Equal(execution.StatusFailed)) | ||
| Expect(err).To(MatchError(ContainSubstring( | ||
| "expected environment variables do not exist: NODEWRIGHT_MISSING_ONE, NODEWRIGHT_MISSING_TWO", | ||
| ))) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make the missing-variable test independent of the parent environment.
If either named variable exists in CI, this test executes the command instead of exercising the missing-variable branch. Generate guaranteed-absent names or explicitly unset and restore them during cleanup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/go/internal/step/regular_step_test.go` around lines 232 - 245, Update
the missing-variable test around NewRegularStep and Run to use environment
variable names guaranteed not to exist in the parent environment, or explicitly
unset both variables before execution and restore their original values with
cleanup. Preserve the assertion that both missing names are reported before
command execution.
| test: unit-tests merge-coverage ## Run unit tests and report coverage. | ||
| $(GO) tool cover -func=$(REPORTING)/cover.out |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the cited Makefile region with line numbers.
nl -ba agent/go/Makefile | sed -n '1,140p'
# Find the coverage-related targets and references.
rg -n "^(test:|merge-coverage:|unit-tests:|reporting:|\.PHONY:|.*cover\.out|.*coverprofile)" agent/go/MakefileRepository: NVIDIA/nodewright
Length of output: 195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant Makefile section with line numbers.
cat -n agent/go/Makefile | sed -n '1,160p'
# Show only the coverage-related targets and references.
rg -n "^(test:|merge-coverage:|unit-tests:|reporting:|\.PHONY:|.*cover\.out|.*coverprofile)" agent/go/MakefileRepository: NVIDIA/nodewright
Length of output: 4792
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/Makefile" <<'EOF'
.PHONY: test a b
test: a b
`@echo` "test recipe"
a:
`@echo` "start a $$(date +%s%3N)"
`@sleep` 1
`@echo` "end a $$(date +%s%3N)"
b:
`@echo` "start b $$(date +%s%3N)"
`@sleep` 1
`@echo` "end b $$(date +%s%3N)"
EOF
echo "== make -j2 test =="
make -C "$tmpdir" -j2 test
echo
echo "== go tool cover on missing file =="
if go tool cover -func="$tmpdir/does-not-exist.out" >/tmp/cover.out 2>/tmp/cover.err; then
echo "unexpected success"
else
cat /tmp/cover.err
fi
echo
echo "== stale file scenario =="
printf 'mode: set\nfoo.go:1.1,1.2 1 1\n' > "$tmpdir/cover.out"
go tool cover -func="$tmpdir/cover.out"Repository: NVIDIA/nodewright
Length of output: 218
Sequence coverage merging after unit tests. In agent/go/Makefile:50-51,68-74, merge-coverage is an independent prerequisite of test, so a parallel make can run it before unit-tests produces any *.coverprofile files. That leaves cover.out missing or stale, and the final go tool cover reads the wrong result. Make the merge run after unit-tests and clear/guard $(REPORTING)/cover.out before reporting.
🤖 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 50 - 51, Update the Makefile test target and
merge-coverage dependency flow so merge-coverage always runs after unit-tests,
preventing parallel execution from reading incomplete profiles. Ensure
$(REPORTING)/cover.out is cleared or guarded before the go tool cover report,
while preserving the existing coverage reporting behavior.
| func (ServiceRestart) Type() string { return "service_restart" } | ||
| func (ServiceRestart) Type() InterruptType { return ServiceRestartType } | ||
|
|
||
| func (s ServiceRestart) Run(ctx context.Context, config execution.Config) (execution.Status, error) { |
There was a problem hiding this comment.
The exec path here is only covered by the root-gated interrupts_linux_test.go, which skips in CI (go tool cover shows Run at 0% for service_restart, node_restart, and restart_all_services). Separate from whether we make that skip a hard failure: the portable guard branches (nil ctx, ctx.Err(), config.Validate()) return before any command runs and need no root, yet these three types have no tests for them, unlike NoOp/ScriptInterrupt which do. Worth adding cheap portable It blocks for the nil-context and invalid-config paths on all three so a dropped Validate() cannot pass CI green.
| } | ||
|
|
||
| runner := command.NewRunner(command.WithChroot(config.RootMount())) | ||
| for index, cmd := range commands { |
There was a problem hiding this comment.
Parity note (likely out of scope for this PR, flagging for the follow-up wiring): the Python agent writes a per-command completion flag ({type}_{i}.complete) and skips already-run commands on retry, so a ServiceRestart that succeeds on service 1 but fails on service 2 skips the daemon-reload and service 1 on the next attempt. This loop re-runs daemon-reload and every service on each invocation. Benign since restarts are idempotent, but if partial-progress resumption ever matters, the per-command granularity lives in the orchestration layer and would need to be reproduced there.
| command.WithStderr(config.Stderr()), | ||
| ) | ||
| result, runErr := runner.Run(ctx, cmd) | ||
| if nodeRestartCompleted(result) && (runErr == nil || errors.Is(runErr, context.Canceled)) { |
There was a problem hiding this comment.
Parity note: Python do_interrupt treats the reboot/-15 case as a failure (returns exit 1) but deliberately does NOT remove the completion flag, so the post-reboot re-run sees the flag and skips the second reboot. Here Run returns StatusSuccess directly (cleaner), but the "do not reboot twice" invariant now depends on the out-of-scope orchestration layer persisting a completion flag whenever Run returns StatusSuccess, including this SIGTERM path. Just flagging so the follow-up that wires flags in preserves it.
2 participants
Description
Adds Go-native interrupt execution to the agent rewrite.
Closes #218.
Checklist