Add always-on skills for new tasks - #3735
Closed
adboio wants to merge 12 commits into
Closed
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
React Doctor found 2 issues in 1 file · 1 error & 1 warning. Errors
1 warning
Reviewed by React Doctor for commit |
Contributor
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
Contributor
🦔 ReviewHog reviewed this pull requestFound 2 must fix, 3 should fix, 2 consider. Published 7 findings (view the review). |
Contributor
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 4 issues
Files (20)
packages/agent/src/server/agent-server.tspackages/core/src/sessions/cloudArtifactIdentifiers.tspackages/core/src/sessions/cloudArtifactService.tspackages/core/src/sessions/sessionService.tspackages/core/src/task-detail/taskCreationSaga.tspackages/core/src/task-detail/taskInput.tspackages/host-router/src/routers/skills.router.tspackages/shared/src/index.tspackages/shared/src/sessions.tspackages/shared/src/task-creation-domain.tspackages/shared/src/task.tspackages/ui/src/features/settings/settingsStore.tspackages/ui/src/features/skills/SkillCard.tsxpackages/ui/src/features/skills/SkillDetailPanel.tsxpackages/ui/src/features/task-detail/components/AlwaysOnSkillsFailureDialog.tsxpackages/ui/src/features/task-detail/hooks/useTaskCreation.tspackages/ui/src/features/task-detail/stores/alwaysOnSkillsFailureStore.tspackages/ui/src/router/routes/__root.tsxpackages/workspace-server/src/services/skills/schemas.tspackages/workspace-server/src/services/skills/skills.ts
2 tasks
|
This PR has had no activity for 7 days and has been marked stale. We are moving to the monorepo and tightening PR staleness in preparation, so it will be closed in 7 days if no further activity occurs. |
adboio
force-pushed
the
posthog-code/always-on-skills
branch
from
July 30, 2026 18:54
dd72940 to
24df19f
Compare
Contributor
There was a problem hiding this comment.
ReviewHog Report
Business logic
Issues: 2 issues
Files (10)
packages/shared/src/task-creation-domain.tspackages/shared/src/index.tspackages/shared/src/domain-types.tspackages/core/src/sessions/cloudArtifactIdentifiers.tspackages/core/src/sessions/cloudArtifactService.tspackages/core/src/task-detail/taskCreationSaga.tspackages/core/src/task-detail/taskInput.tspackages/core/src/sessions/sessionService.tspackages/ui/src/features/sessions/sessionConfigStore.tspackages/ui/src/features/sessions/sessionServiceHost.ts
What were the main changes
- Add shared AlwaysOnSkillRef type and always_on artifact metadata field, exported from @posthog/shared
- taskCreationSaga.addAlwaysOnSkills merges/dedupes always-on skills into the cloud prompt transport's skillBundles and into local connectParams
- cloudArtifactService/cloudArtifactIdentifiers mark uploaded skill bundle artifacts with always_on: true
- taskInput threads alwaysOnSkills through TaskCreationInput for both local and cloud paths
- sessionService persists/reconnects alwaysOnSkills and falls back to previous session's alwaysOnSkillInstructions on reconnect (flagged: this fallback is lost after an app restart since sessionStore is not persisted)
- New sessionConfigStore persistence (getPersisted/setPersisted/removePersistedAlwaysOnSkills) wired into sessionServiceHost as the always-on skill store backing sessionService
Business logic
Issues: 1 issue
Files (5)
packages/agent/src/server/agent-server.tspackages/workspace-server/src/services/agent/agent.tspackages/workspace-server/src/services/agent/schemas.tspackages/workspace-server/src/services/skills/schemas.tspackages/workspace-server/src/services/skills/skills.ts
What were the main changes
- agent-server.ts: builds an always-on skill prompt block from skill_bundle artifacts with metadata.always_on, sorted canonically by source/name, and merges it into invoked/attached skill contexts (flagged: can duplicate skill content when the same skill is also invoked/mentioned)
- workspace-server agent.ts: injects skillsService.renderAlwaysOnSkillInstructions output into buildSystemPrompt and logs per-skill render failures
- skills.ts: new renderAlwaysOnSkillInstructions resolves each skill ref's SKILL.md, sorted canonically, returning partial per-skill failures instead of failing the whole batch (addresses earlier all-or-nothing Promise.all issue)
- Zod schema updates (agent/schemas.ts, skills/schemas.ts) add alwaysOnSkills/alwaysOn fields to session-connect and skill-bundle-ref payloads
Frontend
Issues: 4 issues
Files (7)
packages/ui/src/features/settings/settingsStore.tspackages/ui/src/features/skills/SkillCard.tsxpackages/ui/src/features/skills/SkillDetailPanel.tsxpackages/ui/src/features/task-detail/components/AlwaysOnSkillChips.tsxpackages/ui/src/features/task-detail/components/TaskInput.tsxpackages/ui/src/features/canvas/components/ChannelHomeComposer.tsxpackages/ui/src/features/task-detail/hooks/useTaskCreation.ts
What were the main changes
- settingsStore: new persisted alwaysOnSkills preference list plus setSkillAlwaysOn toggle action
- SkillCard/SkillDetailPanel: 'Always on for new tasks' badge and toggle switch in the skills UI (disabled for bundled skills)
- New AlwaysOnSkillChips component and useAlwaysOnSkillSelection hook to display and per-task exclude always-on skills
- TaskInput and ChannelHomeComposer render the always-on skill chips row and reset per-task exclusions after successful task creation
- useTaskCreation resolves the effective always-on skill list (minus excluded ones) via hostClient.skills.renderAlwaysOn with retry/cancel/disable recovery on failure (flagged: failure/recovery is all-or-nothing across the whole skill set rather than per-skill)
Comment on lines
343
to
+346
| onPendingStart({ id, prompt }); | ||
|
|
||
| const created = await handleSubmit(content); | ||
| if (created) resetAlwaysOnSkillSelection(); |
Contributor
There was a problem hiding this comment.
resetAlwaysOnSkillSelection() on submit success clears ALL pending exclusions, racing with exclusions the user makes for their next (not-yet-submitted) task
Why we think it's a valid issue
- Checked: the actual PR-head code (checked out
9934eea1, since the local working tree was stale — it did not contain the always-on changes at all). ReadChannelHomeComposer.tsx:331-361(thesubmitcallback),AlwaysOnSkillChips.tsx:9-25(useAlwaysOnSkillSelection),useTaskCreation.ts:330-333(where the excluded-keys snapshot filtersalwaysOnSkills), plus the parallelTaskInput.tsx:977-984and its chip render. - Found:
ChannelHomeComposer.tsx:346callsresetAlwaysOnSkillSelection()on any success, andreset()(AlwaysOnSkillChips.tsx:22) empties the wholeexcludedKeysset — not just the submitted task's keys. The chip exclude buttons (AlwaysOnSkillChips.tsx:58-67) take nodisabledprop and the row (ChannelHomeComposer.tsx:476-484) is gated only on!canvasArmed && includedAlwaysOnSkills.length > 0, never onisBusy, so they remain clickable mid-submit. Task 1's own exclusion snapshot is correctly captured via theexcludedAlwaysOnSkillKeysclosure dep (useTaskCreation.ts:330-333); the defect is solely thatreset()also discards a key added after Task 1 started. Same shape confirmed atTaskInput.tsx:980. - Impact: the race is genuine and reproducible, but narrow and low-impact. Submits are serialized (
canSubmitrequires!isCreatingTask,useTaskCreation.ts:250), so the user cannot submit the next task during the window; they must (a) exclude a different skill for a future task within the briefcreateTaskin-flight window, then (b) submit later without noticing that the excluded skill's chip visibly reappeared in the 'Using:' row when reset fired. The consequence is transient UI-state (an always-on skill re-included on a task), self-signalling via the reappearing chip, with no persistent-data loss, corruption, or security exposure. - Priority: downgrading
should_fix->consider. Real, nameable trigger and consequence, but the interaction is uncommon, the timing window small, and the effect visible and recoverable — worth keeping on record, below the bar forshould_fix.
Issue description
resetAlwaysOnSkillSelection() is called unconditionally whenever a submit succeeds (if (created) resetAlwaysOnSkillSelection();), clearing the entire excludedKeys set. But this composer explicitly supports overlapping/back-to-back submissions (pendingIdsRef's own comment: 'no row is ever orphaned, even if two creates briefly overlap'), and the 'Using:' chip row with its exclude buttons is not disabled while a previous submission is still in flight. Sequence: user excludes Skill A and submits Task 1 (async, editor already cleared); while Task 1 is still awaiting handleSubmit, the user excludes Skill B intending to leave it out of their next task; Task 1 then resolves successfully and resetAlwaysOnSkillSelection() wipes the entire exclusion set — including the Skill B exclusion the user just made for their upcoming task, which was never actually part of Task 1. The next submission silently includes Skill B despite the user having just excluded it.
Suggested fix
Only clear the exclusions that were actually part of the submission that just succeeded (e.g. reset to the complement of the excluded-keys snapshot taken at that submit's call time), rather than clearing the composer's whole current exclusion state after any success. The same pattern applies to packages/ui/src/features/task-detail/components/TaskInput.tsx lines 977-985.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/canvas/components/ChannelHomeComposer.tsx#L343-346
<issue_description>
`resetAlwaysOnSkillSelection()` is called unconditionally whenever a submit succeeds (`if (created) resetAlwaysOnSkillSelection();`), clearing the entire `excludedKeys` set. But this composer explicitly supports overlapping/back-to-back submissions (`pendingIdsRef`'s own comment: 'no row is ever orphaned, even if two creates briefly overlap'), and the 'Using:' chip row with its exclude buttons is not disabled while a previous submission is still in flight. Sequence: user excludes Skill A and submits Task 1 (async, editor already cleared); while Task 1 is still awaiting `handleSubmit`, the user excludes Skill B intending to leave it out of their next task; Task 1 then resolves successfully and `resetAlwaysOnSkillSelection()` wipes the entire exclusion set — including the Skill B exclusion the user just made for their upcoming task, which was never actually part of Task 1. The next submission silently includes Skill B despite the user having just excluded it.
</issue_description>
<issue_validation>
- **Checked:** the actual PR-head code (checked out `9934eea1`, since the local working tree was stale — it did not contain the always-on changes at all). Read `ChannelHomeComposer.tsx:331-361` (the `submit` callback), `AlwaysOnSkillChips.tsx:9-25` (`useAlwaysOnSkillSelection`), `useTaskCreation.ts:330-333` (where the excluded-keys snapshot filters `alwaysOnSkills`), plus the parallel `TaskInput.tsx:977-984` and its chip render.
- **Found:** `ChannelHomeComposer.tsx:346` calls `resetAlwaysOnSkillSelection()` on any success, and `reset()` (`AlwaysOnSkillChips.tsx:22`) empties the whole `excludedKeys` set — not just the submitted task's keys. The chip exclude buttons (`AlwaysOnSkillChips.tsx:58-67`) take no `disabled` prop and the row (`ChannelHomeComposer.tsx:476-484`) is gated only on `!canvasArmed && includedAlwaysOnSkills.length > 0`, never on `isBusy`, so they remain clickable mid-submit. Task 1's own exclusion snapshot is correctly captured via the `excludedAlwaysOnSkillKeys` closure dep (`useTaskCreation.ts:330-333`); the defect is solely that `reset()` also discards a key added *after* Task 1 started. Same shape confirmed at `TaskInput.tsx:980`.
- **Impact:** the race is genuine and reproducible, but narrow and low-impact. Submits are serialized (`canSubmit` requires `!isCreatingTask`, `useTaskCreation.ts:250`), so the user cannot submit the next task during the window; they must (a) exclude a *different* skill for a future task within the brief `createTask` in-flight window, then (b) submit later without noticing that the excluded skill's chip visibly reappeared in the 'Using:' row when reset fired. The consequence is transient UI-state (an always-on skill re-included on a task), self-signalling via the reappearing chip, with no persistent-data loss, corruption, or security exposure.
- **Priority:** downgrading `should_fix` -> `consider`. Real, nameable trigger and consequence, but the interaction is uncommon, the timing window small, and the effect visible and recoverable — worth keeping on record, below the bar for `should_fix`.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Only clear the exclusions that were actually part of the submission that just succeeded (e.g. reset to the complement of the excluded-keys snapshot taken at that submit's call time), rather than clearing the composer's whole current exclusion state after any success. The same pattern applies to `packages/ui/src/features/task-detail/components/TaskInput.tsx` lines 977-985.
</potential_solution>
Comment on lines
+9
to
+24
| export function useAlwaysOnSkillSelection() { | ||
| const alwaysOnSkills = useSettingsStore((state) => state.alwaysOnSkills); | ||
| const [excludedKeys, setExcludedKeys] = useState(() => new Set<string>()); | ||
| const includedSkills = alwaysOnSkills.filter( | ||
| (skill) => !excludedKeys.has(`${skill.source}:${skill.path}`), | ||
| ); | ||
| const exclude = useCallback((skill: AlwaysOnSkillRef) => { | ||
| setExcludedKeys((current) => { | ||
| const next = new Set(current); | ||
| next.add(`${skill.source}:${skill.path}`); | ||
| return next; | ||
| }); | ||
| }, []); | ||
| const reset = useCallback(() => setExcludedKeys(new Set()), []); | ||
|
|
||
| return { includedSkills, excludedKeys, exclude, reset }; |
Contributor
There was a problem hiding this comment.
Always-on skill exclusion state is not scoped per channel/session, so exclusions leak across channel switches
Why we think it's a valid issue
- Checked: whether the components that hold
useAlwaysOnSkillSelection'suseStateremount on a channel switch. Traced the channel composer render chain (router/routes/website/$channelId/index.tsx->WebsiteChannelHome.tsx:307->ChannelHomeComposer), searched for anykey/remountDeps, and compared the un-guardedexcludedAlwaysOnSkillKeysagainst the adjacentchannelContextDismissedguard inTaskInput.tsx. - Found (instance is reused, not remounted):
createFileRoute("/website/$channelId/")renders<WebsiteChannelHome channelId={channelId}/>($channelId/index.tsx:8-16), which renders<ChannelHomeComposer channelId={channelId} ...>(WebsiteChannelHome.tsx:307-309) — neither has akeyorremountDeps, so a param-only nav/website/A/->/website/B/preserves the component instance and itsuseState. Corroborated by surrounding code written for that reuse:WebsiteChannelHomekeeps onependingarray filtered bychannelId(:112-124) andopenByChannel[channelId](:133), andChannelHomeComposerderivessessionId = channel-home:${channelId}(:93) to key the external draft store. - Found (leaking state lacks scoping):
excludedKeysis plain component-localuseState(() => new Set<string>())(AlwaysOnSkillChips.tsx:11), cleared only byreset(), which runs solely on a successful submit (ChannelHomeComposer.tsx:346,TaskInput.tsx:980). No channel/session-keyed reset. The exact analogous guard already exists one block above the hook call —TaskInput.tsx:305-311useslastChannelContextRef+ an effect to resetchannelContextDismissedonchannelContextchange ('so a dismissal doesn't stick across channels') — whileexcludedAlwaysOnSkillKeys(:299-304) gets none. - Impact: excluding an always-on skill while composing in Channel A, then navigating to Channel B without submitting, carries the exclusion into Channel B (
excludedAlwaysOnSkillKeysstill holds the key), silently dropping the skill from Channel B's task with only a missing 'Using:' chip as a cue. Reachable via a common flow (exclude -> switch channel -> submit) and it defeats the feature's goal of applying opted-in skills to every task — a genuine per-view state-scoping bug directly analogous to one the author already fixed for sibling state.
Issue description
useAlwaysOnSkillSelection keeps excludedKeys in a plain useState(() => new Set<string>()) that only clears via reset(), called solely on a successful task submit. Both consumers of this hook — ChannelHomeComposer.tsx (lines ~284-289) and TaskInput.tsx (lines ~299-304) — are components that are NOT remounted when the surrounding channel/context changes. This is directly evidenced in the same files: ChannelHomeComposer.tsx derives sessionId = channel-home:${channelId} specifically because the component instance is reused across channels (its route, /website/$channelId/, does not remount on a TanStack Router param-only navigation), and TaskInput.tsx already has an explicit lastChannelContextRef + useEffect that resets channelContextDismissed 'whenever the source context changes (e.g. switching channels) so a dismissal doesn't stick across channels' (lines 291-301) — i.e. this exact class of bug was already recognized and fixed for a conceptually identical piece of per-task-view state, but the same fix was not applied to the new excludedAlwaysOnSkillKeys state. Concretely: a user excludes 'Deploy Runbook' while composing in Channel A, navigates to Channel B without submitting, and the skill stays silently excluded for Channel B's task too, with no visual indication other than it missing from the 'Using:' chip row.
Suggested fix
Scope excludedKeys to the current channel/session the same way channelContextDismissed is scoped — e.g. add a useEffect that resets exclusions whenever channelId/channelContext/sessionId changes, or key the hook's internal state off that identifier (e.g. reset when it differs from a ref, mirroring the lastChannelContextRef pattern already in TaskInput.tsx).
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/task-detail/components/AlwaysOnSkillChips.tsx#L9-24
<issue_description>
`useAlwaysOnSkillSelection` keeps `excludedKeys` in a plain `useState(() => new Set<string>())` that only clears via `reset()`, called solely on a successful task submit. Both consumers of this hook — `ChannelHomeComposer.tsx` (lines ~284-289) and `TaskInput.tsx` (lines ~299-304) — are components that are NOT remounted when the surrounding channel/context changes. This is directly evidenced in the same files: `ChannelHomeComposer.tsx` derives `sessionId = channel-home:${channelId}` specifically because the component instance is reused across channels (its route, `/website/$channelId/`, does not remount on a TanStack Router param-only navigation), and `TaskInput.tsx` already has an explicit `lastChannelContextRef` + `useEffect` that resets `channelContextDismissed` 'whenever the source context changes (e.g. switching channels) so a dismissal doesn't stick across channels' (lines 291-301) — i.e. this exact class of bug was already recognized and fixed for a conceptually identical piece of per-task-view state, but the same fix was not applied to the new `excludedAlwaysOnSkillKeys` state. Concretely: a user excludes 'Deploy Runbook' while composing in Channel A, navigates to Channel B without submitting, and the skill stays silently excluded for Channel B's task too, with no visual indication other than it missing from the 'Using:' chip row.
</issue_description>
<issue_validation>
- **Checked:** whether the components that hold `useAlwaysOnSkillSelection`'s `useState` remount on a channel switch. Traced the channel composer render chain (`router/routes/website/$channelId/index.tsx` -> `WebsiteChannelHome.tsx:307` -> `ChannelHomeComposer`), searched for any `key`/`remountDeps`, and compared the un-guarded `excludedAlwaysOnSkillKeys` against the adjacent `channelContextDismissed` guard in `TaskInput.tsx`.
- **Found (instance is reused, not remounted):** `createFileRoute("/website/$channelId/")` renders `<WebsiteChannelHome channelId={channelId}/>` (`$channelId/index.tsx:8-16`), which renders `<ChannelHomeComposer channelId={channelId} ...>` (`WebsiteChannelHome.tsx:307-309`) — neither has a `key` or `remountDeps`, so a param-only nav `/website/A/` -> `/website/B/` preserves the component instance and its `useState`. Corroborated by surrounding code written for that reuse: `WebsiteChannelHome` keeps one `pending` array filtered by `channelId` (`:112-124`) and `openByChannel[channelId]` (`:133`), and `ChannelHomeComposer` derives `sessionId = channel-home:${channelId}` (`:93`) to key the external draft store.
- **Found (leaking state lacks scoping):** `excludedKeys` is plain component-local `useState(() => new Set<string>())` (`AlwaysOnSkillChips.tsx:11`), cleared only by `reset()`, which runs solely on a successful submit (`ChannelHomeComposer.tsx:346`, `TaskInput.tsx:980`). No channel/session-keyed reset. The exact analogous guard already exists one block above the hook call — `TaskInput.tsx:305-311` uses `lastChannelContextRef` + an effect to reset `channelContextDismissed` on `channelContext` change ('so a dismissal doesn't stick across channels') — while `excludedAlwaysOnSkillKeys` (`:299-304`) gets none.
- **Impact:** excluding an always-on skill while composing in Channel A, then navigating to Channel B without submitting, carries the exclusion into Channel B (`excludedAlwaysOnSkillKeys` still holds the key), silently dropping the skill from Channel B's task with only a missing 'Using:' chip as a cue. Reachable via a common flow (exclude -> switch channel -> submit) and it defeats the feature's goal of applying opted-in skills to every task — a genuine per-view state-scoping bug directly analogous to one the author already fixed for sibling state.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Scope `excludedKeys` to the current channel/session the same way `channelContextDismissed` is scoped — e.g. add a `useEffect` that resets exclusions whenever `channelId`/`channelContext`/`sessionId` changes, or key the hook's internal state off that identifier (e.g. reset when it differs from a ref, mirroring the `lastChannelContextRef` pattern already in `TaskInput.tsx`).
</potential_solution>
Comment on lines
+34
to
+41
| const { requestSkill } = useSkillsSelectionActions(); | ||
| const openSkill = useCallback( | ||
| (name: string) => { | ||
| requestSkill(name); | ||
| openSettings("skills"); | ||
| }, | ||
| [requestSkill], | ||
| ); |
Contributor
There was a problem hiding this comment.
The always-on skill chip's 'View' action can open the wrong skill's detail panel when two skills share the same name
Why we think it's a valid issue
- Checked: the full 'View' path and whether the name match is disambiguated —
AlwaysOnSkillChips.tsx:35-51(openSkill(skill.name)->requestSkill(skill.name)),skillsSelectionStore.ts:24(stores barerequestedSkillName),SkillsView.tsx:77-85(resolution),skills.ts:4-14(SkillInfo.name), plus how always-on actually resolves skills (settingsStorematch on source+path; server render byref.pathatskills.ts:525). - Found (premise holds): the chip's only jump-to-skill affordance passes name only;
SkillsView.tsx:79resolves withskills.find((s) => s.name === requestedSkillName)— first name match, no source/path tiebreak — then selectsmatch.path.SkillInfo.name(skills.ts:5) is unconstrained frontmatter, so two skills sharing a name (e.g. a forked/similarly-named repo vs bundled skill) can collide and the wrong panel can open. Toggling in the opened panel mutates the displayed skill's preference (SkillDetailPanel.tsx:330-338), so the escalation is technically reachable. - Impact: the collision affects only a convenience navigation link. Which skill's instructions actually apply to a task is keyed by
source+path(settingsStore filter and server render byref.path), so a name collision never changes what runs — the feature stays correct. The confirmed consequence is a wrong detail panel opening under an uncommon precondition, and the panel plainly shows source badge/description/SKILL.md, making the mismatch self-evident; the 'silently toggles a different skill' escalation further requires the user to ignore that displayed identity and toggle a switch that visibly reflects the wrong skill's state. Rare precondition + self-evident + no effect on skill application = low-severity edge case. - Priority: downgrading
should_fix->consider. Premise verified and the PR does introduce a new higher-stakes caller of the ambiguous by-name mechanism, so keep it on record; but a minor, self-evident navigation quirk that never changes which skill is applied does not warrant surfacing atshould_fix.
Issue description
openSkill (lines 34-41) calls requestSkill(skill.name) — passing only the free-text name, not source/path — and the button's onClick={() => openSkill(skill.name)} (line 51) is the only way this PR lets a user jump from an always-on chip to its skill. requestSkill just stores that bare string (packages/ui/src/features/skills/skillsSelectionStore.ts:24, requestedSkillName: name), and the consumer, SkillsView.tsx:79, resolves it with skills.find((s) => s.name === requestedSkillName) — a name-only match with no source/path disambiguation, taking whichever skill happens to come first in the array. SkillInfo.name (packages/shared/src/skills.ts:5) is arbitrary frontmatter text with no uniqueness constraint enforced anywhere, across sources (bundled/user/repo/marketplace/codex) or even within one source, so two independently-authored skills sharing a name (e.g. a bundled 'Deploy Runbook' and a user- or repo-authored one with the same title) will collide. Concretely: a user has a repo skill named 'Deploy Runbook' marked always-on; a different, unrelated bundled or user skill is also named 'Deploy Runbook'; clicking 'View Deploy Runbook' on the always-on chip opens whichever of the two skills.find happens to hit first — which may not be the one that's actually configured as always-on, showing the wrong SKILL.md content and, if the user then toggles its switch believing it's the one they clicked from the chip, silently changing a completely different skill's always-on preference instead of (or in addition to) the intended one. This is a new failure mode introduced by this PR's reuse of the pre-existing by-name requestSkill mechanism (previously only used for scout links) for a new, higher-stakes always-on toggle action.
Suggested fix
Extend requestSkill/requestedSkillName (or add a sibling field) to carry source and path alongside name, and have SkillsView.tsx's resolution effect match on source+path first (falling back to name only for legacy by-name callers like the scout links), so the always-on chip always opens the exact skill instance it represents.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/task-detail/components/AlwaysOnSkillChips.tsx#L34-41
@packages/ui/src/features/task-detail/components/AlwaysOnSkillChips.tsx#L48-51
<issue_description>
`openSkill` (lines 34-41) calls `requestSkill(skill.name)` — passing only the free-text `name`, not `source`/`path` — and the button's `onClick={() => openSkill(skill.name)}` (line 51) is the only way this PR lets a user jump from an always-on chip to its skill. `requestSkill` just stores that bare string (`packages/ui/src/features/skills/skillsSelectionStore.ts:24`, `requestedSkillName: name`), and the consumer, `SkillsView.tsx:79`, resolves it with `skills.find((s) => s.name === requestedSkillName)` — a name-only match with no source/path disambiguation, taking whichever skill happens to come first in the array. `SkillInfo.name` (packages/shared/src/skills.ts:5) is arbitrary frontmatter text with no uniqueness constraint enforced anywhere, across sources (bundled/user/repo/marketplace/codex) or even within one source, so two independently-authored skills sharing a name (e.g. a bundled 'Deploy Runbook' and a user- or repo-authored one with the same title) will collide. Concretely: a user has a repo skill named 'Deploy Runbook' marked always-on; a different, unrelated bundled or user skill is also named 'Deploy Runbook'; clicking 'View Deploy Runbook' on the always-on chip opens whichever of the two `skills.find` happens to hit first — which may not be the one that's actually configured as always-on, showing the wrong SKILL.md content and, if the user then toggles its switch believing it's the one they clicked from the chip, silently changing a completely different skill's always-on preference instead of (or in addition to) the intended one. This is a new failure mode introduced by this PR's reuse of the pre-existing by-name `requestSkill` mechanism (previously only used for scout links) for a new, higher-stakes always-on toggle action.
</issue_description>
<issue_validation>
- **Checked:** the full 'View' path and whether the name match is disambiguated — `AlwaysOnSkillChips.tsx:35-51` (`openSkill(skill.name)` -> `requestSkill(skill.name)`), `skillsSelectionStore.ts:24` (stores bare `requestedSkillName`), `SkillsView.tsx:77-85` (resolution), `skills.ts:4-14` (`SkillInfo.name`), plus how always-on actually resolves skills (`settingsStore` match on source+path; server render by `ref.path` at `skills.ts:525`).
- **Found (premise holds):** the chip's only jump-to-skill affordance passes name only; `SkillsView.tsx:79` resolves with `skills.find((s) => s.name === requestedSkillName)` — first name match, no source/path tiebreak — then selects `match.path`. `SkillInfo.name` (`skills.ts:5`) is unconstrained frontmatter, so two skills sharing a name (e.g. a forked/similarly-named repo vs bundled skill) can collide and the wrong panel can open. Toggling in the opened panel mutates the *displayed* skill's preference (`SkillDetailPanel.tsx:330-338`), so the escalation is technically reachable.
- **Impact:** the collision affects only a convenience *navigation* link. Which skill's instructions actually apply to a task is keyed by `source`+`path` (settingsStore filter and server render by `ref.path`), so a name collision never changes what runs — the feature stays correct. The confirmed consequence is a wrong detail panel opening under an uncommon precondition, and the panel plainly shows source badge/description/SKILL.md, making the mismatch self-evident; the 'silently toggles a different skill' escalation further requires the user to ignore that displayed identity and toggle a switch that visibly reflects the wrong skill's state. Rare precondition + self-evident + no effect on skill application = low-severity edge case.
- **Priority:** downgrading `should_fix` -> `consider`. Premise verified and the PR does introduce a new higher-stakes caller of the ambiguous by-name mechanism, so keep it on record; but a minor, self-evident navigation quirk that never changes which skill is applied does not warrant surfacing at `should_fix`.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Extend `requestSkill`/`requestedSkillName` (or add a sibling field) to carry `source` and `path` alongside `name`, and have `SkillsView.tsx`'s resolution effect match on `source`+`path` first (falling back to `name` only for legacy by-name callers like the scout links), so the always-on chip always opens the exact skill instance it represents.
</potential_solution>
Generated-By: PostHog Code Task-Id: 73390913-7191-4450-9fd3-31dd12879508
Generated-By: PostHog Code Task-Id: 73390913-7191-4450-9fd3-31dd12879508
Generated-By: PostHog Code Task-Id: 73390913-7191-4450-9fd3-31dd12879508
Generated-By: PostHog Code Task-Id: 73390913-7191-4450-9fd3-31dd12879508
Generated-By: PostHog Code Task-Id: b0e948e2-b733-4d51-be6a-efd68d54216e
Generated-By: PostHog Code Task-Id: b0e948e2-b733-4d51-be6a-efd68d54216e
Generated-By: PostHog Code Task-Id: b0e948e2-b733-4d51-be6a-efd68d54216e
Generated-By: PostHog Code Task-Id: b0e948e2-b733-4d51-be6a-efd68d54216e
Generated-By: PostHog Code Task-Id: b0e948e2-b733-4d51-be6a-efd68d54216e
Generated-By: PostHog Code Task-Id: b0e948e2-b733-4d51-be6a-efd68d54216e
Generated-By: PostHog Code Task-Id: b0e948e2-b733-4d51-be6a-efd68d54216e
Generated-By: PostHog Code Task-Id: b0e948e2-b733-4d51-be6a-efd68d54216e
adboio
force-pushed
the
posthog-code/always-on-skills
branch
from
July 31, 2026 14:29
dafdf66 to
b11c98d
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| skill, | ||
| ]) ?? [], | ||
| ); | ||
| const normalizedRepoPath = target.repoPath?.replace(/[\\/]+$/, ""); |
2 tasks
2 tasks
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
2 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.
Problem
People can configure a skill locally, but Code-originated cloud tasks cannot rely on that local filesystem. This makes instructions that should apply to every new task inconsistent between local and cloud execution.
Changes
always_on: true; existing explicit skills and dependencies keep their existing behavior.How did you test this?
pnpm typecheckgit commitcommands before reaching the test body.Automatic notifications
Created with PostHog Code