fix(permissions): validate set --allow/--deny patterns against known operations (#688) - #693
Conversation
padak
left a comment
There was a problem hiding this comment.
Review of #693 — fix(permissions): validate permissions set --allow/--deny patterns against known operations
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR closes #688: kbagent permissions set --allow/--deny PATTERN used to accept any string with zero validation, silently persisting dead rules (typos, fabricated operation names) that matched nothing at runtime. The fix adds a shared pattern_matches_known_operation() helper used both to reject unknown patterns at permissions set write time (VALIDATION_ERROR, exit 2, before the interactive confirmation) and to generalize find_inert_patterns() (used by permissions show / kbagent doctor) so it flags any dead pattern, not just the retired tool:* namespace. This is a well-scoped, well-tested fix to an existing command's validation logic — no new CLI command, no new command group, no server-router or OPERATION_REGISTRY surface to update. All documentation drift surfaces I checked were updated correctly, make check passes clean (6171 passed, 12 skipped), and I reproduced every claimed behavior live (JSON + human mode rejection, permissions show mixed-hint output, doctor warn). Verdict: APPROVE — no blocking findings.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 2
- Nits: 0
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/services/doctor_service.py:386 — details["inert_since"] is now conditionally present instead of always present
Before this PR, doctor --json's inert_permission_patterns check always included details.inert_since whenever status == "warn" (the only inert patterns possible were tool:*). After the generalization, inert_since is included only when at least one offending pattern starts with tool: — a policy with only typo'd patterns (e.g. stroage.upload-table) now omits the key entirely. This is intentional and tested (tests/test_doctor_service.py::test_warn_lists_generic_dead_patterns_without_tool_prefix asserts "inert_since" not in result["details"]), and no in-repo consumer reads it unconditionally, so it's low risk. Still, any external kbagent serve / doctor --json consumer that previously assumed details.inert_since is always present on a warn status will now hit a KeyError/None for a typo-only policy. Worth a one-line callout in the PR description or docs/web-server-endpoints.md-adjacent doctor docs if any external tooling depends on this shape (I found none in this repo).
[NB-2] plugins/kbagent/agents/keboola-expert.md §3 — no inline gotcha added for the new permissions set validation
keboola-expert.md §3 (Inline Gotchas) has no entry for permissions set at all — not for this PR's new VALIDATION_ERROR rejection, and not even for the pre-existing tool:* inert-pattern gotcha it builds on. This is consistent with pre-existing debt (the tool:* gotcha was never added there either), and per CONTRIBUTING.md's checklist this is explicitly non-blocking judgement territory ("if the command's behavior is non-obvious, add a bullet"). Since the keboola-expert subagent occasionally proposes permissions set calls on behalf of a user, a short bullet ("a typo'd/fabricated pattern in permissions set --allow/--deny now fails fast with VALIDATION_ERROR instead of silently persisting") would help it self-correct instead of retrying with a guessed pattern name. Optional; the behavior is fully documented in gotchas.md and AGENT_CONTEXT, both of which the agent can reach.
Nits
(none)
Verification log
gh auth status→ authenticated aspadak, scopes includerepo✓gh pr view 693 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ OPEN,main<-claude/issue-688-pr-23ae21, +514/-34 across 12 files, conventionalfix(permissions):matches the bug-fix nature of the change ✓git rev-parse --abbrev-ref HEAD→claude/issue-688-pr-23ae21matches<branch>input ✓git diff main...HEAD -- src/keboola_agent_cli/cli.py 'src/keboola_agent_cli/commands/**/*.py' | grep '\.command('→ empty: confirmed no new CLI command/group was added, soOPERATION_REGISTRY, server routers, and the §2 Tool Selection Matrix inkeboola-expert.mddo not need new entries ✓- Layer-violation greps (typer/formatter in
permissions.py/services/; httpx incommands/) → all empty, no violations ✓ - Raw
error_code="..."string literal grep → empty; confirmedErrorCode.VALIDATION_ERRORis a real enum member used correctly (categorical-first kwarg order per CONTRIBUTING.md) ✓ - Bare
except:,print()in production code, token-leak grep, newtuple[...]return grep, magic-number grep → all empty ✓ grep -rn "inert_since" src/ tests/ plugins/ docs/→ only doctor_service.py + its own tests reference it; no other consumer, confirms NB-1 is low-risk ✓make check(full run, background) →SKILL.md is up-to-date,version is in sync (plugin.json, marketplace.json, uv.lock),6171 passed, 12 skipped, 12 warnings— exit 0 ✓ (ran twice, both clean)- Doc drift surfaces checked against CONTRIBUTING.md's Plugin synchronization map:
CLAUDE.md✓,commands/context.pyAGENT_CONTEXT✓,commands-reference.md✓,gotchas.md(tagged(since vNEXT), includes the version-skew case) ✓,permissions-workflow.md✓,docs/guide.md✓. Nochangelog.pyentry and nopyproject.tomlversion bump — correct per the "no version bumps in feature PRs" rule (#648 process) ✓ - Live reproduction against
/tmp/kbagent-review-cfgdirconfig dir (throwaway, removed after):kbagent --json permissions set --mode deny --allow "tool.admin"→ exit 2,error.code: VALIDATION_ERROR,error.details.invalid_patterns: ["tool.admin"]✓ matches PR claimkbagent permissions set --mode deny --allow "stroage.upload-table"(human mode) → exit 2, readable error, no interactive prompt reached ✓- Persisted a policy with
deny: ["stroage.upload-table", "tool:write"]directly viaConfigStore(bypassing the new write-time gate, simulating a pre-existing policy) →permissions show(both human and--json) correctly shows both thetool:-specific MCP-migration hint AND the generic typo hint,inert_patternslists both ✓ kbagent doctor --jsonon the same config →inert_permission_patternscheck statuswarn, message includes both hints,details.inert_since == "0.85.0"(present because atool:pattern was in the mix) ✓
python3 -c "... pattern_matches_known_operation ordering check"— confirmedCLI_CATEGORY_PATTERNS(module constant) is defined beforepattern_matches_known_operationandfind_inert_patterns, no forward-reference issue ✓
Open questions for the author
(none)
…lout + expert gotcha (#693) Document that doctor --json's inert_permission_patterns.details.inert_since is only present when a tool:-prefixed pattern is among the offenders, and add a keboola-expert.md §3 gotcha for permissions set pattern validation.
|
Both non-blocking findings addressed in 04c09d0:
|
…operations (#688) kbagent permissions set previously accepted any pattern string with zero validation, so a typo like tool.admin or stroage.upload-table silently persisted a dead rule. permissions set now rejects any --allow/--deny pattern that is not a cli:* category, an exact OPERATION_REGISTRY / FLAG_ESCALATIONS key, or a glob matching at least one known operation -- VALIDATION_ERROR, exit 2, every offending pattern listed, checked BEFORE the interactive confirmation so a bad call never reaches the prompt. find_inert_patterns() is generalized the same way: it now flags any pattern matching zero known operations, not only the retired tool: namespace, so permissions show and kbagent doctor surface any dead rule in an already-persisted policy (e.g. one written before this gate existed). PermissionEngine stays lenient at evaluation time -- only permissions set is strict. CLI_CATEGORY_PATTERNS is extracted so _matches_pattern and the new pattern_matches_known_operation() cannot drift apart, and a new UNMATCHED_PATTERN_HINT covers the generic (non-tool:) case alongside the existing tool:/MCP-migration hint.
…ces (#688) permissions set now validates every --allow/--deny pattern before the interactive confirmation (commit 8ee3f13), and find_inert_patterns() generalized detection to any dead pattern, not only tool:*. Sync the silent-drift doc surfaces (convention #17) that describe permissions set: - CLAUDE.md `## All CLI Commands` permissions block - plugins/kbagent/skills/kbagent/references/gotchas.md: new (since vNEXT) entry - plugins/kbagent/skills/kbagent/references/commands-reference.md: extend the permissions set / show bullets - src/keboola_agent_cli/commands/context.py (AGENT_CONTEXT): note the validation and generalized dead-pattern detection keboola-expert.md was checked but left untouched -- its permissions mentions (403 handling, "permission handling built into kbagent") do not describe permissions set's pattern acceptance, so nothing there is falsified by this change. Feature PR: tagged (since vNEXT), no version bump, no changelog entry.
Generalize the two missed drift surfaces (permissions-workflow.md, docs/guide.md) that still described tool:* as the only inert pattern -- `permissions set` now validates every --allow/--deny pattern against all four categories (cli:* category, exact operation, or glob), and find_inert_patterns() flags any dead pattern, not only tool:*. Enumerate the four cli:* categories in UNMATCHED_PATTERN_HINT so the message is actionable instead of vague. Add a validator/engine parity test (TestValidatorEngineParity) pinning pattern_matches_known_operation against _matches_pattern for a sample of accepted and rejected patterns, so a future change reintroducing the #688 drift between the two functions fails in tests instead of shipping. Note in the gotchas.md #688 entry that a pattern naming an operation that only exists on a newer kbagent version is rejected on an older build too -- it would be inert there -- so persist it after upgrading.
…lout + expert gotcha (#693) Document that doctor --json's inert_permission_patterns.details.inert_since is only present when a tool:-prefixed pattern is among the offenders, and add a keboola-expert.md §3 gotcha for permissions set pattern validation.
04c09d0 to
7317088
Compare
Closes #688.
Problem
kbagent permissions set --allow/--denyperformed zero validation on pattern strings:tool.admin,stroage.upload-tableor any other junk was persisted toconfig.jsonand echoed back as a normal rule, while matching nothing at runtime.find_inert_patterns(behind thepermissions show/doctorwarnings) only detected the one retiredtool:colon prefix, so a typo'd or fabricated pattern was never surfaced -- the operator walked away believing the firewall changed when it was functionally untouched.Fix
1.
permissions setvalidates every--allow/--denypattern (commands/permissions.py), before the interactive random-code confirmation. A pattern is valid when it is:cli:*categories (cli:read,cli:write,cli:destructive,cli:admin),OPERATION_REGISTRYkeys plusFLAG_ESCALATIONSstrings (auth.logout --remove-projectsis a real operation string passed tois_allowed()), orfnmatch.filterover the union).Anything else is rejected with
ErrorCode.VALIDATION_ERROR, exit 2, mirroring the existing--modevalidation directly above it. The message and JSONerror.details.invalid_patternslist every offending pattern (deduped), so a caller can fix the whole invocation in one round trip. Validation runs before the confirmation prompt -- no wasted interactive confirm, and the rejection is testable without a TTY.2.
find_inert_patternsgeneralized (permissions.py): any persisted pattern that is not acli:*category and matches zero known operations is flagged -- not only thetool:prefix.permissions showandkbagent doctorwarn accordingly; the MCP-migration hint appears only when atool:-prefixed pattern is actually present, other dead patterns get a generic check-for-typos hint. The newpattern_matches_known_operation()helper is shared by the validator and the inert detection, andCLI_CATEGORY_PATTERNSis hoisted so_matches_patternand the validator cannot drift.Runtime evaluation is unchanged:
PermissionEnginestays lenient, a pre-existing policy carrying dead patterns still loads and evaluates exactly as before (pinned by a regression test). Only thepermissions setwrite path gates.Docs
Drift surfaces updated with
(since vNEXT)tags per convention #17:CLAUDE.md,context.py(AGENT_CONTEXT),gotchas.md(including the version-skew case: a pattern naming an operation that only exists on a newer kbagent is rejected on the older build),commands-reference.md,permissions-workflow.md,docs/guide.md. No version bump, no changelog entry (feature PR per #648 process).Tests
pattern_matches_known_operationunit coverage (categories, exact keys, globs, flag-escalation keys, rejections incl.cli:*andtool:*).invalid_patternsdetails and the confirmation prompt is never reached; happy paths unchanged;permissions showwarns on a typo'd persisted pattern.detailskeys preserved.ruff/ty/make skill-check/make version-gate-check/check_command_sync.pyclean.JSON shape note (review NB-1)
doctor --json'sinert_permission_patternscheck now includesdetails.inert_sinceonly when atool:-prefixed pattern is among the offenders (it is meaningless for a plain typo). A typo-only policy omits the key, so external consumers must not assume it is always present onstatus: "warn". No in-repo consumer reads it unconditionally.