Skip to content

action/setup: composite setup action for dokimi CI; repo-eval uses it (part of #318) - #319

Open
napetrov wants to merge 23 commits into
intel:mainfrom
napetrov:ci/action-setup
Open

napetrov wants to merge 23 commits into
intel:mainfrom
napetrov:ci/action-setup

Conversation

@napetrov

@napetrov napetrov commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Step 2 of #318: a composite action at actions/setup that installs dokimi at one pinned commit, sets up Python, and exports what a dokimi run expects on CI. repo-eval.yml now uses it for its install and setup steps. Its inputs, outputs and permissions are unchanged, and no secret is added. Part of #318.

Changes

  • actions/setup/action.yml (new, composite). Inputs: dokimi-ref (SHA or tag; a branch is refused), dokimi-repository, python-version, out-dir, cache, paid, max-usd. Outputs: sha (the resolved commit) and out-dir. It exports CI=true, DOKIMI_RUNS_DIR (the runs/ directory under out-dir) and, when max-usd is set, DOKIMI_MAX_USD.
  • repo-eval.yml: the ref resolution and the two install steps moved out of the jobs into the action. validate, scan and tasks-check now check out the action from job.workflow_sha (the called workflow's own commit, sparse /actions/, into .dokimi-action/) and call it. The checkout stays until the job ends, because the composite's post steps read its files from that path (removing it failed CI run 41170348715).
  • repo-eval-self.yml: actions/** triggers the self-check, and actions/ is in its smoke paths.
  • ci.yml (zizmor step) and .pre-commit-config.yaml (zizmor hook) cover actions/ as well as the workflows.

Design notes

  • Paid steps. paid defaults to false, so a free caller needs no cap. With paid: true, a missing max-usd fails the job before anything is installed. A max-usd that is not a positive number also fails.
  • Where the action comes from. A reusable workflow cannot read a local ./action from the dokimi checkout, because ./ resolves against the caller's workspace. The action is therefore checked out at job.workflow_sha. That is the commit the caller's uses: line pins, so the action always matches the workflow. dokimi-ref only decides which dokimi is installed.
  • Pip cache. cache: true keys pip's cache by the resolved commit. Off by default, so repo-eval's behaviour is unchanged.
  • GITHUB_ENV. Each value written there is checked again in the output step (no newline, a number for max-usd). zizmor's github-env finding on that step is ignored with that reason on the run: line.

Evidence

CI run 41170348715 on the first push (aa50176): self / repo validate failed only in Post Set up dokimi, after the setup and validate steps had passed; scan and tasks-check were skipped. Fixed in 6cd5a9a by keeping the checkout.

Head c2c85558: CI on the PR is green (21 checks passed, 2 skipped: the Pages deploy and the latest-deps tests, as before). repo-eval-self run 41173755046 on this head: self / repo validate, self / repo scan and both self / repo tasks check jobs succeeded. Local: zizmor 1.31.1 offline rc=0 on .github/workflows and actions; the comment-block lint rc=0 (the first CI red, run 41170747074, was a 3-line comment, fixed in 6ea3442); pytest on the task-selection, advisory-list and cli-interface workflow tests, 108 passed. make ci-local was not run.

Merge danger

  • External SHA-pinned callers (oneDAL and others). A caller pinned to an older repo-eval.yml SHA is unaffected. When it bumps uses: to a commit with this change, the workflow checks out intel/dokimi at that commit and reads the action from it. The rule that uses: and dokimi-ref name the same commit still holds. A caller whose dokimi-ref predates this PR still works, because the action is not read from dokimi-ref.
  • Cost. Each job does one extra sparse checkout and one setup step (a few seconds). .dokimi-action/ (one file, actions/setup/action.yml) stays in the caller's workspace for the job, so an image build context that copies the whole tree sees it.
  • Not exercised here. A caller in another repository (job.workflow_sha resolving against intel/dokimi from there) cannot run in this repo's CI. Only the self call (a local ./.github/workflows/repo-eval.yml) is exercised by repo-eval-self. The path depends on job.workflow_sha, which the GitHub docs define for jobs of a called workflow.
  • Tags. Tag resolution moved into the action unchanged, but it needs the network (git ls-remote). An environment without it fails at that step, as before.

🤖 Generated with Claude Code

…l#318)

action/setup installs dokimi at a pinned commit (SHA, or a tag resolved to
its commit; a branch is refused) with the locked dependencies, sets up
Python, and exports CI=true, DOKIMI_RUNS_DIR and, with max-usd, DOKIMI_MAX_USD.
The output directory has a fixed layout (runs/) for upload-artifact. A paid
job (paid: true) must pass max-usd; a free job needs none.

repo-eval.yml moves its ref resolution and install into the action; its
inputs, outputs and permissions are unchanged. The action is read from the
called workflow's own commit (job.workflow_sha), so external callers need no
checkout of their own, and it is removed again after the setup step.

zizmor covers action/ in CI and pre-commit; repo-eval-self runs on action/ changes.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
intel#318)

The composite's post steps read its action.yml from .dokimi-action at job
end. Removing the checkout after the setup step made "Post Set up dokimi"
fail (CI run 41170348715, self / repo validate; scan and tasks-check were
skipped behind it). The checkout is one sparse file tree, so it stays.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The comment block above the validate job's setup-action checkout ran to
three lines, which the comment-block check fails (AGENTS.md: two lines of
reason, link the rest). Same content, shorter: the action comes from the
workflow's commit, and it stays until the job ends because the composite's
post steps read it.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
@codecov-commenter

codecov-commenter commented Oct 31, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.22%. Comparing base (d3e2a26) to head (ef19fa0).
⚠️ Report is 1 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #319      +/-   ##
==========================================
+ Coverage   94.20%   94.22%   +0.01%     
==========================================
  Files         304      304              
  Lines       38748    38766      +18     
  Branches     6561     6567       +6     
==========================================
+ Hits        36504    36526      +22     
+ Misses       1430     1428       -2     
+ Partials      814      812       -2     

see 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Follows the layout of abicheck: sub-actions live under actions/<name>/, and
the public entry, if one is added later, is a root action.yml. Paths move in
repo-eval.yml (sparse checkout and the uses line), repo-eval-self.yml (the
path filter and smoke paths), ci.yml (the zizmor scope) and the pre-commit
zizmor files regex. No behaviour change.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
@napetrov
napetrov marked this pull request as ready for review October 31, 2026 17:01
@napetrov
napetrov requested a review from a team as a code owner October 31, 2026 17:01
@exai-bot

exai-bot commented Oct 31, 2026 •

Copy link
Copy Markdown

🔍 OpenCodeReview found 1 issue(s) in this PR.

By severity: 1 medium

Severity Category Location Finding
medium test test_setup_action.py:185 Must-fix: This test does not guard which revision supplies the dependency lock.
Review details
  • Commit reviewed: ef19fa0 against merge base d3e2a26
  • Coverage: 8 files in 3 groups
  • Engine: OpenCodeReview v1.13.3, us.openai.gpt-6.1-sol
  • Usage: 347k tokens (193k from cache), 43s
  • Tool calls: 22 (12 file_read, 6 code_search, 3 file_read_diff, 1 code_comment)
  • Shared setup action and CI integration: 3 files
  • Repository evaluation workflows, contract, and checkout tests: 4 files
  • Pre-commit configuration: 1 file

Comment thread .github/workflows/repo-eval.yml Outdated
- name: Check out the setup action
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
repository: ${{ inputs.dokimi-repository }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
Must-fix: The action-source repository is being taken from the installation input. For example, a caller using intel/dokimi/.github/workflows/repo-eval.yml@A with dokimi-repository: user/dokimi and a fork installation ref B should still load the setup action from intel/dokimi at A. Once the workflow SHA is resolved correctly, this checkout instead requests A from the fork and fails if that commit is absent. Keep the workflow/action source repository independent of dokimi-repository, which selects the package to install. Apply the change to all three action checkouts.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on a decision before I change this: which repository the setup action is checked out from.

Context: the action checkout (repo-eval.yml, lines 125 and similar) uses repository: ${{ inputs.dokimi-repository }}. I confirmed on the code that a caller with intel/dokimi/.github/workflows/repo-eval.yml@A and dokimi-repository: user/dokimi asks the fork for commit A. The fork fails when A is not there.

Options:

  1. Always check out intel/dokimi. Simple, but fork testing of action changes then needs a fork-hosted workflow.
  2. A new input dokimi-action-repository, default intel/dokimi. Fork testing becomes explicit.
  3. Keep as is and document that the fork must hold the SHA.

My recommendation: option 2. Its default fixes the reported case, and fork testing stays explicit. The matching ref fix (job.workflow_sha, see the thread on line 126) depends on the same decision.

The thread stays open until you choose.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 71dbbbf, pushed as 5b88af0.

repo-eval.yml has a new input dokimi-action-repository (default intel/dokimi). The setup-action checkout in all three jobs (validate, scan, tasks-check) takes its repository from it, not from dokimi-repository, which now only selects the package installed. repo-eval-self.yml passes its own repository as dokimi-action-repository, so the fork's self-check still checks out the commit it runs.

Testing a fork's commit now sets both dokimi-action-repository and dokimi-repository to the fork. The contract doc lists the input.

Test: dokimi/tests/test_repo_eval_action_checkout.py (red before the change, green at 5b88af0).

Comment thread .github/workflows/repo-eval.yml Outdated
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
repository: ${{ inputs.dokimi-repository }}
ref: ${{ job.workflow_sha }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · high
Must-fix: workflow_sha is not a supported field of the GitHub Actions job context (job_workflow_sha is an OIDC claim, not a context property). This expression therefore evaluates to an empty string. Checkout then selects the caller's event ref when checking out the same repository, or the target repository's default branch otherwise. In particular, an external SHA-pinned caller loads the action from the default branch rather than the pinned reusable-workflow commit. Resolve the called workflow's source commit through a supported mechanism and use it in all three action checkouts (validate, scan, and tasks-check).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on a decision before I change this: which ref the setup action is checked out at.

Context: I confirmed that job.workflow_sha is not a property of the GitHub Actions job context, so the expression is empty and the checkout falls back to the caller's ref or the default branch. The fix needs a source for the called workflow's own commit. The job_workflow_sha OIDC claim needs id-token: write, which this workflow does not grant.

Options:

  1. Check out the action at inputs.dokimi-ref. The contract already requires the caller to pin dokimi-ref to the same commit as its uses: line, so this matches what runs. It is enforced only by the docs.
  2. Grant id-token: write and read job_workflow_sha. Adds a permission the caller must grant.

My recommendation: option 1, in the repository chosen on the thread for line 125. The decision is the same one, so it waits on that answer.

The thread stays open until you choose.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 71dbbbf, pushed as 5b88af0.

The three setup-action checkouts now use ref: ${{ inputs.dokimi-ref }} instead of job.workflow_sha, in the repository from dokimi-action-repository (see the thread on line 125). No new input was needed: dokimi-ref already exists on the workflow and is the commit the caller pins in uses:. As before, the docs require the caller to pin dokimi-ref to the same commit as uses:; nothing enforces that.

Test: test_job_workflow_sha_is_not_used and test_every_action_checkout_is_at_dokimi_ref in dokimi/tests/test_repo_eval_action_checkout.py (red before the change, green at 5b88af0).

Comment thread actions/setup/action.yml Outdated
with:
python-version: ${{ inputs.python-version }}
cache: ${{ inputs.cache == 'true' && 'pip' || '' }}
cache-dependency-path: ${{ runner.temp }}/dokimi-setup/pip-key.txt

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
Must-fix: With cache: 'true', setup-python hashes this path using @actions/glob.hashFiles, which excludes files outside GITHUB_WORKSPACE. On standard runners, runner.temp is outside that workspace, so the key file produces an empty hash and setup-python fails because no dependency file matched. The default cache-disabled callers do not exercise this path. Create the key file under GITHUB_WORKSPACE and pass that path here; add coverage for a cache-enabled invocation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8ee83fa. The pip cache key now lives under $GITHUB_WORKSPACE/.dokimi-setup/, and cache-dependency-path points at that file, so setup-python's hashFiles sees it.

Coverage: dokimi/tests/test_setup_action.py pins both ends statically (red on c2c8555, green on 8ee83fa). I did not run a cache-enabled job on a runner, so that path is verified by the static check only.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseded by 3aa37ff. The pip cache is gone, with its key file and cache-dependency-path. CodeQL reports actions/cache-poisoning/poisonable-step on the setup steps (a checkout of the caller's dokimi-ref, then a cache restore). The cache input had no caller, so it was removed rather than kept dead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL actions/cache-poisoning/poisonable-step alerts 82-84 are dismissed as false positives, per the maintainer decision on this PR. No cache is restored in these steps; the setup step runs setup-python without a cache.

Comment thread actions/setup/action.yml Outdated
fi
echo "dir=$dir" >> "$GITHUB_OUTPUT"
echo "CI=true" >> "$GITHUB_ENV"
echo "DOKIMI_RUNS_DIR=$dir/runs" >> "$GITHUB_ENV"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
Must-fix: This redirects artifact readers, but not artifact writers. DOKIMI_RUNS_DIR is consumed only by dokimi.runs.scan; for example, novelty still defaults to results/novelty/&lt;ref> in commands/novelty.py::_default_dir. After setup, a novelty run without an explicit output path therefore writes outside the advertised output directory, dokimi runs list scans the empty directory created here, and uploading the action's out-dir misses the results. Wire the output root into writers, or require callers to pass explicit output paths and avoid overriding the reader root implicitly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on a decision before I change this: what the DOKIMI_RUNS_DIR export should do.

Context: I confirmed the claim. DOKIMI_RUNS_DIR is read only by dokimi runs (runs/scan.py). The writers do not read it: novelty's _default_dir writes to results/novelty/<ref> relative to the working directory. So the action's out-dir can be empty while results land elsewhere.

Options:

  1. Drop the DOKIMI_RUNS_DIR export. Callers pass explicit output paths, and out-dir only holds what they write there. Smallest change, matches the reviewer's second option.
  2. Wire the output root into every writer (novelty and any other command that writes results). Larger, and it changes CLI defaults.

My recommendation: option 1 in this PR, with option 2 as a follow-up if you want the default output location.

The thread stays open until you choose.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 71dbbbf, pushed as 5b88af0.

The setup action no longer exports DOKIMI_RUNS_DIR, and no longer creates the runs/ directory it pointed at. out-dir is the directory the caller writes under; its description no longer names a runs root. Callers pass explicit output paths to the writers (novelty and the rest), as option 1 says. Wiring the output root into the writers is not in this PR.

Test: test_the_action_exports_no_dokimi_runs_dir_or_max_usd in dokimi/tests/test_setup_action.py (red before the change, green at 5b88af0).

Comment thread actions/setup/action.yml Outdated
echo "dir=$dir" >> "$GITHUB_OUTPUT"
echo "CI=true" >> "$GITHUB_ENV"
echo "DOKIMI_RUNS_DIR=$dir/runs" >> "$GITHUB_ENV"
if [ -n "$MAX_USD" ]; then echo "DOKIMI_MAX_USD=$MAX_USD" >> "$GITHUB_ENV"; fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · high
Must-fix: paid: 'true' with max-usd: '1' passes validation, but exporting this variable does not enforce a spending cap. The only mention of DOKIMI_MAX_USD in the package is the data-only cli_flags.py registry; novelty's parser defaults max_usd to None, and _spend_cap() reads only that argument. Thus a subsequent paid novelty command without --max-usd still runs uncapped. Wire the environment value into the supported paid commands before accepting it as a safety guard. Also, a whole-job cap needs shared accounting across commands; independent per-command caps would still allow the job to exceed this input.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on a decision before I change this: whether the action keeps paid and max-usd until a cap is enforced.

Context: I confirmed the claim. DOKIMI_MAX_USD is set in the environment, but no code reads it. The cli_flags.py entry is registry data only, and novelty's _spend_cap reads only --max-usd. So paid: 'true' with max-usd: '1' passes validation and does not cap spend.

Options:

  1. Remove paid and max-usd from the action until a cap is enforced. A safety input that enforces nothing should not ship.
  2. Read DOKIMI_MAX_USD as the default --max-usd of the paid commands. This is a per-command cap, so a job with several commands can still exceed the input.
  3. Option 2 plus shared accounting across commands for a whole-job cap. The largest change.

My recommendation: option 1 now, and track options 2 and 3 as a follow-up issue. Say if you want the follow-up opened.

The thread stays open until you choose.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 71dbbbf, pushed as 5b88af0.

The paid and max-usd inputs are removed, with their validation step and the DOKIMI_MAX_USD export. Nothing else in the repo or its docs documents them, so no documented input breaks. Options 2 and 3 (a per-command cap, then whole-job accounting) are not in this PR.

Test: test_the_action_declares_no_paid_or_max_usd_input in dokimi/tests/test_setup_action.py (red before the change, green at 5b88af0).

@napetrov

Copy link
Copy Markdown
Contributor Author

Correction to the reply on the pip cache key thread: the fix is now at 6c4a171, not 8ee83fa. The first push lacked the Signed-off-by line, so the DCO check failed. The commit was amended to add it, with the same content. The other earlier replies on this PR are unchanged.

setup-python hashes cache-dependency-path with glob.hashFiles, which only
sees files under GITHUB_WORKSPACE. The key file was in runner.temp, so a
cache-enabled call hashed nothing and setup-python failed.

The key file now sits in $GITHUB_WORKSPACE/.dokimi-setup, and
cache-dependency-path points at it. The test pins both ends statically;
no runner runs a cache-enabled job here.

Thread: PRRT_kwDOVBg7g86ySzDB
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The reusable workflow checked out the setup action from dokimi-repository,
the package source, at job.workflow_sha. That expression is not a context
property, so the ref was empty, and a fork's dokimi-repository asked the
fork for a commit it may not hold.

- New input dokimi-action-repository (default intel/dokimi) is the repository
  of the action checkout; all three checkouts use it.
- The checkout ref is dokimi-ref, the commit the caller already pins in uses:.
- repo-eval-self.yml passes its own repository, so a fork's self-check still
  finds its commit.

The setup action also drops the paid and max-usd inputs and the DOKIMI_RUNS_DIR
and DOKIMI_MAX_USD exports. Nothing enforced the cap, and no writer reads
DOKIMI_RUNS_DIR; out-dir is the directory the caller writes to.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Copilot AI balanced review requested due to automatic review settings November 1, 2026 01:07
…ghten the key test

- repo-eval.yml: the checkout comment said the composite's post steps read the
  checkout; the composite has no post step.
- .pre-commit-config.yaml: the zizmor hook matched only actions/<dir>/action.yml,
  not nested composites that the CI zizmor run on `actions` covers.
- test_setup_action.py: the pip-key test matched the path anywhere in a step;
  it now needs the path to be the target of a redirect.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Comment on lines +131 to +133
repository: ${{ inputs.dokimi-action-repository }}
ref: ${{ inputs.dokimi-ref }}
path: .dokimi-action

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
Must-fix: With a supported tag such as dokimi-ref: vX.Y.Z, validate can check out action commit A, then scan and tasks-check can check out commit B if the tag moves while validate runs or matrix jobs queue. Those jobs still install validate's pinned package SHA, so they can execute a different setup implementation against the old package (or fail if the action path/inputs changed). This affects both downstream checkout blocks. Capture the actual action checkout's commit in validate (e.g. git -C .dokimi-action rev-parse HEAD), expose it as a separate job output, and use that SHA for downstream action checkouts; do not assume the package SHA identifies the action commit when the repositories differ. Update the checkout tests to enforce this pinning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3aa37ff. validate records the commit its setup-action checkout resolved to (git -C .dokimi-action rev-parse HEAD) as the job output action-sha.
scan and tasks-check check the action out at needs.validate.outputs.action-sha, not at dokimi-ref, so a tag that moves mid-run cannot split the jobs. The package was already pinned by dokimi-sha.
Test: test_downstream_jobs_check_the_action_out_at_the_commit_validate_recorded in dokimi/tests/test_repo_eval_action_checkout.py.

Comment thread actions/setup/action.yml Outdated
run: | # zizmor: ignore[github-env] -- intended: the only value written to GITHUB_ENV is the constant CI=true
out=${OUT:-$RUNNER_TEMP/dokimi-out}
mkdir -p "$out"
dir=$(cd "$out" && pwd)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
Must-fix: Bash command substitution strips trailing newlines. With an out-dir such as $'reports\n', mkdir creates that directory, but dir=$(cd "$out" && pwd) removes the trailing newline from its absolute path. The subsequent check passes and exports a different, potentially nonexistent directory, so output writers or artifact uploads using this output target the wrong path. Reject newline-containing input before creating the directory, and preserve trailing newlines when obtaining the absolute path so the final validation remains effective.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3aa37ff. An out-dir with a newline is refused before mkdir, and the captured absolute path keeps a trailing newline (printf x), so the check after the capture sees it too.
Tests: test_out_dir_newline_is_refused_before_mkdir and test_out_dir_capture_keeps_a_trailing_newline in dokimi/tests/test_setup_action.py.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Trailing-newline output paths can bypass validation and produce invalid exported paths, while downstream checkouts can resolve moved release tags to an action that differs from the validated version.

2 open issues
What changed in this PR

3 new findings: 2 must-fix and 1 nit.

This PR introduces a reusable setup action for dokimi CI and adopts it in repository evaluation, advancing the action building blocks planned in #318.

Changes:

  • Centralizes pinned installation, Python setup, optional pip caching, and output-directory creation.
  • Updates repository-evaluation jobs to check out the action, with explicit support for fork repositories.
  • Adds regression tests and extends self-check and security-scanning coverage to composite actions.
File Description
dokimi/​tests/​test_setup_action.py Tests cache paths and the setup-only input/export contract.
dokimi/​tests/​test_repo_eval_action_checkout.py Tests action repository and ref selection.
dokimi/​docs/​repo-evaluation-contract.md Documents separate package and action repositories.
actions/​setup/​action.yml Adds the shared installation and setup action.
.pre-commit-config.yaml Includes composite actions in security scanning.
.github/​workflows/​repo-eval.yml Replaces repeated installation steps with the action.
.github/​workflows/​repo-eval-self.yml Tests action changes and passes the self-check repository.
.github/​workflows/​ci.yml Extends CI security scanning to actions/.

Review effort: Balanced

Comment thread .github/workflows/repo-eval.yml Outdated
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
repository: ${{ inputs.dokimi-action-repository }}
ref: ${{ inputs.dokimi-ref }}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3aa37ff, same change as the validate thread: scan (checkout at line 297) and tasks-check (line 369) check the action out at needs.validate.outputs.action-sha, not at dokimi-ref.
The package stays pinned by dokimi-sha, as before.

Comment thread actions/setup/action.yml Outdated
run: | # zizmor: ignore[github-env] -- intended: the only value written to GITHUB_ENV is the constant CI=true
out=${OUT:-$RUNNER_TEMP/dokimi-out}
mkdir -p "$out"
dir=$(cd "$out" && pwd)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3aa37ff, same change as the exai-bot thread on this line: the newline check now runs before mkdir, and the captured path keeps its trailing newline.

CodeQL reports actions/cache-poisoning/poisonable-step on the setup
steps: a pip cache restored in a run that checks out the caller's
dokimi-ref is the path it flags. The cache input was opt-in and no
caller or doc uses it, so setup-python runs without a cache, and the
key file that fed it is gone.

out-dir: a newline is refused before mkdir, and the captured absolute
path keeps a trailing newline so the check after the capture sees it.

validate records the commit the setup action was checked out at
(action-sha). scan and tasks-check check the action out at that SHA,
not at dokimi-ref again, so a tag that moves mid-run does not split
the jobs.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Comment on lines +145 to +147
- name: Set up dokimi
id: dokimi
uses: ./.dokimi-action/actions/setup

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · high
Must-fix: This newly introduced setup call always fails with the current composite action, including when out-dir is omitted. In actions/setup/action.yml, dir=$(cd "$out" && pwd && printf x); dir=${dir%x} preserves the newline that pwd itself prints. Removing x leaves that newline in dir, so the subsequent newline check exits 1 for every normal directory. Consequently validate never runs, and scan/tasks-check are skipped (their setup calls have the same defect). Capture the directory without adding a newline, e.g. dir=$(cd "$out" && printf '%s' "$PWD" && printf x); dir=${dir%x}, and add a shell-level test that default setup succeeds while newline-containing paths are rejected.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same root cause, fixed in 16df6e9 (the out-dir capture in actions/setup/action.yml). A normal out-dir now passes setup, so validate, scan and tasks-check can run again.

Comment thread actions/setup/action.yml Outdated
if [[ "$out" == *$'\n'* ]]; then echo "::error title=out-dir::out-dir has a newline"; exit 1; fi
mkdir -p "$out"
# The x keeps a trailing newline from being stripped by the command substitution.
dir=$(cd "$out" && pwd && printf x); dir=${dir%x}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · high
Must-fix: pwd emits a terminating newline, and the appended x prevents command substitution from stripping it. Removing only x therefore leaves every ordinary path (including the default $RUNNER_TEMP/dokimi-out) ending in a newline, so the next check always exits 1. This also blocks repo-eval's validate job and its dependent jobs. Capture the path without adding pwd's line terminator, and add a test that executes this shell step with a normal directory and checks successful output; the current sentinel-syntax test passes with this broken implementation.

Suggestion:

Suggested change
dir=$(cd "$out" && pwd && printf x); dir=${dir%x}
dir=$(cd "$out" && printf '%sx' "$PWD"); dir=${dir%x}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 16df6e9. The capture keeps the x sentinel again, so a newline at the end of $PWD is kept and refused; a normal path, including the default, is written as one line.
Tests run the step script under bash -e -o pipefail: the default path, an out-dir with a newline (refused before mkdir), and a cwd ending in a newline (refused).

Comment thread dokimi/tests/test_setup_action.py Outdated
Comment on lines +35 to +36
refuse = run.index("*$'\\n'*")
assert refuse < run.index('mkdir -p "$out"')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test · medium
Must-fix: This checks only the position of a substring, not whether a newline is rejected before directory creation. Replacing the first guard's exit 1 with : leaves this test green, but OUT=$'reports\n' then reaches mkdir and creates the forbidden directory before the later check fails. Execute the extracted output-step script in a subprocess with temporary RUNNER_TEMP/GITHUB_OUTPUT/GITHUB_ENV paths, and assert both a nonzero exit and that the newline-named directory was not created.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced the substring check with a test that runs the output step under bash -e -o pipefail. It asserts the default path is written as one dir= line, and that OUT=$'reports\n' exits nonzero with no reports* directory created (16df6e9).

pwd prints a line terminator, so the capture always kept a newline and
the next check refused every out-dir, failing validate and its
dependents. The capture keeps the sentinel so a newline at the end of
the path is still seen and refused. The tests now run the step script
with bash, covering the default path, an out-dir with a newline, and a
cwd ending in a newline.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Comment thread .github/workflows/repo-eval.yml Outdated
Comment on lines +132 to +133
repository: ${{ inputs.dokimi-action-repository }}
ref: ${{ inputs.dokimi-ref }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security · high
Must-fix: Resolve the action ref as a SHA or an explicit tag before checking it out. For a supported tag such as release that also has a branch named release, actions/checkout gives the branch precedence for this unqualified ref, whereas the setup action resolves the package with git ls-remote --tags. Consequently validate executes the mutable branch's action rather than the pinned tag's action; recording action-sha only propagates that incorrect selection to scan and tasks-check. Branch-only inputs also execute branch-controlled action code before the action can reject them. Validate/resolve the ref against dokimi-action-repository before checkout and pass the resolved commit SHA (or a validated refs/tags/... ref), with coverage for a branch/tag name collision.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fe4f247. Validate now resolves dokimi-ref to a commit before the checkout: a 40-digit SHA passes through, a tag resolves to its peeled ^{} commit, anything else is refused. The checkout and action-sha both use that commit, so a same-named branch cannot run.

Comment thread actions/setup/action.yml Outdated
out=${OUT:-$RUNNER_TEMP/dokimi-out}
# Refused before mkdir, so a newline name is never created.
if [[ "$out" == *$'\n'* ]]; then echo "::error title=out-dir::out-dir has a newline"; exit 1; fi
mkdir -p "$out"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
Must-fix: With out-dir: -reports, mkdir -p "$out" interprets the valid relative directory name as options and exits unsuccessfully, failing setup after installation. cd "$out" has the same option-parsing problem if only mkdir is fixed. Add -- to both commands and a regression test using _run_out_step(tmp_path, "-reports") that asserts success and the correct absolute output path.

Suggestion:

Suggested change
mkdir -p "$out"
mkdir -p -- "$out"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already at head, no change. actions/setup/action.yml:97-100 has [[ "$out" == /* ]] || out=./$out, then mkdir -p -- "$out" and cd -- "$out". test_setup_action.py::_run_out_step(tmp_path, "-reports") asserts success and the absolute output path.

Comment on lines +56 to +58
assert "action-sha" in workflow["jobs"]["validate"]["outputs"]
recorder = [s for s in workflow["jobs"]["validate"]["steps"] if s.get("id") == "action"]
assert recorder and "rev-parse HEAD" in recorder[0]["run"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test · medium
Must-fix: This regression test still passes if action-sha is changed to ${{ steps.dokimi.outputs.sha }}, or if the recorder loses -C .dokimi-action and records the caller repository's HEAD instead. Either mutation can make downstream jobs check out the wrong commit or fail, particularly when the action and package repositories differ. Assert the exact output mapping to steps.action.outputs.sha and that the recorder reads HEAD from .dokimi-action, not merely that the output exists and the script contains rev-parse HEAD.

Suggestion:

Suggested change
assert "action-sha" in workflow["jobs"]["validate"]["outputs"]
recorder = [s for s in workflow["jobs"]["validate"]["steps"] if s.get("id") == "action"]
assert recorder and "rev-parse HEAD" in recorder[0]["run"]
assert workflow["jobs"]["validate"]["outputs"]["action-sha"] == "${{ steps.action.outputs.sha }}"
recorder = [s for s in workflow["jobs"]["validate"]["steps"] if s.get("id") == "action"]
assert recorder and recorder[0]["run"] == 'echo "sha=$(git -C .dokimi-action rev-parse HEAD)" >> "$GITHUB_OUTPUT"'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already at head (a2f5d46). test_repo_eval_action_checkout.py asserts action-sha == steps.action.outputs.sha exactly, and the recorder command git -C .dokimi-action rev-parse HEAD. Changing either mapping fails the test.

@antonwolfy antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at 16df6e9. The newline handling in the output step is right now, and the action/workflow split holds. The description no longer matches the head, and the merge also waits on one open bot point. Three nits inline.

What I ran, on a Linux review host at 16df6e9:

  • make ci-local: 30 passed, 0 failed, 7 skipped (the skips need a PR event, micromamba or semgrep); changed-line coverage passes.
  • The output step's script under bash -e -o pipefail, with the runner env files pointed at a temp dir.
  • Mutations of the new tests against broken copies of the workflow and the action.

Findings outside the diff lines

  1. The description and the contract no longer say what the head does. The description says a dokimi-ref older than this PR still works because the action is not read from dokimi-ref. At this head the action is checked out at ref: ${{ inputs.dokimi-ref }} (repo-eval.yml:133) and run from .dokimi-action/actions/setup. A dokimi-ref without actions/setup gives an empty sparse checkout, and the job fails at "Set up dokimi". The description also still lists paid, max-usd, cache, DOKIMI_RUNS_DIR and job.workflow_sha, none of which exist now, and says the repo-eval inputs are unchanged although dokimi-action-repository is new. The header of repo-eval.yml and repo-evaluation-contract.md (~line 1493) say dokimi-ref selects the package; they should also say it must contain actions/setup. Suggestion: update the description, and add that sentence to both places.

Open bot points I confirm:

  • I agree with exai-bot that validate checks the action out at the bare dokimi-ref (repo-eval.yml:133), so a tag that shares its name with a branch can check out the branch while the package resolves from the tag; this is from reading how actions/checkout picks a ref, not a run.
Bot points I checked that no longer apply at this head (12)
  • exai-bot's point on the action repository does not apply: the checkouts now read it from dokimi-action-repository, independent of dokimi-repository.
  • exai-bot's point on job.workflow_sha does not apply: no checkout uses it any more, and test_job_workflow_sha_is_not_used pins that.
  • exai-bot's point on the pip cache key file does not apply: the pip cache and its key file are removed.
  • exai-bot's point on DOKIMI_RUNS_DIR does not apply: the action no longer exports it.
  • exai-bot's point on DOKIMI_MAX_USD does not apply: the paid and max-usd inputs and the export are removed.
  • exai-bot's point on a moving tag between jobs does not apply: scan and tasks-check check the action out at the action-sha that validate records.
  • exai-bot's point on the trailing newline in out-dir does not apply: a newline is refused before mkdir, and the captured path keeps its trailing newline.
  • Copilot's point on a moving tag between jobs does not apply: both downstream checkouts use needs.validate.outputs.action-sha.
  • Copilot's point on the trailing newline does not apply: the capture keeps it and the guard refuses it; I ran the step with a cwd ending in a newline and it exits 1.
  • exai-bot's point on every normal out-dir failing does not apply: the step now writes a one-line dir= for the default path (reproduced under bash -e -o pipefail).
  • exai-bot's point on the pwd newline does not apply at this head: the capture uses printf '%sx' "$PWD", so a normal path is written as one line.
  • exai-bot's point on the newline test does not apply: the tests now run the step script and assert a nonzero exit and no reports* directory.

Comment thread actions/setup/action.yml Resolved
Comment thread dokimi/tests/test_setup_action.py Resolved
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Comment thread actions/setup/action.yml Resolved
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

@antonwolfy antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit per nit, up to 0880899, which fix the three nits from my review:

  • ff84af5: actions/setup/action.yml:96: mkdir -p -- and cd -- now treat an out-dir starting with a dash as a path; a test runs -reports and asserts exit 0 and dir=<tmp>/-reports.
  • 3181a51: dokimi/tests/test_setup_action.py:70: the newline-cwd test now runs the step with out-dir ., so only the x sentinel catches the newline.
  • a2f5d46: dokimi/tests/test_repo_eval_action_checkout.py:58: asserts the exact action-sha output mapping and the exact recorder command.

The review bot then flagged lines of those commits; fixed as well:

  • 0880899: actions/setup/action.yml:96: a relative out-dir is prefixed with ./ before mkdir and cd, so a lone - is the directory and not cd's OLDPWD shorthand; two tests cover - with OLDPWD unset and set.

make ci-local at 0880899: 30 passed, 0 failed, 7 skipped.

Not approving yet. Still open: the risk from my review; exai-bot on tag/branch name collision: validate checks the action out at the bare dokimi-ref, so a branch can win over the pinned tag.

dokimi-ref used to go to actions/checkout as is, so a branch named like a
tag could make the checkout run branch code. The validate job now resolves
the ref to a commit before the checkout: a 40-digit SHA passes through, a
tag resolves to its peeled commit, anything else is refused. The test runs
the resolver with a fake git that lists a tag and its peeled commit.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Comment thread .github/workflows/repo-eval.yml Outdated
id: dokimi
uses: ./.dokimi-action/actions/setup
with:
dokimi-ref: ${{ inputs.dokimi-ref }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
Must-fix: With both repository inputs set to the same repository and dokimi-ref: release, the action resolver can resolve the tag to commit A, then the tag can move to B before this setup call. The composite independently runs git ls-remote again, so validate executes action A but installs package B; downstream jobs preserve that initial mismatch. When the repositories are identical, pass the already-resolved action SHA here instead of the original tag. Retain separate package resolution for different repositories, and add a regression test for a tag moving between the two resolutions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1817460. When the dokimi and action repositories match, Validate now installs the commit the "Resolve the setup action commit" step resolved, so a second ls-remote of a tag that moved cannot pick another commit.
Tests: test_validate_installs_the_commit_it_resolved_when_the_repositories_match (repo-eval wiring) and the two setup-action resolution tests.
Local: 12052 passed, 28 skipped, 6 xfailed (full suite).

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…atch

validate resolved dokimi-ref for the action checkout, then the setup step
resolved the same tag again with its own ls-remote. A tag that moved in
between made validate run one commit and install another. With
dokimi-repository equal to dokimi-action-repository the setup step now gets
the resolved commit; a different package repository still resolves its own ref.

Tests: the wiring is pinned in test_repo_eval_action_checkout; a commit ref
is installed as given with no ls-remote, and a tag resolves to its peeled commit.
Comment block in repo-eval.yml cut to two lines (check_comment_blocks).

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Comment thread actions/setup/action.yml Outdated
Comment on lines +56 to +57
if [[ "$REF" =~ ^[0-9a-f]{40}$ ]]; then
sha=$REF

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
Must-fix: A valid 40-digit SHA containing uppercase hex letters (for example, BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB) falls through to the tag lookup and is rejected unless an identically named tag exists. The input contract says “40 hex digits,” without a lowercase restriction. Accept [0-9A-Fa-f] and normalize the accepted SHA to lowercase before exporting/installing it; add an uppercase or mixed-case regression case to _resolve_ref. The setup-action resolver in repo-eval.yml has the same restriction and needs the corresponding change for workflow callers.

Suggestion:

Suggested change
if [[ "$REF" =~ ^[0-9a-f]{40}$ ]]; then
sha=$REF
if [[ "$REF" =~ ^[0-9A-Fa-f]{40}$ ]]; then
sha=${REF,,}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed on this branch in c1f06d9 (antonwolfy): action.yml and the repo-eval.yml resolver accept [0-9A-Fa-f]{40} and lower-case it.
Regression: test_an_uppercase_commit_ref_is_lowercased; 1de901d adds the uppercase case to the repo-eval resolver test too.

@antonwolfy antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at 1817460. The tag-versus-branch hole is closed: the action is checked out at a commit resolved first, and a branch name can no longer win. Five points are left, four of them to fix before merge: three tests that leave the pinning lines unguarded, one doc gap that was open in my earlier review, and one nit.

What I ran, on a Linux review host at 1817460:

  • make ci-local: 30 passed, 0 failed, 7 skipped (the skips need a PR event, micromamba or semgrep). zizmor --offline --min-severity medium .github/workflows actions reports no findings.
  • Both resolvers and the out-dir step under bash -e -o pipefail with a fake git ls-remote: a branch with the same name as a tag, an annotated tag, a moved tag, -x, -, .., a trailing newline.
  • A mutation run on the two new test files: 77 mutants, 76 applied, 48 of which pass the full suite. The ones that matter are inline.
  • All 16 commits carry a Signed-off-by.

Findings outside the diff lines

  1. Nothing says dokimi-ref must contain actions/setup; this was open in my earlier review. The action is checked out at dokimi-ref (sparse /actions/) and run from .dokimi-action/actions/setup. With a dokimi-ref that predates this PR, the sparse checkout is empty and the job fails at "Set up dokimi" with a missing action.yml. The dokimi-ref input description (repo-eval.yml:30), the header comment (repo-eval.yml:14-18) and the contract (repo-evaluation-contract.md:1572) still say only that it selects the package. The PR description also still says an older dokimi-ref works and still lists paid, max-usd, cache, DOKIMI_RUNS_DIR and job.workflow_sha, none of which exist at this head. Suggestion: add one sentence to the input description and the contract ("dokimi-ref must contain actions/setup") and update the description.

Checked, no issues

  • Ref resolution: a ref main that is both a tag and a branch resolves to the tag's commit. An annotated tag resolves to its peeled commit. -x, an empty ref, a short SHA and a trailing newline are refused with exit 1.
  • Moved tag: the SHA is written once and every checkout and install downstream uses it. With one repository for both inputs, the setup call gets the already-resolved commit.
  • out-dir: empty, -reports, a lone -, a trailing slash and a newline in the name all behave as the tests say; the newline name is never created.
  • Checkout order: the caller's tree is checked out before .dokimi-action in all three jobs, so nothing removes the action before the post steps.
  • Wiring: actions/** and actions/ are in sync between paths: and smoke-paths; the zizmor step and hook cover actions/.
Bot points I checked that no longer apply at this head (16)
  • exai-bot's point on the action repository does not apply: the checkouts read it from dokimi-action-repository, independent of dokimi-repository.
  • exai-bot's point on job.workflow_sha does not apply: no checkout uses it any more.
  • exai-bot's point on the pip cache key file does not apply: the pip cache and its key file are removed.
  • exai-bot's point on DOKIMI_RUNS_DIR does not apply: the action no longer exports it.
  • exai-bot's point on DOKIMI_MAX_USD does not apply: the paid and max-usd inputs and the export are removed.
  • exai-bot's point on a moving tag between jobs does not apply: scan and tasks-check check the action out at the action-sha that validate records.
  • exai-bot's point on the trailing newline in out-dir does not apply: a newline is refused before mkdir, and the captured path keeps its trailing newline.
  • Copilot's point on a moving tag between jobs does not apply: both downstream checkouts use needs.validate.outputs.action-sha.
  • Copilot's point on the trailing newline does not apply: the capture keeps it and the guard refuses it.
  • exai-bot's point on every normal out-dir failing does not apply: the step writes a one-line dir= for the default path.
  • exai-bot's point on the pwd newline does not apply: the capture uses printf '%sx' "$PWD".
  • exai-bot's point on the newline test does not apply: the tests run the step script and assert a nonzero exit and no reports* directory.
  • exai-bot's point on a tag that shares a branch's name does not apply: validate resolves dokimi-ref to a commit with ls-remote --tags before the checkout, and a branch-only ref is refused.
  • exai-bot's point on out-dir: -reports does not apply: the step prefixes ./ and uses mkdir -p -- and cd --.
  • exai-bot's point on the action-sha test does not apply: it asserts the exact mapping and the exact recorder command.
  • exai-bot's point on a tag that moves between the action resolution and the install does not apply: with one repository for both inputs, validate passes the resolved commit to the setup action.

Comment thread .github/workflows/repo-eval.yml Resolved
Comment thread .github/workflows/repo-eval.yml Resolved
Comment thread actions/setup/action.yml Resolved
Comment thread actions/setup/action.yml Resolved
antonwolfy and others added 3 commits November 1, 2026 20:44
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
…ll step, uppercase resolver

- scan and tasks-check install the package commit validate gated (dokimi-sha), not dokimi-ref.
- Every actions/checkout in repo-eval.yml sets persist-credentials: false.
- The Install dokimi step installs the resolved SHA, with --require-hashes for the lock and --no-deps for dokimi.
- The repo-eval resolver lower-cases an uppercase 40-digit SHA, as action.yml does.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…uses the resolved SHA

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Comment thread actions/setup/action.yml Outdated
fi
if [[ "$REF" =~ ^[0-9A-Fa-f]{40}$ ]]; then
sha=${REF,,}
elif [[ "$REF" =~ ^[A-Za-z0-9][A-Za-z0-9._-]*$ ]]; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
Must-fix: dokimi-ref: releases/v1.0.0 is a valid Git release tag, but this regex rejects it before git ls-remote runs, so setup fails even when the tag exists in the selected repository. The new action promises release-tag support without restricting tags to a single path segment. Validate with git check-ref-format "refs/tags/$REF" (and retain the exact tag lookup), or otherwise accept valid namespaced tags, and add a regression test. The setup-action resolver in .github/workflows/repo-eval.yml has the same restriction, inherited from the previous implementation.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

@antonwolfy antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at caf19ac. Both resolvers, the install step and the new wiring tests are right for the cases they pin, and every earlier thread is fixed. Two things are left to fix before merge, one inline and one in the body. One nit inline.

What I ran, on a Linux review host at caf19ac:

  • make ci-local: 30 passed, 0 failed, 7 skipped (the skips need a PR event, micromamba or semgrep); changed-line coverage passes
  • Both resolvers under bash -e -o pipefail with a fake git ls-remote: uppercase and mixed-case SHA (lower-cased, no ls-remote call), 39 and 41 digits, a branch name and a non-hex 40-character string (refused with exit 1).
  • A mutation run on the two test files: 31 mutants. Two that matter pass the full suite and are listed below.
  • zizmor --offline --min-severity medium on .github/workflows and actions: no findings.
  • All 19 non-merge commits carry a Signed-off-by.

Findings outside the diff lines

  1. Nothing says dokimi-ref must contain actions/setup; this was open in my earlier review and is unchanged. Each job checks the action out at the resolved dokimi-ref (sparse /actions/, repo-eval.yml:156-164) and runs ./.dokimi-action/actions/setup. A dokimi-ref from before this PR gives an empty checkout and the job fails at "Set up dokimi" with a missing action.yml. The dokimi-ref input description (repo-eval.yml:30-34, "Use the ref of the uses: line"), the header comment (repo-eval.yml:14-18) and the contract (repo-evaluation-contract.md:1600-1610) still describe it only as the package to install. The PR description still says an older dokimi-ref works, lists paid, max-usd, cache, DOKIMI_RUNS_DIR and job.workflow_sha, none of which exist at this head, and says the repo-eval inputs are unchanged although dokimi-action-repository is new. Suggestion: add "dokimi-ref must contain actions/setup" to the input description and the contract, and update the PR description.

Checked, no issues

  • Resolvers: repo-eval.yml:142-143 and action.yml:56-57 give the same result for the same input, and the output is always 40 lower-case hex digits, so the downstream dokimi-sha and action-sha uses are safe.
  • Persist-credentials: the new test fails when any one of the six checkouts is set to true or loses the key (12 mutants, all caught).
  • Install step: dropping --require-hashes, dropping --no-deps, @main instead of @$SHA, a raw inputs.dokimi-ref all fail the new test.
Bot points I checked that no longer apply at this head (17)
  • exai-bot's point on the action repository does not apply: the checkouts read it from dokimi-action-repository, independent of dokimi-repository.
  • exai-bot's point on job.workflow_sha does not apply: no checkout uses it any more.
  • exai-bot's point on the pip cache key file does not apply: the pip cache and its key file are removed.
  • exai-bot's point on DOKIMI_RUNS_DIR does not apply: the action no longer exports it.
  • exai-bot's point on DOKIMI_MAX_USD does not apply: the paid and max-usd inputs and the export are removed.
  • exai-bot's point on a moving tag between jobs does not apply: scan and tasks-check check the action out at the action-sha that validate records.
  • exai-bot's point on the trailing newline in out-dir does not apply: a newline is refused before mkdir, and the captured path keeps its trailing newline.
  • Copilot's point on a moving tag between jobs does not apply: both downstream checkouts use needs.validate.outputs.action-sha.
  • Copilot's point on the trailing newline does not apply: the capture keeps it and the guard refuses it.
  • exai-bot's point on every normal out-dir failing does not apply: the step writes a one-line dir= for the default path.
  • exai-bot's point on the pwd newline does not apply: the capture uses printf '%sx' "$PWD".
  • exai-bot's point on the newline test does not apply: the tests run the step script and assert a nonzero exit and no reports* directory.
  • exai-bot's point on a tag that shares a branch's name does not apply: validate resolves dokimi-ref to a commit with ls-remote --tags before the checkout, and a branch-only ref is refused.
  • exai-bot's point on out-dir: -reports does not apply: the step prefixes ./ and uses mkdir -p -- and cd --.
  • exai-bot's point on the action-sha test does not apply: it asserts the exact mapping and the exact recorder command.
  • exai-bot's point on a tag that moves between the action resolution and the install does not apply: with one repository for both inputs, validate passes the resolved commit to the setup action.
  • exai-bot's point on an uppercase 40-digit SHA does not apply: both resolvers now accept it and lower-case it, and the tests pin it.

for job_id in ("scan", "tasks-check"):
setup = [s for s in workflow["jobs"][job_id]["steps"] if s.get("uses") == f"./{CHECKOUT_PATH}/actions/setup"]
assert len(setup) == 1, job_id
assert "validate" in _as_list(workflow["jobs"][job_id]["needs"]), job_id

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test does not pin where dokimi-sha comes from, so the package commit validate gated is not actually guarded. It asserts that scan and tasks-check read needs.validate.outputs.dokimi-sha, but not what that output is (repo-eval.yml:117).

Mutation check: changing dokimi-sha: ${{ steps.dokimi.outputs.sha }} to ${{ inputs.dokimi-ref }} passes both test files (24 passed). With a tag in dokimi-ref, scan and tasks-check then resolve the tag again, and a tag that moved installs a different dokimi than the one validate gated. Pointing it at steps.action-ref.outputs.sha also passes, and installs the action's commit when the two repositories differ.

Suggestion: in this test add assert workflow["jobs"]["validate"]["outputs"]["dokimi-sha"] == "${{ steps.dokimi.outputs.sha }}".

Comment thread actions/setup/action.yml Outdated
fi
if [[ "$REF" =~ ^[0-9A-Fa-f]{40}$ ]]; then
sha=${REF,,}
elif [[ "$REF" =~ ^[A-Za-z0-9][A-Za-z0-9._-]*$ ]]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: a tag with a slash, such as releases/v1.0.0, is refused before ls-remote runs. ^[A-Za-z0-9][A-Za-z0-9._-]*$ has no /, so the step ends at "neither a 40-digit commit SHA nor a tag" for a tag that exists. I agree with exai-bot on this. The resolver in repo-eval.yml:137 has the same pattern, and the PR's base has it too.

Suggestion: allow / after the first character, ^[A-Za-z0-9][A-Za-z0-9._/-]*$, in both places. The exact refs/tags/$REF match in the awk stays, and a leading - is still refused. Add a releases/v1 case to _resolve_ref.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Comment thread actions/setup/action.yml Outdated
fi
if [[ "$REF" =~ ^[0-9A-Fa-f]{40}$ ]]; then
sha=${REF,,}
elif [[ "$REF" =~ ^[A-Za-z0-9][A-Za-z0-9._/-]*$ ]]; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
Must-fix: dokimi-ref: v1.2.3+build.1 is a valid Git tag (and a SemVer version with build metadata), but this allowlist rejects it before git ls-remote runs, even when the tag exists. The input contract does not restrict release-tag characters. Accept + and add a fake-remote regression case. The modified setup-action resolver in .github/workflows/repo-eval.yml has the same restriction and needs the corresponding update.

Suggestion:

Suggested change
elif [[ "$REF" =~ ^[A-Za-z0-9][A-Za-z0-9._/-]*$ ]]; then
elif [[ "$REF" =~ ^[A-Za-z0-9][A-Za-z0-9._/+_-]*$ ]]; then

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
# Without the resolved SHA a moved tag installs another commit; without --require-hashes the lock is not verified.
install = next(s for s in _steps() if s.get("name") == "Install dokimi")
assert install["env"]["SHA"] == "${{ steps.ref.outputs.sha }}"
assert "pip install --require-hashes -r" in install["run"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test · medium
Must-fix: This test does not guard which revision supplies the dependency lock. Changing the lock URL in the action from /$SHA/dokimi/requirements.lock.txt to /main/dokimi/requirements.lock.txt leaves every assertion here passing, although setup would then install dependencies from a different revision than the pinned package. Assert the complete lock URL (including $DOKIMI_REPO and $SHA), or execute the install script with a fake pip and check both recorded arguments.

Suggestion:

Suggested change
assert "pip install --require-hashes -r" in install["run"]
assert 'pip install --require-hashes -r "https://raw.githubusercontent.com/$DOKIMI_REPO/$SHA/dokimi/requirements.lock.txt"' in install["run"]

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants