diff --git a/.github/workflows/spec-drift.yml b/.github/workflows/spec-drift.yml new file mode 100644 index 0000000..09624a5 --- /dev/null +++ b/.github/workflows/spec-drift.yml @@ -0,0 +1,45 @@ +name: spec-drift + +# Glean publishes its OpenAPI specs independently of this repo. The conformance +# test (tests/test_spec_conformance.py) checks client.py against a committed +# manifest so the suite stays offline; this job is what notices the manifest has +# fallen behind the published spec. +# +# Deliberately NOT part of the release workflow: a spec update is an external +# event, and it should not fail an unrelated pull request. + +on: + schedule: + - cron: "0 7 * * 1" # Mondays, 07:00 UTC + workflow_dispatch: + +permissions: + contents: read + +jobs: + check: + name: is tests/spec_manifest.json still current? + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: "3.12" + + # PyYAML is a tooling-only dependency: the package and the test suite + # never import it, so the zero-dependency guarantee is unaffected. + - name: Install the tooling dependency + run: python -m pip install --quiet pyyaml + + - name: Compare the manifest against the published spec + run: python3 tools/refresh_spec_manifest.py --check + + - name: What to do if this failed + if: failure() + run: | + echo "::notice::Glean has published a spec change." + echo "Refresh the manifest and let the conformance test show what moved:" + echo " python3 -m pip install pyyaml" + echo " python3 tools/refresh_spec_manifest.py" + echo " python3 -m unittest tests.test_spec_conformance -v" diff --git a/CLAUDE.md b/CLAUDE.md index 286e21a..60de19d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -30,6 +30,11 @@ python3 -m unittest tests.test_commands.TestParseArgs.test_flag_with_value There is no build step, no `pip install`, no linter config, and no `pyproject.toml`/`setup.py`. Tests use only the stdlib (`unittest`, `unittest.mock`) and make no network calls. +`tools/refresh_spec_manifest.py` is the single exception: it fetches Glean's OpenAPI +specs and needs PyYAML. It is tooling only — never imported by the package or the +tests — so the zero-dependency guarantee still holds. The scheduled `spec-drift` +workflow runs it with `--check` weekly. + `install.py` is the only thing that produces an artifact: it stages the package in a temp directory and emits a zipapp, so nothing is ever built inside the repo. On macOS set `PYTHONPYCACHEPREFIX=~/.cache/python` to keep `__pycache__` out of the working tree — @@ -83,6 +88,7 @@ Client API (`/rest/api/v1`, `api_token`) and Indexing API (`/api/index/v1`, `ind 4. Add a `DOCS` entry in help_docs.py (summary, usage, params, examples, endpoint) or the command is hidden from `/help` and the planner. 5. If it writes/deletes/changes auth, add the dotted name to `_NL_DESTRUCTIVE` in commands.py. A command that takes its verb positionally (`/config set`, `/personal purge`, `/flow purge`) goes in `_NL_DESTRUCTIVE_SUBS` instead, keyed by command name with the mutating sub-verbs. 6. Add tests (mock-mode handler test + client/mock test). Reuse the `_mock_session()` helper pattern in the test files. +7. Confirm the path exists in the spec. `tests/test_spec_conformance.py` asserts every REST path and method in `client.py` appears in `tests/spec_manifest.json`, which is generated from Glean's published OpenAPI specs by `tools/refresh_spec_manifest.py`. If the test fails, the endpoint is not in the spec — check the path before assuming the manifest is stale. Twelve calls once drifted precisely because mock responses are keyed on the same path strings the live client posts to, so a wrong path was wrong consistently and the suite stayed green. **Token safety** — never let real secrets reach disk or the history buffer. Secure refs (`token.secure.client` / `token.secure.indexing`) are stored verbatim and resolved from `$GLEAN_CLIENT_TOKEN` / `$GLEAN_INDEXING_TOKEN` at request time via `resolve_secure`. `_sanitize_for_history` masks `--token`/`--indexing-token` values and `/config set ` values before they enter `command_history`. `_display_token` masks literals to `***1234`. Preserve all of this when adding any command that handles a token. diff --git a/assets/glean_code_cli_header.jpeg b/assets/glean_code_cli_header.jpeg new file mode 100644 index 0000000..54aa199 Binary files /dev/null and b/assets/glean_code_cli_header.jpeg differ diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index a010ad1..a891c2a 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -1385,7 +1385,7 @@ List callable tools exposed to agents in the current workspace. **Output** — Tool names and descriptions. -**Endpoint** — `POST /rest/api/v1/tools/list` +**Endpoint** — `GET /rest/api/v1/tools/list` --- @@ -1461,7 +1461,7 @@ Fetch the permission list for a document. **Mock mode** — The document's author comes back as `owner` and the rest of the [people roster](MOCK_CORPUS.md#the-people-roster) as `viewer`, e.g. `/docs.permissions doc_plan_process` → `priya.raman@acme.com` as owner. -**Endpoint** — `POST /rest/api/v1/getdocumentpermissions` +**Endpoint** — `POST /rest/api/v1/getdocpermissions` --- @@ -1521,24 +1521,6 @@ Look up a person's full profile by email address. --- -#### /announcements.list - -List all current announcements in the workspace. - -```text -/announcements.list -``` - -```text -/announcements.list -``` - -**Output** — Announcement ids, titles, and metadata. - -**Endpoint** — `POST /rest/api/v1/announcements/list` - ---- - #### /announcements.create Create a new workspace announcement. @@ -1560,7 +1542,7 @@ Create a new workspace announcement. **Output** — The new announcement id and creation status. -**Endpoint** — `POST /rest/api/v1/announcements/create` +**Endpoint** — `POST /rest/api/v1/createannouncement` --- @@ -1574,7 +1556,7 @@ Delete an announcement by id. | Parameter | Description | | --- | --- | -| `id` | The announcement id (from `/announcements.list`). | +| `id` | The announcement id (returned by `/announcements.create`). | ```text /announcements.delete ann_123 @@ -1582,7 +1564,7 @@ Delete an announcement by id. **Output** — Confirms the announcement was deleted. -**Endpoint** — `POST /rest/api/v1/announcements/delete` +**Endpoint** — `POST /rest/api/v1/deleteannouncement` --- diff --git a/docs/COMMAND_INDEX.md b/docs/COMMAND_INDEX.md index e93347a..f03a4c4 100644 --- a/docs/COMMAND_INDEX.md +++ b/docs/COMMAND_INDEX.md @@ -15,7 +15,7 @@ Every command, grouped by surface. | Insights & activity | `/insights` `/activity.report` | | Agents and tools | `/agents.list` `/agents.run` `/tools.list` `/tools.call` | | Docs and people | `/docs.get` `/docs.permissions` `/entities.list` `/people.get` | -| Announcements, collections, pins | `/announcements.list` `/announcements.create` `/announcements.delete` `/collections.list` `/collections.create` `/collections.delete` `/pins.list` `/pins.create` `/pins.delete` | +| Announcements, collections, pins | `/announcements.create` `/announcements.delete` `/collections.list` `/collections.create` `/collections.delete` `/pins.list` `/pins.create` `/pins.delete` | | Shortcuts (Go Links) | `/shortcuts.list` `/shortcuts.get` `/shortcuts.create` `/shortcuts.update` `/shortcuts.delete` | | Answers | `/answers.list` `/answers.get` `/answers.create` `/answers.update` `/answers.delete` | | Verification | `/verification.list` `/verification.verify` `/verification.remind` | diff --git a/docs/MOCK_CORPUS.md b/docs/MOCK_CORPUS.md index bad2774..9288e82 100644 --- a/docs/MOCK_CORPUS.md +++ b/docs/MOCK_CORPUS.md @@ -348,7 +348,7 @@ Queries the corpus is written to answer well: | [`/recommendations`](COMMANDS.md#recommendations) | `/recommendations` | The freshest documents, in full result shape | | [`/datasources.list`](COMMANDS.md#datasourceslist) | `/search` (facets) | The five datasource names and their index counts | | [`/docs.get`](COMMANDS.md#docsget) | `/getdocuments` | Lookup by id or URL; unknown specs echo back | -| [`/docs.permissions`](COMMANDS.md#docspermissions) | `/getdocumentpermissions` | Author as `owner`, roster as `viewer` | +| [`/docs.permissions`](COMMANDS.md#docspermissions) | `/getdocpermissions` | Author as `owner`, roster as `viewer` | | [`/summarize`](COMMANDS.md#summarize) | `/summarize` | A summary built from that document's body, author, and age | | [`/entities.list`](COMMANDS.md#entitieslist) | `/listentities` | The people roster | | [`/people.get`](COMMANDS.md#peopleget) | `/people` | Profile for a roster email | diff --git a/docs/REST_PATHS.md b/docs/REST_PATHS.md index 4af328a..019aa82 100644 --- a/docs/REST_PATHS.md +++ b/docs/REST_PATHS.md @@ -13,15 +13,14 @@ POST /feedback POST /agents/search POST /agents/runs/wait POST /agents/runs/stream -POST /tools/list +GET /tools/list POST /tools/call POST /getdocuments -POST /getdocumentpermissions +POST /getdocpermissions POST /listentities POST /people -POST /announcements/list -POST /announcements/create -POST /announcements/delete +POST /createannouncement +POST /deleteannouncement POST /listcollections POST /createcollection POST /listpins diff --git a/docs/TESTING.md b/docs/TESTING.md index 862c4c7..94699de 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -157,3 +157,35 @@ The module patches out the mock client's simulated 0.25s network latency; withou - A corrupt database returns an error string rather than a traceback The linking fixture deliberately uses five documents rather than two. With two, every shared phrase appears in every document, so IDF cannot distinguish "topically shared" from "common vocabulary" and nothing can score above the threshold — a property of IDF at that scale, not a bug worth distorting the scoring to hide. + +## Spec conformance + +`tests/test_spec_conformance.py` reads `glean_code/client.py` with `ast` and asserts +that every REST path and HTTP method it calls exists in Glean's published OpenAPI +spec. It compares against `tests/spec_manifest.json` rather than the live spec, so +it stays offline like the rest of the suite. + +Regenerate the manifest when Glean ships a spec update: + +```bash +python3 -m pip install pyyaml # tooling-only dependency +python3 tools/refresh_spec_manifest.py +python3 -m unittest tests.test_spec_conformance -v +``` + +The check exists because twelve Client API calls once drifted from the spec +undetected: `_mock_response` is keyed on the same path strings the live client +posts to, so a wrong path was wrong in both places and every test passed. It +catches wrong paths, wrong methods, and invented endpoints. + +Two deliberate limits: + +- It does **not** require the client to call every documented endpoint. Plenty are + intentionally unimplemented. +- It does not check request *body* fields. Those are covered by + `TestSpecConformantBodies` in `tests/test_client_extended.py`. + +Endpoints the spec declares but leaves undocumented (currently +`/indexemployeelist`) are tracked in the manifest's `undocumented` section and +allowed, but the test fails if one becomes documented again — the signal to drop +the exception. diff --git a/glean_code/client.py b/glean_code/client.py index d552524..6135dda 100644 --- a/glean_code/client.py +++ b/glean_code/client.py @@ -16,16 +16,15 @@ /tools/list List callable tools /tools/call Invoke a tool /getdocuments Fetch documents by id or URL - /getdocumentpermissions Fetch permissions for a document + /getdocpermissions Fetch permissions for a document /listentities List entities (people, teams, etc.) /people Get a person profile - /announcements/create Create an announcement - /announcements/list List announcements - /announcements/delete Delete an announcement + /createannouncement Create an announcement + /deleteannouncement Delete an announcement /listcollections List collections /createcollection Create a collection /listpins List pinned results - /createpin Create a pinned result + /pin Create a pinned result """ from __future__ import annotations @@ -37,6 +36,7 @@ try: import urllib.request import urllib.error + import urllib.parse except Exception: # pragma: no cover urllib = None # type: ignore @@ -79,7 +79,15 @@ def _capture(self, path: str, body: Dict[str, Any], resp: Dict[str, Any]) -> Non if _flow.capture_enabled(setting, self.config.effective_mode): _flow.record(self.config, path, body, resp) - def _post(self, path: str, body: Dict[str, Any]) -> Dict[str, Any]: + def _post(self, path: str, body: Dict[str, Any], + query: Optional[Dict[str, Any]] = None, + method: str = "POST") -> Dict[str, Any]: + """Call a Client API endpoint. + + `query` holds parameters the spec defines as query-string rather than + body fields; `method` covers the handful of GET endpoints. Mock and + local dispatch stay keyed on the bare `path`, so neither is affected. + """ if self.config.effective_mode == "local": resp = _local_response(path, body) self._capture(path, body, resp) @@ -97,8 +105,13 @@ def _post(self, path: str, body: Dict[str, Any]) -> Dict[str, Any]: if not base: raise GleanError("No base URL configured. Run /login or /config set instance .") url = f"{base}{path}" - data = json.dumps(body).encode("utf-8") - req = urllib.request.Request(url, data=data, headers=self._headers(), method="POST") + if query: + pairs = {k: v for k, v in query.items() if v is not None} + if pairs: + url += "?" + urllib.parse.urlencode(pairs, doseq=True) + # GET endpoints must not carry a body, or the server rejects the request. + data = None if method == "GET" else json.dumps(body).encode("utf-8") + req = urllib.request.Request(url, data=data, headers=self._headers(), method=method) try: with urllib.request.urlopen(req, timeout=60) as resp: raw = resp.read().decode("utf-8") @@ -425,27 +438,49 @@ def recommendations(self, user: Optional[str] = None) -> Dict[str, Any]: body["user"] = user return self._post("/recommendations", body) + # Friendly aliases for the two Feedback.event values the CLI exposes. + _FEEDBACK_EVENTS = {"THUMBS_UP": "UPVOTE", "THUMBS_DOWN": "DOWNVOTE"} + def feedback(self, tracking_token: str, rating: str, - comments: Optional[str] = None) -> Dict[str, Any]: - body = {"trackingToken": tracking_token, "category": rating} + comments: Optional[str] = None, + category: Optional[str] = None) -> Dict[str, Any]: + """Send feedback for a tracking token. + + `rating` accepts THUMBS_UP / THUMBS_DOWN (mapped to the spec's + UPVOTE / DOWNVOTE) or any Feedback.event value verbatim. `event` and + `trackingTokens` are both required by the API; `category` (SEARCH, + CHAT, ...) is a separate enum and stays optional. + """ + event = self._FEEDBACK_EVENTS.get(rating.upper(), rating.upper()) + body: Dict[str, Any] = { + "event": event, + "trackingTokens": [tracking_token], + } + if category: + body["category"] = category.upper() if comments: - body["comments"] = comments + body["manualFeedbackInfo"] = {"comments": comments} return self._post("/feedback", body) # ---------------- agents and tools ---------------- def agents_search(self, query: Optional[str] = None) -> Dict[str, Any]: - return self._post("/agents/search", {"query": query or ""}) + # The API filters agents by name; `query` is the CLI's flag name. + return self._post("/agents/search", {"name": query or ""}) def agent_run(self, agent_id: str, input: str, stream: bool = False) -> Dict[str, Any]: path = "/agents/runs/stream" if stream else "/agents/runs/wait" - return self._post(path, {"agentId": agent_id, "input": input}) + return self._post(path, {"agent_id": agent_id, "input": input}) - def tools_list(self) -> Dict[str, Any]: - return self._post("/tools/list", {}) + def tools_list(self, tool_names: Optional[List[str]] = None) -> Dict[str, Any]: + # GET, per the spec — the optional filter is a query parameter. + return self._post("/tools/list", {}, method="GET", + query={"toolNames": tool_names} if tool_names else None) def tools_call(self, name: str, arguments: Dict[str, Any]) -> Dict[str, Any]: - return self._post("/tools/call", {"name": name, "arguments": arguments}) + # The wire field is `parameters`; `arguments` is kept as the Python + # argument name because that is what the /tools.call command calls it. + return self._post("/tools/call", {"name": name, "parameters": arguments}) # ---------------- documents and people ---------------- @@ -459,7 +494,7 @@ def get_documents(self, ids: Optional[List[str]] = None, return self._post("/getdocuments", body) def document_permissions(self, doc_id: str) -> Dict[str, Any]: - return self._post("/getdocumentpermissions", {"documentSpec": {"id": doc_id}}) + return self._post("/getdocpermissions", {"documentId": doc_id}) def list_entities(self, kind: str = "PEOPLE", page_size: int = 10, query: Optional[str] = None) -> Dict[str, Any]: @@ -469,22 +504,19 @@ def list_entities(self, kind: str = "PEOPLE", page_size: int = 10, return self._post("/listentities", body) def person(self, email: str) -> Dict[str, Any]: - return self._post("/people", {"email": email}) + return self._post("/people", {"emailIds": [email]}) # ---------------- announcements, collections, pins ---------------- - def announcements_list(self) -> Dict[str, Any]: - return self._post("/announcements/list", {}) - def announcement_create(self, title: str, body_text: str, audience: Optional[str] = None) -> Dict[str, Any]: body: Dict[str, Any] = {"title": title, "body": {"text": body_text}} if audience: body["audienceFilters"] = [{"filter": audience}] - return self._post("/announcements/create", body) + return self._post("/createannouncement", body) def announcement_delete(self, ann_id: str) -> Dict[str, Any]: - return self._post("/announcements/delete", {"id": ann_id}) + return self._post("/deleteannouncement", {"id": ann_id}) def collections_list(self) -> Dict[str, Any]: return self._post("/listcollections", {}) @@ -498,8 +530,28 @@ def collection_create(self, name: str, description: Optional[str] = None) -> Dic def pins_list(self) -> Dict[str, Any]: return self._post("/listpins", {}) - def pin_create(self, url: str, query: str) -> Dict[str, Any]: - return self._post("/createpin", {"url": url, "query": query}) + def pin_create(self, url: Optional[str] = None, query: str = "", + doc_id: Optional[str] = None) -> Dict[str, Any]: + """Pin a document as a result for one or more queries. + + The API pins by document id, while the CLI has always taken a URL, so a + URL is resolved through /getdocuments first. Pass `doc_id` to skip that + round trip. + """ + if not doc_id: + if not url: + raise GleanError("pin_create needs either a url or a doc_id.") + resolved = self.get_documents(urls=[url]) + docs = resolved.get("documents") or resolved.get("results") or [] + if isinstance(docs, dict): # some tenants key by spec + docs = list(docs.values()) + doc_id = next((d.get("id") for d in docs if isinstance(d, dict) and d.get("id")), None) + if not doc_id: + raise GleanError( + f"Could not resolve {url} to a document id, so it cannot be pinned. " + "Pass --doc-id instead." + ) + return self._post("/pin", {"documentId": doc_id, "queries": [query]}) def pin_delete(self, pin_id: str) -> Dict[str, Any]: return self._post("/unpin", {"id": pin_id}) @@ -581,9 +633,9 @@ def summarize(self, url: Optional[str] = None, query: Optional[str] = None) -> Dict[str, Any]: body: Dict[str, Any] = {} if doc_id: - body["documentSpec"] = {"id": doc_id} + body["documentSpecs"] = [{"id": doc_id}] elif url: - body["documentSpec"] = {"url": url} + body["documentSpecs"] = [{"url": url}] if query: body["query"] = query return self._post("/summarize", body) @@ -591,10 +643,9 @@ def summarize(self, url: Optional[str] = None, # ---------------- verification ---------------- def verification_list(self, count: Optional[int] = None) -> Dict[str, Any]: - body: Dict[str, Any] = {} - if count: - body["count"] = count - return self._post("/listverifications", body) + # `count` is a query parameter in the spec, not a body field. + return self._post("/listverifications", {}, + query={"count": count} if count else None) def verification_verify(self, doc_id: str, action: Optional[str] = None) -> Dict[str, Any]: @@ -756,15 +807,21 @@ def _mock_response(path: str, body: Dict[str, Any]) -> Dict[str, Any]: if path == "/feedback": return {"status": "ok"} if path == "/agents/search": - return {"agents": [ + agents = [ {"id": "agt_research", "name": "Research Agent", "description": "Deep research across company knowledge."}, {"id": "agt_sales", "name": "Sales Assistant", "description": "Summarises accounts and prepares call notes."}, - ]} + ] + # The API filters by agent name; mirror that so offline behaviour and + # live behaviour agree. + name = (body.get("name") or "").strip().lower() + if name: + agents = [a for a in agents if name in a["name"].lower()] + return {"agents": agents} if path in ("/agents/runs/wait", "/agents/runs/stream"): return {"runId": f"run_{uuid.uuid4().hex[:6]}", - "output": f"[mock agent {body.get('agentId')}] Finished task: " + "output": f"[mock agent {body.get('agent_id')}] Finished task: " f"{body.get('input','')[:60]}..."} if path == "/tools/list": return {"tools": [ @@ -773,7 +830,7 @@ def _mock_response(path: str, body: Dict[str, Any]) -> Dict[str, Any]: ]} if path == "/tools/call": return {"name": body.get("name"), "result": "ok", - "output": f"[mock tool {body.get('name')}] args={body.get('arguments')}"} + "output": f"[mock tool {body.get('name')}] args={body.get('parameters')}"} if path == "/getdocuments": specs = body.get("documentSpecs", []) docs = [] @@ -793,8 +850,8 @@ def _mock_response(path: str, body: Dict[str, Any]) -> Dict[str, Any]: "title": "Untitled document (not in the mock corpus)", "url": s.get("url", "https://example.com")}) return {"documents": docs} - if path == "/getdocumentpermissions": - doc = mock_corpus.find(body.get("documentSpec") or {}) + if path == "/getdocpermissions": + doc = mock_corpus.find(body) owner = (doc or {}).get("author") roster = mock_corpus.people(4) perms = [] @@ -806,12 +863,11 @@ def _mock_response(path: str, body: Dict[str, Any]) -> Dict[str, Any]: if path == "/listentities": return {"results": mock_corpus.people(int(body.get("pageSize") or 10))} if path == "/people": - return mock_corpus.person(body.get("email")) - if path == "/announcements/list": - return {"announcements": [{"id": "ann_1", "title": "Welcome to Glean"}]} - if path == "/announcements/create": + emails = body.get("emailIds") or [] + return mock_corpus.person(emails[0] if emails else None) + if path == "/createannouncement": return {"id": f"ann_{uuid.uuid4().hex[:6]}", "status": "created"} - if path == "/announcements/delete": + if path == "/deleteannouncement": return {"id": body.get("id"), "status": "deleted"} if path == "/listcollections": return {"collections": [ @@ -823,8 +879,10 @@ def _mock_response(path: str, body: Dict[str, Any]) -> Dict[str, Any]: return {"id": f"col_{uuid.uuid4().hex[:6]}", "name": body.get("name")} if path == "/listpins": return {"pins": [{"id": "pin_1", "query": "pto", "url": "https://example.com/pto"}]} - if path == "/createpin": - return {"id": f"pin_{uuid.uuid4().hex[:6]}", "status": "created"} + if path == "/pin": + return {"id": f"pin_{uuid.uuid4().hex[:6]}", "status": "created", + "documentId": body.get("documentId"), + "queries": body.get("queries") or []} if path == "/unpin": return {"id": body.get("id"), "status": "unpinned"} if path == "/deletecollection": @@ -876,7 +934,8 @@ def _mock_response(path: str, body: Dict[str, Any]) -> Dict[str, Any]: if path == "/deleteanswer": return {"id": body.get("id"), "status": "deleted"} if path == "/summarize": - spec = body.get("documentSpec", {}) + specs = body.get("documentSpecs") or [] + spec = specs[0] if specs else {} src = spec.get("url") or spec.get("id") or "the document" doc = mock_corpus.find(spec) if doc: diff --git a/glean_code/commands.py b/glean_code/commands.py index d5b9978..39618bf 100644 --- a/glean_code/commands.py +++ b/glean_code/commands.py @@ -2005,14 +2005,6 @@ def cmd_people_get(s: Session, pos, flags): # -------------------- announcements -------------------- -@register("announcements.list") -def cmd_ann_list(s: Session, pos, flags): - try: - print(_render_json(s.client.announcements_list())) - except GleanError as e: - ui.print_err(str(e)) - - @register("announcements.create") def cmd_ann_create(s: Session, pos, flags): title = flags.get("title") @@ -2072,12 +2064,17 @@ def cmd_pins_list(s: Session, pos, flags): @register("pins.create") def cmd_pins_create(s: Session, pos, flags): url = flags.get("url") + doc_id = flags.get("doc-id") or flags.get("doc_id") query = flags.get("query") - if not url or not query: - ui.print_err("Usage: /pins.create --query --url ") + if not query or not (url or doc_id): + ui.print_err("Usage: /pins.create --query (--url | --doc-id )") return try: - print(_render_json(s.client.pin_create(str(url), str(query)))) + print(_render_json(s.client.pin_create( + url=str(url) if url else None, + query=str(query), + doc_id=str(doc_id) if doc_id else None, + ))) except GleanError as e: ui.print_err(str(e)) diff --git a/glean_code/help_docs.py b/glean_code/help_docs.py index 3c7f4c6..6cd7897 100644 --- a/glean_code/help_docs.py +++ b/glean_code/help_docs.py @@ -390,7 +390,7 @@ "usage": "/tools.list", "params": [], "examples": ["/tools.list"], - "endpoint": "POST /rest/api/v1/tools/list", + "endpoint": "GET /rest/api/v1/tools/list", }, "tools.call": { "summary": "Invoke a tool with a JSON argument object.", @@ -422,7 +422,7 @@ "usage": "/docs.permissions ", "params": [("doc-id", "Glean document id.")], "examples": ["/docs.permissions doc_123"], - "endpoint": "POST /rest/api/v1/getdocumentpermissions", + "endpoint": "POST /rest/api/v1/getdocpermissions", }, "entities.list": { "summary": "List entities such as people, teams or groups.", @@ -444,13 +444,6 @@ }, # ---------------- announcements ---------------- - "announcements.list": { - "summary": "List current announcements.", - "usage": "/announcements.list", - "params": [], - "examples": ["/announcements.list"], - "endpoint": "POST /rest/api/v1/announcements/list", - }, "announcements.create": { "summary": "Create an announcement.", "usage": "/announcements.create --title --body [--audience ]", @@ -460,14 +453,14 @@ ("--audience", "Optional audience filter string."), ], "examples": ["/announcements.create --title \"All hands Friday\" --body \"10am PT\""], - "endpoint": "POST /rest/api/v1/announcements/create", + "endpoint": "POST /rest/api/v1/createannouncement", }, "announcements.delete": { "summary": "Delete an announcement by id.", "usage": "/announcements.delete ", "params": [("id", "Announcement id.")], "examples": ["/announcements.delete ann_123"], - "endpoint": "POST /rest/api/v1/announcements/delete", + "endpoint": "POST /rest/api/v1/deleteannouncement", }, # ---------------- collections ---------------- @@ -1135,7 +1128,7 @@ ("Verification", ["verification.list", "verification.verify", "verification.remind"]), ("Messages", ["messages.get"]), ("Activity", ["activity.report"]), - ("Announcements", ["announcements.list", "announcements.create", "announcements.delete"]), + ("Announcements", ["announcements.create", "announcements.delete"]), ("Collections", ["collections.list", "collections.create", "collections.delete"]), ("Pins", ["pins.list", "pins.create", "pins.delete"]), ("Indexing", ["datasources.status", "datasources.config", "indexing.rotate-token", diff --git a/incompat_report.md b/incompat_report.md index 7598868..61e24a4 100644 --- a/incompat_report.md +++ b/incompat_report.md @@ -1,5 +1,13 @@ # API Incompatibility Report — glean-code-cli vs. Glean REST API +> **Status: resolved 2026-09-27.** All twelve findings below have been fixed and +> the client now conforms to spec 0.9.0 on every Client API path and method. +> Ten were corrected directly; two needed a product decision: +> `/announcements.list` was **removed** (no endpoint in the API backs it), and +> `/pins.create` keeps its `--url` flag by resolving the URL to a document id +> through `/getdocuments` before pinning, with `--doc-id` added to skip that +> round trip. The report is kept as the record of what was wrong and why. + **Date:** 2026-08-15 **Scope:** every REST call made by `glean_code/client.py` — 40 Client API, 32 Indexing API, 5 Custom Metadata. diff --git a/tests/spec_manifest.json b/tests/spec_manifest.json new file mode 100644 index 0000000..345fbd6 --- /dev/null +++ b/tests/spec_manifest.json @@ -0,0 +1,399 @@ +{ + "_comment": "Generated by tools/refresh_spec_manifest.py. Do not edit by hand. Paths are relative to each surface's base URL, with path parameters collapsed to {}.", + "spec_version": "0.9.0", + "spec_commit": "0fef9ef256ed1e463d8534370e0afb074c4f3f8d", + "surfaces": { + "client": { + "/actions/actionpack/{}/auth": [ + "GET", + "POST" + ], + "/activity": [ + "POST" + ], + "/addcollectionitems": [ + "POST" + ], + "/addverificationreminder": [ + "POST" + ], + "/adminsearch": [ + "POST" + ], + "/agents": [ + "POST" + ], + "/agents/runs/stream": [ + "POST" + ], + "/agents/runs/wait": [ + "POST" + ], + "/agents/search": [ + "POST" + ], + "/agents/{}": [ + "GET", + "POST" + ], + "/agents/{}/import": [ + "POST" + ], + "/agents/{}/schemas": [ + "GET" + ], + "/autocomplete": [ + "POST" + ], + "/chat": [ + "POST" + ], + "/chat#stream": [ + "POST" + ], + "/chat-files/{}": [ + "GET" + ], + "/checkdatasourceauth": [ + "POST" + ], + "/configure/datasources/{}/instances/{}": [ + "GET", + "PATCH" + ], + "/createannouncement": [ + "POST" + ], + "/createanswer": [ + "POST" + ], + "/createauthtoken": [ + "POST" + ], + "/createcollection": [ + "POST" + ], + "/createshortcut": [ + "POST" + ], + "/datasource/{}/credentials": [ + "POST" + ], + "/datasource/{}/credentialstatus": [ + "GET" + ], + "/deleteallchats": [ + "POST" + ], + "/deleteannouncement": [ + "POST" + ], + "/deleteanswer": [ + "POST" + ], + "/deletechatfiles": [ + "POST" + ], + "/deletechats": [ + "POST" + ], + "/deletecollection": [ + "POST" + ], + "/deletecollectionitem": [ + "POST" + ], + "/deleteshortcut": [ + "POST" + ], + "/editanswer": [ + "POST" + ], + "/editcollection": [ + "POST" + ], + "/editcollectionitem": [ + "POST" + ], + "/editpin": [ + "POST" + ], + "/feed": [ + "POST" + ], + "/feedback": [ + "POST" + ], + "/getanswer": [ + "POST" + ], + "/getchat": [ + "POST" + ], + "/getchatapplication": [ + "POST" + ], + "/getchatfiles": [ + "POST" + ], + "/getcollection": [ + "POST" + ], + "/getdocpermissions": [ + "POST" + ], + "/getdocuments": [ + "POST" + ], + "/getdocumentsbyfacets": [ + "POST" + ], + "/getpin": [ + "POST" + ], + "/getshortcut": [ + "POST" + ], + "/governance/data/findings/exports": [ + "GET", + "POST" + ], + "/governance/data/findings/exports/{}": [ + "DELETE", + "GET" + ], + "/governance/data/policies": [ + "GET", + "POST" + ], + "/governance/data/policies/{}": [ + "GET", + "POST" + ], + "/governance/data/policies/{}/download": [ + "GET" + ], + "/governance/data/reports": [ + "POST" + ], + "/governance/data/reports/{}/download": [ + "GET" + ], + "/governance/data/reports/{}/status": [ + "GET" + ], + "/governance/documents/visibilityoverrides": [ + "GET", + "POST" + ], + "/insights": [ + "POST" + ], + "/listanswers": [ + "POST" + ], + "/listchats": [ + "POST" + ], + "/listcollections": [ + "POST" + ], + "/listentities": [ + "POST" + ], + "/listpins": [ + "POST" + ], + "/listshortcuts": [ + "POST" + ], + "/listverifications": [ + "POST" + ], + "/messages": [ + "POST" + ], + "/people": [ + "POST" + ], + "/people/{}/photo": [ + "GET" + ], + "/pin": [ + "POST" + ], + "/recommendations": [ + "POST" + ], + "/search": [ + "POST" + ], + "/summarize": [ + "POST" + ], + "/tool-servers/{}/auth": [ + "GET", + "POST" + ], + "/tool-servers/{}/tools": [ + "GET" + ], + "/tools/call": [ + "POST" + ], + "/tools/list": [ + "GET" + ], + "/unpin": [ + "POST" + ], + "/updateannouncement": [ + "POST" + ], + "/updateshortcut": [ + "POST" + ], + "/uploadchatfiles": [ + "POST" + ], + "/verify": [ + "POST" + ] + }, + "metadata": { + "/custom-metadata/schema/{}": [ + "DELETE", + "GET", + "PUT" + ], + "/document/{}/custom-metadata/{}": [ + "DELETE", + "PUT" + ], + "/submissions/{}/{}": [ + "POST" + ] + }, + "indexing": { + "/adddatasource": [ + "POST" + ], + "/betausers": [ + "POST" + ], + "/bulkindexdocuments": [ + "POST" + ], + "/bulkindexemployees": [ + "POST" + ], + "/bulkindexgroups": [ + "POST" + ], + "/bulkindexmemberships": [ + "POST" + ], + "/bulkindexshortcuts": [ + "POST" + ], + "/bulkindexteams": [ + "POST" + ], + "/bulkindexusers": [ + "POST" + ], + "/checkdocumentaccess": [ + "POST" + ], + "/debug/{}/document": [ + "POST" + ], + "/debug/{}/document/events": [ + "POST" + ], + "/debug/{}/documents": [ + "POST" + ], + "/debug/{}/status": [ + "POST" + ], + "/debug/{}/user": [ + "POST" + ], + "/deletedocument": [ + "POST" + ], + "/deleteemployee": [ + "POST" + ], + "/deletegroup": [ + "POST" + ], + "/deletemembership": [ + "POST" + ], + "/deleteteam": [ + "POST" + ], + "/deleteuser": [ + "POST" + ], + "/getdatasourceconfig": [ + "POST" + ], + "/getdocumentcount": [ + "POST" + ], + "/getdocumentstatus": [ + "POST" + ], + "/getusercount": [ + "POST" + ], + "/indexdocument": [ + "POST" + ], + "/indexdocuments": [ + "POST" + ], + "/indexemployee": [ + "POST" + ], + "/indexgroup": [ + "POST" + ], + "/indexmembership": [ + "POST" + ], + "/indexteam": [ + "POST" + ], + "/indexuser": [ + "POST" + ], + "/processalldocuments": [ + "POST" + ], + "/processallemployeesandteams": [ + "POST" + ], + "/processallmemberships": [ + "POST" + ], + "/rotatetoken": [ + "POST" + ], + "/updatepermissions": [ + "POST" + ], + "/uploadshortcuts": [ + "POST" + ] + } + }, + "undocumented": { + "indexing": [ + "/indexemployeelist" + ] + } +} diff --git a/tests/test_client.py b/tests/test_client.py index 628b779..cc9f521 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -125,12 +125,12 @@ def test_agents_have_id_name_description(self): # --- /agents/runs/wait and /agents/runs/stream --- def test_agent_run_wait_returns_output(self): - resp = _mock_response("/agents/runs/wait", {"agentId": "agt_research", "input": "hello"}) + resp = _mock_response("/agents/runs/wait", {"agent_id": "agt_research", "input": "hello"}) self.assertIn("output", resp) self.assertIn("agt_research", resp["output"]) def test_agent_run_stream_returns_output(self): - resp = _mock_response("/agents/runs/stream", {"agentId": "agt_sales", "input": "hi"}) + resp = _mock_response("/agents/runs/stream", {"agent_id": "agt_sales", "input": "hi"}) self.assertIn("output", resp) self.assertIn("runId", resp) @@ -173,10 +173,10 @@ def test_get_multiple_documents(self): resp = _mock_response("/getdocuments", {"documentSpecs": specs}) self.assertEqual(len(resp["documents"]), 3) - # --- /getdocumentpermissions --- + # --- /getdocpermissions --- def test_document_permissions_returns_permissions_list(self): - resp = _mock_response("/getdocumentpermissions", {"documentSpec": {"id": "d1"}}) + resp = _mock_response("/getdocpermissions", {"documentId": "d1"}) self.assertIn("permissions", resp) self.assertIsInstance(resp["permissions"], list) @@ -196,28 +196,24 @@ def test_entity_has_name_and_email(self): # --- /people --- def test_people_returns_profile(self): - resp = _mock_response("/people", {"email": "alice@example.com"}) + resp = _mock_response("/people", {"emailIds": ["alice@example.com"]}) self.assertEqual(resp["email"], "alice@example.com") self.assertIn("name", resp) # --- /announcements/* --- - def test_announcements_list_returns_list(self): - resp = _mock_response("/announcements/list", {}) - self.assertIn("announcements", resp) - def test_announcements_create_returns_id_and_status(self): - resp = _mock_response("/announcements/create", {"title": "Hi", "body": {"text": "hello"}}) + resp = _mock_response("/createannouncement", {"title": "Hi", "body": {"text": "hello"}}) self.assertIn("id", resp) self.assertEqual(resp["status"], "created") def test_announcements_create_unique_ids(self): - r1 = _mock_response("/announcements/create", {"title": "A", "body": {}}) - r2 = _mock_response("/announcements/create", {"title": "B", "body": {}}) + r1 = _mock_response("/createannouncement", {"title": "A", "body": {}}) + r2 = _mock_response("/createannouncement", {"title": "B", "body": {}}) self.assertNotEqual(r1["id"], r2["id"]) def test_announcements_delete_echoes_id(self): - resp = _mock_response("/announcements/delete", {"id": "ann_42"}) + resp = _mock_response("/deleteannouncement", {"id": "ann_42"}) self.assertEqual(resp["id"], "ann_42") self.assertEqual(resp["status"], "deleted") @@ -239,7 +235,7 @@ def test_pins_list_returns_pins(self): self.assertIn("pins", resp) def test_pin_create_returns_id_and_status(self): - resp = _mock_response("/createpin", {"url": "https://pto.com", "query": "pto"}) + resp = _mock_response("/pin", {"documentId": "doc_pto", "queries": ["pto"]}) self.assertIn("id", resp) self.assertEqual(resp["status"], "created") @@ -349,14 +345,14 @@ def test_agents_search_no_query(self): mock_fn.return_value = {"agents": []} self.client.agents_search() body = mock_fn.call_args[0][1] - self.assertEqual(body["query"], "") + self.assertEqual(body["name"], "") def test_agents_search_with_query(self): with patch("glean_code.client._mock_response") as mock_fn: mock_fn.return_value = {"agents": []} self.client.agents_search(query="sales") body = mock_fn.call_args[0][1] - self.assertEqual(body["query"], "sales") + self.assertEqual(body["name"], "sales") def test_agent_run_wait_path(self): with patch("glean_code.client._mock_response") as mock_fn: @@ -389,9 +385,11 @@ def test_feedback_body(self): mock_fn.return_value = {"status": "ok"} self.client.feedback("tok_1", "THUMBS_UP", comments="great") body = mock_fn.call_args[0][1] - self.assertEqual(body["trackingToken"], "tok_1") - self.assertEqual(body["category"], "THUMBS_UP") - self.assertEqual(body["comments"], "great") + # The spec requires event + trackingTokens; THUMBS_UP maps to UPVOTE. + self.assertEqual(body["event"], "UPVOTE") + self.assertEqual(body["trackingTokens"], ["tok_1"]) + self.assertEqual(body["manualFeedbackInfo"]["comments"], "great") + self.assertNotIn("category", body) # category is a separate enum def test_feedback_no_comments(self): with patch("glean_code.client._mock_response") as mock_fn: @@ -426,12 +424,36 @@ def test_collection_create_with_description(self): self.assertEqual(body["description"], "New hire docs") def test_pin_create_body(self): + """A URL is resolved to a document id, then pinned by id.""" with patch("glean_code.client._mock_response") as mock_fn: - mock_fn.return_value = {"id": "pin_1", "status": "created"} + mock_fn.side_effect = [ + {"documents": [{"id": "doc_pto"}]}, # /getdocuments + {"id": "pin_1", "status": "created"}, # /pin + ] self.client.pin_create("https://example.com/pto", "pto policy") - body = mock_fn.call_args[0][1] - self.assertEqual(body["url"], "https://example.com/pto") - self.assertEqual(body["query"], "pto policy") + lookup_path, lookup_body = mock_fn.call_args_list[0][0][:2] + pin_path, pin_body = mock_fn.call_args_list[1][0][:2] + self.assertEqual(lookup_path, "/getdocuments") + self.assertEqual(lookup_body["documentSpecs"], [{"url": "https://example.com/pto"}]) + self.assertEqual(pin_path, "/pin") + self.assertEqual(pin_body, {"documentId": "doc_pto", "queries": ["pto policy"]}) + + def test_pin_create_with_doc_id_skips_the_lookup(self): + with patch("glean_code.client._mock_response") as mock_fn: + mock_fn.return_value = {"id": "pin_1", "status": "created"} + self.client.pin_create(query="pto policy", doc_id="doc_pto") + self.assertEqual(mock_fn.call_count, 1, "no /getdocuments round trip needed") + path, body = mock_fn.call_args[0][:2] + self.assertEqual(path, "/pin") + self.assertEqual(body, {"documentId": "doc_pto", "queries": ["pto policy"]}) + + def test_pin_create_errors_when_the_url_cannot_be_resolved(self): + from glean_code.client import GleanError + with patch("glean_code.client._mock_response") as mock_fn: + mock_fn.return_value = {"documents": []} + with self.assertRaises(GleanError) as ctx: + self.client.pin_create("https://example.com/missing", "q") + self.assertIn("--doc-id", str(ctx.exception)) class TestGleanClientIndexingErrors(unittest.TestCase): diff --git a/tests/test_client_extended.py b/tests/test_client_extended.py index 27b529d..818644c 100644 --- a/tests/test_client_extended.py +++ b/tests/test_client_extended.py @@ -155,12 +155,12 @@ def test_deleteanswer_echoes_id(self): def test_summarize_by_url_returns_summary(self): resp = _mock_response("/summarize", - {"documentSpec": {"url": "https://example.com/doc"}}) + {"documentSpecs": [{"url": "https://example.com/doc"}]}) self.assertIn("summary", resp) self.assertIn("https://example.com/doc", resp["summary"]) def test_summarize_by_id_returns_summary(self): - resp = _mock_response("/summarize", {"documentSpec": {"id": "doc_123"}}) + resp = _mock_response("/summarize", {"documentSpecs": [{"id": "doc_123"}]}) self.assertIn("summary", resp) self.assertIn("doc_123", resp["summary"]) @@ -394,14 +394,14 @@ def test_summarize_by_url(self): mock_fn.return_value = {"summary": "..."} self.client.summarize(url="https://example.com/doc") body = mock_fn.call_args[0][1] - self.assertEqual(body["documentSpec"]["url"], "https://example.com/doc") + self.assertEqual(body["documentSpecs"], [{"url": "https://example.com/doc"}]) def test_summarize_by_id(self): with patch("glean_code.client._mock_response") as mock_fn: mock_fn.return_value = {"summary": "..."} self.client.summarize(doc_id="doc_123") body = mock_fn.call_args[0][1] - self.assertEqual(body["documentSpec"]["id"], "doc_123") + self.assertEqual(body["documentSpecs"], [{"id": "doc_123"}]) def test_summarize_with_query(self): with patch("glean_code.client._mock_response") as mock_fn: @@ -418,10 +418,11 @@ def test_verification_list_default(self): self.assertNotIn("count", body) def test_verification_list_with_count(self): + # `count` is a query parameter in the spec, so it must NOT be in the body. with patch("glean_code.client._mock_response") as mock_fn: mock_fn.return_value = {"verifications": []} self.client.verification_list(count=10) - self.assertEqual(mock_fn.call_args[0][1]["count"], 10) + self.assertEqual(mock_fn.call_args[0][1], {}) def test_verification_verify_body(self): with patch("glean_code.client._mock_response") as mock_fn: @@ -580,3 +581,128 @@ def test_mock_attach_acks_with_count(self): if __name__ == "__main__": unittest.main() + + +class TestWireLevelRequests(unittest.TestCase): + """What actually goes on the wire: HTTP method, URL and body. + + These endpoints diverge from the "everything is a POST with a JSON body" + house style, and nothing exercised the live path before, which is how the + divergences went unnoticed. + """ + + def setUp(self): + from glean_code.config import Config + self.client = GleanClient( + Config(instance="acme-be.glean.com", api_token="t", mode="live") + ) + + def _capture(self, call): + """Run `call` against a stubbed urlopen and report the request made.""" + seen = {} + + class FakeResp: + def read(self_inner): + return b'{"ok":true}' + def __enter__(self_inner): + return self_inner + def __exit__(self_inner, *a): + return False + + def fake_urlopen(req, timeout=None): + seen["url"] = req.full_url + seen["method"] = req.get_method() + seen["data"] = req.data + return FakeResp() + + with patch("glean_code.client.urllib.request.urlopen", fake_urlopen): + call() + return seen + + # --- /tools/list is a GET --- + + def test_tools_list_is_a_get_with_no_body(self): + seen = self._capture(lambda: self.client.tools_list()) + self.assertEqual(seen["method"], "GET") + self.assertTrue(seen["url"].endswith("/rest/api/v1/tools/list")) + self.assertIsNone(seen["data"], "a GET must not carry a request body") + + def test_tools_list_filter_becomes_repeated_query_params(self): + seen = self._capture(lambda: self.client.tools_list(tool_names=["search", "chat"])) + self.assertEqual(seen["method"], "GET") + self.assertIn("toolNames=search", seen["url"]) + self.assertIn("toolNames=chat", seen["url"]) + + # --- /listverifications takes count as a query param --- + + def test_verification_count_goes_in_the_query_string(self): + seen = self._capture(lambda: self.client.verification_list(count=5)) + self.assertEqual(seen["method"], "POST") + self.assertTrue(seen["url"].endswith("/listverifications?count=5")) + + def test_verification_without_count_has_no_query_string(self): + seen = self._capture(lambda: self.client.verification_list()) + self.assertTrue(seen["url"].endswith("/listverifications")) + self.assertNotIn("?", seen["url"]) + + # --- the ordinary case still behaves --- + + def test_search_is_still_a_post_with_a_body(self): + seen = self._capture(lambda: self.client.search("q2 plan")) + self.assertEqual(seen["method"], "POST") + self.assertIsNotNone(seen["data"]) + self.assertNotIn("?", seen["url"]) + + +class TestSpecConformantBodies(unittest.TestCase): + """Request bodies for the endpoints whose field names were wrong.""" + + def setUp(self): + from glean_code.config import Config + self.client = GleanClient(Config(mode="mock", api_token="t", + instance="acme-be.glean.com")) + + def _body(self, call): + with patch("glean_code.client._mock_response") as m: + m.return_value = {} + call() + return m.call_args[0][1] + + def test_tools_call_sends_parameters_not_arguments(self): + body = self._body(lambda: self.client.tools_call("search", {"query": "pto"})) + self.assertEqual(body["parameters"], {"query": "pto"}) + self.assertNotIn("arguments", body) + + def test_person_sends_email_ids_as_a_list(self): + body = self._body(lambda: self.client.person("a@b.com")) + self.assertEqual(body["emailIds"], ["a@b.com"]) + self.assertNotIn("email", body) + + def test_summarize_sends_document_specs_as_a_list(self): + body = self._body(lambda: self.client.summarize(doc_id="d1")) + self.assertEqual(body["documentSpecs"], [{"id": "d1"}]) + self.assertNotIn("documentSpec", body) + + def test_agent_run_sends_snake_case_agent_id(self): + body = self._body(lambda: self.client.agent_run("agt_1", "hello")) + self.assertEqual(body["agent_id"], "agt_1") + self.assertNotIn("agentId", body) + + def test_feedback_maps_thumbs_to_vote_events(self): + up = self._body(lambda: self.client.feedback("tok", "THUMBS_UP")) + self.assertEqual(up["event"], "UPVOTE") + down = self._body(lambda: self.client.feedback("tok", "THUMBS_DOWN")) + self.assertEqual(down["event"], "DOWNVOTE") + + def test_feedback_passes_other_events_through(self): + body = self._body(lambda: self.client.feedback("tok", "click")) + self.assertEqual(body["event"], "CLICK") + + def test_feedback_keeps_category_separate_from_event(self): + body = self._body(lambda: self.client.feedback("tok", "UPVOTE", category="search")) + self.assertEqual(body["event"], "UPVOTE") + self.assertEqual(body["category"], "SEARCH") + + def test_document_permissions_sends_a_bare_document_id(self): + body = self._body(lambda: self.client.document_permissions("d1")) + self.assertEqual(body, {"documentId": "d1"}) diff --git a/tests/test_commands.py b/tests/test_commands.py index 031bc0a..d2d60e6 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -695,7 +695,7 @@ def test_create_calls_client(self): s, [], {"url": "https://example.com/pto", "query": "pto policy"} ) s.client.pin_create.assert_called_once_with( - "https://example.com/pto", "pto policy" + url="https://example.com/pto", query="pto policy", doc_id=None ) def test_list_calls_client(self): diff --git a/tests/test_mock_corpus.py b/tests/test_mock_corpus.py index 407c640..dae18d3 100644 --- a/tests/test_mock_corpus.py +++ b/tests/test_mock_corpus.py @@ -130,7 +130,7 @@ def test_search_result_url_resolves_in_getdocuments(self): def test_search_result_url_summarizes_to_the_same_document(self): hit = _mock_response("/search", {"query": "pto policy", "pageSize": 1})["results"][0] summary = _mock_response("/summarize", - {"documentSpec": {"url": hit["url"]}})["summary"] + {"documentSpecs": [{"url": hit["url"]}]})["summary"] self.assertIn(hit["title"], summary) def test_chat_citations_track_the_question(self): @@ -154,14 +154,15 @@ def test_search_honours_facet_filter_shape(self): def test_permissions_owner_is_the_document_author(self): doc = mock_corpus.find({"id": "doc_plan_charter"}) - perms = _mock_response("/getdocumentpermissions", - {"documentSpec": {"id": "doc_plan_charter"}})["permissions"] + perms = _mock_response("/getdocpermissions", + {"documentId": "doc_plan_charter"})["permissions"] self.assertEqual(perms[0]["email"], doc["author"]) self.assertEqual(perms[0]["role"], "owner") def test_people_lookup_uses_the_roster(self): - resp = _mock_response("/people", {"email": "priya.raman@acme.com"}) + resp = _mock_response("/people", {"emailIds": ["priya.raman@acme.com"]}) self.assertEqual(resp["name"], "Priya Raman") + self.assertEqual(resp["email"], "priya.raman@acme.com") def test_no_endpoint_leaks_raw_placeholders(self): calls = [ diff --git a/tests/test_spec_conformance.py b/tests/test_spec_conformance.py new file mode 100644 index 0000000..afcdadf --- /dev/null +++ b/tests/test_spec_conformance.py @@ -0,0 +1,210 @@ +"""Every REST path in client.py must exist in Glean's published OpenAPI spec. + +This is the guard the August 2026 audit recommended. Twelve Client API calls had +drifted from the spec — wrong paths, a POST where the spec says GET, misnamed +body fields — and nothing caught them, because the mock dispatcher is keyed on +the same path strings the live client posts to. A wrong path was wrong +consistently, so the suite passed while the calls would 404 against a tenant. + +The check is structural: it reads client.py with `ast` and compares every +endpoint it calls against tests/spec_manifest.json, which is generated from the +published specs by tools/refresh_spec_manifest.py. No network access, per the +project's rule that tests make no network calls. + +It deliberately does NOT require the client to cover every documented endpoint — +plenty are intentionally unimplemented. It only asserts that what the client +does call, exists. +""" +import ast +import json +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +REPO = Path(__file__).resolve().parent.parent +CLIENT_PY = REPO / "glean_code" / "client.py" +MANIFEST = Path(__file__).resolve().parent / "spec_manifest.json" + +# How each helper maps onto a surface in the manifest. +POST_HELPERS = {"_post": "client", "_indexing_post": "indexing"} +BASE_TO_SURFACE = { + "effective_indexing_base_url": "indexing", + "effective_metadata_base_url": "metadata", + "effective_base_url": "client", +} + + +def _literal_path(node): + """Render a path argument as the manifest spells it, or None. + + Handles plain strings and f-strings; an f-string's substitutions become {} + so that f"/debug/{datasource}/status" matches /debug/{}/status. + """ + if isinstance(node, ast.Constant) and isinstance(node.value, str): + return node.value + if isinstance(node, ast.JoinedStr): + out = [] + for part in node.values: + if isinstance(part, ast.Constant) and isinstance(part.value, str): + out.append(part.value) + else: + out.append("{}") + return "".join(out) + return None + + +def _kwarg(call, name): + for kw in call.keywords: + if kw.arg == name: + return _literal_path(kw.value) or ( + kw.value.value if isinstance(kw.value, ast.Constant) else None + ) + return None + + +def collect_calls(): + """Return {(surface, path, method)} for every endpoint client.py calls.""" + tree = ast.parse(CLIENT_PY.read_text(encoding="utf-8")) + found = set() + + for fn in ast.walk(tree): + if not isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + # Paths appearing anywhere in this function, for the case where the + # call site passes a variable (agent_run picks wait vs stream). + local_paths = [ + s.value for s in ast.walk(fn) + if isinstance(s, ast.Constant) and isinstance(s.value, str) + and s.value.startswith("/") and " " not in s.value + ] + + for call in ast.walk(fn): + if not isinstance(call, ast.Call) or not isinstance(call.func, ast.Attribute): + continue + helper = call.func.attr + + if helper in POST_HELPERS: + surface = POST_HELPERS[helper] + if not call.args: + continue + method = _kwarg(call, "method") or "POST" + path = _literal_path(call.args[0]) + if path: + found.add((surface, path, method)) + elif isinstance(call.args[0], ast.Name): + # Variable path: attribute every literal in the function. + for p in local_paths: + found.add((surface, p, method)) + + elif helper == "_indexing_request": + # _indexing_request(method, base, path, body=...) + if len(call.args) < 3: + continue + method = _literal_path(call.args[0]) or "POST" + base = call.args[1] + surface = "indexing" + if isinstance(base, ast.Attribute): + surface = BASE_TO_SURFACE.get(base.attr, "indexing") + path = _literal_path(call.args[2]) + if path: + found.add((surface, path, method)) + + return found + + +class TestSpecManifest(unittest.TestCase): + def test_manifest_exists_and_is_populated(self): + self.assertTrue(MANIFEST.exists(), + "run tools/refresh_spec_manifest.py to generate it") + data = json.loads(MANIFEST.read_text()) + self.assertIn("surfaces", data) + for surface in ("client", "indexing", "metadata"): + self.assertGreater(len(data["surfaces"].get(surface, {})), 0, + f"{surface} surface is empty") + + def test_manifest_records_the_spec_version_it_came_from(self): + data = json.loads(MANIFEST.read_text()) + self.assertRegex(data.get("spec_version", ""), r"^\d+\.\d+") + self.assertRegex(data.get("spec_commit", ""), r"^[0-9a-f]{7,40}$") + + +class TestClientConformsToSpec(unittest.TestCase): + """The regression guard itself.""" + + @classmethod + def setUpClass(cls): + data = json.loads(MANIFEST.read_text()) + cls.surfaces = data["surfaces"] + cls.undocumented = data.get("undocumented", {}) + cls.calls = collect_calls() + + def _is_undocumented(self, surface, path): + return path in self.undocumented.get(surface, []) + + def test_extractor_found_the_calls(self): + # A silent extraction failure would make every other test here vacuous. + self.assertGreater(len(self.calls), 60, + "the AST extractor found suspiciously few calls") + + def test_every_path_exists_in_the_spec(self): + """A path absent from the spec entirely is a hard failure. + + A path the spec declares but leaves undocumented is tracked separately + (see test_undocumented_paths_are_still_undocumented) because the + endpoint does exist in the document — Glean has simply stopped + describing its operation. + """ + missing = sorted( + f"{surface}: {method} {path}" + for surface, path, method in self.calls + if path not in self.surfaces.get(surface, {}) + and not self._is_undocumented(surface, path) + ) + self.assertEqual(missing, [], "paths not present in the published spec:\n " + + "\n ".join(missing)) + + def test_undocumented_paths_are_still_undocumented(self): + """Self-cleaning exception list. + + The client calls /indexemployeelist, which the spec declares with no + operation. If Glean documents it again this test fails, which is the + signal to drop the exception and let the normal check cover it. + """ + called = {(s, p) for s, p, _ in self.calls} + for surface, paths in self.undocumented.items(): + for path in paths: + if (surface, path) in called: + self.assertNotIn( + path, self.surfaces.get(surface, {}), + f"{surface}:{path} is now documented — remove the exception", + ) + + def test_the_exception_list_stays_small(self): + """Undocumented endpoints are a risk, not a pattern to grow.""" + exercised = [ + (s, p) for s, ps in self.undocumented.items() for p in ps + if (s, p) in {(a, b) for a, b, _ in self.calls} + ] + self.assertLessEqual( + len(exercised), 2, + f"the client leans on too many undocumented endpoints: {exercised}", + ) + + def test_every_method_is_allowed_for_its_path(self): + wrong = [] + for surface, path, method in sorted(self.calls): + allowed = self.surfaces.get(surface, {}).get(path) + if allowed and method not in allowed: + wrong.append(f"{surface}: {method} {path} (spec allows {','.join(allowed)})") + self.assertEqual(wrong, [], "methods the spec does not allow:\n " + + "\n ".join(wrong)) + + def test_the_three_surfaces_are_all_exercised(self): + used = {surface for surface, _, _ in self.calls} + self.assertEqual(used, {"client", "indexing", "metadata"}) + + +if __name__ == "__main__": + unittest.main() diff --git a/tools/refresh_spec_manifest.py b/tools/refresh_spec_manifest.py new file mode 100644 index 0000000..b7c0e12 --- /dev/null +++ b/tools/refresh_spec_manifest.py @@ -0,0 +1,141 @@ +#!/usr/bin/env python3 +"""Regenerate tests/spec_manifest.json from Glean's published OpenAPI specs. + +The conformance test (tests/test_spec_conformance.py) must run offline, so it +compares client.py against this committed manifest rather than the live spec. +Run this whenever Glean publishes a spec update: + + python3 tools/refresh_spec_manifest.py # fetch and rewrite + python3 tools/refresh_spec_manifest.py --check # fail if stale (for CI) + +This is the only file in the repo that reaches the network, and it is not +imported by the package or the test suite. + +Requires PyYAML, which is a tooling dependency only — never a runtime one. +""" +from __future__ import annotations + +import argparse +import json +import re +import sys +import urllib.request +from pathlib import Path +from typing import Any, Dict + +SPECS = { + "client": "https://gleanwork.github.io/open-api/specs/final/client_rest.yaml", + "indexing": "https://gleanwork.github.io/open-api/specs/final/indexing.yaml", +} + +# The three base paths the client speaks to, longest first so that +# /rest/api/index is not swallowed by /rest/api. +BASES = [ + ("client", "/rest/api/v1"), + ("metadata", "/rest/api/index"), + ("indexing", "/api/index/v1"), +] + +MANIFEST = Path(__file__).resolve().parent.parent / "tests" / "spec_manifest.json" +HTTP_METHODS = ("get", "post", "put", "delete", "patch") + + +def normalise(path: str) -> str: + """Collapse path parameters so {groupName} and {group} compare equal. + + The spec and the client name their parameters differently; only the shape + of the path is a contract. + """ + return re.sub(r"\{[^}]*\}", "{}", path) + + +def fetch(url: str) -> str: + with urllib.request.urlopen(url, timeout=120) as resp: + return resp.read().decode("utf-8") + + +def build() -> Dict[str, Any]: + try: + import yaml + except ImportError: + sys.exit( + "refresh_spec_manifest needs PyYAML (a tooling-only dependency):\n" + " python3 -m pip install pyyaml" + ) + + surfaces: Dict[str, Dict[str, list]] = {name: {} for name, _ in BASES} + undocumented: Dict[str, list] = {} + meta: Dict[str, Any] = {} + + for label, url in SPECS.items(): + doc = yaml.safe_load(fetch(url)) + info = doc.get("info", {}) + meta.setdefault("spec_version", info.get("version")) + meta.setdefault("spec_commit", info.get("x-source-commit-sha")) + + for raw_path, node in (doc.get("paths") or {}).items(): + methods = sorted(m.upper() for m in (node or {}) if m in HTTP_METHODS) + for surface, base in BASES: + if raw_path.startswith(base): + rel = normalise(raw_path[len(base):]) or "/" + if methods: + surfaces[surface][rel] = methods + else: + # Declared in the spec but carrying no operation. Not the + # same as absent: the endpoint exists in the document and + # its request schema often still does too, but Glean has + # stopped documenting it. Recorded separately so the + # conformance test can treat it as a known risk rather + # than a hard failure. + undocumented.setdefault(surface, []).append(rel) + break + + return { + "_comment": ( + "Generated by tools/refresh_spec_manifest.py. Do not edit by hand. " + "Paths are relative to each surface's base URL, with path parameters " + "collapsed to {}." + ), + **meta, + "surfaces": {k: dict(sorted(v.items())) for k, v in surfaces.items()}, + "undocumented": {k: sorted(v) for k, v in sorted(undocumented.items())}, + } + + +def main(argv=None) -> int: + ap = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + ap.add_argument("--check", action="store_true", + help="exit non-zero if the committed manifest is out of date") + args = ap.parse_args(argv) + + fresh = build() + counts = {k: len(v) for k, v in fresh["surfaces"].items()} + + if args.check: + if not MANIFEST.exists(): + print("spec_manifest.json is missing", file=sys.stderr) + return 1 + current = json.loads(MANIFEST.read_text()) + if (current.get("surfaces") != fresh["surfaces"] + or current.get("undocumented") != fresh.get("undocumented")): + print( + "spec_manifest.json is out of date.\n" + f" committed: {current.get('spec_commit')}\n" + f" published: {fresh.get('spec_commit')}\n" + "Refresh it with: python3 tools/refresh_spec_manifest.py", + file=sys.stderr, + ) + return 1 + print(f"spec_manifest.json is current ({fresh['spec_commit'][:7]})") + return 0 + + MANIFEST.write_text(json.dumps(fresh, indent=2) + "\n") + print(f"wrote {MANIFEST.relative_to(Path.cwd()) if MANIFEST.is_relative_to(Path.cwd()) else MANIFEST}") + print(f" spec {fresh['spec_version']} @ {(fresh['spec_commit'] or '?')[:7]}") + for surface, n in counts.items(): + print(f" {surface:9} {n} paths") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main())