Milestone 5: observability - event log, usage ledger, trace (Tasks 22-24)#8
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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: 1
🧹 Nitpick comments (1)
src/observability/trace.ts (1)
42-46: ⚡ Quick winConsider parallelizing outcome fetches.
Fetching outcomes sequentially adds cumulative latency for sessions with many tasks. Since
getOutcomecalls are independent, they can be parallelized.⚡ Proposed refactor to parallelize outcome fetches
- const outcomes: OutcomeRecord[] = []; - for (const t of tasks) { - const o = await store.getOutcome(sessionId, t.taskId); - if (o) outcomes.push(o); - } + const outcomePromises = tasks.map((t) => store.getOutcome(sessionId, t.taskId)); + const outcomeResults = await Promise.all(outcomePromises); + const outcomes = outcomeResults.filter((o): o is OutcomeRecord => o !== undefined);🤖 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 `@src/observability/trace.ts` around lines 42 - 46, The current loop in which outcomes are fetched sequentially should be changed to run store.getOutcome calls in parallel: replace the for-await pattern over tasks with constructing an array of promises via tasks.map(t => store.getOutcome(sessionId, t.taskId)), await Promise.all(...) to get results, then filter out any undefined/null entries into the outcomes array; ensure you preserve existing behavior by keeping the OutcomeRecord[] type, and propagate or handle errors the same way as before (or wrap individual promises if you need per-call isolation).
🤖 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 `@src/observability/events.ts`:
- Around line 54-57: The current fallback uses || which treats 0 as falsy;
change the logic in the prompt, completion, and totalRaw assignments to detect
presence of keys instead of truthiness so zero is preserved — e.g. replace
expressions like const prompt = num(u["prompt_tokens"]) ||
num(u["input_tokens"]) || num(u["promptTokens"]); with a presence-based chain
such as: if ('prompt_tokens' in u) use num(u['prompt_tokens']) else if
('input_tokens' in u) use num(u['input_tokens']) else use
num(u['promptTokens']); apply the same pattern to completion and totalRaw (or
alternatively use the nullish coalescing approach on raw values:
num(u['prompt_tokens'] ?? u['input_tokens'] ?? u['promptTokens']) if num handles
undefined/null), referencing the variables prompt, completion, totalRaw and the
num(u[...]) calls.
---
Nitpick comments:
In `@src/observability/trace.ts`:
- Around line 42-46: The current loop in which outcomes are fetched sequentially
should be changed to run store.getOutcome calls in parallel: replace the
for-await pattern over tasks with constructing an array of promises via
tasks.map(t => store.getOutcome(sessionId, t.taskId)), await Promise.all(...) to
get results, then filter out any undefined/null entries into the outcomes array;
ensure you preserve existing behavior by keeping the OutcomeRecord[] type, and
propagate or handle errors the same way as before (or wrap individual promises
if you need per-call isolation).
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56a647a7-9107-4f0e-8d39-6ff6427ae0d9
📒 Files selected for processing (11)
.kiro/specs/loop-engine/tasks.mdpackage.jsonsrc/adapters/roleBindings.tssrc/observability/events.tssrc/observability/instrument.tssrc/observability/trace.tssrc/observability/usage.tssrc/session.tssrc/storage/store.tssrc/trace.tstest/observability.test.ts
Comment on lines
+54
to
+57
| const prompt = num(u["prompt_tokens"]) || num(u["input_tokens"]) || num(u["promptTokens"]); | ||
| const completion = | ||
| num(u["completion_tokens"]) || num(u["output_tokens"]) || num(u["completionTokens"]); | ||
| const totalRaw = num(u["total_tokens"]) || num(u["totalTokens"]); |
There was a problem hiding this comment.
Fix the || chaining logic to handle zero values correctly.
The current logic uses || to try fallback keys, but this incorrectly prefers fallback keys when the primary key exists with a valid zero value. For example, {prompt_tokens: 0, input_tokens: 5} would return promptTokens: 5 instead of the correct 0.
This corrupts usage aggregation when a provider legitimately returns zero tokens (e.g., for an empty prompt or certain model responses).
🔧 Proposed fix using key existence checks
- const prompt = num(u["prompt_tokens"]) || num(u["input_tokens"]) || num(u["promptTokens"]);
- const completion =
- num(u["completion_tokens"]) || num(u["output_tokens"]) || num(u["completionTokens"]);
- const totalRaw = num(u["total_tokens"]) || num(u["totalTokens"]);
+ const prompt = "prompt_tokens" in u ? num(u["prompt_tokens"])
+ : "input_tokens" in u ? num(u["input_tokens"])
+ : num(u["promptTokens"]);
+ const completion = "completion_tokens" in u ? num(u["completion_tokens"])
+ : "output_tokens" in u ? num(u["output_tokens"])
+ : num(u["completionTokens"]);
+ const totalRaw = "total_tokens" in u ? num(u["total_tokens"]) : num(u["totalTokens"]);📝 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
| const prompt = num(u["prompt_tokens"]) || num(u["input_tokens"]) || num(u["promptTokens"]); | |
| const completion = | |
| num(u["completion_tokens"]) || num(u["output_tokens"]) || num(u["completionTokens"]); | |
| const totalRaw = num(u["total_tokens"]) || num(u["totalTokens"]); | |
| const prompt = "prompt_tokens" in u ? num(u["prompt_tokens"]) | |
| : "input_tokens" in u ? num(u["input_tokens"]) | |
| : num(u["promptTokens"]); | |
| const completion = "completion_tokens" in u ? num(u["completion_tokens"]) | |
| : "output_tokens" in u ? num(u["output_tokens"]) | |
| : num(u["completionTokens"]); | |
| const totalRaw = "total_tokens" in u ? num(u["total_tokens"]) : num(u["totalTokens"]); |
🤖 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 `@src/observability/events.ts` around lines 54 - 57, The current fallback uses
|| which treats 0 as falsy; change the logic in the prompt, completion, and
totalRaw assignments to detect presence of keys instead of truthiness so zero is
preserved — e.g. replace expressions like const prompt = num(u["prompt_tokens"])
|| num(u["input_tokens"]) || num(u["promptTokens"]); with a presence-based chain
such as: if ('prompt_tokens' in u) use num(u['prompt_tokens']) else if
('input_tokens' in u) use num(u['input_tokens']) else use
num(u['promptTokens']); apply the same pattern to completion and totalRaw (or
alternatively use the nullish coalescing approach on raw values:
num(u['prompt_tokens'] ?? u['input_tokens'] ?? u['promptTokens']) if num handles
undefined/null), referencing the variables prompt, completion, totalRaw and the
num(u[...]) calls.
…-24) Task 22 - structured event log: Store gains a generic event stream (recordEvent/listEvents); observability/instrument.ts wraps each runner so every invocation emits a role-attributed runner_call event (chars, duration, quota, normalized token usage). createRoles takes an onRunnerCall hook; session.runGoal records runner calls plus session_started / plan_reviewed / session_finished lifecycle markers to the store. Task 23 - observability/usage.ts: computeUsage aggregates runner calls per role and per run (calls, tokens, duration, quota hits), with optional per-1k-token rates yielding a cost estimate. normalizeUsage handles OpenAI- and input/output-style usage objects. Task 24 - observability/trace.ts: buildTrace assembles a session's lifecycle, per-task state history, attempts, outcomes, events, and usage ledger; formatTrace renders it. New 'npm run trace -- <sessionId>' CLI. Adds 6 tests (125 total); typecheck clean.
Select the first usage key that is actually present rather than the first truthy one. The previous '||' chain discarded a legitimate 0 (e.g. an empty prompt) in favor of a later alias, corrupting usage aggregation.
This was referenced Jun 13, 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.
This pull request was created by @kiro-agent on behalf of @inhaq 👻
Comment with /kiro fix to address specific feedback or /kiro all to address everything.
Learn about Kiro Web
Summary
Completes Milestone 5 (Observability) — Tasks 22, 23, 24.
This is the final engine milestone — with it, the headless engine (Milestones 1–5) is feature-complete against the spec.
Task 22 — Structured event log
Storegains a generic event stream (recordEvent/listEvents).observability/instrument.tswraps any runner so every invocation emits a role-attributedrunner_callevent (prompt/output chars, duration, quota flag, normalized token usage) — transparently, returning the result unchanged.createRolestakes anonRunnerCallhook;session.runGoalrecords runner calls plussession_started/plan_reviewed/session_finishedlifecycle markers. (State transitions/attempts/outcomes were already persisted in M3.)Task 23 — Usage/cost ledger (
observability/usage.ts)computeUsageaggregates runner calls per role and per run: calls, prompt/completion/total tokens, duration, quota hits. Optional per-1k-tokenratesyield a dollar estimate.normalizeUsagehandles OpenAI-style and input/output-style usage objects.Task 24 — Session trace (
observability/trace.ts)buildTraceassembles a session's lifecycle, per-task state history, attempts, outcomes, events, and the usage ledger into one inspectable object;formatTracerenders it for humans. Newnpm run trace -- <sessionId>CLI.Testing
npm run typecheckclean.npm test: 125 passing (+6). Covers usage normalization, transparent instrumentation with token capture, per-role/total/cost aggregation, and an end-to-end run asserting the persisted event stream (lifecycle + runner calls) and a built trace.Summary by CodeRabbit
New Features
Chores