chore(phenoAI): push main snapshot 2026-06-10#56
Conversation
- 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).
|
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 58 minutes. 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
![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 7 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.
| # 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.
Hardcoded just import path
High Severity
The optional import? points at a fixed absolute path on one developer machine. On CI or other clones, the shared phenotype.just recipes (including fmt-check, deny, and clippy lint) never load, so just lint and just audit fail or behave differently than intended.
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.
Just lint recurses infinitely
High Severity
The lint recipe lists itself as a dependency via just --justfile {{justfile_path()}} lint. Running just lint invokes lint again after fmt-check, causing unbounded recursion instead of running clippy from the shared justfile.
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.
Just audit recurses infinitely
High Severity
The audit recipe invokes just ... audit on the same justfile. That re-enters the audit recipe instead of only running deny and a distinct audit command from shared tooling, so just audit never completes normally.
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 keeps URI scheme
High Severity
HuggingFaceLoader.load passes ref.uri straight into from_pretrained, but registry URIs use the hf:// scheme documented on ModelRef. LocalLoader strips local://; Hugging Face expects a repo id like org/model, not hf://org/model.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
| 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) | ||
| text = self._tok.decode(out[0], skip_special_tokens=True) |
There was a problem hiding this comment.
Decode includes prompt text
Medium Severity
infer decodes the full generate output tensor, which includes the input prompt tokens. Callers receive the prompt plus completion as InferenceResponse.text instead of newly generated text only.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
| 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.
Stack detection order wrong
Medium Severity
Stack detection overwrites STACK with each manifest found, so the last match wins. A repo with both Cargo.toml and package.json (or go.mod) is graded with the wrong toolchain instead of a defined priority.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
| echo " [PASS] $name" | ||
| else | ||
| 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.
Fail detail breaks JSON
Medium Severity
Failed check details are pasted into JSON strings without escaping quotes or backslashes from command output. grade.sh --json can emit invalid JSON when a failing tool prints " or newlines in the first lines of its log.
Reviewed by Cursor Bugbot for commit 5117999. Configure here.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fleet-wide project grading engine (grade.sh), git hook configurations (lefthook.yml), and a new hexagonal ModelLoader port with HuggingFace and Local adapters. Feedback on these changes highlights several critical issues: a hardcoded absolute path in the justfile import, a failure to strip the hf:// prefix before loading Hugging Face models, fragile directory navigation using cd - in the pre-commit hook, potential JSON malformation in grade.sh due to unescaped quotes/backslashes, and fragile JSON parsing using grep and cut when generating the HTML report.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # 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.
Avoid hardcoding absolute paths of a specific user's local directory (e.g., /Users/kooshapari/...) in shared configuration files. This will fail on other developers' machines and in CI/CD environments. Use a relative path instead.
import? "../phenotype-tooling/just/phenotype.just"
| 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.
The ModelRef.uri contains the scheme prefix (e.g., hf://). Passing this directly to from_pretrained will cause Hugging Face's transformers library to fail to load the model. Strip the hf:// prefix before loading.
| self._tok = AutoTokenizer.from_pretrained(ref.uri, revision=ref.revision) | |
| self._model = AutoModelForCausalLM.from_pretrained(ref.uri, revision=ref.revision) | |
| repo_id = ref.uri.replace("hf://", "") | |
| self._tok = AutoTokenizer.from_pretrained(repo_id, revision=ref.revision) | |
| self._model = AutoModelForCausalLM.from_pretrained(repo_id, revision=ref.revision) |
| 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 | ||
| elif [ -f "$dir/package.json" ]; then | ||
| 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 | ||
| fi | ||
| cd - > /dev/null | ||
| done |
There was a problem hiding this comment.
Using cd - inside the loop is fragile because if none of the conditions match, cd is never executed but cd - is still run, which will change the working directory to an unexpected location and break subsequent iterations. Use a subshell (cd "$dir" && ...) to isolate the directory change safely.
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)
elif [ -f "$dir/package.json" ]; then
(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)
fi
done| 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.
If the check output (detail) contains double quotes or backslashes, inserting it directly into the JSON string will produce malformed JSON and break the --json report generation. Sanitize the detail string by removing or escaping double quotes and backslashes.
| local detail="$(head -5 "$REPORT_DIR/${name}.raw" | tr '\n' ' ')" | |
| CHECKS+=("{\"name\":\"$name\",\"status\":\"fail\",\"score\":0,\"max\":$weight,\"detail\":\"$detail\"}") | |
| local detail="$(head -5 "$REPORT_DIR/${name}.raw" | tr '\n' ' ' | tr -d '"' | tr -d '\\')" | |
| CHECKS+=("{\"name\":\"$name\",\"status\":\"fail\",\"score\":0,\"max\":$weight,\"detail\":\"$detail\"}") |
| name=$(echo "$check" | grep -o '"name":"[^"]*"' | cut -d'"' -f4) | ||
| status=$(echo "$check" | grep -o '"status":"[^"]*"' | cut -d'"' -f4) | ||
| score=$(echo "$check" | grep -o '"score":[0-9]*' | cut -d':' -f2) | ||
| max=$(echo "$check" | grep -o '"max":[0-9]*' | cut -d':' -f2) |
There was a problem hiding this comment.
Parsing JSON with a simple grep -o and cut is fragile because if the detail field contains keys like "name":"..." or "status":"..." (e.g., from compiler/test outputs), grep will match multiple lines and produce garbled HTML. Use anchored sed patterns to extract the fields robustly from the start of the JSON string.
| name=$(echo "$check" | grep -o '"name":"[^"]*"' | cut -d'"' -f4) | |
| status=$(echo "$check" | grep -o '"status":"[^"]*"' | cut -d'"' -f4) | |
| score=$(echo "$check" | grep -o '"score":[0-9]*' | cut -d':' -f2) | |
| max=$(echo "$check" | grep -o '"max":[0-9]*' | cut -d':' -f2) | |
| name=$(echo "$check" | sed -E 's/^\{"name":"([^"]*)".*/\1/') | |
| status=$(echo "$check" | sed -E 's/^\{"name":"[^"]*","status":"([^"]*)".*/\1/') | |
| score=$(echo "$check" | sed -E 's/^\{"name":"[^"]*","status":"[^"]*","score":([0-9]+).*/\1/') | |
| max=$(echo "$check" | sed -E 's/^\{"name":"[^"]*","status":"[^"]*","score":[0-9]+,"max":([0-9]+).*/\1/') |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| # Stub: in production, call llama-cpp-python or rust binding | ||
| return InferenceResponse(text=f"[local inference: {req.prompt[:20]}...]", tokens_used=len(req.prompt)//4, finish_reason="stop") |
There was a problem hiding this comment.
Suggestion: Replace this placeholder inference path with a fully implemented backend call (or block the path with an explicit not-implemented error) so unfinished behavior is not shipped as functional output. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The code explicitly contains a stubbed inference path and returns a fabricated response instead of performing real backend inference. That matches the suggestion's concern about unfinished behavior being shipped as functional output.
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:** 14:15
**Comment:**
*Custom Rule: Replace this placeholder inference path with a fully implemented backend call (or block the path with an explicit not-implemented error) so unfinished behavior is not shipped as functional output.
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()))?; |
There was a problem hiding this comment.
Suggestion: Replace eager error construction with lazy error construction here to avoid clippy's or_fun_call warning under -D warnings. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The code eagerly constructs the error value inside ok_or(...), which means name.to_string() is evaluated even when the option is Some. This is the exact pattern that can trigger clippy's or_fun_call warning, so the suggestion correctly identifies a real 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:** crates/mcp-server/src/lib.rs
**Line:** 108:110
**Comment:**
*Custom Rule: Replace eager error construction with lazy error construction here to avoid clippy's `or_fun_call` warning under `-D warnings`.
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 resource = resources | ||
| .get(uri) | ||
| .ok_or(McpError::ResourceNotFound(uri.to_string()))?; |
There was a problem hiding this comment.
Suggestion: Use deferred error creation at this lookup site as well so clippy does not flag eager allocation when warnings are denied. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is the same eager error-construction pattern as the tool lookup above. The uri.to_string() allocation happens unconditionally inside ok_or(...), so the lint concern is real and the suggestion is verified.
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:** 130:132
**Comment:**
*Custom Rule: Use deferred error creation at this lookup site as well so clippy does not flag eager allocation when warnings are denied.
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 _response = self.client | ||
| let _response = self |
There was a problem hiding this comment.
Suggestion: Do not suppress the provider response with an underscore binding; consume and validate the HTTP response (status/body) and map it into the typed return value instead of silently discarding it. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The code sends the request and stores the result in _response, but never inspects the HTTP status or body before continuing. That is a real omission in the embedding path and matches the suggestion's complaint about discarding the provider response.
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/pheno-embedding/src/lib.rs
**Line:** 65:72
**Comment:**
*Custom Rule: Do not suppress the provider response with an underscore binding; consume and validate the HTTP response (status/body) and map it into the typed return value instead of silently discarding it.
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
| lint: fmt-check (just --justfile {{justfile_path()}} lint) | ||
|
|
||
| # Watch for changes (cargo-watch) | ||
| 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.
🟠 Architect Review — HIGH
Local lint and audit recipes override the shared ones and then invoke just --justfile {{justfile_path()}} lint/audit from the same justfile, so just lint and just audit can only recurse/fail and never reach the imported recipes, leaving these core commands non-functional.
Suggestion: Redefine the local lint/audit recipes as thin wrappers that call concrete shared recipe names or inline the actual commands, avoiding re-invoking the same recipe name via just --justfile {{justfile_path()}} ....
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:** justfile
**Line:** 6:9
**Comment:**
*HIGH: Local `lint` and `audit` recipes override the shared ones and then invoke `just --justfile {{justfile_path()}} lint`/`audit` from the same justfile, so `just lint` and `just audit` can only recurse/fail and never reach the imported recipes, leaving these core commands non-functional.
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| if [ -f "Justfile" ]; then | ||
| just grade | ||
| elif [ -f "Taskfile.yml" ]; then | ||
| task grade |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
The pre-push grade hook checks for Justfile (capital J) even though this repo provides justfile, so it falls through to task grade on Taskfile.yml where no grade task exists, causing task: Task "grade" not found and blocking pushes.
Suggestion: Update the pre-push script to detect the actual justfile name and/or call ./grade.sh directly when no grade task exists in Taskfile, ensuring the enforced grade check runs without failing on a nonexistent task.
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:** lefthook.yml
**Line:** 68:71
**Comment:**
*CRITICAL: The pre-push `grade` hook checks for `Justfile` (capital J) even though this repo provides `justfile`, so it falls through to `task grade` on `Taskfile.yml` where no `grade` task exists, causing `task: Task "grade" not found` and blocking pushes.
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
| 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: Failed-check log text is interpolated into JSON without escaping, so quotes/backslashes in command output can produce invalid JSON in grade.json. Serialize this field with proper JSON escaping (or use jq/a JSON encoder) before appending to CHECKS. [logic error]
Severity Level: Major ⚠️
- ❌ grade.json invalid when failed checks emit quotes/backslashes.
- ⚠️ Tooling consuming JSON reports fails or misbehaves.Steps of Reproduction ✅
1. From the repo root `/workspace/phenoAI`, run `just grade-json` which is defined in
`justfile:19-21` to execute `./grade.sh --json`.
2. `grade.sh` detects the Rust stack (Cargo.toml present) and runs a series of checks via
`run_check()` at `grade.sh:40-61`; force one check to fail (for example, temporarily
introduce a syntax error in `crates/llm-router/src/lib.rs` so `cargo build --workspace`
fails with an error message containing double quotes).
3. On failure, `run_check()` captures the first 5 lines of the failing command's output
into `detail` at `grade.sh:57` using `head ... | tr '\n' ' '`, and appends a JSON fragment
to the `CHECKS` array at `grade.sh:58` by interpolating `$detail` directly inside
`"detail":"$detail"` without any JSON escaping.
4. When JSON output is requested, `grade.sh` writes `grade.json` at `grade.sh:141-155` by
joining the `CHECKS` entries; because `detail` can contain unescaped quotes/backslashes
from the tool output, `grade.json` becomes invalid JSON (e.g., `jq .
.grade-reports/grade.json` fails with a parse error).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:**
*Logic Error: Failed-check log text is interpolated into JSON without escaping, so quotes/backslashes in command output can produce invalid JSON in `grade.json`. Serialize this field with proper JSON escaping (or use `jq`/a JSON encoder) before appending to `CHECKS`.
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 an absolute, developer-specific filesystem path, so it will not exist on CI/other developers' machines and shared recipes will silently not load. Replace it with a repository-relative path or another portable mechanism. [code quality]
Severity Level: Major ⚠️
- ⚠️ Shared just recipes missing on CI/other developer machines.
- ⚠️ Lint/audit wrappers fail when shared justfile not found.Steps of Reproduction ✅
1. Clone `/workspace/phenoAI` onto any machine where the absolute path
`/Users/kooshapari/CodeProjects/Phenotype/repos/phenotype-tooling/just/phenotype.just`
does not exist (e.g., CI or another developer's non-macOS home directory).
2. From the repo root, run `just --list` or `just lint`; `just` loads `justfile:1-25`,
encounters the `import?` at `justfile:3`, and silently skips it because the referenced
file is missing.
3. With the shared justfile not loaded, only the locally defined recipes (`lint`, `audit`,
`grade*` at `justfile:6-25`) are available; running `just lint` immediately fails because
its dependency `fmt-check` (expected from the shared file) is undefined.
4. Any expectations that this repo exposes the standard Phenotype shared recipes (from
`phenotype-tooling/just/phenotype.just`) now fail on CI/other machines, leading to missing
tasks and inconsistent developer experience compared to the author's local environment
where the absolute path exists.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:**
*Code Quality: The imported shared justfile path is an absolute, developer-specific filesystem path, so it will not exist on CI/other developers' machines and shared recipes will silently not load. Replace it with a repository-relative path or another portable mechanism.
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 calls just ... lint from inside the lint recipe itself, which recursively invokes the same recipe and can loop until process/resource exhaustion. Call the underlying imported recipe directly (or rename the local wrapper recipe) instead of re-invoking lint. [logic error]
Severity Level: Major ⚠️
- ❌ `just lint` cannot complete; spawns infinite nested processes.
- ⚠️ Developers cannot rely on unified lint recipe via just.Steps of Reproduction ✅
1. Ensure the shared justfile imported at `justfile:3` defines a `fmt-check` recipe (this
is intended to live at `phenotype-tooling/just/phenotype.just`); from the repo root
`/workspace/phenoAI`, run `just lint`.
2. `just` loads the local `justfile` and resolves the `lint` recipe at `justfile:6`, first
running its dependency `fmt-check` from the imported shared justfile, then executing the
body `(just --justfile {{justfile_path()}} lint)`.
3. The body spawns a new `just` process with `--justfile {{justfile_path()}} lint`, which
again loads the same `justfile` and runs the same `lint` recipe, repeating step 2 and
spawning yet another `just` process.
4. This self-recursive invocation continues indefinitely, causing `just lint` to never
complete and to create an unbounded chain of nested `just` processes until the user
terminates it or the system hits resource limits.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 calls `just ... lint` from inside the `lint` recipe itself, which recursively invokes the same recipe and can loop until process/resource exhaustion. Call the underlying imported recipe directly (or rename the local wrapper recipe) instead of re-invoking `lint`.
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: The audit recipe re-invokes audit via just --justfile ... audit, which creates self-recursion and prevents the task from completing. Rename the wrapper or invoke the concrete underlying checks directly without calling audit again. [logic error]
Severity Level: Major ⚠️
- ❌ `just audit` never finishes; repeatedly re-runs itself.
- ⚠️ Automated dependency audits via just unusable in pipelines.Steps of Reproduction ✅
1. On a machine where the shared justfile imported at `justfile:3` exists and defines a
`deny` recipe, run `just audit` from `/workspace/phenoAI`.
2. `just` resolves the `audit` recipe at `justfile:9`, executing the first command `(just
--justfile {{justfile_path()}} deny)`, which runs the underlying `deny` audit check from
the shared justfile as intended.
3. Next, the same `audit` recipe executes `(just --justfile {{justfile_path()}} --justfile
{{justfile_path()}} audit)`, spawning a new `just` process pointed at the same local
`justfile` and again invoking the `audit` recipe.
4. The newly spawned `just` instance repeats step 2 and 3, re-running `deny` and spawning
yet another `audit` process, leading to unbounded self-recursion where `just audit` never
finishes and repeatedly re-invokes itself until interrupted.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: The `audit` recipe re-invokes `audit` via `just --justfile ... audit`, which creates self-recursion and prevents the task from completing. Rename the wrapper or invoke the concrete underlying checks directly without calling `audit` again.
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, so importing this adapter raises ModuleNotFoundError. Import from the parent package instead. [import error]
Severity Level: Critical 🚨
- ❌ Importing `ports.registry` fails due to adapter import error.
- ❌ `ports/tests/test_model_loader.py` cannot be imported or executed.
- ⚠️ Any future HuggingFace adapter usage breaks immediately on import.Steps of Reproduction ✅
1. From the repo root, run `pytest ports/tests/test_model_loader.py` which loads the test
module at `ports/tests/test_model_loader.py:1-5`.
2. The test file imports the registry with `from ..registry import resolve, register` at
`ports/tests/test_model_loader.py:5`, causing Python to import `ports.registry` defined in
`ports/registry.py:1-22`.
3. Inside `ports/registry.py`, module import executes the line `from .adapters.huggingface
import HuggingFaceLoader` at `ports/registry.py:3`, which in turn loads the adapter module
`ports/adapters/huggingface.py`.
4. When `ports/adapters/huggingface.py` is imported, line `from .model_loader import
ModelLoader, ModelRef, InferenceRequest, InferenceResponse` at
`ports/adapters/huggingface.py:2` tries to resolve `.model_loader` relative to the
`ports.adapters` package, i.e. `ports.adapters.model_loader`, but `LS` of
`/workspace/phenoAI/ports/adapters` shows only `huggingface.py` and `local.py` (no
`model_loader.py`), so Python raises `ModuleNotFoundError: No module named
'ports.adapters.model_loader'` and test import fails before any tests 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:** 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, so importing this adapter raises `ModuleNotFoundError`. Import from the parent package 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 documents HF URIs as hf://..., but transformers.from_pretrained expects a repo id/path (for example meta-llama/Llama-3.1-8B), not a custom scheme URI. Passing ref.uri directly will fail model/tokenizer loading for valid ModelRef inputs. [api mismatch]
Severity Level: Major ⚠️
- ❌ HuggingFace-backed ModelLoader fails to load configured models.
- ⚠️ Registry entry for `"hf://"` cannot successfully initialize adapters.
- ⚠️ Any feature relying on HF models via this port is unusable.Steps of Reproduction ✅
1. Construct a `ModelRef` for a HuggingFace model using the documented URI format at
`ports/model_loader.py:11-16`, for example `ref =
ModelRef(uri="hf://meta-llama/Llama-3.1-8B")`.
2. Obtain a loader for this scheme via the registry by calling `loader =
resolve("hf://meta-llama/Llama-3.1-8B")` using `resolve()` defined in
`ports/registry.py:12-17`, which picks `HuggingFaceLoader` from `_REGISTRY` at
`ports/registry.py:6-8` when the URI starts with `"hf://"`.
3. Call `await loader.load(ref)` which executes `HuggingFaceLoader.load()` in
`ports/adapters/huggingface.py:7-10`.
4. Inside `load()`, lines `self._tok = AutoTokenizer.from_pretrained(ref.uri,
revision=ref.revision)` and `self._model = AutoModelForCausalLM.from_pretrained(ref.uri,
revision=ref.revision)` at `ports/adapters/huggingface.py:9-10` pass the scheme-prefixed
string `"hf://meta-llama/Llama-3.1-8B"` directly to `transformers` APIs that expect a repo
id/path (e.g. `"meta-llama/Llama-3.1-8B"` or a filesystem path), causing model/tokenizer
loading to fail for otherwise valid `ModelRef` URIs.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:**
*Api Mismatch: `ModelRef` documents HF URIs as `hf://...`, but `transformers.from_pretrained` expects a repo id/path (for example `meta-llama/Llama-3.1-8B`), not a custom scheme URI. Passing `ref.uri` directly will fail model/tokenizer loading for valid `ModelRef` inputs.
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: This inference call ignores top_p and stop from InferenceRequest, so callers cannot control nucleus sampling or stopping behavior even though the port contract exposes those fields. Thread those request options into generation to match the interface semantics. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ `top_p` on `InferenceRequest` has no effect for HF.
- ⚠️ `stop` sequences are ignored by HuggingFace adapter.
- ⚠️ Port contract and adapter behavior are inconsistent for sampling.Steps of Reproduction ✅
1. Note that `InferenceRequest` in `ports/model_loader.py:18-24` defines `top_p: float =
0.95` and `stop: tuple[str, ...] = ()` as part of the public request contract.
2. Implement client code that constructs a request with non-default sampling and stop
behavior, e.g. `req = InferenceRequest(prompt="hi", max_tokens=10, temperature=0.2,
top_p=0.3, stop=("STOP",))` using the dataclass from `ports/model_loader.py:18-24`.
3. After obtaining and loading a `HuggingFaceLoader` instance (via `resolve()` in
`ports/registry.py:12-17` and `load()` in `ports/adapters/huggingface.py:7-10`), call
`await loader.infer(req)` which executes `HuggingFaceLoader.infer()` at
`ports/adapters/huggingface.py:12-16`.
4. Inside `infer()`, the generation call `self._model.generate(**inputs,
max_new_tokens=req.max_tokens, temperature=req.temperature, do_sample=req.temperature >
0)` at `ports/adapters/huggingface.py:14` threads only `max_tokens` and `temperature`,
ignoring `req.top_p` and `req.stop`, so nucleus sampling and stop sequences requested by
the caller are silently ignored and the response text will not respect these
interface-level parameters.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:**
*Incomplete Implementation: This inference call ignores `top_p` and `stop` from `InferenceRequest`, so callers cannot control nucleus sampling or stopping behavior even though the port contract exposes those fields. Thread those request options into generation to match the interface semantics.
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 uses the same wrong relative import depth as the HuggingFace adapter, so loading the local adapter will fail with ModuleNotFoundError before any logic runs. Change it to import model_loader from the parent package. [import error]
Severity Level: Critical 🚨
- ❌ LocalLoader cannot be imported due to bad relative import.
- ❌ `TestModelLoaderPort` in `ports/tests/test_model_loader.py` cannot run.
- ❌ Registry mapping `"local://"` in `ports/registry.py` becomes unusable.Steps of Reproduction ✅
1. From the repo root, run `pytest ports/tests/test_model_loader.py` which loads
`ports/tests/test_model_loader.py:1-5`.
2. The test file imports the local adapter with `from ..adapters.local import LocalLoader`
at `ports/tests/test_model_loader.py:4`, causing Python to import the module
`ports.adapters.local` from `ports/adapters/local.py:1-21`.
3. During import of `ports.adapters.local`, line `from .model_loader import ModelLoader,
ModelRef, InferenceRequest, InferenceResponse` at `ports/adapters/local.py:2` attempts to
resolve `.model_loader` relative to the `ports.adapters` package
(`ports.adapters.model_loader`).
4. A directory listing of `/workspace/phenoAI/ports/adapters` (via `LS`) shows only
`huggingface.py` and `local.py` and no `model_loader.py`, so Python raises
`ModuleNotFoundError: No module named 'ports.adapters.model_loader'`, causing the
LocalLoader import and all tests depending on it to fail before execution.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 uses the same wrong relative import depth as the HuggingFace adapter, so loading the local adapter will fail with `ModuleNotFoundError` before any logic runs. Change it to import `model_loader` from the parent package.
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
| from ..model_loader import ModelRef, InferenceRequest, InferenceResponse | ||
| from ..adapters.local import LocalLoader | ||
| from ..registry import resolve, register |
There was a problem hiding this comment.
Suggestion: These relative imports rely on the test module being imported as part of a package, but this repository has no package markers/config for ports/tests, so pytest -v can import this file as a top-level module and fail with "attempted relative import with no known parent package." Use absolute imports (for example from ports...) or add proper package setup so test discovery/import mode is stable. [import error]
Severity Level: Critical 🚨
- ❌ Python grade pipeline fails during pytest collection.
- ❌ Local pytest -v cannot import test module.
- ⚠️ Developers blocked from running new ModelLoader tests.Steps of Reproduction ✅
1. From the repository root `/workspace/phenoAI`, run the grading pipeline which invokes
pytest, for example: `./grade.sh python` (grade script at `grade.sh:96-105` calls `pytest
-v` at `grade.sh:98`).
2. During pytest collection, the test file `ports/tests/test_model_loader.py` (module path
`ports/tests/test_model_loader.py:1-53`) is discovered as a test module.
3. Python attempts to import this module; because neither `ports/` nor `ports/tests/`
contain an `__init__.py` (verified by `LS /workspace/phenoAI/ports` and missing
`/workspace/phenoAI/ports/__init__.py` and `/workspace/phenoAI/ports/tests/__init__.py`),
the test file is imported without a known parent package.
4. When executing `from ..model_loader import ModelRef, InferenceRequest,
InferenceResponse` at `ports/tests/test_model_loader.py:3`, Python resolves `..` relative
to a non-package context and raises `ImportError: attempted relative import with no known
parent package`, causing pytest collection to fail and the grade pipeline to abort.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/tests/test_model_loader.py
**Line:** 3:5
**Comment:**
*Import Error: These relative imports rely on the test module being imported as part of a package, but this repository has no package markers/config for `ports/tests`, so `pytest -v` can import this file as a top-level module and fail with "attempted relative import with no known parent package." Use absolute imports (for example from `ports...`) or add proper package setup so test discovery/import mode is stable.
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
|
CodeAnt AI finished reviewing your PR. |
|
Closing duplicate: PR #57 is the canonical v2 of this snapshot. |
1 participant
User description
Local main was ahead of origin. Branch snapshot for review.
Note
Medium Risk
Pre-push now runs the full grade pipeline (can block pushes) and the justfile depends on a developer-specific absolute import path; new inference adapters are stubby but load real HF weights when used.
Overview
Introduces a fleet-wide quality gate via new
grade.sh(stack-detected build/test/lint/coverage checks with--fast, JSON, and HTML reports), wired through just recipes (grade,grade-fast, etc.) and lefthook (scoped pre-commit lint/tests, conventional commit-msg, full grade on pre-push)..gitignorenow excludes.grade-reports/.The justfile is slimmed down: shared recipes are pulled from an optional
phenotype-toolingimport, with local overrides forlint,audit, and grade targets—replacing the previous inline build/test/ci recipes.Adds a new
ports/Python hexagonalModelLoader(datatypes + ABC), HuggingFace and local safetensors adapters, a URI-scheme registry, and pytest smoke tests (local loader lifecycle and registry resolution).Rust changes in
llm-router,mcp-server, andpheno-embeddingare formatting-only (no behavioral edits).Reviewed by Cursor Bugbot for commit 5117999. Bugbot is set up for automated code reviews on this repo. Configure here.
CodeAnt-AI Description
Add fleet-wide grading hooks and a pluggable model loader port
What Changed
ModelLoaderport with registry-based adapter lookup, plus HuggingFace and local safetensors loadersImpact
✅ Stricter push checks before code reaches shared branches✅ Quicker feedback on changed files during commits✅ Easier switching between local and HuggingFace model sources💡 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.