fix(#1957): align optional project number inputs - #7057
Conversation
Allow runs without a project board while retaining the existing RICE comment. Signed-off-by: Shai Revivo <srevivo@redhat.com> Co-authored-by: Codex <noreply@openai.com>
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoAlign optional project number contracts for prioritize workflows
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Document comment-only RICE runs and optional board updates. Record deprecated per-org compatibility context in the PR history. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
Site previewPreview: https://446ab2c4-site.fullsend-ai.workers.dev Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
[MEDIUM] PR description's added deprecation framing contradicts the repo's own workflow-contracts.md
In response to the qodo bot's review comment, the PR description was updated to add a "Deprecated installation-mode context" section claiming reusable-prioritize.yml "is retained for compatibility with the deprecated per-org installation chain" (ADR 0044) and that the change "does not add new per-org functionality; it keeps the remaining contract compatible while per-repo remains the supported installation model."
This is contradicted by docs/contributing/workflow-contracts.md itself (verified on the PR head at 9a18f12), which states an explicit, named exception: "Exception: prioritize.yml is installed per-repo as a thin caller that receives workflow_dispatch from the org-level scheduler and calls reusable-prioritize.yml directly, bypassing the dispatch shim." So reusable-prioritize.yml's contract is live for the supported per-repo installation path too, not merely a deprecated per-org compatibility shim as the (already-revised) PR description now claims.
This is a new inaccuracy introduced by the fix-round reply, not something the existing qodo/human comment thread caught or addressed — that thread only asked for deprecation context to be added, and got an added-but-incorrect explanation.
Suggestion: correct the PR description's deprecation section to note that reusable-prioritize.yml also serves the supported per-repo prioritize.yml thin-caller path per workflow-contracts.md's documented exception, so reviewers don't underweight the blast radius of this contract change to a mix of "deprecated-only" cleanup.
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 8:37 PM UTC · Completed 8:56 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.85 |
|
Risk Assessment: moderate (2/5) DetailsFocused bug fix aligning an optional workflow input across 3 files with tests present; CI workflow change and active multi-author hotspot files add some risk, but the narrow scope, clear issue linkage, and good test ratio keep this at moderate. |
ReviewFindingsMedium
Low
Next steps:
|
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
.github/workflows/reusable-prioritize.yml(file-level): Line 142 · [low] scope-gap
Issue #1957 authorized an early-exit guard for empty project_number. No such guard appears in any changed file. However, reusable-dispatch.yml already declared project_number as optional, meaning the agent/post-script already handles empty values via that path. The docs describe the conditional behavior with high specificity, suggesting the guard pre-exists.
Suggested fix: Confirm the early-exit guard exists in the post-script before merging.
internal/scaffold/workflow_call_alignment_test.go(file-level): Line 414 · [low] API shape patterns
assert.True(t, strings.Contains(s, ...)) is used for a substring containment check, inconsistent with the established assert.Contains(t, s, ...) idiom. The assert.True form produces less helpful failure output.
Suggested fix: Replace with: assert.Contains(t, s, "PRIORITIZE_PROJECT_NUMBER: ${{ inputs.project_number }}", "prioritize job should thread project_number to PRIORITIZE_PROJECT_NUMBER env var")
Summary
Make
project_numberoptional for prioritize workflows so comment-only RICEruns work when no GitHub Projects board is configured. Update the prioritize
agent documentation to explain that behavior.
Related Issue
Fixes #1957
Changes
project_numberin both reusable workflow contractsInstallation-mode context
reusable-prioritize.ymlserves both the supported per-repo installation path,where
prioritize.ymlis a thin caller, and the deprecated per-org installationchain described by ADR 0044.
This change updates the shared input contract for both paths; it does not add new
per-org functionality, and per-repo remains the supported installation model.
Testing
make lintpasses (changes staged first)go test ./internal/scaffold -run TestProjectNumberInputsAreOptional -count=1Checklist