Reject expression input for max-turn-cache-misses instead of silently defaulting#54707
max-turn-cache-misses instead of silently defaulting#54707Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
max-turn-cache-misses instead of silently defaulting
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has only 15 new lines of code in business logic directories (threshold: 100).
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Pull request overview
Attempts to reject expression values for integer-only max-turn-cache-misses.
Changes:
- Refactors positive-integer parsing and adds an expression test.
- Refreshes generated workflow harness hashes.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/engine_config_parser.go |
Updates positive-integer parsing. |
pkg/workflow/engine_config_parser_test.go |
Adds expression-input coverage. |
.github/workflows/smoke-copilot.lock.yml |
Refreshes harness hash. |
.github/workflows/smoke-copilot-aoai-entra.lock.yml |
Refreshes harness hash. |
.github/workflows/smoke-copilot-aoai-apikey.lock.yml |
Refreshes harness hash. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
| engineLog.Printf("Ignoring invalid %s value: %q", fieldName, rawStr) | ||
| return 0 |
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.
Request changes
This patch changes the logging path, but it still does not actually reject invalid max-turn-cache-misses expressions — the parser collapses them back into the same “unset” sentinel, so misconfigured workflows continue to compile and run with an implicit default.
Blocking theme
- Invalid
max-turn-cache-missesinput is still indistinguishable from an omitted value at the configuration boundary. - Because
0still means “not configured”, the new branch only adds a log line; it does not enforce the “expression input is invalid” contract described by the PR. - The tests codify the same fallback behavior, so they do not prove a real rejection path yet.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 7.04 AIC · ⌖ 8.12 AIC · ⊞ 7K
Comment /review to run again
| // treated as 0 (not configured) because these fields are integer-only. | ||
| // GitHub Actions expression strings (e.g. "${{ inputs.value }}") are treated | ||
| // as invalid for integer-only fields. | ||
| func parsePositiveIntValue(raw any, fieldName string) int { |
There was a problem hiding this comment.
This change still silently falls back to the default max-turn-cache-misses behavior, so it does not actually “reject” expression input in any enforceable way and will keep masking misconfigured workflows.
💡 Why this is still broken
parsePositiveIntValue now logs and returns 0 for expression-shaped strings, but 0 is the existing sentinel for “not configured”, and GetMaxTurnCacheMisses() later converts that straight back into the built-in or enterprise default. In other words, an author can still write ${{ inputs.max_turn_cache_misses }}, compilation still succeeds, and the workflow still runs with an implicit default rather than failing or preserving intent.
That means the PR description overstates the behavioral change: this is only better logging, not a real rejection path. If the goal is to reject expressions for integer-only fields, the parser needs a way to distinguish absent from present-but-invalid and surface that as a validation error.
A fix here usually means returning an (int, bool)/error-style result or recording a validation diagnostic instead of collapsing invalid input into the same 0 sentinel used for “unset”.
There was a problem hiding this comment.
The fix is correct and well-scoped. parsePositiveIntValue now explicitly rejects expression strings instead of silently treating them as 0, and the new test case confirms the behavior. The log message (engineLog.Printf) gives operators visibility when an invalid/expression value is ignored. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.5 AIC · ⌖ 8.95 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Ponytail review (over-engineering only): one finding — duplicated string-parsing branch in parsePositiveIntValue that mirrors parseIntOrExpressionValue.
net: -8 lines possible.
Generated by ✂️ Ponytail Reviewer for #54707 · auto · 21.4 AIC · ⌖ 6.78 AIC · ⊞ 7.3K
Comment /ponytail to run again
| func parsePositiveIntValue(raw any, fieldName string) int { | ||
| s := parseIntOrExpressionValue(raw, 1, fieldName) | ||
| if s == "" || isExpression(s) { | ||
| if val, ok := typeutil.ParseIntValue(raw); ok && val >= 1 { | ||
| return val | ||
| } | ||
| rawStr, ok := raw.(string) |
There was a problem hiding this comment.
L42-59: yagni: reimplements the trim/atoi string-parsing branch already in parseIntOrExpressionValue (L104-111) just to reject expressions. Extract a shared string-parsing helper, or call parseIntOrExpressionValue and treat isExpression results as invalid, instead of duplicating the logic.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — approving with one minor suggestion.
📋 Summary
What the PR does well
- ✅ The fix correctly tightens
parsePositiveIntValueto reject expression strings rather than silently returning0. - ✅
engineLog.Printfgives a visible signal when an invalid value is ignored — future debugging is easier. - ✅ The new test case precisely specifies the corrected behavior for
max-turn-cache-misses. - ✅ Lock-file bumps are the only other changes; no unrelated code was touched.
Minor gap
max-runs is the only other caller of parsePositiveIntValue and currently has no expression-rejection test. Adding one (see inline comment) would close the spec gap symmetrically.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 30.1 AIC · ⌖ 9.79 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/engine_config_parser_test.go:18
[/tdd] max-runs shares parsePositiveIntValue but has no expression-rejection test — the same silent-default bug could recur there unnoticed.
<details>
<summary>💡 Suggested addition</summary>
{name: "max-runs expression invalid", parse: parseMaxRunsValue, raw: "${{ inputs.max_runs }}", expected: 0},Adding this alongside the existing max-runs cases gives the same specification-level coverage the PR adds for max-turn-cache-misses.
</details>
@copilot please address thi…
…3f-a5d4-2f52046b8a38
|
@copilot this PR still needs forward progress before maintainer review.
Run: https://github.com/github/gh-aw/actions/runs/32553383419
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
github-actions
Bot
added
pr-agent:copilot
pr-priority:high
pr-risk:low
pr-type:bug
labels
PR Triage
|
PR TriageCategory: Rejects invalid expression input for max-turn-cache-misses. Mixed CI (2 failures/25 success) + CHANGES_REQUESTED alongside 2 APPROVED — needs CI fix before merge. Automated triage — run 32572524009
|
…3f-a5d4-2f52046b8a38
|
🛠️ Agentic Maintenance updated this pull request branch. |
|
@copilot valid expressions should still be allowed |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot the expression should be maintained into the lock.yml instead of being converted to 0 |
|
@copilot this PR still needs forward progress before maintainer review.
Run: https://github.com/github/gh-aw/actions/runs/32578995676
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 4468708 (with a follow-up guard in 00361c9): valid imported expressions are preserved in the generated lock file via |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Validated the current expression-preservation contract and pushed |
…3f-a5d4-2f52046b8a38
|
🛠️ Agentic Maintenance updated this pull request branch. |
max-turn-cache-missesaccepted expression-shaped strings but then silently degraded to the default behavior (0→ fallback to configured default) with no explicit invalid-path handling. This created a schema/parser consistency gap versus user expectations from nearby templatable limits.Parser behavior correction
parsePositiveIntValuenow performs strict positive-integer parsing directly (native int or numeric string).Targeted guardrail coverage
max-turn-cache-missesto codify that expression input is invalid and does not count as configured.Resulting behavior (before vs after)