fix: wrap malformed info.toml failures in ManifestError - #54
Conversation
load() previously let several failure modes escape as raw exceptions instead of ManifestError, so the CLI's top-level ManifestError handler (which prints a friendly `pythonlings: ...` message and exits 2) never caught them and a full traceback leaked to the user instead: - invalid TOML syntax raised tomllib.TOMLDecodeError - a missing/wrongly-typed `name` or `path` field raised KeyError or a downstream TypeError - an absolute or `..`-traversal path was not explicitly rejected and could reach outside the exercises/ tree All three now raise a contextual ManifestError before any unsafe filesystem access, matching the existing behavior for the already-handled cases (missing info.toml, bad format_version, empty exercises list, duplicate names, missing exercise/check files). Adds unit tests for each new rejection path plus two CLI integration tests asserting exit code 2 with no traceback text in stderr.
📝 WalkthroughWalkthroughThe manifest loader now reports malformed TOML, invalid exercise fields, and unsafe exercise or check paths as ChangesManifest safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ManifestLoader
participant Filesystem
CLI->>ManifestLoader: load info.toml
ManifestLoader->>Filesystem: parse and resolve manifest paths
Filesystem-->>ManifestLoader: parsed data or path error
ManifestLoader-->>CLI: ManifestError
CLI-->>CLI: print pythonlings: error and exit 2
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pythonlings/core/manifest.py`:
- Around line 93-102: Update manifest path validation around rel_path to resolve
each exercise and derived checks candidate after the existing lexical checks,
then require the resolved paths to remain relative to the resolved workspace
exercises/ and checks/ directories respectively before calling exists() or
downstream access. Reject symlink escapes with ManifestError while preserving
valid in-tree paths.
In `@tests/unit/test_manifest.py`:
- Around line 215-218: Update the pytest.raises match pattern in
test_load_rejects_invalid_toml_syntax to use a raw regex with the dot in
“info.toml” escaped, preserving the existing filename assertion while satisfying
Ruff RUF043.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9f7aa2a-4c7a-4d4f-a14f-43304a399b85
📒 Files selected for processing (3)
pythonlings/core/manifest.pytests/integration/test_cli_verify.pytests/unit/test_manifest.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
tests/integration/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep integration tests in
tests/integration/directory
Files:
tests/integration/test_cli_verify.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use Python 3.11+ idioms in all Python code
Use 4-space indentation in all Python code
Prefer small, typed functions where practical in Python code
**/*.py: Guard newer-stdlib usage withrequires-python = ">=3.9"and use fallbacks (e.g.tomllibfalls back totomli) in modules likecore/manifest.py
Includefrom __future__ import annotationsat the top of Python modules
Files:
tests/integration/test_cli_verify.pytests/unit/test_manifest.pypythonlings/core/manifest.py
tests/**/*test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Name test files as
test_<behavior>.py
Files:
tests/integration/test_cli_verify.pytests/unit/test_manifest.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Name test functions astest_<expected_behavior>
Use pytest for all tests with pytest-asyncio in auto mode for async tests
Files:
tests/integration/test_cli_verify.pytests/unit/test_manifest.py
**/test_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Name tests as
test_<behavior>.pyortest_<expected_behavior>(e.g.,test_runner.py,test_state.py)
Files:
tests/integration/test_cli_verify.pytests/unit/test_manifest.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep unit tests in
tests/unit/directory
Files:
tests/unit/test_manifest.py
pythonlings/core/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Core exercise loading, workspace setup, state, reset, solutions, and runner logic must live in
pythonlings/core/directoryKeep UI behavior in
screens/andwidgets/modules; keep behavior logic incore/modules—do not import UI in core
Files:
pythonlings/core/manifest.py
pythonlings/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep UI behavior in
screens/orwidgets/directories; keep filesystem, manifest, reset, and runner behavior incore/
Files:
pythonlings/core/manifest.py
🪛 Ruff (0.16.1)
tests/unit/test_manifest.py
[warning] 217-217: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
🔇 Additional comments (3)
pythonlings/core/manifest.py (1)
58-91: LGTM!tests/unit/test_manifest.py (1)
221-281: LGTM!tests/integration/test_cli_verify.py (1)
69-90: LGTM!
The lexical path checks (no absolute path, no '..' components, starts with exercises/) accept exercises/link/file.py even when 'link' is a symlink pointing outside the workspace -- CodeRabbit correctly flagged that a value can look clean lexically and still resolve elsewhere. Now resolve() both the exercise path and the derived check path and confirm they stay within the resolved exercises/ and checks/ directories before touching the filesystem further, raising ManifestError otherwise. Added a regression test that creates a real symlink escaping the workspace and confirms it raises; reverting the fix makes this test fail (with a different, wrong error), proving it's a real check. Also fixed the RUF043 warning on the new test_load_rejects_invalid_toml_syntax test: match="info.toml" treated '.' as a regex wildcard; changed to the raw/escaped match=r"info\.toml". tests/unit/test_manifest.py + tests/integration/test_cli_verify.py: 31 passed. ruff check: clean (the one PLW1510 warning ruff reports is pre-existing in test_cli_verify.py's _run() helper, untouched by this diff, as already noted in the original PR).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pythonlings/core/manifest.py (1)
58-61: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWrap
info.tomlreads and decodes intoManifestError.Move the
info_path.open("rb")call inside thetry, then catchOSErrorfor read failures. Also catchUnicodeDecodeErrorbeforetomllib.TOMLDecodeErrorand convert both toManifestErrorwhile preserving the original exception as the cause.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pythonlings/core/manifest.py` around lines 58 - 61, Update the manifest-loading method around info_path.open and tomllib.load: move the binary open call inside the existing try, catch OSError and UnicodeDecodeError, and convert each to ManifestError while chaining the original exception. Keep the tomllib.TOMLDecodeError conversion and ensure UnicodeDecodeError is handled before the TOML decode exception.
🧹 Nitpick comments (1)
tests/unit/test_manifest.py (1)
284-308: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for the derived check-path escape.
This test fails during exercise-path validation. It does not execute the derived check-path containment check in
pythonlings/core/manifest.pylines 126-134. Add a test with a regular exercise file and achecks/<topic>symlink that resolves outside the workspace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_manifest.py` around lines 284 - 308, Add a separate manifest-loading test covering derived check-path containment, using a regular exercise file and a checks/<topic> symlink targeting a directory outside the workspace. Ensure the exercise path passes validation, the derived check path resolves through the symlink, and load raises ManifestError with the existing “escapes the workspace” message from the check-path validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pythonlings/core/manifest.py`:
- Around line 58-61: Update the manifest-loading method around info_path.open
and tomllib.load: move the binary open call inside the existing try, catch
OSError and UnicodeDecodeError, and convert each to ManifestError while chaining
the original exception. Keep the tomllib.TOMLDecodeError conversion and ensure
UnicodeDecodeError is handled before the TOML decode exception.
---
Nitpick comments:
In `@tests/unit/test_manifest.py`:
- Around line 284-308: Add a separate manifest-loading test covering derived
check-path containment, using a regular exercise file and a checks/<topic>
symlink targeting a directory outside the workspace. Ensure the exercise path
passes validation, the derived check path resolves through the symlink, and load
raises ManifestError with the existing “escapes the workspace” message from the
check-path validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3258cb78-771c-416d-9659-dc89b53944d4
📒 Files selected for processing (2)
pythonlings/core/manifest.pytests/unit/test_manifest.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep unit tests in
tests/unit/directory
Files:
tests/unit/test_manifest.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use Python 3.11+ idioms in all Python code
Use 4-space indentation in all Python code
Prefer small, typed functions where practical in Python code
**/*.py: Guard newer-stdlib usage withrequires-python = ">=3.9"and use fallbacks (e.g.tomllibfalls back totomli) in modules likecore/manifest.py
Includefrom __future__ import annotationsat the top of Python modules
Files:
tests/unit/test_manifest.pypythonlings/core/manifest.py
tests/**/*test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Name test files as
test_<behavior>.py
Files:
tests/unit/test_manifest.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Name test functions astest_<expected_behavior>
Use pytest for all tests with pytest-asyncio in auto mode for async tests
Files:
tests/unit/test_manifest.py
**/test_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Name tests as
test_<behavior>.pyortest_<expected_behavior>(e.g.,test_runner.py,test_state.py)
Files:
tests/unit/test_manifest.py
pythonlings/core/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Core exercise loading, workspace setup, state, reset, solutions, and runner logic must live in
pythonlings/core/directoryKeep UI behavior in
screens/andwidgets/modules; keep behavior logic incore/modules—do not import UI in core
Files:
pythonlings/core/manifest.py
pythonlings/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep UI behavior in
screens/orwidgets/directories; keep filesystem, manifest, reset, and runner behavior incore/
Files:
pythonlings/core/manifest.py
🔇 Additional comments (3)
pythonlings/core/manifest.py (2)
68-91: LGTM!Also applies to: 105-134
58-60: 🩺 Stability & AvailabilityNo change needed. The project declares
requires-python = ">=3.9", andpythonlings/core/manifest.pyuses atomllib/tomlifallback.tests/unit/test_manifest.py (1)
215-218: LGTM!
abhiksark
left a comment
abhiksark
left a comment
There was a problem hiding this comment.
Welcome to Pythonlings, @agu2347, and thank you for your first contribution! The contextual field and path validation is thoughtful, and the resolved symlink checks are a useful safety improvement. I ran the full suite on the current head (python -m pytest -q: 156 passed) and the passing-curriculum verification (both fixtures passed). One error boundary still needs attention before merge: opening or decoding info.toml can still raise raw exceptions. On this head, invalid UTF-8 raises UnicodeDecodeError, and an unreadable manifest path raises OSError/IsADirectoryError, so the CLI can still show a traceback. Please move the file open into the guarded block, translate OSError and UnicodeDecodeError to contextual ManifestError values, and add focused regressions. Please also record the repository-required full-suite and curriculum-verification commands in the PR description after the update. Once that is addressed, this should be straightforward to re-review.
abhiksark
left a comment
abhiksark
left a comment
There was a problem hiding this comment.
Approved at 5d8d0c9. The manifest loader now translates read, decode, path-resolution, and field-validation failures into contextual ManifestError values. The added regressions cover invalid UTF-8, a directory-valued info.toml, directory-valued exercise and check paths, inner and top-level symlink escapes, non-string fields, exit status 2, and absence of tracebacks. Local validation passed with 172 tests and both passing-curriculum fixtures. Fresh hosted CI passed on Python 3.11, 3.12, and 3.13, including package build, wheel installation, and installed CLI smoke flows.
2 participants
Closes #44.
Summary
info.tomlread failures into contextualManifestErrorvaluesexercises/andchecks/symlinks that resolve outside the workspaceRoot cause
The CLI already handled
ManifestError, but several manifest parsing, filesystem, and type failures bypassed that boundary. Raw exceptions therefore reached users as tracebacks.Behavior
Valid manifests and the manifest schema are unchanged. Invalid, unreadable, or unsafe manifests now report a
pythonlings:error and exit with status 2.Tests
Coverage includes malformed TOML, invalid UTF-8, a directory-valued
info.toml, missing or invalid fields, non-string text fields, absolute and traversal paths, directory-valued exercise and check paths, inner and top-level symlink escapes, path-resolution failures, exit status 2, and absence of tracebacks.Validation
Validated at head
5d8d0c9:python -m pytest tests/unit/test_manifest.py tests/integration/test_cli_verify.py -q->47 passedpython -m ruff check pythonlings/core/manifest.py tests/unit/test_manifest.py tests/integration/test_cli_verify.py-> passedpython -m pytest -q->172 passedpythonlings --root tests/fixtures/passing_curriculum verify->passing1,passing2git diff dev...HEAD --check-> passedScreenshots are not applicable because this change affects CLI error handling only.