Skip to content

feat: support OpenAPI Overlay 1.2 - #651

Merged
daveshanley merged 6 commits into
mainfrom
codex/overlay-1.2
Oct 7, 2026
Merged

daveshanley merged 6 commits into
mainfrom
codex/overlay-1.2

Conversation

@daveshanley

@daveshanley daveshanley commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Add Overlay 1.2 document identity and reusable actions, with parsing, rendering, hashing, local reference resolution, and public API coverage for YAML and JSON inputs.

Correct action semantics: primitive replacement, array appends, recursive type compatibility, removal precedence, and snapshot-based copies. Wide object merges use indexed lookup while small merges keep zero lookup allocations. Errors and warnings retain the original action and its source location.

Risk: Medium. These specification corrections also apply to existing 1.0/1.1 overlays. Incompatible container updates now fail; copy and update on one action suppress each other and emit a warning for matched targets; missing targets and malformed structures are rejected. Fixed fields use exact case, Info rejects $ref, and application renders block YAML. Scalar string values, including unquoted numeric versions, remain accepted. Version strings accept 1. with an optional patch, including later minors, without claiming support for future features. Apply does not validate unused document identifiers; URI resolution validates them when needed. Compatibility details and URI behavior.

Validation on d5849aa: full local suite with network tests enabled passed at 100% statement coverage, with zero uncovered blocks. All three Overlay packages also pass with the race detector and 100% coverage. Public API regressions and independent repair review passed. All five review findings are fixed and resolved. Hosted CI is rerunning on this head.

Six-sample wide-merge medians improved from 24.6 µs to 5.05 µs at 200 properties, and 3.33 ms to 72.5 µs at 2,000 properties. Small merges retain zero lookup allocations.

@codecov

codecov Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (ce94592) to head (d5849aa).

Additional details and impacted files
@@            Coverage Diff             @@
##              main      #651    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          301       308     +7     
  Lines        38415     38781   +366     
==========================================
+ Hits         38415     38781   +366     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@daveshanley
daveshanley marked this pull request as ready for review October 6, 2026 15:09

@daveshanley daveshanley left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Needs changes before this merges. Four P2s and one P3, all inline. Reviewed at cddaee1a.

The core of this is good work. The merge rules finally match the spec (primitive replace, array append, recursive compatibility errors instead of quietly replacing containers). Snapshotting the copy source before mutating is the right call. Keeping $ref resolution local, with no loader and no index, is exactly how it should be. And the wide-merge fix (map only above the threshold, linear scan below it) gets the quadratic scan out without making small merges allocate. Tests are solid, and the overlay, datamodel/low/overlay and datamodel/high/overlay packages plus the root Overlay tests all pass locally.

The problem is strictness. Several new checks reject overlays that applied fine on main, and none of the rejected things affect the result. I ran the same probes through ApplyOverlayFromBytesToSpecBytes on main and on this head:

input main this PR
info.version: 1 or 1.0 applies parse error
overlay: 1.0 unquoted applies parse error
overlay: 1.3.0 applies ErrUnsupportedVersion
extends with a space in the path applies URI error
copy + update on one action copy, then update no-op, 0 warnings

The first four break consumers (vacuum apply-overlay included) on upgrade, for no gain. The last one is spec-correct, but it's silent, so people won't find out their overlay stopped doing anything.

A few things I checked that are fine:

  • Security: no new I/O. ResolveExtends is pure, and references to other documents are rejected. No new exposure.
  • Performance: one CloneYAMLNode per copy action, the property map only exists for wide merges, the version regex compiles once, and an action without $ref resolves to itself with no allocation. I didn't re-run the benchmarks, so the µs numbers in the description are yours, not mine.
  • Memory: nothing long-lived is added. Reusable update nodes are shared across references, but every insert path clones, so nothing from the overlay ends up aliased into the target document.

One thing that isn't this PR: YAML aliases in the target (allOf: [*base]). On main, an update through an alias quietly replaced it and lost type: object. Here it errors. That's better, but neither is right. Worth a follow-up to dereference with utils.NodeAlias.

Comment thread datamodel/low/overlay/extract.go Outdated
Comment thread overlay/validation.go Outdated
Comment thread overlay/validation.go Outdated
Comment thread overlay/engine.go
Comment thread overlay/reusable.go

Copy link
Copy Markdown
Member Author

Addressed all five review findings in df19201 and d5849aa, replied with evidence, and resolved every inline thread. The full local suite passes with 100% statement coverage and zero uncovered blocks; the three Overlay packages also pass with the race detector. Independent repair review found no issues. Rebased onto main ce94592, so this PR also includes the merged final-line coverage fix.

The target YAML-alias behavior noted in the review remains outside these repairs, as the review identifies it as inherited behavior.

@greptileai review
Please review d5849aa. Since cddaee1, the changes restore scalar/version/unused-URI compatibility, warn on copy+update no-ops, and retain the original source action in diagnostics.

@daveshanley
daveshanley merged commit 55aa758 into main Oct 7, 2026
8 checks passed
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.

1 participant