fix: address Codacy post-merge findings on PR #28 - #29
Conversation
- sarif-to-annotations.py: normalize artifactLocation URIs by stripping file:// scheme and leading slashes so GitHub Actions receives a path relative to the repository root - gc-watchdog/watchdog.sh: add head -1 to MAYOR and SUSPENDED grep pipelines for consistency with SESSIONS and CONTROLLER parsing; narrow MAYOR grep to whole-word match on harness.mayor Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe watchdog expands issue filtering and centralizes status parsing. The SARIF converter adds URI, coordinate, input-size, encoding, and schema validation. The merge workflow uses trial merge-tree verification before lower-PR cleanup. Standalone tests cover all three changes. ChangesWatchdog filtering
SARIF conversion
Merge-stack cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
MergerNeeds Review The runbook has a diff-confirmed data-integrity risk: each LOWER_PR uses a fixed placeholder branch instead of resolving its own head branch, so verification and deletion can target the wrong branch. CI also did not run, so human review is needed before merging. Commit |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR addresses Codacy findings from PR #28 with fixes for grep patterns and URI normalization. The bash script changes look good, but there's a critical logic error in the Python URI parsing that would cause incorrect file paths to be sent to GitHub Actions.
Critical Issue
- scripts/sarif-to-annotations.py:198 — URI parsing bug that extracts only the filename instead of the full path for non-file:// schemes, breaking GitHub Actions annotations
Changes Reviewed
- ✅ watchdog.sh: grep improvements with
-wandhead -1for robustness ⚠️ sarif-to-annotations.py: URI normalization logic has a path extraction bug
Please fix the identified issue before merging.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Up to standards ✅🟢 Issues
|
PR Summary by QodoFix Codacy findings: safer SARIF parsing, watchdog parsing, and merge-stack cleanup
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
While this PR is technically 'Up to Standards' according to Codacy, there are significant implementation risks and testing gaps. The logic for URI normalization in scripts/sarif-to-annotations.py is fragile and likely to fail on complex URIs (e.g., those with ports or query strings). In watchdog.sh, the hardening of the 'Suspended:' variable extraction is inconsistent with the 'MAYOR' variable, leaving it vulnerable to false positives from lines like 'Total Suspended:'.
Most importantly, there are no functional tests provided for these changes. The current verification relies solely on syntax validation, which does not confirm that URI paths are correctly relative or that shell extractions handle multi-line inputs as intended.
About this PR
- The PR does not include functional tests to verify the URI normalization logic or the shell script's grep improvements. Relying solely on syntax validation is insufficient for changes affecting path resolution and variable extraction.
Test suggestions
- Verify SARIF 'file://' URIs are converted to relative paths (e.g., 'file:///path/to/file' -> 'path/to/file')
- Verify SARIF URIs with other schemes (e.g., 'https://host/path') have the authority stripped
- Verify 'watchdog.sh' correctly handles multiple 'harness.mayor' matches by selecting the first whole-word match
- Verify 'watchdog.sh' correctly handles multiple 'Suspended:' matches by selecting the first occurrence
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify SARIF 'file://' URIs are converted to relative paths (e.g., 'file:///path/to/file' -> 'path/to/file')
2. Verify SARIF URIs with other schemes (e.g., 'https://host/path') have the authority stripped
3. Verify 'watchdog.sh' correctly handles multiple 'harness.mayor' matches by selecting the first whole-word match
4. Verify 'watchdog.sh' correctly handles multiple 'Suspended:' matches by selecting the first occurrence
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
- sarif-to-annotations.py: replace manual string splitting with
urllib.parse.urlparse to correctly extract the path component
from absolute URIs (file://, https:// with ports/query/fragments).
Fixes logic error where split("/", 3)[-1] dropped intermediate
path segments.
- gc-watchdog/watchdog.sh: add -w flag to Suspended: grep to avoid
matching lines like "Total Suspended:"
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![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
🤖 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 @.agents/skills/gc-watchdog/watchdog.sh:
- Line 60: Update the STATUS parsing assignment for MAYOR to use a literal grep
pattern for “harness.mayor”, preferably with grep -m1 -wF and the existing
status extraction pipeline, so similarly named status lines cannot be selected.
In `@scripts/sarif-to-annotations.py`:
- Around line 191-199: Update the file URI normalization logic around file_uri
to parse URI components before emitting the file value: remove the authority for
file://localhost paths, use only the path without query or fragment, and
preserve unrecognized forms such as FILE:/src/a.ts unchanged. Add regression
tests covering localhost, query/fragment, and uppercase FILE scheme inputs.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 012f8eef-942e-494d-b634-a014c274d3c9
📒 Files selected for processing (2)
.agents/skills/gc-watchdog/watchdog.shscripts/sarif-to-annotations.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Amazon Q Developer
🔇 Additional comments (1)
.agents/skills/gc-watchdog/watchdog.sh (1)
60-60: 🩺 Stability & AvailabilityDo not flag these pipelines for
pipefail.watchdog.shusesset -uo pipefail, notset -e; aSIGPIPEstatus does not stop the watchdog.grep -m1can also causeSIGPIPEfrom the precedingechoon large input.> Likely an incorrect or invalid review comment.
- gc-watchdog/watchdog.sh: use grep -m1 -wF for literal fixed-string match on "harness.mayor" (dot is a regex wildcard in grep -w) - sarif-to-annotations.py: handle file:/ (single slash) URI form in addition to file://, broaden scheme detection to catch FILE:/ uppercase variants Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
baz-reviewer
Bot
added
baz: needs review
baz: ready to merge
and removed
baz: ready to merge
baz: needs review
labels
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
13 tests covering: empty locations, relative paths, absolute paths, file:// (triple slash, localhost, single slash, uppercase scheme), query/fragment stripping, https:// URIs, URL-encoded paths, and region field extraction. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- sarif-to-annotations.py: validate location coordinates are positive
integers before emitting (skip non-int/negative/string values to
prevent workflow-command injection)
- sarif-to-annotations.py: distinguish missing runs field from falsey
non-list values ({} , "", 0, false now exit 2 instead of passing)
- sarif-to-annotations.py: read from stdin.buffer and check byte length
(not character count) for accurate 100MB limit on multibyte UTF-8
- gc-watchdog/watchdog.sh: broaden bead ID regex to sv-[a-z0-9]{3,}
with optional .N hierarchical suffix (was {4,} which excluded
valid 3-char IDs like sv-a1b and hierarchical sv-a1b.1)
- merge-stack.toml: replace git merge-base --is-ancestor with content
diff check for squash merges (squash creates new commit, ancestry
check always fails)
- test_sarif_to_annotations.py: +4 tests for integer validation
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.agents/skills/gc-watchdog/watchdog.sh:
- Line 43: Update the exclusion matching adjacent to the watchdog’s ID
extraction pipeline so the wisp and nudge filters match only complete IDs,
allowing an optional hierarchical suffix and requiring a field boundary.
Preserve exclusions for exact `sv-wisp` and `sv-nudge` IDs while keeping IDs
such as `sv-wispish` and `sv-nudgeable` in the results.
In `@pack/formulas/merge-stack.toml`:
- Around line 105-107: The merge-stack verification currently compares diff
sizes rather than confirming that lower-branch content exists on origin/main.
Replace the DIFF_ON_MAIN/BRANCH_CHANGES threshold logic in the merge decision
with a content-aware Git check, such as a temporary three-way merge or tree
comparison, and only proceed with PR closure and branch deletion when
verification succeeds; retain the branch when verification is inconclusive. Add
a regression case covering a larger unrelated main diff.
In `@scripts/sarif-to-annotations.py`:
- Around line 230-240: The coordinate validation loop in
scripts/sarif-to-annotations.py (lines 230-240) must reject Boolean values
before the integer check so values such as startLine=True are not emitted;
continue accepting only positive integers. Add a regression test in
scripts/test_sarif_to_annotations.py (lines 110-140) covering startLine=True and
asserting that the invalid coordinate is omitted.
In `@scripts/test_sarif_to_annotations.py`:
- Around line 143-161: Add end-to-end tests invoking main for invalid UTF-8
input, malformed runs, and missing or null runs values, then register them in
TESTS. Each case must assert the expected exit status and corresponding error
output while preserving the existing unit-test coverage.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 4b60091f-8cf8-4858-a6ef-70f32eefeb2a
📒 Files selected for processing (4)
.agents/skills/gc-watchdog/watchdog.shpack/formulas/merge-stack.tomlscripts/sarif-to-annotations.pyscripts/test_sarif_to_annotations.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Codacy Static Code Analysis
🧰 Additional context used
🪛 Ruff (0.16.1)
scripts/test_sarif_to_annotations.py
[warning] 20-20: Missing return type annotation for private function _loc
(ANN202)
[warning] 20-20: Missing type annotation for **region
(ANN003)
[warning] 28-28: Missing return type annotation for private function _file_uri
(ANN202)
[warning] 101-101: Missing return type annotation for private function _annotation_line
(ANN202)
[warning] 101-101: Missing type annotation for **region
(ANN003)
[warning] 106-106: Unpacked variable is_error is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 170-172: try-except within a loop incurs performance overhead
(PERF203)
[warning] 173-173: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (9)
scripts/sarif-to-annotations.py (4)
192-198: Normalize relative URI references.Line 195 bypasses parsing for relative SARIF URI references. As a result,
src/a%20b.tsremains encoded, and relative query or fragment components remain infile. Parse and decode every artifact URI before emitting its path.
scripts/sarif-to-annotations.py#L192-L198: applyurlparseandunquoteto relative URI references too.scripts/test_sarif_to_annotations.py#L95-L97: add encoded relative-path and relative query/fragment cases.Proposed fix
- if "://" in file_uri or "file:/" in file_uri.lower(): - parsed = urlparse(file_uri) - file_uri = unquote(parsed.path) + parsed = urlparse(file_uri) + file_uri = unquote(parsed.path)#!/bin/bash set -euo pipefail python3 - <<'PY' from urllib.parse import unquote, urlparse cases = { "src/a%20b.ts": "src/a b.ts", "src/a.ts?line=1#fragment": "src/a.ts", } for uri, expected in cases.items(): actual = unquote(urlparse(uri).path).lstrip("/") assert actual == expected, (uri, actual, expected) PY
33-33: LGTM!
257-274: LGTM!
298-302: LGTM!scripts/test_sarif_to_annotations.py (2)
1-92: LGTM!
164-181: LGTM!.agents/skills/gc-watchdog/watchdog.sh (3)
63-63: Anchor theSuspended:match at the start of the line.
grep -w "Suspended:"still matchesTotal Suspended:. Therefore,head -1can extract the wrong status value. Use an anchored first-match pattern.Proposed fix
- SUSPENDED=$(echo "$STATUS" | grep -w "Suspended:" | head -1 | awk '{print $2}') + SUSPENDED=$(printf '%s\n' "$STATUS" | grep -m1 -E '^[[:space:]]*Suspended:' | awk '{print $2}')
25-26: LGTM!
61-61: LGTM!
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…ests Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
|
|
Running ultrareview automatically — The PR modifies production automation (git merge-stack cleanup with branch deletion, GC watchdog parsing, and SARIF annotation logic) where subtle bugs could cause erroneous branch deletion or broken CI annotations, warranting a deep multi-pass review.. I'll post findings when complete. |
|
I can't run this ultrareview because you've reached your trial's review limit. Trial plans have lower review limits than paid plans. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
There was a problem hiding this comment.
Pull Request Overview
This PR resolves the targeted Codacy findings regarding SARIF normalization and GC watchdog parsing. It also introduces a more robust branch cleanup strategy using git merge-tree to support squash-merge workflows and a new runbook.
While the implementation appears sound and meets the primary acceptance criteria, there is a notable discrepancy between the PR's title/summary ('address 3 Codacy findings') and the actual breadth of changes, which includes significant infrastructure and cleanup logic. Codacy analysis confirms the code is currently up to standards.
About this PR
- The scope of this PR is broader than the '3 Codacy findings' mentioned in the title. It introduces a new runbook and a complex git-based verification system for merge stacks. While the technical quality is high, ensure this additional scope is intended for this specific fix.
Test suggestions
- Verify SARIF artifactLocation URIs are correctly normalized from absolute paths and file:// schemes.
- Ensure SARIF coordinate validation rejects non-integer or non-positive values (including booleans).
- Test that GC watchdog correctly parses individual status fields and ignores repeated or irrelevant lines.
- Confirm that real bead IDs are counted while ephemeral wisp/nudge IDs are excluded in the watchdog.
- Verify that git merge-tree logic correctly identifies content inclusion for squash merges even when ancestry check fails.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
|
Code review by qodo was updated up to the latest commit 15aeef3 |
1 participant
Summary
Addresses 3 Codacy findings that appeared on PR #28 after merge:
scripts/sarif-to-annotations.py:190— Normalize SARIFartifactLocationURIs by strippingfile://scheme and leading slashes so GitHub Actions receives a path relative to the repository root.agents/skills/gc-watchdog/watchdog.sh:60— NarrowMAYORgrep to whole-word match (grep -w) and addhead -1to avoid multi-line variable values.agents/skills/gc-watchdog/watchdog.sh:62— Addhead -1toSUSPENDEDgrep for consistency withSESSIONSandCONTROLLERparsingThreads resolved on merged PR #28: PRRT_kwDOTyoI9s6YYoxo, PRRT_kwDOTyoI9s6YYoxq, PRRT_kwDOTyoI9s6YYoxy
Test plan
python3 -c "import ast; ast.parse(open('scripts/sarif-to-annotations.py').read())"— syntax OKbash -n .agents/skills/gc-watchdog/watchdog.sh— syntax OKGenerated with Devin
Summary by cubic
Fixes post-merge findings from PR #28 by hardening SARIF parsing, GC watchdog parsing/filters, and merge-stack post-merge cleanup. Adds a documented post-merge runbook and regression tests, and restores correct GitHub Actions annotations.
artifactLocationURIs withurllib.parse(supportsfile://,file:/, uppercase,https://), decode to repo-relative paths, and emit only positive-int coordinates (skip booleans/invalid). Enforce a 100 MB byte limit with UTF-8 validation and fail on non-listrunswhile allowing missingruns. Addedscripts/test_sarif_to_annotations.pywith e2e coverage.harness.mayor,Sessions:,Suspended:, andController:with anchored, first-match logic via shared_parse_gc_status; count real beads withsv-[a-zA-Z0-9]{3,}(\.[0-9]+)?; exclude only completewisp/nudgeIDs (including hierarchical). Added test-source markers andscripts/test_gc_watchdog.pyregressions.git merge-tree --write-tree; validate the resulting tree OID (SHA-1 or SHA-256) and type; resolve each lower PR’s head branch viagh pr viewand use it for verification/deletion; fetch origin once and fail closed on fetch; keep unverified lower PRs/branches open. Documented inengdocs/runbooks/merge-stack-post-merge.mdand referenced bypack/formulas/merge-stack.toml. Addedscripts/test_merge_stack.pyandscripts/testlib.py.Written for commit 15aeef3. Summary will update on new commits.