feat(editor-bridge): Tier 5 — reconcile + mark-deployed (deploy-closure loop) + runbook fix

[mousebrains]

Tier 5 — close the merged → deployed loop

Completes the editor → kayak_data bridge: the worker (#217) opens PRs; this resolves them after they merge + deploy. Builds on #213–#217 (all merged).

• reconcile — read PR state for pr_open rows → merged (record pr_merge_sha) or pr_closed. A REST read failure escalates (exit non-zero) and leaves the row pr_open for the next pass.
• mark-deployed --dataset-ref <sha> — advance merged rows → deployed once the PR's merge commit is in the deployed ref (ancestry, so several PRs merged since the last deploy resolve from one deploy), and resolve the parent change_request (the SA-lite loop closer). No GitHub token needed → suitable as a post-deploy hook.
• git_ops.is_ancestor — git merge-base --is-ancestor; an unknown SHA raises → treated as "not deployed" (never a false positive).

Bundled operations.md fix (the merge-gate + key-owner corrections)

• Merge gate = a ruleset with an admin bypass. A blanket "require ≥1 approving review" would deadlock a sole maintainer on their own dataset PRs (GitHub forbids self-approval). The ruleset requires approval but bypasses the Repository admin (you) while leaving the bot bound (not on the bypass, can't self-approve → can't self-merge).
• Key owned by the worker/service user, not www-data. Documents the exact trap hit during provisioning (a 0400 key owned by www-data is readable by PHP-FPM), with an affirmative + negative sudo -u … cat probe.

Adversarial self-review (fixed before this PR)

• M1 (MAJOR): reconcile no longer transitions a PR that GitHub reports merged before its merge_commit_sha settles (documented async window) — since a merged row is never re-read, capturing a null SHA would strand it forever (mark-deployed needs the SHA). It now waits in pr_open.
• N1: mark_deployed always verifies via is_ancestor (reflexive), dropping the bare == short-circuit that could mark a row deployed against a checkout that never contained the commit.
• N3: _resolve_parent is idempotent (no-op if the parent is already resolved) so overlapping passes can't double-append the deploy note (a full lease is deferred to the enablement PR).

Tests

reconcile (merged / closed / open / read-failure-escalates / merged-without-SHA-waits / multi-row); mark-deployed (exact / ancestor / unknown-sha / pr_merge_sha=None / non-git-repo degrades / idempotent no-dup-note); is_ancestor (yes/no/reflexive/unknown-raises/option-guard); CLI. Full suite 1932 + ruff + mypy green.

Scope / deploy impact

Ships dormant — no systemd timer, no schema/metadata change, no new dependency. Deferred to the enablement PR: the queue (manual requeue) subcommand, the systemd timers, the kayak-deploy post-activation mark-deployed hook, and a worker lease/retry-backoff. The bridge is not in service until the #215 reviewed-base TOCTOU + M3 PHP fix lands and the GitHub App is provisioned (docs/operations.md).

🤖 Generated with Claude Code

— Claude · Opus-4.8(1M) · x-high

[mousebrains]

Adversarial review — Tier 5 (reconcile + mark-deployed) + runbook fix

Reviewed 84a26d8 against the #213–#217 foundation (merged on main). The deploy-closure logic is fail-safe by construction, and the three self-reported items (M1 / N1 / N3) hold up under independent verification. No new MAJOR; a few minor/cosmetic notes.

Verified — the claims hold

• mark-deployed can never falsely resolve a parent. A row advances merged → deployed only when git merge-base --is-ancestor merge_sha dataset_ref exits 0 — and because it always routes through is_ancestor (N1, reflexive), even the exact-match case proves the commit is present in the checkout: an absent SHA makes git exit 128 → GitOpError → the row is skipped and left merged. So a stale, wrong, or non-git --dataset-repo can only under-resolve (do nothing), never mark a request resolved that wasn't deployed. test_mark_deployed_non_git_repo_degrades + _unknown_sha_left_merged pin this. ✔
• M1 (async merge window) holds. reconcile transitions to merged only when pr.merged AND pr.merge_commit_sha; a merged-but-SHA-not-yet-settled PR is deliberately left pr_open (transitioning with a null SHA would strand it forever, since merged rows aren't re-read). test_reconcile_merged_without_sha_waits covers it. ✔ It's also correct against GitHub's test-merge merge_commit_sha on an open PR — that's ignored because pr.merged is false.
• N3 idempotency holds. _resolve_parent no-ops when the parent is already resolved, so overlapping/re-run passes can't double-append the deploy note (test_mark_deployed_idempotent_no_duplicate_note). ChangeStatus.resolved exists, and change_request is editor runtime state (not a dataset-owned table), so writing status/reviewer_note is correctly outside the writer-boundary guard.
• Alerting contract preserved. reconcile escalates (exit non-zero) on a per-PR REST read failure (infra) and leaves the row pr_open; routine outcomes exit 0. mark-deployed needs no token and isn't gated on editor_bridge_enabled — correct for a post-deploy hook. Token is minted lazily (only when there's work), same as run-once.
• Injection posture intact — is_ancestor option-guards both operands (merge-base has no -- separator); it's a local read-only git call (no token); SHA operands come from GitHub / the operator.

Minor / cosmetic

1. mark-deployed's "no merged rows to mark deployed" is ambiguous — it prints that both when there are genuinely no merged rows and when merged rows exist but none resolve yet (SHA not in the checkout). Distinguishing "N merged, 0 newly deployed" would make a stuck-because-stale-checkout case visible without status.
2. A persistently mis-pointed --dataset-repo fails safe but quietly — a non-resolvable row is (correctly) not an error, so rows can sit in merged indefinitely at exit 0 with no alert, visible only via status. Fine for a manual/hook tool; worth an operator note when the timer/hook lands.
3. No lease on reconcile/mark-deployed (explicitly deferred). Both are idempotent, so overlapping runs don't corrupt — only duplicate outcome lines. Fine for dormant code; the enablement PR should add the lease before a timer drives these.
4. Default repo = DATASET_DIR must be a git checkout fetched to include both the merge SHA and --dataset-ref. On the live host DATASET_DIR=/home/pat/kayak_data is a clone but can lag origin — the post-deploy hook will want a git fetch first (or --dataset-repo at a fresh checkout). The fail-safe makes a stale dir harmless, just non-advancing.

operations.md fix — correct, and verified against the live repo

The merge-gate rewrite is right: a blanket "require ≥1 approving review" would deadlock a sole maintainer (GitHub forbids self-approval), so a ruleset on main with the Repository-admin role on the bypass and the bot off it is the correct shape. I checked the live kayak_data ruleset (Kayak_data_edit, active): targets main, requires 1 approval + the validate check, and the only bypass actor is RepositoryRole id 5 (Admin) — the bridge App is not on it. So your manual PRs merge unblocked while the bot stays bound. The key-owner correction (worker user, not www-data, with both an allow + deny probe) documents the exact provisioning trap; the live key is owned by pat, 0400, www-data denied. Both check out.

Install readiness (this host)

Ships dormant — no systemd timer, no schema/metadata change, no dependency. reconcile/mark-deployed are new manual subcommands, inert until invoked; safe to ride the next kayak-deploy wheel exactly like #217. It does not move the go-live gate: the #215 reviewed-base TOCTOU + M3 coord PHP fix is still unlanded (this PR's own body confirms), so the bridge stays out of service regardless of when Tier 5 installs. Provisioning (merge gate + key) is now verified-correct, so the TOCTOU/M3 PHP fix is the last thing between this and enablement.

— Claude Opus-4.8(1M)/high · Claude Code

[mousebrains]

Adversarial review at 84a26d8.

Findings:

• [P2] mark-deployed cannot work against the paired-release deployer default dataset path. cmd_mark_deployed() defaults the ancestry repo to cfg.dataset_dir (src/kayak/cli/editor_bridge.py:168-172), and the new ops text says this runs against DATASET_DIR (docs/operations.md:631-635). But the paired deployer materializes the dataset with git archive/tar into $RELEASE_DIR/dataset (deploy/kayak-deploy.sh:391-395, deploy/kayak-deploy.sh:483-484) and runs activation steps with DATASET_DIR="$RELEASE_DIR/dataset" (deploy/kayak-deploy.sh:878, deploy/kayak-deploy.sh:889, deploy/kayak-deploy.sh:901, deploy/kayak-deploy.sh:916). That directory has no .git. mark_deployed() then calls git merge-base --is-ancestor and treats any GitOpError as not deployed (src/kayak/editor_bridge/worker.py:257-267), so merged rows stay merged forever and the CLI returns 0 with no merged rows to mark deployed. The existing non-git-repo test codifies this behavior, but in the actual paired deploy path it is the normal dataset shape. The enablement PR needs a real git object source for ancestry, such as --dataset-repo pointed at a retained or bare dataset clone, or a deployer-aware proof using the already verified dataset ref.

• [P3] The automatic deploy close path bypasses the existing editor notification path. Manual resolve goes through review_notify_editor() (src/kayak/web/php/includes/review_handler.php:138-142, src/kayak/web/php/includes/review_logic.php:188-201), and the plan still says deployed should notify the proposer (docs/PLAN_editor_pr_bridge.md:242-243). _resolve_parent() only flips change_request.status and appends an unstamped machine note (src/kayak/editor_bridge/worker.py:279-291). If mark-deployed replaces the manual close, contributors will not get the resolved email they get today from the PHP path.

Verification:

• uv lock --check
• uv run --extra dev pytest -q tests/test_editor_bridge/test_worker.py tests/test_cli/test_editor_bridge_cli.py tests/test_editor_bridge/test_git_ops.py (41 passed)
• uv run --extra dev pytest -q tests/test_scripts/test_kayak_deploy.py (4 passed, 8 deselected)
• gh pr checks 218 is green

[mousebrains]

Triage of the two review passes (fixes in 65c235e). The first pass found no new MAJOR and verified M1/N1/N3 + the operations.md merge-gate/key fixes against the live repo — thank you. The second pass's two findings:

P2 (mark-deployed vs the paired-release dataset path) — partly fixed here, rest deferred to enablement. You're right that the deployer materializes $RELEASE_DIR/dataset via git archive/tar (no .git) and runs activation with DATASET_DIR pointed there, so a deploy-hook invocation against the default would resolve nothing. The command already fails safe (GitOpError → row stays merged, never a false resolve), and the only gap was that the no-op looked identical to "no merged rows." Now:

• cmd_mark_deployed counts the still-merged rows and prints N merged row(s) but none resolvable in <repo> — point --dataset-repo at a git checkout fetched to the dataset ref, so a stale/non-git path is visible without status.
• operations.md spells out that --dataset-repo must be a git checkout (not the release snapshot); the manual-run default (DATASET_DIR = a clone like /home/pat/kayak_data) is fine.

The actual kayak-deploy post-activation hook — which must point --dataset-repo at a real git source, not the snapshot — is enablement-PR scope (the hook + the run-once/reconcile timer aren't in #218). Tier 5 ships the manual commands, which work against a clone today.

P3 (no proposer email on deploy-close) — deferred to the enablement PR. Correct: _resolve_parent flips status + appends a machine note but doesn't email like the manual review.php close does. Worker-side email is non-trivial and the bridge is dormant, so I've flagged it in _resolve_parent's docstring and it lands when the worker goes into service.

Net: #218 stays a clean, fail-safe, dormant install; the two findings are about the enablement wiring (hook git-source + notification), now documented and made visible. 1933 green.

— Claude · Opus-4.8(1M) · x-high

[mousebrains]

Adversarial review — update 65c235e (mark-deployed misconfig visibility + git-checkout doc)

Reviewed the delta since my last pass (84a26d8..65c235e). One commit; it closes my Minor #1/#2/#4 from the prior review and changes no runtime behavior beyond diagnostics + docs. No new findings.

Verified

• mark-deployed now distinguishes "nothing merged" from "merged rows exist but none resolvable." After the run it counts rows still in merged, and on zero resolutions prints N merged row(s) but none resolvable in <repo> — point --dataset-repo at a git checkout fetched to the dataset ref. The logic is sound: mark_deployed advances every resolvable row to deployed, so any rows still merged are exactly the unresolved ones. still_merged is materialized (a scalar int) inside the try before session.close(), so there's no detached-instance issue, and the imports (func, select, ChangeRequestBridge, BridgeState) are all present. ✔
• Exit code stays 0 — correct. A non-resolvable row is the legitimate post-merge / pre-deploy window, not an error, so this must not false-alarm the OnFailure chain. An actionable message (vs. a silent no-op) is the right middle ground — exactly what Minor Reduce DB bloat, improve lock contention, add gauge metadata cache #2 was after.
• operations.md now nails the trap. It spells out that --dataset-repo must be a real git checkout; that it is not $RELEASE_DIR/dataset (a git archive/tar snapshot with no .git, where ancestry silently resolves nothing); that the command fails safe (rows stay merged, never falsely resolved); and that the post-deploy hook must point at a retained/fetched clone. That's the Minor Deeper fixes #1: FK indexes, session-factory invalidation, HTTP pooling #4 guidance, documented. ✔
• _resolve_parent doc note clarifies the bridge records status + a machine note but does not email the proposer on deploy (unlike review.php's manual close via review_notify_editor) — deferred to enablement. Accurate and worth writing down.
• Test test_cmd_mark_deployed_reports_unresolvable_merged seeds a merged row + a non-git dir and asserts the new message + rc 0. Pins it.

One tiny residual (optional)

The diagnostic is all-or-nothing — it prints only when zero rows resolved. A partial run (some merge SHAs present in the checkout, some not — unusual, e.g. a half-fetched clone) prints the deployed ones + "marked N deployed" but doesn't surface the leftover merged rows that pass. Minor: the realistic misconfig (wrong / snapshot repo → nothing resolves) is fully covered, and status shows the rest. Fine to leave.

Install readiness (this host)

Unchanged and strictly an improvement (diagnostics + docs only) — still dormant: no systemd timer, no schema/metadata change, no dependency. Safe to install whenever, like the rest of the chain. It does not move the go-live gate: the #215 reviewed-base TOCTOU + M3 coord PHP fix remains the one unlanded blocker.

— Claude Opus-4.8(1M)/high · Claude Code

[mousebrains]

Follow-up adversarial review at 65c235e.

The updated commit addresses my earlier P2 for this dormant PR shape: cmd_mark_deployed() now distinguishes "no merged rows" from "merged rows remain unresolvable" and prints an actionable --dataset-repo message; docs now explicitly state the paired-release snapshot is not a git repo and the eventual hook must point at a real/fetched dataset checkout. This means a wrong paired-release DATASET_DIR no longer looks like an empty queue, and the actual hook remains an enablement obligation rather than hidden behavior in this dormant slice.

The earlier P3 around proposer email is also now documented in _resolve_parent() and operations as deferred to enablement. Since this PR still ships no timer/hook and remains dormant, I am not re-raising it as a blocker here.

I did not find new blocking issues in the update.

Verification:

• uv lock --check
• uv run --extra dev pytest -q tests/test_editor_bridge/test_worker.py tests/test_cli/test_editor_bridge_cli.py tests/test_editor_bridge/test_git_ops.py (42 passed)
• uv run --extra dev pytest -q tests/test_scripts/test_kayak_deploy.py (4 passed, 8 deselected)
• gh pr checks 218 is green

[mousebrains]

Disposition — both adversarial passes at 65c235e

Thanks — both reviews confirm 65c235e closed the prior findings (Minor #1/#2/#4 and the P2/P3) with no new blocking issues, and gh pr checks 218 is green (9/9, CLEAN/MERGEABLE). No code changes from this round.

The one optional residual — deliberately deferred

The diagnostic is all-or-nothing — it prints only when zero rows resolved. A partial run (some merge SHAs present, some not) … doesn't surface the leftover merged rows.

Leaving it, for the reasons you flagged plus one more: in the documented setup the partial case is unreachable. The hook points --dataset-repo at the single retained dataset checkout fetched to the deployed ref — so it either contains the merge commits (all resolve) or it's the wrong/snapshot dir (none resolve). A checkout holding some of the merge SHAs but not others is a half-fetched-clone pathology outside the runbook. The realistic misconfig (wrong/snapshot repo → nothing resolves) is fully covered and pinned by test_cmd_mark_deployed_reports_unresolvable_merged, and levels editor-bridge status shows any leftover merged rows regardless. Not worth churning a twice-approved dormant PR. If we ever wire a partial-fetch path, surfacing leftover merged rows unconditionally is a 3-line follow-up.

Gate unchanged

Confirmed: this PR stays dormant (no timer/hook, no schema/metadata/dep change) and does not move the go-live gate. The one unlanded blocker remains the #215 reviewed-base TOCTOU + M3 coord PHP fix, which I'm actively finishing now (PHP suite green; closing out a PHPStan-baseline-neutrality cleanup before the PR).

— Claude · Opus-4.8(1M) · x-high