engine: forward timeout-minutes to execution step for codex, gemini, pi, antigravity#49495
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…steps
- Codex engine (the original issue): `timeout-minutes` from workflow frontmatter was
not forwarded to the Execute Codex CLI step, causing silent 6-hour job timeouts.
- Extended the same fix to gemini, pi, and antigravity engines which had the
same latent bug.
- Added TestCodexEngineTimeout_{Default,Explicit,Expression} in codex_engine_test.go.
- Added TestAllEnginesEmitTimeoutMinutes in agentic_engine_test.go — a systematic
cross-engine regression test that verifies every registered engine emits
timeout-minutes on its agentic_execution step.
- Recompiled all 266 workflow lock files; affected lock files now contain the
missing timeout-minutes field on Execute steps.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Forwards workflow timeout settings to Codex, Gemini, Pi, and Antigravity execution steps, preventing unintended six-hour defaults.
Changes:
- Adds explicit or default step-level timeouts to four engines.
- Adds Codex-specific and registry-wide regression tests.
- Recompiles affected workflow lock files.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/codex_engine.go |
Emits execution-step timeout. |
pkg/workflow/gemini_engine.go |
Emits execution-step timeout. |
pkg/workflow/pi_engine.go |
Emits execution-step timeout. |
pkg/workflow/antigravity_engine.go |
Emits execution-step timeout. |
pkg/workflow/codex_engine_test.go |
Tests default, explicit, and expression timeouts. |
pkg/workflow/agentic_engine_test.go |
Tests timeout emission across registered engines. |
.github/workflows/unbloat-docs.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/spec-enforcer.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/smoke-pi.lock.yml |
Adds compiled Pi timeout. |
.github/workflows/smoke-gemini.lock.yml |
Adds compiled Gemini timeouts. |
.github/workflows/smoke-codex.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/smoke-call-workflow.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/smoke-antigravity.lock.yml |
Adds compiled Antigravity timeouts. |
.github/workflows/schema-feature-coverage.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/schema-consistency-checker.lock.yml |
Adds compiled Pi timeout. |
.github/workflows/pr-sous-chef.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/poem-bot.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/necromancer.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/lint-monster.lock.yml |
Adds compiled Pi timeout. |
.github/workflows/issue-monster.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/issue-arborist.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/hippo-embed.lock.yml |
Adds compiled Pi timeout. |
.github/workflows/grumpy-reviewer.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/eslint-monster.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/duplicate-code-detector.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/dev.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/daily-observability-report.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/daily-news.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/daily-multi-device-docs-tester.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/daily-hippo-learn.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/daily-function-namer.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/daily-fact.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/daily-evals-report.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/daily-doc-updater.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/daily-cache-strategy-analyzer.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/daily-byok-ollama-test.lock.yml |
Synchronizes generated firewall configuration. |
.github/workflows/daily-awf-spec-compiler-surfacing.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/commit-changes-analyzer.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/codex-github-remote-mcp-test.lock.yml |
Adds compiled Codex timeout. |
.github/workflows/chaos-pr-bundle-fuzzer.lock.yml |
Adds compiled Pi timeout. |
.github/workflows/changeset.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/auto-triage-issues.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/ai-moderator.lock.yml |
Adds compiled Codex timeouts. |
.github/workflows/agent-persona-explorer.lock.yml |
Adds compiled Pi timeouts. |
.github/workflows/ab-testing-advisor.lock.yml |
Adds compiled Pi timeouts. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 45/45 changed files
- Comments generated: 0
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: engine: forward timeout-minutes to execution step for codex, gemini, pi, antigravity
The fix is correct and consistent with the existing pattern in claude_engine.go. All four engines now properly forward timeout-minutes to the agentic_execution step using strings.TrimPrefix(workflowData.TimeoutMinutes, "timeout-minutes: "), which correctly handles the format stored by extractTopLevelYAMLSection.
What was reviewed:
codex_engine.go,gemini_engine.go,pi_engine.go,antigravity_engine.go— timeout forwarding logic is identical to the reference implementation inclaude_engine.go✓- Default fallback to
DefaultAgenticWorkflowTimeoutwhenTimeoutMinutesis empty ✓ agentic_engine_test.go—TestAllEnginesEmitTimeoutMinutesprovides strong regression coverage across all registered engines for default, explicit, and expression timeout cases ✓- Lock file recompilation reflects the new timeouts correctly ✓
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 43.9 AIC · ⌖ 7.28 AIC · ⊞ 5.3K
…execution step Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (164 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report❌ Test Quality Score: 32/100 — Poor
📊 Metrics (4 tests)
Verdict
Recommendation: Remove the three redundant Codex timeout tests. The parametrized test
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on one consistency issue.
📋 Key Themes & Highlights
Key Issue
- Inconsistent timeout parsing: The four new engines use
strings.TrimPrefix(workflowData.TimeoutMinutes, "timeout-minutes: ")(exact single-space match), whilecopilot_engine_execution.gousesstrings.CutPrefix+strings.TrimSpace(robust). A value like"timeout-minutes:30"or"timeout-minutes: 30"would silently produce invalid YAML in the new engines. This inconsistency should be fixed by extracting a shared helper (see inline comment oncodex_engine.go:511).
Positive Highlights
- ✅
TestAllEnginesEmitTimeoutMinutesis excellent — a single cross-engine regression test that will catch any future engine that forgets to forward the timeout - ✅ Correct handling of both the explicit-value and empty-value (default) cases
- ✅ The evals
agentic_executionstep also picks up the timeout correctly becauseevalsData.TimeoutMinutesis empty, triggering the default path (20 min =DefaultAgenticWorkflowTimeout) - ✅ The
antigravityengine was included — easy to overlook since it is less prominent than the other three
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 95.3 AIC · ⌖ 10.6 AIC · ⊞ 7K
Comment /matt to run again
|
|
||
| // Add timeout at step level (GitHub Actions standard) | ||
| if workflowData.TimeoutMinutes != "" { | ||
| timeoutValue := strings.TrimPrefix(workflowData.TimeoutMinutes, "timeout-minutes: ") |
There was a problem hiding this comment.
[/codebase-design] strings.TrimPrefix with the exact literal "timeout-minutes: " is fragile — if extractTopLevelYAMLSection returns "timeout-minutes:30" (no space) or "timeout-minutes: 30" (two spaces), the prefix will not match and timeoutValue becomes the full raw string, emitting broken YAML like timeout-minutes: timeout-minutes:30.
copilot_engine_execution.go already uses the more robust pattern: strings.CutPrefix + strings.TrimSpace (line 327). The four new engines should align with that approach.
💡 Suggested shared helper
Extract the common logic into a package-level helper and call it from all five engines:
func resolveTimeoutMinutesValue(workflowData *WorkflowData, defaultMinutes int) string {
if workflowData.TimeoutMinutes == "" {
return strconv.Itoa(defaultMinutes)
}
raw := strings.TrimSpace(workflowData.TimeoutMinutes)
if after, ok := strings.CutPrefix(raw, "timeout-minutes:"); ok {
raw = strings.TrimSpace(after)
}
if raw != "" {
return raw
}
return strconv.Itoa(defaultMinutes)
}Then in each engine: stepLines = append(stepLines, " timeout-minutes: "+resolveTimeoutMinutesValue(workflowData, defaultMins))
@copilot please address this.
| } | ||
| } | ||
|
|
||
| func TestCodexEngineTimeout_Explicit(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The codex-specific tests (TestCodexEngineTimeout_Default, TestCodexEngineTimeout_Explicit, TestCodexEngineTimeout_Expression) duplicate what TestAllEnginesEmitTimeoutMinutes now covers for all engines. The per-engine tests add no additional edge case coverage but add maintenance cost. Consider either removing the redundant codex-specific tests or having them cover cases not in the cross-engine suite (e.g., verifying the timeout appears before the run: key, or testing timeout with a detection run IsDetectionRun: true).
@copilot please address this.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · auto · 56.7 AIC · ⌖ 10.9 AIC · ⊞ 7.8K
Comment /review to run again
| // Add timeout at step level (GitHub Actions standard) | ||
| if workflowData.TimeoutMinutes != "" { | ||
| timeoutValue := strings.TrimPrefix(workflowData.TimeoutMinutes, "timeout-minutes: ") | ||
| stepLines = append(stepLines, " timeout-minutes: "+timeoutValue) |
There was a problem hiding this comment.
This 8-line timeout-minutes if/else block is now duplicated verbatim across 6 engine files (claude, copilot-variant, antigravity, codex, gemini, pi), which undercuts the PR's own goal of preventing engines from silently missing this logic.
💡 Why this matters
The PR's stated motivation is that a future new engine could forget to forward timeout-minutes. Copy-pasting the identical block into 6 files doesn't remove that risk, it just relocates it: any future fix (e.g. hardening the TrimPrefix call below) now has to be applied identically in 6 places, and it's easy to miss one — which is exactly the bug class this PR is trying to close.
Suggested fix — extract a shared helper and call it from every engine:
func timeoutMinutesLine(workflowData *WorkflowData, indent string) string {
if workflowData.TimeoutMinutes == "" {
return fmt.Sprintf("%stimeout-minutes: %d", indent, int(constants.DefaultAgenticWorkflowTimeout/time.Minute))
}
v := strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(workflowData.TimeoutMinutes), "timeout-minutes:"))
return indent + "timeout-minutes: " + v
}The new TestAllEnginesEmitTimeoutMinutes test would then be verifying one code path instead of six independent copies, which is a much stronger regression guard than string-matching six near-identical implementations.
| if workflowData.TimeoutMinutes != "" { | ||
| timeoutValue := strings.TrimPrefix(workflowData.TimeoutMinutes, "timeout-minutes: ") | ||
| stepLines = append(stepLines, " timeout-minutes: "+timeoutValue) | ||
| } else { |
There was a problem hiding this comment.
strings.TrimPrefix(workflowData.TimeoutMinutes, "timeout-minutes: ") only strips on an exact literal match (one space, no extra whitespace); any deviation silently produces malformed doubled YAML like timeout-minutes: timeout-minutes: 15.
💡 Details
This mirrors the pre-existing pattern in claude_engine.go, so it isn't a new bug introduced here, but this PR extends the same fragile parsing to 4 more engines, quadrupling the blast radius if WorkflowData.TimeoutMinutes is ever produced with different formatting (e.g. copilot_engine_execution.go's own getCopilotTimeoutValue uses strings.TrimSpace + strings.CutPrefix("timeout-minutes:") without the trailing space, so the two implementations already tolerate different inputs). Today tools.go always emits the value as fmt.Sprintf("timeout-minutes: %d", ...), so the exact-match TrimPrefix currently works, but it's one refactor away from silently emitting invalid YAML with no compiler error, only failing at GitHub Actions parse time. Consider trimming defensively (strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(v), "timeout-minutes:"))), matching copilot's approach, and add a test asserting the emitted step contains the timeout-minutes: key exactly once (not just assert.Contains) so a doubled-value regression would actually fail.
PR Triage
Note: Large diff (mostly regenerated lock files); 3 CI failures need triage; mergeable_state blocked
|
|
@copilot please address the remaining review feedback, refresh the branch if GitHub allows it, and run the Current maintainer-facing blockers:
Please summarize which blockers are fixed, whether any review threads can be resolved, and what still needs maintainer action.
|
…ve redundant codex timeout tests; accept ADR-49495 Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All three blockers are now addressed in commit Shared helper / robust whitespace handling (changes-requested) Redundant codex timeout tests (Test Quality Sentinel) ADR gate |
| // WorkflowData field can be stored in either raw YAML ("timeout-minutes: 30") | ||
| // or bare ("30") form. When workflowData is nil or TimeoutMinutes is empty | ||
| // it falls back to DefaultAgenticWorkflowTimeout. | ||
| func resolveStepTimeoutValue(workflowData *WorkflowData) string { |
There was a problem hiding this comment.
@copilot don't parse, walk the parsed merged workflow frontmatter data instead.
There was a problem hiding this comment.
Done in the latest commit. resolveStepTimeoutValue now reads directly from workflowData.ParsedFrontmatter.TimeoutMinutes (*TemplatableInt32) — no string parsing needed. getCopilotTimeoutValue is also simplified to delegate to resolveStepTimeoutValue, and the cross-engine tests now populate ParsedFrontmatter.TimeoutMinutes instead of the raw TimeoutMinutes string field.
…TimeoutValue Instead of parsing the raw WorkflowData.TimeoutMinutes string (which requires stripping the 'timeout-minutes:' key prefix), walk the already- parsed merged workflow frontmatter data via ParsedFrontmatter.TimeoutMinutes (*TemplatableInt32). This removes the brittle string-splitting logic and makes the intent clearer. Also consolidates getCopilotTimeoutValue to delegate to resolveStepTimeoutValue, and updates TestAllEnginesEmitTimeoutMinutes to populate ParsedFrontmatter rather than the raw TimeoutMinutes string field. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. @copilot quick triage for this PR:
|
…atter - Update TestWasmGolden_AllEngines golden files (codex, gemini, pi, copilot) to include timeout-minutes on the agentic_execution step - Update TestWasmGolden_CompileFixtures golden files for version bump (1.0.75 → 1.0.77) from merged main - Fix TestCopilotEngineExecutionStepsWithCopilotSDKTimeoutExpression to populate ParsedFrontmatter.TimeoutMinutes instead of the raw WorkflowData.TimeoutMinutes string field Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All four failing checks are now fixed:
All unit tests pass locally. |
timeout-minutesin workflow frontmatter was silently dropped for the codex engine'sagentic_executionstep — and the same bug existed in gemini, pi, and antigravity engines. Without an explicit step-level timeout, GitHub Actions inherits the 360-minute job default, makingtimeout-minutesa no-op for these engines.Changes
codex_engine.go,gemini_engine.go,pi_engine.go,antigravity_engine.go: Added the sametimeout-minutesforwarding already present inclaude_engine.goandcopilot_engine_execution.go— emits the frontmatter value when set, falls back toDefaultAgenticWorkflowTimeout(20 min) otherwise.codex_engine_test.go: Three targeted tests for default, explicit value, and expression (${{ inputs.timeout }}) timeout on the codex step.agentic_engine_test.go:TestAllEnginesEmitTimeoutMinutes— iterates every engine registered in the global registry and assertstimeout-minutesis present on theagentic_executionstep for all three cases. Prevents future regressions when new engines are added.*.lock.yml: Recompiled; affected workflows (smoke-codex, smoke-gemini, smoke-pi, smoke-antigravity, and others using these engines) now emittimeout-minuteson their Execute steps.Run: https://github.com/github/gh-aw/actions/runs/30689325959
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
run: https://github.com/github/gh-aw/actions/runs/30692749654