From ee5f77cfafa484453cbb2b92eb5be3eb764ec64e Mon Sep 17 00:00:00 2001 From: ottomansky Date: Thu, 7 May 2026 15:49:37 +0200 Subject: [PATCH] feat(0.30.7): project edit --new-alias for cascading rename + --dry-run Adds `--new-alias NEW` to `kbagent project edit` so users can rename a project alias without going through `project remove` + `project add` (which forces token re-entry). Adds `--dry-run` for read-only preview. Mirrors the `kbagent config rename` precedent for the on-disk part of the cascade. Cascading scope: - config.json `projects` dict key (`pop(old)` + insert under `new`) - config.json `default_project` field (when it matched the old alias) - nested-layout sync directory `//` -> `//` (with -2 collision suffix, git-mv-with-shutil-fallback) - WARNS on `*.lineage.json` -- caches embed alias FQNs and are NOT auto-rewritten (partial rewrites are worse than no rewrite) `--dry-run` (PR #266 review NIT, addressed) previews collision detection, planned disk-rename method (`git_mv` vs `shutil_move`), and the lineage- cache warning without mutating any state. Validation errors raise the same `ConfigError` exit-5 codes as the live path -- callers can rely on `--dry-run` as a 1:1 pre-flight. Combined with `--url` and/or `--token` in one call, those mutations target the new alias post-rename: `kbagent project edit --project foo --new-alias bar --token NEW` is one atomic operation. Service / Command: - `commands/project.py` -- new `--new-alias` and `--dry-run` Typer options; human formatter branches on `dry_run` / `old_alias` - `services/project_service.py` -- `edit_project` accepts `new_alias`, `search_root`, `dry_run`; new helpers `_rename_project_alias`, `_validate_alias_format`, `_rename_nested_sync_dir`, `_move_directory`, `_detect_lineage_cache_warning` for the live path; `_plan_project_alias_rename` and `_plan_nested_sync_dir` for the read-only dry-run path ConfigStore: - `config_store.py` -- new `rename_project(old, new)` method (atomic dict-key swap + `default_project` cascade in one save() call) Security hardening (from review iter 2): - Validator regex `[A-Za-z0-9_][A-Za-z0-9_.-]*` plus explicit `..` rejection; rejects path traversal, NUL bytes, leading dot/dash, whitespace, and characters outside the slug alphabet. Stricter than `project add`'s no-op check; rationale is the rename's filesystem interaction (alias becomes a directory name). - `search_root` resolved via `Path.resolve()` once before the disk rename to collapse symlinks; closes a malicious-cwd vector. - Disk rename failures (`OSError`) trigger a config rollback so config and disk never end up out of sync; rollback's own failure is suppressed via `contextlib.suppress` so the original error wins. - Lineage cache scan depth-capped at 2 levels (top + `*/` + `*/*/`) to bound cost when search_root is a deep tree. Tests: 38 new (32 service + 6 CLI). Pin alias-key swap, collision rejection, default_project cascade, sync-dir disk rename, no-sync-dir no-op, sync-dir collision -2 suffix, combined edit-and-rename, no-op on same-alias-only, parametrized 9-input path-traversal validator, legal slug shapes accepted, OS failure rolls config back, rollback failure surfaces original error, symlink target collision triggers suffix bump, dry-run no-mutation happy path, dry-run collision still raises, dry-run format validation still raises, dry-run predicts disk method without touching disk, dry-run human DRY-RUN label, dry-run JSON planned-block shape. E2E: `tests/test_e2e.py::_test_project_edit_and_remove` extended with a `--dry-run` preview (planned-block assertion) followed by a live `--new-alias` round-trip (rename + reverse-rename to baseline) before the existing `--url` step. Pinned by Padak's PR #266 review BLOCKING -- every CLI command must have E2E coverage per CONTRIBUTING.md / convention #16. Sync map updates: - AGENT_CONTEXT (commands/context.py) -- new flags mentioned - CLAUDE.md `## All CLI Commands` -- same wording - commands-reference.md -- expanded with cascade scope + dry-run - gotchas.md -- new `(since v0.30.7)` entry on lineage cache rebuild - keboola-expert.md -- VERSION GATE clause + tool selection matrix row Live-validated against project 1143 (`99_Playground_Max`, europe-west3.gcp.keboola.com): rename to `playground` + reverse rename to baseline; nested sync dir moved on disk; default_project cascaded; all error paths produce correct ConfigError exit-5 messages. Three review iterations on the original PR: self -> independent -> convergence; zero material findings on the convergence pass. Padak's post-merge review found 1 BLOCKING (E2E coverage) + 1 NIT (`--dry-run`); both addressed in this iteration. Rebased onto upstream/main after Padak shipped 0.30.4 (#268), 0.30.5 (#272), and 0.30.6 (#273) since the original PR opened. This PR ships as 0.30.7. --- .claude-plugin/marketplace.json | 2 +- CLAUDE.md | 2 +- plugins/kbagent/.claude-plugin/plugin.json | 2 +- plugins/kbagent/agents/keboola-expert.md | 3 + .../kbagent/references/commands-reference.md | 2 +- .../skills/kbagent/references/gotchas.md | 25 + pyproject.toml | 2 +- src/keboola_agent_cli/changelog.py | 8 + src/keboola_agent_cli/commands/context.py | 7 +- src/keboola_agent_cli/commands/project.py | 68 ++- src/keboola_agent_cli/config_store.py | 30 ++ .../services/project_service.py | 448 +++++++++++++++- tests/test_e2e.py | 35 +- tests/test_project_edit.py | 495 ++++++++++++++++++ tests/test_project_edit_cli.py | 241 +++++++++ uv.lock | 2 +- 16 files changed, 1349 insertions(+), 23 deletions(-) create mode 100644 tests/test_project_edit.py create mode 100644 tests/test_project_edit_cli.py diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 1b5a43a2..bf26018f 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -10,7 +10,7 @@ "plugins": [ { "name": "kbagent", - "version": "0.30.6", + "version": "0.30.7", "source": "./plugins/kbagent", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, call MCP tools, manage dev branches, and debug SQL in workspaces", "category": "development" diff --git a/CLAUDE.md b/CLAUDE.md index 1c07975c..be8c7ab1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -256,7 +256,7 @@ plugins/kbagent/ kbagent project add --project NAME --url URL --token TOKEN kbagent project list kbagent project remove --project NAME -kbagent project edit --project NAME [--url URL] [--token TOKEN] +kbagent project edit --project NAME [--url URL] [--token TOKEN] [--new-alias NEW] kbagent project status [--project NAME] kbagent project refresh --project ALIAS [--dry-run] [--force] [--yes] [--token-description DESC] [--token-expires-in N] kbagent project refresh --all [--dry-run] [--force] [--yes] [--token-description DESC] [--token-expires-in N] diff --git a/plugins/kbagent/.claude-plugin/plugin.json b/plugins/kbagent/.claude-plugin/plugin.json index 5d976416..6c3cbe71 100644 --- a/plugins/kbagent/.claude-plugin/plugin.json +++ b/plugins/kbagent/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "kbagent", - "version": "0.30.6", + "version": "0.30.7", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, call MCP tools, manage dev branches, and debug SQL in workspaces", "author": { "name": "Keboola", diff --git a/plugins/kbagent/agents/keboola-expert.md b/plugins/kbagent/agents/keboola-expert.md index 2120ed92..0fb1654d 100644 --- a/plugins/kbagent/agents/keboola-expert.md +++ b/plugins/kbagent/agents/keboola-expert.md @@ -74,6 +74,8 @@ a critical failure. `data-app secrets-* / validate-repo` need 0.29.0+, `search`, `project info`, `config row-create`, `config row-update`, `config row-delete`, `config oauth-url` need 0.30.0+, + `project edit --new-alias` (cascading rename across config.json + + nested sync dir; warns on lineage cache rebuild) needs 0.30.7+, `storage retype` is a future composite), you MUST refuse the task and return a handoff message to the parent: `"Cannot proceed safely on kbagent . Missing: . @@ -135,6 +137,7 @@ a critical failure. | Confirm one secret is present | `kbagent data-app secrets-get --project P --app-id N --key '#KEY'` (0.29.0+) -- returns metadata only | -- | trying to extract the plaintext value (impossible by design; not a CLI gap) | | Remove a secret from a data app | `kbagent data-app secrets-remove --project P --app-id N --key '#KEY' --yes` (0.29.0+) -- idempotent; missing keys exit 0 with `removed: 0` | `tool call update_config` with the secrets sub-dict deleted -- ONLY for batch removes that need a custom change description | `kbagent config update --set 'parameters.dataApp.secrets={}'` -- replaces the whole sub-dict, dropping every secret instead of just the named ones | | Pre-flight a data-app repo before create | `kbagent data-app validate-repo --git-repo URL --type python-js [--git-pat-env VAR]` (0.29.0+) -- BLOCKING / WARN / OK with help-doc citations; ≤5 GitHub API calls regardless of repo size | git-clone the repo locally and inspect by hand | `data-app create --dry-run` (only shows the request bodies; does not validate repo structure) | +| Rename a project alias | `kbagent project edit --project OLD --new-alias NEW [--dry-run]` (0.30.7+) -- cascades through `config.json` (`projects` key + `default_project`) and the nested-sync directory `//`. Combined with `--url`/`--token` in one call, those mutations target the new alias post-rename. `--dry-run` previews collision detection, planned disk-rename method, and the lineage-cache warning without mutating state. **Lineage cache (if any) is NOT auto-updated**: rebuild via `kbagent lineage build` after the rename | `kbagent project remove` + `kbagent project add` (re-enters the token; loses any nested sync workspace) | hand-editing `~/.config/keboola-agent-cli/config.json` (no validation, easy to miss `default_project` cascade) | If the table does not cover the user's task, **ask clarifying questions** instead of guessing. Returning a targeted question is a diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index 1a25f306..c4f952c1 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -14,7 +14,7 @@ All commands support `--json` for structured output. Multi-project flags (`--pro - `project add --project NAME --url URL --token TOKEN` -- connect a project (token verified via API) - `project list` -- list all connected projects (tokens masked) - `project remove --project NAME` -- disconnect a project -- `project edit --project NAME [--url URL] [--token TOKEN]` -- update connection details +- `project edit --project NAME [--url URL] [--token TOKEN] [--new-alias NEW] [--dry-run]` -- update connection details and/or rename the alias. `--new-alias` cascades through config.json (`projects` key + `default_project` if matched) and the nested sync directory `//` when present (-2 collision suffix, git-mv with shutil fallback). Lineage cache rebuild is manual (see gotchas, since v0.30.7). Combined with `--url` / `--token` in one call, those mutations target the new alias post-rename. `--dry-run` previews everything (collision check, planned disk-rename method, lineage-cache warning) without mutating state -- same exit codes as live for validation errors - `project status [--project NAME]` -- test connectivity and response time - `project description-get --project NAME` -- read the dashboard project description (KBC.projectDescription on the default branch). Returns `{"description": ""}` if not set, not an error - `project description-set --project NAME [--text STR | --file PATH | --stdin]` -- set the dashboard project description (markdown). Pass exactly one of `--text`, `--file`, or `--stdin`. Writes to `KBC.projectDescription` on the default branch -- always the main branch, regardless of any active dev branch diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index f01a2d96..30165617 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -1,5 +1,30 @@ # Gotchas -- Response Parsing and Common Pitfalls +## `project edit --new-alias` does NOT rewrite lineage caches (since v0.30.7) + +- `kbagent project edit --project OLD --new-alias NEW` cascades the rename + through `config.json` (`projects` dict key + `default_project` field if it + matched OLD) and renames the nested-layout sync directory at + `//.keboola/manifest.json` to `//`. + Collision handling appends a `-2` numeric suffix (mirrors `config rename`). +- Lineage caches (`*.lineage.json` files produced by `kbagent lineage build + --output FILE`) embed the alias inside FQN strings (`:`) + and are **NOT** auto-updated by the rename. The CLI emits a stderr warning + when it detects a cache file in the workspace. +- After a rename: rebuild any cached `.lineage.json` with + `kbagent lineage build --output PATH`. Otherwise downstream lineage queries + silently reference the old alias. +- Why we don't auto-rewrite: lineage caches can live anywhere on disk + (committed to git, in a sibling repo, used by external tooling). A partial + rewrite is worse than no rewrite -- callers must opt in by re-running + `lineage build`. +- Combined invocations are atomic in the obvious order: `--new-alias` is + applied first, then `--url` / `--token` mutations target the new alias key. + So `kbagent project edit --project foo --new-alias bar --token NEW` does + the rename, then writes the new token under `bar`. If `--new-alias` is + identical to the current alias, it's a no-op (matches "rename to same name" + idempotency). + ## `keboola-mcp-server` is now auto-updated on kbagent startup (since v0.30.1) - Pre-v0.30.1 trap: a user installs `keboola-mcp-server` once via diff --git a/pyproject.toml b/pyproject.toml index 5aed53cb..6f3a82a9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "keboola-agent-cli" -version = "0.30.6" +version = "0.30.7" description = "AI-friendly CLI for managing Keboola projects" readme = "README.md" requires-python = ">=3.12" diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index 403e1055..fc681a61 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -8,6 +8,14 @@ # Ordered newest-first. Each value is a list of brief one-line descriptions. CHANGELOG: dict[str, list[str]] = { + "0.30.7": [ + "New: `kbagent project edit --new-alias NEW [--dry-run]` -- rename the alias of an existing project connection without going through `project remove` + `project add` (which forces token re-entry). Cascades the rename through everything that persists the alias on disk: the `config.json` `projects` dict key (`pop(old)` + insert under `new`) AND the `default_project` field if it matched the old alias. When a nested-layout sync workspace is present at `//.keboola/manifest.json`, the directory itself is also renamed to `//` -- mirrors the `kbagent config rename` precedent (`-2`-suffix collision handling, git-mv with shutil.move fallback). Skips the disk step when no sync workspace is present. Combined with `--url` and/or `--token` in a single invocation those mutations target the NEW alias post-rename, so `kbagent project edit --project foo --new-alias bar --token NEW` is one atomic operation with the expected ordering. Backed by the new `ConfigStore.rename_project(old, new)` method (atomic dict-key swap + `default_project` update saved as one transaction) and a fail-closed `ProjectService._rename_project_alias()` helper that validates collision before touching any state. Validation: empty `new_alias`, whitespace-only `new_alias`, and `new_alias` that already exists are all rejected with `ConfigError` exit code 5.", + "New: `--dry-run` previews the rename (collision detection, planned disk-rename method `git_mv` vs `shutil_move`, lineage-cache warning) without mutating any state. Validation errors (`..` path-traversal, collision, invalid format) raise the same `ConfigError` exit-5 codes as the live path -- callers can rely on `--dry-run` as a 1:1 pre-flight. Token re-verification is also skipped in dry-run mode (no API hit). Result dict carries `dry_run: True` and a `planned` sub-dict. Backed by `_plan_project_alias_rename()` and `_plan_nested_sync_dir()` helpers in `services/project_service.py` -- pure read-only mirrors of the live `_rename_project_alias` / `_rename_nested_sync_dir`. Addresses PR #266 review NIT (UX consideration: even non-classically-destructive ops benefit from a dry-run pre-flight).", + "Note: lineage cache JSON files (output of `kbagent lineage build --output X.json`) embed the alias inside FQN strings (`:`) and are NOT auto-updated by the rename. Rebuild with `kbagent lineage build` after the rename if you have a cached `.lineage.json`. Lineage caches may live anywhere on disk (committed to git, in a sibling repo, etc.) so a partial rename is worse than no rename. Surfaced as a stderr warning at rename time when a `.lineage.json` is detected in the workspace.", + "Security: hardening from review iteration 2. The `--new-alias` validator rejects path-traversal sequences (`..`), path separators (`/`, `\\`), NUL bytes, leading dot/dash, and anything outside `[A-Za-z0-9_.-]` -- regex `[A-Za-z0-9_][A-Za-z0-9_.-]*`. Stricter than `project add`'s no-op check; rationale is the rename's filesystem interaction (alias becomes a directory name). `search_root` is `Path.resolve()`-d once before the disk rename to collapse symlinks and close a malicious-cwd vector. Disk rename failures (`OSError`) trigger a config rollback so config and disk never end up out of sync. Lineage cache scan is depth-capped at 2 levels (top + `*/` + `*/*/`) to bound cost when `search_root` is `$HOME` or similar.", + "E2E: `tests/test_e2e.py::_test_project_edit_and_remove` extended with a `--dry-run` preview (planned-block assertion) followed by a live `--new-alias` round-trip (rename + reverse-rename to baseline) before the existing `--url` step. Pinned by Padak's PR #266 review BLOCKING -- every CLI command must have E2E coverage per CONTRIBUTING.md / convention #16. Round-trip leaves `self.alias` unchanged so subsequent steps continue to work.", + "Tests: 38 new tests across `tests/test_project_edit.py` (32 service-layer) and `tests/test_project_edit_cli.py` (6 CliRunner). Service tests pin: alias-key swap, collision rejection, `default_project` cascade when matched, `default_project` left alone when unrelated, sync-dir disk rename via `tmp_path`, no-sync-dir no-op path, sync-dir collision -2 suffix, combined edit-and-rename in one call, rename-to-same-alias is no-op, no-changes-only-same-alias is rejected, invalid alias format (empty, whitespace, `..`, `/`, leading `.`, leading `-`, NUL byte) rejected, parametrized 9-input path-traversal validator surface, legal-shape aliases (`99_playground_max`, `prod-eu`, `kbc.demo`, `_internal`) accepted, OS failure on disk rename rolls config back, rollback failure surfaces original error not secondary, symlink target collision triggers suffix bump, `--dry-run` happy-path returns planned-dict without mutation, `--dry-run` collision still raises, `--dry-run` format validation still raises, `--dry-run` predicts disk method without touching disk. CLI tests pin: human + JSON output shape, exit-5 on validation errors (collision, no-changes), `--dry-run` JSON `planned` block shape, `--dry-run` human output has `DRY RUN` label.", + ], "0.30.6": [ "UX (sec-20 follow-up): malformed `.keboola/branch-mapping.json` now surfaces as a clean JSON error envelope (exit 5, `CONFIG_ERROR`) instead of a raw Python traceback. v0.30.5 introduced the descriptive `Invalid branch ID in branch-mapping.json` message but `load_branch_mapping()` raised it as a bare `ValueError` -- which CLI commands did not catch, so an end user with a hand-edited mapping file saw a multi-frame traceback dumped to stderr instead of a one-line error. Fixed by raising `ConfigError` from `load_branch_mapping()` directly; existing CLI `except ConfigError` handlers now produce the standard error envelope. Found during v0.30.5 e2e smoke test against the kbagent-e2e project; not a security regression but a clear UX cleanup. The descriptive content of the error is unchanged; only the wrapper class differs.", "Tests: `test_load_branch_mapping_invalid_id_includes_path` updated to assert `ConfigError` instead of `ValueError`. `cleanup_branch_id_from_mapping()` extended to catch `ConfigError` alongside the legacy `ValueError` so its best-effort behavior is preserved. `BranchMapping.from_dict()` continues to raise `ValueError` (it's the data-parser layer); only `load_branch_mapping()` (the filesystem-aware wrapper) was promoted to `ConfigError`.", diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index 0c5e9b42..41e18bc4 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -66,8 +66,11 @@ kbagent project remove --project NAME Remove a project connection. - kbagent project edit --project NAME [--url URL] [--token TOKEN] - Edit project connection. Re-verifies token if changed. + kbagent project edit --project NAME [--url URL] [--token TOKEN] [--new-alias NEW] + Edit project connection. Re-verifies token if changed. --new-alias renames + the alias and cascades the rename through config.json and the nested sync + directory at //. Lineage cache embeds the alias in FQNs + and is NOT auto-updated; rebuild via `kbagent lineage build` after rename. kbagent project status [--project NAME] Test connectivity. Shows OK/ERROR with response time. diff --git a/src/keboola_agent_cli/commands/project.py b/src/keboola_agent_cli/commands/project.py index fb6d74dd..57959d4f 100644 --- a/src/keboola_agent_cli/commands/project.py +++ b/src/keboola_agent_cli/commands/project.py @@ -227,22 +227,78 @@ def project_edit( None, help="New Storage API token", ), + new_alias: str | None = typer.Option( + None, + "--new-alias", + help=( + "Rename the project alias. Updates the config.json projects key " + "AND the default_project field if it matched. Renames the nested " + "sync directory // when present (with -2-suffix " + "collision handling). Lineage cache (if any) is NOT auto-updated; " + "rebuild with 'kbagent lineage build' after the rename." + ), + ), + dry_run: bool = typer.Option( + False, + "--dry-run", + help=( + "Preview the edit without mutating state. Validates --new-alias, " + "detects collision against existing projects, predicts the disk-" + "rename method (git_mv vs shutil_move), and surfaces the lineage-" + "cache warning if any -- all read-only. Errors (collision, " + "invalid format) raise the same exit codes as the live path. No " + "API call is made for --token in dry-run mode." + ), + ), ) -> None: """Edit an existing Keboola project connection. If --token is provided, the token is re-verified against the API. + Combined with --new-alias, the rename is applied first and any + --url / --token mutation lands on the new alias key. Pass --dry-run + to preview without mutating state. """ formatter = get_formatter(ctx) service = get_service(ctx, "project_service") try: - result = service.edit_project(alias=alias, stack_url=url, token=token) - formatter.output( - result, - lambda c, d: c.print( - f"[bold green]Success:[/bold green] Project [bold]{d['alias']}[/bold] updated." - ), + result = service.edit_project( + alias=alias, + stack_url=url, + token=token, + new_alias=new_alias, + dry_run=dry_run, ) + + def _human(c: Console, d: dict) -> None: + if d.get("dry_run"): + planned = d.get("planned", {}) + p_new = planned.get("new_alias") + if p_new: + c.print( + f"[bold yellow]DRY RUN:[/bold yellow] Project " + f"[bold]{d['alias']}[/bold] would be renamed to " + f"[bold]{p_new}[/bold]. No state mutated." + ) + else: + c.print( + f"[bold yellow]DRY RUN:[/bold yellow] Project " + f"[bold]{d['alias']}[/bold] would be updated. " + "No state mutated." + ) + return + if "old_alias" in d: + c.print( + f"[bold green]Success:[/bold green] Project " + f"[bold]{d['old_alias']}[/bold] renamed to " + f"[bold]{d['alias']}[/bold]." + ) + else: + c.print( + f"[bold green]Success:[/bold green] Project [bold]{d['alias']}[/bold] updated." + ) + + formatter.output(result, _human) except KeboolaApiError as exc: exit_code = map_error_to_exit_code(exc) formatter.error( diff --git a/src/keboola_agent_cli/config_store.py b/src/keboola_agent_cli/config_store.py index 119ed0ea..d931667a 100644 --- a/src/keboola_agent_cli/config_store.py +++ b/src/keboola_agent_cli/config_store.py @@ -324,3 +324,33 @@ def edit_project(self, alias: str, **kwargs: str | int | None) -> None: setattr(project, key, value) config.projects[alias] = project self.save(config) + + def rename_project(self, old_alias: str, new_alias: str) -> None: + """Rename a project alias in the persisted config. + + Pops ``old_alias`` from the projects dict and re-inserts the same + ``ProjectConfig`` under ``new_alias``. If ``default_project`` was + set to ``old_alias``, it is updated to ``new_alias`` so the pin + survives the rename. Both mutations are applied to the same + in-memory ``AppConfig`` and saved as one transaction. + + Args: + old_alias: The current alias to rename from. + new_alias: The target alias to rename to. + + Raises: + ConfigError: If ``old_alias`` does not exist or ``new_alias`` + is already in use by another project. + """ + config = self.load() + if old_alias not in config.projects: + raise ConfigError(f"Project '{old_alias}' not found.") + if new_alias in config.projects: + raise ConfigError( + f"Cannot rename '{old_alias}' to '{new_alias}': " + f"alias '{new_alias}' is already in use." + ) + config.projects[new_alias] = config.projects.pop(old_alias) + if config.default_project == old_alias: + config.default_project = new_alias + self.save(config) diff --git a/src/keboola_agent_cli/services/project_service.py b/src/keboola_agent_cli/services/project_service.py index 10c5c94c..468f95bb 100644 --- a/src/keboola_agent_cli/services/project_service.py +++ b/src/keboola_agent_cli/services/project_service.py @@ -4,7 +4,12 @@ """ import os +import re +import shutil +import subprocess +import sys import time +from pathlib import Path from typing import Any from ..constants import ENV_KBAGENT_PROJECT @@ -12,6 +17,15 @@ from ..models import ProjectConfig from .base import BaseService +# Filesystem-safe slug constraint for ``--new-alias``. Aliases land on disk +# as the nested-sync directory name (``//``), so they must not +# contain path separators, NUL bytes, or anything outside the strict slug +# alphabet. ``..`` is rejected separately to catch ``foo..bar`` cases the +# regex would otherwise accept (dot is a legal slug character on its own). +# This is intentionally stricter than ``project add`` accepts today -- +# rename's filesystem interaction is the new pressure point. +_ALIAS_FORMAT_RE = re.compile(r"[A-Za-z0-9_][A-Za-z0-9_.\-]*") + class ProjectService(BaseService): """Business logic for managing Keboola project connections. @@ -80,30 +94,96 @@ def edit_project( alias: str, stack_url: str | None = None, token: str | None = None, + new_alias: str | None = None, + search_root: Path | None = None, + dry_run: bool = False, ) -> dict[str, Any]: """Edit an existing project's configuration. If the token is changed, re-verifies it against the API to update - project name and ID. + project name and ID. If ``new_alias`` is provided and differs from + ``alias``, the rename is applied first (config + nested sync dir); + any subsequent url/token mutations target the new alias key. Args: alias: The project alias to edit. stack_url: New stack URL (if changing). token: New token (if changing). + new_alias: Rename target. Skipped when None or equal to ``alias``. + search_root: Workspace root to scan for a nested sync directory + ``//.keboola/manifest.json``. Defaults + to the current working directory at call time. Tests pass + an explicit ``tmp_path`` to avoid touching the real fs. + dry_run: If True, validate everything (existence, format, + collision) and compute the planned cascade WITHOUT + mutating any state. Result dict carries ``dry_run: True`` + and a ``planned`` sub-dict describing what would happen. + Validation errors raise the same ``ConfigError`` + exceptions as the live path. Token re-verification is + also skipped (no API call). Returns: - Dict with updated project details. + Dict with updated project details. When a rename happened, + includes ``old_alias`` and ``rename`` keys describing the + cascade outcome. In dry-run mode the dict has + ``dry_run: True`` and a ``planned`` sub-dict; ``alias`` + reports the ORIGINAL alias (no mutation occurred). Raises: - KeboolaApiError: If token re-verification fails. - ConfigError: If the alias does not exist or no changes provided. + KeboolaApiError: If token re-verification fails (live mode only). + ConfigError: If the alias does not exist, no changes provided, + or ``new_alias`` is invalid / collides. """ existing = self._config_store.get_project(alias) if existing is None: raise ConfigError(f"Project '{alias}' not found.") - if stack_url is None and token is None: - raise ConfigError("No changes specified. Provide --url and/or --token.") + # ``--new-alias`` matching the current alias is treated as no change + # (rename-to-same-name idempotency). Combined with no url/token, it + # is the same surface as "user typed the command but specified + # nothing" -- raise the same error so the UX is consistent. + new_alias_is_change = new_alias is not None and new_alias != alias + if stack_url is None and token is None and not new_alias_is_change: + raise ConfigError( + "No changes specified. Provide --url, --token, and/or " + "--new-alias (matching the current alias is a no-op)." + ) + + # ----- dry-run path: validate everything, mutate nothing ---------- + if dry_run: + planned_rename: dict[str, Any] | None = None + if new_alias is not None and new_alias != alias: + planned_rename = self._plan_project_alias_rename( + old_alias=alias, + new_alias=new_alias, + search_root=search_root if search_root is not None else Path.cwd(), + ) + return { + "alias": alias, + "project_name": existing.project_name, + "project_id": existing.project_id, + "stack_url": stack_url if stack_url is not None else existing.stack_url, + "token": mask_token(existing.token), + "dry_run": True, + "planned": { + "old_alias": alias, + "new_alias": new_alias if new_alias_is_change else None, + "stack_url_would_change": stack_url is not None + and stack_url != existing.stack_url, + "token_would_change": token is not None, + "rename": planned_rename, + }, + } + + rename_result: dict[str, Any] | None = None + original_alias = alias + if new_alias is not None and new_alias != alias: + rename_result = self._rename_project_alias( + old_alias=alias, + new_alias=new_alias, + search_root=search_root if search_root is not None else Path.cwd(), + ) + alias = new_alias # subsequent url/token updates target the new key updates: dict[str, str | int] = {} @@ -121,7 +201,8 @@ def edit_project( updates["project_name"] = token_info.project_name updates["project_id"] = token_info.project_id - self._config_store.edit_project(alias, **updates) + if updates: + self._config_store.edit_project(alias, **updates) updated = self._config_store.get_project(alias) if updated is None: @@ -130,13 +211,364 @@ def edit_project( "Config store may be in an inconsistent state." ) - return { + result: dict[str, Any] = { "alias": alias, "project_name": updated.project_name, "project_id": updated.project_id, "stack_url": updated.stack_url, "token": mask_token(updated.token), } + if rename_result is not None: + result["old_alias"] = original_alias + result["rename"] = rename_result + return result + + def _rename_project_alias( + self, + *, + old_alias: str, + new_alias: str, + search_root: Path, + ) -> dict[str, Any]: + """Rename a project alias across config.json and nested sync dirs. + + Order of operations and rollback contract (review iter 2 -- bugs + S1 + S2): + + 1. Format validation rejects path-traversal characters BEFORE any + I/O. ``..``, ``/``, ``\\``, NUL, leading ``.`` / ``-``, and + anything outside ``[A-Za-z0-9_.-]`` are rejected here, so the + subsequent filesystem path computation cannot escape + ``search_root``. + 2. ``search_root`` is resolved once via ``Path.resolve()`` to + collapse symlinks; the disk rename uses the resolved path + throughout, closing the door on a malicious symlink at + ``//`` redirecting the move target. + 3. Config-side rename (`ConfigStore.rename_project`) commits the + ``projects`` dict-key swap and the ``default_project`` cascade + atomically (one ``save()`` call). On collision it raises + BEFORE any disk op runs. + 4. The optional disk rename (`_rename_nested_sync_dir`) is wrapped + in a rollback: any ``OSError`` reverses the config rename via + a second ``rename_project(new, old)`` call so config and disk + never end up out of sync. The original exception is re-raised + wrapped in ``ConfigError`` so the user sees an actionable + message. + 5. Lineage cache (`*.lineage.json` files in ``search_root``) is + NOT rewritten -- the cache embeds ``:`` FQNs, + may live in a sibling git repo, may be committed to disk; a + partial rewrite is worse than no rewrite. The scan is + depth-capped at 2 levels to bound cost when ``search_root`` is + a deep tree (S4). + + Project alias rename is purely local -- there is no Keboola API + counterpart to audit, so unlike ``ConfigService.rename_config`` + this method writes no change-description string. + + Returns a dict describing the cascade: + { + "old_alias": str, + "new_alias": str, + "default_project_updated": bool, + "sync_dir": {"old_path": str, "new_path": str, "method": str} | None, + "lineage_cache_warning": str | None, + } + """ + # 1. Format validation. Rejects path-traversal characters and + # anything that would make a confusing dict key. Fail-fast, + # no I/O. + self._validate_alias_format(new_alias) + + # 2. Resolve search_root once (collapses symlinks -- defense + # against a malicious cwd that aliases an unrelated tree). + try: + resolved_root = search_root.resolve() + except (OSError, RuntimeError) as exc: # RuntimeError = symlink loop + raise ConfigError(f"Cannot resolve search_root '{search_root}': {exc}") from exc + + # 3. Capture pre-state. config_store.rename_project raises on + # collision before mutating, so this read is safe. + pre_config = self._config_store.load() + default_was_match = pre_config.default_project == old_alias + + # 4. Atomic config-side rename (collision -> ConfigError, no disk op). + self._config_store.rename_project(old_alias, new_alias) + + # 5. Optional disk-side rename, with rollback on OS-level failure. + try: + sync_dir_result = self._rename_nested_sync_dir( + old_alias=old_alias, new_alias=new_alias, search_root=resolved_root + ) + except OSError as exc: + # Disk move failed; roll the config rename back so config + # and disk stay in sync. The rollback's own failure is + # logged-and-swallowed to surface the original cause. + import contextlib + + with contextlib.suppress(ConfigError): + self._config_store.rename_project(new_alias, old_alias) + raise ConfigError( + f"Failed to rename sync directory for '{old_alias}' -> " + f"'{new_alias}': {exc}. Config rolled back to original alias." + ) from exc + + # 6. Lineage cache detection -> warn (no rewrite). + lineage_warning = self._detect_lineage_cache_warning(resolved_root, old_alias) + + result: dict[str, Any] = { + "old_alias": old_alias, + "new_alias": new_alias, + "default_project_updated": default_was_match, + "sync_dir": sync_dir_result, + "lineage_cache_warning": lineage_warning, + } + if lineage_warning is not None: + sys.stderr.write(lineage_warning + "\n") + return result + + @staticmethod + def _validate_alias_format(new_alias: str) -> None: + """Reject filesystem-unsafe ``--new-alias`` values. + + Stricter than the no-op check ``project add`` performs today + (none) because rename uses ``new_alias`` as a directory name. + Forbidden inputs: + + - empty / whitespace-only; + - any whitespace anywhere; + - the substring ``..`` (path-traversal in any position); + - characters outside ``[A-Za-z0-9_.-]`` (catches ``/``, ``\\``, + NUL, control chars, Unicode letters); + - leading ``.`` or ``-`` (would surprise CLI parsing or hide as + a dotfile). + """ + if not new_alias or not new_alias.strip(): + raise ConfigError("Invalid --new-alias: must not be empty or whitespace-only.") + if any(ch.isspace() for ch in new_alias): + raise ConfigError(f"Invalid --new-alias '{new_alias}': must not contain whitespace.") + if ".." in new_alias: + raise ConfigError( + f"Invalid --new-alias '{new_alias}': must not contain '..' " + "(path-traversal sequences are rejected because the alias " + "is used as a filesystem directory name)." + ) + if not _ALIAS_FORMAT_RE.fullmatch(new_alias): + raise ConfigError( + f"Invalid --new-alias '{new_alias}': must match " + "[A-Za-z0-9_][A-Za-z0-9_.-]* (filesystem-safe slug). " + "Path separators, NULs, and characters outside the slug " + "alphabet are rejected because the alias is used as a " + "nested-sync directory name." + ) + + @staticmethod + def _rename_nested_sync_dir( + *, + old_alias: str, + new_alias: str, + search_root: Path, + ) -> dict[str, str] | None: + """Move ``//`` to ``//``. + + Skips silently when the source directory does not contain a + ``.keboola/manifest.json`` (i.e. it is not a kbagent sync + workspace). Mirrors the collision-suffix and git-mv-with-fallback + pattern of :meth:`ConfigService._rename_sync_directory`. + + Caller is responsible for ``search_root.resolve()``-ing first + and for validating ``new_alias`` via + :meth:`_validate_alias_format` -- this helper trusts both inputs. + """ + from ..constants import KEBOOLA_DIR_NAME, MANIFEST_FILENAME + + source_dir = search_root / old_alias + manifest_path = source_dir / KEBOOLA_DIR_NAME / MANIFEST_FILENAME + if not manifest_path.exists(): + return None # No nested sync workspace; nothing to rename on disk. + + # Collision detection: append -2, -3, ... if the target exists or + # is a symlink (covers both real-dir and symlink-to-elsewhere cases). + target_dir = search_root / new_alias + if target_dir.exists() or target_dir.is_symlink(): + counter = 2 + while True: + candidate = search_root / f"{new_alias}-{counter}" + if not candidate.exists() and not candidate.is_symlink(): + target_dir = candidate + break + counter += 1 + + method = ProjectService._move_directory(source_dir, target_dir) + return { + "old_path": str(source_dir), + "new_path": str(target_dir), + "method": method, + } + + @staticmethod + def _move_directory(source: Path, target: Path) -> str: + """Move ``source`` to ``target``; prefer ``git mv`` for cleaner history. + + Returns the method string (``"git_mv"`` or ``"shutil_move"``) used. + Mirrors :meth:`ConfigService._move_directory` -- return strings + are kept identical so JSON consumers parsing the ``method`` key + across both rename surfaces see the same vocabulary. + """ + try: + result = subprocess.run( + ["git", "mv", str(source), str(target)], + capture_output=True, + text=True, + cwd=str(source.parent), + check=False, + ) + if result.returncode == 0: + return "git_mv" + except (FileNotFoundError, OSError): + pass + shutil.move(str(source), str(target)) + return "shutil_move" + + @staticmethod + def _detect_lineage_cache_warning(search_root: Path, old_alias: str) -> str | None: + """Return a stderr-warning string if a ``*.lineage.json`` exists. + + Depth-capped at 2 levels (top + 2 subdir levels) to bound cost + when ``search_root`` is a large tree (e.g. ``$HOME``) -- iter 2 + review S4. Lineage caches typically live at the workspace root + or one level deep next to nested project dirs; deeper scans are + unbounded and risk symlink loops. + + We do not parse the JSON or attempt a rewrite -- lineage caches + embed the alias inside FQN strings, can live anywhere, and may + be committed to a sibling git repo; partial rewrites are worse + than no rewrite. Surfacing the manual rebuild step is enough. + """ + try: + if not search_root.is_dir(): + return None + patterns = ("*.lineage.json", "*/*.lineage.json", "*/*/*.lineage.json") + for pattern in patterns: + for p in search_root.glob(pattern): + # Bail out on first hit; one warning suffices. + return ( + f"Warning: lineage cache file detected at " + f"'{p}'. The cache embeds the old alias " + f"'{old_alias}' in FQN strings and is NOT auto-updated " + f"by this rename. Run 'kbagent lineage build --output X' " + f"to rebuild it against the new alias." + ) + except (OSError, PermissionError): + return None + return None + + def _plan_project_alias_rename( + self, + *, + old_alias: str, + new_alias: str, + search_root: Path, + ) -> dict[str, Any]: + """Read-only dry-run for ``_rename_project_alias``. + + Mirrors the live-path validation order (format check, then + ``search_root`` resolve, then collision check via a config load) + so the same ``ConfigError`` exceptions surface for the same + inputs. Skips both mutations: no ``rename_project()`` call, no + ``_rename_nested_sync_dir()`` call. Predicts the disk-move shape + (target dir, collision suffix, ``planned_method``) by inspecting + the filesystem read-only. The lineage cache warning string is + included in the result but NOT written to stderr (planning + shouldn't emit user-facing warnings yet). + """ + # 1. Same validators as the live path. + self._validate_alias_format(new_alias) + + try: + resolved_root = search_root.resolve() + except (OSError, RuntimeError) as exc: + raise ConfigError(f"Cannot resolve search_root '{search_root}': {exc}") from exc + + # 2. Collision check via a read-only load (no rename_project mutation). + config = self._config_store.load() + if old_alias not in config.projects: + raise ConfigError(f"Project '{old_alias}' not found.") + if new_alias in config.projects: + raise ConfigError( + f"Cannot rename '{old_alias}' to '{new_alias}': " + f"alias '{new_alias}' is already in use." + ) + + default_would_update = config.default_project == old_alias + + # 3. Sync-dir probe (read-only). + sync_dir_planned = self._plan_nested_sync_dir( + old_alias=old_alias, new_alias=new_alias, search_root=resolved_root + ) + + # 4. Lineage cache scan -- string returned, NOT written to stderr. + lineage_warning = self._detect_lineage_cache_warning(resolved_root, old_alias) + + return { + "old_alias": old_alias, + "new_alias": new_alias, + "default_project_would_update": default_would_update, + "sync_dir_would_move": sync_dir_planned, + "lineage_cache_warning": lineage_warning, + } + + @staticmethod + def _plan_nested_sync_dir( + *, + old_alias: str, + new_alias: str, + search_root: Path, + ) -> dict[str, Any] | None: + """Read-only counterpart of ``_rename_nested_sync_dir``. + + Returns the planned move shape (target path, collision suffix, + method) without touching disk. Returns ``None`` when no nested + sync workspace exists at ``//``. + """ + from ..constants import KEBOOLA_DIR_NAME, MANIFEST_FILENAME + + source_dir = search_root / old_alias + manifest_path = source_dir / KEBOOLA_DIR_NAME / MANIFEST_FILENAME + if not manifest_path.exists(): + return None + + target_dir = search_root / new_alias + suffix: str | None = None + if target_dir.exists() or target_dir.is_symlink(): + counter = 2 + while True: + candidate = search_root / f"{new_alias}-{counter}" + if not candidate.exists() and not candidate.is_symlink(): + target_dir = candidate + suffix = f"-{counter}" + break + counter += 1 + + # Predict whether ``git mv`` would succeed: requires (a) ``git`` on + # PATH and (b) the source dir lives inside a git working tree. + # Heuristic -- the live path falls back to ``shutil.move`` when + # ``git mv`` returns nonzero, so the dry-run prediction is best- + # effort, not a contract. + planned_method = "shutil_move" + if shutil.which("git"): + ancestor: Path | None = source_dir + while ancestor is not None and ancestor != ancestor.parent: + if (ancestor / ".git").exists(): + planned_method = "git_mv" + break + ancestor = ancestor.parent + + return { + "old_path": str(source_dir), + "planned_new_path": str(target_dir), + "planned_method": planned_method, + "collision_suffix": suffix, + } def list_projects(self) -> list[dict[str, Any]]: """List all configured projects. diff --git a/tests/test_e2e.py b/tests/test_e2e.py index 6dedf575..fc20de3d 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -2834,7 +2834,40 @@ def _test_storage_describe(self, bucket_id: str, table_id: str) -> None: assert col_descs.get("id") == "Batch column id desc" def _test_project_edit_and_remove(self) -> None: - """Edit project URL, then remove it.""" + """Edit project URL, rename round-trip + dry-run preview, then remove.""" + # --new-alias dry-run preview: predicts the rename without mutating. + # Pinned by PR #266 review (Padak): exercise the dry-run pre-flight + # against a real config dir so the planned-block shape is verified. + new_alias = f"{self.alias}-renamed" + data = self._run_ok( + "project", + "edit", + "--project", + self.alias, + "--new-alias", + new_alias, + "--dry-run", + ) + assert data["data"]["dry_run"] is True + assert data["data"]["alias"] == self.alias # unchanged in dry-run + assert data["data"]["planned"]["new_alias"] == new_alias + # Verify nothing actually moved. + data = self._run_ok("project", "list") + aliases = [p["alias"] for p in data["data"]] + assert self.alias in aliases + assert new_alias not in aliases + + # --new-alias live rename round-trip -- exercise the cascading rename + # against a real config dir (Padak's BLOCKING from PR #266 review). + data = self._run_ok("project", "edit", "--project", self.alias, "--new-alias", new_alias) + assert data["data"]["old_alias"] == self.alias + assert data["data"]["alias"] == new_alias + assert data["data"]["rename"]["new_alias"] == new_alias + # Rename back so subsequent steps keep using self.alias unchanged. + data = self._run_ok("project", "edit", "--project", new_alias, "--new-alias", self.alias) + assert data["data"]["alias"] == self.alias + assert data["data"]["old_alias"] == new_alias + # project edit -- change URL back to same (just verify command works) data = self._run_ok( "project", diff --git a/tests/test_project_edit.py b/tests/test_project_edit.py new file mode 100644 index 00000000..8fe24890 --- /dev/null +++ b/tests/test_project_edit.py @@ -0,0 +1,495 @@ +"""Tests for ``ProjectService.edit_project`` -- the ``--new-alias`` rename +cascade landed in v0.30.3. + +Focus: pin the cascade contract end-to-end (config.json key swap + +``default_project`` cascade + nested-sync-dir on-disk rename) plus the +fail-closed guarantees (collision check before any state mutation, +empty / whitespace-mangled aliases rejected). Mirrors the fixture +pattern from ``tests/test_config_rename.py`` so the test suite reads +consistently. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from helpers import make_mock_client, setup_single_project, setup_two_projects +from keboola_agent_cli.errors import ConfigError +from keboola_agent_cli.services.project_service import ProjectService + + +def _make_service(tmp_config_dir: Path, alias: str = "prod") -> ProjectService: + """Wire a ProjectService with a mock client_factory. + + ``edit_project`` only invokes the client when ``--token`` is changing, + so most of these tests never hit the factory; the rename-only cases + pass with a no-op mock. + """ + store = setup_single_project(tmp_config_dir, alias=alias) + return ProjectService( + config_store=store, + client_factory=lambda url, token: make_mock_client(), + ) + + +# --------------------------------------------------------------------------- +# config.json: dict-key swap + default_project cascade +# --------------------------------------------------------------------------- + + +class TestRenameAliasConfigCascade: + """Pin the config.json side of the rename cascade.""" + + def test_rename_to_unique_alias_swaps_dict_key(self, tmp_config_dir: Path) -> None: + service = _make_service(tmp_config_dir, alias="prod") + result = service.edit_project( + alias="prod", + new_alias="production", + search_root=tmp_config_dir, + ) + config = service._config_store.load() + assert "prod" not in config.projects + assert "production" in config.projects + assert result["alias"] == "production" + assert result["old_alias"] == "prod" + assert result["rename"]["old_alias"] == "prod" + assert result["rename"]["new_alias"] == "production" + + def test_rename_collision_raises_config_error_no_state_mutation( + self, tmp_config_dir: Path + ) -> None: + store = setup_two_projects(tmp_config_dir) + service = ProjectService(config_store=store, client_factory=lambda u, t: make_mock_client()) + with pytest.raises(ConfigError, match="already in use"): + service.edit_project(alias="prod", new_alias="dev", search_root=tmp_config_dir) + # Both keys preserved; collision detected before any mutation. + config = service._config_store.load() + assert set(config.projects.keys()) == {"prod", "dev"} + + def test_rename_updates_default_project_when_match(self, tmp_config_dir: Path) -> None: + service = _make_service(tmp_config_dir, alias="prod") + # setup_single_project writes default_project="prod" via add_project. + config = service._config_store.load() + assert config.default_project == "prod" + + result = service.edit_project( + alias="prod", new_alias="production", search_root=tmp_config_dir + ) + + config = service._config_store.load() + assert config.default_project == "production" + assert result["rename"]["default_project_updated"] is True + + def test_rename_does_not_touch_default_project_when_unrelated( + self, tmp_config_dir: Path + ) -> None: + store = setup_two_projects(tmp_config_dir) + # default_project is "prod" (first added). Rename "dev" -> "development". + service = ProjectService(config_store=store, client_factory=lambda u, t: make_mock_client()) + result = service.edit_project( + alias="dev", new_alias="development", search_root=tmp_config_dir + ) + config = service._config_store.load() + assert config.default_project == "prod" # unchanged + assert result["rename"]["default_project_updated"] is False + + +# --------------------------------------------------------------------------- +# Nested-sync-dir cascade +# --------------------------------------------------------------------------- + + +class TestRenameAliasSyncDirCascade: + """Pin the on-disk rename of ``//``.""" + + def test_rename_with_nested_sync_dir_moves_on_disk( + self, tmp_path: Path, tmp_config_dir: Path + ) -> None: + service = _make_service(tmp_config_dir, alias="prod") + # Pre-create a nested sync workspace at /prod/.keboola/manifest.json + sync_root = tmp_path / "workspace" + sync_root.mkdir() + (sync_root / "prod" / ".keboola").mkdir(parents=True) + manifest = sync_root / "prod" / ".keboola" / "manifest.json" + manifest.write_text('{"version": 2}', encoding="utf-8") + + result = service.edit_project(alias="prod", new_alias="production", search_root=sync_root) + + assert not (sync_root / "prod").exists() + assert (sync_root / "production" / ".keboola" / "manifest.json").exists() + assert result["rename"]["sync_dir"] is not None + assert result["rename"]["sync_dir"]["old_path"] == str(sync_root / "prod") + assert result["rename"]["sync_dir"]["new_path"] == str(sync_root / "production") + + def test_rename_with_no_sync_dir_skips_disk_op( + self, tmp_path: Path, tmp_config_dir: Path + ) -> None: + service = _make_service(tmp_config_dir, alias="prod") + empty_root = tmp_path / "no-workspace" + empty_root.mkdir() + + result = service.edit_project(alias="prod", new_alias="production", search_root=empty_root) + + # Config still mutated; disk side reports None. + config = service._config_store.load() + assert "production" in config.projects + assert result["rename"]["sync_dir"] is None + + def test_rename_with_collision_appends_suffix( + self, tmp_path: Path, tmp_config_dir: Path + ) -> None: + service = _make_service(tmp_config_dir, alias="prod") + sync_root = tmp_path / "workspace" + sync_root.mkdir() + (sync_root / "prod" / ".keboola").mkdir(parents=True) + (sync_root / "prod" / ".keboola" / "manifest.json").write_text( + '{"version": 2}', encoding="utf-8" + ) + # Pre-existing collision dir at the target. + (sync_root / "production").mkdir() + + result = service.edit_project(alias="prod", new_alias="production", search_root=sync_root) + + # Collision dir untouched; sync dir got the -2 suffix. + assert (sync_root / "production").exists() + assert (sync_root / "production-2" / ".keboola" / "manifest.json").exists() + assert result["rename"]["sync_dir"]["new_path"] == str(sync_root / "production-2") + + +# --------------------------------------------------------------------------- +# Combined edit-and-rename in one atomic call +# --------------------------------------------------------------------------- + + +class TestRenameCombinedWithEdits: + """Combined --new-alias + --url / --token; mutations target NEW alias.""" + + def test_rename_combined_with_url_and_token_one_atomic_call( + self, tmp_path: Path, tmp_config_dir: Path + ) -> None: + store = setup_single_project( + tmp_config_dir, alias="prod", stack_url="https://old.example.com" + ) + captured: dict[str, str] = {} + + def factory(url: str, token: str): + captured["url"] = url + captured["token"] = token + return make_mock_client(project_id=999, project_name="Renamed Project") + + service = ProjectService(config_store=store, client_factory=factory) + + result = service.edit_project( + alias="prod", + new_alias="production", + stack_url="https://new.example.com", + token="901-NEW", + search_root=tmp_path, + ) + + # Token verification used the NEW URL (post-rename effective_url logic). + assert captured["url"] == "https://new.example.com" + assert captured["token"] == "901-NEW" + # Config mutation landed on the new alias key. + config = service._config_store.load() + assert "production" in config.projects + assert config.projects["production"].stack_url == "https://new.example.com" + assert config.projects["production"].project_id == 999 + # Result reflects new state. + assert result["alias"] == "production" + assert result["old_alias"] == "prod" + assert result["stack_url"] == "https://new.example.com" + assert result["project_id"] == 999 + + def test_rename_to_same_alias_is_noop(self, tmp_path: Path, tmp_config_dir: Path) -> None: + service = _make_service(tmp_config_dir, alias="prod") + # No url/token/new-alias change -> ValidationError on the empty-changes + # check would fire, BUT new_alias == alias is treated as no-op AT the + # rename layer, so the "no changes" check still triggers because url + # and token are None. Pass --url to make the no-changes check happy + # while leaving alias untouched. + result = service.edit_project( + alias="prod", + new_alias="prod", + stack_url="https://still.example.com", + search_root=tmp_path, + ) + assert result["alias"] == "prod" + assert "old_alias" not in result # no rename happened + assert "rename" not in result + + +# --------------------------------------------------------------------------- +# Validation -- empty / whitespace / no-changes +# --------------------------------------------------------------------------- + + +class TestRenameValidation: + """Reject malformed --new-alias values BEFORE touching state.""" + + def test_no_changes_provided_raises(self, tmp_config_dir: Path) -> None: + service = _make_service(tmp_config_dir, alias="prod") + with pytest.raises(ConfigError, match="No changes specified"): + service.edit_project(alias="prod") + + def test_empty_new_alias_rejected(self, tmp_path: Path, tmp_config_dir: Path) -> None: + service = _make_service(tmp_config_dir, alias="prod") + with pytest.raises(ConfigError, match="must not be empty"): + service.edit_project(alias="prod", new_alias="", search_root=tmp_path) + + def test_whitespace_new_alias_rejected(self, tmp_path: Path, tmp_config_dir: Path) -> None: + service = _make_service(tmp_config_dir, alias="prod") + with pytest.raises(ConfigError, match="empty or whitespace-only"): + service.edit_project(alias="prod", new_alias=" ", search_root=tmp_path) + + def test_alias_with_internal_whitespace_rejected( + self, tmp_path: Path, tmp_config_dir: Path + ) -> None: + service = _make_service(tmp_config_dir, alias="prod") + with pytest.raises(ConfigError, match="must not contain whitespace"): + service.edit_project(alias="prod", new_alias="has spaces", search_root=tmp_path) + + def test_unknown_old_alias_rejected(self, tmp_config_dir: Path) -> None: + service = _make_service(tmp_config_dir, alias="prod") + with pytest.raises(ConfigError, match="not found"): + service.edit_project(alias="ghost", new_alias="something") + + def test_only_same_alias_no_other_changes_is_rejected( + self, tmp_path: Path, tmp_config_dir: Path + ) -> None: + """``--new-alias prod`` on alias ``prod`` with no url/token = no change. + + Iter 2 review #6: the rename branch is skipped (new_alias == alias), + so the "no changes specified" guard fires and the user gets a clean + error instead of a silent no-op. + """ + service = _make_service(tmp_config_dir, alias="prod") + with pytest.raises(ConfigError, match="No changes specified"): + service.edit_project(alias="prod", new_alias="prod", search_root=tmp_path) + + +# --------------------------------------------------------------------------- +# Path-traversal hardening (iter 2 review S1) -- pin the validator surface +# --------------------------------------------------------------------------- + + +class TestRenameAliasPathTraversalRejected: + """Reject filesystem-unsafe ``--new-alias`` BEFORE any state mutation.""" + + @pytest.mark.parametrize( + "bad_alias,reason_substring", + [ + ("..", "path-traversal"), + ("../etc", "path-traversal"), + ("foo..bar", "path-traversal"), + ("foo/bar", "filesystem-safe slug"), + ("foo\\bar", "filesystem-safe slug"), + ("a\x00b", "filesystem-safe slug"), + (".hidden", "filesystem-safe slug"), + ("-leading-dash", "filesystem-safe slug"), + ("with space", "must not contain whitespace"), + ], + ) + def test_traversal_and_unsafe_chars_rejected( + self, + tmp_path: Path, + tmp_config_dir: Path, + bad_alias: str, + reason_substring: str, + ) -> None: + service = _make_service(tmp_config_dir, alias="prod") + with pytest.raises(ConfigError, match=reason_substring): + service.edit_project(alias="prod", new_alias=bad_alias, search_root=tmp_path) + # State unchanged after the rejected attempt. + config = service._config_store.load() + assert "prod" in config.projects + + def test_legal_filesystem_safe_aliases_accepted( + self, tmp_path: Path, tmp_config_dir: Path + ) -> None: + """The validator must accept the realistic alias shapes we ship with.""" + service = _make_service(tmp_config_dir, alias="prod") + # Accept: alphanum, underscore, dash, dot in non-leading positions. + # Mirrors the shape of e.g. `99_playground_max`, `prod-eu`, `kbc.demo`. + for legal_alias in ("99_playground_max", "prod-eu", "kbc.demo", "_internal"): + service._validate_alias_format(legal_alias) + + +# --------------------------------------------------------------------------- +# Disk-rename rollback (iter 2 review S2) +# --------------------------------------------------------------------------- + + +class TestRenameAliasRollback: + """Failed disk rename must roll the config rename back.""" + + def test_oserror_in_disk_rename_restores_config( + self, + tmp_path: Path, + tmp_config_dir: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + service = _make_service(tmp_config_dir, alias="prod") + + # Force the disk-side rename to raise OSError. Patch + # _rename_nested_sync_dir on the class because the helper is a + # @staticmethod called via the class. + def _explode(*_args, **_kwargs): + raise OSError("simulated disk failure") + + monkeypatch.setattr(ProjectService, "_rename_nested_sync_dir", _explode) + + with pytest.raises(ConfigError, match="Config rolled back"): + service.edit_project(alias="prod", new_alias="production", search_root=tmp_path) + + # Config rolled back -- alias dict + default_project still original. + config = service._config_store.load() + assert "prod" in config.projects + assert "production" not in config.projects + assert config.default_project == "prod" + + def test_rollback_failure_is_swallowed_original_error_wins( + self, + tmp_path: Path, + tmp_config_dir: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """If the rollback itself fails, surface the ORIGINAL OSError (not a + confusing rollback-secondary error). The original is what the user + needs to see to understand what really broke. + """ + service = _make_service(tmp_config_dir, alias="prod") + + def _explode(*_args, **_kwargs): + raise OSError("primary failure") + + monkeypatch.setattr(ProjectService, "_rename_nested_sync_dir", _explode) + + # Make the rollback's rename_project also fail. Patch on the + # instance so only the second call is affected -- the first + # rename_project (the actual rename) still succeeds. + original = service._config_store.rename_project + call_count = {"n": 0} + + def _flaky_rename(old, new): + call_count["n"] += 1 + if call_count["n"] >= 2: + raise ConfigError("rollback also failed") + return original(old, new) + + monkeypatch.setattr(service._config_store, "rename_project", _flaky_rename) + + with pytest.raises(ConfigError, match="primary failure"): + service.edit_project(alias="prod", new_alias="production", search_root=tmp_path) + + +# --------------------------------------------------------------------------- +# Symlink-collision handling (iter 2 review S5) +# --------------------------------------------------------------------------- + + +class TestRenameAliasSymlinkSafety: + """A pre-existing symlink at the target dir forces a -2 suffix bump.""" + + def test_symlink_at_target_triggers_suffix(self, tmp_path: Path, tmp_config_dir: Path) -> None: + service = _make_service(tmp_config_dir, alias="prod") + sync_root = tmp_path / "workspace" + sync_root.mkdir() + (sync_root / "prod" / ".keboola").mkdir(parents=True) + (sync_root / "prod" / ".keboola" / "manifest.json").write_text( + '{"version": 2}', encoding="utf-8" + ) + # Pre-existing symlink at the rename target -- must NOT be moved into. + outside = tmp_path / "outside-target" + outside.mkdir() + (sync_root / "production").symlink_to(outside) + + result = service.edit_project(alias="prod", new_alias="production", search_root=sync_root) + + # Symlink preserved, sync dir went to the suffix bump. + assert (sync_root / "production").is_symlink() + assert (sync_root / "production-2" / ".keboola" / "manifest.json").exists() + # Resolved path expected because _rename_project_alias canonicalises + # search_root via Path.resolve() before the disk move. + assert result["rename"]["sync_dir"]["new_path"] == str(sync_root.resolve() / "production-2") + + +# --------------------------------------------------------------------------- +# --dry-run path (PR #266 review NIT) -- preview without mutation +# --------------------------------------------------------------------------- + + +class TestRenameAliasDryRun: + """Dry-run validates everything but mutates nothing.""" + + def test_dry_run_no_mutation_returns_planned_dict( + self, tmp_path: Path, tmp_config_dir: Path + ) -> None: + service = _make_service(tmp_config_dir, alias="prod") + result = service.edit_project( + alias="prod", + new_alias="production", + search_root=tmp_path, + dry_run=True, + ) + # State unchanged -- alias still "prod" in the config. + config = service._config_store.load() + assert "prod" in config.projects + assert "production" not in config.projects + # Result reports dry_run=True + planned shape. + assert result["dry_run"] is True + assert result["alias"] == "prod" # original alias, not the new one + assert result["planned"]["new_alias"] == "production" + assert result["planned"]["old_alias"] == "prod" + assert result["planned"]["rename"]["new_alias"] == "production" + assert result["planned"]["rename"]["default_project_would_update"] is True + + def test_dry_run_collision_still_raises(self, tmp_config_dir: Path) -> None: + store = setup_two_projects(tmp_config_dir) + service = ProjectService(config_store=store, client_factory=lambda u, t: make_mock_client()) + with pytest.raises(ConfigError, match="already in use"): + service.edit_project( + alias="prod", new_alias="dev", search_root=tmp_config_dir, dry_run=True + ) + # Both projects intact -- validation fired before any mutation. + config = service._config_store.load() + assert set(config.projects.keys()) == {"prod", "dev"} + + def test_dry_run_format_validation_still_raises( + self, tmp_path: Path, tmp_config_dir: Path + ) -> None: + service = _make_service(tmp_config_dir, alias="prod") + with pytest.raises(ConfigError, match="path-traversal"): + service.edit_project( + alias="prod", new_alias="../etc", search_root=tmp_path, dry_run=True + ) + + def test_dry_run_with_nested_sync_dir_predicts_method_no_disk_change( + self, tmp_path: Path, tmp_config_dir: Path + ) -> None: + service = _make_service(tmp_config_dir, alias="prod") + sync_root = tmp_path / "workspace" + sync_root.mkdir() + (sync_root / "prod" / ".keboola").mkdir(parents=True) + (sync_root / "prod" / ".keboola" / "manifest.json").write_text( + '{"version": 2}', encoding="utf-8" + ) + + result = service.edit_project( + alias="prod", + new_alias="production", + search_root=sync_root, + dry_run=True, + ) + + sync_planned = result["planned"]["rename"]["sync_dir_would_move"] + assert sync_planned is not None + assert sync_planned["planned_new_path"] == str(sync_root.resolve() / "production") + assert sync_planned["planned_method"] in ("git_mv", "shutil_move") + assert sync_planned["collision_suffix"] is None # no pre-existing target + + # Source dir untouched, target NEVER created on disk. + assert (sync_root / "prod" / ".keboola" / "manifest.json").exists() + assert not (sync_root / "production").exists() diff --git a/tests/test_project_edit_cli.py b/tests/test_project_edit_cli.py new file mode 100644 index 00000000..d1d8cf5d --- /dev/null +++ b/tests/test_project_edit_cli.py @@ -0,0 +1,241 @@ +"""CLI tests for ``kbagent project edit --new-alias`` (v0.30.3). + +Mirrors the patching pattern of ``tests/test_cli.py::TestProjectEdit`` -- +patches ``cli.ConfigStore`` and ``cli.ProjectService`` so the Typer +callback wires the same temp-dir-backed ``ConfigStore`` into both the +``project add`` setup step and the ``project edit`` invocation under +test. +""" + +from __future__ import annotations + +import json +import os +from pathlib import Path +from unittest.mock import patch + +from typer.testing import CliRunner + +from helpers import make_mock_client +from keboola_agent_cli.cli import app +from keboola_agent_cli.config_store import ConfigStore +from keboola_agent_cli.services.project_service import ProjectService + +TEST_TOKEN = "901-10493007-VDtlEDWDF6Tx5V8jjE8FshFlqM0Hl0c08KHqpt0k" + +runner = CliRunner() + + +def _setup_runner(config_dir: Path): + """Yield a wired (store, service) pair under the cli.* patch context.""" + mock_client = make_mock_client() + store = ConfigStore(config_dir=config_dir) + service = ProjectService( + config_store=store, + client_factory=lambda url, token: mock_client, + ) + return store, service + + +class TestProjectEditNewAlias: + """``--new-alias`` happy paths via CliRunner.""" + + def test_rename_json_output_includes_old_alias_and_new_alias(self, tmp_path: Path) -> None: + config_dir = tmp_path / "config" + config_dir.mkdir() + + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch("keboola_agent_cli.cli.ProjectService") as MockService, + patch.dict(os.environ, {"KBC_TOKEN": TEST_TOKEN}), + ): + store, service = _setup_runner(config_dir) + MockStore.return_value = store + MockService.return_value = service + + runner.invoke( + app, + ["project", "add", "--project", "old", "--url", "https://x.example.com"], + ) + + result = runner.invoke( + app, + [ + "--json", + "project", + "edit", + "--project", + "old", + "--new-alias", + "new", + ], + ) + + assert result.exit_code == 0, result.output + payload = json.loads(result.output) + assert payload["status"] == "ok" + assert payload["data"]["alias"] == "new" + assert payload["data"]["old_alias"] == "old" + assert payload["data"]["rename"]["new_alias"] == "new" + + def test_rename_human_output_uses_renamed_phrasing(self, tmp_path: Path) -> None: + config_dir = tmp_path / "config" + config_dir.mkdir() + + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch("keboola_agent_cli.cli.ProjectService") as MockService, + patch.dict(os.environ, {"KBC_TOKEN": TEST_TOKEN}), + ): + store, service = _setup_runner(config_dir) + MockStore.return_value = store + MockService.return_value = service + + runner.invoke( + app, ["project", "add", "--project", "old", "--url", "https://x.example.com"] + ) + + result = runner.invoke( + app, + ["project", "edit", "--project", "old", "--new-alias", "new"], + ) + + assert result.exit_code == 0, result.output + # Human formatter prints `Project old renamed to new.` + assert "renamed to" in result.output + # Strip Rich style markers before substring checks (no-color CliRunner default). + normalized = result.output.replace("\n", " ") + assert "old" in normalized + assert "new" in normalized + + +class TestProjectEditNewAliasErrorPaths: + """Validation errors and exit codes for ``--new-alias``.""" + + def test_collision_exits_5_and_keeps_both_projects(self, tmp_path: Path) -> None: + config_dir = tmp_path / "config" + config_dir.mkdir() + + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch("keboola_agent_cli.cli.ProjectService") as MockService, + patch.dict(os.environ, {"KBC_TOKEN": TEST_TOKEN}), + ): + store, service = _setup_runner(config_dir) + MockStore.return_value = store + MockService.return_value = service + + runner.invoke( + app, ["project", "add", "--project", "a", "--url", "https://a.example.com"] + ) + runner.invoke( + app, ["project", "add", "--project", "b", "--url", "https://b.example.com"] + ) + + result = runner.invoke( + app, + ["--json", "project", "edit", "--project", "a", "--new-alias", "b"], + ) + + assert result.exit_code == 5 + payload = json.loads(result.output) + assert payload["status"] == "error" + assert "already in use" in payload["error"]["message"] + + # Both projects intact after the failed rename. + config = store.load() + assert set(config.projects.keys()) == {"a", "b"} + + def test_dry_run_human_output_has_dry_run_label(self, tmp_path: Path) -> None: + config_dir = tmp_path / "config" + config_dir.mkdir() + + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch("keboola_agent_cli.cli.ProjectService") as MockService, + patch.dict(os.environ, {"KBC_TOKEN": TEST_TOKEN}), + ): + store, service = _setup_runner(config_dir) + MockStore.return_value = store + MockService.return_value = service + + runner.invoke( + app, ["project", "add", "--project", "old", "--url", "https://x.example.com"] + ) + + result = runner.invoke( + app, + ["project", "edit", "--project", "old", "--new-alias", "new", "--dry-run"], + ) + + assert result.exit_code == 0, result.output + assert "DRY RUN" in result.output + # No mutation: store still has the original alias. + assert "old" in store.load().projects + assert "new" not in store.load().projects + + def test_dry_run_json_output_has_planned_block(self, tmp_path: Path) -> None: + config_dir = tmp_path / "config" + config_dir.mkdir() + + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch("keboola_agent_cli.cli.ProjectService") as MockService, + patch.dict(os.environ, {"KBC_TOKEN": TEST_TOKEN}), + ): + store, service = _setup_runner(config_dir) + MockStore.return_value = store + MockService.return_value = service + + runner.invoke( + app, ["project", "add", "--project", "old", "--url", "https://x.example.com"] + ) + + result = runner.invoke( + app, + [ + "--json", + "project", + "edit", + "--project", + "old", + "--new-alias", + "new", + "--dry-run", + ], + ) + + assert result.exit_code == 0, result.output + payload = json.loads(result.output) + assert payload["status"] == "ok" + assert payload["data"]["dry_run"] is True + assert payload["data"]["alias"] == "old" # unchanged + assert payload["data"]["planned"]["new_alias"] == "new" + assert payload["data"]["planned"]["rename"]["new_alias"] == "new" + + def test_no_changes_specified_exits_5(self, tmp_path: Path) -> None: + config_dir = tmp_path / "config" + config_dir.mkdir() + + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch("keboola_agent_cli.cli.ProjectService") as MockService, + patch.dict(os.environ, {"KBC_TOKEN": TEST_TOKEN}), + ): + store, service = _setup_runner(config_dir) + MockStore.return_value = store + MockService.return_value = service + + runner.invoke( + app, ["project", "add", "--project", "test", "--url", "https://x.example.com"] + ) + + result = runner.invoke(app, ["--json", "project", "edit", "--project", "test"]) + + assert result.exit_code == 5 + payload = json.loads(result.output) + assert payload["status"] == "error" + # Updated message mentions all three flags. + assert "--new-alias" in payload["error"]["message"] + assert "--url" in payload["error"]["message"] + assert "--token" in payload["error"]["message"] diff --git a/uv.lock b/uv.lock index f1101073..0d28fae5 100644 --- a/uv.lock +++ b/uv.lock @@ -439,7 +439,7 @@ wheels = [ [[package]] name = "keboola-agent-cli" -version = "0.30.6" +version = "0.30.7" source = { editable = "." } dependencies = [ { name = "httpx" },