diff --git a/CLAUDE.md b/CLAUDE.md index 2e31c8c6..6842c8ad 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -401,10 +401,13 @@ kbagent auth register-projects [--stack URL|alias] [--all] [--project-id ID ...] # is also the FIRST router to enforce the `permissions` policy: every route declares # `Depends(require_permission(...))`, so a denial answers HTTP 403 `PERMISSION_DENIED` over REST # exactly as on the CLI. The other ~30 routers do not check the engine yet. -# The policy comes from the config dir `serve` RESOLVES (its own `--config-dir`, then -# KBAGENT_CONFIG_DIR, then the local/global chain) -- not from the root callback's `--config-dir` -# -- plus the session flags of the invocation. Reachable ways to enforce: a persisted narrow -# policy (`kbagent permissions set --mode allow --deny auth.register-projects`, needs a real +# The policy comes from the config dir `serve` RESOLVES (its own `--config-dir`, then -- since +# vNEXT, #679 -- an explicit root `kbagent --config-dir`, then KBAGENT_CONFIG_DIR, then the +# local/global chain) plus the session flags of the invocation. On 0.90.1 the root-level +# `--config-dir` was IGNORED here, so a policy stored beside the projects the caller named was +# silently not the one enforced; on that version pass --config-dir to `serve` itself. +# Reachable ways to enforce: a persisted narrow policy +# (`kbagent permissions set --mode allow --deny auth.register-projects`, needs a real # terminal for the confirmation code), or `--mode deny` with `serve` (and the reads you want) in # the allow list. `kbagent --deny-writes serve` does NOT work: `serve` is admin-class and # `--deny-writes` appends `cli:write`, which spans write+destructive+admin, so the CLI callback @@ -1000,6 +1003,14 @@ kbagent update [--beta] kbagent changelog [--limit N] [--full] # Default shows a one-line summary (first sentence) per version; --full / -v expands every note. kbagent serve [--host HOST] [--port PORT] [--ui] [--ui-dist PATH] [--reload] [--log-level LVL] [--cors-origin ORIGIN] [--config-dir DIR] [--no-banner] +# `--config-dir` on serve (since vNEXT, #679): `serve` is the only subcommand with a --config-dir of +# its own, and most specific wins -- `serve --config-dir X` beats a root `kbagent --config-dir Y`, +# which in turn beats KBAGENT_CONFIG_DIR / the .kbagent walk-up / global. Passing both is NOT an +# error. Up to 0.90.1 the ROOT flag was ignored by serve entirely, silently: `kbagent --config-dir A +# serve` exposed a different project set than the caller named, and (since 0.90.1's /auth/* +# enforcement) applied a different directory's `permissions` policy. On <=0.90.1 always pass +# --config-dir to `serve` itself. Only an explicit root flag propagates -- an env-var/walk-up/global +# resolution is left to the server, which reaches the identical directory on its own. # `--no-banner` (since 0.90.0): suppress the web UI's "What's new" popup. The UI shows a curated # per-version highlights modal once per version (localStorage `kbagent.whatsnew.seen`); this flag # turns the UNSOLICITED popup off fleet-wide. Surfaced to the SPA via `GET /ui-config` diff --git a/docs/web-server.md b/docs/web-server.md index 9c7076f9..e1bebc9a 100644 --- a/docs/web-server.md +++ b/docs/web-server.md @@ -516,10 +516,11 @@ carried. Two consequences worth knowing before you reach for a flag: policy works too, but its allow list must then include `serve` (and the reads you want to keep), or the server will not start for the same reason as above. - Pass `--config-dir` **to `serve`**: the server resolves its own config dir - (`--config-dir` on the `serve` command, then `KBAGENT_CONFIG_DIR`, then the - local/global chain), so a root-level `kbagent --config-dir ... serve` sets the - directory for the CLI invocation, not for the served process. + Either spelling of `--config-dir` selects the served directory *(since + vNEXT)* — see [Which config directory `serve` + uses](#which-config-directory-serve-uses) below. On **0.90.1 and older**, + only `serve --config-dir` did: a root-level `kbagent --config-dir ... serve` + was ignored, so the policy above was silently not the one enforced. A missing or expired session reaches `GET /auth/projects` and `POST /auth/register-projects` as a **thrown error**, both funnelled through @@ -600,6 +601,27 @@ the same view of Keboola the operator does, it can call any endpoint in the reference, and its full response (including any tools it called) is captured into the run history. +### Which config directory `serve` uses + +`serve` is the only subcommand with a `--config-dir` of its own, so there are +two places the flag can appear. Most specific wins *(since vNEXT)*: + +1. `kbagent serve --config-dir X` → serves `X`. +2. `kbagent --config-dir Y serve` → serves `Y`. +3. Neither → `KBAGENT_CONFIG_DIR`, then the `.kbagent` walk-up from the CWD, + then the global directory (`config_store.resolve_config_dir`). + +Giving both is not an error — the `serve`-level flag simply wins, as in rule 1. +Only an explicit root flag is forwarded; an env-var/walk-up/global resolution is +left to the server, which reaches the identical directory on its own. + +> **On 0.90.1 and older, rule 2 did not exist** (issue #679): the root-level +> `--config-dir` was ignored by `serve` entirely, with no warning. The server +> then exposed a different set of projects than the caller named, and — once +> `/auth/*` began enforcing `permissions` in 0.90.1 — enforced a different +> directory's policy. On those versions always pass `--config-dir` to `serve` +> itself. + ### State on disk Everything the server persists lives under one config directory diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index a7c9dfaf..9cdfc5f2 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -440,7 +440,7 @@ CLI parity for the `/agents` REST surface. Reads/writes `/agents.jso - `doctor` -- health checks (no `--fix` since v0.85.0 -- it only installed the MCP server). Includes a `sync_secrets` check (since 0.55.0): when run inside a sync working tree (`.keboola/manifest.json`), warns if any in-sync config holds plaintext `#`-secrets (#378); `skip` outside a sync tree. Also an `mcp_tool_tasks` check: FAILs when `agents.json` holds tasks using the removed `mcp_tool` action, listing each task in `details.tasks[]` -- map the tool to its native command via `docs/mcp-migration.md` and recreate the task as `--type cli_command` - `version` -- show the kbagent version and check for kbagent updates (kbagent only since v0.85.0); on a standalone binary it advertises the native channel's upgrade command instead of `kbagent update` (v0.79.0+) - `context` -- full usage instructions for AI agents -- `serve [--host HOST] [--port PORT] [--ui] [--ui-dist PATH] [--reload] [--log-level LVL] [--cors-origin ORIGIN] [--config-dir DIR] [--no-banner]` -- launch the FastAPI HTTP server backing the web UI. `--ui` mounts the built SPA at `/` and authenticates the browser with an HttpOnly `kbagent_session` cookie (nothing is injected into `index.html`). `--no-banner` *(since 0.90.0)* suppresses the UI's "What's new" popup -- a curated per-version highlights modal shown once per version, dismissal persisted in localStorage `kbagent.whatsnew.seen`. The SPA reads the switch from `GET /ui-config` -> `{"banner": bool}`; it gates only the UNSOLICITED popup, so the command palette's "What's new" action still opens it on request. Requires the optional `server` extra +- `serve [--host HOST] [--port PORT] [--ui] [--ui-dist PATH] [--reload] [--log-level LVL] [--cors-origin ORIGIN] [--config-dir DIR] [--no-banner]` -- launch the FastAPI HTTP server backing the web UI. `--ui` mounts the built SPA at `/` and authenticates the browser with an HttpOnly `kbagent_session` cookie (nothing is injected into `index.html`). `--no-banner` *(since 0.90.0)* suppresses the UI's "What's new" popup -- a curated per-version highlights modal shown once per version, dismissal persisted in localStorage `kbagent.whatsnew.seen`. The SPA reads the switch from `GET /ui-config` -> `{"banner": bool}`; it gates only the UNSOLICITED popup, so the command palette's "What's new" action still opens it on request. `--config-dir` picks the directory the server exposes (projects AND the `permissions` policy the `/auth/*` routes enforce); *(since vNEXT)* an explicit root-level `kbagent --config-dir Y serve` is honored too, with the serve-level flag winning when both are given -- on 0.90.1 and older the root-level one was silently ignored, so pass it to `serve` itself there. Requires the optional `server` extra ## Global Flags | Flag | Description | diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 79993bab..a4e9e0df 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -4475,7 +4475,8 @@ though single-project `job list` looked correctly time-ordered. ## `kbagent serve` permission enforcement is `/auth/*`-only so far (since v0.90.1) `create_app` builds a `PermissionEngine` from the persisted `permissions` -policy of **the config dir `serve` resolves** (its own `--config-dir`, then +policy of **the config dir `serve` resolves** (its own `--config-dir`, then -- +since vNEXT -- an explicit root-level `kbagent --config-dir`, then `KBAGENT_CONFIG_DIR`, then the local/global chain), and `kbagent serve` forwards only the session FLAGS of the invocation on top. But of the ~30 routers, only the three `/auth/*` routes (`server/routers/auth.py`) declare @@ -4504,8 +4505,6 @@ shapes. A `--mode deny` policy works too, but then `serve` (and the reads you want to keep) must be in its allow list, or the server will not start either. - Pass `--config-dir` to `serve` itself: the root-level `kbagent --config-dir - ... serve` sets the dir for the CLI invocation, not for the server process. - **A deny policy does NOT firewall the whole REST surface.** `permissions set --mode deny --deny cli:write` blocks `POST /auth/register-projects` (HTTP 403, `error_code: PERMISSION_DENIED`) but @@ -4520,3 +4519,28 @@ shapes. - Treat this as a gap being closed incrementally, not the design end state: today a deny policy gates only `/auth/*`, not the ~30 other routers a session token can otherwise reach. + +## `serve` honors the root-level `--config-dir` (since vNEXT) + +`serve` is the only subcommand carrying a `--config-dir` of its own, so the +flag has two possible positions. The precedence is **most specific wins**, +matching what `kbagent repl` does with the root flags: + +1. `kbagent serve --config-dir X` -> serves `X`. +2. `kbagent --config-dir Y serve` -> serves `Y`. +3. neither -> `KBAGENT_CONFIG_DIR`, then the `.kbagent` walk-up, then global. + +Passing both is not an error and produces no warning -- rule 1 simply wins. +Only an *explicit* root flag is forwarded; an env-var / walk-up / global +resolution is left to the server, which lands on the same directory anyway. + +- **On 0.90.1 and older, rule 2 did not exist** (issue #679) and nothing was + printed about it: `kbagent --config-dir A serve` served whatever rule 3 + resolved instead, so `GET /projects` listed a + different set of aliases than the caller named, and the `permissions` policy + enforced on `/auth/*` came from that other directory too. If you are on + <= 0.90.1, always pass `--config-dir` to `serve` itself. +- Quick check on any version: start the server and + `curl -s -H "Authorization: Bearer $KBAGENT_SERVE_TOKEN" localhost:PORT/projects` + -- if the aliases are not the ones in the directory you named, the flag was + in the position your version ignores. diff --git a/scripts/check_version_gates.py b/scripts/check_version_gates.py index c8f9994f..49d5c739 100644 --- a/scripts/check_version_gates.py +++ b/scripts/check_version_gates.py @@ -70,12 +70,25 @@ # The silent-drift surfaces from CONTRIBUTING.md's "Plugin synchronization map". # Anything an AI agent reads to decide whether a command exists belongs here. +# +# ``src/**/*.py`` is in scope because a version gate written in a Python +# comment is agent-facing documentation exactly like a markdown one -- and its +# absence is not hypothetical: ``(since vNEXT)`` in ``permissions.py`` survived +# the 0.90.1 release precisely because nothing under ``src/`` was scanned +# (``commands/context.py`` used to be listed alone; this glob subsumes it). +# Its first run also surfaced a stale ``(since v0.26.1)`` in ``commands/ +# project.py`` -- a version that never shipped, left behind by a renumber. +# +# ``scripts/*.py`` is deliberately NOT scanned. This module has to NAME the +# placeholder it is looking for (its ``--release`` usage line and the +# ``VNEXT_TOKEN`` constant), so it would flag itself forever -- a self- +# referencing failure no regex can tell apart from a real gate. Do not retry it. SCANNED_GLOBS: tuple[str, ...] = ( "CLAUDE.md", "docs/*.md", "plugins/kbagent/**/*.md", "plugins/kbagent/.claude-plugin/CLAUDE.md", - "src/keboola_agent_cli/commands/context.py", + "src/**/*.py", ) # ``(since v0.84.0)`` / ``(since 0.84.0)`` and ``0.73.0+`` / ``v0.73.0+``. @@ -85,10 +98,15 @@ # The placeholder a feature PR writes when it cannot know its release version. VNEXT_TOKEN = "vNEXT" -# An inline-code span. Stripping these before looking for VNEXT_TOKEN is what -# separates a live gate from prose quoting the token (see the module docstring -# for why this must not be applied to GATE_RE). -INLINE_CODE_RE = re.compile(r"`[^`]*`") +# An inline-code span, single- OR double-backtick. Stripping these before +# looking for VNEXT_TOKEN is what separates a live gate from prose quoting the +# token (see the module docstring for why this must not be applied to GATE_RE). +# The double-backtick alternative must come FIRST -- regex alternation is +# left-biased, so a single-backtick-first pattern would match the empty span +# between the two opening backticks of ``x`` and leave the token exposed. +# It matters because Python docstrings under ``src/`` use the RST convention: +# a ``(since vNEXT)`` written there is prose, not a gate, exactly as in markdown. +INLINE_CODE_RE = re.compile(r"``[^`]*``|`[^`]*`") # Fenced blocks are deliberately NOT stripped, even though the same "code means # quotation" argument seems to apply. Measured: CLAUDE.md's `## All CLI @@ -121,7 +139,15 @@ def find_vnext_residue(paths: list[Path]) -> list[VnextResidue]: rel = path.relative_to(REPO_ROOT).as_posix() except ValueError: rel = path.as_posix() - for lineno, line in enumerate(path.read_text(errors="replace").splitlines(), start=1): + # UTF-8 is pinned, never left to the platform default: on Windows that + # default is cp1252, and 73 of the files this now scans carry non-ASCII + # (em dashes, box-drawing rules in section comments). Decoded as cp1252 + # their bytes turn to mojibake, which can move or destroy the backticks + # INLINE_CODE_RE keys on -- so a real placeholder could read as quoted + # prose on one OS and as residue on another. + for lineno, line in enumerate( + path.read_text(encoding="utf-8", errors="replace").splitlines(), start=1 + ): if VNEXT_TOKEN not in line: continue if VNEXT_TOKEN in INLINE_CODE_RE.sub("", line): @@ -143,7 +169,9 @@ def collect_gates(paths: list[Path]) -> dict[str, list[tuple[str, int]]]: rel = path.relative_to(REPO_ROOT).as_posix() except ValueError: rel = path.as_posix() - for lineno, line in enumerate(path.read_text(errors="replace").splitlines(), start=1): + for lineno, line in enumerate( + path.read_text(encoding="utf-8", errors="replace").splitlines(), start=1 + ): for match in GATE_RE.finditer(line): version = match.group(1) or match.group(2) gates[version].append((rel, lineno)) diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index 4d6d4d49..c53c4599 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -1866,6 +1866,15 @@ subprocesses). Requires the optional 'server' extra: `uv pip install -e ".[server]"`. + Config directory served -- most specific wins: `serve --config-dir X`, + then an explicit root-level `kbagent --config-dir Y serve`, then the + normal chain (KBAGENT_CONFIG_DIR, .kbagent walk-up, global). Passing both + is not an error; the serve-level flag wins. That directory decides which + projects the REST surface exposes AND which persisted `permissions` + policy the /auth/* routes enforce. NOTE for older installs: up to 0.90.1 + `serve` ignored the root-level flag entirely, silently serving a + different directory -- there, always pass --config-dir to `serve` itself. + --no-banner (since 0.90.0) suppresses the web UI's "What's new" popup -- a curated per-version highlights modal the UI shows once per version (dismissal persisted in localStorage `kbagent.whatsnew.seen`). The SPA @@ -1993,6 +2002,8 @@ 8. Config resolution order: --config-dir flag > KBAGENT_CONFIG_DIR env > .kbagent/ in CWD/parents > ~/.config/keboola-agent-cli/ + `serve` is the only subcommand with a --config-dir of its own: its flag wins over the + root-level one, which wins over the rest of the chain above (see `kbagent serve`). 9. Historical MCP tool names: the `tool` group was removed in v0.85.0. If a user or an old script names a tool (get_configs, query_data, ...), map it to diff --git a/src/keboola_agent_cli/commands/project.py b/src/keboola_agent_cli/commands/project.py index 6fe3c340..a0a0f1a7 100644 --- a/src/keboola_agent_cli/commands/project.py +++ b/src/keboola_agent_cli/commands/project.py @@ -836,7 +836,7 @@ def project_info( formatter.output(result, _format_info_table) -# ── Project members & invitations (since v0.26.1) ───────────────────── +# ── Project members & invitations (since v0.29.0) ───────────────────── def _format_invite_result(console: Console, data: dict[str, Any]) -> None: diff --git a/src/keboola_agent_cli/commands/serve.py b/src/keboola_agent_cli/commands/serve.py index 5be59778..2ae37bf4 100644 --- a/src/keboola_agent_cli/commands/serve.py +++ b/src/keboola_agent_cli/commands/serve.py @@ -133,6 +133,39 @@ def _write_banner(text: str) -> None: sys.stdout.flush() +def _effective_config_dir(serve_config_dir: str | None, ctx: typer.Context) -> str | None: + """Pick the config directory ``create_app`` should serve. + + Most specific wins -- the same precedence `kbagent repl` applies when it + forwards the root flags into a subcommand's argv: + + 1. ``serve --config-dir X`` -- the subcommand's own flag, unchanged, so + every script written against the old behavior keeps working. + 2. root ``kbagent --config-dir Y serve`` -- returned here (issue #679). + Before this, `serve` ignored the root flag entirely and silently served + a different directory: the caller's projects were not the ones exposed, + and since the `/auth/*` routes started enforcing `permissions`, the + policy stored next to those projects was not read either. + 3. neither -- ``None``, leaving ``create_app`` to run its own fallback + chain (``KBAGENT_CONFIG_DIR`` -> ``.kbagent`` walk-up -> global). + + Only a ``cli-flag`` source is propagated. For ``env-var`` / ``local`` / + ``global``, ``create_app`` resolves the identical directory on its own, and + forwarding those would pin a resolution made at a different moment (a + different CWD for the walk-up, a since-changed env var) instead of the one + the server would make for itself. + + ``ctx.obj`` is ``None`` when the root callback never ran (a direct + invocation of the command in tests). + """ + if serve_config_dir is not None: + return serve_config_dir + root_store = (ctx.obj or {}).get("config_store") + if root_store is not None and root_store.source == "cli-flag": + return str(root_store.config_dir) + return None + + def serve_command( ctx: typer.Context, host: str = typer.Option( @@ -163,7 +196,11 @@ def serve_command( config_dir: str | None = typer.Option( None, "--config-dir", - help="Override config directory path (matches kbagent --config-dir).", + help=( + "Config directory to serve. Wins over a root-level " + "`kbagent --config-dir`; when neither is given, the server falls " + "back to KBAGENT_CONFIG_DIR, then the .kbagent walk-up, then global." + ), ), ui: bool = typer.Option( False, @@ -274,7 +311,7 @@ def serve_command( resolved_ui_dist = str(candidate) app = create_app( - config_dir=config_dir, + config_dir=_effective_config_dir(config_dir, ctx), auth_token=auth_token, cors_origins=cors, serve_url=serve_url, @@ -286,10 +323,10 @@ def serve_command( # surface; without them the flags would guard the CLI while every route # on the same process stayed wide open. Only the FLAGS travel -- the # persisted policy is loaded by create_app from the config dir it - # actually serves, because `--config-dir` here may point somewhere else - # than the root callback's own `--config-dir`, and the served - # directory's policy is the one that must apply. `ctx.obj` is None when - # the callback never ran (direct invocation in tests). + # actually serves, because `serve --config-dir` may still point + # somewhere else than the root callback's own `--config-dir`, and the + # served directory's policy is the one that must apply. `ctx.obj` is + # None when the callback never ran (direct invocation in tests). deny_writes=bool((ctx.obj or {}).get("deny_writes")), deny_destructive=bool((ctx.obj or {}).get("deny_destructive")), ) diff --git a/src/keboola_agent_cli/permissions.py b/src/keboola_agent_cli/permissions.py index 844cb901..c46fa16d 100644 --- a/src/keboola_agent_cli/permissions.py +++ b/src/keboola_agent_cli/permissions.py @@ -26,7 +26,7 @@ # tokens, never a real credential) -- same risk class as login/logout, # not the "admin" class `project add` uses for a pasted static token. "auth.register-projects": "write", - # Serve-only (since vNEXT): `GET /auth/projects` lists the session's + # Serve-only (since 0.90.1): `GET /auth/projects` lists the session's # registerable project candidates. It has no CLI leaf command -- the # terminal equivalent is the interactive picker inside # `auth register-projects` -- so it is exempted from the dead-key check @@ -322,8 +322,15 @@ "semantic-layer.reference-data.set": "write", "semantic-layer.reference-data.delete": "destructive", # Raw HTTP client against `kbagent serve` (used by AI subprocesses). - # Categorised by the underlying HTTP method: GET = read, mutating verbs - # = write. The serve's own routes enforce their own permissions on top. + # Categorised by the underlying HTTP method under the taxonomy at the top + # of this registry: GET = read, POST/PATCH = write (they create/modify), + # DELETE = destructive (it deletes). DELETE is deliberately a rung above + # the other mutating verbs -- lumping it in as `write` would let a + # `--deny-destructive` session delete through the REST boundary. + # These keys are the ONLY firewall an `http.*` call meets for + # most routes: on the serve side only `/auth/*` re-checks the policy + # (since 0.90.1) -- every other router is still unguarded, so a deny + # policy does not survive the REST boundary there (issue #655). "http.get": "read", "http.post": "write", "http.patch": "write", diff --git a/tests/test_check_version_gates.py b/tests/test_check_version_gates.py index 4bbd7117..5e7c3c79 100644 --- a/tests/test_check_version_gates.py +++ b/tests/test_check_version_gates.py @@ -25,9 +25,14 @@ def _write(tmp_path: Path, name: str, body: str) -> Path: - """Write a fixture inside pytest's tmp_path -- never into the repo tree.""" + """Write a fixture inside pytest's tmp_path -- never into the repo tree. + + UTF-8 is explicit because fixtures mirror real repo lines, and those carry + em dashes and box-drawing rules. Left to the platform default this raises + UnicodeEncodeError on Windows (cp1252) while passing everywhere else. + """ target = tmp_path / name - target.write_text(body) + target.write_text(body, encoding="utf-8") return target @@ -185,7 +190,9 @@ def test_live_scan_reports_well_formed_gates(self) -> None: # branch stores an absolute path instead; `REPO_ROOT / ` # yields that path unchanged, so both cases are covered. lines = ( - (check_version_gates.REPO_ROOT / gate.path).read_text(errors="replace").splitlines() + (check_version_gates.REPO_ROOT / gate.path) + .read_text(encoding="utf-8", errors="replace") + .splitlines() ) assert 1 <= gate.line <= len(lines), f"{gate.path}:{gate.line} is out of range" assert check_version_gates.VNEXT_TOKEN in lines[gate.line - 1], ( @@ -198,7 +205,7 @@ def test_prose_mentions_are_still_present_and_ignored(self) -> None: mentions = sum( line.count(check_version_gates.VNEXT_TOKEN) for path in paths - for line in path.read_text(errors="replace").splitlines() + for line in path.read_text(encoding="utf-8", errors="replace").splitlines() ) assert mentions > 0, "no vNEXT mentions at all -- the scan globs are probably broken" @@ -262,10 +269,96 @@ def test_gate_inside_a_fence_is_still_a_gate(self) -> None: with tempfile.TemporaryDirectory() as tmp: f = Path(tmp) / "commands.md" - f.write_text("```\n# component detail (since vNEXT): falls back to ...\n```\n") + f.write_text( + "```\n# component detail (since vNEXT): falls back to ...\n```\n", + encoding="utf-8", + ) found = residue([f]) assert len(found) == 1, "a fenced gate must not be exempt -- see the class docstring" def test_inline_backticks_remain_the_escape_hatch(self, tmp_path: Path) -> None: f = _write(tmp_path, "doc.md", "Resolve them: run `grep -rn '(since vNEXT)' docs/`\n") assert residue([f]) == [] + + +class TestPythonSourcesAreScanned: + """``src/**/*.py`` is in scope -- its absence shipped a placeholder in 0.90.1. + + A gate in a Python comment is agent-facing documentation exactly like a + markdown one, and until this glob existed only one hand-picked file under + ``src/`` was read: the placeholder in ``permissions.py`` went out in a + release, and a stale ``v0.26.1`` marker in ``commands/project.py`` -- a + version that never shipped -- sat undetected for three months. + """ + + def test_scanned_globs_cover_python_sources(self) -> None: + """Cheap guard: the glob cannot be dropped again without a red test.""" + assert any( + pattern.startswith("src/") and pattern.endswith("*.py") + for pattern in check_version_gates.SCANNED_GLOBS + ), f"src Python sources must stay in scope: {check_version_gates.SCANNED_GLOBS}" + + def test_the_scan_reaches_a_file_that_used_to_be_invisible(self) -> None: + """Behavioural half: a glob can be present and still resolve to nothing. + + ``permissions.py`` is the file whose placeholder shipped in 0.90.1 and + was never covered by the old hand-picked entry, so it is the honest + witness that the widened scan actually reads more than it used to. + """ + scanned = {p.name for p in check_version_gates.resolve_paths()} + assert "permissions.py" in scanned + assert "project.py" in scanned + + def test_scripts_are_not_scanned(self) -> None: + """The gate script names the placeholder it hunts -- scanning it self-flags.""" + assert not any( + pattern.startswith("scripts/") for pattern in check_version_gates.SCANNED_GLOBS + ), "scripts/ cannot be scanned: check_version_gates.py itself quotes vNEXT" + + def test_bare_placeholder_in_a_python_comment_is_a_gate(self, tmp_path: Path) -> None: + """The exact 0.90.1 shape: a registry comment tagged with the placeholder.""" + f = _write( + tmp_path, + "registry.py", + '"""Module docstring."""\n\n# Serve-only (since vNEXT): a new REST operation.\nX = 1\n', + ) + found = residue([f]) + assert len(found) == 1 + assert found[0].line == 3 + + def test_python_files_contribute_numeric_gates(self, tmp_path: Path) -> None: + """A `(since vX.Y.Z)` in a comment is audited against CHANGELOG like any other.""" + f = _write(tmp_path, "cmd.py", "# ── Project members (since v0.29.0) ──\n") + assert list(collect([f])) == ["0.29.0"] + + +class TestDoubleBacktickSpans: + """RST-style ``x`` spans count as inline code -- the docstring style under ``src/``. + + Without this the first ``(since vNEXT)`` written in a Python docstring + becomes a false positive; with it, markdown behaviour is unchanged (a + single-backtick span is still a span, and a bare placeholder is still live). + """ + + def test_double_backtick_span_is_prose(self, tmp_path: Path) -> None: + f = _write( + tmp_path, "mod.py", '"""Tag new behavior ``(since vNEXT)`` in a feature PR."""\n' + ) + assert residue([f]) == [] + + def test_single_backtick_span_is_still_prose(self, tmp_path: Path) -> None: + """Regression: widening the pattern must not break the markdown case.""" + f = _write(tmp_path, "doc.md", "the literal `vNEXT` placeholder\n") + assert residue([f]) == [] + + def test_bare_placeholder_beside_a_double_backtick_span_still_flags( + self, tmp_path: Path + ) -> None: + """One quoted mention must not launder a live gate sharing the line.""" + f = _write(tmp_path, "mod.py", '"""``vNEXT`` is the token; (since vNEXT) is live."""\n') + assert len(residue([f])) == 1 + + def test_double_backticks_do_not_hide_numeric_gates(self, tmp_path: Path) -> None: + """The GATE_RE asymmetry survives: code spans are never stripped for versions.""" + f = _write(tmp_path, "mod.py", '"""Device-enrollment primitives (``0.66.0+``)."""\n') + assert list(collect([f])) == ["0.66.0"] diff --git a/tests/test_server_permissions.py b/tests/test_server_permissions.py index c458924f..82daa626 100644 --- a/tests/test_server_permissions.py +++ b/tests/test_server_permissions.py @@ -287,6 +287,123 @@ def test_no_flags_forwards_false(self, tmp_path: Path, monkeypatch: pytest.Monke assert captured["deny_destructive"] is False +class TestServeCommandCarriesTheRootConfigDir: + """The root `kbagent --config-dir` must reach `create_app` (issue #679). + + `serve` is the only subcommand with a `--config-dir` of its own. It used to + forward ONLY that one, so `kbagent --config-dir A serve` silently served a + completely different directory -- the wrong projects, and (since the + `/auth/*` routes started enforcing it) the wrong `permissions` policy. + Precedence is now most-specific-wins, as in `kbagent repl`. + """ + + def _captured_config_dir( + self, monkeypatch: pytest.MonkeyPatch, argv: list[str] + ) -> tuple[Any, Any]: + result, captured = TestServeCommandCarriesTheFlags()._invoke_serve(monkeypatch, argv) + return result, captured.get("config_dir") + + def test_root_flag_is_used_when_serve_has_none( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + root_dir = tmp_path / "root" + result, config_dir = self._captured_config_dir( + monkeypatch, ["--config-dir", str(root_dir), "serve"] + ) + assert result.exit_code == 0, result.output + assert config_dir == str(root_dir) + + def test_serve_flag_alone_still_wins( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + serve_dir = tmp_path / "serve" + result, config_dir = self._captured_config_dir( + monkeypatch, ["serve", "--config-dir", str(serve_dir)] + ) + assert result.exit_code == 0, result.output + assert config_dir == str(serve_dir) + + def test_serve_flag_beats_the_root_flag( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + root_dir = tmp_path / "root" + serve_dir = tmp_path / "serve" + result, config_dir = self._captured_config_dir( + monkeypatch, + ["--config-dir", str(root_dir), "serve", "--config-dir", str(serve_dir)], + ) + assert result.exit_code == 0, result.output + assert config_dir == str(serve_dir) + + def test_neither_flag_leaves_resolution_to_create_app( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + result, config_dir = self._captured_config_dir(monkeypatch, ["serve"]) + assert result.exit_code == 0, result.output + assert config_dir is None + + def test_env_var_source_is_not_propagated( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # The root callback resolved this dir from KBAGENT_CONFIG_DIR, not from + # an explicit flag. `create_app` reads the same env var and lands on the + # same directory, so forwarding it would only pin a resolution made at a + # different moment. Only the `cli-flag` source travels. + monkeypatch.setenv("KBAGENT_CONFIG_DIR", str(tmp_path / "from-env")) + result, config_dir = self._captured_config_dir(monkeypatch, ["serve"]) + assert result.exit_code == 0, result.output + assert config_dir is None + + +class TestRootConfigDirPolicyReachesTheRestSurface: + """End-to-end: the security half of issue #679. + + A deny policy persisted next to the projects the caller named must be the + one the served app enforces -- previously `kbagent --config-dir A serve` + built its engine from an entirely different directory. + """ + + def _serve_app(self, monkeypatch: pytest.MonkeyPatch, argv: list[str]) -> Any: + """Run the real CLI (real `create_app`) and capture the app uvicorn got.""" + import uvicorn + from typer.testing import CliRunner + + from keboola_agent_cli.cli import app as cli_app + + monkeypatch.setenv("KBAGENT_AUTO_UPDATE", "false") + monkeypatch.setenv("KBAGENT_SERVE_TOKEN", TOKEN) + captured: dict[str, Any] = {} + + def _fake_run(app: Any, **_kwargs: Any) -> None: + captured["app"] = app + + monkeypatch.setattr(uvicorn, "run", _fake_run) + result = CliRunner().invoke(cli_app, argv) + assert result.exit_code == 0, result.output + return captured["app"] + + def test_persisted_deny_in_the_root_dir_blocks_the_route( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + served = tmp_path / "served" + _persist_policy(served, PermissionPolicy(mode="allow", deny=["auth.register-projects"])) + + app = self._serve_app(monkeypatch, ["--config-dir", str(served), "serve"]) + client = TestClient(app) + + denied = client.post("/auth/register-projects", json={"all": True}, headers=AUTH) + assert denied.status_code == 403, denied.text + assert denied.json()["error"]["code"] == "PERMISSION_DENIED" + # `/auth/status` is gated by a DIFFERENT registry key (`auth.status`), + # which the policy above does not name -- so a non-403 here proves the + # deny is narrow rather than a blanket failure of the whole surface. + # It still errors PAST the guard: with no default project and no + # `?stack=`, `AuthService.status()` -> `_resolve_stack_url()` raises + # ConfigError -> HTTP 400 CONFIG_ERROR. Hence "not denied", not 200. + allowed = client.get("/auth/status", headers=AUTH) + assert allowed.status_code != 403, allowed.text + + class TestDenyWritesBlocksTheServeCommandItself: """`kbagent --deny-writes serve` never starts the server.