Skip to content

Resolve relative local collection sources - #3124

Closed
OskarEichler wants to merge 1 commit into
ruby:masterfrom
OskarEichler:codex/relative-local-source
Closed

Resolve relative local collection sources#3124
OskarEichler wants to merge 1 commit into
ruby:masterfrom
OskarEichler:codex/relative-local-source

Conversation

@OskarEichler

Copy link
Copy Markdown
Contributor

Summary: resolve local collection source paths relative to the configuration directory and expand the result, instead of anchoring them to the process working directory.

Verification: focused baseline/fixed reproduction, combined RBS 4.2.0 consumer models, and RuboCop (738 files, zero offenses). No tests are added in this PR.

Compatibility: no public API removal or dependency/version change.

@pocke

pocke commented Sep 7, 2026

Copy link
Copy Markdown
Member

Thank you for the patch. I am closing this in favour of #3146, which takes the comment deletion on its own.

You were right to question the TODO: it has been stale since 2023. I dug through the history of #854 and wrote it up in #3146. The short version is that the comment arrived in a 2022-02-11 force-push, when the constructor really did take the path as written, and it was answered on 2023-02-08 by the commit that added the base_directory: keyword. Only the code was updated. Because #854 was squashed into a single commit, the leftover is invisible from git log -L on the file, which is presumably why it survived.

The reason I am splitting it is the other half of this PR. (base_directory / path).expand_path is a real change with its own behaviour, and it deserves to be judged separately:

  • It only does anything when base_directory is relative. When it is absolute, Pathname#+ has already collapsed any .. lexically, so expand_path is a no-op. I checked this with a path: ../shared_sig source: the symlink target is byte-identical with and without your change, so existing installations see no churn.
  • The rbs CLI never produces a relative base_directory. config_path is either an absolute path from Config.find_config_path or the expanded argument of --collection. I could only reproduce the broken symlink by building a Lockfile with a relative lockfile_path directly, which is what a caller like Steep can do.
  • When I did reproduce it, the bug was clear: full_path stayed sig, so File.symlink wrote sig/ast/2.4 as the target, which the kernel then resolved relative to the symlink's own directory rather than the working directory, leaving a dangling link.

So the change is correct and worth having. What it needs is a test that builds the relative case and shows the link resolving, which would also pin down that the CLI is not affected. If you would like to open that as its own PR, I would be glad to review it.

@pocke pocke closed this Sep 7, 2026
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.

2 participants