Skip to content

fix: harden linked issue validation - #881

Open
nabinchha wants to merge 2 commits into
mainfrom
codex/fix-linked-issue-workflow
Open

fix: harden linked issue validation#881
nabinchha wants to merge 2 commits into
mainfrom
codex/fix-linked-issue-workflow

Conversation

@nabinchha

@nabinchha nabinchha commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • require linked issues to be open as well as triaged
  • re-run validation when the selected issue is closed, reopened, or gains or loses triaged
  • keep validation and triage-triggered rechecks aligned on the first closing reference
  • paginate bot-comment lookup so existing status comments are reliably updated or removed
  • clarify that the workflow is advisory unless repository rules require Linked Issue Check / check

Root cause

The workflow validated the triaged label without checking issue state, did not re-run when issue validity later changed, used different reference-selection rules between validation and retriggering, and only inspected the first page of PR comments. The contribution guidance also described the check as a required merge gate even though the current main protection does not require that status context.

These issues were identified while reviewing and fixing the corresponding workflow in NVIDIA-NeMo/DataDesignerPlugins#82.

Validation

  • parsed .github/workflows/pr-linked-issue.yml with yq
  • ran bash -n across every embedded workflow shell block
  • exercised first-reference and open/closed-state behavior with jq fixtures
  • git diff --check
  • make check-all
  • make test — 4,020 passed, 1 skipped
  • repository pre-commit hooks

Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
@nabinchha
nabinchha marked this pull request as ready for review August 19, 2026 16:19
@nabinchha
nabinchha requested a review from a team as a code owner August 19, 2026 16:19
@nabinchha
nabinchha deployed to agentic-ci August 19, 2026 16:19 — with GitHub Actions Active
@greptile-apps

greptile-apps Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR hardens linked-issue validation and resolves the previously reported stale-check behavior.

  • Requires external contributors’ linked issues to be open and triaged.
  • Revalidates PRs when the selected issue is labeled, unlabeled, closed, or reopened.
  • Aligns validation and retriggering on the first closing reference.
  • Paginates status-comment lookup and clarifies that merge blocking depends on repository rules.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
.github/workflows/pr-linked-issue.yml Adds issue-state validation, event-driven rechecks, aligned first-reference matching, and paginated bot-comment lookup; the prior stale-check issue is fixed.
CONTRIBUTING.md Accurately documents open-and-triaged issue validation and the workflow’s advisory status unless repository rules require it.

Reviews (2): Last reviewed commit: "Handle linked issue state changes" | Re-trigger Greptile

Comment on lines 239 to 243
github.repository_owner == 'NVIDIA-NeMo'
&& github.event_name == 'issues'
&& github.event.label.name == 'triaged'
&& github.event.issue.state == 'open'
runs-on: ubuntu-latest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Issue state changes leave stale checks

When a linked issue is closed or reopened after its PR check completes, neither transition triggers revalidation, leaving a successful check green for a closed issue or a prior failure red for a reopened issue until an unrelated PR event occurs.

Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/pr-linked-issue.yml
Line: 239-243

Comment:
**Issue state changes leave stale checks**

When a linked issue is closed or reopened after its PR check completes, neither transition triggers revalidation, leaving a successful check green for a closed issue or a prior failure red for a reopened issue until an unrelated PR event occurs.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@github-actions

Copy link
Copy Markdown
Contributor

Code Review: PR #881fix: harden linked issue validation

Author: nabinchha (Nabin Mulepati) · Base: main · Head: codex/fix-linked-issue-workflow
Changes: 2 files, +38 / −9 (.github/workflows/pr-linked-issue.yml, CONTRIBUTING.md)

Summary

This PR hardens the external-contributor "linked issue" gate workflow. It makes four related fixes:

  1. Requires the linked issue to be open, not just triaged. A new is_open output is threaded through the validate → comment steps, with a dedicated "issue is closed" failure message.
  2. Aligns reference selection between the PR-validation path (grep ... | head -1) and the triage-retrigger path (previously a test(...) regex that could match any embedded number; now a capture(...) that extracts the first fixes|closes|resolves #N and compares it for exact equality).
  3. Paginates the bot-comment lookup (gh api --paginate ...?per_page=100) so status comments beyond the first page of PR comments are reliably found, then updated or deleted.
  4. Corrects CONTRIBUTING.md to describe the check as advisory ("should", "reports whether…") unless repo rules separately require the Linked Issue Check / check status context.

The changes are limited to workflow logic and docs; no product code is touched.

Findings

Correctness — looks sound

  • Alignment claim holds. The validation path parses with grep -ioP '(?:fixes|closes|resolves)\s+#\K\d+' | head -1 (first match). The retrigger path now uses capture("(?:fixes|closes|resolves)\s+#(?<issue>[0-9]+)") — jq's capture returns the first match in the string, and earliest-position == earliest-line for normal bodies, so both paths select the same "first closing reference." The switch from the old test(...\b...) to exact $reference.issue == $issue equality is a genuine improvement: it avoids partial/substring matches (e.g. #123 no longer matching a triage of #12).
  • capture(...)? error handling is correct. When the body has no closing reference, capture errors, the trailing ? suppresses it to empty, and the ... as $reference | ... binding then yields nothing for that element — so non-referencing PRs are simply filtered out. Good.
  • Elif ordering is correct. ISSUE_EXISTS != true is checked before ISSUE_OPEN != true, so a not-found issue reports "not found" rather than falling through to "closed" (the validate step sets both is_open=false and is_triaged=false on the not-found/is-a-PR early exits, which is the right defensive default).
  • Pagination fix is correct. With --paginate + a per-element --jq emitting .id, gh concatenates matching ids across all pages newline-separated; tail -1 reproduces the prior | last semantics (newest matching comment). Empty result → COMMENT_ID=""[ -n "$COMMENT_ID" ] false, preserving the create-new-comment path.
  • retrigger gated on github.event.issue.state == 'open' — consistent with the new open-issue requirement; no point re-triggering for a closed issue that would fail validation anyway.

Security — no concerns

  • PR_BODY is passed via env and written with printf '%s' to a file before parsing, avoiding shell interpolation of untrusted input. ISSUE_NUM is constrained to \d+ by the parse regex, so it is safe to interpolate into the gh api URL. Comment bodies are passed via -f body="$(cat ...)". No injection surface introduced.

Minor / non-blocking

  • Multi-issue PRs track only the first reference (by design). A PR body with Fixes #10, Closes #20 is validated against #10 and will only be re-triggered when #10 is triaged, not #20. Both paths agree, so this is internally consistent — but it is a real limitation. If not already documented, a one-line note in the workflow comment (or CONTRIBUTING) that only the first closing keyword is considered would save contributor confusion.
  • Regex duplicated in two languages. The (?:fixes|closes|resolves)\s+#N pattern now lives as a PCRE grep (parse step) and an oniguruma jq capture (retrigger step). They are equivalent today, but future edits to one can silently drift from the other. Not worth refactoring in a workflow file, but worth a # keep in sync with the parse step comment near the jq block.
  • CONTRIBUTING.md wording is now accurate. Downgrading "must … enforced by a required status check" to "should … repository rules must separately require Linked Issue Check / check" correctly reflects that the workflow only fails a non-required context. This is an honest correction, not a weakening of policy.

Testing

  • No automated tests accompany the change, which is acceptable — this is a GitHub Actions workflow with embedded shell, not importable code, and the repo's make test suite does not cover it. The PR body documents manual validation (yq parse, bash -n on shell blocks, jq fixtures for first-reference and open/closed behavior, git diff --check, make check-all, full make test). Given the logic paths (open/closed/not-found/not-triaged/no-reference, pagination), the described jq-fixture exercising is the appropriate level of verification here.

Verdict

Approve (advisory). A clean, well-scoped hardening PR. Each of the four fixes addresses a real defect (missing open-state check, misaligned reference selection, single-page comment lookup, inaccurate docs), the implementations are correct, and there is no new security surface. The only follow-ups are optional documentation/comment nits around the first-reference-only behavior and the duplicated regex. No changes required before merge.

(Note: per review constraints, this review does not approve or request changes on GitHub — it is advisory only.)

Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
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