Repository navigation
Phase 5: Integration tests, edge cases, linting, final polish - #6
Merged
Merged
Conversation
Add comprehensive test coverage and code quality tooling: - Integration test suite (tests/test_integration.py) with full workflow test that exercises add/list/status/config-list/doctor/remove cycle, skipped without KBA_TEST_TOKEN_AWS env var - Edge case tests for client: malformed JSON, empty responses, large responses, 404 handling, stack URL normalization - Edge case tests for config_store: corrupted files (binary, truncated, null JSON, array), missing directories, permission denied, multiple save/load cycles - Additional CLI tests: help output, verbose flag, missing required args, token re-verification on edit - Ruff linting and formatting configuration with ruff>=0.8 dev dependency - Fixed all linting issues: unused imports, unused variables, raise-from in except clauses, modern type annotations - Hardened config_store to handle non-dict JSON and binary file content - All 209 tests pass (173 original + 36 new), 3 integration tests skipped Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Closed
5 of 8 tasks
Matovidlo
added a commit
that referenced
this pull request
Jun 15, 2026
…, 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>
Matovidlo
added a commit
that referenced
this pull request
Jun 17, 2026
…, 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>
martinsifra
added a commit
that referenced
this pull request
Aug 26, 2026
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>
martinsifra
added a commit
that referenced
this pull request
Aug 31, 2026
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>
martinsifra
added a commit
that referenced
this pull request
Sep 2, 2026
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>
martinsifra
added a commit
that referenced
this pull request
Sep 3, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implementation Summary
tests/test_integration.py) with full workflow test (skipped without env vars)ruff>=0.8) and formatting configuration inpyproject.tomlraise ... fromin except clauses, modern type annotationsconfig_store.pyto handle non-dict JSON and binary file content gracefullyChanges
pyproject.toml: Added[tool.ruff],[tool.ruff.lint],[tool.ruff.lint.isort]configuration,ruff>=0.8dev dependency, pytestintegrationmarkertests/test_integration.py(NEW): 3 integration tests with@pytest.mark.skipifwhenKBA_TEST_TOKEN_AWSnot settests/test_client.py: +10 edge case tests (malformed JSON, empty response, large response, 404, URL normalization)tests/test_config_store.py: +12 edge case tests (corrupted files, missing dirs, permissions, save/load cycles)tests/test_cli.py: +13 tests (help, verbose, missing args, token re-verification)src/keboola_agent_cli/config_store.py: Handle non-dict JSON andUnicodeDecodeErrorgracefullysrc/keboola_agent_cli/client.py: Removed unusedlast_exceptionvariableraise ... from Nonefor B904 compliance, unused import cleanupAcceptance Criteria
209 passed, 3 skipped)All checks passed!)26 files already formatted)Tests
🤖 Generated with Claude Code