Skip to content

feat(fixer): repair path parameters missing required: true - #390

Merged
erraggy merged 1 commit into
mainfrom
fix/381-path-param-required-fixtype
Jul 27, 2026
Merged

feat(fixer): repair path parameters missing required: true#390
erraggy merged 1 commit into
mainfrom
fix/381-path-param-required-fixtype

Conversation

@erraggy

@erraggy erraggy commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Fixes #381.

#378 added the OAS 2.0 required: true check for path parameters and made the defect visible in both validators. It did not make it fixable — there was no corresponding FixType, and oastools fix left every one of those errors behind.

The fix

FixTypePathParameterNotRequired ("path-parameter-not-required") sets required: true on any in: path parameter that omits it. The spec permits no other value, so there is no judgment call, nothing to configure, and no information to lose.

It runs at the three sites the validator reports the defect, with paths that match the validator's exactly:

Site Reported path
OAS 2.0 root-level parameters parameters.{name}

Both constraints carried over from #378 hold: parameters that are a $ref are skipped so the definition they name is repaired once instead of at every use site, and the path-item pass runs once per path item rather than inside the per-operation loop.

Keeping the validator and fixer aligned

Rather than duplicate the traversal, the defect test moved to paramutil.NeedsRequiredTrue and both packages now call it. That collapsed four copies of the condition into one and fixed a latent inconsistency along the way: validator/oas3.go compared against a bare "path" literal while oas2.go used parser.ParamInPath.

TestFixPathParamsRequired_ResolvesValidatorErrors is the guard: it validates a defective spec, asserts the error is reported, fixes, re-validates, and asserts no required: true error survives — for both OAS versions, including through $ref use sites. If the two sides ever drift, this fails.

Enabled by default

The issue's complaint was that oastools fix could not repair this, so leaving the fix opt-in would have needed a new flag to actually resolve it. It joins FixTypeMissingPathParameter in the always-on set via a new exported fixer.DefaultEnabledFixes(), which the CLI and MCP server now call instead of each hardcoding their own copy of the list.

Corpus impact is nil: DigitalOcean, Asana, Plaid, and GitHub each produce 0 fixes with unchanged validation error counts; none of them exhibit this defect.

Notes

  • Honors DryRun (records the fix, leaves the document alone), which FixTypeMissingPathParameter currently does not.
  • webhooks path items are untouched, matching the validator, which does not check them for this rule either.
  • Docs updated: package docs, fixer/deep_dive.md, CLI --help and cli-reference.md, MCP tool description and mcp-server.md, whitepaper.

Verified with make check (green: lint + 8861 tests) and gopls diagnostics (clean), plus an end-to-end CLI round trip in both versions: validate reports the errors, fix repairs them, validate then passes.

Fixes #381.

#378 added the OAS 2.0 `required: true` check for path parameters and made the
defect visible in both validators. It did not make it fixable — there was no
corresponding FixType, and `oastools fix` left every one of those errors behind.

## The fix

`FixTypePathParameterNotRequired` ("path-parameter-not-required") sets
`required: true` on any `in: path` parameter that omits it. The spec permits no
other value, so there is no judgment call, nothing to configure, and no
information to lose.

It runs at the three sites the validator reports the defect, with paths that
match the validator's exactly:

| Site | Reported path |
|---|---|
| OAS 2.0 root-level `parameters` | `parameters.{name}` |
| OAS 3.x `components.parameters` | `components.parameters.{name}` |
| Path item and operation, both versions | `paths.{path}[.{method}].parameters[{i}]` |

Both constraints carried over from #378 hold: parameters that are a `$ref` are
skipped so the definition they name is repaired once instead of at every use
site, and the path-item pass runs once per path item rather than inside the
per-operation loop.

## Keeping the validator and fixer aligned

Rather than duplicate the traversal, the *defect test* moved to
`paramutil.NeedsRequiredTrue` and both packages now call it. That collapsed four
copies of the condition into one and fixed a latent inconsistency along the way:
`validator/oas3.go` compared against a bare `"path"` literal while `oas2.go` used
`parser.ParamInPath`.

`TestFixPathParamsRequired_ResolvesValidatorErrors` is the guard: it validates a
defective spec, asserts the error is reported, fixes, re-validates, and asserts
no `required: true` error survives — for both OAS versions, including through
`$ref` use sites. If the two sides ever drift, this fails.

## Enabled by default

The issue's complaint was that `oastools fix` could not repair this, so leaving
the fix opt-in would have needed a new flag to actually resolve it. It joins
`FixTypeMissingPathParameter` in the always-on set via a new exported
`fixer.DefaultEnabledFixes()`, which the CLI and MCP server now call instead of
each hardcoding their own copy of the list.

Corpus impact is nil: DigitalOcean, Asana, Plaid, and GitHub each produce 0 fixes
with unchanged validation error counts — none of them exhibit this defect.

## Notes

- Honors `DryRun` (records the fix, leaves the document alone), which
  `FixTypeMissingPathParameter` currently does not.
- `webhooks` path items are untouched, matching the validator, which does not
  check them for this rule either.
- Docs updated: package docs, fixer/deep_dive.md, CLI `--help` and
  cli-reference.md, MCP tool description and mcp-server.md, whitepaper.

Verified with `make check` (green: lint + 8861 tests) and gopls diagnostics
(clean), plus an end-to-end CLI round trip in both versions: validate reports the
errors, fix repairs them, validate then passes.
@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a default-enabled fixer for existing OpenAPI path parameters missing required: true, shared requiredness detection across validators, OAS 2.0/3.x repair logic, CLI/MCP wiring, integration aliases, and documentation.

Changes

Path parameter requiredness

Layer / File(s) Summary
Shared requiredness predicate and validation
internal/paramutil/*, validator/*
Centralizes detection of non-reference path parameters missing required: true and applies it across OAS 2.0 and OAS 3.x validation.
Fix type defaults and pipeline wiring
fixer/fixer.go, fixer/pipeline.go, fixer/fixer_test.go, fixer/*_test.go
Adds FixTypePathParameterNotRequired, exposes fresh default fixes, and wires version-specific repair passes after missing path-parameter fixes.
OAS path parameter repair and tests
fixer/pathparam_required.go, fixer/pathparam_required_test.go
Repairs reusable, path-item, and operation parameters across OAS versions, with dry-run, reference, ordering, nil-safety, and validator-parity coverage.
Command, MCP, integration, and documentation updates
cmd/oastools/commands/fix.go, internal/mcpserver/*, integration/harness/*, docs/*, fixer/*.md, fixer/doc.go
Uses the expanded default fix set in CLI and MCP flows, adds configuration aliases, and documents the new fix behavior and scope.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant OpenAPI document
  participant Validator
  participant Fixer
  Validator->>OpenAPI document: Report path parameters missing required true
  Fixer->>OpenAPI document: Set required true on repairable parameters
  Fixer-->>Validator: Revalidate the updated document
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR satisfies #381 by adding the fix type, covering all required OAS locations, skipping $ref, and reusing validator logic.
Out of Scope Changes check ✅ Passed The extra docs, tests, CLI/MCP updates, and validator/paramutil changes all support the requested fixer and stay in scope.
Docstring Coverage ✅ Passed Docstring coverage is 92.00% which is sufficient. The required threshold is 80.00%.
Title check ✅ Passed The title clearly and concisely summarizes the main change: fixing path parameters missing required:true.
Description check ✅ Passed The description directly explains the path-parameter fixer, its scope, default behavior, and related validation changes.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/381-path-param-required-fixtype

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@erraggy erraggy self-assigned this Jul 26, 2026
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.53086% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.05%. Comparing base (4655686) to head (b3fac56).

Files with missing lines Patch % Lines
fixer/pathparam_required.go 96.77% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #390      +/-   ##
==========================================
+ Coverage   85.02%   85.05%   +0.03%     
==========================================
  Files         194      196       +2     
  Lines       27666    27739      +73     
==========================================
+ Hits        23522    23593      +71     
- Misses       2812     2813       +1     
- Partials     1332     1333       +1     
Files with missing lines Coverage Δ
fixer/fixer.go 91.42% <100.00%> (+0.09%) ⬆️
fixer/pipeline.go 97.53% <100.00%> (+0.27%) ⬆️
internal/mcpserver/server.go 91.85% <100.00%> (ø)
internal/mcpserver/tools_fix.go 79.71% <100.00%> (-0.29%) ⬇️
internal/paramutil/pathparam.go 100.00% <100.00%> (ø)
validator/oas2.go 89.76% <100.00%> (ø)
validator/oas3.go 86.33% <100.00%> (ø)
validator/path_validation.go 96.72% <100.00%> (ø)
fixer/pathparam_required.go 96.77% <96.77%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@erraggy
erraggy merged commit 5e595cb into main Jul 27, 2026
8 checks passed
@erraggy
erraggy deleted the fix/381-path-param-required-fixtype branch July 27, 2026 00:08
erraggy added a commit that referenced this pull request Jul 27, 2026
* chore: pre-release doc accuracy and examples for v1.56.0

Release-prep review found statements that #390 made false, plus the one
gap in godoc example coverage.

## Statements the new default fix made false

- plugin/skills/fix-spec/SKILL.md named a single default fix
- examples/workflows/fixer-showcase/README.md said only
  FixTypeMissingPathParameter was enabled by default
- fixer/deep_dive.md's "start with defaults" named one fix type

All three now name both members of DefaultEnabledFixes().

## Exhaustiveness claims softened

fixer-showcase claimed to demonstrate every fix type but covers 6 of 8 —
stale since v1.46.0 for FixTypeStubMissingRef, and now missing
FixTypePathParameterNotRequired as well. Rather than extend an example
program on a release branch, the claim is now "common fix types" in
main.go (package comment, printed header, footer list) and across the
four READMEs that repeated it. Extending the showcase to all 8 is filed
as a follow-up.

Also corrected that README's stale "Found 4 validation errors" to 5. The
extra error is the component-name check added in v1.54.0, which had gone
unrecorded here for four releases.

## godoc examples

fixer/example_test.go gains Example_pathParameterNotRequired, which was
the only FixType of the eight without an example, and
ExampleDefaultEnabledFixes for the newly exported helper. 8861 -> 8863
tests.

## Lint config

markdownlint globbed agent worktrees under .claude/worktrees/, where a
full checkout defeats every path exclusion above it: docs/plans/ is
excluded, but .claude/worktrees/<name>/docs/plans/ was not, so an
intentionally-skipped file failed the lint through its worktree path.
Excluding that directory fixes `make lint-md` for anyone with a worktree
open.

Verified with make check: 0 golangci-lint issues, 0 markdownlint issues,
8863 tests.

* chore: bump plugin version to 1.56.0

* chore: add benchmark results for v1.56.0

Generated by CI benchmark workflow on chore/v1.56.0-release-prep

🤖 Generated automatically

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

Fixer has no FixType for path parameters missing required: true

1 participant