fix(0.30.6): sec-20 follow-up -- malformed branch-mapping raises ConfigError (clean envelope)

[padak]

Quick UX cleanup follow-up to v0.30.5 / PR #272.

What v0.30.5 shipped (sec-20)

load_branch_mapping() raises a descriptive error with file path + bad value when branch-mapping.json contains a malformed branch ID:

Failed to parse /home/user/repo/.keboola/branch-mapping.json: Invalid branch ID in branch-mapping.json: 'not-a-number'. Expected null or an integer; got str.

That message is correct. The error was raised as a bare ValueError.

What v0.30.5 missed

CLI commands wrap their service calls in except ConfigError (mapped to exit 5 + clean JSON envelope) and except KeboolaApiError. Plain ValueError is not caught — so on a malformed mapping file, the user actually sees:

╭───────── Traceback (most recent call last) ─────────╮
│ /Users/.../branch_mapping.py:33 in _coerce_keboola_id │
│  ❱  33 │   │   return int(raw)                      │
│ ValueError: invalid literal for int() with base 10  │
... 4 more frames ...
│ ValueError: Failed to parse /.../branch-mapping.json│
│ : Invalid branch ID in branch-mapping.json: ...     │
╰─────────────────────────────────────────────────────╯
EXIT=1

Found during the v0.30.5 e2e smoke test against the kbagent-e2e project. Not a security regression — the message is correct — but the multi-frame traceback masks the actual error and exits 1 (general error) instead of 5 (config error).

Fix

load_branch_mapping() now raises ConfigError directly with the same descriptive message. Existing CLI except ConfigError handlers catch it and emit the standard envelope:

{
  "status": "error",
  "error": {
    "code": "CONFIG_ERROR",
    "error_type": "configuration",
    "message": "Failed to parse /.../branch-mapping.json: Invalid branch ID in branch-mapping.json: 'not-a-number'. Expected null or an integer; got str.",
    "project": "",
    "retryable": false
  }
}

Exit 5. Zero traceback lines.

Scope

• BranchMapping.from_dict() — unchanged. Still raises ValueError. It's the data-parser layer; raising the generic Python exception is correct there.
• load_branch_mapping() — changed. Wraps the inner ValueError and re-raises as ConfigError. This is the filesystem-aware boundary, so it has the path context to make a "ConfigError"-class error meaningful.
• cleanup_branch_id_from_mapping() — extended to catch ConfigError alongside the legacy ValueError so its best-effort skip behavior on a corrupted workspace mapping is preserved.

Test

Updated regression test from v0.30.5:

def test_load_branch_mapping_invalid_id_raises_config_error(self, tmp_path: Path) -> None:
    ...
    with pytest.raises(ConfigError, match=r"Failed to parse .*branch-mapping\.json"):
        load_branch_mapping(tmp_path)

E2E verification (preserved in /tmp/kbagent-e2e-logs/sec20-cleanenv-*.log):

=== sec-20 with new fix: error must be clean JSON, no traceback ===
{
  "status": "error",
  "error": {
    "code": "CONFIG_ERROR",
    "message": "Failed to parse /private/tmp/.../branch-mapping.json: Invalid branch ID ...",
    ...
  }
}
EXIT=5

Traceback lines: 0
Exit code: 5
✓ sec-20 clean envelope VERIFIED

Test plan

• make lint format-check skill-check changelog-check check-error-codes
• pytest tests/ --ignore=... → 2834 passed
• E2E sec-20 repro: descriptive error in clean JSON envelope, exit 5, no traceback

Why a separate PR vs. amending #272

• fix(0.30.5): close issue #269 -- 9 verified findings from security audit #272 already merged; this is a cleanup found post-merge during smoke test
• 7-file diff (+30/-13) is genuinely tiny and orthogonal to the 9 security fixes in fix(0.30.5): close issue #269 -- 9 verified findings from security audit #272
• Easier to follow review history this way

Related

• Closes the smoke-test follow-up from Security audit: 9 verified findings (path traversal, env leak, file perms, XSS, etc.) #269 (security audit)
• Predecessor: fix(0.30.5): close issue #269 -- 9 verified findings from security audit #272 (v0.30.5)

[padak]

Review of #273 — fix(0.30.6): sec-20 follow-up -- malformed branch-mapping raises ConfigError (clean envelope)

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

The PR converts ValueError to ConfigError in load_branch_mapping() so that CLI commands with except ConfigError handlers catch it and emit a clean exit-5 JSON envelope instead of a Python traceback. The boundary choice is correct: BranchMapping.from_dict() (data parser) continues to raise ValueError, while load_branch_mapping() (filesystem-aware wrapper) converts it. One gap was found: sync_branch_status in commands/sync.py only catches FileNotFoundError and would still surface a traceback for a corrupted mapping file. The dead-code comment on the ValueError catch in cleanup_branch_id_from_mapping is mildly misleading. All other affected call sites are safe. make check passes (2834 passed, 7 skipped). Verdict: REQUEST CHANGES (one blocking finding).

Verdict

• Verdict: REQUEST CHANGES
• Blocking findings: 1
• Non-blocking findings: 1
• Nits: 1

Blocking findings

[B-1] src/keboola_agent_cli/commands/sync.py:992-996 — sync branch-status does not catch ConfigError, nullifying the fix for that command

sync_branch_status (line 982) wraps its service call in try: ... except FileNotFoundError only. SyncService.branch_status() calls load_branch_mapping() at line 2087 inside a try: ... except FileNotFoundError block too; a ConfigError from a malformed file escapes that inner handler and propagates up to the command, which has no ConfigError catch. The user still sees an unhandled exception traceback when running kbagent sync branch-status against a workspace with a corrupted branch-mapping.json, defeating the purpose of this PR for that command.

Fix: add except ConfigError as exc: formatter.error(...); raise typer.Exit(code=5) from None to sync_branch_status in commands/sync.py, mirroring the pattern used in sync_branch_link (line 926) and sync_branch_unlink (line 967). All other call sites (branch_link, branch_unlink, sync_pull, sync_push, _resolve_branch_id) already have upstream except ConfigError coverage.

Non-blocking findings

[NB-1] src/keboola_agent_cli/sync/branch_mapping.py:159 — ValueError catch in cleanup_branch_id_from_mapping is now dead code with a misleading justification comment

The comment says "ValueError preserved for backward compat with callers that may still trigger it via custom paths." After this PR, load_branch_mapping() wraps every ValueError as ConfigError before returning, so no ValueError can reach line 159 from load_branch_mapping(). save_branch_mapping() and find_sync_workspace() (the other functions callable from the same scope) do not raise ValueError. The dead catch is harmless at runtime, but the comment implies a real code path exists when it does not, which will mislead the next contributor.

Fix: either drop the ValueError from the tuple (making it except (FileNotFoundError, ConfigError)) and update the comment, or change the comment to accurately say "ValueError is a defensive catch; no current code path in load_branch_mapping() produces it after 0.30.6."

Nits

• [NIT-1] src/keboola_agent_cli/changelog.py:47-48 — The two changelog entries for 0.30.6 are extremely long (828 and 440 characters). The repo's existing style (see the 0.30.5 entries) is also verbose, so this is consistent, but the 0.30.6 entries read more like commit messages than user-facing release notes. Consider trimming the second entry ("Tests: ...") to a single short sentence -- or dropping it entirely since test changes are not user-facing.

Verification log

• gh pr view 273 --json title,body,files,additions,deletions,state -> 7 files, +30/-13, state OPEN, conventional fix(0.30.6): prefix matches bug-fix nature of change. OK.
• git rev-parse --abbrev-ref HEAD -> fix/sec-20-clean-error-envelope (matches PR branch). OK.
• grep -E 'from typer|import typer|formatter\.|console\.print' /tmp/kbagent-pr-273.diff -> empty. No layer violation.
• grep -E 'from httpx|import httpx' /tmp/kbagent-pr-273.diff -> empty. No HTTP in commands.
• grep -E 'except\s*:' /tmp/kbagent-pr-273.diff -> empty. No bare except.
• grep -E 'print\(' /tmp/kbagent-pr-273.diff | grep src/ -> empty. No raw print in production code.
• grep -E '(token|TOKEN)' /tmp/kbagent-pr-273.diff | grep -v mask_token -> empty. No token exposure.
• Layer boundary check: BranchMapping.from_dict() still raises ValueError (data-parser, correct). load_branch_mapping() converts to ConfigError (filesystem boundary, correct). _coerce_keboola_id() still raises ValueError (internal helper, correct).
• cleanup_branch_id_from_mapping() caller check: catches (FileNotFoundError, ConfigError, ValueError). ConfigError catches the new exception shape. Dead ValueError confirmed (no load_branch_mapping code path emits it post-fix).
• Call-site audit for all load_branch_mapping() callers in sync_service.py:
  • Line 1958 (branch_link): except FileNotFoundError + CLI except ConfigError at command line 926. ConfigError propagates safely. OK.
  • Line 2053 (branch_unlink): no local try/except; CLI except ConfigError at command line 967 catches it. OK.
  • Line 2087 (branch_status): except FileNotFoundError only. CLI sync_branch_status at line 994 catches only FileNotFoundError. ConfigError escapes uncaught. GAP -> [B-1].
  • Line 2146 (_resolve_branch_id): except FileNotFoundError only; callers are pull/push/pull_all/push_all which all propagate to CLI commands that do catch ConfigError (lines 522, 848). OK.
• make check -> 2834 passed, 7 skipped, 73 deselected, 14 warnings (exit 0). OK.
• ConfigError inheritance: class ConfigError(Exception) -- NOT a subclass of ValueError. Existing code catching ValueError from load_branch_mapping would miss the new exception. No such external callers found in src/ (only cleanup_branch_id_from_mapping explicitly catches both). OK.
• Plugin sync surfaces: no new command added; only a version bump. pyproject.toml, plugin.json, marketplace.json, changelog.py all updated. No keboola-expert.md, commands-reference.md, or gotchas.md update required (error-envelope shape is an internal UX fix, not an AI-agent-observable behavior change).
• Behavior reproduction: could not reproduce against live environment (no E2E credentials in this session). PR describes E2E log output confirming exit-5 + zero traceback lines; claimed smoke-test result accepted without live re-run.

Open questions for the author

• Is sync branch-status with a corrupted mapping file intentionally left as best-effort (return "not linked") rather than error? If so, the fix belongs in the service layer (branch_status should catch ConfigError from load_branch_mapping and treat it like FileNotFoundError). If the intent is to surface the error to the user, the fix belongs in the command layer. Either approach resolves [B-1]; the choice depends on whether a corrupted mapping during a read-only status query should silently degrade or loudly fail.

[padak]

Thanks for catching this. Pushed ca94598 addressing the blocking finding.

What was missing

You're right: sync_branch_status only caught FileNotFoundError. After PR #273's ValueError → ConfigError conversion at the load_branch_mapping() boundary, a corrupted .keboola/branch-mapping.json still produced a Python traceback when the user reached for the diagnostic command — which is exactly the worst time for that to happen.

All other sync commands (sync_pull, sync_push, sync_diff, sync_branch_link, sync_branch_unlink) already had the except ConfigError handler. sync_branch_status was the lone outlier.

Fix

commands/sync.py:992-996 now also catches ConfigError:

try:
    result = service.branch_status(project_root=project_root)
except FileNotFoundError as exc:
    formatter.error(message=str(exc), error_code=ErrorCode.NOT_INITIALIZED)
    raise typer.Exit(code=1) from None
