ENH: Add Claude Code project scaffolding and developer tooling #34
Open
aylward wants to merge 2 commits intoProject-MONAI:mainfrom
Open
ENH: Add Claude Code project scaffolding and developer tooling #34aylward wants to merge 2 commits intoProject-MONAI:mainfrom
aylward wants to merge 2 commits intoProject-MONAI:mainfrom
Conversation
WalkthroughAdds Claude Code agent and skill configs, developer guidance, API-map automation, worktree/PR utilities, and supporting docs/config so AI-assisted workflows and toolchains are tracked in-repo. Changes
Sequence Diagram(s)sequenceDiagram
participant Local as Local Script
participant GitHub as GitHub API
participant Claude as Claude CLI
participant FS as Filesystem
Local->>GitHub: fetch PR metadata, reviews, inline comments
GitHub-->>Local: PR data, comments, reviews
Local->>FS: compose prompt file (includes CLAUDE.md/AGENTS.md + diffs)
FS-->>Local: prompt saved
alt dry-run
Local->>FS: print prompt and exit
else invoke
Local->>Claude: pipe prompt via stdin (non‑interactive)
Claude-->>Local: generated review summary (stdout)
Local->>FS: write pr_<n>_review_summary.md
FS-->>Local: summary file present
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds Claude Code project scaffolding plus developer tooling to streamline AI-assisted contributions, including generated API indexing and a Windows utility for isolated feature worktrees.
Changes:
- Add Claude Code guidance files, subagents, and slash-command skills under
.claude/. - Add utilities:
utils/setup_feature_worktree.py(Windows worktree+venv setup) andutils/generate_api_map.py(AST-based API index generation). - Generate
docs/API_MAP.md, updateREADME.md, and add a pre-commit hook to keep the API map in sync.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
utils/setup_feature_worktree.py |
New Windows-oriented script to create a feature worktree, venv, install uv, and install deps. |
utils/generate_api_map.py |
New AST-based tool to generate a Markdown API index without importing modules. |
src/physiomotion4d/vtk_to_usd/CLAUDE.md |
Subpackage-specific guidance to keep vtk_to_usd internal and avoid misuse. |
docs/API_MAP.md |
Generated API map output to help locate public symbols quickly. |
README.md |
Adds an “AI-Assisted Development (Claude Code)” section describing workflows and included config. |
CLAUDE.md |
Updates global Claude Code guidance, commands, conventions, and workflow expectations. |
AGENTS.md |
Adds role-based guidance for implementation/testing/docs/architecture agents. |
.pre-commit-config.yaml |
Adds local hook intended to regenerate docs/API_MAP.md when .py files change. |
.gitignore |
Stops ignoring .claude so the Claude Code configuration can be committed. |
.claude/skills/test-feature/SKILL.md |
Adds /test-feature skill instructions for synthetic-data pytest coverage. |
.claude/skills/plan/SKILL.md |
Adds /plan skill instructions for inspection + implementation planning. |
.claude/skills/impl/SKILL.md |
Adds /impl skill instructions for small-diff implementation workflow. |
.claude/skills/doc-feature/SKILL.md |
Adds /doc-feature skill instructions for docstrings + API map refresh. |
.claude/agents/testing.md |
Adds testing subagent guidance and run commands. |
.claude/agents/implementation.md |
Adds implementation subagent process and code rules. |
.claude/agents/docs.md |
Adds docs subagent scope and docstring conventions. |
.claude/agents/architecture.md |
Adds architecture subagent output format and design invariants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.pre-commit-config.yaml
Outdated
| hooks: | ||
| - id: generate-api-map | ||
| name: Regenerate API map (docs/API_MAP.md) | ||
| entry: bash -c "python utils/generate_api_map.py && git add docs/API_MAP.md" |
There was a problem hiding this comment.
This hook uses bash -c ... and git add ..., which is not portable (especially on Windows) and can interact poorly with pre-commit’s stashing behavior. Prefer a cross-platform entry (e.g. python utils/generate_api_map.py) and let pre-commit fail if docs/API_MAP.md changes so the user can re-stage, rather than auto-staging inside the hook.
Suggested change
| entry: bash -c "python utils/generate_api_map.py && git add docs/API_MAP.md" | |
| entry: python utils/generate_api_map.py |
| - Backward compatibility is **not** a priority — breaking changes are acceptable | ||
| - Single quotes for strings; double quotes for docstrings | ||
| - Full type hints (`mypy` strict; `disallow_untyped_defs = true`) | ||
| - `Optional[X]` not `X | None` (ruff `UP007` suppressed) |
There was a problem hiding this comment.
This guidance says to use Optional[X] instead of X | None, but the existing codebase already uses | None in multiple places (e.g. src/physiomotion4d/usd_tools.py, src/physiomotion4d/test_tools.py). To avoid misleading contributors/agents, either relax this rule (e.g. “prefer Optional only where ITK typing requires it”) or align the codebase and enforcement with the stated convention.
Suggested change
| - `Optional[X]` not `X | None` (ruff `UP007` suppressed) | |
| - Prefer `X | None` over `Optional[X]`, except where third-party typing | |
| (e.g. ITK or mypy plugins) requires `Optional[X]` |
| - All classes inherit from `PhysioMotion4DBase`; new classes must too. | ||
| - Use `self.log_info()` / `self.log_debug()` — never `print()`. | ||
| - Single quotes for strings; double quotes for docstrings. 88-char line limit. | ||
| - Full type hints (`mypy` strict). Use `Optional[X]` not `X | None`. |
There was a problem hiding this comment.
This states “Use Optional[X] not X | None”, but the repository already uses PEP604 unions (| None) in several modules. Consider updating this guideline to match current practice (or, if Optional is truly required for ITK typing, scope the rule to those annotations) to prevent agents from making inconsistent style changes.
Suggested change
| - Full type hints (`mypy` strict). Use `Optional[X]` not `X | None`. | |
| - Full type hints (`mypy` strict). Prefer PEP 604 unions like `X | None` for optional types; avoid `Optional[X]` unless required for compatibility. |
utils/setup_feature_worktree.py
Outdated
|
|
||
| if branch_exists(branch_name): | ||
| print(f'[ERROR] Branch "{branch_name}" already exists locally.') | ||
| print(" Delete it with: git branch -D {branch_name}") |
There was a problem hiding this comment.
The branch deletion hint prints {branch_name} literally because this string is not an f-string. This is user-facing output and will be confusing when a branch already exists; make it an f-string (or otherwise interpolate branch_name).
Suggested change
| print(" Delete it with: git branch -D {branch_name}") | |
| print(f" Delete it with: git branch -D {branch_name}") |
Comment on lines
+196
to
+200
| slug = raw.lower() | ||
| slug = re.sub(r"[\s_]+", "-", slug) # spaces/underscores → hyphens | ||
| slug = re.sub(r"[^a-z0-9\-.]", "", slug) # remove unsafe chars | ||
| slug = re.sub(r"-{2,}", "-", slug) # collapse runs of hyphens | ||
| slug = slug.strip("-.") |
There was a problem hiding this comment.
sanitize_name() allows dots in the slug, which means it can emit Git-invalid branch names (e.g. input a..b -> feature/a..b). Consider validating branch_name with git check-ref-format --branch (or explicitly rejecting .., @{, trailing .lock, etc.) before attempting git worktree add so failures are caught with a clear error.
Comment on lines
+371
to
+389
| # pyproject.toml is present but we cannot assume a uv lockfile exists or | ||
| # that the project uses uv's project workflow (uv sync). The most robust | ||
| # fallback that works with any PEP 517 build backend is an editable install, | ||
| # which reads [project.dependencies] from pyproject.toml and installs them. | ||
| # If the caller truly wants a non-editable install, they should use | ||
| # --dependency-mode editable and adjust afterwards. | ||
| pyproject_file = worktree_path / "pyproject.toml" | ||
| if not pyproject_file.exists(): | ||
| print(f"[ERROR] pyproject.toml not found at: {pyproject_file}") | ||
| sys.exit(1) | ||
| print( | ||
| " (pyproject mode: using editable install to read [project.dependencies])" | ||
| ) | ||
| run( | ||
| [str(uv_exe), "pip", "install", "-e", "."], | ||
| cwd=worktree_path, | ||
| description="uv pip install -e . (pyproject)", | ||
| ) | ||
|
|
There was a problem hiding this comment.
pyproject and editable dependency modes both end up doing the same thing (uv pip install -e .). This makes --dependency-mode behavior harder to understand and the inline comment about “non-editable install” is contradictory. Either merge these modes, or make pyproject do something meaningfully different (e.g. uv sync when a uv.lock/poetry.lock exists) and update the help text accordingly.
Suggested change
| # pyproject.toml is present but we cannot assume a uv lockfile exists or | |
| # that the project uses uv's project workflow (uv sync). The most robust | |
| # fallback that works with any PEP 517 build backend is an editable install, | |
| # which reads [project.dependencies] from pyproject.toml and installs them. | |
| # If the caller truly wants a non-editable install, they should use | |
| # --dependency-mode editable and adjust afterwards. | |
| pyproject_file = worktree_path / "pyproject.toml" | |
| if not pyproject_file.exists(): | |
| print(f"[ERROR] pyproject.toml not found at: {pyproject_file}") | |
| sys.exit(1) | |
| print( | |
| " (pyproject mode: using editable install to read [project.dependencies])" | |
| ) | |
| run( | |
| [str(uv_exe), "pip", "install", "-e", "."], | |
| cwd=worktree_path, | |
| description="uv pip install -e . (pyproject)", | |
| ) | |
| # Prefer lockfile-based installation when possible: | |
| # - If uv.lock exists, use `uv sync` to install according to the lockfile. | |
| # - If poetry.lock exists, perform a non-editable project install. | |
| # - Otherwise, fall back to an editable install, which reads | |
| # [project.dependencies] from pyproject.toml and installs them. | |
| pyproject_file = worktree_path / "pyproject.toml" | |
| if not pyproject_file.exists(): | |
| print(f"[ERROR] pyproject.toml not found at: {pyproject_file}") | |
| sys.exit(1) | |
| uv_lock = worktree_path / "uv.lock" | |
| poetry_lock = worktree_path / "poetry.lock" | |
| if uv_lock.exists(): | |
| print(" (pyproject mode: detected uv.lock; running `uv sync`)") | |
| run( | |
| [str(uv_exe), "sync"], | |
| cwd=worktree_path, | |
| description="uv sync (pyproject)", | |
| ) | |
| elif poetry_lock.exists(): | |
| print( | |
| " (pyproject mode: detected poetry.lock; running " | |
| "`uv pip install .` for a non-editable project install)" | |
| ) | |
| run( | |
| [str(uv_exe), "pip", "install", "."], | |
| cwd=worktree_path, | |
| description="uv pip install . (pyproject, poetry.lock)", | |
| ) | |
| else: | |
| print( | |
| " (pyproject mode: no lockfile found; using editable install " | |
| "to read [project.dependencies])" | |
| ) | |
| run( | |
| [str(uv_exe), "pip", "install", "-e", "."], | |
| cwd=worktree_path, | |
| description="uv pip install -e . (pyproject, no lockfile)", | |
| ) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #34 +/- ##
=======================================
Coverage ? 13.70%
=======================================
Files ? 49
Lines ? 6575
Branches ? 0
=======================================
Hits ? 901
Misses ? 5674
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
utils/setup_feature_worktree.py (3)
282-309: Consider adding pip upgrade before installing uv.Older pip versions may have issues with newer packages. A
pip install --upgrade pipstep could improve reliability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/setup_feature_worktree.py` around lines 282 - 309, In install_uv, upgrade pip inside the venv before installing uv: after confirming venv_python exists in install_uv, call run([str(venv_python), "-m", "pip", "install", "--upgrade", "pip", "--quiet"], description="pip upgrade") (or similar) and only then run the existing pip install uv call; keep the same error handling (verify uv_exe exists and exit on failure) and reuse the venv_python/uv_exe symbols to locate the changes.
29-29: Consider using modern union syntax for consistency.The script uses
list[str](Python 3.9+) but importsOptionalfrom typing. Since Python 3.10+, you can usestr | Nonedirectly.♻️ Suggested change
-from typing import OptionalThen replace
Optional[str]withstr | NoneandOptional[Path]withPath | Nonethroughout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/setup_feature_worktree.py` at line 29, Update the typing to use modern union syntax: remove the Optional import from the top of utils/setup_feature_worktree.py and replace all usages of Optional[str] and Optional[Path] with str | None and Path | None respectively (search for any function signatures, variable annotations, and return types referencing Optional). Ensure imports still include Path from pathlib if used and run a quick type-check/flake8 to confirm no remaining Optional references.
317-335: Edge case:pyproject.tomlwithout[project.dependencies]falls back to editable install.The
detect_dependency_modefunction returns"pyproject"whenpyproject.tomlexists, but the subsequent editable install may fail if the project isn't properly configured. The current behavior is acceptable since the error will surface clearly, but you might document this edge case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/setup_feature_worktree.py` around lines 317 - 335, detect_dependency_mode currently returns "pyproject" solely based on the file's existence which can mislead when pyproject.toml has no declared dependencies; update the function to parse pyproject.toml and only return "pyproject" if it contains dependency declarations (check for keys like project.dependencies and tool.poetry.dependencies), otherwise fall back to "editable", and also add a brief docstring note explaining the edge case so callers know why a pyproject file may still result in an editable install.utils/generate_api_map.py (2)
234-242: Consider skippingsetup.pyorconftest.pyfiles.These files often contain public symbols that aren't part of the actual API. This is optional based on project conventions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/generate_api_map.py` around lines 234 - 242, Update find_python_files to exclude repository-level helper files like setup.py and conftest.py: inside the loop in the find_python_files function (and/or by adding them to SKIP_DIRS), check path.name and skip when it equals "setup.py" or "conftest.py" so those files are not appended to results; reference the existing SKIP_DIRS check and the path variable to implement the exclusion.
109-122: Consider handling augmented assignment for__all__.The function only checks
ast.Assign, but some codebases use__all__ += [...]or__all__.extend([...]). This is a minor edge case that likely won't affect most modules in this project.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/generate_api_map.py` around lines 109 - 122, module_all_names only inspects ast.Assign and misses augmented assignments and extend calls; update module_all_names to also handle ast.AugAssign where target is Name("__all__") and the op is ast.Add (merge string constants from the value list/tuple into the names set), and inspect ast.Expr nodes for Call expressions where the func is an Attribute whose value is Name("__all__") and attr is "extend" (collect string constants from the first arg if it is a list/tuple and merge them); ensure you accumulate into a single set (instead of returning on first find) and return that set or None if no __all__ occurrences are found..pre-commit-config.yaml (1)
30-38: Consider handling API map generation failures gracefully.The
&&chain means ifgenerate_api_map.pyfails,git addwon't run, which is correct. However, ifdocs/API_MAP.mddoesn't exist (e.g., first run or deleted),git addwill fail. Consider adding a check:♻️ More robust entry command
- entry: bash -c "python utils/generate_api_map.py && git add docs/API_MAP.md" + entry: bash -c "python utils/generate_api_map.py && [ -f docs/API_MAP.md ] && git add docs/API_MAP.md"Alternatively, the script already creates the parent directory and writes the file, so this may be fine in practice. The current implementation will still work since
generate_api_map.pycreates the file beforegit addruns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml around lines 30 - 38, The generate-api-map pre-commit hook's entry currently chains python utils/generate_api_map.py && git add docs/API_MAP.md which will fail if docs/API_MAP.md is missing; update the hook entry (id: generate-api-map) so after running utils/generate_api_map.py it checks for the generated file before attempting git add (or otherwise guard the git add with a conditional/ignore-failure), ensuring utils/generate_api_map.py still runs but git add only executes when docs/API_MAP.md exists.docs/API_MAP.md (1)
802-804: Generator output should align with markdownlint emphasis style.These lines produce MD049 warnings (
*modules*style). Since this file is generated, consider updatingutils/generate_api_map.pyto emit the repo’s expected emphasis style so lint noise doesn’t recur.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/API_MAP.md` around lines 802 - 804, The generated API map uses asterisks for emphasis which triggers MD049; update the generator (utils/generate_api_map.py) so the render path that builds entries for parse_module, find_python_files and render_markdown emits emphasis using the repo's expected style (use underscores, e.g. _modules_, or consistent style configured), i.e., change the string templates/formatting in the functions that produce those lines to use underscores instead of asterisks (or apply a single helper that normalizes emphasis) so all generated lines comply with markdownlint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/agents/architecture.md:
- Around line 12-24: The fenced code block in the architecture.md documentation
is missing a language token (triggering MD040); edit the fenced block that lists
the src/physiomotion4d files (the triple-backtick block containing
"src/physiomotion4d/" and the file list such as physiomotion4d_base.py,
segment_anatomy_base.py, etc.) and add a language identifier (e.g., change ```
to ```text) so the markdown linter no longer flags MD040.
In @.claude/agents/testing.md:
- Line 22: The pytest invocation references a likely non-existent function name
`test_extract_surface`; update the example command so it targets an actual test
node id from tests/test_contour_tools.py (for example use the class-based node
id pattern like `::TestContourTools::test_extract_surface_method_name` or the
fully qualified path to the test method) so the single-test command runs; locate
the example line that mentions `::test_extract_surface` and replace it with the
correct class/method node id (e.g., referencing `TestContourTools` and the
concrete test method name).
In `@AGENTS.md`:
- Around line 12-13: Update the blanket statement that "All classes inherit from
`PhysioMotion4DBase`" to clarify that only runtime components which require
built-in logging and lifecycle behavior should inherit from `PhysioMotion4DBase`
(e.g., major workflow, segmentation, registration, and other runtime tool
classes); leave data/container/helper/utility classes as plain classes or
dataclasses. Also keep the logging guidance to use `self.log_info()` /
`self.log_debug()` for classes that do inherit from `PhysioMotion4DBase`, and
add a short example or parenthetical list (workflow, segmentation, registration)
to make the intended scope explicit.
In `@CLAUDE.md`:
- Around line 53-55: The docs line contradicts the preferred Python invocation;
update the instruction that currently says "Regenerate it after any public API
change: `python utils/generate_api_map.py`" to use the project's chosen
convention (`py`) to match Line 7 (e.g., `py utils/generate_api_map.py`) so the
guidance in the CLAUDE.md file is consistent with the "use `py`" rule; ensure
the only change is the command string referencing the generator script.
- Around line 67-69: The README references a stale symbol "CompareImages";
update the documentation to reference the actual exported test utilities from
the module (replace "CompareImages" with "TestTools" or the correct public
symbol exposed by src/physiomotion4d/test_tools.py), and ensure the markers list
remains accurate; adjust the text so it mentions TestTools (and any specific
utilities it exposes) instead of the incorrect CompareImages name.
In `@README.md`:
- Around line 618-701: The fenced code blocks containing the command examples
(e.g., the blocks with "/plan add a confidence-weighted voting mode to
SegmentChestEnsemble", "/impl add set_regularization_weight() to
RegisterImagesANTs", "/impl fix the RAS-to-Y-up transform being applied twice in
vtk_to_usd/usd_utils.py", "/test-feature ..." examples, and "/doc-feature update
docstrings for RegisterImagesANTs" and "/plan redesign the segmentation return
type...") are unlabeled and trigger MD040; fix by adding a language identifier
(use "text") to each opening fence so they become "```text", updating each of
the unlabeled code fences listed in the diff to satisfy markdownlint.
In `@src/physiomotion4d/vtk_to_usd/CLAUDE.md`:
- Around line 7-10: The README guidance forbids external imports of
physiomotion4d.vtk_to_usd but the package currently exports that module in
src/physiomotion4d/vtk_to_usd/__init__.py and documents it in the README;
reconcile by either changing the CLAUDE.md phrasing to mark
physiomotion4d.vtk_to_usd as a "preferred" (not forbidden) entry point or by
removing/deprecating the public export and README references; update the text on
lines referenced (including 7-10, 26-28, 58-60) to use "preferred entry point"
language if keeping the export, or remove the module export from __init__.py and
add a deprecation notice in README and __init__.py if you intend to disallow
external imports.
In `@utils/setup_feature_worktree.py`:
- Around line 240-242: The second error print is not an f-string so
{branch_name} won't interpolate; update the print that currently reads "
Delete it with: git branch -D {branch_name}" to use f-string interpolation
(referencing the branch_name variable used earlier alongside the existing print
that mentions Branch "{branch_name}"), so the actual branch name is printed
before calling sys.exit(1).
---
Nitpick comments:
In @.pre-commit-config.yaml:
- Around line 30-38: The generate-api-map pre-commit hook's entry currently
chains python utils/generate_api_map.py && git add docs/API_MAP.md which will
fail if docs/API_MAP.md is missing; update the hook entry (id: generate-api-map)
so after running utils/generate_api_map.py it checks for the generated file
before attempting git add (or otherwise guard the git add with a
conditional/ignore-failure), ensuring utils/generate_api_map.py still runs but
git add only executes when docs/API_MAP.md exists.
In `@docs/API_MAP.md`:
- Around line 802-804: The generated API map uses asterisks for emphasis which
triggers MD049; update the generator (utils/generate_api_map.py) so the render
path that builds entries for parse_module, find_python_files and render_markdown
emits emphasis using the repo's expected style (use underscores, e.g. _modules_,
or consistent style configured), i.e., change the string templates/formatting in
the functions that produce those lines to use underscores instead of asterisks
(or apply a single helper that normalizes emphasis) so all generated lines
comply with markdownlint.
In `@utils/generate_api_map.py`:
- Around line 234-242: Update find_python_files to exclude repository-level
helper files like setup.py and conftest.py: inside the loop in the
find_python_files function (and/or by adding them to SKIP_DIRS), check path.name
and skip when it equals "setup.py" or "conftest.py" so those files are not
appended to results; reference the existing SKIP_DIRS check and the path
variable to implement the exclusion.
- Around line 109-122: module_all_names only inspects ast.Assign and misses
augmented assignments and extend calls; update module_all_names to also handle
ast.AugAssign where target is Name("__all__") and the op is ast.Add (merge
string constants from the value list/tuple into the names set), and inspect
ast.Expr nodes for Call expressions where the func is an Attribute whose value
is Name("__all__") and attr is "extend" (collect string constants from the first
arg if it is a list/tuple and merge them); ensure you accumulate into a single
set (instead of returning on first find) and return that set or None if no
__all__ occurrences are found.
In `@utils/setup_feature_worktree.py`:
- Around line 282-309: In install_uv, upgrade pip inside the venv before
installing uv: after confirming venv_python exists in install_uv, call
run([str(venv_python), "-m", "pip", "install", "--upgrade", "pip", "--quiet"],
description="pip upgrade") (or similar) and only then run the existing pip
install uv call; keep the same error handling (verify uv_exe exists and exit on
failure) and reuse the venv_python/uv_exe symbols to locate the changes.
- Line 29: Update the typing to use modern union syntax: remove the Optional
import from the top of utils/setup_feature_worktree.py and replace all usages of
Optional[str] and Optional[Path] with str | None and Path | None respectively
(search for any function signatures, variable annotations, and return types
referencing Optional). Ensure imports still include Path from pathlib if used
and run a quick type-check/flake8 to confirm no remaining Optional references.
- Around line 317-335: detect_dependency_mode currently returns "pyproject"
solely based on the file's existence which can mislead when pyproject.toml has
no declared dependencies; update the function to parse pyproject.toml and only
return "pyproject" if it contains dependency declarations (check for keys like
project.dependencies and tool.poetry.dependencies), otherwise fall back to
"editable", and also add a brief docstring note explaining the edge case so
callers know why a pyproject file may still result in an editable install.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1bc850f-b69a-4729-a807-be37e92fd5f2
📒 Files selected for processing (17)
.claude/agents/architecture.md.claude/agents/docs.md.claude/agents/implementation.md.claude/agents/testing.md.claude/skills/doc-feature/SKILL.md.claude/skills/impl/SKILL.md.claude/skills/plan/SKILL.md.claude/skills/test-feature/SKILL.md.gitignore.pre-commit-config.yamlAGENTS.mdCLAUDE.mdREADME.mddocs/API_MAP.mdsrc/physiomotion4d/vtk_to_usd/CLAUDE.mdutils/generate_api_map.pyutils/setup_feature_worktree.py
💤 Files with no reviewable changes (1)
- .gitignore
Comment on lines
+12
to
+13
| - All classes inherit from `PhysioMotion4DBase`; new classes must too. | ||
| - Use `self.log_info()` / `self.log_debug()` — never `print()`. |
There was a problem hiding this comment.
Narrow the inheritance rule to logging-enabled runtime classes.
Line 12 is too absolute. Not every class in this repo is intended to inherit from PhysioMotion4DBase (e.g., data/container/helper classes). Consider scoping this to major workflow/segmentation/registration tools to avoid incorrect refactors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` around lines 12 - 13, Update the blanket statement that "All
classes inherit from `PhysioMotion4DBase`" to clarify that only runtime
components which require built-in logging and lifecycle behavior should inherit
from `PhysioMotion4DBase` (e.g., major workflow, segmentation, registration, and
other runtime tool classes); leave data/container/helper/utility classes as
plain classes or dataclasses. Also keep the logging guidance to use
`self.log_info()` / `self.log_debug()` for classes that do inherit from
`PhysioMotion4DBase`, and add a short example or parenthetical list (workflow,
segmentation, registration) to make the intended scope explicit.
Comment on lines
+7
to
+10
| **Do not add calls to `vtk_to_usd` from outside this subpackage.** | ||
|
|
||
| The correct entry point for all VTK→USD conversion in PhysioMotion4D is: | ||
|
|
There was a problem hiding this comment.
This guidance conflicts with the currently exported public API.
These lines prohibit external use of physiomotion4d.vtk_to_usd, but the package currently exports that module-level API (src/physiomotion4d/vtk_to_usd/__init__.py) and README documents it as a supported option. Please reword to “preferred entry point” instead of “never import,” or deprecate/remove the public exports consistently.
Also applies to: 26-28, 58-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/physiomotion4d/vtk_to_usd/CLAUDE.md` around lines 7 - 10, The README
guidance forbids external imports of physiomotion4d.vtk_to_usd but the package
currently exports that module in src/physiomotion4d/vtk_to_usd/__init__.py and
documents it in the README; reconcile by either changing the CLAUDE.md phrasing
to mark physiomotion4d.vtk_to_usd as a "preferred" (not forbidden) entry point
or by removing/deprecating the public export and README references; update the
text on lines referenced (including 7-10, 26-28, 58-60) to use "preferred entry
point" language if keeping the export, or remove the module export from
__init__.py and add a deprecation notice in README and __init__.py if you intend
to disallow external imports.
**PR:** ENH: Add Claude Code project scaffolding and developer tooling
**Branch:** `claude_agents` -> `main`
**Reviewed:** 2026-03-28
**Comments processed:** 16 (2 PR-level, 14 inline)
| # | File | Line | Reviewer | Decision | Reasoning |
|---|------|------|----------|----------|-----------|
| 1 | *(PR overview)* | — | copilot | SKIPPED | Overview comment only; no actionable change |
| 2 | *(PR overview)* | — | coderabbitai | SKIPPED | Overview comment only; no actionable change |
| 3 | `.pre-commit-config.yaml` | 35 | copilot | APPLIED | Fixed `python` -> `py` per project convention (Windows launcher) |
| 4 | `CLAUDE.md` | 107 | copilot | REJECTED | Suggested `X \| None`; CLAUDE.md explicitly mandates `Optional[X]` (UP007 suppressed) |
| 5 | `AGENTS.md` | 15 | copilot | REJECTED | Same `Optional[X]` rule — project convention is unambiguous |
| 6 | `utils/setup_feature_worktree.py` | 241 | copilot | APPLIED | Fixed missing `f` prefix — `{branch_name}` was printing as a literal string |
| 7 | `utils/setup_feature_worktree.py` | 200 | copilot | REJECTED | Adding `git check-ref-format` validation is beyond stated scope of the script |
| 8 | `utils/setup_feature_worktree.py` | 389 | copilot | REJECTED | Lockfile detection is speculative; current editable-install fallback is intentional design |
| 9 | `.claude/agents/architecture.md` | 12 | coderabbitai | APPLIED | Added `text` language tag to unlabeled code fence (MD040) |
| 10 | `.claude/agents/testing.md` | 22 | coderabbitai | REVISED | `::test_extract_surface` does not exist; replaced with `::TestContourTools` (verified against test file) |
| 11 | `AGENTS.md` | 13 | coderabbitai | REJECTED | Narrowing the `Optional[X]` rule to specific contexts would diverge from CLAUDE.md, which states it universally |
| 12 | `CLAUDE.md` | 55 | coderabbitai | APPLIED | Fixed `python` -> `py` in API map regeneration command |
| 13 | `CLAUDE.md` | 69 | coderabbitai | APPLIED | Fixed stale class name `CompareImages` -> `TestTools` (confirmed by reading `test_tools.py`) |
| 14 | `README.md` | 701 | coderabbitai | APPLIED | Added `text` language tag to 7 unlabeled fenced code blocks (MD040) |
| 15 | `vtk_to_usd/CLAUDE.md` | 10 | coderabbitai | REJECTED | Strict wording is intentional; softening it risks agents bypassing `ConvertVTKToUSD` |
| 16 | `utils/setup_feature_worktree.py` | 242 | coderabbitai | SKIPPED | Duplicate of Project-MONAI#6; f-string fix already applied |
- **`.pre-commit-config.yaml`** — `python` -> `py` (Windows launcher consistency)
- **`utils/setup_feature_worktree.py:242`** — added missing `f` prefix to f-string; `{branch_name}` was printing as a literal
- **`.claude/agents/architecture.md`** — added `text` to unlabeled code fence (MD040 lint)
- **`.claude/agents/testing.md`** — replaced nonexistent test node ID `::test_extract_surface` with `::TestContourTools`
- **`CLAUDE.md:55`** — `python` -> `py` in API map regeneration command
- **`CLAUDE.md:69`** — `CompareImages` -> `TestTools` (stale class name)
- **`README.md`** — `text` tag added to 7 unlabeled fenced code blocks
- **`X | None` syntax (×2, copilot + coderabbitai):** CLAUDE.md explicitly mandates `Optional[X]`; ruff UP007 is suppressed project-wide. No exceptions.
- **`git check-ref-format` validation (copilot):** Speculative complexity for a utility script with a clear, narrow scope.
- **Lockfile/pip-upgrade detection (copilot):** The editable-install fallback is a documented intentional design choice, not an oversight.
- **`vtk_to_usd/CLAUDE.md` wording (coderabbitai):** The strict "never call this from outside `ConvertVTKToUSD`" language is a deliberate guardrail; weakening it undermines the purpose of the file.
- **`AGENTS.md` `Optional[X]` scoping (coderabbitai):** The rule is universal by design; scoping it to specific contexts would create ambiguity.
1. **`python` vs `py`** appeared in 3 files; all fixed. This is a recurring slip — the Windows launcher convention should be enforced by pre-commit or linting if it keeps appearing.
2. **`Optional[X]` vs `X | None`** was the dominant reviewer conflict. Both bots flagged it repeatedly. The project rule is clear and was correctly rejected each time, but it may be worth adding a note to `CONTRIBUTING` so external reviewers understand it is intentional.
3. **The f-string bug** (`{branch_name}` missing `f` prefix) was independently caught by both reviewers — a genuine defect. Worth adding a pre-commit check or test for this pattern in utility scripts.
4. **CodeRabbit nitpicks** consistently trended toward adding speculative complexity (lockfile variants, pip upgrade paths, extended validation) that conflicts with CLAUDE.md's explicit instruction to avoid handling impossible or out-of-scope scenarios.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
AGENTS.md (1)
22-23:⚠️ Potential issue | 🟡 MinorScope the
PhysioMotion4DBase/ logging rule to runtime classes.These bullets read as universal, but this repo already contains plain helper/data classes and standalone scripts that should not inherit from
PhysioMotion4DBaseor useself.log_*(). Left as-is, this will push agents toward incorrect refactors in utilities likeutils/generate_api_map.pyandutils/claude_github_reviews.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 22 - 23, Update the two bullets in AGENTS.md to apply only to runtime agent classes (e.g., classes that implement agent behavior and lifecycle) rather than universally; state that only runtime agent classes should inherit from PhysioMotion4DBase and use self.log_info()/self.log_debug(), and explicitly exclude plain helper/data classes and standalone scripts (for example generate_api_map and claude_github_reviews) from that requirement so maintainers don’t refactor utilities into the base class or replace prints in non-agent scripts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/API_MAP.md`:
- Around line 3-4: Update the header template in utils/generate_api_map.py so
the autogenerated API_MAP.md uses the Windows launcher convention ("py") instead
of "python": locate the header template string (e.g., HEADER_TEMPLATE or the
top-of-file template in utils/generate_api_map.py) and change the line that
renders "Re-run `python utils/generate_api_map.py` whenever public APIs change."
to use "py" (e.g., "Re-run `py utils/generate_api_map.py`..."), then re-run the
generator to regenerate docs/API_MAP.md.
In `@README.md`:
- Around line 679-680: Replace the inconsistent command string "python
utils/generate_api_map.py" with the Windows launcher form used elsewhere (e.g.,
"py -3 utils/generate_api_map.py") so the API-map step matches the commands in
CLAUDE.md and AGENTS.md; update the README.md line that currently contains
"python utils/generate_api_map.py" to use the "py -3" launcher variant.
In `@utils/claude_github_reviews.py`:
- Around line 271-276: The rejection rules in the screener that treat uses of
print() and non-PhysioMotion4DBase classes as automatic rejects are too broad;
update the code that builds/applies the "Rejection triggers" (the screener logic
that reads CLAUDE.md) to scope those specific checks only to logging-enabled
runtime components by: detect if a target file/module/class is a runtime
component with logging (e.g., has logger attributes or imports the project's
logging utilities, or is part of the runtime package) before applying the
print()/base-class rules, and skip these checks for standalone scripts,
helper/data types, or tests; adjust the functions that enumerate triggers (the
screener rule application code) to add this conditional gating so
print()/non-PhysioMotion4DBase checks run only when the file/module qualifies as
a logging-enabled runtime component.
- Around line 340-352: The subprocess.run invocation that calls "claude" should
fail fast on non-zero exits: update the call in utils/claude_github_reviews.py
(the try/except around subprocess.run) to either pass check=True or capture the
CompletedProcess and inspect .returncode and .stderr, and on non-zero return
print the actual error output and then call _save_prompt_fallback(prompt,
repo_root) and sys.exit(1); ensure the exception handler still catches
FileNotFoundError separately but that other failures surface immediately with
the claude CLI stderr included in the error message.
- Line 319: Remove Bash/shell capability from the tools enabled for Claude and
sanitize/escape all untrusted GitHub text before injecting into the prompt: stop
enabling the Bash tool in the agent creation call (the list passed when the
prompt/template at prompt_template/comments_block is sent to Claude) and instead
either remove "Bash" from the allowed tools list or use a safe toolset; also
ensure any use of parts.append(r["body"].strip()), the inline comment append
logic that builds comments_block, and any diff-hunk concatenation first escapes
or sanitizes special characters and control sequences (e.g., backticks, shell
metacharacters, newline-embedded directives) or canonicalizes them to plain text
so that untrusted review bodies cannot inject instructions into the prompt.
---
Duplicate comments:
In `@AGENTS.md`:
- Around line 22-23: Update the two bullets in AGENTS.md to apply only to
runtime agent classes (e.g., classes that implement agent behavior and
lifecycle) rather than universally; state that only runtime agent classes should
inherit from PhysioMotion4DBase and use self.log_info()/self.log_debug(), and
explicitly exclude plain helper/data classes and standalone scripts (for example
generate_api_map and claude_github_reviews) from that requirement so maintainers
don’t refactor utilities into the base class or replace prints in non-agent
scripts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 231df9fd-db34-4c64-9c60-df08f139638e
📒 Files selected for processing (10)
.claude/agents/architecture.md.claude/agents/testing.md.claude/settings.json.pre-commit-config.yamlAGENTS.mdCLAUDE.mdREADME.mddocs/API_MAP.mdutils/claude_github_reviews.pyutils/setup_feature_worktree.py
✅ Files skipped from review due to trivial changes (3)
- .claude/settings.json
- .claude/agents/testing.md
- .claude/agents/architecture.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .pre-commit-config.yaml
- CLAUDE.md
- utils/setup_feature_worktree.py
Comment on lines
+3
to
+4
| _Generated by `utils/generate_api_map.py`. Do not edit manually._ | ||
| _Re-run `python utils/generate_api_map.py` whenever public APIs change._ |
There was a problem hiding this comment.
Keep the autogenerated API-map instructions on py.
Line 4 reintroduces python utils/generate_api_map.py, which conflicts with the Windows launcher convention used elsewhere in this PR. Because this file is regenerated, please fix the header template in utils/generate_api_map.py rather than hand-editing the artifact.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/API_MAP.md` around lines 3 - 4, Update the header template in
utils/generate_api_map.py so the autogenerated API_MAP.md uses the Windows
launcher convention ("py") instead of "python": locate the header template
string (e.g., HEADER_TEMPLATE or the top-of-file template in
utils/generate_api_map.py) and change the line that renders "Re-run `python
utils/generate_api_map.py` whenever public APIs change." to use "py" (e.g.,
"Re-run `py utils/generate_api_map.py`..."), then re-run the generator to
regenerate docs/API_MAP.md.
Comment on lines
+679
to
+680
| Claude will update affected docstrings in NumPy style, add shape/axis annotations | ||
| where arrays are involved, and run `python utils/generate_api_map.py`. |
There was a problem hiding this comment.
Use the Windows launcher in the API-map step too.
Line 680 switches back to python utils/generate_api_map.py, while the rest of the new tooling docs standardize on py. Please keep this command consistent with CLAUDE.md and AGENTS.md.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 679 - 680, Replace the inconsistent command string
"python utils/generate_api_map.py" with the Windows launcher form used elsewhere
(e.g., "py -3 utils/generate_api_map.py") so the API-map step matches the
commands in CLAUDE.md and AGENTS.md; update the README.md line that currently
contains "python utils/generate_api_map.py" to use the "py -3" launcher variant.
Comment on lines
+271
to
+276
| Rejection triggers (from CLAUDE.md — treat these as hard rules): | ||
| - Introduces `X | None` instead of `Optional[X]` (ruff UP007 is suppressed) | ||
| - Adds backward-compat shims, re-exports, or removed-symbol stubs | ||
| - Adds error handling for internal states that cannot happen | ||
| - Uses `print()` instead of `self.log_info()` / `self.log_debug()` | ||
| - New class does not inherit from `PhysioMotion4DBase` |
There was a problem hiding this comment.
These rejection triggers are broader than the codebase.
Making print() and non-PhysioMotion4DBase classes automatic rejects will misclassify valid changes in standalone scripts and helper/data types. The screener should scope those rules to logging-enabled runtime components instead of treating them as universal.
🧭 Narrow the rule set
- - Uses `print()` instead of `self.log_info()` / `self.log_debug()`
- - New class does not inherit from `PhysioMotion4DBase`
+ - In classes that already inherit from `PhysioMotion4DBase`, prefer
+ `self.log_info()` / `self.log_debug()` over `print()`
+ - New runtime workflow / segmentation / registration classes should
+ inherit from `PhysioMotion4DBase`; helper/data/container classes need not📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Rejection triggers (from CLAUDE.md — treat these as hard rules): | |
| - Introduces `X | None` instead of `Optional[X]` (ruff UP007 is suppressed) | |
| - Adds backward-compat shims, re-exports, or removed-symbol stubs | |
| - Adds error handling for internal states that cannot happen | |
| - Uses `print()` instead of `self.log_info()` / `self.log_debug()` | |
| - New class does not inherit from `PhysioMotion4DBase` | |
| Rejection triggers (from CLAUDE.md — treat these as hard rules): | |
| - Introduces `X | None` instead of `Optional[X]` (ruff UP007 is suppressed) | |
| - Adds backward-compat shims, re-exports, or removed-symbol stubs | |
| - Adds error handling for internal states that cannot happen | |
| - In classes that already inherit from `PhysioMotion4DBase`, prefer | |
| `self.log_info()` / `self.log_debug()` over `print()` | |
| - New runtime workflow / segmentation / registration classes should | |
| inherit from `PhysioMotion4DBase`; helper/data/container classes need not |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/claude_github_reviews.py` around lines 271 - 276, The rejection rules
in the screener that treat uses of print() and non-PhysioMotion4DBase classes as
automatic rejects are too broad; update the code that builds/applies the
"Rejection triggers" (the screener logic that reads CLAUDE.md) to scope those
specific checks only to logging-enabled runtime components by: detect if a
target file/module/class is a runtime component with logging (e.g., has logger
attributes or imports the project's logging utilities, or is part of the runtime
package) before applying the print()/base-class rules, and skip these checks for
standalone scripts, helper/data types, or tests; adjust the functions that
enumerate triggers (the screener rule application code) to add this conditional
gating so print()/non-PhysioMotion4DBase checks run only when the file/module
qualifies as a logging-enabled runtime component.
|
|
||
| --- | ||
|
|
||
| {comments_block} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f -n claude_github_reviews.pyRepository: Project-MONAI/physiomotion4d
Length of output: 302
🏁 Script executed:
wc -l utils/claude_github_reviews.pyRepository: Project-MONAI/physiomotion4d
Length of output: 106
🏁 Script executed:
sed -n '310,350p' utils/claude_github_reviews.pyRepository: Project-MONAI/physiomotion4d
Length of output: 1417
🏁 Script executed:
sed -n '1,100p' utils/claude_github_reviews.py | head -50Repository: Project-MONAI/physiomotion4d
Length of output: 1743
🏁 Script executed:
sed -n '280,330p' utils/claude_github_reviews.pyRepository: Project-MONAI/physiomotion4d
Length of output: 1564
🏁 Script executed:
rg -n "comments_block\s*=" utils/claude_github_reviews.pyRepository: Project-MONAI/physiomotion4d
Length of output: 107
🏁 Script executed:
sed -n '220,280p' utils/claude_github_reviews.pyRepository: Project-MONAI/physiomotion4d
Length of output: 2748
🏁 Script executed:
sed -n '100,180p' utils/claude_github_reviews.pyRepository: Project-MONAI/physiomotion4d
Length of output: 2600
🏁 Script executed:
rg -n "review|comment" utils/claude_github_reviews.py | head -20Repository: Project-MONAI/physiomotion4d
Length of output: 1296
🏁 Script executed:
sed -n '140,230p' utils/claude_github_reviews.pyRepository: Project-MONAI/physiomotion4d
Length of output: 3070
🏁 Script executed:
sed -n '169,220p' utils/claude_github_reviews.pyRepository: Project-MONAI/physiomotion4d
Length of output: 1804
🏁 Script executed:
rg -n "escape|sanitize|filter" utils/claude_github_reviews.pyRepository: Project-MONAI/physiomotion4d
Length of output: 91
🏁 Script executed:
cat -n utils/claude_github_reviews.py | grep -A 5 -B 5 "r\[\"body\"\]"Repository: Project-MONAI/physiomotion4d
Length of output: 523
Remove Bash from Claude's allowed tools; untrusted GitHub reviewer text is injected verbatim into the prompt.
Review bodies are fetched directly from the GitHub API at lines 135–141 and inserted without any escaping or sanitization at line 182 (parts.append(r["body"].strip())). Similarly, inline comment bodies and diff hunks are appended at lines 212–217 without sanitization. This unescaped content is joined into comments_block (line 231) and injected into the prompt template at line 319, which is then passed to Claude with Bash capability enabled at line 342.
A malicious PR reviewer can craft comments that inject arbitrary instructions into the prompt. While Claude may have defenses against some injection attacks, combining untrusted external input with shell-capable agent execution is a critical security anti-pattern.
Recommended fix
- ["claude", "--print", "--allowedTools", "Read,Edit,Glob,Grep,Bash"],
+ ["claude", "--print", "--allowedTools", "Read,Edit,Glob,Grep"],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/claude_github_reviews.py` at line 319, Remove Bash/shell capability
from the tools enabled for Claude and sanitize/escape all untrusted GitHub text
before injecting into the prompt: stop enabling the Bash tool in the agent
creation call (the list passed when the prompt/template at
prompt_template/comments_block is sent to Claude) and instead either remove
"Bash" from the allowed tools list or use a safe toolset; also ensure any use of
parts.append(r["body"].strip()), the inline comment append logic that builds
comments_block, and any diff-hunk concatenation first escapes or sanitizes
special characters and control sequences (e.g., backticks, shell metacharacters,
newline-embedded directives) or canonicalizes them to plain text so that
untrusted review bodies cannot inject instructions into the prompt.
Comment on lines
+340
to
+352
| try: | ||
| subprocess.run( | ||
| ["claude", "--print", "--allowedTools", "Read,Edit,Glob,Grep,Bash"], | ||
| input=prompt, | ||
| text=True, | ||
| encoding="utf-8", | ||
| cwd=repo_root, | ||
| ) | ||
| except FileNotFoundError: | ||
| print('[ERROR] "claude" CLI not found.') | ||
| print(" Install Claude Code: https://claude.ai/code") | ||
| _save_prompt_fallback(prompt, repo_root) | ||
| sys.exit(1) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the file around the specified lines
cat -n utils/claude_github_reviews.py | sed -n '335,360p'Repository: Project-MONAI/physiomotion4d
Length of output: 1214
🏁 Script executed:
# Check the broader context of the invoke_claude function
cat -n utils/claude_github_reviews.py | sed -n '310,365p'Repository: Project-MONAI/physiomotion4d
Length of output: 2276
🏁 Script executed:
# Search for where the summary file is checked to confirm the masking issue
rg "summary.*file|summary.*exist" -A 3 -B 3 utils/claude_github_reviews.pyRepository: Project-MONAI/physiomotion4d
Length of output: 1531
Fail fast on Claude command failures.
If claude exits non-zero here, the script continues and later only reports a missing summary file, masking the actual failure. Use check=True or inspect returncode so invalid flags and tool errors stop immediately with the real error message.
🛠️ Propagate `claude` failures
try:
subprocess.run(
["claude", "--print", "--allowedTools", "Read,Edit,Glob,Grep,Bash"],
input=prompt,
text=True,
encoding="utf-8",
cwd=repo_root,
+ check=True,
)
except FileNotFoundError:
print('[ERROR] "claude" CLI not found.')
print(" Install Claude Code: https://claude.ai/code")
_save_prompt_fallback(prompt, repo_root)
sys.exit(1)
+ except subprocess.CalledProcessError as exc:
+ print(f'[ERROR] "claude" exited with status {exc.returncode}.')
+ sys.exit(exc.returncode)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/claude_github_reviews.py` around lines 340 - 352, The subprocess.run
invocation that calls "claude" should fail fast on non-zero exits: update the
call in utils/claude_github_reviews.py (the try/except around subprocess.run) to
either pass check=True or capture the CompletedProcess and inspect .returncode
and .stderr, and on non-zero return print the actual error output and then call
_save_prompt_fallback(prompt, repo_root) and sys.exit(1); ensure the exception
handler still catches FileNotFoundError separately but that other failures
surface immediately with the claude CLI stderr included in the error message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores