docs: correct the deployment and configuration docs, and fix the all-in-one health check by AlexeyShalaev · Pull Request #61 · bedrock-python/mr-review

AlexeyShalaev · 2026-09-06T18:28:36Z

Writing docs/agents.md against the source turned up nine defects in the docs and the
deployment files. This fixes them. One section per finding: what was wrong, how I know,
what changed. Nothing here is breaking; the only behaviour change is the all-in-one
container's health check.

1. The AI providers page documented environment variables that do not exist

docs/features/ai-providers.md described AI_PROVIDER, AI_API_KEY, AI_MODEL and
AI_BASE_URL as dotenv configuration, and named the provider types anthropic and
openai-compatible.

None of those four names appear anywhere outside that page:

$ grep -rn 'AI_PROVIDER\|AI_API_KEY\|AI_MODEL\|AI_BASE_URL' --include='*' . | grep -v node_modules
docs/features/ai-providers.md:20:AI_PROVIDER=anthropic
… 11 hits, all in that one file

Providers are created through the settings form — POST /api/v1/ai-providers — and stored
in ai_providers.yaml. The types are claude, openai and openai_compat
(AIProviderType in core/ai_providers/entities.py; the form offers Claude / OpenAI /
OpenAI-compat).

Rewrote the page against the code: the three types and the two backends behind them, the
fields with their real defaults, how base_url is used (and ignored by claude), where
the model list comes from, and the per-dispatch temperature / reasoning_budget /
reasoning_effort. Dropped the recommended-model table — GET /api/v1/ai-providers/{id}/models
asks the endpoint itself, which cannot go stale. The quick start repeated the same two
wrong type names; corrected there too.

2. `MR_REVIEW__*` in a compose `.env` does nothing

Both deploy/*/.env.example files listed MR_REVIEW__LOGGING__LEVEL and
MR_REVIEW__LOGGING__USE_JSON, and getting-started/configuration.md documented them as
.env variables. Compose reads .env only to substitute ${...} in the compose file;
neither compose file lists those names under environment: and neither sets env_file:,
so an operator who edits them gets no change. MR_REVIEW__LOGGING__USE_JSON is
additionally hard-coded to "true" in both compose files, which would win in any case.

Fixed on the docs side rather than by adding env_file:: the compose files are correct as
they are, env_file: on a file the installation guide never tells you to create would make
docker compose up fail for anyone who followed the guide, and docs/agents.md already
states the current behaviour as rule 6. The examples now carry only the four names Compose
actually substitutes, and the configuration page splits "what .env can change" from
"application settings, which go in the environment: block".

3. The all-in-one health check probed a route that does not exist

deploy/all-in-one/Dockerfile probed http://localhost:8000/health. No router registers
that path. Because the image sets MR_REVIEW__STATIC_DIR, the SPA fallback answers any
unmatched path with index.html and a 200 — so the probe was green regardless of the state
of the application. Reproduced against the app the image runs:

/health                      -> 200  text/html; charset=utf-8  '<!doctype html>…'
/system/health/livez         -> 200  application/json          '{"status":"ok"}'
/system/health/readyz        -> 200  application/json          '{"status":"ok"}'
/totally-made-up             -> 200  text/html; charset=utf-8  '<!doctype html>…'

The probe now names /system/health/livez, which deploy/standard/docker-compose.yml
already used.

Two tests come with it. tests/unit/test_deploy_health_probes.py reads the probe URLs out
of deploy/ and asserts each one is a route the health router registers — it fails on
master and passes here:

E   AssertionError: Dockerfile probes /health, which the API does not serve
E   assert '/health' in {'/system/health/livez', '/system/health/readyz'}

tests/integration/http/test_spa_fallback.py pins the fallback behaviour that made the bad
probe silent. That path had no coverage at all before.

4. There is no SQLite database

docs/index.md, getting-started/installation.md and getting-started/configuration.md
all described SQLite storage, and so did services/mr-review/README.md ("SQLite
(SQLAlchemy)"). grep -rniE 'sqlite|sqlalchemy|alembic' over services/ returns nothing:
the three repositories are file-backed (FileHostRepository, FileAIProviderRepository,
FileReviewRepository) and write hosts.yaml, ai_providers.yaml and
reviews/<uuid>.yaml. The pages now say what is actually on disk, and that it holds tokens
and API keys in plain text.

The v0.1.0 entry in CHANGELOG.md also claims SQLite storage. I left it alone — it is a
released entry, and rewriting history felt like your call rather than mine. It is published
as the site's changelog, so you may want to decide either way.

5. Three host types were missing from the docs

docs/features/hosts.md and the configuration and quick start pages described GitLab and
GitHub only. Host.type has been gitlab | github | gitea | forgejo | bitbucket for a
while, each with its own auth, and Bitbucket is the awkward one: base_url is ignored (it
is always Bitbucket Cloud) and the token field holds username:app_password for Basic
auth, with a colon-less value sent as a bearer token.

The hosts page now covers all five, with the API base each type derives from base_url,
the token each one wants, the owner/repo rule (only GitLab takes nested groups) and what
"add repository by URL" does.

Two smaller claims on that page went with it, both checked against the code:

"After saving, mr-review immediately tests the connection. A green indicator means…" —
nothing calls GET /api/v1/hosts/{id}/test from the UI (hostApi.test has no caller),
and there is no such indicator. The page now describes the endpoint instead.
"This removes the host and all associated review history" — DeleteHostUseCase removes
the host from hosts.yaml and nothing else; reviews stay.

6. README ports did not match the compose files it hands you

The quick start said http://localhost:8000 twice and the standard section 8000/8080,
while deploy/all-in-one/docker-compose.yml maps ${PORT:-17240}:8000 and the standard
file ${API_PORT:-17241}:8000 and ${WEB_PORT:-17242}:8080.

While there: the documentation table linked /quick-start/ and /configuration/, which
live under /getting-started/, and /deployment/ and /development/, which are not pages
on the site at all. Confirmed against a local zensical build.

7. The contributing guide described a repository that is not this one

It told contributors to branch from and target main, and to run these at the root:

$ uv sync --group dev
error: Group `dev` is not defined in the project's `dependency-groups` table
$ make check
make: *** No rule to make target `check'.  Stop.
$ make test-unit
make: *** No rule to make target `test-unit'.  Stop.

The root pyproject.toml has only a docs group, the default branch is master, and the
toolchains live in the two service directories. The setup and check sections are now
per-service, plus the root targets that do exist (dev, run-services, fmt-services,
docs-serve — those I ran).

make check and make test-unit now exist in services/mr-review/Makefile. Both this
guide and services/mr-review/CONTRIBUTING.md already documented them and CI already runs
exactly those commands, so the Makefile was the side that had drifted:

check:  ##@Code Lint, format check and type check
	uv run --all-extras ruff check
	uv run --all-extras ruff format --check
	uv run --all-extras mypy mr_review

test-unit:	##@App Test application (unit tests only)
	uv run --all-extras pytest -m unit

Also in that file: the architecture section forbade SQLAlchemy in the domain and required
use cases to accept AsyncUnitOfWork — neither exists in this project — and the release
section described a manual git tag && git push origin main that release-please and
publish.yml took over.

8. Two Russian planning drafts were being published

docs/ROADMAP.md and docs/specs/inline-fix-suggestions.md were built into an
English site, absent from the nav, reachable only by guessing the URL — and since
scripts/emit_markdown.py landed, published as .md twins as well.

I moved them out of docs_dir rather than translating them: they are drafts about intent
(the roadmap still says "SQLAlchemy + SQLite, Python 3.11+"), not documentation of what is
deployed, and translating a draft would have meant guessing at your intent in two long
documents. They are now ROADMAP.md and specs/inline-fix-suggestions.md in the
repository; the agents page's documentation map says where they went. The site builds ten
pages instead of twelve, with no issues.

Say the word if you would rather have the roadmap in the nav in English and I will
translate it.

9. Documented, not changed

Both confirmed and both left as they are:

A provider's concurrency cap is fixed at first use. fence.py creates the semaphore
on the first dispatch and keeps it for the process lifetime, deliberately and per its own
docstring. Worth noting that max_concurrent has no field in the settings form at all —
it is set through the API or by editing ai_providers.yaml — so the effect is smaller
than it sounds. Both facts are now on the AI providers page.
The all-in-one image is amd64 only. publish.yml builds api and web-app for
linux/amd64,linux/arm64 and the combined image for linux/amd64, so on Apple Silicon
the recommended deployment is the emulated one. Said so in the installation guide, where
someone choosing between the two deployments will see it.

Verification

$ make check          # services/mr-review
All checks passed!
146 files already formatted
Success: no issues found in 111 source files

$ make test           # services/mr-review
181 passed, 1 warning in 6.65s

$ make test-unit      # services/mr-review
145 passed, 36 deselected, 1 warning in 5.40s

$ uv run --frozen --no-dev --group docs zensical build --clean
No issues found
$ uv run --frozen python scripts/emit_markdown.py
wrote 10 Markdown pages into site/

uv sync was run with --frozen; neither uv.lock changed. The frontend is untouched.

Noticed, not touched

make help at the repository root fails on macOS and Linux: the help targets in
Makefile and scripts/*/Makefile use the Windows echo. idiom, and the shell answers
make: echo.: No such file or directory. It is the default goal, and the README points
at it.
The root CHANGELOG.md — the one the site publishes — has a single v0.1.0 entry while
the services are at 0.2.1 and 0.2.2, whose own changelogs release-please maintains.
scripts/tools/Makefile still offers alembic-new-migration.

