Skip to content

fix: preserve selected GitHub skills across resync - #2873

Draft
vincentkoc wants to merge 2 commits into
mainfrom
codex/fix-trending-warmup
Draft

fix: preserve selected GitHub skills across resync#2873
vincentkoc wants to merge 2 commits into
mainfrom
codex/fix-trending-warmup

Conversation

@vincentkoc

Copy link
Copy Markdown
Member

Summary

  • persist explicit GitHub skill selections on each source
  • preview multi-skill repos and require an explicit selection before syncing
  • preserve legacy all-skills sources and prevent stale selections from being replaced by unrelated skills
  • fix the search route test router mock after adding the Link action

Root cause

GitHub source setup previously synced every detected skill in a repository and stored no selection state. After deletion and re-add, discovery revived every candidate, including bundled skills, without asking the owner which skill to keep.

Validation

  • bun run ci:unit
  • bun run ci:static
  • bunx tsc --noEmit --incremental false --pretty false
  • focused GitHub sync/settings tests
  • autoreview clean

@vercel

vercel Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
clawhub Ready Ready Preview, Comment Jun 25, 2026 7:47am

Request Review

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jun 26, 2026
@clawsweeper

clawsweeper Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed August 5, 2026, 12:05 PM ET / 16:05 UTC.

ClawSweeper review

What this changes

The PR adds a settings dialog and persisted skill-folder selections so a GitHub-backed publisher source syncs only chosen skills.

Merge readiness

Blocked by patch quality or review findings - 10 items remain

Keep open for explicit member/maintainer handling: the selected-path feature is still absent from current main, but this conflicted branch must be rebuilt on the current identity-checked sync flow and supplied with real browser proof.

Priority: P2
Reviewed head: b03a08f546be828cc3ccfa319366092618ad53b7
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) PR readiness rating was derived from proof quality, review findings, security review, and reviewer confidence.
Proof confidence 🌊 off-meta tidepool Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🧂 unranked krab (1/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Evidence reviewed 6 items Current main lacks selected-path state: No current source, schema, or settings implementation references selectedSkillPaths or previewPublicGitHubSkillSource; the current sync path records repository identity and iterates the full snapshot.
Current source authorization contract: Current setup revalidates immutable GitHub repository and owner identifiers before applying a source snapshot; the PR’s older selection flow predates this contract.
Submitted selection implementation: The branch persists selected paths and filters discovered skills, but its configure handler is based on the pre-identity-validation source flow.
Findings 3 actionable findings [P1] Port selection into the identity-checked source flow
[P1] Remove the already-landed Trending fallback
[P3] Report selected paths in administrator dry runs
Security Needs attention Preserve immutable repository authorization: Current main verifies repository and owner identifiers before applying source updates, while the branch’s older configuration flow does not carry that contract; conflict resolution must not remove it.

How this fits together

ClawHub imports skill folders from a publisher’s public GitHub repository into its catalog. Publisher settings configure the source, then scheduled syncs validate repository identity and update the published skills.

flowchart LR
  A[Publisher settings] --> B[GitHub source preview]
  B --> C[Selected skill paths]
  C --> D[Authorized source record]
  D --> E[Scheduled repository sync]
  E --> F[Published skill catalog]
Loading

Decision needed

Question Recommendation
Should ClawHub add persistent publisher-selected skill paths to the current authorization-aware GitHub source workflow? Sponsor a narrow port: Retain the selection capability, but rebase it onto the current source authorization contract and require upgrade plus browser proof.

Why: The central capability is not on main, but it adds a durable publisher setting and must be reconciled with the newer identity and authorization boundary before implementation can proceed safely.

Before merge

  • Port selection into the identity-checked source flow (P1) - This selection handler predates current main’s immutable GitHub repository and owner validation. The branch is dirty; rebase the feature onto that flow rather than resolving source conflicts in favor of this older handler, or source authorization can regress. This was visible at the previously reviewed head and is raised late after the current-main authorization comparison.
  • Remove the already-landed Trending fallback (P1) - This fallback is already on current main from d639f67 and shipped in v0.23.1. Keeping a duplicate copy in this PR creates avoidable merge conflict and couples unrelated catalog behavior to the source-selection work.
  • Report selected paths in administrator dry runs (P3) - Once a source has an explicit selection, dry-run still returns the full snapshot count at this line. Operators will be told that unselected skills would sync, so calculate the count from the same selected subset used by the real sync.
  • Resolve security concern: Preserve immutable repository authorization - Current main verifies repository and owner identifiers before applying source updates, while the branch’s older configuration flow does not carry that contract; conflict resolution must not remove it.
  • Resolve merge risk (P1) - The branch is merge-conflicted and predates current immutable repository-identity checks; resolving it without a deliberate port could weaken source authorization.
  • Resolve merge risk (P1) - Persisted selections change which existing source skills become hidden on resync, so fresh-source, legacy-source, and selection-change behavior need upgrade coverage.
  • Complete next step (P2) - A maintainer must first choose whether persistent source selection belongs in the current authorization-aware workflow; the stale conflicted branch is not a safe automated repair target.
  • Improve patch quality - Resolve the security review concern or explain why the changed path is safe.
  • Improve patch quality - Address the highest-priority review finding and re-run the changed-surface validation.

Findings

  • [P1] Port selection into the identity-checked source flow — convex/githubSkillSync.ts:1004-1065
  • [P1] Remove the already-landed Trending fallback — convex/skills.ts:5887-5905
  • [P3] Report selected paths in administrator dry runs — convex/githubSkillSync.ts:1214-1222
  • [medium] Preserve immutable repository authorization — convex/githubSkillSync.ts:1004
Agent review details

Security

Needs attention: The diff introduces no new dependency or artifact execution, but its stale source-configuration path must preserve current repository-identity authorization before merge.

Review metrics

Metric Value Why it matters
Patch size +725/-7 across 11 files The branch combines the selection feature with an already-landed Trending fallback, increasing rebase and review scope.

Merge-risk options

Maintainer options:

  1. Port after confirming the contract (recommended)
    Rebuild the selection flow on current main, retain immutable repository authorization, and test legacy plus selected-source upgrades before merge.
  2. Pause the feature
    If persistent publisher selection is not desired, close this conflicted branch and keep current all-skills source behavior.

Technical review

Best possible solution:

Rebuild the selected-path capability narrowly on current main’s authorization-aware source model, preserving legacy all-skills sources and proving selection, resync, and administrator dry-run behavior.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: current main has no selected-path state and syncs the complete discovered snapshot, while the branch introduces the missing selection boundary. A real deployment reproduction was not provided.

Is this the best way to solve the issue?

No; the branch is not the best landing shape because it predates the current identity-checked source architecture and includes an already-released unrelated fallback.

Full review comments:

  • [P1] Port selection into the identity-checked source flow — convex/githubSkillSync.ts:1004-1065
    This selection handler predates current main’s immutable GitHub repository and owner validation. The branch is dirty; rebase the feature onto that flow rather than resolving source conflicts in favor of this older handler, or source authorization can regress. This was visible at the previously reviewed head and is raised late after the current-main authorization comparison.
    Confidence: 0.95
    Late finding: first raised on code an earlier review cycle already covered.
  • [P1] Remove the already-landed Trending fallback — convex/skills.ts:5887-5905
    This fallback is already on current main from d639f67 and shipped in v0.23.1. Keeping a duplicate copy in this PR creates avoidable merge conflict and couples unrelated catalog behavior to the source-selection work.
    Confidence: 0.99
  • [P3] Report selected paths in administrator dry runs — convex/githubSkillSync.ts:1214-1222
    Once a source has an explicit selection, dry-run still returns the full snapshot count at this line. Operators will be told that unselected skills would sync, so calculate the count from the same selected subset used by the real sync.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against f9ea25e14f14.

Labels

Label justifications:

  • P2: This is a bounded publisher catalog-management feature with no demonstrated urgent production outage.
  • merge-risk: 🚨 compatibility: Persisted source selections can hide previously synced skills unless legacy and upgrade behavior is preserved.
  • merge-risk: 🚨 security-boundary: The conflicted branch touches source configuration that current main protects with immutable repository identity checks.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.

Evidence

Security concerns:

  • [medium] Preserve immutable repository authorization — convex/githubSkillSync.ts:1004
    Current main verifies repository and owner identifiers before applying source updates, while the branch’s older configuration flow does not carry that contract; conflict resolution must not remove it.
    Confidence: 0.95

What I checked:

  • Current main lacks selected-path state: No current source, schema, or settings implementation references selectedSkillPaths or previewPublicGitHubSkillSource; the current sync path records repository identity and iterates the full snapshot. (convex/githubSkillSync.ts:1044, f9ea25e14f14)
  • Current source authorization contract: Current setup revalidates immutable GitHub repository and owner identifiers before applying a source snapshot; the PR’s older selection flow predates this contract. (convex/githubSkillSync.ts:2571, f9ea25e14f14)
  • Submitted selection implementation: The branch persists selected paths and filters discovered skills, but its configure handler is based on the pre-identity-validation source flow. (convex/githubSkillSync.ts:1004, b03a08f546be)
  • Duplicate Trending fallback is already released: The branch includes the bounded Trending warm-up already implemented on main by d639f67 and shipped in v0.23.1. (convex/skills.ts:6032, d639f6720b85)
  • Dry-run mismatch: After adding selected paths, the administrator dry-run still reports snapshot.skills.length rather than the selected subset. (convex/githubSkillSync.ts:1222, b03a08f546be)
  • Real behavior proof is absent: The PR body lists CI and focused tests, but the supplied discussion contains no after-fix browser capture, runtime trace, or redacted live output for the new settings selection flow.

Likely related people:

  • Patrick Erichsen: Introduced the current repository-identity and authorization-aware GitHub source implementation that a selection feature must preserve. (role: current source-sync refactor author; confidence: high; commits: 87ca030c30f3; files: convex/githubSkillSync.ts, convex/githubSkillSources.ts)
  • vincentkoc: Authored this branch and the independently landed Trending fallback on current main. (role: recent adjacent contributor; confidence: high; commits: d639f6720b85, b03a08f546be; files: convex/skills.ts, convex/githubSkillSync.ts, src/routes/settings.tsx)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (16 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-05T05:29:01.751Z sha b03a08f :: found issues before merge. :: [P2] Capture the selection dialog in a real browser | [P3] Honor selected paths in admin dry-run counts
  • reviewed 2026-08-05T07:22:00.947Z sha b03a08f :: found issues before merge. :: [P1] Remove the already-landed Trending fallback | [P2] Show the selection flow in a real browser | [P3] Filter administrator dry-run counts by selected paths
  • reviewed 2026-08-05T07:28:54.956Z sha b03a08f :: found issues before merge. :: [P1] Remove the already-landed Trending fallback | [P2] Capture the selection dialog in a real browser | [P3] Honor selected paths in admin dry-run counts
  • reviewed 2026-08-05T08:37:28.095Z sha b03a08f :: found issues before merge. :: [P1] Remove the already-landed Trending fallback | [P2] Add real-browser proof for the selection dialog | [P3] Filter administrator dry-run discovery counts
  • reviewed 2026-08-05T09:42:43.629Z sha b03a08f :: found issues before merge. :: [P1] Remove the already-landed Trending fallback | [P2] Show the selection flow in a real browser | [P3] Filter administrator dry-run discovery counts
  • reviewed 2026-08-05T11:11:29.365Z sha b03a08f :: found issues before merge. :: [P1] Remove the already-landed Trending fallback | [P2] Show the selection dialog in a real browser | [P3] Report selected paths in administrator dry runs
  • reviewed 2026-08-05T12:31:28.080Z sha b03a08f :: found issues before merge. :: [P1] Remove the already-landed Trending fallback | [P2] Show the selection dialog in a real browser | [P3] Make administrator dry-runs honor selected paths
  • reviewed 2026-08-05T14:04:48.809Z sha b03a08f :: found issues before merge. :: [P1] Remove the already-landed Trending fallback | [P2] Show the selection flow in a real browser | [P3] Make administrator dry-runs honor selected paths

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale due to inactivity.
Please update it or it will be closed.

@github-actions github-actions Bot added the stale label Jul 31, 2026
@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 5, 2026
@github-actions github-actions Bot removed the stale label Aug 5, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 5, 2026
@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 5, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale due to inactivity.
Please update it or it will be closed.

@github-actions github-actions Bot added the stale label Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. stale status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant