Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .github/workflows/validate.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,11 @@ jobs:
m = re.match(r'^---\n(.*?)\n---\n', text, re.S)
if not m:
print(f"FAIL {skill}: no YAML frontmatter"); fail = True; continue
fm = m.group(1)
if fm != fm.strip() or '\n\n' in fm:
print(f"FAIL {skill}: blank line inside frontmatter"); fail = True
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
for field in ('name', 'description'):
if not re.search(rf'^{field}:', m.group(1), re.M):
if not re.search(rf'^{field}:', fm, re.M):
print(f"FAIL {skill}: frontmatter missing {field}"); fail = True
sys.exit(1 if fail else 0)
EOF
Expand Down
22 changes: 22 additions & 0 deletions docs/roadmap.md
Original file line number Diff line number Diff line change
Expand Up @@ -147,3 +147,25 @@ Next up:
- `weekly-review` / `decision-log` (✨) — speculative `journal` siblings once `worklog` has proven out.
- `triage-issues` (♻️) — the other issue-intake candidate, still on the list.
- **Migration** — onboard a real repo (pepper or obsidian-gemini) to prove the whole suite end-to-end.

## Settled decisions (don't re-litigate)

Things that came up, got decided with a reason, and shouldn't be reopened without new information.

- **`code-review` and `typescript-patterns.md` / `review-checklist.md` stay deleted** (dropped in
#14). Claude Code ships `/code-review`, so the skill was redundant *and* the name collided. The
two reference docs went with it, and the recurring suggestion is to restore them as seed material
for `bootstrap`'s `coding.md` scaffold. **Don't.** `coding.md` is what `audit-architecture` and
any reviewer read as *"the rules this repo wrote down"*; seeding it with ~400 lines of generic
TypeScript craft would make the audits enforce generic practice — the exact boundary #15 drew
when it handed dangerous-code-patterns to `/security-review` and kept only repo-specific
invariants. Generic practice belongs to the built-in reviewer; `guidelines/*.md` is for what a
general reviewer *cannot* know. Both files remain in git history if that judgement ever changes.
- **No documentation site.** `homepage`/`repository` point at the repo and the per-plugin READMEs
(#19). 1 of 13 official plugins sets those fields at all, the repo already ships its docs as
markdown, and a generated site would add a sync surface with no checker — the failure mode #10
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
and #12 exist to prevent.
- **`auto-dev` may exceed the 5,000-word skill guideline.** It sits at ~5,200 and the remainder is
the tick state machine plus the safety invariants and hard prohibitions. Those must be in front
of the model on an unattended run that writes code; moving them to satisfy a word count would
optimise the metric against its purpose.
49 changes: 5 additions & 44 deletions plugins/audits/skills/audit-architecture/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,45 +41,9 @@ The detection table is **language-switched on `config.language`**: run the `###

If `config.guidelines.invariants` is missing or still full of `TODO` markers, note that in the report (invariant coverage is only as good as that file) and proceed with the categories you can still check.

### Python

Run this block when `config.language == "python"`.

| Category | Detection | Default routing |
| ------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------- |
| **Oversized files** | `find config.paths.source -name '*.py' -exec wc -l {} \; \| sort -rn`; flag `.py` files >500 lines (raise the bar for known-large modules — e.g. a generated-ish ORM/models file at >800, a `**/router.py` at >600; tune per repo). Don't use `wc -l config.paths.source/**/*.py` — that needs bash 4+ with `shopt -s globstar` and silently no-ops on macOS's default bash 3.2. Use `\;` not `+` so each `wc` call sees one file and skips the cumulative `N total` row that would otherwise sort to the top | **Issue** — splitting a big module is a design decision |
| **`Any` / `cast(Any, ...)` overuse** | `grep -rn ': Any\>\|-> Any\>\|cast(Any\>' config.paths.source` (ignore `from typing import Any` lines). POSIX BRE word-end anchor `\>` rather than the GNU-extension `\b` so the pattern is portable across BSD/macOS grep | **PR** if 1–3 sites in a single file with obvious correct type; **issue** otherwise |
| **`# type: ignore` / `# noqa`** | `grep -rn '# type: ignore\|# noqa' config.paths.source` | **PR** if the suppression is removable today; **issue** with explanation if not |
| **Bare / swallowed excepts** | `grep -rn 'except:\|except Exception:[[:space:]]*pass\|except Exception:[[:space:]]*\.\.\.\|except Exception:[[:space:]]*$' config.paths.source` plus reading for the `try: ... except Exception: logger.warning(...); return None` shape that silently masks bugs. (POSIX `[[:space:]]` instead of `\s` because BSD grep treats `\s` as literal in BRE; the `$`-anchored arm is listed *last* because BSD grep silently drops it from non-final alternation positions) | **Issue** — silent failure is a cardinal sin (see `config.guidelines.coding`) |
| **Missing test files** | For each `config.paths.source/<subsystem>/`, check whether *any* test under `config.paths.tests` references its module path. Subsystems with zero test imports are the strong signal | **Issue** — writing a first test for a previously-untested subsystem is non-trivial |
| **Dead exports / unused modules** | `uv run vulture config.paths.source config.paths.tests --min-confidence 80`. Pass the test root too so vulture sees test-only references and doesn't flag e.g. fixture-imported helpers as dead. Triage the report: vulture over-reports on FastAPI route handlers (decorator-registered, never imported by name), pydantic field defaults, SQLAlchemy column attributes, and anything in `app.state` wiring — verify each hit by grepping `config.paths.source` + `config.paths.tests` for the symbol before routing it | **PR** — deletion is mechanical and reversible |
| **`print()` in library code** | `grep -rn '^[[:space:]]*print(' config.paths.source` (excluding a CLI/`scripts/` dir if one exists; CLI scripts may legitimately print). Convention is `logger = logging.getLogger(__name__)` | **PR** — mechanical replacement |
| **Naive `datetime` usage** | `grep -rn 'datetime\.now()\|datetime\.utcnow()\|\.astimezone([[:space:]]*)' config.paths.source`. `datetime.utcnow()` is deprecated in 3.13; `dt.astimezone()` with no argument resolves to the container's local zone (UTC in prod — silently misrenders for non-UTC users — see `config.guidelines.invariants`) | **PR** if local fix; **issue** if it touches the user-facing render path |
| **Sync DB calls in async paths** | `grep -rn 'session\.execute\|session\.commit\|session\.flush\|session\.add' config.paths.source` and visually confirm each is `await`-ed. In an all-async codebase a missing `await` is a latent bug | **PR** — adding `await` is mechanical |
| **DRY violations** | Manual reading: look for near-duplicate helper functions, repeated control-flow blocks (>10 lines duplicated >2 places), parallel `if`/`match` ladders, copy-pasted third-party client setup, copy-pasted header parsing | **Issue** — extraction is a design decision |
| **Weak abstractions** | Manual reading: "god" service classes (>15 public methods), routers that mix unrelated concerns, settings objects passed everywhere instead of focused dependencies, `**kwargs` plumbing where a typed dataclass would do | **Issue** |
| **Improper typing** | `Optional[X]` instead of `X \| None` (UP007 should catch — skip if so); `dict`/`list` without parameters in signatures; index signatures (`dict[str, Any]`) where a `TypedDict` or pydantic model would carry the invariant | **PR** if local fix; **issue** if structural |

You're not limited to this table — if a senior Python reviewer would flag something else (mutable default arguments, shared mutable state in module globals, `asyncio.create_task` without a reference, swallowed task exceptions), capture it. Just keep the routing rule: mechanical and small → PR; structural or judgment-heavy → issue.

### TypeScript

Run this block when `config.language == "typescript"`.

| Category | Detection | Default routing |
| ------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ----------------------------------------------------------------------------------- |
| **Oversized files** | `wc -l config.paths.source/**/*.ts`; flag `.ts` files >700 lines (>1000 for the main entry file, e.g. `src/main.ts`) | **Issue** — splitting a big file is a design decision |
| **`any` / `as any` overuse** | `grep -rn ': any\b\|as any' config.paths.source` | **PR** if 1–3 sites in a single file with obvious correct type; **issue** otherwise |
| **`@ts-ignore` / `@ts-expect-error`** | `grep -rn '@ts-ignore\|@ts-expect-error' config.paths.source` | **PR** if the suppression is removable today; **issue** with explanation if not |
| **Missing test files** | For each `config.paths.source/**/*.ts` (excluding barrels, types, declarations), check the `config.paths.tests/**/*.test.ts` mirror exists | **Issue** — writing tests for a previously-untested module is non-trivial |
| **Dead exports** | `npm run knip` (or `grep` for each exported symbol's references across `config.paths.source` and `config.paths.tests`); flag exports with 0 external references that aren't entry points or re-exported via the index barrel | **PR** — deletion is mechanical and reversible |
| **DRY violations** | Manual reading: look for near-duplicate helper functions, repeated control-flow blocks (>10 lines duplicated >2 places), parallel `if`/`switch` ladders | **Issue** — extraction is a design decision |
| **Weak abstractions** | Manual reading: look for "god" interfaces (>15 members), classes that mix unrelated responsibilities, settings objects passed everywhere instead of focused dependencies | **Issue** |
| **Improper typing** | `Object`, `Function`, `{}` as types; non-null assertions (`!`) in non-trivial spots; index signatures where a discriminated union would do | **PR** if local fix; **issue** if structural |
| **Console misuse** | `grep -rn 'console\.\(log\|debug\|error\|warn\)' config.paths.source` — repo convention is a structured logger, not `console` (see `config.guidelines.coding` / `config.guidelines.invariants`) | **PR** — mechanical replacement |
| **Circular imports** | `grep` for the known smell: `import { X } from './foo'` in a file that `foo` also imports from | **Issue** |

You're not limited to this table — if a senior TypeScript reviewer would flag something else (dead branches, swallowed errors, magic numbers in agent loops), capture it. Just keep the routing rule: mechanical and small → PR; structural or judgment-heavy → issue.
Detection mechanics per language — the greps, tools and thresholds — live in
[`references/language-detection.md`](references/language-detection.md). Read the block matching
`config.language`; the judgment about what counts as a finding stays in the table above.
Comment thread
coderabbitai[bot] marked this conversation as resolved.

## What it does NOT look for

Expand Down Expand Up @@ -377,8 +341,5 @@ Tune the per-run caps (`config.audits.prCap` / `config.audits.issueCap`) downwar

## When integrated with scheduling

This skill is **not** part of the `daily-update` meta-skill, because `daily-update` bundles its work into one PR and this skill explicitly opens many. Schedule it as its own slot (e.g. nightly at 2am local time) via the `schedule` skill. The schedule should invoke this skill directly; there is no autonomous-prompt variant — pass a literal `/audit-architecture` or equivalent.

If the user is running short on `/schedule` slots and wants to combine with `daily-update`, the right consolidation is to have this skill run *first*, produce its PRs/issues, and then let `daily-update` run its own one-PR sweep on top — but they remain logically separate runs from the maintainer's point of view.

**Model tier:** DRY/abstraction judgment, invariant drift, and PR-vs-issue routing are judgment-heavy — schedule this on the **`capable`** tier (a smaller model mis-routes and over-files). See [`../../references/model-tiers.md`](../../references/model-tiers.md).
Cadence, the `daily-update` relationship, and the model tier this run wants are in
[`references/scheduling.md`](references/scheduling.md).
Loading
Loading