Skip to content

refactor: simplify scenario execution by removing complex turn manage…#166

Closed
drewdrewthis wants to merge 4 commits into
mainfrom
issue165/refactor-scenario-runner-smart-runner-dumb-agents-architecture
Closed

refactor: simplify scenario execution by removing complex turn manage…#166
drewdrewthis wants to merge 4 commits into
mainfrom
issue165/refactor-scenario-runner-smart-runner-dumb-agents-architecture

Conversation

@drewdrewthis
Copy link
Copy Markdown
Collaborator

…ment

  • Remove pendingRolesOnTurn, pendingAgentsOnTurn, and turn orchestration logic
  • Simplify user(), agent(), judge() methods to call agents directly
  • Replace complex proceed() with simple user→agent→judge loop
  • Remove scriptCallAgent and callAgent abstraction layers
  • Keep fail-fast judge evaluation behavior
  • Reduce code complexity from 682 lines to 136 lines in execution system

This maintains the same functionality while dramatically simplifying the codebase.

@drewdrewthis drewdrewthis force-pushed the issue165/refactor-scenario-runner-smart-runner-dumb-agents-architecture branch from 15dddc2 to c3cf697 Compare November 25, 2025 20:18
@rogeriochaves rogeriochaves force-pushed the main branch 2 times, most recently from 77a92af to 9fdb87c Compare December 16, 2025 15:54
@drewdrewthis drewdrewthis added the grinding Grinder is actively managing this PR label May 25, 2026
…ment

- Remove pendingRolesOnTurn, pendingAgentsOnTurn, and turn orchestration logic
- Simplify user(), agent(), judge() methods to call agents directly
- Replace complex proceed() with simple user→agent→judge loop
- Remove scriptCallAgent and callAgent abstraction layers
- Keep fail-fast judge evaluation behavior
- Reduce code complexity from 682 lines to 136 lines in execution system

This maintains the same functionality while dramatically simplifying the codebase.
- Create scenario-execution.utils.ts with ScenarioExecutionUtils export
- Move convertAgentReturnTypesToMessages and extractErrorInfo to utils
- Update scenario-execution.ts to import and use utils
- Improve file structure compliance with single responsibility principle
- Simplify overly verbose class and method documentation
- Remove implementation details from API docs
- Focus on essential usage information
- Keep docs concise and user-focused
@drewdrewthis drewdrewthis force-pushed the issue165/refactor-scenario-runner-smart-runner-dumb-agents-architecture branch from 912626a to 7d09aff Compare May 25, 2026 03:00
Comment thread javascript/src/domain/scenarios/script-commands.ts Fixed
@drewdrewthis
Copy link
Copy Markdown
Collaborator Author

[grinder] BLOCKED — ACs not met, human decision needed

This PR (Nov 2025) aimed to simplify scenario execution from 682 → 136 lines by removing complex turn management. The main branch has since expanded the same files to 1,614 lines with substantially different architecture (voice adapter pattern, progressive discovery tools, span collectors, etc.).

During rebase, all 5 commits with conflicts in `scenario-execution.ts` were resolved in favor of HEAD — the simplification could not be applied without losing significant functionality added to main since Nov 2025.

What was actually merged: utility/doc additions from non-conflicted commits

  • `javascript/src/domain/scenarios/script-commands.ts` (new)
  • `javascript/src/execution/scenario-execution.utils.ts` (new)
  • JSDoc cleanup pass

What was NOT applied: core simplification — user/agent/judge method rewrites, removal of turn orchestration.

CI: green (zero failing, zero pending — ci-checks skipped)

Options for Drew:

  1. Close this PR (the simplification goal was superseded by the voice execution architecture)
  2. Re-open as a fresh PR targeting the current execution architecture
  3. Keep open as-is for the utility file additions only

Keeping the `grinding` label pending your decision.

@drewdrewthis
Copy link
Copy Markdown
Collaborator Author

[grinder] BLOCKED — ACs superseded, needs human decision

This Nov 2025 simplification (682→136 lines) is superseded by HEAD's 1614-line evolution. Conflicts resolved in favor of HEAD throughout rebase but the core AC (reduce to 136 lines) is fundamentally unmet. Needs Drew to decide: close as superseded, or rewrite ACs to match current HEAD?

Removing grinding label.

@drewdrewthis drewdrewthis removed the grinding Grinder is actively managing this PR label May 25, 2026
@github-actions github-actions Bot added the low-risk-change PR qualifies as low-risk per policy and can be merged without manual review label May 25, 2026
github-actions[bot]
github-actions Bot previously approved these changes May 25, 2026
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Approved by automation: PR qualifies as low-risk-change under the documented policy.

…ommands

ScenarioExecutionStateLike is used (onTurn/onStep callbacks); the wider
ScenarioExecutionLike was imported but never referenced. Flagged by
github-code-quality bot on PR #166.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@drewdrewthis drewdrewthis added the grinding Grinder is actively managing this PR label May 25, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Automated low-risk assessment

This PR was evaluated against the repository's Low-Risk Pull Requests procedure.

  • Scope: Update .cursor/rules/file-structure.mdc and add javascript/src/domain/scenarios/script-commands.ts (typed script command definitions) and javascript/src/execution/scenario-execution.utils.ts (utility functions for converting agent returns and extracting error info).
  • Exclusions confirmed: no changes to auth, security settings, database schema, business-critical logic, or external integrations.
  • Classification: low-risk-change under the documented policy.

The diff only modifies a documentation file and adds two new TypeScript files (typed script commands and scenario execution utilities). It does not touch authentication/authorization, secrets, database schemas/migrations, business‑critical logic, or external integrations, so it meets the low‑risk criteria. These changes are small, easily reverted, and do not alter protected systems or integrations.

An approving review has been submitted by automation. The PR may merge once required CI checks pass.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Approved by automation: PR qualifies as low-risk-change under the documented policy.

@drewdrewthis
Copy link
Copy Markdown
Collaborator Author

[grinder] Standing skip — per operator instructions

This PR (#166) is listed in the grinder's skip list alongside #154 and #185. CI is green and it shows MERGEABLE, but the standing instruction from the session operator says to skip this PR entirely.

Removing grinding label. If Drew wants this driven to pr-ready, remove it from the skip list.

@drewdrewthis drewdrewthis removed the grinding Grinder is actively managing this PR label May 25, 2026
@drewdrewthis drewdrewthis marked this pull request as ready for review May 26, 2026 09:41
@drewdrewthis
Copy link
Copy Markdown
Collaborator Author

Closing as superseded by #561 (voice/TS consolidation). This PR modifies javascript/src/execution/scenario-execution.utils.ts and javascript/src/domain/scenarios/script-commands.ts, both of which were removed from main when #561 landed (both 404 on main). The execution architecture this refactor targets no longer exists, so there's nothing left to rebase onto. Thanks for the work — the simplification intent was largely absorbed into the consolidated stack.

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

Labels

low-risk-change PR qualifies as low-risk per policy and can be merged without manual review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant