fix(solve): non-mutating dry-run and fail loudly on unmatched provider filter#766
Merged
Conversation
…r filter - `ref solve --dry-run` is now a read-only preview: it no longer creates execution groups, reaps stale executions, or builds an executor - `ref solve` now fails with remediation when no diagnostic providers are configured, or when a --provider filter matches none of them, instead of exiting 0 having done nothing
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthrough
ChangesProvider Validation and Dry-Run Execution Flow
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
packages/climate-ref/tests/unit/cli/test_solve.py (1)
44-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a mixed-case substring assertion here.
This only exercises full lower-case matches. Since
_validate_provider_filter()is meant to mirror the solver’s case-insensitive substring matching, adding something like--provider ESMor--provider Lambwould lock that contract down and catch future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8021f0f3-660f-42c6-a431-015e1f983e70
📒 Files selected for processing (5)
changelog/766.fix.mdpackages/climate-ref/src/climate_ref/cli/solve.pypackages/climate-ref/src/climate_ref/solver.pypackages/climate-ref/tests/unit/cli/test_solve.pypackages/climate-ref/tests/unit/test_solver.py
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Lock down the case-insensitive substring matching contract in _validate_provider_filter() so future drift is caught.
A real solve reaps stale in-progress executions before evaluating ExecutionGroup.should_run, flipping abandoned dirty groups to runnable. The read-only dry-run skipped that reap and so under-reported them. Pass the same stale cutoff to should_run during dry-run so abandoned executions are treated as failed (reaped) for the preview, without mutating the database.
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.
Description
Fixing some oddities about
ref solve--dry-runis now genuinely non-mutating. Previouslyref solve --dry-runstill createdExecutionGrouprows (viaget_or_create), reaped stale in-progress executions, and built/joined an executor. It now performs a read-only preview: it looks groups up without creating them, skips the stale-execution sweep, and never builds an executor. A group that does not yet exist is reported as one the solve would create. A regression test asserts the execution-group row count is unchanged by a dry run, and that a subsequent real solve over the same catalog does create groups.Provider filters now fail loudly.
ref solve --provider <name>previously exited 0 with no work done when no providers were configured or the filter matched none of them, which looks like success. The command now validates before solving and exits non-zero with remediation (install a provider, regenerate config, or add it under[[diagnostic_providers]]). Matching mirrors the solver: a filter matches when it is a case-insensitive substring of a configured provider slug.Checklist
Please confirm that this pull request has done the following:
changelog/Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
ref solvenow fails with error + remediation when no diagnostic providers are configured, or when--providermatches none of the configured providers.ref solve --dry-runis now strictly read-only: it no longer creates execution groups or triggers execution steps, and it won’t run or wait on executors.Tests