Phase 2: Project management (client, config store, service, CLI) by padak · Pull Request #2 · keboola/cli

padak · 2026-02-26T12:01:37Z

Implementation Summary

Full implementation of the project management lifecycle (Phase 2): add, list, remove, edit, status. Includes the Keboola API client with retry/timeouts, config persistence with secure file permissions, project service with dependency injection, and all CLI commands with proper exit codes and dual JSON/Rich output.

Files Changed

New files

src/keboola_agent_cli/client.py — KeboolaClient with httpx, retry (exponential backoff 1s/2s/4s for 429/5xx, max 3), timeouts (connect=5s, read=30s, write=10s, pool=5s), User-Agent header, verify_token(), list_components(), get_config_detail()
src/keboola_agent_cli/services/project_service.py — ProjectService with DI (client_factory callable), add/remove/edit/list/status operations
tests/test_client.py — 17 tests: verify_token success/401/403, retry on 503/429, retry exhaustion, no retry on 400, timeout, connect error, token masking, headers, context manager, list_components, get_config_detail
tests/test_config_store.py — 25 tests: load empty, save/load round-trip, file permissions 0600, add/remove/edit/get project, default project management, duplicate alias, version check, invalid JSON, invalid structure
tests/test_services.py — 18 tests: add project success/failure/duplicate/network, remove success/nonexistent, edit url/token/no-changes/nonexistent, list empty/multiple/token-masked, status ok/mixed/specific/nonexistent/token-masked
tests/test_cli.py — 14 tests: project add JSON/human/invalid-token-exit-3/timeout-exit-4, project list JSON-empty/JSON-with-data/human/human-empty, project remove success/nonexistent-exit-5, project status JSON/human, project edit url/config-error-exit-5

Modified files

src/keboola_agent_cli/models.py — Added TokenVerifyResponse model
src/keboola_agent_cli/config_store.py — Full implementation with version checking, error handling (ConfigError), duplicate alias protection
src/keboola_agent_cli/commands/project.py — Replaced stubs with real commands (Rich tables, proper exit codes, env var support)
src/keboola_agent_cli/cli.py — Wired up ConfigStore and ProjectService in context object

Acceptance Criteria

Tests

uv run pytest tests/ -v — 114 tests passing (40 Phase 1 + 74 Phase 2, 0 failures)

🤖 Generated with Claude Code

