fix: improve assistant instruction file handling#159
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR converts Lola's instruction installation from automatic (opt-out) to opt-in with user preference persistence. Changes include a new ChangesInstructions opt-in migration with interactive state management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Fixes #158. Assisted-by: Codex
SecKatie
force-pushed
the
issue-158-instructions-replacement
branch
from
11b5f47 to
338c106
Compare
| module_name: str, | ||
| ) -> bool: | ||
| """Generate/update module instructions in a managed section.""" | ||
| """Replace the assistant instructions file with module instructions.""" |
There was a problem hiding this comment.
Would anyone ever actually use this? I can't imagine wanting lola to nuke my AGENTS.md.
There was a problem hiding this comment.
I think the insertion method might be required for Gemini and other CLI tools that don't auto-read the installed files. In those cases, it would make more sense to me to use a pointer to an installed file instead of updating huge chunks of text.
There was a problem hiding this comment.
Agree here, this would be a regression in GEMINI.md instructions. As for "nuke AGENTS.md" I agree this can be necessary sometimes when you want a full bootstrap project, such as when using the context module or plugin; however, it must not be the default. Perhaps it will be clearer to call this overwrite.
An use:
lola install my-mod my-project --overwrite or something along those lines.
Moreover:
it would make more sense to me to use a pointer to an installed file instead of updating huge chunks of text
@trevor-vaughan Thats mostly for what --append-context is created for. @SecKatie, With the current PR, we are also creating some regression in this behavior. That's aligned with my comment here: https://github.com/LobsterTrap/lola/pull/159/changes#r3299635672
Please check out, and let us know.
| install_instructions is None | ||
| and instructions_dest.exists() | ||
| and not instructions_dest.is_dir() | ||
| and not append_context |
There was a problem hiding this comment.
This can be dangerous if CLAUDE.md exists, and I pass --append-context this can bypass and replace entirely the file.
| "(e.g., module/AGENTS.md).", | ||
| ) | ||
| @click.option( | ||
| "--instructions/--no-instructions", |
There was a problem hiding this comment.
This looks more as --overwrite. We can have it with -o|--overwrite but it must be default to FALSE and if using as a bool, the --no-instructions might not be needed anymore.
Also, prefer to keep it fully separated from --append-context. The append context was designed to have an instruction inside a "main.spec (CLAUDE.md, AGENTS.md, GEMINI.md)" telling where a "sub project" main instructions can be found. With that, we should have a clear separation of concerns here, ensuring overwrite is exactly the opposite of append, the code should not touch each other's functions, aside from the mutually exclusive flags.
| module_name: str, | ||
| ) -> bool: | ||
| """Generate/update module instructions in a managed section.""" | ||
| """Replace the assistant instructions file with module instructions.""" |
There was a problem hiding this comment.
Agree here, this would be a regression in GEMINI.md instructions. As for "nuke AGENTS.md" I agree this can be necessary sometimes when you want a full bootstrap project, such as when using the context module or plugin; however, it must not be the default. Perhaps it will be clearer to call this overwrite.
An use:
lola install my-mod my-project --overwrite or something along those lines.
Moreover:
it would make more sense to me to use a pointer to an installed file instead of updating huge chunks of text
@trevor-vaughan Thats mostly for what --append-context is created for. @SecKatie, With the current PR, we are also creating some regression in this behavior. That's aligned with my comment here: https://github.com/LobsterTrap/lola/pull/159/changes#r3299635672
Please check out, and let us know.
sergio-correia
left a comment
sergio-correia
left a comment
There was a problem hiding this comment.
Not in this diff, but closely related: uninstall_cmd at src/lola/cli/install.py:1191-1199 has its own inline instruction-removal logic that calls target.remove_instructions() directly. Since generate_instructions no longer writes marker blocks, that call finds no markers and returns False -- the file is never deleted.
The correct full-file deletion logic exists in _uninstall_instructions() (targets/install.py:768-771) but uninstall_cmd does not call it. The test test_uninstall_deletes_full_file_instructions tests _uninstall_instructions directly (not via uninstall_cmd), so it passes despite the CLI path being broken.
Either have uninstall_cmd call _uninstall_instructions(target, inst), or add the full-file branch inline:
if inst.has_instructions:
instructions_dest = target.get_instructions_path(path_context, inst_scope)
if inst.install_instructions and instructions_dest.exists() \
and not instructions_dest.is_dir():
instructions_dest.unlink()
removed_count += 1
elif target.remove_instructions(instructions_dest, module_name):
removed_count += 1| keep_installing_instructions = existing_installation.install_instructions | ||
| has_instructions = existing_installation.has_instructions | ||
| else: | ||
| keep_installing_instructions = not module.has_instructions |
There was a problem hiding this comment.
This expression is inverted: when a module has no instructions (module.has_instructions is False), not module.has_instructions stores install_instructions=True in the registry.
If the module later adds an AGENTS.md and the user runs lola update, _update_instructions sees install_instructions=True and calls _install_instructions(install_instructions=True), which bypasses the existing-file protection check (lines 379-393 only fire when install_instructions is None). The user's hand-written CLAUDE.md gets silently overwritten.
| keep_installing_instructions = not module.has_instructions | |
| keep_installing_instructions = False |
A module without instructions cannot infer user consent to manage instruction files. If instructions appear in a future version, the normal existence-check flow should apply.
4 participants
Summary
Fixes #158.
This changes Lola instruction installation so assistant instruction files are no longer appended to with Lola marker blocks. New instruction installs replace the assistant instruction file in full, while existing files are protected by default.
What changed
--instructions/--no-instructionsto control assistant instruction file installation.--instructions.<!-- lola:instructions:* -->blocks for assistant instruction files.Validation
pytestruff check src teststy checkAI disclosure
This contribution was implemented with assistance from Codex.
Summary by CodeRabbit
New Features
--instructions/--no-instructionsflags to the install command for explicit control over assistant instruction file handling.Bug Fixes