Skip to content

feat: simplify opencode prompt send#164

Open
i-am-thor[bot] wants to merge 6 commits into
mainfrom
feat/simplify-opencode-prompt-send
Open

feat: simplify opencode prompt send#164
i-am-thor[bot] wants to merge 6 commits into
mainfrom
feat/simplify-opencode-prompt-send

Conversation

@i-am-thor
Copy link
Copy Markdown
Contributor

@i-am-thor i-am-thor Bot commented May 27, 2026

Summary

  • simplify the runner trigger success path to always return NDJSON starting with start
  • remove the runner busy/status/wait flow and the gateway busy retry contract
  • replace dynamic model limit loading with hardcoded limits for the supported OpenAI models and update tests/docs

Testing

  • pnpm --filter @thor/runner typecheck
  • pnpm --filter @thor/runner exec vitest run src/trigger.test.ts
  • pnpm --filter @thor/gateway typecheck
  • pnpm --filter @thor/gateway exec vitest run src/service.test.ts src/app.test.ts
  • pnpm --filter @thor/common typecheck
  • Unit Tests workflow
  • Core E2E workflow

AI-generated — verify before acting. View Thor context

Co-authored-by: Son Dao <son.dao@katalon.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR simplifies Thor’s Runner /trigger contract and Gateway dispatch flow by removing the “busy/status/wait” retry mechanism and standardizing successful trigger responses to NDJSON (always starting with a start event). It also replaces dynamic model context-limit loading with a hardcoded model-limit table for supported OpenAI models, updating tests and documentation accordingly.

Changes:

  • Runner: remove session status() busy check + abort settle wait; always return application/x-ndjson on success (one-line start when stream=false, full stream to done when stream=true).
  • Gateway: drop { busy: true } handling and stop parsing successful runner bodies; simplify TriggerResult to { rejected: true|false }.
  • Runner: replace provider-based model limit fetching/caching with a constant MODEL_CONTEXT_LIMITS map and update trigger tests.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/runner/src/index.ts Simplifies /trigger orchestration, unifies successful responses to NDJSON, removes busy/status/wait flow, and switches to constant model context limits.
packages/runner/src/trigger.test.ts Updates trigger harness/tests to assert NDJSON start receipt, no status checks, interrupt behavior, and new model-limit expectations.
packages/runner/src/event-bus.ts Removes waitForSessionSettled helper now unused after abort-settle logic removal.
packages/runner/src/event-bus.test.ts Removes unit tests for waitForSessionSettled and updates imports/fixtures accordingly.
packages/gateway/src/service.ts Simplifies runner trigger result contract; drops success-body parsing and {busy:true} retry behavior.
packages/gateway/src/service.test.ts Updates gateway tests to treat any 2xx runner response as accepted and removes {busy:true} fixtures.
packages/gateway/src/app.ts Removes “busy” outcome logging/branching; treats only rejected vs fired outcomes.
packages/gateway/src/app.test.ts Updates gateway integration tests to reflect new runner response behavior and retry-on-5xx.
packages/gateway/src/queue.ts Updates queue docs/comments to describe retryable failures rather than “runner busy”.
packages/gateway/src/queue.test.ts Updates test comment to reflect deferred retry semantics.
packages/common/src/service-env.ts Removes ABORT_TIMEOUT runner env since abort-settle waiting is removed.
docs/plan/2026052702_simplify-opencode-prompt-send.md Adds/updates the design plan documenting the new NDJSON contract and simplified control flow.
AGENTS.md Adds contributor guidance to keep MODEL_CONTEXT_LIMITS aligned with OpenCode model config changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
throw new Error(`Runner returned ${response.status}: ${text}`);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 1017aa2: the gateway now best-effort cancels successful runner response bodies before returning, and I added a service test covering the cancel path.


AI-generated — verify before acting. View Thor context

Co-authored-by: Son Dao <son.dao@katalon.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

- `emit()` (`:989-1026`) gates `res.write()` on the `stream` flag at `:1013`. The `start` event is already constructed at `:1028-1033` but never reaches the wire in fire-and-forget mode.
- Gateway consumer: `packages/gateway/src/service.ts:543-573`. Reads `json.busy`; that is the only field it consumes from the success body. Confirmed by grep — no callsite reads `accepted`, `sessionId`, or `resumed` from the gateway side.
- Dispatch handler busy-log: `packages/gateway/src/app.ts:1154-1161`. The `result.busy` branch returns without calling `ack()`; that is what causes the queue to retain the file for the next scan (`packages/gateway/src/queue.ts:70-74`).
- Verified behavior of opencode (from reading `/Users/son.dao/repos/daohoangson/opencode`):
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 57f6421: the plan now references the public upstream source at https://github.com/anomalyco/opencode instead of the local absolute path.


AI-generated — verify before acting. View Thor context

Co-authored-by: Son Dao <son.dao@katalon.com>
@daohoangson
Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Copy link
Copy Markdown
Contributor

Copilot AI commented May 31, 2026

@copilot resolve the merge conflicts in this pull request

Resolved in commit fix: resolve merge conflicts with main branch.

The four conflicting files were resolved as follows:

  • runner/src/index.ts and runner/src/event-bus.test.ts: kept our branch's removal of waitForSessionSettled and aligned the import extension to .ts (matching main's convention).
  • runner/src/trigger.test.ts: imported a new labels python3 bash wrappers test from main (with an unused correlation key), kept our simplified interrupt/busy tests, and dropped the old providerList-based tests that no longer apply after replacing dynamic model-limit loading with the hardcoded table.
  • gateway/src/service.test.ts: kept our NDJSON start-event mock response and updated the dynamic import extension to .ts.

Copilot AI requested a review from daohoangson May 31, 2026 14:53
@daohoangson
Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Copy link
Copy Markdown
Contributor

Copilot AI commented Jun 2, 2026

@copilot resolve the merge conflicts in this pull request

Resolved in commit 98d5d2e. The only conflict was in packages/gateway/src/queue.ts — a comment in the EventHandler type doc where main used "runner busy" and our branch used "retryable runner failure". Kept our branch's wording since this PR removes the busy concept.

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.

3 participants