Repository navigation
Conversation
![postil-dev[bot]](https://avatars.githubusercontent.com/in/3455612?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 finding posted inline
1 suppressed
- Manual alert tests no longer trigger notifications at
.github/workflows/production-monitor.yml:452: below the configured confidence threshold; severity error, confidence 0.04. The notification job now runs only when the smoke job fails, so manual runs withtest_alert=trueskip the external alert. Preserve the operator-requested test-alert branch and its delivery verification.
There was a problem hiding this comment.
1 finding posted inline
3 suppressed
- Allow the manual canary to run when smoke fails at
.github/workflows/production-monitor.yml:540: cites a line the named construct does not sit on; severity warn, confidence 0.94. Because this job needssmokeand its condition is not wrapped inalways(), GitHub Actions skips the job whensmokefails or is cancelled, even wheninputs.test_alertis true. This regresses the prior manual test-alert path, which could validate delivery during monitor failures. Remove the dependency or use an always-evaluated condition that explicitly permits the manual canary to run. - Preserve external alerts for manual test runs at
.github/workflows/production-monitor.yml:598: cites a line the named construct does not sit on; severity warn, confidence 0.98. This condition removed theinputs.test_alert == truebranch, so a manually requested test run no longer reaches the external alert action even when the test is intended to verify alert delivery. Retain the test-alert condition or otherwise route test runs to the alert action without affecting production failure handling. - Restore test-alert notification delivery at
.github/workflows/production-monitor.yml:598: below the configured confidence threshold; severity error, confidence 0.04. The notification job no longer runs for operator-requested test alerts, so successful smoke and recovery checks cannot exercise the external alert path. Restore the test-alert condition or invoke an equivalent explicit alert action.
|
@postil re-review |
There was a problem hiding this comment.
1 finding posted inline
1 suppressed
- Preserve operator-requested test alerts at
.github/workflows/production-monitor.yml:598: cites a line the named construct does not sit on; severity error, confidence 0.94. This condition removed theinputs.test_alert == truebranch. When an operator requests a test alert and both prerequisite jobs pass,notifyis skipped, so the alerting path cannot be tested and the requested external alert is never sent. Include|| inputs.test_alert == truein the condition, while preserving the existing failure and cancellation cases.
There was a problem hiding this comment.
2 findings posted inline
1 suppressed
- Use the intended current-attempt handoff state at
scripts/reconcile-ilert-alert-stream.ts:1069: below the configured confidence threshold; severity warn, confidence 0.30. This branch checks for the literal handoff value "true", while the surrounding finalizer logic handles the named states "cleaned" and "unknown". If the current-attempt handoff uses the corresponding named state rather than the string "true", this discovery never loads or accounts for the current alert; cleanup can then submit a resolve event without verifying the persisted alert and eventually fail or leave it unmanaged. Replace this comparison with the actual handoff enum/state used by the producer, and verify each handoff path accounts for the current alert.
Before the next push
Run postil review --staged.
| if [[ "${EVENT_NAME}" != "workflow_run" ]]; then | ||
| exit 0 | ||
| fi | ||
| if [[ "${DEPLOY_EVENT}" != "workflow_run" && "${DEPLOY_EVENT}" != "workflow_dispatch" ]]; then |
There was a problem hiding this comment.
warn · confidence 0.72 · kind: risk
The monitor rejects deployments whose originating event is a push, even though the deploy workflow authorizes successful push-triggered runs on main. Accept the workflow's actual provenance and validate the originating main branch before running recovery or smoke jobs.
There was a problem hiding this comment.
These are nested workflow_run events: deploy runs after completed CI, so production-monitor runs after completed deploy and github.event.workflow_run.event is workflow_run, not the CI run origin push; this real deploy run records event: workflow_run. The monitor validates the triggering deploy attempt jobs, and the focused trust cases report 13 pass and 151 assertions (8 skipped).
|
@postil re-review |
There was a problem hiding this comment.
3 findings posted inline
1 suppressed
- Remove the committed webhook credential at
tests/reconcile-ilert-alert-stream.test.ts:1016: deterministically non-actionable; severity error, confidence 0.99. Rotate the credential, purge it from git history because the commit is permanent otherwise, and move it to an environment variable or secrets store. Even as a test fixture, embedding credential material in a webhook URL risks accidental reuse or disclosure.
Before the next push
Run postil review --staged.
| any(.jobs[]; .name == "Authorize deployment source" and .conclusion == "success") | ||
| ' <<<"${jobs}" >/dev/null; then |
There was a problem hiding this comment.
warn · confidence 0.91 · kind: risk
The deployment verifier aggregates authorization across all inspected attempts, allowing an earlier successful attempt to establish trust for a later rerun. Require successful authorization from the current deployment attempt before allowing recovery or monitoring decisions to proceed.
| needs: release-recovery | ||
| if: ${{ always() && (needs.release-recovery.outputs.clear == 'true' || (github.event_name == 'workflow_run' && github.event.workflow_run.conclusion == 'success')) }} | ||
| needs: [deployment-trust, release-recovery] | ||
| if: ${{ always() && needs.deployment-trust.outputs.trusted == 'true' && (needs.release-recovery.outputs.clear == 'true' || (github.event_name == 'workflow_run' && needs.deployment-trust.outputs.conclusion == 'success')) && (github.event_name != 'workflow_dispatch' || github.ref == 'refs/heads/main') }} |
There was a problem hiding this comment.
warn · confidence 0.76 · kind: risk
This condition replaces the prior github.event.workflow_run.conclusion check with needs.deployment-trust.outputs.conclusion. The deployment-trust dependency is only used here for its trusted output, so its conclusion output may be unset; on successful workflow_run events the resolver can therefore be skipped and leave postil-release-recovery open. Restore the event conclusion check or explicitly expose and use a verified equivalent output from deployment-trust.
|
@postil re-review |
| // dispatched. The provider offers no conditional write, so an out-of-band | ||
| // change after these reads remains a detected postcondition risk. |
There was a problem hiding this comment.
warn · confidence 0.94 · kind: risk
The PUT or POST can overwrite an out-of-band alert-action change after the final source and inventory reads complete; the code explicitly acknowledges that this remains an undetected postcondition risk. This can replace another operator's webhook, trigger set, or source binding. Use a provider-supported conditional write or an external distributed lock covering the read/mutation/verification sequence, and fail closed when the precondition cannot be enforced.
There was a problem hiding this comment.
1 finding posted inline
1 suppressed
Remove the committed credential-like webhook value
tests/reconcile-ilert-alert-stream.test.ts:1016
Suppressed: deterministically non-actionable.
Severity error, confidence 0.98.
This added fixture embeds a credential-shaped value directly in a webhook URL. Even if intended only for tests, it can trigger secret exposure and may be copied into logs or tooling. Rotate the value if it has ever been used, purge it from git history, and replace credential material with an environment variable or secrets-store reference; otherwise use a clearly non-credential fixture that cannot be mistaken for a secret.
| any(.jobs[]; .name == "Authorize deployment source" and .conclusion == "success") | ||
| ' <<<"${jobs}" >/dev/null; then | ||
| authorization_trusted=true | ||
| fi |
There was a problem hiding this comment.
error · confidence 0.90 · kind: risk
The authorization loop retains authorization_trusted after an earlier attempt succeeds, while deployment_conclusion is taken from only the current attempt. A rerun can therefore pass trust checks without authorization on its current attempt. Reset and evaluate authorization exclusively for the current attempt.
There was a problem hiding this comment.
2 findings posted inline
3 suppressed
Reconciliation rejects existing delay triggers
scripts/reconcile-ilert-alert-stream.ts:232
Suppressed: below the configured confidence threshold.
Severity error, confidence 0.42.
Reconciliation rejects an existing action when a carried escalation or not-resolved delay has a trigger absent from the desired trigger list. Preserve those trigger types or include them in the desired action before validating carried delay fields.
Handle the unknown handoff state when locating the current canary
scripts/reconcile-ilert-alert-stream.ts:1119
Suppressed: cites a line the named construct does not sit on.
Severity error, confidence 0.99.
This branch compares handoff with the literal string "true", while the surrounding state handling uses "unknown" and "cleaned". For an unknown handoff, the finalizer therefore skips the current-attempt lookup; if that alert is not found through the prior-key inventory, it can submit RESOLVE without setting currentAccountedFor and eventually fail cleanup even when the canary was accepted. Compare against the intended unknown-handoff state and retain the regression covering an accepted current attempt.
Canary migration test seeds unreconcilable index drift
tests/ilert-canary-alert-key-migration.test.ts:153
Suppressed: below the configured confidence threshold.
Severity error, confidence 0.28.
The test creates an existing canary index with a definition the reconciliation path rejects, then invokes reconciliation. Seed a missing index or a drift shape that the reconciler repairs so the migration test exercises a valid repair path.
Before the next push
Run postil review --staged.
| jq -e ' | ||
| (.total_count | type == "number") and | ||
| (.jobs | type == "array") and | ||
| (.total_count == (.jobs | length)) and |
There was a problem hiding this comment.
error · confidence 0.90 · kind: risk
The authorization loop retains authorization_trusted after an earlier attempt succeeds, while deployment_conclusion is taken from only the current attempt. A rerun can therefore pass trust checks without authorization on its current attempt. Reset and evaluate authorization exclusively for the current attempt.
| // and every later step applies in order. A retry therefore never presents | ||
| // an already journaled migration to an earlier step as unknown. | ||
| let pending = false; | ||
| for (const step of reviewed.slice(Math.max(latestApplied, 0))) { |
There was a problem hiding this comment.
error · confidence 0.99 · kind: risk
The migration runner starts at latestApplied instead of the following index, so an already applied migration is executed again. Start at latestApplied + 1, while retaining index 0 when none is applied.
There was a problem hiding this comment.
2 findings posted inline
1 suppressed
Use the actual current-attempt handoff state
scripts/reconcile-ilert-alert-stream.ts:1119
Suppressed: cites a line the named construct does not sit on.
Severity error, confidence 0.86.
This branch only inventories the current attempt when handoff equals the literal string "true", while the surrounding logic treats handoff values as states such as "cleaned" and "unknown". For the non-cleaned states that do not equal "true", the current alert is never discovered here, so a successfully submitted current canary can remain unaccounted for and cleanup can fail or omit it. Replace this comparison with the intended handoff state and verify each handoff variant accounts for the current alert before cleanup succeeds.
Before the next push
Run postil review --staged.
| // and every later step applies in order. A retry therefore never presents | ||
| // an already journaled migration to an earlier step as unknown. | ||
| let pending = false; | ||
| for (const step of reviewed.slice(Math.max(latestApplied, 0))) { |
There was a problem hiding this comment.
error · confidence 0.98 · kind: risk
When any reviewed migration is already journaled, latestApplied points to that migration's index, but slice(Math.max(latestApplied, 0)) includes it again. For example, after migration 0062 succeeds, the retry starts at 0062 rather than 0063; after 0064 succeeds, it starts at 0064. This defeats the stated ordered retry behavior and can make releases fail when an already-recorded migration is presented again. Start at latestApplied + 1, while preserving index 0 when none are applied.
| }); | ||
| expect(service.requests.filter((request) => request.method === "PUT").map((request) => | ||
| new URL(request.url).pathname | ||
| )).toEqual(["/api/alerts/98/resolve"]); |
There was a problem hiding this comment.
error · confidence 0.99 · kind: risk
This test claims to verify exact-key cleanup, but its fixture assigns the compatibility key to alert 99 and unrelated keys to alerts 98 and 100. The assertion instead requires a PUT for alert 98, so an implementation that resolves the unrelated alert can pass while exact-key filtering is broken. Change the expected path to /api/alerts/99/resolve and retain assertions that alerts 98 and 100 remain pending.
There was a problem hiding this comment.
3 findings posted inline
1 suppressed
Prevent migration test setup from aborting on expected index drift
tests/ilert-canary-alert-key-migration.test.ts:155
Suppressed: below the configured confidence threshold.
Severity error, confidence 0.58.
The test setup now creates an intentionally invalid managed index and immediately invokes reconciliation without handling its expected failure. The beforeAll setup can therefore abort the suite before the drift assertions run. Move the drift creation into the specific fail-closed test or catch and record the expected error.
Before the next push
Run postil review --staged.
| invalidValidationError = result.validationError; | ||
| validationContextErrors = [...validationContextErrors, result.validationError].slice(-20); | ||
| } | ||
| if (handoff === "true" && !currentAccountedFor) { |
There was a problem hiding this comment.
error · confidence 0.79 · kind: risk
The new branch checks handoff against the literal string true, although the surrounding state machine uses named states such as cleaned and unknown. This skips current-attempt accounting and can trigger an unnecessary RESOLVE failure. Compare against the intended non-cleaned state.
| // and every later step applies in order. A retry therefore never presents | ||
| // an already journaled migration to an earlier step as unknown. | ||
| let pending = false; | ||
| for (const step of reviewed.slice(Math.max(latestApplied, 0))) { |
There was a problem hiding this comment.
error · confidence 0.98 · kind: risk
The migration runner starts at the latest applied index, causing that migration to execute again. Start at latestApplied + 1 while retaining index 0 when no migration has been applied.
| # when the POSTIL_ILERT_CANARY_SWEEP_ENABLED repository variable is 'true'. | ||
| # The trust job reports the disabled state on every routine run. | ||
| needs: deployment-trust | ||
| if: ${{ needs.deployment-trust.outputs.trusted == 'true' && vars.POSTIL_ILERT_CANARY_SWEEP_ENABLED == 'true' && (github.event_name == 'schedule' || (github.event_name == 'workflow_dispatch' && inputs.reconcile_alert_stream != true && inputs.test_alert != true)) && (github.event_name != 'workflow_dispatch' || github.ref == 'refs/heads/main') }} |
There was a problem hiding this comment.
error · confidence 0.67 · kind: risk
The newly added orphan-sweep job is gated on POSTIL_ILERT_CANARY_SWEEP_ENABLED == 'true'. Unless that repository variable is configured, interrupted canary alerts are not recovered and can remain active indefinitely. Enable the sweep in production or remove the opt-in gate.
This branch has not been deployed
1 participant
The existing iLert receiver and operator stream cannot establish that production alert actions deliver events end to end. This change adds manual action reconciliation and a canary that checks persisted create and resolve events while retaining the production monitor's alert jobs. An additive schema change stores optional alert keys for exact canary checks, and the managed release builds the lookup index concurrently before rollout.
A manual run on
mainchecks the configured alert source, Event API credential, management access, deployed receiver, and webhook credential before reconciling the reserved iLert action. Reconciliation creates a missing action, repairs drift, and updates its webhook credential after secret rotation. It rejects an incompatible nonzero action delay before an update. The HIGH-priority canary resolves discovered alerts and checks for delayed duplicates during cleanup. The separate manual test alert has its own resolution step. Either test can invoke the configured escalation policy.The manual workflow requires a production integration credential, a dedicated management identity with access to the selected source, and a webhook secret shared with the deployed receiver. The managed release requires a target-bound rollback package with a pinned source image and active deadline. It checks the source fleet and replacement, then attempts guarded restoration of the captured source release if rollout fails; a failed guard can leave restoration incomplete. Tests cover reconciliation, mutation guards, canary cleanup, the schema change and index, workflow trust, and manual test-alert resolution. A concurrent external edit after reconciliation's final action read can still be overwritten by its update.