fix(sync): distinguish missing active PR from missing delivery record

[stranske]

Source: Issue #3342

Closes #3342

Automated Status Summary

Scope

maint71_merge_sync_prs.js reports missing_delivery_record for two different states. selectMergeEligibleSyncPr in .github/scripts/sync_pr_merge_contract.js:776-780 returns eligibility: null when no open PR matches the expected branch for the active sync hash, and returns { eligible: false, reason: 'missing_delivery_record' } only when a matching PR exists whose body has no delivery record. The caller at .github/scripts/maint71_merge_sync_prs.js:1987 then does selection.eligibility?.reason || 'missing_delivery_record', so the null case prints the same words as the real missing-record case.

Observed 2026-09-04: Maint 71 run 33825274345 (a workflow_dispatch with an empty active_sync_hash) printed Delivery contract blocks merge: missing_delivery_record for stranske/Travel-Plan-Permission while the canary PR #1480's body carried a valid sync-pr-delivery-record:v1 whose mergeEligibility evaluates to current_unexpired. The operator reading that line looked for a missing record that was present; the actual state was "no PR matched the expected lane because no sync hash was supplied". One sentinel meaning both "unmeasurable" and "measured absent" is the latched-gate reporting defect: the message points the reader at the wrong drain.

Context for Agent

Related Issues/PRs

• #1480

Tasks

• selectMergeEligibleSyncPr already exposes missingExpected and expectedBranch on the selection; the caller can print no_active_sync_pr (expected <branch>, active hash '<hash>') when selection.eligibility is null.
• Keep missing_delivery_record for the case that actually means it. Do not change mergeEligibility.

Acceptance criteria

