ronantakizawa/codereview-bench huggingface dataset:
Repository: aipotheosis-labs/aci
File: aipolabs/common/db/crud/projects.py (Python)

Code before:
        )
    else:
        # Increment the daily quota
        statement = (
            update(Project)
            .where(Project.id == project.id)
            .values(
                {
                    Project.daily_quota_used: project.daily_quota_used + 1,
                    Project.total_quota_used: project.total_quota_used + 1,
                }
            )
        )

    db_session.execute(statement)


def create_agent(
    db_session: Session,
    project_id: UUID,
    name: str,
    description: str,
    excluded_apps: list[UUID],
    excluded_functions: list[UUID],
    # TODO: decide if is best to set default to empty dict here
    custom_instructions: CustomInstructions | None = None,
) -> Agent:
    """
    Create a new agent under a project, and create a new API key for the agent.
    """
    # Create the agent
    agent = Agent(
        project_id=project_id,
        name=name,
        description=description,
        excluded_apps=excluded_apps,
        excluded_functions=excluded_functions,
        custom_instructions=custom_instructions if custom_instructions is not None else {},
    )
    db_session.add(agent)

    # Create the API key for the agent
    api_key = APIKey(key=secrets.token_hex(32), agent_id=agent.id, status=APIKeyStatus.ACTIVE)
    db_session.add(api_key)

    db_session.flush()
    db_session.refresh(agent)

    return agent



Review comment (bug):
not really right or wrong, but should follow the convention as in the case of excluded_apps/excluded_functions: Should not provide default value here, take and assign the value as is. Any validation or default assignment should be done prior to calling this function.
Alternatively you can pass AgentCreate as a whole instead

Code after:
        )
    else:
        # Increment the daily quota
        statement = (
            update(Project)
            .where(Project.id == project.id)
            .values(
                {
                    Project.daily_quota_used: project.daily_quota_used + 1,
                    Project.total_quota_used: project.total_quota_used + 1,
                }
            )
        )

    db_session.execute(statement)


def create_agent(
    db_session: Session,
    project_id: UUID,
    name: str,
    description: str,
    excluded_apps: list[UUID],
    excluded_functions: list[UUID],
    custom_instructions: CustomInstructions,
) -> Agent:
    """
    Create a new agent under a project, and create a new API key for the agent.
    """
    # Create the agent
    agent = Agent(
        project_id=project_id,
        name=name,
        description=description,
        excluded_apps=excluded_apps,
        excluded_functions=excluded_functions,
        custom_instructions=custom_instructions,
    )
    db_session.add(agent)

    # Create the API key for the agent
    api_key = APIKey(key=secrets.token_hex(32), agent_id=agent.id, status=APIKeyStatus.ACTIVE)
    db_session.add(api_key)

    db_session.flush()
    db_session.refresh(agent)

    return agent


