feat(slice-3): admin Create-pack wizard + POST /api/packs/scaffold#26
Merged
Conversation
Closes v1.7 slice 3 — wires the admin's previously-silent "Install
pack" placeholder button to a real wizard that scaffolds packs on
disk through the same `runPackNew` code path as the `aqa pack new`
CLI.
## Server side (@aqa/server)
- New `POST /api/packs/scaffold` endpoint (`packs:install` permission)
- New optional `ApiContext.projectRoot` field — endpoints that touch
the filesystem anchor to this server-configured path, never to a
client-supplied root (security)
- `@aqa/kit` added as workspace dependency; `runPackNew` now exported
from `@aqa/kit`'s public surface so other server-side code can
reuse the same scaffolder
- 7 new unit tests covering: happy-path 201, missing projectRoot,
missing/invalid slug, unsupported sut_type, 409 on duplicate
without force, force=true overwrite, permission requirement
## Admin UI (@aqa/admin)
- Renamed the previously-silent "Install pack" button to "Create
pack" and wired its onClick
- New `<CreatePackWizard>` modal: slug input with live regex/length
validation mirroring the @aqa/schemas Slug constraint (52-char
cap), sut-type dropdown, Advanced disclosure for
description/author/license/force checkbox, success panel showing
the server's pack_dir + scaffolded files
- Network errors and server-side errors both render an alert inside
the wizard rather than just toasting — the user can fix the input
and retry without losing their work
- 4 new Playwright e2e tests using `page.route()` to mock the
server endpoint (admin runs in mock-data mode in CI/dev): wizard
opens, invalid-slug feedback, happy-path 201, 409 with retry path
via Advanced → Force overwrite
## Audit doc
- `docs/internal/admin-placeholder-audit.md` line 6850 marked
**DONE** for slice 3; line 6853 ("Import manifest") still pending
for slice 4b
## Tests
- @aqa/kit: 54 tests (unchanged, the new `runPackNew` export change
is API-only and the existing tests cover the underlying function)
- @aqa/server: 26 tests (19 existing + 7 new for /api/packs/scaffold)
- @aqa/admin: 40 e2e tests (36 existing + 4 new for the wizard)
- Lint + typecheck clean
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements v1.7 slice 3: adds a server endpoint POST /api/packs/scaffold (delegating to runPackNew from @aqa/kit) and an admin "Create pack" wizard modal that posts to it, replacing the previous silent "Install pack" placeholder button on the Packs screen.
Changes:
- New server route
POST /api/packs/scaffold(anchored to a boot-timeApiContext.projectRoot, requirespacks:install) plus 7 unit tests;@aqa/kitadded as a workspace dep andrunPackNewexported from its index. - New
<CreatePackWizard>React component inpackages/admin/src/app.tsxwith slug/SUT-type fields, advanced disclosure, result panel, and 4 Playwright e2e tests; Packs page button renamed from "Install pack" to "Create pack" and wired to open the wizard. - Docs updated:
docs/PROGRESS.mdnotes the slice anddocs/internal/admin-placeholder-audit.mdstrikes through the now-wired button.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/server/src/api.ts | Adds projectRoot to ApiContext and the POST /api/packs/scaffold route handler. |
| packages/server/test/api.test.ts | Adds 7 tests for the new scaffold endpoint via tmp project roots. |
| packages/server/package.json | Adds @aqa/kit workspace dependency. |
| packages/kit/src/index.ts | Re-exports runPackNew + types for server consumption. |
| packages/admin/src/app.tsx | New CreatePackWizard modal and Packs page button rename/wire-up. |
| packages/admin/test/e2e/create-pack.e2e.ts | New Playwright tests for the wizard happy path and 409 conflict. |
| docs/PROGRESS.md | Adds a 2026-05-18 entry referencing the slice. |
| docs/internal/admin-placeholder-audit.md | Marks the Packs top-bar placeholder as done. |
| bun.lock | Adds @aqa/kit workspace edge for @aqa/server. |
Comment on lines
+311
to
+313
| // "already exists" → 409 Conflict (the resource exists), every | ||
| // other validation failure → 400. | ||
| const status = /already exists/i.test(result.error ?? '') ? 409 : 400; |
Comment on lines
+301
to
+309
| const result = runPackNew({ | ||
| root: ctx.projectRoot, | ||
| slug: body.slug, | ||
| sutType: body.sut_type, | ||
| ...(body.force !== undefined ? { force: body.force } : {}), | ||
| ...(body.description !== undefined ? { description: body.description } : {}), | ||
| ...(body.author !== undefined ? { author: body.author } : {}), | ||
| ...(body.license !== undefined ? { license: body.license } : {}), | ||
| }); |
Comment on lines
+30
to
+32
| function tmpProjectRoot(): string { | ||
| return mkdtempSync(join(tmpdir(), 'aqa-server-pack-')); | ||
| } |
Comment on lines
+4259
to
+4261
| setError( | ||
| `Could not reach /api/packs/scaffold (${msg}). The admin is in mock-data mode or the server is down — no files were written.`, | ||
| ); |
Comment on lines
+287
to
+309
| const body = (req.body ?? {}) as { | ||
| slug?: string; | ||
| sut_type?: string; | ||
| force?: boolean; | ||
| description?: string; | ||
| author?: string; | ||
| license?: string; | ||
| }; | ||
| if (typeof body.slug !== 'string' || body.slug.trim() === '') { | ||
| return asResponse({ error: 'slug is required (non-empty string)' }, 400); | ||
| } | ||
| if (typeof body.sut_type !== 'string' || body.sut_type.trim() === '') { | ||
| return asResponse({ error: 'sut_type is required (non-empty string)' }, 400); | ||
| } | ||
| const result = runPackNew({ | ||
| root: ctx.projectRoot, | ||
| slug: body.slug, | ||
| sutType: body.sut_type, | ||
| ...(body.force !== undefined ? { force: body.force } : {}), | ||
| ...(body.description !== undefined ? { description: body.description } : {}), | ||
| ...(body.author !== undefined ? { author: body.author } : {}), | ||
| ...(body.license !== undefined ? { license: body.license } : {}), | ||
| }); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34f48f470d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| root: ctx.projectRoot, | ||
| slug: body.slug, | ||
| sutType: body.sut_type, | ||
| ...(body.force !== undefined ? { force: body.force } : {}), |
There was a problem hiding this comment.
force as boolean before forwarding scaffold args
The new scaffold endpoint forwards any defined force value directly to runPackNew, but runPackNew uses truthiness (if (!opts.force)) to decide whether overwrite is allowed. If a client sends JSON like "force": "false" (string) or another truthy non-boolean, this is treated as enabled force mode and can overwrite an existing pack unexpectedly. Because this route is an API boundary, it should reject non-boolean force values (400) before calling runPackNew.
Useful? React with 👍 / 👎.
…eanup PR #26 iter 1 review fixes (Copilot + Codex): ## Codex P1 / Copilot — `force` smuggling via truthy string The endpoint forwarded any defined `force` value to `runPackNew`, which checks via truthiness (`if (!opts.force)`). A client sending `{"force": "yes"}` would silently enable force-overwrite mode and clobber an existing pack. Now the API boundary strictly type-checks `force` as a boolean and `description` / `author` / `license` as strings, returning 400 with a clear error for any other shape. New regression test: `{slug, sut_type, force: "yes"}` → 400 with `/force.*boolean/i`. Same for description=123 / author=123 / license=123. ## Copilot — brittle 409 detection via regex on error string The endpoint mapped to 409 Conflict by regex-matching `/already exists/i` against `result.error`. Any future rewording in `runPackNew` would silently demote the conflict to a generic 400. Now `runPackNew` returns a structured `code: 'EEXIST' | 'EINVAL' | 'EIO'` field, and the endpoint switches on that to pick the HTTP status (EEXIST → 409, EIO → 500, default → 400). The code also propagates to the response body as `{error, code}` so clients can act on it programmatically. New regression test: assert `code: 'EEXIST'` in body for dup-slug conflict; `code: 'EINVAL'` for bad-slug input. ## Copilot — tmpdir leak in tests Every test created a `mkdtempSync` root and never cleaned it up. Six tests × N runs = mounting pile of full-pack trees under the OS temp dir. Now tracked in a module-level array with a single `after` hook that `rmSync`s all of them best-effort. ## Copilot — sync FS work in async handler Documented the tradeoff with an explicit comment: `runPackNew` is sync but writes ~5 small files (typical wall-clock ~10ms). The create-pack flow is an explicit, infrequent user action from a wizard, so blocking the event loop for that window is acceptable vs duplicating runPackNew's logic in an async variant. If the endpoint ever gets called from a high-fanout path, revisit. ## Copilot — inconsistent toast on network error Catch branch in the admin wizard previously only set inline `error` state but didn't push a toast, unlike the HTTP-failure branch above. Now both branches push a toast for parity. Tests: @aqa/server 24 unit (was 21, 3 new), @aqa/kit 54 (unchanged behavior on existing tests, code field added without breaking the result shape), @aqa/admin 40 Playwright (4 wizard tests still green). Lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
iter 2 on |
Comment on lines
+301
to
+363
| method: 'POST', | ||
| path: '/api/packs/scaffold', | ||
| requires: 'packs:install', | ||
| async handle(req, ctx) { | ||
| if (!ctx.projectRoot) { | ||
| return asResponse( | ||
| { | ||
| error: | ||
| 'server has no projectRoot configured — pack scaffolding requires the server to know which on-disk project to write into', | ||
| }, | ||
| 400, | ||
| ); | ||
| } | ||
| const body = (req.body ?? {}) as Record<string, unknown>; | ||
| // Required fields — strict type + non-empty check. | ||
| if (typeof body.slug !== 'string' || body.slug.trim() === '') { | ||
| return asResponse({ error: 'slug is required (non-empty string)' }, 400); | ||
| } | ||
| if (typeof body.sut_type !== 'string' || body.sut_type.trim() === '') { | ||
| return asResponse({ error: 'sut_type is required (non-empty string)' }, 400); | ||
| } | ||
| // Optional fields — strict type check at the API boundary so a | ||
| // client can't smuggle truthy non-booleans through `force` (which | ||
| // `runPackNew` checks via `if (!opts.force)` truthiness). A | ||
| // request like `{"force": "no"}` would be silently treated as | ||
| // force-enabled without this guard. | ||
| if (body.force !== undefined && typeof body.force !== 'boolean') { | ||
| return asResponse( | ||
| { error: 'force must be a boolean when provided (got non-boolean)' }, | ||
| 400, | ||
| ); | ||
| } | ||
| for (const k of ['description', 'author', 'license'] as const) { | ||
| const v = body[k]; | ||
| if (v !== undefined && typeof v !== 'string') { | ||
| return asResponse( | ||
| { error: `${k} must be a string when provided (got non-string)` }, | ||
| 400, | ||
| ); | ||
| } | ||
| } | ||
| const result = runPackNew({ | ||
| root: ctx.projectRoot, | ||
| slug: body.slug, | ||
| sutType: body.sut_type, | ||
| ...(body.force !== undefined ? { force: body.force as boolean } : {}), | ||
| ...(body.description !== undefined ? { description: body.description as string } : {}), | ||
| ...(body.author !== undefined ? { author: body.author as string } : {}), | ||
| ...(body.license !== undefined ? { license: body.license as string } : {}), | ||
| }); | ||
| if (!result.ok) { | ||
| // Map the structured `code` field to an HTTP status. This is | ||
| // stable across error-message wording changes, unlike the | ||
| // earlier regex-on-error approach. | ||
| const httpStatus = errorCodeToStatus(result.code); | ||
| return asResponse( | ||
| { error: result.error ?? 'unknown error', code: result.code ?? 'EINVAL' }, | ||
| httpStatus, | ||
| ); | ||
| } | ||
| return asResponse({ pack_dir: result.packDir, files: result.files ?? [] }, 201); | ||
| }, | ||
| }, |
Comment on lines
+344
to
+345
| slug: body.slug, | ||
| sutType: body.sut_type, |
Comment on lines
+4171
to
+4228
| const [license, setLicense] = React.useState('Apache-2.0'); | ||
| const [force, setForce] = React.useState(false); | ||
| const [advancedOpen, setAdvancedOpen] = React.useState(false); | ||
| const [submitting, setSubmitting] = React.useState(false); | ||
| const [error, setError] = React.useState(null); | ||
| const [result, setResult] = React.useState(null); | ||
| const toast = useToast(); | ||
|
|
||
| // Mirror the Slug schema regex from @aqa/schemas so the user gets | ||
| // immediate feedback rather than waiting for the server round-trip. | ||
| // The server is still the source of truth (this is a usability check, | ||
| // not a security boundary). | ||
| const SLUG_PATTERN = /^[a-z0-9](?:-?[a-z0-9])*$/; | ||
| const MAX_SLUG_LEN = 52; | ||
| const slugTrimmed = slug.trim(); | ||
| const slugError = (() => { | ||
| if (slugTrimmed === '') return null; // empty is "not yet entered", not an error | ||
| if (slugTrimmed.length > MAX_SLUG_LEN) { | ||
| return `${slugTrimmed.length} chars — max ${MAX_SLUG_LEN}`; | ||
| } | ||
| if (!SLUG_PATTERN.test(slugTrimmed)) { | ||
| return 'lowercase a-z, 0-9, single dashes only'; | ||
| } | ||
| return null; | ||
| })(); | ||
| const canSubmit = slugTrimmed !== '' && slugError === null && !submitting; | ||
|
|
||
| function reset() { | ||
| setSlug(''); | ||
| setSutType('api'); | ||
| setDescription(''); | ||
| setAuthor(''); | ||
| setLicense('Apache-2.0'); | ||
| setForce(false); | ||
| setAdvancedOpen(false); | ||
| setError(null); | ||
| setResult(null); | ||
| setSubmitting(false); | ||
| } | ||
|
|
||
| function handleClose() { | ||
| if (submitting) return; // don't abandon a request mid-flight | ||
| reset(); | ||
| onClose?.(); | ||
| } | ||
|
|
||
| async function handleSubmit() { | ||
| if (!canSubmit) return; | ||
| setSubmitting(true); | ||
| setError(null); | ||
| try { | ||
| const body = { | ||
| slug: slugTrimmed, | ||
| sut_type: sutType, | ||
| ...(description.trim() ? { description: description.trim() } : {}), | ||
| ...(author.trim() ? { author: author.trim() } : {}), | ||
| ...(license.trim() ? { license: license.trim() } : {}), | ||
| ...(force ? { force: true } : {}), |
Comment on lines
+292
to
+350
| // Synchronous FS work in an async handler: `runPackNew` is sync | ||
| // (mkdir / writeFile / rename of ~5 small files plus a few schema | ||
| // validations — typical wall-clock ~10ms on a developer laptop). | ||
| // For the admin's create-pack flow that's an explicit, infrequent | ||
| // user action (clicked from a wizard), so blocking the event loop | ||
| // for that brief window is an acceptable tradeoff vs the cost of | ||
| // duplicating runPackNew's logic in an async variant. If this | ||
| // endpoint ever gets called from a high-fanout path (e.g. bulk | ||
| // pack import), revisit and offload. | ||
| method: 'POST', | ||
| path: '/api/packs/scaffold', | ||
| requires: 'packs:install', | ||
| async handle(req, ctx) { | ||
| if (!ctx.projectRoot) { | ||
| return asResponse( | ||
| { | ||
| error: | ||
| 'server has no projectRoot configured — pack scaffolding requires the server to know which on-disk project to write into', | ||
| }, | ||
| 400, | ||
| ); | ||
| } | ||
| const body = (req.body ?? {}) as Record<string, unknown>; | ||
| // Required fields — strict type + non-empty check. | ||
| if (typeof body.slug !== 'string' || body.slug.trim() === '') { | ||
| return asResponse({ error: 'slug is required (non-empty string)' }, 400); | ||
| } | ||
| if (typeof body.sut_type !== 'string' || body.sut_type.trim() === '') { | ||
| return asResponse({ error: 'sut_type is required (non-empty string)' }, 400); | ||
| } | ||
| // Optional fields — strict type check at the API boundary so a | ||
| // client can't smuggle truthy non-booleans through `force` (which | ||
| // `runPackNew` checks via `if (!opts.force)` truthiness). A | ||
| // request like `{"force": "no"}` would be silently treated as | ||
| // force-enabled without this guard. | ||
| if (body.force !== undefined && typeof body.force !== 'boolean') { | ||
| return asResponse( | ||
| { error: 'force must be a boolean when provided (got non-boolean)' }, | ||
| 400, | ||
| ); | ||
| } | ||
| for (const k of ['description', 'author', 'license'] as const) { | ||
| const v = body[k]; | ||
| if (v !== undefined && typeof v !== 'string') { | ||
| return asResponse( | ||
| { error: `${k} must be a string when provided (got non-string)` }, | ||
| 400, | ||
| ); | ||
| } | ||
| } | ||
| const result = runPackNew({ | ||
| root: ctx.projectRoot, | ||
| slug: body.slug, | ||
| sutType: body.sut_type, | ||
| ...(body.force !== undefined ? { force: body.force as boolean } : {}), | ||
| ...(body.description !== undefined ? { description: body.description as string } : {}), | ||
| ...(body.author !== undefined ? { author: body.author as string } : {}), | ||
| ...(body.license !== undefined ? { license: body.license as string } : {}), | ||
| }); |
…ngle-tenant scope PR #26 iter 2 review fixes (Copilot — 3 real new items, 3 stale re-flags already addressed in iter 1): ## Real fixes 1. **Slug/sut_type not trimmed before forwarding to runPackNew** (Copilot api.ts:345): the endpoint checked `body.slug.trim() !== ''` for non-empty but then forwarded the untrimmed `body.slug`. A request like `{"slug": " pack-x "}` therefore passed the boundary check but ran into runPackNew's `SLUG_PATTERN` whitespace rejection with a confusing "must be lowercase alphanumeric…" error. Now we trim before forwarding. New regression test asserts `{"slug": " pack-trim ", "sut_type": " api "}` → 201. 2. **Multi-tenant clarification** (Copilot api.ts:363): the scaffold endpoint ignores `x-aqa-org` / `x-aqa-project` and always writes to a single server-global `ctx.projectRoot`, unlike the other pack endpoints which are scope-aware. Documented this as intentionally single-tenant-only with a long inline comment explaining the tradeoff and what a multi-tenant deployment would need to do (front this endpoint with a per-tenant projectRoot via separate processes or a routing proxy). In-process per-tenant scaffolding is intentionally out of scope for v1.7 — it requires material design work around directory layout, aqa run discovery semantics, and FS-level vs API-level isolation. 3. **Admin license auto-default override** (Copilot app.tsx:4228): the wizard initialized `license = 'Apache-2.0'` and unconditionally forwarded it on submit (because `.trim()` is truthy), even when the user never opened Advanced. This silently overrode whatever default runPackNew would otherwise pick. Now the field starts empty and is only forwarded when the user actually types something. The License input shows "Apache-2.0 (default if left blank)" as a placeholder + a hint line so the user knows what they'd get by leaving it blank. ## Stale re-flags (no action needed) - api.ts:350 (sync FS in async handler): inline comment explaining the tradeoff was added in iter 1; iter-2 review repeated the same flag AND a separate acknowledgment with future-proofing advice. The future-proofing (setImmediate yield or worker offload) is noted in the inline comment for when this hits a high-fanout path. - api.test.ts:38 (tmpdir leak): the `after` cleanup hook was added in iter 1; iter-2 review re-flagged the original code from before the cleanup landed. Tests: @aqa/server 25 (was 24, +1 trim regression). @aqa/admin 40 Playwright (4 wizard tests still green). Lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
iter 3 on |
Comment on lines
+359
to
+372
| // Trim the required string inputs before forwarding so a caller | ||
| // who sends `" pack-x "` gets the right validation outcome | ||
| // (it's a clean slug) rather than runPackNew's "must be | ||
| // lowercase alphanumeric" error rejecting the whitespace. | ||
| // The admin wizard already trims; this normalizes for direct | ||
| // API callers too. | ||
| const result = runPackNew({ | ||
| root: ctx.projectRoot, | ||
| slug: body.slug.trim(), | ||
| sutType: body.sut_type.trim(), | ||
| ...(body.force !== undefined ? { force: body.force as boolean } : {}), | ||
| ...(body.description !== undefined ? { description: body.description as string } : {}), | ||
| ...(body.author !== undefined ? { author: body.author as string } : {}), | ||
| ...(body.license !== undefined ? { license: body.license as string } : {}), |
Comment on lines
+4280
to
+4281
| title="Create pack" | ||
| sub="Scaffolds a runnable pack under <project>/packs/<slug>/. Same code path as `aqa pack new` on the CLI." |
| const msg = e instanceof Error ? e.message : String(e); | ||
| const full = `Could not reach /api/packs/scaffold (${msg}). The admin is in mock-data mode or the server is down — no files were written.`; | ||
| setError(full); | ||
| toast.push({ kind: 'error', title: 'Create pack failed', body: msg }); |
Comment on lines
+332
to
+338
| // Required fields — strict type + non-empty check. | ||
| if (typeof body.slug !== 'string' || body.slug.trim() === '') { | ||
| return asResponse({ error: 'slug is required (non-empty string)' }, 400); | ||
| } | ||
| if (typeof body.sut_type !== 'string' || body.sut_type.trim() === '') { | ||
| return asResponse({ error: 'sut_type is required (non-empty string)' }, 400); | ||
| } |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…title; toast parity PR #26 iter 3 review fixes (Copilot — 4 real new items, 4 stale re-flags already addressed in prior iters): ## Real fixes 1. **Empty/whitespace optional strings forwarded to runPackNew** (Copilot api.ts:372): a request like `{"description": " "}` was forwarded as-is, baking a blank `description:` line into the generated `pack.yaml`. The endpoint now trims AND drops empty values from the forwarded payload, so the scaffolder falls back to its own sensible defaults (e.g. "Pack scaffolded by aqa pack new"). New regression test asserts the resulting `pack.yaml` carries the fallback description, not a blank line. 2. **Modal title stuck on "Create pack" after success** (Copilot app.tsx:4281): the modal kept its pre-submit title/sub even when showing the success alert. Now flips to "Pack created" / "Your pack is on disk and ready to edit…" when the result panel is active, so the modal chrome matches its current state. 3. **Toast/inline-alert mismatch on network failure** (Copilot app.tsx:4270): the HTTP-failure branch's toast surfaced the server's error string, but the network-failure branch's toast surfaced only the cryptic exception message — losing the helpful "admin is in mock-data mode or the server is down" context that the user needs most. Toast body now uses the same `full` string as the inline alert. 4. **API boundary slug length** (Copilot api.ts:338): the boundary comment promised "strict boundary validation" but slug length is actually delegated to `runPackNew` (MAX_SLUG_LEN=52). Added an inline note explaining that delegation is intentional — single source of truth for the cap means the boundary follows automatically if the kit ever changes the limit. ## Stale re-flags (no action needed) Iter 3 also re-flagged: sync FS in async handler (documented iter 1), tmpdir leak (cleanup added iter 1), multi-tenant scope (documented iter 2). All verified against the current tree. Tests: @aqa/server 26 (was 25, +1 empty-optional regression). @aqa/admin 40 Playwright (4 wizard tests still green). Lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
iter 4 on |
5 tasks
lopadova
added a commit
that referenced
this pull request
May 19, 2026
…#28) * feat(slice-4b): admin Import-manifest wizard + POST /api/packs/import v1.7 slice 4b — wires the "Import manifest" placeholder button on the Packs page to a real wizard that POSTs a YAML pack manifest to a new server endpoint, which parses + validates + installs into the store. ## Server side (@aqa/server) - New `POST /api/packs/import` endpoint (`packs:install` permission) - Body: `{ yaml: string, force?: boolean }` - Parses YAML server-side (the existing `POST /api/packs` accepts pre-parsed JSON; this endpoint accepts raw `pack.yaml` text so the admin doesn't need to YAML-parse client-side) - Validates against `@aqa/schemas/PackManifest` (canonical Zod schema) - On duplicate name without `force=true`, returns 409 with `code: 'EEXIST'` (reuses the structured-error pattern from PR #26) - Adds `yaml` workspace dep to `@aqa/server` - 8 new unit tests: happy-path 201, missing body.yaml, YAML parse error, schema-invalid manifest, 409 duplicate, force=true overwrite, non-boolean force rejected, permission requirement ## Admin UI (@aqa/admin) - New `<ImportManifestWizard>` modal opened from the (previously silent) "Import manifest" button - YAML textarea + native file picker for loading a `pack.yaml` from disk; selecting a file fills the textarea, submit still requires explicit click - Force-overwrite checkbox surfaces the 409-retry path - Result panel shows installed name + version + next-step guidance (matching pack directory still needs to exist on disk for `aqa run` to discover it) - All errors render inline as `<Alert kind="error" role="alert">` (a11y carried over from slice 4a) - 4 Playwright e2e tests: open/disabled-submit, happy-path 201, schema-validation 400 keeps wizard open, 409 → toggle force → retry succeeds ## Audit doc - Lines 6850/6853 marked **DONE** (Install pack via PR #26 + Import manifest via this PR) - Slice 4b checklist item marked **SHIPPED** ## Tests - @aqa/server: 34 unit (was 26, +8 import tests) - @aqa/admin: 55 Playwright (51 existing + 4 import wizard) - Lint + typecheck clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(slice-4b): 2xx-without-pack as error, file-input cleanup, TS narrowing PR #28 iter 1 review fixes (Copilot — 2 real items + CI typecheck): ## Workspace TS narrowing (CI) `let posted: T | null = null` inside a `page.route` closure narrows to `null` in the outer scope under the workspace TS config, so `posted?.yaml` becomes a `never` property access. Same pattern + fix as PR #27 iter 2: use a `const captured = { value: ... }` wrapper — TS doesn't narrow object properties through closures. Applied to both `happy-path` and `409 duplicate` tests. ## Real iter-1 fixes 1. **2xx + empty/non-JSON body treated as success** (Copilot app.tsx:4735): a 200 response with empty body left `parsed = null` (falsy), so `setResult(null)` kept the wizard in form-state while the success toast fired. The user saw a success notification but the modal didn't transition to the result panel — confusing. Now the wizard explicitly checks for the documented `{pack: {...}}` shape in the response. If it's missing (empty body, non-JSON, or unexpected shape) we treat it as an integration error and surface a clear message. New regression test: a 200 with empty body shows the error alert and does NOT show the result panel. 2. **File-input UX** (Copilot app.tsx:4716): after successfully reading a file into the textarea, the wizard didn't clear a stale "could not read file" error from a prior selection. Additionally, the `<input type="file">` value persisted, so re-selecting the same file didn't fire `onChange` in Chrome/Edge. Both fixed: `setError(null)` on success, and `fileInput.value = ''` in a `finally` block so re-selection works reliably. Tests: @aqa/admin 56 Playwright (was 55, +1 for 2xx-without-pack). Lint + typecheck clean (locally + workspace-wide). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(slice-4b): concise Zod errors, code assertions, apiUrl in Create-pack, audit doc PR #28 iter 2 review fixes (Copilot — 6 real new items + 2 stale re-flags): ## Real fixes 1. **Verbose Zod error.message** (Copilot api.ts:456): the schema-invalid response surfaced `validated.error.message`, which is a multi-line JSON-ish dump (often 5KB+) and unreadable in the admin's inline alert. New `formatZodError(err)` helper walks `err.issues` and produces a concise `path: message; path: message` list ("applies_when.sut_type: Required" etc). Falls back to a truncated `message` if `issues` is missing. Test asserts the error is not a multi-line dump. 2. **Test code-field assertions** (Copilot api.test.ts:214 + 228): the YAML-parse-failure and schema-invalid-manifest tests asserted only the status + error string, not the structured `code` field. Both now assert `code === 'EINVAL'` to lock the contract. 3. **CreatePackWizard didn't use apiUrl()** (Copilot app.tsx:4739): ImportManifestWizard correctly used `apiUrl(...)` to honor VITE_AQA_SERVER_URL, but CreatePackWizard still hard-coded the relative path. Now both pack wizards go through `apiUrl()`, keeping the deployment story consistent. 4. **`POST POST` in audit doc** (Copilot audit-doc:57): the wizard description double-prefixed "POSTs to `POST /api/packs/import`". Reworded to "calls `POST /api/packs/import`". 5. **Endpoint-consolidation note** (Copilot api.ts:466): the pre-existing `POST /api/packs` route doesn't validate or conflict-check (MemoryStore.installPack silently overwrites), while `/api/packs/import` does both. Consolidating onto a shared validate-then-install helper is the right architectural move, but it'd change long-standing behavior in the JSON route that callers may depend on — out of scope for this slice. Added an inline NOTE on the `POST /api/packs` handler tracking it as a v1.7.x follow-up and steering callers toward `/api/packs/import` for safety guarantees. ## Stale re-flags (no action) `app.tsx:4746` (2xx-without-pack as error) and `app.tsx:4721` (file-input clear-error + reset-value) were both addressed in iter 2's c8fede7 push — Copilot's iter-2 review re-flagged them based on stale line numbers. Tests: @aqa/server 34 (unchanged count; 2 tests upgraded to assert `code`). @aqa/admin 56 Playwright. Lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes v1.7 slice 3 — the admin's "Install pack" placeholder button (audit doc line 6850) is now a real Create pack wizard that POSTs to a new
POST /api/packs/scaffoldendpoint, which delegates to the samerunPackNewfunction used by theaqa pack newCLI from PR #25.This is the third of four planned v1.7 slices:
docs/PACK-AUTHORING.md(PR feat(kit): v1.7 slices 1+2 — pack-authoring tutorial + aqa pack new CLI #25, v1.7.0-rc.1)aqa pack newCLI (PR feat(kit): v1.7 slices 1+2 — pack-authoring tutorial + aqa pack new CLI #25, v1.7.0-rc.1)Test plan
bun --filter @aqa/server test— 26 tests pass, including 7 new scaffold-endpoint testsbun --filter @aqa/admin test(Playwright) — 40 e2e tests pass, including 4 new Create-pack wizard testsbun --filter @aqa/kit test— 54 tests pass (unchanged; the runPackNew export change is API-only)bun run typecheck— clean across all packagesbun run lint— clean (4 pre-existing warnings, 0 errors)bun --filter @aqa/admin build— Vite build succeeds (423 KB JS / 115 KB gzip)What's wired
Endpoint (
@aqa/server):POST /api/packs/scaffold— body{slug, sut_type, force?, description?, author?, license?}, returns201 {pack_dir, files}on success or400/409on validation/conflictApiContext.projectRoot(never client-supplied) for security@aqa/kitadded as a workspace dep so the server can callrunPackNewdirectly — same code path as the CLIWizard (
@aqa/admin):<CreatePackWizard>modal opened from a renamedCreate packbutton on the Packs screenWhat's intentionally NOT in this PR
projectRootisn't yet wired to a real bootstrap path in any deployment context — that's a tiny follow-up (env var or CLI flag) once we ship a real server binary🤖 Generated with Claude Code