Conversation
The cockpit already installs as a desktop app in Chrome and in Safari 18, but ships no manifest, so the installed window wears the org mark and a browser-derived Dock label. Specs the manifest, the cezar icon set behind it, and the three static routes that let either be fetched at all. Service worker and offline support are rejected on their own merits (a worker at scope / on localhost:4321 outlives the process and 4321 is also Astro's dev port); Web Push is deferred behind the hosting decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
matgren
added
documentation
🏷️ label rationale
|
📦 npm preview published —
|
|
🤖 |
pat-lewczuk
left a comment
pat-lewczuk
left a comment
There was a problem hiding this comment.
🔍 Code Review
Specification review: .ai/specs/2026-08-31-installable-cockpit-web-app-manifest.md
🎯 Summary
This is a spec-only PR — the diff is one new 362-line design document under .ai/specs/ and nothing else — so it got the specification review rather than the code checklist: the document was read end to end and then every claim it makes about this codebase was checked against the worktree at the PR head.
The grounding result is genuinely strong. Fourteen separate claims were verified and all fourteen hold: static-ui.ts really does pass through only /api/*, /assets/* and /open-mercato.svg (packages/cezar/src/server/static-ui.ts:28); packages/cezar/package.json's files array really is ["dist", "web/dist", "scripts", "README.md"], so web/dist really does cover new public/ assets; findPackGaps really is a two-item presence check and not a whitelist (packages/cezar/src/pack-check.ts:18); ASSET_CACHE_CONTROL really is scoped to the fingerprinted /assets/:file route alone (packages/cezar/src/server/server.ts:1417); applyResolvedTheme(root, resolved) really is root-scoped with a no-side-effects header, and every one of its tests really does pass a detached document.createElement('html') (packages/web/src/lib/theme.test.ts:127); pickPort really walks start + 50, i.e. 4321→4370 (packages/cezar/src/index.ts:319); --background: #0d0d0d really is the dark token (packages/web/src/styles/index.css:79); the favicon comment in index.html really is stale, and in a second way the spec did not even claim — it says "Relative" about an absolute href="/open-mercato.svg"; app.routes really is established test machinery (bc-route-inventory.test.ts:120, versioned-surface.test.ts:93); and no existing test really does assert a 200 on /open-mercato.svg. A spec that describes its own codebase this accurately is rare, and the design decisions built on those facts — relative start_url/scope, id: "/", separate any and maskable artwork, keeping ASSET_CACHE_CONTROL off stable filenames, putting the theme-color write beside applyResolvedTheme rather than inside it — are all correct and correctly argued. The rejection of the service worker is the strongest section in the document.
What blocks it is not the design but the plan's executability and its verification story. Phase 1 step 1 sources its artwork from /Users/maciejgren/Documents/cezar-logo.svg, a path on one contributor's laptop, and every later phase depends on that step. Separately, the spec routes all of its meaningful assertions into npm run test:e2e on the belief that this is a gate — it is not: no workflow in .github/workflows/ invokes it, it is absent from .ai/agentic.config.json's validation.commands, and .ai/scripts/e2e.sh is explicitly designed to exit 0 when the browser provider cannot be provisioned. That single mistaken premise is what makes the spec's own most-emphasized requirement — that cache-control must not reach these routes — unverified in practice.
Verdict: ❌ Request changes. One blocker and three majors. None of them is a disagreement with the design; all four are fixable inside the document.
🧪 Validation Gate
| Command | Result | Note |
|---|---|---|
npm run typecheck |
⏭️ skipped | Docs-only diff — no TypeScript changed. |
npm test |
⏭️ skipped | No test reads .ai/specs/; the three files that mention the directory do so only in comments. |
npm run test:unit |
⏭️ skipped | Same — nothing under test consumes the spec corpus. |
npm run build |
⏭️ skipped | Nothing to compile; public/ is untouched by this PR. |
npm run test:package |
⏭️ skipped | The tarball is unaffected; .ai/ is not in the published files list. |
Per the spec-only review path, only the docs-applicable subset of validation.commands runs, and this repository configures no markdown linter or link checker, so the whole gate is legitimately skipped and listed rather than run. The three GitHub checks on the PR — "Unit, build, E2E, and package", "Publish npm snapshot", and license/cla — are all green, and no check is pending.
💥 What can go wrong
Blocker — Phase 1 step 1 cannot be executed by anyone but the author, and everything depends on it.
.ai/specs/2026-08-31-installable-cockpit-web-app-manifest.md:279 names the source artwork as /Users/maciejgren/Documents/cezar-logo.svg — an absolute path on one contributor's machine, not a file in this repository and not a URL anyone else can fetch. The spec then compounds this at :138 by deciding, correctly on its own terms, that the four PNGs are committed rather than generated because "the repo has no image toolchain". The combination leaves the first step of the first phase requiring a hand export, from a file only one person has, using a rasterizer the spec has just declined to add. Every subsequent step in Phase 1 (:287 writes the manifest whose test asserts "every src resolves to a file that exists in public/"), all of Phase 2 (:318), and all of Phase 3 (:331) are blocked behind it. This spec is written to feed om-auto-implement-spec; an agent picking it up would hit step 1 and have nothing to do. The colour values at :131 (#9655fd plate, #1b1b1b mark, 176×176) describe the artwork but do not substitute for it. Resolve it by committing cezar-logo.svg and the derived PNGs to this PR alongside the spec, or by naming an in-repo or otherwise reachable source and recording an explicit "artwork must land before Phase 1 starts" precondition in the phasing section.
Minor — display: "standalone" collides with the port-fallback failure the spec itself documents.
:153 picks standalone, and :225 accepts that "an app installed at 4321 opens a dead page on a day cezar landed on 4322" as out of scope. Those two decisions are fine apart and bad together: standalone is precisely the display mode with no address bar, so the user hits the dead page in the one window that offers no way to navigate to 4322. This is the same argument the spec uses at :73 to reject the service worker — "a window with no address bar to hard-reload from". display: "minimal-ui" keeps a URL/reload affordance, costs one token, and does not otherwise change the install; at minimum the spec should record why standalone still wins given its own port caveat.
Minor — the /icons/:file guard is described with only half of the /assets/:file protection.
:178 says the route works "as /assets/:file, reusing isSafeAssetFilename and assetContentType", but :220 then describes the runtime check as just "its existsSync check". The real /assets/:file handler at packages/cezar/src/server/server.ts:1421 guards with !existsSync(path) || !statSync(path).isFile(), and the isFile() half is load-bearing: isSafeAssetFilename rejects separators but not a plain name that happens to be a directory on disk, and readFileSync on a directory throws EISDIR — a 500 where a 404 belongs. Say existsSync and statSync(...).isFile() explicitly so the implementer copies both.
🔁 Backward compatibility
The spec handles the protected surface correctly and deliberately. BACKWARD_COMPATIBILITY.md §2 does list /assets/:file and /open-mercato.svg as protected static paths (BACKWARD_COMPATIBILITY.md:27), the spec adds the three new paths to that list at :307 rather than quietly relying on them, it keeps /open-mercato.svg and its route in place at :188, and it names the durable commitment as an intended bargain at :258. Phase 3's favicon switch (:331) changes what index.html points at without removing the old URL, which is the right shape. No API, schema, event, CLI, or stored-data surface is touched, and the claim at :212 that versioned-surface.test.ts and bc-route-inventory.test.ts both filter on /api/ is accurate — registeredApiRoutes at bc-route-inventory.test.ts:78 and inventoriedApiRoutes at :92 both do exactly that, so the new static paths are invisible to them. That is a clean, no-breaking-change design with a named commitment, and it needs no waiver.
Minor — step 7's proposed guard is one-sided and would pass vacuously the moment the route is deleted.
:307 proposes closing the §2 gap for static paths with "one expect(doc).toContain(path) per new route". That pins the document containing a string; it says nothing about whether the route is still registered. Delete the route from server.ts and all three assertions stay green — which is the drift bc-route-inventory.test.ts exists to catch, and the file already guards explicitly against exactly this class of vacuous pass at :136 ("guards against a vacuous pass"). The machinery for a two-sided check is right there: read the static paths off app.routes and require each to be named in §2, the same way registeredApiRoutes does for /api/v1/*.
🧩 What's missing
Major — npm run test:e2e is not a gate, so the spec's byte-level assertions never run.
:255 states the testing split as "everything requiring bytes on disk in npm run test:e2e, which boots a built app", and Phase 1 step 5 (:299) puts the entire meaningful verification there: 200 plus application/manifest+json, 200 plus image/png, and the absence of cache-control. The premise is wrong on three counts. First, no workflow runs it — .github/workflows/ci.yml runs npm run typecheck (line 44), npm run test:unit (47), npm test (50), npm run build (53) and npm run test:package (56), and test:e2e appears in no workflow file at all. Second, it is not in .ai/agentic.config.json's validation.commands, so the pipeline's gate never invokes it either. Third, even when someone runs it by hand, .ai/scripts/e2e.sh:22 documents that it exits 0 with TEST_E2E_STATUS=skipped whenever the browser provider cannot be provisioned — deliberately non-blocking. CODE_REVIEW.md:3 and SDLC.md:93 both classify it as the QA layer, not the correctness gate, and this PR carries skip-qa. The net effect is that the requirement the spec calls out most emphatically — :184, "ASSET_CACHE_CONTROL must not reach these routes" — would ship with no automated check anywhere, which is a strictly worse outcome than the vacuous-404 assertion the spec rejected at :253 for being misleading. There is a home for these assertions that is in the gate: npm run test:package runs packages/cezar/test/e2e/*.test.ts after npm run build in CI, and package-cli.test.ts already inspects the packed file list and installs and drives the tarball, so it can both assert the icon and manifest files ship and boot the installed server to check the response headers. Rework the testing split around that.
Major — nothing guards that public/ assets actually reach the tarball, and §2 now promises they do.
:50 correctly observes that findPackGaps "asserts web/dist/index.html and at least one web/dist/assets/* exist, so it is a presence check, not a whitelist" — and then treats that as reassurance. It is the opposite: because it is a presence check over exactly two things, nothing anywhere verifies that manifest.webmanifest, cezar-logo.svg or icons/* are in the published package. Vite's publicDir copy is the only mechanism putting them there (packages/web/vite.config.ts sets root: appDir and outDir: packages/cezar/web/dist, with the default public/), and a future config change, a publicDir: false, or a build refactor would silently publish a tarball whose §2-protected URLs all 404, with every gate green and check:pack satisfied. Once :307 makes those three paths a protected surface the guarantee needs a guard. One line added to findPackGaps, or one entry in package-cli.test.ts's packagedPaths required list (which already checks web/dist/index.html), closes it — and unlike step 5's e2e assertions, both of those run in CI today.
Major — Phase 4 step 15's "extract the IIFE's decision into a testable function" contradicts a documented repo contract and does not achieve what it claims.
:345 proposes extracting the pre-paint script's light/dark decision into a testable function "rather than leaving it inline — nothing in the repo currently executes that script, so an inline-only change is unverifiable", with a unit test over cez-theme values as the verification. The problem is that the duplication is deliberate and documented: packages/web/src/lib/theme.ts:5 says "packages/web/index.html's inline script duplicates resolveTheme + applyResolvedTheme on purpose — it has to run before the bundle exists. Change one, change the other." The extraction cannot change that: an inline <script> in <head> cannot import a module without becoming type="module", which defers past first paint and reintroduces exactly the flash the script exists to prevent. So the extracted function would live in theme.ts — where resolveTheme already lives and is already table-tested at theme.test.ts — and the unit test would exercise a copy of the shipped logic, leaving the inline script as unverified as before. That is the precise failure the step says it is fixing. The spec should either drop the extraction and keep the existing "change one, change the other" convention while stating it, or propose something that actually closes the gap — build-time inlining of the function into index.html, or a DOM-level assertion in a suite that loads the real built index.html.
Nit — the spec skeleton is missing the sections this repo's recent specs use for exactly this content. There is no Resolved assumptions (autonomous defaults) / Open Questions section and no Non-goals section, both of which appear in the comparable specs in this directory (2026-08-09-issue-linked-pr-chip.md, 2026-07-30-foldable-task-table-columns.md, 2026-07-30-session-usage-metrics.md). The content exists but is scattered into prose: the non-goals are a one-line "No service worker, no offline mode, no push" at :16, and the one genuinely unresolved question — whether browsers fetch start_url and the manifest with basic-auth credentials on a hosted server-install instance, at :230 — is buried in Edge Cases as "This needs verifying rather than assuming" instead of being tracked where a reader looks for open items.
📈 How can this specification be improved
The document is unusually well written — the prose carries its reasoning instead of asserting conclusions, the "Research: what comparable projects do" section (:97) earns its place by producing three decisions the spec would otherwise have missed rather than name-dropping projects, and "Doing nothing stays viable and is the honest baseline" (:94) is the right note to end an alternatives section on. The phase dependency graph at :264 is explicit about what blocks what, and Phase 2's parenthetical about why it cannot ship before Phase 1 (:270 — the <link> would fall through to the catch-all and receive index.html as text/html) is the kind of detail that prevents a real bug.
Two improvements beyond the findings above. First, most of the acceptance criteria for the actual user-visible outcome are manual (:311, :325, :340, :361) — which is honest, since no automated check can see a Dock label — but they are written as prose checklists inside numbered steps rather than as criteria someone can tick off, and the four "Manual acceptance" steps would be more useful gathered where a QA reviewer will find them. Second, :284 proposes a unit test that reads each PNG's IHDR chunk to assert width, height and colour type 6, which is a genuinely good idea for catching a bad export; it is worth saying explicitly that this test belongs in a suite that runs against packages/web/public/ (which exists at checkout time) rather than web/dist/ (which does not), because that is the same trap the rest of the testing plan fell into.
✂️ Is this the simplest possible solution
Broadly yes, and the scope discipline is the best thing about this spec — cutting the service worker, rejecting vite-plugin-pwa as mostly-Workbox, deferring Web Push to the HTTPS-origin work, and sending "keep the server alive" to its own spec are four correct decisions that each shrink the change. Committing static PNGs rather than adding sharp or resvg to packages/web is the right trade for four files, and the spec is honest about the cost at :140. The four phases are each independently shippable and the dependency graph is minimal.
Minor — the param-route justification is inverted. :180 argues that "one param route rather than four literals keeps the guard surface to the one already written and unit-tested." It is the other way round: four literal staticFile() registrations — the /open-mercato.svg precedent at server.ts:1431 — have no filename guard surface at all, because there is no user-controlled path segment. Choosing /icons/:file is what creates the need for isSafeAssetFilename, the statSync().isFile() check, the path.startsWith('/icons/') entry in isStaticAsset (:292), and the two extra route tests at :294 and :298 for /icons/a/b.png and /icons/... Reusing an existing tested guard is a perfectly defensible choice and the resulting design is safe — but the stated reason for it is backwards, and a reader deciding between the two options from this paragraph would be misled. Either fix the rationale (the honest one is "one registration that scales as icons are added, at the cost of a guarded path segment") or take the literal routes, which delete an entire class of edge case from Phase 1.
🧪 Test Coverage
Not applicable in the usual sense — the diff adds no code, so there is nothing to cover. The spec's proposed coverage is assessed above and is where three of the four findings land: the pure-function tests (:284, :288) and route-table tests (:296) are well chosen and genuinely run without a build, but the assertions that would prove the feature works are parked in a suite no gate invokes, the tarball contents are unguarded, and Phase 4's proposed unit test would verify a copy rather than the shipped script.
🔁 Breaking Changes
None. No protected contract surface is broken: three static paths are added to BACKWARD_COMPATIBILITY.md §2 rather than removed or moved, /open-mercato.svg and its route survive Phase 3 explicitly, and no /api route, schema, migration, event, CLI flag or stored-data shape is touched. Already-installed apps degrade to the favicon rather than breaking, as :246 notes. No waiver is needed and none is claimed.
This review was produced by om-auto-review-pr on the PR head, in an isolated worktree. Autofix: skipped (not my PR — re-run with --autofix to fix it here).
pat-lewczuk
added
changes-requested
|
🤖
|
|
Thanks @matgren — review found actionable items, so I'm handing this PR back to you for the next pass. When the updates are pushed, re-request review and the automation can pick it up from the latest head. |
|
🤖 Specification review of the spec-only diff: 1 blocker (Phase 1 step 1's artwork source is a path outside the repository), 3 majors ( |
2 participants
What this is
Design only. One new file under
.ai/specs/; no code changes. Implementation ships on its own PR referencing this one.The cockpit already installs as a desktop app today — Chrome's "Install page as app" and Safari 18's "Add to Dock" both work against
http://localhost:4321, because localhost is a secure context. What they produce is a window named after<title>wearing the Open Mercato org mark, becausepackages/cezar/web/distships nomanifest.webmanifest. There is noshort_nameeither, and the Dock label is manifest-owned, so no amount of runtimedocument.titlework can influence it.The spec adds the manifest, the cezar icon set behind it, and the three static routes that let either be fetched at all.
Scope
Four phases with explicit dependencies. Phase 1 stands alone; Phase 2 needs Phase 1's
/icons/:fileroute; Phases 3 and 4 are independent of each other and of Phase 2.any+maskableicons, three routes, the BC §2 inventory entries.apple-touch-iconandapple-mobile-web-app-title.What was rejected, and why
A service worker with an offline fallback was scoped in and cut. A worker at scope
/onhttp://localhost:4321outlives the cezar process and keeps controlling that origin, and 4321 is also Astro's default dev port, so the next dev server started there would be intercepted by a cockpit that is no longer running. A cached shell also survives annpxupgrade and would boot old markup against a new API inside a window with no address bar to reload from. And the fallback page would claim "cezar isn't running" precisely when cezar has hopped to 4322 and is running.vite-plugin-pwais rejected for the same reason: most of what it buys is Workbox and a generated service worker.Keeping the server alive (launchd,
--daemon, a launcher.app) is the better answer to "the icon does not startcez", and it is a different capability — its own spec.Web Push is deferred: it is blocked on a reachable HTTPS origin, not on anything here.
Constraints the spec had to design around
npm testbeforenpm run build, andresolveWebDir()is hardcoded with no injection point, so in the unit suitesweb/distdoes not exist and these routes always 404. Byte-level assertions therefore live innpm run test:e2e; the unit suites cover the pure functions, the Hono route table, and the 404 paths. A "nocache-controlheader" assertion against a possible 404 would pass vacuously.ASSET_CACHE_CONTROLmust not reach the new routes — the immutable year-long cache is correct only for Vite's fingerprinted/assets/names.applyResolvedTheme(root, resolved)is root-scoped by contract and its tests pass detached elements, so the theme-color write goes intheme-provider.tsxbeside the call, not inside it.BACKWARD_COMPATIBILITY.md§2 lists the static surface by hand and nothing machine-checks it for non-/apipaths; the spec adds threeexpect(doc).toContain(path)lines tobc-route-inventory.test.tsto close that.Closed #354 listed "No PWA/offline support, no native wrappers" as out of scope. This reverses the first half narrowly: installability and icon metadata only, with the service worker rejected on its own merits rather than by inheritance.
🤖 Generated with Claude Code