Jminnetian/askskills#1410
Open
jsourcebot wants to merge 38 commits into
Open
Conversation
Contributor
|
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:
WalkthroughThis PR adds Ask Sourcebot skills end to end: new persistence and server actions, command catalog and auto-invocation plumbing, chat composer/thread support for command mentions, skill editor and management UIs, source-sync status APIs, and related navigation, guard, and error-handling updates. ChangesAsk Sourcebot Skills feature
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant AgentStream as createAgentStream
participant LoadSkillTool as load_skill tool
participant Registry as resolveAutoInvocableSkill
participant Prisma
AgentStream->>AgentStream: buildSkillRegistry(userId, orgId)
AgentStream->>AgentStream: createPrompt(skillRegistry)
AgentStream->>LoadSkillTool: activeTools include load_skill
LoadSkillTool->>Registry: resolveAutoInvocableSkill(skillId)
Registry->>Prisma: agentSkill.findFirst
Prisma-->>Registry: skill row or null
Registry-->>LoadSkillTool: resolved skill or null
sequenceDiagram
participant ChatRoute as ee/chat route
participant Materializer as materializeCommandMessageTexts
participant Prisma
participant Stream as createMessageStream
ChatRoute->>Materializer: messages, prisma, userId, orgId, requestSource
Materializer->>Prisma: agentSkill.findMany
Prisma-->>Materializer: matching skill instructions
Materializer->>Materializer: inject expandedText and data-source parts
Materializer-->>ChatRoute: materialized messages
ChatRoute->>Stream: createMessageStream(materialized messages)
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
License Audit
Weak Copyleft Packages (informational)
Resolved Packages (17)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/web/src/app/(app)/settings/layout.tsx (1)
86-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNew "Skills" nav item is unreachable for exactly the users it's meant to reach.
The new "Skills" entry (and the pre-existing "Ask Sourcebot" entry) is only added to the Account nav group when
hasAskEntitlementis true — non-entitled orgs get no entry at all. ButSkillsEntitlementMessage(rendered by/settings/skillswhen!hasEntitlement('ask')) is explicitly built as "the upsell surface for free-plan users" so it can render for free-plan users. Since the sidebar never surfaces/settings/skillsfor such users, and the deep-link source (workspace admin table) is likewise gated behindisAskAgentAvailable, this upsell page is only reachable by manually typing the URL — defeating its purpose.Compare with the Workspace group's "Ask Sourcebot" item (line 133-137 below), which stays visible with
requiredEntitlement: 'ask'so the Nav component can show an upgrade badge instead of hiding the link entirely. Consider applying the same pattern to "Skills" so free-plan users can actually discover and reach the upsell.♻️ Proposed fix
+ { + title: "Skills", + href: `/settings/skills`, + hrefRegex: `/settings/skills(/.*)?$`, + icon: "sparkles" as const, + requiredEntitlement: 'ask', + }, ...(hasAskEntitlement ? [ { title: "Ask Sourcebot", href: `/settings/accountAskAgent`, hrefRegex: `/settings/accountAskAgent(/.*)?$`, icon: "bot" as const, }, - { - title: "Skills", - href: `/settings/skills`, - hrefRegex: `/settings/skills(/.*)?$`, - icon: "sparkles" as const, - } ] : []),🤖 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 `@packages/web/src/app/`(app)/settings/layout.tsx around lines 86 - 99, The “Skills” nav entry in the settings sidebar is gated behind hasAskEntitlement, which hides the upsell page from the free-plan users it is meant for. Update the Account nav items in the layout component so “Skills” is always present, similar to the Workspace “Ask Sourcebot” item, and use requiredEntitlement: 'ask' or equivalent entitlement metadata instead of conditionally removing it. Keep the existing route symbols like settings/accountAskAgent, settings/skills, and the nav item definitions in layout.tsx aligned so Nav can render the link with an upgrade state rather than hiding it entirely.packages/web/src/app/api/(server)/ee/chat/route.ts (1)
93-105: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPer-turn budget check should run after command materialization. The current guard only measures the raw user text, so a short
/skillcan expand into much larger instructions and bypassATTACHMENT_MAX_TURN_TEXT_BYTESbefore the message is sent and persisted. MovematerializeCommandMessageTextsahead of the size check, or measure the materialized message text instead.🤖 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 `@packages/web/src/app/api/`(server)/ee/chat/route.ts around lines 93 - 105, The per-turn budget guard in the chat route is checking the raw user text too early, so command expansion can bypass the limit. In the route handler around latestMessage handling, move materializeCommandMessageTexts before the ATTACHMENT_MAX_TURN_TEXT_BYTES check, or change the size check to use the materialized message text instead of the original input. Keep the enforcement in the same request path so the persisted message and prompt both reflect the final expanded content.
🧹 Nitpick comments (18)
packages/web/src/features/git/getFileSourceApi.ts (2)
121-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate repo/path/ref validation logic with
getFileSourceForRepo.The repo lookup,
isPathValid/isGitRefValidchecks, and git-instance/gitRefsetup (lines 121-143) are copy-pasted fromgetFileSourceForRepo(lines 30-48). Consider extracting a shared helper (e.g.resolveRepoAndGitRef) returning either the validated{ repo, git, gitRef }or the appropriateServiceError, used by both functions, to avoid future divergence.🤖 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 `@packages/web/src/features/git/getFileSourceApi.ts` around lines 121 - 143, The repo lookup, path/ref validation, and git setup logic in resolveFileBlobShaForRepo is duplicated from getFileSourceForRepo, so extract the shared repo resolution flow into a helper such as resolveRepoAndGitRef that performs the prisma.repo lookup, isPathValid/isGitRefValid checks, and builds the simpleGit().cwd(repoPath) plus gitRef value. Update both getFileSourceForRepo and resolveFileBlobShaForRepo to call the shared helper and return or propagate the same ServiceError results to keep the two paths consistent.
64-73: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCompute
blobShafromfileContentinstead of a secondrev-parsecall. Since this endpoint already returns text content as a string, hashing the fetched body is sufficient here; it also removes an extra git subprocess on a broad path and avoids a mismatch ifgitRefmoves between reads.🤖 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 `@packages/web/src/features/git/getFileSourceApi.ts` around lines 64 - 73, The blobSha calculation in getFileSourceApi should be derived from the already-fetched fileContent instead of calling git.raw with rev-parse again. Update the blobSha assignment logic in the getFileSourceApi path so it hashes the returned text body locally, preserves the current best-effort/undefined-on-failure behavior, and removes the extra git subprocess while keeping the existing filePath/gitRef-based lookup flow intact.packages/web/src/features/chat/components/chatBox/suggestionsBox.tsx (1)
133-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collapsing the two near-identical
titleDetailbranches.The
titleDetail-present andtitleDetail-absent branches only differ by one extra<span>; the outer wrapper, title span, and badge logic are duplicated. Could be simplified to a single block that conditionally renders thetitleDetailspan inline.♻️ Proposed simplification
- {titleDetail ? ( - <div className="flex flex-row items-baseline gap-2 min-w-0"> - <span className="text-sm font-medium truncate"> - {title} - </span> - <span className="text-sm text-muted-foreground truncate"> - {titleDetail} - </span> - {badge && ( - <SourceLabelBadge className="ml-auto">{badge}</SourceLabelBadge> - )} - </div> - ) : ( - <div className="flex flex-row items-baseline gap-2 min-w-0"> - <span className="text-sm font-medium truncate"> - {title} - </span> - {badge && ( - <SourceLabelBadge className="ml-auto">{badge}</SourceLabelBadge> - )} - </div> - )} + <div className="flex flex-row items-baseline gap-2 min-w-0"> + <span className="text-sm font-medium truncate"> + {title} + </span> + {titleDetail && ( + <span className="text-sm text-muted-foreground truncate"> + {titleDetail} + </span> + )} + {badge && ( + <SourceLabelBadge className="ml-auto">{badge}</SourceLabelBadge> + )} + </div>🤖 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 `@packages/web/src/features/chat/components/chatBox/suggestionsBox.tsx` around lines 133 - 172, The SuggestionListItem component duplicates nearly the same JSX in the titleDetail and non-titleDetail branches. Refactor the SuggestionListItem return so the shared wrapper, title span, and badge rendering live in one block, and render the titleDetail span conditionally inline only when present. Use the SuggestionListItem and titleDetail symbols to locate the duplicated branch logic.packages/web/src/ee/features/chat/useUnsavedChangesGuard.test.tsx (1)
38-128: 📐 Maintainability & Code Quality | 🔵 TrivialMissing coverage for
bypass()and the one-shotresolve()latch.Good coverage of the
beforeunloadand controlled/confirm push paths, but there's no test exercisingbypass()(used for save-then-redirect flows) or thedecisionMadeRefone-shot guard inresolve(). Givenbypass()has a state-reset bug (seeuseUnsavedChangesGuard.tscomment), a test here would have caught it.🤖 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 `@packages/web/src/ee/features/chat/useUnsavedChangesGuard.test.tsx` around lines 38 - 128, Add test coverage in useUnsavedChangesGuard test suite for the bypass() path and the one-shot resolve() behavior. Create a case that triggers the guard, calls bypass(), and verifies the next navigation is allowed without re-prompting or leaving stale active state, then add a case that exercises resolve() twice to confirm decisionMadeRef only lets the first decision take effect. Use the existing useUnsavedChangesGuard hook and Probe/Noop patterns to locate and validate these flows.packages/db/prisma/migrations/20260624202848_refactor_agent_skills/migration.sql (1)
44-53: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant index alongside unique constraint.
AgentSkill_orgId_visibility_scopeId_idx(orgId, visibility, scopeId) is a strict column-prefix of the unique indexAgentSkill_orgId_visibility_scopeId_slug_key(orgId, visibility, scopeId, slug). Postgres can already satisfy prefix-column lookups using the unique btree index, so the plain index adds write/storage overhead without a query benefit.♻️ Proposed fix
--- CreateIndex -CREATE INDEX "AgentSkill_orgId_visibility_scopeId_idx" ON "AgentSkill"("orgId", "visibility", "scopeId"); - -- CreateIndex CREATE INDEX "AgentSkill_orgId_visibility_scopeId_idx" ON "AgentSkill"("orgId", "visibility", "scopeId", "enabled", "featured" DESC, "updatedAt" DESC, "name");Alternatively, simply drop the standalone index in a follow-up migration and rely on
AgentSkill_orgId_visibility_scopeId_slug_keyfor prefix lookups.🤖 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 `@packages/db/prisma/migrations/20260624202848_refactor_agent_skills/migration.sql` around lines 44 - 53, Remove the redundant AgentSkill_orgId_visibility_scopeId_idx index from the migration because AgentSkill_orgId_visibility_scopeId_slug_key already covers the same orgId, visibility, and scopeId prefix. Update the migration.sql block that creates AgentSkill indexes so only the unique index remains for prefix lookups, and keep the existing unique constraint creation intact.packages/web/src/ee/features/chat/skills/actions.ts (4)
358-360: 📐 Maintainability & Code Quality | 🔵 TrivialInconsistent ServiceError narrowing idiom.
These sites narrow
requireManageableSharedSkill's result with"errorCode" in x, while the rest of the file (e.g. lines 516, 547, 643) uses theisServiceError()helper already imported at the top. UsingisServiceError()consistently avoids having two different type-narrowing idioms for the same union type.♻️ Example
- if ("errorCode" in manageableSkill) { + if (isServiceError(manageableSkill)) { return manageableSkill; }Also applies to: 682-684, 1054-1056, 1110-1112
🤖 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 `@packages/web/src/ee/features/chat/skills/actions.ts` around lines 358 - 360, Replace the ad hoc ServiceError check in requireManageableSharedSkill call sites with the existing isServiceError() helper for consistent narrowing. Update the branches in actions.ts that currently use "errorCode" in manageableSkill to use isServiceError(manageableSkill) instead, matching the patterns already used elsewhere in the file. Make the same consistency change in the other listed call sites so all ServiceError unions are narrowed through the same helper.
63-67: 📐 Maintainability & Code Quality | 🔵 Trivial
skillSourceInvalidandskillNotSyncedshare the sameerrorCode.Both errors use
ErrorCode.INVALID_REQUEST_BODYdespite representing distinct conditions (missing source link vs. now-invalid source content) and different HTTP status codes (400 vs 422). Clients branching onerrorCodealone can't distinguish them. Since two new codes (AGENT_SKILL_ALREADY_EXISTS,AGENT_SKILL_NOT_FOUND) were already added for this feature, consider adding dedicated codes here too.🤖 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 `@packages/web/src/ee/features/chat/skills/actions.ts` around lines 63 - 67, The skill error helpers currently reuse ErrorCode.INVALID_REQUEST_BODY for two different failure cases, so update the chat skills error definitions to use distinct error codes for skillNotSynced and skillSourceInvalid. Add dedicated codes alongside the existing feature-specific codes, then wire each helper in actions.ts to the appropriate new code while keeping their current status codes and messages. Reference the skillNotSynced and skillSourceInvalid functions so the separation is clear and clients can branch on errorCode reliably.
340-381: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate fetch pattern for manageable shared skills.
getSharedAgentSkillandupdateSharedAgentSkillFromSourceboth callrequireManageableSharedSkill(which already does afindFirstwithenabled: true) and then immediately re-fetch the same row with a secondfindFirstto pull additional columns.updateSharedAgentSkillavoids this by using the fields already returned fromrequireManageableSharedSkill. Consider making the select inrequireManageableSharedSkillgeneric/parameterizable so callers can request the extra columns they need in one round trip.Also applies to: 664-724
🤖 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 `@packages/web/src/ee/features/chat/skills/actions.ts` around lines 340 - 381, In getSharedAgentSkill, avoid the extra prisma.agentSkill.findFirst after requireManageableSharedSkill by making requireManageableSharedSkill return the needed fields up front, similar to updateSharedAgentSkill. Refactor requireManageableSharedSkill to accept a caller-provided select/include (or equivalent generic selection) so getSharedAgentSkill can request the columns needed by canAccessSkillSource and toAgentSkillListItem in one query. Apply the same one-round-trip pattern to updateSharedAgentSkillFromSource as well.
876-909: 🗄️ Data Integrity & Integration | 🔵 TrivialUn-sharing a skill silently drops it for all other adopters.
When the creator calls
makeSharedAgentSkillPersonal, the sharedAgentSkillrow is deleted (line 894-898). Per the migration,AgentSkillAdoptionrows cascade-delete onAgentSkillremoval, so every other user who had adopted or auto-enrolled this shared skill loses it immediately with no warning. If this is intended (converting a shared skill to personal necessarily un-shares it for everyone), consider surfacing adopter counts in the UI so the creator can make an informed decision before converting.🤖 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 `@packages/web/src/ee/features/chat/skills/actions.ts` around lines 876 - 909, The personal-conversion flow in makeSharedAgentSkillPersonal is deleting the shared AgentSkill record when sharedSkill.createdById matches user.id, which cascades removal for every AgentSkillAdoption. Update this branch so converting a creator-owned shared skill to personal does not silently unshare it for all adopters, or otherwise make the behavior explicit before deletion. Use the prisma.$transaction block, tx.agentSkill.delete, and removeSharedSkillForUser as the key spots to adjust, and keep the creator’s personal copy creation separate from any global unshare behavior.packages/web/src/ee/features/chat/skills/commands.server.ts (1)
5-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
console.errorinstead of the project's structured logger.These three functions log failures with
console.error, while the surrounding chat/skills tests mockcreateLoggerfrom@sourcebot/shared, suggesting other modules in this feature use the structured logger. Usingconsole.errorhere bypasses whatever log level/formatting/observability pipeline the structured logger provides.#!/bin/bash # Check whether sibling skills files use createLogger instead of console.error rg -n 'createLogger|console\.error' packages/web/src/ee/features/chat/skills🤖 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 `@packages/web/src/ee/features/chat/skills/commands.server.ts` around lines 5 - 33, The three command-loading helpers currently use console.error, which bypasses the feature’s structured logging path. Update listPersonalAgentSkillCommandsOrEmpty, listSharedSkillCommandsOrEmpty, and listAgentSkillCommandsOrEmpty to use the shared logger from `@sourcebot/shared` via createLogger instead of direct console calls, keeping the same failure context and return-[] fallback behavior.packages/db/prisma/schema.prisma (1)
807-812: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant index:
@@index([orgId, visibility, scopeId])is a prefix of the unique constraint.
@@unique([orgId, visibility, scopeId, slug])(Line 807) already supports lookups filtered by(orgId, visibility, scopeId)as a leftmost-prefix scan, making the separate@@index([orgId, visibility, scopeId])(Line 811) largely redundant and adding write overhead with no read benefit.🤖 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 `@packages/db/prisma/schema.prisma` around lines 807 - 812, Remove the redundant `@@index([orgId, visibility, scopeId])` from the Prisma model since `@@unique([orgId, visibility, scopeId, slug])` in the same schema already covers that prefix for lookups. Keep the other indexes intact, and verify the remaining `AgentSkill` indexing still supports the intended query patterns without duplicating write cost.packages/web/src/ee/features/chat/skills/components/personalSkillEditorPage.tsx (1)
183-211: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNon-null assertions on
skill!.idfor edit modes.
saveModeguaranteesskill !== nullwheneditShared/editPersonalexecute, but TypeScript can't infer that through the map-lookup pattern, hence the!assertions. Functionally safe given the currentsaveModederivation, but fragile to future refactors ofsaveModeConfig/saveMode.🤖 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 `@packages/web/src/ee/features/chat/skills/components/personalSkillEditorPage.tsx` around lines 183 - 211, The save handler in handleSubmit uses non-null assertions on skill.id for editShared and editPersonal, which is fragile even though saveMode currently implies a non-null skill. Refactor the saveMode dispatch in personalSkillEditorPage so the edit paths are only constructed or called after an explicit skill null check, or otherwise narrow skill before accessing its id; keep the createPersonalAgentSkill/createSharedAgentSkill branches unchanged and avoid relying on skill! inside the map lookup.packages/web/src/ee/features/chat/skills/components/skillsPage.tsx (1)
903-903: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCurly-brace bodies should be on their own line.
Both
onOpenChangehandlers place theifbody on the same line, unlike the rest of the file which consistently formats multi-line.As per coding guidelines: "Always use curly braces for
ifstatements, with the body on a new line — even for single-line bodies."🎨 Suggested formatting
- onOpenChange={(open) => { if (!open) { setPendingDiscard(null); } }} + onOpenChange={(open) => { + if (!open) { + setPendingDiscard(null); + } + }}- onOpenChange={(open) => { if (!open) { navGuard.resolve(false); } }} + onOpenChange={(open) => { + if (!open) { + navGuard.resolve(false); + } + }}Also applies to: 930-930
🤖 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 `@packages/web/src/ee/features/chat/skills/components/skillsPage.tsx` at line 903, The onOpenChange handlers in skillsPage.tsx use a one-line if body inside the arrow function, which breaks the file’s brace-and-newline formatting convention. Update both onOpenChange callbacks so the if statement keeps curly braces and places the body on its own line, matching the surrounding style in this component and the related handler near the other reported location.Source: Coding guidelines
packages/web/src/ee/features/chat/skills/components/skillsPage.test.tsx (1)
159-187: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for the owner-(non-creator) "make personal" path.
All make-personal tests use
sharedSkill/syncedSkill, which are treated as user-created (isCreatedByUsernot overridden tofalsefor an owner acting on someone else's skill). This gap would have caught the stale-state issue flagged inskillsPage.tsx'shandleMakePersonal.🤖 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 `@packages/web/src/ee/features/chat/skills/components/skillsPage.test.tsx` around lines 159 - 187, Add test coverage for the owner-but-not-creator make-personal flow in skillsPage.test.tsx, since the current make-personal cases only use sharedSkill/syncedSkill and never exercise the non-user-created path. Update the relevant test setup around renderSkillsPage and handleMakePersonal so the selected skill has isCreatedByUser set to false while still belonging to the owner, then verify the confirmation dialog and make-personal action behave correctly. Use the existing makeSharedAgentSkillPersonal and skillsPage.tsx handleMakePersonal flow as the anchors for locating the path.packages/web/src/ee/features/chat/skills/components/importFromRepoDialog.tsx (2)
143-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCurly-brace body should be on its own line.
The inline
if (!isOpen) { resetAndClose(); }violates the project's if-statement formatting rule.As per coding guidelines: "Always use curly braces for
ifstatements, with the body on a new line — even for single-line bodies."🎨 Suggested formatting
- <Dialog open={open} onOpenChange={(isOpen) => { if (!isOpen) { resetAndClose(); } }} modal={true}> + <Dialog + open={open} + onOpenChange={(isOpen) => { + if (!isOpen) { + resetAndClose(); + } + }} + modal={true} + >🤖 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 `@packages/web/src/ee/features/chat/skills/components/importFromRepoDialog.tsx` at line 143, The inline if statement inside the Dialog onOpenChange handler in importFromRepoDialog should follow the project’s brace formatting rule. Update the onOpenChange callback so the if (!isOpen) block uses curly braces with the body on its own line, matching the style used elsewhere in the component and keeping the resetAndClose call inside that properly formatted block.Source: Coding guidelines
48-62: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRepo search fires a request on every keystroke.
repoQuerydrives theuseQuerykey directly with no debounce, so each keystroke issues a newlistReposcall (confirmed by the accompanying test asserting multiple calls for"wid"/"widg"). Debouncing the input would meaningfully cut backend load with minimal code.⚡ Suggested debounce
- const [repoQuery, setRepoQuery] = useState(""); + const [repoQuery, setRepoQuery] = useState(""); + const [debouncedRepoQuery, setDebouncedRepoQuery] = useState(""); + + useEffect(() => { + const id = setTimeout(() => setDebouncedRepoQuery(repoQuery), 250); + return () => clearTimeout(id); + }, [repoQuery]); ... - queryKey: ["skillImportRepos", repoQuery], + queryKey: ["skillImportRepos", debouncedRepoQuery], queryFn: () => unwrapServiceError(listRepos({ page: 1, perPage: MAX_RESULTS, sort: "name", direction: "asc", - query: repoQuery, + query: debouncedRepoQuery, })),🤖 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 `@packages/web/src/ee/features/chat/skills/components/importFromRepoDialog.tsx` around lines 48 - 62, The repo search in importFromRepoDialog’s useQuery is firing listRepos on every keystroke because repoQuery is used directly in the query key and queryFn. Add a debounce layer for the search input so the query only updates after typing pauses, while keeping the existing selectedRepo/open gating and keepPreviousData behavior intact. Update the repoQuery flow in importFromRepoDialog and any related input state so the debounced value is what drives the query.packages/web/src/ee/features/chat/skills/components/workspaceSkillMutations.ts (1)
24-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
flagparam is accepted but not used to build the update payload.
data: { autoEnrolled: checked }is hardcoded, ignoringflag. Works today sinceOrgSkillFlagKeyhas a single member, but the signature implies genericity that the body doesn't honor — a future second flag key would silently write to the wrong field.♻️ Proposed fix
export async function updateWorkspaceSkillFlag({ skillId, + flag, checked, updateOrgSkills, }: { skillId: string; flag: OrgSkillFlagKey; checked: boolean; updateOrgSkills: (updater: (skills: SharedAgentSkillManagementItem[]) => SharedAgentSkillManagementItem[]) => void; }): Promise<ServiceError | null> { const result = await setSharedSkillFlag({ skillId, - data: { autoEnrolled: checked }, + data: { [flag]: checked } as Record<OrgSkillFlagKey, boolean>, });🤖 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 `@packages/web/src/ee/features/chat/skills/components/workspaceSkillMutations.ts` around lines 24 - 46, The updateWorkspaceSkillFlag function accepts a flag parameter but always sends data.autoEnrolled, so the payload is not actually driven by the selected flag. Update the setSharedSkillFlag call to build its data from the flag argument inside updateWorkspaceSkillFlag, so the function stays aligned with OrgSkillFlagKey and will update the correct field if more flag keys are added later.packages/web/src/ee/features/chat/agent.ts (1)
605-659: 🚀 Performance & Scalability | 🔵 TrivialSkill catalog gating and load_skill wiring look correct.
Entitlement/identity gating mirrors the existing MCP-tool pattern, and
alwaysActiveToolsis defined once so both the initialactiveToolsand everyprepareSteprebuild stay in sync automatically. Reasonable defense-in-depth against the registry snapshot going stale (re-resolved inresolveAutoInvocableSkill).One minor unbounded-growth consideration worth keeping in mind operationally:
buildSkillRegistryloads every visible personal+shared skill on every request without any cap. As orgs accumulate many skills, this could inflate the dynamic prompt size and per-message tool-call description tokens. Not a defect introduced here (it mirrors the manual command catalog), but worth monitoring/pagination if catalog sizes grow.🤖 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 `@packages/web/src/ee/features/chat/agent.ts` around lines 605 - 659, The skill catalog can grow without bound because buildSkillRegistry loads every visible personal/shared skill on each request, which can bloat the dynamic prompt and tool descriptions. Update the ask-agent flow around buildSkillRegistry/createPrompt to either cap the catalog size or add pagination/selection for large orgs, and keep the load_skill wiring and alwaysActiveTools behavior unchanged.
🤖 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 `@CHANGELOG.md`:
- Line 23: The Unreleased changelog item for the Ask Sourcebot skills entry is
missing the trailing GitHub PR link, unlike the other sibling entries. Update
the `CHANGELOG.md` Unreleased entry so the line ends with the pull request
reference in the required `[#<id>](<url>)` format, matching the existing pattern
used by the nearby changelog items.
In `@packages/db/prisma/schema.prisma`:
- Around line 773-780: The AgentSkill audit relation is too destructive:
`createdBy` currently cascades on user deletion, which can delete shared/org
skills and their adopters. Update the `AgentSkill` model’s
`createdBy`/`createdById` relation in the Prisma schema to stop using cascade
delete and allow the creator reference to be cleared instead, matching the
intent of audit metadata. Then update any `createdById` callers and mappings
that assume a required user, including `toSharedAgentSkillCatalogItem` and any
`Pick<AgentSkill, ... "createdById" ...>` usages, so they handle a nullable
creator.
In `@packages/web/src/components/ui/tooltip.tsx`:
- Around line 18-28: The TooltipContent in tooltip.tsx is being portaled, which
moves the nested Docs link outside SelectContent’s dismissable tree and can
close the select before navigation. Update the TooltipPrimitive.Content usage in
the tooltip component so this tooltip renders inline for the Select/docs case,
or gate the Portal wrapper behind a prop/branch that keeps the click target
inside the SelectContent subtree; use TooltipPrimitive.Content and
TooltipPrimitive.Portal as the key symbols to adjust.
In `@packages/web/src/ee/features/chat/agent.ts`:
- Around line 181-187: The sanitizeSkillCatalogText helper in agent.ts only
collapses whitespace, so markup-like values can still break the <agent_skills>
block. Update this helper (and the call sites that build the catalog line) to
escape or encode prompt-sensitive characters such as <, >, and & before
interpolation, or switch the skill catalog serialization to a safe non-markup
format so names, hints, and descriptions cannot be reinterpreted as tags.
In `@packages/web/src/ee/features/chat/skills/actions.ts`:
- Around line 579-609: The shared-skill lookup in getAgentSkillSourceStatus is
too permissive because it uses sharedAgentSkillAuthScope(org.id) with enabled:
true instead of the same visibility checks used by getSharedAgentSkill and
adoptSharedSkill. Update the Prisma findFirst filter to scope shared skills
through canAccessSkillSource or sharedAgentSkillVisibleToUserWhere so only
skills visible to the caller can have their sync status queried, while keeping
the personalAgentSkillAuthScope path unchanged.
In `@packages/web/src/ee/features/chat/skills/commandResolution.ts`:
- Around line 292-304: The manual skill invocation event path in
commandResolution.ts is not preserving the request source, so all events fall
back to the default attribution. Thread the source value from route.ts through
materializeCommandMessageTexts into buildAskSkillInvokedEvent, and use that
value when emitting ask_skill_invoked for the manual invocation branch so
web/MCP/API-originated calls are attributed correctly.
In
`@packages/web/src/ee/features/chat/skills/components/personalSkillEditorPage.tsx`:
- Around line 82-113: `SkillEditor` still allows editing description and
instructions for synced skills even though those fields are ignored on save.
Update the `personalSkillEditorPage.tsx` flow in `SkillEditor` to detect synced
skills from `skill.source` (or equivalent existing skill metadata) and make the
description/instructions inputs read-only/disabled, or replace them with a
synced-from-repo notice. Keep the existing create/edit logic intact, but ensure
the UI reflects that only supported fields are editable for synced skills.
In `@packages/web/src/ee/features/chat/skills/components/skillsPage.tsx`:
- Around line 588-617: The shared-skill state update in handleMakePersonal
leaves a stale entry behind for the non-creator owner path. Update the
make-personal flow so that, after a successful makeSharedAgentSkillPersonal
call, the skill is removed from sharedSkills in both branches rather than only
toggling flags; use the existing handleMakePersonal logic and the
sharedSkills/setSharedSkills updates to locate it. Keep the personalSkills
update and selection/toast behavior unchanged, but ensure the shared catalog no
longer renders the converted skill after the destructive operation.
In
`@packages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.tsx`:
- Around line 228-238: The Switch in workspaceSharedSkillsManager currently uses
the same static aria-label for every row, so each auto-enroll toggle is
indistinguishable to screen readers. Update the Switch’s aria-label in the skill
row render to include the current skill name, following the same pattern used by
the delete button, and keep the change localized around the `Switch` inside the
workspaceSharedSkillsManager component.
In `@packages/web/src/ee/features/chat/useUnsavedChangesGuard.ts`:
- Around line 22-26: The bypass in useUnsavedChangesGuard is effectively
permanent because bypassRef.current is set true in bypass() and never cleared,
which disables the enabled() guard for all future unsaved changes. Update
useUnsavedChangesGuard so bypass() only suppresses the next navigation attempt,
then resets bypassRef.current back to false as part of the guard flow (for
example, when enabled() observes and consumes the bypass flag). Make sure the
logic around enabled, bypassRef, and bypass preserves normal guarding after the
deliberate programmatic navigation.
---
Outside diff comments:
In `@packages/web/src/app/`(app)/settings/layout.tsx:
- Around line 86-99: The “Skills” nav entry in the settings sidebar is gated
behind hasAskEntitlement, which hides the upsell page from the free-plan users
it is meant for. Update the Account nav items in the layout component so
“Skills” is always present, similar to the Workspace “Ask Sourcebot” item, and
use requiredEntitlement: 'ask' or equivalent entitlement metadata instead of
conditionally removing it. Keep the existing route symbols like
settings/accountAskAgent, settings/skills, and the nav item definitions in
layout.tsx aligned so Nav can render the link with an upgrade state rather than
hiding it entirely.
In `@packages/web/src/app/api/`(server)/ee/chat/route.ts:
- Around line 93-105: The per-turn budget guard in the chat route is checking
the raw user text too early, so command expansion can bypass the limit. In the
route handler around latestMessage handling, move materializeCommandMessageTexts
before the ATTACHMENT_MAX_TURN_TEXT_BYTES check, or change the size check to use
the materialized message text instead of the original input. Keep the
enforcement in the same request path so the persisted message and prompt both
reflect the final expanded content.
---
Nitpick comments:
In
`@packages/db/prisma/migrations/20260624202848_refactor_agent_skills/migration.sql`:
- Around line 44-53: Remove the redundant
AgentSkill_orgId_visibility_scopeId_idx index from the migration because
AgentSkill_orgId_visibility_scopeId_slug_key already covers the same orgId,
visibility, and scopeId prefix. Update the migration.sql block that creates
AgentSkill indexes so only the unique index remains for prefix lookups, and keep
the existing unique constraint creation intact.
In `@packages/db/prisma/schema.prisma`:
- Around line 807-812: Remove the redundant `@@index([orgId, visibility,
scopeId])` from the Prisma model since `@@unique([orgId, visibility, scopeId,
slug])` in the same schema already covers that prefix for lookups. Keep the
other indexes intact, and verify the remaining `AgentSkill` indexing still
supports the intended query patterns without duplicating write cost.
In `@packages/web/src/ee/features/chat/agent.ts`:
- Around line 605-659: The skill catalog can grow without bound because
buildSkillRegistry loads every visible personal/shared skill on each request,
which can bloat the dynamic prompt and tool descriptions. Update the ask-agent
flow around buildSkillRegistry/createPrompt to either cap the catalog size or
add pagination/selection for large orgs, and keep the load_skill wiring and
alwaysActiveTools behavior unchanged.
In `@packages/web/src/ee/features/chat/skills/actions.ts`:
- Around line 358-360: Replace the ad hoc ServiceError check in
requireManageableSharedSkill call sites with the existing isServiceError()
helper for consistent narrowing. Update the branches in actions.ts that
currently use "errorCode" in manageableSkill to use
isServiceError(manageableSkill) instead, matching the patterns already used
elsewhere in the file. Make the same consistency change in the other listed call
sites so all ServiceError unions are narrowed through the same helper.
- Around line 63-67: The skill error helpers currently reuse
ErrorCode.INVALID_REQUEST_BODY for two different failure cases, so update the
chat skills error definitions to use distinct error codes for skillNotSynced and
skillSourceInvalid. Add dedicated codes alongside the existing feature-specific
codes, then wire each helper in actions.ts to the appropriate new code while
keeping their current status codes and messages. Reference the skillNotSynced
and skillSourceInvalid functions so the separation is clear and clients can
branch on errorCode reliably.
- Around line 340-381: In getSharedAgentSkill, avoid the extra
prisma.agentSkill.findFirst after requireManageableSharedSkill by making
requireManageableSharedSkill return the needed fields up front, similar to
updateSharedAgentSkill. Refactor requireManageableSharedSkill to accept a
caller-provided select/include (or equivalent generic selection) so
getSharedAgentSkill can request the columns needed by canAccessSkillSource and
toAgentSkillListItem in one query. Apply the same one-round-trip pattern to
updateSharedAgentSkillFromSource as well.
- Around line 876-909: The personal-conversion flow in
makeSharedAgentSkillPersonal is deleting the shared AgentSkill record when
sharedSkill.createdById matches user.id, which cascades removal for every
AgentSkillAdoption. Update this branch so converting a creator-owned shared
skill to personal does not silently unshare it for all adopters, or otherwise
make the behavior explicit before deletion. Use the prisma.$transaction block,
tx.agentSkill.delete, and removeSharedSkillForUser as the key spots to adjust,
and keep the creator’s personal copy creation separate from any global unshare
behavior.
In `@packages/web/src/ee/features/chat/skills/commands.server.ts`:
- Around line 5-33: The three command-loading helpers currently use
console.error, which bypasses the feature’s structured logging path. Update
listPersonalAgentSkillCommandsOrEmpty, listSharedSkillCommandsOrEmpty, and
listAgentSkillCommandsOrEmpty to use the shared logger from `@sourcebot/shared`
via createLogger instead of direct console calls, keeping the same failure
context and return-[] fallback behavior.
In
`@packages/web/src/ee/features/chat/skills/components/importFromRepoDialog.tsx`:
- Line 143: The inline if statement inside the Dialog onOpenChange handler in
importFromRepoDialog should follow the project’s brace formatting rule. Update
the onOpenChange callback so the if (!isOpen) block uses curly braces with the
body on its own line, matching the style used elsewhere in the component and
keeping the resetAndClose call inside that properly formatted block.
- Around line 48-62: The repo search in importFromRepoDialog’s useQuery is
firing listRepos on every keystroke because repoQuery is used directly in the
query key and queryFn. Add a debounce layer for the search input so the query
only updates after typing pauses, while keeping the existing selectedRepo/open
gating and keepPreviousData behavior intact. Update the repoQuery flow in
importFromRepoDialog and any related input state so the debounced value is what
drives the query.
In
`@packages/web/src/ee/features/chat/skills/components/personalSkillEditorPage.tsx`:
- Around line 183-211: The save handler in handleSubmit uses non-null assertions
on skill.id for editShared and editPersonal, which is fragile even though
saveMode currently implies a non-null skill. Refactor the saveMode dispatch in
personalSkillEditorPage so the edit paths are only constructed or called after
an explicit skill null check, or otherwise narrow skill before accessing its id;
keep the createPersonalAgentSkill/createSharedAgentSkill branches unchanged and
avoid relying on skill! inside the map lookup.
In `@packages/web/src/ee/features/chat/skills/components/skillsPage.test.tsx`:
- Around line 159-187: Add test coverage for the owner-but-not-creator
make-personal flow in skillsPage.test.tsx, since the current make-personal cases
only use sharedSkill/syncedSkill and never exercise the non-user-created path.
Update the relevant test setup around renderSkillsPage and handleMakePersonal so
the selected skill has isCreatedByUser set to false while still belonging to the
owner, then verify the confirmation dialog and make-personal action behave
correctly. Use the existing makeSharedAgentSkillPersonal and skillsPage.tsx
handleMakePersonal flow as the anchors for locating the path.
In `@packages/web/src/ee/features/chat/skills/components/skillsPage.tsx`:
- Line 903: The onOpenChange handlers in skillsPage.tsx use a one-line if body
inside the arrow function, which breaks the file’s brace-and-newline formatting
convention. Update both onOpenChange callbacks so the if statement keeps curly
braces and places the body on its own line, matching the surrounding style in
this component and the related handler near the other reported location.
In
`@packages/web/src/ee/features/chat/skills/components/workspaceSkillMutations.ts`:
- Around line 24-46: The updateWorkspaceSkillFlag function accepts a flag
parameter but always sends data.autoEnrolled, so the payload is not actually
driven by the selected flag. Update the setSharedSkillFlag call to build its
data from the flag argument inside updateWorkspaceSkillFlag, so the function
stays aligned with OrgSkillFlagKey and will update the correct field if more
flag keys are added later.
In `@packages/web/src/ee/features/chat/useUnsavedChangesGuard.test.tsx`:
- Around line 38-128: Add test coverage in useUnsavedChangesGuard test suite for
the bypass() path and the one-shot resolve() behavior. Create a case that
triggers the guard, calls bypass(), and verifies the next navigation is allowed
without re-prompting or leaving stale active state, then add a case that
exercises resolve() twice to confirm decisionMadeRef only lets the first
decision take effect. Use the existing useUnsavedChangesGuard hook and
Probe/Noop patterns to locate and validate these flows.
In `@packages/web/src/features/chat/components/chatBox/suggestionsBox.tsx`:
- Around line 133-172: The SuggestionListItem component duplicates nearly the
same JSX in the titleDetail and non-titleDetail branches. Refactor the
SuggestionListItem return so the shared wrapper, title span, and badge rendering
live in one block, and render the titleDetail span conditionally inline only
when present. Use the SuggestionListItem and titleDetail symbols to locate the
duplicated branch logic.
In `@packages/web/src/features/git/getFileSourceApi.ts`:
- Around line 121-143: The repo lookup, path/ref validation, and git setup logic
in resolveFileBlobShaForRepo is duplicated from getFileSourceForRepo, so extract
the shared repo resolution flow into a helper such as resolveRepoAndGitRef that
performs the prisma.repo lookup, isPathValid/isGitRefValid checks, and builds
the simpleGit().cwd(repoPath) plus gitRef value. Update both
getFileSourceForRepo and resolveFileBlobShaForRepo to call the shared helper and
return or propagate the same ServiceError results to keep the two paths
consistent.
- Around line 64-73: The blobSha calculation in getFileSourceApi should be
derived from the already-fetched fileContent instead of calling git.raw with
rev-parse again. Update the blobSha assignment logic in the getFileSourceApi
path so it hashes the returned text body locally, preserves the current
best-effort/undefined-on-failure behavior, and removes the extra git subprocess
while keeping the existing filePath/gitRef-based lookup flow intact.
🪄 Autofix (Beta)
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
Run ID: d888b4a8-037d-40ce-b0f4-199f0fb4fb6b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (98)
CHANGELOG.mddocs/api-reference/sourcebot-public.openapi.jsonpackages/db/prisma/migrations/20260624202848_refactor_agent_skills/migration.sqlpackages/db/prisma/migrations/20260626120000_remove_featured_from_agent_skill/migration.sqlpackages/db/prisma/migrations/20260627223058_add_agent_skill_source_provenance/migration.sqlpackages/db/prisma/schema.prismapackages/db/src/index.tspackages/web/package.jsonpackages/web/src/app/(app)/@sidebar/components/settingsSidebar/nav.tsxpackages/web/src/app/(app)/askgh/[owner]/[repo]/components/landingPage.tsxpackages/web/src/app/(app)/askgh/[owner]/[repo]/page.tsxpackages/web/src/app/(app)/chat/[id]/page.tsxpackages/web/src/app/(app)/chat/chatLandingPage.tsxpackages/web/src/app/(app)/chat/components/landingPageChatBox.tsxpackages/web/src/app/(app)/chat/layout.tsxpackages/web/src/app/(app)/components/searchModeSelector.tsxpackages/web/src/app/(app)/layout.tsxpackages/web/src/app/(app)/settings/accountAskAgent/skills/[skillId]/page.tsxpackages/web/src/app/(app)/settings/accountAskAgent/skills/new/page.tsxpackages/web/src/app/(app)/settings/accountAskAgent/workspaceSkills/[skillId]/page.tsxpackages/web/src/app/(app)/settings/components/settingsContentFrame.test.tsxpackages/web/src/app/(app)/settings/components/settingsContentFrame.tsxpackages/web/src/app/(app)/settings/layout.tsxpackages/web/src/app/(app)/settings/skills/page.tsxpackages/web/src/app/(app)/settings/skills/skillsEntitlementMessage.tsxpackages/web/src/app/(app)/settings/workspaceAskAgent/page.tsxpackages/web/src/app/(app)/settings/workspaceAskAgent/workspaceAskAgentPage.test.tsxpackages/web/src/app/(app)/settings/workspaceAskAgent/workspaceAskAgentPage.tsxpackages/web/src/app/api/(client)/client.tspackages/web/src/app/api/(server)/ee/askmcp/configuration/[serverId]/tools/route.test.tspackages/web/src/app/api/(server)/ee/chat/route.tspackages/web/src/app/api/(server)/ee/skills/sourceStatus/route.tspackages/web/src/components/ui/tooltip.tsxpackages/web/src/ee/features/chat/agent.tspackages/web/src/ee/features/chat/components/chatThread/chatThread.tsxpackages/web/src/ee/features/chat/components/chatThread/detailsCard.test.tsxpackages/web/src/ee/features/chat/components/chatThread/detailsCard.tsxpackages/web/src/ee/features/chat/components/chatThread/tableOfContents.tsxpackages/web/src/ee/features/chat/components/chatThread/tools/loadSkillToolComponent.tsxpackages/web/src/ee/features/chat/components/chatThreadPanel.test.tsxpackages/web/src/ee/features/chat/components/chatThreadPanel.tsxpackages/web/src/ee/features/chat/mcp/components/accountAskAgentPage.test.tsxpackages/web/src/ee/features/chat/mcp/components/accountAskAgentPage.tsxpackages/web/src/ee/features/chat/mcp/components/connectorsMenu.tsxpackages/web/src/ee/features/chat/mcp/mcpToolSets.tspackages/web/src/ee/features/chat/skills/actions.test.tspackages/web/src/ee/features/chat/skills/actions.tspackages/web/src/ee/features/chat/skills/commandCatalog.tspackages/web/src/ee/features/chat/skills/commandResolution.test.tspackages/web/src/ee/features/chat/skills/commandResolution.tspackages/web/src/ee/features/chat/skills/commands.server.tspackages/web/src/ee/features/chat/skills/components/importFromRepoDialog.test.tsxpackages/web/src/ee/features/chat/skills/components/importFromRepoDialog.tsxpackages/web/src/ee/features/chat/skills/components/personalSkillEditorPage.tsxpackages/web/src/ee/features/chat/skills/components/skillInstructionsEditor.test.tspackages/web/src/ee/features/chat/skills/components/skillInstructionsEditor.tsxpackages/web/src/ee/features/chat/skills/components/skillsPage.test.tsxpackages/web/src/ee/features/chat/skills/components/skillsPage.tsxpackages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.test.tsxpackages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.tsxpackages/web/src/ee/features/chat/skills/components/workspaceSkillMutations.tspackages/web/src/ee/features/chat/skills/components/workspaceSkillShared.tsxpackages/web/src/ee/features/chat/skills/loadSkillTool.test.tspackages/web/src/ee/features/chat/skills/loadSkillTool.tspackages/web/src/ee/features/chat/skills/registry.test.tspackages/web/src/ee/features/chat/skills/registry.tspackages/web/src/ee/features/chat/skills/skillAnalytics.test.tspackages/web/src/ee/features/chat/skills/skillAnalytics.tspackages/web/src/ee/features/chat/skills/sourceRepoAccess.test.tspackages/web/src/ee/features/chat/skills/sourceRepoAccess.tspackages/web/src/ee/features/chat/skills/types.test.tspackages/web/src/ee/features/chat/skills/types.tspackages/web/src/ee/features/chat/useTOCItems.tspackages/web/src/ee/features/chat/useUnsavedChangesGuard.test.tsxpackages/web/src/ee/features/chat/useUnsavedChangesGuard.tspackages/web/src/ee/features/oauth/server.tspackages/web/src/features/chat/commands/types.tspackages/web/src/features/chat/commands/utils.test.tspackages/web/src/features/chat/commands/utils.tspackages/web/src/features/chat/components/chatBox/chatBox.tsxpackages/web/src/features/chat/components/chatBox/keyboard.test.tspackages/web/src/features/chat/components/chatBox/keyboard.tspackages/web/src/features/chat/components/chatBox/sourceLabelBadge.tsxpackages/web/src/features/chat/components/chatBox/suggestionsBox.tsxpackages/web/src/features/chat/components/chatBox/types.tspackages/web/src/features/chat/components/chatBox/useSuggestionModeAndQuery.tspackages/web/src/features/chat/components/chatBox/useSuggestionsData.tspackages/web/src/features/chat/mcpOAuthDraft.test.tspackages/web/src/features/chat/mcpOAuthDraft.tspackages/web/src/features/chat/types.tspackages/web/src/features/chat/utils.test.tspackages/web/src/features/chat/utils.tspackages/web/src/features/git/getFileSourceApi.tspackages/web/src/features/git/schemas.tspackages/web/src/lib/errorCodes.tspackages/web/src/lib/posthogEvents.tspackages/web/src/lib/prismaErrors.tspackages/web/tailwind.config.ts
| - [EE] Added DPoP sender-constrained OAuth tokens for MCP clients. [#1395](https://github.com/sourcebot-dev/sourcebot/pull/1395) | ||
| - [EE] Added text file attachments to Ask Sourcebot, letting users attach text/code/config files to a chat message via the paperclip button, drag-and-drop, or paste, with large pastes auto-converted to attachments. [#1374](https://github.com/sourcebot-dev/sourcebot/pull/1374) | ||
| - [EE] Added image attachments to Ask Sourcebot, letting users attach images to a chat message when the selected model supports image input. [#1375](https://github.com/sourcebot-dev/sourcebot/pull/1375) | ||
| - [EE] Added Ask Sourcebot skills, letting users create, import, share, sync, and auto-invoke reusable chat instructions across personal and workspace scopes. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add missing PR link to changelog entry.
This is the only [Unreleased] entry without a trailing PR link; every sibling entry (lines 16-22) ends with [#<id>](<url>).
📝 Proposed fix
-- [EE] Added Ask Sourcebot skills, letting users create, import, share, sync, and auto-invoke reusable chat instructions across personal and workspace scopes.
+- [EE] Added Ask Sourcebot skills, letting users create, import, share, sync, and auto-invoke reusable chat instructions across personal and workspace scopes. [#<PR>](https://github.com/sourcebot-dev/sourcebot/pull/<PR>)As per path instructions, "Update CHANGELOG.md with an entry under [Unreleased] linking to the new PR" and "entries must include the GitHub pull request id at the end of the line, formatted as [#<id>](<url>)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - [EE] Added Ask Sourcebot skills, letting users create, import, share, sync, and auto-invoke reusable chat instructions across personal and workspace scopes. | |
| [EE] Added Ask Sourcebot skills, letting users create, import, share, sync, and auto-invoke reusable chat instructions across personal and workspace scopes. [#<PR>](https://github.com/sourcebot-dev/sourcebot/pull/<PR>) |
🤖 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 `@CHANGELOG.md` at line 23, The Unreleased changelog item for the Ask Sourcebot
skills entry is missing the trailing GitHub PR link, unlike the other sibling
entries. Update the `CHANGELOG.md` Unreleased entry so the line ends with the
pull request reference in the required `[#<id>](<url>)` format, matching the
existing pattern used by the nearby changelog items.
Source: Path instructions
Comment on lines
+773
to
+780
| // The user who authored the skill. This is audit metadata, separate from | ||
| // the namespace that owns the slash-command slug. | ||
| createdBy User @relation("AgentSkillCreatedBy", fields: [createdById], references: [id], onDelete: Cascade) | ||
| createdById String | ||
|
|
||
| updatedBy User? @relation("AgentSkillUpdatedBy", fields: [updatedById], references: [id], onDelete: SetNull) | ||
| updatedById String? | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
createdBy cascade delete can silently wipe shared/org skills.
createdBy uses onDelete: Cascade (Line 775) while updatedBy uses onDelete: SetNull (Line 778) for what the comment (Lines 773-774) calls audit metadata, "separate from the namespace that owns the slash-command slug." For PERSONAL skills this is harmless since scopeId == createdById anyway. But for SHARED skills, deleting the creator's User row (e.g., offboarding an employee) cascades to delete the entire org-wide shared skill — along with every other user's AgentSkillAdoption rows for it — even though the skill is meant to persist as an org resource independent of its original author.
This contradicts the stated intent and risks unexpected data loss on a routine admin action (user removal).
🛠️ Suggested fix
- createdBy User `@relation`("AgentSkillCreatedBy", fields: [createdById], references: [id], onDelete: Cascade)
- createdById String
+ createdBy User? `@relation`("AgentSkillCreatedBy", fields: [createdById], references: [id], onDelete: SetNull)
+ createdById String?Note: this requires updating all createdById: userId call sites and Pick<AgentSkill, ... "createdById" ...> mapping code (e.g. toSharedAgentSkillCatalogItem in types.ts) to tolerate null.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The user who authored the skill. This is audit metadata, separate from | |
| // the namespace that owns the slash-command slug. | |
| createdBy User @relation("AgentSkillCreatedBy", fields: [createdById], references: [id], onDelete: Cascade) | |
| createdById String | |
| updatedBy User? @relation("AgentSkillUpdatedBy", fields: [updatedById], references: [id], onDelete: SetNull) | |
| updatedById String? | |
| // The user who authored the skill. This is audit metadata, separate from | |
| // the namespace that owns the slash-command slug. | |
| createdBy User? `@relation`("AgentSkillCreatedBy", fields: [createdById], references: [id], onDelete: SetNull) | |
| createdById String? | |
| updatedBy User? `@relation`("AgentSkillUpdatedBy", fields: [updatedById], references: [id], onDelete: SetNull) | |
| updatedById String? |
🤖 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 `@packages/db/prisma/schema.prisma` around lines 773 - 780, The AgentSkill
audit relation is too destructive: `createdBy` currently cascades on user
deletion, which can delete shared/org skills and their adopters. Update the
`AgentSkill` model’s `createdBy`/`createdById` relation in the Prisma schema to
stop using cascade delete and allow the creator reference to be cleared instead,
matching the intent of audit metadata. Then update any `createdById` callers and
mappings that assume a required user, including `toSharedAgentSkillCatalogItem`
and any `Pick<AgentSkill, ... "createdById" ...>` usages, so they handle a
nullable creator.
Contributor
Author
There was a problem hiding this comment.
As of current this was actually intentional, but it's unclear if it should be this way or if a shared skill truly becomes an org resource (but then it's weird that the creator is the sole person who can then remove everybodies access to it).
Contributor
There was a problem hiding this comment.
@jsourcebot Thanks for the context — that's a fair tension to flag. A few ways I'd think about resolving the ambiguity, depending on which mental model you want for SHARED skills:
- Skill is an org resource, ownership decoupled from a single user — switch
createdBy/createdByIdtoSetNull(matchingupdatedBy), and introduce an explicit notion of "current owner/maintainer" that's independent from the original author (e.g., allow any orgOWNERto delete/manage aSHAREDskill, not just the creator). This is the cleanest mental model for "shared skills survive offboarding," but requires updating the delete/update authorization logic (currently gated to the creator inactions.ts) plus makingcreatedByIdnullable everywhere it's consumed (e.g.,toSharedAgentSkillCatalogItem). - Keep cascade, but guard the destructive path — leave the schema as-is, but before deleting a
User, check forSHAREDskills they authored and either block the deletion or prompt to reassign/transfer ownership first. This avoids a schema/type migration but adds friction to user offboarding flows. - Status quo — accept that removing a user prunes their shared skills too, treating "shared" as more like "shared while the author remains a member" rather than a durable org asset. Simplest, but as you noted, has surprising blast radius (also deletes all
AgentSkillAdoptionrows for every other member who adopted it).
Given this is a genuine product/design decision rather than a clear-cut bug, I'll leave it to you and the team to decide the intended semantics. If you'd like, I can open a follow-up issue to track "clarify/rework AgentSkill ownership semantics for org offboarding" so it doesn't get lost — just let me know.
✏️ Learnings added
Learnt from: jsourcebot
Repo: sourcebot-dev/sourcebot PR: 1410
File: packages/db/prisma/schema.prisma:773-780
Timestamp: 2026-07-02T02:23:27.352Z
Learning: In Sourcebot's Prisma schema (packages/db/prisma/schema.prisma), the `AgentSkill.createdBy` relation intentionally uses `onDelete: Cascade` (as of PR `#1410`), meaning deleting a `User` who authored a `SHARED` (org-wide) skill also deletes that skill and all associated `AgentSkillAdoption` rows for every other user who adopted it. The team (jsourcebot) has acknowledged this is a known tradeoff/open design question — it's unclear whether shared skills should become independent org resources decoupled from their creator (which would require reworking ownership/authorization, e.g., allowing any org owner to manage a shared skill) or whether the current author-tied-deletion behavior is intended. Do not flag this as a definitive bug in future reviews unless the schema or stated intent changes; treat it as an open question already known to the maintainers.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: fatmcgav
Repo: sourcebot-dev/sourcebot PR: 1143
File: packages/db/prisma/schema.prisma:640-660
Timestamp: 2026-04-22T07:09:48.693Z
Learning: In Sourcebot’s Prisma schema, treat junction/relationship tables (e.g., AgentConfigToRepo, AgentConfigToConnection) as single-tenant and do not require a denormalized `orgId` column for tenant isolation. Tenant isolation is enforced by foreign keys/cascades from `AgentConfig` (which contains `orgId`). When reviewing queries/relations involving these junction tables, avoid raising multi-tenant isolation concerns as long as access is gated through `AgentConfig` filtered by `orgId`.
Comment on lines
+579
to
+609
| export const getAgentSkillSourceStatus = async ( | ||
| skillId: string, | ||
| ): Promise<{ status: AgentSkillSourceStatus } | ServiceError> => sew(() => | ||
| withAuth(async ({ org, user, prisma }) => { | ||
| const askError = await checkAskEntitlement(); | ||
| if (askError) { | ||
| return askError; | ||
| } | ||
|
|
||
| const skill = await prisma.agentSkill.findFirst({ | ||
| where: { | ||
| id: skillId, | ||
| OR: [ | ||
| personalAgentSkillAuthScope(user.id, org.id), | ||
| { ...sharedAgentSkillAuthScope(org.id), enabled: true }, | ||
| ], | ||
| }, | ||
| select: { | ||
| sourceRepoName: true, | ||
| sourceFilePath: true, | ||
| sourceRevision: true, | ||
| sourceBlobSha: true, | ||
| }, | ||
| }); | ||
|
|
||
| if (!skill) { | ||
| return skillNotFound(); | ||
| } | ||
|
|
||
| return resolveSourceStatus(skill, { org, prisma }); | ||
| })); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Sync-status lookup isn't scoped to visible shared skills.
getAgentSkillSourceStatus allows any authenticated org member to query the freshness status of any enabled shared skill (sharedAgentSkillAuthScope(org.id) + enabled: true), regardless of whether that skill is auto-enrolled, adopted by the caller, or passes canAccessSkillSource (unlike getSharedAgentSkill/adoptSharedSkill, which both gate on canAccessSkillSource). While no skill content is returned, a caller who somehow obtains a skillId not otherwise visible to them (e.g. a manager-only shared skill they aren't subscribed to) can still probe its sync status. Consider reusing canAccessSkillSource/sharedAgentSkillVisibleToUserWhere here for consistency with the other read paths.
🤖 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 `@packages/web/src/ee/features/chat/skills/actions.ts` around lines 579 - 609,
The shared-skill lookup in getAgentSkillSourceStatus is too permissive because
it uses sharedAgentSkillAuthScope(org.id) with enabled: true instead of the same
visibility checks used by getSharedAgentSkill and adoptSharedSkill. Update the
Prisma findFirst filter to scope shared skills through canAccessSkillSource or
sharedAgentSkillVisibleToUserWhere so only skills visible to the caller can have
their sync status queried, while keeping the personalAgentSkillAuthScope path
unchanged.
Comment on lines
+228
to
+238
| <Switch | ||
| checked={skill.autoEnrolled} | ||
| disabled={isFlagPending} | ||
| onCheckedChange={(checked) => void handleFlagChange(skill, "autoEnrolled", checked)} | ||
| aria-label="Auto" | ||
| className={cn( | ||
| "data-[state=unchecked]:bg-muted-foreground/40 data-[state=unchecked]:border-muted-foreground/70", | ||
| "data-[state=checked]:bg-green-600 data-[state=checked]:border-green-600", | ||
| "[&>span]:bg-foreground", | ||
| )} | ||
| /> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Switch aria-label is identical across all skill rows.
Every row's Switch uses the static aria-label="Auto", so screen readers announce the same accessible name for every skill's auto-enroll toggle, making rows indistinguishable to assistive tech. The adjacent delete button already differentiates via aria-label={Delete ${skill.name}} — apply the same pattern here.
♿ Proposed fix
<Switch
checked={skill.autoEnrolled}
disabled={isFlagPending}
onCheckedChange={(checked) => void handleFlagChange(skill, "autoEnrolled", checked)}
- aria-label="Auto"
+ aria-label={`Auto-enroll ${skill.name}`}
className={cn(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Switch | |
| checked={skill.autoEnrolled} | |
| disabled={isFlagPending} | |
| onCheckedChange={(checked) => void handleFlagChange(skill, "autoEnrolled", checked)} | |
| aria-label="Auto" | |
| className={cn( | |
| "data-[state=unchecked]:bg-muted-foreground/40 data-[state=unchecked]:border-muted-foreground/70", | |
| "data-[state=checked]:bg-green-600 data-[state=checked]:border-green-600", | |
| "[&>span]:bg-foreground", | |
| )} | |
| /> | |
| <Switch | |
| checked={skill.autoEnrolled} | |
| disabled={isFlagPending} | |
| onCheckedChange={(checked) => void handleFlagChange(skill, "autoEnrolled", checked)} | |
| aria-label={`Auto-enroll ${skill.name}`} | |
| className={cn( | |
| "data-[state=unchecked]:bg-muted-foreground/40 data-[state=unchecked]:border-muted-foreground/70", | |
| "data-[state=checked]:bg-green-600 data-[state=checked]:border-green-600", | |
| "[&>span]:bg-foreground", | |
| )} | |
| /> |
🤖 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
`@packages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.tsx`
around lines 228 - 238, The Switch in workspaceSharedSkillsManager currently
uses the same static aria-label for every row, so each auto-enroll toggle is
indistinguishable to screen readers. Update the Switch’s aria-label in the skill
row render to include the current skill name, following the same pattern used by
the delete button, and keep the change localized around the `Switch` inside the
workspaceSharedSkillsManager component.
Contributor
nit: looks like the "x" button is not centered vertically |
Contributor
Not something we need to do now, but it would be cool if the icon was clickable and it revealed a icon/emoji dropdown, similar to Linear: 
|
msukkari
reviewed
Jul 2, 2026
| // (activationMethod: 'auto', via the load_skill tool) or manually by the | ||
| // user (activationMethod: 'manual', via a slash command). Multi-source, so | ||
| // no wa_ prefix; source identifies the origin. | ||
| ask_skill_invoked: { |
Contributor
There was a problem hiding this comment.
are we sending posthog events for other instances (ex. create skill, edit skill, share skill, etc) @jsourcebot ? we should add events for every interaction
Contributor
Author
There was a problem hiding this comment.
No, I can add more extensive posthog logging though
Contributor
There was a problem hiding this comment.
sg thanks, we should try to add posthog events for every major action
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.test.tsx (1)
74-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding test coverage for the delete flow.
This suite tests filtering, badges, and auto-enroll toggling well, but doesn't exercise
handleDelete/DeleteWorkspaceSkillDialogconfirm flow, a destructive mutation path in the component.🤖 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 `@packages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.test.tsx` around lines 74 - 178, Add a test for the delete path in WorkspaceSharedSkillsManager to cover the destructive confirm flow that is currently missing. Render the component, open the delete action for a skill, assert that DeleteWorkspaceSkillDialog appears, confirm the deletion, and verify handleDelete triggers the expected mutation/action and updates/removes the row from the UI. Use the existing WorkspaceSharedSkillsManager, handleDelete, and DeleteWorkspaceSkillDialog symbols to locate the right interaction points.
🤖 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.
Nitpick comments:
In
`@packages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.test.tsx`:
- Around line 74-178: Add a test for the delete path in
WorkspaceSharedSkillsManager to cover the destructive confirm flow that is
currently missing. Render the component, open the delete action for a skill,
assert that DeleteWorkspaceSkillDialog appears, confirm the deletion, and verify
handleDelete triggers the expected mutation/action and updates/removes the row
from the UI. Use the existing WorkspaceSharedSkillsManager, handleDelete, and
DeleteWorkspaceSkillDialog symbols to locate the right interaction points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ad32956-2546-4b10-bfd5-c74639e23666
📒 Files selected for processing (11)
packages/web/src/app/(app)/settings/workspaceAskAgent/page.tsxpackages/web/src/app/(app)/settings/workspaceAskAgent/workspaceAskAgentPage.tsxpackages/web/src/ee/features/chat/skills/actions.test.tspackages/web/src/ee/features/chat/skills/actions.tspackages/web/src/ee/features/chat/skills/components/skillsPage.test.tsxpackages/web/src/ee/features/chat/skills/components/skillsPage.tsxpackages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.test.tsxpackages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.tsxpackages/web/src/ee/features/chat/skills/components/workspaceSkillMutations.tspackages/web/src/ee/features/chat/skills/components/workspaceSkillShared.tsxpackages/web/src/ee/features/chat/skills/types.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/web/src/ee/features/chat/skills/components/workspaceSkillMutations.ts
- packages/web/src/ee/features/chat/skills/components/workspaceSkillShared.tsx
- packages/web/src/ee/features/chat/skills/components/skillsPage.test.tsx
- packages/web/src/ee/features/chat/skills/components/skillsPage.tsx
- packages/web/src/ee/features/chat/skills/types.ts
- packages/web/src/ee/features/chat/skills/actions.ts
- packages/web/src/ee/features/chat/skills/actions.test.ts
jsourcebot
force-pushed
the
jminnetian/askskills
branch
from
3f103b0 to
e89dcd7
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/ee/features/chat/skills/commandCatalog.ts (1)
63-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared query/filter/map pipeline.
Both functions differ only in the
whereclause and source id/label; the findMany → filterSkillsBySourceRepoAccess → map pipeline is duplicated. A small private helper takingwhere,sourceId, andsourceLabelwould remove the duplication while keeping the "can never drift" guarantee the comments already aim for.♻️ Possible consolidation
+const listAgentSkillCommandsWhere = async ({ + prisma, + where, + orgId, + sourceId, + sourceLabel, +}: { + prisma: PrismaClient; + where: Prisma.AgentSkillWhereInput; + orgId: number; + sourceId: string; + sourceLabel: string; +}): Promise<AskCommandDefinition[]> => { + const skills = await prisma.agentSkill.findMany({ + where, + orderBy: agentSkillOrderBy, + select: agentSkillCommandSelect, + }); + const accessibleSkills = await filterSkillsBySourceRepoAccess(skills, { prisma, orgId }); + return accessibleSkills.map((skill) => toAskCommandDefinition(skill, sourceId, sourceLabel)); +};🤖 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 `@packages/web/src/ee/features/chat/skills/commandCatalog.ts` around lines 63 - 119, The query/filter/map flow in listPersonalAgentSkillCommandsForContext and listSharedAgentSkillCommandsForContext is duplicated and should be consolidated. Extract a private helper that takes the Prisma where clause plus the command source id and source label, then reuse it for both functions. Keep the existing findMany, filterSkillsBySourceRepoAccess, and toAskCommandDefinition pipeline in that helper so only the scope-specific inputs differ.
🤖 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.
Nitpick comments:
In `@packages/web/src/ee/features/chat/skills/commandCatalog.ts`:
- Around line 63-119: The query/filter/map flow in
listPersonalAgentSkillCommandsForContext and
listSharedAgentSkillCommandsForContext is duplicated and should be consolidated.
Extract a private helper that takes the Prisma where clause plus the command
source id and source label, then reuse it for both functions. Keep the existing
findMany, filterSkillsBySourceRepoAccess, and toAskCommandDefinition pipeline in
that helper so only the scope-specific inputs differ.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 887112df-d72a-4dfa-8ae7-33a2689b2cfe
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (98)
CHANGELOG.mddocs/api-reference/sourcebot-public.openapi.jsonpackages/db/prisma/migrations/20260624202848_refactor_agent_skills/migration.sqlpackages/db/prisma/migrations/20260626120000_remove_featured_from_agent_skill/migration.sqlpackages/db/prisma/migrations/20260627223058_add_agent_skill_source_provenance/migration.sqlpackages/db/prisma/schema.prismapackages/db/src/index.tspackages/web/package.jsonpackages/web/src/app/(app)/@sidebar/components/settingsSidebar/nav.tsxpackages/web/src/app/(app)/askgh/[owner]/[repo]/components/landingPage.tsxpackages/web/src/app/(app)/askgh/[owner]/[repo]/page.tsxpackages/web/src/app/(app)/chat/[id]/page.tsxpackages/web/src/app/(app)/chat/chatLandingPage.tsxpackages/web/src/app/(app)/chat/components/landingPageChatBox.tsxpackages/web/src/app/(app)/chat/layout.tsxpackages/web/src/app/(app)/components/searchModeSelector.tsxpackages/web/src/app/(app)/layout.tsxpackages/web/src/app/(app)/settings/accountAskAgent/skills/[skillId]/page.tsxpackages/web/src/app/(app)/settings/accountAskAgent/skills/new/page.tsxpackages/web/src/app/(app)/settings/accountAskAgent/workspaceSkills/[skillId]/page.tsxpackages/web/src/app/(app)/settings/components/settingsContentFrame.test.tsxpackages/web/src/app/(app)/settings/components/settingsContentFrame.tsxpackages/web/src/app/(app)/settings/layout.tsxpackages/web/src/app/(app)/settings/skills/page.tsxpackages/web/src/app/(app)/settings/skills/skillsEntitlementMessage.tsxpackages/web/src/app/(app)/settings/workspaceAskAgent/page.tsxpackages/web/src/app/(app)/settings/workspaceAskAgent/workspaceAskAgentPage.test.tsxpackages/web/src/app/(app)/settings/workspaceAskAgent/workspaceAskAgentPage.tsxpackages/web/src/app/api/(client)/client.tspackages/web/src/app/api/(server)/ee/askmcp/configuration/[serverId]/tools/route.test.tspackages/web/src/app/api/(server)/ee/chat/route.tspackages/web/src/app/api/(server)/ee/skills/sourceStatus/route.tspackages/web/src/components/ui/tooltip.tsxpackages/web/src/ee/features/chat/agent.tspackages/web/src/ee/features/chat/components/chatThread/chatThread.tsxpackages/web/src/ee/features/chat/components/chatThread/detailsCard.test.tsxpackages/web/src/ee/features/chat/components/chatThread/detailsCard.tsxpackages/web/src/ee/features/chat/components/chatThread/tableOfContents.tsxpackages/web/src/ee/features/chat/components/chatThread/tools/loadSkillToolComponent.tsxpackages/web/src/ee/features/chat/components/chatThreadPanel.test.tsxpackages/web/src/ee/features/chat/components/chatThreadPanel.tsxpackages/web/src/ee/features/chat/mcp/components/accountAskAgentPage.test.tsxpackages/web/src/ee/features/chat/mcp/components/accountAskAgentPage.tsxpackages/web/src/ee/features/chat/mcp/components/connectorsMenu.tsxpackages/web/src/ee/features/chat/mcp/mcpToolSets.tspackages/web/src/ee/features/chat/skills/actions.test.tspackages/web/src/ee/features/chat/skills/actions.tspackages/web/src/ee/features/chat/skills/commandCatalog.tspackages/web/src/ee/features/chat/skills/commandResolution.test.tspackages/web/src/ee/features/chat/skills/commandResolution.tspackages/web/src/ee/features/chat/skills/commands.server.tspackages/web/src/ee/features/chat/skills/components/importFromRepoDialog.test.tsxpackages/web/src/ee/features/chat/skills/components/importFromRepoDialog.tsxpackages/web/src/ee/features/chat/skills/components/personalSkillEditorPage.tsxpackages/web/src/ee/features/chat/skills/components/skillInstructionsEditor.test.tspackages/web/src/ee/features/chat/skills/components/skillInstructionsEditor.tsxpackages/web/src/ee/features/chat/skills/components/skillsPage.test.tsxpackages/web/src/ee/features/chat/skills/components/skillsPage.tsxpackages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.test.tsxpackages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.tsxpackages/web/src/ee/features/chat/skills/components/workspaceSkillMutations.tspackages/web/src/ee/features/chat/skills/components/workspaceSkillShared.tsxpackages/web/src/ee/features/chat/skills/loadSkillTool.test.tspackages/web/src/ee/features/chat/skills/loadSkillTool.tspackages/web/src/ee/features/chat/skills/registry.test.tspackages/web/src/ee/features/chat/skills/registry.tspackages/web/src/ee/features/chat/skills/skillAnalytics.test.tspackages/web/src/ee/features/chat/skills/skillAnalytics.tspackages/web/src/ee/features/chat/skills/sourceRepoAccess.test.tspackages/web/src/ee/features/chat/skills/sourceRepoAccess.tspackages/web/src/ee/features/chat/skills/types.test.tspackages/web/src/ee/features/chat/skills/types.tspackages/web/src/ee/features/chat/useTOCItems.tspackages/web/src/ee/features/chat/useUnsavedChangesGuard.test.tsxpackages/web/src/ee/features/chat/useUnsavedChangesGuard.tspackages/web/src/ee/features/oauth/server.tspackages/web/src/features/chat/commands/types.tspackages/web/src/features/chat/commands/utils.test.tspackages/web/src/features/chat/commands/utils.tspackages/web/src/features/chat/components/chatBox/chatBox.tsxpackages/web/src/features/chat/components/chatBox/keyboard.test.tspackages/web/src/features/chat/components/chatBox/keyboard.tspackages/web/src/features/chat/components/chatBox/sourceLabelBadge.tsxpackages/web/src/features/chat/components/chatBox/suggestionsBox.tsxpackages/web/src/features/chat/components/chatBox/types.tspackages/web/src/features/chat/components/chatBox/useSuggestionModeAndQuery.tspackages/web/src/features/chat/components/chatBox/useSuggestionsData.tspackages/web/src/features/chat/mcpOAuthDraft.test.tspackages/web/src/features/chat/mcpOAuthDraft.tspackages/web/src/features/chat/types.tspackages/web/src/features/chat/utils.test.tspackages/web/src/features/chat/utils.tspackages/web/src/features/git/getFileSourceApi.tspackages/web/src/features/git/schemas.tspackages/web/src/lib/errorCodes.tspackages/web/src/lib/posthogEvents.tspackages/web/src/lib/prismaErrors.tspackages/web/tailwind.config.ts
✅ Files skipped from review due to trivial changes (6)
- packages/web/src/ee/features/chat/skills/sourceRepoAccess.test.ts
- packages/db/prisma/migrations/20260627223058_add_agent_skill_source_provenance/migration.sql
- packages/web/src/ee/features/chat/components/chatThread/tableOfContents.tsx
- packages/web/src/lib/errorCodes.ts
- packages/web/package.json
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (87)
- packages/web/src/ee/features/chat/mcp/components/accountAskAgentPage.test.tsx
- packages/web/src/ee/features/chat/skills/components/skillInstructionsEditor.test.ts
- packages/web/src/app/(app)/settings/components/settingsContentFrame.test.tsx
- packages/web/src/app/(app)/settings/accountAskAgent/skills/new/page.tsx
- packages/web/src/features/chat/components/chatBox/keyboard.test.ts
- packages/web/src/ee/features/chat/skills/commands.server.ts
- packages/web/src/features/chat/commands/utils.ts
- packages/web/src/ee/features/chat/skills/skillAnalytics.ts
- packages/web/src/features/chat/components/chatBox/keyboard.ts
- packages/web/src/app/(app)/askgh/[owner]/[repo]/page.tsx
- packages/web/src/app/api/(server)/ee/chat/route.ts
- packages/web/src/ee/features/chat/components/chatThreadPanel.test.tsx
- packages/web/src/app/(app)/layout.tsx
- packages/web/src/features/chat/mcpOAuthDraft.ts
- packages/web/src/ee/features/chat/components/chatThread/tools/loadSkillToolComponent.tsx
- packages/web/src/features/chat/components/chatBox/sourceLabelBadge.tsx
- packages/web/src/ee/features/chat/mcp/components/connectorsMenu.tsx
- docs/api-reference/sourcebot-public.openapi.json
- packages/web/src/lib/posthogEvents.ts
- packages/web/src/features/chat/commands/utils.test.ts
- packages/web/src/ee/features/chat/useTOCItems.ts
- packages/web/src/app/(app)/askgh/[owner]/[repo]/components/landingPage.tsx
- packages/web/src/features/chat/components/chatBox/types.ts
- packages/web/src/app/(app)/components/searchModeSelector.tsx
- packages/web/src/lib/prismaErrors.ts
- packages/db/src/index.ts
- packages/web/src/components/ui/tooltip.tsx
- packages/web/src/app/(app)/chat/[id]/page.tsx
- packages/web/src/ee/features/chat/skills/loadSkillTool.test.ts
- packages/web/src/ee/features/chat/mcp/components/accountAskAgentPage.tsx
- packages/web/src/app/(app)/@sidebar/components/settingsSidebar/nav.tsx
- packages/web/src/ee/features/chat/skills/components/workspaceSkillMutations.ts
- packages/web/src/features/chat/components/chatBox/useSuggestionsData.ts
- packages/web/src/app/(app)/settings/layout.tsx
- packages/web/src/ee/features/chat/components/chatThread/detailsCard.test.tsx
- packages/web/src/features/chat/commands/types.ts
- packages/web/src/features/chat/components/chatBox/useSuggestionModeAndQuery.ts
- packages/web/src/ee/features/chat/skills/types.test.ts
- packages/web/src/features/chat/utils.ts
- packages/web/src/ee/features/chat/useUnsavedChangesGuard.test.tsx
- packages/web/src/app/(app)/settings/accountAskAgent/skills/[skillId]/page.tsx
- packages/web/src/app/(app)/settings/workspaceAskAgent/workspaceAskAgentPage.tsx
- packages/web/src/ee/features/oauth/server.ts
- packages/web/src/ee/features/chat/skills/loadSkillTool.ts
- packages/web/src/ee/features/chat/skills/skillAnalytics.test.ts
- packages/web/src/app/(app)/settings/skills/skillsEntitlementMessage.tsx
- packages/web/src/app/(app)/settings/skills/page.tsx
- packages/web/src/app/api/(server)/ee/skills/sourceStatus/route.ts
- packages/web/src/app/api/(client)/client.ts
- packages/web/src/app/(app)/settings/components/settingsContentFrame.tsx
- packages/web/src/ee/features/chat/skills/sourceRepoAccess.ts
- packages/web/src/ee/features/chat/useUnsavedChangesGuard.ts
- packages/web/src/app/(app)/settings/accountAskAgent/workspaceSkills/[skillId]/page.tsx
- packages/web/src/features/chat/utils.test.ts
- packages/web/src/ee/features/chat/mcp/mcpToolSets.ts
- packages/web/src/features/chat/mcpOAuthDraft.test.ts
- packages/web/src/app/(app)/settings/workspaceAskAgent/workspaceAskAgentPage.test.tsx
- packages/web/src/features/chat/components/chatBox/suggestionsBox.tsx
- packages/web/src/ee/features/chat/skills/components/personalSkillEditorPage.tsx
- packages/web/src/features/chat/types.ts
- packages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.tsx
- packages/web/src/ee/features/chat/skills/components/workspaceSharedSkillsManager.test.tsx
- packages/web/src/app/(app)/chat/components/landingPageChatBox.tsx
- packages/web/src/ee/features/chat/components/chatThreadPanel.tsx
- packages/web/src/app/(app)/chat/chatLandingPage.tsx
- packages/web/src/ee/features/chat/skills/components/importFromRepoDialog.tsx
- packages/web/tailwind.config.ts
- packages/web/src/features/git/getFileSourceApi.ts
- packages/web/src/features/git/schemas.ts
- packages/db/prisma/migrations/20260624202848_refactor_agent_skills/migration.sql
- packages/web/src/ee/features/chat/components/chatThread/detailsCard.tsx
- packages/web/src/ee/features/chat/skills/registry.test.ts
- packages/web/src/ee/features/chat/skills/components/workspaceSkillShared.tsx
- packages/db/prisma/schema.prisma
- packages/web/src/app/api/(server)/ee/askmcp/configuration/[serverId]/tools/route.test.ts
- packages/web/src/app/(app)/settings/workspaceAskAgent/page.tsx
- packages/web/src/ee/features/chat/skills/commandResolution.test.ts
- packages/web/src/ee/features/chat/skills/registry.ts
- packages/web/src/app/(app)/chat/layout.tsx
- packages/web/src/ee/features/chat/skills/components/importFromRepoDialog.test.tsx
- packages/web/src/ee/features/chat/skills/components/skillsPage.test.tsx
- packages/web/src/ee/features/chat/components/chatThread/chatThread.tsx
- packages/web/src/features/chat/components/chatBox/chatBox.tsx
- packages/web/src/ee/features/chat/skills/types.ts
- packages/web/src/ee/features/chat/skills/actions.ts
- packages/web/src/ee/features/chat/skills/components/skillsPage.tsx
- packages/web/src/ee/features/chat/agent.ts
… for the organization
Any org member can create a skill into the shared workspace catalog; members opt in by adopting it, while only owners can feature or auto-enroll a skill for everyone. Personal and org skills may share a slug since commands resolve by source and id, so duplicates from different sources coexist.
The Ask model can now discover opt-in skills from an <agent_skills> catalog in the system prompt and load their instructions on demand through a single load_skill tool, reusing the existing auth-scope helpers and argument substitution. Auto-invocation is gated per skill by a new autoInvocationEnabled flag and a global env kill-switch; manual slash-command invocation is unchanged.
Surface load_skill tool calls in the Ask details card via a new tool-part type and component so users can see which skill the model loaded and its instructions. Also folds in review hardening: sanitize every skill catalog field injected into the system prompt, make load_skill fail closed on transient lookup errors, and consolidate the manual and auto skill catalogs onto one shared query helper. Includes the migration for the autoInvocationEnabled column.
… personal skills Skills are now always model-invocable: removed the per-skill autoInvocationEnabled flag, its UI toggle, and the env kill-switch. Skills no longer take arguments, so the substitution engine and argumentNames are gone. Personal skills are bound to the current org, and the ORG visibility is renamed to SHARED across schema, server, and UI. The five skill migrations are collapsed into one.
Extract personal and shared skill management out of the account Ask Sourcebot settings into a routed /settings/skills two-pane page with inline editing. The account page now manages connectors only; existing skill actions are reused, with the shared catalog surfacing instructions and creator email for the detail view.
Make the instructions editor flush with the name/command bars, let split view use the full pane, and keep the view toggle and counter stationary across mode switches. Replace the misleading Shared toggle on other members' shared skills with an enable toggle in the list, color the Auto/Featured badges instead of using icons, add a Skills entry to the chat + menu, and stop the table-of-contents focus ring from lingering after a click.
Drop the `featured` flag entirely from agent skills: the schema column and shared-catalog index, the server selects/sort/flag-input schema, the Featured badge and admin toggle, and all related tests. Shared skills now sort by recency then name; the remaining auto-enroll flag is unchanged. Includes a migration dropping the column.
Remove the unused OrgSkillFlagToggle export (and its now-unused Switch import), which had no consumers after the shared-skills admin table redesign. Simplify the delete button's onClick, which is already disabled while a flag mutation is pending.
jsourcebot
force-pushed
the
jminnetian/askskills
branch
from
5806e26 to
603b292
Compare
| // Routes that opt out of the centered settings column and render edge-to-edge | ||
| // (e.g. the full-bleed skill editor). Everything else stays in the constrained, | ||
| // centered column shared by the rest of settings. | ||
| const FULL_BLEED_ROUTE_PATTERNS = [ |
Contributor
There was a problem hiding this comment.
could we include the members page as part of this pattern?
Contributor
There was a problem hiding this comment.
maybe we want to standardize around the existing SettingsContainer?
Contributor
|
@jsourcebot could you include UX screenshots in the PR description? |
| * instructions directly. This keeps the tool set static across steps, so the | ||
| * prompt cache is never invalidated by skill loading. | ||
| */ | ||
| export const createLoadSkillTool = ({ |
Contributor
There was a problem hiding this comment.
Thoughts on: for the purposes of consistency in the codebase, would it make sense for this tool (as well as tool_request_activation tool) be defined using the ToolDefinition interface and be located next to the other tools in /features/tools?
Contributor
There was a problem hiding this comment.
this fils is marked .server.ts - is this accurate?
| )} | ||
| {...props} | ||
| /> | ||
| <TooltipPrimitive.Portal> |
Contributor
There was a problem hiding this comment.
Heads up: iirc I had done this before, but it broke certain Tooltips in the app, so you might want to double check this isn't going to break anything. Additionally, I know there are some tooltips (e.g., upgradeBadge.tsx) that are already wrapping the Tooltip in a portal.
Contributor
There was a problem hiding this comment.
nit: from a maintainability & readability pov, consider breaking this file up a bit
| ]; | ||
| })); | ||
|
|
||
| export const createSharedAgentSkill = async ( |
Contributor
There was a problem hiding this comment.
I see that you're using this server action as part of a form submission, which in that case makes sense in terms of validating against a schema. In other parts of the codebase, we are using useForm, which is a hook that allows you to validate a form schema client-side. After it's been validated client-side, we just call a server action with direct parameters.
We may want to consider doing that instead, because it's kind of the pattern that we've been using in the codebase thus far.
| })); | ||
| }; | ||
|
|
||
| export const updateSharedAgentSkill = async ( |
Contributor
There was a problem hiding this comment.
same comment as above
| input: SharedSkillFlagInput, | ||
| analytics?: SkillAnalyticsContext, | ||
| ): Promise<SharedAgentSkillManagementItem | ServiceError> => { | ||
| const parsed = sharedSkillFlagInputSchema.safeParse(input); |
Contributor
There was a problem hiding this comment.
same comment as above
| ...sharedAgentSkillAuthScope(org.id), | ||
| enabled: true, | ||
| }, | ||
| select: { |
Contributor
There was a problem hiding this comment.
do we need to do this select? Applies to other parts of this file.
| <p {...props.attributes} className="m-0">{props.children}</p> | ||
| ); | ||
|
|
||
| const FileMentionComponent = ({ |
Contributor
There was a problem hiding this comment.
nit: this feels like it should be shared with whatever we are doing in the Chatbox
brendan-kellam
approved these changes
Jul 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added Ask Sourcebot skills, letting users create, import, share, sync, and auto-invoke reusable chat instructions across personal and workspace scopes.
Summary by CodeRabbit
New Features
Bug Fixes