Skip to content

NO-ISSUE: Fix miscellaneous issues in dev.py tooling#681

Open
jhernand wants to merge 1 commit into
osac-project:mainfrom
jhernand:fix_misc_dev_py_issues
Open

NO-ISSUE: Fix miscellaneous issues in dev.py tooling#681
jhernand wants to merge 1 commit into
osac-project:mainfrom
jhernand:fix_misc_dev_py_issues

Conversation

@jhernand

@jhernand jhernand commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Escape regex metacharacters in dev/setup.py artifact name lookup so dots in
    filenames like golangci-lint-2.12.2-linux-amd64.tar.gz are matched literally
    rather than as wildcards.
  • Add explicit return False to is_installed when the tool is not found on
    PATH, satisfying the -> bool annotation on all code paths.
  • Document the binary download security posture in AGENTS.md (two-stage
    SHA-256 verification, failure behaviour, trust boundary).

Test plan

  • uv run dev.py setup still downloads and installs golangci-lint correctly.
  • python -c "import dev.setup" succeeds without errors.
  • CI passes (lint, unit tests).

Summary by CodeRabbit

  • Documentation

    • Expanded guidance on how the setup command verifies downloaded tool binaries through checksum validation.
  • Bug Fixes

    • Improved tool version detection reliability during installation.
    • Enhanced checksum verification process for downloaded artifacts.

These are issues that were reported by the AI code review tool but were
not addressed before merging the commit that added `dev.py`. The changes
are:

In `dev/setup.py`, use `re.escape()` on the artifact name before
interpolating it into the regex pattern that searches the checksums
file. Without this, dots in filenames like `golangci-lint-2.12.2-
linux-amd64.tar.gz` act as regex wildcards and could match unintended
lines.

In `dev/setup.py`, add an explicit `return False` at the end of
`is_installed` for the code path where `shutil.which` returns `None`.
Previously the function fell through to an implicit `None` return,
violating its `-> bool` annotation.

In `AGENTS.md`, document the security posture of the binary download
verification performed by the `setup` command, including the two-stage
SHA-256 check, the failure behaviour, and the trust boundary.

Assisted-by: Cursor
Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
@openshift-ci-robot

Copy link
Copy Markdown

@jhernand: This pull request explicitly references no jira issue.

Details

In response to this:

Summary

  • Escape regex metacharacters in dev/setup.py artifact name lookup so dots in
    filenames like golangci-lint-2.12.2-linux-amd64.tar.gz are matched literally
    rather than as wildcards.
  • Add explicit return False to is_installed when the tool is not found on
    PATH, satisfying the -> bool annotation on all code paths.
  • Document the binary download security posture in AGENTS.md (two-stage
    SHA-256 verification, failure behaviour, trust boundary).

Test plan

  • uv run dev.py setup still downloads and installs golangci-lint correctly.
  • python -c "import dev.setup" succeeds without errors.
  • CI passes (lint, unit tests).

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci

openshift-ci Bot commented Jun 12, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhernand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 297a5b76-2c02-4c20-9b2c-dcae01a44285

📥 Commits

Reviewing files that changed from the base of the PR and between c385789 and fc4ffca.

📒 Files selected for processing (2)
  • AGENTS.md
  • dev/setup.py

Walkthrough

This PR hardens tool verification logic in the development setup pipeline by escaping regex patterns in checksum parsing to mitigate injection risk, adding explicit return-value handling in version detection, and documenting the two-stage SHA-256 verification process.

Changes

Tool Verification Improvements

Layer / File(s) Summary
Checksum parsing and version-check verification
dev/setup.py
Checksum regex matching now uses pre-escaped artifact patterns to prevent regex injection risk, and version-mismatch detection explicitly returns False instead of falling through without a clear return value.
Verification process documentation
AGENTS.md
Documentation describes the setup command's two-stage SHA-256 verification: manifest checksum comparison against dev/tools.py, then downloaded artifact hash validation, with failure behavior and guidance for updating tool versions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

  • adriengentil
  • rgolangh

Poem

🔐 Checksums align with care,
Regexes properly escaped from snares,
Returns explicit, clean and true,
Two-stage hashes verify anew.

🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'NO-ISSUE: Fix miscellaneous issues in dev.py tooling' is vague and uses the generic term 'miscellaneous issues,' failing to convey the specific security-focused changes (regex escaping for artifact matching, explicit return statements, binary verification documentation). Revise the title to be more specific, such as 'Fix regex escaping and binary verification in dev.py tooling' or 'Add explicit return values and document binary checksum verification' to clearly indicate the primary technical changes.
✅ Passed checks (10 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets ✅ Passed PR #681 modifies only AGENTS.md and dev/setup.py; no embedded-credential URLs, private-key blocks, or api_key/secret/token/password string literals found (only a path matched a loose base64 pattern).
No-Weak-Crypto ✅ Passed No weak crypto detected in PR #681: scans show only SHA-256 hashing and no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or custom crypto/non-constant secret compares.
No-Injection-Vectors ✅ Passed PR changes are limited to dev/setup.py/AGENTS.md; dev/setup.py avoids shell=True/os.system/sql concat and doesn’t use Python eval/exec/pickle/yaml.load. commands.eval is a safe subprocess wrapper.
Container-Privileges ✅ Passed Security scan found 0 occurrences of privileged:true, hostPID/hostNetwork/hostIPC, SYS_ADMIN, allowPrivilegeEscalation:true, or runAsUser:0 in charts/ and manifests/ YAML/templating files.
No-Sensitive-Data-In-Logs ✅ Passed dev/setup.py logging only records tool name/version, installed path, and artifact checksum; no tokens/passwords/PII/internal hostnames found. AGENTS.md contains no logs.
Ai-Attribution ✅ Passed PASS: PR/commit message includes Red Hat-style trailer 'Assisted-by: Cursor' and no 'Co-Authored-By' for AI; thus attribution is present (risk: low audit/compliance impact).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci

openshift-ci Bot commented Jun 12, 2026

Copy link
Copy Markdown

@jhernand: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/unit fc4ffca link true /test unit
ci/prow/images fc4ffca link true /test images
ci/prow/e2e-vmaas fc4ffca link true /test e2e-vmaas

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@jhernand jhernand requested a review from sk-ilya June 12, 2026 08:41
@openshift-ci

openshift-ci Bot commented Jun 16, 2026

Copy link
Copy Markdown

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants