feat(plumbing): hermetic mock Auth0 CLI + trace-aware judge for tenant-config evals#82
feat(plumbing): hermetic mock Auth0 CLI + trace-aware judge for tenant-config evals#82sanchitmehtagit wants to merge 15 commits into
Conversation
…t-config evals
Shared framework infrastructure for tenant-configuration evals (no eval content
of its own — MFA and Custom Token Exchange feature branches build on this).
- Mock Auth0 CLI (mocks/auth0): version-controlled no-op stub prepended to the
agent PATH so tenant commands succeed hermetically — no auth, network, or live
side effects. Stateful read-after-write via EVAL_MOCK_STATE_DIR (a per-run temp
dir outside the workspace). Path normalization collapses bare, leading-slash,
/api/v2/, and full-URL forms to one route. Unmapped writes echo {"ok":true}.
- Trace-aware LLM judge: judge() gains { includeCommandTrace } so a file-less
CLI eval's command trace is judgeable. Off by default — file-based judges are
unchanged.
- Wiring: EVAL_MOCK_BIN_DIR / EVAL_MOCK_STATE_DIR forwarded via filteredEnv();
run.ts creates per-run state dir locally; docker Dockerfile/entrypoint for the
sandbox.
- Tests: auth0-mock, judge trace, primitives, env, discovery, loader, validators.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a manifest-driven mock runtime for Auth0 CLI calls in eval-core, wired through Docker and CLI runners via environment variables. It also adds judge command-trace support, a ChangesDeclarative Auth0 mock runtime
Mock environment wiring
Grader command trace support
Discovery refactor, shared context, and formatting cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Entrypoint as docker/entrypoint.sh
participant AgentCLI as auth0 mock CLI
participant Engine as runMockCli
participant Manifests as loadManifests/applyVerb
participant State as MockState
Entrypoint->>Entrypoint: export EVAL_MOCK_BIN_DIR, EVAL_MOCK_STATE_DIR
AgentCLI->>Engine: runMockCli(argv, config, handlers)
Engine->>Manifests: loadManifests(manifestDirs)
Manifests-->>Engine: RouteManifest[]
Engine->>Engine: routeMatches(pattern, method, path)
alt route verb is handler
Engine->>Manifests: handlers[route.handler](context)
else declarative verb
Engine->>Manifests: applyVerb(route, state, fixturesDir)
Manifests->>State: has/set/clear(key)
State-->>Manifests: marker state
end
Manifests-->>Engine: response body
Engine-->>AgentCLI: JSON stdout
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…nery
The standalone-eval design (one eval per tenant-config method) supersedes the
frontmatter-driven fan-out. Nothing uses it: no PROMPT.md declares
tenant_config_methods, every graders.ts calls bare defineGraders(), and
--tenant-config filters on an always-undefined field.
Removes:
- discovery.ts: tenant_config_methods fan-out + scaffold_<method> resolution
- loader.ts: tenantConfigMethod / variantScaffold / {{tenant_config_instruction}}
substitution + defineGraders(method) arg
- eval-graders: TenantConfigMethod type + TENANT_CONFIG_INSTRUCTIONS map
- CLI: --tenant-config flag, RunConfig.tenantConfig, validateTenantConfig
- the fan-out-only tests (tenant-config-method, validators, discovery/loader cases)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Split the monolithic mocks/auth0 case-statement into a dispatcher that owns only
the shared mechanism and per-feature route files that own their API surface.
- mocks/auth0: parses `api <method> <path>`, normalizes the path, sources every
mocks/routes/*.sh until one handles the request (sets HANDLED via emit()), then
applies the fallthrough (writes → {"ok":true}, reads → {}). Knows nothing about
guardian, token-exchange, or any specific surface.
- mocks/lib.sh: shared helpers — emit / record_state / has_state / clear_state.
- mocks/routes/README.md: the route-file contract.
A feature now adds its endpoints by dropping ONE file in mocks/routes/ — no edit
to the dispatcher, so route files never merge-conflict. Route files are named by
API surface (guardian.sh, token-exchange.sh) and ship with the PR of the feature
that consumes them; guardian/token-exchange routes move out of plumbing into the
MFA and CTE PRs respectively.
Dispatcher tests are now feature-agnostic (use a throwaway fixture route),
exercising normalization, route discovery, read-after-write via helpers, and the
fallthrough.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/eval-core/src/loader.ts (1)
142-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScaffold error paths hardcode
PROMPT.md, ignoringpromptFile.
resolveScaffoldFromMetabuildsEvalConfigErrorpaths with a literaljoin(evalPath, 'PROMPT.md')(Lines 153, 158), butloadEvalnow allows an alternate prompt filename viaevalConfig.promptFile(Line 51). If a config uses a non-defaultpromptFile, a scaffold resolution failure will report aPROMPT.mdpath that doesn't exist for that eval, misleading debugging.🐛 Proposed fix to thread promptFileName through
-function resolveScaffoldFromMeta(scaffoldMeta: string | undefined, evalPath: string, frameworkRoot: string): string { +function resolveScaffoldFromMeta( + scaffoldMeta: string | undefined, + evalPath: string, + frameworkRoot: string, + promptFileName: string, +): string { if (!scaffoldMeta) { return join(evalPath, 'scaffold'); } let resolvedPath: string; try { resolvedPath = resolveInside(frameworkRoot, scaffoldMeta); } catch { throw new EvalConfigError( `scaffold path is invalid or escapes project root: ${scaffoldMeta}`, - join(evalPath, 'PROMPT.md'), + join(evalPath, promptFileName), ); } if (!existsSync(resolvedPath)) { - throw new EvalConfigError(`scaffold path does not exist: ${resolvedPath}`, join(evalPath, 'PROMPT.md')); + throw new EvalConfigError(`scaffold path does not exist: ${resolvedPath}`, join(evalPath, promptFileName)); } return resolvedPath; }And update the call site to pass
promptFileName.🤖 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/eval-core/src/loader.ts` around lines 142 - 159, `resolveScaffoldFromMeta` is hardcoding `PROMPT.md` in its `EvalConfigError` paths, which breaks when `loadEval` uses a custom `evalConfig.promptFile`. Update `resolveScaffoldFromMeta` to accept the prompt filename (or full prompt path) from `loadEval` and use that value in both error cases instead of the literal `PROMPT.md`. Make sure the call site in `loadEval` threads the existing `promptFileName` through so scaffold resolution failures point to the correct prompt file.packages/eval/src/cli/run.ts (1)
146-180: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid mutating shared
process.envfor per-run mock stateLocal agent jobs run under the
pLimit(workers)pool, soprocess.env.EVAL_MOCK_STATE_DIRcan be overwritten or deleted by a sibling job beforefilteredEnv()snapshots it for the child process. That can send two evals to the same mock-state directory or a removed one.Thread the mock state dir through the spawned env instead of mutating global
process.env, or serialize local non-sandboxed runs.🤖 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/eval/src/cli/run.ts` around lines 146 - 180, The local run path in run.ts is mutating shared process.env for EVAL_MOCK_STATE_DIR, which can be clobbered by concurrent pLimit(workers) jobs before filteredEnv() captures it. Stop setting the mock state dir on process.env in run() and instead thread the per-run mockStateDir directly into the spawned child env (the same place filteredEnv() builds the agent environment), or otherwise serialize non-sandboxed local execution. Keep the per-run directory creation in mkdtempSync and ensure each job gets its own isolated state dir without relying on global process.env.
🧹 Nitpick comments (7)
packages/eval-core/src/graders/executors/llm-judge.ts (1)
37-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
RUN_COMMAND_NAMESis hardcoded to two runner tool names.Tool names that represent shell execution across runners (Claude: run_command, Gemini: bash). This is fine for the current base PR, but as more agent runners are wired into evals (e.g. Codex), this set will need updating in lockstep or their commands will silently be dropped from the trace with no warning.
Not blocking for this PR since it's scoped as shared plumbing, but worth keeping in mind as future runners are added.
🤖 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/eval-core/src/graders/executors/llm-judge.ts` around lines 37 - 53, The command-trace filter in formatCommandTrace currently hardcodes RUN_COMMAND_NAMES to only run_command and bash, so commands from newly added runners can be silently omitted. Update the shell-tool पहचान in llm-judge.ts to use a centralized/extensible source of runner tool names (or a shared helper/constants map) so adding runners only requires one place to update, and keep formatCommandTrace using that symbol when deciding which EventToolCall entries to include.packages/eval-core/src/discovery.ts (1)
87-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
buildEvalConfigsalways returns a single-element array.The function is pluralized and returns
EvalConfig[], but it only ever builds and returns one config from a singlePROMPT.md. Per the PR history, the fan-out machinery (tenant_config_methods) that would have justified multiple configs per prompt was removed in favor of standalone evals, so this array wrapping now appears to be residual generality with no current caller that produces more than one element. Consider returning a singleEvalConfigdirectly and letfindEvalDirspush it, unless a near-term follow-up (e.g., the MFA Leg 2 / Custom Token Exchange PRs mentioned in the stack) is expected to reintroduce multi-config output from one prompt.🤖 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/eval-core/src/discovery.ts` around lines 87 - 106, buildEvalConfigs currently always produces exactly one EvalConfig, so the EvalConfig[] return type and array literal are unnecessary residual generality. Update buildEvalConfigs to return a single EvalConfig instead of an array, and adjust its caller in findEvalDirs to push that object directly; keep the existing validation and field construction (id, name, category, path) unchanged.mocks/lib.sh (2)
21-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilence the SC2034 false positive on
HANDLED.Shellcheck can't see that
HANDLEDis consumed bymocks/auth0after sourcing this file, hence the "appears unused" warning. Add a disable directive so CI shellcheck runs stay clean without masking real findings later.🔇 Suggested annotation
emit() { printf '%s\n' "$1" + # shellcheck disable=SC2034 # consumed by the dispatcher (mocks/auth0) after sourcing HANDLED=1 }🤖 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 `@mocks/lib.sh` around lines 21 - 27, The HANDLED variable in emit() is triggering a ShellCheck SC2034 false positive because its use is only visible in mocks/auth0 after sourcing this file. Add a ShellCheck disable directive around the HANDLED assignment in mocks/lib.sh, keeping the emit() behavior unchanged while silencing only this warning and preserving future checks.Source: Linters/SAST tools
12-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMarker-key path helpers don't sanitize input against traversal.
record_state/clear_state/has_statebuild the target path via plain"$STATE/$1"concatenation. Callers are expected to pass static, namespaced literals (per the doc comment), but since this is shared infra other feature route files will build on (MFA Leg 2, CTE), if any future route ever derives the key from request data (e.g.$PATH_, which is influenced by the agent's command argument), a value like../../etc/passwdwould lettouch/rm/test operations escape$STATE. Worth a defensive guard now while the helper is small.🛡️ Proposed guard
+_sanitize_key() { + case "$1" in + */*|*..*) return 1 ;; + esac +} + -record_state() { touch "$STATE/$1" 2>/dev/null || true; } +record_state() { _sanitize_key "$1" && touch "$STATE/$1" 2>/dev/null || true; } -clear_state() { rm -f "$STATE/$1" 2>/dev/null || true; } +clear_state() { _sanitize_key "$1" && rm -f "$STATE/$1" 2>/dev/null || true; } -has_state() { [ -f "$STATE/$1" ]; } +has_state() { _sanitize_key "$1" && [ -f "$STATE/$1" ]; }🤖 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 `@mocks/lib.sh` around lines 12 - 19, The state marker helpers in lib.sh accept raw keys and concatenate them directly into "$STATE/$1", so harden record_state, clear_state, and has_state against path traversal by validating or sanitizing the key before using it as a path segment. Update the shared helper logic so only safe marker names are allowed, and keep the existing behavior for legitimate static namespaced literals used by the route files.docker/Dockerfile (1)
70-78: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueMock stubs aren't covered by the later read-only lockdown.
Line 93 (
RUN chmod -R a-w /app/node_modules /app/packages /app/apps) doesn't include/app/mocks. In practice this is already effectively immutable to the non-root runtime user sinceCOPYleaves it root-owned andchmod +xsets755(no write bit for non-owner), but for consistency with the stated intent ("Make application code read-only to prevent tampering") it'd be cleaner to include it explicitly.🔒 Optional consistency fix
-RUN chmod -R a-w /app/node_modules /app/packages /app/apps +RUN chmod -R a-w /app/node_modules /app/packages /app/apps /app/mocks🤖 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 `@docker/Dockerfile` around lines 70 - 78, The mock CLI stubs copied by the Dockerfile should be explicitly covered by the read-only lockdown for consistency with the immutability intent. Update the final permission-hardening step to include /app/mocks alongside /app/node_modules, /app/packages, and /app/apps, using the existing Dockerfile setup around COPY mocks/ /app/mocks/ and the chmod hardening command.mocks/README.md (1)
73-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFenced code block missing a language tag.
The directory-tree fence (line 73) has no language identifier, per the markdownlint MD040 hint.
Proposed fix
-``` +```text mocks/ ├── auth0 # dispatcher — shared mechanism, no feature knowledge🤖 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 `@mocks/README.md` around lines 73 - 81, The directory tree in mocks/README.md is missing a fenced code block language tag, triggering the markdownlint MD040 warning. Update the fence around the directory-tree example to use the appropriate language identifier so the block is explicitly marked as text, keeping the content unchanged. Locate the fenced example near the mocks/README.md directory layout section and apply the tag consistently to the opening fence.Source: Linters/SAST tools
packages/eval-core/tests/auth0-mock.test.ts (1)
31-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest writes a fixture route file directly into the real
mocks/routes/source directory.
beforeEach/afterEachcreate and deletemocks/routes/zz-mock-test-fixture.shin the actual package directory rather than an isolated location, because the dispatcher's$MOCK_DIR/routesis alwaysdirname "$0". If the test process is killed mid-run (timeout, CI cancellation, crash) beforeafterEachfires, the fixture file is left behind in the working tree, where it would (a) show up as untracked repo state and (b) be picked up by the dispatcher for any other concurrent eval/process sharing the same checkout (e.g., parallel Vitest workers or a concurrently-running eval that also invokesmocks/auth0).Consider allowing the dispatcher to read the routes directory from an overridable env var (e.g.
EVAL_MOCK_ROUTES_DIR, defaulting to$MOCK_DIR/routes) so this test can point at a throwaway temp directory instead of mutating the realmocks/routes/.🤖 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/eval-core/tests/auth0-mock.test.ts` around lines 31 - 49, The test currently mutates the real mocks/routes directory by creating zz-mock-test-fixture.sh in beforeEach and deleting it in afterEach, which can leak state if cleanup never runs. Update the auth0 mock dispatcher (and the test setup) so the routes directory can be overridden via an env var such as EVAL_MOCK_ROUTES_DIR, defaulting to the current $MOCK_DIR/routes behavior. Then point the test at a temp routes directory instead of writing into the package source tree.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/eval-core/src/utils/env.ts`:
- Around line 44-53: The mock-bin PATH injection in env.ts bypasses the existing
case-insensitive environment-key lookup, so on Windows it may create a new PATH
entry instead of updating the resolved key. Update the mock injection logic to
reuse the same resolved PATH key used earlier in the function, and prepend
mockBinDir to that value instead of hardcoding env.PATH. This keeps the behavior
consistent in env normalization and avoids duplicate Path/PATH entries.
---
Outside diff comments:
In `@packages/eval-core/src/loader.ts`:
- Around line 142-159: `resolveScaffoldFromMeta` is hardcoding `PROMPT.md` in
its `EvalConfigError` paths, which breaks when `loadEval` uses a custom
`evalConfig.promptFile`. Update `resolveScaffoldFromMeta` to accept the prompt
filename (or full prompt path) from `loadEval` and use that value in both error
cases instead of the literal `PROMPT.md`. Make sure the call site in `loadEval`
threads the existing `promptFileName` through so scaffold resolution failures
point to the correct prompt file.
In `@packages/eval/src/cli/run.ts`:
- Around line 146-180: The local run path in run.ts is mutating shared
process.env for EVAL_MOCK_STATE_DIR, which can be clobbered by concurrent
pLimit(workers) jobs before filteredEnv() captures it. Stop setting the mock
state dir on process.env in run() and instead thread the per-run mockStateDir
directly into the spawned child env (the same place filteredEnv() builds the
agent environment), or otherwise serialize non-sandboxed local execution. Keep
the per-run directory creation in mkdtempSync and ensure each job gets its own
isolated state dir without relying on global process.env.
---
Nitpick comments:
In `@docker/Dockerfile`:
- Around line 70-78: The mock CLI stubs copied by the Dockerfile should be
explicitly covered by the read-only lockdown for consistency with the
immutability intent. Update the final permission-hardening step to include
/app/mocks alongside /app/node_modules, /app/packages, and /app/apps, using the
existing Dockerfile setup around COPY mocks/ /app/mocks/ and the chmod hardening
command.
In `@mocks/lib.sh`:
- Around line 21-27: The HANDLED variable in emit() is triggering a ShellCheck
SC2034 false positive because its use is only visible in mocks/auth0 after
sourcing this file. Add a ShellCheck disable directive around the HANDLED
assignment in mocks/lib.sh, keeping the emit() behavior unchanged while
silencing only this warning and preserving future checks.
- Around line 12-19: The state marker helpers in lib.sh accept raw keys and
concatenate them directly into "$STATE/$1", so harden record_state, clear_state,
and has_state against path traversal by validating or sanitizing the key before
using it as a path segment. Update the shared helper logic so only safe marker
names are allowed, and keep the existing behavior for legitimate static
namespaced literals used by the route files.
In `@mocks/README.md`:
- Around line 73-81: The directory tree in mocks/README.md is missing a fenced
code block language tag, triggering the markdownlint MD040 warning. Update the
fence around the directory-tree example to use the appropriate language
identifier so the block is explicitly marked as text, keeping the content
unchanged. Locate the fenced example near the mocks/README.md directory layout
section and apply the tag consistently to the opening fence.
In `@packages/eval-core/src/discovery.ts`:
- Around line 87-106: buildEvalConfigs currently always produces exactly one
EvalConfig, so the EvalConfig[] return type and array literal are unnecessary
residual generality. Update buildEvalConfigs to return a single EvalConfig
instead of an array, and adjust its caller in findEvalDirs to push that object
directly; keep the existing validation and field construction (id, name,
category, path) unchanged.
In `@packages/eval-core/src/graders/executors/llm-judge.ts`:
- Around line 37-53: The command-trace filter in formatCommandTrace currently
hardcodes RUN_COMMAND_NAMES to only run_command and bash, so commands from newly
added runners can be silently omitted. Update the shell-tool पहचान in
llm-judge.ts to use a centralized/extensible source of runner tool names (or a
shared helper/constants map) so adding runners only requires one place to
update, and keep formatCommandTrace using that symbol when deciding which
EventToolCall entries to include.
In `@packages/eval-core/tests/auth0-mock.test.ts`:
- Around line 31-49: The test currently mutates the real mocks/routes directory
by creating zz-mock-test-fixture.sh in beforeEach and deleting it in afterEach,
which can leak state if cleanup never runs. Update the auth0 mock dispatcher
(and the test setup) so the routes directory can be overridden via an env var
such as EVAL_MOCK_ROUTES_DIR, defaulting to the current $MOCK_DIR/routes
behavior. Then point the test at a temp routes directory instead of writing into
the package source tree.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 92ada64b-1f1d-478f-8d03-cea88366e7d8
📒 Files selected for processing (21)
docker/Dockerfiledocker/entrypoint.shmocks/README.mdmocks/auth0mocks/lib.shmocks/routes/README.mdpackages/eval-core/src/discovery.tspackages/eval-core/src/graders/executors/llm-judge.tspackages/eval-core/src/loader.tspackages/eval-core/src/utils/env.tspackages/eval-core/tests/auth0-mock.test.tspackages/eval-core/tests/env.test.tspackages/eval-core/tests/graders/executors.test.tspackages/eval-graders/src/index.tspackages/eval-graders/src/primitives.tspackages/eval-graders/src/types.tspackages/eval-graders/tests/primitives.test.tspackages/eval/src/cli/run.tspackages/eval/src/runners/claude-code/agent.tspackages/eval/src/runners/codex/agent.tspackages/eval/tests/runners/claude-code-agent.test.ts
| // Mock CLI shims. When EVAL_MOCK_BIN_DIR points at an existing directory, | ||
| // prepend it to PATH so stubbed external CLIs (e.g. `auth0`) resolve to the | ||
| // hermetic no-op stubs in mocks/ instead of a real binary. Prepending | ||
| // (not appending) guarantees a real install on the host can never be hit — | ||
| // keeping runs deterministic and preventing accidental live side effects. | ||
| // Both execution paths set this: entrypoint.sh in the sandbox, run.ts locally. | ||
| const mockBinDir = process.env.EVAL_MOCK_BIN_DIR; | ||
| if (mockBinDir && existsSync(mockBinDir)) { | ||
| env.PATH = env.PATH ? `${mockBinDir}${delimiter}${env.PATH}` : mockBinDir; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
env.PATH bypasses the file's own case-insensitive Windows key handling.
Lines 20-36 of this function deliberately resolve PATH (and other keys) case-insensitively because on Windows, env var names like Path, ComSpec, SystemRoot are common mixed-case variants; this ensures we find them regardless of casing. The stored key in env therefore may be Path, not PATH.
The new mock-injection code hardcodes env.PATH:
env.PATH = env.PATH ? `${mockBinDir}${delimiter}${env.PATH}` : mockBinDir;
On Windows, if the resolved key is Path, env.PATH reads as undefined and this line creates a second, distinct PATH key holding only mockBinDir — the object ends up with both Path (full original value) and PATH (just the mock dir) as separate properties. Downstream, whichever key wins for the spawned process could either drop the mock dir entirely or clobber the real PATH, breaking CLI resolution for the agent.
🐛 Proposed fix
const mockBinDir = process.env.EVAL_MOCK_BIN_DIR;
if (mockBinDir && existsSync(mockBinDir)) {
- env.PATH = env.PATH ? `${mockBinDir}${delimiter}${env.PATH}` : mockBinDir;
+ const pathKey = Object.keys(env).find((k) => k.toLowerCase() === 'path') ?? 'PATH';
+ env[pathKey] = env[pathKey] ? `${mockBinDir}${delimiter}${env[pathKey]}` : mockBinDir;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Mock CLI shims. When EVAL_MOCK_BIN_DIR points at an existing directory, | |
| // prepend it to PATH so stubbed external CLIs (e.g. `auth0`) resolve to the | |
| // hermetic no-op stubs in mocks/ instead of a real binary. Prepending | |
| // (not appending) guarantees a real install on the host can never be hit — | |
| // keeping runs deterministic and preventing accidental live side effects. | |
| // Both execution paths set this: entrypoint.sh in the sandbox, run.ts locally. | |
| const mockBinDir = process.env.EVAL_MOCK_BIN_DIR; | |
| if (mockBinDir && existsSync(mockBinDir)) { | |
| env.PATH = env.PATH ? `${mockBinDir}${delimiter}${env.PATH}` : mockBinDir; | |
| } | |
| // Mock CLI shims. When EVAL_MOCK_BIN_DIR points at an existing directory, | |
| // prepend it to PATH so stubbed external CLIs (e.g. `auth0`) resolve to the | |
| // hermetic no-op stubs in mocks/ instead of a real binary. Prepending | |
| // (not appending) guarantees a real install on the host can never be hit — | |
| // keeping runs deterministic and preventing accidental live side effects. | |
| // Both execution paths set this: entrypoint.sh in the sandbox, run.ts locally. | |
| const mockBinDir = process.env.EVAL_MOCK_BIN_DIR; | |
| if (mockBinDir && existsSync(mockBinDir)) { | |
| const pathKey = Object.keys(env).find((k) => k.toLowerCase() === 'path') ?? 'PATH'; | |
| env[pathKey] = env[pathKey] ? `${mockBinDir}${delimiter}${env[pathKey]}` : mockBinDir; | |
| } |
🤖 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/eval-core/src/utils/env.ts` around lines 44 - 53, The mock-bin PATH
injection in env.ts bypasses the existing case-insensitive environment-key
lookup, so on Windows it may create a new PATH entry instead of updating the
resolved key. Update the mock injection logic to reuse the same resolved PATH
key used earlier in the function, and prepend mockBinDir to that value instead
of hardcoding env.PATH. This keeps the behavior consistent in env normalization
and avoids duplicate Path/PATH entries.
…nostic The mock Auth0 CLI is Auth0-specific (binary named `auth0`, normalizes the Management API /api/v2 path, guardian/token-exchange routes), so it belongs with the app that owns the Auth0 evals — not in generic framework code. eval-core keeps only the generic contract it already had: prepend EVAL_MOCK_BIN_DIR to PATH, forward EVAL_MOCK_STATE_DIR. It never learns the CLI is Auth0. - Move mocks/ → apps/auth0-evals/mocks/ (git rename; dispatcher + lib.sh + routes/ contract). - run.ts / sandbox-runner: anchor EVAL_MOCK_BIN_DIR to <cwd>/mocks (= apps/auth0-evals/mocks) instead of a fragile ../../../../ walk from dist. - Per-eval routes: an eval may ship routes/*.sh next to its PROMPT.md; the runner sets EVAL_MOCK_ROUTES_DIRS and the dispatcher sources it after the app-level routes/. env.ts forwards the var. - Docker: COPY apps/auth0-evals/mocks/ → /app/mocks (unchanged runtime path). - Guardrails (defense-in-depth; route files never reach a workspace by construction): add `routes` to EXCLUDED_EVAL_DIRS and to the judge's excluded dirs, plus tests asserting per-eval route discovery and the exclusions. Spec: docs/superpowers/specs/2026-07-07-mock-location-design.md Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/eval/src/cli/run.ts (2)
141-188: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffMock env plumbing relies on an unenforced "one job per process" invariant.
EVAL_MOCK_BIN_DIR/EVAL_MOCK_ROUTES_DIRS/EVAL_MOCK_STATE_DIRare written toprocess.env(global mutable state) per job. This is currently safe only becauserunCli()dispatches to a subprocess-per-job wheneverjobs.length > 1(Line 455-497), so the in-processpLimit(workers)loop that callsrunJob/runAgentJobconcurrently (Line 527-558) only ever runs a single job at a time. If that dispatch threshold or batching logic is ever changed, concurrentrunAgentJobcalls in the same process would race on these env vars and could leak one eval's mock routes/state into another eval's run, silently corrupting eval results.Consider either asserting this invariant explicitly (e.g., throw if
runAgentJobis invoked while a mock state dir is already active) or plumbing these values through function parameters/an options object instead of globalprocess.env, so correctness doesn't depend on an implicit cross-file contract withrunCli().Also applies to: 230-235
🤖 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/eval/src/cli/run.ts` around lines 141 - 188, The mock environment setup in run.ts depends on an implicit “one job per process” guarantee because runJob/runAgentJob mutate process.env globals like EVAL_MOCK_BIN_DIR, EVAL_MOCK_ROUTES_DIRS, and EVAL_MOCK_STATE_DIR. Fix this by making the invariant explicit in the job entry path (for example in runAgentJob/runJob) or by passing mock env values through parameters/options instead of global process.env, so concurrent runs can’t race and leak state between evals.
176-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate per-eval routes-dir resolution with sandbox-runner.ts.
This
evalRoutesDirexistence check andEVAL_MOCK_ROUTES_DIRSassignment duplicates the same logic inpackages/eval/src/cli/sandbox-runner.ts(Lines 106-109). Consider extracting a small shared helper (e.g., in@a0/eval-core) that both the local and sandboxed runners call, to keep the "routes/ next to PROMPT.md" convention defined in one place.♻️ Proposed shared helper
+// packages/eval-core/src/utils/mock-routes.ts +export function resolveEvalMockRoutesDir(evalPath: string): string | undefined { + const dir = join(evalPath, 'routes'); + return existsSync(dir) ? dir : undefined; +}Then in both call sites:
-const evalRoutesDir = join(evalDef.path, 'routes'); -if (existsSync(evalRoutesDir)) { - process.env.EVAL_MOCK_ROUTES_DIRS = evalRoutesDir; -} else { - delete process.env.EVAL_MOCK_ROUTES_DIRS; -} +const evalRoutesDir = resolveEvalMockRoutesDir(evalDef.path); +if (evalRoutesDir) { + process.env.EVAL_MOCK_ROUTES_DIRS = evalRoutesDir; +} else { + delete process.env.EVAL_MOCK_ROUTES_DIRS; +}🤖 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/eval/src/cli/run.ts` around lines 176 - 181, The per-eval routes directory resolution in run.ts duplicates the same “routes next to PROMPT.md” logic already used by sandbox-runner.ts, so extract it into a small shared helper in `@a0/eval-core` and have both runners call that helper instead of reimplementing the existsSync/process.env.EVAL_MOCK_ROUTES_DIRS handling. Use the existing evalRoutesDir setup in run.ts and the matching code in sandbox-runner.ts as the locations to replace with the shared function so the convention is defined in one place.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/eval-core/tests/auth0-mock.test.ts`:
- Around line 17-18: The fixture route is being written into the shared mocks
routes tree, which can leak between test runs; update the auth0 mock test setup
to use a per-test temp routes directory instead of ROUTES_DIR under
apps/auth0-evals/mocks/routes. Adjust the auth0-mock.test.ts fixture creation
logic in beforeEach so zz-mock-test-fixture.sh is created under a temporary
directory, and make sure the mock dispatcher points to that temp path rather
than the shared route set.
---
Nitpick comments:
In `@packages/eval/src/cli/run.ts`:
- Around line 141-188: The mock environment setup in run.ts depends on an
implicit “one job per process” guarantee because runJob/runAgentJob mutate
process.env globals like EVAL_MOCK_BIN_DIR, EVAL_MOCK_ROUTES_DIRS, and
EVAL_MOCK_STATE_DIR. Fix this by making the invariant explicit in the job entry
path (for example in runAgentJob/runJob) or by passing mock env values through
parameters/options instead of global process.env, so concurrent runs can’t race
and leak state between evals.
- Around line 176-181: The per-eval routes directory resolution in run.ts
duplicates the same “routes next to PROMPT.md” logic already used by
sandbox-runner.ts, so extract it into a small shared helper in `@a0/eval-core` and
have both runners call that helper instead of reimplementing the
existsSync/process.env.EVAL_MOCK_ROUTES_DIRS handling. Use the existing
evalRoutesDir setup in run.ts and the matching code in sandbox-runner.ts as the
locations to replace with the shared function so the convention is defined in
one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 435858eb-cb92-4e6c-97d8-e965239502db
📒 Files selected for processing (13)
apps/auth0-evals/mocks/README.mdapps/auth0-evals/mocks/auth0apps/auth0-evals/mocks/lib.shapps/auth0-evals/mocks/routes/README.mddocker/Dockerfiledocs/superpowers/specs/2026-07-07-mock-location-design.mdpackages/eval-core/src/graders/engine.tspackages/eval-core/src/graders/executors/llm-judge.tspackages/eval-core/src/utils/env.tspackages/eval-core/tests/auth0-mock.test.tspackages/eval-core/tests/graders/executors.test.tspackages/eval/src/cli/run.tspackages/eval/src/cli/sandbox-runner.ts
💤 Files with no reviewable changes (2)
- apps/auth0-evals/mocks/routes/README.md
- apps/auth0-evals/mocks/lib.sh
✅ Files skipped from review due to trivial changes (1)
- apps/auth0-evals/mocks/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/eval-core/src/utils/env.ts
- packages/eval-core/tests/graders/executors.test.ts
- docker/Dockerfile
…lidation - Add collectRefProblems() to packages/eval-core/src/mock/manifest.ts to validate fixture/handler refs - Export loadManifests, collectRefProblems, resolveBody from mock namespace - Create mock:new script to scaffold <surface>.routes.json, fixtures, and handlers stub - Create mock:check script to probe mock CLI with METHOD/path probes - Add mock:new and mock:check to package.json scripts - Add harness test at packages/eval-core/tests/mock/harness.test.ts to validate all app manifests Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
docs/superpowers/specs/2026-07-07-declarative-mock-runtime-design.md (1)
95-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the handler artifact contract. The spec still reads as if handlers are imported from
<feature>.handlers.ts/dist, but the runtime and scaffolder usehandlers.jsnext to the manifest dir. Call out one runtime contract explicitly and keep the generator, dispatcher, and docs on the same path/extension.🤖 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 `@docs/superpowers/specs/2026-07-07-declarative-mock-runtime-design.md` around lines 95 - 115, The handler artifact contract is inconsistent between the spec and the runtime, so update the documentation and any referenced generator/dispatcher expectations to use one explicit path and extension for handlers. Align the `HandlerContext`/handler loading description with the actual runtime contract used by the scaffolder and dispatcher (the `handlers.js` file beside the manifest directory), and remove or revise any `<feature>.handlers.ts`/`dist` wording so the generator, runtime, and docs all point to the same artifact location.apps/auth0-evals/scripts/mock-check.mjs (1)
6-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame ESLint Node-globals gap as
mock-new.mjs.🤖 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 `@apps/auth0-evals/scripts/mock-check.mjs` around lines 6 - 17, The CLI script in mock-check.mjs still relies on Node globals that ESLint flags, matching the issue in mock-new.mjs. Update the top-level setup around cwd, args, and process.exit usage so it follows the same pattern as the other mock script, explicitly using the appropriate Node globals/imports and keeping the argument parsing and usage handling in sync with mock-new.mjs.Source: Linters/SAST tools
apps/auth0-evals/scripts/mock-new.mjs (1)
6-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winESLint no-undef errors on Node globals indicate missing Node env in lint config.
Static analysis flags
URL,process, andconsoleas undefined. These are legitimate Node.js globals, so this is a lint-config gap (missingenv: node/globals.node) rather than a code defect — but since these are real errors reported for this new file, they will likely fail CI lint if this path is covered by that config.Also applies to: 63-63
🤖 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 `@apps/auth0-evals/scripts/mock-new.mjs` around lines 6 - 17, The new script uses Node globals like URL, process, and console, but the lint setup is treating them as undefined. Update the ESLint configuration to apply a Node environment/globals override for the mock scripts path so the mock:new script is linted with Node globals enabled, rather than changing the script itself. Make sure the override covers the mock-new.mjs file and any related scripts that use the same Node APIs.Source: Linters/SAST tools
packages/eval-core/src/mock/state.ts (1)
6-8: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMarker-name collisions between distinct state keys.
markerNamereplaces every non-[A-Za-z0-9._-]character with_, so distinct keys likefeature.a-bandfeature.a_bcollapse to the same marker file (feature.a_b). Since manifests only require dotted namespacing (Line 17-19 inmanifest.ts) and not full name sanitization uniqueness, two routes using differently-punctuated state keys could silently share state.Low practical risk given current naming conventions, but worth a small hardening.
♻️ Suggested collision-safe encoding
-function markerName(key: string): string { - return key.replace(/[^A-Za-z0-9._-]/g, '_'); -} +function markerName(key: string): string { + return Buffer.from(key).toString('base64url'); +}🤖 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/eval-core/src/mock/state.ts` around lines 6 - 8, The current marker naming in markerName can collapse different state keys into the same filename, so make the encoding collision-safe instead of simple character replacement. Update the marker generation used by the state mock to preserve uniqueness for distinct keys like punctuation variants, and keep the existing manifest namespace handling intact while changing only the transformation logic in markerName (and any callers in mock state that rely on it).packages/eval-core/tests/mock/manifest.test.ts (1)
15-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing failure-path coverage for two
validateRoutebranches.
validateRoute(inmanifest.tsLines 14-16, 20-22) throws for acreate/set/reflectroute missingstate, and for ahandlerroute missinghandler. Neither branch is exercised here; only the unknown-verb and un-namespaced-state branches are covered.✅ Suggested additional test cases
+ it('rejects a create verb missing a state key', () => { + writeManifest('bad.routes.json', { + surface: 'bad', routes: [{ match: 'POST x', verb: 'create', body: {} }], + }); + expect(() => loadManifests([dir])).toThrow(/state/i); + }); + + it('rejects a handler verb missing a handler name', () => { + writeManifest('bad.routes.json', { + surface: 'bad', routes: [{ match: 'GET x', verb: 'handler' }], + }); + expect(() => loadManifests([dir])).toThrow(/handler/i); + });As per path instructions, "Every new function and every logic change must be accompanied by tests in the package where the code lives; new functions need at least one happy-path test and one failure/edge-case test."
🤖 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/eval-core/tests/mock/manifest.test.ts` around lines 15 - 44, Add missing failure-path tests in loadManifests to cover the validateRoute branches for stateful verbs and handler routes. In manifest.test.ts, extend the existing describe('loadManifests') block with cases that write a manifest using a create/set/reflect route without state and a handler route without handler, then assert loadManifests([dir]) throws with the appropriate error. Keep the tests alongside the current unknown-verb and namespace checks so validateRoute is fully covered.Source: Path instructions
packages/eval-core/src/mock/manifest.ts (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
join()instead of template-literal path concatenation.
node:path'sjoinis already imported and used at Line 2/51 for path building, butcollectRefProblems(and the equivalent construction inengine.ts) buildsfixturesDirvia string interpolation instead. This is inconsistent within the same file and less robust to trailing slashes inm.dir.♻️ Suggested fix
- const fixturesDir = `${m.dir}/fixtures/${m.surface}`; + const fixturesDir = join(m.dir ?? '', 'fixtures', m.surface);🤖 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/eval-core/src/mock/manifest.ts` at line 58, `collectRefProblems` is building `fixturesDir` with template-string path concatenation instead of the imported `join()` helper, which is inconsistent and less robust. Update the `fixturesDir` construction in `manifest.ts` to use `join(m.dir, "fixtures", m.surface)` and apply the same path-building approach in the equivalent `engine.ts` logic so both locations use the shared path utility consistently.packages/eval-core/src/mock/types.ts (1)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
unknown | stringunions.
unknown | stringis equivalent tounknown— thestringmember is entirely subsumed. As written, the type gives no compile-time signal thatbody/present/absentcan be either an inline JSON value or a fixture-filename string (that distinction is only enforced at runtime inresolveBodyviatypeof ref !== 'string').♻️ Suggested type clarification
- body?: unknown | string; - present?: unknown | string; - absent?: unknown | string; + /** Inline JSON body, or a fixture filename string resolved via resolveBody(). */ + body?: unknown; + present?: unknown; + absent?: unknown;🤖 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/eval-core/src/mock/types.ts` around lines 20 - 22, The `unknown | string` unions in `types.ts` are redundant and don’t communicate the intended “inline JSON value or fixture filename” contract. Update the `body`, `present`, and `absent` property types to use a clearer shape or alias that reflects the runtime check in `resolveBody` (which branches on `typeof ref !== 'string'`), so callers can see the intended distinction at compile time and the mock types stay self-documenting.packages/eval-core/tests/mock/state.test.ts (1)
7-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for cross-instance (marker-file) persistence.
All three tests operate on a single
createState()instance. The actual use case — per PR objectives, state persists via marker files across separate CLI invocations (EVAL_MOCK_STATE_DIR) — isn't verified: a freshcreateState(dir)call should see keys set by a prior instance pointed at the samedir.As per coding guidelines, "Every new function and every logic change must be accompanied by tests in the package where the code lives."
✅ Suggested additional test
+ it('persists state across separate createState() instances (same dir)', () => { + const s1 = createState(dir); + s1.set('cte.action'); + const s2 = createState(dir); + expect(s2.has('cte.action')).toBe(true); + });🤖 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/eval-core/tests/mock/state.test.ts` around lines 7 - 30, Add a new test in createState to verify marker-file persistence across instances: create one state with createState(dir), set a key, then create a fresh createState(dir) and assert it can read the same key as present. Use the existing createState, set, and has symbols so the test covers the cross-invocation behavior backed by the shared EVAL_MOCK_STATE_DIR directory.Source: Coding guidelines
packages/eval-core/src/mock/engine.ts (1)
22-22: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
payloadincludes the subcommand/method/path, not just the actual request body/args.
argv.join(' ')embedssub,method, andrawPathalongside any actual data flags, which is unusual for a field namedpayload. If handler functions do naive substring parsing onpayloadto extract request data, this noise could cause false matches.♻️ Possible fix (verify against actual handler usage first)
- const payload = argv.join(' '); + const payload = argv.slice(3).join(' ');🤖 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/eval-core/src/mock/engine.ts` at line 22, The mock request payload in the engine currently includes routing metadata from argv rather than only the request body/args. Update the payload construction in the mock engine logic so `payload` contains just the actual user-provided data flags, and keep `sub`, `method`, and `rawPath` separate for dispatching. Verify any consumers of the mock engine payload still receive the expected body-only content and adjust the `engine.ts` request assembly accordingly.packages/eval-core/src/mock/verbs.ts (1)
1-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for
applyVerb
engine.test.tsexercises the create, reflect (present/absent), static, and handler paths indirectly, but a smallpackages/eval-core/src/mock/verbs.tstest would make this branching easier to maintain.🤖 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/eval-core/src/mock/verbs.ts` around lines 1 - 21, Add a focused unit test suite for applyVerb in verbs.ts to cover each branch directly instead of relying only on engine.test.ts. Exercise the create/set path, reflect with both present and absent state, static resolution, and the default handler/undefined case, using applyVerb plus MockState/RouteDef fixtures to keep the assertions close to the branching logic.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In `@apps/auth0-evals/mocks/auth0`:
- Around line 19-24: The dynamic import in the handlers-loading loop uses a bare
absolute path from join(), which is not a valid ESM specifier across platforms.
Update the import in the manifestDirs loop to convert hFile to a file URL first
using pathToFileURL(hFile).href before calling import(), keeping the existing
existsSync and Object.assign logic in place.
In `@apps/auth0-evals/scripts/mock-check.mjs`:
- Around line 28-45: The mock-check script is calling runMockCli with the wrong
argument shape and treating its return value like an array. Update the call in
mock-check.mjs so the argv array is passed first, followed by the config object
and probes/handlers as expected by runMockCli. Then parse the returned JSON
string for each probe before logging method/path/body, instead of iterating
result directly.
In `@docs/superpowers/plans/2026-07-07-declarative-mock-runtime.md`:
- Around line 687-692: The dynamic import in the handler-loading logic uses a
filesystem path directly, which can resolve inconsistently across platforms.
Update the import in the manifest directory scan to use a file URL by converting
the `hFile` path with `pathToFileURL(hFile).href` before calling `import`,
keeping the rest of the `handlers` merge logic unchanged.
In `@docs/superpowers/specs/2026-07-07-declarative-mock-runtime-design.md`:
- Around line 49-67: The architecture layout fence in the declarative mock
runtime design doc is missing a language tag, which triggers markdownlint MD040.
Update the fenced block in the mock runtime directory layout section to use a
text/plaintext language label so the docs lint passes.
In `@packages/eval-core/src/mock/engine.ts`:
- Around line 29-38: The mock response path in engine.ts can return a non-string
when a handler or verb result is undefined because JSON.stringify(undefined)
yields undefined. Update the logic in the route handling block around runMockCli
so the handler call and applyVerb result are checked before stringifying, and
coerce missing responses to null or throw a clear validation error instead of
returning undefined. Use the handler lookup and applyVerb branches to locate the
fix.
In `@packages/eval-core/tests/mock/harness.test.ts`:
- Line 5: The MOCKS path in the harness test points to the wrong directory, so
loadManifests() never sees the real manifests and the assertions remain vacuous.
Update the MOCKS constant in harness.test.ts to resolve to the actual
apps/auth0-evals/mocks location, using the existing fileURLToPath/new URL setup
so the harness exercises real manifests in the test.
---
Nitpick comments:
In `@apps/auth0-evals/scripts/mock-check.mjs`:
- Around line 6-17: The CLI script in mock-check.mjs still relies on Node
globals that ESLint flags, matching the issue in mock-new.mjs. Update the
top-level setup around cwd, args, and process.exit usage so it follows the same
pattern as the other mock script, explicitly using the appropriate Node
globals/imports and keeping the argument parsing and usage handling in sync with
mock-new.mjs.
In `@apps/auth0-evals/scripts/mock-new.mjs`:
- Around line 6-17: The new script uses Node globals like URL, process, and
console, but the lint setup is treating them as undefined. Update the ESLint
configuration to apply a Node environment/globals override for the mock scripts
path so the mock:new script is linted with Node globals enabled, rather than
changing the script itself. Make sure the override covers the mock-new.mjs file
and any related scripts that use the same Node APIs.
In `@docs/superpowers/specs/2026-07-07-declarative-mock-runtime-design.md`:
- Around line 95-115: The handler artifact contract is inconsistent between the
spec and the runtime, so update the documentation and any referenced
generator/dispatcher expectations to use one explicit path and extension for
handlers. Align the `HandlerContext`/handler loading description with the actual
runtime contract used by the scaffolder and dispatcher (the `handlers.js` file
beside the manifest directory), and remove or revise any
`<feature>.handlers.ts`/`dist` wording so the generator, runtime, and docs all
point to the same artifact location.
In `@packages/eval-core/src/mock/engine.ts`:
- Line 22: The mock request payload in the engine currently includes routing
metadata from argv rather than only the request body/args. Update the payload
construction in the mock engine logic so `payload` contains just the actual
user-provided data flags, and keep `sub`, `method`, and `rawPath` separate for
dispatching. Verify any consumers of the mock engine payload still receive the
expected body-only content and adjust the `engine.ts` request assembly
accordingly.
In `@packages/eval-core/src/mock/manifest.ts`:
- Line 58: `collectRefProblems` is building `fixturesDir` with template-string
path concatenation instead of the imported `join()` helper, which is
inconsistent and less robust. Update the `fixturesDir` construction in
`manifest.ts` to use `join(m.dir, "fixtures", m.surface)` and apply the same
path-building approach in the equivalent `engine.ts` logic so both locations use
the shared path utility consistently.
In `@packages/eval-core/src/mock/state.ts`:
- Around line 6-8: The current marker naming in markerName can collapse
different state keys into the same filename, so make the encoding collision-safe
instead of simple character replacement. Update the marker generation used by
the state mock to preserve uniqueness for distinct keys like punctuation
variants, and keep the existing manifest namespace handling intact while
changing only the transformation logic in markerName (and any callers in mock
state that rely on it).
In `@packages/eval-core/src/mock/types.ts`:
- Around line 20-22: The `unknown | string` unions in `types.ts` are redundant
and don’t communicate the intended “inline JSON value or fixture filename”
contract. Update the `body`, `present`, and `absent` property types to use a
clearer shape or alias that reflects the runtime check in `resolveBody` (which
branches on `typeof ref !== 'string'`), so callers can see the intended
distinction at compile time and the mock types stay self-documenting.
In `@packages/eval-core/src/mock/verbs.ts`:
- Around line 1-21: Add a focused unit test suite for applyVerb in verbs.ts to
cover each branch directly instead of relying only on engine.test.ts. Exercise
the create/set path, reflect with both present and absent state, static
resolution, and the default handler/undefined case, using applyVerb plus
MockState/RouteDef fixtures to keep the assertions close to the branching logic.
In `@packages/eval-core/tests/mock/manifest.test.ts`:
- Around line 15-44: Add missing failure-path tests in loadManifests to cover
the validateRoute branches for stateful verbs and handler routes. In
manifest.test.ts, extend the existing describe('loadManifests') block with cases
that write a manifest using a create/set/reflect route without state and a
handler route without handler, then assert loadManifests([dir]) throws with the
appropriate error. Keep the tests alongside the current unknown-verb and
namespace checks so validateRoute is fully covered.
In `@packages/eval-core/tests/mock/state.test.ts`:
- Around line 7-30: Add a new test in createState to verify marker-file
persistence across instances: create one state with createState(dir), set a key,
then create a fresh createState(dir) and assert it can read the same key as
present. Use the existing createState, set, and has symbols so the test covers
the cross-invocation behavior backed by the shared EVAL_MOCK_STATE_DIR
directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fbe9e3f-686a-4ed2-89de-75bbca3fce22
📒 Files selected for processing (21)
apps/auth0-evals/mocks/auth0apps/auth0-evals/mocks/routes/README.mdapps/auth0-evals/package.jsonapps/auth0-evals/scripts/mock-check.mjsapps/auth0-evals/scripts/mock-new.mjsdocs/superpowers/plans/2026-07-07-declarative-mock-runtime.mddocs/superpowers/specs/2026-07-07-declarative-mock-runtime-design.mdpackages/eval-core/src/index.tspackages/eval-core/src/mock/engine.tspackages/eval-core/src/mock/index.tspackages/eval-core/src/mock/manifest.tspackages/eval-core/src/mock/matcher.tspackages/eval-core/src/mock/state.tspackages/eval-core/src/mock/types.tspackages/eval-core/src/mock/verbs.tspackages/eval-core/tests/auth0-mock.test.tspackages/eval-core/tests/mock/engine.test.tspackages/eval-core/tests/mock/harness.test.tspackages/eval-core/tests/mock/manifest.test.tspackages/eval-core/tests/mock/matcher.test.tspackages/eval-core/tests/mock/state.test.ts
✅ Files skipped from review due to trivial changes (1)
- apps/auth0-evals/mocks/routes/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/eval-core/tests/auth0-mock.test.ts
| // Load handler maps from every handlers.js next to a manifest dir. | ||
| const handlers = {}; | ||
| for (const dir of manifestDirs) { | ||
| const hFile = join(dir, 'handlers.js'); | ||
| if (existsSync(hFile)) Object.assign(handlers, (await import(hFile)).default ?? {}); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Dynamic import of bare absolute path violates guideline; breaks on Windows and with special characters.
hFile is an absolute path built with join(), then passed directly to import(). Per Node's ESM loader, absolute paths must be valid file: URLs — bare absolute paths only work by accident on POSIX (via URL path-reset semantics) and definitively break on Windows (Only URLs with a scheme in: file, data are supported... On Windows, absolute paths must be valid file:// URLs), and any path containing #, ?, spaces, or other reserved URL characters will resolve incorrectly since ESM specifiers are parsed as URLs.
🔧 Proposed fix
-import { existsSync } from 'node:fs';
+import { existsSync } from 'node:fs';
+import { pathToFileURL } from 'node:url';
...
- if (existsSync(hFile)) Object.assign(handlers, (await import(hFile)).default ?? {});
+ if (existsSync(hFile)) Object.assign(handlers, (await import(pathToFileURL(hFile).href)).default ?? {});As per coding guidelines, "For dynamic imports of absolute paths, use pathToFileURL(path).href instead of importing bare absolute paths."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Load handler maps from every handlers.js next to a manifest dir. | |
| const handlers = {}; | |
| for (const dir of manifestDirs) { | |
| const hFile = join(dir, 'handlers.js'); | |
| if (existsSync(hFile)) Object.assign(handlers, (await import(hFile)).default ?? {}); | |
| } | |
| import { existsSync } from 'node:fs'; | |
| import { pathToFileURL } from 'node:url'; | |
| // Load handler maps from every handlers.js next to a manifest dir. | |
| const handlers = {}; | |
| for (const dir of manifestDirs) { | |
| const hFile = join(dir, 'handlers.js'); | |
| if (existsSync(hFile)) Object.assign(handlers, (await import(pathToFileURL(hFile).href)).default ?? {}); | |
| } |
🤖 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 `@apps/auth0-evals/mocks/auth0` around lines 19 - 24, The dynamic import in the
handlers-loading loop uses a bare absolute path from join(), which is not a
valid ESM specifier across platforms. Update the import in the manifestDirs loop
to convert hFile to a file URL first using pathToFileURL(hFile).href before
calling import(), keeping the existing existsSync and Object.assign logic in
place.
Source: Coding guidelines
| // Load handler maps from every <surface>.handlers.js next to a manifest dir. | ||
| const handlers = {}; | ||
| for (const dir of manifestDirs) { | ||
| const hFile = join(dir, 'handlers.js'); | ||
| if (existsSync(hFile)) Object.assign(handlers, (await import(hFile)).default ?? {}); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='docs/superpowers/plans/2026-07-07-declarative-mock-runtime.md'
printf '== context ==\n'
wc -l "$file"
printf '\n== targeted lines ==\n'
sed -n '670,705p' "$file"
printf '\n== search for related import usage ==\n'
rg -n "import\(hFile\)|pathToFileURL|handlers\.js|manifestDirs" "$file" .Repository: auth0/auth0-evals
Length of output: 9490
Import handlers via a file URL. import(hFile) is using a filesystem path here; convert it with pathToFileURL(hFile).href first so the dynamic import resolves consistently across platforms.
🤖 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 `@docs/superpowers/plans/2026-07-07-declarative-mock-runtime.md` around lines
687 - 692, The dynamic import in the handler-loading logic uses a filesystem
path directly, which can resolve inconsistently across platforms. Update the
import in the manifest directory scan to use a file URL by converting the
`hFile` path with `pathToFileURL(hFile).href` before calling `import`, keeping
the rest of the `handlers` merge logic unchanged.
Source: Coding guidelines
| ``` | ||
| packages/eval-core/src/mock/ # GENERIC engine (no Auth0 knowledge) | ||
| dispatcher.ts # parse argv, normalize (configurable), route, respond | ||
| manifest.ts # load + schema-validate *.routes.json | ||
| state.ts # marker-file state under EVAL_MOCK_STATE_DIR | ||
| verbs.ts # create / set / reflect / static | ||
| types.ts # RouteManifest, HandlerContext, ... | ||
|
|
||
| apps/auth0-evals/mocks/ | ||
| auth0 # thin entrypoint: #!/usr/bin/env node → runs the engine | ||
| # with Auth0 config (binary=auth0, normalize=/api/v2) | ||
| <feature>.routes.json # app-level shared manifests (optional) | ||
| fixtures/<feature>/*.json # response bodies | ||
| <feature>.handlers.ts # computed-field handlers (optional) | ||
|
|
||
| apps/auth0-evals/src/evals/<eval>/mock/ # OPTIONAL per-eval, discovered via EVAL_MOCK_ROUTES_DIRS | ||
| <feature>.routes.json | ||
| fixtures/... , handlers.ts | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tag the architecture fence with a language.
The bare fence here triggers markdownlint MD040. Mark it as text/plaintext so the docs lint stays green. As per static analysis, fenced code blocks should have a language specified.
Suggested fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| packages/eval-core/src/mock/ # GENERIC engine (no Auth0 knowledge) | |
| dispatcher.ts # parse argv, normalize (configurable), route, respond | |
| manifest.ts # load + schema-validate *.routes.json | |
| state.ts # marker-file state under EVAL_MOCK_STATE_DIR | |
| verbs.ts # create / set / reflect / static | |
| types.ts # RouteManifest, HandlerContext, ... | |
| apps/auth0-evals/mocks/ | |
| auth0 # thin entrypoint: #!/usr/bin/env node → runs the engine | |
| # with Auth0 config (binary=auth0, normalize=/api/v2) | |
| <feature>.routes.json # app-level shared manifests (optional) | |
| fixtures/<feature>/*.json # response bodies | |
| <feature>.handlers.ts # computed-field handlers (optional) | |
| apps/auth0-evals/src/evals/<eval>/mock/ # OPTIONAL per-eval, discovered via EVAL_MOCK_ROUTES_DIRS | |
| <feature>.routes.json | |
| fixtures/... , handlers.ts | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/superpowers/specs/2026-07-07-declarative-mock-runtime-design.md` around
lines 49 - 67, The architecture layout fence in the declarative mock runtime
design doc is missing a language tag, which triggers markdownlint MD040. Update
the fenced block in the mock runtime directory layout section to use a
text/plaintext language label so the docs lint passes.
Source: Linters/SAST tools
| if (route.verb === 'handler') { | ||
| const fn = handlers[route.handler!]; | ||
| if (!fn) continue; // unknown handler → keep searching, then fallthrough | ||
| return JSON.stringify(fn({ method: (method ?? '').toLowerCase(), path, payload, state })); | ||
| } | ||
| // manifestDirs entries hold fixtures/<surface>/ alongside the manifest. | ||
| const fixturesDir = `${manifest.dir}/fixtures/${manifest.surface}`; | ||
| return JSON.stringify(applyVerb(route, state, fixturesDir)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and surrounding definitions.
git ls-files packages/eval-core/src/mock/engine.ts
wc -l packages/eval-core/src/mock/engine.ts
cat -n packages/eval-core/src/mock/engine.ts | sed -n '1,220p'Repository: auth0/auth0-evals
Length of output: 2303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first, then inspect related symbols and call sites.
ast-grep outline packages/eval-core/src/mock/engine.ts --view expanded
rg -n "runMockCli|applyVerb|HandlerFn|JSON.stringify|never throws|Promise<string>" packages/eval-core/src/mock/engine.ts packages/eval-core/src -SRepository: auth0/auth0-evals
Length of output: 2565
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the relevant region only.
sed -n '1,160p' packages/eval-core/src/mock/engine.tsRepository: auth0/auth0-evals
Length of output: 1923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the full target file with line numbers.
cat -n packages/eval-core/src/mock/engine.tsRepository: auth0/auth0-evals
Length of output: 2224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect consumers and related mock helpers for error handling and return-shape expectations.
rg -n "runMockCli\(|runMockCli\b|stdout\.write|process\.stdout\.write|console\.log|never throws|Promise<string>|JSON.stringify\(applyVerb|HandlerFn|applyVerb\(" packages/eval-core/src -S
echo '--- verbs.ts ---'
cat -n packages/eval-core/src/mock/verbs.ts
echo '--- types.ts ---'
cat -n packages/eval-core/src/mock/types.tsRepository: auth0/auth0-evals
Length of output: 3106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the CLI entrypoint that consumes runMockCli.
fd -a '.*' packages/eval-core/src | sed -n '1,80p'
echo '--- search cli files ---'
rg -n "runMockCli|stdout\.write|process\.stdout\.write|console\.log|writeFile|stderr" packages/eval-core/src -SRepository: auth0/auth0-evals
Length of output: 5677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the CLI entrypoint and the verb helper.
cat -n packages/eval-core/src/mock/verbs.ts
echo '---'
rg -n "runMockCli|stdout\.write|process\.stdout\.write|console\.log|never throws|Promise<string>" packages/eval-core/src -SRepository: auth0/auth0-evals
Length of output: 1463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any documentation of the mock engine's error/output contract.
rg -n "mock.*never throws|never throws|runMockCli|auth0 \(mock\)|Successfully logged in \(mock\)" -S .Repository: auth0/auth0-evals
Length of output: 5412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect applyVerb and any documentation for the mock engine's error contract.
cat -n packages/eval-core/src/mock/verbs.ts
echo '---'
rg -n "never throws|runMockCli|mock engine|auth0 \(mock\)|Successfully logged in \(mock\)" packages/eval-core/src packages/eval-core/README* README* -SRepository: auth0/auth0-evals
Length of output: 1564
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect resolveBody and the existing mock tests for handler behavior.
cat -n packages/eval-core/src/mock/manifest.ts
echo '--- tests ---'
cat -n packages/eval-core/tests/mock/engine.test.tsRepository: auth0/auth0-evals
Length of output: 6509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the mock CLI wrapper to understand whether engine exceptions are expected to surface.
cat -n apps/auth0-evals/mocks/auth0
echo '---'
cat -n apps/auth0-evals/scripts/mock-check.mjsRepository: auth0/auth0-evals
Length of output: 3213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect resolveBody semantics directly.
rg -n "export function resolveBody|function resolveBody|resolveBody\(" packages/eval-core/src/mock/manifest.ts packages/eval-core/src/mock -S
cat -n packages/eval-core/src/mock/manifest.tsRepository: auth0/auth0-evals
Length of output: 4028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any explicit contract around undefined values in mock responses.
rg -n "undefined|null|Promise<string>|JSON.stringify\(.*undefined|handler.*undefined|runMockCli" packages/eval-core/src packages/eval-core/tests docs -SRepository: auth0/auth0-evals
Length of output: 17344
Handle undefined mock responses before stringifying packages/eval-core/src/mock/engine.ts:32-36
If a handler or verb result is undefined, JSON.stringify(...) returns undefined, so runMockCli can resolve with a non-string and the CLI prints undefined. Coerce missing results to null or validate them explicitly.
🤖 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/eval-core/src/mock/engine.ts` around lines 29 - 38, The mock
response path in engine.ts can return a non-string when a handler or verb result
is undefined because JSON.stringify(undefined) yields undefined. Update the
logic in the route handling block around runMockCli so the handler call and
applyVerb result are checked before stringifying, and coerce missing responses
to null or throw a clear validation error instead of returning undefined. Use
the handler lookup and applyVerb branches to locate the fix.
| import { fileURLToPath } from 'node:url'; | ||
| import { loadManifests, collectRefProblems } from '../../src/mock/manifest.js'; | ||
|
|
||
| const MOCKS = fileURLToPath(new URL('../../../apps/auth0-evals/mocks/', import.meta.url)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node -e "
const { fileURLToPath } = require('url');
const base = 'file:///repo/packages/eval-core/tests/mock/harness.test.ts';
console.log(fileURLToPath(new URL('../../../apps/auth0-evals/mocks/', base)));
console.log(fileURLToPath(new URL('../../../../apps/auth0-evals/mocks/', base)));
"Repository: auth0/auth0-evals
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== harness test ==\n'
sed -n '1,120p' packages/eval-core/tests/mock/harness.test.ts
printf '\n== search loadManifests ==\n'
rg -n "function loadManifests|const loadManifests|loadManifests\\(" packages/eval-core -g '!**/dist/**' -g '!**/build/**'
printf '\n== candidate files ==\n'
fd -a "loadManifests" packages/eval-coreRepository: auth0/auth0-evals
Length of output: 2062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== manifest.ts ==\n'
sed -n '1,220p' packages/eval-core/src/mock/manifest.ts
printf '\n== mock path existence ==\n'
python3 - <<'PY'
from pathlib import Path
paths = [
Path('apps/auth0-evals/mocks'),
Path('packages/apps/auth0-evals/mocks'),
]
for p in paths:
print(f"{p}: exists={p.exists()} is_dir={p.is_dir()}")
if p.exists():
for child in sorted(p.glob('*'))[:10]:
print(' ', child)
PY
printf '\n== manifest tests ==\n'
sed -n '1,120p' packages/eval-core/tests/mock/manifest.test.tsRepository: auth0/auth0-evals
Length of output: 4754
MOCKS points at a non-existent directory, so the harness never exercises real manifests.
new URL('../../../apps/auth0-evals/mocks/', import.meta.url) resolves to /repo/packages/apps/auth0-evals/mocks/, but the manifests live under /repo/apps/auth0-evals/mocks/. Because loadManifests() skips missing directories, both checks stay vacuous.
🔧 Proposed fix
-const MOCKS = fileURLToPath(new URL('../../../apps/auth0-evals/mocks/', import.meta.url));
+const MOCKS = fileURLToPath(new URL('../../../../apps/auth0-evals/mocks/', import.meta.url));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const MOCKS = fileURLToPath(new URL('../../../apps/auth0-evals/mocks/', import.meta.url)); | |
| const MOCKS = fileURLToPath(new URL('../../../../apps/auth0-evals/mocks/', import.meta.url)); |
🤖 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/eval-core/tests/mock/harness.test.ts` at line 5, The MOCKS path in
the harness test points to the wrong directory, so loadManifests() never sees
the real manifests and the assertions remain vacuous. Update the MOCKS constant
in harness.test.ts to resolve to the actual apps/auth0-evals/mocks location,
using the existing fileURLToPath/new URL setup so the harness exercises real
manifests in the test.
…fold - mock-check: fix runMockCli signature from (config, probes) to (argv, config, handlers). Build argv per probe, loop over each, manage temp state dir with try/finally, load handlers.js if present. Output: "METHOD path -> body" per probe. - mock-new: fix reflect route scaffold from body:'example.json' to present:'example.json', absent:[] to comply with reflect verb requirements. - Both validated: manifest loads sans errors, mock-check runs without crashing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds scaffolds/cli/platform-context/AGENTS.md — a generic, feature-agnostic platform-engineer context for CLI tenant-config evals. Unlike the per-feature cli/auth0 and cli/cte scaffolds (which embed the exact guardian/actions commands the graders check, letting an agent copy-paste the answer), this ships only the generic `auth0 api <METHOD> <path>` passthrough shape — so evals test whether the agent knows the tenant flow. Rewords mocks/routes/README.md: manifests co-locate in each consuming eval's routes/ dir; the shared mocks/ dir holds only feature-neutral surfaces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@apps/auth0-evals/scripts/mock-check.mjs`:
- Line 37: The handlers import in mock-check.mjs is using a filesystem path
directly, but handlersFile is already an absolute path from fileURLToPath.
Update the import to use pathToFileURL(handlersFile).href and add pathToFileURL
from node:url so the dynamic import works reliably; keep the change localized
around the handlers assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65cea29e-04f3-44d1-a824-765cc32b2a4a
📒 Files selected for processing (4)
apps/auth0-evals/mocks/routes/README.mdapps/auth0-evals/scripts/mock-check.mjsapps/auth0-evals/scripts/mock-new.mjsapps/auth0-evals/src/evals/scaffolds/cli/platform-context/AGENTS.md
✅ Files skipped from review due to trivial changes (2)
- apps/auth0-evals/src/evals/scaffolds/cli/platform-context/AGENTS.md
- apps/auth0-evals/mocks/routes/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/auth0-evals/scripts/mock-new.mjs
| let handlers = {}; | ||
| const handlersFile = join(MOCKS, 'handlers.js'); | ||
| if (existsSync(handlersFile)) { | ||
| handlers = (await import(handlersFile)).default ?? {}; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect lines 1-28 to find the MOCKS definition
sed -n '1,28p' apps/auth0-evals/scripts/mock-check.mjsRepository: auth0/auth0-evals
Length of output: 968
Use pathToFileURL for the handlers import
MOCKS is built from fileURLToPath(...), so handlersFile is an absolute filesystem path. Import it as await import(pathToFileURL(handlersFile).href) and add pathToFileURL from node:url.
🤖 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 `@apps/auth0-evals/scripts/mock-check.mjs` at line 37, The handlers import in
mock-check.mjs is using a filesystem path directly, but handlersFile is already
an absolute path from fileURLToPath. Update the import to use
pathToFileURL(handlersFile).href and add pathToFileURL from node:url so the
dynamic import works reliably; keep the change localized around the handlers
assignment.
Source: Coding guidelines
A CLI tenant-config eval's shared AGENTS.md is platform *context* (operating-model guidance every such eval inherits), not a *scaffold* (starter code), and it should not have to be declared per-eval. Attach it by convention instead: - loadSharedContextForEval: an eval that ships a `routes/` dir is mocking a CLI (it drives EVAL_MOCK_ROUTES_DIRS), i.e. it's a CLI tenant-config eval, so it auto-inherits src/evals/contexts/cli-platform/AGENTS.md — no frontmatter field. Merged on top of the scaffold record; writeAgentGuidance injects it into the runner's context file. Wins over a scaffold AGENTS.md. - Move the shared file scaffolds/cli/platform-context/AGENTS.md → contexts/cli-platform/AGENTS.md; remove the now-empty scaffolds/cli/. - Tests: injects when routes/ present; no-op without routes/; no-op (no throw) when the shared file is absent; shared context wins over scaffold AGENTS.md. Safe by default: no routes/ dir or missing shared file → nothing injected, so every existing scaffold-based eval is unchanged. No workspace.ts/run.ts/type change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ADDING_EVALS.md: explain that an eval shipping a routes/ dir auto-inherits src/evals/contexts/cli-platform/AGENTS.md — shared platform context (guidance), distinct from a scaffold (starter code). No frontmatter field; edit one file to change the context for every CLI tenant-config eval. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…text The mfa_tenant_cli eval no longer declares a scaffold. Its routes/ dir now triggers auto-injection of the shared src/evals/contexts/cli-platform/AGENTS.md (the platform context is guidance shared across all CLI-config evals, not per-eval starter code). Removes the leaky per-feature scaffolds/cli/platform-context. Depends on the loader convention landing on plumbing (#82); on rebase, the routes/-triggered injection supplies the platform context. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…text The cte_tenant_cli eval no longer declares a scaffold. Its routes/ dir triggers auto-injection of the shared src/evals/contexts/cli-platform/AGENTS.md (platform context shared across all CLI-config evals, not per-eval starter code). Removes the leaky per-feature scaffolds/cli/platform-context. Depends on the loader convention landing on plumbing (#82). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/eval-core/src/loader.ts (1)
112-114: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winError message hardcodes
PROMPT.mdeven when a custompromptFileis used.
parsePromptMdthrows'PROMPT.md not found'regardless of the actual filename. WhenevalConfig.promptFileis set (e.g.,CUSTOM.md), this message is misleading during debugging.Proposed fix
if (!existsSync(promptPath)) { - throw new EvalConfigError('PROMPT.md not found', promptPath); + throw new EvalConfigError(`${basename(promptPath)} not found`, promptPath); }Ensure
basenameis imported fromnode:path(withnode:prefix per coding guidelines).🤖 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/eval-core/src/loader.ts` around lines 112 - 114, The error message in parsePromptMd is hardcoded to PROMPT.md, so it can be misleading when evalConfig.promptFile points to a different file. Update the missing-file branch to derive the displayed filename from the actual prompt path or configured promptFile, and keep throwing EvalConfigError with that dynamic name. Also ensure basename is imported from node:path using the node: prefix so the message can reference the correct file cleanly.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@docs/ADDING_EVALS.md`:
- Line 323: The Mock CLI stubs reference in this section points to the wrong
AGENTS.md location. Update the link so it targets the shared CLI context file
used by CLI evals, and keep the surrounding guidance aligned with the existing
routes/ and scaffold notes in ADDING_EVALS.md so readers are directed to the
correct source.
In `@packages/eval-core/tests/loader.test.ts`:
- Around line 324-383: Add a regression test in loadEval to cover a non-default
evalConfig.promptFile, since loader.ts now supports a custom prompt filename
path. Extend the existing loader.test.ts coverage by creating an eval that uses
a prompt file name other than the default and assert loadEval reads that file
correctly, using the loadEval helper and EVAL_CONFIG setup to verify the custom
promptFile behavior.
---
Outside diff comments:
In `@packages/eval-core/src/loader.ts`:
- Around line 112-114: The error message in parsePromptMd is hardcoded to
PROMPT.md, so it can be misleading when evalConfig.promptFile points to a
different file. Update the missing-file branch to derive the displayed filename
from the actual prompt path or configured promptFile, and keep throwing
EvalConfigError with that dynamic name. Also ensure basename is imported from
node:path using the node: prefix so the message can reference the correct file
cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 212f726b-d44b-452f-b5ad-1d85d7fe993e
📒 Files selected for processing (4)
apps/auth0-evals/src/evals/contexts/cli-platform/AGENTS.mddocs/ADDING_EVALS.mdpackages/eval-core/src/loader.tspackages/eval-core/tests/loader.test.ts
💤 Files with no reviewable changes (1)
- apps/auth0-evals/src/evals/contexts/cli-platform/AGENTS.md
|
|
||
| > **An eval that ships a `routes/` dir automatically inherits `src/evals/contexts/cli-platform/AGENTS.md`.** | ||
|
|
||
| A `routes/` dir means the eval mocks the Auth0 CLI (see [Mock CLI stubs](../AGENTS.md)), i.e. it's a CLI tenant-config eval — so it gets the shared "you are a platform engineer; use the Auth0 CLI; it's already authenticated" context injected into the agent's native context file. No frontmatter field is needed (and CLI evals declare no `scaffold`). The shared file is generic — it names no feature-specific commands, so it never leaks a grader's expected answer. Edit that one file to change the context for every CLI eval at once. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the root AGENTS.md exists (the link target from docs/ADDING_EVALS.md)
ls -la AGENTS.md 2>/dev/null && echo "EXISTS" || echo "NOT FOUND"
# Also check if there's a mock-related AGENTS.md elsewhere
fd -t f "AGENTS.md" --max-depth 3 | head -20Repository: auth0/auth0-evals
Length of output: 232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file map =="
fd -H -t f 'AGENTS.md' .
echo
echo "== contexts/cli-platform =="
fd -H -t f '.' contexts/cli-platform 2>/dev/null || true
echo
echo "== docs/ADDING_EVALS.md around cited lines =="
sed -n '315,330p' docs/ADDING_EVALS.md
echo
echo "== root AGENTS.md excerpt =="
grep -n -A4 -B4 'Mock CLI stubs\|cli-platform\|Auth0 CLI' AGENTS.md || true
echo
echo "== search for mock cli stubs references =="
rg -n 'Mock CLI stubs|cli-platform|contexts/cli-platform|AGENTS.md' -S docs AGENTS.md contexts -g '!**/.git/**' || trueRepository: auth0/auth0-evals
Length of output: 7109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/ADDING_EVALS.md (around link) =="
sed -n '318,326p' docs/ADDING_EVALS.md
echo
echo "== root AGENTS.md around any CLI/context guidance =="
rg -n -A3 -B3 'AGENTS\.md|cli-platform|Mock CLI stubs|injected agent guidance|routes/ dir' AGENTS.md
echo
echo "== moved shared context file =="
sed -n '1,80p' apps/auth0-evals/src/evals/contexts/cli-platform/AGENTS.mdRepository: auth0/auth0-evals
Length of output: 6309
Update the Mock CLI stubs link
../AGENTS.md points to the repo-root AGENTS.md; the shared CLI context now lives at apps/auth0-evals/src/evals/contexts/cli-platform/AGENTS.md. Point the link there or it will keep sending readers to the wrong file.
🤖 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 `@docs/ADDING_EVALS.md` at line 323, The Mock CLI stubs reference in this
section points to the wrong AGENTS.md location. Update the link so it targets
the shared CLI context file used by CLI evals, and keep the surrounding guidance
aligned with the existing routes/ and scaffold notes in ADDING_EVALS.md so
readers are directed to the correct source.
| // ── shared CLI platform-context auto-injection tests ───────────────────────── | ||
| // Convention: an eval that ships a `routes/` dir is a CLI tenant-config eval and | ||
| // auto-inherits the shared src/evals/contexts/cli-platform/AGENTS.md — no | ||
| // frontmatter field. See loadSharedContextForEval in loader.ts. | ||
|
|
||
| const SHARED_CLI_CONTEXT_REL = 'src/evals/contexts/cli-platform'; | ||
|
|
||
| function makeSharedCliContext(base: string, content = 'You are a platform engineer.\n'): void { | ||
| makeScaffoldVariant(base, SHARED_CLI_CONTEXT_REL, { 'AGENTS.md': content }); | ||
| } | ||
|
|
||
| function addRoutesDir(evalDir: string): void { | ||
| const routes = join(evalDir, 'routes'); | ||
| mkdirSync(routes, { recursive: true }); | ||
| writeFileSync(join(routes, 'guardian.routes.json'), '{"surface":"guardian","routes":[]}'); | ||
| } | ||
|
|
||
| describe('loadEval - shared CLI platform context', () => { | ||
| it('auto-injects the shared context AGENTS.md when the eval ships a routes/ dir', async () => { | ||
| makeSharedCliContext(tmpBase); | ||
| const evalDir = makeEvalDir(tmpBase); | ||
| addRoutesDir(evalDir); | ||
|
|
||
| const result = await loadEval(EVAL_CONFIG, tmpBase); | ||
|
|
||
| expect(result.scaffold['AGENTS.md']).toContain('You are a platform engineer'); | ||
| }); | ||
|
|
||
| it('does not inject context when the eval has no routes/ dir', async () => { | ||
| makeSharedCliContext(tmpBase); | ||
| makeEvalDir(tmpBase); // no routes/ | ||
|
|
||
| const result = await loadEval(EVAL_CONFIG, tmpBase); | ||
|
|
||
| expect(result.scaffold['AGENTS.md']).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('is a no-op (no throw) when routes/ exists but the shared context file is absent', async () => { | ||
| const evalDir = makeEvalDir(tmpBase); | ||
| addRoutesDir(evalDir); // routes/ present, but no contexts/cli-platform | ||
|
|
||
| const result = await loadEval(EVAL_CONFIG, tmpBase); | ||
|
|
||
| expect(result.scaffold['AGENTS.md']).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('shared context AGENTS.md wins over a scaffold AGENTS.md', async () => { | ||
| makeSharedCliContext(tmpBase, 'Shared platform context.\n'); | ||
| // local scaffold/ also ships an AGENTS.md | ||
| const evalDir = makeEvalDir(tmpBase, MINIMAL_PROMPT, DEFAULT_GRADERS, { | ||
| 'AGENTS.md': 'Local scaffold guidance.\n', | ||
| }); | ||
| addRoutesDir(evalDir); | ||
|
|
||
| const result = await loadEval(EVAL_CONFIG, tmpBase); | ||
|
|
||
| expect(result.scaffold['AGENTS.md']).toContain('Shared platform context'); | ||
| expect(result.scaffold['AGENTS.md']).not.toContain('Local scaffold'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for tests referencing promptFile in the test file
rg -n 'promptFile' packages/eval-core/tests/loader.test.tsRepository: auth0/auth0-evals
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## promptFile occurrences in repo\n'
rg -n --hidden --glob '!**/node_modules/**' 'promptFile' packages/eval-core || true
printf '\n## loader.ts outline\n'
ast-grep outline packages/eval-core/src/loader.ts --view expanded || true
printf '\n## test files mentioning loadEval or loader\n'
rg -n --hidden --glob 'packages/eval-core/tests/**/*.ts' 'loadEval|loader|promptFile' packages/eval-core/tests || trueRepository: auth0/auth0-evals
Length of output: 6161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## promptFile occurrences across eval-core\n'
rg -n --hidden --glob '!**/node_modules/**' 'promptFile' packages/eval-core || true
printf '\n## loader.ts around promptFile\n'
line=$(rg -n 'promptFile' packages/eval-core/src/loader.ts | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$(( line > 25 ? line - 25 : 1 ))
end=$(( line + 60 ))
sed -n "${start},${end}p" packages/eval-core/src/loader.ts | cat -n
fi
printf '\n## tests in packages/eval-core\n'
rg -n --hidden --glob 'packages/eval-core/tests/**/*.ts' 'promptFile|loadEval|loadSharedContextForEval|AGENTS.md' packages/eval-core/tests || trueRepository: auth0/auth0-evals
Length of output: 11330
Add a regression test for promptFile
packages/eval-core/src/loader.ts:49-53 adds a custom prompt filename path; packages/eval-core/tests/loader.test.ts should cover a non-default evalConfig.promptFile.
🤖 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/eval-core/tests/loader.test.ts` around lines 324 - 383, Add a
regression test in loadEval to cover a non-default evalConfig.promptFile, since
loader.ts now supports a custom prompt filename path. Extend the existing
loader.test.ts coverage by creating an eval that uses a prompt file name other
than the default and assert loadEval reads that file correctly, using the
loadEval helper and EVAL_CONFIG setup to verify the custom promptFile behavior.
Source: Coding guidelines
1 participant
What this adds
Shared framework infrastructure that lets evals verify tenant configuration hermetically. This PR contains no eval content of its own — it is the base that the MFA Leg 2 and Custom Token Exchange enterprise-leg PRs stack on top of.
Components
mocks/auth0) — a version-controlled no-op stub prepended to the agent'sPATH(viaEVAL_MOCK_BIN_DIR) so tenant commands succeed deterministically — no auth, no network, no live side effects.packages/eval-core/src/mock/) applies verbs (static/create/reflect/handler) from*.routes.jsonmanifests; feature content is data, not code.routes/dir (<surface>.routes.json+fixtures/+handlers.js) next to itsPROMPT.md;run.tssetsEVAL_MOCK_ROUTES_DIRSand the dispatcher merges it with the sharedmocks/dir. So a surface lives with the eval that uses it, not centrally.EVAL_MOCK_STATE_DIR(a per-run temp dir outside the workspace, so graders never see it): aput guardian/factors/otpis reflected by a laterget guardian/factors.guardian/factors/otp), leading-slash (/api/v2/...), and full-URL (https://<tenant>/api/v2/...) forms to one route, so the agent's command works whichever form it emits and never misreads a valid call as a no-op.{"ok":true}; unmapped reads fall through to{}.src/evals/contexts/cli-platform/AGENTS.md) — feature-agnostic platform-engineer guidance with only theauth0 api <METHOD> <path>passthrough shape. Auto-attached by convention: any eval that ships aroutes/dir (i.e. mocks the CLI → is a CLI tenant-config eval) inherits it vialoadSharedContextForEvalin the loader — noscaffold:/context:frontmatter. It's context (guidance), not a scaffold (starter code), and being generic it never leaks a grader's expected commands. One file changes the context for every CLI eval.judge()gains an opt-in{ includeCommandTrace }. When set, the agent's successful shell commands are appended to the judge input (under a// COMMAND TRACEheader), so a file-less CLI eval has a judgeable artifact. Off by default — every file-based judge is unchanged.filteredEnv()forwardsEVAL_MOCK_BIN_DIR/EVAL_MOCK_STATE_DIR/EVAL_MOCK_ROUTES_DIRS;run.tscreates a per-run state dir locally and sets per-eval routes;docker/Dockerfile+entrypoint.shwire the sandbox.Testing
npm run build,npm run lint,npm testall pass. New/updated tests:auth0-mock, judge trace (executors),primitives,env,discovery,loader.Stacked PRs
Two feature PRs build on this branch — both CLI-only tenant-config evals with co-located mock routes:
mfa_tenant_clicte_tenant_cli🤖 Generated with Claude Code