except ConfigError as exc:
    # Corrupted .keboola/branch-mapping.json -- surface as clean
    # exit-5 envelope so the user sees the descriptive message
    # ("Failed to parse ...: Invalid branch ID ...") instead of a
    # Python traceback. Mirrors the handler other sync commands
    # already have (issue #269 sec-20 follow-up).
    formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR)
    raise typer.Exit(code=5) from None

Test

Added test_sync_branch_status_corrupted_mapping_clean_envelope to tests/test_sync_cli.py:TestSyncBranchStatusCli. Mocks the service raising ConfigError, asserts:

• Exit code 5
• "Traceback" not in output
• JSON payload error.code == "CONFIG_ERROR"
• Message contains "Invalid branch ID"

E2E re-verified against /tmp/kbagent-e2e/sync-sec20-branchstatus:

=== sync branch-status against corrupted mapping ===
{
  "status": "error",
  "error": {
    "code": "CONFIG_ERROR",
    "message": "Failed to parse /private/tmp/.../branch-mapping.json: Invalid branch ID ...",
    ...
  }
}
EXIT=5
Traceback lines: 0
✓ branch-status clean envelope VERIFIED

NB-1 / NIT-1

Both already validated as not affecting safety:

• NB-1 (branch_link/branch_unlink ConfigError handling): both commands already had except ConfigError at the CLI layer (lines 926, 967). Fix is end-to-end clean.
• NIT-1 (changelog wording): "UX cleanup" framing in changelog is accurate per the original sec-20 fix being correct in content but missing the envelope wrapper. No change needed.

Total suite: 2835 passed (was 2834; +1 new test). Lint + format clean. Re-review when convenient.

[padak]

Review of #273 — fix(0.30.6): sec-20 follow-up -- malformed branch-mapping raises ConfigError (clean envelope)

Generated by kbagent-pr-reviewer subagent (re-review after commit ca94598). Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR fixes the UX gap left by v0.30.5: load_branch_mapping() now raises ConfigError instead of bare ValueError when branch-mapping.json is malformed, so all existing except ConfigError handlers in commands/sync.py produce a clean exit-5 JSON envelope instead of a Python traceback. The previously blocking finding (B-1 from the first review pass) has been addressed in commit ca94598, which adds the missing except ConfigError clause to sync_branch_status and a regression test that exercises the new code path end-to-end via CliRunner. The diff is small, targeted, and mechanically correct.

Verdict: APPROVE. Zero blocking findings. One non-blocking observation about a comment that is now technically misleading (the ValueError arm in cleanup_branch_id_from_mapping's except tuple is unreachable via load_branch_mapping itself, but is harmless). One NIT on changelog verbosity.

Verdict

• Verdict: APPROVE
• Blocking findings: 0
• Non-blocking findings: 1
• Nits: 1

Blocking findings

(none)

Non-blocking findings

[NB-1] src/keboola_agent_cli/sync/branch_mapping.py:159 — ValueError arm in cleanup_branch_id_from_mapping except-tuple is now unreachable via load_branch_mapping

The comment on line 160-162 justifies keeping ValueError in except (FileNotFoundError, ConfigError, ValueError) as "backward compat with callers that may still trigger it via custom paths." However, load_branch_mapping is the only function called inside that try block, and after this PR it converts every ValueError from _coerce_keboola_id / json.loads into ConfigError before it exits. No other call-site invokes load_branch_mapping with a path that could produce a raw ValueError. The ValueError arm is therefore dead code in all current production paths.

This is harmless -- an extra arm in an except tuple has zero runtime cost and no behavior change -- but the accompanying comment is now misleading because it implies an active code path. If a future refactor ever removes the ConfigError conversion in load_branch_mapping, a reviewer might trust the comment and assume ValueError is still covered, leading to confusion.

Fix (optional): either drop ValueError from the tuple (and the comment) or strengthen the comment: "ValueError is dead code here since load_branch_mapping no longer propagates it, but kept so that any future direct call to BranchMapping.from_dict (the data-parser layer, which does still raise ValueError) would also be silently swallowed."

Nits

• [NIT-1] src/keboola_agent_cli/changelog.py:12-13 — The two changelog strings for 0.30.6 are unusually long (the second entry is ~370 chars). The changelog.py docstring says "brief one-line descriptions." Both entries read more like a commit message than a user-facing changelog bullet. Consider collapsing into one concise entry like: "UX (sec-20): malformed .keboola/branch-mapping.json now shows a clean JSON error (exit 5, CONFIG_ERROR) instead of a Python traceback. sync_branch_status added the missing ConfigError handler; load_branch_mapping() raises ConfigError instead of bare ValueError." Cosmetic only.

Verification log

• gh auth status → authenticated as padak on github.com ✓
• git rev-parse --abbrev-ref HEAD → fix/sec-20-clean-error-envelope (matches <branch>) ✓
• gh pr view 273 --json state → "OPEN" ✓; additions: 79, deletions: 13, 9 files touched ✓
• gh pr diff 273 → 225-line diff captured to /tmp/kbagent-pr-273.diff ✓
• Read CONTRIBUTING.md Plugin synchronization map: this PR introduces no new CLI commands (no new @*_app.command() decorators), bumps only pyproject.toml/plugin.json/marketplace.json version, and updates changelog and two test files. No plugin-sync-map rows are applicable except the release-time ones (version bump + changelog). Both are updated: pyproject.toml → 0.30.6, changelog.py has entry, plugin.json and marketplace.json auto-synced. ✓
• grep -E '^\+(from typer|import typer|...)' diff | grep services/ → empty ✓ (no layer violations)
• grep -E '^\+(from httpx|import httpx|...)' diff | grep commands/ → empty ✓
• B-1 closed: commands/sync.py:997-1004 now has except ConfigError as exc: formatter.error(...); raise typer.Exit(code=5) immediately after the FileNotFoundError handler in sync_branch_status. Verified: all other sync commands (sync_pull line 522, sync_push line 848, sync_branch_link line 926, sync_branch_unlink line 967) already had equivalent handlers pre-patch. ✓
• New test exercises the code path: test_sync_branch_status_corrupted_mapping_clean_envelope sets mock_sync.branch_status.side_effect = ConfigError(...), invokes ["--json", "sync", "branch-status", ...] via CliRunner, and asserts exit_code == 5, "Traceback" not in output, data["error"]["code"] == "CONFIG_ERROR", "Invalid branch ID" in data["error"]["message"]. The mock injects ConfigError directly at the service boundary, which is the correct layer to test the CLI handler in isolation. ✓
• No regression in other sync_branch_ commands*: branch_link, branch_unlink, and branch_status service methods call load_branch_mapping catching only FileNotFoundError; ConfigError propagates to the command layer where all three have matching handlers. _resolve_branch_id (used by sync_pull/sync_push) also only catches FileNotFoundError from load_branch_mapping; ConfigError propagates to the pull/push handlers which already have it. No handler gap found. ✓
• BranchMapping.from_dict still raises ValueError: confirmed at branch_mapping.py:36. No external production code calls from_dict directly (only load_branch_mapping does, which wraps it). Test test_branch_mapping_from_dict_invalid_id_descriptive_error (line 176) still asserts ValueError from the data-parser layer. ✓
• Convention checks: no magic numbers, no raw error_code= string literals, no bare except:, no print() in prod code, no token patterns in new log output. ✓ (all grep checks empty)
• make check → 2835 passed, 7 skipped, 73 deselected, 15 warnings — exit 0 ✓
• Behavior reproduction: Could not re-run the full e2e smoke (no E2E_API_TOKEN available in this session). The author's E2E log excerpt in the PR description shows EXIT=5, "code": "CONFIG_ERROR", Traceback lines: 0. The unit test provides adequate coverage for the specific new handler; marking as confirmed-by-test rather than confirmed-by-e2e.

Open questions for the author

(none)