From d943aefd959a04b29927fe00ab787fbe8843ffce Mon Sep 17 00:00:00 2001 From: Randy Olson Date: Wed, 22 Jul 2026 07:47:01 -0700 Subject: [PATCH 1/5] Add put-file and rm-file for single-path skill edits Change one file in a hosted skill without resending the rest of its tree: goodeye skills put-file --from-file ./notes.md goodeye skills rm-file Paths the command does not name ride forward untouched, unlike `goodeye skills publish `, which uploads a whole-tree snapshot where any omitted path is deleted. A file's executable mark and role label keep their current values unless the caller sets them, so a flag that was not passed is left off the wire rather than sent as a default that would reset a setting the caller never mentioned. Text and binary content pick their wire field through one shared decision, now used by both the whole-tree snapshot builder and these commands, so the two can never disagree about whether a given file is text. With no --expected-version-token, the current token is read first and passed. That is not a weaker guard: the server still rejects a writer who landed between the read and the write, which is the only race a single command invocation has. The flag stays for scripts that already hold a token. After a successful change, the local sync index records the new state for that path (or drops it, for a removal) in every target mirroring the skill. Without that, the recorded hash would stay stale and the next `goodeye skills sync push` would report drift for a file the registry already holds, so the commands would introduce false drift on their own. When no target tracks the skill, the index is left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/goodeye_cli/client.py | 34 ++ src/goodeye_cli/commands/skills.py | 267 ++++++++++- src/goodeye_cli/sync.py | 145 +++++- src/goodeye_cli/wire.py | 18 + tests/test_commands_skill_files.py | 738 +++++++++++++++++++++++++++++ 5 files changed, 1179 insertions(+), 23 deletions(-) create mode 100644 tests/test_commands_skill_files.py diff --git a/src/goodeye_cli/client.py b/src/goodeye_cli/client.py index f328314..1f1ba96 100644 --- a/src/goodeye_cli/client.py +++ b/src/goodeye_cli/client.py @@ -76,6 +76,7 @@ WorkflowDeleteResult, WorkflowDeleteVersionResult, WorkflowDetail, + WorkflowFilePatchResult, WorkflowGrantList, WorkflowGrantResult, WorkflowGrantRevokeResult, @@ -529,6 +530,39 @@ def save_workflow( response = self._request("POST", "/v1/skills", json_body=payload) return WorkflowSaveResult.model_validate(_alias_skill_id(response.json())) + def patch_workflow_files( + self, + id_or_slug: str, + *, + expected_version_token: str, + files: list[dict[str, Any]] | None = None, + delete_paths: list[str] | None = None, + source: str | None = None, + ) -> WorkflowFilePatchResult: + """PATCH /v1/skills/{id_or_slug}/files: change named paths, keep the rest. + + Only the paths named in ``files`` and ``delete_paths`` are touched; + every other path in the skill rides forward unchanged. This is the + opposite of ``save_workflow``, whose ``files`` is a whole-tree snapshot + that deletes any path left out. + + Each entry in ``files`` carries ``path`` plus exactly one of + ``content`` (verbatim UTF-8 text), ``content_base64`` (base64-encoded + bytes), or ``sha256`` (a blob the registry can already read). Omitting + ``executable`` or ``purpose`` on an entry keeps that file's current + value, so a flag the caller did not set must be left off entirely + rather than sent as a default. + """ + payload: dict[str, Any] = { + "expected_version_token": expected_version_token, + "files": list(files or []), + "delete_paths": list(delete_paths or []), + } + if source is not None: + payload["source"] = source + response = self._request("PATCH", f"/v1/skills/{id_or_slug}/files", json_body=payload) + return WorkflowFilePatchResult.model_validate(_alias_skill_id(response.json())) + def archive_workflow(self, workflow_id: str) -> WorkflowArchiveResult: response = self._request("POST", f"/v1/skills/{workflow_id}/archive", json_body={}) return WorkflowArchiveResult.model_validate(_alias_skill_id(response.json())) diff --git a/src/goodeye_cli/commands/skills.py b/src/goodeye_cli/commands/skills.py index 21b1eb4..6e5d0dc 100644 --- a/src/goodeye_cli/commands/skills.py +++ b/src/goodeye_cli/commands/skills.py @@ -2,6 +2,7 @@ from __future__ import annotations +import hashlib import logging import re import sys @@ -16,7 +17,7 @@ from goodeye_cli.client import GoodeyeClient from goodeye_cli.commands import workflows_sync from goodeye_cli.commands.prompts import confirm_destructive -from goodeye_cli.config import get_api_key, get_server +from goodeye_cli.config import get_api_key, get_config_paths, get_server from goodeye_cli.errors import AuthRequired, ValidationFailed from goodeye_cli.frontmatter import ( coerce_outcome, @@ -32,7 +33,7 @@ next_page_hint, resolve_output_mode, ) -from goodeye_cli.wire import SafetyCheckResult, WorkflowDetail +from goodeye_cli.wire import SafetyCheckResult, WorkflowDetail, WorkflowFilePatchResult _log = logging.getLogger(__name__) @@ -603,6 +604,266 @@ def publish( _print_authoring_notes([*result.authoring_notes, *extra_notes]) +def _read_file_bytes(source: Path) -> bytes: + """Read the raw bytes of a local file named as file content.""" + if not source.exists(): + raise ValidationFailed( + slug="validation_error", + message=f"File not found: {source}", + ) + if not source.is_file(): + raise ValidationFailed( + slug="validation_error", + message=f"Not a file: {source}", + ) + try: + return source.read_bytes() + except OSError as exc: + raise ValidationFailed( + slug="validation_error", + message=f"Could not read file: {source}", + ) from exc + + +def _read_stdin_bytes() -> bytes: + """Read standard input as raw bytes so binary content survives the pipe.""" + buffer = getattr(sys.stdin, "buffer", None) + if buffer is None: # pragma: no cover - only on a text-only stdin stub + return sys.stdin.read().encode("utf-8") + return buffer.read() + + +def _resolve_expected_version_token( + client: GoodeyeClient, skill_id: str, supplied: str | None +) -> str: + """Return the token to write against, reading the current one when unset. + + Resolving it here is not a weaker guard than demanding the flag: the server + still rejects the write if another writer landed between this read and it, + which is the only race a single command invocation can have. The flag stays + for scripts that already hold a token and want to skip the round-trip. + """ + if supplied: + return supplied + detail = client.get_workflow(skill_id) + assert isinstance(detail, WorkflowDetail) + if not detail.version_token: + raise ValidationFailed( + slug="validation_error", + message=f"Could not read the current version token for {skill_id}.", + hint="Pass --expected-version-token with the token from `goodeye skills get --json`.", + ) + return detail.version_token + + +def _refresh_sync_index( + result: WorkflowFilePatchResult, + *, + path: str, + sha256: str | None, + executable: bool | None = None, + purpose: str | None = None, +) -> None: + """Bring the local sync index in line with the change that just landed. + + A tracked mirror records a hash per file. Leaving it stale after a + single-path change makes the next `goodeye skills sync push` report drift + for a file the registry already holds, so the change would introduce false + drift on its own. When no target tracks this skill there is nothing to + record and the index is left untouched. + + Best-effort: the write already succeeded, so an unreadable or unwritable + index is reported and moves on rather than failing the command. + """ + from goodeye_cli import sync + + slug = result.slug or result.name + try: + paths = get_config_paths() + state = sync.load_sync_state(paths) + if sha256 is None: + touched = sync.record_file_removed( + state, slug=slug, skill_id=result.workflow_id, path=path + ) + else: + touched = sync.record_file_written( + state, + slug=slug, + skill_id=result.workflow_id, + path=path, + sha256=sha256, + executable=executable, + purpose=purpose, + ) + 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 sync index could not be updated; " + "run `goodeye skills sync pull` to resync." + ) + + +def _print_file_change(result: WorkflowFilePatchResult) -> None: + """Report what the change touched, and what it left alone. + + Paths and the skill name come back from the server, so they are escaped: a + path containing square brackets would otherwise read as Rich markup. + """ + console = Console() + console.print( + f"[green]Updated[/green] {rich_escape(result.name)} v{result.version} " + f"(skill_id={result.workflow_id}, version_token={result.version_token})" + ) + if result.changed: + console.print(f" changed: {rich_escape(', '.join(result.changed))}") + if result.deleted: + console.print(f" deleted: {rich_escape(', '.join(result.deleted))}") + console.print(f" kept unchanged: {result.carried_forward} file(s)") + _print_authoring_notes(result.authoring_notes) + + +@app.command("put-file") +def put_file( + skill_id: str = typer.Argument(..., help="Skill UUID or name."), + path: str = typer.Argument( + ..., + help=( + "Path of the file inside the skill, relative and POSIX-style " + "(e.g. references/rubric.md). Use SKILL.md to rewrite the runbook." + ), + ), + from_file: Path | None = typer.Option( + None, "--from-file", help="Read the new content for PATH from this local file." + ), + stdin: bool = typer.Option( + False, "--stdin", help="Read the new content for PATH from standard input." + ), + executable: bool | None = typer.Option( + None, + "--executable/--no-executable", + help=( + "Mark the file executable when extracted, or clear that mark. " + "Omit both to keep the file's current setting." + ), + ), + purpose: str | None = typer.Option( + None, + "--purpose", + help=( + "Short label for the file's role in the skill. Omit to keep the file's current label." + ), + ), + expected_version_token: str | None = typer.Option( + None, + "--expected-version-token", + help=( + "Token for the version you are changing, for scripts that already " + "hold one. Omit to read the current token first." + ), + ), +) -> None: + """Write one file in a hosted skill, keeping every other path unchanged. + + Only the path you name changes: the rest of the skill's files ride forward + untouched, unlike `goodeye skills publish `, which replaces the whole + tree so any path missing from the directory is deleted. Send the file's + complete new content, not a patch or a diff. + + \b + goodeye skills put-file my-skill references/rubric.md --from-file ./rubric.md + cat rubric.md | goodeye skills put-file my-skill references/rubric.md --stdin + + The file's executable mark and role label keep their current values unless + you set them, and the skill's description, outcome, tags, and verifier + references carry forward untouched. This writes the next version of the + skill and requires edit access. + """ + if from_file is not None and stdin: + raise ValidationFailed( + slug="validation_error", + message="Use either --from-file or --stdin, not both.", + ) + if from_file is None and not stdin: + raise ValidationFailed( + slug="validation_error", + message="No content given for the file.", + hint="Pass --from-file to read a local file, or --stdin to pipe it in.", + ) + + from goodeye_cli.sync import inline_content_field + + # Exactly one source is set by the checks above, so an unset --from-file + # means the content is on stdin. + raw = _read_stdin_bytes() if from_file is None else _read_file_bytes(from_file) + entry: dict[str, object] = {"path": path, **inline_content_field(raw)} + # An absent `executable` or `purpose` means "keep the current value" on this + # route, so a flag the caller did not pass must be left off entirely rather + # than sent as a default that would reset a setting they never mentioned. + if executable is not None: + entry["executable"] = executable + if purpose is not None: + entry["purpose"] = purpose + + with _client(require_auth=True) as client: + token = _resolve_expected_version_token(client, skill_id, expected_version_token) + result = client.patch_workflow_files( + skill_id, + expected_version_token=token, + files=[entry], + ) + + _print_file_change(result) + _refresh_sync_index( + result, + path=path, + sha256=hashlib.sha256(raw).hexdigest(), + executable=executable, + purpose=purpose, + ) + + +@app.command("rm-file") +def rm_file( + skill_id: str = typer.Argument(..., help="Skill UUID or name."), + path: str = typer.Argument( + ..., + help=( + "Path of the file to remove from the skill, relative and " + "POSIX-style (e.g. references/rubric.md)." + ), + ), + expected_version_token: str | None = typer.Option( + None, + "--expected-version-token", + help=( + "Token for the version you are changing, for scripts that already " + "hold one. Omit to read the current token first." + ), + ), +) -> None: + """Remove one file from a hosted skill, keeping every other path unchanged. + + Only the path you name is removed: the rest of the skill's files ride + forward untouched, unlike `goodeye skills publish `, which replaces the + whole tree so any path missing from the directory is deleted. The path must + already be in the skill, and the runbook (SKILL.md) cannot be removed. + + This writes the next version of the skill and requires edit access. + """ + with _client(require_auth=True) as client: + token = _resolve_expected_version_token(client, skill_id, expected_version_token) + result = client.patch_workflow_files( + skill_id, + expected_version_token=token, + delete_paths=[path], + ) + + _print_file_change(result) + _refresh_sync_index(result, path=path, sha256=None) + + @app.command("lineage") def lineage( skill_id: str = typer.Argument(..., help="Skill UUID or name."), @@ -1178,7 +1439,9 @@ def audit( "optimize", "optimize_description", "publish", + "put_file", "revoke_grant", + "rm_file", "teach", "transfer_ownership", "unarchive", diff --git a/src/goodeye_cli/sync.py b/src/goodeye_cli/sync.py index b57e4f7..45db1d2 100644 --- a/src/goodeye_cli/sync.py +++ b/src/goodeye_cli/sync.py @@ -697,6 +697,93 @@ def upsert_entry(state: SyncState, entry: SyncEntry) -> None: state.entries.append(entry) +def entries_for_skill(state: SyncState, *, slug: str, skill_id: str) -> list[SyncEntry]: + """Return every tracked entry for one skill, across all targets. + + The same skill may be mirrored into more than one target, so a change to it + touches every copy. Matching on either identifier keeps a mirror recognized + when one of them has moved on: an entry written before a rename still + carries the old slug under the same id, and an entry from an older index + may carry the slug the caller typed while its id is what the registry + returned. + """ + return [entry for entry in state.entries if entry.slug == slug or entry.skill_id == skill_id] + + +def record_file_written( + state: SyncState, + *, + slug: str, + skill_id: str, + path: str, + sha256: str, + executable: bool | None = None, + purpose: str | None = None, +) -> bool: + """Record that ``path`` now holds content hashing to ``sha256``. + + Called after a single-path write lands on the registry so the index matches + what the server holds. Without it the recorded manifest keeps the previous + hash and the next push reports drift for a file that is already current. + + ``executable`` and ``purpose`` are carry-forward sentinels, mirroring the + write itself: ``None`` keeps the value already recorded for that path (or + falls back to ``False`` / ``None`` for a path new to the manifest), and an + explicit value replaces it. + + ``SKILL.md`` is the body rather than a sibling, so it updates + ``body_sha256`` and never enters ``files``; recording it as a file would + manufacture permanent drift, since the on-disk walk never yields it. + + Returns whether any entry changed, so the caller can skip persisting an + index no target tracks this skill in. + """ + touched = False + for entry in entries_for_skill(state, slug=slug, skill_id=skill_id): + if path == "SKILL.md": + entry.body_sha256 = sha256 + touched = True + continue + recorded = next((f for f in entry.files if f.path == path), None) + updated = FileState( + path=path, + sha256=sha256, + executable=( + executable + if executable is not None + else (recorded.executable if recorded is not None else False) + ), + purpose=( + purpose + if purpose is not None + else (recorded.purpose if recorded is not None else None) + ), + ) + if recorded is None: + entry.files.append(updated) + else: + entry.files[entry.files.index(recorded)] = updated + entry.files.sort(key=lambda f: f.path) + touched = True + return touched + + +def record_file_removed(state: SyncState, *, slug: str, skill_id: str, path: str) -> bool: + """Drop ``path`` from every tracked copy of a skill's recorded manifest. + + The mirror image of ``record_file_written`` for a removal: a path left in + the manifest after it is gone from the registry reads as a local deletion + on the next push. Returns whether any entry changed. + """ + touched = False + for entry in entries_for_skill(state, slug=slug, skill_id=skill_id): + remaining = [f for f in entry.files if f.path != path] + if len(remaining) != len(entry.files): + entry.files = remaining + touched = True + return touched + + # ----- scope selection + change detection ----- @@ -2440,6 +2527,31 @@ def _untracked_push_items( return items +def inline_content_field(raw: bytes) -> dict[str, str]: + """Return the wire content field carrying *raw* inline. + + Text and binary use distinct fields so the server never has to guess: + ``content`` is verbatim UTF-8 text, ``content_base64`` is base64-encoded + bytes. A short text file whose content is coincidentally valid base64 + (e.g. ``test``) must go through ``content`` so it round-trips losslessly and + its stored sha matches the sha of the bytes on disk. Bytes that are not + valid UTF-8, or that carry a NUL, are binary. + + The single decision point for both file-sending surfaces: the whole-tree + snapshot ``build_files_payload`` sends and the single-path change + ``goodeye skills put-file`` sends. Keeping it here is what stops the two + from disagreeing about whether a given file is text. + """ + try: + text = raw.decode("utf-8") + except UnicodeDecodeError: + pass + else: + if "\x00" not in text: + return {"content": text} + return {"content_base64": base64.b64encode(raw).decode("ascii")} + + def build_files_payload( skill_dir: Path, recorded_files: list[FileState] | None, @@ -2515,27 +2627,14 @@ def build_files_payload( entry["purpose"] = purpose entries.append(entry) else: - # Inline entry. Text and binary use distinct wire fields so the - # server never has to guess: ``content`` is verbatim UTF-8 text, - # ``content_base64`` is base64-encoded bytes. A short text file - # whose content is coincidentally valid base64 (e.g. ``test``) must - # go through ``content`` so it round-trips losslessly and its stored - # sha matches the on-disk sha recorded below. - try: - content_str = raw.decode("utf-8") - if "\x00" in content_str: - raise ValueError("NUL byte") - inline: dict[str, Any] = { - "path": rel, - "content": content_str, - "executable": executable, - } - except (UnicodeDecodeError, ValueError): - inline = { - "path": rel, - "content_base64": base64.b64encode(raw).decode("ascii"), - "executable": executable, - } + # Inline entry: the shared decision picks the text or binary field, + # so this snapshot and a single-path change always agree on which + # channel a given file goes through. + inline: dict[str, Any] = { + "path": rel, + **inline_content_field(raw), + "executable": executable, + } if purpose is not None: inline["purpose"] = purpose entries.append(inline) @@ -2651,9 +2750,11 @@ def _push_candidate( "body_sha256", "build_files_payload", "ensure_identity", + "entries_for_skill", "expand_target_path", "find_entry", "find_target_by_path", + "inline_content_field", "is_modified_locally", "list_targets", "load_sync_config", @@ -2664,6 +2765,8 @@ def _push_candidate( "prune_from_allowlist", "pull", "read_local_body", + "record_file_removed", + "record_file_written", "remove_target", "resolve_preset", "save_sync_config", diff --git a/src/goodeye_cli/wire.py b/src/goodeye_cli/wire.py index 10527e7..472faa5 100644 --- a/src/goodeye_cli/wire.py +++ b/src/goodeye_cli/wire.py @@ -244,6 +244,24 @@ class WorkflowSaveResult(_WireBase): next_step: str | None = None +class WorkflowFilePatchResult(_WireBase): + """Result of changing named files in a skill (PATCH /v1/skills/{id}/files). + + ``changed`` and ``deleted`` are sorted path lists; ``carried_forward`` is a + count of the files the change left untouched, not a list of them. + """ + + workflow_id: str + version: int + version_token: str + name: str + slug: str = "" + changed: list[str] = Field(default_factory=list) + deleted: list[str] = Field(default_factory=list) + carried_forward: int = 0 + authoring_notes: list[str] = Field(default_factory=list) + + class SaveWorkflowInput(_WireBase): """Flat POST /v1/skills body the CLI constructs (documentation + parity).""" diff --git a/tests/test_commands_skill_files.py b/tests/test_commands_skill_files.py new file mode 100644 index 0000000..8b6dd14 --- /dev/null +++ b/tests/test_commands_skill_files.py @@ -0,0 +1,738 @@ +"""Tests for `goodeye skills put-file` and `goodeye skills rm-file`. + +These commands change named paths in a hosted skill and keep the rest, unlike +`goodeye skills publish `, which replaces the whole tree. The suite covers +the content sources, how text and binary pick their wire field, how the +expected version token is resolved, and the local sync-index refresh that keeps +a later `sync push` from reporting drift that is not real. +""" + +from __future__ import annotations + +import base64 +import hashlib +import json as _json +from pathlib import Path + +import httpx +import pytest +import respx +from typer.testing import CliRunner + +from goodeye_cli.app import app +from goodeye_cli.config import ConfigPaths, save_credentials +from goodeye_cli.errors import ValidationFailed +from goodeye_cli.sync import ( + FileState, + SyncEntry, + SyncState, + SyncTarget, + body_sha256, + build_files_payload, + load_sync_state, + save_sync_state, + tree_push_drifted, +) + +SERVER = "https://example.test" + +_SKILL_MD = "---\nname: my-skill\ndescription: A test skill.\n---\n\nDo 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 _detail_route(*, token: str = "tok-1", slug: str = "my-skill") -> respx.Route: + """Mock the read `put-file`/`rm-file` use to resolve the current token.""" + return respx.get(f"{SERVER}/v1/skills/{slug}").mock( + return_value=httpx.Response( + 200, + json={ + "id": "skl_01", + "name": slug, + "version": 2, + "body": _SKILL_MD, + "version_token": token, + }, + ) + ) + + +def _patch_route( + *, + slug: str = "my-skill", + version: int = 3, + token: str = "tok-2", + changed: list[str] | None = None, + deleted: list[str] | None = None, + carried_forward: int = 2, +) -> respx.Route: + return respx.patch(f"{SERVER}/v1/skills/{slug}/files").mock( + return_value=httpx.Response( + 200, + json={ + "skill_id": "skl_01", + "version": version, + "version_token": token, + "name": slug, + "slug": slug, + "changed": changed if changed is not None else ["notes.md"], + "deleted": deleted if deleted is not None else [], + "carried_forward": carried_forward, + "authoring_notes": [], + }, + ) + ) + + +def _sent(route: respx.Route) -> dict: + return _json.loads(route.calls.last.request.content.decode()) + + +# ----- content sources ----- + + +@respx.mock +def test_put_file_sends_local_text_file_inline( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """--from-file sends the file's text through `content`, and names one path.""" + _setup_creds(monkeypatch, tmp_config_paths) + local = tmp_path / "notes.md" + local.write_text("fresh notes\n", encoding="utf-8") + _detail_route() + route = _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(local)] + ) + assert result.exit_code == 0, result.output + + sent = _sent(route) + assert sent["files"] == [{"path": "notes.md", "content": "fresh notes\n"}] + assert sent["delete_paths"] == [] + + +@respx.mock +def test_put_file_reads_content_from_stdin( + tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """--stdin is the other content source, for generated agent output.""" + _setup_creds(monkeypatch, tmp_config_paths) + _detail_route() + route = _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, + ["skills", "put-file", "my-skill", "notes.md", "--stdin"], + input="piped notes\n", + ) + assert result.exit_code == 0, result.output + + sent = _sent(route) + assert sent["files"] == [{"path": "notes.md", "content": "piped notes\n"}] + + +def test_put_file_rejects_both_content_sources( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """--from-file and --stdin together is a usage error, not a silent winner.""" + _setup_creds(monkeypatch, tmp_config_paths) + local = tmp_path / "notes.md" + local.write_text("fresh notes\n", encoding="utf-8") + + runner = CliRunner() + result = runner.invoke( + app, + ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(local), "--stdin"], + ) + assert result.exit_code != 0 + assert isinstance(result.exception, ValidationFailed) + + +def test_put_file_requires_a_content_source( + tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """Neither source given is a usage error: there is nothing to write.""" + _setup_creds(monkeypatch, tmp_config_paths) + + runner = CliRunner() + result = runner.invoke(app, ["skills", "put-file", "my-skill", "notes.md"]) + assert result.exit_code != 0 + assert isinstance(result.exception, ValidationFailed) + + +def test_put_file_rejects_a_missing_local_file( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + _setup_creds(monkeypatch, tmp_config_paths) + + runner = CliRunner() + result = runner.invoke( + app, + ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(tmp_path / "gone.md")], + ) + assert result.exit_code != 0 + assert isinstance(result.exception, ValidationFailed) + + +# ----- text vs binary ----- + + +@pytest.mark.parametrize( + "raw", + [ + pytest.param(b"test", id="text-that-looks-like-base64"), + pytest.param("café\n".encode(), id="utf8-text"), + pytest.param(b"\x00\x01\x02", id="nul-bytes"), + pytest.param(b"\xff\xfe\xfa", id="invalid-utf8"), + ], +) +@respx.mock +def test_put_file_picks_the_same_wire_field_as_the_snapshot_builder( + raw: bytes, tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A binary file goes out as content_base64 while text goes through content. + + The choice must match the whole-tree snapshot payload builder exactly, so a + file patched here and the same file uploaded by a directory publish are + never sent through different channels. + """ + _setup_creds(monkeypatch, tmp_config_paths) + local = tmp_path / "asset.bin" + local.write_bytes(raw) + _detail_route() + route = _patch_route(changed=["asset.bin"]) + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "asset.bin", "--from-file", str(local)] + ) + assert result.exit_code == 0, result.output + sent_entry = _sent(route)["files"][0] + + skill_dir = tmp_path / "snapshot" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text(_SKILL_MD, encoding="utf-8") + (skill_dir / "asset.bin").write_bytes(raw) + snapshot_payload, _states = build_files_payload(skill_dir, None, []) + snapshot_entry = next(e for e in snapshot_payload if e["path"] == "asset.bin") + + channels = {"content", "content_base64"} + assert channels & set(sent_entry) == channels & set(snapshot_entry) + if "content_base64" in sent_entry: + assert sent_entry["content_base64"] == base64.b64encode(raw).decode("ascii") + else: + assert sent_entry["content"] == raw.decode("utf-8") + + +# ----- carry-forward flags ----- + + +@respx.mock +def test_put_file_omits_flags_the_caller_did_not_pass( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """An unset flag must be absent from the wire entry, not sent as a default. + + On this route an absent `executable` or `purpose` means "keep what the file + already had", so sending a default would silently reset a label the caller + never mentioned. + """ + _setup_creds(monkeypatch, tmp_config_paths) + local = tmp_path / "notes.md" + local.write_text("fresh notes\n", encoding="utf-8") + _detail_route() + route = _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(local)] + ) + assert result.exit_code == 0, result.output + + entry = _sent(route)["files"][0] + assert "executable" not in entry + assert "purpose" not in entry + + +@respx.mock +def test_put_file_sends_explicit_flags( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + _setup_creds(monkeypatch, tmp_config_paths) + local = tmp_path / "run.sh" + local.write_text("#!/bin/sh\necho hi\n", encoding="utf-8") + _detail_route() + route = _patch_route(changed=["run.sh"]) + + runner = CliRunner() + result = runner.invoke( + app, + [ + "skills", + "put-file", + "my-skill", + "run.sh", + "--from-file", + str(local), + "--executable", + "--purpose", + "script", + ], + ) + assert result.exit_code == 0, result.output + + entry = _sent(route)["files"][0] + assert entry["executable"] is True + assert entry["purpose"] == "script" + + +@respx.mock +def test_put_file_sends_no_executable_as_an_explicit_false( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + _setup_creds(monkeypatch, tmp_config_paths) + local = tmp_path / "run.sh" + local.write_text("#!/bin/sh\necho hi\n", encoding="utf-8") + _detail_route() + route = _patch_route(changed=["run.sh"]) + + runner = CliRunner() + result = runner.invoke( + app, + [ + "skills", + "put-file", + "my-skill", + "run.sh", + "--from-file", + str(local), + "--no-executable", + ], + ) + assert result.exit_code == 0, result.output + assert _sent(route)["files"][0]["executable"] is False + + +# ----- token resolution ----- + + +@respx.mock +def test_put_file_resolves_the_current_token_with_a_read( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + _setup_creds(monkeypatch, tmp_config_paths) + local = tmp_path / "notes.md" + local.write_text("fresh notes\n", encoding="utf-8") + detail = _detail_route(token="tok-current") + route = _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(local)] + ) + assert result.exit_code == 0, result.output + assert detail.called + assert _sent(route)["expected_version_token"] == "tok-current" + + +@respx.mock +def test_put_file_uses_a_supplied_token_without_reading( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """--expected-version-token stays available for scripting and skips the read.""" + _setup_creds(monkeypatch, tmp_config_paths) + local = tmp_path / "notes.md" + local.write_text("fresh notes\n", encoding="utf-8") + detail = _detail_route(token="tok-current") + route = _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, + [ + "skills", + "put-file", + "my-skill", + "notes.md", + "--from-file", + str(local), + "--expected-version-token", + "tok-scripted", + ], + ) + assert result.exit_code == 0, result.output + assert not detail.called + assert _sent(route)["expected_version_token"] == "tok-scripted" + + +@respx.mock +def test_rm_file_resolves_the_current_token_with_a_read( + tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + _setup_creds(monkeypatch, tmp_config_paths) + detail = _detail_route(token="tok-current") + route = _patch_route(changed=[], deleted=["notes.md"]) + + runner = CliRunner() + result = runner.invoke(app, ["skills", "rm-file", "my-skill", "notes.md"]) + assert result.exit_code == 0, result.output + assert detail.called + + sent = _sent(route) + assert sent["expected_version_token"] == "tok-current" + assert sent["files"] == [] + assert sent["delete_paths"] == ["notes.md"] + + +@respx.mock +def test_rm_file_uses_a_supplied_token_without_reading( + tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + _setup_creds(monkeypatch, tmp_config_paths) + detail = _detail_route(token="tok-current") + route = _patch_route(changed=[], deleted=["notes.md"]) + + runner = CliRunner() + result = runner.invoke( + app, + [ + "skills", + "rm-file", + "my-skill", + "notes.md", + "--expected-version-token", + "tok-scripted", + ], + ) + assert result.exit_code == 0, result.output + assert not detail.called + assert _sent(route)["expected_version_token"] == "tok-scripted" + + +# ----- sync-index refresh ----- + + +def _write_index( + tmp_config_paths: ConfigPaths, + *, + slug: str, + target_path: str, + files: list[FileState], + body: str = _SKILL_MD, + skill_id: str = "skl_01", +) -> None: + state = SyncState( + identity="owner@example.com", + entries=[ + SyncEntry( + skill_id=skill_id, + slug=slug, + target_path=target_path, + synced_version=2, + version_token="tok-1", + body_sha256=body_sha256(body), + files=files, + ) + ], + ) + save_sync_state(state, tmp_config_paths) + + +@respx.mock +def test_put_file_updates_the_tracked_file_state( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A tracked path's recorded sha follows the write, keeping the label intact.""" + _setup_creds(monkeypatch, tmp_config_paths) + _write_index( + tmp_config_paths, + slug="my-skill", + target_path=str(tmp_path / "skills"), + files=[ + FileState(path="notes.md", sha256=_sha(b"stale\n"), executable=True, purpose="reference") + ], + ) + local = tmp_path / "notes.md" + local.write_bytes(b"fresh notes\n") + _detail_route() + _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(local)] + ) + assert result.exit_code == 0, result.output + + reloaded = load_sync_state(tmp_config_paths) + recorded = {f.path: f for f in reloaded.entries[0].files} + assert recorded["notes.md"].sha256 == _sha(b"fresh notes\n") + # Flags the caller did not pass carry the recorded value forward, matching + # what the server does with the same absent fields. + assert recorded["notes.md"].executable is True + assert recorded["notes.md"].purpose == "reference" + + +@respx.mock +def test_put_file_records_a_path_new_to_the_tracked_tree( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + _setup_creds(monkeypatch, tmp_config_paths) + _write_index( + tmp_config_paths, + slug="my-skill", + target_path=str(tmp_path / "skills"), + files=[FileState(path="other.md", sha256=_sha(b"other\n"))], + ) + local = tmp_path / "notes.md" + local.write_bytes(b"fresh notes\n") + _detail_route() + _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, + [ + "skills", + "put-file", + "my-skill", + "notes.md", + "--from-file", + str(local), + "--executable", + "--purpose", + "reference", + ], + ) + assert result.exit_code == 0, result.output + + recorded = {f.path: f for f in load_sync_state(tmp_config_paths).entries[0].files} + assert set(recorded) == {"other.md", "notes.md"} + assert recorded["notes.md"].sha256 == _sha(b"fresh notes\n") + assert recorded["notes.md"].executable is True + assert recorded["notes.md"].purpose == "reference" + assert recorded["other.md"].sha256 == _sha(b"other\n") + + +@respx.mock +def test_put_file_leaves_the_index_alone_when_no_target_tracks_the_slug( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + _setup_creds(monkeypatch, tmp_config_paths) + _write_index( + tmp_config_paths, + slug="another-skill", + skill_id="skl_other", + target_path=str(tmp_path / "skills"), + files=[FileState(path="notes.md", sha256=_sha(b"untouched\n"))], + ) + before = tmp_config_paths.sync_state_file.read_text(encoding="utf-8") + local = tmp_path / "notes.md" + local.write_bytes(b"fresh notes\n") + _detail_route() + _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(local)] + ) + assert result.exit_code == 0, result.output + assert tmp_config_paths.sync_state_file.read_text(encoding="utf-8") == before + + +@respx.mock +def test_put_file_on_the_runbook_updates_the_recorded_body_hash( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """SKILL.md is the body, not a sibling, so it never enters the file manifest.""" + _setup_creds(monkeypatch, tmp_config_paths) + _write_index( + tmp_config_paths, + slug="my-skill", + target_path=str(tmp_path / "skills"), + files=[FileState(path="notes.md", sha256=_sha(b"notes\n"))], + ) + new_body = _SKILL_MD + "\nOne more step.\n" + local = tmp_path / "SKILL.md" + local.write_text(new_body, encoding="utf-8") + _detail_route() + _patch_route(changed=["SKILL.md"]) + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "SKILL.md", "--from-file", str(local)] + ) + assert result.exit_code == 0, result.output + + entry = load_sync_state(tmp_config_paths).entries[0] + assert entry.body_sha256 == body_sha256(new_body) + assert [f.path for f in entry.files] == ["notes.md"] + + +@respx.mock +def test_put_file_updates_every_target_mirroring_the_skill( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """One skill mirrored into two targets has both copies recorded.""" + _setup_creds(monkeypatch, tmp_config_paths) + stale = FileState(path="notes.md", sha256=_sha(b"stale\n")) + state = SyncState( + identity="owner@example.com", + entries=[ + SyncEntry( + skill_id="skl_01", + slug="my-skill", + target_path=str(tmp_path / "claude"), + synced_version=2, + version_token="tok-1", + body_sha256=body_sha256(_SKILL_MD), + files=[stale], + ), + SyncEntry( + skill_id="skl_01", + slug="my-skill", + target_path=str(tmp_path / "agents"), + synced_version=2, + version_token="tok-1", + body_sha256=body_sha256(_SKILL_MD), + files=[stale], + ), + ], + ) + save_sync_state(state, tmp_config_paths) + local = tmp_path / "notes.md" + local.write_bytes(b"fresh notes\n") + _detail_route() + _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(local)] + ) + assert result.exit_code == 0, result.output + + reloaded = load_sync_state(tmp_config_paths) + assert [entry.files[0].sha256 for entry in reloaded.entries] == [ + _sha(b"fresh notes\n"), + _sha(b"fresh notes\n"), + ] + + +@respx.mock +def test_rm_file_drops_the_tracked_file_state( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + _setup_creds(monkeypatch, tmp_config_paths) + _write_index( + tmp_config_paths, + slug="my-skill", + target_path=str(tmp_path / "skills"), + files=[ + FileState(path="notes.md", sha256=_sha(b"notes\n")), + FileState(path="other.md", sha256=_sha(b"other\n")), + ], + ) + _detail_route() + _patch_route(changed=[], deleted=["notes.md"]) + + runner = CliRunner() + result = runner.invoke(app, ["skills", "rm-file", "my-skill", "notes.md"]) + assert result.exit_code == 0, result.output + + recorded = [f.path for f in load_sync_state(tmp_config_paths).entries[0].files] + assert recorded == ["other.md"] + + +@respx.mock +def test_rm_file_leaves_the_index_alone_when_no_target_tracks_the_slug( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + _setup_creds(monkeypatch, tmp_config_paths) + _write_index( + tmp_config_paths, + slug="another-skill", + skill_id="skl_other", + target_path=str(tmp_path / "skills"), + files=[FileState(path="notes.md", sha256=_sha(b"untouched\n"))], + ) + before = tmp_config_paths.sync_state_file.read_text(encoding="utf-8") + _detail_route() + _patch_route(changed=[], deleted=["notes.md"]) + + runner = CliRunner() + result = runner.invoke(app, ["skills", "rm-file", "my-skill", "notes.md"]) + assert result.exit_code == 0, result.output + assert tmp_config_paths.sync_state_file.read_text(encoding="utf-8") == before + + +@respx.mock +def test_put_file_from_a_mirrored_directory_leaves_no_push_drift( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """The whole point: writing a mirrored file must not manufacture drift. + + Editing a file inside a sync target and sending it with put-file leaves the + server holding exactly what is on disk, so the next push must see nothing to + do. Without the index refresh the recorded sha stays stale and push reports + a change that is not real. + """ + _setup_creds(monkeypatch, tmp_config_paths) + target_dir = tmp_path / "skills" + slug_dir = target_dir / "my-skill" + slug_dir.mkdir(parents=True) + (slug_dir / "SKILL.md").write_text(_SKILL_MD, encoding="utf-8") + (slug_dir / "notes.md").write_bytes(b"stale\n") + _write_index( + tmp_config_paths, + slug="my-skill", + target_path=str(target_dir), + files=[FileState(path="notes.md", sha256=_sha(b"stale\n"))], + ) + target = SyncTarget(path=str(target_dir), scope="owned") + assert not tree_push_drifted(load_sync_state(tmp_config_paths).entries[0], target, []) + + # The user edits the mirrored file and sends just that path. + (slug_dir / "notes.md").write_bytes(b"fresh notes\n") + _detail_route() + _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, + ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(slug_dir / "notes.md")], + ) + assert result.exit_code == 0, result.output + + assert not tree_push_drifted(load_sync_state(tmp_config_paths).entries[0], target, []) + + +# ----- help text ----- + + +def test_put_file_help_contrasts_with_publish(plain) -> None: + runner = CliRunner() + result = runner.invoke(app, ["skills", "put-file", "--help"]) + assert result.exit_code == 0, result.output + text = plain(result.output) + assert "publish" in text + assert "—" not in text + + +def test_rm_file_help_contrasts_with_publish(plain) -> None: + runner = CliRunner() + result = runner.invoke(app, ["skills", "rm-file", "--help"]) + assert result.exit_code == 0, result.output + text = plain(result.output) + assert "publish" in text + assert "—" not in text From 0146f714c22d544d87e9603d75d2c5fa788b8503 Mon Sep 17 00:00:00 2001 From: Randy Olson Date: Wed, 22 Jul 2026 07:47:01 -0700 Subject: [PATCH 2/5] chore: bump version to 0.25.2 Co-Authored-By: Claude Opus 4.8 (1M context) --- pyproject.toml | 2 +- uv.lock | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index fc6d737..f5e6008 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "goodeye" -version = "0.25.1" +version = "0.25.2" 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 26fefa9..2dbe072 100644 --- a/uv.lock +++ b/uv.lock @@ -176,7 +176,7 @@ wheels = [ [[package]] name = "goodeye" -version = "0.25.1" +version = "0.25.2" source = { editable = "." } dependencies = [ { name = "httpx" }, From 36622c29c495b4bdf7fe9b0ae7dfec5698923a0a Mon Sep 17 00:00:00 2001 From: Randy Olson Date: Wed, 22 Jul 2026 08:11:20 -0700 Subject: [PATCH 3/5] Keep the local sync index consistent after a single-path file change The per-file refresh after put-file and rm-file updated every mirror of the skill while advancing the recorded version on none of them, leaving each index claiming content from the new version and a sync point at the old one. The next push then sent a token the server had already replaced, was told it conflicted, and pointed the user at a pull that refuses whenever there is also a local edit, so the only way forward was a forced pull that discards that edit. No second writer was involved: the conflict was manufactured by the index. The refresh now applies one predicate. A mirror is updated only when its recorded version token matches the one the change was written against, and such a mirror moves its file state and its sync point together onto the version the change produced. Mirrors recorded at any other version are left for a pull. Two smaller fixes ride along. A path absent from the recorded manifest is no longer recorded at all: the response carries no per-file metadata, and invented executable and purpose values would be read back by the next full push and sent, clearing what the server holds. And the recorded hash for a SKILL.md write now applies the same newline translation that reading the body back applies, so a runbook with CRLF line endings no longer reports drift that can never clear. Also strengthens the help-text tests to assert the contrast with publish rather than the bare word, and covers the four server error statuses for both commands. --- src/goodeye_cli/commands/skills.py | 40 ++-- src/goodeye_cli/sync.py | 175 ++++++++++------ tests/test_commands_skill_files.py | 315 +++++++++++++++++++++++++---- 3 files changed, 416 insertions(+), 114 deletions(-) diff --git a/src/goodeye_cli/commands/skills.py b/src/goodeye_cli/commands/skills.py index 6e5d0dc..a74b66d 100644 --- a/src/goodeye_cli/commands/skills.py +++ b/src/goodeye_cli/commands/skills.py @@ -2,7 +2,6 @@ from __future__ import annotations -import hashlib import logging import re import sys @@ -659,39 +658,49 @@ def _resolve_expected_version_token( def _refresh_sync_index( result: WorkflowFilePatchResult, *, + expected_version_token: str, path: str, - sha256: str | None, + content: bytes | None, executable: bool | None = None, purpose: str | None = None, ) -> None: """Bring the local sync index in line with the change that just landed. - A tracked mirror records a hash per file. Leaving it stale after a - single-path change makes the next `goodeye skills sync push` report drift - for a file the registry already holds, so the change would introduce false - drift on its own. When no target tracks this skill there is nothing to - record and the index is left untouched. + A tracked mirror records a hash per file plus the version it is synced at. + Leaving either stale after a single-path change makes the next + `goodeye skills sync push` report a change that is not real, so the command + would introduce false drift on its own. + + Only mirrors recorded at the version the change was written against are + updated, and each one is moved onto the new version along with the file it + records. A mirror sitting at any other version has a different base and is + left for a pull. When no mirror matches there is nothing to record and the + index is left untouched. + + ``content`` is the bytes just written, or None for a removal. Best-effort: the write already succeeded, so an unreadable or unwritable index is reported and moves on rather than failing the command. """ from goodeye_cli import sync - slug = result.slug or result.name try: paths = get_config_paths() state = sync.load_sync_state(paths) - if sha256 is None: + if content is None: touched = sync.record_file_removed( - state, slug=slug, skill_id=result.workflow_id, path=path + state, + result, + expected_version_token=expected_version_token, + path=path, ) else: touched = sync.record_file_written( state, - slug=slug, - skill_id=result.workflow_id, + result, + expected_version_token=expected_version_token, path=path, - sha256=sha256, + content=content, executable=executable, purpose=purpose, ) @@ -817,8 +826,9 @@ def put_file( _print_file_change(result) _refresh_sync_index( result, + expected_version_token=token, path=path, - sha256=hashlib.sha256(raw).hexdigest(), + content=raw, executable=executable, purpose=purpose, ) @@ -861,7 +871,7 @@ def rm_file( ) _print_file_change(result) - _refresh_sync_index(result, path=path, sha256=None) + _refresh_sync_index(result, expected_version_token=token, path=path, content=None) @app.command("lineage") diff --git a/src/goodeye_cli/sync.py b/src/goodeye_cli/sync.py index 45db1d2..9125421 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 WorkflowSummary + from goodeye_cli.wire import WorkflowFilePatchResult, WorkflowSummary SyncScope = Literal["owned", "all", "selected"] @@ -697,90 +697,151 @@ def upsert_entry(state: SyncState, entry: SyncEntry) -> None: state.entries.append(entry) -def entries_for_skill(state: SyncState, *, slug: str, skill_id: str) -> list[SyncEntry]: - """Return every tracked entry for one skill, across all targets. +def entries_at_version( + state: SyncState, *, slug: str, skill_id: str, version_token: str +) -> list[SyncEntry]: + """Return the tracked entries for one skill that sit at ``version_token``. + + The same skill may be mirrored into more than one target, and those copies + can be recorded at different versions. A single-path change is made against + exactly one version, so only the copies recorded at that version describe + the content it started from and can be moved onto the version it produced. + A copy recorded at any other version is a different base: it is left + untouched for a pull to reconcile. + + Matching on either identifier keeps a mirror recognized when one of them has + moved on: an entry written before a rename still carries the old slug under + the same id, and an entry from an older index may carry the slug the caller + typed while its id is what the registry returned. + """ + return [ + entry + for entry in state.entries + if (entry.slug == slug or entry.skill_id == skill_id) + and entry.version_token == version_token + ] + + +def _recorded_body_sha256(raw: bytes) -> str: + """Return the body hash for raw ``SKILL.md`` bytes as a later read recomputes it. + + Every reader of ``body_sha256`` compares against a hash taken over the body + read back from disk as text, and reading text translates ``\\r\\n`` and + ``\\r`` to ``\\n``. Hashing the bytes exactly as sent would leave a runbook + with CRLF line endings reporting drift on every status and push, forever, + with no local edit behind it: precisely the false drift the single-path + commands exist to remove. Applying the same translation here puts the + recorded hash on the same footing as the one it will be compared against. + """ + text = raw.decode("utf-8") + return body_sha256(text.replace("\r\n", "\n").replace("\r", "\n")) + - The same skill may be mirrored into more than one target, so a change to it - touches every copy. Matching on either identifier keeps a mirror recognized - when one of them has moved on: an entry written before a rename still - carries the old slug under the same id, and an entry from an older index - may carry the slug the caller typed while its id is what the registry - returned. +def _move_to_new_version(entry: SyncEntry, result: WorkflowFilePatchResult) -> 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 + the sync point has to move too. Leaving the superseded token behind makes + the next push send a token the server has already replaced: the server + rejects it, the push reports a conflict pointing at a pull, and that pull + refuses too whenever there is also a local edit (a local edit plus a moved + server is the skipped-conflict case). The only way out is then a forced + pull, which discards the local edit. No second writer appears anywhere in + that story: the conflict would be manufactured entirely by an index + describing a version that no longer exists. """ - return [entry for entry in state.entries if entry.slug == slug or entry.skill_id == skill_id] + entry.synced_version = result.version + entry.version_token = result.version_token def record_file_written( state: SyncState, + result: WorkflowFilePatchResult, *, - slug: str, - skill_id: str, + expected_version_token: str, path: str, - sha256: str, + content: bytes, executable: bool | None = None, purpose: str | None = None, ) -> bool: - """Record that ``path`` now holds content hashing to ``sha256``. + """Record a single-path write on every mirror that sat on the version it changed. - Called after a single-path write lands on the registry so the index matches - what the server holds. Without it the recorded manifest keeps the previous - hash and the next push reports drift for a file that is already current. + Called after the write lands on the registry so the index matches what the + server now holds. Without it the recorded manifest keeps the previous hash + and the next push reports drift for a file that is already current. - ``executable`` and ``purpose`` are carry-forward sentinels, mirroring the - write itself: ``None`` keeps the value already recorded for that path (or - falls back to ``False`` / ``None`` for a path new to the manifest), and an - explicit value replaces it. + Each entry is updated whole or not at all: content and sync point move + together, so an entry never claims content from one version while claiming + to be synced at another. ``SKILL.md`` is the body rather than a sibling, so it updates ``body_sha256`` and never enters ``files``; recording it as a file would manufacture permanent drift, since the on-disk walk never yields it. + ``executable`` and ``purpose`` are carry-forward sentinels, mirroring the + write itself: ``None`` keeps the value already recorded for that path, and + an explicit value replaces it. A path absent from the recorded manifest has + no value to carry forward and the response carries no metadata to consult, + so it is left out of the manifest rather than recorded with an invented + ``False`` / ``None``. Inventing them would be worse than leaving the path + unrecorded: a full push reads the recorded flags back and would send the + invented ones, clearing an executable bit or a role label the server holds. + Left out, the path reads as ordinary drift and the next full push sends it + with the values observed on disk. + Returns whether any entry changed, so the caller can skip persisting an - index no target tracks this skill in. + index that no target tracks this skill in at this version. """ + is_body = path == "SKILL.md" + new_hash = _recorded_body_sha256(content) if is_body else hashlib.sha256(content).hexdigest() touched = False - for entry in entries_for_skill(state, slug=slug, skill_id=skill_id): - if path == "SKILL.md": - entry.body_sha256 = sha256 - touched = True - continue - recorded = next((f for f in entry.files if f.path == path), None) - updated = FileState( - path=path, - sha256=sha256, - executable=( - executable - if executable is not None - else (recorded.executable if recorded is not None else False) - ), - purpose=( - purpose - if purpose is not None - else (recorded.purpose if recorded is not None else None) - ), - ) - if recorded is None: - entry.files.append(updated) + for entry in entries_at_version( + state, + slug=result.slug or result.name, + skill_id=result.workflow_id, + version_token=expected_version_token, + ): + if is_body: + entry.body_sha256 = new_hash else: - entry.files[entry.files.index(recorded)] = updated - entry.files.sort(key=lambda f: f.path) + recorded = next((f for f in entry.files if f.path == path), None) + if recorded is not None: + entry.files[entry.files.index(recorded)] = FileState( + path=path, + sha256=new_hash, + executable=executable if executable is not None else recorded.executable, + purpose=purpose if purpose is not None else recorded.purpose, + ) + _move_to_new_version(entry, result) touched = True return touched -def record_file_removed(state: SyncState, *, slug: str, skill_id: str, path: str) -> bool: - """Drop ``path`` from every tracked copy of a skill's recorded manifest. +def record_file_removed( + state: SyncState, + result: WorkflowFilePatchResult, + *, + expected_version_token: str, + path: str, +) -> bool: + """Drop ``path`` from every mirror that sat on the version the removal changed. - The mirror image of ``record_file_written`` for a removal: a path left in - the manifest after it is gone from the registry reads as a local deletion - on the next push. Returns whether any entry changed. + The mirror image of ``record_file_written``: a path left in the manifest + after it is gone from the registry reads as a local deletion on the next + push. Entries recorded at another version are left for a pull, and an + updated entry's sync point moves onto the version the removal created. + Returns whether any entry changed. """ touched = False - for entry in entries_for_skill(state, slug=slug, skill_id=skill_id): - remaining = [f for f in entry.files if f.path != path] - if len(remaining) != len(entry.files): - entry.files = remaining - touched = True + for entry in entries_at_version( + state, + slug=result.slug or result.name, + skill_id=result.workflow_id, + version_token=expected_version_token, + ): + entry.files = [f for f in entry.files if f.path != path] + _move_to_new_version(entry, result) + touched = True return touched @@ -2750,7 +2811,7 @@ def _push_candidate( "body_sha256", "build_files_payload", "ensure_identity", - "entries_for_skill", + "entries_at_version", "expand_target_path", "find_entry", "find_target_by_path", diff --git a/tests/test_commands_skill_files.py b/tests/test_commands_skill_files.py index 8b6dd14..6ed4311 100644 --- a/tests/test_commands_skill_files.py +++ b/tests/test_commands_skill_files.py @@ -3,8 +3,11 @@ These commands change named paths in a hosted skill and keep the rest, unlike `goodeye skills publish `, which replaces the whole tree. The suite covers the content sources, how text and binary pick their wire field, how the -expected version token is resolved, and the local sync-index refresh that keeps -a later `sync push` from reporting drift that is not real. +expected version token is resolved, how the server's errors surface, and the +local sync-index refresh that keeps a later `sync push` from reporting drift +that is not real. That refresh is version-aware: it moves the mirrors recorded +at the version the change was written against onto the version it produced, and +leaves mirrors at any other version for a pull. """ from __future__ import annotations @@ -21,7 +24,7 @@ from goodeye_cli.app import app from goodeye_cli.config import ConfigPaths, save_credentials -from goodeye_cli.errors import ValidationFailed +from goodeye_cli.errors import Conflict, Forbidden, GoodeyeError, NotFound, ValidationFailed from goodeye_cli.sync import ( FileState, SyncEntry, @@ -29,7 +32,9 @@ SyncTarget, body_sha256, build_files_payload, + is_modified_locally, load_sync_state, + read_local_body, save_sync_state, tree_push_drifted, ) @@ -97,6 +102,19 @@ def _sent(route: respx.Route) -> dict: return _json.loads(route.calls.last.request.content.decode()) +def _me_route(email: str = "owner@example.com") -> respx.Route: + """Mock the read the sync identity guard makes before a push.""" + return respx.get(f"{SERVER}/v1/me").mock( + return_value=httpx.Response(200, json={"email": email}) + ) + + +def _seed_target(path: str) -> None: + """Configure a sync target through the CLI, as a user would.""" + add = CliRunner().invoke(app, ["skills", "sync", "target", "add", path, "--scope", "owned"]) + assert add.exit_code == 0, add.output + + # ----- content sources ----- @@ -123,9 +141,7 @@ def test_put_file_sends_local_text_file_inline( @respx.mock -def test_put_file_reads_content_from_stdin( - tmp_config_paths: ConfigPaths, monkeypatch -) -> None: +def test_put_file_reads_content_from_stdin(tmp_config_paths: ConfigPaths, monkeypatch) -> None: """--stdin is the other content source, for generated agent output.""" _setup_creds(monkeypatch, tmp_config_paths) _detail_route() @@ -160,9 +176,7 @@ def test_put_file_rejects_both_content_sources( assert isinstance(result.exception, ValidationFailed) -def test_put_file_requires_a_content_source( - tmp_config_paths: ConfigPaths, monkeypatch -) -> None: +def test_put_file_requires_a_content_source(tmp_config_paths: ConfigPaths, monkeypatch) -> None: """Neither source given is a usage error: there is nothing to write.""" _setup_creds(monkeypatch, tmp_config_paths) @@ -461,7 +475,9 @@ def test_put_file_updates_the_tracked_file_state( slug="my-skill", target_path=str(tmp_path / "skills"), files=[ - FileState(path="notes.md", sha256=_sha(b"stale\n"), executable=True, purpose="reference") + FileState( + path="notes.md", sha256=_sha(b"stale\n"), executable=True, purpose="reference" + ) ], ) local = tmp_path / "notes.md" @@ -475,19 +491,32 @@ def test_put_file_updates_the_tracked_file_state( ) assert result.exit_code == 0, result.output - reloaded = load_sync_state(tmp_config_paths) - recorded = {f.path: f for f in reloaded.entries[0].files} + entry = load_sync_state(tmp_config_paths).entries[0] + recorded = {f.path: f for f in entry.files} assert recorded["notes.md"].sha256 == _sha(b"fresh notes\n") # Flags the caller did not pass carry the recorded value forward, matching # what the server does with the same absent fields. assert recorded["notes.md"].executable is True assert recorded["notes.md"].purpose == "reference" + # The content and the sync point move together: an entry that claimed the + # new content while still claiming the old version would send a superseded + # token on the next push and be told it conflicts. + assert entry.synced_version == 3 + assert entry.version_token == "tok-2" @respx.mock -def test_put_file_records_a_path_new_to_the_tracked_tree( +def test_put_file_leaves_a_manifest_new_path_out_of_the_record( tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch ) -> None: + """A path the manifest never held is not recorded on a guess. + + The response carries no per-file metadata, so there is nothing to record the + executable mark and role label from. Writing invented values would be worse + than recording nothing: a full push reads those flags back out of the index + and would send the invented ones, clearing what the server holds. Left out, + the path reads as ordinary drift that the next full push resolves from disk. + """ _setup_creds(monkeypatch, tmp_config_paths) _write_index( tmp_config_paths, @@ -502,27 +531,17 @@ def test_put_file_records_a_path_new_to_the_tracked_tree( runner = CliRunner() result = runner.invoke( - app, - [ - "skills", - "put-file", - "my-skill", - "notes.md", - "--from-file", - str(local), - "--executable", - "--purpose", - "reference", - ], + app, ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(local)] ) assert result.exit_code == 0, result.output - recorded = {f.path: f for f in load_sync_state(tmp_config_paths).entries[0].files} - assert set(recorded) == {"other.md", "notes.md"} - assert recorded["notes.md"].sha256 == _sha(b"fresh notes\n") - assert recorded["notes.md"].executable is True - assert recorded["notes.md"].purpose == "reference" + entry = load_sync_state(tmp_config_paths).entries[0] + recorded = {f.path: f for f in entry.files} + assert set(recorded) == {"other.md"} assert recorded["other.md"].sha256 == _sha(b"other\n") + # The version still moved: the registry did write a new version. + assert entry.synced_version == 3 + assert entry.version_token == "tok-2" @respx.mock @@ -627,6 +646,67 @@ def test_put_file_updates_every_target_mirroring_the_skill( _sha(b"fresh notes\n"), _sha(b"fresh notes\n"), ] + assert [entry.version_token for entry in reloaded.entries] == ["tok-2", "tok-2"] + + +@respx.mock +def test_put_file_leaves_a_target_recorded_at_another_version_untouched( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """Two mirrors of one skill can sit at different versions. + + The change is written against one version, so only the mirrors recorded at + that version describe the content it started from. A mirror left behind at + an older version has a different base: recording the new file state on it + would claim content it was never given, and moving its sync point forward + would claim a version it never received. It is left for a pull. + """ + _setup_creds(monkeypatch, tmp_config_paths) + behind = SyncEntry( + skill_id="skl_01", + slug="my-skill", + target_path=str(tmp_path / "agents"), + synced_version=1, + version_token="tok-older", + body_sha256=body_sha256("an older body\n"), + files=[FileState(path="notes.md", sha256=_sha(b"older\n"), purpose="reference")], + ) + state = SyncState( + identity="owner@example.com", + entries=[ + SyncEntry( + skill_id="skl_01", + slug="my-skill", + target_path=str(tmp_path / "claude"), + synced_version=2, + version_token="tok-1", + body_sha256=body_sha256(_SKILL_MD), + files=[FileState(path="notes.md", sha256=_sha(b"stale\n"))], + ), + behind, + ], + ) + save_sync_state(state, tmp_config_paths) + before_behind = behind.model_dump() + local = tmp_path / "notes.md" + local.write_bytes(b"fresh notes\n") + _detail_route(token="tok-1") + _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(local)] + ) + assert result.exit_code == 0, result.output + + reloaded = {entry.target_path: entry for entry in load_sync_state(tmp_config_paths).entries} + current = reloaded[str(tmp_path / "claude")] + assert current.files[0].sha256 == _sha(b"fresh notes\n") + assert current.synced_version == 3 + assert current.version_token == "tok-2" + # Nothing about the older mirror changed, not the file state and not the + # version it reports being synced at. + assert reloaded[str(tmp_path / "agents")].model_dump() == before_behind @respx.mock @@ -650,8 +730,10 @@ def test_rm_file_drops_the_tracked_file_state( result = runner.invoke(app, ["skills", "rm-file", "my-skill", "notes.md"]) assert result.exit_code == 0, result.output - recorded = [f.path for f in load_sync_state(tmp_config_paths).entries[0].files] - assert recorded == ["other.md"] + entry = load_sync_state(tmp_config_paths).entries[0] + assert [f.path for f in entry.files] == ["other.md"] + assert entry.synced_version == 3 + assert entry.version_token == "tok-2" @respx.mock @@ -717,22 +799,171 @@ def test_put_file_from_a_mirrored_directory_leaves_no_push_drift( assert not tree_push_drifted(load_sync_state(tmp_config_paths).entries[0], target, []) -# ----- help text ----- +@respx.mock +def test_put_file_then_a_further_local_edit_pushes_cleanly( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """The ordinary authoring loop: send one file, keep editing, then push. + The push must go out against the version the change created. Sending the + superseded one is rejected by the server, and the resulting conflict has no + way out: the pull it points at refuses while a local edit is present, so the + only unblock is a forced pull, which discards that edit. No second writer + appears in this story, so any conflict here would be manufactured. + """ + _setup_creds(monkeypatch, tmp_config_paths) + _me_route() + target_dir = tmp_path / "skills" + _seed_target(str(target_dir)) + slug_dir = target_dir / "my-skill" + slug_dir.mkdir(parents=True) + (slug_dir / "SKILL.md").write_text(_SKILL_MD, encoding="utf-8") + (slug_dir / "notes.md").write_bytes(b"stale\n") + _write_index( + tmp_config_paths, + slug="my-skill", + target_path=str(target_dir), + files=[FileState(path="notes.md", sha256=_sha(b"stale\n"))], + ) -def test_put_file_help_contrasts_with_publish(plain) -> None: + (slug_dir / "notes.md").write_bytes(b"fresh notes\n") + _detail_route() + _patch_route() runner = CliRunner() - result = runner.invoke(app, ["skills", "put-file", "--help"]) + patched = runner.invoke( + app, + ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(slug_dir / "notes.md")], + ) + assert patched.exit_code == 0, patched.output + + # The author keeps working: one more edit before pushing. + (slug_dir / "SKILL.md").write_text(_SKILL_MD + "\nOne more step.\n", encoding="utf-8") + save_route = respx.post(f"{SERVER}/v1/skills").mock( + return_value=httpx.Response( + 200, + json={ + "skill_id": "skl_01", + "version": 4, + "name": "my-skill", + "version_token": "tok-3", + "verifiers": [], + }, + ) + ) + pushed = runner.invoke(app, ["skills", "sync", "push"]) + assert pushed.exit_code == 0, pushed.output + + item = _json.loads(pushed.output)["items"][0] + assert item["action"] == "pushed" + assert _sent(save_route)["expected_version_token"] == "tok-2" + + +@respx.mock +def test_put_file_on_a_crlf_runbook_records_the_hash_a_read_recomputes( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A runbook with CRLF line endings must not be left drifting forever. + + The recorded body hash is compared against one recomputed from the body read + back as text, and that read translates CRLF to LF. Recording the hash of the + bytes as sent would never match it again, so the entry would report drift on + every status and push with no local edit behind it. + """ + _setup_creds(monkeypatch, tmp_config_paths) + target_dir = tmp_path / "skills" + slug_dir = target_dir / "my-skill" + slug_dir.mkdir(parents=True) + crlf_body = _SKILL_MD.replace("\n", "\r\n") + (slug_dir / "SKILL.md").write_bytes(crlf_body.encode("utf-8")) + _write_index( + tmp_config_paths, + slug="my-skill", + target_path=str(target_dir), + files=[], + body="an older body\n", + ) + _detail_route() + _patch_route(changed=["SKILL.md"]) + + runner = CliRunner() + result = runner.invoke( + app, + ["skills", "put-file", "my-skill", "SKILL.md", "--from-file", str(slug_dir / "SKILL.md")], + ) + assert result.exit_code == 0, result.output + + entry = load_sync_state(tmp_config_paths).entries[0] + target = SyncTarget(path=str(target_dir), scope="owned") + assert not is_modified_locally(entry, read_local_body(target, "my-skill")) + assert not tree_push_drifted(entry, target, []) + + +# ----- server errors ----- + + +@pytest.mark.parametrize( + ("status", "slug", "expected"), + [ + pytest.param(400, "validation_error", ValidationFailed, id="rejected-request"), + pytest.param(409, "conflict", Conflict, id="version-moved"), + pytest.param(403, "forbidden", Forbidden, id="no-edit-access"), + pytest.param(404, "not_found", NotFound, id="unknown-skill"), + ], +) +@pytest.mark.parametrize("command", ["put-file", "rm-file"]) +@respx.mock +def test_file_commands_surface_server_errors( + command: str, + status: int, + slug: str, + expected: type[GoodeyeError], + tmp_path: Path, + tmp_config_paths: ConfigPaths, + monkeypatch, +) -> None: + """A rejected change fails the command with the server's own error.""" + _setup_creds(monkeypatch, tmp_config_paths) + local = tmp_path / "notes.md" + local.write_bytes(b"fresh notes\n") + _detail_route() + respx.patch(f"{SERVER}/v1/skills/my-skill/files").mock( + return_value=httpx.Response(status, json={"error": slug, "message": "Nope."}) + ) + + args = ["skills", command, "my-skill", "notes.md"] + if command == "put-file": + args += ["--from-file", str(local)] + + result = CliRunner().invoke(app, args) + assert result.exit_code != 0 + assert isinstance(result.exception, expected) + assert result.exception.slug == slug + + +# ----- help text ----- + + +def _help_text(plain, command: str) -> str: + """Return the command's help as one line, so wrapping cannot hide a phrase.""" + result = CliRunner().invoke(app, ["skills", command, "--help"]) assert result.exit_code == 0, result.output - text = plain(result.output) - assert "publish" in text + return " ".join(plain(result.output).split()) + + +def test_put_file_help_contrasts_with_publish(plain) -> None: + """Naming publish is not enough: the help has to say how the two differ.""" + text = _help_text(plain, "put-file") + assert "Only the path you name changes" in text + assert "the rest of the skill's files ride forward untouched" in text + assert "unlike `goodeye skills publish `, which replaces the whole tree" in text + assert "any path missing from the directory is deleted" in text assert "—" not in text def test_rm_file_help_contrasts_with_publish(plain) -> None: - runner = CliRunner() - result = runner.invoke(app, ["skills", "rm-file", "--help"]) - assert result.exit_code == 0, result.output - text = plain(result.output) - assert "publish" in text + text = _help_text(plain, "rm-file") + assert "Only the path you name is removed" in text + assert "the rest of the skill's files ride forward untouched" in text + assert "unlike `goodeye skills publish `, which replaces the whole tree" in text + assert "any path missing from the directory is deleted" in text assert "—" not in text From 313fe514415f575eff8f1a0170d0dc43694d9efc Mon Sep 17 00:00:00 2001 From: Randy Olson Date: Wed, 22 Jul 2026 09:51:56 -0700 Subject: [PATCH 4/5] Move the mirrored directory with the index after a single-path change The refresh advanced the recorded manifest and the sync point but left the directory those describe untouched, so the index claimed content the mirror did not hold. Every route out of that state loses the change. The push builds its snapshot from the directory, so a stale copy is sent straight back: after rm-file the removed file is uploaded again and the removal is undone, and after a put-file whose content came from anywhere other than the mirror the old bytes overwrite the new ones. The pull that would repair it sees a sync point already at the version the change produced and either reports the mirror up to date or, once the directory counts as locally modified, refuses it, so neither surface reconciles. The refresh now moves the directory with the index. A mirror updated by the change has the bytes written into it, or the removed path taken out of it, at the same time as its recorded file state and sync point; a mirror recorded at another version is still left for a pull. The mark applied to the local copy is the one the caller passed, else the one already recorded, and nothing is written where the skill has no directory under the target: that mirror was never materialized and the pull path fetches it whole. Covers the three cases the previous shape got wrong: a put-file sourced from outside the mirror, the same for the runbook, and an rm-file, each asserted to leave the directory correct and the next push with nothing to do. --- src/goodeye_cli/commands/skills.py | 25 ++--- src/goodeye_cli/sync.py | 98 +++++++++++++++--- tests/test_commands_skill_files.py | 157 ++++++++++++++++++++++++++++- 3 files changed, 253 insertions(+), 27 deletions(-) diff --git a/src/goodeye_cli/commands/skills.py b/src/goodeye_cli/commands/skills.py index a74b66d..c8f452f 100644 --- a/src/goodeye_cli/commands/skills.py +++ b/src/goodeye_cli/commands/skills.py @@ -664,23 +664,24 @@ def _refresh_sync_index( executable: bool | None = None, purpose: str | None = None, ) -> None: - """Bring the local sync index in line with the change that just landed. + """Bring the local mirror in line with the change that just landed. - A tracked mirror records a hash per file plus the version it is synced at. - Leaving either stale after a single-path change makes the next - `goodeye skills sync push` report a change that is not real, so the command - would introduce false drift on its own. + A tracked mirror is a directory on disk plus an index recording a hash per + file and the version it is synced at. Leaving any of that stale after a + single-path change makes the next `goodeye skills sync push` report a change + that is not real, or send the old copy back and revert the one just made, so + the command would work against itself. Only mirrors recorded at the version the change was written against are - updated, and each one is moved onto the new version along with the file it - records. A mirror sitting at any other version has a different base and is - left for a pull. When no mirror matches there is nothing to record and the - index is left untouched. + updated, and each one moves its directory, its recorded file state, and its + sync point together onto the new version. A mirror sitting at any other + version has a different base and is left for a pull. When no mirror matches + there is nothing to update and the index is left untouched. ``content`` is the bytes just written, or None for a removal. - Best-effort: the write already succeeded, so an unreadable or unwritable - index is reported and moves on rather than failing the command. + Best-effort: the write already succeeded, so an unreadable index or an + unwritable mirror is reported and moves on rather than failing the command. """ from goodeye_cli import sync @@ -709,7 +710,7 @@ def _refresh_sync_index( except Exception: _log.debug("could not refresh the local sync index", exc_info=True) Console(stderr=True).print( - "[yellow]Note[/yellow] the local sync index could not be updated; " + "[yellow]Note[/yellow] the local copy could not be updated; " "run `goodeye skills sync pull` to resync." ) diff --git a/src/goodeye_cli/sync.py b/src/goodeye_cli/sync.py index 9125421..bb390dc 100644 --- a/src/goodeye_cli/sync.py +++ b/src/goodeye_cli/sync.py @@ -737,6 +737,64 @@ def _recorded_body_sha256(raw: bytes) -> str: return body_sha256(text.replace("\r\n", "\n").replace("\r", "\n")) +def _mirrored_path(entry: SyncEntry, path: str) -> Path | None: + """Return where ``entry`` keeps ``path`` on disk, or None when there is no copy. + + None when the skill has no directory under the target (nothing was ever + mirrored, so there is nothing to keep in step and a pull will materialize + it) or when the path fails the containment check every other writer here + applies. + """ + slug_dir = expand_target_path(entry.target_path) / entry.slug + if not slug_dir.is_dir() or not _is_safe_sibling_path(slug_dir, path): + return None + return slug_dir / path + + +def _write_mirrored_file( + entry: SyncEntry, path: str, content: bytes, *, executable: bool | None +) -> None: + """Put the bytes just written to the registry into this entry's local copy. + + The recorded manifest and the directory it describes have to move together. + Advancing one without the other is what turns a change into its own undoing: + a push builds its snapshot from the directory, so a stale copy on disk is + sent straight back and reverts the change, while a pull sees a sync point + already at the new version and reports the mirror up to date or refuses it + as locally modified. Neither reconciles, and the file the caller just wrote + is lost. + + Written as raw bytes so the local copy is what the registry was given. + ``executable`` is applied only when it is known, so a file whose mark was + neither passed nor recorded keeps whatever mode it already had. + """ + local = _mirrored_path(entry, path) + if local is None: + return + local.parent.mkdir(parents=True, exist_ok=True) + local.write_bytes(content) + if executable is not None: + mode = os.stat(local).st_mode + os.chmod(local, (mode | 0o111) if executable else (mode & ~0o111)) + + +def _remove_mirrored_file(entry: SyncEntry, path: str) -> None: + """Take a removed path out of this entry's local copy. + + The counterpart to ``_write_mirrored_file``: a file left on disk after it is + gone from the registry is re-sent by the next push, which restores it and + undoes the removal, while a pull reports the mirror up to date and leaves it + sitting there. + """ + local = _mirrored_path(entry, path) + if local is None or not local.exists(): + return + try: + local.unlink() + except OSError as exc: + _log.warning("could not remove %s from the local copy: %s", local, exc) + + def _move_to_new_version(entry: SyncEntry, result: WorkflowFilePatchResult) -> None: """Move an entry's recorded sync point onto the version the change created. @@ -766,13 +824,17 @@ def record_file_written( ) -> bool: """Record a single-path write on every mirror that sat on the version it changed. - Called after the write lands on the registry so the index matches what the - server now holds. Without it the recorded manifest keeps the previous hash - and the next push reports drift for a file that is already current. + Called after the write lands on the registry so the local copy matches what + the server now holds. Without it the recorded manifest keeps the previous + hash and the next push reports drift for a file that is already current. - Each entry is updated whole or not at all: content and sync point move - together, so an entry never claims content from one version while claiming - to be synced at another. + Each entry is updated whole or not at all: the recorded manifest, the file + on disk, and the sync point all move together, so an entry never claims + content from one version while claiming to be synced at another, and never + describes content its directory does not hold. The bytes go to disk even + when they came from somewhere outside the mirror, which is the case where + leaving the directory behind would let the next push send the old copy back + and revert the write. ``SKILL.md`` is the body rather than a sibling, so it updates ``body_sha256`` and never enters ``files``; recording it as a file would @@ -786,8 +848,8 @@ def record_file_written( ``False`` / ``None``. Inventing them would be worse than leaving the path unrecorded: a full push reads the recorded flags back and would send the invented ones, clearing an executable bit or a role label the server holds. - Left out, the path reads as ordinary drift and the next full push sends it - with the values observed on disk. + Left out but still written to disk, the path reads as ordinary drift and the + next full push sends it with the values observed there. Returns whether any entry changed, so the caller can skip persisting an index that no target tracks this skill in at this version. @@ -801,10 +863,19 @@ def record_file_written( skill_id=result.workflow_id, version_token=expected_version_token, ): + mirror_executable: bool | None = None if is_body: entry.body_sha256 = new_hash else: recorded = next((f for f in entry.files if f.path == path), None) + # The mark to put on the local copy: what the caller passed, else + # what is already recorded for the path. A path the manifest never + # held has neither, so the copy keeps the mode it has. + mirror_executable = ( + executable + if executable is not None + else (recorded.executable if recorded is not None else None) + ) if recorded is not None: entry.files[entry.files.index(recorded)] = FileState( path=path, @@ -812,6 +883,7 @@ def record_file_written( executable=executable if executable is not None else recorded.executable, purpose=purpose if purpose is not None else recorded.purpose, ) + _write_mirrored_file(entry, path, content, executable=mirror_executable) _move_to_new_version(entry, result) touched = True return touched @@ -826,11 +898,12 @@ def record_file_removed( ) -> bool: """Drop ``path`` from every mirror that sat on the version the removal changed. - The mirror image of ``record_file_written``: a path left in the manifest + The counterpart to ``record_file_written``: a path left in the manifest after it is gone from the registry reads as a local deletion on the next - push. Entries recorded at another version are left for a pull, and an - updated entry's sync point moves onto the version the removal created. - Returns whether any entry changed. + push, and a copy left on disk is sent back by that push and undoes the + removal, so both go. Entries recorded at another version are left for a + pull, and an updated entry's sync point moves onto the version the removal + created. Returns whether any entry changed. """ touched = False for entry in entries_at_version( @@ -840,6 +913,7 @@ def record_file_removed( version_token=expected_version_token, ): entry.files = [f for f in entry.files if f.path != path] + _remove_mirrored_file(entry, path) _move_to_new_version(entry, result) touched = True return touched diff --git a/tests/test_commands_skill_files.py b/tests/test_commands_skill_files.py index 6ed4311..e5155db 100644 --- a/tests/test_commands_skill_files.py +++ b/tests/test_commands_skill_files.py @@ -4,9 +4,10 @@ `goodeye skills publish `, which replaces the whole tree. The suite covers the content sources, how text and binary pick their wire field, how the expected version token is resolved, how the server's errors surface, and the -local sync-index refresh that keeps a later `sync push` from reporting drift -that is not real. That refresh is version-aware: it moves the mirrors recorded -at the version the change was written against onto the version it produced, and +local mirror refresh that keeps a later `sync push` from reporting drift that is +not real or sending the old copy back and reverting the change. That refresh is +version-aware: it moves the mirrors recorded at the version the change was +written against onto the version it produced, directory and index together, and leaves mirrors at any other version for a pull. """ @@ -15,6 +16,7 @@ import base64 import hashlib import json as _json +import os from pathlib import Path import httpx @@ -799,6 +801,155 @@ def test_put_file_from_a_mirrored_directory_leaves_no_push_drift( assert not tree_push_drifted(load_sync_state(tmp_config_paths).entries[0], target, []) +@respx.mock +def test_put_file_from_outside_the_mirror_updates_the_local_copy( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """Content from anywhere else still lands in the mirrored directory. + + The push builds its snapshot from that directory, so a copy left holding the + old bytes is sent straight back and reverts the change, and the pull that + would repair it sees a sync point already at the new version and refuses the + directory as locally modified instead. + """ + _setup_creds(monkeypatch, tmp_config_paths) + target_dir = tmp_path / "skills" + slug_dir = target_dir / "my-skill" + slug_dir.mkdir(parents=True) + (slug_dir / "SKILL.md").write_text(_SKILL_MD, encoding="utf-8") + (slug_dir / "notes.md").write_bytes(b"stale\n") + _write_index( + tmp_config_paths, + slug="my-skill", + target_path=str(target_dir), + files=[ + FileState( + path="notes.md", sha256=_sha(b"stale\n"), executable=True, purpose="reference" + ) + ], + ) + source = tmp_path / "elsewhere" / "notes.md" + source.parent.mkdir() + source.write_bytes(b"fresh notes\n") + _detail_route() + _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(source)] + ) + assert result.exit_code == 0, result.output + + assert (slug_dir / "notes.md").read_bytes() == b"fresh notes\n" + # The mark the manifest already held is applied to the copy, so the file on + # disk carries what the registry holds rather than the source file's mode. + assert os.stat(slug_dir / "notes.md").st_mode & 0o100 + target = SyncTarget(path=str(target_dir), scope="owned") + assert not tree_push_drifted(load_sync_state(tmp_config_paths).entries[0], target, []) + + +@respx.mock +def test_put_file_on_the_runbook_from_outside_the_mirror_rewrites_it( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """The runbook is the body, and its mirrored copy has to move with it too.""" + _setup_creds(monkeypatch, tmp_config_paths) + target_dir = tmp_path / "skills" + slug_dir = target_dir / "my-skill" + slug_dir.mkdir(parents=True) + (slug_dir / "SKILL.md").write_text(_SKILL_MD, encoding="utf-8") + _write_index(tmp_config_paths, slug="my-skill", target_path=str(target_dir), files=[]) + new_body = _SKILL_MD + "\nOne more step.\n" + source = tmp_path / "elsewhere" / "SKILL.md" + source.parent.mkdir() + source.write_text(new_body, encoding="utf-8") + _detail_route() + _patch_route(changed=["SKILL.md"]) + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "SKILL.md", "--from-file", str(source)] + ) + assert result.exit_code == 0, result.output + + target = SyncTarget(path=str(target_dir), scope="owned") + assert read_local_body(target, "my-skill") == new_body + assert not tree_push_drifted(load_sync_state(tmp_config_paths).entries[0], target, []) + + +@respx.mock +def test_put_file_creates_no_local_copy_where_none_was_mirrored( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A target whose directory was never materialized is left for a pull. + + Writing a lone file into a directory that holds no skill would leave a + fragment with no runbook beside it, which the pull path already handles by + fetching the whole thing. + """ + _setup_creds(monkeypatch, tmp_config_paths) + target_dir = tmp_path / "skills" + _write_index( + tmp_config_paths, + slug="my-skill", + target_path=str(target_dir), + files=[FileState(path="notes.md", sha256=_sha(b"stale\n"))], + ) + source = tmp_path / "notes.md" + source.write_bytes(b"fresh notes\n") + _detail_route() + _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(source)] + ) + assert result.exit_code == 0, result.output + + assert not target_dir.exists() + entry = load_sync_state(tmp_config_paths).entries[0] + assert entry.version_token == "tok-2" + + +@respx.mock +def test_rm_file_removes_the_local_copy( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A removal has to reach the directory, or the next push puts the file back. + + A file left on disk after it is gone from the registry is part of the + snapshot the push builds, so it is uploaded again and the removal is undone. + """ + _setup_creds(monkeypatch, tmp_config_paths) + target_dir = tmp_path / "skills" + slug_dir = target_dir / "my-skill" + (slug_dir / "references").mkdir(parents=True) + (slug_dir / "SKILL.md").write_text(_SKILL_MD, encoding="utf-8") + (slug_dir / "references" / "rubric.md").write_bytes(b"rubric\n") + (slug_dir / "other.md").write_bytes(b"other\n") + _write_index( + tmp_config_paths, + slug="my-skill", + target_path=str(target_dir), + files=[ + FileState(path="other.md", sha256=_sha(b"other\n")), + FileState(path="references/rubric.md", sha256=_sha(b"rubric\n")), + ], + ) + _detail_route() + _patch_route(changed=[], deleted=["references/rubric.md"]) + + runner = CliRunner() + result = runner.invoke(app, ["skills", "rm-file", "my-skill", "references/rubric.md"]) + assert result.exit_code == 0, result.output + + assert not (slug_dir / "references" / "rubric.md").exists() + # Only the named path goes: every other file in the directory stays. + assert (slug_dir / "other.md").read_bytes() == b"other\n" + target = SyncTarget(path=str(target_dir), scope="owned") + assert not tree_push_drifted(load_sync_state(tmp_config_paths).entries[0], target, []) + + @respx.mock def test_put_file_then_a_further_local_edit_pushes_cleanly( tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch From 718f7258076c7ceb6a2e53535ccede9fc18370a0 Mon Sep 17 00:00:00 2001 From: Randy Olson Date: Wed, 22 Jul 2026 10:18:07 -0700 Subject: [PATCH 5/5] fix: keep a label set on a brand-new path from being cleared A path the local manifest never held was left out of it, so the next full push both reported drift for a file already current and sent it with no label, clearing server-side the very label the caller had just set: the push is a full snapshot and reads labels only out of the index. It is now recorded whenever the caller passed a mark or a label, where nothing has to be invented. The value passed is what the registry holds, and the other one is read off the local copy the write just made, which is what the next push would read there anyway. A path named with neither still stays out, and a path with no local copy is never recorded, so the manifest never describes content its directory does not hold. --- src/goodeye_cli/sync.py | 55 ++++++++++-- tests/test_commands_skill_files.py | 139 +++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+), 8 deletions(-) diff --git a/src/goodeye_cli/sync.py b/src/goodeye_cli/sync.py index bb390dc..37e6729 100644 --- a/src/goodeye_cli/sync.py +++ b/src/goodeye_cli/sync.py @@ -751,6 +751,20 @@ def _mirrored_path(entry: SyncEntry, path: str) -> Path | None: return slug_dir / path +def _mirrored_executable(entry: SyncEntry, path: str) -> bool | None: + """Report the execute mark on this entry's local copy, or None when it has none. + + Reads the mark rather than assuming one, for the path whose mark the caller + did not pass. It is the value the next full push would read off disk for + that file anyway, so recording it leaves what gets sent unchanged. None + means the entry keeps no copy of this path, so there is nothing to observe. + """ + local = _mirrored_path(entry, path) + if local is None or not local.exists(): + return None + return bool(os.stat(local).st_mode & 0o100) + + def _write_mirrored_file( entry: SyncEntry, path: str, content: bytes, *, executable: bool | None ) -> None: @@ -842,14 +856,20 @@ def record_file_written( ``executable`` and ``purpose`` are carry-forward sentinels, mirroring the write itself: ``None`` keeps the value already recorded for that path, and - an explicit value replaces it. A path absent from the recorded manifest has - no value to carry forward and the response carries no metadata to consult, - so it is left out of the manifest rather than recorded with an invented - ``False`` / ``None``. Inventing them would be worse than leaving the path - unrecorded: a full push reads the recorded flags back and would send the - invented ones, clearing an executable bit or a role label the server holds. - Left out but still written to disk, the path reads as ordinary drift and the - next full push sends it with the values observed there. + an explicit value replaces it. + + A path absent from the recorded manifest is recorded only when the caller + passed a mark or a label for it. Then there is nothing to invent: the value + passed is the value the registry now holds, and the other one is read off + the local copy the write just made rather than guessed. Without the record + the path is a stranger to the manifest, so the next full push both reports + drift for a file already current and sends it with no label at all, which + clears server-side the very label the caller just set (the push is a full + snapshot and reads labels only out of the index). A path the caller named + with neither a mark nor a label stays out: nothing is known about it beyond + its bytes, and a recorded ``False`` / ``None`` would be a guess that a full + push turns into a clear. Left out but still written to disk, that path + reads as ordinary drift the next full push resolves from disk. Returns whether any entry changed, so the caller can skip persisting an index that no target tracks this skill in at this version. @@ -864,6 +884,7 @@ def record_file_written( version_token=expected_version_token, ): mirror_executable: bool | None = None + recorded: FileState | None = None if is_body: entry.body_sha256 = new_hash else: @@ -884,6 +905,24 @@ def record_file_written( purpose=purpose if purpose is not None else recorded.purpose, ) _write_mirrored_file(entry, path, content, executable=mirror_executable) + if not is_body and recorded is None and (executable is not None or purpose is not None): + # The mark comes off the copy just written when the caller did not + # pass one, which is the same value the next full push would read + # there, so recording it changes nothing about what gets sent. A + # path with no local copy is not recorded at all: the manifest must + # never describe content its directory does not hold, or the next + # push reads the record as a local deletion and removes the file + # from the registry. + local_mark = _mirrored_executable(entry, path) + if local_mark is not None: + entry.files.append( + FileState( + path=path, + sha256=new_hash, + executable=executable if executable is not None else local_mark, + purpose=purpose, + ) + ) _move_to_new_version(entry, result) touched = True return touched diff --git a/tests/test_commands_skill_files.py b/tests/test_commands_skill_files.py index e5155db..8d5e6e3 100644 --- a/tests/test_commands_skill_files.py +++ b/tests/test_commands_skill_files.py @@ -546,6 +546,145 @@ def test_put_file_leaves_a_manifest_new_path_out_of_the_record( assert entry.version_token == "tok-2" +@respx.mock +def test_put_file_records_a_new_path_the_caller_labelled( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A label set on a path the manifest never held has to survive the next push. + + The push sends a full snapshot and reads labels only out of the index, so a + path missing from it goes out with no label and clears server-side the very + one just set. The mark and the label the caller passed are what the registry + now holds, so recording them invents nothing. + """ + _setup_creds(monkeypatch, tmp_config_paths) + target_dir = tmp_path / "skills" + slug_dir = target_dir / "my-skill" + slug_dir.mkdir(parents=True) + (slug_dir / "SKILL.md").write_text(_SKILL_MD, encoding="utf-8") + _write_index( + tmp_config_paths, + slug="my-skill", + target_path=str(target_dir), + files=[], + ) + source = tmp_path / "elsewhere" / "run.sh" + source.parent.mkdir() + source.write_bytes(b"#!/bin/sh\necho hi\n") + _detail_route() + _patch_route(changed=["scripts/run.sh"]) + + runner = CliRunner() + result = runner.invoke( + app, + [ + "skills", + "put-file", + "my-skill", + "scripts/run.sh", + "--from-file", + str(source), + "--executable", + "--purpose", + "entrypoint", + ], + ) + assert result.exit_code == 0, result.output + + entry = load_sync_state(tmp_config_paths).entries[0] + recorded = {f.path: f for f in entry.files} + assert recorded["scripts/run.sh"].sha256 == _sha(b"#!/bin/sh\necho hi\n") + assert recorded["scripts/run.sh"].executable is True + assert recorded["scripts/run.sh"].purpose == "entrypoint" + + # No drift for a file that is already current, and the next full push + # re-sends the label rather than dropping it. + target = SyncTarget(path=str(target_dir), scope="owned") + assert not tree_push_drifted(entry, target, []) + payload, _ = build_files_payload(slug_dir, entry.files, []) + sent = {e["path"]: e for e in payload} + assert sent["scripts/run.sh"]["purpose"] == "entrypoint" + assert sent["scripts/run.sh"]["executable"] is True + + +@respx.mock +def test_put_file_records_a_new_path_labelled_without_a_mark( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """With only a label passed, the mark is read off the copy just written. + + That is the value the next full push would read there anyway, so recording + it leaves what gets sent unchanged while the label stops being dropped. + """ + _setup_creds(monkeypatch, tmp_config_paths) + target_dir = tmp_path / "skills" + slug_dir = target_dir / "my-skill" + slug_dir.mkdir(parents=True) + (slug_dir / "SKILL.md").write_text(_SKILL_MD, encoding="utf-8") + _write_index(tmp_config_paths, slug="my-skill", target_path=str(target_dir), files=[]) + source = tmp_path / "elsewhere" / "rubric.md" + source.parent.mkdir() + source.write_bytes(b"score it\n") + _detail_route() + _patch_route(changed=["references/rubric.md"]) + + runner = CliRunner() + result = runner.invoke( + app, + [ + "skills", + "put-file", + "my-skill", + "references/rubric.md", + "--from-file", + str(source), + "--purpose", + "rubric", + ], + ) + assert result.exit_code == 0, result.output + + entry = load_sync_state(tmp_config_paths).entries[0] + recorded = {f.path: f for f in entry.files} + assert recorded["references/rubric.md"].purpose == "rubric" + assert recorded["references/rubric.md"].executable is False + target = SyncTarget(path=str(target_dir), scope="owned") + assert not tree_push_drifted(entry, target, []) + + +@respx.mock +def test_put_file_leaves_a_mirrored_new_path_unlabelled_out_of_the_record( + tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch +) -> None: + """A new path with neither a mark nor a label stays out even when mirrored. + + Nothing is known about it beyond its bytes, so a recorded ``False`` / + ``None`` would be a guess that the next full push turns into a clear of + whatever the registry holds. Left out, it reads as ordinary drift that the + push resolves from disk. + """ + _setup_creds(monkeypatch, tmp_config_paths) + target_dir = tmp_path / "skills" + slug_dir = target_dir / "my-skill" + slug_dir.mkdir(parents=True) + (slug_dir / "SKILL.md").write_text(_SKILL_MD, encoding="utf-8") + _write_index(tmp_config_paths, slug="my-skill", target_path=str(target_dir), files=[]) + source = tmp_path / "elsewhere" / "notes.md" + source.parent.mkdir() + source.write_bytes(b"fresh notes\n") + _detail_route() + _patch_route() + + runner = CliRunner() + result = runner.invoke( + app, ["skills", "put-file", "my-skill", "notes.md", "--from-file", str(source)] + ) + assert result.exit_code == 0, result.output + + entry = load_sync_state(tmp_config_paths).entries[0] + assert [f.path for f in entry.files] == [] + + @respx.mock def test_put_file_leaves_the_index_alone_when_no_target_tracks_the_slug( tmp_path: Path, tmp_config_paths: ConfigPaths, monkeypatch