Skip to content

Clarify same-context subcommand boundaries - #419

Open
LeanAndMean wants to merge 5 commits into
mainfrom
feature/issue-413-clarify-subcommand-contract
Open

Clarify same-context subcommand boundaries#419
LeanAndMean wants to merge 5 commits into
mainfrom
feature/issue-413-clarify-subcommand-contract

Conversation

@LeanAndMean

Copy link
Copy Markdown
Owner

Summary

  • Retain the delegate callable after a neutral 44-trial real-runtime evaluation and clarify that the current agent executes returned subcommand instructions immediately in the same conversation.
  • Restrict same-context loading to live active callers and delegate-only targets, reject report-pending work and root-inclusive cycles without mutation, and preserve caller-based tool scoping.
  • Align bundled Mach 12 commands, diagnostics, architecture guidance, and regression coverage with the enforced subcommand contract.

Test plan

  • Run npm run typecheck
  • Run npm run build
  • Run npm test
  • Run npm run lint
  • Complete the 44-trial cross-provider real-runtime name evaluation
  • Verify same-context loading and parked-input behavior through isolated RPC smoke tests

Fixes #413

@LeanAndMean

Copy link
Copy Markdown
Owner Author

Automated Review: Clarify same-context subcommand boundaries

Critical

None.

Important

F1: A terminal report and delegation in the same tool batch can start subcommand work after completion has been committed.

The new report-pending gate in packages/scramjet/src/delegate.ts:162-166 only observes lifecycle state when delegate executes. Model tool calls are parallel by default (packages/agent/src/agent.ts:272, packages/agent/src/agent-loop.ts:386-391), and a batch terminates only when every result has terminate: true (packages/agent/src/agent-loop.ts:538-540). Therefore, when one assistant message emits delegate before report_scramjet_command_status, delegate can return the subcommand body before the status tool sets lastReport; its non-terminating result also prevents the accepted terminal report from ending the batch. The next model turn receives both the delegated instructions and the accepted terminal result while lifecycle state is already reported.

I ran a disposable Agent-loop probe with the relevant shape:

let reported = false;
const tools = [
  { name: "delegate", execute: async () => ({ content: [{ type: "text", text: reported ? "rejected" : "delegated-body" }] }) },
  { name: "status", execute: async () => { reported = true; return { content: [{ type: "text", text: "completed" }], terminate: true }; } },
];
// First model response: [delegate(), status()]; second response: ordinary assistant text.

Observed output:

{"streamCalls":2,"reported":true,"toolResults":["delegated-body","completed"],"lastText":"continued-after-report"}

This contradicts the new contract that “no more subcommand work may start” once terminal status is pending and can delay routing while additional work runs under reported lifecycle state. Enforce terminal reporting at the tool-batch boundary (rather than relying only on execution-time state), and add real Agent-loop regressions for both [delegate, terminal-report] and [terminal-report, delegate]. Per mach12:silent-failure-hunter.

Suggestions

S1: Make delegate-only routing diagnostics lifecycle-aware. suggest_scramjet_next_steps is idle-gated, yet its new validation message tells the agent to load the target via delegate (packages/scramjet/src/suggest-next-steps.ts:120-122), which rejects idle calls. Completed next-step validation gives the same advice while a terminal report is pending (packages/scramjet/src/auto-continue.ts:263-268), another phase where delegate rejects. State instead that delegate-only commands must be loaded by an active caller during command work; do not instruct the agent to call delegate from phases where it is unavailable. The not_subcommand recovery at packages/scramjet/src/delegate.ts:182-186 should likewise avoid recommending the idle-only suggestion tool as an immediate action. Per mach12:comment-analyzer.

S2: Strengthen the report-pending no-mutation regression with populated state. The test at packages/scramjet/tests/delegate.test.ts:336-395 starts with an empty delegate stack/sidebar/journal, so it would not detect a rejection path that accidentally cleared earlier delegated state. Seed a prior frame and history sentinel, as the cycle-preservation coverage does at packages/scramjet/tests/delegate.test.ts:603-636, and assert they survive the rejection. Per mach12:test-analyzer.

Strengths

  • The live-caller, delegate-only-target, stale-registry, and root-inclusive cycle gates are explicit and precede normal success-path mutation (packages/scramjet/src/delegate.ts:155-205).
  • Caller-based tool-scope intersection remains independent of nesting depth and sibling frames (packages/scramjet/src/delegate.ts:194-204), with focused regression coverage.
  • The command catalog, bundled Mach 12 prose, and authoring/vision documentation consistently distinguish same-context subcommands from separate-agent and future top-level routing.
  • Completeness review found all issue 413 acceptance criteria and latest-plan requirements represented, including delegate-only active callers, root back-edges, RPC input parking, and the documented 44-trial evaluation.
  • Full verification reported 2,268 passing workspace tests plus successful typecheck, build, and lint; no premature version or changelog change is present.

Solution Assessment

  • Root request: Determine whether PR 419 safely and completely enforces issue 413’s same-context subcommand contract before merge.
  • Candidates considered:
    1. Config/docs only — insufficient for F1; packages/scramjet/docs/command-authoring.md and packages/scramjet/docs/scramjet-vision.md can describe the boundary but cannot enforce mixed tool-batch ordering.
    2. Documented extension point used as intended — partially viable; ToolDefinition.executionMode exists at packages/coding-agent/src/core/extensions/types.ts:499-506, but serialization alone does not change the all-results termination rule in packages/agent/src/agent-loop.ts:538-540.
    3. Small code change — viable; enforce terminal-report exclusivity through the existing Agent/Scramjet tool-batch hooks and add focused ordering tests.
    4. New abstraction or integration layer — unnecessary; existing tool execution and lifecycle primitives are sufficient.
  • Chosen tier: 3 — the defect is runtime behavior that prose and current execution-time guards cannot solve.
  • Proposed size: Approximately 30–60 LOC plus focused tests.

Reviewed by GPT-5.6 Sol

This is an automated review.

@LeanAndMean

Copy link
Copy Markdown
Owner Author

Independent Review Assessment

Assessing review comment: #419 (comment)

No finding was subsequently discussed, resolved, or deferred in the PR conversation.

Classifications

F1 — Genuine issue

Original finding: A terminal report and delegation in the same tool batch can start subcommand work after completion has been committed.

The problem is real: model tools run in parallel by default (packages/agent/src/agent.ts:272, packages/agent/src/agent-loop.ts:385-391), while a mixed batch stops only if every result terminates (packages/agent/src/agent-loop.ts:538-539). Thus the accepted status result at packages/scramjet/src/command-status.ts:392-395 cannot stop a batch containing non-terminating delegate, and another provider turn starts at packages/agent/src/agent-loop.ts:210-214. A real Agent-loop probe reproduced continuation in both call orders.

Sound fix approach: Add a backward-compatible unconditional batch-termination mode for accepted terminal reports, preserving the current all-results rule for ordinary terminate: true, and cover both [delegate, terminal-report] and [terminal-report, delegate] with Agent-loop regressions.

S1 — Nitpick

Original finding: Make delegate-only routing diagnostics lifecycle-aware.

The wording is misleading and worth a small correction. Idle suggestion validation tells the agent to use delegate even though it requires an active command (packages/scramjet/src/suggest-next-steps.ts:72-74,120-121); completed validation is deferred until idle after the active command is cleared (packages/scramjet/src/auto-continue.ts:1067-1070,810-812), not report-pending as the review stated, but the recommendation is still unavailable in that phase. Context-specific messages are a safe net improvement, including removing immediate idle-only suggestion advice from packages/scramjet/src/delegate.ts:182-185.

S2 — Nitpick

Original finding: Strengthen the report-pending no-mutation regression with populated state.

The existing case starts with empty stack/sidebar/journal state (packages/scramjet/tests/delegate.test.ts:336-346), so it would not detect accidental clearing of earlier same-turn delegation state. Seeding and snapshotting populated state, following the cycle-preservation pattern at packages/scramjet/tests/delegate.test.ts:603-636, is a cheap and focused improvement.

Summary

  • Genuine issues: 1
  • Nitpicks: 2
  • False positives: 0
  • Deferred: 0
  • Regressions: 0

Solution Assessment

  • Root request: Determine which review findings warrant changes and identify the minimum safe implementation.
  • Candidates considered:
    1. Config/settings/env — not viable for F1; toolExecution only selects parallel versus sequential execution (packages/agent/src/agent-loop.ts:385-391) and cannot make one terminating result stop a mixed batch.
    2. Existing documented extension point — insufficient; current terminate: true semantics require every finalized result to terminate (packages/coding-agent/docs/extensions.md:1832, packages/agent/src/types.ts:366-375).
    3. Small code change — viable: add one backward-compatible result termination mode, adopt it for accepted terminal reports, and add focused regressions.
    4. New abstraction/integration layer — unnecessary; the existing tool execution and lifecycle paths are sufficient.
  • Chosen tier: 3 — current termination semantics cannot express an unconditional stop while preserving compatibility.
  • Proposed size: Approximately 25–40 production LOC plus focused tests and documentation.

Staged Implementation Plan

Required Stage 1 — Enforce terminal tool-batch completion (F1)

  • packages/agent/src/types.ts — add an explicit result termination mode, preserving omitted mode as today’s all-results behavior.
  • packages/agent/src/agent-loop.ts — terminate after sibling results finalize when either every result requests normal termination or any result requests unconditional termination; preserve the mode through result finalization/hooks.
  • packages/scramjet/src/command-status.ts — mark accepted terminal reports for unconditional batch termination; leave rejected and continuing reports unchanged.
  • packages/agent/tests/tool-termination.test.ts — prove legacy mixed batches continue and unconditional termination suppresses the next provider request.
  • packages/scramjet/tests/command-status.test.ts — assert only accepted terminal reports request unconditional termination.
  • packages/scramjet/tests/delegate-status-batch.test.ts — exercise both delegate/status call orders through the real Agent loop and assert no post-report model turn.
  • packages/coding-agent/docs/extensions.md, packages/scramjet/docs/command-authoring.md, and UPSTREAM_DIVERGENCE.md — document the additive termination contract and runtime divergence.

Optional Stage 2 — Correct lifecycle-specific diagnostics (S1)

  • Update packages/scramjet/src/suggest-next-steps.ts, packages/scramjet/src/auto-continue.ts, and packages/scramjet/src/delegate.ts so each error explains where delegate-only subcommands can be loaded without recommending an unavailable tool.
  • Update the corresponding assertions in packages/scramjet/tests/suggest-next-steps.test.ts, packages/scramjet/tests/auto-continue.test.ts, and packages/scramjet/tests/delegate.test.ts.

Optional Stage 3 — Preserve populated rejection state (S2)

  • In packages/scramjet/tests/delegate.test.ts, perform a successful delegation, transition to reported state, snapshot stack/sidebar/journal/lifecycle/generation, attempt another delegation, and assert report_pending plus exact state preservation.

Assessed by GPT-5.6 Sol

@LeanAndMean

Copy link
Copy Markdown
Owner Author

Addressed review findings F1, S1, and S2 in commit c9fa20ba.

  • Added backward-compatible tool-result termination modes: existing mixed batches retain all-results semantics, while accepted terminal command reports use terminationMode: \"any\" to suppress another provider turn after every sibling result finalizes.
  • Added Agent-loop and Scramjet integration regressions for legacy behavior, unconditional termination, hook preservation, and both delegate/status call orders.
  • Corrected delegate-only routing diagnostics so idle/completed phases no longer recommend an unavailable immediate delegate call.
  • Strengthened report-pending rejection coverage with populated delegate stack, sidebar, journal, lifecycle, and generation state.
  • Documented the extension contract, command-authoring behavior, and upstream runtime divergence.

Verification: full build, typecheck, lint, and all 2,275 workspace tests pass. The termination mode deliberately does not cancel sibling tools; it waits for their normal finalization and persisted result artifacts before stopping the provider loop.

Reviewed by GPT-5.6 Sol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify the same-context subcommand tool contract

1 participant