Skip to content

fix(bin): require a real PR reference for the CI-ready done note #6234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
karotkriss wants to merge 1 commit into
base: main
Choose a base branch
from
+26 −2
Open

fix(bin): require a real PR reference for the CI-ready done note #6234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions bin/fm-dod-lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,11 +460,15 @@ fm_dod_ref_contains() { # <repo> <ref-namespace> <sha>
# 0 when a done: note reports the no-mistakes CI-ready PR (`PR <url> checks
# green`, with any surrounding text). bin/fm-crew-state.sh takes its CI-ready
# path on this same test, so every CI-ready line it acts on is gated.
# The note must carry a real PR reference - a word-bounded `PR #N`, a GitHub
# `/pull/N` URL, or a GitLab `/merge_requests/N` URL - so an uppercase "PR"
# inside a word such as PROD or PROPERTIES never counts.
fm_dod_note_reports_ci_ready() { # <note>
local re='(^|[^[:alnum:]_])PR #[0-9]+|/pull/[0-9]+|/merge_requests/[0-9]+'

@greptile-apps greptile-apps Bot Sep 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Bare paths count as PRs A note such as done: checks green; see /pull/12 passes this matcher even though /pull/12 is not a PR URL. If the run is still monitoring CI and the worker’s head is reachable outside its worktree, crew-state can report the task as done instead of working. Require the URL alternatives to match an actual PR URL, and add a negative test for bare paths.

case "$1" in
*PR*"checks green"*|*"checks green"*PR*) return 0 ;;
*"checks green"*) [[ $1 =~ $re ]] ;;
*) return 1 ;;
esac
return 1
}

# 0 when a done: note reports a change published to a Gerrit review server
Expand Down
20 changes: 20 additions & 0 deletions tests/fm-dod-lib.test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,25 @@ test_ci_ready_variants_are_gated() {
pass "no-mistakes CI-ready done: with extra text is gated"
}

test_ci_ready_needs_a_real_pr_reference() {
local note
for note in \
'PROD deploy watch, checks green on TEST' \
'PROPERTIES table migration, checks green' \
'checks green, PR pending'; do
! fm_dod_note_reports_ci_ready "$note" \
|| fail "note without a PR reference counted as CI-ready: $note"
done
for note in \
'PR #12 checks green' \
'PR https://github.com/o/r/pull/12 checks green' \
'PR https://gitlab.example.test/g/s/p/-/merge_requests/7 checks green'; do
fm_dod_note_reports_ci_ready "$note" \
|| fail "note with a PR reference was not CI-ready: $note"
done
pass "a CI-ready note needs checks green and a real PR reference"
}

test_keyed_and_spaced_done_lines_are_gated() {
local repo wt line mode rc
repo="$TMP_ROOT/keyed-repo"
Expand Down Expand Up @@ -393,6 +412,7 @@ test_merge_marker_binds_to_the_named_pr
test_forge_recorded_head_is_accepted_without_local_object
test_direct_pr_recorded_head_does_not_cover_unpushed_commit
test_ci_ready_variants_are_gated
test_ci_ready_needs_a_real_pr_reference
test_keyed_and_spaced_done_lines_are_gated
test_local_only_linked_branch_is_accepted
test_local_only_detached_head_is_refused
Expand Down
Loading