ContextQMD
Back to Aionui

Skill-Library Pilot — Post-Pilot Follow-ups

docs/backend-migration/post-pilot/2026-04-23-skill-library-followups.md

1.9.26-dev-e9bbc4318.1 KB
Original Source

Skill-Library Pilot — Post-Pilot Follow-ups

Opened: 2026-04-23 (after pilot closure at 2026-04-22 22/29 e2e) Latest Skill e2e rerun: 2026-04-23 ~12:50 → 23 PASS / 5 FAIL / 1 SKIP (was 22/7 in Phase D). Gains: TC-S-08 (FAIL→PASS via P1-2 stable-testid fix), TC-S-06 (FAIL→SKIP via P1-1 env-gate). Remaining 5 fail all gated on P0-1/P0-2: TC-S-17 (P0-1 product decision), TC-S-15/25/28 (P0-2 sandbox), TC-S-27 (extension registry integration, long-term). Context: pilot was closed successfully with transport/migration layer CLEAN. These are the items pilot surfaced but doesn't own — deferred per coordinator's Phase D ruling.

References:

  • Module record: docs/backend-migration/modules/skill-library.md → "Final pilot outcome (post-Phase-D)".
  • E2E report: docs/backend-migration/e2e-reports/2026-04-22-skill-library.md.
  • E2E-tester handoff: docs/backend-migration/handoffs/e2e-tester-skill-library-2026-04-22.md.

P0 — blocks module-2 start

P0-1: Backend — reject duplicate path on POST /api/skills/external-paths

Symptom: TC-S-17 expects the UI to show an error and keep the "Add Path" modal open when the user submits a path that already exists as a custom external source. Currently the backend silently de-dupes by path (overwriting the existing entry's name) and returns {"success":true, HTTP 200}, so the renderer's handleAddCustomPath closes the modal on the no-throw path.

Root-cause evidence: direct curl probe captured during Phase D (see e2e report "Phase D trace findings — TC-S-17") showed back-to-back POSTs with the same path both return 200.

Proposed fix:

  • File: aionui-backend/crates/aionui-extension/src/external_paths_manager.rs (or wherever ExternalPathsManager::add_custom_external_path lives — grep for add_custom_external_path in crates/aionui-extension/src/).
  • Behavior: before appending, check whether path already exists in the stored list. If yes, return Err(ExtensionError::DuplicatePath(…)).
  • Error mapping: in aionui-extension/src/error.rs add a DuplicatePath(String) variant; in crates/aionui-app/src/error.rs (or the common AppError mapping) map it to StatusCode::CONFLICT (409) with a machine-readable code: "DUPLICATE_PATH".
  • Tests:
    • crates/aionui-extension/tests/… — unit test asserting duplicate returns DuplicatePath.
    • crates/aionui-app/tests/extension_e2e.rs — HTTP test: POST twice with same path → second returns 409 with expected code.
  • Renderer side: handleAddCustomPath (src/renderer/pages/settings/SkillsHubSettings.tsx:209–223) already handles catch with Message.error('Failed to add custom path') and leaves the modal open via not calling setShowAddPathModal(false). No renderer change needed once the backend throws on duplicate.

Scope estimate: ~1 function + 1 error variant + 1 route-level map + 3 tests. Small-diff fix.

P0-2: Test-infra — sandbox ~/.aionui/skills/ and custom-skill-paths.json

Symptom: the Phase B first attempt returned 13 PASS / 16 FAIL because ~/.aionui/custom-skill-paths.json had accumulated 24 leaked entries across prior runs (plus 89 leftover /var/folders/.../aionui-e2e-external-* temp dirs). Resetting the json and removing the temp dirs improved the rerun to 22/7. The sandbox mechanism in tests/e2e/fixtures.ts only isolates extension-states.json; custom-skill-paths.json writes straight through to the real user dir via ExternalPathsManager::new(&data_dir) where data_dir is hardcoded.

Proposed fix (multi-part):

  1. Backend: honor --data-dir for skill paths. aionui-extension::resolve_skill_paths at crates/aionui-extension/src/skill_service.rs:37–50 currently does:

    rust
    let data_dir = dirs::home_dir().unwrap_or_else(|| PathBuf::from(".")).join(".aionui");

    It should accept a data_dir: &Path argument and use that. Similarly, aionui-app::build_extension_states at crates/aionui-app/src/lib.rs:529–554 hardcodes dirs::home_dir().join(".aionui"). Both must thread the CLI's --data-dir all the way into these functions. Consider introducing a shared data_dir resolver in aionui-app that honors CLI args, env vars (AIONUI_DATA_DIR), and a final fallback.

  2. Fixtures: pass per-test data-dir to Electron-launched backend. src/process/backend/lifecycleManager.ts:21 already passes --data-dir <dbPath>. With part 1 in place, this same flag will also control user_skills_dir and custom-skill-paths.json, so e2e tests get automatic isolation.

  3. fixtures.ts cleanup (defense in depth): in the singleton Electron fixture's beforeEach/afterAll, still wipe the test-run's data-dir to prevent within-run pollution. The existing e2eStateSandboxDir mkdtemp pattern can be extended to cover the whole aionui/ data subtree, not just extension-states.json.

Acceptance criterion: bun run test:e2e tests/e2e/features/settings/skills/ can run repeatedly from any starting state without manual echo '[]' > ~/.aionui/custom-skill-paths.json or rm -rf /var/folders/.../aionui-e2e-external-*. Current 22/29 should be reproducible from a cold cache.

Scope estimate: medium. The data-dir threading touches 2 crates + 1 TS file. Worth doing carefully before module-2 starts.

P1 — test-authoring debt (non-blocking)

P1-1: TC-S-06 — "should not show delete button for builtin skills" ✅ FIXED (2026-04-23)

Status: Picked option (b) — use test.skip when no builtin skills detected. Smoke-tested: skips cleanly in dev env. Test will resume running (and asserting no-delete-button UI) as soon as any builtin skill exists in the target env (packaged build, or after P0-2 fixture seeding lands).

(Original diagnosis kept below for reference.)

Diagnosis: test queries skills.filter(s => s.source === 'builtin') and asserts builtinSkills.length > 0. On a fresh sandbox, no builtin skills exist (builtin_skills_dir points at the app bundle resource directory; dev mode has that empty). TC-S-06 fails at the sandbox-wide precondition, not at the delete-button logic.

Fix: either (a) seed a builtin skill fixture before the test using a new helper that copies a minimal SKILL.md to builtin_skills_dir, or (b) use test.skip with a clear reason if no builtin skills are detected, matching other fixture-gated tests.

P1-2: TC-S-08 — external source tab matcher collision ✅ FIXED (2026-04-23)

Status: Smoke-tested PASS (6.7s). Replaced fragile substring match with stable [data-testid="external-source-tab-custom-${tempSource.path}"]. Matches backend slug contract (custom paths → custom-<absolute-path>).

(Original diagnosis kept below for reference.)

Diagnosis: test locator button:has-text("E2E Test Source") is a loose substring match. TC-S-11 earlier in the run adds "E2E Test Source TC11" to the external-paths list. Even with correct per-test cleanup, there's a narrow window where both sources co-exist in the UI, producing a Playwright strict-mode violation.

Fix: replace with page.getByRole('button', { name: 'E2E Test Source', exact: true }) or use the stable [data-testid="external-source-tab-custom-${encodeURIComponent(path)}"] selector.

P1-3: TC-S-15 — within-run state leakage via failed afterEach

Diagnosis: test expects customSourceTabs.count() === 0. When an earlier test's own afterEach (cleanupTestSkills) fails or is skipped (e.g., test crashed mid-setup before removeCustomExternalPath could run), the leaked custom path persists. Phase B saw 5 residual tabs.

Fix: add a suite-level beforeEach in the Skills Hub test files that hits a backend admin endpoint or directly resets custom-skill-paths.json (after P0-2 lands, this is a single HTTP call against the test-scoped --data-dir). Alternatively, implement DELETE /api/skills/external-paths?all=1 (admin-only, gated on --local mode) as a test utility.

P1-4: TC-S-27 — Extension Skills board

Diagnosis: extension-skills-section renders only when extensionSkills.length > 0. Backend does not yet emit source: 'extension' (backend-dev handoff §"Known issues" #1 flagged this). Until extension registry is wired to list_available_skills, the section is always hidden.

Fix: either (a) wait for extension registry integration (tracked separately) and keep the test failing as a canary, or (b) weaken the assertion to "section exists OR no extension skills". Recommend (a) — the test is serving its design purpose.

P1-5: TC-S-28 — Auto Skills board

Diagnosis: auto-skills-section renders only when builtinAutoSkills.length > 0. Fresh sandbox has no <builtin_skills_dir>/_builtin/ contents. Same shape as P1-4.

Fix: same options. Recommend: seed a minimal _builtin/<skill>/SKILL.md in the fixture setup for the boards-rendering test file only, so other tests don't start seeing auto-injected skills in their assistant lists unexpectedly.

P2 — scale investigation (non-blocking)

P2-1: TC-S-25 bulk-import at N=20

Diagnosis: in-test fetchData returns 3 cards after importing 20 unique-named skills via serial await POST /api/skills/import-symlink. Standalone backend probe returns all 20 correctly (20 ProbeSkill imports returned 23 total skills against the same data-dir). So the backend is not racing or de-duping in isolation.

Precondition for useful investigation:

  • P0-2 must land first — the ambient ~/.aionui/skills/ had 125 dangling symlinks from prior runs. That environment may interact with scan_skill_dirs at volume in ways that don't repro on a clean dir. Once P0-2 gives us true isolation, re-measure.

Suggested investigation plan after P0-2 lands:

  1. Add debug!(count, ?names) in aionui-extension::skill_service::list_available_skills emitting the entry count and first N names found in user_skills_dir.
  2. Rerun TC-S-25 single-test with E2E_TRACE=1 and RUST_LOG=aionui_extension=debug.
  3. Compare backend stderr log for the TC-S-25 GET /api/skills call:
    • If it reports 20+ Bulk names present → serialization or response truncation (possibly a content-length cap in the axum layer).
    • If it reports <20 → scan_skill_dirs is filtering. Likely candidate: entry_path.is_dir() may return false for some symlinks depending on macOS FS state, or the broken-symlink check follows through to the target's file type.
  4. Cross-check with a renderer-side log at SkillsHubSettings.tsx:87-88 dumping the length of skills in the fetchData success path.

Pilot impact: none. The pilot passed all 5 transport-layer tests and 17 other interaction tests; bulk-import at N=20 is an edge case that the pilot surfaced but doesn't gate.

Ticket skeleton for project tracker

If you open these in an issue tracker, suggested titles:

  • [skill-library][P0] Reject duplicate path in /api/skills/external-paths
  • [skill-library][P0] Sandbox test-run data-dir for e2e (thread --data-dir into resolve_skill_paths + build_extension_states)
  • [skill-library][P1] TC-S-06 builtin-skill fixture
  • [skill-library][P1] TC-S-08 external-source-tab matcher
  • [skill-library][P1] TC-S-15 within-run state leak — suite-level reset
  • [skill-library][P1] TC-S-27/28 extension + auto-skill fixture
  • [skill-library][P2] Investigate TC-S-25 bulk-import at N=20

Not a pilot regression

All items above exist independently of the pilot's migration work. Backend-dev's E1–E5 implementation + source field fix and frontend-dev's helper migration are both clean and validated. These followups address pre-existing TS contract gaps, test-authoring assumptions, and e2e infrastructure that the pilot was the first opportunity to observe end-to-end.

Addendum — discovered during Assistant verification (2026-04-23)

P1 — test-authoring / infra

P1-A1 — STATUS: VERIFIED FIX LANDED on feat/backend-migration-e2e-helper-fix. Initial commit d96d189aa migrated 7 invokeBridge call sites to httpBridge.ts; follow-up commit (same branch tip) corrected DELETE /api/skills/external-paths to send path in JSON body (backend rejects query-param form) and cleaned up 4 more stragglers missed by initial replace_all. Smoke-tested in real Electron:

  • P2-3 PASS (was FAIL with "Bridge invoke timeout: add-custom-external-path", 9.5s)
  • P1-20 PASS
  • P1-21 PASS
  • P1-23 now reveals a different, unrelated bug (see new P1-A3 below): drawer doesn't auto-open from sessionStorage intent within 10s. Prior invokeBridge timeout was masking this.

Expected full-suite Assistant e2e result: 35/37 PASS (was 32/37; +3 for P2-3/P1-20/P1-21; P1-23 fails for new reason; P1-18 still env-dependent).

Still open for other invokeBridge callers outside assistants: helpers/extensions.ts, helpers/chatAionrs.ts, cron/team/ext specs — those targets may still have live IPC handlers, audit per-key before migrating.

Branch feat/backend-migration-e2e-helper-fix pending user review; not merged into base or coordinator.

P1-A3 (NEW, surfaced by P1-A1 fix): P1-23 drawer auto-open from sessionStorage intent ✅ FIXED (2026-04-23)

Status: P1-23 PASS (11.9s). Neither (a) nor (b) — it was a test-side data-source bug. Trace showed:

  • GET /api/extensions/assistants returns [] in dev env (endpoint only exposes extension-contributed assistants).
  • Visible assistant list is populated via a separate path (builtin bundle + registry init) — card ids like builtin-ppt-creator / builtin-excel-creator.
  • Test was falling back to hardcoded 'builtin-agent' (not in the visible list), so the renderer's useEffect at AssistantSettings/index.tsx:127-128 correctly returned early (targetAssistant undefined) and never opened the drawer.

Fix: test now harvests a real id from a rendered [data-testid^="assistant-card-"] DOM element before planting the intent. Renderer behavior (the useEffect consumer) was always correct; the test just fed it a non-existent id.

Unblocks: Assistant e2e is now expected at 36/37 (was 35/37 after P1-A1) — remaining P1-18 is env-fixture (builtin-auto skills), gated behind P0-2.

Confirmed 2026-04-23 12:32 by full-suite rerun: 36 PASS / 1 FAIL (P1-18, auto-injected section) in 1.9 min. Transport/migration layer is clean; the single remaining failure is test-fixture gated behind P0-2 (sandbox ~/.aionui/skills/builtin-auto/).

Aside: if the renderer ever wants to expose ALL assistants (not just extension-contributed) via HTTP, that's a separate API extension task — not required to unblock this test.

P1-A1 (original issue — now scoped to remaining callers): tests/e2e/helpers/bridge.ts has no-op provider() and no WebSocket subscribe-* handler after HTTP migration.

  • Symptom: 4 Assistant tests (P2-3, P1-20, P1-21, P1-23) time out in beforeEach because the helper tries to seed fixtures via invokeBridge. UI path works — only test-helper fixture-seeding broken.
  • Skill-Library pilot fixed this for tests/e2e/helpers/skillsHub.ts only (commit 000676801, new tests/e2e/helpers/httpBridge.ts). The underlying fallback in bridge.ts was not touched.
  • Fix: migrate bridge.ts to route migrated keys through HTTP (reusing httpBridge.ts), keeping a legacy-IPC path only for keys not yet migrated. Or: delete the legacy path and hard-error on any non-migrated key so the breakage is loud instead of a silent timeout.
  • Scope estimate: ~50 lines of helper + one pass through all tests/e2e/features/** for any remaining invokeBridge calls.

P1-A2: P1-18 (Assistant "Auto-injected Skills" section) — asserts visible but sandbox returns [] from GET /api/skills/builtin-auto.

  • Symptom: test expects non-empty builtin auto-skills list; unsandboxed user FS returns empty.
  • Same root cause family as Skill P1-6/27/28 fixture assumptions. Fix lands alongside P0-2 sandboxing or as a suite-level fixture seeding pass.

P2 — API contract docs

P2-A1: Pin DELETE-with-path-param convention in docs/api-spec/13-extension.md.

  • Symptom: during Assistant verification, frontend-dev's first curl probe against DELETE /api/skills/assistant-rule/:id used a JSON body carrying {assistantId} and got 404. Path-param form worked. The body-vs-path convention is not currently documented.
  • Fix: add a short convention note to the spec + link from each DELETE endpoint in the Skill Library section. ~10 lines.

P2-A2: DELETE /api/skills/external-paths rejects requests without Content-Type: application/json header even with no body.

  • Symptom: e2e-tester noticed this as a minor backend quirk during Assistant e2e run (the endpoint accepts the path as a query param, but still requires the header).
  • Fix: relax the axum extractor or document the requirement. Low priority — renderer sends the header anyway.

P3 — plan-authoring improvements (for future modules)

  • P3-1: Remove "manual GUI spot-check" from frontend-dev plan templates. Frontend-dev is a non-interactive agent and cannot drive Electron windows. Replace with "headless curl probe" and delegate real-UI verification exclusively to e2e-tester's Playwright suite. The Assistant verify plan (2026-04-23-assistant-module-verification-plan.md) hit this mid-run and had to redefine A.3 on the fly.

  • P3-2: Check "is it already migrated?" BEFORE writing a migration plan. Assistant endpoints were 100% implemented during Skill-Library pilot as a side effect; a full migration track was unnecessary. First pass for any module-N plan: grep skill_routes.rs, routes.rs in aionui-extension for the target endpoint paths. If all hits exist, scope the track as verification, not migration. Saves ~7 hours of wall clock.

  • P3-3: Branch-baseline drift between coordinator branch and teammate branches. frontend-dev-2 couldn't see the Assistant verification plan because its branch was based on origin/feat/backend-migration and the plan was committed to the coordinator branch after. Either (a) commit plan to both branches up front, or (b) tell teammates to read the plan via an absolute path that exists on the coordinator branch checked out in a separate shell. Option (b) is simpler and what this track actually did for the module log.