module 2#147
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces 'Module 2' functionality, enabling Paper-to-Context (P2C) capabilities within the application. It establishes the foundational database structures, defines the necessary API endpoints, and implements the persistence layer to manage the lifecycle of reproduction context packs. This allows for the generation, storage, retrieval, and management of these context packs, facilitating the creation of reproduction sessions from research papers. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
4978ac4
into
fix/studio-output-dir-validation-p2c-design
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'module 2' for handling 'ReproContext' packs, including API endpoints, database storage, and the corresponding data models and persistence logic, structured with a ports and adapters pattern. While the architecture is sound, the API implementation lacks critical security controls, making multiple endpoints vulnerable to Insecure Direct Object Reference (IDOR) due to insufficient user ownership verification for context packs. There is also a risk of prompt injection from unvalidated user input in LLM prompts; implementing a robust authentication and authorization layer is highly recommended. Additionally, suggestions include enhancing error handling for better debug information, utilizing dependency injection for the database store, clarifying data identifier ownership, ensuring accurate progress reporting, and improving logging for data parsing to detect potential data corruption.
| @router.post("/generate") | ||
| async def generate_context_pack(request: GenerateContextPackRequest): |
There was a problem hiding this comment.
The generate_context_pack endpoint allows a user to specify any user_id in the request body. This user_id is used directly to associate the newly created context pack with a user in the database. Without verifying that the user_id matches the authenticated user, an attacker could create context packs on behalf of other users, potentially leading to data pollution or unauthorized resource consumption.
| async def list_context_packs( | ||
| user_id: str = "default", |
There was a problem hiding this comment.
The list_context_packs endpoint accepts a user_id as a query parameter and uses it to filter context packs without any authorization check. This is a classic Insecure Direct Object Reference (IDOR) vulnerability, allowing any user to list context packs belonging to any other user by simply providing their user_id.
| @router.get("/{pack_id}") | ||
| async def get_context_pack(pack_id: str): |
There was a problem hiding this comment.
The get_context_pack endpoint retrieves a context pack by its pack_id from the URL path but fails to verify if the requesting user is the owner of that pack. This allows unauthorized users to access potentially sensitive research data and LLM-generated content in any context pack if they can obtain the pack_id.
| @router.post("/{pack_id}/session") | ||
| async def create_repro_session(pack_id: str, request: CreateSessionRequest): |
There was a problem hiding this comment.
The create_repro_session endpoint allows creating a reproduction session for any pack_id without verifying ownership. An attacker could use this to trigger session creation for other users' context packs and access the resulting roadmap and prompt data.
| @router.delete("/{pack_id}") | ||
| async def delete_context_pack(pack_id: str): |
There was a problem hiding this comment.
The delete_context_pack endpoint allows any user to soft-delete any context pack by providing its pack_id. There is no verification that the requesting user owns the pack, leading to an IDOR vulnerability that allows unauthorized data deletion.
| "runbook_id": runbook_id, | ||
| "initial_steps": initial_steps, | ||
| "initial_prompt": ( | ||
| f"Based on the reproduction context pack for paper {pack.get('paper_id', '')}, " |
There was a problem hiding this comment.
The initial_prompt is constructed by embedding the user-supplied paper_id directly into a string. Since paper_id is not validated or sanitized, this is a potential source for prompt injection. If this prompt is subsequently processed by an LLM, a malicious paper_id could manipulate the LLM's behavior.
|
|
||
| router = APIRouter() | ||
|
|
||
| _store = SqlAlchemyReproContextStore() |
There was a problem hiding this comment.
Using a global instance of SqlAlchemyReproContextStore makes testing more difficult and couples this module to a specific implementation. It's a good practice in FastAPI to use the dependency injection system. This would allow you to easily swap the store implementation for tests or other environments.
For example, you could define a dependency:
def get_store() -> SqlAlchemyReproContextStore:
return SqlAlchemyReproContextStore()And then use it in your route functions:
@router.get("/{pack_id}")
async def get_context_pack(pack_id: str, store: SqlAlchemyReproContextStore = Depends(get_store)):
pack = store.get(pack_id)
# ...|
|
||
| async def _generate_stream(request: GenerateContextPackRequest): | ||
| """SSE generator for context pack generation via Module 1 ExtractionOrchestrator.""" | ||
| pack_id = new_context_pack_id() |
There was a problem hiding this comment.
There seems to be a redundant generation of pack_id. A pack_id is generated here, but the ExtractionOrchestrator also generates its own pack_id internally. This route then overwrites the orchestrator's ID with its own on line 147. This is confusing and could lead to subtle bugs.
A cleaner approach would be to generate the ID once in this route and pass it to the orchestrator to use. This would likely require a small change to the ExtractionOrchestrator.run method to accept an optional pack_id.
| _DONE = object() | ||
| _ERROR = object() | ||
|
|
||
| total_stages = 2 if request.depth == "fast" else 6 |
There was a problem hiding this comment.
The total_stages is hardcoded here. However, the actual number of stages depends not only on the depth but also on the paper_type, which is determined inside the ExtractionOrchestrator. This means the progress calculation (stages_done / total_stages) on line 105 can be incorrect.
To fix this, the orchestrator should be the source of truth for the total number of stages. You could modify the on_stage_complete callback to receive the current stage index and the total number of stages from the orchestrator, which has access to the full stage sequence.
| try: | ||
| return json.loads(self.pack_json or "{}") | ||
| except Exception: | ||
| return {} |
There was a problem hiding this comment.
Silently catching Exception and returning an empty dictionary can hide data corruption issues. If pack_json contains invalid JSON, it would be valuable to log a warning to make this issue visible for debugging. For example: logging.warning(f"Failed to parse pack_json for pack {self.id}", exc_info=True). The same applies to get_supports on line 1234.
1 participant
add module2 ReproContext pack DB, API, port and store. Related to #139