nimmo/nixos-config

Watch

Fork 0

fix(remediation): record review and ntfy delivery status #188

Merged

nimmo merged 2 commits from fix/issue-168-remediation-notification-feedback into main

2026-08-23 19:13:53 +01:00

nimmo commented

2026-08-23 17:54:18 +01:00

Owner

Summary

add an idempotent tracking-issue comment when a controller-published temporary-pin PR is ready for human review
retry ntfy publication and fail visibly when configuration or delivery fails
record a bounded, deduplicated delivery-failure comment on each affected tracking issue without exposing ntfy credentials to Forgejo-authorised code or raw ntfy responses

Validation

./tests/nixos-update-remediation-regressions.sh
actionlint (with the existing custom nova runner-label warning excluded)
git diff --check

Refs #168

## Summary - add an idempotent tracking-issue comment when a controller-published temporary-pin PR is ready for human review - retry ntfy publication and fail visibly when configuration or delivery fails - record a bounded, deduplicated delivery-failure comment on each affected tracking issue without exposing ntfy credentials to Forgejo-authorised code or raw ntfy responses ## Validation - `./tests/nixos-update-remediation-regressions.sh` - actionlint (with the existing custom `nova` runner-label warning excluded) - `git diff --check` Refs #168

nimmo added this to the Automated Update Remediation milestone

2026-08-23 17:54:18 +01:00

nimmo added the

service:auto-update

project:auto-update-remediation

labels

2026-08-23 17:54:18 +01:00

nimmo self-assigned this

2026-08-23 17:54:18 +01:00

nimmo added 1 commit

2026-08-23 17:54:22 +01:00

fix(remediation): record review and ntfy delivery status

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

Details

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

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

3d65583b06

nimmo commented

2026-08-23 17:56:05 +01:00

Author

Owner

Automated code review

Reviewed commit: b7d93621d7d677f80d5296a740d71092515fdc66

Verdict: Requires further work

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

Overall assessment

The PR implements a feedback loop for the automated NixOS update remediation system. It adds logic to record when a remediation proposal is ready for human review (via a marker-deduplicated comment on the tracking issue) and handles ntfy notification delivery failures by recording them as bounded, deduplicated comments on the affected tracking issues. The changes span workflow definitions (attic-cache.yml), Python publication logic (publication.py), and regression tests.

Blocking findings

None.

Important findings

Workflow job dependency creates a potential circular or deadlocked execution path (.forgejo/workflows/attic-cache.yml:427; medium confidence)
The record-remediation-notify-failure job has needs: [remediate, notify-remediation]. The notify-remediation job itself has needs: remediate. If notify-remediation fails, record-remediation-notify-failure runs. However, if notify-remediation is skipped (e.g., due to if: ${{ always() && needs.remediate.result != 'skipped' }} logic or other conditions), the dependency resolution might behave unexpectedly depending on Forgejo's specific handling of skipped vs failed jobs in always() contexts. More critically, if notify-remediation fails, it outputs delivery_failures. If it succeeds, it also outputs delivery_failures (empty array). The logic seems sound, but the explicit dependency on both ensures it only runs after notification attempts.
Impact: If the dependency logic is misinterpreted by the runner or if notify-remediation fails in a way that doesn't trigger the if condition correctly, the failure reporting might not occur, leaving operators unaware of ntfy delivery issues.
Suggested fix: Verify Forgejo's behavior when a needed job fails vs is skipped. Ensure the if condition on record-remediation-notify-failure (needs.notify-remediation.result == 'failure') accurately captures all failure modes of the notification step.

Suggestions

Hardcoded repository name in Tea CLI command (.forgejo/workflows/attic-cache.yml:513; medium confidence)
$TEA_BIN comment --repo "${GITHUB_REPOSITORY:-nimmo/nixos-config}" "$issue_index" "$comment"
Impact: If this workflow is ever forked or used in a different repository without updating the environment variable GITHUB_REPOSITORY, it will default to nimmo/nixos-config. While GITHUB_REPOSITORY is typically set by the runner, relying on a fallback with a hardcoded value is less robust than ensuring the variable is always present or using a workflow-level input.
Suggested fix: Ensure GITHUB_REPOSITORY is always provided by the runner environment (which it usually is in GitHub/Forgejo Actions) and consider removing the fallback or making it explicit that it's a safety net for local testing only.

Tests and validation

No configured deterministic checks.

Questions

Does the notify-remediation job's if: ${{ always() && needs.remediate.result != 'skipped' }} condition correctly handle cases where remediate is skipped? If remediate is skipped, notify-remediation should also be skipped or handled appropriately to avoid false failure reports.
In the record-remediation-notify-failure job, why is TEA_BIN passed from needs.remediate.outputs.tea_bin? Is it guaranteed that tea_bin is available even if remediate failed? If remediate fails to produce tea_bin, the failure reporter will fail with 'remediation publication client is unavailable', which might mask the actual ntfy failure.
The PR mentions 'retry ntfy publication'. The workflow uses --retry 3 --retry-all-errors. Are there any specific transient errors that should be handled differently (e.g., rate limiting vs. network timeout)?

