fix(data-app): read setup.sh code, not its comments, in validate-repo - #636
Merged
Conversation
`validate-repo` greps keboola-config/setup.sh for `pip install` and blocks
the repo when it matches. It greps the raw file, so a COMMENT warning
against the wrong installer -- exactly what a careful author writes above
the right one -- is read as the violation it warns against:
#!/bin/bash
# always uv sync here, never pip install
uv sync
That repo reports BLOCKING while being entirely correct. The rule penalises
the author who documents it.
The same reading runs the other way for the companion rule: a comment merely
mentioning `uv sync` satisfied the check that the script actually invokes it,
so a script that installs nothing could pass.
Both rules ask what the script RUNS, so both now read comment-stripped code.
`_strip_shell_comments` is deliberately not a shell parser -- it tracks
single/double quotes so a `#` inside a string survives, and treats a `#` at
line start or after whitespace as opening a comment. That covers real
setup.sh files without pretending to handle heredocs or ${...#...}.
Worth noting the surrounding comment in the module already justifies these
heuristics on the grounds that "false positives are operationally cheap (a
WARN, not a BLOCKING)" -- which is true of the other regexes there, but this
one emits BLOCKING, so that justification never covered it.
Found while building a sample data app: the repo was rejected over its own
comment.
padak
added a commit
that referenced
this pull request
Aug 22, 2026
…rade to `upgrade_command` in `kbagent version --json` is documented as a string a consumer may shell out to verbatim, but `get_kbagent_version_info()` built it unconditionally. Both non-False states of `up_to_date` therefore handed out an actively wrong command: * `true` (local >= latest). A caller on a pre-release compares ahead of the stable release a non-`--beta` fetch returns (`0.44.0b1` >= `0.43.3`), so it read `up_to_date: true` beside a `--force --reinstall` command pinned to the OLDER stable wheel. Running it is a silent downgrade off the beta -- the exact foot-gun the beta channel's three gates exist to prevent. * `null` (release feed unreachable). `resolve_kbagent_wheel_url(None)` yields no asset, so it fell through to the unpinned `git+` source install, resolving whatever the default branch happens to be -- on the one code path reached precisely because kbagent could not establish what the current release is. `prepare_kbagent_update_plan()` already gated on `up_to_date is False`, so `kbagent update` itself was never affected; only the reported string was. The frozen (PyInstaller) channel branch is untouched -- `brew upgrade` and friends are no-ops when current, not downgrades, and `install_channel` consumers rely on that shape. Two tests added; both fail on the parent commit. Also in this commit, for the 0.88.0 release span: - changelog: `(#issue)` decorations on all 24 bullets. The `SDK:` prefix is not in `_PREFIX_RE`, so that bullet rendered uncoloured -- it becomes `New (#622):` with the SDK context kept in the sentence. - changelog: entries for #636 and #637, which carried no version bump and so arrived with no changelog at all. - four first sentences shortened under the 160-char headline budget that `test_changelog_render.py` enforces -- two of them were pushed over by the decorations added here.
padak
added a commit
that referenced
this pull request
Aug 22, 2026
…rade to `upgrade_command` in `kbagent version --json` is documented as a string a consumer may shell out to verbatim, but `get_kbagent_version_info()` built it unconditionally. Both non-False states of `up_to_date` therefore handed out an actively wrong command: * `true` (local >= latest). A caller on a pre-release compares ahead of the stable release a non-`--beta` fetch returns (`0.44.0b1` >= `0.43.3`), so it read `up_to_date: true` beside a `--force --reinstall` command pinned to the OLDER stable wheel. Running it is a silent downgrade off the beta -- the exact foot-gun the beta channel's three gates exist to prevent. * `null` (release feed unreachable). `resolve_kbagent_wheel_url(None)` yields no asset, so it fell through to the unpinned `git+` source install, resolving whatever the default branch happens to be -- on the one code path reached precisely because kbagent could not establish what the current release is. `prepare_kbagent_update_plan()` already gated on `up_to_date is False`, so `kbagent update` itself was never affected; only the reported string was. The frozen (PyInstaller) channel branch is untouched -- `brew upgrade` and friends are no-ops when current, not downgrades, and `install_channel` consumers rely on that shape. Two tests added; both fail on the parent commit. Also in this commit, for the 0.88.0 release span: - changelog: `(#issue)` decorations on all 24 bullets. The `SDK:` prefix is not in `_PREFIX_RE`, so that bullet rendered uncoloured -- it becomes `New (#622):` with the SDK context kept in the sentence. - changelog: entries for #636 and #637, which carried no version bump and so arrived with no changelog at all. - four first sentences shortened under the 160-char headline budget that `test_changelog_render.py` enforces -- two of them were pushed over by the decorations added here.
padak
added a commit
that referenced
this pull request
Aug 22, 2026
…rade to `upgrade_command` in `kbagent version --json` is documented as a string a consumer may shell out to verbatim, but `get_kbagent_version_info()` built it unconditionally. Both non-False states of `up_to_date` therefore handed out an actively wrong command: * `true` (local >= latest). A caller on a pre-release compares ahead of the stable release a non-`--beta` fetch returns (`0.44.0b1` >= `0.43.3`), so it read `up_to_date: true` beside a `--force --reinstall` command pinned to the OLDER stable wheel. Running it is a silent downgrade off the beta -- the exact foot-gun the beta channel's three gates exist to prevent. * `null` (release feed unreachable). `resolve_kbagent_wheel_url(None)` yields no asset, so it fell through to the unpinned `git+` source install, resolving whatever the default branch happens to be -- on the one code path reached precisely because kbagent could not establish what the current release is. `prepare_kbagent_update_plan()` already gated on `up_to_date is False`, so `kbagent update` itself was never affected; only the reported string was. The frozen (PyInstaller) channel branch is untouched -- `brew upgrade` and friends are no-ops when current, not downgrades, and `install_channel` consumers rely on that shape. Two tests added; both fail on the parent commit. Also in this commit, for the 0.88.0 release span: - changelog: `(#issue)` decorations on all 24 bullets. The `SDK:` prefix is not in `_PREFIX_RE`, so that bullet rendered uncoloured -- it becomes `New (#622):` with the SDK context kept in the sentence. - changelog: entries for #636 and #637, which carried no version bump and so arrived with no changelog at all. - four first sentences shortened under the 160-char headline budget that `test_changelog_render.py` enforces -- two of them were pushed over by the decorations added here.
padak
added a commit
that referenced
this pull request
Aug 22, 2026
…eport fix (#638) Nothing between v0.87.0 and today had been published: #629 bumped pyproject to 0.88.0, #633 bumped it again to 0.89.0, and both changelog blocks sat unreleased. Ship the whole span as one release instead of publishing two versions retroactively. - changelog: merge the 0.89.0 bullets into 0.88.0, reordered so the #624 column-description fix leads -- the first bullet is what `kbagent changelog` renders as the one-line summary. Add (#issue) decorations to all 24 bullets; the `SDK:` prefix is not in \_PREFIX_RE so that bullet rendered uncoloured and becomes `New (#622):`. Add entries for #636 and #637, which carried no version bump and so arrived with no changelog at all. - pyproject 0.89.0 -> 0.88.0, propagated to plugin.json, marketplace.json and uv.lock via `make version-sync`. - rewrite every `0.89.0+` / `(since v0.89.0)` version gate across CLAUDE.md, the keboola-expert prompt, commands-reference.md, gotchas.md, context.py, docs/sdk.md and two test comments. A stale gate makes the agent refuse flags that do exist on the user's installed version. - fix: `kbagent version --json` no longer advertises an `upgrade_command` when there is nothing to upgrade to. It was built unconditionally, so a caller on a pre-release read `up_to_date: true` beside a `--force --reinstall` command pinned to the OLDER stable wheel (a silent downgrade), and an unreachable release feed produced an unpinned `git+` default-branch install. `kbagent update` itself was never affected. Two tests added; both fail on the parent.
padak
added a commit
that referenced
this pull request
Aug 22, 2026
Audited every 0.88.0 changelog bullet against the agent-facing reference surfaces (commands-reference.md, gotchas.md, the *-workflow.md set, keboola-expert.md). Everything was covered except one change, plus two entries that were accurate but incomplete. - gotchas: `data-app validate-repo`'s two setup.sh rules now read comment-stripped code (#636). This had NO coverage anywhere. It matters to an agent because it broke in both directions before 0.88.0: a script whose comment read `# never pip install` was rejected as BLOCKING, and a script whose only `uv sync` was in a comment satisfied the dependency-install rule while installing nothing. An agent advising on a repo validated by an older kbagent needs to know a BLOCKING may be about a comment. The stripper's limits (not a shell parser; heredocs out of scope) are stated too, so nobody reads it as a guarantee. - commands-reference: the same note on the `validate-repo` entry. - commands-reference: the `table-detail` entry documented #621's `definition` thoroughly but said nothing about #624's READ side -- the resolution precedence into `column_details[].description` and the `legacy_column_descriptions` field. The `describe-column` entry pointed at table-detail for read-back; table-detail did not advertise it. - data-app-workflow: the same comment-stripping note in the pre-flight validation section. No behaviour change, docs only. `check_version_gates.py` now resolves 419 markers (up from 418).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
kbagent data-app validate-repochecks thatkeboola-config/setup.shdoes not use the pip installer (the data-app runtime blocks it;uv syncis the supported path). It enforces that by grepping the raw file:Comments are part of the raw file, so this setup.sh is rejected:
It does the right thing and says so, and the saying is what blocks it. The rule penalises precisely the author who documents it for their colleagues.
The same misreading runs the other way for the companion rule.
golden-rule.setup-sh-uv-syncwarns when pyproject declares dependencies but setup.sh does not invokeuv sync— and a comment merely mentioninguv syncsatisfied it. A script that installed nothing could pass.Worth flagging
The module comment above these regexes justifies the heuristics like this:
That holds for the other regexes there. It does not hold for this one:
setup-sh-no-pipemitsSEVERITY_BLOCKING. The stated rationale for tolerating imprecision never covered the one check where imprecision is expensive.The fix
Both rules ask what the script runs, so both now read comment-stripped code.
_strip_shell_commentsis deliberately not a shell parser. It tracks single/double quotes so a#inside a string survives, and treats a#at line start or after whitespace as opening a comment. That covers real setup.sh files without pretending to handle heredocs or${...#...}expansions — consistent with the heuristic-over-parser choice the module already made.Tests
Four added. Two of them target the bug and fail on
main:test_pip_install_only_in_a_comment_does_not_block— was BLOCKING, now OKtest_uv_sync_only_in_a_comment_still_warns— was OK, now WARNTwo are regression guards against over-stripping, and pass either way:
test_pip_install_with_a_trailing_comment_still_blocks— a real invocation with a trailing comment must not slip throughtest_hash_inside_a_quoted_string_is_not_a_comment— quoted#must not truncate the lineFull suite: 5832 passed, 61 skipped.
ruff check,ruff format --check,ty checkclean.Notes
Found while building a sample data app for the E2E fixtures — the repo was rejected over its own comment. No version bump or changelog entry; the version is being bumped once across several PRs.
Related but not addressed here:
validate-reponever inspects the nginxlistenport, which is a hardcoded platform contract (8888). A repo can report 0 BLOCKING and still fail to start. Happy to open that separately.