Skip to content

fix: select exact production route executable - #692

Merged
ScriptedAlchemy merged 13 commits into
mainfrom
fix/issue-680-manifest-executable
Sep 6, 2026
Merged

fix: select exact production route executable#692
ScriptedAlchemy merged 13 commits into
mainfrom
fix/issue-680-manifest-executable

Conversation

@ScriptedAlchemy

@ScriptedAlchemy ScriptedAlchemy commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Summary

  • resolve one exact production executable and preparation from the leased manifest before starting the child
  • reject ineligible hosts and surfaces, missing executables, and missing preparation contracts without reaching the handler
  • never probe or retry a second Flight worker after import, preflight, or handler failure
  • follow routes.events[].execution for shared, standalone, and standalone-fallback event workers

Duplicate consolidation

Compared #684 and #692 symbol-by-symbol. #692 survives because it carries the manifest-owned binding from the leased parent into the child, has real compiled multi-server/import-sentinel coverage, and does not add the resolver abstraction prohibited by #680. #684 is closed as superseded and its remote branch is deleted. Ported its unique event execution-record semantics, shared hooks-worker path constant, missing preparation-export regression, standalone-fallback coverage, and bilingual Workbench wording.

Validation

Branch contains current origin/main (3ddbead90e).

  • pnpm build && pnpm typecheck && pnpm lint && pnpm test:unit — pass (4,425 tests; 4,419 passed, 6 skipped)
  • pnpm test:integration:run packages/agent-bundle/tests/route-invocation-dev-server.test.ts — pass (5 tests)
  • pnpm docs:site:build — pass (language parity; 0 broken links / 28,317 anchors)

Review hygiene

Deslop: GPT-5.6 Sol, 5 edits — removed unrelated timeout increases; retained one 60 s child-process timeout only after the full unit gate reproduced the 30 s failure.

Self-review

Reviewer: Claude Fable 5.1 Thinking High.

  • Hostless non-preflight events initially failed AB8251: fixed with one manifest-selected direct binding and compiled coverage.
  • Shared runtime initially ignored executable hosts: fixed by selecting the first eligible generated server and adding a multi-server regression.
  • Standalone routes could incorrectly select a shared server: fixed by honoring routes.events[].execution; added standalone import sentinel and fallback coverage.
  • Non-rendered cli: routes could reach a bin worker and fail AB8236: fixed to fail AB8251 before the child, with a handler sentinel.
  • The first CLI fix also rejected projected .ts MCP tools: corrected the extension check to cli: routes only and added a successful compiled projected-tool regression.
  • Parent-side binding failures were mislabeled as child failures: fixed to preserve the diagnostic message.
  • Merging current main (fix(workbench): bound producer / completed-envelope render history end to end #715) conflicted in production rendering: resolved by retaining one manifest-selected worker while adopting bounded render-event forwarding/history; targeted retention tests and the full gate pass.
  • Final re-review at b66a8e45fa found no concrete merge risks.

Closes #680

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@changeset-bot

changeset-bot Bot commented Sep 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: b66a8e4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
agent-bundle Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ScriptedAlchemy
ScriptedAlchemy merged commit 6bce189 into main Sep 6, 2026
3 checks passed
@ScriptedAlchemy
ScriptedAlchemy deleted the fix/issue-680-manifest-executable branch September 6, 2026 23:29

Copy link
Copy Markdown
Owner Author

Owner-requested review — original #680 findings addressed at the inspected head

Reviewed b66a8e45fa25154c29d9eb10efc3e7557b550c4f through GitHub. The PR merged as 6bce18953daebb7ea52c52fdfe19d035caf457bc while this review was in progress. This is a source/test review, not a claim that I executed local tests.

The implementation now fixes the specific execution-selection defects from the earlier audit:

  • workbench-server.ts reads the authoritative manifest from the acquired epoch and closes the reference on an invalid/missing manifest.
  • route-invocation-service.ts::productionBindingFor selects the owning server/bin/script and event preparation in the parent, checks operation-specific emitted host eligibility, and respects routes.events[].execution for shared, standalone and allowed fallback behavior.
  • route-invocation-production.ts imports only that preparation and runs one worker. It removes the directory scans, candidate CLI imports and error-message-driven worker retry. A missing required preparation export now fails instead of passing raw input through.

The changed compiled integration tests are materially stronger than manifest-only assertions: unrelated server import sentinels stay untouched; import/handler/preflight errors containing the old lookup-error wording do not select another worker; missing preparation/worker records fail; a Claude-only event submitted as Codex is rejected before route import/handler execution; valid denial does not reach the handler. The tests also preserve standalone and projected .ts tool behavior rather than solving the problem by rejecting every non-JSX source.

I found no additional concrete blocker in the inspected changes. Do not carry the original #680 probing/preflight findings forward as still open. Preserve the manifest-owned binding together with the bounded render forwarding adopted from main when subsequent branches are integrated.

The updated PR body records the local gate against main 3ddbead90e (4,419 unit tests passed, 6 skipped; 5 route-invocation integration tests; clean docsite). Those are the author's recorded results. The PR-triggered workflows were pending when queried, so I am not independently certifying their completion. The current tip's post-merge result remains the appropriate verification target under the repository's merge policy.

This closes the old execution-selection slice; it does not by itself establish native conformance for the 21 newly researched hosts.

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.

Workbench: resolve exact compiled executable from manifest; fail-closed preflight (no flight probing)

1 participant