diff --git a/.github/CHANGELOG.md b/.github/CHANGELOG.md index a7c5c716..c14ff587 100644 --- a/.github/CHANGELOG.md +++ b/.github/CHANGELOG.md @@ -21,11 +21,11 @@ Use this format for new updates: ## 2026-06-02 -- Added `internal-gateway-idea-brainstorming` as a visible fourth gateway skill and thin Copilot wrapper agent, owning substantive idea definition, guided decision interview, convergence, Definition Brief, mandatory critical pass, and validated handoff before operational planning. +- Added `internal-gateway-idea` as a visible fourth gateway skill and thin Copilot wrapper agent, owning substantive idea definition, guided decision interview, convergence, Definition Brief, mandatory critical pass, and validated handoff before operational planning. - Absorbed and retired `internal-idea-define-advisor`, moving its useful owner-map and question-bank concepts into the new gateway bundle without a compatibility stub or alias. - Narrowed `internal-gateway-operational-flow` to delegate unresolved idea work visibly to the new gateway and consume validated Definition Brief handoffs for `plan` without repeating ideation or its critical pass. - Added guided decision interview semantics: evidence-first discovery, iterative numbered question blocks through `grill-me`, visible defaults, compact decision ledger, proportional depth, and checkpoint states. -- Retired the standalone `idea-refine` skill immediately after extracting its useful shaping frameworks and evaluation criteria into `internal-gateway-idea-brainstorming` references, preserving its upstream license and removing the competing runtime entrypoint. +- Retired the standalone `idea-refine` skill immediately after extracting its useful shaping frameworks and evaluation criteria into `internal-gateway-idea` references, preserving its upstream license and removing the competing runtime entrypoint. - Updated adjacent routing owners (`internal-agent-support-next-step`, `internal-agent-support-lane-change-engine`, `internal-gateway-simple-task`, `internal-gateway-critical-master`) to recognize the new gateway in handoffs and lane-change recommendations. - Updated `.github/agents/README.md`, `.github/README.md`, `INTERNAL_CONTRACT.md`, `docs`, wrapper-alignment, mode-contracts, subagent-patterns, home-sync catalog, token benchmarks, and regression tests for the four-gateway model. @@ -133,7 +133,7 @@ Use this format for new updates: - Added `internal-python-runner.sh`, Bash wrappers for every executable repository Python entrypoint, explicit `PyYAML` requirements for `openai-skill-creator`, and wrapper-first invocation guidance across prompts, skills, scripts, security docs, and maintainer workflow references. - Hardened `.github/scripts/validate-copilot-customizations.py`, `tests/test_contract_runner.py`, and `tests/test_validate_copilot_customizations.py` so canonical operational agents must point only to real canonical escalation targets, must not self-route, and stale retired-agent references are caught case-insensitively. - Clarified `internal-router` and `internal-agent-routing-engine` so medium-confidence routing asks at most one targeted clarification question with two clear options before falling back to `internal-planning-leader`. -- Strengthened `.github/skills/internal-code-review/SKILL.md` with a standalone quick-start so the skill stays usable directly as a tactical review asset, not only as the review guard's engine. +- Strengthened `.github/skills/internal-review-code/SKILL.md` with a standalone quick-start so the skill stays usable directly as a tactical review asset, not only as the review guard's engine. - Refreshed `.github/README.md` so the maintainer-facing catalog now matches the live `internal-*`, `obra-*`, and imported support families, the canonical internal agent model, and the actual scripts and workflow present on disk after recent repository restructuring. ## 2026-04-04 diff --git a/.github/INVENTORY.md b/.github/INVENTORY.md index 7c16be2a..2e4f734b 100644 --- a/.github/INVENTORY.md +++ b/.github/INVENTORY.md @@ -32,6 +32,8 @@ This file is the exact path inventory for the live GitHub Copilot catalog in thi ## Skills +- `.github/skills/addyosmani-code-review-and-quality/SKILL.md` +- `.github/skills/addyosmani-code-simplification/SKILL.md` - `.github/skills/agent-os-discover-standards/SKILL.md` - `.github/skills/agent-os-index-standards/SKILL.md` - `.github/skills/agent-os-inject-standards/SKILL.md` @@ -53,11 +55,11 @@ This file is the exact path inventory for the live GitHub Copilot catalog in thi - `.github/skills/awesome-copilot-codeql/SKILL.md` - `.github/skills/awesome-copilot-dependabot/SKILL.md` - `.github/skills/awesome-copilot-secret-scanning/SKILL.md` +- `.github/skills/awesome-copilot-security-review/SKILL.md` - `.github/skills/grill-me/SKILL.md` - `.github/skills/internal-agent-creator/SKILL.md` - `.github/skills/internal-agent-support-lane-change-engine/SKILL.md` - `.github/skills/internal-agent-support-next-step/SKILL.md` -- `.github/skills/internal-ai-resource-review/SKILL.md` - `.github/skills/internal-aws-governance/SKILL.md` - `.github/skills/internal-aws-lambda/SKILL.md` - `.github/skills/internal-aws-mcp-research/SKILL.md` @@ -73,7 +75,6 @@ This file is the exact path inventory for the live GitHub Copilot catalog in thi - `.github/skills/internal-bash/SKILL.md` - `.github/skills/internal-changelog-automation/SKILL.md` - `.github/skills/internal-cloud-policy/SKILL.md` -- `.github/skills/internal-code-review/SKILL.md` - `.github/skills/internal-context-handoff/SKILL.md` - `.github/skills/internal-copilot-audit/SKILL.md` - `.github/skills/internal-copilot-docs-research/SKILL.md` @@ -84,7 +85,6 @@ This file is the exact path inventory for the live GitHub Copilot catalog in thi - `.github/skills/internal-excel/SKILL.md` - `.github/skills/internal-gateway-critical-master/SKILL.md` - `.github/skills/internal-gateway-execute-plans/SKILL.md` -- `.github/skills/internal-gateway-idea-brainstorming/SKILL.md` - `.github/skills/internal-gateway-idea/SKILL.md` - `.github/skills/internal-gateway-simple-task/SKILL.md` - `.github/skills/internal-gateway-writing-plans/SKILL.md` @@ -99,7 +99,6 @@ This file is the exact path inventory for the live GitHub Copilot catalog in thi - `.github/skills/internal-github-pr/SKILL.md` - `.github/skills/internal-github-strategic/SKILL.md` - `.github/skills/internal-go/SKILL.md` -- `.github/skills/internal-high-level-review/SKILL.md` - `.github/skills/internal-java-project/SKILL.md` - `.github/skills/internal-java-spring-boot-development/SKILL.md` - `.github/skills/internal-java/SKILL.md` @@ -116,6 +115,9 @@ This file is the exact path inventory for the live GitHub Copilot catalog in thi - `.github/skills/internal-python-project/SKILL.md` - `.github/skills/internal-python-script/SKILL.md` - `.github/skills/internal-python/SKILL.md` +- `.github/skills/internal-review-ai-resources/SKILL.md` +- `.github/skills/internal-review-code/SKILL.md` +- `.github/skills/internal-review-high-level/SKILL.md` - `.github/skills/internal-skill-creator/SKILL.md` - `.github/skills/internal-tdd/SKILL.md` - `.github/skills/internal-terraform/SKILL.md` @@ -124,7 +126,8 @@ This file is the exact path inventory for the live GitHub Copilot catalog in thi - `.github/skills/local-agent-sync-global-copilot-configs-into-repo/SKILL.md` - `.github/skills/local-agent-sync-install-ai-resources/SKILL.md` - `.github/skills/local-copilot-log-analyzer/SKILL.md` -- `.github/skills/mattpocock-caveman/SKILL.md` +- `.github/skills/mattpocock-handoff/SKILL.md` +- `.github/skills/mattpocock-research/SKILL.md` - `.github/skills/openai-docx/SKILL.md` - `.github/skills/openai-gh-address-comments/SKILL.md` - `.github/skills/openai-gh-fix-ci/SKILL.md` @@ -147,6 +150,7 @@ This file is the exact path inventory for the live GitHub Copilot catalog in thi - `.github/skills/superpowers-writing-plans/SKILL.md` - `.github/skills/terraform-terraform-search-import/SKILL.md` - `.github/skills/terraform-terraform-test/SKILL.md` +- `.github/skills/vercel-find-skills/SKILL.md` ### Support-only imported office skills @@ -168,21 +172,22 @@ These imported `openai-*` office skills remain support-only depth for repositori - `.github/scripts/lib/internal_skills.py` - `.github/scripts/lib/inventory.py` - `.github/scripts/lib/jsonc.py` +- `.github/scripts/lib/repo_paths.py` - `.github/scripts/lib/shared.py` +- `.github/scripts/lib/sync_exclusions.py` - `.github/scripts/lib/syncing.py` - `.github/scripts/lib/token_risks.py` - `.github/scripts/run.sh` - `.github/scripts/sync_copilot_catalog.py` -- `.github/scripts/sync_home_ai_resources.py` - `.github/scripts/validate_internal_skills.py` ## Agents -- `.github/agents/internal-code-review.agent.md` - `.github/agents/internal-gateway-critical-master.agent.md` -- `.github/agents/internal-gateway-idea-brainstorming.agent.md` +- `.github/agents/internal-gateway-idea.agent.md` - `.github/agents/internal-gateway-review.agent.md` - `.github/agents/internal-gateway-simple-task.agent.md` +- `.github/agents/internal-review-code.agent.md` - `.github/agents/local-sync-external-resources.agent.md` - `.github/agents/local-sync-global-copilot-configs-into-repo.agent.md` - `.github/agents/local-sync-install-ai-resources.agent.md` diff --git a/.github/README.md b/.github/README.md index 5b589070..b895198b 100644 --- a/.github/README.md +++ b/.github/README.md @@ -10,10 +10,10 @@ customization assets maintained in `cloud-strategy.github`. ## Agents - Canonical repository-owned gateway agents: - `internal-gateway-idea-brainstorming`, `internal-gateway-review`, + `internal-gateway-idea`, `internal-gateway-review`, `internal-gateway-critical-master`, `internal-gateway-simple-task` - Specialist repository-owned review agents: - `internal-code-review` for code-focused review before merge or follow-up action. + `internal-review-code` for code-focused review before merge or follow-up action. - Approved `extended` retained plans execute through `internal-gateway-execute-plans`. - When extra provenance helps, offer it as an optional follow-up detail and accept diff --git a/.github/agents/README.md b/.github/agents/README.md index f4a7d136..b8aff5f6 100644 --- a/.github/agents/README.md +++ b/.github/agents/README.md @@ -5,11 +5,11 @@ repo-only sync workflows. ## Agent-Owned Core -- `internal-gateway-idea-brainstorming`: owns substantive idea definition, +- `internal-gateway-idea`: owns substantive idea definition, critical challenge, and retained planning before execution. - `internal-gateway-review`: owns generic defect-first review for non-code and mixed artifacts before fixes. -- `internal-code-review`: owns dedicated code-focused review for source, tests, +- `internal-review-code`: owns dedicated code-focused review for source, tests, scripts, build metadata, dependency metadata, and code diffs. - `internal-gateway-critical-master`: owns pressure testing. - `internal-gateway-simple-task`: owns concrete execution and approved compact @@ -19,9 +19,9 @@ repo-only sync workflows. | Agent | Use when | | --- | --- | -| `internal-gateway-idea-brainstorming` | A vague idea or unresolved goal needs definition and retained planning. | +| `internal-gateway-idea` | A vague idea or unresolved goal needs definition and retained planning. | | `internal-gateway-review` | A concrete non-code or mixed artifact needs defect-first review before fixes. | -| `internal-code-review` | A concrete code target needs dedicated code review before merge or follow-up action. | +| `internal-review-code` | A concrete code target needs dedicated code review before merge or follow-up action. | | `internal-gateway-critical-master` | A proposal or plan needs pressure before action. | | `internal-gateway-simple-task` | A concrete low-to-medium-risk task can finish through one focused lane. | diff --git a/.github/agents/internal-gateway-idea-brainstorming.agent.md b/.github/agents/internal-gateway-idea.agent.md similarity index 90% rename from .github/agents/internal-gateway-idea-brainstorming.agent.md rename to .github/agents/internal-gateway-idea.agent.md index b0af104f..8c4cc341 100644 --- a/.github/agents/internal-gateway-idea-brainstorming.agent.md +++ b/.github/agents/internal-gateway-idea.agent.md @@ -1,5 +1,5 @@ --- -name: internal-gateway-idea-brainstorming +name: internal-gateway-idea description: "Use this agent when a repository-owned request starts with a vague idea, unclear goal, unresolved option set, or needs substantive definition, convergence, critical challenge, and retained planning before execution." tools: ["read", "edit", "search", "execute", "web"] disable-model-invocation: true @@ -19,8 +19,8 @@ handoffs: send: false --- -# Internal Gateway Idea Brainstorming +# Internal Gateway Idea ## Core Skill -- `internal-gateway-idea-brainstorming` +- `internal-gateway-idea` diff --git a/.github/agents/internal-gateway-review.agent.md b/.github/agents/internal-gateway-review.agent.md index d9b2018c..f7c06f3f 100644 --- a/.github/agents/internal-gateway-review.agent.md +++ b/.github/agents/internal-gateway-review.agent.md @@ -49,7 +49,7 @@ Evaluate the target through the dimensions that apply to its surface: - **Plans and review packages:** retained plans, specs, audit packages, issue analysis, and decision-support reports. - **Mixed artifacts:** any target where code is secondary evidence inside a broader repository-owned artifact. -Prefer `internal-code-review` when the target is purely code: source, tests, scripts, build files, dependency files, generated-code boundaries, or a code-focused diff. +Prefer `internal-review-code` when the target is purely code: source, tests, scripts, build files, dependency files, generated-code boundaries, or a code-focused diff. ## Critical Counter-Analysis @@ -113,7 +113,7 @@ Use this report shape: - Use this agent when the user asks for review, audit, critique, merge-readiness assessment, prompt or agent review, workflow review, policy review, plan review, or artifact risk assessment. - Use this agent when the review target is not purely code or when the surface is mixed. -- Prefer `internal-code-review` when the requested review is specifically for source code, tests, scripts, build files, dependency files, or a code-focused diff. +- Prefer `internal-review-code` when the requested review is specifically for source code, tests, scripts, build files, dependency files, or a code-focused diff. - Do not use this agent when the user has already approved implementation, remediation, or execution. - Do not use this agent when there is no concrete review target; ask for the artifact, diff, file, PR, or package to review. - Do not delegate to peer agents or hand off to fix lanes. Name likely follow-up owners only as report context when that helps the user choose a next step. diff --git a/.github/agents/internal-gateway-simple-task.agent.md b/.github/agents/internal-gateway-simple-task.agent.md index c78eb7ad..44db7a10 100644 --- a/.github/agents/internal-gateway-simple-task.agent.md +++ b/.github/agents/internal-gateway-simple-task.agent.md @@ -1,22 +1,9 @@ --- name: internal-gateway-simple-task -description: "Use this agent when a concrete low-to-medium-risk repository-owned task can be answered, edited, diagnosed, validated, or planned quickly, or switched to plan mode to produce a retained plan instead of same-chat execution." +description: "Use this agent when a concrete low-to-medium-risk repository-owned task can be answered, edited, diagnosed, or validated quickly in one bounded run, and should stop with reason when complexity or cost breaks that boundary." tools: ["read", "edit", "search", "execute", "web"] disable-model-invocation: true agents: [] -handoffs: - - label: "Next step: Reopen planning" - agent: "internal-gateway-idea-brainstorming" - prompt: "Reopen planning because this task no longer fits the simple single-lane fast path. Preserve scope, validation, and risk, then choose the right retained-plan profile before execution." - send: false - - label: "Next step: Pressure-test task" - agent: "internal-gateway-critical-master" - prompt: "Pressure-test the reasoning, assumptions, or failure modes that made this task leave the simple fast path." - send: false - - label: "Next step: Review target" - agent: "internal-gateway-review" - prompt: "The work above became defect-first analysis rather than direct execution. Review the concrete target and stop before applying fixes." - send: false --- # Internal Gateway Simple Task diff --git a/.github/agents/internal-code-review.agent.md b/.github/agents/internal-review-code.agent.md similarity index 99% rename from .github/agents/internal-code-review.agent.md rename to .github/agents/internal-review-code.agent.md index b0b2d066..61925eff 100644 --- a/.github/agents/internal-code-review.agent.md +++ b/.github/agents/internal-review-code.agent.md @@ -1,5 +1,5 @@ --- -name: internal-code-review +name: internal-review-code description: "Senior repository code reviewer for source code, tests, scripts, build files, dependency files, and code-focused diffs before merge or follow-up action." tools: ["read", "search", "execute"] disable-model-invocation: true diff --git a/.github/agents/local-sync-external-resources.agent.md b/.github/agents/local-sync-external-resources.agent.md index 6b207aab..b0a5efd6 100644 --- a/.github/agents/local-sync-external-resources.agent.md +++ b/.github/agents/local-sync-external-resources.agent.md @@ -1,6 +1,6 @@ --- name: local-sync-external-resources -description: Use this agent when applying, auditing, or planning changes to the declared sync-managed GitHub Copilot catalog in this repository, including keep/update/extract/retire decisions and governance-drift cleanup within the approved managed scope. +description: Use this agent when applying, auditing, or planning declared external resource refreshes through the staged sync CLI. tools: ["read", "edit", "search", "execute", "web"] disable-model-invocation: true agents: [] @@ -10,101 +10,36 @@ agents: [] ## Role -You are the source-side sync and catalog-governance wrapper for this -repository's GitHub Copilot customization assets. - -Use this agent for route selection, managed-scope boundary decisions, approval -posture, and completion expectations. Keep the reusable keep, update, extract, -retire, and anti-drift procedure in the paired core skill. +You are the manifest-driven sync wrapper for this repository's declared +external resource refreshes. Use this agent for audit, plan, and apply +operations through the single canonical CLI. ## Core Skill - `local-agent-sync-external-resources` -## Routing Rules - -- Use this agent for source-side `.github/` catalog governance inside the - declared managed external scope. -- Use this agent when the task is about catalog coherence, naming - normalization, overlap removal, governance drift, or repo-owned replacements - across the managed catalog. -- Treat `sync` as `apply` by default unless the user explicitly asks for an - audit, plan, or dry run. -- Treat `apply` as invalid until `internal-copilot-audit` has completed - preflight and no unresolved `blocking` findings remain. -- Start with `internal-gateway-idea-brainstorming` when the catalog problem still - needs option framing, staged planning, or a user-supplied multi-step - remediation plan. -- Use `internal-agent-creator` when the task is one concrete agent contract, - agent routing boundary, or agent/skill split rather than catalog-wide sync - governance. -- Do not use this agent for target-repository baseline propagation; recommend - `local-sync-global-copilot-configs-into-repo` instead. -- When current platform behavior decides policy, validate it through - `internal-copilot-docs-research` before changing the sync contract. - -## Boundary Definition +## Boundary -- Stay in this lane while the work is source-side `.github/` catalog governance - inside the declared managed scope. -- Keep prefix ownership, imported-resource posture, and source-side propagation - boundaries visible in this wrapper. -- If the request is really source-side planning, consumer-repository sync, or a - local edit outside catalog-governance scope, explain the mismatch and - recommend the better owner visibly. -- Do not route, dispatch, or delegate from this lane. +- `sync` means `apply` by default unless the user explicitly asks for `audit` or `plan`. +- Refuse `apply` when a managed target has uncommitted changes unless the user supplies `--allow-dirty`. +- Stage all fetched and transformed resources outside the repository. +- Do not modify repository targets until the complete candidate tree, normalizations, overrides, and generated patch pass validation. +- Do not refresh or modify any imported skill while implementing sync tooling. -## Scope Contract +## Safety -- This agent owns the visible source-side catalog boundary and the approval - posture for sync-managed assets. -- Repository-owned source assets use `internal-*` by default; source-only sync - tooling uses `local-*`; imported assets keep their managed local ids unless an - approved replacement takes over. -- Imported assets are support depth by default. Prefer an `internal-*` owner - only when routing, governance, terminology, output shape, safety - expectations, or a missing owner requires it. -- Every approved imported in-place override must be mapped in - `.github/skills/local-agent-sync-external-resources/references/imported-asset-overrides.yaml` - and replayable through the bundled override script. -- Allow a direct in-place override only for a strong repo-specific need that - the user explicitly counter-validates and registers in the approved override - bundle. -- Use - `.github/skills/local-agent-sync-external-resources/references/managed-resource-scope.md` - for the exact upstream family map, retained support-only office posture, and - approved imported-override context. -- Active `mattpocock/skills` imports in scope are `grill-me` -> `grill-me` - plus retained `caveman` -> `mattpocock-caveman` from the previous managed - snapshot while `caveman` is absent from current upstream; keep retired Matt - Pocock imports out of the live managed scope. -- Do not add new prefixes, external families, compatibility aliases, or hidden - imported forks unless the user explicitly expands scope. -- When catalog meaning changes, re-check root `AGENTS.md`, - `.github/copilot-instructions.md`, and `.github/INVENTORY.md` in the same - pass. -- Keep retained sync evidence under repository-root `tmp/`. +- Workspace must live under `tmp/sync-externals-skills/`. +- Dirty managed targets block `apply` unless `--allow-dirty` is supplied. +- Override replay is atomic: if any override fails, no candidate changes reach the repository. -## Output Expectations - -Follow the completion-report contract from `.github/copilot-instructions.md`. +## Completion Output In `Outcome`, include: - `Mode`: `apply`, `audit`, or `plan`. -- `Catalog scope`: files reviewed and why. -- `Governance files reviewed`: whether `.github/copilot-instructions.md` and - root `AGENTS.md` were reviewed, changed, or intentionally left unchanged. -- `Canonical decisions`: `keep`, `update`, `extract`, or `retire`. +- `Workspace`: external staging path or `n/a`. +- `Managed assets`: count from the manifest. +- `Changed paths`: list of repository-relative paths that changed. +- `Override results`: status of each replayed override. - `Validation`: commands run and remaining gaps. -- `Remaining blockers or drift`: unresolved issues that prevent or narrow - `apply`. - -Add the following outcome details when refresh execution is involved: - -- `Workspace guard`: where upstream snapshots were staged and whether the - bundled workspace guard passed. -- `Graphify guard`: whether graphify ran after repo-local refresh leftovers - were absent. -- `Scoped validation`: whether whitespace and diff checks were scoped away from - verbatim upstream content, with any accepted upstream notices named. +- `Blockers`: unresolved issues that prevent or narrow `apply`. diff --git a/.github/agents/local-sync-global-copilot-configs-into-repo.agent.md b/.github/agents/local-sync-global-copilot-configs-into-repo.agent.md index ffd2f44e..8f6cfb02 100644 --- a/.github/agents/local-sync-global-copilot-configs-into-repo.agent.md +++ b/.github/agents/local-sync-global-copilot-configs-into-repo.agent.md @@ -23,7 +23,7 @@ Use this agent for route selection, mode selection, approval posture, and bounda - Use this agent for consumer-repository baseline propagation, drift assessment, `plan`, `audit`, and explicit `apply` runs. - Use this agent when the target repository must inherit the root `AGENTS.md` policy model, review-only Copilot configuration, `.github/INVENTORY.md`, repository-root `LESSONS_LEARNED.md`, and explicitly shared hygiene files. - Select `apply` only on explicit request, after the current evidence shows a conflict-safe plan and no unmanaged target-local cleanup is being implied. -- Do not use this agent for source-side catalog governance, external-resource refreshes, or managed-scope redesign in this repository; recommend `local-sync-external-resources` or `internal-gateway-idea-brainstorming` as appropriate. +- Do not use this agent for source-side catalog governance, external-resource refreshes, or managed-scope redesign in this repository; recommend `local-sync-external-resources` or `internal-gateway-idea` as appropriate. - Do not use this agent for one-resource agent or skill authoring; recommend `internal-agent-creator` or `internal-skill-creator` as appropriate. - When current platform behavior decides sync policy, validate it through `internal-copilot-docs-research` before changing the contract. diff --git a/.github/prompts/internal-review-ai-resources.prompt.md b/.github/prompts/internal-review-ai-resources.prompt.md index 03ad1fb2..28fbeb82 100644 --- a/.github/prompts/internal-review-ai-resources.prompt.md +++ b/.github/prompts/internal-review-ai-resources.prompt.md @@ -35,9 +35,9 @@ Use these repository sources first: - [.github/INVENTORY.md](../INVENTORY.md) - [INTERNAL_CONTRACT.md](../../INTERNAL_CONTRACT.md) - [.github/agents/internal-gateway-review.agent.md](../agents/internal-gateway-review.agent.md) -- [.github/skills/internal-ai-resource-review/SKILL.md](../skills/internal-ai-resource-review/SKILL.md) +- [.github/skills/internal-review-ai-resources/SKILL.md](../skills/internal-review-ai-resources/SKILL.md) -Then use `internal-ai-resource-review` as the reusable qualitative owner for +Then use `internal-review-ai-resources` as the reusable qualitative owner for profile selection, target coverage, lifecycle checks, report shape, and bundle review depth. diff --git a/.github/repo-profiles.yml b/.github/repo-profiles.yml index 51cf4e82..5ef230c1 100644 --- a/.github/repo-profiles.yml +++ b/.github/repo-profiles.yml @@ -9,7 +9,7 @@ version: 1 _common_skills: &common_skills - skills/superpowers-brainstorming/SKILL.md - - skills/internal-code-review/SKILL.md + - skills/internal-review-code/SKILL.md - skills/superpowers-dispatching-parallel-agents/SKILL.md - skills/superpowers-executing-plans/SKILL.md - skills/superpowers-finishing-a-development-branch/SKILL.md diff --git a/.github/scripts/benchmark_skill_tokens.py b/.github/scripts/benchmark_skill_tokens.py index 2bf7ca6d..cc203524 100644 --- a/.github/scripts/benchmark_skill_tokens.py +++ b/.github/scripts/benchmark_skill_tokens.py @@ -107,15 +107,14 @@ def build_scenario_report(root: Path) -> list[dict[str, Any]]: return reports -GATEWAY_SKILL = "internal-gateway-idea-brainstorming" +GATEWAY_SKILL = "internal-gateway-idea" REVIEW_COUNTERCHECK_SKILL = "internal-gateway-critical-master" GATEWAY_REQUIRED_CONTEXT_SCENARIOS: dict[str, list[str]] = { "Direct execute": [GATEWAY_SKILL], "Define Gate 0": [GATEWAY_SKILL, "grill-me"], "Define idea and critical": [ - GATEWAY_SKILL, "grill-me", "internal-gateway-idea-brainstorming", - "internal-gateway-critical-master", + GATEWAY_SKILL, "grill-me", "internal-gateway-critical-master", ], "Plan handoff": [GATEWAY_SKILL, "internal-gateway-writing-plans", "internal-agent-support-next-step"], "Approved apply-plan": [GATEWAY_SKILL, "internal-gateway-execute-plans"], @@ -124,7 +123,7 @@ def build_scenario_report(root: Path) -> list[dict[str, Any]]: ], } -IDEA_GATEWAY_SKILL = "internal-gateway-idea-brainstorming" +IDEA_GATEWAY_SKILL = "internal-gateway-idea" IDEA_GATEWAY_SCENARIOS: dict[str, list[str]] = { "Idea core entry": [IDEA_GATEWAY_SKILL], diff --git a/.github/scripts/lib/catalog_checks.py b/.github/scripts/lib/catalog_checks.py index 933deef9..24142d83 100644 --- a/.github/scripts/lib/catalog_checks.py +++ b/.github/scripts/lib/catalog_checks.py @@ -13,7 +13,7 @@ IMPORTED_ASSET_OVERRIDES_PATH, INVENTORY_PATH, LEGACY_AGENT_TOOL_IDS, - SUPERPOWERS_NORMALIZATION_PATH, + MANAGED_EXTERNAL_RESOURCES_PATH, Finding, finding_sort_key, is_imported_asset, @@ -55,6 +55,7 @@ def run_consistency_checks(root: Path, include_token_risks: bool = False) -> lis findings.extend(check_source_instruction_contracts(root)) findings.extend(check_residual_instruction_family_references(root)) findings.extend(check_imported_asset_overrides(root)) + findings.extend(check_external_resource_manifest(root)) findings.extend(check_superpowers_import_naming(root)) findings.extend(check_broken_local_links(root)) if include_token_risks: @@ -742,183 +743,157 @@ def check_imported_asset_overrides(root: Path) -> list[Finding]: return findings -def check_superpowers_import_naming(root: Path) -> list[Finding]: - config_path = root / SUPERPOWERS_NORMALIZATION_PATH - if not config_path.exists(): +def check_external_resource_manifest(root: Path) -> list[Finding]: + manifest_path = root / MANAGED_EXTERNAL_RESOURCES_PATH + if not manifest_path.exists(): return [ Finding( severity="blocking", - code="superpowers-normalization-reference-missing", - path=SUPERPOWERS_NORMALIZATION_PATH, - message="The obra/superpowers import normalization reference is missing.", - suggestion="Restore the reference so sync refreshes and catalog validation share one naming map.", + code="external-resource-manifest-missing", + path=MANAGED_EXTERNAL_RESOURCES_PATH, + message="The consolidated external resource manifest is missing.", + suggestion="Restore references/managed-resources.yaml in the sync bundle.", ) ] try: - payload = yaml.safe_load(read_text(config_path)) or {} + payload = yaml.safe_load(read_text(manifest_path)) or {} except yaml.YAMLError as error: return [ Finding( severity="blocking", - code="superpowers-normalization-reference-invalid-yaml", - path=SUPERPOWERS_NORMALIZATION_PATH, - message=f"The obra/superpowers normalization reference is not valid YAML: {error}.", - suggestion="Fix the YAML syntax before relying on sync normalization.", + code="external-resource-manifest-invalid-yaml", + path=MANAGED_EXTERNAL_RESOURCES_PATH, + message=f"The consolidated manifest is not valid YAML: {error}.", + suggestion="Fix the YAML syntax so the manifest remains auditable.", ) ] - managed_skills = normalize_superpowers_managed_skills(payload) - if not managed_skills: + if not isinstance(payload, dict) or payload.get("version") != 1: return [ Finding( severity="blocking", - code="superpowers-normalization-reference-empty", - path=SUPERPOWERS_NORMALIZATION_PATH, - message="The obra/superpowers normalization reference does not declare managed skills.", - suggestion="Add the managed skill map from the retained migration plan.", + code="external-resource-manifest-invalid-shape", + path=MANAGED_EXTERNAL_RESOURCES_PATH, + message="The consolidated manifest must be a version 1 YAML mapping.", + suggestion="Set version: 1 and declare sources, normalizations, and watchlist.", ) ] - findings: list[Finding] = [] - for entry in managed_skills: - legacy_local = entry["legacy_local"] - local = entry["local"] - legacy_directory = root / ".github/skills" / legacy_local - if legacy_directory.exists(): - findings.append( - Finding( - severity="blocking", - code="superpowers-import-legacy-skill-directory", - path=legacy_directory.relative_to(root).as_posix(), - message="A managed obra/superpowers skill still uses the retired `obra-*` directory name.", - suggestion=f"Rename the skill directory to `.github/skills/{local}`.", - ) + raw_sources = payload.get("sources") + if not isinstance(raw_sources, dict) or not raw_sources: + return [ + Finding( + severity="blocking", + code="external-resource-manifest-invalid-shape", + path=MANAGED_EXTERNAL_RESOURCES_PATH, + message="The consolidated manifest must declare a non-empty sources mapping.", + suggestion="Add at least one source with pinned ref and assets.", ) + ] - for skill_file in (legacy_directory / "SKILL.md", root / ".github/skills" / local / "SKILL.md"): - if not skill_file.exists(): + findings: list[Finding] = [] + seen_local: set[str] = set() + for source_id, raw_source in raw_sources.items(): + if not isinstance(raw_source, dict): + continue + raw_assets = raw_source.get("assets") + if not isinstance(raw_assets, list): + continue + for raw_asset in raw_assets: + if not isinstance(raw_asset, dict): continue - skill_name = load_frontmatter(skill_file).get("name") - if skill_name == local: + local = raw_asset.get("local") + if not isinstance(local, str) or not local.strip(): continue - findings.append( - Finding( - severity="blocking", - code="superpowers-import-skill-name-mismatch", - path=skill_file.relative_to(root).as_posix(), - message="A managed obra/superpowers skill frontmatter name does not match its canonical local id.", - suggestion=f"Set `name: {local}` so the skill id matches the directory name.", + local = local.strip() + if local in seen_local: + findings.append( + Finding( + severity="blocking", + code="external-resource-manifest-duplicate-target", + path=local, + message=f"The managed target {local} is declared more than once.", + suggestion="Keep one manifest entry per managed local path.", + ) ) - ) + else: + seen_local.add(local) - findings.extend(check_superpowers_legacy_references(root, payload, managed_skills)) return findings -def normalize_superpowers_managed_skills(payload: dict[str, object]) -> list[dict[str, str]]: - raw_entries = payload.get("managed_skills") - if not isinstance(raw_entries, list): +def check_superpowers_import_naming(root: Path) -> list[Finding]: + manifest_path = root / MANAGED_EXTERNAL_RESOURCES_PATH + if not manifest_path.exists(): return [] - normalized_entries: list[dict[str, str]] = [] - for raw_entry in raw_entries: - if not isinstance(raw_entry, dict): + try: + payload = yaml.safe_load(read_text(manifest_path)) or {} + except yaml.YAMLError: + return [] + + if not isinstance(payload, dict): + return [] + + raw_sources = payload.get("sources") + if not isinstance(raw_sources, dict): + return [] + + superpowers_source = raw_sources.get("obra-superpowers") + if not isinstance(superpowers_source, dict): + return [] + + raw_assets = superpowers_source.get("assets") + if not isinstance(raw_assets, list): + return [] + + managed_skills: list[dict[str, str]] = [] + for raw_asset in raw_assets: + if not isinstance(raw_asset, dict): continue - upstream = raw_entry.get("upstream") - legacy_local = raw_entry.get("legacy_local") - local = raw_entry.get("local") - if not all(isinstance(value, str) and value.strip() for value in (upstream, legacy_local, local)): + local = raw_asset.get("local") + canonical_name = raw_asset.get("canonical_name") + if not ( + isinstance(local, str) + and local.strip() + and isinstance(canonical_name, str) + and canonical_name.strip() + ): continue - normalized_entries.append( + local_dir = local.strip().removeprefix(".github/skills/") + managed_skills.append( { - "upstream": upstream.strip(), - "legacy_local": legacy_local.strip(), "local": local.strip(), + "local_dir": local_dir, + "canonical_name": canonical_name.strip(), } ) - return normalized_entries + if not managed_skills: + return [] -def check_superpowers_legacy_references( - root: Path, - payload: dict[str, object], - managed_skills: list[dict[str, str]], -) -> list[Finding]: findings: list[Finding] = [] - legacy_tokens = {entry["legacy_local"] for entry in managed_skills} - upstream_reference_tokens = { - f"superpowers:{entry['upstream']}": entry["local"] for entry in managed_skills - } - - for relative_path in collect_superpowers_scan_paths(root, payload): - text = read_text(root / relative_path) - for legacy_token in sorted(legacy_tokens): - if legacy_token not in text: - continue - findings.append( - Finding( - severity="blocking", - code="superpowers-import-legacy-reference", - path=relative_path, - message=f"A live catalog asset still references retired local id `{legacy_token}`.", - suggestion="Replace managed obra/superpowers ids with the canonical `superpowers-*` ids.", - ) - ) - - for upstream_token, local in sorted(upstream_reference_tokens.items()): - if upstream_token not in text: - continue - findings.append( - Finding( - severity="blocking", - code="superpowers-import-upstream-reference", - path=relative_path, - message=f"A live catalog asset still references upstream skill id `{upstream_token}`.", - suggestion=f"Use the canonical local id `{local}` instead.", - ) + for entry in managed_skills: + skill_file = root / entry["local"] / "SKILL.md" + if not skill_file.exists(): + continue + skill_name = load_frontmatter(skill_file).get("name") + if skill_name == entry["canonical_name"]: + continue + findings.append( + Finding( + severity="blocking", + code="superpowers-import-skill-name-mismatch", + path=skill_file.relative_to(root).as_posix(), + message="A managed obra/superpowers skill frontmatter name does not match its canonical local id.", + suggestion=f"Set `name: {entry['canonical_name']}` so the skill id matches the directory name.", ) + ) return findings -def collect_superpowers_scan_paths(root: Path, payload: dict[str, object]) -> list[str]: - live_scan = payload.get("live_scan") if isinstance(payload.get("live_scan"), dict) else {} - raw_includes = live_scan.get("include") if isinstance(live_scan, dict) else None - includes = raw_includes if isinstance(raw_includes, list) else [] - ignored_files = set(IGNORED_SYNC_FILENAMES) - raw_ignored_files = live_scan.get("ignored_files") if isinstance(live_scan, dict) else None - if isinstance(raw_ignored_files, list): - ignored_files.update(item for item in raw_ignored_files if isinstance(item, str)) - - paths: set[str] = set() - for include in includes: - if not isinstance(include, str) or not include.strip(): - continue - candidate = root / include - if candidate.is_file() and should_scan_superpowers_path(root, candidate, ignored_files): - paths.add(candidate.relative_to(root).as_posix()) - continue - if not candidate.is_dir(): - continue - for child in candidate.rglob("*"): - if child.is_file() and should_scan_superpowers_path(root, child, ignored_files): - paths.add(child.relative_to(root).as_posix()) - - return sorted(paths) - - -def should_scan_superpowers_path(root: Path, path: Path, ignored_files: set[str]) -> bool: - relative_path = path.relative_to(root).as_posix() - if path.name in ignored_files: - return False - if any(part in IGNORED_SYNC_PARTS for part in path.relative_to(root).parts): - return False - if relative_path.startswith("tmp/"): - return False - return path.suffix in {".md", ".yaml", ".yml", ".json", ".jsonc", ".py", ".patch"} - - def check_broken_local_links(root: Path) -> list[Finding]: findings: list[Finding] = [] for path in collect_repository_owned_markdown_paths(root): diff --git a/.github/scripts/lib/repo_paths.py b/.github/scripts/lib/repo_paths.py new file mode 100644 index 00000000..da0625c8 --- /dev/null +++ b/.github/scripts/lib/repo_paths.py @@ -0,0 +1,13 @@ +"""Shared repository root discovery.""" + +from __future__ import annotations + +from pathlib import Path + + +def find_repo_root(start: Path) -> Path: + candidate = start.resolve() + for current in (candidate, *candidate.parents): + if (current / ".github").is_dir(): + return current + raise FileNotFoundError(f"Unable to find repository root from {start}") diff --git a/.github/scripts/lib/shared.py b/.github/scripts/lib/shared.py index 923da65e..50415e2b 100644 --- a/.github/scripts/lib/shared.py +++ b/.github/scripts/lib/shared.py @@ -74,7 +74,7 @@ ) INVENTORY_PATH = ".github/INVENTORY.md" IMPORTED_ASSET_OVERRIDES_PATH = ".github/skills/local-agent-sync-external-resources/references/imported-asset-overrides.yaml" -SUPERPOWERS_NORMALIZATION_PATH = ".github/skills/local-agent-sync-external-resources/references/superpowers-normalization.yaml" +MANAGED_EXTERNAL_RESOURCES_PATH = ".github/skills/local-agent-sync-external-resources/references/managed-resources.yaml" @dataclass(frozen=True) @@ -190,38 +190,50 @@ def sha256_file(path: Path) -> str: def git_revision(root: Path) -> str | None: - result = subprocess.run( - ["git", "rev-parse", "--short", "HEAD"], - cwd=root, - text=True, - capture_output=True, - check=False, - ) - if result.returncode != 0: + try: + result = subprocess.run( + ["git", "rev-parse", "--short", "HEAD"], + cwd=root, + text=True, + capture_output=True, + check=False, + timeout=10, + ) + if result.returncode != 0: + return None + return result.stdout.strip() or None + except (subprocess.TimeoutExpired, OSError): return None - return result.stdout.strip() or None def is_git_dirty(root: Path) -> bool: - result = subprocess.run( - ["git", "status", "--porcelain"], - cwd=root, - text=True, - capture_output=True, - check=False, - ) - if result.returncode != 0: + try: + result = subprocess.run( + ["git", "status", "--porcelain"], + cwd=root, + text=True, + capture_output=True, + check=False, + timeout=10, + ) + if result.returncode != 0: + return False + return bool(result.stdout.strip()) + except (subprocess.TimeoutExpired, OSError): return False - return bool(result.stdout.strip()) def git_dirty_paths(root: Path) -> list[str]: - result = subprocess.run( - ["git", "status", "--porcelain=v1", "-z", "--untracked-files=all"], - cwd=root, - capture_output=True, - check=False, - ) + try: + result = subprocess.run( + ["git", "status", "--porcelain=v1", "-z", "--untracked-files=all"], + cwd=root, + capture_output=True, + check=False, + timeout=10, + ) + except (subprocess.TimeoutExpired, OSError): + return [] if result.returncode != 0 or not result.stdout: return [] diff --git a/.github/scripts/lib/sync_exclusions.py b/.github/scripts/lib/sync_exclusions.py new file mode 100644 index 00000000..285c73d4 --- /dev/null +++ b/.github/scripts/lib/sync_exclusions.py @@ -0,0 +1,34 @@ +"""Shared sync exclusion rules for runtime artifacts.""" + +from __future__ import annotations + +from pathlib import Path + +IGNORED_SYNC_PARTS: frozenset[str] = frozenset({ + ".venv", + "__pycache__", + ".pytest_cache", +}) + +IGNORED_SYNC_SUFFIXES: frozenset[str] = frozenset({ + ".pyc", + ".pyo", +}) + + +def should_ignore_sync_path(path: Path) -> bool: + parts = path.parts + if any(part in IGNORED_SYNC_PARTS for part in parts): + return True + if path.suffix in IGNORED_SYNC_SUFFIXES: + return True + return False + + +def sync_copytree_ignore(directory: str, names: list[str]) -> set[str]: + ignored: set[str] = set() + for name in names: + candidate = Path(directory) / name + if should_ignore_sync_path(candidate): + ignored.add(name) + return ignored diff --git a/.github/scripts/lib/token_risks.py b/.github/scripts/lib/token_risks.py index c078885c..e09c9787 100644 --- a/.github/scripts/lib/token_risks.py +++ b/.github/scripts/lib/token_risks.py @@ -51,7 +51,7 @@ ) -GATEWAY_CORE_SKILL_PATH = ".github/skills/internal-gateway-idea-brainstorming/SKILL.md" +GATEWAY_CORE_SKILL_PATH = ".github/skills/internal-gateway-idea/SKILL.md" GATEWAY_CORE_BYTE_BUDGET = 16286 GATEWAY_REQUIRED_CONTEXT_BYTE_BUDGET = 30000 GATEWAY_UNIVERSAL_PRELOAD_MARKERS = ( @@ -153,7 +153,7 @@ def check_delegated_review_prompt_budget(root: Path) -> list[Finding]: ), suggestion=( "Keep user inputs and the analysis-only boundary in the prompt, but move reusable workflow " - "detail into .github/skills/internal-ai-resource-review/." + "detail into .github/skills/internal-review-ai-resources/." ), ) ] diff --git a/.github/scripts/run.sh b/.github/scripts/run.sh index 257f1f78..a7a126e4 100755 --- a/.github/scripts/run.sh +++ b/.github/scripts/run.sh @@ -138,7 +138,7 @@ resolve_script() { printf '%s\n' "$SCRIPT_DIR/detect_token_risks.py" ;; sync_home_ai_resources|sync_home_ai_resources.py) - printf '%s\n' "$SCRIPT_DIR/sync_home_ai_resources.py" + printf '%s\n' "$REPO_ROOT/.github/skills/local-agent-sync-install-ai-resources/scripts/run.sh" ;; sync_copilot_catalog|sync_copilot_catalog.py) printf '%s\n' "$SCRIPT_DIR/sync_copilot_catalog.py" @@ -206,6 +206,10 @@ main() { } shift + if [[ "$script_path" == *.sh ]]; then + exec bash "$script_path" "$@" + fi + load_required_python_version select_python_bin require_command "$PYTHON_BIN" diff --git a/.github/scripts/sync_home_ai_resources.py b/.github/scripts/sync_home_ai_resources.py deleted file mode 100755 index 7ca83aea..00000000 --- a/.github/scripts/sync_home_ai_resources.py +++ /dev/null @@ -1,52 +0,0 @@ -#!/usr/bin/env python3 -"""Purpose: repo-local wrapper for the bundled home AI resource sync skill script. - -Usage examples: - python3 ./.github/scripts/sync_home_ai_resources.py sync --targets skills --format report - python3 ./.github/scripts/sync_home_ai_resources.py apply --targets codex --create-missing-dirs --format report -""" - -from __future__ import annotations - -import importlib.util -import sys -from pathlib import Path - -SKILL_SCRIPT_DIR = ( - Path(__file__).resolve().parents[1] - / "skills/local-agent-sync-install-ai-resources/scripts" -) -SKILL_CLI_PATH = SKILL_SCRIPT_DIR / "sync_home_ai_resources.py" - - -def load_skill_cli(): - inserted_path = False - if SKILL_SCRIPT_DIR.as_posix() not in sys.path: - sys.path.insert(0, SKILL_SCRIPT_DIR.as_posix()) - inserted_path = True - try: - spec = importlib.util.spec_from_file_location( - "_local_agent_sync_home_ai_resources_cli", - SKILL_CLI_PATH, - ) - if spec is None or spec.loader is None: - raise RuntimeError(f"Unable to load skill CLI from {SKILL_CLI_PATH}") - module = importlib.util.module_from_spec(spec) - sys.modules[spec.name] = module - spec.loader.exec_module(module) - return module - finally: - if inserted_path: - sys.path.remove(SKILL_SCRIPT_DIR.as_posix()) - - -SKILL_CLI = load_skill_cli() -parse_args = SKILL_CLI.parse_args - - -def main() -> int: - return SKILL_CLI.run(parse_args()) - - -if __name__ == "__main__": - raise SystemExit(main()) diff --git a/.github/security-baseline.md b/.github/security-baseline.md index ed92640b..25109b6b 100644 --- a/.github/security-baseline.md +++ b/.github/security-baseline.md @@ -65,8 +65,8 @@ Provide a portable baseline that teams can apply before enabling repository-wide | `shellcheck` on `.github/scripts/` | Automated | pre-commit + CI | | Secret placeholder avoidance in examples/generated artifacts | Partial | pre-commit hooks + review | | OIDC over long-lived secrets | Manual review | `internal-github-actions` | -| IAM least privilege (AWS/Azure/GCP) | Manual review | `internal-code-review` + `internal-terraform` | -| Supply chain hardening | Manual review | `internal-github-actions` + `internal-code-review` | +| IAM least privilege (AWS/Azure/GCP) | Manual review | `internal-review-code` + `internal-terraform` | +| Supply chain hardening | Manual review | `internal-github-actions` + `internal-review-code` | | Branch protection for `.github/**` | Manual review | repository settings requiring `_github-catalog-validation` | | Read-only reviewer agents | Manual review | agent review | | CHANGELOG-based change governance | Manual review | PR review | diff --git a/.github/skills/addyosmani-code-review-and-quality/SKILL.md b/.github/skills/addyosmani-code-review-and-quality/SKILL.md new file mode 100644 index 00000000..ad789bf8 --- /dev/null +++ b/.github/skills/addyosmani-code-review-and-quality/SKILL.md @@ -0,0 +1,381 @@ +--- +name: addyosmani-code-review-and-quality +description: Conducts multi-axis code review. Use before merging any change. Use when reviewing code written by yourself, another agent, or a human. Use when you need to assess code quality across multiple dimensions before it enters the main branch. +--- + +# Code Review and Quality + +## Overview + +Multi-dimensional code review with quality gates. Every change gets reviewed before merge — no exceptions. Review covers five axes: correctness, readability, architecture, security, and performance. + +**The approval standard:** Approve a change when it definitely improves overall code health, even if it isn't perfect. Perfect code doesn't exist — the goal is continuous improvement. Don't block a change because it isn't exactly how you would have written it. If it improves the codebase and follows the project's conventions, approve it. + +## When to Use + +- Before merging any PR or change +- After completing a feature implementation +- When another agent or model produced code you need to evaluate +- When refactoring existing code +- After any bug fix (review both the fix and the regression test) + +## The Five-Axis Review + +Every review evaluates code across these dimensions: + +### 1. Correctness + +Does the code do what it claims to do? + +- Does it match the spec or task requirements? +- Are edge cases handled (null, empty, boundary values)? +- Are error paths handled (not just the happy path)? +- Does it pass all tests? Are the tests actually testing the right things? +- Are there off-by-one errors, race conditions, or state inconsistencies? + +### 2. Readability & Simplicity + +Can another engineer (or agent) understand this code without the author explaining it? + +- Are names descriptive and consistent with project conventions? (No `temp`, `data`, `result` without context) +- Is the control flow straightforward (avoid nested ternaries, deep callbacks)? +- Is the code organized logically (related code grouped, clear module boundaries)? +- Are there any "clever" tricks that should be simplified? +- **Could this be done in fewer lines?** (1000 lines where 100 suffice is a failure) +- **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 + +Does the change fit the system's design? + +- Does it follow existing patterns or introduce a new one? If new, is it justified? +- Does it maintain clean module boundaries? +- 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 + +For detailed security guidance, see `security-and-hardening`. Does the change introduce vulnerabilities? + +- Is user input validated and sanitized? +- Are secrets kept out of code, logs, and version control? +- Is authentication/authorization checked where needed? +- Are SQL queries parameterized (no string concatenation)? +- Are outputs encoded to prevent XSS? +- Are dependencies from trusted sources with no known vulnerabilities? +- Is data from external sources (APIs, logs, user content, config files) treated as untrusted? +- Are external data flows validated at system boundaries before use in logic or rendering? + +### 5. Performance + +For detailed profiling and optimization, see `performance-optimization`. Does the change introduce performance problems? + +- Any N+1 query patterns? +- Any unbounded loops or unconstrained data fetching? +- Any synchronous operations that should be async? +- Any unnecessary re-renders in UI components? +- 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: + +``` +~100 lines changed → Good. Reviewable in one sitting. +~300 lines changed → Acceptable if it's a single logical change. +~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:** + +| Strategy | How | When | +|----------|-----|------| +| **Stack** | Submit a small change, start the next one based on it | Sequential dependencies | +| **By file group** | Separate changes for groups needing different reviewers | Cross-cutting concerns | +| **Horizontal** | Create shared code/stubs first, then consumers | Layered architecture | +| **Vertical** | Break into smaller full-stack slices of the feature | Feature work | + +**When large changes are acceptable:** Complete file deletions and automated refactoring where the reviewer only needs to verify intent, not every line. + +**Separate refactoring from feature work.** A change that refactors existing code and adds new behavior is two changes — submit them separately. Small cleanups (variable renaming) can be included at reviewer discretion. + +## Change Descriptions + +Every change needs a description that stands alone in version control history. + +**First line:** Short, imperative, standalone. "Delete the FizzBuzz RPC" not "Deleting the FizzBuzz RPC." Must be informative enough that someone searching history can understand the change without reading the diff. + +**Body:** What is changing and why. Include context, decisions, and reasoning not visible in the code itself. Link to bug numbers, benchmark results, or design docs where relevant. Acknowledge approach shortcomings when they exist. + +**Anti-patterns:** "Fix bug," "Fix build," "Add patch," "Moving code from A to B," "Phase 1," "Add convenience functions." + +## Review Process + +### Step 1: Understand the Context + +Before looking at code, understand the intent: + +``` +- What is this change trying to accomplish? +- What spec or task does it implement? +- What is the expected behavior change? +``` + +### Step 2: Review the Tests First + +Tests reveal intent and coverage: + +``` +- Do tests exist for the change? +- Do they test behavior (not implementation details)? +- Are edge cases covered? +- Do tests have descriptive names? +- Would the tests catch a regression if the code changed? +``` + +### Step 3: Review the Implementation + +Walk through the code with the five axes in mind: + +``` +For each file changed: +1. Correctness: Does this code do what the test says it should? +2. Readability: Can I understand this without help? +3. Architecture: Does this fit the system? +4. Security: Any vulnerabilities? +5. Performance: Any bottlenecks? +``` + +### Step 4: Categorize Findings + +Label every comment with its severity so the author knows what's required vs optional: + +| Prefix | Meaning | Author Action | +|--------|---------|---------------| +| *(no prefix)* | Required change | Must address before merge | +| **Critical:** | Blocks merge | Security vulnerability, data loss, broken functionality | +| **Nit:** | Minor, optional | Author may ignore — formatting, style preferences | +| **Optional:** / **Consider:** | Suggestion | Worth considering but not required | +| **FYI** | Informational only | No action needed — context for future reference | + +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: + +``` +- What tests were run? +- Did the build pass? +- Was the change tested manually? +- Are there screenshots for UI changes? +- Is there a before/after comparison? +``` + +## Multi-Model Review Pattern + +Use different models for different review perspectives: + +``` +Model A writes the code + │ + ▼ +Model B reviews for correctness and architecture + │ + ▼ +Model A addresses the feedback + │ + ▼ +Human makes the final call +``` + +This catches issues that a single model might miss — different models have different blind spots. + +**Example prompt for a review agent:** +``` +Review this code change for correctness, security, and adherence to +our project conventions. The spec says [X]. The change should [Y]. +Flag any issues as Critical, Required, Optional, or Nit. +``` + +## Dead Code Hygiene + +After any refactoring or implementation change, check for orphaned code: + +1. Identify code that is now unreachable or unused +2. List it explicitly +3. **Ask before deleting:** "Should I remove these now-unused elements: [list]?" + +Don't leave dead code lying around — it confuses future readers and agents. But don't silently delete things you're not sure about. When in doubt, ask. + +``` +DEAD CODE IDENTIFIED: +- formatLegacyDate() in src/utils/date.ts — replaced by formatDate() +- OldTaskCard component in src/components/ — replaced by TaskCard +- LEGACY_API_URL constant in src/config.ts — no remaining references +→ Safe to remove these? +``` + +## Review Speed + +Slow reviews block entire teams. The cost of context-switching to review is less than the waiting cost imposed on others. + +- **Respond within one business day** — this is the maximum, not the target +- **Ideal cadence:** Respond shortly after a review request arrives, unless deep in focused coding. A typical change should complete multiple review rounds in a single day +- **Prioritize fast individual responses** over quick final approval. Quick feedback reduces frustration even if multiple rounds are needed +- **Large changes:** Ask the author to split them rather than reviewing one massive changeset + +## Handling Disagreements + +When resolving review disputes, apply this hierarchy: + +1. **Technical facts and data** override opinions and preferences +2. **Style guides** are the absolute authority on style matters +3. **Software design** must be evaluated on engineering principles, not personal preference +4. **Codebase consistency** is acceptable if it doesn't degrade overall health + +**Don't accept "I'll clean it up later."** Experience shows deferred cleanup rarely happens. Require cleanup before submission unless it's a genuine emergency. If surrounding issues can't be addressed in this change, require filing a bug with self-assignment. + +## Honesty in Review + +When reviewing code — whether written by you, another agent, or a human: + +- **Don't rubber-stamp.** "LGTM" without evidence of review helps no one. +- **Don't soften real issues.** "This might be a minor concern" when it's a bug that will hit production is dishonest. +- **Quantify problems when possible.** "This N+1 query will add ~50ms per item in the list" is better than "this could be slow." +- **Push back on approaches with clear problems.** Sycophancy is a failure mode in reviews. If the implementation has issues, say so directly and propose alternatives. +- **Accept override gracefully.** If the author has full context and disagrees, defer to their judgment. Comment on code, not people — reframe personal critiques to focus on the code itself. + +## Dependency Discipline + +Part of code review is dependency review: + +**Before adding any dependency:** +1. Does the existing stack solve this? (Often it does.) +2. How large is the dependency? (Check bundle impact.) +3. Is it actively maintained? (Check last commit, open issues.) +4. Does it have known vulnerabilities? (`npm audit`) +5. What's the license? (Must be compatible with the project.) + +**Rule:** Prefer standard library and existing utilities over new dependencies. Every dependency is a liability. + +## The Review Checklist + +```markdown +## Review: [PR/Change title] + +### Context +- [ ] I understand what this change does and why + +### Correctness +- [ ] Change matches spec/task requirements +- [ ] Edge cases handled +- [ ] Error paths handled +- [ ] Tests cover the change adequately + +### Readability +- [ ] Names are clear and consistent +- [ ] Logic is straightforward +- [ ] No unnecessary complexity + +### Architecture +- [ ] 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 +- [ ] Input validated at boundaries +- [ ] No injection vulnerabilities +- [ ] Auth checks in place +- [ ] External data sources treated as untrusted + +### Performance +- [ ] No N+1 patterns +- [ ] No unbounded operations +- [ ] Pagination on list endpoints + +### Verification +- [ ] Tests pass +- [ ] Build succeeds +- [ ] Manual verification done (if applicable) + +### Verdict +- [ ] **Approve** — Ready to merge +- [ ] **Request changes** — Issues must be addressed +``` +## See Also + +- For detailed security review guidance, see `references/security-checklist.md` +- For performance review checks, see `references/performance-checklist.md` + +## Common Rationalizations + +| Rationalization | Reality | +|---|---| +| "It works, that's good enough" | Working code that's unreadable, insecure, or architecturally wrong creates debt that compounds. | +| "I wrote it, so I know it's correct" | Authors are blind to their own assumptions. Every change benefits from another set of eyes. | +| "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 + +- PRs merged without any review +- Review that only checks if tests pass (ignoring other axes) +- "LGTM" without evidence of actual review +- Security-sensitive changes without security-focused review +- Large PRs that are "too big to review properly" (split them) +- 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 + +After review is complete: + +- [ ] All Critical issues are resolved +- [ ] All Required (no-prefix) changes are resolved or explicitly deferred with justification +- [ ] 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. diff --git a/.github/skills/addyosmani-code-simplification/SKILL.md b/.github/skills/addyosmani-code-simplification/SKILL.md new file mode 100644 index 00000000..3f31ca73 --- /dev/null +++ b/.github/skills/addyosmani-code-simplification/SKILL.md @@ -0,0 +1,331 @@ +--- +name: addyosmani-code-simplification +description: Simplifies code for clarity. Use when refactoring code for clarity without changing behavior. Use when code works but is harder to read, maintain, or extend than it should be. Use when reviewing code that has accumulated unnecessary complexity. +--- + +# Code Simplification + +> Inspired by the [Claude Code Simplifier plugin](https://github.com/anthropics/claude-plugins-official/blob/main/plugins/code-simplifier/agents/code-simplifier.md). Adapted here as a model-agnostic, process-driven skill for any AI coding agent. + +## Overview + +Simplify code by reducing complexity while preserving exact behavior. The goal is not fewer lines — it's code that is easier to read, understand, modify, and debug. Every simplification must pass a simple test: "Would a new team member understand this faster than the original?" + +## When to Use + +- After a feature is working and tests pass, but the implementation feels heavier than it needs to be +- During code review when readability or complexity issues are flagged +- When you encounter deeply nested logic, long functions, or unclear names +- When refactoring code written under time pressure +- When consolidating related logic scattered across files +- After merging changes that introduced duplication or inconsistency + +**When NOT to use:** + +- Code is already clean and readable — don't simplify for the sake of it +- You don't understand what the code does yet — comprehend before you simplify +- The code is performance-critical and the "simpler" version would be measurably slower +- You're about to rewrite the module entirely — simplifying throwaway code wastes effort + +## The Five Principles + +### 1. Preserve Behavior Exactly + +Don't change what the code does — only how it expresses it. All inputs, outputs, side effects, error behavior, and edge cases must remain identical. If you're not sure a simplification preserves behavior, don't make it. + +``` +ASK BEFORE EVERY CHANGE: +→ Does this produce the same output for every input? +→ Does this maintain the same error behavior? +→ Does this preserve the same side effects and ordering? +→ Do all existing tests still pass without modification? +``` + +### 2. Follow Project Conventions + +Simplification means making code more consistent with the codebase, not imposing external preferences. Before simplifying: + +``` +1. Read CLAUDE.md / project conventions +2. Study how neighboring code handles similar patterns +3. Match the project's style for: + - Import ordering and module system + - Function declaration style + - Naming conventions + - Error handling patterns + - Type annotation depth +``` + +Simplification that breaks project consistency is not simplification — it's churn. + +### 3. Prefer Clarity Over Cleverness + +Explicit code is better than compact code when the compact version requires a mental pause to parse. + +```typescript +// UNCLEAR: Dense ternary chain +const label = isNew ? 'New' : isUpdated ? 'Updated' : isArchived ? 'Archived' : 'Active'; + +// CLEAR: Readable mapping +function getStatusLabel(item: Item): string { + if (item.isNew) return 'New'; + if (item.isUpdated) return 'Updated'; + if (item.isArchived) return 'Archived'; + return 'Active'; +} +``` + +```typescript +// UNCLEAR: Chained reduces with inline logic +const result = items.reduce((acc, item) => ({ + ...acc, + [item.id]: { ...acc[item.id], count: (acc[item.id]?.count ?? 0) + 1 } +}), {}); + +// CLEAR: Named intermediate step +const countById = new Map(); +for (const item of items) { + countById.set(item.id, (countById.get(item.id) ?? 0) + 1); +} +``` + +### 4. Maintain Balance + +Simplification has a failure mode: over-simplification. Watch for these traps: + +- **Inlining too aggressively** — removing a helper that gave a concept a name makes the call site harder to read +- **Combining unrelated logic** — two simple functions merged into one complex function is not simpler +- **Removing "unnecessary" abstraction** — some abstractions exist for extensibility or testability, not complexity +- **Optimizing for line count** — fewer lines is not the goal; easier comprehension is + +### 5. Scope to What Changed + +Default to simplifying recently modified code. Avoid drive-by refactors of unrelated code unless explicitly asked to broaden scope. Unscoped simplification creates noise in diffs and risks unintended regressions. + +## The Simplification Process + +### Step 1: Understand Before Touching (Chesterton's Fence) + +Before changing or removing anything, understand why it exists. This is Chesterton's Fence: if you see a fence across a road and don't understand why it's there, don't tear it down. First understand the reason, then decide if the reason still applies. + +``` +BEFORE SIMPLIFYING, ANSWER: +- What is this code's responsibility? +- What calls it? What does it call? +- What are the edge cases and error paths? +- Are there tests that define the expected behavior? +- Why might it have been written this way? (Performance? Platform constraint? Historical reason?) +- Check git blame: what was the original context for this code? +``` + +If you can't answer these, you're not ready to simplify. Read more context first. + +### Step 2: Identify Simplification Opportunities + +Scan for these patterns — each one is a concrete signal, not a vague smell: + +**Structural complexity:** + +| Pattern | Signal | Simplification | +|---------|--------|----------------| +| Deep nesting (3+ levels) | Hard to follow control flow | Extract conditions into guard clauses or helper functions | +| Long functions (50+ lines) | Multiple responsibilities | Split into focused functions with descriptive names | +| Nested ternaries | Requires mental stack to parse | Replace with if/else chains, switch, or lookup objects | +| Boolean parameter flags | `doThing(true, false, true)` | Replace with options objects or separate functions | +| Repeated conditionals | Same `if` check in multiple places | Extract to a well-named predicate function | + +**Naming and readability:** + +| Pattern | Signal | Simplification | +|---------|--------|----------------| +| Generic names | `data`, `result`, `temp`, `val`, `item` | Rename to describe the content: `userProfile`, `validationErrors` | +| Abbreviated names | `usr`, `cfg`, `btn`, `evt` | Use full words unless the abbreviation is universal (`id`, `url`, `api`) | +| Misleading names | Function named `get` that also mutates state | Rename to reflect actual behavior | +| Comments explaining "what" | `// increment counter` above `count++` | Delete the comment — the code is clear enough | +| Comments explaining "why" | `// Retry because the API is flaky under load` | Keep these — they carry intent the code can't express | + +**Redundancy:** + +| Pattern | Signal | Simplification | +|---------|--------|----------------| +| Duplicated logic | Same 5+ lines in multiple places | Extract to a shared function | +| Dead code | Unreachable branches, unused variables, commented-out blocks | Remove (after confirming it's truly dead) | +| Unnecessary abstractions | Wrapper that adds no value | Inline the wrapper, call the underlying function directly | +| Over-engineered patterns | Factory-for-a-factory, strategy-with-one-strategy | Replace with the simple direct approach | +| Redundant type assertions | Casting to a type that's already inferred | Remove the assertion | + +### Step 3: Apply Changes Incrementally + +Make one simplification at a time. Run tests after each change. **Submit refactoring changes separately from feature or bug fix changes.** A PR that refactors and adds a feature is two PRs — split them. + +``` +FOR EACH SIMPLIFICATION: +1. Make the change +2. Run the test suite +3. If tests pass → commit (or continue to next simplification) +4. If tests fail → revert and reconsider +``` + +Avoid batching multiple simplifications into a single untested change. If something breaks, you need to know which simplification caused it. + +**The Rule of 500:** If a refactoring would touch more than 500 lines, invest in automation (codemods, sed scripts, AST transforms) rather than making the changes by hand. Manual edits at that scale are error-prone and exhausting to review. + +### Step 4: Verify the Result + +After all simplifications, step back and evaluate the whole: + +``` +COMPARE BEFORE AND AFTER: +- Is the simplified version genuinely easier to understand? +- Did you introduce any new patterns inconsistent with the codebase? +- Is the diff clean and reviewable? +- Would a teammate approve this change? +``` + +If the "simplified" version is harder to understand or review, revert. Not every simplification attempt succeeds. + +## Language-Specific Guidance + +### TypeScript / JavaScript + +```typescript +// SIMPLIFY: Unnecessary async wrapper +// Before +async function getUser(id: string): Promise { + return await userService.findById(id); +} +// After +function getUser(id: string): Promise { + return userService.findById(id); +} + +// SIMPLIFY: Verbose conditional assignment +// Before +let displayName: string; +if (user.nickname) { + displayName = user.nickname; +} else { + displayName = user.fullName; +} +// After +const displayName = user.nickname || user.fullName; + +// SIMPLIFY: Manual array building +// Before +const activeUsers: User[] = []; +for (const user of users) { + if (user.isActive) { + activeUsers.push(user); + } +} +// After +const activeUsers = users.filter((user) => user.isActive); + +// SIMPLIFY: Redundant boolean return +// Before +function isValid(input: string): boolean { + if (input.length > 0 && input.length < 100) { + return true; + } + return false; +} +// After +function isValid(input: string): boolean { + return input.length > 0 && input.length < 100; +} +``` + +### Python + +```python +# SIMPLIFY: Verbose dictionary building +# Before +result = {} +for item in items: + result[item.id] = item.name +# After +result = {item.id: item.name for item in items} + +# SIMPLIFY: Nested conditionals with early return +# Before +def process(data): + if data is not None: + if data.is_valid(): + if data.has_permission(): + return do_work(data) + else: + raise PermissionError("No permission") + else: + raise ValueError("Invalid data") + else: + raise TypeError("Data is None") +# After +def process(data): + if data is None: + raise TypeError("Data is None") + if not data.is_valid(): + raise ValueError("Invalid data") + if not data.has_permission(): + raise PermissionError("No permission") + return do_work(data) +``` + +### React / JSX + +```tsx +// SIMPLIFY: Verbose conditional rendering +// Before +function UserBadge({ user }: Props) { + if (user.isAdmin) { + return Admin; + } else { + return User; + } +} +// After +function UserBadge({ user }: Props) { + const variant = user.isAdmin ? 'admin' : 'default'; + const label = user.isAdmin ? 'Admin' : 'User'; + return {label}; +} + +// SIMPLIFY: Prop drilling through intermediate components +// Before — consider whether context or composition solves this better. +// This is a judgment call — flag it, don't auto-refactor. +``` + +## Common Rationalizations + +| Rationalization | Reality | +|---|---| +| "It's working, no need to touch it" | Working code that's hard to read will be hard to fix when it breaks. Simplifying now saves time on every future change. | +| "Fewer lines is always simpler" | A 1-line nested ternary is not simpler than a 5-line if/else. Simplicity is about comprehension speed, not line count. | +| "I'll just quickly simplify this unrelated code too" | Unscoped simplification creates noisy diffs and risks regressions in code you didn't intend to change. Stay focused. | +| "The types make it self-documenting" | Types document structure, not intent. A well-named function explains *why* better than a type signature explains *what*. | +| "This abstraction might be useful later" | Don't preserve speculative abstractions. If it's not used now, it's complexity without value. Remove it and re-add when needed. | +| "The original author must have had a reason" | Maybe. Check git blame — apply Chesterton's Fence. But accumulated complexity often has no reason; it's just the residue of iteration under pressure. | +| "I'll refactor while adding this feature" | Separate refactoring from feature work. Mixed changes are harder to review, revert, and understand in history. | + +## Red Flags + +- Simplification that requires modifying tests to pass (you likely changed behavior) +- "Simplified" code that is longer and harder to follow than the original +- Renaming things to match your preferences rather than project conventions +- Removing error handling because "it makes the code cleaner" +- Simplifying code you don't fully understand +- Batching many simplifications into one large, hard-to-review commit +- Refactoring code outside the scope of the current task without being asked + +## Verification + +After completing a simplification pass: + +- [ ] All existing tests pass without modification +- [ ] Build succeeds with no new warnings +- [ ] Linter/formatter passes (no style regressions) +- [ ] Each simplification is a reviewable, incremental change +- [ ] The diff is clean — no unrelated changes mixed in +- [ ] Simplified code follows project conventions (checked against CLAUDE.md or equivalent) +- [ ] No error handling was removed or weakened +- [ ] No dead code was left behind (unused imports, unreachable branches) +- [ ] A teammate or review agent would approve the change as a net improvement diff --git a/.github/skills/awesome-copilot-codeql/references/workflow-configuration.md b/.github/skills/awesome-copilot-codeql/references/workflow-configuration.md index 6700a00f..5984cb90 100644 --- a/.github/skills/awesome-copilot-codeql/references/workflow-configuration.md +++ b/.github/skills/awesome-copilot-codeql/references/workflow-configuration.md @@ -263,7 +263,7 @@ The `category` value appears as `.automationDetails.id` in the SARIF output Create `.github/codeql/codeql-config.yml` for advanced path and query configuration: ```yaml -name: "CodeQL Configuration" +name: awesome-copilot-codeql # Directories to scan paths: diff --git a/.github/skills/awesome-copilot-dependabot/SKILL.md b/.github/skills/awesome-copilot-dependabot/SKILL.md index 1985d6dd..40e2bf2c 100644 --- a/.github/skills/awesome-copilot-dependabot/SKILL.md +++ b/.github/skills/awesome-copilot-dependabot/SKILL.md @@ -1,14 +1,6 @@ --- name: awesome-copilot-dependabot -description: >- - Comprehensive guide for configuring and managing GitHub Dependabot. Use this skill when - users ask about creating or optimizing dependabot.yml files, managing Dependabot pull requests, - configuring dependency update strategies, setting up grouped updates, monorepo patterns, - multi-ecosystem groups, security update configuration, auto-triage rules, or any GitHub - Advanced Security (GHAS) supply chain security topic related to Dependabot. For pre-commit - dependency vulnerability scanning in AI coding agents via the GitHub MCP Server, this skill - references the Advanced Security plugin (`advanced-security@copilot-plugins`). Use this skill - when an agent needs to scan dependencies for known vulnerabilities before committing. +description: Use for Dependabot setup, grouping, update policy, and GHAS dependency update guidance. --- # Dependabot Configuration & Management diff --git a/.github/skills/awesome-copilot-secret-scanning/SKILL.md b/.github/skills/awesome-copilot-secret-scanning/SKILL.md index 184c0f74..3f649d7c 100644 --- a/.github/skills/awesome-copilot-secret-scanning/SKILL.md +++ b/.github/skills/awesome-copilot-secret-scanning/SKILL.md @@ -1,6 +1,6 @@ --- name: awesome-copilot-secret-scanning -description: 'Guide for configuring and managing GitHub secret scanning, push protection, custom patterns, and secret alert remediation. For pre-commit secret scanning in AI coding agents via the GitHub MCP Server, this skill references the Advanced Security plugin (`advanced-security@copilot-plugins`). Use this skill when enabling secret scanning, setting up push protection, defining custom patterns, triaging alerts, resolving blocked pushes, or when an agent needs to scan code for secrets before committing.' +description: Use for GitHub secret scanning, push protection, custom patterns, and secret alert remediation. --- # Secret Scanning diff --git a/.github/skills/awesome-copilot-security-review/SKILL.md b/.github/skills/awesome-copilot-security-review/SKILL.md new file mode 100644 index 00000000..8f5f7539 --- /dev/null +++ b/.github/skills/awesome-copilot-security-review/SKILL.md @@ -0,0 +1,168 @@ +--- +name: awesome-copilot-security-review +description: Use for security-focused code review covering injections, secrets, auth, access control, and dependency risk. +--- + +# Security Review + +An AI-powered security scanner that reasons about your codebase the way a human security +researcher would — tracing data flows, understanding component interactions, and catching +vulnerabilities that pattern-matching tools miss. + +## When to Use This Skill + +Use this skill when the request involves: + +- Scanning a codebase or file for security vulnerabilities +- Running a security review or vulnerability check +- Checking for SQL injection, XSS, command injection, or other injection flaws +- Finding exposed API keys, hardcoded secrets, or credentials in code +- Auditing dependencies for known CVEs +- Reviewing authentication, authorization, or access control logic +- Detecting insecure cryptography or weak randomness +- Performing a data flow analysis to trace user input to dangerous sinks +- Any request phrasing like "is my code secure?", "scan this file", or "check my repo for vulnerabilities" +- Running `/security-review` or `/security-review ` + +## How This Skill Works + +Unlike traditional static analysis tools that match patterns, this skill: +1. **Reads code like a security researcher** — understanding context, intent, and data flow +2. **Traces across files** — following how user input moves through your application +3. **Self-verifies findings** — re-examines each result to filter false positives +4. **Assigns severity ratings** — CRITICAL / HIGH / MEDIUM / LOW / INFO +5. **Proposes targeted patches** — every finding includes a concrete fix +6. **Requires human approval** — nothing is auto-applied; you always review first + +## Execution Workflow + +Follow these steps **in order** every time: + +### Step 1 — Scope Resolution +Determine what to scan: +- If a path was provided (`/security-review src/auth/`), scan only that scope +- If no path given, scan the **entire project** starting from the root +- Identify the language(s) and framework(s) in use (check package.json, requirements.txt, + go.mod, Cargo.toml, pom.xml, Gemfile, composer.json, etc.) +- Read `references/language-patterns.md` to load language-specific vulnerability patterns + +### Step 2 — Dependency Audit +Before scanning source code, audit dependencies first (fast wins): +- **Node.js**: Check `package.json` + `package-lock.json` for known vulnerable packages +- **Python**: Check `requirements.txt` / `pyproject.toml` / `Pipfile` +- **Java**: Check `pom.xml` / `build.gradle` +- **Ruby**: Check `Gemfile.lock` +- **Rust**: Check `Cargo.toml` +- **Go**: Check `go.sum` +- Flag packages with known CVEs, deprecated crypto libs, or suspiciously old pinned versions +- Read `references/vulnerable-packages.md` for a curated watchlist + +### Step 3 — Secrets & Exposure Scan +Scan ALL files (including config, env, CI/CD, Dockerfiles, IaC) for: +- Hardcoded API keys, tokens, passwords, private keys +- `.env` files accidentally committed +- Secrets in comments or debug logs +- Cloud credentials (AWS, GCP, Azure, Stripe, Twilio, etc.) +- Database connection strings with credentials embedded +- Read `references/secret-patterns.md` for regex patterns and entropy heuristics to apply + +### Step 4 — Vulnerability Deep Scan +This is the core scan. Reason about the code — don't just pattern-match. +Read `references/vuln-categories.md` for full details on each category. + +**Injection Flaws** +- SQL Injection: raw queries with string interpolation, ORM misuse, second-order SQLi +- XSS: unescaped output, dangerouslySetInnerHTML, innerHTML, template injection +- Command Injection: exec/spawn/system with user input +- LDAP, XPath, Header, Log injection + +**Authentication & Access Control** +- Missing authentication on sensitive endpoints +- Broken object-level authorization (BOLA/IDOR) +- JWT weaknesses (alg:none, weak secrets, no expiry validation) +- Session fixation, missing CSRF protection +- Privilege escalation paths +- Mass assignment / parameter pollution + +**Data Handling** +- Sensitive data in logs, error messages, or API responses +- Missing encryption at rest or in transit +- Insecure deserialization +- Path traversal / directory traversal +- XXE (XML External Entity) processing +- SSRF (Server-Side Request Forgery) + +**Cryptography** +- Use of MD5, SHA1, DES for security purposes +- Hardcoded IVs or salts +- Weak random number generation (Math.random() for tokens) +- Missing TLS certificate validation + +**Business Logic** +- Race conditions (TOCTOU) +- Integer overflow in financial calculations +- Missing rate limiting on sensitive endpoints +- Predictable resource identifiers + +### Step 5 — Cross-File Data Flow Analysis +After the per-file scan, perform a **holistic review**: +- Trace user-controlled input from entry points (HTTP params, headers, body, file uploads) + all the way to sinks (DB queries, exec calls, HTML output, file writes) +- Identify vulnerabilities that only appear when looking at multiple files together +- Check for insecure trust boundaries between services or modules + +### Step 6 — Self-Verification Pass +For EACH finding: +1. Re-read the relevant code with fresh eyes +2. Ask: "Is this actually exploitable, or is there sanitization I missed?" +3. Check if a framework or middleware already handles this upstream +4. Downgrade or discard findings that aren't genuine vulnerabilities +5. Assign final severity: CRITICAL / HIGH / MEDIUM / LOW / INFO + +### Step 7 — Generate Security Report +Output the full report in the format defined in `references/report-format.md`. + +### Step 8 — Propose Patches +For every CRITICAL and HIGH finding, generate a concrete patch: +- Show the vulnerable code (before) +- Show the fixed code (after) +- Explain what changed and why +- Preserve the original code style, variable names, and structure +- Add a comment explaining the fix inline + +Explicitly state: **"Review each patch before applying. Nothing has been changed yet."** + +## Severity Guide + +| Severity | Meaning | Example | +|----------|---------|---------| +| 🔴 CRITICAL | Immediate exploitation risk, data breach likely | SQLi, RCE, auth bypass | +| 🟠 HIGH | Serious vulnerability, exploit path exists | XSS, IDOR, hardcoded secrets | +| 🟡 MEDIUM | Exploitable with conditions or chaining | CSRF, open redirect, weak crypto | +| 🔵 LOW | Best practice violation, low direct risk | Verbose errors, missing headers | +| ⚪ INFO | Observation worth noting, not a vulnerability | Outdated dependency (no CVE) | + +## Output Rules + +- **Always** produce a findings summary table first (counts by severity) +- **Never** auto-apply any patch — present patches for human review only +- **Always** include a confidence rating per finding (High / Medium / Low) +- **Group findings** by category, not by file +- **Be specific** — include file path, line number, and the exact vulnerable code snippet +- **Explain the risk** in plain English — what could an attacker do with this? +- If the codebase is clean, say so clearly: "No vulnerabilities found" with what was scanned + +## Reference Files + +For detailed detection guidance, load the following reference files as needed: + +- `references/vuln-categories.md` — Deep reference for every vulnerability category with detection signals, safe patterns, and escalation checkers + - Search patterns: `SQL injection`, `XSS`, `command injection`, `SSRF`, `BOLA`, `IDOR`, `JWT`, `CSRF`, `secrets`, `cryptography`, `race condition`, `path traversal` +- `references/secret-patterns.md` — Regex patterns, entropy-based detection, and CI/CD secret risks + - Search patterns: `API key`, `token`, `private key`, `connection string`, `entropy`, `.env`, `GitHub Actions`, `Docker`, `Terraform` +- `references/language-patterns.md` — Framework-specific vulnerability patterns for JavaScript, Python, Java, PHP, Go, Ruby, and Rust + - Search patterns: `Express`, `React`, `Next.js`, `Django`, `Flask`, `FastAPI`, `Spring Boot`, `PHP`, `Go`, `Rails`, `Rust` +- `references/vulnerable-packages.md` — Curated CVE watchlist for npm, pip, Maven, Rubygems, Cargo, and Go modules + - Search patterns: `lodash`, `axios`, `jsonwebtoken`, `Pillow`, `log4j`, `nokogiri`, `CVE` +- `references/report-format.md` — Structured output template for security reports with finding cards, dependency audit, secrets scan, and patch proposal formatting + - Search patterns: `report`, `format`, `template`, `finding`, `patch`, `summary`, `confidence` diff --git a/.github/skills/awesome-copilot-security-review/references/language-patterns.md b/.github/skills/awesome-copilot-security-review/references/language-patterns.md new file mode 100644 index 00000000..d6af534a --- /dev/null +++ b/.github/skills/awesome-copilot-security-review/references/language-patterns.md @@ -0,0 +1,221 @@ +# Language-Specific Vulnerability Patterns + +Load the relevant section during Step 1 (Scope Resolution) after identifying languages. + +--- + +## JavaScript / TypeScript (Node.js, React, Next.js, Express) + +### Critical APIs/calls to flag +```js +eval() // arbitrary code execution +Function('return ...') // same as eval +child_process.exec() // command injection if user input reaches it +fs.readFile // path traversal if user controls path +fs.writeFile // path traversal if user controls path +``` + +### Express.js specific +```js +// Missing helmet (security headers) +const app = express() +// Should have: app.use(helmet()) + +// Body size limits missing (DoS) +app.use(express.json()) +// Should have: app.use(express.json({ limit: '10kb' })) + +// CORS misconfiguration +app.use(cors({ origin: '*' })) // too permissive +app.use(cors({ origin: req.headers.origin })) // reflects any origin + +// Trust proxy without validation +app.set('trust proxy', true) // only safe behind known proxy +``` + +### React specific +```jsx +
// XSS +link // javascript: URL injection +``` + +### Next.js specific +```js +// Server Actions without auth +export async function deleteUser(id) { // missing: auth check + await db.users.delete(id) +} + +// API Routes missing method validation +export default function handler(req, res) { + // Should check: if (req.method !== 'POST') return res.status(405) + doSensitiveAction() +} +``` + +--- + +## Python (Django, Flask, FastAPI) + +### Django specific +```python +# Raw SQL +User.objects.raw(f"SELECT * FROM users WHERE name = '{name}'") # SQLi + +# Missing CSRF +@csrf_exempt # Only OK for APIs with token auth + +# Debug mode in production +DEBUG = True # in settings.py — exposes stack traces + +# SECRET_KEY +SECRET_KEY = 'django-insecure-...' # must be changed for production + +# ALLOWED_HOSTS +ALLOWED_HOSTS = ['*'] # too permissive +``` + +### Flask specific +```python +# Debug mode +app.run(debug=True) # never in production + +# Secret key +app.secret_key = 'dev' # weak + +# eval/exec with user input +eval(request.args.get('expr')) + +# render_template_string with user input (SSTI) +render_template_string(f"Hello {name}") # Server-Side Template Injection +``` + +### FastAPI specific +```python +# Missing auth dependency +@app.delete("/users/{user_id}") # No Depends(get_current_user) +async def delete_user(user_id: int): + ... + +# Arbitrary file read +@app.get("/files/{filename}") +async def read_file(filename: str): + return FileResponse(f"uploads/{filename}") # path traversal +``` + +--- + +## Java (Spring Boot) + +### Spring Boot specific +```java +// SQL Injection +String query = "SELECT * FROM users WHERE name = '" + name + "'"; +jdbcTemplate.query(query, ...); + +// XXE +DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); +// Missing: dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true) + +// Deserialization +ObjectInputStream ois = new ObjectInputStream(inputStream); +Object obj = ois.readObject(); // only safe with allowlist + +// Spring Security — permitAll on sensitive endpoint +.antMatchers("/admin/**").permitAll() + +// Actuator endpoints exposed +management.endpoints.web.exposure.include=* # in application.properties +``` + +--- + +## PHP + +```php +// Direct user input in queries +$result = mysql_query("SELECT * FROM users WHERE id = " . $_GET['id']); + +// File inclusion +include($_GET['page'] . ".php"); // local/remote file inclusion + +// eval +eval($_POST['code']); + +// extract() with user input +extract($_POST); // overwrites any variable + +// Loose comparison +if ($password == "admin") {} // use === instead + +// Unserialize +unserialize($_COOKIE['data']); // remote code execution +``` + +--- + +## Go + +```go +// Command injection +exec.Command("sh", "-c", userInput) + +// SQL injection +db.Query("SELECT * FROM users WHERE name = '" + name + "'") + +// Path traversal +filePath := filepath.Join("/uploads/", userInput) // sanitize userInput first + +// Insecure TLS +http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: true}} + +// Goroutine leak / missing context cancellation +go func() { + // No done channel or context + for { ... } +}() +``` + +--- + +## Ruby on Rails + +```ruby +# SQL injection (safe alternatives use placeholders) +User.where("name = '#{params[:name]}'") # VULNERABLE +User.where("name = ?", params[:name]) # SAFE + +# Mass assignment without strong params +@user.update(params[:user]) # should be params.require(:user).permit(...) + +# eval / send with user input +eval(params[:code]) +send(params[:method]) # arbitrary method call + +# Redirect to user-supplied URL (open redirect) +redirect_to params[:url] + +# YAML.load (allows arbitrary object creation) +YAML.load(user_input) # use YAML.safe_load instead +``` + +--- + +## Rust + +```rust +// Unsafe blocks — flag for manual review +unsafe { + // Reason for unsafety should be documented +} + +// Integer overflow (debug builds panic, release silently wraps) +let result = a + b; // use checked_add/saturating_add for financial math + +// Unwrap/expect in production code (panics on None/Err) +let value = option.unwrap(); // prefer ? or match + +// Deserializing arbitrary types +serde_json::from_str::(&user_input) // generally safe +// But: bincode::deserialize from untrusted input — can be exploited +``` diff --git a/.github/skills/awesome-copilot-security-review/references/report-format.md b/.github/skills/awesome-copilot-security-review/references/report-format.md new file mode 100644 index 00000000..55fe5717 --- /dev/null +++ b/.github/skills/awesome-copilot-security-review/references/report-format.md @@ -0,0 +1,194 @@ +# Security Report Format + +Use this template for all `/security-review` output. Generated during Step 7. + +--- + +## Report Structure + +### Header +``` +╔══════════════════════════════════════════════════════════╗ +║ 🔐 SECURITY REVIEW REPORT ║ +║ Generated by: /security-review skill ║ +╚══════════════════════════════════════════════════════════╝ + +Project: +Scan Date: +Scope: +Languages Detected: +Frameworks Detected: +``` + +--- + +### Executive Summary Table + +Always show this first — at a glance overview: + +``` +┌────────────────────────────────────────────────┐ +│ FINDINGS SUMMARY │ +├──────────────┬──────────────────────────────── ┤ +│ 🔴 CRITICAL │ findings │ +│ 🟠 HIGH │ findings │ +│ 🟡 MEDIUM │ findings │ +│ 🔵 LOW │ findings │ +│ ⚪ INFO │ findings │ +├──────────────┼─────────────────────────────────┤ +│ TOTAL │ findings │ +└──────────────┴─────────────────────────────────┘ + +Dependency Audit: vulnerable packages found +Secrets Scan: exposed credentials found +``` + +--- + +### Findings (Grouped by Category) + +For EACH finding, use this card format: + +``` +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +[SEVERITY EMOJI] [SEVERITY] — [VULNERABILITY TYPE] +Confidence: HIGH / MEDIUM / LOW +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + +📍 Location: src/routes/users.js, Line 47 + +🔍 Vulnerable Code: + const query = `SELECT * FROM users WHERE id = ${req.params.id}`; + db.execute(query); + +⚠️ Risk: + An attacker can manipulate the `id` parameter to execute arbitrary + SQL commands, potentially dumping the entire database, bypassing + authentication, or deleting data. + + Example attack: GET /users/1 OR 1=1-- + +✅ Recommended Fix: + Use parameterized queries: + + const query = 'SELECT * FROM users WHERE id = ?'; + db.execute(query, [req.params.id]); + +📚 Reference: OWASP A03:2021 – Injection +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +``` + +--- + +### Dependency Audit Section + +``` +📦 DEPENDENCY AUDIT +══════════════════ + +🟠 HIGH — lodash@4.17.20 (package.json) + CVE-2021-23337: Prototype pollution via zipObjectDeep() + Fix: npm install lodash@4.17.21 + +🟡 MEDIUM — axios@0.27.2 (package.json) + CVE-2023-45857: CSRF via withCredentials + Fix: npm install axios@1.6.0 + +⚪ INFO — express@4.18.2 + No known CVEs. Current version is 4.19.2 — consider updating. +``` + +--- + +### Secrets Scan Section + +``` +🔑 SECRETS & EXPOSURE SCAN +═══════════════════════════ + +🔴 CRITICAL — Hardcoded API Key + File: src/config/database.js, Line 12 + + Found: STRIPE_SECRET_KEY = "sk_live_FAKE_KEY_..." + + Action Required: + 1. Rotate this key IMMEDIATELY at https://dashboard.stripe.com + 2. Remove from source code + 3. Add to .env file and load via process.env.STRIPE_SECRET_KEY + 4. Add .env to .gitignore + 5. Audit git history — key may be in previous commits: + git log --all -p | grep "sk_live_" + Use git-filter-repo or BFG to purge from history if found. +``` + +--- + +### Patch Proposals Section + +Only include for CRITICAL and HIGH findings: + +```` +🛠️ PATCH PROPOSALS +══════════════════ +⚠️ REVIEW EACH PATCH BEFORE APPLYING — Nothing has been changed yet. + +───────────────────────────────────────────── +Patch 1/3: SQL Injection in src/routes/users.js +───────────────────────────────────────────── + +BEFORE (vulnerable): +```js +// Line 47 +const query = `SELECT * FROM users WHERE id = ${req.params.id}`; +db.execute(query); +``` + +AFTER (fixed): +```js +// Line 47 — Fixed: Use parameterized query to prevent SQL injection +const query = 'SELECT * FROM users WHERE id = ?'; +db.execute(query, [req.params.id]); +``` + +Apply this patch? (Review first — AI-generated patches may need adjustment) +───────────────────────────────────────────── +```` + +--- + +### Footer + +``` +══════════════════════════════════════════════════════════ + +📋 SCAN COVERAGE + Files scanned: + Lines analyzed: + Scan duration: