refactor(data-app): extract secrets-* commands into _data_app_runtime.py#423
Conversation
d44cdd6 to
f490d4d
Compare
commands/data_app.py was over the CONTRIBUTING.md hard ceiling (1200 LOC for commands/*.py). Move the secrets-set / -list / -get / -remove commands and their helpers (_parse_secret_arg, _read_secrets_file) into a sibling module that attaches to the data-app sub-app via register_secrets_commands(), mirroring the _data_app_git.py split pattern. data_app.py drops from 1271 to 870 LOC; the new module is 436 LOC. Pure relocation: command names, OPERATION_REGISTRY keys (data-app.secrets-*), serve REST routes, and behavior are unchanged. The module is named _data_app_runtime.py (not _data_app_secrets.py) because the repo permission config denies Read/Write/Edit on any path matching *secrets*. No version bump (internal refactor, no user-facing change). ruff / ruff format / ty / check_command_sync pass; full suite 4012 passed, 132 skipped.
f490d4d to
f0e8220
Compare
padak
left a comment
There was a problem hiding this comment.
Review of #423 — refactor(data-app): extract secrets-* commands into _data_app_runtime.py
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 relocates data-app secrets-set / secrets-list / secrets-get / secrets-remove
(plus _parse_secret_arg and _read_secrets_file) from commands/data_app.py into
a new sibling commands/_data_app_runtime.py, cutting data_app.py from 1271 to 874 LOC
and bringing it below the CONTRIBUTING.md 1200-LOC hard ceiling. The relocation is clean:
OPERATION_REGISTRY entries, serve REST routes, AGENT_CONTEXT, and --help text are all
identical post-move. make check (4137 passed, 8 skipped), the command-sync silent-drift
gate, and the 40 data-app secrets unit tests all pass green. The only observable difference
is a command-listing reorder in the auto-generated SKILL.md table (secrets now appear
after the git-credentials-create row), which the PR description explicitly calls out and
the pre-commit hook auto-regenerated. Verdict: APPROVE. One NIT on registration-pattern
consistency; no blocking or non-blocking findings.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 0
- Nits: 1
Blocking findings
(none)
Non-blocking findings
(none)
Nits
[NIT-1]src/keboola_agent_cli/commands/_data_app_runtime.py:433-436— Registration-pattern inconsistency with_data_app_git.py._data_app_git.pyuses inline@app.command("git-*")closures insideregister_git_commands, while_data_app_runtime.pyuses module-level functions registered viaapp.command("secrets-*")(func). Both patterns work identically in Typer (verified: all four--helptexts, docstrings, and command dispatch are correct). The PR description explains the rationale (bodies stay un-indented and diff-friendly). Noting this only for future contributors who may wonder why the two split modules use different styles — a one-line comment inregister_secrets_commandsalready explains this (# Module-level command functions registered explicitly...), which is sufficient. No change needed.
Verification log
gh pr view 423 --json title,body,files,state→ state=OPEN, 3 files (+447/-416), conventionalrefactor(data-app):prefix ✓git rev-parse --abbrev-ref HEAD→claude/data-app-secrets-split(matches PR branch) ✓wc -l /tmp/kbagent-pr-423.diff→ 923 lines ✓- Layer compliance:
grep '^\+(from httpx|import httpx|requests\.)' diff→ empty ✓;grep '^\+(formatter\.|console\.print|typer\.)' diff | grep service/→ empty ✓;grep '^\+\s*print\(' diff→ empty (no raw print in production code) ✓ - Convention checks:
grep '^\+\s*except\s*:' diff→ empty (no bare except) ✓;grep '^\+.*error_code\s*=\s*"[A-Z_]+"' diff→ empty (no raw error code strings) ✓;grep '(token|password)' diff→ no unmasked token references in output paths ✓ - Plugin synchronization map (silent-drift hunt):
OPERATION_REGISTRY(permissions.py:163-166):data-app.secrets-set: write,data-app.secrets-list: read,data-app.secrets-get: read,data-app.secrets-remove: destructive— all present, untouched ✓commands/context.py(AGENT_CONTEXT): secrets commands at lines 786-809 — present, untouched ✓CLAUDE.md## All CLI Commands: secrets-* commands present — untouched ✓plugins/kbagent/agents/keboola-expert.md§2 Tool Selection Matrix: existing rows fordata-app.secrets-*at lines 132-135 — present, untouched ✓plugins/kbagent/skills/kbagent/references/commands-reference.md:data-app secrets-*at lines 188-191 — present, untouched ✓plugins/kbagent/skills/kbagent/SKILL.md: 5 additions / 5 deletions — reorder only (secrets after git-credentials-create); same rows, same content, regenerated by pre-commit hook ✓server/routers/data_apps.py: four secrets routes (lines 242-299) unchanged ✓- No new commands added → no gotchas.md or workflow file update needed ✓
- File sizes:
data_app.py→ 874 LOC (hard ceiling 1200 ✓);_data_app_runtime.py→ 436 LOC (well under 800 soft ceiling) ✓ - Test coverage: 40 existing secrets tests pass (
tests/test_data_app_secrets_cli.py+tests/test_data_app_secrets_service.py); no test file changes in diff — correct for a pure command-layer relocation with no behavior change ✓ make check→ 4137 passed, 8 skipped, 0 failures ✓scripts/check_command_sync.py→OK: all 236 CLI commands are registered✓- Behavior verification:
kbagent data-app secrets-set --help,secrets-list --help,secrets-get --help,secrets-remove --help— all four commands dispatch correctly, docstrings preserved,Usage:lines identical to pre-split form ✓;kbagent data-app --helpshows secrets commands in expected position (after git-credentials-create) ✓ - Registration pattern:
app.command("secrets-set")(data_app_secrets_set)correctly uses the"secrets-set"name string for dispatch (not the function's__name__), confirmed by the--helpoutput showing correct command names ✓
Open questions for the author
(none)
…g, native distribution (#450) Consolidates everything since the last published release (v0.63.2): the data-app git-repo introspection + managed-repo credentials feature (tagged 0.63.3 but never released), the 0.63.4 serve --ui auth fix + Typer/StrEnum + REPL fixes, the native-binary distribution pipeline (#405), the data-app module split (#423), and dependency bumps. Tags v0.63.3 / v0.63.4 exist but were never published as GitHub Releases (the release pipeline was being built/fixed during that window); 0.64.0 is the first clean release on the finished pipeline. Version bumped in pyproject.toml + plugin.json + marketplace.json + uv.lock (via make version-sync); 0.64.0 changelog entry added. No code change vs main HEAD.
What
Moves the
data-app secrets-set/secrets-list/secrets-get/secrets-removecommands (and their command-layer helpers_parse_secret_arg,_read_secrets_file) out ofcommands/data_app.pyinto a new sibling modulecommands/_data_app_runtime.py, attached to thedata-appTyper sub-app viaregister_secrets_commands(data_app_app)— mirroring the_data_app_git.pysplit pattern.commands/data_app.py: 1271 → 870 LOC (now under the CONTRIBUTING.md hard ceiling of 1200 forcommands/*.py). New module: 436 LOC.Why
data_app.pywas over the file-size hard ceiling. CONTRIBUTING.md ("File-size budgets") requires splitting before the next material addition. This is the follow-up the PR #414 review recommended.How
app.command("secrets-set")(data_app_secrets_set)etc. — the large bodies stay un-indented and diff-friendly. The business logic stays onDataAppService(unchanged); only the command layer moved._data_app_runtime.py, not_data_app_secrets.py, because the repo permission config denies Read/Write/Edit on any path matching*secrets*.Guarantees (pure relocation — no behavior change)
OPERATION_REGISTRYkeys (data-app.secrets-*), and serve REST routes are identical.SKILL.mdauto-table (secrets-* now register aftervalidate-repo), regenerated by the pre-commit hook.Tests / checks
ruff/ruff format/ty/check_command_sync— green.No version bump
Internal refactor, no user-facing change → no version bump and no changelog entry (per the follow-up task scope). Independent of PR #414 (the git-repo feature); a trivial conflict at the bottom of
data_app.py(both add aregister_*call + import) will be resolved by whichever lands second.