Skip to content

feat(0.22.0): ErrorCode enum + sync init --adopt-existing - #201

Closed
ottomansky wants to merge 7 commits into
keboola:release/0.22.0from
ottomansky:fiia/pr9-errors-and-sync-adopt
Closed

feat(0.22.0): ErrorCode enum + sync init --adopt-existing#201
ottomansky wants to merge 7 commits into
keboola:release/0.22.0from
ottomansky:fiia/pr9-errors-and-sync-adopt

Conversation

@ottomansky

Copy link
Copy Markdown
Contributor

Summary

  • P2-1 · Stable error-code enum: ErrorCode(StrEnum) in errors.py with 46 typed constants. All error_code="STRING" literals across 28 source files replaced with ErrorCode.<MEMBER>. Wire format unchanged (StrEnum is a str subtype). scripts/check_error_codes.py CI guard wired into make check rejects new raw literals. docs/error-codes.md documents semver policy (add=minor, rename/remove=major).
  • P2-2 · sync init --adopt-existing: idempotently adopts a .keboola/manifest.json written by the kbc Go CLI without overwriting it. Validates manifest project_id against the alias token; rejects mismatch with ConfigError (exit 5). Falls through to normal init when no manifest exists. Manifest.git_branching gains default_factory so minimal manifests without the field parse cleanly.

Resolution

Both features close FIIA gap items P2-1 and P2-2. The ErrorCode enum is a pure refactor — no wire-format change. The adopt-existing path is additive (new flag, new code path, no changes to existing init/pull behavior).

Architecture

  • ErrorCode(StrEnum) in errors.py — StrEnum members are str subtypes so they compare equal to and serialize as plain strings everywhere.
  • CI guard uses ast.walk to detect error_code keyword args with ast.Constant string values; skips errors.py, tests/, and migration tooling.
  • _adopt_existing_manifest() in sync_service.py — calls client.verify_token() to get the real project_id, compares to manifest, raises ConfigError on mismatch, calls save_manifest() for a clean round-trip on adopt.
  • Manifest.git_branching default: default_factory=ManifestGitBranching so kbc manifests that omit the field parse cleanly.

Test plan

  • tests/test_errors.py::TestErrorCode — 7 tests: str equality, isinstance(str), json.dumps, no duplicates, known codes present, default uses enum, accepts enum
  • tests/test_sync_service.py::TestAdoptExistingManifest — 5 tests: happy path, project_id mismatch, missing manifest falls through, no-flag still raises on existing, idempotent re-run
  • tests/test_sync_cli.py::TestSyncInitAdoptExistingCli — 4 tests: flag in --help, JSON output, human output shows "Adopted", ConfigError exits 5
  • tests/test_integration.py::TestCheckErrorCodesGuard — 3 tests: clean source passes, planted literal caught, enum usage ignored
  • tests/test_e2e.py::TestE2ESyncAdoptExisting — 2 tests against connection.europe-west3.gcp.keboola.com: adopt success + sync status works, wrong project_id rejected with exit 5
  • Full suite: 1850 passing, 17 pre-existing Windows-only failures (fcntl, file permissions, Rich truncation)
  • make check: lint + format + error-code guard all clean

Pre-PR review loop

Three parallel agents reviewed the diff before commit:

  • Wire-contract audit: ErrorCode StrEnum wire format verified unchanged. Manifest.git_branching missing-default was CRITICAL — fixed by adding default_factory=ManifestGitBranching.
  • Silent-failure audit: assert err.error_code is ErrorCode.QUEUE_JOB_FAILED identity check (HIGH) — fixed to ==. _adopt_existing_manifest propagates FileNotFoundError naturally via load_manifest.
  • Test-coverage audit: service/CLI/integration/E2E test matrix complete. No gaps found beyond what was already planned.

P2-1: Add ErrorCode(StrEnum) to errors.py with 46 typed constants.
All error_code="STRING" literals across 28 source files replaced with
ErrorCode.<MEMBER>. Wire format unchanged (StrEnum is a str subtype).
CI guard scripts/check_error_codes.py wired into 'make check'.
docs/error-codes.md documents semver policy for adding/renaming codes.

P2-2: sync init --adopt-existing idempotently adopts a .keboola/manifest.json
written by the kbc Go CLI without overwriting it. Validates manifest
project_id against the alias token; rejects mismatch with ConfigError
(exit 5). Falls through to normal init when no manifest exists.
Manifest.git_branching gains a default_factory so minimal manifests
without the field parse cleanly.
@ottomansky
ottomansky marked this pull request as ready for review April 22, 2026 12:58
padak
padak previously approved these changes Apr 22, 2026
@padak
padak dismissed their stale review April 22, 2026 21:41

Replacing with English version for broader team visibility.

@padak padak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve with a small follow-up

The refactor to the ErrorCode enum is non-breaking (StrEnum), the AST-based (not regex) guard script is clean, tests + docs are complete. Before merge please also:

Required fix

src/keboola_agent_cli/commands/context.py (~line 95) — the sync init signature is missing --adopt-existing:

kbagent sync init --project ALIAS [--directory DIR] [--git-branching] [--adopt-existing]

kbagent context is the primary reference for AI agents (CONTRIBUTING.md line 200); without this entry the agent won't know the flag exists.

Nice-to-have (follow-up)

  • scripts/migrate_error_codes.py is a one-shot migration script — after merge either delete it, or refactor it to parse errors.py dynamically (otherwise the static VALID_CODES list will drift out of sync with the enum).
  • src/keboola_agent_cli/errors.py:53 + :85 both have a # Storage section comment — merge them into one section.
  • For future PRs: the commit 1b9cefe feat(0.22.0): ErrorCode enum + sync init --adopt-existing mixes two independent units (P2-1 refactor + P2-2 feature). CONTRIBUTING.md requires "One logical change per commit" — split next time into two commits/PRs.

Positives

  • The StrEnum trick preserves wire format → zero breaking change for consumers.
  • The make check AST guard will keep this (and others) from regressing.
  • docs/error-codes.md (46 codes) + semver policy is a great artifact.
  • 218 raise sites migrated cleanly (grep -r 'error_code="' src/ → 0 hits).

@ottomansky

Copy link
Copy Markdown
Contributor Author

Addressed the required fix + both nice-to-haves:

  • docs(context): add --adopt-existing to sync init reference signature33878af.
  • chore: remove one-shot scripts/migrate_error_codes.py8305c5f. Also removed the two dangling references in scripts/check_error_codes.py (docstring + error hint).
  • refactor(errors): merge duplicate # Storage section commentsafb128a — consolidated the stray second block into the first; tests only assert membership/values (not enum order), so safe.

Targeted tests: 23/23 pass. make lint format-check skill-check version-check changelog-check all green.

Noted the meta-feedback on mixed-scope commits — will split into two PRs next time.

@padak
padak changed the base branch from main to release/0.22.0 April 23, 2026 11:56
padak added a commit that referenced this pull request Apr 23, 2026
P2-1: ErrorCode(StrEnum) in errors.py with 49 typed constants (46 original
+ 3 new: JOB_TIMEOUT_TERMINATED, INVALID_FLOW_DAG, SCHEDULE_DELETE_FAILED).
All error_code='STRING' literals across 30+ source files replaced with
ErrorCode.<MEMBER>. Wire format unchanged (StrEnum subclasses str).

P2-2: sync init --adopt-existing idempotently adopts a .keboola/manifest.json
written by the kbc Go CLI without overwriting it. Validates manifest
project_id against the alias token; rejects mismatch with ConfigError.

scripts/check_error_codes.py CI guard rejects new raw literals.
docs/error-codes.md documents semver policy.
@padak

padak commented Apr 23, 2026

Copy link
Copy Markdown
Member

Integrated into release/0.22.0 as squash commit b592d5f (rebased locally on top of v0.21.2 to resolve merge conflicts with main; all Max's review fixes preserved). Original branch fiia/... unchanged.

Full integration PR coming as release/0.22.0 -> main.

@padak padak closed this Apr 23, 2026
padak added a commit that referenced this pull request Apr 23, 2026
- sync-workflow.md: new "Adopting an existing kbc Go CLI checkout" section for sync init --adopt-existing (#201).
- gotchas.md: new entries for exit code 7 (JOB_TIMEOUT_TERMINATED), --poll-strategy fixed, --log-tail-lines N, logTail response semantics (#202); new section for --deny-writes / --deny-destructive (#203); new section for sync init --adopt-existing (#201).
- workspace-workflow.md: new "Orphan detection + garbage collection" section for workspace list --orphaned + workspace gc (#204).
- commands-reference.md: sync init --adopt-existing flag documented.
- README.md: "What it does" table + "All commands" block updated with the 15 new commands shipped in 0.22.0 (flow CRUD, config metadata, workspace GC, storage describe, project pin + firewall, queue polling parity).
ottomansky pushed a commit to ottomansky/keboola-agent-cli that referenced this pull request Apr 30, 2026
…tion

Three PRs' changelog entries were dropped during the git merge --squash
conflict resolution of release/0.22.0:

- keboola#201 ErrorCode enum + sync init --adopt-existing (3 entries)
- keboola#202 queue polling parity (5 entries)
- keboola#205 storage describe-bucket/table/column/batch (6 entries)

Without this fix, 'kbagent changelog' on installed 0.22.0 would show
only 18 of the 32 features shipped in this release. Adding the missing
14 entries before tagging v0.22.0.
@ottomansky
ottomansky deleted the fiia/pr9-errors-and-sync-adopt branch May 11, 2026 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants