feat: add bazel-deps-query config to dynamically resolve additional paths#13
feat: add bazel-deps-query config to dynamically resolve additional paths#13helenyugithub wants to merge 5 commits intomainfrom
Conversation
…aths Co-Authored-By: helen <helen@helenyu.tech>
Original prompt from helen |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
| Filename | Overview |
|---|---|
| src/util/bazel-query.ts | New utility module with well-structured functions for parsing bazel query output, resolving config values, and executing queries. Uses execFileSync (no shell) for safety. Minor concern: synchronous blocking could slow CI for large monorepos. |
| src/manifest.ts | Integrates bazel-deps-query into the build pipeline — resolves queries per package and merges with static additional paths before CommitSplit. Has minor duplicate logging with runBazelQuery. |
| schemas/config.json | Adds bazel-deps-query schema definition as oneOf [boolean, string] and enables propagation from top-level config to packages. |
| test/util/bazel-query.ts | Good unit test coverage for parseBazelQueryOutput and resolveBazelQuery. Missing test for resolveBazelQuery(false, ...) and edge cases like whitespace-only strings. |
| test/manifest.ts | Adds integration tests verifying config reading, bazel-resolved dependency path triggering releases, and merging with static additional paths. Proper use of sinon stubs for the bazel module. |
| test/fixtures/manifest/config/bazel-deps-query.json | Simple fixture config for testing bazel-deps-query: true with a single package. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[buildPullRequests] --> B{For each package}
B --> C{bazelDepsQuery set?}
C -- Yes --> D[resolveBazelQuery]
C -- No --> G[Use static additionalPaths only]
D -- "true" --> E["buildBazelQueryExpression\ndeps(//packagePath)"]
D -- "string" --> F[Parse expression from config]
E --> H[runBazelQuery\nexecFileSync bazel query expr]
F --> H
H --> I[parseBazelQueryOutput\nFilter externals, dedupe, exclude self]
I --> J[Merge bazel paths + static additionalPaths]
G --> J
J --> K[Deduplicate merged paths]
K --> L[CommitSplit with resolved paths]
L --> M[Continue release flow]
Last reviewed commit: 54b3a81
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/util/bazel-query.ts
Outdated
| logger?.info(`Running bazel deps query: ${query}`); | ||
|
|
||
| try { | ||
| const output = execSync(query, { |
There was a problem hiding this comment.
Arbitrary command execution via execSync
The query parameter is passed directly to execSync, which spawns a shell and executes it as-is. Since this value comes from release-please-config.json, anyone with write access to the config file can execute arbitrary commands on the CI runner — not just bazel queries.
For example, a config value like "rm -rf / && bazel query 'deps(//app)'" would be executed. Consider either:
- Validating that the command starts with
bazel query(e.g.,/^bazel\s+query\s/), or - Splitting the command and using
execFileSync('bazel', ['query', ...args])instead, which avoids shell interpretation entirely.
This was acknowledged in the PR description's review checklist, but adding a basic prefix validation would be a low-cost safeguard.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/util/bazel-query.ts
Line: 96
Comment:
**Arbitrary command execution via `execSync`**
The `query` parameter is passed directly to `execSync`, which spawns a shell and executes it as-is. Since this value comes from `release-please-config.json`, anyone with write access to the config file can execute arbitrary commands on the CI runner — not just bazel queries.
For example, a config value like `"rm -rf / && bazel query 'deps(//app)'"` would be executed. Consider either:
1. Validating that the command starts with `bazel query` (e.g., `/^bazel\s+query\s/`), or
2. Splitting the command and using `execFileSync('bazel', ['query', ...args])` instead, which avoids shell interpretation entirely.
This was acknowledged in the PR description's review checklist, but adding a basic prefix validation would be a low-cost safeguard.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Addressed in 8cdd609:
- Switched to
execFileSync('bazel', ['query', ...])(no shell) to mitigate command-injection risk. bazel-deps-querynow supportstrue(auto-constructs default expression from package path) or a string (either a query expression likedeps(//...)or a fullbazel query '...'command, from which we extract the expression before execution).
schemas/config.json
Outdated
| } | ||
| }, | ||
| "bazel-deps-query": { | ||
| "description": "A bazel query command to run to dynamically resolve additional dependency paths. The output is parsed to extract local package paths which are merged with additional-paths. Example: \"bazel query 'deps(//my-service)'\"", |
There was a problem hiding this comment.
@devin you should be able to construct the query based on the package name
There was a problem hiding this comment.
Good call! I'll update it so the query is auto-constructed from the package path. I'll make bazel-deps-query accept either:
true(boolean) → auto-constructsbazel query 'deps(//package-path)'- a string → uses it as a custom query (for advanced use cases)
So the typical config becomes just:
{
"packages": {
"apps/my-app": {
"bazel-deps-query": true
}
}
}![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Co-Authored-By: helen <helen@helenyu.tech>
| // If the expression is quoted, strip surrounding quotes. | ||
| if ( | ||
| (expr.startsWith('"') && expr.endsWith('"')) || | ||
| (expr.startsWith("'") && expr.endsWith("'")) | ||
| ) { | ||
| expr = expr.slice(1, -1); | ||
| } |
There was a problem hiding this comment.
Quote stripping can corrupt multi-part expressions
The quote-stripping logic checks if the expression starts and ends with the same quote character, then strips both. This will silently corrupt valid multi-argument bazel query inputs.
For example, bazel query 'deps(//a)' union 'deps(//b)' after prefix removal yields 'deps(//a)' union 'deps(//b)'. This starts with ' and ends with ', so the code strips them, producing deps(//a)' union 'deps(//b) — a broken expression.
A safer approach would be to only strip quotes when the inner content contains no unescaped quotes of the same type:
| // If the expression is quoted, strip surrounding quotes. | |
| if ( | |
| (expr.startsWith('"') && expr.endsWith('"')) || | |
| (expr.startsWith("'") && expr.endsWith("'")) | |
| ) { | |
| expr = expr.slice(1, -1); | |
| } | |
| if ( | |
| (expr.startsWith('"') && expr.endsWith('"') && !expr.slice(1, -1).includes('"')) || | |
| (expr.startsWith("'") && expr.endsWith("'") && !expr.slice(1, -1).includes("'")) | |
| ) { | |
| expr = expr.slice(1, -1); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/util/bazel-query.ts
Line: 119-125
Comment:
**Quote stripping can corrupt multi-part expressions**
The quote-stripping logic checks if the expression starts and ends with the same quote character, then strips both. This will silently corrupt valid multi-argument bazel query inputs.
For example, `bazel query 'deps(//a)' union 'deps(//b)'` after prefix removal yields `'deps(//a)' union 'deps(//b)'`. This starts with `'` and ends with `'`, so the code strips them, producing `deps(//a)' union 'deps(//b)` — a broken expression.
A safer approach would be to only strip quotes when the inner content contains no unescaped quotes of the same type:
```suggestion
if (
(expr.startsWith('"') && expr.endsWith('"') && !expr.slice(1, -1).includes('"')) ||
(expr.startsWith("'") && expr.endsWith("'") && !expr.slice(1, -1).includes("'"))
) {
expr = expr.slice(1, -1);
}
```
How can I resolve this? If you propose a fix, please make it concise.| if (config.bazelDepsQuery) { | ||
| const queryExpression = resolveBazelQuery(config.bazelDepsQuery, path); | ||
| bazelPaths = runBazelQuery(queryExpression, path, this.logger); |
There was a problem hiding this comment.
Empty query expression passed to runBazelQuery
When config.bazelDepsQuery is a non-empty string (truthy), resolveBazelQuery can still return an empty string — for example, if the config value is "bazel query " (just the prefix with trailing whitespace). In that case, runBazelQuery("") would be called, executing bazel query "" which will fail with a confusing error.
Consider guarding against an empty resolved expression:
| if (config.bazelDepsQuery) { | |
| const queryExpression = resolveBazelQuery(config.bazelDepsQuery, path); | |
| bazelPaths = runBazelQuery(queryExpression, path, this.logger); | |
| if (config.bazelDepsQuery) { | |
| const queryExpression = resolveBazelQuery(config.bazelDepsQuery, path); | |
| if (queryExpression) { | |
| bazelPaths = runBazelQuery(queryExpression, path, this.logger); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/manifest.ts
Line: 681-683
Comment:
**Empty query expression passed to `runBazelQuery`**
When `config.bazelDepsQuery` is a non-empty string (truthy), `resolveBazelQuery` can still return an empty string — for example, if the config value is `"bazel query "` (just the prefix with trailing whitespace). In that case, `runBazelQuery("")` would be called, executing `bazel query ""` which will fail with a confusing error.
Consider guarding against an empty resolved expression:
```suggestion
if (config.bazelDepsQuery) {
const queryExpression = resolveBazelQuery(config.bazelDepsQuery, path);
if (queryExpression) {
bazelPaths = runBazelQuery(queryExpression, path, this.logger);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
| let bazelPaths: string[] = []; | ||
| if (config.bazelDepsQuery) { | ||
| const queryExpression = resolveBazelQuery(config.bazelDepsQuery, path); | ||
| bazelPaths = runBazelQuery(queryExpression, path, this.logger); |
There was a problem hiding this comment.
Added debug logging for the resolved paths in 555a901:
Manifest.buildPullRequests()now logsbazel-deps-query resolved paths for <path>: [...]viathis.logger.debug(...).runBazelQuery()also logs start/results vialogger.debug(...).
So you should see the resolved paths when debug logging is enabled.
There was a problem hiding this comment.
Done in 54b3a81: switched the bazel-deps-query logs in both Manifest.buildPullRequests() and runBazelQuery() to info level.
Co-Authored-By: helen <helen@helenyu.tech>
Co-Authored-By: helen <helen@helenyu.tech>
Co-Authored-By: helen <helen@helenyu.tech>
1 participant
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕
Summary
Adds a new
bazel-deps-queryconfiguration option that allows a package to dynamically resolve its dependency paths by running a bazel query. The resolved paths are merged with any staticadditional-pathsand used to determine which commits should trigger a release for the package.Example config (recommended)
{ "packages": { "apps/my-app": { "bazel-deps-query": true, "additional-paths": ["libs/manually-specified-lib"] } } }Example config (custom query)
You can also provide either:
"deps(//combined-service)"), or"bazel query 'deps(//combined-service)'"), from which the query expression will be extracted.{ "packages": { "apps/my-app": { "bazel-deps-query": "deps(//combined-service)" } } }Changes
src/util/bazel-query.ts(new):parseBazelQueryOutput()parses bazel query output into local package paths (filters@...externals, dedupes, optional excludePath).resolveBazelQuery()supportstrue(auto-construct default) or a string (expression orbazel query ...).runBazelQuery()executes viaexecFileSync('bazel', ['query', <expr>])(no shell) with a 2-minute timeout.schemas/config.json: Addedbazel-deps-queryschema definition asoneOf: [boolean, string].src/manifest.ts:bazelDepsQuery?: boolean | stringto config types (ReleaserConfig,ReleaserConfigJson,extractReleaserConfig,mergeReleaserConfig).buildPullRequests()to resolvebazel-deps-queryper package and merge results with staticadditionalPathsbeforeCommitSplit.parseBazelQueryOutputandresolveBazelQueryadditionalPathsUpdates since last revision
bazel-deps-querycan be set totrueso the query is auto-constructed from the package path.execSync(shell) toexecFileSync(no shell) to mitigate command injection concerns from config-provided strings.Human Review Checklist
bazel-deps-query: true, we rundeps(//<packagePath>). Confirm//<packagePath>is a valid label pattern for your Bazel repo layout (package path vs. target naming conventions).bazelis available wherever release-please runs (local/CI/container) and that the 2-minute timeout is acceptable for your dependency graphs.bazel queryfails (bazel missing, invalid label, query error), release-please will throw and abort the run. Confirm fail-hard is desired.parseBazelQueryOutputmatches the query output format you expect (target shapes, root targets, external deps filtering).Link to Devin session: https://app.devin.ai/sessions/ccaef19250184b538e9c3f320b16a148
Requested by: @helenyugithub