serve: honor the root-level --config-dir, and fix two stale permissions comments - #681
Merged
Conversation
…nt claim Two corrections in OPERATION_REGISTRY's comments, both fallout of #677: - `auth.projects` still carried `(since vNEXT)` after the 0.90.1 release. The release-time gate (scripts/check_version_gates.py) only scans markdown surfaces -- CLAUDE.md, docs/*.md, plugins/**/*.md -- so a placeholder in src/ resolves nowhere and ships silently. Pinned to 0.90.1, the release that actually carries the endpoint. - The `http.*` block claimed "the serve's own routes enforce their own permissions on top". Issue #655 flagged that as load-bearing and false: it was true for no route at all. Since 0.90.1 it is true for /auth/* and nothing else, so the comment now states that scope instead of a blanket claim that misleads the risk classification directly beneath it.
zajca
reviewed
Aug 23, 2026
zajca
left a comment
Member
There was a problem hiding this comment.
Selected findings from the automated read-only review.
`serve` is the only subcommand carrying a `--config-dir` of its own, and it forwarded only that one into `create_app`. The root-level `kbagent --config-dir A serve` was ignored entirely, with no warning: the server exposed whatever the fallback chain resolved (usually the global config) instead of the directory the caller named. Since 0.90.1, when the `/auth/*` routes began enforcing the persisted `permissions` policy, that also meant a deny policy stored beside the caller's projects was silently not the one in force -- verified live: `POST /auth/register-projects` answered 401 (the other directory's session state) where the named directory's policy says 403. Precedence is now most-specific-wins, the same rule `kbagent repl` applies when forwarding root flags into a subcommand: `serve --config-dir X` beats a root `kbagent --config-dir Y`, which beats KBAGENT_CONFIG_DIR / the .kbagent walk-up / global. Passing both is not an error. Only an explicit `cli-flag` source propagates -- for env-var/local/global, `create_app` resolves the identical directory itself, and forwarding those would pin a resolution made at a different moment. Tests cover all four flag combinations plus the env-var non-propagation, and one end-to-end case proving a deny policy in the root-flag directory reaches the REST surface. Docs updated on docs/web-server.md, CLAUDE.md, gotchas.md, commands-reference.md and the `kbagent context` text; the new behavior is gated `(since vNEXT)` for the release PR to resolve.
The comment justifying the `!= 403` assertion said "there is no session in this fixture dir", implying a SESSION_NOT_FOUND / 401 path. Verified: the response is HTTP 400 CONFIG_ERROR -- the fixture dir has no default project and the request carries no `?stack=`, so `AuthService.status()` -> `_resolve_stack_url()` raises ConfigError before any session lookup happens. Comment rewritten to name that mechanism and to lead with the point it is there to make: `/auth/status` is gated by a different registry key (`auth.status`), which the deny policy does not name, so a non-403 proves the denial is narrow rather than a blanket failure of the whole REST surface. The assertion is unchanged -- it was correct.
The previous commit rewrote a `(since vNEXT)` that survived the 0.90.1 release, but not the reason it survived: check_version_gates.py resolved its file list from SCANNED_GLOBS, which listed markdown plus exactly one hand-picked Python file (commands/context.py). permissions.py was never read, so the placeholder was invisible to the very check built to catch it -- and the next feature PR writing a gate in a Python comment would have shipped the same defect. Fixing only the symptom leaves the hole. A version gate in a Python comment is agent-facing documentation exactly like a markdown one, so src/**/*.py is now scanned as a whole (it subsumes the context.py entry). Verified against the 0.90.1 tree: the widened scan reports permissions.py:29, the marker that shipped. Its first run on the current tree also surfaced a real, pre-existing defect -- `(since v0.26.1)` on the project members & invitations block. 0.26.1 never existed: it is not a CHANGELOG key, `git tag -l "v0.26*"` lists only v0.26.0, and `gh release view v0.26.1` reports no release. The feature shipped in 0.29.0 (CHANGELOG 0.29.0 describes the seven `project` member/invitation commands; the commit that added this very block is c7d7078 "feat(0.29.0): ... + member lifecycle", drafted as 0.26.1 and renumbered -- the docs were rewritten at release, the Python comment was not). Corrected to v0.29.0. scripts/*.py is deliberately left out and the constant now says why: this module has to NAME the placeholder it hunts (its --release usage line, the VNEXT_TOKEN constant), so scanning it self-flags forever. INLINE_CODE_RE also learns double-backtick spans. Python docstrings here use RST convention, so the first ``(since vNEXT)`` written in one would otherwise be a false positive. Measured: markdown residue is unchanged at 7, i.e. no live gate is lost to over-stripping. The double-backtick alternative comes first because alternation is left-biased -- the single-backtick pattern would match the empty span between the two opening backticks and leave the token exposed. Gate counts: 67 files / 476 markers -> 295 files / 529 markers.
…stry The block opened with "GET = read, mutating verbs = write" and then classified `http.delete` as destructive three lines below it. DELETE is a mutating verb, so a reader auditing the four keys against the stated rule hits a contradiction on the strictest one -- and this comment is the security rationale those keys are audited against. State the mapping that the registry actually implements, in the taxonomy declared at the top of the file (read = no side effects, write = creates/modifies, destructive = deletes): GET = read, POST/PATCH = write, DELETE = destructive. The extra rung is load-bearing, not cosmetic: apply_firewall_flags adds `cli:destructive`, which matches only the destructive class, so classifying http.delete as `write` would let a --deny-destructive session delete through the REST boundary.
The Windows job failed on the new test with UnicodeEncodeError: cp1252 is the platform default there, and the fixture mirrors a real repo line whose section rule is box-drawing characters. Reproduced locally -- write_text(body, encoding='cp1252') raises exactly the CI error, and the same bytes read back as cp1252 turn '--' rules into mojibake. The read side matters beyond the test. Widening SCANNED_GLOBS to src/**/*.py put 73 non-ASCII-carrying source files in front of a read_text() that never named an encoding, so on Windows the scan would regex over mangled text -- and since INLINE_CODE_RE keys on backticks, mojibake can move or destroy the very characters that separate a live placeholder from prose quoting one. A gate that classifies differently per OS is worse than no gate. Both read_text() calls in the script and every read/write in its tests now name UTF-8 explicitly.
padak
added a commit
that referenced
this pull request
Aug 25, 2026
Moves the (since vNEXT) tags from three section headings -- the two new ignored-components sections plus the pre-existing serve --config-dir gotcha from #681 -- to the sections' first body line, so the heading anchor slugs stay stable when the placeholder resolves at release.
padak
added a commit
that referenced
this pull request
Aug 25, 2026
Moves the (since vNEXT) tags from three section headings -- the two new ignored-components sections plus the pre-existing serve --config-dir gotcha from #681 -- to the sections' first body line, so the heading anchor slugs stay stable when the placeholder resolves at release.
padak
added a commit
that referenced
this pull request
Aug 25, 2026
Moves the (since vNEXT) tags from three section headings -- the two new ignored-components sections plus the pre-existing serve --config-dir gotcha from #681 -- to the sections' first body line, so the heading anchor slugs stay stable when the placeholder resolves at release.
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
2 participants
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.
Closes #679.
1.
kbagent serveignored the root-level--config-dirserveis the only subcommand in the repo that defines its own--config-dir, and it forwarded only that one intocreate_app. The directory the root callback had already resolved intoctx.obj["config_store"]was never read, and the callback exports noKBAGENT_CONFIG_DIR, so there was no indirect channel either.kbagent --config-dir X servetherefore served whatever the default chain found — silently, with no warning.Two consequences, both reproduced live before the fix:
kbagent --config-dir dirA serveanswered/projectswith the 51 aliases of the global config instead of dirA's singlealpha-only.create_appresolves, so a persisted deny policy stored next to the projects the caller named was not read at all. In the end-to-end fixture,POST /auth/register-projectsreturned 401 SESSION_NOT_FOUND (policy never loaded) where it should have returned 403 PERMISSION_DENIED.Fix — most-specific-wins precedence, matching the pattern
commands/repl.pyalready uses when forwarding the root flag into a subcommand:serve --config-dir Xgiven → serve X (unchanged; every existing invocation keeps working)--config-dir Ygiven explicitly → serve Y (this is the fix)KBAGENT_CONFIG_DIR→.kbagentwalk-up → global (unchanged)"Given explicitly" is read off
ConfigStore.source == "cli-flag"rather than by comparing or re-resolving paths. Only that source travels: forenv-var/local/global, lettingcreate_appresolve on its own yields the identical directory, and propagating them would pin a resolution made at a different moment. Deliberately no fail-fast and no warning when both flags are given and differ — that would break invocations that work today, and the precedence is unambiguous.The serve
--config-dirhelp text now states the precedence; its old wording ("matches kbagent --config-dir") is what made the bug plausible in the first place.Verification
Live A/B against the real CLI with two fixture config dirs, plus 6 new tests in
tests/test_server_permissions.py— five precedence cases (root-only, serve-only, both, neither, env-var-set-without-root-flag) asserting theconfig_dirkwargcreate_appreceives, and one end-to-end test that persists a deny policy in the served directory and asserts403 PERMISSION_DENIEDonPOST /auth/register-projectswhile a sibling operation outside the policy is not denied. A negative control confirmed exactly the two new-behavior tests fail without the fix.--config-dir dirA servealpha-onlyserve --config-dir dirAalpha-onlyalpha-only2. Two comment corrections in
permissions.pyFound while filing #679; both are fallout of #677.
(since vNEXT)placeholder shipped in 0.90.1.auth.projectsstill carried it onmain, becausescripts/check_version_gates.pyscannedCLAUDE.md,docs/*.mdandplugins/kbagent/**/*.mdonly — a placeholder anywhere undersrc/was invisible to it and shipped unresolved. Pinned to0.90.1, and the cause is fixed too (below).3. The version gate now scans
src/**/*.pyAdded on review: pinning the placeholder alone would let the next feature PR ship the same defect, since writing
(since vNEXT)in a Python comment is a patternpermissions.pyitself establishes as normal. Verified the widened scan would have caught the original — run againstgit show 11724656:src/keboola_agent_cli/permissions.py, it flags line 29. Scanned files 67 → 295, gates 476 → 529,vNEXTresidue unchanged at 7.scripts/*.pystays out, deliberately: the gate script must name the token it hunts (VNEXT_TOKEN = "vNEXT", its own--releaseusage example), so scanning itself self-flags forever. The comment says so, to stop the next person retrying it.INLINE_CODE_REalso learned RST double-backtick spans, since Python docstrings use them and a(since vNEXT)in one would otherwise false-positive. Measured: markdown residue identical at 7, so no live gate is lost. The double-backtick alternative must come first in the alternation — with the single-backtick one first, the pattern matches the empty span between the two opening backticks and leaves the token exposed.The wider scan immediately found a real pre-existing defect:
commands/project.py:839carried(since v0.26.1), a version that was never released (git tag -l "v0.26*"→ onlyv0.26.0; no such GitHub release; not aCHANGELOGkey). Corrected tov0.29.0—CHANGELOG["0.29.0"]describes those seven member/invitation commands verbatim, and the commit that added the line isc7d70788 feat(0.29.0): … + member lifecycle (#256), whose message still carries the pre-renumber(since v0.26.1)draft. Same failure mode as the 0.90.1 placeholder, three months older.http.*block claimed serve-side enforcement that did not exist. It read "The serve's own routes enforce their own permissions on top." — serve: PermissionEngine firewall is not enforced on REST routes #655 flagged this as load-bearing and false, and it was: before serve: expose auth register-projects over REST (POST /auth/register-projects) #677 no serve route consulted the engine, so anhttp.postclassifiedwritemet no second check anywhere. Since 0.90.1 it is true for/auth/*and nothing else, so the comment now says exactly that and names serve: PermissionEngine firewall is not enforced on REST routes #655.Docs
docs/web-server.md,CLAUDE.mdandgotchas.mdcarried #677's stopgap advice ("pass--config-dirtoserveitself"), which describes the bug rather than the intended behavior — rewritten to the fixed precedence, with the old trap kept intact for readers on 0.90.1.commands-reference.mdandcontext.py'sAGENT_CONTEXTare on CONTRIBUTING's synchronization map for flag-behavior changes and were updated too. New behavior is gated with the literal(since vNEXT);context.pydeliberately carries no version tag, since a placeholder in a.pyfile is exactly what the gate cannot resolve.Testing
make checkgreen: 6123 passed, 12 skipped.tyclean for every touched file (one pre-existing unrelated diagnostic inscripts/hatch_build.pyremains).check_command_sync.py,check_version_gates.py, ruff check + format all OK. The version-gate tests grew by 9, mutation-checked to fail against the pre-change state. No version bump, no changelog entry.