Add unified BuilderNet mkosi recipe

[canercidam]

(Fixing merge issues and avoiding commit loss)

[claude]

Claude finished @canercidam's task in 5m 9s —— View job

---

PR Review: Add unified BuilderNet mkosi recipe

• Gather context and understand changes
• Review Go source changes
• Review new Go files and tests
• Review shell scripts
• Review recipe changes
• Post review feedback

---

Summary

This PR adds lifecycle hooks support to the YAML recipe system and a BuilderNet mkosi custom recipe that uses QEMU VMs. The architecture is well-structured — lifecycle hooks (init/start/stop) are a clean abstraction for managing non-Docker services. The test coverage for the new lifecycle and validation code is solid.

Key Issues

Bugs / Correctness:

1. main.go:436 — Generated YAML files get 0o755 permissions (executable) instead of 0o644. YAML config files don't need execute bits.

2. recipe_buildernet.go:162-170 — registerBuilder accepts ctx context.Context but never uses it. All HTTP calls use http.Get/http.Post instead of http.NewRequestWithContext, so cancellation doesn't propagate.

3. local_runner.go:1224-1240 — RunLifecycleHooks() uses new(errgroup.Group) (no context cancellation), while Run() was updated to use errgroup.WithContext(ctx). If one lifecycle service fails, siblings continue running.

Architecture / Ordering:

4. main.go:761-786 — WaitForReady + ExecutePostHookActions moved before the interactive output block, so users see no output during health check wait. Also, RunLifecycleHooks() has no timeout — if QEMU fails to start, the user gets no feedback.

5. local_runner.go:1213 — Lifecycle services run after Run() completes and WaitForReady() succeeds. This two-phase startup is fine but should be documented since it's a non-obvious ordering dependency.

6. local_runner_lifecycle.go:112-120 — Race between context cancellation and mainctx.IsExiting() check. Consider also checking ctx.Err() != nil before sending exit error.

Security / Config:

7. custom-recipes/buildernet/mkosi/sync.sh:8 — Hardcoded branch fryd/mkosi-playground for flashbots-images repo. This should be updated to the target branch before merging.

8. recipe_buildernet.go:243-247 — Same config JSON sent as both "builder configuration" and "builder secrets". Worth confirming this is intentional.

9. utils/builderhub-config.yaml:14-18 — relay_secret_key and coinbase_secret_key lack test-only comments (unlike disk_encryption.key which has one).

Minor:

10. custom-recipes/buildernet/mkosi/start.sh:63-83 — 20 lines of dead commented-out code should be cleaned up.

11. recipe_buildernet.go:75 — DependsOnNone() clears all dependencies from mev-boost-relay. If upstream adds dependencies later, they'll be silently dropped.

See inline comments for details and suggested fixes.

[claude]

PR Review: Add unified BuilderNet mkosi recipe

• Gather context and understand changes
• Review Go source changes (components, manifest, recipes, runner)
• Review new validation command and tests
• Review shell scripts and mkosi recipe
• Review test changes
• Post review feedback

---

Summary

This PR adds a lifecycle hooks system to the YAML recipe layer, enabling non-Docker services (like QEMU VMs) to be managed via init/start/stop shell commands. It also introduces a Fileserver component, a validate command, and the BuilderNet mkosi recipe that uses these features.

Architecture

The lifecycle hooks design is sound: services declare lifecycle_hooks: true in YAML with init/start/stop commands that run on the host. This fits well within the existing component/recipe model. The separation into local_runner_lifecycle.go is clean.

Issues Found

Bugs/Correctness:

1. Stop ordering (local_runner.go:289-292): stopAllProcessesWithSignal(os.Kill) kills lifecycle start processes before stop commands run. For non-daemonizing start commands, stop hooks become pointless since the process is already dead. See inline comment

2. Shell quoting in start.sh (start.sh:25-28): Unquoted $(cat) in PID check can break with unexpected pidfile content. See inline comment

3. Unused context parameter (components.go:1074-1098): ctx context.Context is accepted by registerBuilderHook but HTTP calls don't use it — no cancellation propagation. See inline comment

Design/Correctness concerns:
4. Lifecycle readiness gap (local_runner.go:1227-1239): RunLifecycleHooks runs after WaitForReady, but lifecycle services' own readiness checks aren't verified before reporting "All services started!". See inline comment

5. Overly permissive file permissions (custom_recipes.go:237): All extracted files get 0o755 including YAML and README files. See inline comment

6. Lifecycle fields in manifest JSON (manifest.go:407-417): Init, Start, Stop, RecipeDir are serialized into the JSON manifest but are only meaningful at runtime. Consider json:"-" like other runtime-only fields. See inline comment

Cleanup:
7. Dead code in start.sh (start.sh:62-83): 20+ lines of commented-out debug notes should be removed before merging. See inline comment

8. send-tx.sh missing safety flags: No set -eu unlike other scripts in the directory. See inline comment

Minor:
9. Validation gap (cmd_validate.go:112-140): replace_args isn't checked for incompatibility with lifecycle_hooks. See inline comment

10. Security documentation (local_runner_lifecycle.go:30): Lifecycle hooks execute arbitrary shell commands via sh -c — worth documenting the trust boundary. See inline comment

Tests

The new tests are well-structured and thorough. The lifecycle test coverage is good with tests for init success/failure, start commands, stop command continuation on error, and validation edge cases. The YAML recipe tests cover the new lifecycle parsing and application paths.

Pre-existing issue (not from this PR)

The Validate() method in manifest.go:323-349 contains a duplicated file-mount validation block — the exact same loop appears twice in sequence.

---

Note: I was unable to run make test or make lint due to tool permission restrictions in this environment.

View job run