Skip to content

fix: improve assistant instruction file handling #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SecKatie wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: improve assistant instruction file handling #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 30 additions & 29 deletions src/lola/cli/install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
get_target,
install_to_assistant,
)
from lola.targets.install import _install_instructions
from lola.utils import ensure_lola_dirs, get_local_modules_path
from lola.cli.utils import handle_lola_error

Expand Down Expand Up @@ -527,11 +528,12 @@ def _update_instructions(ctx: UpdateContext, verbose: bool) -> bool:

Returns True if instructions were successfully installed.
"""
from lola.models import INSTRUCTIONS_FILE

path_context = ctx.inst.project_path or ""
scope = ctx.inst.scope

if not ctx.inst.install_instructions:
return False

if not ctx.has_instructions:
# Always attempt removal - handles stale installation records
instructions_dest = ctx.target.get_instructions_path(path_context, scope)
Expand All @@ -540,35 +542,19 @@ def _update_instructions(ctx: UpdateContext, verbose: bool) -> bool:
console.print(" [yellow]- instructions[/yellow] [dim](removed)[/dim]")
return False

instructions_dest = ctx.target.get_instructions_path(path_context, scope)

# Respect --append-context from the original installation
if ctx.inst.append_context:
from lola.targets.install import _install_instructions

success = _install_instructions(
ctx.target,
ctx.global_module,
ctx.source_module,
ctx.inst.project_path,
ctx.inst.append_context,
scope,
)
if success and verbose:
console.print(" [green]instructions (appended)[/green]")
return success

content_path = _get_content_path(ctx.source_module)
instructions_source = content_path / INSTRUCTIONS_FILE
if not instructions_source.exists():
return False

success = ctx.target.generate_instructions(
instructions_source, instructions_dest, ctx.inst.module_name
success = _install_instructions(
ctx.target,
ctx.global_module,
ctx.source_module,
ctx.inst.project_path,
ctx.inst.append_context,
scope,
install_instructions=True,
)

if success and verbose:
console.print(" [green]instructions[/green]")
label = "instructions (appended)" if ctx.inst.append_context else "instructions"
console.print(f" [green]{label}[/green]")

return success

Expand Down Expand Up @@ -736,6 +722,13 @@ def _format_update_summary(result: UpdateResult) -> str:
"Pass the path to the main context file relative to the module root "
"(e.g., module/AGENTS.md).",
)
@click.option(
"--instructions/--no-instructions",
Copy link
Copy Markdown
Collaborator

@mrbrandao mrbrandao May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

default=None,
help="Install module instructions into assistant instruction files. "
"By default, existing instruction files are prompted for interactively "
"and skipped in non-interactive mode.",
)
@click.option(
"--workspace",
type=str,
Expand All @@ -760,6 +753,7 @@ def install_cmd(
pre_install: Optional[str],
post_install: Optional[str],
append_context: Optional[str],
instructions: Optional[bool],
workspace: Optional[str],
scope: str,
project_path: str,
Expand All @@ -784,6 +778,11 @@ def install_cmd(
"""
project_path = _resolve_install_path(assistant, project_path, workspace)

if append_context and instructions is False:
raise click.UsageError("--append-context cannot be used with --no-instructions")
if append_context and instructions is None:
instructions = True

ensure_lola_dirs()

# Resolve module_name interactively when omitted
Expand Down Expand Up @@ -938,6 +937,7 @@ def install_cmd(
effective_pre_install,
effective_post_install,
append_context,
instructions,
)

# Update installation records with version from marketplace metadata
Expand Down Expand Up @@ -1346,7 +1346,8 @@ def update_cmd(module_name: Optional[str], assistant: Optional[str], verbose: bo
inst.commands = list(ctx.current_commands)
inst.agents = list(ctx.current_agents)
inst.mcps = list(ctx.current_mcps)
inst.has_instructions = result.instructions_ok
if inst.install_instructions:
inst.has_instructions = result.instructions_ok
registry.add(inst)

# Print summary line for this installation
Expand Down
3 changes: 3 additions & 0 deletions src/lola/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,7 @@ class Installation:
mcps: list[str] = field(default_factory=list)
has_instructions: bool = False
append_context: Optional[str] = None
install_instructions: bool = True

def to_dict(self) -> dict:
"""Convert to dictionary for YAML serialization."""
Expand All @@ -563,6 +564,7 @@ def to_dict(self) -> dict:
"agents": self.agents,
"mcps": self.mcps,
"has_instructions": self.has_instructions,
"install_instructions": self.install_instructions,
}
if self.project_path:
result["project_path"] = self.project_path
Expand All @@ -587,6 +589,7 @@ def from_dict(cls, data: dict) -> "Installation":
mcps=data.get("mcps", []),
has_instructions=data.get("has_instructions", False),
append_context=data.get("append_context"),
install_instructions=data.get("install_instructions", False),
)


