feat: add 5 new code-review lenses

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 <noreply@anthropic.com>
2025-12-31 18:18:01 -05:00 · 2025-12-31 18:18:01 -05:00 · d4a0bdb158
parent 34afa86b77
commit d4a0bdb158
8 changed files with 253 additions and 5 deletions
--- 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"}
--- 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

--- 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

--- a/skills/code-review/lenses/boundaries.md
+++ 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] <severity:HIGH|MED|LOW> <file:line>
+Type: <layer-violation|direction|cohesion|placement>
+Issue: <what's in the wrong place>
+Suggest: <move X to Y layer, extract to domain, inject via interface>
+```
+
+## 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
--- a/skills/code-review/lenses/coupling.md
+++ 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] <severity:HIGH|MED|LOW> <file:line>
+Type: <tight|circular|leaky|layer-violation>
+Issue: <what's coupled inappropriately>
+Suggest: <introduce interface, inject dependency, extract module, invert dependency>
+```
+
+## 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
--- a/skills/code-review/lenses/error-handling.md
+++ 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] <severity:HIGH|MED|LOW> <file:line>
+Type: <swallowed|missing|inconsistent|poor-info>
+Issue: <what's wrong with error handling>
+Suggest: <add try/catch, check return value, add context, etc.>
+```
+
+## 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)
--- a/skills/code-review/lenses/evolvability.md
+++ 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] <severity:HIGH|MED|LOW> <file:line>
+Type: <hard-coded|concretion|amplification|no-seam>
+Issue: <what makes this hard to change>
+Suggest: <extract strategy, add interface, introduce adapter, externalize config>
+```
+
+## 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?
--- a/skills/code-review/lenses/security.md
+++ 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] <severity:HIGH|MED|LOW> <file:line>
+Vuln: <CWE or OWASP category if applicable>
+Issue: <what's vulnerable>
+Suggest: <parameterized query, sanitize input, use X library, etc.>
+```
+
+## 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