From 991801a528b0b0d7938f7cb162dda94a306ea2fd Mon Sep 17 00:00:00 2001 From: Randy Olson Date: Tue, 28 Jul 2026 15:16:42 -0700 Subject: [PATCH 1/5] Record the sync point a publish creates `goodeye skills publish` moved the registry without ever touching the local sync index. A directory publish set both drift signals at once: the uploaded tree left the recorded hashes on superseded content while the version advanced, which classifies as a conflict. Nothing could clear it, because a pull refuses a dirty tree and a push sends a token the registry has already replaced, so the mirror stayed stuck until a forced pull discarded the local edits it was protecting. A directory publish now records the whole sync point, found by the published directory's own path. The tree was uploaded, so the registry holds what the directory holds whatever version the index was sitting at, which also recovers a mirror already stranded. A body-only publish reads no directory, so it records only what it knows, only on mirrors sitting at the version it replaced, and writes the new body to disk only when nothing unsaved is there to lose. Publishing a directory also cleared every sibling-file label. A label lives only in the registry, and a snapshot that omits one clears the stored value, so a tracked mirror's recorded labels now ride along with the upload the way a push already sends them. Co-Authored-By: Claude Opus 5 (1M context) --- src/goodeye_cli/commands/skills.py | 174 +++++++++++- src/goodeye_cli/sync.py | 137 +++++++++- tests/test_publish_sync_index.py | 421 +++++++++++++++++++++++++++++ 3 files changed, 720 insertions(+), 12 deletions(-) create mode 100644 tests/test_publish_sync_index.py diff --git a/src/goodeye_cli/commands/skills.py b/src/goodeye_cli/commands/skills.py index c8f452f..92b8b6b 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_purposes(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,141 @@ 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_purposes( + payload: list[dict[str, Any]], + states: list[FileState], + skill_dir: Path, + effective_name: str, +) -> None: + """Re-attach the file labels a tracked mirror already recorded. + + A file's label says what it is for, and it lives only in the registry: a + file on disk has nowhere to hold one. A directory publish sends a full + snapshot, and an entry carrying no label clears the stored one, so + publishing a directory strips every label the skill had. Copying the + recorded label back onto each surviving path is what a push already does + by sending its recorded manifest. + + Labels are 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 labels are not another's. + + A path the mirror never recorded keeps no label rather than being given an + invented one, and a mirror with no labels 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 labels", exc_info=True) + return + if entry is None or entry.slug != effective_name: + return + labels = {f.path: f.purpose for f in entry.files if f.purpose is not None} + if not labels: + return + for wire_entry in payload: + label = labels.get(wire_entry["path"]) + if label is not None: + wire_entry["purpose"] = label + for state_entry in states: + label = labels.get(state_entry.path) + if label is not None: + state_entry.purpose = label + + +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 +866,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..119c2c8 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 ----- @@ -2937,10 +3067,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..3a5db54 --- /dev/null +++ b/tests/test_publish_sync_index.py @@ -0,0 +1,421 @@ +"""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.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"] + + +# ----- 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 From 2d2253cdd9eb1c32407d3a0232863b6fab961609 Mon Sep 17 00:00:00 2001 From: Randy Olson Date: Tue, 28 Jul 2026 15:16:42 -0700 Subject: [PATCH 2/5] chore: bump version to 0.26.0 Co-Authored-By: Claude Opus 5 (1M context) --- pyproject.toml | 2 +- uv.lock | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index f5e6008..4f4d95e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "goodeye" -version = "0.25.2" +version = "0.26.0" 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/uv.lock b/uv.lock index 2dbe072..d3729ce 100644 --- a/uv.lock +++ b/uv.lock @@ -176,7 +176,7 @@ wheels = [ [[package]] name = "goodeye" -version = "0.25.2" +version = "0.26.0" source = { editable = "." } dependencies = [ { name = "httpx" }, From 68baab5ab3c2e9ed240b1c03a4a0176b97937c21 Mon Sep 17 00:00:00 2001 From: Randy Olson Date: Tue, 28 Jul 2026 15:40:56 -0700 Subject: [PATCH 3/5] Carry a recorded execute bit through a directory publish A directory publish builds its snapshot from the walk alone, so the recorded manifest is the only source for anything the walk cannot recover. The role label was already carried back; the execute bit was not. A filesystem that does not preserve the bit reports every file as non-executable, so publishing from a Windows checkout or a FAT/exFAT mount cleared a flag the registry rightly held, and now recorded that loss in the manifest as well. For a file whose content is unchanged the recorded bit is authoritative, the same rule a push already applies; a changed file still carries what disk reports, since its content and any permission meant to go with it are what is being uploaded. Also wrap a test line the formatter rejected. Co-Authored-By: Claude Opus 5 (1M context) --- src/goodeye_cli/commands/skills.py | 70 ++++++++++++++++++---------- tests/test_publish_sync_index.py | 73 ++++++++++++++++++++++++++++-- 2 files changed, 114 insertions(+), 29 deletions(-) diff --git a/src/goodeye_cli/commands/skills.py b/src/goodeye_cli/commands/skills.py index 92b8b6b..914a0c1 100644 --- a/src/goodeye_cli/commands/skills.py +++ b/src/goodeye_cli/commands/skills.py @@ -590,7 +590,7 @@ def publish( # without changing the fallback behavior. _log.debug("could not fetch ignore defaults from server config", exc_info=True) files_payload, file_states = build_files_payload(skill_dir, None, ignore_defaults) - _carry_recorded_purposes(files_payload, file_states, skill_dir, effective_name) + _carry_recorded_file_metadata(files_payload, file_states, skill_dir, effective_name) else: files_payload = None @@ -692,28 +692,38 @@ def _warn_sync_index_stale() -> None: ) -def _carry_recorded_purposes( +def _carry_recorded_file_metadata( payload: list[dict[str, Any]], states: list[FileState], skill_dir: Path, effective_name: str, ) -> None: - """Re-attach the file labels a tracked mirror already recorded. - - A file's label says what it is for, and it lives only in the registry: a - file on disk has nowhere to hold one. A directory publish sends a full - snapshot, and an entry carrying no label clears the stored one, so - publishing a directory strips every label the skill had. Copying the - recorded label back onto each surviving path is what a push already does - by sending its recorded manifest. - - Labels are read only when the directory is published as the skill it + """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 labels are not another's. + skill's metadata is not another's. - A path the mirror never recorded keeps no label rather than being given an - invented one, and a mirror with no labels at all leaves the payload - untouched. + 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 @@ -721,21 +731,31 @@ def _carry_recorded_purposes( 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 labels", exc_info=True) + _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 - labels = {f.path: f.purpose for f in entry.files if f.purpose is not None} - if not labels: + 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: - label = labels.get(wire_entry["path"]) - if label is not None: - wire_entry["purpose"] = label + 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: - label = labels.get(state_entry.path) - if label is not None: - state_entry.purpose = label + 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( diff --git a/tests/test_publish_sync_index.py b/tests/test_publish_sync_index.py index 3a5db54..4a8d3f9 100644 --- a/tests/test_publish_sync_index.py +++ b/tests/test_publish_sync_index.py @@ -213,9 +213,7 @@ def test_folder_publish_under_another_name_leaves_the_index_alone( # 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"] - ) + 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 @@ -298,10 +296,77 @@ def test_folder_publish_does_not_invent_a_label( 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"]} + 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.") From 85ac0e6a65dbca31d4e61fb38375a2a981e4f4a5 Mon Sep 17 00:00:00 2001 From: Randy Olson Date: Tue, 28 Jul 2026 16:02:53 -0700 Subject: [PATCH 4/5] Cover the best-effort index contract and reuse the binding helper A publish reaches the local index only after its write has landed on the registry, so an unreadable index or an unwritable mirror warns and moves on rather than reporting a completed publish as a failure. Nothing tested that, so a change letting the exception escape would have turned a successful publish into an error. Two tests pin it, one per input mode. A push records the verifier refs a save left behind, which is what `_bindings_from_save` was extracted for, so it goes through the helper instead of repeating the comprehension. Co-Authored-By: Claude Opus 5 (1M context) --- src/goodeye_cli/sync.py | 5 +--- tests/test_publish_sync_index.py | 48 ++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/src/goodeye_cli/sync.py b/src/goodeye_cli/sync.py index 119c2c8..e8d421e 100644 --- a/src/goodeye_cli/sync.py +++ b/src/goodeye_cli/sync.py @@ -3018,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 diff --git a/tests/test_publish_sync_index.py b/tests/test_publish_sync_index.py index 4a8d3f9..cdf5b54 100644 --- a/tests/test_publish_sync_index.py +++ b/tests/test_publish_sync_index.py @@ -17,6 +17,7 @@ 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 ( @@ -484,3 +485,50 @@ def test_piped_publish_clearing_files_leaves_the_index_alone( ) 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 From 47e703fca0d20abcb57ac692ee7bd088512deaa5 Mon Sep 17 00:00:00 2001 From: Randy Olson Date: Tue, 28 Jul 2026 16:40:30 -0700 Subject: [PATCH 5/5] fix: correct release version to 0.25.3 This branch carries only a bug fix (the publish sync-point record and best-effort index reuse), not a feature. The single-path file commands that originally justified a minor bump already shipped in 0.25.2 (tag v0.25.2). No v0.26.0 tag exists yet, so nothing is pinned to it. --- pyproject.toml | 2 +- uv.lock | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 4f4d95e..232892d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "goodeye" -version = "0.26.0" +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/uv.lock b/uv.lock index d3729ce..8c0e996 100644 --- a/uv.lock +++ b/uv.lock @@ -176,7 +176,7 @@ wheels = [ [[package]] name = "goodeye" -version = "0.26.0" +version = "0.25.3" source = { editable = "." } dependencies = [ { name = "httpx" },