Skip to content

fix: triage jsonmarshalignoredeerror violations and enforce linter in CI#36032

Merged
pelikhan merged 3 commits into
mainfrom
copilot/triage-enforce-jsonmarshalignoredeerror
May 31, 2026
Merged

fix: triage jsonmarshalignoredeerror violations and enforce linter in CI#36032
pelikhan merged 3 commits into
mainfrom
copilot/triage-enforce-jsonmarshalignoredeerror

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 31, 2026

19 production sites across 11 files discarded json.Marshal errors with _, causing silent nil-byte writes into generated workflow YAML/JSON. The jsonmarshalignoredeerror custom linter existed but was not enforced and had no //nolint directive support.

Changes

Linter: add //nolint directive support

pkg/linters/jsonmarshalignoredeerror/jsonmarshalignoredeerror.go — integrated internal/nolint (matching the errstringmatch pattern) so callers can suppress with //nolint:jsonmarshalignoredeerror. Added a Suppressed() test case to testdata.

Tier 1 — propagate error (function already returns error)

pkg/workflow/safe_outputs_config_generation.go:198 — the surrounding function returns (string, error) but was silently swallowing the marshal failure, yielding "" with a nil error:

// before
configJSON, _ := json.Marshal(safeOutputsConfig)
return string(configJSON), nil

// after
configJSON, err := json.Marshal(safeOutputsConfig)
if err != nil {
    return "", fmt.Errorf("marshaling safe-outputs config: %w", err)
}
return string(configJSON), nil

Tier 3 — annotate provably-safe sites (18 sites, 10 files)

All remaining sites marshal types that cannot fail (string, []string, *ExperimentConfig / VSCodeMCPServer with only basic-type fields). Each gets a //nolint:jsonmarshalignoredeerror comment with a brief justification, e.g.:

allowedExtsJSON, _ := json.Marshal(cache.AllowedExtensions) //nolint:jsonmarshalignoredeerror // marshaling a string slice cannot fail

CI enforcement

Added -jsonmarshalignoredeerror to LINTER_FLAGS in .github/workflows/cgo.yml, locking the guard alongside the 8 previously enforced analyzers.

Copilot AI and others added 2 commits May 31, 2026 01:32
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
- Tier 1: properly handle json.Marshal error in safe_outputs_config_generation.go
- Add //nolint:jsonmarshalignoredeerror to all provably-safe sites (string,
  []string, VSCodeMCPServer, ExperimentConfig marshal calls)
- Add nolint directive support to jsonmarshalignoredeerror linter (matching
  the errstringmatch pattern using internal/nolint package)
- Add Suppressed() test case to linter testdata to cover nolint behavior
- Enforce linter by adding -jsonmarshalignoredeerror to LINTER_FLAGS in cgo.yml

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

Hey @Copilot 👋 — great to see work starting on enforcing the jsonmarshalignoredeerror linter across production call sites! Tightening up ignored marshal errors is solid hygiene. A couple of things to address before this is ready for review:

  • Missing description — the PR body is currently a task checklist, but there's no prose explaining what is being done and why (e.g., what the linter catches, what risk it mitigates, what strategy was chosen for Tier 1 vs. Tier 3 sites). A short summary would help reviewers understand the intent at a glance.
  • No tests — none of the diff touches test files. Once the Tier 1 fix (proper json.Marshal error handling in safe_outputs_config_generation.go) and the Tier 3 //nolint annotations land, consider adding or updating linter integration tests to confirm the new flag is exercised in CI.
  • Diff vs. checklist mismatch — the only actual code change is a body_hash bump in .github/workflows/daily-model-inventory.lock.yml. All 13 checklist items remain unchecked. The PR should stay in draft until the substantive changes are committed.

If you'd like a hand closing out the checklist, you can assign this prompt to your coding agent:

Complete the jsonmarshalignoredeerror linter enforcement work in this WIP PR:

1. Tier 1 fix: In `safe_outputs_config_generation.go:198`, handle the error returned by `json.Marshal` properly instead of ignoring it.

2. Tier 3 nolint annotations: For each of the following locations where the value being marshalled is a safe primitive type (string, []string, or a known-safe struct), add a `//nolint:jsonmarshalignoredeerror` comment with a short justification:
   - compiler_experiments.go:451,457,461
   - compiler_pre_activation_job.go:168,172,230
   - compiler_yaml.go:814
   - cache.go:636,917
   - repo_memory.go:717
   - mcp_config_file.go:59,60
   - mcp_config_playwright_renderer.go:73,80,87
   - args.go:62
   - mcp_renderer_github.go:249
   - copilot_logs.go:97

3. Enforce: Add `-jsonmarshalignoredeerror` to `LINTER_FLAGS` in `.github/workflows/cgo.yml`.

4. Validate locally with:
   make golint-custom LINTER_FLAGS="-jsonmarshalignoredeerror -test=false"

5. Update the PR description with a short prose summary of what the linter catches, why Tier 1 sites get a real fix vs. Tier 3 sites get nolint, and confirm CI passes.

Generated by ✅ Contribution Check · sonnet46 2.9M ·

Copilot AI changed the title [WIP] Triage and enforce jsonmarshalignoredeerror linter for prod sites fix: triage jsonmarshalignoredeerror violations and enforce linter in CI May 31, 2026
Copilot AI requested a review from gh-aw-bot May 31, 2026 01:39
@pelikhan pelikhan marked this pull request as ready for review May 31, 2026 01:55
Copilot AI review requested due to automatic review settings May 31, 2026 01:55
@pelikhan pelikhan merged commit f31ccb9 into main May 31, 2026
@pelikhan pelikhan deleted the copilot/triage-enforce-jsonmarshalignoredeerror branch May 31, 2026 01:55
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

This PR fixes and enforces handling for ignored encoding/json marshal errors by propagating the one actionable production error, documenting safe suppressions, and enabling the existing custom analyzer in CI.

Changes:

  • Propagates json.Marshal errors from safe outputs config generation.
  • Adds //nolint:jsonmarshalignoredeerror support and testdata for the custom linter.
  • Annotates currently safe ignored marshal sites and enables the linter in CI.
Show a summary per file
File Description
pkg/workflow/safe_outputs_config_generation.go Propagates marshal failure with contextual error.
pkg/workflow/repo_memory.go Suppresses safe string-slice marshal discard.
pkg/workflow/mcp_renderer_github.go Suppresses safe string marshal discard.
pkg/workflow/mcp_config_playwright_renderer.go Suppresses safe string marshal discards in JSON array rendering.
pkg/workflow/copilot_logs.go Suppresses len-only input-size marshal discard.
pkg/workflow/compiler_yaml.go Suppresses safe string-slice marshal discard.
pkg/workflow/compiler_pre_activation_job.go Suppresses safe string-slice marshal discards.
pkg/workflow/compiler_experiments.go Suppresses safe experiment spec marshal discards.
pkg/workflow/cache.go Suppresses safe string-slice marshal discards.
pkg/workflow/args.go Suppresses safe string marshal discard.
pkg/linters/jsonmarshalignoredeerror/testdata/src/jsonmarshalignoredeerror/jsonmarshalignoredeerror.go Adds a suppression testdata case.
pkg/linters/jsonmarshalignoredeerror/jsonmarshalignoredeerror.go Adds shared nolint directive support.
pkg/cli/mcp_config_file.go Suppresses safe VS Code MCP server config marshal discards.
.github/workflows/daily-model-inventory.lock.yml Updates generated metadata hash.
.github/workflows/cgo.yml Enables jsonmarshalignoredeerror in custom linter CI flags.

Copilot's findings

Tip

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

  • Files reviewed: 15/15 changed files
  • Comments generated: 1

Comment on lines +59 to +62
position := pass.Fset.PositionFor(call.Pos(), false)
if nolint.HasDirective(position, noLintLinesByFile) {
return
}
@github-actions github-actions Bot mentioned this pull request May 31, 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.

Triage & enforce jsonmarshalignoredeerror: 19 prod sites discard json.Marshal errors (silent corruption in code-gen)

4 participants