feat(0.30.7): project edit --new-alias + --dry-run (cascading rename) - #266
Conversation
5a05101 to
4a66ead
Compare
Rebase noteRebased onto upstream/main after PR #265 (closes #263) merged at `8dedfd1` and took the 0.30.3 slot. This PR now ships as 0.30.4:
State: `mergeStateStatus: CLEAN`, `mergeable: MERGEABLE`. Ready for human review whenever you have a moment, @padak. |
padak
left a comment
There was a problem hiding this comment.
Review summary
Well-executed PR addressing a real UX gap (rename without re-entering token, cascading config + nested-sync-dir rename in one atomic call). 3-layer separation is clean, security hardening from iter 2 (path-traversal validator, disk-rename rollback, Path.resolve(), depth-cap, symlink-target collision) is solid, and the silent-drift surfaces are all updated correctly:
gotchas.mdcarries the(since v0.30.4)tagkeboola-expert.mdhas both Rule 6 VERSION GATE and Tool Selection Matrix entriescommands-reference.md,commands/context.pyAGENT_CONTEXT, andCLAUDE.mdAll-CLI-Commands all match- Changelog is detailed and accurate
- 32 new tests (28 service + 4 CLI), including parametrized 9-input path-traversal surface, rollback (both happy + rollback-also-fails), and symlink collision
One blocker before merge, plus a NIT.
BLOCKING: E2E coverage missing
CONTRIBUTING.md "Tests (mandatory!)" / coding convention #16 in CLAUDE.md: Every CLI command must have E2E coverage. The existing _test_project_edit_and_remove() at tests/test_e2e.py:2836 only exercises --url; --new-alias is a new on-disk operation surface (filesystem rename + rollback contract) and warrants its own hop in the E2E flow.
Suggested minimal round-trip insertion at the top of _test_project_edit_and_remove (before the existing --url step):
# Rename round-trip -- exercise --new-alias against a real config dir
new_alias = f"{self.alias}-renamed"
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
# 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.aliasThen make test-e2e to confirm.
NIT (optional): consider --dry-run
CONTRIBUTING.md "UX considerations": Destructive operations have --dry-run and --yes flags. Rename isn't classically destructive (rollback exists, no remote data lost), but a dry-run would surface (a) collision detection, (b) the lineage-cache warning, (c) the planned method (git_mv vs shutil_move) without any mutation. Skip if you'd rather defer.
Explicitly NOT flagged
-
--hint client/--hint service-- intentionally skipped.project edit --new-aliasis a purely local operation (config.json + filesystem), no Keboola API counterpart. The wholeproject.editfamily is hint-less today (hints/definitions/project.pyonly registersdescription-get,description-set,info) and that's appropriate for non-API commands. Not a regression introduced by this PR. -
Permission split (
project.edit-credentialsadmin vsproject.edit-local-aliaswrite) -- you already flagged this as out-of-scope. Default-denyadminis the safer default; can ship as-is and revisit only if a user actually hits the policy friction. -
Cross-device partial-copy cleanup -- already documented in your iter-3 NIT log; non-destructive failure mode.
Other observations
- Rollback contract is well documented and tested (
test_oserror_in_disk_rename_restores_config,test_rollback_failure_is_swallowed_original_error_wins) -- exactly the right belt-and-suspenders. - Path-traversal validator's stricter-than-
project addrationale is correctly documented inline at_validate_alias_format. - Mirror pattern with
ConfigService._move_directory(returninggit_mv/shutil_movestrings) keeps JSON-consumer vocabulary consistent across rename surfaces.
Once E2E lands, this is good to go.
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 `<cwd>/<old-alias>/` -> `<cwd>/<new-alias>/` (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 keboola#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 keboola#266 review BLOCKING -- every CLI command must have E2E coverage per CONTRIBUTING.md / convention keboola#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 (keboola#268), 0.30.5 (keboola#272), and 0.30.6 (keboola#273) since the original PR opened. This PR ships as 0.30.7.
4a66ead to
ee5f77c
Compare
Iter 2 — addressed your review (BLOCKING + NIT both done)@padak Both review items landed in commit `ee5f77c`: BLOCKING — E2E coverage ✅Extended `tests/test_e2e.py::_test_project_edit_and_remove` with the rename round-trip you suggested PLUS a `--dry-run` preview hop, both before the existing `--url` step: ```python 1. dry-run pre-flightdata = self._run_ok("project", "edit", "--project", self.alias, 2. live rename + reverse-rename round-tripdata = self._run_ok("project", "edit", "--project", self.alias, rename back so subsequent steps keep using self.alias unchangeddata = self._run_ok("project", "edit", "--project", new_alias, Round-trip leaves `self.alias` unchanged so the existing `--url` step + `project remove` still work as before. NIT — `--dry-run` ✅Added `--dry-run` to `kbagent project edit` end-to-end:
Live verificationSmoke-tested against project 1143 in my testing workspace: ```json Config file unchanged after the dry-run. Test count: 32 → 38 (+6 new)`tests/test_project_edit.py` (32 service): added `TestRenameAliasDryRun` with happy-path, collision-still-raises, format-validation-still-raises, predicts-method-no-disk-change. `tests/test_project_edit_cli.py` (6 CLI): added human-DRY-RUN-label and JSON-planned-block tests. Rebase noteRebased onto upstream/main after you shipped #268 (0.30.4), #272 (0.30.5), and #273 (0.30.6) overnight. PR now ships as 0.30.7. No code conflicts on my files; only the `changelog.py` 0.30.4 slot needed to move — your sync-bug-fix entries from #268 stayed at 0.30.4 verbatim, mine moved up to a new 0.30.7 block. State
Ready for re-review whenever you have a moment, @padak. |
|
@padak — iter-2 addressed both review items (E2E round-trip + |
…276) Both PR #266 (feat: project edit --new-alias + --dry-run) and PR #275 (fix: per-element sql_resplit closes ODBC statement-count crash on #274) shipped as patch bumps (0.30.7 / 0.30.8) but never carried an external release. Consolidate them into a single minor release 0.31.0 -- a feature warrants the minor bump and the bug-fix rides along. No code changes; this is purely the version-label rename across the silent-drift sync surfaces: - pyproject.toml + plugin.json + marketplace.json + uv.lock -> 0.31.0 (via make version-sync) - changelog.py: 0.30.8 + 0.30.7 keys merged into one 0.31.0 block, features first (highlight ordering), then the SQL fix, then tests - gotchas.md: 5 references to (since v0.30.7) / (since v0.30.8) -> (since v0.31.0) - commands-reference.md: project edit gotcha pointer + config update auto-normalize version-gate annotation -> 0.31.0 - keboola-expert.md: 6 references in Rule 6 VERSION GATE, Tool Selection Matrix rows, and inline gotchas -> 0.31.0 make check (lint + format + skill freshness + version sync + changelog completeness + 2895 tests) clean. No regressions.
Summary
Adds
--new-alias NEWtokbagent project editso users can rename a project alias withoutproject remove+project add(which forces token re-entry). Cascading scope:config.jsonprojectsdict key +default_projectfield<cwd>/<old-alias>/-><cwd>/<new-alias>/(-2collision suffix,git mvwithshutil.movefallback -- mirrorskbagent config renameprecedent)*.lineage.json-- caches embed FQNs and are NOT auto-rewritten (partial rewrites are worse than no rewrite)Combined with
--url/--tokenin one call, those mutations target the new alias post-rename:kbagent project edit --project foo --new-alias bar --token NEWis one atomic operation.Closes the gap noted in user feedback after v0.29.0 -- previously the only way to rename was hand-editing `~/.config/keboola-agent-cli/config.json`.
Live validation results
Against project 1143 (`99_Playground_Max`, europe-west3.gcp.keboola.com):
Files changed
Security hardening
Iter-2 review surfaced two BLOCKING items that are now closed:
Plus from iter 2 NON-BLOCKING:
Test plan
Out-of-scope follow-ups
I'll file these as separate issues if the maintainer agrees they're worth tracking; flagging here so the deferred scope doesn't orphan: