Skip to content

fix(policy): handle OSError from is_file() on macOS PATH_MAX limit#860

Open
chaobo8484 wants to merge 10 commits intomicrosoft:mainfrom
chaobo8484:fix/issue-848-macos-path-max-crash
Open

fix(policy): handle OSError from is_file() on macOS PATH_MAX limit#860
chaobo8484 wants to merge 10 commits intomicrosoft:mainfrom
chaobo8484:fix/issue-848-macos-path-max-crash

Conversation

@chaobo8484
Copy link
Copy Markdown

@chaobo8484 chaobo8484 commented Apr 23, 2026

When load_policy() receives a YAML string that exceeds ~1023 bytes, the is_file() syscall fails on macOS (PATH_MAX ≈ 1024) with OSError [Errno 63] File name too long. Wrap the call in try/except to gracefully fall back to treating the input as a YAML string.

Fixes #848

Description

Brief description of changes and motivation.

Fixes # (issue)

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Maintenance / refactor

Testing

  • Tested locally
  • All existing tests pass
  • Added tests for new functionality (if applicable)

When load_policy() receives a YAML string that exceeds ~1023 bytes,
the is_file() syscall fails on macOS (PATH_MAX ≈ 1024) with
OSError [Errno 63] File name too long. Wrap the call in try/except
to gracefully fall back to treating the input as a YAML string.

Fixes microsoft#848
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes load_policy() to avoid crashing on macOS when given a long YAML string that triggers Path.is_file() to raise OSError: [Errno 63] File name too long, preserving the intended "path or YAML content" input behavior.

Changes:

  • Wraps the Path.is_file() probe in try/except OSError and falls back to treating the input as YAML content when the probe fails.
Show a summary per file
File Description
src/apm_cli/policy/parser.py Adds defensive handling around Path.is_file() so long YAML inputs do not raise on macOS PATH_MAX limits.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

Comment thread src/apm_cli/policy/parser.py Outdated
Comment thread src/apm_cli/policy/parser.py Outdated
Copy link
Copy Markdown
Collaborator

@danielmeppiel danielmeppiel left a comment

Choose a reason for hiding this comment

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

Per Copilot reviewer

danielmeppiel added a commit that referenced this pull request Apr 23, 2026
* docs(ci): document merge-gate architecture and ruleset context gotcha

- Document merge-gate.yml as the single-authority PR-time aggregator
- Mark ci-integration-pr-stub.yml as DEPRECATED (slated for deletion)
- Renumber workflow list (now 6 entries, was misnumbered with two #3s)
- New section: Branch Protection & Required Checks
  - Ruleset 'context' field MUST match check-run name ('gate'), not the
    UI display string ('Merge Gate / gate'). Storing the display string
    causes permanent 'Expected - Waiting for status to be reported' that
    blocked PR #860 today
  - Adding new required checks goes through EXPECTED_CHECKS in
    merge-gate.yml, not the ruleset

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* chore(ci): remove deprecated PR-time stub workflow

The four PR stubs (Build/Smoke/Integration/Release Validation - Linux)
were a holdover from the pre-merge-gate model where branch protection
required each Tier 2 check name directly. After #867, branch protection
requires only the single 'gate' check from merge-gate.yml, so the stubs
are dead weight that fire on every PR for no reason.

Changes:
- Delete .github/workflows/ci-integration-pr-stub.yml
- Reduce EXPECTED_CHECKS in merge-gate.yml to just 'Build & Test (Linux)'
  (the only PR-time check we still emit)
- Update merge-gate.yml + ci-integration.yml header comments
- Update cicd.instructions.md (drop DEPRECATED entry, renumber to 5
  workflows)
- Drop stale CODEOWNERS reference to the deleted file
- CHANGELOG entry under [Unreleased] > Removed

Stacked on #874 (docs).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@chaobo8484
Copy link
Copy Markdown
Author

@danielmeppiel I have made adjustments according to Copilot's suggestion.Have a good day!

@danielmeppiel danielmeppiel added the panel-review Trigger the apm-review-panel gh-aw workflow label Apr 23, 2026
@github-actions
Copy link
Copy Markdown

DevX UX Review — PR #860

Scope: CLI error ergonomics & silent-failure risk. No command surface / flag / help-text changes in this PR — review is limited to internal load_policy() behaviour and its downstream user-visible effects.


Findings

  1. ✅ Error recovery is solid. When ENAMETOOLONG fires, the code falls back to treating the input as YAML. If that YAML is malformed, yaml.safe_load raises YAMLError, which is wrapped into a PolicyValidationError with a human-readable message. The user still gets a clear "YAML parse error: ..." diagnostic — no silent swallowing of bad input.

  2. _looks_like_yaml_content() is a reasonable fast-path. The heuristics (newlines, ---, - , {, [, : ) match what every YAML-literate developer would expect. This avoids the filesystem probe entirely for the vast majority of inline YAML, which is the right default. No UX concern here.

  3. ⚠️ Minor: a valid-looking file path that doesn't exist is silently parsed as YAML. This is pre-existing behaviour, not introduced by this PR, but worth noting: load_policy("apm-policy.yml") where the file is missing will try to YAML-parse the string "apm-policy.yml" and return {name: apm-policy.yml} (bare YAML scalar coerced to a mapping? — actually it would hit "Policy must be a YAML mapping"). So the failure mode is noisy enough. No new risk from this PR.

  4. ✅ No user-facing output changes. apm audit --policy <path> UX is unaffected — callers in discovery.py already pass either a Path object or pre-read content strings. The fix only matters when raw YAML strings happen to exceed PATH_MAX, which is a programmatic edge case (not a CLI-surface issue).

  5. ✅ Test coverage is appropriate. The new test constructs a >1 KiB YAML payload and asserts it parses without OSError. Good enough for a defensive fix — no over-engineering.

Verdict: Ship it. Clean defensive fix, no UX regressions, no silent failures introduced. 👍

Generated by PR Review Panel for issue #860 · ● 4M ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

panel-review Trigger the apm-review-panel gh-aw workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

policy: load_policy() raises OSError on macOS when policy YAML > 1023 bytes

3 participants