Implement the full project management lifecycle: add, list, remove, edit, status. - client.py: KeboolaClient with httpx, retry (exponential backoff for 429/5xx), timeouts (connect=5s, read=30s, write=10s, pool=5s), User-Agent header, verify_token(), list_components(), get_config_detail(), token masking in errors - config_store.py: Full ConfigStore with platformdirs, load/save with 0o600 permissions, add/remove/edit/get project, version checking, error handling - services/project_service.py: ProjectService with DI (client_factory callable), add/remove/edit/list/status with token verification and masked output - commands/project.py: All commands fully flag-based (non-interactive), proper exit codes (0/3/4/5), Rich table output for human mode, JSON for machine mode - cli.py: Wired up context object with config_store and project_service - models.py: Added TokenVerifyResponse model - Support for KBC_TOKEN and KBC_STORAGE_API_URL env vars Tests: 114 passing (40 original + 74 new) - test_client.py: verify_token success/401, retry on 503/429, timeout, token masking - test_config_store.py: load empty, add/remove/edit, permissions, version check - test_services.py: add/remove/edit/list/status with mocks, mixed success/failure - test_cli.py: project add/list/remove/edit/status via CliRunner, JSON and human mode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…, dedup Thermo-nuclear review follow-ups (behavior-preserving): - #1/#6: move inline nfpm loop -> build/package/linux/build_packages.sh and the zip+sha256 -> build/package/zip_binary.sh (explicit per-OS archiver); workflow is now pure orchestration. - #2: .github/actions/setup-build composite (uv + optional Node) replaces the repeated setup preamble in pypi/freeze. - #3: index.sh deb/rpm/apk collapsed into a publish_repo() helper + per-format indexers. - #5: sign.sh parses the Azure token with jq instead of a sed regex. - #7: nfpm.yaml notes it's rendered via envsubst. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The big one (#2, upgraded): the diff sides' wire shape was WRONG in the implementation and in the test fixtures that defined it. Verified against connection (ConfigurationVersionResponse + ConfigurationDiffData OA schemas, now recorded in the notes wire-truth table): each side is {version, isDeleted, diff: {name, description, changeDescription, isDisabled, configuration, rows}} -- content NESTED under diff, version/deletion as side metadata. The classification and both take modes now read the envelope; the flat-side code would have been dead on arrival against the live API. #1: resolve_conflict no longer takes branch_id -- it derives the branch from the MR itself (branches.branchFromId), so the conflict-set guard and the branch being written to can never disagree; a caller-supplied id could point the REPLACing rebase at an unrelated dev branch the guard never checked. A published/canceled MR (null branchFromId) is refused readably. #3: take=theirs of a deleted side collapses to the delete resolution, symmetric with ours ('production deleted it, dev changed it' is a live conflict shape). #4: deletion surfaces as top-level ours_deleted/theirs_deleted booleans on get_config_diff (None = side never existed) -- it is side metadata, not a content path. #5: a 'both' row where the sides agree on the identical value carries agreed: true -- agreement, not a conflict hotspot. #2 (message half): a take side missing required envelope keys is reported as a backend contract violation pointing at the resolved-body workaround, not as caller error. #6: wire ids compared via _same_id / int-coerced (find_merge_request_for_ branch, merge()'s was_active) -- a string-serialized branchFromId can no longer silently defeat the post-merge cleanup. #7: the feature pre-flight raises FeatureNotEnabledError carrying the new ErrorCode.FEATURE_NOT_ENABLED (value matches the string SearchService already emits; categorized 'configuration' like PAYG_NOT_AVAILABLE). #8: ConfigError imported from ..errors like every other service; the isDefault scan hoisted to services.base.find_default_branch_id and the copies in config/sync/workspace services migrated (lib.py keeps its own loop -- the SDK facade does not import the services layer); verify_token is skipped when the server already serialized viewer (the polyfill's cost dies with the polyfill); list --state validates against the closed vocabulary instead of returning a silent count: 0 on a typo. #9: test imports hoisted (no mid-file noqa), mocks spec'd at the L3 seam (KeboolaClient + MergeRequests -- a renamed L3 method now fails the tests), and regression tests added for every finding (82 tests total). Doc drift: layer2/layer3 references updated to BRANCHES_MERGE_REQUESTS_ FEATURE, the layer3 open nit closed, tokens.py line ref fixed. Review: tasks/pr-703-review.md (2026-08-27). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…st outcomes, wire-equivalent take [DMD-1899] All seven findings confirmed against HEAD (one already half-landed by the parallel Phase-5 round); the pattern they share is second-order effects of earlier review fixes -- locally correct changes whose downstream consumers were not re-examined: - merge(): a branchFromId that does not coerce is now RECORDED (warning + warnings[] naming the raw value, the skipped cleanups and the manual recovery: branch reset + sync branch-unlink). Before, the degradation was silent and indistinguishable from the legitimate null -- the merge reported unqualified success while active_branch_id kept pointing at the deleted branch. - merge(): the 'Active branch reset to main' message is gated on the RESET'S OUTCOME, not on was_active -- it no longer asserts the exact opposite of the warning beside it; the warning names `kbagent branch reset` as the recovery. - resolve_conflict take path: an absent description composes as None instead of being refused -- rebase_config omits the key for None and the server treats null and absent identically (isset), so the refusal blocked a wire-equivalent input and prescribed hand-authoring the very body it declined to compose. The resolved= path still requires presence (caller intent). isDisabled stays strict everywhere -- defaulting it would reintroduce the previous round's blocking finding. - non-bool isDisabled on a take side raises VALIDATION_ERROR blaming the server envelope (was: ConfigError blaming the caller); is_disabled forwards verbatim, no bool() coercion left. - _classify_three_way skips a tombstoned or empty-envelope side by the same 'no content to take' criterion resolve_conflict uses -- the null check alone left the flag-vs-paths contradiction reachable. - _branch_from_id_of: 'absent' and 'present but not numeric' get separate messages -- the lifecycle explanation no longer fires for a payload fault (it contradicted the state it printed in the same sentence). - find_default_branch_id: coerce-or-skip policy (an uncoercible id no longer raises ValueError mid-operation), first unit tests for the helper, and sync_service's truthiness test on the id tightened to `is not None`. Regression tests for each; the two pins the review called out (non-numeric-degrades, failed-reset) now assert the recorded warning and the outcome-gated message. Ported onto the pure Layer 2 branch: this fix was first authored on top of the Layer 1 working tree (which had renamed merge()'s soft-failure key and already carried half of the isDisabled type check). Here the merge() key stays `cleanup_warnings`, the full isDisabled boolean check lands as part of finding #2, and the Layer 1-only test class does not come along. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…900] Layer 2 moved under the RFC through four review rounds (Zajca, Copilot, findings O001-O003/S001 + a blocking second pass). Seven consequences for Layer 1 were walked one by one and decided; three of them open Layer 2 again, in the same PR as the commands. Layer 2 changes this PR carries (new section listing them in one place): - get_config_diff returns `resolution_candidate` -- the ours envelope through _DIFF_CONTENT_KEYS, description as explicit null, changeDescription excluded, null when ours is absent/deleted. After the blocking five-key replace guard, a `--output` prefill built anywhere else would produce a file kbagent itself refuses. One constant feeds guard and candidate. - merge() renames cleanup_warnings -> warnings. One soft-failure key for the group; a renderer reading `warnings` must not silently drop the merge's. - new get_merge_request_row(alias, id) -- client.merge_requests.get(id) through _enrich_row, nothing else. The armed check and the merge prompt need the row BEFORE the write, by id, without the detail's conflicts() + verify_token(). The row tier the L2 RFC already defines for list/find, addressed by id -- L3 had the GET all along. Layer 1 decisions: - One error handler for the whole group, no per-command `except`. FeatureNotEnabledError now surfaces from the resolver behind every omitted --merge-request-id (find runs the feature pre-flight on no-match, O002), i.e. from reads too -- exactly where a copied branch.py idiom would flatten it to CONFIG_ERROR. Two wordings (missing vs SOX) into gotchas. - diff renders the *_deleted flags BEFORE the table and recommends the --take: since #4, a null side yields zero per-path rows, so "production deleted it, dev changed it" is `changes: []` + `theirs_deleted: true` -- a table-first renderer prints three empty sections and "No changes" for the sharpest conflict there is. - MR_MERGE_CONFLICT's human render appends "list truncated -- run `merge-request conflicts`" whenever details.api_error_params_truncated is set (O003 bounded params to 20 entries); no number in the text. - allowed_actions feature-blindness (review #2) was DECLINED by Layer 2 and is inherited here as a documented Known gap, not compensated: paying the same GET one layer up is the same cost under another name. Owner: DMD-1988. - Both target flags at once -> exit 2, not silent precedence. - Under --json a state-derived escalation exits 2 only AFTER resolving the target -- deliberate, the information does not exist earlier; the error names the MR and the flag to pass. Also: corrected the false "only one GET" cost claim, and the three line references that drifted in the rebase (permissions.py:558/:621, constants.py:207). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…flict resolution (DMD-1899) (#703) * refactor(json): expose compute_diff's walk as structured DiffEntry data compute_diff returned pre-formatted strings, so a caller needing the changed paths as data (the MR three-way conflict classification, DMD-1899, intersects two pairwise diffs per path) had nothing to build on short of parsing the strings back. Split the recursive walk into compute_diff_entries() returning frozen DiffEntry dataclasses (path + old/new with an _ABSENT sentinel distinct from an explicit None), and keep compute_diff() as a formatter over it -- byte-identical output, pinned by a delegation test against the existing format tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(errors): MR merge-409 codes + surface the API's string code in details [DMD-1899] Three pieces of error plumbing the merge-request service needs: - ErrorCode.MR_NOT_READY_TO_MERGE / MR_MERGE_CONFLICT (+ docs/error-codes.md, both categorized 'conflict'). The merge 409 has four causes in two wire shapes; the split follows the backend's own line (the three transient causes carry storage.mergeRequests.notReadyToMerge, a conflict carries no code). Mapping happens in the service -- only it knows the 409 came from the merge endpoint. - http_base: a Keboola user error's machine string `code` now survives into KeboolaApiError.details['api_error_code'] (all four raise sites). The message holds only the human `error` text, so without this no caller can branch on the code. Additive; no behavior change when absent. - constants: FEATURE_BRANCHES_MERGE_REQUESTS renamed to BRANCHES_MERGE_REQUESTS_FEATURE (the file's dominant suffix convention; the constant was unused until now). SOX-fence assumption spelled out in the comment per the L2 RFC. Backend counterpart asking for codes on every MR error: DMD-1984. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(service): MR status-derivation polyfill + admin identity on verify [DMD-1899] The four derivates from the L2 RFC ('Derived status'), as pure module-level functions in the new services/merge_request_service.py: derive_state (the UI list badge's decision table incl. the rejected / closed-by-creator reviewer overrides), derive_merge_blockers (a list, so concurrent blockers don't mask each other; conflicts=None means not-fetched, not conflict-free), derive_allowed_actions (state-only; roles/features stay with the pre-flight), derive_viewer (is_creator / has_approved relative to the caller). All four are a POLYFILL: they read the future server-serialized field first (derivedState / mergeBlockers / allowedActions / viewer -- Connection issue DMD-1988) and fall back to the local tables; the fallbacks get deleted when DMD-1988 lands. Same defensive-read pattern as changeLog and the DMD-1969 approvals count. verify_token now also parses the response's top-level admin block into TokenVerifyResponse.admin_id/admin_name (additive; absent for scoped tokens) -- the anchor derive_viewer compares creator.id and approverId against. approverId is a string on the wire, ids are ints; comparisons normalize via str() like the UI does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(service): MergeRequestService reads -- list, find-by-branch, detail [DMD-1899] The service class (BaseService DI: ConfigStore + client_factory) and its three read methods: - list_merge_requests: rows enriched with derived_state; --state filters client-side (the endpoint has no query params) and matches the derived vocabulary (rejected/merged/closed/...) as well as raw states. - find_merge_request_for_branch: the branch->MR resolver behind L1's optional --mr-id (a branch has at most one MR ever, so the match is unambiguous); no MR -> NOT_FOUND naming merge-request create as the next step. - get_merge_request: detail with the full derived status -- merge_blockers/ mergeable/allowed_actions/viewer + the live conflicts list, fetched only for open MRs (a published/canceled MR's source branch is deleted, the conflicts endpoint is moot). viewer anchors on verify_token's admin id; a scoped token yields honest None flags. Derivations stay informational: the merge 409 remains the authority. Also the write pre-flight helper (_require_merge_requests_feature) with the SOX-fence assumption spelled out, used by the write methods that follow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(service): MR create, update and review transitions [DMD-1899] - create_merge_request: resolves the target itself (always the default branch -- the backend rejects any other) and refuses the default branch as source with a readable error instead of the backend's confusing 404. The source branch arrives explicit; L1 resolves it via the house resolve_branch() idiom per the RFC decision. - update_merge_request: None = leave unchanged (the API cannot clear to null; empty string clears description/externalId server-side). - request_review / approve / request_changes: thin transitions with the feature pre-flight; request_changes doubles as the close mechanism (no cancel endpoint exists -- the UI's cancel is this call by the creator, and derived_state renders it as closed). Every write runs _require_merge_requests_feature first; returns are the raw MR enriched with derived_state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(service): merge -- 409 mapping + post-merge local cleanup [DMD-1899] merge() waits for the Storage job (Layer 3, MERGE_JOB_MAX_WAIT) and adds the two service-level concerns from the RFC: 409 mapping (_remap_merge_conflict): the shape carrying storage.mergeRequests.notReadyToMerge (read from details.api_error_code, plumbed by the earlier http_base commit) becomes MR_NOT_READY_TO_MERGE, retryable -- all three causes are transient; a 409 without the code is the conflict validation and becomes MR_MERGE_CONFLICT, not retryable, with 'kbagent merge-request conflicts' named as the next step. Mapped here and not in http_base because only this call site knows the 409 came from the merge endpoint. Non-409 errors (STORAGE_JOB_FAILED rollback included) pass through unmapped. Post-merge cleanup mirrors BranchService.delete_branch: branchFromId is captured from the PRE-merge payload (nullable once published), active_branch_id resets only if it pointed at the merged branch (the was_active logic -- deliberately not get_merge_url's unconditional reset), and the sync branch-mapping entry is unlinked. The whole cleanup block is best-effort: the merge already happened, so a failure degrades to cleanup_warnings in the result instead of a failed command. The message says the source branch 'is being deleted' -- never that it is gone (second async job, no handle). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(service): conflicts, per-path 3-way diff, resolve via rebase [DMD-1899] The conflict-resolution half of the service: - list_conflicts: pass-through with count (conflicts are computed live by the backend on every call/merge attempt -- rebasing each listed config is sufficient, no re-validate step exists). - get_config_diff: flattens the nested base/ours/theirs (each nullable) into a per-path classification -- changed_by: ours | theirs | both, built by intersecting two compute_diff_entries() pairwise diffs. A side that removed a key shows None (never the base value); an untouched side shows the base. rows compare wholesale. onto_version carries theirs.version -- the rebase version trap spelled out once, in data. - resolve_conflict: every mode goes through the rebase endpoint (RFC decision; the UI's reset-to-default for take=theirs is DMD-1987). take=ours|theirs composes the full replace body from the diff side with version=theirs.version; take=ours of a deleted side becomes the delete resolution; take=delete sends the tombstone; a caller-authored resolved body must spell out name/rows/configuration explicitly (rebase REPLACES -- a defaulted key would be silent data loss). The config must be in the MR's live conflict set, and rebase runs the feature pre-flight (the one endpoint gated on branches-merge-requests specifically). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(service): apply the PR #703 self-review findings [DMD-1899] The big one (#2, upgraded): the diff sides' wire shape was WRONG in the implementation and in the test fixtures that defined it. Verified against connection (ConfigurationVersionResponse + ConfigurationDiffData OA schemas, now recorded in the notes wire-truth table): each side is {version, isDeleted, diff: {name, description, changeDescription, isDisabled, configuration, rows}} -- content NESTED under diff, version/deletion as side metadata. The classification and both take modes now read the envelope; the flat-side code would have been dead on arrival against the live API. #1: resolve_conflict no longer takes branch_id -- it derives the branch from the MR itself (branches.branchFromId), so the conflict-set guard and the branch being written to can never disagree; a caller-supplied id could point the REPLACing rebase at an unrelated dev branch the guard never checked. A published/canceled MR (null branchFromId) is refused readably. #3: take=theirs of a deleted side collapses to the delete resolution, symmetric with ours ('production deleted it, dev changed it' is a live conflict shape). #4: deletion surfaces as top-level ours_deleted/theirs_deleted booleans on get_config_diff (None = side never existed) -- it is side metadata, not a content path. #5: a 'both' row where the sides agree on the identical value carries agreed: true -- agreement, not a conflict hotspot. #2 (message half): a take side missing required envelope keys is reported as a backend contract violation pointing at the resolved-body workaround, not as caller error. #6: wire ids compared via _same_id / int-coerced (find_merge_request_for_ branch, merge()'s was_active) -- a string-serialized branchFromId can no longer silently defeat the post-merge cleanup. #7: the feature pre-flight raises FeatureNotEnabledError carrying the new ErrorCode.FEATURE_NOT_ENABLED (value matches the string SearchService already emits; categorized 'configuration' like PAYG_NOT_AVAILABLE). #8: ConfigError imported from ..errors like every other service; the isDefault scan hoisted to services.base.find_default_branch_id and the copies in config/sync/workspace services migrated (lib.py keeps its own loop -- the SDK facade does not import the services layer); verify_token is skipped when the server already serialized viewer (the polyfill's cost dies with the polyfill); list --state validates against the closed vocabulary instead of returning a silent count: 0 on a typo. #9: test imports hoisted (no mid-file noqa), mocks spec'd at the L3 seam (KeboolaClient + MergeRequests -- a renamed L3 method now fails the tests), and regression tests added for every finding (82 tests total). Doc drift: layer2/layer3 references updated to BRANCHES_MERGE_REQUESTS_ FEATURE, the layer3 open nit closed, tokens.py line ref fixed. Review: tasks/pr-703-review.md (2026-08-27). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(service): apply the Opus wire-truth review -- 409 codes, allowed_actions, derive_state honesty [DMD-1899] Adversarial review of every wire assumption against Connection source (tasks/pr-703-opus-wire-review.md): 7 CONFIRMED, 3 MISMATCH. The fixes: 409 mapping (finding #4): the conflict 409 is NOT code-less -- an earlier reading missed ExceptionConverter, which serializes MergeValidationException's own string code storage.mergeRequests.validation top-level as `code`, plus the conflicting configurations in `params.errors`. _remap_merge_conflict now matches BOTH codes explicitly (code-less 409 falls back to conflict for older stacks; a 409 with any OTHER code passes through unmapped instead of being confidently mislabeled a conflict), and http_base surfaces `params` as details.api_error_params so the conflict list travels with the error instead of demanding a second round trip. Stale 'carries no code' claims corrected in errors.py, the L3 docstring, error-codes.md, the notes wire-truth table and the RFC. allowed_actions (finding #7): `approve` removed from the `approved` tuple -- the transition's sole `from` place is in_review; from approved the backend answers 422 (the UI button offering it there is wrong). `update` added to in_merge (the server blocks update only in terminal states). Docstring records the AddApprovalGuard gating and that with the non-SOX default of 0 required approvals, approve is 422 in every state and in_review itself is unreachable. derive_state honesty (finding #10, the significant one): reviewers[].status is populated only within a review round anchored by a review_requested activity event -- which skip_review never writes -- and explicit reviewers shadow every non-reviewer's decision (the creator can never BE a reviewer). So in a default non-SOX project the rejected / self-closed derivations never fire; the UI badge has the identical blind spot, since our table is its port. Documented in the docstring and the RFC rather than re-derived from the activity log client-side: the reliable fix is server-side (commented on DMD-1988 -- derive from the activity log, not reviewers[]). Sharp edges from confirmed items: a take side with a null/empty `name` (nullable in the diff envelope, required non-empty by the rebase validator) is refused as a contract violation instead of sailing into a server 400; _branch_from_id_of documents that its null check is racy (branchFromId is nulled by the FK when the async branch delete lands, not by the state change); the viewer docstring notes detail/conflicts require an admin token anyway (MergeRequestVoter). Regression tests for each fix; 87 service tests total. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * style(service): hoist side_value out of the loop, typed test sentinel [DMD-1899] Two quality nits from the pre-review self-audit: the side_value helper was re-defined on every classification loop iteration (it captures nothing loop-scoped -- hoisted above the loop); the test _arm fixture abused Ellipsis as a keep-default sentinel with a type: ignore -- replaced with a typed module object sentinel, which also unbreaks `make typecheck` over tests (ty rejects an EllipsisType default on a dict|None parameter). Also pinned down the test_changelog_render 'environmental' failures for good: they reproduce exactly when FORCE_COLOR is set (Warp exports FORCE_COLOR=3), which makes Rich emit ANSI inside the asserted substrings ('New: ' renders bold, splitting 'New: alpha thing.'). A main-branch test robustness issue, unrelated to this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(service): one predicate for the server viewer block (Copilot review) [DMD-1899] get_merge_request's verify_token skip accepted ANY dict as a server viewer, while derive_viewer required isCreator/hasApproved keys -- a bare `viewer: {}` (or one with foreign keys) would skip the call and yield {None, None} instead of the local derivation. Both sites now share _server_viewer(), the single predicate for 'did DMD-1988 land', so they cannot disagree. Regression test included. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(service): tell a SOX refusal apart from a missing feature [DMD-1899] The pre-flight refusal said the same thing to two very different projects: a SOX project (protected-default-branch -- refused as deliberate CLI policy, nothing to enable) and a plain project that simply does not have merge requests turned on (something to ask support for). The features cache is already loaded by the first has_feature call, so the distinction is free: a new PROTECTED_DEFAULT_BRANCH_FEATURE constant + a dedicated SOX message pointing at the Keboola UI, vs. the enable-the-feature hint. Both paths stay FeatureNotEnabledError / FEATURE_NOT_ENABLED -- the split is in the wording, not the contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(service): apply the Layer 1 RFC findings [DMD-1899] Seven findings surfaced while writing the Layer 1 command RFC (DMD-1900), all verified against Connection (tasks/dmd-1899-findings-from-layer1.md): 1. get_config_diff now derives the branch from the merge request (signature: alias, merge_request_id, component_id, config_id) -- the same make-the-wrong-call-unrepresentable reasoning resolve_conflict already carries; a caller-supplied branch id had no relation to any MR. The resolved branch_id is echoed in the result; a published/canceled MR is refused readably. The old tests passed the shifted positional args straight into MagicMock without failing -- rewritten to pin the wiring (the L3 diff call must receive branchFromId). 2. _enrich_row now adds allowed_actions alongside derived_state, so every return (list rows, find, create, update, transitions, merge's post-merge state) answers 'what can I do next' without a second call. 3. SAFETY FACT recorded: autoMergeStrategy=immediately makes a background backend tick merge any approved MR through the same MergeProcessor under a system token (AutoMergeCandidateRepository.php:38-47, AutoMergeTickHandler.php:86) -- create+request_review can end in a production merge with merge() never called. New notes section + one sentence in both create/update docstrings. 4. Notes wire-truth table gains the full MergeRequestResponse shape: merge{mergedAt,mergerId,mergerName} is NESTED, createdAt is top-level, autoMergeStrategy/autoMergeAt are response fields too (MergeRequestResponseProvider.php:86-117; list==detail item shape). 5. MergeRequestVoter recorded in the notes (admin-identity axis vs. the role axis -- not a contradiction), 403 added to the detail/conflicts rows in the layer3 doc, derive_viewer docstring softened (the None-flags branch is defense in depth, not a reachable path via get_merge_request). 6. An empty list result now carries feature_enabled from has_feature, so 'no merge requests' and 'feature not enabled' stop being the same 200 + []; the extra verify_token GET is spent only on the empty case. 7. STATE_FILTER_VOCABULARY and TAKE_MODES are public for Layer 1 to enumerate in help text and pre-validate to exit 2 (precedent: notification_service's KNOWN_EVENTS). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(service,http): apply Zajca's PR review -- null-value guards, feature-aware find, bounded params [DMD-1899] Three of the four findings applied (the fourth -- a stale _STATE_FILTER_VOCABULARY reference in the Layer 1 RFC -- was already fixed by the RFC rewrite in 980b28d): 1. resolve_conflict's replaced-content guard now refuses present-but-null values for EVERY required key, not just name. The backend's `?? new stdClass()` fires on null as well as absence, so a `configuration: null` in a caller-authored resolution file would have REPLACED the config body with {} and returned 200 -- silent data loss, the exact thing the guard exists to refuse. A null `rows` is equally ambiguous ([] means delete-all and must stay expressible) and is refused too; a genuine [] keeps working (pinned by test). 2. find_merge_request_for_branch runs the feature pre-flight on the no-match path: the ungated list answers 200 + [] on a project without branches-merge-requests, and the old NOT_FOUND prescribed a create that was guaranteed to fail with FEATURE_NOT_ENABLED -- a user-facing loop on the resolver behind every command's optional --mr-id. The check costs one verify_token GET, spent only when nothing matched (pinned: the happy path never pays it). Same treatment list_merge_requests already had. 3. details['api_error_params'] is now BOUNDED like the message beside it: over MAX_API_ERROR_PARAMS_LENGTH (8192 serialized), top-level lists are truncated to MAX_API_ERROR_PARAMS_LIST_ITEMS (20) entries; if still over, params is dropped -- either way details carries api_error_params_truncated: true. A merge over hundreds of conflicting configs no longer floods the agent-consumed --json envelope, and an unserializable payload is dropped rather than crashing the error path. Regression tests for each (+ the []-stays-legal and happy-path-cost pins). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(http): restore details= on the 401 INVALID_TOKEN raise lost in the rebase [DMD-1899] The rebase onto main resolved this branch's 'surface code/params in details' commit against #717's 401 split (INVALID_TOKEN vs AUTH_REJECTED): the new AUTH_REJECTED branch got details=, the surviving INVALID_TOKEN branch lost it -- a 401 whose body carries a machine code/params would silently drop them. All five raise sites in _raise_api_error now carry details= again. Found by range-diffing the pre-rebase set against the rebased one; the other 16 commits replayed byte-identically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(service): apply Zajca's second review -- full replace-body guard, null-side classification, delete warning [DMD-1899] BLOCKING finding: resolve_conflict guarded 3 of the 5 replaced-body keys. Rebase REPLACES, so a custom body without isDisabled silently RE-ENABLED a disabled configuration (and the merge would push that to production), one without description wiped it. All five keys are now required: name/rows/ configuration/isDisabled refuse absent-or-null, description refuses only ABSENCE -- an explicit null is a decision (the field is genuinely nullable), an omission is not. is_disabled forwards verbatim (bool(body['isDisabled'])), never defaulted. Non-blocking findings: - _classify_three_way returns no per-path rows when either side is null: fabricating them against an empty stand-in rendered every base key as 'that side removed it' next to a *_deleted flag saying the side does not exist. Side-level facts stay on the flags. - take=delete with a change_description now warns that the text is dropped -- the delete tombstone's wire envelope ({'version': N, 'diff': {}}) cannot carry one, so plumbing it through Layer 3 is impossible; silent was the only wrong option. - allowed_actions staying feature-blind is now a documented decision on _enrich_row (closing it costs a verify_token GET per non-empty list for the rare ex-feature project; DMD-1988's server-side allowedActions is the real fix). Nits: history-narrating comments removed (reviewer names/dates belong in the PR, not the code); search_service uses ErrorCode.FEATURE_NOT_ENABLED instead of the raw string; merge()'s two post-merge cleanups run in independent try blocks (a failed config write no longer skips the mapping unlink); wire branch ids coerce through _coerce_branch_id (non-numeric degrades to 'no usable id' instead of an unhandled ValueError); find_default_branch_id tolerates an isDefault entry without an id. Not changed: cleanup_branch_id_from_mapping's project-unscoped matching is inherited from BranchService.delete_branch house-wide -- noted for a follow-up rather than diverging the two call sites here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(code): repoint RFC references to the ms/merge-requests-rfcs branch [DMD-1899] The merge-request RFC set (layer1/2/3 + wire-truth notes) moved off this branch to ms/merge-requests-rfcs -- RFCs deliberately do not merge to main. The five docstring/comment references to docs/merge-requests-*.md paths would have dangled on main; the module docstring now names the RFC branch once and the other sites reference 'the L2 RFC' / the wire-truth notes through it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(service): apply Zajca's third review -- record degradations, honest outcomes, wire-equivalent take [DMD-1899] All seven findings confirmed against HEAD (one already half-landed by the parallel Phase-5 round); the pattern they share is second-order effects of earlier review fixes -- locally correct changes whose downstream consumers were not re-examined: - merge(): a branchFromId that does not coerce is now RECORDED (warning + warnings[] naming the raw value, the skipped cleanups and the manual recovery: branch reset + sync branch-unlink). Before, the degradation was silent and indistinguishable from the legitimate null -- the merge reported unqualified success while active_branch_id kept pointing at the deleted branch. - merge(): the 'Active branch reset to main' message is gated on the RESET'S OUTCOME, not on was_active -- it no longer asserts the exact opposite of the warning beside it; the warning names `kbagent branch reset` as the recovery. - resolve_conflict take path: an absent description composes as None instead of being refused -- rebase_config omits the key for None and the server treats null and absent identically (isset), so the refusal blocked a wire-equivalent input and prescribed hand-authoring the very body it declined to compose. The resolved= path still requires presence (caller intent). isDisabled stays strict everywhere -- defaulting it would reintroduce the previous round's blocking finding. - non-bool isDisabled on a take side raises VALIDATION_ERROR blaming the server envelope (was: ConfigError blaming the caller); is_disabled forwards verbatim, no bool() coercion left. - _classify_three_way skips a tombstoned or empty-envelope side by the same 'no content to take' criterion resolve_conflict uses -- the null check alone left the flag-vs-paths contradiction reachable. - _branch_from_id_of: 'absent' and 'present but not numeric' get separate messages -- the lifecycle explanation no longer fires for a payload fault (it contradicted the state it printed in the same sentence). - find_default_branch_id: coerce-or-skip policy (an uncoercible id no longer raises ValueError mid-operation), first unit tests for the helper, and sync_service's truthiness test on the id tightened to `is not None`. Regression tests for each; the two pins the review called out (non-numeric-degrades, failed-reset) now assert the recorded warning and the outcome-gated message. Ported onto the pure Layer 2 branch: this fix was first authored on top of the Layer 1 working tree (which had renamed merge()'s soft-failure key and already carried half of the isDisabled type check). Here the merge() key stays `cleanup_warnings`, the full isDisabled boolean check lands as part of finding #2, and the Layer 1-only test class does not come along. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

…900] Layer 2 moved under the RFC through four review rounds (Zajca, Copilot, findings O001-O003/S001 + a blocking second pass). Seven consequences for Layer 1 were walked one by one and decided; three of them open Layer 2 again, in the same PR as the commands. Layer 2 changes this PR carries (new section listing them in one place): - get_config_diff returns `resolution_candidate` -- the ours envelope through _DIFF_CONTENT_KEYS, description as explicit null, changeDescription excluded, null when ours is absent/deleted. After the blocking five-key replace guard, a `--output` prefill built anywhere else would produce a file kbagent itself refuses. One constant feeds guard and candidate. - merge() renames cleanup_warnings -> warnings. One soft-failure key for the group; a renderer reading `warnings` must not silently drop the merge's. - new get_merge_request_row(alias, id) -- client.merge_requests.get(id) through _enrich_row, nothing else. The armed check and the merge prompt need the row BEFORE the write, by id, without the detail's conflicts() + verify_token(). The row tier the L2 RFC already defines for list/find, addressed by id -- L3 had the GET all along. Layer 1 decisions: - One error handler for the whole group, no per-command `except`. FeatureNotEnabledError now surfaces from the resolver behind every omitted --merge-request-id (find runs the feature pre-flight on no-match, O002), i.e. from reads too -- exactly where a copied branch.py idiom would flatten it to CONFIG_ERROR. Two wordings (missing vs SOX) into gotchas. - diff renders the *_deleted flags BEFORE the table and recommends the --take: since #4, a null side yields zero per-path rows, so "production deleted it, dev changed it" is `changes: []` + `theirs_deleted: true` -- a table-first renderer prints three empty sections and "No changes" for the sharpest conflict there is. - MR_MERGE_CONFLICT's human render appends "list truncated -- run `merge-request conflicts`" whenever details.api_error_params_truncated is set (O003 bounded params to 20 entries); no number in the text. - allowed_actions feature-blindness (review #2) was DECLINED by Layer 2 and is inherited here as a documented Known gap, not compensated: paying the same GET one layer up is the same cost under another name. Owner: DMD-1988. - Both target flags at once -> exit 2, not silent precedence. - Under --json a state-derived escalation exits 2 only AFTER resolving the target -- deliberate, the information does not exist earlier; the error names the MR and the flag to pass. Also: corrected the false "only one GET" cost claim, and the three line references that drifted in the rebase (permissions.py:558/:621, constants.py:207). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

