feat: [#859] Generate Agent Skills#1412
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1412 +/- ##
==========================================
- Coverage 67.18% 66.86% -0.33%
==========================================
Files 348 351 +3
Lines 25609 26001 +392
==========================================
+ Hits 17206 17386 +180
- Misses 7674 7861 +187
- Partials 729 754 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughIntroduces an AI agent skills system with helpers for version tracking, manifest fetching, and branch resolution. Adds two new console commands: Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Install as Install Command
participant Resolver as Version Resolver
participant Manifest as Manifest Fetcher
participant File as File Fetcher
participant Local as Local Storage
User->>Install: Execute with version/facade args
Install->>Resolver: resolveVersionAndBranch()
alt Version provided
Resolver->>Resolver: Validate & resolve branch
else Auto-detect
Resolver->>Resolver: Parse go.mod
alt go.mod parse fails
Resolver->>Manifest: Fetch available branches
Manifest-->>Resolver: Branch list
Resolver->>User: Prompt for selection
User-->>Resolver: Selected version
end
end
Resolver-->>Install: version, branch
Install->>Manifest: Fetch manifest for branch
alt Empty manifest & not fallback branch
Manifest->>Manifest: Try fallback (master)
end
Manifest-->>Install: ManifestEntry list
Install->>Install: Filter by facades/--all/defaults
Install->>File: Download files concurrently
File-->>Install: File contents ([]byte)
Install->>Local: Read existing .version file
Install->>Local: Merge metadata & update SHA256s
Install->>Local: Write files to disk
Install->>Local: Write updated .version
Install->>User: Report success summary
sequenceDiagram
participant User as User/CLI
participant Update as Update Command
participant Local as Local Storage
participant Manifest as Manifest Fetcher
participant File as File Fetcher
participant Conflict as Conflict Detector
User->>Update: Execute with facade/--all args
Update->>Local: Read .version file
alt No .version
Update->>User: Error: not installed
else Version exists
Update->>Update: Validate version support
alt Unsupported
Update->>User: Error: upgrade Goravel
else Supported
Update->>Manifest: Fetch manifest
Manifest-->>Update: ManifestEntry list
Update->>Update: Filter installed entries
loop For each target entry
Update->>File: Fetch upstream file
File-->>Update: Content
Update->>Local: Read local file
Local-->>Update: Content + SHA256
Update->>Conflict: Compare hashes
alt Hashes match
Conflict-->>Update: Up-to-date
else Local modified, upstream same
Conflict-->>Update: Skipped
else Both modified & no --force
Conflict-->>Update: Conflict
else Can update
Update->>Local: Write file
Update->>Local: Update SHA256
Conflict-->>Update: Updated
end
end
Update->>Local: Write merged .version
Update->>User: Report summary (updated/skipped/conflicts/up-to-date)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ai/console/helpers_test.go`:
- Around line 38-42: The test currently ignores errors from os.Remove and
f.Close which triggers linter failures; update the teardown to check those
returns (e.g., replace defer os.Remove(f.Name()) with a deferred closure that
captures and asserts the error, and change f.Close() to capture its error and
call assert.NoError(t, err) or require.NoError(t, err)); reference the test
helper where the file is created (the variable f and the calls os.Remove and
f.Close in helpers_test.go) and ensure both removal and close errors are
asserted so the linter is satisfied and failures surface in the test runner.
In `@ai/console/helpers.go`:
- Line 156: The deferred call "defer f.Close()" leaves the Close error
unchecked; replace it with a deferred closure that captures and handles the
error (e.g., defer func() { if err := f.Close(); err != nil { /* log or
propagate the error */ } }()) so the Close() failure is either logged via the
package logger or returned to the caller; locate the occurrence of "defer
f.Close()" in helpers.go and update it to use the deferred anonymous function
handling f.Close()'s error.
- Line 176: The deferred resp.Body.Close() call currently ignores its returned
error; change the defer to wrap Close in an inline function that checks the
error (e.g., defer func() { if err := resp.Body.Close(); err != nil { /* log or
handle error */ } }()), and either log the error via the package logger or
propagate it if the surrounding function returns an error. Locate the defer
usage where resp.Body.Close() is called (the resp variable) and replace it with
this checked-close pattern.
- Line 109: The deferred resp.Body.Close() call currently ignores its returned
error; modify the function containing that line to capture and handle Close()'s
error (e.g., assign the result of resp.Body.Close(), check if err != nil and
either log it via the existing logger or return/wrap it up the call chain).
Locate the resp variable and the function that performs the HTTP request in
ai/console/helpers.go, replace the bare defer resp.Body.Close() with a deferred
closure or explicit check that captures the error and handles it consistently
with the function's error-handling strategy (log/return/wrap) so the linter no
longer flags an unchecked error.
In `@ai/console/install_command_test.go`:
- Around line 24-27: The cleanup function currently ignores errors from
os.RemoveAll(".ai") and os.Remove("AGENTS.md"), causing static analysis
failures; update the cleanup closure to capture and handle those errors (e.g.,
assign err := os.RemoveAll(".ai") and err := os.Remove("AGENTS.md") and either
ignore NotExist via if err != nil && !os.IsNotExist(err) { t.Fatalf(...) } or
explicitly discard with _ = os.RemoveAll(".ai") / _ = os.Remove("AGENTS.md")
depending on desired behavior) so the unchecked-return lint is resolved while
keeping references to the cleanup function and the two remove calls.
- Around line 160-161: The test setup is ignoring errors from os.MkdirAll and
os.WriteFile; update the test to check those returns (e.g., use require.NoError
or assert.NoError from testify) and fail the test on error. Capture the
testing.T instance in the closure or move the setup into the top-level test
function so you can call require.NoError(t, err) for the os.MkdirAll and
os.WriteFile calls that create ".ai" and write versionFilePath
(`{"version":"v1.16","files":{}}`). Ensure both calls propagate or assert
failures so pipeline static analysis and CI will detect filesystem errors.
In `@ai/console/update_command_test.go`:
- Line 14: The os.RemoveAll(".ai") call has an unchecked error; capture its
return (e.g., err := os.RemoveAll(".ai")) and handle it in the test by failing
the test on error (e.g., use t.Fatalf/t.Fatal or your test helper like
require.NoError) so the linter is satisfied and the pipeline won't fail; update
the occurrence in update_command_test.go where os.RemoveAll(".ai") is invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9043236-8a39-4ae6-b6bf-51b147ae8a59
📒 Files selected for processing (7)
ai/console/helpers.goai/console/helpers_test.goai/console/install_command.goai/console/install_command_test.goai/console/update_command.goai/console/update_command_test.goai/service_provider.go
hwbrzzl
left a comment
There was a problem hiding this comment.
Thanks, awesome. I haven't reviewed this PR carefully. I'll re-review it after the document PR is merged. And please add more details in the PR description after that.
| &command.StringFlag{ | ||
| Name: "version", | ||
| Usage: "Force a specific version (e.g. v1.17)", | ||
| }, |
There was a problem hiding this comment.
This flag may be unnecessary. We should check the version according to the go.mod file.
There was a problem hiding this comment.
That is also there, but in case the user wants to get something specific, the priority is: first from the version, then auto-detect from the mod file, and then the interactive setup.
There was a problem hiding this comment.
Got it, I'd suggest adding the flag when users actually need it.
There was a problem hiding this comment.
I added this for my testing purpose for now as we don't publish v1.17 docs as separate branch. So I had to add so that I can pull the master.
📑 Description
Closes goravel/goravel#859
Summary by CodeRabbit
agents:installcommand to install AI agent skill files for your Goravel version with selective facade installation or bulk installation optionsagents:updatecommand to keep installed agents current, with conflict detection and resolution handling✅ Checks