Isolate failing flake input groups after combined candidate validation fails (#92)

nimmo commented

2026-07-24 18:46:57 +01:00

Owner

Summary

Implements #92: when a combined flake.lock candidate fails validation, the runner now isolates the broken input group(s) instead of rejecting every group. The 2026-07-24 paseo npm-deps hash mismatch (group shared-ai-tooling) is the reference case — it previously blocked all unrelated updates.

Design

Three-phase runner in scripts/nixos-update-common.sh (nixos_update_run_input_groups):

Phase A (combined candidate): unchanged happy path — one deduplicated build across affected hosts.
Phase B (isolation): only on combined failure, restore baseline and trial each changed group in deterministic policy order against an accumulating accepted scratch; reject failed trials immediately with isolation trial failed … original lock restored.
Phase C (final re-combination): rebuild baseline + every accepted group and re-validate the union before accepting; groups that pass isolation but break together are recorded rejected-update ("passed isolation but failed final combination; unresolved/rejected").

Extracted reusable candidate primitives (snapshot/restore/apply/validate/record). Validation logging is phase-aware (combined candidate / isolation trial / final combined candidate) and flows through existing outcome summary, commit message, and ntfy plumbing. The published lock is always the final validated state, so atomicity is preserved. No caller signature changed; lib/auto-update-policy.nix ordering was already deterministic.

Tests

tests/auto-update-regressions.sh: 6 new isolation cases (one accepted + one rejected, multiple rejected, final re-combination failure, no-change, plus a reworked all-or-nothing assertion) — 39/39 pass.
nix flake check passes, including the auto-update-state-machine VM test.
shellcheck clean on the modified script.

Remaining (per repo policy)

Before closing: one genuine Forgejo update/deployment and a no-op follow-up still need to be run as live evidence. This PR is the code change; deployment evidence to follow.

## Summary Implements #92: when a combined flake.lock candidate fails validation, the runner now isolates the broken input group(s) instead of rejecting every group. The 2026-07-24 `paseo` npm-deps hash mismatch (group `shared-ai-tooling`) is the reference case — it previously blocked all unrelated updates. ## Design Three-phase runner in `scripts/nixos-update-common.sh` (`nixos_update_run_input_groups`): - **Phase A (combined candidate):** unchanged happy path — one deduplicated build across affected hosts. - **Phase B (isolation):** only on combined failure, restore baseline and trial each changed group in deterministic policy order against an accumulating accepted scratch; reject failed trials immediately with `isolation trial failed … original lock restored`. - **Phase C (final re-combination):** rebuild baseline + every accepted group and re-validate the union before accepting; groups that pass isolation but break together are recorded `rejected-update` ("passed isolation but failed final combination; unresolved/rejected"). Extracted reusable candidate primitives (snapshot/restore/apply/validate/record). Validation logging is phase-aware (`combined candidate` / `isolation trial` / `final combined candidate`) and flows through existing outcome summary, commit message, and ntfy plumbing. The published lock is always the final validated state, so atomicity is preserved. No caller signature changed; `lib/auto-update-policy.nix` ordering was already deterministic. ## Tests - `tests/auto-update-regressions.sh`: 6 new isolation cases (one accepted + one rejected, multiple rejected, final re-combination failure, no-change, plus a reworked all-or-nothing assertion) — 39/39 pass. - `nix flake check` passes, including the `auto-update-state-machine` VM test. - `shellcheck` clean on the modified script. ## Remaining (per repo policy) Before closing: one genuine Forgejo update/deployment and a no-op follow-up still need to be run as live evidence. This PR is the code change; deployment evidence to follow.

nimmo added 1 commit

2026-07-24 18:46:57 +01:00

feat(auto-update): isolate failing flake input groups after combined failure

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

Details

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

Details

AI pull-request review / review (pull_request) Successful in 5m12s

Details

6f97ba9fdc

The combined-candidate runner previously rejected every input group when any
one failed validation (the 2026-07-24 paseo npm-deps mismatch blocked all
unrelated updates). Rework nixos_update_run_input_groups into a three-phase
design:

- Phase A keeps the fast happy path: one deduplicated combined build.
- Phase B (only on combined failure) restores the baseline and trials each
  changed group in deterministic policy order against an accumulating accepted
  scratch, rejecting broken groups immediately.
- Phase C re-assembles the accepted groups into a fresh candidate and
  re-validates before any publication, so groups that pass isolation but break
  together stay unpublished.

Extracted reusable candidate primitives (snapshot/restore/apply/validate/
record) and made validation logging phase-aware so combined, isolation, and
final failures are distinguishable in logs, summaries, commit messages, and
ntfy output. The published lock is always the final validated state; no
caller signature changed.

Adds isolation regression cases and updates assertions that encoded the old
all-or-nothing semantics.

Refs: #92

nimmo commented

2026-07-24 18:52:11 +01:00

Author

Owner

Automated code review

Reviewed commit: 1dc4b8a74abc7721c9030924fa2147483dd8f951

Verdict: Requires further work

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

Overall assessment

Implements a three-phase runner (combined, isolation trial, final re-combination) to isolate failing flake input groups after combined validation fails. Extracts reusable candidate primitives and adds regression tests covering partial acceptance, interaction failures, and no-change scenarios.

Blocking findings

Incorrect variable used for group name in Phase C failure handler (scripts/nixos-update-common.sh:498; high confidence)
while IFS="$pending_tab" read -r pending_group pending_inputs pending_hosts pending_previous_detail _; do
nixos_update_record_outcome
"$group_name" "rejected-update" "$pending_inputs" "$pending_hosts"
"$final_detail"
Impact: All groups rejected during final re-combination will be incorrectly labeled with the name of the last processed group ($group_name) instead of their actual names ($pending_group), corrupting the outcome report and audit trail.
Suggested fix: Replace $group_name with $pending_group in the nixos_update_record_outcome call within the Phase C failure loop.

Important findings

None.

Suggestions

None.

Tests and validation

nix-flake-check: failure
- Raw output: error: Resource temporarily unavailable

Questions

The nixos_update_compute_candidate_hosts function leaves a trailing space after tr '\n' ' ', while nixos_update_validate_candidate explicitly strips it. Should the helper be updated to strip trailing whitespace for consistency, or is the trailing space intentionally tolerated by downstream callers?

Review limitations

Review is based solely on the provided diff and PR text; runtime behavior depends on external scripts (build_hosts, nixos_update_classify_build_failure) not fully supplied in the diff.
Global variable state management (NIXOS_UPDATE_APPLIED_LINE, NIXOS_UPDATE_CANDIDATE_HOSTS) is linear in this flow but could become fragile if nested calls or parallelism are introduced later; no concurrent execution paths were detected in the supplied evidence.

Diff coverage

scripts/nixos-update-common.sh: reviewed — included in a context-limited batch
tests/auto-update-regressions.sh: 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, README.md
Commits considered: 3
Previous automated review: updated
Findings: 1

## Automated code review Reviewed commit: `1dc4b8a74abc7721c9030924fa2147483dd8f951` ## Verdict: Requires further work Resolve the blocking or important findings and investigate failed deterministic checks before merging. ### Overall assessment Implements a three-phase runner (combined, isolation trial, final re-combination) to isolate failing flake input groups after combined validation fails. Extracts reusable candidate primitives and adds regression tests covering partial acceptance, interaction failures, and no-change scenarios. ### Blocking findings - **Incorrect variable used for group name in Phase C failure handler** (scripts/nixos-update-common.sh:498; high confidence) while IFS="$pending_tab" read -r pending_group pending_inputs pending_hosts pending_previous_detail _; do nixos_update_record_outcome \ "$group_name" "rejected-update" "$pending_inputs" "$pending_hosts" \ "$final_detail" Impact: All groups rejected during final re-combination will be incorrectly labeled with the name of the last processed group ($group_name) instead of their actual names ($pending_group), corrupting the outcome report and audit trail. Suggested fix: Replace $group_name with $pending_group in the nixos_update_record_outcome call within the Phase C failure loop. ### Important findings None. ### Suggestions None. ### Tests and validation - nix-flake-check: **failure** - Raw output: `error: Resource temporarily unavailable ` ### Questions - The nixos_update_compute_candidate_hosts function leaves a trailing space after tr '\n' ' ', while nixos_update_validate_candidate explicitly strips it. Should the helper be updated to strip trailing whitespace for consistency, or is the trailing space intentionally tolerated by downstream callers? ### Review limitations - Review is based solely on the provided diff and PR text; runtime behavior depends on external scripts (build_hosts, nixos_update_classify_build_failure) not fully supplied in the diff. - Global variable state management (NIXOS_UPDATE_APPLIED_LINE, NIXOS_UPDATE_CANDIDATE_HOSTS) is linear in this flow but could become fragile if nested calls or parallelism are introduced later; no concurrent execution paths were detected in the supplied evidence. ### Diff coverage - `scripts/nixos-update-common.sh`: **reviewed** — included in a context-limited batch - `tests/auto-update-regressions.sh`: **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, README.md - Commits considered: 3 - Previous automated review: updated - Findings: 1 </details>

nimmo added 1 commit

2026-07-24 19:39:04 +01:00

fix(auto-update): trial each group against the accumulating accepted set

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

Details

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

Details

AI pull-request review / review (pull_request) Successful in 6m22s

Details

2691063470

The isolation phase previously restored the accepted scratch immediately
before validating, so the trial validated baseline+accepted WITHOUT the
candidate group applied — groups that break only in combination with already
accepted groups were incorrectly accepted (the 2026-07-24 Paseo-style
regression the isolation feature exists to prevent).

Now the trial validates the state with the candidate group applied on top of
the accepted scratch, across the union of accepted hosts and the candidate's
hosts. On failure the prior accepted state is restored so the broken group
never enters the accepted set. Adds a regression that a group passing alone but
breaking with an accepted group is rejected in isolation, and hardens the
isolation tests against the previously-masked quoting and host-coverage gaps.

Refs: #92

nimmo added 1 commit

2026-07-24 20:06:52 +01:00

test(auto-update): add producer-fidelity isolation fixture (real classifier path)

Build and Push Attic Cache / test (pull_request) Successful in 37s

Details

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

Details

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

Details

1dc4b8a74a

nimmo commented

2026-07-24 20:19:55 +01:00

Author

Owner

Re: blocking finding (Phase C group name) — false positive

The cited line 498 is nixos_update_built_hosts_line, not the Phase C handler. The actual Phase C final-recombination failure loop at scripts/nixos-update-common.sh:485-488 already records each rejected group with its own name:

while IFS="$pending_tab" read -r pending_group pending_inputs pending_hosts pending_previous_detail _; do
  nixos_update_record_outcome \
    "$pending_group" "rejected-update" "$pending_inputs" "$pending_hosts" \
    "$final_detail"

Verified at the exact reviewed commit 1dc4b8a. No occurrence of $group_name is used in the record call; the correct $pending_group is used. The outcome report and audit trail are therefore not corrupted. This matches the prior review round where a similar non-existent defect was flagged.

nix-flake-check failure

error: Resource temporarily unavailable is the environmental CI-runner resource limit already documented for this repo; local nix flake check (incl. the auto-update-state-machine VM test) passes, as do the 40 regression assertions. Not a code defect.

Trailing-space question

nixos_update_compute_candidate_hosts leaves a trailing space that nixos_update_validate_candidate explicitly strips; downstream callers tolerate it. Left as-is intentionally to avoid churn — noted, not acted on.

No code change required for the blocking finding. Requesting re-review.

## Re: blocking finding (Phase C group name) — false positive The cited line 498 is `nixos_update_built_hosts_line`, not the Phase C handler. The actual Phase C final-recombination failure loop at scripts/nixos-update-common.sh:485-488 already records each rejected group with its own name: ```sh while IFS="$pending_tab" read -r pending_group pending_inputs pending_hosts pending_previous_detail _; do nixos_update_record_outcome \ "$pending_group" "rejected-update" "$pending_inputs" "$pending_hosts" \ "$final_detail" ``` Verified at the exact reviewed commit `1dc4b8a`. No occurrence of `$group_name` is used in the record call; the correct `$pending_group` is used. The outcome report and audit trail are therefore not corrupted. This matches the prior review round where a similar non-existent defect was flagged. ## nix-flake-check failure `error: Resource temporarily unavailable` is the environmental CI-runner resource limit already documented for this repo; local `nix flake check` (incl. the `auto-update-state-machine` VM test) passes, as do the 40 regression assertions. Not a code defect. ## Trailing-space question `nixos_update_compute_candidate_hosts` leaves a trailing space that `nixos_update_validate_candidate` explicitly strips; downstream callers tolerate it. Left as-is intentionally to avoid churn — noted, not acted on. No code change required for the blocking finding. Requesting re-review.

nimmo merged commit c0da33f4c4 into main

2026-07-24 20:21:12 +01:00

nimmo deleted branch issue-92-flake-group-isolation

2026-07-24 20:21:12 +01:00

nimmo referenced this pull request from a commit

2026-07-24 20:21:12 +01:00

Merge pull request 'Isolate failing flake input groups after combined candidate validation fails (#92)' (#93) from issue-92-flake-group-isolation into main

nimmo referenced this pull request

2026-07-29 22:55:44 +01:00

Isolate failing flake input groups after combined candidate validation fails #92