refactor: consolidate equality helpers, fix silent limits, centralize file permissions

[erraggy]

Summary

• 🧬 Extract generic EqualPtr[T comparable] into internal/equalutil, replacing 5 type-specific pointer-equality functions duplicated across parser/ and joiner/
• 🔔 Add depth-limit warnings to two places that previously silently truncated data: RefCollector.Warnings field in fixer/refs.go (with slog.Warn at all 4 call sites), and slog.Warn in internal/jsonpath/eval.go
• 📁 Centralize file permissions in internal/fileutil (OwnerReadWrite 0o600, ReadableByAll 0o644), replacing hardcoded literals across 12 production files

Details

Generic Pointer Equality (internal/equalutil)

Removed	Package	Replaced with
equalFloat64Ptr	parser	equalutil.EqualPtr
equalIntPtr	parser	equalutil.EqualPtr
equalBoolPtr	parser	equalutil.EqualPtr
equalPointerFloat	joiner	equalutil.EqualPtr
equalPointerInt	joiner	equalutil.EqualPtr

Silent Limit Warnings

Location	Before	After
fixer/refs.go collectRefsFromMapWithDepth	Silent return at depth 100	Appends to Warnings field + slog.Warn at all 4 call sites
internal/jsonpath/eval.go recursiveDescend	Silent return nil at depth 500	slog.Warn with depth context
internal/jsonpath/eval.go collectAllDescendants	Silent return at depth 500	slog.Warn with depth context

File Permission Constants (internal/fileutil)

Constant	Value	Replaces	Used by
OwnerReadWrite	0o600	outputFileMode, literal 0600	joiner, builder, cmd/, mcpserver/
ReadableByAll	0o644	literal 0644	generator/writer

Test plan

• make check passes (8500 tests — +2 new: TestRefCollector_DepthLimitWarning, TestRecursiveDescentDepthLimit_WarningEmitted)
• Grep confirms zero remaining references to old function names
• Grep confirms no remaining hardcoded 0600/0o600 in production code (only tests, docs, help text)
• go_diagnostics clean on all 27 modified files
• CodeRabbit review: 0 critical issues (2 nitpicks applied)
• PR review toolkit (4 agents): critical finding fixed (RefCollector.Warnings propagation)

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@erraggy has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 17 minutes and 28 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between a60dc1c and 30c869c.

📒 Files selected for processing (27)

• builder/builder.go
• cmd/oastools/commands/convert.go
• cmd/oastools/commands/fix.go
• cmd/oastools/commands/join.go
• cmd/oastools/commands/overlay.go
• fixer/oas2.go
• fixer/oas3.go
• fixer/refs.go
• fixer/refs_test.go
• fixer/stub_missing_refs.go
• generator/writer.go
• internal/equalutil/equalutil.go
• internal/equalutil/equalutil_test.go
• internal/fileutil/fileutil.go
• internal/jsonpath/eval.go
• internal/jsonpath/jsonpath_test.go
• internal/mcpserver/tools_convert.go
• internal/mcpserver/tools_fix.go
• internal/mcpserver/tools_join.go
• internal/mcpserver/tools_overlay.go
• joiner/equivalence.go
• joiner/joiner.go
• parser/equals.go
• parser/equals_helpers_test.go
• parser/parameters_equals.go
• parser/paths_equals.go
• parser/schema_equals.go

📝 Walkthrough

Walkthrough

Centralizes file permission constants and a generic pointer-equality helper, updates callers to use them, and adds depth-limit warning collection and slog warnings in JSONPath evaluation and reference pruning/stubbing. Tests added/updated for pointer equality and depth-limit warnings.

Changes

Cohort / File(s)	Summary
New utility packages internal/fileutil/fileutil.go, internal/equalutil/equalutil.go, internal/equalutil/equalutil_test.go	Add file permission constants (OwnerReadWrite, ReadableByAll) and generic EqualPtr[T comparable] with tests.
Migrate 0600 -> OwnerReadWrite (CLI & tools) builder/builder.go, cmd/oastools/commands/convert.go, cmd/oastools/commands/fix.go, cmd/oastools/commands/join.go, cmd/oastools/commands/overlay.go, internal/mcpserver/tools_convert.go, internal/mcpserver/tools_fix.go, internal/mcpserver/tools_join.go, internal/mcpserver/tools_overlay.go, joiner/joiner.go	Replace hard-coded 0o600 with fileutil.OwnerReadWrite; remove local outputFileMode where present; preserve control flow and error handling.
Migrate 0644 -> ReadableByAll generator/writer.go	Replace hard-coded 0644 with fileutil.ReadableByAll for generator output files.
Replace per-type pointer-equals with generic joiner/equivalence.go, parser/parameters_equals.go, parser/paths_equals.go, parser/schema_equals.go	Switch pointer comparisons to equalutil.EqualPtr[T]; remove redundant per-type helpers usage.
Remove old pointer helpers & tests parser/equals.go, parser/equals_helpers_test.go	Delete legacy pointer-equality helper functions and their test suites.
RefCollector warnings + tests fixer/refs.go, fixer/refs_test.go	Add Warnings []string to RefCollector; append depth-limit warning strings when truncating collection; add test asserting warning recorded and deep $ref omitted.
Emit slog warnings in prune/stub flows fixer/oas2.go, fixer/oas3.go, fixer/stub_missing_refs.go	Log collector warnings via slog.Warn after reference collection in OAS2/OAS3 prune and stub flows; imports log/slog.
JSONPath depth-limit logging and test helpers internal/jsonpath/eval.go, internal/jsonpath/jsonpath_test.go	Add package-level jsonpathLogger, emit warnings when recursion/descend depth exceeded, and add test helper suppressJSONPathLogger plus tests verifying warnings and depth-limited behavior.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• docs: add MCP server development blog post #334: Modifies output-writing codepaths including joiner.WriteResult; overlaps with this PR's output-file permission and write handling changes.
• feat(mcp): add MCP server exposing all oastools capabilities #310: Touches internal/mcpserver tool handlers (tools_*.go), overlapping with this PR's permission-constant migrations in those files.
• perf(validator,converter,schemautil,fixer): reduce allocations and eliminate regex overhead #344: Alters ref-collection logic related to fixer/refs.go; likely related to the added warning collection and depth-limit behavior.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 67.74% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely summarizes the main changes: consolidating equality helpers, fixing silent limits, and centralizing file permissions—all key refactoring objectives of the PR.
Description check	✅ Passed	The description is comprehensive, well-organized with tables, and directly addresses all major changes in the PR including the three refactoring areas and test verification.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor/consolidate-helpers-and-constants

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 73.97260% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.77%. Comparing base (39b310d) to head (30c869c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
joiner/equivalence.go	25.00%	0 Missing and 6 partials ⚠️
fixer/stub_missing_refs.go	0.00%	2 Missing and 2 partials ⚠️
fixer/oas2.go	0.00%	1 Missing and 1 partial ⚠️
fixer/oas3.go	0.00%	1 Missing and 1 partial ⚠️
generator/writer.go	0.00%	0 Missing and 2 partials ⚠️
joiner/joiner.go	0.00%	0 Missing and 2 partials ⚠️
builder/builder.go	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #348      +/-   ##
==========================================
- Coverage   84.78%   84.77%   -0.02%     
==========================================
  Files         193      194       +1     
  Lines       27255    27246       -9     
==========================================
- Hits        23108    23097      -11     
- Misses       2828     2829       +1     
- Partials     1319     1320       +1     

Files with missing lines	Coverage Δ
fixer/refs.go	75.56% <100.00%> (+0.50%)	⬆️
internal/equalutil/equalutil.go	100.00% <100.00%> (ø)
internal/jsonpath/eval.go	72.36% <100.00%> (+0.74%)	⬆️
internal/mcpserver/tools_convert.go	86.20% <100.00%> (ø)
internal/mcpserver/tools_fix.go	80.00% <100.00%> (ø)
internal/mcpserver/tools_join.go	91.42% <100.00%> (ø)
internal/mcpserver/tools_overlay.go	71.42% <100.00%> (ø)
parser/equals.go	95.76% <ø> (-0.37%)	⬇️
parser/parameters_equals.go	96.96% <100.00%> (ø)
parser/paths_equals.go	99.02% <100.00%> (ø)
... and 8 more

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@fixer/refs_test.go`:
- Around line 907-911: Replace the hard-coded loop count `102` in the test with
the canonical limit constant so the test adapts to configuration changes; in the
block that builds nested maps (the loop that updates `current =
map[string]any{"items": current}` in refs_test.go), use `maxRefCollectionDepth`
(or `maxRefCollectionDepth` +/- an appropriate offset if the test intends to
exercise boundary behavior) instead of `102` so the nesting depth derives from
the authoritative `maxRefCollectionDepth` symbol.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 39b310d and e81ea3d.

📒 Files selected for processing (27)

• builder/builder.go
• cmd/oastools/commands/convert.go
• cmd/oastools/commands/fix.go
• cmd/oastools/commands/join.go
• cmd/oastools/commands/overlay.go
• fixer/oas2.go
• fixer/oas3.go
• fixer/refs.go
• fixer/refs_test.go
• fixer/stub_missing_refs.go
• generator/writer.go
• internal/equalutil/equalutil.go
• internal/equalutil/equalutil_test.go
• internal/fileutil/fileutil.go
• internal/jsonpath/eval.go
• internal/jsonpath/jsonpath_test.go
• internal/mcpserver/tools_convert.go
• internal/mcpserver/tools_fix.go
• internal/mcpserver/tools_join.go
• internal/mcpserver/tools_overlay.go
• joiner/equivalence.go
• joiner/joiner.go
• parser/equals.go
• parser/equals_helpers_test.go
• parser/parameters_equals.go
• parser/paths_equals.go
• parser/schema_equals.go

💤 Files with no reviewable changes (2)

• parser/equals.go
• parser/equals_helpers_test.go

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@fixer/oas2.go`:
- Around line 191-193: Replace the unstructured calls to slog.Warn by emitting a
static message and attaching the warning string as a structured attribute:
iterate over collector.Warnings and call slog.Warn with a constant message like
"ref collection warning" and include the warning text as an attribute (e.g.,
using slog.String("warning", w)) so tools can aggregate by message while
preserving the specific detail; update the occurrences around collector.Warnings
and slog.Warn to use this structured form.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e81ea3d and a60dc1c.

📒 Files selected for processing (27)

• builder/builder.go
• cmd/oastools/commands/convert.go
• cmd/oastools/commands/fix.go
• cmd/oastools/commands/join.go
• cmd/oastools/commands/overlay.go
• fixer/oas2.go
• fixer/oas3.go
• fixer/refs.go
• fixer/refs_test.go
• fixer/stub_missing_refs.go
• generator/writer.go
• internal/equalutil/equalutil.go
• internal/equalutil/equalutil_test.go
• internal/fileutil/fileutil.go
• internal/jsonpath/eval.go
• internal/jsonpath/jsonpath_test.go
• internal/mcpserver/tools_convert.go
• internal/mcpserver/tools_fix.go
• internal/mcpserver/tools_join.go
• internal/mcpserver/tools_overlay.go
• joiner/equivalence.go
• joiner/joiner.go
• parser/equals.go
• parser/equals_helpers_test.go
• parser/parameters_equals.go
• parser/paths_equals.go
• parser/schema_equals.go

💤 Files with no reviewable changes (2)

• parser/equals_helpers_test.go
• parser/equals.go