fix: 6 proactive bugs in wayfind_runner, map_step_runner, and settings deny globs - #403
Merged
Merged
Conversation
…s deny globs Bug 3 (wayfind_runner): _resolve_evidence_path now rejects absolute paths BEFORE constructing the candidate. On POSIX, Path(base) / "/abs/path" discards base, so an absolute path inside the map dir previously passed the containment check and was stored verbatim in state.json, breaking all markdown link targets. Bug 4 (wayfind_runner): add_ticket now validates that the from_fog entry has status "open" before accepting it. Previously a graduated or retired fog entry could be re-used as a from_fog source. Bug 6 (map_step_runner): _resolve_subtask_diff_base is now called INSIDE the try/except (OSError, TimeoutExpired) block in validate_mutation_boundary so subprocess OSErrors (e.g. git not on PATH) are caught and returned as status="error" rather than propagating as an uncaught exception. Bug 7 (map_step_runner): record_scope_baseline now writes the baseline JSON atomically via _write_json_file() (temp-file + replace) and renames the loop-shadowing variable `path` to `baseline_path` to avoid confusion. Bug 8 (map_step_runner): validate_mutation_boundary and record_scope_baseline now strip surrounding double-quotes from git porcelain v1 entries so files with spaces in their names (e.g. "file with spaces.txt") are correctly included in actual_set and the baseline, rather than being stored with literal quote characters. Security Gap 12 (settings.json): Write(**) and MultiEdit(**) deny patterns now mirror the Edit(**) patterns for .env*, *credentials*, and *secret* files. Claude Code treats Edit, Write, and MultiEdit as separate tools; the previous settings only denied Edit operations on these sensitive file globs. All changes implemented as jinja source edits and propagated via make render-templates. Tests added for all 6 fixes across test_wayfind_runner.py, test_map_step_runner.py, and test_settings_deny_globs.py. 4318 tests pass, lint clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DJ7HcC1k6tCEm5qSuGHcg1
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe change hardens Git path parsing and error handling, rejects unsafe evidence paths, restricts fog ticket sources to open entries, and expands sensitive-file deny rules for ChangesValidation and sensitive-file hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Proactive bug audit of recently changed files surfaced 6 actionable issues. All fixes are in the jinja source files and propagated via
make render-templates.Bug 3 —
wayfind_runner: absolute path inside map dir bypassed containment check_resolve_evidence_pathnow rejects absolute paths before constructing the candidate path. On POSIX,Path(base) / "/absolute/path"discardsbase, so an absolute path pointing inside the map dir previously passed the containment check and was stored verbatim instate.json, breaking all markdown link targets inmap.md,handoff.md, and ticket files.Bug 4 —
wayfind_runner: graduated fog entry accepted asfrom_fogadd_ticketnow validates that thefrom_fogfog entry hasstatus == "open"before accepting it. Previously a graduated or retired fog ID could be re-used as afrom_fogsource.Bug 6 —
map_step_runner: OSError from_resolve_subtask_diff_baseescapedvalidate_mutation_boundary_resolve_subtask_diff_basewas called outside thetry/except (OSError, TimeoutExpired)block, so subprocess failures (e.g. git not on PATH) propagated as uncaught exceptions instead of returning{"status": "error"}. Moved the call inside the try block.Bug 7 —
map_step_runner:record_scope_baselinewrote non-atomicallyBaseline JSON is now written atomically via
_write_json_file()(temp-file +replace()). Also renamed the loop-shadowing variablepathtobaseline_path.Bug 8 —
map_step_runner: porcelain-quoted paths with spaces lost from actual/baselinegit status --porcelain v1double-quotes paths containing spaces (e.g.?? "file with spaces.txt"). Bothvalidate_mutation_boundaryandrecord_scope_baselinenow strip surrounding quotes so such files are correctly included inactual_setand the scope baseline.Security Gap 12 —
settings.json:WriteandMultiEditdeny globs were missingClaude Code treats
Edit,Write, andMultiEditas distinct tools — anEdit(...)deny rule does not blockWrite(...). The previous settings had onlyEdit(...)patterns for.env*,*credentials*, and*secret*files. Added matchingWrite(...)andMultiEdit(...)patterns for all 11 sensitive globs (33 new deny rules total, up from 11 Edit-only).Changes
src/mapify_cli/templates_src/map/scripts/wayfind_runner.py.jinja— Bugs 3, 4src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja— Bugs 6, 7, 8src/mapify_cli/templates_src/settings.json.jinja— Security Gap 12make render-templates(.claude/,.map/scripts/,src/mapify_cli/templates/)tests/test_wayfind_runner.py— regression tests for Bugs 3, 4tests/test_map_step_runner.py— regression tests for Bugs 6, 8tests/test_settings_deny_globs.py— regression tests for Security Gap 12Test plan
uv run pytest tests/test_wayfind_runner.py::TestEvidencePathContainment— 9 passed (includes new Bug 3 test)uv run pytest tests/test_wayfind_runner.py::TestFogGraduation— passes (includes new Bug 4 test)uv run pytest tests/test_map_step_runner.py::TestValidateMutationBoundary— 18 passed (includes new Bug 6, 8 tests)uv run pytest tests/test_settings_deny_globs.py— 148 passed (includes new Write/MultiEdit tests)uv run pytest— 4318 passed, 4 skippedmake lint— ruff, mypy, pyright all cleanGenerated by Claude Code
Summary by CodeRabbit
Security
Bug Fixes
Reliability