Phase 2: Foundation — Shared Constants & HTTP Base by padak · Pull Request #8 · keboola/cli

padak · 2026-02-28T00:13:29Z

Summary

Extract duplicated constants and HTTP retry logic into shared modules (constants.py, http_base.py)
Both KeboolaClient and ManageClient now inherit from BaseHttpClient, eliminating ~220 lines of duplicated code
All hardcoded values (retry params, timeouts, job limits, env var names, token description, stack URL) centralized in constants.py

Acceptance Criteria

No duplicate RETRYABLE_STATUS_CODES, MAX_RETRIES, BACKOFF_BASE definitions remain
No duplicate _do_request() or _raise_api_error() methods remain
No hardcoded timeout values in client.py or manage_client.py — all from constants.py
BaseHttpClient in http_base.py contains all shared HTTP logic
KeboolaClient and ManageClient both inherit from BaseHttpClient
All existing tests pass (466 passed, 3 skipped)
No hardcoded "kbagent-cli" strings remain outside constants.py
No hardcoded 50 / 500 job limits remain outside constants.py

Tests

All 449 existing tests pass unchanged
17 new tests added in test_http_base.py:
- TestBaseHttpClientRetry — retry on 503, 429, timeout, connect error, exhaustion, alternate client
- TestBaseHttpClientErrorSanitization — message truncation, error code mapping, token masking, non-JSON handling
- TestBaseHttpClientContextManager — context manager protocol

Files Changed

NEW: src/keboola_agent_cli/constants.py — all shared constants
NEW: src/keboola_agent_cli/http_base.py — BaseHttpClient with shared HTTP logic
NEW: tests/test_http_base.py — 17 tests for BaseHttpClient
Modified: src/keboola_agent_cli/client.py — inherits from BaseHttpClient
Modified: src/keboola_agent_cli/manage_client.py — inherits from BaseHttpClient
Modified: src/keboola_agent_cli/services/base.py — imports from constants.py
Modified: src/keboola_agent_cli/commands/project.py — imports from constants.py
Modified: src/keboola_agent_cli/commands/job.py — imports from constants.py
Modified: src/keboola_agent_cli/services/job_service.py — imports from constants.py
Modified: src/keboola_agent_cli/services/org_service.py — imports from constants.py
Modified: src/keboola_agent_cli/commands/org.py — imports from constants.py
Modified: tests/test_client.py — updated imports and sleep patches for http_base

- NEW: constants.py with all shared constants (retry params, timeouts, env var names, default values, job limits, token description) - NEW: http_base.py with BaseHttpClient providing shared _do_request() and _raise_api_error() with retry logic and message sanitization - Refactor KeboolaClient and ManageClient to inherit from BaseHttpClient - Remove ~220 lines of duplicated HTTP retry/error code - Replace all hardcoded constants across 8 files with imports from constants.py (job limits, stack URL, token description, env vars) - Add 17 new BaseHttpClient tests (retry, error mapping, sanitization) - All 466 existing + new tests pass

upload-table now auto-creates the target bucket and/or table if they don't exist, making it a one-shot command. Columns are inferred as STRING from the CSV header row. Use --no-auto-create to require the table to already exist (previous behaviour).

* feat: add storage create-bucket, create-table, upload-table Adds three write operations to `kbagent storage` that were missing: - `storage create-bucket` — POST /v2/storage/buckets (sync) - `storage create-table` — POST /v2/storage/buckets/{id}/tables-definition (async job) - `storage upload-table` — POST /v2/storage/tables/{id}/import (sync, multipart CSV) Column types for create-table: STRING, INTEGER, NUMERIC, FLOAT, BOOLEAN, DATE, TIMESTAMP. Upload supports --incremental flag, custom --delimiter and --enclosure. Also fixes a pre-existing Windows bug in config_store.py where os.replace() failed with WinError 5 because the lock fd held the destination file open. On Windows (no fcntl), the fd is now closed before os.replace(). Tested against connection.europe-west3.gcp.keboola.com. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: address PR review -- async upload, permissions, validation, docs - client.py: replace sync /import with 3-step async flow (prepare file → upload to cloud storage → import-async + poll); supports files up to 5 GB. Add prepare_file_upload(), _upload_to_cloud(), import_table_async() methods. Add max_wait param to _wait_for_storage_job() (import uses IMPORT_JOB_MAX_WAIT=600s). Use pathlib.Path instead of os.path (drop import os). - constants.py: add IMPORT_JOB_MAX_WAIT, VALID_COLUMN_TYPES, FILE_UPLOAD_TIMEOUT. - services/storage_service.py: validate stage ('in'/'out') and column types against VALID_COLUMN_TYPES; raise ValueError with clear message on invalid input. Add file_size_bytes to upload_table() response. Use pathlib.Path. - commands/storage.py: catch ValueError -> exit 2 in create-bucket and create-table. Show file size (MB) in human-readable upload output. Use pathlib.Path. - permissions.py: register storage.create-bucket, storage.create-table, storage.upload-table as 'write' operations. - commands/context.py: add create-bucket, create-table, upload-table to Storage section. - SKILL.md: add 3 rows to decision table. - tests/test_storage_write.py: add tests for stage/column-type validation, file_size_bytes, fix service tests to use real temp files for stat() call. * fix: avoid boolean serialization bug in prepare_file_upload and document ABS limitation Python False serializes to the string 'False' in form-data, which many backends treat as truthy. Omitting isPermanent/isPublic (both default to false in the API) prevents files from accidentally being marked public. Also document ABS (Azure Blob Storage) as unsupported in _upload_to_cloud. * test: add client-level tests for 3-step async upload flow Add TestPrepareFileUpload, TestUploadToCloud, TestImportTableAsync, TestUploadTableClient to test_client.py using httpx_mock. Key assertions: - prepare_file_upload sends only name+sizeBytes (no boolean fields) - _upload_to_cloud handles 204 (S3), 200 (GCS), and raises on 4xx - import_table_async polls job until success and raises on failure - upload_table full-flow integration: prepare -> S3 -> import-async -> poll Also fix misleading '(S3/GCS/ABS)' reference in upload_table docstring. * feat: auto-create bucket and table on upload-table (#8) upload-table now auto-creates the target bucket and/or table if they don't exist, making it a one-shot command. Columns are inferred as STRING from the CSV header row. Use --no-auto-create to require the table to already exist (previous behaviour). * fix: handle GCP gcsUploadParams bearer-token upload path GCP stacks return gcsUploadParams (bucket + key + OAuth2 access_token) instead of a presigned POST URL. _upload_to_cloud now checks for gcsUploadParams first and uses a GCS JSON API PUT with Authorization: Bearer header. S3 (uploadParams non-empty) and signed-URL PUT paths (uploadParams empty) are preserved unchanged. * fix: update upload docstring, complete test mocks, exclude append file from lint - upload_table docstring: clarify cloud storage paths (GCP bearer, S3 POST, signed URL PUT) - test_upload_table_json mock: add file_size_bytes, auto_created_bucket, auto_created_table - pyproject.toml: exclude _test_upload_client_append.py from ruff (append-only staging file) * fix: uploading progress message, GCP full-flow test, docstring accuracy - Show "Uploading <file> (X.XX MB) to <table>..." before upload starts - Add TestUploadTableClient.test_full_flow_gcp: end-to-end test for the GCP bearer-token upload path (gcsUploadParams → PUT to storage.googleapis.com) - Expand prepare_file_upload docstring Returns to mention gcsUploadParams (GCP stacks return bearer token + bucket/key instead of presigned POST URL) * fix: remove ruff exclude for uncommitted local test file tests/_test_upload_client_append.py was never committed to the repo, so its ruff exclude entry in pyproject.toml was a local artifact. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <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>

…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>

padak merged commit 917defb into main Feb 28, 2026

padak deleted the phase-2-foundation-constants-http branch February 28, 2026 00:14

ottomansky mentioned this pull request May 3, 2026

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

Closed

5 of 8 tasks

padak mentioned this pull request Sep 25, 2026

Test suite audit: ~280 low-value tests, 8 false-passing negative controls, test-only production seams #790

Open

Navigation Menu

Repository navigation

Phase 2: Foundation — Shared Constants & HTTP Base - #8

Phase 2: Foundation — Shared Constants & HTTP Base#8
padak merged 1 commit into
mainfrom
phase-2-foundation-constants-http

padak commented Feb 28, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

padak commented Feb 28, 2026

Summary

Acceptance Criteria

Tests

Files Changed

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant