chore(phenoAI): main snapshot 2026-06-10#57
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
More reviews will be available in 15 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
codeant-ai
Bot
added
the
size:XL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| parallel: true | ||
| commands: | ||
| editorconfig: | ||
| run: git diff --cached --name-only | xargs -n1 test -f && echo "Checking editorconfig..." |
There was a problem hiding this comment.
Suggestion: Replace this placeholder-style command with a real editorconfig validation step so the hook actually enforces formatting rules instead of only printing a message. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The command only verifies that staged paths exist and then prints a message; it does not perform any editorconfig validation. That is a real no-op enforcement gap in the hook.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** lefthook.yml
**Line:** 9:9
**Comment:**
*Custom Rule: Replace this placeholder-style command with a real editorconfig validation step so the hook actually enforces formatting rules instead of only printing a message.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| cd "$dir" && npx eslint --ext .ts,.tsx . 2>/dev/null || true | ||
| elif [ -f "$dir/pyproject.toml" ]; then | ||
| cd "$dir" && ruff check . 2>/dev/null || true | ||
| elif [ -f "$dir/go.mod" ]; then | ||
| cd "$dir" && go vet ./... 2>/dev/null || true |
There was a problem hiding this comment.
Suggestion: Make lint failures blocking for these tool branches by removing failure suppression so the pre-commit hook does not leave quality enforcement partially implemented. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
Each lint command suppresses stderr and then appends || true, which means failures are ignored and the pre-commit hook can pass even when linting fails. That matches the suggested violation.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** lefthook.yml
**Line:** 22:26
**Comment:**
*Custom Rule: Make lint failures blocking for these tool branches by removing failure suppression so the pre-commit hook does not leave quality enforcement partially implemented.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| just grade-fast 2>/dev/null || true | ||
| elif [ -f "Taskfile.yml" ]; then | ||
| task grade-fast 2>/dev/null || true |
There was a problem hiding this comment.
Suggestion: Do not suppress grade-fast failures here; failed fast-test checks should stop the commit flow to avoid leaving verification behavior effectively unfinished. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The hook explicitly ignores failures from grade-fast by redirecting output and using || true, so a failing fast test will not block the commit. This is a real enforcement issue.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** lefthook.yml
**Line:** 41:43
**Comment:**
*Custom Rule: Do not suppress `grade-fast` failures here; failed fast-test checks should stop the commit flow to avoid leaving verification behavior effectively unfinished.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 8 potential issues.
Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issues.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
| task grade | ||
| else | ||
| ./grade.sh | ||
| fi |
There was a problem hiding this comment.
Pre-push grade task missing
High Severity
The pre-push hook looks for Justfile (capital I), but the repo uses justfile. On case-sensitive filesystems it falls through to Taskfile.yml and runs task grade, which is not defined there, so the hook fails and blocks pushes.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
| default: | ||
| @just --list | ||
| # Lint with clippy (warnings as errors) AND fmt-check | ||
| lint: fmt-check (just --justfile {{justfile_path()}} lint) |
There was a problem hiding this comment.
Lint recipe calls itself
High Severity
The lint recipe runs just --justfile {{justfile_path()}} lint, which invokes the same lint recipe in this file again after fmt-check, causing unbounded recursion whenever just lint is run.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
| dev: | ||
| cargo watch -x check -x test | ||
| # Audit: cargo-deny + cargo-audit (combined) | ||
| audit: (just --justfile {{justfile_path()}} deny) (just --justfile {{justfile_path()}} --justfile {{justfile_path()}} audit) |
There was a problem hiding this comment.
Audit recipe calls itself
High Severity
The audit recipe includes just --justfile {{justfile_path()}} audit, which re-enters the same local audit recipe and recurses without end when just audit is run.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
| async def load(self, ref: ModelRef) -> None: | ||
| from transformers import AutoModelForCausalLM, AutoTokenizer | ||
| self._tok = AutoTokenizer.from_pretrained(ref.uri, revision=ref.revision) | ||
| self._model = AutoModelForCausalLM.from_pretrained(ref.uri, revision=ref.revision) |
There was a problem hiding this comment.
HF loader ignores URI scheme
High Severity
load passes ref.uri straight to from_pretrained, but documented and registry URIs use the hf:// prefix (e.g. hf://meta-llama/Llama-3.1-8B). Hugging Face expects a repo id, not that prefixed string, so loads fail after resolve.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
| # Phenotype-org standard justfile | ||
| # Phenotype-org shared justfile. Imported from phenotype-tooling/just/phenotype.just. | ||
| # To override a recipe locally, redefine it after the import. | ||
| import? "/Users/kooshapari/CodeProjects/Phenotype/repos/phenotype-tooling/just/phenotype.just" |
There was a problem hiding this comment.
Machine-specific just import path
Medium Severity
The optional import uses a fixed absolute path under /Users/kooshapari/..., so other machines and CI cannot load phenotype.just, leaving shared recipes like fmt-check and deny unavailable unless redefined locally.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
| if [ -n "$CHANGED" ]; then | ||
| echo "$CHANGED" | xargs -n1 dirname | sort -u | while read dir; do | ||
| if [ -f "$dir/Cargo.toml" ]; then | ||
| cd "$dir" && cargo fmt -- --check && cargo clippy -- -D warnings |
There was a problem hiding this comment.
Pre-commit Rust lint skipped
Medium Severity
Rust lint only runs when $dir/Cargo.toml exists for each changed file’s dirname. Edits under paths like crates/foo/src/ never see a manifest there, so workspace Rust changes skip cargo fmt and cargo clippy while the hook still succeeds.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
| inputs = self._tok(req.prompt, return_tensors="pt") | ||
| out = self._model.generate(**inputs, max_new_tokens=req.max_tokens, temperature=req.temperature, do_sample=req.temperature > 0) | ||
| text = self._tok.decode(out[0], skip_special_tokens=True) | ||
| return InferenceResponse(text=text, tokens_used=len(out[0]), finish_reason="stop") |
There was a problem hiding this comment.
Inference text includes prompt
Medium Severity
infer decodes the full generate output sequence and returns it as InferenceResponse.text, which includes the input prompt, not just newly generated tokens, unlike the local stub adapter’s completion-only text.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
| from abc import ABC, abstractmethod | ||
| from dataclasses import dataclass | ||
| from pathlib import Path | ||
| from typing import AsyncIterator, Optional |
There was a problem hiding this comment.
Unused imports in model_loader
Low Severity
The new ports/model_loader.py module imports Path and Optional but never uses them in the file.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
| @dataclass(frozen=True) | ||
| class ModelRef: | ||
| """Reference to a model artifact. Format depends on the adapter.""" | ||
| uri: str # e.g. "hf://meta-llama/Llama-3.1-8B", "local:///path/to/model.safetensors", "remote://api.example.com/v1/models/foo" |
There was a problem hiding this comment.
Suggestion: The port contract advertises hf://... URIs as the canonical HuggingFace format, but downstream loading code uses the raw uri directly with HuggingFace APIs that expect a repo id/path, not a custom scheme. If callers follow this contract literally, model loading will fail at runtime; define a normalized HuggingFace identifier format in the port or require/guarantee scheme stripping before adapter API calls. [api mismatch]
Severity Level: Major ⚠️
- ❌ HuggingFaceLoader fails for URIs documented as hf:// scheme.
- ⚠️ Registry.resolve provides adapter but incompatible URI string.
- ⚠️ Future HF-based inference flows will crash on load.Steps of Reproduction ✅
1. Create a `ModelRef` instance following the documented HuggingFace URI example in
`ports/model_loader.py:11-16`, e.g. `ref = ModelRef(uri="hf://meta-llama/Llama-3.1-8B")`,
which includes the custom `hf://` scheme as shown in the comment at line 14.
2. Resolve a loader for the same URI using the registry in `ports/registry.py:6-16`:
`loader = resolve("hf://meta-llama/Llama-3.1-8B")`; this returns a `HuggingFaceLoader`
because `"hf://"` is mapped to `HuggingFaceLoader` at line 7.
3. Call the adapter's load method with that `ModelRef`: `await loader.load(ref)`, which
executes `HuggingFaceLoader.load` in `ports/adapters/huggingface.py:7-10`, passing
`ref.uri` directly into `AutoTokenizer.from_pretrained(ref.uri, revision=ref.revision)`
and `AutoModelForCausalLM.from_pretrained(ref.uri, revision=ref.revision)`.
4. Because `ref.uri` still includes the `hf://` scheme (the registry does not strip it and
there is no normalizer anywhere in the repo; confirmed by searching for `hf://` in
`ports/model_loader.py` and `ports/registry.py`), the HuggingFace `from_pretrained` calls
receive an invalid identifier instead of a plain repo id like `"meta-llama/Llama-3.1-8B"`,
causing model resolution to fail at runtime when attempting to load models using the
port's advertised `hf://` URI format.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ports/model_loader.py
**Line:** 14:14
**Comment:**
*Api Mismatch: The port contract advertises `hf://...` URIs as the canonical HuggingFace format, but downstream loading code uses the raw `uri` directly with HuggingFace APIs that expect a repo id/path, not a custom scheme. If callers follow this contract literally, model loading will fail at runtime; define a normalized HuggingFace identifier format in the port or require/guarantee scheme stripping before adapter API calls.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| STACK="unknown" | ||
| if [[ -f "Cargo.toml" ]]; then STACK="rust"; fi | ||
| if [[ -f "package.json" ]]; then STACK="node"; fi | ||
| if [[ -f "pyproject.toml" || -f "setup.py" ]]; then STACK="python"; fi | ||
| if [[ -f "go.mod" ]]; then STACK="go"; fi |
There was a problem hiding this comment.
Suggestion: Stack detection overwrites STACK on every matching manifest, so in polyglot repos the final if wins and only one ecosystem is graded. That causes incomplete grading and false pass/fail outcomes because checks from other detected stacks are silently skipped. [logic error]
Severity Level: Critical 🚨
- ❌ Polyglot repos run checks for only one stack.
- ⚠️ Quality gate ignores failing tests in other languages.
- ⚠️ JSON grade report misrepresents overall project health.Steps of Reproduction ✅
1. Place `grade.sh` at the root of a polyglot repo containing both `Cargo.toml` and
`pyproject.toml` (or `setup.py`) alongside potential `package.json`/`go.mod` manifests,
matching the stack detection logic at `grade.sh:27-31`.
2. From that repo root, run `./grade.sh --json` so that the script executes the stack
detection block at `grade.sh:27-31` and then enters the `case "$STACK" in` dispatch at
`grade.sh:69-125`.
3. Observe that each `if` in `grade.sh:28-31` unconditionally overwrites `STACK`, so the
last matching manifest (e.g. `pyproject.toml` setting `STACK="python"` at line 30)
determines the single stack whose checks are run in the big `case` at `grade.sh:69-120`.
4. Confirm that checks for other present stacks (e.g. Rust `cargo test`, Node `npm test`,
or Go `go test` branches) never execute, so the JSON report written at `grade.sh:143-156`
and the overall grade at `grade.sh:127-139` only reflect the final stack, silently
skipping failures in the other ecosystems.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** grade.sh
**Line:** 27:31
**Comment:**
*Logic Error: Stack detection overwrites `STACK` on every matching manifest, so in polyglot repos the final `if` wins and only one ecosystem is graded. That causes incomplete grading and false pass/fail outcomes because checks from other detected stacks are silently skipped.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| local detail="$(head -5 "$REPORT_DIR/${name}.raw" | tr '\n' ' ')" | ||
| CHECKS+=("{\"name\":\"$name\",\"status\":\"fail\",\"score\":0,\"max\":$weight,\"detail\":\"$detail\"}") |
There was a problem hiding this comment.
Suggestion: Command output is interpolated directly into JSON string fields without escaping, so quotes/backslashes/newlines in log text can produce invalid JSON output in both in-memory check entries and grade.json. Escape JSON strings before constructing these objects. [type error]
Severity Level: Major ⚠️
- ⚠️ Machine consumers cannot reliably parse `grade.json`.
- ⚠️ CI dashboards depending on JSON reports may break.
- ⚠️ Debugging failed checks harder without valid structured data.Steps of Reproduction ✅
1. From the repo root, run `./grade.sh --json` so the Python stack branch at
`grade.sh:96-107` (or any stack) is exercised and JSON output is enabled at
`grade.sh:141-159`.
2. Induce a failing check whose error output contains JSON-breaking characters, e.g. add a
failing assertion in a Python test under `ports/tests/test_model_loader.py:8-37` that
raises `ValueError("bad \"value\"")`, causing pytest (invoked at `grade.sh:98`) to print a
message with embedded double quotes.
3. When the failing check runs, the `run_check` function at `grade.sh:40-61` captures the
first lines of the command's stderr/stdout into `detail` at `grade.sh:57`, without any
JSON escaping, and interpolates it directly into the JSON fragment appended to `CHECKS` at
`grade.sh:58`.
4. After all checks, the JSON report is written at `grade.sh:143-156` using `$(IFS=,; echo
"${CHECKS[*]}")`; attempts to parse `.grade-reports/grade.json` with a standard JSON
parser will fail due to unescaped quotes/backslashes in the `"detail"` field, despite the
file being advertised as machine-readable.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** grade.sh
**Line:** 57:58
**Comment:**
*Type Error: Command output is interpolated directly into JSON string fields without escaping, so quotes/backslashes/newlines in log text can produce invalid JSON output in both in-memory check entries and `grade.json`. Escape JSON strings before constructing these objects.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| @@ -0,0 +1,23 @@ | |||
| """HuggingFace adapter for ModelLoader (uses transformers).""" | |||
| from .model_loader import ModelLoader, ModelRef, InferenceRequest, InferenceResponse | |||
There was a problem hiding this comment.
Suggestion: This relative import is incorrect for a module inside ports/adapters: .model_loader resolves to ports.adapters.model_loader, which does not exist. Importing this adapter will fail immediately with ModuleNotFoundError; use the parent-package import path instead. [import error]
Severity Level: Critical 🚨
- ❌ Importing `ports.registry` crashes with ModuleNotFoundError.
- ❌ Any future HuggingFace adapter usage fails immediately.
- ⚠️ Pytest cannot run tests that import the registry.Steps of Reproduction ✅
1. From the repo root `/workspace/phenoAI`, start a Python REPL with this directory on
`PYTHONPATH` (pytest and other tooling assume this layout) and import the registry via
`from ports import registry` to exercise `ports/registry.py:1-4`.
2. The import of `ports.registry` executes `ports/registry.py`, which at
`ports/registry.py:2-4` performs `from .adapters.huggingface import HuggingFaceLoader` and
`from .adapters.local import LocalLoader`, importing both adapters.
3. When Python loads `ports.adapters.huggingface` from
`ports/adapters/huggingface.py:1-5`, it executes `from .model_loader import ModelLoader,
ModelRef, InferenceRequest, InferenceResponse` at line 2, resolving `.model_loader`
relative to the `ports.adapters` package.
4. Because there is no `ports/adapters/model_loader.py` (the interface lives at
`ports/model_loader.py:11-47`), the import machinery raises `ModuleNotFoundError: No
module named 'ports.adapters.model_loader'`, preventing `ports.registry` (and any code
using `resolve()` at `ports/registry.py:12-17`) from importing successfully.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ports/adapters/huggingface.py
**Line:** 2:2
**Comment:**
*Import Error: This relative import is incorrect for a module inside `ports/adapters`: `.model_loader` resolves to `ports.adapters.model_loader`, which does not exist. Importing this adapter will fail immediately with `ModuleNotFoundError`; use the parent-package import path instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| self._tok = AutoTokenizer.from_pretrained(ref.uri, revision=ref.revision) | ||
| self._model = AutoModelForCausalLM.from_pretrained(ref.uri, revision=ref.revision) |
There was a problem hiding this comment.
Suggestion: ModelRef.uri is documented as hf://..., but this code passes that full URI directly into from_pretrained, which expects a Hugging Face repo id/path, not a custom scheme prefix. This will make model loading fail for valid ModelRef values unless the scheme is stripped first. [logic error]
Severity Level: Major ⚠️
- ❌ HuggingFace-backed models cannot be loaded using `hf://` URIs.
- ⚠️ Adapter contract in `ModelRef` comment is violated.
- ⚠️ Future HF-based smoke tests will consistently fail.Steps of Reproduction ✅
1. Fix the import issue in `ports/adapters/huggingface.py:2` (for example, change to `from
..model_loader import ...`) so the module can be imported, then import `HuggingFaceLoader`
from `ports.adapters.huggingface` to exercise its `load` method at
`ports/adapters/huggingface.py:7-10`.
2. Construct a `ModelRef` from `ports/model_loader.py:11-17` with an HF-style URI, e.g.
`ref = ModelRef(uri="hf://meta-llama/Llama-3.1-8B")`, matching the documented example in
the `ModelRef.uri` comment at `ports/model_loader.py:14`.
3. Call `await HuggingFaceLoader().load(ref)`; the `load` implementation at
`ports/adapters/huggingface.py:7-10` passes `ref.uri` directly into
`AutoTokenizer.from_pretrained` and `AutoModelForCausalLM.from_pretrained` as the
`pretrained_model_name_or_path`.
4. Observe that the Transformers library does not recognize the custom `hf://` scheme and
fails to resolve the model (e.g. HTTP 404 or path error), because it expects a repo id
like `"meta-llama/Llama-3.1-8B"` or a filesystem path, not a URI with a `hf://` prefix;
stripping the scheme before calling `from_pretrained` is required for valid `ModelRef`
values to load.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ports/adapters/huggingface.py
**Line:** 9:10
**Comment:**
*Logic Error: `ModelRef.uri` is documented as `hf://...`, but this code passes that full URI directly into `from_pretrained`, which expects a Hugging Face repo id/path, not a custom scheme prefix. This will make model loading fail for valid `ModelRef` values unless the scheme is stripped first.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| async def unload(self) -> None: self._model = None; self._tok = None | ||
| async def infer(self, req: InferenceRequest) -> InferenceResponse: | ||
| inputs = self._tok(req.prompt, return_tensors="pt") | ||
| out = self._model.generate(**inputs, max_new_tokens=req.max_tokens, temperature=req.temperature, do_sample=req.temperature > 0) |
There was a problem hiding this comment.
Suggestion: generate() is a heavy synchronous call executed directly inside an async def, so it blocks the event loop and can stall other coroutines under concurrent load. Run inference in a worker thread/executor (or use an async backend) to avoid event-loop blocking. [performance]
Severity Level: Major ⚠️
- ⚠️ Async services using this adapter experience event-loop blocking.
- ⚠️ Concurrent inferences serialize, reducing throughput significantly.
- ⚠️ Upstream callers may see increased latency and timeouts.Steps of Reproduction ✅
1. After fixing the import path in `ports/adapters/huggingface.py:2` so the adapter loads,
write a small async script in this repo that imports `HuggingFaceLoader` from
`ports.adapters.huggingface` and defines an async function calling its `infer` method at
`ports/adapters/huggingface.py:12-16`.
2. In that script, create a `ModelRef` and `InferenceRequest` using the dataclasses from
`ports/model_loader.py:11-30`, then schedule multiple concurrent tasks
`asyncio.gather(*(loader.infer(req) for _ in range(10)))` under a single event loop.
3. When each task reaches the line `out = self._model.generate(...)` at
`ports/adapters/huggingface.py:14`, it invokes the heavy, synchronous `transformers`
`.generate()` call directly on the event loop thread, with no offloading to a worker
executor.
4. Under concurrent load, these synchronous calls block the event loop until each
generation completes, preventing other coroutines (including unrelated I/O-bound work such
as network handlers) from making progress, which would manifest in any async service using
this port as poor latency and timeouts during inference.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ports/adapters/huggingface.py
**Line:** 14:14
**Comment:**
*Performance: `generate()` is a heavy synchronous call executed directly inside an `async def`, so it blocks the event loop and can stall other coroutines under concurrent load. Run inference in a worker thread/executor (or use an async backend) to avoid event-loop blocking.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| let handler = tools | ||
| .get(name) | ||
| .ok_or(McpError::ToolNotFound(name.to_string()))?; | ||
|
|
||
| let result = | ||
| (handler.handler)(arguments).map_err(|e| McpError::InvalidRequest(e.to_string()))?; |
There was a problem hiding this comment.
Suggestion: The handler is executed while still borrowing from the tools read lock, so callback execution holds the lock for the full tool runtime and can block writers (register_tool) or deadlock if callbacks re-enter mutation paths. Clone the handler Arc out of the map, drop the lock guard, then invoke the callback. [performance]
Severity Level: Critical 🚨
- ❌ Tool handlers that mutate tools can deadlock call_tool.
- ⚠️ Long-running tools block concurrent tool registration/modification.Steps of Reproduction ✅
1. In `crates/mcp-server/src/lib.rs:102-121`, `McpServer::call_tool` acquires a read lock
on `self.tools` at line 107 (`let tools = self.tools.read().await;`) and then looks up a
handler at lines 108-110 and invokes it at lines 112-113.
2. In `tests/mcp_server_test.rs:63-88`, `test_call_tool` demonstrates registering a tool
with a closure handler and invoking it via `server.call_tool("echo", ...)`, exercising
this code path.
3. Extend this pattern with a tool handler that captures `server: McpServer` (as done for
other closures in `tests/mcp_server_test.rs:31-35, 46-55, 77-82`) and, inside the handler,
attempts to mutate the tool map (e.g., by calling `server.register_tool(...)` via
`tokio::runtime::Handle::current().block_on`).
4. When `call_tool` executes, it holds the `RwLock` read guard from line 107 for the
entire duration of the handler; the handler then attempts to acquire a write lock via
`register_tool` (lines 72-83 in `lib.rs`), which cannot proceed while the read lock is
held, leading to a deadlock in that task and blocking all writers for as long as tool
callbacks run.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/mcp-server/src/lib.rs
**Line:** 108:113
**Comment:**
*Performance: The handler is executed while still borrowing from the `tools` read lock, so callback execution holds the lock for the full tool runtime and can block writers (`register_tool`) or deadlock if callbacks re-enter mutation paths. Clone the handler `Arc` out of the map, drop the lock guard, then invoke the callback.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| # Phenotype-org standard justfile | ||
| # Phenotype-org shared justfile. Imported from phenotype-tooling/just/phenotype.just. | ||
| # To override a recipe locally, redefine it after the import. | ||
| import? "/Users/kooshapari/CodeProjects/Phenotype/repos/phenotype-tooling/just/phenotype.just" |
There was a problem hiding this comment.
Suggestion: The imported shared justfile path is machine-specific and will not exist on other developers' machines or CI, so shared recipes are silently unavailable and dependent targets fail. Replace this with a repository-relative path or a configurable environment variable. [hardcoded config]
Severity Level: Major ⚠️
- ❌ `fmt-check`/`deny` missing on machines without that path.
- ⚠️ `just lint`/`just audit` fail for most developers and CI.Steps of Reproduction ✅
1. On a non-macOS or non-author machine (e.g., CI container at `/workspace/phenoAI` with a
different user home), install `just` and clone this repo; the path
`/Users/kooshapari/CodeProjects/Phenotype/repos/phenotype-tooling/just/phenotype.just`
does not exist there.
2. The `justfile:3` uses `import?` with that absolute path, so the optional import
silently fails and none of the shared recipes (`fmt-check`, `deny`, etc.) from
`phenotype.just` are loaded into this project.
3. Run `just lint` as defined at `justfile:6`; Just attempts to resolve the dependency
recipe `fmt-check`, which is only provided by the imported file and is now missing,
causing Just to error with a "recipe not found" failure instead of running linting.
4. Similarly, running `just audit` (line 9) relies on `deny` imported from the same shared
justfile; without that file present on other machines or CI, `deny` is unavailable and
`just audit` fails, demonstrating the non-portability of the hardcoded absolute path.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** justfile
**Line:** 3:3
**Comment:**
*Hardcoded Config: The imported shared justfile path is machine-specific and will not exist on other developers' machines or CI, so shared recipes are silently unavailable and dependent targets fail. Replace this with a repository-relative path or a configurable environment variable.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| default: | ||
| @just --list | ||
| # Lint with clippy (warnings as errors) AND fmt-check | ||
| lint: fmt-check (just --justfile {{justfile_path()}} lint) |
There was a problem hiding this comment.
Suggestion: This recipe invokes just lint against the same justfile from inside lint, causing infinite self-recursion instead of running an underlying shared lint task. Call the imported concrete recipe directly (or rename wrapper/target) to avoid recursive invocation. [logic error]
Severity Level: Major ⚠️
- ❌ `just lint` recurses and never completes linting.
- ⚠️ Developers lose the intended clippy+fmt convenience task.Steps of Reproduction ✅
1. From the repository root (`/workspace/phenoAI`), run `just lint`, which resolves to the
`lint` recipe defined in `justfile:6`.
2. Just first runs the dependency `fmt-check` from the same justfile or imported file
(line 6 comment at line 5 indicates this is meant to be a clippy/fmt wrapper).
3. After `fmt-check`, Just executes the command `(just --justfile {{justfile_path()}}
lint)` from line 6, which spawns a new Just process targeting the same `lint` recipe in
the same justfile.
4. Each spawned Just process repeats step 3, recursively invoking `lint` again rather than
a distinct underlying lint recipe, causing unbounded self-recursion (until Just or the OS
aborts), so `just lint` never completes the intended linting.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** justfile
**Line:** 6:6
**Comment:**
*Logic Error: This recipe invokes `just lint` against the same justfile from inside `lint`, causing infinite self-recursion instead of running an underlying shared lint task. Call the imported concrete recipe directly (or rename wrapper/target) to avoid recursive invocation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| dev: | ||
| cargo watch -x check -x test | ||
| # Audit: cargo-deny + cargo-audit (combined) | ||
| audit: (just --justfile {{justfile_path()}} deny) (just --justfile {{justfile_path()}} --justfile {{justfile_path()}} audit) |
There was a problem hiding this comment.
Suggestion: This recipe recursively calls audit in the same justfile, which will loop indefinitely rather than executing a distinct imported audit target. Point this command to a non-recursive recipe name. [logic error]
Severity Level: Major ⚠️
- ❌ `just audit` self-recurses instead of running audits once.
- ⚠️ Dependency/security audit workflows fail via Just.Steps of Reproduction ✅
1. From the repository root, run `just audit`, which uses the `audit` recipe at
`justfile:9`.
2. Just executes the first command `(just --justfile {{justfile_path()}} deny)`, which
runs the `deny` recipe defined in the imported/shared justfile (if available), as
intended.
3. Next, Just executes the second command `(just --justfile {{justfile_path()}} --justfile
{{justfile_path()}} audit)` from line 9, which again invokes `just audit` on the same
justfile instead of a distinct, imported audit task.
4. This causes `audit` to recursively re-invoke itself in a new process, repeating step 3
indefinitely and preventing the audit command from ever completing the intended cargo-deny
+ cargo-audit checks.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** justfile
**Line:** 9:9
**Comment:**
*Logic Error: This recipe recursively calls `audit` in the same justfile, which will loop indefinitely rather than executing a distinct imported audit target. Point this command to a non-recursive recipe name.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| @@ -0,0 +1,21 @@ | |||
| """Local safetensors adapter for ModelLoader (uses llama.cpp or pure Rust).""" | |||
| from .model_loader import ModelLoader, ModelRef, InferenceRequest, InferenceResponse | |||
There was a problem hiding this comment.
Suggestion: This relative import points to ports/adapters/model_loader.py, which does not exist, so importing this adapter will raise ModuleNotFoundError at runtime. Import from the parent package instead so registry resolution can instantiate the adapter. [import error]
Severity Level: Critical 🚨
- ❌ Importing `ports.adapters.local` raises ModuleNotFoundError.
- ❌ `ports/tests/test_model_loader.py` cannot run LocalLoader tests.Steps of Reproduction ✅
1. Run the Python test suite for the ports package, e.g., `pytest
ports/tests/test_model_loader.py`, which imports `TestModelLoaderPort` at
`ports/tests/test_model_loader.py:1-8`.
2. That test module imports `LocalLoader` via `from ..adapters.local import LocalLoader`
at `ports/tests/test_model_loader.py:4`, which requires importing `ports.adapters.local`.
3. When Python imports `ports.adapters.local` (`ports/adapters/local.py:1-3`), it executes
the top-level line 2: `from .model_loader import ModelLoader, ModelRef, InferenceRequest,
InferenceResponse`, which tries to resolve `ports.adapters.model_loader`.
4. There is no `ports/adapters/model_loader.py` file (the correct module is
`ports/model_loader.py` as seen in `ports/model_loader.py:1-47`), so Python raises
`ModuleNotFoundError: No module named 'ports.adapters.model_loader'`, causing import of
`LocalLoader` and any tests or code using it to fail.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ports/adapters/local.py
**Line:** 2:2
**Comment:**
*Import Error: This relative import points to `ports/adapters/model_loader.py`, which does not exist, so importing this adapter will raise `ModuleNotFoundError` at runtime. Import from the parent package instead so registry resolution can instantiate the adapter.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| async def load(self, ref: ModelRef) -> None: | ||
| from pathlib import Path | ||
| path = Path(ref.uri.replace("local://", "")) | ||
| assert path.exists(), f"Model file not found: {path}" |
There was a problem hiding this comment.
Suggestion: Using assert for existence checks is unsafe because assertions are disabled with Python optimization flags, which would let missing model paths pass silently and leave invalid loader state. Raise a regular exception for this runtime validation. [logic error]
Severity Level: Major ⚠️
- ⚠️ Missing local model files go undetected under `python -O`.
- ⚠️ Future local inference may fail later with unclear errors.Steps of Reproduction ✅
1. Start Python with optimizations enabled (e.g., `python -O`) so that `assert` statements
are stripped at runtime.
2. In that process, import `LocalLoader` and `ModelRef` from the ports package: `from
ports.adapters.local import LocalLoader` and `from ports.model_loader import ModelRef`
(defined in `ports/model_loader.py:11-17`).
3. In an async context, call `await loader.load(ModelRef(uri="local:///does/not/exist"))`,
which executes `LocalLoader.load` at `ports/adapters/local.py:7-11`, computing `path` at
line 9 and hitting the existence check at line 10.
4. Because optimizations remove the `assert` at line 10, the missing model file is not
detected; execution proceeds to line 11, setting `self._ctx` and making `loader.is_loaded`
(property at lines 18-19) return `True`, leaving the loader in an invalid state with a
nonexistent model path and no clear error raised at load time.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ports/adapters/local.py
**Line:** 10:10
**Comment:**
*Logic Error: Using `assert` for existence checks is unsafe because assertions are disabled with Python optimization flags, which would let missing model paths pass silently and leave invalid loader state. Raise a regular exception for this runtime validation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| @@ -0,0 +1,23 @@ | |||
| """HuggingFace adapter for ModelLoader (uses transformers).""" | |||
| from .model_loader import ModelLoader, ModelRef, InferenceRequest, InferenceResponse | |||
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
HuggingFace and Local adapters import ModelLoader via .model_loader, but model_loader.py lives in the parent ports package, so importing these adapters (or ports.registry) raises ModuleNotFoundError and prevents the adapter modules from loading.
Suggestion: Update both adapters to import from the parent package (for example, from ..model_loader import ModelLoader, ModelRef, InferenceRequest, InferenceResponse) and rerun the ports.tests.test_model_loader smoke tests to confirm the modules import cleanly.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** ports/adapters/huggingface.py
**Line:** 2:2
**Comment:**
*CRITICAL: HuggingFace and Local adapters import `ModelLoader` via `.model_loader`, but `model_loader.py` lives in the parent `ports` package, so importing these adapters (or `ports.registry`) raises `ModuleNotFoundError` and prevents the adapter modules from loading.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
- Add grade.sh for Rust stack quality checks - Add lefthook.yml for git hook enforcement - Add grade/grade-fast/grade-json/grade-html recipes to justfile - Create docs/acceptance-contracts/ directory - Add .grade-reports/ to .gitignore - Fix cargo fmt formatting issues
…8 tests) The port trait is the SSOT: domain code depends on ModelLoader, not on HuggingFace or llama.cpp. Adapters are swappable at runtime via the URI-scheme registry (ports/registry.py). Adapters: HuggingFaceLoader (transformers), LocalLoader (safetensors). RemoteLoader (HTTP) is registered on-demand by the api server. Tests (8): default state, frozen request, finish_reason valid, ref defaults, local loader lifecycle, registry resolve known scheme, registry resolve unknown scheme raises, register custom adapter. DAG: T61 (R4 - hexagonal wave). SSOT: phenotype-voxel/src/ports/* (the reference hexagonal skeleton).
KooshaPari
force-pushed
the
chore/main-snapshot-2026-06-10-v2
branch
from
5117999 to
5e7d4fa
Compare
1 participant
User description
Local main ahead of origin. Snapshot branch for review.
Note
Medium Risk
Pre-push full grade and a machine-specific just import can block pushes or break clones; new model adapters add optional heavy HF/transformers usage though local path is stubbed.
Overview
Adds a fleet-wide grading pipeline via new
grade.sh(stack detection for Rust/Node/Python/Go, weighted checks, optional--fast/--json/--html, reports under.grade-reports/, exit non-zero below 85%)..gitignorenow ignores grade reports and drops worktree-related ignore patterns.Developer workflow shifts: the root
justfileimports sharedphenotype.just(absolute path on disk) and exposesgrade*recipes instead of the previous inline build/test/lint/ci recipes;lefthook.ymladds conventional commit validation, scoped pre-commit lint/test-fast, and fullgradeon pre-push.Introduces a new
ports/Python hexagonal ModelLoader API (ModelRef, async load/infer/stream), HuggingFace and local adapters (local inference is stubbed), URI-scheme registry (resolve/register), plus pytest smoke tests.Rust crates (
llm-router,mcp-server,pheno-embedding) change only formatting—no behavior changes.Reviewed by Cursor Bugbot for commit 5e7d4fa. Bugbot is set up for automated code reviews on this repo. Configure here.
CodeAnt-AI Description
Add a shared project grade workflow and a new model loader port
What Changed
Impact
✅ One-command project health checks✅ Clearer push-time quality gates✅ Faster model adapter selection💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.