Repository navigation
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #261 +/- ##
==========================================
+ Coverage 93.59% 93.65% +0.05%
==========================================
Files 277 277
Lines 35878 35942 +64
Branches 6085 6091 +6
==========================================
+ Hits 33580 33661 +81
+ Misses 1484 1469 -15
+ Partials 814 812 -2 🚀 New features to boost your workflow:
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
The push-scoped decision is sound, and every later step is still gated on the output. One thing is worth fixing before merge: a docs-only push can cancel the smoke run of the code push before it and then skip it (inline).
What I ran, on a Linux review host at d01b50a:
make ci-local: 27 passed, 1 failed, 7 skipped; 9,298 tests passed. The one FAIL ispip-audit, which lists filelock advisories in the tool lock files; this PR touches onlyagent-quality.yml. The Docker smoke job is skipped locally.- zizmor 1.31.1 on the file: no findings, same as at the base.
- I did not execute the
changesscript itself; the shell behaviour below comes from reading it.
Waits for the author: the commit has no Signed-off-by line, and ci-local does not run the DCO check. @napetrov needs to sign off d01b50a (git commit -s --amend) before merge.
Checked, no issues
- Gating: every step after
changescarriesif: steps.changes.outputs.code == 'true'; the upload step keepsalways() &&with it.benchmarkhas noneedson this job. - Injection:
EVENT,ACTION,BASE_REF,BEFORE,AFTERandDOCSall reach the script throughenv; nothing is interpolated intorun:. - Fail-safe paths: an empty or missing
before/after, an unfetched commit, a force push and an empty diff all end incode=true. Underbash -e, theif !and|| {...}forms do not abort the script.
|
🔍 OpenCodeReview found 1 issue(s) in this PR. By severity: 1 medium
Review details
|
| && git merge-base --is-ancestor "$BEFORE" "$AFTER"; } 2>/dev/null; then | ||
| set_code true "force push, or the commits are not fetched"; exit 0 | ||
| fi | ||
| scope=push |
There was a problem hiding this comment.
Must-fix: per-push scoping combined with cancel-in-progress: true can leave code untested. Push A (code) starts the smoke test. Push B (docs only) cancels A's run (same PR concurrency group). B's run sees only .md changes in A..B, skips every step and reports success. The PR head then has a green External catalog smoke test although the code change from A was never verified. The job doesn't run again unless something else triggers it. Decide the scope from the whole PR (base...HEAD) for every PR event. Alternatively, keep the per-push scope but set cancel-in-progress to false for PRs. Either way, a docs-only push must not skip when an earlier push's code run was cancelled or failed.
There was a problem hiding this comment.
Waiting on a decision before I change this: how to stop a docs-only push from masking the smoke run of an earlier code push. Context: with scope=push and cancel-in-progress: true for pull_request, docs push B cancels code run A and then skips every code step, so the head shows green with the code never verified (and a red A is replaced by a green B). Options: (1) decide from the whole PR (base...HEAD) on every PR event: closes it, but a docs push after a code push runs the smoke test again, which is what this PR set out to avoid. (2) cancel-in-progress false for PRs: B no longer cancels A, but a skipped green B still replaces a red A on the head. (3) keep the push scope, but skip only when everything since the last commit whose smoke run passed its code steps (Verify pass rate succeeded, read with the Actions API, actions: read) is docs-only; a cancelled or failed A then forces B to run. My recommendation: (3), because it is the only one that closes both cases and keeps the savings; it costs a few API reads and a longer changes step. The thread stays open until then. (Same finding as the other thread on this line; one decision answers both.)
The changes step diffed the whole PR (base...HEAD), so one code commit anywhere in a PR kept every later docs-only push running the Docker smoke test. A new push (synchronize) now decides from the push itself (before..after). Every other PR run still decides from the whole PR, and a push that cannot be diffed (force push, missing commits, empty diff) runs everything. Renames are diffed with --no-renames so a code file renamed to Markdown still counts as code. Documentation is Markdown in the root guides, dokimi/*.md, dokimi/docs/, dokimi/changelog.d/ and .github/. dokimi/data/ (read at run time), the tests and .agent-eval/ count as code. Thread: PRR_kwDOVBg7g88AAAABU5mMWA.b Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
d01b50a to
0b6686d
Compare
|
(a) Waiting on a decision before I change this: the cancel-and-skip finding is held, see the ask on PRRT_kwDOVBg7g86xi50L; the three options and my recommendation are there. (b) Sign-off: Fixed in 54b6efb: the PR commit now carries |
|
This review has no claim of its own (only the OpenCodeReview run marker); its one finding is the inline thread PRRT_kwDOVBg7g86xlzGd, answered there. |
|
Nothing to change here: the report says all modified lines are covered; installing the Codecov app is an organization setting. |
|
The one issue counted here is the inline thread PRRT_kwDOVBg7g86xlzGd (cancel then skip); it is held for a decision there. No separate change. |
| set_code() { echo "code=$1" >> "$GITHUB_OUTPUT"; echo "changes: code=$1 ($2)"; } | ||
| if [ "$EVENT" != pull_request ]; then set_code true "not a pull request"; exit 0; fi | ||
| # --no-renames: a rename from code to Markdown must count as the code it removes. | ||
| if [ "$ACTION" = synchronize ]; then |
There was a problem hiding this comment.
Must-fix: per-push scoping can leave code unchecked. Push A changes code and starts the smoke test. Push B, docs-only, arrives. The concurrency group has cancel-in-progress for PRs, so it cancels A's run. B's run sees only docs, skips every step and reports success. The head commit is green and the code from A never went through the Docker smoke test. If A's run had already failed, B's green run hides that failure in the same way. Fix: decide from the whole PR (base...HEAD) for every PR event, which is the old behaviour. If you want the per-push skip, apply it only when the previous run for BEFORE succeeded.
There was a problem hiding this comment.
Fixed in 6c1ed98d: option 3. A docs-only push now skips the code steps only when everything since the newest commit whose smoke run passed its code steps is documentation.
What changed in .github/workflows/agent-quality.yml:
- The
changesstep diffs from that passed commit, not frombefore. It finds the commit through the Actions API (actions: readon the smoke job): the newest completed run of this branch whoseVerify pass ratestep succeeded, reachable from the push. - A run that was cancelled by the next push, or that failed, does not count, so the code it carried runs again.
- With no passed run, or an unreadable run list, everything runs.
Pinned by dokimi/tests/test_agent_quality_changes_scope.py. It runs the step as the workflow has it, with gh stubbed. Before the change, the cancelled-run, failed-run, no-passed-run and unreadable-API cases returned code=false. After it, all six scope cases and the permission check pass, including docs after a passed run (still skips).
The cancellation itself is GitHub's concurrency behaviour; I have not run this on Actions.
The changes step decided a synchronize run from before..after alone. A docs-only push B cancels an in-flight code run A (same PR concurrency group, cancel-in-progress), so A's smoke test never finished and B skipped every code step: the head went green with A's code unverified, and a red A was replaced by a green B. Diff from the newest commit whose smoke run passed its code steps instead (Verify pass rate succeeded, read with the Actions API; the smoke job gets actions: read). A cancelled or failed run does not count, so its code runs again. With no passed run, or an unreadable run list, everything runs. tests/test_agent_quality_changes_scope.py runs the changes step as the workflow has it, with gh stubbed. Red before this change: the cancelled-run, failed-run, no-passed-run and unreadable-API cases return code=false, and the permission check fails. Green after: all six scope cases and the permission check pass. Review thread on PR 261: .github/workflows/agent-quality.yml:94. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
🔍 OpenCodeReview found 2 issue(s) in this PR. By severity: 1 medium, 1 low
Review details
|
| # steps below (the Docker smoke test cannot be affected by it); the benchmark | ||
| # job measures the docs, so it always runs. The job itself still takes a | ||
| # runner, as its steps are skipped, not the job. | ||
| # Whether this run changes code (anything but documentation). Code-free runs skip |
There was a problem hiding this comment.
This comment block is about 15 lines. The repo rule (scripts/check_comment_blocks.py) allows blocks of at most two # lines. It fails once a PR adds a block of three or more lines. Condense it to the constraint or reason. Move the scope and docs-set detail to the PR description or dokimi/docs.
There was a problem hiding this comment.
Declined: the 21-line block at lines 28-48 is already on intel/main and this PR does not touch it (its hunks are at 51 and 66+). check_comment_blocks.py --base intel/main --head HEAD passes; trimming that block belongs in a separate cleanup.
| set_code() { echo "code=$1" >> "$GITHUB_OUTPUT"; echo "changes: code=$1 ($2)"; } | ||
| if [ "$EVENT" != pull_request ]; then set_code true "not a pull request"; exit 0; fi | ||
| # --no-renames: a rename from code to Markdown must count as the code it removes. | ||
| if [ "$ACTION" = synchronize ]; then |
There was a problem hiding this comment.
Per-push scoping can turn the check green without the code ever being smoke-tested. The concurrency group cancels the in-flight run on each new push. So code push A starts the smoke test, a docs-only push B cancels it, and B's run skips every step and reports success. The head commit then shows a green check although the Docker smoke test never passed on that code. The same happens if A's run had already failed and B is docs-only. Required checks are not enforced today, so this is not blocking, but the result is misleading. Options: keep the whole-PR scope for the smoke job (base...HEAD), or only use push scope when the previous run for BEFORE succeeded.
There was a problem hiding this comment.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Under bash's pipefail, `printf | grep -qvE` fails when grep exits on the first code path before printf has written the rest of a long list, so a code push with many changed paths was read as documentation and skipped the smoke test. Feed the list to grep as a here-string instead. A new scope test pins a push with 2000 changed paths (red before the fix). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| # Newest commit reachable from AFTER whose smoke run passed (Verify pass rate succeeded); prints nothing if none. | ||
| # An unreadable run list also prints nothing; the caller then runs everything. | ||
| last_verified() { | ||
| runs=$(gh api "repos/$REPO/actions/workflows/agent-quality.yml/runs?branch=$HEAD_REF&event=pull_request&status=completed&per_page=20" \ |
There was a problem hiding this comment.
HEAD_REF goes into the URL query without encoding. Git allows branch names with &, # or +, and these would change or cut off the query. For example, a&per_page=1 or a#b would filter on the wrong branch, or on no branch. The result is a wrong or empty run list. An empty list falls back to code=true, so this is not unsafe, but it disables the optimization for those branches. Passing the parameters with -f lets gh encode them (on a GET request they are sent as query parameters).
Suggestion:
| runs=$(gh api "repos/$REPO/actions/workflows/agent-quality.yml/runs?branch=$HEAD_REF&event=pull_request&status=completed&per_page=20" \ | |
| runs=$(gh api "repos/$REPO/actions/workflows/agent-quality.yml/runs" \ | |
| -f branch="$HEAD_REF" -f event=pull_request -f status=completed -f per_page=20 \ |
There was a problem hiding this comment.
Fixed in 5d22ccb, unpushed: the runs query sends the branch as a GET field, so gh encodes &, # and +. The new test uses a&per_page=1#b+c, and the old URL form fails it (1 failed).
| || { set_code true "PR diff failed"; exit 0; } | ||
| fi | ||
| if [ -z "$files" ]; then set_code true "$scope diff is empty"; exit 0; fi | ||
| # A here-string, not a pipe: under pipefail, grep -q exiting early on a long list |
There was a problem hiding this comment.
This step has no shell: key, so the runner uses bash -e {0}, without pipefail. The comment gives a reason that does not apply here, although the here-string is still harmless. Separately, git diff --name-only quotes non-ASCII paths (core.quotepath), so a doc file such as "docs/é.md" ends in " and does not match DOCS. It is then counted as code. That fails safe, but you can avoid it with git -c core.quotepath=false diff ... in both diffs.
The changes step put HEAD_REF into the URL query unencoded, so a branch name with & # or + changed or cut off the query and listed the wrong runs. Pass the fields with -f and --method GET: gh turns -f fields into a POST unless the method is GET. The test fails on the old URL and passes now. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…step git quotes a non-ASCII path in --name-only output unless core.quotepath is off. A quoted "docs/é.md" does not match the DOCS pattern, so a docs-only change was read as code. It failed safe, but ran the smoke test for nothing. Both diffs now run with core.quotepath=false. The tests fail on the old diffs and pass now. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The step has no shell key, so it runs bash -e without pipefail. The comment gave a pipefail reason that does not apply to it. The here-string is still harmless, so only the comment changes. 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 9c88ee4. The push-scoped decision now diffs from the last commit whose smoke run passed Verify pass rate, which closes the cancel-then-skip case from my earlier thread. One thing is worth fixing before merge: the new tests do not pin the docs set or the rename handling, and those decide whether the smoke test is skipped.
What I ran, on a Linux review host at 9c88ee4:
make ci-local: 29 passed, 0 failed, 7 skipped (the Docker smoke job is among the skips). The 9 new tests pass, and the comment-block check passes.- The
changesstep script, executed against a scratch git repo with a stubbedgh, underbash -eandbash -eo pipefail: cancelled, failed, skipped and unreadable-run cases all end incode=true. A passed run on a commit that is not an ancestor of the push is ignored. Output is the same under both shells. - A mutation run on the step: 39 mutants, 30 of which pass the new test file.
Checked, no issues
- Masking: only a
Verify pass rateconclusion ofsuccesscounts as verified. A skipped docs-only run does not, so it cannot hide a failed or cancelled one. - Fail-safe paths: an unreadable run list, an unreadable jobs call, no passed run, a force push and an empty diff all give
code=true. - Long file lists: the here-string handles 2000 changed paths under
pipefail. - Permissions:
actions: readis added to the smoke job only, and a test pins it. - Commits: every non-merge commit carries
Signed-off-by.
|
|
||
| # (history after base, runs newest first as (commit, Verify pass rate conclusion) or None | ||
| # for an unreadable API, before, after, the code output expected) | ||
| SCENARIOS = { |
There was a problem hiding this comment.
The tests do not pin the docs set or the rename handling, so loosening either silently skips the smoke test. Every scenario uses README.md as the only docs path and src/app.py as the only code path, on a linear history with no renames. Mutation checks on the changes step, each of which passes all 9 tests:
DOCSending in\.[a-z]+$instead of\.md$(any extension counts as docs). Unanchoring the end, allowing nested paths indokimi/changelog.d/, and droppingdokimi/docs/or.github/also pass.- Removing
--no-renamesfrom both diffs. Asrc/app.pytoREADME.mdrename then lists onlyREADME.md, so code is read as docs. - Dropping the
git merge-base --is-ancestor "$sha" "$AFTER"check inlast_verified. After a rebase, an older passed run on a non-ancestor commit is then accepted as the diff base.
Each of these ends in code=false for a push that should run the smoke test.
Suggestion: add scenarios for (1) a push touching dokimi/data/x.md, dokimi/tests/x.md and a .py file next to a .md, expecting true; (2) a push touching dokimi/docs/sub/a.md and dokimi/changelog.d/1.md, expecting false; (3) a git mv src/app.py README.md commit, expecting true; (4) a passed run on a commit outside the history of after, expecting true.
There was a problem hiding this comment.
Fixed in 7d654b3: scenarios now cover a non-Markdown file under docs/, Markdown under data/ and tests/, docs/ at depth, a flat and a nested changelog.d entry, a code-to-docs rename and a passed run off the push's history. Each of the six mutants in your list now fails at least one test.
…nges step The scope tests used README.md as the only docs path and a linear history with no renames, so loosening the DOCS pattern, dropping --no-renames or the ancestor check in last_verified left them green. Add scenarios for each: a non-Markdown file under docs/, Markdown under data/ and tests/, docs/ at depth, a changelog.d entry one level down, a code-to-docs rename, and a passed run on a commit outside the push's history. Each of the six mutants fails at least one test; the workflow is unchanged. Thread: PRRT_kwDOVBg7g86x68oj Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| from=$(last_verified) | ||
| if [ -z "$from" ]; then set_code true "push: no passed smoke run to diff from"; exit 0; fi | ||
| scope="push since ${from:0:7}" | ||
| files=$(git -c core.quotepath=false diff --no-renames --name-only "$from" "$AFTER") \ |
There was a problem hiding this comment.
Must-fix: this diff ignores changes to the PR's base branch since the successful smoke run. Checkout uses the default PR merge commit, so if V passes against main=M1, main then receives a code change M2, and the PR pushes only README.md, V..AFTER contains only docs and this job skips testing the new merge with M2. Head ancestry alone does not establish that the checked-out code was verified. Record/validate the base revision (or compare the previously tested merge tree with the current checkout) before allowing the skip, and add a scenario where the base changes between the successful run and the docs-only push.
| def _runs_request(fake: Path) -> tuple[str, dict[str, list[str]]]: | ||
| """The method and the query of the runs request: the URL's own query, plus the -f fields.""" | ||
| args = (fake / "runs-request").read_text(encoding="utf-8").splitlines() | ||
| method = args[args.index("--method") + 1] if "--method" in args else "GET" |
There was a problem hiding this comment.
Must-fix: this fallback does not match gh api, which defaults to POST when -f/-F fields are supplied. Removing --method GET from the workflow therefore still passes the branch-query test: the stub serves the fixture regardless of method, and this helper reports GET. Infer POST when field arguments are present (unless an explicit method overrides it), so the test catches that broken runs-list request.
Suggestion:
| method = args[args.index("--method") + 1] if "--method" in args else "GET" | |
| field_flags = ("-f", "-F", "--raw-field", "--field") | |
| default_method = "POST" if any(flag in args for flag in field_flags) else "GET" | |
| method = args[args.index("--method") + 1] if "--method" in args else default_method |
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
_history had one if/elif chain over the change kinds, complexity 13 over the 10 cap with no baseline entry. Each kind is now a writer function in a dispatch table, the plain edit is _write_change, and the commit step is _commit. Same files, same order, same commits; the 18 tests are unchanged. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| verdict=$(gh api "repos/$REPO/actions/runs/$id/jobs" \ | ||
| --jq '.jobs[] | select(.name | startswith("External catalog smoke test")) | .steps[] | select(.name == "Verify pass rate") | .conclusion') || continue | ||
| if [ "$verdict" = success ]; then echo "$sha"; return 0; fi | ||
| done <<< "$runs" |
There was a problem hiding this comment.
Nit: declare shell: bash on the changes step. The new here-strings and ${from:0:7} expansion require Bash, but this step also runs on the self-hosted dokimi-ec2 runner without an explicit shell. GitHub's unspecified Linux shell can fall back to sh, which cannot parse these constructs. The new tests explicitly invoke Bash, so they do not cover that fallback.
| "PATH": f"{fake}{os.pathsep}{os.environ['PATH']}", | ||
| } | ||
| proc = subprocess.run( | ||
| ["bash", "--noprofile", "--norc", "-eo", "pipefail", "-c", step["run"]], |
There was a problem hiding this comment.
Must-fix: the harness enables pipefail, but the workflow has no explicit shell or shell default, so GitHub runs this step with bash -e without pipefail. This makes the bulk-path scenario reject a valid printf '%s\n' "$files" | grep -qvE "$DOCS" implementation: an early grep exit can give printf SIGPIPE, making the pipeline false only in the test. Run with the workflow's actual shell flags (or explicitly enable the same flags in the workflow) so these tests exercise CI's error handling.
Suggestion:
| ["bash", "--noprofile", "--norc", "-eo", "pipefail", "-c", step["run"]], | |
| ["bash", "--noprofile", "--norc", "-e", "-c", step["run"]], |
Summary
In
agent-quality.yml, a new push to a PR now decides whether the code steps run from the push itself, not from the whole PR. A docs-only push skips the Docker smoke test steps.What is skipped
The
changesstep sits in theexternal-catalog-smokejob, not in its own job. Whencode=false, the job's steps after the checkout are skipped (Python setup, install, clone of uxlfoundation/skills, catalog scan, task run, pass-rate check, artifact upload). The job reports success. Thebenchmarkjob still runs on every event, since it measures the docs.ci.yml(static, mypy, tests, package-smoke, windows-smoke) is unchanged, so a docs-only push still runs those jobs. See Risks.Scope of the decision:
synchronize(a new push): diffsgithub.event.before..github.event.after. These are the push's own commits. HEAD is not used, because onpull_requestevents the checkout is the merge commit.opened,reopened, push to main,workflow_dispatch: unchanged (whole PR, or always on).DOCSin the step env): root Markdown,dokimi/*.md,dokimi/docs/,dokimi/changelog.d/,.github/**.md. Not counted as docs:dokimi/data/(read at run time),dokimi/tests/,.agent-eval/.--no-renameson both diffs, so a code file renamed to Markdown still counts as code. This also makes the whole-PR decision stricter, only for renames.Evidence
bash -erun of the extracted step script on a scratch repo with real git (sim/simulate.sh): 19 of 19 pass.Static: zizmor 1.31.1 (the pinned version)
--offline --min-severity medium: no findings. actionlint 1.7.7.23: clean.Self-review (pr-review local mode, lane L1): no confirmed blocking findings. Run by hand in one pass; no subagent tool was available.
Risks
github.event.beforeandafteron asynchronizeevent are assumed from the GitHub payload. If either is missing, the fallback runs the full job, so this fails safe.ci.yml's code jobs (test matrix, mypy, package-smoke, windows-smoke) still run on every push. Gating them is a separate change.🤖 Generated with Claude Code