Address Holistic Review Workflow Feedback#131031
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 15 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
There was a problem hiding this comment.
Pull request overview
This PR refines the Holistic Review workflow guidance to more explicitly evaluate when additional complexity in a PR is justified by its goals, and updates the compiled lockfile accordingly.
Changes:
- Add explicit complexity-vs-maintainability tradeoff assessment guidance (including when to escalate to
⚠️ Needs Human Review) in the Holistic Review instructions. - Regenerate the
holistic-review.lock.ymlmetadata to reflect the updated workflow source.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.github/workflows/holistic-review.md |
Adds guidance instructing reviewers to explicitly assess whether added complexity is proportionate and well-justified, and when to escalate for maintainer judgment. |
.github/workflows/holistic-review.lock.yml |
Updates the generated lock metadata (hash) corresponding to the workflow text change. |
| Explicitly assess whether the PR's added complexity is necessary and proportionate to its validated | ||
| goal. Do not treat size, low-level code, or specialized algorithms as concerns by themselves when |
|
Is there consideration for having it not post a comment at all if it's deemed not actionable by the LLM itself? Or would you still want it to always post, but perhaps with less verbosity? E.g. in #130802 (review) there's a whole page of text for what boils down to |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
82c5aba to
029bbef
Compare
| # Reusable workflows retain the worker's event context. Bind state to that | ||
| # context rather than trusting the agent-controlled callback payload. | ||
| PR_NUMBER: ${{ github.event.inputs.pr_number }} | ||
| HEAD_SHA: ${{ github.event.inputs.pr_head_sha }} | ||
| OUTCOME: ${{ inputs.outcome }} |
| state_issue_comment_prefix='<!-- holistic-review-orchestrator-state:pr-' | ||
| state_issue_comment_pattern='^<!-- holistic-review-orchestrator-state:pr-[1-9][0-9]* -->\n' | ||
|
|
||
| state_issue_number="$STATE_ISSUE_NUMBER" |
| pr_title="$(jq -er '.title' <<< "$entry")" | ||
| pr_title_html="$(html_escape "$pr_title")" | ||
| base_ref="$(jq -er '.base_ref' <<< "$entry")" |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.github/workflows/holistic-review-completed.yml:62
- The workflow_call inputs are defined as
pr_numberandhead_sha, but this job readsgithub.event.inputs.pr_head_sha, which doesn’t exist for this reusable workflow. That makesHEAD_SHAempty and causes orchestrator-reconcile-state.sh to fail the head SHA check, preventing state from being reconciled/upserted.
# Reusable workflows retain the worker's event context. Bind state to that
# context rather than trusting the agent-controlled callback payload.
PR_NUMBER: ${{ github.event.inputs.pr_number }}
HEAD_SHA: ${{ github.event.inputs.pr_head_sha }}
OUTCOME: ${{ inputs.outcome }}
This draft collects and addresses feedback from the recently enabled Holistic Review automation. Maintainers are encouraged to push refinements directly to this draft while reviewing its results.
Holistic Review feedback addressed
Other feedback addressed in merged PRs
Note
This PR description was generated by GitHub Copilot.