Expand Down
9 changes: 9 additions & 0 deletions src/lola/prompts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,3 +170,12 @@ def prompt_agent_conflict(agent_name: str, module_name: str) -> tuple[str, str]:
).execute()
return "rename", str(new_name)
return str(action) if action is not None else "skip", ""


def confirm_replace_instructions(instructions_path: str) -> bool:
"""Prompt before writing to an existing assistant instructions file."""
result = inquirer.confirm(
message=f"{instructions_path} already exists. Install Lola instructions there?",
default=False,
).execute()
return bool(result)
76 changes: 9 additions & 67 deletions src/lola/targets/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,10 +557,11 @@ def remove_skill(self, dest_path: Path, skill_name: str) -> bool:


class ManagedInstructionsTarget:
"""Mixin for targets that use managed sections for module instructions.
"""Mixin for targets that write module instructions to assistant files.

This provides shared logic for inserting/removing module instructions
into markdown files like CLAUDE.md, GEMINI.md, AGENTS.md.
New installs replace files like CLAUDE.md, GEMINI.md, and AGENTS.md in full.
Removal still understands legacy managed sections so older installations can
be cleaned up.
"""

INSTRUCTIONS_START_MARKER: str = "<!-- lola:instructions:start -->"
Expand All @@ -580,72 +581,13 @@ def generate_instructions(
dest_path: Path,
module_name: str,
) -> bool:
"""Generate/update module instructions in a managed section."""
"""Replace the assistant instructions file with module instructions."""
Copy link
Copy Markdown
Contributor

@trevor-vaughan trevor-vaughan May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would anyone ever actually use this? I can't imagine wanting lola to nuke my AGENTS.md.

Copy link
Copy Markdown
Contributor

@trevor-vaughan trevor-vaughan May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@mrbrandao mrbrandao May 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

instructions_content = _resolve_source_content(source)
if not instructions_content:
return False

# Read existing file content
if dest_path.exists():
content = dest_path.read_text()
else:
dest_path.parent.mkdir(parents=True, exist_ok=True)
content = ""

module_start, module_end = self._get_module_markers(module_name)

# Build the module block
module_block = f"{module_start}\n{instructions_content}\n{module_end}"

# Check if managed section exists
if (
self.INSTRUCTIONS_START_MARKER in content
and self.INSTRUCTIONS_END_MARKER in content
):
start_idx = content.index(self.INSTRUCTIONS_START_MARKER)
end_idx = content.index(self.INSTRUCTIONS_END_MARKER) + len(
self.INSTRUCTIONS_END_MARKER
)
existing_section = content[start_idx:end_idx]
section_content = existing_section[
len(self.INSTRUCTIONS_START_MARKER) : -len(self.INSTRUCTIONS_END_MARKER)
]

# Remove existing module section if present
if module_start in section_content:
mod_start_idx = section_content.index(module_start)
mod_end_idx = section_content.index(module_end) + len(module_end)
section_content = (
section_content[:mod_start_idx] + section_content[mod_end_idx:]
)

# Collect all module blocks and sort them alphabetically
module_blocks = self._extract_module_blocks(section_content)
module_blocks[module_name] = module_block

# Build new section with sorted modules
sorted_blocks = [
module_blocks[name] for name in sorted(module_blocks.keys())
]
new_section_content = "\n\n".join(sorted_blocks)
if new_section_content:
new_section_content = "\n" + new_section_content + "\n"

new_section = (
self.INSTRUCTIONS_START_MARKER
+ new_section_content
+ self.INSTRUCTIONS_END_MARKER
)
content = content[:start_idx] + new_section + content[end_idx:]
else:
# Create new managed section at the end
new_section = (
f"\n\n{self.INSTRUCTIONS_START_MARKER}\n{module_block}\n"
f"{self.INSTRUCTIONS_END_MARKER}\n"
)
content = content.rstrip() + new_section

dest_path.write_text(content)
dest_path.parent.mkdir(parents=True, exist_ok=True)
dest_path.write_text(instructions_content)
return True

def _extract_module_blocks(self, section_content: str) -> dict[str, str]:
Expand All @@ -661,14 +603,14 @@ def _extract_module_blocks(self, section_content: str) -> dict[str, str]:
def remove_instructions(self, dest_path: Path, module_name: str) -> bool:
"""Remove a module's instructions from the managed section."""
if not dest_path.exists():
return True
return False

content = dest_path.read_text()
if (
self.INSTRUCTIONS_START_MARKER not in content
or self.INSTRUCTIONS_END_MARKER not in content
):
return True
return False

module_start, module_end = self._get_module_markers(module_name)

Expand Down
Loading
Loading