chore: release pipeline, testing helper, docs, dev-loop hardening#2
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (23)
📝 WalkthroughWalkthroughAdds ChangesFakeSnowflake testing module and client
Repository infrastructure, CI/CD, tooling, and user-facing docs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
.github/workflows/publish.yml (2)
12-29: ⚡ Quick winRestrict permissions for lint and test jobs to read-only.
The
lintandtestjobs inherit default write permissions but only need read access to the repository. Explicitly settingpermissions: contents: readfollows the principle of least privilege.🛡️ Proposed fix
Add to both jobs:
lint: name: Lint and type-check runs-on: ubuntu-latest + permissions: + contents: read steps:test: name: Test (py${{ matrix.python-version }}) runs-on: ubuntu-latest + permissions: + contents: read strategy:Also applies to: 31-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish.yml around lines 12 - 29, The lint and test GitHub Actions jobs currently inherit default write permissions but should be restricted to read-only access following the principle of least privilege. Add a permissions section with contents set to read to both the lint job (starting with name: Lint and type-check) and the test job (mentioned in the Also applies to section). This explicitly grants only read access to the repository content, which is all that is needed for linting, type-checking, and testing operations.
16-17: ⚖️ Poor tradeoffConsider pinning GitHub Actions to commit SHAs for supply-chain security.
Using version tags like
@v4or@v5allows action maintainers to update the code behind the tag, which could introduce malicious changes. Pinning to commit SHAs (with a comment showing the version) prevents this attack vector.🔐 Example: Pin actions/checkout@v4 to SHA
- - uses: actions/checkout@v4 + - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4Note: This is a defense-in-depth measure; assess whether it fits the project's risk tolerance. Tools like Dependabot can automate SHA updates.
Also applies to: 39-40, 61-65, 77-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish.yml around lines 16 - 17, The GitHub Actions in the publish.yml workflow are using version tags like `@v4` and `@v5` for actions/checkout and actions/setup-python, which poses a supply-chain security risk since maintainers can update the code behind these tags. Replace each version tag with a pinned commit SHA (format: `@COMMIT_SHA`) and add a comment above each line indicating the version it corresponds to (e.g., # v4, # v5). Apply this fix to all instances of GitHub Actions throughout the file, including the ones mentioned in lines 39-40, 61-65, and 77-77.CONTRIBUTING.md (1)
43-43: ⚡ Quick winChecklist item omits
timezoneandschemafrom hardcoding guidance.Line 43 lists only "account/region/role/warehouse"; per the coding guidelines and
AGENTS.md(line 87–89), the full set is "account, region, role, warehouse, timezone, schema". Developers reading onlyCONTRIBUTING.mdbefore opening a PR might miss the latter two. Consider adding them for consistency.✨ Proposed fix
- [ ] No hardcoded account/region/role/warehouse values - configuration is generic. + [ ] No hardcoded account/region/role/warehouse/timezone/schema values - configuration is generic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTRIBUTING.md` at line 43, The checklist item for hardcoded configuration values on line 43 is incomplete and should include all configuration items per the AGENTS.md guidelines. Update the checkbox item that currently reads "No hardcoded account/region/role/warehouse values" to also explicitly include timezone and schema in the list of prohibited hardcoded values, ensuring consistency with the full set of configuration items documented in AGENTS.md lines 87-89.Source: Coding guidelines
docs/getting-started.md (1)
99-99: 💤 Low valueMinor style: consider "username" instead of "user name".
Per static analysis, the single-word form is now more common.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/getting-started.md` at line 99, In the documentation table entry for the SNOWFLAKE_USER environment variable, change the description from "user name (required)" to "username (required)" to align with modern style conventions where "username" is now the standard single-word form.Source: Linters/SAST tools
docs/testing.md (1)
20-25: ⚡ Quick winUse a context manager in quick-start to avoid leaking client resources.
The first example should demonstrate closing the client cleanly.
✍️ Suggested doc edit
-client = make_client(fake) -assert client.query("SELECT id, name FROM users") == [ - {"ID": 1, "NAME": "alice"}, - {"ID": 2, "NAME": "bob"}, -] +with make_client(fake) as client: + assert client.query("SELECT id, name FROM users") == [ + {"ID": 1, "NAME": "alice"}, + {"ID": 2, "NAME": "bob"}, + ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/testing.md` around lines 20 - 25, The quick-start example in the testing documentation creates a client using make_client(fake) but does not demonstrate proper resource cleanup, which can lead to resource leaks. Refactor the code example to use a context manager (with statement) to wrap the client creation and query execution, ensuring the client is automatically closed after the example completes. This demonstrates best practices for resource management in the documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/publish.yml:
- Line 16: The checkout actions in the publish workflow are missing the
persist-credentials configuration, which leaves git credentials in .git/config
and poses a security risk. Add persist-credentials: false to all three instances
of the actions/checkout@v4 action in the workflow file to prevent credential
leakage from potentially compromised dependencies or build steps that run during
publishing.
In `@snowflake_sql_api/testing.py`:
- Around line 576-577: Remove the hardcoded default values for the account and
user parameters in the factory function. The account parameter currently
defaults to "testorg-testaccount" and the user parameter currently defaults to
"TEST_USER" at lines 576-577 and again at lines 600-601. Instead, make these
parameters required by removing their default values, or alternatively source
these values from environment variables or configuration objects passed as
parameters. This ensures the public factory API does not contain hardcoded
sensitive configuration values as per the coding guidelines.
- Around line 579-594: The function accepts **kwargs documented as pass-through
to the SnowflakeClient constructor, but explicitly sets private_key,
http_client, and poll_interval as named arguments. If these reserved keywords
are passed in **kwargs, it will cause a duplicate keyword argument TypeError.
Add validation before the SnowflakeClient instantiation to check if private_key,
http_client, or poll_interval are present in the kwargs dictionary, and raise a
clear error message explaining that these parameters cannot be passed through
**kwargs as they are managed by the factory function. Apply the same guard to
the similar function at lines 603-615.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 12-29: The lint and test GitHub Actions jobs currently inherit
default write permissions but should be restricted to read-only access following
the principle of least privilege. Add a permissions section with contents set to
read to both the lint job (starting with name: Lint and type-check) and the test
job (mentioned in the Also applies to section). This explicitly grants only read
access to the repository content, which is all that is needed for linting,
type-checking, and testing operations.
- Around line 16-17: The GitHub Actions in the publish.yml workflow are using
version tags like `@v4` and `@v5` for actions/checkout and actions/setup-python,
which poses a supply-chain security risk since maintainers can update the code
behind these tags. Replace each version tag with a pinned commit SHA (format:
`@COMMIT_SHA`) and add a comment above each line indicating the version it
corresponds to (e.g., # v4, # v5). Apply this fix to all instances of GitHub
Actions throughout the file, including the ones mentioned in lines 39-40, 61-65,
and 77-77.
In `@CONTRIBUTING.md`:
- Line 43: The checklist item for hardcoded configuration values on line 43 is
incomplete and should include all configuration items per the AGENTS.md
guidelines. Update the checkbox item that currently reads "No hardcoded
account/region/role/warehouse values" to also explicitly include timezone and
schema in the list of prohibited hardcoded values, ensuring consistency with the
full set of configuration items documented in AGENTS.md lines 87-89.
In `@docs/getting-started.md`:
- Line 99: In the documentation table entry for the SNOWFLAKE_USER environment
variable, change the description from "user name (required)" to "username
(required)" to align with modern style conventions where "username" is now the
standard single-word form.
In `@docs/testing.md`:
- Around line 20-25: The quick-start example in the testing documentation
creates a client using make_client(fake) but does not demonstrate proper
resource cleanup, which can lead to resource leaks. Refactor the code example to
use a context manager (with statement) to wrap the client creation and query
execution, ensuring the client is automatically closed after the example
completes. This demonstrates best practices for resource management in the
documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5af72f03-4468-4076-bfdb-c49fd3b529fd
📒 Files selected for processing (23)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/PULL_REQUEST_TEMPLATE.md.github/dependabot.yml.github/workflows/ci.yml.github/workflows/publish.yml.pre-commit-config.yamlAGENTS.mdCONTRIBUTING.mdRELEASING.mdSECURITY.mddocs/authentication.mddocs/cli.mddocs/getting-started.mddocs/testing.mddocs/troubleshooting.mdpyproject.tomlsnowflake_sql_api/__init__.pysnowflake_sql_api/aclient.pysnowflake_sql_api/client.pysnowflake_sql_api/testing.pytests/support.pytests/test_testing.py
- Add a tag-driven PyPI release path: version from the git tag via hatch-vcs, plus a publish.yml workflow that builds and publishes through OIDC trusted publishing (no API token). Documented in RELEASING.md. - Ship snowflake_sql_api.testing so downstream code can test against the client with no network and no Snowflake account: a FakeSnowflake (httpx.MockTransport) registry, make_client/make_async_client, and pytest fixtures auto-registered via a pytest11 entry point. Adds an http_client= passthrough on both clients (no new runtime dependency; httpx is already core). - Enforce the coverage gate that was previously only a comment (fail_under = 89), and run coverage via `coverage run -m pytest` so tracing starts before the shipped pytest plugin imports. - Add .pre-commit-config.yaml (ruff, black, mypy, detect-private-key) and wire it into the contributor flow. - Add user docs under docs/ (getting-started, authentication, cli, testing, troubleshooting) and supply-chain hygiene: SECURITY.md, Dependabot for pip and GitHub Actions, and issue/PR templates.
hampsterx
force-pushed
the
chore/maturity-hardening
branch
from
f34da9d to
d2a6abc
Compare
1 participant
Summary
hatch-vcs, plus apublish.ymlworkflow that builds and publishes through OIDC trusted publishing (no API token). Documented inRELEASING.md.snowflake_sql_api.testingso downstream code can test against the client with no network and no Snowflake account: aFakeSnowflake(httpx.MockTransport) registry,make_client/make_async_client, and pytest fixtures auto-registered via apytest11entry point. Adds anhttp_client=passthrough on both clients (no new runtime dependency; httpx is already core).fail_under = 89), and run coverage viacoverage run -m pytestso tracing starts before the shipped pytest plugin imports..pre-commit-config.yaml(ruff, black, mypy, detect-private-key), pinned to a Python-3.9-compatible black so the dev toolchain matches the runtime floor.docs/(getting-started, authentication, cli, testing, troubleshooting) and supply-chain hygiene:SECURITY.md, Dependabot (pip + actions), and issue/PR templates.Backwards compatibility
Additive only. No public API removed or changed; the sole public-surface addition is
http_client=(optional) on both clients and the newsnowflake_sql_api.testingmodule. The version is now derived from the git tag (was a hardcoded__version__); installed behaviour is unchanged.Review
Internal (bugs + polish) and external cross-family review (Codex + Kimi-agentic) before push. Fixed: a pre-1970 sub-second timestamp encoder off-by-one in the testing helper, a
Decimalscientific-notation encoding crash, out-of-range partition fetches now raise, and a py3.9with-syntax / black-version compatibility issue. Each fixed bug has a regression test.Changes
Test plan
coverage run -m pytest && coverage reportgreen (175 passed, 5 skipped, 94%; gate 89%)pre-commit run --all-filesclean (ruff, black, mypy, detect-private-key)python -m build+twine check dist/*pass; wheel version derives from the git tagFakeSnowflakewith no network, no respxast.parse(feature_version=(3,9)))pypienvironment are exercised by a tagSummary by CodeRabbit
Release Notes
New Features
FakeSnowflakefor mocking Snowflake queries without network access.Documentation
Chores