-
Notifications
You must be signed in to change notification settings - Fork 224
fix: correct memory database cache path for pr-reviewer GHA #1489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: correct memory database cache path for pr-reviewer GHA #1489
Conversation
|
/review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
Found 1 critical compilation error in the changed code.
cmd/root/push.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("resolving agent file: %w", err) | ||
| } | ||
| agentSource := config.Resolve(agentFilename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ COMPILATION ERROR: Missing error handling for config.Resolve()
The config.Resolve() function returns two values (Source, error) as defined in pkg/config/resolve.go:
func Resolve(agentFilename string) (Source, error)In Go, you cannot assign a two-value return to a single variable. This code will fail to compile with an error like:
assignment mismatch: 1 variable but config.Resolve returns 2 values
The previous code correctly handled both return values:
agentSource, err := config.Resolve(agentFilename)
if err != nil {
return fmt.Errorf("resolving agent file: %w", err)
}This error handling is necessary because Resolve() can fail in multiple scenarios:
- When
filepath.Abs()fails - When resolving sources fails in
ResolveSources()
You need to restore the error handling or the PR will not build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good find!
This reverts commit 265d66b.
|
/review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
✅ No issues found - All path changes are consistent and correctly address the cache path mismatch.
The PR correctly fixes the problem by:
- Updating agent configs to use relative path
pr-review-memory.db(resolves from agent config directory) - Updating all workflow cache paths to absolute path
.github/workflows/agents/pr-review-memory.db - Ensuring both resolve to the same location for proper cache operation
All 4 workflow cache locations and both agent configs are consistently updated.
Summary
.github/directoryProblem
The workflow was attempting to cache
.github/pr-review-memory.db, but cagent was actually creating the file at.github/workflows/agents/.github/pr-review-memory.db.This happened because:
cagent-actionchanges directory withcdbut doesn't pass--working-dirto cagent.github/pr-review-memory.db+ parent dir.github/workflows/agents/=.github/workflows/agents/.github/pr-review-memory.dbChanges
pr-review.yaml,pr-review-feedback.yaml) to usepr-review-memory.dbinstead of.github/pr-review-memory.dbpr-review.ymlto.github/workflows/agents/pr-review-memory.db