From 267faec9ea633f863773c8f0365e1bf94cf2c29f Mon Sep 17 00:00:00 2001 From: Randy Olson Date: Mon, 20 Jul 2026 11:47:38 -0700 Subject: [PATCH] Leave verifier refs out of a push for a skill you do not own Only a skill's owner may change its verifier references, so a push for a skill shared with you has nothing to say about them. The registry carries the stored references forward whenever the field is absent, so omitting it is both correct and sufficient. The push payload sent them regardless. `_verifier_payload` returned a list rather than `None`, and the client only drops a key whose value is `None`, so a skill with no bindings still went out with an empty list attached. A registry that reads the field's presence as an attempt to set it refuses the whole save, which surfaced here as a read-only skip and made an ordinary edit look like a missing grant. Return `None` for a skill someone else owns so the key is left out. Bindings the registry reports back are still recorded in the index, so it keeps tracking references the caller cannot edit. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/goodeye_cli/sync.py | 10 +++++++- tests/test_sync.py | 51 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/src/goodeye_cli/sync.py b/src/goodeye_cli/sync.py index eade930..3ddbad6 100644 --- a/src/goodeye_cli/sync.py +++ b/src/goodeye_cli/sync.py @@ -1907,13 +1907,21 @@ class PushResult(_SyncBase): items: list[PushItem] = Field(default_factory=list) -def _verifier_payload(entry: SyncEntry) -> list[dict[str, Any]]: +def _verifier_payload(entry: SyncEntry) -> list[dict[str, Any]] | None: """Build the save-payload verifier bindings from an index entry. Mirrors the publish path's binding shape (``name`` + ``verifier_id``) and additionally preserves a pinned ``version`` when the recorded binding has one, so a push never drops a version pin the workflow carried. + + Returns ``None`` for a skill someone else owns, which leaves the field out + of the payload entirely. Only an owner may rewire the refs, and the + registry carries the stored ones forward when the field is absent, so a + grantee has nothing to gain by sending them and older servers reject the + attempt outright. """ + if entry.effective_role != "owner": + return None payload: list[dict[str, Any]] = [] for binding in entry.verifier_bindings: row: dict[str, Any] = {"name": binding.name, "verifier_id": binding.verifier_id} diff --git a/tests/test_sync.py b/tests/test_sync.py index 805c73a..3af8067 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -1905,6 +1905,57 @@ def test_push_omits_version_for_unpinned_binding( assert sent["verifiers"] == [{"name": "tone", "verifier_id": "vrf_1"}] +@respx.mock +def test_push_omits_verifiers_for_shared_skill( + tmp_path: Path, tmp_config_paths: ConfigPaths +) -> None: + """A push of someone else's skill leaves the verifier refs out of the payload. + + Rewiring the refs is reserved to the skill's owner, so an edit grantee has + nothing to say about them: the server carries the stored refs forward when + the field is absent. Sending them back would be asking to set a field this + caller may not set, which older servers refuse outright. + """ + _me_route() + target_dir = tmp_path / "skills" + config = SyncConfig(targets=[SyncTarget(path=str(target_dir), scope="all")]) + body = _push_body(slug="shared-runbook") + _write_skill(target_dir, "shared-runbook", body) + state = SyncState( + entries=[ + _modified_entry( + target_dir, + id_="skl_shared", + slug="shared-runbook", + role="edit", + verifiers=[SyncVerifierBinding(name="tone", verifier_id="vrf_1", version=3)], + ) + ] + ) + save_route = respx.post(f"{SERVER}/v1/skills").mock( + return_value=_save_response( + workflow_id="skl_shared", + name="shared-runbook", + verifiers=[{"name": "tone", "verifier_id": "vrf_1", "version": 3}], + ) + ) + + with GoodeyeClient(SERVER, api_key="good_live_EXAMPLE") as client: + result = push(client, config, state, slugs=[], target_path=None, paths=tmp_config_paths) + + sent = json.loads(save_route.calls[0].request.content) + assert "verifiers" not in sent + assert sent["skill_id"] == "skl_shared" + assert [(i.slug, i.action) for i in result.items] == [("shared-runbook", "pushed")] + + # The bindings the server reports back are still recorded locally, so the + # index keeps tracking refs the caller cannot edit. + reloaded = load_sync_state(tmp_config_paths) + assert reloaded.entries[0].verifier_bindings == [ + SyncVerifierBinding(name="tone", verifier_id="vrf_1", version=3) + ] + + @respx.mock def test_push_edited_metadata_reaches_request( tmp_path: Path, tmp_config_paths: ConfigPaths