Skip to content

feat(curiocity): make max turns configurable - #284

Merged
isolomatov-gd merged 2 commits into
griddynamics:mainfrom
shin4141:codex/fix-260-configurable-max-turns
Aug 19, 2026
Merged

feat(curiocity): make max turns configurable#284
isolomatov-gd merged 2 commits into
griddynamics:mainfrom
shin4141:codex/fix-260-configurable-max-turns

Conversation

@shin4141

Copy link
Copy Markdown
Contributor

Summary

  • add optional maxTurns to Curiocity case configuration
  • add a --max-turns CLI override and show the resolved value in dry-run output
  • carry the value through the matrix and TrialSpec into InteractionEngine
  • preserve the existing 100-turn engine default when the option is unset

Why

Curiocity already accepts maxTurns in InteractionEngine, but no configuration or CLI path could supply it. Long-running trials therefore always used the hard-coded default even when their wall-clock timeout was configured independently.

Validation

  • npm test — 432 passed
  • npm run lint
  • npm run build
  • node dist/cli.js run --source test/fixtures/cases --max-turns 7 --dry-run

Fixes #260

AI assistance

AI assistance was used to inspect the issue, implement the focused change, and run validation. I reviewed the complete diff.

Signed-off-by: Shin <128954611+shin4141@users.noreply.github.com>
@isolomatov-gd
isolomatov-gd marked this pull request as draft August 18, 2026 17:37
@isolomatov-gd
isolomatov-gd marked this pull request as ready for review August 18, 2026 17:37
@isolomatov-gd

isolomatov-gd commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Thanks @shin4141.

To fix:

  • Add maxTurns to docs/architecture.md case-config example.
  • Add --max-turns to docs/architecture.md run-options table.
  • Add maxTurns to demo/cases/healthcheck/config.json, matching README.
  • Always print the effective turn cap in dry-run, not only when set.

Optional, your call: consider DEFAULT_MAX_TURNS in config/defaults.ts beside DEFAULT_TIMEOUT_SEC, and optionally expose a max_turns input in e2e-testing.yml.

On the first two: docs/ARCHITECTURE.md:172 points at src/curiocity/docs/architecture.md as the full design, so README and that doc now disagree about the config surface. On the defaults question: defaults.ts is the declared D13 lowest-precedence layer, and maxTurns is currently the only optional scalar in ResolvedCaseConfig — your shape avoids duplicating the constant, which is defensible, so treat it as a discussion.

What checks out: the plumbing is exactly right. trialSpecSchema in shared/ipc.ts:29 was the critical one — zod strips unknown keys, so without it maxTurns would have been silently dropped before IPC.

The failing triage check is our bot timing out, not your code.

Signed-off-by: Shin <128954611+shin4141@users.noreply.github.com>
@shin4141

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. The requested follow-ups are reflected in 63f79af: the architecture example and run-options table document maxTurns, the healthcheck config includes it, and dry-run always shows the effective cap. Local validation is green: 155/155 suites, 432/432 tests, lint, build, and git diff --check; dry-run was verified with 100, 7, and 9. I left the discussion-only defaults placement and optional workflow input unchanged. Ready for re-review.

@isolomatov-gd isolomatov-gd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @shin4141 — all four required items verified, and item 5 by execution rather than reading: with no maxTurns in the case config and no flag passed, dry-run prints maxTurns=100, and that 100 is the same constant the engine falls back to at engine.ts:152. --max-turns 7 prints 7, --max-turns 0 is correctly rejected.

Your call on the defaults placement is the right one — exporting DEFAULT_MAX_TURNS from engine.ts and consuming it in both the engine and the printer keeps one source of truth, which is better than duplicating it in defaults.ts. The workflow input stays out, as you said.

Docs check out: docs/architecture.md:385 and :518, and demo/cases/healthcheck/config.json:4 now agrees with the README example it claims to quote.

42 test files / 432 tests pass, tsc --noEmit clean, merges cleanly against current main.

Approved.

@isolomatov-gd
isolomatov-gd merged commit 2195c4b into griddynamics:main Aug 19, 2026
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.

[ROSETTA] curiocity maxTurns is hardcoded, unlike configurable timeoutSec

2 participants