feat(cli): add bm cloud share command group (create/list/update/revoke)#965
feat(cli): add bm cloud share command group (create/list/update/revoke)#965groksrc wants to merge 4 commits into
Conversation
Surface the cloud /api/shares endpoints from the CLI so users can manage
public share links for notes without leaving the terminal.
New `bm cloud share` subcommand group:
- create <project> <permalink> [--expires-at] -> POST /api/shares
- list [--project] -> GET /api/shares
- update <token> [--enable|--disable|--expires-at] -> PATCH /api/shares/{token}
- revoke <token> [--force] -> DELETE /api/shares/{token}
Reuses the existing make_api_request() helper, ConfigManager cloud_host
lookup, and SubscriptionRequired/CloudAPIError handling that snapshot.py
uses. Rich table output mirrors `bm project list`. Payloads match the
cloud CreateShareRequest/UpdateShareRequest contracts (project_name,
note_permalink, expires_at, enabled).
Adds PublicShareResponse/PublicShareListResponse schemas and unit tests
with mocked HTTP following the snapshot CLI test patterns.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
…emas Address review findings on `bm cloud share`: - `create` re-wrapped the `typer.Exit(1)` from `_parse_expires_at()` as a spurious "Unexpected error: 1" trailing line because the broad `except Exception` caught it (typer.Exit subclasses Exception). Parse and validate --expires-at before entering the try block so the error surfaces as a single clean message, and add the `except typer.Exit: raise` guard for parity with `update`/`revoke`. - Strengthen test_create_share_invalid_expires_at to assert "Unexpected error" is absent, which was masking the bug. - Drop unused PublicShareResponse/PublicShareListResponse schemas; responses are parsed as raw dicts, so the schemas were dead code. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22901c366d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| response = await make_api_request( | ||
| method="POST", | ||
| url=f"{host_url}/api/shares", | ||
| json_data=payload, | ||
| ) |
There was a problem hiding this comment.
For cloud projects that are configured in a non-default/team workspace, this request only authenticates and never sends the X-Workspace-ID header used elsewhere for workspace-scoped cloud calls, so /api/shares will be evaluated against the default tenant and the passed project name can be missing or refer to the wrong workspace. Resolve the workspace from the project/default config or expose --workspace, then pass it through the request headers for create/list/update/revoke.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and confirmed. The cloud /api/shares endpoints all depend on ResolvedWorkspaceDep, which resolve_workspace derives from the X-Workspace-ID header (see basic-memory-cloud deps.py); with no header the request falls back to the caller’s API-key/personal tenant, so a team-workspace project would resolve against the wrong tenant. Sibling commands already handle this in cloud_utils._workspace_headers.
Fixed in a8e452d: added a _workspace_headers() helper that resolves the workspace via resolve_configured_workspace (explicit --workspace > project’s configured workspace_id > global default) and sends X-Workspace-ID on create/list/update/revoke. Added a --workspace option to each command, mirroring bm cloud pull/push. New mocked tests assert the header is built and routed, and that an unresolved workspace sends no header (default tenant, unchanged behavior).
| url = f"{host_url}/api/shares" | ||
| if project: | ||
| url += f"?project_name={project}" |
There was a problem hiding this comment.
When --project contains query-reserved characters such as &, +, or # (project names are not restricted to URL-safe slugs), interpolating it directly into the URL changes what the server receives; for example R&D is sent as project_name=R plus an extra D parameter. Build this request with encoded query params so filtering works for valid project names with special characters.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a8e452d. The list filter now builds the query with urllib.parse.urlencode({"project_name": project}) instead of raw f-string interpolation, so a name like R&D+notes #1 is sent as a single project_name=R%26D%2Bnotes+%231 value rather than splitting into stray query params. Extended the mocked tests with a special-characters case asserting both the encoded form is present and the raw ambiguous form never reaches the URL.
… project filter The `bm cloud share` commands authenticated but never sent the X-Workspace-ID header that sibling cloud calls (cloud_utils._workspace_headers) use for workspace scoping. The cloud /api/shares endpoints resolve the target tenant from that header (resolve_workspace in basic-memory-cloud deps.py), so share create/list/update/revoke for a team-workspace project were evaluated against the caller's default tenant. Resolve the workspace via resolve_configured_workspace (explicit --workspace > project's configured workspace_id > global default) and pass X-Workspace-ID on all four commands, mirroring the established cloud command pattern. Adds a --workspace option to each, matching `bm cloud pull/push`. Also URL-encode the list `--project` filter with urllib.parse.urlencode so project names containing query-reserved characters (&, +, #, spaces) reach the server as a single faithful value instead of splitting into stray query params. Extends the mocked tests to assert the header is built/routed and that a project name with special characters is percent-encoded. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
…space Live prod QA found `bm cloud share ... --workspace <slug>` failed with an opaque 400: resolve_configured_workspace returns the explicit --workspace value verbatim, but the cloud's X-Workspace-ID resolver only accepts a workspace/tenant UUID. Users see slugs and display names (list-workspaces output, memory:// URLs), so the natural input never routed. The share commands now resolve the workspace header client-side: a UUID is forwarded verbatim (covers per-project config workspace_id and the default chain, zero extra API calls), while any other value is treated as a human identifier and mapped to the tenant UUID via a single get_available_workspaces lookup, matching with slug > tenant_id > name precedence (mirroring #979). Ambiguous and unknown identifiers fail fast with errors that name the candidate / available workspace slugs, and the list command now re-raises typer.Exit ahead of its broad handler so the resolution errors aren't re-wrapped as "Unexpected error". Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
|
Live prod QA found that This commit resolves the workspace header client-side in the share commands:
🤖 Generated with Claude Code |
|
This is good to go, but it needs to ship in v0.next and we have a maintenance release ahead of it. |
1 participant
Closes #880
Summary
Adds a
bm cloud sharesubcommand group that surfaces the cloud/api/sharesendpoints. Four subcommands are implemented insrc/basic_memory/cli/commands/cloud/shares.py:create <project> <permalink> [--expires-at/-e]— POST/api/shares; printsshare_urlplus metadata on success.list [--project/-p]— GET/api/shares(optional?project_name=filter); displays a rich Table with Token / Project / Note / Enabled / Expires / Views / URL columns.update <token> [--enable|--disable|--expires-at]— PATCH/api/shares/{token};--expires-at noneclears the expiry (sendsnull); rejects conflicting (--enable+--disable) or empty updates before hitting the network.revoke <token> [--force/-f]— DELETE/api/shares/{token}; interactive confirmation prompt unless--force.Design decisions
revokesubcommand rather than a top-level flag or alias, matching the issue's recommended minimal surface.updateexposes--enable/--disable/--expires-atbecause the PATCH endpoint andUpdateShareRequest(withmodel_fields_setsemantics) support all three toggles.make_api_request(),ConfigManager().config.cloud_host, andSubscriptionRequiredError/CloudAPIErrorhandling consistent withsnapshot.py.cloud/__init__.pyviacloud_app.add_typer(share_app, name="share").Files changed (4 files, +1001 lines)
src/basic_memory/cli/commands/cloud/shares.pysrc/basic_memory/cli/commands/cloud/__init__.pyshare_appsrc/basic_memory/cli/commands/cloud/schemas.pytests/cli/test_share_commands.pyTesting
--expires-atparsing/validation, enable/disable conflict and empty-update guards, and revoke confirmation cancel.ruff check/ruff formatandty typecheckclean on changed files.basic-memory-cloudsource (CreateShareRequest,UpdateShareRequest, DELETE → 204,project_namequery param).Review notes
Dead code (schemas):
PublicShareResponseandPublicShareListResponsePydantic schemas were added tosrc/.../cloud/schemas.pybut are never imported or used byshares.py, which parses all responses with rawdata.get(...)dicts. By contrastsnapshot.pyboth defines and uses its schema (BucketSnapshotBrowseResponse.model_validate(...)). Either wire the schemas into response parsing (more type-safe, matches house style preferring Pydantic at boundaries) or drop them. — Update: they were dropped in the fix commit; this note is here for visibility in case the reviewer prefers them wired in instead.URL encoding in
list: The query string is built via an unencoded f-string?project_name={project}, so a project name with spaces/special chars would produce a malformed URL. This matches the existingsnapshot.pyconvention (?prefix={prefix}), so it is consistent with the repo and not a regression, but usinghttpx params=orurllib.parse.quotewould be more correct.API contract verified accurate:
CreateShareRequest(project_name / note_permalink / expires_at),UpdateShareRequest(enabled / expires_at, withmodel_fields_setsemantics so sending"expires_at": nullcorrectly clears it), DELETE returns 204, list filter viaproject_nameQuery param. The--expires-at none→ null clear path is correct.Coverage gate: Could not reproduce the repo's 100% coverage gate locally (cov + beartype circular-import env issue unrelated to this change), but branch coverage looks complete by inspection. All 21 new tests exercise distinct paths.
🤖 Generated with Claude Code