Skip to content

tsk-leewzc [OPEN] Make dev+test deps install by default: move to [de - #219

Merged
jaylfc merged 1 commit into
masterfrom
exec/tsk-leewzc
Jul 28, 2026
Merged

tsk-leewzc [OPEN] Make dev+test deps install by default: move to [de#219
jaylfc merged 1 commit into
masterfrom
exec/tsk-leewzc

Conversation

@jaylfc

@jaylfc jaylfc commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Autonomous build of board card tsk-leewzc.

REVIEW WARNING (automated): this card's text asks for tests, but the diff changes no test file. Either the acceptance criteria are unmet or the card needs correcting. Do not merge without resolving this.

Files:
pyproject.toml | 10 ++++++++++
1 file changed, 10 insertions(+)

Summary by CodeRabbit

  • Chores
    • Added a standardized development dependency group for consistent test and development environment setup.
    • Included optional authentication and registry-related development dependencies.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a PEP 735 dev dependency group to pyproject.toml containing pytest, pytest-asyncio, pyjwt, and cryptography for consistent development and test environment provisioning.

Changes

Development dependency provisioning

Layer / File(s) Summary
Define development dependency group
pyproject.toml
Adds a [dependency-groups] section with a dev group containing testing and authentication dependencies.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: it describes moving dev/test dependencies into dependency groups and making them install by default.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch exec/tsk-leewzc

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.

❤️ Share

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

@gitar-bot

gitar-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

Gitar

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Install test and registry-auth dependencies with default uv sync

⚙️ Configuration changes ✨ Enhancement 🕐 Less than 5 minutes

Grey Divider

AI Description

• Adds a default uv development group containing test and registry-auth dependencies.
• Preserves pip extras to keep core runtime installations lean.
• Enables clean checkouts to run tests after a bare uv sync.
Diagram

graph TD
  A["Developer"] --> B["uv sync"] --> C["Dev group"] --> G["Auth dependencies"] --> D["Test environment"]
  E["pip user"] --> F["Registry extra"] --> G
Loading
High-Level Assessment

The PEP 735 dependency group is the appropriate approach because uv installs the dev group by default, while retaining the existing optional extras preserves pip compatibility and a lean core installation. Moving authentication libraries into core dependencies or removing the pip extras would unnecessarily affect runtime consumers.

Files changed (1) +10 / -0

Other (1) +10 / -0
pyproject.tomlAdd a default PEP 735 development dependency group +10/-0

Add a default PEP 735 development dependency group

• Defines a 'dev' dependency group containing pytest, pytest-asyncio, PyJWT, and cryptography so a bare 'uv sync' creates a complete test environment. The existing pip development and registry extras remain unchanged for compatibility and opt-in runtime installation.

pyproject.toml

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
pyproject.toml (1)

38-39: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Add CI coverage for the new default-install contract.

No tests or installation checks are changed in this PR. Add a smoke check that runs uv sync --locked when uv.lock is tracked, then imports pytest, pytest_asyncio, jwt, and cryptography; this will catch stale lockfile changes and regressions where bare synchronization omits the group.

🤖 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 `@pyproject.toml` around lines 38 - 39, Add CI coverage for the
dependency-group contract by adding a smoke-check step that runs `uv sync
--locked` when uv.lock is tracked, then imports pytest, pytest_asyncio, jwt, and
cryptography. Ensure the check uses bare synchronization and fails on stale
lockfiles or omitted dev dependencies.
🤖 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.

Nitpick comments:
In `@pyproject.toml`:
- Around line 38-39: Add CI coverage for the dependency-group contract by adding
a smoke-check step that runs `uv sync --locked` when uv.lock is tracked, then
imports pytest, pytest_asyncio, jwt, and cryptography. Ensure the check uses
bare synchronization and fails on stale lockfiles or omitted dev dependencies.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9823310e-269e-4bf0-bea6-dbca4749e316

📥 Commits

Reviewing files that changed from the base of the PR and between 25bcdc4 and 656962b.

📒 Files selected for processing (1)
  • pyproject.toml

@jaylfc

jaylfc commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Verified against every acceptance criterion and merging. This is the card that unblocks lane testing across the fleet.

Criterion 1 — the actual defect, fixed. Bare uv sync, no flags, on a clean worktree of this branch:

{'pytest': True, 'pytest_asyncio': True, 'jwt': True, 'cryptography': True}

Criterion 2 — the exact command that died in twelve lane logs (uv run pytest tests/test_collections_ingest.py -q): 28 passed.

Criterion 3 — the auth surface now runs instead of silently skipping (issue #214): test_registry_auth.py + test_http_server_trust_enforcement.py37 passed, collected and executed.

Criterion 4 — core dependencies untouched. No change to [project] dependencies; pyjwt/cryptography stay out of the runtime install and live only in the dev group and the opt-in registry extra.

Criterion 5 — extras intact. registry = ["pyjwt>=2.8", "cryptography>=42"] unchanged, so pip install taosmd[registry] keeps working. The duplication with the dev group is deliberate and the comment in the file explains why.

Criterion 6 — before and after, both run rather than reasoned:

result
master, bare uv sync + uv run pytest error: Failed to spawn: pytest — cannot run at all
master, deps forced explicitly 10 failed / 1279 passed / 2 skipped
this branch, bare uv sync 10 failed / 1279 passed / 2 skipped

Failing count and skip count identical; the branch reaches by default exactly what master reaches only with four manual flags. That first row is the whole point — it is the defect reproduced on master minutes before merging.

On the automated REVIEW WARNING ("card asks for tests but the diff changes no test file"): a false positive here, and worth recording rather than dismissing. This card deliberately changes no test file — it changes dependency declaration, and its acceptance criteria ask for verification output, not new tests. The guard is right to fire on the general shape; it cannot distinguish "no tests written" from "no tests warranted". I have supplied the output above in its place.

One correction I owe, on my own card text. It says "runtime stays stdlib-only, non-negotiable", and I have repeated that phrase on the bus. It is wrong: [project] dependencies is ['onnxruntime', 'numpy', 'transformers']. The principle was right and is satisfied — auth and test dependencies must not enter the core install — but my description of the baseline was inaccurate, and a builder reading that card could reasonably have been confused by finding three runtime deps already there. Future cards will say "core dependencies stay minimal and must not gain auth/test deps."

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.

1 participant