Fix Codex fetch integration tests and restore timeout-minutes propagation fallback - #49858
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ 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. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (only 20 additions detected). |
There was a problem hiding this comment.
Pull request overview
Restores timeout propagation compatibility and fixes Codex fetch integration tests.
Changes:
- Adds legacy timeout fallback handling.
- Adds numeric and expression regression tests.
- Removes invalid Codex bash allow-list configuration.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/fetch_integration_test.go |
Fixes Codex test fixtures. |
pkg/workflow/agentic_engine.go |
Restores extracted timeout fallback. |
pkg/workflow/agentic_engine_test.go |
Covers legacy timeout forms. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| return v | ||
| } | ||
| } | ||
| if raw := strings.TrimSpace(workflowData.TimeoutMinutes); raw != "" { |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 23.4 AIC · ⌖ 9.31 AIC · ⊞ 5.4K
Comments that could not be inline-anchored
pkg/workflow/agentic_engine.go:714
The doc comment says "no raw-string parsing is needed" but the new fallback block (lines 727–734) does exactly that — it strips the timeout-minutes: prefix from a raw YAML string. The comment should be updated to reflect the new fallback path, e.g.:
// resolveStepTimeoutValue returns the timeout value string to emit on an
// agentic_execution step's timeout-minutes field. It first reads the
// already-parsed TimeoutMinutes from ParsedFrontmatter; when that is absent
// it falls back …
</details>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — one minor comment, overall solid fix.
📋 Key Themes & Highlights
Key Themes
- Stale doc comment:
resolveStepTimeoutValuedoc says "no raw-string parsing is needed" but the new fallback does exactly that. Flagged inline. - Regression coverage: two new test cases directly cover the bug being fixed — good practice.
- Fetch test fix: removing the invalid
tools.bashallow-list is the correct minimal fix; the test now validates the intended Codex fetch behavior without noise.
Positive Highlights
- ✅ The fallback strips the
timeout-minutes:prefix cleanly withstrings.CutPrefix— handles both numeric and expression forms. - ✅ New test cases use descriptive names and cover both value shapes (numeric and expression).
- ✅ No unrelated changes; PR scope is tight.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 27.7 AIC · ⌖ 7.89 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/agentic_engine.go:714
[/diagnosing-bugs] The function doc comment says no raw-string parsing is needed and only mentions ParsedFrontmatter, but the new fallback at lines 727-734 does parse raw strings from workflowData.TimeoutMinutes. The comment is now misleading. Please update it to describe both code paths. @copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (2 tests)
Quality Signals✅ Behavioral contracts tested: Both new test cases verify the timeout-minutes fallback mechanism added to ✅ Test infrastructure:
✅ Fixture cleanup: Deletions in Verdict
|
There was a problem hiding this comment.
Verdict: Request changes
Small, targeted fix, but the new raw-string fallback in resolveStepTimeoutValue skips the validation its sibling function does, risking invalid generated YAML on unexpected input.
💡 Themes
- The Codex bash allow-list test cleanup and the new legacy-timeout test cases are correct and match the described regression.
- The
resolveStepTimeoutValuefallback trustsworkflowData.TimeoutMinutesunconditionally after prefix-stripping, unlikeresolveAWFContainerAgentTimeoutMinutes, which validates the value is numeric before using it. SinceworkflowData.TimeoutMinutesis produced by generic YAML re-marshaling (extractTopLevelYAMLSection), non-numeric/quoted content can slip through, worth hardening for consistency and to avoid the malformed value being emitted verbatim into the generated Actions YAML.
🔎 Code quality review by PR Code Quality Reviewer · auto · 77.1 AIC · ⌖ 4.69 AIC · ⊞ 7.9K
Comment /review to run again
| if raw != "" { | ||
| return raw | ||
| } | ||
| } |
There was a problem hiding this comment.
The new fallback returns any non-empty raw value verbatim without validating it is a well-formed integer or GitHub Actions expression, unlike the near-identical logic in resolveAWFContainerAgentTimeoutMinutes.
💡 Why this matters
If workflowData.TimeoutMinutes contains a malformed value (e.g. leftover quotes from extractTopLevelYAMLSection producing timeout-minutes: "30", since a plain-string TimeoutMinutes gets quoted by the YAML marshaler, or any other unexpected content), this function emits it straight into the generated step YAML as timeout-minutes: <raw>. That produces an invalid GitHub Actions workflow only caught later at Actions runtime/schema-validation — a much harder failure to diagnose than catching it here.
resolveAWFContainerAgentTimeoutMinutes (awf_config.go:775-788) handles the same input source but validates with strconv.Atoi and falls back to the default on parse failure. This new code path should do the same (or explicitly allow GH Actions expression syntax) instead of trusting the raw string unconditionally.
Suggested test: add a case with a quoted/malformed non-numeric, non-expression TimeoutMinutes value to confirm the function falls back to the default rather than emitting invalid YAML.
There was a problem hiding this comment.
Fixed in 028ea2b. The raw fallback now validates with strconv.Atoi (positive integer required) and isExpression (GitHub Actions expression allowed), matching the sibling resolveAWFContainerAgentTimeoutMinutes pattern. Anything else is rejected with a log warning and the default is used. A new test case (legacy timeout-minutes field — malformed value falls back to default) covers this path across all registered engines.
…e doc comment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The CI failure was a stale golden file: Also updated the |
|
@copilot Please address the remaining blocker on this PR, then run the Outstanding review item:
Branch refresh may help once you are ready.
|
…to prevent malformed YAML Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Hardened in the latest commit. The raw |
|
🎉 This pull request is included in a new release. Release: |
The linked CI job failed in
pkg/workflowdue to two regressions: Codex integration tests used an invalid restrictedtools.bashconfig, and engine step timeout emission missed workflows that only had legacy extractedTimeoutMinutes.Timeout resolution compatibility
resolveStepTimeoutValue(...)to fall back toworkflowData.TimeoutMinuteswhenParsedFrontmatter.TimeoutMinutesis absent.timeout-minutes:prefix before returning the value.Regression coverage for timeout emission
TestAllEnginesEmitTimeoutMinuteswith legacy-form cases:timeout-minutes: 30timeout-minutes: ${{ inputs.timeout }}Codex fetch integration test alignment
tools.bashallow-list usage from Codex test workflows in:TestCodexFetchDisabledByDefaultTestNoWebFetchNoMCPFetchServerRun: https://github.com/github/gh-aw/actions/runs/30771772506