Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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.githubusercontent.com/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
There was a problem hiding this comment.
Confirmed and it now runs on all templates a6d5549
| 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
…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
Backport the six OpenAPI lint rules developed in sensor-monitor so every
generated project enforces them:
- path-no-generic-id: path id params must be explicit, not a bare {id}
- path-param-snake-case / query-param-snake-case
- response-media-type-json / request-media-type-json
- create-body-no-required-id: POST bodies must not require id
- no-action-verbs-in-path
- location-header-on-201
Ruleset-only change. The template backend skeleton (healthcheck + shutdown)
does not exercise any of them, so the generated project stays green. Rule
blocks are byte-identical to the sensor-monitor ruleset (which is this file
plus the copier-managed warning header).
Cant use lirbaries so add simplistic plural check with some allow list words that are likely to be used.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
template/.config/vacuum-ruleset.yaml (1)
61-63: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winEnable the OWASP ruleset for these entries
owasp-define-error-validation,owasp-integer-format, andowasp-integer-limitaren’t covered byvacuum:oas; with only that set extended, thesetrueentries won’t run. Add[vacuum:owasp, all]toextendsor replace them with full rule definitions.🤖 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/.config/vacuum-ruleset.yaml` around lines 61 - 63, Update the ruleset’s extends configuration to include the vacuum:owasp rules alongside the existing sets, ensuring owasp-define-error-validation, owasp-integer-format, and owasp-integer-limit entries are activated. Preserve the current rule settings and avoid replacing them with full definitions.
🤖 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.
Outside diff comments:
In `@template/.config/vacuum-ruleset.yaml`:
- Around line 61-63: Update the ruleset’s extends configuration to include the
vacuum:owasp rules alongside the existing sets, ensuring
owasp-define-error-validation, owasp-integer-format, and owasp-integer-limit
entries are activated. Preserve the current rule settings and avoid replacing
them with full definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 556f685f-1a86-4d2a-b44d-62191dee513e
📒 Files selected for processing (4)
template/.config/vacuum-functions/resourcesPlural.jstemplate/.config/vacuum-ruleset.yamltemplate/.pre-commit-config.yaml.jinjatemplate/{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinja
graphql is a singleton endpoint, not a REST collection, so it should never be flagged by resources-plural. Backported from lab-sync/sensor-monitor.
Bare summary table hides which path/rule failed. Add -d --no-clip so failures print file:line, message, and rule name per violation. Backported from lab-sync/sensor-monitor.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
New Features
systemquery that returns the application version.Quality Improvements