feat(skill): add kbagent-promotion-pipeline skill for source->dest project promotion - #560
Conversation
…oject promotion Adds a from-scratch generator (plugins/kbagent/skills/kbagent-promotion-pipeline/) for a GitHub Actions pipeline that promotes Keboola configs between two distinct projects (e.g. dev -> prod), for the "one repo covers the whole org" pattern. Mechanic (kbagent-native, no git-branching or GH-Environment-per-branch magic needed): a pull workflow fetches the SOURCE project into a shared directory and opens one PR against main; a validate workflow runs `sync push --dry-run` against the DESTINATION project on that PR, showing exactly what would change there; a push workflow (environment-gated) ships it once the PR merges. Every step uses KBAGENT_PROJECT_FROM_ENV=1 + --project __env__, so no token ever touches disk. A single repo can host several independent pipelines via a JSON config (one entry per source/dest pair). Generator is stdlib-only, mirrors the kbagent-cicd-migration skill's structure; verified by generating both single- and multi-pipeline configs and inline sanity assertions on the rendered YAML.
|
@claude review |
There was a problem hiding this comment.
Pull request overview
Adds a new kbagent-promotion-pipeline skill under plugins/kbagent/skills/ that guides users through (and programmatically generates) a GitHub Actions–based source → destination promotion flow using kbagent sync with the KBAGENT_PROJECT_FROM_ENV=1 / --project __env__ CI authentication model.
Changes:
- Introduces a new skill runbook (
SKILL.md) describing the pull → validate → push promotion mechanic and required GitHub setup. - Adds a stdlib-only generator script that scaffolds three GitHub Actions workflows (
pull,validate,push) for one or multiple pipelines. - Adds reference docs for env-injection rationale and GitHub secrets/environment setup.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| plugins/kbagent/skills/kbagent-promotion-pipeline/SKILL.md | Skill runbook for the promotion pipeline pattern, steps, and guardrails. |
| plugins/kbagent/skills/kbagent-promotion-pipeline/scripts/generate_promotion_pipeline.py | Workflow generator producing pull/validate/push GitHub Actions YAML and a secrets checklist. |
| plugins/kbagent/skills/kbagent-promotion-pipeline/references/secrets-setup.md | Documentation for required secrets and prod environment gating. |
| plugins/kbagent/skills/kbagent-promotion-pipeline/references/env-injection.md | Background on KBAGENT_PROJECT_FROM_ENV / __env__ and why it’s used in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nd PR-trigger blockers Independent code-quality + OWASP/security review found two blockers and two majors that would have broken this skill in production. All fixed: - BLOCKER: `sync pull --directory <dir>` against the SOURCE token overwrote <dir>'s manifest (bootstrapped once from DEST, Step 4) with SOURCE's config IDs, so every subsequent `sync push` to DEST failed to match existing configs by ID and created duplicates on every promotion cycle instead of converging. Fixed: pull now targets a throwaway scratch directory and only the *content* (never .keboola/manifest.json) is merged into the tracked directory, keeping DEST's ID mapping stable forever. - BLOCKER: `peter-evans/create-pull-request` opened PRs with the default GITHUB_TOKEN, which GitHub deliberately excludes from triggering pull_request workflows -- kbagent-promote-validate.yml never ran, so reviewers approved blind with no destination-side diff. Fixed: PR-open step now requires a PROMOTION_PR_TOKEN PAT, documented in secrets-setup.md. - MAJOR: every pipeline's push ran as a *step* inside one shared job, so one approval click unlocked every pipeline's destination push at once, undermining "a human approves this specific promotion." Fixed: push.yml now generates one job per pipeline, each with its own environment: prod -- GitHub's approval gate is per job run, not per environment name, so each pipeline now needs its own separate approval and one pipeline failing no longer silently blocks the others (no shared job, no needs: dependency). - HIGH (security): #-secret values were never called out as project-scope-bound ciphertext that can't decrypt across projects -- documented explicitly in secrets-setup.md and the Guardrails section. - MEDIUM (security): documented the missing required-status-check step so merging can't bypass the validate gate. - LOW (security): added input validation rejecting unsafe directory/ stack_url values and a pipeline-name label-collision guard, both of which would otherwise corrupt generated YAML/shell quoting. - Removed a redundant `sync init` before `sync pull` in the Step 4 bootstrap instructions (pull auto-inits). Added tests/test_generate_promotion_pipeline.py (12 cases) covering the new scratch-pull mechanic, per-pipeline job isolation, and the validation guards -- this generator had no test coverage before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@claude review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
plugins/kbagent/skills/kbagent-promotion-pipeline/references/secrets-setup.md:6
- The opening paragraph says there is one
prodGitHub Environment “per pipeline”, but the rest of the doc (and the generator output) sets up a single sharedprodenvironment once per repo and relies on per-job approvals. Update the wording to avoid confusion during setup.
Each pipeline needs **two** Storage API token secrets -- one for the source
project, one for the destination project -- plus one repo-wide PAT for
opening promotion PRs, plus one `prod` GitHub Environment per pipeline used
for push approval gating (each pipeline gets its own approval; see below).
…erator - Escape directory/version/git-ref values with shlex.quote() before embedding them in generated shell commands, instead of naive single-quote wrapping. - The generated merge-content step now checks is_symlink() before deleting or copying entries: is_dir()/is_file() follow symlinks, so an unguarded rmtree()/copytree() could traverse outside the tracked directory. Deletion now unlinks a symlink directly instead of following it; a symlink in the pulled source content now fails the step loudly instead of silently copying through it (kbagent's own sync pull never produces symlinks). - _validate_pipelines now also rejects unsafe characters in pipeline `name` and two pipelines sharing the same `directory` (guaranteed wrong: two different tokens pulling/pushing into one folder). - --config parsing now catches OSError/JSONDecodeError/a missing required key and exits 2 with a clear message instead of an uncaught traceback. - Corrected SKILL.md wording: validate's dry-run step runs unconditionally for every configured pipeline once the workflow triggers, not only for "pipelines touched by the PR" (the paths: trigger gates the whole workflow, not individual per-pipeline steps). Added test coverage for all of the above (7 new cases). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
padak
left a comment
There was a problem hiding this comment.
Review of #560 — feat(skill): add kbagent-promotion-pipeline skill for source->dest project promotion
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check/CI, not duplicated here.Reviewed commit:
ae92b5a051dd849e9d70e99f080a664ed0893250(PR head, fetched viagit fetch origin pull/560/headand inspected in an isolated detached worktree; the invoking session's own worktree was never checked out to the PR branch and remains untouched).
Summary
This PR adds a brand-new, self-contained Claude Code skill (plugins/kbagent/skills/kbagent-promotion-pipeline/) that generates a 3-workflow GitHub Actions pipeline (pull -> validate -> push) for promoting Keboola configs from a source project to a destination project via kbagent sync, plus a 143-line stdlib-only test file. It touches no src/keboola_agent_cli/ code, adds no CLI commands, and needs no version bump — which takes almost the entire CLI-command "Plugin synchronization map" checklist out of scope for this review. The PR description documents an unusually thorough self-review (three real bugs found and fixed: non-convergent pull mechanic, a silently-never-firing validate check, and a shared-approval security gap across pipelines), and every actionable Copilot automated-review comment on the PR was addressed with a matching fix commit. CI is green (check, test 3.12/3.13, Windows job). Verdict: COMMENT — no blocking findings, three non-blocking/nit items worth a look before merge, most notably one Copilot-suppressed comment that appears to have been missed despite the PR title claiming full Copilot-comment coverage.
Verdict
- Verdict: COMMENT
- Blocking findings: 0
- Non-blocking findings: 3
- Nits: 1
Blocking findings
(none)
Non-blocking findings
[NB-1] plugins/kbagent/skills/kbagent-promotion-pipeline/references/secrets-setup.md:5-6 — leftover incorrect wording flagged by a (suppressed) Copilot comment
The intro says "plus one prod GitHub Environment per pipeline used for push approval gating." The actual mechanic (confirmed in the generator: secrets_report() prints a single gh api -X PUT repos/.../environments/prod, and _push_job() has every pipeline's job reference the same environment: prod) creates exactly one shared prod environment, with per-job-run approval — which the doc's own "One environment, per-pipeline approval" section (lines 69-78) correctly explains. Copilot posted this exact discrepancy as a review comment on commit 0994c391 ("The opening paragraph says... per pipeline, but the rest of the doc... sets up a single shared prod environment"), but GitHub marked it suppressed so it likely never surfaced to the author, and it is not among the comments Matovidlo replied "Fixed" to. Recommend rewording line 5-6 to "one prod GitHub Environment (shared across pipelines, gating each pipeline's push job separately)."
[NB-2] plugins/kbagent/skills/kbagent-promotion-pipeline/SKILL.md:20 — dangling relative link to a sibling skill that does not exist yet
[kbagent-cicd-migration](../kbagent-cicd-migration/SKILL.md) points at a skill directory that is not present on main (nor in this PR) — the referenced PR (#402, "kbc→kbagent CI/CD migration skill") is still open, unmerged. If #560 merges before #402, an AI agent (or a human) following this link gets a 404/missing-file. Not a functional break for this skill's own operation, but worth either merging #402 first, or softening the link to prose ("see the CI/CD migration skill, if present in this repo") until the dependency lands.
[NB-3] plugins/kbagent/skills/kbagent-promotion-pipeline/scripts/generate_promotion_pipeline.py:297-298,328,412 — --schedule/--main-branch are exempt from the input-validation hardening applied to every other admin-supplied field
The PR's own "LOW (security)" fix added _validate_pipelines() specifically because name/directory/*_stack_url are interpolated raw into generated YAML/shell and an admin-authored value with a stray quote/colon/newline would corrupt the workflow (the PR body cites this as CWE-78-adjacent). The same threat model applies unchanged to args.schedule (embedded unescaped as - cron: '{schedule}', line 298) and args.main_branch (embedded unescaped as base: {main_branch}, line 328, and branches: [{main_branch}], line 412) — both are plain argparse strings with no regex/charset check before being spliced into YAML. A stray ' in --schedule or a ]/: in --main-branch breaks the generated workflow the same way an unvalidated directory would have before this PR's fix. Recommend extending _validate_pipelines() (or a sibling check in run()) to cover these two CLI-level fields for consistency with the rest of the hardening pass.
Nits
[NIT-1]PR description ("Test plan") says the new test suite has "12 cases," buttests/test_generate_promotion_pipeline.pyactually has 18 test functions across 5 classes (confirmed:18 passedlocally). Cosmetic inaccuracy in the self-reported test count, not a code issue.
Verification log
gh auth status→ authenticated aspadak,repo/workflowscopes ✓- Read
CONTRIBUTING.md(Checklist: Adding a New CLI Command, Plugin synchronization map, Releasing a new version) andCLAUDE.md(convention #17,## All CLI Commands) ✓ — confirmed no CLI command surface changed in this PR, so most of the checklist/sync-map rows are not applicable here. - Read
plugins/kbagent/agents/keboola-expert.md§1/§3 ✓ — no Rule-6 version-gate or gotchas update needed (no new/changed CLI behavior). gh pr view 560 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ OPEN,feat(skill):prefix matches change type, +1040/-0, 5 files (SKILL.md,env-injection.md,secrets-setup.md,generate_promotion_pipeline.py,tests/test_generate_promotion_pipeline.py) ✓git fetch origin pull/560/head→ae92b5a051dd849e9d70e99f080a664ed0893250(matches the PR head SHA given in the task) ✓git worktree add --detach <scratch>/pr560-worktree ae92b5a0...→ isolated review checkout; the invoking session's own worktree (claude/pr-560-review-a8c632, HEADc5a23d6) was never touched ✓; worktree removed after review.uv sync --extra serverin the isolated worktree → deps installed ✓uv run pytest tests/test_generate_promotion_pipeline.py -v→ 18 passed in 1.92s ✓ (allTestPullMechanic,TestPushIsolation,TestValidate,TestValidatePipelines,TestConfigParsingcases green)uv run ruff check+uv run ruff format --checkon the new script + test file → clean, already formatted ✓uv run ty check plugins/kbagent/skills/kbagent-promotion-pipeline/scripts/generate_promotion_pipeline.py→ "All checks passed!" ✓gh pr checks 560→checkpass (37s),test (3.12)pass,test (3.13)pass, Windows job pass — all green ✓ (relied on CI for the full 5417-test suite rather than re-running it locally, per the PR's own reported5417 passed, 11 skipped, 0 failed)grep -E '^\+.*-> ?tuple\['/error_code\s*=\s*"[A-Z_]+"/except\s*:against the diff → all empty, no new bare-tuple returns, raw error-code strings, or bare excepts ✓- No layer-violation greps applicable (
services/,commands/,client/untouched by this diff) ✓ - No
OPERATION_REGISTRYentry needed (no new CLI command) ✓ - Manually traced
--directorysupport forsync pull/sync pushinsrc/keboola_agent_cli/commands/sync.py(lines ~471, ~923) to confirm the generated workflows'sync pull/push --directory <dir>invocations are valid against the real CLI surface (note:CLAUDE.md's## All CLI Commandslisting forsync pull/sync pushis itself missing--directoryin its signature — pre-existing drift, out of scope for this PR, flagged here only as a heads-up, not as a finding against #560). - Confirmed
__env__/KBAGENT_PROJECT_FROM_ENVis implemented generically inconfig_store.py(not command-specific), so it correctly applies tosync pull/sync pushas the skill assumes ✓ gh api repos/keboola/cli/pulls/560/comments→ cross-checked all 7 non-suppressed Copilot review comments against Matovidlo's "Fixed --" replies on the same lines — all 7 addressed with matching code changes (shlex.quote on version/git-ref/directory, symlink guards on both delete and copy sides of the merge step,--configparse error handling with exit 2, name/directory collision guards). One Copilot comment was markedsuppressedby GitHub and does not appear to have been seen/addressed (see NB-1).gh pr view 560 --json reviewDecision→REVIEW_REQUIRED,mergeStateStatus: BLOCKED(missing required approving review, not a merge conflict —mergeable: MERGEABLE).
Open questions for the author
- Was the whole-org multi-pipeline design manually exercised end-to-end against real GitHub Actions (not just YAML-parses-cleanly unit tests), i.e. did an actual
peter-evans/create-pull-requestrun with a realPROMOTION_PR_TOKENconfirm the validate check fires on the resulting PR? The PR body says "Generator manually exercised end-to-end," but it's ambiguous whether "end-to-end" reached an actual GitHub Actions run or stopped at local YAML generation +yaml.safe_loadvalidity.
The new promotion-pipeline skill is the first sibling skill next to skills/kbagent/, and nothing linked to it: an agent already inside the kbagent skill walks the bottom workflow table and sync-workflow.md, so the pipeline was unreachable from the surface it belongs to. - kbagent/SKILL.md: add the skill to the workflow-reference table, flagged as a separate skill rather than a references/*.md doc. - references/sync-workflow.md: point from the sync/clone chapter at the ongoing dev->prod promotion loop, and record why the source pull goes through a scratch directory (the tracked manifest stays bound to the destination project's config ids). - CONTRIBUTING.md: add a Plugin synchronization map row for sibling skills -- make skill-check only regenerates kbagent/SKILL.md and never looks at them, so this is a silent-drift surface.
The intro claimed one `prod` GitHub Environment "per pipeline", which contradicts both the generator (`secrets_report()` prints a single `gh api -X PUT .../environments/prod`, and every pipeline's push job references the same `environment: prod`) and this file's own "One environment, per-pipeline approval" section. What is per-pipeline is the approval prompt, not the environment.
…o YAML
_validate_pipelines() hardened the per-pipeline fields (name, directory,
*_stack_url) because they are interpolated raw into the generated
workflows. The same is true of the two CLI-level fields, which were
exempt: --schedule lands in `- cron: '{schedule}'`, --main-branch in
`base: {main_branch}` and `branches: [{main_branch}]`. A stray quote,
bracket, colon or newline in either produced a workflow GitHub cannot
parse.
_validate_workflow_options() applies the same fail-fast treatment: the
branch must match a safe charset with no `..`, and the schedule must be
exactly five space-separated POSIX cron fields. The cron check is a
single regex rather than split()+charset, because split() also splits on
newlines and would let `0 6 * *\n1` through as five valid-looking
fields.
8 new tests (26 total in the file), including the end-to-end exit-2 path.
Changelog entries for everything riding the 0.84.0 catch-up release. No code change. 0.84.0 is still untagged -- the last release is v0.82.0, with 0.83.0 and 0.84.0 sitting on main unreleased -- so these go under the existing key rather than claiming a version of their own. 0.85.0 stays reserved for the epic #390 removal.
#591 added a test asserting the newest version's release notes are not shown truncated. #589 was written and CI'd before that test existed and was never re-run against the newer main, so both PRs were green and merging them in order produced a red main. Its Tests note opened with a 161-character sentence, one over CHANGELOG_HEADLINE_MAX_CHARS, so the summary broke off at '... and CI stayed green …'. One dash becomes a period: same words, 161 -> 99 chars. This is the failure mode issue #585 describes -- CI checks the merge commit but only recomputes it when the PR is updated, so a branch that sits while main moves reports green for a merge that no longer exists. Second occurrence today; #560 hit it earlier at 33 commits stale and was caught only because the merge was simulated by hand.
Summary
kbagent-promotion-pipelineskill (plugins/kbagent/skills/kbagent-promotion-pipeline/): a from-scratch generator (scripts/generate_promotion_pipeline.py, stdlib-only) that scaffolds a GitHub Actions pipeline promoting Keboola configuration changes from a named source project (e.g. dev) to a named destination project (e.g. prod).kbagent-promote-pull.yml(pulls the source project into a scratch directory, merges its content into the tracked directory, and opens/updates one PR againstmain),kbagent-promote-validate.yml(runssync push --dry-runagainst the destination project on that PR -- the cross-project diff), andkbagent-promote-push.yml(one job per pipeline, each pushing to its destination project once merged, each gated by its ownprodGitHub Environment approval).{name, directory, source_stack_url, dest_stack_url}entry per pair) -- the "one GitHub repo covers the whole org" pattern.KBAGENT_PROJECT_FROM_ENV=1+--project __env__env-injection model, so no token ever touches disk.Why
Follow-up to #402 (kbc->kbagent CI/CD migration). That PR ports an existing kbc-managed repo; this one covers the separate, from-scratch use case of standing up a new dev->prod (or any source->destination) promotion pipeline that didn't exist before, modeled after the pattern in
keboola/cli-based-sync-generatorbut adapted to kbagent's project-alias-per-invocation model rather than that tool's git-branch-bound-to-a-GitHub-Environment mechanic. Kept as a separate PR (rather than folding into #402) since it's a genuinely new capability, not a bugfix, and #402 was deliberately squeezed down in scope during its own review pass.Review pass (code quality + OWASP/security, independent agents) -- this PR was substantially reworked as a result
The first draft had a fundamentally broken cross-project sync mechanic and two other real bugs. All are fixed in this PR; details for reviewers:
BLOCKER -- push never converged.
sync pull --directory <dir>against the SOURCE token overwrote<dir>'s.keboola/manifest.json(bootstrapped once from the DESTINATION project in Step 4) with the SOURCE project's config IDs. Every subsequentsync pushto DEST then failed to match any config by ID and created duplicates on every single promotion cycle instead of updating the originals -- the pipeline would never converge. Fix: pull now targets a throwaway/tmp/promote-scratch/<dir>and a small inline Python step merges only the content (everything except.keboola/) into the tracked directory, so the destination-bound manifest is never touched by a source pull.BLOCKER -- validate never actually ran.
peter-evans/create-pull-requestopened the promotion PR using the workflow's defaultGITHUB_TOKEN. GitHub deliberately suppressespull_request-triggered workflow runs for PRs opened with that token, sokbagent-promote-validate.yml-- the whole "see the destination-side diff before approving" premise -- silently never fired; reviewers would approve blind. Fix: the PR-open step now requires aPROMOTION_PR_TOKENPAT/GitHub-App token, documented with the "why" inreferences/secrets-setup.md.MAJOR -- one approval unlocked every pipeline. Every pipeline's destination push ran as a step inside one shared job with one
environment: prodgate, so a single reviewer approval simultaneously authorized every unrelated pipeline bundled into that push run. Fix:kbagent-promote-push.ymlnow generates one independent job per pipeline (push_<label>), each with its ownenvironment: prod. GitHub's required-reviewer gate is enforced per job run, not per environment name, so each pipeline now needs its own separate approval click, and one pipeline's job failing no longer silently blocks the others (no shared job, noneeds:dependency between them).HIGH (security) -- undocumented secret-promotion footgun.
#-prefixed (encrypted) config values are stored on disk as project-scoped ciphertext that the destination project cannot decrypt -- promoting them throughsync pull/sync pusheither hard-fails (fail-closed encryption) or, worse, could land as an inert non-functional string that looks like a secret but isn't. NeitherSKILL.mdnor the references said this. Fix: explicit new section insecrets-setup.md+ a new Guardrails bullet: never promote#-secret values across projects; set destination secrets independently on the destination project.MEDIUM (security) -- PR gate described, not enforced. Nothing instructed setting up a required status check for
validateonmain's branch protection, so a PR could merge even if validate never ran. Fix: added to secrets-setup.md and the Guardrails section.LOW (security) -- unsanitized fields could corrupt generated YAML/shell.
directory/*_stack_urlwere interpolated raw into single-quoted shell strings and YAMLpaths:lists. Fix: added_validate_pipelines()rejecting path-traversal/unsafe-charset directories and non-well-formed stack URLs, plus a pipeline-name label-collision guard (two names that sanitize to the same secret-name label now fail fast instead of silently colliding).Minor: removed a redundant
sync initbeforesync pullin the Step 4 bootstrap instructions (sync pullauto-initializes).This generator previously had no test coverage; added
tests/test_generate_promotion_pipeline.py(12 cases) covering the scratch-pull/merge mechanic, per-pipeline job isolation (no shared approval, no cross-pipelineneeds:), and all three validation guards.Change type
Feature — new skill. No source/CLI-command changes, no version bump.
Impact analysis
plugins/kbagent/skills/kbagent-promotion-pipeline/+tests/test_generate_promotion_pipeline.py.src/, no new CLI commands, no public API or behavior change.Test plan
tests/test_generate_promotion_pipeline.py-- 12 tests, green (scratch-pull mechanic, per-pipeline job isolation, PAT usage, all 3 input-validation guards, YAML validity of every generated workflow).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): 5417 passed, 11 skipped, 0 failed.Deployment
Merge & automatic deploy. No migration.
Rollback plan
Revert of this PR.
🤖 Generated with Claude Code