diff --git a/agents/code-reviewer.md b/agents/code-reviewer.md index 3a6a92b..edaa756 100644 --- a/agents/code-reviewer.md +++ b/agents/code-reviewer.md @@ -1,19 +1,45 @@ --- name: code-reviewer -description: Reviews code diffs against section plans. Used by /deep-implement for section review. +description: Adversarial code reviewer for /deep-implement section diffs. Finds issues that tests and linters cannot catch. tools: Read, Grep, Glob model: inherit --- -You are a code reviewer for the deep-implement workflow. +You are an adversarial code reviewer. You HATE this code. Your job is to find every issue that automated tests and linters cannot catch. You will receive two file paths: 1. **Section plan** - The specification describing what should be built and why -2. **Diff file** - The actual code changes as a result of the section plan +2. **Diff file** - The actual code changes -Read both files. Reconcile the implementation and the plan. +Read both files. Then tear the implementation apart. -Pretend you're a senior architect who hates this implementation. What would you criticize? What is missing? +## Universal Checks (all runtimes) + +1. **Security** — injection, hardcoded secrets, unsafe deserialization, path traversal, missing auth checks +2. **Error handling** — swallowed errors, missing nil/null checks, errors that propagate without context +3. **Contract compliance** — does the implementation match the plan? Missing requirements? Extra scope? +4. **Scope violations** — files or features not described in the section plan +5. **Missing tests** — untested error paths, edge cases, missing assertions for key behavior +6. **Concurrency** — race conditions, shared mutable state without synchronization +7. **Resource leaks** — unclosed handles, missing defers/finally, connection pool exhaustion + +## Go-Specific Checks + +When reviewing Go code, also check: + +- **Hex-arch imports**: inner layers MUST NOT import outer layers (domain → ports → service → handler → adapter). Flag any `import` of an outer package from an inner one. +- **File sizes by layer**: ports ≤100 lines, domain ≤250, service/adapter ≤300, handler ≤250, test ≤500. Flag violations. +- **Function length**: non-test ≤75 lines, test ≤150 lines. Flag violations. +- **Error wrapping**: errors crossing package boundaries must wrap with `fmt.Errorf("context: %w", err)`. Flag bare `return err` across boundaries. +- **Prohibited patterns**: `panic()` outside init, `os.Exit()` outside main, `log.Fatal` in library code. + +## Python-Specific Checks + +When reviewing Python code, also check: + +- **Type hints** — public functions without type annotations +- **Resource management** — file/connection handles without `with` or explicit close +- **Circular imports** — import patterns that will fail at runtime ## Output Format @@ -28,8 +54,9 @@ Return a JSON object with this EXACT structure: ## Rules -1. Return ONLY the JSON object - no preamble or explanation -2. Be specific - reference exact line numbers/function names -3. Prioritize high-severity issues (security, data loss, crashes) -4. Check implementation against the plan's requirements -5. If no issues found, return that the implementation looked good +1. Return ONLY the JSON object — no preamble or explanation +2. Be specific — reference exact file paths, line numbers, and function names +3. **Severity order**: security > correctness > contract compliance > architecture > style +4. Every finding must include: what's wrong, where (file:line or function), and why it matters +5. If the code is genuinely solid, say so — but look harder first +6. Do NOT flag style preferences that linters already catch (formatting, naming conventions) diff --git a/hooks/hooks.json b/hooks/hooks.json index c569a17..2554230 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -5,7 +5,7 @@ "hooks": [ { "type": "command", - "command": "uv run ${CLAUDE_PLUGIN_ROOT}/scripts/hooks/capture-session-id.py" + "command": "uv run --script ${CLAUDE_PLUGIN_ROOT}/scripts/hooks/capture-session-id.py" } ] } diff --git a/scripts/checks/setup_implementation_session.py b/scripts/checks/setup_implementation_session.py index ae96f75..e08ce3e 100644 --- a/scripts/checks/setup_implementation_session.py +++ b/scripts/checks/setup_implementation_session.py @@ -22,7 +22,7 @@ sys.path.insert(0, str(Path(__file__).parent.parent.parent)) from scripts.lib.config import load_session_config, save_session_config, create_session_config -from scripts.lib.sections import parse_manifest_block, parse_project_config_block, validate_section_file, get_completed_sections +from scripts.lib.sections import parse_manifest_block, parse_project_config_block, parse_section_concerns, sort_sections_by_concern, validate_section_file, get_completed_sections from scripts.lib.task_storage import TaskToWrite, write_tasks, build_dependency_graph, TaskStatus from scripts.lib.task_reconciliation import TaskListContext from scripts.lib.impl_tasks import ( @@ -377,6 +377,87 @@ def check_pre_commit_hooks(git_root: Path) -> dict: } +def check_github_remote(git_root: Path) -> dict: + """ + Check for GitHub remote and parse owner/repo. + + Args: + git_root: Git repository root + + Returns: + {"has_remote": bool, "remote_url": str | None, "owner_repo": str | None} + """ + try: + result = subprocess.run( + ["git", "remote", "get-url", "origin"], + cwd=git_root, + capture_output=True, + text=True, + ) + if result.returncode == 0: + url = result.stdout.strip() + # Parse owner/repo from various URL formats: + # https://github.com/owner/repo.git + # git@github.com:owner/repo.git + # https://github.com/owner/repo + match = re.search(r"github\.com[:/]([^/]+/[^/\s]+?)(?:\.git)?$", url) + if match: + return { + "has_remote": True, + "remote_url": url, + "owner_repo": match.group(1), + } + # Non-GitHub remote + return {"has_remote": True, "remote_url": url, "owner_repo": None} + except Exception: + pass + return {"has_remote": False, "remote_url": None, "owner_repo": None} + + +def check_gh_cli() -> dict: + """ + Check if GitHub CLI is installed and authenticated. + + Returns: + {"installed": bool, "authenticated": bool, "gh_version": str | None} + """ + # Check installation + try: + version_result = subprocess.run( + ["gh", "--version"], + capture_output=True, + text=True, + ) + if version_result.returncode != 0: + return {"installed": False, "authenticated": False, "gh_version": None} + except FileNotFoundError: + return {"installed": False, "authenticated": False, "gh_version": None} + + # Parse version string (first line: "gh version X.Y.Z (date)") + gh_version = None + first_line = version_result.stdout.strip().split("\n")[0] + version_match = re.search(r"(\d+\.\d+\.\d+)", first_line) + if version_match: + gh_version = version_match.group(1) + + # Check authentication + try: + auth_result = subprocess.run( + ["gh", "auth", "status"], + capture_output=True, + text=True, + ) + authenticated = auth_result.returncode == 0 + except Exception: + authenticated = False + + return { + "installed": True, + "authenticated": authenticated, + "gh_version": gh_version, + } + + def detect_section_review_state(state_dir: Path, section_name: str) -> dict: """ Detect the code review state for a specific section. @@ -718,6 +799,17 @@ def main(): sections = validation["sections"] project_config = validation["project_config"] + # Concern-based ordering (opt-in via PROJECT_CONFIG) + concern_ordering = project_config.get("concern_ordering", "").lower() == "true" + index_content = (sections_dir / "index.md").read_text() + section_concerns = {} + execution_order = list(sections) # default: manifest order + if concern_ordering: + section_concerns = parse_section_concerns(index_content) + if section_concerns: + execution_order = sort_sections_by_concern(sections, section_concerns) + sections = execution_order + # State directory (sibling to sections) for session config and reviews state_dir = sections_dir.parent / "implementation" @@ -745,6 +837,21 @@ def main(): # Check pre-commit hooks pre_commit = check_pre_commit_hooks(git_root) + # Check GitHub availability + github_remote = check_github_remote(git_root) + gh_cli = check_gh_cli() + github_available = ( + github_remote.get("owner_repo") is not None + and gh_cli.get("installed", False) + and gh_cli.get("authenticated", False) + ) + github_info = { + "available": github_available, + "owner_repo": github_remote.get("owner_repo"), + "gh_installed": gh_cli.get("installed", False), + "gh_authenticated": gh_cli.get("authenticated", False), + } + # Infer session state state = infer_session_state(sections_dir, state_dir, git_root) @@ -785,6 +892,17 @@ def main(): session_id_matched = (context_session_id == env_session_id) # Context values for task generation + # GitHub state is populated later by SKILL.md after user opt-in; + # on resume, read from existing config + existing_config = load_session_config(state_dir) if state["mode"] == "resume" else None + github_enabled = "false" + github_issue = "none" + if existing_config and existing_config.get("github", {}).get("enabled"): + github_enabled = "true" + issue_num = existing_config["github"].get("issue_number") + if issue_num: + github_issue = str(issue_num) + context_values = { "plugin_root": str(plugin_root), "sections_dir": str(sections_dir), @@ -792,6 +910,9 @@ def main(): "state_dir": str(state_dir), "runtime": project_config["runtime"], "test_command": project_config["test_command"], + "github_enabled": github_enabled, + "github_issue": github_issue, + "concern_ordering": str(concern_ordering).lower(), } # Generate implementation tasks @@ -834,11 +955,15 @@ def main(): "dirty_files": working_tree["dirty_files"], "commit_style": commit_style, "pre_commit": pre_commit, + "github": github_info, "project_config": project_config, "sections": sections, "completed_sections": state["completed_sections"], "resume_from": state["resume_from"], "resume_section_state": state.get("resume_section_state"), + "concern_ordering": concern_ordering, + "section_concerns": section_concerns, + "execution_order": execution_order, "tasks_written": write_result.tasks_written if write_result else 0, "task_write_error": task_write_error, # Session ID diagnostics diff --git a/scripts/checks/validate_go_guardrails.py b/scripts/checks/validate_go_guardrails.py new file mode 100644 index 0000000..0cc3736 --- /dev/null +++ b/scripts/checks/validate_go_guardrails.py @@ -0,0 +1,367 @@ +#!/usr/bin/env python3 +"""Go code guardrail validation for deep-implement. + +Validates Go source files against hexagonal architecture constraints: +file sizes, function lengths, import direction, and prohibited patterns. + +Ported from devx-design-crew/impl_guardrails.py — simplified for +deep-implement (no exception system, no LLM fallback). + +Usage: + uv run scripts/checks/validate_go_guardrails.py --target-dir /path/to/repo + uv run scripts/checks/validate_go_guardrails.py --target-dir /path/to/repo --override max_file_lines=450 +""" + +import argparse +import json +import os +import re +import subprocess +import sys + +# --- Constants (from CLAUDE.md file sizing table) --- + +MAX_FILE_LINES = 300 # service/adapter files +MAX_FUNCTION_LINES = 75 # non-test functions +MAX_PORT_LINES = 100 # port interface files +MAX_ENTITY_LINES = 250 # domain entity files +MAX_HANDLER_LINES = 250 # handler files +MAX_TEST_FILE_LINES = 500 # test files +MAX_TEST_FUNCTION_LINES = 150 # test functions + +# Regex for Go function definitions +_FUNC_RE = re.compile(r"^func\s", re.MULTILINE) + +# Regex for Go import blocks +_IMPORT_SINGLE_RE = re.compile(r'^import\s+"([^"]+)"', re.MULTILINE) +_IMPORT_BLOCK_RE = re.compile(r"^import\s*\((.*?)\)", re.MULTILINE | re.DOTALL) +_IMPORT_LINE_RE = re.compile(r'"([^"]+)"') + +# Regex for package declaration +_PACKAGE_RE = re.compile(r"^package\s+\w+", re.MULTILINE) + +# Prohibited patterns: (regex, message, skip_condition) +# skip_condition: "main" = skip in /cmd/ paths, "test" = skip in _test.go +_PROHIBITED_PATTERNS = [ + (re.compile(r'(?:password|secret|api[_-]?key)\s*[:=]\s*"[^"]{8,}"'), "Hardcoded secret detected", None), + (re.compile(r"\bos\.Exit\b"), "os.Exit() in non-main package", "cmd"), + (re.compile(r"\bpanic\b\s*\("), "panic() call (use error returns instead)", "test"), +] + +# Hex architecture layers — inner must not import outer +_HEX_LAYERS = { + "domain": 0, + "ports": 1, + "service": 2, + "handler": 3, + "adapter": 4, +} + + +# --- File discovery --- + +def discover_go_files(target_dir: str) -> list[str]: + """Find Go files to validate using git or filesystem walk.""" + go_files = [] + + # Try git diff HEAD first (changed files) + try: + result = subprocess.run( + ["git", "diff", "--name-only", "HEAD"], + capture_output=True, text=True, cwd=target_dir, timeout=10, + ) + if result.returncode == 0 and result.stdout.strip(): + for f in result.stdout.strip().splitlines(): + if f.endswith(".go"): + full = os.path.join(target_dir, f) + if os.path.isfile(full): + go_files.append(f) + if go_files: + return go_files + except (subprocess.SubprocessError, FileNotFoundError): + pass + + # Try git ls-files (all tracked files) + try: + result = subprocess.run( + ["git", "ls-files", "*.go"], + capture_output=True, text=True, cwd=target_dir, timeout=10, + ) + if result.returncode == 0 and result.stdout.strip(): + for f in result.stdout.strip().splitlines(): + full = os.path.join(target_dir, f) + if os.path.isfile(full): + go_files.append(f) + if go_files: + return go_files + except (subprocess.SubprocessError, FileNotFoundError): + pass + + # Fallback: walk filesystem + for root, _dirs, files in os.walk(target_dir): + for fname in files: + if fname.endswith(".go"): + rel = os.path.relpath(os.path.join(root, fname), target_dir) + go_files.append(rel) + + return go_files + + +# --- Validation functions --- + +def max_lines_for_file(filepath: str) -> int: + """Determine max lines based on file type/location.""" + if filepath.endswith("_test.go"): + return MAX_TEST_FILE_LINES + if "/ports/" in filepath: + return MAX_PORT_LINES + if "/domain/" in filepath: + return MAX_ENTITY_LINES + if "/handler/" in filepath: + return MAX_HANDLER_LINES + return MAX_FILE_LINES + + +def check_file_size(filepath: str, content: str) -> list[str]: + """Check file does not exceed line limit for its hex layer.""" + line_count = len(content.split("\n")) + max_lines = max_lines_for_file(filepath) + if line_count > max_lines: + return [f"{filepath}: {line_count} lines (max {max_lines})"] + return [] + + +def check_function_lengths(filepath: str, content: str) -> list[str]: + """Check no function exceeds length limit.""" + violations = [] + lines = content.split("\n") + func_starts: list[tuple[int, str]] = [] + + for i, line in enumerate(lines): + if _FUNC_RE.match(line): + rest = line[len("func "):].strip() + if rest.startswith("("): + # Method receiver: func (r *Repo) Name( + close = rest.index(")") + rest = rest[close + 1:].strip() + name = rest.split("(")[0].strip() + func_starts.append((i, name)) + + is_test = filepath.endswith("_test.go") + max_func = MAX_TEST_FUNCTION_LINES if is_test else MAX_FUNCTION_LINES + + for idx, (start, name) in enumerate(func_starts): + end = func_starts[idx + 1][0] if idx + 1 < len(func_starts) else len(lines) + func_len = end - start + if func_len > max_func: + violations.append( + f"{filepath}:{start + 1} func {name}: {func_len} lines (max {max_func})" + ) + + return violations + + +def check_package_declaration(filepath: str, content: str) -> list[str]: + """Check file has a package declaration.""" + if not _PACKAGE_RE.search(content): + return [f"{filepath}: missing package declaration"] + return [] + + +def extract_imports(content: str) -> list[str]: + """Extract all import paths from Go source.""" + imports = [] + for m in _IMPORT_SINGLE_RE.finditer(content): + imports.append(m.group(1)) + for m in _IMPORT_BLOCK_RE.finditer(content): + block = m.group(1) + for line_m in _IMPORT_LINE_RE.finditer(block): + imports.append(line_m.group(1)) + return imports + + +def get_hex_layer(filepath: str) -> tuple[str, int] | None: + """Return (layer_name, level) if file is in a hex layer, else None.""" + for layer_name, level in _HEX_LAYERS.items(): + if f"/internal/{layer_name}/" in filepath or filepath.endswith(f"/internal/{layer_name}"): + return layer_name, level + return None + + +def check_hex_layers(filepath: str, content: str) -> list[str]: + """Check hex architecture import direction (inner must not import outer).""" + violations = [] + layer_info = get_hex_layer(filepath) + if not layer_info: + return violations + + file_layer, file_level = layer_info + imports = extract_imports(content) + + for imp in imports: + for layer_name, level in _HEX_LAYERS.items(): + if f"/internal/{layer_name}" in imp and level > file_level: + violations.append( + f"{filepath}: {file_layer} layer imports {layer_name} layer ({imp})" + ) + + return violations + + +def check_cross_service_imports(filepath: str, content: str) -> list[str]: + """Check for imports of other services' internal packages.""" + violations = [] + imports = extract_imports(content) + + for imp in imports: + if "/internal/" in imp and "services/" in imp: + # Extract service name from file path + file_service = None + if "services/" in filepath: + parts = filepath.split("services/")[1].split("/") + if parts: + file_service = parts[0] + + # Extract service name from import + imp_service = None + if "services/" in imp: + parts = imp.split("services/")[1].split("/") + if parts: + imp_service = parts[0] + + if file_service and imp_service and file_service != imp_service: + violations.append( + f"{filepath}: imports internal package from service '{imp_service}': {imp}" + ) + + return violations + + +def check_self_imports(filepath: str, content: str) -> list[str]: + """Check for self-import (import cycle).""" + violations = [] + imports = extract_imports(content) + + # Get file's directory path components + file_dir = "/".join(filepath.split("/")[:-1]) + if not file_dir: + return violations + + for imp in imports: + if file_dir and imp.endswith("/" + file_dir.split("/")[-1]): + # Check the full path matches + if file_dir in imp: + violations.append( + f"{filepath}: self-import detected ({imp})" + ) + + return violations + + +def check_prohibited_patterns(filepath: str, content: str) -> list[str]: + """Check for prohibited patterns (secrets, os.Exit, panic).""" + violations = [] + is_test = filepath.endswith("_test.go") + is_cmd = "/cmd/" in filepath or filepath.startswith("cmd/") + + for pattern, message, skip in _PROHIBITED_PATTERNS: + if pattern.search(content): + if skip == "cmd" and is_cmd: + continue + if skip == "test" and is_test: + continue + violations.append(f"{filepath}: {message}") + + return violations + + +def validate_file(filepath: str, content: str) -> list[str]: + """Run all validation checks on a single file.""" + violations = [] + violations.extend(check_file_size(filepath, content)) + violations.extend(check_function_lengths(filepath, content)) + violations.extend(check_package_declaration(filepath, content)) + violations.extend(check_hex_layers(filepath, content)) + violations.extend(check_cross_service_imports(filepath, content)) + violations.extend(check_self_imports(filepath, content)) + violations.extend(check_prohibited_patterns(filepath, content)) + return violations + + +def validate_directory(target_dir: str) -> dict: + """Validate all Go files in target directory.""" + go_files = discover_go_files(target_dir) + all_violations = [] + files_checked = 0 + + for relpath in go_files: + full_path = os.path.join(target_dir, relpath) + try: + with open(full_path, encoding="utf-8") as f: + content = f.read() + except (OSError, UnicodeDecodeError): + continue + + files_checked += 1 + violations = validate_file(relpath, content) + all_violations.extend(violations) + + passed = len(all_violations) == 0 + summary = ( + f"{len(all_violations)} violation{'s' if len(all_violations) != 1 else ''} " + f"in {files_checked} Go file{'s' if files_checked != 1 else ''}" + ) + + return { + "passed": passed, + "files_checked": files_checked, + "violations": all_violations, + "summary": summary, + } + + +def apply_overrides(overrides: dict[str, str]) -> None: + """Apply CLI --override values to module constants.""" + mapping = { + "max_file_lines": "MAX_FILE_LINES", + "max_function_lines": "MAX_FUNCTION_LINES", + "max_port_lines": "MAX_PORT_LINES", + "max_entity_lines": "MAX_ENTITY_LINES", + "max_handler_lines": "MAX_HANDLER_LINES", + "max_test_file_lines": "MAX_TEST_FILE_LINES", + "max_test_function_lines": "MAX_TEST_FUNCTION_LINES", + } + for key, value in overrides.items(): + const_name = mapping.get(key) + if const_name: + globals()[const_name] = int(value) + else: + print(f"Warning: unknown override key '{key}'", file=sys.stderr) + + +def main() -> None: + parser = argparse.ArgumentParser(description="Validate Go code guardrails") + parser.add_argument("--target-dir", required=True, help="Path to Go repository") + parser.add_argument( + "--override", action="append", default=[], + help="Override a limit: key=value (e.g., max_file_lines=450)", + ) + args = parser.parse_args() + + # Parse overrides + overrides = {} + for item in args.override: + if "=" in item: + k, v = item.split("=", 1) + overrides[k.strip()] = v.strip() + + if overrides: + apply_overrides(overrides) + + result = validate_directory(args.target_dir) + print(json.dumps(result, indent=2)) + sys.exit(0 if result["passed"] else 1) + + +if __name__ == "__main__": + main() diff --git a/scripts/hooks/capture-session-id.py b/scripts/hooks/capture-session-id.py index e8aebaf..fe8bc22 100644 --- a/scripts/hooks/capture-session-id.py +++ b/scripts/hooks/capture-session-id.py @@ -1,4 +1,8 @@ #!/usr/bin/env python3 +# /// script +# requires-python = ">=3.11" +# dependencies = [] +# /// """Capture session_id and plugin_root, expose via Claude's context. This hook runs when Claude Code starts a session. It reads session_id from diff --git a/scripts/lib/config.py b/scripts/lib/config.py index c6914f5..2b2c7bb 100644 --- a/scripts/lib/config.py +++ b/scripts/lib/config.py @@ -59,6 +59,7 @@ def create_session_config( test_command: str = "uv run pytest", sections: list[str] | None = None, pre_commit: dict | None = None, + github: dict | None = None, ) -> dict: """ Create a new session config with all required fields. @@ -73,6 +74,7 @@ def create_session_config( test_command: Command to run tests sections: List of section names from manifest pre_commit: Pre-commit hook configuration dict + github: GitHub integration configuration dict Returns: New config dict @@ -93,6 +95,13 @@ def create_session_config( "may_modify_files": False, "detected_formatters": [] }, + "github": github or { + "enabled": False, + "owner_repo": None, + "base_branch": None, + "issue_number": None, + "section_prs": {}, + }, "created_at": datetime.now(timezone.utc).isoformat(), } diff --git a/scripts/lib/impl_tasks.py b/scripts/lib/impl_tasks.py index 50f6a7a..474b151 100644 --- a/scripts/lib/impl_tasks.py +++ b/scripts/lib/impl_tasks.py @@ -113,6 +113,9 @@ class TaskDefinition: "state_dir", "runtime", "test_command", + "github_enabled", + "github_issue", + "concern_ordering", ] diff --git a/scripts/lib/sections.py b/scripts/lib/sections.py index 103a777..e2a3d21 100644 --- a/scripts/lib/sections.py +++ b/scripts/lib/sections.py @@ -45,10 +45,17 @@ def parse_project_config_block(index_content: str) -> dict[str, str]: return config +VALID_CONCERNS = ["scaffold", "functional", "observability", "configuration", "resilience", "integration"] +CONCERN_ORDER = {c: i for i, c in enumerate(VALID_CONCERNS)} + + def parse_manifest_block(index_content: str) -> list[str]: """ Extract section names from SECTION_MANIFEST block. + Handles optional concern tags after section names (e.g., "section-01-foundation scaffold"). + Tags are ignored here — use parse_section_concerns() to extract them. + Args: index_content: Content of index.md file @@ -71,11 +78,114 @@ def parse_manifest_block(index_content: str) -> list[str]: # Skip empty lines and comments if not line or line.startswith('#'): continue - sections.append(line) + # Support optional concern tag: "section-01-foundation scaffold" + parts = line.split() + sections.append(parts[0]) return sections +def parse_section_concerns(index_content: str) -> dict[str, str]: + """ + Extract concern tags from SECTION_MANIFEST lines. + + Each manifest line can optionally include a concern tag: + section-01-foundation scaffold + section-02-models functional + + Args: + index_content: Content of index.md file + + Returns: + Dict mapping section name to concern type for sections that have valid tags. + Sections without tags or with invalid tags are omitted. + """ + pattern = r'' + match = re.search(pattern, index_content, re.DOTALL) + + if not match: + return {} + + manifest_content = match.group(1) + concerns = {} + + for line in manifest_content.split('\n'): + line = line.strip() + if not line or line.startswith('#'): + continue + parts = line.split() + if len(parts) >= 2 and parts[1] in VALID_CONCERNS: + concerns[parts[0]] = parts[1] + + return concerns + + +def sort_sections_by_concern(sections: list[str], concerns: dict[str, str]) -> list[str]: + """ + Reorder sections by concern type, preserving number order within same concern. + + Concern execution order: scaffold → functional → observability → + configuration → resilience → integration. + + Untagged sections go after all tagged sections, in their original order. + + Args: + sections: List of section names in manifest order + concerns: Dict mapping section name to concern type + + Returns: + Reordered list of section names. + """ + if not concerns: + return sections + + tagged = [(s, CONCERN_ORDER[concerns[s]]) for s in sections if s in concerns] + untagged = [s for s in sections if s not in concerns] + + # Sort tagged by concern order, then by original position (stable sort preserves manifest order) + tagged.sort(key=lambda x: x[1]) + + return [s for s, _ in tagged] + untagged + + +def parse_section_meta(section_content: str) -> dict[str, str]: + """ + Extract SECTION_META block from section file content. + + Expected format within a section .md file: + + + Args: + section_content: Content of a section markdown file + + Returns: + Dict with keys: concern, target_files, estimated_lines (all optional). + Returns empty dict when no SECTION_META block found. + """ + pattern = r'' + match = re.search(pattern, section_content, re.DOTALL) + + if not match: + return {} + + meta_content = match.group(1) + meta = {} + + for line in meta_content.split('\n'): + line = line.strip() + if not line or line.startswith('#'): + continue + if ':' in line: + key, value = line.split(':', 1) + meta[key.strip()] = value.strip() + + return meta + + def validate_section_file(section_path: Path) -> dict: """ Check section file exists and has content. diff --git a/scripts/tools/update_github_state.py b/scripts/tools/update_github_state.py new file mode 100644 index 0000000..01e83f4 --- /dev/null +++ b/scripts/tools/update_github_state.py @@ -0,0 +1,80 @@ +#!/usr/bin/env python3 +"""Update GitHub state in session config. + +Persists issue number and per-section PR info for resume support. + +Usage: + # Store tracking issue number + uv run {plugin_root}/scripts/tools/update_github_state.py \ + --state-dir "{state_dir}" \ + --issue-number 42 + + # Store per-section PR info + uv run {plugin_root}/scripts/tools/update_github_state.py \ + --state-dir "{state_dir}" \ + --section "section-01-foundation" \ + --pr-number 43 \ + --pr-url "https://github.com/owner/repo/pull/43" +""" + +import argparse +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).parent.parent.parent)) + +from scripts.lib.config import load_session_config, save_session_config + + +def main() -> int: + parser = argparse.ArgumentParser(description="Update GitHub state in session config") + parser.add_argument("--state-dir", required=True, help="Path to state directory") + parser.add_argument("--issue-number", type=int, help="Tracking issue number") + parser.add_argument("--section", help="Section name (required with --pr-number)") + parser.add_argument("--pr-number", type=int, help="PR number for section") + parser.add_argument("--pr-url", help="PR URL for section") + args = parser.parse_args() + + state_dir = Path(args.state_dir) + + config = load_session_config(state_dir) + if config is None: + print(f"Error: No config found in {state_dir}") + return 1 + + if "github" not in config: + config["github"] = { + "enabled": False, + "owner_repo": None, + "base_branch": None, + "issue_number": None, + "section_prs": {}, + } + + if args.issue_number is not None: + config["github"]["issue_number"] = args.issue_number + config["github"]["enabled"] = True + save_session_config(state_dir, config) + print(f"Updated issue_number={args.issue_number}") + return 0 + + if args.pr_number is not None: + if not args.section: + print("Error: --section is required with --pr-number") + return 1 + if "section_prs" not in config["github"]: + config["github"]["section_prs"] = {} + config["github"]["section_prs"][args.section] = { + "number": args.pr_number, + "url": args.pr_url or "", + } + save_session_config(state_dir, config) + print(f"Updated {args.section}: pr_number={args.pr_number}") + return 0 + + print("Error: Must provide --issue-number or --pr-number") + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/skills/deep-implement/SKILL.md b/skills/deep-implement/SKILL.md index 7f64334..71b1768 100644 --- a/skills/deep-implement/SKILL.md +++ b/skills/deep-implement/SKILL.md @@ -142,6 +142,46 @@ AskUserQuestion: If user chooses "Exit", stop the workflow. +**GitHub Integration (opt-in):** + +If the setup output has `github.available == true`: + +``` +AskUserQuestion: + question: "GitHub detected ({github.owner_repo}). Enable issue + PR workflow?" + options: + - label: "Yes — create tracking issue and PR per section" + description: "Creates a tracking issue and one PR per section for team review" + - label: "No — commit only (current behavior)" + description: "Commit to current branch, no push or PRs" +``` + +If user opts in: +1. Store `base_branch` = current branch name (PRs will target this branch) +2. Create a tracking issue with section checklist: + ```bash + gh issue create --title "deep-implement: {plan name}" --body "$(cat <<'EOF' + ## Implementation Tracking + + Sections: + - [ ] section-01-name + - [ ] section-02-name + ... + + Created by deep-implement. + EOF + )" + ``` +3. Parse issue number from output +4. Store issue number via: + ```bash + uv run {plugin_root}/scripts/tools/update_github_state.py \ + --state-dir "{state_dir}" \ + --issue-number {N} + ``` + +If GitHub is not available or user opts out, proceed with commit-only workflow (existing behavior). + ### G. Handle Working Tree Status If `working_tree_clean == false`: @@ -168,8 +208,10 @@ Working tree: {Clean | Dirty (N files)} Pre-commit: {Detected (type) | None} {May modify files: Yes (formatters) | No | Unknown} Test command: {test_command} +GitHub: {Enabled (issue #N) | Available (opted out) | Not available} Sections: {N} detected Completed: {M} already done +Concern order: {Enabled (K tagged) | Not enabled} State storage: {state_dir} ═══════════════════════════════════════════════════════════════ ``` @@ -209,7 +251,7 @@ Mark each task as `completed` when done: `TaskUpdate(taskId=X, status="completed ## Implementation Loop -For each incomplete section (in manifest order): +For each incomplete section (in execution order — concern order if enabled, otherwise manifest order): **Task milestone mapping:** | Task Subject | Workflow Steps | @@ -234,10 +276,14 @@ Update task: `TaskUpdate(taskId=X, status="in_progress")` Read {sections_dir}/section-NN-.md ``` +If the section file contains a `SECTION_META` block, note the concern type, target_files, and estimated_lines for use in guardrail validation and review context. See [concern-ordering.md](references/concern-ordering.md). + ### Step 3: Implement Section See [implementation-loop.md](references/implementation-loop.md) +**Go projects:** Read [go-guardrails.md](references/go-guardrails.md) for file size limits and architecture rules before writing code. + Follow TDD workflow: 1. Create skeleton files for imports 2. Write tests from section spec @@ -250,8 +296,23 @@ Follow TDD workflow: Maintain list of all files created during implementation. +### Step 4.5: Guardrail Validation (Go projects) + +If runtime is `go`: +1. Run `uv run {plugin_root}/scripts/checks/validate_go_guardrails.py --target-dir "{target_dir}"` +2. If passed: continue to Step 5 +3. If failed: fix violations (split files, extract helpers, fix imports), re-run tests, re-validate +4. After 2 failed retries: proceed anyway, let code reviewer flag issues + ### Step 5: Stage Changes +**If GitHub enabled:** Create a section branch before staging: +```bash +git checkout -b implement/{section-name} +``` +(e.g., `implement/section-01-foundation`). If the branch already exists (resume), check it out instead. + +**Then stage changes (always):** ```bash # Stage new files git add {created_files...} @@ -345,6 +406,54 @@ EOF )" ``` +### Step 10.5: Push and Create PR (GitHub only) + +**Skip this step if GitHub is not enabled.** + +See [pre-push-review.md](references/pre-push-review.md) and [github-workflow.md](references/github-workflow.md) + +1. **Pre-push review (optional):** Run the code-reviewer subagent against the full section diff as an aggregate quality check. If CRITICAL findings, ask user: fix / push anyway / stop. + +2. **Push the branch:** + ```bash + git push -u origin implement/{section-name} + ``` + +3. **Create PR:** + ```bash + gh pr create --title "section-NN: {name}" --body "$(cat <<'EOF' + ## Summary + {Brief description from section plan} + + Closes #{issue_number} + EOF + )" --base {base_branch} + ``` + +4. **Store PR info:** + ```bash + uv run {plugin_root}/scripts/tools/update_github_state.py \ + --state-dir "{state_dir}" \ + --section "{section_name}" \ + --pr-number {N} \ + --pr-url "{url}" + ``` + +5. **Update issue checkbox** (best-effort): + ```bash + # Fetch current body, check the section's checkbox, update + gh issue edit {issue_number} --body "$(updated body with [x] for this section)" + ``` + +6. **Print PR URL** for the user. + +7. **Return to base branch:** + ```bash + git checkout {base_branch} + ``` + +**Error handling:** If push or PR creation fails (network, permissions), print the error and continue. GitHub ops are best-effort — they never block implementation progress. + ### Step 11: Update State After successful commit, update the session config: @@ -358,6 +467,8 @@ uv run {plugin_root}/scripts/tools/update_section_state.py \ This records the commit hash so the section is recognized as complete on resume. +If GitHub is enabled and a PR was created, the PR URL is already stored via `update_github_state.py` in Step 10.5. + ### Step 12: Mark Complete Update task: `TaskUpdate(taskId=X, status="completed")` @@ -404,6 +515,7 @@ After all sections complete, see [finalization.md](references/finalization.md): 1. Generate `{state_dir}/usage.md` with usage guide for what was built 2. Print completion summary with commits, files, and next steps +3. If GitHub enabled: print summary table of all PRs with URLs and tracking issue link --- @@ -464,6 +576,11 @@ Please review the section file. The setup script detects completed sections via `deep_implement_config.json` and marks their tasks complete. You'll resume from the next pending section with fresh instructions. +**GitHub state on resume:** The config file stores `github.enabled`, `github.base_branch`, `github.issue_number`, and `github.section_prs`. On resume: +- If GitHub was enabled, continue creating PRs for remaining sections +- Check which sections already have PRs (skip push/PR for those) +- Reuse the existing tracking issue + **After compaction (if user chose "continue"):** 1. Call `TaskList` to see current state @@ -471,6 +588,8 @@ The setup script detects completed sections via `deep_implement_config.json` and - `plugin_root=...` - extract value after `=` - `sections_dir=...` - extract value after `=` - `state_dir=...` - extract value after `=` + - `github_enabled=...` - whether GitHub workflow is active + - `github_issue=...` - tracking issue number 3. Find next pending, unblocked task 4. Resume workflow from that task @@ -486,3 +605,6 @@ The setup script detects completed sections via `deep_implement_config.json` and - [git-operations.md](references/git-operations.md) - Git handling - [pre-commit-handling.md](references/pre-commit-handling.md) - Hook handling - [finalization.md](references/finalization.md) - Usage guide and completion summary +- [github-workflow.md](references/github-workflow.md) - GitHub integration (issue, branch, PR per section) +- [pre-push-review.md](references/pre-push-review.md) - Pre-push aggregate code review +- [concern-ordering.md](references/concern-ordering.md) - Concern-type execution ordering diff --git a/skills/deep-implement/references/concern-ordering.md b/skills/deep-implement/references/concern-ordering.md new file mode 100644 index 0000000..cb8d662 --- /dev/null +++ b/skills/deep-implement/references/concern-ordering.md @@ -0,0 +1,72 @@ +# Concern-Type Execution Ordering + +Concern ordering reorders sections by concern type instead of manifest number order. This prevents the most common LLM failure: referencing types or interfaces that don't exist yet. + +## Opt-In + +Set `concern_ordering: true` in the `PROJECT_CONFIG` block of `index.md`: + +```markdown + +``` + +When not set or set to `false`, sections execute in manifest number order (existing behavior). + +## The 6 Concern Types + +Executed in this order: + +| Order | Concern | Purpose | Examples | +|-------|---------|---------|----------| +| 1 | `scaffold` | Directory structure, module init, stub routes, empty interfaces | `go mod init`, empty port interfaces, directory layout | +| 2 | `functional` | Core logic, ports, domain types, service implementations | Domain entities, service methods, handler routes | +| 3 | `observability` | Structured logging, metrics, tracing | OTel instrumentation, log middleware | +| 4 | `configuration` | Env vars, secrets, feature flags | Config loaders, validation | +| 5 | `resilience` | Error handling, graceful shutdown, health checks | Circuit breakers, retry logic, liveness probes | +| 6 | `integration` | Adapter wiring, cross-service calls, end-to-end tests | DI containers, integration tests | + +## Tagging Sections + +Add a concern tag after the section name in `SECTION_MANIFEST`: + +```markdown + +``` + +- Sections with the same concern execute in manifest number order. +- Sections without a concern tag execute after all tagged sections. + +## SECTION_META Block + +Section files can include an optional metadata block for guardrail integration: + +```markdown + +``` + +Fields (all optional): +- `concern` — The concern type (redundant with manifest tag, useful as section-local context) +- `target_files` — Comma-separated list of files this section creates/modifies +- `estimated_lines` — Approximate total lines of code for guardrail calibration + +## Fallback Behavior + +- If `concern_ordering` is absent or `false` in PROJECT_CONFIG: manifest number order +- If `concern_ordering` is `true` but no sections have concern tags: manifest number order +- Mixed tagged/untagged: tagged sections first (in concern order), then untagged (in manifest order) diff --git a/skills/deep-implement/references/finalization.md b/skills/deep-implement/references/finalization.md index 20a6509..b2364bf 100644 --- a/skills/deep-implement/references/finalization.md +++ b/skills/deep-implement/references/finalization.md @@ -63,3 +63,24 @@ Next steps: - Create PR if ready ═══════════════════════════════════════════════════════════════ ``` + +### GitHub Summary (if enabled) + +If GitHub workflow was enabled, add a PR summary section: + +``` +Pull Requests: + PR #{N}: section-01-foundation — {url} + PR #{N}: section-02-models — {url} + PR #{N}: section-03-api — {url} + ... + +Tracking Issue: #{issue_number} +``` + +Read PR info from the session config (`github.section_prs` and `github.issue_number`). + +Replace "Create PR if ready" in next steps with: +``` + - Review open PRs and merge when ready +``` diff --git a/skills/deep-implement/references/github-workflow.md b/skills/deep-implement/references/github-workflow.md new file mode 100644 index 0000000..062a6e5 --- /dev/null +++ b/skills/deep-implement/references/github-workflow.md @@ -0,0 +1,84 @@ +# GitHub Workflow + +GitHub integration for deep-implement: one PR per section with optional tracking issue. + +## Prerequisites + +The setup script checks three conditions: +1. **GitHub remote** — `git remote get-url origin` points to `github.com` +2. **`gh` CLI installed** — `gh --version` succeeds +3. **`gh` authenticated** — `gh auth status` succeeds + +All three must pass for `github.available = true` in setup output. If any fail, GitHub workflow is not offered. + +## Opt-In + +At Step F (Branch Check), if `github.available`, ask the user. If they opt in: +- Store `base_branch` = current branch +- Create tracking issue with section checklist +- Store issue number in config via `update_github_state.py` + +## Per-Section Flow + +When GitHub is enabled, the workflow wraps each section: + +### Before implementation (Step 5) +```bash +git checkout -b implement/{section-name} +``` + +### After commit (Step 10.5) +```bash +git push -u origin implement/{section-name} +gh pr create --title "section-NN: {name}" \ + --body "## Summary\n{description}\n\nCloses #{issue}" \ + --base {base_branch} +``` + +Store PR info, update issue checkbox, print URL, return to base branch. + +### Between sections +```bash +git checkout {base_branch} +``` + +## Tracking Issue + +Created at start with section checklist: +```markdown +## Implementation Tracking + +Sections: +- [ ] section-01-foundation +- [ ] section-02-models +... +``` + +Checkboxes updated after each section PR. The last PR's `Closes #N` auto-closes the issue on merge. + +## Idempotency on Resume + +- **Issue**: Check for existing issue by title prefix before creating a new one +- **Branch**: If `implement/{section-name}` exists, check it out instead of creating +- **PR**: Check for existing PR on the branch before creating (`gh pr list --head implement/{section-name}`) +- **Config**: `github.section_prs` tracks which sections already have PRs + +## Error Handling + +GitHub operations are **best-effort**. Network failures, permission errors, or API rate limits print a warning and continue. Implementation progress is never blocked by GitHub. + +If push fails: warn user, continue to next section (commit is saved locally). +If PR creation fails: warn user, print manual command they can run later. +If issue update fails: silently continue. + +## Finalization + +When all sections complete, print summary table: +``` +Pull Requests: + PR #1: section-01-foundation — https://github.com/owner/repo/pull/1 + PR #2: section-02-models — https://github.com/owner/repo/pull/2 +Tracking Issue: #42 +``` + +No push or PR at finalization — everything was handled per-section. diff --git a/skills/deep-implement/references/go-guardrails.md b/skills/deep-implement/references/go-guardrails.md new file mode 100644 index 0000000..44aacd4 --- /dev/null +++ b/skills/deep-implement/references/go-guardrails.md @@ -0,0 +1,54 @@ +# Go Guardrails + +File size and architecture constraints for Go hex-architecture projects. Read this before writing any code. + +## File Size Limits + +| File type | Path pattern | Max lines | +|-----------|-------------|-----------| +| Port interfaces | `internal/ports/*.go` | 100 | +| Domain entities | `internal/domain/*.go` | 250 | +| Handler files | `internal/handler/**/*.go` | 250 | +| Service files | `internal/service/*.go` | 300 | +| Adapter files | `internal/adapter/**/*.go` | 300 | +| Test files | `*_test.go` | 500 | +| Other `.go` files | (default) | 300 | + +## Function Length Limits + +| Context | Max lines | +|---------|-----------| +| Non-test functions | 75 | +| Test functions | 150 | + +## Hex Architecture Import Rules + +Inner layers must NOT import outer layers: + +``` +domain (0) → ports (1) → service (2) → handler (3) → adapter (4) +``` + +- `domain` may import: stdlib, contracts, pkg +- `ports` may import: domain, stdlib, contracts, pkg +- `service` may import: ports, domain, stdlib, contracts, pkg +- `handler` may import: service, ports, domain, stdlib, contracts, pkg +- `adapter` may import: everything above + +Cross-service `internal/` imports are prohibited. Use `contracts/` or `pkg/` instead. + +## Prohibited Patterns + +- Hardcoded secrets (`password := "..."`, `apiKey := "..."`) +- `os.Exit()` outside `cmd/` packages +- `panic()` in non-test code (use error returns) + +## File Splitting Strategy + +When approaching limits: + +1. **One entity per file** — `blueprint.go`, `project.go`, not `entities.go` +2. **Extract helpers** — move shared logic to `internal/domain/` or `pkg/` +3. **Split adapters by operation** — `repo_read.go`, `repo_write.go` +4. **Extract validation** — `validate.go` for complex validation logic +5. **Table-driven tests** — keep test cases as data, loop body small diff --git a/skills/deep-implement/references/implementation-loop.md b/skills/deep-implement/references/implementation-loop.md index b1512f6..7dddd99 100644 --- a/skills/deep-implement/references/implementation-loop.md +++ b/skills/deep-implement/references/implementation-loop.md @@ -106,6 +106,16 @@ Would you like to: 3. Stop implementation ``` +### 8.5. Guardrail Validation (Go projects) + +If the project runtime is `go`, run guardrail validation after tests pass. See [go-guardrails.md](go-guardrails.md) for the constraint rules. + +```bash +uv run {plugin_root}/scripts/checks/validate_go_guardrails.py --target-dir "{target_dir}" +``` + +If violations are found: fix them (split files, extract helpers, fix imports), re-run tests, then re-validate. After 2 failed retries, proceed and let code review catch remaining issues. + ## Next Steps After the implementation loop completes for a section: diff --git a/skills/deep-implement/references/pre-push-review.md b/skills/deep-implement/references/pre-push-review.md new file mode 100644 index 0000000..dc959b8 --- /dev/null +++ b/skills/deep-implement/references/pre-push-review.md @@ -0,0 +1,43 @@ +# Pre-Push Aggregate Review + +Optional quality gate before pushing each section branch and creating a PR. + +## When + +Runs in Step 10.5, after commit but before `git push`. Only when GitHub workflow is enabled. + +## Process + +1. Generate the full section diff against base branch: + ```bash + git diff {base_branch}...HEAD + ``` + +2. Write diff to `{code_review_dir}/section-NN-prepush-diff.md` + +3. Launch `code-reviewer` subagent with the aggregate diff and section plan + +4. Parse review findings + +## Handling Findings + +**No findings or LOW/MEDIUM only:** Proceed with push. + +**CRITICAL findings:** +``` +AskUserQuestion: + question: "Pre-push review found critical issues. How to proceed?" + options: + - label: "Fix issues" + description: "Address critical findings before pushing" + - label: "Push anyway" + description: "Accept the risk and create PR" + - label: "Stop" + description: "Pause to investigate manually" +``` + +If user chooses "Fix issues": apply fixes, re-run tests, amend commit, re-review (max 1 retry). + +## Skipping + +The pre-push review is a quality signal, not a gate. If it adds too much latency or the user wants to skip, they can always choose "Push anyway". The per-section code review (Step 6-8) already catches most issues. diff --git a/tests/checks/__init__.py b/tests/checks/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/checks/test_validate_go_guardrails.py b/tests/checks/test_validate_go_guardrails.py new file mode 100644 index 0000000..04d96bc --- /dev/null +++ b/tests/checks/test_validate_go_guardrails.py @@ -0,0 +1,258 @@ +"""Tests for validate_go_guardrails.py — no git dependency, tests against string content.""" + +import json +import subprocess +import sys + +import pytest + +# Import validation functions directly +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parents[2] / "scripts")) +from checks.validate_go_guardrails import ( + check_cross_service_imports, + check_file_size, + check_function_lengths, + check_hex_layers, + check_package_declaration, + check_prohibited_patterns, + check_self_imports, + validate_file, +) + + +# --- File size tests --- + + +class TestFileSize: + def test_service_file_under_limit(self): + content = "package svc\n" + "// line\n" * 298 + assert check_file_size("internal/service/foo.go", content) == [] + + def test_service_file_over_limit(self): + content = "package svc\n" + "// line\n" * 310 + violations = check_file_size("internal/service/foo.go", content) + assert len(violations) == 1 + assert "max 300" in violations[0] + + def test_port_file_limit(self): + content = "package ports\n" + "// line\n" * 105 + violations = check_file_size("internal/ports/repo.go", content) + assert len(violations) == 1 + assert "max 100" in violations[0] + + def test_domain_file_limit(self): + content = "package domain\n" + "// line\n" * 260 + violations = check_file_size("internal/domain/entity.go", content) + assert len(violations) == 1 + assert "max 250" in violations[0] + + def test_handler_file_limit(self): + content = "package handler\n" + "// line\n" * 260 + violations = check_file_size("internal/handler/http/api.go", content) + assert len(violations) == 1 + assert "max 250" in violations[0] + + def test_test_file_limit(self): + content = "package svc_test\n" + "// line\n" * 498 + assert check_file_size("internal/service/foo_test.go", content) == [] + + def test_test_file_over_limit(self): + content = "package svc_test\n" + "// line\n" * 510 + violations = check_file_size("internal/service/foo_test.go", content) + assert len(violations) == 1 + assert "max 500" in violations[0] + + +# --- Function length tests --- + + +class TestFunctionLength: + def test_short_function_passes(self): + content = "package svc\n\nfunc DoThing() error {\n" + "\t// line\n" * 30 + "}\n" + assert check_function_lengths("internal/service/foo.go", content) == [] + + def test_long_function_fails(self): + content = "package svc\n\nfunc DoThing() error {\n" + "\t// line\n" * 80 + "}\n" + violations = check_function_lengths("internal/service/foo.go", content) + assert len(violations) == 1 + assert "func DoThing" in violations[0] + assert "max 75" in violations[0] + + def test_method_receiver(self): + content = "package svc\n\nfunc (s *Svc) DoThing() error {\n" + "\t// line\n" * 80 + "}\n" + violations = check_function_lengths("internal/service/foo.go", content) + assert len(violations) == 1 + assert "max 75" in violations[0] + + def test_test_function_higher_limit(self): + content = "package svc_test\n\nfunc TestBigTable(t *testing.T) {\n" + "\t// line\n" * 140 + "}\n" + assert check_function_lengths("internal/service/foo_test.go", content) == [] + + def test_test_function_over_limit(self): + content = "package svc_test\n\nfunc TestBigTable(t *testing.T) {\n" + "\t// line\n" * 160 + "}\n" + violations = check_function_lengths("internal/service/foo_test.go", content) + assert len(violations) == 1 + assert "max 150" in violations[0] + + +# --- Package declaration tests --- + + +class TestPackageDeclaration: + def test_valid_package(self): + assert check_package_declaration("foo.go", "package main\n\nfunc main() {}") == [] + + def test_missing_package(self): + violations = check_package_declaration("foo.go", "func main() {}") + assert len(violations) == 1 + assert "missing package" in violations[0] + + +# --- Hex layer import tests --- + + +class TestHexLayers: + def test_domain_importing_adapter_fails(self): + content = 'package domain\n\nimport "github.com/org/repo/services/foo/internal/adapter/postgres"\n' + violations = check_hex_layers("services/foo/internal/domain/entity.go", content) + assert len(violations) == 1 + assert "domain layer imports adapter layer" in violations[0] + + def test_domain_importing_service_fails(self): + content = 'package domain\n\nimport "github.com/org/repo/services/foo/internal/service"\n' + violations = check_hex_layers("services/foo/internal/domain/entity.go", content) + assert len(violations) == 1 + assert "domain layer imports service layer" in violations[0] + + def test_adapter_importing_domain_ok(self): + content = 'package postgres\n\nimport "github.com/org/repo/services/foo/internal/domain"\n' + assert check_hex_layers("services/foo/internal/adapter/postgres/repo.go", content) == [] + + def test_service_importing_ports_ok(self): + content = 'package service\n\nimport "github.com/org/repo/services/foo/internal/ports"\n' + assert check_hex_layers("services/foo/internal/service/svc.go", content) == [] + + def test_non_hex_file_ignored(self): + content = 'package main\n\nimport "github.com/org/repo/services/foo/internal/adapter/postgres"\n' + assert check_hex_layers("cmd/server/main.go", content) == [] + + def test_ports_importing_handler_fails(self): + content = 'package ports\n\nimport "github.com/org/repo/services/foo/internal/handler/http"\n' + violations = check_hex_layers("services/foo/internal/ports/repo.go", content) + assert len(violations) == 1 + assert "ports layer imports handler layer" in violations[0] + + +# --- Cross-service import tests --- + + +class TestCrossServiceImports: + def test_cross_service_internal_import(self): + content = 'package svc\n\nimport "github.com/org/repo/services/other/internal/domain"\n' + violations = check_cross_service_imports("services/control-plane/internal/service/svc.go", content) + assert len(violations) == 1 + assert "other" in violations[0] + + def test_same_service_internal_ok(self): + content = 'package svc\n\nimport "github.com/org/repo/services/control-plane/internal/domain"\n' + assert check_cross_service_imports("services/control-plane/internal/service/svc.go", content) == [] + + +# --- Self-import tests --- + + +class TestSelfImports: + def test_self_import_detected(self): + content = 'package domain\n\nimport "github.com/org/repo/services/foo/internal/domain"\n' + violations = check_self_imports("services/foo/internal/domain/entity.go", content) + assert len(violations) == 1 + assert "self-import" in violations[0] + + def test_different_package_ok(self): + content = 'package service\n\nimport "github.com/org/repo/services/foo/internal/domain"\n' + assert check_self_imports("services/foo/internal/service/svc.go", content) == [] + + +# --- Prohibited patterns --- + + +class TestProhibitedPatterns: + def test_hardcoded_secret(self): + content = 'package svc\n\nvar api_key = "super-secret-key-12345"\n' + violations = check_prohibited_patterns("internal/service/foo.go", content) + assert any("Hardcoded secret" in v for v in violations) + + def test_os_exit_in_non_main(self): + content = "package svc\n\nimport \"os\"\n\nfunc cleanup() { os.Exit(1) }\n" + violations = check_prohibited_patterns("internal/service/foo.go", content) + assert any("os.Exit" in v for v in violations) + + def test_os_exit_in_main_ok(self): + content = "package main\n\nimport \"os\"\n\nfunc main() { os.Exit(1) }\n" + assert check_prohibited_patterns("cmd/server/main.go", content) == [] + + def test_panic_in_non_test(self): + content = "package svc\n\nfunc bad() { panic(\"oops\") }\n" + violations = check_prohibited_patterns("internal/service/foo.go", content) + assert any("panic()" in v for v in violations) + + def test_panic_in_test_ok(self): + content = "package svc_test\n\nfunc helper() { panic(\"test helper\") }\n" + assert check_prohibited_patterns("internal/service/foo_test.go", content) == [] + + +# --- Non-Go files ignored --- + + +class TestNonGoFiles: + def test_non_go_file_no_violations(self): + # validate_file should still work but non-.go files aren't discovered + content = "This is a markdown file.\n" * 500 + # The function still checks — but discovery filters .go only + # So we test that the script's discover step would skip it + assert "README.md".endswith(".go") is False + + +# --- Override flag --- + + +class TestOverrides: + def test_override_increases_limit(self): + """After applying override, a previously-violating file passes.""" + import scripts.checks.validate_go_guardrails as mod + + original = mod.MAX_FILE_LINES + try: + mod.apply_overrides({"max_file_lines": "450"}) + assert mod.MAX_FILE_LINES == 450 + content = "package svc\n" + "// line\n" * 400 + assert mod.check_file_size("internal/service/foo.go", content) == [] + finally: + mod.MAX_FILE_LINES = original + + def test_unknown_override_ignored(self, capsys): + import scripts.checks.validate_go_guardrails as mod + + mod.apply_overrides({"bogus_key": "999"}) + captured = capsys.readouterr() + assert "unknown override" in captured.err + + +# --- Integration: validate_file combines all checks --- + + +class TestValidateFile: + def test_clean_file_passes(self): + content = ( + "package svc\n\n" + "func DoThing() error {\n" + "\treturn nil\n" + "}\n" + ) + assert validate_file("internal/service/foo.go", content) == [] + + def test_multiple_violations(self): + content = "func bad() { panic(\"oops\") }\n" * 400 + violations = validate_file("internal/service/foo.go", content) + # Should have: over file limit, missing package, panic, possibly function length + assert len(violations) >= 3 diff --git a/tests/conftest.py b/tests/conftest.py index c15c049..c4d93b0 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -92,5 +92,12 @@ def sample_config(): }, "sections": ["section-01-foundation", "section-02-models"], "sections_state": {}, + "github": { + "enabled": False, + "owner_repo": None, + "base_branch": None, + "issue_number": None, + "section_prs": {}, + }, "created_at": "2025-01-14T10:30:00Z" } diff --git a/tests/test_concern_ordering.py b/tests/test_concern_ordering.py new file mode 100644 index 0000000..438e2e7 --- /dev/null +++ b/tests/test_concern_ordering.py @@ -0,0 +1,239 @@ +"""Tests for concern-type ordering functions in sections.py.""" + +import pytest + +from scripts.lib.sections import ( + parse_manifest_block, + parse_section_concerns, + parse_section_meta, + sort_sections_by_concern, +) + + +class TestParseManifestBlockWithConcernTags: + """Verify parse_manifest_block ignores concern tags (backward compat).""" + + def test_manifest_with_concern_tags_returns_bare_names(self): + """Tags should be stripped — only section names returned.""" + content = """""" + result = parse_manifest_block(content) + assert result == [ + "section-01-foundation", + "section-02-models", + "section-03-api", + ] + + def test_manifest_mixed_tagged_and_bare(self): + """Lines with and without tags both parse correctly.""" + content = """""" + result = parse_manifest_block(content) + assert result == [ + "section-01-foundation", + "section-02-models", + "section-03-api", + ] + + +class TestParseSectionConcerns: + """Tests for parse_section_concerns().""" + + def test_with_tags(self): + content = """""" + result = parse_section_concerns(content) + assert result == { + "section-01-foundation": "scaffold", + "section-02-models": "functional", + "section-03-logging": "observability", + } + + def test_no_tags(self): + content = """""" + result = parse_section_concerns(content) + assert result == {} + + def test_mixed_tags(self): + content = """""" + result = parse_section_concerns(content) + assert result == { + "section-01-foundation": "scaffold", + "section-03-api": "functional", + } + + def test_invalid_tag_ignored(self): + content = """""" + result = parse_section_concerns(content) + assert result == { + "section-01-foundation": "scaffold", + "section-03-api": "functional", + } + assert "section-02-models" not in result + + def test_no_manifest_block(self): + content = "# Just a regular file" + result = parse_section_concerns(content) + assert result == {} + + def test_all_concern_types(self): + content = """""" + result = parse_section_concerns(content) + assert len(result) == 6 + assert result["section-04-config"] == "configuration" + assert result["section-05-errors"] == "resilience" + assert result["section-06-wiring"] == "integration" + + +class TestSortSectionsByConcern: + """Tests for sort_sections_by_concern().""" + + def test_reorders_by_concern(self): + sections = [ + "section-01-api", # functional + "section-02-init", # scaffold + "section-03-logging", # observability + ] + concerns = { + "section-01-api": "functional", + "section-02-init": "scaffold", + "section-03-logging": "observability", + } + result = sort_sections_by_concern(sections, concerns) + assert result == [ + "section-02-init", # scaffold first + "section-01-api", # functional second + "section-03-logging", # observability third + ] + + def test_preserves_number_order_within_concern(self): + sections = [ + "section-01-ports", + "section-02-domain", + "section-03-service", + ] + concerns = { + "section-01-ports": "functional", + "section-02-domain": "functional", + "section-03-service": "functional", + } + result = sort_sections_by_concern(sections, concerns) + # All functional — original order preserved + assert result == sections + + def test_untagged_sections_last(self): + sections = [ + "section-01-api", + "section-02-init", + "section-03-misc", + ] + concerns = { + "section-01-api": "functional", + "section-02-init": "scaffold", + } + result = sort_sections_by_concern(sections, concerns) + assert result == [ + "section-02-init", # scaffold + "section-01-api", # functional + "section-03-misc", # untagged last + ] + + def test_empty_concerns_returns_original(self): + sections = ["section-01-a", "section-02-b", "section-03-c"] + result = sort_sections_by_concern(sections, {}) + assert result == sections + + def test_full_concern_ordering(self): + sections = [ + "section-01-wiring", + "section-02-errors", + "section-03-config", + "section-04-logging", + "section-05-core", + "section-06-init", + ] + concerns = { + "section-01-wiring": "integration", + "section-02-errors": "resilience", + "section-03-config": "configuration", + "section-04-logging": "observability", + "section-05-core": "functional", + "section-06-init": "scaffold", + } + result = sort_sections_by_concern(sections, concerns) + assert result == [ + "section-06-init", # scaffold + "section-05-core", # functional + "section-04-logging", # observability + "section-03-config", # configuration + "section-02-errors", # resilience + "section-01-wiring", # integration + ] + + +class TestParseSectionMeta: + """Tests for parse_section_meta().""" + + def test_parses_full_meta(self): + content = """# Section 01: Foundation + + + +## Implementation +Create the base structure. +""" + result = parse_section_meta(content) + assert result["concern"] == "scaffold" + assert "internal/ports/repository.go" in result["target_files"] + assert result["estimated_lines"] == "150" + + def test_missing_meta_returns_empty(self): + content = """# Section 01: Foundation + +## Implementation +Create the base structure. +""" + result = parse_section_meta(content) + assert result == {} + + def test_partial_meta(self): + content = """ + +# Content here +""" + result = parse_section_meta(content) + assert result == {"concern": "functional"} + assert "target_files" not in result diff --git a/tests/tools/test_update_github_state.py b/tests/tools/test_update_github_state.py new file mode 100644 index 0000000..3c1852e --- /dev/null +++ b/tests/tools/test_update_github_state.py @@ -0,0 +1,165 @@ +"""Tests for update_github_state CLI tool.""" + +import json +import subprocess +import sys +from pathlib import Path + +import pytest + +PLUGIN_ROOT = Path(__file__).parent.parent.parent +SCRIPT_PATH = PLUGIN_ROOT / "scripts" / "tools" / "update_github_state.py" + + +class TestUpdateGitHubStateCLI: + """Tests for update_github_state.py CLI script.""" + + def test_stores_issue_number(self, mock_implementation_dir, sample_config): + """Should store tracking issue number and enable GitHub.""" + config_path = mock_implementation_dir / "deep_implement_config.json" + config_path.write_text(json.dumps(sample_config)) + + result = subprocess.run( + [ + sys.executable, + str(SCRIPT_PATH), + "--state-dir", str(mock_implementation_dir), + "--issue-number", "42", + ], + capture_output=True, + text=True, + ) + + assert result.returncode == 0 + assert "issue_number=42" in result.stdout + + config = json.loads(config_path.read_text()) + assert config["github"]["issue_number"] == 42 + assert config["github"]["enabled"] is True + + def test_stores_section_pr(self, mock_implementation_dir, sample_config): + """Should store PR number and URL for a section.""" + config_path = mock_implementation_dir / "deep_implement_config.json" + config_path.write_text(json.dumps(sample_config)) + + result = subprocess.run( + [ + sys.executable, + str(SCRIPT_PATH), + "--state-dir", str(mock_implementation_dir), + "--section", "section-01-foundation", + "--pr-number", "43", + "--pr-url", "https://github.com/owner/repo/pull/43", + ], + capture_output=True, + text=True, + ) + + assert result.returncode == 0 + assert "pr_number=43" in result.stdout + + config = json.loads(config_path.read_text()) + pr_info = config["github"]["section_prs"]["section-01-foundation"] + assert pr_info["number"] == 43 + assert pr_info["url"] == "https://github.com/owner/repo/pull/43" + + def test_pr_requires_section(self, mock_implementation_dir, sample_config): + """Should error if --pr-number given without --section.""" + config_path = mock_implementation_dir / "deep_implement_config.json" + config_path.write_text(json.dumps(sample_config)) + + result = subprocess.run( + [ + sys.executable, + str(SCRIPT_PATH), + "--state-dir", str(mock_implementation_dir), + "--pr-number", "43", + ], + capture_output=True, + text=True, + ) + + assert result.returncode == 1 + assert "--section is required" in result.stdout + + def test_handles_missing_config(self, mock_implementation_dir): + """Should return error for missing config file.""" + result = subprocess.run( + [ + sys.executable, + str(SCRIPT_PATH), + "--state-dir", str(mock_implementation_dir), + "--issue-number", "42", + ], + capture_output=True, + text=True, + ) + + assert result.returncode == 1 + assert "No config found" in result.stdout + + def test_requires_issue_or_pr(self, mock_implementation_dir, sample_config): + """Should error if neither --issue-number nor --pr-number provided.""" + config_path = mock_implementation_dir / "deep_implement_config.json" + config_path.write_text(json.dumps(sample_config)) + + result = subprocess.run( + [ + sys.executable, + str(SCRIPT_PATH), + "--state-dir", str(mock_implementation_dir), + ], + capture_output=True, + text=True, + ) + + assert result.returncode == 1 + assert "Must provide" in result.stdout + + def test_preserves_existing_prs(self, mock_implementation_dir, sample_config): + """Should not overwrite other sections' PR info.""" + sample_config["github"]["section_prs"] = { + "section-01-foundation": {"number": 10, "url": "https://example.com/10"} + } + config_path = mock_implementation_dir / "deep_implement_config.json" + config_path.write_text(json.dumps(sample_config)) + + result = subprocess.run( + [ + sys.executable, + str(SCRIPT_PATH), + "--state-dir", str(mock_implementation_dir), + "--section", "section-02-models", + "--pr-number", "11", + "--pr-url", "https://example.com/11", + ], + capture_output=True, + text=True, + ) + + assert result.returncode == 0 + + config = json.loads(config_path.read_text()) + assert config["github"]["section_prs"]["section-01-foundation"]["number"] == 10 + assert config["github"]["section_prs"]["section-02-models"]["number"] == 11 + + def test_creates_github_key_if_missing(self, mock_implementation_dir, sample_config): + """Should create github dict if config has no github key.""" + del sample_config["github"] + config_path = mock_implementation_dir / "deep_implement_config.json" + config_path.write_text(json.dumps(sample_config)) + + result = subprocess.run( + [ + sys.executable, + str(SCRIPT_PATH), + "--state-dir", str(mock_implementation_dir), + "--issue-number", "99", + ], + capture_output=True, + text=True, + ) + + assert result.returncode == 0 + config = json.loads(config_path.read_text()) + assert config["github"]["issue_number"] == 99