⚡ Bolt: optimize hot-loop path parsing to reduce CPU overhead - #904
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough문자열 경로 처리가 Changes경로 처리 최적화
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.jules/bolt.md (1)
76-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win경로 처리 지침의 상충을 정리하십시오.
기존 지침은
replace("\\", "/")를 권고합니다. 새 지침은 같은 핫 루프에서replace()를 피하라고 권고합니다. 이전 항목을 superseded로 표시하거나 최신 지침으로 통합하십시오.이 평가는
.jules/bolt.md의 기존 항목과 새 항목을 비교한 결과입니다.🤖 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 @.jules/bolt.md around lines 76 - 79, Resolve the conflicting path-processing guidance in the relevant entries of bolt.md: mark the older recommendation to use replace("\\", "/") as superseded or revise it to align with the newer hot-loop guidance. Ensure the consolidated guidance consistently recommends rfind(), max(), and split()-based string handling in performance-critical paths.appguardrail_core/language.py (1)
233-233: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift핫 루프의
replace()및split()할당을 제거하십시오.Line 233은 각 경로마다 정규화 문자열과 구성 요소 목록을 새로 할당합니다. 이는 PR의 중간 문자열 및 garbage-collection 오버헤드 감소 목표와 맞지 않습니다.
구분자를 직접 처리하는 단일 순회 helper를 사용하십시오. 또는 해당 할당이 허용된다는 근거를 벤치마크로 확인하고 문서화하십시오. 이 평가는 PR 최적화 목표와
.jules/bolt.md의 최신 조치 항목에 근거합니다.🤖 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 `@appguardrail_core/language.py` at line 233, 핫 루프에서 path.replace와 split을 호출하는 대신, 백슬래시와 슬래시 구분자를 직접 순회하며 구성 요소를 처리하는 단일 순회 helper를 도입하고 해당 경로 처리 로직에서 재사용하십시오. appguardrail_core/language.py의 주변 함수 흐름과 반환 동작은 유지하되 중간 정규화 문자열 및 불필요한 구성 요소 목록 할당을 제거하십시오.
🤖 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 `@appguardrail_core/language.py`:
- Around line 207-215: Update the framework-marker detection around lowered_path
to normalize both “/” and “\” separators consistently, then compare path
components rather than substring matches. Detect exact templates or views
components, including root-level and mixed-separator paths, while excluding
names such as mytemplates.
- Around line 97-102: 두 경로 helper에서 문자열 판별 조건을 type(file_path) is not str 형태가 아닌
isinstance(file_path, str) 기반으로 변경하십시오. appguardrail_core/language.py 97-102의
anchor와 scanner/cli/appguardrail.py 1177의 sibling 모두 수정해 str 하위 클래스가 문자열 경로 처리
분기로 들어가도록 하고, 두 helper에 str 하위 클래스 입력을 검증하는 회귀 테스트를 추가하십시오.
---
Nitpick comments:
In @.jules/bolt.md:
- Around line 76-79: Resolve the conflicting path-processing guidance in the
relevant entries of bolt.md: mark the older recommendation to use replace("\\",
"/") as superseded or revise it to align with the newer hot-loop guidance.
Ensure the consolidated guidance consistently recommends rfind(), max(), and
split()-based string handling in performance-critical paths.
In `@appguardrail_core/language.py`:
- Line 233: 핫 루프에서 path.replace와 split을 호출하는 대신, 백슬래시와 슬래시 구분자를 직접 순회하며 구성 요소를
처리하는 단일 순회 helper를 도입하고 해당 경로 처리 로직에서 재사용하십시오. appguardrail_core/language.py의 주변
함수 흐름과 반환 동작은 유지하되 중간 정규화 문자열 및 불필요한 구성 요소 목록 할당을 제거하십시오.
🪄 Autofix
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 Plus
Run ID: a02d3f24-13ba-499e-86eb-04fa79012c96
📒 Files selected for processing (3)
.jules/bolt.mdappguardrail_core/language.pyscanner/cli/appguardrail.py
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head85d1d01f157f133eb815ffd0b80d7369455decb2. -
Head SHA:
85d1d01f157f133eb815ffd0b80d7369455decb2 -
Workflow run: 31246587989
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (3 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: test_language_path_optimization_contract.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_language_path_optimization_contract.py"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: test_language_path_optimization_contract.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_language_path_optimization_contract.py"]
R2 --> V2["targeted test run"]
|
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 `@tests/test_language_path_optimization_contract.py`:
- Around line 46-48: _display_path의 문자열 경로 분기가 replace 호출이나 문자열 재할당에 의존하지 않도록
검증하는 AST 또는 소스 기반 계약 테스트를 추가하세요. 기존
test_string_subclass_uses_string_display_path_branch의 동작 검증은 유지하고, 해당 함수 구현에서
replace 호출과 문자열 재할당이 없음을 명시적으로 검사하세요.
- Around line 69-76: Extend
test_signal_detection_avoids_replace_split_hot_loop_allocations to also inspect
_iter_lower_path_components, checking its source or AST for .replace( and
.split( usage. Keep the existing _detect_signals assertions and ensure
regressions in either path-processing function fail the contract test.
🪄 Autofix
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 Plus
Run ID: c6d565ba-4c49-4634-989a-fef72a9626a8
📒 Files selected for processing (2)
appguardrail_core/language.pytests/test_language_path_optimization_contract.py
🚧 Files skipped from review as they are similar to previous changes (1)
- appguardrail_core/language.py
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current heada46c125c8273eaeebd8f6bf9bb2398204b60eb3c. -
Head SHA:
a46c125c8273eaeebd8f6bf9bb2398204b60eb3c -
Workflow run: 31294228487
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: test_language_path_optimization_contract.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_language_path_optimization_contract.py"]
R2 --> V2["targeted test run"]
|
@coderabbitai review @opencode-agent Review-only request for exact current head All seven exact-head repository workflows are now successful and the former Tests failure was root-caused: an implementation-ban test incorrectly prohibited CPython's bounded |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='0d07baae44a40edfcaec5e42c7fb9351510ca9f0'
head='08c04317370b03f3c5c8e49ed7de4578cc074cf3'
printf '%s\n' '== Commit identity and ancestry =='
git rev-parse HEAD
git show -s --format='HEAD %H%n%P%n%s' HEAD
git cat-file -e "${base}^{commit}"
git cat-file -e "${head}^{commit}"
git merge-base --is-ancestor "$base" "$head"
git show -s --format='BASE %H%n%s' "$base"
git show -s --format='TARGET %H%n%s' "$head"
printf '%s\n' '== Changed files and complete patch =='
git diff --check "$base" "$head"
git diff --stat "$base" "$head"
git diff --find-renames --find-copies --no-ext-diff "$base" "$head" -- \
.jules/bolt.md appguardrail_core/language.py appguardrail.py tests/test_language_path_optimization_contract.py
printf '%s\n' '== Source outlines =='
ast-grep outline appguardrail_core/language.py --items all
ast-grep outline appguardrail.py --items all
ast-grep outline tests/test_language_path_optimization_contract.py --items all
printf '%s\n' '== Relevant implementation and contract-test text =='
rg -n -C 8 --glob 'appguardrail_core/language.py' \
'def |class |_display_path|Path|replace\(|rfind\(|split\(|templates|views|signal|accessibility' .
rg -n -C 8 --glob 'appguardrail.py' \
'def _display_path|Path|replace\(|rfind\(' .
cat -n tests/test_language_path_optimization_contract.py
printf '%s\n' '== Relevant call sites =='
rg -n -C 4 --glob '*.py' \
'\b(detect_language|detect_framework|detect_signals|is_web_accessibility|_display_path)\s*\(' \
appguardrail_core appguardrail.py testsLength of output: 103115 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='0d07baae44a40edfcaec5e42c7fb9351510ca9f0'
head='08c04317370b03f3c5c8e49ed7de4578cc074cf3'
repo='ContextualWisdomLab/appguardrail'
printf '%s\n' '== Exact checkout and complete unfiltered diff =='
test "$(git rev-parse HEAD)" = "$head"
git status --short
git diff --check "$base" "$head"
git diff --name-status "$base" "$head"
git diff --find-renames --find-copies --no-ext-diff "$base" "$head"
printf '%s\n' '== Changed implementation: base and head =='
for revision in "$base" "$head"; do
printf '\n-- %s: appguardrail_core/language.py Lines 90-290 --\n' "$revision"
git show "${revision}:appguardrail_core/language.py" | sed -n '90,290p'
done
printf '%s\n' '== CLI display-path contract and callers =='
sed -n '1168,1182p' scanner/cli/appguardrail.py
rg -n -C 3 --type py '\b_display_path\s*\(' scanner tests appguardrail_core
printf '%s\n' '== Parser call sites and relevant test coverage =='
rg -n -C 3 --type py \
'\b(detect_language_axes|detect_stack_profile|_detect_framework_markers|_detect_signals|_is_web_reachable|_iter_lower_path_components)\s*\(' \
appguardrail_core scanner tests
rg -n -C 2 --type py \
'(windows|backslash|templates|views|WEB_SIGNAL_DIRS|detect_stack_profile|detect_language_axes)' \
tests
printf '%s\n' '== Exact-head GitHub check runs =='
gh api "repos/${repo}/commits/${head}/check-runs?per_page=100" \
--jq '.check_runs[] | [.name, .status, (.conclusion // "null"), .details_url] | `@tsv`'Length of output: 55509
I reviewed The complete diff changes three files only. The path parser preserves support for
This is a review-only verdict. I did not approve, merge, mutate, tag, publish, release, or change gates. You are interacting with an AI system. |
Acknowledged. |
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head08c04317370b03f3c5c8e49ed7de4578cc074cf3. -
Head SHA:
08c04317370b03f3c5c8e49ed7de4578cc074cf3 -
Workflow run: 31323847526
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: test_language_path_optimization_contract.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_language_path_optimization_contract.py"]
R2 --> V2["targeted test run"]
Carry the reviewed language-profile implementation and focused regression suite onto current protected develop while preserving the newer cumulative Bolt learning log from develop.
Dismissed as stale predecessor-head review anchored to 85d1d01. Current head 64f2d12 has been reconciled with protected develop and its exact-head repository checks are successful except the still-running Security Scan. Dismissal is not approval; a fresh current-head independent review is being requested.
|
@cwl-noema-review |
1 participant
Scope
Reduce redundant full-path normalization where language and framework detection only need a filename or separator-delimited components, while preserving the public
str | Pathcontract and both slash conventions.Implementation
rfind()rather than allocating normalized intermediate paths;strsubclasses as string inputs;_display_pathon CPython's C-levelstr.replace()because its public contract requires every Windows separator to change;RCA for the prior failing Tests gate
Both Python matrices failed
test_display_path_avoids_replace_and_local_reassignment. The immediate cause was that the test bannedreplacewhile production correctly used it for complete display normalization. The root cause was conflating component parsing, which can avoid whole-string normalization, with output normalization, which necessarily creates a changed string. A directtranslatesubstitution was rejected after a local comparative probe showed materially higher cost for the representative Windows path.Current-head reconciliation
The branch now merges protected
develop@77e3e0c5867b1143970fcdce80962bda8a8fc80fat head64f2d1223d374d12ecc4d3e558541c2a348d46d0and is no longer behind or conflicted.Only the reviewed production file and focused regression suite are overlaid on current develop. The branch-local
.jules/bolt.mdcopy was intentionally not overlaid because protected develop already contains later cumulative performance lessons; this avoids replacing newer shared documentation with an older branch snapshot.Verification evidence
Pre-reconciliation verification for the reviewed implementation:
git diff --check: clean;Fresh hosted checks on exact head
64f2d1223d374d12ecc4d3e558541c2a348d46d0remain authoritative. No predecessor result is treated as current-head proof, and no unverified wall-clock speedup or allocation-free Windows normalization is claimed.