Skip to content

Add configurable file-size limit to prevent OOM - #130

Merged
jeduden merged 12 commits into
mainfrom
claude/add-file-size-limit-Yrkil
Apr 19, 2026
Merged

Add configurable file-size limit to prevent OOM#130
jeduden merged 12 commits into
mainfrom
claude/add-file-size-limit-Yrkil

Conversation

@jeduden

@jeduden jeduden commented Apr 10, 2026

Copy link
Copy Markdown
Owner

Summary

Implements a configurable file-size limit across all file-read paths in mdsmith to prevent out-of-memory (OOM) errors when processing unexpectedly large files. The default limit is 2 MB, configurable via .mdsmith.yml or the --max-input-size CLI flag.

Key Changes

  • New size-limit helpers (internal/lint/limits.go):

    • ReadFileLimited() and ReadFSFileLimited() enforce byte-size caps on file reads
    • Zero or negative limits disable the cap (unlimited mode)
    • Error messages include actual file size and configured limit
  • Size parsing (internal/config/size.go):

    • ParseSize() parses human-readable sizes: 2MB, 500KB, 1GB, bare integers, or 0
    • Case-insensitive, uses binary units (1 MB = 1,048,576 bytes)
  • Configuration:

    • Added MaxInputSize field to config.Config
    • Added MaxInputBytes field to engine.Runner, fix.Fixer, and lint.File
    • Config file reads capped at 1 MB to prevent accidental OOM from config itself
  • CLI integration:

    • New --max-input-size flag on check and fix subcommands
    • CLI flag overrides config file setting
    • Default: 2MB
  • Comprehensive coverage:

    • Replaced ~15 os.ReadFile() and fs.ReadFile() calls with limited variants
    • Affected: runner, fixer, stdin, includes, catalog, cross-file references, required-structure, metrics, merge driver, and config loading
    • Rules access limit via lint.File.MaxInputBytes field
  • Testing:

    • Unit tests for ReadFileLimited, ReadFSFileLimited, and ParseSize
    • E2E tests: file exceeding limit produces error diagnostic and exit code 2
    • Config override test: CLI flag overrides config setting

Implementation Details

  • Files exceeding the limit are rejected with exit code 2 and a clear error message
  • The +1 sentinel in readLimited() distinguishes "exactly at limit" from "truncated"
  • When available, actual file size is reported in error messages (via Stat())
  • Unlimited mode (max <= 0) bypasses all limit checks for performance
  • Config merge logic refactored to use new copyConfig() helper for consistency

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ

Copilot AI review requested due to automatic review settings April 10, 2026 11:16
@codecov

codecov Bot commented Apr 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.41033% with 48 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.35%. Comparing base (1bde75e) to head (ece99e8).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
cmd/mdsmith/main.go 86.88% 10 Missing and 6 partials ⚠️
cmd/mdsmith/mergedriver.go 54.54% 10 Missing and 5 partials ⚠️
internal/config/load.go 80.95% 2 Missing and 2 partials ⚠️
internal/lint/limits.go 86.66% 2 Missing and 2 partials ⚠️
internal/rules/catalog/rule.go 84.61% 3 Missing and 1 partial ⚠️
internal/config/merge.go 91.30% 1 Missing and 1 partial ⚠️
internal/rules/crossfilereferenceintegrity/rule.go 84.61% 2 Missing ⚠️
internal/rules/requiredstructure/rule.go 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
+ Coverage   86.31%   86.35%   +0.04%     
==========================================
  Files          92       94       +2     
  Lines        9950    10149     +199     
==========================================
+ Hits         8588     8764     +176     
- Misses        889      903      +14     
- Partials      473      482       +9     

☔ View full report in Codecov by Sentry.
📢 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a configurable maximum input size (default 2 MiB) to harden mdsmith against OOM by enforcing byte caps across file-read paths, with configuration via .mdsmith.yml and --max-input-size.

Changes:

  • Introduces limited read helpers (ReadFileLimited, ReadFSFileLimited) and threads MaxInputBytes through runner/fixer/rules.
  • Adds human-readable size parsing (ParseSize) and wires CLI/config merging + docs updates.
  • Expands test coverage with unit tests for helpers/parsing and e2e tests for limit behavior and CLI override.

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
plan/81_oom-file-size-limit.md Marks plan complete and checks off tasks/acceptance criteria.
PLAN.md Updates plan index status for item 81 to ✅.
internal/rules/requiredstructure/rule.go Applies size-limited reads to schemas and schema includes; threads max bytes into parsing flow.
internal/rules/requiredstructure/rule_test.go Updates tests for new parseSchema(..., maxBytes) signature.
internal/rules/include/rule.go Applies size-limited reads to includes and cycle scanning.
internal/rules/crossfilereferenceintegrity/rule.go Applies size-limited reads when resolving targets for cross-file anchor checks.
internal/rules/catalog/rule.go Applies size-limited reads for front matter extraction and include-cycle scanning.
internal/rules/catalog/rule_test.go Updates tests for new readFrontMatter(..., maxBytes) signature.
internal/metrics/rank.go Applies size-limited reads in metrics collection and threads max bytes into API.
internal/metrics/metrics_coverage_test.go Updates tests for new Collect(..., maxBytes) signature.
internal/lint/limits.go Adds default cap constant and limited-read helpers with size error reporting.
internal/lint/limits_test.go Adds unit tests for limited-read helpers (normal/at/over/unlimited/etc).
internal/lint/file.go Adds MaxInputBytes field to thread limit into rules.
internal/fix/fix.go Uses limited reads for fix input and propagates max bytes into lint.File.
internal/engine/runner.go Uses limited reads for check input and propagates max bytes into lint.File.
internal/config/size.go Adds ParseSize for human-readable size strings.
internal/config/size_test.go Adds unit tests for ParseSize valid/invalid cases.
internal/config/merge.go Merges MaxInputSize and refactors default-copy logic via helper.
internal/config/load.go Caps config file reads to 1 MiB to avoid config-induced OOM.
internal/config/config.go Adds max-input-size config field.
docs/reference/cli.md Documents --max-input-size flag behavior and defaults.
cmd/mdsmith/metrics.go Uses limited metrics reads with default cap.
cmd/mdsmith/mergedriver.go Uses limited reads in merge-driver paths.
cmd/mdsmith/main.go Adds --max-input-size flag to check/fix, stdin limiting, and config/CLI resolution logic.
cmd/mdsmith/e2e_test.go Adds e2e tests for oversized files, unlimited mode, and CLI override vs config.
Comments suppressed due to low confidence (2)

internal/rules/include/rule.go:198

  • This diagnostic says the include file was "not found" for any read error, but ReadFSFileLimited can also fail with "file too large". This makes the error misleading. Consider changing the message to something like "cannot read include file" or branching on the error type so oversized files are reported accurately.
	data, err := lint.ReadFSFileLimited(readFS, readPath, f.MaxInputBytes)
	if err != nil {
		return "", []lint.Diagnostic{makeDiag(filePath, line,
			fmt.Sprintf("include file %q not found: %v", file, err))}
	}

internal/rules/requiredstructure/rule.go:507

  • The wrapper error text always reports "not found" on read failure, but ReadFileLimited can also return "file too large". That makes oversized schema include failures look like missing files. Consider changing this to a neutral "cannot read" message or handling the size-limit error separately so the user gets the correct cause.
	fragData, err := lint.ReadFileLimited(includedPath, maxBytes)
	if err != nil {
		return nil, "", fmt.Errorf(
			"schema include file %q not found: %w", includedPath, err)
	}

Comment thread internal/config/size.go
Comment thread internal/lint/limits.go
Comment thread internal/lint/limits.go Outdated
Comment thread cmd/mdsmith/main.go Outdated
Comment thread internal/config/load.go Outdated
Copilot AI review requested due to automatic review settings April 17, 2026 22:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.

Comment thread internal/lint/limits.go
Comment thread internal/engine/runner.go
Comment thread cmd/mdsmith/main.go Outdated
Comment thread cmd/mdsmith/metrics.go Outdated
@jeduden
jeduden force-pushed the claude/add-file-size-limit-Yrkil branch from 3c33633 to 12813ea Compare April 17, 2026 22:20
Copilot AI review requested due to automatic review settings April 17, 2026 22:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.

Comment thread cmd/mdsmith/main.go
Comment thread cmd/mdsmith/mergedriver.go Outdated
Comment thread internal/rules/catalog/rule.go Outdated
Copilot AI review requested due to automatic review settings April 17, 2026 22:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.

Comment thread cmd/mdsmith/metrics.go Outdated
Comment thread internal/config/merge.go Outdated
Comment thread docs/reference/cli.md Outdated
Comment thread cmd/mdsmith/main.go
Copilot AI review requested due to automatic review settings April 17, 2026 22:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.

Comment thread internal/rules/catalog/rule.go
Comment thread internal/rules/catalog/rule_test.go
Comment thread internal/rules/crossfilereferenceintegrity/rule.go
Copilot AI review requested due to automatic review settings April 17, 2026 23:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.

Comment thread docs/reference/cli.md Outdated
Comment thread docs/reference/cli.md
Copilot AI review requested due to automatic review settings April 19, 2026 14:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.

Comment thread internal/config/config_test.go Outdated
@jeduden
jeduden force-pushed the claude/add-file-size-limit-Yrkil branch from 98397dc to 70ea368 Compare April 19, 2026 14:50
@jeduden
jeduden requested a review from Copilot April 19, 2026 14:55
claude added 4 commits April 19, 2026 14:56
Guard every file-read path against OOM by enforcing a configurable
byte-size cap (default 2 MB). Adds --max-input-size CLI flag for
check/fix, max-input-size config key, and ReadFileLimited/
ReadFSFileLimited helpers. All ~15 read sites (runner, fixer, stdin,
include, catalog, cross-file-ref, required-structure, metrics, merge
driver, config) now use size-limited reads.

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ
- Add int64 overflow check in ParseSize before multiplication
- Remove duplicate doc comment block in readLimited
- Drop "reading %q:" prefix from readLimited error to avoid
  double-wrapping at call sites
- Make resolveMaxInputBytes return an error instead of silently
  falling back to the default on invalid input (exit code 2)
- Use Stat() in readLimitedConfig for accurate file size in
  error messages
- Change misleading "not found" to "cannot read" in include
  and schema-include error messages

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ
Covers new error paths added in the review-fix commit:
- Invalid --max-input-size flag value on check and fix
- Stdin exceeding max-input-size
- Stdin unlimited mode

Improves patch coverage for cmd/mdsmith/main.go.

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ
- Guard against int64 overflow in max+1 sentinel: treat
  math.MaxInt64 as unlimited in ReadFileLimited,
  ReadFSFileLimited, and readStdinLimited
- Set f.MaxInputBytes in Runner.RunSource so rules that read
  secondary files (e.g. schema includes) respect the limit on
  the stdin path
- Wire metrics rank to config max-input-size instead of
  hard-coded DefaultMaxInputBytes; add --max-input-size flag

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ
claude added 8 commits April 19, 2026 14:56
Cover new error paths added by wiring metrics rank to
config max-input-size: invalid flag value and oversized
file rejection from config.

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ
- Thread config max-input-size through query subcommand
  (added --config and --max-input-size flags; readFrontMatterRaw
  now takes a maxBytes parameter)
- Thread max-input-size through merge-driver paths, including
  setting Fixer.MaxInputBytes in fixFileInPlace so the fix
  pipeline respects the limit too
- Surface size-limit errors in catalog's readFrontMatter: return
  (map, error) and emit a diagnostic at the directive site
  instead of silently treating "file too large" as "no front matter"
- Extract mergeAndClean helper and parseQueryFlags helper to
  stay under funlen limits

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ
Covers the new error path added when wiring query subcommand
to config max-input-size resolution.

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ
Cover the new (entries, diagnostics) return signature:
- Size-limit failure emits a "cannot read front matter"
  diagnostic attached to the directive file+line
- Normal path returns entries with no diagnostics

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ
- metrics rank now loads config once and passes it into
  resolveRankFiles (was loading twice)
- copyConfig now copies all Config fields (Ignore,
  Overrides, FilesExplicit, ExplicitRules) to stay safe
  as future fields are added
- docs/reference/cli.md: clarify that exit code follows
  the usual precedence (1 for diagnostics, 2 for runtime-
  only errors) rather than always 2 for oversized files
- Update buildCatalogEntries comment to explain that
  Generate treats read errors as fatal to avoid silently
  producing incomplete catalogs
- TestReadFrontMatter_UnreadableFile now asserts err != nil
- Cross-file-reference rule distinguishes "cannot read"
  (e.g. file too large) from "not found" in diagnostics

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ
Cover the new unreadableTargetDiag function and verify
that resolveTargetFile's read closure honors f.MaxInputBytes,
surfacing "file too large" instead of "not found".

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ
Copilot noted the flag section was labeled "(check, fix)"
but the PR also wires the flag into query and metrics rank.
Add "Other Subcommand Flags" section and rewrite the
--max-input-size prose to describe per-command behavior:
runtime errors for check/fix, verbose-only for query,
fail-fast for metrics rank.

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ
- Config: merge preserves MaxInputSize, readLimitedConfig
  happy-path and oversized (maxConfigBytes+1) path,
  Load parses max-input-size from YAML
- Catalog: readFrontMatter returns error on oversized
  file, returns nil error on empty file
- Addresses Copilot review about misleading test name

https://claude.ai/code/session_01HHssHCn4zfbMv1praKoxGJ
@jeduden
jeduden force-pushed the claude/add-file-size-limit-Yrkil branch from 70ea368 to ece99e8 Compare April 19, 2026 15:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.

@jeduden jeduden added queue Add to a PR to enqueue it queue:active Applied automatically when a PR is in an active batch and removed queue Add to a PR to enqueue it labels Apr 19, 2026
@jeduden

jeduden commented Apr 19, 2026

Copy link
Copy Markdown
Owner Author

🟢 Merge Queue — picked up

This PR is in the queue and will be batched with other queue-labelled PRs.

Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run.

@jeduden

jeduden commented Apr 19, 2026

Copy link
Copy Markdown
Owner Author

🔵 Merge Queue — CI running

Merged into batch branch merge-queue/batch-130-1776615112. View CI run.

Next: No action needed — you'll be notified when CI completes.

@jeduden

jeduden commented Apr 19, 2026

Copy link
Copy Markdown
Owner Author

Merge Queue — merged

This PR landed on main via commit ccd272a. CI run that validated the merge.

Next: Done — nothing more to do here.

@jeduden jeduden removed the queue:active Applied automatically when a PR is in an active batch label Apr 19, 2026
@jeduden
jeduden merged commit ccd272a into main Apr 19, 2026
11 of 12 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.

3 participants