padak merged commit 33cd47b into main Feb 26, 2026

padak deleted the phase-2-projects branch March 2, 2026 23:05

padak mentioned this pull request Mar 26, 2026

feat: add --token-expires-in to org setup #61

Merged

3 tasks done

ottomansky mentioned this pull request Apr 22, 2026

fix(0.21.2): job run banner reads resolved id, hint threads branch_id, whitespace strip #199

Merged

6 tasks done

This was referenced May 3, 2026

feat(0.28.0): manage-token default-deny — env var ignored without --allow-env-manage-token #239

Closed

v0.30.1: keboola-mcp-server auto-update banner re-fires on every kbagent invocation #263

Closed

padak mentioned this pull request May 7, 2026

sync git-branching: branch_id type confusion + walker scope guard block GitOps workflow #267

Closed

padak mentioned this pull request Jun 1, 2026

feat(storage): clone-table + correct typify/swap dev-branch semantics (0.52.0) #368

Merged

ottomansky mentioned this pull request Jun 3, 2026

feat(semantic-layer): add reference-data commands (Chart of Accounts in the metastore) #379

Merged

padak mentioned this pull request Jun 3, 2026

fix(config): encrypt #-prefixed secrets before write (0.54.0) (#378) #380

Merged

padak mentioned this pull request Jun 13, 2026

feat(data-app): git-repo introspection + managed-repo credentials (0.61.0) #414

Merged

padak mentioned this pull request Aug 28, 2026

docs: add partition-table-workflow reference guide #712

Open

martinsifra mentioned this pull request Sep 2, 2026

feat(service): merge-request Layer 2 - lifecycle, derived status, conflict resolution (DMD-1899) #703

Merged

zajca mentioned this pull request Sep 7, 2026

feat(cli): merge-request command group -- Layer 1 over MergeRequestService (DMD-1900) #736

Merged

soustruh mentioned this pull request Sep 14, 2026

fix(sync): keep the config folder on pull and clone (CLI-9) #753

Merged

MichalProchazkaP3 mentioned this pull request Sep 24, 2026

Self-update triggered mid-session leaves kbagent completely uninstalled (worse than #771's stale-version case) #786

Closed

This was referenced Sep 26, 2026

fix(sync): promoting main/ to a dev branch is idempotent (#792 D) #794

Merged

fix(sync): pull never deletes a dir it wrote or a locally edited dir (#792 A, C) #796

Merged

Navigation Menu

Repository navigation

Phase 2: Project management (client, config store, service, CLI) - #2

Phase 2: Project management (client, config store, service, CLI)#2
padak merged 1 commit into
mainfrom
phase-2-projects

padak commented Feb 26, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

padak commented Feb 26, 2026

Implementation Summary

Files Changed

New files

Modified files

Acceptance Criteria

Tests

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant