refactor: architectural hardening — slog, sentinel errors, prompts init - #171
Merged
Conversation
…it, ContextManager docs - Replace deprecated log package with log/slog (2 call sites) - Add sentinel errors: ErrMaxIterations, ErrToolDenied, ErrLoopDetected, ErrRoleNotFound, ErrToolTimeout, ErrToolNotFound - Fix prompts.go init: replace os.Exit with lazy sync.Once validation - Clean up ContextManager docs: remove stale Phase 1/Phase 2 markers - Remove redundant l.CtxMgr.Messages = msgs from compactFn closure - Wire errors.Is in REPL loop and loop detection test - Format: gofmt -w (fixes pre-existing indentation in loop.go)
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds typed errors for agent and tool failures, updates callers and tests to use ChangesError contracts and resilient diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (19 files)
Reviewed by step-3.7-flash · Input: 119.8K · Output: 19.1K · Cached: 555K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/loop.go`:
- Around line 18-20: Replace the package-level sentinels with comparable typed
errors implementing Is: internal/agent/loop.go:18-20 for max-iteration,
internal/agent/agent_tools.go:20-22 for tool-denied,
internal/agent/pipeline/loopdetect.go:14-16 for loop-detected,
internal/agent/subagent/role.go:10-12 for role-not-found, and
internal/tools/tools.go:72-77 for tool-timeout and tool-not-found. Update all
callers to use zero-value typed targets with errors.Is so existing matching
behavior is preserved, and remove the global sentinel declarations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc25c608-a289-408c-b0a2-207d6cb7215b
📒 Files selected for processing (19)
cmd/yaah/repl_loop.gocmd/yaah/tui.gocmd/yaah/web_view.gointernal/agent/agent_context.gointernal/agent/agent_tools.gointernal/agent/agent_wrapup_test.gointernal/agent/context_manager.gointernal/agent/lifecycle_init.gointernal/agent/loop.gointernal/agent/pipeline/loopdetect.gointernal/agent/subagent/role.gointernal/agent/types.gointernal/prompts/prompts.gointernal/tools/bash.gointernal/tools/git.gointernal/tools/powershell.gointernal/tools/role.gointernal/tools/task.gointernal/tools/tools.go
💤 Files with no reviewable changes (1)
- internal/agent/lifecycle_init.go
Replace six var-sentinel errors with typed error types that implement
the Is method for errors.Is compatibility with zero-value targets:
- ErrMaxIterations → MaxIterationsError
- ErrToolDenied → ToolDeniedError
- ErrLoopDetected → LoopDetectedError (with Tool/Count/Window fields)
- ErrRoleNotFound → RoleNotFoundError
- ErrToolTimeout → ToolTimeoutError (with Tool/Timeout fields)
- ErrToolNotFound → ToolNotFoundError
Callers use errors.Is(err, ErrorType{}) instead of package variables.
All builder values are now constructor-returned, complying with the
no-globals-in-non-command-files convention.
… tests On macOS TempDir returns /var/folders/... which is a symlink to /private/var/folders/... On Windows it may return short names like RUNNER~1. Resolve the workspace and outside directories with EvalSymlinks before using them to construct test paths, and loosen the AskFn path comparison to verify non-empty rather than strict string equality.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/path_validator_test.go`:
- Around line 144-145: Update the assertion in the AskFn test callback to
require gotPath equals the canonical outside path, while retaining the existing
failure behavior for an incorrect or empty path. Use the outside path symbol
already defined by the test rather than only checking gotPath is non-empty.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fe9521d-56fd-442b-9c9e-9e69ab7947b9
📒 Files selected for processing (1)
internal/tools/path_validator_test.go
…cess tests - Replace Write-Host with echo (cross-platform) - Add defer m.Stop() to clean up lingering processes - Replace single time.Sleep in failing command test with poll loop - Accept 'finished' or 'running' status for fast-exit commands
Resolve the outside path through the same filepath.Abs + EvalSymlinks chain that ResolvePath uses internally so the comparison is stable across macOS /var→/private/var symlinks and Windows short-name resolution.
Staticcheck SA5011: defer captures info.ID before nil check in StartFailingCommand. Move nil guards before defer statements so the pointer is proven non-nil when the deferred Stop runs.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/process/process_test.go`:
- Around line 30-33: Reorder cleanup registration in the affected process-start
tests so `err` and `info` are validated before evaluating `info.ID`. Update both
the initial test block and `TestStartFailingCommand`, registering `defer
m.Stop(info.ID)` only after confirming `info` is non-nil.
- Around line 128-145: Update the status check in the polling test around
info.Status so it returns successfully only when status equals "error". Treat
any other status, including non-running terminal states, as unexpected and
preserve the existing diagnostic logging and timeout failure behavior.
- Around line 38-40: Update the test assertion around Info.Status to lock
info.mu, copy info.Status to a local variable, unlock the mutex, and validate
the copied status instead of reading the field unsynchronized.
- Around line 75-79: Update the command passed to m.Start in the process test to
use a PowerShell-compatible separator such as `;` instead of `&&`, while
preserving the existing assertions and cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b08ec15c-ff39-4b1d-bcd8-90766fb374bc
📒 Files selected for processing (1)
internal/process/process_test.go
- Lock info.mu before reading Status in TestStartSimpleCommand - Use ; instead of && for PowerShell 5.1 compatibility in echo test - Require 'error' status (not just non-'running') in poll loop - Keep Start-Sleep tests as-is (require pwsh on runner)
The 500ms sleep wasn't always enough on slow CI runners for the process to complete and flush its output. Poll for a non-'running' status and check logs when the process exits.
Background processes inherited the parent stdin which never closes on CI runners, causing shells (pwsh/sh) to block indefinitely waiting for EOF. Set cmd.Stdin = nil so the child gets an immediate EOF and exits normally. Replaced fixed 500ms sleep in echo test with poll loop for consistent behavior on slow runners.
This was referenced Aug 7, 2026
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.
Summary
Implements the high-impact, low-effort items from the architectural review, plus gofmt fixes for pre-existing formatting issues.
Changes
1. Replace deprecated
logwithlog/sloginternal/agent/agent_context.go:log.Printf→slog.Errorwith structured key-value pairscmd/yaah/tui.go:log.Printf→slog.Warn(keepslogimport forSetOutputto devnull)2. Sentinel errors
internal/agent/loop.go): replaces string-matched"max iterations (%d) reached"— now testable witherrors.Isinternal/agent/agent_tools.go): replaces 2xfmt.Errorf("tool denied")internal/agent/pipeline/loopdetect.go): replaces string-matched loop detection errorinternal/agent/subagent/role.go): wired intotools/role.goandtools/task.gointernal/tools/tools.go): wired intobash.go,git.go,powershell.gointernal/tools/tools.go): replacesfmt.Errorf("unknown tool: %s")repl_loop.go,agent_wrapup_test.go) fromstrings.Contains→errors.Is3. Fix
prompts.go initinit()withsync.Oncelazy validationos.Exit(1)on failureslog.Errorand returns empty strings from accessors4. ContextManager cleanup
l.CtxMgr.Messages = msgsline fromcompactFnclosure (already set byl.compactContext)5. Formatting
gofmt -wapplied to entire project — fixes pre-existing indentation inloop.go,types.go,web_view.goTesting
go build ./...— cleango vet ./...— cleangofmt -l .— emptygo test ./...— all pass (3 pre-existing macOS path validator failures on/varvs/private/var)staticcheck ./...— cleanBreaking changes
None. Sentinel errors wrap the original messages —
errors.Ismatches the new sentinels;err.Error()output is backward-compatible.Summary by CodeRabbit
Bug Fixes
Documentation