feat: kbc→kbagent CI/CD migration skill + sync CLI behavior tests

[Matovidlo]

Summary

• Adds the kbagent-cicd-migration skill (plugins/kbagent/skills/kbagent-cicd-migration/): a guided, evidence-based runbook plus a stdlib-only generator (scripts/migrate_cicd.py) that migrates a kbc (keboola-as-code) project-as-code GitHub CI/CD pipeline to the new kbagent sync engine.
  • The generator discovers every project from .keboola/manifest.json, detects the legacy kbc workflows/actions it supersedes, and emits clean kbagent-native validate / pull / push GitHub workflows using uv tool install + kbagent sync with the KBAGENT_PROJECT_FROM_ENV=1 auth model (per-project token secrets, GitHub-Environment approval gating, no committed tokens).
  • References cover the kbc↔kbagent command/flag/env mapping, GitHub secrets/environments setup, the one-time breaking-conversion runbook, and the single-branch vs git-branching decision guide.
  • Each generated project step reuses the customer's own discovered directory path verbatim (--directory '<discovered-dir>'), so existing multi-project folder conventions (e.g. <project_id> folders) are preserved rather than re-derived or renamed -- confirmed in code, not just documented.
• Adds sync CLI behavior tests (tests/test_sync_cli_behavior.py): pin the project-selection guards that define kbagent's orchestrator model (--project/--all-projects required and mutually exclusive, --branch is per-project) and that push --dry-run propagates the dry-run flag without writing.

Why

Customers run kbc-based GitHub CI/CD (per-project pull/push PRs, multi-project promotion). The new CLI's sync is a manifest-compatible successor but not a drop-in: the on-disk format differs (config.json/meta.json → _config.yml) and sync is an orchestrator over registered project aliases, not cwd-per-folder. This skill gives a safe, repeatable cutover, and the tests lock the guard behaviors that prevent wrong-target or whole-tree operations.

Validated live against a real project: native pull round-trips to zero drift; the adopt-existing → diff "136 to delete" footgun and the orphaned-config.json cleanup are documented in the runbook.

Review pass (code quality + OWASP/security, independent agents)

Security review found no material issues: secret-handling guidance is consistently correct throughout (GitHub Environment-scoped secrets, never plaintext, explicit warning against --allow-plaintext-on-encrypt-failure in CI, generator only ever prints secret names never values), and the test file uses an obviously-fake token with everything else mocked -- no real credentials, no eval/shell=True.

Quality review found no blockers -- verdict was "ready for human review as-is" -- but three minor/nit items, all fixed in a follow-up commit:

• A zero-logic _install_steps_placeholder() wrapper that just returned a module constant -- deleted, call sites reference the constant directly (ponytail: one less indirection layer for no benefit).
• The test module's docstring claimed --project/--all-projects mutual-exclusion and --branch+--all-projects conflict guards were pinned for pull/diff/push alike, but only pull is actually exercised (the identical guard is duplicated in all three per commands/sync.py) -- docstring narrowed to say so accurately instead of overclaiming coverage.
• A test helper returned a bare 2-tuple of semantically distinct values (exit code, mock) -- replaced with a NamedTuple per this repo's "no new bare-tuple returns" convention.

Change type

Feature — migration tooling + tests. No source/CLI-command changes, no version bump.

Impact analysis

• New files only: one skill tree under plugins/kbagent/skills/ and one test file under tests/.
• No changes to src/, no new CLI commands, no public API or behavior change.
• Fully backwards-compatible.

Test plan

• tests/test_sync_cli_behavior.py — 6 tests, green.
• Full make check (ruff, format, ty, skill-check, version-check, command-sync-check, changelog-check, check-error-codes, check-sentinel-guards, loc-check, full pytest suite): 5411 passed, 11 skipped, 0 failed.
• Generator exercised against the CLI-based-sync-demo repo (2 projects discovered, 3 legacy actions flagged, 3 valid workflows generated).

Deployment

Merge & automatic deploy. No migration.

Rollback plan

Revert of this PR.

🤖 Generated with Claude Code

[Matovidlo]

@claude review

[Copilot]

Pull request overview

This PR adds a new kbagent CI/CD migration skill (docs + a stdlib-only workflow generator) intended to help customers migrate legacy kbc GitHub Actions pipelines to kbagent sync, and it adds behavioral CLI tests to lock down key sync safety guards (explicit project selection and dry-run propagation).

Changes:

• Added kbagent-cicd-migration skill documentation + reference materials under plugins/kbagent/skills/.
• Added scripts/migrate_cicd.py generator to detect legacy kbc workflows and emit kbagent-native validate/pull/push workflows.
• Added tests/test_sync_cli_behavior.py to pin CLI guard behavior for project selection and push --dry-run.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
tests/test_sync_cli_behavior.py	Adds CLI behavior regression tests for project selection guards and dry-run propagation.
plugins/kbagent/skills/kbagent-cicd-migration/SKILL.md	Migration skill runbook explaining the breaking JSON→YAML conversion and CI/CD cutover steps.
plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py	Analyzer + generator that discovers projects and emits GitHub Actions workflows + secrets checklist.
plugins/kbagent/skills/kbagent-cicd-migration/references/secrets-setup.md	Documents GitHub secrets/environments setup for the generated workflows.
plugins/kbagent/skills/kbagent-cicd-migration/references/migration-runbook.md	Ordered PR sequence / operational runbook for a safe migration.
plugins/kbagent/skills/kbagent-cicd-migration/references/command-mapping.md	Reference mapping between kbc and kbagent commands/flags/env vars.
plugins/kbagent/skills/kbagent-cicd-migration/references/branching-model.md	Decision guide for single-branch vs git-branching workflow models.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (6)

plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py:321

• _guess_repo_slug fails to parse repo names containing dots because the regex excludes . in the repo segment, which can produce an incorrect --repo <owner>/<repo> in the generated gh commands. Capture the full slug and strip an optional .git suffix instead.

    if config.exists():
        m = re.search(r"github\.com[:/]([^/]+/[^/\s.]+)", config.read_text(errors="ignore"))
        if m:
            return m.group(1)

plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py:104

• discover_projects accepts manifests with missing project.id / project.apiHost, which later yields invalid workflow env (e.g. KBC_STORAGE_API_URL: https://) and misleading secrets output. Treat these fields as required and skip or error on invalid manifests.

        proj = data.get("project", {})
        projects.append(
            Project(
                alias=_alias_from_dir(rel),
                directory=rel,

plugins/kbagent/skills/kbagent-cicd-migration/SKILL.md:33

• The code reference constants.py:425 is incorrect (it currently points at unrelated OTLP constants), which undermines the “verified against the code” claim. Prefer stable file/symbol references (or just file paths) instead of brittle line numbers.

   - `kbagent` writes per config: **`_config.yml`** (YAML, with `name`/`description`/
     `parameters` hoisted + a `_configuration_extra` block) + extracted code files
     (`constants.py:425`, `sync/config_format.py`).
   - The first `kbagent sync pull` therefore **rewrites every configuration** into a

plugins/kbagent/skills/kbagent-cicd-migration/references/command-mapping.md:34

• The file/line references here are stale: constants.py:163 and config_store.py:193 don’t correspond to the env-injection mechanism. Use stable symbol/file references (e.g., constants.py ENV_PROJECT_FROM_ENV and ConfigStore._inject_env_project) rather than line numbers.

| *(implicit)* | `KBAGENT_PROJECT_FROM_ENV=1` | **Required** opt-in so kbagent synthesizes an ephemeral project from the env in CI (no `config.json` on disk). See `constants.py:163`, `config_store.py:193` |

plugins/kbagent/skills/kbagent-cicd-migration/references/secrets-setup.md:50

• The .gitignore reference is incorrect: config_store.py:359 is unrelated to gitignore creation. Point to the actual helper (or drop the line number) so readers can verify the claim reliably.

- Do **not** commit `.kbagent/config.json` with tokens (the new CLI auto-writes a
  `.gitignore` for its config dir — `config_store.py:359`).
- Do **not** pass `--allow-plaintext-on-encrypt-failure` in CI.

plugins/kbagent/skills/kbagent-cicd-migration/SKILL.md:40

• The parenthetical reference sync.py:67,495 appears to be stale and doesn’t point to the sync guardrails described here. Removing the line numbers (or pointing at the correct module) will avoid misleading users as the code moves.

2. **kbagent sync is an ORCHESTRATOR, not cwd-per-folder.** `kbc pull` runs against
   whatever directory you `cd` into. `kbagent sync pull` *requires* `--project ALIAS`
   (resolved from a central config store) or `--all-projects` (`sync.py:67,495`). In
   CI we bridge this with env-injection: `KBAGENT_PROJECT_FROM_ENV=1` synthesizes a
   project under the reserved alias `__env__`, and every command passes

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py:149

• The workflow generator pins uv tool install to the legacy distribution name keboola-agent-cli. In this repo, end-user installation and self-update are based on the current keboola-cli distribution name, so generated CI can end up installing the wrong package (or fail if the legacy name is not published for the requested version).

    elif version:
        spec = f"keboola-agent-cli=={version}"
    else:
        # Unpinned: only acceptable for non-production lanes. The skill warns.
        spec = "keboola-agent-cli"

plugins/kbagent/skills/kbagent-cicd-migration/references/migration-runbook.md:21

• The version pin guidance references keboola-agent-cli==X.Y.Z, but the repo’s current distribution name used for installs/updates is keboola-cli. Aligning this avoids copy/paste installs that target the legacy package name.

- [ ] **Pick a kbagent version** and pin it (`keboola-agent-cli==X.Y.Z` or
      `git+...@vX.Y.Z`). Never unpinned on a prod lane.

plugins/kbagent/skills/kbagent-cicd-migration/references/command-mapping.md:10

• The install mapping row uses keboola-agent-cli==<ver>, but this repo’s documented install/update distribution name is keboola-cli. Using the legacy name here can break reproducible installs or pin the wrong artifact.

| Download Go binary zip from `keboola/keboola-as-code` GitHub release, unzip to `/usr/local/bin/kbc` | `uv tool install keboola-agent-cli==<ver>` (PyPI) or `uv tool install 'git+https://github.com/keboola/cli@<tag>'` |

plugins/kbagent/skills/kbagent-cicd-migration/SKILL.md:108

• This install example uses keboola-agent-cli==<ver>, but the repo’s installation docs and current distribution name are keboola-cli. Using the legacy name here can lead to users pinning/installing the wrong package.

  `uv tool install keboola-agent-cli==<ver>` (see Step 2 for version pin) or a

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (5)

plugins/kbagent/skills/kbagent-cicd-migration/SKILL.md:192

• This example command also hardcodes --version 0.58.0. To keep the runbook evergreen, use X.Y.Z (or reference “current stable version”) instead of a specific historical version.

python <skill_dir>/scripts/migrate_cicd.py /path/to/repo --write \
    --version 0.58.0 --main-branch main --schedule "0 * * * *"

**tests/test_sync_cli_behavior.py:83**
* Similarly, `sync push` has both `push` and `push_all` code paths. Adding an assertion that `push_all` wasn’t called makes this guard test fully cover the “must not reach the service” intent.

def test_push_without_project_is_usage_error(self, tmp_path: Path) -> None:
    code, mock = _invoke(["sync", "push", "--directory", str(tmp_path)], tmp_path)
    assert code == 2
    mock.push.assert_not_called()

**plugins/kbagent/skills/kbagent-cicd-migration/SKILL.md:183**
* The migration guide hardcodes version `0.58.0` and states “PyPI, once published”, but this repo is already at 0.80.0. Using a stale example version can mislead users into pinning an unnecessarily old release (or thinking PyPI isn’t available). Prefer a placeholder (X.Y.Z) and optionally mention the current release as an example.

This issue also appears on line 190 of the same file.

• Pinned (recommended for prod lanes): --version 0.58.0 (PyPI, once published)
  or --git-ref v0.58.0 (git tag, until PyPI exists). Reproducible CI.

**plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py:35**
* The script docstring also hardcodes `0.58.0` / `v0.58.0` and mentions “no PyPI release yet”, which is likely to go stale. Use placeholders (`X.Y.Z` / `vX.Y.Z`) so copy/paste guidance stays accurate across releases.

python migrate_cicd.py <repo_dir> [--write] \\
    [--version 0.58.0 | --git-ref vX.Y.Z] \\
    [--main-branch main] [--schedule "0 * * * *"]

Examples:
# Inspect what would change (no writes):
python migrate_cicd.py ../CLI-based-sync-demo

# Generate workflows pinned to a published PyPI version:
python migrate_cicd.py ../CLI-based-sync-demo --write --version 0.58.0

# Pin to a git tag instead (no PyPI release yet):
python migrate_cicd.py ../CLI-based-sync-demo --write --git-ref v0.58.0

**tests/test_sync_cli_behavior.py:78**
* This test asserts the service method `diff` wasn’t called, but `sync diff` also has an `--all-projects` path that calls `diff_all`. Since the docstring says usage errors must not reach the service, it’s worth asserting neither `diff` nor `diff_all` was invoked so a regression can’t slip through by accidentally defaulting to the bulk path.

This issue also appears on line 79 of the same file.

def test_diff_without_project_is_usage_error(self, tmp_path: Path) -> None:
    code, mock = _invoke(["sync", "diff", "--directory", str(tmp_path)], tmp_path)
    assert code == 2
    mock.diff.assert_not_called()

</details>

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (5)

tests/test_sync_cli_behavior.py:142

• This invocation also benefits from an explicit --config-dir so the CLI cannot resolve a real on-disk config via CWD traversal. (--config-dir is a global option, so it needs to be placed before the sync subcommand.)

            result = runner.invoke(
                app,
                [
                    "--json",
                    "sync",
                    "push",
                    "--project",
                    "prod",
                    "--dry-run",
                    "--directory",
                    str(tmp_path),
                ],
            )

plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py:221

• main_branch is unused in gen_pull as well. Rename it to _main_branch (or remove it and adjust call sites) so the interface doesn’t imply the output depends on this parameter.

def gen_pull(projects: list[Project], main_branch: str, schedule: str | None) -> str:

tests/test_sync_cli_behavior.py:62

• To keep these CLI tests hermetic and avoid accidentally picking up a developer’s real local/global config (via resolve_config_dir walking up from CWD), pass an explicit --config-dir pointing at the tmp config directory in the invocation. This reduces flakiness and makes the test intent clearer (everything should run against the temp store).

This issue also appears on line 130 of the same file.

def _invoke(args: list[str], tmp_path: Path) -> tuple[int, MagicMock]:
    """Invoke the CLI with a mocked SyncService; return (exit_code, mock)."""
    store = _store(tmp_path / "config")
    mock_sync = MagicMock()
    with (
        patch("keboola_agent_cli.cli.ConfigStore") as MockStore,
        patch("keboola_agent_cli.cli.ProjectService") as MockProj,
        patch("keboola_agent_cli.cli.SyncService") as MockSync,
    ):
        MockStore.return_value = store
        MockProj.return_value = ProjectService(config_store=store)
        MockSync.return_value = mock_sync
        result = runner.invoke(app, args)
    return result.exit_code, mock_sync

plugins/kbagent/skills/kbagent-cicd-migration/SKILL.md:40

• The inline code reference sync.py:67,495 is brittle and already out of date as line numbers drift; it makes future verification harder because readers may not find the referenced logic. Prefer a stable file reference (optionally with the symbol/command names) instead of line numbers.

2. **kbagent sync is an ORCHESTRATOR, not cwd-per-folder.** `kbc pull` runs against
   whatever directory you `cd` into. `kbagent sync pull` *requires* `--project ALIAS`
   (resolved from a central config store) or `--all-projects` (`sync.py:67,495`). In
   CI we bridge this with env-injection: `KBAGENT_PROJECT_FROM_ENV=1` synthesizes a
   project under the reserved alias `__env__`, and every command passes

plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py:192

• main_branch is unused in gen_validate, which makes the signature misleading and may trigger unused-argument lint in stricter configurations. If it’s intentionally unused, rename it to _main_branch to document that and silence unused-arg checks.

This issue also appears on line 221 of the same file.

def gen_validate(projects: list[Project], main_branch: str) -> str:

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

plugins/kbagent/skills/kbagent-cicd-migration/references/command-mapping.md:24

• The command mapping table documents sync pull/push/diff without --project/--all-projects, but the CLI enforces that one of those selectors must be provided (see src/keboola_agent_cli/commands/sync.py, which exits with usage error when neither is set). As written, these rows will fail if copy/pasted and also contradict the skill’s own “orchestrator” guardrail narrative.

| `kbc pull -d DIR --force` | `kbagent sync pull --directory DIR --force` | `--force` overrides local-vs-remote conflicts (3-way diff) |
| `kbc push -d DIR` | `kbagent sync push --directory DIR` | Encrypts `#`-secrets fail-closed before write |
| `kbc push -d DIR --force` | `kbagent sync push --directory DIR --force` | Push's `--force` removes remote configs deleted locally (there is no `--allow-delete` flag — same flag name as pull's `--force`, but a different meaning per command) |
| `kbc push --dry-run` / push-dry action | `kbagent sync push --dry-run --directory DIR` | Shows planned changes without writing |
| `kbc diff -d DIR` | `kbagent [--json] sync diff --directory DIR` | `--json` is a **global** option (before `sync`, not after `diff`); gives structured drift for CI gating |

[Matovidlo]

@claude review

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (2)

plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py:362

• allowedBranches is parsed into Project.allowed_branches but never surfaced in the analyzer output or used in generation, even though the module docstring claims it is read to surface scoping. Either print it in the discovery report (to make the scoping explicit) or remove the unused field/parse to avoid dead data.

    for p in projects:
        subset = ""
        if p.ignored_components:
            subset = f"  [subset: {len(p.ignored_components)} ignored component(s)]"
        print(

plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py:189

• The generated workflow uses single quotes around the discovered directory (--directory '{p.directory}'). If a repo folder name contains a single quote (or other shell-sensitive characters), this produces an invalid shell token and the generated CI step will fail. Use shell-safe quoting (e.g., shlex.quote) when rendering the directory argument.

        f"          KBC_STORAGE_API_URL: {p.stack_url}\n"
        "        run: |\n"
        f"          {prefix}sync {command} --project __env__ --directory '{p.directory}'\n"

[padak]

Review of #402 — feat: kbc→kbagent CI/CD migration skill + sync CLI behavior tests

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/pytest, not duplicated here.

Summary

This PR adds a new, self-contained Claude Code skill (plugins/kbagent/skills/kbagent-cicd-migration/) that guides a customer through migrating a kbc-based GitHub CI/CD pipeline to kbagent sync, plus a stdlib-only generator script (migrate_cicd.py) that emits kbagent-native GitHub Actions workflows, and 6 new CLI-behavior tests pinning sync pull/diff/push project-selection guards. It touches no src/ code and adds no CLI commands, so most of the Plugin synchronization map (CLI-surface rows) does not apply. The reference content is unusually well fact-checked against current main (v0.80.0) — every spot-checked claim (never_fetched diff semantics, COMPONENT_TYPE_MAP, sync/manifest.py field names, sync push --force semantics, "Manifest already exists" error text, ENV_PROJECT_FROM_ENV) verified correct against the live source. However, reproducing the generator against a synthetic fixture repo surfaced a real shell-injection vulnerability in the generated CI YAML (directory names are interpolated unescaped into a single-quoted run: string) and a secrets/environment-scoping inconsistency between secrets-setup.md's guidance and the generated validate workflow. Verdict: REQUEST CHANGES on the injection finding; everything else is solid.

Verdict

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

Blocking findings

[B-1] plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py:167-189 (_project_step) — unescaped shell interpolation of p.directory in generated CI

_project_step() builds each generated workflow's run: line as
f" {prefix}sync {command} --project __env__ --directory '{p.directory}'\n".
p.directory comes straight from a real on-disk folder name discovered via
repo.glob("**/.keboola/manifest.json") (discover_projects, line 92) with no
sanitization. Reproduced live: a project directory named
foo'; touch pwned; echo ' renders as
kbagent sync pull --force --project __env__ --directory 'foo'; touch pwned; echo ''
— a syntactically valid bash command that breaks out of the quoted argument and
executes arbitrary shell commands in a step that has KBC_TOKEN /
KBC_STORAGE_API_URL secrets in its env: block, i.e. secret exfiltration is a
one-line addition to the injected payload. Any contributor who can add a folder +
.keboola/manifest.json to the customer's repo before the operator runs the
generator (a normal PR in a multi-contributor repo) controls this string. The
PR's own "security review" pass (per description) reports "no material issues" —
this class of bug is exactly what that pass should have caught. Fix: sanitize
Project.directory at discovery time (reject/escape non-[A-Za-z0-9._/-]
characters) or render with shlex.quote() in _project_step before embedding.

Non-blocking findings

[NB-1] plugins/kbagent/skills/kbagent-cicd-migration/references/secrets-setup.md:34 vs scripts/migrate_cicd.py gen_validate — environment-scoped secrets break the generated validate workflow

secrets-setup.md step 1 instructs: "Scope KBC_TOKEN_* for production
projects to the prod environment." But gen_validate() (migrate_cicd.py,
~line 192) emits a validate job with no environment: key, and it runs
sync diff + sync push --dry-run for every project including prod ones on
every pull_request. GitHub Actions only exposes environment-scoped secrets to
jobs that declare that environment: — reproduced by generating the fixture
workflow and confirming kbagent-validate.yml's validate job has no
environment: field while kbagent-push.yml's does. Following the documented
secrets setup verbatim means KBC_TOKEN_<prod-alias> resolves to an empty
string inside validate, and every PR-time diff/dry-run against a prod project
fails auth. Either add a matching environment: to the validate steps for
prod-scoped projects, or amend secrets-setup.md to say environment scoping
applies to push only (not diff/dry-run).

[NB-2] plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py — no unit tests for the generator itself

The PR's new test file (tests/test_sync_cli_behavior.py) only exercises
kbagent's existing sync CLI guards; it does not test migrate_cicd.py's
own logic (discover_projects, _alias_from_dir, _project_step,
detect_legacy_ci). A basic test asserting the rendered run: line is safe for
a directory name containing a single quote would have caught [B-1] before
merge. Not one of CONTRIBUTING.md's mandatory CLI-command test tiers (this is a
skill script, not a commands//services/ change), so non-blocking, but worth
adding given the script is meant to run unattended against customer repos.

[NB-3] plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py:159,161 — generated GitHub Action versions are one major behind this repo's own CI

The generator pins actions/checkout@v4 and astral-sh/setup-uv@v5, while
this repo's own .github/workflows/{ci,e2e,release*}.yml all use
actions/checkout@v5 and astral-sh/setup-uv@v7. Functionally harmless (older
majors still work), but since the skill is meant to hand customers "clean,
current" CI, bumping the pinned action versions to match what kbagent itself
uses would avoid handing out stale-on-day-one workflows.

Nits

• [NIT-1] plugins/kbagent/skills/kbagent-cicd-migration/SKILL.md / references/*.md — the live-verification dates (2026-08-06, "kbagent v0.80.0") are baked into prose sentences rather than a single "verified against" header; harmless today (main is in fact v0.80.0) but will read as a stale citation once the CLI moves past 0.80.x, with no (since vX.Y.Z)-style tag to signal it's a point-in-time repro note rather than a version floor.

Verification log

• gh pr view 402 --json title,body,files,... → 7 files added (+1211/-0), title feat: kbc→kbagent CI/CD migration skill + sync CLI behavior tests, conventional prefix feat: matches (new skill + tests) ✓
• gh pr diff 402 / gh api repos/keboola/cli/contents/<path>?ref=f98d237a... → fetched all 7 PR files at the actual PR head SHA (working tree stayed on claude/code-review-pr-402-26ec37/main throughout; no checkout performed) ✓
• gh pr view 402 --json commits → last commit f98d237a on 2026-08-10 (today), not 2026-06-09 — the PR was rebased/actively maintained through today despite the original open date; live-verification claims inside the docs (v0.80.0, 2026-08-06/08-10) line up with current main's actual pyproject.toml version (0.80.0) ✓ — staleness concern from the task focus does not materialize for this PR's content
• Cross-checked doc claims against current main source: sync/manifest.py:120-121 (allowedBranches/ignoredComponents fields) ✓, sync/config_format.py COMPONENT_TYPE_MAP exists ✓, services/sync_service.py "Manifest already exists" error text ✓, never_fetched diff/push semantics ✓, constants.py ENV_PROJECT_FROM_ENV = "KBAGENT_PROJECT_FROM_ENV" ✓, config_store.py _inject_env_project/_ensure_gitignore ✓, commands/sync.py pull --force/--theirs and push --force (no --allow-delete) flag semantics match command-mapping.md word-for-word ✓
• uv run pytest <fetched tests_test_sync_cli_behavior.py> (run against current main, package rebuilt via uv run) → 6 passed ✓
• Ran the fetched migrate_cicd.py against a synthetic 2-project fixture repo (--write --version 0.80.0 --main-branch main --schedule "0 * * * *") → generated 3 workflow files; python3 -c "import yaml; yaml.safe_load(...)" on all 3 → valid YAML ✓
• Reproduced the shell-injection finding [B-1]: imported migrate_cicd.py directly and called _project_step() with a directory name containing '; touch pwned; echo ' → rendered run: line breaks out of the quoted string (see finding) ✗
• Confirmed [NB-1] by inspecting the generated kbagent-validate.yml's validate job — no environment: key, while kbagent-push.yml's push job has one ✓ (bug reproduced, not just read from the diff)
• plugins/kbagent/.claude-plugin/plugin.json / .claude-plugin/marketplace.json — no per-skill registration list; Claude Code auto-discovers skills/*/SKILL.md, so no silent-drift surface applies to adding a second skill directory ✓
• scripts/generate_skill.py SKILL_MD constant is hardcoded to plugins/kbagent/skills/kbagent/SKILL.md → make skill-check/make skill-gen do not touch the new skill's SKILL.md, no CI drift risk ✓
• scripts/check_file_size.py PKG_ROOT.rglob scans only src/keboola_agent_cli → the 422-line migrate_cicd.py under plugins/kbagent/skills/.../scripts/ is exempt from loc-check ✓
• No new CLI commands added (confirmed via git diff main...HEAD -- src/keboola_agent_cli producing no hits in gh pr diff) → OPERATION_REGISTRY, AGENT_CONTEXT, CLAUDE.md "All CLI Commands", commands-reference.md, server routers, and keboola-expert.md §2 matrix rows are all N/A for this PR ✓
• Grepped fetched files for realistic-token patterns and bare except: → none found; print() usage in migrate_cicd.py is acceptable (stdlib-only standalone CLI script outside src/, not subject to convention #10's "no print()" rule for the package) ✓
• Did not run full make check against the PR's exact tree: doing so would require checking out f98d237a... or copying PR files into the working tree, both of which are writes to the repo working tree that this reviewer's read-only mandate forbids. Verified the new test file in isolation instead (see pytest run above); PR description claims a full make check run (5411 passed, 11 skipped) which is consistent with the repo's current scale but was not independently re-run here.

Open questions for the author

• Was the "security review" mentioned in the PR description (independent agent pass, "no material issues") run against the actual rendered YAML output, or only against the generator source read statically? A static read of _project_step alone is easy to mis-judge as safe because the f-string looks like a normal quoted shell arg; only rendering it with an adversarial directory name surfaces the injection.

[Matovidlo]

Addressing the review — fixed in b48f257 (all findings; make check: 5424 passed, 0 failed):

[B-1] Shell injection via unescaped p.directory — Fixed both ends, per your suggested options: discover_projects() now rejects a directory name outside [A-Za-z0-9._-] at discovery time (skip + stderr warning, same pattern as the existing missing-project.id handling), and _project_step() renders --directory via shlex.quote() as defense-in-depth. Verified against your exact repro string (foo'; touch pwned; echo ') — the project is now skipped entirely at discovery, no workflow is generated for it, and tests/test_migrate_cicd.py::TestDiscoverProjects::test_rejects_unsafe_directory_name + TestProjectStepShellSafety::test_directory_with_single_quote_is_shell_safe pin both layers as regression tests.

[NB-1] Environment-scoping breaks validate — Confirmed your repro. Rather than restructuring validate into per-project jobs (disproportionate for a migration skill), amended secrets-setup.md to spell out the interaction explicitly: scoping KBC_TOKEN_* to prod makes it invisible to the un-scoped validate job, which needs it for its read-only diff/dry-run against prod projects. The doc now tells the operator to either keep an unscoped copy available to validate too, or accept that validate won't dry-run prod-scoped projects until push.

[NB-2] No unit tests for the generator — Added tests/test_migrate_cicd.py (13 cases): discover_projects (multi-project layout, missing project.id, invalid JSON, the unsafe-directory rejection), _alias_from_dir (root/nested/sanitization), detect_legacy_ci (kbc command detection, legacy env var, clean-workflow non-flagging), and the shell-injection regression tests above.

[NB-3] Stale action versions — Bumped actions/checkout@v4 → v5 and astral-sh/setup-uv@v5 → v7 to match this repo's own .github/workflows/*.yml.

[NIT-1] Scattered live-verification dates — Consolidated into one "Verified against" header per doc file (SKILL.md, migration-runbook.md, command-mapping.md) instead of re-stating the date in every sentence.

Open question — Honestly, no: the earlier "security review" pass was a static read of the generator source by an independent review agent, not a rendering with adversarial input. That's exactly the gap that let B-1 through, as you pointed out — a static read of _project_step looks like a normal quoted shell arg. Good catch; noted for how I frame future security-focused passes on generator/templating code (render with hostile input, don't just read the f-string).

[padak]

Delta review of #402 — kbc → kbagent CI/CD migration skill (fix commit b48f257)

Generated by kbagent-pr-reviewer subagent. This is a delta review against
my prior review posted 2026-08-10 (target f98d237a), re-verified against the
new head b48f2578 after the author's fix commit. Verdict and findings below
are advisory; the human author retains every veto.

Summary

The fix commit (b48f257) genuinely closes the shell-injection hole: discover_projects()
now rejects unsafe directory names at discovery time and _project_step() independently
shlex.quote()s the value as defense-in-depth — I reproduced the original exploit string
against both layers and confirmed no injection is possible. The claimed 13-case
tests/test_migrate_cicd.py file exists, imports cleanly, and all 13 pass; the 6-case
tests/test_sync_cli_behavior.py also passes against current main. The NB-1 doc fix
is coherent. However, two of the "fully addressed" claims are only partially true:
the action-version bump (NB-3) missed one reference-doc mention that still shows the old
version, and the "consolidated into one header" claim (NIT-1) left several scattered
inline dates untouched. Neither is a regression with functional impact — verdict is
COMMENT, no blocking findings.

Priority hypothesis: nested-directory regex regression — CLEARED

I built a fixture repo with a nested project at projects/12345/.keboola/manifest.json
(the multi-project layout the PR explicitly promises to preserve) and ran the generator.
The nested project was discovered and included in all three generated workflows,
alias PROJECTS_12345, --directory projects/12345 rendered correctly. The regex is
_SAFE_DIRECTORY_RE = re.compile(r"^[A-Za-z0-9._][A-Za-z0-9._/-]*$") (migrate_cicd.py:90)
— the rest-of-string character class explicitly includes /, so nested paths pass. The
hypothesis that the B-1 fix would silently drop nested projects to a stderr warning is
not what happens. Confirmed correct behavior, no correctness regression.

Verdict

• Verdict: COMMENT
• Blocking findings: 0
• Non-blocking findings: 2
• Nits: 2

Per-prior-finding disposition

ID	Prior severity	Status	Evidence
B-1 shell injection via unescaped p.directory	BLOCKING	RESOLVED	Exact repro string foo'; touch pwned; echo ' rejected at discover_projects() (skip + stderr warning, no workflow emitted); independently confirmed _project_step() alone (bypassing discovery) renders it via shlex.quote() into a properly '\''-escaped literal — no shell breakout at either layer.
NB-1 secrets env-scoping breaks validate	NON-BLOCKING	RESOLVED	secrets-setup.md now explicitly explains the scoping/visibility interaction and gives the operator two concrete unambiguous options (a)/(b). No broken default is left implicit.
NB-2 no unit tests for the generator	NON-BLOCKING	RESOLVED	tests/test_migrate_cicd.py — 13 cases as claimed, all pass (see Verification log). Both named regression tests exist and genuinely pin the two layers (TestDiscoverProjects::test_rejects_unsafe_directory_name, TestProjectStepShellSafety::test_directory_with_single_quote_is_shell_safe).
NB-3 stale pinned action versions	NON-BLOCKING	PARTIALLY RESOLVED — see [NB-1-new] below	Generator itself (_install_steps, gen_validate/gen_pull/gen_push) correctly bumped to checkout@v5 / setup-uv@v7, matching this repo's own CI. But references/command-mapping.md — a file the fix commit did touch (to add the "Verified against" header) — still recommends the old setup-uv@v5 two lines below that header.
NIT-1 scattered live-verification dates	NIT	PARTIALLY RESOLVED	A consolidating "Verified against" header was added to SKILL.md, migration-runbook.md, command-mapping.md as claimed, but the commit message's framing ("consolidated ... instead of re-stating the date in every sentence") overstates it — several original per-sentence 2026-08-06 / "live-verified" mentions remain inline in SKILL.md and migration-runbook.md.

Non-blocking findings

[NB-1-new] plugins/kbagent/skills/kbagent-cicd-migration/references/command-mapping.md:16 — install-mapping row still recommends setup-uv@v5

The fix commit bumped the generator's action pins to checkout@v5/setup-uv@v7
(migrate_cicd.py:180, verified against this repo's own .github/workflows/*.yml, which
are uniformly on v5/v7). But command-mapping.md's "kbc (old) → kbagent (new)" table
still has | Custom installcomposite action |astral-sh/setup-uv@v5+ oneuv tool
install line | — a human reading this reference doc as the authoritative mapping (the
file's own opening line calls it "Authoritative") gets a stale recommendation that
contradicts what the generator actually emits. Low impact (the generated workflow itself
is correct, and any human copying from the doc would just be one version behind), but it's
exactly the kind of doc/generator drift this PR's tooling is supposed to prevent for
end-users migrating off kbc.

[NB-2-new] plugins/kbagent/skills/kbagent/SKILL.md — hub skill's description/workflow table doesn't reference the new CI/CD-migration topic

Correction to my prior review's conclusion that "no silent-drift surface applies" — that
was too lenient. CONTRIBUTING.md's Plugin synchronization map has a row for
plugins/kbagent/skills/kbagent/SKILL.md — description / rules / workflow links: "New
topic area in description triggers; new workflow file added to bottom table | CI catches
drift: NO". This PR introduces the second skill directory ever under
plugins/kbagent/skills/ (only kbagent existed before) and a genuinely new topic area
(kbc→kbagent CI/CD migration). I re-verified at head b48f257: plugins/kbagent/skills/kbagent/SKILL.md
is untouched by this PR (git show b48f257 --stat for that path is empty) and still has
zero references to cicd/ci-cd/kbc/migration (grep hits only unrelated lines:
KBC.configuration.folderName, "KBC UI merge URL", sql-migration-workflow). This is
mitigated by the new skill carrying its own strong, keyword-rich description: frontmatter
(kbagent-cicd-migration/SKILL.md:2-13), which Claude Code's skill discovery matches
independently per-skill-directory — so the new skill is not literally undiscoverable, just
not cross-linked from the primary hub a user/agent is most likely to already have loaded.
Non-blocking given that mitigation, but worth a follow-up per the mandatory checklist in
CONTRIBUTING.md > "Documentation changes (mandatory!)".

Nits

• [NIT-1] plugins/kbagent/skills/kbagent-cicd-migration/scripts/migrate_cicd.py:113 — the rejection stderr message says directory names "must match [A-Za-z0-9._-] only", but the actual _SAFE_DIRECTORY_RE (line 90) permits / in every position after the first char (by design — nested project paths need it, confirmed above). The message is misleading for an operator debugging a skip; consider "(must start with a letter/digit/./_ and contain only those plus / -)". Same character-class note: because . is allowed everywhere after the first char, a (currently unreachable, since discover_projects() only ever feeds it real glob-derived descendant paths) string like "../etc" would technically pass the regex — not exploitable today, but worth a ..-segment guard if this validator is ever reused for a different, less-trusted directory source.
• [NIT-2] plugins/kbagent/skills/kbagent-cicd-migration/references/migration-runbook.md:58,97 and SKILL.md:61 — per-sentence 2026-08-06 / "live-verified" / "confirmed live" mentions remain despite the commit message describing the date consolidation as complete; harmless but slightly overclaims the fix's scope.

Verification log

• gh pr view 402 --json headRefOid → b48f25782787fbd6b3477292f72d521dafa7bbf7, matches the SHA given in the task ✓
• gh pr diff 402 → 1437-line diff, 8 files touched (skill tree + 2 test files), no src/ changes ✓
• Built a fixture repo with projects/12345/.keboola/manifest.json (nested layout) + a root-level manifest, ran migrate_cicd.py (fetched at b48f257) → both projects discovered, projects/12345 correctly rendered with --directory projects/12345, alias=PROJECTS_12345 — hypothesis CLEARED ✓
• Built a fixture repo with directory name foo'; touch pwned; echo ', ran generator dry-run and --write → project skipped at discovery (! skipping ... unsafe directory name), zero workflow files reference it, no pwned file created anywhere ✓
• Called _project_step() directly with a Project whose directory is the malicious string (bypassing discover_projects() entirely) → rendered as --directory 'foo'"'"'; touch pwned; echo '"'"'', a correctly shlex.quote()-escaped single argument; no pwned file created — confirms defense-in-depth holds even if the discovery-layer guard were ever bypassed ✓
• Copied fetched tests/test_migrate_cicd.py + migrate_cicd.py into an isolated scratch tree mirroring the real relative path, ran with this repo's own .venv/bin/pytest → 13 passed (matches the claimed 13 cases and file's own test count via grep -c 'def test_') ✓
• Copied fetched tests/test_sync_cli_behavior.py into tests/ on the current (main) checkout, ran with .venv/bin/pytest → 6 passed, temp file removed afterward, working tree confirmed clean (git status --short empty) ✓
• .venv/bin/ruff check + ruff format --check on both fetched files → clean, already formatted ✓
• Generated actual workflow YAML via the fetched generator (--write --version 0.80.0 --schedule "0 6 * * *"), parsed all three .github/workflows/*.yml with PyYAML → parses without error (the on: → boolean-True-key artifact is a standard, harmless PyYAML/YAML-1.1 quirk that GitHub's own parser handles specially; every workflow in this repo's own .github/workflows/ uses the same bare on:) ✓
• grep -rn "actions/checkout@\|setup-uv@" .github/workflows/*.yml on current main → uniformly checkout@v5 / setup-uv@v7 (one unrelated pre-existing checkout@v4 in mcp-parity-canary.yml, not touched by this PR) — confirms the generator's version bump now matches repo convention, and surfaces the stale command-mapping.md mention (NB-1-new) ✓
• git diff f98d237..b48f257 -- <doc files> → isolated exactly what the fix commit changed per file, used to confirm which claims are literally true vs. partially true ✓
• pyproject.toml version on main = 0.80.0; gh release list → v0.80.0 is Latest (2026-08-03); PR touches no version/changelog/plugin.json files → correctly no version bump needed for this PR (informational note in prior context, not a blocker) ✓
• Could not run the full make check (5424-test claim) in this delta review — no src/ files changed by this PR, and the two new/changed test files were independently verified in isolation (13 + 6 = 19 tests, all passing); the arithmetic 5411 (prior claim) + 13 (new file) = 5424 (new claim) is internally consistent with the author's numbers.

Open questions for the author

(none)

[padak]

Approving — the blocking finding is genuinely closed and CI is green (4/4).

Verified at b48f2578: the shell-injection fix holds at both layers. _SAFE_DIRECTORY_RE = ^[A-Za-z0-9._][A-Za-z0-9._/-]*$ splits the first character from the tail, which is the right shape — excluding - and / from the leading position stops a directory name from masquerading as a flag or an absolute path, something shlex.quote() alone would not prevent, while the tail still admits / so nested multi-project layouts survive. That last point was my main regression worry after reading the fix summary (which abbreviated the charset as [A-Za-z0-9._-]); a fixture repo with projects/12345/.keboola/manifest.json discovers and renders correctly, so the concern does not apply.

Two non-blocking follow-ups from the delta review, both one-liners — fine either in this PR or a follow-up, not worth holding the merge:

1. references/command-mapping.md:16 still recommends astral-sh/setup-uv@v5 while the generator now emits v7. The file opens by calling itself authoritative, so it is the one place a human is most likely to copy the stale pin from.
2. plugins/kbagent/skills/kbagent/SKILL.md has no reference to the new CI/CD-migration topic area. Per CONTRIBUTING.md's Plugin synchronization map this is the "new topic area in description triggers" row, which CI does not catch. Mitigated by the new skill's own description being rich enough for discovery — flagging it so it is a deliberate choice rather than an oversight. (My first review called this N/A; that was too lenient.)

One release-time note, not a change request: this PR correctly carries no version bump, but v0.80.0 is already published and plugin.json/marketplace.json stay at it, so the skill reaches nobody until 0.81.0 ships. It also joins the pool of no-bump PRs that need backfilling into changelog.py at release — make changelog-check audits releases against the changelog, not PRs, so it will not flag the omission.

Nice work on the generator test file, and on the straight answer about the earlier security pass being a static read.

[Matovidlo]

Thanks for the delta review and the approval — and for confirming the nested-project charset shape was intentional (the /-in-tail-only design), that was exactly the point.

Both non-blocking follow-ups from your delta review are now in 5084d8c (approval remains valid, these are doc/message-wording only, no behavioral change):

1. command-mapping.md:16 — bumped astral-sh/setup-uv@v5 → v7 to match what the generator actually emits.
2. Hub SKILL.md cross-link — added a trigger sentence in the description: frontmatter plus a workflow-links table row pointing to kbagent-cicd-migration, per CONTRIBUTING.md's sync map.

Also, since I was in there: actually trimmed the remaining inline 2026-08-06/"live-verified" dates in SKILL.md/migration-runbook.md this time (the prior commit added a "Verified against" header but left several per-sentence dates untouched — your NIT-1/NIT-2 catch was right, that commit message overclaimed it), and fixed the discover_projects() rejection message to describe the real character class (it claimed no / when the regex allows it in the tail) plus added an explicit ..-segment guard as defense-in-depth per your NIT-1.

make check green (ruff, format, skill-check, full suite). Given you've already approved, I won't re-request review — flagging here for visibility in case you want another look at the doc tweaks, but not blocking on it.