[alekspetrov]

Summary

Automated PR created by Pilot for task GH-6329.

Closes #6329

Changes

GitHub Issue GH-6329: fix(review): post-merge minors from PR #6325 for issue #6320

Context

The post-merge review of PR #6325 (merge of #6320) returned APPROVE-WITH-NOTES with 10 minor defects; 8 pass the filing filter and are listed under Implementation. Fix only the listed items; each names the file and line as the reviewer saw them at the merge commit. Review: #6325

Implementation

1. scripts/box/setup-agent-user.sh line 246 — in dry-run mode rm -f "$SUDOERS_TMP" runs outside run, so it is never printed, and it deletes a temp file left by an earlier interrupted run. That breaks issue §3 "--dry-run prints every command and changes nothing" in that state. Evidence: the line is a bare rm -f "$SUDOERS_TMP" while every other mutation at lines 247 to 256 goes through run/write_file.
2. scripts/box/setup-agent-user.sh line 475 — a failed step calls print_summary; exit 1 at once, so later steps get no summary line. Issue §4 asks for "A final summary line per step: done, skipped or failed". Evidence: the script header at line 42 says this on purpose: "the run stops at the first failed step".
3. scripts/box/setup-agent-user.sh line 216 — the shared-read idempotency check reads only the top-level group and mode of repos/bin. Files created later with a restrictive umask or another group stay unreadable to the group, and a second apply reports skipped. Evidence: design line 96 says "the agent group only reads and executes the binary (the gh shim needs it)".
4. scripts/box/setup-agent-user.sh line 294 — the restore after a failed visudo -c (and the undo restore at line 398) writes the cloud-init file back as 0440 root:root, not with the mode and owner it had before. Today that matches the usual cloud-init file, but the original metadata is not kept. Evidence: run install -m 0440 -o root -g root "$CLOUD_INIT_BACKUP" "$CLOUD_INIT_SUDOERS".
5. scripts/box/setup-agent-user.sh line 398 — the undo restore of the cloud-init file is not followed by visudo -c, unlike the apply path at line 292.
6. scripts/box/setup-agent-user.sh line 287 — an existing backup is reused without a fresh copy (if [ ! -e "$CLOUD_INIT_BACKUP" ]). If the cloud-init file changed after an earlier apply, the backup is stale, and undo at :398 restores the stale content.
7. scripts/box/setup-agent-user.sh line 305 — only IPv4 -d 127.0.0.1 --dport 9091 is rejected. The gateway on ::1 or on a host address is not covered when host: "0.0.0.0" is set (internal/config/config_test.go:289 shows that setting); design line 72 says "local port 9091". This meets the issue's literal 127.0.0.1 wording.
8. scripts/box/setup-agent-user_test.sh line 143 — the dry-run check grep -qF "+ visudo -c" also matches the + visudo -cf line, so it proves nothing about whether the cloud-init visudo -c is printed.

Add one test per item that fails without the change; put a comment // mutation pin: <item summary> -> this test fails directly above each test.

Acceptance

• Each listed item has a test with a mutation pin: comment above it in the test file.

Out of scope

scripts/box/setup-agent-user_test.sh line 303 — the mocks record arguments but cannot check real iptables/visudo/systemd syntax (for example, the meaning of -w -n -L CHAIN or visudo -cf). The issue asks for mocks and "A run on a real host" is out of scope, so this is a known limit, not a defect in the contract under test (dropped: out of scope)
PR — body (dropped: no file location)
No change to any file not named above.

Refs

• GH-6320: feat(box): host setup script for the agent user — user, group, home, caches, sudoers rule, firewall rules, with dry-run, undo and a mocked-PATH test #6325
• Issue feat(box): host setup script for the agent user — user, group, home, caches, sudoers rule, firewall rules, with dry-run, undo and a mocked-PATH test #6320

Pre-PR review

Mode: enforce
Verdict: APPROVE-WITH-NOTES
Fix: none

Defects:

• [minor] at scripts/box/setup-agent-user.sh:20 — the script header is out of date: it still says firewall- rejects "the metadata address and TCP to 127.0.0.1:9091". firewall_rules now emits a ::1 rule and an addrtype LOCAL rule
• [minor] at scripts/box/setup-agent-user.sh:321 — every apply now replaces the backup without checking it. Suppose cloud-init adds a new daemon line but keeps the line this script already commented out. The new backup then holds "# disabled by setup-agent-user: ..." text, and undo at :435 restores the commented line instead of the original rule
• [minor] at scripts/box/setup-agent-user.sh:213 — shared_read_ok forks one stat for every entry under repos/, which holds full clones, so a second apply on a large tree is slow
• [minor] at scripts/box/setup-agent-user_test.sh:461 — the shared-read test relies on the mock chown keeping the group only for the top path, so it cannot show that chown -R fixes nested entries

Not-verified:

• Whether real ip6tables accepts -m addrtype --dst-type LOCAL and --reject-with tcp-reset on the target host kernel. The mocks at scripts/box/testdata/mock-host-cmds.sh:48-50 only record rule text (a known limit, out of scope)
• Item 4 met: restore_cloud_init at :290-295 reads %a:%U:%G from the live file and falls back to 440:root:root. Both the apply restore at :326 and the undo restore at :435 use it. Because sed -i keeps the original mode and owner, the live file still has them at restore time. Test checks install -m 640 -o root -g wheel on both paths. — not traced
• Item 5 met: undo runs visudo -c at :437 after the restore and keeps the backup when validation fails (:438-439). The test checks the order in CALL_LOG, the failed state and that the backup is kept. — not traced
• … 2 more not-verified items; 2 requirements marked met but not traced to a file read omitted (capped at 3)

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!