Conversation
added 3 commits
June 27, 2026 00:42
- Register crates/gliner2-server in workspace members - Add Cargo.toml with axum, tokio, serde, clap dependencies - Create main.rs with CLI parsing, port retry, startup JSON protocol - Create health.rs with health endpoint - Create types.rs with InferRequest/Response types - Configure cargo target-dir to E:\cargo-target\anonymize - Add ndarray + half features to ort dependency for gliner2_inference compatibility
- Use node:child_process instead of Bun APIs for portability - Fix signal type (null instead of undefined) - Import Entity from types.ts instead of pipeline.ts - Fix env var access with bracket notation
Reviewer's GuideAdds a Rust-based GLiNER2 PII HTTP sidecar and a TypeScript client/inference factory that integrate with the existing anonymize pipeline via the NerInferenceFn contract, including label-mapping logic and workspace/infra tweaks for the new crate and build artifacts. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
More reviews will be available in 29 minutes and 32 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughAdded a new ChangesGLiNER2 Sidecar Integration
Sequence Diagram(s)sequenceDiagram
participant Gliner2Client
participant gliner2ServerMain as gliner2-server main
participant healthHandler as health_handler
participant inferHandler as infer_handler
participant Gliner2Engine
Gliner2Client->>gliner2ServerMain: start()
gliner2ServerMain-->>Gliner2Client: stdout {"event":"listening"}
loop until model_loaded
Gliner2Client->>healthHandler: GET /v1/health
healthHandler-->>Gliner2Client: HealthResponse
end
Gliner2Client->>inferHandler: POST /v1/infer
inferHandler->>Gliner2Engine: get_or_init(...)
inferHandler->>Gliner2Engine: extract(...)
Gliner2Engine-->>inferHandler: entities
inferHandler-->>Gliner2Client: InferResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Dependency ReviewThe following issues were found:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
.cargo/config.tomltarget-diris hardcoded toE:\cargo-target\anonymize, which will break on CI and other developers' environments; consider using a relative path or environment-based configuration instead. - In
Gliner2Client.infer, you're passingsignal: signal ?? nulltofetch;signalshould be anAbortSignal | undefined, so drop the property when undefined instead of passingnullto avoid relying on implicit coercion. - In
Gliner2Client.start, if the sidecar process exits before emitting thelisteningJSON line, you'll fall through to the generic 'no listening event' error; it would be helpful to detect premature process exit and surface the exit code or error to make startup failures easier to debug.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `.cargo/config.toml` `target-dir` is hardcoded to `E:\cargo-target\anonymize`, which will break on CI and other developers' environments; consider using a relative path or environment-based configuration instead.
- In `Gliner2Client.infer`, you're passing `signal: signal ?? null` to `fetch`; `signal` should be an `AbortSignal | undefined`, so drop the property when undefined instead of passing `null` to avoid relying on implicit coercion.
- In `Gliner2Client.start`, if the sidecar process exits before emitting the `listening` JSON line, you'll fall through to the generic 'no listening event' error; it would be helpful to detect premature process exit and surface the exit code or error to make startup failures easier to debug.
## Individual Comments
### Comment 1
<location path=".cargo/config.toml" line_range="6-7" />
<code_context>
protocol = "sparse"
+# Build artifacts to E: (fast NVMe, keeps C: free)
+[build]
+target-dir = "E:\\cargo-target\\anonymize"
+
[alias]
</code_context>
<issue_to_address>
**issue (bug_risk):** Hard-coded Windows-specific target-dir is likely to break on other machines and CI environments
Using `E:\cargo-target\anonymize` assumes that drive and path exist, which will break builds on most non-Windows machines and many Windows/CI environments. Please move this to a local-only config (outside the repo) or make it configurable (e.g., via an env var) so the workspace stays portable.
</issue_to_address>
### Comment 2
<location path="packages/anonymize/src/gliner2/client.ts" line_range="154-159" />
<code_context>
+ this.stop().catch(() => {});
+ }
+
+ private async resolveBinary(): Promise<string> {
+ const envPath = process.env.ANONYMIZE_GLINER2_SERVER_PATH;
+ if (envPath) return envPath;
+ // TODO: check bundled binary in node_modules, fallback to download
+ throw new Error(
+ "gliner2-server binary not found. " +
+ "Set ANONYMIZE_GLINER2_SERVER_PATH or use a bundled installation."
+ );
</code_context>
<issue_to_address>
**issue:** The `binaryPath` option is currently unused; `resolveBinary` only consults the environment variable
`Gliner2ClientOptions` exposes `binaryPath`, but `resolveBinary` never uses it and only reads `ANONYMIZE_GLINER2_SERVER_PATH`, which makes the option misleading and forces env-based configuration. Consider preferring `opts.binaryPath`, then falling back to the env var, and finally throwing if neither is provided.
</issue_to_address>
### Comment 3
<location path="packages/anonymize/src/gliner2/client.ts" line_range="28-29" />
<code_context>
+ };
+ }
+
+ get isRunning(): boolean {
+ return this.process !== null && this.port !== null;
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `isRunning` does not detect a crashed or exited child process
Because this only checks for non-null `process`/`port`, a crashed or exited child will still be reported as running, and `infer` will continue to target a dead sidecar. Please also check `this.process.exitCode` / `this.process.killed`, and consider clearing `process`/`port` on `exit`/`error` so callers see an accurate liveness signal.
Suggested implementation:
```typescript
get isRunning(): boolean {
if (!this.process || this.port == null) {
return false;
}
const { exitCode, killed } = this.process;
// If the process has an exit code or was marked as killed, it's not running anymore
if (exitCode !== null || killed) {
return false;
}
return true;
}
```
To fully implement the suggestion:
1. In the code where the child process is spawned (e.g. where `this.process = spawn(...)`), register `exit` and `error` handlers that clear the state:
- `this.process.on("exit", () => { this.process = null; this.port = null; });`
- `this.process.on("error", () => { this.process = null; this.port = null; });`
2. If you maintain any additional liveness-related state (e.g. a "ready" flag), also reset it in those handlers so `isRunning` plus that state reflect accurate liveness for callers like `infer`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
docs/superpowers/plans/2026-06-26-gliner2-pii-rust-implementation.md (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix heading level to satisfy markdownlint.
The
### Prerequisiteat Line 15 skipsh2. Add an##section before it, or change to## Prerequisite: Register crate in workspace.🤖 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 `@docs/superpowers/plans/2026-06-26-gliner2-pii-rust-implementation.md` at line 15, The markdown heading hierarchy is skipping a level in the plan document. Update the “Prerequisite: Register crate in workspace” heading so it uses the correct heading level, either by introducing an `##` section before it or by changing the heading itself to `## Prerequisite: Register crate in workspace`, and keep the surrounding section structure consistent.Source: Linters/SAST tools
.cargo/config.toml (1)
5-7: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMove the
target-diroverride out of shared Cargo config..cargo/config.tomlhardcodesE:\cargo-target\anonymize, which makes this repo config machine-specific; keep it in local untracked config or switch to a portable setup.🤖 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 @.cargo/config.toml around lines 5 - 7, The shared Cargo config is hardcoding a machine-specific target directory, which makes the repo non-portable. Remove the `build.target-dir` override from `.cargo/config.toml` and move it to a local untracked Cargo config or another developer-specific setup; if you keep it configurable, reference the `build` section so the repo defaults stay portable.packages/anonymize/src/gliner2/__test__/label-map.test.ts (1)
4-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an invariant over the full mapping table.
These examples only sample a few labels, so adding or reordering entries in
PIPELINE_TO_MODELcan break expand/collapse behavior without touching the tests. Please add a table-driven invariant that iterates the whole map and asserts each expanded model label collapses back into one of the requested pipeline labels, plus a collision case for multi-label requests.Suggested test shape
+import { PIPELINE_TO_MODEL, expandLabels, collapseLabel } from "../label-map"; -import { expandLabels, collapseLabel } from "../label-map"; + +it("round-trips every mapped label into one of the requested pipeline labels", () => { + for (const [pipelineLabel, modelLabels] of Object.entries(PIPELINE_TO_MODEL)) { + for (const modelLabel of modelLabels) { + expect( + collapseLabel(modelLabel, new Set([pipelineLabel])), + ).toBe(pipelineLabel); + } + } +});As per coding guidelines, in test files favor invariant-based tests; based on learnings, prefer invariants over examples when the input space is large.
🤖 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 `@packages/anonymize/src/gliner2/__test__/label-map.test.ts` around lines 4 - 51, The current tests in expandLabels and collapseLabel only cover a few examples, so changes to PIPELINE_TO_MODEL could slip through unnoticed. In label-map.test.ts, add a table-driven invariant that iterates the full mapping table and verifies each expanded model label collapses back to one of the originally requested pipeline labels using expandLabels and collapseLabel, and include a collision case covering multiple pipeline labels that share the same model label to confirm the preferred label is selected.Sources: Coding guidelines, Learnings
🤖 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 `@crates/gliner2-server/src/engine.rs`:
- Around line 13-23: Move the synchronous cold-start work in the ENGINE
initializer off the Tokio worker thread. In the async OnceCell setup around
Gliner2Engine::from_pretrained and ort::init().commit(), wrap the blocking model
load/download in a blocking task so the runtime thread is not stalled, then
await that task before returning the Arc< Gliner2Engine >. Keep the
ENGINE.get_or_try_init entrypoint and the Gliner2Engine::from_pretrained call as
the key locations to update.
In `@crates/gliner2-server/src/health.rs`:
- Around line 19-22: The HealthResponse in the health endpoint is hardcoding an
outdated version string instead of reporting the crate’s actual version. Update
the response construction in the health handler to use env!("CARGO_PKG_VERSION")
for the version field so it always matches the workspace-inherited crate
version, keeping the rest of the health payload unchanged.
In `@crates/gliner2-server/src/infer.rs`:
- Around line 32-39: The synchronous engine.extract call inside the infer
handler is blocking the async runtime thread; move the inference work into a
blocking task so /v1/infer does not stall other requests. Keep the existing
request parsing and error mapping in infer.rs, but run the engine.extract
invocation for req.text, tasks, and params via a blocking-thread helper and
return its result back into the handler before building the response.
In `@crates/gliner2-server/src/main.rs`:
- Around line 51-52: The startup handshake in main should be flushed immediately
after writing the JSON readiness line so the parent process does not hang
waiting for discovery. Update the stdout write in main to ensure the startup
event is emitted and flushed right away, using the startup handshake path around
writeln!(std::io::stdout(), ...) and the local binding information before
serving begins.
In `@docs/superpowers/plans/2026-06-26-gliner2-pii-rust-implementation.md`:
- Around line 449-457: The import-time reverse-map initialization for
MODEL_TO_PIPELINE is a module-level side effect in a shared module. Move the
construction logic into a lazy accessor such as getModelToPipeline() or a
lazySingleton, and have callers use that accessor instead of relying on eager
initialization. Keep the first-registration-wins behavior while ensuring the
mapping is built only on demand.
- Around line 862-864: The integration test cleanup currently relies on
GC/process exit, but the Rust sidecar client needs explicit shutdown to avoid
leaking child processes. Update the test setup around nerInference so the
created client is reachable at teardown (for example, attach it to the returned
function or keep it in a module-level variable), then change the afterAll
cleanup to call client.stop() if present. Make sure the fix uses the
nerInference/client symbols so the sidecar is always disposed in watch mode and
repeated runs.
- Around line 960-968: The release asset path in the cross-compilation workflow
is likely pointing at the wrong Cargo output location. Check whether
`.cargo/config.toml` or the build setup for `gliner2-server` sets a crate-local
`target-dir`; if not, update the `softprops/action-gh-release` files path to
match the workspace root `target/${{ matrix.target }}/release/...` produced by
`cargo build` in the `gliner2-server` build step.
- Around line 688-699: The stop() logic in the process manager has a stale
fallback race: the Bun.sleep(5000) SIGKILL callback can fire after a later
start() and kill the new process. Update stop() to capture the current process
in a local variable and ensure the fallback only targets that instance, or
replace the sleep-based fallback with a cancellable timeout/AbortController so
it is cleared when the process exits; use the stop() and process fields to
locate the fix. Also update dispose() to stop swallowing errors silently by
logging failures from stop() so shutdown issues are observable.
In `@packages/anonymize/src/gliner2/client.ts`:
- Around line 32-79: The gliner2 sidecar startup in start() only waits for
stdout and can hang or leave a stale child behind on failure. Update start() in
gliner2/client.ts to race the stdout “listening” parse against the child process
error/exit events and a startup timeout, and make sure any failure path tears
down this.process before throwing so later calls don’t reuse a half-started
process.
- Around line 109-114: The fetch in the GLiNER client’s inference path has no
built-in deadline, so add an internal timeout around the `/v1/infer` request in
the client’s inference method. Update the relevant method in `client.ts` (the
one that builds the `fetch` call) to create a default abort timeout and compose
it with any caller-provided `AbortSignal`, rather than passing `signal ?? null`
directly. Make sure the timeout is owned by the client and applies even when
callers do not supply a signal.
- Around line 154-161: Gliner2Client.resolveBinary currently ignores the
constructor-provided binaryPath and only checks ANONYMIZE_GLINER2_SERVER_PATH.
Update Gliner2Client and its resolveBinary method to prefer the configured
Gliner2ClientOptions.binaryPath first, then fall back to the env var, and only
throw if neither is present. Make sure the binary resolution logic still works
with the existing constructor options and preserves the current error path when
no valid path is available.
In `@packages/anonymize/src/gliner2/label-map.ts`:
- Around line 19-26: The label collision handling in collapseLabel should
preserve the caller’s requested label order instead of relying on the first
pipeline entry in PIPELINE_TO_MODEL. Update the mapping logic in label-map.ts so
each model label keeps all matching pipeline labels, then have collapseLabel
pick the first candidate based on the original input order. Use the existing
collapseLabel helper and the PIPELINE_TO_MODEL / MODEL_TO_PIPELINE lookup code
to locate and adjust the resolution path.
---
Nitpick comments:
In @.cargo/config.toml:
- Around line 5-7: The shared Cargo config is hardcoding a machine-specific
target directory, which makes the repo non-portable. Remove the
`build.target-dir` override from `.cargo/config.toml` and move it to a local
untracked Cargo config or another developer-specific setup; if you keep it
configurable, reference the `build` section so the repo defaults stay portable.
In `@docs/superpowers/plans/2026-06-26-gliner2-pii-rust-implementation.md`:
- Line 15: The markdown heading hierarchy is skipping a level in the plan
document. Update the “Prerequisite: Register crate in workspace” heading so it
uses the correct heading level, either by introducing an `##` section before it
or by changing the heading itself to `## Prerequisite: Register crate in
workspace`, and keep the surrounding section structure consistent.
In `@packages/anonymize/src/gliner2/__test__/label-map.test.ts`:
- Around line 4-51: The current tests in expandLabels and collapseLabel only
cover a few examples, so changes to PIPELINE_TO_MODEL could slip through
unnoticed. In label-map.test.ts, add a table-driven invariant that iterates the
full mapping table and verifies each expanded model label collapses back to one
of the originally requested pipeline labels using expandLabels and
collapseLabel, and include a collision case covering multiple pipeline labels
that share the same model label to confirm the preferred label is selected.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 029079d0-7b1c-4577-be90-5ed92ded149c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.ai/local/agents.md.cargo/config.toml.gitignoreAGENTS.mdCargo.tomlcrates/gliner2-server/Cargo.tomlcrates/gliner2-server/src/engine.rscrates/gliner2-server/src/health.rscrates/gliner2-server/src/infer.rscrates/gliner2-server/src/main.rscrates/gliner2-server/src/types.rsdocs/superpowers/plans/2026-06-26-gliner2-pii-rust-implementation.mdpackages/anonymize/src/gliner2/__test__/label-map.test.tspackages/anonymize/src/gliner2/client.tspackages/anonymize/src/gliner2/inference.tspackages/anonymize/src/gliner2/label-map.tspackages/anonymize/src/gliner2/types.tspackages/anonymize/src/index-shared.ts
Comment on lines
+449
to
+457
| const MODEL_TO_PIPELINE: Record<string, string> = {}; | ||
| for (const [pipeline, models] of Object.entries(PIPELINE_TO_MODEL)) { | ||
| for (const model of models) { | ||
| // First registration wins (earliest pipeline label takes priority) | ||
| if (!(model in MODEL_TO_PIPELINE)) { | ||
| MODEL_TO_PIPELINE[model] = pipeline; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Avoid module-level side effects in shared modules.
The for...of loop at Lines 450-457 runs at import time to populate MODEL_TO_PIPELINE. As per coding guidelines, do not introduce module-level side effects in shared modules. Move the reverse-map construction into a lazy initializer (e.g., getModelToPipeline() or lazySingleton) so utilities can be imported without triggering initialization.
// Replace with lazy singleton
let _modelToPipeline: Record<string, string> | undefined;
function getModelToPipeline(): Record<string, string> {
if (_modelToPipeline) return _modelToPipeline;
_modelToPipeline = {};
for (const [pipeline, models] of Object.entries(PIPELINE_TO_MODEL)) {
for (const model of models) {
if (!(model in _modelToPipeline)) {
_modelToPipeline[model] = pipeline;
}
}
}
return _modelToPipeline;
}🤖 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 `@docs/superpowers/plans/2026-06-26-gliner2-pii-rust-implementation.md` around
lines 449 - 457, The import-time reverse-map initialization for
MODEL_TO_PIPELINE is a module-level side effect in a shared module. Move the
construction logic into a lazy accessor such as getModelToPipeline() or a
lazySingleton, and have callers use that accessor instead of relying on eager
initialization. Keep the first-registration-wins behavior while ensuring the
mapping is built only on demand.
Source: Coding guidelines
added 3 commits
June 27, 2026 17:04
- Move blocking model init to spawn_blocking in engine.rs - Use CARGO_PKG_VERSION in health.rs - Move extract call to spawn_blocking in infer.rs - Flush stdout immediately in main.rs - Add timeout and abort controller in client.ts infer() - Fix label collision handling to preserve PIPELINE_TO_MODEL order - Add invariant tests in label-map.test.ts - Make cargo target-dir configurable via env var - Fix heading hierarchy in plan doc
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.
Summary
Implements GLiNER2 PII detection using a Rust HTTP sidecar binary with the SemplificaAI fragmented ONNX model.
Changes
Rust Sidecar (crates/gliner2-server/)
TypeScript Client (packages/anonymize/src/gliner2/)
Integration
Testing
Configuration
Future Work
esolveBinary()
Summary by Sourcery
Introduce a Rust-based GLiNER2 inference sidecar and a TypeScript client that integrates it into the anonymization pipeline via the existing NER inference contract.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes