Improve frontmatter diagnostics for invalid tools config shape#43409
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
tools config shape
🤖 PR Triage — Run §28715668077
Improves frontmatter diagnostics for invalid tools config shape. +137 lines new error file, +4/-1 Go, tests included. Medium complexity.
|
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This PR improves YAML/frontmatter diagnostics in gh-aw so malformed tools.<name> frontmatter produces a more accurate caret location, a clearer (tool-aware) message, and is classified as a syntax-first failure for recovery guidance.
Changes:
- Re-anchors certain YAML parser diagnostics to the offending scalar line/value (instead of the nested child key line) and renders updated source context for the new position.
- Expands syntax error classification patterns and adds regression tests to ensure YAML syntax issues get the correct “fix syntax first” hinting.
- Adds a YAML parser message translation for the “scalar with child keys” pattern.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/frontmatter_error.go | Adjusts frontmatter error positioning/message for a known scalar-with-nested-keys YAML mistake and re-renders context at the improved position. |
| pkg/workflow/error_recovery.go | Treats additional YAML/tool-shape parse failures as syntax-critical to prioritize syntax remediation guidance. |
| pkg/workflow/error_recovery_test.go | Adds a regression test ensuring YAML syntax errors receive syntax-category guidance (not unrelated hints). |
| pkg/workflow/data/action_pins.json | Large formatting-only reindent of embedded action pins data (appears unrelated to the PR’s stated purpose). |
| pkg/workflow/compiler_yaml_test.go | Adds a regression test for tools.github scalar + nested key caret/message behavior. |
| pkg/parser/yaml_error.go | Adds translation for a specific goccy/go-yaml internal phrase into a clearer author-facing message. |
| pkg/parser/yaml_error_test.go | Adds coverage for the new YAML parser phrase translation. |
| pkg/actionpins/data/action_pins.json | Large formatting-only reindent of embedded action pins data (appears unrelated to the PR’s stated purpose). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/8 changed files
- Comments generated: 3
- Review effort level: Low
| valueCol := strings.Index(parentLine, parentValue) | ||
| if valueCol < 0 { | ||
| valueCol = colNum - 1 | ||
| } | ||
|
|
||
| return strconv.Itoa(parentLineNum), strconv.Itoa(valueCol + 1), | ||
| fmt.Sprintf("tools.%s tool config must be an object, not a string (for example: toolsets: [default])", parentKey) | ||
| } |
| { | ||
| "entries": { | ||
| "actions-ecosystem/action-add-labels@v1.1.3": { | ||
| "repo": "actions-ecosystem/action-add-labels", | ||
| "version": "v1.1.3", | ||
| "sha": "c96b68fec76a0987cd93957189e9abd0b9a72ff1", | ||
| "inputs": { | ||
| "github_token": { | ||
| "description": "A GitHub token.", | ||
| "default": "${{ github.token }}" | ||
| }, | ||
| "labels": { | ||
| "description": "The labels' name to be added. Must be separated with line breaks if there're multiple labels.", | ||
| "required": true | ||
| }, | ||
| "number": { | ||
| "description": "The number of the issue or pull request." | ||
| }, | ||
| "repo": { | ||
| "description": "The owner and repository name. e.g.) Codertocat/Hello-World", | ||
| "default": "${{ github.repository }}" | ||
| "entries": { | ||
| "actions-ecosystem/action-add-labels@v1.1.3": { | ||
| "repo": "actions-ecosystem/action-add-labels", | ||
| "version": "v1.1.3", |
| { | ||
| "entries": { | ||
| "actions-ecosystem/action-add-labels@v1.1.3": { | ||
| "repo": "actions-ecosystem/action-add-labels", | ||
| "version": "v1.1.3", | ||
| "sha": "c96b68fec76a0987cd93957189e9abd0b9a72ff1", | ||
| "inputs": { | ||
| "github_token": { | ||
| "description": "A GitHub token.", | ||
| "default": "${{ github.token }}" | ||
| }, | ||
| "labels": { | ||
| "description": "The labels' name to be added. Must be separated with line breaks if there're multiple labels.", | ||
| "required": true | ||
| }, | ||
| "number": { | ||
| "description": "The number of the issue or pull request." | ||
| }, | ||
| "repo": { | ||
| "description": "The owner and repository name. e.g.) Codertocat/Hello-World", | ||
| "default": "${{ github.repository }}" | ||
| "entries": { | ||
| "actions-ecosystem/action-add-labels@v1.1.3": { | ||
| "repo": "actions-ecosystem/action-add-labels", | ||
| "version": "v1.1.3", |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (3 tests)
Verdict
References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Skills-Based Review Summary 🧠Applied Overall verdict: REQUEST_CHANGES — the core diagnostic improvements are well-structured, but there are a few correctness/coverage items worth addressing before merge. 📋 Issues Filed (7 inline comments)
@copilot please address the review comments above.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on test coverage gaps and one correctness issue.
📋 Key Themes & Highlights
Key Themes
- Dead detection branch (
frontmatter_error.go:155): the raw-parser-wording guard inimproveFrontmatterDiagnosticis unreachable becauseTranslateYAMLMessageruns first; only the translated form is ever seen. - Unit test gaps for new functions:
improveFrontmatterDiagnostic,renderSourceContextForPosition, and the two newclassifyErrorMessagearms ("must be an object, not a string","unexpected ':'") are covered only through the compiler integration harness, not direct unit tests. - Message accuracy for non-string scalars:
isLikelyYAMLContainerdoesn't filter booleans/integers, so the error message can claim"not a string"when the value is actually an integer or boolean. - Duplicate utility:
parsePositiveIntis defined both here and incompilerenv; should be extracted to a shared location consistent with the existingpkg/parsersharing pattern.
Positive Highlights
- ✅ The two-layer fix (translate at the parser layer + re-point the location in
improveFrontmatterDiagnostic) is architecturally clean: each layer does exactly one thing. - ✅ The
nearestAncestorKeywalk correctly skips blank lines and YAML comments, and handles the indent-depth guard robustly. - ✅ The column offset calculation for the scalar value (not the key) gives users a precise caret.
- ✅ Consistent test naming convention (
TestBuildPrioritizedErrorReportFromMessages_*) maintained throughout.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 129.4 AIC · ⌖ 6.22 AIC · ⊞ 6.6K
Comment /matt to run again
| // point at the true source line and provide user-facing wording. | ||
| func improveFrontmatterDiagnostic(content, line, col, message string) (string, string, string) { | ||
| lower := strings.ToLower(strings.TrimSpace(message)) | ||
| isScalarWithNestedKey := |
There was a problem hiding this comment.
[/tdd] The isScalarWithNestedKey detection runs after TranslateYAMLMessage, so the raw parser wording ("value is not allowed in this context") can never reach this branch — only the translated wording can. The first condition is dead code.
💡 Details
Call site in createFrontmatterError:
message = parser.TranslateYAMLMessage(message) // line 101
line, col, message = improveFrontmatterDiagnostic(...) // line 102After translation the message is already "value cannot have child keys here — ...", so the first guard in improveFrontmatterDiagnostic is unreachable.
Either remove the first condition and keep only strings.Contains(lower, "value cannot have child keys here"), or add a test that passes the raw (untranslated) message directly to improveFrontmatterDiagnostic to prove the first branch is intentionally reachable.
@copilot please address this.
| } | ||
|
|
||
| ancestorKey := nearestAncestorKey(lines, parentLineNum-1, parentIndent) | ||
| if ancestorKey != "tools" { |
There was a problem hiding this comment.
[/tdd] improveFrontmatterDiagnostic silently passes through whenever the ancestor key is not "tools", but there is no test verifying this guard. A scalar-with-children mistake under permissions: or engine: would produce the raw YAML error wording rather than a helpful message — and the function would return unchanged without any signal.
💡 Suggested test
Add a table-driven unit test for improveFrontmatterDiagnostic directly, covering the non-tools ancestor case:
{
name: "non-tools parent is not rewritten",
content: "permissions:\n contents: \"invalid-string\"\n extra: value",
line: "3", col: "5",
message: "value cannot have child keys here — ...",
// expect line/col/message unchanged
}This makes the guard explicit and prevents silent failures if the scope is widened later.
@copilot please address this.
| fmt.Sprintf("tools.%s tool config must be an object, not a string (for example: toolsets: [default])", parentKey) | ||
| } | ||
|
|
||
| func parsePositiveInt(v string) int { |
There was a problem hiding this comment.
[/codebase-design] parsePositiveInt is defined twice: once here and once in pkg/workflow/compilerenv/manager_test.go (which even has a regression test TestParsePositiveIntEnvVar_OverflowRegression). Duplicating a parsing utility fragments test coverage and makes future fixes diverge.
💡 Suggestion
Extract parsePositiveInt to a shared internal package (e.g., pkg/internal/strutil) or to pkg/parser, and have both call sites import it. This is consistent with the existing pattern of sharing TranslateYAMLMessage through pkg/parser.
@copilot please address this.
| parentIndent := len(parentMatch[1]) | ||
| parentKey := parentMatch[2] | ||
| parentValue := strings.TrimSpace(parentMatch[3]) | ||
| if parentValue == "" || isLikelyYAMLContainer(parentValue) { |
There was a problem hiding this comment.
[/tdd] isLikelyYAMLContainer only guards against block scalars (|, >) and empty collections ({}, []). A parent value like 123, true, false, or null would pass through and produce a misleading tools.<key> tool config must be an object, not a string error message even though the value is not a string.
💡 Suggestion
Rename to isLikelyNonScalar (or widen the guard) and add coverage for the integer/boolean cases:
{
name: "integer parent value is not rewritten",
content: "tools:\n github: 42\n toolsets: [default]",
// expect line/col/message unchanged (42 is not a string)
}Alternatively, keep the check narrow but ensure the generated message says "not a scalar" rather than "not a string" so it covers all scalar types.
@copilot please address this.
| strings.Contains(lower, "did not find expected key"), | ||
| strings.Contains(lower, "missing ':' after key"), | ||
| strings.Contains(lower, "unexpected ':'"), | ||
| strings.Contains(lower, "must be an object, not a string"): |
There was a problem hiding this comment.
[/tdd] "must be an object, not a string" is the exact substring of the message produced by improveFrontmatterDiagnostic ("tools.github tool config must be an object, not a string"), so the classifier correctly promotes these to SeverityCritical/syntax. However, the new TestBuildPrioritizedErrorReportFromMessages_YAMLSyntaxGetsSyntaxHint test only covers "missing ':' after key" — the "must be an object, not a string" and "unexpected ':'" paths each need their own test case to prove they route to SeverityCritical and get the correct suggestion.
💡 Suggested additions to error_recovery_test.go
func TestBuildPrioritizedErrorReportFromMessages_ToolScalarErrorGetsSyntaxHint(t *testing.T) {
message := `/tmp/wf.md:4:11: error: tools.github tool config must be an object, not a string (for example: toolsets: [default])`
report := BuildPrioritizedErrorReportFromMessages([]string{message}, true)
require.Len(t, report.DisplayedErrors, 1)
assert.Equal(t, SeverityCritical, report.DisplayedErrors[0].Severity)
assert.Equal(t, "syntax", report.DisplayedErrors[0].Category)
}@copilot please address this.
| return "" | ||
| } | ||
|
|
||
| func renderSourceContextForPosition(content string, targetLine, targetCol int) string { |
There was a problem hiding this comment.
[/tdd] renderSourceContextForPosition has no direct unit test. The only coverage is through the integration path (TestCompileWorkflowWithInvalidYAML), which makes it hard to verify edge cases: targetCol past the line's length, a targetLine of exactly len(lines), or an empty content string.
💡 Why this matters
The caret rendering (strings.Repeat(" ", 7+targetCol-1)) will silently produce a misaligned caret if targetCol is larger than the actual line width. A dedicated unit test would catch this without needing a full compiler round-trip.
Consider extracting the test cases from the existing compiler integration test into a focused TestRenderSourceContextForPosition table test.
@copilot please address this.
| # Test Workflow | ||
|
|
||
| Invalid YAML with scalar github tool config that has nested keys.`, | ||
| expectedErrorLine: 4, // highlight the invalid github scalar value line, not the nested key line |
There was a problem hiding this comment.
[/tdd] The test YAML content is technically invalid — github: "invalid-string" with an indented child key is a YAML syntax error, not a valid frontmatter snippet — so the test asserts only the first compiler error path. There's no assertion that expectedMessagePart does not appear in any other error produced on the same input, which means a latent regression could slip through if the parser emits the old wording as a second error.
💡 Suggestion
Add an excludedMessagePart field to the test struct and assert that the raw YAML parser wording ("map key-value is pre-defined") is absent from all error messages, not just the main one. This is consistent with the pattern already used in TestTranslateYAMLMessage.
@copilot please address this.
Records the design decision to intercept and rewrite YAML scalar-with- nested-key diagnostics in the error formatting layer rather than modifying the upstream parser or adding pre-parse structural checks.
There was a problem hiding this comment.
Review: Improve frontmatter diagnostics for invalid tools config shape
Overall the PR is well-structured and the intent is clear: better caret position, better message, better hint category for the scalar-with-nested-key YAML pattern in tools: frontmatter. The new test coverage is solid.
I found two correctness issues that should be fixed before merge, plus two non-blocking notes.
🔴 Blocking
-
Dead-code detection branch (
frontmatter_error.go:156) — the raw parser phrase that the first||arm matches has already been translated beforeimproveFrontmatterDiagnosticis called; only the second arm fires. If the translated phrase changes, detection silently stops working. -
Classification routing conflict (
error_recovery.go:273) — the new"must be an object, not a string"guard in thesyntax / Criticalcase will match the improved message, making it critical-syntax and triggeringshouldSuppressCascadingError, which can hide real tools-config errors. Intended severity/category should be confirmed and a targeted test added.
🟡 Non-blocking
-
Column accuracy for quoted scalars (
frontmatter_error.go:201) —strings.Indexwith a still-quotedparentValuepoints at the opening quote, not the value body; divergence possible for single-quoted or unquoted scalars. -
Scope documentation gap (
frontmatter_error.go:197) — the function is tools-only but the comment implies generic improvement; a clarifying doc comment would reduce future confusion.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 101.4 AIC · ⌖ 6.59 AIC · ⊞ 4.8K
| lower := strings.ToLower(strings.TrimSpace(message)) | ||
| isScalarWithNestedKey := | ||
| (strings.Contains(lower, "value is not allowed in this context") && strings.Contains(lower, "map key-value is pre-defined")) || | ||
| strings.Contains(lower, "value cannot have child keys here") |
There was a problem hiding this comment.
Dead code branch — unreachable condition
The first clause of the || will never match because message has already been run through parser.TranslateYAMLMessage() on line 102 of createFrontmatterError before improveFrontmatterDiagnostic is called:
// createFrontmatterError, lines 101-102
message = parser.TranslateYAMLMessage(message)
line, col, message = improveFrontmatterDiagnostic(content, line, col, message)By the time we reach this check the raw parser phrase "value is not allowed in this context. map key-value is pre-defined" has already been replaced with "value cannot have child keys here — ...", so the first pair of strings.Contains calls (lines 156–157) is unreachable. If the translated phrase in yaml_error.go is ever changed, detection silently breaks.
Simplify to a single check:
isScalarWithNestedKey := strings.Contains(lower, "value cannot have child keys here")@copilot please address this.
| return line, col, message | ||
| } | ||
|
|
||
| valueCol := strings.Index(parentLine, parentValue) |
There was a problem hiding this comment.
First-match column is wrong for quoted strings
strings.Index(parentLine, parentValue) finds the value in the raw source line. But when the YAML value is a quoted string like "invalid-string", parentValue has been through strings.TrimSpace(parentMatch[3]) (line 186), which still includes the quotes. The regex capture group (.+?) for group 3 in yamlKeyLinePattern will include the surrounding quotes.
For the documented example:
github: "invalid-string"parentValue = "invalid-string" (with quotes), so the reported column points at the opening quote, not the value itself. The test at line 211 asserts expectedErrorColumn: 11 — verify that this column truly lands on the value, not on a position that diverges for single-quoted or unquoted scalars.
If the intent is to highlight the value content (not the quote character), strip surrounding quotes before computing valueCol:
bareValue := strings.Trim(parentValue, "'\"")\n
if bare := strings.Index(parentLine, bareValue); bare >= 0 {\n valueCol = bare\n}@copilot please address this.
| } | ||
|
|
||
| ancestorKey := nearestAncestorKey(lines, parentLineNum-1, parentIndent) | ||
| if ancestorKey != "tools" { |
There was a problem hiding this comment.
Only tools parent key is handled — other keys silently fall back to generic message
The check if ancestorKey != "tools" hard-codes the only key for which the improved diagnostic fires. That is intentional for this PR, but the code in improveFrontmatterDiagnostic claims via its comment to improve any "scalar-with-nested-key" parser diagnostic.
A user who writes:
engine: "bad-string"\n sub: valuewill still get the raw translated message ("value cannot have child keys here") instead of a tool-aware message. Consider either:
- Broadening the check to any known scalar-enforced top-level key, or
- Adding a doc comment to
improveFrontmatterDiagnosticexplicitly stating it is tools-only at this stage.
This is a non-blocking suggestion; the current behaviour is at least no worse than before.
@copilot please address this.
| strings.Contains(lower, "did not find expected key"), | ||
| strings.Contains(lower, "missing ':' after key"), | ||
| strings.Contains(lower, "unexpected ':'"), | ||
| strings.Contains(lower, "must be an object, not a string"): |
There was a problem hiding this comment.
Classification routing conflict: tools.github ... error lands in syntax instead of tools
The new message produced by improveFrontmatterDiagnostic is:
tools.github tool config must be an object, not a string (for example: toolsets: [default])
In classifyErrorMessage (line 262), this message matches "must be an object, not a string" at line 273, placing it in the syntax bucket with severity Critical. But it also contains "tools." (line 312), which would place it in the tools bucket with severity High.
Due to switch fall-through rules the first matching case wins, so the message is classified as syntax / Critical. That triggers shouldSuppressCascadingError and will suppress all non-critical, non-configuration errors — potentially hiding real tools-config issues from the user.
The "must be an object, not a string" guard was added to catch this exact error pattern; verify the intended severity and category: is it a YAML syntax violation (syntax / Critical) or a tool misconfiguration (tools / High)? If it is the former, the test at line 153 of error_recovery_test.go (TestBuildPrioritizedErrorReportFromMessages_YAMLSyntaxGetsSyntaxHint) should be extended to cover this specific message.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — The diagnostic improvement logic has three correctness bugs that affect the quality of the output it was designed to fix, plus a structural fragility that makes the whole path silent-failure-prone.
### Blocking issues (4)
-
Caret off-by-one (
renderSourceContextForPosition, line 271) —strings.Repeat(" ", 7+targetCol-1)is consistently one short. The source context^always points at the wrong character. The test only validates the VSCode header line, not the context block, so this is invisible to the suite. -
strings.Indexfinds key before value (improveFrontmatterDiagnostic, line 201) — when key and value share a substring (e.g.foo: foo),strings.Index(parentLine, parentValue)returns the key offset. The caret ends up on the key name, defeating the diagnostic improvement. -
Dead code + fragile coupling (line 155-157) —
improveFrontmatterDiagnosticruns afterTranslateYAMLMessage, so condition 1 ofisScalarWithNestedKeyis always false (the raw parser phrase is gone). The function silently depends on the exact wording of the translation output. Any future rewording of the translation silently breaks the entire feature with no test failure. -
lineNum <= 1over-strict guard (line 163) — incorrectly excludeslineNum == 2, which has a valid parent atlines[0]. Should belineNum < 2.
🔎 Code quality review by PR Code Quality Reviewer · 150.4 AIC · ⌖ 9.62 AIC · ⊞ 5.4K
Comment /review to run again
| } | ||
| fmt.Fprintf(&b, "%s %3d | %s\n", prefix, lineNum, lines[lineNum-1]) | ||
| if lineNum == targetLine { | ||
| fmt.Fprintf(&b, "%s^\n", strings.Repeat(" ", 7+targetCol-1)) |
There was a problem hiding this comment.
Caret is consistently off by one position: the rendered ^ always points one character to the left of the intended column, so the improved diagnostic still has misaligned output.
💡 Analysis and fix
The line format is "%s %3d | %s\n" where:
%sprefix = 1 char (>or%3d= 3 chars|= 3 chars
Total chars before content = 8.
For targetCol=1 the caret formula strings.Repeat(" ", 7+1-1) = 7 spaces places ^ at index 7, but content starts at index 8 — one short.
For targetCol=11:
> 4 | github: "invalid-string"
^ ← lands here (col 10)
Should be:
> 4 | github: "invalid-string"
^ ← col 11
Fix:
fmt.Fprintf(&b, "%s^\n", strings.Repeat(" ", 8+targetCol-1))This bug affects every caller of renderSourceContextForPosition. The test for expectedErrorColumn: 11 validates only the VSCode-format header line, not the caret in the source context block — so this stays invisible to the test suite.
| return line, col, message | ||
| } | ||
|
|
||
| valueCol := strings.Index(parentLine, parentValue) |
There was a problem hiding this comment.
strings.Index finds the first substring match and will return the wrong column when the value string also appears as (or inside) the key name: the caret will point at the key, not the value.
💡 Analysis and fix
parentValue is extracted from the line with yamlKeyLinePattern, but strings.Index(parentLine, parentValue) scans the whole line from the beginning, including the key portion.
Reproduction:
tools:
foo: foo # key and value are identicalparentValue = "foo"strings.Index(" foo: foo", "foo") = 2(the key position)- Correct value position = 7
Fix: search for the value only after the : separator:
colonIdx := strings.Index(parentLine, ":")
if colonIdx < 0 {
valueCol = colNum - 1
} else {
rel := strings.Index(parentLine[colonIdx:], parentValue)
if rel < 0 {
valueCol = colNum - 1
} else {
valueCol = colonIdx + rel
}
}The current test uses github: "invalid-string" where the value string "invalid-string" does not appear in the key, so the test does not catch this.
| func improveFrontmatterDiagnostic(content, line, col, message string) (string, string, string) { | ||
| lower := strings.ToLower(strings.TrimSpace(message)) | ||
| isScalarWithNestedKey := | ||
| (strings.Contains(lower, "value is not allowed in this context") && strings.Contains(lower, "map key-value is pre-defined")) || |
There was a problem hiding this comment.
improveFrontmatterDiagnostic depends on the translated message text, not the original parser text: after TranslateYAMLMessage runs, condition 1 of isScalarWithNestedKey is always false, and the function silently falls back to condition 2 — matching the translated string it itself produced.
💡 Why this is fragile
The call order is:
message = parser.TranslateYAMLMessage(message) // line 101
line, col, message = improveFrontmatterDiagnostic(...) // line 102At line 155-157, the check is:
isScalarWithNestedKey :=
(strings.Contains(lower, "value is not allowed in this context") && ...) || // always false after translation
strings.Contains(lower, "value cannot have child keys here") // matches translated textBy the time improveFrontmatterDiagnostic runs, the original parser phrase is already gone. The whole function depends on TranslateYAMLMessage having produced exactly "value cannot have child keys here". If that translation entry is ever reworded or removed, this matching silently stops working, and the diagnostic quality regression is undetected because the test only checks for the final improved message.
Fix options:
- Move
improveFrontmatterDiagnosticto run on the raw parser message (before translation), and pass the untranslated message as a separate parameter. - Or: check for the raw parser phrase only (condition 1 alone) before translation runs.
This also means condition 1 of the || is dead code.
| } | ||
|
|
||
| lineNum, colNum := parsePositiveInt(line), parsePositiveInt(col) | ||
| if lineNum <= 1 || colNum <= 0 { |
There was a problem hiding this comment.
lineNum <= 1 guard silently swallows errors on line 2: the parent of a child at line 2 would be line 1, which is valid content, but this guard returns the original diagnostic unchanged.
💡 Details
lineNum, colNum := parsePositiveInt(line), parsePositiveInt(col)
if lineNum <= 1 || colNum <= 0 {
return line, col, message
}The condition lineNum <= 1 bails out when lineNum == 1 (correct — no parent possible) but also when lineNum == 2, which has a valid parent at lines[0]. In practice the frontmatter starts with --- so lines[0] is never a YAML key, but this is accidental protection. The guard is overly strict and would hide a real scalar-at-line-2 error in a future format. The correct guard for "no parent possible" is lineNum < 2.
// Fix:
if lineNum < 2 || colNum <= 0 {
return line, col, message
}
|
@copilot revert action pins changes |
…quality-improve-diagnostics Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — reverted the |
|
/souschef |
|
✅ PR Sous Chef finished PR sous-chef nudges. processed=5; skipped_checks_running=1; skipped_last_comment_from_sous_chef=1; skipped_cooldown=0; nudged=2; branch_update_attempts=0; formatter_pushes=0; merge_main_scheduled=0 |
|
🍳 Acknowledged — sous-chef was invoked and will triage this PR.
|
|
@copilot run pr-finisher skill |
- Remove dead-code detection branch in improveFrontmatterDiagnostic: only the translated phrase can appear since TranslateYAMLMessage runs first - Change "not a string" message to "must be a mapping (object), not a scalar value" to be accurate for all scalar types (bool/int/null/string) - Fix strings.Index key/value collision by searching for the value after the colon rather than scanning the full line - Fix caret off-by-one in renderSourceContextForPosition (8-char prefix needs 7+targetCol spaces, not 7+targetCol-1) - Update classifyErrorMessage pattern to match new message wording - Update compiler_yaml_test.go expectedMessagePart to match new wording - Add regression test for tool config mapping classification Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Syntax/configuration diagnostics were low quality for malformed tool config: the compiler pointed at the wrong line, surfaced parser-internal wording, and sometimes attached non-syntax recovery hints. This change makes the error location, message, and hint category align with the actual authoring mistake.
Diagnostic targeting for scalar-with-children YAML mistakes
tools.<name>key is set to a scalar but followed by nested keys.github: "invalid-string") instead of the child key line.Human-readable message for known tool-shape violation
tools.github tool config must be an object, not a string (for example: toolsets: [default])Context-specific recovery classification
Focused regression coverage
tools.githubscalar + nested key caret/message behavior,