chore: unship skills feature#957
Conversation
|
Claude (@claude) review |
|
Greptile (@greptileai) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 1m 53s —— View job PR Review: chore: unship skills feature
SummaryThis is a clean, thorough unship. 3,368 lines deleted, 16 added — the additions are all defensive test assertions confirming the feature is gone. The scope matches the description: product-facing Skills surface removed end-to-end, ACPX runtime skills untouched. No obvious dangling references. What looks goodComplete surface removal — Navigation item, route registration, redirect aliases, page components, tips copy, feature gate, and the Server cleanup is thorough — Startup lifecycle cleaned — ACPX runtime skills untouched —
Prompt section count deduplication — Updating "includes all 14 v6 sections" to "includes all v6 sections" is a pragmatic improvement; the hardcoded count was a brittle assertion waiting to cause confusion on the next section change. Minor observationsAwkward type assertion in const prompt = buildRegular({
skillsCatalog: '<available_skills>...',
} as Partial<BuildSystemPromptOptions> & { skillsCatalog: string })
expect(prompt).not.toContain('<available_skills>')The intent (prove that passing a deleted option has no effect) is valid, but the double-cast is a bit tortured. Since Leftover user data on disk Existing users with custom skills in
VerdictLooks good to merge. The removal is complete and consistent, ACPX skills are correctly preserved, and the test changes are sound. The one minor issue (type cast) is non-blocking. |
✅ Tests passed — 1106/1110
|
Greptile SummaryThis PR completely removes the user-facing Skills feature from both the agent frontend and the server backend, including routes, UI navigation, prompt injection, startup sync/migration, bundled default skills, constants, and all associated tests. ACPX runtime skills are intentionally preserved as they are a separate harness mechanism.
Confidence Score: 5/5Safe to merge — this is a clean end-to-end deletion with no partial removals or dangling references in the retained code paths. All skills-related surface area is consistently removed across the frontend, server, constants, and tests. The ACPX runtime skills are intentionally left untouched per the stated design. No cross-cutting references to the deleted modules remain in active code, the gray-matter dependency is cleanly dropped, and a regression test confirms the prompt builder never injects a skills catalog even if the property is passed via a type cast. No files require special attention. The deletion is thorough and consistent. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Before["Before - Removed"]
A[Agent Run] --> B[loadSkills]
B --> C[buildSkillsCatalog]
C --> D[buildSystemPrompt with skillsCatalog]
E[Server Startup] --> F[migrateBuiltinSkills]
F --> G[syncBuiltinSkills]
G --> H[startSkillSync interval]
I[HTTP Server] --> J["'/skills' route"]
K[Agent UI] --> L["'/home/skills' page"]
L --> M[Feature.SKILLS_SUPPORT gate]
end
subgraph After["After - Current"]
A2[Agent Run] --> D2[buildSystemPrompt no skills]
E2[Server Startup] --> EnsureDir[ensureBrowserosDir no skills dirs]
I2[HTTP Server] --> NoSkills[No skills route]
K2[Agent UI] --> NoPage[No skills navigation]
ACPX[ACPX Runtime Skills] --> Preserved[Kept intact]
end
Reviews (1): Last reviewed commit: "chore: unship skills feature" | Re-trigger Greptile |
1 participant
Summary
Design
This unships Skills by deleting the product-facing feature end to end rather than hiding it behind another gate. The agent no longer advertises
/home/skills, the server no longer exposes/skillsor injects a skills catalog into prompts, and the bundled defaults are removed so the feature can be restored later by reverting this single commit.Test plan
git diff --checkbun run --filter @browseros/server typecheckbun run --filter @browseros/agent typecheckbun run --filter @browseros/shared typecheckbun run --filter @browseros/server test:core