[mono]ADD: System Required Reviewer #1647

AidCheng · 2025-11-18T14:10:42Z

Added system_required field for reviewer
Set system_required field false by default
Modified corresponding logic to forbid the removal of system_required reviewer
Created API endpoints reviewer/system/set to modify system_required status

Signed-off-by: AidCheng <[email protected]>

Copilot

Pull Request Overview

This PR adds a system_required field to the reviewer system, enabling reviewers to be marked as system-required and preventing their removal through normal deletion operations. The implementation includes database schema changes, storage layer methods, API endpoints, and model updates.

Key Changes:

Database migration adds system_required boolean field to mega_cl_reviewer table with a default value of false
Storage layer provides methods to set/remove system-required status and prevents deletion of system-required reviewers
New API endpoint /{link}/reviewer/system/set allows modification of system-required status

Reviewed Changes

Copilot reviewed 53 out of 53 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
mono/src/api/router/cl_router.rs	Adds new API endpoint for setting/removing system-required reviewer status and includes the field in reviewer listing responses
jupiter/src/storage/cl_reviewer_storage.rs	Implements storage methods for managing system-required status and modifies remove_reviewers to filter out system-required reviewers
jupiter/src/migration/mod.rs	Registers the new migration module for the system_required field
jupiter/src/migration/m20251117_181240_add_system_required_field_for_reviewer.rs	Creates database migration to add system_required boolean column with default false
jupiter/callisto/src/mega_cl_reviewer.rs	Updates entity model to include system_required field
ceres/src/model/change_list.rs	Adds SetSystemReviewersPayload for API requests and includes system_required in ReviewerInfo response
jupiter/callisto/src/*.rs (multiple files)	Generated entity files showing sea-orm-codegen version changes (unrelated to feature)

mono/src/api/router/cl_router.rs

jupiter/src/storage/cl_reviewer_storage.rs

Copilot

Pull Request Overview

Copilot reviewed 54 out of 54 changed files in this pull request and generated 5 comments.

Copilot · 2025-11-18T23:50:42Z

jupiter/src/storage/cl_reviewer_storage.rs

+    pub async fn update_system_required_reviewers(
+        &self,
+        cl_link: &str,
+        reviewers: &Vec<String>,


The parameter reviewers should be &[String] instead of &Vec<String>. Taking a reference to a Vec is an anti-pattern; use a slice reference instead for better API design and performance.

Copilot · 2025-11-18T23:50:42Z

mono/src/api/router/cl_router.rs

+async fn set_system_required_reviewers(
+    _user: LoginUser,
+    state: State<MonoApiServiceState>,
+    Path(link): Path<String>,
+    Json(payload): Json<SetSystemReviewersPayload>,
+) -> Result<Json<CommonResult<String>>, ApiError> {
+    // TODO: Add permission check with cedar policy


The endpoint lacks any permission/authorization check to restrict who can modify the system-required status of reviewers. While there's a TODO comment acknowledging this, this is a security concern that should be addressed before merging. Setting system-required status is a privileged operation that should only be available to administrators or users with specific permissions. Consider implementing at least basic role-based access control here, or ensure this is blocked from public access until proper Cedar policy checks are implemented.

Copilot · 2025-11-18T23:50:43Z

mono/src/api/router/cl_router.rs

+) -> Result<Json<CommonResult<String>>, ApiError> {
+    // TODO: Add permission check with cedar policy
+    state
+        .storage
+        .reviewer_storage()
+        .update_system_required_reviewers(
+            &link,
+            &payload.target_reviewer_usernames,
+            payload.is_system_required,
+        )
+        .await?;
+    Ok(Json(CommonResult::success(None)))


The API endpoint returns CommonResult<String> but passes None to the success response. The return type should be CommonResult<()> instead to accurately reflect that no data is being returned. This makes the API contract clearer.

Copilot · 2025-11-18T23:50:43Z

jupiter/callisto/src/vault.rs

@@ -1,4 +1,4 @@
-//! `SeaORM` Entity, @generated by sea-orm-codegen 1.1.17
+//! `SeaORM` Entity, @generated by sea-orm-codegen 1.1.15


All the entity files in jupiter/callisto/src/ show the version comment changed from sea-orm-codegen 1.1.17 to sea-orm-codegen 1.1.15, which is a downgrade. However, the workspace Cargo.toml specifies sea-orm = "1.1.17". This inconsistency suggests these files were regenerated with an older version of sea-orm-codegen. Please regenerate these entity files using the correct version (1.1.17) that matches the project dependencies to ensure consistency and avoid potential compatibility issues.

Copilot

Pull Request Overview

Copilot reviewed 54 out of 54 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

jupiter/src/storage/cl_reviewer_storage.rs:102

The remove_reviewers function silently skips system-required reviewers due to the .filter(mega_cl_reviewer::Column::SystemRequired.eq(false)) condition. This could confuse API consumers who attempt to remove a system-required reviewer, as they will receive a successful response even though the reviewer was not removed.

Consider either:

Checking if any of the target reviewers are system-required before deletion and returning an error with a clear message like "Cannot remove system-required reviewer: {username}"
Returning information about which reviewers were actually removed vs. skipped

This would provide better feedback to API consumers and avoid silent failures.

Copilot · 2025-11-19T01:17:00Z

jupiter/callisto/src/mega_cl_reviewer.rs

@@ -1,4 +1,4 @@
-//! `SeaORM` Entity, @generated by sea-orm-codegen 1.1.17
+//! `SeaORM` Entity, @generated by sea-orm-codegen 1.1.15


All callisto entity files show a version downgrade in the generation comment from 1.1.17 to 1.1.15. This appears to indicate the entities were regenerated with an older version of sea-orm-codegen.

This could lead to inconsistencies if different parts of the codebase were generated with different versions. Please verify:

Whether this downgrade is intentional

If not, regenerate the entities with the consistent version (preferably the latest: 1.1.17)

Ensure all team members use the same version of sea-orm-codegen for entity generation

Suggested change

//! `SeaORM` Entity, @generated by sea-orm-codegen 1.1.15

//! `SeaORM` Entity, @generated by sea-orm-codegen 1.1.17

Copilot · 2025-11-19T01:17:00Z

jupiter/src/migration/m20251117_181240_add_system_required_field_for_reviewer.rs

+#[async_trait::async_trait]
+impl MigrationTrait for Migration {
+    async fn up(&self, manager: &SchemaManager) -> Result<(), DbErr> {
+        // Replace the sample below with your own migration scripts


This boilerplate comment should be removed or replaced with a description of what this migration does. The comment "Replace the sample below with your own migration scripts" is a leftover template comment that doesn't belong in production code.

Copilot · 2025-11-19T01:17:00Z

mono/src/api/router/cl_router.rs

+async fn set_system_required_reviewers(
+    _user: LoginUser,
+    state: State<MonoApiServiceState>,
+    Path(link): Path<String>,
+    Json(payload): Json<SetSystemReviewersPayload>,
+) -> Result<Json<CommonResult<String>>, ApiError> {
+    // TODO: Add permission check with cedar policy
+    state
+        .storage
+        .reviewer_storage()
+        .update_system_required_reviewers(
+            &link,
+            &payload.target_reviewer_usernames,
+            payload.is_system_required,
+        )
+        .await?;
+    Ok(Json(CommonResult::success(None)))
+}


The set_system_required_reviewers endpoint lacks authorization checks. This is a security concern as marking reviewers as "system required" is a privileged operation that should be restricted to administrators or users with appropriate permissions.

The TODO comment acknowledges this, but the endpoint is exposed without any access control. Consider implementing the Cedar policy check before merging this PR, or at minimum, add a temporary permission check (e.g., checking if the user is an admin) to prevent unauthorized modifications.

benjamin-747 · 2025-11-19T08:50:15Z

link #1632

benjamin-747 · 2025-11-19T08:51:07Z

jupiter/callisto/src/access_token.rs

@@ -1,4 +1,4 @@
-//! `SeaORM` Entity, @generated by sea-orm-codegen 1.1.17


请升级cli 到最新的版本并提交

benjamin-747 · 2025-11-21T07:07:00Z

@AidCheng 需要同时解决合并冲突问题

Signed-off-by: AidCheng <[email protected]>

Signed-off-by: Aiden <[email protected]>

Copilot

Pull request overview

Copilot reviewed 57 out of 57 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

jupiter/src/storage/cl_reviewer_storage.rs:102

The remove_reviewers function silently skips system-required reviewers without notifying the caller. Consider returning information about which reviewers were actually removed vs. which were skipped due to being system-required, or at least logging when a removal attempt is skipped. This would provide better feedback to API consumers.

Example approach:

pub async fn remove_reviewers(
    &self,
    cl_link: &str,
    reviewers: Vec<String>,
) -> Result<Vec<String>, MegaError> {
    let mut removed = Vec::new();
    for reviewer in reviewers {
        let result = mega_cl_reviewer::Entity::delete_many()
            .filter(mega_cl_reviewer::Column::ClLink.eq(cl_link))
            .filter(mega_cl_reviewer::Column::Username.eq(reviewer.clone()))
            .filter(mega_cl_reviewer::Column::SystemRequired.eq(false))
            .exec(self.get_connection())
            .await
            .map_err(|e| {
                tracing::error!("{}", e);
                MegaError::with_message(format!("fail to remove reviewer {}", reviewer))
            })?;
        
        if result.rows_affected > 0 {
            removed.push(reviewer);
        } else {
            tracing::info!("Skipped removal of system-required reviewer: {}", reviewer);
        }
    }
    Ok(removed)
}

Copilot · 2025-11-23T16:12:30Z

jupiter/src/storage/cl_reviewer_storage.rs

+    pub async fn update_system_required_reviewers(
+        &self,
+        cl_link: &str,
+        reviewers: &Vec<String>,
+        system_required: bool,
+    ) -> Result<(), MegaError> {
+        for reviewer in reviewers {
+            let mut rev: mega_cl_reviewer::ActiveModel = mega_cl_reviewer::Entity::find()
+                .filter(mega_cl_reviewer::Column::ClLink.eq(cl_link))
+                .filter(mega_cl_reviewer::Column::Username.eq(reviewer))
+                .one(self.get_connection())
+                .await
+                .map_err(|e| {
+                    tracing::error!("{}", e);
+                    MegaError::with_message(format!("fail to find reviewer {}", reviewer))
+                })?
+                .ok_or_else(|| MegaError::with_message(format!("reviewer {} not found", reviewer)))?
+                .into_active_model();
+
+            rev.system_required = Set(system_required);
+            rev.updated_at = Set(chrono::Utc::now().naive_utc());
+            rev.update(self.get_connection()).await.map_err(|e| {
+                tracing::error!("{}", e);
+                MegaError::with_message(format!(
+                    "fail to update system required for reviewer {}",
+                    reviewer
+                ))
+            })?;
+        }
+        Ok(())
+    }


The PR description mentions creating API endpoints reviewer/system/set to modify system_required status, but this endpoint is not present in the code changes. The update_system_required_reviewers method is defined in the storage layer but there's no corresponding API endpoint to expose this functionality. Consider adding the missing endpoint or updating the PR description to reflect what was actually implemented.

Copilot · 2025-11-23T16:12:31Z

saturn/src/lib.rs


 pub enum ActionEnum {
    // ** Anyone
+    UnprotectedRequest,


The UnprotectedRequest action variant is added but never used in this codebase. It's not mapped in the CL_ROUTER_ACTIONS HashMap or the Display implementation. If this is intended for future use, consider documenting that with a comment. Otherwise, remove it to avoid dead code.

AidCheng added 2 commits November 17, 2025 20:03

[mono]FEAT: System Required Reviewer

a3c914c

[Jupiter]FIX: Set system_required default to false

f6232bc

Signed-off-by: AidCheng <[email protected]>

genedna requested a review from Copilot November 18, 2025 15:41

Merge branch 'main' into dev

0badf47

genedna requested a review from benjamin-747 November 18, 2025 15:41

Copilot started reviewing on behalf of genedna November 18, 2025 15:41 View session

Copilot finished reviewing on behalf of genedna November 18, 2025 15:45

Copilot AI reviewed Nov 18, 2025

View reviewed changes

[jupiter]UPDATE: refactor storage service into one handler

c1a2036

genedna requested a review from Copilot November 18, 2025 23:46

Copilot started reviewing on behalf of genedna November 18, 2025 23:46 View session

Copilot finished reviewing on behalf of genedna November 18, 2025 23:49

Copilot AI reviewed Nov 18, 2025

View reviewed changes

genedna requested a review from Copilot November 19, 2025 01:12

Copilot started reviewing on behalf of genedna November 19, 2025 01:13 View session

Copilot finished reviewing on behalf of genedna November 19, 2025 01:16

Copilot AI reviewed Nov 19, 2025

View reviewed changes

benjamin-747 requested changes Nov 19, 2025

View reviewed changes

Merge branch 'main' into dev

cc1f9df

genedna assigned AidCheng Nov 20, 2025

AidCheng and others added 3 commits November 23, 2025 16:00

UPDATE: sea-orm version

6bbf9ca

Signed-off-by: AidCheng <[email protected]>

[mono]REMOVE: modify system_required API

0a6168d

Signed-off-by: AidCheng <[email protected]>

Merge branch 'main' into dev

9198b36

Signed-off-by: Aiden <[email protected]>

genedna requested a review from Copilot November 23, 2025 16:09

Copilot started reviewing on behalf of genedna November 23, 2025 16:09 View session

Copilot finished reviewing on behalf of genedna November 23, 2025 16:11

Copilot AI reviewed Nov 23, 2025

View reviewed changes

		@@ -1,4 +1,4 @@
		//! `SeaORM` Entity, @generated by sea-orm-codegen 1.1.17
		//! `SeaORM` Entity, @generated by sea-orm-codegen 1.1.15

[mono]ADD: System Required Reviewer #1647

Are you sure you want to change the base?

[mono]ADD: System Required Reviewer #1647

Uh oh!

Conversation

AidCheng commented Nov 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

benjamin-747 commented Nov 19, 2025

Uh oh!

benjamin-747 Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

benjamin-747 commented Nov 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants