Conversation
Add provider-owned minor-series and exact-patch claim normalization and intersection. Enforce canonical claims in binding records and solver accumulation, including cumulative active-provider intersection coverage.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| func validatePortableToolSupportedPythonClaimsV1(values []string) error { | ||
| if values == nil { | ||
| return fmt.Errorf("supported Python must use an array") | ||
| } | ||
| if len(values) == 0 { | ||
| return fmt.Errorf("supported Python must not be empty") | ||
| } | ||
| if len(values) > portableToolCatalogMaxReferencesV1 { | ||
| return fmt.Errorf("supported Python must use at most %d entries", portableToolCatalogMaxReferencesV1) | ||
| } | ||
| var previous []int | ||
| for _, value := range values { | ||
| if err := ValidatePythonInterpreterVersionV1(value); err != nil { | ||
| return fmt.Errorf("supported Python version %q: %w", value, err) | ||
| } |
There was a problem hiding this comment.
💡 Quality: Duplicate canonical-claim logic risks drifting out of sync
validatePortableToolSupportedPythonClaimsV1 in record_validate.go hand-rolls the canonical-claim rules (sorted, unique, no exact patch subsumed by a same-minor series), while providers.NormalizeSupportedPythonClaimsV1/IntersectSupportedPythonClaimsV1 implement the same contract independently for the solver. The solver assumes binding records that passed record validation are already fixed points of the provider normalizer; if either implementation changes (e.g. a new subsumption rule) without the other, published records can validate at record time yet be re-reduced or rejected during solving, breaking the stated invariant that a record rejected at publication cannot become acceptable after locking. Since portabletool cannot import providers (import cycle), consider a shared test asserting the two implementations agree on a corpus of claim lists to guard against drift.
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 2 findingsAdds provider-owned minor-series and exact-patch claim normalization and intersection, enforcing canonical claims in binding records and solver accumulation. Consider consolidating the duplicate canonical-claim logic in 💡 Quality: Duplicate canonical-claim logic risks drifting out of sync📄 internal/portabletool/record_validate.go:210-224 📄 internal/providers/python_claim.go:79-93
💡 Quality: Missing Changie fragment for claim-canonicalization change📄 internal/portabletool/record_validate.go:210-224 📄 internal/toolcatalog/solver.go:339-348 This PR enforces canonical Python support claims in binding records and active-provider constraints, so records that previously validated (e.g. unsorted or non-normalized 🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
| func validatePortableToolSupportedPythonClaimsV1(values []string) error { | ||
| if values == nil { | ||
| return fmt.Errorf("supported Python must use an array") | ||
| } | ||
| if len(values) == 0 { | ||
| return fmt.Errorf("supported Python must not be empty") | ||
| } | ||
| if len(values) > portableToolCatalogMaxReferencesV1 { | ||
| return fmt.Errorf("supported Python must use at most %d entries", portableToolCatalogMaxReferencesV1) | ||
| } | ||
| var previous []int | ||
| for _, value := range values { | ||
| if err := ValidatePythonInterpreterVersionV1(value); err != nil { | ||
| return fmt.Errorf("supported Python version %q: %w", value, err) | ||
| } |
There was a problem hiding this comment.
💡 Quality: Missing Changie fragment for claim-canonicalization change
This PR enforces canonical Python support claims in binding records and active-provider constraints, so records that previously validated (e.g. unsorted or non-normalized supported_python lists) will now be rejected — a maintainer-facing behavior change. Per the changelog convention, add a .changes/unreleased/ fragment (e.g. kind Changed) describing the new normalization/enforcement.
Was this helpful? React with 👍 / 👎
|
PR-cycle state — maintained automatically. Do not edit by hand. Mechanical PR-cycle state (JSON){
"check_observations": [],
"delivery_deferrals": [],
"findings": [],
"pr": {
"base_ref": "pr143",
"base_sha": "4f0e4249d48ae4f3d8c4d135180fba096a26262f",
"body_digest": "sha256:ed86b08fdf6f8a34d9c59a9571b77925862bc7d5027b5f20198bafd0884d440a",
"diff_digest": "sha256:3d8352349088ade15dc1fbc7c9c9bab96780e57bd11cf4fe85b4c3bb1d0bbfe8",
"head_ref": "pr144",
"head_sha": "bfb27d07dbe264205a05bfa073d0c5c6e6da64c1",
"pr": 145,
"repository": "omry/reploy",
"scope_authority": [
{
"digest": "sha256:bd02f5450b2940b3a960f6419994aa9c4e5e65ccb4dc5365b4d17ac233b49d9d",
"path": "docs/PORTABLE_TOOL_DEFINITION_DESIGN.md"
},
{
"digest": "sha256:570286ac642fd82e3177fb8ddb36dd15ee97395470726d69df086508be174a2e",
"path": "docs/PORTABLE_TOOL_DEFINITION_IMPLEMENTATION_PLAN.md"
}
],
"title_digest": "sha256:672b70a810833ced9e7b88f748c11bf7c4e5ec92aed313df8f598972ac6a8f65"
},
"record_version": 1,
"review_observations": [],
"review_requests": [],
"review_results": [],
"revision_fingerprint": "sha256:195415fbad96dfd27e5df3717220ea8cf5853cf24a85f7668697ae958d121466",
"rounds": [],
"schema": "awd:swe:pr-cycle-state",
"version": 2
} |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfb27d07db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _, ok := available[version]; ok { | ||
| intersection = append(intersection, version) | ||
| } | ||
| intersection, err := pythonprovider.IntersectSupportedPythonClaimsV1(previous.supported, normalized) |
There was a problem hiding this comment.
When two bindings in a shared package-manager domain claim 3.12 and 3.12.2, this solver path now correctly retains 3.12.2, but BuildPortableToolProviderDAGV1 still checks the same records through portableToolPythonSupportedIntersectionV1 (internal/providers/portable_tool_dag.go:616-624), which performs literal string intersection. The selected closure is therefore accepted here and subsequently rejected as a shared-domain conflict during DAG construction; route that validation through the canonical minor/patch intersection as well.
Useful? React with 👍 / 👎.
1 participant
Add provider-owned minor-series and exact-patch claim normalization and intersection. Enforce canonical claims in binding records and solver accumulation, including cumulative active-provider intersection coverage.