Enable and validate leaf-package remediation proposals #210

Merged

nimmo merged 9 commits from fix/remediation-leaf-proposals into main 2026-09-09 21:38:45 +01:00

Conversation 1 Commits 9 Files changed 10 +467 -34

nimmo commented 2026-09-09 18:23:33 +01:00

Owner

Copy link

Historical Moonlight failures could not reach an accepted temporary-pin proposal: the incident misidentified failed builders, the worker lacked package/dependency context and an allowed leaf target, and real Nix behavior exposed validation gaps hidden by fixtures. The controller now supplies bounded baseline/candidate recipes and dependency alternatives, identifies existing editable leaf files, computes proposal checksums, canonicalizes diff hunks, separates JSON stdout from diagnostics, and avoids Nix outPath coercion in package proofs. Moonlight has a dedicated leaf expression that preserves its current upstream derivation and override behavior.

Refs #208; milestone: Automated Update Remediation. #207 is merged; this MR now targets main and uses merged updater MR #2. The lock pins updater main at merge a32a0ba5a1563e10dcc41debfd31424a2020f89c.

Validation:

• Full flake check and remediation regression gate pass with the published helper pin; pin lifecycle regressions pass. Tests cover wrong evidence targets, incorrect advisory hunk counts, Nix stderr warnings, outPath JSON coercion, and canonical patch/removal metadata.
• A real-model-selected Moonlight-only FFmpeg 8 override reached validated-ready in a disposable reduced flake: baseline built, actual candidate compile failure reproduced, patched package built, flake/closure/exact-package proofs passed. Captured model output was replayed through the repaired validator without semantic changes.
• The controller-generated removal patch restores the original file exactly; the restored unpinned package builds on a newer real nixpkgs revision. Details and revision identities are recorded in #208.

Remaining evidence: the reduced flake exposes one real package as a synthetic target; it does not establish fleet/production-host validation. Required live deployment and no-op follow-up remain before issue closure. Model explanatory prose still needs human review. No merge, deployment, or production package pin was applied.

Integration update: merged config main into head c75d17d1 and resolved flake.lock using current main plus updater main at MR #2's merge commit. Full flake check, remediation regressions, and pin-lifecycle regressions pass on this exact tree. The automated review claim that model.py lacks import time is a false positive: import time is present and package_evidence/time.monotonic is exercised by the passing integration test.

Automated-review follow-up: a71b8be addresses the retained inherited-stderr-pipe finding. Reader cleanup now terminates children that retain the isolated command process group's descriptors, then explicitly closes stdout/stderr as a bounded final fallback. The regression launches a child that holds both pipes for 30 seconds and proves the parent result returns in under five seconds. The earlier missing-time-import finding was false (import time is present and exercised). The inline Nix projection and fake-command organization remain advisory maintenance suggestions; package evidence fails closed when projection evaluation is unavailable, and real-Nix replay evidence is recorded in #208.

Final exact-head review (ee73c54): CI and automated review jobs pass. The reviewer retained one “important” item but explicitly concluded “The current implementation is robust. No change needed.” The described fallback occurs only after the command leader has exited, then TERM and KILL of its isolated process group have failed to close inherited pipes; closing the controller's read descriptors cannot signal the already-exited leader. A regression covers a live descendant retaining both descriptors. Remaining findings are advisory and either already policy-backed or explicitly handled.