def update_agent(
Repository: aipotheosis-labs/aci
File: aipolabs/common/exceptions.py (Python)

Code before:

class LinkedAccountOAuth2Error(AipolabsException):
    """
    Exception raised when an OAuth2 error occurs
    """

    def __init__(self, message: str | None = None):
        super().__init__(
            title="Linked account OAuth2 error",
            message=message,
            error_code=status.HTTP_400_BAD_REQUEST,
        )


class AgentNotFound(AipolabsException):
    """
    Exception raised when an agent is not found
    """

    def __init__(self, message: str | None = None):
        super().__init__(
            title="Agent not found", message=message, error_code=status.HTTP_404_NOT_FOUND
        )


class CustomInstructionFailed(AipolabsException):
    """
    Exception raised when a function execution is reject due to a custom instruction
    """

    def __init__(self, message: str | None = None):
        super().__init__(
            title="Custom instruction failed", message=message, error_code=status.HTTP_403_FORBIDDEN
        )


Review comment (bug):
the naming `CustomInstructionFailed`  is a bit confusing to me. It sounds more like my instruction failed (to prevent something from happening), instead of the action from user violates my instructions. 
If this error is thrown when some action violates user's instructions, the something like "CustomInstructionViolation", "CustomInstructionViolated", "ViolateCustomInstruction" etc would be better?

Code after:

class LinkedAccountOAuth2Error(AipolabsException):
    """
    Exception raised when an OAuth2 error occurs
    """

    def __init__(self, message: str | None = None):
        super().__init__(
            title="Linked account OAuth2 error",
            message=message,
            error_code=status.HTTP_400_BAD_REQUEST,
        )


class AgentNotFound(AipolabsException):
    """
    Exception raised when an agent is not found
    """

    def __init__(self, message: str | None = None):
        super().__init__(
            title="Agent not found", message=message, error_code=status.HTTP_404_NOT_FOUND
        )


class CustomInstructionViolation(AipolabsException):
    """
    Exception raised when a function execution is reject due to a custom instruction
    """

    def __init__(self, message: str | None = None):
        super().__init__(
            title="Custom instruction failed", message=message, error_code=status.HTTP_403_FORBIDDEN
        )
Repository: aipotheosis-labs/aci
File: aipolabs/common/filter.py (Python)

Code before:
from typing import Any

from aipolabs.common.db.sql_models import Function
from aipolabs.common.logging import get_logger
from aipolabs.common.openai_service import OpenAIService
from aipolabs.common.schemas.function import FilterResponse

logger = get_logger(__name__)


def filter_function_call(
    openai_service: OpenAIService,
    function: Function,
    function_input: dict,
    custom_instructions: str,
) -> Any:

    messages = [
        {
            "role": "system",
            "content": (
                "You are a helpful assistant that decides if a function call should be allowed or not based on a rule, function definition, and input."
            ),
        },
        {
            "role": "user",
            "content": (
                f"Decide if the function call should be allowed or not based on the rule, function definition, and input. "
                f"Function Definition: {function.name}, "
                f"Function Description: {function.description}, "
                f"Function Parameters: {function.parameters}, "
                f"Function Input: {function_input},"
                f"Rule: {custom_instructions}"
            ),
        },
    ]

    return openai_service.filter(messages, FilterResponse)


Review comment (refactor):
i don't think filter business logic should be coupled inside openai_service. I myself is guilty for putting "generate_fuzzy_function_call_arguments" inside for the adhoc testing purpose.
Have a think if it can be abstract out to somewhere else? ideally it should be inference provider agnostic

Code after:
    custom_instructions: str,
) -> Any:

    messages = [
        {
            "role": "system",
            "content": (
                "You are a helpful assistant that decides if a function call should be allowed or not based on a rule, function definition, and input."
            ),
        },
        {
            "role": "user",
            "content": (
                f"Decide if the function call should be allowed or not based on the rule, function definition, and input. "
                f"Function Definition: {function.name}, "
                f"Function Description: {function.description}, "
                f"Function Parameters: {function.parameters}, "
                f"Function Input: {function_input},"
                f"Rule: {custom_instructions}"
            ),
        },
    ]
    # TODO: abstract out to InferenceService
    # - make an inference layer to handle embeddings, filtering, rag etc
    # - bad to have business logic inside openai_service
    return openai_service.filter(messages, FilterResponse)
Repository: aipotheosis-labs/aci
File: aipolabs/cli/commands/create_functions.py (Python)

Code before:
    with utils.create_db_session(config.DB_FULL_URL) as db_session:
        with open(functions_file, "r") as f:
            functions_create: list[FunctionCreate] = [
                FunctionCreate.model_validate(function) for function in json.load(f)
            ]

        function_embeddings = embeddings.generate_function_embeddings(
            functions_create,
            openai_service,
            embedding_model=config.OPENAI_EMBEDDING_MODEL,
            embedding_dimension=config.OPENAI_EMBEDDING_DIMENSION,
        )

        functions = crud.functions.create_functions(
            db_session, functions_create, function_embeddings
        )
        if not skip_dry_run:
            click.echo(create_headline(f"will create {len(functions)} functions"))
            for function in functions:
                click.echo(function.name)
            click.echo(create_headline("provide --skip-dry-run to commit changes"))
            db_session.rollback()
        else:
            click.echo(create_headline(f"committing creation of {len(functions)} functions"))
            db_session.commit()

        return [function.id for function in functions]


Review comment (suggestion):
Should line 55 change as well? 
`click.echo(function.name)` -> `click.echo(f"function id={function.id}, name={function.name}")`

Code after:
    with utils.create_db_session(config.DB_FULL_URL) as db_session:
        with open(functions_file, "r") as f:
            functions_create: list[FunctionCreate] = [
                FunctionCreate.model_validate(function) for function in json.load(f)
            ]

        function_embeddings = embeddings.generate_function_embeddings(
            functions_create,
            openai_service,
            embedding_model=config.OPENAI_EMBEDDING_MODEL,
            embedding_dimension=config.OPENAI_EMBEDDING_DIMENSION,
        )

        functions = crud.functions.create_functions(
            db_session, functions_create, function_embeddings
        )
        if not skip_dry_run:
            click.echo(create_headline(f"will create {len(functions)} functions"))
            for function in functions:
                click.echo(function.name)
            click.echo(create_headline("provide --skip-dry-run to commit changes"))
            db_session.rollback()
        else:
            click.echo(create_headline(f"committing creation of {len(functions)} functions"))
            for function in functions:
                click.echo(f"function id={function.id}, name={function.name}")
            db_session.commit()

        return [function.id for function in functions]
Repository: aipotheosis-labs/aci
File: aipolabs/server/sentry.py (Python)

Code before:
import sentry_sdk
from aipolabs.server import config


def setup_sentry():
    if config.ENVIRONMENT != "local":
        sentry_sdk.init(
            dsn="https://9cebb66f55b0782e37370b08be11f1f5@o4508859021459456.ingest.us.sentry.io/4508915251871744",
            # Add data like request headers and IP for users,
            # see https://docs.sentry.io/platforms/python/data-management/data-collected/ for more info
            send_default_pii=True,
            traces_sample_rate=0.1 if config.ENVIRONMENT == "production" else 1.0,
            _experiments={
                "continuous_profiling_auto_start": True,
            },
        )


Review comment (suggestion):
is dsn gonna be env specific, like in staging we'd have a different value?
if so we might want to make it env var now like the name `SENTRY_DSN` in the comment 

Code after:
import sentry_sdk
from aipolabs.server import config


def setup_sentry():
    if config.ENVIRONMENT != "local":
        sentry_sdk.init(
            environment=config.ENVIRONMENT,
            dsn="https://9cebb66f55b0782e37370b08be11f1f5@o4508859021459456.ingest.us.sentry.io/4508915251871744",
            # Add data like request headers and IP for users,
            # see https://docs.sentry.io/platforms/python/data-management/data-collected/ for more info
            send_default_pii=True,
            traces_sample_rate=0.1 if config.ENVIRONMENT == "production" else 1.0,
            _experiments={
                "continuous_profiling_auto_start": True,
            },
        )
Repository: aipotheosis-labs/aci
File: backend/aci/common/db/crud/linked_accounts.py (Python)

Code before:
        linked_account.security_scheme, security_credentials
    )

    linked_account.security_credentials = security_credentials.model_dump(mode="json")
    db_session.flush()
    db_session.refresh(linked_account)
    return linked_account


def update_linked_account(
    db_session: Session,
    linked_account: LinkedAccount,
    linked_account_update: LinkedAccountUpdate,
) -> LinkedAccount:
    if linked_account_update.enabled is not None:
        linked_account.enabled = linked_account_update.enabled
    db_session.flush()
    db_session.refresh(linked_account)
    return linked_account


def update_linked_account_last_used_at(
    db_session: Session,
    linked_account: LinkedAccount,
) -> LinkedAccount:
    linked_account.last_used_at = func.now()
    db_session.flush()
    db_session.refresh(linked_account)
    return linked_account


def delete_linked_accounts(db_session: Session, project_id: UUID, app_name: str) -> int:
    statement = (
        select(LinkedAccount)
        .join(App, LinkedAccount.app_id == App.id)
        .filter(LinkedAccount.project_id == project_id, App.name == app_name)
    )
    linked_accounts_to_delete = db_session.execute(statement).scalars().all()
    for linked_account in linked_accounts_to_delete:
        db_session.delete(linked_account)
    db_session.flush()
    return len(linked_accounts_to_delete)


Review comment (suggestion):
don't think it's suitable to use func.now here (which is used more in raw sql context, like server default, or onupdate triggers). The codebase use "now: datetime = datetime.now(UTC)" on application level.


Code after:
    validators.security_scheme.validate_scheme_and_credentials_type_match(
        linked_account.security_scheme, security_credentials
    )

    linked_account.security_credentials = security_credentials.model_dump(mode="json")
    db_session.flush()
    db_session.refresh(linked_account)
    return linked_account


def update_linked_account(
    db_session: Session,
    linked_account: LinkedAccount,
    linked_account_update: LinkedAccountUpdate,
) -> LinkedAccount:
    if linked_account_update.enabled is not None:
        linked_account.enabled = linked_account_update.enabled
    db_session.flush()
    db_session.refresh(linked_account)
    return linked_account


