Shepherd trace - #192
Conversation
Record every tool call and turn boundary to a content-addressed, append-only SQLite store via a new `shepherd_trace` pipeline middleware (enabled by default). Each fact is cryptographically hashed and causally chained, yielding tamper-evident, replayable execution history that survives crashes and is inspectable independent of OTel exporters. New `yaah shepherd-trace` commands expose the store: - `list` — show trace sessions with fact counts - `show <session>` — tool-call sequence with args and status - `show --latest` — most recent session - `profile <session>` — aggregate turns, token usage, tool stats, and success rates Sub-agents now trace under their own owners (`sub-<role>-sess-*-<id>`) in the same store. When a sub-agent fails, the orchestrator reads its trace and appends the failing tool-call sequence to the retry prompt, so the parent sees exactly which calls led to the error. A new `subagent_trace` tool enables proactive queries during a run. Configuration: - `agents.default.shepherd_trace_dir` (default `~/.yaah/traces/`) controls the store location. - No separate enable flag — tracing is active whenever `shepherd_trace` is in the pipeline (default on) and disabled via `middleware.disabled: [shepherd_trace]`. Adds the `shepherd-kernel-go` dependency (local replace for now) and a noop middleware fallback that degrades gracefully when the store cannot be opened.
- Add shepherd_trace to default pipeline list in tui2 - Use atomic counter for ordinal to support multiple middleware instances - Replace stderr errors with structured slog logging - Move agent status display into sessioninfo component
📝 WalkthroughWalkthroughAdds durable Shepherd SQLite traces for agent and sub-agent execution. Adds trace middleware, lifecycle recording, CLI and tool inspection commands, configuration, documentation, tests, and updated TUI agent status output. ChangesShepherd tracing
TUI agent information
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant TraceMiddleware
participant SQLiteTraceStore
participant YAAHCLI
Agent->>TraceMiddleware: Start turn and record tool results
TraceMiddleware->>SQLiteTraceStore: Append declarations and captures
Agent->>TraceMiddleware: End or fail turn
TraceMiddleware->>SQLiteTraceStore: Persist lifecycle result
YAAHCLI->>SQLiteTraceStore: Read sessions and facts
SQLiteTraceStore-->>YAAHCLI: Return trace records
YAAHCLI-->>Agent: Render list, show, or profile output
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/agent/loop.go (1)
227-254: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClose successful tool-call turns before the next iteration.
After
executeToolPhasesucceeds, the loop continues without callingEndTurn. Each tool-call turn remains inturn:startedstate until the nextStartTurnoverwritesturnRootFactIDs. On exhaustion,FailTurn(l.Config.MaxLoopCycles, err)then records a failure for a turn that was not started.Call
EndTurnafter successful tool execution. RecordMaxIterationsErroras a run-level event, or associate it with an explicitly started turn.Proposed lifecycle completion
if err != nil { if traceMw != nil { traceMw.FailTurn(iter, err) } return "", err } + + if traceMw != nil { + traceMw.EndTurn(iter, result.Usage.PromptTokens, result.Usage.CompletionTokens) + } if turnSpan != nil {🤖 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/loop.go` around lines 227 - 254, In the loop around executeToolPhase, close each successful tool-call turn by calling the trace middleware’s EndTurn before the next iteration, while preserving the existing failure handling for unsuccessful execution. Update max-iteration exhaustion so MaxIterationsError is recorded as a run-level event, or start and associate an explicit turn before calling FailTurn instead of failing an unstarted turn.
🧹 Nitpick comments (9)
internal/tui2/components/sessioninfo/sessioninfo_test.go (1)
21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the new status output.
The formatter now emits
Status:, but this test does not set or checkInfo.Status. SetStatus: "active"and assert the rendered value.Proposed test update
th := colors.NewDarkTheme() - out := Format(Info{Provider: "openai", Model: "gpt-4", Version: "1.2.3"}, &th) + th.NoColor = true + out := Format(Info{ + Provider: "openai", + Model: "gpt-4", + Version: "1.2.3", + Status: "active", + }, &th) + if !strings.Contains(out, "Status: active") { + t.Errorf("should contain status, got %q", out) + }🤖 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/tui2/components/sessioninfo/sessioninfo_test.go` around lines 21 - 25, Update TestFormat_Version to populate Info.Status with "active" and assert that Format renders both the "Status:" label and the "active" value in the output.internal/tui2/components/infopane/infopane.go (1)
68-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover both
AgentActivestates.
Formatnow mapsState.AgentActiveto"active"or"idle". Add tests ininternal/tui2/components/infopane/infopane_test.gofor both values and assert the renderedStatus:line. This verifies the new state-to-text wiring separately fromsessioninfo.Format.🤖 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/tui2/components/infopane/infopane.go` around lines 68 - 77, Add infopane tests covering both true and false AgentActive values, asserting that the rendered Status: line contains “active” and “idle” respectively. Exercise the wiring through the infopane rendering path rather than testing sessioninfo.Format directly.cmd/yaah/trace.go (4)
464-469: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDo not discard the configuration load error.
Line 465 ignores the error from
config.Load(). A malformedconfig.yamlthen silently falls back to~/.yaah/traces, and the user sees an unrelated "no trace store found" message. Log or return the error.🤖 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/trace.go` around lines 464 - 469, The openShepherdTraceStore function must handle errors returned by config.Load instead of discarding them. Capture the load error and return or log it before applying the default trace directory, while preserving the existing configuration-based directory selection for successful loads.
263-264: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an index instead of a pointer into the
turnsslice.
currentpoints into the backing array ofturns.appendon Line 286 can reallocate that array. The code reassignscurrentimmediately after each append, so the current behavior is correct. The pattern breaks if any future change appends toturnsbetween two tool records. Track the index instead.♻️ Proposed refactor
- var turns []turnRecord - var current *turnRecord + var turns []turnRecord + currentIdx := -1Then replace
current == nilwithcurrentIdx < 0andcurrent.tools = append(...)withturns[currentIdx].tools = append(...).Also applies to: 286-287
🤖 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/trace.go` around lines 263 - 264, Replace the current *turnRecord pointer with an integer currentIdx initialized to -1, and update the surrounding turn-processing logic to use currentIdx < 0 for the unset check. After appending a turn record, assign its index to currentIdx, and append tools through turns[currentIdx].tools instead of current.tools.
95-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared latest-session lookup and the read limit.
The show and profile commands repeat the same three steps: read all owners, sort them in reverse, and take the first. The literal
99999appears four times as the read limit. Extract one helper and one named constant.♻️ Proposed helper
const traceReadLimit = 99999 func latestTraceSession(store *shepherd.SQLiteTraceStore) (string, error) { slice, err := store.ReadOwnerPrefix(shepherd.TrustedReadContext, "", traceReadLimit, "declarations_only") if err != nil { return "", fmt.Errorf("read traces: %w", err) } ownerIDs := make([]string, 0, len(slice.OwnerPaths)) for owner := range slice.OwnerPaths { ownerIDs = append(ownerIDs, owner) } if len(ownerIDs) == 0 { return "", nil } sort.Sort(sort.Reverse(sort.StringSlice(ownerIDs))) return ownerIDs[0], nil }Then both commands call
latestTraceSessionand treat an empty result as "No trace sessions found.".Also applies to: 116-119, 199-224
🤖 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/trace.go` around lines 95 - 114, The show and profile command paths duplicate latest-session lookup logic and the literal read limit. Define a shared traceReadLimit constant and latestTraceSession helper that reads owners, handles read errors, sorts IDs in reverse, and returns the newest ID or an empty result; update both command paths to use it while preserving the existing “No trace sessions found.” output.
513-518: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
truncateStringagainst small limits.If
maxLenis less than 3,s[:maxLen-3]panics with a negative index. The current callers pass 50 and 60, so the panic is not reachable today. Add the guard so future callers cannot trigger it. The function also slices bytes, so it can split a multi-byte rune. Slice runes if trace arguments can contain non-ASCII text.🛡️ Proposed fix
func truncateString(s string, maxLen int) string { - if len(s) <= maxLen { + if maxLen <= 0 { + return "" + } + if len(s) <= maxLen { return s } - return s[:maxLen-3] + "..." + if maxLen <= 3 { + return s[:maxLen] + } + r := []rune(s) + if len(r) <= maxLen { + return s + } + return string(r[:maxLen-3]) + "..." }🤖 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/trace.go` around lines 513 - 518, Update truncateString to safely handle maxLen values below 3 without negative slicing, returning an appropriately bounded result while preserving the truncation behavior. Also convert the truncation logic from byte slicing to rune-aware slicing so non-ASCII trace arguments are not split mid-rune.cmd/yaah/trace_test.go (3)
385-403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the table cases in subtests.
TestFormatNumiterates its table withoutt.Run, so a failure does not name the case.TestTruncateStringuses two inline assertions instead of a table with subtests. The coding guidelines requiret.Run("name", func(t *testing.T) { ... })for subtests.♻️ Proposed fix
for _, tt := range tests { - got := formatNum(tt.n) - if got != tt.want { - t.Errorf("formatNum(%d) = %q, want %q", tt.n, got, tt.want) - } + t.Run(tt.want, func(t *testing.T) { + if got := formatNum(tt.n); got != tt.want { + t.Errorf("formatNum(%d) = %q, want %q", tt.n, got, tt.want) + } + }) }As per coding guidelines: "Place tests next to the code they test and use
t.Run("name", func(t *testing.T) { ... })for subtests."Also applies to: 376-383
🤖 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/trace_test.go` around lines 385 - 403, Update TestFormatNum to execute each table entry inside a named t.Run subtest, using a descriptive case name and the subtest’s *testing.T for the assertion. Also convert TestTruncateString’s inline assertions into named table-driven subtests, preserving the existing inputs and expected results.Source: Coding guidelines
84-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd profile assertions for turn status and token totals.
seedTestTraceStorechains theturn:completedcapture fromlastFactIDs, which is the last tool result, not theturn:createddeclaration.newShepherdTraceProfileCmdreads turn status and tokens only from the capture whose causal parent is theturn:createddeclaration. The seeded data therefore never exercises that path, andTestTraceProfileonly checks for the stringsTools:andSUCCESS. Add assertions forcompletedand for the token line so the profile token attribution is covered.Also applies to: 181-210
🤖 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/trace_test.go` around lines 84 - 97, Update seedTestTraceStore so the turn:completed capture in the AppendBatch uses the turn:created declaration as its causal parent instead of lastFactIDs, then extend TestTraceProfile assertions to require the completed status and expected token totals line. This must exercise the attribution path read by newShepherdTraceProfileCmd while preserving existing Tools and SUCCESS checks.
13-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the errors in the test helpers.
os.WriteFileon Line 21 andos.MkdirAllon Line 122 return errors that the code discards. If the configuration file is not written,openShepherdTraceStorefalls back to~/.yaah/traces, and the test then asserts against a store it did not seed. The failure message would not identify the cause. The same applies to thestore.Appendcalls inseedTestTraceStore(Lines 37, 54, 68, 85, 101) and toshepherd.NewSQLiteTraceStoreon Line 320.♻️ Proposed fix for the config helper
func setTestTraceConfig(t *testing.T, home, traceDir string) { t.Helper() configContent := ` agents: default: model: test/model shepherd_trace_dir: ` + traceDir + ` ` - os.WriteFile(filepath.Join(home, "config.yaml"), []byte(configContent), 0o644) + if err := os.WriteFile(filepath.Join(home, "config.yaml"), []byte(configContent), 0o644); err != nil { + t.Fatalf("write test config: %v", err) + } }Apply the same pattern to
os.MkdirAlland to eachstore.Appendcall inseedTestTraceStore.Also applies to: 117-124
🤖 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/trace_test.go` around lines 13 - 22, Handle returned errors in the trace test helpers: assert or fail the test with context for os.WriteFile in setTestTraceConfig, os.MkdirAll, every store.Append in seedTestTraceStore, and shepherd.NewSQLiteTraceStore. Ensure failures identify the operation and stop the test before continuing with an uninitialized or unseeded store.
🤖 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/trace.go`:
- Around line 488-502: Update captureStatus to distinguish a completed failed
capture from a missing capture: when the record has a capture result with
success false and no error message, return "error" rather than "pending".
Preserve "pending" for records without a usable capture and keep the existing
"ok" and explicit-error behavior.
- Around line 477-480: Handle the error returned by os.UserHomeDir in the ~/
expansion block around traceDir: if the lookup fails, return or propagate that
error instead of continuing with an empty home path. Preserve the existing
filepath.Join behavior when the lookup succeeds.
In `@docs/configuration.md`:
- Around line 65-68: Update the documented configuration’s explicit
middleware.enabled list to include shepherd_trace so the nearby
shepherd_trace_dir setting actually activates tracing for copied configurations;
keep the existing middleware entries unchanged.
In `@docs/features.md`:
- Around line 278-280: Update the error message returned by
openShepherdTraceStore to remove the nonexistent shepherd_trace_enabled
configuration key and instruct users to run yaah with the shepherd_trace
middleware enabled, while preserving the trace store path in the message.
- Line 245: Update the fenced code block containing the sample profile output in
features.md to specify the text language, using text after the opening fence so
markdownlint MD040 is satisfied.
- Around line 221-223: Update all three stale middleware-count references in
README.md to state that the pipeline has 12 middleware and 9 enabled by default,
including replacing “Middle ground: 11 middleware and counting.”
In `@go.mod`:
- Line 33: Replace the local shepherd-kernel-go dependency setup with an
immutable published module version, then remove the replace directive pointing
to ../shepherd-kernel-go. Keep github.com/buchenberg/shepherd-kernel-go declared
as a direct dependency so normal clones and CI can resolve it without a sibling
directory.
In `@internal/agent/pipeline/config_test.go`:
- Around line 67-84: Update TestShepherdTraceBuilder_UnwritableDirBuildsNoop to
make initialization fail deterministically by creating a regular file and
setting ShepherdTraceDir to a child path beneath that file. Ensure the
shepherd_trace builder’s directory-creation path invokes os.MkdirAll against
this invalid descendant, then preserve the assertion that it returns
noopShepherdTraceMiddleware.
In `@internal/agent/pipeline/config.go`:
- Around line 108-115: Update the trace directory setup in the shepherd trace
configuration flow around MkdirAll and NewShepherdTraceStore to use owner-only
permissions (0700), and validate existing directories rather than relying solely
on creation mode. Add a test that fails when the trace-store directory permits
group or world access.
- Around line 112-117: In the Loop.Run flow, defer traceMw.Close() immediately
after obtaining the middleware so the trace store is closed on normal return,
errors, cancellation, and recovered panics. Ensure the defer applies to
middleware returned by both NewFromConfig and NewSubAgentLoop, without changing
existing run behavior.
In `@internal/agent/pipeline/trace.go`:
- Around line 259-264: Update the frontier identity construction in
Loop.runMiddleware to use a monotonic session-scoped ordinal rather than the
resettable turnNumber for both FrontierID and AppendIntentID. Preserve the
existing ID formats and ensure each frontier generated during a session remains
unique across ContinueAfterMaxIter cycles.
- Around line 29-37: Remove the package-level nextOrdinal counter and the
ordinal initialization from NewShepherdTraceMiddleware. Replace it with a
session-scoped ordinal strategy, and enforce uniqueness of sessionID when
constructing or registering ShepherdTraceMiddleware while preserving sessionID
as the namespace for append intent IDs.
- Around line 107-112: Update the capture flow around m.lastFactIDs so it
assigns captureReceipt.FactIDs only when the capture append succeeds (err ==
nil). Preserve the existing m.lastFactIDs value when capture fails, while
retaining the current error logging behavior.
- Line 10: Update the Go module configuration used by the Shepherd import in
trace.go so CI can resolve github.com/buchenberg/shepherd-kernel-go without
relying on the unavailable ../shepherd-kernel-go checkout; replace the local
path dependency with a resolvable module version or otherwise provision the
dependency within the repository workflow.
In `@internal/tools/subagent_trace.go`:
- Around line 74-105: Bind SubagentTraceTool operations to the active parent
session: update executeList to return only the sub-agent sessions owned by that
parent, and update executeProfile to reject sessionID values that are not
descendants of or associated with the bound parent session before reading facts.
Add and use the tool’s parent-session identifier when constructing or invoking
it, preserving existing trace formatting and errors for authorized sessions.
- Around line 146-155: Update the status and success-count logic in the
trace-processing loop around captureByParent to check whether the factID has a
captured entry before using it. Count a call as successful only when the capture
exists and cap.success is true; treat missing captures and unsuccessful captures
without error text as non-successful while preserving the existing error message
for failed captures.
---
Outside diff comments:
In `@internal/agent/loop.go`:
- Around line 227-254: In the loop around executeToolPhase, close each
successful tool-call turn by calling the trace middleware’s EndTurn before the
next iteration, while preserving the existing failure handling for unsuccessful
execution. Update max-iteration exhaustion so MaxIterationsError is recorded as
a run-level event, or start and associate an explicit turn before calling
FailTurn instead of failing an unstarted turn.
---
Nitpick comments:
In `@cmd/yaah/trace_test.go`:
- Around line 385-403: Update TestFormatNum to execute each table entry inside a
named t.Run subtest, using a descriptive case name and the subtest’s *testing.T
for the assertion. Also convert TestTruncateString’s inline assertions into
named table-driven subtests, preserving the existing inputs and expected
results.
- Around line 84-97: Update seedTestTraceStore so the turn:completed capture in
the AppendBatch uses the turn:created declaration as its causal parent instead
of lastFactIDs, then extend TestTraceProfile assertions to require the completed
status and expected token totals line. This must exercise the attribution path
read by newShepherdTraceProfileCmd while preserving existing Tools and SUCCESS
checks.
- Around line 13-22: Handle returned errors in the trace test helpers: assert or
fail the test with context for os.WriteFile in setTestTraceConfig, os.MkdirAll,
every store.Append in seedTestTraceStore, and shepherd.NewSQLiteTraceStore.
Ensure failures identify the operation and stop the test before continuing with
an uninitialized or unseeded store.
In `@cmd/yaah/trace.go`:
- Around line 464-469: The openShepherdTraceStore function must handle errors
returned by config.Load instead of discarding them. Capture the load error and
return or log it before applying the default trace directory, while preserving
the existing configuration-based directory selection for successful loads.
- Around line 263-264: Replace the current *turnRecord pointer with an integer
currentIdx initialized to -1, and update the surrounding turn-processing logic
to use currentIdx < 0 for the unset check. After appending a turn record, assign
its index to currentIdx, and append tools through turns[currentIdx].tools
instead of current.tools.
- Around line 95-114: The show and profile command paths duplicate
latest-session lookup logic and the literal read limit. Define a shared
traceReadLimit constant and latestTraceSession helper that reads owners, handles
read errors, sorts IDs in reverse, and returns the newest ID or an empty result;
update both command paths to use it while preserving the existing “No trace
sessions found.” output.
- Around line 513-518: Update truncateString to safely handle maxLen values
below 3 without negative slicing, returning an appropriately bounded result
while preserving the truncation behavior. Also convert the truncation logic from
byte slicing to rune-aware slicing so non-ASCII trace arguments are not split
mid-rune.
In `@internal/tui2/components/infopane/infopane.go`:
- Around line 68-77: Add infopane tests covering both true and false AgentActive
values, asserting that the rendered Status: line contains “active” and “idle”
respectively. Exercise the wiring through the infopane rendering path rather
than testing sessioninfo.Format directly.
In `@internal/tui2/components/sessioninfo/sessioninfo_test.go`:
- Around line 21-25: Update TestFormat_Version to populate Info.Status with
"active" and assert that Format renders both the "Status:" label and the
"active" value in the output.
🪄 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: c478d850-c809-46f6-b777-cd16c5ce04ec
📒 Files selected for processing (28)
README.mdcmd/yaah/build_loop.gocmd/yaah/trace.gocmd/yaah/trace_test.gocmd/yaah/tui2.godocs/configuration.mddocs/features.mdgo.modinternal/agent/loop.gointernal/agent/options.gointernal/agent/pipeline/config.gointernal/agent/pipeline/config_test.gointernal/agent/pipeline/pipeline.gointernal/agent/pipeline/pipeline_test.gointernal/agent/pipeline/trace.gointernal/agent/pipeline/trace_test.gointernal/agent/runner/runner.gointernal/agent/runner/runner_test.gointernal/agent/subagent_loop.gointernal/agent/types.gointernal/config/load.gointernal/config/load_test.gointernal/prompts/tools/subagent_trace.mdinternal/tools/subagent_trace.gointernal/tools/subagent_trace_test.gointernal/tui2/components/infopane/infopane.gointernal/tui2/components/sessioninfo/sessioninfo.gointernal/tui2/components/sessioninfo/sessioninfo_test.go
| if strings.HasPrefix(traceDir, "~/") { | ||
| home, _ := os.UserHomeDir() | ||
| traceDir = filepath.Join(home, traceDir[2:]) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle the home-directory error during ~/ expansion.
Line 478 discards the error from os.UserHomeDir(). If the lookup fails, home is empty and traceDir becomes a relative path such as .yaah/traces. The command then reports a confusing "no trace store found" message for a path that is relative to the working directory. Return the error instead.
🐛 Proposed fix
if strings.HasPrefix(traceDir, "~/") {
- home, _ := os.UserHomeDir()
+ home, err := os.UserHomeDir()
+ if err != nil {
+ return nil, fmt.Errorf("expand trace dir %q: %w", traceDir, err)
+ }
traceDir = filepath.Join(home, traceDir[2:])
}📝 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.
| if strings.HasPrefix(traceDir, "~/") { | |
| home, _ := os.UserHomeDir() | |
| traceDir = filepath.Join(home, traceDir[2:]) | |
| } | |
| if strings.HasPrefix(traceDir, "~/") { | |
| home, err := os.UserHomeDir() | |
| if err != nil { | |
| return nil, fmt.Errorf("expand trace dir %q: %w", traceDir, err) | |
| } | |
| traceDir = filepath.Join(home, traceDir[2:]) | |
| } |
🤖 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/trace.go` around lines 477 - 480, Handle the error returned by
os.UserHomeDir in the ~/ expansion block around traceDir: if the lookup fails,
return or propagate that error instead of continuing with an empty home path.
Preserve the existing filepath.Join behavior when the lookup succeeds.
| func captureStatus(fact shepherd.VisibleRecord) string { | ||
| rec, ok := fact.(shepherd.Record) | ||
| if !ok { | ||
| return "pending" | ||
| } | ||
| success, _ := rec.Body.Payload["success"].(bool) | ||
| hasError := rec.Body.Payload["error"] != nil | ||
| if hasError { | ||
| return "error" | ||
| } | ||
| if success { | ||
| return "ok" | ||
| } | ||
| return "pending" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report failed tool calls as errors even without an error message.
captureStatus returns "pending" when success is false and no error key exists. The show output then presents a completed but failed tool call as still running. Distinguish a missing capture from a failed capture.
🐛 Proposed fix
success, _ := rec.Body.Payload["success"].(bool)
hasError := rec.Body.Payload["error"] != nil
if hasError {
return "error"
}
if success {
return "ok"
}
- return "pending"
+ return "failed"
}📝 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.
| func captureStatus(fact shepherd.VisibleRecord) string { | |
| rec, ok := fact.(shepherd.Record) | |
| if !ok { | |
| return "pending" | |
| } | |
| success, _ := rec.Body.Payload["success"].(bool) | |
| hasError := rec.Body.Payload["error"] != nil | |
| if hasError { | |
| return "error" | |
| } | |
| if success { | |
| return "ok" | |
| } | |
| return "pending" | |
| } | |
| func captureStatus(fact shepherd.VisibleRecord) string { | |
| rec, ok := fact.(shepherd.Record) | |
| if !ok { | |
| return "pending" | |
| } | |
| success, _ := rec.Body.Payload["success"].(bool) | |
| hasError := rec.Body.Payload["error"] != nil | |
| if hasError { | |
| return "error" | |
| } | |
| if success { | |
| return "ok" | |
| } | |
| return "failed" | |
| } |
🤖 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/trace.go` around lines 488 - 502, Update captureStatus to
distinguish a completed failed capture from a missing capture: when the record
has a capture result with success false and no error message, return "error"
rather than "pending". Preserve "pending" for records without a usable capture
and keep the existing "ok" and explicit-error behavior.
| # Execution tracing via Shepherd — records every tool call to a durable, | ||
| # inspectable trace store. Active when shepherd_trace is in the pipeline. | ||
| shepherd_trace_dir: ~/.yaah/traces # default, optional | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Activate tracing in the documented configuration.
The same example defines an explicit middleware.enabled list at Lines 111-118. That list omits shepherd_trace, so shepherd_trace_dir does not enable tracing for users who copy this configuration.
Add shepherd_trace to the shown enabled list, or remove the trace setting from this explicit-pipeline example.
Proposed fix
middleware:
enabled: # explicit pipeline order
- steer
- followup
- compaction
- approval
- loop_detection
+ - shepherd_trace🤖 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 `@docs/configuration.md` around lines 65 - 68, Update the documented
configuration’s explicit middleware.enabled list to include shepherd_trace so
the nearby shepherd_trace_dir setting actually activates tracing for copied
configurations; keep the existing middleware entries unchanged.
| | `shepherd_trace` | ✓ | Records every tool call as a durable, inspectable execution trace | | ||
| | `sub_agent` | — | Enforces sub-agent depth limits | | ||
| | `prompt_caching` | — | Anthropic cache-control breakpoints | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find every place that states the middleware stage counts.
set -euo pipefail
rg -n -C 2 'middleware pipeline|stage middleware|on by default' README.md docsRepository: buchenberg/yaah
Length of output: 5034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
features = Path("docs/features.md").read_text()
table = features.split("## Middleware pipeline", 1)[1].split("## Execution traces", 1)[0]
rows = re.findall(r'^\| `([^`]+)` \| ([✓—]) \|', table, re.M)
print("features middleware rows:", len(rows))
print("features enabled rows:", sum(enabled == "✓" for _, enabled in rows))
print("features rows:", rows)
readme = Path("README.md").read_text()
for i, line in enumerate(readme.splitlines(), 1):
if "middleware" in line and ("11" in line or "8" in line or "pipeline" in line):
print(f"README:{i}:{line}")
PY
printf '\nMiddleware-related declarations and defaults:\n'
rg -n -C 2 'shepherd_trace|prompt_caching|sub_agent|soft_prune|middleware\.(enabled|disabled)|11 middleware|8 on by default|12 middleware|9 on by default' --glob '*.go' --glob '*.md' .Repository: buchenberg/yaah
Length of output: 28163
Update all stale middleware counts in README.md.
The pipeline has 12 middleware and 9 enabled by default. Update the three references that still state 11 and 8, including “Middle ground: 11 middleware and counting.”
🤖 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 `@docs/features.md` around lines 221 - 223, Update all three stale
middleware-count references in README.md to state that the pipeline has 12
middleware and 9 enabled by default, including replacing “Middle ground: 11
middleware and counting.”
|
|
||
| Profile output: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced code block.
markdownlint reports MD040 for this block. Use text for the sample profile output.
📝 Proposed fix
-```
+```text
Session: sess-xxx🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 245-245: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/features.md` at line 245, Update the fenced code block containing the
sample profile output in features.md to specify the text language, using text
after the opening fence so markdownlint MD040 is satisfied.
Source: Linters/SAST tools
| var nextOrdinal atomic.Int64 | ||
|
|
||
| // NewShepherdTraceMiddleware creates a trace middleware backed by the given store. | ||
| func NewShepherdTraceMiddleware(store *shepherd.SQLiteTraceStore, sessionID string) *ShepherdTraceMiddleware { | ||
| return &ShepherdTraceMiddleware{ | ||
| store: store, | ||
| sessionID: sessionID, | ||
| ordinal: int(nextOrdinal.Add(1 << 20)), | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Remove the package global.
nextOrdinal is not an approved global. Use a session-scoped ordinal strategy and enforce unique trace session IDs instead. The session ID already namespaces append intent IDs.
As per coding guidelines, “Do not use globals except the explicitly approved build-time variables, serve-mode state, role registry, and initialized OTel metric instruments.”
🤖 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/pipeline/trace.go` around lines 29 - 37, Remove the
package-level nextOrdinal counter and the ordinal initialization from
NewShepherdTraceMiddleware. Replace it with a session-scoped ordinal strategy,
and enforce uniqueness of sessionID when constructing or registering
ShepherdTraceMiddleware while preserving sessionID as the namespace for append
intent IDs.
Source: Coding guidelines
| if err != nil { | ||
| slog.Error("shepherd_trace: capture failed", "err", err) | ||
| } | ||
|
|
||
| m.lastFactIDs = captureReceipt.FactIDs | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the previous causal parent when capture append fails.
If the capture append fails, captureReceipt.FactIDs can be empty. Line 111 then clears m.lastFactIDs, so the next declaration has no causal parent. Update m.lastFactIDs only after a successful capture append.
Proposed fix
if err != nil {
slog.Error("shepherd_trace: capture failed", "err", err)
+ continue
}
-
m.lastFactIDs = captureReceipt.FactIDs📝 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.
| if err != nil { | |
| slog.Error("shepherd_trace: capture failed", "err", err) | |
| } | |
| m.lastFactIDs = captureReceipt.FactIDs | |
| } | |
| if err != nil { | |
| slog.Error("shepherd_trace: capture failed", "err", err) | |
| continue | |
| } | |
| m.lastFactIDs = captureReceipt.FactIDs | |
| } |
🤖 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/pipeline/trace.go` around lines 107 - 112, Update the capture
flow around m.lastFactIDs so it assigns captureReceipt.FactIDs only when the
capture append succeeds (err == nil). Preserve the existing m.lastFactIDs value
when capture fails, while retaining the current error logging behavior.
| spec := shepherd.FrontierSpec{ | ||
| FrontierID: fmt.Sprintf("%s:frontier:%d", m.sessionID, turnNumber), | ||
| TargetTraceOwnerID: m.sessionID, | ||
| ThroughFactID: m.lastFactIDs[len(m.lastFactIDs)-1], | ||
| AppendIntentID: fmt.Sprintf("%s:frontier_intent:%d", m.sessionID, turnNumber), | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use a monotonic frontier identity.
Loop.runMiddleware resets iter to zero after ContinueAfterMaxIter. Reusing turnNumber here repeats both FrontierID and AppendIntentID within one session. Later frontiers can collide with an earlier execution cycle and leave inspection commands with stale boundaries. Use the monotonic ordinal or a separate session-scoped turn sequence for these IDs.
🤖 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/pipeline/trace.go` around lines 259 - 264, Update the frontier
identity construction in Loop.runMiddleware to use a monotonic session-scoped
ordinal rather than the resettable turnNumber for both FrontierID and
AppendIntentID. Preserve the existing ID formats and ensure each frontier
generated during a session remains unique across ContinueAfterMaxIter cycles.
| func (t *SubagentTraceTool) executeList(store *shepherd.SQLiteTraceStore) (string, error) { | ||
| slice, err := store.ReadOwnerPrefix(shepherd.TrustedReadContext, "", 99999, "declarations_only") | ||
| if err != nil { | ||
| return "", fmt.Errorf("subagent_trace: read store: %w", err) | ||
| } | ||
|
|
||
| var sb strings.Builder | ||
| sb.WriteString("Sub-agent trace sessions:\n") | ||
|
|
||
| found := false | ||
| for owner, paths := range slice.OwnerPaths { | ||
| if !strings.HasPrefix(owner, "sub-") { | ||
| continue | ||
| } | ||
| if !found { | ||
| found = true | ||
| } | ||
| sb.WriteString(fmt.Sprintf(" %s (%d facts)\n", owner, len(paths))) | ||
| } | ||
| if !found { | ||
| sb.WriteString(" (no sub-agent sessions found)\n") | ||
| } | ||
| return sb.String(), nil | ||
| } | ||
|
|
||
| func (t *SubagentTraceTool) executeProfile(store *shepherd.SQLiteTraceStore, sessionID string) (string, error) { | ||
| slice, err := store.ReadOwnerPrefix(shepherd.TrustedReadContext, sessionID, 99999, "both") | ||
| if err != nil { | ||
| return "", fmt.Errorf("subagent_trace: read session: %w", err) | ||
| } | ||
| if len(slice.FactIDs()) == 0 { | ||
| return "", fmt.Errorf("subagent_trace: no facts found for session %q", sessionID) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Restrict trace access to the current parent session.
executeList reads every trace owner, and executeProfile accepts any session ID. A later agent session can enumerate prior sub-* sessions and return their tool arguments and error text to its model context. This crosses session boundaries and can expose sensitive prompts, paths, commands, or credentials.
Bind SubagentTraceTool to the active parent session. Filter list results and reject profile IDs that do not belong to that parent session.
🤖 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/subagent_trace.go` around lines 74 - 105, Bind
SubagentTraceTool operations to the active parent session: update executeList to
return only the sub-agent sessions owned by that parent, and update
executeProfile to reject sessionID values that are not descendants of or
associated with the bound parent session before reading facts. Add and use the
tool’s parent-session identifier when constructing or invoking it, preserving
existing trace formatting and errors for authorized sessions.
| seq++ | ||
| total++ | ||
| cap := captureByParent[factID] | ||
| status := "ok" | ||
| if cap.errMsg != "" { | ||
| status = "error: " + cap.errMsg | ||
| } | ||
| if status == "ok" { | ||
| okCount++ | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not report missing or failed captures as successful calls.
The zero-value map entry produces status == "ok" when a declaration has no capture. A capture with success: false and no error text also reports success. This makes the displayed success rate incorrect for incomplete or failed trace records.
Use a presence check for captureByParent. Count a call as successful only when a capture exists and cap.success is true.
🤖 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/subagent_trace.go` around lines 146 - 155, Update the status
and success-count logic in the trace-processing loop around captureByParent to
check whether the factID has a captured entry before using it. Count a call as
successful only when the capture exists and cap.success is true; treat missing
captures and unsuccessful captures without error text as non-successful while
preserving the existing error message for failed captures.
1 participant
Summary by CodeRabbit