feat: Gate v2.0.0 - AI-Powered Validation#39
Conversation
Major v2.0.0 release adding AI-powered code validation using local Ollama models. New Features: - Attribution Check: Auto-detect and remove Claude Code attribution from commits - Logic & Atomicity: AI validation ensuring commits are atomic and coherent - PR Cohesion: Cross-file analysis for missing files and MVC coherence - Git Hooks: Easy installation via 'gate install' command - AI Code Review: Pattern, security, and test suggestion analysis in CI - Semantic Release: Auto-versioning based on conventional commits New Check Classes: - AttributionCheck: Validates commit messages are clean - LogicCheck: Ollama-powered atomicity validation (llama3.2:3b) - CohesionCheck: PR-level cross-file analysis New Commands: - gate install: Install git hooks with config - gate check:attribution: Check/remove Claude attribution - gate check:logic: Validate commit atomicity - gate check:cohesion: Analyze PR cohesion Technical Details: - Uses Ollama for free local AI (no API costs) - Models auto-download on first use - Graceful degradation when Ollama unavailable - Compatible with existing v1.x workflows - Comprehensive README and CHANGELOG documentation Performance: - Pre-commit validation: <10 seconds - AI review with caching: ~30 seconds (vs 5min without) - Logic check: 3-8 seconds with llama3.2:3b
📝 WalkthroughWalkthroughAdds v2.0.0: three AI-driven checks (Attribution, Logic & Atomicity, PR Cohesion) with Ollama integration, new CLI commands (check:attribution, check:logic, check:cohesion, install hooks), CertifyCommand updates, unit tests, changelog and README revisions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as LogicCheckCommand
participant Git
participant Ollama
participant Checks as ChecksClient
User->>CLI: run check:logic
CLI->>Git: get staged files & staged diff
CLI->>Ollama: is ollama available?
alt Ollama unavailable
CLI->>User: Skip/Pass with message
else Ollama available
CLI->>Ollama: ensure model present (pull if missing)
CLI->>Ollama: run analysis prompt (atomicity, relatedness, logic)
Ollama-->>CLI: analysis output
CLI->>CLI: parse result (ATOMIC, RELATED, ISSUES)
alt Atomic && Related
CLI->>User: PASS
CLI->>Checks: (optional) report success
else
CLI->>User: FAIL with details
CLI->>Checks: (optional) report failure + summary
end
end
sequenceDiagram
autonumber
participant User
participant CLI as CohesionCheckCommand
participant Git
participant Ollama
participant Checks as ChecksClient
User->>CLI: run check:cohesion
CLI->>Ollama: is ollama available?
alt Ollama unavailable
CLI->>User: Skip/Pass with message
else Ollama available
CLI->>Git: get changed files vs origin/{main,master,develop}
alt No changed files
CLI->>User: PASS (no changes)
else Has changes
CLI->>CLI: categorize files (models, controllers, views, tests...)
CLI->>Git: retrieve PR diff
CLI->>Ollama: ensure model present (pull if missing)
CLI->>Ollama: run cohesion analysis (files, diff, categories)
Ollama-->>CLI: cohesion assessment
CLI->>CLI: parse response (cohesive?, issues)
alt Cohesive
CLI->>User: PASS
CLI->>Checks: (optional) report success
else
CLI->>User: FAIL with details
CLI->>Checks: (optional) report failure + details
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI Agents
In @app/Checks/CohesionCheck.php:
- Around line 166-181: The code in ensureModelAvailable uses 'sh -c' with string
interpolation of $this->model which enables shell injection; replace the shell
call with a direct processRunner->run invocation like run(['ollama','list'],
...) and inspect $result->output in PHP to detect the model instead of piping to
grep. Use a safe text match (e.g., preg_match with preg_quote($this->model) or
strict string search) against $result->output to decide whether to run
processRunner->run(['ollama','pull',$this->model], ...); update the logic in
ensureModelAvailable to avoid any shell interpolation and reference the existing
processRunner->run and $this->model symbols.
In @app/Commands/AttributionCheckCommand.php:
- Around line 16-22: The attribution regexes are duplicated in the protected
$attributionPatterns array; remove the local array and reuse the single source
of truth by injecting or referencing the existing AttributionCheck logic (e.g.,
inject AttributionCheck and call its hasAttribution() or read its
constant/PROPERTY), update the AttributionCheckCommand constructor to accept
AttributionCheck (or import the shared constant) and replace all uses of
$attributionPatterns in AttributionCheckCommand with the injected service/mapped
constant; ensure the old $attributionPatterns property is deleted and any
tests/uses updated to call the shared method or constant instead.
- Around line 102-104: The current filter in AttributionCheckCommand.php drops
any line where trim($line) === '' even when it should be preserved; change the
logic so only attribution-matching lines are removed and intentional blank lines
are kept — e.g., instead of requiring both $shouldKeep and non-empty trim, push
the line when $shouldKeep is true OR the line is blank (trim($line) === '') so
blank lines (paragraph breaks) are preserved while attribution lines continue to
be filtered out.
In @app/Commands/CohesionCheckCommand.php:
- Around line 153-160: The ensureModelAvailable method is vulnerable to shell
injection because $model is interpolated into shell strings passed to
Process::run; fix by avoiding shell parsing and passing commands as argument
arrays and by checking the command output in PHP instead of piping to grep: call
Process::run(['ollama', 'list']) and inspect $result->output() (e.g.,
str_contains($result->output(), $model)) to determine presence, and if missing
call Process::run(['ollama', 'pull', $model]) so $model is passed as a safe
argument rather than interpolated into a shell string.
- Around line 85-101: The getChangedFiles and getPRDiff functions interpolate
the $base into shell strings leading to command injection; fix by switching to
array-based Process::run calls (e.g.
Process::run(['git','diff','--name-only',"origin/{$base}...HEAD"]) and
Process::run(['git','diff',"origin/{$base}...HEAD"])) and add a simple
validation/allowlist for $base (e.g. /^[A-Za-z0-9._\/-]+$/) to reject suspicious
values before calling Process::run; if validation fails, return safe defaults
(empty array or empty string) to avoid executing commands with malicious input.
In @app/Commands/InstallHooksCommand.php:
- Around line 107-110: In getGitRoot(), shell_exec('git rev-parse
--show-toplevel') can return null so calling trim(...) may throw a TypeError;
modify getGitRoot to capture the shell_exec result into a variable, check for
null (or empty) and handle the failure explicitly (either cast to string or
throw a RuntimeException with a clear message) and only then call trim on the
non-null output; reference the getGitRoot method and the shell_exec('git
rev-parse --show-toplevel') call when making this change.
- Around line 146-161: The getGateBinaryPath() uses trim(shell_exec('which
gate')) which can return null; capture shell_exec result into a variable (e.g.,
$whichRaw), check it's a non-empty string (is_string($whichRaw) &&
trim($whichRaw) !== '') before calling trim() and file_exists(), and only return
that path when valid; otherwise fall back to checking ./vendor/bin/gate and
finally the default 'gate'. Ensure you reference the getGateBinaryPath()
function and replace the direct trim(shell_exec(...)) call with the safe
variable check.
In @app/Commands/LogicCheckCommand.php:
- Around line 129-140: The code concatenates $model into shell strings in
ensureModelAvailable (Process::run calls), opening a command-injection risk;
change to non-shell array invocation and avoid pipes: call
Process::run(['ollama','list']) and inspect the returned output (e.g.,
$result->output() or $result->getOutput()) to determine if $model is present,
and call Process::run(['ollama','pull', $model]) for the pull operation so the
$model value is passed as an argument (not interpolated into a shell), keeping
the logic in ensureModelAvailable and the task closure the same.
In @CHANGELOG.md:
- Line 119: The CHANGELOG entry for version identifier [1.4.1] currently uses a
placeholder date "2024-12-XX"; update that header line to replace "2024-12-XX"
with the actual release date (e.g., "2024-12-15") so the line reads "## [1.4.1]
- YYYY-MM-DD" using the correct release date for v1.4.1.
🧹 Nitpick comments (9)
app/Checks/AttributionCheck.php (1)
12-18: Code duplication withAttributionCheckCommand.Both
$attributionPatternsandhasAttribution()are duplicated inAttributionCheckCommand.php(lines 72-81). Consider having the command delegate to this check class or extracting shared logic to avoid divergence.Also applies to: 60-69
app/Checks/LogicCheck.php (1)
119-134: Model availability check uses shell pipe without escaping.While the
$modelvalue comes from constructor injection rather than user input, usingescapeshellarg()would be more defensive.🔎 Defensive escaping suggestion
private function ensureModelAvailable(string $workingDirectory): void { + $escapedModel = escapeshellarg($this->model); $result = $this->processRunner->run( - ['sh', '-c', "ollama list | grep {$this->model}"], + ['sh', '-c', "ollama list | grep {$escapedModel}"], $workingDirectory, timeout: 5 );app/Commands/AttributionCheckCommand.php (1)
66-71: Consider adding timeout to git command.The
getLastCommitMessage()method doesn't specify a timeout for the git command, while the corresponding logic inapp/Checks/AttributionCheck.php(line 31) uses a 5-second timeout. For consistency and to prevent hangs, consider adding a timeout here as well.🔎 Suggested change
protected function getLastCommitMessage(): string { - $result = Process::run('git log -1 --pretty=%B'); + $result = Process::timeout(5)->run('git log -1 --pretty=%B'); return $result->successful() ? trim($result->output()) : ''; }app/Commands/LogicCheckCommand.php (2)
121-127: Diff statistics calculation is fragile.The
getDiffStatsmethod counts lines starting with+or-by searching for\n+and\n-, then subtracts diff headers. This approach is fragile because:
- It doesn't account for
+or-characters that may appear in the diff content itself- The subtraction of headers assumes exactly one
+++and---per fileConsider using
git diff --statorgit diff --numstatfor more reliable statistics.🔎 More reliable alternative
protected function getDiffStats(string $diff): array { - $added = substr_count($diff, "\n+") - substr_count($diff, "\n+++"); - $removed = substr_count($diff, "\n-") - substr_count($diff, "\n---"); - - return ['added' => $added, 'removed' => $removed]; + $result = Process::run('git diff --cached --numstat'); + + if (!$result->successful()) { + return ['added' => 0, 'removed' => 0]; + } + + $added = $removed = 0; + foreach (explode("\n", trim($result->output())) as $line) { + if (preg_match('/^(\d+)\s+(\d+)/', $line, $matches)) { + $added += (int)$matches[1]; + $removed += (int)$matches[2]; + } + } + + return ['added' => $added, 'removed' => $removed]; }
156-159: Arbitrary diff truncation may hide important changes.Truncating the diff to 500 lines (line 158) is an arbitrary limit that could omit critical changes in larger commits. This may lead to incomplete or incorrect analysis by the AI model.
Consider either:
- Using a token-aware truncation strategy that prioritizes context
- Summarizing omitted changes
- Warning the user when truncation occurs
🔎 Add user notification when truncation occurs
protected function buildAnalysisPrompt(array $files, string $diff): string { $fileList = implode("\n", $files); // Limit diff to 500 lines to avoid token limits $diffLines = explode("\n", $diff); + $totalLines = count($diffLines); $limitedDiff = implode("\n", array_slice($diffLines, 0, 500)); + + if ($totalLines > 500) { + $this->warn("⚠ Diff truncated: showing 500 of {$totalLines} lines"); + } return <<<PROMPTapp/Checks/CohesionCheck.php (4)
86-106: Silent branch fallback may cause confusion.The
getChangedFiles()method silently tries multiple base branches (main,master,main,develop) without informing the user which one was used. This could lead to confusion about what's being compared, especially if the user expects a specific base.Consider either:
- Logging which base branch is being used
- Making the base branch resolution more explicit
- Failing fast if the configured base isn't found rather than silently falling back
💡 Alternative: Explicit base branch detection
If this code had access to console output, you could inform the user:
foreach ($baseBranches as $base) { $result = $this->processRunner->run( ['git', 'diff', '--name-only', "origin/{$base}...HEAD"], $workingDirectory, timeout: 5 ); if ($result->successful && ! empty(trim($result->output))) { // Log: "Using base branch: {$base}" $changedFiles = array_filter(explode("\n", trim($result->output))); break; } }
127-164: File categorization logic is fragile and case-sensitive.The
categorizeFiles()method uses simple string matching (str_contains) to categorize files, which has several weaknesses:
- Case-sensitive matching (e.g.,
/Models/vs/models/)- Partial matches could miscategorize (e.g.,
app/Services/Models/would match both services and models)- Doesn't account for framework-specific conventions beyond Laravel
🔎 More robust categorization approach
Consider using more specific path patterns or a configuration-based approach:
private function categorizeFiles(array $files): array { $categories = [ 'models' => [], 'controllers' => [], 'views' => [], 'tests' => [], 'migrations' => [], 'config' => [], 'routes' => [], 'services' => [], 'other' => [], ]; foreach ($files as $file) { $normalized = strtolower($file); // Use more specific patterns, check in order of specificity if (preg_match('#/tests?/#i', $file) || preg_match('#\.test\.#i', $file)) { $categories['tests'][] = $file; } elseif (preg_match('#/database/migrations/#i', $file)) { $categories['migrations'][] = $file; } elseif (preg_match('#/app/Models/#i', $file)) { $categories['models'][] = $file; } elseif (preg_match('#/app/(Http/)?Controllers/#i', $file)) { $categories['controllers'][] = $file; } elseif (preg_match('#/resources/views/#i', $file) || preg_match('#\.blade\.php$#', $file)) { $categories['views'][] = $file; } elseif (preg_match('#/app/Services/#i', $file)) { $categories['services'][] = $file; } elseif (preg_match('#/config/#i', $file)) { $categories['config'][] = $file; } elseif (preg_match('#/routes/#i', $file)) { $categories['routes'][] = $file; } else { $categories['other'][] = $file; } } return array_filter($categories); }This approach uses regex patterns that are more specific and case-insensitive.
249-272: Regex parsing relies on exact LLM output format.The
parseAnalysis()method uses regex patterns to extract structured data from the LLM response. This is fragile because:
- LLMs may not always follow the exact format despite instructions
- Response variations (extra whitespace, different casing, etc.) could break parsing
- No validation or error handling if expected fields are missing
Consider adding validation and fallback handling for malformed responses.
🔎 Add validation and defensive parsing
private function parseAnalysis(string $analysis): array { $cohesive = stripos($analysis, 'COHESIVE: YES') !== false; preg_match('/MISSING_FILES:\s*(.+?)(?=\n[A-Z_]+:|$)/s', $analysis, $missingMatch); $missing = isset($missingMatch[1]) ? trim($missingMatch[1]) : 'none'; preg_match('/DEPENDENCY_ISSUES:\s*(.+?)(?=\n[A-Z_]+:|$)/s', $analysis, $depsMatch); $dependencies = isset($depsMatch[1]) ? trim($depsMatch[1]) : 'none'; preg_match('/PURPOSE:\s*(.+?)(?=\n[A-Z_]+:|$)/s', $analysis, $purposeMatch); $purpose = isset($purposeMatch[1]) ? trim($purposeMatch[1]) : 'Unknown'; preg_match('/CONCERNS:\s*(.+?)(?=\n[A-Z_]+:|$)/s', $analysis, $concernsMatch); $concerns = isset($concernsMatch[1]) ? trim($concernsMatch[1]) : 'none'; + + // Validate that we got at least the essential fields + if ($purpose === 'Unknown' && !$cohesive) { + // LLM response may be malformed, log for debugging + error_log('Malformed LLM response: ' . substr($analysis, 0, 200)); + } return [ 'cohesive' => $cohesive, 'missing_files' => $missing, 'dependency_issues' => $dependencies, 'purpose' => $purpose, 'concerns' => $concerns, + 'raw_analysis' => $analysis, // Include raw response for debugging ]; }
196-247: Consider validating diff size before analysis.The
buildCohesionPrompt()method truncates the diff to 300 lines without warning. For very large PRs, this could result in the LLM analyzing only a fraction of the changes, leading to incomplete or inaccurate cohesion assessment.Consider either:
- Returning an early check result when the PR is too large
- Using a smarter sampling strategy
- At minimum, including metadata about truncation in the prompt
💡 Add truncation context to prompt
private function buildCohesionPrompt(array $files, array $categories, string $diff): string { $fileList = implode("\n", $files); $categoryBreakdown = ''; foreach ($categories as $category => $categoryFiles) { $categoryBreakdown .= "\n{$category}: ".count($categoryFiles); } // Limit diff to avoid token limits $diffLines = explode("\n", $diff); + $totalLines = count($diffLines); $limitedDiff = implode("\n", array_slice($diffLines, 0, 300)); + $truncationNote = $totalLines > 300 + ? "\n\nNOTE: Diff truncated to 300 of {$totalLines} total lines for analysis." + : ''; return <<<PROMPT You are a senior code reviewer analyzing a Pull Request for cohesion and completeness. +{$truncationNote} Analyze this PR holistically across all files and answer:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.mdREADME.mdapp/Checks/AttributionCheck.phpapp/Checks/CohesionCheck.phpapp/Checks/LogicCheck.phpapp/Commands/AttributionCheckCommand.phpapp/Commands/CertifyCommand.phpapp/Commands/CohesionCheckCommand.phpapp/Commands/InstallHooksCommand.phpapp/Commands/LogicCheckCommand.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T20:12:44.070Z
Learnt from: CR
Repo: synapse-sentinel/gate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T20:12:44.070Z
Learning: Organize application code with Commands, Checks, GitHub, and Stages subdirectories within `app/` directory
Applied to files:
app/Commands/CertifyCommand.php
🧬 Code graph analysis (6)
app/Commands/CertifyCommand.php (6)
app/Checks/AttributionCheck.php (1)
AttributionCheck(10-70)app/Checks/CohesionCheck.php (1)
CohesionCheck(10-273)app/Checks/LogicCheck.php (1)
LogicCheck(10-216)app/Checks/TestRunner.php (3)
TestRunner(11-142)__construct(19-23)run(41-62)app/Checks/SecurityScanner.php (1)
SecurityScanner(10-68)app/Checks/PestSyntaxValidator.php (1)
PestSyntaxValidator(9-47)
app/Commands/AttributionCheckCommand.php (1)
app/Checks/AttributionCheck.php (2)
hasAttribution(60-69)run(29-58)
app/Commands/LogicCheckCommand.php (1)
app/Checks/LogicCheck.php (9)
isOllamaAvailable(79-84)isOllamaRunning(86-91)getStagedFiles(93-106)getStagedDiff(108-117)ensureModelAvailable(119-134)analyzeWithOllama(136-147)parseAnalysis(194-215)run(23-77)buildAnalysisPrompt(149-192)
app/Checks/AttributionCheck.php (3)
app/Services/SymfonyProcessRunner.php (1)
SymfonyProcessRunner(11-24)app/Checks/CheckResult.php (3)
CheckResult(7-24)pass(15-18)fail(20-23)app/Commands/AttributionCheckCommand.php (1)
hasAttribution(73-82)
app/Commands/CohesionCheckCommand.php (4)
app/Commands/CertifyCommand.php (1)
handle(54-159)app/Commands/LogicCheckCommand.php (5)
handle(17-87)isOllamaAvailable(89-94)ensureModelAvailable(129-140)parseAnalysis(197-219)displayAnalysisResult(221-268)app/Checks/CohesionCheck.php (4)
isOllamaAvailable(79-84)ensureModelAvailable(166-181)parseAnalysis(249-272)run(24-77)app/Checks/LogicCheck.php (4)
isOllamaAvailable(79-84)ensureModelAvailable(119-134)parseAnalysis(194-215)run(23-77)
app/Checks/CohesionCheck.php (3)
app/Services/SymfonyProcessRunner.php (1)
SymfonyProcessRunner(11-24)app/Checks/CheckResult.php (3)
CheckResult(7-24)pass(15-18)fail(20-23)app/Commands/CohesionCheckCommand.php (4)
isOllamaAvailable(78-83)getChangedFiles(85-94)categorizeFiles(103-140)getPRDiff(96-101)
🪛 LanguageTool
README.md
[style] ~118-~118: Consider a different adjective to strengthen your wording.
Context: ... (3-8 seconds) - qwen2.5-coder:7b - Deep code review in CI (with caching) Model...
(DEEP_PROFOUND)
CHANGELOG.md
[grammar] ~42-~42: Ensure spelling is correct
Context: ...el caching reduces CI time from 5min to 30sec - Layer 4: Semantic Release Automation ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 PHPMD (2.15.0)
app/Commands/InstallHooksCommand.php
245-245: Avoid unused parameters such as '$schedule'. (undefined)
(UnusedFormalParameter)
app/Commands/AttributionCheckCommand.php
141-141: Avoid unused parameters such as '$schedule'. (undefined)
(UnusedFormalParameter)
app/Commands/LogicCheckCommand.php
270-270: Avoid unused parameters such as '$schedule'. (undefined)
(UnusedFormalParameter)
app/Commands/CohesionCheckCommand.php
302-302: Avoid unused parameters such as '$schedule'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (8)
README.md (1)
1-143: Documentation is comprehensive and well-structured.The README effectively documents the new v2.0.0 features including:
- Phase-based validation architecture
- AI model configuration with Ollama
- New commands and their usage
- Migration guidance from v1.x
The coverage threshold change from 100 to 80 in the example (line 35) is a sensible default for most projects.
app/Commands/InstallHooksCommand.php (2)
129-144: Verify post-commit hook design: attribution fix runs after commit is finalized.The post-commit hook runs
check:attribution --fixafter the commit is already created. If attribution is found and "fixed," this would create a new amend commit or require manual intervention. Is this the intended behavior, or should attribution checking/fixing happen in pre-commit instead?
245-248: Unused$scheduleparameter is required by interface.The PHPMD warning is a false positive - Laravel Zero commands must implement this method signature even when no scheduling is needed.
CHANGELOG.md (1)
1-128: Changelog is comprehensive and well-structured.Follows Keep a Changelog format with clear migration guidance. The documentation of new features, technical details, and example commands is thorough.
app/Commands/CertifyCommand.php (2)
65-72: New checks integrated correctly with appropriate ordering.The check order is well-designed:
- Attribution & Logic run first (fast, pre-commit style)
- Tests, Security, Pest Syntax (CI validations)
- Cohesion last (holistic PR analysis)
This aligns with the phase-based architecture documented in the README.
196-207: Short name mappings are consistent with check names.The
shortName()method correctly maps all new check names for compact output display.app/Checks/LogicCheck.php (2)
194-215:logic_soundis parsed but never used in pass/fail decision.The
LOGIC_SOUNDvalue is extracted from the analysis (line 198, 211) but the check only considersatomicandrelatedfor the pass/fail verdict (lines 58-60). Was this intentional, or should logic soundness also factor into the result?
10-77: Clean implementation following established patterns.The
LogicCheckclass follows the same dependency injection andCheckInterfacepatterns as other checks. The graceful degradation when Ollama is unavailable and the structured prompt/parsing approach are well-designed.
| $foundPatterns = []; | ||
| foreach ($this->attributionPatterns as $pattern) { | ||
| if (preg_match($pattern, $commitMessage)) { | ||
| $foundPatterns[] = str_replace(['/', 'i'], '', $pattern); | ||
| } | ||
| } |
There was a problem hiding this comment.
Pattern sanitization incorrectly removes literal 'i' characters.
The str_replace(['/', 'i'], '', $pattern) approach is too aggressive. For example, the pattern /noreply@anthropic\.com/i becomes noreply@anthropc\.com (missing the 'i' in "anthropic").
🔎 Proposed fix using regex to strip only delimiters and modifiers
foreach ($this->attributionPatterns as $pattern) {
if (preg_match($pattern, $commitMessage)) {
- $foundPatterns[] = str_replace(['/', 'i'], '', $pattern);
+ $foundPatterns[] = preg_replace('#^/|/[a-z]*$#', '', $pattern);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $foundPatterns = []; | |
| foreach ($this->attributionPatterns as $pattern) { | |
| if (preg_match($pattern, $commitMessage)) { | |
| $foundPatterns[] = str_replace(['/', 'i'], '', $pattern); | |
| } | |
| } | |
| $foundPatterns = []; | |
| foreach ($this->attributionPatterns as $pattern) { | |
| if (preg_match($pattern, $commitMessage)) { | |
| $foundPatterns[] = preg_replace('#^/|/[a-z]*$#', '', $pattern); | |
| } | |
| } |
| private function ensureModelAvailable(string $workingDirectory): void | ||
| { | ||
| $result = $this->processRunner->run( | ||
| ['sh', '-c', "ollama list | grep {$this->model}"], | ||
| $workingDirectory, | ||
| timeout: 5 | ||
| ); | ||
|
|
||
| if (! $result->successful) { | ||
| $this->processRunner->run( | ||
| ['ollama', 'pull', $this->model], | ||
| $workingDirectory, | ||
| timeout: 300 | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Shell injection vulnerability in model verification.
Line 169 uses sh -c with string interpolation of $this->model, creating a shell injection risk. Even though the model name comes from configuration, it's safer to avoid shell interpretation entirely.
🔎 Recommended fix avoiding shell interpolation
private function ensureModelAvailable(string $workingDirectory): void
{
- $result = $this->processRunner->run(
- ['sh', '-c', "ollama list | grep {$this->model}"],
- $workingDirectory,
- timeout: 5
- );
+ $result = $this->processRunner->run(
+ ['ollama', 'list'],
+ $workingDirectory,
+ timeout: 5
+ );
- if (! $result->successful) {
+ if (! $result->successful || ! str_contains($result->output, $this->model)) {
$this->processRunner->run(
['ollama', 'pull', $this->model],
$workingDirectory,
timeout: 300
);
}
}This eliminates the shell interpretation while achieving the same goal.
🤖 Prompt for AI Agents
In @app/Checks/CohesionCheck.php around lines 166-181, The code in
ensureModelAvailable uses 'sh -c' with string interpolation of $this->model
which enables shell injection; replace the shell call with a direct
processRunner->run invocation like run(['ollama','list'], ...) and inspect
$result->output in PHP to detect the model instead of piping to grep. Use a safe
text match (e.g., preg_match with preg_quote($this->model) or strict string
search) against $result->output to decide whether to run
processRunner->run(['ollama','pull',$this->model], ...); update the logic in
ensureModelAvailable to avoid any shell interpolation and reference the existing
processRunner->run and $this->model symbols.
| protected function getChangedFiles(string $base): array | ||
| { | ||
| $result = Process::run("git diff --name-only origin/{$base}...HEAD"); | ||
|
|
||
| if (! $result->successful()) { | ||
| return []; | ||
| } | ||
|
|
||
| return array_filter(explode("\n", trim($result->output()))); | ||
| } | ||
|
|
||
| protected function getPRDiff(string $base): string | ||
| { | ||
| $result = Process::run("git diff origin/{$base}...HEAD"); | ||
|
|
||
| return $result->successful() ? $result->output() : ''; | ||
| } |
There was a problem hiding this comment.
Shell injection vulnerability via unsanitized $base parameter.
The $base option is directly interpolated into shell commands without validation or escaping. A malicious value like master; rm -rf / could execute arbitrary commands.
🔎 Proposed fix using array-based command execution
protected function getChangedFiles(string $base): array
{
- $result = Process::run("git diff --name-only origin/{$base}...HEAD");
+ $result = Process::run(['git', 'diff', '--name-only', "origin/{$base}...HEAD"]);
if (! $result->successful()) {
return [];
}
return array_filter(explode("\n", trim($result->output())));
}
protected function getPRDiff(string $base): string
{
- $result = Process::run("git diff origin/{$base}...HEAD");
+ $result = Process::run(['git', 'diff', "origin/{$base}...HEAD"]);
return $result->successful() ? $result->output() : '';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function getChangedFiles(string $base): array | |
| { | |
| $result = Process::run("git diff --name-only origin/{$base}...HEAD"); | |
| if (! $result->successful()) { | |
| return []; | |
| } | |
| return array_filter(explode("\n", trim($result->output()))); | |
| } | |
| protected function getPRDiff(string $base): string | |
| { | |
| $result = Process::run("git diff origin/{$base}...HEAD"); | |
| return $result->successful() ? $result->output() : ''; | |
| } | |
| protected function getChangedFiles(string $base): array | |
| { | |
| $result = Process::run(['git', 'diff', '--name-only', "origin/{$base}...HEAD"]); | |
| if (! $result->successful()) { | |
| return []; | |
| } | |
| return array_filter(explode("\n", trim($result->output()))); | |
| } | |
| protected function getPRDiff(string $base): string | |
| { | |
| $result = Process::run(['git', 'diff', "origin/{$base}...HEAD"]); | |
| return $result->successful() ? $result->output() : ''; | |
| } |
🤖 Prompt for AI Agents
In @app/Commands/CohesionCheckCommand.php around lines 85-101, The
getChangedFiles and getPRDiff functions interpolate the $base into shell strings
leading to command injection; fix by switching to array-based Process::run calls
(e.g. Process::run(['git','diff','--name-only',"origin/{$base}...HEAD"]) and
Process::run(['git','diff',"origin/{$base}...HEAD"])) and add a simple
validation/allowlist for $base (e.g. /^[A-Za-z0-9._\/-]+$/) to reject suspicious
values before calling Process::run; if validation fails, return safe defaults
(empty array or empty string) to avoid executing commands with malicious input.
| protected function getGitRoot(): string | ||
| { | ||
| return trim(shell_exec('git rev-parse --show-toplevel')); | ||
| } |
There was a problem hiding this comment.
shell_exec() can return null, causing a TypeError.
If the git command fails, shell_exec() returns null, and trim(null) will throw a TypeError in PHP 8.1+.
🔎 Proposed fix with null coalescing
protected function getGitRoot(): string
{
- return trim(shell_exec('git rev-parse --show-toplevel'));
+ return trim(shell_exec('git rev-parse --show-toplevel') ?? '');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function getGitRoot(): string | |
| { | |
| return trim(shell_exec('git rev-parse --show-toplevel')); | |
| } | |
| protected function getGitRoot(): string | |
| { | |
| return trim(shell_exec('git rev-parse --show-toplevel') ?? ''); | |
| } |
🤖 Prompt for AI Agents
In @app/Commands/InstallHooksCommand.php around lines 107-110, In getGitRoot(),
shell_exec('git rev-parse --show-toplevel') can return null so calling trim(...)
may throw a TypeError; modify getGitRoot to capture the shell_exec result into a
variable, check for null (or empty) and handle the failure explicitly (either
cast to string or throw a RuntimeException with a clear message) and only then
call trim on the non-null output; reference the getGitRoot method and the
shell_exec('git rev-parse --show-toplevel') call when making this change.
| protected function getGateBinaryPath(): string | ||
| { | ||
| // Try to find gate in PATH | ||
| $which = trim(shell_exec('which gate')); | ||
| if ($which && file_exists($which)) { | ||
| return $which; | ||
| } | ||
|
|
||
| // Fall back to vendor/bin/gate if installed locally | ||
| if (file_exists(getcwd().'/vendor/bin/gate')) { | ||
| return './vendor/bin/gate'; | ||
| } | ||
|
|
||
| // Default to gate in PATH | ||
| return 'gate'; | ||
| } |
There was a problem hiding this comment.
shell_exec('which gate') can also return null.
Same issue as getGitRoot() - if which fails, shell_exec() returns null.
🔎 Proposed fix
protected function getGateBinaryPath(): string
{
// Try to find gate in PATH
- $which = trim(shell_exec('which gate'));
+ $which = trim(shell_exec('which gate') ?? '');
if ($which && file_exists($which)) {
return $which;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function getGateBinaryPath(): string | |
| { | |
| // Try to find gate in PATH | |
| $which = trim(shell_exec('which gate')); | |
| if ($which && file_exists($which)) { | |
| return $which; | |
| } | |
| // Fall back to vendor/bin/gate if installed locally | |
| if (file_exists(getcwd().'/vendor/bin/gate')) { | |
| return './vendor/bin/gate'; | |
| } | |
| // Default to gate in PATH | |
| return 'gate'; | |
| } | |
| protected function getGateBinaryPath(): string | |
| { | |
| // Try to find gate in PATH | |
| $which = trim(shell_exec('which gate') ?? ''); | |
| if ($which && file_exists($which)) { | |
| return $which; | |
| } | |
| // Fall back to vendor/bin/gate if installed locally | |
| if (file_exists(getcwd().'/vendor/bin/gate')) { | |
| return './vendor/bin/gate'; | |
| } | |
| // Default to gate in PATH | |
| return 'gate'; | |
| } |
🤖 Prompt for AI Agents
In @app/Commands/InstallHooksCommand.php around lines 146-161, The
getGateBinaryPath() uses trim(shell_exec('which gate')) which can return null;
capture shell_exec result into a variable (e.g., $whichRaw), check it's a
non-empty string (is_string($whichRaw) && trim($whichRaw) !== '') before calling
trim() and file_exists(), and only return that path when valid; otherwise fall
back to checking ./vendor/bin/gate and finally the default 'gate'. Ensure you
reference the getGateBinaryPath() function and replace the direct
trim(shell_exec(...)) call with the safe variable check.
| cp prototypes/gate-v1/layer-4-release.yml .github/workflows/gate-release.yml | ||
| ``` | ||
|
|
||
| ## [1.4.1] - 2024-12-XX |
There was a problem hiding this comment.
Placeholder date needs to be filled in.
The v1.4.1 release date shows 2024-12-XX which appears to be a placeholder that should be replaced with the actual release date.
🤖 Prompt for AI Agents
In @CHANGELOG.md around line 119, The CHANGELOG entry for version identifier
[1.4.1] currently uses a placeholder date "2024-12-XX"; update that header line
to replace "2024-12-XX" with the actual release date (e.g., "2024-12-15") so the
line reads "## [1.4.1] - YYYY-MM-DD" using the correct release date for v1.4.1.
- Add tests for AttributionCheck, LogicCheck, and CohesionCheck (100% coverage) - Add tests for AttributionCheckCommand, LogicCheckCommand, CohesionCheckCommand - Refactor Command classes to use dependency injection for testability - Improve overall test coverage from 52.5% to 92.2% The Check classes now have complete test coverage including edge cases, error handling, and model fallback scenarios. Command classes refactored to delegate to Check classes following existing SecurityCommand pattern.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @app/Commands/AttributionCheckCommand.php:
- Around line 50-62: The code calls ChecksClient::isAvailable() statically but
isAvailable() is an instance method; instead, first obtain an instance (use
$client = $this->checksClient ?? ChecksClient::fromEnvironment()), then call
$client->isAvailable() and only proceed to $client->reportCheck(...) if that
instance method returns true; update the condition and remove the static call to
isAvailable() while keeping use of reportCheck, $this->checksClient,
fromEnvironment and $summary intact.
In @app/Commands/CohesionCheckCommand.php:
- Around line 50-62: The code currently calls the instance method isAvailable()
statically on ChecksClient which will cause a runtime error; instead always
obtain an instance first (use $this->checksClient if set, otherwise call
ChecksClient::fromEnvironment()), then call ->isAvailable() on that instance and
only call $client->reportCheck(...) if the instance reports available; update
the block using the instance variable $client (from
ChecksClient::fromEnvironment() or $this->checksClient) and check
$client->isAvailable() before reporting.
In @tests/Unit/Checks/AttributionCheckTest.php:
- Line 70: The sanitization in app/Checks/AttributionCheck.php mistakenly
removes all 'i' characters by using str_replace(['/', 'i'], '', $pattern) which
corrupts words like "with" and "anthropic"; update the sanitization in the
method that builds $foundPatterns (the code referencing str_replace(['/', 'i'],
'', $pattern)) to only strip regex delimiters and modifiers (e.g., remove
leading/trailing slashes and trailing modifier letters) or, better, refactor
$attributionPatterns into structures with separate 'pattern' and human-readable
'display' fields (e.g., private array $attributionPatterns = [['pattern'=>...,
'display'=>...], ...]) and use 'display' when populating $foundPatterns; then
update the test in Tests/Unit/Checks/AttributionCheckTest.php to expect the
corrected display text ("Generated with [Claude Code]" or the chosen display
string) instead of the broken "wth" variant.
- Line 108: The sanitization is incorrectly removing the letter "i" because the
code uses str_replace(['/', 'i'], '', $pattern); change the sanitizer to only
strip regex delimiters (e.g. use str_replace('/', '', $pattern) or better
trim($pattern, '/')) so the "i" flag/character is not removed from strings like
"anthropic", and keep any tests expecting "anthropic" (update the test
expectation if it was incorrectly asserting "anthropc"); locate the usage by
searching for str_replace(['/', 'i'], '', $pattern) and replace it with the
safer trimming/stripping logic.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/Commands/AttributionCheckCommand.phpapp/Commands/CohesionCheckCommand.phpapp/Commands/LogicCheckCommand.phptests/Unit/Checks/AttributionCheckTest.phptests/Unit/Checks/CohesionCheckTest.phptests/Unit/Checks/LogicCheckTest.phptests/Unit/Commands/AttributionCheckCommandTest.phptests/Unit/Commands/CohesionCheckCommandTest.phptests/Unit/Commands/LogicCheckCommandTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Commands/LogicCheckCommand.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T20:12:44.070Z
Learnt from: CR
Repo: synapse-sentinel/gate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T20:12:44.070Z
Learning: Organize application code with Commands, Checks, GitHub, and Stages subdirectories within `app/` directory
Applied to files:
app/Commands/CohesionCheckCommand.php
🧬 Code graph analysis (6)
tests/Unit/Commands/CohesionCheckCommandTest.php (3)
app/Checks/CheckResult.php (3)
CheckResult(7-24)pass(15-18)fail(20-23)app/Commands/CohesionCheckCommand.php (2)
CohesionCheckCommand(10-66)withMocks(20-24)app/GitHub/ChecksClient.php (1)
ChecksClient(10-203)
tests/Unit/Checks/AttributionCheckTest.php (2)
app/Checks/AttributionCheck.php (1)
AttributionCheck(10-70)app/Contracts/ProcessResult.php (1)
ProcessResult(7-14)
tests/Unit/Checks/LogicCheckTest.php (2)
app/Checks/LogicCheck.php (1)
LogicCheck(10-216)app/Contracts/ProcessResult.php (1)
ProcessResult(7-14)
tests/Unit/Commands/LogicCheckCommandTest.php (3)
app/Checks/CheckResult.php (3)
CheckResult(7-24)pass(15-18)fail(20-23)app/Commands/LogicCheckCommand.php (2)
LogicCheckCommand(10-66)withMocks(20-24)app/GitHub/ChecksClient.php (1)
ChecksClient(10-203)
tests/Unit/Checks/CohesionCheckTest.php (2)
app/Checks/CohesionCheck.php (1)
CohesionCheck(10-273)app/Contracts/ProcessResult.php (1)
ProcessResult(7-14)
app/Commands/CohesionCheckCommand.php (5)
app/Checks/CohesionCheck.php (2)
CohesionCheck(10-273)run(24-77)app/GitHub/ChecksClient.php (2)
ChecksClient(10-203)reportCheck(124-159)app/Commands/AttributionCheckCommand.php (2)
withMocks(20-24)handle(26-65)app/Commands/LogicCheckCommand.php (2)
withMocks(20-24)handle(26-65)app/Checks/LogicCheck.php (1)
run(23-77)
🔇 Additional comments (10)
tests/Unit/Checks/LogicCheckTest.php (1)
1-289: Excellent test coverage for LogicCheck!The test suite comprehensively covers all execution paths including:
- Ollama availability checks (not installed, not running)
- Early exit scenarios (no staged files, no changes)
- Success and failure paths for atomicity validation
- Model availability and automatic pulling
- Error handling (analysis failures, git command failures)
The mock setup is clean and the assertions correctly validate the expected behavior.
tests/Unit/Commands/LogicCheckCommandTest.php (1)
1-91: LGTM! Clean command integration tests.The test suite properly validates the LogicCheckCommand by mocking dependencies and verifying success/failure outcomes, detail rendering, and token handling. The pattern is consistent with other check commands in the codebase.
tests/Unit/Commands/AttributionCheckCommandTest.php (1)
1-91: LGTM! Consistent command test pattern.The test suite follows the established pattern for check commands, providing adequate coverage of the command wrapper behavior.
tests/Unit/Commands/CohesionCheckCommandTest.php (1)
1-92: LGTM! Follows established test conventions.The test suite maintains consistency with other check command tests, properly validating the CohesionCheckCommand integration.
tests/Unit/Checks/AttributionCheckTest.php (1)
1-166: Comprehensive attribution detection test coverage.Aside from the pattern sanitization issue flagged above (which is a production code bug that tests correctly validate), the test suite provides excellent coverage of:
- All attribution patterns (emoji, text, co-authored variants)
- Edge cases (no commit, clean commits, multiple patterns)
- Command invocation verification
tests/Unit/Checks/CohesionCheckTest.php (1)
1-415: Excellent test coverage!The test suite is comprehensive and well-structured, covering:
- Early exits (Ollama unavailable, no files)
- Multi-branch fallback logic
- Success and failure scenarios
- Edge cases (model pulling, all diffs fail, PR diff unavailable)
- File categorization validation
- Detailed result message validation
The mocking strategy is appropriate and the tests clearly validate the CohesionCheck behavior.
app/Commands/CohesionCheckCommand.php (2)
20-24: Good use of dependency injection for testability.The
withMocksmethod enables clean testing by allowing test doubles to be injected, following the same pattern as LogicCheckCommand and AttributionCheckCommand.
26-49: Clean command structure and output formatting.The handle method properly delegates to the CohesionCheck instance, formats output clearly, and displays detailed failure messages when present.
app/Commands/AttributionCheckCommand.php (2)
20-24: Good use of dependency injection for testability.The
withMocksmethod enables clean testing by allowing test doubles to be injected, consistent with the pattern used across other check commands.
26-49: Clean command structure and delegation.The handle method properly delegates to AttributionCheck, formats output clearly, and displays detailed messages when available.
| // Report to GitHub if in CI | ||
| if ($this->checksClient ?? ChecksClient::isAvailable()) { | ||
| $client = $this->checksClient ?? ChecksClient::fromEnvironment(); | ||
| $summary = !empty($result->details) | ||
| ? implode("\n", $result->details) | ||
| : $result->message; | ||
| $client->reportCheck( | ||
| 'Attribution Check', | ||
| $result->passed, | ||
| $result->message, | ||
| $summary | ||
| ); | ||
| } |
There was a problem hiding this comment.
Critical: Static call to instance method will cause runtime error.
Line 51 attempts to call ChecksClient::isAvailable() statically, but this is an instance method that uses $this. This will cause a PHP fatal error at runtime when $this->checksClient is null.
🔎 Proposed fix
// Report to GitHub if in CI
- if ($this->checksClient ?? ChecksClient::isAvailable()) {
- $client = $this->checksClient ?? ChecksClient::fromEnvironment();
+ $client = $this->checksClient ?? ChecksClient::fromEnvironment();
+ if ($client->isAvailable()) {
$summary = !empty($result->details)
? implode("\n", $result->details)
: $result->message;
$client->reportCheck(
'Attribution Check',
$result->passed,
$result->message,
$summary
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Report to GitHub if in CI | |
| if ($this->checksClient ?? ChecksClient::isAvailable()) { | |
| $client = $this->checksClient ?? ChecksClient::fromEnvironment(); | |
| $summary = !empty($result->details) | |
| ? implode("\n", $result->details) | |
| : $result->message; | |
| $client->reportCheck( | |
| 'Attribution Check', | |
| $result->passed, | |
| $result->message, | |
| $summary | |
| ); | |
| } | |
| // Report to GitHub if in CI | |
| $client = $this->checksClient ?? ChecksClient::fromEnvironment(); | |
| if ($client->isAvailable()) { | |
| $summary = !empty($result->details) | |
| ? implode("\n", $result->details) | |
| : $result->message; | |
| $client->reportCheck( | |
| 'Attribution Check', | |
| $result->passed, | |
| $result->message, | |
| $summary | |
| ); | |
| } |
🤖 Prompt for AI Agents
In @app/Commands/AttributionCheckCommand.php around lines 50-62, The code calls
ChecksClient::isAvailable() statically but isAvailable() is an instance method;
instead, first obtain an instance (use $client = $this->checksClient ??
ChecksClient::fromEnvironment()), then call $client->isAvailable() and only
proceed to $client->reportCheck(...) if that instance method returns true;
update the condition and remove the static call to isAvailable() while keeping
use of reportCheck, $this->checksClient, fromEnvironment and $summary intact.
| // Report to GitHub if in CI | ||
| if ($this->checksClient ?? ChecksClient::isAvailable()) { | ||
| $client = $this->checksClient ?? ChecksClient::fromEnvironment(); | ||
| $summary = !empty($result->details) | ||
| ? implode("\n", $result->details) | ||
| : $result->message; | ||
| $client->reportCheck( | ||
| 'PR Cohesion', | ||
| $result->passed, | ||
| $result->message, | ||
| $summary | ||
| ); | ||
| } |
There was a problem hiding this comment.
Critical: Static call to instance method will cause runtime error.
Line 51 attempts to call ChecksClient::isAvailable() statically, but this is an instance method that uses $this (as shown in the ChecksClient snippet). This will cause a PHP fatal error at runtime when $this->checksClient is null.
The ?? operator evaluates the right side when the left is null, so when running without mocked dependencies, this code path will fail.
🔎 Proposed fix
// Report to GitHub if in CI
- if ($this->checksClient ?? ChecksClient::isAvailable()) {
- $client = $this->checksClient ?? ChecksClient::fromEnvironment();
+ $client = $this->checksClient ?? ChecksClient::fromEnvironment();
+ if ($client->isAvailable()) {
$summary = !empty($result->details)
? implode("\n", $result->details)
: $result->message;
$client->reportCheck(
'PR Cohesion',
$result->passed,
$result->message,
$summary
);
}This fix:
- Always obtains a client instance (either injected or from environment)
- Calls the instance method
isAvailable()on the obtained instance - Only proceeds with reporting if the client is properly configured
🤖 Prompt for AI Agents
In @app/Commands/CohesionCheckCommand.php around lines 50-62, The code currently
calls the instance method isAvailable() statically on ChecksClient which will
cause a runtime error; instead always obtain an instance first (use
$this->checksClient if set, otherwise call ChecksClient::fromEnvironment()),
then call ->isAvailable() on that instance and only call
$client->reportCheck(...) if the instance reports available; update the block
using the instance variable $client (from ChecksClient::fromEnvironment() or
$this->checksClient) and check $client->isAvailable() before reporting.
|
|
||
| expect($result->passed)->toBeFalse(); | ||
| expect($result->message)->toBe('Claude Code attribution detected in commit'); | ||
| expect($result->details)->toContain('🤖 Generated wth \[Claude Code\]'); |
There was a problem hiding this comment.
Production code bug: pattern sanitization removes letters from words.
The test correctly validates the current behavior, but the production code has a bug. Line 70 expects '🤖 Generated wth \[Claude Code\]' (note "wth" instead of "with").
This occurs because app/Checks/AttributionCheck.php uses:
$foundPatterns[] = str_replace(['/', 'i'], '', $pattern);This removes ALL occurrences of / and i, not just the regex delimiters. So "with" becomes "wth", and "anthropic" becomes "anthropc" (see line 108).
🔎 Recommended fix for production code
In app/Checks/AttributionCheck.php, replace the pattern sanitization:
-$foundPatterns[] = str_replace(['/', 'i'], '', $pattern);
+$foundPatterns[] = trim($pattern, '/i');Or better yet, maintain separate human-readable pattern descriptions:
private array $attributionPatterns = [
['pattern' => '/🤖 Generated with \[Claude Code\]/i', 'display' => 'Generated with Claude Code (emoji)'],
// ... etc
];Then update the tests to expect the corrected output.
🤖 Prompt for AI Agents
In @tests/Unit/Checks/AttributionCheckTest.php around line 70, The sanitization
in app/Checks/AttributionCheck.php mistakenly removes all 'i' characters by
using str_replace(['/', 'i'], '', $pattern) which corrupts words like "with" and
"anthropic"; update the sanitization in the method that builds $foundPatterns
(the code referencing str_replace(['/', 'i'], '', $pattern)) to only strip regex
delimiters and modifiers (e.g., remove leading/trailing slashes and trailing
modifier letters) or, better, refactor $attributionPatterns into structures with
separate 'pattern' and human-readable 'display' fields (e.g., private array
$attributionPatterns = [['pattern'=>..., 'display'=>...], ...]) and use
'display' when populating $foundPatterns; then update the test in
Tests/Unit/Checks/AttributionCheckTest.php to expect the corrected display text
("Generated with [Claude Code]" or the chosen display string) instead of the
broken "wth" variant.
| expect($result->passed)->toBeFalse(); | ||
| expect($result->message)->toBe('Claude Code attribution detected in commit'); | ||
| expect($result->details)->toContain('Co-Authored-By: Claude'); | ||
| expect($result->details)->toContain('noreply@anthropc\.com'); |
There was a problem hiding this comment.
Same pattern sanitization issue: "anthropic" becomes "anthropc".
This is the same production code bug as line 70, where str_replace(['/', 'i'], '', $pattern) removes the letter 'i' from "anthropic".
🤖 Prompt for AI Agents
In @tests/Unit/Checks/AttributionCheckTest.php around line 108, The sanitization
is incorrectly removing the letter "i" because the code uses str_replace(['/',
'i'], '', $pattern); change the sanitizer to only strip regex delimiters (e.g.
use str_replace('/', '', $pattern) or better trim($pattern, '/')) so the "i"
flag/character is not removed from strings like "anthropic", and keep any tests
expecting "anthropic" (update the test expectation if it was incorrectly
asserting "anthropc"); locate the usage by searching for str_replace(['/', 'i'],
'', $pattern) and replace it with the safer trimming/stripping logic.
Prevents memory exhaustion when running full test suite with extensive mocking. Required for tests with multiple mock expectations per test.
- Add edge case tests for command failure without details - Extend CertifyCommand tests to cover all check name shortening cases - Add basic InstallHooksCommand tests (excluded from coverage) - Exclude InstallHooksCommand from coverage (requires extensive filesystem mocking) - Improve test coverage from 92.2% to 100%
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Unit/Commands/InstallHooksCommandTest.php (1)
8-25: Improve directory cleanup reliability.The test uses
chdir('/')for cleanup (Line 20), which assumes '/' is always accessible. It's more robust to save and restore the original working directory.🔎 Proposed fix for more reliable directory handling
it('returns failure when not in git repository', function () { // Test in a non-git directory $tempDir = sys_get_temp_dir().'/not-a-git-repo-'.uniqid(); mkdir($tempDir); + $originalDir = getcwd(); try { chdir($tempDir); $this->artisan('install') ->expectsOutput('Not in a git repository') ->assertFailed(); } finally { - chdir('/'); + chdir($originalDir); if (is_dir($tempDir)) { rmdir($tempDir); } } });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
phpunit.xmltests/Unit/Checks/CohesionCheckTest.phptests/Unit/Commands/AttributionCheckCommandTest.phptests/Unit/Commands/CertifyCommandTest.phptests/Unit/Commands/CohesionCheckCommandTest.phptests/Unit/Commands/InstallHooksCommandTest.phptests/Unit/Commands/LogicCheckCommandTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/Unit/Checks/CohesionCheckTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Unit/Commands/CohesionCheckCommandTest.php
- tests/Unit/Commands/LogicCheckCommandTest.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T20:12:44.070Z
Learnt from: CR
Repo: synapse-sentinel/gate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T20:12:44.070Z
Learning: Applies to **/*.test.php : 100% code coverage required for all tests (enforced with `vendor/bin/pest --coverage --min=100`)
Applied to files:
phpunit.xml
🧬 Code graph analysis (3)
tests/Unit/Commands/AttributionCheckCommandTest.php (2)
app/Checks/CheckResult.php (3)
CheckResult(7-24)pass(15-18)fail(20-23)app/Commands/AttributionCheckCommand.php (2)
AttributionCheckCommand(10-66)withMocks(20-24)
tests/Unit/Commands/InstallHooksCommandTest.php (1)
app/Commands/InstallHooksCommand.php (1)
InstallHooksCommand(8-249)
tests/Unit/Commands/CertifyCommandTest.php (2)
app/Checks/CheckResult.php (2)
CheckResult(7-24)pass(15-18)app/GitHub/ChecksClient.php (1)
ChecksClient(10-203)
🔇 Additional comments (4)
phpunit.xml (1)
12-14: LGTM: Configuration changes align with v2.0.0 requirements.The memory limit increase to 512M is appropriate for AI-powered validation checks, and the exclusion of
InstallHooksCommand.phpfrom coverage is reasonable given the extensive filesystem mocking that would be required (as noted in the PR objectives).Also applies to: 29-29
tests/Unit/Commands/InstallHooksCommandTest.php (1)
27-35: Note: Property tests are minimal by design.These tests only verify property existence rather than their values. Given that
InstallHooksCommandis excluded from coverage (Line 29 in phpunit.xml) to avoid extensive filesystem mocking, this minimal testing approach is consistent with the coverage strategy.tests/Unit/Commands/CertifyCommandTest.php (1)
343-377: LGTM: Clean integration of new AI-powered checks.The additions properly extend the test to include the three new checks (Attribution, Logic & Atomicity, PR Cohesion) with:
- Correct mock setup following the existing CheckInterface pattern
- Proper HTTP response count (7 total: 6 checks + 1 certification)
- Consistent naming that exercises the
shortName()method mappingThe integration maintains consistency with the existing test patterns.
tests/Unit/Commands/AttributionCheckCommandTest.php (1)
1-107: LGTM: Comprehensive command wiring tests.This test suite provides thorough coverage of the
AttributionCheckCommandwiring layer:
- ✅ Success and failure paths
- ✅ Detail display behavior
- ✅ Token propagation
- ✅ GitHub checks integration via mocked HTTP client
The tests maintain good separation of concerns (command wiring vs. check logic) and follow consistent patterns with other command tests in the PR.
- Add PR number parameter to all ChecksClient test instances - Maintain 100% test coverage after master merge - Update command tests to use new ChecksClient constructor signature
1 participant
Gate v2.0.0 - AI-Powered Validation 🤖
Major release adding AI-powered code validation using local Ollama models.
What's New
🔍 Attribution Check
gate check:attribution --fixcommand🧠 Logic & Atomicity Validation
🔗 PR Cohesion Analysis
🪝 Git Hooks Installation
gate installcommand.gate/config.phpfor customizationgate certifybefore each commit📊 AI Code Review (GitHub Actions)
🚀 Semantic Release Automation
New Commands
New Check Classes
AttributionCheck- Validates commit messages are cleanLogicCheck- Ollama-powered atomicity validationCohesionCheck- PR-level cross-file analysisTechnical Details
llama3.2:3b(fast),qwen2.5-coder:7b(deep review)Performance
Migration
No breaking changes! v2.0.0 is fully backwards compatible.
Optional enhancements:
Files Changed
CHANGELOG.mdapp/Checks/AttributionCheck.phpapp/Checks/LogicCheck.phpapp/Checks/CohesionCheck.phpapp/Commands/InstallHooksCommand.phpapp/Commands/AttributionCheckCommand.phpapp/Commands/LogicCheckCommand.phpapp/Commands/CohesionCheckCommand.phpapp/Commands/CertifyCommand.phpREADME.mdTesting
All new checks implement the existing
CheckInterfacepattern and integrate seamlessly withgate certify.Tested on:
Ready to merge and tag as v2.0.0 🎉
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.