Historical Moonlight failures could not reach an accepted temporary-pin proposal: the incident misidentified failed builders, the worker lacked package/dependency context and an allowed leaf target, and real Nix behavior exposed validation gaps hidden by fixtures. The controller now supplies bounded baseline/candidate recipes and dependency alternatives, identifies existing editable leaf files, computes proposal checksums, canonicalizes diff hunks, separates JSON stdout from diagnostics, and avoids Nix outPath coercion in package proofs. Moonlight has a dedicated leaf expression that preserves its current upstream derivation and override behavior. Refs #208; milestone: Automated Update Remediation. #207 is merged; this MR now targets main and uses merged updater MR #2. The lock pins updater main at merge a32a0ba5a1563e10dcc41debfd31424a2020f89c. Validation: - Full flake check and remediation regression gate pass with the published helper pin; pin lifecycle regressions pass. Tests cover wrong evidence targets, incorrect advisory hunk counts, Nix stderr warnings, outPath JSON coercion, and canonical patch/removal metadata. - A real-model-selected Moonlight-only FFmpeg 8 override reached validated-ready in a disposable reduced flake: baseline built, actual candidate compile failure reproduced, patched package built, flake/closure/exact-package proofs passed. Captured model output was replayed through the repaired validator without semantic changes. - The controller-generated removal patch restores the original file exactly; the restored unpinned package builds on a newer real nixpkgs revision. Details and revision identities are recorded in #208. Remaining evidence: the reduced flake exposes one real package as a synthetic target; it does not establish fleet/production-host validation. Required live deployment and no-op follow-up remain before issue closure. Model explanatory prose still needs human review. No merge, deployment, or production package pin was applied. Integration update: merged config main into head c75d17d1 and resolved flake.lock using current main plus updater main at MR #2's merge commit. Full flake check, remediation regressions, and pin-lifecycle regressions pass on this exact tree. The automated review claim that model.py lacks import time is a false positive: import time is present and package_evidence/time.monotonic is exercised by the passing integration test. Automated-review follow-up: a71b8be addresses the retained inherited-stderr-pipe finding. Reader cleanup now terminates children that retain the isolated command process group's descriptors, then explicitly closes stdout/stderr as a bounded final fallback. The regression launches a child that holds both pipes for 30 seconds and proves the parent result returns in under five seconds. The earlier missing-time-import finding was false (`import time` is present and exercised). The inline Nix projection and fake-command organization remain advisory maintenance suggestions; package evidence fails closed when projection evaluation is unavailable, and real-Nix replay evidence is recorded in #208. Final exact-head review (`ee73c54`): CI and automated review jobs pass. The reviewer retained one “important” item but explicitly concluded “The current implementation is robust. No change needed.” The described fallback occurs only after the command leader has exited, then TERM and KILL of its isolated process group have failed to close inherited pipes; closing the controller's read descriptors cannot signal the already-exited leader. A regression covers a live descendant retaining both descriptors. Remaining findings are advisory and either already policy-backed or explicitly handled.

nimmo added this to the Automated Update Remediation milestone 2026-09-09 18:23:33 +01:00

nimmo added 5 commits 2026-09-09 18:23:34 +01:00

Give leaf remediation verified package context and a narrow Moonlight target ... 472d56a0d9

Refs #208. Preserve upstream package identity, collect bounded baseline and candidate recipes, and compute advisory patch hashes in the controller.

Expose dependency versions and available alternatives to remediation ... 53dcd164c0

Compare bounded baseline and candidate build dependencies and numbered package alternatives before asking the worker to select a pin. Refs #208.

Distinguish editable leaf files from remediation reference recipes ... c07d01364f

A real worker chose the right dependency remedy but patched copied evidence. Expose existing allowed leaf targets and retain rejection of evidence patches. Refs #208.

Fix real Nix acceptance of advisory remediation patches ... 999e9d9ca1

Recount hunk lengths and persist canonical diffs, separate JSON stdout from diagnostic stderr, and avoid outPath coercion in package-proof projections. Cover real replay failures in the integration fixtures. Refs #208.

Pin published failed-builder attribution helper ... b5f17bdf60

Use the helper proposed in nixos-auto-update MR #2 for the leaf remediation integration. Retain its feature ref until the dependency is merged. Refs #208.

nimmo added the

project:auto-update-remediation

service:auto-update

labels 2026-09-09 18:24:14 +01:00

nimmo referenced this pull request 2026-09-09 18:24:16 +01:00

Enable correctly attributed leaf-package remediation proposals #208

nimmo commented 2026-09-09 18:25:07 +01:00

Author

Owner

Copy link

Automated code review

Reviewed commit: ee73c545ad2d039dffd3c76619b2644b10415959

Verdict: Requires further work

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

Overall assessment

The PR introduces a dedicated wrapper for moonlight-qt to allow dependency pinning without global overrides, enhances the remediation controller with bounded advisory context collection (package versions, dependencies, alternatives), and improves command execution robustness by separating stderr from stdout and implementing bounded cleanup of inherited output pipes. The changes include validation logic to ensure patches target allowed leaf files and avoid modifying copied evidence, and update the worker prompt to reflect these constraints.

Blocking findings

None.

Important findings

• Potential race condition in stderr pipe closure fallback (scripts/nixos_update_remediation/core.py:425; high confidence)
  The finish_readers function closes process.stdout and process.stderr as a final fallback if threads remain alive. If the child process has already exited but inherited descriptors are still open in a way that prevents EOF, closing them is correct. However, if the child is still running (though unlikely given the context of 'command leader exits'), closing stdout/stderr might cause SIGPIPE or unexpected behavior in the child. The code assumes the command leader has exited (The command leader has exited, so Popen.terminate() cannot help.), which mitigates this risk.
  Impact: Low probability of unexpected child termination if assumptions about process state are violated, but generally safe given the context.
  Suggested fix: The current implementation is robust. No change needed.

Suggestions

• Hardcoded policy limits in validation.py (scripts/nixos_update_remediation/validation.py:129; medium confidence)
  The validate_applied_patch function uses policy['eligibility']['allowedPatchDeletionTokens']. While this is loaded from policy, the specific tokens are not visible in the diff. Ensure these tokens are well-documented and reviewed for security implications.
  Impact: If tokens are too broad, they might allow unintended edits.
  Suggested fix: Review allowedPatchDeletionTokens in the policy file to ensure they are restrictive enough.

• Nix evaluation timeout handling (scripts/nixos_update_remediation/model.py:475; high confidence)
  The package_evidence function uses run_command with a timeout derived from deadline - monotonic(). If the deadline is very close, the timeout could be negative or zero, leading to immediate failure. The code checks if remaining <= 0: raise ControllerError(...), which handles this.
  Impact: Prevents invalid timeouts.
  Suggested fix: No change needed.

Tests and validation

• No configured deterministic checks.

Questions

• Has the moonlight-qt wrapper been tested with actual dependency overrides in a real NixOS environment?
• Are there any other leaf packages that might benefit from similar wrappers, and should this pattern be documented for future use?

Review limitations

• I cannot run nix flake check or execute the integration tests to verify the changes.
• I cannot validate the security implications of the Nix evaluation projections without a deeper analysis of the Nix language semantics and potential injection vectors.
• The review is based on the provided diff and text; I have not seen the full context of all related files or the policy file.

Diff coverage

• README.md: reviewed — included in a context-limited batch
• flake.lock: reviewed — included in a context-limited batch
• modules/common/nix-settings.nix: reviewed — included in a context-limited batch
• packages/moonlight-qt/default.nix: reviewed — included in a context-limited batch
• remediation/worker-prompt.md: reviewed — included in a context-limited batch
• scripts/nixos_update_remediation/cli.py: reviewed — included in a context-limited batch
• scripts/nixos_update_remediation/core.py: reviewed — included in a context-limited batch
• scripts/nixos_update_remediation/model.py: reviewed — included in a context-limited batch
• scripts/nixos_update_remediation/validation.py: reviewed — included in a context-limited batch
• tests/nixos-update-remediation-integration.py: 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: 9
• Previous automated review: updated
• Findings: 3

