Development environment setup#20
Merged
Merged
Conversation
- AGENTS.md: Cloud-specific instructions for dev environment - testbench.yaml: Register WardenServiceProvider for vendor/bin/testbench Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
Required by tests in tests/ which use Orchestra\Testbench\TestCase. This also brings in phpunit, mockery, and laravel/framework. Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
1. Rename --silent to --no-notify to avoid collision with Laravel 12's built-in --silent flag that suppresses all console output 2. Suppress version line and spinner when --output is set so JSON/CI output is machine-parseable (no stdout pollution) 3. Warn when no notification channels are configured instead of falsely printing 'Notifications sent.' 4. Replace Cache::flush() with targeted key deletion to avoid nuking the entire application cache on --force Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
- Validate --severity values and error on invalid input - Show filtered count when severity filter hides issues - Fix grammar: '1 vulnerabilities' -> '1 issue' - Add severity emoji indicators in console table - Hide CVE/Link/Affected columns when no findings have CVE data - Emit ::notice annotation for zero-finding GitHub Actions output - Warn on conflicting --enable --disable flags in schedule command - Update success message to '✅ No security issues found.' Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
- Move CustomAuditWrapper to its own file under Services/ - Remove dead JsonFormatter class (inline methods in command are used) - Remove stray info() debug calls from ComposerAuditService - Fix hardcoded version 'v1.3.5' in email template - Remove unused config keys: retry_attempts, retry_delay, severity_filter, output_formats (none had backing code) Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
- Replace single-purpose phpstan.yml with comprehensive tests.yml that runs PHPStan, PHPUnit, and Rector across PHP 8.3 and 8.4 - Add phpunit.xml configuration - Fix WardenSyntaxCommand to use constructor DI instead of new - Fix WardenAuditCommand to resolve services from container via app() - Rewrite both test files with correct mock expectations: - Mock addAudit() on ParallelAuditExecutor - Use --no-notify instead of removed --silent flag - Match updated grammar in assertions - All 5 tests now pass Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
- Rename ParallelAuditExecutor -> AuditExecutor (it was never parallel; no service implements runAsync). Remove dead async code path. - Update spinner text from 'parallel' to just 'Running security audits...' - Extract duplicated getSeverityCounts/getHighestSeverity/getSeverityEmoji/ groupFindingsBySource into HasSeverityHelpers trait - Apply trait to EmailChannel, TeamsChannel (removing ~100 lines of duplication) - Update all references: command, provider, tests Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
Existing users passing --silent still get notification suppression. On Symfony Console 7.2+ (Laravel 11+), --silent is a framework-level option that also suppresses all output. We detect this via the output's isSilent() method so both behaviors work together: --no-notify: suppress notifications, keep output visible --silent: suppress notifications AND all console output (framework) Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
The Laravel Prompts spin() function emits ANSI escape codes that render as garbage in CI logs. Replace with per-audit line-by-line output: Interactive terminal (TTY): - Shows '⏳ Running X audit ...' then overwrites with '✓ X audit (Nms)' - Failed audits show '✗ X audit failed (Nms)' CI / piped output (non-TTY): - Both 'running' and 'done/failed' lines visible as plain text - No escape sequences, no spinner artifacts Machine output (--output=json/github/gitlab/jenkins): - No progress lines at all, clean structured output Also adds timing per audit and a styled version header. Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
- Remove redundant is_string() check for severity option (already narrowed) - Replace method_exists() with instanceof for audit service getName() - Use iterator_to_array() for RouteCollection to fix foreach.nonIterable Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
…phpstan-analysis-issues-f0b8
Phpstan analysis issues
- WardenAuditCommand: Remove redundant method_exists check (output is always OutputStyle) - WardenAuditCommand: Initialize totalBeforeFilter and severityOption to fix undefined variable - WardenAuditCommand: Guard severityOption usage with null check - DebugModeAuditService: Use getRoutes()->getRoutes() instead of iterator_to_array for RouteCollectionInterface Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces ParallelAuditExecutor with a DI-driven AuditExecutor and AuditServiceInterface; adds CustomAuditWrapper and HasSeverityHelpers; removes JsonFormatter and ParallelAuditExecutor; refactors commands for DI, machine-output and --no-notify; narrows cache clearing; updates CI/test workflows, docs, tests, config and view footer. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (artisan)
participant Cmd as WardenAuditCommand
participant Exec as AuditExecutor
participant Svc as AuditService
participant Cache as AuditCacheService
participant Notif as NotificationChannels
CLI->>Cmd: run audit command (--no-notify / format)
Cmd->>Exec: addAudit(service)
Cmd->>Exec: execute(onProgress)
Exec->>Svc: run() / runAsync()
Svc-->>Exec: result (success, findings, duration)
Exec-->>Cmd: aggregated results
Cmd->>Cache: clear/forget audit keys (if requested)
Cmd->>Notif: sendNotifications(filtered findings) [skipped if suppressed]
Cmd-->>CLI: exit code / formatted output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Providers/WardenServiceProvider.php (1)
58-58:⚠️ Potential issue | 🟠 MajorUpdate flag from
--silentto--no-notify.The warden:audit command signature defines only
--no-notify(which replaces the deprecated--silent), but the scheduled command at line 58 still references--silent. This will cause the scheduled audit to fail. Change'warden:audit --silent'to'warden:audit --no-notify'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/WardenServiceProvider.php` at line 58, In WardenServiceProvider update the scheduled command string: replace the deprecated flag `--silent` with the current `--no-notify` so the `$schedule->command('warden:audit --silent')` call becomes `$schedule->command('warden:audit --no-notify')`; locate this in the schedule registration (WardenServiceProvider, the line creating $event) and modify the command argument accordingly to match the warden:audit signature.src/Commands/WardenAuditCommand.php (2)
140-162:⚠️ Potential issue | 🟠 MajorMachine output is still polluted in sequential mode.
$isMachineOutputis currently unused, so Lines 152 and 161 still print human-oriented status text when--outputis set. This can break parsers expecting pure JSON/CI payloads.Proposed fix
protected function runSequentialAudits(bool $isMachineOutput = false): int { @@ if (!$this->option('force') && $this->cacheService->hasRecentAudit($auditName)) { - $this->info(sprintf('Using cached results for %s audit...', $auditName)); + if (!$isMachineOutput) { + $this->info(sprintf('Using cached results for %s audit...', $auditName)); + } $cached = $this->cacheService->getCachedResult($auditName); @@ - $this->info(sprintf('Running %s audit...', $auditName)); + if (!$isMachineOutput) { + $this->info(sprintf('Running %s audit...', $auditName)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Commands/WardenAuditCommand.php` around lines 140 - 162, The runSequentialAudits method is ignoring the $isMachineOutput flag, so human-oriented messages still print; wrap the user-facing $this->info(...) calls (at least the "Using cached results for %s audit..." and "Running %s audit..." messages inside runSequentialAudits) in a conditional that checks !$isMachineOutput before emitting them, ensuring cached merging and audit logic (cacheService->hasRecentAudit, cacheService->getCachedResult, array_merge into $allFindings) remain unchanged while suppressing human output when $isMachineOutput is true.
716-727:⚠️ Potential issue | 🟠 MajorEscape GitHub Actions workflow command fields before writing annotations.
Dynamic values (
$title,$package,$finding['severity']) are written directly into workflow commands and must be percent-encoded. The GitHub Actions runner parses the format::command key=value::messageand re-interprets special characters.For property fields (like
title=), escape:%→%25,\r→%0D,\n→%0A,:→%3A,,→%2CFor message content, escape:
%→%25,\r→%0D,\n→%0AUnescaped colons or commas in
$titleor$packagewill corrupt the command parsing. Unescaped percent signs, newlines, or carriage returns anywhere will produce unexpected output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Commands/WardenAuditCommand.php` around lines 716 - 727, Escape workflow command fields and message content before writing annotations: when iterating over $findings in the foreach ($findings as $finding) block, percent-encode property fields used in the command key=value pairs (escape %→%25, \r→%0D, \n→%0A, :→%3A, ,→%2C) for $title and any other property values, and percent-encode message content (escape %→%25, \r→%0D, \n→%0A) for the message part that includes $package and $finding['severity']; update the $this->output->writeln call to use these encoded values (replace raw $title, $package, $finding['severity'] with their encoded counterparts) so the GitHub Actions ::command key=value::message format is never corrupted.
🧹 Nitpick comments (4)
src/Providers/WardenServiceProvider.php (1)
24-26: Remove unused$appparameter.The
$appparameter in the closure is not used sinceAuditExecutorhas no dependencies.💡 Suggested fix
- $this->app->bind(AuditExecutor::class, function ($app) { + $this->app->bind(AuditExecutor::class, function () { return new AuditExecutor(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Providers/WardenServiceProvider.php` around lines 24 - 26, The closure passed to $this->app->bind for AuditExecutor includes an unused $app parameter; remove the unused parameter so the binding becomes a no-argument closure that simply returns new AuditExecutor() (update the binding that references AuditExecutor in WardenServiceProvider to use function() instead of function($app)). Ensure there are no references to $app inside the closure before removing it.src/Services/AuditExecutor.php (1)
13-16: Consider using an interface type hint instead ofobject.The
addAuditmethod accepts anyobject, but later callsgetName(),run(), andgetFindings()on it. Using a more specific type (e.g., anAuditServiceInterfaceorAbstractAuditService) would provide compile-time safety and better IDE support.💡 Suggested improvement
Based on the relevant code snippets,
AbstractAuditServicealready definesrun(): boolandgetFindings(): array. Consider:+use Dgtlss\Warden\Services\Audits\AbstractAuditService; + class AuditExecutor { - protected array $audits = []; + /** `@var` array<string, AbstractAuditService> */ + protected array $audits = []; - public function addAudit(object $auditService): void + public function addAudit(AbstractAuditService $auditService): void { $this->audits[$auditService->getName()] = $auditService; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/AuditExecutor.php` around lines 13 - 16, The addAudit method in AuditExecutor currently type-hints a generic object but then calls getName(), run(), and getFindings(); change the parameter to the specific contract (e.g., AbstractAuditService or AuditServiceInterface) so callers get compile-time checks and IDE hints. Update the method signature of addAudit(object $auditService): void to use the chosen type (e.g., addAudit(AbstractAuditService $auditService): void), and adjust any related property typing for $audits to reflect the new type (and add the appropriate use/import for AbstractAuditService or the interface)..github/workflows/tests.yml (1)
30-31: Consider caching Composer dependencies for faster CI runs.Adding dependency caching can significantly speed up workflow execution, especially as the dependency count grows.
💡 Suggested improvement
- name: Set up PHP uses: shivammathur/setup-php@v2 with: php-version: ${{ matrix.php }} tools: composer coverage: none + - name: Get Composer cache directory + id: composer-cache + run: echo "dir=$(composer config cache-files-dir)" >> $GITHUB_OUTPUT + + - name: Cache Composer dependencies + uses: actions/cache@v4 + with: + path: ${{ steps.composer-cache.outputs.dir }} + key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.lock') }} + restore-keys: ${{ runner.os }}-composer- + - name: Install dependencies run: composer install --no-interaction --no-progress🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 30 - 31, Add a Composer cache step before the "Install dependencies" step to speed CI: create a cache using actions/cache keyed by composer.lock (and optionally matrix PHP version) to store ~/.composer/cache (or vendor) so subsequent runs restore dependencies, then run "composer install --no-interaction --no-progress" as before; update the workflow to add the cache step immediately before the step named "Install dependencies" and reference composer.lock in the cache key to invalidate when dependencies change.src/Services/AuditCacheService.php (1)
75-95: Hardcoded audit names may drift from actual implementations.The
AUDIT_NAMESconstant requires manual updates when new audits are added, creating a maintenance burden. Consider discovering audit names dynamically from registered services, or at minimum, add a comment documenting that this list must be kept in sync.Also, the constant placement between the
clearCachedocblock (line 72-74) and the method signature (line 87) is unconventional—constants are typically declared at the top of the class.Proposed fix: Move constant to class top
class AuditCacheService { + /** + * Known audit names used for cache key generation. + * Keep in sync with audit service getName() returns. + */ + private const AUDIT_NAMES = [ + 'composer', + 'npm', + 'environment', + 'storage', + 'debug-mode', + 'PHP Syntax', + ]; + protected string $cachePrefix = 'warden:audit:'; protected int $cacheDuration;And remove lines 75-85 from their current location.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/AuditCacheService.php` around lines 75 - 95, The AUDIT_NAMES constant is hardcoded and misplaced; move the private const AUDIT_NAMES = [...] declaration to the top of the class (above properties/constructor) and either replace it with a dynamic discovery of audit names from the registered audit services or add a clear inline comment on AUDIT_NAMES stating it must be kept in sync with registered audits; then update clearCache(?string $auditName) to iterate discovered names (or AUDIT_NAMES) when $auditName is null and continue using $this->getCacheKey($name) and Cache::forget as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 3: Update the header text "Cursor Cloud specific instructions" to
hyphenate the compound modifier so it reads "Cursor Cloud-specific
instructions"; locate the header in AGENTS.md and replace the unhyphenated
phrase wherever used in that heading to maintain consistent style.
- Line 7: Edit the sentence containing "Orchestra Testbench" in AGENTS.md to add
a comma after that phrase for readability; specifically update the fragment
"Orchestra Testbench which bootstraps" to "Orchestra Testbench, which
bootstraps" so the sentence reads that it bootstraps a minimal Laravel
environment in-memory.
In `@composer.json`:
- Around line 37-38: composer.json declares support for illuminate/support ^7.0
|| ^8.0 || ^9.0 || ^10.0 || ^11.0 || ^12.0 but the dev dependency
"orchestra/testbench": "^10.9" pins tests to Laravel 12; update the testbench
constraint to match the full Laravel support matrix (e.g., add a multi-version
set like "orchestra/testbench": "^7.0 || ^8.0 || ^9.0 || ^10.0" to cover Laravel
7–11 and ^10.9 for 12, or choose the specific testbench major that matches the
lowest supported Laravel and add separate CI matrix entries), or alternatively
document in README/composer that tests only target Laravel 12+—adjust the
"orchestra/testbench" entry accordingly and ensure CI test matrix aligns with
the chosen approach.
In `@src/Commands/WardenAuditCommand.php`:
- Around line 185-201: The call to handleAbandonedPackages currently runs before
checking the --output option, causing human-readable warnings to be emitted
before machine-formatted output; move the handleAbandonedPackages(...)
invocation so it only runs in the non-formatted branch (after checking output
and after outputFormattedResults is NOT chosen), i.e., keep
outputFormattedResults($allFindings, (string)$outputFormat) and its early return
path untouched but relocate handleAbandonedPackages($abandonedPackages) to run
only when $outputFormat is falsy (or run it after the formatted-output branch
returns), ensuring when option('output') is set no abandoned-package output is
emitted before outputFormattedResults.
In `@src/Notifications/Channels/EmailChannel.php`:
- Around line 11-12: Add a blank line between the trait use statement and the
property declaration to satisfy NewlineBetweenClassLikeStmtsRector: insert a
single empty line after the "use
\Dgtlss\Warden\Notifications\Concerns\HasSeverityHelpers;" line so that the
protected ?string $recipients; property is separated from the trait use (in
class EmailChannel or wherever HasSeverityHelpers is used).
In `@src/Notifications/Channels/TeamsChannel.php`:
- Around line 10-11: Add a blank line between the trait use statement
HasSeverityHelpers and the property declaration $webhookUrl in the TeamsChannel
class so there is an empty line separating class-like statements; update the
area where "use \Dgtlss\Warden\Notifications\Concerns\HasSeverityHelpers;" and
"protected ?string $webhookUrl;" appear to include one newline between them to
satisfy NewlineBetweenClassLikeStmtsRector.
In `@src/Services/AuditExecutor.php`:
- Around line 32-62: The execute() method currently builds results in a local
$results variable but never assigns it to the instance property, so accessor
methods (getAllFindings(), hasFailures(), getFailedAudits()) see an empty
$this->results; fix by populating the instance property—either assign
$this->results = $results just before returning in execute(), or build directly
into $this->results inside the foreach (use $this->results[$name] = ...),
ensuring subsequent calls to getAllFindings(), hasFailures(), and
getFailedAudits() operate on the populated $this->results.
In `@src/Services/CustomAuditWrapper.php`:
- Around line 23-32: The run() method only assigns $this->findings when
$this->customAudit->audit() returns false, losing findings when audit() returns
true; modify run() (in class CustomAuditWrapper) to always retrieve and store
findings by calling $this->customAudit->getFindings() after calling audit(),
assigning the result to $this->findings regardless of $success, then return
$success.
---
Outside diff comments:
In `@src/Commands/WardenAuditCommand.php`:
- Around line 140-162: The runSequentialAudits method is ignoring the
$isMachineOutput flag, so human-oriented messages still print; wrap the
user-facing $this->info(...) calls (at least the "Using cached results for %s
audit..." and "Running %s audit..." messages inside runSequentialAudits) in a
conditional that checks !$isMachineOutput before emitting them, ensuring cached
merging and audit logic (cacheService->hasRecentAudit,
cacheService->getCachedResult, array_merge into $allFindings) remain unchanged
while suppressing human output when $isMachineOutput is true.
- Around line 716-727: Escape workflow command fields and message content before
writing annotations: when iterating over $findings in the foreach ($findings as
$finding) block, percent-encode property fields used in the command key=value
pairs (escape %→%25, \r→%0D, \n→%0A, :→%3A, ,→%2C) for $title and any other
property values, and percent-encode message content (escape %→%25, \r→%0D,
\n→%0A) for the message part that includes $package and $finding['severity'];
update the $this->output->writeln call to use these encoded values (replace raw
$title, $package, $finding['severity'] with their encoded counterparts) so the
GitHub Actions ::command key=value::message format is never corrupted.
In `@src/Providers/WardenServiceProvider.php`:
- Line 58: In WardenServiceProvider update the scheduled command string: replace
the deprecated flag `--silent` with the current `--no-notify` so the
`$schedule->command('warden:audit --silent')` call becomes
`$schedule->command('warden:audit --no-notify')`; locate this in the schedule
registration (WardenServiceProvider, the line creating $event) and modify the
command argument accordingly to match the warden:audit signature.
---
Nitpick comments:
In @.github/workflows/tests.yml:
- Around line 30-31: Add a Composer cache step before the "Install dependencies"
step to speed CI: create a cache using actions/cache keyed by composer.lock (and
optionally matrix PHP version) to store ~/.composer/cache (or vendor) so
subsequent runs restore dependencies, then run "composer install
--no-interaction --no-progress" as before; update the workflow to add the cache
step immediately before the step named "Install dependencies" and reference
composer.lock in the cache key to invalidate when dependencies change.
In `@src/Providers/WardenServiceProvider.php`:
- Around line 24-26: The closure passed to $this->app->bind for AuditExecutor
includes an unused $app parameter; remove the unused parameter so the binding
becomes a no-argument closure that simply returns new AuditExecutor() (update
the binding that references AuditExecutor in WardenServiceProvider to use
function() instead of function($app)). Ensure there are no references to $app
inside the closure before removing it.
In `@src/Services/AuditCacheService.php`:
- Around line 75-95: The AUDIT_NAMES constant is hardcoded and misplaced; move
the private const AUDIT_NAMES = [...] declaration to the top of the class (above
properties/constructor) and either replace it with a dynamic discovery of audit
names from the registered audit services or add a clear inline comment on
AUDIT_NAMES stating it must be kept in sync with registered audits; then update
clearCache(?string $auditName) to iterate discovered names (or AUDIT_NAMES) when
$auditName is null and continue using $this->getCacheKey($name) and
Cache::forget as before.
In `@src/Services/AuditExecutor.php`:
- Around line 13-16: The addAudit method in AuditExecutor currently type-hints a
generic object but then calls getName(), run(), and getFindings(); change the
parameter to the specific contract (e.g., AbstractAuditService or
AuditServiceInterface) so callers get compile-time checks and IDE hints. Update
the method signature of addAudit(object $auditService): void to use the chosen
type (e.g., addAudit(AbstractAuditService $auditService): void), and adjust any
related property typing for $audits to reflect the new type (and add the
appropriate use/import for AbstractAuditService or the interface).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/phpstan.yml.github/workflows/tests.ymlAGENTS.mdcomposer.jsonphpunit.xmlsrc/Commands/WardenAuditCommand.phpsrc/Commands/WardenScheduleCommand.phpsrc/Commands/WardenSyntaxCommand.phpsrc/Notifications/Channels/EmailChannel.phpsrc/Notifications/Channels/TeamsChannel.phpsrc/Notifications/Concerns/HasSeverityHelpers.phpsrc/Providers/WardenServiceProvider.phpsrc/Services/AuditCacheService.phpsrc/Services/AuditExecutor.phpsrc/Services/Audits/ComposerAuditService.phpsrc/Services/Audits/DebugModeAuditService.phpsrc/Services/CustomAuditWrapper.phpsrc/Services/OutputFormatters/JsonFormatter.phpsrc/Services/ParallelAuditExecutor.phpsrc/config/warden.phpsrc/views/mail/enhanced-report.blade.phptestbench.yamltests/Commands/WardenAuditCommandTest.phptests/Commands/WardenSyntaxCommandTest.php
💤 Files with no reviewable changes (5)
- src/Services/Audits/ComposerAuditService.php
- src/config/warden.php
- src/Services/OutputFormatters/JsonFormatter.php
- src/Services/ParallelAuditExecutor.php
- .github/workflows/phpstan.yml
…2+ test target Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
…ctor) Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
…ustomAuditWrapper findings Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
…comment Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
…, suppress info when machine output, escape GitHub Actions annotations Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
…ameParamToMatchType, NullableCompareToNull) Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
genericmilk
approved these changes
Feb 27, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/tests.yml (1)
9-11: Declare explicit least-privilegeGITHUB_TOKENpermissions.Without an explicit
permissionsblock, the effective scope depends on your repository's workflow permissions setting. GitHub recommends declaring permissions explicitly at the job level, with each job granted only what it needs.Proposed change
jobs: tests: + permissions: + contents: read runs-on: ubuntu-latestFor read-only CI workflows,
contents: readis the appropriate minimal permission.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 9 - 11, The tests job currently omits explicit GITHUB_TOKEN permissions; update the "tests" job definition to include a minimal permissions block granting read-only access (e.g., set permissions: contents: read) so the job runs with least privilege—add the permissions stanza under the tests job (refer to the "tests" job key in the workflow) and ensure it's applied at job scope rather than relying on repository-level defaults.src/Services/AuditExecutor.php (1)
14-17: Prevent silent overwrite of audits with duplicate names.At Line 16, registering by name without a guard can replace an existing audit invisibly. Fail fast here to avoid missing audits.
♻️ Proposed refactor
public function addAudit(AuditServiceInterface $auditService): void { - $this->audits[$auditService->getName()] = $auditService; + $name = $auditService->getName(); + if (isset($this->audits[$name])) { + throw new \LogicException(sprintf('Audit "%s" is already registered.', $name)); + } + $this->audits[$name] = $auditService; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/AuditExecutor.php` around lines 14 - 17, The addAudit method in AuditExecutor currently overwrites existing entries in the audits array; modify addAudit(AuditServiceInterface $auditService) to first check if an entry for $auditService->getName() already exists (using isset or array_key_exists on $this->audits) and fail fast by throwing a specific exception (e.g., InvalidArgumentException or a custom DuplicateAuditException) that includes the duplicate name instead of silently replacing it; keep the rest of the method unchanged so new audits are only added when the name is unique.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Line 21: Replace mutable action version tags with full commit SHAs for each
uses entry in the workflow (e.g., replace actions/checkout@v4,
actions/setup-node@v2, actions/cache@v3) so the workflow points to immutable
commits; locate the uses lines in .github/workflows/tests.yml, look up the
corresponding full-length commit SHA from the official action repositories (not
forks), and update each uses: reference to the verified SHA (keeping a comment
with the human-friendly version tag optional).
In `@src/Commands/WardenAuditCommand.php`:
- Around line 317-322: The failure labelling in handleAuditFailure currently
uses instanceof checks against
\Dgtlss\Warden\Services\Audits\AbstractAuditService and CustomAuditWrapper which
can mislabel valid AuditServiceInterface implementations; instead call
$auditService->getName() directly to derive the service name (fall back to
'Unknown service' only if getName() returns empty/null). Update
handleAuditFailure to use the AuditServiceInterface->getName() result and remove
the concrete-class instanceof checks (references: handleAuditFailure,
AuditServiceInterface::getName(), CustomAuditWrapper, AbstractAuditService).
- Around line 60-66: The call to $this->output->isSilent() in
shouldSuppressNotifications() can be fatal on older Symfony Console versions
(used by Laravel 7–10); wrap that call with a runtime guard and provide a safe
fallback: update shouldSuppressNotifications() to check
method_exists($this->output, 'isSilent') and if present return its value,
otherwise fall back to a compatible check such as returning !
$this->output->isVerbose() (or another existing output method) so older
frameworks won’t crash.
---
Nitpick comments:
In @.github/workflows/tests.yml:
- Around line 9-11: The tests job currently omits explicit GITHUB_TOKEN
permissions; update the "tests" job definition to include a minimal permissions
block granting read-only access (e.g., set permissions: contents: read) so the
job runs with least privilege—add the permissions stanza under the tests job
(refer to the "tests" job key in the workflow) and ensure it's applied at job
scope rather than relying on repository-level defaults.
In `@src/Services/AuditExecutor.php`:
- Around line 14-17: The addAudit method in AuditExecutor currently overwrites
existing entries in the audits array; modify addAudit(AuditServiceInterface
$auditService) to first check if an entry for $auditService->getName() already
exists (using isset or array_key_exists on $this->audits) and fail fast by
throwing a specific exception (e.g., InvalidArgumentException or a custom
DuplicateAuditException) that includes the duplicate name instead of silently
replacing it; keep the rest of the method unchanged so new audits are only added
when the name is unique.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/tests.ymlAGENTS.mdsrc/Commands/WardenAuditCommand.phpsrc/Contracts/AuditServiceInterface.phpsrc/Notifications/Channels/EmailChannel.phpsrc/Notifications/Channels/TeamsChannel.phpsrc/Providers/WardenServiceProvider.phpsrc/Services/AuditCacheService.phpsrc/Services/AuditExecutor.phpsrc/Services/Audits/AbstractAuditService.phpsrc/Services/CustomAuditWrapper.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Providers/WardenServiceProvider.php
- AGENTS.md
- src/Services/AuditCacheService.php
…notify - Add Command Reference table for quick lookup of all commands/options - Document --no-notify (preferred over --silent), note --silent backward compat - Remove deprecated config: WARDEN_OUTPUT_JSON, WARDEN_OUTPUT_JUNIT, WARDEN_SEVERITY_FILTER - Remove retry_attempts, retry_delay, severity_filter from config examples - Add PHP syntax audit env config (WARDEN_PHP_SYNTAX_AUDIT_ENABLED) - Update CI examples: PHP 8.4, --no-notify in GitLab - Add Troubleshooting to TOC - Add source field to custom audit getFindings example Co-authored-by: Nathan Langer <dgtlss@users.noreply.github.com>
…ure-ebf8 Readme documentation structure
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@readme.md`:
- Around line 519-520: Update the directional cross-reference for the CLI
options line containing `--output` and `--severity` that currently reads "See
Command Reference below": change "below" to "above" (or remove the directional
word entirely) so the reference to the "Command Reference" section is accurate
relative to its current position; locate the line with the text "**Output &
severity:** Use `--output` and `--severity` CLI options (not config). See
[Command Reference](`#-command-reference`)" and apply the wording change.
- Line 39: Update the broken TOC fragment links by changing the invalid anchors
`[Configuration](`#-configuration`)` and `[Troubleshooting](`#-troubleshooting`)` to
match the actual heading IDs or by adding explicit anchors; either replace them
with `[Configuration](`#configuration`)` and `[Troubleshooting](`#troubleshooting`)`
to match standard slugified headings, or insert explicit HTML anchors (e.g., `<a
name="configuration"></a>`) immediately before the corresponding headings so the
links resolve correctly.
Add Snyk security guidance and editor settings, update ignore list and README formatting, and make audit command more robust. - Added .cursor/rules/snyk_rules.mdc with Snyk code-scan guidance for new code. - Added .vscode/settings.json to enable Snyk auto-select organization. - Updated .gitignore to include Snyk rules and coverage-html directory. - Fixed README table-of-contents list formatting and a minor wording change. - Updated src/Commands/WardenAuditCommand.php to: detect and use output->isSilent() when available (Symfony Console 7.2+/Laravel 11+), fall back to using !isVerbose() for older versions, and handle empty/null audit service names by falling back to "Unknown service". These changes improve compatibility and error messaging.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Commands/WardenAuditCommand.php (1)
60-73:⚠️ Potential issue | 🟠 MajorFallback logic inverts expected notification behaviour.
On Symfony Console < 7.2 (Laravel 7–10), the fallback
return !$this->output->isVerbose()suppresses notifications when output is at normal verbosity (the common case), but allows them only when verbose flags are passed. This is inverted—notifications should be sent by default unless explicitly suppressed.Since there's no
--silentdetection available on older versions, the fallback should returnfalseto allow notifications:🐛 Proposed fix
// Check if isSilent() exists (Symfony Console 7.2+/Laravel 11+) if (method_exists($this->output, 'isSilent')) { return $this->output->isSilent(); } - // Fallback for older Symfony Console versions - return !$this->output->isVerbose(); + // Fallback for older Symfony Console versions: allow notifications by default + // (no --silent detection available, user must use --no-notify explicitly) + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Commands/WardenAuditCommand.php` around lines 60 - 73, The fallback logic in shouldSuppressNotifications() is inverted: locate the shouldSuppressNotifications() method and change the fallback branch (the case where method_exists($this->output, 'isSilent') returns false) to return false instead of returning !$this->output->isVerbose(); keep the early check for $this->option('no-notify') and the method_exists($this->output, 'isSilent') path that returns $this->output->isSilent(), but ensure older Symfony Console versions simply return false so notifications are not suppressed by default.
🧹 Nitpick comments (2)
src/Commands/WardenAuditCommand.php (1)
324-330: Remove dead code:getName()returnsstring, nevernull.Per the
AuditServiceInterfacecontract,getName()returnsstring. The=== nullcheck is dead code flagged by static analysis.♻️ Suggested simplification
protected function handleAuditFailure(AuditServiceInterface $auditService): void { $serviceName = $auditService->getName(); - if ($serviceName === '' || $serviceName === null) { + if ($serviceName === '') { $serviceName = 'Unknown service'; } $this->error($serviceName . ' audit failed to run.');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Commands/WardenAuditCommand.php` around lines 324 - 330, The method handleAuditFailure contains a redundant null check because AuditServiceInterface::getName() is typed to return string; remove the "=== null" branch and simplify the logic to only treat an empty string as unknown (e.g., if ($serviceName === '') $serviceName = 'Unknown service'), then call $this->error($serviceName . ' audit failed to run.'); ensure you only reference getName(), $serviceName, handleAuditFailure and $this->error when updating the code.readme.md (1)
38-48: TOC fragment links may not resolve correctly due to emoji prefixes in headings.The headings use emoji prefixes (e.g.
## 📌 Command Reference,## 🛠️ Troubleshooting), but the TOC anchors omit them. Most Markdown renderers generate anchors like#-command-reference(with a leading dash from the emoji). Additionally, line 519 uses#-command-referencewhilst the TOC uses#command-reference.Consider adding explicit HTML anchors for consistency:
📝 Suggested fix with explicit anchors
+ <a id="command-reference"></a> ## 📌 Command Reference+ <a id="troubleshooting"></a> ## 🛠️ TroubleshootingAnd update line 519 to match:
- See [Command Reference](`#-command-reference`) above. + See [Command Reference](`#command-reference`) above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@readme.md` around lines 38 - 48, The TOC links don't match the emoji-prefixed headings because markdown renderers generate emoji-derived anchors (e.g., headings like "## 📌 Command Reference" become anchors with a leading dash such as "#-command-reference"); add explicit HTML anchor tags immediately above each emoji-prefixed heading (for example add <a name="command-reference"></a> above the "📌 Command Reference" heading and similar anchors like "troubleshooting", "configuration", etc.) and update the TOC entries to point to those explicit names (e.g., use `#command-reference`) so anchors resolve consistently across renderers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Commands/WardenAuditCommand.php`:
- Around line 60-73: The fallback logic in shouldSuppressNotifications() is
inverted: locate the shouldSuppressNotifications() method and change the
fallback branch (the case where method_exists($this->output, 'isSilent') returns
false) to return false instead of returning !$this->output->isVerbose(); keep
the early check for $this->option('no-notify') and the
method_exists($this->output, 'isSilent') path that returns
$this->output->isSilent(), but ensure older Symfony Console versions simply
return false so notifications are not suppressed by default.
---
Nitpick comments:
In `@readme.md`:
- Around line 38-48: The TOC links don't match the emoji-prefixed headings
because markdown renderers generate emoji-derived anchors (e.g., headings like
"## 📌 Command Reference" become anchors with a leading dash such as
"#-command-reference"); add explicit HTML anchor tags immediately above each
emoji-prefixed heading (for example add <a name="command-reference"></a> above
the "📌 Command Reference" heading and similar anchors like "troubleshooting",
"configuration", etc.) and update the TOC entries to point to those explicit
names (e.g., use `#command-reference`) so anchors resolve consistently across
renderers.
In `@src/Commands/WardenAuditCommand.php`:
- Around line 324-330: The method handleAuditFailure contains a redundant null
check because AuditServiceInterface::getName() is typed to return string; remove
the "=== null" branch and simplify the logic to only treat an empty string as
unknown (e.g., if ($serviceName === '') $serviceName = 'Unknown service'), then
call $this->error($serviceName . ' audit failed to run.'); ensure you only
reference getName(), $serviceName, handleAuditFailure and $this->error when
updating the code.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.cursor/rules/snyk_rules.mdc.gitignore.vscode/settings.jsonreadme.mdsrc/Commands/WardenAuditCommand.php
✅ Files skipped from review due to trivial changes (1)
- .cursor/rules/snyk_rules.mdc
Replace the fallback that returned !$this->output->isVerbose() with a hardcoded false so notifications are not suppressed for older Symfony Console versions. Update the comment to clarify the intent.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes critical bugs, enhances CLI user experience, improves CI, and refactors code for the Warden package.
The
--silentflag was causing issues with Laravel 12's framework-level--silentoption, leading to completely suppressed output. The fix ensures backward compatibility while providing a new--no-notifyflag for clearer intent. Additionally, thewarden:auditcommand's progress spinner was polluting CI logs with ANSI escape codes; this has been replaced with clean, per-audit progress reporting suitable for both interactive terminals and CI environments.Summary by CodeRabbit
New Features
Improvements
Documentation
Chores
Tests