Merge origin/main into docs/skill-contributing-guardrails

Resolve AGENTS.md conflict: keep this PR's pointer to docs/skill-anatomy.md
(the section main patched via #307 is the one this PR removes, so the
line-level fix is moot once the section is gone).

Also fix a stale path the port reintroduced: skill-anatomy.md referenced
/mnt/skills/user/... for scripts; correct it to the repo-relative
skills/<skill-name>/scripts/<script>.sh, matching #307.
This commit is contained in:
Addy Osmani committed 2026-06-28 01:20:50 -07:00
commit da05b987c6
12 files changed
+127 -4

No files matched your search

+2 -2
View File
@@ -7,7 +7,7 @@ Invoke the shipping-and-launch skill.
## Phase A — Parallel fan-out
Spawn three subagents concurrently. Gemini CLI exposes each custom subagent in `.gemini/agents/` as a tool with the same name — so `code-reviewer.md` becomes a `code-reviewer` tool the main agent can call, and `@code-reviewer` works as an explicit invocation in the prompt. **Issue all three subagent tool calls in a single assistant turn so they execute in parallel** — sequential calls defeat the purpose of this command.
Spawn three subagents concurrently. Gemini CLI exposes each custom subagent in `agents/` as a tool with the same name — so `code-reviewer.md` becomes a `code-reviewer` tool the main agent can call, and `@code-reviewer` works as an explicit invocation in the prompt. **Issue all three subagent tool calls in a single assistant turn so they execute in parallel** — sequential calls defeat the purpose of this command.
Dispatch each persona by tool name:
@@ -22,7 +22,7 @@ Constraints (from Gemini CLI's subagent model):
- Do not let one persona delegate to another — keep the fan-out flat.
- For richer multi-agent collaboration where teammates talk to each other instead of just reporting back, see `references/orchestration-patterns.md`.
**Persona resolution.** If you've defined your own `code-reviewer`, `security-auditor`, or `test-engineer` in `.gemini/agents/` or `~/.gemini/agents/`, those take precedence over this plugin's versions — `/ship` picks up your customizations automatically. This is intentional: plugin subagents sit at the bottom of Gemini CLI's scope priority table, so user-level definitions win by design.
**Persona resolution.** If you've defined your own `code-reviewer`, `security-auditor`, or `test-engineer` in `agents/` (this plugin's agents, at the repo root) or `~/.gemini/agents/` (user-level), those take precedence over this plugin's versions — `/ship` picks up your customizations automatically. This is intentional: plugin subagents sit at the bottom of Gemini CLI's scope priority table, so user-level definitions win by design.
## Phase B — Merge in main context
+1
View File
@@ -238,6 +238,7 @@ Quick-reference material that skills pull in when needed:
| Reference | Covers |
|-----------|--------|
| [definition-of-done.md](references/definition-of-done.md) | Project-wide standing bar every change clears, contrasted with per-task acceptance criteria |
| [testing-patterns.md](references/testing-patterns.md) | Test structure, naming, mocking, React/API/E2E examples, anti-patterns |
| [security-checklist.md](references/security-checklist.md) | Pre-commit checks, auth, input validation, headers, CORS, OWASP Top 10 |
| [performance-checklist.md](references/performance-checklist.md) | Core Web Vitals targets, frontend/backend checklists, measurement commands |
+6
View File
@@ -116,6 +116,12 @@ The `.claude/commands/` directory contains slash commands for Claude Code:
| `/ship` | shipping-and-launch |
| `/webperf` | web-performance-auditor (specialist agent, web apps only) |
> **Note:** When installed as a Claude Code plugin you may see a warning like
> _"Default commands/ folder is ignored because the manifest sets 'commands'"_.
> This is expected. The root `commands/` directory belongs to the Antigravity CLI
> and is intentionally separate from `.claude/commands/`. All Claude Code slash
> commands load correctly from `.claude/commands/`; the warning is cosmetic.
## Using References
The `references/` directory contains supplementary checklists:
+1 -1
View File
@@ -126,7 +126,7 @@ When a skill ships runnable helpers under `scripts/`, each script follows these
- Write status messages to stderr: `echo "Message" >&2`.
- Write machine-readable output (JSON) to stdout.
- Include a cleanup trap for temporary files.
- Reference the script path as `/mnt/skills/user/<skill-name>/scripts/<script>.sh`.
- Reference the script path as `skills/<skill-name>/scripts/<script>.sh` (repo-relative).
## Writing Principles
+67
View File
@@ -0,0 +1,67 @@
# Definition of Done
A standing, project-wide bar that every change must clear before it counts as done. Unlike acceptance criteria, which vary per task and answer "did we build the right thing?", the Definition of Done is the same every time and answers "is this finished to our standard?". Use it as the final gate in `planning-and-task-breakdown`, `incremental-implementation`, and `shipping-and-launch`.
## Definition of Done vs. Acceptance Criteria
| | Acceptance Criteria | Definition of Done |
|---|---|---|
| Scope | Specific to one task or spec | Applies to every increment |
| Changes | Different for each item | Fixed and reused |
| Answers | "Did we build *this thing*?" | "Is it *ready*?" |
| Owner | Defined when planning the task | Defined once for the project |
| Example | "User can reset password via email link" | "Tests pass, no regressions, docs updated" |
The two are complementary. A task is done only when **its** acceptance criteria are met **and** the standing Definition of Done is satisfied. Skipping either leaves work that looks finished but is not.
## The Standing Checklist
Apply this to every change before declaring it done.
### Correctness
- [ ] All acceptance criteria for the task are met
- [ ] Code runs and behaves as intended, verified at runtime, not just compiled or typechecked
- [ ] New behavior is covered by tests that fail without the change and pass with it
- [ ] Existing tests still pass; no regressions introduced
- [ ] Edge cases and error paths are handled, not just the happy path
### Quality
- [ ] Code reveals intent through naming and structure; no comments needed to explain *what* it does
- [ ] No duplicated business logic
- [ ] No dead code, debug output, or commented-out blocks left behind
- [ ] Changes are scoped to the task; no unrelated refactors snuck in
- [ ] Linting and formatting pass
The depth behind these items lives in `code-review-and-quality` (the five-axis review) and `code-simplification` (reducing complexity without changing behavior).
### Integration
- [ ] Change works with the rest of the system, not just in isolation
- [ ] Database migrations, config changes, and feature flags are accounted for
- [ ] Backward compatibility considered for any public interface or API change
### Documentation
- [ ] Public interfaces, APIs, and user-facing behavior are documented
- [ ] Architectural decisions worth preserving are recorded (see `documentation-and-adrs`)
- [ ] Documentation describes the current state in timeless language, not the change history
### Ship-readiness
- [ ] Security implications reviewed for any untrusted input, auth, or data handling (see `security-and-hardening`)
- [ ] Observability in place for new critical paths (logs, metrics, traces) (see `observability-and-instrumentation`)
- [ ] Rollback path exists for anything risky (see `shipping-and-launch`)
- [ ] The human has reviewed and approved before merge or deploy
## How to Apply
- **Per task**: confirm the Correctness and Quality sections before checking the task off.
- **Per feature**: confirm Integration and Documentation before considering the feature complete.
- **Per release**: the full checklist is the floor; `shipping-and-launch` adds the deploy-specific gates on top.
Tailor the list to the project once, then reuse it unchanged. A Definition of Done that is renegotiated every sprint is not a Definition of Done.
## Red Flags
- "It's done, I just haven't run it yet": unverified work is not done.
- "Tests pass" used as a synonym for done while docs, regressions, or runtime verification are skipped.
- A different bar applied depending on deadline pressure.
- Acceptance criteria treated as the whole bar, with no standing quality floor.
- "Done" declared before human review on changes that need it.
+34
View File
@@ -45,6 +45,8 @@ Can another engineer (or agent) understand this code without the author explaini
- **Are abstractions earning their complexity?** (Don't generalize until the third use case)
- Would comments help clarify non-obvious intent? (But don't comment obvious code.)
- Are there dead code artifacts: no-op variables (`_unused`), backwards-compat shims, or `// removed` comments?
- **Is a new conditional bolted onto an unrelated flow?** That's a design smell, not a nit — push the logic into its own helper, state, or policy instead of tangling an existing path.
- **Do repeated conditionals on the same shape appear?** They signal a missing model or dispatcher. A "temporary" branch is usually permanent debt.
### 3. Architecture
@@ -55,6 +57,9 @@ Does the change fit the system's design?
- Is there code duplication that should be shared?
- Are dependencies flowing in the right direction (no circular dependencies)?
- Is the abstraction level appropriate (not over-engineered, not too coupled)?
- **Does this refactor reduce complexity or just relocate it?** Count the concepts a reader must hold to follow the change. If a "cleaner" version leaves that count unchanged, it isn't cleaner — prefer the restructuring that makes whole branches, modes, or layers disappear over one that re-centralizes the same logic. Prefer deleting an abstraction to polishing it.
- **Is feature-specific logic leaking into a shared or general-purpose module?** Keep logic in its owning layer, reuse the existing canonical helper instead of a near-duplicate, and don't normalize architectural drift.
- **Are type boundaries explicit?** Question gratuitous `any`/`unknown`/optional/casts and silent fallbacks that paper over an unclear invariant — making the boundary explicit often makes the surrounding control flow simpler.
### 4. Security
@@ -80,6 +85,21 @@ For detailed profiling and optimization, see `performance-optimization`. Does th
- Any missing pagination on list endpoints?
- Any large objects created in hot paths?
## Structural Remedies
When you flag a structural problem, propose the move — not just the problem. A review that only says "this is complex" leaves the author guessing. Reach for a named restructuring:
- **Replace a chain of conditionals** with a typed model or an explicit dispatcher.
- **Collapse duplicate branches** into a single clearer flow.
- **Separate orchestration from business logic** so each reads on its own.
- **Move feature-specific logic** out of a shared module into the package that owns the concept.
- **Reuse the canonical helper** instead of a bespoke near-duplicate.
- **Make a type boundary explicit** so downstream branching disappears.
- **Delete a pass-through wrapper** that adds indirection without clarifying the API.
- **Extract a helper, or split a large file** into focused modules.
Prefer the remedy that removes moving pieces over one that spreads the same complexity around.
## Change Sizing
Small, focused changes are easier to review, faster to merge, and safer to deploy. Target these sizes:
@@ -90,6 +110,8 @@ Small, focused changes are easier to review, faster to merge, and safer to deplo
~1000 lines changed → Too large. Split it.
```
**Watch file size, not just diff size.** A small diff can still push a file past a healthy boundary — around 1000 *total* lines in a single file (distinct from the ~1000 *changed*-lines threshold above) is a common inspection signal, not a hard cap. When a change materially grows an already-large file, ask whether to extract helpers, subcomponents, or modules *first*, before piling more on. Decompose, then add.
**What counts as "one change":** A single self-contained modification that addresses one thing, includes related tests, and keeps the system functional after submission. One part of a feature — not the whole feature.
**Splitting strategies when a change is too large:**
@@ -166,6 +188,8 @@ Label every comment with its severity so the author knows what's required vs opt
This prevents authors from treating all feedback as mandatory and wasting time on optional suggestions.
**Lead with what matters.** Order findings by leverage: correctness and security first, then structural regressions and missed simplifications, then everything else. Don't bury a real issue under cosmetic nits — a few high-conviction comments beat a long list. If you have one structural problem and ten nits, the structural problem *is* the review.
### Step 5: Verify the Verification
Check the author's verification story:
@@ -288,6 +312,8 @@ Part of code review is dependency review:
- [ ] Follows existing patterns
- [ ] No unnecessary coupling or dependencies
- [ ] Appropriate abstraction level
- [ ] Refactors reduce complexity rather than relocate it
- [ ] No feature logic in shared modules; file stays within a healthy size
### Security
- [ ] No secrets in code
@@ -324,6 +350,8 @@ Part of code review is dependency review:
| "We'll clean it up later" | Later never comes. The review is the quality gate — use it. Require cleanup before merge, not after. |
| "AI-generated code is probably fine" | AI code needs more scrutiny, not less. It's confident and plausible, even when wrong. |
| "The tests pass, so it's good" | Tests are necessary but not sufficient. They don't catch architecture problems, security issues, or readability concerns. |
| "The refactor makes it cleaner" | Relocating complexity isn't reducing it. If the reader still holds the same number of concepts, the structure didn't improve — look for the version where branches disappear. |
| "It's only a small addition to this file" | Small diffs still push files past a healthy size and bolt branches onto unrelated flows. Judge the resulting structure, not the diff size. |
## Red Flags
@@ -335,6 +363,10 @@ Part of code review is dependency review:
- No regression tests with bug fix PRs
- Review comments without severity labels — makes it unclear what's required vs optional
- Accepting "I'll fix it later" — it never happens
- A refactor that moves code around without reducing the number of concepts a reader must hold
- A change that grows an already-large file instead of decomposing it
- New conditionals scattered into unrelated code paths (a missing abstraction)
- A bespoke helper that duplicates an existing canonical one, or feature logic placed in a shared module
## Verification
@@ -345,3 +377,5 @@ After review is complete:
- [ ] Tests pass
- [ ] Build succeeds
- [ ] The verification story is documented (what changed, how it was verified)
**Presumptive blockers:** surface and propose the simpler design for each of these; escalate to Required only when the change actively makes structure worse: a refactor that relocates complexity instead of reducing it; a change that pushes a file past the size boundary with no decomposition; feature logic added to a shared module; a near-duplicate of an existing canonical helper; a silent fallback that hides an unclear invariant.
+1 -1
View File
@@ -19,7 +19,7 @@ This skill is primarily an interactive dialogue. Invoke it with an idea, and the
```bash
# Optional: Initialize the ideas directory
bash /mnt/skills/user/idea-refine/scripts/idea-refine.sh
bash skills/idea-refine/scripts/idea-refine.sh
```
**Trigger Phrases:**
@@ -243,3 +243,7 @@ After completing all increments for a task:
- [ ] The build is clean
- [ ] The feature works end-to-end as specified
- [ ] No uncommitted changes remain
## See Also
Per-increment verification is the local check. Before declaring a task done, apply the project-wide Definition of Done as the final gate, the standing bar every increment clears regardless of the task. See `references/definition-of-done.md`.
@@ -221,3 +221,7 @@ Before starting implementation, confirm:
- [ ] No task touches more than ~5 files
- [ ] Checkpoints exist between major phases
- [ ] The human has reviewed and approved the plan
## See Also
Acceptance criteria are per-task and answer "did we build the right thing?". They sit on top of the project-wide Definition of Done, the standing bar every task clears before it counts as done. See `references/definition-of-done.md`.
+1
View File
@@ -265,6 +265,7 @@ Every deployment needs a rollback plan before it happens:
```
## See Also
- For the project-wide Definition of Done that every change must clear before this checklist, see `references/definition-of-done.md`
- For security pre-launch checks, see `references/security-checklist.md`
- For performance pre-launch checklist, see `references/performance-checklist.md`
- For accessibility verification before launch, see `references/accessibility-checklist.md`
+4
View File
@@ -138,6 +138,8 @@ With the validated spec, generate a technical implementation plan:
4. Identify what can be built in parallel vs. what must be sequential
5. Define verification checkpoints between phases
> Follow `planning-and-task-breakdown` for the dependency-graph mapping and vertical-slicing mechanics behind these steps; it is the canonical source. The bullets above are a lightweight summary; if they ever diverge, `planning-and-task-breakdown` takes precedence.
The plan should be reviewable: the human should be able to read it and say "yes, that's the right approach" or "no, change X."
### Phase 3: Tasks
@@ -150,6 +152,8 @@ Break the plan into discrete, implementable tasks:
- Tasks are ordered by dependency, not by perceived importance
- No task should require changing more than ~5 files
> Follow `planning-and-task-breakdown` for the full task-sizing and dependency-ordering mechanics; it is the canonical source. The template below is a lightweight inline form; if they ever diverge, `planning-and-task-breakdown` takes precedence.
**Task template:**
```markdown
- [ ] Task: [Description]
+2
View File
@@ -110,6 +110,8 @@ Your job is surgical precision, not unsolicited renovation.
Every skill includes a verification step. A task is not complete until verification passes. "Seems right" is never sufficient — there must be evidence (passing tests, build output, runtime data).
Per-skill verification is the local check. The project-wide bar that applies to *every* change, regardless of which skill is active, is the Definition of Done: tests pass, no regressions, behavior verified at runtime, docs updated. See `references/definition-of-done.md`. It complements each task's acceptance criteria rather than replacing them.
## Failure Modes to Avoid
These are the subtle errors that look like productivity but create problems: