Skip to content

feat: add Action.RESPONSES for /responses endpoint authorization#1600

Merged
tisnik merged 2 commits into
lightspeed-core:mainfrom
Lifto:feat/action-responses
Apr 27, 2026
Merged

feat: add Action.RESPONSES for /responses endpoint authorization#1600
tisnik merged 2 commits into
lightspeed-core:mainfrom
Lifto:feat/action-responses

Conversation

@Lifto
Copy link
Copy Markdown
Contributor

@Lifto Lifto commented Apr 27, 2026

Summary

  • Add Action.RESPONSES = "responses" to the Action enum in src/models/config.py, immediately after Action.QUERY = "query", following the existing comment-per-constant style.
  • Change @authorize(Action.QUERY)@authorize(Action.RESPONSES) on the /responses endpoint handler in src/app/endpoints/responses.py.
  • Update dummy_request_fixture in tests/unit/app/endpoints/test_responses.py to use Action.RESPONSES instead of Action.QUERY.

Why

Previously both /query and /responses were guarded by Action.QUERY. This meant:

  • Granting query permission gave access to both endpoints.
  • The two endpoints could not be authorized independently.

With this change:

  • query → controls /query endpoint only.
  • responses → controls /responses endpoint only.

⚠️ Deployment Sequencing (IMPORTANT)

The lscore-deploy MR that adds responses to the allowed actions list MUST be deployed BEFORE this PR is deployed.

If lightspeed-stack is deployed first, the /responses endpoint will return 403 Forbidden for all users because the deployment config will not yet include the new responses action. The lscore-deploy MR for this is being opened simultaneously.

Changes

File Change
src/models/config.py Add RESPONSES = "responses" to Action enum after QUERY
src/app/endpoints/responses.py @authorize(Action.QUERY)@authorize(Action.RESPONSES)
tests/unit/app/endpoints/test_responses.py Action.QUERYAction.RESPONSES in dummy_request_fixture

Verification

  • uv run make format
  • uv run make verify ✅ (pylint 10.00/10, pyright 0 errors, ruff clean, black clean, pydocstyle clean; pre-existing check-types failure in dev-tools/ unrelated to this change)
  • uv run make test-unit ✅ (2067 passed; 1 pre-existing failure in test_user_data_collection_wrong_directory_path unrelated to this change)

Ultraworked with Sisyphus

Summary by CodeRabbit

  • Bug Fixes

    • Corrected the authorization permission scope for the responses endpoint to enforce more specific and appropriate access control requirements. This ensures that user permissions are properly validated when processing response-related requests and operations across the application.
  • Tests

    • Updated test fixtures to verify the corrected authorization requirements for the responses endpoint.

Add Action.RESPONSES = "responses" to the Action enum immediately after
Action.QUERY. Update the /responses endpoint decorator from
@authorize(Action.QUERY) to @authorize(Action.RESPONSES), and update
the corresponding unit test fixture to use Action.RESPONSES.

This allows /query and /responses to be authorized independently.
Previously both endpoints shared Action.QUERY, making it impossible
to grant access to one without the other.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Signed-off-by: Ellis Low <elow@redhat.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 27, 2026

Warning

Rate limit exceeded

@Lifto has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 39 minutes and 6 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fe4a788a-74d8-4e69-bf5e-4e6031cf3e6e

📥 Commits

Reviewing files that changed from the base of the PR and between 2579184 and ab30ffb.

📒 Files selected for processing (1)
  • tests/unit/app/endpoints/test_responses.py

Walkthrough

The PR updates the /responses endpoint's authorization requirement from Action.QUERY to a new Action.RESPONSES permission action. This involves adding the new enum value to the action configuration, updating the endpoint's authorization decorator, and adjusting the corresponding test fixture.

Changes

Cohort / File(s) Summary
Authorization configuration
src/models/config.py
Added new Action.RESPONSES = "responses" enum value to extend available authorization actions.
Endpoint authorization
src/app/endpoints/responses.py
Updated /responses POST handler decorator from @authorize(Action.QUERY) to @authorize(Action.RESPONSES).
Test fixture
tests/unit/app/endpoints/test_responses.py
Updated test request state to include Action.RESPONSES instead of Action.QUERY in authorized actions set.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and directly describes the main change: introducing a new Action.RESPONSES authorization level for the /responses endpoint.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/unit/app/endpoints/test_responses.py`:
- Line 166: Add a negative test in tests/unit/app/endpoints/test_responses.py
that constructs a Request whose req.state.authorized_actions includes all
existing permissions except Action.RESPONSES, then calls the endpoint (the
handler protected by `@authorize`(Action.RESPONSES)) or resolve_response_context
path exercised in this file and asserts it raises HTTPException with status 403;
ensure you reference the same Request shape used in other tests and import
HTTPException and Action to make the assertion explicit so the new test locks in
the permission boundary for authorize(Action.RESPONSES).
🪄 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: 1de38d15-1bab-4620-a6ec-b60227da51ad

📥 Commits

Reviewing files that changed from the base of the PR and between 2d0c79a and 2579184.

📒 Files selected for processing (3)
  • src/app/endpoints/responses.py
  • src/models/config.py
  • tests/unit/app/endpoints/test_responses.py
📜 Review details
⏰ 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). (13)
  • GitHub Check: E2E: server mode / ci / group 1
  • GitHub Check: E2E: library mode / ci / group 2
  • GitHub Check: E2E: server mode / ci / group 3
  • GitHub Check: E2E: server mode / ci / group 2
  • GitHub Check: E2E: library mode / ci / group 1
  • GitHub Check: E2E: library mode / ci / group 3
  • GitHub Check: E2E Tests for Lightspeed Evaluation job
  • GitHub Check: Pylinter
  • GitHub Check: radon
  • GitHub Check: unit_tests (3.13)
  • GitHub Check: build-pr
  • GitHub Check: unit_tests (3.12)
  • GitHub Check: integration_tests (3.13)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Use absolute imports for internal modules: from authentication import get_auth_dependency
Import FastAPI dependencies with: from fastapi import APIRouter, HTTPException, Request, status, Depends
Import Llama Stack client with: from llama_stack_client import AsyncLlamaStackClient
Check constants.py for shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Use logger = get_logger(__name__) from log.py for module logging
Type aliases defined at module level for clarity
Use Final[type] as type hint for all constants
All functions require docstrings with brief descriptions
Complete type annotations for parameters and return types in functions
Use typing_extensions.Self for model validators in Pydantic models
Use modern union type syntax str | int instead of Union[str, int]
Use Optional[Type] for optional type hints
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Use async def for I/O operations and external API calls
Handle APIConnectionError from Llama Stack in error handling
Use standard log levels with clear purposes: debug, info, warning, error
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes: Configuration, Error/Exception, Resolver, Interface
Use ABC for abstract base classes with @abstractmethod decorators
Use @model_validator and @field_validator for Pydantic model validation
Complete type annotations for all class attributes; use specific types, not Any
Follow Google Python docstring conventions with Parameters, Returns, Raises, and Attributes sections

Files:

  • tests/unit/app/endpoints/test_responses.py
  • src/app/endpoints/responses.py
  • src/models/config.py
tests/{unit,integration}/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest; pytest is the standard for this project
Use pytest-mock for AsyncMock objects in tests
Use marker pytest.mark.asyncio for async tests
Unit tests require 60% coverage, integration tests 10%

Files:

  • tests/unit/app/endpoints/test_responses.py
src/app/endpoints/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Use FastAPI HTTPException with appropriate status codes for API endpoints

Files:

  • src/app/endpoints/responses.py
src/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Pydantic models extend ConfigurationBase for config, BaseModel for data models

Files:

  • src/app/endpoints/responses.py
  • src/models/config.py
src/**/config*.py

📄 CodeRabbit inference engine (AGENTS.md)

src/**/config*.py: All config uses Pydantic models extending ConfigurationBase
Base class sets extra="forbid" to reject unknown fields in Pydantic models
Use @field_validator and @model_validator for custom validation in Pydantic models
Use type hints like Optional[FilePath], PositiveInt, SecretStr in Pydantic models

Files:

  • src/models/config.py
🧠 Learnings (3)
📚 Learning: 2026-04-06T20:18:07.852Z
Learnt from: major
Repo: lightspeed-core/lightspeed-stack PR: 1463
File: src/app/endpoints/rlsapi_v1.py:266-271
Timestamp: 2026-04-06T20:18:07.852Z
Learning: In the lightspeed-stack codebase, within `src/app/endpoints/` inference/MCP endpoints, treat `tools: Optional[list[Any]]` in MCP tool definitions as an intentional, consistent typing pattern (used across `query`, `responses`, `streaming_query`, `rlsapi_v1`). Do not raise or suggest this as a typing issue during code review; changing it in isolation could break endpoint typing consistency across the codebase.

Applied to files:

  • src/app/endpoints/responses.py
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.

Applied to files:

  • src/models/config.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.

Applied to files:

  • src/models/config.py
🔇 Additional comments (2)
src/app/endpoints/responses.py (1)

207-207: LGTM — decorator now correctly gates /responses independently of /query.

Action.RESPONSES is properly imported via from models.config import Action (line 42), and the downstream request.state.authorized_actions lookup for READ_OTHERS_CONVERSATIONS (line 275) is independent of this change, so conversation read-through behavior is preserved. The split between Action.QUERY (for /query) and Action.RESPONSES (for /responses) is exactly the separation called out in the PR.

src/models/config.py (1)

994-996: Confirm deployment ordering with lscore-deploy before merging.

The enum addition is clean and correctly implements the authorization change. However, switching /responses from Action.QUERY to Action.RESPONSES creates a breaking change for access control: any role authorized via query permission will now receive 403 on /responses until the corresponding lscore-deploy MR updates the access_rules for those roles to include the new "responses" action.

To avoid cascading 403 errors across environments after this PR is deployed:

  • Verify the lscore-deploy MR adding "responses" to the relevant role(s) is merged and deployed to dev, stage, and prod before or immediately concurrent with this PR's promotion.
  • Confirm whether any external role/permission documentation in this repository also needs updates to reflect the new action requirement.

Comment thread tests/unit/app/endpoints/test_responses.py
@tisnik tisnik requested a review from asimurka April 27, 2026 17:19
Copy link
Copy Markdown
Contributor

@asimurka asimurka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Copy link
Copy Markdown
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tisnik tisnik merged commit 2ee5f84 into lightspeed-core:main Apr 27, 2026
30 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants