Fix Timeout type drift between BoundedQueriesConfig and AWFBoundedQueriesConfig - #53694
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Looks great! This is a solid type-safety improvement. Your fix elegantly resolves the The changes are well-scoped:
All tests are included, and the PR body clearly explains the motivation and impact. This is ready for review!
|
There was a problem hiding this comment.
Pull request overview
Aligns bounded-query timeout types to preserve nil-versus-zero semantics across frontmatter and generated AWF configuration.
Changes:
- Changes
AWFBoundedQueriesConfig.Timeoutto*int. - Copies timeout pointers directly and updates tests.
- Refreshes the documented struct signature.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/awf_config.go |
Aligns timeout type and extraction logic. |
pkg/workflow/bounded_queries_test.go |
Updates pointer assertions and JSON coverage. |
pkg/workflow/README.md |
Updates the generated type reference. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. This PR only aligns a Timeout field type from int to *int and simplifies extractBoundedQueriesConfig by removing a manual nil-check (net reduction in code). No over-engineering, dead flexibility, or unnecessary complexity found.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53694 does not have the 'implementation' label and has only 9 new lines of code in business logic directories (threshold: 100).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
Comment only: this change looks mechanically correct and I did not find a blocking regression in the touched lines.
Notes
I checked the timeout pointer alignment in the AWF config struct, the direct pointer copy in extractBoundedQueriesConfig, and the updated tests/docs. The nil-vs-zero behavior now matches the frontmatter shape, and I did not find a changed-line correctness issue worth blocking on.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 7.01 AIC · ⌖ 6.79 AIC · ⊞ 4.5K
Comment /review to run again
There was a problem hiding this comment.
Review Summary
The change correctly aligns AWFBoundedQueriesConfig.Timeout from int to *int, matching BoundedQueriesConfig.Timeout and preserving nil-vs-zero semantics so an omitted timeout is faithfully round-tripped through JSON.
One blocking issue: new(30) on line 610 of bounded_queries_test.go is invalid Go syntax — new accepts a type, not a value literal. This will not compile. A local variable (timeout := 30; &timeout) or a package ptr helper should be used instead.
Everything else (README update, extractBoundedQueriesConfig simplification, test assertions) looks correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 22.8 AIC · ⌖ 8.87 AIC · ⊞ 5.7K
| }, | ||
| Runtime: BoundedQueryRuntimeSbx, | ||
| Timeout: 30, | ||
| Timeout: new(30), |
There was a problem hiding this comment.
new(30) is not valid Go — new takes a type, not a value. This will fail to compile.
Use a local variable or a helper instead:
// option 1
timeout := 30
Timeout: &timeout,
// option 2 (if a ptr helper exists in the package)
Timeout: ptr(30),@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — requesting changes for a compile error introduced in the test file.
📋 Key Themes & Highlights
Key Themes
- Compile error in test:
Timeout: new(30)on line 610 ofbounded_queries_test.go—newtakes a type, not an integer literal. This will prevent the test package from compiling.
Positive Highlights
- ✅ Core fix is correct:
AWFBoundedQueriesConfig.Timeout int → *inteliminates nil-vs-zero ambiguity - ✅ Removing the explicit nil-check in
extractBoundedQueriesConfigsimplifies the code nicely - ✅ Tests correctly updated to use
require.NotNil+ pointer dereference andassert.Nilfor the zero-value case - ✅ README struct signature updated to stay in sync
Skill selection used fallback heuristic — triage unavailable; PR title contains "Fix" so
/diagnosing-bugswas selected.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 80.1 AIC · ⌖ 9.94 AIC · ⊞ 7.8K
Comment /matt to run again
| }, | ||
| Runtime: BoundedQueryRuntimeSbx, | ||
| Timeout: 30, | ||
| Timeout: new(30), |
There was a problem hiding this comment.
[/diagnosing-bugs] new(30) is not valid Go — new is a builtin that takes a type, not a value literal. This line will fail to compile.
💡 Suggested fix
Use an address-of with a temporary variable, consistent with pkg/workflow/compiler_safe_outputs_steps_test.go:
timeout := 30
Timeout: &timeout,Or add a small package-level helper (as done in checkout_manager_test.go):
ptr := func(n int) *int { return &n }
// ...
Timeout: ptr(30),Note: new(30) also appears on earlier lines (87, 191, 271) of this file for BoundedQueriesConfig.Timeout — those are pre-existing and should be addressed in a follow-up. This PR introduces one new occurrence on this line.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
BoundedQueriesConfig(yaml-tagged frontmatter shape) andAWFBoundedQueriesConfig(json-tagged AWF-config-file shape) are parallel structs for the bounded-query subsystem, but theirTimeoutfields had drifted:*intin one,intin the other. This risks silently mismatching nil-vs-zero timeout semantics for anyone converting between the two shapes.Type alignment
AWFBoundedQueriesConfig.Timeoutfromintto*intinpkg/workflow/awf_config.go, matchingBoundedQueriesConfig.Timeout.extractBoundedQueriesConfigto copy the pointer directly instead of dereferencing it, removing the manual nil-check that previously bridged the type mismatch.Tests & docs
pkg/workflow/bounded_queries_test.goto construct/assertTimeoutas a pointer.pkg/workflow/README.md.