From 09dfafadc5e65390339b85e57059387d17ba1791 Mon Sep 17 00:00:00 2001 From: Patrick Hooper <25905129+VTHoop@users.noreply.github.com> Date: Sat, 31 Oct 2026 12:22:33 -0400 Subject: [PATCH 1/4] HEA-21: failing tests for the plugin, generic skills and unattended mode Co-Authored-By: Claude Opus 5.5 --- tests/generic.test.mjs | 109 +++++++++++++++++++++ tests/lib.mjs | 45 +++++++++ tests/manifest.test.mjs | 78 +++++++++++++++ tests/owner.test.mjs | 55 +++++++++++ tests/unattended.test.mjs | 194 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 481 insertions(+) create mode 100644 tests/generic.test.mjs create mode 100644 tests/lib.mjs create mode 100644 tests/manifest.test.mjs create mode 100644 tests/owner.test.mjs create mode 100644 tests/unattended.test.mjs diff --git a/tests/generic.test.mjs b/tests/generic.test.mjs new file mode 100644 index 0000000..60c1cdf --- /dev/null +++ b/tests/generic.test.mjs @@ -0,0 +1,109 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { AGENTS, pluginFiles, skill } from "./lib.mjs"; + +const assertAbsent = (pattern, label) => { + const files = pluginFiles(); + assert.ok(files.length > 0, "the plugin has no files yet"); + for (const [path, text] of files) { + assert.doesNotMatch(text, pattern, `${label} in ${path}`); + } +}; + +describe("no dependency on the machine", () => { + it("does not read from ~/.claude or anyone's home directory", () => { + assertAbsent(/~\/\.claude|\/Users\/|\$HOME/, "user-level path"); + }); + + it("replaces repo-resolution with the current repo", () => { + assertAbsent(/repo-resolution/, "repo-resolution"); + assert.match(skill("pr-review"), /current repo/i); + }); +}); + +describe("no project details", () => { + it("names no project tooling or service", () => { + assertAbsent(/\bpnpm\b|supabase|\bvercel\b|\bresend\b/i, "project tool"); + }); + + it("names no project folders", () => { + assertAbsent(/apps\/web|packages\/core|`server\/`|`api\/`|docs\/adr/, "project folder"); + }); + + it("uses only the placeholder for issue keys", () => { + // CWE ids are the security reviewer's references, not issue keys. + for (const [path, text] of pluginFiles()) { + for (const [key] of text.matchAll(/\b[A-Z]{2,}-\d+\b/g)) { + assert.ok(/^(PROJ|CWE)-/.test(key), `${key} in ${path}`); + } + } + }); + + it("names no lowercase project branch", () => { + assertAbsent(/\b[a-z]{2,}\/[a-z]{2,}-\d+-/, "project branch"); + }); + + it("does not assume the project has a Working agreements heading", () => { + assertAbsent(/Working agreements/, "project heading"); + }); +}); + +describe("no skills outside the plugin", () => { + it("does not send the reader to an outside skill", () => { + assertAbsent(/\/self-review|\/refine-ticket|\/retro\b|\/next-up/, "outside skill"); + }); + + it("says the ticket needs refining where unclear AC stops a run", () => { + assert.match(skill("start-ticket"), /needs refining/i); + }); + + it("calls its own skills by their namespaced names", () => { + for (const [path, text] of pluginFiles()) { + // The marker and heading keep their wording because the script reads them. + const rest = text + .replaceAll("Generated by `/pr-review`", "") + .replaceAll("Response to /pr-review", ""); + assert.doesNotMatch( + rest, + /(^|[\s`("*])\/(pr-review|start-ticket|address-review)\b/m, + path, + ); + } + }); +}); + +describe("verification comes from the project", () => { + for (const name of ["start-ticket", "address-review"]) { + it(`${name} reads commands and rules from AGENTS.md`, () => { + assert.match(skill(name), /`AGENTS\.md`/); + assert.match(skill(name), /`## Verification`/); + assert.match(skill(name), /project rules/i); + }); + + it(`${name} says so instead of guessing when the section is missing`, () => { + assert.match(skill(name), /no `## Verification` section/i); + assert.match(skill(name), /say so instead of guessing/i); + }); + } +}); + +describe("pr-review spawns its own reviewers", () => { + it("spawns all six by namespaced name", () => { + for (const a of AGENTS) { + assert.match(skill("pr-review"), new RegExp(`\`software-factory:${a}\``)); + } + }); + + it("never names a reviewer bare", () => { + assert.doesNotMatch(skill("pr-review"), /`sdlc-review-/); + }); +}); + +describe("no prompt asks an agent to recount its reasoning", () => { + it("has no such wording in any skill or agent", () => { + assertAbsent( + /(recount|recap|restate|repeat|summari[sz]e|explain|describe|walk through|share|show)\s+(\w+\s+){0,3}(reasoning|thinking|thought process|chain of thought)/i, + "reasoning request", + ); + }); +}); diff --git a/tests/lib.mjs b/tests/lib.mjs new file mode 100644 index 0000000..2366e5e --- /dev/null +++ b/tests/lib.mjs @@ -0,0 +1,45 @@ +import { existsSync, readdirSync, readFileSync } from "node:fs"; +import { join } from "node:path"; + +export const root = join(import.meta.dirname, ".."); +export const pluginDir = join(root, "plugins", "software-factory"); + +export const SKILLS = ["address-review", "pr-review", "start-ticket"]; +export const AGENTS = [ + "sdlc-review-impact", + "sdlc-review-modernization", + "sdlc-review-patterns", + "sdlc-review-performance", + "sdlc-review-quality", + "sdlc-review-requirements", +]; + +export const read = (...parts) => readFileSync(join(root, ...parts), "utf8"); +export const json = (...parts) => JSON.parse(read(...parts)); +export const exists = (...parts) => existsSync(join(root, ...parts)); + +const readOrEmpty = (path) => (existsSync(path) ? readFileSync(path, "utf8") : ""); + +export const skill = (name) => + readOrEmpty(join(pluginDir, "skills", name, "SKILL.md")); +export const agent = (name) => readOrEmpty(join(pluginDir, "agents", `${name}.md`)); + +export const listDir = (...parts) => { + const dir = join(root, ...parts); + return existsSync(dir) ? readdirSync(dir).sort() : []; +}; + +// Every file the plugin ships, as [relative path, contents]. +export const pluginFiles = () => { + if (!existsSync(pluginDir)) return []; + return readdirSync(pluginDir, { recursive: true, encoding: "utf8" }) + .filter((f) => /\.(md|json)$/.test(f)) + .map((f) => [f, readFileSync(join(pluginDir, f), "utf8")]); +}; + +// The text between `` and ``, or null. +export const block = (text, id) => { + const start = text.indexOf(``); + const end = text.indexOf(``); + return start === -1 || end === -1 ? null : text.slice(start, end); +}; diff --git a/tests/manifest.test.mjs b/tests/manifest.test.mjs new file mode 100644 index 0000000..9ea1581 --- /dev/null +++ b/tests/manifest.test.mjs @@ -0,0 +1,78 @@ +import assert from "node:assert/strict"; +import { execFileSync, spawnSync } from "node:child_process"; +import { describe, it } from "node:test"; +import { AGENTS, agent, exists, json, listDir, read, root, SKILLS } from "./lib.mjs"; + +const hasClaude = spawnSync("claude", ["--version"]).status === 0; + +describe("marketplace and plugin", () => { + it("names the marketplace software-factory and lists the plugin from a subdirectory", () => { + const market = json(".claude-plugin", "marketplace.json"); + assert.equal(market.name, "software-factory"); + assert.equal(market.plugins.length, 1); + assert.equal(market.plugins[0].name, "software-factory"); + assert.equal(market.plugins[0].source, "./plugins/software-factory"); + }); + + it("names the plugin software-factory with a semver version", () => { + const plugin = json("plugins", "software-factory", ".claude-plugin", "plugin.json"); + assert.equal(plugin.name, "software-factory"); + assert.match(plugin.version, /^\d+\.\d+\.\d+$/); + }); + + it("passes claude plugin validate", { skip: !hasClaude && "claude not on PATH" }, () => { + for (const target of [".", "plugins/software-factory"]) { + execFileSync("claude", ["plugin", "validate", target], { cwd: root, stdio: "pipe" }); + } + }); +}); + +describe("contents", () => { + it("holds exactly the three skills", () => { + assert.deepEqual(listDir("plugins", "software-factory", "skills"), SKILLS); + }); + + it("holds exactly the six review agents, each named after its file", () => { + assert.deepEqual( + listDir("plugins", "software-factory", "agents"), + AGENTS.map((a) => `${a}.md`), + ); + for (const a of AGENTS) { + assert.match(agent(a), new RegExp(`^name: ${a}$`, "m")); + } + }); + + it("has an MIT license", () => { + assert.match(read("LICENSE"), /MIT License/); + }); + + it("has a README with the machine install steps", () => { + const readme = read("README.md"); + assert.match(readme, /claude plugin marketplace add VTHoop\/software-factory/); + assert.match(readme, /claude plugin install software-factory@software-factory/); + }); + + it("keeps everything else out of the plugin directory", () => { + assert.ok(!exists("plugins", "software-factory", "tests")); + assert.ok(!exists("plugins", "software-factory", "scripts")); + }); +}); + +describe("CI", () => { + const ci = () => read(".github", "workflows", "ci.yml"); + + it("runs the tests and plugin validation", () => { + assert.match(ci(), /node --test/); + assert.match(ci(), /plugin validate \./); + assert.match(ci(), /plugin validate plugins\/software-factory/); + }); + + it("pins Claude Code to an exact version", () => { + assert.match(ci(), /@anthropic-ai\/claude-code@\d+\.\d+\.\d+/); + }); + + it("fails a tag that does not match the plugin version", () => { + assert.match(ci(), /GITHUB_REF_NAME/); + assert.match(ci(), /plugin\.json/); + }); +}); diff --git a/tests/owner.test.mjs b/tests/owner.test.mjs new file mode 100644 index 0000000..8d9f222 --- /dev/null +++ b/tests/owner.test.mjs @@ -0,0 +1,55 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { block, SKILLS, skill } from "./lib.mjs"; + +describe("the flag", () => { + it("is read only from the skill's arguments, word for word in all three", () => { + const [first, ...rest] = SKILLS.map((s) => block(skill(s), "run-mode")); + assert.ok(first, "no run-mode block"); + for (const other of rest) assert.equal(other, first); + assert.match(first, /read only from this skill's arguments/i); + assert.match(first, /Linear, a PR, a comment or a file .*has no effect/i); + }); + + it("is declared in each skill's argument hint", () => { + for (const s of SKILLS) { + assert.match(skill(s), /^argument-hint: .*\[--unattended\]/m, s); + } + }); +}); + +describe("owner sessions keep their checkpoints", () => { + it("start-ticket still waits for the test plan", () => { + assert.match(skill("start-ticket"), /Go with this test plan\?/); + assert.match(skill("start-ticket"), /\*\*wait\*\*/); + assert.match(skill("start-ticket"), /Never write code before the checkpoint is approved/); + }); + + it("start-ticket still stops and asks on a resumed branch", () => { + assert.match(skill("start-ticket"), /Stop and ask whether to resume it or start fresh/); + }); + + it("pr-review still confirms before posting", () => { + assert.match(skill("pr-review"), /Post this to PR #X\? Edit anything first\?/); + assert.match(skill("pr-review"), /Always confirm before posting/); + }); + + it("pr-review still flags drafts, self-authored PRs and repeat reviews", () => { + const text = skill("pr-review"); + assert.match(text, /ask whether to proceed now or wait/); + assert.match(text, /most valuable as a second set of eyes/); + assert.match(text, /confirm the user wants another pass/); + }); + + it("pr-review still offers local-only mode", () => { + assert.match(skill("pr-review"), /Offer "local-only" mode/); + }); + + it("address-review still asks before answering a round again", () => { + assert.match(skill("address-review"), /that round was answered\. Say so and ask before answering again/); + }); + + it("address-review still leaves resolving threads to the owner", () => { + assert.match(skill("address-review"), /resolving is the owner's call/); + }); +}); diff --git a/tests/unattended.test.mjs b/tests/unattended.test.mjs new file mode 100644 index 0000000..4f26411 --- /dev/null +++ b/tests/unattended.test.mjs @@ -0,0 +1,194 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { block, SKILLS, skill } from "./lib.mjs"; + +const unattended = (name) => { + const text = block(skill(name), "unattended"); + assert.ok(text, `${name} has no unattended section`); + return text; +}; + +describe("structured result", () => { + it("is word for word the same in all three skills", () => { + const [first, ...rest] = SKILLS.map((s) => block(skill(s), "unattended-result")); + assert.ok(first, "no unattended-result block"); + for (const other of rest) assert.equal(other, first); + }); + + it("gives a status of finished or stopped, and the reason if stopped", () => { + const text = block(skill("start-ticket"), "unattended-result"); + assert.ok(text.includes('SOFTWARE_FACTORY_RESULT: {"status":"finished"}')); + assert.ok(text.includes('SOFTWARE_FACTORY_RESULT: {"status":"stopped","reason":"')); + assert.match(text, /last line/i); + }); + + it("posts no escalation comment or label on a stop", () => { + const text = block(skill("start-ticket"), "unattended-result"); + assert.match(text, /no escalation comment or label/i); + }); + + it("never waits for an answer", () => { + const text = block(skill("start-ticket"), "unattended-result"); + assert.match(text, /never wait/i); + }); +}); + +describe("start-ticket --unattended", () => { + const text = () => unattended("start-ticket"); + + it("posts the AC → test table and approach to Linear instead of waiting", () => { + assert.match(text(), /AC → test table and the approach/); + assert.match(text(), /`save_comment`/); + assert.match(text(), /do not wait for approval/i); + }); + + it("then implements test-first", () => { + assert.match(text(), /first commit is the failing tests/i); + }); + + it("runs the project's verification commands before opening a PR", () => { + assert.match(text(), /run every command .*`## Verification`.* before opening a PR/i); + }); + + it("opens a non-draft PR on the Linear branch with the key in the title", () => { + assert.match(text(), /non-draft PR/i); + assert.match(text(), /Linear branch name/i); + assert.match(text(), /issue key in the title/i); + assert.doesNotMatch(text(), /--draft/); + }); + + it("has the fixed manual-test heading with a checklist and pre-test steps", () => { + assert.match(text(), /^\s*## Owner manual test$/m); + assert.match(text(), /^\s*### Before testing$/m); + assert.match(text(), /^\s*### Checklist$/m); + assert.match(text(), /migration to apply/i); + assert.match(text(), /env var to add/i); + assert.match(text(), /real-key check/i); + }); + + it("ends each stop condition in a stopped result and opens no PR", () => { + for (const condition of [ + /uncommitted/i, + /own commits/i, + /branch already exists/i, + /Linear is unreachable/i, + /Done or Canceled/i, + /In Review/i, + /blocked by/i, + /contradicts a project document/i, + /no AC section/i, + /no `## Verification` section/i, + ]) { + assert.match(text(), condition); + } + assert.match(text(), /stopped result with the reason.*opens no PR/is); + }); + + it("stops for something only the owner can provide only when the work needs it", () => { + assert.match(text(), /only if the work can't be implemented and verified without it/i); + assert.match(text(), /otherwise .*`### Before testing`/i); + }); + + it("keeps the branch local when verification still fails", () => { + assert.match(text(), /still fails/i); + assert.match(text(), /open no PR and do not push/i); + assert.match(text(), /names? the failing commands/i); + assert.match(text(), /branch stays local/i); + }); + + it("reports a check that can't run locally without calling it passing", () => { + assert.match(text(), /can't run locally does not stop the run/i); + assert.match(text(), /never report it as passing/i); + }); + + it("never applies a migration to a hosted database where the owner does that", () => { + assert.match(text(), /only the owner applies migrations/i); + assert.match(text(), /never apply it to a hosted database/i); + assert.match(text(), /local reset used for verification is allowed/i); + }); +}); + +describe("pr-review --unattended", () => { + const text = () => unattended("pr-review"); + + it("spawns all six however small the diff, never reviewing inline", () => { + assert.match(text(), /all six review agents, however small the diff/i); + assert.match(text(), /never review inline in their place/i); + }); + + it("posts the summary without asking, and no inline comments", () => { + assert.match(text(), /post the summary comment without asking/i); + assert.match(text(), /no inline comments/i); + }); + + it("continues when already reviewed or authored by the current account", () => { + assert.match(text(), /already reviewed/i); + assert.match(text(), /authored by the current account/i); + assert.match(text(), /continue/i); + }); + + it("stops with the reason on each listed condition", () => { + for (const condition of [ + /draft/i, + /merged/i, + /closed/i, + /no substantive changes/i, + /several matching PRs/i, + /no ticket key/i, + /Linear is unreachable/i, + /`gh` failure/i, + ]) { + assert.match(text(), condition); + } + }); + + it("does not skip the requirements reviewer or fall back to local-only", () => { + assert.match(text(), /never skip the requirements reviewer/i); + assert.match(text(), /never fall back to local-only/i); + }); + + it("leaves the comment format unchanged", () => { + const full = skill("pr-review"); + assert.ok(full.includes("*Generated by `/pr-review` · Lenses:")); + assert.ok(full.includes("**Overall:** [Approved | Needs Revision | Blocked]")); + }); +}); + +describe("address-review --unattended", () => { + const text = () => unattended("address-review"); + + it("ends every ask in a stopped result", () => { + for (const condition of [ + /not `OPEN`/, + /cross-repository/i, + /another worktree/i, + /uncommitted changes/i, + /diverged/i, + /no trusted review/i, + /already answered/i, + /no `## Verification` section/i, + ]) { + assert.match(text(), condition); + } + assert.match(text(), /stopped result with the reason/i); + }); + + it("still lists deferrals and dismissals for the owner", () => { + assert.match(text(), /deferred findings and dismissals are still listed/i); + }); + + it("still does not merge, approve, resolve threads or create issues", () => { + assert.match(text(), /does not merge, approve, resolve threads or create follow-up issues/i); + }); + + it("keeps the rules unchanged", () => { + const full = skill("address-review"); + assert.match(full, /Never weaken a test, lower a gate, add a suppression, or use `--no-verify`/); + assert.match(full, /One disposition per finding, and every finding gets one/); + assert.match(full, /`OWNER`, `MEMBER` or `COLLABORATOR`/); + }); + + it("keeps the response heading unchanged", () => { + assert.match(skill("address-review"), /^## Response to \/pr-review$/m); + }); +}); From 86732403d3e5837fc967d6b555953be7511f73c0 Mon Sep 17 00:00:00 2001 From: Patrick Hooper <25905129+VTHoop@users.noreply.github.com> Date: Sat, 31 Oct 2026 12:25:22 -0400 Subject: [PATCH 2/4] HEA-21: the plugin: skills, review agents, CI and README Co-Authored-By: Claude Opus 5.5 --- .claude-plugin/marketplace.json | 16 ++ .github/workflows/ci.yml | 27 +++ LICENSE | 21 ++ README.md | 76 +++++++ .../.claude-plugin/plugin.json | 10 + .../agents/sdlc-review-impact.md | 70 ++++++ .../agents/sdlc-review-modernization.md | 101 +++++++++ .../agents/sdlc-review-patterns.md | 78 +++++++ .../agents/sdlc-review-performance.md | 78 +++++++ .../agents/sdlc-review-quality.md | 97 +++++++++ .../agents/sdlc-review-requirements.md | 51 +++++ .../skills/address-review/SKILL.md | 160 ++++++++++++++ .../skills/pr-review/SKILL.md | 201 ++++++++++++++++++ .../skills/start-ticket/SKILL.md | 143 +++++++++++++ tests/generic.test.mjs | 2 +- 15 files changed, 1130 insertions(+), 1 deletion(-) create mode 100644 .claude-plugin/marketplace.json create mode 100644 .github/workflows/ci.yml create mode 100644 LICENSE create mode 100644 README.md create mode 100644 plugins/software-factory/.claude-plugin/plugin.json create mode 100644 plugins/software-factory/agents/sdlc-review-impact.md create mode 100644 plugins/software-factory/agents/sdlc-review-modernization.md create mode 100644 plugins/software-factory/agents/sdlc-review-patterns.md create mode 100644 plugins/software-factory/agents/sdlc-review-performance.md create mode 100644 plugins/software-factory/agents/sdlc-review-quality.md create mode 100644 plugins/software-factory/agents/sdlc-review-requirements.md create mode 100644 plugins/software-factory/skills/address-review/SKILL.md create mode 100644 plugins/software-factory/skills/pr-review/SKILL.md create mode 100644 plugins/software-factory/skills/start-ticket/SKILL.md diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json new file mode 100644 index 0000000..58f0239 --- /dev/null +++ b/.claude-plugin/marketplace.json @@ -0,0 +1,16 @@ +{ + "name": "software-factory", + "owner": { + "name": "VTHoop" + }, + "metadata": { + "description": "Skills that take a refined Linear ticket to a reviewed, addressed pull request." + }, + "plugins": [ + { + "name": "software-factory", + "source": "./plugins/software-factory", + "description": "start-ticket, pr-review, address-review and six review agents, each runnable by an owner or unattended." + } + ] +} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..92047ff --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,27 @@ +name: CI + +on: + push: + pull_request: + +permissions: + contents: read + +jobs: + checks: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - uses: actions/setup-node@v6 + with: + node-version: 24 + - name: Install Claude Code + run: npm install --global @anthropic-ai/claude-code@2.1.316 + - run: claude plugin validate . + - run: claude plugin validate plugins/software-factory + - run: node --test + - name: Check the tag matches the plugin version + if: startsWith(github.ref, 'refs/tags/') + run: | + version=$(node -p "require('./plugins/software-factory/.claude-plugin/plugin.json').version") + test "$GITHUB_REF_NAME" = "v$version" diff --git a/LICENSE b/LICENSE new file mode 100644 index 0000000..8eca996 --- /dev/null +++ b/LICENSE @@ -0,0 +1,21 @@ +MIT License + +Copyright (c) 2026 VTHoop + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/README.md b/README.md new file mode 100644 index 0000000..7b6d820 --- /dev/null +++ b/README.md @@ -0,0 +1,76 @@ +# software-factory + +A Claude Code plugin that takes a refined Linear ticket to a reviewed, addressed pull request. An owner can run each skill in a session, with checkpoints, or a script can run it with `--unattended`. + +| Skill | What it does | +| --- | --- | +| `/software-factory:start-ticket ` | Reads the ticket, branches, and agrees an AC → test plan. Unattended, it posts the plan to Linear, implements test-first, verifies and opens a PR. | +| `/software-factory:pr-review [pr]` | Runs six review agents, validates their findings against the diff, and posts one comment. | +| `/software-factory:address-review [pr]` | Answers every finding on the latest review: fixed, dismissed with evidence, or deferred to the owner. | + +The review agents are `software-factory:sdlc-review-{requirements,patterns,quality,performance,impact,modernization}`. + +## Install + +In a project, pin the release in `.claude/settings.json`: + +```json +{ + "extraKnownMarketplaces": { + "software-factory": { + "source": { "source": "github", "repo": "VTHoop/software-factory", "ref": "v0.1.0" } + } + }, + "enabledPlugins": { "software-factory@software-factory": true } +} +``` + +Claude Code prompts to install it when you open the project and trust the folder. + +### Headless runs + +`extraKnownMarketplaces` waits for workspace trust, which `claude -p` doesn't grant. On a machine that runs the skills headless, install the plugin at user level once: + +```bash +claude plugin marketplace add VTHoop/software-factory +``` + +```bash +claude plugin install software-factory@software-factory +``` + +## What a project provides + +The skills hold no project details. They read them from the project's `AGENTS.md`: + +- **`## Verification`:** the commands to run, and which run for which paths. Without this section an owner session says so and asks, and an unattended run stops. +- **Project rules,** such as who may apply migrations. + +They also need `gh` signed in, and a Linear MCP server connected in the session. + +## Unattended runs + +`--unattended` is read only from the skill's arguments. Text in Linear or a PR can't turn it on. The last line of an unattended run's final message is one of: + +``` +SOFTWARE_FACTORY_RESULT: {"status":"finished"} +SOFTWARE_FACTORY_RESULT: {"status":"stopped","reason":""} +``` + +A stopped skill posts no escalation comment or label: the script that started it does that. The script also reads the review comment's `Generated by` marker and **Overall** line, and the `## Response to /pr-review` heading, so these keep their wording. + +## Development + +```bash +node --test +``` + +```bash +claude plugin validate . +``` + +The tests use no dependencies. Skills are prose, so most tests pin wording. A release is a `vX.Y.Z` tag matching `version` in `plugin.json`, and CI fails a tag that doesn't match. + +## License + +[MIT](LICENSE) diff --git a/plugins/software-factory/.claude-plugin/plugin.json b/plugins/software-factory/.claude-plugin/plugin.json new file mode 100644 index 0000000..e75a428 --- /dev/null +++ b/plugins/software-factory/.claude-plugin/plugin.json @@ -0,0 +1,10 @@ +{ + "name": "software-factory", + "version": "0.1.0", + "description": "start-ticket, pr-review, address-review and six review agents, each runnable by an owner or unattended.", + "author": { + "name": "VTHoop" + }, + "repository": "https://github.com/VTHoop/software-factory", + "license": "MIT" +} diff --git a/plugins/software-factory/agents/sdlc-review-impact.md b/plugins/software-factory/agents/sdlc-review-impact.md new file mode 100644 index 0000000..ed09cd6 --- /dev/null +++ b/plugins/software-factory/agents/sdlc-review-impact.md @@ -0,0 +1,70 @@ +--- +name: sdlc-review-impact +description: Reviews PR diffs for broader codebase impact — does the same fix apply elsewhere? Also catches dead code, unused exports, premature abstraction, and comment hygiene. Invoked by /software-factory:pr-review. +tools: Read, Grep, Glob, Bash +--- + +You are a broader-impact reviewer. Your main question for every change in the diff: **"Does this same problem exist elsewhere in the codebase, and was it left untouched?"** You also catch leftovers — dead code, unused exports, gratuitous comments, premature abstractions. Language-agnostic; the analysis works the same in Java, Ruby, Python, or TypeScript. + +# What you look for + +**Same bug / change elsewhere** +- A fix that changed one call site for a pattern that exists in 3+ other places. Use `Grep` to find them. +- A new utility added without removing the duplicated inline logic in the same files the diff touched. + +**Dead code introduced or left behind** +- New functions/methods/classes/components with no callers +- Imports added for symbols that aren't used +- Removed code that left orphaned imports, dead types, or now-unreachable branches + +**Comment hygiene** +- First, check the repo's documented comment convention (`CLAUDE.md`, `AGENTS.md`, `.claude/rules/`, a style guide — and if `CLAUDE.md` is just a pointer to another file, read the file it names). Enforce *that*. +- If the repo documents a convention, apply it. If it documents none, apply only a light, universal floor — and don't impose a "no comments" rule the repo hasn't adopted: + - Comments that restate WHAT the code does (redundant) + - Leftover `// TODO` / "removed" / commented-out code + - Comments referencing this PR/ticket/author ("added for PROJ-123") — those belong in commit messages +- Never flag a comment that explains a non-obvious WHY (hidden constraint, bug workaround, surprising edge case). + +**Premature abstraction** +- New helper that wraps a single call site (inline it) +- Generic interface/base class for one consumer +- A few similar lines factored into a helper that obscures more than it saves + +# Inputs the orchestrator gives you + +- Destination branch, source branch +- List of changed files +- Optional: lessons from prior misses (`.claude/flywheel.md` `[review]` bucket) + +# How to work + +1. Generate the diff: `git diff ...`. +2. For every "fix" or "change," ask: is the original problem present elsewhere? Use `Grep` aggressively. **Report only matches you've actually grep-confirmed.** +3. For new exports/functions/methods, grep for callers across the repo. Zero callers = dead code (account for framework entry points, reflection, DI registration, and test-only use before declaring something dead). +4. For new comments, ask: would a future reader be confused without this? If no, flag it (subject to the repo's convention). + +# Output format + +If clean, return exactly: +``` +LGTM +``` + +Otherwise, one finding per issue: +``` +**Severity:** Major | Minor +**Category:** Same-issue-elsewhere | Dead code | Comment | Premature abstraction +**Where:** file:line (or list of file:lines for cross-cutting) +**Issue:** [one sentence] +**Suggested action:** [one sentence] +``` + +`Major`: same bug present in 3+ other files; new dead exports. +`Minor`: comment hygiene; premature abstractions; dead imports. + +# Constraints + +- For "same-issue-elsewhere" findings: list at least one other file:line that exhibits it. Don't flag without proof. +- Respect documented per-area conventions — some directories (legacy code, generated code, vendored code) may have different rules; honor them. +- Don't propose splitting the PR. Other reviewers handle scope; you handle leftovers and reach. +- **Read the diff locally — `git` only.** Read the diff and files with local `git` in the checkout directory the orchestrator names (`git -C …`); if a ref is missing, `git fetch origin ` first. Don't contact the PR service yourself (no `gh`, no forge API) — the orchestrator already handed you the branches and changed files. diff --git a/plugins/software-factory/agents/sdlc-review-modernization.md b/plugins/software-factory/agents/sdlc-review-modernization.md new file mode 100644 index 0000000..1b24ed2 --- /dev/null +++ b/plugins/software-factory/agents/sdlc-review-modernization.md @@ -0,0 +1,101 @@ +--- +name: sdlc-review-modernization +description: Reviews PR diffs for missed opportunities to use newer language/framework capabilities available in the version the repo actually runs. Java, Ruby, Python, TS/JS, Angular/RxJS. Always non-blocking recommendations. Invoked by /software-factory:pr-review. +tools: Read, Grep, Glob, Bash +--- + +You are a modernization scout. For code in the diff you ask: **"Given the version actually installed, would a newer capability simplify this?"** Output is **always** non-blocking Recommendations — never blockers. + +# Grounding rule (do this first, every time) +1. Read the project manifest to find installed versions: `pom.xml`/`build.gradle` (`java.version`), `Gemfile`/`.ruby-version`, `pyproject.toml`/`setup.cfg`/`.python-version`, `package.json` + `angular.json`/`tsconfig.json`. +2. Recommend a capability **only if its "Since" version ≤ the installed version**. No "consider records" on Java 11. +3. If the repo documents a direction (a signals roadmap, a style guide), **defer to it** over these defaults. +4. **In-scope only** — only files already in the diff. Never "while you're here, also migrate X." +5. **Adversarial filter** — each recommendation states a concrete one-sentence benefit; if you can't, drop it. **Cap at 5**; keep the highest-leverage. A tasteful, working pattern is not a modernization candidate. + +# Candidate tables (verified "Since" versions) + +### Java +| Old | Modern | Since | Benefit | +|---|---|---|---| +| anonymous class | lambda | 8 | Concise inline single-method functions. | +| null checks | `Optional` | 8 | Makes absence explicit in the type. | +| imperative loop | Stream | 8 | Declarative filter/map/collect. | +| explicit local type | `var` | 10 | Cuts redundant type noise. | +| switch statement | switch expression | 14 | Returns a value, no fall-through. | +| string concat | text block | 15 | Readable multi-line literals (SQL/JSON). | +| data-holder boilerplate | `record` | 16 | Generated ctor/equals/hashCode/accessors. | +| `instanceof` + cast | pattern-matching `instanceof` | 16 | Binds the cast in one expression. | +| open hierarchy | `sealed` | 17 | Restricts subtypes → exhaustive switch. | + +### Ruby +| Old | Modern | Since | Benefit | +|---|---|---|---| +| `x && x.foo` | safe nav `&.` | 2.3 | Short-circuits on nil, no repeated receiver. | +| nested hash access | `dig` | 2.3 | Reaches deep keys without nil-guard chains. | +| `{ \|x\| x.up }` | `&:sym` | 1.8.7 | One-token block for single-method calls. | +| long `case`/`when` | pattern matching `case/in` | 3.0 | Destructures and matches shape in one construct. | +| one-line wrapper method | endless method `def f = …` | 3.0 | Trivial method with no `end`. | + +### Python +| Old | Modern | Since | Benefit | +|---|---|---|---| +| `%` / `.format()` | f-string | 3.6 | Inline expressions, faster and clearer. | +| `os.path` | `pathlib` | 3.4 | Object paths with `/` join. | +| `__init__` boilerplate | `@dataclass` | 3.7 | Generated init/repr/eq from annotations. | +| assign-then-test | walrus `:=` | 3.8 | Assigns inside the condition. | +| `typing.List[str]` | `list[str]` | 3.9 | Built-in generics, no `typing` import. | +| if/elif type dispatch | `match` | 3.10 | Structural pattern matching. | + +### TypeScript / JavaScript +| Old | Modern | Since | Benefit | +|---|---|---|---| +| `a && a.b` | optional chaining `?.` | TS 3.7 | Stops at null/undefined without nested guards. | +| ternary null-default | nullish `??` | TS 3.7 | Defaults only on null/undefined, not `0`/`""`/`false`. | +| `as` cast | `satisfies` | TS 4.9 | Validates against a type without widening inference. | +| `any` | `unknown` + narrowing | TS 3.0 | Forces a type check before use. | + +### Angular / RxJS +| Old | Modern | Since | Benefit | +|---|---|---|---| +| `BehaviorSubject` + async pipe | `signal()` | 17 | Synchronous reactive reads, no subscription bookkeeping. | +| `Observable` + async pipe | `toSignal()` | 16 | Stream → signal with auto-unsubscribe. | +| `@Input()` | `input()` | 17.1 | Reactive typed input; supports `required`. | +| `@ViewChild()` | `viewChild()` | 17.2 | Query result as a signal, no `AfterViewInit` timing. | +| `@Output()` | `output()` | 17.3 | RxJS-free, lighter event emitter. | +| `*ngIf` / `*ngFor` | `@if` / `@for` | 17 | Built-in control flow with mandatory `track`. | +| constructor DI | `inject()` | 14 | Field-initializer DI; composable, inheritance-friendly. | +| `NgModule` | standalone | 14 | Drops module wiring; deps on the component. | +| async pipe used twice | `@let` | 18.1 | Binds the value once, reused across the template. | + +*(Several Angular signal APIs shipped in developer preview and stabilized in v19 — if the repo pins to stable-only APIs, note that rather than pushing the change.)* + +# Inputs the orchestrator gives you +- Destination branch, source branch; the list of changed files +- Optional: lessons from prior misses (`.claude/flywheel.md` `[review]` bucket) + +How to work: read the manifest; `git diff ...`; for each in-scope file, scan for the table patterns for that stack; verify the installed version supports the replacement; apply the adversarial filter; keep ≤5. + +# Output format + +If no opportunities, return exactly: +``` +LGTM +``` + +Otherwise, every finding is severity `Recommendation`: +``` +**Severity:** Recommendation +**Where:** file:line +**Current:** [one phrase — the existing pattern] +**Modern:** [one phrase — the replacement] (available since [version]) +**Why this works:** [one sentence — concrete benefit] +``` + +# Constraints +- Severity is always `Recommendation`. Never escalate. +- Never recommend rewriting a working, well-formed pattern just because a newer one exists. +- Never recommend changes outside the diff scope. +- Don't flag style preferences (arrow vs function, quote style) — only genuine version-gated capabilities. +- If you can't confirm the installed version supports it, don't recommend it. +- **Read the diff locally — `git` only.** Read the diff and files with local `git` in the checkout directory the orchestrator names (`git -C …`); if a ref is missing, `git fetch origin ` first. Don't contact the PR service yourself (no `gh`, no forge API) — the orchestrator already handed you the branches and changed files. diff --git a/plugins/software-factory/agents/sdlc-review-patterns.md b/plugins/software-factory/agents/sdlc-review-patterns.md new file mode 100644 index 0000000..dde86ac --- /dev/null +++ b/plugins/software-factory/agents/sdlc-review-patterns.md @@ -0,0 +1,78 @@ +--- +name: sdlc-review-patterns +description: Reviews PR diffs for conformance to THE PROJECT'S OWN documented or idiomatic patterns — discovered from the host repo's CLAUDE.md, AGENTS.md, .claude/rules, docs, and lint configs, never from training-data preferences. Invoked by /software-factory:pr-review. +tools: Read, Grep, Glob, Bash +--- + +You are a pattern-conformance reviewer. Your job: flag where the diff deviates from **the project's own** documented or idiomatic patterns. You **never** flag deviation from your own training-data preferences. This lens is project-sourced by design — if a convention isn't discoverable in the repo, it isn't a finding. + +# 1. Discover where this repo's conventions live (read these first) + +**Prose / agent docs:** root `CLAUDE.md` **and** root `AGENTS.md` — both are conventional agent-instruction files and a repo may use either or both. Follow any `@`-imported files (shared rules sometimes live in a sibling repo). Then `.claude/rules/`, `.claude/skills/*/SKILL.md`, `docs/`, `README.md`, `CONTRIBUTING.md`, `ARCHITECTURE.md`. + +> **Follow pointers — this is the most common way this lens comes up empty.** A short `CLAUDE.md` whose content is mostly a reference to another file (commonly `AGENTS.md`) is a *pointer*, not the conventions. Read the file it names. Concluding "no documented conventions" after reading a two-line pointer is a failure of this step, not a property of the repo — and it silently downgrades you to the §4 no-docs fallback when the repo actually has a detailed convention set. + +**Machine-enforceable config (encodes conventions even with zero prose):** +| Stack | Config to read | +|---|---| +| Java/Maven/Gradle | `pom.xml`/`build.gradle` plugins (checkstyle/PMD/spotbugs/spotless), `checkstyle.xml`, `.editorconfig` | +| Ruby/Rails | `.rubocop.yml` (+`_todo`), `.editorconfig`, `Gemfile` (rubocop/standard) | +| Python | `pyproject.toml` (ruff/black/mypy/isort), `setup.cfg`, `.flake8`, `ruff.toml` | +| TS/Angular | `.eslintrc*` / `eslint.config.*`, `.prettierrc`, `tsconfig.json`, `angular.json` | +| Any | `.editorconfig`, the CI lint/format step (`.drone.yml` etc.) — the ground truth of what's enforced | + +# 2. Pattern categories to check (stack-agnostic buckets) + +- **HTTP/IO call sites** — raw client where the repo mandates a wrapper/gateway/service layer. +- **Data-access layer** — ad-hoc query/ORM use bypassing the repo's repository/mapper/model conventions. +- **DI / wiring** — instantiation/injection style differing from the repo norm. +- **Error handling** — swallowed/rethrown errors not matching the repo's exception/Result/loadable pattern. +- **Naming & file placement** — names or directories not matching the established scheme. +- **Module / layer boundaries** — imports crossing the declared dependency direction. +- **In-file consistency** — changed lines diverging from the surrounding file's own established style. + +# 3. Discover → enforce algorithm + +1. **Discover (once):** read the sources in §1; extract concrete rules, each with its evidence source (doc path / config key). +2. **Classify** each changed file by path + language into one or more §2 buckets. +3. **Per changed hunk, find the governing convention,** preferring: (a) explicit doc rule → (b) machine-config rule → (c) dominant pattern in sibling files of the same bucket → (d) in-file surrounding style. +4. **Compare & flag** only real mismatches. Cite the changed line, the convention, and **where the convention came from**. +5. **Skip** anything with no discoverable governing convention rather than inventing one. + +# 4. No-docs fallback (e.g. a bare repo with no CLAUDE.md/AGENTS.md or lint config) + +Only enter this fallback after §1 genuinely came up empty — including following any pointer file. Then drop to (a) **in-repo consistency** — compare the change to the dominant pattern in sibling files — and (b) **language idiom for genuine defects only** (resource leaks, bare excepts, shell injection). Honor self-documenting conventions (directory layout, file-naming schemes) even without prose. **Label every fallback finding** "fallback judgment (no documented rule)" and phrase it as a suggestion, never a violation. + +# Inputs the orchestrator gives you +- Destination branch, source branch; the list of changed files +- Optional: lessons from prior misses (`.claude/flywheel.md` `[review]` bucket) + +How to work: `git diff ...`, classify each changed file, check against discovered conventions, grep for cross-cutting violations. + +# Output format + +If conformant, return exactly: +``` +LGTM +``` + +Otherwise, one finding per deviation: +``` +**Severity:** Major | Minor +**Where:** file:line +**Pattern:** [the convention, e.g., "API calls go through the gateway layer"] +**Deviation:** [one sentence — what the code does instead] +**Source:** [where the convention is declared — doc path / config key / sibling file] +**Confidence:** documented | config-enforced | sibling-consistency | idiom/fallback +``` + +`Major`: violations of an explicitly documented or lint-enforced rule. `Minor`: in-file consistency, sibling-pattern drift. + +# Reviewer anti-patterns (do NOT do) + +- Don't impose framework/training-data "best practices" the repo hasn't adopted. +- Don't flag a pattern the repo uses consistently just because you'd write it differently. +- Don't treat the absence of a linter as license to invent its rules. +- Every finding must name its source. If you can't cite where the convention lives, it isn't a finding. +- Don't propose migrating stable, out-of-diff legacy patterns — flag only what's in the diff. +- **Read the diff locally — `git` only.** Read the diff and files with local `git` in the checkout directory the orchestrator names (`git -C …`); if a ref is missing, `git fetch origin ` first. Don't contact the PR service yourself (no `gh`, no forge API) — the orchestrator already handed you the branches and changed files. diff --git a/plugins/software-factory/agents/sdlc-review-performance.md b/plugins/software-factory/agents/sdlc-review-performance.md new file mode 100644 index 0000000..027d72d --- /dev/null +++ b/plugins/software-factory/agents/sdlc-review-performance.md @@ -0,0 +1,78 @@ +--- +name: sdlc-review-performance +description: Reviews PR diffs for performance and resource-lifecycle hazards that would actually fire at runtime — leaks, N+1 queries, unbounded growth, blocking/concurrency, wasted work, bundle bloat — across Java, Ruby, Python, and TS/Angular. Invoked by /software-factory:pr-review. +tools: Read, Grep, Glob, Bash +--- + +You are a performance and resource-lifecycle reviewer. Narrow scope: issues that would **actually fire at runtime** (leaks, jank, exhaustion, wasted work) — never theoretical micro-optimizations. + +**Discipline for every flag:** read enough of the file to confirm the hazard is on a *runtime/hot* path (request handler, loop, stream consumer, render/change-detection) and that no teardown/batching/bound already exists nearby (try-with-resources, `ensure`, `with`, `takeUntilDestroyed`, `includes`, pagination, timeout, `shareReplay`). If you can't confirm it's real, don't flag it. Tests, one-shot scripts, and CLI entry points → downgrade or skip. + +# Inputs the orchestrator gives you +- Destination branch, source branch; the list of changed files +- Optional: lessons from prior misses (`.claude/flywheel.md` `[review]` bucket) + +Work from `git diff ...`; for suspect files, read the full file — diffs hide surrounding teardown/batching. + +# Hazard catalog (by category, with per-stack signals) + +### 1. Resource leaks — Major on a per-request/per-record path; Minor in one-shot scripts +- **Java:** `FileInputStream`/`Reader`, `Connection`/`Statement`/`ResultSet`, `HttpClient`/`AsyncHttpClient`, producers (e.g. Kinesis) created but not in try-with-resources / never `.close()`d. Confirm: is the type `AutoCloseable` and method-scoped (leak) vs a managed singleton/bean (fine)? +- **Ruby:** `File.open`/`Net::HTTP.start`/raw connection in non-block form without `ensure … .close`. Block form auto-closes — only flag the non-block form. +- **Python:** `open(...)`, `requests.Session()`, DB connection/cursor used without `with` / `try/finally` / a fixture. +- **TS/Angular:** `.subscribe(` without teardown (`takeUntilDestroyed`/`takeUntil`/async pipe); `addEventListener`/`setInterval`/`fromEvent` with no removal in `ngOnDestroy`. One-shot HTTP observables self-complete — don't flag those. + +### 2. N+1 / inefficient data access — Major (a query per iteration scaling with N) +- **Ruby/ActiveRecord:** association access inside `.each`/`.map` with no `.includes`/`.preload`/`.eager_load`. +- **Java/JPA:** `LAZY` association dereferenced inside a loop; no `@BatchSize`/`join fetch`/`@EntityGraph`. +- **Python (Django/SQLAlchemy):** related access in a loop with no `select_related`/`prefetch_related` (Django) or `joinedload`/`selectinload` (SQLAlchemy). +- **Confirm:** the query is genuinely per-iteration (not hoisted/cached above the loop). N small and bounded → Minor. + +### 3. Unbounded growth / memory — Major when input size is caller/data-driven +- **All:** fetch-all-then-filter-in-app (`findAll()`, `.all`, `Model.objects.all()`, `SELECT *` without LIMIT) on a growing table; building a collection of every row; reading a whole file into memory. +- **Ruby:** `.all`/`.pluck` of a full table vs `find_each`/`in_batches`. +- **Python:** `f.read()`/`list(cursor)` over a full query vs streaming/`iterator()`/chunks. +- **Confirm:** result set is plausibly large and not already paginated/batched/known-tiny (enum/lookup → Minor). + +### 4. Blocking / concurrency — Major (event-loop block, missing outbound timeout, pool exhaustion) +- **Python async:** blocking call (`requests.*`, `time.sleep`, blocking driver) inside `async def`. +- **Node/TS:** sync FS (`readFileSync`) or CPU-heavy loop on the request path / event loop. +- **All outbound HTTP:** client call with **no timeout** (`requests.get` without `timeout=`, `RestTemplate`/`HttpClient` without connect+read timeout, `fetch` without abort) — these hang threads/connections. +- **Pools/threads:** unbounded `Thread`/executor/`Promise.all`/`map_async` fan-out over a caller-sized list (no semaphore/`max_workers`/bound); a new connection pool per request. +- **Confirm:** the call is on a serving path and the timeout/bound isn't already set globally. + +### 5. Wasted work / caching — Major when it doubles outbound/DB calls in one request +- **TS/RxJS:** same observable subscribed/async-piped multiple times with no `shareReplay`/`share` → duplicate HTTP; heavy expression/getter recomputed each change-detection cycle. +- **Angular:** missing `ChangeDetectionStrategy.OnPush` on a component doing real work; new array/object literals in template bindings forcing CD churn. +- **All backends:** identical query/HTTP issued twice in one handler with no memoization; recompute of a stable value inside a loop (hoist it). +- **Confirm:** the duplication is real (trace both call sites) and there's no existing cache/`@Cacheable`/memo/`shareReplay`. Single recompute of a cheap value → Minor. + +### 6. Bundle size (frontend only) — usually Minor unless a large dep lands in the main chunk +- Whole-library imports (`import _ from 'lodash'`, `import * as`, `import moment`) where a tree-shakeable named/subpath import or lighter lib exists; unused `dependencies`/dead imports shipped to the client. +- **Confirm:** the import lands in a shipped bundle (not a `.spec`/build script/`devDependency`). + +# Output format + +If clean, return exactly: +``` +LGTM +``` + +Otherwise: +``` +**Severity:** Major | Minor +**Category:** Resource leak | N+1 | Unbounded | Blocking/Concurrency | Wasted work | Bundle +**Where:** file:line +**Issue:** [one sentence — the specific runtime hazard] +**Why it matters:** [one sentence — the runtime/user-visible consequence] +**Fix:** [one sentence — concrete suggestion] +``` + +`Major`: real leaks, N+1 scaling with data, unbounded reads, missing timeouts, pool exhaustion, duplicate per-request calls. +`Minor`: missing `OnPush`, recompute papercuts, bundle-size nits. + +# Constraints +- Don't flag pattern conformance (a different reviewer owns "should use the wrapper"). +- Don't speculate about theoretical perf — flag what would fire on a real request/record. +- Read enough of the file to confirm the hazard, and that no teardown/batching/bound already exists, before flagging. +- **Read the diff locally — `git` only.** Read the diff and files with local `git` in the checkout directory the orchestrator names (`git -C …`); if a ref is missing, `git fetch origin ` first. Don't contact the PR service yourself (no `gh`, no forge API) — the orchestrator already handed you the branches and changed files. diff --git a/plugins/software-factory/agents/sdlc-review-quality.md b/plugins/software-factory/agents/sdlc-review-quality.md new file mode 100644 index 0000000..1f381ef --- /dev/null +++ b/plugins/software-factory/agents/sdlc-review-quality.md @@ -0,0 +1,97 @@ +--- +name: sdlc-review-quality +description: Reviews PR diffs across three lenses — security (OWASP/CWE-grounded), accessibility (UI surfaces only), and test coverage (per the repo's own conventions). Invoked by /software-factory:pr-review. +tools: Read, Grep, Glob, Bash +--- + +You are a quality reviewer covering three narrow lenses: **security**, **accessibility**, and **test coverage**. Report only issues **demonstrable in the diff** — a concrete tainted-source → dangerous-sink path (security), real rendered markup (a11y), or genuinely untested new behavior. No theoretical or defense-in-depth nits. + +# Inputs the orchestrator gives you +- Destination branch, source branch; the list of changed files +- Optional: lessons from prior misses (`.claude/flywheel.md` `[review]` bucket) + +Work from `git diff ...`; read changed files in full to confirm a real source→sink path before flagging. + +--- + +# SECURITY + +**Every security finding must cite its OWASP Top 10 (2021) category and primary CWE.** If nothing maps cleanly, it's not a security finding. + +| Class | OWASP 2021 | CWE | Severity | +|---|---|---|---| +| Hardcoded secrets / credentials committed | A07 (·A05) | CWE-798, CWE-259 | Blocker | +| SQL injection | A03 | CWE-89 | Blocker | +| OS command injection | A03 | CWE-77/78 | Blocker | +| Code / expression-language / template injection | A03 | CWE-94/917 | Blocker | +| XSS / missing output encoding | A03 | CWE-79 | Major → Blocker (user data, reflected/stored) | +| Broken authorization incl. IDOR | A01 | CWE-639/862/285 | Blocker | +| Missing/broken authentication | A07 | CWE-287/384 | Blocker | +| Path traversal | A01 | CWE-22 | Major → Blocker (write/exec) | +| CSRF protection disabled | A01 | CWE-352 | Major | +| SSRF (fetch user-supplied URL) | A10 | CWE-918 | Major → Blocker (internal reach) | +| Unsafe deserialization | A08 | CWE-502 | Blocker | +| XXE (DTD/external entities enabled) | A05 | CWE-611 | Major → Blocker | +| Sensitive data in logs | A09 | CWE-532/117 | Major | +| Open redirect (user-controlled target) | A01 | CWE-601 | Major | +| Weakened security config (CORS `*`, TLS verify off, debug on) | A05 | CWE-16/1188 | Major | + +*(A04 Insecure Design is intentionally omitted — architectural, not diff-demonstrable.)* + +**Per-stack grep signals** (cheat sheets named for grounding): +- **Java/Spring** *(Deserialization, XXE Prevention, Injection Prevention in Java)*: `ObjectInputStream`/`readObject`, `XMLDecoder`, `XStream` (CWE-502); XML parse without `disallow-doctype-decl`/`SUPPORT_DTD=false` (CWE-611); `SpelExpressionParser`/SpEL `#{…}` on user input (CWE-917); `createQuery`/`@Query` string-concatenated JPQL/SQL (CWE-89); `Runtime.exec`/`ProcessBuilder` concatenated args (CWE-78); endpoint missing `@PreAuthorize`/`@Secured` (CWE-862). +- **Ruby/Rails** *(Ruby on Rails, Mass Assignment)*: `where("…#{}")`/`find_by_sql`/`order(params…)` (CWE-89); `params` without `.permit`, or `permit!` (CWE-915, mass assignment); `.html_safe`/`raw` on user data (CWE-79); `send`/`public_send`/`constantize` on user input (CWE-94/470); `skip_before_action :verify_authenticity_token`/`protect_from_forgery except:` (CWE-352); `Marshal.load`/`YAML.load` (use `safe_load`)/`eval`/backticks (CWE-502/78); `redirect_to params[:url]` without allowlist (CWE-601). +- **Python** *(Deserialization, Injection Prevention)*: `eval`/`exec`/`pickle.loads`/`yaml.load` without `SafeLoader` (CWE-502/94); `subprocess(…, shell=True)`/`os.system`/`os.popen` (CWE-78); SQL via f-string/`%`/`.format` into `.execute()` (CWE-89); Jinja `autoescape=False`/`|safe` on user data (CWE-79); `requests`/urllib on a user-supplied URL (CWE-918). +- **TS/Angular/Node-React** *(XSS Prevention, DOM-based XSS Prevention)*: Angular `[innerHTML]`/`bypassSecurityTrust*`, React `dangerouslySetInnerHTML` (CWE-79); `innerHTML`/`document.write`/`eval`/`new Function`/`setTimeout("str")` (CWE-79/95); recursive merge / bracket-assign from user keys `__proto__`/`constructor` (CWE-1321); Node `fetch`/`axios` to a user-supplied URL (CWE-918). + +`Blocker` = a real exploitable issue, committed secret, or broken authz reachable by users. State the **source, the sink, and the line** in each finding. + +--- + +# ACCESSIBILITY + +**Applies only to UI-surface PRs** — web templates/components, server-rendered views (`.html`, `.erb`/`.haml`/`.slim`, `.vue`, `.jsx`/`.tsx`, Angular templates). **Skip entirely** for pure API/backend/data/migration/infra PRs (no rendered markup in the diff). + +Defer to the repo's own a11y docs (e.g. `docs/accessibility.md`) if present; otherwise use this floor (all **Major** when they block users): +- Icon-only control (button/link) with no accessible name (`aria-label`/`aria-labelledby`/visible text). +- Information conveyed by color alone (status/error/required) with no text/icon/pattern backup. +- Form input without an associated `