Implement agent management and job processing framework#132
Merged
David-Crty merged 11 commits intomainfrom Mar 10, 2026
Merged
Implement agent management and job processing framework#132David-Crty merged 11 commits intomainfrom
David-Crty merged 11 commits intomainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a distributed agent subsystem: models, migrations, agent API + middleware, CLI agent runner and lease recovery, agent HTTP client and payload builder, routing backups to agents, Livewire CRUD/UI for agents, DTO payload (de)serialization, config/bootstrap changes, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Remote Agent
participant App as App Server
participant DB as Database
participant Queue as Job Queue
rect rgba(100,150,200,0.5)
note over Agent,App: Agent polling, claim, execute, and reporting
Agent->>App: POST /api/v1/agent/heartbeat
App->>DB: update agents.last_heartbeat_at
App-->>Agent: 200 OK
Agent->>App: POST /api/v1/agent/jobs/claim
App->>DB: SELECT pending/expired AgentJob FOR UPDATE
alt job found
App->>DB: set AgentJob.status=claimed, lease_expires_at, claimed_at, attempts++
App-->>Agent: job payload (id, snapshot_id, payload, attempts, max_attempts)
Agent->>Agent: execute backup/discovery using payload
Agent->>App: POST /api/v1/agent/jobs/{id}/heartbeat (logs)
App->>DB: extend lease, append logs
alt success
Agent->>App: POST /api/v1/agent/jobs/{id}/ack (filename,size,checksum,logs)
App->>DB: mark AgentJob completed, update snapshot/backup job status
App->>Queue: notify/process completion
else failure
Agent->>App: POST /api/v1/agent/jobs/{id}/fail (error_message,logs)
App->>DB: mark AgentJob failed, mark backup job failed, send failure notification
end
else no job
App-->>Agent: null
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
============================================
+ Coverage 90.93% 91.47% +0.53%
- Complexity 1552 1722 +170
============================================
Files 140 156 +16
Lines 5662 6367 +705
============================================
+ Hits 5149 5824 +675
- Misses 513 543 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (12)
app/Services/Backup/DTO/VolumeConfig.php (1)
27-29: Tighten the PHPDoc array shape to list the expected keys.The current
array<string, mixed>is generic. Since the expected structure is well-known (one required key, two optional), a named-key shape communicates the contract more precisely and matches the project guideline for useful array shape type definitions.✏️ Proposed PHPDoc improvement
- /** - * `@param` array<string, mixed> $payload - */ + /** + * `@param` array{type: string, name?: string, config?: array<string, mixed>} $payload + */As per coding guidelines, "Add useful array shape type definitions in PHPDoc when appropriate."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Backup/DTO/VolumeConfig.php` around lines 27 - 29, Update the PHPDoc for the parameter $payload in VolumeConfig to use a named array shape instead of the generic array<string, mixed>; replace the current docblock on the method/constructor that declares `@param` array<string, mixed> $payload with an array shape like array{requiredKey: string, optionalKey1?: string, optionalKey2?: array<string,mixed>} using the actual key names and types used by VolumeConfig so callers and static analysis know the exact contract.app/Console/Commands/AgentRunCommand.php (1)
52-56:sleep()receivesmixedfromconfig().
config('agent.poll_interval', 5)returnsmixed. While the config file casts to(int), theconfig()return type is stillmixed. Cast at the call site or at Line 26 to be explicit.Proposed fix
- $pollInterval = config('agent.poll_interval', 5); + $pollInterval = (int) config('agent.poll_interval', 5);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Console/Commands/AgentRunCommand.php` around lines 52 - 56, The config value used for sleeping is typed as mixed, so ensure $pollInterval is an int before passing to sleep: cast the result of config('agent.poll_interval', 5) to (int) where $pollInterval is assigned (or cast at each sleep(...) call); update the assignment in AgentRunCommand (the variable referenced by sleep($pollInterval) inside the try/catch) to explicitly (int)$pollInterval so sleep() always receives an integer.app/Services/Agent/AgentApiClient.php (1)
46-54: Ifack()fails after a successful backup, the job is incorrectly reported as failed (or lost).In
AgentRunCommand::executeJob, a successful$backupTask->execute()followed by a network failure onack()falls into the catch block, which callsfail(). This means a completed backup gets marked as failed on the server. Consider adding a simple retry forack()before falling through to failure reporting, since it's the only call where the local side-effect (backup file) has already been committed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Agent/AgentApiClient.php` around lines 46 - 54, The ack() call currently posts once then ->throw(), so a transient network error after a successful backup causes AgentRunCommand::executeJob to treat the job as failed; modify AgentApiClient::ack to perform a small definite retry loop (e.g., 2–3 attempts with a short backoff) around the $this->post("/agent/jobs/{$jobId}/ack", ...)->throw() call and only propagate the exception after retries are exhausted; this ensures AgentRunCommand::executeJob sees the ack transiently retried before its catch path (fail()) is triggered.resources/views/livewire/database-server/_form.blade.php (1)
34-35: Consider moving$agentOptionsassignment to the Livewire component.Line 34 uses a
@phpblock to invoke$form->getAgentOptions()and assign the result to a local template variable. While consistent with pre-existing@phpblocks in this file, the guideline asks for PHP logic to live in Livewire component classes. A computed property or a direct$form->agentOptionspublic getter exposed to the template would keep the blade purely declarative.♻️ Suggested approach
In the Livewire form class (
DatabaseServerForm), expose the computed result directly:// app/Livewire/Forms/DatabaseServerForm.php public function getAgentOptionsProperty(): array { return $this->getAgentOptions(); }Then in the blade, remove the
@phpblock and reference it directly:- `@php` $agentOptions = $form->getAgentOptions(); `@endphp` - `@if`(count($agentOptions) > 0 || !empty($form->agent_id)) + `@if`(count($form->getAgentOptions()) > 0 || !empty($form->agent_id)) <x-select wire:model="form.agent_id" :label="__('Remote Agent')" - :options="$agentOptions" + :options="$form->getAgentOptions()"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/database-server/_form.blade.php` around lines 34 - 35, Move the template-side call to getAgentOptions into the Livewire component by adding a public computed property on DatabaseServerForm (e.g., getAgentOptionsProperty that returns $this->getAgentOptions()), then remove the `@php` block in the blade and reference the property as $form->agentOptions (and keep the existing conditional using count($form->agentOptions) or !empty($form->agent_id)); this keeps the Blade declarative and centralizes logic in DatabaseServerForm.resources/views/livewire/agent/_form.blade.php (2)
1-1: DefaultsubmitLabelis not translation-ready.The default value
'Save'is a raw English string. If this partial is ever used without an explicitsubmitLabel, the button text won't pass through__(). Consider wrapping the default.-@props(['form', 'submitLabel' => 'Save', 'cancelRoute' => 'agents.index']) +@props(['form', 'submitLabel' => __('Save'), 'cancelRoute' => 'agents.index'])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/agent/_form.blade.php` at line 1, The default submitLabel in the Blade component props is a raw string ('Save') and should be made translation-ready; update the `@props` declaration so the default value for submitLabel uses the translation helper (e.g., __('Save')) so any usage that omits submitLabel will output a localized label; change the `@props` line that defines 'submitLabel' to use the translation helper.
17-17: Prefer:linkdynamic binding over{{ }}interpolation.Consistent with the same pattern flagged in the index view.
- <x-button class="btn-ghost w-full sm:w-auto" link="{{ route($cancelRoute) }}" wire:navigate> + <x-button class="btn-ghost w-full sm:w-auto" :link="route($cancelRoute)" wire:navigate>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/agent/_form.blade.php` at line 17, The x-button's link attribute uses Blade interpolation ({{ $cancelRoute }}) instead of a dynamic binding; change the attribute to use the Blade component dynamic binding syntax :link="$cancelRoute" on the <x-button> (the element with class "btn-ghost w-full sm:w-auto") so it receives the route value as a bound prop rather than a string interpolation, matching the pattern used elsewhere (e.g., index view).tests/Feature/Agent/AgentCrudTest.php (1)
112-120: Consider removing this test — it only validates a framework rule (required).This test asserts that an empty
form.nametriggers a validation error, which is testing a built-in Laravel validation rule (required). The coding guidelines recommend focusing on business logic, authorization, and edge cases rather than framework-provided validation.As per coding guidelines: "Do not test framework internals, form validation rules (required, max:255, etc.)… Focus on business logic, authorization, edge cases in your code, and integration points."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/Agent/AgentCrudTest.php` around lines 112 - 120, Remove the redundant framework validation test named "name is required" in AgentCrudTest.php: delete the Livewire test block that acts as a user, mounts Create::class, sets 'form.name' to '' and calls 'save' asserting errors on ['form.name']; this test only verifies Laravel's built-in "required" rule and should be removed to focus on business logic, authorization, and edge cases instead.app/Services/Backup/TriggerBackupAction.php (1)
74-83: Consider wrappingdispatchToAgentin a transaction.If
payloadBuilder->build()throws mid-loop, someAgentJobrecords will be persisted while later snapshots won't get agent jobs. This leaves the backup in a partially-dispatched state that would be hard to recover from. Wrapping in aDB::transactionwould make the operation atomic.Proposed fix
+use Illuminate\Support\Facades\DB; + private function dispatchToAgent(array $snapshots): void { - foreach ($snapshots as $snapshot) { - AgentJob::create([ - 'snapshot_id' => $snapshot->id, - 'status' => AgentJob::STATUS_PENDING, - 'payload' => $this->payloadBuilder->build($snapshot), - ]); - } + DB::transaction(function () use ($snapshots) { + foreach ($snapshots as $snapshot) { + AgentJob::create([ + 'snapshot_id' => $snapshot->id, + 'status' => AgentJob::STATUS_PENDING, + 'payload' => $this->payloadBuilder->build($snapshot), + ]); + } + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Backup/TriggerBackupAction.php` around lines 74 - 83, Wrap the whole dispatchToAgent loop in a DB transaction so AgentJob::create calls are atomic: begin a DB::transaction around the loop in dispatchToAgent(array $snapshots) and move payloadBuilder->build($snapshot) and AgentJob::create(...) inside the transaction closure; if payloadBuilder->build() throws or any create fails the transaction will roll back preventing partial persistence and leaving the backup in a consistent state. Ensure you import/use the DB facade or transaction helper used in the project and keep the same behavior for successful commits.resources/views/livewire/agent/index.blade.php (1)
24-24: Prefer:linkdynamic binding over{{ }}interpolation for route attributes.While the coding guideline specifically targets translated strings, using the dynamic binding form is more consistent and avoids subtle encoding issues.
- <x-button :label="__('Add Agent')" link="{{ route('agents.create') }}" icon="o-plus" class="btn-primary btn-sm" wire:navigate /> + <x-button :label="__('Add Agent')" :link="route('agents.create')" icon="o-plus" class="btn-primary btn-sm" wire:navigate />The same applies to line 100:
- link="{{ route('agents.edit', $agent) }}" + :link="route('agents.edit', $agent)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/agent/index.blade.php` at line 24, Replace the interpolated route attribute on the x-button component with a dynamic binding to avoid encoding issues: change the prop usage from link="{{ route('agents.create') }}" to use the bound form :link="route('agents.create')" (and make the same change for the similar x-button at line 100) so the component receives the route as a raw value instead of an HTML-escaped string; locate the x-button instances in the Blade view (the Add Agent button and the other button at line 100) to apply this fix.app/Livewire/Agent/Edit.php (1)
57-63: Token revocation interrupts any currently in-progress agent job.
tokens()->delete()takes effect immediately. If the agent has an activeCLAIMEDorRUNNINGAgentJobwhen a token is regenerated, all subsequent heartbeat, ack, and fail calls from that agent will be rejected with403byEnsureAgentToken. The job will remain stuck untilagent:recover-leasesresets or fails it. Consider documenting this in the UI (e.g., a warning in the token-modal) so operators know that regenerating a token during an active job will require a lease recovery cycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Livewire/Agent/Edit.php` around lines 57 - 63, Regenerating tokens via tokens()->delete() immediately invalidates agent credentials and will cause active AgentJob states (CLAIMED or RUNNING) to fail heartbeats/acks with 403 (handled by EnsureAgentToken) and require running agent:recover-leases to recover; update the token modal UI (where newToken and showTokenModal are set after createToken('agent')) to display a clear warning that regenerating the token will interrupt any in-progress jobs and may require running agent:recover-leases to recover stuck jobs, and include the relevant AgentJob statuses (CLAIMED, RUNNING) and guidance for operators.app/Console/Commands/RunScheduledBackups.php (1)
78-82: Minor: snapshot relationships may not be eager-loaded for payload builder.
$payloadBuilder->build($snapshot)accesses$snapshot->databaseServerand$snapshot->volume(seeAgentJobPayloadBuilder.phplines 19-20), which may trigger lazy-loaded queries per snapshot. This is low-impact in a console command, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Console/Commands/RunScheduledBackups.php` around lines 78 - 82, The payload builder accesses related models (AgentJobPayloadBuilder::build reads $snapshot->databaseServer and $snapshot->volume) which may trigger N+1 lazy queries; ensure the Snapshot instances are eager-loaded with those relations before calling $payloadBuilder->build($snapshot) (for example, load or query with(['databaseServer','volume']) on the Snapshot model) so AgentJob creation uses snapshots with databaseServer and volume already loaded.database/factories/AgentJobFactory.php (1)
23-23: PreferAgentJob::STATUS_*constants over string literals for status values.The factory uses raw strings (
'pending','claimed','running', etc.) instead of the constants defined onAgentJob. Using constants ensures the factory stays in sync if values ever change.♻️ Example changes
+use App\Models\AgentJob; + // In definition(): - 'status' => 'pending', + 'status' => AgentJob::STATUS_PENDING, // In claimed(): - 'status' => 'claimed', + 'status' => AgentJob::STATUS_CLAIMED, // In running(): - 'status' => 'running', + 'status' => AgentJob::STATUS_RUNNING, // In completed(): - 'status' => 'completed', + 'status' => AgentJob::STATUS_COMPLETED, // In failed(): - 'status' => 'failed', + 'status' => AgentJob::STATUS_FAILED, // In expiredLease(): - 'status' => 'claimed', + 'status' => AgentJob::STATUS_CLAIMED,Also applies to: 56-56, 68-69, 84-84, 98-98, 113-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/factories/AgentJobFactory.php` at line 23, The factory currently uses raw status strings (e.g., 'pending', 'claimed', 'running', etc.) in AgentJobFactory; replace these literals with the corresponding AgentJob::STATUS_* constants (for example AgentJob::STATUS_PENDING, AgentJob::STATUS_CLAIMED, AgentJob::STATUS_RUNNING, etc.) wherever the factory sets 'status' (including the state definitions currently using those string values) so the factory stays in sync with AgentJob's canonical status constants.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
app/Console/Commands/AgentRunCommand.phpapp/Console/Commands/RecoverAgentLeasesCommand.phpapp/Console/Commands/RunScheduledBackups.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Http/Middleware/EnsureAgentToken.phpapp/Livewire/Agent/Create.phpapp/Livewire/Agent/Edit.phpapp/Livewire/Agent/Index.phpapp/Livewire/ApiToken/Index.phpapp/Livewire/Forms/AgentForm.phpapp/Livewire/Forms/DatabaseServerForm.phpapp/Models/Agent.phpapp/Models/AgentJob.phpapp/Models/DatabaseServer.phpapp/Policies/AgentPolicy.phpapp/Queries/AgentQuery.phpapp/Services/Agent/AgentApiClient.phpapp/Services/Agent/AgentJobPayloadBuilder.phpapp/Services/Backup/DTO/DatabaseConnectionConfig.phpapp/Services/Backup/DTO/VolumeConfig.phpapp/Services/Backup/TriggerBackupAction.phpbootstrap/app.phpconfig/agent.phpdatabase/factories/AgentFactory.phpdatabase/factories/AgentJobFactory.phpdatabase/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.phpresources/views/layouts/app.blade.phpresources/views/livewire/agent/_form.blade.phpresources/views/livewire/agent/_token-modal.blade.phpresources/views/livewire/agent/create.blade.phpresources/views/livewire/agent/edit.blade.phpresources/views/livewire/agent/index.blade.phpresources/views/livewire/database-server/_form.blade.phproutes/api.phproutes/console.phproutes/web.phptests/Feature/Agent/AgentApiClientTest.phptests/Feature/Agent/AgentApiTest.phptests/Feature/Agent/AgentCrudTest.phptests/Feature/Agent/RecoverAgentLeasesTest.phptests/Feature/Agent/TriggerBackupWithAgentTest.php
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Console/Commands/AgentRunCommand.php`:
- Around line 85-113: The created $workingDirectory in the AgentRunCommand
try/catch block is never removed causing disk leaks; add a finally block after
the existing try/catch that checks for existence of $workingDirectory and calls
a new private helper removeDirectory(string $directory): void to recursively
delete files and the directory, ensuring any exceptions during removal are
caught/logged but do not mask the original exception; implement removeDirectory
using RecursiveDirectoryIterator/RecursiveIteratorIterator to unlink files and
rmdir directories, and reference $workingDirectory and removeDirectory in the
finally block inside the same class (AgentRunCommand).
In `@app/Console/Commands/RecoverAgentLeasesCommand.php`:
- Around line 17-20: The query that builds $expiredJobs in
RecoverAgentLeasesCommand is causing an N+1 when later accessing each job's
snapshot and its job; update the initial Eloquent query to eager-load the
relations (e.g. use with(['snapshot.job']) or the actual relation names on
AgentJob) so snapshot and snapshot->job are fetched in the same query, keeping
the existing whereIn/where conditions and leaving the rest of the loop logic
unchanged.
- Around line 44-48: The code calls $job->snapshot->job->markFailed(...) which
can throw a TypeError when $job->snapshot or $job->snapshot->job is null; update
the loop in RecoverAgentLeasesCommand to null-check $job->snapshot and
$job->snapshot->job before calling markFailed (e.g., if snapshot or its job is
null, skip markFailed, increment $failedCount only when you actually mark a job
failed or handle the nil case by logging and counting), and ensure the loop
continues for remaining jobs so the command exits with an accurate status;
reference the $job->snapshot and markFailed(...) call to locate and protect the
vulnerable call site.
In `@app/Http/Controllers/Api/V1/AgentController.php`:
- Around line 175-206: The fail() endpoint performs multiple related updates
that must be atomic: wrap the calls in a DB::transaction() so
AgentJob->markFailed($message), merging/updating backup job logs
(backupJob->update(...)) and backupJob->markFailed(new RuntimeException(...))
execute together or roll back on error; ensure you import/use
Illuminate\Support\Facades\DB, move the snapshot/backup job logic inside the
transaction closure, and keep validation and permission checks outside so only
the state mutations are transactional.
- Around line 127-168: The ack() method performs multiple related DB writes
(updating snapshot, calling snapshot->markCompleted(), updating backup job logs,
and calling agentJob->markCompleted() and backupJob->markCompleted()) and must
be wrapped in a DB::transaction to ensure atomicity; modify ack() to run the
sequence of operations (snapshot update, snapshot->markCompleted(...),
conditional backupJob->update([...]) merge, and the two markCompleted() calls)
inside a DB::transaction(...) closure so that any exception rolls back all
changes (follow the same pattern used in claimJob), and rethrow or return an
error response on failure.
In `@app/Models/AgentJob.php`:
- Around line 105-113: The claim() method updates attempts but never enforces
max_attempts, allowing perpetual reclaims; update the logic so jobs are only
claimed when attempts < max_attempts (or max_attempts is null/unlimited). Either
adjust the AgentController::claimJob query to include a condition like
max_attempts IS NULL OR attempts < max_attempts when selecting candidate jobs,
or add a guard inside AgentJob::claim() that checks $this->attempts <
$this->max_attempts (or null) before performing the update and throw/return
false if the job exhausted attempts; ensure lease_expires_at, STATUS_CLAIMED and
attempts are only modified when the guard passes.
In `@app/Queries/AgentQuery.php`:
- Around line 25-27: The ternary uses in_array($sortBy['column'] ??
'created_at', self::ALLOWED_SORT_COLUMNS, true) but then accesses
$sortBy['column'] directly, causing an undefined key warning; fix by resolving
the column into a local variable first (e.g. $requested = $sortBy['column'] ??
'created_at') and then compute $column = in_array($requested,
self::ALLOWED_SORT_COLUMNS, true) ? $requested : 'created_at' so $column never
reads an undefined $sortBy['column'] and orderBy receives a valid string.
In `@app/Services/Agent/AgentApiClient.php`:
- Around line 70-75: The post method can produce a double-slash if $this->url
ends with '/', so update the URL construction in the post method to trim any
trailing slashes from $this->url (use rtrim($this->url, '/')) before
concatenating with "/api/v1{$path}" to ensure a single slash; modify the
post(string $path, ...) implementation (referenced as post and the $this->url
property) to use the trimmed base URL when building the request URL.
In `@app/Services/Agent/AgentJobPayloadBuilder.php`:
- Around line 63-76: resolveBackupPath can attempt to access $server->backup
when backup is null, causing "attempt to read property on null"; change the
property access to use PHP's null-safe operator so the nullable HasOne is
guarded (e.g. obtain the path via the null-safe access on the
DatabaseServer->backup relationship and fall back to an empty string), keeping
the existing empty-string return and behavior in resolveBackupPath.
In `@app/Services/Backup/DTO/DatabaseConnectionConfig.php`:
- Around line 74-86: The fromPayload factory in DatabaseConnectionConfig
currently coerces a missing port to 0 which is invalid; update
DatabaseConnectionConfig::fromPayload to validate the port instead of defaulting
to 0: either throw a clear exception when 'port' is missing/zero or apply a
sensible default based on DatabaseType::from($dbConfig['type']) (e.g., map
mysql=>3306, pgsql=>5432, etc.) before casting, and ensure the resulting port is
a positive int when constructing the DTO (adjust the port assignment and add a
short validation branch in fromPayload).
In
`@database/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.php`:
- Line 27: The migration uses $table->json('payload') but AgentJob casts payload
as encrypted:array, which writes encrypted strings that MySQL will reject as
invalid JSON; update the migration to use a text-type column (e.g., text or
longText) instead of json for the payload column in the CreateAgentsAndAgentJobs
migration so encrypted ciphertext can be stored, then run/refresh migrations;
reference the AgentJob model's payload cast and the migration's payload column
definition when making the change.
In `@resources/views/livewire/agent/edit.blade.php`:
- Around line 29-31: Replace the interpolated attribute wire:confirm="{{
__('...') }}" with the dynamic binding :wire:confirm so the translated string is
not double-encoded; locate the button that has wire:click="regenerateToken" and
change its wire:confirm attribute to use the dynamic binding (e.g.,
:wire:confirm with the same __('...') translation call) so special characters
like apostrophes are preserved correctly.
---
Nitpick comments:
In `@app/Console/Commands/AgentRunCommand.php`:
- Around line 52-56: The config value used for sleeping is typed as mixed, so
ensure $pollInterval is an int before passing to sleep: cast the result of
config('agent.poll_interval', 5) to (int) where $pollInterval is assigned (or
cast at each sleep(...) call); update the assignment in AgentRunCommand (the
variable referenced by sleep($pollInterval) inside the try/catch) to explicitly
(int)$pollInterval so sleep() always receives an integer.
In `@app/Console/Commands/RunScheduledBackups.php`:
- Around line 78-82: The payload builder accesses related models
(AgentJobPayloadBuilder::build reads $snapshot->databaseServer and
$snapshot->volume) which may trigger N+1 lazy queries; ensure the Snapshot
instances are eager-loaded with those relations before calling
$payloadBuilder->build($snapshot) (for example, load or query
with(['databaseServer','volume']) on the Snapshot model) so AgentJob creation
uses snapshots with databaseServer and volume already loaded.
In `@app/Livewire/Agent/Edit.php`:
- Around line 57-63: Regenerating tokens via tokens()->delete() immediately
invalidates agent credentials and will cause active AgentJob states (CLAIMED or
RUNNING) to fail heartbeats/acks with 403 (handled by EnsureAgentToken) and
require running agent:recover-leases to recover; update the token modal UI
(where newToken and showTokenModal are set after createToken('agent')) to
display a clear warning that regenerating the token will interrupt any
in-progress jobs and may require running agent:recover-leases to recover stuck
jobs, and include the relevant AgentJob statuses (CLAIMED, RUNNING) and guidance
for operators.
In `@app/Services/Agent/AgentApiClient.php`:
- Around line 46-54: The ack() call currently posts once then ->throw(), so a
transient network error after a successful backup causes
AgentRunCommand::executeJob to treat the job as failed; modify
AgentApiClient::ack to perform a small definite retry loop (e.g., 2–3 attempts
with a short backoff) around the $this->post("/agent/jobs/{$jobId}/ack",
...)->throw() call and only propagate the exception after retries are exhausted;
this ensures AgentRunCommand::executeJob sees the ack transiently retried before
its catch path (fail()) is triggered.
In `@app/Services/Backup/DTO/VolumeConfig.php`:
- Around line 27-29: Update the PHPDoc for the parameter $payload in
VolumeConfig to use a named array shape instead of the generic array<string,
mixed>; replace the current docblock on the method/constructor that declares
`@param` array<string, mixed> $payload with an array shape like array{requiredKey:
string, optionalKey1?: string, optionalKey2?: array<string,mixed>} using the
actual key names and types used by VolumeConfig so callers and static analysis
know the exact contract.
In `@app/Services/Backup/TriggerBackupAction.php`:
- Around line 74-83: Wrap the whole dispatchToAgent loop in a DB transaction so
AgentJob::create calls are atomic: begin a DB::transaction around the loop in
dispatchToAgent(array $snapshots) and move payloadBuilder->build($snapshot) and
AgentJob::create(...) inside the transaction closure; if payloadBuilder->build()
throws or any create fails the transaction will roll back preventing partial
persistence and leaving the backup in a consistent state. Ensure you import/use
the DB facade or transaction helper used in the project and keep the same
behavior for successful commits.
In `@database/factories/AgentJobFactory.php`:
- Line 23: The factory currently uses raw status strings (e.g., 'pending',
'claimed', 'running', etc.) in AgentJobFactory; replace these literals with the
corresponding AgentJob::STATUS_* constants (for example
AgentJob::STATUS_PENDING, AgentJob::STATUS_CLAIMED, AgentJob::STATUS_RUNNING,
etc.) wherever the factory sets 'status' (including the state definitions
currently using those string values) so the factory stays in sync with
AgentJob's canonical status constants.
In `@resources/views/livewire/agent/_form.blade.php`:
- Line 1: The default submitLabel in the Blade component props is a raw string
('Save') and should be made translation-ready; update the `@props` declaration so
the default value for submitLabel uses the translation helper (e.g., __('Save'))
so any usage that omits submitLabel will output a localized label; change the
`@props` line that defines 'submitLabel' to use the translation helper.
- Line 17: The x-button's link attribute uses Blade interpolation ({{
$cancelRoute }}) instead of a dynamic binding; change the attribute to use the
Blade component dynamic binding syntax :link="$cancelRoute" on the <x-button>
(the element with class "btn-ghost w-full sm:w-auto") so it receives the route
value as a bound prop rather than a string interpolation, matching the pattern
used elsewhere (e.g., index view).
In `@resources/views/livewire/agent/index.blade.php`:
- Line 24: Replace the interpolated route attribute on the x-button component
with a dynamic binding to avoid encoding issues: change the prop usage from
link="{{ route('agents.create') }}" to use the bound form
:link="route('agents.create')" (and make the same change for the similar
x-button at line 100) so the component receives the route as a raw value instead
of an HTML-escaped string; locate the x-button instances in the Blade view (the
Add Agent button and the other button at line 100) to apply this fix.
In `@resources/views/livewire/database-server/_form.blade.php`:
- Around line 34-35: Move the template-side call to getAgentOptions into the
Livewire component by adding a public computed property on DatabaseServerForm
(e.g., getAgentOptionsProperty that returns $this->getAgentOptions()), then
remove the `@php` block in the blade and reference the property as
$form->agentOptions (and keep the existing conditional using
count($form->agentOptions) or !empty($form->agent_id)); this keeps the Blade
declarative and centralizes logic in DatabaseServerForm.
In `@tests/Feature/Agent/AgentCrudTest.php`:
- Around line 112-120: Remove the redundant framework validation test named
"name is required" in AgentCrudTest.php: delete the Livewire test block that
acts as a user, mounts Create::class, sets 'form.name' to '' and calls 'save'
asserting errors on ['form.name']; this test only verifies Laravel's built-in
"required" rule and should be removed to focus on business logic, authorization,
and edge cases instead.
database/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.php
Outdated
Show resolved
Hide resolved
David-Crty
commented
Feb 25, 2026
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
app/Services/Agent/AgentJobPayloadBuilder.php (1)
45-48: Capturenow()once for placeholder expansion.Using a single timestamp avoids rare cross-boundary inconsistencies (e.g., around midnight) and removes redundant calls.
♻️ Proposed tweak
- return str_replace( - ['{year}', '{month}', '{day}'], - [now()->format('Y'), now()->format('m'), now()->format('d')], - $path - ); + $now = now(); + + return str_replace( + ['{year}', '{month}', '{day}'], + [$now->format('Y'), $now->format('m'), $now->format('d')], + $path + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Agent/AgentJobPayloadBuilder.php` around lines 45 - 48, In AgentJobPayloadBuilder::build (the code returning str_replace for ['{year}','{month}','{day}']), capture now() once into a single variable (e.g., $now) and use $now->format('Y'), $now->format('m'), $now->format('d') for the replacement array so the same timestamp is used for all placeholders and redundant now() calls are removed.tests/Feature/Agent/AgentCrudTest.php (1)
12-20: AvoidassertViewHas('agents')in this feature test.This assertion checks component/view internals rather than domain behavior. Prefer observable output/state assertions only.
♻️ Minimal change
Livewire::actingAs($user) ->test(Index::class) - ->assertOk() - ->assertViewHas('agents'); + ->assertOk();As per coding guidelines: "Do not test framework internals ... Focus on business logic, authorization, edge cases in your code, and integration points."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/Agent/AgentCrudTest.php` around lines 12 - 20, Remove the assertion that inspects view internals and instead assert observable output/state: in the test body using Livewire::actingAs($user)->test(Index::class) drop ->assertViewHas('agents') and replace it with assertions that a real user would observe, e.g. assertSee or assertSeeText for agent attributes created by Agent::factory() (use a representative field like agent name or email) or assertSeeHtml/element counts; keep Index::class and the created Agent::factory()->count(3) but assert visible content (e.g., assertSee($agentName)) or other public state rather than assertViewHas('agents').app/Services/Backup/DTO/BackupConfig.php (1)
44-46: Use an array-shape PHPDoc for the payload contract.
fromPayload()sits on a cross-process boundary; a concrete array-shape helps static analysis catch payload drift earlier.♻️ Proposed PHPDoc refinement
- /** - * Reconstruct from an agent payload. - * - * `@param` array<string, mixed> $payload - */ + /** + * Reconstruct from an agent payload. + * + * `@param` array{ + * database: array{ + * type: string, + * host?: string, + * port?: int|string, + * username?: string, + * password?: string, + * extra_config?: array<string, mixed>|null, + * database_name: string + * }, + * volume: array{ + * type: string, + * name?: string, + * config?: array<string, mixed> + * }, + * server_name: string, + * backup_path?: string, + * compression?: array{type?: string|null, level?: int|null} + * } $payload + */As per coding guidelines: "Add useful array shape type definitions in PHPDoc when appropriate."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Backup/DTO/BackupConfig.php` around lines 44 - 46, Add a precise array-shape PHPDoc for the $payload parameter of BackupConfig::fromPayload to document the expected cross-process contract: replace the generic "array<string, mixed>" with an array shape like array{key1: string, key2: int, optionalKey?: bool, ...} that enumerates each required and optional payload field and its type (matching what BackupConfig::fromPayload reads/uses). Ensure the shape names/types reflect the actual keys used inside fromPayload and mark optional fields with a trailing ? so static analyzers can catch payload drift.app/Http/Controllers/Api/V1/AgentController.php (1)
67-68: Drop inline comments that only restate obvious code.These comments add noise in controller methods; removing them (or extracting helpers) better matches the project’s PHP style rule.
As per coding guidelines: "Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex."
Also applies to: 148-149, 156-157, 164-165, 196-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Api/V1/AgentController.php` around lines 67 - 68, Remove inline comments that merely restate the code (e.g. the comment before $job->snapshot->job->markRunning()) and the similar comments at the other noted locations (lines referenced). Instead, either delete those single-line comments or, if explanation is needed, move explanatory text into a PHPDoc block on the containing method or extract the logic into a well-named helper method so the intent is clear from the symbol name; ensure no redundant inline comments remain in AgentController methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Console/Commands/AgentRunCommand.php`:
- Line 24: Clamp the configured poll interval to a positive value before calling
sleep by validating the $pollInterval obtained from
config('agent.poll_interval', 5) (and the other occurrences around lines 50-54)
and ensuring it is at least 1 (or a safe minimum) — e.g., if $pollInterval <= 0
set $pollInterval = 1 — so the AgentRunCommand loop never calls sleep(0) or
sleep(negative) and cannot spin aggressively.
- Around line 77-80: Move all accesses to $payload (and derived vars like
$databaseName and $payload['server_name']) into the guarded try block so any
exception triggers the job failure path; specifically, take the lines that read
$payload = $job['payload'], $databaseName =
$payload['database']['database_name'] ?? '', and the $this->info("Processing
job...") call and place them inside the inner try that eventually calls
$this->client->fail(...) on error, and do the same for other payload reads in
the region handled by the inner try (the code around the existing try/catch that
spans the block containing lines referenced as 84-102) so that any payload
access error results in $this->client->fail(...) being executed in the catch.
In `@app/Console/Commands/RecoverAgentLeasesCommand.php`:
- Line 34: Remove the inline comment(s) inside the RecoverAgentLeasesCommand
method body (e.g., the "Reset to pending for retry" comment and the other
comment at the second noted location); edit the RecoverAgentLeasesCommand class
(specifically the handle method) to delete these non-essential comments so the
code relies on clear variable/method names rather than inline commentary.
In `@app/Http/Controllers/Api/V1/AgentController.php`:
- Around line 106-108: AgentController currently accepts an unconstrained 'logs'
array; create a reusable set of stricter rules (e.g., add a private static or
protected method like rulesForLogs() or a $logRules variable) and replace the
loose 'logs' => 'nullable|array' with something like 'logs' =>
'nullable|array|max:100' plus per-item rules 'logs.*.timestamp' =>
'required|date', 'logs.*.level' =>
'required|string|in:debug,info,warning,error,critical', 'logs.*.message' =>
'required|string|max:2000', 'logs.*.meta' => 'nullable|array' (adjust fields to
match your logger schema), then use that shared rule set in all three endpoints
that currently validate 'logs' so the same constraints are enforced
consistently.
In `@tests/Feature/Agent/AgentCrudTest.php`:
- Around line 119-124: Remove the redirect assertion from the Livewire test: in
the Livewire::actingAs($user)->test(Edit::class, ['agent' =>
$agent])->set('form.name', 'New Name')->call('save') chain, delete the
->assertRedirect(route('agents.index')) call so the test only performs the
action and relies on the subsequent assertion that the agent's name was changed;
keep the Edit::class invocation, the 'save' call, and the post-save
business-effect assertions intact.
---
Nitpick comments:
In `@app/Http/Controllers/Api/V1/AgentController.php`:
- Around line 67-68: Remove inline comments that merely restate the code (e.g.
the comment before $job->snapshot->job->markRunning()) and the similar comments
at the other noted locations (lines referenced). Instead, either delete those
single-line comments or, if explanation is needed, move explanatory text into a
PHPDoc block on the containing method or extract the logic into a well-named
helper method so the intent is clear from the symbol name; ensure no redundant
inline comments remain in AgentController methods.
In `@app/Services/Agent/AgentJobPayloadBuilder.php`:
- Around line 45-48: In AgentJobPayloadBuilder::build (the code returning
str_replace for ['{year}','{month}','{day}']), capture now() once into a single
variable (e.g., $now) and use $now->format('Y'), $now->format('m'),
$now->format('d') for the replacement array so the same timestamp is used for
all placeholders and redundant now() calls are removed.
In `@app/Services/Backup/DTO/BackupConfig.php`:
- Around line 44-46: Add a precise array-shape PHPDoc for the $payload parameter
of BackupConfig::fromPayload to document the expected cross-process contract:
replace the generic "array<string, mixed>" with an array shape like array{key1:
string, key2: int, optionalKey?: bool, ...} that enumerates each required and
optional payload field and its type (matching what BackupConfig::fromPayload
reads/uses). Ensure the shape names/types reflect the actual keys used inside
fromPayload and mark optional fields with a trailing ? so static analyzers can
catch payload drift.
In `@tests/Feature/Agent/AgentCrudTest.php`:
- Around line 12-20: Remove the assertion that inspects view internals and
instead assert observable output/state: in the test body using
Livewire::actingAs($user)->test(Index::class) drop ->assertViewHas('agents') and
replace it with assertions that a real user would observe, e.g. assertSee or
assertSeeText for agent attributes created by Agent::factory() (use a
representative field like agent name or email) or assertSeeHtml/element counts;
keep Index::class and the created Agent::factory()->count(3) but assert visible
content (e.g., assertSee($agentName)) or other public state rather than
assertViewHas('agents').
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/Console/Commands/AgentRunCommand.phpapp/Console/Commands/RecoverAgentLeasesCommand.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Livewire/Forms/DatabaseServerForm.phpapp/Models/DatabaseServer.phpapp/Queries/AgentQuery.phpapp/Services/Agent/AgentApiClient.phpapp/Services/Agent/AgentJobPayloadBuilder.phpapp/Services/Backup/DTO/BackupConfig.phpapp/Services/Backup/DTO/DatabaseConnectionConfig.phpapp/Services/Backup/DTO/VolumeConfig.phpdatabase/factories/AgentJobFactory.phpdatabase/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.phpresources/views/livewire/agent/_form.blade.phpresources/views/livewire/agent/edit.blade.phpresources/views/livewire/agent/index.blade.phpresources/views/livewire/database-server/_form.blade.phptests/Feature/Agent/AgentCrudTest.php
🚧 Files skipped from review as they are similar to previous changes (9)
- resources/views/livewire/agent/index.blade.php
- app/Services/Backup/DTO/DatabaseConnectionConfig.php
- app/Livewire/Forms/DatabaseServerForm.php
- app/Services/Backup/DTO/VolumeConfig.php
- app/Services/Agent/AgentApiClient.php
- app/Queries/AgentQuery.php
- app/Models/DatabaseServer.php
- database/factories/AgentJobFactory.php
- resources/views/livewire/database-server/_form.blade.php
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
app/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/**/*.php: Use constructor property promotion in PHP 8 __construct() methods. Do not allow empty __construct() methods with zero parameters unless the constructor is private.
Always use explicit return type declarations for methods and functions. Use appropriate PHP type hints for method parameters.
Always use curly braces for control structures, even for single-line bodies.
Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex. Add useful array shape type definitions in PHPDoc when appropriate.
Avoid DB::; prefer Model::query(). Generate code that leverages Laravel's ORM capabilities rather than bypassing them. Generate code that prevents N+1 query problems by using eager loading.
Use Laravel's built-in authentication and authorization features (gates, policies, Sanctum, etc.).
Use environment variables only in configuration files - never use the env() function directly outside of config files. Always use config('key'), not env('KEY').
Use Laravel's query builder for very complex database operations instead of raw SQL queries.
Run 'vendor/bin/pint --dirty' before finalizing changes to ensure code matches the project's expected style. Use 'pint' to fix issues, not 'pint --test'.
Files:
app/Services/Backup/DTO/BackupConfig.phpapp/Console/Commands/AgentRunCommand.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Console/Commands/RecoverAgentLeasesCommand.phpapp/Services/Agent/AgentJobPayloadBuilder.php
app/Console/Commands/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Console commands in app/Console/Commands/ are automatically available and do not require manual registration.
Files:
app/Console/Commands/AgentRunCommand.phpapp/Console/Commands/RecoverAgentLeasesCommand.php
resources/views/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
resources/views/**/*.blade.php: All UI components use Mary UI (built on daisyUI) without prefixes (e.g., , , ). Use Heroicons for icons (e.g., icon="o-user" for outline, icon="s-user" for solid).
Modal pattern in Mary UI: Add boolean property to component class and use wire:model in blade file.
Select component pattern in Mary UI: Use :options prop with array format [['id' => 'value', 'name' => 'Label']].
Alert component pattern in Mary UI: Use class="alert-success", class="alert-error", etc. format.
In Blade component attributes, use :attr binding (dynamic syntax) instead of {{ }} interpolation when passing translated strings. The {{ }} syntax double-encodes special characters.
When generating links to other pages, prefer named routes and the route() function.
Files:
resources/views/livewire/agent/_form.blade.phpresources/views/livewire/agent/edit.blade.php
resources/views/livewire/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
Blade files should contain only view markup; all PHP logic should be in Livewire component classes.
Files:
resources/views/livewire/agent/_form.blade.phpresources/views/livewire/agent/edit.blade.php
database/migrations/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying a column in a migration, the migration must include all of the attributes that were previously defined on the column. Otherwise, they will be dropped and lost.
Files:
database/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.php
tests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.php: Do not test framework internals, form validation rules (required, max:255, etc.), Eloquent relationships (hasMany, belongsTo), Eloquent cascades (onDelete('cascade')), session flash messages, redirect responses, or multiple variations of the same thing. Focus on business logic, authorization, edge cases in your code, and integration points.
Mock external API services and third-party libraries (AWS SDK, S3 client, etc.), but do NOT mock Model/ORM methods or simple utility functions.
When creating models for tests, use the factories for the models. Check if the factory has custom states that can be used before manually setting up the model.
Use Faker methods such as $this->faker->word() or fake()->randomDigit(). Follow existing conventions whether to use $this->faker or fake().
When creating tests, use 'php artisan make:test [options] {name}' to create a feature test, and pass --unit to create a unit test. Most tests should be feature tests.
Files:
tests/Feature/Agent/AgentCrudTest.php
🧠 Learnings (22)
📓 Common learnings
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Http/Controllers/Api/V1/AgentController.php:175-206
Timestamp: 2026-02-25T10:46:35.025Z
Learning: In the agent job processing flow (app/Http/Controllers/Api/V1/AgentController.php), state updates in endpoints like ack() and fail() are not wrapped in DB::transaction(). The codebase relies on the scheduled agent:recover-leases command to handle recovery of incomplete or failed jobs, rather than using strict transactional consistency for each endpoint.
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Services/Backup/Databases/*.php : All database type implementations must implement DatabaseInterface with methods: setConfig, dump, restore, prepareForRestore, listDatabases, testConnection. Types without PDO support (e.g., Redis) must throw in buildDsn() and handle connection testing via CLI.
Applied to files:
app/Services/Backup/DTO/BackupConfig.php
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
app/Services/Backup/DTO/BackupConfig.phpapp/Console/Commands/AgentRunCommand.phpresources/views/livewire/agent/_form.blade.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Console/Commands/RecoverAgentLeasesCommand.phpapp/Services/Agent/AgentJobPayloadBuilder.phpdatabase/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.phpresources/views/livewire/agent/edit.blade.phptests/Feature/Agent/AgentCrudTest.php
📚 Learning: 2026-02-25T10:46:35.025Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Http/Controllers/Api/V1/AgentController.php:175-206
Timestamp: 2026-02-25T10:46:35.025Z
Learning: In the agent job processing flow (app/Http/Controllers/Api/V1/AgentController.php), state updates in endpoints like ack() and fail() are not wrapped in DB::transaction(). The codebase relies on the scheduled agent:recover-leases command to handle recovery of incomplete or failed jobs, rather than using strict transactional consistency for each endpoint.
Applied to files:
app/Console/Commands/AgentRunCommand.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Console/Commands/RecoverAgentLeasesCommand.phpapp/Services/Agent/AgentJobPayloadBuilder.phpdatabase/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.php
📚 Learning: 2026-02-25T10:48:40.261Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Services/Agent/AgentJobPayloadBuilder.php:63-76
Timestamp: 2026-02-25T10:48:40.261Z
Learning: In app/Services/Agent/AgentJobPayloadBuilder.php and similar service classes, prefer fail-fast error handling over defensive null-safe operators when upstream callers (e.g., TriggerBackupAction, RunScheduledBackups) guarantee certain relationships exist. For example, in resolveBackupPath(), $server->backup is intentionally accessed without the null-safe operator because backup existence is enforced at call sites; using the null-safe operator would hide bugs that violate this invariant.
Applied to files:
app/Console/Commands/AgentRunCommand.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Console/Commands/RecoverAgentLeasesCommand.phpapp/Services/Agent/AgentJobPayloadBuilder.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Jobs/ProcessBackupJob.php : Backup workflow: User clicks 'Backup' → ProcessBackupJob dispatched to queue → Queue worker executes BackupTask service → Creates Snapshot record with status tracking. RestoreTask handles cross-server and same-server restores with appropriate SSL/connection handling for MySQL, MariaDB, and PostgreSQL.
Applied to files:
app/Console/Commands/AgentRunCommand.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Services/Agent/AgentJobPayloadBuilder.php
📚 Learning: 2026-02-13T11:05:44.422Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:44.422Z
Learning: The project uses a global `afterEach` hook in `tests/Pest.php` to clean up temp directories created during tests. The hook includes patterns for `/sqlite-db-test-*`, `/backup-task-test-*`, `/restore-task-test-*`, and `/volume-test-*`, so individual test files don't need their own cleanup logic for temp directories following these naming conventions.
Applied to files:
app/Console/Commands/AgentRunCommand.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Jobs/*.php : Use queued jobs for time-consuming operations with the ShouldQueue interface. Jobs should use the 'backups' queue and handle retries and timeouts appropriately.
Applied to files:
app/Console/Commands/AgentRunCommand.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Services/Agent/AgentJobPayloadBuilder.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Livewire/**/*.php : Use Livewire class-based components for all interactive pages. Authentication flows use plain Blade templates rendered by Laravel Fortify, not Livewire components.
Applied to files:
resources/views/livewire/agent/_form.blade.phpresources/views/livewire/agent/edit.blade.phptests/Feature/Agent/AgentCrudTest.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Livewire/**/*.php : Public properties in Livewire components are automatically bound to views. Use #[Validate] attributes or Form objects for validation. Call $this->validate() before processing data.
Applied to files:
resources/views/livewire/agent/_form.blade.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to resources/views/**/*.blade.php : Modal pattern in Mary UI: Add boolean property to component class and use wire:model in blade file.
Applied to files:
resources/views/livewire/agent/_form.blade.php
📚 Learning: 2026-01-30T22:27:46.107Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 61
File: resources/views/livewire/volume/connectors/s3-config.blade.php:1-13
Timestamp: 2026-01-30T22:27:46.107Z
Learning: In Blade template files (any .blade.php) within the databasement project, allow using alert-info for informational content inside <x-alert> components. The guideline that permits alert-success and alert-error does not exclude using alert-info for informational purposes. Apply this consistently to all Blade components that render alerts; ensure semantic usage and accessibility.
Applied to files:
resources/views/livewire/agent/_form.blade.phpresources/views/livewire/agent/edit.blade.php
📚 Learning: 2026-02-06T10:34:43.585Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 75
File: resources/views/livewire/backup-job/_filters.blade.php:36-40
Timestamp: 2026-02-06T10:34:43.585Z
Learning: In Blade template files, when creating compact inline filter controls, prefer using native <input type="checkbox"> elements with daisyUI classes (e.g., checkbox checkbox-warning checkbox-xs) over the Mary UI <x-checkbox> component. The <x-checkbox> component adds wrapper markup (e.g., <div><fieldset><label> with gap-3) that can break tight inline flex layouts. Use the native input approach for compact inline controls, but reserve <x-checkbox> for form fields that require labels, hints, and errors.
Applied to files:
resources/views/livewire/agent/_form.blade.phpresources/views/livewire/agent/edit.blade.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Services/Backup/BackupJobFactory.php : Types that backup the whole instance (e.g., Redis, SQLite) should short-circuit in BackupJobFactory.createSnapshots() to create a single snapshot instead of per-database snapshots.
Applied to files:
app/Http/Controllers/Api/V1/AgentController.phpapp/Console/Commands/RecoverAgentLeasesCommand.phpapp/Services/Agent/AgentJobPayloadBuilder.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/**/*.php : Avoid DB::; prefer Model::query(). Generate code that leverages Laravel's ORM capabilities rather than bypassing them. Generate code that prevents N+1 query problems by using eager loading.
Applied to files:
app/Console/Commands/RecoverAgentLeasesCommand.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to database/migrations/*.php : When modifying a column in a migration, the migration must include all of the attributes that were previously defined on the column. Otherwise, they will be dropped and lost.
Applied to files:
database/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Models/**/*.php : Database models use ULIDs (Universally Unique Lexicographically Sortable Identifiers) for primary keys instead of auto-incrementing integers.
Applied to files:
database/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to resources/views/**/*.blade.php : In Blade component attributes, use :attr binding (dynamic syntax) instead of {{ }} interpolation when passing translated strings. The {{ }} syntax double-encodes special characters.
Applied to files:
resources/views/livewire/agent/edit.blade.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to tests/**/*.php : When creating tests, use 'php artisan make:test [options] {name}' to create a feature test, and pass --unit to create a unit test. Most tests should be feature tests.
Applied to files:
tests/Feature/Agent/AgentCrudTest.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to tests/**/*.php : Do not test framework internals, form validation rules (required, max:255, etc.), Eloquent relationships (hasMany, belongsTo), Eloquent cascades (onDelete('cascade')), session flash messages, redirect responses, or multiple variations of the same thing. Focus on business logic, authorization, edge cases in your code, and integration points.
Applied to files:
tests/Feature/Agent/AgentCrudTest.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to routes/*.php : All routes use Route::livewire() for full-page Livewire components (e.g., Route::livewire('database-servers', \App\Livewire\DatabaseServer\Index::class)). Authentication routes use plain Blade views rendered by Laravel Fortify.
Applied to files:
tests/Feature/Agent/AgentCrudTest.php
📚 Learning: 2026-02-13T11:05:37.072Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:37.072Z
Learning: Adopt a global afterEach hook in tests/Pest.php (or equivalent Pest bootstrap) to clean up temporary directories created during tests. Specifically handle temp dirs named with the prefixes sqlite-db-test-*, backup-task-test-*, restore-task-test-*, and volume-test-*, so individual test files don’t need their own cleanup logic. This applies to all PHP test files under the tests directory.
Applied to files:
tests/Feature/Agent/AgentCrudTest.php
🧬 Code graph analysis (6)
app/Services/Backup/DTO/BackupConfig.php (2)
app/Services/Backup/DTO/DatabaseConnectionConfig.php (3)
toPayload(72-82)fromPayload(87-106)DatabaseConnectionConfig(8-107)app/Services/Backup/DTO/VolumeConfig.php (3)
toPayload(21-28)fromPayload(42-49)VolumeConfig(7-50)
app/Console/Commands/AgentRunCommand.php (5)
app/Services/Agent/AgentApiClient.php (4)
AgentApiClient(9-83)heartbeat(16-19)claimJob(24-33)ack(46-59)app/Services/Backup/DTO/BackupConfig.php (2)
BackupConfig(7-62)fromPayload(46-61)app/Services/Backup/InMemoryBackupLogger.php (2)
InMemoryBackupLogger(7-90)flush(75-81)app/Support/FilesystemSupport.php (2)
FilesystemSupport(8-57)createWorkingDirectory(19-29)app/Http/Controllers/Api/V1/AgentController.php (3)
heartbeat(23-31)claimJob(38-86)ack(128-169)
app/Http/Controllers/Api/V1/AgentController.php (2)
app/Models/Agent.php (1)
Agent(38-79)app/Models/AgentJob.php (8)
AgentJob(44-169)agent(89-92)claim(105-114)snapshot(97-100)markRunning(119-124)extendLease(154-159)markCompleted(129-136)markFailed(141-149)
app/Console/Commands/RecoverAgentLeasesCommand.php (1)
app/Models/AgentJob.php (3)
AgentJob(44-169)markFailed(141-149)snapshot(97-100)
database/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.php (2)
app/Policies/AgentPolicy.php (2)
create(32-35)delete(50-53)app/Livewire/Agent/Index.php (1)
delete(81-96)
tests/Feature/Agent/AgentCrudTest.php (4)
app/Models/Agent.php (1)
Agent(38-79)app/Models/DatabaseServer.php (2)
DatabaseServer(63-287)agent(120-123)app/Models/User.php (1)
User(68-227)app/Policies/AgentPolicy.php (1)
create(32-35)
🔇 Additional comments (6)
app/Console/Commands/RecoverAgentLeasesCommand.php (1)
46-48: No changes needed — the code is correct.
$job->snapshot->jobresolves to aBackupJobinstance via theSnapshot::job()relationship.BackupJob::markFailedaccepts a\Throwable(line 109), not astring. SinceRuntimeExceptionis aThrowable, passing it tomarkFailed()is type-safe and correct.Likely an incorrect or invalid review comment.
database/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.php (1)
67-69: Rollback cleanup is correct—no morph aliases are configured, and modifier preservation is intact.The Agent model does not override
getMorphClass()or use polymorphic aliases. The originaltokenable_idcolumn has no special modifiers beyondunsignedBigInteger, so the up() and down() migrations correctly preserve this structure. The deletion query targeting'App\\Models\\Agent'is sufficient and will delete all agent tokens before the type conversion.Likely an incorrect or invalid review comment.
app/Services/Backup/DTO/BackupConfig.php (1)
24-39: Payload serialization/deserialization contract is clean and symmetric.The
toPayload()structure aligns well with reconstruction infromPayload(), which keeps the agent payload flow predictable.resources/views/livewire/agent/_form.blade.php (1)
1-29: LGTM — form bindings and Mary UI usage are consistent.The component follows the expected Livewire + Mary patterns, and translated attribute bindings are correctly passed via
:attr.resources/views/livewire/agent/edit.blade.php (1)
25-31: Token regeneration action wiring looks correct.Using
:wire:confirmhere preserves translated special characters and matches the expected Blade binding pattern.app/Services/Agent/AgentJobPayloadBuilder.php (1)
20-35: Nice composition of DTOs into a self-contained payload.
build()cleanly centralizes payload assembly for agent jobs.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/Console/Commands/RecoverAgentLeasesCommand.php (1)
17-21: Eager-load fix confirmed — LGTM.
with(['snapshot.job'])is in place and resolves the previously identified N+1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Console/Commands/RecoverAgentLeasesCommand.php` around lines 17 - 21, The query fetching expired jobs must eagerly load the nested relation to avoid N+1: ensure the AgentJob::query() call in RecoverAgentLeasesCommand includes with(['snapshot.job']) (as shown) when selecting expired jobs, verify the AgentJob model has a snapshot() relation and the Snapshot model has a job() relation matching those names, and keep the ->get() call; run tests to confirm no N+1 remains.
🧹 Nitpick comments (4)
tests/Unit/Services/Backup/DTO/DatabaseConnectionConfigTest.php (1)
110-136: Split into two focused tests.This test covers two independent scenarios — normal reconstruction and default port fallback — in a single function. A failure won't immediately indicate which behaviour broke.
As per coding guidelines (
tests/**/*.php): do not test multiple variations of the same thing.♻️ Proposed split
-test('fromPayload reconstructs config with default port fallback', function () { - $config = DatabaseConnectionConfig::fromPayload([ - 'type' => 'mysql', - 'host' => 'db.example.com', - 'port' => 3306, - 'username' => 'root', - 'password' => 'secret', - 'extra_config' => null, - ], 'My Server'); - - expect($config->databaseType)->toBe(DatabaseType::MYSQL) - ->and($config->serverName)->toBe('My Server') - ->and($config->host)->toBe('db.example.com') - ->and($config->port)->toBe(3306) - ->and($config->password)->toBe('secret') - ->and($config->sshConfig)->toBeNull(); - - // Uses default port when missing or zero - $noPort = DatabaseConnectionConfig::fromPayload([ - 'type' => 'postgres', - 'host' => 'localhost', - 'port' => 0, - 'username' => 'admin', - 'password' => 'pass', - ], 'PG Server'); - - expect($noPort->port)->toBe(5432); -}); +test('fromPayload reconstructs connection config from payload', function () { + $config = DatabaseConnectionConfig::fromPayload([ + 'type' => 'mysql', + 'host' => 'db.example.com', + 'port' => 3306, + 'username' => 'root', + 'password' => 'secret', + 'extra_config' => null, + ], 'My Server'); + + expect($config->databaseType)->toBe(DatabaseType::MYSQL) + ->and($config->serverName)->toBe('My Server') + ->and($config->host)->toBe('db.example.com') + ->and($config->port)->toBe(3306) + ->and($config->password)->toBe('secret') + ->and($config->sshConfig)->toBeNull(); +}); + +test('fromPayload falls back to default port when port is zero', function () { + $config = DatabaseConnectionConfig::fromPayload([ + 'type' => 'postgres', + 'host' => 'localhost', + 'port' => 0, + 'username' => 'admin', + 'password' => 'pass', + ], 'PG Server'); + + expect($config->port)->toBe(5432); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Services/Backup/DTO/DatabaseConnectionConfigTest.php` around lines 110 - 136, The test combines two scenarios; split into two focused tests: keep the existing test named like test('fromPayload reconstructs config with default port fallback', ...) but rename it to test('fromPayload reconstructs config with explicit port') and only assert the reconstruction when port is provided using DatabaseConnectionConfig::fromPayload, checking databaseType, serverName, host, port, password and sshConfig; create a new test (e.g., test('fromPayload applies default port fallback')) that calls DatabaseConnectionConfig::fromPayload with a payload where 'type' => 'postgres', 'host' => 'localhost', and 'port' => 0 (or omitted) and assert $noPort->port->toBe(5432) to isolate the default-port behavior.app/Console/Commands/RecoverAgentLeasesCommand.php (1)
15-53: Guard against concurrent runs on multi-server deployments.The query fetches expired jobs without a
lockForUpdate(), so two simultaneous invocations (e.g., a multi-server scheduler) will process the same jobs twice — causing duplicatemarkFailedcalls or duplicate resets. Consider adding->lockForUpdate()to the query and/or scheduling this command with->onOneServer()->withoutOverlapping()inroutes/console.php.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Console/Commands/RecoverAgentLeasesCommand.php` around lines 15 - 53, The handle() method fetches expired AgentJob rows without any DB locking, so concurrent runs can process the same rows twice; update the AgentJob::query() in handle() to acquire a row lock (e.g., add ->lockForUpdate() or ->sharedLock() as appropriate) and wrap the work in a transaction to ensure the lock is honored before updating/marking jobs (references: AgentJob::query(), handle(), markFailed()); additionally ensure the command is scheduled single-instance in routes/console.php (use ->onOneServer()->withoutOverlapping()) to guard against multi-server scheduler races.app/Http/Controllers/Api/V1/AgentController.php (1)
89-90: Avoid inline implementation comments in controller methods.Prefer method-level PHPDoc (or remove if redundant) instead of in-body comments here.
As per coding guidelines: "Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Api/V1/AgentController.php` around lines 89 - 90, Remove the inline comment above the call to $job->snapshot->job->markRunning() and either delete it entirely or move its content into the containing controller method's PHPDoc block (e.g., the PHPDoc for the AgentController method handling this action) so implementation comments are not placed inline; ensure the PHPDoc briefly states the intent ("mark backup job as running so dashboard shows correct status and duration") and leave the call to $job->snapshot->job->markRunning() unchanged.app/Console/Commands/AgentRunCommand.php (1)
88-88: Replace the PHPStan suppression with an explicit payload guard.
//@phpstan-ignoreargument.typemasks the contract at the call site. Validate payload shape before callingBackupConfig::fromPayload()and remove the inline suppression.Suggested change
- $payload = $job['payload']; + $payload = $job['payload'] ?? null; + if (! is_array($payload)) { + throw new \RuntimeException("Invalid payload for job {$job['id']}"); + } $databaseName = $payload['database']['database_name'] ?? ''; @@ - $config = BackupConfig::fromPayload($payload, $workingDirectory); // `@phpstan-ignore` argument.type + $config = BackupConfig::fromPayload($payload, $workingDirectory);As per coding guidelines: "Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Console/Commands/AgentRunCommand.php` at line 88, Validate the $payload before calling BackupConfig::fromPayload instead of using the inline phpstan ignore: add an explicit guard in AgentRunCommand (or the method preparing $payload) that checks $payload is the expected type/shape (e.g. array/object and required keys/types), throw or return a clear InvalidArgumentException or console error when validation fails, then call BackupConfig::fromPayload($payload, $workingDirectory) and remove the // `@phpstan-ignore` argument.type comment; document the expected payload shape in a PHPDoc block for the method that constructs/receives $payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Http/Controllers/Api/V1/AgentController.php`:
- Line 65: Clamp the configured lease duration to a positive integer before
using it: read config('agent.lease_duration', 300) into $leaseDuration, cast to
int and ensure it's at least 1 (e.g., using max(1, intval(...))). Apply this
change wherever $leaseDuration is used in AgentController (the occurrences
setting $leaseDuration at the top and the second occurrence around lines
130-131) so leases never get a zero or negative expiry.
In `@tests/Unit/Services/Backup/DTO/BackupConfigTest.php`:
- Around line 34-42: The symmetric test in BackupConfigTest is missing
assertions for restored->volume->name, restored->volume->config, and
restored->database->serverName; add assertions to the existing expectation chain
that check restored->volume->name equals the original name,
restored->volume->config equals the original config array/object, and
restored->database->serverName equals the original server name (verifying the
server_name → serverName mapping done by DatabaseConfig::fromPayload and
VolumeConfig::fromPayload preserves name and config).
---
Duplicate comments:
In `@app/Console/Commands/RecoverAgentLeasesCommand.php`:
- Around line 17-21: The query fetching expired jobs must eagerly load the
nested relation to avoid N+1: ensure the AgentJob::query() call in
RecoverAgentLeasesCommand includes with(['snapshot.job']) (as shown) when
selecting expired jobs, verify the AgentJob model has a snapshot() relation and
the Snapshot model has a job() relation matching those names, and keep the
->get() call; run tests to confirm no N+1 remains.
---
Nitpick comments:
In `@app/Console/Commands/AgentRunCommand.php`:
- Line 88: Validate the $payload before calling BackupConfig::fromPayload
instead of using the inline phpstan ignore: add an explicit guard in
AgentRunCommand (or the method preparing $payload) that checks $payload is the
expected type/shape (e.g. array/object and required keys/types), throw or return
a clear InvalidArgumentException or console error when validation fails, then
call BackupConfig::fromPayload($payload, $workingDirectory) and remove the //
`@phpstan-ignore` argument.type comment; document the expected payload shape in a
PHPDoc block for the method that constructs/receives $payload.
In `@app/Console/Commands/RecoverAgentLeasesCommand.php`:
- Around line 15-53: The handle() method fetches expired AgentJob rows without
any DB locking, so concurrent runs can process the same rows twice; update the
AgentJob::query() in handle() to acquire a row lock (e.g., add ->lockForUpdate()
or ->sharedLock() as appropriate) and wrap the work in a transaction to ensure
the lock is honored before updating/marking jobs (references: AgentJob::query(),
handle(), markFailed()); additionally ensure the command is scheduled
single-instance in routes/console.php (use
->onOneServer()->withoutOverlapping()) to guard against multi-server scheduler
races.
In `@app/Http/Controllers/Api/V1/AgentController.php`:
- Around line 89-90: Remove the inline comment above the call to
$job->snapshot->job->markRunning() and either delete it entirely or move its
content into the containing controller method's PHPDoc block (e.g., the PHPDoc
for the AgentController method handling this action) so implementation comments
are not placed inline; ensure the PHPDoc briefly states the intent ("mark backup
job as running so dashboard shows correct status and duration") and leave the
call to $job->snapshot->job->markRunning() unchanged.
In `@tests/Unit/Services/Backup/DTO/DatabaseConnectionConfigTest.php`:
- Around line 110-136: The test combines two scenarios; split into two focused
tests: keep the existing test named like test('fromPayload reconstructs config
with default port fallback', ...) but rename it to test('fromPayload
reconstructs config with explicit port') and only assert the reconstruction when
port is provided using DatabaseConnectionConfig::fromPayload, checking
databaseType, serverName, host, port, password and sshConfig; create a new test
(e.g., test('fromPayload applies default port fallback')) that calls
DatabaseConnectionConfig::fromPayload with a payload where 'type' => 'postgres',
'host' => 'localhost', and 'port' => 0 (or omitted) and assert
$noPort->port->toBe(5432) to isolate the default-port behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/Console/Commands/AgentRunCommand.phpapp/Console/Commands/RecoverAgentLeasesCommand.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Services/Agent/AgentJobPayloadBuilder.phpapp/Services/Backup/DTO/BackupConfig.phptests/Feature/Agent/AgentCrudTest.phptests/Feature/Agent/AgentJobPayloadBuilderTest.phptests/Feature/Agent/TriggerBackupWithAgentTest.phptests/Feature/Console/AgentRunCommandTest.phptests/Feature/Console/RunScheduledBackupsTest.phptests/Unit/Services/Backup/DTO/BackupConfigTest.phptests/Unit/Services/Backup/DTO/DatabaseConnectionConfigTest.phptests/Unit/Services/Backup/DTO/VolumeConfigTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Services/Backup/DTO/BackupConfig.php
- tests/Feature/Agent/AgentCrudTest.php
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.php: Do not test framework internals, form validation rules (required, max:255, etc.), Eloquent relationships (hasMany, belongsTo), Eloquent cascades (onDelete('cascade')), session flash messages, redirect responses, or multiple variations of the same thing. Focus on business logic, authorization, edge cases in your code, and integration points.
Mock external API services and third-party libraries (AWS SDK, S3 client, etc.), but do NOT mock Model/ORM methods or simple utility functions.
When creating models for tests, use the factories for the models. Check if the factory has custom states that can be used before manually setting up the model.
Use Faker methods such as $this->faker->word() or fake()->randomDigit(). Follow existing conventions whether to use $this->faker or fake().
When creating tests, use 'php artisan make:test [options] {name}' to create a feature test, and pass --unit to create a unit test. Most tests should be feature tests.
Files:
tests/Feature/Agent/AgentJobPayloadBuilderTest.phptests/Feature/Agent/TriggerBackupWithAgentTest.phptests/Feature/Console/RunScheduledBackupsTest.phptests/Feature/Console/AgentRunCommandTest.phptests/Unit/Services/Backup/DTO/DatabaseConnectionConfigTest.phptests/Unit/Services/Backup/DTO/BackupConfigTest.phptests/Unit/Services/Backup/DTO/VolumeConfigTest.php
app/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/**/*.php: Use constructor property promotion in PHP 8 __construct() methods. Do not allow empty __construct() methods with zero parameters unless the constructor is private.
Always use explicit return type declarations for methods and functions. Use appropriate PHP type hints for method parameters.
Always use curly braces for control structures, even for single-line bodies.
Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex. Add useful array shape type definitions in PHPDoc when appropriate.
Avoid DB::; prefer Model::query(). Generate code that leverages Laravel's ORM capabilities rather than bypassing them. Generate code that prevents N+1 query problems by using eager loading.
Use Laravel's built-in authentication and authorization features (gates, policies, Sanctum, etc.).
Use environment variables only in configuration files - never use the env() function directly outside of config files. Always use config('key'), not env('KEY').
Use Laravel's query builder for very complex database operations instead of raw SQL queries.
Run 'vendor/bin/pint --dirty' before finalizing changes to ensure code matches the project's expected style. Use 'pint' to fix issues, not 'pint --test'.
Files:
app/Services/Agent/AgentJobPayloadBuilder.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Console/Commands/RecoverAgentLeasesCommand.phpapp/Console/Commands/AgentRunCommand.php
app/Console/Commands/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Console commands in app/Console/Commands/ are automatically available and do not require manual registration.
Files:
app/Console/Commands/RecoverAgentLeasesCommand.phpapp/Console/Commands/AgentRunCommand.php
🧠 Learnings (12)
📓 Common learnings
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Http/Controllers/Api/V1/AgentController.php:175-206
Timestamp: 2026-02-25T10:46:35.025Z
Learning: In the agent job processing flow (app/Http/Controllers/Api/V1/AgentController.php), state updates in endpoints like ack() and fail() are not wrapped in DB::transaction(). The codebase relies on the scheduled agent:recover-leases command to handle recovery of incomplete or failed jobs, rather than using strict transactional consistency for each endpoint.
📚 Learning: 2026-02-25T10:48:40.261Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Services/Agent/AgentJobPayloadBuilder.php:63-76
Timestamp: 2026-02-25T10:48:40.261Z
Learning: In app/Services/Agent/AgentJobPayloadBuilder.php and similar service classes, prefer fail-fast error handling over defensive null-safe operators when upstream callers (e.g., TriggerBackupAction, RunScheduledBackups) guarantee certain relationships exist. For example, in resolveBackupPath(), $server->backup is intentionally accessed without the null-safe operator because backup existence is enforced at call sites; using the null-safe operator would hide bugs that violate this invariant.
Applied to files:
tests/Feature/Agent/AgentJobPayloadBuilderTest.phptests/Feature/Agent/TriggerBackupWithAgentTest.phptests/Feature/Console/RunScheduledBackupsTest.phpapp/Services/Agent/AgentJobPayloadBuilder.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Console/Commands/RecoverAgentLeasesCommand.phpapp/Console/Commands/AgentRunCommand.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Services/Backup/BackupJobFactory.php : Types that backup the whole instance (e.g., Redis, SQLite) should short-circuit in BackupJobFactory.createSnapshots() to create a single snapshot instead of per-database snapshots.
Applied to files:
tests/Feature/Agent/AgentJobPayloadBuilderTest.phptests/Feature/Agent/TriggerBackupWithAgentTest.phptests/Feature/Console/RunScheduledBackupsTest.phpapp/Services/Agent/AgentJobPayloadBuilder.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Console/Commands/RecoverAgentLeasesCommand.php
📚 Learning: 2026-02-13T11:05:37.072Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:37.072Z
Learning: Adopt a global afterEach hook in tests/Pest.php (or equivalent Pest bootstrap) to clean up temporary directories created during tests. Specifically handle temp dirs named with the prefixes sqlite-db-test-*, backup-task-test-*, restore-task-test-*, and volume-test-*, so individual test files don’t need their own cleanup logic. This applies to all PHP test files under the tests directory.
Applied to files:
tests/Feature/Agent/AgentJobPayloadBuilderTest.phptests/Feature/Agent/TriggerBackupWithAgentTest.phptests/Feature/Console/RunScheduledBackupsTest.phptests/Feature/Console/AgentRunCommandTest.phptests/Unit/Services/Backup/DTO/DatabaseConnectionConfigTest.phptests/Unit/Services/Backup/DTO/BackupConfigTest.phptests/Unit/Services/Backup/DTO/VolumeConfigTest.php
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
tests/Feature/Agent/AgentJobPayloadBuilderTest.phptests/Feature/Agent/TriggerBackupWithAgentTest.phptests/Feature/Console/RunScheduledBackupsTest.phptests/Feature/Console/AgentRunCommandTest.phptests/Unit/Services/Backup/DTO/DatabaseConnectionConfigTest.phpapp/Services/Agent/AgentJobPayloadBuilder.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Console/Commands/RecoverAgentLeasesCommand.phpapp/Console/Commands/AgentRunCommand.phptests/Unit/Services/Backup/DTO/BackupConfigTest.phptests/Unit/Services/Backup/DTO/VolumeConfigTest.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to tests/**/*.php : When creating tests, use 'php artisan make:test [options] {name}' to create a feature test, and pass --unit to create a unit test. Most tests should be feature tests.
Applied to files:
tests/Feature/Agent/TriggerBackupWithAgentTest.phptests/Feature/Console/AgentRunCommandTest.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Jobs/ProcessBackupJob.php : Backup workflow: User clicks 'Backup' → ProcessBackupJob dispatched to queue → Queue worker executes BackupTask service → Creates Snapshot record with status tracking. RestoreTask handles cross-server and same-server restores with appropriate SSL/connection handling for MySQL, MariaDB, and PostgreSQL.
Applied to files:
tests/Feature/Agent/TriggerBackupWithAgentTest.phptests/Feature/Console/RunScheduledBackupsTest.phpapp/Services/Agent/AgentJobPayloadBuilder.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Console/Commands/AgentRunCommand.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Jobs/*.php : Use queued jobs for time-consuming operations with the ShouldQueue interface. Jobs should use the 'backups' queue and handle retries and timeouts appropriately.
Applied to files:
tests/Feature/Agent/TriggerBackupWithAgentTest.phptests/Feature/Console/RunScheduledBackupsTest.phpapp/Services/Agent/AgentJobPayloadBuilder.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Console/Commands/AgentRunCommand.php
📚 Learning: 2026-02-25T10:46:35.025Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Http/Controllers/Api/V1/AgentController.php:175-206
Timestamp: 2026-02-25T10:46:35.025Z
Learning: In the agent job processing flow (app/Http/Controllers/Api/V1/AgentController.php), state updates in endpoints like ack() and fail() are not wrapped in DB::transaction(). The codebase relies on the scheduled agent:recover-leases command to handle recovery of incomplete or failed jobs, rather than using strict transactional consistency for each endpoint.
Applied to files:
tests/Feature/Agent/TriggerBackupWithAgentTest.phptests/Feature/Console/RunScheduledBackupsTest.phpapp/Services/Agent/AgentJobPayloadBuilder.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Console/Commands/RecoverAgentLeasesCommand.phpapp/Console/Commands/AgentRunCommand.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Services/Backup/Databases/*.php : All database type implementations must implement DatabaseInterface with methods: setConfig, dump, restore, prepareForRestore, listDatabases, testConnection. Types without PDO support (e.g., Redis) must throw in buildDsn() and handle connection testing via CLI.
Applied to files:
tests/Unit/Services/Backup/DTO/DatabaseConnectionConfigTest.phptests/Unit/Services/Backup/DTO/BackupConfigTest.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/**/*.php : Avoid DB::; prefer Model::query(). Generate code that leverages Laravel's ORM capabilities rather than bypassing them. Generate code that prevents N+1 query problems by using eager loading.
Applied to files:
app/Console/Commands/RecoverAgentLeasesCommand.php
📚 Learning: 2026-02-13T11:05:44.422Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:44.422Z
Learning: The project uses a global `afterEach` hook in `tests/Pest.php` to clean up temp directories created during tests. The hook includes patterns for `/sqlite-db-test-*`, `/backup-task-test-*`, `/restore-task-test-*`, and `/volume-test-*`, so individual test files don't need their own cleanup logic for temp directories following these naming conventions.
Applied to files:
app/Console/Commands/AgentRunCommand.php
🧬 Code graph analysis (9)
tests/Feature/Agent/AgentJobPayloadBuilderTest.php (2)
app/Services/Agent/AgentJobPayloadBuilder.php (2)
AgentJobPayloadBuilder(13-53)build(20-35)database/factories/SnapshotFactory.php (1)
forServer(59-68)
tests/Feature/Agent/TriggerBackupWithAgentTest.php (3)
app/Models/AgentJob.php (2)
AgentJob(44-169)agent(89-92)app/Models/DatabaseServer.php (2)
DatabaseServer(63-287)agent(120-123)app/Services/Backup/TriggerBackupAction.php (2)
TriggerBackupAction(12-84)execute(26-55)
tests/Feature/Console/RunScheduledBackupsTest.php (6)
app/Models/DatabaseServer.php (3)
agent(120-123)DatabaseServer(63-287)backup(128-131)app/Models/Agent.php (1)
Agent(38-79)app/Models/Snapshot.php (1)
backup(205-208)app/Http/Controllers/Api/V1/DatabaseServerController.php (1)
backup(53-73)app/Models/Backup.php (1)
backupSchedule(85-88)app/Models/AgentJob.php (1)
AgentJob(44-169)
tests/Unit/Services/Backup/DTO/DatabaseConnectionConfigTest.php (1)
app/Services/Backup/DTO/DatabaseConnectionConfig.php (3)
DatabaseConnectionConfig(8-107)toPayload(72-82)fromPayload(87-106)
app/Http/Controllers/Api/V1/AgentController.php (2)
app/Models/Agent.php (1)
Agent(38-79)app/Models/AgentJob.php (8)
AgentJob(44-169)agent(89-92)claim(105-114)snapshot(97-100)markRunning(119-124)extendLease(154-159)markCompleted(129-136)markFailed(141-149)
app/Console/Commands/RecoverAgentLeasesCommand.php (1)
app/Models/AgentJob.php (3)
AgentJob(44-169)markFailed(141-149)snapshot(97-100)
app/Console/Commands/AgentRunCommand.php (6)
app/Services/Agent/AgentApiClient.php (6)
AgentApiClient(9-83)heartbeat(16-19)claimJob(24-33)jobHeartbeat(38-41)ack(46-59)fail(64-70)app/Services/Backup/BackupTask.php (1)
BackupTask(17-160)app/Services/Backup/DTO/BackupConfig.php (2)
BackupConfig(7-68)fromPayload(52-67)app/Services/Backup/InMemoryBackupLogger.php (2)
InMemoryBackupLogger(7-90)flush(75-81)app/Support/FilesystemSupport.php (2)
FilesystemSupport(8-57)createWorkingDirectory(19-29)app/Http/Controllers/Api/V1/AgentController.php (5)
heartbeat(45-53)claimJob(60-108)jobHeartbeat(115-141)ack(148-186)fail(193-223)
tests/Unit/Services/Backup/DTO/BackupConfigTest.php (3)
app/Services/Backup/DTO/BackupConfig.php (1)
BackupConfig(7-68)app/Services/Backup/DTO/DatabaseConnectionConfig.php (1)
DatabaseConnectionConfig(8-107)app/Services/Backup/DTO/VolumeConfig.php (1)
VolumeConfig(7-50)
tests/Unit/Services/Backup/DTO/VolumeConfigTest.php (1)
app/Services/Backup/DTO/VolumeConfig.php (1)
VolumeConfig(7-50)
🔇 Additional comments (9)
tests/Unit/Services/Backup/DTO/DatabaseConnectionConfigTest.php (1)
89-108: LGTM!Assertions cover all keys in
toPayload()and correctly verify thepostgresenum value.tests/Unit/Services/Backup/DTO/VolumeConfigTest.php (1)
1-38: LGTM!All three tests are focused, cover the meaningful behaviors (
toPayloadshape, normal reconstruction, and missing-field defaults), and align exactly with thefromPayloadimplementation.tests/Unit/Services/Backup/DTO/BackupConfigTest.php (1)
45-65: LGTM!Correctly verifies both the nesting of
server_name(top-level) vsdatabase_name(underdatabase), and thatcompression.type/compression.levelarenullwhen not supplied.app/Console/Commands/RecoverAgentLeasesCommand.php (1)
44-46: No issues found.BackupJob::markFailed(\Throwable $exception)correctly accepts the RuntimeException being passed on line 45. The method signatures are properly aligned across the relationship chain:AgentJob → Snapshot → BackupJob.Likely an incorrect or invalid review comment.
tests/Feature/Console/AgentRunCommandTest.php (1)
3-9: Good failure-path coverage for required agent configuration.This gives a clear regression check for the command’s startup guard.
app/Services/Agent/AgentJobPayloadBuilder.php (1)
20-35: Builder composition looks clean and consistent.Using DTOs plus
toPayload()keeps the agent contract centralized.tests/Feature/Console/RunScheduledBackupsTest.php (1)
185-201: Nice integration check for the scheduled “via agent” path.The assertions clearly protect the agent-routing branch from regressing to queue dispatch.
tests/Feature/Agent/AgentJobPayloadBuilderTest.php (1)
7-39: Good focused tests for backup path resolution edge cases.These assertions map well to the two key branches in
resolveBackupPath().tests/Feature/Agent/TriggerBackupWithAgentTest.php (1)
9-49: Strong branch coverage for agent-vs-queue dispatch behavior.This pair of tests gives clear protection for both execution routes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/Feature/Console/AgentRunCommandTest.php (1)
49-61:⚠️ Potential issue | 🟠 MajorThese two tests (
exits cleanly when no jobsandhandles http errors) each sleeppoll_intervalseconds due to the--once/sleepordering inAgentRunCommand.With
beforeEachsettingpoll_intervalto5andmax(1, …)in the command enforcing a 1-second minimum, the test suite is at minimum 2 seconds slower per run (and 10 seconds with the current config), solely from the unnecessarysleep()before the--oncebreak. This is resolved by applying the fix proposed inAgentRunCommand.php— no changes are needed here once the command is patched.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/Console/AgentRunCommandTest.php` around lines 49 - 61, The tests are slowed by AgentRunCommand sleeping before honoring the --once break; update the AgentRunCommand handle() loop so it checks the --once flag and exits immediately when a single run is requested (or when no jobs are returned) before calling sleep(poll_interval), i.e., move or guard the sleep() so that the code path that breaks on $this->option('once') (or the --once handling) executes prior to any sleep, ensuring poll_interval (and the max(1, …) minimum) is not applied for a single-run invocation; verify the loop and any uses of sleep() and poll_interval in AgentRunCommand::handle() are adjusted accordingly.
🧹 Nitpick comments (3)
app/Http/Controllers/Api/V1/AgentController.php (2)
124-124: Use strictin_arraycomparisons throughout (also applies to Lines 157 and 202).All three status-guard calls omit the
truestrict-mode flag. While DB string values make accidental coercion unlikely today, strict comparison is safer ifstatusever becomes a typed enum or backing value changes.♻️ Proposed change (apply identically at lines 157 and 202)
- if (! in_array($agentJob->status, [AgentJob::STATUS_CLAIMED, AgentJob::STATUS_RUNNING])) { + if (! in_array($agentJob->status, [AgentJob::STATUS_CLAIMED, AgentJob::STATUS_RUNNING], true)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Api/V1/AgentController.php` at line 124, The in_array checks in AgentController are missing strict comparison; update each call that checks $agentJob->status (the one using in_array($agentJob->status, [AgentJob::STATUS_CLAIMED, AgentJob::STATUS_RUNNING]) and the analogous checks around the other guards at the locations referenced) to pass the strict flag true as the third argument (i.e., in_array(..., ..., true)) so comparisons against AgentJob::STATUS_* use strict type-safe matching.
89-89: Remove inline comment — use PHPDoc or remove per coding guidelines.The
//inline comment on line 89 explains non-complex bookkeeping and should be replaced with a PHPDoc on the method or removed. As per coding guidelines, inline comments are reserved for exceptionally complex logic.♻️ Proposed change
- // Mark the backup job as running so dashboard shows correct status and duration tracks $job->snapshot->job->markRunning();If the explanation is important, move it into the method's PHPDoc block instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Api/V1/AgentController.php` at line 89, Remove the inline "// Mark the backup job as running so dashboard shows correct status and duration tracks" comment in AgentController and either delete it or move its text into the PHPDoc for the method that contains the backup-job bookkeeping (add a short `@summary` or description above that method in the class PHPDoc). Ensure no remaining inline comments remain in that method and that the PHPDoc clearly documents the bookkeeping behavior.app/Console/Commands/AgentRunCommand.php (1)
92-92: Consider moving the@phpstan-ignoresuppression to a PHPStan baseline or a PHPDoc@paramannotation.An inline
//@phpstan-ignore`` comment inside method bodies is technically a violation of the "Never use comments within the code itself" guideline. If this suppression is needed long-term, registering it inphpstan-baseline.neonkeeps the method body clean without losing the suppression.As per coding guidelines: "Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Console/Commands/AgentRunCommand.php` at line 92, Remove the inline "// `@phpstan-ignore` argument.type" on the BackupConfig::fromPayload($payload, $workingDirectory) call and instead register the suppression in your PHPStan configuration (phpstan-baseline.neon) or add an appropriate PHPDoc type annotation for the method/variable that clarifies the expected payload type (e.g., update the docblock for the caller or for BackupConfig::fromPayload with `@param` types) so the type mismatch is suppressed via configuration or PHPDoc rather than an inline comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Console/Commands/AgentRunCommand.php`:
- Around line 49-59: The loop currently calls sleep($pollInterval) in both the
"no job" branch and the catch block before checking $this->option('once'),
causing unnecessary delays for single-run (--once) mode; update the logic in
AgentRunCommand (the branches that call sleep and the surrounding try/catch) so
that you either: 1) check $this->option('once') immediately after the work/error
handling and skip calling sleep when true, or 2) gate each sleep($pollInterval)
call with if (!$this->option('once')) to avoid sleeping when --once is set;
ensure you adjust both the no-job branch and the catch block so both skip
sleeping for --once.
---
Duplicate comments:
In `@tests/Feature/Console/AgentRunCommandTest.php`:
- Around line 49-61: The tests are slowed by AgentRunCommand sleeping before
honoring the --once break; update the AgentRunCommand handle() loop so it checks
the --once flag and exits immediately when a single run is requested (or when no
jobs are returned) before calling sleep(poll_interval), i.e., move or guard the
sleep() so that the code path that breaks on $this->option('once') (or the
--once handling) executes prior to any sleep, ensuring poll_interval (and the
max(1, …) minimum) is not applied for a single-run invocation; verify the loop
and any uses of sleep() and poll_interval in AgentRunCommand::handle() are
adjusted accordingly.
---
Nitpick comments:
In `@app/Console/Commands/AgentRunCommand.php`:
- Line 92: Remove the inline "// `@phpstan-ignore` argument.type" on the
BackupConfig::fromPayload($payload, $workingDirectory) call and instead register
the suppression in your PHPStan configuration (phpstan-baseline.neon) or add an
appropriate PHPDoc type annotation for the method/variable that clarifies the
expected payload type (e.g., update the docblock for the caller or for
BackupConfig::fromPayload with `@param` types) so the type mismatch is suppressed
via configuration or PHPDoc rather than an inline comment.
In `@app/Http/Controllers/Api/V1/AgentController.php`:
- Line 124: The in_array checks in AgentController are missing strict
comparison; update each call that checks $agentJob->status (the one using
in_array($agentJob->status, [AgentJob::STATUS_CLAIMED,
AgentJob::STATUS_RUNNING]) and the analogous checks around the other guards at
the locations referenced) to pass the strict flag true as the third argument
(i.e., in_array(..., ..., true)) so comparisons against AgentJob::STATUS_* use
strict type-safe matching.
- Line 89: Remove the inline "// Mark the backup job as running so dashboard
shows correct status and duration tracks" comment in AgentController and either
delete it or move its text into the PHPDoc for the method that contains the
backup-job bookkeeping (add a short `@summary` or description above that method in
the class PHPDoc). Ensure no remaining inline comments remain in that method and
that the PHPDoc clearly documents the bookkeeping behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/Console/Commands/AgentRunCommand.phpapp/Http/Controllers/Api/V1/AgentController.phptests/Feature/Console/AgentRunCommandTest.phptests/Unit/Services/Backup/DTO/BackupConfigTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/Services/Backup/DTO/BackupConfigTest.php
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
app/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/**/*.php: Use constructor property promotion in PHP 8 __construct() methods. Do not allow empty __construct() methods with zero parameters unless the constructor is private.
Always use explicit return type declarations for methods and functions. Use appropriate PHP type hints for method parameters.
Always use curly braces for control structures, even for single-line bodies.
Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex. Add useful array shape type definitions in PHPDoc when appropriate.
Avoid DB::; prefer Model::query(). Generate code that leverages Laravel's ORM capabilities rather than bypassing them. Generate code that prevents N+1 query problems by using eager loading.
Use Laravel's built-in authentication and authorization features (gates, policies, Sanctum, etc.).
Use environment variables only in configuration files - never use the env() function directly outside of config files. Always use config('key'), not env('KEY').
Use Laravel's query builder for very complex database operations instead of raw SQL queries.
Run 'vendor/bin/pint --dirty' before finalizing changes to ensure code matches the project's expected style. Use 'pint' to fix issues, not 'pint --test'.
Files:
app/Console/Commands/AgentRunCommand.phpapp/Http/Controllers/Api/V1/AgentController.php
app/Console/Commands/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Console commands in app/Console/Commands/ are automatically available and do not require manual registration.
Files:
app/Console/Commands/AgentRunCommand.php
tests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.php: Do not test framework internals, form validation rules (required, max:255, etc.), Eloquent relationships (hasMany, belongsTo), Eloquent cascades (onDelete('cascade')), session flash messages, redirect responses, or multiple variations of the same thing. Focus on business logic, authorization, edge cases in your code, and integration points.
Mock external API services and third-party libraries (AWS SDK, S3 client, etc.), but do NOT mock Model/ORM methods or simple utility functions.
When creating models for tests, use the factories for the models. Check if the factory has custom states that can be used before manually setting up the model.
Use Faker methods such as $this->faker->word() or fake()->randomDigit(). Follow existing conventions whether to use $this->faker or fake().
When creating tests, use 'php artisan make:test [options] {name}' to create a feature test, and pass --unit to create a unit test. Most tests should be feature tests.
Files:
tests/Feature/Console/AgentRunCommandTest.php
🧠 Learnings (12)
📓 Common learnings
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Http/Controllers/Api/V1/AgentController.php:175-206
Timestamp: 2026-02-25T10:46:35.025Z
Learning: In the agent job processing flow (app/Http/Controllers/Api/V1/AgentController.php), state updates in endpoints like ack() and fail() are not wrapped in DB::transaction(). The codebase relies on the scheduled agent:recover-leases command to handle recovery of incomplete or failed jobs, rather than using strict transactional consistency for each endpoint.
📚 Learning: 2026-02-25T10:46:35.025Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Http/Controllers/Api/V1/AgentController.php:175-206
Timestamp: 2026-02-25T10:46:35.025Z
Learning: In the agent job processing flow (app/Http/Controllers/Api/V1/AgentController.php), state updates in endpoints like ack() and fail() are not wrapped in DB::transaction(). The codebase relies on the scheduled agent:recover-leases command to handle recovery of incomplete or failed jobs, rather than using strict transactional consistency for each endpoint.
Applied to files:
app/Console/Commands/AgentRunCommand.phptests/Feature/Console/AgentRunCommandTest.phpapp/Http/Controllers/Api/V1/AgentController.php
📚 Learning: 2026-02-25T10:48:40.261Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Services/Agent/AgentJobPayloadBuilder.php:63-76
Timestamp: 2026-02-25T10:48:40.261Z
Learning: In app/Services/Agent/AgentJobPayloadBuilder.php and similar service classes, prefer fail-fast error handling over defensive null-safe operators when upstream callers (e.g., TriggerBackupAction, RunScheduledBackups) guarantee certain relationships exist. For example, in resolveBackupPath(), $server->backup is intentionally accessed without the null-safe operator because backup existence is enforced at call sites; using the null-safe operator would hide bugs that violate this invariant.
Applied to files:
app/Console/Commands/AgentRunCommand.phptests/Feature/Console/AgentRunCommandTest.phpapp/Http/Controllers/Api/V1/AgentController.php
📚 Learning: 2026-02-13T11:05:44.422Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:44.422Z
Learning: The project uses a global `afterEach` hook in `tests/Pest.php` to clean up temp directories created during tests. The hook includes patterns for `/sqlite-db-test-*`, `/backup-task-test-*`, `/restore-task-test-*`, and `/volume-test-*`, so individual test files don't need their own cleanup logic for temp directories following these naming conventions.
Applied to files:
app/Console/Commands/AgentRunCommand.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Jobs/ProcessBackupJob.php : Backup workflow: User clicks 'Backup' → ProcessBackupJob dispatched to queue → Queue worker executes BackupTask service → Creates Snapshot record with status tracking. RestoreTask handles cross-server and same-server restores with appropriate SSL/connection handling for MySQL, MariaDB, and PostgreSQL.
Applied to files:
app/Console/Commands/AgentRunCommand.phpapp/Http/Controllers/Api/V1/AgentController.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Jobs/*.php : Use queued jobs for time-consuming operations with the ShouldQueue interface. Jobs should use the 'backups' queue and handle retries and timeouts appropriately.
Applied to files:
app/Console/Commands/AgentRunCommand.phpapp/Http/Controllers/Api/V1/AgentController.php
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
app/Console/Commands/AgentRunCommand.phptests/Feature/Console/AgentRunCommandTest.phpapp/Http/Controllers/Api/V1/AgentController.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to tests/**/*.php : When creating tests, use 'php artisan make:test [options] {name}' to create a feature test, and pass --unit to create a unit test. Most tests should be feature tests.
Applied to files:
tests/Feature/Console/AgentRunCommandTest.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to tests/**/*.php : Mock external API services and third-party libraries (AWS SDK, S3 client, etc.), but do NOT mock Model/ORM methods or simple utility functions.
Applied to files:
tests/Feature/Console/AgentRunCommandTest.php
📚 Learning: 2026-02-17T08:43:03.630Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 114
File: tests/Integration/SshTunnelTest.php:37-46
Timestamp: 2026-02-17T08:43:03.630Z
Learning: In the databasement project, integration tests use RefreshDatabase (configured in tests/Pest.php), which automatically rolls back all database changes after each test. Therefore, explicit model deletion calls like $server->delete(), $volume->delete(), or $sshConfig->delete() are unnecessary in integration tests.
Applied to files:
tests/Feature/Console/AgentRunCommandTest.php
📚 Learning: 2026-02-13T11:05:37.072Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:37.072Z
Learning: Adopt a global afterEach hook in tests/Pest.php (or equivalent Pest bootstrap) to clean up temporary directories created during tests. Specifically handle temp dirs named with the prefixes sqlite-db-test-*, backup-task-test-*, restore-task-test-*, and volume-test-*, so individual test files don’t need their own cleanup logic. This applies to all PHP test files under the tests directory.
Applied to files:
tests/Feature/Console/AgentRunCommandTest.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Services/Backup/BackupJobFactory.php : Types that backup the whole instance (e.g., Redis, SQLite) should short-circuit in BackupJobFactory.createSnapshots() to create a single snapshot instead of per-database snapshots.
Applied to files:
app/Http/Controllers/Api/V1/AgentController.php
🧬 Code graph analysis (2)
app/Console/Commands/AgentRunCommand.php (7)
app/Services/Agent/AgentApiClient.php (6)
AgentApiClient(9-83)heartbeat(16-19)claimJob(24-33)jobHeartbeat(38-41)ack(46-59)fail(64-70)app/Services/Backup/BackupTask.php (1)
BackupTask(17-160)app/Services/Backup/DTO/BackupConfig.php (2)
BackupConfig(7-68)fromPayload(52-67)app/Services/Backup/InMemoryBackupLogger.php (2)
InMemoryBackupLogger(7-90)flush(75-81)app/Support/FilesystemSupport.php (2)
FilesystemSupport(8-57)createWorkingDirectory(19-29)app/Http/Controllers/Api/V1/AgentController.php (5)
heartbeat(45-53)claimJob(60-108)jobHeartbeat(115-141)ack(148-186)fail(193-223)app/Services/Backup/DTO/DatabaseConnectionConfig.php (1)
fromPayload(87-106)
tests/Feature/Console/AgentRunCommandTest.php (2)
app/Services/Backup/BackupTask.php (1)
BackupTask(17-160)app/Services/Backup/DTO/BackupResult.php (1)
BackupResult(5-12)
🔇 Additional comments (2)
app/Http/Controllers/Api/V1/AgentController.php (1)
45-223: Overall LGTM — well-structured lifecycle controller.The shared
logRules()helper, clamped lease duration, ownership/status guards, and consistent response shapes are all clean. The no-transaction pattern inack()/fail()is consistent with the codebase's recovery-via-agent:recover-leasesdesign. No new concerns beyond the two nits above.tests/Feature/Console/AgentRunCommandTest.php (1)
63-84: LGTM — job success path is well-tested.Mocking
BackupTaskat the service level while usingHttp::fake()to verify theackpayload shape (filename,file_size,checksum) is the right approach. Skipping ajobHeartbeatstub is correct since theonProgresscallback is never invoked whenBackupTask::executeis mocked to return directly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/Livewire/Agent/Edit.php (1)
55-60: Remove self-evident inline comments.Both comments describe operations that are immediately obvious from the code.
♻️ Proposed cleanup
- // Revoke all existing tokens $this->form->agent->tokens()->delete(); - // Create new token $token = $this->form->agent->createToken('agent'); $this->showTokenModal($token->plainTextToken);As per coding guidelines: "Never use comments within the code itself unless the logic is exceptionally complex."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Livewire/Agent/Edit.php` around lines 55 - 60, Remove the two self-evident inline comments above the token operations in Edit.php: delete the "// Revoke all existing tokens" and "// Create new token" lines so the code remains concise and follows the guideline; keep the existing calls to $this->form->agent->tokens()->delete(), $this->form->agent->createToken('agent') and $this->showTokenModal($token->plainTextToken) unchanged.app/Livewire/Concerns/HasAgentToken.php (1)
31-31: Avoid hardcoding:latestDocker image tag.Using
latestmakes deployments non-reproducible and can silently introduce breaking changes for users. Consider driving this from a config value (e.g.,config('agent.docker_image', 'davidcrty/databasement:latest')).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Livewire/Concerns/HasAgentToken.php` at line 31, The hardcoded Docker image string "' davidcrty/databasement:latest \\'" should be driven from configuration to avoid using the non-reproducible ":latest" tag; update the construction in HasAgentToken (where the image string is defined/returned) to read the image from config('agent.docker_image', 'davidcrty/databasement:1.0.0') or a sensible pinned default, and replace the inline literal with that config value so consumers can override the tag via configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Livewire/Concerns/HasAgentToken.php`:
- Around line 26-33: The docker env assignment for DATABASEMENT_URL in
HasAgentToken.php builds $this->dockerCommand using $url unquoted; update the
command so DATABASEMENT_URL is quoted (same shell-safe quoting used for
DATABASEMENT_AGENT_TOKEN) or properly escaped when constructing
$this->dockerCommand to avoid shell interpolation/word-splitting for values like
$url; locate the $this->dockerCommand assembly and change the " -e
DATABASEMENT_URL={$url} \\" fragment to use a quoted form referencing $url (and
keep DATABASEMENT_AGENT_TOKEN handling consistent with $this->newToken).
---
Nitpick comments:
In `@app/Livewire/Agent/Edit.php`:
- Around line 55-60: Remove the two self-evident inline comments above the token
operations in Edit.php: delete the "// Revoke all existing tokens" and "//
Create new token" lines so the code remains concise and follows the guideline;
keep the existing calls to $this->form->agent->tokens()->delete(),
$this->form->agent->createToken('agent') and
$this->showTokenModal($token->plainTextToken) unchanged.
In `@app/Livewire/Concerns/HasAgentToken.php`:
- Line 31: The hardcoded Docker image string "' davidcrty/databasement:latest
\\'" should be driven from configuration to avoid using the non-reproducible
":latest" tag; update the construction in HasAgentToken (where the image string
is defined/returned) to read the image from config('agent.docker_image',
'davidcrty/databasement:1.0.0') or a sensible pinned default, and replace the
inline literal with that config value so consumers can override the tag via
configuration.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/Console/Commands/AgentRunCommand.phpapp/Livewire/Agent/Create.phpapp/Livewire/Agent/Edit.phpapp/Livewire/Concerns/HasAgentToken.phpresources/views/livewire/agent/_token-modal.blade.php
🚧 Files skipped from review as they are similar to previous changes (3)
- app/Livewire/Agent/Create.php
- app/Console/Commands/AgentRunCommand.php
- resources/views/livewire/agent/_token-modal.blade.php
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-app-image
🧰 Additional context used
📓 Path-based instructions (2)
app/Livewire/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/Livewire/**/*.php: Use Livewire class-based components for all interactive pages. Authentication flows use plain Blade templates rendered by Laravel Fortify, not Livewire components.
Public properties in Livewire components are automatically bound to views. Use #[Validate] attributes or Form objects for validation. Call $this->validate() before processing data.
Use Session::flash() for one-time messages in Livewire components, shown via@if(session('success')). Return $this->redirect() with navigate: true for SPA-like navigation.
Files:
app/Livewire/Concerns/HasAgentToken.phpapp/Livewire/Agent/Edit.php
app/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/**/*.php: Use constructor property promotion in PHP 8 __construct() methods. Do not allow empty __construct() methods with zero parameters unless the constructor is private.
Always use explicit return type declarations for methods and functions. Use appropriate PHP type hints for method parameters.
Always use curly braces for control structures, even for single-line bodies.
Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex. Add useful array shape type definitions in PHPDoc when appropriate.
Avoid DB::; prefer Model::query(). Generate code that leverages Laravel's ORM capabilities rather than bypassing them. Generate code that prevents N+1 query problems by using eager loading.
Use Laravel's built-in authentication and authorization features (gates, policies, Sanctum, etc.).
Use environment variables only in configuration files - never use the env() function directly outside of config files. Always use config('key'), not env('KEY').
Use Laravel's query builder for very complex database operations instead of raw SQL queries.
Run 'vendor/bin/pint --dirty' before finalizing changes to ensure code matches the project's expected style. Use 'pint' to fix issues, not 'pint --test'.
Files:
app/Livewire/Concerns/HasAgentToken.phpapp/Livewire/Agent/Edit.php
🧠 Learnings (5)
📓 Common learnings
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Http/Controllers/Api/V1/AgentController.php:175-206
Timestamp: 2026-02-25T10:46:35.025Z
Learning: In the agent job processing flow (app/Http/Controllers/Api/V1/AgentController.php), state updates in endpoints like ack() and fail() are not wrapped in DB::transaction(). The codebase relies on the scheduled agent:recover-leases command to handle recovery of incomplete or failed jobs, rather than using strict transactional consistency for each endpoint.
📚 Learning: 2026-02-18T09:45:52.485Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 116
File: app/Livewire/DatabaseServer/ConnectionStatus.php:18-18
Timestamp: 2026-02-18T09:45:52.485Z
Learning: In Livewire components, Eloquent model properties (e.g., public DatabaseServer $server) are automatically locked by the framework to prevent client-side ID tampering. The #[Locked] attribute is only needed for scalar properties (int, string, bool, etc.) that require protection from client-side mutation. Apply this guidance to all Livewire PHP components; use #[Locked] only on primitive properties that you want to shield from client manipulation, and rely on automatic locking for Eloquent model properties.
Applied to files:
app/Livewire/Concerns/HasAgentToken.phpapp/Livewire/Agent/Edit.php
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
app/Livewire/Concerns/HasAgentToken.phpapp/Livewire/Agent/Edit.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to app/Livewire/**/*.php : Use Livewire class-based components for all interactive pages. Authentication flows use plain Blade templates rendered by Laravel Fortify, not Livewire components.
Applied to files:
app/Livewire/Agent/Edit.php
📚 Learning: 2026-02-20T23:44:19.974Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.974Z
Learning: Applies to routes/*.php : All routes use Route::livewire() for full-page Livewire components (e.g., Route::livewire('database-servers', \App\Livewire\DatabaseServer\Index::class)). Authentication routes use plain Blade views rendered by Laravel Fortify.
Applied to files:
app/Livewire/Agent/Edit.php
🧬 Code graph analysis (1)
app/Livewire/Concerns/HasAgentToken.php (1)
app/Livewire/Agent/Create.php (1)
closeTokenModal(44-50)
🔇 Additional comments (2)
app/Livewire/Agent/Edit.php (1)
25-30: LGTM!app/Livewire/Concerns/HasAgentToken.php (1)
43-54: LGTM — clean hook design.
closeTokenModal()delegates toresetTokenModal(), giving consumers (e.g.,Create.php) a stable override point to attach post-close behaviour (flash + redirect) without duplicating the reset logic.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Livewire/Agent/Edit.php (1)
54-67: Consider wrapping token deletion and creation in a transaction.If
createToken()fails aftertokens()->delete()succeeds, the agent will be left without any authentication token and cannot reconnect until a new token is manually generated. Unlike job processing where theagent:recover-leasescommand handles recovery, there's no automatic recovery path for lost tokens.🛡️ Proposed fix to ensure atomicity
public function regenerateToken(): void { if ($this->abortIfDemoMode('agents.index')) { return; } $this->authorize('update', $this->form->agent); - $this->form->agent->tokens()->delete(); - - $token = $this->form->agent->createToken('agent'); + $token = \Illuminate\Support\Facades\DB::transaction(function () { + $this->form->agent->tokens()->delete(); + + return $this->form->agent->createToken('agent'); + }); + $this->showRegenerateModal = false; $this->showTokenModal($token->plainTextToken); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Livewire/Agent/Edit.php` around lines 54 - 67, Wrap the current tokens()->delete() and createToken('agent') calls in a single database transaction inside regenerateToken() so both operations are atomic: begin a DB::transaction, perform $this->form->agent->tokens()->delete() then $this->form->agent->createToken('agent'), commit only on success, and on exception roll back and handle the error (log and surface an error message / keep $this->showRegenerateModal true). Ensure showTokenModal($token->plainTextToken) and setting showRegenerateModal = false only run after the transaction commits; reference the regenerateToken method, tokens()->delete(), createToken('agent'), showRegenerateModal and showTokenModal to locate and update the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/Livewire/Agent/Edit.php`:
- Around line 54-67: Wrap the current tokens()->delete() and
createToken('agent') calls in a single database transaction inside
regenerateToken() so both operations are atomic: begin a DB::transaction,
perform $this->form->agent->tokens()->delete() then
$this->form->agent->createToken('agent'), commit only on success, and on
exception roll back and handle the error (log and surface an error message /
keep $this->showRegenerateModal true). Ensure
showTokenModal($token->plainTextToken) and setting showRegenerateModal = false
only run after the transaction commits; reference the regenerateToken method,
tokens()->delete(), createToken('agent'), showRegenerateModal and showTokenModal
to locate and update the logic.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/Livewire/Agent/Edit.phpapp/Livewire/Concerns/HasAgentToken.phpapp/Livewire/Forms/DatabaseServerForm.phpresources/views/livewire/agent/edit.blade.phpresources/views/livewire/agent/index.blade.phpresources/views/livewire/database-server/_form.blade.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Livewire/Concerns/HasAgentToken.php
- resources/views/livewire/agent/edit.blade.php
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-app-image
- GitHub Check: ci
🧰 Additional context used
📓 Path-based instructions (4)
app/Livewire/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/Livewire/**/*.php: Use Livewire class-based components for all interactive pages. Authentication flows use plain Blade templates rendered by Laravel Fortify, not Livewire components.
Public properties in Livewire components are automatically bound to views. Use #[Validate] attributes or Form objects for validation. Call $this->validate() before processing data.
Use Session::flash() for one-time messages in Livewire components, shown via@if(session('success')). Return $this->redirect() with navigate: true for SPA-like navigation.
Files:
app/Livewire/Agent/Edit.phpapp/Livewire/Forms/DatabaseServerForm.php
app/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/**/*.php: Use constructor property promotion in PHP 8 __construct() methods. Do not allow empty __construct() methods with zero parameters unless the constructor is private.
Always use explicit return type declarations for methods and functions. Use appropriate PHP type hints for method parameters.
Always use curly braces for control structures, even for single-line bodies.
Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex. Add useful array shape type definitions in PHPDoc when appropriate.
Avoid DB::; prefer Model::query(). Generate code that leverages Laravel's ORM capabilities rather than bypassing them. Generate code that prevents N+1 query problems by using eager loading.
Use Laravel's built-in authentication and authorization features (gates, policies, Sanctum, etc.).
Use environment variables only in configuration files - never use the env() function directly outside of config files. Always use config('key'), not env('KEY').
Use Laravel's query builder for very complex database operations instead of raw SQL queries.
Run 'vendor/bin/pint --dirty' before finalizing changes to ensure code matches the project's expected style. Use 'pint' to fix issues, not 'pint --test'.
Files:
app/Livewire/Agent/Edit.phpapp/Livewire/Forms/DatabaseServerForm.php
resources/views/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
resources/views/**/*.blade.php: All UI components use Mary UI (built on daisyUI) without prefixes (e.g., , , ). Use Heroicons for icons (e.g., icon="o-user" for outline, icon="s-user" for solid).
Modal pattern in Mary UI: Add boolean property to component class and use wire:model in blade file.
Select component pattern in Mary UI: Use :options prop with array format [['id' => 'value', 'name' => 'Label']].
Alert component pattern in Mary UI: Use class="alert-success", class="alert-error", etc. format.
In Blade component attributes, use :attr binding (dynamic syntax) instead of {{ }} interpolation when passing translated strings. The {{ }} syntax double-encodes special characters.
When generating links to other pages, prefer named routes and the route() function.
Files:
resources/views/livewire/agent/index.blade.phpresources/views/livewire/database-server/_form.blade.php
resources/views/livewire/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
Blade files should contain only view markup; all PHP logic should be in Livewire component classes.
Files:
resources/views/livewire/agent/index.blade.phpresources/views/livewire/database-server/_form.blade.php
🧠 Learnings (14)
📓 Common learnings
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Http/Controllers/Api/V1/AgentController.php:175-206
Timestamp: 2026-02-25T10:46:42.914Z
Learning: In the agent job processing flow (app/Http/Controllers/Api/V1/AgentController.php), state updates in endpoints like ack() and fail() are not wrapped in DB::transaction(). The codebase relies on the scheduled agent:recover-leases command to handle recovery of incomplete or failed jobs, rather than using strict transactional consistency for each endpoint.
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Livewire/**/*.php : Use Livewire class-based components for all interactive pages. Authentication flows use plain Blade templates rendered by Laravel Fortify, not Livewire components.
Applied to files:
app/Livewire/Agent/Edit.phpresources/views/livewire/agent/index.blade.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to resources/views/**/*.blade.php : Modal pattern in Mary UI: Add boolean property to component class and use wire:model in blade file.
Applied to files:
app/Livewire/Agent/Edit.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Livewire/**/*.php : Public properties in Livewire components are automatically bound to views. Use #[Validate] attributes or Form objects for validation. Call $this->validate() before processing data.
Applied to files:
app/Livewire/Agent/Edit.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to routes/*.php : All routes use Route::livewire() for full-page Livewire components (e.g., Route::livewire('database-servers', \App\Livewire\DatabaseServer\Index::class)). Authentication routes use plain Blade views rendered by Laravel Fortify.
Applied to files:
app/Livewire/Agent/Edit.php
📚 Learning: 2026-02-18T09:45:52.485Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 116
File: app/Livewire/DatabaseServer/ConnectionStatus.php:18-18
Timestamp: 2026-02-18T09:45:52.485Z
Learning: In Livewire components, Eloquent model properties (e.g., public DatabaseServer $server) are automatically locked by the framework to prevent client-side ID tampering. The #[Locked] attribute is only needed for scalar properties (int, string, bool, etc.) that require protection from client-side mutation. Apply this guidance to all Livewire PHP components; use #[Locked] only on primitive properties that you want to shield from client manipulation, and rely on automatic locking for Eloquent model properties.
Applied to files:
app/Livewire/Agent/Edit.phpapp/Livewire/Forms/DatabaseServerForm.php
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
app/Livewire/Agent/Edit.phpresources/views/livewire/agent/index.blade.phpapp/Livewire/Forms/DatabaseServerForm.phpresources/views/livewire/database-server/_form.blade.php
📚 Learning: 2026-01-30T22:27:46.107Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 61
File: resources/views/livewire/volume/connectors/s3-config.blade.php:1-13
Timestamp: 2026-01-30T22:27:46.107Z
Learning: In Blade template files (any .blade.php) within the databasement project, allow using alert-info for informational content inside <x-alert> components. The guideline that permits alert-success and alert-error does not exclude using alert-info for informational purposes. Apply this consistently to all Blade components that render alerts; ensure semantic usage and accessibility.
Applied to files:
resources/views/livewire/agent/index.blade.phpresources/views/livewire/database-server/_form.blade.php
📚 Learning: 2026-02-06T10:34:43.585Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 75
File: resources/views/livewire/backup-job/_filters.blade.php:36-40
Timestamp: 2026-02-06T10:34:43.585Z
Learning: In Blade template files, when creating compact inline filter controls, prefer using native <input type="checkbox"> elements with daisyUI classes (e.g., checkbox checkbox-warning checkbox-xs) over the Mary UI <x-checkbox> component. The <x-checkbox> component adds wrapper markup (e.g., <div><fieldset><label> with gap-3) that can break tight inline flex layouts. Use the native input approach for compact inline controls, but reserve <x-checkbox> for form fields that require labels, hints, and errors.
Applied to files:
resources/views/livewire/agent/index.blade.phpresources/views/livewire/database-server/_form.blade.php
📚 Learning: 2026-02-25T10:48:46.954Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Services/Agent/AgentJobPayloadBuilder.php:63-76
Timestamp: 2026-02-25T10:48:46.954Z
Learning: In app/Services/Agent/AgentJobPayloadBuilder.php and similar service classes, prefer fail-fast error handling over defensive null-safe operators when upstream callers (e.g., TriggerBackupAction, RunScheduledBackups) guarantee certain relationships exist. For example, in resolveBackupPath(), $server->backup is intentionally accessed without the null-safe operator because backup existence is enforced at call sites; using the null-safe operator would hide bugs that violate this invariant.
Applied to files:
app/Livewire/Forms/DatabaseServerForm.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Services/Backup/Databases/*.php : All database type implementations must implement DatabaseInterface with methods: setConfig, dump, restore, prepareForRestore, listDatabases, testConnection. Types without PDO support (e.g., Redis) must throw in buildDsn() and handle connection testing via CLI.
Applied to files:
app/Livewire/Forms/DatabaseServerForm.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Services/Backup/Databases/DatabaseProvider.php : DatabaseProvider centralizes type dispatch via make() method, so BackupTask, RestoreTask, and connection testing require no changes when adding new database types.
Applied to files:
app/Livewire/Forms/DatabaseServerForm.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Services/Backup/BackupJobFactory.php : Types that backup the whole instance (e.g., Redis, SQLite) should short-circuit in BackupJobFactory.createSnapshots() to create a single snapshot instead of per-database snapshots.
Applied to files:
app/Livewire/Forms/DatabaseServerForm.php
📚 Learning: 2026-02-24T10:43:22.287Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 136
File: app/Models/DatabaseServer.php:250-258
Timestamp: 2026-02-24T10:43:22.287Z
Learning: Pattern mode (database_selection_mode = 'pattern') is not available for SQLite or Redis database servers. The UI conditionally hides the segmented control for these types in resources/views/livewire/database-server/_form.blade.php (line 242: !$form->isSqlite() && !$form->isRedis()).
Applied to files:
resources/views/livewire/database-server/_form.blade.php
🧬 Code graph analysis (2)
app/Livewire/Agent/Edit.php (5)
app/Models/Agent.php (1)
Agent(38-79)app/Livewire/Forms/AgentForm.php (3)
AgentForm(8-47)setAgent(24-28)update(39-46)app/Livewire/Concerns/HandlesDemoMode.php (1)
abortIfDemoMode(7-17)app/Policies/AgentPolicy.php (3)
update(41-44)delete(50-53)view(23-26)app/Livewire/Concerns/HasAgentToken.php (1)
showTokenModal(20-41)
app/Livewire/Forms/DatabaseServerForm.php (2)
app/Models/Agent.php (1)
Agent(38-79)app/Models/DatabaseServer.php (1)
agent(120-123)
🔇 Additional comments (13)
app/Livewire/Agent/Edit.php (4)
1-26: LGTM! Well-structured Livewire component.The component properly uses Form objects for validation (AgentForm), follows Livewire conventions with typed public properties, and correctly applies traits for reusable functionality.
27-32: LGTM!Authorization check on mount ensures the user can update the agent before initializing the form.
34-47: LGTM!The save method follows guidelines: checks demo mode first, authorizes the action, delegates update logic to the form object (which handles validation internally), uses
session()->flash()for status messages, and redirects withnavigate: true.
69-72: LGTM!Render method correctly returns the view with explicit
Viewreturn type.resources/views/livewire/agent/index.blade.php (1)
1-139: LGTM!Well-structured agent index view with proper Mary UI components, permission checks, responsive design, and a thoughtful delete confirmation modal that warns about associated database servers.
app/Livewire/Forms/DatabaseServerForm.php (4)
79-81: LGTM!Agent-related properties are properly typed with nullable string for
agent_id(ULID) and boolean foruse_agenttoggle.
204-214: LGTM!The
updatedUseAgent()lifecycle hook correctly clearsagent_idwhen the toggle is disabled and resets the connection test state, ensuring UI consistency.
505-530: LGTM!Agent-related methods follow established patterns in this form class.
getAgentOptions()properly uses Eloquent with correct PHPDoc annotations, andhasAgent()/getSelectedAgent()provide clean helper accessors.
590-590: LGTM!Validation rule
'nullable|exists:agents,id'correctly validates the optional agent relationship.resources/views/livewire/database-server/_form.blade.php (4)
33-90: LGTM!Clean agent selection UI implementation with proper conditionals. The toggle pattern follows Mary UI conventions, agent options are fetched once, and the status display correctly handles all three states (online/offline/never connected) with appropriate visual indicators.
215-285: LGTM!Well-handled connection test gating for agent-managed servers. The informative alert explains that agents will test connectivity during backups, avoiding user confusion when the test button isn't available.
291-306: LGTM!Form sections correctly gated on
$form->hasAgent()alongside existing conditions, ensuring the backup configuration UI is accessible when an agent is assigned without requiring a direct connection test.
451-455: LGTM!Smart differentiation in the pattern preview alert: uses
alert-infowith an informational icon for agent-managed servers (where preview isn't possible), andalert-warningfor non-agent cases where the user should test the connection first.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (8)
resources/views/livewire/backup-job/_logs-modal.blade.php (1)
157-157: Move$hasDetailscomputation into the Livewire component.Line 157 adds decision logic in the view; please precompute this (e.g.,
has_details) in the component and keep this Blade file markup-only.As per coding guidelines,
resources/views/livewire/**/*.blade.php: “Blade files should contain only view markup; all PHP logic should be in Livewire component classes.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/backup-job/_logs-modal.blade.php` at line 157, Move the `$hasDetails` decision into the Livewire component: compute a boolean (e.g., has_details) in the component class using the same logic ($isCommand || !empty($log['context']) || $isError) and either add it as a property on each log (e.g., $log['has_details']) or expose it as a variable for the view, then update the Blade to use that precomputed value instead of evaluating `$isCommand || !empty($log['context']) || $isError`; reference the existing symbols `$hasDetails`, `$isCommand`, `$log['context']`, and `$isError` when moving the logic and ensure the Blade remains markup-only.tests/Feature/DatabaseServer/ConnectionStatusTest.php (2)
34-50: Lock in the integration behavior by assertingDatabaseProvideris not called.Since these scenarios validate agent heartbeat status, explicitly asserting no direct DB connection test call will better protect the agent-managed path from regressions.
🧪 Suggested assertion hardening
test('shows agent online when agent has recent heartbeat', function () { $user = User::factory()->create(); $agent = Agent::factory()->create(['last_heartbeat_at' => now()]); $server = DatabaseServer::factory()->create(['agent_id' => $agent->id]); + $this->mock(DatabaseProvider::class, function ($mock) { + $mock->shouldNotReceive('testConnectionForServer'); + }); + Livewire::withoutLazyLoading() ->actingAs($user) ->test(ConnectionStatus::class, ['server' => $server]) ->assertSeeHtml('bg-success') ->assertSee('Agent online'); }); test('shows agent offline when agent has no recent heartbeat', function () { $user = User::factory()->create(); $agent = Agent::factory()->create(['last_heartbeat_at' => now()->subMinutes(5)]); $server = DatabaseServer::factory()->create(['agent_id' => $agent->id]); + $this->mock(DatabaseProvider::class, function ($mock) { + $mock->shouldNotReceive('testConnectionForServer'); + }); + Livewire::withoutLazyLoading() ->actingAs($user) ->test(ConnectionStatus::class, ['server' => $server]) ->assertSeeHtml('bg-error') ->assertSee('Agent offline'); });As per coding guidelines, "Focus on business logic, authorization, edge cases in your code, and integration points. Mock external API services and third-party libraries (AWS SDK, S3 client, etc.), but do NOT mock Model/ORM methods or simple utility functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/DatabaseServer/ConnectionStatusTest.php` around lines 34 - 50, Add an assertion that the DatabaseProvider integration is not invoked when the Agent heartbeat determines status: create a mock/spy for DatabaseProvider (the service/class that exposes the connection test method, e.g., DatabaseProvider::testConnection or the provider binding used by ConnectionStatus) before rendering the Livewire component in the tests for ConnectionStatus, inject or bind that mock into the container, and assert the mock's connection test method was never called (e.g., ->shouldNotHaveReceived('testConnection') or ->assertNotCalled) in both the "agent online" and "agent offline" test cases to lock in the agent-managed path.
31-44: Prefer Agent factory states for heartbeat setup.These tests read cleaner and stay aligned with shared factory conventions if they use semantic states (
online/offline) instead of manuallast_heartbeat_atvalues.♻️ Suggested cleanup
- $agent = Agent::factory()->create(['last_heartbeat_at' => now()]); + $agent = Agent::factory()->online()->create(); ... - $agent = Agent::factory()->create(['last_heartbeat_at' => now()->subMinutes(5)]); + $agent = Agent::factory()->offline()->create();As per coding guidelines, "When creating models for tests, use the factories for the models. Check if the factory has custom states that can be used before manually setting up the model."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/DatabaseServer/ConnectionStatusTest.php` around lines 31 - 44, Replace manual last_heartbeat_at setup with the Agent factory states: instead of Agent::factory()->create(['last_heartbeat_at' => now()]) use Agent::factory()->online()->create() and instead of Agent::factory()->create(['last_heartbeat_at' => now()->subMinutes(5)]) use Agent::factory()->offline()->create(); update the two tests that instantiate Agent (the ones using Agent::factory() together with DatabaseServer::factory() and testing ConnectionStatus::class) to use the semantic online/offline states so they follow factory conventions and remain clearer.app/Enums/DatabaseType.php (1)
57-61: Move this rationale out of inline comments.The logic is clear enough to be self-documenting; keep code comment-free here and, if needed, document the behavior in method PHPDoc.
As per coding guidelines: "Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Enums/DatabaseType.php` around lines 57 - 61, Remove the inline explanatory comments around the MySQL host handling and instead document the rationale in the surrounding PHPDoc (e.g., on the DatabaseType enum method that contains the conditional where "$this === self::MYSQL && $host === 'localhost'") so the code remains comment-free and self-documenting; update the method PHPDoc to state that localhost is rewritten to 127.0.0.1 to force TCP for MySQL, then delete the two inline comment lines above the conditional.database/factories/AgentJobFactory.php (1)
68-89: UseDatabaseSelectionModeenum in discovery payload instead of string literal.At Line 83,
'all'should come from the enum to avoid drift.♻️ Suggested update
+use App\Enums\DatabaseSelectionMode; @@ - 'selection_mode' => 'all', + 'selection_mode' => DatabaseSelectionMode::All->value,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/factories/AgentJobFactory.php` around lines 68 - 89, Replace the hardcoded 'all' string in AgentJobFactory::discover() payload with the DatabaseSelectionMode enum value; locate the 'selection_mode' => 'all' entry in the discover() state and change it to DatabaseSelectionMode::ALL (or DatabaseSelectionMode::ALL->value if the enum is backed) so the payload uses the enum constant instead of a literal string.resources/views/livewire/database-server/_form.blade.php (1)
35-36: Move inline PHP logic from Blade into the Livewire class/form object.These computed values (
$agentOptions,$selectedAgent,$modes) should be exposed from the component/form as properties/methods to keep the view markup-only.As per coding guidelines, "resources/views/livewire/**/*.blade.php: Blade files should contain only view markup; all PHP logic should be in Livewire component classes."
Also applies to: 67-67, 317-323
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/database-server/_form.blade.php` around lines 35 - 36, The Blade view contains inline PHP computing $agentOptions, $selectedAgent, and $modes (calls like $form->getAgentOptions() and $form->hasAgent()) which should be moved into the Livewire form/component class; add public properties or accessor methods (e.g., getAgentOptions(), getSelectedAgent(), getModes() or public $agentOptions, $selectedAgent, $modes) on the Livewire form/component that compute and return these values, update the view to reference those properties (e.g., $form->agentOptions, $form->selectedAgent, $form->modes) and remove all `@php` blocks in the Blade file (also change the other occurrences mentioned around lines 67 and 317-323 to use the new component properties).tests/Feature/DatabaseServer/CreateTest.php (1)
214-215: Prefer factories over direct model creation in these tests.These new test setups use
Volume::create(...); switch toVolume::factory()->create(...)for consistency and future-proofing with model defaults/states.♻️ Suggested refactor
- $localVolume = Volume::create(['name' => 'Local Vol', 'type' => 'local', 'config' => ['path' => '/backups']]); + $localVolume = Volume::factory()->create(['name' => 'Local Vol', 'type' => 'local', 'config' => ['path' => '/backups']]); - $s3Volume = Volume::create(['name' => 'S3 Vol', 'type' => 's3', 'config' => ['bucket' => 'test']]); + $s3Volume = Volume::factory()->create(['name' => 'S3 Vol', 'type' => 's3', 'config' => ['bucket' => 'test']]);As per coding guidelines, "When creating models for tests, use the factories for the models. Check if the factory has custom states that can be used before manually setting up the model."
Also applies to: 233-234, 249-250, 260-260, 272-272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/DatabaseServer/CreateTest.php` around lines 214 - 215, Replace direct model creation calls to Volume::create(...) with factory-based creation using Volume::factory()->create(...) so tests use the model factory and respect defaults/states; update each occurrence (the two instances shown and the other occurrences referenced in the comment) to call Volume::factory()->create with the same attributes (name, type, config) or, when available, use any custom factory states on the Volume factory instead of manually setting attributes. Ensure you update all spots mentioned (the additional Volume::create calls referenced) to keep test setup consistent.app/Console/Commands/RunScheduledBackups.php (1)
67-110: Extract shared agent-dispatch logic to avoid manual/scheduled drift.This block now mirrors the same discovery/backup branching in
app/Services/Backup/TriggerBackupAction.php. Consolidating into one dispatcher service will prevent behavior drift and reduce dual-fix overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Console/Commands/RunScheduledBackups.php` around lines 67 - 110, The dispatch logic in RunScheduledBackups::dispatch duplicates discovery vs backup branching already implemented in app/Services/Backup/TriggerBackupAction.php; extract this into a single dispatcher service (e.g., BackupDispatcher or DispatchBackupService) that accepts a Backup/DatabaseServer along with BackupJobFactory and AgentJobPayloadBuilder and encapsulates the AgentJob::create(...) (TYPE_DISCOVER, TYPE_BACKUP, STATUS_PENDING) and ProcessBackupJob::dispatch(...) flows using payloadBuilder->buildDiscovery(...) and payloadBuilder->build(...); then replace the body of RunScheduledBackups::dispatch to call the new service (e.g., $this->backupDispatcher->dispatchForBackup($backup)) so both scheduled and manual triggers reuse the same implementation and avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Http/Controllers/Api/V1/AgentController.php`:
- Around line 174-190: The ack() handling in AgentController dereferences
$agentJob->snapshot unconditionally which will throw for non-backup jobs (e.g.,
discovery jobs); update ack() to first guard that $agentJob->type (or a helper
like $agentJob->isBackup()) indicates a backup job before accessing
$agentJob->snapshot, and return an appropriate 4xx response when it’s not a
backup; only perform snapshot->update(), snapshot->markCompleted(), and the
backup job updates (backupJob->update(), backupJob->markCompleted()) inside that
guarded branch while still calling $agentJob->markCompleted() as appropriate.
In `@app/Http/Middleware/ThrottleFailedAgentAuth.php`:
- Around line 22-30: The current early RateLimiter::tooManyAttempts($key, 10)
check runs before authentication and will return 429 for valid requests after
prior failures; to fix, defer the throttle decision until after the request is
processed: call $response = $next($request) first, then if
$response->getStatusCode() is 401 or 403 call RateLimiter::hit($key, 300) and
after that check RateLimiter::tooManyAttempts($key, 10) and return the 429
response only when the threshold is exceeded. Update the ThrottleFailedAgentAuth
middleware to use this post-auth check flow so valid agent tokens are not
blocked.
In `@app/Livewire/Forms/DatabaseServerForm.php`:
- Around line 606-607: The form allows sending an agent_id even when use_agent
is false; update the validation in DatabaseServerForm so agent_id cannot be set
unless use_agent is true and ensure runtime checks use validated data: change
the 'agent_id' rule (where rules() returns 'agent_id' =>
'nullable|exists:agents,id') to include a prohibition like
'prohibited_unless:use_agent,true' (or 'prohibited_if:use_agent,false' /
'required_if:use_agent,true' depending on desired behavior), and modify the
later enforcement logic around use_agent/agent handling (the block that
currently reads $this->use_agent at lines ~623-625) to consult the validated
input (e.g. $validated['use_agent'] or validated data) instead of raw UI state
so a crafted payload cannot desynchronize use_agent and agent_id.
In `@app/Models/AgentJob.php`:
- Around line 122-130: The claim() method is doing a non-atomic in-memory
increment of attempts which can be lost by concurrent claimers; change to a
conflict-safe update by performing an atomic DB operation: use the
AgentJob::where('id', $this->id)->where('attempts',
$this->attempts)->update(...) (or a DB transaction with SELECT ... FOR UPDATE)
to set agent_id, status (self::STATUS_CLAIMED), lease_expires_at, claimed_at and
attempts = attempts + 1, then check the affected-row count and handle
zero-updates (retry or surface failure) so stale-write races cannot overwrite
concurrent claimers. Ensure you reference and update the same fields used now
(agent_id, attempts, lease_expires_at, claimed_at, status) and make claim()
return or throw when the conditional update fails.
In `@app/Services/Backup/TriggerBackupAction.php`:
- Around line 85-95: dispatchToAgent currently creates multiple AgentJob rows in
a loop via AgentJob::create and payloadBuilder->build without a transaction, so
failures yield partial writes; wrap the entire creation loop in a single DB
transaction using the AgentJob model's connection (e.g.,
DB::connection(AgentJob::getConnectionName())->transaction(...)) so either all
AgentJob records for the snapshots are committed or none are, building payloads
and calling AgentJob::create inside that transaction and rethrowing any
exceptions to preserve original error behavior.
In `@app/Support/helpers.php`:
- Around line 3-5: The helper agent_mode() is reading env() directly; create a
config entry (e.g., add an agent.php config with an 'enabled' key that sources
the env value like env('DATABASEMENT_URL') or a dedicated AGENT_ENABLED) and
update agent_mode() to return (bool) config('agent.enabled') instead of
env(...); ensure the new config key is used everywhere and remove env(...) usage
from app/Support/helpers.php so the helper only uses config('agent.enabled') and
casts to bool.
In `@config/database.php`:
- Line 19: agent_mode() currently calls env() from app/Support/helpers.php which
violates the rule to only call env() in config files; move the env() call into a
config entry (e.g., add a new boolean like 'agent_mode' or 'agent.enabled' using
env('AGENT_MODE', false) in a config file such as config/app.php), update
agent_mode() to return config('app.agent_mode') (or the chosen key) instead of
env(), and then leave config/database.php's line ('default' => agent_mode() ?
'agent' : env('DB_CONNECTION', 'sqlite')) as-is (or optionally reference the new
config key directly); apply the same pattern to config/queue.php,
config/session.php, and config/cache.php so no helper calls env() outside config
files.
In `@Makefile`:
- Around line 41-46: The create-bucket recipe currently fails if the bucket
already exists; change the docker run invocation that calls "amazon/aws-cli ...
s3 mb s3://$(or $(BUCKET),test-bucket)" to first check for the bucket and only
create it when missing (for example by running an existence check like "aws s3
ls s3://<bucket>" or "aws s3api head-bucket --bucket <bucket>" inside the same
docker run and falling back to "aws s3 mb s3://<bucket>" only on failure).
Update the command that builds the container (the docker run line in the
Makefile) so it performs the existence check and then conditionally runs the s3
mb to make the operation idempotent.
In `@phpstan.neon`:
- Around line 19-21: The suppression in phpstan.neon hides use of env() in
app/Support/helpers.php; open app/Support/helpers.php, locate any direct calls
to env(...) (and functions that wrap it) and refactor them to read from
config('...') instead, moving any env(...) usages into an appropriate
config/*.php file where you set config keys from env(...); once helpers.php uses
config(...) only, remove the corresponding ignore block from phpstan.neon so the
guardrail stays enabled.
In `@tests/Feature/Services/Backup/TriggerBackupActionTest.php`:
- Around line 123-128: The test uses AgentJob::where('database_server_id',
$server->id)->first() which will hide duplicate discovery-job regressions;
change this to select the unique discovery job and fail on duplicates by
filtering by type and using sole(), e.g. query AgentJob where database_server_id
= $server->id and type = AgentJob::TYPE_DISCOVER then call sole() to retrieve
the single record, and keep the existing assertions (snapshot_id null,
payload['type']=='discover', payload['selection_mode']=='all') against that sole
result.
- Around line 168-172: The test is relying on DB return order by indexing
$agentJobs[0]; change the assertion to be order-independent by either adding an
explicit ordering to the query (use AgentJob::where('database_server_id',
$server->id)->orderBy('id')->get() and then assert types/snapshot_id) or,
better, assert collection-level invariants using collection helpers (e.g.,
$agentJobs = AgentJob::where(...)->get(); expect($agentJobs->where('type',
AgentJob::TYPE_BACKUP))->toHaveCount(2) and
expect($agentJobs->pluck('snapshot_id')->filter()->count())->toBe(2) or use
$agentJobs->every(fn($j)=> $j->type === AgentJob::TYPE_BACKUP &&
!is_null($j->snapshot_id)) so the test no longer depends on ordering).
---
Nitpick comments:
In `@app/Console/Commands/RunScheduledBackups.php`:
- Around line 67-110: The dispatch logic in RunScheduledBackups::dispatch
duplicates discovery vs backup branching already implemented in
app/Services/Backup/TriggerBackupAction.php; extract this into a single
dispatcher service (e.g., BackupDispatcher or DispatchBackupService) that
accepts a Backup/DatabaseServer along with BackupJobFactory and
AgentJobPayloadBuilder and encapsulates the AgentJob::create(...)
(TYPE_DISCOVER, TYPE_BACKUP, STATUS_PENDING) and ProcessBackupJob::dispatch(...)
flows using payloadBuilder->buildDiscovery(...) and payloadBuilder->build(...);
then replace the body of RunScheduledBackups::dispatch to call the new service
(e.g., $this->backupDispatcher->dispatchForBackup($backup)) so both scheduled
and manual triggers reuse the same implementation and avoid drift.
In `@app/Enums/DatabaseType.php`:
- Around line 57-61: Remove the inline explanatory comments around the MySQL
host handling and instead document the rationale in the surrounding PHPDoc
(e.g., on the DatabaseType enum method that contains the conditional where
"$this === self::MYSQL && $host === 'localhost'") so the code remains
comment-free and self-documenting; update the method PHPDoc to state that
localhost is rewritten to 127.0.0.1 to force TCP for MySQL, then delete the two
inline comment lines above the conditional.
In `@database/factories/AgentJobFactory.php`:
- Around line 68-89: Replace the hardcoded 'all' string in
AgentJobFactory::discover() payload with the DatabaseSelectionMode enum value;
locate the 'selection_mode' => 'all' entry in the discover() state and change it
to DatabaseSelectionMode::ALL (or DatabaseSelectionMode::ALL->value if the enum
is backed) so the payload uses the enum constant instead of a literal string.
In `@resources/views/livewire/backup-job/_logs-modal.blade.php`:
- Line 157: Move the `$hasDetails` decision into the Livewire component: compute
a boolean (e.g., has_details) in the component class using the same logic
($isCommand || !empty($log['context']) || $isError) and either add it as a
property on each log (e.g., $log['has_details']) or expose it as a variable for
the view, then update the Blade to use that precomputed value instead of
evaluating `$isCommand || !empty($log['context']) || $isError`; reference the
existing symbols `$hasDetails`, `$isCommand`, `$log['context']`, and `$isError`
when moving the logic and ensure the Blade remains markup-only.
In `@resources/views/livewire/database-server/_form.blade.php`:
- Around line 35-36: The Blade view contains inline PHP computing $agentOptions,
$selectedAgent, and $modes (calls like $form->getAgentOptions() and
$form->hasAgent()) which should be moved into the Livewire form/component class;
add public properties or accessor methods (e.g., getAgentOptions(),
getSelectedAgent(), getModes() or public $agentOptions, $selectedAgent, $modes)
on the Livewire form/component that compute and return these values, update the
view to reference those properties (e.g., $form->agentOptions,
$form->selectedAgent, $form->modes) and remove all `@php` blocks in the Blade file
(also change the other occurrences mentioned around lines 67 and 317-323 to use
the new component properties).
In `@tests/Feature/DatabaseServer/ConnectionStatusTest.php`:
- Around line 34-50: Add an assertion that the DatabaseProvider integration is
not invoked when the Agent heartbeat determines status: create a mock/spy for
DatabaseProvider (the service/class that exposes the connection test method,
e.g., DatabaseProvider::testConnection or the provider binding used by
ConnectionStatus) before rendering the Livewire component in the tests for
ConnectionStatus, inject or bind that mock into the container, and assert the
mock's connection test method was never called (e.g.,
->shouldNotHaveReceived('testConnection') or ->assertNotCalled) in both the
"agent online" and "agent offline" test cases to lock in the agent-managed path.
- Around line 31-44: Replace manual last_heartbeat_at setup with the Agent
factory states: instead of Agent::factory()->create(['last_heartbeat_at' =>
now()]) use Agent::factory()->online()->create() and instead of
Agent::factory()->create(['last_heartbeat_at' => now()->subMinutes(5)]) use
Agent::factory()->offline()->create(); update the two tests that instantiate
Agent (the ones using Agent::factory() together with DatabaseServer::factory()
and testing ConnectionStatus::class) to use the semantic online/offline states
so they follow factory conventions and remain clearer.
In `@tests/Feature/DatabaseServer/CreateTest.php`:
- Around line 214-215: Replace direct model creation calls to
Volume::create(...) with factory-based creation using
Volume::factory()->create(...) so tests use the model factory and respect
defaults/states; update each occurrence (the two instances shown and the other
occurrences referenced in the comment) to call Volume::factory()->create with
the same attributes (name, type, config) or, when available, use any custom
factory states on the Volume factory instead of manually setting attributes.
Ensure you update all spots mentioned (the additional Volume::create calls
referenced) to keep test setup consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
CLAUDE.mdMakefileapp/Console/Commands/AgentRunCommand.phpapp/Console/Commands/RunScheduledBackups.phpapp/Enums/DatabaseSelectionMode.phpapp/Enums/DatabaseType.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Http/Middleware/ThrottleFailedAgentAuth.phpapp/Livewire/Concerns/HasAgentToken.phpapp/Livewire/DatabaseServer/ConnectionStatus.phpapp/Livewire/Forms/DatabaseServerForm.phpapp/Models/AgentJob.phpapp/Models/DatabaseServer.phpapp/Services/Agent/AgentApiClient.phpapp/Services/Agent/AgentAuthenticationException.phpapp/Services/Agent/AgentJobPayloadBuilder.phpapp/Services/Backup/BackupJobFactory.phpapp/Services/Backup/TriggerBackupAction.phpapp/Support/helpers.phpbootstrap/app.phpcomposer.jsonconfig/agent.phpconfig/cache.phpconfig/database.phpconfig/queue.phpconfig/session.phpdatabase/factories/AgentJobFactory.phpdatabase/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.phpdocker/php/scripts/start.shphpstan.neonresources/views/livewire/agent/_token-modal.blade.phpresources/views/livewire/backup-job/_logs-modal.blade.phpresources/views/livewire/database-server/_form.blade.phpresources/views/livewire/database-server/index.blade.phproutes/api.phptests/Feature/Agent/AgentApiTest.phptests/Feature/Console/AgentRunCommandTest.phptests/Feature/Console/RunScheduledBackupsTest.phptests/Feature/DatabaseServer/ConnectionStatusTest.phptests/Feature/DatabaseServer/CreateTest.phptests/Feature/Services/Backup/TriggerBackupActionTest.php
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/Feature/Console/RunScheduledBackupsTest.php
- app/Livewire/Concerns/HasAgentToken.php
- app/Models/DatabaseServer.php
- tests/Feature/Console/AgentRunCommandTest.php
- resources/views/livewire/agent/_token-modal.blade.php
- routes/api.php
- app/Services/Agent/AgentJobPayloadBuilder.php
- config/agent.php
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-base-image
- GitHub Check: ci
🧰 Additional context used
📓 Path-based instructions (10)
app/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/**/*.php: Use constructor property promotion in PHP 8 __construct() methods. Do not allow empty __construct() methods with zero parameters unless the constructor is private.
Always use explicit return type declarations for methods and functions. Use appropriate PHP type hints for method parameters.
Always use curly braces for control structures, even for single-line bodies.
Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex. Add useful array shape type definitions in PHPDoc when appropriate.
Avoid DB::; prefer Model::query(). Generate code that leverages Laravel's ORM capabilities rather than bypassing them. Generate code that prevents N+1 query problems by using eager loading.
Use Laravel's built-in authentication and authorization features (gates, policies, Sanctum, etc.).
Use environment variables only in configuration files - never use the env() function directly outside of config files. Always use config('key'), not env('KEY').
Use Laravel's query builder for very complex database operations instead of raw SQL queries.
Run 'vendor/bin/pint --dirty' before finalizing changes to ensure code matches the project's expected style. Use 'pint' to fix issues, not 'pint --test'.
Files:
app/Http/Middleware/ThrottleFailedAgentAuth.phpapp/Support/helpers.phpapp/Enums/DatabaseSelectionMode.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Services/Agent/AgentApiClient.phpapp/Livewire/DatabaseServer/ConnectionStatus.phpapp/Services/Agent/AgentAuthenticationException.phpapp/Models/AgentJob.phpapp/Console/Commands/RunScheduledBackups.phpapp/Console/Commands/AgentRunCommand.phpapp/Services/Backup/TriggerBackupAction.phpapp/Services/Backup/BackupJobFactory.phpapp/Enums/DatabaseType.phpapp/Livewire/Forms/DatabaseServerForm.php
app/Enums/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Enum keys should be TitleCase (e.g., FavoritePerson, BestLake, Monthly).
Files:
app/Enums/DatabaseSelectionMode.phpapp/Enums/DatabaseType.php
app/Livewire/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/Livewire/**/*.php: Use Livewire class-based components for all interactive pages. Authentication flows use plain Blade templates rendered by Laravel Fortify, not Livewire components.
Public properties in Livewire components are automatically bound to views. Use #[Validate] attributes or Form objects for validation. Call $this->validate() before processing data.
Use Session::flash() for one-time messages in Livewire components, shown via@if(session('success')). Return $this->redirect() with navigate: true for SPA-like navigation.
Files:
app/Livewire/DatabaseServer/ConnectionStatus.phpapp/Livewire/Forms/DatabaseServerForm.php
tests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.php: Do not test framework internals, form validation rules (required, max:255, etc.), Eloquent relationships (hasMany, belongsTo), Eloquent cascades (onDelete('cascade')), session flash messages, redirect responses, or multiple variations of the same thing. Focus on business logic, authorization, edge cases in your code, and integration points.
Mock external API services and third-party libraries (AWS SDK, S3 client, etc.), but do NOT mock Model/ORM methods or simple utility functions.
When creating models for tests, use the factories for the models. Check if the factory has custom states that can be used before manually setting up the model.
Use Faker methods such as $this->faker->word() or fake()->randomDigit(). Follow existing conventions whether to use $this->faker or fake().
When creating tests, use 'php artisan make:test [options] {name}' to create a feature test, and pass --unit to create a unit test. Most tests should be feature tests.
Files:
tests/Feature/Agent/AgentApiTest.phptests/Feature/DatabaseServer/CreateTest.phptests/Feature/Services/Backup/TriggerBackupActionTest.phptests/Feature/DatabaseServer/ConnectionStatusTest.php
resources/views/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
resources/views/**/*.blade.php: All UI components use Mary UI (built on daisyUI) without prefixes (e.g., , , ). Use Heroicons for icons (e.g., icon="o-user" for outline, icon="s-user" for solid).
Modal pattern in Mary UI: Add boolean property to component class and use wire:model in blade file.
Select component pattern in Mary UI: Use :options prop with array format [['id' => 'value', 'name' => 'Label']].
Alert component pattern in Mary UI: Use class="alert-success", class="alert-error", etc. format.
In Blade component attributes, use :attr binding (dynamic syntax) instead of {{ }} interpolation when passing translated strings. The {{ }} syntax double-encodes special characters.
When generating links to other pages, prefer named routes and the route() function.
Files:
resources/views/livewire/database-server/index.blade.phpresources/views/livewire/backup-job/_logs-modal.blade.phpresources/views/livewire/database-server/_form.blade.php
resources/views/livewire/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
Blade files should contain only view markup; all PHP logic should be in Livewire component classes.
Files:
resources/views/livewire/database-server/index.blade.phpresources/views/livewire/backup-job/_logs-modal.blade.phpresources/views/livewire/database-server/_form.blade.php
app/Models/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/Models/**/*.php: Database models use ULIDs (Universally Unique Lexicographically Sortable Identifiers) for primary keys instead of auto-incrementing integers.
Always use proper Eloquent relationship methods with return type hints. Prefer relationship methods over raw queries or manual joins. Use Eloquent models before suggesting raw database queries.
Use casts() method on models rather than the $casts property. Follow existing conventions from other models.
Files:
app/Models/AgentJob.php
database/migrations/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying a column in a migration, the migration must include all of the attributes that were previously defined on the column. Otherwise, they will be dropped and lost.
Files:
database/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.php
app/Console/Commands/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Console commands in app/Console/Commands/ are automatically available and do not require manual registration.
Files:
app/Console/Commands/RunScheduledBackups.phpapp/Console/Commands/AgentRunCommand.php
app/Services/Backup/BackupJobFactory.php
📄 CodeRabbit inference engine (CLAUDE.md)
Types that backup the whole instance (e.g., Redis, SQLite) should short-circuit in BackupJobFactory.createSnapshots() to create a single snapshot instead of per-database snapshots.
Files:
app/Services/Backup/BackupJobFactory.php
🧠 Learnings (27)
📓 Common learnings
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Http/Controllers/Api/V1/AgentController.php:175-206
Timestamp: 2026-02-25T10:46:42.914Z
Learning: In the agent job processing flow (app/Http/Controllers/Api/V1/AgentController.php), state updates in endpoints like ack() and fail() are not wrapped in DB::transaction(). The codebase relies on the scheduled agent:recover-leases command to handle recovery of incomplete or failed jobs, rather than using strict transactional consistency for each endpoint.
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
app/Http/Middleware/ThrottleFailedAgentAuth.phpapp/Support/helpers.phpconfig/cache.phpapp/Enums/DatabaseSelectionMode.phpapp/Http/Controllers/Api/V1/AgentController.phpdatabase/factories/AgentJobFactory.phpapp/Services/Agent/AgentApiClient.phpapp/Livewire/DatabaseServer/ConnectionStatus.phpconfig/database.phptests/Feature/Agent/AgentApiTest.phpbootstrap/app.phptests/Feature/DatabaseServer/CreateTest.phpapp/Services/Agent/AgentAuthenticationException.phptests/Feature/Services/Backup/TriggerBackupActionTest.phpresources/views/livewire/database-server/index.blade.phpapp/Models/AgentJob.phpdatabase/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.phpconfig/queue.phpresources/views/livewire/backup-job/_logs-modal.blade.phptests/Feature/DatabaseServer/ConnectionStatusTest.phpapp/Console/Commands/RunScheduledBackups.phpconfig/session.phpapp/Console/Commands/AgentRunCommand.phpapp/Services/Backup/TriggerBackupAction.phpapp/Services/Backup/BackupJobFactory.phpapp/Enums/DatabaseType.phpresources/views/livewire/database-server/_form.blade.phpapp/Livewire/Forms/DatabaseServerForm.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Jobs/ProcessBackupJob.php : Backup workflow: User clicks 'Backup' → ProcessBackupJob dispatched to queue → Queue worker executes BackupTask service → Creates Snapshot record with status tracking. RestoreTask handles cross-server and same-server restores with appropriate SSL/connection handling for MySQL, MariaDB, and PostgreSQL.
Applied to files:
CLAUDE.mdapp/Http/Controllers/Api/V1/AgentController.phptests/Feature/Services/Backup/TriggerBackupActionTest.phpapp/Models/AgentJob.phpapp/Console/Commands/RunScheduledBackups.phpapp/Console/Commands/AgentRunCommand.phpapp/Services/Backup/TriggerBackupAction.phpapp/Services/Backup/BackupJobFactory.php
📚 Learning: 2026-02-25T10:46:42.914Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Http/Controllers/Api/V1/AgentController.php:175-206
Timestamp: 2026-02-25T10:46:42.914Z
Learning: In the agent job processing flow (app/Http/Controllers/Api/V1/AgentController.php), state updates in endpoints like ack() and fail() are not wrapped in DB::transaction(). The codebase relies on the scheduled agent:recover-leases command to handle recovery of incomplete or failed jobs, rather than using strict transactional consistency for each endpoint.
Applied to files:
CLAUDE.mdapp/Http/Controllers/Api/V1/AgentController.phpdatabase/factories/AgentJobFactory.phpapp/Services/Agent/AgentApiClient.phptests/Feature/Agent/AgentApiTest.phptests/Feature/Services/Backup/TriggerBackupActionTest.phpapp/Models/AgentJob.phpdatabase/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.phpapp/Console/Commands/RunScheduledBackups.phpapp/Console/Commands/AgentRunCommand.phpapp/Services/Backup/TriggerBackupAction.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Jobs/*.php : Use queued jobs for time-consuming operations with the ShouldQueue interface. Jobs should use the 'backups' queue and handle retries and timeouts appropriately.
Applied to files:
CLAUDE.mdapp/Http/Controllers/Api/V1/AgentController.phptests/Feature/Services/Backup/TriggerBackupActionTest.phpapp/Models/AgentJob.phpapp/Console/Commands/RunScheduledBackups.phpapp/Console/Commands/AgentRunCommand.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Enums/*.php : Enum keys should be TitleCase (e.g., FavoritePerson, BestLake, Monthly).
Applied to files:
app/Enums/DatabaseSelectionMode.php
📚 Learning: 2026-02-25T10:48:46.954Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Services/Agent/AgentJobPayloadBuilder.php:63-76
Timestamp: 2026-02-25T10:48:46.954Z
Learning: In app/Services/Agent/AgentJobPayloadBuilder.php and similar service classes, prefer fail-fast error handling over defensive null-safe operators when upstream callers (e.g., TriggerBackupAction, RunScheduledBackups) guarantee certain relationships exist. For example, in resolveBackupPath(), $server->backup is intentionally accessed without the null-safe operator because backup existence is enforced at call sites; using the null-safe operator would hide bugs that violate this invariant.
Applied to files:
app/Http/Controllers/Api/V1/AgentController.phpdatabase/factories/AgentJobFactory.phpapp/Services/Agent/AgentApiClient.phptests/Feature/Agent/AgentApiTest.phptests/Feature/Services/Backup/TriggerBackupActionTest.phpapp/Models/AgentJob.phptests/Feature/DatabaseServer/ConnectionStatusTest.phpapp/Console/Commands/RunScheduledBackups.phpapp/Console/Commands/AgentRunCommand.phpapp/Services/Backup/TriggerBackupAction.phpapp/Services/Backup/BackupJobFactory.phpapp/Livewire/Forms/DatabaseServerForm.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Services/Backup/BackupJobFactory.php : Types that backup the whole instance (e.g., Redis, SQLite) should short-circuit in BackupJobFactory.createSnapshots() to create a single snapshot instead of per-database snapshots.
Applied to files:
app/Http/Controllers/Api/V1/AgentController.phpdatabase/factories/AgentJobFactory.phptests/Feature/Agent/AgentApiTest.phptests/Feature/Services/Backup/TriggerBackupActionTest.phpapp/Models/AgentJob.phpapp/Console/Commands/RunScheduledBackups.phpapp/Services/Backup/TriggerBackupAction.phpapp/Services/Backup/BackupJobFactory.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to tests/**/*.php : When creating models for tests, use the factories for the models. Check if the factory has custom states that can be used before manually setting up the model.
Applied to files:
database/factories/AgentJobFactory.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/**/*.php : Use environment variables only in configuration files - never use the env() function directly outside of config files. Always use config('key'), not env('KEY').
Applied to files:
phpstan.neon
📚 Learning: 2026-02-18T09:45:52.485Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 116
File: app/Livewire/DatabaseServer/ConnectionStatus.php:18-18
Timestamp: 2026-02-18T09:45:52.485Z
Learning: In Livewire components, Eloquent model properties (e.g., public DatabaseServer $server) are automatically locked by the framework to prevent client-side ID tampering. The #[Locked] attribute is only needed for scalar properties (int, string, bool, etc.) that require protection from client-side mutation. Apply this guidance to all Livewire PHP components; use #[Locked] only on primitive properties that you want to shield from client manipulation, and rely on automatic locking for Eloquent model properties.
Applied to files:
app/Livewire/DatabaseServer/ConnectionStatus.phpapp/Livewire/Forms/DatabaseServerForm.php
📚 Learning: 2026-02-13T11:05:37.072Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:37.072Z
Learning: Adopt a global afterEach hook in tests/Pest.php (or equivalent Pest bootstrap) to clean up temporary directories created during tests. Specifically handle temp dirs named with the prefixes sqlite-db-test-*, backup-task-test-*, restore-task-test-*, and volume-test-*, so individual test files don’t need their own cleanup logic. This applies to all PHP test files under the tests directory.
Applied to files:
tests/Feature/Agent/AgentApiTest.phptests/Feature/DatabaseServer/CreateTest.phptests/Feature/Services/Backup/TriggerBackupActionTest.phptests/Feature/DatabaseServer/ConnectionStatusTest.php
📚 Learning: 2026-01-11T22:24:57.673Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 25
File: docker/php/scripts/fix-permissions.sh:25-28
Timestamp: 2026-01-11T22:24:57.673Z
Learning: In docker/php/scripts/fix-permissions.sh, the script intentionally exits with status 0 (success) even when the GROUP validation fails or permissions cannot be set. This non-blocking behavior ensures the application continues to start, prioritizing availability over strict permission enforcement.
Applied to files:
docker/php/scripts/start.sh
📚 Learning: 2026-02-17T08:43:03.630Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 114
File: tests/Integration/SshTunnelTest.php:37-46
Timestamp: 2026-02-17T08:43:03.630Z
Learning: In the databasement project, integration tests use RefreshDatabase (configured in tests/Pest.php), which automatically rolls back all database changes after each test. Therefore, explicit model deletion calls like $server->delete(), $volume->delete(), or $sshConfig->delete() are unnecessary in integration tests.
Applied to files:
tests/Feature/DatabaseServer/CreateTest.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to routes/*.php : All routes use Route::livewire() for full-page Livewire components (e.g., Route::livewire('database-servers', \App\Livewire\DatabaseServer\Index::class)). Authentication routes use plain Blade views rendered by Laravel Fortify.
Applied to files:
tests/Feature/DatabaseServer/CreateTest.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Livewire/Volume/Connectors/*.php : All volume config components extend BaseConfig which handles mounting, validation, and rendering. Sensitive fields in sensitiveFields() are automatically encrypted in database and masked in browser.
Applied to files:
tests/Feature/DatabaseServer/CreateTest.php
📚 Learning: 2026-02-24T10:43:22.287Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 136
File: app/Models/DatabaseServer.php:250-258
Timestamp: 2026-02-24T10:43:22.287Z
Learning: Pattern mode (database_selection_mode = 'pattern') is not available for SQLite or Redis database servers. The UI conditionally hides the segmented control for these types in resources/views/livewire/database-server/_form.blade.php (line 242: !$form->isSqlite() && !$form->isRedis()).
Applied to files:
resources/views/livewire/database-server/index.blade.phpresources/views/livewire/database-server/_form.blade.phpapp/Livewire/Forms/DatabaseServerForm.php
📚 Learning: 2026-01-30T22:27:46.107Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 61
File: resources/views/livewire/volume/connectors/s3-config.blade.php:1-13
Timestamp: 2026-01-30T22:27:46.107Z
Learning: In Blade template files (any .blade.php) within the databasement project, allow using alert-info for informational content inside <x-alert> components. The guideline that permits alert-success and alert-error does not exclude using alert-info for informational purposes. Apply this consistently to all Blade components that render alerts; ensure semantic usage and accessibility.
Applied to files:
resources/views/livewire/database-server/index.blade.phpresources/views/livewire/backup-job/_logs-modal.blade.phpresources/views/livewire/database-server/_form.blade.php
📚 Learning: 2026-02-06T10:34:43.585Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 75
File: resources/views/livewire/backup-job/_filters.blade.php:36-40
Timestamp: 2026-02-06T10:34:43.585Z
Learning: In Blade template files, when creating compact inline filter controls, prefer using native <input type="checkbox"> elements with daisyUI classes (e.g., checkbox checkbox-warning checkbox-xs) over the Mary UI <x-checkbox> component. The <x-checkbox> component adds wrapper markup (e.g., <div><fieldset><label> with gap-3) that can break tight inline flex layouts. Use the native input approach for compact inline controls, but reserve <x-checkbox> for form fields that require labels, hints, and errors.
Applied to files:
resources/views/livewire/database-server/index.blade.phpresources/views/livewire/backup-job/_logs-modal.blade.phpresources/views/livewire/database-server/_form.blade.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Always use 'make test' for running tests, never use 'docker compose exec ... php artisan test' directly. Tests run in parallel by default using Pest's parallel testing feature for speed.
Applied to files:
Makefile
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: All PHP commands (php, composer, vendor/bin/*) MUST be run through Docker using 'docker compose exec --user application -T app <command>' or Makefile targets. Never run these commands directly on the host.
Applied to files:
Makefile
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to database/migrations/*.php : When modifying a column in a migration, the migration must include all of the attributes that were previously defined on the column. Otherwise, they will be dropped and lost.
Applied to files:
database/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Models/**/*.php : Database models use ULIDs (Universally Unique Lexicographically Sortable Identifiers) for primary keys instead of auto-incrementing integers.
Applied to files:
database/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.php
📚 Learning: 2026-02-13T11:05:44.422Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:44.422Z
Learning: The project uses a global `afterEach` hook in `tests/Pest.php` to clean up temp directories created during tests. The hook includes patterns for `/sqlite-db-test-*`, `/backup-task-test-*`, `/restore-task-test-*`, and `/volume-test-*`, so individual test files don't need their own cleanup logic for temp directories following these naming conventions.
Applied to files:
app/Console/Commands/AgentRunCommand.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Services/Backup/Databases/*.php : All database type implementations must implement DatabaseInterface with methods: setConfig, dump, restore, prepareForRestore, listDatabases, testConnection. Types without PDO support (e.g., Redis) must throw in buildDsn() and handle connection testing via CLI.
Applied to files:
app/Services/Backup/BackupJobFactory.phpapp/Enums/DatabaseType.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to app/Services/Backup/Databases/DatabaseProvider.php : DatabaseProvider centralizes type dispatch via make() method, so BackupTask, RestoreTask, and connection testing require no changes when adding new database types.
Applied to files:
app/Services/Backup/BackupJobFactory.php
📚 Learning: 2026-02-20T23:44:19.988Z
Learnt from: CR
Repo: David-Crty/databasement PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T23:44:19.988Z
Learning: Applies to resources/views/**/*.blade.php : Select component pattern in Mary UI: Use :options prop with array format [['id' => 'value', 'name' => 'Label']].
Applied to files:
resources/views/livewire/database-server/_form.blade.php
🧬 Code graph analysis (18)
config/cache.php (1)
app/Support/helpers.php (1)
agent_mode(3-6)
app/Http/Controllers/Api/V1/AgentController.php (9)
app/Http/Controllers/Controller.php (1)
Controller(5-8)app/Models/Agent.php (1)
Agent(38-79)app/Models/AgentJob.php (9)
AgentJob(47-186)agent(98-101)claim(122-131)snapshot(114-117)markRunning(136-141)extendLease(171-176)markCompleted(146-153)markFailed(158-166)databaseServer(106-109)app/Models/DatabaseServer.php (2)
DatabaseServer(64-289)agent(122-125)app/Services/Agent/AgentJobPayloadBuilder.php (2)
AgentJobPayloadBuilder(13-72)build(20-35)app/Services/Backup/BackupJobFactory.php (2)
BackupJobFactory(16-141)createSnapshot(87-116)app/Services/FailureNotificationService.php (2)
FailureNotificationService(15-99)notifyBackupFailed(17-20)app/Models/OAuthIdentity.php (1)
user(59-62)app/Policies/AgentPolicy.php (2)
update(41-44)create(32-35)
database/factories/AgentJobFactory.php (1)
app/Models/AgentJob.php (3)
AgentJob(47-186)snapshot(114-117)agent(98-101)
app/Services/Agent/AgentApiClient.php (1)
app/Services/Agent/AgentAuthenticationException.php (1)
AgentAuthenticationException(5-5)
app/Livewire/DatabaseServer/ConnectionStatus.php (2)
app/Models/DatabaseServer.php (1)
agent(122-125)app/Models/Agent.php (1)
isOnline(74-78)
config/database.php (1)
app/Support/helpers.php (1)
agent_mode(3-6)
tests/Feature/Agent/AgentApiTest.php (5)
app/Models/Agent.php (1)
Agent(38-79)app/Models/AgentJob.php (3)
AgentJob(47-186)agent(98-101)snapshot(114-117)app/Models/Snapshot.php (1)
Snapshot(72-308)database/factories/AgentJobFactory.php (3)
expiredLease(152-161)claimed(95-104)discover(68-90)app/Notifications/BackupFailedNotification.php (1)
BackupFailedNotification(7-31)
tests/Feature/DatabaseServer/CreateTest.php (5)
app/Models/Agent.php (1)
Agent(38-79)app/Models/Volume.php (1)
Volume(41-156)app/Livewire/Forms/DatabaseServerForm.php (1)
getVolumeOptions(546-559)app/Models/DatabaseServer.php (1)
agent(122-125)tests/Pest.php (1)
dailySchedule(90-96)
tests/Feature/Services/Backup/TriggerBackupActionTest.php (3)
app/Models/Agent.php (2)
Agent(38-79)agentJobs(66-69)app/Models/AgentJob.php (2)
AgentJob(47-186)agent(98-101)app/Models/DatabaseServer.php (2)
agent(122-125)DatabaseServer(64-289)
app/Models/AgentJob.php (3)
database/factories/AgentJobFactory.php (1)
AgentJobFactory(13-162)app/Models/DatabaseServer.php (3)
casts(106-117)agent(122-125)DatabaseServer(64-289)app/Models/Agent.php (2)
casts(48-53)Agent(38-79)
database/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.php (2)
app/Policies/AgentPolicy.php (2)
create(32-35)delete(50-53)app/Livewire/Agent/Index.php (1)
delete(81-96)
config/queue.php (1)
app/Support/helpers.php (1)
agent_mode(3-6)
tests/Feature/DatabaseServer/ConnectionStatusTest.php (2)
app/Models/Agent.php (1)
Agent(38-79)app/Models/DatabaseServer.php (2)
agent(122-125)DatabaseServer(64-289)
app/Console/Commands/RunScheduledBackups.php (3)
app/Models/AgentJob.php (2)
AgentJob(47-186)snapshot(114-117)app/Services/Agent/AgentJobPayloadBuilder.php (3)
AgentJobPayloadBuilder(13-72)buildDiscovery(43-54)build(20-35)app/Services/Backup/BackupJobFactory.php (1)
BackupJobFactory(16-141)
config/session.php (1)
app/Support/helpers.php (1)
agent_mode(3-6)
app/Console/Commands/AgentRunCommand.php (9)
app/Models/DatabaseServer.php (2)
DatabaseServer(64-289)forConnectionTest(194-211)app/Services/Agent/AgentApiClient.php (7)
AgentApiClient(9-101)heartbeat(16-25)claimJob(30-39)jobHeartbeat(44-47)ack(52-66)fail(71-77)reportDiscoveredDatabases(82-87)app/Services/Agent/AgentAuthenticationException.php (1)
AgentAuthenticationException(5-5)app/Services/Backup/Databases/DatabaseProvider.php (2)
DatabaseProvider(11-216)listDatabasesForServer(176-187)app/Services/Backup/DTO/BackupConfig.php (2)
BackupConfig(7-68)fromPayload(52-67)app/Services/Backup/InMemoryBackupLogger.php (2)
InMemoryBackupLogger(7-90)flush(75-81)app/Support/FilesystemSupport.php (2)
FilesystemSupport(8-57)createWorkingDirectory(19-29)app/Http/Middleware/EnsureAgentToken.php (1)
handle(12-20)app/Http/Controllers/Api/V1/AgentController.php (5)
heartbeat(49-57)claimJob(64-114)jobHeartbeat(121-147)ack(154-192)fail(199-236)
app/Services/Backup/TriggerBackupAction.php (3)
app/Models/AgentJob.php (2)
AgentJob(47-186)snapshot(114-117)app/Services/Agent/AgentJobPayloadBuilder.php (3)
AgentJobPayloadBuilder(13-72)build(20-35)buildDiscovery(43-54)app/Console/Commands/RunScheduledBackups.php (1)
dispatch(67-110)
app/Livewire/Forms/DatabaseServerForm.php (3)
app/Models/Agent.php (1)
Agent(38-79)app/Models/Volume.php (1)
Volume(41-156)app/Models/DatabaseServer.php (1)
agent(122-125)
🪛 PHPMD (2.15.0)
app/Livewire/Forms/DatabaseServerForm.php
[warning] 622-622: Avoid unused parameters such as '$attribute'. (undefined)
(UnusedFormalParameter)
Comment on lines
+22
to
+30
| if (RateLimiter::tooManyAttempts($key, 10)) { | ||
| return response()->json(['message' => 'Too many failed attempts. Try again later.'], 429); | ||
| } | ||
|
|
||
| $response = $next($request); | ||
|
|
||
| if ($response->getStatusCode() === 401 || $response->getStatusCode() === 403) { | ||
| RateLimiter::hit($key, 300); | ||
| } |
There was a problem hiding this comment.
Throttle check currently locks out valid requests after prior failures.
Line 22 runs before auth middleware, so after the threshold is reached, valid agent tokens will also get 429. That contradicts the contract in Lines 13-15 and can block healthy agents.
💡 Suggested fix
- if (RateLimiter::tooManyAttempts($key, 10)) {
- return response()->json(['message' => 'Too many failed attempts. Try again later.'], 429);
- }
-
$response = $next($request);
- if ($response->getStatusCode() === 401 || $response->getStatusCode() === 403) {
- RateLimiter::hit($key, 300);
+ if (in_array($response->getStatusCode(), [401, 403], true)) {
+ if (RateLimiter::tooManyAttempts($key, 10)) {
+ return response()->json(['message' => 'Too many failed attempts. Try again later.'], 429);
+ }
+
+ RateLimiter::hit($key, 300);
+ } else {
+ RateLimiter::clear($key);
}📝 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.
Suggested change
| if (RateLimiter::tooManyAttempts($key, 10)) { | |
| return response()->json(['message' => 'Too many failed attempts. Try again later.'], 429); | |
| } | |
| $response = $next($request); | |
| if ($response->getStatusCode() === 401 || $response->getStatusCode() === 403) { | |
| RateLimiter::hit($key, 300); | |
| } | |
| $response = $next($request); | |
| if (in_array($response->getStatusCode(), [401, 403], true)) { | |
| if (RateLimiter::tooManyAttempts($key, 10)) { | |
| return response()->json(['message' => 'Too many failed attempts. Try again later.'], 429); | |
| } | |
| RateLimiter::hit($key, 300); | |
| } else { | |
| RateLimiter::clear($key); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Http/Middleware/ThrottleFailedAgentAuth.php` around lines 22 - 30, The
current early RateLimiter::tooManyAttempts($key, 10) check runs before
authentication and will return 429 for valid requests after prior failures; to
fix, defer the throttle decision until after the request is processed: call
$response = $next($request) first, then if $response->getStatusCode() is 401 or
403 call RateLimiter::hit($key, 300) and after that check
RateLimiter::tooManyAttempts($key, 10) and return the 429 response only when the
threshold is exceeded. Update the ThrottleFailedAgentAuth middleware to use this
post-auth check flow so valid agent tokens are not blocked.
Comment on lines
+606
to
607
| 'agent_id' => 'nullable|exists:agents,id', | ||
| 'backups_enabled' => 'boolean', |
There was a problem hiding this comment.
Volume/agent constraint can be bypassed by desynchronizing use_agent and agent_id.
Line 623 uses $this->use_agent (UI state) for enforcement, while Line 606 accepts agent_id independently. A crafted payload can keep use_agent=false but send agent_id, allowing invalid local-volume + agent combinations.
🛠️ Suggested fix
private function getBaseValidationRules(): array
{
return [
'name' => 'required|string|max:255',
+ 'use_agent' => 'boolean',
'database_type' => ['required', 'string', Rule::in(array_map(
fn (DatabaseType $type) => $type->value,
DatabaseType::cases()
))],
'description' => 'nullable|string|max:1000',
- 'agent_id' => 'nullable|exists:agents,id',
+ 'agent_id' => [Rule::requiredIf(fn () => $this->use_agent), 'nullable', 'exists:agents,id'],
'backups_enabled' => 'boolean',
];
}
@@
function (string $attribute, mixed $value, \Closure $fail): void {
- if ($this->use_agent
+ if (! empty($this->agent_id)
&& \App\Models\Volume::whereKey($value)->where('type', \App\Enums\VolumeType::LOCAL->value)->exists()
) {
$fail(__('Local volumes cannot be used with remote agents.'));
}
},Also applies to: 623-625
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Livewire/Forms/DatabaseServerForm.php` around lines 606 - 607, The form
allows sending an agent_id even when use_agent is false; update the validation
in DatabaseServerForm so agent_id cannot be set unless use_agent is true and
ensure runtime checks use validated data: change the 'agent_id' rule (where
rules() returns 'agent_id' => 'nullable|exists:agents,id') to include a
prohibition like 'prohibited_unless:use_agent,true' (or
'prohibited_if:use_agent,false' / 'required_if:use_agent,true' depending on
desired behavior), and modify the later enforcement logic around use_agent/agent
handling (the block that currently reads $this->use_agent at lines ~623-625) to
consult the validated input (e.g. $validated['use_agent'] or validated data)
instead of raw UI state so a crafted payload cannot desynchronize use_agent and
agent_id.
Comment on lines
+122
to
+130
| public function claim(Agent $agent, int $leaseDurationSeconds = 300): void | ||
| { | ||
| $this->update([ | ||
| 'agent_id' => $agent->id, | ||
| 'status' => self::STATUS_CLAIMED, | ||
| 'lease_expires_at' => now()->addSeconds($leaseDurationSeconds), | ||
| 'claimed_at' => now(), | ||
| 'attempts' => $this->attempts + 1, | ||
| ]); |
There was a problem hiding this comment.
Make claim() conflict-safe to avoid stale-write races.
At Line 129, attempts is incremented from in-memory state. Concurrent claims can overwrite each other and produce incorrect attempt/job ownership metadata.
🛠️ Safer claim pattern (conflict-aware update)
+use RuntimeException;
@@
public function claim(Agent $agent, int $leaseDurationSeconds = 300): void
{
- $this->update([
+ $updated = static::query()
+ ->whereKey($this->id)
+ ->where('attempts', $this->attempts)
+ ->update([
'agent_id' => $agent->id,
'status' => self::STATUS_CLAIMED,
'lease_expires_at' => now()->addSeconds($leaseDurationSeconds),
'claimed_at' => now(),
'attempts' => $this->attempts + 1,
- ]);
+ ]);
+
+ if ($updated === 0) {
+ throw new RuntimeException('Agent job claim conflict detected; retry claim.');
+ }
+
+ $this->refresh();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Models/AgentJob.php` around lines 122 - 130, The claim() method is doing
a non-atomic in-memory increment of attempts which can be lost by concurrent
claimers; change to a conflict-safe update by performing an atomic DB operation:
use the AgentJob::where('id', $this->id)->where('attempts',
$this->attempts)->update(...) (or a DB transaction with SELECT ... FOR UPDATE)
to set agent_id, status (self::STATUS_CLAIMED), lease_expires_at, claimed_at and
attempts = attempts + 1, then check the affected-row count and handle
zero-updates (retry or surface failure) so stale-write races cannot overwrite
concurrent claimers. Ensure you reference and update the same fields used now
(agent_id, attempts, lease_expires_at, claimed_at, status) and make claim()
return or throw when the conditional update fails.
Makefile
Outdated
Comment on lines
+41
to
+46
| docker run --rm --network=$$(docker network ls --filter name=databasement -q | head -1) \ | ||
| -e AWS_ACCESS_KEY_ID=rustfsadmin \ | ||
| -e AWS_SECRET_ACCESS_KEY=rustfsadmin \ | ||
| amazon/aws-cli \ | ||
| --endpoint-url=http://rustfs:9000 \ | ||
| s3 mb s3://$(or $(BUCKET),test-bucket) 2>/dev/null || echo "Bucket may already exist" | ||
| s3 mb s3://$(or $(BUCKET),test-bucket) |
There was a problem hiding this comment.
create-bucket should be idempotent for repeatable setup.
This now hard-fails on reruns instead of tolerating pre-existing state. Please add an existence check (or equivalent fallback) before create.
🔧 Suggested idempotent pattern
create-bucket: ## Create S3 bucket in rustfs (usage: make create-bucket BUCKET=my-bucket)
- docker run --rm --network=$$(docker network ls --filter name=databasement -q | head -1) \
+ `@bucket`="$(or $(BUCKET),test-bucket)"; \
+ docker run --rm --network=$$(docker network ls --filter name=databasement -q | head -1) \
+ -e AWS_ACCESS_KEY_ID=rustfsadmin \
+ -e AWS_SECRET_ACCESS_KEY=rustfsadmin \
+ amazon/aws-cli \
+ --endpoint-url=http://rustfs:9000 \
+ s3 ls s3://$$bucket >/dev/null 2>&1 || \
+ docker run --rm --network=$$(docker network ls --filter name=databasement -q | head -1) \
-e AWS_ACCESS_KEY_ID=rustfsadmin \
-e AWS_SECRET_ACCESS_KEY=rustfsadmin \
amazon/aws-cli \
--endpoint-url=http://rustfs:9000 \
- s3 mb s3://$(or $(BUCKET),test-bucket)
+ s3 mb s3://$$bucket📝 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.
Suggested change
| docker run --rm --network=$$(docker network ls --filter name=databasement -q | head -1) \ | |
| -e AWS_ACCESS_KEY_ID=rustfsadmin \ | |
| -e AWS_SECRET_ACCESS_KEY=rustfsadmin \ | |
| amazon/aws-cli \ | |
| --endpoint-url=http://rustfs:9000 \ | |
| s3 mb s3://$(or $(BUCKET),test-bucket) 2>/dev/null || echo "Bucket may already exist" | |
| s3 mb s3://$(or $(BUCKET),test-bucket) | |
| `@bucket`="$(or $(BUCKET),test-bucket)"; \ | |
| docker run --rm --network=$$(docker network ls --filter name=databasement -q | head -1) \ | |
| -e AWS_ACCESS_KEY_ID=rustfsadmin \ | |
| -e AWS_SECRET_ACCESS_KEY=rustfsadmin \ | |
| amazon/aws-cli \ | |
| --endpoint-url=http://rustfs:9000 \ | |
| s3 ls s3://$$bucket >/dev/null 2>&1 || \ | |
| docker run --rm --network=$$(docker network ls --filter name=databasement -q | head -1) \ | |
| -e AWS_ACCESS_KEY_ID=rustfsadmin \ | |
| -e AWS_SECRET_ACCESS_KEY=rustfsadmin \ | |
| amazon/aws-cli \ | |
| --endpoint-url=http://rustfs:9000 \ | |
| s3 mb s3://$$bucket |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 41 - 46, The create-bucket recipe currently fails if
the bucket already exists; change the docker run invocation that calls
"amazon/aws-cli ... s3 mb s3://$(or $(BUCKET),test-bucket)" to first check for
the bucket and only create it when missing (for example by running an existence
check like "aws s3 ls s3://<bucket>" or "aws s3api head-bucket --bucket
<bucket>" inside the same docker run and falling back to "aws s3 mb
s3://<bucket>" only on failure). Update the command that builds the container
(the docker run line in the Makefile) so it performs the existence check and
then conditionally runs the s3 mb to make the operation idempotent.
phpstan.neon
Outdated
Comment on lines
+19
to
+21
| - | ||
| message: "#Called 'env' outside of the config directory#" | ||
| path: app/Support/helpers.php |
There was a problem hiding this comment.
Keep the env()-outside-config guardrail enabled.
This suppression hides a policy violation and makes future leaks harder to catch. Please fix app/Support/helpers.php to read from config(...) (with env() only in config/*.php) and remove this ignore rule.
As per coding guidelines: "Use environment variables only in configuration files - never use the env() function directly outside of config files. Always use config('key'), not env('KEY')."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@phpstan.neon` around lines 19 - 21, The suppression in phpstan.neon hides use
of env() in app/Support/helpers.php; open app/Support/helpers.php, locate any
direct calls to env(...) (and functions that wrap it) and refactor them to read
from config('...') instead, moving any env(...) usages into an appropriate
config/*.php file where you set config keys from env(...); once helpers.php uses
config(...) only, remove the corresponding ignore block from phpstan.neon so the
guardrail stays enabled.
Comment on lines
+168
to
+172
| $agentJobs = AgentJob::where('database_server_id', $server->id)->get(); | ||
| expect($agentJobs)->toHaveCount(2) | ||
| ->and($agentJobs[0]->type)->toBe(AgentJob::TYPE_BACKUP) | ||
| ->and($agentJobs[0]->snapshot_id)->not->toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Avoid order-dependent assertions on unordered collections.
$agentJobs[0] assumes DB return order. Assert collection-level invariants instead (or add explicit ordering).
💡 Suggested assertion style
- $agentJobs = AgentJob::where('database_server_id', $server->id)->get();
- expect($agentJobs)->toHaveCount(2)
- ->and($agentJobs[0]->type)->toBe(AgentJob::TYPE_BACKUP)
- ->and($agentJobs[0]->snapshot_id)->not->toBeNull();
+ $agentJobs = AgentJob::where('database_server_id', $server->id)
+ ->where('type', AgentJob::TYPE_BACKUP)
+ ->orderBy('id')
+ ->get();
+ expect($agentJobs)->toHaveCount(2)
+ ->and($agentJobs->every(fn (AgentJob $job) => $job->snapshot_id !== null))->toBeTrue();📝 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.
Suggested change
| $agentJobs = AgentJob::where('database_server_id', $server->id)->get(); | |
| expect($agentJobs)->toHaveCount(2) | |
| ->and($agentJobs[0]->type)->toBe(AgentJob::TYPE_BACKUP) | |
| ->and($agentJobs[0]->snapshot_id)->not->toBeNull(); | |
| }); | |
| $agentJobs = AgentJob::where('database_server_id', $server->id) | |
| ->where('type', AgentJob::TYPE_BACKUP) | |
| ->orderBy('id') | |
| ->get(); | |
| expect($agentJobs)->toHaveCount(2) | |
| ->and($agentJobs->every(fn (AgentJob $job) => $job->snapshot_id !== null))->toBeTrue(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Feature/Services/Backup/TriggerBackupActionTest.php` around lines 168 -
172, The test is relying on DB return order by indexing $agentJobs[0]; change
the assertion to be order-independent by either adding an explicit ordering to
the query (use AgentJob::where('database_server_id',
$server->id)->orderBy('id')->get() and then assert types/snapshot_id) or,
better, assert collection-level invariants using collection helpers (e.g.,
$agentJobs = AgentJob::where(...)->get(); expect($agentJobs->where('type',
AgentJob::TYPE_BACKUP))->toHaveCount(2) and
expect($agentJobs->pluck('snapshot_id')->filter()->count())->toBe(2) or use
$agentJobs->every(fn($j)=> $j->type === AgentJob::TYPE_BACKUP &&
!is_null($j->snapshot_id)) so the test no longer depends on ordering).
David-Crty
added a commit
that referenced
this pull request
Mar 2, 2026
- Guard AgentController::ack() against null snapshot (non-backup jobs) - Clear rate limiter on successful auth in ThrottleFailedAgentAuth - Wrap dispatchToAgent in DB::transaction for atomicity - Make Makefile create-bucket idempotent - Use sole() in TriggerBackupActionTest for stricter assertions - Use DatabaseSelectionMode enum in AgentJobFactory
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Services/Backup/BackupJobFactory.php (1)
40-47: 🛠️ Refactor suggestion | 🟠 MajorSQLite should short-circuit to a single snapshot in
createSnapshots().Line 41 currently loops each configured path and creates multiple snapshots, but this factory is expected to treat SQLite as a whole-instance backup path like Redis.
As per coding guidelines, "Types that backup the whole instance (e.g., Redis, SQLite) should short-circuit in BackupJobFactory.createSnapshots() to create a single snapshot instead of per-database snapshots."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Backup/BackupJobFactory.php` around lines 40 - 47, The current SQLite branch in BackupJobFactory::createSnapshots() iterates over each database path and creates multiple snapshots; instead short-circuit for DatabaseType::SQLITE to create a single snapshot for the whole instance by calling $this->createSnapshot(...) exactly once (not in a loop) and returning that single-item $snapshots array; update the createSnapshot call site to pass a single representative database identifier (e.g., null or the first path) or adjust createSnapshot to accept a whole-instance indicator so SQLite behaves like Redis (one snapshot total).
♻️ Duplicate comments (8)
app/Http/Middleware/ThrottleFailedAgentAuth.php (1)
22-32:⚠️ Potential issue | 🟠 MajorThrottle decision is still pre-auth and can lock out valid agent tokens.
Because Line 22 runs before
$next($request), previously failed attempts can cause all subsequent requests from that IP to return 429, including valid credentials.🔧 Suggested post-auth flow
- if (RateLimiter::tooManyAttempts($key, 10)) { - return response()->json(['message' => 'Too many failed attempts. Try again later.'], 429); - } - $response = $next($request); - if ($response->getStatusCode() === 401 || $response->getStatusCode() === 403) { + if (in_array($response->getStatusCode(), [401, 403], true)) { RateLimiter::hit($key, 300); + + if (RateLimiter::tooManyAttempts($key, 10)) { + return response()->json(['message' => 'Too many failed attempts. Try again later.'], 429); + } } else { RateLimiter::clear($key); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Middleware/ThrottleFailedAgentAuth.php` around lines 22 - 32, The current pre-auth check using RateLimiter::tooManyAttempts($key, 10) runs before $next($request) and can block valid requests; move the throttle decision so you call $response = $next($request) first, then check the response status and only apply RateLimiter::hit($key, 300) when $response->getStatusCode() indicates auth failure (401/403), otherwise call RateLimiter::clear($key); remove or relocate the initial RateLimiter::tooManyAttempts check so it does not reject requests before authentication completes.phpstan.neon (1)
19-21:⚠️ Potential issue | 🟠 MajorDo not suppress the
env()guardrail here.This ignore block disables a core policy check and allows
env()usage outside config to pass static analysis.As per coding guidelines: "Use environment variables only in configuration files - never use the env() function directly outside of config files. Always use config('key'), not env('KEY')."🔧 Suggested change
- - - message: "#Called 'env' outside of the config directory#" - path: app/Support/helpers.php🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpstan.neon` around lines 19 - 21, Remove the suppression that ignores the "Called 'env' outside of the config directory" rule in phpstan.neon so the guardrail remains active; then update app/Support/helpers.php to stop calling env() directly by refactoring any env('...') usages into config keys (use config('your.package.key') in the helper) or pass required values from configuration where the helper is invoked so the helper no longer depends on env(); ensure references to the env() call sites in helpers.php (and any functions/classes defined there) are replaced with config-based accessors and restore the phpstan.neon entry so the rule is enforced.Makefile (1)
41-46:⚠️ Potential issue | 🟠 Major
create-bucketnow masks real provisioning failures.Line 46 uses
|| true, which swallows auth/network/endpoint errors the same as “bucket already exists”.🔧 Idempotent (but fail-fast) pattern
- docker run --rm --network=$$(docker network ls --filter name=databasement -q | head -1) \ + `@bucket`="$(or $(BUCKET),test-bucket)"; \ + docker run --rm --network=$$(docker network ls --filter name=databasement -q | head -1) \ + -e AWS_ACCESS_KEY_ID=rustfsadmin \ + -e AWS_SECRET_ACCESS_KEY=rustfsadmin \ + amazon/aws-cli \ + --endpoint-url=http://rustfs:9000 \ + s3 ls s3://$$bucket >/dev/null 2>&1 || \ + docker run --rm --network=$$(docker network ls --filter name=databasement -q | head -1) \ -e AWS_ACCESS_KEY_ID=rustfsadmin \ -e AWS_SECRET_ACCESS_KEY=rustfsadmin \ amazon/aws-cli \ --endpoint-url=http://rustfs:9000 \ - s3 mb s3://$(or $(BUCKET),test-bucket) 2>/dev/null || true + s3 mb s3://$$bucket🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 41 - 46, The current docker aws-cli invocation line "docker run ... amazon/aws-cli ... s3 mb s3://$(or $(BUCKET),test-bucket) 2>/dev/null || true" suppresses all errors; replace this by removing the blanket "|| true" and stop redirecting stderr so failures propagate, and implement an explicit check that treats only the "bucket already exists/owned" condition as non-fatal while failing fast for auth/network/endpoint errors; locate the command string shown above and change it to let the aws-cli error output surface and add logic to inspect the aws error (or use aws s3api/head-bucket) to ignore the specific "BucketAlreadyOwnedByYou/BucketAlreadyExists" result but exit non-zero for other errors.app/Support/helpers.php (1)
3-5:⚠️ Potential issue | 🟠 MajorAvoid
env()access in application helper code.Line 5 reads directly from
env(). Move env resolution into config and read throughconfig(...)here to keep runtime behavior consistent.🔧 Proposed fix
function agent_mode(): bool { - return (bool) env('DATABASEMENT_URL'); + return (bool) config('agent.enabled', false); }As per coding guidelines, "Use environment variables only in configuration files - never use the env() function directly outside of config files. Always use config('key'), not env('KEY')."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/helpers.php` around lines 3 - 5, The helper function agent_mode() currently reads env('DATABASEMENT_URL') directly; move that env resolution into a config file (e.g., add a key like databasement_url or databasement.enabled in a relevant config such as config/app.php or config/services.php using env('DATABASEMENT_URL') there) and then update agent_mode() to return (bool) config('...') (use the chosen config key) instead of calling env(...) directly; ensure the config key name is consistent between the new config entry and the agent_mode() call.tests/Feature/Services/Backup/TriggerBackupActionTest.php (1)
166-170:⚠️ Potential issue | 🟡 MinorMake the selected-mode assertion order-independent.
Indexing
$agentJobs[0]can fail nondeterministically because DB result order is not guaranteed.💡 Suggested assertion tightening
- $agentJobs = AgentJob::where('database_server_id', $server->id)->get(); - expect($agentJobs)->toHaveCount(2) - ->and($agentJobs[0]->type)->toBe(AgentJob::TYPE_BACKUP) - ->and($agentJobs[0]->snapshot_id)->not->toBeNull(); + $agentJobs = AgentJob::where('database_server_id', $server->id) + ->where('type', AgentJob::TYPE_BACKUP) + ->get(); + + expect($agentJobs)->toHaveCount(2) + ->and($agentJobs->every(fn (AgentJob $job) => $job->snapshot_id !== null))->toBeTrue();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/Services/Backup/TriggerBackupActionTest.php` around lines 166 - 170, The test assumes a stable DB order by indexing $agentJobs[0], which makes it flaky; in TriggerBackupActionTest replace index-based checks with order-independent assertions on the AgentJob collection (AgentJob::where(...)->get()) such as asserting the collection has count 2, that the collection contains the expected TYPE_BACKUP values (e.g., by plucking 'type' and asserting it contains the value twice or that the count of TYPE_BACKUP equals 2), and that all returned items have non-null snapshot_id (e.g., assert every item->snapshot_id is not null) so the test no longer depends on row order.config/database.php (1)
19-19:⚠️ Potential issue | 🟠 MajorAvoid
agent_mode()indirection in config default resolution.This still depends on a helper that reads
env()fromapp/Support/helpers.php. Prefer resolving directly in config (or via a config key) to keep env access in config-only paths.As per coding guidelines "Use environment variables only in configuration files - never use the env() function directly outside of config files. Always use config('key'), not env('KEY')."💡 Suggested local change
- 'default' => agent_mode() ? 'agent' : env('DB_CONNECTION', 'sqlite'), + 'default' => env('DATABASEMENT_URL') ? 'agent' : env('DB_CONNECTION', 'sqlite'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/database.php` at line 19, The config 'default' key currently calls the helper agent_mode(), which pulls env() from app helpers; remove that indirection and resolve the mode directly in config by replacing agent_mode() with a config or env lookup (e.g. use config('agent.mode', false) if you have an agent config, or env('AGENT_MODE', false) here since this is a config file) so the 'default' entry becomes 'default' => (config('agent.mode', false) ? 'agent' : env('DB_CONNECTION', 'sqlite')), avoiding use of the agent_mode() helper.app/Models/AgentJob.php (1)
122-130:⚠️ Potential issue | 🔴 Critical
claim()still allows stale-write races onattempts.
attemptsis incremented from local model state. Concurrent claimers can overwrite each other unless this method itself is conflict-safe.🔒 Conflict-aware update pattern
public function claim(Agent $agent, int $leaseDurationSeconds = 300): void { - $this->update([ + $updated = static::query() + ->whereKey($this->id) + ->where('attempts', $this->attempts) + ->update([ 'agent_id' => $agent->id, 'status' => self::STATUS_CLAIMED, 'lease_expires_at' => now()->addSeconds($leaseDurationSeconds), 'claimed_at' => now(), 'attempts' => $this->attempts + 1, - ]); + ]); + + if ($updated === 0) { + throw new \RuntimeException('Agent job claim conflict detected; retry claim.'); + } + + $this->refresh(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/AgentJob.php` around lines 122 - 130, The claim() method in AgentJob is performing a stale-write on the attempts column by using the model's local state; make the update conflict-safe by performing an atomic increment/update in the database instead of setting 'attempts' => $this->attempts + 1 on the model. Modify AgentJob::claim to issue a DB-level update that sets lease_expires_at, claimed_at, agent_id, status and uses an atomic expression for attempts (e.g. DB::raw('attempts + 1') or an equivalent atomic increment) or perform the update inside a conditional query (where id = $this->id and current lease/state) so concurrent claimers cannot overwrite each other's increments. Ensure you reference and update the same fields used now: 'agent_id', 'status', 'lease_expires_at', 'claimed_at', and 'attempts' in the atomic update.app/Livewire/Forms/DatabaseServerForm.php (1)
597-607:⚠️ Potential issue | 🟠 Major
agent_idcan still bypassuse_agentintent and local-volume rules.
agent_idis accepted even whenuse_agentis false, and the local-volume guard only checks$this->use_agent. A desynced payload can persist an agent with an incompatible local volume.🛡️ Suggested fix
private function getBaseValidationRules(): array { return [ 'name' => 'required|string|max:255', + 'use_agent' => 'boolean', 'database_type' => ['required', 'string', Rule::in(array_map( fn (DatabaseType $type) => $type->value, DatabaseType::cases() ))], 'description' => 'nullable|string|max:1000', - 'agent_id' => 'nullable|exists:agents,id', + 'agent_id' => [ + Rule::requiredIf(fn () => (bool) $this->use_agent), + 'nullable', + 'exists:agents,id', + 'prohibited_unless:use_agent,true', + ], 'backups_enabled' => 'boolean', ]; } @@ function (string $attribute, mixed $value, \Closure $fail): void { - if ($this->use_agent + if (! empty($this->agent_id) && \App\Models\Volume::whereKey($value)->where('type', \App\Enums\VolumeType::LOCAL->value)->exists() ) { $fail(__('Local volumes cannot be used with remote agents.')); } },Also applies to: 619-625
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Livewire/Forms/DatabaseServerForm.php` around lines 597 - 607, The validation currently allows agent_id even when use_agent is false and validates local_volume without ensuring an agent is actually selected; update getBaseValidationRules so agent_id cannot be supplied unless use_agent is true (e.g. make agent_id required_if:use_agent,true or change nullable to conditional) and tighten the exists rule to also constrain agent compatibility (use Rule::exists(...)->where(...) to assert the agent supports the chosen local_volume if applicable); also adjust the local_volume-related rules to run only when an agent is selected (check use_agent or agent_id) so a desynced payload cannot persist an incompatible agent/local-volume combination. Ensure references: getBaseValidationRules, agent_id, use_agent, local_volume, and the Rule::exists usage are updated accordingly.
🧹 Nitpick comments (6)
resources/views/livewire/backup-job/_logs-modal.blade.php (1)
157-157: Move$hasDetailscomputation out of Blade and into the Livewire class.Line 157 adds conditional logic in the view layer. Please precompute this flag in the component (or in prepared log DTO data) and keep the Blade file markup-only.
As per coding guidelines:
resources/views/livewire/**/*.blade.php: “Blade files should contain only view markup; all PHP logic should be in Livewire component classes.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/backup-job/_logs-modal.blade.php` at line 157, Move the `$hasDetails` computation out of the Blade by computing it in the Livewire component that supplies the `$log` data (e.g., in the method that prepares logs or in render()/mount()), add a boolean property/field on each log (e.g., `hasDetails`) computed as `$isCommand || !empty($log['context']) || $isError`, and update the view to use `{{ $log['hasDetails'] }}` (or the passed boolean) instead of evaluating `$isCommand || !empty($log['context']) || $isError` in the Blade; ensure the Livewire component sets `hasDetails` for every log before passing the logs collection to the view.tests/Feature/DatabaseServer/ConnectionStatusTest.php (1)
29-51: Lock in the early-return contract by asserting provider is not called.These tests verify rendered state, but adding a
shouldNotReceive('testConnectionForServer')expectation would prevent regressions where agent-backed checks accidentally hit direct connection logic again.🔧 Example addition
test('shows agent online when agent has recent heartbeat', function () { $user = User::factory()->create(); $agent = Agent::factory()->create(['last_heartbeat_at' => now()]); $server = DatabaseServer::factory()->create(['agent_id' => $agent->id]); + $this->mock(DatabaseProvider::class, fn ($mock) => $mock->shouldNotReceive('testConnectionForServer'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/DatabaseServer/ConnectionStatusTest.php` around lines 29 - 51, Add an assertion to both tests that the direct provider method testConnectionForServer is not invoked to lock the early-return contract: in the ConnectionStatus tests (the Livewire::test cases) set an expectation on the connection provider/mock that it shouldNotReceive('testConnectionForServer') so agent-backed checks never fall through to direct provider logic; locate references to the ConnectionStatus Livewire component and the testConnectionForServer symbol to add the shouldNotReceive expectation before invoking Livewire::test().tests/Feature/DatabaseServer/CreateTest.php (1)
214-215: PreferVolumefactory states over manualVolume::create(...)fixtures.These cases should use
Volume::factory()->local()/->s3()to stay aligned with test conventions and reduce brittle setup.As per coding guidelines: "When creating models for tests, use the factories for the models. Check if the factory has custom states that can be used before manually setting up the model."🔧 Example refactor
- $localVolume = Volume::create(['name' => 'Local Vol', 'type' => 'local', 'config' => ['path' => '/backups']]); - $s3Volume = Volume::create(['name' => 'S3 Vol', 'type' => 's3', 'config' => ['bucket' => 'test']]); + $localVolume = Volume::factory()->local()->create(['name' => 'Local Vol']); + $s3Volume = Volume::factory()->s3()->create(['name' => 'S3 Vol']);Also applies to: 233-233, 249-249, 260-260, 272-272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/DatabaseServer/CreateTest.php` around lines 214 - 215, Replace direct Volume::create(...) test fixtures with the Volume factory states: change calls to Volume::create(['name'=>..., 'type'=>'local', ...]) and Volume::create(... 'type'=>'s3' ...) to Volume::factory()->local()->create(...) and Volume::factory()->s3()->create(...) (or Volume::factory()->state([...])->create(...) if custom attributes are needed); update all other Volume::create usages in this test to use the factory states so tests follow the factory-based conventions (look for Volume::create occurrences in this test file and swap to Volume::factory()->local()/->s3()).app/Services/Backup/TriggerBackupAction.php (1)
41-42: Replace inline comments with method-level PHPDoc or clearer naming.These inline comments can be removed by relying on the method extraction (
dispatchDiscoveryJob) and existing docblocks.As per coding guidelines "Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Backup/TriggerBackupAction.php` around lines 41 - 42, Replace the inline comment above the dispatchDiscoveryJob call by adding or updating the method-level PHPDoc on dispatchDiscoveryJob (or renaming it for clarity) so the behavior is self-documenting; remove the two-line inline comment and ensure dispatchDiscoveryJob's PHPDoc describes that agent-backed servers in all/pattern mode may return empty snapshots and thus require a discovery job to list databases first, referencing the TriggerBackupAction::dispatchDiscoveryJob call to locate where the comment was removed.app/Console/Commands/AgentRunCommand.php (1)
106-106: Remove inline@phpstan-ignoreand enforce payload typing explicitly.Suppressing the type check here makes payload-shape regressions easier to miss.
As per coding guidelines "Prefer PHPDoc blocks over inline comments. Never use comments within the code itself unless the logic is exceptionally complex."💡 Suggested approach
- $config = BackupConfig::fromPayload($payload, $workingDirectory); // `@phpstan-ignore` argument.type + /** `@var` array<string, mixed> $payload */ + $config = BackupConfig::fromPayload($payload, $workingDirectory);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Console/Commands/AgentRunCommand.php` at line 106, Remove the inline `@phpstan-ignore` on the BackupConfig::fromPayload call and make the payload's shape explicit: update the AgentRunCommand handler so $payload is properly typed/validated before calling BackupConfig::fromPayload (either by adding a PHPDoc block for $payload in the method, introducing a typed DTO/value-object for payload, or calling a validation/normalization routine that returns a typed array/object), and ensure BackupConfig::fromPayload() accepts that explicit type; reference the BackupConfig::fromPayload call and the AgentRunCommand method to locate where to add the PHPDoc/validation and remove the inline ignore.resources/views/livewire/database-server/_form.blade.php (1)
35-36: Move data-fetching logic out of the Blade partial.
$form->getAgentOptions()and$form->getSelectedAgent()are computed in-view. Please expose these from the Livewire class/form state and keep this file markup-focused.As per coding guidelines, "
resources/views/livewire/**/*.blade.php: Blade files should contain only view markup; all PHP logic should be in Livewire component classes."Also applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/database-server/_form.blade.php` around lines 35 - 36, The Blade partial is performing data fetching via $form->getAgentOptions() and $form->getSelectedAgent(); move this logic into the Livewire component (the form class) by adding public properties or computed getters (e.g., public array $agentOptions and public $selectedAgent or getAgentOptionsProperty()/getSelectedAgentProperty()) that populate those values from the component's state, then update the template to reference those properties (e.g., $agentOptions and $selectedAgent) and remove any `@php/data-fetching` calls from the view; also replace the second occurrence that uses getSelectedAgent() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Console/Commands/RunScheduledBackups.php`:
- Around line 78-85: When snapshots are empty the code unconditionally calls
AgentJob::create for a discovery job which can enqueue duplicates; before
creating the job, query AgentJob for the same database_server_id and type
AgentJob::TYPE_DISCOVER with a non-terminal status (e.g.
AgentJob::STATUS_PENDING and any in-progress status your app uses) and only call
AgentJob::create if that exists() check returns false; update the
RunScheduledBackups logic to perform this existence check using the server->id
and AgentJob status constants to prevent enqueuing duplicate in-flight discovery
jobs.
In `@app/Http/Controllers/Api/V1/AgentController.php`:
- Around line 269-270: The payload validation allows duplicate database names;
add the Laravel "distinct" rule to prevent duplicates by updating the validation
for 'databases.*' to include distinct (e.g., 'databases' =>
'required|array|min:1' and 'databases.*' => 'required|string|max:255|distinct'),
and apply the same change to the other validation block referenced (lines
~282-299) so duplicate entries in the incoming 'databases' array are rejected.
In `@resources/views/livewire/database-server/_form.blade.php`:
- Line 260: Replace the static Blade interpolation on the component attribute
label with Blade dynamic binding: update the component attribute where label="{{
__('Troubleshooting Guide') }}" is used to use a colon-prefixed dynamic
attribute and pass the translation helper directly (i.e., use :label with the
translation helper rather than {{ }}), so the translated string is not
double-encoded when rendered.
---
Outside diff comments:
In `@app/Services/Backup/BackupJobFactory.php`:
- Around line 40-47: The current SQLite branch in
BackupJobFactory::createSnapshots() iterates over each database path and creates
multiple snapshots; instead short-circuit for DatabaseType::SQLITE to create a
single snapshot for the whole instance by calling $this->createSnapshot(...)
exactly once (not in a loop) and returning that single-item $snapshots array;
update the createSnapshot call site to pass a single representative database
identifier (e.g., null or the first path) or adjust createSnapshot to accept a
whole-instance indicator so SQLite behaves like Redis (one snapshot total).
---
Duplicate comments:
In `@app/Http/Middleware/ThrottleFailedAgentAuth.php`:
- Around line 22-32: The current pre-auth check using
RateLimiter::tooManyAttempts($key, 10) runs before $next($request) and can block
valid requests; move the throttle decision so you call $response =
$next($request) first, then check the response status and only apply
RateLimiter::hit($key, 300) when $response->getStatusCode() indicates auth
failure (401/403), otherwise call RateLimiter::clear($key); remove or relocate
the initial RateLimiter::tooManyAttempts check so it does not reject requests
before authentication completes.
In `@app/Livewire/Forms/DatabaseServerForm.php`:
- Around line 597-607: The validation currently allows agent_id even when
use_agent is false and validates local_volume without ensuring an agent is
actually selected; update getBaseValidationRules so agent_id cannot be supplied
unless use_agent is true (e.g. make agent_id required_if:use_agent,true or
change nullable to conditional) and tighten the exists rule to also constrain
agent compatibility (use Rule::exists(...)->where(...) to assert the agent
supports the chosen local_volume if applicable); also adjust the
local_volume-related rules to run only when an agent is selected (check
use_agent or agent_id) so a desynced payload cannot persist an incompatible
agent/local-volume combination. Ensure references: getBaseValidationRules,
agent_id, use_agent, local_volume, and the Rule::exists usage are updated
accordingly.
In `@app/Models/AgentJob.php`:
- Around line 122-130: The claim() method in AgentJob is performing a
stale-write on the attempts column by using the model's local state; make the
update conflict-safe by performing an atomic increment/update in the database
instead of setting 'attempts' => $this->attempts + 1 on the model. Modify
AgentJob::claim to issue a DB-level update that sets lease_expires_at,
claimed_at, agent_id, status and uses an atomic expression for attempts (e.g.
DB::raw('attempts + 1') or an equivalent atomic increment) or perform the update
inside a conditional query (where id = $this->id and current lease/state) so
concurrent claimers cannot overwrite each other's increments. Ensure you
reference and update the same fields used now: 'agent_id', 'status',
'lease_expires_at', 'claimed_at', and 'attempts' in the atomic update.
In `@app/Support/helpers.php`:
- Around line 3-5: The helper function agent_mode() currently reads
env('DATABASEMENT_URL') directly; move that env resolution into a config file
(e.g., add a key like databasement_url or databasement.enabled in a relevant
config such as config/app.php or config/services.php using
env('DATABASEMENT_URL') there) and then update agent_mode() to return (bool)
config('...') (use the chosen config key) instead of calling env(...) directly;
ensure the config key name is consistent between the new config entry and the
agent_mode() call.
In `@config/database.php`:
- Line 19: The config 'default' key currently calls the helper agent_mode(),
which pulls env() from app helpers; remove that indirection and resolve the mode
directly in config by replacing agent_mode() with a config or env lookup (e.g.
use config('agent.mode', false) if you have an agent config, or
env('AGENT_MODE', false) here since this is a config file) so the 'default'
entry becomes 'default' => (config('agent.mode', false) ? 'agent' :
env('DB_CONNECTION', 'sqlite')), avoiding use of the agent_mode() helper.
In `@Makefile`:
- Around line 41-46: The current docker aws-cli invocation line "docker run ...
amazon/aws-cli ... s3 mb s3://$(or $(BUCKET),test-bucket) 2>/dev/null || true"
suppresses all errors; replace this by removing the blanket "|| true" and stop
redirecting stderr so failures propagate, and implement an explicit check that
treats only the "bucket already exists/owned" condition as non-fatal while
failing fast for auth/network/endpoint errors; locate the command string shown
above and change it to let the aws-cli error output surface and add logic to
inspect the aws error (or use aws s3api/head-bucket) to ignore the specific
"BucketAlreadyOwnedByYou/BucketAlreadyExists" result but exit non-zero for other
errors.
In `@phpstan.neon`:
- Around line 19-21: Remove the suppression that ignores the "Called 'env'
outside of the config directory" rule in phpstan.neon so the guardrail remains
active; then update app/Support/helpers.php to stop calling env() directly by
refactoring any env('...') usages into config keys (use
config('your.package.key') in the helper) or pass required values from
configuration where the helper is invoked so the helper no longer depends on
env(); ensure references to the env() call sites in helpers.php (and any
functions/classes defined there) are replaced with config-based accessors and
restore the phpstan.neon entry so the rule is enforced.
In `@tests/Feature/Services/Backup/TriggerBackupActionTest.php`:
- Around line 166-170: The test assumes a stable DB order by indexing
$agentJobs[0], which makes it flaky; in TriggerBackupActionTest replace
index-based checks with order-independent assertions on the AgentJob collection
(AgentJob::where(...)->get()) such as asserting the collection has count 2, that
the collection contains the expected TYPE_BACKUP values (e.g., by plucking
'type' and asserting it contains the value twice or that the count of
TYPE_BACKUP equals 2), and that all returned items have non-null snapshot_id
(e.g., assert every item->snapshot_id is not null) so the test no longer depends
on row order.
---
Nitpick comments:
In `@app/Console/Commands/AgentRunCommand.php`:
- Line 106: Remove the inline `@phpstan-ignore` on the BackupConfig::fromPayload
call and make the payload's shape explicit: update the AgentRunCommand handler
so $payload is properly typed/validated before calling BackupConfig::fromPayload
(either by adding a PHPDoc block for $payload in the method, introducing a typed
DTO/value-object for payload, or calling a validation/normalization routine that
returns a typed array/object), and ensure BackupConfig::fromPayload() accepts
that explicit type; reference the BackupConfig::fromPayload call and the
AgentRunCommand method to locate where to add the PHPDoc/validation and remove
the inline ignore.
In `@app/Services/Backup/TriggerBackupAction.php`:
- Around line 41-42: Replace the inline comment above the dispatchDiscoveryJob
call by adding or updating the method-level PHPDoc on dispatchDiscoveryJob (or
renaming it for clarity) so the behavior is self-documenting; remove the
two-line inline comment and ensure dispatchDiscoveryJob's PHPDoc describes that
agent-backed servers in all/pattern mode may return empty snapshots and thus
require a discovery job to list databases first, referencing the
TriggerBackupAction::dispatchDiscoveryJob call to locate where the comment was
removed.
In `@resources/views/livewire/backup-job/_logs-modal.blade.php`:
- Line 157: Move the `$hasDetails` computation out of the Blade by computing it
in the Livewire component that supplies the `$log` data (e.g., in the method
that prepares logs or in render()/mount()), add a boolean property/field on each
log (e.g., `hasDetails`) computed as `$isCommand || !empty($log['context']) ||
$isError`, and update the view to use `{{ $log['hasDetails'] }}` (or the passed
boolean) instead of evaluating `$isCommand || !empty($log['context']) ||
$isError` in the Blade; ensure the Livewire component sets `hasDetails` for
every log before passing the logs collection to the view.
In `@resources/views/livewire/database-server/_form.blade.php`:
- Around line 35-36: The Blade partial is performing data fetching via
$form->getAgentOptions() and $form->getSelectedAgent(); move this logic into the
Livewire component (the form class) by adding public properties or computed
getters (e.g., public array $agentOptions and public $selectedAgent or
getAgentOptionsProperty()/getSelectedAgentProperty()) that populate those values
from the component's state, then update the template to reference those
properties (e.g., $agentOptions and $selectedAgent) and remove any
`@php/data-fetching` calls from the view; also replace the second occurrence that
uses getSelectedAgent() accordingly.
In `@tests/Feature/DatabaseServer/ConnectionStatusTest.php`:
- Around line 29-51: Add an assertion to both tests that the direct provider
method testConnectionForServer is not invoked to lock the early-return contract:
in the ConnectionStatus tests (the Livewire::test cases) set an expectation on
the connection provider/mock that it shouldNotReceive('testConnectionForServer')
so agent-backed checks never fall through to direct provider logic; locate
references to the ConnectionStatus Livewire component and the
testConnectionForServer symbol to add the shouldNotReceive expectation before
invoking Livewire::test().
In `@tests/Feature/DatabaseServer/CreateTest.php`:
- Around line 214-215: Replace direct Volume::create(...) test fixtures with the
Volume factory states: change calls to Volume::create(['name'=>...,
'type'=>'local', ...]) and Volume::create(... 'type'=>'s3' ...) to
Volume::factory()->local()->create(...) and Volume::factory()->s3()->create(...)
(or Volume::factory()->state([...])->create(...) if custom attributes are
needed); update all other Volume::create usages in this test to use the factory
states so tests follow the factory-based conventions (look for Volume::create
occurrences in this test file and swap to Volume::factory()->local()/->s3()).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
CLAUDE.mdMakefileapp/Console/Commands/AgentRunCommand.phpapp/Console/Commands/RunScheduledBackups.phpapp/Enums/DatabaseSelectionMode.phpapp/Enums/DatabaseType.phpapp/Http/Controllers/Api/V1/AgentController.phpapp/Http/Middleware/ThrottleFailedAgentAuth.phpapp/Livewire/Concerns/HasAgentToken.phpapp/Livewire/DatabaseServer/ConnectionStatus.phpapp/Livewire/Forms/DatabaseServerForm.phpapp/Models/AgentJob.phpapp/Models/DatabaseServer.phpapp/Services/Agent/AgentApiClient.phpapp/Services/Agent/AgentAuthenticationException.phpapp/Services/Agent/AgentJobPayloadBuilder.phpapp/Services/Backup/BackupJobFactory.phpapp/Services/Backup/TriggerBackupAction.phpapp/Support/helpers.phpbootstrap/app.phpcomposer.jsonconfig/agent.phpconfig/cache.phpconfig/database.phpconfig/queue.phpconfig/session.phpdatabase/factories/AgentJobFactory.phpdatabase/migrations/2026_02_20_130646_create_agents_and_agent_jobs_tables.phpdocker/php/scripts/start.shphpstan.neonresources/views/livewire/agent/_token-modal.blade.phpresources/views/livewire/backup-job/_logs-modal.blade.phpresources/views/livewire/database-server/_form.blade.phpresources/views/livewire/database-server/index.blade.phproutes/api.phptests/Feature/Agent/AgentApiTest.phptests/Feature/Console/AgentRunCommandTest.phptests/Feature/Console/RunScheduledBackupsTest.phptests/Feature/DatabaseServer/ConnectionStatusTest.phptests/Feature/DatabaseServer/CreateTest.phptests/Feature/Services/Backup/TriggerBackupActionTest.php
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (6)
- app/Services/Agent/AgentJobPayloadBuilder.php
- tests/Feature/Console/RunScheduledBackupsTest.php
- bootstrap/app.php
- config/agent.php
- routes/api.php
- resources/views/livewire/agent/_token-modal.blade.php
Introduce the core data layer for agent-based backups: - Agent model with Sanctum token support and ULID primary keys - AgentJob model with status tracking, lease management, and payload encryption - DatabaseSelectionMode enum replacing raw string values - Migration creating agents and agent_jobs tables with agent_id FK on database_servers - Factories, policy, and query builder for the new models
Add config/agent.php for agent-specific settings (url, token, poll_interval). Update database, cache, queue, and session configs to detect DATABASEMENT_URL and swap to in-memory drivers when running in agent mode. Register EnsureAgentToken and ThrottleFailedAgentAuth middleware in bootstrap/app.php. Update Docker start script to detect agent mode and exec agent:run command. Document agent mode in CLAUDE.md.
Add toPayload() and fromPayload() methods to BackupConfig, DatabaseConnectionConfig, and VolumeConfig DTOs for round-trip serialization into encrypted agent job payloads. Add AgentJobPayloadBuilder service to assemble payloads for backup and discovery jobs. Add AgentAuthenticationException. Fix MySQL localhost DSN to force TCP.
Add AgentController with heartbeat, claim-job, job-heartbeat, ack, fail, and discovered-databases endpoints under v1/agent API routes with Sanctum auth and throttling. Add AgentApiClient for agent-side HTTP communication. Add RecoverAgentLeasesCommand to reset or fail jobs with expired leases, scheduled every minute.
Implement the long-running polling agent command that authenticates via heartbeat, claims jobs, executes backups or discovery using BackupTask, reports progress via job heartbeats, and sends ack/fail responses. Supports --once flag for single-iteration mode and SIGTERM/SIGINT for graceful shutdown.
Update TriggerBackupAction and RunScheduledBackups to route work through agent jobs when a server has an agent assigned. For agent-backed servers in all/pattern selection mode, dispatch discovery jobs since the web app cannot reach the remote database. Update BackupJobFactory to handle agent servers and make createSnapshot() public for post-discovery use. Use DatabaseSelectionMode enum throughout.
Add Index (list with search, pagination, delete), Create (form + token modal), and Edit (form + token regeneration) Livewire components. Introduce HasAgentToken trait for shared token display with Docker command generation. Add AgentForm for validation. Register routes and add Agents menu item to app layout. Include token modal partial with Docker, env vars, and token display tabs.
… status Update DatabaseServer form with agent assignment toggle, agent selection dropdown with online/offline status, volume restrictions for agent-backed servers, and DatabaseSelectionMode enum usage. Update ConnectionStatus component to show agent status instead of connection test when using agents. Fix log detail visibility in backup-job logs modal and ULID-safe comparison in ApiToken/Index.
Remove @ prefix from create-bucket for better debugging visibility and replace fallback echo with || true for cleaner error handling.
David-Crty
force-pushed
the
feat/agent-based-backups-v3
branch
from
97dbf5d to
4b7264b
Compare
Add tests across all layers: Agent API (auth, heartbeat, claim, ack, fail, discovery), Agent CRUD (index, create, edit, delete, access control), API client HTTP tests, agent:run command (config, polling, backup/discovery execution), payload builder, lease recovery, DTO round-trip serialization, and updates to existing ConnectionStatus, CreateServer, RunScheduledBackups, and TriggerBackupAction tests. Use datasets to consolidate ownership checks, log accumulation, SSH tunnel detection, and volume/agent toggle tests.
Block restore for agent-backed servers and display an error notification. Add a "Beta" badge to the Agents menu item for improved visibility.
David-Crty
force-pushed
the
feat/agent-based-backups-v3
branch
from
4b7264b to
406215a
Compare
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
1 participant
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.
Description
This pull request introduces a framework for agent management and job processing. Key features included:
AgentandAgentJobmodels using ULID primary keys.DatabaseConnectionConfigto support agent payload requirements.config/agent.php) for agent-related settings.agentsandagent_jobstables with token compatibility.Checklist
Summary by CodeRabbit
New Features
Data & Config
Tests