feat(om-test-drive): boot an Open Mercato PR, prove login, seed it, hand over the keys - #45
Conversation
…and over the keys
Reviewing a change by hand is mostly setup tax: boot a throwaway app from the
branch, wait out a full build, find out whether login even works, work out which
screens the diff actually touches, then discover the feature is invisible because
nothing in the seeded data exercises it.
`explain` deliberately stops at the diff — it says so itself ("not a verification
run (a verify skill runs the app)"). `test-drive` is that verify step, aimed at a
person rather than a pipeline: boot on a disposable instance, prove auth with a
real HTTP round-trip, seed the data that makes the change visible through the
app's own API, and hand back a click route with a live URL and credentials.
Open-Mercato-aware — it knows `mercato test:ephemeral`, the ready line, the
ephemeral state file, and the credentials that env pins — behind a discovery
ladder so any other repo degrades to its own documented boot. No browser: login
is curl'd, the walkthrough is written.
The guardrails are the point. Never write to the database directly (a row
inserted behind the app skips validation, events, and indexing). Never seed
against a database that isn't the throwaway one. And don't demo a build you
can't attribute: the ephemeral command silently attaches to an already-running
instance based on file mtimes and a TTL, which after a branch switch hands you a
running build of different code while everything looks fine.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…o PR Validated the skill end-to-end against fullstackhouse/open-mercato#93. The boot phase was wrong in ways that would have stopped the skill dead on its first run: four consecutive failures before an instance came up, none of which the skill gave any guidance for. The ephemeral command is not a from-zero installer. It assumes a repo already installed and built once, and it runs `initialize` *before* its own codegen and build steps — so a fresh tree, or any checkout that moves the lockfile, fails on missing artifacts with errors naming the symptom rather than the cause: Couldn't find the node_modules state file -> yarn install Cannot find module packages/cli/dist/bin.js -> yarn build:packages Cannot find module .../entities.ids.generated.js -> yarn generate + build:packages The second is a genuine chicken-and-egg: `yarn mercato` *is* the built CLI, so the thing that orchestrates the build must itself be built first. The third presents as a database failure — every migration applies, then bootstrap dies on codegen. Phase 2 now carries the sequence (which the root `build` script already encodes) and an error-to-missing-rung table. Also corrected, both observed rather than reasoned: - `gh pr checkout <N>` needs an explicit `--repo`. A checkout with a fork alongside its upstream has no default repo set and the bare form errors out. - Never derive the change surface from an unfetched local base ref. The local `develop` here was five weeks stale, turning a 15-file PR into a 4,141-file diff — a wrong answer that looks like a right one. - `--verbose` is not the first response to a failed boot; it adds log volume, not a missing artifact. It earns its place only once the tree is bootstrapped. Phase 4 gains the route mapping this PR demonstrated (backend/<path>/page.tsx is /backend/<path>, and detail pages re-export each other, so one changed component surfaces under several routes). Phase 5 gains: read the change's own integration spec first — it is the author's recipe for the state the change needs. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The instance is now up and the full drive completed against fullstackhouse/open-mercato#93, which surfaced two problems the earlier fixes had not. A throwaway instance runs in production mode, and `apps/mercato/.env` ships `JWT_SECRET=change-me-dev-secret` straight from `.env.example` — a value the app's own guard rejects as a published placeholder. The build succeeds, the server starts, and only then does it exit, so the boot reports nothing but "Application process exited before readiness check" while the actual refusal sits in the app's stderr, visible only under --verbose. Phase 2 now names this class and supplies real secrets through the environment rather than editing the user's .env, which Hard rule 6 forbids. The larger correction is to Hard rule 1, which invited an overclaim. It demanded every click-route URL return 200 and every record be read back, and implied that constituted proof the change works. It does not, in two ways observed here: - `/backend` answers 307 anonymously. That is the login redirect behaving correctly, and the old rule would have read it as a broken route. - With the login cookie the order detail page answers 200 with 1.3 MB of HTML that contains none of the seeded values, because the backend is client- rendered and fetches its data after hydration. Grepping that HTML for the change finds nothing even when everything works. So curl can prove the route resolves and the data persisted; it cannot prove the UI paints it. That ceiling is now stated in the hard rule, in Phase 3 alongside the cookie-jar recipe and how to read 307 vs 200, and as the first line of the handover's "what this drive can't show". Verified end to end: instance on :60770 from c361d3caa4, login round-trip 200 with a working token, order ORDER-20260824-00001 seeded via the PR's own spec recipe and read back carrying phone, taxId and taxIdType. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The skill is Open Mercato-first in substance, not just by default: its boot ladder, bootstrap ordering table, credentials, ephemeral state file and config traps are all Mercato specifics earned by running it against a real PR. The name should say so rather than promising a generality the body doesn't carry. The generic fallback stays — a non-Mercato repo still degrades to its own documented boot via the Throwaway instance profile knob — but the skill is no longer listed among the repo-agnostic ones, where the name would contradict the roll-call. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Adds an Open Mercato test-drive workflow for booting disposable instances, verifying authentication, seeding data, and providing a click-through handover.
Changes:
- Added the
om-test-driveskill and operational guardrails. - Documented the skill and throwaway-instance profile.
- Bumped the plugin version to
1.2.0.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Summary and final review comments |
|---|---|
skills/om-test-drive/SKILL.md |
Adds the test-drive workflow. Changes required for 10 findings: working-tree diffs are omitted (moderate, 4 votes); fallback setup is gated incorrectly (critical, 2 votes); authentication is hard-coded for Open Mercato (critical, 2 votes); route validation precedes route discovery (moderate, 2 votes); generated artifacts can dirty the worktree (moderate, 2 votes); instance reuse lacks verifiable source identity (critical, 3 votes); session cookies remain in the repository (moderate, 3 votes); stop behavior is inaccurately documented (moderate, 2 votes); frontmatter may be invalid YAML (critical, 1 vote); shared-database seeding contradicts the guardrail (critical, 1 vote). |
README.md |
Documents the new skill and throwaway-instance profile configuration. |
.claude-plugin/plugin.json |
Updates the plugin description and version. |
Suppressed comments (9)
skills/om-test-drive/SKILL.md:176
- The handover template hard-codes
GET <route> → 200, but Phase 3 says the authenticated request is an API route. Successful endpoints can legitimately return 201/204, and a changed route may be POST-only, so this template can report a valid drive as failed or force a meaningless GET. Record the actual method and its expected success status instead.
Verified: POST /api/auth/login → 200; GET <route> → 200
skills/om-test-drive/SKILL.md:82
- On a stock Open Mercato checkout, the CLI's environment loader copies
.env.exampletoapps/mercato/.envwhen the parentNODE_ENVis notproduction, before dispatchingtest:ephemeral. This command therefore mutates the worktree despite Hard rule 6; set the parent mode (or use an isolated worktree) in the documented invocation.
JWT_SECRET=$(openssl rand -hex 32) AUTH_SECRET=$(openssl rand -hex 32) \
yarn mercato test:ephemeral
skills/om-test-drive/SKILL.md:33
- This hard rule conflicts with the explicit fallback in Phase 5: that step allows seeding through a UI form when no API route exists, but this line still requires every seeded record to be read back through an API. Clarify that API read-back is required when available and define the UI verification/limitation for the no-API case.
1. **Hand over nothing you haven't verified, and don't overclaim what you did verify.** Every URL in the click route must have been fetched *authenticated* and resolved. Every seeded record must have been read back through the API. But know the ceiling of a no-browser drive: you have proven the route resolves and the data persisted — **not** that the UI renders it. Say which of the two you checked; never let a 200 stand in for "the change works".
skills/om-test-drive/SKILL.md:45
- The checkout is explicitly scoped with
--repo, but the followinggh pr diff <N>is not. In a fork/upstream clone, that unqualified number can resolve a different repository's PR; carry the resolved owner/name into this command.
- **PR argument** → verify the tree is clean, then `gh pr checkout <N> --repo <owner/name>`. Pass `--repo` explicitly: a checkout with several remotes (a fork alongside its upstream) usually has no default repo set, and bare `gh pr checkout` just errors out. Take the change surface from `gh pr diff <N>`.
skills/om-test-drive/SKILL.md:120
- This only prints the login response; it never assigns
TOKEN, checks for a 200, or verifies that the authenticated request succeeded. As written, the next command can sendBearerand still look like a completed probe, so the skill can hand over an unauthenticated instance.
Expect 200 and a `token` in the body. Then **use the token** — one authenticated request against a route the change touches:
```bash
curl -s -o /dev/null -w '%{http_code}\n' -H "Authorization: Bearer $TOKEN" "$BASE_URL/api/<route>"
skills/om-test-drive/SKILL.md:132
- Because the ephemeral app runs with
NODE_ENV=productionat anhttp://127.0.0.1URL, its login response marks the auth cookiesSecure.curl -b jar.txthonors that flag and will not send them over HTTP, so this page probe redirects anonymously instead of proving page auth; it also leavesjar.txtin the worktree. Extract the cookie pairs and send an explicitCookie:header from a temporary jar (or expose HTTPS).
**Fetching pages, not just the API.** An authenticated page needs the login *cookie*, not the bearer token — grab a jar on login and reuse it:
```bash
curl -s -c jar.txt -X POST "$BASE_URL/api/auth/login" \
-H 'content-type: application/x-www-form-urlencoded' \
--data-urlencode '[email protected]' --data-urlencode 'password=secret' -o /dev/null
curl -s -b jar.txt -o /dev/null -w '%{http_code}\n' "$BASE_URL/backend/<path>"
**skills/om-test-drive/SKILL.md:94**
* `--no-reuse-env` is not guaranteed to boot: the runner acquires a shared runtime lock and refuses a fresh build while another ephemeral process is active. Since this is the prescribed response to an untrusted reuse, tell the agent to stop/report the lock owner before retrying rather than promising a fresh instance.
Useful flags: --verbose (full bootstrap/build logs — worth a re-run once the tree is bootstrapped and a boot still fails silently), --no-reuse-env (always a brand-new instance on an isolated port), --no-screenshots (irrelevant here; this skill doesn't drive a browser).
**skills/om-test-drive/SKILL.md:37**
* This is stated as a universal property even though the skill explicitly falls back to non-Mercato profiles. Those environments may have different email, scheduler, enterprise, and cache behavior, so the handover can make false claims; make the limitations profile-driven and keep this list Open Mercato-specific.
- Say what the drive can't show. A throwaway instance runs with outbound email, the scheduler, and enterprise modules disabled — though queue workers do run, so job-backed flows work. A click route that omits that invites the user to conclude a feature is broken when it's merely switched off.
**skills/om-test-drive/SKILL.md:175**
* The handover template hardcodes Open Mercato credentials even on the non-Mercato fallback. That can hand the user unusable credentials (or invent a credential guarantee); substitute the credentials returned by the selected throwaway profile and keep the OM values in an OM-only example.
- Login: [email protected] / secret
</details>
---
💡 <a href="/fullstackhouse/skills/new/main?filename=.github/skills/code-review/SKILL.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add a `code-review` agent skill</a> or configure MCP servers for context-aware, tailored reviews. <a href="https://docs.github.com/en/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review#mcp-servers-and-agent-skills" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn more in the docs.</a>
| @@ -0,0 +1,219 @@ | |||
| --- | |||
| name: om-test-drive | |||
| description: Boot a change on a throwaway instance, prove login actually works, seed the data the change needs to be visible, then hand over a click-by-click route through it. Use when you want to see a change working before merging — "boot this PR so I can click through it", "set me up to review X by hand", "let me try this". Knows Open Mercato's ephemeral command; degrades to any repo's documented boot. Args: nothing (current worktree), or a PR number/URL to check out first. | |||
There was a problem hiding this comment.
Not taking this one, with evidence. PyYAML does reject the unquoted Args: nothing — verified. But the claimed consequence, rejection before discovery, doesn't hold: seven skills already on main (design-polish, design-explore, review-queue, pr-polish, ticket-refresh, ticket-polish, spec-polish) carry the identical construct and load fine, so Claude Code's parser tolerates it. It's a real latent fragility but a repo-wide, pre-existing one; quoting this skill alone would make the set inconsistent. Leaving the thread open for @jtomaszewski to decide as a repo-wide follow-up.
…lse promises Copilot's review landed ten findings; eight were right and are fixed here. Most of them are places where the skill contradicted itself or promised something it could not deliver. Contradictions between the hard rules and the phases: - Hard rule 3 forbade seeding a non-throwaway database while Phase 2 offered to do it "with consent". Consent does not make another developer's database disposable — the shared-stack path now drops to --no-seed or stops. - Hard rule 1 demanded API read-back of every seeded record while Phase 5 allows a UI-form fallback where no API route exists. Read-back is now required where a route exists, and the no-API case must be declared unverified. - Hard rule 6 said "never edit files" while the bootstrap it prescribes runs yarn install, generate and build:packages. That mutation is unavoidable — the instance is built from the tree — so the rule is now scoped to source, and the handover has to report what was installed, generated or rebuilt and which branch the checkout was left on. Ordering and provenance: - Phase 3 checked "a route the change touches" before Phase 4 discovers routes, and a UI-only change may add no API route at all. It now probes a known collection endpoint and re-checks the changed surfaces after Phase 4, accepting whatever success status each returns rather than demanding 200. - The reuse trap told the agent to prove provenance from the state file. The file records startedAt, a port and a database URL and no source SHA, so a process that started after the checkout can still serve stale build output. Provenance is now treated as unprovable: after any branch switch, always --no-reuse-env. - Node/Docker preconditions and the Mercato bootstrap ran before the boot command was resolved, so a non-Mercato repo would be rejected for lacking Node 24. Profile resolution now comes first. Correctness of what the skill tells the user: - The handover claimed stopping always destroys the instance. When the runner attaches to an existing environment, Ctrl+C only detaches and the data survives for its owner. Now conditional. - The handover hardcoded Open Mercato credentials and its switched-off surfaces even on the non-Mercato path, where both would be false. - The cookie jar was written to the working directory. It is a live session credential; it now goes to mktemp under an EXIT trap. - gh pr diff went unscoped while the checkout beside it used --repo. Two findings are not fixed, with evidence: - "The frontmatter is invalid YAML and will be rejected before discovery." PyYAML does reject the unquoted `Args: nothing`, but seven skills already on main carry the identical construct and load fine, so the claimed consequence does not hold. Real but repo-wide and pre-existing; fixing one skill of twenty would just make the set inconsistent. - "Secure cookies won't be sent over http, so the page probe proves nothing." Measured against the live instance: curl 8.7.1 sends both Secure cookies to 127.0.0.1 and the page returns 200, against 307 without the jar. curl treats loopback as a secure context. Noted inline so the next reader doesn't re-derive it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
On the curl 8.7.1 sends both over plain http to The remaining suppressed comments — hardcoded |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
Suppressed comments (7)
.claude-plugin/plugin.json:4
- The Verification section says the version was bumped from 1.0.0 to 1.2.0, but the changed manifest is 1.2.1. Update the claim or the manifest so the release evidence is internally consistent.
"version": "1.2.1",
skills/om-test-drive/SKILL.md:132
python3is not a stated prerequisite and is not needed by Open Mercato; on a Node/Docker-only machine this command fails to parse a valid login response and then reportslogin failed. Parse with the already-required Node runtime or add an explicit Python preflight.
| python3 -c 'import json,sys; print(json.load(sys.stdin).get("token",""))')
skills/om-test-drive/SKILL.md:90
- This production-config recovery text is not scoped to the monorepo, but a
create-mercato-appscaffold keeps.env.example/.envat the repository root rather thanapps/mercato/.env. A scaffold user following this troubleshooting step will inspect a path that does not exist; derive the env path from the detected profile or limit this block to the monorepo.
**A throwaway instance runs in production mode, so dev-safe placeholders become hard failures.** `apps/mercato/.env` ships `JWT_SECRET=change-me-dev-secret` (straight out of `.env.example`), and the app's own production guard refuses to start on a known placeholder secret. The build succeeds, the server starts, and *then* it exits — the boot reports only `Application process exited before readiness check`, with the actual refusal buried in the app's stderr where you'll only see it under `--verbose`. Supply real secrets for the run instead:
```bash
JWT_SECRET=$(openssl rand -hex 32) AUTH_SECRET=$(openssl rand -hex 32) \
yarn mercato test:ephemeral
Pass them in the environment; do not edit the repo's .env — it's the user's file and this skill is read-only on the tree (Hard rule 6). Treat any "exited before readiness" as this class of problem until --verbose proves otherwise: the app process failing after a clean build is a configuration refusal far more often than a code fault.
**skills/om-test-drive/SKILL.md:87**
* `openssl` is likewise an unstated dependency in the mandatory recovery command; a machine with Node 24 and Docker but no OpenSSL cannot reach the runner. Generate the secrets with Node's crypto module, which is already guaranteed, or preflight `openssl`.
JWT_SECRET=$(openssl rand -hex 32) AUTH_SECRET=$(openssl rand -hex 32)
yarn mercato test:ephemeral
**skills/om-test-drive/SKILL.md:168**
* This Phase 4 lookup omits the explicit `--repo <owner/name>` required above. In a fork/upstream checkout, an unqualified PR number can resolve a same-number PR in the wrong repository and produce a route for the wrong change. Use the resolved repo here as the other PR commands do (for example, `gh pr view ... --json files`).
- Take the changed-file list from
gh pr diff <N> --name-only(or a freshly-fetched base), then map each file to a route with the repo's convention. In Open Mercato a module'sbackend/<path>/page.tsxis/backend/<path>, and detail pages re-export each other —sales/orders/[id]renders thesales/documents/[id]component, so one changed component surfaces under several routes.
**skills/om-test-drive/SKILL.md:101**
* "Run it backgrounded" conflicts with saying the command holds the terminal until `Ctrl+C`. Once launched with `&`, `Ctrl+C` at the user's prompt does not stop that process, so the handover cannot provide the promised exact stop behavior and can leave the container alive. Choose a foreground/persistent terminal and hand over `Ctrl+C`, or capture the process group and document a cleanup command.
Run it backgrounded. The command holds the terminal until Ctrl+C — that's by design, it's what keeps the instance alive for the user.
**skills/om-test-drive/SKILL.md:227**
* This rule says every record is read back, but Hard rule 1 and Phase 5 allow the no-API UI-form fallback to be explicitly unverified. Following this wording forces the handover to overclaim the fallback or contradict itself. Distinguish API records read back from UI-created records marked unverified.
- Every URL is one you fetched authenticated. Every record is one you read back. Neither is a claim about what the screen looks like.
</details>
…files, failing checks Eight more findings, all correct. The most consequential is one I could not have reasoned my way to and had to go read the template for. The discovery ladder assumed a root `test:integration:ephemeral:start` meant the monorepo. It doesn't: the current `create-mercato-app` template defines that same script, has no `build:packages`, and is laid out flat with no `apps/mercato`. So the ladder mislabelled a scaffold as the monorepo and then prescribed a warm-up step that does not exist there. Bootstrap now branches on layout rather than script name — the doubled `build:packages` dance only applies where the CLI is in-tree and has to build itself; a scaffold gets `yarn install && yarn generate`, because its `mercato` ships built as a dependency and there is no chicken-and-egg. The rest: - Untracked source was invisible to the change surface. `git diff` in every form misses `??` rows, so a brand-new route or component could be live in the built instance while the handover said no such surface existed. - The authorization probe printed a status nobody checked. A 401 could scroll past into a handover claiming login was proven; it now exits non-zero. - `--no-seed` skipped the only steps that find or verify a record, so it could produce a handover with nothing concrete in it. It is now read-only, not verification-free: name an existing record that exercises the change, or say the route ends at an empty state. - The handover attributed the build to a HEAD SHA even though Phase 1 now folds in uncommitted work — on a dirty tree that SHA does not identify what is running, and the template says so. - The production-secret example hardcoded the monorepo's `yarn mercato` rather than whichever command the ladder selected. - Token parsing used `python3`, which is not a prerequisite anywhere; a Node-only machine would read a valid login as a failure. Uses `node` now. - The README knob contradicted the fixed hard rule, still promising the skill asks before seeding a shared environment. It refuses and drives read-only. - The same knob had no field for the auth contract the skill now requires, so a non-Mercato consumer had nowhere to supply it. Four fields now: boot command, URL state file, credentials, auth contract. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
Suppressed comments (3)
skills/om-test-drive/SKILL.md:195
--no-seedis advertised above as still requiring an existing exercise record to be found and named, but this step tells the agent to skip the entire phase, including that lookup. On an empty instance the run can therefore produce the unqualified handover that the argument contract forbids. Make this branch perform the read-only lookup (and an explicitly empty-state handover if none exists) before skipping only the create steps.
Skip if `--no-seed`.
skills/om-test-drive/SKILL.md:186
- The Phase 4 PR path drops the
--repo <owner/name>scope that Phase 1 requires because fork/upstream checkouts can resolve the same PR number in the wrong repository. This can build a route map and seed plan for an unrelated PR; keep the resolved repository scope on this lookup too.
- Take the changed-file list from `gh pr diff <N> --name-only` (or a freshly-fetched base), then map each file to a route with the repo's convention. In Open Mercato a module's `backend/<path>/page.tsx` is `/backend/<path>`, and detail pages re-export each other — `sales/orders/[id]` renders the `sales/documents/[id]` component, so one changed component surfaces under several routes.
skills/om-test-drive/SKILL.md:120
- At startup on an unchanged branch, the default command can still attach to an environment created by a previous invocation. Since the state file has no source SHA or build digest, the skill cannot attribute that process to the current tree, which violates Hard rule 4 even without a branch switch. Make no-reuse the default unless this run created the environment from this tree.
You cannot verify your way out of this: the state file carries `startedAt`, a port and a database URL, but **no source SHA or build digest**, and a process that started after your checkout may still be serving output built before it. So don't try to reason from the timestamp — **after any branch switch or PR checkout, always pass `--no-reuse-env`** (`--fresh` does it for you). Reuse is only safe when you booted the instance yourself, in this run, from this tree.
…cato-pr-boot-skill # Conflicts: # .claude-plugin/plugin.json
…layout paths Six more findings, all correct. Two of them are mistakes I introduced while fixing the previous round, which is its own lesson. The worst was self-inflicted: after hitting the placeholder-JWT refusal during validation, I had written "treat any 'exited before readiness' as this class of problem". But that line is the harness's generic symptom, and a startup crash introduced by the change under test prints exactly the same thing — so the instruction told the agent to retry with fresh secrets and, in the case that matters most, hide the very bug it was sent to find. It now requires reading the actual stderr first and applying the override only when the refusal is shown. The subtlest: Phase 1 said to fetch the base and then diffed `<default>...HEAD`. `git fetch origin main` updates `origin/main`, not the local `main`, so the stale-base failure this phase exists to prevent survived its own fix. Now diffs `origin/<default>` and carries that ref into Phase 4. The rest: - The page probe printed its status and continued, unlike the API probe I had just fixed. A 307 (cookie didn't take) or 404 (route gone) would reach a handover whose click route Hard rule 1 claims was resolved. It exits now. - The `.env` recovery hardcoded `apps/mercato/.env`, which does not exist on the flat scaffold layout this skill started supporting last round. - The writing rule and the handover template both claimed every record was read back through the API, contradicting Phase 5's UI-form fallback and Hard rule 1's requirement to mark those unverified. Both are conditional on how the record was created now. Declined, verified rather than argued: "gh pr diff does not provide --name-only". It does — `gh pr diff 45 --repo fullstackhouse/skills --name-only` returns the file list and exits 0 on gh 2.90.0, and this session used it against another PR earlier. Also merges origin/main, which shipped docs-audit at 1.1.0; the manifest keeps both description additions and this branch's higher version. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (10)
skills/om-test-drive/SKILL.md:112
- The discovery ladder cannot distinguish Open Mercato from a non-Mercato repo that happens to define
test:integration:ephemeral:start(or expose amercatoscript). Rungs 1–2 win before the documentedThrowaway instanceprofile, so the agent can apply the Open Mercato bootstrap, auth, and route assumptions to the wrong stack instead of using the profile. Require an explicit Mercato marker for rungs 1–3 or give an explicit non-Mercato profile precedence.
**Resolve the boot command** — first hit wins, and the last rung is a real probe rather than an assumption:
1. Root `package.json` has `test:integration:ephemeral:start` → `yarn test:integration:ephemeral:start`. **Both** the monorepo and a current `create-mercato-app` scaffold define this, so the script's presence tells you nothing about which one you're in — see the layout check below before bootstrapping.
2. `yarn mercato test:ephemeral` (equivalently `yarn mercato test ephemeral`).
3. An older scaffold laid out under `apps/mercato` with no root script — probe first, then use it: `yarn --cwd apps/mercato exec mercato --help`, then `yarn --cwd apps/mercato exec mercato test:ephemeral`.
4. The repo's own documented boot (`## Skill profile` → **Throwaway instance**, or `CLAUDE.md` / `AGENTS.md`). If that resolves to a **long-lived shared dev stack** rather than a disposable one, say so and drop to `--no-seed` for the rest of the run — do not ask for permission to write into it (Hard rule 3).
skills/om-test-drive/SKILL.md:21
- The profile now records a non-Mercato auth contract, but the procedure still hard-codes Open Mercato's form endpoint,
.tokenresponse, bearer header, and cookie flow in Phases 3 and 5. Supplying a different documented contract therefore does not make the fallback usable: it boots and then deterministically fails. Either implement the profile-driven auth/seeding branches, or explicitly make a differing contract an unsupported boot-only fallback and stop before verification.
- **How to authenticate against it** — the login route, its method and payload shape, and whether it returns a bearer token or sets a session cookie. Phases 3 and 5 are written around Open Mercato's `POST /api/auth/login` → `{token}`; **a repo that authenticates differently will boot and then fail every later phase**, so if its profile doesn't document the auth contract, stop and ask rather than guessing at a login route.
skills/om-test-drive/SKILL.md:169
- This page probe still contains an unresolved
<path>before Phase 4 has identified any route. Following the block literally requests/backend/<path>and fails; choosing a changed route here recreates the ordering problem for UI-only changes. Probe a documented generic authenticated page in Phase 3, then probe each concrete click-route URL after Phase 4.
CODE=$(curl -s -b "$JAR" -o /dev/null -w '%{http_code}' "$BASE_URL/backend/<path>")
skills/om-test-drive/SKILL.md:189
- Phase 1 explicitly warns that an unscoped PR number can resolve against the wrong repository, but this later command drops
--repowhile building the route map. In a fork/upstream checkout with the same PR number, Phase 4 can inspect a different diff than the branch that was booted; retain the explicit repository scope here or carry Phase 1's file list forward.
- Take the changed-file list from `gh pr diff <N> --name-only` (or a freshly-fetched base), then map each file to a route with the repo's convention. In Open Mercato a module's `backend/<path>/page.tsx` is `/backend/<path>`, and detail pages re-export each other — `sales/orders/[id]` renders the `sales/documents/[id]` component, so one changed component surfaces under several routes.
skills/om-test-drive/SKILL.md:198
--no-seedis required above to find and name an existing exercise record, but this skips the entire phase containing the only existing-data check (step 3). The shared-stack path therefore reaches the handover without verifying a record, and the empty-state allowance here also conflicts with the hard named-record requirement in line 14. Keep the existing-record lookup/read-back when no-seed is selected, and reconcile the no-record outcome across these instructions (or stop without a test-drive handover).
Skip if `--no-seed`.
skills/om-test-drive/SKILL.md:111
- The actual discovery ladder is below the bootstrap and precondition instructions, even though line 55 says it must be resolved first. A top-to-bottom run reaches the Node/Docker checks and Open Mercato bootstrap before it has selected a command or determined whether those checks apply. Move this ladder above the preconditions, or make the phase explicitly branch at that point.
**Resolve the boot command** — first hit wins, and the last rung is a real probe rather than an assumption:
1. Root `package.json` has `test:integration:ephemeral:start` → `yarn test:integration:ephemeral:start`. **Both** the monorepo and a current `create-mercato-app` scaffold define this, so the script's presence tells you nothing about which one you're in — see the layout check below before bootstrapping.
2. `yarn mercato test:ephemeral` (equivalently `yarn mercato test ephemeral`).
3. An older scaffold laid out under `apps/mercato` with no root script — probe first, then use it: `yarn --cwd apps/mercato exec mercato --help`, then `yarn --cwd apps/mercato exec mercato test:ephemeral`.
skills/om-test-drive/SKILL.md:207
- Phase 5 creates the concrete record URL only here, after the earlier page probe has run, but never instructs a final authenticated fetch of that URL. A dynamic detail URL can therefore appear in the click route without satisfying Hard rule 1. Add a post-seed probe for each concrete URL, including dynamic IDs, and record its status before writing the handover.
For each seeded record, note its **human-visible identifier** (name, number, title) and the **backend URL where the user will find it**. That pairing is what makes the handover clickable rather than a description.
skills/om-test-drive/SKILL.md:120
- The freshness requirement only fires after a branch switch or PR checkout, but no-argument runs explicitly include staged, unstaged, and untracked source changes. An existing environment has no source SHA or build digest, so it can serve the pre-change build even when the branch never changed. Require
--no-reuse-envwhenever the target includes worktree changes, or whenever this invocation did not create the environment.
You cannot verify your way out of this: the state file carries `startedAt`, a port and a database URL, but **no source SHA or build digest**, and a process that started after your checkout may still be serving output built before it. So don't try to reason from the timestamp — **after any branch switch or PR checkout, always pass `--no-reuse-env`** (`--fresh` does it for you). Reuse is only safe when you booted the instance yourself, in this run, from this tree.
skills/om-test-drive/SKILL.md:218
- The template always says the instance was "built from ", but the same handover explicitly supports attaching to an already-running environment. The state file has no source SHA or build digest, so current HEAD cannot establish what that process serves even on a clean tree. Make provenance conditional for attached runs and say "unverified" rather than attributing the build.
- URL: <baseUrl>/<app path> — port <n>, built from <sha> "<subject>"
<on a dirty tree: + uncommitted work — N modified, M untracked; HEAD alone does not identify what's running>
skills/om-test-drive/SKILL.md:147
- Following these snippets literally makes three POSTs to the rate-limited login endpoint: the initial probe, token capture, and cookie-jar login. That consumes the attempt budget before Phase 5 and can turn a valid drive into
429, especially on retries. Reuse one response, or make the first request explicitly illustrative rather than executable.
TOKEN=$(curl -s -X POST "$BASE_URL/api/auth/login" \
-H 'content-type: application/x-www-form-urlencoded' \
--data-urlencode '[email protected]' --data-urlencode 'password=secret' \
| node -pe 'JSON.parse(require("fs").readFileSync(0,"utf8")).token || ""')
…empty state The intro declared any handover without a named record a failure of the skill, while --no-seed — rewritten last round to be read-only rather than verification-free — explicitly permits a route that ends at an empty state. An agent could not satisfy both. The rule now holds wherever the environment could have carried such a record, with an honest empty state as the stated exception. Also lets a documented `Throwaway instance` profile short-circuit the discovery ladder. The rungs key on script names a non-Mercato repo can define too, so sniffing before reading the profile risks applying Open Mercato's bootstrap, auth and route assumptions to a stack that shares nothing with it but a script. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The problem
Reviewing an Open Mercato PR by hand is mostly setup tax. Before you can form an opinion you
have to boot a throwaway app from the branch, wait out a full build, find out whether login
even works, work out which of the dozen-odd backend screens the diff actually touches, and
then discover the feature is invisible because nothing in the seeded demo data exercises it.
No existing skill closes that gap.
explaindeliberately stops at the diff — its own textsays it is "not a verification run (a verify skill runs the app)". This is that missing
verify step, aimed at a person rather than a pipeline.
The fix
/fsh:om-test-driveboots the change on a disposable instance, proves auth with a real HTTPround-trip, reads the diff well enough to say what it does in product terms, seeds the data
the change needs through the app's own API, and hands back a click-by-click route with a live
URL and working credentials. No browser: login is curl'd, the walkthrough is written.
The guardrails are the point. Never write to the database directly — a row inserted behind the
app skips validation, events and indexing, so the drive would demonstrate something that
cannot happen in production. Never seed against a database that isn't the throwaway one. And
don't demo a build you can't attribute: the ephemeral command silently attaches to an
already-running instance based on file mtimes and a TTL, which after a branch switch hands you
a running build of different code while everything looks fine.
Validated against a real PR
This did not ship on reasoning. It was run end-to-end against a live Open Mercato PR, which
took six boot attempts to get an instance up and produced seven corrections — the first
draft would have died on attempt one with no guidance for recovery.
The root error was a single wrong assumption: the ephemeral command is not a from-zero
installer. It runs
initializebefore its own codegen and build steps, so it only works onan already-warm repo. That cascaded into four failures, each reporting a symptom rather than a
cause — most deceptively, every migration applying successfully and then a bootstrap death
on a missing codegen artifact, so a codegen fault wore a database costume. Phase 2 now carries
the ordering (which the repo's own
buildscript already encodes) and an error-to-missing-rungtable.
Two findings worth more than their mechanics:
developin the testcheckout was five weeks old, so
git diff develop...HEADreported 4,141 files for a 15-filePR. Every downstream conclusion would have inherited it. The skill now takes its surface from
gh pr diffor a freshly-fetched base.200 and implied that proved the change worked. It does not:
/backendanswers 307 anonymously(a correct login redirect the rule would have called broken), and with a login cookie the
detail page returns 200 with 1.3 MB of shell HTML containing none of the seeded values,
because the backend renders client-side. The skill now states that ceiling in the hard rule,
in the phase, and at the top of every handover it writes.
Details
Open-Mercato-first by name and substance, with a discovery ladder so a non-Mercato repo
degrades to its own documented boot via a new Throwaway instance profile knob. It is
deliberately not listed among the repo-agnostic skills — the name would contradict the
roll-call.
Scope boundary: driving a browser, screenshots and pass/fail QA evidence stay out. That is a
different job and duplicating it here would be worse at it.
Verification
Ran against
fullstackhouse/open-mercato#93on this machine: instance up on port 60770 builtfrom
c361d3ca, login round-trip 200 with a token that authorizes a real read, and an orderseeded via the PR's own integration-spec recipe and read back carrying
phone,taxIdandtaxIdType. Every route in the produced handover was fetched authenticated and returned 200.Repo-local gates: version bumped
1.0.0→1.2.2(CI'sversion-check.ymlrequires it), all20 skills pass a name/dir + frontmatter structural check, README links resolve.
Two rounds of review feedback are folded in — 18 findings, 16 fixed. The two declined are
answered with measurements rather than argument: the frontmatter's unquoted
Args:is rejectedby strict YAML but seven skills already on
mainshare it and load fine (repo-wide follow-up,not this PR), and
Securecookies are sent by curl over loopback (200 with the jar, 307without), so the page probe does prove page auth.
The review also caught something I had no way to reason to: the current
create-mercato-apptemplate defines the same root
test:integration:ephemeral:startas the monorepo while havingno
build:packagesand noapps/mercato, so the discovery ladder mislabelled a scaffold andprescribed a warm-up step that doesn't exist there. Bootstrap now branches on layout.
Follow-ups
apps/mercato/.envshipsJWT_SECRET=change-me-dev-secretfrom.env.example, and theephemeral env runs in production mode where the app's own guard refuses that placeholder —
so
mercato test:ephemeralcannot boot a stock tree without an override. That looks like abug worth filing upstream rather than a quirk to document forever.