Repository navigation
Share discarded-error matching across Go analyzers - #65567
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
||
| require ( | ||
| github.com/rhysd/actionlint v1.7.12 | ||
| go.yaml.in/yaml/v3 v3.0.5 |
There was a problem hiding this comment.
Switched the workflow test to the existing gopkg.in/yaml.v3 package and restored go.yaml.in/yaml/v3 to indirect-only. Committed as f97b43d.
There was a problem hiding this comment.
Updated the test to use the existing goccy/go-yaml package and kept go.yaml.in/yaml/v3 indirect in go.mod (commit 7e0b20d).
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Queue validation rejects compiler-generated expressions, and removed routes break existing centralized commands.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Centralizes discarded-error matching across three Go analyzers, addressing #65436 while preserving JSON-specific checks.
Changes:
- Adds shared assignment-matching and package-resolution helpers.
- Migrates analyzers and tests aliases, named errors, and method calls.
- Also changes queue validation, command routing, and dependency classification.
| File | Description |
|---|---|
pkg/workflow/schemas/github-workflow.json |
Restricts queue values to literals. |
pkg/linters/strconvparseignorederror/strconvparseignorederror.go |
Uses the shared matcher. |
pkg/linters/jsonmarshalignoredeerror/jsonmarshalignoredeerror.go |
Uses shared helpers while retaining JSON-specific cases. |
pkg/linters/internal/astutil/astutil.go |
Adds reusable matching helpers. |
pkg/linters/internal/astutil/astutil_test.go |
Covers aliases and rejected assignment forms. |
pkg/linters/globwalkignorederror/globwalkignorederror.go |
Uses shared matching and package-path diagnostics. |
go.mod |
Marks YAML v3 as a direct dependency. |
.github/workflows/agentic_commands.yml |
Removes command routes and related metadata. |
| # runner-guard:ignore RGS-016 -- routing tables below contain emoji variation selectors (U+FE0F) and zero-width joiners (U+200D) used to render standard emoji sequences, not steganographic payloads. | ||
| env: | ||
| GH_AW_SLASH_ROUTING: '{"*":[{"workflow":"skillet","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🍳","status_comment":true}],"ace":[{"workflow":"ace-editor","events":["pull_request_comment"],"ai_reaction":"eyes","emoji":"✏️","status_comment":true}],"approach-validator":[{"workflow":"approach-validator","events":["issue_comment","pull_request_comment"],"ai_reaction":"eyes","emoji":"✅","status_comment":true}],"archie":[{"workflow":"archie","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🏛️","status_comment":true}],"cloclo":[{"workflow":"cloclo","events":["discussion","discussion_comment","issue_comment","issues","pull_request","pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"📊","status_comment":true}],"craft":[{"workflow":"craft","events":["issues"],"ai_reaction":"eyes","emoji":"✍️","status_comment":true}],"dependabot-burner":[{"workflow":"dependabot-burner","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🔥","status_comment":true}],"grumpy":[{"workflow":"grumpy-reviewer","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🔍","status_comment":true}],"matt":[{"workflow":"mattpocock-skills-reviewer","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🔍","status_comment":true}],"mergefest":[{"workflow":"mergefest","events":["pull_request_comment"],"ai_reaction":"eyes","emoji":"🔀","status_comment":true}],"nit":[{"workflow":"pr-nitpick-reviewer","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🔍","status_comment":true}],"plan":[{"workflow":"plan","events":["discussion_comment","issue_comment"],"ai_reaction":"eyes","emoji":"📋","status_comment":true}],"poem-bot":[{"workflow":"poem-bot","events":["issues"],"ai_reaction":"eyes","emoji":"🎭","status_comment":true}],"ponytail":[{"workflow":"ponytail-reviewer","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"✂️","status_comment":true}],"review":[{"workflow":"design-decision-gate","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🏗️","status_comment":true},{"workflow":"pr-code-quality-reviewer","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🔍","status_comment":true},{"workflow":"test-quality-sentinel","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"ruflo":[{"workflow":"ruflo-backed-task","events":["issue_comment"],"ai_reaction":"eyes","status_comment":true}],"scout":[{"workflow":"scout","events":["discussion","discussion_comment","issue_comment","issues","pull_request","pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🔭","status_comment":true}],"security-review":[{"workflow":"security-review","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🔒","status_comment":true}],"smoke-agent-all-merged":[{"workflow":"smoke-agent-all-merged","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-agent-all-none":[{"workflow":"smoke-agent-all-none","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-agent-public-approved":[{"workflow":"smoke-agent-public-approved","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-agent-public-none":[{"workflow":"smoke-agent-public-none","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-agent-scoped-approved":[{"workflow":"smoke-agent-scoped-approved","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-aider":[{"workflow":"smoke-aider","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"rocket","emoji":"🧑✈️","status_comment":true}],"smoke-call-workflow":[{"workflow":"smoke-call-workflow","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-checkout-pr-dispatch":[{"workflow":"smoke-checkout-pr-dispatch","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-claude":[{"workflow":"smoke-claude","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"heart","emoji":"🧪","status_comment":true}],"smoke-claude-on-copilot":[{"workflow":"smoke-claude-on-copilot","events":["pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-codex":[{"workflow":"smoke-codex","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"hooray","emoji":"🧪","status_comment":true}],"smoke-copilot":[{"workflow":"smoke-copilot","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-copilot-aoai-apikey":[{"workflow":"smoke-copilot-aoai-apikey","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-copilot-aoai-entra":[{"workflow":"smoke-copilot-aoai-entra","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-copilot-arm":[{"workflow":"smoke-copilot-arm","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-copilot-mai":[{"workflow":"smoke-copilot-mai","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"⚡","status_comment":true}],"smoke-copilot-sdk":[{"workflow":"smoke-copilot-sdk","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🔬","status_comment":true}],"smoke-copilot-small":[{"workflow":"smoke-copilot-small","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🪶","status_comment":true}],"smoke-create-cross-repo-pr":[{"workflow":"smoke-create-cross-repo-pr","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-crush":[{"workflow":"smoke-crush","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-cursor":[{"workflow":"smoke-cursor","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"rocket","emoji":"🖱️","status_comment":true}],"smoke-deepseek-harness":[{"workflow":"smoke-deepseek-harness","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-drive":[{"workflow":"smoke-drive","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"rocket","emoji":"💾","status_comment":true}],"smoke-gemini":[{"workflow":"smoke-gemini","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"rocket","emoji":"🧪","status_comment":true}],"smoke-github-claude":[{"workflow":"smoke-github-claude","events":["pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-goose":[{"workflow":"smoke-goose","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"rocket","emoji":"🪿","status_comment":true}],"smoke-kiro":[{"workflow":"smoke-kiro","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"rocket","emoji":"🧭","status_comment":true}],"smoke-multi-pr":[{"workflow":"smoke-multi-pr","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-opencode":[{"workflow":"smoke-opencode","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"rocket","emoji":"🧪","status_comment":true}],"smoke-otel-backends":[{"workflow":"smoke-otel-backends","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-pi":[{"workflow":"smoke-pi","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"rocket","emoji":"🧪","status_comment":true}],"smoke-project":[{"workflow":"smoke-project","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-pydantic":[{"workflow":"smoke-pydantic","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"rocket","emoji":"🐍","status_comment":true}],"smoke-service-ports":[{"workflow":"smoke-service-ports","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-temporary-id":[{"workflow":"smoke-temporary-id","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-test-tools":[{"workflow":"smoke-test-tools","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-update-cross-repo-pr":[{"workflow":"smoke-update-cross-repo-pr","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"souschef":[{"workflow":"pr-sous-chef","events":["pull_request_comment"],"ai_reaction":"eyes","emoji":"👨🍳","status_comment":true}],"squad-plan":[{"workflow":"squad-plan","events":["issue_comment"],"ai_reaction":"eyes","emoji":"🧑🤝🧑","status_comment":true}],"summarize":[{"workflow":"pdf-summary","events":["issue_comment","issues"],"ai_reaction":"eyes","emoji":"📄","status_comment":true}],"tidy":[{"workflow":"tidy","events":["pull_request_comment"],"ai_reaction":"eyes","emoji":"🧹","status_comment":true}],"unbloat":[{"workflow":"unbloat-docs","events":["pull_request_comment"],"ai_reaction":"eyes","emoji":"📝","status_comment":true}],"windows":[{"workflow":"windows","events":["discussion","discussion_comment","issue_comment","issues","pull_request","pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🪟","status_comment":true}]}' | ||
| GH_AW_SLASH_ROUTING: '{"*":[{"workflow":"skillet","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🍳","status_comment":true}],"ace":[{"workflow":"ace-editor","events":["pull_request_comment"],"ai_reaction":"eyes","emoji":"✏️","status_comment":true}],"approach-validator":[{"workflow":"approach-validator","events":["issue_comment","pull_request_comment"],"ai_reaction":"eyes","emoji":"✅","status_comment":true}],"archie":[{"workflow":"archie","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🏛️","status_comment":true}],"cloclo":[{"workflow":"cloclo","events":["discussion","discussion_comment","issue_comment","issues","pull_request","pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"📊","status_comment":true}],"craft":[{"workflow":"craft","events":["issues"],"ai_reaction":"eyes","emoji":"✍️","status_comment":true}],"dependabot-burner":[{"workflow":"dependabot-burner","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🔥","status_comment":true}],"grumpy":[{"workflow":"grumpy-reviewer","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🔍","status_comment":true}],"mergefest":[{"workflow":"mergefest","events":["pull_request_comment"],"ai_reaction":"eyes","emoji":"🔀","status_comment":true}],"nit":[{"workflow":"pr-nitpick-reviewer","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🔍","status_comment":true}],"plan":[{"workflow":"plan","events":["discussion_comment","issue_comment"],"ai_reaction":"eyes","emoji":"📋","status_comment":true}],"poem-bot":[{"workflow":"poem-bot","events":["issues"],"ai_reaction":"eyes","emoji":"🎭","status_comment":true}],"ruflo":[{"workflow":"ruflo-backed-task","events":["issue_comment"],"ai_reaction":"eyes","status_comment":true}],"scout":[{"workflow":"scout","events":["discussion","discussion_comment","issue_comment","issues","pull_request","pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🔭","status_comment":true}],"security-review":[{"workflow":"security-review","events":["pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🔒","status_comment":true}],"smoke-claude-on-copilot":[{"workflow":"smoke-claude-on-copilot","events":["pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-copilot":[{"workflow":"smoke-copilot","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-copilot-aoai-apikey":[{"workflow":"smoke-copilot-aoai-apikey","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-copilot-aoai-entra":[{"workflow":"smoke-copilot-aoai-entra","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-copilot-mai":[{"workflow":"smoke-copilot-mai","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"⚡","status_comment":true}],"smoke-copilot-sdk":[{"workflow":"smoke-copilot-sdk","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🔬","status_comment":true}],"smoke-copilot-small":[{"workflow":"smoke-copilot-small","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🪶","status_comment":true}],"smoke-github-claude":[{"workflow":"smoke-github-claude","events":["pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-otel-backends":[{"workflow":"smoke-otel-backends","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"smoke-service-ports":[{"workflow":"smoke-service-ports","events":["issue_comment","issues","pull_request","pull_request_comment"],"ai_reaction":"eyes","emoji":"🧪","status_comment":true}],"souschef":[{"workflow":"pr-sous-chef","events":["pull_request_comment"],"ai_reaction":"eyes","emoji":"👨🍳","status_comment":true}],"squad-plan":[{"workflow":"squad-plan","events":["issue_comment"],"ai_reaction":"eyes","emoji":"🧑🤝🧑","status_comment":true}],"summarize":[{"workflow":"pdf-summary","events":["issue_comment","issues"],"ai_reaction":"eyes","emoji":"📄","status_comment":true}],"tidy":[{"workflow":"tidy","events":["pull_request_comment"],"ai_reaction":"eyes","emoji":"🧹","status_comment":true}],"unbloat":[{"workflow":"unbloat-docs","events":["pull_request_comment"],"ai_reaction":"eyes","emoji":"📝","status_comment":true}],"windows":[{"workflow":"windows","events":["discussion","discussion_comment","issue_comment","issues","pull_request","pull_request_comment","pull_request_review_comment"],"ai_reaction":"eyes","emoji":"🪟","status_comment":true}]}' |
There was a problem hiding this comment.
The merged branch has the complete router, including all three /review handlers and /smoke-codex; the file now matches the pre-PR routing state, so no router change was needed.
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ 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.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One small simplification opportunity.
net: -8 lines possible.
Generated by ✂️ Ponytail Reviewer for #65567 · codex · gpt56 · 12.7 AIC · ⌖ 6.52 AIC · ⊞ 13.4K
Comment /ponytail to run again
| if pass == nil || pass.TypesInfo == nil || assign == nil || len(assign.Lhs) != 2 || len(assign.Rhs) != 1 { | ||
| return nil, "", "", false | ||
| } | ||
| var secondLHS ast.Expr |
There was a problem hiding this comment.
L54: shrink: loops retrieve fixed indexes after exact length guards. Use assign.Lhs[1] and assign.Rhs[0], 8 fewer lines.
There was a problem hiding this comment.
I kept the guarded loops: direct slice indexing is flagged by this repository's uncheckedsliceindex custom analyzer, even after the length checks. The safe form preserves lint compliance.
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.
|
There was a problem hiding this comment.
Request changes
Blocking regressions are still in this patch even after the analyzer refactor.
Blocking themes
pkg/workflow/schemas/github-workflow.jsonnow rejects expression-valuedqueue, so workflows that legitimately emit${{ github.event_name == 'pull_request' && 'single' || 'max' }}will fail schema validation..github/workflows/agentic_commands.ymldrops existing slash-command routes/workflows that were not part of the stated change, which is a production behavior change far outside a discarded-error helper extraction.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 45.2 AIC · ⌖ 7.93 AIC · ⊞ 19.4K
Comment /review to run again
🏗️ Design Decision Gate: ADR RequiredThis PR triggers ADR enforcement and no ADR was found. A draft ADR has been committed to your branch for review. Why this PR was gated
Business-logic changes land in ADR search results
Decision inferred from the diff
Draft ADR committed📄 Two items are marked
Next actionReview the draft ADR, resolve the two
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design — requesting changes due to a confirmed test-breaking regression bundled into this PR.
📋 Key Themes & Highlights
Key Themes
- Blocking regression:
pkg/workflow/schemas/github-workflow.jsondrops theexpressionSyntaxalternative forconcurrency.queue, butpkg/workflow/concurrency.gostill emits the${{ github.event_name == ... }}expression form for command-triggered workflows. Runninggo test ./pkg/workflow/... -run TestConcurrencyQueueSchemaValidationon this branch fails with'oneOf' failed, none matched. This file is unrelated to the PR's stated goal (sharing discarded-error AST matching across analyzers) and looks like an accidental revert swept in with the change. - Minor readability nit:
astutil.MatchDiscardedErrorCall(and the mirrored code injsonmarshalignoredeerror.go) replaces simpleLhs[1]/Rhs[0]indexing with manualfor-loops to extract a single already-bounds-checked element, adding noise without benefit.
Positive Highlights
- ✅ The new
astutil.MatchDiscardedErrorCall/PackageCallhelpers are a clean extraction — all three analyzers (globwalkignorederror,strconvparseignorederror,jsonmarshalignoredeerror) now share one well-tested matcher instead of duplicating package-resolution logic. - ✅
astutil_test.goadds solid coverage for the shared helper: aliased imports, named-error assignments, and method-call false positives are all exercised. - ✅
go.modcleanup correctly promotesgo.yaml.in/yaml/v3to a direct dependency now that it's imported directly in test code.
Please drop the unrelated github-workflow.json schema tightening (or update concurrency.go to match it) so TestConcurrencyQueueSchemaValidation passes again, then this is good to merge.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 61.2 AIC · ⌖ 14.6 AIC · ⊞ 10K
Comment /matt to run again
| "$ref": "#/definitions/expressionSyntax" | ||
| } | ||
| ], | ||
| "type": "string", |
There was a problem hiding this comment.
[/diagnosing-bugs] This removes the expressionSyntax alternative for concurrency.queue, but pkg/workflow/concurrency.go:43 still emits queue: ${{ github.event_name == 'pull_request' && 'single' || 'max' }} for command-triggered workflows. Compiling any such workflow now fails GitHub Actions schema validation with 'oneOf' failed, none matched.
💡 Repro
Running go test ./pkg/workflow/... -run TestConcurrencyQueueSchemaValidation -v on this branch fails with:
GitHub Actions schema validation failed ... 'oneOf' failed, none matched
- at '/concurrency': got object, want string
- at '/concurrency/queue': value must be one of 'single', 'max'
This looks unrelated to the PR's stated goal (sharing discarded-error matching across analyzers) — it appears to be an accidental revert bundled into this change. Please restore the oneOf with the expressionSyntax $ref, or confirm concurrency.go is being changed to stop emitting the expression form.
@copilot please address this.
There was a problem hiding this comment.
Restored the expressionSyntax alternative for concurrency.queue in commit 1af0c55; TestConcurrencyQueueSchemaValidation passes. The latest main merge also contains this schema fix.
| func MatchDiscardedErrorCall(pass *analysis.Pass, assign *ast.AssignStmt) (call *ast.CallExpr, pkgPath, funcName string, ok bool) { | ||
| if pass == nil || pass.TypesInfo == nil || assign == nil || len(assign.Lhs) != 2 || len(assign.Rhs) != 1 { | ||
| return nil, "", "", false | ||
| } |
There was a problem hiding this comment.
[/codebase-design] assign.Lhs[1] and assign.Rhs[0] are replaced with manual for-loops just to grab a single known-index element — this obscures intent versus the original direct indexing (lengths are already checked just above). Same pattern repeats in jsonmarshalignoredeerror.go's checkDiscardedJSONAssign.
💡 Suggestion
secondLHS := assign.Lhs[1]
blank, ok := secondLHS.(*ast.Ident)
...
rhs := assign.Rhs[0]
call, ok = rhs.(*ast.CallExpr)The bounds are already guarded by the len(...) check on the previous line, so direct indexing is safe and much easier to read than a loop with a break.
@copilot please address this.
There was a problem hiding this comment.
I kept the guarded loops here for the same reason: direct slice indexing is flagged by the repository's uncheckedsliceindex custom analyzer. The existing guarded iteration retains the required safety behavior.
There was a problem hiding this comment.
Reviewed with a correctness/maintainability focus (Impeccable modes distill/extract don't directly apply to this Go-only refactor).
Core refactor (astutil.go, the three analyzers, tests): Clean. MatchDiscardedErrorCall/PackageCall correctly centralize the shared logic, alias handling is preserved, and the verbose for ... { if index == N { ...; break } } pattern (instead of direct slice indexing) is necessary — I built and ran this repo's own uncheckedsliceindex custom analyzer against the new code and confirmed direct indexing (e.g. assign.Lhs[1]) would trigger it, while the loop form does not. go build ./... and the affected package tests all pass.
Pre-existing unresolved threads I independently verified as still valid and blocking:
pkg/workflow/schemas/github-workflow.json: removing theexpressionSyntaxoneOf branch forconcurrency.queueis a real regression. I reproduced it with a schema-validation unit test against the exact stringconcurrency.goemits for command/PR workflows (queue: ${{ github.event_name == 'pull_request' && 'single' || 'max' }}) and it now fails validation ('oneOf' failed, none matched).go.mod: movinggo.yaml.in/yaml/v3from indirect to directrequirelooks unrelated to this PR's stated scope (discarded-error matching); worth confirming it's intentional/needed.agentic_commands.yml: the diff drops routing for workflows unrelated to this change (/review,/matt,/smoke-codex, etc.) — likely a stale/regenerated lock artifact rather than an intended change.
No new inline comments added since these are already called out in open review threads. Requesting changes until the schema regression (and ideally the unrelated workflow/go.mod diffs) are resolved.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 109.7 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
|
@copilot resolve the merge conflicts on this branch. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…e-fix # Conflicts: # pkg/workflow/concurrency_command_test.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged the latest |
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…e-fix Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the PR finisher: restored queue-schema expression support, merged the latest |
|
🎉 This pull request is included in a new release. Release: |
The strconv, glob, and JSON analyzers duplicated assignment-shape checks and imported-package resolution for discarded errors. Centralize that matching while preserving JSON’s additional Unmarshal and expression-statement cases.