ci: route lockfile drift alerts - #1553
Open
allyblockcast[bot] wants to merge 2 commits into
Open
Conversation
Author
|
🔗 Paperclip issue: BLO-27611 |
1 similar comment
Author
|
🔗 Paperclip issue: BLO-27611 |
Author
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
Author
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 83a53c9
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [native-codex]
.github/scripts/post-lockfile-drift-alert.mjs:43— whenDRIFT_SUMMARYis unset, this passes{ violations: [null] }tobuildAlert, producing an alert description containing- nullrather than the function's(summary unavailable - see the run log)fallback.- Pass
nullas the summary when the environment variable is absent, or filter unset values before constructing the violations array.
- Pass
Strengths
- Stable Alertmanager labels and a two-hour TTL provide useful deduplication across the hourly schedule.
- Alert delivery failures remain visible while the workflow still fails on detected drift.
- The new helper has focused unit coverage for labels, descriptions, and TTL behavior.
Recommended Action
- Consider addressing the unset-summary fallback before relying on this helper from other workflows or manual dispatches.
Author
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Author
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: f24e973
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [native-codex]
.github/scripts/post-lockfile-drift-alert.mjs:43— the current head correctly passesnullwhenDRIFT_SUMMARYis unset, preservingbuildAlert's summary fallback. Keep this regression covered if the workflow environment changes.
Strengths
- Stable Alertmanager labels and a two-hour TTL provide deduplication across hourly failures.
- Alert delivery failures remain fatal, while detected drift still causes the workflow to fail.
- The helper tests cover labels, descriptions, fallback behavior, and TTL semantics.
- The workflow retains read-only repository permissions and separates alert delivery from the final failure step.
Recommended Action
- No Critical or Important code changes are required from this review.
- Resolve the currently failing repository policy check rejecting the shared App-attributed commit before merge; the dependent verification lanes are skipped as a result.
- Consider the Suggestion opportunistically.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Route failures from the scheduled lockfile drift monitor to Alertmanager with stable labels and preserve the failing workflow status.
.github/scripts/post-lockfile-drift-alert.mjswith a stableLockfileDriftDetectedalert identity.contents: read; no repository write permission is added.Verification
node --test .github/scripts/tests/post-lockfile-drift-alert.test.mjs scripts/check-lockfile-overrides-consistency.test.mjs(15/15 passed)node scripts/check-lockfile-overrides-consistency.mjs(current tree consistent)git diff --check(clean)Post-merge verification remains: run a deliberate drift failure twice to confirm one Alertmanager incident with updated context, then confirm a normal scheduled run stays green and emits no alert.