feat(0.22.0): project pin + firewall flags (FIIA P0-4) - #203
Conversation
Replacing with English version for broader team visibility.
padak
left a comment
There was a problem hiding this comment.
Approve — merge-ready
Security-wise correct (permission registry complete, the firewall merge is additive, the REPL fix correctly forwards flags, mutation safety is tested). Test coverage is ~40 new tests including E2E against a live API. No blockers.
Nice-to-have (follow-up PR)
1. Add a section to plugins/kbagent/skills/kbagent/references/permissions-workflow.md — "Session firewall flags" — a security topic deserves its own place in the references:
- WIDE (
--deny-writesblocks write+destructive+admin) vs NARROW (--deny-destructiveblocks only data destruction) - REPL forwarding behavior
- Relationship to persisted policy (additive merge, never relaxes)
2. Add to gotchas.md about the KBAGENT_PROJECT env var:
- Empty string = unset
- Points-to-unregistered alias → fail hard with
CONFIG_ERROR - Precedence:
--project>KBAGENT_PROJECT> pin > sole-project fallback
3. --hint for project use/current — if the hint framework can render ConfigStore mutations, add it; otherwise skip with a comment (local-only operation).
4. src/keboola_agent_cli/cli.py:90-144 _apply_firewall_flags — the underscore prefix + cross-module import from commands/permissions.py = leaky API boundary. Either rename to apply_firewall_flags, or move it to permissions.py as a public helper.
Positives
- WIDE vs NARROW semantics after iter-4 review are precisely specified in the help string and documentation.
resolve_pinned_alias()+resolve_project_alias()is a well-designed public API; future PRs can adopt it.test_flags_do_not_mutate_persistedexplicitly locks the invariant "session flags never subtract from the persisted policy".- REPL forwarding is non-trivial (duplicate guard) and is tested.
|
Bundled all four follow-ups on this branch (simpler than a separate follow-up PR):
Targeted tests: 30/30 pass. |
Adds an explicit project-pin UX + session-only firewall sugar that together close the FIIA P0-4 gap. New surface: - `kbagent project use <alias>` -- pins default_project in config.json. The field already existed (auto-set on first add, auto-rotated on remove) but had no explicit CLI verb. Now it does. - `kbagent project current` -- prints effective alias + source (env / pin / none). Reports both the env override AND the persisted pin so misconfigurations surface loudly instead of silently. - Top-level `--deny-writes` / `--deny-destructive` flags -- synthesize a session-only PermissionPolicy and merge with any persisted policy. Never written to config.json. `--deny-writes` covers the wide net (write+destructive+admin); `--deny-destructive` narrowly covers data destruction so callers can keep build-up capabilities while blocking tear-down. - `KBAGENT_PROJECT` env var overrides the persisted pin for a single shell. Resolution precedence for single-project ops: `--project` > `KBAGENT_PROJECT` > pin > sole-project > fail-hard with a repair-friendly CONFIG_ERROR. - `ProjectService.resolve_pinned_alias()` and the thin `commands._helpers.resolve_project_alias()` helper -- public contract for future single-project write commands to adopt without retrofitting every read-path fan-out. Wire notes: - `project.use` registered as 'write', `project.current` as 'read' in the permission engine's OPERATION_REGISTRY so persisted policies gate them correctly. - `_apply_firewall_flags()` builds a fresh PermissionPolicy and never mutates the persisted deny list. - Stale pin (default_project pointing at a deleted alias) now raises a repair-friendly CONFIG_ERROR instead of silently fanning out.
…mmands Review caught a silent-elevation bug: starting the CLI with `kbagent --deny-writes` and then entering the REPL (or typing `repl`) let inner commands re-parse WITHOUT the firewall flags, restoring full write/destructive access. The same PR5 firewall flag that a user opted into was silently dropped the moment they landed at the prompt. Root cause: `_run_repl()` only forwarded four flags (--json, --verbose, --no-color, --config-dir) when rebuilding argv for each inner command; the two new session-firewall flags were never threaded through. Fix: - Extend `_run_repl(deny_writes, deny_destructive)` and re-append both flags on every inner invocation unless the user already typed them. - Thread the flags from `cli.py::main()` into both REPL launch paths (implicit no-subcommand launch + explicit `kbagent repl`) via `ctx.obj["deny_writes"]` / `ctx.obj["deny_destructive"]`. - Banner advertises the active firewall so users can see what's on. Tests (`TestReplFirewallPropagation`): - `test_deny_writes_appended_to_inner_argv` -- guard against the regression by asserting every rebuilt argv carries --deny-writes. - `test_deny_destructive_appended_to_inner_argv` -- same for destructive. - `test_no_duplicate_when_user_retypes_flag` -- idempotence guard. Also closes other review-loop gaps (non-REPL): - `test_project_help` now asserts `use` and `current` subcommands. - `test_root_help_lists_firewall_flags` guards the global flags. - `test_list_includes_project_use_and_current` guards the registry. - `test_hint_on_non_api_project_use/current` cover --hint graceful exit. - `test_deny_writes_composes_with_persisted_deny_mode` validates the runtime composition under default-deny mode.
The original `test_root_help_lists_firewall_flags` rendered `kbagent --help` via CliRunner and string-matched `--deny-writes` / `--deny-destructive`. CI's narrow default terminal width made Rich's typer help panel truncate the long option names into `...`, failing the assertion even though the flags are registered correctly. Switch to inspecting `cli.main()` function signature. The signature is the source of truth for registered options and is independent of terminal width. Still catches the regression (accidental flag removal).
UX gap surfaced in the review loop: `permissions show` / `permissions list`
previously inspected ONLY the persisted policy. A user running
`kbagent --deny-writes permissions show` (or inside a REPL launched with
those flags) saw misleading output: "No permission policy configured"
even though their writes were in fact blocked for the invocation.
Fix:
- `permissions list` now builds the engine from the MERGED policy
(`_apply_firewall_flags(persisted, deny_writes=..., deny_destructive=...)`).
The allowed/denied column matches what the command will actually do.
Human-mode footer advertises active session flags.
- `permissions show` reports both layers in the JSON envelope:
{
"active": bool, # true if persisted OR session present
"persisted": {mode, allow, deny} | null,
"session_flags": [...],
# legacy top-level keys preserved when persisted != null
"mode": ..., "allow": ..., "deny": ...
}
Human-mode prints a "Session firewall:" line whenever flags are active.
Legacy JSON consumers that read `.mode` / `.allow` / `.deny` keep working.
- `permissions show` with no persisted policy + no session flags still
prints the legacy "No permission policy configured" line (empty-state
contract preserved for existing tests).
Tests added:
- `test_list_reflects_session_deny_writes` -- project.add flips to denied
when --deny-writes is active.
- `test_show_reports_session_flags_without_persisted_policy` --
active=true even without a persisted policy.
- `test_show_reports_session_flags_alongside_persisted` -- both layers
reported + legacy keys still present.
- `test_show_human_mode_no_policy_no_session` -- empty-state regression.
…y-writes Iter-4 review flagged a doc/flag-help ambiguity: the `--deny-destructive` help string said "destructive (delete/kill/reset) operations" which a user could reasonably read as covering 'project remove' and 'org setup'. The actual behavior (correctly) leaves admin-classified ops allowed, because 'admin' is a separate risk category from 'destructive' in the OPERATION registry. Only --deny-writes catches the wide net. Fixes: - `cli.py`: spell out WIDE vs NARROW in each flag's help string. Explicitly call out 'project remove' / 'org setup' as NOT blocked by --deny-destructive. - `context.py`: mirror the same WIDE vs NARROW contract in the AI-agent reference text (global flags section). Tests added (`TestFirewallFlags`): - `test_deny_destructive_allows_admin_ops` -- contract lock: running `kbagent --deny-destructive project remove prod` succeeds (exit != 6). - `test_deny_writes_blocks_admin_ops` -- complement: --deny-writes IS the wide net and DOES block admin ops (exit 6). Test hygiene (`TestPermissionsShow`): - `test_show_reports_session_flags_without_persisted_policy` now also asserts the legacy `mode`/`allow`/`deny` top-level keys are ABSENT when there is no persisted policy. Documents the new shape contract for downstream JSON consumers.
…orkflow.md Document --deny-writes (WIDE) vs --deny-destructive (NARROW) semantics, REPL forwarding behavior, and the additive-merge relationship with any persisted policy (session flags never relax what is persisted, never land in config.json). Cross-references the project-pin UX.
Record the three non-obvious rules: empty string = unset (Unix convention), pointing to an unregistered alias fails hard with CONFIG_ERROR (exit 5), and the full precedence chain --project > KBAGENT_PROJECT > persisted pin > sole-project fallback > fail.
Both commands operate purely on the local ConfigStore (no HTTP or service call to render), so registering HintRegistry entries would be meaningless. Add a single-line comment at each command so future contributors don't rediscover this by accident.
…_flags The function is imported from commands/permissions.py, which made the underscore prefix misleading (it looked private but wasn't). Drop the leading underscore at the definition, both call sites (cli.py, commands/permissions.py), and the test suite so the API boundary is honest.
77f402d to
6a84df6
Compare
- sync-workflow.md: new "Adopting an existing kbc Go CLI checkout" section for sync init --adopt-existing (#201). - gotchas.md: new entries for exit code 7 (JOB_TIMEOUT_TERMINATED), --poll-strategy fixed, --log-tail-lines N, logTail response semantics (#202); new section for --deny-writes / --deny-destructive (#203); new section for sync init --adopt-existing (#201). - workspace-workflow.md: new "Orphan detection + garbage collection" section for workspace list --orphaned + workspace gc (#204). - commands-reference.md: sync init --adopt-existing flag documented. - README.md: "What it does" table + "All commands" block updated with the 15 new commands shipped in 0.22.0 (flow CRUD, config metadata, workspace GC, storage describe, project pin + firewall, queue polling parity).
Summary
Closes FIIA P0-4 (project pin + firewall flags). Two orthogonal but complementary additions:
default_projectfield already lived onAppConfigand was auto-populated on firstproject add— but there was no CLI verb to change it, no env override, and no introspection. This PR addskbagent project use <alias>,kbagent project current, and aKBAGENT_PROJECTenv override, with a clean precedence rule for write-ops:--project>KBAGENT_PROJECT> pin > sole-project > fail-hard.--deny-writes/--deny-destructiveflags synthesize aPermissionPolicyfor the current invocation only and merge with any persisted policy. Never written toconfig.json— callers can safely run one-off blocked-mode commands without mutating their saved policy.Version bumped to 0.22.0.
Behavior
kbagent project use <alias>default_project = "<alias>"toconfig.json. Fails withCONFIG_ERROR/ exit 5 if the alias isn't registered. Registered aswritein the permission engine.kbagent project currentenv/pin/none). Reports BOTH the env override and the pinned value so misconfigurations are visible. Registered asread.KBAGENT_PROJECT=<alias>--project. Empty string treated as unset (Unix convention).--deny-writescli:write+tool:write(spans write+destructive+admin). Exit 6 on blocked op.--deny-destructivecli:destructive+tool:destructive(narrower — destructive only). Pure-write ops (e.g.storage create-bucket) stay allowed.CONFIG_ERROR("run 'kbagent project use ' to repair") instead of silently fanning out.Architecture
ProjectService.use_project(alias)/current_project()/resolve_pinned_alias()— new business logic in the service layer.commands._helpers.resolve_project_alias()— thin CLI adapter that calls the service and convertsConfigError→ exit 5. Public contract for future single-project write commands to adopt; not yet retrofitted into existing read paths (intentional — avoids UX regressions for callers that rely on implicit fan-out).cli._apply_firewall_flags()— pure function that builds a freshPermissionPolicyfrom the flags + any persisted policy. Merge-without-duplicates on the deny list; preserves persistedmodeandallow. Never mutates the persisted policy by reference.permissions.OPERATION_REGISTRYpicked up two new entries (project.use/project.current). The existing fail-closed "unknown op defaults towrite" behavior in_matches_pattern()means the flags already cover any new commands future PRs forget to register.Three-layer boundaries respected: no HTTP in services, no business logic in commands, no Typer imports in services.
Test plan
tests/test_services.py::TestUseAndCurrentProject— 15 tests coveringuse_project,current_project,resolve_pinned_alias(every precedence branch, including stale-pin repair).tests/test_helpers.py::TestResolveProjectAlias+TestApplyFirewallFlags— 12 tests covering the CLI adapter + the firewall-merge helper (fresh synthesis, merge-with-persisted-no-duplicates, persisted-not-mutated, both-flags-compose).tests/test_cli.py::TestProjectUse+TestFirewallFlags— 16 tests covering CLI wiring: JSON and human output, env override reporting, exit codes, persisted policy gatesproject use, read-classifiedproject currentbypassesdeny cli:write.connection.europe-west3.gcp.keboola.com), new focused classTestPinAndFirewallE2E— 5 tests, all pass:test_pin_lifecycle_against_real_project— add → current → use → verify persistence + env overridetest_deny_writes_blocks_real_write_op—storage create-bucketblocked (exit 6), defensive post-check via direct API confirms the bucket was NEVER createdtest_deny_destructive_blocks_real_destructive_op—storage delete-bucketblocked (exit 6)test_deny_destructive_allows_read_op—storage bucketssucceeds under--deny-destructivetest_firewall_flags_never_persist— post-runconfig.jsoninspection confirmspermissionsis stillNoneruff check+ruff format --checkclean.maintoo — not regressions).Live E2E tail
The comprehensive
TestFullE2E::test_full_cli_e2eflow was also exercised end-to-end against project 1143; it got 80% through before hitting a pre-existing Windows-onlydownload-tabletemp-filePermissionErrorunrelated to this PR. PR5's new step_test_project_pin_and_firewallis integrated into that flow (step 42.5) and runs the same assertions as the standalone class.Pre-PR review loop
Three Explore agents run in parallel on the final diff (wire-contract / silent-failure audit / coverage gaps). Consolidated findings:
Wire contract
cli:destructivepattern not broad-expanded. Rejected as intentional — the two flags exist precisely so--deny-destructivecan be narrower than--deny-writes(block destruction, allow builds). Confirmed bytest_deny_destructive_allows_write. Added an inline comment incli.py::_apply_firewall_flags()documenting the design.PermissionPolicyconstruction matchesmodels.py,ConfigStore.save()is atomic,OPERATION_REGISTRYentries correctly classified,remove_project()auto-rotation still works post-use_project.Silent-failure audit
KBAGENT_PROJECT=""silently treated as unset — intentional (Unix shell convention, keeps CI callers that conditionally export the var unsurprised). Documented in a code comment.Coverage gaps
project currentRich human-mode output — addedtest_project_current_human_mode_with_pin+test_project_current_human_mode_env_warns_unknown.project.usepermission gate untested — addedtest_project_use_blocked_by_persisted_deny_writes.resolve_pinned_aliasstale-pin branch untested — addedtest_resolve_pinned_alias_points_to_unregistered.