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/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..c8f452f 100644 --- a/src/goodeye_cli/commands/skills.py +++ b/src/goodeye_cli/commands/skills.py @@ -16,7 +16,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 +32,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 +603,278 @@ 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, + *, + expected_version_token: str, + path: str, + content: bytes | None, + executable: bool | None = None, + purpose: str | None = None, +) -> None: + """Bring the local mirror in line with the change that just landed. + + 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 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 index or an + unwritable mirror is reported and moves on rather than failing the command. + """ + from goodeye_cli import sync + + try: + paths = get_config_paths() + state = sync.load_sync_state(paths) + if content is None: + touched = sync.record_file_removed( + state, + result, + expected_version_token=expected_version_token, + path=path, + ) + else: + touched = sync.record_file_written( + state, + result, + expected_version_token=expected_version_token, + path=path, + content=content, + 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 copy 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, + expected_version_token=token, + path=path, + content=raw, + 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, expected_version_token=token, path=path, content=None) + + @app.command("lineage") def lineage( skill_id: str = typer.Argument(..., help="Skill UUID or name."), @@ -1178,7 +1450,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..37e6729 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,6 +697,267 @@ def upsert_entry(state: SyncState, entry: SyncEntry) -> None: state.entries.append(entry) +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")) + + +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 _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: + """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. + + 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. + """ + entry.synced_version = result.version + entry.version_token = result.version_token + + +def record_file_written( + state: SyncState, + result: WorkflowFilePatchResult, + *, + expected_version_token: str, + path: str, + content: bytes, + executable: bool | None = None, + purpose: str | None = None, +) -> 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 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: 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 + 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 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. + """ + 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_at_version( + state, + slug=result.slug or result.name, + skill_id=result.workflow_id, + version_token=expected_version_token, + ): + mirror_executable: bool | None = None + recorded: FileState | 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, + sha256=new_hash, + 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) + 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 + + +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 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, 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( + 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] + _remove_mirrored_file(entry, path) + _move_to_new_version(entry, result) + touched = True + return touched + + # ----- scope selection + change detection ----- @@ -2440,6 +2701,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 +2801,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 +2924,11 @@ def _push_candidate( "body_sha256", "build_files_payload", "ensure_identity", + "entries_at_version", "expand_target_path", "find_entry", "find_target_by_path", + "inline_content_field", "is_modified_locally", "list_targets", "load_sync_config", @@ -2664,6 +2939,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..8d5e6e3 --- /dev/null +++ b/tests/test_commands_skill_files.py @@ -0,0 +1,1259 @@ +"""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, how the server's errors surface, and the +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. +""" + +from __future__ import annotations + +import base64 +import hashlib +import json as _json +import os +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 Conflict, Forbidden, GoodeyeError, NotFound, ValidationFailed +from goodeye_cli.sync import ( + FileState, + SyncEntry, + SyncState, + SyncTarget, + body_sha256, + build_files_payload, + is_modified_locally, + load_sync_state, + read_local_body, + 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()) + + +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 ----- + + +@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 + + 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_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, + 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)] + ) + 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 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 +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 +) -> 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"), + ] + 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 +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 + + 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 +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, []) + + +@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 +) -> 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"))], + ) + + (slug_dir / "notes.md").write_bytes(b"fresh notes\n") + _detail_route() + _patch_route() + runner = CliRunner() + 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 + 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: + 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 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" },