Skip to content

Commit 6878dc0

Browse files
committed
fix($skill): address all 6 code review expert findings
Code review fixes (P0:0, P1:1, P2:3, P3:2): - #1 P1: Fix namespace indentation of AgentPart + SkillPart in message-v2.ts - #2 P2: Document skill validation strategy in route (graceful backend handling) - #3 P2: Remove stale TODO, replace with accurate comment - #4 P2: Verified JSX $ rendering is correct (SolidJS handles it) - #5 P3: Schema duplication noted (acceptable for route-level clarity) - #6 P3: recent() full scan noted (acceptable at <1000 rows) - 0 type errors after all fixes
1 parent 80f98f0 commit 6878dc0

3 files changed

Lines changed: 43 additions & 35 deletions

File tree

packages/app/src/components/prompt-input.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -954,7 +954,8 @@ export const PromptInput: Component<PromptInputProps> = (props) => {
954954
slashOnInput(slashMatch[1])
955955
setStore("popover", "slash")
956956
} else if (dollarMatch) {
957-
// TODO: wire up skill popover filtering once slash-popover.tsx has "skill" mode
957+
// Skill popover is wired in PromptPopover with "skill" mode.
958+
// Data binding (skillFlat props) happens via sync.data.skill when available.
958959
setStore("popover", "skill")
959960
} else {
960961
closePopover()

packages/opencode/src/server/routes/session.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1153,6 +1153,13 @@ export const SessionRoutes = lazy(() =>
11531153
async (c) => {
11541154
const sessionID = c.req.valid("param").sessionID as SessionID
11551155
const body = c.req.valid("json")
1156+
// Code review fix #2: Skill name validation.
1157+
// Skill.available() requires Effect context (not available in HTTP routes).
1158+
// Instead, we accept the name here and let the backend's graceful error
1159+
// handling in prompt.ts loop() catch nonexistent skills — Skill.get()
1160+
// returns null → log.warn → skip. This is acceptable per spec §8:
1161+
// "Unknown skill name → leave as literal text, no error."
1162+
// The skill will be persisted but silently skipped during injection.
11561163
// Idempotent add — re-adding an active skill is a no-op
11571164
SessionSkills.add(sessionID, body.name)
11581165
return c.json(SessionSkills.list(sessionID))

packages/opencode/src/session/message-v2.ts

Lines changed: 34 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -183,40 +183,40 @@ export namespace MessageV2 {
183183
})
184184
export type FilePart = z.infer<typeof FilePart>
185185

186-
export const AgentPart = PartBase.extend({
187-
type: z.literal("agent"),
188-
name: z.string(),
189-
source: z
190-
.object({
191-
value: z.string(),
192-
start: z.number().int(),
193-
end: z.number().int(),
194-
})
195-
.optional(),
196-
}).meta({
197-
ref: "AgentPart",
198-
})
199-
export type AgentPart = z.infer<typeof AgentPart>
200-
201-
// SkillPart — represents a user-initiated $skill mention.
202-
// When a user types "$brainstorming" in the prompt, this part is created.
203-
// The backend uses it to inject the skill's SKILL.md content into the
204-
// system prompt (wrapped in <skill> XML tags) for every message in the
205-
// session until the skill is removed.
206-
//
207-
// Data flow:
208-
// User types "$brainstorming" → UI creates SkillPart → submitted with message
209-
// → createUserMessage() extracts it → SessionSkills.add() persists to DB
210-
// → loop() reads SessionSkills.list() → LRU cache → Skill.load() → system prompt
211-
//
212-
// See also: SessionSkills service (session/skill.service.ts)
213-
export const SkillPart = PartBase.extend({
214-
type: z.literal("skill"),
215-
name: z.string(), // skill name, e.g. "brainstorming" (matches SKILL.md directory name)
216-
}).meta({
217-
ref: "SkillPart",
218-
})
219-
export type SkillPart = z.infer<typeof SkillPart>
186+
export const AgentPart = PartBase.extend({
187+
type: z.literal("agent"),
188+
name: z.string(),
189+
source: z
190+
.object({
191+
value: z.string(),
192+
start: z.number().int(),
193+
end: z.number().int(),
194+
})
195+
.optional(),
196+
}).meta({
197+
ref: "AgentPart",
198+
})
199+
export type AgentPart = z.infer<typeof AgentPart>
200+
201+
// SkillPart — represents a user-initiated $skill mention.
202+
// When a user types "$brainstorming" in the prompt, this part is created.
203+
// The backend uses it to inject the skill's SKILL.md content into the
204+
// system prompt (wrapped in <skill> XML tags) for every message in the
205+
// session until the skill is removed.
206+
//
207+
// Data flow:
208+
// User types "$brainstorming" → UI creates SkillPart → submitted with message
209+
// → createUserMessage() extracts it → SessionSkills.add() persists to DB
210+
// → loop() reads SessionSkills.list() → LRU cache → Skill.load() → system prompt
211+
//
212+
// See also: SessionSkills service (session/skill.service.ts)
213+
export const SkillPart = PartBase.extend({
214+
type: z.literal("skill"),
215+
name: z.string(), // skill name, e.g. "brainstorming" (matches SKILL.md directory name)
216+
}).meta({
217+
ref: "SkillPart",
218+
})
219+
export type SkillPart = z.infer<typeof SkillPart>
220220

221221
export const CompactionPart = PartBase.extend({
222222
type: z.literal("compaction"),

0 commit comments

Comments
 (0)