review: drip-77 batch 3 — OpenHands/crush + INDEX update (8 PRs across 8 repos)

Author: Bojun-Vvibe

reviews/2026-W17/drip-77/All-Hands-AI-OpenHands-pr-14127.md

@@ -0,0 +1,82 @@
+---
+pr: 14127
+repo: All-Hands-AI/OpenHands
+sha: ef76371b4e7c
+verdict: merge-after-nits
+date: 2026-04-26
+---
+
+# All-Hands-AI/OpenHands #14127 — Reduce GitHub resolver comment noise by editing acknowledgement comment
+
+- **URL**: https://github.com/All-Hands-AI/OpenHands/pull/14127
+- **Author**: Lumen-Founder
+- **Head SHA**: ef76371b4e7c
+- **Size**: +436/-16 across 5 files (`enterprise/integrations/github/github_comment_utils.py` (new), `github_manager.py`, `github_v1_callback_processor.py`, two test files)
+
+## Scope
+
+Replaces the previous "post a brand-new summary comment when the resolver finishes" behavior with "find the existing acknowledgement comment and edit it in place". Goal: cut comment-thread noise on GitHub issues that the OpenHands resolver agent works on.
+
+Mechanism:
+
+- **`build_ack_marker(conversation_id) → "<!-- openhands-ack:{uuid} -->"`** (`github_comment_utils.py:17`): hidden HTML-comment marker embedded in every acknowledgement comment so the final-summary pass can find it.
+- **`append_ack_marker(message, conversation_id)`** (`:22`): idempotent append (no-op if marker already present, handles trailing newline).
+- **`ensure_conversation_link(message, conversation_id)`** (`:31`): idempotent injection of the conversation tracking link.
+- **`build_final_resolver_comment(summary, conversation_id)`** (`:44`): composes the final body = `ensure_conversation_link(summary.strip())` + `append_ack_marker`.
+- **`iter_recent_paginated_items(paginated_list, max_items=None)`** (`:50`): newest-first iteration over a PyGithub paginated list by walking pages in reverse.
+
+Wiring:
+
+- `github_manager.py:411`: every initial acknowledgement comment now appends the marker via `append_ack_marker(msg_info, conversation_id)` so the final pass can find it later.
+- `github_v1_callback_processor.py:81`: callback processor calls `_post_final_summary_to_github(summary, conversation_id)` instead of `_post_summary_to_github(summary)`.
+- `github_v1_callback_processor.py:126-227`: the new method searches both issue comments and PR review comments (200 most-recent) for the marker, edits the matched comment in place with the final body, and falls back to creating a new comment if the marker isn't found.
+
+## Specific findings
+
+- **The "edit existing comment" pattern is the right UX choice** for resolver-style bots that can otherwise leave 3-5 comments per issue (ack → progress → progress → summary). GitHub's notification system fires once for the original ack and once for the edit, vs. once per new comment, so noise reduction is real for subscribers. Good direction.
+
+- **HTML-comment marker (`<!-- openhands-ack:{uuid} -->`) is the standard pattern** used by dependabot, renovate, github-actions/stale, etc. Invisible to humans, greppable, idempotent. `build_ack_marker` correctly uses the conversation_id as the discriminator so two concurrent resolver runs on the same issue don't stomp each other's markers. Good.
+
+- **`iter_recent_paginated_items` reads as suspicious at a glance.** It calls `paginated_list.totalCount` (a property that triggers a network round-trip in PyGithub to learn the page count) and then walks pages in reverse. Two concerns:
+  - **Cost**: `totalCount` on a large issue's comment list does a HEAD-style request. For an issue with hundreds of comments, this is one extra round-trip per resolver-finish event. Acceptable but worth knowing.
+  - **Race**: between reading `totalCount` and walking pages, new comments can be added. A new comment posted during iteration would shift page boundaries, potentially causing duplicate or missed items. For a marker-search use-case, that's tolerable because we're looking for an old comment, not a newly-posted one — but doc-comment that limitation.
+  - **Page ordering assumption**: the docstring at `:55-58` says "This assumes the underlying pages are ordered oldest-first". Verify this against PyGithub's `IssueComment` paginated list — if it's actually newest-first (some endpoints are), the iteration is reversed twice and you get the oldest 200 comments instead of the newest. That would silently miss the marker on any active issue.
+
+- **`max_items=200` cap at the call sites** (`:202`, `:224`) is a reasonable safety bound but could miss the marker on an extremely active issue. The acknowledgement marker is *deterministic* (it's posted by openhands itself), so an alternative architecture is: persist the comment ID in the conversation's metadata at ack-time, then look it up by ID at finish-time. That's O(1), no pagination, no race, no scan. Worth filing as a follow-up — the current approach works but is more brittle than necessary.
+
+- **Searching both issue comments AND PR review comments** (`:202` issue comments, `:224` review comments) is correct because a PR resolver run could ack on either surface. But note: review comments have file/line context that issue comments don't; if the original ack was a review comment with line context, the final-summary edit needs to preserve that context (or downgrade gracefully). Reviewer should confirm `comment.edit(body)` on a review comment doesn't strip the path/position.
+
+- **Fallback to "post new comment if marker not found"** (visible in the docstring around `:130`) is correct defense-in-depth: the resolver's final summary always lands somewhere, even if the ack was deleted manually.
+
+- **`build_final_resolver_comment` composition order** at `:46-47`: `ensure_conversation_link` → `append_ack_marker`. So the final body shape is `<summary>\n\nTrack progress [here](...)\n\n<!-- openhands-ack:{uuid} -->`. The marker stays at the very end, which is correct for both grep-friendliness and human readability (HTML comment is invisible, so the visible tail is the tracking link).
+
+- **`ensure_conversation_link` idempotence at `:34`** checks `if link_fragment in message` — that's a substring match, so a previous edit that included the conversation_id in the body (e.g. in a backtick code block) could falsely satisfy the check and prevent the link from being appended. Tighten to a regex that matches the actual `[here](...conversations/{id})` format if you want to be strict; loose substring match is probably fine in practice.
+
+- **`append_ack_marker` idempotence at `:23-24`**: checks for the literal marker substring. Correct (the marker is unique enough). Handles `endswith('\n')` to control double-newline. Good.
+
+- **`hashable summary.strip()` before composition** (`:46`): correctly handles incoming whitespace from upstream summary generation.
+
+- **Tests (`test_github_comment_utils.py`, `test_github_v1_callback_processor.py`)**: visible in the file list. Reviewer should confirm coverage includes: marker idempotence, link idempotence, marker-not-found fallback, multi-page paginated search, and review-comment-vs-issue-comment routing.
+
+- **PR scoped purely to enterprise/integrations/github/**: no leakage into the open-source surface. Reasonable for an enterprise-feature PR.
+
+## Risk
+
+Low-medium. The mechanism is well-trodden (HTML-comment markers are standard practice). Risk concentration is in `iter_recent_paginated_items` — page-ordering assumption, race-with-new-comments, and the `totalCount` round-trip cost. The persistence-of-ack-comment-id alternative would eliminate those risks entirely but is out of scope for this PR.
+
+## Nits
+
+1. Verify PyGithub's `get_issue_comments()` and `get_review_comments()` paginated lists are oldest-first (matches the iteration assumption); if not, the search silently misses recent comments.
+2. Confirm `comment.edit(body)` on a review comment preserves path/position context.
+3. Consider tightening `ensure_conversation_link`'s substring match to a regex matching the actual `[here](...)` markdown format.
+4. File a follow-up: persist ack comment ID in conversation metadata at ack-time so the final-summary pass is O(1) instead of paginated scan.
+5. Doc-comment the race window in `iter_recent_paginated_items` (new comments arriving during iteration may shift boundaries).
+6. Note the `totalCount` round-trip cost in the function docstring.
+
+## Verdict
+
+**merge-after-nits** — solid noise-reduction approach, idempotent helpers, correct marker scheme, sensible fallback. Page-ordering verification is the gating ask before merge; the rest are doc/follow-up.
+
+## What I learned
+
+The "edit ack comment instead of posting summary" pattern is one of those small UX choices that compounds: every resolver run that hits an issue cuts the comment count from 2 → 1, the notification volume from 2 → 1.5 (edit notifications are softer in most clients), and the "find the bot's status" cognitive load from O(n) scrolling to O(1) (it's always the most recent edit of one specific comment). The implementation cost is the marker scheme + a search-or-fallback pattern, both of which are reusable across any bot that posts to issues. The right architectural endpoint is to persist the comment ID in the conversation's metadata so the search step disappears entirely — but the marker-scan approach is a reasonable starting point and degrades gracefully when the ack comment was deleted manually.

reviews/2026-W17/drip-77/charmbracelet-crush-pr-2694.md

@@ -0,0 +1,71 @@
+---
+pr: 2694
+repo: charmbracelet/crush
+sha: 0491832a3c54
+verdict: merge-as-is
+date: 2026-04-26
+---
+
+# charmbracelet/crush #2694 — fix(skills): deduplicate skills discovered via symlinked directories
+
+- **URL**: https://github.com/charmbracelet/crush/pull/2694
+- **Author**: octo-patch
+- **Head SHA**: 0491832a3c54
+- **Size**: +49/-2 across 2 files (`internal/skills/skills.go`, `internal/skills/skills_test.go`)
+
+## Scope
+
+Fixes duplicate skill loading when the same on-disk skill file is reachable via two discovery paths that resolve to the same target through a symlink — e.g. `~/.config/crush/skills` symlinked to a project's `.claude/skills`, with both paths registered as discovery roots.
+
+The fix at `skills.go:213-228`:
+- After `filepath.WalkDir` reaches a `SKILL.md`, run `filepath.EvalSymlinks(path)` and use the resolved path as the dedup key (`realPath`) for the `seen` map.
+- Fall back to `path` if `EvalSymlinks` errors (preserves existing behavior for the no-symlinks-involved case).
+- Continue to call `Parse(path)` (the original path), so error messages and discovered metadata still reference the discovery-relative path the user wrote.
+
+Test at `skills_test.go:285-326`:
+- Builds a real skills dir + a symlink dir pointing at the same target.
+- Calls `Discover([realDir, symlinkDir])`.
+- Subscribes to `SubscribeEvents` and asserts `len(skills) == 1` and exactly one `StateNormal` state event was emitted.
+- Comment notes "Not parallel: shares global broker with other Discover tests".
+
+## Specific findings
+
+- **Right semantics: dedup by resolved target, parse by discovery path.** This preserves the user's mental model ("the skill I see in my list came from path X") while preventing the broker from double-counting. Excellent split.
+
+- **Fall-through when `EvalSymlinks` errors is conservative-correct.** A broken symlink, permission error, or `EvalSymlinks` returning a path that doesn't exist would all fall back to the original `path` as the dedup key — same as pre-fix behavior. So the worst-case post-PR behavior is "no improvement" rather than "skipped skill".
+
+- **Locking shape is preserved.** `mu.Lock` / check-and-set-`seen` / `mu.Unlock` happens before `Parse`, which is fine because `Parse` is the slow part and we don't want to hold the mutex across IO. The original code had this shape; the fix only changes which key is checked. No new race.
+
+- **`filepath.EvalSymlinks` cost concern is minor.** It's one `lstat`+`readlink` per discovered `SKILL.md` (not per file walked). For repos with O(10) skills, this is negligible. For a hypothetical user with hundreds of skill dirs, it's still bounded by the count of `SKILL.md` files, not directory entries.
+
+- **Test gates correctly: `Not parallel`.** Skills tests share a global broker, and parallel tests would race on the event subscription. The author noted this matches the convention in the rest of the file. Good.
+
+- **Test cleanup**: `t.TempDir()` handles teardown, including the symlink (Go's `os.Symlink` creates a symlink, and `t.TempDir`'s cleanup `os.RemoveAll` removes symlinks without following them). Correct.
+
+- **Edge case not covered**: what if `realDir` is itself reachable via two different symlinked routes from outside the workspace? E.g. `/Volumes/work` symlinks to `/Users/x/work` on macOS and discovery includes both — `EvalSymlinks` resolves to the same canonical path, dedup works. ✓
+- **Edge case partially covered**: what if two *different* skill files have the same name (`SKILL.md` is the only filename matched per `:215`) but live in different real dirs that happen to alias? The dedup key is the *resolved* path of the file itself, so two truly distinct files have two different `realPath` values and both load. ✓
+
+- **Path-canonicalization choice.** `filepath.EvalSymlinks` resolves all components, but doesn't apply OS-specific case-folding (mac/HFS+/APFS) or unicode normalization. On case-insensitive filesystems, two paths differing only in case would resolve to two different `realPath` values and both load — minor, almost certainly out of scope, but worth a one-line comment if anyone has reported it.
+
+- **`Parse(path)` still called with the un-resolved path.** Confirms the user-visible "where this skill came from" stays user-friendly (their configured discovery dir, not the resolved symlink target). If `Parse` writes errors with the path, those errors stay in the user's mental model. Good UX.
+
+- **Comment quality at `:213-217`** is exactly right: identifies the user-visible scenario (`~/.config/crush/skills` symlinked to project's `.claude/skills`), explains the fix in one sentence, points at the symptom. Future maintainer will read this and understand.
+
+- **No drive-by changes**, no formatting churn, no whitespace edits. Tightly scoped diff.
+
+## Risk
+
+Trivially low. Pure addition of a defensive symlink resolution + a regression test. Failure mode is "user with a symlink loop has skills listed once instead of twice", which is unambiguously the intended behavior. No callsite of `Discover` cares about duplicates being silently dropped (the deduplication already existed; this just makes the dedup key correct under symlinks).
+
+## Nits
+
+1. (Optional) one-line comment noting case-insensitive-filesystem edge case is not handled.
+2. (Optional) consider documenting the symlink-resolution policy at the package level (`internal/skills/doc.go` or similar) so future contributors know "we dedup on the resolved target, not the discovery path".
+
+## Verdict
+
+**merge-as-is** — minimal, correctly-shaped, covered by a focused regression test, with a comment that explains the user scenario. Closes a real footgun for any user whose config directory is a symlink to a project skills directory.
+
+## What I learned
+
+`filepath.EvalSymlinks` is the right primitive for filesystem-deduplication. The pattern of "resolve for dedup, original for display" — `realPath` for the `seen` map but `path` for `Parse` — is the same one good archive extractors use ("validate against absolute path, present relative path to user") and Linux mount-namespace tools use ("kernel sees the resolved path, user sees the bind-mount path"). Whenever you have a path used for two purposes (uniqueness + identity), using two different forms of it is almost always correct.