Normalize build-time paths embedded into fs.c and main.c - #66
Merged
Conversation
kompo-vfs compares embedded paths as raw bytes, so any path that reaches fs.c or main.c unnormalized silently breaks lookups rather than failing loudly. Two values were escaping the File.expand_path that PATHS entries already go through. - WD[]: WorkDir#run assigned the cached work_dir from metadata.json raw, even though valid_tmpdir_path? validated the expanded form. kompo-vfs uses WD as a byte prefix of every path, so a trailing slash there makes every lookup miss and disables the VFS entirely. Normalize on read, and again in MakeFsC where WD is emitted (that prefix also drives .kompoignore matching). - main.c entrypoint: CopyProjectFiles built entrypoint_path with a raw File.join of the CLI value, so `-e lib/../main.rb` embedded a ".." component. kompo-vfs derives WORKING_DIR from Path::parent() of that string and parent() does not collapse "..". Normalize at the source so it stays byte-identical to the matching PATHS entry; MakeMainC keeps sanitizing rather than normalizing, so junk input is still escaped instead of raising. Also deduplicate on the embedded path instead of the source path in MakeFsC#add_file. The ruby-install-dir replacement can map two source paths onto one embedded path, and checking beforehand would emit that path twice in PATHS, orphaning the earlier bytes in FILES. Document that PATHS emission order (per-directory, DFS pre-order with sorted siblings) is a contract kompo-vfs relies on for locality, not an implementation detail to be sorted globally. The fs.c format is unchanged: PATHS separators and trailing NUL, FILES concatenation, FILES_SIZES as N+1 cumulative offsets, emission order, and the COMPRESSED_SIZES dummy are all byte-identical before and after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012bpbZACT4YWiN8ucX6ZvyV
Follow-up cleanup on the path-normalization change. Same behavior, less
duplication, and the invariant is enforced instead of described.
- WorkDir gains a private canonical_path that both branches of #run funnel
through, so the cached and freshly-created paths are normalized to the
same strength. Previously the mktmpdir branch resolved symlinks while
the cache branch only expanded lexically. Normalizing now happens after
valid_tmpdir_path?, which removes the start_with?("/") guard that only
existed to preserve that check's relative-path rejection, and the double
File.expand_path it caused.
- MakeFsC no longer re-expands WorkDir.path. It was the only one of 20+
WorkDir.path call sites to do so, and it is redundant now that the
source guarantees the shape. build_template_context asserts the
invariant instead: an unnormalized work_dir would otherwise ship a
binary whose VFS silently matches nothing, so failing the build is
strictly better than quietly repairing it.
- Test coverage moves to where the behavior lives: work_dir_test.rb now
covers an unnormalized metadata.json (the real reported input), and the
MakeFsC tests cover what MakeFsC owns - that WD[] is emitted as a prefix
of every PATHS entry, and that a bad work_dir is rejected.
- decode_embedded_paths, decode_wd, and the open-coded COMPRESSED_FILES
decode share one decode_byte_array helper, so the fs.c array literal
format is described in one place.
- Trim comments that were stated two or three times over, and restore the
"Prune certain directories" label that a comment rewrite had dropped.
fs.c output is byte-identical to before the original change: declarations,
PATHS separators and trailing NUL, emission order, FILES_SIZES as N+1
cumulative offsets, and the COMPRESSED_SIZES dummy all verified unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012bpbZACT4YWiN8ucX6ZvyV
Comments should carry what the code cannot: the kompo-vfs contract, the reason an ordering matters, why something is deliberately not done. Removed the mechanical narration - "entries land in add_file call order", "directories are never emitted as PATHS entries", "dedup after the rewrite, not before", "the single place work_dir is canonicalized" - and the doc comments on decode_byte_array/decode_embedded_paths/decode_wd, whose names already say it. Restored the original "Prune certain directories" label a previous rewrite had replaced. What is left states only non-obvious facts: that kompo-vfs assigns node IDs in PATHS order, that Path::parent() does not collapse "..", that File.expand_path raises on the junk c_string_escape exists to absorb. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012bpbZACT4YWiN8ucX6ZvyV
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe change normalizes entrypoint and work-directory paths, validates canonical paths during C template generation, and deduplicates embedded paths after installation-directory mapping. Tests cover these behaviors and update byte-array decoding helpers. ChangesPath handling and generated filesystem paths
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![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 `@lib/kompo/tasks/make_fs_c.rb`:
- Around line 184-199: Update the embedded_path rewrite condition around
`@current_ruby_install_dir` so replacement occurs only when path equals the
install directory or begins with it followed by File::SEPARATOR; preserve
unchanged paths for sibling directory names. Add a regression case covering a
sibling install-directory path and verify it is not rewritten.
In `@lib/kompo/tasks/work_dir.rb`:
- Around line 107-109: Update canonical_path in lib/kompo/tasks/work_dir.rb at
lines 107-109 to resolve the deepest existing ancestor with File.realpath,
append any missing components, and revalidate the resolved result against
File.realpath(Dir.tmpdir) before accepting or creating it; update
lib/kompo/tasks/make_fs_c.rb at lines 221-223 to compare the existing work
directory using File.realpath(`@work_dir`) instead of File.expand_path(`@work_dir`);
add regression coverage for cached and mocked MakeFsC work directories located
beneath a temp-directory symlink.
🪄 Autofix (Beta)
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: a986606d-637d-49e7-af4b-de9769139835
📒 Files selected for processing (7)
lib/kompo/tasks/copy_project_files.rblib/kompo/tasks/make_fs_c.rblib/kompo/tasks/make_main_c.rblib/kompo/tasks/work_dir.rbtest/tasks/copy_files_test.rbtest/tasks/make_c_test.rbtest/tasks/work_dir_test.rb
A cached work_dir of "/tmp/link/work" satisfied valid_tmpdir_path? no matter where link pointed, because the check only ever saw the lexical path. When the leaf did not exist yet, canonical_path had no path to resolve and returned the alias unchanged, so FileUtils.mkdir_p created the work directory - and its marker file - outside the system temp directory. Confirmed by running the new test against the previous commit: it wrote .kompo_work_dir_marker into the repository root. - Canonicalize before validating, so the tmpdir check sees where the path actually lands. Absolute-only, so a relative path is still rejected rather than resolved against the cwd. - canonical_path now resolves the deepest existing ancestor when the path itself does not exist, which is the case the symlink slipped through. - MakeFsC asserts against File.realpath rather than File.expand_path, so an aliased work_dir is caught too - expand_path cannot see a symlink. The unbounded start_with? in add_file's install-dir rewrite was raised in review as a sibling-directory hazard. It is real as a pattern but unreachable: InstallRuby sets original_ruby_install_dir to ruby_install_dir on every code path, so the branch never runs. Left alone rather than hardening dead code. fs.c output remains byte-identical: declarations, PATHS separators and trailing NUL, emission order, FILES_SIZES as N+1 cumulative offsets, COMPRESSED_SIZES. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012bpbZACT4YWiN8ucX6ZvyV
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/tasks/work_dir_test.rb`:
- Around line 56-75: Update
test_work_dir_rejects_cached_work_dir_behind_an_escaping_symlink to use a unique
test-owned symlink target guaranteed to be outside Dir.tmpdir, and ensure that
target is removed during cleanup even if assertions fail. Replace the raw
path.start_with? assertion with a separator-aware containment check based on the
resolved Dir.tmpdir path, preserving the rejection and no-creation assertions.
🪄 Autofix (Beta)
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: b9f3b6e3-e1dc-43d9-8d6e-8b51baae2b73
📒 Files selected for processing (4)
lib/kompo/tasks/make_fs_c.rblib/kompo/tasks/work_dir.rbtest/tasks/make_c_test.rbtest/tasks/work_dir_test.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/kompo/tasks/make_fs_c.rb
- lib/kompo/tasks/work_dir.rb
- test/tasks/make_c_test.rb
The escape target was the repository checkout, reached via File.expand_path
("../..", __dir__). That is only outside Dir.tmpdir by coincidence - a checkout
below /tmp would make the cached path legitimately valid and fail the test for
the wrong reason. It also meant a regression wrote into the working tree: running
this test against the pre-fix commit created kompo_escape_probe/ with a marker
file in the repository root.
Point TMPDIR at a directory inside the per-test tmpdir instead, so both ends of
the escape live under with_tmpdir. The target stays writable, so a regression
still really creates the directory and is still caught, but the fallout is
cleaned up with the rest of the fixture even when an assertion fails.
Also make the containment assertion separator-aware, so a sibling like
/tmp_evil cannot satisfy it.
Verified both directions: passes on HEAD, fails on HEAD~1 at the same
"directory was created outside the temp dir" assertion, and leaves the working
tree clean either way.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012bpbZACT4YWiN8ucX6ZvyV
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.
kompo-vfs compares embedded paths as raw bytes, so any path that reaches
fs.c or main.c unnormalized silently breaks lookups rather than failing
loudly. Two values were escaping the File.expand_path that PATHS entries
already go through.
WD[]: WorkDir#run assigned the cached work_dir from metadata.json raw,
even though valid_tmpdir_path? validated the expanded form. kompo-vfs
uses WD as a byte prefix of every path, so a trailing slash there makes
every lookup miss and disables the VFS entirely. Normalize on read, and
again in MakeFsC where WD is emitted (that prefix also drives
.kompoignore matching).
main.c entrypoint: CopyProjectFiles built entrypoint_path with a raw
File.join of the CLI value, so
-e lib/../main.rbembedded a ".."component. kompo-vfs derives WORKING_DIR from Path::parent() of that
string and parent() does not collapse "..". Normalize at the source so
it stays byte-identical to the matching PATHS entry; MakeMainC keeps
sanitizing rather than normalizing, so junk input is still escaped
instead of raising.
Also deduplicate on the embedded path instead of the source path in
MakeFsC#add_file. The ruby-install-dir replacement can map two source
paths onto one embedded path, and checking beforehand would emit that
path twice in PATHS, orphaning the earlier bytes in FILES.
Document that PATHS emission order (per-directory, DFS pre-order with
sorted siblings) is a contract kompo-vfs relies on for locality, not an
implementation detail to be sorted globally.
The fs.c format is unchanged: PATHS separators and trailing NUL, FILES
concatenation, FILES_SIZES as N+1 cumulative offsets, emission order, and
the COMPRESSED_SIZES dummy are all byte-identical before and after.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_012bpbZACT4YWiN8ucX6ZvyV
Summary by CodeRabbit
Bug Fixes
Tests