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!
1 participant
Closes #44.
manifest.load()already had aManifestErrortype and the CLI already had a top-level handler incli.py::main()that catchesManifestErrorspecifically, prints a friendlypythonlings: ...message, and exits 2 — but several failure modes never went throughManifestError, so they fell through to the genericraiseand surfaced a raw traceback instead:tomllib.TOMLDecodeErrordirectly.nameorpathfield in an[[exercises]]entry raisedKeyError(viaentry["name"]/entry["path"]).name = 123) would eventually hit aTypeErrordeeper in the loop./etc/passwd) or a path containing..components (e.g.exercises/../../etc/passwd) was not explicitly rejected —path.parts[0] != "exercises"happens to reject absolute paths on POSIX as a side effect, but a..-traversal path starting withexercises/slipped past that check entirely and could reach outside the workspace once resolved.This PR wraps all of these in
ManifestErrorwith contextual messages, checked before any filesystem access for the path-safety cases, so they're handled the same way as the failure modes that already worked (missinginfo.toml, badformat_version, empty exercises list, duplicate names, missing exercise/check files).No schema changes, no change to valid-manifest behavior — confirmed via the existing fixture-based tests plus new ones below.
Tests added:
tests/unit/test_manifest.py: invalid TOML syntax, missingname, wrong-typedname, missingpath, absolute path, traversal path (6 new tests, all raisingManifestErrorwith a message matching what the CLI already expects).tests/integration/test_cli_verify.py: malformed TOML and traversal-path cases both asserted to exit 2 withpythonlings:-prefixed stderr and noTracebacktext (2 new tests).Verification:
30 passed (18 pre-existing + 6 new unit tests + 2 new integration tests, plus the 4 pre-existing integration tests in that file). Also ran
ruff checkon the three changed files — clean except one pre-existing, unrelated warning intest_cli_verify.py's already-there_run()helper (not touched by this diff).Summary by CodeRabbit
Bug Fixes
Tests