• node --test .github/scripts/__tests__/*.test.js exits 0 and includes the two new cases.
• Deliberate-break → revert: restore the || 'missing_delivery_record' default on a scratch copy and run the tests; case (a) must fail; revert; tests green again.
• A report-only Maint 71 run against open sync candidates with a nonmatching active_sync_hash prints no_active_sync_pr with the expected branch and active hash, and persists that delivery_reason in artifacts/sync-pr-merge-report.json. An empty selector preserves newest-candidate selection and must not falsely report either missing state for a valid record. Verify with node --test .github/scripts/__tests__/maint71_merge_sync_prs.test.js and retain the branch-specific Maint 71 smoke run URL/report.

Implementation and validation evidence

Maint 71 now reports no_active_sync_pr with the expected branch and active hash when open sync PRs exist but none matches the selector. The JSON row carries the same delivery_reason; target_missing and merge eligibility behavior stay unchanged. A selected PR without a record still reports missing_delivery_record.

The current selector already handles an empty hash by selecting the newest candidate. The issue's historical empty-hash reproduction therefore no longer describes main. An executor regression explicitly preserves this behavior; changing it would violate the issue's non-goal of preserving selection/merge semantics. The second default in confirmExactHeadReviewClear correctly represents an existing PR losing its record and was retained. This is Workflows-only reporting; no distributed template or sync-manifest entry changes.

• Full JS suite: 1,528 passed, 1 skipped, zero failed. Related workflow tests: 28 passed.
• Original source and deliberate misreport mutation each: 1 failed, 2 passed. Restored: 3 passed; node syntax and diff-check pass.
• Live report-only empty-selector smoke dispatched exactly once: https://github.com/stranske/Workflows/actions/runs/34284427604 (dry_run=true, auto_merge=false, cleanup_branches=false, explicit Travel-Plan-Permission target). Completed successfully. Downloaded sync-pr-merge-and-canary-evidence: exact branch/head, dry_run=true, auto_merge=false, empty sync_hash; one result Travel-Plan-Permission: no_prs, zero mutation/error rows. Fresh live inventory confirms no open PRs in that repo. This validates branch execution and artifact delivery only; no live missing-target fixture was available. The actual-executor fixtures prove the three reporting cases; do not claim the live run exercised them.
• Fresh Gate34284379913 completed SUCCESS at unchanged295e1af860811477de3320edc4758c60dd5c01b0; earlier startup cancellations are not product-check failures or PASS. Zero full GraphQL review threads at audit.
• Evidence directory: /Users/teacher/.codex/automations/pd-workloop-resume/runs/20260908T2201.

All implementation acceptance is covered by the executor regressions and retained smoke artifact; the historical live candidate1480 is no longer open. Keepalive/closer should preserve the stated evidence limit instead of creating artificial sync delivery work.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-08T22:11:56.894484Z	295e1af	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[stranske-keepalive]

🤖 Keepalive Loop Status

PR #3415 | Agent: Codex | Iteration 2/12

Current State

Metric	Value
Iteration progress	[##--------] 2/12
Action	run (agent-run-skipped)
Agent status	✅ ALL TASKS COMPLETE
Gate	success
Tasks	5/5 complete
Timeout	45 min (default)
Timeout usage	3m elapsed (7%, 42m remaining)
Keepalive	✅ enabled
Autofix	❌ disabled

Last Codex Run

Result	Value
Status	⏭️ Skipped
Reason	agent-run-skipped

To retry:

• Add the agent:retry label, OR
• Wait for conditions to resolve (e.g., Gate success, labels present)

🔍 Failure Classification

| Error type | infrastructure |
| Error category | transient |
| Suggested recovery | Capture logs and context; retry once and escalate if the issue persists. |

[stranske-keepalive]

Keepalive Work Log (click to expand)

#	Time (UTC)	Agent	Action	Result	Files	Tasks	Progress	Commit	Gate
0	2026-09-08 22:09:26	Codex	wait (gate-cancelled-transient-transient)	skipped	—	0	0/5	—	cancelled
1	2026-09-08 22:12:16	Codex	run (bypass-rate-limit-gate)	success	32 file(s)	+4	4/5	—	cancelled
2	2026-09-08 22:18:46	Claude	run (verify-acceptance)	success	—	+1	5/5	—	success
2	2026-09-08 22:27:55	Codex	wait (gate-not-success)	skipped	—	0	5/5	—	—
2	2026-09-08 22:30:38	Codex	run (agent-run-skipped)	skipped	—	0	5/5	—	success

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 6f78dce0-f598-4ad3-a2ea-f282b8092ec9

📥 Commits

Reviewing files that changed from the base of the PR and between 551f1a9 and 295e1af.

📒 Files selected for processing (3)

• .github/scripts/__tests__/maint71_merge_sync_prs.test.js
• .github/scripts/maint71_merge_sync_prs.js
• docs/ops/CONSUMER_REPO_MAINTENANCE.md

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

---

📝 Walkthrough

Walkthrough

Maint 71 now distinguishes missing active sync PRs from missing delivery records. New tests cover both classifications and empty-hash candidate selection. Operations documentation defines the resulting decision-log and JSON-row behavior.

Changes

Maint 71 delivery classification

Layer / File(s)	Summary
Update delivery reason reporting .github/scripts/maint71_merge_sync_prs.js, docs/ops/CONSUMER_REPO_MAINTENANCE.md	Maint 71 reports no_active_sync_pr with active hash metadata when no expected sync PR exists. It retains missing_delivery_record for selected PRs without delivery records. The documentation describes these classifications and empty-selector behavior.
Validate selection classifications .github/scripts/__tests__/maint71_merge_sync_prs.test.js	Tests cover nonmatching hashes, missing delivery records, and empty-hash dispatches with valid delivery records.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 295e1

Maint 71 now reports absent matching sync PRs separately from selected PRs lacking delivery records, with coverage and documentation for the distinct outcomes. No merge-blocking risk remains.

Suggested reviewers: codex-automation

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes satisfy issue [#3342]. They distinguish no_active_sync_pr from missing_delivery_record, preserve target-missing and empty-hash selection behavior, update fallback handling, add focused…
Out of Scope Changes check	✅ Passed	The changes remain within issue [#3342]. The implementation, tests, and documentation directly support the requested diagnostic distinction and do not modify merge semantics or sync-manifest behavior.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: distinguishing a missing active sync PR from a missing delivery record.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/issue-3342-sync-reasons

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Runner dispatch state for codex on PR #3415. Do not edit.

[github-actions]

Runner dispatch state for claude on PR #3415. Do not edit.

[Copilot]

🟡 Changes recommended

The new no_active_sync_pr fallback in the contract-blocked path is unsafe/incomplete for the “eligibility is null” case and should be handled explicitly to avoid incorrect output and potential runtime errors.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes Maint 71’s merge-block reporting so operators can distinguish “no open sync PR matched the expected branch” from “a selected PR is missing a delivery record,” aligning logs and JSON output with the delivery/merge contract.

Changes:

• Update Maint 71 decision logging + per-repo JSON rows to emit no_active_sync_pr (including expected branch + active hash) when the expected PR is missing.
• Preserve missing_delivery_record strictly for the “selected PR exists but has no delivery record” case.
• Add focused Node tests covering the two distinct states and an empty-hash dispatch sanity case.

File summaries

File	Description
docs/ops/CONSUMER_REPO_MAINTENANCE.md	Documents the two distinct Maint 71 reporting states and how to interpret them.
.github/scripts/maint71_merge_sync_prs.js	Implements distinct no_active_sync_pr vs missing_delivery_record reporting in logs/JSON.
.github/scripts/tests/maint71_merge_sync_prs.test.js	Adds regression tests for the two reporting states and the empty-hash selection behavior.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[stranske]

Review disposition for 295e1af860811477de3320edc4758c60dd5c01b0 (thread PRRT_kwDOQprj9M6gbgXT): the null-eligibility concern is already guarded in this caller. .github/scripts/maint71_merge_sync_prs.js:1899-1920 emits the detailed no_active_sync_pr log and target_missing row (expected branch, active hash, open PRs), then continues before dereferencing selection.active. Empty candidate lists continue at 1886. generatedPrsForSyncSelector filters the dev-tool lane before that empty-list check. The second selector call uses the same candidate array and hash; adding the observed tree hash changes eligibility only, not which PR is active. An existing PR without a record returns the explicit missing_delivery_record eligibility result.

Fresh validation on the unchanged head: node --test .github/scripts/__tests__/maint71_merge_sync_prs.test.js .github/scripts/__tests__/sync_pr_merge_contract.test.js passes all 85 tests. In an isolated source/config copy, the three executor tests pass; disabling only the early missingExpected guard makes the missing-target case fail (error rather than target_missing, 1 failed/2 passed); restoring it returns 3 passed. No implementation changes are needed for this finding. The existing regression checks the exact log context and JSON fields requested by the reviewer. The initial incomplete scratch fixture lacked the manifest/policy and was rejected as invalid evidence; the cited red/green run uses the complete required configuration.

Premerge absent-check audit: the mirror reporter is missing (exit 2), so this is an explicit manual audit, not a reporter PASS. All 126 reported check runs on this head and 179 from reference #3404 were enumerated. The reference-only Python reusable children are conditional on Python/run_core changes; this PR changes only two JS files and one operations Markdown file, and its parent python ci reports SKIPPED. Health 40 workflow lint children and Health 51 zizmor/its SARIF check are workflow-path gated; Health 73 template completeness, Health 74 template drift, Health 69 shadow evidence, and Selftest Reusables (rate gate/scenarios/aggregate/publish) have path filters that do not match these three files. Semgrep/SARIF children have a reported skipped parent after successful classification. The post-merge verifier is a later lifecycle stage. These reference-only contexts are inapplicable under the committed workflow conditions, not missing expected executions. Gate34284379913 reports every declared job, with successful scripts/package/docs/ledger/test-quality/consistency/environment/seal/summary checks and conditional Python/Docker skips. The separate Selftest CI reports successful Python, JS, package, and lint/format/YAML tests. Health45, template-sync, manifest, workflow syntax, PR11, dependency-enforcement and Copilot checks also succeeded. Earlier startup cancellations have successful replacement runs; cancelled rows are not treated as passes.

The required summary check is SUCCESS on Gate34284379913. Original push was recorded before PR creation at 22:09:26 UTC; the exact head has remained unchanged for more than seven minutes. Final merge still requires a fresh unchanged-head, zero-active-thread, required-check and direct mergeability read.

[github-actions]

Provider Comparison Report

Provider Summary

Provider	Model	Verdict	Confidence	Summary
openai	gpt-5.6-terra	PASS	93%	The change separates the previously conflated states at the Maint 71 caller: a null eligibility/no expected active-sync PR is reported as no_active_sync_pr and includes both the expected branch a...
anthropic	claude-sonnet-5	PASS	68%	The PR is narrowly scoped (small code change of +6/-2 lines in maint71_merge_sync_prs.js, plus a new 122-line test file and doc update), consistent with the stated goal of distinguishing the null-e...

📋 Full Provider Details (click to expand)

openai

• Model: gpt-5.6-terra
• Verdict: PASS
• Confidence: 93%
• Scores:
  • Correctness: 10.0/10
  • Completeness: 9.0/10
  • Quality: 9.0/10
  • Testing: 9.0/10
  • Risks: 9.0/10
• Summary: The change separates the previously conflated states at the Maint 71 caller: a null eligibility/no expected active-sync PR is reported as no_active_sync_pr and includes both the expected branch and supplied active hash, while an actual evaluated missing record retains missing_delivery_record. The delivery reason is persisted in the merge-report output. The new focused Maint 71 tests exercise the nonmatching-selector/report case and the empty-selector valid-record behavior, including the distinction that would regress if the old fallback were restored. The implementation is small, readable, scoped to reporting behavior, and does not alter merge-eligibility evaluation or introduce meaningful compatibility, security, or performance risk.

anthropic

• Model: claude-sonnet-5
• Verdict: PASS
• Confidence: 68%
• Scores:
  • Correctness: 8.0/10
  • Completeness: 8.0/10
  • Quality: 7.0/10
  • Testing: 7.0/10
  • Risks: 8.0/10
• Summary: The PR is narrowly scoped (small code change of +6/-2 lines in maint71_merge_sync_prs.js, plus a new 122-line test file and doc update), consistent with the stated goal of distinguishing the null-eligibility 'no_active_sync_pr' case from the true 'missing_delivery_record' case. CI (pr-00-gate, pr-11-ci-smoke, selftest-ci including JS/Python tests) all passed on the merge commit, supporting that node --test suites are green. The new test file appears to add scenarios for both selection outcomes (with/without delivery record), addressing the acceptance criteria's requirement for new test cases. Documentation was also updated to reflect the new reporting behavior. Due to truncation of the actual script diff in the review context, full line-level verification of the conditional logic was not possible, but the size, structure, and passing CI strongly suggest the implementation matches the described fix. No functional regressions or security/performance risks are evident given the limited, targeted scope of the change.
• Concerns:
  • The full diff for maint71_merge_sync_prs.js (the actual +6/-2 fix) was truncated in the review context, so the exact conditional logic distinguishing no_active_sync_pr from missing_delivery_record could not be directly inspected line-by-line.
  • The linked issue [P3] Maint 71 reports missing_delivery_record when no sync PR matched the active hash; name the two states separately #3342 still shows unchecked task/acceptance boxes, which is expected since it's a tracking issue, but it means external confirmation of the fix relies on the PR's own checklist and CI rather than a fully re-verified issue state.
  • The 'deliberate-break → revert' verification step described in the acceptance criteria is a manual QA step; there's no artifact in the diff (e.g., a git-bisect script or documented log) proving this was actually executed, though CI passing is a reasonable proxy.
  • Cannot fully confirm the report artifact (artifacts/sync-pr-merge-report.json) persists delivery_reason correctly for the new state without seeing the complete script diff.

Agreement

• Verdict: PASS (all providers)
• Completeness: scores within 1 point (avg 8.5/10, range 8.0-9.0)
• Risks: scores within 1 point (avg 8.5/10, range 8.0-9.0)

Disagreement

Dimension	openai	anthropic
Correctness	10.0/10	8.0/10
Quality	9.0/10	7.0/10
Testing	9.0/10	7.0/10

Unique Insights

• openai: The change separates the previously conflated states at the Maint 71 caller: a null eligibility/no expected active-sync PR is reported as no_active_sync_pr and includes both the expected branch and supplied active hash, while an actual evaluated missing record retains missing_delivery_record....
• anthropic: The full diff for maint71_merge_sync_prs.js (the actual +6/-2 fix) was truncated in the review context, so the exact conditional logic distinguishing no_active_sync_pr from missing_delivery_record could not be directly inspected line-by-line.; The linked issue [P3] Maint 71 reports missing_delivery_record when no sync PR matched the active hash; name the two states separately #3342 still shows unchecked task/acceptance boxes, which is expected since it's a tracking issue, but it means external confirmation of the fix relies on the PR's own checklist and CI rather than a fully re-verified issue state.; The 'deliberate-break → revert' verification step described in the acceptance criteria is a manual QA step; there's no artifact in the diff (e.g., a git-bisect script or documented log) proving this was actually executed, though CI passing is a reasonable proxy.; Cannot fully confirm the report artifact (artifacts/sync-pr-merge-report.json) persists delivery_reason correctly for the new state without seeing the complete script diff.

🔍 LangSmith Traces

• openai
• anthropic

[stranske]

Verifier disposition complete — source #3342 closed

Post-merge verification for exact head 295e1af860811477de3320edc4758c60dd5c01b0 (merge 1f269cd20716e0ebb4ddd54c0923be9d44a8d5f1) is complete.

Provider comparison (durable report): OpenAI PASS 93%, Anthropic PASS 68%. Both providers agree the change correctly distinguishes no_active_sync_pr (null eligibility / no matching expected branch) from missing_delivery_record (matching PR without delivery record), persists delivery_reason in the merge report, and is covered by focused Maint 71 tests.

Human-disposition audit: Anthropic's caveats about truncated diff context and unchecked issue boxes are dispositioned:

• The +6/-2 caller fix and 122-line test file were audited in the pre-merge round (85 focused tests PASS; guard-removal mutation 1FAIL/2PASS/restored 3PASS; report-only smoke run 34284427604 SUCCESS).
• Zero active non-outdated review threads remain.
• Acceptance criteria (node --test, deliberate-break→revert, nonmatching-selector smoke) were satisfied before merge; no bounded follow-up gap remains.

Closing source #3342. No follow-up debt.