fix runtime error printing #1129
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 44 minutes and 30 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (22)
WalkthroughThis PR refactors error handling across dfetch commands to provide per-project error context instead of accumulating and displaying errors at the end. The ChangesPer-Project Error Handling Refactor
Documentation Build System Enhancements
Sequence Diagram(s)sequenceDiagram
participant Command as Diff command loop
participant Helper as _diff_project
participant SuperProject
participant Filesystem
Command->>Helper: try _diff_project(superproject, project)
Helper->>Helper: validate destination exists
Helper->>SuperProject: get_sub_project(project.name)
Helper->>Helper: derive old_rev from metadata if needed
Helper->>SuperProject: diff(subproject, old_rev, new_rev)
SuperProject-->>Helper: patch content
Helper->>Filesystem: write project.patch if non-empty
Helper-->>Command: return (success) or RuntimeError
Command->>Command: except: log error, set had_errors, continue
sequenceDiagram
participant Command as UpdatePatch command loop
participant Helper as _process_project
participant SuperProject
participant PatchFile
Command->>Helper: try _process_project(superproject, project)
Helper->>Helper: validate preconditions (patch exists, fetched, no local changes)
Helper->>SuperProject: metadata_force_update
Helper->>SuperProject: generate reverse diff
Helper->>PatchFile: _update_patch with reverse diff
Helper->>SuperProject: metadata_force_update to apply patch
Helper-->>Command: return (success) or RuntimeError
Command->>Command: except: log error, set had_errors, continue
flowchart TD
A["ScenarioIncludeDirective.run()"]
B["Register appendix entry metadata"]
C["Emit ScenarioIncludePlaceholder node"]
D["doctree-resolved phase"]
E{"HTML or inline build?"}
F["Render inline HTML details block"]
G{"PDF/LaTeX build?"}
H["Emit ScenarioAppendixRef node"]
I["process_scenario_appendix<br/>builds grouped appendix sections"]
A --> B
B --> C
C --> D
D --> E
E -->|Yes| F
E -->|No| G
G -->|Yes| H
H --> I
F --> I
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dfetch/commands/freeze.py (1)
87-111:⚠️ Potential issue | 🟡 MinorInconsistent error handling pattern compared to other commands.
Unlike
check.py,update.py,diff.py, andupdate_patch.py, this command does not trackhad_errorsor raiseRuntimeError()at the end when projects fail. The warning is logged, but the command exits successfully even if some projects failed to freeze.If this is intentional (e.g., to allow partial freezes), consider adding a comment explaining the design choice. Otherwise, consider aligning with the pattern used in other commands:
🔧 Proposed fix to align with other commands
def __call__(self, args: argparse.Namespace) -> None: """Perform the freeze.""" del args # unused superproject = create_super_project() projects: list[ProjectEntry] = [] + had_errors: bool = False with in_directory(superproject.root_directory): for project in superproject.manifest.projects: try: # ... existing logic ... projects.append(project) except RuntimeError as exc: logger.print_warning_line(project.name, str(exc)) + had_errors = True # ... manifest creation and dump ... + + if had_errors: + raise RuntimeError()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/freeze.py` around lines 87 - 111, The freeze command currently logs RuntimeError per-project but does not set/fail the overall command like other commands (check.py, update.py, diff.py, update_patch.py); update the freeze implementation (the loop that calls dfetch.project.create_sub_project(...), sub_project.freeze_project(...), and the except RuntimeError as exc block) to follow the same pattern: introduce/maintain a had_errors flag, set had_errors = True when catching a RuntimeError, and after processing all projects raise RuntimeError() (or rethrow) if had_errors is True so the command exits non‑zero; if the current behavior is intentional, add a brief explanatory comment above that try/except to document why partial failures should not fail the command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@dfetch/commands/freeze.py`:
- Around line 87-111: The freeze command currently logs RuntimeError per-project
but does not set/fail the overall command like other commands (check.py,
update.py, diff.py, update_patch.py); update the freeze implementation (the loop
that calls dfetch.project.create_sub_project(...),
sub_project.freeze_project(...), and the except RuntimeError as exc block) to
follow the same pattern: introduce/maintain a had_errors flag, set had_errors =
True when catching a RuntimeError, and after processing all projects raise
RuntimeError() (or rethrow) if had_errors is True so the command exits non‑zero;
if the current behavior is intentional, add a brief explanatory comment above
that try/except to document why partial failures should not fail the command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 539e8463-5a41-40ab-9401-785f53c2026c
📒 Files selected for processing (41)
CHANGELOG.rstdfetch/__init__.pydfetch/commands/check.pydfetch/commands/diff.pydfetch/commands/format_patch.pydfetch/commands/freeze.pydfetch/commands/update.pydfetch/commands/update_patch.pydfetch/util/util.pyfeatures/check-archive.featurefeatures/check-git-repo.featurefeatures/check-specific-projects.featurefeatures/check-svn-repo.featurefeatures/checked-project-has-dependencies.featurefeatures/diff-in-git.featurefeatures/diff-in-svn.featurefeatures/fetch-archive.featurefeatures/fetch-checks-destination.featurefeatures/fetch-file-pattern-git.featurefeatures/fetch-file-pattern-svn.featurefeatures/fetch-git-repo-with-submodule.featurefeatures/fetch-git-repo.featurefeatures/fetch-single-file-git.featurefeatures/fetch-single-file-svn.featurefeatures/fetch-with-ignore-git.featurefeatures/fetch-with-ignore-svn.featurefeatures/guard-against-overwriting-git.featurefeatures/guard-against-overwriting-svn.featurefeatures/handle-invalid-metadata.featurefeatures/journey-basic-patching.featurefeatures/journey-basic-usage.featurefeatures/list-projects.featurefeatures/patch-after-fetch-git.featurefeatures/patch-after-fetch-svn.featurefeatures/patch-fuzzy-matching-git.featurefeatures/report-sbom.featurefeatures/suggest-project-name.featurefeatures/update-patch-in-git.featurefeatures/update-patch-in-svn.featurefeatures/updated-project-has-dependencies.featurefeatures/validate-manifest.feature
💤 Files with no reviewable changes (1)
- dfetch/util/util.py
spoorcc
force-pushed
the
claude/fix-runtime-error-printing-Vayam
branch
from
037049d to
e59cfaa
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dfetch/commands/freeze.py (1)
112-140:⚠️ Potential issue | 🟠 MajorRestore a failing exit status when any project freeze fails.
Line 135 now catches and logs per-project
RuntimeErrors, but this command never records that a failure happened or raises after the loop. A run that freezes some projects and fails others will therefore exit successfully, which hides partial failures from CI/scripts and can leave a partially updated manifest reported as success.💡 Suggested fix
def __call__(self, args: argparse.Namespace) -> None: """Perform the freeze.""" superproject = create_super_project() make_backup = isinstance(superproject, NoVcsSuperProject) manifest_updated = False + had_errors = False with in_directory(superproject.root_directory): manifest_path = superproject.manifest.path projects_to_freeze = superproject.manifest.selected_projects(args.projects) @@ except RuntimeError as exc: logger.print_warning_line(project.name, str(exc)) + had_errors = True if manifest_updated: superproject.manifest.dump() logger.info(f"Updated manifest ({manifest_path}) in {os.getcwd()}") + + if had_errors: + raise RuntimeError()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/freeze.py` around lines 112 - 140, The loop currently swallows per-project RuntimeError in the except block (except RuntimeError as exc) without recording failure, so partial failures return success; modify the except to set a failure flag (e.g., had_errors = True) when catching RuntimeError (and still log the warning), then after the loop check the flag and if True call sys.exit(1) (or raise a RuntimeError) to restore a non-zero exit status; reference the except RuntimeError as exc block, the manifest_updated variable/superproject.manifest.dump() post-loop, and ensure the new had_errors flag is used to decide exit after any manifest updates are handled.
🤖 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/settings.json:
- Around line 1-27: PR metadata and changed file contents are inconsistent: the
PR title/description mention "fix runtime error printing vayam" and
runtime/version changes, but the only changed file is .claude/settings.json (the
"permissions.allow" list). Update the PR by either (a) adding the intended code
changes related to runtime error handling/version bump (ensure relevant files
and functions are included) or (b) editing the PR title and description to
accurately describe this change to Claude permissions; confirm the actual diff
includes the expected symbols (e.g., the runtime error handling functions or
version bump commits) before marking the PR ready, and mention
.claude/settings.json in the PR body if you keep only this change.
- Line 12: The allowlist entry "Bash(pip install:*)" is too permissive; replace
it with scoped pip patterns that limit what can be installed — e.g., allow only
editable local installs ("Bash(pip install -e .:*)"), or explicit package
installs with pinned versions ("Bash(pip install some-package==1.2.3:*)"), or
installs to a restricted directory ("Bash(pip install --target=./venv
some-package==1.2.3:*)"); update the entry that currently contains "Bash(pip
install:*)" to one or more of these safer patterns and remove the wildcard form
to prevent arbitrary package execution.
---
Outside diff comments:
In `@dfetch/commands/freeze.py`:
- Around line 112-140: The loop currently swallows per-project RuntimeError in
the except block (except RuntimeError as exc) without recording failure, so
partial failures return success; modify the except to set a failure flag (e.g.,
had_errors = True) when catching RuntimeError (and still log the warning), then
after the loop check the flag and if True call sys.exit(1) (or raise a
RuntimeError) to restore a non-zero exit status; reference the except
RuntimeError as exc block, the manifest_updated
variable/superproject.manifest.dump() post-loop, and ensure the new had_errors
flag is used to decide exit after any manifest updates are handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54ab4938-9689-4ccf-a91f-e832c8000ffb
📒 Files selected for processing (44)
.claude/settings.jsonCHANGELOG.rstdfetch/__init__.pydfetch/commands/check.pydfetch/commands/diff.pydfetch/commands/format_patch.pydfetch/commands/freeze.pydfetch/commands/update.pydfetch/commands/update_patch.pydfetch/util/util.pyfeatures/check-archive.featurefeatures/check-git-repo.featurefeatures/check-specific-projects.featurefeatures/check-svn-repo.featurefeatures/checked-project-has-dependencies.featurefeatures/diff-in-git.featurefeatures/diff-in-svn.featurefeatures/fetch-archive.featurefeatures/fetch-checks-destination.featurefeatures/fetch-file-pattern-git.featurefeatures/fetch-file-pattern-svn.featurefeatures/fetch-git-repo-with-submodule.featurefeatures/fetch-git-repo.featurefeatures/fetch-single-file-git.featurefeatures/fetch-single-file-svn.featurefeatures/fetch-with-ignore-git.featurefeatures/fetch-with-ignore-svn.featurefeatures/guard-against-overwriting-git.featurefeatures/guard-against-overwriting-svn.featurefeatures/handle-invalid-metadata.featurefeatures/journey-basic-patching.featurefeatures/journey-basic-usage.featurefeatures/list-projects.featurefeatures/patch-after-fetch-git.featurefeatures/patch-after-fetch-svn.featurefeatures/patch-fuzzy-matching-git.featurefeatures/report-sbom.featurefeatures/suggest-project-name.featurefeatures/update-patch-in-git.featurefeatures/update-patch-in-svn.featurefeatures/updated-project-has-dependencies.featurefeatures/validate-manifest.featuretests/test_report.pytests/test_stdout_reporter.py
💤 Files with no reviewable changes (1)
- dfetch/util/util.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/settings.json:
- Line 12: The entry "Bash(git update-index:*)" grants overly broad git
update-index privileges; locate the string "Bash(git update-index:*)" in
settings.json and either remove it if unused or replace the wildcard with an
explicit, minimal set of allowed invocations (e.g., enumerate the specific
flags/arguments your code actually needs) so the permission scope is constrained
and auditable; update the settings to list only those exact forms instead of
using "*" and save the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1206ce9-e287-40a4-a8f8-39712d56d059
📒 Files selected for processing (4)
.claude/settings.jsonfeatures/check-git-repo.featurefeatures/check-svn-repo.featurefeatures/patch-after-fetch-svn.feature
spoorcc
force-pushed
the
claude/fix-runtime-error-printing-Vayam
branch
from
30b4025 to
74cf207
Compare
ben-edna
force-pushed
the
claude/fix-runtime-error-printing-Vayam
branch
from
74cf207 to
9578b85
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dfetch/commands/freeze.py (1)
111-139:⚠️ Potential issue | 🟠 MajorPreserve a failing exit status when any project cannot be frozen.
This handler only logs the
RuntimeError. After this change,dfetch freezereturns success even when one or more projects failed, which hides partial failures from scripts/CI and can still leave a partially updated manifest on disk. Track ahad_errorsflag and raise after the loop, like the other commands in this PR.Suggested fix
def __call__(self, args: argparse.Namespace) -> None: """Perform the freeze.""" superproject = create_super_project() make_backup = isinstance(superproject, NoVcsSuperProject) manifest_updated = False + had_errors: bool = False with in_directory(superproject.root_directory): manifest_path = superproject.manifest.path projects_to_freeze = superproject.manifest.selected_projects(args.projects) @@ except RuntimeError as exc: logger.print_warning_line(project.name, str(exc)) + had_errors = True if manifest_updated: superproject.manifest.dump() logger.info(f"Updated manifest ({manifest_path}) in {os.getcwd()}") + + if had_errors: + raise RuntimeError()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/freeze.py` around lines 111 - 139, The loop over projects_to_freeze currently catches RuntimeError and only logs it, causing dfetch freeze to exit success even on failures; add a had_errors boolean (initialized False) and set it True inside the except RuntimeError exc handler (where logger.print_warning_line(project.name, str(exc)) is called), then after the loop (after potential superproject.manifest.dump()) raise a RuntimeError or exit non‑zero if had_errors is True to preserve failing exit status; reference the loop that calls dfetch.project.create_sub_project, sub_project.freeze_project, the manifest_updated logic, and superproject.manifest.dump when adding the had_errors flag and final check.dfetch/commands/check.py (1)
107-118:⚠️ Potential issue | 🟠 MajorWrap the sub-manifest scan in the same project-scoped
tryblock.Right now only
check_for_update()is caught. Ifcheck_sub_manifests()hits an invalid nesteddfetch.yaml, theRuntimeErrorraised by manifest parsing still escapes the loop, so later projects are skipped and this failure path never goes throughhad_errors. That makesdfetch checkinconsistent with the new per-project error handling introduced elsewhere in this PR.Suggested fix
had_errors: bool = False for project in superproject.manifest.selected_projects(args.projects): try: dfetch.project.create_sub_project(project).check_for_update( reporters, files_to_ignore=superproject.ignored_files(project.destination), ) + if not args.no_recommendations and os.path.isdir(project.destination): + with in_directory(project.destination): + check_sub_manifests(superproject.manifest, project) except RuntimeError as exc: logger.print_warning_line(project.name, str(exc)) had_errors = True - - if not args.no_recommendations and os.path.isdir(project.destination): - with in_directory(project.destination): - check_sub_manifests(superproject.manifest, project)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/check.py` around lines 107 - 118, The call to check_sub_manifests(superproject.manifest, project) must be executed inside the same try/except that wraps dfetch.project.create_sub_project(project).check_for_update(...) so that any RuntimeError from parsing nested dfetch.yaml is caught, logged, and sets had_errors; move the if-block (or wrap its body) into the existing try and in the except RuntimeError as exc handler call logger.print_warning_line(project.name, str(exc)) and set had_errors = True just like for check_for_update to ensure consistent per-project error handling.
🤖 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/settings.json:
- Around line 8-10: The entries "Bash(git add:*)" and "Bash(git pull:*)" in
.claude/settings.json are too broad; tighten these scopes by replacing the
wildcard patterns with explicit, least-privilege command patterns or removing
them entirely—for example restrict to specific files or exact subcommands you
need (reference the exact strings "Bash(git add:*)" and "Bash(git pull:*)") or
replace with safer alternatives like only allowing "Bash(git fetch:*)" which is
already present; ensure the new entries enumerate allowed targets or exact
commands to prevent nondeterministic workspace mutations.
In `@dfetch/commands/update_patch.py`:
- Around line 90-97: The loop currently catches RuntimeError from
self._process_project(superproject, project), logs via logger.print_warning_line
and sets had_errors to continue; change this so patch failures stop immediately:
remove the try/except (or after calling logger.print_warning_line re-raise the
same RuntimeError) so that RuntimeError from _process_project (and underlying
apply_patch) propagates immediately instead of being deferred by had_errors;
leave the final had_errors block unreachable/adjust as needed. Ensure
references: _process_project, logger.print_warning_line, had_errors, and
apply_patch.
---
Outside diff comments:
In `@dfetch/commands/check.py`:
- Around line 107-118: The call to check_sub_manifests(superproject.manifest,
project) must be executed inside the same try/except that wraps
dfetch.project.create_sub_project(project).check_for_update(...) so that any
RuntimeError from parsing nested dfetch.yaml is caught, logged, and sets
had_errors; move the if-block (or wrap its body) into the existing try and in
the except RuntimeError as exc handler call
logger.print_warning_line(project.name, str(exc)) and set had_errors = True just
like for check_for_update to ensure consistent per-project error handling.
In `@dfetch/commands/freeze.py`:
- Around line 111-139: The loop over projects_to_freeze currently catches
RuntimeError and only logs it, causing dfetch freeze to exit success even on
failures; add a had_errors boolean (initialized False) and set it True inside
the except RuntimeError exc handler (where
logger.print_warning_line(project.name, str(exc)) is called), then after the
loop (after potential superproject.manifest.dump()) raise a RuntimeError or exit
non‑zero if had_errors is True to preserve failing exit status; reference the
loop that calls dfetch.project.create_sub_project, sub_project.freeze_project,
the manifest_updated logic, and superproject.manifest.dump when adding the
had_errors flag and final check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d6c1aca-37b4-41c5-abf9-fc46b333f2ba
📒 Files selected for processing (44)
.claude/settings.jsonCHANGELOG.rstdfetch/__init__.pydfetch/commands/check.pydfetch/commands/diff.pydfetch/commands/format_patch.pydfetch/commands/freeze.pydfetch/commands/update.pydfetch/commands/update_patch.pydfetch/util/util.pyfeatures/check-archive.featurefeatures/check-git-repo.featurefeatures/check-specific-projects.featurefeatures/check-svn-repo.featurefeatures/checked-project-has-dependencies.featurefeatures/diff-in-git.featurefeatures/diff-in-svn.featurefeatures/fetch-archive.featurefeatures/fetch-checks-destination.featurefeatures/fetch-file-pattern-git.featurefeatures/fetch-file-pattern-svn.featurefeatures/fetch-git-repo-with-submodule.featurefeatures/fetch-git-repo.featurefeatures/fetch-single-file-git.featurefeatures/fetch-single-file-svn.featurefeatures/fetch-with-ignore-git.featurefeatures/fetch-with-ignore-svn.featurefeatures/guard-against-overwriting-git.featurefeatures/guard-against-overwriting-svn.featurefeatures/handle-invalid-metadata.featurefeatures/journey-basic-patching.featurefeatures/journey-basic-usage.featurefeatures/list-projects.featurefeatures/patch-after-fetch-git.featurefeatures/patch-after-fetch-svn.featurefeatures/patch-fuzzy-matching-git.featurefeatures/report-sbom.featurefeatures/suggest-project-name.featurefeatures/update-patch-in-git.featurefeatures/update-patch-in-svn.featurefeatures/updated-project-has-dependencies.featurefeatures/validate-manifest.featuretests/test_report.pytests/test_stdout_reporter.py
💤 Files with no reviewable changes (1)
- dfetch/util/util.py
Comment on lines
+90
to
+97
| try: | ||
| self._process_project(superproject, project) | ||
| except RuntimeError as exc: | ||
| logger.print_warning_line(project.name, str(exc)) | ||
| had_errors = True | ||
|
|
||
| if had_errors: | ||
| raise RuntimeError() |
Contributor
There was a problem hiding this comment.
Do not continue to the next project after a patch-update failure.
_process_project() does two forced subproject.update() calls and may overwrite the patch file before the final re-apply. Catching RuntimeError here and then moving on to other projects means a failed patch operation can leave the repo in an intermediate state while the command keeps mutating more projects. For this command, log the project-scoped warning and re-raise immediately instead of deferring failure until the end.
Suggested fix
with in_directory(superproject.root_directory):
for project in superproject.manifest.selected_projects(args.projects):
try:
self._process_project(superproject, project)
except RuntimeError as exc:
logger.print_warning_line(project.name, str(exc))
- had_errors = True
-
- if had_errors:
- raise RuntimeError()
+ raiseBased on learnings, "patch application failures should fail immediately by letting RuntimeError propagate from apply_patch() rather than catching and continuing with remaining patches. The project prefers explicit failures for patch operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/commands/update_patch.py` around lines 90 - 97, The loop currently
catches RuntimeError from self._process_project(superproject, project), logs via
logger.print_warning_line and sets had_errors to continue; change this so patch
failures stop immediately: remove the try/except (or after calling
logger.print_warning_line re-raise the same RuntimeError) so that RuntimeError
from _process_project (and underlying apply_patch) propagates immediately
instead of being deferred by had_errors; leave the final had_errors block
unreachable/adjust as needed. Ensure references: _process_project,
logger.print_warning_line, had_errors, and apply_patch.
Contributor
Author
There was a problem hiding this comment.
patch application failures should fail immediately within a subproject, patch application can continue for other subprojects
Contributor
There was a problem hiding this comment.
@spoorcc, understood — the failure should be immediate within a subproject (i.e., _process_project exits on its first RuntimeError without attempting further steps for that same project), but the outer loop correctly continues on to the remaining subprojects. The current implementation already does exactly that. My suggestion was wrong. I'll update the learning to reflect the correct scope.
✏️ Learnings added
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 1129
File: dfetch/commands/update_patch.py:90-97
Timestamp: 2026-04-13T17:26:12.856Z
Learning: In dfetch's patch-related commands (e.g., dfetch/commands/update_patch.py), a RuntimeError during processing of a single subproject should cause that subproject's processing to stop immediately (fail fast within the subproject), but the outer loop should continue processing remaining subprojects. After the loop, a bare RuntimeError() is raised if any project failed. This is the intentional design: per-subproject failure isolation, not abort-all-on-first-error.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
⛔ Learnings removed
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 942
File: dfetch/project/subproject.py:143-166
Timestamp: 2026-01-09T23:16:35.480Z
Learning: In dfetch project's dfetch/project/subproject.py, patch application failures should fail immediately by letting RuntimeError propagate from apply_patch() rather than catching and continuing with remaining patches. The project prefers explicit failures for patch operations.
🧠 Learnings used
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 944
File: dfetch/commands/update_patch.py:124-129
Timestamp: 2026-01-30T22:08:36.677Z
Learning: In dfetch's update-patch command (dfetch/commands/update_patch.py), when generating a reverse diff for a subproject patch, the old_revision parameter should be superproject.current_revision() (not subproject.current_revision()) because the patch is computed against the superproject's VCS commit to capture local changes within the superproject's version control context.
Learnt from: spoorcc
Repo: dfetch-org/dfetch PR: 942
File: dfetch/project/subproject.py:143-166
Timestamp: 2026-01-09T23:16:35.480Z
Learning: In dfetch project's dfetch/project/subproject.py, patch application failures should fail immediately by letting RuntimeError propagate from apply_patch() rather than catching and continuing with remaining patches. The project prefers explicit failures for patch operations.
Learnt from: ben-edna
Repo: dfetch-org/dfetch PR: 898
File: script/package.py:201-205
Timestamp: 2026-01-01T11:05:43.761Z
Learning: In the dfetch project's script/package.py, prefer explicit failures ("crash and burn") over silently handling missing configuration values, ensuring that builds fail if required metadata (e.g., URLs in pyproject.toml) is not properly configured.
Contributor
Author
|
@coderabbitai resume @coderabbitai review |
Contributor
|
✅ Actions performedReviews resumed. Review triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dfetch/commands/freeze.py (1)
97-145: 🛠️ Refactor suggestion | 🟠 MajorCyclomatic complexity exceeds the threshold of 8.
The
__call__method now has approximately 10 branches (base + with + if make_backup + for + try/except + if/elif/else for new_version + if manifest_updated + if had_errors). As per coding guidelines, cyclomatic complexity must stay below 8 per function.Consider extracting the version-detection and logging logic (lines 118-135) into a helper method:
♻️ Proposed refactor to reduce complexity
+ def _log_freeze_result( + self, project: dfetch.manifest.project.ProjectEntry, + on_disk_version: str | None, + new_version: str | None + ) -> None: + """Log the result of freezing a project.""" + if new_version is None: + if on_disk_version: + logger.print_info_line( + project.name, + f"Already pinned in manifest on version {on_disk_version}", + ) + else: + logger.print_warning_line( + project.name, + "No version on disk, first update with 'dfetch update'", + ) + else: + logger.print_info_line( + project.name, + f"Frozen on version {new_version}", + ) + def __call__(self, args: argparse.Namespace) -> None: """Perform the freeze.""" # ... existing setup code ... for project in projects_to_freeze: try: sub_project = dfetch.project.create_sub_project(project) on_disk_version = sub_project.on_disk_version() new_version = sub_project.freeze_project(project) - if new_version is None: - if on_disk_version: - logger.print_info_line( - project.name, - f"Already pinned in manifest on version {on_disk_version}", - ) - else: - logger.print_warning_line( - project.name, - "No version on disk, first update with 'dfetch update'", - ) - else: - logger.print_info_line( - project.name, - f"Frozen on version {new_version}", - ) + self._log_freeze_result(project, on_disk_version, new_version) + if new_version: superproject.manifest.update_project_version(project) manifest_updated = True except RuntimeError as exc: # ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/freeze.py` around lines 97 - 145, The __call__ method in freeze.py is over-complex; extract the per-project version detection and logging (the logic that calls dfetch.project.create_sub_project, sub_project.on_disk_version(), sub_project.freeze_project(project), and the subsequent logger.print_* calls and superproject.manifest.update_project_version(project) behavior) into a new helper function (e.g., _process_and_freeze_project(superproject, project) or similar). Move the try/except into that helper so the loop in __call__ becomes a simple iterate-and-call, have the helper return (manifest_was_updated: bool, had_error: bool) or raise as appropriate, and update manifest_updated and had_errors in __call__ accordingly; keep create_super_project(), NoVcsSuperProject backup handling, in_directory context, and final manifest.dump()/logger.info and had_errors check in __call__ unchanged.
🤖 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/settings.json:
- Around line 21-22: The current settings.json uses broad command patterns
"Bash(radon *)" and "Bash(ruff check:*)" which run across the whole workspace;
restrict these patterns to the specific project paths to avoid scanning
unrelated repos. Update the two entries to include the repository-specific
path(s) or glob(s) (e.g., "Bash(radon <project_path> *)" and "Bash(ruff
check:<project_path> *)" or similar per your project layout) so the radon and
ruff commands only target the intended codebase; keep the exact string forms
"Bash(radon *)" and "Bash(ruff check:*)" as the identifiers to locate and
replace them in .claude/settings.json.
- Line 15: The entry "Bash(pre-commit run:*)" is too permissive — replace the
wildcard pattern with explicit allowed commands/hook names so only known
pre-commit hooks can execute (e.g., enumerate exact hook names instead of using
:*); do the same for the other permissive entries that use the :* wildcard (the
similar Bash(...) entries on the file), and ensure the allowed set matches
concrete script names in your pre-commit config to tighten execution scope and
improve auditability.
---
Outside diff comments:
In `@dfetch/commands/freeze.py`:
- Around line 97-145: The __call__ method in freeze.py is over-complex; extract
the per-project version detection and logging (the logic that calls
dfetch.project.create_sub_project, sub_project.on_disk_version(),
sub_project.freeze_project(project), and the subsequent logger.print_* calls and
superproject.manifest.update_project_version(project) behavior) into a new
helper function (e.g., _process_and_freeze_project(superproject, project) or
similar). Move the try/except into that helper so the loop in __call__ becomes a
simple iterate-and-call, have the helper return (manifest_was_updated: bool,
had_error: bool) or raise as appropriate, and update manifest_updated and
had_errors in __call__ accordingly; keep create_super_project(),
NoVcsSuperProject backup handling, in_directory context, and final
manifest.dump()/logger.info and had_errors check in __call__ unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a242edc4-ab2e-4a33-ab5a-e49766aec8fa
📒 Files selected for processing (10)
.claude/settings.jsondfetch/commands/check.pydfetch/commands/freeze.pyfeatures/check-git-repo.featurefeatures/check-svn-repo.featurefeatures/fetch-archive.featurefeatures/fetch-checks-destination.featurefeatures/patch-after-fetch-git.featurefeatures/patch-after-fetch-svn.featurefeatures/updated-project-has-dependencies.feature
💤 Files with no reviewable changes (1)
- features/fetch-checks-destination.feature
ben-edna
force-pushed
the
claude/fix-runtime-error-printing-Vayam
branch
from
dce1821 to
a6d4836
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/settings.json:
- Line 8: The entry "Bash(git fetch:*)" grants unrestricted git fetch args;
narrow it to the exact fetch invocations the workflow requires by replacing
"Bash(git fetch:*)" with explicit, least-privilege fetch forms (e.g., fetch from
the specific remote names and exact refspecs or flags your workflow uses) so
only those exact git fetch commands are allowed; update the settings.json entry
accordingly and remove the wildcard form.
In `@dfetch/util/util.py`:
- Around line 360-371: The function glob_within_root misclassifies matches
because it compares Path(p).resolve() to the caller-provided root without
resolving root first; change the implementation in glob_within_root to resolve
the caller-provided root once (e.g., root_resolved = root.resolve()) before the
loop and then use Path(p).resolve().is_relative_to(root_resolved) to classify
into safe and escaped (keep using the same safe/escaped lists and the existing
sorted(glob.glob(pattern)) iteration).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1226d8e-4d56-47e0-b0c9-c604806cb34f
📒 Files selected for processing (16)
.claude/settings.jsonCHANGELOG.rstdfetch/commands/check.pydfetch/commands/diff.pydfetch/commands/format_patch.pydfetch/commands/freeze.pydfetch/commands/update.pydfetch/commands/update_patch.pydfetch/util/util.pyfeatures/check-git-repo.featurefeatures/check-svn-repo.featurefeatures/fetch-archive.featurefeatures/fetch-checks-destination.featurefeatures/patch-after-fetch-git.featurefeatures/patch-after-fetch-svn.featurefeatures/updated-project-has-dependencies.feature
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/settings.json:
- Line 8: The entry "Bash(git fetch:*)" grants unrestricted git fetch args;
narrow it to the exact fetch invocations the workflow requires by replacing
"Bash(git fetch:*)" with explicit, least-privilege fetch forms (e.g., fetch from
the specific remote names and exact refspecs or flags your workflow uses) so
only those exact git fetch commands are allowed; update the settings.json entry
accordingly and remove the wildcard form.
In `@dfetch/util/util.py`:
- Around line 360-371: The function glob_within_root misclassifies matches
because it compares Path(p).resolve() to the caller-provided root without
resolving root first; change the implementation in glob_within_root to resolve
the caller-provided root once (e.g., root_resolved = root.resolve()) before the
loop and then use Path(p).resolve().is_relative_to(root_resolved) to classify
into safe and escaped (keep using the same safe/escaped lists and the existing
sorted(glob.glob(pattern)) iteration).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1226d8e-4d56-47e0-b0c9-c604806cb34f
📒 Files selected for processing (16)
.claude/settings.jsonCHANGELOG.rstdfetch/commands/check.pydfetch/commands/diff.pydfetch/commands/format_patch.pydfetch/commands/freeze.pydfetch/commands/update.pydfetch/commands/update_patch.pydfetch/util/util.pyfeatures/check-git-repo.featurefeatures/check-svn-repo.featurefeatures/fetch-archive.featurefeatures/fetch-checks-destination.featurefeatures/patch-after-fetch-git.featurefeatures/patch-after-fetch-svn.featurefeatures/updated-project-has-dependencies.feature
🛑 Comments failed to post (2)
.claude/settings.json (1)
8-8:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope
git fetchmore tightly.Line 8 allows arbitrary
git fetcharguments, which gives the agent unrestricted remote selection, refspec updates, and network egress in a security-sensitive allowlist. Replace the wildcard with the exact fetch forms the workflow needs so the permission stays auditable and least-privilege.🔒 Suggested hardening
- "Bash(git fetch:*)", + "Bash(git fetch origin)", + "Bash(git fetch origin --tags)", + "Bash(git fetch --all --prune)"📝 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."Bash(git fetch origin)", "Bash(git fetch origin --tags)", "Bash(git fetch --all --prune)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/settings.json at line 8, The entry "Bash(git fetch:*)" grants unrestricted git fetch args; narrow it to the exact fetch invocations the workflow requires by replacing "Bash(git fetch:*)" with explicit, least-privilege fetch forms (e.g., fetch from the specific remote names and exact refspecs or flags your workflow uses) so only those exact git fetch commands are allowed; update the settings.json entry accordingly and remove the wildcard form.dfetch/util/util.py (1)
360-371:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve
rootbefore classifying glob matches.Line 370 compares
Path(p).resolve()against the caller-providedrootas-is. Ifrootis relative or still contains symlinks /.., safe matches can be misclassified as escaped.Suggested fix
def glob_within_root(pattern: str, root: Path) -> tuple[list[str], list[str]]: """Expand *pattern* and split results by whether they resolve within *root*. @@ """ safe: list[str] = [] escaped: list[str] = [] + resolved_root = root.resolve() for p in sorted(glob.glob(pattern)): - (safe if Path(p).resolve().is_relative_to(root) else escaped).append(p) + resolved_path = Path(p).resolve() + (safe if resolved_path.is_relative_to(resolved_root) else escaped).append(p) return safe, escaped📝 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.def glob_within_root(pattern: str, root: Path) -> tuple[list[str], list[str]]: """Expand *pattern* and split results by whether they resolve within *root*. Returns: A ``(safe, escaped)`` tuple where *safe* contains sorted paths that resolve inside *root* and *escaped* contains those that do not. """ safe: list[str] = [] escaped: list[str] = [] resolved_root = root.resolve() for p in sorted(glob.glob(pattern)): resolved_path = Path(p).resolve() (safe if resolved_path.is_relative_to(resolved_root) else escaped).append(p) return safe, escaped🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dfetch/util/util.py` around lines 360 - 371, The function glob_within_root misclassifies matches because it compares Path(p).resolve() to the caller-provided root without resolving root first; change the implementation in glob_within_root to resolve the caller-provided root once (e.g., root_resolved = root.resolve()) before the loop and then use Path(p).resolve().is_relative_to(root_resolved) to classify into safe and escaped (keep using the same safe/escaped lists and the existing sorted(glob.glob(pattern)) iteration).
spoorcc
force-pushed
the
claude/fix-runtime-error-printing-Vayam
branch
from
e806cdd to
70412c7
Compare
spoorcc
force-pushed
the
claude/fix-runtime-error-printing-Vayam
branch
from
70412c7 to
f676382
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.claude/settings.json (1)
15-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-scoping broad
Bash(...)wildcard permissions is needed.Line 15, Lines 20–24, Lines 27–28, and Lines 32–35 reintroduce very broad execution permissions (e.g.,
*-based patterns) for commands that can execute project code or mutate the environment. This weakens least-privilege and auditability in the same way previously flagged patterns did.Suggested hardening examples
- "Bash(python -m behave *)", + "Bash(python -m behave features:*)", - "Bash(python -m pytest tests/*)", + "Bash(pytest tests/*)", - "Bash(radon *)", + "Bash(radon dfetch:*)", - "Bash(ruff check:*)", + "Bash(ruff check dfetch:*)", - "Bash(xenon *)", + "Bash(xenon dfetch:*)", - "Bash(pip install *)", + "Bash(pip install -e .)"Also applies to: 20-24, 27-28, 32-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/settings.json at line 15, The file .claude/settings.json contains overly broad wildcard patterns in Bash permission rules that weaken least-privilege security. At line 15, replace the broad pattern "Bash(pre-commit run:*)" with a specific list of allowed pre-commit hooks instead of using the wildcard. At lines 20-24, 27-28, and 32-35, apply the same principle by removing wildcard patterns and replacing them with explicit, narrow permission scopes that specify exactly which commands or command variations are permitted rather than allowing any command matching a broad pattern. Ensure each Bash entry specifies only the minimum necessary execution permissions needed for intended functionality.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_fuzzing.py`:
- Around line 56-65: The SAFE_VERSION strategy in test_fuzzing.py no longer
includes string generation, which means the Str() branch of the MANIFEST_SCHEMA
version validator is not being tested during fuzzing. Add string generation to
the SAFE_VERSION st.one_of() call to ensure all three branches (Int(), Float(),
and Str()) are covered by the fuzzed test paths (test_manifest_can_be_created,
test_check, test_update), while maintaining the safety constraints already
documented in the comments about avoiding non-float strings that would cause
strictyaml to raise ValueError.
---
Duplicate comments:
In @.claude/settings.json:
- Line 15: The file .claude/settings.json contains overly broad wildcard
patterns in Bash permission rules that weaken least-privilege security. At line
15, replace the broad pattern "Bash(pre-commit run:*)" with a specific list of
allowed pre-commit hooks instead of using the wildcard. At lines 20-24, 27-28,
and 32-35, apply the same principle by removing wildcard patterns and replacing
them with explicit, narrow permission scopes that specify exactly which commands
or command variations are permitted rather than allowing any command matching a
broad pattern. Ensure each Bash entry specifies only the minimum necessary
execution permissions needed for intended functionality.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48ef632a-b1a3-46b5-a033-fe131a4af4d7
📒 Files selected for processing (21)
.claude/settings.jsonCHANGELOG.rstdfetch/commands/check.pydfetch/commands/diff.pydfetch/commands/environment.pydfetch/commands/format_patch.pydfetch/commands/freeze.pydfetch/commands/update.pydfetch/commands/update_patch.pydfetch/log.pydfetch/project/svnsubproject.pydfetch/util/util.pydfetch/vcs/svn.pyfeatures/check-git-repo.featurefeatures/check-svn-repo.featurefeatures/fetch-archive.featurefeatures/fetch-checks-destination.featurefeatures/patch-after-fetch-git.featurefeatures/patch-after-fetch-svn.featurefeatures/updated-project-has-dependencies.featuretests/test_fuzzing.py
💤 Files with no reviewable changes (1)
- features/fetch-checks-destination.feature
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
doc/_ext/scenario_directive.py (1)
497-514:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNode reuse causes only the last placeholder to receive content when multiple exist.
If multiple
.. scenario-appendix::directives are present, the samenotenode (HTML) orappendix_nodeslist (PDF) is passed toreplace_selffor each placeholder. In docutils, inserting a node into a new parent removes it from the old one, so only the last placeholder retains content.While documentation says to use the directive once, the code should handle duplicates gracefully.
Proposed fix: create fresh note per placeholder in HTML case
if app.builder.name not in ("latex", "rinoh"): + for placeholder in placeholders: + note = nodes.note() + para = nodes.paragraph() + para += nodes.Text( + "In the HTML edition, feature examples appear as expandable " + "blocks directly within each guide section. " + "In the PDF edition they are collected here, grouped by command." + ) + note += para + placeholder.replace_self([note]) + return - note = nodes.note() - para = nodes.paragraph() - para += nodes.Text( - "In the HTML edition, feature examples appear as expandable " - "blocks directly within each guide section. " - "In the PDF edition they are collected here, grouped by command." - ) - note += para - for placeholder in placeholders: - placeholder.replace_self([note]) - return entries = getattr(app.env, "scenario_appendix_entries", {}) appendix_nodes = _build_appendix_nodes(entries) if entries else [] - for placeholder in placeholders: - placeholder.replace_self(appendix_nodes) + for i, placeholder in enumerate(placeholders): + if i == 0: + placeholder.replace_self(appendix_nodes) + else: + # Remove duplicate appendix placeholders; content appears once + placeholder.replace_self([])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doc/_ext/scenario_directive.py` around lines 497 - 514, The issue is that the same node object (note in HTML case, appendix_nodes in PDF case) is being reused across multiple placeholders in the loop. In docutils, nodes can only have one parent, so when the same node is inserted into multiple parents, it gets removed from previous parents. To fix this, for the HTML case (when app.builder.name is not "latex" or "rinoh"), move the note node creation inside the for loop iterating over placeholders so each placeholder receives its own fresh note object. Similarly, for the PDF case, either move the _build_appendix_nodes call inside the loop for each placeholder, or create independent copies of the appendix_nodes for each placeholder instead of reusing the same list across all iterations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@doc/_ext/scenario_directive.py`:
- Around line 497-514: The issue is that the same node object (note in HTML
case, appendix_nodes in PDF case) is being reused across multiple placeholders
in the loop. In docutils, nodes can only have one parent, so when the same node
is inserted into multiple parents, it gets removed from previous parents. To fix
this, for the HTML case (when app.builder.name is not "latex" or "rinoh"), move
the note node creation inside the for loop iterating over placeholders so each
placeholder receives its own fresh note object. Similarly, for the PDF case,
either move the _build_appendix_nodes call inside the loop for each placeholder,
or create independent copies of the appendix_nodes for each placeholder instead
of reusing the same list across all iterations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7097eaa8-1626-432f-bcb0-35a7da59be4c
📒 Files selected for processing (3)
doc/_ext/colordot.pydoc/_ext/scenario_directive.pytests/test_fuzzing.py
Replace the pattern of collecting RuntimeErrors across all subprojects and printing them at the end with immediate in-context logging via `logger.print_warning_line(project.name, str(exc))` at the point of failure. This ensures errors like "svn not available on system" appear next to the subproject they belong to instead of at the end of the run. The final `raise RuntimeError()` (no message) still produces a non-zero exit code without double-printing. Also bumps version to 0.14.0 and updates CHANGELOG, feature tests. https://claude.ai/code/session_01RTgPqrX37jFkK843dq65Bu Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract per-project logic from Diff.__call__ (was CC=11) into _diff_project and from UpdatePatch.__call__ (was CC=10) into _process_project. All methods now have CC <= 8. https://claude.ai/code/session_01RTgPqrX37jFkK843dq65Bu Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the pattern of collecting RuntimeErrors across all subprojects and printing them at the end with immediate in-context logging via `logger.print_warning_line(project.name, str(exc))` at the point of failure. This ensures errors like "svn not available on system" appear next to the subproject they belong to instead of at the end of the run. The final `raise RuntimeError()` (no message) still produces a non-zero exit code without double-printing. Also bumps version to 0.14.0 and updates CHANGELOG, feature tests. https://claude.ai/code/session_01RTgPqrX37jFkK843dq65Bu Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolve root once before the loop in glob_within_root so symlinked or relative root paths compare correctly against resolved match paths. Narrow the Claude Code permission from the wildcard "git fetch:*" to "git fetch origin:*", restricting fetches to the origin remote only. https://claude.ai/code/session_01RTgPqrX37jFkK843dq65Bu
Docutils nodes can only have one parent. Creating a single note (HTML) or a single appendix-nodes list (PDF) and inserting it into multiple placeholders caused earlier placeholders to silently lose their node once it was re-parented. Move node creation inside the placeholder loop so each replacement receives its own fresh node tree. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
spoorcc
force-pushed
the
claude/fix-runtime-error-printing-Vayam
branch
from
dc0e595 to
c80559e
Compare
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
3 participants
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