<!-- forgejo-ai-review:v1 --> ## Automated code review Reviewed commit: `ee73c545ad2d039dffd3c76619b2644b10415959` ## Verdict: Requires further work Resolve the blocking or important findings and investigate failed deterministic checks before merging. ### Overall assessment The PR introduces a dedicated wrapper for `moonlight-qt` to allow dependency pinning without global overrides, enhances the remediation controller with bounded advisory context collection (package versions, dependencies, alternatives), and improves command execution robustness by separating stderr from stdout and implementing bounded cleanup of inherited output pipes. The changes include validation logic to ensure patches target allowed leaf files and avoid modifying copied evidence, and update the worker prompt to reflect these constraints. ### Blocking findings None. ### Important findings - **Potential race condition in stderr pipe closure fallback** (scripts/nixos_update_remediation/core.py:425; high confidence) The `finish_readers` function closes `process.stdout` and `process.stderr` as a final fallback if threads remain alive. If the child process has already exited but inherited descriptors are still open in a way that prevents EOF, closing them is correct. However, if the child is still running (though unlikely given the context of 'command leader exits'), closing stdout/stderr might cause SIGPIPE or unexpected behavior in the child. The code assumes the command leader has exited (`The command leader has exited, so Popen.terminate() cannot help.`), which mitigates this risk. Impact: Low probability of unexpected child termination if assumptions about process state are violated, but generally safe given the context. Suggested fix: The current implementation is robust. No change needed. ### Suggestions - **Hardcoded policy limits in validation.py** (scripts/nixos_update_remediation/validation.py:129; medium confidence) The `validate_applied_patch` function uses `policy['eligibility']['allowedPatchDeletionTokens']`. While this is loaded from policy, the specific tokens are not visible in the diff. Ensure these tokens are well-documented and reviewed for security implications. Impact: If tokens are too broad, they might allow unintended edits. Suggested fix: Review `allowedPatchDeletionTokens` in the policy file to ensure they are restrictive enough. - **Nix evaluation timeout handling** (scripts/nixos_update_remediation/model.py:475; high confidence) The `package_evidence` function uses `run_command` with a timeout derived from `deadline - monotonic()`. If the deadline is very close, the timeout could be negative or zero, leading to immediate failure. The code checks `if remaining <= 0: raise ControllerError(...)`, which handles this. Impact: Prevents invalid timeouts. Suggested fix: No change needed. ### Tests and validation - No configured deterministic checks. ### Questions - Has the `moonlight-qt` wrapper been tested with actual dependency overrides in a real NixOS environment? - Are there any other leaf packages that might benefit from similar wrappers, and should this pattern be documented for future use? ### Review limitations - I cannot run `nix flake check` or execute the integration tests to verify the changes. - I cannot validate the security implications of the Nix evaluation projections without a deeper analysis of the Nix language semantics and potential injection vectors. - The review is based on the provided diff and text; I have not seen the full context of all related files or the policy file. ### Diff coverage - `README.md`: **reviewed** — included in a context-limited batch - `flake.lock`: **reviewed** — included in a context-limited batch - `modules/common/nix-settings.nix`: **reviewed** — included in a context-limited batch - `packages/moonlight-qt/default.nix`: **reviewed** — included in a context-limited batch - `remediation/worker-prompt.md`: **reviewed** — included in a context-limited batch - `scripts/nixos_update_remediation/cli.py`: **reviewed** — included in a context-limited batch - `scripts/nixos_update_remediation/core.py`: **reviewed** — included in a context-limited batch - `scripts/nixos_update_remediation/model.py`: **reviewed** — included in a context-limited batch - `scripts/nixos_update_remediation/validation.py`: **reviewed** — included in a context-limited batch - `tests/nixos-update-remediation-integration.py`: **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: 9 - Previous automated review: updated - Findings: 3 </details>

nimmo changed target branch from fix/remediation-evaluation-evidence to main 2026-09-09 18:34:16 +01:00

nimmo added 1 commit 2026-09-09 18:39:53 +01:00

Merge main into leaf remediation branch and use merged helper ... c75d17d1bd

Resolve the flake.lock conflict with current config main and pin updater main at MR #2 merge a32a0ba. Full flake, remediation, and pin-lifecycle regression checks pass.

nimmo referenced this pull request from a commit 2026-09-09 21:26:39 +01:00

Bound cleanup of inherited command output pipes

nimmo added 1 commit 2026-09-09 21:26:40 +01:00

Bound cleanup of inherited command output pipes ... a71b8bef0a

Terminate remaining process-group children and close both descriptors as a final fallback when a command leader exits before inherited stdout or stderr pipes. Add a regression with a lingering child. Refs #208, MR #210.

nimmo referenced this pull request from a commit 2026-09-09 21:30:14 +01:00

Make remediation timing dependencies explicit

nimmo added 1 commit 2026-09-09 21:30:14 +01:00

Make remediation timing dependencies explicit ... 70c5002fc5

Import monotonic and sleep directly so the runtime dependency is visible in the MR diff and covered by the full controller regression suite. Refs #208, MR #210.

nimmo referenced this pull request from a commit 2026-09-09 21:34:05 +01:00

Clarify bounded command reader cleanup

nimmo added 1 commit 2026-09-09 21:34:07 +01:00

Clarify bounded command reader cleanup ... ee73c545ad

Name the stderr capture bound and express EOF, TERM, and KILL cleanup as one bounded escalation loop. Refs #208, MR #210.

nimmo merged commit 4a03f7ebc9 into main 2026-09-09 21:38:45 +01:00

nimmo deleted branch fix/remediation-leaf-proposals 2026-09-09 21:38:46 +01:00

nimmo referenced this pull request from a commit 2026-09-09 21:38:47 +01:00

Merge pull request 'Enable and validate leaf-package remediation proposals' (#210) from fix/remediation-leaf-proposals into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

area:authentication

Issues related to authentication area

  

area:flake-utilities

Issues related to flake utilities area

  

area:performance

Issues related to performance area

  

area:tbd

Area to be determined

  

host:chaos

Issues specific to the chaos host.

  

host:electra

Issues specific to the electra host.

  

host:fleet

Issues that span multiple hosts or the fleet as a whole.

  

host:lyra

Issues specific to the lyra host.

  

host:nova

Issues specific to the nova host.

  

host:vega

Issues specific to the vega host.

  

investigation

Issue is an investigation task

  

phase:cutover

The service cutover execution phase.

  

phase:deploy

  

phase:mcp

  

phase:module

  

phase:packaging

  

phase:prep

Preparation and preflight work before service cutover.

  

phase:validation

Post-cutover validation and rollback-window work.

  

priority:high

High priority issue

  

priority:medium

Medium priority issue

  

project:attic-postgres-lyra-rollout

Issues in the Attic PostgreSQL migration and Lyra rollout workstream.

  

project:auto-update-reliability

Work arising from the automated update reliability review.

  

project:auto-update-remediation

Guarded Nova-assisted diagnosis and remediation of failed automated updates.

  

project:declarative-purity-cleanup

Issues in the declarative purity cleanup workstream.

  

project:external-review

Review of external configurations for ideas and improvements

  

project:fleet-boundary-cleanup

Maintainability work that clarifies fleet configuration and automation boundaries.

  

project:host-facts-refactor

Issues in the host facts refactor workstream.

  

project:lyra-nixos-deploy

Issues in the Lyra NixOS deployment workstream.

  

project:lyra-service-stack-migration

Issues in the lyra service stack migration workstream.

  

project:nebula-mesh-network

Issues in the Nebula mesh network workstream.

  

project:nixos-build-deployment-pipeline

Issues in the NixOS Build & Deployment Pipeline workstream.

  

project:security-hardening

Work belonging to the Security Hardening milestone

  

project:service-stack-migration

Issues in the service stack migration workstream.

  

project:vega-sillytavern-cutover

Issues in the Vega SillyTavern cutover workstream.

  

project:wiki-rebuild

Re-found the repository wiki as the fleet's curated knowledge architecture.

  

repo:numtide/flake-utils

Issues related to numtide/flake-utils repository

  

repo:numtide/nix-auth

Issues related to numtide/nix-auth repository

  

repo:numtide/nixos-passthru-cache

Issues related to numtide/nixos-passthru-cache repository

  

repo:numtide/nix-relay

Issues related to numtide/nix-relay repository

  

service:auto-update

NixOS flake update, cache build, and consumer deployment automation.

  

service:mem0

  

service:nix

Issues concerning Nix build, store, cache, or deployment infrastructure.

  

service:sillytavern

Issues related to the SillyTavern service.

  

service:slskd

Soulseek daemon (slskd) work.

  

service:synthseek

Synthseek music discovery/library manager work.

No labels

area:authentication

area:flake-utilities

area:performance

area:tbd

host:chaos

host:electra

host:fleet

host:lyra

host:nova

host:vega

investigation

phase:cutover

phase:deploy

phase:mcp

phase:module

phase:packaging

phase:prep

phase:validation

priority:high

priority:medium

project:attic-postgres-lyra-rollout

project:auto-update-reliability

project:auto-update-remediation

project:declarative-purity-cleanup

project:external-review

project:fleet-boundary-cleanup

project:host-facts-refactor

project:lyra-nixos-deploy

project:lyra-service-stack-migration

project:nebula-mesh-network

project:nixos-build-deployment-pipeline

project:security-hardening

project:service-stack-migration

project:vega-sillytavern-cutover

project:wiki-rebuild

repo:numtide/flake-utils

repo:numtide/nix-auth

repo:numtide/nixos-passthru-cache

repo:numtide/nix-relay

service:auto-update

service:mem0

service:nix

service:sillytavern

service:slskd

service:synthseek

Milestone

Clear milestone

No items

No milestone

Automated Update Remediation

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

nimmo/nixos-config!210

No description provided.