gate: fail on every gopls diagnostic

hexajon commented

2026-09-24 18:15:16 +00:00

Owner

What

The gate passed while gopls reported 34 findings across the module. The gate runs staticcheck and modernize, but not gopls, whose analyzers (for example unusedparams, the staticcheck quick fixes, and deprecation notices) report what the editor shows. The gate should fail on every one of those warnings.

29 of the 34 have gopls quick fixes. #631 applies them, and it merges before this issue. The rest are below.

How

A new heavy-block leg in scripts/gate.sh runs gopls check -severity=hint over every git-visible Go file and fails on any output. gopls check exits 0 even with findings, so the leg must test the output, not the exit code. It must check the whole module at once: a run over part of it missed findings. The leg needs gopls installed in ci.yml too, pinned to the setup-dev.sh version.

Findings still open

internal/gitexec/env_test.go:29: "do not pass a nil Context". The test passes a nil context on purpose, to cover gitexec.NewCmd's nil tolerance. If no production caller still passes nil, the nil tolerance and that test case both go.
internal/mcp/repoguard.go:157: session.ListRoots and mcp.ListRootsParams are deprecated (SEP-2577). #428 keeps the roots call on purpose until a client supports SEP-2322. The line already carries //lint:ignore SA1019 with that reason, which is why staticcheck passes, but gopls does not read that directive. A gate that fails on every gopls warning conflicts with #428.

What the roots call feeds today: only guardLocalGitTarget (internal/mcp/multirepo.go), which guards the local-git tools (add, commit, push, pull, branch create and delete, finish) when a call passes no dir=. The per-call wrong-repo guard in consolidated_tool.go passes a nil session and never uses roots.

Options:
- Migrate off roots now. The gate stays exception-free. A session whose repo contains nested repos must pass dir= or set MADTEA_CALLER_REPO for local-git calls, because the guard can no longer confirm the target. A mismatch between the client's root and the server's repo is no longer caught until SEP-2322.
- Keep roots and hold this leg until the migration. No behavior change, but gopls findings stay ungated into 2027 at the latest.
- Keep roots, and let the leg honor the existing recorded exception on that one line, failing on everything else.

Acceptance

scripts/gate.sh has a gopls leg that fails on any gopls output over the whole module, and ci.yml installs the pinned gopls.
The leg is green on main, with the roots finding resolved by whichever option is chosen.

## What The gate passed while gopls reported 34 findings across the module. The gate runs staticcheck and modernize, but not gopls, whose analyzers (for example `unusedparams`, the staticcheck quick fixes, and deprecation notices) report what the editor shows. The gate should fail on every one of those warnings. 29 of the 34 have gopls quick fixes. #631 applies them, and it merges before this issue. The rest are below. ## How A new heavy-block leg in `scripts/gate.sh` runs `gopls check -severity=hint` over every git-visible Go file and fails on any output. `gopls check` exits 0 even with findings, so the leg must test the output, not the exit code. It must check the whole module at once: a run over part of it missed findings. The leg needs `gopls` installed in `ci.yml` too, pinned to the `setup-dev.sh` version. ## Findings still open 1. `internal/gitexec/env_test.go:29`: "do not pass a nil Context". The test passes a nil context on purpose, to cover `gitexec.NewCmd`'s nil tolerance. If no production caller still passes nil, the nil tolerance and that test case both go. 2. `internal/mcp/repoguard.go:157`: `session.ListRoots` and `mcp.ListRootsParams` are deprecated (SEP-2577). #428 keeps the roots call on purpose until a client supports SEP-2322. The line already carries `//lint:ignore SA1019` with that reason, which is why staticcheck passes, but gopls does not read that directive. A gate that fails on every gopls warning conflicts with #428. What the roots call feeds today: only `guardLocalGitTarget` (`internal/mcp/multirepo.go`), which guards the local-git tools (add, commit, push, pull, branch create and delete, finish) when a call passes no `dir=`. The per-call wrong-repo guard in `consolidated_tool.go` passes a nil session and never uses roots. Options: - **Migrate off roots now.** The gate stays exception-free. A session whose repo contains nested repos must pass `dir=` or set `MADTEA_CALLER_REPO` for local-git calls, because the guard can no longer confirm the target. A mismatch between the client's root and the server's repo is no longer caught until SEP-2322. - **Keep roots and hold this leg** until the migration. No behavior change, but gopls findings stay ungated into 2027 at the latest. - **Keep roots, and let the leg honor the existing recorded exception** on that one line, failing on everything else. ## Acceptance - `scripts/gate.sh` has a gopls leg that fails on any gopls output over the whole module, and `ci.yml` installs the pinned gopls. - The leg is green on main, with the roots finding resolved by whichever option is chosen.

hexajon added the

status/needs-decision

testing

labels

2026-09-24 18:15:16 +00:00

hexajon referenced this issue

2026-09-24 18:15:25 +00:00

security: adopt the SDK roots-replacement (SEP-2322) so the wrong-repo write guard survives 2026-07-28 clients #428

hexajon commented

2026-09-24 18:42:41 +00:00

Author

Owner

The roots question is already decided in the record. The ADR 0019 amendment of 2026-09-08 (runtime working directory) says a root the operator adds at runtime is in scope, and that this "relies on the MCP roots capability, deprecated (SEP-2577) but functional into 2027; migration to its successor (SEP-2322) is tracked in issue 428."

So roots stay until that migration, and the call at internal/mcp/repoguard.go:157 is intentional. Its //lint:ignore SA1019 directive records the reason. The gopls leg honors a finding only when the line above carries such a directive, and fails on everything else.

The nil-context finding goes the other way: no production caller passes a nil context (staticcheck SA1012 in the gate rejects a literal nil, and both finish entry points set an unset context before use). So gitexec.NewCmd's nil guard and the test that passed nil are removed, together with their directive.

The roots question is already decided in the record. The ADR 0019 amendment of 2026-09-08 (runtime working directory) says a root the operator adds at runtime is in scope, and that this "relies on the MCP `roots` capability, deprecated (SEP-2577) but functional into 2027; migration to its successor (SEP-2322) is tracked in issue 428." So roots stay until that migration, and the call at `internal/mcp/repoguard.go:157` is intentional. Its `//lint:ignore SA1019` directive records the reason. The gopls leg honors a finding only when the line above carries such a directive, and fails on everything else. The nil-context finding goes the other way: no production caller passes a nil context (staticcheck SA1012 in the gate rejects a literal nil, and both finish entry points set an unset context before use). So `gitexec.NewCmd`'s nil guard and the test that passed nil are removed, together with their directive.

hexajon removed the

status/needs-decision

label

2026-09-24 18:42:50 +00:00

hexajon referenced this issue

2026-09-24 19:35:55 +00:00

Apply the gopls quick fixes on main #631

hexajon added a new dependency

2026-09-24 19:36:01 +00:00

#631 Apply the gopls quick fixes on main

hexajon referenced this issue

2026-09-28 18:45:41 +00:00