Skip to content

fix: Migrate Ruff action to astral-sh/ruff-action@v3 - #107

Merged
m0hamed-ait merged 1 commit into
mainfrom
chore/migrate-ruff-action-v3
Sep 8, 2026
Merged

m0hamed-ait merged 1 commit into
mainfrom
chore/migrate-ruff-action-v3

Conversation

@m0hamed-ait

@m0hamed-ait m0hamed-ait commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary of Changes

Migrating the Ruff GitHub Action from chartboost/ruff-action to astral-sh/ruff-action@v3 pinned to version 0.16.0.

  • Replaced chartboost/ruff-action@v1 with astral-sh/ruff-action@v3 pinned to version: 0.16.0.
  • Reconciled workflow inputs and dropped legacy chartboost-only parameters.
  • Applied automated lint/import formatting fixes.

@ostorlab-ai-pr-review

ostorlab-ai-pr-review Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

📊 PR Complexity Assessment: Simple (Score: 1/5)

Single-file, CI-only change replacing chartboost/ruff-action@v1 with the official astral-sh/ruff-action in lint_format_checker.yaml, pinned to commit SHA 4919ec5 (v3.6.1) with ruff version 0.16.0, applied identically to the 'Checking code formatting' and 'Running linter' steps. No application code, public API, schema, or infrastructure behavior is affected; the blast radius is limited to the lint/format CI job (worst case: pipeline fails or enforces different lint rules). The SHA pin is actually a supply-chain security improvement over the mutable @v1 tag. Cognitive load is minimal — review reduces to verifying SHA↔release consistency, input compatibility of the new action, and that pinned ruff 0.16.0 does not fail format/lint checks on the existing codebase. Auto-merge is withheld only because a CI/CD workflow file (sensitive path) is modified.

🔍 Complexity Drivers

  • Third-party CI action dependency swap (chartboost/ruff-action@v1 → astral-sh/ruff-action@v3)
  • Dual version pinning (action SHA = v3.6.1 vs ruff tool = 0.16.0) creates a small consistency/verification surface
  • Ruff version bump may enforce new lint/format rules, potentially breaking CI on existing code
  • PR description claims 'automated lint/import formatting fixes' that do not appear in the diff (minor description/diff mismatch)

🎯 Suggested Attention Areas

  • .github/workflows/lint_format_checker.yaml (Action SHA pin integrity): Confirm commit SHA 4919ec5cf1f49eff0871dbcea0da843445b837e6 genuinely resolves to the official astral-sh/ruff-action v3.6.1 release (cross-check against upstream GitHub tags/releases). A mistyped or spoofed SHA would silently execute untrusted CI code; the SHA pin is the core security hardening of this PR, so it must be exact.
  • .github/workflows/lint_format_checker.yaml (Input compatibility with astral-sh/ruff-action@v3): Verify the 'version' input is a supported input of astral-sh/ruff-action@v3 (not a chartboost-only parameter) and that 'args: format --check' retains identical semantics under the new action. Confirm nothing in the job relied on chartboost-specific defaults (e.g., src, working-directory, tool resolution) that are now missing after 'reconciling workflow inputs'.
  • .github/workflows/lint_format_checker.yaml (Ruff 0.16.0 behavior parity in CI): Assess whether pinning ruff to 0.16.0 changes effective lint/format results versus the previously effective version (chartboost v1 default or repo-configured ruff). New formatter rules or added lints can fail the job on unmodified code; confirm the workflow passes on the PR head and note any pre-existing violations the new version surfaces.
  • .github/workflows/lint_format_checker.yaml (Description vs. diff mismatch): The PR description claims 'automated lint/import formatting fixes', but the changed-files summary and diff show only this workflow file. Confirm no source-code changes are hidden elsewhere in the PR (or that the description is stale); if formatting fixes do exist they should be listed and reviewed explicitly rather than riding along with a CI change.

⚠️ Sensitive Paths Touched

  • .github/workflows/lint_format_checker.yaml

⚡ Auto-Merge Assessment

❌ Not recommended for auto-merge — manual peer review required.

💡 PR Decomposition Recommendation

null

@ostorlab-ai-pr-review ostorlab-ai-pr-review Bot 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.

Code Review Summary

Total Issues Found: 12 (0 critical, 0 security)

  • Bugs (7): Committed test-run artifacts
  • Suggestions (5): Import-style violations

Bugs — committed test-run artifacts (7 files)

requirements.txt, go.mod, pom.xml, packages.lock.json, gradle.lockfile, buildscript-gradle.lockfile, and verification-metadata.xml are placeholder outputs written into the working directory by _run_osv() during test runs. None are valid files in their respective formats, so they break pip, Go tooling/dependency graph, Maven, NuGet restore, Gradle dependency locking/verification, and Dependabot. All seven must be deleted from this commit (and ideally _run_osv() should use a temp directory so this cannot recur).

Suggestions — import-style violations (5 locations)

Grouped parenthesized imports from the same module violate the project's coding convention in agent/api_manager/osv_service_api.py (L10), agent/cve_service_api.py (L8), agent/osv_agent.py (L20 and L30), and tests/osv_agent_test.py (L12). Each should be converted to single-line imports.

Overall

