Update patch policy generation and tests - #45799
Conversation
Rework GenerateQueryForManifest to build the NOT EXISTS query more robustly: switch from string templates to helper functions (parenthesizeWhereClause, versionCompareSuffix, versionCompareColumn, tableFromExistsQuery) that trim/adjust the exists query, only parenthesize WHERE bodies containing OR, and choose the correct version column (use file_version for file table on Windows). Remove previous percent-escaping approach and simplify query assembly. Update tests and fixtures to match the new SQL formatting (remove extra inner parentheses) and to use file_version in the Codex CLI Windows manifest.
allenhouchins
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45799 +/- ##
==========================================
+ Coverage 66.76% 66.77% +0.01%
==========================================
Files 2746 2747 +1
Lines 219601 219842 +241
Branches 11008 10996 -12
==========================================
+ Hits 146609 146796 +187
- Misses 59738 59774 +36
- Partials 13254 13272 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
WalkthroughThis PR refactors the patch-policy SQL query generation in Possibly related PRs
🚥 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 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates patch policy query generation for maintained app manifests by rebuilding the NOT EXISTS patch query in a more SQL-idiomatic way, and adjusting tests/fixtures to match the new formatting and Windows file version semantics.
Changes:
- Reworked
GenerateQueryForManifestto appendversion_compare(...)inside the exists query’sWHEREclause, and only parenthesize theWHEREbody when it containsOR. - Added helper functions to derive the correct version column (including
file_versionfor Windowsfiletable queries) and to extract the table name from the exists query. - Updated Go tests and the Codex CLI Windows manifest fixture to reflect the new SQL formatting and column selection.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/patch_policy/patch_policy.go | Refactors patch policy query generation into helper functions; fixes OR-precedence handling and selects file_version for Windows file queries. |
| pkg/patch_policy/patch_policy_test.go | Updates expected SQL strings and adds coverage for OR-precedence + Windows file_version behavior. |
| ee/maintained-apps/outputs/codex-cli/windows.json | Updates patched query to use file_version for the Windows file table. |
| ee/maintained-apps/ingesters/homebrew/ingester_test.go | Updates expected patched query formatting to match the new generator output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/patch_policy/patch_policy.go`:
- Around line 44-50: Ensure the generated NOT EXISTS SQL only appends the
version_compare predicate when the original EXISTS query contains a WHERE
clause: after computing before (from p.ExistsQuery and parenthesizeWhereClause),
validate that before contains a WHERE clause (use a case-insensitive check or
regex for word boundary "WHERE") and if not, return an explicit error instead of
returning notExistsStart + before + suffix; update the function that builds this
SQL (referencing p.ExistsQuery, before, parenthesizeWhereClause, notExistsStart,
and suffix) to perform this guard and emit a clear error message when WHERE is
missing.
- Around line 71-77: The function versionCompareSuffix builds an SQL literal by
interpolating the version argument directly, which allows a single quote in
version to break the SQL; modify versionCompareSuffix to escape single quotes in
version (replace ' with '' per SQL standard) before formatting the string so the
produced clause is safe, keeping use of versionCompareColumn(platform,
existsQuery) unchanged and returning the formatted AND version_compare(...)
clause with the escaped version.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c5574bb-4768-450b-b98c-3a10439af816
📒 Files selected for processing (4)
ee/maintained-apps/ingesters/homebrew/ingester_test.goee/maintained-apps/outputs/codex-cli/windows.jsonpkg/patch_policy/patch_policy.gopkg/patch_policy/patch_policy_test.go
| before, _ := strings.CutSuffix(p.ExistsQuery, ";") | ||
| // Escape any literal '%' in the exists query (e.g. SQL LIKE patterns) | ||
| // so fmt.Sprintf doesn't interpret them as format verbs. | ||
| before = strings.ReplaceAll(before, "%", "%%") | ||
| before = strings.TrimSpace(before) | ||
| if strings.Contains(before, " OR ") { | ||
| before = parenthesizeWhereClause(before) | ||
| } | ||
|
|
||
| return notExistsStart + before + suffix, nil |
There was a problem hiding this comment.
Validate ExistsQuery has a WHERE clause before appending AND version_compare(...).
Line 50 can emit invalid SQL for inputs like SELECT 1 FROM apps; (... FROM apps AND version_compare(...)) and also permits unscoped matching. Guard this early and return an explicit error when WHERE is missing.
Suggested fix
var (
ErrWrongPlatform = errors.New("platform should be darwin or windows")
ErrNoExistsQuery = errors.New("exists query was not provided")
+ ErrInvalidExistsQuery = errors.New("exists query must include a WHERE clause")
)
@@
before, _ := strings.CutSuffix(p.ExistsQuery, ";")
before = strings.TrimSpace(before)
+ if !strings.Contains(strings.ToUpper(before), " WHERE ") {
+ return "", ErrInvalidExistsQuery
+ }
if strings.Contains(before, " OR ") {
before = parenthesizeWhereClause(before)
}As per coding guidelines: “ensure that appropriate filtering criteria are applied… Check for missing WHERE clauses…”
🤖 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 `@pkg/patch_policy/patch_policy.go` around lines 44 - 50, Ensure the generated
NOT EXISTS SQL only appends the version_compare predicate when the original
EXISTS query contains a WHERE clause: after computing before (from p.ExistsQuery
and parenthesizeWhereClause), validate that before contains a WHERE clause (use
a case-insensitive check or regex for word boundary "WHERE") and if not, return
an explicit error instead of returning notExistsStart + before + suffix; update
the function that builds this SQL (referencing p.ExistsQuery, before,
parenthesizeWhereClause, notExistsStart, and suffix) to perform this guard and
emit a clear error message when WHERE is missing.
| func versionCompareSuffix(platform, existsQuery, version string) (string, error) { | ||
| column, err := versionCompareColumn(platform, existsQuery) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return fmt.Sprintf(" AND version_compare(%s, '%s') < 0);", column, version), nil | ||
| } |
There was a problem hiding this comment.
Escape single quotes in version before SQL interpolation.
Line 76 interpolates version directly into a quoted SQL literal. A version containing ' can break SQL or alter predicate semantics. Escape with SQL-standard doubled quotes.
Suggested fix
func versionCompareSuffix(platform, existsQuery, version string) (string, error) {
column, err := versionCompareColumn(platform, existsQuery)
if err != nil {
return "", err
}
- return fmt.Sprintf(" AND version_compare(%s, '%s') < 0);", column, version), nil
+ safeVersion := strings.ReplaceAll(version, "'", "''")
+ return fmt.Sprintf(" AND version_compare(%s, '%s') < 0);", column, safeVersion), nil
}As per coding guidelines: “Review all SQL queries for possible SQL injection.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func versionCompareSuffix(platform, existsQuery, version string) (string, error) { | |
| column, err := versionCompareColumn(platform, existsQuery) | |
| if err != nil { | |
| return "", err | |
| } | |
| return fmt.Sprintf(" AND version_compare(%s, '%s') < 0);", column, version), nil | |
| } | |
| func versionCompareSuffix(platform, existsQuery, version string) (string, error) { | |
| column, err := versionCompareColumn(platform, existsQuery) | |
| if err != nil { | |
| return "", err | |
| } | |
| safeVersion := strings.ReplaceAll(version, "'", "''") | |
| return fmt.Sprintf(" AND version_compare(%s, '%s') < 0);", column, safeVersion), nil | |
| } |
🤖 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 `@pkg/patch_policy/patch_policy.go` around lines 71 - 77, The function
versionCompareSuffix builds an SQL literal by interpolating the version argument
directly, which allows a single quote in version to break the SQL; modify
versionCompareSuffix to escape single quotes in version (replace ' with '' per
SQL standard) before formatting the string so the produced clause is safe,
keeping use of versionCompareColumn(platform, existsQuery) unchanged and
returning the formatted AND version_compare(...) clause with the escaped
version.
Resolve modify/delete conflict for codex-cli/windows.json by accepting main's removal of Codex CLI (#45802).
This pull request refactors how patch policy SQL queries are generated and validated, with the main goal of simplifying and correcting the construction of
NOT EXISTSqueries for version checks. The changes ensure that the generated queries are more accurate, especially in cases involving SQLORconditions and platform-specific version columns. The update also adapts related test cases to match the new query structure.Patch policy query generation improvements:
GenerateQueryForManifestto append theversion_compareclause directly inside the originalWHEREclause, rather than wrapping the entire query in extra parentheses. This results in simpler, more standard SQL queries.ORconditions in theWHEREclause and wrap them in parentheses to ensure correct SQL precedence when appending theAND version_compare(...)clause.bundle_short_version,version, orfile_version) based on platform and table name, ensuring correct queries for both macOS and Windows policies.Test updates:
patch_policy_test.goto expect the new, simplified query format, removing the extra parentheses and validating correct handling of SQL withORand platform-specific columns. [1] [2] [3] [4]