Review limitations

I cannot execute the workflow or Python scripts to verify runtime behavior.
I cannot verify the exact Forgejo Actions environment variables or runner capabilities beyond what is documented.
The review assumes the provided diff and file contents are accurate and complete.

Diff coverage

.forgejo/workflows/attic-cache.yml: reviewed — included in a context-limited batch
README.md: reviewed — included in a context-limited batch
scripts/nixos_update_remediation/publication.py: reviewed — included in a context-limited batch
tests/nixos-update-remediation-integration.py: reviewed — included in a context-limited batch
tests/nixos-update-remediation-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: 2
Previous automated review: updated
Findings: 2

## Automated code review Reviewed commit: `b7d93621d7d677f80d5296a740d71092515fdc66` ## Verdict: Requires further work Resolve the blocking or important findings and investigate failed deterministic checks before merging. ### Overall assessment The PR implements a feedback loop for the automated NixOS update remediation system. It adds logic to record when a remediation proposal is ready for human review (via a marker-deduplicated comment on the tracking issue) and handles ntfy notification delivery failures by recording them as bounded, deduplicated comments on the affected tracking issues. The changes span workflow definitions (`attic-cache.yml`), Python publication logic (`publication.py`), and regression tests. ### Blocking findings None. ### Important findings - **Workflow job dependency creates a potential circular or deadlocked execution path** (.forgejo/workflows/attic-cache.yml:427; medium confidence) The `record-remediation-notify-failure` job has `needs: [remediate, notify-remediation]`. The `notify-remediation` job itself has `needs: remediate`. If `notify-remediation` fails, `record-remediation-notify-failure` runs. However, if `notify-remediation` is skipped (e.g., due to `if: ${{ always() && needs.remediate.result != 'skipped' }}` logic or other conditions), the dependency resolution might behave unexpectedly depending on Forgejo's specific handling of skipped vs failed jobs in `always()` contexts. More critically, if `notify-remediation` fails, it outputs `delivery_failures`. If it succeeds, it also outputs `delivery_failures` (empty array). The logic seems sound, but the explicit dependency on both ensures it only runs after notification attempts. Impact: If the dependency logic is misinterpreted by the runner or if `notify-remediation` fails in a way that doesn't trigger the `if` condition correctly, the failure reporting might not occur, leaving operators unaware of ntfy delivery issues. Suggested fix: Verify Forgejo's behavior when a needed job fails vs is skipped. Ensure the `if` condition on `record-remediation-notify-failure` (`needs.notify-remediation.result == 'failure'`) accurately captures all failure modes of the notification step. ### Suggestions - **Hardcoded repository name in Tea CLI command** (.forgejo/workflows/attic-cache.yml:513; medium confidence) $TEA_BIN comment --repo "${GITHUB_REPOSITORY:-nimmo/nixos-config}" "$issue_index" "$comment" Impact: If this workflow is ever forked or used in a different repository without updating the environment variable `GITHUB_REPOSITORY`, it will default to `nimmo/nixos-config`. While `GITHUB_REPOSITORY` is typically set by the runner, relying on a fallback with a hardcoded value is less robust than ensuring the variable is always present or using a workflow-level input. Suggested fix: Ensure `GITHUB_REPOSITORY` is always provided by the runner environment (which it usually is in GitHub/Forgejo Actions) and consider removing the fallback or making it explicit that it's a safety net for local testing only. ### Tests and validation - No configured deterministic checks. ### Questions - Does the `notify-remediation` job's `if: ${{ always() && needs.remediate.result != 'skipped' }}` condition correctly handle cases where `remediate` is skipped? If `remediate` is skipped, `notify-remediation` should also be skipped or handled appropriately to avoid false failure reports. - In the `record-remediation-notify-failure` job, why is `TEA_BIN` passed from `needs.remediate.outputs.tea_bin`? Is it guaranteed that `tea_bin` is available even if `remediate` failed? If `remediate` fails to produce `tea_bin`, the failure reporter will fail with 'remediation publication client is unavailable', which might mask the actual ntfy failure. - The PR mentions 'retry ntfy publication'. The workflow uses `--retry 3 --retry-all-errors`. Are there any specific transient errors that should be handled differently (e.g., rate limiting vs. network timeout)? ### Review limitations - I cannot execute the workflow or Python scripts to verify runtime behavior. - I cannot verify the exact Forgejo Actions environment variables or runner capabilities beyond what is documented. - The review assumes the provided diff and file contents are accurate and complete. ### Diff coverage - `.forgejo/workflows/attic-cache.yml`: **reviewed** — included in a context-limited batch - `README.md`: **reviewed** — included in a context-limited batch - `scripts/nixos_update_remediation/publication.py`: **reviewed** — included in a context-limited batch - `tests/nixos-update-remediation-integration.py`: **reviewed** — included in a context-limited batch - `tests/nixos-update-remediation-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: 2 - Previous automated review: updated - Findings: 2 </details>

nimmo added 1 commit

2026-08-23 18:26:20 +01:00

test(remediation): cover review-ready comment recovery

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

Details

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

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