Skip to content

ci: fix monitor-ci.sh grabbing the wrong workflow run - #57

Merged
jnasbyupgrade merged 2 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:fix/ci-monitor-wrong-run
Jul 31, 2026
Merged

ci: fix monitor-ci.sh grabbing the wrong workflow run#57
jnasbyupgrade merged 2 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:fix/ci-monitor-wrong-run

Conversation

@jnasbyupgrade

Copy link
Copy Markdown
Contributor

Summary

  • monitor-ci.sh's commit-based run lookup had no --workflow filter, so on a push it could nondeterministically grab either the CI run or the Claude Code Review run for the same SHA (both trigger on the same push) and silently report on the wrong one.
  • Fix: look up each workflow explicitly by name and monitor both to completion. OVERALL: ALL_PASS now requires both to succeed (or, for Claude Code Review, legitimately skip — it no-ops on draft PRs / untrusted-fork PRs).
  • Also fixes a related, previously-silent bug in the same code path: the === BRANCHES: === extraction used an anchored grep '^=== BRANCHES:', which never matched because the jobs-logs API prefixes every line with an ISO-8601 timestamp. That safeguard had never actually fired even when the right run was picked.

Both fixes verified against the real run pair for commit 2cbe91a (the SHA from the original bug report) in Postgres-Extensions/pgxntool-test.

Test plan

  • bash -n syntax check
  • Ran the script directly against the real, already-completed CI + Claude Code Review runs for commit 2cbe91a810283d683eb5c07ef5fd901bd79b22c1 and confirmed it finds both runs by name, prints a clean === BRANCHES: === line, reports all matrix jobs, and returns OVERALL: ALL_PASS
  • CI on this PR itself (monitoring in background)

Every PR push triggers two workflows: CI (the real test matrix, event
pull_request) and Claude Code Review (event pull_request_target). The
old commit-based lookup, `gh run list --commit SHA` with no workflow
filter, returned both runs in an order that isn't guaranteed to put CI
first - so it could silently report on the Claude Code Review run
instead of the actual test matrix, without ever noticing the mismatch.

Fix by looking up each workflow explicitly by name (-w/--workflow) and
monitoring both to completion. OVERALL: ALL_PASS now requires both
runs to succeed (or, for Claude Code Review, legitimately skip - it
no-ops on draft PRs and PRs from untrusted forks).

Also fix the === BRANCHES: === line extraction: the anchored
grep '^=== BRANCHES:' never matched, because every line from the jobs
logs API is prefixed with an ISO-8601 timestamp. Drop the anchor and
strip the timestamp for a clean line.
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 68110091-64a7-4932-99c6-106b69301de6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@jnasbyupgrade
jnasbyupgrade marked this pull request as ready for review July 31, 2026 19:30
@github-actions

Copy link
Copy Markdown

Code review

4 issues found (3 bugs, 1 CLAUDE.md compliance violation). Note: this run's inline-comment tool wasn't available in this session (and gh api is blocked here too), so these are posted as one consolidated comment instead of separate inline threads.

1. Failure-log lookup always resolves to a null job ID

local result
result=$(gh run view "$rid" --repo "$repo" \
--json status,conclusion,jobs \
--jq '{status: .status, conclusion: .conclusion,
jobs: [.jobs[] | {name: .name, status: .status, conclusion: .conclusion}]}' \
2>/dev/null || true)

Step 3's cached results[$wf] projection only keeps name, status, conclusion per job — no databaseId. Step 5 (lines 216-221) now sources failed-job IDs from that same cached blob instead of re-querying GitHub live (as the old code did). Since .databaseId is absent, every failed job's job_id becomes the literal string "null". The subsequent select(.databaseId == $id) then matches every job (all have null), so .[0] returns the wrong job name, and gh run view --job null --log-failed errors out — silently swallowed by 2>&1 ... || true. Net effect: on a real failure the script still reports OVERALL: FAIL, but the === FAILURE (...) === section names the wrong job and prints no actual failure log, which defeats the point of Step 5.

Fix: add databaseId: .databaseId to the Step 3 jq projection (line 168's job object).

2. Un-anchored BRANCHES grep can capture the echoed (unexpanded) step source

# unexpanded ${VARS}) before it runs, so more than one line can match;
# `tail -1` keeps the last (actual, expanded) occurrence.
branches_line=$(gh api "repos/${repo}/actions/jobs/${first_job_id}/logs" \
2>/dev/null | grep "=== BRANCHES:" | tail -1 \
| sed -E 's/^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(\.[0-9]+)?Z //' || true)
fi
if [[ -z "$branches_line" ]]; then

Removing the ^ anchor was necessary (the logs API prefixes every line with a timestamp), but GitHub also echoes the step's verbatim, unexpanded source in a ##[group]Run ... header before the step executes. .github/workflows/ci.yml's "Resolve pgxntool branch" step does a networked git ls-remote before its echo "=== BRANCHES: ..." lines run, so a poll landing in that window sees only the echoed source (literal ${HEAD_OWNER}/${TEST_SHA}, always the "no paired branch" else-branch) in the log so far. tail -1 returns that line, satisfies the while [[ -z "$branches_line" ]] loop, and the script reports garbage branch info with no error — silently defeating the safeguard this script exists for ("always verify the BRANCHES line matches your intent").

Suggested fix: strip the timestamp first, then re-apply the ^ anchor (the echoed source lines are indented, never at column 0):

| sed -E 's/^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(\.[0-9]+)?Z //' \
| grep '^=== BRANCHES:' | tail -1 || true

3. Status label changed to SUCCESS, but this PR's own SKILL.md example still shows PASS

Code:

echo "${results[$wf]}" | jq -r '.jobs[] | "\(if .conclusion == "success" or .conclusion == "skipped" then .conclusion | ascii_upcase elif .conclusion == null then .status else .conclusion | ascii_upcase end) \(.name)"' \

Doc:
[pgxntool-test] Polling... (still running: CI, Claude Code Review)
[pgxntool-test] Run 12345678 (CI) completed: SUCCESS
[pgxntool-test] PASS 🐘 PostgreSQL 12
[pgxntool-test] PASS 🐘 PostgreSQL 15
[pgxntool-test] FAIL 🐘 PostgreSQL 13
[pgxntool-test] === FAILURE (CI): 🐘 PostgreSQL 13 ===
... failure log lines ...

The Step 4 jq filter changed from a hardcoded "PASS" literal to .conclusion | ascii_upcase for successful jobs, so a passing job now prints SUCCESS (verified by running the filter). But the SKILL.md example output block updated in this same PR (the +[pgxntool-test] SKIPPED claude-review line right next to it confirms it was touched) still shows PASS 🐘 PostgreSQL 12 / PASS 🐘 PostgreSQL 15 for successful jobs — code and doc now contradict each other on the real output format.

Suggested fix: update SKILL.md's example lines to SUCCESS 🐘 PostgreSQL 12 / SUCCESS 🐘 PostgreSQL 15.

4. CLAUDE.md violation: echo "" used for a blank line

--argjson id "$job_id" \
'[.jobs[] | select(.databaseId == $id) | .name] | .[0]' 2>/dev/null || true)
echo ""
echo "$label === FAILURE ($wf): ${job_name:-job $job_id} ==="
# Use --log-failed to get only the failed step output, keeping output compact.
gh run view --repo "$repo" --job "$job_id" --log-failed 2>&1 \

This PR restructures the failure-reporting loop and re-adds echo "" (line 222) to print a blank line before each === FAILURE (...) === header. Per the repo's CLAUDE.md, General Guidelines: "NEVER use echo \"\" to print a blank line; just use echo with no arguments."

Suggested fix: change echo "" to bare echo.

Rework the previous fix (hardcoded CI + Claude Code Review names) into
a default-include model: discover every workflow run GitHub actually
triggered for the pushed commit and require all of them to pass (or
legitimately skip), with an EXCLUDE_WORKFLOWS array as the escape hatch
for a workflow that should never gate this check. This way any future
workflow added to either repo is covered automatically, without having
to remember to update a hardcoded list here.

Also fixes a bug introduced while testing the rework: gh run list --jq
only accepts a plain jq expression, not extra jq flags like --argjson.
Passing --argjson straight to gh run list failed with 'unknown
command', which the surrounding `|| echo "[]"` fallback silently
swallowed - discovery always returned zero runs and looped for the
full timeout. Fixed by piping gh's raw --json output into a real jq
invocation instead.
@jnasbyupgrade
jnasbyupgrade merged commit 9ef28fd into Postgres-Extensions:master Jul 31, 2026
10 checks passed
@jnasbyupgrade
jnasbyupgrade deleted the fix/ci-monitor-wrong-run branch July 31, 2026 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant