Skip to content

Fix AI Review Findings in PR #229 - #231

Merged
rakaarwaky merged 5 commits into
mainfrom
fix/reviewer-fixes-20260812
Aug 12, 2026
Merged

Fix AI Review Findings in PR #229#231
rakaarwaky merged 5 commits into
mainfrom
fix/reviewer-fixes-20260812

Conversation

@rakaarwaky

@rakaarwaky rakaarwaky commented Aug 11, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes the AI reviewer findings from PR #229 that were verified as valid (CodeRabbit + cubic).

Changes

  1. utility_signature_parser.rsbrace_pair now guards against reversed ranges (close < open) so the &line[open + 1..close] slice can never panic on arbitrary source text.
  2. utility_signature_parser.rscollect_python_return_primitives now checks token boundaries: -> IntervalVO no longer reports int, -> StrategyVO no longer reports str, etc.
  3. taxonomy_filesystem_vo.rsboundary_suffix is now deterministic: picks the longest (most specific) matching key instead of whichever HashMap iteration finds first.
  4. .repowiseIgnore — narrowed barrel exclusions: **/lib.rs re-includes external-lint/src/lib.rs (has convert_executor_error), dropped **/build.rs (shared/build.rs is a real build script). Only verified-pure barrels stay excluded from health scoring.

Verification

  • cargo clippy -p shared-lint-arwaky --all-targets -D warnings
  • cargo nextest run -p shared-lint-arwaky — 302/302 pass ✅
  • lint-arwaky-cli scan . — 0 violations ✅

Reviewer findings assessed as NOT valid (not changed)

  • boundary_ends_with off-by-one claim — analysis shows the logic is correct (before is the index of the character preceding the suffix because of ends_with).
  • LineNumber/Score Eq issues — those types no longer exist (removed in prior refactor).

Summary by cubic

Fixes verified reviewer findings from PR #229 to prevent panics, reduce Python parsing false positives, and make path matching deterministic with normalized keys. Also updates .repowiseIgnore so files with real logic are scored.

  • Bug Fixes
    • brace_pair guards reversed {} so slicing can’t panic.
    • Python parsing: params use whole-token matching (: int fires; : internal_id doesn’t); returns use token boundaries (-> IntervalVO doesn’t match int); list [T]/dict [K, V] (with or without a space) are treated as parameterized in both params and returns.
    • Path matching: boundary_suffix normalizes paths (strips leading .///), picks the longest normalized key and tie-breaks lexicographically, so /src/a.rs and src/a.rs rank the same; boundary_ends_with normalizes both sides and checks the byte before the suffix to fix the off-by-one.
    • .repowiseIgnore narrows exclusions: re-includes crates/external-lint/src/lib.rs; drops **/build.rs so real build scripts are scored; only verified-pure barrels stay excluded.

Written for commit 20d41bd. Summary will update on new commits.

Review in cubic

Addresses issues raised by CodeRabbit and cubic that were verified valid:

1. utility_signature_parser.rs: guard `brace_pair` against reversed ranges
   (`close < open`) so the `&line[open + 1..close]` slice can never panic.
2. utility_signature_parser.rs: token-boundary check in
   `collect_python_return_primitives` so `-> IntervalVO` no longer matches
   `int`, `-> StrategyVO` no longer matches `str`, etc.
3. taxonomy_filesystem_vo.rs: make `boundary_suffix` deterministic by
   picking the longest (most specific) matching key instead of relying on
   arbitrary HashMap iteration order.
4. .repowiseIgnore: narrow `**/lib.rs` / drop `**/build.rs` so files with
   real logic (`external-lint/src/lib.rs`, `shared/build.rs`) stay scored;
   only verified-pure barrels remain excluded.

Verification: clippy clean, 302 shared tests pass, self-lint 0 violations.

Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
@kapybro

kapybro Bot commented Aug 11, 2026

Copy link
Copy Markdown

AI Summary

This issue fixes valid AI reviewer findings from PR #229, focusing on improving the signature parser's handling of reversed ranges and token boundaries. It also refines barrel exclusions for health scoring.

@kapybro kapybro Bot changed the title fix(shared): address valid AI reviewer findings from PR #229 Fix AI Review Findings in PR #229 Aug 11, 2026
@repowise-bot

repowise-bot Bot commented Aug 11, 2026

Copy link
Copy Markdown

⚠️ Health of changed files: 6.6 → 5.7 (-0.8)
⚠️ Change risk: moderate, riskier than 61% of this repo's commits.

📋 At a glance
2 files changed health · 2 hotspots touched · 2 below the 7.0 health floor · 2 new findings introduced · 1 co-change pair left out · 2 files with recent fix history · 10 dead-code findings · no tests touched. Scoped to crates.

⚠️ Health gate (advisory)
would block if enforced: crates/shared/src/filesystem/taxonomy_filesystem_vo.rs is at 4.9, below the 7.0 floor (+1 more)

📌 Before you merge

  • No test file imports the changed files and none were touched: consider adding coverage
  • .../src/agent_filesystem_orchestrator.rs changed together with .../filesystem/taxonomy_filesystem_vo.rs in 10 past commits and isn't in this PR
🔎 More signals (4)

🗺️ Change map

flowchart LR
  subgraph PR ["Changed in this PR (2 with dependents)"]
    f_crates_shared_src_common_utility_signature_parser_rs[".../common/utility_signature_parser.rs 🔥"]:::changed
    f_crates_shared_src_filesystem_taxonomy_filesystem_vo_rs[".../filesystem/taxonomy_filesystem_vo.rs 🔥"]:::changed
  end
  f_crates_import_rules_src_capabilities_cycle_import_analyzer_rs[".../src/capabilities_cycle_import_analyzer.rs"]
  f_crates_shared_src_common_utility_signature_parser_rs --> f_crates_import_rules_src_capabilities_cycle_import_analyzer_rs
  f_crates_mcp_server_benches_bench_mcp_server_rs[".../benches/bench_mcp_server.rs"]
  f_crates_shared_src_common_utility_signature_parser_rs --> f_crates_mcp_server_benches_bench_mcp_server_rs
  f_crates_naming_rules_src_capabilities_naming_convention_checker_rs[".../src/capabilities_naming_convention_checker.rs"]
  f_crates_shared_src_common_utility_signature_parser_rs --> f_crates_naming_rules_src_capabilities_naming_convention_checker_rs
  f_crates_role_rules_src_agent_role_orchestrator_rs[".../src/agent_role_orchestrator.rs"]
  f_crates_shared_src_common_utility_signature_parser_rs --> f_crates_role_rules_src_agent_role_orchestrator_rs
  f_crates_filesystem_src_agent_filesystem_orchestrator_rs[".../src/agent_filesystem_orchestrator.rs"]
  f_crates_shared_src_filesystem_taxonomy_filesystem_vo_rs --> f_crates_filesystem_src_agent_filesystem_orchestrator_rs
  f_crates_filesystem_src_capabilities_ast_parser_rs[".../src/capabilities_ast_parser.rs"]
  f_crates_shared_src_filesystem_taxonomy_filesystem_vo_rs --> f_crates_filesystem_src_capabilities_ast_parser_rs
  f_crates_filesystem_src_lib_rs[".../src/lib.rs"]
  f_crates_shared_src_filesystem_taxonomy_filesystem_vo_rs --> f_crates_filesystem_src_lib_rs
  f_crates_orphan_rules_src_capabilities_orphan_contract_analyzer_rs[".../src/capabilities_orphan_contract_analyzer.rs"]
  f_crates_shared_src_filesystem_taxonomy_filesystem_vo_rs --> f_crates_orphan_rules_src_capabilities_orphan_contract_analyzer_rs
  more(["+4 more dependents"])
  PR --> more
  w_crates_filesystem_src_agent_filesystem_orchestrator_rs(["⚠️ .../src/agent_filesystem_orchestrator.rs changed together 10×, not in PR"]):::warn
  f_crates_shared_src_filesystem_taxonomy_filesystem_vo_rs -.- w_crates_filesystem_src_agent_filesystem_orchestrator_rs
  classDef changed fill:#dbeafe,stroke:#1d4ed8,color:#1e3a5f
  classDef warn fill:#fef3c7,stroke:#b45309,color:#78350f
  classDef guard fill:#dcfce7,stroke:#15803d,color:#14532d
Loading

Solid arrows: code that imports the changed files (12 direct dependents, from the last indexed snapshot). Dashed: history/tests.

🔥 Hotspots touched (2)

  • .../common/utility_signature_parser.rs: 4 commits/90d, 5 dependents
  • .../filesystem/taxonomy_filesystem_vo.rs: 15 commits/90d, 7 dependents

🔗 Hidden coupling (1 file)

  • .../filesystem/taxonomy_filesystem_vo.rs co-changes with .../src/agent_filesystem_orchestrator.rs (10×, 🟡 notable), not in this PR.

💀 Dead code (10 findings)

  • 💀 .../filesystem/taxonomy_filesystem_vo.rs FileEntry (confidence 1.00)
  • 💀 .../filesystem/taxonomy_filesystem_vo.rs RustModItem (confidence 1.00)
  • 💀 .../filesystem/taxonomy_filesystem_vo.rs RustFnItem (confidence 1.00)
7 more
  • 💀 .../filesystem/taxonomy_filesystem_vo.rs PythonClassItem (confidence 1.00)
  • 💀 .../filesystem/taxonomy_filesystem_vo.rs PythonFnItem (confidence 1.00)
  • 💀 .../filesystem/taxonomy_filesystem_vo.rs TSClassItem (confidence 1.00)
  • 💀 .../filesystem/taxonomy_filesystem_vo.rs TSFnItem (confidence 1.00)
  • 💀 .../filesystem/taxonomy_filesystem_vo.rs ImportType (confidence 1.00)
  • 💀 .../filesystem/taxonomy_filesystem_vo.rs FileNodeVO (confidence 1.00)
  • 💀 .../filesystem/taxonomy_filesystem_vo.rs ImportEdgeVO (confidence 1.00)

📊 See the full report for this PR
Your repo map with this PR's blast radius lit up, every caller of the contracts it changes, and health before and after. No sign-in. · ⭐ Star Repowise · 📥 Install bot · Silence on a single PR with [skip repowise] in the title · Per-repo toggle on repowise.dev/settings?tab=bot · Updated 2026-08-12 04:23 UTC

@kapybro kapybro Bot added bug Something isn't working priority: high High priority issue shared shared crate labels Aug 11, 2026
@github-actions github-actions Bot added the rust label Aug 11, 2026
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 3e68ae85-a29f-47fc-9569-2fd3a57987a1

📥 Commits

Reviewing files that changed from the base of the PR and between 20ea8af and 20d41bd.

📒 Files selected for processing (2)
  • crates/shared/src/common/utility_signature_parser.rs
  • crates/shared/tests/unit_shared_common_vo.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/shared/tests/unit_shared_common_vo.rs
  • crates/shared/src/common/utility_signature_parser.rs
📜 Recent review details
⏰ Context from checks skipped due to timeout. (9)
  • GitHub Check: cubic · AI code reviewer
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: github-advanced-security
  • GitHub Check: Clippy
  • GitHub Check: Build
  • GitHub Check: Analyze (python)
  • GitHub Check: Analyze (actions)
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (javascript-typescript)

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Improved Python type parsing for generic annotations, whitespace variations, and similarly named types.
    • Prevented invalid TypeScript brace ranges from causing errors.
    • Improved inbound link matching by normalizing paths and selecting the most specific match deterministically.
  • Tests
    • Added coverage for Python generic annotations and overlapping inbound link paths.
  • Chores
    • Updated repository ignore rules for build and library files.

Walkthrough

The changes update ignore rules, Python and TypeScript signature parsing, and inbound suffix matching. Python generic annotations now use token boundaries. TypeScript rejects reversed brace ranges. Suffix matching normalizes paths and selects the longest deterministic match.

Changes

Ignore rule updates

Layer / File(s) Summary
Ignore pattern corrections
.repowiseIgnore
The blanket build.rs exclusion is removed. mod.rs and lib.rs exclusions remain. crates/external-lint/src/lib.rs is explicitly re-included.

Signature parser corrections

Layer / File(s) Summary
Signature validation fixes
crates/shared/src/common/utility_signature_parser.rs, crates/shared/tests/unit_shared_common_vo.rs
Python parameter and return detection validates whole tokens and excludes parameterized list and dict types, including spaced brackets. TypeScript brace_pair rejects reversed ranges. Tests cover spaced Python generics.

Taxonomy suffix matching

Layer / File(s) Summary
Deterministic suffix selection
crates/shared/src/filesystem/taxonomy_filesystem_vo.rs, crates/shared/tests/unit_shared_filesystem_vo.rs
boundary_suffix normalizes paths, evaluates all boundary-aligned suffixes, and selects the longest match. Equal-length matches use lexicographic order. Tests cover the most specific suffix.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Poem

I hop through rules where build files hide,
And guard each token’s outer side.
Braces now point the proper way,
Suffixes choose the longest ray.
Thump, thump—clean paths greet the day!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies that the pull request fixes verified AI review findings from PR #229.
Description check ✅ Passed The description directly explains the parser fixes, deterministic path matching, ignore-rule changes, and verification results.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/reviewer-fixes-20260812
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/reviewer-fixes-20260812

Comment @coderabbitai help to get the list of available commands.

@codacy-production

codacy-production Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -25 complexity · 0 duplication

Metric Results
Complexity -25
Duplication 0

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

@codacy-production codacy-production Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

The PR aims to resolve technical debt and bugs identified in previous reviews, specifically around string parsing and filesystem mapping. Although Codacy indicates the PR is up to standards, manual inspection revealed a high-severity off-by-one error in boundary_ends_with that will cause path boundary checks to fail.

Furthermore, while the intent was to fix Python primitive type detection, the fix was only partially applied to return signatures, leaving parameter signatures inconsistent. The determinism logic in boundary_suffix also requires a tie-breaker for equal-length matches to be truly stable. The absence of new test cases for the panic guards and mapping logic is a significant gap given the nature of the fixes.

About this PR

  • This PR addresses several logic bugs and potential panics, but does not include new test cases to verify these fixes or prevent regressions. It is recommended to add unit tests for the reversed brace scenario in brace_pair and the longest-match logic in boundary_suffix.
  • The improvement to Python primitive detection was applied to collect_python_return_primitives but missed in collect_python_param_primitives (lines 186-203). The latter still uses .contains(': int'), which will continue to produce false positives (e.g., matching : internal_id as int). Please apply the whole-token matching logic consistently across both functions.
1 comment outside of the diff
crates/shared/src/filesystem/taxonomy_filesystem_vo.rs

line 633 🔴 HIGH RISK
The path boundary check incorrectly inspects the first character of the suffix instead of the character immediately preceding it. This will cause suffix matching to fail unless the separator is explicitly included in the match string.

            .get(before - 1)

Test suggestions

  • Test brace_pair with reversed braces (e.g., '} {') to ensure it returns None instead of a range that would panic.
  • Verify collect_python_return_primitives correctly ignores types that merely start with a primitive name (e.g., 'IntervalVO', 'strategy').
  • Verify collect_python_return_primitives correctly identifies bare primitives ('int', 'str', 'float').
  • Test boundary_suffix with multiple overlapping matches to ensure the longest (most specific) match is returned.
  • Verify is_token_end helper correctly identifies non-identifier characters and EOF as valid token boundaries.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Test `brace_pair` with reversed braces (e.g., '} {') to ensure it returns None instead of a range that would panic.
2. Verify `collect_python_return_primitives` correctly ignores types that merely start with a primitive name (e.g., 'IntervalVO', 'strategy').
3. Verify `collect_python_return_primitives` correctly identifies bare primitives ('int', 'str', 'float').
4. Test `boundary_suffix` with multiple overlapping matches to ensure the longest (most specific) match is returned.
5. Verify `is_token_end` helper correctly identifies non-identifier characters and EOF as valid token boundaries.

TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback

None
};
if let Some(len) = matched_len
&& best.is_none_or(|(_, best_len)| len > best_len)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

The implementation remains non-deterministic when multiple keys have the same matched length because HashMap iteration order is unstable. Add a lexicographical tie-breaker to ensure consistent results.

Try updating the boundary_suffix loop to track the matching key and use it as a tie-breaker when matched_len == best_len.

Comment on lines +233 to +235
s.as_bytes()
.get(prefix_len)
.is_none_or(|b| !b.is_ascii_alphanumeric() && *b != b'_')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ LOW RISK

Suggestion: Use Unicode-aware logic to identify token boundaries, as Python identifiers may contain non-ASCII characters. is_ascii_alphanumeric may incorrectly flag the end of a token if followed by a Unicode identifier character.

Suggested change
s.as_bytes()
.get(prefix_len)
.is_none_or(|b| !b.is_ascii_alphanumeric() && *b != b'_')
s.get(prefix_len..)
.and_then(|suffix| suffix.chars().next())
.map_or(true, |c| !c.is_alphanumeric() && c != '_')

Comment thread .repowiseIgnore Outdated
**/lib.rs
**/build.rs
!crates/external-lint/src/lib.rs
!crates/shared/build.rs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ LOW RISK

Nitpick: This inclusion is redundant because the general **/build.rs exclusion was removed. Consider reverting the removal of **/build.rs if you want to keep other build scripts ignored.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@crates/shared/src/common/utility_signature_parser.rs`:
- Around line 221-226: Update the bare-type checks in the return-type parsing
logic to trim whitespace from the suffix after the list/dict token before
testing for a generic bracket, so spaced annotations such as list [ResultVO] and
dict [KeyVO, ValueVO] are not flagged. Add regression coverage for both
annotations while preserving detection of genuinely unparameterized list and
dict types.

In `@crates/shared/src/filesystem/taxonomy_filesystem_vo.rs`:
- Around line 606-615: Update the reverse suffix-match branch in the candidate
scoring logic to record k_clean.len() rather than clean.len(), so longer
matching candidate keys receive higher scores. Add a regression test covering
multiple matching suffixes, such as /src/a.rs and /a.rs, and assert that the
more-specific key is selected.
🪄 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: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 6149324f-8168-4e26-9324-8dd326f20f48

📥 Commits

Reviewing files that changed from the base of the PR and between 5a9a19b and a13c498.

📒 Files selected for processing (3)
  • .repowiseIgnore
  • crates/shared/src/common/utility_signature_parser.rs
  • crates/shared/src/filesystem/taxonomy_filesystem_vo.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Build
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (python)
⚠️ CI failures not shown inline (2)

GitHub Actions: Code scanning AI findings on PR #231 / github-advanced-security: Code scanning AI findings on PR #231

Conclusion: failure

View job details

##[group]Run set -euo pipefail
 �[36;1mset -euo pipefail�[0m
 �[36;1mecho "RUNNER_TEMP=$RUNNER_TEMP"�[0m
 �[36;1mfind "$RUNNER_TEMP" -maxdepth 1 -type f -name 'git-credentials-*.config' -print -delete�[0m
 �[36;1m�[0m
 �[36;1m# Generate a unique token and stop processing workflow commands to prevent the runtime from injecting commands�[0m
 �[36;1mSTOP_***REDACTED_SECRET_ASSIGNMENT*** /proc/sys/kernel/random/uuid)�[0m
 �[36;1m�[0m
 �[36;1m# Use a trap to ensure we always resume command processing and check for�[0m
 �[36;1m# fallback error annotations, even if the runtime exits with a non-zero code�[0m
 �[36;1m# (which would otherwise cause set -e to abort the shell before we get here).�[0m
 �[36;1m# The trap preserves the original exit code.�[0m
 �[36;1mcopilot_cleanup() {�[0m
 �[36;1m  �[0m
 �[36;1m  if [ -n "${GIT_PROXY_PID:-}" ] && kill -0 "$GIT_PROXY_PID" 2>/dev/null; then�[0m
 �[36;1m    echo "Stopping git-proxy (pid=$GIT_PROXY_PID)..."�[0m
 �[36;1m    kill "$GIT_PROXY_PID" 2>/dev/null || true�[0m
 �[36;1m    for _ in {1..25}; do�[0m
 �[36;1m      if ! kill -0 "$GIT_PROXY_PID" 2>/dev/null; then break; fi�[0m
 �[36;1m      sleep 0.2�[0m
 �[36;1m    done�[0m
 �[36;1m    if kill -0 "$GIT_PROXY_PID" 2>/dev/null; then�[0m
 �[36;1m      echo "git-proxy did not stop gracefully; forcing termination."�[0m
 �[36;1m      kill -KILL "$GIT_PROXY_PID" 2>/dev/null || true�[0m
 �[36;1m    fi�[0m
 �[36;1m    wait "$GIT_PROXY_PID" 2>/dev/null || true�[0m
 �[36;1m  fi�[0m
 �[36;1m  �[0m
 �[36;1m  echo "::$STOP_***REDACTED_SECRET_ASSIGNMENT***
 �[36;1m  FALLBACK_FILE="${RUNNER_TEMP}/copilot-fallback-error.txt"�[0m
 �[36;1m  if [ -f "$FALLBACK_FILE" ]; then�[0m
 �[36;1m    FALLBACK_MSG=$(head -c 500 "$FALLBACK_FILE" | tr -d '\n\r')�[0m
 �[36;1m    echo "::error title=Copilot Error::${FALLBACK_MSG}"�[0m

GitHub Actions: Code scanning AI findings on PR #231 / 0_github-advanced-security.txt: Code scanning AI findings on PR #231

Conclusion: failure

View job details

##[group]Run set -euo pipefail
 �[36;1mset -euo pipefail�[0m
 �[36;1mecho "RUNNER_TEMP=$RUNNER_TEMP"�[0m
 �[36;1mfind "$RUNNER_TEMP" -maxdepth 1 -type f -name 'git-credentials-*.config' -print -delete�[0m
 �[36;1m�[0m
 �[36;1m# Generate a unique token and stop processing workflow commands to prevent the runtime from injecting commands�[0m
 �[36;1mSTOP_***REDACTED_SECRET_ASSIGNMENT*** /proc/sys/kernel/random/uuid)�[0m
 �[36;1m�[0m
 �[36;1m# Use a trap to ensure we always resume command processing and check for�[0m
 �[36;1m# fallback error annotations, even if the runtime exits with a non-zero code�[0m
 �[36;1m# (which would otherwise cause set -e to abort the shell before we get here).�[0m
 �[36;1m# The trap preserves the original exit code.�[0m
 �[36;1mcopilot_cleanup() {�[0m
 �[36;1m  �[0m
 �[36;1m  if [ -n "${GIT_PROXY_PID:-}" ] && kill -0 "$GIT_PROXY_PID" 2>/dev/null; then�[0m
 �[36;1m    echo "Stopping git-proxy (pid=$GIT_PROXY_PID)..."�[0m
 �[36;1m    kill "$GIT_PROXY_PID" 2>/dev/null || true�[0m
 �[36;1m    for _ in {1..25}; do�[0m
 �[36;1m      if ! kill -0 "$GIT_PROXY_PID" 2>/dev/null; then break; fi�[0m
 �[36;1m      sleep 0.2�[0m
 �[36;1m    done�[0m
 �[36;1m    if kill -0 "$GIT_PROXY_PID" 2>/dev/null; then�[0m
 �[36;1m      echo "git-proxy did not stop gracefully; forcing termination."�[0m
 �[36;1m      kill -KILL "$GIT_PROXY_PID" 2>/dev/null || true�[0m
 �[36;1m    fi�[0m
 �[36;1m    wait "$GIT_PROXY_PID" 2>/dev/null || true�[0m
 �[36;1m  fi�[0m
 �[36;1m  �[0m
 �[36;1m  echo "::$STOP_***REDACTED_SECRET_ASSIGNMENT***
 �[36;1m  FALLBACK_FILE="${RUNNER_TEMP}/copilot-fallback-error.txt"�[0m
 �[36;1m  if [ -f "$FALLBACK_FILE" ]; then�[0m
 �[36;1m    FALLBACK_MSG=$(head -c 500 "$FALLBACK_FILE" | tr -d '\n\r')�[0m
 �[36;1m    echo "::error title=Copilot Error::${FALLBACK_MSG}"�[0m
🔇 Additional comments (3)
.repowiseIgnore (1)

28-31: 🎯 Functional Correctness

Keep the global barrel patterns.

The global **/mod.rs and **/lib.rs matches contain only module declarations and re-exports. Logic-bearing files are covered by separate test or workspace rules, and crates/external-lint/src/lib.rs is re-included.

			> Likely an incorrect or invalid review comment.
crates/shared/src/common/utility_signature_parser.rs (2)

211-220: LGTM!


312-314: LGTM!

Comment thread crates/shared/src/common/utility_signature_parser.rs Outdated
Comment thread crates/shared/src/filesystem/taxonomy_filesystem_vo.rs Outdated
1. boundary_ends_with: normalize leading `./`/`/` on both sides before the
   boundary check — a suffix like `/b_vo.rs` is treated the same as
   `b_vo.rs`, and the separator check applies at the byte before the
   matched suffix. Fixes the off-by-one reported by Codacy without
   breaking the existing boundary tests.
2. boundary_suffix: score reverse-direction candidates by k_clean.len()
   (key specificity) instead of clean.len(), so the most specific key
   always wins; tie-break equal lengths lexicographically.
3. collect_python_param_primitives: whole-token matching via
   contains_token_prefix (`: int` fires, `: internal_id` does not) —
   matches the return-side fix, Unicode-aware.
4. collect_python_return_primitives: allow whitespace before generic
   brackets (`list [ResultVO]`, `dict [KeyVO, ValueVO]` are parameterized,
   not bare).
5. Tests: regression coverage for spaced generic brackets, most-specific
   key selection, and bare list/dict still flagged.

Verification: clippy clean, 304 shared tests pass, self-lint 0 violations.

Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
@github-actions github-actions Bot added the tests label Aug 11, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 5 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread crates/shared/src/filesystem/taxonomy_filesystem_vo.rs Outdated
Comment thread crates/shared/src/common/utility_signature_parser.rs Outdated
1. boundary_suffix: score candidates by normalized key length. Keys are
   normalized via normalize_path (strips leading ./ and /) before scoring,
   so /src/a.rs and src/a.rs are treated as equally specific — the leading
   slash can no longer tip the selection. best now stores an owned String
   for the tie-break key.
2. collect_python_param_primitives: use contains_bare_token for list/dict
   so `x: list [ResultVO]` (space before bracket) is parameterized, not a
   bare list — consistent with the return-side fix. Previously the guard
   only matched the no-space `: list[` form.

Verification: clippy clean, 304 shared tests pass, self-lint 0 violations.

Co-authored-by: CommandCodeBot <noreply@commandcode.ai>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@crates/shared/src/common/utility_signature_parser.rs`:
- Around line 209-219: Update contains_bare_token to trim leading whitespace
from rest before checking whether the next token starts with '['; spaced generic
annotations such as list [T] and dict [K, V] must not be classified as bare
primitives. Add parameter regression cases covering both forms.
🪄 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: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: a4890304-f6c9-459b-833f-fa2e7a127284

📥 Commits

Reviewing files that changed from the base of the PR and between a13c498 and 20ea8af.

📒 Files selected for processing (5)
  • .repowiseIgnore
  • crates/shared/src/common/utility_signature_parser.rs
  • crates/shared/src/filesystem/taxonomy_filesystem_vo.rs
  • crates/shared/tests/unit_shared_common_vo.rs
  • crates/shared/tests/unit_shared_filesystem_vo.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • .repowiseIgnore
  • crates/shared/src/filesystem/taxonomy_filesystem_vo.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
  • GitHub Check: cubic · AI code reviewer
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: github-advanced-security
  • GitHub Check: Build
  • GitHub Check: Clippy
  • GitHub Check: Analyze (python)
  • GitHub Check: Analyze (actions)
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
crates/shared/tests/unit_shared_filesystem_vo.rs (1)

202-210: LGTM!

Comment thread crates/shared/src/common/utility_signature_parser.rs
…st/dict

contains_bare_token now trims whitespace after the token before checking
for `[`, so parameter annotations like `items: list [ResultVO]` and
`mapping: dict [KeyVO, ValueVO]` are treated as parameterized — not bare
primitives. Adds parameter-side regression coverage.

Verification: clippy clean, 304 shared tests pass, self-lint 0 violations.

Co-authored-by: CommandCodeBot <noreply@commandcode.ai>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 2 files (changes from recent commits).

Confidence score: 4/5

  • In crates/shared/src/filesystem/taxonomy_filesystem_vo.rs, boundary-suffix winner selection is still nondeterministic when two mapping keys normalize to the same value, so different runs can pick different winners and produce inconsistent taxonomy resolution — add a deterministic tie-break for equal normalized keys (or preserve stable ordering explicitly).
  • In crates/shared/src/filesystem/taxonomy_filesystem_vo.rs, normalization does not fully collapse mixed prefixes like /./..., which means equivalent paths can score differently by k_norm.len() and select the wrong “most specific” mapping — expand normalization to handle mixed-prefix forms before suffix scoring.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/shared/src/filesystem/taxonomy_filesystem_vo.rs">

<violation number="1" location="crates/shared/src/filesystem/taxonomy_filesystem_vo.rs:621">
P2: Boundary-suffix winner selection can still vary across runs when two mapping keys normalize to the same value. The equal-length tie-break only uses strict `<` on `k_norm`, so equal normalized keys keep whichever entry HashMap yields first; adding a secondary stable comparator (for example raw key) would keep this deterministic.</violation>

<violation number="2" location="crates/shared/src/filesystem/taxonomy_filesystem_vo.rs:634">
P2: Path normalization misses mixed prefixes like `/./...`, so equivalent keys do not always normalize identically before suffix scoring. That can skew "most specific" selection because `k_norm.len()` is used as the score.</violation>
</file>

Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic


/// Strip leading `./` and `/` so equivalent path forms compare identically.
fn normalize_path(p: &str) -> &str {
p.trim_start_matches("./").trim_start_matches('/')

@cubic-dev-ai cubic-dev-ai Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Path normalization misses mixed prefixes like /./..., so equivalent keys do not always normalize identically before suffix scoring. That can skew "most specific" selection because k_norm.len() is used as the score.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/shared/src/filesystem/taxonomy_filesystem_vo.rs, line 634:

<comment>Path normalization misses mixed prefixes like `/./...`, so equivalent keys do not always normalize identically before suffix scoring. That can skew "most specific" selection because `k_norm.len()` is used as the score.</comment>

<file context>
@@ -595,39 +595,45 @@ impl InboundLinkMap {
 
+/// Strip leading `./` and `/` so equivalent path forms compare identically.
+fn normalize_path(p: &str) -> &str {
+    p.trim_start_matches("./").trim_start_matches('/')
+}
+
</file context>
Suggested change
p.trim_start_matches("./").trim_start_matches('/')
p.trim_start_matches('/').trim_start_matches("./").trim_start_matches('/')
Fix with cubic

let better = match best {
None => true,
Some((_, best_len, ref best_key)) => {
len > best_len || (len == best_len && k_norm < best_key.as_str())

@cubic-dev-ai cubic-dev-ai Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Boundary-suffix winner selection can still vary across runs when two mapping keys normalize to the same value. The equal-length tie-break only uses strict < on k_norm, so equal normalized keys keep whichever entry HashMap yields first; adding a secondary stable comparator (for example raw key) would keep this deterministic.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/shared/src/filesystem/taxonomy_filesystem_vo.rs, line 621:

<comment>Boundary-suffix winner selection can still vary across runs when two mapping keys normalize to the same value. The equal-length tie-break only uses strict `<` on `k_norm`, so equal normalized keys keep whichever entry HashMap yields first; adding a secondary stable comparator (for example raw key) would keep this deterministic.</comment>

<file context>
@@ -595,39 +595,45 @@ impl InboundLinkMap {
-                Some((_, best_len, best_key)) => {
-                    len > best_len || (len == best_len && k_clean < best_key)
+                Some((_, best_len, ref best_key)) => {
+                    len > best_len || (len == best_len && k_norm < best_key.as_str())
                 }
             };
</file context>
Fix with cubic

@rakaarwaky
rakaarwaky merged commit fd31ace into main Aug 12, 2026
16 of 17 checks passed
@rakaarwaky
rakaarwaky deleted the fix/reviewer-fixes-20260812 branch August 12, 2026 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: high High priority issue rust shared shared crate tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant