Conversation
…ent gate, join the roster
…t provenance, anatomy-override scaffolding
…OG for om-mockup-prototype
|
🤖
|
|
🤖 🎯 What this PR doesExecutes Phase 2 — Add the skill here (spec steps 8–14) of ⛔ Merge gateImport source: commit 📝 What landed, step by step
🧪 Validation gate
🔍 ReviewThe in-run review pass covered correctness (the scaffolding's three branches exercised by real runs; generated init code imports and executes), scope (every changed line traces to spec steps 8–14; no other skill's content touched), security (no ⏭️ After upstream merges
|
|
🤖 🎯 What this PR doesExecutes Phase 2 — Add the skill here (spec steps 8–14) of 📝 Changes
🧪 Verification
⛔ Merge gateBlocked until open-mercato/open-mercato#5832 merges; then the imported files must be re-verified against the merged commit (recorded import source: 🔍 ReviewIn-run review pass: correctness (equivalence of imported scripts with the verified Phase 1 versions; scaffolding no-op when the override path is the skill's own file, which is exactly the upstream Phase 3 layout), security (no shell interpolation, no network, localhost-only server, untrusted-content boundary stated), scope (every change traces to spec steps 8–14), contracts (no other skill's files touched beyond the roster line; no shared format changed). |
|
🤖 |
pkarw
left a comment
There was a problem hiding this comment.
Code Review: feat(skills): add om-mockup-prototype — interactive prototypes with anchored review comments
Summary
This is a useful additive skill, and the imported engine/scripts are syntactically valid, lint-clean, and not duplicated on main. The review cannot approve it yet: its declared upstream rollout prerequisite remains unmet, and the current implementation omits required test coverage and two concrete contract behaviors from the accepted spec.
Verdict
request changes — one external rollout blocker and three actionable major findings must be resolved before this can merge.
Validation Gate
| Command | Status | Notes |
|---|---|---|
bash scripts/lint.sh |
PASS | Printed Lint OK. on PR head df9f488. |
The PR checks lint and license/cla are also successful. Branch protection is not readable, so all reported checks were treated as required.
Findings
Blocker
- The accepted rollout prerequisite has not landed.
.ai/runs/2026-09-02-import-om-mockup-prototype.md:3identifies open-mercato/open-mercato#5832 as the source and explicitly makes its merge plus a post-merge re-verification a prerequisite. That upstream PR is still open withREVIEW_REQUIRED, so the source cannot yet be verified against a merged commit. Keep this PR unmergeable; once #5832 lands, compare every imported file with its merged head and record the result (re-importing any reviewed changes).
Major
-
Executable behavior has no committed automated tests. The diff adds
skills/om-mockup-prototype/scripts/init-mockup.mjs,skills/om-mockup-prototype/scripts/sync-tokens.mjs, and a 736-line browser engine, but no*.test.*,*.spec.*, or equivalent test file. The accepted spec requires the initialization/token and full comment-engine checklists. Import or adapt the upstream test suite and cover argument/path rejection, atomic cleanup, token source/drift, symlink containment, storage isolation, reply focus, pins, re-anchoring, tombstones, and export. -
The implementation drops the accepted configurable path/token-source contract and hardcodes repository paths.
skills/om-mockup-prototype/SKILL.md:8-35,scripts/init-mockup.mjs:21, andscripts/sync-tokens.mjs:11,31-37hardcode.ai/prototypesand.ai/ds/ds-tokens.json;references/agentic-setup.md:8-11says config is irrelevant. The accepted spec defines optionalpaths.prototypesanddesignTokens, while this repository requires working paths to come from.ai/agentic.config.json. Load and validate those fields with backward-compatible defaults, pass them consistently to both scripts and reporting, and add both optional fields/defaults to the setup schema and documentation. -
A scaffold failure leaves a CLI run failed but permanently occupies the prototype slug.
skills/om-mockup-prototype/scripts/init-mockup.mjs:145-149renames the staged prototype into place beforeensureAnatomyOverride()runs. If the override scaffold then fails (for example, an unwritable or invalid override path), the command exits 2 but leaves the prototype directory behind; the next run refuses to overwrite it. Coordinate or roll back the two initialization outputs so a failed command is safely retryable, and add a regression test for this failure path.
Breaking Changes
- No exported/public symbol removed or renamed without a deprecation path.
- No function signature changed in a breaking way.
- No required type or response field removed or narrowed.
- No HTTP route, event/message name, CLI flag, database schema, or existing config key changed.
- The new skill name and roster entry are additive.
- Merge rollout is not yet safe because the imported upstream source has not reached its required merged state.
Test Coverage
Not covered in this PR. Direct verification described in comments is useful evidence but does not replace committed regression tests for the new executable scripts and browser interaction engine. The upstream source already contains a substantial om-mockup-prototype.test.mjs; adapt it to this collection and ensure the repository can run it without relying on an undeclared package.
|
🤖
|
|
Thanks @matgren — review found actionable items, so I am handing this PR back to you for the next pass. I am continuing the autonomous fix-forward pass now; when the updates are pushed, re-request review from the latest head. |
# Conflicts: # DECISIONS.md # skills/om-setup-agent-pipeline/SKILL.md
pkarw
left a comment
There was a problem hiding this comment.
Re-review: feat(skills): add om-mockup-prototype — interactive prototypes with anchored review comments
Summary
Autofix iteration 1 resolved all three actionable major findings, and the branch was then merged with the latest main to clear the newly surfaced conflicts. The implementation is now lint-clean, CI-green, regression-tested, and browser-smoke-verified; only the explicitly accepted upstream rollout prerequisite remains unresolved.
Verdict
request changes — the code findings are resolved, but the upstream source PR must merge and be re-verified before this PR can safely enter the merge queue.
Validation Gate
| Command | Status | Notes |
|---|---|---|
bash scripts/lint.sh |
PASS | Printed Lint OK. on final head 456a652. |
Additional verification also passed:
node --test scripts/test-mockup-prototype.mjs— 9/9 tests passed.node scripts/test-browser-providers.mjs— provider contract passed.node scripts/test-tracker-providers.mjs— 42 operations and two split providers passed.node scripts/test-classify-runs.mjs— classifier contract passed.node scripts/test-close-keywords.mjs— keyword contract passed.- GitHub checks
lintandlicense/claare successful. - Real-browser smoke verification passed for light/dark switching, anchored comment creation, pin rendering, and persistence across reload.
Findings
Blocker
- The rollout prerequisite is still open.
.ai/runs/2026-09-02-import-om-mockup-prototype.md:3requires open-mercato/open-mercato#5832 to merge and the imported files to be compared with its merged commit. That upstream PR remains open withREVIEW_REQUIRED. Once it merges, re-diff the imported assets/scripts against the merged head, carry forward any upstream review changes, rerun the gates, and re-request review here.
All three previous major findings are resolved by f9ca0a9: the optional designTokens and paths.prototypes contracts are implemented with contained-path validation and documented setup defaults; failed anatomy scaffolding now rolls back the new prototype so retry is safe; and the new dependency-free regression suite is wired into CI. Merge commit 456a652 resolves the base conflicts and preserves both branches' setup/decision changes.
Breaking Changes
- No exported/public symbol removed or renamed without a deprecation path.
- No function signature changed in a breaking way.
- No required type or response field removed or narrowed.
- No HTTP route, event/message name, CLI flag, database schema, or existing config key changed.
- The new config fields are optional and retain backward-compatible defaults.
- The new skill name, roster entry, test command, and CI step are additive.
- The rollout prerequisite is not complete until the upstream source is merged and re-verified.
Test Coverage
Covered for initialization, argument/path containment, token-source precedence, CSS-input safety, atomic cleanup, override scaffolding, symlink rejection, variable resolution, drift detection, and repository-root fallback. The unchanged browser engine matches upstream source commit 9ea8320 and received an additional local browser smoke pass; its full interaction checklist remains part of the upstream prerequisite verification after #5832 merges.
|
🤖 |
|
🤖 Claiming this PR — starting |
…-prototype # Conflicts: # package.json # skills/om-setup-agent-pipeline/references/skill-coverage.md
The base branch grew `validation.commands` from one entry to five while this branch added the `node --test scripts/test-mockup-prototype.mjs` step to the lint workflow. The merge kept base's five-entry list, so the repository config no longer mirrored the workflow and `test-browser-providers.mjs` failed its "validation.commands must mirror the lint workflow in order" assertion. Take the union both sides intended. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🤖 |
pat-lewczuk
left a comment
There was a problem hiding this comment.
Code Review: feat(skills): add om-mockup-prototype — interactive prototypes with anchored review comments
Verdict
request changes — the code is ready; the release is not. Every actionable code finding from the earlier pass is fixed, the branch is now merged with the latest main, and the full validation gate is green on head 730a1d6. The single remaining blocker is the accepted rollout prerequisite: the upstream import source is still an open PR, so the imported files cannot yet be re-verified against a merged commit.
What this changes
Adds om-mockup-prototype to the collection: a skill that generates a self-contained, click-through HTML prototype from a requirements doc, with an anchored-comment engine reviewers use in the browser and export back into the repo as a committed comments.js. Two Node scripts back it — init-mockup.mjs (scaffold a prototype directory) and sync-tokens.mjs (generate tokens.css from a design-token snapshot, with drift checking). Nothing existing changes behavior; the roster, docs, and CI step are additive, and the two new optional config keys (designTokens, paths.prototypes) default to today's behavior.
Validation Gate
| Command | Status | Notes |
|---|---|---|
bash scripts/lint.sh |
PASS | Lint OK. on head 730a1d6. |
node scripts/test-browser-providers.mjs |
PASS | Passed after the fix below; failed on the raw merge result — see Blocker-adjacent note. |
node scripts/test-tracker-providers.mjs |
PASS | 42 operations, 2 split providers. |
node scripts/test-classify-runs.mjs |
NOT RUN locally | jq is unavailable in this reviewer's sandbox. Verified green in CI instead: the lint job on head 730a1d6 runs this exact command and the job succeeded. |
node scripts/test-close-keywords.mjs |
PASS | Contract OK. |
node --test scripts/test-mockup-prototype.mjs |
PASS | 9/9. (Requires TMPDIR outside the repository — see nit 4.) |
Required checks on head 730a1d6: lint success (run). main is unprotected, so all reported checks are treated as required.
Findings
Blocker
- The rollout prerequisite is still unresolved (inherited from @pkarw's review, 2026-09-04) —
.ai/runs/2026-09-02-import-om-mockup-prototype.md:3and this PR's own body gate the merge on upstream open-mercato/open-mercato#5832. Re-checked at review time: that PR is stillOPEN,mergedAt: null. Until it merges, the imported files underskills/om-mockup-prototype/cannot be re-diffed against a merged commit, so any upstream review change would land here as drift. This is release coordination, not a code defect — nothing in this diff needs to change for it. Action: when #5832 merges, re-diff the imported assets and scripts against its merged head, carry forward any upstream changes, rerun the gate, and re-request review.
Not filed as a finding, but recorded: the base merge in 265f8e2 surfaced a real integration break that this pass fixed in 730a1d6. main grew validation.commands from one entry to five while this branch added node --test scripts/test-mockup-prototype.mjs to the lint workflow; the merge kept base's five-entry list, so test-browser-providers.mjs:124 ("repository validation.commands must mirror the lint workflow in order") failed. Resolved by taking the union both sides intended. Two textual conflicts (package.json scripts, the skill-coverage.md ROSTER) were likewise resolved as unions of om-discover and om-mockup-prototype.
Minor
-
renderTemplatecorrupts output when the requirements path contains$—skills/om-mockup-prototype/scripts/init-mockup.mjs:115.String.prototype.replaceAllinterprets$&,$`,$'and$1in the replacement string, andescapeHtmldoes not escape$. Verified:--requirements 'docs/a$&b.md'rendersdocs/a{{REQUIREMENTS}}b.md;x$`ysplices the preceding template text into the file. Fix: pass a replacer function —content.replaceAll(\{{${placeholder}}}`, () => value)`. -
ensureAnatomyOverrideignores the caller's repo root —skills/om-mockup-prototype/scripts/init-mockup.mjs:160.initializePrototypeWithAnatomyacceptsoptionsand threadsoptions.prototypesRootintoinitializePrototype, but calls(options.ensureAnatomyOverride || ensureAnatomyOverride)()with no arguments, sooptions.repoRootnever reaches it. A caller that scopes the prototype to a sandbox still gets.ai/skills/om-mockup-prototype/references/screen-patterns.mdwritten to the realREPO_ROOT. The test suite only exercises this through a stub, so the gap is not covered. Fix: forward{ repoRoot: options.repoRoot }. -
exportMarkdowninterpolates a screen id straight into a CSS selector —skills/om-mockup-prototype/references/assets/prototype.js:588,'#' + screenId + ' .screen-meta h2'. Ascreenvalue that is not a bare CSS identifier (leading digit, space, quote) makesquerySelectorthrowSyntaxErrorand aborts the whole Markdown export.screencomes from operation payloads incomments.js, whichnormalizeOperationsdoes not constrain. Fix:document.getElementById(screenId)?.querySelector('.screen-meta h2').
Nit
-
HTML escaping applied in a JavaScript string context —
escapeHtml(slug)fills{{SLUG}}inside a single-quoted JS literal inreferences/assets/comments.js:10, and{{MODULE}}/{{REQUIREMENTS}}are HTML-escaped intoreferences/assets/README.md, a Markdown file where&would render as&. Harmless today only becauseSLUG_PATTERNalready forbids quotes — the escaping is the wrong function for two of the three sinks, not a live defect. Worth escaping per target format if these templates grow. -
test-mockup-prototype.mjsdepends onTMPDIRbeing outside the repository — the repository-root fallback case assertsresolveRepoRoot()returns the temp directory, which only holds whenmkdtemplands outside any git checkout. On a machine whoseTMPDIRsits inside the repo the test fails with a confusing diff. Consider forcingos.tmpdir()explicitly.
Security
No new attack surface. The browser engine has no innerHTML, eval, new Function, document.write, fetch, or XMLHttpRequest — every reviewer-supplied string reaches the DOM through textContent or setAttribute, and export goes through a local Blob download with no network egress. Both Node scripts validate configured paths against the repository root, reject absolute paths, traversal, and symlink escapes (sync-tokens.mjs:50-72, init-mockup.mjs:99-110), and tokenDeclarations rejects token names that are not valid custom-property identifiers and values containing ;, {, }, or url( before they reach generated CSS.
Breaking Changes
- No exported symbol, HTTP route, event name, CLI flag, schema, or existing config key removed, renamed, or narrowed.
-
designTokensandpaths.prototypesare optional and fall back to current behavior. - The new skill, roster entry, test command, and CI step are purely additive.
Test Coverage
Adequate for the two scripts: 9 cases covering argument rejection, HTML substitution, configured-path containment and defaults, token-source precedence, atomic/retry-safe initialization, override scaffolding idempotence, symlink and ambiguous-target rejection, bundled-variable auditing, drift detection, and the repo-root fallback. The browser engine is covered by manual smoke verification rather than automated tests, which matches the earlier pass's evidence; findings 2–4 are each currently uncovered and would be worth a case if they are fixed here rather than upstream.
|
🤖
|
|
🤖 Not merge-ready, and no code change is left to make. The branch is now merged with the latest What this run changed ( 🧪 CI Four non-blocking findings (review) were deliberately not patched here and are tracked in #113 instead: they sit in files imported from #5832, and fixing them locally would create exactly the drift the re-diff gate exists to catch. Next: when #5832 merges, re-diff the imported assets and scripts against its merged head, carry forward anything upstream changed, rerun the gate, and re-request review. Then |
|
@matgren — nothing to change in the code: the review found no blocking defect, and this run merged 🤖 |
|
🤖 @matgren, do you agree to replace this PR's requirement that open-mercato/open-mercato#5832 must merge first with a verified import from a pinned source commit? The original condition supported the Phase 1 → Phase 2 extraction in #91. This PR now carries the detailed-design workflow agreed in #107 under Proposed replacement gate for this PR:
The snapshot-validation defect found during the upstream review is already addressed here. A separate, tested two-file patch is available for your review on #5832, so that PR can be handled on its own merits. Until we agree on the revised rollout condition, the |
|
Agreed: replace the "#5832 merges first" prerequisite with the verified import from the pinned commit, exactly as you list it (source commit, file mapping, accounted differences, retained evidence, fresh review). The original ordering served the Phase 1 → Phase 2 extraction; after the rename to Two things before the label moves: record the revised condition in the #91 rollout section and in this PR's description, so the decision lives with the spec and not in a comment thread; and @pkarw, since the gate came from your 4 Sept review, please confirm and take the re-review, then |
|
🤖 The condition accepted by @matgren is now recorded in the #91 specification's Rollout section, #91's rollout description, and this PR's merge gate. Documentation commit: The verification record maps all 13 files from upstream @pkarw, please confirm the revised prerequisite and re-review the current head, as requested in @matgren's decision. |
🎯 Goal
Add
om-ux-designfor detailed screens and connected prototypes from a specification or selected backlog scope, using the repository's real design system and existing components. This is the renamed and improved prototype capability in this PR. The naming split agreed in #107 reservesom-mockup-prototypefor neutral discovery experiments.Subject issue: #104. Original rollout specification: #91. Detailed update specification:
.ai/specs/2026-09-14-ux-design-skill.md.📝 Changes
🧪 Verification
Implementation verification at
7e3a766: all six configured validation commands passed, including 38 helper and comment-engine regression tests. The earlier independent implementation review findings were fixed and cleared on re-review. Three local browser executions passed: an existing component runtime, a no-DS portable design, and a refresh with existing feedback. Full flows were inspected at 360px and 1280px. Feedback export/reload, resolve/reopen, deletion preservation and isolation between prototype IDs passed. Screenshots and disposable fixtures remain local.Native date entry exposed an automation limitation: CLI
fillleft the field empty. The component-runtime trial seeded the date through DOM events and verified real arrow-key changes; the portable trial used individual date-segment key presses.⛔ Merge gate
@matgren accepted the revised rollout condition: verify the import from pinned upstream source
9ea83205be7447867c042bbcfd3caaa9b4cadfb5, with complete mapping, accounted differences, retained evidence and fresh review. This is recorded in the #91 specification's Rollout section and comparison/evidence record.skills/om-ux-design/; four portable CSS files are byte-identical and all 124 token records retain their values. Workflow, template, helper and comment-engine changes are accounted for in the record.7e3a766; the three browser executions and upstream baseline are retained historical evidence, not newly repeated browser runs.blockedis removed, as requested by @matgren. Green tests and this amendment do not replace that review or authorize merging.Once the revised condition is confirmed and the evidence accepted, open-mercato/open-mercato#5832 merging is no longer a prerequisite. This collection is the source going forward; the upstream PR and its consumption follow-up can proceed independently.
Tracking plan: .ai/runs/2026-09-02-import-om-mockup-prototype.md
Status: complete