Skip to content

feat: add scalable workspace member and bot management#47

Merged
shakkernerd merged 31 commits into
mainfrom
feat/member-bot-management
Jul 10, 2026
Merged

feat: add scalable workspace member and bot management#47
shakkernerd merged 31 commits into
mainfrom
feat/member-bot-management

Conversation

@shakkernerd

Copy link
Copy Markdown
Member

Summary

  • add performant cursor-paginated workspace member listing with search, role filters, totals, and role breakdowns
  • add workspace and account settings interfaces for member and bot management
  • tighten authorization for service bots and user-owned bots
  • add bot removal, owned-bot listing, and redacted token metadata
  • add valid OpenClaw configuration and token setup snippets
  • update OpenAPI, TypeScript SDK contracts, documentation, and embedded web assets

Performance

  • use stable keyset pagination instead of offset pagination
  • add indexed member sort and search keys
  • calculate counts only on the first page
  • add Unicode-aware SQLite search normalization
  • batch bot-listing queries to avoid N+1 database access

Authorization

  • workspace managers create and manage service bots
  • only the owner creates or manages tokens for a user-owned bot
  • workspace managers can remove bots from a workspace
  • bot removal revokes active workspace tokens
  • raw bot tokens remain visible only when created

Verification

  • pnpm check
  • pnpm test:e2e — 45 passed
  • automated branch review findings resolved

@clawsweeper clawsweeper Bot added the rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. label Jul 10, 2026
@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal priority bug or improvement with limited blast radius. labels Jul 10, 2026
@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 10, 2026
@clawsweeper

clawsweeper Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed July 10, 2026, 9:59 AM ET / 13:59 UTC.

Summary
The PR adds scalable workspace-member discovery and account/workspace bot management across database schemas, authorization, HTTP/OpenAPI contracts, the TypeScript SDK, documentation, and web settings UI.

Reproducibility: yes. for the blocking compatibility defect: omitting --created-by deterministically reaches both stores with an empty actor and returns created_by is required. The broader requested capability is a feature rather than a reported current-main bug.

Review metrics: 4 noteworthy metrics.

  • Prior findings resolved: 2 of 3 fixed. Both unreachable helpers are removed, while the P1 admin CLI compatibility defect remains.
  • Change surface: 111 files, +9,303/-484. The branch crosses persistent storage, authorization, public contracts, web UI, documentation, and generated assets.
  • Database migrations: 3 added. One PostgreSQL and two SQLite migrations require fresh-install and existing-database validation.
  • Release-owned files: 1 changed. The PR directly edits CHANGELOG.md even though release work owns that file.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Resolve the trusted-admin CLI compatibility boundary consistently in code, docs, and tests.
  • Remove the CHANGELOG.md edit.
  • [P1] Add redacted live fresh-install and upgrade proof for both database backends and bot ownership modes.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR reports automated checks and E2E tests but no redacted live fresh-install or upgrade evidence; add terminal/API output and browser recordings, redact private data, then update the PR body to trigger review or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] Existing scripts and deployments invoking clickclack admin bot create without --created-by will stop working after upgrade.
  • [P1] The persistent member sort-key migrations have no inspected real pre-existing-database upgrade proof for either supported backend.
  • [P1] Authorization now spans legacy and workspace-scoped routes, CLI behavior, service bots, user-owned bots, both stores, SDK helpers, and UI gates; inconsistency could expose or deny credential-management operations.

Maintainer options:

  1. Restore compatibility and prove upgrades (recommended)
    Preserve a supported trusted-admin path, remove the changelog edit, and add redacted fresh-install and upgrade evidence for both databases before merge.
  2. Approve the breaking actor boundary
    Keep the mandatory actor requirement only with explicit maintainer approval, complete operator migration documentation, and upgrade validation.
  3. Pause the combined feature
    Pause or split the PR if the settings UI, stored-model migrations, legacy APIs, and authorization changes cannot be reviewed and proven coherently.

Next step before merge

  • [P2] A maintainer must first choose the trusted local-admin security and compatibility contract; after that, the changelog cleanup and required proof are concrete contributor follow-up.

Maintainer decision needed

  • Question: Should trusted local administrators retain a backward-compatible bot-creation path, or must every CLI bot creation identify an existing authorized workspace actor with --created-by?
  • Rationale: Explicit actor attribution strengthens authorization auditing, but making it mandatory changes a shipped local-administration contract; repository history cannot determine which trust boundary maintainers intend.
  • Likely owner: steipete — The shipped bot/admin contract and recent adjacent administrative deployment flow trace directly to his merged commits.
  • Options:
    • Preserve trusted-admin compatibility (recommended): Keep existing local-admin bot creation working while retaining strict actor and ownership checks for HTTP and explicitly attributed operations.
    • Require an authorized actor: Approve the breaking administrative migration and require complete operator guidance, updated recipes, and fresh/upgrade proof.
    • Pause the broad landing: Pause this PR if the CLI, schema migrations, legacy routes, and authorization model cannot be validated as one coherent upgrade.

Security
Needs attention: The ownership rules are directionally stricter, but the trusted local-admin actor boundary and upgraded credential lifecycle still require maintainer approval and live validation.

Review findings

  • [P1] Preserve the existing admin bot-creation contract — apps/api/cmd/clickclack/main.go:294
  • [P2] Remove the release-owned changelog entry — CHANGELOG.md:5
Review details

Best possible solution:

Preserve a coherent trusted-admin and HTTP authorization model, remove the release-owned changelog edit, and demonstrate fresh-install plus upgrade behavior for SQLite and PostgreSQL across service- and user-owned bot workflows.

Do we have a high-confidence way to reproduce the issue?

Yes for the blocking compatibility defect: omitting --created-by deterministically reaches both stores with an empty actor and returns created_by is required. The broader requested capability is a feature rather than a reported current-main bug.

Is this the best way to solve the issue?

No, not yet: the scalable member-listing approach is coherent, but unconditional CLI breakage, a release-owned changelog edit, and missing real upgrade proof prevent this from being the safest maintainable landing.

Full review comments:

  • [P1] Preserve the existing admin bot-creation contract — apps/api/cmd/clickclack/main.go:294
    Existing clickclack admin bot create invocations omit --created-by, but the branch now sends an empty actor to both stores and fails with created_by is required. Preserve a trusted-local-admin fallback, or obtain explicit maintainer approval and provide a complete breaking migration for scripts and deployments.
    Confidence: 0.99
  • [P2] Remove the release-owned changelog entry — CHANGELOG.md:5
    CHANGELOG.md is release-owned and normal feature PRs should not edit it. Remove this entry and keep the user-visible release-note context in the PR body or commit message; this line was unchanged at the earlier reviewed head, so this is a late discovery.
    Confidence: 0.99
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 956e78112199.

Label changes

Label justifications:

  • P2: This is a broad normal-priority feature with meaningful compatibility and authorization concerns but no evidence of an active production emergency.
  • merge-risk: 🚨 compatibility: Merging changes a shipped administrative command, legacy token-route behavior, SDK contracts, and persistent database schemas.
  • merge-risk: 🚨 security-boundary: Merging changes who may create bots and inspect, mint, revoke, or remove bot credentials and workspace memberships.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Needs real behavior proof before merge: The PR reports automated checks and E2E tests but no redacted live fresh-install or upgrade evidence; add terminal/API output and browser recordings, redact private data, then update the PR body to trigger review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

Security concerns:

  • [medium] Resolve the trusted local-admin actor boundary — apps/api/cmd/clickclack/main.go:294
    The store now requires an existing workspace actor for bot creation, changing the trust contract of a local administrative command. Maintainers must decide whether that command remains privileged independently or requires attributable membership before the authorization model can be considered settled.
    Confidence: 0.97

What I checked:

  • Feature remains unique: Current main lacks the new member-directory routes, workspace settings pages, bot-management endpoints, and associated stored sort keys added by this branch. (apps/api/internal/httpapi/server.go:117, 36be1e3ff16c)
  • CLI compatibility defect: The branch advertises --created-by as mandatory and forwards an empty value when omitted, while both changed store implementations reject empty CreatedBy; previously supported invocations therefore fail with created_by is required. (apps/api/cmd/clickclack/main.go:294, 36be1e3ff16c)
  • Prior findings resolved: The latest commit removes the unreachable token-list helper from both PostgreSQL and SQLite, resolving two findings from the previous review cycle. (apps/api/internal/store/postgres/bots.go:458, 36be1e3ff16c)
  • Stored-model upgrade surface: Three migrations add and backfill persistent member sort keys and indexes across SQLite and PostgreSQL, but the changed test set contains no focused pre-existing-database upgrade test for these migrations. (apps/api/internal/store/sqlite/migrations/0019_workspace_member_page_indexes.sql:1, 36be1e3ff16c)
  • Release-owned file changed: The branch adds its own Unreleased changelog entry even though repository review policy reserves CHANGELOG.md for release work; this line was already present at the previously reviewed head. (CHANGELOG.md:5, 36be1e3ff16c)
  • Feature-history provenance: The shipped admin bot command and both database bot implementations trace to the v0.1.0 release commit, with recent staging work also touching the administrative CLI surface. (apps/api/cmd/clickclack/main.go:292, 8453b5f758c9)

Likely related people:

  • steipete: The v0.1.0 commit introduced the central CLI and store contract, and the recent FakeCo staging integration also maintained the adjacent administrative flow. (role: introduced shipped bot/admin behavior; confidence: high; commits: 8453b5f758c9, 54dba16c27c1; files: apps/api/cmd/clickclack/main.go, apps/api/internal/store/sqlite/bots.go, apps/api/internal/store/postgres/bots.go)
  • shakkernerd: Prior merged work covers chat storage scaling, route-driven web navigation, and the current-main realtime cursor, making this contributor relevant to the data-scaling and web portions beyond merely proposing this PR. (role: recent adjacent area contributor; confidence: medium; commits: 4d52642c79b7, 9bbeab6b74e0, 956e78112199; files: apps/web/src/ChatApp.svelte, apps/api/internal/store/sqlite/members.go, tests/e2e/chat.spec.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-07-10T13:46:17.903Z sha 888b69b :: found issues before merge. :: [P1] Preserve existing admin bot creation | [P2] Remove the unreachable PostgreSQL helper | [P2] Remove the unreachable SQLite helper

@shakkernerd shakkernerd self-assigned this Jul 10, 2026
@shakkernerd shakkernerd merged commit 1ef89aa into main Jul 10, 2026
7 checks passed
@shakkernerd

Copy link
Copy Markdown
Member Author

Merged using rebase.

What changed after review:

  • Preserved explicit workspace actor attribution in admin bot creation and corrected all documented command paths.
  • Added SQLite and PostgreSQL upgrade proof for member sort keys, indexes, Unicode search, and pagination.
  • Removed dead bot-token helpers and the release-owned changelog entry.
  • Reset workspace settings state across client-side workspace switches.

Verification:

  • Rebased onto current origin/main before pushing.
  • AWS Crabbox cbx_aaf9088ed1ef: live PostgreSQL migration/smoke/concurrency (run_9ad9fab87fda), SQLite migration, and CLI bot creation (run_d040892e7477).
  • Full pnpm check and focused Playwright workspace-switch regression passed.
  • Final exact-head review: no actionable findings remain.
  • GitHub Go, TypeScript, Playwright, Docker, Linux, macOS, and Windows checks all passed.

Landed on main:

  • 956e7811219972c8cdb31633526ca731547d936c...1ef89aafc874f267e2a432c633148b1c1b200d2a

Thanks @shakkernerd!

@shakkernerd shakkernerd deleted the feat/member-bot-management branch July 10, 2026 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant