Repository navigation
fix: reconcile keepalive authority receipts using exact worker evidence - #3601
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 122 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Review configuration: ⚙️ Run configurationConfiguration used: Repository: stranske/Workflows/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThe keepalive authority flow now indexes receipts by owner attempt, classifies worker execution for exact workflow attempts, and reconciles eligible failures. Workflow finalization and reporting handle failure or cancellation, and settled recoveries are projected into keepalive summaries. ChangesAuthority receipt recovery
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Reporter as Reporter workflow
participant Evidence as Worker evidence helper
participant Jobs as Workflow jobs API
participant Authority as Authority state
participant Summary as Keepalive summary
Reporter->>Evidence: Request evidence for exact run attempt
Evidence->>Jobs: List jobs for that attempt
Jobs-->>Evidence: Return job and step outcomes
Evidence-->>Reporter: Return started, not-started, or unknown
Reporter->>Authority: Resolve target and reconcile failed attempt
Authority-->>Reporter: Return recovery status
Reporter->>Summary: Project released or reopened recovery
Merge Risk: 🟡 Moderate · up to The authority recovery flow can refund a consumed single-use receipt based on an unverified summary value. It can also leave receipts stuck when a worker is cancelled before it starts, and it reports failures for skipped runs. The refund path should be fixed before merging. 🚥 Pre-merge checks | ✅ 3 | ❌ 1 | ❓ 1❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The implementation addresses the coding objectives in Resolution Provide exact-head results for the required targeted test suites, including Full details: Docstring CoverageExplanation Docstring coverage is 5.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 15 files. (9 skipped: 9 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Automated Status SummaryHead SHA: 43153ee
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Low Coverage Files (<50.0%)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeThe current Workflows-source keepalive authority protocol can strand a prepared or consumed receipt for up to 24 hours after a failed/cancelled preflight or mark-running attempt. Conversely, the failure reporter currently assumes no agent executed, which could replay a single-use authority generation if liveness recovery is broadened without an execution-evidence guard. The active generated canary stranske/Portable-Alpha-Extension-Model#2318 has two unresolved P1 threads on exact head Context for AgentRelated Issues/PRs
Tasks
Acceptance criteria
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d7ac8ca29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - name: Require PR association for failed originating run | ||
| if: >- | ||
| github.event.workflow_run.conclusion != 'success' && | ||
| !github.event.workflow_run.pull_requests[0].number | ||
| run: | |
There was a problem hiding this comment.
Hourly authority challenges are started by agents-keepalive-sweep.yml as workflow_dispatch runs on the default branch, with the target carried only in the pr_number input. Their subsequent workflow_run payload can therefore have an empty pull_requests array, so this guard exits before collecting worker evidence or reopening the consumed receipt. A failure or cancellation after finalization but before the worker starts—the exact case this reporter is meant to repair—then leaves the receipt spent and the PR stuck. Persist or otherwise recover the dispatch input instead of requiring a workflow-run PR association; the synced consumer reporter has the same guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on b8ee068: both reporters now recover an unassociated workflow_dispatch run from the unique authority-ledger receipt matching its exact owner attempt, fail closed on ambiguity/no target, and use the ledger PR head for summary projection. Added source/template coverage and validated focused tests, template drift, and actionlint. Please reassess this finding on the current head.
There was a problem hiding this comment.
The fresh review completed on b8ee068 without an in-thread disposition here. The missing-PR recovery remains in current head 4176c45; the later change only skips the ledger scan when the webhook already identifies a PR. Please explicitly confirm whether this finding is satisfied on the current head, or identify a remaining defect.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8ee06851c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| authorityTarget = await findAuthorityPrForAttempt({ | ||
| request: authorityRequest, | ||
| repository: `${context.repo.owner}/${context.repo.repo}`, | ||
| ownerAttempt, | ||
| }); |
There was a problem hiding this comment.
For normal failed runs whose workflow_run.pull_requests[0] already identifies the PR, this unconditional lookup still lists the entire authority directory and findAuthorityPrForAttempt then reads every historical *.json ledger file. Reporting therefore costs O(all previously challenged PRs) API calls, aborts if any unrelated ledger file is unreadable or malformed, and is guaranteed to fail once the listing reaches the explicit 1,000-entry guard. Only perform this repository-wide lookup when prNumber is absent; an associated run can reconcile directly against its known PR's ledger.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on 4176c45: the ledger-wide lookup now runs only when workflow_run has no PR association; associated runs use their known PR directly. Both source and consumer reporters are updated, with the focused delivery assertion, template drift, and actionlint passing. Please disposition this thread on the current head.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4176c4578f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!Array.isArray(entries) || entries.length >= 1000) { | ||
| throw new Error('Authority directory listing unavailable or incomplete'); |
There was a problem hiding this comment.
Fresh evidence after the associated-run optimization is that authority challenges are still workflow_dispatch runs whose webhook commonly has no PR association, so their failed-run recovery necessarily reaches this branch. Because authority files are retained per PR with no cleanup, once the directory reaches 1,000 entries every such recovery is rejected here, while even earlier failures require one API read per historical PR and can exhaust the App's rate limit. Persist an attempt-to-PR lookup or another direct target reference instead of depending on a full Contents API directory scan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on 92eeb50: preparation now creates an immutable SHA-keyed exact-attempt locator before the PR ledger reservation, and finalization verifies it. Missing-PR reporter recovery uses one direct index GET plus one PR ledger GET with receipt-ID/binding validation; there is no historical directory scan. Lost create responses deny that invocation, and released attempts need a new run attempt. Source/template mirror, manifest, docs, and 228 JS + 273 Python focused tests pass. Please disposition this finding on the current head.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50ff464445
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4481165dc9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (settled.state.status !== 'available' || | ||
| !receiptMatches(settled.state.released_receipt, claim, ownerAttempt, | ||
| receipt.provider, state.head_sha)) return { status: 'uncertain' }; | ||
| return { status: 'released', reason: result.reason, state: settled.state }; |
There was a problem hiding this comment.
If this reporter runs or is manually retried after the prepared generation's expires_at, this branch returns the released state with its original, already-expired due/expiry window. projectRecoveredAuthorityState then publishes those timestamps, while selectDueAuthorityChallenge rejects the projection when now >= expires_at, so the promised retry is never dispatched and the PR remains stuck. Rotate to a fresh generation/window here when the released state has expired, as the consumed-receipt recovery path already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on head 6c314e2: exact non-start release now rotates an expired prepared window under one ledger CAS, retaining the immutable released receipt, original generation, and lineage. Replays of an expired release refresh the window only for the same indexed attempt and open same-head automation-labeled PR without a hard human blocker; projection accepts exact release lineage, and expiry alone no longer reinitializes a same-head prepared/consumed/confirmed receipt. Source/template code and docs updated. Focused validation: 256 JS and 73 Python tests passed, including expiry, replay, response-loss, confirmed-state and multi-window projection regressions. Please reassess and disposition this finding on the exact current head.
|
@codex review |
|
Reviewer disposition handoff for current head 6c314e2: completed Codex reviews have not resolved five active non-outdated threads despite owner fix replies. Originating reviewer, please give explicit thread-level acceptance or remaining changes for #3601 (comment), #3601 (comment), #3601 (comment), #3601 (comment), and #3601 (comment). The expiry finding has a fresh exact-head reply at #3601 (comment). If the bot cannot disposition its threads, an authorized human reviewer must do so; the source owner will not self-resolve or merge through active threads. Recheck after the current review/CI, no later than the 20:25 UTC campaign run. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Repository-owner reviewer handoff: the exact-head Codex review of 6c314e2 reported no major issues, but five original non-outdated finding threads remain active without explicit same-head acceptance. The existing thread list and fixes are at #3601 (comment). No independent human reviewer is currently configured among repository collaborators (only stranske and stranske-automation-bot). Please designate or authorize an independent human reviewer to state accepted or remaining defect in each original thread, or have the originating Codex reviewer do so. I will not self-resolve or merge through active threads. Next automatic check: 2026-09-28 02:25 UTC. |
|
Closer exact-head review disposition for 6c314e2: all five active findings are satisfied by the current source/template changes. Attempt-aware reporter fingerprinting binds run ID plus run attempt; both mark-running cleanup paths pass the signed fingerprint; worker evidence loads the registry at the originating run head and returns unknown on unavailable evidence; ordinary unassociated dispatches skip only after exact producer/index classification while authority-candidate or uncertain evidence fails closed; root reporter Node/API setup is guarded by the applicability skip. Independent focused validation passed: node --test over worker evidence, reporter applicability, and authority state: 40/40; PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 pytest -q tests/workflows/test_keepalive_authority_delivery.py -p no:cacheprovider: 3/3. A bounded Astra Medium assessment independently reached the same five satisfied verdicts and identified only two future test-strength opportunities (behavioral fingerprint hashing and expression-equality coverage), not remaining production defects. Resolving the five threads; no source change or push was required. |
|
Final merge gate for exact head 6c314e2: CLEAN and MERGEABLE; required summary check passed; no failed or pending checks; complete thread pagination found zero active non-outdated threads. The named expected-check helper is absent and was not treated as a pass. Direct same-repository topology comparison against merged workflow-heavy PRs #3600 and #3603 shows the identical 18 non-skipped Gate jobs, all successful here, including privilege gate, Python 3.12/3.13, scripts, ledger validation, test-quality, and summary. The head predates this round and no push occurred, so the seven-minute exact-head review floor is satisfied. Squash merging without deleting the branch, then applying verify:compare. |
Provider Comparison ReportProvider Summary
📋 Full Provider Details (click to expand)openai
anthropic
Agreement
Disagreement
Unique Insights
🔍 LangSmith Traces |
This branch was successfully deployed
2 participants
Closes #3595
Automated Status Summary
Scope
The current Workflows-source keepalive authority protocol can strand a prepared or consumed receipt for up to 24 hours after a failed/cancelled preflight or mark-running attempt. Conversely, the failure reporter currently assumes no agent executed, which could replay a single-use authority generation if liveness recovery is broadened without an execution-evidence guard. The active generated canary stranske/Portable-Alpha-Extension-Model#2318 has two unresolved P1 threads on exact head
09b3ae8f4b98dc1530636b091d51e7c2afaa5fc0. Related findings on stranske/trip-planner#1869 and stranske/Travel-Plan-Permission#1638 concern the same Workflows source.Context for Agent
Related Issues/PRs
Tasks
scripts/runner_lib/core.py, makerelease_authority_challengeretryable for the same exact receipt, reservation and owner attempt when the first ledger release failed after completion was recorded; reject stale or replacement receipts.templates/consumer-repo/.github/workflows/agents-81-gate-followups.ymland.github/workflows/agents-keepalive-loop.yml, cover cancellation and failure between preparation and finalization, and move finalization after failure-prone mark-running setup while keeping worker execution gated on successful finalization.templates/consumer-repo/.github/workflows/agents-keepalive-loop-reporter.ymland.github/workflows/agents-keepalive-loop-reporter.yml, deriveagent_execution_startedfrom exact originating worker-job evidence with started, definitely-not-started, and unknown states; make absent PR association and API errors retryable rather than asserting false..github/scripts/keepalive_loop.jsand.github/scripts/keepalive_authority_state.js, reconcile the exact receipt and owner attempt independently of fresh authority-failure classification and presentationstate.running, and never reopen a consumed or confirmed receipt when worker execution is started or unknown.templates/consumer-repo/.github/scripts/copies byte-aligned, update.github/sync-manifest.ymldescriptions or entries as appropriate, document recovery ownership indocs/keepalive/Agents.mdanddocs/keepalive/GoalsAndPlumbing.md, and add focused regression tests.Acceptance criteria
pytest -q tests/workflows/test_keepalive_authority_delivery.pyand the applicable authority and runner suites.Head SHA: 6c314e2
Latest Runs: ✅ success — Gate
Required: gate: ✅ success
Summary by CodeRabbit