Skip to content

Commit 1f0476f

Browse files
Kasper JungeRalphify
authored andcommitted
fix: resolve lockfile not updating on partial add failure
When `agr add` was called with multiple refs and some failed, save_and_summarize_results raised SystemExit(1) before _update_lockfile_for_adds could execute. This left the lockfile inconsistent with the config — successful deps appeared in agr.toml but not in agr.lock. Move the lockfile update before the summary/exit so successful installs are always persisted to the lockfile regardless of whether other refs in the same batch failed. Co-authored-by: Ralphify <noreply@ralphify.co>
1 parent 9a4020f commit 1f0476f

2 files changed

Lines changed: 36 additions & 2 deletions

File tree

agr/commands/add.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,11 @@ def _print_add_result(result: CommandResult) -> None:
277277
console.print(f"[red]Failed:[/red] {result.ref}")
278278
console.print(f" [dim]{result.message}[/dim]", soft_wrap=True)
279279

280+
# Update lockfile before save_and_summarize_results because the latter
281+
# raises SystemExit(1) on partial failure, which would skip the lockfile
282+
# write and leave it inconsistent with the config.
283+
_update_lockfile_for_adds(lockfile_updates, config_path)
284+
280285
save_and_summarize_results(
281286
results,
282287
config,
@@ -286,8 +291,6 @@ def _print_add_result(result: CommandResult) -> None:
286291
print_result=_print_add_result,
287292
)
288293

289-
_update_lockfile_for_adds(lockfile_updates, config_path)
290-
291294

292295
def _maybe_suggest_repo_skills(
293296
ref: str,

tests/cli/agr/test_add.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,3 +131,34 @@ def test_add_both_markers_fails(self, agr, cli_project):
131131
result = agr("add", "./both-type")
132132

133133
assert_cli(result).failed().stdout_contains("SKILL.md and RALPH.md")
134+
135+
136+
class TestAgrAddLockfileOnPartialFailure:
137+
"""Regression: lockfile must be updated for successful adds even when others fail."""
138+
139+
def test_lockfile_updated_on_partial_add_failure(self, agr, cli_project, cli_skill):
140+
"""agr add updates lockfile for successful installs even when some refs fail.
141+
142+
When `agr add good-skill bad-skill` partially succeeds, the config
143+
is saved with the good skill but the lockfile must also be updated.
144+
Previously, SystemExit(1) from the summary prevented the lockfile
145+
write, leaving it inconsistent with the config.
146+
"""
147+
result = agr("add", "./skills/test-skill", "./nonexistent")
148+
149+
# Partial failure: exit code 1, but one skill succeeded
150+
assert_cli(result).failed()
151+
assert "Added:" in result.stdout
152+
153+
# Config should have the successful dep
154+
config = AgrConfig.load(cli_project / "agr.toml")
155+
assert any(d.path == "./skills/test-skill" for d in config.dependencies)
156+
157+
# Lockfile must also have the successful dep's entry
158+
from agr.lockfile import load_lockfile
159+
160+
lockfile = load_lockfile(cli_project / "agr.lock")
161+
assert lockfile is not None, "agr.lock should exist after partial add"
162+
assert len(lockfile.skills) == 1
163+
assert lockfile.skills[0].path == "./skills/test-skill"
164+
assert lockfile.skills[0].installed_name == "test-skill"

0 commit comments

Comments
 (0)