refactor(cli): unify MakaRunOutcome classification - #3092
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review. 📝 WalkthroughWhat this PR solvesThis PR unifies The PR preserves distinct incomplete-terminal policies for live and stored execution. It also pins ordinary and graph success and failure exit codes at the CLI boundary. The PR removes obsolete Source of truthThe PR extends the existing outcome-handling path. It does not create a parallel classifier. Live events and stored messages are normalized and passed through the same internal reducer. The reducer also serves Agent Graph terminal turns. This removes the previous event accumulator and stored sandbox-boundary helper. Scope and complexityThe solution is the smallest coherent change described by the objectives:
The added normalization and reducer branches are necessary to support two input representations and their different terminal policies. The removed helpers and exports reduce dead code and public surface. No further deletion is apparent without weakening regression coverage. The new test fixtures and helpers simplify repeated setup and keep the 24 focused CLI cases maintainable. Complexity delta
Total maintenance complexity decreases. The reducer adds local state complexity, but it removes duplicated authorities and dead public APIs. ValidationThe PR reports validation for build, typecheck, formatting, Biome checks, and 24 focused CLI tests. The test changes cover Host Turn failures, graph completion and failure exit codes, recovered sandbox boundaries, cancellations, step-cap failures, and later completion replacing a failed graph outcome. Final check status remains unverified without direct check results. Review-relevant risksThe PR changes user-visible CLI exit codes and outcome classification for The PR removes exported functions, aliases, and a public type. Consumers outside the reviewed call sites may be affected. These public-contract changes require independent human review under repository policy. No security, licensing, release, or governance effect was identified in the current diff. The person performing the merge must review the final diff. A maintainer makes the final determination. WalkthroughThe CLI now uses one classifier for live session events and stored messages. It reports explicit completion and failure outcomes, tracks sandbox recovery, validates Host and Graph exit codes, and removes unused exports and helpers. ChangesRuntime outcome handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change unifies CLI outcome handling and removes unused internal helpers; no actionable merge-blocking correctness or production risk is identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
dfc300b to
c865cd0
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. |
c865cd0 to
0f410bf
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
One material finding is inline.
Codex-assisted review performed under the maintainer-approved review workflow.
Normalize live SessionEvent and durable StoredMessage inputs through one outcome reducer. Keep incomplete live streams fail-closed while durable graph projections remain pending until their terminal Turn is stored, and pin ordinary and graph exit codes. Generated-by: Codex
Remove InvocationResult-era sandbox helpers and unused CLI wrappers and aliases while retaining the live SessionEvent boundary classifier and internal diff row renderer. Generated-by: Codex
Let the latest normalized terminal observation determine the CLI outcome, matching the append-only durable turn-state authority. Remove coverage and branches for terminal sequences that the Runtime Host projector cannot emit, and pin failed-to-completed recovery at the public adapter seam. Generated-by: Codex
Remove the stale recap-cleaner documentation and keep the sandbox failure reason type private to its only module. Generated-by: Codex
Treat raw stream boundary failures as unclassified until the corresponding outcome records recovery or an unresolved failure. Keep unresolved outcomes aggregated by outcome id so a later successful Graph result cannot overwrite an earlier boundary decision. Generated-by: Codex
Keep the established human-readable failure message when the final Agent Graph Turn is classified from durable messages, and pin the public CLI stderr contract. Generated-by: Codex
Track live and durable tool calls in the unified outcome classifier so a sandbox boundary failure is recovered only by a successful tool result from a later provider step. Same-step sibling successes and unpaired legacy history remain unresolved, while a later failure restores the unresolved state. Add public CLI regressions for ordinary and Graph runs plus coverage for later-step recovery, missing step identity, and repeated failures. Generated-by: Codex
0f410bf to
aff982d
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Two material findings are inline.
Codex-assisted review performed under the maintainer-approved review workflow.
Carry only the bounded sandbox failure reason across the Runtime Host live tool-result projection so CLI outcome classification does not depend on stripped result content. Bump the compatibility epoch because older closed-frame decoders reject the new field. Distinguish live terminal fallbacks from durable terminal replacement so trailing completion events cannot erase earlier errors or cancellations, while standalone failure-bearing stop reasons retain the core failure taxonomy. Generated-by: Codex
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
One material finding is inline.
Codex-assisted review performed under the maintainer-approved review workflow.
| content: { | ||
| kind: 'text', | ||
| text: '', | ||
| ...(event.sandboxFailureReason |
There was a problem hiding this comment.
[P1] Keep a denied boundary request from clearing this failure. On the normal non-interactive path, this projection marks the original sandbox_boundary_required result unresolved; the model then calls request_sandbox_boundary, and NonInteractiveInteractionController denies it. ToolRuntime returns that { request: { status: 'denied' }, changed: false } settlement as a successful JSON tool result, so its different provider step makes TurnOutcomeClassifier mark the boundary recovered. runOutcomeObserver then clears the raw failure flag, and a final model message can make maka run exit 0 without widening the sandbox or retrying the blocked operation. Track the tool name with the step and exclude request_sandbox_boundary itself from recovery (for both live and durable classification), then add ordinary and Graph regressions for failure -> denied request -> final output.
Track the tool name alongside provider-step identity in the unified run outcome classifier. A successful request_sandbox_boundary settlement is control-plane completion, not evidence that the blocked work recovered, so it cannot clear an unresolved sandbox failure. Pin ordinary and Graph public CLI behavior for sandbox failure followed by a denied boundary request and final output. Generated-by: Codex
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed the current head against the previous denied-boundary-request finding. Live and durable classification now retain each tool name with its provider step, so a successful request_sandbox_boundary settlement cannot clear the unresolved failure; the ordinary and Graph public CLI regressions both return failure as required. The complete build, related CLI/Runtime Host/ToolRuntime suites, formatting, lint, and required CI passed. No actionable findings remain.
Codex-assisted review performed under the maintainer-approved review workflow.
3 participants
Summary
SessionEventand durableStoredMessageinputs through one internal outcome reducer while preserving their distinct incomplete-terminal policies.maka runandmaka run --graph.Fixes #3088
Verification
npm --workspace maka-agent run buildnode --test packages/cli/dist/__tests__/runtime-host-run-command.test.js— 25 passednpm --workspace maka-agent run typechecknpm run format:checknpx biome checkon all changed filesAI use
Select exactly one:
Tool(s) and scope: Codex analyzed the duplicate outcome paths, implemented the reducer and dead-code cleanup, authored and ran the focused tests, and drafted this PR description. The commits include
Generated-by: Codextrailers.Checklist
Does this PR entail a change in behavior?