refactor: break tools→agent/subagent cycle, extract control types, un… - #176
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR centralizes agent loop construction, adds forced compaction, validates configuration during startup, introduces canonical control messages, extracts role resolution behind interfaces, and moves provider error stubs into shared packages. ChangesAgent loop and compaction
Control and role contracts
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CompactCommand
participant LoopBuilder
participant AgentLoop
participant CompactProvider
CompactCommand->>LoopBuilder: Build compact loop
LoopBuilder-->>CompactCommand: configured AgentLoop
CompactCommand->>AgentLoop: ForceCompact
AgentLoop->>CompactProvider: compact messages
CompactProvider-->>AgentLoop: compacted messages
AgentLoop-->>CompactCommand: updated messages
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/yaah/compact_cmd.go (1)
78-97: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe before and after token estimates use different formulas.
Lines 49-55 count
Content,ReasoningContent, and tool call arguments. Lines 78-81 count onlyContent. The reported reduction therefore includes text that was never compacted away. The status line overstates the saving.Line 93 also decides success by message count. Compaction can reduce tokens while keeping the message count, and the command then reports "no messages were compacted".
Extract one estimator and compare token counts.
🔧 Proposed fix
+// estimateTokens approximates the token cost of a message slice. +func estimateTokens(msgs []types.Message) int { + n := 0 + for _, m := range msgs { + n += len(m.Content)/4 + len(m.ReasoningContent)/4 + for _, tc := range m.ToolCalls { + n += len(tc.Function.Arguments)/4 + len(tc.Function.Name)/4 + } + } + return n +}- estTokens := 0 - for _, m := range s.messages { - estTokens += len(m.Content)/4 + len(m.ReasoningContent)/4 - for _, tc := range m.ToolCalls { - estTokens += len(tc.Function.Arguments)/4 + len(tc.Function.Name)/4 - } - } + estTokens := estimateTokens(s.messages) @@ - newEstimate := 0 - for _, m := range s.messages { - newEstimate += len(m.Content) / 4 - } + newEstimate := estimateTokens(s.messages) @@ - if len(s.messages) < beforeMsgs { + if newEstimate < estTokens { msg(fmt.Sprintf("compacted: %d/%d tokens (%d%%)", newEstimate, window, newEstimate*100/window)) } else { msg("no messages were compacted (context too small or compaction ineffective)") }
beforeMsgsat line 73 then becomes unused; remove it.🤖 Prompt for 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. In `@cmd/yaah/compact_cmd.go` around lines 78 - 97, Extract a shared token-estimation helper from the existing before-count logic, including Content, ReasoningContent, and tool-call arguments, and use it both before and after compaction. In the status logic, compare the before and after token estimates rather than message counts, while preserving the existing reporting messages and removing the now-unused beforeMsgs variable.
🧹 Nitpick comments (6)
internal/agent/agent_context.go (1)
408-415: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDocument that
ForceCompactis not safe during a concurrentRun.Lines 412-415 temporarily set
l.Viewto nil and callapplyDefaults. If another goroutine runsRunon the same loop, it can observe the nilView. Every current caller builds a fresh loop, so no defect exists today. Add a doc note so callers keep that contract.🤖 Prompt for 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. In `@internal/agent/agent_context.go` around lines 408 - 415, Document on the ForceCompact method that it must not be called concurrently with Run because it temporarily sets l.View to nil; state that callers must use a loop that is not running concurrently.cmd/yaah/compact_cmd.go (1)
4-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the doc comment to name the exported entry point.
Lines 4 and 21 refer to
agent.Loop.compactContext. That method is unexported and is not reachable from this package. The command callsloop.ForceCompactat line 74. NameForceCompactso the comment matches the code.🤖 Prompt for 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. In `@cmd/yaah/compact_cmd.go` around lines 4 - 22, Update the compact command’s doc comment to refer to the exported agent.Loop.ForceCompact entry point instead of the unexported compactContext method, including both references in the comment while preserving the existing description.cmd/yaah/config_parity_test.go (2)
78-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a range loop over the field count and handle embedded structs.
Two points:
- The coding guidelines prefer
for ... rangeover index-based loops. Go 1.22 and newer supportfor i := range t.NumField().structFieldsskips promoted fields of embedded structs. If eitherconfig.Defaultsoragent.AgentConfiggains an embedded struct, the parity check silently ignores those fields.♻️ Proposed change
func structFields(t reflect.Type) map[string]string { m := make(map[string]string) - for i := 0; i < t.NumField(); i++ { + for i := range t.NumField() { f := t.Field(i) if !f.IsExported() { continue } + if f.Anonymous && f.Type.Kind() == reflect.Struct { + for name, typ := range structFields(f.Type) { + m[name] = typ + } + continue + } m[f.Name] = f.Type.String() } return m }As per coding guidelines: "prefer
for ... rangeover index-based loops".🤖 Prompt for 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. In `@cmd/yaah/config_parity_test.go` around lines 78 - 88, Update structFields to iterate over t.NumField() with a range loop, and recursively include fields from exported embedded structs so promoted fields from config.Defaults or agent.AgentConfig are represented in the parity map. Preserve skipping unexported fields and ensure embedded fields do not get silently omitted.Source: Coding guidelines
127-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every mapped field instead of a manual subset.
The test sets 20 fields but asserts 10. The stated purpose is to catch silent zero-value drops. The unasserted fields, such as
RawCompactionThreshold,CompactMaxMessages,WrapUpThreshold,MaxInlineToolsPerTurn,ReasoningProtect,ToolResultMaxLines,ToolResultMaxBytes, and thePrune*values, are exactly where a dropped mapping stays hidden. Add the remaining assertions, or compareloop.Configto the expectedagent.AgentConfigwith reflection.🤖 Prompt for 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. In `@cmd/yaah/config_parity_test.go` around lines 127 - 143, Expand the parity checks in the test to cover all 20 fields assigned in the setup, including RawCompactionThreshold, CompactMaxMessages, WrapUpThreshold, MaxInlineToolsPerTurn, ReasoningProtect, ToolResultMaxLines, ToolResultMaxBytes, and every Prune* field. Keep the existing checks and expected values, or compare the complete loop.Config against the expected agent.AgentConfig using reflection.internal/config/validate.go (1)
8-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider returning a pointer-free error that supports
errors.Ason both forms.
ValidatereturnsValidationErroras a value. Callers that writeerrors.As(err, &ve)with*ValidationErrorwill not match. The current value receiver onError()is consistent, so this works today. Document the value form, or add a small constructor, so callers use one form only.🤖 Prompt for 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. In `@internal/config/validate.go` around lines 8 - 20, Standardize how callers match ValidationError from Validate so errors.As reliably supports the returned value form. Document that callers must target ValidationError rather than *ValidationError, or add a constructor/API that consistently returns and matches one form; preserve the existing value-receiver Error behavior.internal/config/validate_test.go (1)
20-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific validation message, not only that an error occurred.
Each test checks
err != nil. A failure from an unrelated rule would still pass. Assert the expected substring so each test proves the intended rule fired. Consider grouping the cases witht.Runsubtests over a table.♻️ Example for one case
func TestValidateInvalidApproval(t *testing.T) { cfg := defaultConfig() cfg.Agent.Default.Approval = "banana" err := Validate(cfg) if err == nil { t.Fatal("invalid approval mode should fail") } + if !strings.Contains(err.Error(), "approval") { + t.Errorf("expected approval error, got %v", err) + } }Based on the coding guideline "Place tests next to the code they test and use
t.Run("name", func(t *testing.T) { ... })for subtests".🤖 Prompt for 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. In `@internal/config/validate_test.go` around lines 20 - 85, Update the validation tests in TestValidateInvalidApproval, TestValidateCompactionThresholdRange, TestValidateNegativeContextWindow, TestValidateFallbackProviderMissing, TestValidateSubAgentProviderMissing, and TestValidateMCPServerInvalidTransport to assert that err contains the specific expected validation message, not merely that it is non-nil. Preserve the no-error assertion in TestValidateMCPServerNoCommandOrURL, and optionally consolidate related cases with named t.Run subtests.Source: Coding guidelines
🤖 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 `@cmd/yaah/build_loop.go`:
- Around line 107-115: Remove the unused agent.View parameter from
agentSession.loopBuilder and update all three call sites in the build, serve,
and compact command flows to stop passing the view argument, leaving
agent.LoopBuildOptions.View wiring unchanged.
In `@cmd/yaah/compact_cmd.go`:
- Line 74: Update the :compact command’s ForceCompact call to use a context with
a finite timeout instead of context.Background(). Define compactTimeout locally
or obtain the configured timeout, derive the context with that duration, and
pass it to loop.ForceCompact while preserving the existing compaction behavior.
In `@cmd/yaah/serve.go`:
- Line 221: In the serve flow before calling s.loopBuilder, snapshot s.provider
and s.modelName while holding s.mu.RLock, then use those local values when
constructing the loop. Follow the locking pattern already used by runPrompt, and
keep the existing outer-mutex release before runHeadless unchanged.
In `@internal/agent/agent_context.go`:
- Around line 426-431: Update ForceCompact around the temporary LastPromptTokens
assignment to save its original value, set the forced context-window value for
compactContext, then restore the saved value after compaction completes. Ensure
restoration occurs before returning so subsequent automatic checks and reused
calls observe the original token estimate.
- Around line 420-424: Handle and propagate or log the error returned by
SetCompactionCooldown in the context reset flow, while preserving the existing
state reset. Ensure compactContext does not silently continue when clearing the
persisted cooldown fails.
In `@internal/agent/builder.go`:
- Around line 60-61: Remove the unused LoopBuilder.QualityGates field and delete
the corresponding QualityGates initializer in cmd/yaah/build_loop.go, leaving
WithAgentConfig and Cfg.QualityGates as the single configuration path consumed
by Build.
- Around line 66-88: Update the comments on LoopBuildOptions.OtelEnabled and
ApprovalMode to match Build: document that nil OtelEnabled defaults to false,
and that an empty ApprovalMode leaves the loop’s default unchanged because
WithApprovalMode is omitted.
In `@internal/config/validate.go`:
- Around line 46-74: Update the comments above the compaction threshold,
MaxLoopCycles, and EstimateFactor checks to accurately state that zero is
allowed and only negative values are rejected; leave the existing validation
conditions unchanged.
In `@internal/tools/role_resolver.go`:
- Around line 8-30: Remove the global ErrRoleNotFound sentinel and the
RoleNotFoundError.Is method. Add an IsRoleNotFound(error) bool helper that uses
errors.As to detect a RoleNotFoundError, while retaining RoleNotFoundError.Error
and exact comparable-error matching so differing roles do not match.
---
Outside diff comments:
In `@cmd/yaah/compact_cmd.go`:
- Around line 78-97: Extract a shared token-estimation helper from the existing
before-count logic, including Content, ReasoningContent, and tool-call
arguments, and use it both before and after compaction. In the status logic,
compare the before and after token estimates rather than message counts, while
preserving the existing reporting messages and removing the now-unused
beforeMsgs variable.
---
Nitpick comments:
In `@cmd/yaah/compact_cmd.go`:
- Around line 4-22: Update the compact command’s doc comment to refer to the
exported agent.Loop.ForceCompact entry point instead of the unexported
compactContext method, including both references in the comment while preserving
the existing description.
In `@cmd/yaah/config_parity_test.go`:
- Around line 78-88: Update structFields to iterate over t.NumField() with a
range loop, and recursively include fields from exported embedded structs so
promoted fields from config.Defaults or agent.AgentConfig are represented in the
parity map. Preserve skipping unexported fields and ensure embedded fields do
not get silently omitted.
- Around line 127-143: Expand the parity checks in the test to cover all 20
fields assigned in the setup, including RawCompactionThreshold,
CompactMaxMessages, WrapUpThreshold, MaxInlineToolsPerTurn, ReasoningProtect,
ToolResultMaxLines, ToolResultMaxBytes, and every Prune* field. Keep the
existing checks and expected values, or compare the complete loop.Config against
the expected agent.AgentConfig using reflection.
In `@internal/agent/agent_context.go`:
- Around line 408-415: Document on the ForceCompact method that it must not be
called concurrently with Run because it temporarily sets l.View to nil; state
that callers must use a loop that is not running concurrently.
In `@internal/config/validate_test.go`:
- Around line 20-85: Update the validation tests in TestValidateInvalidApproval,
TestValidateCompactionThresholdRange, TestValidateNegativeContextWindow,
TestValidateFallbackProviderMissing, TestValidateSubAgentProviderMissing, and
TestValidateMCPServerInvalidTransport to assert that err contains the specific
expected validation message, not merely that it is non-nil. Preserve the
no-error assertion in TestValidateMCPServerNoCommandOrURL, and optionally
consolidate related cases with named t.Run subtests.
In `@internal/config/validate.go`:
- Around line 8-20: Standardize how callers match ValidationError from Validate
so errors.As reliably supports the returned value form. Document that callers
must target ValidationError rather than *ValidationError, or add a
constructor/API that consistently returns and matches one form; preserve the
existing value-receiver Error behavior.
🪄 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: 500cebbb-5c67-40bb-af3a-f21c38b93483
📒 Files selected for processing (27)
AGENTS.mdcmd/yaah/build_loop.gocmd/yaah/compact_cmd.gocmd/yaah/config.gocmd/yaah/config_parity_test.gocmd/yaah/provider_resolve.gocmd/yaah/serve.gocmd/yaah/wiring.gointernal/agent/agent.gointernal/agent/agent_context.gointernal/agent/agent_hooks.gointernal/agent/builder.gointernal/agent/persist.gointernal/agent/runner/role_resolver.gointernal/agent/runner/runner.gointernal/config/validate.gointernal/config/validate_test.gointernal/control/control.gointernal/mcp/http_server.gointernal/memory/memory.gointernal/providers/stubs.gointernal/tools/list_subagents.gointernal/tools/role.gointernal/tools/role_resolver.gointernal/tools/task.gointernal/tools/task_test.gointernal/types/control.go
💤 Files with no reviewable changes (1)
- internal/agent/agent_hooks.go
Comment on lines
+420
to
+424
| cm := l.ctxMgr() | ||
| cm.State.IneffectiveCompactions = 0 | ||
| if cm.SessionID != "" && cm.DB != nil { | ||
| cm.DB.SetCompactionCooldown(cm.SessionID, 0, 0) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the SetCompactionCooldown signature and existing call sites.
ast-grep run --pattern 'func ($_ $_) SetCompactionCooldown($$$) $$$ { $$$ }' --lang go
rg -nP --type=go -C3 'SetCompactionCooldown'Repository: buchenberg/yaah
Length of output: 675
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target context ---'
sed -n '380,445p' internal/agent/agent_context.go
printf '%s\n' '--- all call sites ---'
rg -n -P --type=go -C4 'SetCompactionCooldown\s*\(' .
printf '%s\n' '--- repository error-handling patterns ---'
rg -n -P --type=go -C3 'SetCompactionCooldown|IneffectiveCompactions' internalRepository: buchenberg/yaah
Length of output: 14413
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- compaction guard and persistence ---'
sed -n '235,285p' internal/agent/context_manager.go
sed -n '440,465p' internal/agent/context_manager.go
printf '%s\n' '--- context manager state loading and database access ---'
rg -n -P -C4 'persisterDB|GetCompactionCooldown|SessionID|IneffectiveCompactions' internal/agent/context_manager.go internal/agent/*.go
printf '%s\n' '--- Loop error/reporting fields and ForceCompact callers ---'
rg -n -P -C3 'type Loop struct|Logger|log\.|ForceCompact\s*\(' internal/agentRepository: buchenberg/yaah
Length of output: 31077
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
# Read-only behavioral probe for the control flow shown in compactContext.
def compact_context(state_ineffective, persisted_cooldown, now, last_compaction_tokens=0, estimated=0):
if state_ineffective >= 2 and last_compaction_tokens > 0 and estimated >= last_compaction_tokens * 3 // 2:
state_ineffective = 0
if state_ineffective >= 2:
return "skipped_by_ineffective_state"
if persisted_cooldown > 0 and now < persisted_cooldown:
return "skipped_by_persisted_cooldown"
return "compaction_started"
# ForceCompact clears only in-memory state. A failed reset leaves the persisted
# future cooldown unchanged before compactContext reads it.
result = compact_context(
state_ineffective=0,
persisted_cooldown=2_000,
now=1_000,
)
assert result == "skipped_by_persisted_cooldown", result
print("stale persisted cooldown causes the forced compaction pass to be skipped")
PY
printf '%s\n' '--- available logging conventions ---'
rg -n -P -C2 --type=go 'slog\.(Error|Warn)\(' internal/agent | head -80Repository: buchenberg/yaah
Length of output: 803
Handle the SetCompactionCooldown error.
If the reset fails, the persisted cooldown remains active and compactContext can skip the forced pass silently.
🤖 Prompt for 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.
In `@internal/agent/agent_context.go` around lines 420 - 424, Handle and propagate
or log the error returned by SetCompactionCooldown in the context reset flow,
while preserving the existing state reset. Ensure compactContext does not
silently continue when clearing the persisted cooldown fails.
…ify compaction Architecture review remediation addressing dependency cycles, god-package concerns, and business-logic leakage into cmd/. Key changes: - Break tools → agent/subagent design cycle: introduce tools.RoleResolver interface and tools-local types (RoleNotFoundError, ContractField, RoleEntry). agent/runner.SubAgentRoleResolver bridges to the subagent RoleRegistry. internal/tools no longer imports agent/subagent. - Extract control-plane types to internal/control: CtrlMsg and related types now canonical in internal/control; internal/types/control.go holds type aliases for backward compatibility. internal/types no longer imports internal/todo — the de-facto core is dependency-free. - Unify :compact command with in-loop compactor: replace the standalone parallel summarizer in compact_cmd.go with loop.ForceCompact(), which shares cooldowns, adaptive budgets, chunked fallback, and events. ForceCompact nils View to avoid creating a broker/forwarder goroutine, clears cooldowns/ineffective-compaction state so the explicit request always runs, and flushes the persister afterward. - Extract LoopBuilder in internal/agent: eliminates ~70 lines of duplicated agent.NewLoop option construction between build_loop.go and serve.go. - Move provider stubs to internal/providers: NoProviderStub and OAuthErrorStub are provider implementations, not CLI wiring. - Add config.Validate() for fail-fast startup: validates approval mode, compaction thresholds, context window, fallback/subagent providers, and MCP transport. Wired into newAgentSessionWithOptions. - Log swallowed errors: DB open (wiring.go), MCP notification dispatch (http_server.go). config edit now falls back to ResolveEditor(nil) on load error instead of hard-failing. - Inline agent_hooks.go into persist.go; add package docs to internal/agent and internal/memory. Document globals in AGENTS.md (tuiMCPBuf, defaultRoleReg, OTel metrics). - Add config struct parity test (config.Defaults ↔ agent.AgentConfig) and LoopBuilder roundtrip test to catch silent drift. All tests pass; vet, staticcheck, and gofmt clean.
buchenberg
force-pushed
the
refactor/architecture-review-findings
branch
from
August 7, 2026 04:04
f5409af to
1a3b73f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/config/validate.go`:
- Around line 103-110: Extend the QualityGates validation loop in the config
validator to inspect each validator role string and reject empty names, not just
empty validator lists. Add an error for any empty role while preserving the
existing no-validator-list validation and error path.
In `@internal/process/process.go`:
- Around line 66-78: The Windows shell selection in the process setup must not
retain the default sh fallback. Update the logic around runtime.GOOS and
exec.LookPath to select pwsh or powershell only, and return an error when
neither is available; add a Windows-specific test covering the missing-shell
case.
In `@internal/tools/role_resolver.go`:
- Around line 23-37: Update IsRoleNotFound to use an explicit marker interface
implemented by both tools.RoleNotFoundError and
agent/subagent.RoleNotFoundError, allowing errors.As to recognize wrapped value
and pointer errors. Remove the "role " error-string prefix fallback and retain
classification only through the marker.
🪄 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: d4f3352a-2bf9-435e-9f5b-ebd958859e41
📒 Files selected for processing (28)
AGENTS.mdcmd/yaah/build_loop.gocmd/yaah/compact_cmd.gocmd/yaah/config.gocmd/yaah/config_parity_test.gocmd/yaah/provider_resolve.gocmd/yaah/serve.gocmd/yaah/wiring.gointernal/agent/agent.gointernal/agent/agent_context.gointernal/agent/agent_hooks.gointernal/agent/builder.gointernal/agent/persist.gointernal/agent/runner/role_resolver.gointernal/agent/runner/runner.gointernal/config/validate.gointernal/config/validate_test.gointernal/control/control.gointernal/mcp/http_server.gointernal/memory/memory.gointernal/process/process.gointernal/providers/stubs.gointernal/tools/list_subagents.gointernal/tools/role.gointernal/tools/role_resolver.gointernal/tools/task.gointernal/tools/task_test.gointernal/types/control.go
💤 Files with no reviewable changes (1)
- internal/agent/agent_hooks.go
🚧 Files skipped from review as they are similar to previous changes (23)
- internal/memory/memory.go
- internal/tools/task.go
- internal/agent/persist.go
- internal/mcp/http_server.go
- internal/agent/agent.go
- internal/tools/task_test.go
- cmd/yaah/config.go
- cmd/yaah/compact_cmd.go
- internal/config/validate_test.go
- internal/tools/list_subagents.go
- cmd/yaah/provider_resolve.go
- internal/agent/runner/role_resolver.go
- AGENTS.md
- internal/agent/agent_context.go
- internal/types/control.go
- internal/providers/stubs.go
- internal/agent/builder.go
- internal/agent/runner/runner.go
- cmd/yaah/config_parity_test.go
- internal/control/control.go
- cmd/yaah/build_loop.go
- cmd/yaah/wiring.go
- internal/tools/role.go
Comment on lines
+103
to
+110
| // Quality gate validator roles must be non-empty strings. | ||
| for role, validators := range cfg.Agent.QualityGates { | ||
| if len(validators) == 0 { | ||
| errs = append(errs, fmt.Sprintf( | ||
| "agents.quality_gates.%s has no validator roles", | ||
| role)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject empty validator role names.
Line 105 only rejects an empty validator list. A list such as [""] passes validation even though Line 103 requires non-empty validator roles.
Proposed fix
for role, validators := range cfg.Agent.QualityGates {
if len(validators) == 0 {
errs = append(errs, fmt.Sprintf(
"agents.quality_gates.%s has no validator roles",
role))
}
+ for _, validator := range validators {
+ if validator == "" {
+ errs = append(errs, fmt.Sprintf(
+ "agents.quality_gates.%s contains an empty validator role",
+ role))
+ }
+ }
}📝 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
| // Quality gate validator roles must be non-empty strings. | |
| for role, validators := range cfg.Agent.QualityGates { | |
| if len(validators) == 0 { | |
| errs = append(errs, fmt.Sprintf( | |
| "agents.quality_gates.%s has no validator roles", | |
| role)) | |
| } | |
| } | |
| // Quality gate validator roles must be non-empty strings. | |
| for role, validators := range cfg.Agent.QualityGates { | |
| if len(validators) == 0 { | |
| errs = append(errs, fmt.Sprintf( | |
| "agents.quality_gates.%s has no validator roles", | |
| role)) | |
| } | |
| for _, validator := range validators { | |
| if validator == "" { | |
| errs = append(errs, fmt.Sprintf( | |
| "agents.quality_gates.%s contains an empty validator role", | |
| role)) | |
| } | |
| } | |
| } |
🤖 Prompt for 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.
In `@internal/config/validate.go` around lines 103 - 110, Extend the QualityGates
validation loop in the config validator to inspect each validator role string
and reject empty names, not just empty validator lists. Add an error for any
empty role while preserving the existing no-validator-list validation and error
path.
Comment on lines
+66
to
+78
| // Use powershell on Windows, sh on Unix for the shell wrapper. | ||
| // On Unix we must NOT prefer pwsh even if it is on PATH (GitHub Actions | ||
| // ubuntu runners include pwsh, whose startup is ~1-2s and causes | ||
| // trivial commands like `echo` to blow past test timeouts). | ||
| shell, shellFlag := "sh", "-c" | ||
| if _, err := exec.LookPath("pwsh"); err == nil { | ||
| shell = "pwsh" | ||
| shellFlag = "-Command" | ||
| } else if _, err := exec.LookPath("powershell"); err == nil { | ||
| shell = "powershell" | ||
| shellFlag = "-Command" | ||
| if runtime.GOOS == "windows" { | ||
| if _, err := exec.LookPath("pwsh"); err == nil { | ||
| shell = "pwsh" | ||
| shellFlag = "-Command" | ||
| } else if _, err := exec.LookPath("powershell"); err == nil { | ||
| shell = "powershell" | ||
| shellFlag = "-Command" | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'internal/process/process.go' 'internal/process/*' 'internal/tools/background_process.go' '*process*test*' | sed -n '1,120p'
printf '%s\n' '--- process outline ---'
ast-grep outline internal/process/process.go --view compact
printf '%s\n' '--- relevant source ---'
cat -n internal/process/process.go | sed -n '1,150p'
printf '%s\n' '--- shell/process references ---'
rg -n --glob '*.go' 'LookPath|shellFlag|Manager\.Start|Start\(' internal/process internal/tools | sed -n '1,240p'Repository: buchenberg/yaah
Length of output: 450
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- process source ---'
sed -n '1,150p' internal/process/process.go
printf '%s\n' '--- tests and related callers ---'
git ls-files | rg '(^|/)(process|background_process).*(_test)?\.go$' | xargs -r -n1 sh -c 'echo "--- $0"; sed -n "1,220p" "$0"'
rg -n --glob '*.go' 'exec\.Command|Start\(|shellFlag|LookPath|runtime\.GOOS' internal | sed -n '1,260p'Repository: buchenberg/yaah
Length of output: 25314
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1,150p' internal/process/process.go
rg -n --glob '*.go' 'LookPath|shellFlag|exec\.Command|Manager\.Start|runtime\.GOOS' internalRepository: buchenberg/yaah
Length of output: 7915
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- remaining process tests ---'
cat -n internal/process/process_test.go | sed -n '180,360p'
printf '%s\n' '--- background-process and shell contract references ---'
rg -n -i --glob '*.go' --glob '*.md' --glob '*.yaml' --glob '*.yml' \
'background_process|Start-Sleep|powershell|pwsh|shell wrapper|shell command|PowerShell' . \
| sed -n '1,260p'
printf '%s\n' '--- related shell implementation ---'
cat -n internal/tools/bash.go | sed -n '1,110p'
printf '%s\n' '--- standard-library Windows exec behavior references ---'
if command -v go >/dev/null 2>&1; then
goroot="$(go env GOROOT)"
printf 'GOROOT=%s\n' "$goroot"
rg -n 'func \(c \*Cmd\) Start|LookPath|CreateProcess|exec\.Error|ErrNotFound' \
"$goroot/src/os/exec" "$goroot/src/syscall/exec_windows.go" 2>/dev/null \
| sed -n '1,220p'
else
echo 'go is unavailable'
fi
printf '%s\n' '--- read-only branch model ---'
python3 - <<'PY'
from itertools import product
def select(goos, pwsh, powershell):
shell, flag = "sh", "-c"
if goos == "windows":
if pwsh:
shell, flag = "pwsh", "-Command"
elif powershell:
shell, flag = "powershell", "-Command"
return shell, flag
for pwsh, powershell, sh in product((False, True), repeat=3):
shell, flag = select("windows", pwsh, powershell)
print(f"pwsh={pwsh} powershell={powershell} sh={sh} -> {shell} {flag}")
PYRepository: buchenberg/yaah
Length of output: 28735
🏁 Script executed:
#!/bin/bash
set -eu
cat -n internal/process/process_test.go | sed -n '180,360p'
rg -n -i --glob '*.go' --glob '*.md' 'background_process|Start-Sleep|powershell|pwsh|shell wrapper|PowerShell' . | sed -n '1,220p'
python3 - <<'PY'
from itertools import product
for pwsh, powershell in product((False, True), repeat=2):
if pwsh:
result = ("pwsh", "-Command")
elif powershell:
result = ("powershell", "-Command")
else:
result = ("sh", "-c")
print(pwsh, powershell, "=>", result)
PYRepository: buchenberg/yaah
Length of output: 13194
Do not fall back to sh on Windows. Select only pwsh or powershell; return an error when neither is on PATH. The current fallback can apply POSIX semantics or fail to start background commands. Add a Windows test for this case.
🤖 Prompt for 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.
In `@internal/process/process.go` around lines 66 - 78, The Windows shell
selection in the process setup must not retain the default sh fallback. Update
the logic around runtime.GOOS and exec.LookPath to select pwsh or powershell
only, and return an error when neither is available; add a Windows-specific test
covering the missing-shell case.
Source: Coding guidelines
Comment on lines
+23
to
+37
| // IsRoleNotFound reports whether err is a RoleNotFoundError (from either | ||
| // the tools or agent/subagent package). It uses errors.As so wrapped | ||
| // errors are detected. | ||
| func IsRoleNotFound(err error) bool { | ||
| var rnfe RoleNotFoundError | ||
| if errors.As(err, &rnfe) { | ||
| return true | ||
| } | ||
| // The agent/subagent package has its own RoleNotFoundError type | ||
| // with the same shape. Match it by error string prefix so tools | ||
| // does not need to import agent/subagent. | ||
| if err != nil && len(err.Error()) >= 5 && err.Error()[:5] == "role " { | ||
| return true | ||
| } | ||
| return false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use an explicit role-not-found marker.
Lines 31-36 classify every error that starts with "role " as a role-not-found error. For example, errors.New("role configuration is invalid") returns true.
The value target at Line 27 also misses a wrapped *RoleNotFoundError. Replace the message-prefix fallback with a marker interface that both role error types implement. This preserves wrapped-error detection without parsing Error() text.
#!/bin/bash
set -euo pipefail
# Inspect role-not-found error definitions and all classifiers.
# Expect: each implementation exposes one explicit marker and no classifier
# relies on an Error() string prefix.
rg -n -C 4 --type go \
'type\s+RoleNotFoundError|func\s+\(.*RoleNotFoundError.*\)\s+(Error|RoleNotFound)|IsRoleNotFound\(' \
internal🤖 Prompt for 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.
In `@internal/tools/role_resolver.go` around lines 23 - 37, Update IsRoleNotFound
to use an explicit marker interface implemented by both tools.RoleNotFoundError
and agent/subagent.RoleNotFoundError, allowing errors.As to recognize wrapped
value and pointer errors. Remove the "role " error-string prefix fallback and
retain classification only through the marker.
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.
…ify compaction
Architecture review remediation addressing dependency cycles, god-package concerns, and business-logic leakage into cmd/.
Key changes:
Break tools → agent/subagent design cycle: introduce tools.RoleResolver interface and tools-local types (RoleNotFoundError, ContractField, RoleEntry). agent/runner.SubAgentRoleResolver bridges to the subagent RoleRegistry. internal/tools no longer imports agent/subagent.
Extract control-plane types to internal/control: CtrlMsg and related types now canonical in internal/control; internal/types/control.go holds type aliases for backward compatibility. internal/types no longer imports internal/todo — the de-facto core is dependency-free.
Unify :compact command with in-loop compactor: replace the standalone parallel summarizer in compact_cmd.go with loop.ForceCompact(), which shares cooldowns, adaptive budgets, chunked fallback, and events. ForceCompact nils View to avoid creating a broker/forwarder goroutine, clears cooldowns/ineffective-compaction state so the explicit request always runs, and flushes the persister afterward.
Extract LoopBuilder in internal/agent: eliminates ~70 lines of duplicated agent.NewLoop option construction between build_loop.go and serve.go.
Move provider stubs to internal/providers: NoProviderStub and OAuthErrorStub are provider implementations, not CLI wiring.
Add config.Validate() for fail-fast startup: validates approval mode, compaction thresholds, context window, fallback/subagent providers, and MCP transport. Wired into newAgentSessionWithOptions.
Log swallowed errors: DB open (wiring.go), MCP notification dispatch (http_server.go). config edit now falls back to ResolveEditor(nil) on load error instead of hard-failing.
Inline agent_hooks.go into persist.go; add package docs to internal/agent and internal/memory. Document globals in AGENTS.md (tuiMCPBuf, defaultRoleReg, OTel metrics).
Add config struct parity test (config.Defaults ↔ agent.AgentConfig) and LoopBuilder roundtrip test to catch silent drift.
All tests pass; vet, staticcheck, and gofmt clean.
Summary by CodeRabbit
New Features
Bug Fixes
Tests