Skip to content

feat: add file/line context to validation errors#33466

Open
Copilot wants to merge 4 commits into
mainfrom
copilot/add-file-line-context-validation-errors
Open

feat: add file/line context to validation errors#33466
Copilot wants to merge 4 commits into
mainfrom
copilot/add-file-line-context-validation-errors

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 20, 2026

Validation errors lacked source location context, forcing users to manually search workflow files for the problematic field. This PR adds file:line:col: positioning to validation errors so IDE tooling can jump directly to the offending line.

Infrastructure

  • FieldLocation struct (File, Line, Column) added to pkg/workflow/workflow_errors.go
  • WorkflowValidationError extended with File, Line, Column fields
  • NewValidationErrorWithLocation constructor for errors with known source positions
  • Error() format: omits timestamp when Line > 0 — cleaner when the compiler already prefixes file:line:col:

Error formatting

formatCompilerError now promotes location from *WorkflowValidationError instead of always defaulting to 1:1:

// Before: always emitted workflow.md:1:1: error: ...
// After: emits workflow.md:15:1: error: ... when Line is set

err := NewValidationErrorWithLocation("engine", "copiliot",
    "not a valid engine", "Did you mean 'copilot'?",
    FieldLocation{File: "workflow.md", Line: 15})
return formatCompilerError(markdownPath, "error", err.Error(), err)
// → workflow.md:15:1: error: Validation failed for field 'engine'
//                             Value: copiliot
//                             Reason: not a valid engine
//                             Suggestion: Did you mean 'copilot'?

Line tracking in the parser

  • FrontmatterResult.FieldLines map[string]int — absolute 1-based line numbers for every top-level YAML key, populated by new extractTopLevelFieldLines (text scan, no AST overhead)
  • WorkflowData.FrontmatterFieldLines — surfaced to all validation functions so they can look up line numbers when creating errors

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/graphql
    • Triggering command: /usr/bin/gh gh repo view --json owner,name --jq .owner.login + "/" + .name 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh repo view --json owner,name --jq .owner.login + "/" + .name At,event,headBranch,headSha,displayTitle GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh pr list --repo github/gh-aw --state all --author app/github-actions --search created:2026-05-11T23:58:00Z..2026-05-12T00:05:00Z --limit 1 --json number --jq .[0].number GOMOD GOMODCACHE go (http block)
  • https://api.github.com/orgs/owner/actions/secrets
    • Triggering command: /usr/bin/gh gh api /orgs/owner/actions/secrets --jq .secrets[].name go1.25.8 -c=4 -nolocalimports -importcfg /tmp/go-build2643043042/b525/importcfg -pack /tmp/go-build2643043042/b525/_testmain.go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/orgs/test-owner/actions/secrets
    • Triggering command: /usr/bin/gh gh api /orgs/test-owner/actions/secrets --jq .secrets[].name -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/actions/ai-inference/git/ref/tags/v1
    • Triggering command: /usr/bin/gh gh api /repos/actions/ai-inference/git/ref/tags/v1 --jq [.object.sha, .object.type] | @tsv ithub/workflows/agent-performance-analyzer.md -buildtags 3043042/b555=> -errorsas b/gh-aw/pkg/semv-1 -nilfunc git comm�� ithub-script/git/ref/tags/v9 initial commit bject.type] | @tsv -json GO111MODULE 64/bin/go 3043042/b555/importcfg (http block)
  • https://api.github.com/repos/actions/checkout/git/ref/tags/v3
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v3 --jq [.object.sha, .object.type] | @tsv architecture-guardian.md o 1/x64/bin/node GOINSECURE GOMOD GOMODCACHE 3043042/b394/importcfg t-ha�� vaScript3106012653/001/test-frontmatter-with-nes-errorsas GO111MODULE 64/pkg/tool/linux_amd64/link GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/link (http block)
  • https://api.github.com/repos/actions/checkout/git/ref/tags/v5
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq [.object.sha, .object.type] | @tsv 495/001/stability-test.md GO111MODULE ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/x--jq env 5619-33208/test-3223998073/.github/workflows GO111MODULE 1/x64/bin/node GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/x64/pkg/tool/linuremote.origin.url (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq [.object.sha, .object.type] | @tsv 9DSwdag_RvDB2FtdE3Xz/9DSwdag_RvDB2FtdE3Xz 3043042/b541/_testmain.go /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/compile go1.25.8 -c=4 -nolocalimports /opt/hostedtoolcache/go/1.25.8/x--jq -V=f�� -aw/git/ref/tags/v3.0.0 /tmp/go-build2643043042/b498/_testmain.go ache/node/24.14.1/x64/bin/node -json GO111MODULE 64/bin/go ache/node/24.14.1/x64/bin/node (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq [.object.sha, .object.type] | @tsv -stringintconv -tests /opt/hostedtoolcache/node/24.14.1/x64/bin/node -json GO111MODULE x_amd64/vet node /tmp�� /home/REDACTED/work/gh-aw/gh-aw/.github/workflows/api-consumption-report.md resolved$ (http block)
  • https://api.github.com/repos/actions/checkout/git/ref/tags/v6
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v6 --jq [.object.sha, .object.type] | @tsv -json piler}} /opt/hostedtoolcache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env ExpressionCompiledOutput3034362390/001 (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v6 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v6 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE /opt/hostedtoolcache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env 528171551/.github/workflows GO111MODULE .cfg GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/actions/github-script/git/ref/tags/v8
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq [.object.sha, .object.type] | @tsv sistency_KeyOrdering2540445059/001/test1.md {{context.GOARCH}} {{context.Compiler}} /usr/bin/gh unsafe GO111MODULE 64/bin/go gh api ithub-script/git/ref/tags/v9 --jq bject.type] | @tsv -json GO111MODULE 64/bin/go git (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq [.object.sha, .object.type] | @tsv k/gh-aw/gh-aw/.github/workflows/agent-performance-analyzer.md -trimpath /usr/bin/gh -p github.com/githurev-parse -lang=go1.25 gh api /repos/actions/github-script/git/ref/tags/v9 --jq /usr/bin/git -c=4 -nolocalimports -importcfg git (http block)
  • https://api.github.com/repos/actions/github-script/git/ref/tags/v9
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9 --jq [.object.sha, .object.type] | @tsv /tmp/go-build243GOINSECURE -trimpath 64/bin/go -d main -lang=go1.25 go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9 --jq [.object.sha, .object.type] | @tsv /tmp/go-build243GOINSECURE -trimpath 64/bin/go -p golang.org/x/tooenv -lang=go1.25 go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9 --jq [.object.sha, .object.type] | @tsv /tmp/go-build243GOINSECURE -trimpath 64/bin/go -p main -lang=go1.25 go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/actions/github-script/git/ref/tags/v9.0.0
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9.0.0 --jq [.object.sha, .object.type] | @tsv /tmp/go-build243GOINSECURE -trimpath 64/bin/go -p golang.org/x/tooenv -lang=go1.25 go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9.0.0 --jq [.object.sha, .object.type] | @tsv /tmp/go-build243GOINSECURE -trimpath 64/bin/go -p main -lang=go1.25 go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9.0.0 --jq [.object.sha, .object.type] | @tsv /tmp/go-build243GOINSECURE -trimpath 64/bin/go -p github.com/githuenv -lang=go1.25 go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/actions/setup-go/git/ref/tags/v4
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-go/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv -bool -buildtags /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/compile -errorsas -ifaceassert -nilfunc /opt/hostedtoolcache/go/1.25.8/x--jq -o /tmp/go-build2643043042/b497/_pkg_.a url .cfg -p github.com/githu-1 -lang=go1.25 3043042/b562/stats.test (http block)
  • https://api.github.com/repos/actions/setup-node/git/ref/tags/v4
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE 64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/compile env g_.a GO111MODULE r: $owner, name: $name) { hasDiscussionsEnabled } } s eutil GOMODCACHE cutil.test (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv 4dh0uLCKc -buildtags 1/x64/bin/node -errorsas -ifaceassert -nilfunc ortcfg t-ha�� ithub/workflows/archie.md g/stringutil/identifiers.go 3043042/b538/_pkg_.a -errorsas -ifaceassert -nilfunc /usr/lib/git-core/git (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv git-upload-pack '/tmp/TestParseDefaultBranchFromLsRemoteWithReal-errorsas l /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/link -json GO111MODULE 64/bin/go /opt/hostedtoolc--package-lock-only -V=f�� GOMODCACHE go .0/x64/bin/go -json GO111MODULE 64/bin/go git (http block)
  • https://api.github.com/repos/actions/setup-node/git/ref/tags/v6
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v6 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE /opt/hostedtoolcache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env ll-sweep (enforce_all)' GO111MODULE e/git l GOMOD GOMODCACHE e/git (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v6 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE 3043042/b498/fileutil.test GOINSECURE GOMOD GOMODCACHE 3043042/b498/filowner/test-repo e=/t�� t0 GO111MODULE (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v6 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/xowner/test-repo env 528171551/.github/workflows GO111MODULE 3043042/b432/vet.cfg GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/actions/upload-artifact/git/ref/tags/v4
    • Triggering command: /usr/bin/gh gh api /repos/actions/upload-artifact/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE 3043042/b505/jsonutil.test GOINSECURE GOMOD GOMODCACHE 3043042/b505/jsoowner/repo e=/t�� /ref/tags/v9.0.0 GO111MODULE sv m0s GOMOD (http block)
  • https://api.github.com/repos/aws-actions/configure-aws-credentials/git/ref/tags/v4
    • Triggering command: /usr/bin/gh gh api /repos/aws-actions/configure-aws-credentials/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv run url /usr/bin/git --detach GO111MODULE 64/bin/go git -C /tmp/gh-aw-test-runs/20260520-055619-33208/test-3640699418/.github/workflows l ache/node/24.14.1/x64/bin/node remote.upstream.grep GO111MODULE 64/bin/go ache/node/24.14./tmp/gh-aw/aw-master.patch (http block)
    • Triggering command: /usr/bin/gh gh api /repos/aws-actions/configure-aws-credentials/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv ithub-script/git/ref/tags/v9 -importcfg bject.type] | @tsv -s -w -buildmode=exe git -C image:v1.0.0 config ache/node/24.14.1/x64/bin/node remote.origin.urgit GO111MODULE 64/bin/go ache/node/24.14.1/x64/bin/node (http block)
  • https://api.github.com/repos/azure/login/git/ref/tags/v2
    • Triggering command: /usr/bin/gh gh api /repos/azure/login/git/ref/tags/v2 --jq [.object.sha, .object.type] | @tsv 3043042/b571/_pkg_.a git-upload-pack '/tmp/TestParseDefaultBranchFromLsRemoteWithRealGitbranch_with_hunsafe ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet -json GO111MODULE 64/bin/go ache/go/1.25.8/x64/pkg/tool/linu--jq -C AGoV/wVIjl6JE2QkluJttAGoV config e/git remote.origin.ur/usr/lib/git-core/git GO111MODULE 64/bin/go e/git (http block)
  • https://api.github.com/repos/docker/login-action/git/ref/tags/v3
    • Triggering command: /usr/bin/gh gh api /repos/docker/login-action/git/ref/tags/v3 --jq [.object.sha, .object.type] | @tsv ithub-script/git/ref/tags/v9.0.0 --auto bject.type] | @tsv --detach GO111MODULE 64/bin/go /tmp/go-build2643043042/b545/log--jq -tes�� -test.paniconexit0 s/12346/artifacts /usr/bin/git -test.timeout=10git -test.run=^Test -test.short=true. git (http block)
  • https://api.github.com/repos/github/gh-aw-actions/git/ref/tags/v0.1.2
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw-actions/git/ref/tags/v0.1.2 --jq [.object.sha, .object.type] | @tsv t0 -buildtags r: $owner, name: $name) { hasDiscussionsEnabled } } m0s -ifaceassert (http block)
  • https://api.github.com/repos/github/gh-aw-actions/git/ref/tags/v1.0.0
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw-actions/git/ref/tags/v1.0.0 --jq [.object.sha, .object.type] | @tsv 5619-33208/test-3640699418/.gith-c=4 GO111MODULE ache/go/1.25.8/x64/pkg/tool/linu-importcfg GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/x--jq env OKEN }} GO111MODULE sv GOINSECURE GOMOD GOMODCACHE (http block)
  • https://api.github.com/repos/github/gh-aw-actions/git/ref/tags/v1.2.3
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw-actions/git/ref/tags/v1.2.3 --jq [.object.sha, .object.type] | @tsv 5619-33208/test-1692733353/.github/workflows 3043042/b455/vet.cfg /opt/hostedtoolcache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env st-1589422392 GO111MODULE r: $owner, name: $name) { hasDiscussionsEnabled } } GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs
    • Triggering command: /usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --limit 100 --created >=2026-05-13 GOMOD GOMODCACHE x_amd64/compile env -json GO111MODULE 64/pkg/tool/linux_amd64/link GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/link (http block)
    • Triggering command: /usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --limit 100 --created >=2026-04-20 GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/vet env -json GO111MODULE 64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/compile (http block)
    • Triggering command: /usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --limit 100 --created >=2026-02-19 GOMOD GOMODCACHE 64/pkg/tool/linuorigin env -json oFiles,IgnoredOt-nolocalimports 64/pkg/tool/linu-importcfg GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linu/home/REDACTED/work/gh-aw/gh-aw/pkg/syncutil/onceloader_test.go (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/1/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/1/artifacts --jq .artifacts[].name .cfg ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env y_with_explicit_@{u} GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh run download 1 --dir test-logs/run-1 test.go ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env edOutput544737685/001 GO111MODULE 64/pkg/tool/linux_amd64/asm GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/asm (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/12345/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/12345/artifacts --jq .artifacts[].name GO111MODULE 64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE pBvTgXO/DnQKkYlYtest@example.com env 2362163896/.github/workflows GO111MODULE 64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linu^remote\..*\.gh-resolved$ (http block)
    • Triggering command: /usr/bin/gh gh run download 12345 --dir test-logs/run-12345 GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env report.md GO111MODULE .cfg GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/1234567890
    • Triggering command: /usr/bin/gh gh api repos/{owner}/{repo}/actions/runs/1234567890 --jq {databaseId: .id, number: .run_number, url: .html_url, status: .status, conclusion: .conclusion, workflowName: .name, workflowPath: .path, createdAt: .created_at, startedAt: .run_started_at, updatedAt: .updated_at, event: .event, headBranch: .head_branch, -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/12346/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/12346/artifacts --jq .artifacts[].name GO111MODULE x_amd64/compile GOINSECURE GOMOD GOMODCACHE x_amd64/compile env e-analyzer.md GO111MODULE .cfg GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh run download 12346 --dir test-logs/run-12346 GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE 5v/xGCZYgHhEK_tLconfig GOMODCACHE go env or.md GO111MODULE .cfg GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/2/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/2/artifacts --jq .artifacts[].name .cfg ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env y.md GO111MODULE k GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh run download 2 --dir test-logs/run-2 GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env 29522563/001 GO111MODULE ck GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/3/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/3/artifacts --jq .artifacts[].name GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env rdian.md GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE ortcfg (http block)
    • Triggering command: /usr/bin/gh gh run download 3 --dir test-logs/run-3 GO111MODULE 64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linuorigin env edOutput544737685/001 GO111MODULE .cfg GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/4/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/4/artifacts --jq .artifacts[].name oFiles,IgnoredOtherFiles,CFiles,CgoFiles,CXXFiles,MFiles,HFiles,FFiles,SFiles,SwigFiles,SwigCXXFremote 64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE sY5xy3c/9ezsDU_Vstatus env rdian.md GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE til GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh run download 4 --dir test-logs/run-4 GO111MODULE 64/pkg/tool/linux_amd64/link GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linuremote.origin.url epOn�� mpiledOutput4076594964/001 GO111MODULE ortcfg.link GOINSECURE GOMOD GOMODCACHE LBy97bxPkfrkiwhLXh/3XLIOuLPxEGBR-trimpath (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/5/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/5/artifacts --jq .artifacts[].name l_test.go ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env y.md GO111MODULE 64/pkg/tool/linux_amd64/link GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linuTest User (http block)
    • Triggering command: /usr/bin/gh gh run download 5 --dir test-logs/run-5 .cfg ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env mpiledOutput184441634/001 GO111MODULE 64/pkg/tool/linux_amd64/asm GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linutest@example.com (http block)
  • https://api.github.com/repos/github/gh-aw/actions/workflows
    • Triggering command: /usr/bin/gh gh workflow list --json name,state,path -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-workflow-12345 --limit 100 GOMOD GOMODCACHE x_amd64/vet env -json herFiles,CFiles,CgoFiles,CXXFiles,MFiles,HFiles,FFiles,SFiles,SwigFiles,SwigCXXFiles,SysoFiles,Tconfig 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-workflow-12345 --limit 6 setup/js/node_moconfig GOMODCACHE 64/pkg/tool/linutest@example.com env 3623416137/.github/workflows GO111MODULE x_amd64/link GOINSECURE GOMOD GOMODCACHE x_amd64/link (http block)
  • https://api.github.com/repos/github/gh-aw/contents/.github/workflows/shared/reporting.md
    • Triggering command: /tmp/go-build2643043042/b478/cli.test /tmp/go-build2643043042/b478/cli.test -test.testlogfile=/tmp/go-build2643043042/b478/testlog.txt -test.paniconexit0 -test.v=true -test.parallel=4 -test.timeout=10m0s -test.run=^Test -test.short=true -p github.com/githuenv -lang=go1.25 go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/github/gh-aw/git/ref/tags/dev
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/dev --jq [.object.sha, .object.type] | @tsv /ref/tags/v9.0.0 --jq sv -json GO111MODULE 64/bin/go git -C /tmp/TestGuardPolicyTrustedUsersRequiresMinIntegrity3285162960/001 config /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet remote.origin.urinfocmp git 64/bin/go /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet (http block)
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/dev --jq [.object.sha, .object.type] | @tsv 3043042/b593/_pkg_.a go 3043042/b593=> -json GO111MODULE 64/bin/go git conf�� poJ_/sX9FX53sm1OTZ6jdpoJ_ Test User /usr/bin/git -json GO111MODULE 64/bin/go git (http block)
  • https://api.github.com/repos/github/gh-aw/git/ref/tags/v0.47.4
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v0.47.4 --jq [.object.sha, .object.type] | @tsv t0 /dev/null .test m0s -nolocalimports (http block)
  • https://api.github.com/repos/github/gh-aw/git/ref/tags/v1.0.0
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v1.0.0 --jq [.object.sha, .object.type] | @tsv t1848615406/.github/workflows GO111MODULE 64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/vet env 495/001/stability-test.md GO111MODULE 64/pkg/tool/linux_amd64/link GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/link (http block)
  • https://api.github.com/repos/github/gh-aw/git/ref/tags/v1.2.3
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v1.2.3 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/github/gh-aw/git/ref/tags/v2.0.0
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v2.0.0 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env -json GO111MODULE x_amd64/compile GOINSECURE GOMOD GOMODCACHE x_amd64/compile (http block)
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v2.0.0 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v2.0.0 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env ub/workflows GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/repos/github/gh-aw/git/ref/tags/v3.0.0
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v3.0.0 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/github/gh-aw/issues/17
    • Triggering command: /usr/bin/gh gh api repos/github/gh-aw/issues/17 go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/google-github-actions/auth/git/ref/tags/v2
    • Triggering command: /usr/bin/gh gh api /repos/google-github-actions/auth/git/ref/tags/v2 --jq [.object.sha, .object.type] | @tsv runs/20260520-055619-33208/aw-manifest-legacy-747511632/.github/workflows --revs /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet l --delta-base-off-1 -q /opt/hostedtoolcache/go/1.25.8/x--jq -ato�� -bool (http block)
  • https://api.github.com/repos/nonexistent/action/git/ref/tags/v999.999.999
    • Triggering command: /usr/bin/gh gh api /repos/nonexistent/action/git/ref/tags/v999.999.999 --jq [.object.sha, .object.type] | @tsv t1848615406/.github/workflows GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE ache/go/1.25.8/x64/pkg/tool/linu-buildmode=exe GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/xorigin (http block)
  • https://api.github.com/repos/nonexistent/repo/actions/runs/12345
    • Triggering command: /usr/bin/gh gh run view 12345 --repo nonexistent/repo --json status,conclusion GOINSECURE GOMOD GOMODCACHE ters.test 6430�� dgWJmpNjg GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/repos/org/repo/pulls/1
    • Triggering command: /usr/bin/gh gh api repos/org/repo/pulls/1 go env 01 02/work 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/owner/repo/actions/secrets
    • Triggering command: /usr/bin/gh gh api /repos/owner/repo/actions/secrets --jq .secrets[].name go1.25.8 -c=4 -nolocalimports -importcfg /tmp/go-build2643043042/b522/importcfg -pack /tmp/go-build2643043042/b522/_testmain.go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/owner/repo/actions/workflows
    • Triggering command: /usr/bin/gh gh workflow list --json name,state,path --repo owner/repo 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-remote-workflow --limit 30 --repo owner/repo yLNKNaz/um86pbnuaZfPHtRi8CIm 6430�� -json GO111MODULE 64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linuremote2 (http block)
    • Triggering command: /usr/bin/gh gh workflow list --repo owner/repo --json name,path,state 64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linuremote.origin.url env 53/001/test-frontmatter-with-arrays.md GO111MODULE 64/pkg/tool/linux_amd64/link GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/link (http block)
  • https://api.github.com/repos/test-owner/test-repo/actions/secrets
    • Triggering command: /usr/bin/gh gh api /repos/test-owner/test-repo/actions/secrets --jq .secrets[].name -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/test/repo
    • Triggering command: /usr/bin/gh gh api /repos/test/repo --jq .default_branch -json GO111MODULE ck GOINSECURE GOMOD GOMODCACHE go sRem�� nifest-legacy-747511632/.github/workflows GO111MODULE .cfg GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/x--json (http block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI linked an issue May 20, 2026 that may be closed by this pull request
9 tasks
Copilot AI and others added 2 commits May 20, 2026 05:57
- Add FieldLocation struct with File, Line, Column fields
- Add File, Line, Column fields to WorkflowValidationError
- Add NewValidationErrorWithLocation constructor
- Update WorkflowValidationError.Error() to omit timestamp when Line>0
  (cleaner output when compiler adds file:line:col: prefix)
- Update formatCompilerError to use location from WorkflowValidationError
  when Line>0, enabling precise IDE-navigable file:line:col: positions
- Add FieldLines map[string]int to FrontmatterResult to track absolute
  line numbers of top-level frontmatter keys
- Add extractTopLevelFieldLines helper that scans YAML text for key positions
- Add FrontmatterFieldLines to WorkflowData for use by validation functions
- Populate FrontmatterFieldLines in buildInitialWorkflowData
- Add tests for all new functionality

Agent-Logs-Url: https://github.com/github/gh-aw/sessions/16e4519d-23d2-443f-bb46-fbbdbe64519e

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…hars

Add a comment explaining why brackets and quotes are excluded from YAML key
names in extractTopLevelFieldLines. These characters indicate non-standard
key forms (inline maps, quoted keys) not used in workflow frontmatter.
Also exclude quote characters to be safe.

Agent-Logs-Url: https://github.com/github/gh-aw/sessions/16e4519d-23d2-443f-bb46-fbbdbe64519e

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Copilot AI changed the title [WIP] Add file and line context to validation errors feat: add file/line context to validation errors May 20, 2026
Copilot AI requested a review from gh-aw-bot May 20, 2026 06:00
@pelikhan pelikhan marked this pull request as ready for review May 20, 2026 06:13
Copilot AI review requested due to automatic review settings May 20, 2026 06:13
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds infrastructure to attach source file/line/column context to workflow validation errors so compiler-style diagnostics can point IDEs at the offending frontmatter field.

Changes:

  • Introduces location fields (File, Line, Column) for WorkflowValidationError plus a NewValidationErrorWithLocation constructor, and updates Error() formatting to omit timestamps when a line is present.
  • Updates formatCompilerError to promote file/line/col from a *WorkflowValidationError cause instead of always defaulting to 1:1.
  • Extends frontmatter parsing to compute absolute line numbers for top-level YAML keys and plumbs them into WorkflowData, with new parser tests.
Show a summary per file
File Description
pkg/workflow/workflow_errors.go Adds location support to validation errors and a new constructor; adjusts error string formatting.
pkg/workflow/compiler_error_formatter.go Promotes file:line:col from *WorkflowValidationError when formatting compiler errors.
pkg/parser/frontmatter_content.go Adds FieldLines to frontmatter parse results and implements top-level key line scanning.
pkg/parser/frontmatter_field_lines_test.go Adds unit tests for extracting top-level frontmatter key line numbers.
pkg/workflow/compiler_types.go Adds FrontmatterFieldLines to WorkflowData to carry parser-derived line info.
pkg/workflow/workflow_builder.go Plumbs FieldLines from the parser into WorkflowData.
pkg/workflow/error_helpers_test.go Adds tests for the new location-aware validation error constructor and formatting.
pkg/workflow/compiler_error_formatting_test.go Adds tests to ensure compiler formatting uses location from validation errors.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 8/8 changed files
  • Comments generated: 4

// filePath: the file path to include in the error (typically markdownPath or lockFile)
// errType: the error type ("error" or "warning")
// message: the error message text
// cause: optional underlying error to wrap (use nil for validation errors)
Comment on lines 449 to 454
FrontmatterName string // name field from frontmatter (for code scanning alert driver default)
FrontmatterEmoji string // emoji field from frontmatter (for display in footers and UI)
FrontmatterYAML string // raw frontmatter YAML content (rendered as comment in lock file for reference)
FrontmatterHash string // SHA-256 hash of frontmatter (computed before job building, used to derive stable heredoc delimiters)
FrontmatterFieldLines map[string]int // absolute 1-based line numbers of top-level frontmatter keys in the source file (populated by parser)
RawMarkdown string // raw markdown body before include expansion, used for frontmatter hash computation without re-reading the file
assert.Contains(t, msg, "Reason: not a valid engine")
assert.Contains(t, msg, "Suggestion: Did you mean 'copilot'?")
// No timestamp prefix when location is set
assert.NotContains(t, msg, "[20")
Comment on lines +252 to +256
assert.Contains(t, errStr, "workflow.md", "Should contain file path")
assert.Contains(t, errStr, "15", "Should use the line number from WorkflowValidationError")
assert.Contains(t, errStr, "3", "Should use the column number from WorkflowValidationError")
// Should NOT default to 1:1 when location is known
assert.NotContains(t, errStr, "1:1", "Should not fall back to 1:1 when location is set")
@github-actions
Copy link
Copy Markdown
Contributor

Hey @app/copilot-swe-agent 👋 — thanks for working on validation error improvements! The technical implementation looks solid (focused changes, tests included, detailed description), but there's a process concern:

  • Missing issue discussion — According to CONTRIBUTING.md, this project uses an issue-first workflow where non-core contributors should create detailed agentic plans in issues for discussion before a PR is created. This PR appears to have been created directly without a linked issue.
  • Verify core team status — If you're part of the core team, this can be disregarded. Otherwise, the documented process asks that you open an issue first to discuss the approach with maintainers.

The technical quality is excellent — the feature adds valuable IDE-friendly error positioning, includes comprehensive tests, and maintains focus. If this PR was created as part of an approved workflow or by a core team member, please link the related issue or confirm the authorization.

If you're a community contributor and this was created before discussion, consider:

Create a GitHub issue for gh-aw describing the validation error context enhancement.

Title: "Add file:line:col positioning to validation errors for IDE integration"

Body should include:
1. **Problem**: Validation errors currently default to line 1:1, forcing manual search
2. **Proposed solution**: Track YAML field line numbers during parsing and surface them in error messages
3. **Benefits**: IDE tooling can jump directly to error locations
4. **Implementation approach**: 
   - Add FieldLocation struct with File/Line/Column
   - Extend WorkflowValidationError with location fields
   - Add FrontmatterResult.FieldLines map for line tracking
   - Update formatCompilerError to promote real locations
5. **Testing**: Unit tests for line extraction and error formatting

Label the issue with appropriate tags (enhancement, DX improvement, etc.)

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • patchdiff.githubusercontent.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "patchdiff.githubusercontent.com"

See Network Configuration for more information.

Generated by ✅ Contribution Check ·

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

PR Code Quality Reviewer completed the code quality review.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

🧪 Test Quality Sentinel completed test quality analysis.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This PR adds IDE-navigable source locations to validation errors — a valuable UX improvement that will help users quickly jump to problematic fields in their workflow files.

✅ Strengths:

  • Comprehensive test coverage with well-chosen edge cases
  • Thoughtful UX decisions (e.g., omitting timestamps when location is known)
  • Clean error wrapping using errors.As to extract validation error details
  • Good documentation of function behavior

💡 Minor Suggestions:
I've left two non-blocking comments about code clarity:

  1. The line counting increment pattern could be slightly more obvious
  2. The filePath override behavior should be documented in the function comment

Both suggestions are about maintainability and clarity for future readers rather than correctness issues.

Overall: The implementation is solid and ready to merge after addressing the minor documentation gap if desired.

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.

Note

🔒 Integrity filter blocked 1 item

The following item was blocked because it doesn't meet the GitHub integrity level.

  • #33466 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

🔎 Code quality review by PR Code Quality Reviewer · ● 5.9M

// directly as file:line positions for IDE-navigable error messages.
func extractTopLevelFieldLines(yamlContent string, frontmatterStart int) map[string]int {
fieldLines := make(map[string]int)
relLine := 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Line counting pattern could be clearer

The increment-first pattern works correctly but is subtle:

relLine := 0
for line := range strings.SplitSeq(yamlContent, "\n") {
    relLine++  // First iteration: relLine becomes 1 ✓

Consider starting at 1 and incrementing at the end for clarity:

relLine := 1
for line := range strings.SplitSeq(yamlContent, "\n") {
    // ... process line ...
    relLine++
}

This makes the 1-based indexing more obvious to future readers. The current code is correct, just less immediately clear.

}
if vErr.File != "" {
filePath = vErr.File
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation gap: filePath parameter can be overridden

This code silently overrides the filePath parameter when the validation error has its own file:

if vErr.File != "" {
    filePath = vErr.File
}

While the tests confirm this is intentional behavior ("uses file from validation error when different from filePath"), the function documentation doesn't mention this override.

Suggestion: Update the function comment to note that when cause is a *WorkflowValidationError with a non-empty File field, that file path takes precedence over the filePath parameter. This prevents confusion for future callers.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /tdd (test coverage analysis) and /zoom-out (architectural fit) based on this feature addition.


Key Themes

1. Fragile YAML parsing approach
The string-based line extraction logic in extractTopLevelFieldLines is brittle and doesn't leverage the YAML parser's built-in position tracking. The goccy/go-yaml library already provides AST nodes with line/column metadata — using that would be more robust and handle all YAML edge cases correctly.

2. Missing integration test
While unit tests cover individual components well, there's no end-to-end test proving that a validation error in a real workflow markdown file produces an IDE-navigable error message with file:line:col: prefix. This is the critical user-facing behavior.

3. Top-level fields only
The current implementation only tracks top-level frontmatter keys (e.g., engine, tools). Nested field validation errors (e.g., tools.github.toolsets) won't get location data, which limits the feature's usefulness since most validation errors are in nested config.

4. Test coverage gaps
The YAML parsing tests don't cover quoted keys, multiline block scalars, anchors/aliases, or flow collections — all valid YAML syntax that could appear in workflow frontmatter.


Positive Highlights

Clean architectural separation — Parser extracts locations, error types hold them, compiler formatter uses them. Good layering.

Backward compatible — The existing NewValidationError() constructor still works; new code can opt into locations.

Well-structured tests — Table-driven test approach in frontmatter_field_lines_test.go is exemplary.

Good documentation — Function comments clearly explain the line number calculations and edge cases.


Verdict

Requesting changes primarily on:

  1. YAML parsing robustness — Consider AST-based approach or add comprehensive edge case tests
  2. Integration testing — Add at least one end-to-end test of the error flow
  3. Documentation — Document the top-level-only limitation in FieldLocation struct

The core idea is solid and the implementation is clean, but the YAML parsing brittleness and missing integration test are blockers for production reliability.


Matt Pocock Skills Reference:

  • /tdd skill — Red-green-refactor, test-first development
  • /zoom-out skill — Architectural context and broader perspective

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 5.2M

FrontmatterStart int // Line number where frontmatter starts (1-based)
FieldLines map[string]int // Absolute line numbers (1-based) of top-level frontmatter keys in the file
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The YAML parsing logic in extractTopLevelFieldLines is fragile and lacks edge case coverage. Consider testing:

  • Quoted keys: "engine": copilot or 'engine': copilot
  • Multiline values: Keys with | or > block scalars
  • Anchors/aliases: &anchor and *alias syntax
  • Flow collections: tools: {github: {toolsets: default}}

Without these tests, the parser may silently fail on valid YAML, causing location data to be missing when users need it most.

{
    name: "quoted keys are skipped",
    yamlContent: `"engine": copilot`,
    frontmatterStart: 2,
    wantFieldLines: map[string]int{}, // Currently excluded by containsAny check
},
{
    name: "multiline block scalar",
    yamlContent: `description: |
  Line one
  Line two
on: push`,
    frontmatterStart: 2,
    wantFieldLines: map[string]int{"description": 2, "on": 5},
},

// number of the first frontmatter content line (i.e. the line immediately after the
// opening "---" delimiter). The returned line numbers are absolute: they can be used
// directly as file:line positions for IDE-navigable error messages.
func extractTopLevelFieldLines(yamlContent string, frontmatterStart int) map[string]int {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/zoom-out] This string-based YAML parsing approach is fragile compared to using the actual YAML parser's position tracking. The goccy/go-yaml library already provides line/column information in its AST.

Consider using yaml.NodeToValue() or yaml.ValueToNode() to access position metadata:

import "github.com/goccy/go-yaml"

func extractFieldLinesFromAST(yamlContent string) (map[string]int, error) {
    var node yaml.Node
    if err := yaml.Unmarshal([]byte(yamlContent), &node); err != nil {
        return nil, err
    }
    // Walk the AST and extract Line from each MappingNode key
    // This handles ALL YAML syntax correctly, not just simple cases
}

This architectural choice would make the feature more robust and maintainable.

// FieldLines is nil when there is no frontmatter
assert.Nil(t, result.FieldLines, "FieldLines should be nil when there is no frontmatter")
})
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Missing integration test for the full error flow. These unit tests verify extractTopLevelFieldLines works in isolation, but there's no test proving:

  1. A validation error in a workflow triggers with the correct line number
  2. The formatted compiler error includes file:line:col: prefix
  3. The error message is IDE-navigable

Consider adding an integration test:

func TestValidationErrorWithLocation_Integration(t *testing.T) {
    markdownWithError := `---
engine: copiliot
tools:
  github:
    toolsets: default
---
# Test
`
    compiler := NewCompiler()
    err := compiler.Compile("test.md", markdownWithError)
    require.Error(t, err)
    
    // Should contain "test.md:2:" prefix (engine is on line 2)
    assert.Contains(t, err.Error(), "test.md:2:")
    assert.Contains(t, err.Error(), "copiliot")
}


// FieldLocation represents a source file location for a validation error.
// File and Line are optional; zero values mean "location unknown".
type FieldLocation struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/zoom-out] The FieldLocation infrastructure only supports top-level frontmatter keys. Nested field validation errors (e.g., tools.github.toolsets) won't get location data.

This limitation means the most common validation errors (nested configuration) won't benefit from IDE navigation. Consider documenting this constraint in the struct comment:

// FieldLocation represents a source file location for a validation error.
// File and Line are optional; zero values mean "location unknown".
// 
// Current limitation: Only top-level frontmatter keys are tracked.
// Nested field errors (e.g., tools.github.toolsets) will have Line=0.
type FieldLocation struct {

Or, consider extending FieldLines to support dot-notation keys like "tools.github.toolsets": 4.

line, column := 1, 1
// Promote precise source location from WorkflowValidationError when available so that
// the emitted "file:line:col: error:" prefix points directly at the problematic field
// rather than always defaulting to line 1, column 1.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The location promotion logic silently overwrites filePath when vErr.File is set. This could be surprising if the caller passes a different file path.

Consider testing this edge case:

func TestFormatCompilerError_FilePathOverride(t *testing.T) {
    loc := FieldLocation{File: "actual.md", Line: 5}
    vErr := NewValidationErrorWithLocation("field", "val", "bad", "", loc)
    
    // Caller passes "other.md" but error has "actual.md"
    wrapped := formatCompilerError("other.md", "error", vErr.Error(), vErr)
    
    // Which file wins? Should be documented.
    assert.Contains(t, wrapped.Error(), "actual.md")
    assert.NotContains(t, wrapped.Error(), "other.md")
}

Also document the precedence in the function comment.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

PR Code Quality Reviewer completed the code quality review.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 4 new/modified tests verify behavioral contracts with comprehensive edge case coverage.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

🧪 Test Quality Sentinel completed test quality analysis.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds valuable IDE-navigable error location tracking to validation errors. The implementation is mostly solid, but I've identified a few areas for improvement:

Key Findings

Correctness & Robustness:

  • Loop variable capture issue in extractTopLevelFieldLines (consider using explicit indexing)
  • Line number calculation formula is correct but could be clearer
  • Potential file path override issue in error formatter when mixing relative/absolute paths

Maintainability:

  • Some edge case handling could be more explicit (though current code works correctly)
  • Test coverage could be expanded for malformed YAML edge cases
  • Comments could better match the actual implementation details

What's Done Well

✅ Clear separation of concerns with FieldLocation struct
✅ Comprehensive test coverage for happy paths
✅ Good use of zero values to indicate "location unknown"
✅ Thoughtful handling of duplicate keys (first occurrence wins)

Overall, this is a well-structured feature that will significantly improve developer experience. The issues I've flagged are refinements rather than blockers.

Note

🔒 Integrity filter blocked 1 item

The following item was blocked because it doesn't meet the GitHub integrity level.

  • #33466 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

🔎 Code quality review by PR Code Quality Reviewer · ● 5.4M

// directly as file:line positions for IDE-navigable error messages.
func extractTopLevelFieldLines(yamlContent string, frontmatterStart int) map[string]int {
fieldLines := make(map[string]int)
relLine := 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loop variable capture bug: The line variable from strings.SplitSeq is reused across iterations. If you later reference it outside the loop (e.g., in deferred functions or goroutines), this could cause bugs. While not a problem in this specific code, it's safer to use:

for line := range strings.SplitSeq(yamlContent, "\n") {
    lineCopy := line  // Create copy for safe capture
    relLine++
    // ... rest of logic using lineCopy
}

Or better yet, use the explicit index form to avoid any ambiguity:

lines := strings.Split(yamlContent, "\n")
for i, line := range lines {
    relLine := i + 1
    // ... rest of logic
}

This makes the line numbering explicit and avoids potential capture issues.

key := strings.TrimSpace(trimmed[:colonIdx])
// Accept simple unquoted keys only. Bracket characters in the key position
// ({, }, [, ]) indicate inline YAML maps/sequences rather than plain string keys
// (e.g. `[anchor]: value` or `{implicit_key}: value`). These forms are not used
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line numbering inconsistency: The formula relLine + frontmatterStart - 1 is confusing and error-prone. Let me trace through an example:

  • frontmatterStart = 2 (first content line after ---)
  • First iteration: relLine = 1 (first line in YAML)
  • Calculation: 1 + 2 - 1 = 2 ✅ Correct

But this is harder to understand than necessary. Consider this clearer approach:

// absoluteLine = (relLine - 1) + frontmatterStart
// Which simplifies to the same formula, but the comment makes the offset clear
absoluteLine := frontmatterStart + relLine - 1

Or even better, remove the magic -1 by starting relLine at 0:

relLine := 0
for line := range strings.SplitSeq(yamlContent, "\n") {
    // ... logic ...
    if key != "" && !strings.ContainsAny(key, " \t{}[]\"'") {
        if _, alreadySeen := fieldLines[key]; !alreadySeen {
            fieldLines[key] = frontmatterStart + relLine
        }
    }
    relLine++
}

This makes it immediately clear that we're adding the relative line offset to the frontmatter start position.

if trimmed == "" || strings.HasPrefix(trimmed, "#") {
continue
}
// Top-level keys have no leading indentation
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing edge case handling: The code checks for empty trimmed strings and comments, but what happens if a line contains ONLY a colon (:) or has the colon at position 0?

: value

The current logic would extract an empty key (trimmed[:0]), which would then fail the key != "" check. However, this could be made more explicit:

colonIdx := strings.Index(trimmed, ":")
if colonIdx > 0 {  // Changed from >= to > to skip colon-at-start
    key := strings.TrimSpace(trimmed[:colonIdx])
    if key != "" && !strings.ContainsAny(key, " \t{}[]\"'") {
        // ... rest of logic
    }
}

Actually, the existing colonIdx > 0 check already handles this correctly! But the nested key != "" check is redundant since colonIdx > 0 guarantees at least one character before the colon. You could simplify:

if colonIdx > 0 {
    key := strings.TrimSpace(trimmed[:colonIdx])
    // No need for key != "" check - colonIdx > 0 ensures non-empty substring
    if !strings.ContainsAny(key, " \t{}[]\"'") {
        // ... rest of logic
    }
}

// the emitted "file:line:col: error:" prefix points directly at the problematic field
// rather than always defaulting to line 1, column 1.
var vErr *WorkflowValidationError
if errors.As(cause, &vErr) && vErr.Line > 0 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential nil pointer dereference: While errors.As guarantees vErr is non-nil after a successful match, the code accesses vErr.Line, vErr.Column, and vErr.File without checking if these are valid values. This is actually safe because the struct fields are value types (int, string), but the logic has a subtle issue:

If vErr.Line > 0 but vErr.File == "", you'll use the filePath parameter. However, if vErr.File != "", you override filePath even if the caller's filePath was more specific. This could lead to incorrect file paths in error messages.

Consider this edge case:

  • Caller passes filePath = "/absolute/path/workflow.md"
  • vErr.File = "workflow.md" (relative path)
  • Result: Error message points to relative path instead of absolute path

Recommendation:

if errors.As(cause, &vErr) && vErr.Line > 0 {
    line = vErr.Line
    if vErr.Column > 0 {
        column = vErr.Column
    }
    // Only override filePath if vErr.File is non-empty AND is an absolute path
    if vErr.File != "" && filepath.IsAbs(vErr.File) {
        filePath = vErr.File
    }
}

Or better yet, document that FieldLocation.File should always contain absolute paths, and add validation in NewValidationErrorWithLocation.

// Skip empty lines and YAML comments
trimmed := strings.TrimSpace(line)
if trimmed == "" || strings.HasPrefix(trimmed, "#") {
continue
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment clarity issue: The comment says "Top-level keys have no leading indentation" but the check is line[0] != ' ' && line[0] != '\t'. This doesn't account for:

  1. Mixed whitespace: A line starting with \t (tab+space) would pass this check but is still indented
  2. Other whitespace: Non-breaking spaces or other Unicode whitespace characters

While these are rare in YAML files, the comment should be more precise:

// Top-level keys start at column 0 (no leading space or tab)
if len(line) > 0 && line[0] != ' ' && line[0] != '\t' {

Or use a more robust check:

// Top-level keys have no leading whitespace
if len(trimmed) > 0 && trimmed == line {
    // trimmed == line means no leading/trailing whitespace was removed

This makes the intent clearer and handles all whitespace types correctly.

yamlContent: `# workflow config
engine: copilot

on: push`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for malformed YAML: The tests cover happy paths and some edge cases, but don't test how the parser handles malformed YAML that could cause issues:

{
    name: "malformed YAML with no colon",
    yamlContent: `engine copilot
on push`,
    frontmatterStart: 2,
    wantFieldLines: map[string]int{}, // Should skip lines without colons
},
{
    name: "keys with special characters",
    yamlContent: `"key-with-quotes": value
key[0]: value
{inline}: value`,
    frontmatterStart: 2,
    wantFieldLines: map[string]int{}, // Should skip non-simple keys
},
{
    name: "indented top-level key with tab",
    yamlContent: `\tengine: copilot`,
    frontmatterStart: 2,
    wantFieldLines: map[string]int{}, // Should skip indented lines
},

These tests would verify that the parser correctly handles edge cases mentioned in the code comments.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /tdd (test-driven development) and /zoom-out (architectural consistency) based on this feature addition with comprehensive test coverage.


Key Themes

Test Coverage:

  • ✅ Excellent table-driven tests for the core parsing logic
  • ⚠️ Missing edge cases: zero-column handling, nested key name collisions
  • ⚠️ Weak assertions in some tests (e.g., checking for "3" instead of verifying "15:3" format)

Code Quality:

  • ⚠️ Potential bug in column default handling (line 37 of compiler_error_formatter.go)
  • ⚠️ Minor performance issue: double TrimSpace() call per line in hot path
  • ⚠️ Comment clarity: offset calculation could explain the "-1" more explicitly

Positive Highlights

  • Clean architectural layering: FieldLocationValidationErrorCompilerError is well-structured
  • Backward compatibility preserved: Existing errors without location still work (defaults to 1:1)
  • Thoughtful edge case handling: Comments, blank lines, duplicate keys, special characters all covered
  • Clear value proposition: IDE-navigable errors are a significant UX improvement

Verdict

The feature is solid and well-tested overall. The inline comments identify specific improvements that would strengthen the implementation:

  1. Fix the column zero-value handling bug
  2. Add missing test cases for edge scenarios
  3. Tighten test assertions to verify exact format
  4. Minor optimizations and comment clarity

None of these are blockers, but addressing them (especially #1) would prevent subtle bugs in production.


Learn more about the applied skills:

  • /tdd — Test-driven development practices
  • /zoom-out — Architectural perspective and broader impact

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 5.2M

return formatCompilerErrorWithPosition(filePath, 1, 1, errType, message, cause)
line, column := 1, 1
// Promote precise source location from WorkflowValidationError when available so that
// the emitted "file:line:col: error:" prefix points directly at the problematic field
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Potential bug: Column value handling is inconsistent.

The code checks vErr.Column > 0 but then unconditionally assigns column = vErr.Column, which could still be 0 if the check fails:

if vErr.Column > 0 {
    column = vErr.Column  // This still assigns 0 if Column == 0
}

This means errors with Line: 10, Column: 0 will produce file:10:0: instead of file:10:1:. Consider:

if vErr.Column > 0 {
    column = vErr.Column
} // else keep column = 1 (already set above)

Add a test case for FieldLocation{Line: 10, Column: 0} to verify the expected behavior.

// directly as file:line positions for IDE-navigable error messages.
func extractTopLevelFieldLines(yamlContent string, frontmatterStart int) map[string]int {
fieldLines := make(map[string]int)
relLine := 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/zoom-out] Minor performance: strings.TrimSpace() is called twice per line.

Line 32 trims the line, then line 39 trims again when checking for the colon. Consider:

trimmed := strings.TrimSpace(line)
if trimmed == "" || strings.HasPrefix(trimmed, "#") {
    continue
}
if len(line) > 0 && line[0] != ' ' && line[0] != '\t' {
    colonIdx := strings.Index(trimmed, ":")
    if colonIdx > 0 {
        key := trimmed[:colonIdx]  // Already trimmed
        // ...
    }
}

Not a blocker, but frontmatter parsing is on the hot path during compilation.

"tools": 3,
"on": 6,
},
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Test coverage gap: Missing edge case for nested key name collision.

The tests verify nested keys are excluded, but what happens when a nested key has the same name as a top-level key? Example:

tools:         # line 2
  nested:
    tools:     # line 4 - same name, but nested
      value: x

Should FieldLines["tools"] be 2 (first occurrence wins)? Add a test case to document this behavior explicitly.


wrapped := formatCompilerError("workflow.md", "error", vErr.Error(), vErr)
require.Error(t, wrapped)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Test assertion is weak: doesn't verify column format.

The test creates Column: 3 and asserts the string contains "3", but doesn't verify the format is actually line:column and not column:line or some other arrangement.

Consider a more specific assertion:

assert.Contains(t, errStr, "15:3", "Should format as line:column")

Or use a regex to verify the exact format: workflow.md:15:3: error:

// (e.g. `[anchor]: value` or `{implicit_key}: value`). These forms are not used
// in workflow frontmatter, so we skip them to avoid false positives.
// Quoted YAML keys such as `"key[0]"` are also not used in workflow frontmatter
// and are excluded by this check (the extracted substring will contain the quote).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/zoom-out] Comment could be clearer about the offset calculation.

// absoluteLine = relLine + frontmatterStart - 1 is correct but doesn't explain why we subtract 1.

Consider:

// Convert relative line number (1-based within YAML) to absolute file line:
// relLine=1 + frontmatterStart=2 - 1 = line 2 (first line after '---')
fieldLines[key] = relLine + frontmatterStart - 1

This helps future readers understand the indexing immediately.

@github-actions
Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 90/100

Excellent (with minor caution on test inflation)

Metric Value
New/modified tests analyzed 4
✅ Design tests (behavioral contracts) 4 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 4 (100%)
Duplicate test clusters 0
Test inflation detected YES (2 of 3 files exceed 2:1 ratio)
🚨 Coding-guideline violations 0

Test Classification Summary

Test File Classification Edge Cases Notes
TestExtractTopLevelFieldLines pkg/parser/frontmatter_field_lines_test.go:12 ✅ Design Yes Table-driven with 5 cases including empty/duplicates
TestFrontmatterResultFieldLines pkg/parser/frontmatter_field_lines_test.go:82 ✅ Design Yes Tests public API integration with/without frontmatter
TestFormatCompilerError_UsesLocationFromValidationError pkg/workflow/compiler_error_formatting_test.go:243 ✅ Design Yes 4 subtests covering location promotion logic
TestNewValidationErrorWithLocation pkg/workflow/error_helpers_test.go:47 ✅ Design Yes 4 subtests with timestamp/truncation edge cases

Test Inflation Analysis

⚠️ 2 of 3 test files exceed the 2:1 test-to-production ratio:

Test File Test Lines Production File Prod Lines Ratio
frontmatter_field_lines_test.go 106 frontmatter_content.go 44 2.41:1 ⚠️
compiler_error_formatting_test.go 50 compiler_error_formatter.go 17 2.94:1 ⚠️
error_helpers_test.go 48 workflow_errors.go 44 1.09:1

Impact: 10-point penalty applied to final score (reduced from 100 to 90).

Note: The inflation is primarily due to comprehensive table-driven test cases with multiple edge cases per function. This is acceptable given the critical nature of error reporting, but worth noting for future reference.


Detailed Test Analysis

📊 View Per-Test Quality Assessment

TestExtractTopLevelFieldLines

File: pkg/parser/frontmatter_field_lines_test.go:12
Classification: Design Test (Behavioral Contract)
Assertions: 1 per subtest (5 total via table-driven)
Error Coverage: Edge cases only (no runtime errors expected)

What design invariant does this test enforce?
The function correctly extracts line numbers for top-level YAML keys in frontmatter, handling edge cases like comments, blank lines, nested content, empty input, and duplicate keys.

What would break if deleted?
Regressions in line number calculation for edge cases (empty YAML, comments, duplicates) would go undetected, impacting error message accuracy.

Suggested improvements: None — comprehensive edge case coverage.


TestFrontmatterResultFieldLines

File: pkg/parser/frontmatter_field_lines_test.go:82
Classification: Design Test (Behavioral Contract)
Assertions: 8 (including require.NoError checks)
Error Coverage: Yes (require.NoError for parsing)

What design invariant does this test enforce?
The public API ExtractFrontmatterFromContent correctly populates FieldLines in its result, distinguishing between content with frontmatter (populated map) and without (nil map).

What would break if deleted?
Integration between content extraction and field line tracking would be untested. Bugs where FieldLines is incorrectly nil or populated would not be caught.

Suggested improvements: None — tests integration at public API boundary.


TestFormatCompilerError_UsesLocationFromValidationError

File: pkg/workflow/compiler_error_formatting_test.go:243
Classification: Design Test (Behavioral Contract)
Assertions: 11 (across 4 subtests)
Error Coverage: Yes (4 require.Error checks)

What design invariant does this test enforce?
When a WorkflowValidationError has location information, the formatted compiler error message includes that precise location instead of defaulting to 1:1. Critical for user-facing error messages.

What would break if deleted?
Users would see incorrect "1:1" locations in error messages even when precise location is known, significantly degrading error message quality.

Suggested improvements: None — comprehensive coverage of location promotion logic including edge cases (no location, different file, non-validation errors).


TestNewValidationErrorWithLocation

File: pkg/workflow/error_helpers_test.go:47
Classification: Design Test (Behavioral Contract)
Assertions: 17 (across 4 subtests)
Error Coverage: Yes (3 require.Error/require.NotNil checks)

What design invariant does this test enforce?
The new constructor correctly creates validation errors with location tracking, omitting timestamps when location is set, and storing file/line/column fields for downstream error formatting.

What would break if deleted?
Bugs in timestamp logic or field storage would not be caught, breaking the error reporting system's location tracking feature.

Suggested improvements: None — thorough testing of constructor behavior including edge cases (zero-line location, value truncation).


Verdict

Check passed. 0% of new tests are implementation tests (threshold: 30%). All tests enforce behavioral contracts with comprehensive edge case coverage.

Summary: This PR demonstrates excellent test quality. All 4 new/modified test functions are high-value design tests that verify behavioral contracts rather than implementation details. Every test includes edge case coverage (empty inputs, error paths, boundary conditions). The only concern is test inflation in 2 files, which is mitigated by the fact that the tests are table-driven with legitimate edge cases. The 10-point penalty acknowledges the verbosity while recognizing the value.


📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.


References:

🧪 Test quality analysis by Test Quality Sentinel · ● 6.1M ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 4 tests verify behavioral contracts with comprehensive edge case coverage.

@github-actions
Copy link
Copy Markdown
Contributor

@copilot review all comments
Please address the 22 unresolved review threads, then rerun checks.

Generated by 👨‍🍳 PR Sous Chef · ● 17M ·

@github-actions github-actions Bot mentioned this pull request May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[plan] Add file/line context to validation errors

4 participants