refactor(sandbox): address PR #250 round-3 review — dead import, env validation, uuid validator, docs (#243)
Must-fix:
- mcp.module: drop the now-dead EnvironmentModule import (and its stale
comment). McpService no longer injects EnvironmentService; EnvironmentModule
is @Global and imported at the app root, so DI still resolves.
Stability:
- environment.service: route getSandboxTtlMs + the three SANDBOX_MAX_*_BYTES
caps through a shared getPositiveIntEnv() helper that warns once per key and
falls back to the default on a non-integer or <= 0 value (previously the byte
caps did a bare parseInt, so SANDBOX_MAX_TOTAL_BYTES=0 made every stash_page
fail against a 0-byte cap). TTL behavior is unchanged.
Simplification:
- sandbox.controller: replace the homemade UUID_RE with the project's shared
`uuid` validator (import { validate as isValidUUID } from 'uuid'), matching
the attachment routes; update the spec fixtures to valid v4 UUIDs.
- mcp.service: inline the single-caller one-liner buildSandboxConfig() to
this.sandboxStore.asSink() at the wiring site.
Docs:
- CHANGELOG: add an [Unreleased] > Added entry for #243 (stash_page tool,
anonymous GET /api/sb/:id, five SANDBOX_* env vars).
- AGENTS.md: note that GET /api/sb/:id is in the workspace-gate preHandler's
excludedPaths and is fully tokenless, unlike /api/files/public/... which
still resolves a workspace and needs an attachment JWT.
Tests: cap-getter validation (0/-5/abc -> default, valid -> parsed), updated
UUID fixtures. apps/server jest sandbox/environment/mcp: 233 pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -41,6 +41,59 @@ describe('EnvironmentService', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// The three byte caps share the same getPositiveIntEnv() helper as the TTL,
|
||||
// so a non-integer / non-positive value ('0'/'-5'/'abc') falls back to the
|
||||
// documented default and a valid positive integer is returned parsed. Note
|
||||
// parseInt truncates '1.5' -> 1 (a valid positive integer), so that value is
|
||||
// accepted, not rejected — same as the pre-existing TTL getter.
|
||||
describe.each([
|
||||
{
|
||||
name: 'getSandboxMaxBytes',
|
||||
key: 'SANDBOX_MAX_BYTES',
|
||||
def: 8_388_608,
|
||||
getter: (s: EnvironmentService) => s.getSandboxMaxBytes(),
|
||||
},
|
||||
{
|
||||
name: 'getSandboxMaxImageBytes',
|
||||
key: 'SANDBOX_MAX_IMAGE_BYTES',
|
||||
def: 20_971_520,
|
||||
getter: (s: EnvironmentService) => s.getSandboxMaxImageBytes(),
|
||||
},
|
||||
{
|
||||
name: 'getSandboxMaxTotalBytes',
|
||||
key: 'SANDBOX_MAX_TOTAL_BYTES',
|
||||
def: 134_217_728,
|
||||
getter: (s: EnvironmentService) => s.getSandboxMaxTotalBytes(),
|
||||
},
|
||||
])('$name', ({ key, def, getter }) => {
|
||||
// ConfigService stub: get(k, d) returns the configured value for THIS cap's
|
||||
// key (falling back to d), and the default for every other key.
|
||||
const build = (value?: string) =>
|
||||
new EnvironmentService({
|
||||
get: (k: string, d?: string) =>
|
||||
k === key ? (value ?? d) : d,
|
||||
} as any);
|
||||
|
||||
it.each(['0', '-5', 'abc'])(
|
||||
`falls back to the ${def} default for invalid value %s`,
|
||||
(value) => {
|
||||
expect(getter(build(value))).toBe(def);
|
||||
},
|
||||
);
|
||||
|
||||
it('returns the parsed value for a valid positive integer', () => {
|
||||
expect(getter(build('4096'))).toBe(4096);
|
||||
});
|
||||
|
||||
it('truncates a non-integer like "1.5" to 1 via parseInt (not rejected)', () => {
|
||||
expect(getter(build('1.5'))).toBe(1);
|
||||
});
|
||||
|
||||
it(`uses the ${def} default when the env is unset`, () => {
|
||||
expect(getter(build(undefined))).toBe(def);
|
||||
});
|
||||
});
|
||||
|
||||
describe('getSandboxPublicUrl', () => {
|
||||
// Stub that resolves BOTH keys the public-url logic consults.
|
||||
const build = (vals: { sandboxUrl?: string; appUrl?: string }) =>
|
||||
|
||||
Reference in New Issue
Block a user