🐛 fix(kanban): dedup review cards on PR URL in create_task - #3
Conversation
cwest
left a comment
cwest
left a comment
There was a problem hiding this comment.
The dedup guard does what it claims. I pulled the head and ran the changed test file in a clean clone: 174 passed / 1 skipped, and the 8 new dedup tests all pass. The two failures I first saw were a missing python-multipart in my venv, not a regression; once installed the file is green.
The design holds up under the cases that matter. The guard sits after the idempotency-key check, so a card with both a key and a PR URL still short-circuits on the key first. The SQL prefilter matches the JSON-quoted token ("github-code-review"), so a skill that only contains the name as a substring doesn't trigger it — the substring test confirms that. Archived cards don't block a re-review, mirroring the existing key semantics. Candidate set is bounded to review cards, so the in-Python canonicalisation stays cheap.
One real gap, not a blocker: owner/repo case isn't normalised. GitHub treats owner/repo case-insensitively, but _canonical_pr_url preserves the captured case, so cwest/hermes-agent and cwest/Hermes-Agent produce different identities and won't dedup. I verified this against the head — host casing and www. collapse, but a repo-case variant slips through. In practice the webhook and a hand-copied URL come from the same place, so they'll match; worth a casefold on owner/repo if you want to close it. Noted inline.
The SELECT-then-INSERT outside a write lock can let two concurrent creators both pass the guard, but that's the same documented race the idempotency-key path already accepts, so it's consistent rather than new.
Leaving this as a comment since the PR is still a draft.
| if not m: | ||
| return None | ||
| return ( | ||
| f"https://github.com/{m.group('owner')}/{m.group('repo')}" |
There was a problem hiding this comment.
Owner/repo are interpolated with their captured case. GitHub resolves owner/repo case-insensitively, so two cards naming cwest/hermes-agent and cwest/Hermes-Agent for the same PR won't dedup here. The host already normalises via the IGNORECASE match; a .casefold() (or .lower()) on owner and repo would make the identity match GitHub's own. Low-likelihood in the webhook+manual path, but it's the one input that defeats the guard.
A second review card for the same PR could be created whenever the idempotency key missed it — specifically a manually-filed review card carrying a different (or absent) key for a PR the webhook also auto-files. The phantom card looks stuck in the pipeline and its bogus gate blocks the downstream merge card. Guard create_task on the PR identity itself: when a card carries the github-code-review skill and names a GitHub PR URL (title first, then body), no-op and return any existing non-archived review card for the same PR. The PR URL is canonicalised (host casing/www, trailing path/query/fragment stripped) so URL variants collapse to one identity. Archived cards do not block, mirroring the idempotency-key semantics, so a deliberate re-review still works. The SQL prefilter matches the JSON-quoted skill token to avoid substring false-positives; the in-Python URL compare is authoritative. Closes the duplicate-review-card path regardless of how the card was filed (webhook retry or hand-built chain).
3147b6e to
755eefc
Compare
cwest
left a comment
cwest
left a comment
There was a problem hiding this comment.
The dedup logic holds up. Membership is checked against the already-cleaned skills list, so github-code-review-extra correctly doesn't match, and the SQL prefilter on the JSON-quoted token plus the in-Python canonical compare gives you both a cheap candidate set and an exact decision. Running the full file against this head: 175 passed, 0 failed, including all eight new dedup cases. Commit is signed and follows the convention.
One thing to confirm, noted inline: the new scan and the existing idempotency-key check are both global across boards/tenants. For a PR-URL identity that's probably what you want, but create_task does take board/tenant, so worth a deliberate decision rather than an accident.
Leaving this as a comment rather than an approval since the PR is still a draft.
| # authoritative check. | ||
| rows = conn.execute( | ||
| "SELECT id, title, body FROM tasks " | ||
| "WHERE status != 'archived' AND skills LIKE ? " |
There was a problem hiding this comment.
This candidate query (like the idempotency-key lookup just above) isn't scoped by board or tenant, so a review card for the same PR URL on a different board would dedup to this one. Given a PR URL is globally unique that's likely the intended semantic, and it matches the existing key-dedup behavior. Flagging only to confirm it's a choice, not an oversight — elsewhere lookups do filter on tenant.
| return None | ||
| if REVIEW_SKILL not in skills_list: | ||
| return None | ||
| return _canonical_pr_url(title) or _canonical_pr_url(body) |
There was a problem hiding this comment.
Title-first then body matches how the webhook files cards. Worth knowing: if a card's title happens to mention a different PR than its body, title wins silently. Not a problem for the webhook path; just the one ambiguity in the precedence.
Deduplicate review cards on their PR URL in create_task so a repeated PR-review request does not open a second review card for the same PR. upstream-pending: fork PR #3
Blocking #1 — gateway-connecting-overlay.tsx reduced-motion regression: the top `if (reduce) setPhase('gone')` fired unconditionally on mount whenever reduce-motion was on, so every OS reduced-motion user lost the CONNECTING overlay during cold boot entirely (jumped to 'gone' before the gateway was even open). The intent was to skip the exit *choreography*, not to skip showing the overlay. Removed the unconditional top block and the redundant nested preview block; kept only the third branch (`gatewayState === 'open' && shownRef.current` → `reduce ? 'gone' : 'text-out'`) which correctly gates the short-circuit on connect. Also fixed `if(reduce)` missing-space, 6-space misindent, and the same 3-line comment pasted three times. Nit #1 — tsconfig excludes e2e, so specs were never typechecked in CI. Added tsconfig.e2e.json (extends base, includes e2e/ + playwright.config.ts, adds @playwright/test types) and wired it into the typecheck script. This surfaced three latent type errors that are fixed in the same commit: - fix-electron-tracing.ts: `app._context` and `electron._playwright` are private APIs — added `as any` on the access before the existing cast. - playwright.config.ts: `reducedMotion: 'reduce'` directly under `use:` is not a valid UseOptions property in playwright 1.58; it's a BrowserContextOption accessed via `contextOptions: { reducedMotion: 'reduce' }`. The old form was silently ignored at runtime, so reduced-motion emulation wasn't actually active — screenshots could catch overlays mid-fade (exactly what the comment warned about). Nit #2 — fix-electron-tracing.ts reaches into Playwright internals (_playwright, _allContexts, _context) with no public contract. Added a header comment calling out the `@playwright/test` exact pin (=1.58.2) so a future bump knows to re-verify the private symbols still exist. Nit #3 — main.ts TEST_WORKER_INDEX block had stray 6-space indentation. Verified: tsc -p . && tsconfig.electron && tsconfig.e2e → 0 errors; vitest boot-failure-overlay (3/3) + boot-failure-reauth (21/21) pass; npm run build clean; playwright e2e/boot-failure.spec.ts 2/2 pass.
…d curator The skill-authoring guide and curator prompt both reference descriptions as the primary discovery mechanism but never mentioned the 57-char system prompt truncation. Add explicit guidance: - Authoring guide: frontmatter docs, template comment, size limits, pitfall #3 with good/bad examples, verification checklist - Curator prompt: parenthetical noting the 57-char window when writing umbrella skill descriptions
1 participant
What
Make a second review card for the same PR impossible to create, regardless of how the card is filed.
create_taskalready dedups on the idempotency key, but that misses one real case: a manually-filed review card for a PR the webhook also auto-files carries a different (or absent) key, so the key check passes and a duplicate review card lands. The phantom card looks stuck in the pipeline and its bogus gate blocks the downstream merge card.How
Add a guard in
create_taskkeyed on the PR identity itself, independent of the idempotency key and independent of the creation path (webhook retry OR hand-built chain):skillsincludegithub-code-review(newREVIEW_SKILLconstant) and it names a GitHub PR URL (title first — where the webhook puts it — then body)._canonical_pr_url): host casing /www.normalised, trailing path / query / fragment stripped, so…/pull/43,…/pull/43/files, and…/pull/43?w=1collapse to one identity."github-code-review") to avoid substring false-positives (e.g.github-code-review-extra); the in-Python URL compare is authoritative.Tests (TDD: RED → GREEN)
8 new tests in
test_kanban_core_functionality.py:dedup-without-key, pr-url-in-body-only, ignored-for-archived, only-for-review-skill, distinct-PRs-no-collide, without-PR-URL-not-deduped, canonicalises-URL-variants, substring-skill-not-matched.
Verified on a clean base (
70cf64cee) viascripts/run_tests.sh:tests/hermes_cli/test_kanban_core_functionality.py— 174/174 passed, zero regressions.Follow-up (NOT in this PR)
The companion documentation rule — "the webhook owns review-card creation; hand-built chains must not manually file a review card for a PR" — belongs in the
homestead-dispatch-golden-pathskill, which is on the forge single-copy path (cwest/hermes-skills-forge, active worktreetopic/skill-dedup-single-copy). Documented as a handoff rather than edited here to avoid creating a duplicate, diverged skill copy.