def update_linked_account_last_used_at(
    db_session: Session,
    last_used_at: datetime,
    linked_account: LinkedAccount,
) -> LinkedAccount:
    linked_account.last_used_at = last_used_at
    db_session.flush()
    db_session.refresh(linked_account)
    return linked_account


def delete_linked_accounts(db_session: Session, project_id: UUID, app_name: str) -> int:
    statement = (
        select(LinkedAccount)
        .join(App, LinkedAccount.app_id == App.id)
        .filter(LinkedAccount.project_id == project_id, App.name == app_name)
    )
    linked_accounts_to_delete = db_session.execute(statement).scalars().all()
    for linked_account in linked_accounts_to_delete:
        db_session.delete(linked_account)
    db_session.flush()
    return len(linked_accounts_to_delete)
Repository: aipotheosis-labs/aci
File: frontend/src/lib/types/app.ts (TypeScript)

Code before:
import { AppFunction } from "./appfunction";

export interface App {
  id: string;
  name: string;
  display_name: string;
  provider: string;
  version: string;
  description: string;
  logo: string;
  categories: string[];
  visibility: string;
  active: boolean;
  security_schemes: string[];
  functions: AppFunction[];
  supported_security_schemes: Record<string, { scope?: string }>;
  created_at: string;
  updated_at: string;
}


Review comment (security):
`security_schemes` should be just removed and use `supported_security_schemes` instead of any data you need.

Code after:
import { AppFunction } from "./appfunction";

export interface App {
  id: string;
  name: string;
  display_name: string;
  provider: string;
  version: string;
  description: string;
  logo: string;
  categories: string[];
  visibility: string;
  active: boolean;
  functions: AppFunction[];
  supported_security_schemes: Record<string, { scope?: string }>;
  created_at: string;
  updated_at: string;
}
Repository: aipotheosis-labs/aci
File: backend/aci/common/exceptions.py (Python)

Code before:
        )


class ProjectIsLastInOrgError(ACIException):
    """
    Exception raised when a project cannot be deleted
    """

    def __init__(self, message: str | None = None):
        super().__init__(
            title="Project is the last in the organization",
            message=message,
            error_code=status.HTTP_409_CONFLICT,
        )


class MaxAgentSecretsReached(ACIException):
    """
    Exception raised when a project has reached the maximum number of agent secrets allowed by their subscription plan
    """

    def __init__(self, message: str | None = None):
        super().__init__(
            title="Max agent secrets reached",
            message=message
            or "You have reached the maximum number of agent secrets allowed for your subscription plan",
            error_code=status.HTTP_403_FORBIDDEN,
        )


Review comment (security):
if you insist on a default message, at least make it a default on input parameter level so it's not hidden from callers.
`message: str | None = "You have reached the maximum number of agent secrets allowed for your subscription plan"`

Code after:
        )


class ProjectIsLastInOrgError(ACIException):
    """
    Exception raised when a project cannot be deleted
    """

    def __init__(self, message: str | None = None):
        super().__init__(
            title="Project is the last in the organization",
            message=message,
            error_code=status.HTTP_409_CONFLICT,
        )


class MaxAgentSecretsReached(ACIException):
    """
    Exception raised when a project has reached the maximum number of agent secrets allowed by their subscription plan
    """

    def __init__(self, message: str | None = None):
        super().__init__(
            title="Max agent secrets reached",
            message=message,
            error_code=status.HTTP_403_FORBIDDEN,
        )
Repository: aipotheosis-labs/aci
File: backend/aci/server/routes/organizations.py (Python)

Code before:
from typing import Annotated
from uuid import UUID

from fastapi import APIRouter, Depends, Header, status
from propelauth_fastapi import User

from aci.common.enums import OrganizationRole
from aci.common.logging_setup import get_logger
from aci.common.schemas.organizations import InviteMemberRequest
from aci.server import acl

router = APIRouter()
logger = get_logger(__name__)

auth = acl.get_propelauth()


@router.post("/invite", status_code=status.HTTP_204_NO_CONTENT)
async def invite_member(
    user: Annotated[User, Depends(auth.require_user)],
    org_id: Annotated[UUID, Header(alias="X-ACI-ORG-ID")],
    body: InviteMemberRequest,
) -> None:
    """
    Invite a member to the organization.
    Only organization owners and admins can invite new members.
    """
    logger.info(
        "inviting member to organization",
        extra={
            "user_id": user.user_id,
            "org_id": org_id,
            "invitee_email": body.email,
            "role": body.role,
        },
    )

    # Only owners and admins can invite members
    acl.require_org_member_with_minimum_role(user, org_id, OrganizationRole.ADMIN)

    # Create the invitation
    auth.invite_user_to_org(
        org_id=str(org_id),
        email=body.email,
        role=body.role,
    )

