Repository navigation
[INT-3877] Add CI quality gates, lint, and security checks - #69
Conversation
Use .get() for the optional error and request fields. A payload without them now raises the intended exception (HttpError, ValidationError, ...) instead of KeyError. Fixes #30. Re-applied from PR #54 after the git history rewrite. Co-authored-by: wooooodward <25610026+wooooodward@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UmWS7B7g3HKpoenKRaGnNu
- Add CI workflow on push and pull_request to master: lint (ruff), pip-audit, full-history gitleaks scan, and an offline test job on a Python 3.10/3.11/3.12 matrix. The live-API suite runs on push to master only, since Actions secrets are not exposed to fork PRs. - Add minimal ruff config and clean the lint baseline (import order, re-exports in __all__). - Delete the dead .travis.yml. - pypi-publish.yml: bump actions/checkout and actions/setup-python, add a test job, and make publish require it (resolves the TODO). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UmWS7B7g3HKpoenKRaGnNu
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Fail test-live and the release test job when ONFLEET_API_KEY is absent, instead of passing green with every test skipped. - Verify the gitleaks tarball against its published SHA-256 before execution. - Cancel in-progress runs on pull_request only; a cancelled live run would leave test resources behind. - Run the release test job on a single Python version; concurrent live suites race each other in the shared test organization. - Pin pip-audit and build versions. - Enforce the 120-column limit (E5) and wrap the long lines. - Use .get() for the inner message field too, so a payload without it degrades to HttpError instead of KeyError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UmWS7B7g3HKpoenKRaGnNu
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The offline matrix ran zero assertions on PRs: without the secret, every live-API test skips. Add unit tests for the Request URL helpers, response handling, and error classification, including regression tests for the missing-field fix from PR #54. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UmWS7B7g3HKpoenKRaGnNu
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- Assert the exception with assertRaises so the regression test cannot pass silently when no exception is raised. - Remove the internal ticket reference from the public CI error message, per the repository PR template rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UmWS7B7g3HKpoenKRaGnNu
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The auth/test message no longer embeds the organization ID, so the regex cross-check crashed with AttributeError. Assert successful authentication and the organization ID shape instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UmWS7B7g3HKpoenKRaGnNu
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UmWS7B7g3HKpoenKRaGnNu
This reverts commit 35b7260.
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
- Drop paths-ignore: required checks and paths-ignore do not compose; doc-only PRs would be stuck once branch protection lands (step 12) - Gate test-live on the LIVE_SUITE_ENABLED repository variable so master stays green until the ONFLEET_API_KEY secret exists - Extract the live suite into a reusable workflow (live-suite.yml) called from ci.yml and pypi-publish.yml - Install the built wheel in the import smoke test instead of the source tree - Drop the redundant ci- prefix from the concurrency group - Add timeout-minutes to the publish and notify jobs - request.py: raise HttpError for malformed error payloads (non-JSON body, missing or non-dict top-level message), with regression tests - Add the missing trailing newline in onfleet/__init__.py Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
- Import the smoke-test wheel from outside the repo root, where the onfleet/ source directory shadows the installed package - Serialize live-suite runs across callers with a shared job-level concurrency group in the reusable workflow Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Adds CI quality gates (lint, audit, secret scan, offline/live tests), updates the PyPI publish workflow to require tests, removes dead Travis config, and fixes error payload parsing with regression tests; bounded tooling plus focused bug fix.
Re-trigger cubic
izzyandrade
left a comment
izzyandrade
left a comment
There was a problem hiding this comment.
Code review · round 1
Reviewed INT-3877-ci against master with the PR description as ticket context (INT-3877 itself was not reachable from the review environment). All CI jobs on the PR are green. The offline suite was re-run locally (26 tests, 5 skipped, OK), and the two bug findings below were reproduced on the branch.
No blocking findings. 4 suggestions and 6 neats. Posted as a comment because more than two suggestions are open.
| Finding | Severity | Type | Where |
|---|---|---|---|
| Star import now shadows the builtin PermissionError | Suggestion | bug (potential) | onfleet/__init__.py |
| Null or string error codes still raise TypeError | Suggestion | bug (potential) | onfleet/request.py |
| Dead import hack in the live test file drives the ruff ignores | Suggestion | code quality improvement | test/test_onfleet.py, ruff.toml |
| Releases are hard blocked until the API key secret exists | Suggestion | release process | .github/workflows/pypi-publish.yml |
| CHANGELOG Unreleased section not updated | Neat | docs | CHANGELOG.md |
| CI tool pins sit outside Dependabot and gitleaks is stale | Neat | dependency maintenance | .github/workflows/ci.yml |
| Publish workflow has no permissions block | Neat | security | .github/workflows/pypi-publish.yml |
| Publish workflow housekeeping is inconsistent | Neat | code quality improvement | .github/workflows/pypi-publish.yml |
| HttpError status field mixes HTTP status and Onfleet error code | Neat | code quality improvement | onfleet/request.py |
| PR description mentions a paths ignore the workflow does not have | Neat | docs | .github/workflows/ci.yml |
Details and fixes are in the inline comments, except for one finding on a file this PR does not touch:
CHANGELOG Unreleased section not updated (Neat · docs · found in round 1). The project follows Keep a Changelog, and the git history shows entries are written at release time, so no change is needed in this PR. Reminder for whoever prepares the next release: list the #30 fix under Fixed, and the public export change if the wider __all__ stays.
Findings drafted with Claude Code (claude-fable-5-1) and screened by the reviewer before posting.
- Revert __all__ to ["Onfleet"]: a star import must not shadow the builtin PermissionError; ignore F401 for the re-export module - Guard non-int error codes in request.py so null or string codes raise HttpError instead of TypeError, with regression tests - Delete the dead sys.path hack in test_onfleet.py and drop the test/* ruff carve-out - Bump gitleaks to 8.30.1 (checksum verified against the release asset); track GitHub Actions refs in dependabot - pypi-publish: add permissions contents:read, pin build==1.6.1, set Python 3.12, leave a TODO for the slack-action bump - Document the status field semantics on _OnfleetError Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 9 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would require human review. Adds CI quality gates, release test gate, and error handling fixes; however, the unannounced change to __all__ in onfleet/__init__.py alters star-import behavior and requires human review.
Re-trigger cubic
The non-int guard routed a float code such as 1000.0 to the generic HttpError. The range checks compare floats fine, so widen the guard to (int, float) and keep the classification master had. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Adds CI quality gates (lint, audit, secret scan, multi-version tests), fixes error handling for malformed API payloads, and removes Travis; all changes are bounded dev/tooling or bug fixes with tests.
Re-trigger cubic
2 participants
Summary
Implements the remaining code items of INT-3877 (steps 6–11 and 13).
New CI workflow (
ci.yml) onpush/pull_requesttomaster, withpermissions: contents: read, concurrency cancellation, 10-minute timeouts:lint— ruff with a minimal pinned config (ruff.toml); baseline cleaned in this PRaudit—pip-auditoverrequirements.txt(note: pip-audit has no severity filter, so it fails on any known vulnerability — stricter than the "high severity" gate the ticket asks for)secret-scan— full-history gitleaks scan (pinned CLI instead ofgitleaks-action, which requires a paid license for org repos)test-offline— Python 3.10/3.11/3.12 matrix: build, import smoke test, unittest (live-API tests skip without the secret)test-live— push tomasteronly, uses theONFLEET_API_KEYActions secret (fork PRs never see it). Disabled until theLIVE_SUITE_ENABLEDrepository variable is set totrue(pending new account access). The suite lives in a reusable workflow (live-suite.yml) shared with the release gateHousekeeping (step 11): deleted dead
.travis.yml; bumpedactions/checkoutv2→v4 andactions/setup-pythonv2→v5 inpypi-publish.yml; added atestjob thatpublishnow requires (resolves the old TODO).Error handling fix (step 13, re-applied from #54 with credit to @wooooodward):
onfleet/request.pyuses.get()for optionalerror/requestfields, so malformed error payloads raise the intended exception instead ofKeyError. Fixes #30.Release gate: releases are blocked until the
ONFLEET_API_KEYActions secret is set. Thetestjob inpypi-publish.ymlfails loudly by design — a release must never skip tests.Not in this PR (admin settings)
masterrequiring these checks (step 12)Test plan
ruff check .cleanpip-audit -r requirements.txt— no known vulnerabilitiespython -m unittest discover -s testwithoutONFLEET_API_KEY— 5 skipped, exit 0python -m buildsucceeds🤖 Generated with Claude Code
https://claude.ai/code/session_01UmWS7B7g3HKpoenKRaGnNu