nimmo/nixos-config

Watch

Fork 0

feat(nix): add controller-to-target test workflow #246

Merged

nimmo merged 2 commits from feat/issue-236-electra-nova-vega into main

2026-09-20 11:07:13 +01:00

nimmo commented

2026-09-20 10:47:30 +01:00

Owner

Refs #236

Adds a test-only Electra controller -> Nova builder -> Vega target workflow, including a controller-only signing key for verified closure transfer.

Local validation: nix flake check, shell syntax, and existing deploy regression suites. Live canary evidence remains required before merge.

Refs #236 Adds a test-only Electra controller -> Nova builder -> Vega target workflow, including a controller-only signing key for verified closure transfer. Local validation: `nix flake check`, shell syntax, and existing deploy regression suites. Live canary evidence remains required before merge.

nimmo added 2 commits

2026-09-20 10:47:30 +01:00

feat(nix): add controller-to-target test workflow bfcd524280

feat(nix): sign controller closure transfers

AI pull-request review / review (pull_request) Successful in 1m23s

Details

Build and Push Attic Cache / test (pull_request) Successful in 1m31s

Details

Build and Push Attic Cache / build (pull_request) Has been skipped

Details

Build and Push Attic Cache / pin-lifecycle (pull_request) Has been skipped

Details

Build and Push Attic Cache / remediate (pull_request) Has been skipped

Details

Build and Push Attic Cache / notify-remediation (pull_request) Has been skipped

Details

Build and Push Attic Cache / record-remediation-notify-failure (pull_request) Has been skipped

Details

912ac4aff4

nimmo commented

2026-09-20 10:48:55 +01:00

Author

Owner

Automated code review

Reviewed commit: 30c8e27a55849b4b366e12aab4ad474a670e2ce7

Verdict: Requires further work

Resolve the blocking or important findings and investigate failed deterministic checks before merging.

Overall assessment

The PR introduces a controller-to-target test workflow allowing Electra to build a NixOS configuration for Vega on the Nova remote builder, sign the resulting closure with a dedicated Electra key, and activate it on Vega using switch-to-configuration test. It introduces a new SOPS-managed signing key, updates common Nix settings to trust this key, and adds documentation and Justfile recipes.

Blocking findings

Script fails if target host is unreachable or SSH connection drops during copy/activation (scripts/nixos-controller-test.sh:98; high confidence)
The script uses set -euo pipefail and chains commands like nix copy and ssh ... switch-to-configuration test. If the SSH connection to the target drops during the large closure copy or activation, the script exits with an error, leaving the target in a potentially inconsistent state (e.g., partially copied files) or simply failing without cleanup of the temporary snapshot.
Impact: The test workflow is fragile and may leave temporary artifacts or fail silently on network issues, making debugging difficult.
Suggested fix: Add explicit error handling for nix copy and ssh commands. Consider adding a timeout to the SSH command and ensuring the temporary snapshot directory is cleaned up even if intermediate steps fail (the trap handles this, but the user experience is poor).

Important findings

Hardcoded Nova hostname in script and documentation (scripts/nixos-controller-test.sh:85; high confidence)
The script hardcodes nova in the builder string: --option builders "ssh://nix-remote-builder@nova ...". The documentation also refers to Nova as the remote builder.
Impact: This couples the controller test workflow to a specific host named 'nova'. If the remote builder hostname changes or if this workflow is adapted for other hosts, the script must be modified. It reduces flexibility.
Suggested fix: Make the remote builder hostname configurable via an environment variable (e.g., NIXOS_CONTROLLER_REMOTE_BUILDER_HOST) with a default of 'nova'.

Suggestions

Missing validation of target host's NixOS version compatibility (scripts/nixos-controller-test.sh:1; medium confidence)
The script builds a configuration for $target and copies it. It does not check if the target host is running a compatible NixOS version or if the switch-to-configuration binary is present.
Impact: If the target host is not running NixOS or is running a significantly different version, the activation step may fail in unexpected ways.
Suggested fix: Add a preliminary check to verify that the target host is reachable and has a compatible NixOS installation before attempting the build and copy.
Secret key paths rely on environment variables with defaults (scripts/nixos-controller-test.sh:75; medium confidence)
The script uses NIXOS_CONTROLLER_REMOTE_BUILDER_KEY and NIXOS_CONTROLLER_SIGNING_KEY with defaults pointing to /run/secrets/.... These paths assume SOPS has decrypted the secrets into these specific locations.
Impact: If the secrets are not deployed to these exact paths, the script will fail. This is a valid assumption given the host configuration, but it's worth noting that the script is tightly coupled to the SOPS deployment structure.
Suggested fix: Ensure the documentation clearly states that these secrets must be present at the default locations or that the environment variables must be set accordingly.

Tests and validation

No configured deterministic checks.

Questions

Why is nom used instead of nix for the build command in the script?
Is there any mechanism to verify the signature of the closure on the target host before activation?

Review limitations

I cannot run nix flake check or test the script locally.
I cannot verify the live deployment evidence mentioned in the PR description.
I am assuming the SOPS secrets are correctly deployed to the expected paths.

Diff coverage

.sops.yaml: reviewed — included in a context-limited batch
README.md: reviewed — included in a context-limited batch
docs/nova-remote-builder.md: reviewed — included in a context-limited batch
hosts/electra/default.nix: reviewed — included in a context-limited batch
justfiles/deploy.just: reviewed — included in a context-limited batch
modules/common/nix-settings.nix: reviewed — included in a context-limited batch
scripts/nixos-controller-test.sh: reviewed — included in a context-limited batch
secrets/nix-controller/electra-signing-key.json: reviewed — included in a context-limited batch

Review metadata

Reviewer: forgejo-ai-review v1
Model: qwen3.6:35b-a3b-q8_0
Guidance loaded: .forgejo/ai-review.md, .forgejo/ai-review.yml, AGENTS.md, docs/auto-update-remediation.md, docs/nova-remote-builder.md, README.md
Commits considered: 2
Previous automated review: updated
Findings: 4

## Automated code review Reviewed commit: `30c8e27a55849b4b366e12aab4ad474a670e2ce7` ## Verdict: Requires further work Resolve the blocking or important findings and investigate failed deterministic checks before merging. ### Overall assessment The PR introduces a controller-to-target test workflow allowing Electra to build a NixOS configuration for Vega on the Nova remote builder, sign the resulting closure with a dedicated Electra key, and activate it on Vega using `switch-to-configuration test`. It introduces a new SOPS-managed signing key, updates common Nix settings to trust this key, and adds documentation and Justfile recipes. ### Blocking findings - **Script fails if target host is unreachable or SSH connection drops during copy/activation** (scripts/nixos-controller-test.sh:98; high confidence) The script uses `set -euo pipefail` and chains commands like `nix copy` and `ssh ... switch-to-configuration test`. If the SSH connection to the target drops during the large closure copy or activation, the script exits with an error, leaving the target in a potentially inconsistent state (e.g., partially copied files) or simply failing without cleanup of the temporary snapshot. Impact: The test workflow is fragile and may leave temporary artifacts or fail silently on network issues, making debugging difficult. Suggested fix: Add explicit error handling for `nix copy` and `ssh` commands. Consider adding a timeout to the SSH command and ensuring the temporary snapshot directory is cleaned up even if intermediate steps fail (the trap handles this, but the user experience is poor). ### Important findings - **Hardcoded Nova hostname in script and documentation** (scripts/nixos-controller-test.sh:85; high confidence) The script hardcodes `nova` in the builder string: `--option builders "ssh://nix-remote-builder@nova ..."`. The documentation also refers to Nova as the remote builder. Impact: This couples the controller test workflow to a specific host named 'nova'. If the remote builder hostname changes or if this workflow is adapted for other hosts, the script must be modified. It reduces flexibility. Suggested fix: Make the remote builder hostname configurable via an environment variable (e.g., `NIXOS_CONTROLLER_REMOTE_BUILDER_HOST`) with a default of 'nova'. ### Suggestions - **Missing validation of target host's NixOS version compatibility** (scripts/nixos-controller-test.sh:1; medium confidence) The script builds a configuration for `$target` and copies it. It does not check if the target host is running a compatible NixOS version or if the `switch-to-configuration` binary is present. Impact: If the target host is not running NixOS or is running a significantly different version, the activation step may fail in unexpected ways. Suggested fix: Add a preliminary check to verify that the target host is reachable and has a compatible NixOS installation before attempting the build and copy. - **Secret key paths rely on environment variables with defaults** (scripts/nixos-controller-test.sh:75; medium confidence) The script uses `NIXOS_CONTROLLER_REMOTE_BUILDER_KEY` and `NIXOS_CONTROLLER_SIGNING_KEY` with defaults pointing to `/run/secrets/...`. These paths assume SOPS has decrypted the secrets into these specific locations. Impact: If the secrets are not deployed to these exact paths, the script will fail. This is a valid assumption given the host configuration, but it's worth noting that the script is tightly coupled to the SOPS deployment structure. Suggested fix: Ensure the documentation clearly states that these secrets must be present at the default locations or that the environment variables must be set accordingly. ### Tests and validation - No configured deterministic checks. ### Questions - Why is `nom` used instead of `nix` for the build command in the script? - Is there any mechanism to verify the signature of the closure on the target host before activation? ### Review limitations - I cannot run `nix flake check` or test the script locally. - I cannot verify the live deployment evidence mentioned in the PR description. - I am assuming the SOPS secrets are correctly deployed to the expected paths. ### Diff coverage - `.sops.yaml`: **reviewed** — included in a context-limited batch - `README.md`: **reviewed** — included in a context-limited batch - `docs/nova-remote-builder.md`: **reviewed** — included in a context-limited batch - `hosts/electra/default.nix`: **reviewed** — included in a context-limited batch - `justfiles/deploy.just`: **reviewed** — included in a context-limited batch - `modules/common/nix-settings.nix`: **reviewed** — included in a context-limited batch - `scripts/nixos-controller-test.sh`: **reviewed** — included in a context-limited batch - `secrets/nix-controller/electra-signing-key.json`: **reviewed** — included in a context-limited batch <details><summary>Review metadata</summary> - Reviewer: forgejo-ai-review v1 - Model: qwen3.6:35b-a3b-q8_0 - Guidance loaded: .forgejo/ai-review.md, .forgejo/ai-review.yml, AGENTS.md, docs/auto-update-remediation.md, docs/nova-remote-builder.md, README.md - Commits considered: 2 - Previous automated review: updated - Findings: 4 </details>

nimmo force-pushed feat/issue-236-electra-nova-vega from 912ac4aff4

AI pull-request review / review (pull_request) Successful in 1m23s

Details

Build and Push Attic Cache / test (pull_request) Successful in 1m31s

Details

Build and Push Attic Cache / build (pull_request) Has been skipped

Details

Build and Push Attic Cache / pin-lifecycle (pull_request) Has been skipped

Details

Build and Push Attic Cache / remediate (pull_request) Has been skipped

Details

Build and Push Attic Cache / notify-remediation (pull_request) Has been skipped