Skip to content

chore: drop stale T0 reference in synchronize comment - #72

Closed
ChonSong wants to merge 8 commits into
mainfrom
ws3-livetest-pr
Closed

ChonSong wants to merge 8 commits into
mainfrom
ws3-livetest-pr

Conversation

@ChonSong

@ChonSong ChonSong commented Aug 7, 2026 •

Copy link
Copy Markdown
Owner

Trivial comment-only change: the synchronize rate-limiter comment still said 'avoid T0 flooding', but the PR path now runs the deterministic companion flow (WS-3).

Summary by CodeRabbit

  • Tests
    • Added internal helpers to support synchronization testing by normalizing item keys and safely handling conversion errors.
  • Documentation
    • Clarified rate-limiting guidance by removing outdated terminology.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@ChonSong, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 34 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6c498a90-df14-4af3-8c6b-e396467e04f0

📥 Commits

Reviewing files that changed from the base of the PR and between 3628105 and 06df591.

📒 Files selected for processing (1)
  • riptide/webhook.py
📝 Walkthrough

Walkthrough

The webhook module adds two private synchronization test helpers. They suppress per-item exceptions, sort extracted keys, or stringify items. The synchronization rate-limit comment is also updated.

Changes

Synchronization Test Helpers

Layer / File(s) Summary
Deterministic synchronization helpers
riptide/webhook.py
Adds _sync_test_helper and _sync_test_helper_delta for deterministic synchronization-path testing. Both ignore per-item exceptions. The first sorts extracted keys, and the second returns stringified items. The rate-limit comment removes T0-specific wording.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Poem

A rabbit checks each syncing key,
Sorts them neatly, row by row.
Errors vanish, strings appear,
Rate-limit words grow clear.
Hop, hop—the tests can go!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the main change but omits the required Summary, Changes, and Test plan sections from the repository template. Use the repository template and add the required Summary, Changes, and Test plan sections, including the applicable test results.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: removing the stale T0 reference from the synchronize comment.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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

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.

❤️ Share

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

@riptide-review riptide-review Bot added bot/labeled Bot has applied labels to this item (dedup guard) comp/github-app GitHub App plumbing: JWT auth, webhook server, installation sync priority/low Cosmetic, nice to have, P3-equivalent scope/tiny < 10 lines changed sweeper:blast-contained Blast radius: contained — one narrow path / opt-in / few users type/refactor Code restructuring, no behavior change labels Aug 7, 2026
@riptide-review riptide-review Bot added scope/small 10–99 lines changed and removed scope/tiny < 10 lines changed labels Aug 7, 2026
@riptide-review

riptide-review Bot commented Aug 7, 2026 •

Copy link
Copy Markdown

✨ ⚠️ Review

@ChonSong:
⚠️ 3 error_handling issues found

  • 🟡 Exception silently ignored (pass or empty body) (riptide/tests/test_diff_analyzer.py)
  • 🟡 Exception silently ignored (pass or empty body) (riptide/webhook.py)
  • 🟡 Exception silently ignored (pass or empty body) (riptide/webhook.py)

🤖 Generated by Riptide · deterministic analysis (Phase 1) · 🔍 enrichment in progress…

@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 `@riptide/webhook.py`:
- Around line 114-117: Update _sync_test_helper and _sync_test_helper_delta to
stop swallowing all exceptions while parsing fixture items. Catch only the
specific malformed-item exceptions allowed by the WS-3 fixture contract, and
re-raise unexpected failures from item["key"] or str(item); add or update tests
covering both accepted malformed items and propagated unexpected exceptions.
🪄 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: f9bc8dfa-405a-4776-b676-6cee097cc138

📥 Commits

Reviewing files that changed from the base of the PR and between 5629911 and 3628105.

📒 Files selected for processing (1)
  • riptide/webhook.py

Comment thread riptide/webhook.py
Comment on lines +114 to +117
try:
keys.append(item["key"])
except Exception as e: # noqa: BLE001 - finding: silently ignored exception
pass

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.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

configs="$(fd -HI -t f '^(pyproject\.toml|ruff\.toml|\.ruff\.toml|setup\.cfg)$' . || true)"
while IFS= read -r file; do
  [ -z "$file" ] && continue
  rg -n 'S110|select|extend-select|ignore|extend-ignore' "$file" || true
done <<< "$configs"

if command -v ruff >/dev/null 2>&1; then
  ruff check riptide/webhook.py --select S110
fi

Repository: ChonSong/riptide

Length of output: 2228


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== webhook.py relevant sections =="
sed -n '90,135p' riptide/webhook.py | nl -ba -v90

echo
echo "== function definitions / calls =="
rg -n "def .*sync|def .*fixture|fixtures\.|riptide\.webhook|webhook\." -S . | head -n 120

echo
echo "== tests mentioning webhook / fixtures / sync =="
rg -n "fixtures|sync|webhook|key|str\(item\)|riptide/webhook\|riptide\.webhook" -S tests . --glob '!__pycache__/**' | head -n 200

echo
echo "== pyproject ruff config =="
sed -n '1,80p' pyproject.toml | nl -ba -v1

Repository: ChonSong/riptide

Length of output: 229


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== webhook.py relevant sections =="
sed -n '90,135p' riptide/webhook.py | awk '{print NR+89 ":" $0}'

echo
echo "== function definitions / calls =="
rg -n "def .*sync|def .*fixture|fixtures\.|riptide\.webhook|webhook\." -S . | head -n 120

echo
echo "== tests mentioning webhook / fixtures / sync =="
rg -n "fixtures|sync|webhook|key|str\(item\)|riptide/webhook\|riptide\.webhook" -S tests . --glob '!__pycache__/**' | head -n 200

echo
echo "== pyproject ruff config =="
sed -n '1,80p' pyproject.toml | awk '{print NR ":" "$0"}'

Repository: ChonSong/riptide

Length of output: 26759


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== riptide/webhook.py lines 330-365 =="
sed -n '330,365p' riptide/webhook.py | awk '{print NR+329 ":" $0}'

echo
echo "== all helpers usages in webhook.py =="
rg -n "_sync_test_helper(_delta)?\(" riptide/webhook.py

echo
echo "== riptide/tests path =="
find riptide/tests -maxdepth 2 -type f -print | sort | awk '{print NR ":" $0}' | tail -n 80

echo
echo "== relevant pytest references =="
rg -n "_sync_test_helper(_delta)?\(|sync_test_helper|delta|fixture|synchronize|pull_request" riptide/tests riptide -S | head -n 200

echo
echo "== pyproject ruff config =="
sed -n '1,40p' pyproject.toml | awk '{print NR ":" "$0"}'

Repository: ChonSong/riptide

Length of output: 18817


Do not hide fixture parsing exceptions in the sync helpers.

These _sync_test_helper and _sync_test_helper_delta helpers catch every Exception and drop the item. An unexpected item["key"] or str(item) failure can make the live sync fixture produce fewer expected results while avoiding a visible failure. Catch only malformed-item exceptions that support the WS-3 fixture contract, or re-raise unexpected exceptions and add tests for the contract.

Also applies to: 126-129

🧰 Tools
🪛 Ruff (0.16.1)

[error] 116-117: try-except-pass detected, consider logging the exception

(S110)

🤖 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 `@riptide/webhook.py` around lines 114 - 117, Update _sync_test_helper and
_sync_test_helper_delta to stop swallowing all exceptions while parsing fixture
items. Catch only the specific malformed-item exceptions allowed by the WS-3
fixture contract, and re-raise unexpected failures from item["key"] or
str(item); add or update tests covering both accepted malformed items and
propagated unexpected exceptions.

Source: Linters/SAST tools

ChonSong added a commit that referenced this pull request Aug 7, 2026
…iews (#73)

The mapping loop compared lstrip()ed patch lines against stripped added
lines, but patch lines retain their '+'/' ' diff prefixes, so lstrip()
never removed them and the mapping was always empty. The get(i, i)
fallback then misaligned indices whenever context lines preceded a
finding — the synchronize re-review (delta) case — silently dropping
error-handling findings on re-sync.

Fix: strip the diff prefix before comparing. Verified against the live
PR #72 delta (0 findings -> 1) and full PR diff (regression: 2 findings).
Regression test added; suite 551 passing.

Co-authored-by: Hermes Agent <agent@chonsong.io>
@ChonSong

ChonSong commented Aug 7, 2026

Copy link
Copy Markdown
Owner Author

Closing as the WS-3 live-test vehicle — the sync-path finding fixtures served their purpose (they exposed the delta-mapping bug fixed in #73 and verified the canonical-thread PATCH path). The one real change (drop stale T0 reference) is preserved in #74.

@ChonSong ChonSong closed this Aug 7, 2026
@ChonSong
ChonSong deleted the ws3-livetest-pr branch August 7, 2026 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot/labeled Bot has applied labels to this item (dedup guard) comp/github-app GitHub App plumbing: JWT auth, webhook server, installation sync priority/low Cosmetic, nice to have, P3-equivalent scope/small 10–99 lines changed sweeper:blast-contained Blast radius: contained — one narrow path / opt-in / few users type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant