From 90eb60bf7501e128b874f64ce2e37af2798c16d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Gonz=C3=A1lez?= Date: Mon, 13 Jul 2026 16:07:46 +0200 Subject: [PATCH 1/4] feat(skills): add atomic design skill Provide atomic design guidance with type-filtered slot composition and a reusable slot preparation helper. --- skills/atomic-design/SKILL.md | 131 +++++++++++++++ .../references/atomic-design-methodology.md | 71 ++++++++ .../references/prepareComponentSlots.ts | 110 +++++++++++++ .../references/slot-based-composition.md | 154 ++++++++++++++++++ 4 files changed, 466 insertions(+) create mode 100644 skills/atomic-design/SKILL.md create mode 100644 skills/atomic-design/references/atomic-design-methodology.md create mode 100644 skills/atomic-design/references/prepareComponentSlots.ts create mode 100644 skills/atomic-design/references/slot-based-composition.md diff --git a/skills/atomic-design/SKILL.md b/skills/atomic-design/SKILL.md new file mode 100644 index 0000000..8d43860 --- /dev/null +++ b/skills/atomic-design/SKILL.md @@ -0,0 +1,131 @@ +--- +name: atomic-design +description: Atomic design methodology with type-filtered slot composition for UI implementation, validation, review, and audits. Use when implementing, validating, reviewing, or auditing frontend/UI code through atomic design principles, component hierarchy, atoms, molecules, organisms, templates, pages, reusable design systems, React-style compound components, named slots, child filtering by type, fixed slot positioning, Storybook or component state coverage, content structure, and page variation resilience. +--- + +# Atomic Design + +Apply atomic design as a UI design-system mental model, not as a rigid build sequence. Organize components by logical responsibility using atomic design, then compose their variable regions through slots. + +Read [Atomic Design Methodology](references/atomic-design-methodology.md) when you need the stage taxonomy, React-oriented placement rules, source notes, or detailed audit prompts. + +Read [Slot-Based Composition](references/slot-based-composition.md) when you need compound component exports, child filtering by type, fixed slot positioning, or review prompts for slot composition. + +## Workflow + +1. Inspect the existing UI architecture before introducing atomic vocabulary. +2. Preserve the repo's naming, routing, styling, testing, and component patterns unless the user explicitly asks for a reorganization. +3. Map the target code to the smallest atomic stage that can own the responsibility. +4. Expose variable regions as slots when a component has stable structure but flexible content. +5. Check the same component in isolation and in its composed page context. +6. Validate with the repo's normal commands and with rendered UI inspection when visual behavior matters. + +## Stage Ownership + +| Stage | Owns | Avoid | +| -------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------- | +| Atom | Primitive UI elements, design tokens in use, accessible base controls, typographic primitives, icons, inputs, buttons. | Page-specific layout, margins, route data, application workflows. | +| Molecule | Small functional groups of atoms, such as search fields, form rows, nav items, summary badges, or card headers. | Full page sections, global data fetching, unrelated optional regions. | +| Organism | Distinct interface sections composed of atoms, molecules, or other organisms, such as headers, product grids, checkout forms, sidebars, and feature panels. | Route ownership, full-page layout skeletons, hard-coded page-only content. | +| Template | Page-level layout and content structure, including grid regions, slots, skeletons, and constraints for dynamic content. | Final production copy, business-specific records, route side effects. | +| Page | Real representative content, route integration, application state wiring, user-role or data-volume variations, and final resilience checks. | Reusable component internals that belong lower in the hierarchy. | + +## Implementation Rules + +- Prefer the existing component taxonomy. Introduce `atoms`, `molecules`, `organisms`, `templates`, and `pages` folders only when that matches or improves the local system. +- Keep dependencies flowing upward through the hierarchy: atoms should not import molecules, molecules may import atoms, organisms may import molecules and atoms, templates may import organisms and lower stages, and pages may wire all stages together. +- Keep atoms portable. They can expose variants and states, but they must not assume where they sit on a page. +- Compose molecules from atoms to create one focused function. Split a molecule when independent concerns start sharing props, state, or styles. +- Compose organisms as reusable interface sections. They may coordinate child layout but should stay independent enough to work in multiple page contexts. +- Keep templates about structure. They arrange regions and define content constraints without binding final records, permissions, or navigation behavior. +- Use pages to connect templates and components to real app data, routing, representative content, and meaningful variations. +- Centralize design tokens or variables according to the repo's existing style system. Do not duplicate token values inside components. +- Cover meaningful states where the repo supports component examples, stories, screenshots, or interaction tests. + +## Slot-Based Composition Rules + +- Use slots when props like `headerContent`, `footerActions`, `leftIcon`, or `descriptionNode` begin to multiply or when callers need to provide real JSX while the component owns structure. +- Prefer semantic compound slots such as `Card.Header`, `Card.Body`, `Card.Footer`, `Dialog.Title`, or `Toolbar.Action` for reusable molecules and organisms. +- Colocate slot components inside the owning component folder and export them from the root component API. +- Put generic child filtering in a shared `prepareComponentSlots` helper, then wrap it in a colocated hook named like `useSlots`. +- Let slot components import lower-stage components when needed. For example, a molecule slot may import an atom, but an atom slot must not import a molecule. +- Define slot components as function components. Type filtering compares `child.type` with the function reference registered in the slot map. +- Keep slot names semantic, not incidental. Prefer `Title`, `Description`, `Actions`, `Media`, and `Footer` over `Top`, `Left`, or `BlueArea` unless the component is explicitly a layout primitive. +- Filter slot children by component type when the parent owns positioning. The consumer may write slots in a readable order, but the parent renders each recognized slot into its defined region. +- Preserve DOM reading order and accessibility. Do not visually reorder slots in a way that creates a different keyboard or screen-reader order. + +Example organization: + +```text +src/ + components/ + atoms/ + component-a/ + index.tsx + molecules/ + component-b/ + title.tsx + body.tsx + use-component-b-slots.ts + index.tsx +``` + +Example slot hook: + +```ts +import type { ReactNode } from "react"; + +import { prepareComponentSlots } from "../../hooks/prepare-component-slots.js"; +import { Body } from "./body.js"; +import { Title } from "./title.js"; + +const usePreparedComponentBSlots = prepareComponentSlots({ + body: [Body], + title: Title, +}); + +export function useComponentBSlots(children: ReactNode) { + return usePreparedComponentBSlots(children); +} +``` + +Example root export: + +```tsx +import type { ReactNode } from "react"; + +import { Body } from "./body.js"; +import { Title } from "./title.js"; +import { useComponentBSlots } from "./use-component-b-slots.js"; + +export interface ComponentBProps { + children: ReactNode; +} + +export function ComponentBRoot({ children }: ComponentBProps) { + const { body, title } = useComponentBSlots(children); + + return ( +
+ {title} +
{body}
+
+ ); +} + +export const ComponentB = Object.assign(ComponentBRoot, { + Body, + Title, +}); +``` + +## Review And Audit Rules + +- Report misplaced responsibilities as concrete code findings: atom with page layout, molecule doing route work, organism hard-coding one page, template owning production content, or page duplicating component internals. +- Check whether components remain reusable with different content lengths, empty states, disabled states, roles, and data volumes. +- Flag styling that breaks portability, especially margins and positioning buried in low-level atoms. +- Look for duplicate primitives or one-off components that should share an atom or molecule. +- Flag prop-heavy components that should expose named slots or compound subcomponents. +- Flag slots that break atomic dependency direction, hide business logic in reusable component internals, or require consumers to know private child ordering. +- Verify that templates expose content structure and that pages prove the structure with real representative content. +- Treat atomic design as a communication and resilience model. Do not require the exact stage names if the repo uses another clear taxonomy. diff --git a/skills/atomic-design/references/atomic-design-methodology.md b/skills/atomic-design/references/atomic-design-methodology.md new file mode 100644 index 0000000..556406e --- /dev/null +++ b/skills/atomic-design/references/atomic-design-methodology.md @@ -0,0 +1,71 @@ +# Atomic Design Methodology + +## Sources + +- [Atomic Design Methodology](https://atomicdesign.bradfrost.com/chapter-2/) by Brad Frost. +- [Atomic Design and ReactJS](https://danilowoz.com/blog/atomic-design-with-react) by Danilo Woznica. +- [danilowoz/react-atomic-design](https://github.com/danilowoz/react-atomic-design), used as a historical React/Storybook implementation example. + +## Core Model + +Atomic design breaks UI systems into five related stages: atoms, molecules, organisms, templates, and pages. Use the stages concurrently. The point is to move between abstract parts and concrete interfaces, not to build every atom before touching a page. + +Atomic design applies to user interfaces broadly. It is not a CSS architecture, JavaScript architecture, React requirement, or folder-name mandate. Use the local codebase's language when it is clearer, but preserve the hierarchy of responsibility. + +## Stage Guide + +| Stage | Definition | Good implementation signals | Audit failures | +| --------- | ----------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------- | +| Atoms | The smallest functional UI building blocks, such as labels, inputs, buttons, icons, images, typography, colors, and animation primitives. | Accessible defaults, explicit variants, token-driven styles, no knowledge of page placement. | Margins or positioning baked into controls, business records in props, inaccessible states, duplicate primitives. | +| Molecules | Simple groups of atoms working as one functional unit. A label, input, and button can become a search form molecule. | Single responsibility, portable behavior, small prop surface, states documented in isolation. | Too many unrelated options, duplicated atom behavior, direct route or global app coupling. | +| Organisms | More complex sections composed from molecules, atoms, or other organisms. Examples include headers, product grids, forms, and feature sections. | Distinct reusable interface section, clear slots or data contracts, resilient composition. | Hard-coded page copy, page-only layout assumptions, hidden dependencies on a single route. | +| Templates | Page-level layout objects that place components into structure and reveal content constraints. | Grid or slot ownership, skeleton states, character length and media constraints, placeholder or representative structure. | Production records, permissions, data fetching, route side effects, component internals. | +| Pages | Specific instances of templates with real representative content and app integration. | Route wiring, user roles, empty and loaded states, long and short content, real interaction paths. | Repeating lower-level markup, hiding content stress cases, only testing happy-path copy. | + +## React-Oriented Guidance + +Use these rules as architecture guidance, not as framework requirements: + +- Keep shared variables, tokens, and theme primitives centralized. +- Keep atoms free of page-specific margins and positioning. +- Let molecules and organisms compose and arrange their children, but keep their layout portable across contexts. +- Let templates define page grids, slots, and content structure. +- Let pages bind templates to real app state, routing, representative content, and variations. +- Use component examples or Storybook stories to show each meaningful state separately when the repo has that tooling. + +The source repository demonstrates a common structure: + +```text +src/components/ + _settings/ + atoms// + molecules// + organisms// + templates// +``` + +Each sample component colocates implementation, styles, and stories. Borrow the colocated-example idea when it fits the repo, but do not copy its older Flow, Yarn, CSS Modules, or Webpack choices unless the project already uses them. + +## Implementation Prompts + +Use these questions while building: + +- What is the smallest stage that should own this responsibility? +- Will the component work with different content, sizes, themes, and disabled or empty states? +- Is layout owned by the parent level rather than a low-level primitive? +- Does the template show the content structure without taking over page data? +- Does the page prove the design with real representative content and variations? +- Are component examples or tests covering the states users will actually encounter? + +## Review Prompts + +Use these checks while validating, reviewing, or auditing: + +- Are atoms reusable without page context? +- Are molecules focused on one functional grouping? +- Are organisms complete interface sections without owning the route? +- Are templates structural instead of content-specific? +- Are pages connecting real app state without duplicating lower-level component internals? +- Are content stress cases represented, including long text, missing media, empty lists, role differences, and varied item counts? +- Are style tokens reused instead of copied? +- Are states discoverable through tests, stories, screenshots, or examples available in the repo? diff --git a/skills/atomic-design/references/prepareComponentSlots.ts b/skills/atomic-design/references/prepareComponentSlots.ts new file mode 100644 index 0000000..d9c1cda --- /dev/null +++ b/skills/atomic-design/references/prepareComponentSlots.ts @@ -0,0 +1,110 @@ +import { Children, isValidElement, useMemo } from "react"; +import type { ReactElement, ReactNode } from "react"; + +export interface PrepareComponentSlotsOptions { + strict?: boolean; +} + +type SlotComponent = (props: Props) => ReactNode; + +type SlotDefinition = SlotComponent | readonly SlotComponent[]; +type SlotDefinitions = Record; + +type SlotProps = Component extends SlotComponent ? Props : never; + +type PreparedSlot = Definition extends readonly (infer Component)[] + ? Component extends SlotComponent + ? ReactElement>[] + : never + : Definition extends SlotComponent + ? ReactElement> | null + : never; + +type PreparedSlots = { + [Key in keyof Definitions]: PreparedSlot; +}; + +type SlotEntries = { + [Key in keyof Definitions]: [Key, Definitions[Key]]; +}[keyof Definitions][]; + +function buildInitialSlots( + slotEntries: SlotEntries, +): PreparedSlots { + const preparedSlots = {} as PreparedSlots; + + for (const [key, definition] of slotEntries) { + preparedSlots[key] = ( + Array.isArray(definition) ? [] : null + ) as PreparedSlots[typeof key]; + } + + return preparedSlots; +} + +function getSlotComponents(definition: SlotDefinition): readonly SlotComponent[] { + return Array.isArray(definition) ? definition : [definition]; +} + +function resolveSlotEntry( + slotEntries: SlotEntries, + child: ReactElement, +): SlotEntries[number] | undefined { + for (const slotEntry of slotEntries) { + const [, definition] = slotEntry; + + for (const slotComponent of getSlotComponents(definition)) { + if (child.type === slotComponent) { + return slotEntry; + } + } + } + + return undefined; +} + +export function prepareComponentSlots( + definitions: Definitions, + options: PrepareComponentSlotsOptions = {}, +): (children: ReactNode) => PreparedSlots { + const slotEntries = Object.entries(definitions) as SlotEntries; + const { strict = false } = options; + + return function usePreparedComponentSlots(children: ReactNode): PreparedSlots { + return useMemo(() => { + const preparedSlots = buildInitialSlots(slotEntries); + + for (const child of Children.toArray(children)) { + if (!isValidElement(child)) { + if (strict) { + throw new Error("Unexpected non-element child passed to slot-based component."); + } + + continue; + } + + const slotEntry = resolveSlotEntry(slotEntries, child); + + if (slotEntry == null) { + if (strict) { + throw new Error("Unexpected child passed to slot-based component."); + } + + continue; + } + + const [key, definition] = slotEntry; + + if (Array.isArray(definition)) { + const slotChildren = preparedSlots[key] as ReactElement[]; + slotChildren.push(child); + continue; + } + + preparedSlots[key] = child as PreparedSlots[typeof key]; + } + + return preparedSlots; + }, [children]); + }; +} diff --git a/skills/atomic-design/references/slot-based-composition.md b/skills/atomic-design/references/slot-based-composition.md new file mode 100644 index 0000000..0af4a1b --- /dev/null +++ b/skills/atomic-design/references/slot-based-composition.md @@ -0,0 +1,154 @@ +# Slot-Based Composition + +## Sources + +- [Building Component Slots in React](https://sandroroth.com/blog/react-slots/) by Sandro Roth. +- [What is the React Slots pattern?](https://dev.to/neetigyachahar/what-is-the-react-slots-pattern-2ld9) by Neetigya Chahar. +- [Slot-Based APIs in React: Designing Flexible and Composable Components](https://dev.to/talissoncosta/slot-based-apis-in-react-designing-flexible-and-composable-components-7pj) by Talisson Costa. + +## Model + +Slot-based composition gives a component controlled insertion points for caller-provided content. Atomic design decides where a component belongs. Type-filtered slots decide how callers fill the component's stable structure while the parent guarantees positioning. + +Use this pattern when a molecule, organism, or template has named regions that should stay structurally consistent while accepting flexible JSX. Avoid using it as ceremony for components with one simple `children` region. + +## Required Pattern + +- Expose semantic compound slot components from the parent API, such as `Card.Title`, `Card.Description`, and `Card.Actions`. +- Keep slot components colocated with the owning component. +- In the parent root component, inspect direct children and filter recognized slot components by their component type. +- Put generic child filtering logic in a shared `prepareComponentSlots` helper. +- Wrap the shared helper in a colocated component hook named like `useSlots`. +- Define slot components as function components. Type filtering compares `child.type` with the function reference registered in the slot map. +- Render each recognized slot in the parent's fixed structural position. +- Use a single component value for slots that should default to `null`, such as `title: Title`. +- Use an array containing one component value for slots that should default to an empty array, such as `body: [Body]`. +- Document behavior for missing slots, duplicated slots, unknown children, and ordering. + +## Atomic Placement + +- Atoms may expose a default slot through `children` for primitive content. +- Molecules are the usual home for named slots because they group atoms into one small function. +- Organisms may expose larger slots for section regions, actions, filters, media, empty states, or summaries. +- Templates may expose layout slots or render props for page regions, but they must not own final route data. +- Pages fill slots with real content, route wiring, permissions, and state variations. + +## Folder Shape + +Use the owning component folder as the boundary for slot files: + +```text +src/components/ + atoms/ + component-a/ + index.tsx + molecules/ + component-b/ + title.tsx + body.tsx + use-component-b-slots.ts + index.tsx +``` + +The slot file can import lower-stage components: + +```tsx +import type { ReactNode } from "react"; + +import { ComponentA } from "../../atoms/component-a/index.js"; + +export interface TitleProps { + children: ReactNode; +} + +export function Title({ children }: TitleProps) { + return {children}; +} +``` + +The shared helper is declared in [prepareComponentSlots](./prepareComponentSlots.ts). Copy it into the project's shared hooks or utilities area using the project's local file naming and import conventions. + +The colocated component hook wraps the shared helper: + +```tsx +import type { ReactNode } from "react"; + +import { prepareComponentSlots } from "../../hooks/prepare-component-slots.js"; +import { Body } from "./body.js"; +import { Title } from "./title.js"; + +const usePreparedComponentBSlots = prepareComponentSlots({ + body: [Body], + title: Title, +}); + +export function useComponentBSlots(children: ReactNode) { + return usePreparedComponentBSlots(children); +} +``` + +The root file consumes the colocated hook, exports the component, and attaches its slots: + +```tsx +import type { ReactNode } from "react"; + +import { Body } from "./body.js"; +import { Title } from "./title.js"; +import { useComponentBSlots } from "./use-component-b-slots.js"; + +export interface ComponentBProps { + children: ReactNode; +} + +export function ComponentBRoot({ children }: ComponentBProps) { + const { body, title } = useComponentBSlots(children); + + return ( +
+ {title} +
{body}
+
+ ); +} + +export const ComponentB = Object.assign(ComponentBRoot, { + Body, + Title, +}); +``` + +Consumers compose through the parent API: + +```tsx + + Additional body content. + Project status + +``` + +The parent still renders the title and body in the order defined by `ComponentBRoot`. + +With `prepareComponentSlots`, missing single slots resolve to `null`, missing array slots resolve to `[]`, duplicate single slots use the last matching child, duplicate array slots collect every matching child, and unknown children are ignored unless `strict: true` is passed. + +## Implementation Rules + +- Prefer one composition pattern per component family. +- Keep slot components small and semantic. They should name a region, provide structure, or adapt lower-level components. +- Keep business decisions at pages or feature-level integration points, not inside reusable slot components. +- Preserve accessible DOM order. If the parent normalizes slot regions, its rendered order must be the correct reading and keyboard order. +- When normalizing slots by child type, provide clear behavior for missing, duplicated, unknown, and fallback children. +- Keep child filtering shallow unless the component explicitly documents nested slot support. +- Keep the hook focused on resolution. Rendering and layout stay in the root component. +- Keep the generic helper shared and the component-specific hook colocated with the component. + +## Review Prompts + +- Does the atomic stage own the component's responsibility? +- Does the slot API reduce prop bloat without hiding required structure? +- Are slots named after domain or semantic regions? +- Can consumers omit optional slots without layout breakage? +- Are required slots validated by types, tests, stories, or runtime guards according to local practice? +- Does the implementation preserve accessible DOM order and keyboard flow? +- Are lower-stage imports flowing in the correct direction? +- Is slot resolution implemented once in a hook instead of duplicated across roots, stories, or tests? +- Are examples or stories covering default content, each named slot, missing optional slots, duplicated slots, unknown children, and consumer order that differs from rendered order? From 2ed9dec175ca3b1aff442f78d7bd22e40fbdd9c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Gonz=C3=A1lez?= Date: Mon, 13 Jul 2026 16:36:51 +0200 Subject: [PATCH 2/4] refactor(skills): simplify slot helper Reduce the reusable slot helper to the core type-filtering behavior and remove strict-mode handling. --- .../references/prepareComponentSlots.ts | 106 ++++-------------- .../references/slot-based-composition.md | 2 +- 2 files changed, 21 insertions(+), 87 deletions(-) diff --git a/skills/atomic-design/references/prepareComponentSlots.ts b/skills/atomic-design/references/prepareComponentSlots.ts index d9c1cda..9daaf04 100644 --- a/skills/atomic-design/references/prepareComponentSlots.ts +++ b/skills/atomic-design/references/prepareComponentSlots.ts @@ -1,107 +1,41 @@ import { Children, isValidElement, useMemo } from "react"; import type { ReactElement, ReactNode } from "react"; -export interface PrepareComponentSlotsOptions { - strict?: boolean; -} - -type SlotComponent = (props: Props) => ReactNode; - +type SlotComponent = (props: never) => ReactNode; type SlotDefinition = SlotComponent | readonly SlotComponent[]; -type SlotDefinitions = Record; - -type SlotProps = Component extends SlotComponent ? Props : never; - -type PreparedSlot = Definition extends readonly (infer Component)[] - ? Component extends SlotComponent - ? ReactElement>[] - : never - : Definition extends SlotComponent - ? ReactElement> | null - : never; - -type PreparedSlots = { - [Key in keyof Definitions]: PreparedSlot; -}; +type PreparedSlots = Record; -type SlotEntries = { - [Key in keyof Definitions]: [Key, Definitions[Key]]; -}[keyof Definitions][]; - -function buildInitialSlots( - slotEntries: SlotEntries, -): PreparedSlots { - const preparedSlots = {} as PreparedSlots; - - for (const [key, definition] of slotEntries) { - preparedSlots[key] = ( - Array.isArray(definition) ? [] : null - ) as PreparedSlots[typeof key]; - } - - return preparedSlots; -} - -function getSlotComponents(definition: SlotDefinition): readonly SlotComponent[] { - return Array.isArray(definition) ? definition : [definition]; -} +export function prepareComponentSlots( + definitions: Record, +): (children: ReactNode) => PreparedSlots { + const entries = Object.entries(definitions); -function resolveSlotEntry( - slotEntries: SlotEntries, - child: ReactElement, -): SlotEntries[number] | undefined { - for (const slotEntry of slotEntries) { - const [, definition] = slotEntry; + return function usePreparedComponentSlots(children: ReactNode): PreparedSlots { + return useMemo(() => { + const preparedSlots: PreparedSlots = {}; - for (const slotComponent of getSlotComponents(definition)) { - if (child.type === slotComponent) { - return slotEntry; + for (const [key, definition] of entries) { + preparedSlots[key] = Array.isArray(definition) ? [] : null; } - } - } - - return undefined; -} - -export function prepareComponentSlots( - definitions: Definitions, - options: PrepareComponentSlotsOptions = {}, -): (children: ReactNode) => PreparedSlots { - const slotEntries = Object.entries(definitions) as SlotEntries; - const { strict = false } = options; - - return function usePreparedComponentSlots(children: ReactNode): PreparedSlots { - return useMemo(() => { - const preparedSlots = buildInitialSlots(slotEntries); for (const child of Children.toArray(children)) { - if (!isValidElement(child)) { - if (strict) { - throw new Error("Unexpected non-element child passed to slot-based component."); - } + if (!isValidElement(child)) continue; - continue; - } - - const slotEntry = resolveSlotEntry(slotEntries, child); + const match = entries.find(([, definition]) => { + const slotComponents = Array.isArray(definition) ? definition : [definition]; + return slotComponents.includes(child.type as SlotComponent); + }); - if (slotEntry == null) { - if (strict) { - throw new Error("Unexpected child passed to slot-based component."); - } - - continue; - } + if (match == null) continue; - const [key, definition] = slotEntry; + const [key, definition] = match; if (Array.isArray(definition)) { - const slotChildren = preparedSlots[key] as ReactElement[]; - slotChildren.push(child); + (preparedSlots[key] as ReactElement[]).push(child); continue; } - preparedSlots[key] = child as PreparedSlots[typeof key]; + preparedSlots[key] = child; } return preparedSlots; diff --git a/skills/atomic-design/references/slot-based-composition.md b/skills/atomic-design/references/slot-based-composition.md index 0af4a1b..765ef70 100644 --- a/skills/atomic-design/references/slot-based-composition.md +++ b/skills/atomic-design/references/slot-based-composition.md @@ -128,7 +128,7 @@ Consumers compose through the parent API: The parent still renders the title and body in the order defined by `ComponentBRoot`. -With `prepareComponentSlots`, missing single slots resolve to `null`, missing array slots resolve to `[]`, duplicate single slots use the last matching child, duplicate array slots collect every matching child, and unknown children are ignored unless `strict: true` is passed. +With `prepareComponentSlots`, missing single slots resolve to `null`, missing array slots resolve to `[]`, duplicate single slots use the last matching child, duplicate array slots collect every matching child, and unknown children are ignored. ## Implementation Rules From 3e6f96fbb98b5e4202f9f73201771ecddac8cd46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Gonz=C3=A1lez?= Date: Thu, 13 Aug 2026 20:43:54 +0200 Subject: [PATCH 3/4] feat(skills): add global content-addressed store Share verified skill trees across projects while materializing independent project-owned copies. Pin remote installs to exact commits, migrate legacy caches safely, and retain install progress counters. --- .env.example | 20 ++ .gitignore | 1 + .prettierignore | 1 + src/core/cli.ts | 6 +- src/core/context.ts | 14 +- src/core/fs/copy-tree.ts | 66 +++++ src/core/fs/link.ts | 14 +- src/core/index.ts | 5 + src/core/lock.ts | 42 +++- src/core/logger.ts | 42 +++- src/core/paths.ts | 4 +- src/core/resolver.ts | 87 +++++-- src/core/run.ts | 6 +- src/core/skill-materialize.ts | 69 ++++++ src/core/skill-prepare.ts | 97 +++++++- src/core/skill-store.ts | 146 +++++++++++ src/core/state.ts | 13 + src/core/store-path.ts | 41 ++++ src/core/types/public.ts | 19 +- src/domains/skills/concurrency.ts | 33 +++ src/domains/skills/index.ts | 21 +- src/domains/skills/pipeline.ts | 96 ++++++-- src/domains/skills/steps.ts | 228 ++++++++++++++---- test/agents/adapters.test.ts | 2 +- test/agents/cleanup.test.ts | 2 +- test/agents/idempotency.test.ts | 2 +- test/core/context.test.ts | 25 ++ test/core/ensure-link.test.ts | 2 +- test/core/init-steps.test.ts | 2 +- test/core/link.test.ts | 12 +- test/core/lock.test.ts | 13 + test/core/logger-progress.test.ts | 38 +++ ...olver-nocache.test.ts => resolver.test.ts} | 53 +++- test/core/run.test.ts | 53 ++++ test/core/skill-materialize.test.ts | 69 ++++++ test/core/skill-prepare.test.ts | 17 +- test/core/skill-store.test.ts | 82 +++++++ test/core/state.test.ts | 1 + test/core/store-path.test.ts | 27 +++ test/core/watch.test.ts | 2 +- test/docs/compile.test.ts | 2 +- test/domains/commands.test.ts | 4 +- test/domains/empty-slices.test.ts | 2 +- test/domains/global-skill-store.test.ts | 212 ++++++++++++++++ test/domains/skills-concurrency.test.ts | 19 ++ test/domains/skills-pipeline.test.ts | 97 ++++++-- test/rules/rules-domain.test.ts | 2 +- test/skills/pipeline.test.ts | 74 +++++- 48 files changed, 1719 insertions(+), 166 deletions(-) create mode 100644 .env.example create mode 100644 src/core/fs/copy-tree.ts create mode 100644 src/core/skill-materialize.ts create mode 100644 src/core/skill-store.ts create mode 100644 src/core/store-path.ts create mode 100644 src/domains/skills/concurrency.ts create mode 100644 test/core/context.test.ts create mode 100644 test/core/logger-progress.test.ts rename test/core/{resolver-nocache.test.ts => resolver.test.ts} (71%) create mode 100644 test/core/run.test.ts create mode 100644 test/core/skill-materialize.test.ts create mode 100644 test/core/skill-store.test.ts create mode 100644 test/core/store-path.test.ts create mode 100644 test/domains/global-skill-store.test.ts create mode 100644 test/domains/skills-concurrency.test.ts diff --git a/.env.example b/.env.example new file mode 100644 index 0000000..19cf449 --- /dev/null +++ b/.env.example @@ -0,0 +1,20 @@ +# —————————————————————————————————————————————————————————————————————————— +# ——— Agnos: Local CLI configuration + +# Use : Override the user-level content-addressed skill store location. +# Source: Choose a writable directory trusted by the current user. +# Path : Local filesystem > Agnos store +# E.g. : C:\Users\name\AppData\Local\agnos\store +AGNOS_STORE_DIR= + +# Use : Enable diagnostic CLI logging. +# Source: Set locally when troubleshooting Agnos. +# Path : Local shell > Environment +# E.g. : 1 +AGNOS_DEBUG= + +# Use : Override the Model Context Protocol registry endpoint. +# Source: Use the endpoint supplied by the registry operator. +# Path : MCP registry > API endpoint +# E.g. : https://registry.modelcontextprotocol.io +AGNOS_MCP_REGISTRY=https://registry.modelcontextprotocol.io diff --git a/.gitignore b/.gitignore index 0a3c8b3..37c5849 100644 --- a/.gitignore +++ b/.gitignore @@ -13,6 +13,7 @@ scratch/ # ENV .env* +!.env.example # AGENTS .agents/ diff --git a/.prettierignore b/.prettierignore index 0dde962..6690a30 100644 --- a/.prettierignore +++ b/.prettierignore @@ -7,3 +7,4 @@ pnpm-lock.yaml test-output scratch packages/*/dist +.env.example diff --git a/src/core/cli.ts b/src/core/cli.ts index c7aacb8..c9e0ddd 100644 --- a/src/core/cli.ts +++ b/src/core/cli.ts @@ -113,7 +113,11 @@ async function main(): Promise { args: rest, logger: withDomain(ctx.logger, dom.domain), }; - await cmd.run(cmdCtx); + try { + await cmd.run(cmdCtx); + } finally { + await cmdCtx.fetcher.cleanup(); + } return; } diff --git a/src/core/context.ts b/src/core/context.ts index 5aa7473..c4deb88 100644 --- a/src/core/context.ts +++ b/src/core/context.ts @@ -4,6 +4,7 @@ import { createRepoFetcher } from "./resolver.js"; import { createLogger } from "./logger.js"; import { buildPaths, ensureDir } from "./paths.js"; import { readConfigOrDefault } from "./config.js"; +import { resolveGlobalStoreDir } from "./store-path.js"; import type { AgnosConfig, Logger, ResolveContext } from "./types/public.js"; export interface BuildContextOptions { @@ -12,29 +13,32 @@ export interface BuildContextOptions { dryRun?: boolean; logger?: Logger; config?: AgnosConfig; + storeDir?: string; } export async function buildResolveContext(opts: BuildContextOptions): Promise { const config = opts.config ?? (await readConfigOrDefault(path.join(opts.projectRoot, "agnos.json"))); const paths = buildPaths(opts.projectRoot, config); + const logger = opts.logger ?? createLogger(); if (!opts.dryRun) { await ensureDir(paths.agnosRoot); - await ensureDir(paths.cacheDir); + await ensureDir(paths.tempDir); } - const logger = opts.logger ?? createLogger(); const linker = createLinker({ - cacheDir: paths.cacheDir, + probeDir: path.join(paths.tempDir, "link-probes"), logger, copyFallback: opts.copyFallback, }); - const fetcher = createRepoFetcher({ projectRoot: opts.projectRoot, cacheDir: paths.cacheDir }); + const fetcher = createRepoFetcher({ + stagingDir: path.join(paths.tempDir, "repos"), + }); return { projectRoot: opts.projectRoot, configPath: paths.configPath, statePath: paths.statePath, agnosRoot: paths.agnosRoot, - cacheDir: paths.cacheDir, + storeDir: opts.storeDir ?? resolveGlobalStoreDir(), logger, fetcher, linker, diff --git a/src/core/fs/copy-tree.ts b/src/core/fs/copy-tree.ts new file mode 100644 index 0000000..9837d1d --- /dev/null +++ b/src/core/fs/copy-tree.ts @@ -0,0 +1,66 @@ +import { constants } from "node:fs"; +import fs from "node:fs/promises"; +import path from "node:path"; + +export type FileImportMethod = "clone" | "hardlink" | "copy"; + +export interface CopyTreeResult { + methods: Set; +} + +export async function copyRegularTree( + source: string, + destination: string, + importFiles = false, +): Promise { + const methods = new Set(); + await copyDirectory(source, destination, importFiles, methods); + return { methods }; +} + +async function copyDirectory( + source: string, + destination: string, + importFiles: boolean, + methods: Set, +): Promise { + await fs.mkdir(destination, { recursive: true }); + const entries = await fs.readdir(source, { withFileTypes: true }); + await Promise.all( + entries.map(async (entry) => { + const sourcePath = path.join(source, entry.name); + const destinationPath = path.join(destination, entry.name); + if (entry.isDirectory()) { + await copyDirectory(sourcePath, destinationPath, importFiles, methods); + return; + } + if (!entry.isFile()) return; + const method = importFiles + ? await importFile(sourcePath, destinationPath) + : await cloneOrCopyFile(sourcePath, destinationPath); + methods.add(method); + const stat = await fs.stat(sourcePath); + await fs.chmod(destinationPath, stat.mode); + }), + ); +} + +async function importFile(source: string, destination: string): Promise { + try { + await fs.copyFile(source, destination, constants.COPYFILE_FICLONE_FORCE); + return "clone"; + } catch { + try { + await fs.link(source, destination); + return "hardlink"; + } catch { + await fs.copyFile(source, destination); + return "copy"; + } + } +} + +async function cloneOrCopyFile(source: string, destination: string): Promise { + await fs.copyFile(source, destination, constants.COPYFILE_FICLONE); + return "copy"; +} diff --git a/src/core/fs/link.ts b/src/core/fs/link.ts index d98c11a..31357ef 100644 --- a/src/core/fs/link.ts +++ b/src/core/fs/link.ts @@ -4,20 +4,20 @@ import os from "node:os"; import type { LinkKind, Linker, Logger } from "../types/public.js"; interface LinkerOptions { - cacheDir: string; + probeDir: string; logger: Logger; copyFallback?: boolean; } -export function createLinker({ cacheDir, logger, copyFallback }: LinkerOptions): Linker { +export function createLinker({ probeDir, logger, copyFallback }: LinkerOptions): Linker { let cachedFileProbe: boolean | undefined; async function probeFileSymlink(): Promise { if (cachedFileProbe !== undefined) return cachedFileProbe; - await fs.mkdir(cacheDir, { recursive: true }); - const probeDir = await fs.mkdtemp(path.join(cacheDir, "link-probe-")); - const target = path.join(probeDir, "target"); - const link = path.join(probeDir, "link"); + await fs.mkdir(probeDir, { recursive: true }); + const sessionDir = await fs.mkdtemp(path.join(probeDir, "link-probe-")); + const target = path.join(sessionDir, "target"); + const link = path.join(sessionDir, "link"); try { await fs.writeFile(target, ""); await fs.symlink(target, link, "file"); @@ -26,7 +26,7 @@ export function createLinker({ cacheDir, logger, copyFallback }: LinkerOptions): logger.debug(`file symlink probe failed: ${(err as Error).message}`); cachedFileProbe = false; } finally { - await fs.rm(probeDir, { recursive: true, force: true }); + await fs.rm(sessionDir, { recursive: true, force: true }); } return cachedFileProbe; } diff --git a/src/core/index.ts b/src/core/index.ts index 0b7586a..28c5af6 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -22,6 +22,7 @@ export type { Linker, LockFile, Logger, + LoggerProgress, LogInput, LogParts, LogTask, @@ -61,6 +62,8 @@ export { SCHEMA_URL, } from "./config.js"; export { buildResolveContext, workspaceRelativePath } from "./context.js"; +export { resolveGlobalStoreDir } from "./store-path.js"; +export type { GlobalStorePathOptions } from "./store-path.js"; export { loadPlugins, orderedDomains, refToId, resolveAgentByRef } from "./plugin-loader.js"; export type { PluginRegistry, RegisteredAgent, RegisteredDomain } from "./plugin-loader.js"; export { runAll, runOne, runFrom } from "./run.js"; @@ -89,6 +92,8 @@ export type { CommitResolution } from "./commit-resolver.js"; export { findSkillsInRepo, readSkillMeta } from "./skill-discovery.js"; export type { DiscoveredSkill } from "./skill-discovery.js"; export { hashSkillDir } from "./skill-hash.js"; +export { materializeSkill } from "./skill-materialize.js"; +export type { MaterializeSkillResult } from "./skill-materialize.js"; export { prepareSkills } from "./skill-prepare.js"; export type { PrepareResult } from "./skill-prepare.js"; export { diff --git a/src/core/lock.ts b/src/core/lock.ts index 6f1442d..7b21029 100644 --- a/src/core/lock.ts +++ b/src/core/lock.ts @@ -1,5 +1,6 @@ import fs from "node:fs/promises"; import path from "node:path"; +import { randomUUID } from "node:crypto"; import { lockFileSchema } from "./schema.js"; import type { LockFile, SkillLockEntry } from "./types/public.js"; @@ -43,7 +44,46 @@ export async function writeLock(projectRoot: string, lock: LockFile): Promise { + for (let attempt = 0; attempt < 8; attempt += 1) { + try { + await fs.rename(temporary, destination); + return; + } catch (error) { + const current = await fs.readFile(destination, "utf8").catch(() => null); + if (current === contents) return; + if (!isTransient(error) || attempt === 7) { + throw new Error(`failed to publish ${LOCK_FILE}: ${(error as Error).message}`, { + cause: error, + }); + } + await wait(25 * (attempt + 1)); + } + } +} + +function isTransient(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException).code; + return code === "EACCES" || code === "EBUSY" || code === "EEXIST" || code === "EPERM"; +} + +async function wait(milliseconds: number): Promise { + await new Promise((resolve) => { + setTimeout(resolve, milliseconds); + }); } export function upsertSkill(lock: LockFile, key: string, entry: SkillLockEntry): LockFile { diff --git a/src/core/logger.ts b/src/core/logger.ts index 9a68aea..cad8ece 100644 --- a/src/core/logger.ts +++ b/src/core/logger.ts @@ -1,5 +1,12 @@ import colors from "yoctocolors-cjs"; -import type { DomainColor, Logger, LogInput, LogParts, LogTask } from "./types/public.js"; +import type { + DomainColor, + Logger, + LoggerProgress, + LogInput, + LogParts, + LogTask, +} from "./types/public.js"; /** Dim/grey secondary text (e.g. an inline description). No-op off a TTY. */ export function dim(msg: string): string { @@ -85,6 +92,7 @@ function clearActiveLine(): void { } interface SpinnerHandle { + update(parts: LogParts): void; stop(): void; } @@ -102,7 +110,7 @@ function startSpinner( fmt: { domain?: string; color?: DomainColor }, quiet: boolean, ): SpinnerHandle { - if (quiet || !process.stderr.isTTY) return { stop() {} }; + if (quiet || !process.stderr.isTTY) return { update() {}, stop() {} }; if (timer) clearInterval(timer); // supersede any running spinner let message = LEVEL_COLOR[level](parts.message); if (parts.status) message += ` ${colors.dim(colors.italic(parts.status))}`; @@ -112,6 +120,14 @@ function startSpinner( renderActive(); timer = setInterval(renderActive, 80); return { + update(next): void { + if (active !== self) return; + let message = LEVEL_COLOR[level](next.message); + if (next.status) message += ` ${colors.dim(colors.italic(next.status))}`; + self.message = message; + clearActiveLine(); + renderActive(); + }, stop(): void { if (active !== self) return; // a newer spinner took over; leave it alone if (timer) clearInterval(timer); @@ -194,6 +210,17 @@ function makeLogger(state: LoggerState): Logger { }; const logger = { + progress(msg: LogInput): LoggerProgress { + const handle = startSpinner("info", toParts(msg), fmt, suppressed("info")); + return { + update(next): void { + handle.update(toParts(next)); + }, + stop(): void { + handle.stop(); + }, + }; + }, info: make("info"), success: make("success"), warn: make("warn"), @@ -241,6 +268,17 @@ export function indentedLogger(base: Logger, indent: string): Logger { (msg: any): any => (fn as (m: unknown) => unknown)(pad(msg)); return { + progress(msg: LogInput): LoggerProgress { + const handle = base.progress(pad(msg) as LogInput); + return { + update(next): void { + handle.update(pad(next) as LogInput); + }, + stop(): void { + handle.stop(); + }, + }; + }, info: wrap(base.info), success: wrap(base.success), warn: wrap(base.warn), diff --git a/src/core/paths.ts b/src/core/paths.ts index dac4eee..2274fcd 100644 --- a/src/core/paths.ts +++ b/src/core/paths.ts @@ -12,7 +12,7 @@ export interface ProjectPaths { projectRoot: string; configPath: string; agnosRoot: string; - cacheDir: string; + tempDir: string; skillsDir: string; statePath: string; } @@ -24,7 +24,7 @@ export function buildPaths(projectRoot: string, config?: AgnosConfig): ProjectPa projectRoot, configPath: path.join(projectRoot, CONFIG_FILE), agnosRoot, - cacheDir: path.join(agnosRoot, "cache"), + tempDir: path.join(agnosRoot, "tmp"), skillsDir: path.isAbsolute(skillsRel) ? skillsRel : path.join(projectRoot, skillsRel), statePath: path.join(agnosRoot, STATE_FILE), }; diff --git a/src/core/resolver.ts b/src/core/resolver.ts index 17a8a0e..69fbbb3 100644 --- a/src/core/resolver.ts +++ b/src/core/resolver.ts @@ -1,6 +1,6 @@ import fs from "node:fs/promises"; import path from "node:path"; -import { createHash } from "node:crypto"; +import { createHash, randomUUID } from "node:crypto"; import { execFile as execFileCb } from "node:child_process"; import { promisify } from "node:util"; import { type GitSource, type LocalSource, type ParsedSource } from "./source.js"; @@ -14,8 +14,8 @@ const DISCOVERY_SUBDIR = "skills"; export interface RepoFetchOptions { /** Optional commit SHA (or branch/tag) to fetch. Defaults to provider default branch. */ ref?: string; - /** Bypass any cached download; force a fresh fetch. */ - noCache?: boolean; + /** Replace any matching checkout already staged during this run. */ + fresh?: boolean; } export interface RepoFetchResult { @@ -23,25 +23,56 @@ export interface RepoFetchResult { path: string; /** Git ref actually fetched (the explicit ref, or the resolved default branch). */ ref?: string; + /** Commit SHA checked out for a Git source. */ + commit?: string; } export interface RepoFetcher { - /** Fetch a repository source to a cache-managed directory and return the root path. */ + /** Fetch a repository source to a transient directory and return the root path. */ fetch(source: ParsedSource, opts?: RepoFetchOptions): Promise; + /** Remove every transient checkout created by this fetcher. */ + cleanup(): Promise; } export interface CreateRepoFetcherOptions { - projectRoot: string; - cacheDir: string; + stagingDir: string; } export function createRepoFetcher(opts: CreateRepoFetcherOptions): RepoFetcher { + const inFlight = new Map>(); + const sessionDir = path.join(opts.stagingDir, randomUUID()); return { async fetch(source, fetchOpts) { if (source.kind === "local") { return { path: source.absolutePath }; } - return fetchGit(source, opts, fetchOpts); + const subdir = source.subPath ?? DISCOVERY_SUBDIR; + const requestedRef = fetchOpts?.ref ?? source.ref ?? "default"; + const key = `${source.canonical}@${requestedRef}@${subdir}@${fetchOpts?.fresh ? "fresh" : "staged"}`; + const pending = inFlight.get(key); + if (pending) return pending; + + const fetch = fetchGit(source, { stagingDir: sessionDir }, fetchOpts); + inFlight.set(key, fetch); + try { + return await fetch; + } finally { + if (inFlight.get(key) === fetch) inFlight.delete(key); + } + }, + async cleanup() { + await Promise.allSettled([...inFlight.values()]); + inFlight.clear(); + try { + await fs.rm(sessionDir, { + recursive: true, + force: true, + maxRetries: 3, + retryDelay: 50, + }); + } catch { + return; + } }, }; } @@ -61,20 +92,35 @@ async function fetchGit( const explicitRef = opts?.ref ?? source.ref; const ref = explicitRef ?? (await resolveDefaultBranch(source).catch(() => null)) ?? undefined; - // Cache is keyed by repo + ref + subtree so discovery and per-skill installs - // don't clobber each other. - const cacheKey = hashKey(`${source.canonical}@${ref ?? "HEAD"}@${subdir}`); - const destDir = path.join(cfg.cacheDir, "repos", cacheKey); + const destDir = resolveRepoStagingDir(source, cfg.stagingDir, ref); + const stagingKey = path.basename(destDir); - if (!opts?.noCache && (await dirHasFiles(destDir))) { - return { path: destDir, ref }; + if (!opts?.fresh && (await dirHasFiles(destDir))) { + const commit = await resolveCheckoutCommit(destDir); + return { path: destDir, ref, ...(commit ? { commit } : {}) }; } + const parent = path.dirname(destDir); + const temporary = path.join(parent, `.tmp-${stagingKey}-${randomUUID()}`); await fs.rm(destDir, { recursive: true, force: true }); - await fs.mkdir(path.dirname(destDir), { recursive: true }); + await fs.rm(temporary, { recursive: true, force: true }); + await fs.mkdir(parent, { recursive: true }); + + try { + const commit = await sparseClone(buildCloneUrl(source), temporary, ref, subdir); + await fs.rename(temporary, destDir); + return { path: destDir, ref, ...(commit ? { commit } : {}) }; + } catch (error) { + throw new Error(`failed to fetch ${source.canonical}`, { cause: error }); + } finally { + await fs.rm(temporary, { recursive: true, force: true }); + } +} - await sparseClone(buildCloneUrl(source), destDir, ref, subdir); - return { path: destDir, ref }; +function resolveRepoStagingDir(source: GitSource, stagingDir: string, ref?: string): string { + const subdir = source.subPath ?? DISCOVERY_SUBDIR; + const stagingKey = hashKey(`${source.canonical}@${ref ?? "HEAD"}@${subdir}`); + return path.join(stagingDir, stagingKey); } /** @@ -89,7 +135,7 @@ async function sparseClone( dest: string, ref: string | undefined, subdir: string, -): Promise { +): Promise { const cloneArgs = ["clone", "--no-checkout", "--depth", "1", "--filter=blob:none"]; if (ref) cloneArgs.push("--branch", ref); cloneArgs.push(url, dest); @@ -118,6 +164,13 @@ async function sparseClone( ]); await execFile("git", ["-C", dest, "checkout", "FETCH_HEAD"]); } + return resolveCheckoutCommit(dest); +} + +async function resolveCheckoutCommit(directory: string): Promise { + const result = await execFile("git", ["-C", directory, "rev-parse", "HEAD"]); + const commit = result.stdout.trim(); + return commit || undefined; } async function dirHasFiles(p: string): Promise { diff --git a/src/core/run.ts b/src/core/run.ts index 8d9f349..a92d5ad 100644 --- a/src/core/run.ts +++ b/src/core/run.ts @@ -11,7 +11,11 @@ async function runDomain( // Scope the logger to this domain so everything it prints carries the // domain's `[domain]` prefix and color automatically. const scoped: RunContext = { ...ctx, logger: withDomain(ctx.logger, dom.domain) }; - await dom.domain.run(opts, scoped); + try { + await dom.domain.run(opts, scoped); + } finally { + await scoped.fetcher.cleanup?.(); + } } /** diff --git a/src/core/skill-materialize.ts b/src/core/skill-materialize.ts new file mode 100644 index 0000000..78c1df4 --- /dev/null +++ b/src/core/skill-materialize.ts @@ -0,0 +1,69 @@ +import { randomUUID } from "node:crypto"; +import fs from "node:fs/promises"; +import path from "node:path"; +import { copyRegularTree, type CopyTreeResult } from "./fs/copy-tree.js"; + +const SKILL_MARKER = "SKILL.md"; + +export interface MaterializeSkillResult extends CopyTreeResult { + changed: boolean; +} + +export async function materializeSkill( + stored: string, + destination: string, + expectedHash: string, + materializedHash?: string, +): Promise { + if (materializedHash === expectedHash && (await isMaterializedSkill(destination))) { + return { changed: false, methods: new Set() }; + } + + const parent = path.dirname(destination); + const temporary = path.join(parent, `.tmp-${path.basename(destination)}-${randomUUID()}`); + const previous = path.join(parent, `.old-${path.basename(destination)}-${randomUUID()}`); + await fs.mkdir(parent, { recursive: true }); + + try { + const result = await copyRegularTree(stored, temporary, true); + const existing = await fs.lstat(destination).catch(() => null); + if (existing) await retryRename(destination, previous); + try { + await retryRename(temporary, destination); + } catch (error) { + if (existing) await retryRename(previous, destination).catch(() => undefined); + throw error; + } + await fs + .rm(previous, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 }) + .catch(() => undefined); + return { changed: true, methods: result.methods }; + } finally { + await fs.rm(temporary, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 }); + } +} + +async function retryRename(source: string, destination: string): Promise { + for (let attempt = 0; attempt < 8; attempt += 1) { + try { + await fs.rename(source, destination); + return; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + const transient = code === "EACCES" || code === "EBUSY" || code === "EPERM"; + if (!transient || attempt === 7) throw error; + await new Promise((resolve) => { + setTimeout(resolve, 25 * (attempt + 1)); + }); + } + } +} + +async function isMaterializedSkill(directory: string): Promise { + const stat = await fs.lstat(directory).catch(() => null); + if (!stat?.isDirectory() || stat.isSymbolicLink()) return false; + return fs + .access(path.join(directory, SKILL_MARKER)) + .then(() => true) + .catch(() => false); +} diff --git a/src/core/skill-prepare.ts b/src/core/skill-prepare.ts index cef7368..bfaaee2 100644 --- a/src/core/skill-prepare.ts +++ b/src/core/skill-prepare.ts @@ -4,6 +4,9 @@ import { buildPaths } from "./paths.js"; import { getSkill, readLock, upsertSkill, writeLock } from "./lock.js"; import { parseCompositeSkillRef } from "./source.js"; import { hashSkillDir } from "./skill-hash.js"; +import { ensureStoredSkill, findStoredSkill } from "./skill-store.js"; +import { materializeSkill } from "./skill-materialize.js"; +import { readState, writeState } from "./state.js"; import type { AgnosConfig, ResolveContext } from "./types/public.js"; const SKILL_MARKER = "SKILL.md"; @@ -25,14 +28,25 @@ export interface PrepareResult { * - missing entry → write it (fresh-clone reproducibility). * - match → proceed. * - mismatch → fail loudly with a clear remediation step. - * 4. Copy `/` to `/` so the canonical - * bytes are on disk before any agent hook runs. + * 4. Store the content by hash and materialize `/` so the + * canonical bytes are available before any agent hook runs. * * Returns a summary so callers can log what was filled vs. verified. */ export async function prepareSkills( config: AgnosConfig, ctx: ResolveContext, +): Promise { + try { + return await prepareSkillsFromSources(config, ctx); + } finally { + await ctx.fetcher.cleanup(); + } +} + +async function prepareSkillsFromSources( + config: AgnosConfig, + ctx: ResolveContext, ): Promise { const result: PrepareResult = { filled: [], verified: [] }; const entries = Object.entries(config.skills?.sources ?? {}); @@ -40,14 +54,17 @@ export async function prepareSkills( const lockBefore = await readLock(ctx.projectRoot); let lock = lockBefore; + let lockDirty = false; + const state = await readState(ctx.statePath); + let stateDirty = false; const skillsDir = buildPaths(ctx.projectRoot, config).skillsDir; if (!ctx.dryRun) await fs.mkdir(skillsDir, { recursive: true }); for (const [name, composite] of entries) { const ref = parseCompositeSkillRef(composite, { projectRoot: ctx.projectRoot }); - const fetched = await ctx.fetcher.fetch(ref.source); - const skillSrc = - ref.source.kind === "git" ? path.join(fetched.path, ref.subPath) : fetched.path; + const existing = getSkill(lock, composite); + const located = await locatePreparedSkill(name, ref, existing, skillsDir, ctx); + const skillSrc = located.path; if (!(await isSkillDir(skillSrc))) { throw new Error( @@ -58,16 +75,20 @@ export async function prepareSkills( } const hash = await hashSkillDir(skillSrc); - const existing = getSkill(lock, composite); - if (!existing) { + if (ref.source.kind === "git" && !located.commit) { + throw new Error(`could not determine the checked-out commit for skill "${name}"`); + } if (ctx.dryRun) { ctx.logger.info(`would: pin ${name} (${composite}) → ${hash.slice(0, 12)}…`); } else { lock = upsertSkill(lock, composite, { computedHash: hash, resolvedAt: new Date().toISOString(), + ...(located.commit ? { resolvedCommit: located.commit } : {}), + ...(located.ref ? { ref: located.ref } : {}), }); + lockDirty = true; ctx.logger.info(`pinned ${name} (${composite}) → ${hash.slice(0, 12)}…`); } result.filled.push(name); @@ -79,24 +100,80 @@ export async function prepareSkills( `Run \`agnos skill update ${name}\` to accept the new content.`, ); } else { + if (ref.source.kind === "git" && located.commit && !existing.resolvedCommit) { + lock = upsertSkill(lock, composite, { ...existing, resolvedCommit: located.commit }); + lockDirty = true; + } result.verified.push(name); } if (!ctx.dryRun) { const dst = path.join(skillsDir, name); - await fs.rm(dst, { recursive: true, force: true }); - await fs.cp(skillSrc, dst, { recursive: true, force: true }); + const stored = await ensureStoredSkill(skillSrc, ctx.storeDir, hash); + await materializeSkill(stored, dst, hash, state.materializedSkills?.[name]); + if (state.materializedSkills?.[name] !== hash) { + state.materializedSkills = { ...(state.materializedSkills ?? {}), [name]: hash }; + stateDirty = true; + } } } // Only write the lock if anything actually changed and we're not in dry-run. - if (!ctx.dryRun && result.filled.length > 0) { + if (!ctx.dryRun && lockDirty) { await writeLock(ctx.projectRoot, lock); } + if (!ctx.dryRun && stateDirty) await writeState(ctx.statePath, state); return result; } +interface LocatedPreparedSkill { + path: string; + ref?: string; + commit?: string; +} + +async function locatePreparedSkill( + name: string, + composite: ReturnType, + existing: ReturnType, + skillsDir: string, + ctx: ResolveContext, +): Promise { + if (composite.source.kind === "local") return { path: composite.source.absolutePath }; + + if (existing) { + const global = await findStoredSkill(ctx.storeDir, existing.computedHash); + if (global && existing.resolvedCommit) { + return { path: global, ref: existing.ref, commit: existing.resolvedCommit }; + } + + const candidates = [ + path.join(skillsDir, name), + path.join(ctx.agnosRoot, "cache", "skills", existing.computedHash), + ]; + for (const candidate of candidates) { + if ((await hashSkillDir(candidate).catch(() => null)) !== existing.computedHash) continue; + const stored = await ensureStoredSkill(candidate, ctx.storeDir, existing.computedHash); + if (existing.resolvedCommit) { + return { path: stored, ref: existing.ref, commit: existing.resolvedCommit }; + } + } + } + + const trackedRef = composite.source.ref ?? existing?.ref; + const checkoutRef = existing?.resolvedCommit ?? trackedRef; + const fetched = await ctx.fetcher.fetch( + composite.source, + checkoutRef ? { ref: checkoutRef } : undefined, + ); + return { + path: path.join(fetched.path, composite.subPath), + ...((trackedRef ?? fetched.ref) ? { ref: trackedRef ?? fetched.ref } : {}), + ...(fetched.commit ? { commit: fetched.commit } : {}), + }; +} + async function isSkillDir(p: string): Promise { try { const s = await fs.stat(p); diff --git a/src/core/skill-store.ts b/src/core/skill-store.ts new file mode 100644 index 0000000..56f56bc --- /dev/null +++ b/src/core/skill-store.ts @@ -0,0 +1,146 @@ +import { randomUUID } from "node:crypto"; +import fs from "node:fs/promises"; +import path from "node:path"; +import { copyRegularTree } from "./fs/copy-tree.js"; +import { hashSkillDir } from "./skill-hash.js"; + +const SKILL_MARKER = "SKILL.md"; +const storeWrites = new Map>(); +const PUBLISH_RETRIES = 8; +const RETRY_DELAY_MS = 25; + +export function resolveStoredSkill(storeDir: string, hash: string): string { + return path.join(storeDir, "skills", hash); +} + +export async function findStoredSkill(storeDir: string, hash: string): Promise { + const stored = resolveStoredSkill(storeDir, hash); + return (await isStoredSkillValid(stored, hash)) ? stored : null; +} + +export async function ensureStoredSkill( + source: string, + storeDir: string, + hash: string, +): Promise { + const stored = resolveStoredSkill(storeDir, hash); + if (path.resolve(source) === path.resolve(stored)) return stored; + + const pending = storeWrites.get(stored); + if (pending) return pending; + + const write = populateStoredSkill(source, stored); + storeWrites.set(stored, write); + try { + return await write; + } finally { + if (storeWrites.get(stored) === write) storeWrites.delete(stored); + } +} + +async function populateStoredSkill(source: string, stored: string): Promise { + if (await isStoredSkillValid(stored, path.basename(stored))) return stored; + + const parent = path.dirname(stored); + const temporary = path.join(parent, `.tmp-${path.basename(stored)}-${randomUUID()}`); + await fs.mkdir(parent, { recursive: true }); + + try { + await retryTransient(async () => { + await copyRegularTree(source, temporary); + }); + const expectedHash = path.basename(stored); + if ((await hashSkillDir(temporary)) !== expectedHash) { + throw new Error(`source content does not match expected hash ${expectedHash}`); + } + return await publishStoredSkill(temporary, stored, expectedHash); + } catch (error) { + throw new Error(`failed to populate skill store at ${stored}: ${formatError(error)}`, { + cause: error, + }); + } finally { + await fs.rm(temporary, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 }); + } +} + +async function publishStoredSkill( + temporary: string, + stored: string, + expectedHash: string, +): Promise { + for (let attempt = 0; attempt < PUBLISH_RETRIES; attempt += 1) { + try { + await fs.rename(temporary, stored); + return stored; + } catch (error) { + if (await isStoredSkillValid(stored, expectedHash)) return stored; + await quarantineInvalidStoreEntry(stored); + if (!isTransient(error) || attempt === PUBLISH_RETRIES - 1) throw error; + await wait(RETRY_DELAY_MS * (attempt + 1)); + } + } + throw new Error(`could not publish ${stored}`); +} + +async function retryTransient(operation: () => Promise): Promise { + for (let attempt = 0; attempt < PUBLISH_RETRIES; attempt += 1) { + try { + await operation(); + return; + } catch (error) { + if (!isTransient(error) || attempt === PUBLISH_RETRIES - 1) throw error; + await wait(RETRY_DELAY_MS * (attempt + 1)); + } + } +} + +function isTransient(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException).code; + return ( + code === "EACCES" || + code === "EBUSY" || + code === "EEXIST" || + code === "ENOTEMPTY" || + code === "EPERM" + ); +} + +function formatError(error: unknown): string { + const code = (error as NodeJS.ErrnoException).code; + const message = error instanceof Error ? error.message : String(error); + return code ? `${code}: ${message}` : message; +} + +async function wait(milliseconds: number): Promise { + await new Promise((resolve) => { + setTimeout(resolve, milliseconds); + }); +} + +async function isSkillDir(directory: string): Promise { + try { + const stat = await fs.stat(directory); + if (!stat.isDirectory()) return false; + await fs.access(path.join(directory, SKILL_MARKER)); + return true; + } catch { + return false; + } +} + +async function isStoredSkillValid(directory: string, expectedHash: string): Promise { + if (!(await isSkillDir(directory))) return false; + return (await hashSkillDir(directory).catch(() => null)) === expectedHash; +} + +async function quarantineInvalidStoreEntry(directory: string): Promise { + const stat = await fs.lstat(directory).catch(() => null); + if (!stat) return; + const quarantine = `${directory}.invalid-${randomUUID()}`; + try { + await fs.rename(directory, quarantine); + await fs.rm(quarantine, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; + } +} diff --git a/src/core/state.ts b/src/core/state.ts index ff4ef6b..4d40102 100644 --- a/src/core/state.ts +++ b/src/core/state.ts @@ -14,6 +14,8 @@ export interface AgnosState { * `##` sections untouched. */ rulesSections?: Record; + /** Materialized skill name to the content hash last installed by Agnos. */ + materializedSkills?: Record; } const DEFAULT_STATE: AgnosState = { @@ -22,6 +24,7 @@ const DEFAULT_STATE: AgnosState = { initializedDomains: [], importedDomains: {}, rulesSections: {}, + materializedSkills: {}, }; export async function readState(statePath: string): Promise { @@ -91,6 +94,7 @@ function normalize(parsed: Partial): AgnosState { : [], importedDomains: normalizeStringListMap(parsed.importedDomains), rulesSections: normalizeStringListMap(parsed.rulesSections), + materializedSkills: normalizeStringMap(parsed.materializedSkills), }; } @@ -104,3 +108,12 @@ function normalizeStringListMap(value: unknown): Record { } return out; } + +function normalizeStringMap(value: unknown): Record { + if (!value || typeof value !== "object") return {}; + return Object.fromEntries( + Object.entries(value as Record).filter( + (entry): entry is [string, string] => typeof entry[1] === "string", + ), + ); +} diff --git a/src/core/store-path.ts b/src/core/store-path.ts new file mode 100644 index 0000000..cb71f41 --- /dev/null +++ b/src/core/store-path.ts @@ -0,0 +1,41 @@ +import os from "node:os"; +import path from "node:path"; + +export interface GlobalStorePathOptions { + env?: NodeJS.ProcessEnv; + homeDir?: string; + platform?: NodeJS.Platform; +} + +export function resolveGlobalStoreDir(options: GlobalStorePathOptions = {}): string { + const env = options.env ?? process.env; + const homeDir = options.homeDir ?? os.homedir(); + const platform = options.platform ?? process.platform; + const paths = platform === "win32" ? path.win32 : path.posix; + const override = env["AGNOS_STORE_DIR"]; + const baseDir = override + ? paths.resolve(override) + : resolveDefaultStoreBase(env, homeDir, platform, paths); + return paths.join(baseDir, "v1"); +} + +function resolveDefaultStoreBase( + env: NodeJS.ProcessEnv, + homeDir: string, + platform: NodeJS.Platform, + paths: path.PlatformPath, +): string { + if (platform === "win32") { + return paths.join( + env["LOCALAPPDATA"] ?? paths.join(homeDir, "AppData", "Local"), + "agnos", + "store", + ); + } + if (platform === "darwin") return paths.join(homeDir, "Library", "agnos", "store"); + return paths.join( + env["XDG_DATA_HOME"] ?? paths.join(homeDir, ".local", "share"), + "agnos", + "store", + ); +} diff --git a/src/core/types/public.ts b/src/core/types/public.ts index b4b8381..49b0df8 100644 --- a/src/core/types/public.ts +++ b/src/core/types/public.ts @@ -220,7 +220,17 @@ export interface LogTask extends LogParts { done?: LogInput | ((value: T) => LogInput); } +/** A live TTY log line that can be replaced in place until work completes. */ +export interface LoggerProgress { + /** Replace the progress line with current text. */ + update(msg: LogInput): void; + /** Clear the progress line and release its terminal state. */ + stop(): void; +} + export interface Logger { + /** Start a live progress line. It is inert in quiet and non-TTY output. */ + progress(msg: LogInput): LoggerProgress; info(msg: LogTask): Promise; info(msg: LogInput): void; warn(msg: LogTask): Promise; @@ -244,15 +254,16 @@ export interface Linker { /** * Repository fetcher. Materializes a parsed git or local source into a - * cache-managed directory and returns the root path. The result represents + * transient directory and returns the root path. The result represents * the repository root, not a specific skill — domains walk into it to find * what they need (e.g. domain-skills looks under `./skills/*`). */ export interface RepoFetcher { fetch( source: ParsedSourceRef, - opts?: { ref?: string; noCache?: boolean }, - ): Promise<{ path: string; ref?: string }>; + opts?: { ref?: string; fresh?: boolean }, + ): Promise<{ path: string; ref?: string; commit?: string }>; + cleanup(): Promise; } /** @@ -273,7 +284,7 @@ export type ParsedSourceRef = export interface ResolveContext { agnosRoot: string; projectRoot: string; - cacheDir: string; + storeDir: string; configPath: string; statePath: string; logger: Logger; diff --git a/src/domains/skills/concurrency.ts b/src/domains/skills/concurrency.ts new file mode 100644 index 0000000..cfd1e05 --- /dev/null +++ b/src/domains/skills/concurrency.ts @@ -0,0 +1,33 @@ +const SKILL_CONCURRENCY = 8; + +export async function runSkillTasks( + values: T[], + worker: (value: T) => Promise, +): Promise { + const results = new Array(values.length); + let nextIndex = 0; + + async function runWorker(): Promise { + while (nextIndex < values.length) { + const index = nextIndex; + nextIndex += 1; + const value = values[index]; + if (value === undefined) continue; + results[index] = await worker(value); + } + } + + const workers = Array.from({ length: Math.min(SKILL_CONCURRENCY, values.length) }, () => + runWorker(), + ); + const settled = await Promise.allSettled(workers); + const failure = settled.find( + (result): result is PromiseRejectedResult => result.status === "rejected", + ); + if (failure) { + const reason: unknown = failure.reason; + if (reason instanceof Error) throw reason; + throw new Error("skill task failed", { cause: reason }); + } + return results; +} diff --git a/src/domains/skills/index.ts b/src/domains/skills/index.ts index 0c64624..1fb66a7 100644 --- a/src/domains/skills/index.ts +++ b/src/domains/skills/index.ts @@ -155,7 +155,9 @@ async function diagnose( ctx.logger.info("no skills declared"); return; } - const { steps } = await createSkillSteps(config, ctx); + const { steps } = await createSkillSteps(config, ctx, { + verifyMaterialized: which === "integrity", + }); const bad: string[] = []; await ctx.logger.info({ message: `Checking ${count} skill${count === 1 ? "" : "s"}…`, @@ -440,8 +442,10 @@ const commands: Record = { const handle = await createSkillSteps(config, ctx); const res = await runSkillPipeline(sources, handle.steps, ctx.logger); await handle.flush(); - if (res.installed.length > 0) - ctx.logger.success(`installed ${res.installed.length} skill(s)`); + ctx.logger.success({ + message: "Installing skills 100%", + status: `total ${res.progress.total} | reused ${res.progress.reused} | fetched ${res.progress.fetched}`, + }); }, }, prune: { @@ -575,12 +579,11 @@ export const skillsDomain: Domain = { // bucketed and reported as "Skills need to be updated: …" without throwing, // so the overall run continues (§13.1). const handle = await createSkillSteps(config, ctx); - await ctx.logger.info({ - message: `Downloading and installing ${count} skill${count === 1 ? "" : "s"}`, - waitFor: (async () => { - await runSkillPipeline(sources, handle.steps, ctx.logger); - await handle.flush(); - })(), + const result = await runSkillPipeline(sources, handle.steps, ctx.logger); + await handle.flush(); + ctx.logger.success({ + message: "Installing skills 100%", + status: `total ${result.progress.total} | reused ${result.progress.reused} | fetched ${result.progress.fetched}`, }); return undefined; }, diff --git a/src/domains/skills/pipeline.ts b/src/domains/skills/pipeline.ts index ad2200f..568596b 100644 --- a/src/domains/skills/pipeline.ts +++ b/src/domains/skills/pipeline.ts @@ -1,4 +1,5 @@ import type { Logger } from "../../core/index.js"; +import { runSkillTasks } from "./concurrency.js"; /** * Per-skill preparation buckets. A skill lands in exactly one — the pipeline @@ -15,6 +16,10 @@ export interface FetchResult { src?: string; /** Branch/tag actually fetched (git sources) — threaded to `install` for the lock. */ ref?: string; + /** Commit SHA of the fetched repository checkout. */ + commit?: string; + /** Whether the skill came from the content store or its declared source. */ + source?: "reused" | "fetched"; } /** @@ -32,12 +37,30 @@ export interface SkillSteps { /** Does the content hash match the lock? false → "changed". */ integrity(name: string, src: string): Promise; /** Copy into the canonical dir (copy-if-absent-or-changed); pins/backfills the lock. */ - install(name: string, src: string, ref?: string): Promise; + install(name: string, src: string, ref?: string, commit?: string): Promise; } export interface PipelineResult { buckets: Record; installed: string[]; + progress: SkillInstallProgress; +} + +/** Aggregate progress counters for one skill installation pipeline. */ +export interface SkillInstallProgress { + /** Number of declared skills examined by the pipeline. */ + total: number; + /** Number of skills whose pipeline has completed. */ + completed: number; + /** Number restored from the content-addressed skill store. */ + reused: number; + /** Number loaded from their declared source. */ + fetched: number; +} + +interface SkillResult { + bucket?: Bucket; + installed?: string; } /** @@ -55,18 +78,40 @@ export async function runSkillPipeline( const buckets: Record = { moved: [], changed: [] }; const installed: string[] = []; - for (const [name, ref] of Object.entries(sources)) { - const fetched = await steps.fetch(name, ref); - if (!fetched.ok || !fetched.src) { - buckets.moved.push(name); - continue; - } - if (!(await steps.integrity(name, fetched.src))) { - buckets.changed.push(name); - continue; - } - await steps.install(name, fetched.src, fetched.ref); - installed.push(name); + const entries = Object.entries(sources); + const state: SkillInstallProgress = { + total: entries.length, + completed: 0, + reused: 0, + fetched: 0, + }; + const progress = logger.progress(formatSkillProgress(state)); + let results: SkillResult[]; + try { + results = await runSkillTasks(entries, async ([name, ref]) => { + const fetched = await steps.fetch(name, ref); + let result: SkillResult; + if (!fetched.ok || !fetched.src) { + result = { bucket: "moved" }; + } else if (!(await steps.integrity(name, fetched.src))) { + result = { bucket: "changed" }; + } else { + await steps.install(name, fetched.src, fetched.ref, fetched.commit); + result = { installed: name }; + } + recordSkillProgress(state, fetched.source, progress); + return result; + }); + } finally { + progress.stop(); + } + + for (const [index, result] of results.entries()) { + const entry = entries[index]; + if (!entry) continue; + const [name] = entry; + if (result.bucket) buckets[result.bucket].push(name); + if (result.installed) installed.push(result.installed); } const total = buckets.moved.length + buckets.changed.length; @@ -76,5 +121,28 @@ export async function runSkillPipeline( extra: "run: agnos skills update", }); } - return { buckets, installed }; + return { buckets, installed, progress: state }; +} + +function formatSkillProgress(progress: SkillInstallProgress): { + message: string; + status: string; +} { + const percentage = + progress.total === 0 ? 100 : Math.floor((progress.completed / progress.total) * 100); + return { + message: `Installing skills ${percentage}%`, + status: `total ${progress.total} | reused ${progress.reused} | fetched ${progress.fetched}`, + }; +} + +function recordSkillProgress( + progress: SkillInstallProgress, + source: FetchResult["source"], + reporter: { update(input: { message: string; status: string }): void }, +): void { + progress.completed += 1; + if (source === "reused") progress.reused += 1; + else if (source === "fetched") progress.fetched += 1; + reporter.update(formatSkillProgress(progress)); } diff --git a/src/domains/skills/steps.ts b/src/domains/skills/steps.ts index b3f6042..c892cb9 100644 --- a/src/domains/skills/steps.ts +++ b/src/domains/skills/steps.ts @@ -5,14 +5,19 @@ import { buildPaths, getSkill, hashSkillDir, + materializeSkill, parseCompositeSkillRef, readLock, + readState, removeSkill, resolveGitCommit, resolveLocalCommit, upsertSkill, writeLock, + writeState, } from "../../core/index.js"; +import { ensureStoredSkill, findStoredSkill } from "../../core/skill-store.js"; +import { runSkillTasks } from "./concurrency.js"; import type { SkillSteps } from "./pipeline.js"; const SKILL_MARKER = "SKILL.md"; @@ -33,26 +38,38 @@ interface LocateResult { src: string; /** Branch/tag actually fetched (for git sources) — persisted to the lock. */ ref?: string; + /** Commit SHA of the checkout containing the skill. */ + commit?: string; } /** * Locate the fetched skill content for a composite ref (or null if missing). - * `lockedRef` is the branch/tag recorded in the lock: passing it as the explicit - * fetch ref lets `fetchGit` skip the `ls-remote` default-branch lookup and hit - * the cache directly, so warm runs are fully offline. + * An explicit checkout ref may be a locked commit while `trackedRef` remains + * the symbolic branch or tag persisted for freshness checks. */ async function locate( composite: string, ctx: ResolveContext, - lockedRef?: string, + options?: { checkoutRef?: string; trackedRef?: string; fresh?: boolean }, ): Promise { const parsed = parseCompositeSkillRef(composite, { projectRoot: ctx.projectRoot }); const ownRef = parsed.source.kind === "git" ? parsed.source.ref : undefined; - const explicit = ownRef ?? lockedRef; - const fetched = await ctx.fetcher.fetch(parsed.source, explicit ? { ref: explicit } : undefined); + const checkoutRef = options?.checkoutRef ?? ownRef; + const fetchOptions = { + ...(checkoutRef ? { ref: checkoutRef } : {}), + ...(options?.fresh ? { fresh: true } : {}), + }; + const fetched = await ctx.fetcher.fetch( + parsed.source, + Object.keys(fetchOptions).length > 0 ? fetchOptions : undefined, + ); const src = parsed.source.kind === "git" ? path.join(fetched.path, parsed.subPath) : fetched.path; if (!(await isSkillDir(src))) return null; - return { src, ...(fetched.ref ? { ref: fetched.ref } : {}) }; + return { + src, + ...((options?.trackedRef ?? fetched.ref) ? { ref: options?.trackedRef ?? fetched.ref } : {}), + ...(fetched.commit ? { commit: fetched.commit } : {}), + }; } /** Best-effort upstream commit for the ref (undefined on any failure / no network). */ @@ -75,6 +92,10 @@ export interface SkillStepsHandle { flush(): Promise; } +export interface CreateSkillStepsOptions { + verifyMaterialized?: boolean; +} + export interface PruneSkillsResult { removed: string[]; unpinned: string[]; @@ -89,7 +110,9 @@ export async function pruneSkills( const desiredSources = new Set(Object.values(sources)); const skillsDir = buildPaths(ctx.projectRoot, config).skillsDir; let lock = await readLock(ctx.projectRoot); + const state = await readState(ctx.statePath); let dirty = false; + let stateDirty = false; const result: PruneSkillsResult = { removed: [], unpinned: [] }; let children: string[]; @@ -109,6 +132,12 @@ export async function pruneSkills( ctx.logger.info(`would: remove skill "${name}"`); } else { await fs.rm(candidate, { recursive: true, force: true }); + if (state.materializedSkills?.[name]) { + state.materializedSkills = Object.fromEntries( + Object.entries(state.materializedSkills).filter(([skillName]) => skillName !== name), + ); + stateDirty = true; + } } } @@ -124,6 +153,7 @@ export async function pruneSkills( } if (dirty) await writeLock(ctx.projectRoot, lock); + if (stateDirty) await writeState(ctx.statePath, state); return result; } @@ -131,16 +161,20 @@ export async function pruneSkills( * Concrete `SkillSteps` over the real fetcher + lock + content hash. The * `version` step compares the lock's `resolvedCommit` to the upstream HEAD * (treating an absent baseline or a network failure as "current" rather than - * false-alarming); `install` is copy-if-changed and pins new skills. + * false-alarming); `install` imports content-addressed stored skills and pins + * new skills. */ export async function createSkillSteps( config: AgnosConfig, ctx: ResolveContext, + options?: CreateSkillStepsOptions, ): Promise { const sources = config.skills?.sources ?? {}; const skillsDir = buildPaths(ctx.projectRoot, config).skillsDir; let lock = await readLock(ctx.projectRoot); + const state = await readState(ctx.statePath); let dirty = false; + let stateDirty = false; // Hash each fetched source directory at most once per run — `integrity` and // `install` both need the source hash, and the source tree is immutable. @@ -160,12 +194,52 @@ export async function createSkillSteps( }; const steps: SkillSteps = { - async fetch(_name, composite) { + async fetch(name, composite) { try { - const lockedRef = getSkill(lock, composite)?.ref; - const located = await locate(composite, ctx, lockedRef); + const parsed = parseCompositeSkillRef(composite, { projectRoot: ctx.projectRoot }); + const entry = getSkill(lock, composite); + if (parsed.source.kind === "git" && entry) { + let stored = await findStoredSkill(ctx.storeDir, entry.computedHash); + if (!stored) { + const legacyStore = path.join(ctx.agnosRoot, "cache"); + const legacy = await findStoredSkill(legacyStore, entry.computedHash); + if (legacy) stored = await ensureStoredSkill(legacy, ctx.storeDir, entry.computedHash); + } + if (!stored) { + const materialized = path.join(skillsDir, name); + if ((await hashSkillDir(materialized).catch(() => null)) === entry.computedHash) { + stored = await ensureStoredSkill(materialized, ctx.storeDir, entry.computedHash); + } + } + if (stored && entry.resolvedCommit) { + srcHashes.set(stored, entry.computedHash); + const ref = parsed.source.ref ?? entry.ref; + return { + ok: true, + src: stored, + source: "reused", + ...(ref ? { ref } : {}), + ...(entry.resolvedCommit ? { commit: entry.resolvedCommit } : {}), + }; + } + } + + const trackedRef = + parsed.source.kind === "git" ? (parsed.source.ref ?? entry?.ref) : undefined; + const checkoutRef = + parsed.source.kind === "git" ? (entry?.resolvedCommit ?? trackedRef) : undefined; + const located = await locate(composite, ctx, { + ...(checkoutRef ? { checkoutRef } : {}), + ...(trackedRef ? { trackedRef } : {}), + }); return located - ? { ok: true, src: located.src, ...(located.ref ? { ref: located.ref } : {}) } + ? { + ok: true, + src: located.src, + source: "fetched", + ...(located.ref ? { ref: located.ref } : {}), + ...(located.commit ? { commit: located.commit } : {}), + } : { ok: false }; } catch { return { ok: false }; @@ -181,9 +255,10 @@ export async function createSkillSteps( async integrity(name, src) { const entry = getSkill(lock, compositeOf(name)); if (!entry) return true; // unpinned → install will pin it - return (await hashOnce(src)) === entry.computedHash; + const target = options?.verifyMaterialized ? path.join(skillsDir, name) : src; + return (await hashOnce(target)) === entry.computedHash; }, - async install(name, src, ref) { + async install(name, src, ref, commit) { const composite = compositeOf(name); if (ctx.dryRun) { ctx.logger.info(`would: install skill "${name}"`); @@ -191,15 +266,18 @@ export async function createSkillSteps( } const dst = path.join(skillsDir, name); const srcHash = await hashOnce(src); - const dstHash = (await isSkillDir(dst)) ? await hashSkillDir(dst) : null; - if (dstHash !== srcHash) { - await fs.rm(dst, { recursive: true, force: true }); - await fs.mkdir(path.dirname(dst), { recursive: true }); - await fs.cp(src, dst, { recursive: true, force: true }); + const stored = await ensureStoredSkill(src, ctx.storeDir, srcHash); + await materializeSkill(stored, dst, srcHash, state.materializedSkills?.[name]); + if (state.materializedSkills?.[name] !== srcHash) { + state.materializedSkills = { ...(state.materializedSkills ?? {}), [name]: srcHash }; + stateDirty = true; } const existing = getSkill(lock, composite); if (!existing) { - const commit = await resolveCommit(composite, ctx); + const parsed = parseCompositeSkillRef(composite, { projectRoot: ctx.projectRoot }); + if (parsed.source.kind === "git" && !commit) { + throw new Error(`could not determine the checked-out commit for skill "${name}"`); + } lock = upsertSkill(lock, composite, { computedHash: srcHash, resolvedAt: new Date().toISOString(), @@ -207,10 +285,12 @@ export async function createSkillSteps( ...(ref ? { ref } : {}), }); dirty = true; - } else if (ref && !existing.ref) { - // Backfill the tracked ref for legacy lock entries so subsequent runs - // fetch offline (no `ls-remote`). Self-heals after one run. - lock = upsertSkill(lock, composite, { ...existing, ref }); + } else if ((ref && !existing.ref) || (commit && !existing.resolvedCommit)) { + lock = upsertSkill(lock, composite, { + ...existing, + ...(ref ? { ref } : {}), + ...(commit ? { resolvedCommit: commit } : {}), + }); dirty = true; } }, @@ -220,6 +300,10 @@ export async function createSkillSteps( steps, async flush() { if (dirty && !ctx.dryRun) await writeLock(ctx.projectRoot, lock); + if (stateDirty && !ctx.dryRun) await writeState(ctx.statePath, state); + if (!ctx.dryRun) { + await cleanupLegacyCache(ctx, skillsDir, lock, Object.values(sources)); + } }, }; } @@ -235,33 +319,93 @@ export async function updateSkills( ctx: ResolveContext, ): Promise { const sources = config.skills?.sources ?? {}; - const targets = names.length > 0 ? names : Object.keys(sources); + const targets = [...new Set(names.length > 0 ? names : Object.keys(sources))]; const skillsDir = buildPaths(ctx.projectRoot, config).skillsDir; let lock = await readLock(ctx.projectRoot); - const updated: string[] = []; - - for (const name of targets) { + const state = await readState(ctx.statePath); + const updates = await runSkillTasks(targets, async (name) => { const composite = sources[name]; if (!composite) throw new Error(`skill "${name}" is not declared`); - const located = await locate(composite, ctx, getSkill(lock, composite)?.ref); + const parsed = parseCompositeSkillRef(composite, { projectRoot: ctx.projectRoot }); + const trackedRef = + parsed.source.kind === "git" + ? (parsed.source.ref ?? getSkill(lock, composite)?.ref) + : undefined; + const located = await locate(composite, ctx, { + ...(trackedRef ? { checkoutRef: trackedRef, trackedRef } : {}), + fresh: true, + }); if (!located) throw new Error(`skill "${name}" not found at ${composite}`); const hash = await hashSkillDir(located.src); - const commit = await resolveCommit(composite, ctx); - lock = upsertSkill(lock, composite, { - computedHash: hash, - resolvedAt: new Date().toISOString(), - ...(commit ? { resolvedCommit: commit } : {}), - ...(located.ref ? { ref: located.ref } : {}), - }); + const commit = located.commit; + if (parsed.source.kind === "git" && !commit) { + throw new Error(`could not determine the checked-out commit for skill "${name}"`); + } if (!ctx.dryRun) { const dst = path.join(skillsDir, name); - await fs.rm(dst, { recursive: true, force: true }); - await fs.mkdir(path.dirname(dst), { recursive: true }); - await fs.cp(located.src, dst, { recursive: true, force: true }); + const stored = await ensureStoredSkill(located.src, ctx.storeDir, hash); + await materializeSkill(stored, dst, hash); + state.materializedSkills = { ...(state.materializedSkills ?? {}), [name]: hash }; + } + return { name, composite, hash, commit, ref: located.ref }; + }); + + for (const update of updates) { + lock = upsertSkill(lock, update.composite, { + computedHash: update.hash, + resolvedAt: new Date().toISOString(), + ...(update.commit ? { resolvedCommit: update.commit } : {}), + ...(update.ref ? { ref: update.ref } : {}), + }); + } + + if (!ctx.dryRun && updates.length > 0) { + await writeLock(ctx.projectRoot, lock); + await writeState(ctx.statePath, state); + await cleanupLegacyCache(ctx, skillsDir, lock, Object.values(sources)); + } + return updates.map(({ name }) => name); +} + +async function cleanupLegacyCache( + ctx: ResolveContext, + skillsDir: string, + lock: Awaited>, + declaredSources: string[], +): Promise { + const legacyCache = path.join(ctx.agnosRoot, "cache"); + const stat = await fs.lstat(legacyCache).catch(() => null); + if (!stat) return; + + for (const source of declaredSources) { + const entry = getSkill(lock, source); + if (!entry) continue; + if (!(await findStoredSkill(ctx.storeDir, entry.computedHash))) { + ctx.logger.warn("legacy skill cache retained because migration is incomplete"); + return; + } + } + + const children = await fs.readdir(skillsDir).catch(() => []); + for (const child of children) { + const materialized = path.join(skillsDir, child); + const childStat = await fs.lstat(materialized).catch(() => null); + if (!childStat?.isSymbolicLink()) continue; + const target = await fs.realpath(materialized).catch(() => null); + if (target && isInside(target, legacyCache)) { + ctx.logger.warn(`could not remove legacy skill cache because "${child}" still links to it`); + return; } - updated.push(name); } - if (!ctx.dryRun && updated.length > 0) await writeLock(ctx.projectRoot, lock); - return updated; + try { + await fs.rm(legacyCache, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 }); + } catch (error) { + ctx.logger.warn(`could not remove legacy skill cache: ${(error as Error).message}`); + } +} + +function isInside(candidate: string, parent: string): boolean { + const relative = path.relative(parent, candidate); + return relative !== "" && !relative.startsWith("..") && !path.isAbsolute(relative); } diff --git a/test/agents/adapters.test.ts b/test/agents/adapters.test.ts index 6d65675..0affdc0 100644 --- a/test/agents/adapters.test.ts +++ b/test/agents/adapters.test.ts @@ -16,7 +16,7 @@ function ctxFor(root: string): MaterializeContext { return { agnosRoot: root, projectRoot: root, - cacheDir: path.join(root, ".agnos", "cache"), + storeDir: path.join(root, "store"), configPath: path.join(root, "agnos.json"), statePath: path.join(root, ".agnos", "state.json"), logger: createLogger({ quiet: true }), diff --git a/test/agents/cleanup.test.ts b/test/agents/cleanup.test.ts index 6d047a8..4f494ff 100644 --- a/test/agents/cleanup.test.ts +++ b/test/agents/cleanup.test.ts @@ -13,7 +13,7 @@ function ctxFor(root: string): MaterializeContext { return { agnosRoot: root, projectRoot: root, - cacheDir: path.join(root, ".agnos", "cache"), + storeDir: path.join(root, "store"), configPath: path.join(root, "agnos.json"), statePath: path.join(root, ".agnos", "state.json"), logger: createLogger({ quiet: true }), diff --git a/test/agents/idempotency.test.ts b/test/agents/idempotency.test.ts index 710a503..b2eabb5 100644 --- a/test/agents/idempotency.test.ts +++ b/test/agents/idempotency.test.ts @@ -12,7 +12,7 @@ let tmp: string; const matCtx = (root: string): MaterializeContext => ({ agnosRoot: root, projectRoot: root, - cacheDir: path.join(root, ".agnos", "cache"), + storeDir: path.join(root, "store"), configPath: path.join(root, "agnos.json"), statePath: path.join(root, ".agnos", "state.json"), logger: createLogger({ quiet: true }), diff --git a/test/core/context.test.ts b/test/core/context.test.ts new file mode 100644 index 0000000..0b89cc0 --- /dev/null +++ b/test/core/context.test.ts @@ -0,0 +1,25 @@ +import { afterEach, describe, expect, it } from "vitest"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { buildResolveContext } from "../../src/core/context.js"; + +const roots: string[] = []; + +afterEach(async () => { + await Promise.all(roots.splice(0).map((root) => fs.rm(root, { recursive: true, force: true }))); +}); + +describe("buildResolveContext", () => { + it("uses an injected global store and project-local temporary work", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "agnos-context-")); + roots.push(root); + const storeDir = path.join(root, "global-store"); + + const ctx = await buildResolveContext({ projectRoot: root, storeDir }); + + expect(ctx.storeDir).toBe(storeDir); + await expect(fs.access(path.join(root, ".agnos", "tmp"))).resolves.toBeUndefined(); + await ctx.fetcher.cleanup(); + }); +}); diff --git a/test/core/ensure-link.test.ts b/test/core/ensure-link.test.ts index 7f9a696..96e724d 100644 --- a/test/core/ensure-link.test.ts +++ b/test/core/ensure-link.test.ts @@ -16,7 +16,7 @@ describe("ensureLink", () => { function linker() { return createLinker({ - cacheDir: path.join(dir, ".cache"), + probeDir: path.join(dir, ".tmp"), logger: createLogger(), copyFallback: true, }); diff --git a/test/core/init-steps.test.ts b/test/core/init-steps.test.ts index 96b9919..3ae6df7 100644 --- a/test/core/init-steps.test.ts +++ b/test/core/init-steps.test.ts @@ -20,7 +20,7 @@ const recordingLogger = (): Logger => ({ const ctxFor = (): ResolveContext => ({ agnosRoot: tmp, projectRoot: tmp, - cacheDir: path.join(tmp, ".agnos", "cache"), + storeDir: path.join(tmp, "store"), configPath: path.join(tmp, "agnos.json"), statePath: path.join(tmp, ".agnos", "state.json"), logger: recordingLogger(), diff --git a/test/core/link.test.ts b/test/core/link.test.ts index 7b464f6..2b2028e 100644 --- a/test/core/link.test.ts +++ b/test/core/link.test.ts @@ -7,12 +7,12 @@ import { createLogger } from "../../src/core/logger.js"; describe("createLinker", () => { let dir: string; - let cacheDir: string; + let probeDir: string; beforeEach(async () => { dir = await fs.mkdtemp(path.join(os.tmpdir(), "agnos-link-")); - cacheDir = path.join(dir, "cache"); - await fs.mkdir(cacheDir, { recursive: true }); + probeDir = path.join(dir, "tmp"); + await fs.mkdir(probeDir, { recursive: true }); }); afterEach(async () => { @@ -24,7 +24,7 @@ describe("createLinker", () => { await fs.mkdir(target); await fs.writeFile(path.join(target, "marker.txt"), "hello"); - const linker = createLinker({ cacheDir, logger: createLogger() }); + const linker = createLinker({ probeDir, logger: createLogger() }); const linkPath = path.join(dir, "link"); const { kind } = await linker.link(target, linkPath); expect(["symlink", "junction"]).toContain(kind); @@ -36,14 +36,14 @@ describe("createLinker", () => { it("removes existing link before creating a new one (idempotency)", async () => { const target = path.join(dir, "target-dir"); await fs.mkdir(target); - const linker = createLinker({ cacheDir, logger: createLogger() }); + const linker = createLinker({ probeDir, logger: createLogger() }); const linkPath = path.join(dir, "link"); await linker.link(target, linkPath); await linker.link(target, linkPath); // should not throw }); it("canSymlinkDirs always returns true", async () => { - const linker = createLinker({ cacheDir, logger: createLogger() }); + const linker = createLinker({ probeDir, logger: createLogger() }); expect(await linker.canSymlinkDirs()).toBe(true); }); }); diff --git a/test/core/lock.test.ts b/test/core/lock.test.ts index 97b8c04..9248a74 100644 --- a/test/core/lock.test.ts +++ b/test/core/lock.test.ts @@ -69,6 +69,19 @@ describe("lock (per-skill)", () => { expect(raw.indexOf('"a-key"')).toBeLessThan(raw.indexOf('"z-key"')); }); + it("publishes concurrent equivalent writes atomically", async () => { + const root = await tmpRoot(); + const lock = upsertSkill(emptyLock(), "github:foo/bar/skills/pdf", { + computedHash: "a".repeat(64), + resolvedAt: "2026-08-13T00:00:00.000Z", + }); + + await Promise.all(Array.from({ length: 16 }, async () => writeLock(root, lock))); + + expect(await readLock(root)).toEqual(lock); + expect((await fs.readdir(root)).filter((name) => name.endsWith(".tmp"))).toEqual([]); + }); + it("rejects invalid JSON", async () => { const root = await tmpRoot(); await fs.writeFile(path.join(root, "agnos.lock.json"), "not json"); diff --git a/test/core/logger-progress.test.ts b/test/core/logger-progress.test.ts new file mode 100644 index 0000000..bb6d9a6 --- /dev/null +++ b/test/core/logger-progress.test.ts @@ -0,0 +1,38 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { createLogger } from "../../src/core/logger.js"; + +const originalIsTTY = Object.getOwnPropertyDescriptor(process.stderr, "isTTY"); + +afterEach(() => { + vi.restoreAllMocks(); + if (originalIsTTY) Object.defineProperty(process.stderr, "isTTY", originalIsTTY); + else Reflect.deleteProperty(process.stderr, "isTTY"); +}); + +describe("logger progress", () => { + it("replaces a live TTY progress line and clears it on completion", () => { + Object.defineProperty(process.stderr, "isTTY", { configurable: true, value: true }); + const writes: string[] = []; + vi.spyOn(process.stderr, "write").mockImplementation((value) => { + writes.push(String(value)); + return true; + }); + const logger = createLogger(); + + const progress = logger.progress({ + message: "Installing skills 0%", + status: "total 4 | reused 0 | fetched 0", + }); + progress.update({ + message: "Installing skills 50%", + status: "total 4 | reused 1 | fetched 1", + }); + progress.stop(); + + const output = writes.join(""); + expect(output).toContain("Installing skills 0%"); + expect(output).toContain("Installing skills 50%"); + expect(output).toContain("total 4 | reused 1 | fetched 1"); + expect(output).toContain("\x1b[?25h"); + }); +}); diff --git a/test/core/resolver-nocache.test.ts b/test/core/resolver.test.ts similarity index 71% rename from test/core/resolver-nocache.test.ts rename to test/core/resolver.test.ts index e67dad2..5197d96 100644 --- a/test/core/resolver-nocache.test.ts +++ b/test/core/resolver.test.ts @@ -30,6 +30,10 @@ function installGitMock(): void { cb(null, { stdout: "ref: refs/heads/main\tHEAD\n", stderr: "" }); return; } + if (gitArgs.includes("rev-parse")) { + cb(null, { stdout: "deadbeef\n", stderr: "" }); + return; + } if (gitArgs[0] === "clone") { // dest is the last positional arg; drop a SKILL.md so the dir is non-empty. const dest = gitArgs[gitArgs.length - 1]!; @@ -60,8 +64,7 @@ describe("createRepoFetcher (sparse git clone)", () => { function fetcher() { return createRepoFetcher({ - projectRoot: root, - cacheDir: path.join(root, ".agnos", "cache"), + stagingDir: path.join(root, ".agnos", "tmp", "repos"), }); } @@ -72,6 +75,7 @@ describe("createRepoFetcher (sparse git clone)", () => { const res = await fetcher().fetch(source); expect(res.ref).toBe("main"); // from the mocked ls-remote symref + expect(res.commit).toBe("deadbeef"); await expect( fs.access(path.join(res.path, "skills", "demo", "SKILL.md")), ).resolves.toBeUndefined(); @@ -95,24 +99,41 @@ describe("createRepoFetcher (sparse git clone)", () => { expect(sparse[sparse.length - 1]).toBe("skills/pdf"); }); - it("reuses the cached clone on a second fetch", async () => { + it("reuses a staged checkout during the same session", async () => { const source = parseSource("github:vercel-labs/agent-skills", { projectRoot: root }); if (source.kind !== "git") throw new Error("expected git source"); + const repoFetcher = fetcher(); - await fetcher().fetch(source); + await repoFetcher.fetch(source); const clonesAfterFirst = calls.filter((c) => c[1] === "clone").length; - await fetcher().fetch(source); + await repoFetcher.fetch(source); const clonesAfterSecond = calls.filter((c) => c[1] === "clone").length; expect(clonesAfterFirst).toBe(1); - expect(clonesAfterSecond).toBe(1); // no re-clone + expect(clonesAfterSecond).toBe(1); }); - it("re-clones when noCache is set", async () => { + it("coalesces concurrent fetches for the same source", async () => { const source = parseSource("github:vercel-labs/agent-skills", { projectRoot: root }); if (source.kind !== "git") throw new Error("expected git source"); + const repoFetcher = fetcher(); - await fetcher().fetch(source); - await fetcher().fetch(source, { noCache: true }); + const [first, second] = await Promise.all([ + repoFetcher.fetch(source), + repoFetcher.fetch(source), + ]); + + expect(first.path).toBe(second.path); + expect(calls.filter((c) => c[1] === "ls-remote")).toHaveLength(1); + expect(calls.filter((c) => c[1] === "clone")).toHaveLength(1); + }); + + it("re-clones when fresh is set", async () => { + const source = parseSource("github:vercel-labs/agent-skills", { projectRoot: root }); + if (source.kind !== "git") throw new Error("expected git source"); + const repoFetcher = fetcher(); + + await repoFetcher.fetch(source); + await repoFetcher.fetch(source, { fresh: true }); expect(calls.filter((c) => c[1] === "clone").length).toBe(2); }); @@ -126,4 +147,18 @@ describe("createRepoFetcher (sparse git clone)", () => { const clone = calls.find((c) => c[1] === "clone")!; expect(clone[clone.indexOf("--branch") + 1]).toBe("canary"); }); + + it("removes every checkout created during the session", async () => { + const source = parseSource("github:vercel-labs/agent-skills", { projectRoot: root }); + if (source.kind !== "git") throw new Error("expected git source"); + const repoFetcher = fetcher(); + + const fetched = await repoFetcher.fetch(source); + await expect(fs.access(fetched.path)).resolves.toBeUndefined(); + + await repoFetcher.cleanup(); + + await expect(fs.access(fetched.path)).rejects.toThrow(); + await expect(fs.access(path.join(root, ".agnos", "cache", "repos"))).rejects.toThrow(); + }); }); diff --git a/test/core/run.test.ts b/test/core/run.test.ts new file mode 100644 index 0000000..e3f4128 --- /dev/null +++ b/test/core/run.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it, vi } from "vitest"; +import path from "node:path"; +import { createLogger } from "../../src/core/logger.js"; +import { runOne } from "../../src/core/run.js"; +import type { PluginRegistry } from "../../src/core/plugin-loader.js"; +import type { Domain, RunContext } from "../../src/core/types/public.js"; + +const OPTIONS = { dry: false, once: true, quiet: true, interactive: false }; + +function buildRegistry(domain: Domain): PluginRegistry { + return { + agents: new Map(), + agentsByPackage: new Map(), + domains: new Map([[domain.id, { domain, packageName: "test" }]]), + collisions: [], + }; +} + +function buildContext(cleanup: () => Promise): RunContext { + const root = path.resolve("test-project"); + return { + agnosRoot: path.join(root, ".agnos"), + projectRoot: root, + storeDir: path.join(root, "store"), + configPath: path.join(root, "agnos.json"), + statePath: path.join(root, ".agnos", "state.json"), + logger: createLogger({ quiet: true }), + fetcher: { fetch: vi.fn(), cleanup } as never, + linker: {} as never, + flags: { dry: false, once: true, quiet: true, help: false, init: false, yes: true }, + }; +} + +describe("domain repository staging cleanup", () => { + it("cleans transient checkouts when a domain run fails", async () => { + const cleanup = vi.fn(async () => {}); + const failure = new Error("domain failed"); + const domain: Domain = { + id: "test", + description: "test", + kind: "writer", + priority: 1, + run: async () => { + throw failure; + }, + }; + + await expect( + runOne(buildRegistry(domain), domain.id, OPTIONS, buildContext(cleanup)), + ).rejects.toBe(failure); + expect(cleanup).toHaveBeenCalledOnce(); + }); +}); diff --git a/test/core/skill-materialize.test.ts b/test/core/skill-materialize.test.ts new file mode 100644 index 0000000..7601ae6 --- /dev/null +++ b/test/core/skill-materialize.test.ts @@ -0,0 +1,69 @@ +import { constants } from "node:fs"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { hashSkillDir } from "../../src/core/skill-hash.js"; +import { materializeSkill } from "../../src/core/skill-materialize.js"; + +let root: string; +let stored: string; +let destination: string; +let hash: string; + +beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), "agnos-materialize-")); + stored = path.join(root, "store", "skill"); + destination = path.join(root, "project", "skill"); + await fs.mkdir(path.join(stored, "nested"), { recursive: true }); + await fs.writeFile(path.join(stored, "SKILL.md"), "# Test\n"); + await fs.writeFile(path.join(stored, "nested", "data.txt"), "data\n"); + hash = await hashSkillDir(stored); +}); + +afterEach(async () => { + vi.restoreAllMocks(); + await fs.rm(root, { recursive: true, force: true }); +}); + +describe("materializeSkill", () => { + it("imports an independent project tree and skips an unchanged materialization", async () => { + const first = await materializeSkill(stored, destination, hash); + expect(first.changed).toBe(true); + expect(await hashSkillDir(destination)).toBe(hash); + + await fs.rm(path.join(root, "store"), { recursive: true, force: true }); + expect(await fs.readFile(path.join(destination, "SKILL.md"), "utf8")).toBe("# Test\n"); + + const second = await materializeSkill(destination, destination, hash, hash); + expect(second.changed).toBe(false); + }); + + it("falls back to hard links when forced cloning is unavailable", async () => { + const copyFile = fs.copyFile.bind(fs); + vi.spyOn(fs, "copyFile").mockImplementation(async (source, target, mode) => { + if (mode === constants.COPYFILE_FICLONE_FORCE) throw new Error("clone unavailable"); + await copyFile(source, target, mode); + }); + + const result = await materializeSkill(stored, destination, hash); + + expect(result.methods).toEqual(new Set(["hardlink"])); + expect(await hashSkillDir(destination)).toBe(hash); + }); + + it("falls back to copies when cloning and hard links are unavailable", async () => { + const copyFile = fs.copyFile.bind(fs); + const link = vi.spyOn(fs, "link").mockRejectedValue(new Error("cross-device")); + vi.spyOn(fs, "copyFile").mockImplementation(async (source, target, mode) => { + if (mode === constants.COPYFILE_FICLONE_FORCE) throw new Error("clone unavailable"); + await copyFile(source, target, mode); + }); + + const result = await materializeSkill(stored, destination, hash); + + expect(link).toHaveBeenCalled(); + expect(result.methods).toEqual(new Set(["copy"])); + expect(await hashSkillDir(destination)).toBe(hash); + }); +}); diff --git a/test/core/skill-prepare.test.ts b/test/core/skill-prepare.test.ts index 987ce2f..b48019c 100644 --- a/test/core/skill-prepare.test.ts +++ b/test/core/skill-prepare.test.ts @@ -6,6 +6,7 @@ import { prepareSkills } from "../../src/core/skill-prepare.js"; import { hashSkillDir } from "../../src/core/skill-hash.js"; import { readLock, writeLock, upsertSkill, emptyLock } from "../../src/core/lock.js"; import { createLogger } from "../../src/core/logger.js"; +import { createLinker } from "../../src/core/fs/link.js"; import type { AgnosConfig, ResolveContext } from "../../src/core/types/public.js"; let root: string; @@ -28,22 +29,22 @@ async function seedSkill(rel: string, body = "# Test\n\n"): Promise { } function makeCtx(): ResolveContext { + const storeDir = path.join(root, "store"); + const logger = createLogger({ quiet: true }); return { projectRoot: root, configPath: path.join(root, "agnos.json"), statePath: path.join(root, ".agnos", "state.json"), agnosRoot: path.join(root, ".agnos"), - cacheDir: path.join(root, ".agnos", "cache"), - logger: createLogger({ quiet: true }), + storeDir, + logger, // Stub fetcher: every call returns the shared repoCache root. The composite // ref's subPath then locates the actual skill within it. - fetcher: { fetch: async () => ({ path: repoCache }) }, - linker: { - canSymlinkFiles: async () => true, - canSymlinkDirs: async () => true, - link: async () => ({ kind: "symlink" }), - unlink: async () => {}, + fetcher: { + fetch: async () => ({ path: repoCache, ref: "main", commit: "deadbeef" }), + cleanup: async () => {}, }, + linker: createLinker({ probeDir: path.join(root, ".agnos", "tmp"), logger }), }; } diff --git a/test/core/skill-store.test.ts b/test/core/skill-store.test.ts new file mode 100644 index 0000000..471166a --- /dev/null +++ b/test/core/skill-store.test.ts @@ -0,0 +1,82 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { ensureStoredSkill, resolveStoredSkill } from "../../src/core/skill-store.js"; +import { hashSkillDir } from "../../src/core/skill-hash.js"; + +let root: string; + +beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), "agnos-skill-store-")); +}); + +afterEach(async () => { + vi.restoreAllMocks(); + await fs.rm(root, { recursive: true, force: true }); +}); + +describe("skill store publication", () => { + it("retries a transient Windows rename failure without deleting the final path", async () => { + const source = path.join(root, "source"); + const storeDir = path.join(root, "store"); + await fs.mkdir(source, { recursive: true }); + await fs.writeFile(path.join(source, "SKILL.md"), "# Test\n"); + const hash = await hashSkillDir(source); + const stored = resolveStoredSkill(storeDir, hash); + const rename = fs.rename.bind(fs); + let attempts = 0; + const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async (from, to) => { + attempts += 1; + if (attempts === 1) { + const error = new Error("file is busy") as NodeJS.ErrnoException; + error.code = "EPERM"; + throw error; + } + await rename(from, to); + }); + const rmSpy = vi.spyOn(fs, "rm"); + + expect(await ensureStoredSkill(source, storeDir, hash)).toBe(stored); + expect(renameSpy).toHaveBeenCalledTimes(2); + expect(rmSpy.mock.calls.some(([target]) => path.resolve(String(target)) === stored)).toBe( + false, + ); + expect(await fs.readFile(path.join(stored, "SKILL.md"), "utf8")).toBe("# Test\n"); + }); + + it("replaces a corrupt entry before reuse", async () => { + const source = path.join(root, "source"); + const storeDir = path.join(root, "store"); + await fs.mkdir(source, { recursive: true }); + await fs.writeFile(path.join(source, "SKILL.md"), "# Valid\n"); + const hash = await hashSkillDir(source); + const stored = resolveStoredSkill(storeDir, hash); + await fs.mkdir(stored, { recursive: true }); + await fs.writeFile(path.join(stored, "SKILL.md"), "# Corrupt\n"); + + await ensureStoredSkill(source, storeDir, hash); + + expect(await fs.readFile(path.join(stored, "SKILL.md"), "utf8")).toBe("# Valid\n"); + }); + + it("does not publish symbolic links excluded from the skill hash", async () => { + const source = path.join(root, "source"); + const external = path.join(root, "external"); + const storeDir = path.join(root, "store"); + await fs.mkdir(source, { recursive: true }); + await fs.mkdir(external, { recursive: true }); + await fs.writeFile(path.join(source, "SKILL.md"), "# Valid\n"); + await fs.writeFile(path.join(external, "secret.txt"), "secret\n"); + await fs.symlink( + external, + path.join(source, "linked"), + process.platform === "win32" ? "junction" : "dir", + ); + const hash = await hashSkillDir(source); + + const stored = await ensureStoredSkill(source, storeDir, hash); + + await expect(fs.access(path.join(stored, "linked"))).rejects.toThrow(); + }); +}); diff --git a/test/core/state.test.ts b/test/core/state.test.ts index 5b0328d..7c0e9f8 100644 --- a/test/core/state.test.ts +++ b/test/core/state.test.ts @@ -36,6 +36,7 @@ describe("state", () => { let s = await readState(statePath); s = markAgentInstalled(s, "claude-code"); s = markDomainInitialized(s, "skills"); + s.materializedSkills = { pdf: "a".repeat(64) }; await writeState(statePath, s); const reloaded = await readState(statePath); expect(reloaded).toEqual(s); diff --git a/test/core/store-path.test.ts b/test/core/store-path.test.ts new file mode 100644 index 0000000..298ab5a --- /dev/null +++ b/test/core/store-path.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it } from "vitest"; +import path from "node:path"; +import { resolveGlobalStoreDir } from "../../src/core/store-path.js"; + +describe("resolveGlobalStoreDir", () => { + it("uses AGNOS_STORE_DIR when configured", () => { + expect( + resolveGlobalStoreDir({ env: { AGNOS_STORE_DIR: "/shared/agnos" }, platform: "linux" }), + ).toBe(path.posix.resolve("/shared/agnos", "v1")); + }); + + it("uses the platform user data directory", () => { + expect( + resolveGlobalStoreDir({ + env: { LOCALAPPDATA: "C:\\Users\\test\\AppData\\Local" }, + homeDir: "C:\\Users\\test", + platform: "win32", + }), + ).toBe(path.win32.join("C:\\Users\\test\\AppData\\Local", "agnos", "store", "v1")); + expect(resolveGlobalStoreDir({ env: {}, homeDir: "/Users/test", platform: "darwin" })).toBe( + path.posix.join("/Users/test", "Library", "agnos", "store", "v1"), + ); + expect(resolveGlobalStoreDir({ env: {}, homeDir: "/home/test", platform: "linux" })).toBe( + path.posix.join("/home/test", ".local", "share", "agnos", "store", "v1"), + ); + }); +}); diff --git a/test/core/watch.test.ts b/test/core/watch.test.ts index aa50bef..d50249c 100644 --- a/test/core/watch.test.ts +++ b/test/core/watch.test.ts @@ -14,7 +14,7 @@ function ctxFor(root: string): RunContext { return { agnosRoot: root, projectRoot: root, - cacheDir: path.join(root, ".agnos", "cache"), + storeDir: path.join(root, "store"), configPath: path.join(root, "agnos.json"), statePath: path.join(root, ".agnos", "state.json"), logger: createLogger({ quiet: true }), diff --git a/test/docs/compile.test.ts b/test/docs/compile.test.ts index 9a6c43f..481348d 100644 --- a/test/docs/compile.test.ts +++ b/test/docs/compile.test.ts @@ -13,7 +13,7 @@ function ctxFor(root: string): ResolveContext { return { agnosRoot: root, projectRoot: root, - cacheDir: path.join(root, ".agnos", "cache"), + storeDir: path.join(root, "store"), configPath: path.join(root, "agnos.json"), statePath: path.join(root, ".agnos", "state.json"), logger: createLogger({ quiet: true }), diff --git a/test/domains/commands.test.ts b/test/domains/commands.test.ts index bae1790..802ba1c 100644 --- a/test/domains/commands.test.ts +++ b/test/domains/commands.test.ts @@ -16,13 +16,13 @@ let capturedLogger: Logger | undefined; const ctxFor = (args: string[], extra: Record = {}): CommandContext => ({ agnosRoot: tmp, projectRoot: tmp, - cacheDir: path.join(tmp, ".agnos", "cache"), + storeDir: path.join(tmp, "store"), configPath: path.join(tmp, "agnos.json"), statePath: path.join(tmp, ".agnos", "state.json"), logger: capturedLogger ?? createLogger({ quiet: true }), // Real fetcher: for `file:` (local) sources it just returns the absolute path, // so skills `add` discovery works without any network access. - fetcher: createRepoFetcher({ projectRoot: tmp, cacheDir: path.join(tmp, ".agnos", "cache") }), + fetcher: createRepoFetcher({ stagingDir: path.join(tmp, ".agnos", "tmp", "repos") }), linker: {} as never, dryRun: false, args, diff --git a/test/domains/empty-slices.test.ts b/test/domains/empty-slices.test.ts index 2db5b75..84fdf8b 100644 --- a/test/domains/empty-slices.test.ts +++ b/test/domains/empty-slices.test.ts @@ -21,7 +21,7 @@ function base(root: string) { return { agnosRoot: root, projectRoot: root, - cacheDir: path.join(root, ".agnos", "cache"), + storeDir: path.join(root, "store"), configPath: path.join(root, "agnos.json"), statePath: path.join(root, ".agnos", "state.json"), logger: createLogger({ quiet: true }), diff --git a/test/domains/global-skill-store.test.ts b/test/domains/global-skill-store.test.ts new file mode 100644 index 0000000..7e6cec0 --- /dev/null +++ b/test/domains/global-skill-store.test.ts @@ -0,0 +1,212 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import type { AgnosConfig, ResolveContext } from "../../src/core/index.js"; +import { createLinker, createLogger, hashSkillDir, writeLock } from "../../src/core/index.js"; +import { createSkillSteps } from "../../src/domains/skills/steps.js"; +import { runSkillPipeline } from "../../src/domains/skills/pipeline.js"; + +const COMPOSITE = "github:owner/repo/skills/tool#main"; +const CONFIG: AgnosConfig = { + schemaVersion: 1, + skills: { sources: { tool: COMPOSITE } }, +}; + +let root: string; +let checkout: string; +let storeDir: string; +let hash: string; + +beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), "agnos-global-store-")); + checkout = path.join(root, "checkout"); + storeDir = path.join(root, "store"); + await fs.mkdir(path.join(checkout, "skills", "tool"), { recursive: true }); + await fs.writeFile(path.join(checkout, "skills", "tool", "SKILL.md"), "# Shared\n"); + hash = await hashSkillDir(path.join(checkout, "skills", "tool")); +}); + +afterEach(async () => { + await fs.rm(root, { recursive: true, force: true }); +}); + +function context(projectRoot: string, fetch: ResolveContext["fetcher"]["fetch"]): ResolveContext { + const logger = createLogger({ quiet: true }); + return { + agnosRoot: path.join(projectRoot, ".agnos"), + projectRoot, + storeDir, + configPath: path.join(projectRoot, "agnos.json"), + statePath: path.join(projectRoot, ".agnos", "state.json"), + logger, + fetcher: { fetch, cleanup: async () => {} }, + linker: createLinker({ probeDir: path.join(projectRoot, ".agnos", "tmp"), logger }), + dryRun: false, + }; +} + +async function seedLock( + projectRoot: string, + resolvedCommit: string | null = "deadbeef", +): Promise { + await fs.mkdir(projectRoot, { recursive: true }); + await writeLock(projectRoot, { + version: 1, + skills: { + [COMPOSITE]: { + computedHash: hash, + resolvedAt: "2026-08-13T00:00:00.000Z", + ref: "main", + ...(resolvedCommit ? { resolvedCommit } : {}), + }, + }, + }); +} + +describe("global skill store", () => { + it("shares a locked skill across projects and leaves independent materializations", async () => { + const firstRoot = path.join(root, "first"); + const secondRoot = path.join(root, "second"); + await Promise.all([seedLock(firstRoot), seedLock(secondRoot)]); + const fetch = vi.fn(async (_source, options) => { + expect(options?.ref).toBe("deadbeef"); + return { path: checkout, ref: "deadbeef", commit: "deadbeef" }; + }); + + const first = await createSkillSteps(CONFIG, context(firstRoot, fetch)); + const firstResult = await runSkillPipeline( + CONFIG.skills?.sources ?? {}, + first.steps, + createLogger({ quiet: true }), + ); + await first.flush(); + + const noNetwork = vi.fn(async () => { + throw new Error("network should not be used"); + }); + const second = await createSkillSteps(CONFIG, context(secondRoot, noNetwork)); + const secondResult = await runSkillPipeline( + CONFIG.skills?.sources ?? {}, + second.steps, + createLogger({ quiet: true }), + ); + await second.flush(); + + expect(fetch).toHaveBeenCalledOnce(); + expect(noNetwork).not.toHaveBeenCalled(); + expect(firstResult.progress).toMatchObject({ reused: 0, fetched: 1 }); + expect(secondResult.progress).toMatchObject({ reused: 1, fetched: 0 }); + + await fs.rm(storeDir, { recursive: true, force: true }); + await expect( + fs.readFile(path.join(firstRoot, ".agnos", "skills", "tool", "SKILL.md"), "utf8"), + ).resolves.toBe("# Shared\n"); + await expect( + fs.readFile(path.join(secondRoot, ".agnos", "skills", "tool", "SKILL.md"), "utf8"), + ).resolves.toBe("# Shared\n"); + }); + + it("promotes and removes a valid legacy project cache", async () => { + const projectRoot = path.join(root, "legacy"); + await seedLock(projectRoot); + const legacy = path.join(projectRoot, ".agnos", "cache", "skills", hash); + await fs.cp(path.join(checkout, "skills", "tool"), legacy, { recursive: true }); + const installed = path.join(projectRoot, ".agnos", "skills", "tool"); + await fs.mkdir(path.dirname(installed), { recursive: true }); + await fs.symlink(legacy, installed, process.platform === "win32" ? "junction" : "dir"); + const fetch = vi.fn(async () => { + throw new Error("legacy migration should not fetch"); + }); + + const handle = await createSkillSteps(CONFIG, context(projectRoot, fetch)); + await runSkillPipeline( + CONFIG.skills?.sources ?? {}, + handle.steps, + createLogger({ quiet: true }), + ); + await handle.flush(); + + expect(fetch).not.toHaveBeenCalled(); + expect((await fs.lstat(installed)).isSymbolicLink()).toBe(false); + await expect(fs.access(path.join(projectRoot, ".agnos", "cache"))).rejects.toThrow(); + await expect( + fs.access(path.join(storeDir, "skills", hash, "SKILL.md")), + ).resolves.toBeUndefined(); + }); + + it("fetches and backfills a matching legacy lock entry without a commit", async () => { + const projectRoot = path.join(root, "legacy-lock"); + await seedLock(projectRoot, null); + const fetch = vi.fn(async (_source, options) => { + expect(options?.ref).toBe("main"); + return { path: checkout, ref: "main", commit: "deadbeef" }; + }); + const ctx = context(projectRoot, fetch); + + const handle = await createSkillSteps(CONFIG, ctx); + const result = await runSkillPipeline(CONFIG.skills?.sources ?? {}, handle.steps, ctx.logger); + await handle.flush(); + + const lock = JSON.parse(await fs.readFile(path.join(projectRoot, "agnos.lock.json"), "utf8")); + expect(fetch).toHaveBeenCalledOnce(); + expect(result.installed).toEqual(["tool"]); + expect(lock.skills[COMPOSITE].resolvedCommit).toBe("deadbeef"); + }); + + it("rejects and refetches a corrupt global entry", async () => { + const projectRoot = path.join(root, "corrupt-store"); + await seedLock(projectRoot); + const stored = path.join(storeDir, "skills", hash); + await fs.mkdir(stored, { recursive: true }); + await fs.writeFile(path.join(stored, "SKILL.md"), "# Corrupt\n"); + const fetch = vi.fn(async () => ({ path: checkout, ref: "deadbeef", commit: "deadbeef" })); + const ctx = context(projectRoot, fetch); + + const handle = await createSkillSteps(CONFIG, ctx); + await runSkillPipeline(CONFIG.skills?.sources ?? {}, handle.steps, ctx.logger); + await handle.flush(); + + expect(fetch).toHaveBeenCalledOnce(); + expect(await hashSkillDir(stored)).toBe(hash); + }); + + it("removes an unreferenced legacy cache when a declared source is not pinned", async () => { + const projectRoot = path.join(root, "unpinned-legacy"); + const legacy = path.join(projectRoot, ".agnos", "cache", "skills", "unreferenced"); + await fs.mkdir(legacy, { recursive: true }); + await fs.writeFile(path.join(legacy, "SKILL.md"), "# Unreferenced\n"); + const fetch = vi.fn(async () => { + throw new Error("offline"); + }); + const handle = await createSkillSteps(CONFIG, context(projectRoot, fetch)); + + await runSkillPipeline( + CONFIG.skills?.sources ?? {}, + handle.steps, + createLogger({ quiet: true }), + ); + await handle.flush(); + + await expect(fs.access(path.join(projectRoot, ".agnos", "cache"))).rejects.toThrow(); + }); + + it("retains an unusable legacy cache when migration cannot finish", async () => { + const projectRoot = path.join(root, "failed-legacy"); + await seedLock(projectRoot); + const legacy = path.join(projectRoot, ".agnos", "cache", "skills", hash); + await fs.mkdir(legacy, { recursive: true }); + await fs.writeFile(path.join(legacy, "SKILL.md"), "# Corrupt\n"); + const fetch = vi.fn(async () => { + throw new Error("offline"); + }); + const ctx = context(projectRoot, fetch); + const handle = await createSkillSteps(CONFIG, ctx); + + const result = await runSkillPipeline(CONFIG.skills?.sources ?? {}, handle.steps, ctx.logger); + await handle.flush(); + + expect(result.buckets.moved).toEqual(["tool"]); + await expect(fs.access(path.join(projectRoot, ".agnos", "cache"))).resolves.toBeUndefined(); + }); +}); diff --git a/test/domains/skills-concurrency.test.ts b/test/domains/skills-concurrency.test.ts new file mode 100644 index 0000000..57df3c9 --- /dev/null +++ b/test/domains/skills-concurrency.test.ts @@ -0,0 +1,19 @@ +import { describe, expect, it } from "vitest"; +import { runSkillTasks } from "../../src/domains/skills/concurrency.js"; + +describe("runSkillTasks", () => { + it("waits for active workers before reporting a failure", async () => { + let completed = false; + const run = runSkillTasks(["fail", "finish"], async (value) => { + if (value === "fail") throw new Error("failed"); + await new Promise((resolve) => { + setTimeout(resolve, 10); + }); + completed = true; + return value; + }); + + await expect(run).rejects.toThrow("failed"); + expect(completed).toBe(true); + }); +}); diff --git a/test/domains/skills-pipeline.test.ts b/test/domains/skills-pipeline.test.ts index 60e83fc..b0cc9a9 100644 --- a/test/domains/skills-pipeline.test.ts +++ b/test/domains/skills-pipeline.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import fs from "node:fs/promises"; import path from "node:path"; import os from "node:os"; @@ -8,7 +8,7 @@ import type { LogParts, ResolveContext, } from "../../src/core/index.js"; -import { createLogger } from "../../src/core/index.js"; +import { createLinker, createLogger, hashSkillDir, writeLock } from "../../src/core/index.js"; import { createSkillSteps, pruneSkills, updateSkills } from "../../src/domains/skills/steps.js"; import { runSkillPipeline } from "../../src/domains/skills/pipeline.js"; import skillsDomain from "../../src/domains/skills/index.js"; @@ -16,19 +16,23 @@ import skillsDomain from "../../src/domains/skills/index.js"; let tmp: string; // A fetcher that resolves a local `file:` source to its own directory (no network). -const ctxFor = (): ResolveContext => ({ - agnosRoot: tmp, - projectRoot: tmp, - cacheDir: path.join(tmp, ".agnos", "cache"), - configPath: path.join(tmp, "agnos.json"), - statePath: path.join(tmp, ".agnos", "state.json"), - logger: createLogger({ quiet: true }), - fetcher: { - fetch: async (source: { absolutePath?: string }) => ({ path: source.absolutePath ?? tmp }), - } as never, - linker: {} as never, - dryRun: false, -}); +const ctxFor = (): ResolveContext => { + const storeDir = path.join(tmp, "store"); + const logger = createLogger({ quiet: true }); + return { + agnosRoot: tmp, + projectRoot: tmp, + storeDir, + configPath: path.join(tmp, "agnos.json"), + statePath: path.join(tmp, ".agnos", "state.json"), + logger, + fetcher: { + fetch: async (source: { absolutePath?: string }) => ({ path: source.absolutePath ?? tmp }), + } as never, + linker: createLinker({ probeDir: path.join(tmp, ".agnos", "tmp"), logger }), + dryRun: false, + }; +}; const cfg = (): AgnosConfig => ({ schemaVersion: 1, @@ -79,6 +83,10 @@ describe("skills prep pipeline (steps)", () => { expect(await fs.readFile(path.join(tmp, installed), "utf8")).toContain("My Tool"); const lock = JSON.parse(await fs.readFile(path.join(tmp, "agnos.lock.json"), "utf8")); expect(Object.keys(lock.skills)).toEqual(["file:./skill-src"]); + expect(await fs.lstat(path.join(tmp, ".agnos", "skills", "mytool"))).toMatchObject({}); + expect((await fs.lstat(path.join(tmp, ".agnos", "skills", "mytool"))).isSymbolicLink()).toBe( + false, + ); const h2 = await createSkillSteps(cfg(), ctx); const src = await fetchSrc(h2.steps); @@ -86,6 +94,50 @@ describe("skills prep pipeline (steps)", () => { expect(await h2.steps.version("mytool", src)).toBe(true); // no resolvedCommit baseline }); + it("restores a remote skill directly from the content-addressed store", async () => { + const composite = "github:owner/repo/skills/mytool#main"; + const hash = await hashSkillDir(path.join(tmp, "skill-src")); + const stored = path.join(tmp, "store", "skills", hash); + await fs.cp(path.join(tmp, "skill-src"), stored, { recursive: true }); + await writeLock(tmp, { + version: 1, + skills: { + [composite]: { + computedHash: hash, + resolvedAt: "2026-08-13T00:00:00.000Z", + ref: "main", + resolvedCommit: "deadbeef", + }, + }, + }); + const fetch = vi.fn(async () => { + throw new Error("remote fetch should not run"); + }); + const config: AgnosConfig = { + schemaVersion: 1, + skills: { sources: { mytool: composite } }, + }; + const ctx = { ...ctxFor(), fetcher: { fetch } as never }; + const handle = await createSkillSteps(config, ctx); + const result = await runSkillPipeline(config.skills?.sources ?? {}, handle.steps, ctx.logger); + await handle.flush(); + + expect(result.installed).toEqual(["mytool"]); + expect(fetch).not.toHaveBeenCalled(); + expect( + await fs.readFile(path.join(tmp, ".agnos", "skills", "mytool", "SKILL.md"), "utf8"), + ).toBe("# My Tool\n"); + await expect(fs.access(path.join(tmp, ".agnos", "cache", "repos"))).rejects.toThrow(); + + const materializedMarker = path.join(tmp, ".agnos", "skills", "mytool", "SKILL.md"); + await fs.rm(materializedMarker); + await fs.writeFile(materializedMarker, "# Corrupted\n"); + const verification = await createSkillSteps(config, ctx, { verifyMaterialized: true }); + const fetched = await verification.steps.fetch("mytool", composite); + if (!fetched.src) throw new Error("expected stored skill to resolve"); + expect(await verification.steps.integrity("mytool", fetched.src)).toBe(false); + }); + it("integrity reports changed when content drifts from the lock", async () => { const ctx = ctxFor(); const h = await createSkillSteps(cfg(), ctx); @@ -140,6 +192,21 @@ describe("skills prep pipeline (steps)", () => { }; expect(Object.keys(nextLock.skills)).toEqual(["file:./skill-src"]); }); + + it("prunes a materialized skill without removing stored content", async () => { + const ctx = ctxFor(); + const handle = await createSkillSteps(cfg(), ctx); + await runSkillPipeline(SOURCES, handle.steps, ctx.logger); + await handle.flush(); + const linked = path.join(tmp, ".agnos", "skills", "mytool"); + const lock = JSON.parse(await fs.readFile(path.join(tmp, "agnos.lock.json"), "utf8")); + const stored = path.join(ctx.storeDir, "skills", lock.skills["file:./skill-src"].computedHash); + + await pruneSkills({ schemaVersion: 1, skills: { sources: {} } }, ctx); + + await expect(fs.access(linked)).rejects.toThrow(); + await expect(fs.access(path.join(stored, "SKILL.md"))).resolves.toBeUndefined(); + }); }); describe("skills domain run", () => { diff --git a/test/rules/rules-domain.test.ts b/test/rules/rules-domain.test.ts index cc7fc8c..82ea708 100644 --- a/test/rules/rules-domain.test.ts +++ b/test/rules/rules-domain.test.ts @@ -12,7 +12,7 @@ function ctxFor(root: string): ResolveContext { return { agnosRoot: root, projectRoot: root, - cacheDir: path.join(root, ".agnos", "cache"), + storeDir: path.join(root, "store"), configPath: path.join(root, "agnos.json"), statePath: path.join(root, ".agnos", "state.json"), logger: createLogger({ quiet: true }), diff --git a/test/skills/pipeline.test.ts b/test/skills/pipeline.test.ts index b91bf29..90ff73e 100644 --- a/test/skills/pipeline.test.ts +++ b/test/skills/pipeline.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi } from "vitest"; import { runSkillPipeline, type SkillSteps } from "../../src/domains/skills/pipeline.js"; import { mergeSkillSources } from "../../src/domains/skills/migrate.js"; import { createLogger } from "../../src/core/index.js"; +import type { LogInput } from "../../src/core/index.js"; const logger = createLogger({ quiet: true }); @@ -47,13 +48,13 @@ describe("runSkillPipeline", () => { it("threads the fetched ref into install", async () => { const installSpy = vi.fn(async () => {}); const s: SkillSteps = { - fetch: async () => ({ ok: true, src: "/src/x", ref: "main" }), + fetch: async () => ({ ok: true, src: "/src/x", ref: "main", commit: "deadbeef" }), version: async () => true, integrity: async () => true, install: installSpy, }; await runSkillPipeline({ x: "r" }, s, logger); - expect(installSpy).toHaveBeenCalledWith("x", "/src/x", "main"); + expect(installSpy).toHaveBeenCalledWith("x", "/src/x", "main", "deadbeef"); }); it("installs everything and reports no buckets when all skills are clean", async () => { @@ -61,6 +62,75 @@ describe("runSkillPipeline", () => { expect(res.installed).toEqual(["a", "b"]); expect(res.buckets).toEqual({ moved: [], changed: [] }); }); + + it("reconciles independent skills concurrently while preserving declaration order", async () => { + let active = 0; + let peak = 0; + const concurrentSteps: SkillSteps = { + async fetch(name) { + active += 1; + peak = Math.max(peak, active); + await Promise.resolve(); + active -= 1; + return { ok: true, src: `/src/${name}` }; + }, + version: async () => true, + integrity: async () => true, + install: async () => {}, + }; + + const res = await runSkillPipeline({ a: "r", b: "r", c: "r" }, concurrentSteps, logger); + + expect(peak).toBeGreaterThan(1); + expect(res.installed).toEqual(["a", "b", "c"]); + }); + + it("reports percentage, total, reused, and fetched counters", async () => { + const updates: LogInput[] = []; + const stop = vi.fn(); + const progressLogger = { + ...logger, + progress(initial: LogInput) { + updates.push(initial); + return { + update(next: LogInput) { + updates.push(next); + }, + stop, + }; + }, + }; + const progressSteps: SkillSteps = { + fetch: async (name) => + name === "missing" + ? { ok: false } + : { + ok: true, + src: `/src/${name}`, + source: name === "cached" ? "reused" : "fetched", + }, + version: async () => true, + integrity: async () => true, + install: async () => {}, + }; + + const result = await runSkillPipeline( + { cached: "a", remote: "b", missing: "c" }, + progressSteps, + progressLogger, + ); + + expect(result.progress).toEqual({ total: 3, completed: 3, reused: 1, fetched: 1 }); + expect(updates[0]).toEqual({ + message: "Installing skills 0%", + status: "total 3 | reused 0 | fetched 0", + }); + expect(updates.at(-1)).toEqual({ + message: "Installing skills 100%", + status: "total 3 | reused 1 | fetched 1", + }); + expect(stop).toHaveBeenCalledOnce(); + }); }); describe("mergeSkillSources (migrate policy)", () => { From 8066d1789209306d413f1e1f0658f31a7a3198eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Gonz=C3=A1lez?= Date: Thu, 13 Aug 2026 20:44:08 +0200 Subject: [PATCH 4/4] docs(skills): document global skill storage Describe the shared store, exact-commit reconciliation, legacy migration, progress reporting, and the architectural rationale. --- .docs/index.md | 4 ++ .../technical-decisions/global-skill-store.md | 32 +++++++++++++++ .docs/technical/skills-reconciliation.md | 39 +++++++++++++++++++ README.md | 2 +- 4 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 .docs/technical-decisions/global-skill-store.md diff --git a/.docs/index.md b/.docs/index.md index 497109f..01ae75b 100644 --- a/.docs/index.md +++ b/.docs/index.md @@ -12,3 +12,7 @@ title: Documentation Index - [Config Matching](technical/config-matching.md): How agnos resolves glob-aware documentation ignores and rule fragments. - [MCP Env Resolution](technical/mcp-env-resolution.md): How MCP env key declarations are resolved into secret-bearing agent files. - [Skills Reconciliation](technical/skills-reconciliation.md): How declared skill sources are reconciled with materialized skills and lock entries. + +### technical-decisions + +- [Global Skill Store](technical-decisions/global-skill-store.md): Records why Agnos shares verified skill content globally while keeping project materializations independent. diff --git a/.docs/technical-decisions/global-skill-store.md b/.docs/technical-decisions/global-skill-store.md new file mode 100644 index 0000000..b730911 --- /dev/null +++ b/.docs/technical-decisions/global-skill-store.md @@ -0,0 +1,32 @@ +--- +type: Technical Decision +title: Global Skill Store +description: Records why Agnos shares verified skill content globally while keeping project materializations independent. +resource: "" +tags: [skills, storage, caching] +timestamp: 2026-08-13T00:00:00Z +--- + +# Global Skill Store + +## Decision + +Agnos uses one versioned content-addressed skill store per user. Projects import independent materializations from that store and use their lockfiles to select exact content hashes and commits. + +## Rationale + +- Identical locked skills are downloaded and stored once across repositories. +- Hash verification makes shared reuse deterministic and detects accidental corruption. +- Independent project materializations continue working after the store is removed or pruned. +- Exact commit fetches reproduce missing entries after a tracked branch advances. + +## Consequences + +- The global store is part of the current user's trust boundary. +- Cross-project offline reuse requires a committed lockfile. +- Project materialization consumes filesystem metadata and may consume full file data when cloning and hard links are unavailable. +- Machine-specific store placement remains outside project configuration. + +## References + +- Reconciliation behavior: [Skills Reconciliation](../technical/skills-reconciliation.md). diff --git a/.docs/technical/skills-reconciliation.md b/.docs/technical/skills-reconciliation.md index 4d76d69..2245aae 100644 --- a/.docs/technical/skills-reconciliation.md +++ b/.docs/technical/skills-reconciliation.md @@ -27,9 +27,48 @@ timestamp: 2026-07-09T00:00:00Z - The skills domain prunes before installing or updating skills. - A normal `agnos` run prunes before materializing declared skills. - If no skills are declared, pruning still runs so stale materialized skills can be removed. +- Skill reconciliation runs with bounded concurrency while preserving declaration order in its result. +- Concurrent requests for the same Git source are coalesced into one fetch. +- Git checkouts use an isolated per-run workspace under `.agnos/tmp/repos`. +- Repository workspaces are removed at the end of each command or domain run, including when skills are moved, changed, or fail to install. + +## Content-addressed storage + +- Skill content is stored once per user in a global, versioned store selected from the operating system data directory. +- `AGNOS_STORE_DIR` overrides the global store base for installations that need a custom location. +- Every store reuse recomputes the skill hash. Invalid entries are replaced through atomic publication. +- The configured skills route contains an independent project materialization imported with copy-on-write cloning, hard links, or copies. +- Project materializations remain usable if the global store is removed. +- A pinned remote skill can be restored from its lock entry and global stored content without a Git operation. +- Local skill sources are still hashed on each reconciliation so local edits are detected. +- `agnos skills integrity` explicitly hashes project materializations instead of trusting tool-managed state. +- Updates bypass any checkout already staged during the current run before computing and accepting a new hash. +- Store publication never deletes the final hash path, tolerates concurrent publishers, and retries transient Windows filesystem errors. +- Lock updates use temporary files and atomic publication so concurrent readers never observe partial JSON. + +## Reproducible fetches + +- New remote lock entries record the commit SHA returned by the checkout that supplied the accepted content. +- A missing global entry is refetched at the locked commit rather than the current branch head. +- Legacy lock entries without a commit fetch their tracked ref once and are backfilled only when the content hash still matches. + +## Legacy migration + +- Referenced entries from `.agnos/cache/skills` are verified and promoted into the global store. +- Existing project links are replaced with independent materializations before the legacy cache is removed. +- A failed or incomplete migration retains the project cache and reports a warning. + +## Progress reporting + +- Interactive installs show a live completion percentage. +- The progress line reports the declared skill total, content-store reuses, and source fetches. +- A content-store hit increments `reused`. A skill loaded successfully from its declared source increments `fetched`. +- Quiet and non-interactive runs omit the transient progress line. +- Successful interactive and non-interactive installs emit a final 100 percent summary with the same counters. ## References - Skills command surface: [index.ts](../../src/domains/skills/index.ts). - Skills steps: [steps.ts](../../src/domains/skills/steps.ts). - Skills pipeline tests: [skills-pipeline.test.ts](../../test/domains/skills-pipeline.test.ts). +- Global store decision: [Global Skill Store](../technical-decisions/global-skill-store.md). diff --git a/README.md b/README.md index c5d6829..492fb17 100644 --- a/README.md +++ b/README.md @@ -221,7 +221,7 @@ Injects titled sections (by frontmatter `title`) from fragment files into your c ### `skills` -Fetches, pins, verifies, and installs skills into the canonical skills dir (linked per-agent by `agents`). +Fetches, pins, verifies, and installs skills into the canonical skills dir (linked per-agent by `agents`). Install work is concurrent, with live percentage, total, reused, and fetched counters. Each unique locked skill tree is stored once in a user-level content store shared across repositories. Projects receive independent materializations, so removing the store does not break installed skills. Git checkouts are isolated under `.agnos/tmp/repos/` for the current run and always removed afterward. Set `AGNOS_STORE_DIR` to override the operating system data location used by the store. | Subcommand | Args / Flags | Description | | ----------- | --------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |