Uh oh!
There was an error while loading. Please reload this page.
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
c865cd0CompareNote 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. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
c865cd0 to
0f410bfCompare
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.
Uh oh!
There was an error while loading. Please reload this page.
0f410bf to
aff982dCompare
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
dd1bcc7 to
fa30286CompareAstro-Han
commented
Aug 21, 2026
@hqhq1025 The branch has been rebased onto the current Automated request by Codex on behalf of @Astro-Han. |
hqhq1025
left a comment
There was a problem hiding this comment.
Requesting changes on head fa30286270fd07dbb51538f0540a9c04092cea8d.
Problem reconstruction
This PR correctly identifies the duplicated live/durable outcome classifiers as the root maintenance problem. Normalizing both sources into one reducer, preserving their different incomplete-terminal policies, deleting InvocationResult-era helpers, and pinning public CLI exit codes is the right direction. The latest rebase also correctly carries the bounded sandbox failure reason over the Runtime Host wire and allocates compatibility epoch 32.
Findings
P2: reconnect recovery can erase an unresolved sandbox failure from the active root Turn. The Session channel intentionally cuts queued events and rebuilds from the canonical transcript after a reconnect, but the transcript-replacement listener feeds only Graph terminal handling. The root classifier created in
#observeTurn()consumes only live events. If the sandbox failure is durable but its live frame is lost before the sequence cut, the replacement can later seed only the terminal completion; the classifier returns completed andmaka runcan exit 0.P2: the reducer collapses multiple sandbox failures into one state. Two blocked calls in one provider step both set the same
{ status: 'unresolved', failedStepId }. A later provider step that successfully retries only one call flips the whole Turn torecovered, even though the sibling operation never ran. The public command can consequently exit 0 with unresolved blocked work.
First-principles and Occam assessment
The shared reducer is simpler than the previous duplicated authorities, but its sandbox state is not yet rich enough for the behavior it owns. The final structure should preserve unresolved failures individually, or consume an explicit Runtime recovery fact, and it must be rebuildable from the canonical replacement transcript after a sequence cut. A single mutable boundary flag cannot represent multiple failures or reconnect recovery correctly.
Deletion and tests
The dead exports and InvocationResult helpers are correctly removed; I found no additional production deletion required for merge. The new CLI tests are behavior-focused rather than coverage-only. Add public ordinary-command reconnect coverage and a two-failures/one-retry case for both ordinary and Graph paths. Also strengthen the epoch regression: RUNTIME_HOST_COMPATIBILITY_EPOCH > 29 would still pass at the previous epoch 31, so it does not prove this wire change owns epoch 32.
Merge readiness
Not ready to merge. GitHub CI is green. Locally, 110 focused CLI/Runtime Host tests passed, CLI and Runtime Host typechecks passed, formatting, Astryx inventory, and git diff --check passed. The affected packages built successfully after serializing the workspace build; an initial full build:test attempt was invalidated by local concurrent/dist-generation behavior and was not used as evidence.
Residual gaps after the two blockers are fixed: packaged-binary smoke was not run, and the recovery semantics still rely on inferred provider-step progression rather than an explicit operation-recovery identity.
Codex-assisted adversarial re-review; I verified the exact live head, current-main integration, prior denied-boundary fix, Runtime Host producer/wire/projector chain, reconnect sequence-cut path, and public CLI tests before publication.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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
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
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
Rebuild the active Turn outcome from canonical transcript replacements and track unresolved sandbox failures independently so ambiguous partial recovery fails closed. Cover ordinary reconnect and multi-failure behavior across ordinary and Graph commands, and pin compatibility epoch 33 after integrating the latest main. Generated-by: Codex
90cf955 to
e86ef4fCompareAstro-Han
commented
Aug 21, 2026
@hqhq1025 The two requested changes are addressed in Automated message by Codex. |
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed the current head against both previous blockers. No actionable findings remain.
Problem and mechanism: the PR correctly removes the duplicated live/durable CLI outcome authorities and normalizes both representations through one reducer while preserving their different incomplete-terminal policies. The latest revision now rebuilds the active root classifier from canonical transcript replacements after a sequence cut, so reconnect recovery cannot erase a durable sandbox failure. It also tracks unresolved failures independently by tool identity and fails closed when the current wire cannot correlate a partial retry.
First principles and Occam: one reducer is the simpler final architecture. Canonical transcript replacement is the right reconnect authority, and a collection of unresolved failures accurately represents the state that a single mutable flag could not. The conservative ambiguous-recovery rule is appropriate until the wire carries explicit retry identity.
Optimality, deletion, and tests: the InvocationResult-era helpers and unused exports are correctly removed. I found no additional production code or low-value test that should be deleted, and no deeper refactor is required for this PR. The new ordinary and Graph regressions directly cover reconnect restoration and two-failures/one-retry behavior.
Merge verdict: ready to merge. Residual risk is intentionally conservative false failure when multiple blocked operations are all retried but cannot be correlated; that is safer than returning exit code 0 with unresolved work and can be improved later with explicit recovery identity.
Verification: reviewed head e86ef4f; required CI is green; npm run build:test passed; 158 focused CLI, session-driver, Runtime Host protocol, and continuity tests passed; git diff --check passed.
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?