BRICKS-43: Skills/Memories pages, PillMultiSelect, agent form updates, chat stream fix

[Kaiohz]

Jira

BRICKS-43

Changes

New Pages

• Skills page: grid + create dialog + viewer (markdown rendering with react-markdown + remark-gfm)
• Memories page: grid + create dialog + viewer
• Sidebar entries: Skills (Sparkles icon), Memories (BookOpen icon)

PillMultiSelect Component

• Reusable multi-select using toggle pills (same style as former middleware toggles)
• readOnly mode: displays only selected options (used in AgentConfigViewer)
• onChange optional: disables interaction when not provided

Agent Form Updates

• Replaced StringListEditor for Skills with SkillPillMultiSelect (fetches skill names from frontmatter)
• Replaced StringListEditor for Memory with MemoryPillMultiSelect
• Removed backend type dropdown (always "store"), store_backend select
• Empty messages: "No skills found. Create skills in the Skills page first."

AgentConfigViewer Updates

• Added Skills + Memories sections using the same PillMultiSelect components as the form (read-only)
• Removed backend type display (always "store"), store_backend display

Store API Layer

• IStorePort interface + storeApi (axios implementation)
• 4 TanStack hooks: useStoreFiles, useStoreFile, usePutStoreFile, useDeleteStoreFile
• frontmatter.ts: parse/build YAML frontmatter with js-yaml

Chat Fix

• clearStream() on thread change in ChatPage — prevents showing previous conversation's streaming state

Cleanup

• Removed MiddlewareType, BackendType.FILESYSTEM/COMPOSITE/STATE, root_dir, store_backend
• Updated all tests and fixtures accordingly

Tests

• Unit tests: 618 passed
• TypeScript: clean (tsc --noEmit)
• ESLint: 0 errors
• Prettier: formatted

[Kaiohz]

Review of [BRICKS-43] Skills / Memories pages + agent form refactor

Overall score: 7/10 — well-scoped, well-tested, with strong attention to UX and the strict-schema cleanup. A few real defects and a handful of quality/safety nits to address before merge.

---

Strengths

• TDD discipline is intact. 618 tests passing, every new component has unit coverage, and the negative-case tests for the schema (reject filesystem backend type, reject root_dir, reject middleware field) are exactly the right guardrails for a destructive refactor like this.
• Strict Zod schemas do the heavy lifting. Replacing loose enums with .strict() and z.enum([BackendType.STORE]) is the correct way to remove FILESYSTEM / COMPOSITE / STATE / MiddlewareType — the type system now prevents them from sneaking back in.
• Clean port/adapter for the store layer. IStorePort + storeApi mirrors the existing agentApi pattern, and the four TanStack hooks are slim, correctly key-cached, and invalidate the right families on success. usePutStoreFile even invalidates the specific ["store-file", path] key in addition to the list family — nice touch.
• PillMultiSelect is a genuinely reusable extraction. readOnly mode + optional onChange (disables interaction when absent) is a clean two-axis API; aria-pressed + role=group give it proper a11y.
• SkillMemorySelects collapses the N+1 fetch risk with useQueries and shares the query key with useStoreFile, so the viewer dialog reuses cached frontmatter without a refetch.
• Defensive clearStream() on thread change in ChatPage is the right fix and matches the existing store API (useChatStore.clearStream).
• frontmatter.ts has a sensible lenient fallback for malformed YAML blocks (lines like RAG: "queries & accents" — éàç) instead of throwing, which keeps the viewer useful when users hand-edit files.
• Sidebar and routing are wired correctly with the new lazy-loaded SkillsPage / MemoriesPage.

---

Issues to address

1. parseFrontmatter — closing-delimiter regex is too permissive / wrong

File: src/application/lib/frontmatter.ts (L41)

const closeMatch = rest.match(/^---\s*(\r?\n|$)/m);

