feat: publish query filtering binary#1066
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughAdds a new query-filtering-service CLI to the npm package: package.json exposes a Suggested reviewers
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/packages/npm-package/queryFilteringService.ts:
- Around line 99-108: The version-check currently just returns on failure,
allowing a successful exit; update the failure paths in the version check (the
branch that tests error !== null and the branch that compares binaryVersion !==
PACKAGE_VERSION) to call process.exit(1) after logging the error so the wrapper
exits non-zero on failure. Locate the conditional that references error,
binaryVersion and PACKAGE_VERSION and append a non-zero exit (process.exit(1))
to both failure branches to ensure the process signals failure to the caller.
- Line 8: The PACKAGE_VERSION constant is using a non-existent key
VERSIONS.PRIVATE_EPHEMERAL_VALIDATOR; update the constant to use the correct key
VERSIONS.QUERY_FILTERING_SERVICE (the key returned by getVersions()) so
PACKAGE_VERSION = `query-filtering-service ${VERSIONS.QUERY_FILTERING_SERVICE}`;
ensure you reference the VERSIONS symbol and PACKAGE_VERSION constant when
making the change.
In @.github/version-align.sh:
- Around line 50-55: The if condition is using the wrong guard variable so the
query-filtering-service update never runs; change the guard in the conditional
from private_validator_latest to query_filtering_service_latest (the variable
used in the echo and jq command) so the block executes when
query_filtering_service_latest is set; update the conditional that currently
references private_validator_latest to reference query_filtering_service_latest
in the same if/else block surrounding the jq call that updates
packages/npm-package/package.json.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6f8c719d-d047-4c34-b101-151264f8d6f5
📒 Files selected for processing (4)
.github/packages/npm-package/getVersions.ts.github/packages/npm-package/package.json.github/packages/npm-package/queryFilteringService.ts.github/version-align.sh
lucacillario
left a comment
lucacillario
left a comment
There was a problem hiding this comment.
@Dodecahedr0x look at code rabbit comments, the first one and the last one have to be addresseed
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/packages/npm-package/queryFilteringService.ts:
- Around line 20-26: The Windows fallback in trySystemQueryFilteringService()
currently probes only "/query-filtering-service" so it misses ".exe" (and
".cmd") binaries; update trySystemQueryFilteringService() to mirror
getExePath()'s platform handling by constructing and trying candidate names in
order (e.g., "query-filtering-service.exe", "query-filtering-service.cmd", then
"query-filtering-service") when on Windows (detect via
platform()/process.platform or reuse the same os/extension logic), testing each
candidate in PATH before giving up so the packaged-binary fallback and system
PATH resolution behave consistently.
- Around line 65-70: The current tryPackageQueryFilteringService function
returns true immediately after calling runQueryFilteringService(resolvedPath)
but spawn failures are emitted asynchronously; change
tryPackageQueryFilteringService to await a Promise returned by
runQueryFilteringService (or wrap the internal spawn in a Promise) that only
resolves true when the child emits the "spawn" event and resolves false when the
child emits an "error" event (and attach the error handler to avoid uncaught
exceptions); keep using getExePath to resolve the path and ensure
runWithForwardedExit's child is wired into that Promise so the top-level ||
fallback is reached when the Promise resolves false.
- Around line 36-45: The exit-forwarding currently defers propagation by
installing a process.on("exit") handler inside runWithForwardedExit's
child.on("exit") callback; change runWithForwardedExit to forward immediately
when the child emits "exit": inside the child.on("exit", ...) handler, if a
signal is present call process.kill(process.pid, signal) right away, otherwise
call process.exit(code) immediately (handling null case appropriately), and
remove the nested process.on("exit") usage so the parent exits/forwards as soon
as the child terminates.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 4d3f4cb3-fe5f-4fea-af9b-a015f4ab96f4
📒 Files selected for processing (3)
.github/packages/npm-package/package.json.github/packages/npm-package/queryFilteringService.ts.github/version-align.sh
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/packages/npm-package/queryFilteringService.ts:
- Around line 45-48: The process.on("SIGINT") handler currently sends both
child.kill("SIGINT") and child.kill("SIGTERM") immediately; remove the immediate
SIGTERM and instead send only SIGINT, then start a short timeout to send SIGTERM
if the child hasn't exited (e.g., setTimeout(() => child.kill("SIGTERM"), 5000)
and clear it if the child exits). Update the process.on("SIGINT") handler and
any child exit listeners to clear the escalation timer so SIGTERM is only used
as a fallback.
- Around line 10-17: getBinaryVersion currently treats stdout as a valid version
even when the spawned process exits with a non-zero status; update the function
to check result.status (from spawnSync) and treat any non-zero status as an
error case: if result.status !== 0 (or result.signal is set) build an error
string that includes the exit status/signal and stderr (if present) and return
that as the first tuple element instead of stdout; continue to return stdout
only when status is zero and no result.error exists. Reference: getBinaryVersion
and the spawnSync result fields (status, signal, stderr, stdout).
- Around line 84-116: trySystemQueryFilteringService currently discards the
Promise returned by runQueryFilteringService so spawn failures don't lead to
process.exit; make trySystemQueryFilteringService async, await the call to
runQueryFilteringService(absoluteBinaryPath), and if the awaited result is false
(spawn/error case) call process.exit(1); keep the existing binary/version checks
using getBinaryVersion and PACKAGE_VERSION unchanged but ensure callers that
invoke trySystemQueryFilteringService handle the returned Promise (await or
propagate) so the process exit behavior is honored.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ab817d97-f5f3-40dc-b8dd-ca9fe5bf8e07
📒 Files selected for processing (1)
.github/packages/npm-package/queryFilteringService.ts
| function getBinaryVersion(location: string): [string | null, string | null] { | ||
| const result = spawnSync(location, ["--version"]); | ||
| const error: string | null = | ||
| (result.error && result.error.toString()) || | ||
| (result.stderr.length > 0 && result.stderr.toString().trim()) || | ||
| null; | ||
| return [error, result.stdout && result.stdout.toString().trim()]; | ||
| } |
There was a problem hiding this comment.
Check exit status in version validation.
getBinaryVersion only checks result.error and result.stderr, but ignores result.status. If the binary exits with a non-zero status code while still writing to stdout, the function will treat that output as a valid version string. This could cause version validation to incorrectly accept malformed or error output.
🛡️ Proposed fix
function getBinaryVersion(location: string): [string | null, string | null] {
const result = spawnSync(location, ["--version"]);
const error: string | null =
(result.error && result.error.toString()) ||
+ (result.status !== 0 && `Process exited with code ${result.status}`) ||
(result.stderr.length > 0 && result.stderr.toString().trim()) ||
null;
return [error, result.stdout && result.stdout.toString().trim()];
}🤖 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 @.github/packages/npm-package/queryFilteringService.ts around lines 10 - 17,
getBinaryVersion currently treats stdout as a valid version even when the
spawned process exits with a non-zero status; update the function to check
result.status (from spawnSync) and treat any non-zero status as an error case:
if result.status !== 0 (or result.signal is set) build an error string that
includes the exit status/signal and stderr (if present) and return that as the
first tuple element instead of stdout; continue to return stdout only when
status is zero and no result.error exists. Reference: getBinaryVersion and the
spawnSync result fields (status, signal, stderr, stdout).
| process.on("SIGINT", () => { | ||
| child.kill("SIGINT"); | ||
| child.kill("SIGTERM"); | ||
| }); |
There was a problem hiding this comment.
Remove redundant SIGTERM after SIGINT.
Sending both SIGINT and SIGTERM in immediate succession (lines 46-47) doesn't give the child process time to handle SIGINT gracefully. Typically you would send SIGINT first and only escalate to SIGTERM if the child doesn't exit. The current code sends both signals at once, which may interfere with the child's signal handlers.
♻️ Proposed fix
process.on("SIGINT", () => {
child.kill("SIGINT");
- child.kill("SIGTERM");
});📝 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.
| process.on("SIGINT", () => { | |
| child.kill("SIGINT"); | |
| child.kill("SIGTERM"); | |
| }); | |
| process.on("SIGINT", () => { | |
| child.kill("SIGINT"); | |
| }); |
🤖 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 @.github/packages/npm-package/queryFilteringService.ts around lines 45 - 48,
The process.on("SIGINT") handler currently sends both child.kill("SIGINT") and
child.kill("SIGTERM") immediately; remove the immediate SIGTERM and instead send
only SIGINT, then start a short timeout to send SIGTERM if the child hasn't
exited (e.g., setTimeout(() => child.kill("SIGTERM"), 5000) and clear it if the
child exits). Update the process.on("SIGINT") handler and any child exit
listeners to clear the escalation timer so SIGTERM is only used as a fallback.
| function trySystemQueryFilteringService(): void { | ||
| const absolutePath = process.env.PATH?.split(path.delimiter) | ||
| .filter((dir) => dir !== path.dirname(process.argv[1])) | ||
| .find((dir) => { | ||
| try { | ||
| fs.accessSync(`${dir}/query-filtering-service`, fs.constants.X_OK); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| }); | ||
|
|
||
| if (!absolutePath) { | ||
| console.error("Could not find globally installed query-filtering-service."); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const absoluteBinaryPath = `${absolutePath}/query-filtering-service`; | ||
| const [error, binaryVersion] = getBinaryVersion(absoluteBinaryPath); | ||
|
|
||
| if (error !== null) { | ||
| console.error(`Failed to get version of global binary: ${error}`); | ||
| process.exit(1); | ||
| } | ||
| if (binaryVersion !== PACKAGE_VERSION) { | ||
| console.error( | ||
| `Globally installed query-filtering-service version is not correct. Expected "${PACKAGE_VERSION}", found "${binaryVersion}".`, | ||
| ); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| void runQueryFilteringService(absoluteBinaryPath); | ||
| } |
There was a problem hiding this comment.
Handle spawn failure in system fallback path.
At line 115, void runQueryFilteringService(absoluteBinaryPath) discards the Promise. If the spawn fails (missing executable, permission denied, etc.), the "error" event fires, logs a message, and the Promise resolves to false, but nothing is awaiting it. Control returns to main(), which completes, and the process may exit with code 0 or hang indefinitely depending on event loop state. This contradicts the explicit process.exit(1) calls on lines 98, 106, and 112 for other error conditions.
🐛 Proposed fix
- void runQueryFilteringService(absoluteBinaryPath);
+ const success = await runQueryFilteringService(absoluteBinaryPath);
+ if (!success) {
+ console.error("Failed to spawn query-filtering-service from system PATH.");
+ process.exit(1);
+ }
}
-async function main(): Promise<void> {
+async function main(): Promise<void> {
if (!(await tryPackageQueryFilteringService())) {
- trySystemQueryFilteringService();
+ await trySystemQueryFilteringService();
}
}Make trySystemQueryFilteringService async and await the spawn result so spawn failures exit with code 1.
🤖 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 @.github/packages/npm-package/queryFilteringService.ts around lines 84 - 116,
trySystemQueryFilteringService currently discards the Promise returned by
runQueryFilteringService so spawn failures don't lead to process.exit; make
trySystemQueryFilteringService async, await the call to
runQueryFilteringService(absoluteBinaryPath), and if the awaited result is false
(spawn/error case) call process.exit(1); keep the existing binary/version checks
using getBinaryVersion and PACKAGE_VERSION unchanged but ensure callers that
invoke trySystemQueryFilteringService handle the returned Promise (await or
propagate) so the process exit behavior is honored.
3 participants
Summary by CodeRabbit
New Features
Chores