No security vulnerabilities or critical bugs were found in the feature code itself (osv_service_api, cve_service_api, osv_agent, tests). The main blocker is commit hygiene: the seven bogus ecosystem files actively break builds and dependency tooling for unrelated ecosystems and must be removed before merge; the import-style fixes are trivial mechanical changes.

Comment thread requirements.txt Outdated
Comment thread go.mod Outdated
Comment thread pom.xml Outdated
Comment thread packages.lock.json Outdated
Comment thread gradle.lockfile Outdated
Comment thread agent/api_manager/osv_service_api.py Outdated
Comment thread agent/cve_service_api.py Outdated
Comment thread agent/osv_agent.py Outdated
Comment thread agent/osv_agent.py Outdated
Comment thread tests/osv_agent_test.py Outdated

@Moonflower1911 Moonflower1911 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@ostorlab-ai-pr-review

Copy link
Copy Markdown

Follow-up review — root cause of the 7 artifact files confirmed (verified, not assumed)

I investigated the origin of requirements.txt, go.mod, pom.xml, packages.lock.json, gradle.lockfile, buildscript-gradle.lockfile, and verification-metadata.xml instead of leaving it as a hypothesis:

  • In agent/osv_agent.py, _run_osv() writes the scanned content with a relative path (file_name = pathlib.Path(patched_path) followed by file_name.write_text(...)), so the file is created in the process working directory.
  • The artifact contents match the test fixtures exactly:
    • requirements.txt contains some file content, which is precisely the content of the message in testAgentOSV_whenIosMetadataWithPackageName_prepareVulnerabilityLocation (path /tmp/path/requirements.txt).
    • The other six files contain ostorlab, which is the mocked download content (requests_mock.get(..., content=b"ostorlab")) that the content-url tests brute-force across the names listed in SUPPORTED_OSV_FILE_NAMES — exactly go.mod, pom.xml, packages.lock.json, gradle.lockfile, buildscript-gradle.lockfile, and verification-metadata.xml.

Conclusion: these files were generated by running the test suite from the repository root, not added intentionally. Deleting them (as requested in the earlier inline review comments) is the correct fix for this PR.

To prevent recurrence, a follow-up change to _run_osv() is worth making (outside this PR's scope): write the patched file into a temporary directory (e.g., tempfile.TemporaryDirectory() or pytest's tmp_path equivalent) and pass that path to the osv-scanner commands instead of relying on the CWD. That eliminates this class of working-directory side effect entirely.

For completeness, the substantive migration itself is sound and needs no change:

  • astral-sh/ruff-action@v3 supports the version input, so pinning 0.16.0 is valid.
  • The "Running linter." step without explicit args keeps the previous default behavior (ruff check), matching the old chartboost action's default.
  • I checked for conflicting pins: pyproject.toml and .pre-commit-config.yaml do not exist in this repository, and I found no other workflow referencing chartboost/ruff-action, so there is no version drift and the migration is complete.

@ostorlab-ai-pr-review ostorlab-ai-pr-review Bot 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.

Code review complete: no issues found. The changes are clean, well-structured, and adhere to established quality guidelines — no critical bugs, security vulnerabilities, or improvement suggestions were identified. The PR is ready to merge as planned. (Prior inline findings from earlier review rounds remain documented in their existing threads.)

@ostorlab-ai-pr-review

Copy link
Copy Markdown

Follow-up — cleanup verified and migration re-checked against the current head (8627ef6):

✅ Artifact files resolved: The PR diff now contains only .github/workflows/lint_format_checker.yaml. All seven accidental test-run artifacts flagged earlier (requirements.txt, go.mod, pom.xml, packages.lock.json, gradle.lockfile, buildscript-gradle.lockfile, verification-metadata.xml) and the import-reformatting changes are no longer part of this PR.

✅ Ruff migration confirmed sound:

  • astral-sh/ruff-action@v3 supports the version input, so pinning version: 0.16.0 is valid.
  • The "Running linter." step without explicit args keeps the action's default (ruff check), matching the previous chartboost behavior — the migration is behavior-preserving.
  • No conflicting Ruff pins exist anywhere in the repo: pyproject.toml and .pre-commit-config.yaml do not exist, requirement.txt has no ruff entry, and no other workflow references chartboost/ruff-action.
  • Pinning the action to a commit SHA is good supply-chain practice.

No further changes needed — the migration is complete and self-consistent.

@ostorlab-ai-pr-review ostorlab-ai-pr-review Bot 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.

Review complete — no issues found. ✅

The PR migrates the lint/format CI job from chartboost/ruff-action@v1 to the official astral-sh/ruff-action, pinned to commit SHA with ruff version 0.16.0. All previously flagged items have been resolved:

  • All seven accidental test-run artifact files have been removed from the PR.
  • The action SHA pin is good supply-chain practice, and the version input is supported by astral-sh/ruff-action@v3.
  • The linter step preserves the previous default behavior (ruff check), making the migration behavior-preserving.
  • No conflicting Ruff pins exist elsewhere in the repo.

The changes are clean, well-scoped, and ready to merge as-is.

@m0hamed-ait
m0hamed-ait merged commit 62a366d into main Sep 8, 2026
5 checks passed
@m0hamed-ait
m0hamed-ait deleted the chore/migrate-ruff-action-v3 branch September 8, 2026 17:30
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.

3 participants