The image's HEALTHCHECK asked for /health, which no router registers. With the SPA mounted, the fallback answers any unmatched path with index.html and a 200, so the probe reported healthy whatever state the application was in. Point it at /system/health/livez, the endpoint the standard compose file already uses. Adds a test that reads the probe URLs out of deploy/ and asserts each one names a route the health router registers, and one that pins the SPA fallback behaviour that made the bad probe silent.

The AI providers page documented AI_PROVIDER, AI_API_KEY, AI_MODEL and AI_BASE_URL as dotenv settings. None of those names exist in the service: providers are created through the settings form (POST /api/v1/ai-providers) and stored in ai_providers.yaml. The page also named the types anthropic and openai-compatible; the real ones are claude, openai and openai_compat, and the quick start repeated the same two wrong names. Rewrites the page against the code — types, fields and their defaults, how base_url is used, where the model list comes from, and the per-dispatch options — and drops the recommended-model table in favour of the endpoint's own list. Documents that a provider's concurrency cap is fixed at first use and only changes on a restart.

Both .env.example files listed MR_REVIEW__LOGGING__LEVEL and MR_REVIEW__LOGGING__USE_JSON, and the configuration guide documented them as .env variables. Compose reads .env only to substitute ${...} in the compose file; neither compose file lists those names under environment: or sets env_file:, so setting them there does nothing. USE_JSON is additionally hard-coded to "true" in both compose files, which would win anyway. Drops the inert lines from the examples and splits the configuration page into the variables .env can change (PORT, API_PORT, WEB_PORT, DATA_DIR) and the application settings, which belong in the environment: block.

The home page, installation guide and configuration guide all described a SQLite database. The three repositories are file-backed — hosts.yaml, ai_providers.yaml and reviews/<uuid>.yaml under the data directory — and sqlite appears nowhere in the service. Also notes in the installation guide that the all-in-one image is amd64 only, which is what publish.yml builds, so on Apple Silicon the recommended deployment is the emulated one.

The hosts page described GitLab and GitHub only. Gitea, Forgejo and Bitbucket have been supported for a while, and Bitbucket is the odd one: base_url is ignored (it is always Bitbucket Cloud) and the token field takes username:app_password for Basic auth, with a colon-less value sent as a bearer token. Also corrects two claims that no longer hold: nothing tests a host connection automatically after saving — GET /api/v1/hosts/{id}/test is the check, and it is not called by the form — and deleting a host removes the host from hosts.yaml, not the reviews created against it.

The development block told contributors to run 'uv sync --group dev', 'make fmt', 'make check', 'make test-unit' and 'make test' at the repository root, and to branch from and target 'main'. The root pyproject has no dev group (only docs), the root Makefile has none of those four targets, and the default branch is master: $ uv sync --group dev error: Group `dev` is not defined in the project's `dependency-groups` table $ make check make: *** No rule to make target `check'. Stop. Per-service setup and checks instead, since that is where the toolchains live, plus the root targets that do exist (dev, run-services, fmt-services, docs-serve). 'make check' and 'make test-unit' now exist in services/mr-review — both guides already documented them, and they run exactly what CI runs. Also drops the SQLAlchemy and AsyncUnitOfWork rules from the architecture section: neither exists in this project, and the release section described a manual tag push that release-please and publish.yml took over.

docs/ROADMAP.md and docs/specs/inline-fix-suggestions.md were built into the site and, since scripts/emit_markdown.py landed, published as .md twins too — while being written in Russian, absent from the nav, and reachable only by someone who guessed the URL. Both are drafts about intent, not documentation of what is deployed, so they move to ROADMAP.md and specs/ in the repository instead of being translated. The agents page's documentation map points at where they went.

Quick start and the all-in-one section said http://localhost:8000, and the standard section 8000/8080. The shipped compose files map ${PORT:-17240}:8000, ${API_PORT:-17241}:8000 and ${WEB_PORT:-17242}:8080, which is what the docs site says. Also repoints the documentation table: /quick-start/ and /configuration/ are under /getting-started/, and /deployment/ and /development/ are not pages on the site at all.

…hosts Same two corrections one level down: the backend README claimed SQLite via SQLAlchemy and GitLab/GitHub only, and pointed at a 'make dev' target the service Makefile does not have (it is 'make run-api'; 'make dev' is the root target that starts both services). The pipeline page said comments go back to the MR 'in GitLab or GitHub'.

Follows the move: the agents page's documentation map no longer lists two pages the site does not build, and says where they went instead; the roadmap's own link to the spec drops the docs/ prefix.

The site build reported '## [Unreleased]' as an unresolved link reference — the brackets read as a Markdown reference link. The file's other headings carry no brackets either.

AlexeyShalaev added 12 commits September 6, 2026 21:15

docs: note the health check fix under [Unreleased]

3c1058b

docs: point the map and the roadmap at the moved planning drafts

8baa9cb

Follows the move: the agents page's documentation map no longer lists two pages the site does not build, and says where they went instead; the roadmap's own link to the spec drops the docs/ prefix.

docs: keep the changelog heading out of the link resolver

125692a

The site build reported '## [Unreleased]' as an unresolved link reference — the brackets read as a Markdown reference link. The file's other headings carry no brackets either.

AlexeyShalaev merged commit 68b8169 into master Sep 6, 2026
6 checks passed

AlexeyShalaev deleted the fix/agents-page-findings branch September 6, 2026 18:46

Navigation Menu

Uh oh!

docs: correct the deployment and configuration docs, and fix the all-in-one health check - #61

docs: correct the deployment and configuration docs, and fix the all-in-one health check#61
AlexeyShalaev merged 12 commits into
masterfrom
fix/agents-page-findings

AlexeyShalaev commented Sep 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

AlexeyShalaev commented Sep 6, 2026

1. The AI providers page documented environment variables that do not exist

2. MR_REVIEW__* in a compose .env does nothing

3. The all-in-one health check probed a route that does not exist

4. There is no SQLite database

5. Three host types were missing from the docs

6. README ports did not match the compose files it hands you

7. The contributing guide described a repository that is not this one

8. Two Russian planning drafts were being published

9. Documented, not changed

Verification

Noticed, not touched

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

2. `MR_REVIEW__*` in a compose `.env` does nothing