UN-3636 [DEV] Cover six worker-free critical paths with integration tests#2164
Open
chandrasekharan-zipstack wants to merge 7 commits into
Open
UN-3636 [DEV] Cover six worker-free critical paths with integration tests#2164chandrasekharan-zipstack wants to merge 7 commits into
chandrasekharan-zipstack wants to merge 7 commits into
Conversation
The public deployment endpoint is guarded by a decorator rather than DRF permission classes, so a regression silently opens it. Assert every rejection lands before execute_workflow — no execution row, no dispatch — which makes the path provable without workers. Drop the decorative `entry` field from the critical-path registry; it was render-only and had already drifted from the real routes. Stop health-probing the runner in the e2e tier: container-based execution is being retired in favour of in-worker execution. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
Creating a workflow implicitly materialises its source and destination endpoints; nothing else in the product recreates them, so a workflow that loses them is silently unconfigurable. Assert the endpoints appear, start unconfigured, and accept configuration. Authoring is synchronous, so this half of workflow-create-execute needs no workers. The execute half stays a declared gap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
…al path Provisioning is the only place a deployment's API key is minted, and it is returned exactly once — a regression is unrecoverable for the caller. Assert the key is persisted, active, bound to the deployment, and listable. Also assert the auth guard's accept path: without it, a guard that rejected every request would satisfy the rejection cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
Composing a prompt is synchronous; only running it needs an LLM. Assert a project is creatable before any adapter exists — a fresh org has none — and that prompts bind to their project. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
Workers write usage rows, but the aggregation billed against them is a synchronous read. Under-counting loses revenue and cross-org leakage is a disclosure bug; the scoping lives in a default manager, so a stray switch to the base manager would break it silently. Seed two orgs sharing a run_id and assert only the caller's rows are summed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
…l path Credential validation is the only point where the platform proves it can reach external storage before a workflow depends on it; a version that always answered "valid" would only surface deep inside an execution. Test against the rig's MinIO rather than a mock, and assert registered credentials land encrypted. Adds MinIO to integration-backend's services for that reason. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughAdds critical-path integration tests for deployment, connector, Prompt Studio, workflow authoring, and usage aggregation. It also replaces critical-path entry mappings with marker-based coverage and removes the runner readiness dependency. ChangesCritical path coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
|
| Filename | Overview |
|---|---|
| backend/connector_v2/tests/test_connector_register.py | Covers live MinIO credential validation and checks stored connector metadata through the raw database column. |
| tests/critical_paths.yaml | Adds new marker-backed critical path declarations and removes the unused entry metadata. |
| tests/groups.yaml | Adds MinIO to the backend integration service requirements. |
| tests/rig/critical_paths.py | Updates the critical path dataclass and loader for the manifest shape. |
| tests/rig/reporting.py | Updates critical path markdown output for the manifest shape. |
| tests/rig/runtime.py | Removes the runner from e2e health probe targets. |
Reviews (2): Last reviewed commit: "UN-3636 [DEV] address review: prove ciph..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/usage_v2/tests/test_usage_aggregate.py (1)
42-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
tearDownto clearUserContextthread-local state.
UserContext.set_organization_identifieris called insetUpbut never cleaned up. SinceUserContextis thread-local, the org identifier persists after tests in this class complete and can leak into other test classes that rely onUsage.objects(whose manager filters viaUserContext), causing flaky cross-test contamination.🧹 Suggested cleanup
self.factory = APIRequestFactory() + def tearDown(self) -> None: + super().tearDown() + UserContext.set_organization_identifier(None) + `@pytest.mark.critical_path`("usage-aggregate-read")Verify that
set_organization_identifier(None)(or an equivalentclear/resetmethod onUserContext) is the correct way to unset the thread-local value.🤖 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 `@backend/usage_v2/tests/test_usage_aggregate.py` around lines 42 - 60, Add a tearDown method to UsageAggregateReadTest that clears the thread-local organization context after each test by calling UserContext.set_organization_identifier(None), or the existing equivalent reset/clear API if available. Ensure cleanup runs after every test, including failures, to prevent state leakage into other tests.
🤖 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 `@backend/connector_v2/tests/test_connector_register.py`:
- Around line 27-30: Update the module-level pytestmark skipif guard to also
require MINIO_SECRET_ACCESS_KEY, matching the bracket-based access used by the
MinIO test setup. Keep the existing live-MinIO reason and ensure the test suite
skips cleanly whenever any required MinIO environment variable is absent.
---
Nitpick comments:
In `@backend/usage_v2/tests/test_usage_aggregate.py`:
- Around line 42-60: Add a tearDown method to UsageAggregateReadTest that clears
the thread-local organization context after each test by calling
UserContext.set_organization_identifier(None), or the existing equivalent
reset/clear API if available. Ensure cleanup runs after every test, including
failures, to prevent state leakage into other tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9ff0d00-3b0e-405d-9006-e2b9fdfb0a94
📒 Files selected for processing (15)
backend/api_v2/tests/test_deployment_auth.pybackend/api_v2/tests/test_deployment_provision.pybackend/connector_v2/tests/__init__.pybackend/connector_v2/tests/test_connector_register.pybackend/prompt_studio/prompt_studio_core_v2/tests/test_prompt_studio_author.pybackend/usage_v2/tests/test_usage_aggregate.pybackend/workflow_manager/workflow_v2/tests/__init__.pybackend/workflow_manager/workflow_v2/tests/test_workflow_author.pytests/critical_paths.yamltests/groups.yamltests/rig/critical_paths.pytests/rig/reporting.pytests/rig/runtime.pytests/rig/tests/test_cli.pytests/rig/tests/test_critical_paths.py
💤 Files with no reviewable changes (2)
- tests/rig/tests/test_cli.py
- tests/rig/critical_paths.py
… skip - connector-register-test: assert the raw connector_metadata column is ciphertext (read past the decrypting field descriptor), so a regression to plaintext storage fails instead of passing on the decrypted round-trip. - Require MINIO_SECRET_ACCESS_KEY in the skip guard — the setup accesses it unconditionally, so a partial env now skips cleanly instead of raising KeyError. - usage-aggregate-read: clear the thread-local UserContext in tearDown so the org identifier can't leak into a later manager-filtered query. - Drop an explanatory comment from groups.yaml. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds marker-proven integration tests for six worker-free critical paths, one commit each. Follow-up to #2151 (merged), which shipped the multi-tier CI workflow but left these paths as uncovered gaps.
All six land in
integration-backend(testcontainers, parallel fast leg, no image build, transactional cleanup) and areproof: marker.api-deployment-authexecute_workflow— no execution row, no dispatch. Plus the accept path, so a guard that rejected everything wouldn't pass.workflow-authorapi-deployment-provisionprompt-studio-authorusage-aggregate-readrun_id— only the caller's rows are summed (billing + disclosure).connector-register-testAlso in here
entryfield from the critical-path registry (render-only, already drifted from real routes) — removed fromcritical_paths.yaml, rig code, and rig tests.integration-backend'srequires_services— connector credential validation must hit a real endpoint; mocking it would test nothing.Notes
*-executepaths remain declared gaps (covered_by: [], warn-only), pending hermetic LLM mocking (rubberduck).usage_v2.viewsandendpoint_v2.viewsresolve a queryset at class-body evaluation, which hits the DB at pytest collection time. Latent smell in the product code, flagged not fixed here.Verification
tox -e unitgreentox -e integration -- --fail-on-critical-gapgreen (zero in-scope gaps)tox -e rig -- validate→ 16 groups, 15 critical paths🤖 Generated with Claude Code
https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU