feat(perps): distribute generated review checklists - #163
abretonc7s wants to merge 5 commits into
Conversation
Pre-review pass (17 Sept)A full read of the end state of this branch before asking for review. Checked, nothing to change:
Fixed in Not changed on purpose: nothing else was found that is a factual or text error; structural or test suggestions are out of scope for this PR. PR description refreshed to describe the small body + overlays + per-client templates. |
|
|
aganglada
left a comment
aganglada
left a comment
There was a problem hiding this comment.
Review
Requesting changes for one installer bug. The per-client split (shared skill.md, overlays, on-demand criteria, byte-identical templates) is the right shape, and the three comments from the earlier review look addressed. CI is green. GitHub still shows CHANGES_REQUESTED from the review on 47362fe; this is a re-review of 91a4cd3.
Must fix
User-scope Codex default_prompt is not interpolated. write_user_codex uses a different escape than write_agents. In bash double quotes, \${out_name} writes the literal ${out_name}; project Codex skills get Use $mms-workflow for this task. Inline comment on tools/install. test/explicit-invocation.test.mjs only asserts allow_implicit_invocation: false on the user yaml, so this would not fail CI.
Should fix
Every client still receives every other client’s criteria and templates. copy_bundle_dirs copies all of references/. The install test documents that the files all ship and only the checklist names rows. A Mobile install therefore includes static-perps.extension.md, static-perps.core.md, and references/criteria/{extension,core}/. That undercuts the “1,100–1,450 words, not 6,854” goal: the SKILL body is small, but a model that glob-reads references/ still sees the other clients. Filter the bundle per --repo, or keep other-client templates out of the installed skill.
Local reviewers still get farm worker protocol. Verdict rows tell a Cursor/Claude user to write artifacts/review.md, line-comments.json, satisfy worker-terminal-contract.json, and mark complete --mark-last. The README also says a control plane worker follows a complete per-client template, which contradicts the 17 Sept pre-review note that control-plane language was gone from the README. Either keep that only in the farm templates, or say clearly that local /mms-perps-review-pr ignores those rows.
Suggestions
- Implicit routing is gone on purpose. Description collapsed from “review my perps PR / perps review / QA static step” to “when explicitly invoked by name.” After Mobile/Extension bump
@metamask/skills, agents will not auto-attach this skill. That matchesdisable-model-invocation, but consumers need the invocation name in AGENTS.md / farm wiring before the bump. summary()uses the first body line. Families whose first line is a bullet (e.g. Charts/CTAs) get that bullet as the checklist one-liner, not the thesis paragraph. Prefer the first non-list paragraph when one exists.- Retired-name prune is project-only (
CLAUDE_DIR/CURSOR_DIR/AGENTS_DIR). User-home copies ofmms-recipe-fixbugwould remain. Fine if those names were never user-scoped; worth a one-line comment or extending the loop if they were. - CHANGELOG Unreleased bullet has no
(#163)link, unlike neighboring entries. - Tests are strong on merge equality,
--checkdrift, analyzer--client, and “don’t delete hand-authored skills.” Add thedefault_promptassertion; optional: assert a Mobile install does not copyreferences/templates/review-pr/static-perps.extension.md.
What looks good
- Per-client templates asserted equal to
skill.md+ overlay astools/installmerges them; client-agnosticstatic-perps.mdasserted absent. overlay_body()so frontmatter-less overlays are not emptied bybody_after_frontmatter.- User
agents/openai.yamlwritten only for explicit-only skills, removed only whenMANAGED_YAML_BANNERis present. - Cursor explicit-only as nested
.mdcwithoutdescription/globs, withRULE.md↔.mdcswap. - Generator refuses dirty library sources, stamps revision + sha256,
--checkcatches stray files,--analyzer-outrequires--clientand inlines rules. - Retired-name prune before install so rename leftovers cannot sit beside the new skill.
| echo 'interface:' | ||
| printf ' display_name: '; yaml_quoted "$name"; echo | ||
| yaml_field ' short_description' "$description" | ||
| printf ' default_prompt: '; yaml_quoted "Use \${out_name} for this task."; echo |
There was a problem hiding this comment.
This does not match write_agents (Use \$${out_name} for this task.).
In bash double quotes, \${out_name} is the literal ${out_name}, so user-scope Codex yaml gets Use ${out_name} for this task. instead of Use $mms-personal for this task.
Use the same \$${out_name} interpolation, and assert the expanded prompt in test/explicit-invocation.test.mjs (that test currently only checks allow_implicit_invocation: false).
91a4cd3 to
410b87c
Compare
abretonc7s
changed the title
410b87c to
85f46b9
Compare
Add explicit-only workflow metadata to the skill installer and emit each supported runner's native invocation controls. Preserve repository overlays and custom files during install and reinstall. - Contains installer/schema changes, installation tests and documentation. - Perps review content is isolated in #163, which is stacked on this branch. - Validation: explicit-invocation tests passed on this branch; all 75 repository tests passed on the stacked Perps branch. - The published installer is still 0.3.1. Adoption requires a new installer release and consumer dependency updates.
Co-authored-by: Cursor <cursoragent@cursor.com>
6b94142
This branch has not been deployed
4 participants
Distribute the generated Perps static review checklist for Mobile, Extension and Core.
--analyzer-out <path> --client <client>optionally exports inlined context for an existing PR risk analyzer. It does not enable an analyzer integration.Farmslot validation ran the Core template with Astra medium, registered all 30 child checklist rows, saved three actionable findings plus line comments, and posted the selected review to MetaMask/core#10294 after the human gate.