feat: small improvements to BuilderNet recipe

[fkondej]

No description provided.

[claude]

Claude finished @fkondej's task in 1m 35s —— View job

---

Review: PR #386 — Small improvements to BuilderNet recipe

• Read and analyze all changed files
• Review prepare.sh changes
• Review start.sh changes
• Review main.go changes
• Review local_runner_lifecycle.go changes
• Review test_tx.go changes
• Post inline comments for issues found
• Post final review summary

---

Summary

This PR makes five improvements to the BuilderNet recipe: flexible image sourcing in prepare.sh, configurable QEMU acceleration/resources in start.sh, switching from -daemonize to exec for better process tracking, using Start()+Wait() instead of Run() in a goroutine for lifecycle hooks, and adding an --expected-extra-data validation flag to the test command.

Key findings

local_runner_lifecycle.go — Start()/Wait() change (the most important change)
The switch from Run() in a goroutine to Start() + Wait() is a good fix for the --detached mode race. With the old code, the goroutine calling Run() might not have started the process before the main function returned, leaving the process unlaunched. The new approach ensures the process is started before returning. Logic looks correct.

start.sh — exec replacing -daemonize
Switching to exec (foreground) means the playground's process handle directly references the QEMU process, making cleanup reliable. The -pidfile flag is still written by QEMU itself, so that's fine. Dead code cleanup of the commented-out service masking is welcome.

prepare.sh — URL download and flexible sourcing
Partial download risk and missing trailing newline — see inline comments.

start.sh — Unquoted QEMU_ACCEL_ARGS
Word-splitting reliance is fragile; bash array is the idiomatic fix — see inline comment.

test_tx.go — Extra data validation
Works for ASCII/UTF-8 extra data but block.Extra() returns raw bytes — see inline comment.

main.go — Flag additions
Formatting alignment and new --expected-extra-data flag look correct. No issues.

4 inline comments posted on specific issues.

[claude]

Claude finished @fkondej's task in 1m 59s —— View job

---

Review: PR #386 — Small improvements to BuilderNet recipe (re-review after f8bed14)

• Fetch latest changes and diff against origin/main
• Review prepare.sh changes
• Review start.sh changes
• Review main.go changes
• Review local_runner_lifecycle.go changes
• Review test_tx.go changes
• Post inline comments for any issues found
• Post final review summary

---

Summary

All four issues from the previous review have been addressed in f8bed14:

1. Trailing newline in prepare.sh — Fixed
2. Partial download protection — Fixed (temp file + mv pattern)
3. QEMU_ACCEL_ARGS bash array — Fixed (proper array with "${QEMU_ACCEL_ARGS[@]}")
4. Extra data hex comparison in test_tx.go — Fixed (uses hex.EncodeToString on both sides)

No new issues found. The code looks clean and correct across all five changed files. LGTM.

---

[claude]

Claude finished @fkondej's task in 1m 56s —— View job

---

Review: PR #386 — Small improvements to BuilderNet recipe (re-review after 53466fb)

• Fetch latest diff against origin/main
• Review all 7 changed files
• Post inline comments for issues found
• Post final review summary

---

Summary

All four issues from the first review were addressed in f8bed14. Two new commits landed since the last review:

• 20fa218 — make ./prepare.sh a manual step (removes init hooks from playground.yaml)
• 53466fb — rewrite README.md with updated usage instructions

The code changes (lifecycle hooks, shell scripts, test command) all look correct and clean.

Issues

1 issue found:

• README.md missing trailing newline — The fix from f8bed14 was lost when the README was rewritten in 53466fb. See inline comment.

No other issues. The Start()/Wait() lifecycle fix, exec switch, QEMU accel array, hex comparison, and manual prepare step all look good.

---

[claude]

Claude finished @fkondej's task in 1m 51s —— View job

---

Review: PR #386 — Small improvements to BuilderNet recipe (re-review after f71b9a6)

• Fetch latest diff against origin/main
• Review all 7 changed files
• Post inline comments for issues found
• Post final review summary

---

Summary

All four issues from the first review have been addressed in f8bed14 and f71b9a6:

1. Trailing newline in prepare.sh — Fixed
2. Partial download protection — Fixed (temp file + mv pattern)
3. QEMU_ACCEL_ARGS bash array — Fixed (proper array with "${QEMU_ACCEL_ARGS[@]}")
4. Extra data hex comparison in test_tx.go — Fixed (uses hex.EncodeToString on both sides)
5. README trailing newline — Fixed (previously lost in 53466fb, now restored in f71b9a6)

No new issues found. The Start()/Wait() lifecycle fix, exec switch, QEMU accel array, hex comparison, manual prepare step, and rewritten README all look correct. LGTM.

---

[claude]

Claude finished @fkondej's task in 4m 31s —— View job

---

Review: PR #386 — Small improvements to BuilderNet recipe (re-review after 89b794b)

• Fetch latest diff against origin/main
• Review all 7 changed files
• Post inline comments for issues found
• Post final review summary

---

Summary

All four issues from the first review have been addressed across f8bed14 and f71b9a6:

1. Trailing newline in prepare.sh — Fixed
2. Partial download protection — Fixed (temp file + mv pattern)
3. QEMU_ACCEL_ARGS bash array — Fixed (proper array with "${QEMU_ACCEL_ARGS[@]}")
4. Extra data hex comparison in test_tx.go — Fixed (uses hex.EncodeToString on both sides)
5. README trailing newline — Fixed (previously lost in 53466fb, now restored in f71b9a6)

The latest commit 89b794b re-adds the init section to playground.yaml with BUILDERNET_IMAGE env var support. One minor nit on this — see inline comment.

The Start()/Wait() lifecycle fix, exec switch, QEMU accel array, env var passthrough to lifecycle hooks, hex comparison, and rewritten README all look correct. No blocking issues.

---

[claude]

Claude finished @fkondej's task in 2m 59s —— View job

---

Review: PR #386 — Small improvements to BuilderNet recipe (re-review after cd598d7)

• Fetch latest diff against origin/main
• Review all 7 changed files
• Post inline comments for issues found
• Post final review summary

---

Summary

All four issues from the first review have been addressed across f8bed14 and f71b9a6:

1. Trailing newline in prepare.sh — Fixed
2. Partial download protection — Fixed (temp file + mv pattern)
3. QEMU_ACCEL_ARGS bash array — Fixed (proper array with "${QEMU_ACCEL_ARGS[@]}")
4. Extra data hex comparison in test_tx.go — Fixed (uses hex.EncodeToString on both sides)
5. README trailing newline — Fixed (previously lost in 53466fb, now restored in f71b9a6)

The latest commit cd598d7 adds ./scripts/stop.sh to prepare.sh before rm -rf (ensuring the VM is stopped before replacing runtime files) and removes the now-redundant rm -rf .runtime/buildernet-vm.qcow2 || true from the init section in playground.yaml. This is a clean improvement that addresses the earlier nit about the redundant rm.

No new issues found. All changes look correct:

• local_runner_lifecycle.go: Start()/Wait() pattern and env var passthrough logic are both correct
• start.sh: exec switch, QEMU accel array, configurable CPU/RAM — all clean
• prepare.sh: Stop-before-replace, URL download with temp file, flexible sourcing — all clean
• test_tx.go: Hex-based extra data comparison works correctly for the intended use case
• main.go: Flag additions are correctly wired up
• playground.yaml: BUILDERNET_IMAGE env + simplified init section is clean
• README.md: Comprehensive and well-structured rewrite

LGTM.

---