diff --git a/pyproject.toml b/pyproject.toml index f5e6008..232892d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "goodeye" -version = "0.25.2" +version = "0.25.3" description = "Goodeye CLI: a private home for the skills your AI follows and the verifiers its work must pass, from the terminal." readme = "README.md" license = { file = "LICENSE" } diff --git a/src/goodeye_cli/commands/skills.py b/src/goodeye_cli/commands/skills.py index c8f452f..914a0c1 100644 --- a/src/goodeye_cli/commands/skills.py +++ b/src/goodeye_cli/commands/skills.py @@ -6,7 +6,7 @@ import re import sys from pathlib import Path -from typing import Annotated +from typing import TYPE_CHECKING, Annotated, Any import typer from rich.console import Console @@ -32,7 +32,17 @@ next_page_hint, resolve_output_mode, ) -from goodeye_cli.wire import SafetyCheckResult, WorkflowDetail, WorkflowFilePatchResult +from goodeye_cli.wire import ( + SafetyCheckResult, + WorkflowDetail, + WorkflowFilePatchResult, + WorkflowSaveResult, +) + +if TYPE_CHECKING: + # Imported for annotations only: `sync` is loaded lazily inside the + # commands that need it, to keep CLI startup off that import. + from goodeye_cli.sync import FileState _log = logging.getLogger(__name__) @@ -556,12 +566,14 @@ def publish( ) # Build the files payload. - # Directory mode: upload the full tree (everything is inline since there is - # no recorded state to compare against). + # Directory mode: upload the full tree (every file inline, so a stale + # recorded hash can never turn into a reference to a blob the registry no + # longer holds). # Single-file / stdin: omit files entirely so the server carries the # existing tree forward. # --clear-files: send an empty list to wipe the tree. files_payload: list[dict] | None + file_states: list[FileState] = [] if clear_files: files_payload = [] elif is_dir_mode: @@ -577,7 +589,8 @@ def publish( # swallowed error at debug level so a misconfiguration is diagnosable # without changing the fallback behavior. _log.debug("could not fetch ignore defaults from server config", exc_info=True) - files_payload, _ = build_files_payload(skill_dir, None, ignore_defaults) + files_payload, file_states = build_files_payload(skill_dir, None, ignore_defaults) + _carry_recorded_file_metadata(files_payload, file_states, skill_dir, effective_name) else: files_payload = None @@ -595,6 +608,16 @@ def publish( files=files_payload, ) + _record_publish_sync_point( + result, + is_dir_mode=is_dir_mode, + skill_dir=skill_dir, + body=body, + file_states=file_states, + expected_version_token=expected_version_token, + clear_files=clear_files, + ) + console.print( f"[green]Saved[/green] {result.name} v{result.version} " f"(skill_id={result.workflow_id}, version_token={result.version_token})" @@ -655,6 +678,161 @@ def _resolve_expected_version_token( return detail.version_token +def _warn_sync_index_stale() -> None: + """Report that the local copy is behind, without failing the command. + + Every caller reaches this after its write already landed on the registry. + Raising here would report a completed publish as a failure, so an + unreadable index or an unwritable mirror says so and moves on. + """ + _log.debug("could not refresh the local sync index", exc_info=True) + Console(stderr=True).print( + "[yellow]Note[/yellow] the local copy could not be updated; " + "run `goodeye skills sync pull` to resync." + ) + + +def _carry_recorded_file_metadata( + payload: list[dict[str, Any]], + states: list[FileState], + skill_dir: Path, + effective_name: str, +) -> None: + """Re-attach the file metadata a tracked mirror already recorded. + + A directory publish sends a full snapshot built from the walk alone, so + anything the registry holds that the walk cannot recover is cleared by the + upload. Two fields sit in that position, and a push already carries both + forward by sending its recorded manifest. + + A file's role label says what it is for and lives only in the registry: a + file on disk has nowhere to hold one, so an entry carrying no label clears + the stored one, stripping every label the skill had. + + The execute bit does live on disk, but not on every filesystem: a Windows + checkout or a FAT/exFAT mount reports every file as non-executable, which + would clear a flag the registry rightly holds and then record that loss in + the manifest. So for a file whose content is unchanged the recorded bit is + authoritative, the same rule ``build_files_payload`` applies on a push. A + file whose content changed carries the bit just observed on disk, since its + content, and any permission meant to go with it, is what is being uploaded. + + Metadata is read only when the directory is published as the skill it + mirrors. Republished under another name it is a different skill, and one + skill's metadata is not another's. + + A path the mirror never recorded keeps what the walk found rather than + being given an invented label, and a mirror recording no files at all + leaves the payload untouched. + """ + from goodeye_cli import sync + + try: + state = sync.load_sync_state(get_config_paths()) + entry = sync.published_dir_entry(state, skill_dir) + except Exception: + _log.debug("could not read the local sync index for file metadata", exc_info=True) + return + if entry is None or entry.slug != effective_name: + return + recorded = {f.path: f for f in entry.files} + if not recorded: + return + fresh = {s.path: s for s in states} + for wire_entry in payload: + prior = recorded.get(wire_entry["path"]) + if prior is None: + continue + if prior.purpose is not None: + wire_entry["purpose"] = prior.purpose + local = fresh.get(wire_entry["path"]) + if local is not None and local.sha256 == prior.sha256: + wire_entry["executable"] = prior.executable + for state_entry in states: + prior = recorded.get(state_entry.path) + if prior is None: + continue + if prior.purpose is not None: + state_entry.purpose = prior.purpose + if state_entry.sha256 == prior.sha256: + state_entry.executable = prior.executable + + +def _record_publish_sync_point( + result: WorkflowSaveResult, + *, + is_dir_mode: bool, + skill_dir: Path, + body: str, + file_states: list[FileState], + expected_version_token: str | None, + clear_files: bool, +) -> None: + """Record the version a publish created on whatever mirror it belongs to. + + The two input modes know different things and so record different things. + A directory publish uploaded the tree, so the whole sync point moves. A + body-only publish read no directory, so only mirrors recorded at the + version it replaced can be moved, and only their body moves. + + ``--clear-files`` records nothing: it drops files the mirror still lists, + which a pull reconciles. A body-only publish with no token records nothing + either, since without one there is no way to tell which version the + publish was written against. + """ + if is_dir_mode: + _record_published_tree(result, skill_dir=skill_dir, body=body, file_states=file_states) + elif expected_version_token and not clear_files: + _record_published_body(result, expected_version_token=expected_version_token, body=body) + + +def _record_published_tree( + result: WorkflowSaveResult, + *, + skill_dir: Path, + body: str, + file_states: list[FileState], +) -> None: + """Bring the mirror this directory belongs to onto the version just published. + + Best-effort, for the reason given on ``_warn_sync_index_stale``. + """ + from goodeye_cli import sync + + try: + paths = get_config_paths() + state = sync.load_sync_state(paths) + if sync.record_tree_published( + state, result, skill_dir=skill_dir, body=body, file_states=file_states + ): + sync.save_sync_state(state, paths) + except Exception: + _warn_sync_index_stale() + + +def _record_published_body( + result: WorkflowSaveResult, + *, + expected_version_token: str, + body: str, +) -> None: + """Record a body-only publish on every mirror sitting at the replaced version. + + Best-effort, for the reason given on ``_warn_sync_index_stale``. + """ + from goodeye_cli import sync + + try: + paths = get_config_paths() + state = sync.load_sync_state(paths) + if sync.record_body_published( + state, result, expected_version_token=expected_version_token, body=body + ): + sync.save_sync_state(state, paths) + except Exception: + _warn_sync_index_stale() + + def _refresh_sync_index( result: WorkflowFilePatchResult, *, @@ -708,11 +886,7 @@ def _refresh_sync_index( if touched: sync.save_sync_state(state, paths) except Exception: - _log.debug("could not refresh the local sync index", exc_info=True) - Console(stderr=True).print( - "[yellow]Note[/yellow] the local copy could not be updated; " - "run `goodeye skills sync pull` to resync." - ) + _warn_sync_index_stale() def _print_file_change(result: WorkflowFilePatchResult) -> None: diff --git a/src/goodeye_cli/sync.py b/src/goodeye_cli/sync.py index 37e6729..e8d421e 100644 --- a/src/goodeye_cli/sync.py +++ b/src/goodeye_cli/sync.py @@ -38,7 +38,7 @@ if TYPE_CHECKING: from goodeye_cli.client import GoodeyeClient - from goodeye_cli.wire import WorkflowFilePatchResult, WorkflowSummary + from goodeye_cli.wire import WorkflowFilePatchResult, WorkflowSaveResult, WorkflowSummary SyncScope = Literal["owned", "all", "selected"] @@ -809,7 +809,9 @@ def _remove_mirrored_file(entry: SyncEntry, path: str) -> None: _log.warning("could not remove %s from the local copy: %s", local, exc) -def _move_to_new_version(entry: SyncEntry, result: WorkflowFilePatchResult) -> None: +def _move_to_new_version( + entry: SyncEntry, result: WorkflowFilePatchResult | WorkflowSaveResult +) -> None: """Move an entry's recorded sync point onto the version the change created. The registry moved and this entry's record of the content moved with it, so @@ -958,6 +960,134 @@ def record_file_removed( return touched +def _bindings_from_save(result: WorkflowSaveResult) -> list[SyncVerifierBinding]: + """Record the verifier refs a save left on the skill. + + A push always re-sends the recorded bindings, so an entry holding the set + from before the save would reattach superseded refs and undo the change. + """ + return [ + SyncVerifierBinding(name=v.name, verifier_id=v.verifier_id, version=v.version) + for v in result.verifiers + ] + + +def published_dir_entry(state: SyncState, skill_dir: Path) -> SyncEntry | None: + """Return the tracked mirror a published directory is, or None. + + A mirror lives at ``/``, so the directory's own name is the + slug and its parent is the target. The path is made absolute and collapsed + lexically, never through ``Path.resolve``: targets are stored the same way, + and resolving symlinks here would fail to match a target reached through + one, leaving the caller to quietly skip an index it should have updated. + """ + resolved = Path(os.path.abspath(os.path.expanduser(str(skill_dir)))) + return find_entry(state, slug=resolved.name, target_path=str(resolved.parent)) + + +def record_tree_published( + state: SyncState, + result: WorkflowSaveResult, + *, + skill_dir: Path, + body: str, + file_states: list[FileState], +) -> bool: + """Move the mirror a published directory belongs to onto the version it created. + + A directory publish uploads the whole tree, so once it lands the registry + holds exactly what that directory holds. There is nothing left to + reconcile, and the recorded sync point can move with no version check at + all: unlike a single-path change, this does not describe a delta against + one base, so a mirror recorded at any other version is still described by + it. That is also what lets this recover a mirror already stuck at a stale + version rather than leaving it for a forced pull. + + Without it the recorded hashes stay on the superseded content while the + registry moves, which reads as a local edit and a moved server at once: + the ``conflict`` state, which no pull or push can clear because a pull + refuses a dirty tree and a push sends a token the registry has replaced. + + The entry is left alone when the publish went to a different skill than the + one the mirror tracks, which is what a republish under another ``--name`` + does, and what a mirror of a skill the caller cannot write produces. + + Returns whether the entry changed, so the caller can skip persisting an + index no target tracks this directory in. + """ + entry = published_dir_entry(state, skill_dir) + if entry is None or entry.skill_id != result.workflow_id: + return False + entry.synced_version = result.version + entry.version_token = result.version_token + entry.body_sha256 = _recorded_body_sha256(body.encode("utf-8")) + entry.files = file_states + entry.verifier_bindings = _bindings_from_save(result) + return True + + +def _mirror_body_unchanged(entry: SyncEntry) -> bool: + """Report whether this mirror's ``SKILL.md`` still matches its recorded hash. + + False when the copy is missing or unreadable as well as when it differs. + The caller uses this to decide whether writing is safe, and the only safe + answer about a file that cannot be read is to leave it alone. + """ + local = _mirrored_path(entry, "SKILL.md") + if local is None or not local.is_file(): + return False + try: + text = local.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return False + return not is_modified_locally(entry, text) + + +def record_body_published( + state: SyncState, + result: WorkflowSaveResult, + *, + expected_version_token: str, + body: str, +) -> bool: + """Record a body-only publish on every mirror that sat on the version it replaced. + + A publish that streams its body sends no file tree, so the registry carries + the previous one forward and the recorded manifest still describes it. Only + the body and the sync point move. Nothing here reads the mirror's directory + to decide what was published, so only entries recorded at the version the + publish was written against are touched: any other entry has a different + base, exactly as for a single-path change. + + The new body goes to disk only when the mirror's copy still matches its + recorded hash. A mirror holding unsaved edits keeps them: they read as + ordinary local drift the caller resolves with a push, which is a decision + to make rather than one to lose. Either way the recorded hash moves to the + published body, so the mirror is measured against what the registry now + holds. Leaving it behind would let the next push send the superseded body + back and quietly revert the version just published. + + Returns whether any entry changed. + """ + raw = body.encode("utf-8") + new_hash = _recorded_body_sha256(raw) + bindings = _bindings_from_save(result) + touched = False + for entry in entries_at_version( + state, + slug=result.name, + skill_id=result.workflow_id, + version_token=expected_version_token, + ): + if _mirror_body_unchanged(entry): + _write_mirrored_file(entry, "SKILL.md", raw, executable=None) + entry.body_sha256 = new_hash + entry.verifier_bindings = bindings + _move_to_new_version(entry, result) + touched = True + return touched + + # ----- scope selection + change detection ----- @@ -2888,10 +3018,7 @@ def _push_candidate( entry.synced_version = save_result.version entry.version_token = save_result.version_token entry.body_sha256 = body_sha256(body) - entry.verifier_bindings = [ - SyncVerifierBinding(name=v.name, verifier_id=v.verifier_id, version=v.version) - for v in save_result.verifiers - ] + entry.verifier_bindings = _bindings_from_save(save_result) # Record the file states (each sha256 is over the raw on-disk bytes, so a # binary file converges to a reference on the next push rather than re-uploading). entry.files = file_states @@ -2937,10 +3064,13 @@ def _push_candidate( "local_skill_path", "normalize_target_path", "prune_from_allowlist", + "published_dir_entry", "pull", "read_local_body", + "record_body_published", "record_file_removed", "record_file_written", + "record_tree_published", "remove_target", "resolve_preset", "save_sync_config", diff --git a/tests/test_publish_sync_index.py b/tests/test_publish_sync_index.py new file mode 100644 index 0000000..cdf5b54 --- /dev/null +++ b/tests/test_publish_sync_index.py @@ -0,0 +1,534 @@ +"""Tests for the sync point `goodeye skills publish` records. + +A publish moves the registry. When the directory it publishes is a tracked +mirror, or the skill it names is mirrored somewhere, the local index has to +move with it: a recorded sync point left on the superseded version reads as a +local edit and a moved server at once, which is the `conflict` state that no +pull or push can clear. +""" + +from __future__ import annotations + +import hashlib +import json as _json +from pathlib import Path + +import httpx +import respx +from typer.testing import CliRunner + +from goodeye_cli import sync as sync_module +from goodeye_cli.app import app +from goodeye_cli.config import ConfigPaths, save_credentials +from goodeye_cli.sync import ( + FileState, + SyncEntry, + SyncState, + SyncTarget, + body_sha256, + load_sync_state, + save_sync_state, + tree_push_drifted, +) + +SERVER = "https://example.test" + +_SKILL_MD = ( + "---\n" + "name: my-skill\n" + "description: A test skill.\n" + "outcome: Achieve the test outcome.\n" + "---\n\n" + "Do the work.\n" +) + + +def _setup_creds(monkeypatch, tmp_config_paths: ConfigPaths) -> None: + save_credentials({"api_key": "good_live_EXAMPLE", "server": SERVER}, tmp_config_paths) + monkeypatch.setenv("XDG_CONFIG_HOME", str(tmp_config_paths.config_dir.parent)) + monkeypatch.delenv("GOODEYE_API_KEY", raising=False) + monkeypatch.delenv("GOODEYE_SERVER", raising=False) + + +def _sha(raw: bytes) -> str: + return hashlib.sha256(raw).hexdigest() + + +def _save_route(*, version: int = 3, token: str = "tok-new", workflow_id: str = "skl_01"): + """Mock the save endpoint, returning the version the publish created.""" + return respx.post(f"{SERVER}/v1/skills").mock( + return_value=httpx.Response( + 201, + json={ + "workflow_id": workflow_id, + "version": version, + "version_token": token, + "name": "my-skill", + "verifiers": [], + }, + ) + ) + + +def _mirror(tmp_path: Path, *, slug: str = "my-skill", body: str = _SKILL_MD) -> Path: + """Materialize `//` with a SKILL.md and one sibling file.""" + skill_dir = tmp_path / "skills" / slug + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text(body, encoding="utf-8") + (skill_dir / "notes.md").write_bytes(b"notes\n") + return skill_dir + + +def _track( + tmp_config_paths: ConfigPaths, + tmp_path: Path, + *, + slug: str = "my-skill", + skill_id: str = "skl_01", + synced_version: int = 2, + version_token: str = "tok-old", + body: str = _SKILL_MD, + files: list[FileState] | None = None, + target_dir: str = "skills", + extra: list[SyncEntry] | None = None, +) -> None: + """Record `/` in the index as a mirror synced at a version.""" + entry = SyncEntry( + skill_id=skill_id, + slug=slug, + target_path=str(tmp_path / target_dir), + synced_version=synced_version, + version_token=version_token, + body_sha256=body_sha256(body), + files=files if files is not None else [FileState(path="notes.md", sha256=_sha(b"notes\n"))], + ) + save_sync_state( + SyncState(identity="owner@example.com", entries=[entry, *(extra or [])]), + tmp_config_paths, + ) + + +def _target(tmp_path: Path, target_dir: str = "skills") -> SyncTarget: + return SyncTarget(path=str(tmp_path / target_dir)) + + +# ----- directory mode ----- + + +@respx.mock +def test_folder_publish_leaves_the_mirror_clean( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """The published directory's mirror lands on the version it just created.""" + _setup_creds(monkeypatch, tmp_config_paths) + skill_dir = _mirror(tmp_path) + _track(tmp_config_paths, tmp_path) + # The local edit that makes the pre-fix index read as a conflict. + (skill_dir / "notes.md").write_bytes(b"edited notes\n") + _save_route(version=3, token="tok-new") + + result = CliRunner().invoke(app, ["skills", "publish", str(skill_dir)]) + assert result.exit_code == 0, result.output + + entry = load_sync_state(tmp_config_paths).entries[0] + assert entry.synced_version == 3 + assert entry.version_token == "tok-new" + assert not tree_push_drifted(entry, _target(tmp_path), []) + + +@respx.mock +def test_repeated_folder_publishes_stay_clean( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """Publishing twice never drifts into a conflict: the latch cannot form.""" + _setup_creds(monkeypatch, tmp_config_paths) + skill_dir = _mirror(tmp_path) + _track(tmp_config_paths, tmp_path) + runner = CliRunner() + + for version, token in ((3, "tok-3"), (4, "tok-4")): + (skill_dir / "notes.md").write_bytes(f"round {version}\n".encode()) + respx.reset() + _save_route(version=version, token=token) + result = runner.invoke(app, ["skills", "publish", str(skill_dir)]) + assert result.exit_code == 0, result.output + + entry = load_sync_state(tmp_config_paths).entries[0] + assert entry.synced_version == version + assert entry.version_token == token + assert not tree_push_drifted(entry, _target(tmp_path), []) + + +@respx.mock +def test_folder_publish_recovers_a_mirror_left_behind( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A mirror stranded at an old version is recovered, not left for a forced pull. + + The whole tree was uploaded, so the registry holds what the directory + holds regardless of the version the index was sitting at. + """ + _setup_creds(monkeypatch, tmp_config_paths) + skill_dir = _mirror(tmp_path) + _track(tmp_config_paths, tmp_path, synced_version=22, version_token="tok-22") + (skill_dir / "notes.md").write_bytes(b"local work\n") + _save_route(version=29, token="tok-29") + + result = CliRunner().invoke(app, ["skills", "publish", str(skill_dir)]) + assert result.exit_code == 0, result.output + + entry = load_sync_state(tmp_config_paths).entries[0] + assert entry.synced_version == 29 + assert not tree_push_drifted(entry, _target(tmp_path), []) + + +@respx.mock +def test_folder_publish_of_an_untracked_directory_leaves_the_index_alone( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A directory no target tracks writes nothing to the index.""" + _setup_creds(monkeypatch, tmp_config_paths) + _mirror(tmp_path) + _track(tmp_config_paths, tmp_path) + before = tmp_config_paths.sync_state_file.read_text(encoding="utf-8") + + loose = tmp_path / "elsewhere" / "my-skill" + loose.mkdir(parents=True) + (loose / "SKILL.md").write_text(_SKILL_MD, encoding="utf-8") + _save_route() + + result = CliRunner().invoke(app, ["skills", "publish", str(loose)]) + assert result.exit_code == 0, result.output + assert tmp_config_paths.sync_state_file.read_text(encoding="utf-8") == before + + +@respx.mock +def test_folder_publish_under_another_name_leaves_the_index_alone( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """Republished as a different skill, the mirror's own sync point is untouched.""" + _setup_creds(monkeypatch, tmp_config_paths) + skill_dir = _mirror(tmp_path) + _track(tmp_config_paths, tmp_path) + before = tmp_config_paths.sync_state_file.read_text(encoding="utf-8") + # A different name resolves to a different skill server-side. + _save_route(workflow_id="skl_other") + + result = CliRunner().invoke(app, ["skills", "publish", str(skill_dir), "--name", "other-skill"]) + assert result.exit_code == 0, result.output + assert tmp_config_paths.sync_state_file.read_text(encoding="utf-8") == before + + +@respx.mock +def test_a_second_mirror_of_the_same_skill_is_not_marked_clean( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """Only the published copy moves; another target's copy waits for a pull.""" + _setup_creds(monkeypatch, tmp_config_paths) + skill_dir = _mirror(tmp_path) + other = SyncEntry( + skill_id="skl_01", + slug="my-skill", + target_path=str(tmp_path / "second"), + synced_version=2, + version_token="tok-old", + body_sha256=body_sha256(_SKILL_MD), + files=[], + ) + _track(tmp_config_paths, tmp_path, extra=[other]) + _save_route(version=3, token="tok-new") + + result = CliRunner().invoke(app, ["skills", "publish", str(skill_dir)]) + assert result.exit_code == 0, result.output + + entries = {e.target_path: e for e in load_sync_state(tmp_config_paths).entries} + assert entries[str(tmp_path / "skills")].synced_version == 3 + second = entries[str(tmp_path / "second")] + assert second.synced_version == 2 + assert second.version_token == "tok-old" + + +# ----- file labels ----- + + +@respx.mock +def test_folder_publish_keeps_recorded_file_labels( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A label the registry already held survives the snapshot upload. + + A label lives only in the registry, so a snapshot that omits it clears it. + """ + _setup_creds(monkeypatch, tmp_config_paths) + skill_dir = _mirror(tmp_path) + _track( + tmp_config_paths, + tmp_path, + files=[FileState(path="notes.md", sha256=_sha(b"notes\n"), purpose="reference")], + ) + route = _save_route() + + result = CliRunner().invoke(app, ["skills", "publish", str(skill_dir)]) + assert result.exit_code == 0, result.output + + sent = _json.loads(route.calls.last.request.content.decode()) + uploaded = {f["path"]: f for f in sent["files"]} + assert uploaded["notes.md"]["purpose"] == "reference" + # The index keeps it too, so the next push re-sends it. + recorded = {f.path: f for f in load_sync_state(tmp_config_paths).entries[0].files} + assert recorded["notes.md"].purpose == "reference" + + +@respx.mock +def test_folder_publish_does_not_invent_a_label( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A file the mirror never recorded a label for is sent without one.""" + _setup_creds(monkeypatch, tmp_config_paths) + skill_dir = _mirror(tmp_path) + (skill_dir / "fresh.md").write_bytes(b"brand new\n") + _track( + tmp_config_paths, + tmp_path, + files=[FileState(path="notes.md", sha256=_sha(b"notes\n"), purpose="reference")], + ) + route = _save_route() + + result = CliRunner().invoke(app, ["skills", "publish", str(skill_dir)]) + assert result.exit_code == 0, result.output + + uploaded = { + f["path"]: f for f in _json.loads(route.calls.last.request.content.decode())["files"] + } + assert "purpose" not in uploaded["fresh.md"] + + +# ----- file execute bit ----- + + +@respx.mock +def test_folder_publish_keeps_a_recorded_execute_bit( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """An unchanged file keeps the execute bit the registry already held. + + A Windows checkout or a FAT/exFAT mount reports every file as + non-executable, so reading the bit off disk would clear a flag the registry + rightly holds and then record that loss in the manifest. + """ + _setup_creds(monkeypatch, tmp_config_paths) + skill_dir = _mirror(tmp_path) + (skill_dir / "notes.md").chmod(0o644) + _track( + tmp_config_paths, + tmp_path, + files=[FileState(path="notes.md", sha256=_sha(b"notes\n"), executable=True)], + ) + route = _save_route() + + result = CliRunner().invoke(app, ["skills", "publish", str(skill_dir)]) + assert result.exit_code == 0, result.output + + uploaded = { + f["path"]: f for f in _json.loads(route.calls.last.request.content.decode())["files"] + } + assert uploaded["notes.md"]["executable"] is True + recorded = {f.path: f for f in load_sync_state(tmp_config_paths).entries[0].files} + assert recorded["notes.md"].executable is True + + +@respx.mock +def test_folder_publish_takes_the_local_bit_for_a_changed_file( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A file whose content changed carries the bit disk reports now. + + Its content, and any permission meant to go with it, is what is being + uploaded, so the recorded bit is no longer the better answer. + """ + _setup_creds(monkeypatch, tmp_config_paths) + skill_dir = _mirror(tmp_path) + (skill_dir / "notes.md").write_bytes(b"edited notes\n") + (skill_dir / "notes.md").chmod(0o644) + _track( + tmp_config_paths, + tmp_path, + files=[FileState(path="notes.md", sha256=_sha(b"notes\n"), executable=True)], + ) + route = _save_route() + + result = CliRunner().invoke(app, ["skills", "publish", str(skill_dir)]) + assert result.exit_code == 0, result.output + + uploaded = { + f["path"]: f for f in _json.loads(route.calls.last.request.content.decode())["files"] + } + assert uploaded["notes.md"]["executable"] is False + recorded = {f.path: f for f in load_sync_state(tmp_config_paths).entries[0].files} + assert recorded["notes.md"].executable is False + + +# ----- piped mode ----- + +_NEW_BODY = _SKILL_MD.replace("Do the work.", "Do the improved work.") + + +@respx.mock +def test_piped_publish_updates_a_clean_mirror( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """With nothing unsaved on disk, the mirror takes the published body.""" + _setup_creds(monkeypatch, tmp_config_paths) + skill_dir = _mirror(tmp_path) + _track(tmp_config_paths, tmp_path, version_token="tok-old") + _save_route(version=3, token="tok-new") + + result = CliRunner().invoke( + app, + ["skills", "publish", "-", "--expected-version-token", "tok-old"], + input=_NEW_BODY, + ) + assert result.exit_code == 0, result.output + + assert (skill_dir / "SKILL.md").read_text(encoding="utf-8") == _NEW_BODY + entry = load_sync_state(tmp_config_paths).entries[0] + assert entry.synced_version == 3 + assert entry.version_token == "tok-new" + assert entry.body_sha256 == body_sha256(_NEW_BODY) + + +@respx.mock +def test_piped_publish_keeps_unsaved_local_edits( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """Unsaved work on disk is never overwritten; it becomes drift to push.""" + _setup_creds(monkeypatch, tmp_config_paths) + skill_dir = _mirror(tmp_path) + _track(tmp_config_paths, tmp_path, version_token="tok-old") + in_flight = _SKILL_MD.replace("Do the work.", "My own unsaved edit.") + (skill_dir / "SKILL.md").write_text(in_flight, encoding="utf-8") + _save_route(version=3, token="tok-new") + + result = CliRunner().invoke( + app, + ["skills", "publish", "-", "--expected-version-token", "tok-old"], + input=_NEW_BODY, + ) + assert result.exit_code == 0, result.output + + assert (skill_dir / "SKILL.md").read_text(encoding="utf-8") == in_flight + entry = load_sync_state(tmp_config_paths).entries[0] + assert entry.synced_version == 3 + # Measured against what the registry now holds, so the surviving edit reads + # as ordinary drift a push resolves rather than a conflict. + assert entry.body_sha256 == body_sha256(_NEW_BODY) + + +@respx.mock +def test_piped_publish_without_a_token_leaves_the_index_alone( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """With no token there is no way to tell which version was replaced.""" + _setup_creds(monkeypatch, tmp_config_paths) + _mirror(tmp_path) + _track(tmp_config_paths, tmp_path) + before = tmp_config_paths.sync_state_file.read_text(encoding="utf-8") + _save_route() + + result = CliRunner().invoke(app, ["skills", "publish", "-"], input=_NEW_BODY) + assert result.exit_code == 0, result.output + assert tmp_config_paths.sync_state_file.read_text(encoding="utf-8") == before + + +@respx.mock +def test_piped_publish_with_a_stale_token_leaves_the_index_alone( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A mirror on some other version has a different base and waits for a pull.""" + _setup_creds(monkeypatch, tmp_config_paths) + _mirror(tmp_path) + _track(tmp_config_paths, tmp_path, version_token="tok-old") + before = tmp_config_paths.sync_state_file.read_text(encoding="utf-8") + _save_route() + + result = CliRunner().invoke( + app, + ["skills", "publish", "-", "--expected-version-token", "tok-somewhere-else"], + input=_NEW_BODY, + ) + assert result.exit_code == 0, result.output + assert tmp_config_paths.sync_state_file.read_text(encoding="utf-8") == before + + +@respx.mock +def test_piped_publish_clearing_files_leaves_the_index_alone( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """Clearing the tree drops files the mirror still lists: a pull reconciles it.""" + _setup_creds(monkeypatch, tmp_config_paths) + _mirror(tmp_path) + _track(tmp_config_paths, tmp_path, version_token="tok-old") + before = tmp_config_paths.sync_state_file.read_text(encoding="utf-8") + _save_route() + + result = CliRunner().invoke( + app, + [ + "skills", + "publish", + "-", + "--clear-files", + "--expected-version-token", + "tok-old", + ], + input=_NEW_BODY, + ) + assert result.exit_code == 0, result.output + assert tmp_config_paths.sync_state_file.read_text(encoding="utf-8") == before + + +# ----- best effort ----- + + +def _boom(*_args, **_kwargs): + raise OSError("no") + + +@respx.mock +def test_folder_publish_survives_an_unreadable_index( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """An index that cannot be read says so rather than failing the publish. + + The registry write has already landed by the time the index is touched, so + raising here would report a completed publish as a failure. + """ + _setup_creds(monkeypatch, tmp_config_paths) + skill_dir = _mirror(tmp_path) + _track(tmp_config_paths, tmp_path) + _save_route() + monkeypatch.setattr(sync_module, "load_sync_state", _boom) + + result = CliRunner().invoke(app, ["skills", "publish", str(skill_dir)]) + assert result.exit_code == 0, result.output + assert "could not be updated" in result.stderr + + +@respx.mock +def test_piped_publish_survives_an_unwritable_mirror( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A mirror that cannot be written warns, for the same reason.""" + _setup_creds(monkeypatch, tmp_config_paths) + _mirror(tmp_path) + _track(tmp_config_paths, tmp_path, version_token="tok-old") + _save_route(version=3, token="tok-new") + monkeypatch.setattr(sync_module, "_write_mirrored_file", _boom) + + result = CliRunner().invoke( + app, + ["skills", "publish", "-", "--expected-version-token", "tok-old"], + input=_NEW_BODY, + ) + assert result.exit_code == 0, result.output + assert "could not be updated" in result.stderr diff --git a/uv.lock b/uv.lock index 2dbe072..8c0e996 100644 --- a/uv.lock +++ b/uv.lock @@ -176,7 +176,7 @@ wheels = [ [[package]] name = "goodeye" -version = "0.25.2" +version = "0.25.3" source = { editable = "." } dependencies = [ { name = "httpx" },