feat(feature-development): add android-dev ("Dan") persona + external-skill registry guard - #51
Merged
Merged
Conversation
Matches the existing docs/superpowers/ rule — local-only plan/spec/ledger scaffolding that is not distributed with the repo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s guard The `environment-setup-xcuitest` registry entry pointed at a subdir that 404s upstream (renamed to `skills/setup-xcuitest`). Renaming only the subdir wasn't enough: bin/init.mjs derives the installed skill's directory name from the fetched SKILL.md's `name:` frontmatter, not the registry id, so the id had to be renamed to match upstream's declared `setup-xcuitest` too, or the skill would silently install under the wrong directory and vanish from any bundle overlay referencing the old id. Updated every consumer (bundle.json, README.md, onboarding doc) and regenerated the generated marketplace manifests. Also add an opt-in `--check-externals` mode to bin/validate-bundles.mjs (wired to `npm run validate:externals` and a new CI job) that fetches every skills.json `repo:` entry's upstream SKILL.md and fails if it 404s or its `name:` doesn't match the registry id — the exact bug class this fixes. The default validator path stays network-free. Found but did not fix: `playwright-best-practices` also 404s (missing subdir) under the new check — flagged separately for adjudication. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… entry The --check-externals guard added earlier in this task surfaced a second, pre-existing broken repo: entry: playwright-best-practices had no `subdir`, so it fetched SKILL.md from the repo root and 404'd — the actual file lives at playwright-best-practices/SKILL.md. Unlike the xcuitest bug, the `id` already matched upstream's `name:` frontmatter, so only the subdir needed adding. node bin/validate-bundles.mjs --check-externals now passes 21/21. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…x minors Fix round 2 on the --check-externals guard added earlier in this task, per review: - Export the guard's pure/testable core (buildSkillMdUrl, checkExternalEntry, checkExternals) from bin/validate-bundles.mjs with an injectable fetchImpl, gated behind a new isMainModule() check (same pattern bin/init.mjs already uses) so importing the module for its exports no longer triggers a live bundle validation or network check as a side effect. Add bin/validate-externals.test.mjs (13 tests): 404, name/id mismatch, absent frontmatter, subdir with a trailing slash, missing ref defaulting to main, plus the new retry/timeout/severity behavior below. - Split check results into FAIL (404, name mismatch, malformed/absent frontmatter — registry defects this repo owns, blocking) vs WARN (network transport errors, 429, 5xx — upstream/infra conditions, non-fatal). Each WARN-class outcome is retried once with a short backoff before being reported; every request carries an AbortSignal.timeout. No continue-on-error anywhere — the CI job from 1d needs no change since a WARN-only run already exits 0. - Minors: await/.catch() the top-level check-externals call instead of letting it run unhandled; move `await res.text()` inside its own try so a post-200 body-read failure is captured as a WARN for that one entry instead of aborting the whole run; extract the `name:` frontmatter regex (previously duplicated between this file and bin/init.mjs:977) into a new shared bin/lib/skill-md.mjs, imported by both; README.md's playwright-best-practices row now shows its subdir, matching the playwright-cli row's format. Test count: 73 -> 86 (13 new). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ion layer Fix round 3 on the --check-externals guard, per re-review: - The summary line no longer overclaims when any entry WARNs: a WARN-only run previously printed "All N external skill(s) valid (N WARN ...)" when in fact zero were verified this run. Now reports "<verified> of <total> verified" whenever warnCount > 0, and only says "All N valid" when every entry actually passed. - Reclassify HTTP 403 from FAIL to WARN in httpStatusSeverity, with a comment recording why: raw.githubusercontent.com returns 404, not 403, for a private/nonexistent repo, so a 403 there is near-always abuse-detection/rate-limiting (the transient class), not a real registry defect. - Extract summarizeExternalResults(results) — a pure, exported aggregation of severity counts and the exit-code decision — out of the previously untested runCheckExternals. Add 7 tests: the five severity-mix x exit-code cases (all-PASS, WARN-only, PASS+WARN, FAIL-only, FAIL+WARN), an end-to-end checkExternals -> summarizeExternalResults composition test, and a dedicated 403-is-WARN test mirroring the existing 429 case. Test count: 86 -> 93. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds compose-state-hoisting, kotlin-flow-state-event-modeling, kotlin-coroutines-structured-concurrency, and compose-ui-testing-patterns (chrisbanes/skills), testing-setup and android-cli (android/skills), and setup-uiautomator2 (appium/skills) to skills.json for the upcoming android-dev persona. setup-espresso is deliberately excluded as an alternative Appium driver to setup-uiautomator2. Updates CLAUDE.md's external-skill count from 21 to 28. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… skills npm run gen:marketplaces after registering the Android skills in the prior commit. Keeps the branch bisectable: validate:marketplaces would otherwise fail on every commit between here and Task 6, which is when the android-dev agent itself lands. Task 6 will re-run the (idempotent) generator once that agent exists and add its .claude-plugin/marketplace.json entry by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the android-dev agent to the feature-development bundle, modelled section-for-section on ios-dev. Native Android only — Kotlin, Compose, Gradle/AGP, Room/DataStore — explicitly not React Native/Flutter and not server-side Spring/JVM. The no-device rule is stated as policy rather than as a hazard calculation: instrumented tests and device execution belong to CI and to QA, and local device state is not an input to that decision. An earlier draft argued the hazard applies *only if* a device is attached, which lets an agent under delivery pressure complete the syllogism (empty device list -> hazard absent -> running it is free). ios-dev survives because its rationale is unconditional; this one now is too. The forbidden list covers the Gradle tasks that wrap adb install (installDebug, installDebugAndroidTest, uninstallAll), not just adb itself — that was the highest-frequency accidental-device-write path and forbidding adb alone left it open. Declining device work hands off to a named party (qa-engineer, CI, or the user) with the substitute evidence attached, so a refusal is a routing decision rather than a dead end. Version facts live in one dated baseline block so refreshing the persona is a single-section edit, and every rule defers to AGENTS.md and the project's version catalog, which are the actual authority. Registering the agent in bundle.json, briefings and the docs is deliberately left to the follow-up tasks; this commit only adds the agent directory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gen-marketplaces.mjs discovers agents by directory scan, so adding bundles/feature-development/agents/android-dev/ in a91065e made .cursor-plugin/marketplace.json stale on its own — there is no way to author the agent and leave the manifest current. Regenerating here rather than deferring to the marketplace task keeps npm run validate green on every commit of this branch, so the range stays bisectable. Generator output only, no hand edits. Only the cursor manifest moves (74 -> 75 entries); it is the one generated manifest that enumerates agents, while the codex and github manifests carry skills only. The hand-curated .claude-plugin/marketplace.json is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Scope now reaches the user before install rather than after. description: said "Native Android only" but never said greenfield, so a maintainer with a Fragments/XML app would install Dan and only then discover he has no XML capability by design. With the scope stated, two clauses mirrored too faithfully from ios-dev had to go: the "don't rewrite existing XML in Compose unprompted" line and the "don't reach for a View/XML solution" anti-pattern both promise a capability the file deliberately removed. Greenfield day one had no defined path — Dan was told to read the version catalog "always" and every command assumes a wrapper, neither of which exists before the project does. Project creation is now explicitly the user's, via the Studio wizard, with Dan starting at the first commit. Sharpened the android init ban: it detects every coding agent installed under $HOME and writes a skill into each one, so the harm is machine-wide rather than project-scoped as previously worded. FORBIDDEN now closes by pattern where it enumerated names. Product flavours generate connectedFreeDebugAndroidTest and friends that no list covers, so rule 1 matches the connected prefix; rule 3 becomes an allowlist of exactly one adb subcommand rather than a blocklist that can be read for omissions. The device-gap trigger list is explicitly non-exhaustive, governed by a test instead: if the only way to observe it is on a device, it is a gap. Applied that test to the release rules and found one — R8 failures surface only in a minified build, which nothing in the Verification Cycle produces, so that is a nameable gap too. Scope declines now carry a destination, matching the standard the device section already set, while being honest that no role on this team covers RN/Flutter/Spring-JVM, so the routing is back to the user. Also: SDK-location-not-found framed as an environment condition beside the Robolectric download; standalone install warned that Sage and CI do not exist, so the user is the verifier; README no longer claims RULES.md holds the no-device policy; and the baseline block's "only place version facts live" guarantee made true by deferring the Navigation and Room restatements, with monotone API thresholds named as the one deliberate exception so a refresher does not read them as drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The design doc left metadata.authors as an open item, noting the value was the maintainer's to set rather than to assume. Resolved now: add Daniel Sallai, keep both existing authors. Dan was specified and reviewed in this repo by the current maintainer, so the credit is accurate. But Artem and Alexander authored the persona framework and every sibling agent Dan is modelled on -- ios-dev most directly, whose section order and shared protocol blocks Dan inherits -- so dropping them would misrepresent the lineage. Third author, not replacement. This makes android-dev the only agent with a three-author block; the asymmetry is intentional and should not be swept back to two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deleting the old "don't reach for a View/XML solution" bullet in the scope-consolidation round cut too deep. That bullet was doing a different job from the clauses removed alongside it: those were scope (don't accept brownfield XML projects), this one was an intra-project design preference. Inside a correctly-scoped greenfield Compose app, AndroidView interop is entirely legitimate, which is exactly what makes it over-reachable -- and after the cut nothing discouraged wrapping a View for something Compose already does. All three replacements are scope statements; the only intra-project survivor was the narrow com.google.android.material clause. Restored as one bullet in Compose instructions, phrased as Compose-native-first rather than as an XML boundary so it cannot be misread as a fourth restatement of the scope rule. It affirms AndroidView as legitimate for genuinely unported APIs, names the cost of over-reaching (composition-aware state, previews, recomposition, testability), and ends with a checkable test: be able to name the Compose API you looked for and why it doesn't cover the case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…elopment bundle Registers android-dev in devRoles/localAgents and adds the android platform block (scout/tech-lead/qa-engineer briefings + skillOverlays) to bundle.json, mirroring the ios platform. Adds the four new briefing files: Dan's own android-dev role overlay (heterogeneous JDK/SDK/wrapper/KSP-vs-KAPT environment facts iOS doesn't need), scout's android detection (module graph, KSP vs KAPT, and KMP detection so the parked android-dev/ios-dev seam is at least recorded), tech-lead's Compose state/concurrency ownership, and qa-engineer's emulator discipline — Sage boots exactly one AVD and never targets a physically attached device, keeping her the one role that actually sees the app run since Dan is policy-barred from any device.
… and greenfield claims Address seven review findings on the Task 4 briefings: - qa-engineer.md: name Sage's authorised (serial-pinned) deploy/instrumented-test commands, explicitly distinct from android-dev's forbidden ones; add a zero-AVD branch and first-boot cost disclosure to the emulator rule; route the physical-device escape hatch to the user; disambiguate "on-device" to "instrumented (emulator)"; record the Compose-perf-on-emulator observability gap as an accepted trade. - scout.md: record AVD inventory (existence + a known-good name) since qa-engineer's central rule depends on it and a machine can have a full SDK with zero AVDs. - android-dev.md: state greenfield/Compose as this role's assumption to confirm against scout's findings, not as an installed-time fact — the briefing is seeded before anyone has looked at the project.
…l too The round-2 fix pinned installDebug/adb install but left the instrumented-test command bare. That's the most dangerous of the three: AGP's connected*AndroidTest tasks fan out to every connected device by default (installs just refuse ambiguity instead), so a bare invocation with the user's phone attached alongside the emulator silently runs the suite on the phone too, violating the qa-engineer's own rule 3 four lines below. Pin it and make the surrounding prose state all three commands are pinned, always, rather than reading as a list where the pin happens to appear twice.
… role android-dev was wired into the feature-development bundle in prior commits but stayed invisible across a dozen docs carrying stale python/js/ios-only rosters, install examples, and skill tables. Sweeps README/AGENTS/CLAUDE/ GEMINI, bundles/SPEC.md, the feature-development bundle's BUNDLE/README/ instructions.md, hooks/session-start, role-overrides.md, and the onboarding guides to add android-dev alongside its siblings everywhere a roster is enumerated. Also: instructions.md's Definition of Done now encodes the actual device-verification gate for Android (android-dev's "Not verified:" line must be closed by qa-engineer on an emulator or accepted by the user before merge — this was previously unenforced anywhere); the onboarding prereqs table gains the Android JDK/SDK/Robolectric-download row and a cost-statement callout; test-automation onboarding's mobile mapping gains setup-uiautomator2; corrects a stale --dry-run claim on plain `init` (only fix-copilot supports it); and fixes a skills.json column-alignment nit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… loophole Review found the greenfield-Compose scope restriction (settled to keep the wrong user from installing Dan) never reached a pre-install decision surface — every catalog row/description advertised general Android capability. Add a terse greenfield clause everywhere android-dev is described before install: README (catalog + bundle rows), GEMINI.md, both onboarding indexes, SPEC.md, BUNDLE.md, and the feature-development README/prereqs. Also: give bundles/test-automation's role-overrides.md the same Android routing row feature-development's already got (the localAgents argument against it didn't hold — none of js-dev/python-dev/ios-dev are in that bundle's localAgents either, yet all three already have rows); close the empty-`Not verified:`-line loophole at the project-manager level in instructions.md (previously the anti-loophole clause only lived in a file PM never reads); require the user's merge-gate acceptance to be recorded on the PR/issue, not left in chat; disclose the emulator's real cost (multi-GB image + hardware virtualization, fails on locked-down laptops) in the onboarding heads-up since qa-engineer's emulator is now a merge precondition; and name Android in instructions.md's own opening sentence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nups Add android-dev to the hand-curated .claude-plugin/marketplace.json (mirrors ios-dev's shape), and fold in three small carry-over review fixes: the Android Definition-of-Done gate sentence now covers an empty Not-verified line explicitly, the dev-role picker label says "greenfield", and BUNDLE.md's description reads "greenfield Android Kotlin/Compose" for parity with the iOS phrasing. Generated marketplaces (.cursor-plugin/, .codex-plugin/, .github/plugin/) were already current from earlier tasks — no diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bundles/feature-development/README.md:9 still read "greenfield-Compose Android Kotlin" after cleanup (c) fixed the same phrasing in BUNDLE.md. Match the two surfaces: "greenfield Android Kotlin/Compose". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sclose KMP seam - docs/onboarding/feature-development.md: align the human-facing DoD with the agent-facing instructions.md version (fold the empty Not verified: line into the gate condition, require recorded PR/issue acceptance) so a solo install without project-manager can't merge on an unexamined empty line. - agents/android-dev/AGENT.md: add compose-ui-testing-patterns (Dan authors Compose-via-Robolectric tests, the skill was previously granted only to qa-engineer); add a refresh note on the Play targetSdk deadline, the one baseline fact with no project-level catalog to fall back on. - README.md: bring the compose-ui-testing-patterns table row in line with the credits prose now that android-dev declares it too. - docs/onboarding/feature-development.md: add the KMP ownership disclosure scout's detection logic implied but no doc ever stated.
Eleven PR-review findings across the android-dev branch. SECURITY — path traversal into rmSync (bin/init.mjs). The installed skill's directory name came from a third-party SKILL.md `name:` field, unsanitized, and path.join normalizes: `name: ../../../../tmp/x` escaped the install root, and on --update that path reached rmSync(recursive, force). New exported isBarePathSegment() rejects anything that is not a bare path segment, falls back to the registry id, and reports the rejected value loudly. Verified live against a seeded hostile clone: nothing escaped. Also: warn (non-fatal, on stderr) when upstream's `name:` differs from the registry id — the consumer previously saw a green checkmark for a skill they did not ask for, with no mention of the id that went missing. bin/lib/skill-md.mjs — three parser defects. Scope `name:` to the leading frontmatter block (a `name:` in a fenced code block no longer wins); return null rather than "" for an empty name, so the truthiness-testing installer and the `=== null`-testing guard agree by construction; and `[ \t]*` instead of `\s*`, which matched newlines and made a bare `name:` line capture the next key's value. Header comment rewritten to state the contract. bin/validate-bundles.mjs — reject unknown arguments (a `--check-external` typo silently ran bundle validation and exited 0 under a CI step named "Validate external skill registry entries"); escalate an all-WARN run to exit 1 via a new `verifiedNothing` field on summarizeExternalResults, since a run that verified nothing is an infrastructure failure, not a pass (per-entry WARN stays non-blocking); contain each entry in try/catch so one throw cannot abandon the rest; and correct the header comments (403 in the WARN class, the default FAIL bucket, the leaked R1 label, a dated 403 premise). package.json gains engines >=18 (global fetch, AbortSignal.timeout), and the externals CI job gains timeout-minutes: 5 against ~9.5 min of hung fetches. Docs: android-dev README skills list matched to AGENT.md frontmatter (adds compose-ui-testing-patterns), and README.md corrected — android-cli is in no agent frontmatter and no overlay, so only an explicit --skills installs it. Tests cover the new behaviours only: traversal rejection, empty-name-is-null, unknown-flag rejection, all-WARN escalation, per-entry containment. Verified: npm test 112/112; npm run validate clean; --check-externals still 28/28 PASS; the typo flag exits 2; a real install yields 17 skills on disk with no pending notice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
arozumenko
added a commit
that referenced
this pull request
Aug 9, 2026
Resolves the overlap with #51 (android-dev persona + external-skill registry guard), which touched the same files. skills.json / bundle.json main's structure, with `memory` + `knowledge-curation` re-applied on top package.json keeps BOTH new validate steps — main's validate:externals and this branch's validate:dupes, chained together instructions.md main's body plus the memory-layer section marketplace manifests regenerated rather than hand-merged; they are generated artifacts Verified after resolving: android-dev present, both new skills wired into all four bundles, all four instructions files carry the section, duplicated assets in sync, all bundles valid. Note: `validate:externals` reports 3 pre-existing 404s (chrisbanes/skills Kotlin skills) which arrived with #51 and are unrelated to this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
android-dev("Dan") — a platform-scoped Android developer persona modelled onios-dev— and hardens the external-skill registry that both depend on.Why
android-devand notjava-devThree independent lines of evidence:
bundles/SPEC.mdstates the stack-specific agent is the dev role, andbundle.jsonalready carries aplatforms.iosblock tuning scout / tech-lead / qa-engineer.android-devslots into that machinery with no installer changes;java-devmaps to no platform at all.java-*is occupied ecosystem-wide by a different discipline — every major agent catalog scopes it to server-side JVM (Spring Boot, Java 21+, GraalVM) and none mentions Android. Since a subagent'sdescription:is the entire delegation signal, the name would mis-cue routing.java-devis reserved for a future Spring Boot / JVM-backend persona. Dan'sdescription:disclaims it explicitly.What's included
AGENT.md/SOUL.md/RULES.md/README.md— 17 skills, mirroringios-dev's structureplatforms.androidtuning scout, tech-lead, qa-engineer + 4 briefingsandroid/skills(Google),chrisbanes/skills,appium/skillsinstructions.mdteam contractScope is greenfield Compose, stated on every surface a user reads before installing.
android-devis absolutely no-device;qa-engineergets "one emulator, always" so the verification loop actually closes — a change made after review showed the original design left nobody on the team able to see the app run.No existing agent was modified. The only files under any
agents/directory are the four newandroid-devones.Two live registry bugs fixed
bin/init.mjsderives an installed skill's directory name from the fetchedSKILL.md'sname:field, not from the registryid. A mismatched id installs to the wrong directory and is then silently absent from every role declaring it — while the agent's frontmatter still advertises it.environment-setup-xcuitest404'd — upstream renamed it tosetup-xcuitest. The iOS QA overlay had been advertising a skill it never installed.playwright-best-practiceswas missing itssubdir— found by the new guard immediately after it was added. Worse than it looks: withoutsubdirthe installer was copying the entire clone,.gitincluded, into every consumer's project.bin/validate-bundles.mjs --check-externalsnow asserts both that the subdir resolves and that upstream'sname:equals the registryid— a 200 alone doesn't prove an entry works. Wired into CI.Security fix
bin/init.mjsused the upstreamSKILL.mdname:unsanitized as a directory name.path.joinnormalizes, soname: ../../../../tmp/xescaped the target directory — and under--updatethat path reachedrmSync(recursive: true, force: true). With 28 third-party repos tracked atref: main, this was a live supply-chain surface. Now rejected, with a fallback to the registry id and a loud error.A second traversal vector was found in the same area: the old
/^name:\s*(.+)$/mmatched newlines, so a barename:followed bydescription: ../../..captured the next key's value. The parser is now frontmatter-scoped and newline-safe, and lives in one shared module (bin/lib/skill-md.mjs) imported by both the installer and the validator so they cannot drift.Also hardened: unknown-flag rejection (a typo'd
--check-externalused to fall through to bundle validation and exit 0 under a CI step named for the externals check), escalation when a run verifies nothing (all-WARN used to exit 0),engines: node >=18, and per-entry error containment.Verification
npm test— 112 pass (93 before this branch)npm run validate— bundles + all three generated manifests cleannode bin/validate-bundles.mjs --check-externals— 28/28--target claude/copilot/codex, including--updateidempotency on the CopilotSKILLS-INJECTEDblock (replaced, not duplicated — Dan's 17 skills make it the largest of any agent)android-devalone drops the Playwright/browser skills fromqa-engineer;android-dev,ios-devkeeps them dropped and concatenates both platform briefings;android-dev,js-devcorrectly restores themReview
Reviewed in-role by
tech-leadandbaacross every task, then by a second four-agent pass (code / tests / silent-failures / comments) that found defects in code the first pass had approved — including the path traversal. Both reviewers approve the final state.Known follow-ups (filed, not blocking)
skillOverlayaddreferencing an unknown id warns and exits 0 — same failure class as the bug fixed here, different door. Affects all three bundles.scoutandtest-automation-engineerin both bundles), plus the consistency test that would close the class.runCheckExternalsholdsprocess.exitand the summary string, both survive mutation; no registry-shape guard.../../README.mdrelative links resolve wrong in every agent README (pre-existing).## Current baselineblock needs a refresh owner — the Play targetSdk deadline is the one fact with no project-level authority to fall back on.🤖 Generated with Claude Code