Skip to content

fix(cli): report malformed policy files cleanly - #129

Open
rioyu123 wants to merge 4 commits into
NVIDIA:mainfrom
rioyu123:codex/fix-malformed-policy-yaml
Open

fix(cli): report malformed policy files cleanly#129
rioyu123 wants to merge 4 commits into
NVIDIA:mainfrom
rioyu123:codex/fix-malformed-policy-yaml

Conversation

@rioyu123

@rioyu123 rioyu123 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Malformed, non-UTF-8, deeply nested, or unreadable bundled and custom policy files now follow a path-specific error contract. The CLI converts those failures into concise Click errors instead of exposing raw parser, decoder, recursion, or I/O exceptions.

The change uses one private loader helper for both policy paths, preserves the original exception as the cause, keeps FileNotFoundError behavior unchanged, and does not fall back to a default policy after an explicit invalid --policy input.

Fixes #128.

Verification

  • I am familiar with the Contributing Guidelines
  • Added or updated focused tests
  • Updated documentation for user-visible changes
  • Ran repository-wide Ruff lint
  • Full local pytest suite passes on Windows
  • Ran the package build
  • Did not add credentials, private datasets, or proprietary benchmark content

Focused verification:

  • 14 policy, golden-behavior, and CLI regression tests passed
  • repository-wide Ruff check passed
  • wheel build completed successfully
  • a broader 57-test policy/CLI selection had 56 passes and one pre-existing Windows-only path-separator assertion (\\tmp\\run versus /tmp/run)
  • the earlier full Windows suite reached 45% with unrelated POSIX path/symlink failures, then stalled in Harbor tests at about 3.3 GB with no CPU progress and was interrupted; CI is the authoritative Linux run

Release Impact

  • No user-visible release note needed
  • Updated CHANGELOG.md

Signed-off-by: Rio Yu <52408936+rioyu123@users.noreply.github.com>
Signed-off-by: Rio Yu <52408936+rioyu123@users.noreply.github.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The policy loader now consistently normalizes malformed YAML, invalid UTF-8, parser recursion failures, and read errors while preserving FileNotFoundError behavior and exception causes. I verified concise path-specific CLI failures, 57 focused policy/CLI/golden tests, Ruff, diff checks, and green CI. The implementation looks good. GitHub currently reports a CHANGELOG.md conflict with main; that must still be resolved and the resulting head should complete CI before merge.

@rng1995

rng1995 commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

@rioyu123 - Please resolve merge conflicts.

@rng1995
rng1995 enabled auto-merge (squash) September 12, 2026 04:55
Signed-off-by: Rio Yu <52408936+rioyu123@users.noreply.github.com>
auto-merge was automatically disabled September 12, 2026 05:30

Head branch was pushed to by a user without write access

@rioyu123

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I've merged main (7e189c6) and resolved the changelog conflict, preserving both the upstream entries and this PR's policy-error entry. The policy implementation and regression tests are unchanged.

With the locked dependencies on Linux, the 57 focused tests, lint, and package build pass. The full suite reports 6,008 passed, 51 skipped, and 5 failures; all five also reproduce on unmodified main on this machine (two runtime-discovery tests, an occupied-port test, and two HTML-parser cases). I've left those outside this PR. The updated head still needs hosted CI to pass before merging.

@rng1995
rng1995 enabled auto-merge (squash) September 12, 2026 07:30
@rng1995

rng1995 commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Please fix the CI Test that is failing @rioyu123

Signed-off-by: Rio Yu <52408936+rioyu123@users.noreply.github.com>
auto-merge was automatically disabled September 12, 2026 15:06

Head branch was pushed to by a user without write access

@rioyu123

Copy link
Copy Markdown
Contributor Author

Thanks for flagging this. The test's 50 ms header timeout was still active for the final health check, and it relied on a fixed sleep for earlier connections to finish. It now waits for those connections and workers to finish, then restores the normal timeout before checking health. Bridge production timeouts and the worker limit are unchanged.

On Python 3.13.15, a controlled 150 ms delay before parsing the health request makes both pre-merge main and the previous PR head fail on that same final health request; the updated test passes that probe and 20 normal repetitions. Disabling header expiry still makes the updated test fail. The updated test also passes that probe on Python 3.12.

I also merged current main (d979722) and preserved both changelog entries. Lint and build pass. The full Python 3.13.15 suite reports 6,017 passed, 51 skipped, and two environment-specific failures (runtime discovery and occupied port 18080); both also reproduce on unmodified current main on this machine. The failing hosted-CI test passes locally; the new head still needs hosted CI to confirm.

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.

[BUG]: Malformed policy YAML bypasses the CLI error wrapper

2 participants