Review comment (suggestion):
for header key that is used across multiple places, maybe put it in the config file similiar to `AIPOLABS_API_KEY_NAME = "X-API-KEY"` in the config.py? (maybe here we should rename to `ACI_HEADER_API_KEY`)
e.g., `ACI_HEADER_ORG_ID = "X-ACI-ORG-ID"`

Code after:
from typing import Annotated
from uuid import UUID

from fastapi import APIRouter, Depends, Header, status
from propelauth_fastapi import User

from aci.common.enums import OrganizationRole
from aci.common.exceptions import OrgAccessDenied
from aci.common.logging_setup import get_logger
from aci.common.schemas.organizations import InviteMemberRequest
from aci.server import acl, config

router = APIRouter()
logger = get_logger(__name__)

auth = acl.get_propelauth()


@router.post("/invite_user", status_code=status.HTTP_204_NO_CONTENT)
async def invite_user(
    user: Annotated[User, Depends(auth.require_user)],
    org_id: Annotated[UUID, Header(alias=config.ACI_ORG_ID_HEADER)],
    body: InviteMemberRequest,
) -> None:
    """
    Invite a user to the organization.
    Only organization owners and admins can invite new users.
    Cannot invite users with the OWNER role.
    """
    logger.info(
        "inviting user to organization",
        extra={
            "user_id": user.user_id,
            "org_id": org_id,
            "invitee_email": body.email,
            "role": body.role,
        },
    )

    # Only owners and admins can invite users
    acl.require_org_member_with_minimum_role(user, org_id, OrganizationRole.ADMIN)

    # Prevent inviting users as owners
    if body.role == OrganizationRole.OWNER:
        raise OrgAccessDenied("Cannot invite users with the OWNER role")
Repository: airbnb/javascript
File: packages/eslint-config-airbnb/test/test-base.js (JavaScript)

Code before:
import fs from 'fs';
import path from 'path';
import test from 'tape';

const base = require('../base');

const files = { base };

const rulesDir = path.join(__dirname, '../rules');
fs.readdirSync(rulesDir).forEach((name) => {
  if (name === 'react.js' || name === 'react-a11y.js') {
    return;
  }

  // eslint-disable-next-line import/no-dynamic-require
  files[name] = require(path.join(rulesDir, name));
});

const checkRules = (name, config) => {
  test(`${name}: does not reference react`, (t) => {
    t.plan(2);

    // scan plugins for react and fail if it is found
    const hasReactPlugin = Object.prototype.hasOwnProperty.call(config, 'plugins')
      && config.plugins.indexOf('react') !== -1;
    t.notOk(hasReactPlugin, 'there is no react plugin');

    // scan rules for react/ and fail if any exist
    const reactRuleIds = Object.keys(config.rules)
      .filter((ruleId) => ruleId.indexOf('react/') === 0);
    t.deepEquals(reactRuleIds, [], 'there are no react/ rules');
  });
};

Object.keys(files).forEach((name) => {
  const config = files[name];
  checkRules(name, config);
});

Review comment (suggestion):
```suggestion
  files[name] = require(path.join(rulesDir, name)); // eslint-disable-line global-require
```

Code after:
import fs from 'fs';
import path from 'path';
import test from 'tape';

const base = require('../base');

const files = { base };

const rulesDir = path.join(__dirname, '../rules');
fs.readdirSync(rulesDir).forEach((name) => {
  if (name === 'react.js' || name === 'react-a11y.js') {
    return;
  }

  // eslint-disable-next-line import/no-dynamic-require
  files[name] = require(path.join(rulesDir, name)); // eslint-disable-line global-require
});

Object.keys(files).forEach((name) => {
  const config = files[name];

  test(`${name}: does not reference react`, (t) => {
    t.plan(2);

    // scan plugins for react and fail if it is found
    const hasReactPlugin = Object.prototype.hasOwnProperty.call(config, 'plugins')
      && config.plugins.indexOf('react') !== -1;
    t.notOk(hasReactPlugin, 'there is no react plugin');

    // scan rules for react/ and fail if any exist
    const reactRuleIds = Object.keys(config.rules)
      .filter((ruleId) => ruleId.indexOf('react/') === 0);
    t.deepEquals(reactRuleIds, [], 'there are no react/ rules');
  });
});