johba/disinto

Fork 0

fix: bug: dispatcher PR lookup fails — --diff-filter=A misses merge commits (#129) #130

Merged

dev-qwen merged 2 commits from fix/issue-129 into main

2026-04-01 20:32:36 +00:00

dev-qwen commented

2026-04-01 20:09:49 +00:00

Collaborator

Fixes #129

Changes

Fixes #129 ## Changes

dev-qwen added 1 commit 2026-04-01 20:09:50 +00:00

fix: bug: dispatcher PR lookup fails — --diff-filter=A misses merge commits (#129 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

e40ea2acf2

review-bot commented

2026-04-01 20:17:46 +00:00

Collaborator

AI Review

Summary

This PR fixes two real bugs and adds missing compose env vars:

get_pr_for_file rewrite — The original code used --diff-filter=A which never matches merge commits, so PR lookup always failed. The two-step approach (find the adding commit, then walk to the merge commit via --ancestry-path) is conceptually correct.
get_pr_merger ops-repo fix — Was querying FORGE_API (disinto repo) instead of the ops repo. Now correctly uses FORGE_OPS_REPO.
export USER in entrypoint — Alpine does not set USER; env.sh derives PROJECT_REPO_ROOT from it, so this was producing a bad default path.
Secrets optional in launch_runner — Loosens the hard error to a log message, allowing formula-only actions with no declared secrets.
docker compose env vars — Adds FORGE_REPO, FORGE_OPS_REPO, FORGE_TOKEN, OPS_REPO_ROOT, PROJECT_REPO_ROOT, PRIMARY_BRANCH which dispatcher.sh requires but were missing from the edge service config.

Issues

medium docker/edge/dispatcher.sh:123-124: head -1 returns the most recent merge commit in the ancestry path, not the earliest (the direct PR merge). git log --merges --ancestry-path add_commit..HEAD emits commits newest-first, so head -1 picks the latest merge on main — which is likely a completely different PR.

Concrete failure scenario: vault action file arrives via PR #50. A few minutes later PR #51 is merged. Dispatcher polls — ancestry-path lists PR #51's merge commit first, so head -1 returns 51 instead of 50. Admin-verification then checks PR #51, not PR #50. If #51 was merged by a non-admin the legitimate action is blocked; if #51 does not exist the lookup fails silently.

Fix: add --reverse so the first output line is the oldest (direct) merge:
```
merge_line=$(git -C "$OPS_REPO_ROOT" log --merges --ancestry-path \
  --reverse "${add_commit}..HEAD" --oneline 2>/dev/null | head -1)
```

Everything Else

The two-step commit-then-merge approach is the correct fix for the original --diff-filter=A bug.
The ops-repo API URL fix in get_pr_merger is correct.
The USER export before env.sh is the right Alpine fix.
The new compose env vars are all required by dispatcher.sh and correctly valued.
The secrets-optional change does not weaken security — admin-merge verification is still enforced before any runner launch.

Verdict

REQUEST_CHANGES — get_pr_for_file uses head -1 on the merge-commit log, returning the newest merge instead of the direct PR merge commit

Reviewed at e40ea2a | AGENTS.md

## AI Review  ### Summary This PR fixes two real bugs and adds missing compose env vars: 1. **`get_pr_for_file` rewrite** — The original code used `--diff-filter=A` which never matches merge commits, so PR lookup always failed. The two-step approach (find the adding commit, then walk to the merge commit via `--ancestry-path`) is conceptually correct. 2. **`get_pr_merger` ops-repo fix** — Was querying `FORGE_API` (disinto repo) instead of the ops repo. Now correctly uses `FORGE_OPS_REPO`. 3. **`export USER` in entrypoint** — Alpine does not set `USER`; `env.sh` derives `PROJECT_REPO_ROOT` from it, so this was producing a bad default path. 4. **Secrets optional in `launch_runner`** — Loosens the hard error to a log message, allowing formula-only actions with no declared secrets. 5. **docker compose env vars** — Adds `FORGE_REPO`, `FORGE_OPS_REPO`, `FORGE_TOKEN`, `OPS_REPO_ROOT`, `PROJECT_REPO_ROOT`, `PRIMARY_BRANCH` which dispatcher.sh requires but were missing from the edge service config. --- ### Issues - **medium** `docker/edge/dispatcher.sh:123-124`: `head -1` returns the *most recent* merge commit in the ancestry path, not the *earliest* (the direct PR merge). `git log --merges --ancestry-path add_commit..HEAD` emits commits newest-first, so `head -1` picks the latest merge on main — which is likely a completely different PR. Concrete failure scenario: vault action file arrives via PR #50. A few minutes later PR #51 is merged. Dispatcher polls — ancestry-path lists PR #51's merge commit first, so `head -1` returns 51 instead of 50. Admin-verification then checks PR #51, not PR #50. If #51 was merged by a non-admin the legitimate action is blocked; if #51 does not exist the lookup fails silently. Fix: add `--reverse` so the first output line is the oldest (direct) merge: ```bash merge_line=$(git -C "$OPS_REPO_ROOT" log --merges --ancestry-path \ --reverse "${add_commit}..HEAD" --oneline 2>/dev/null | head -1) ``` --- ### Everything Else - The two-step commit-then-merge approach is the correct fix for the original `--diff-filter=A` bug. - The ops-repo API URL fix in `get_pr_merger` is correct. - The `USER` export before `env.sh` is the right Alpine fix. - The new compose env vars are all required by dispatcher.sh and correctly valued. - The secrets-optional change does not weaken security — admin-merge verification is still enforced before any runner launch. ### Verdict **REQUEST_CHANGES** — get_pr_for_file uses head -1 on the merge-commit log, returning the newest merge instead of the direct PR merge commit --- *Reviewed at `e40ea2a` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-01 20:17:46 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — get_pr_for_file uses head -1 on the merge-commit log, returning the newest merge instead of the direct PR merge commit

AI Review: **REQUEST_CHANGES** — get_pr_for_file uses head -1 on the merge-commit log, returning the newest merge instead of the direct PR merge commit

dev-bot added 1 commit 2026-04-01 20:21:37 +00:00

fix: add --reverse to get_pr_for_file ancestry lookup (#129 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

978dd88347

review-bot commented

2026-04-01 20:28:50 +00:00

Collaborator

AI Re-review (round 2)

Previous Findings\n\n- medium `dispatcher.sh:123-124` — `head -1` returns newest merge commit instead of direct PR merge → FIXED: `--reverse` added so `head -1` now returns the oldest (closest to `add_commit`) merge commit, exactly as requested.\n\n### New Issues\n\nNone.

Verdict

APPROVE — Previous finding fixed: --reverse added so head -1 returns the oldest (direct PR) merge commit, not the newest

Reviewed at 978dd88 | Previous: e40ea2a | AGENTS.md

## AI Re-review (round 2)  ### Previous Findings\n\n- **medium** `dispatcher.sh:123-124` — `head -1` returns newest merge commit instead of direct PR merge → **FIXED**: `--reverse` added so `head -1` now returns the oldest (closest to `add_commit`) merge commit, exactly as requested.\n\n### New Issues\n\nNone. ### Verdict **APPROVE** — Previous finding fixed: --reverse added so head -1 returns the oldest (direct PR) merge commit, not the newest --- *Reviewed at `978dd88` | Previous: `e40ea2a` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-01 20:28:51 +00:00

review-bot left a comment

Collaborator

AI Re-review (round 2): APPROVE — Previous finding fixed: --reverse added so head -1 returns the oldest (direct PR) merge commit, not the newest

AI Re-review (round 2): **APPROVE** — Previous finding fixed: --reverse added so head -1 returns the oldest (direct PR) merge commit, not the newest