-
Notifications
You must be signed in to change notification settings - Fork 55
LCORE-287: fixed field descriptions #535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LCORE-287: fixed field descriptions #535
Conversation
WalkthroughOpenAPI specification and documentation updated: schema defaults replaced with $ref references, sensitive fields marked as password/writeOnly, examples and titles adjusted, and version example bumped. Endpoint code updated to correct concatenated response description strings. No runtime logic changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/app/endpoints/authorized.py (2)
17-36: Double-check 400 vs 401 semantics across modules.Using 400 (noop/noop-with-token) and 401 (k8s) is unusual. If “missing/invalid credentials” is the condition in both, consider standardizing on 401 for consistency across endpoints, or add a brief rationale in docs.
49-56: Docstring out of sync with response shape.Return type now includes
skip_userid_check. Update the docstring to match.- Returns: - AuthorizedResponse: Contains the user ID and username of the authenticated user. + Returns: + AuthorizedResponse: Contains the user ID, username, and skip_userid_check flag.docs/openapi.md (1)
513-521: Added skip_userid_check field — ensure “required” status matches runtime.OpenAPI JSON marks it required; this markdown table doesn’t show requiredness. Make sure the implementation always sets it.
docs/openapi.json (3)
269-272: Config sample now masks llama_stack.api_key — good step.Consider consistent masking length/pattern across docs to avoid inference of original length.
936-961: AuthorizedResponse: added boolean skip_userid_check + examples/description — OK.Minor nit: title uses “Userid” while elsewhere “User ID” is used. Prefer one style repo-wide.
- "title": "Skip Userid Check", + "title": "Skip User ID Check",
1650-1656: InfoResponse inline example updated to 0.2.19 — OK.Optional: align other example blocks to the same versions for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/openapi.json(10 hunks)docs/openapi.md(3 hunks)docs/output.md(3 hunks)src/app/endpoints/authorized.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/endpoints/authorized.py (1)
src/models/responses.py (1)
UnauthorizedResponse(343-357)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (12)
docs/output.md (3)
400-401: Auth error descriptions clarified (noop/noop-with-token vs k8s) — looks good.Strings now read correctly without accidental concatenation artifacts and match the code. Please ensure the same phrasing is used across all docs (openapi.md/json).
887-887: Version example bumped to 0.2.19 — OK.
513-521: skip_userid_check present and required in model, endpoints, and OpenAPI.src/models/responses.py defines skip_userid_check: bool = Field(...); src/app/endpoints/authorized.py returns it; tests reference it; docs/openapi.json lists it as required and type "boolean".
src/app/endpoints/authorized.py (1)
23-26: Fixed response description concatenation — OK.Output strings render as intended for 400/401. Confirm that downstream doc generation picks these up verbatim.
Also applies to: 28-31
docs/openapi.md (2)
400-401: Auth error descriptions clarified (noop/noop-with-token vs k8s) — looks good.
897-898: InfoResponse example updated to 0.2.19 — OK.docs/openapi.json (6)
692-710: Authorized endpoint 400/401 descriptions corrected — OK.Messages clearly denote module-specific handling.
1018-1056: Schema refactor: $ref consolidation for Configuration/auth/database/inference — good.Reduces duplication and improves maintainability. Verify client codegen still resolves defaults as expected.
1678-1680: JwkConfiguration.jwt_configuration now a pure $ref — cleaner.
1784-1795: LlamaStackConfiguration.api_key marked password/writeOnly — good security hygiene.
1909-1914: PostgreSQL password marked password/writeOnly — good.Consider similarly annotating any other secrets if present in future.
2296-2301: ServiceConfiguration.tls_config/cors now reference dedicated schemas — nice normalization.
Description
LCORE-287: fixed field descriptions
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Documentation
Bug Fixes