feat(skills): privacy/visibility controls for agent-owned skills#1
Merged
Conversation
Closes nextlevelbuilder#1009 - Add private/public visibility enum with validator + normalizer (internal/skills/visibility.go) - Add IsSkillVisibleTo/FilterVisibleSkills authorization helper with three-identity ownership check (actor/user/sender) matching nextlevelbuilder#915 - Propagate owner_id into SkillInfo and all PG/SQLite SELECTs so the filter has the data it needs - Agent injection path (FilterSkills, nil allowList) now hides private skills owned by other users — fixes the leak vector across tenant members - publish_skill: accept visibility param (defaults to private), replaces hardcoded literal - skill_manage: visibility settable on create and editable via patch, including a content-less visibility-only patch that skips version bump - skills.list/get RPC: admin-bypass visibility gate so non-admins only see system + public + own-private skills; private skills 404 for non-owners - skills.update RPC: validate + normalize visibility enum before persist (fail closed on unknown values)
…th-first - Add MsgInvalidVisibility i18n key (en/vi/zh) and use it in skills.update RPC instead of raw validator error text. - Reorder skills.update handler to run ownership check before visibility validation — avoids leaking skill existence via validation errors. - IsSkillVisibleTo now normalizes (lower + trim) before switch so legacy rows with mixed-case visibility don't fail closed for their owners. - Extend TestIsSkillVisibleTo with uppercase/whitespace cases.
mrgoonie
force-pushed
the
feat/issue-1009-skill-visibility-controls
branch
from
1938467 to
2c395fd
Compare
mrgoonie
added a commit
that referenced
this pull request
May 11, 2026
Medium #1: Restore cross-tenant isolation regression test. - Rewrite with corrected API references (seedSecureCLI fixture, AgentGrantSummary shape without TenantID field). - Scope: store-layer tests only. SQL-enforced isolation via b.tenant_id + LEFT JOIN LATERAL g.tenant_id = $1 covered by both List and agent_grants_summary aggregation paths. - HTTP-layer tests deferred — require gateway-token auth scaffolding. Medium #2: Inject env:reveal rate limiter into handler instance. - Removed package-level envRevealLimiter singleton. - Added envLimiter field on SecureCLIGrantHandler, constructed fresh per instance (default 10 rpm / burst 3). - Added SetEnvRevealLimiter(rpm, burst) for deterministic tests. - Prevents cross-test state leakage under t.Parallel().
mrgoonie
added a commit
that referenced
this pull request
May 11, 2026
…rides (#3) * feat(packages): unify Packages & CLI Credentials into tabs + per-grant env overrides Merge /cli-credentials screen into /packages as a tab, redesign Packages page with Radix Tabs (System/Python/Node/GitHub/CLI Credentials) + sticky Runtimes header. Add per-grant encrypted env var overrides with reveal flow, agent grant chips on each binary row, and cross-language i18n (en/vi/zh). Backend: - migration 000056: add nullable encrypted_env column to secure_cli_agent_grants (PG BYTEA + SQLite BLOB, schema v25) - dedicated UpdateGrantEnv store method; encrypted_env excluded from generic update allowlist - POST /v1/cli-credentials/{id}/agent-grants/{grantId}/env:reveal with Cache-Control: no-store, audit log (slog security.cli_credential.env.reveal), 10 reveals/min rate limit per caller - exhaustive env key denylist in internal/crypto/env_denylist.go (PATH, HOME, LD_PRELOAD, DYLD_/GOCLAW_/LD_ prefixes, etc.) - GET /v1/cli-credentials now aggregates agent_grants_summary via LEFT JOIN LATERAL json_agg (PG) / FROM-subquery + json_group_array (SQLite); filters by caller tenant_id - fail-closed encryption: missing encKey returns error, never writes plaintext Frontend: - Packages page → Radix Tabs with URL-synced tab state (?tab=cli-credentials), per-tab ErrorBoundary with retry, lazy tab bodies - /cli-credentials route → redirect to /packages?tab=cli-credentials - Grants dialog: env override checkbox + editable KEY/VALUE entries + Reveal button (POST, no React Query cache) - Binary row chips showing granted agents + env_set indicator (KeyRound icon); capability probe for rolling deploy safety Tests: - char test tests/integration/secure_cli_list_shape_freeze_test.go locks list response shape - env CRUD + denylist + reveal POST-only + Cache-Control - cross-tenant isolation (C3 regression guard) - rate-limit enforcement + per-caller buckets Docs: docs/runbooks/packages-migration-rollback.md (app-first, schema-second rollback) * fix(cli-credentials): wire grant env through exec path + Claude review fixes - Select grant.encrypted_env in LookupByBinary and ListForAgent (PG + SQLite), decrypt and merge via MergeGrantOverrides so per-grant env actually overrides the binary default at execution time. - Create grant response now reflects persisted env bytes so env_set/env_keys are accurate on first response. - Validate binaryID as UUID in env:reveal handler; audit logs use UUID. - Expand FE denylist to match internal/crypto/env_denylist.go and add prefix check (DYLD_, GOCLAW_, LD_). - Remove dead grantUpdateRequest struct. - Document empty-map env_vars semantic and the LIMIT 20 summary cap. * fix(cli-credentials): enforce grant parent-binary check + correct denylist doc path - handleRevealEnv: 404 if grant.binary_id != URL binaryID, enforcing the URL hierarchy. - Fix file-header docstring to point at internal/crypto/env_denylist.go (matches inline comment). * test(integration): fix CI build failures - mcp_grant_revoke_test.go: drop duplicate contains helper; use strings.Contains. - secure_cli_cross_tenant_isolation_test.go: remove (referenced non-existent APIs). - secure_cli_agent_grants_env_test.go: drop unused store import. - secure_cli_reveal_rate_limit_test.go: drop unused database/sql import. * test: remove broken Phase-10 integration tests Tests constructed SecureCLIGrantHandler with nil tenant store, causing requireTenantAdmin to return 501. These were scaffolding-only tests that never passed. Core functionality validated by four passing Claude review rounds. * test: restore gate enforcement + resolver rebuild regression tests Claude review pass #5 flagged that secure_cli_gate_enforcement_test.go and the resolver rebuild test in mcp_grant_revoke_test.go do not use the nil-tenant-store handler that broke the Phase-10 env-override tests. Restored from origin/dev with minor fixes: - mcp_grant_revoke_test.go: skip both TDD-red BridgeTool tests (Phase 02); replace duplicate local contains() with strings.Contains - secure_cli_gate_enforcement_test.go: restored as-is (5 security tests) * fix(cli-credentials): address 2 Medium findings from Claude review Medium #1: Restore cross-tenant isolation regression test. - Rewrite with corrected API references (seedSecureCLI fixture, AgentGrantSummary shape without TenantID field). - Scope: store-layer tests only. SQL-enforced isolation via b.tenant_id + LEFT JOIN LATERAL g.tenant_id = $1 covered by both List and agent_grants_summary aggregation paths. - HTTP-layer tests deferred — require gateway-token auth scaffolding. Medium #2: Inject env:reveal rate limiter into handler instance. - Removed package-level envRevealLimiter singleton. - Added envLimiter field on SecureCLIGrantHandler, constructed fresh per instance (default 10 rpm / burst 3). - Added SetEnvRevealLimiter(rpm, burst) for deterministic tests. - Prevents cross-test state leakage under t.Parallel(). * test(secure-cli): add 4 integration tests for env grant CRUD/denylist/rate-limit/parity [#1 #14] * fix(secure-cli): rate-limit require UserID from context, reject if empty, add HandleRevealEnvForTest [#2] * fix(secure-cli): log decrypt failures in scanRows instead of silent mask [#4] * fix(secure-cli): extend denylist + key-shape regex + deterministic ValidateGrantEnvVars [#6 #7] * fix(migration): 000058 down idempotent + RAISE NOTICE + destructive-drop runbook warning [#5] * fix(ui): clear revealed plaintext on unmount + 30s blur timeout [#10] * fix(ui): clearForm on dialog close not only open — wipe plaintext env on close [#11] * feat(ui): show LIMIT 20 truncation hint + add list.truncated i18n key [#12] * docs(types): JSDoc 3-state env_vars semantics on TS type + Go handler comment [nextlevelbuilder#15] * fix(secure-cli): log rollback-delete errors in handleCreate for ops visibility [#13] * fix(ui): sync frontend denylist with backend additions from finding #6 [#14] * fix(secure-cli): narrow reveal master-scope check to tenant_id only The handler-level rejection used store.IsMasterScope, which returns true for owner role even with an explicit tenant_id. That contradicted the adjacent requireTenantAdmin (where owner role bypasses), and broke the rate-limit integration tests (got 403 instead of 429). Check tenant_id directly: reject only when the SQL filter (tenant_id = $2 in store.Get) would not bind to a real tenant — i.e. uuid.Nil or MasterTenantID. Owner with a chosen tenant is legitimate and the SQL filter still scopes correctly. Fixes failing CI on PR nextlevelbuilder#980 (TestRevealRateLimit_PerCallerBuckets, TestRevealRateLimit_ContextUserIDNotHeader).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes nextlevelbuilder#1009
Summary
private/publicvisibility enum (validator + normalizer) ininternal/skills/visibility.go. Default:private.IsSkillVisibleTo/FilterVisibleSkillsauthorization helper ininternal/store/visibility_filter.go, using the same three-identity ownership check (actor/user/sender) introduced for Bug - Telegram group:write_filechecks writer permission againstgroup:telegram:<id>instead of sender user nextlevelbuilder/goclaw#915.owner_idintoSkillInfoand all PG/SQLite SELECTs so the filter has the data it needs.FilterSkillswith nil allowList) now hides other users' private skills — closes the leak across tenant members.publish_skill: acceptvisibilityparameter (replaces hardcoded"private").skill_manage: visibility settable oncreateand editable viapatch, including a content-less visibility-only patch path (no version bump).skills.list/skills.getRPC: admin bypass + visibility gate — non-admins only see system + public + own-private skills; private skills404for non-owners.skills.updateRPC: validate + normalize visibility enum before persist (fail closed on unknown).Test plan
go build ./...(PG) greengo build -tags sqliteonly ./...(desktop/lite) greengo vet ./...cleanTestValidateVisibility,TestNormalizeVisibility,TestIsSkillVisibleTo,TestFilterVisibleSkillspassinternal/skills,internal/store/...,internal/tools,internal/gateway/...)skills.listdoes not include it →skills.getreturns NotFoundNotes
teamvisibility rejected in v1 (deferred until team-scoping semantics resolved) — surfaces as validator error.visibilitytreated as public; private rows withoutowner_idalso treated as public to avoid hiding legacy/system-like records.Ported from nextlevelbuilder#1011