Tier 3 agent runner: streaming, compaction, steering#20
Merged
Conversation
Three pi-harness Tier 3 features for the native agent runner. G) Sub-step activity streaming. RunRequest gains an onEvent sink + a vendor-neutral RunnerActivity type; PiAgentRunner forwards turn_start / tool_start / tool_end from pi's agent events. The role layer (runnerRoles) enriches each activity with role + runner identity and threads onEvent into every runner.run() call; createRoles exposes onActivity; session wires it to store.recordEvent(runner_activity), which the server's store tap already fans to the SSE hub - so a monitor sees the actor's tool calls live with no server.ts change. D) Context compaction. PiAgentRunner gains a compaction option and wires pi's transformContext + convertToLlm: when the transcript grows past budget it summarizes the older middle via pi's generateSummary and keeps the prompt + recent tail. findCompactionCut snaps the retained tail back to a turn-start assistant so a toolResult is never orphaned. Exported for direct testing. E) Steering. RunRequest.steering hands the caller a steer(text) bound to the live loop; PiAgentRunner injects it via agent.steer so a human can nudge a running task (taking effect after the current turn) instead of only cancelling. meta.steerCount reports injections. Tests: runner-level activity emission + role enrichment/threading; compaction cut + summarize/keep + no-trigger + too-short (vs faux); mid- run steering proven to reach the next turn's context. Full suite 197 green; typecheck clean.
|
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)
test/piAgentCompaction.test.ts (1)
89-103: ⚡ Quick winAdd a test for summarize-failure best-effort fallback.
Line 89-103 covers the success path well, but the contract that compaction failure must return the original transcript unchanged is not directly tested. Please add a deterministic faux-provider failure case and assert no compaction is applied (same reference or equivalent untouched transcript).
Suggested test addition
+ it("keeps the original transcript when summary generation fails", async () => { + // no response queued -> summary generation should fail in faux provider path + const transform = createCompactionTransform(opts({ thresholdTokens: 1 }), faux.getModel(), "k"); + const msgs = transcript(); + const out = await transform!(msgs); + expect(out).toBe(msgs); // best-effort fallback + expect(out.find((m) => m.role === "compactionSummary")).toBeUndefined(); + });🤖 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 `@test/piAgentCompaction.test.ts` around lines 89 - 103, The test file currently only covers the success path for the compaction transformation but lacks coverage for the failure fallback behavior. Add a new test case that configures the faux provider to return an error response (using a method like setResponses with an error condition or similar failure state), then call createCompactionTransform with transcript() and assert that the returned output is either the same reference as the input or an equivalent untouched transcript with no compaction applied, demonstrating that the transform gracefully falls back to the original content when summarization fails.
🤖 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/session.ts`:
- Around line 155-164: The onActivity callback is dropping the Promise returned
by store.recordEvent(), which can result in unhandled promise rejections. In the
arrow function that calls store.recordEvent (within the ternary operator in the
onActivity assignment), add explicit error handling to the Promise by chaining a
.catch() method to handle any rejection failures. This makes the fire-and-forget
intent explicit while ensuring that any persistence failures are caught and do
not cause unhandled rejection errors.
---
Nitpick comments:
In `@test/piAgentCompaction.test.ts`:
- Around line 89-103: The test file currently only covers the success path for
the compaction transformation but lacks coverage for the failure fallback
behavior. Add a new test case that configures the faux provider to return an
error response (using a method like setResponses with an error condition or
similar failure state), then call createCompactionTransform with transcript()
and assert that the returned output is either the same reference as the input or
an equivalent untouched transcript with no compaction applied, demonstrating
that the transform gracefully falls back to the original content when
summarization fails.
🪄 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: d831c5f4-4d62-47f9-85e5-01286ede7028
📒 Files selected for processing (9)
src/adapters/roleBindings.tssrc/adapters/runnerRoles.tssrc/observability/events.tssrc/runners/agentRunner.tssrc/runners/piAgentRunner.tssrc/session.tstest/piAgentCompaction.test.tstest/piAgentRunner.test.tstest/runnerRoles.test.ts
Comment on lines
+155
to
+164
| const onActivity: ((e: RunnerActivityEvent) => void) | undefined = | ||
| store && sessionId | ||
| ? (e) => | ||
| store.recordEvent({ | ||
| sessionId, | ||
| at: e.at, | ||
| type: EVENT_TYPES.runnerActivity, | ||
| data: e as unknown as Record<string, unknown>, | ||
| }) | ||
| : roleOpts.onActivity; |
There was a problem hiding this comment.
Handle runner_activity persistence failures explicitly.
This callback drops the Promise from store.recordEvent(...). If persistence rejects, the rejection can go unhandled and destabilize the process under stricter unhandled-rejection settings. Make this fire-and-forget path explicit and catch failures.
Suggested patch
const onActivity: ((e: RunnerActivityEvent) => void) | undefined =
store && sessionId
- ? (e) =>
- store.recordEvent({
- sessionId,
- at: e.at,
- type: EVENT_TYPES.runnerActivity,
- data: e as unknown as Record<string, unknown>,
- })
+ ? (e) => {
+ void store
+ .recordEvent({
+ sessionId,
+ at: e.at,
+ type: EVENT_TYPES.runnerActivity,
+ data: e as unknown as Record<string, unknown>,
+ })
+ .catch((err) => {
+ opts.log?.(`failed to persist runner_activity: ${String((err as Error)?.message ?? err)}`);
+ });
+ }
: roleOpts.onActivity;📝 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 onActivity: ((e: RunnerActivityEvent) => void) | undefined = | |
| store && sessionId | |
| ? (e) => | |
| store.recordEvent({ | |
| sessionId, | |
| at: e.at, | |
| type: EVENT_TYPES.runnerActivity, | |
| data: e as unknown as Record<string, unknown>, | |
| }) | |
| : roleOpts.onActivity; | |
| const onActivity: ((e: RunnerActivityEvent) => void) | undefined = | |
| store && sessionId | |
| ? (e) => { | |
| void store | |
| .recordEvent({ | |
| sessionId, | |
| at: e.at, | |
| type: EVENT_TYPES.runnerActivity, | |
| data: e as unknown as Record<string, unknown>, | |
| }) | |
| .catch((err) => { | |
| opts.log?.(`failed to persist runner_activity: ${String((err as Error)?.message ?? err)}`); | |
| }); | |
| } | |
| : roleOpts.onActivity; |
🤖 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/session.ts` around lines 155 - 164, The onActivity callback is dropping
the Promise returned by store.recordEvent(), which can result in unhandled
promise rejections. In the arrow function that calls store.recordEvent (within
the ternary operator in the onActivity assignment), add explicit error handling
to the Promise by chaining a .catch() method to handle any rejection failures.
This makes the fire-and-forget intent explicit while ensuring that any
persistence failures are caught and do not cause unhandled rejection errors.
The onActivity (and the identical onRunnerCall) sink dropped the Promise from store.recordEvent, so a rejected persistence could surface as an unhandled rejection and destabilize the run under strict settings. Make the fire-and-forget intent explicit with void and catch failures, logging via opts.log. Addresses CodeRabbit review on PR #20. Co-authored-by: Inzimam Ul Haq <27832433+inhaq@users.noreply.github.com>
Restacks Tier 3 onto the updated base (bash opt-in + maxTurns-before- increment from PR #18 review). Resolves the piAgentRunner.ts turn_start handler conflict by enforcing the turn cap before incrementing while keeping the Tier 3 streaming emits (turn_start / tool_start / tool_end). Co-authored-by: Inzimam Ul Haq <27832433+inhaq@users.noreply.github.com>
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
Implements all of Tier 3 from the pi-harness deep-dive on the native agent runner. Stacked on #18 (base =
pi-native-agent-runner), so this diff is just the Tier 3 commit; retarget tomainonce #18 merges.All three features compose at runtime and are exercised offline against pi's
fauxprovider (no network/API key).G — Sub-step activity streaming
The actor's inner tool calls now surface live instead of a single opaque result.
RunRequestgains anonEventsink + a vendor-neutralRunnerActivitytype;PiAgentRunnerforwardsturn_start/tool_start/tool_endfrom pi's agent events.runnerRoles) enriches each activity with the role + runner identity and threadsonEventinto everyrunner.run()call;createRolesexposesonActivity;sessionwires it tostore.recordEvent("runner_activity").tapStoreEventsalready fans every recorded event to the SSE hub, a monitor sees tool calls live with noserver.tschange.D — Context compaction
Long inner loops no longer blow the context window.
compactionprofile option wires pi'stransformContext+convertToLlm. When the transcript grows past budget it summarizes the older middle via pi'sgenerateSummaryand keeps the prompt + recent tail.findCompactionCutsnaps the retained tail back to a turn-start assistant so atoolResultis never orphaned. Both helpers are exported for direct testing.E — Steering
A human/supervisor can nudge a running task instead of only cancelling.
RunRequest.steeringhands the caller asteer(text)bound to the live loop;PiAgentRunnerinjects it viaagent.steer, taking effect after the current turn.meta.steerCountreports injections.Testing
runnerRoles.test.ts(+3): activity forwarded enriched with role/runner identity for actor and critic; noonEventpassed when no sink configured.piAgentRunner.test.ts(+3): real tool activity emitted viaonEvent; mid-run steering proven to reach the next turn's context (got-steer); zero steers when no source.piAgentCompaction.test.ts(new, 6): cut snaps to a turn boundary; summarize-and-keep over threshold; no-op under threshold; too-short transcript untouched.npm run typecheckclean; full suite 197 tests green.Notes / follow-ups
shouldCompactvs the model's window);thresholdTokensforces it (mainly for tuning/tests)./nudgeendpoint (to drive it from the UI) is the natural next wiring step.eventtype withdata.type = "runner_activity"; a small frontend filter would render them as a live tool feed.Summary by CodeRabbit
Release Notes