feat: add pip-style URL fragment support for lola sync#131
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds URL-fragment parsing to ChangesURL Fragment Support for .lola-req
Sequence Diagram(s)sequenceDiagram
participant CLI as sync_cmd
participant Parser as parse_lolareq_line
participant Sync as sync_module_spec
participant Resolve as resolve_and_fetch_module
participant Fetch as fetch_module
participant Save as save_source_info
CLI->>Parser: load `.lola-req` line (URL-like)
Parser->>Parser: split `#`, parse_qs(fragment) -> subdirectory, assistants
Parser->>Sync: return ModuleSpec (subdirectory, assistants)
Sync->>Resolve: chosen ModuleSpec (validated targets)
Resolve->>Fetch: fetch_module(module_content_dirname=spec.subdirectory, ref=git_ref)
Fetch->>Save: save_source_info(content_dirname=spec.subdirectory, origin metadata)
Save-->>Resolve: recorded source info
Resolve-->>Sync: installation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🧹 Nitpick comments (1)
src/lola/sync.py (1)
20-21: ⚡ Quick winUse Python 3.13 union annotations for the newly added fields.
The new annotations should follow project typing style (
str | None,list[str] | None) instead ofOptional[...].💡 Proposed refactor
- subdirectory: Optional[str] = None # Subdirectory path from URL fragment - fragment_assistants: Optional[list[str]] = None # Assistants from URL fragment + subdirectory: str | None = None # Subdirectory path from URL fragment + fragment_assistants: list[str] | None = None # Assistants from URL fragmentAs per coding guidelines,
**/*.py: Use Python 3.13 with modern type hints (e.g.,list[str]instead ofList[str]).🤖 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 `@src/lola/sync.py` around lines 20 - 21, The new fields in src/lola/sync.py use Optional[...]; update their annotations to Python 3.13 union-style types — change subdirectory: Optional[str] = None to subdirectory: str | None = None and fragment_assistants: Optional[list[str]] = None to fragment_assistants: list[str] | None = None; also remove the now-unused Optional import from typing if it becomes unused after this change.
🤖 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 `@README.md`:
- Around line 59-62: Add an explicit precedence note in the "URL fragments
support" section clarifying that when both the URL fragment form
(assistant=name1,name2 or `#assistant`=...) and the inline operator (>>) are
present, the fragment-specified assistants take precedence at runtime; update
the description next to the existing `assistant=name1,name2` / `#assistant=`
line to state this precedence clearly and briefly.
In `@src/lola/cli/sync.py`:
- Around line 173-179: spec.fragment_assistants may contain duplicates which
causes duplicate install attempts; before validating/assigning to
target_assistants, deduplicate the list while preserving order (e.g., use an
order-preserving dedupe like list(dict.fromkeys(...)) or an OrderedDict trick)
and then validate each unique assistant against TARGETS. Update the block
handling spec.fragment_assistants so it replaces the raw list with the
deduplicated list before the existing validation and assignment to
target_assistants.
---
Nitpick comments:
In `@src/lola/sync.py`:
- Around line 20-21: The new fields in src/lola/sync.py use Optional[...];
update their annotations to Python 3.13 union-style types — change subdirectory:
Optional[str] = None to subdirectory: str | None = None and fragment_assistants:
Optional[list[str]] = None to fragment_assistants: list[str] | None = None; also
remove the now-unused Optional import from typing if it becomes unused after
this change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9acd3ceb-4c7c-4462-8ee9-04926c67ccb0
📒 Files selected for processing (4)
README.mdsrc/lola/cli/sync.pysrc/lola/sync.pytests/test_sync.py
rjeffman
force-pushed
the
fix_118
branch
3 times, most recently
from
ecceb65 to
d3cddee
Compare
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 `@docs/guides/declarative.md`:
- Line 62: The fenced example blocks that currently start with ``` (the blocks
containing lines like "# Install from a subdirectory" and "# ✅ Valid - using >>
operator") must include a language tag to satisfy markdownlint MD040; update the
opening fences to use ```text for those example blocks (both occurrences) so
they become ```text and leave the inner content unchanged.
In `@README.md`:
- Line 211: Multiple fenced code blocks in the README are missing language
identifiers (triggering MD040); update each triple-backtick block that contains
the directory listings (the blocks showing "my-skills/" and "my-module/") to
include a language label (e.g., ```text) immediately after the opening backticks
so markdownlint stops flagging them; ensure you update all occurrences mentioned
in the review (the blocks containing the listed directory trees).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b999ae77-2190-4377-b6f7-90f8e111a738
📒 Files selected for processing (6)
README.mddocs/guides/declarative.mdsrc/lola/cli/sync.pysrc/lola/sync.pytests/test_cli_sync.pytests/test_sync.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_sync.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lola/sync.py
Collaborator
|
Is it a good time to just switch from using the @mrbrandao what do you think? @rjeffman I like this very much. Would you be willing to remove the |
Collaborator
|
Since Lola is still pre-1.0, I’d prefer we remove the custom |
Collaborator
Author
|
@SecKatie I didn't remove just because it was there. I'm fine or removing it. |
SecKatie
reviewed
May 18, 2026
Collaborator
SecKatie
left a comment
SecKatie
left a comment
There was a problem hiding this comment.
A couple of small docs and naming requests. Nothing major.
Only blocking issue is the docs should be updated throughout to reflect that >> syntax is gone in this PR.
Thank you so much!
| version_spec: Optional[str] = None # e.g., ">=1.0.0,<2.0" | ||
| assistant: Optional[str] = None # Specific assistant or None for all | ||
| subdirectory: Optional[str] = None # Subdirectory path from URL fragment | ||
| fragment_assistants: Optional[list[str]] = None # Assistants from URL fragment |
Collaborator
There was a problem hiding this comment.
Can we rename this to just be assistants?
Suggested change
| fragment_assistants: Optional[list[str]] = None # Assistants from URL fragment | |
| assistants: Optional[list[str]] = None # Assistants noted by the URL fragment (or all if none are listed) |
| @@ -52,8 +52,16 @@ Create a `.lola-req` in your project: | |||
| python-tools>=1.0.0 | |||
| git-workflow>>claude-code | |||
Collaborator
There was a problem hiding this comment.
We should remove this kind of example throughout the docs.
| - `subdirectory=path/to/module` - Install from a subdirectory in the repository | ||
| - `assistant=name1,name2` - Install to specific assistants (alternative to `>>` operator) | ||
|
|
||
| **Note:** The `#assistant=` fragment and `>>` operator are mutually exclusive - use one or the other, not both. |
Collaborator
There was a problem hiding this comment.
We can remove this line or change it to something like this.
Suggested change
| **Note:** The `#assistant=` fragment and `>>` operator are mutually exclusive - use one or the other, not both. | |
| **Note:** The `>>` operator has been replaced with the `#assistant=` format. Please update your `.lola-req` files if needed. |
Collaborator
Author
There was a problem hiding this comment.
Removed the line.
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 `@README.md`:
- Line 287: The heading "### 4. Add slash commands" is duplicated; update the
section title to the correct sequential number (e.g., change "### 4. Add slash
commands" to "### 5. Add slash commands") and then scan the surrounding README
content for any subsequent step headings or cross-references and increment them
accordingly so the module creation walkthrough numbering is sequential and
consistent.
In `@src/lola/sync.py`:
- Around line 19-20: ModuleSpec uses mixed union syntax; change version_spec,
subdirectory, and assistants to use Python 3.13 union syntax (str | None and
list[str] | None) so all nullable fields are consistent: locate the ModuleSpec
dataclass and update the type annotations for the attributes named version_spec,
subdirectory, and assistants to use the new `| None` form.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f702f43b-e654-427b-9944-60ec2df1bfc8
📒 Files selected for processing (7)
README.mddocs/getting-started/quick-start.mddocs/guides/declarative.mdsrc/lola/cli/sync.pysrc/lola/sync.pytests/test_cli_sync.pytests/test_sync.py
💤 Files with no reviewable changes (1)
- tests/test_cli_sync.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_sync.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
README.md (3)
208-217:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifier to fenced code block.
The directory tree example is missing a language label, triggering markdownlint MD040.
📝 Suggested fix
-``` +```text my-skills/ skills/ example-skill/🤖 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 `@README.md` around lines 208 - 217, The fenced code block showing the directory tree is missing a language identifier which triggers markdownlint MD040; update the opening fence from ``` to ```text (or another appropriate language) for the directory-tree block so the snippet is treated as literal text. Locate the triple-backtick fenced block containing the my-skills/ directory tree and add the language identifier immediately after the opening backticks to fix the lint warning.
276-285:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifier to fenced code block.
The directory tree example is missing a language label, triggering markdownlint MD040.
📝 Suggested fix
-``` +```text my-skills/ skills/ example-skill/🤖 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 `@README.md` around lines 276 - 285, The fenced code block containing the directory tree (the triple-backtick block showing "my-skills/ skills/ example-skill/ SKILL.md") lacks a language identifier causing markdownlint MD040; update that fenced block in README.md by adding a language label (e.g., "text") immediately after the opening ``` so the block becomes ```text and preserves the same contents.
261-270:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifier to fenced code block.
The directory tree example is missing a language label, triggering markdownlint MD040.
📝 Suggested fix
-``` +```text my-skills/ skills/ example-skill/🤖 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 `@README.md` around lines 261 - 270, The fenced code block containing the directory tree example in README.md is missing a language identifier; update the opening triple-backtick to include a language (e.g., add "text" so it becomes ```text) for the directory tree sample to satisfy markdownlint MD040 and ensure proper rendering.docs/guides/declarative.md (1)
48-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifier to fenced code block.
The example block is missing a language label, triggering markdownlint MD040.
📝 Suggested fix
-``` +```text # Install from a subdirectory https://github.com/user/repo.git#subdirectory=plugins/dev🤖 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 `@docs/guides/declarative.md` around lines 48 - 57, The fenced example block in docs/guides/declarative.md is missing a language identifier and triggers markdownlint MD040; update the opening triple-backtick for the example (the block containing the three example URLs) to include a language label such as "text" (i.e., change ``` to ```text) so the code fence is explicitly typed and the linter warning is resolved.
🧹 Nitpick comments (1)
README.md (1)
54-61: ⚡ Quick winClarify that
>>operator and URL fragments are mutually exclusive.The documentation shows URL fragment syntax but doesn't mention that the older
>>operator syntax (if still present in the codebase) cannot be combined with fragments. According to past review discussions, using both simultaneously should raise an error.Consider adding a note explaining this constraint, for example: "Note: URL fragments cannot be combined with the
>>operator syntax."🤖 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 `@README.md` around lines 54 - 61, Add a clear note to the README near the URL fragment examples stating that URL fragments (e.g., `subdirectory=...`, `assistant=...`) cannot be combined with the legacy `>>` operator and that using both must raise an error; update the docs text to include an explicit example/note like "Note: URL fragments cannot be combined with the `>>` operator syntax." and then enforce this in the parser/installer codepath (the function that parses install specifiers—e.g., parseSourceSpecifier / parseInstallTarget) to detect when both a `>>` operator and URL fragment are present and throw a user-facing error with that same message so behavior matches the documentation.
🤖 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 `@README.md`:
- Around line 323-335: The fenced code block showing the directory tree in
README.md is missing a language identifier and triggers markdownlint MD040;
update the opening fence for that block (the triple backticks before
"my-module/") to include a language tag such as "text" (i.e., change ``` to
```text) so the code block is properly labeled and the linter warning is
resolved.
- Around line 291-299: The fenced code block showing the directory tree (the
block containing "my-skills/ skills/ example-skill/ SKILL.md ...") is missing
a language identifier; update that block in README.md by replacing the opening
triple backticks with a labeled fence (e.g., ```text) so markdownlint MD040 is
satisfied and the directory tree is treated as plain text.
In `@src/lola/cli/sync.py`:
- Around line 172-181: The code currently treats spec.assistants truthiness the
same for "not provided" and "provided but empty", causing an explicit empty
fragment to fall back to all targets; change the branch to check for presence
explicitly: use "if spec.assistants is not None:" instead of "if
spec.assistants:", then deduplicate into unique_assistants, validate each asst
against TARGETS (the loop over unique_assistants) and set target_assistants =
unique_assistants (which will be [] if the fragment was explicitly empty); keep
the else branch to set target_assistants = list(TARGETS.keys()) when
spec.assistants is None.
---
Duplicate comments:
In `@docs/guides/declarative.md`:
- Around line 48-57: The fenced example block in docs/guides/declarative.md is
missing a language identifier and triggers markdownlint MD040; update the
opening triple-backtick for the example (the block containing the three example
URLs) to include a language label such as "text" (i.e., change ``` to ```text)
so the code fence is explicitly typed and the linter warning is resolved.
In `@README.md`:
- Around line 208-217: The fenced code block showing the directory tree is
missing a language identifier which triggers markdownlint MD040; update the
opening fence from ``` to ```text (or another appropriate language) for the
directory-tree block so the snippet is treated as literal text. Locate the
triple-backtick fenced block containing the my-skills/ directory tree and add
the language identifier immediately after the opening backticks to fix the lint
warning.
- Around line 276-285: The fenced code block containing the directory tree (the
triple-backtick block showing "my-skills/ skills/ example-skill/ SKILL.md")
lacks a language identifier causing markdownlint MD040; update that fenced block
in README.md by adding a language label (e.g., "text") immediately after the
opening ``` so the block becomes ```text and preserves the same contents.
- Around line 261-270: The fenced code block containing the directory tree
example in README.md is missing a language identifier; update the opening
triple-backtick to include a language (e.g., add "text" so it becomes ```text)
for the directory tree sample to satisfy markdownlint MD040 and ensure proper
rendering.
---
Nitpick comments:
In `@README.md`:
- Around line 54-61: Add a clear note to the README near the URL fragment
examples stating that URL fragments (e.g., `subdirectory=...`, `assistant=...`)
cannot be combined with the legacy `>>` operator and that using both must raise
an error; update the docs text to include an explicit example/note like "Note:
URL fragments cannot be combined with the `>>` operator syntax." and then
enforce this in the parser/installer codepath (the function that parses install
specifiers—e.g., parseSourceSpecifier / parseInstallTarget) to detect when both
a `>>` operator and URL fragment are present and throw a user-facing error with
that same message so behavior matches the documentation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d68fa399-038e-40ca-90c4-be70ffe7ae61
📒 Files selected for processing (7)
README.mddocs/getting-started/quick-start.mddocs/guides/declarative.mdsrc/lola/cli/sync.pysrc/lola/sync.pytests/test_cli_sync.pytests/test_sync.py
💤 Files with no reviewable changes (1)
- tests/test_cli_sync.py
✅ Files skipped from review due to trivial changes (1)
- docs/getting-started/quick-start.md
Collaborator
|
@rjeffman looks great! Can you rebase and push? I'll get it merged today. |
Implement URL fragment parameters for .lola-req files, following pip's subdirectory syntax. Enables precise control over module installation paths and target assistants in declarative configurations. Features: - subdirectory=path/to/module - Install from repository subdirectory - assistant=name1,name2 - Install to specific assistants (comma-separated) - Full compatibility with existing >> operator and @ref syntax Examples: https://github.com/user/repo.git#subdirectory=plugins/dev https://github.com/user/repo.git#assistant=claude-code,cursor https://github.com/user/repo.git@main#subdirectory=plugins&assistant=cursor Changes: - src/lola/sync.py: Add subdirectory and fragment_assistants to ModuleSpec - src/lola/cli/sync.py: Pass subdirectory to fetch_module and handle fragment_assistants - tests/test_sync.py: Add comprehensive test coverage for URL fragments - README.md: Document URL fragment syntax with examples Fixes LobsterTrap#118 Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rafael Guterres Jeffman <rjeffman@redhat.com>
BREAKING CHANGE: The >> operator for specifying assistants in .lola-req files has been removed. Users must now use URL fragment syntax instead. Before: my-module>>claude-code my-module>=1.0.0>>cursor After: https://github.com/user/repo.git#assistant=claude-code https://github.com/user/repo.git#assistant=claude-code,cursor Changes: - Remove `assistant` field from ModuleSpec dataclass - Remove >> operator parsing logic from parse_lolareq_line - Update sync_module_spec to only use fragment_assistants - Remove >> example from help text - Remove all tests using >> operator syntax Closes LobsterTrap#118 Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Simplify naming by removing the "fragment_" prefix from the assistants field in ModuleSpec, since it's clear from context that assistants come from URL fragments. Changes: - Rename ModuleSpec.fragment_assistants to ModuleSpec.assistants - Update all references in parse_lolareq_line and sync_module_spec - Update test assertions to use new field name Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
SecKatie
approved these changes
May 21, 2026
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
Add supoprt for URL fragment parameters for .lola-req files, following pip's
subdirectory syntax. Enables precise control over module installation
paths and target assistants in declarative configurations.
Features:
Examples:
Related Issues
Fixes #118
AI Disclosure
Assisted by Claude-code using Claude Sonnet.
Test Plan
Try to reproduce issues raised in #118
Checklist
pytest)ruff check src tests)ty check)Summary by CodeRabbit
New Features
#subdirectory=...) and assistant targeting (#assistant=...); sync installs specified subdirectory content and honors fragment-based assistant lists (deduplicated, ordered).Documentation
>>assistant-targeting docs.Tests
>>-based assistant tests removed.