With the m flag, ^ matches the start of any line. That means a body containing a line that starts with --- (a common Markdown horizontal rule) will be mis-parsed as the closing delimiter, silently truncating the body and corrupting the data. For example:

---
name: rag
---
# Title
---
some content

would parse data = { name: "rag" } and body = "# Title\n" (truncated).

frontmatter.test.ts should add a regression case for this. Switch to a non-multiline match anchored on the first delimiter or use rest.match(/^---\s*(\r?\n|$)/) (no m flag), then ensure you only match once near the top.

2. buildFrontmatter — lenient parser path can desync round-trip

File: src/application/lib/frontmatter.ts (L86)

buildFrontmatter correctly quotes values containing : , -, ?, leading quotes, or newlines — but the regex misses a few cases that the lenient parseFrontmatter fallback won't then recover from cleanly:

• A value starting with *, &, !, >, |, %, @, ` (block-scalar markers in YAML).
• A value containing a tab character (YAML treats tabs as invalid in plain scalars; js-yaml will fail on parse, then the lenient parser will preserve the tab — round-trip stable but the file is no longer valid YAML, which will break tools like yq).

Add a test for these edge cases, and consider extending formatScalar to quote any value containing any character outside [A-Za-z0-9 _.,()/'-]. Also add a round-trip test (parseFrontmatter(buildFrontmatter({...}, body))) — it's missing entirely.

3. deleteFile / putFile — no error message surfaced

Files: src/infrastructure/api/store/storeApi.ts, src/application/components/skill/SkillViewer.tsx, src/application/components/memory/MemoryViewer.tsx

The axios error.message shown via toast.error(err.message) for a delete / put failure is whatever axios inferred, which is often just "Request failed with status code 500". The backend's actual error body (detail / message) is in error.response.data and is discarded. Add a small helper next to isNotFound:

function extractApiMessage(error: unknown): string {
  const data = (error as any)?.response?.data;
  if (typeof data?.detail === "string") return data.detail;
  if (typeof data?.message === "string") return data.message;
  return (error as Error)?.message ?? "Unknown error";
}

…and use it in every onError: (err) => toast.error(...) in the two viewers + the create dialogs. Low effort, big UX win.

4. useCreateMemory and useCreateSkill — silent overwrite on collision

Files: src/application/hooks/skill/useCreateSkill.ts, src/application/hooks/memory/useCreateMemory.ts

putFile will happily overwrite an existing file. If a user creates a skill named rag and a skill rag already exists, they get a success toast and the previous body is silently replaced. Two reasonable fixes:

• Check existence with useStoreFile(path) and disable the submit button, or
• Have the backend return a 409 and surface it as a distinct error toast ("A skill with this name already exists").

The current behaviour is data loss waiting to happen.

5. MemoryPillMultiSelect has no race protection, and MemoryCard shows a 100-char preview that escapes

File: src/application/components/memory/MemoryGrid.tsx (L46-51)

const truncated = file.content.slice(0, 100);
return file.content.length > 100 ? `${truncated}…` : truncated;

For a multi-line memory file the preview is the first 100 characters of the raw file, which can include frontmatter delimiters, partial tags, or control characters. Render the preview as plain text only (e.g. strip the frontmatter via parseFrontmatter, then take the first 100 chars of body), and consider collapsing whitespace. Also: line-clamp-3 is applied in MemoryCard but <p> doesn't apply line-clamp without display: -webkit-box; -webkit-line-clamp: 3; utilities from Tailwind — confirm the project has the typography plugin or it'll overflow on long previews.

6. AgentConfigForm — McpServersAccordionItem is memo-ed but receives defaultValue instead of the actually-open set

File: src/application/components/agent/AgentConfigForm.tsx (L271)

The isOpen value is derived from defaultValue.includes("mcp-servers"), but defaultValue is DEFAULT_OPEN_SECTIONS which is a constant — so isOpen is always false. The data-state attribute is therefore always "closed", which any consumer relying on it (e.g. tests, styling) will trip over. Either compute the real open set from Accordion context or remove isOpen and the data-state line.

7. SkillViewer.handleExport — no MIME / encoding guard, no error if blob is empty

The Download attribute fallback is fine, but if file.content is undefined (network error recovered into null), new Blob([undefined]) produces a 6-byte file undefined (string). The button is disabled={!file} so the path is blocked, but defensively also:

if (!file?.content) return;

8. SkillPillMultiSelect description rendering

File: src/application/components/agent/SkillMemorySelects.tsx (L57)

description: file ? parseFrontmatter(file.content).data.description : undefined,

parseFrontmatter is called twice per row (once for name, once for description). Memoize or destructure once. Minor, but the hook runs in a useMemo so the duplicate parse happens on every fileResults change.

9. yaml.ts — serializeAgentConfig no longer omits root_dir (regression risk)

The old config may still contain a root_dir key on disk in legacy YAML files. agentConfigSchema.strict() will now reject any YAML containing root_dir, which is correct — but if any pre-existing production YAML has it, the read path will throw on parse. Worth a one-liner in the PR description ("backwards-incompatible: legacy YAML with root_dir or middleware must be migrated by hand before upgrade"), or a friendly migration in parseAgentConfig that strips known-removed keys before passing to Zod.

10. Tests — SkillViewer / MemoryViewer / SkillGrid / MemoryGrid / SkillPillMultiSelect / MemoryPillMultiSelect have no unit tests

The two *Viewer components are ~200-line stateful components with edit/save/delete flows, a double-confirm destructive action, and a markdown export. The two *Grid components contain path-derivation logic (deriveNameFromPath) and the parallel-fetch useQueries orchestration. The two *Selects hooks do the frontmatter-extraction + label fallback. They are all untested, while small <Card> and <Dialog> presentational components are tested. Invert the coverage: prioritise the logic-bearing components.

The good news: the useStoreFile / usePutStoreFile / useDeleteStoreFile hook tests are solid and cover the contract — so once you add a thin SkillViewer.test.tsx that mocks the hooks (the way AgentConfigViewer.test.tsx does), the suite is in good shape.

11. SkillsPage count duplicates the query

File: src/application/pages/SkillsPage.tsx (L11-12)

The page calls useStoreFiles("/skills/") and then <SkillGrid> also calls useStoreFiles(SKILLS_PREFIX). Two subscriptions, same data, both go through the same query key — TanStack dedupes the request but not the subscription. With a large /skills/ directory, this can double the re-render frequency. Drop the page-level call, or expose a total from SkillGrid via a render prop / callback.

Same in MemoriesPage.

12. MemoryViewer — MemoryEditForm keeps memoryName in state but it's read-only

The useState(memoryName) in MemoryEditForm is a leftover — the field is readOnly disabled, no onChange handler. Replace with a plain prop, no state. Tiny but it makes the intent clearer.

13. Accessibility — SkillGrid / MemoryGrid add-card button is missing a label

The + New Skill / + New Memory tiles have visible text, but the parent <button> is a flex column with an icon span and a label. Fine for sighted users, but the aria-label is implicit. Add aria-label="Create a new skill" to be explicit, since the visible text and the action are the same string and screen readers may concatenate weirdly with the icon description.

14. ChatPage clearStream() on every render where threadId is undefined

File: src/application/pages/ChatPage.tsx (L17-20)

useEffect(() => {
  setActiveThread(threadId ?? null);
  clearStream();
}, [threadId, setActiveThread, clearStream]);

This fires on mount (when threadId is undefined) and clears any in-flight stream of a previously selected thread. That's the bug fix, fine. But on subsequent navigations where threadId is unchanged, Zustand's set calls are no-ops, so this is OK. Just confirm: if the user is already on a thread and types a message that triggers a stream, then clicks the same thread in the sidebar, will clearStream() wipe the active turn? It will. Probably acceptable for v1, but worth flagging — consider clearing only when threadId actually changes (use a ref to remember the previous value).

---

Nits (optional, not blocking)

• useStoreFile's queryFn throws on falsy path even though enabled: !!path should make that unreachable. Defensive, but the throw surfaces as a query.error if the guard is ever loosened — fine, keep it.
• Data-od-id is sprinkled across these components for E2E selectors — good. Add one to the dialog root (data-od-id="skill-viewer", "memory-viewer") for symmetry.
• PillMultiSelect's <div role="group"> should also carry an aria-label (e.g. aria-label="Skills") when used in a form context — currently the only way to know what the group is for is the section heading above it.
• formConstants.ts exports AGENT_CONFIG_FORM_ID — CreateSkillDialog and CreateMemoryDialog re-implement the same FORM_ID constant inline. Consider extracting a shared formConstants (or just inline) for symmetry.
• storeApi.getFile 404 handling: when 404 is returned, the useQuery cache stores null and the file is treated as "loading complete, no data". Consumers like SkillGrid use the file as a soft hint (to show description), so this is fine — but if the file is supposed to exist and the backend returns 404, the UI will silently show "Loading skills..." until the user refreshes. A typed error (e.g. ExpectedFileNotFound) might be worth surfacing for the viewer case.

---

Verdict

The change is solid and the test discipline is exemplary for a refactor of this size. The schema-level guardrails (strict, enum-narrowed) are the right safety net. Two real bugs to fix before merge (#1 closing-delimiter regex, #4 silent overwrite), one quality fix that's a 10-line PR (#3 surface backend error messages), and one coverage gap to address (#10 viewer/grid/select tests). Everything else is polish.

Recommend approve with changes — fix #1, #3, #4 and #10 in a follow-up commit, then merge.

[Kaiohz]

Bug risk: the /m flag here means ^ matches the start of any line in rest. A body containing a Markdown horizontal rule (a line that starts with ---) will be mis-detected as the closing delimiter, truncating the body. For example parseFrontmatter('---\nname: a\n---\n# H1\n---\nstuff') will return data: { name: 'a' } and body: '# H1\n', silently dropping everything after the first --- line in the body. Either drop the m flag and search only the start of rest, or — more safely — split rest on the first \n that starts with --- and is followed by EOL/EOF. Add a unit test that locks the intended behaviour. See full review for the proposed fix.

[Kaiohz]

Data loss risk: putFile will silently overwrite an existing skill with the same name. The dialog doesn't pre-check existence, so a user creating a skill named rag when one already exists gets a success toast and the previous body is gone. Either pre-check with useStoreFile(path) and disable the submit button when present, or have the backend return a 409 and surface it as a distinct error toast. Same for useCreateMemory.ts (line 18).

[Kaiohz]

UX nit: axios's error.message is usually just "Request failed with status code 500" — the backend's detail / message lives in error.response.data and is discarded. The viewers / create dialogs toast err.message directly. Add a small extractApiMessage(error) helper next to isNotFound (returning data.detail ?? data.message ?? err.message ?? 'Unknown error') and use it in every onError: (err) => toast.error(...) callback in SkillViewer, MemoryViewer, CreateSkillDialog, CreateMemoryDialog. Low effort, big UX win.

[Kaiohz]

Defensive: new Blob([file.content]) with file.content === undefined produces the literal 6-byte file undefined. The disabled={!file} guard on the button (line 251) blocks the click path today, but the function itself should bail: if (!file?.content) return;. Same pattern in MemoryViewer.handleExport.

[Kaiohz]

Performance nit: parseFrontmatter is called twice per row (once for name on line 48, once for description on line 52). Destructure once: const { data } = file ? parseFrontmatter(file.content) : { data: {} as SkillFrontmatter }, then use data.name and data.description. The useQueries array result here means the cost grows linearly with the number of skills rendered in the form.

[Kaiohz]

Code Review — BRICKS-43

Score : 7.5/10 — Bonne PR, architecture et tests solides, mais quelques points à durcir.

✅ Points forts

• Architecture hexagonale respectée : nouveau port IStorePort + adapter storeApi + entity StoreFile — exactement le pattern attendu dans le repo.
• Refactor du schéma backend propre : agentConfigSchema passe en .strict() ce qui empêche toute régression silencieuse (root_dir, middleware, filesystem/composite). Excellent garde-fou.
• DRY via PillMultiSelect : un seul composant partagé entre AgentConfigForm, AgentConfigViewer, et utilisé en readOnly pour la lecture. Bonne factorisation.
• Parallélisation des fetches : useQueries dans SkillMemorySelects évite le N+1 sur le frontmatter des SKILL.md.
• Fix clearStream() sur changement de thread dans ChatPage : vraie race condition, bonne capture (commit #1).
• Erreur plutôt qu'overwrite silencieux : useCreateSkill/useCreateMemory lèvent une erreur si le fichier existe déjà (commit #2). Bonne décision UX.
• Test coverage : 618 tests passent, couvrant hooks + components + schema + yaml + format. CI ✅.
• Code-splitting : lazy() sur les nouvelles pages.

🐛 Bugs / points à corriger

1. useStoreFile — throw défensif inutile

   queryFn: () => {
     if (!path) throw new Error("path is required");  // ← dead code
     return storeApi.getFile(path);
   },
   enabled: !!path,

   Le enabled: !!path empêche déjà le déclenchement. Le throw est du code mort qui peut polluer les logs React Query en dev. À supprimer et laisser enabled faire son job.

2. Path injection dans useCreateSkill / useCreateMemory

   const path = `/skills/${name}/SKILL.md`;

   Aucun name validation côté front : name="../../rag" ou name="foo/bar" crée des fichiers hors du dossier attendu. Le backend doit sanitiser, mais ajouter une regex ^[a-z0-9_-]+$ côté hook serait défensif (cohérent avec la validation du agentConfigSchema pour name).

3. TOCTOU dans la création

   const existing = await storeApi.getFile(path);
   if (existing !== null) throw new Error(...);
   return storeApi.putFile(path, content ?? "");

   Deux créations concurrentes passent toutes les deux le check puis écrasent. Pas critique en UI single-user, mais l'idéal serait une opération atomique côté port (putIfAbsent) ou utiliser une contrainte unique côté backend. À tracker pour plus tard.

4. Path resolution asymétrique dans MemoryViewer/SkillViewer
resolveMemoryStorePath : replace(/^\/?memories\/?/, "") ne strippe pas le .md. Si quelqu'un passe memoryPath="AGENTS" (sans .md), on écrit/lit /memories/AGENTS (sans extension, introuvable par le grid). Normaliser : name.replace(/\.md$/, "") avant de recomposer.

5. Preview tronquée à 100 chars en milieu de mot (MemoryGrid.tsx:75)

   const truncated = file.content.slice(0, 100);

   Coupe en plein milieu d'un heading ou d'un mot. Considérer file.content.replace(/\s+/g, ' ').slice(0, 100) ou couper à la dernière frontière de mot.

💡 Suggestions

6. N+1 dans MemoryGrid : chaque card fetch son fichier via useStoreFile. Sur 50 memories, 50 requêtes. Le listFiles pourrait renvoyer un preview (premiers 200 chars) pour éviter le round-trip.

7. Cache TanStack non utilisé dans create : useCreateSkill bypass le cache via storeApi.getFile direct. On pourrait utiliser queryClient.fetchQuery(["store-file", path]) pour bénéficier du partage de cache.

8. Tests : import dynamique redondant dans tests/unit/domain/entities/agentConfig.test.ts:12 :

   const mod = await import("@/domain/entities/agent/agentConfig");  // déjà importé en statique ligne 1

   Copier-coller, inoffensif mais nettoyable.

9. Duplication du pattern data-od-id "Configure" entre SkillCard et MemoryCard : extraire un wrapper CardShell (border + block-shadow-raised + hover lift) si vous prévoyez d'autres types de cards. Sinon laisser en l'état.

📊 Métriques

• 53 fichiers, +3513 / -364
• Architecture : ✅ hexagonale, ports/adapters
• Tests : ✅ 618 passed, schema strict, anti-régression
• UX : ✅ preview, edit, export, delete avec confirm
• Sécurité : ⚠️ path injection à sanitiser
• Perf : ⚠️ N+1 sur MemoryGrid

Verdict

Solide. Le refactor backend (suppression middleware/root_dir, ajout checkpoint_backend) est bien net avec des tests anti-régression. Les nouveaux composants respectent les conventions (memo, readonly props, data-od-id). Les findings sont mineurs mais le path injection (#2) mérite une PR de suivi rapide.

[Kaiohz]

Code Review — BRICKS-43 (Skills / Memories pages, PillMultiSelect, agent form refactor, chat stream fix)

Score: 8/10 — well-architected PR, strong test coverage (618 tests pass, strict schema validation, collision-blocking mutations, lenient frontmatter fallback). A few real defects and a handful of polish items below.

✅ Strengths

• Hexagonal architecture respected — new IStorePort + storeApi (axios adapter) + TanStack hooks. Domain stays UI-agnostic, infrastructure stays swappable.
• Strict Zod schemas — .strict() on backendConfigSchema and agentConfigSchema correctly rejects root_dir, store_backend, and middleware leftovers. The "rejects unknown key" tests are exactly what I want to see in a cleanup PR.
• Frontmatter parser is solid — parseFrontmatter correctly handles a --- horizontal rule inside the body (the test does NOT truncate body containing a horizontal rule is the kind of regression-preventing test I appreciate). The lenient fallback for invalid YAML descriptions is a smart defensive measure.
• Mutation hooks prevent silent overwrites — useCreateMemory and useCreateSkill do a getFile → putFile collision check before writing. Good UX over the "PUT overwrites" default.
• clearStream() on thread change in ChatPage is the right fix and minimal.
• Atomic squash to one commit (feat: ...) on the head — easy to revert/cherry-pick.
• N+1 elimination via listFilePreviews(prefix, chars) and useQueries for skill frontmatter — production-aware.
• Reusable PillMultiSelect with readOnly mode and aria-pressed accessibility — extracted exactly the right primitive.

🐛 Real defects (worth fixing)

1. formatScalar regex bug in frontmatter.ts (line ~108):

   if (/[:#\-?][\s]|^["']|[:]{1}$/.test(value) || value.includes("\n")) {
     return JSON.stringify(value);
   }

   • \-? inside a character class means literal - OR ? (both). Likely intended [-:?] to match the YAML-meaningful characters, or simply :?- to be safe. Also [:#-?] already covers - without escaping.
   • The |[:]{1}$ clause is redundant — a single-char alternation ({}) is no-op and the colon is already caught by [:#?][\s].
   • The "special characters in description" test passes thanks to the lenient parser fallback, not because formatScalar produces valid YAML. Round-trip test (buildFrontmatter → parseFrontmatter) for descriptions like RAG: queries is missing.

2. TOCTOU race in useCreateMemory / useCreateSkill: getFile then putFile is not atomic. Two concurrent calls with the same name both pass the collision check and the second silently overwrites the first. For a single-user UI this is negligible, but the function names say "blocks creation" — they don't under concurrency. Consider either:

   • A POST /api/v1/store/files with a 409 Conflict semantics on the backend, or
   • Add a JSDoc note that the check is best-effort for the single-tab case.

3. SkillCard / MemoryCard data-od-id collision risk:

   data-od-id={`skill-card-${name}`}

   If two skills end up with the same frontmatter name (different directories), the selectors collide. Also the parent SkillGrid uses key={card.path} but the cards use name for data-od-id — a future bug if the name/path mapping changes. Prefer data-od-id={skill-card-${card.path}} (the path is the stable unique key).

4. MemoryViewer.resolveMemoryStorePath surprise: passing /memories/foo/bar.md is normalized to /memories/foo.md (the regex \/memories\/? plus the \.md$ strip silently drops bar). It's documented as "memoryName" but the prop is memoryPath and accepts paths. Either rename the prop, accept only filenames, or document the normalization explicitly.

5. 15+ new test files end without a trailing newline (\ No newline at end of file) — the same for MemoryCard.tsx and SkillCard.tsx. Trivial, but a one-shot printf '\n' >> … sweep (or adding an .editorconfig rule) prevents future drift. Prettier usually catches this; worth checking the formatter config.

🧹 Polish

6. storeApi.isNotFound only matches status 404. A 410 Gone (some backends use it for deleted resources) would propagate as an error. Add 410 if your backend emits it; otherwise add a comment that the 404-only contract is intentional.

7. Dead spy in tests/unit/hooks/store/useStoreFiles.test.tsx (the "uses query key" test): getQueryDataSpy = vi.spyOn(...) is set up, getQueryDataSpy.mockRestore() is called, but no assertion is made on the spy itself. The real assertion is the second queryClient.getQueryData(...) call. Either remove the spy or assert on it.

8. PillMultiSelect recomputes selectedSet = new Set(selected) every render — fine for small lists but useMemo(() => new Set(selected), [selected]) would let memo-wrapped children skip re-renders cleanly. Not critical at the current scale.

9. extractApiMessage falls through to error.message for non-axios errors, which works. But it doesn't handle error.data (some axios interceptors attach it without response.data). Minor; add data?.detail/message as a sibling lookup if you ever use a wrapper.

10. No test for ChatPage clearStream thread-change behavior — the fix is unverified. A simple clearStream spy test in a useChatStore test would lock in the regression.

11. Empty-message wording inconsistency: SkillPillMultiSelect says "No skills found. Create skills in the Skills page first." but MemoryPillMultiSelect says "No memories found. Create memories in the Memories page first." — fine, but consider extracting a shared PillMultiSelect emptyMessage generator that builds from the noun to keep the two components DRY.

12. MemoryGrid truncates previews defensively even though useStoreFilePreviews already truncates server-side. Either drop the slice(0, PREVIEW_CHARS) (trust the API contract) or add a comment explaining it's a defense-in-depth against a future API change.

13. New components could split "viewer" further: SkillViewer and MemoryViewer are 230/276 lines and share ~80% of their code (mode toggle, export, delete confirmation, edit form). Consider a MarkdownFileViewer base component with frontmatter: boolean and editableFields: ('name' | 'description' | 'content') props. The buildFrontmatter path makes the skill branch ~10% harder; everything else is duplicate.

🧪 Test coverage suggestions

• frontmatter.test.ts is missing a round-trip test: buildFrontmatter({name, description}, body) then parseFrontmatter(...) for descriptions containing :, ?, ', ", \n. That would catch the formatScalar regex bug.
• AgentConfigForm and AgentConfigViewer test the new checkpoint_backend field, but the form behavior when the user switches from memory → postgres is not exercised (only the static "renders checkpoint_backend" test). A small onValueChange test would lock the form's behavior.
• useCreateMemory and useCreateSkill should have one test for invalid name pattern (name="foo bar" or name="foo/bar") asserting the regex error and no getFile call.

🚦 Status

• Mergeable: ✅ clean
• Commits: 3 (squashed to 1 on head) ✅
• Tests: 618 passed per PR body — I trust the count, not re-run in this review.
• TS / ESLint / Prettier: clean per PR body.

I'd suggest addressing #1 (formatScalar regex) and #2 (TOCTOU doc) before merge. The rest are nice-to-haves for follow-up tickets.

---

Posted by SoluBot (SoluDevTech) on behalf of @Kaiohz
CI webhook trigger 24/07 13:02 UTC