chore(ci): silent-drift gate, ty/changelog/error-code gates, py3.13 matrix, nightly e2e - #385
Conversation
…atrix, nightly e2e Close the gap between local `make check` and CI, and add deterministic CI coverage for the silent-drift surfaces that previously had none. CI (.github/workflows/ci.yml): - Split into a `check` job (static analysis, single 3.12 run) and a `test` job (matrix Python 3.12 + 3.13). Coverage is printed (--cov term-missing) but informational -- no --cov-fail-under threshold, so it never blocks. - Wire in gates that previously ran only locally or in the pre-commit hook: `ty` type-check, changelog completeness, error-code enum check, and the new command-sync gate. Command-sync gate (scripts/check_command_sync.py + tests): - Treats the live Typer command tree as the single source of truth and fails if any command is missing from permissions.py OPERATION_REGISTRY, CLAUDE.md, commands/context.py AGENT_CONTEXT, or commands-reference.md, or if a registry key matches no live command. Calibrated to zero false positives (hidden-alias filter; full-leaf vs 2-segment granularity per surface). - gotchas.md (since vX.Y.Z) tagging is intentionally left to /kbagent:review: distinguishing a new version-specific gotcha from a version-independent section needs judgement, not a deterministic gate. Fixes surfaced by gating `ty` in CI: - 3 pre-existing `ty` errors in tests (missing None-guards / a None client factory) that accumulated while `ty` ran only against staged files. Docs: - Documented 11 previously-undocumented commands (permissions/sharing/project info) in commands-reference.md so the command-sync gate passes. - CONTRIBUTING.md: new "CI workflows" section describing both workflows. E2E (.github/workflows/e2e.yml): - Nightly cron + workflow_dispatch against a dedicated throwaway project; not wired into the per-PR gate. Skips gracefully (green + warning) when the E2E_API_TOKEN / E2E_URL secrets are absent. make check now mirrors CI (adds typecheck + command-sync-check); test-cov added.
The changelog completeness check shells out to `gh release list`, which needs GitHub auth + network and audits whether published *releases* have changelog entries -- a release-time concern, not a per-PR one. It failed in CI (no gh token in the step) and is redundant on PRs (releases don't change between them). It stays in local `make check`. check-error-codes (pure AST, no network) remains gated in CI.
padak
left a comment
There was a problem hiding this comment.
Review of #385 — chore(ci): silent-drift gate, ty/changelog/error-code gates, py3.13 matrix, nightly e2e
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, not duplicated here.
Summary
This PR lifts several previously local-only checks (ty type check,
error-code enum check, and the new scripts/check_command_sync.py
silent-drift gate) into CI, splits the existing single check job into
a check job (static analysis) and a test job (matrix Python 3.12 +
3.13), and adds a nightly e2e.yml workflow. Three pre-existing ty
errors in tests are fixed, and 11 previously undocumented commands are
added to commands-reference.md. The change is pure dev-infrastructure
with no user-facing CLI behaviour changes. APPROVE — the implementation
is correct, make check passes (3890 passed, 8 skipped), and the
check_command_sync.py self-check confirms all 224 commands are properly
registered and documented in every surface.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 1
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] scripts/check_command_sync.py:1 — server/routers/<group>.py coverage gap not declared in script's scope documentation
The script docstring at the top of check_command_sync.py lists four
surfaces it checks and explicitly lists gotchas.md under "NOT checked
here". It does not mention server/routers/<group>.py, which
CONTRIBUTING.md (§ "Checklist: Adding a New CLI Command") requires for
every new command (skip is allowed only for terminal-only commands, with a
documented reason). A contributor reading only the script docstring will
not know this surface is intentionally out of scope, and may incorrectly
assume the CI gate covers it.
Fix: add one line to the "NOT checked here" block in the module
docstring: - server/routers/<group>.py REST mirror (requires judgement on which commands are terminal-only; document skip in PR description per CONTRIBUTING.md).
Nits
-
[NIT-1].github/workflows/ci.yml:80-81— Comment for the
Command-sync silent-drift checkstep includes"and that every gotcha carries a (since vX.Y.Z) tag"butcheck_command_sync.pyline 29
explicitly statesNOT checked here: gotchas.md (since vX.Y.Z) tags.
The comment overstates the gate's scope and may mislead contributors
into thinking gotchas versioning is CI-enforced. Remove or replace with
(gotchas.md (since vX.Y.Z) tagging is left to /kbagent:review). -
[NIT-2].github/workflows/ci.yml— Thetestjob does not declare
needs: check. Both jobs run in parallel, so a type error or lint failure
incheckdoes not block thetestjob from being green. This is a
common and accepted CI pattern (faster feedback on independent failures),
but worth noting so future authors do not assumetestpassing implies
checkpassed when reading job statuses.
Verification log
gh pr view 385 --json title,body,files→ 12 files, +669/-18,chore(ci):prefix for a pure dev-infra change ✓git rev-parse --abbrev-ref HEAD→claude/friendly-rubin-ffcc48(matches PR branch) ✓- Layer violation scan (
grep typer/click in services/,grep httpx in commands/, etc.) →import clickandimport typer.mainappear only inscripts/check_command_sync.py(a standalone script, not production source) ✓ - No new
@*_app.command(...)decorators in diff → no CLI command changes; Plugin synchronization map step 3.4 skipped (no new commands) ✓ make check→ruff: all checks passed;ruff format: 312 files formatted;ty check: 1 warning (downgradedunresolved-importforhatchling, expected and non-blocking per[tool.ty.rules]);SKILL.md: up-to-date; version-sync: in sync;check_command_sync.py:OK: all 224 CLI commands are registered; changelog: all 48 stable releases have entries; error-codes: no raw literals; pytest: 3890 passed, 8 skipped ✓- Three
tyfixes in tests confirmed:test_config_store.py:719andtest_services.py:733add None-guards forget_project()return;test_sync_plaintext_audit.py:748replaceslambda …: Nonefactory withlambda …: MagicMock()to satisfy theKeboolaClient | Nonetype ✓ e2e.ymlcredential guard checks onlyE2E_API_TOKEN;E2E_URLdefaults to"connection.keboola.com"intest_e2e.py:310, so missingE2E_URLwith a setE2E_API_TOKENdoes not silently skip — it would run against the default URL. Acceptable: the setup instructions require both secrets, and the self-skip guard exists for the "no secrets at all" fork-PR case ✓NIT-1confirmed:ci.ymlline 81 text"and that every gotcha carries a (since vX.Y.Z) tag"vscheck_command_sync.pyline 29"NOT checked here: gotchas.md (since vX.Y.Z) tags"— factually contradictoryNB-1confirmed:check_command_sync.pymodule docstring "NOT checked here" block lists onlygotchas.md;server/routers/<group>.pyis not mentioned- No hardcoded secrets, no token leaks, no bare
except:, noprint()in production source, no rawerror_code="..."literals in diff ✓ - PR description matches diff scope: CI infrastructure only, no user-facing CLI behaviour change ✓
Open questions for the author
(none)
Address review findings on #385: - NIT-1: the ci.yml command-sync step comment claimed the gate checks gotcha `(since vX.Y.Z)` tags; it does not. Corrected to match the script. - NB-1: check_command_sync.py docstring now lists server/routers/<group>.py REST mirror alongside gotchas.md under "NOT checked here" (both need judgement -> /kbagent:review), so contributors don't assume the gate covers the REST surface.
…review) Devin review noted the CI `test` job uses `-m "not integration"` while the Makefile `test` target uses `-m "not e2e"`, so "make check mirrors CI" was imprecise. Reworded: make check is a superset (it additionally runs the integration tests CI deselects; they skip/pass without credentials and never fail offline), so a green make check implies CI's selection passes too.
…ge projects (#389) Surfaced by running the E2E suite in CI against a GCP / Snowflake-keypair project (the new nightly e2e.yml from #385). All findings are test/environment robustness, NOT product bugs -- no product code changed. - ConfigSecretEncryption (x2): read `created["data"]["id"]`, not `created["id"]`. `config new --push` returns the standard `{status, data}` envelope; the test forgot to unwrap `["data"]` (the sibling `_test_config_new_push` already does). Manifested as `KeyError: 'id'`. - swap-tables rejection test: assert `"requires a branch"` instead of the old `"dev branch"` wording. The wording was corrected in #373 (swap works on any branch, incl. production); the CLI test was updated there but this E2E assertion was missed. - FullE2E workspace password: keypair-auth workspaces (Snowflake person-keypair login) have no password, so the API returns HTTP 400 "not supported for login type ..." -- skip that step cleanly instead of failing the whole run. - metastore_scope_available preflight: also treat an unreachable metastore host (connection error) as "scope unavailable" and skip, so a misconfigured stack URL yields one clean skip instead of a wall of errors across every dependent test.
…ge projects (#391) Surfaced by running the E2E suite in CI against a GCP / Snowflake-keypair project (the new nightly e2e.yml from #385). All findings are test/environment robustness, NOT product bugs -- no product code changed. - ConfigSecretEncryption (x2): read `created["data"]["id"]`, not `created["id"]`. `config new --push` returns the standard `{status, data}` envelope; the test forgot to unwrap `["data"]` (the sibling `_test_config_new_push` already does). Manifested as `KeyError: 'id'`. - swap-tables rejection test: assert `"requires a branch"` instead of the old `"dev branch"` wording. The wording was corrected in #373 (swap works on any branch, incl. production); the CLI test was updated there but this E2E assertion was missed. - FullE2E workspace password: keypair-auth workspaces (Snowflake person-keypair login) have no password, so the API returns HTTP 400 "not supported for login type ..." -- skip that step cleanly instead of failing the whole run. - metastore_scope_available preflight: also treat an unreachable metastore host (connection error) as "scope unavailable" and skip, so a misconfigured stack URL yields one clean skip instead of a wall of errors across every dependent test.
Why
The repo's CI was a narrower subset of local
make check, not a superset — so "passedmake checklocally" was a stronger guarantee than "passed CI", which left several checks dependent on contributor discipline.ty,changelog-check, andcheck-error-codesran only locally or in the pre-commit hook, and the silent-drift surfaces called out in CONTRIBUTING.md "Plugin synchronization map" / CLAUDE.md convention #17 had no CI coverage at all — forgetting one ships an AI agent that recommends commands the installed kbagent version doesn't have.This PR closes that gap with deterministic gates only. LLM-based review stays out of CI, available as the advisory
/kbagent:review.What
CI (
ci.yml)checkjob (static analysis, single 3.12 run) and atestjob (matrix Python 3.12 + 3.13;requires-pythonis>=3.12).tytype-check, changelog completeness, error-code enum check, and the new command-sync gate.--cov ... --cov-report=term-missing) but informational — no--cov-fail-under, so it never blocks a merge.New silent-drift gate (
scripts/check_command_sync.py)Treats the live Typer command tree as the single source of truth; fails if any command is missing from
permissions.pyOPERATION_REGISTRY,CLAUDE.md## All CLI Commands,commands/context.pyAGENT_CONTEXT, orcommands-reference.md, or if a registry key matches no live command (renamed/removed). Calibrated to zero false positives:sl),gotchas.md (since vX.Y.Z)tagging is intentionally left to/kbagent:review— distinguishing a new version-specific gotcha from a version-independent section needs judgement, not a deterministic gate.E2E (
e2e.yml)Nightly cron (
17 3 * * *UTC) +workflow_dispatchagainst a dedicated throwaway project — not per-PR (too slow/flaky, and it mutates real resources). Skips gracefully (green + warning) whenE2E_API_TOKEN/E2E_URLsecrets are absent.Incidental fixes / docs
tyerrors in tests (missing None-guards / aNoneclient factory) surfaced by gatingtyin CI.permissions,sharing,project info) incommands-reference.md.make check/test-cov/command-sync-checkin the local-CI list.Verification
make checkgreen (lint, format, ty, skill, version, command-sync, changelog, error-codes).check_command_sync.pyis covered bytests/test_check_command_sync.py(8 tests: live-tree-clean guard + per-drift-class detection + granularity).Out of scope (by design)
/kbagent:review.