Repository navigation
Conversation
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
==========================================
+ Coverage 91.40% 91.67% +0.27%
==========================================
Files 216 223 +7
Lines 26738 27505 +767
Branches 4376 4525 +149
==========================================
+ Hits 24439 25216 +777
+ Misses 1635 1620 -15
- Partials 664 669 +5 🚀 New features to boost your workflow:
|
…workflow_run poster Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
@exai-bot /review |
exai-bot
left a comment
exai-bot
left a comment
There was a problem hiding this comment.
Note
AI-generated review of f780f1827ae4, requested by @vchamarthi, using the default review skill. It can be wrong: verify a finding before acting on it. React with a thumbs up or down on a finding to tell the bot whether it helped.
Model us.anthropic.claude-sonnet-5-5, effort default, 4 of 96 turns, 0.8 of 48 minutes, 72k in / 5k out tokens.
This adds a workflow_run job that reads the cli-interface-result artifact and applies the cli-interface-change label and sticky comment with a write token. The trust boundary is well built: no checkout, size caps, strict type checks, head-SHA and base-repo checks, and unzip called with argv only. The tests run the embedded script under node against hostile inputs, which is a good approach. I found only small robustness gaps, and none of them is a security hole.
4 findings are on the changed lines, as comments below.
For a human reviewer
- The workflow depends on #88's artifact
result.jsonhavingpr_number,head_sha,changedanddiff, and onhead_shabeing the PR head rather than the merge commit. That workflow is not in this tree, so I could not check it. - Whether
unzipis installed on theintel-ubuntu-latestrunner image. The job fails closed if it is missing.
Not verified
I could not run the tests or the workflow. I did not confirm that intel-ubuntu-latest has unzip. I did not confirm that #88 sets head_sha to the PR head commit; the cli-interface workflow is not on main.
| if (!['0', '1', 'error'].includes(res.changed)) return bad('changed is not one of 0, 1, error'); | ||
| if (typeof res.diff !== 'string' || res.diff.length > MAX_DIFF_CHARS) return bad('diff is missing or too long'); | ||
|
|
||
| // The number is only a claim of the PR's code: check it is a PR of this |
There was a problem hiding this comment.
Minor (confirmed, correctness): Mention neutralisation can push the comment body past GitHub's 65,536-character limit
- The diff is capped at 60,000 characters before sanitising, but
.replace(/@/g, '@\u200b')adds a character per@. - A diff with about 6,000 or more
@characters, for example from a PR that adds many decorators, can exceed 65,536 once the marker and footer are added. createCommentorupdateCommentthen returns 422 and the job fails with no comment posted.- A hostile PR can trigger this deliberately. The failure is noisy but harmless.
Suggested fix:
Sanitise first, then truncate the result to a safe length (about 60,000 characters) with a "truncated" note. Or apply the cap after the replacement.
There was a problem hiding this comment.
Fixed in a176a67: the 60,000-character cap is now applied after the mention rewrite (and the text is truncated with a note), so the body cannot pass 65,536. Test: test_many_mentions_cannot_push_the_body_past_githubs_limit (fails on the old code).
| if (!['0', '1', 'error'].includes(res.changed)) return bad('changed is not one of 0, 1, error'); | ||
| if (typeof res.diff !== 'string' || res.diff.length > MAX_DIFF_CHARS) return bad('diff is missing or too long'); | ||
|
|
||
| // The number is only a claim of the PR's code: check it is a PR of this |
There was a problem hiding this comment.
Minor (plausible, security): Only @ is neutralised; links, images and #N references from PR-controlled text are posted as github-actions[bot]
- The diff text is PR-controlled and is posted unfenced.
- A fork author can make the bot comment contain clickable phishing links, tracking images, or
#123ororg/repo#Ncross-reference spam. - The comment appears under the bot identity on the maintainers' PR. The author can only write to their own PR, so impact is limited, but the description frames the sanitising as complete.
Suggested fix:
Render the diff inside a fenced code block, choosing a fence longer than any backtick run in the text. Or escape [, < and # as well.
There was a problem hiding this comment.
Fixed in a176a67: the diff is now posted inside a code fence longer than any backtick run in it, so links, images, HTML and #N references do not render; the @ and marker neutralising stays. Test: test_diff_is_fenced_so_links_and_references_do_not_render (fails on the old code).
|
|
||
| permissions: {} | ||
|
|
||
| concurrency: |
There was a problem hiding this comment.
Minor (plausible, correctness): Concurrency is keyed by head SHA, so two runs for the same PR can race and create duplicate sticky comments
- Runs for different pushes to one PR use different groups and can execute in parallel.
- Both can list comments, find no existing marker, and each call
createComment. - Later runs then update only the first match, so a stale duplicate is left behind.
- The same race exists between this job and ci: report user-facing CLI surface changes on every PR #88's same-repo job, which the description acknowledges. The stale-head check narrows the window but does not remove it, because it runs before the comment lookup and write.
Suggested fix:
Key the group on the PR. workflow_run.pull_requests[0].number is empty for forks, so this needs a different key, for example head_repository.full_name plus head_branch. Alternatively, accept the race and have the job delete duplicate marker comments.
There was a problem hiding this comment.
Fixed in a176a67: the group is now workflow + head_repository.full_name + head_branch (pull_requests[] is empty for forks), so pushes to one PR post one at a time; the stale-head check covers an older run that runs later. The race with #88's same-repo job remains (acknowledged in the description). Test: test_concurrency_is_per_pr_branch_not_per_commit.
| assert "createComment" not in _names(calls) | ||
|
|
||
|
|
||
| def test_error_result_comments_and_leaves_the_label(tmp_path): |
There was a problem hiding this comment.
Nit (confirmed, tests): The 'unchanged' path with an existing sticky comment is untested
test_unchanged_result_removes_the_label covers only the case with no existing comment. The branch that rewrites an existing comment to "The CLI surface now matches the base branch" has no test. Neither does the tolerated failure of removeLabel/createLabel. Both are cheap to add with the existing harness.
There was a problem hiding this comment.
Added in a176a67: test_unchanged_result_rewrites_an_existing_comment_even_if_removing_the_label_fails covers the existing-comment rewrite and a throwing removeLabel.
… PR branch Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
@exai-bot /re-review |
exai-bot
left a comment
exai-bot
left a comment
There was a problem hiding this comment.
Note
AI-generated review of a176a67e2c1c, requested by @vchamarthi, using the default review skill. It can be wrong: verify a finding before acting on it. React with a thumbs up or down on a finding to tell the bot whether it helped.
Model us.anthropic.claude-sonnet-5-5, effort default, 3 of 93 turns, 0.6 of 48 minutes, 44k in / 5k out tokens.
Re-review of a176a67e2c1c: 1 new finding.
1 finding is on the changed lines, as comments below.
|
@exai-bot @vchamarthi on the "For a human reviewer" points of the first review: (1) #88's cli-interface.yml (head e4ef0b0) writes result.json with pr_number, head_sha, changed and diff, and head_sha is |
…ot one marker pass Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The fence was one longer than the longest backtick run, so a diff of backticks under the 60,000-character cap produced a body of up to 180,000 characters, which GitHub rejects after the label is applied. Split runs of 8+ backticks before the cap so the fence is at most 8. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Only the bot-login check stops the write-token job from editing a comment another user posted with the marker; removing it left every test green. Cover all three results. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 0043665. The script holds up: I could not break the fence, the 65,536 limit, the marker or the mention rewrite, and the validation of the artifact is right. Two things are worth fixing before merge.
What I ran, on a Linux review host at 0043665:
make ci-local: 25 passed, 1 failed, 6 skipped; 6,709 tests. The one failure ispip-auditon virtualenv 21.14.7 (CVE-2026-113722) from the existing locks, fixed onmainby #152; it is not caused by the PR.- The new test file: 34 passed. Reproductions for the findings, run under node with a fake GitHub client.
- A mutation run on the workflow: 6 mutants applied, all 6 pass the full test file (see the inline comment).
Checked, no issues
- Body size and fence: about 150 random and edge inputs (all
@, all<!--, all backticks, runs of 7, 8, 9 and 20 backticks, CRLF, empty diff, nested marker, a cut inside an emoji). The largest body was 60,159 characters, with one marker and one fence pair every time. - Validation:
pr_number,head_sha,changed,difftype and length, base-repo and stale-head checks each reject their bad input. - Lint:
zizmoron the workflow reports no findings; the action is SHA-pinned and permissions are minimal.
@xaleryb's question on the artifact format (the diff --markdown heading and inner fence land as literal code on fork PRs) waits for the author.
|
On the artifact format (xaleryb): confirmed. #88's |
… the producer's diff once An over-cap diff is truncated instead of dropped, an artifact or result.json over the size caps replaces the sticky comment with a too-large note (label untouched), the producer's heading and fence are unwrapped so the poster writes them once, and the error text is the job's own. Tests pin the guards a mutation pass found unpinned. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 52b0518. Both earlier points are fixed in code and tests: an over-cap diff is truncated, an over-cap artifact or result.json rewrites the sticky comment, and the mutants from the last round now fail. Three things are worth fixing before merge, all in the new code's tests: the mutants below pass the full 46-test file.
What I ran, on a Linux review host at 52b0518:
make ci-local: 25 passed, 1 failed, 6 skipped. The one failure ispip-auditon virtualenv 21.14.7 from the existing locks; the PR does not touch dependencies.- The new test file: 46 passed. Reproductions run under node with a fake GitHub client.
- A mutation run on the workflow: 71 mutants, 25 pass the full test file (6 of those are equivalent). The ones that matter are in the inline comments.
Checked, no issues
- Body size:
@, backtick and emoji-cut inputs, 60k-line inputs and the unwrap regex on 60k+ characters all gave a body of at most 60,270 characters, in under 0.4 s (no backtracking). - Wrapper smuggling: a spoofed heading, a link or an inner fence after an unwrapped diff stays inside the longer fence.
result.jsoncap: 204,740 bytes is read; 204,810 bytes takes the too-large path throughENOBUFS.- Lint:
zizmorreports no findings (one justified ignore);ruff checkpasses; action is SHA-pinned, permissions minimal.
@xaleryb's point on the artifact format is fixed in the poster by the unwrap; whether the regex matches the producer's real bytes cannot be checked from this tree.
| const prs = await github.paginate(github.rest.repos.listPullRequestsAssociatedWithCommit, | ||
| { owner, repo, commit_sha: run.head_sha }); | ||
| for (const p of prs) { | ||
| if (p.state !== 'open' || p.base?.repo?.full_name !== `${owner}/${repo}` || | ||
| p.head?.sha !== run.head_sha) continue; |
There was a problem hiding this comment.
The tooLarge lookup is not pinned: the commit it queries and the loop over several PRs can both break with every test green.
The fake paginate returns the configured PR list whatever arguments it gets, and every test passes a single PR. Mutation check on the workflow alone, full test file run:
commit_sha: run.head_shachanged tocommit_sha: 'x': 46 passed.continueat line 78 changed tobreak: 46 passed. With a closed or fork PR listed before the valid open one, the valid PR keeps its stale diff comment.
By reading the code, an empty lookup result also ends with only a warning and a green job, so the stale comment stays. No test covers that.
Suggestion: assert commit_sha in the recorded call, and add a case where the first associated PR is filtered out (closed, wrong head, other repo) and the second is valid, and one with an empty list.
| // ```diff fence. Fenced again, those would show as literal code: take the diff | ||
| // lines out of it and write the heading and the one fence here, as that job does. | ||
| const wrapped = res.diff.match(/^## CLI interface change detected\n\n[^\n]*\n\n```diff\n([\s\S]*)\n```\n?$/); | ||
| let text = (wrapped ? wrapped[1] : res.diff).replace(/<!--/g, '<!\u200b--').replace(/@/g, '@\u200b') |
There was a problem hiding this comment.
The mention and comment-opener rewrites can be weakened and every test still passes.
Mutation check on this line, full test file run:
.replace(/@/g, ...)changed to.replace(/@/, ...): 46 passed. Only the first@is broken, so every later@userin a PR-controlled diff would ping.'<!\u200b--'changed to'<!--\u200b': 46 passed. The text is still a comment opener.{8,}changed to{9,}on line 140: 46 passed. An 8-backtick run is not split and the fence grows to 9.
The 60,000-@ test only checks the body length, and the mention test uses one @.
Suggestion: assert on a diff with several mentions (@a @b @c) that no @ is followed by a word character, that the body contains no <!-- other than the marker, and that an 8-backtick run gives a fence of at most 8.
| // The producer's `diff --markdown` output carries its own heading, intro and | ||
| // ```diff fence. Fenced again, those would show as literal code: take the diff | ||
| // lines out of it and write the heading and the one fence here, as that job does. | ||
| const wrapped = res.diff.match(/^## CLI interface change detected\n\n[^\n]*\n\n```diff\n([\s\S]*)\n```\n?$/); |
There was a problem hiding this comment.
The producer-wrapper regex is tested with one hand-written input, and it falls back to double fencing on any variation.
Mutation check, full test file run: dropping the leading ^, dropping the trailing $, making ([\s\S]*) lazy, loosening the intro line [^\n]*, and making the trailing \n required all give 46 passed. Only removing the unwrap altogether fails.
The regex also accepts only \n endings and one optional trailing newline. In a run with the fake client, a wrapper with CRLF, or one ending in a blank line after the closing fence, is not unwrapped: the body shows the producer's ## CLI interface change detected heading and its diff fence as literal code under the job's own heading, with no warning.
Suggestion: add cases for a diff that merely contains the heading text in the middle, a CRLF wrapper, a wrapper with a trailing blank line and one with no trailing newline, and decide which of them the unwrap should accept (for example \r?\n and \s*$).
The commit-to-PR lookup on the base repository returns no PR for a fork's head commit, so on fork PRs an over-cap result still left the stale comment. List the open PRs from the run's head branch instead. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at c11ab76. The new fork lookup is right: pulls.list with head=<owner>:<branch> replaces the commit lookup, and the new test pins the query. The three test gaps from my last review are still open, and each one lets a real regression through.
What I ran, on a Linux review host at c11ab76:
make ci-local: 25 passed, 1 failed, 6 skipped; 6,722 tests. The one failure ispip-auditon the existing lock files, which this PR does not touch.- The new test file: 47 passed. Mutants on the workflow alone, full test file run: the ones listed below all pass.
- A mutant that changes the new
headfilter to<repo owner>:<branch>fails the new test, so the query is pinned.
Still open from my earlier review:
.github/workflows/cli-interface-comment.yml:77:continuechanged tobreakstill gives 47 passed. No test lists two PRs with the first one filtered out, and none returns an empty list. (commit_shais gone with the commit lookup; theheadandstatearguments are now asserted.).github/workflows/cli-interface-comment.yml:140:.replace(/@/g, ...)changed to.replace(/@/, ...),'<!\u200b--'changed to'<!--\u200b', and{8,}changed to{9,}(line 141) all still give 47 passed..github/workflows/cli-interface-comment.yml:139: dropping^, dropping$, a lazy([\s\S]*), a loosened[^\n]*and a required trailing\nall still give 47 passed. A CRLF wrapper and a wrapper with a blank line after the closing fence are still double-fenced, and no test covers either.
Checked, no issues
- Fork lookup:
headas<owner>:<branch>is the documented filter form; the head-sha, state and base-repo filters stay client-side. Mutating theheadtemplate,stateor the sha check each fails a test. - Unwrap: the LF wrapper with and without a trailing newline gives one heading and one
difffence; a diff that merely contains the heading mid-way is not unwrapped. - Lint:
ruff,mypy,bandit,deptry,gitleaksand the docs build pass inci-local.
Summary
Fork PRs get a read-only token, so #88's
cli-interfaceworkflow can only write a job summary there. This addscli-interface-comment.yml: aworkflow_runjob that runs in the base repo's context with a write token and applies thecli-interface-changelabel and the sticky comment from the resultcli-interfaceuploads.Not stacked: this branch is based on
mainand touches only new files. The artifact upload it consumes is one extra commit on #88's branch (e4ef0b0), where the workflow lives. Until #88 merges, this workflow never fires (nocli-interfacerun to trigger on).Trust boundary: no checkout, no PR code executed. The artifact is untrusted data: size-capped (zip 256 KiB, JSON 200 KiB), parsed as JSON only,
pr_numbermust be a positive integer,head_shamust equal the run's head, the PR must belong to this repo and still have that head (stale results are skipped), the artifact must belong to the triggering run. Diff text goes only into an API body, with@mentions neutralised and the marker stripped. Permissions:actions: read,pull-requests: write,issues: write; actions SHA-pinned.Evidence
dokimi/tests/test_cli_interface_comment_workflow.py: 25 pass. Static checks (trigger, no checkout, permissions, pins) plus the embedded script run under node against a fake GitHub client with good and hostile artifacts (bad pr_number types, wrong sha, badchanged, oversized diff/artifact, other run's artifact, other repo, stale head, forged marker, mentions).test_cli_surface.py63 pass.workflow_runonly runs from the default branch, so the end-to-end run can happen only after both PRs merge.Merge Danger
workflow_runworkflows are read frommain, so later edits to this file by a PR take effect only after merge.github-actions[bot], as in ci: report user-facing CLI surface changes on every PR #88; once both are live, a same-repo PR gets the comment from both jobs (same marker, so one comment, last writer wins).🤖 Generated with Claude Code