Skip to content

fix: deprecate max-turns and move timeout-minutes to YAML job property#138

Merged
jamesadevine merged 1 commit intomainfrom
jamesdevine/engine-config-cleanup
Apr 8, 2026
Merged

fix: deprecate max-turns and move timeout-minutes to YAML job property#138
jamesadevine merged 1 commit intomainfrom
jamesdevine/engine-config-cleanup

Conversation

@jamesadevine
Copy link
Copy Markdown
Collaborator

Summary

  • max-turns: Deprecated — was specific to Claude Code and is not supported by Copilot CLI. Front matter parsing is preserved for backwards compatibility, but a warning is emitted at compile time and the value is no longer passed as a CLI arg.

  • timeout-minutes: No longer emits --max-timeout as a Copilot CLI arg. Instead, it generates timeoutInMinutes on the PerformAgenticTask job in both standalone and 1ES pipeline templates.

Changes

  • src/compile/types.rs — Mark max_turns as deprecated in docs
  • src/compile/common.rs — Emit deprecation warning for max-turns, add generate_job_timeout(), update tests
  • src/compile/standalone.rs / onees.rs — Wire up {{ job_timeout }} replacement
  • templates/base.yml / 1es-base.yml — Add {{ job_timeout }} marker
  • AGENTS.md / prompts/ — Update documentation

- max-turns was specific to Claude Code and is not supported by Copilot CLI.
  It is now ignored at compile time with a deprecation warning. Front matter
  parsing is preserved for backwards compatibility.
- timeout-minutes no longer emits --max-timeout CLI arg. Instead it generates
  timeoutInMinutes on the PerformAgenticTask job in both standalone and 1ES
  templates.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jamesadevine jamesadevine merged commit 9887c97 into main Apr 8, 2026
4 checks passed
@jamesadevine jamesadevine deleted the jamesdevine/engine-config-cleanup branch April 8, 2026 11:33
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 8, 2026

🔍 Rust PR Review

Summary: Looks good overall — the deprecation/migration is clean and well-tested. One bug in the zero-value warning message.

Findings

🐛 Bugs / Logic Issues

  • src/compile/common.rs:323-325 — The warning text for timeout-minutes: 0 says "The agent job will time out immediately", but this is semantically incorrect. Azure DevOps interprets timeoutInMinutes: 0 as no timeout (unlimited duration). From the ADO docs: "A value of 0 means the job can run indefinitely." The warning should either be removed for the zero case (since 0 is a valid "no timeout" configuration), or corrected to say something like: "Setting timeout-minutes to 0 disables the ADO job timeout — the job will run indefinitely." Note: test_job_timeout_zero correctly asserts the YAML is emitted with timeoutInMinutes: 0, so the output is fine — just the warning text is wrong.

✅ What Looks Good

  • The deprecation path for max-turns is clean: parsing is preserved for BC, warning is emitted, CLI arg is silently dropped. No breakage for existing front matter files.
  • generate_job_timeout is appropriately minimal — a single match returning the property string or empty. No edge-case surprises.
  • YAML indentation is correct in both templates: the existing leading whitespace on the {{ job_timeout }} line handles indentation in both base.yml (4-space job property level) and 1es-base.yml (12-space nested job level) without needing replace_with_indent.
  • The unit test suite is comprehensive: covers with-value, without-value, zero-value, and the no-CLI-arg assertions for both deprecated fields.
  • No integration test fixtures reference timeout-minutes, so no golden-file updates were needed — confirmed in tests/fixtures/.

Generated by Rust PR Reviewer for issue #138 · ● 582.7K ·

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.

1 participant