Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions packages/modal-infra/src/images/primo_overlay.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
localized to the smallest possible hook in base.py.
"""

import modal

AWS_CLI_VERSION = "2.34.50"
AWS_CLI_SHA256 = "0e6f3d4330a0655e2d08f3791a2ee9503bb55accbac5633b839b8e0b66c0e5b5"

Expand Down Expand Up @@ -47,6 +49,21 @@ def primo_sandbox_create_kwargs(repo_owner: str | None, repo_name: str | None) -
}


async def create_primo_sandbox(repo_owner: str | None, repo_name: str | None, **create_kwargs):
"""Create a sandbox the Primo way: our entrypoint wrapper plus per-repo runtime.

Every `modal.Sandbox.create` call in upstream's manager routes through here,
so the fork's patch at each call site is a single line and never reaches
into upstream's `create_kwargs` dict — the spot upstream keeps adding
fields to, and therefore the spot that keeps conflicting.

Caller-supplied kwargs win, so explicit `cpu`/`memory` from session settings
still override the Core defaults.
"""
kwargs = {**primo_sandbox_create_kwargs(repo_owner, repo_name), **create_kwargs}
return await modal.Sandbox.create.aio(*PRIMO_SANDBOX_COMMAND, **kwargs)


def apply_primo_postgres_runtime(image):
if not hasattr(image, "apt_install"):
return image
Expand Down
18 changes: 6 additions & 12 deletions packages/modal-infra/src/sandbox/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,7 @@

from ..app import app, llm_secrets
from ..images.base import base_image
from ..images.primo_overlay import (
PRIMO_SANDBOX_COMMAND,
apply_primo_postgres_runtime,
primo_sandbox_create_kwargs,
)
from ..images.primo_overlay import apply_primo_postgres_runtime, create_primo_sandbox
from .vcs_env import inject_vcs_env_vars

log = get_logger("manager")
Expand Down Expand Up @@ -428,13 +424,12 @@ async def create_sandbox(
"timeout": config.timeout_seconds,
"workdir": "/workspace",
"env": env_vars,
**primo_sandbox_create_kwargs(config.repo_owner, config.repo_name),
**_resource_kwargs(config.settings),
}
if exposed_ports:
create_kwargs["encrypted_ports"] = exposed_ports

sandbox = await modal.Sandbox.create.aio(*PRIMO_SANDBOX_COMMAND, **create_kwargs)
sandbox = await create_primo_sandbox(config.repo_owner, config.repo_name, **create_kwargs)

modal_object_id = sandbox.object_id
code_server_url, ttyd_url, extra_tunnel_urls = await self._resolve_and_setup_tunnels(
Expand Down Expand Up @@ -522,15 +517,15 @@ async def create_build_sandbox(

inject_vcs_env_vars(env_vars, clone_token or None)

sandbox = await modal.Sandbox.create.aio(
*PRIMO_SANDBOX_COMMAND,
sandbox = await create_primo_sandbox(
repo_owner,
repo_name,
image=base_image,
app=app,
secrets=[],
timeout=timeout_seconds,
workdir="/workspace",
env=env_vars,
**primo_sandbox_create_kwargs(repo_owner, repo_name),
)

modal_object_id = sandbox.object_id
Expand Down Expand Up @@ -745,13 +740,12 @@ async def restore_from_snapshot(
"timeout": timeout_seconds,
"workdir": "/workspace",
"env": env_vars,
**primo_sandbox_create_kwargs(repo_owner, repo_name),
**_resource_kwargs(settings),
}
if exposed_ports:
create_kwargs["encrypted_ports"] = exposed_ports

sandbox = await modal.Sandbox.create.aio(*PRIMO_SANDBOX_COMMAND, **create_kwargs)
sandbox = await create_primo_sandbox(repo_owner, repo_name, **create_kwargs)

modal_object_id = sandbox.object_id
code_server_url, ttyd_url, extra_tunnel_urls = await self._resolve_and_setup_tunnels(
Expand Down
25 changes: 0 additions & 25 deletions packages/modal-infra/tests/test_build_sandbox.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import pytest

from src.images.primo_overlay import PRIMO_SANDBOX_COMMAND
from src.sandbox.manager import SandboxManager


Expand Down Expand Up @@ -45,30 +44,6 @@ async def test_env_vars_include_image_build_mode(monkeypatch):
assert env["IMAGE_BUILD_MODE"] == "true"


@pytest.mark.asyncio
async def test_starts_postgres_before_sandbox_runtime(monkeypatch):
captured = {}
monkeypatch.setattr("src.sandbox.manager.modal.Sandbox.create", _fake_sandbox_create(captured))

manager = SandboxManager()
await manager.create_build_sandbox(repo_owner="acme", repo_name="my-repo")

assert captured["args"] == PRIMO_SANDBOX_COMMAND


@pytest.mark.asyncio
async def test_core_build_uses_vm_runtime_with_ci_sized_resources(monkeypatch):
captured = {}
monkeypatch.setattr("src.sandbox.manager.modal.Sandbox.create", _fake_sandbox_create(captured))

manager = SandboxManager()
await manager.create_build_sandbox(repo_owner="primo-devs", repo_name="core")

assert captured["kwargs"]["cpu"] == 2.0
assert captured["kwargs"]["memory"] == 8192
assert captured["kwargs"]["experimental_options"] == {"vm_runtime": True}


@pytest.mark.asyncio
async def test_env_vars_include_repo_info(monkeypatch):
"""Should include REPO_OWNER, REPO_NAME, and SANDBOX_ID."""
Expand Down
90 changes: 90 additions & 0 deletions packages/modal-infra/tests/test_primo_sandbox_creation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
"""Primo's sandbox-creation guarantees, kept out of upstream's test modules.

Upstream's `test_build_sandbox.py` and `test_sandbox_resources.py` churn on
nearly every sync, so fork assertions live here instead — a file upstream will
never touch and git will never have to merge.
"""

from unittest.mock import AsyncMock

import pytest

from src.images.primo_overlay import PRIMO_SANDBOX_COMMAND
from src.sandbox.manager import SandboxConfig, SandboxManager


def _fake_create(captured: dict):
"""Fake `Sandbox.create` that records the argv and kwargs it was called with."""

async def fake_create_aio(*args, **kwargs):
captured["args"] = args
captured["kwargs"] = kwargs

class FakeSandbox:
object_id = "obj-primo-1"
stdout = None

return FakeSandbox()

fake_create_aio.aio = fake_create_aio
return fake_create_aio


@pytest.mark.asyncio
async def test_build_sandbox_starts_postgres_before_sandbox_runtime(monkeypatch):
captured: dict = {}
monkeypatch.setattr("src.sandbox.manager.modal.Sandbox.create", _fake_create(captured))

await SandboxManager().create_build_sandbox(repo_owner="acme", repo_name="my-repo")

assert captured["args"] == PRIMO_SANDBOX_COMMAND


@pytest.mark.asyncio
async def test_build_sandbox_for_core_uses_vm_runtime_with_ci_sized_resources(monkeypatch):
captured: dict = {}
monkeypatch.setattr("src.sandbox.manager.modal.Sandbox.create", _fake_create(captured))

await SandboxManager().create_build_sandbox(repo_owner="primo-devs", repo_name="core")

assert captured["kwargs"]["cpu"] == 2.0
assert captured["kwargs"]["memory"] == 8192
assert captured["kwargs"]["experimental_options"] == {"vm_runtime": True}
Comment on lines +50 to +52

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Reuse the named Core runtime defaults in these assertions.

Lines 50-52 duplicate the factory resource defaults. Line 90 duplicates the VM runtime options default. Define a named Core VM-options constant in src.images.primo_overlay, then import that constant and the existing resource constants here. This keeps the tests synchronized with the factory configuration.

As per coding guidelines, “Define each default value exactly once in a named constant and import or reuse that constant everywhere.”

Also applies to: 88-90

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/modal-infra/tests/test_primo_sandbox_creation.py` around lines 50 -
52, In the test_primo_sandbox_creation.py file, replace the hardcoded values in
the assertions at lines 50-52 and 88-90 with imported constants from
src.images.primo_overlay. First, define a named constant for the VM runtime
options default (the dict with "vm_runtime": True) in the primo_overlay module,
then import that constant along with the existing Core resource constants (cpu
and memory defaults) into the test file. Replace the assertions checking
captured["kwargs"]["cpu"] == 2.0, captured["kwargs"]["memory"] == 8192, and
captured["kwargs"]["experimental_options"] == {"vm_runtime": True} to use these
imported constants instead of hardcoded values, ensuring the test assertions
stay synchronized with the factory configuration.

Source: Coding guidelines



@pytest.mark.asyncio
async def test_session_sandbox_starts_postgres_before_sandbox_runtime(monkeypatch):
captured: dict = {}
monkeypatch.setattr("src.sandbox.manager.modal.Sandbox.create", _fake_create(captured))
monkeypatch.setattr(
SandboxManager,
"_resolve_and_setup_tunnels",
AsyncMock(return_value=(None, None, None)),
)

await SandboxManager().create_sandbox(SandboxConfig(repo_owner="acme", repo_name="my-repo"))

assert captured["args"] == PRIMO_SANDBOX_COMMAND


@pytest.mark.asyncio
async def test_session_sandbox_for_core_lets_explicit_resources_override_vm_defaults(monkeypatch):
captured: dict = {}
monkeypatch.setattr("src.sandbox.manager.modal.Sandbox.create", _fake_create(captured))
monkeypatch.setattr(
SandboxManager,
"_resolve_and_setup_tunnels",
AsyncMock(return_value=(None, None, None)),
)

await SandboxManager().create_sandbox(
SandboxConfig(
repo_owner="primo-devs",
repo_name="core",
settings={"cpuCores": 3, "memoryMib": 6144},
)
)

assert captured["kwargs"]["cpu"] == 3.0
assert captured["kwargs"]["memory"] == 6144
assert captured["kwargs"]["experimental_options"] == {"vm_runtime": True}
23 changes: 0 additions & 23 deletions packages/modal-infra/tests/test_sandbox_resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,29 +87,6 @@ async def test_create_sandbox_omits_resources_without_settings(self, monkeypatch
assert "cpu" not in captured["kwargs"]
assert "memory" not in captured["kwargs"]

@pytest.mark.asyncio
async def test_core_uses_vm_defaults_and_explicit_resources_override_them(self, monkeypatch):
captured: dict = {}
monkeypatch.setattr("src.sandbox.manager.modal.Sandbox.create", _fake_create(captured))
monkeypatch.setattr(
SandboxManager,
"_resolve_and_setup_tunnels",
AsyncMock(return_value=(None, None, None)),
)

manager = SandboxManager()
await manager.create_sandbox(
SandboxConfig(
repo_owner="primo-devs",
repo_name="core",
settings={"cpuCores": 3, "memoryMib": 6144},
)
)

assert captured["kwargs"]["cpu"] == 3.0
assert captured["kwargs"]["memory"] == 6144
assert captured["kwargs"]["experimental_options"] == {"vm_runtime": True}

@pytest.mark.asyncio
async def test_restore_from_snapshot_passes_resources(self, monkeypatch):
captured: dict = {}
Expand Down
117 changes: 117 additions & 0 deletions packages/slack-bot/src/classifier/index.primo.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
/**
* Primo's classifier-prompt guarantees, kept out of upstream's `index.test.ts`.
*
* That file churns on nearly every sync, so fork assertions live here instead —
* a file upstream will never touch and git will never have to merge.
*/
import { beforeEach, describe, expect, it, vi } from "vitest";
import type { Env, RepoConfig } from "../types";

const {
mockMessagesCreate,
mockGetAvailableRepos,
mockBuildRepoDescriptions,
mockGetRoutingRules,
mockGetAvailableEnvironments,
} = vi.hoisted(() => ({
mockMessagesCreate: vi.fn(),
mockGetAvailableRepos: vi.fn(),
mockBuildRepoDescriptions: vi.fn(),
mockGetRoutingRules: vi.fn(),
mockGetAvailableEnvironments: vi.fn(),
}));

vi.mock("@anthropic-ai/sdk", () => ({
// vitest 4 only treats `function`/`class` implementations as constructable;
// an arrow function here throws "is not a constructor" on `new Anthropic()`.
default: vi.fn().mockImplementation(function () {
return { messages: { create: mockMessagesCreate } };
}),
}));

vi.mock("./repos", () => ({
getAvailableRepos: mockGetAvailableRepos,
buildRepoDescriptions: mockBuildRepoDescriptions,
getRoutingRules: mockGetRoutingRules,
}));

vi.mock("./environments", async (importOriginal) => ({
...((await importOriginal()) as object),
getAvailableEnvironments: mockGetAvailableEnvironments,
getEnvironmentById: vi.fn(),
}));

import { RepoClassifier } from "./index";
import { PRIMO_CLASSIFIER_INSTRUCTIONS } from "./primo-classifier-instructions";

const TEST_REPOS: RepoConfig[] = [
{
id: "acme/prod",
owner: "acme",
name: "prod",
fullName: "acme/prod",
displayName: "prod",
description: "Production worker",
defaultBranch: "main",
private: true,
aliases: ["production"],
keywords: ["worker", "slack"],
},
{
id: "acme/web",
owner: "acme",
name: "web",
fullName: "acme/web",
displayName: "web",
description: "Web application",
defaultBranch: "main",
private: true,
aliases: ["frontend"],
keywords: ["react", "ui"],
},
];

const TEST_ENV = {
ANTHROPIC_API_KEY: "test-api-key",
CLASSIFICATION_MODEL: "claude-haiku-4-5",
} as Env;

describe("RepoClassifier (Primo)", () => {
beforeEach(() => {
vi.clearAllMocks();
mockGetAvailableRepos.mockResolvedValue(TEST_REPOS);
mockGetRoutingRules.mockResolvedValue([]);
mockGetAvailableEnvironments.mockResolvedValue([]);
mockBuildRepoDescriptions.mockResolvedValue("- acme/prod\n- acme/web");
mockMessagesCreate.mockResolvedValue({
content: [
{
type: "tool_use",
id: "toolu_primo",
name: "classify_target",
input: {
targetId: "acme/prod",
confidence: "high",
reasoning: "Defaulted to the core repository.",
alternatives: [],
},
},
],
});
});

it("adds the Primo default-repository instructions to the LLM prompt", async () => {
const classifier = new RepoClassifier(TEST_ENV);
await classifier.classify("estas vivo infeliz?", undefined, "trace-primo");

expect(mockMessagesCreate).toHaveBeenCalledWith(
expect.objectContaining({
messages: [
expect.objectContaining({
content: expect.stringContaining(PRIMO_CLASSIFIER_INSTRUCTIONS.trim()),
}),
],
})
);
});
});
Loading
Loading