Repository navigation
docs(Agents): clarify review and Windows workflows - #1548
Conversation
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1548.westeurope.azurestaticapps.net |
Paul-AUB
changed the title
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1548.westeurope.azurestaticapps.net |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1548.westeurope.azurestaticapps.net |
ClemRz
left a comment
ClemRz
left a comment
There was a problem hiding this comment.
Documentation-only change, and the direction is right: inline-anchored findings are genuinely better than a flat list, and fix-review no longer drops older inline threads. Two verifications worth recording:
- The
AGENTS.mdVitest guidance is correct.--run,--maxWorkers, and--testTimeoutare all valid on thevitest runsubcommand in the installed Vitest 4.1.10, and they forward throughyarn test->yarn workspace @grotto-front/web-app test. - Deferring commit types to
AGENTS.mdfixes a real divergence. The removed inline list containedci, whichcommitlint.config.jstype-enumrejects, and omittedtech,improvement, andrevert, which it allows.AGENTS.md:341matches the config.
Findings are inline. Nothing blocking; the --reviewer and APPROVE ones will fail in practice for some authors, so they are worth fixing before this becomes the documented path.
| ### 9. Submit the review with inline comments | ||
|
|
||
| ### Suggestions (Should Consider) | ||
| Build `pr_review_payload.json` with a JSON serializer so multi-line text is escaped correctly. Set `body` to the review body, `event` to `REQUEST_CHANGES` if any Must Fix finding exists or `APPROVE` otherwise, and `comments` to the code-specific findings. Each comment needs `path`, `line`, `side` (`RIGHT` for the new side or `LEFT` for the old side), and `body`. Add `start_line` and `start_side` for a multi-line range. Use one review submission so the body and inline comments are posted together: |
There was a problem hiding this comment.
Should Consider — two failure modes in this submission step.
APPROVEis rejected on your own PR. GitHub returns422 Can not approve your own pull request. Sincegithub-workflownow sets--assignee "@me"and the same authenticated account opens the PRs, self-review is a likely path, and it will fail with no review posted at all..kiro/steering/workflow.mdalso prescribesgh pr review <number> --commentfor this repo. Consider selecting the event based on authorship:
event = "COMMENT" if pr_author == $(gh api user --jq .login) else ("REQUEST_CHANGES" if must_fix else "APPROVE")
POST /pulls/{number}/reviewsis atomic. If a single entry incommentshas an anchor outside the diff, the whole request 422s and neither the inline comments nor the body are posted. Step 8 says to verify each anchor but gives no method. Consider naming the source of truth and a fallback here:
gh api repos/{owner}/{repo}/pulls/{number}/files --jq '.[] | "\(.filename)\n\(.patch)"'Anchors must fall inside a hunk's new-side range (context lines included). And if the submission still 422s, resubmit body-only rather than losing the review.
There was a problem hiding this comment.
Fixed both submission failure modes. Reviews on the author's own PR now use COMMENT instead of APPROVE/REQUEST_CHANGES. The workflow checks anchors against the PR files' diff hunks, and a 422 invalid-anchor response triggers a body-only retry containing every unposted finding rather than silently losing the review.
| - **Write all review content in English** — `pr_review_body.md` must be in English regardless of the conversation language. | ||
| - **NEVER** present the review as chat text. The review MUST be submitted to GitHub. | ||
| - **ALWAYS** use temporary files for multi-line CLI content (the `--body-file` pattern). Never pass review content inline. | ||
| - **ALWAYS** use temporary files for multi-line review text and JSON payloads. Never pass review content inline on the command line. | ||
| - **ALWAYS** clean up temporary files after submission, even if it failed. | ||
| - **Be thorough but respectful.** Critique the code, not the author. Use phrases like "Consider..." or "This might..." rather than "You should..." or "This is wrong." | ||
| - **Reference specific files and line numbers** whenever possible so the author can locate issues quickly. |
There was a problem hiding this comment.
Should Consider — these two rules did not follow step 8 through.
- Line 169 tells the reviewer to reference specific files and line numbers, which step 8 now explicitly forbids in the body ("not a list of file and line references in the review body"). As written, a reader can satisfy line 169 by doing exactly what step 8 rules out. Consider rewording to something like "anchor each finding to its diff line rather than naming the location in prose".
- Line 164 scopes the English rule to
pr_review_body.md, but inline comment bodies now carry most of the review content. Extend it to the inline comments, otherwise the rule no longer covers the majority of what gets posted.
There was a problem hiding this comment.
Fixed both wording inconsistencies: the English-language requirement explicitly includes inline comments, and the location guidance now says to anchor findings to diff lines instead of listing file/line references in the review body.
| ```bash | ||
| git push origin <branch-name> | ||
| gh pr create --title "<type(scope): description>" --body-file pr_body.md --base develop --repo <repo> | ||
| gh pr create --title "<type(scope): description>" --body-file pr_body.md --base develop --repo <repo> --reviewer ClemRz,urien --assignee "@me" |
There was a problem hiding this comment.
Should Consider — this fails whenever ClemRz or urien is the author.
GitHub rejects a review request from the PR author (422 Review cannot be requested from pull request author). gh pr create creates the PR first and requests reviewers in a follow-up call, so the outcome is the worst one: the PR exists, the command exits non-zero, and neither the reviewers nor the assignee are set. .kiro/steering/workflow.md has the agent authenticate as ClemRz by default, so this is not a rare case.
Both logins are confirmed collaborators, so the list itself is fine — it just needs the author removed:
me=$(gh api user --jq .login)
reviewers=$(printf '%s\n' ClemRz urien | grep -vx "$me" | paste -sd, -)
gh pr create ... ${reviewers:+--reviewer "$reviewers"} --assignee "@me"There was a problem hiding this comment.
Fixed: the PR creation example now gets the authenticated login and excludes it from ClemRz/urien case-insensitively before passing --reviewer; guidance for existing PRs likewise excludes the PR author.
| - On Windows, credentials may be hidden inside the sandbox. Run authenticated | ||
| `gh` commands and Git network operations with escalated permissions. If a | ||
| sandboxed auth check fails, retry with escalated permissions before asking. |
There was a problem hiding this comment.
Should Consider — the second and third sentences give opposite instructions.
"Run authenticated gh commands and Git network operations with escalated permissions" is unconditional: always escalate. The next sentence says escalate only after a sandboxed check fails. An agent reading this will take the first one and escalate every gh and git network call on Windows, which drops the sandbox default for the common case where it was working fine.
Consider keeping only the failure-triggered form: run sandboxed first, and escalate the specific command that failed with an auth or credential error.
There was a problem hiding this comment.
Fixed: the Windows guidance now runs commands sandboxed first and escalates only the specific gh or Git network command that failed with an authentication or credential error.
| ``` | ||
|
|
||
| Merge all entries by `created_at`/`submitted_at`, drop anything authored by your own login (from step 0). | ||
| Merge entries by `created_at`/`submitted_at`. Group inline comments with their replies using `in_reply_to_id`, and check whether each thread is resolved (GitHub GraphQL exposes `reviewThreads.isResolved`). Keep your own replies as context, but do not treat them as reviewer asks. Paginate all sources; a newer summary does not replace inline threads. |
There was a problem hiding this comment.
Should Consider — the resolved check has no command, unlike every other source in this step.
Step 3 depends on it ("Skip resolved, superseded, or already addressed asks"), but reviewThreads.isResolved is not reachable from the two REST calls above, and the parenthetical is the only pointer. Consider giving the query outright so the step is executable:
gh api graphql -f query='
query($owner:String!,$repo:String!,$pr:Int!){
repository(owner:$owner,name:$repo){
pullRequest(number:$pr){
reviewThreads(first:100){
nodes{ isResolved isOutdated comments(first:1){ nodes{ databaseId } } }
}
}
}
}' -F owner=... -F repo=... -F pr=...The first comment's databaseId is also the top-level comment ID that step 7 needs for /comments/{id}/replies, so one query covers both.
There was a problem hiding this comment.
Fixed: fix-review now includes an executable GraphQL query returning isResolved, isOutdated, and each thread's top-level databaseId, with explicit cursor pagination guidance.
| description: Given a PR number, address actionable reviewer feedback across review summaries, general comments, and inline threads; fix relevant asks, push, and reply in each thread or on the PR. | ||
| --- | ||
|
|
||
| You are resolving reviewer feedback on an existing pull request. `$ARGUMENTS` is the PR number — if missing, ask the user for it before doing anything else. |
There was a problem hiding this comment.
Optional — the argument-hint removal and this line are inconsistent.
argument-hint is slash-command frontmatter, so dropping it from a skill is reasonable. But $ARGUMENTS is the same slash-command-only mechanism, and this line still depends on it being interpolated. create-icon and translate also still carry argument-hint, so the repo now has both conventions.
Either it is unsupported here, in which case remove it from all four skills and reword this line to take the PR number from the user's request, or it is harmless metadata and the removal is churn. Right now the stated rationale only holds for half the files it applies to.
There was a problem hiding this comment.
Fixed the inconsistency by restoring argument-hint in code-review and fix-review. It is harmless skill metadata, and create-icon and translate already retain it alongside their $ARGUMENTS guidance, so no unrelated skill changes were needed.
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1548.westeurope.azurestaticapps.net |
|
Je ne pense pas pouvoir donner un avis |
ClemRz
left a comment
ClemRz
left a comment
There was a problem hiding this comment.
Re-reviewed against 10f24fe6. All six findings are genuinely addressed, verified in the head file rather than from the replies:
code-review:142—COMMENTon your own PR, with the reason stated. Correct.code-review:136,149— anchors now checked againstpulls/{number}/files, with the atomicity ofPOST /reviewsspelled out and a body-only retry. This goes further than I suggested; the "do not guess an anchor on a truncated patch" clause is a good addition.code-review:165,170— both rules now match step 8.github-workflow:23-26— sandboxed first, escalating only the command that failed. Resolved.fix-review:45-65— executable query, and it returns thedatabaseIdstep 7 needs. Cursor pagination guidance and theisOutdated!=isResolvedcaveat are both welcome.argument-hint— restoring it in both skills is a fine resolution, and it leaves all four skills consistent.
Two leftovers, one inline and one on the description:
The ## 🤔 What section still says "Remove unsupported argument-hint frontmatter from the two review skills", which 10f24fe6 reverted. Since this squash-merges, that line becomes the commit message. Worth replacing with the reasoning from the thread — that it is harmless skill metadata kept for consistency with create-icon and translate.
| me=$(gh api user --jq .login) | ||
| reviewers=$(printf '%s\n' ClemRz urien | grep -vix "$me" | paste -sd, -) | ||
| reviewer_args=() | ||
| if [ -n "$reviewers" ]; then reviewer_args=(--reviewer "$reviewers"); fi | ||
| gh pr create --title "<type(scope): description>" --body-file pr_body.md --base develop --repo <repo> "${reviewer_args[@]}" --assignee "@me" |
There was a problem hiding this comment.
Should Consider — the exclusion logic is correct, but the shape it took has two problems.
-
It is bash-only, in the skill that this PR is making Windows-aware.
me=$(...),reviewer_args=(), and"${reviewer_args[@]}"all fail in PowerShell, and step 4 just below gives explicit bash and PowerShell variants for a one-linerm. So the Windows path now has a documented cleanup command and an undocumented create command. -
It fails open when
gh api userfails. I checked the behaviour:
me="" -> grep -vix "" -> ClemRz,urien
An empty $me matches nothing with -x, so -v keeps both logins and the original 422 comes back — silently, at the step this snippet exists to protect.
The exclusion does not need shell logic at all: the agent already knows which account it authenticated as, and the prose at lines 82-89 already describes the rule. Consider keeping the command declarative and letting the agent apply it:
gh pr create --title "<type(scope): description>" --body-file pr_body.md --base develop --repo <repo> --assignee "@me"
gh pr edit <pr-number> --add-reviewer ClemRz # omit if ClemRz opened the PR
gh pr edit <pr-number> --add-reviewer urien # omit if urien opened the PRThat runs identically in both shells, keeps one failing reviewer from taking the PR creation down with it, and makes a failed gh api user an explicit stop rather than a silent fallback. Case-insensitive matching was the right call either way — gh pr create --reviewer clemrz is accepted, so comparing raw logins would have missed it.
There was a problem hiding this comment.
Fixed in a131a0c. The creation example now uses separate, shell-independent commands to create and assign the PR, then request each eligible reviewer. Reviewer logins are compared with the author case-insensitively, and author/pending-review requests are omitted. A failed or empty gh api user result explicitly stops the workflow after authentication recovery. A failed reviewer request is reported while the other eligible request and final verification can still proceed.
Validation: yarn.cmd lint, skill validation, and git diff --check passed.
|
Addressed the two remaining asks from the latest review:
I also re-read all seven unresolved inline threads and checked the current files. The six earlier findings are already addressed by 10f24fe, as confirmed in your latest review; no additional changes were needed for those points. Validation: |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1548.westeurope.azurestaticapps.net |
ClemRz
left a comment
ClemRz
left a comment
There was a problem hiding this comment.
Re-reviewed against a131a0c8. Both leftovers are resolved and I have nothing further to raise.
github-workflow:71-89 now does more than remove the bash dependency. The failed or empty gh api user case is explicitly fail-closed ("do not continue with an unknown author"), creation and reviewer requests are separated so one rejected reviewer cannot take the PR down with it, and the partial-failure behaviour is stated rather than left to the agent. The ## 🤔 What bullet and the ## 🔍 How paragraph both match the diff again.
One non-blocking observation, no action needed for this PR: lines 93-95 still say "Request reviews from ClemRz and urien, excluding the PR author, and assign the PR to the authenticated account", which is now fully covered by the new block at 71-82 and the commands at 84-89. The existing-PR guidance and the verification command in that paragraph are still the only place they appear, so it cannot just be deleted. Flagging only because the first round of this PR produced its contradiction between step 8 and the Important Rules from exactly this kind of duplication, so it may be worth trimming to the parts that are unique whenever this file is next touched.
All eight findings from the two earlier reviews are verified fixed in the head file. From my side this is ready; the remaining gate is the second approval.
|
Addressed the remaining documentation duplication in 28570be. Removed the repeated reviewer/assignee sentence after the creation commands. The existing-PR guidance, pending-review protection, verification command, and rejection reporting remain in place. Validation: yarn.cmd lint completed with exit code 0, skill validation passed, and git diff --check passed. Application tests were not run for this documentation-only change. |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1548.westeurope.azurestaticapps.net |
ClemRz
left a comment
ClemRz
left a comment
There was a problem hiding this comment.
Re-reviewed against 28570be7. The trim is exactly scoped: only the duplicated reviewer/assignee sentence is gone, and the existing-PR guidance, the already-requested protection, the verification command, and the rejection reporting all survive. Both logins remain discoverable from the commands at lines 87-88, so nothing was lost with the removed profile links.
Nothing outstanding from my side. All nine findings across the three reviews are verified fixed in the head file, and I am raising nothing new — this reads as done.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
28570be to
9aa6345
Compare
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1548.westeurope.azurestaticapps.net |
docs(Agents): clarify review and Windows workflows
🤔 What
yarn.cmd, sandboxEPERM, and Vitest timeout recovery steps inAGENTS.md.fix-reviewhandle all actionable inline review threads as well as review summaries and general comments, with replies in each relevant thread.code-reviewattach code-specific findings to diff lines or ranges, with a concise overall review body.argument-hintas harmless skill metadata in both review skills, consistent withcreate-iconandtranslate.🤷♂️ Why
Preparing PR #1547 exposed environment-specific failures and an embedded credential in a remote URL. The review skills also assumed a single summary comment was enough and placed code findings in a general PR review, making individual code discussions easy to miss. The PR workflow now records the expected reviewers and assignee.
🔍 How
Keep project-wide test guidance in
AGENTS.mdand GitHub-specific guidance in the skills.fix-reviewnow tracks every outstanding ask and answers inline comments in their original threads.code-reviewsubmits one GitHub review containing line-anchored comments and an overall summary.github-workflowidentifies the account before proceeding, creates and assigns the PR, then requests each eligible reviewer separately and verifies the result. Application code is unchanged.🔗 Related API PR
None.
🧪 Testing
yarn.cmd lint— completed with exit code 0.quick_validate.py .agents/skills/github-workflow— skill is valid.quick_validate.py .agents/skills/fix-review— skill is valid.quick_validate.py .agents/skills/code-review— skill is valid.git diff --check— no whitespace errors.Verified that PR docs(Agents): clarify review and Windows workflows #1548 requests reviews from ClemRz and urien and assigns Paul-AUB.
Application tests not run (documentation-only change).
📸 Previews
Not applicable to documentation changes.