From d4a0bdb158eed2f0332098337d196156cf57952a Mon Sep 17 00:00:00 2001 From: dan Date: Wed, 31 Dec 2025 18:18:01 -0500 Subject: [PATCH] feat: add 5 new code-review lenses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New lenses: security, error-handling, coupling, boundaries, evolvability Updated SKILL.md and lenses/README.md to reflect 9 total lenses 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- .beads/issues.jsonl | 2 + skills/code-review/SKILL.md | 14 +++++- skills/code-review/lenses/README.md | 8 ++-- skills/code-review/lenses/boundaries.md | 48 +++++++++++++++++++++ skills/code-review/lenses/coupling.md | 45 +++++++++++++++++++ skills/code-review/lenses/error-handling.md | 46 ++++++++++++++++++++ skills/code-review/lenses/evolvability.md | 48 +++++++++++++++++++++ skills/code-review/lenses/security.md | 47 ++++++++++++++++++++ 8 files changed, 253 insertions(+), 5 deletions(-) create mode 100644 skills/code-review/lenses/boundaries.md create mode 100644 skills/code-review/lenses/coupling.md create mode 100644 skills/code-review/lenses/error-handling.md create mode 100644 skills/code-review/lenses/evolvability.md create mode 100644 skills/code-review/lenses/security.md diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index 44aaa7c..ab3e31c 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -29,6 +29,7 @@ {"id":"skills-8cc","title":"Remove dead code: unused ARGS variable","description":"File: .specify/scripts/bash/create-new-feature.sh\n\nLine 8: ARGS=() declared but never used\nLine 251: export SPECIFY_FEATURE - unclear if used downstream\n\nFix:\n- Remove unused ARGS declaration\n- Verify SPECIFY_FEATURE is used or remove\n\nSeverity: LOW","status":"closed","priority":4,"issue_type":"task","created_at":"2025-12-24T02:50:59.332192076-05:00","updated_at":"2025-12-29T18:38:03.48883384-05:00","closed_at":"2025-12-29T18:38:03.48883384-05:00","close_reason":"Invalid: ARGS is used (line 58, 64). SPECIFY_FEATURE is used by common.sh for feature detection. No dead code."} {"id":"skills-8d4","title":"Compare CLI_REFERENCE.md with upstream","status":"closed","priority":2,"issue_type":"task","created_at":"2025-12-03T20:15:53.268324087-08:00","updated_at":"2025-12-03T20:17:26.552616779-08:00","closed_at":"2025-12-03T20:17:26.552616779-08:00","dependencies":[{"issue_id":"skills-8d4","depends_on_id":"skills-ebh","type":"discovered-from","created_at":"2025-12-03T20:15:53.27265681-08:00","created_by":"daemon","metadata":"{}"}]} {"id":"skills-8d9","title":"Add conversational patterns to orch skill","description":"## Context\nThe orch skill currently documents consensus and single-shot chat, but doesn't\nteach agents how to use orch for multi-turn conversations with external AIs.\n\n## Goal\nAdd documentation and patterns for agent-driven conversations where the calling\nagent (Claude Code) orchestrates multi-turn dialogues using orch primitives.\n\n## Patterns to document\n\n### Session-based multi-turn\n```bash\n# Initial query\nRESPONSE=$(orch chat \"Analyze this\" --model claude --format json)\nSESSION=$(echo \"$RESPONSE\" | jq -r .session_id)\n\n# Continue conversation\norch chat \"Elaborate on X\" --model claude --session $SESSION\n\n# Inspect state\norch sessions info $SESSION\norch sessions show $SESSION --last 2 --format text\n```\n\n### Cross-model dialogue\n```bash\n# Get one model's take\nCLAUDE=$(orch chat \"Review this\" --model claude --format json)\nCLAUDE_SAYS=$(echo \"$CLAUDE\" | jq -r '.responses[0].content')\n\n# Ask another model to respond\norch chat \"Claude said: $CLAUDE_SAYS\n\nWhat's your perspective?\" --model gemini\n```\n\n### When to use conversations vs consensus\n- Consensus: quick parallel opinions on a decision\n- Conversation: deeper exploration, follow-up questions, iterative refinement\n\n## Files\n- skills/orch/SKILL.md\n\n## Related\n- orch-c3r: Design: Session introspection for agent-driven conversations (in orch repo)","status":"closed","priority":2,"issue_type":"feature","created_at":"2025-12-18T19:57:28.201494288-08:00","updated_at":"2025-12-29T15:34:16.254181578-05:00","closed_at":"2025-12-29T15:34:16.254181578-05:00","close_reason":"Added conversational patterns section to orch SKILL.md: sessions, cross-model dialogue, iterative refinement, consensus vs chat guidance."} +{"id":"skills-8ma","title":"worklog skill: remove org-mode references, use markdown instead","description":"The worklog skill currently references org-mode format (.org files) in the template and instructions. Update to use markdown (.md) instead:\n\n1. Update ~/.claude/skills/worklog/templates/worklog-template.org → worklog-template.md\n2. Convert org-mode syntax to markdown (#+TITLE → # Title, * → ##, etc.)\n3. Update skill instructions to reference .md files\n4. Update suggest-filename.sh to output .md extension\n\nContext: org-mode is less widely supported than markdown in tooling and editors.","status":"open","priority":2,"issue_type":"task","created_at":"2025-12-31T08:43:55.761429693-05:00","created_by":"dan","updated_at":"2025-12-31T09:31:19.374921191-05:00"} {"id":"skills-8v0","title":"Consolidate skill list definitions (flake.nix + ai-skills.nix)","description":"Skill list duplicated in:\n- flake.nix (lines 15-27)\n- modules/ai-skills.nix (lines 8-18)\n\nIssues:\n- Manual sync required when adding skills\n- No validation that referenced skills exist\n\nFix:\n- Single source of truth for skill list\n- Consider generating one from the other\n\nSeverity: MEDIUM","status":"open","priority":3,"issue_type":"task","created_at":"2025-12-24T02:51:14.432158871-05:00","updated_at":"2025-12-24T02:51:14.432158871-05:00"} {"id":"skills-8y6","title":"Define skill versioning strategy","description":"Git SHA alone is insufficient. Need tuple approach:\n\n- skill_source_rev: git SHA (if available)\n- skill_content_hash: hash of SKILL.md + scripts\n- runtime_ref: flake.lock hash or Nix store path\n\nQuestions to resolve:\n- Do Protos pin to versions (stable but maintenance) or float on latest (risky)?\n- How to handle breaking changes in skills?\n- Record in wisp trace vs proto definition?\n\nFrom consensus: both models flagged versioning instability as high severity.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-12-23T19:49:30.839064445-05:00","updated_at":"2025-12-23T20:55:04.439779336-05:00","closed_at":"2025-12-23T20:55:04.439779336-05:00","close_reason":"ADRs revised with orch consensus feedback"} {"id":"skills-9af","title":"spec-review: Add spike/research task handling","description":"Tasks like 'Investigate X' can linger without clear outcomes.\n\nAdd to REVIEW_TASKS:\n- Flag research/spike tasks\n- Require timebox and concrete outputs (decision record, prototype, risks)\n- Pattern for handling unknowns","status":"closed","priority":3,"issue_type":"task","created_at":"2025-12-15T00:23:26.887719136-08:00","updated_at":"2025-12-15T14:08:13.441095034-08:00","closed_at":"2025-12-15T14:08:13.441095034-08:00"} @@ -70,6 +71,7 @@ {"id":"skills-h9f","title":"spec-review: Balance negativity bias in prompts","description":"'Be critical' and 'devil's advocate' can bias toward over-flagging without acknowledging what's good.\n\nAdd:\n- 'List top 3 strongest parts of the document'\n- 'Call out where document is sufficiently clear/testable'\n- Categorize :against concerns as confirmed/plausible/rejected","status":"closed","priority":3,"issue_type":"task","created_at":"2025-12-15T00:23:26.418087998-08:00","updated_at":"2025-12-15T14:07:39.520818417-08:00","closed_at":"2025-12-15T14:07:39.520818417-08:00"} {"id":"skills-hgm","title":"Add tests for agent file update logic","description":"File: .specify/scripts/bash/update-agent-context.sh (lines 360-499)\n\nCritical logic with NO test coverage:\n- Malformed agent files (missing sections)\n- Multiple section headers on same line\n- Empty file handling\n- Timestamp update verification\n- State machine correctness\n\nRisk: HIGH - corrupts agent context files on failure\n\nFix:\n- Create test fixtures for various file states\n- Test each state transition\n- Verify idempotency\n\nSeverity: HIGH","status":"open","priority":2,"issue_type":"task","created_at":"2025-12-24T02:51:00.793493549-05:00","updated_at":"2025-12-24T02:51:00.793493549-05:00"} {"id":"skills-hh2","title":"skill: rename_symbol with LSP + validation","description":"Skill that performs safe renames:\n1. Uses LSP textDocument/rename\n2. Runs formatters\n3. Checks LSP diagnostics post-rename\n4. Opens bead if errors remain\n5. Updates doc references using hover/signatureHelp\n\nInput: new name, scope. Output: clean rename or bead with issues.","status":"open","priority":3,"issue_type":"task","created_at":"2025-12-24T02:29:56.87156069-05:00","updated_at":"2025-12-24T02:29:56.87156069-05:00","dependencies":[{"issue_id":"skills-hh2","depends_on_id":"skills-gga","type":"blocks","created_at":"2025-12-24T02:30:06.583813579-05:00","created_by":"daemon"}]} +{"id":"skills-hhz","title":"Add doc-review skill","description":"Create skill for doc-review CLI tool (~/proj/doc-review). Tool lints markdown docs for AI agent consumption using Vale + LLM hybrid architecture. Needs: 1) flake.nix in doc-review project, 2) skill SKILL.md, 3) register in availableSkills","status":"closed","priority":2,"issue_type":"task","created_at":"2025-12-30T11:24:41.321478346-05:00","created_by":"dan","updated_at":"2025-12-31T00:00:30.74314365-05:00","closed_at":"2025-12-31T00:00:30.74314365-05:00","close_reason":"doc-review skill created in skills/doc-review/SKILL.md"} {"id":"skills-hin","title":"ADR: Skills and Molecules Integration Design","description":"Write Architecture Decision Record documenting:\n- Current state (skills via Nix/direnv, molecules via beads)\n- Decision to link via skill: references\n- Wisp trace format spec\n- Elevation pipeline design\n- Anti-patterns to avoid\n\nLocation: docs/adr/001-skills-molecules-integration.md\n\nMigrated from dotfiles-dn8 (was in_progress).\n\n## Current State\n- ADR-001 drafted (high-level integration)\n- ADR-002, 003, 004 drafted and revised (manifest, versioning, security)\n- Orch consensus feedback incorporated\n\n## Parked\nDeferring finalization until we see Steve Yegge's new orchestration work and how it might inform our design.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-12-23T19:20:47.568903148-05:00","updated_at":"2025-12-28T23:27:42.117722575-05:00","closed_at":"2025-12-28T23:27:42.117722575-05:00","close_reason":"Parked: ADR updated to 'Parked' status. Current simpler approach (skills as standalone, agent judgment) works well. Molecules not actively used. Revisit when orchestration needs grow."} {"id":"skills-idb","title":"Handle concurrency and multi-agent execution","description":"Not addressed in ADR. Questions:\n\n- What happens when two agents run same skill on same mol step?\n- How to handle partial failures and resumptions?\n- Trace merging: append-only log vs latest-wins?\n\nNeeds:\n- execution_id and parent_execution_id in traces\n- Step completion idempotency declaration\n- Define how multiple traces attach to one mol node\n\nCan defer until basic integration works.","status":"closed","priority":4,"issue_type":"task","created_at":"2025-12-23T19:49:59.608603168-05:00","updated_at":"2025-12-29T14:37:35.350225933-05:00","closed_at":"2025-12-29T14:37:35.350225933-05:00","close_reason":"Parked: waiting on gastown (Steve Yegge's orchestration layer for beads). Revisit when gastown lands."} {"id":"skills-j2a","title":"worklog: consolidate git commands into extract-metrics.sh","description":"Context Gathering section has raw git commands, but extract-metrics.sh also exists. Feature envy - split logic. Move all git context gathering into the script, skill makes single call. Found by smells lens review.","status":"closed","priority":3,"issue_type":"task","created_at":"2025-12-25T02:03:16.478103649-05:00","updated_at":"2025-12-27T10:11:48.158176684-05:00","closed_at":"2025-12-27T10:11:48.158176684-05:00","close_reason":"Closed"} diff --git a/skills/code-review/SKILL.md b/skills/code-review/SKILL.md index af57574..a9b1e73 100644 --- a/skills/code-review/SKILL.md +++ b/skills/code-review/SKILL.md @@ -1,6 +1,6 @@ --- name: code-review -description: Run multi-lens code review on target files. Analyzes for bloat, smells, dead-code, and redundancy. Interactive - asks before filing issues. +description: Run multi-lens code review on target files. Analyzes for bloat, smells, dead-code, redundancy, security, error-handling, coupling, boundaries, and evolvability. Interactive - asks before filing issues. --- # Code Review Skill @@ -34,6 +34,11 @@ Lenses are focused review prompts located in `~/.config/lenses/`: | `smells.md` | Code smells, naming, control flow, readability | | `dead-code.md` | Unused exports, zombie code, unreachable paths | | `redundancy.md` | Duplication, parallel systems, YAGNI violations | +| `security.md` | Injection, auth gaps, secrets, crypto misuse | +| `error-handling.md` | Swallowed errors, missing handling, failure modes | +| `coupling.md` | Tight coupling, circular deps, layer violations | +| `boundaries.md` | Layer violations, dependency direction, domain cohesion | +| `evolvability.md` | Hard-coded policies, missing seams, change amplification | ## Workflow @@ -100,12 +105,17 @@ The skill produces: ``` User: /code-review src/cli.py -Agent: I'll review src/cli.py with 4 lenses (bloat, smells, dead-code, redundancy). +Agent: I'll review src/cli.py with 9 lenses. [Running bloat lens...] [Running smells lens...] [Running dead-code lens...] [Running redundancy lens...] +[Running security lens...] +[Running error-handling lens...] +[Running coupling lens...] +[Running boundaries lens...] +[Running evolvability lens...] ## Review Summary: src/cli.py diff --git a/skills/code-review/lenses/README.md b/skills/code-review/lenses/README.md index 8595da2..2c48750 100644 --- a/skills/code-review/lenses/README.md +++ b/skills/code-review/lenses/README.md @@ -34,14 +34,16 @@ done | `smells.md` | Code smells, readability, naming issues | | `dead-code.md` | Unused exports, zombie code, unreachable paths | | `redundancy.md` | Duplicate systems, YAGNI, consolidation opportunities | +| `security.md` | Injection, auth gaps, secrets, crypto misuse | +| `error-handling.md` | Swallowed errors, missing handling, failure modes | +| `coupling.md` | Tight coupling, circular deps, layer violations | +| `boundaries.md` | Layer violations, dependency direction, domain cohesion | +| `evolvability.md` | Hard-coded policies, missing seams, change amplification | ## Planned Lenses - `coverage.md` - Test gaps, untested critical paths -- `coupling.md` - Tight coupling, leaky abstractions -- `security.md` - OWASP, injection, auth - `perf.md` - Bottlenecks, N+1 queries -- `architecture.md` - Boundaries, data flow, trade-offs ## Output Convention diff --git a/skills/code-review/lenses/boundaries.md b/skills/code-review/lenses/boundaries.md new file mode 100644 index 0000000..4cee959 --- /dev/null +++ b/skills/code-review/lenses/boundaries.md @@ -0,0 +1,48 @@ +# Boundaries Review Lens + +Review the provided code for **architectural boundary violations and misplaced responsibilities**. + +## What to Look For + +### Layer Violations +- Domain/business logic in controllers, handlers, or CLI commands +- Infrastructure imports (ORM, HTTP, cloud SDKs) in domain modules +- UI/API layer directly calling repositories, bypassing service layer +- Database queries or SQL fragments in business logic + +### Dependency Direction +- Inner layers importing outer layers (domain importing infrastructure) +- Stable modules depending on volatile modules +- Core logic importing presentation or delivery concerns +- Framework dependencies leaking into domain code + +### Domain Cohesion +- Anemic models (data bags with getters/setters, logic elsewhere) +- Business rules scattered across manager/helper/util classes +- Single class handling unrelated domains (User + PDFExport) +- Domain concepts represented as primitives instead of types + +### Responsibility Placement +- Validation logic in wrong layer (UI vs domain vs persistence) +- Authorization checks scattered instead of centralized +- Formatting/presentation logic in data layer +- Configuration parsing mixed with business logic + +## Output Format + +For each finding: +``` +[BOUNDARY] +Type: +Issue: +Suggest: +``` + +## Guidelines + +- HIGH = architectural constraint violated, hard to test/change +- MED = responsibility drift, refactor when touching this code +- LOW = minor placement issue, could be cleaner +- Focus on *where* code belongs, not *how* it's coupled (that's coupling.md) +- Consider the intended architecture (MVC, Clean, Hexagonal) if evident +- Some boundary crossing is pragmatic - flag violations of *stated* patterns diff --git a/skills/code-review/lenses/coupling.md b/skills/code-review/lenses/coupling.md new file mode 100644 index 0000000..a6cd66e --- /dev/null +++ b/skills/code-review/lenses/coupling.md @@ -0,0 +1,45 @@ +# Coupling Review Lens + +Review the provided code for **tight coupling, dependency issues, and boundary violations**. + +## What to Look For + +### Tight Coupling +- Direct instantiation instead of dependency injection +- Concrete types where interfaces would allow flexibility +- Functions reaching deep into other modules' internals +- Changes in one module requiring changes in many others + +### Circular Dependencies +- Module A imports B, B imports A (direct cycle) +- Longer cycles: A → B → C → A +- Runtime cycles hidden by lazy imports + +### Leaky Abstractions +- Internal types exposed in public APIs +- Implementation details in return types +- Database schemas visible in API responses + +### Layer Violations +- UI code directly calling database +- Business logic in controllers/handlers +- Cross-cutting concerns scattered instead of centralized + +## Output Format + +For each finding: +``` +[COUPLING] +Type: +Issue: +Suggest: +``` + +## Guidelines + +- HIGH = architectural violation, makes testing/changes hard +- MED = unnecessary coupling, refactor when touching this code +- LOW = minor dependency that could be cleaner +- Some coupling is necessary - flag inappropriate coupling, not all coupling +- Circular deps are almost always HIGH severity +- Depend on abstractions, not concretions diff --git a/skills/code-review/lenses/error-handling.md b/skills/code-review/lenses/error-handling.md new file mode 100644 index 0000000..b07737b --- /dev/null +++ b/skills/code-review/lenses/error-handling.md @@ -0,0 +1,46 @@ +# Error Handling Review Lens + +Review the provided code for **error handling gaps and failure mode issues**. + +## What to Look For + +### Swallowed Errors +- Empty catch blocks (catch and do nothing) +- Catch-and-log without re-raising or handling +- Silently returning null/None/default on failure +- `except Exception` or `catch (Exception e)` that's too broad + +### Missing Handling +- Unchecked return values from I/O, network, system calls +- No try/catch around operations that can throw +- Missing null checks before dereferencing +- Resource cleanup not in finally/defer/with blocks + +### Inconsistent Patterns +- Mix of exceptions and error codes in same codebase +- Some paths log errors, others don't +- Error messages with different formats or detail levels + +### Poor Error Information +- Generic messages ("Something went wrong") +- Missing context (which operation, what input) +- Stack traces exposed to end users + +## Output Format + +For each finding: +``` +[ERROR] +Type: +Issue: +Suggest: +``` + +## Guidelines + +- HIGH = silent data corruption, crashes, security implications +- MED = poor debuggability, inconsistent user experience +- LOW = could be more defensive +- Consider: what happens when the happy path fails? +- Check for cleanup/resource leaks on error paths +- Empty catch with comment explaining why is acceptable (rare) diff --git a/skills/code-review/lenses/evolvability.md b/skills/code-review/lenses/evolvability.md new file mode 100644 index 0000000..9a4dee4 --- /dev/null +++ b/skills/code-review/lenses/evolvability.md @@ -0,0 +1,48 @@ +# Evolvability Review Lens + +Review the provided code for **barriers to change and missing extension points**. + +## What to Look For + +### Hard-coded Policies +- Long if/else or switch statements branching on type/mode/status +- Business rules embedded in code instead of configuration +- Magic numbers/strings representing changeable policies +- Behavior variations without strategy/plugin pattern + +### Concretion Over Abstraction +- Direct instantiation of dependencies (`new S3Client()`) +- Concrete types where interfaces would enable swapping +- No seam for testing or replacing implementations +- External services called directly without adapter layer + +### Change Amplification +- Adding one feature requires editing many unrelated files +- Internal types tightly mirroring external API/DB schemas +- No DTO/mapping layer between boundaries +- Widespread instanceof/type checks for the same concept + +### Missing Extension Seams +- No hooks, events, or callbacks in volatile areas +- Plugin/provider pattern would help but isn't used +- Configuration that should be runtime is compile-time +- Temporal coupling (methods must be called in specific order) + +## Output Format + +For each finding: +``` +[EVOLVE] +Type: +Issue: +Suggest: +``` + +## Guidelines + +- HIGH = new requirements will require significant rewrites +- MED = change is possible but painful, tech debt accumulating +- LOW = could be more flexible, not urgent +- Focus on *ease of change*, not current coupling (that's coupling.md) +- Not everything needs extension points - flag areas that *will* change +- Consider: "What if we need to add another X?" - is it easy or hard? diff --git a/skills/code-review/lenses/security.md b/skills/code-review/lenses/security.md new file mode 100644 index 0000000..e3c834d --- /dev/null +++ b/skills/code-review/lenses/security.md @@ -0,0 +1,47 @@ +# Security Review Lens + +Review the provided code for **security vulnerabilities and risky patterns**. + +## What to Look For + +### Injection +- SQL queries built with string concatenation +- Shell commands with unsanitized input +- eval(), exec(), or dynamic code execution with user data +- Path traversal vulnerabilities (../ in file paths) + +### Auth & Secrets +- Missing auth checks on sensitive endpoints +- Hardcoded credentials, API keys, or secrets +- Session tokens in URLs or logs +- Missing CSRF protection on state-changing operations + +### Data Exposure +- Sensitive data in error messages or logs +- Overly permissive CORS or access controls +- Missing input validation on external data +- Unsafe deserialization of untrusted data + +### Crypto Misuse +- Weak algorithms (MD5, SHA1 for security purposes) +- Hardcoded IVs or salts +- Predictable random for security tokens + +## Output Format + +For each finding: +``` +[SECURITY] +Vuln: +Issue: +Suggest: +``` + +## Guidelines + +- HIGH = exploitable vulnerability, fix before deploy +- MED = risky pattern, needs hardening +- LOW = defense-in-depth improvement +- Always suggest the secure alternative, not just "fix this" +- Consider the threat model (internal tool vs. public-facing) +- Reference OWASP Top 10 or CWE when applicable