Skip to content

feat: give the host→container boundary a contract and fold aggregate into report --rebuild - #178

Open
uipreliga wants to merge 23 commits into
mainfrom
feat/container-contract-and-command-surface
Open

uipreliga wants to merge 23 commits into
mainfrom
feat/container-contract-and-command-surface

Conversation

@uipreliga

@uipreliga uipreliga commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Breaking changes

  • A skewed or unlabeled driver: docker image is refused before the container starts. The host reads the image's org.coder-eval.version label. If the label is missing, or differs from the installed coder-eval, the run fails. This applies to dockerfile_path images too. Set ALLOW_IMAGE_SKEW=1 (not an empty string) to run a deliberately different image; a missing label is never excused.
  • An image older than the host→container contract is refused. The container must echo the contract it parsed. A result with no echo, or a different one, is refused and moved to task.json.unhonored, and a synthetic ERROR task.json takes its place. This replaces task.json.graded and task.json.rerun.
  • coder-eval aggregate is removed. Use coder-eval report <run_dir> --rebuild.
  • aggregate -o/--output <dir> has no replacement. --rebuild writes in place; gather task directories into the target directory first.

Downstream: UiPath/skills#3318 sets ALLOW_IMAGE_SKEW on the one workflow that tests an ad-hoc image tag against the pinned wheel. It is safe to merge before this release.

Summary

  • ContainerContext contract. context.json is a strict Pydantic model: every field is required, unknown keys are refused, and grade, regrade and replicate_index are strictly typed. A host/image mismatch fails with exit 2 and names the field, instead of falling back to a default.
  • Contract echo. The container writes the contract it parsed into environment_info["container_contract"], after the regrade seed so a prior row cannot overwrite it. One host check, _assert_contract_echoed, replaces _assert_grade_honored, _assert_regrade_honored and _warn_on_version_mismatch.
  • Driver rewrite moved host-side. The host stages the task with driver: tempdir and sends the authored sandbox in the contract, so task.json still records driver: docker and the container needs no CE051 exemption.
  • report --rebuild. A printer-free orchestration/run_summary_rebuild.py backs it. It refuses --format, --output, a task directory, a directory inside another run, and a symlinked run.json/run.md.
  • evaluate refreshes run.json. coder-eval evaluate <task_run_dir> rebuilds the owning run's run.json itself. It is best-effort (never changes the exit code), accepts only a coder-eval run summary on the walk up, and skips when --run-dir sits inside the run. write_run_summary now writes atomically.
  • run/execute option sharing: dropped. A factory module was built and gave byte-identical --help, but review found it saved no lines, so cli/ is unchanged. A test now pins every shared flag as identical on both commands.

Test plan

  • make verify: 5815 passed, 2 skipped, coverage 92.70%, pyright clean, custom lint rules pass
  • Every phase had a spec-compliance and a quality review; a final whole-change review found and fixed three Medium issues (stale echo in prior.json, a refused row vanishing from run.json, the refused record lost on the grading path)
  • run --help and execute --help byte-identical to the pre-change baseline
  • Run a real driver: docker task against a freshly built make docker-image image (0.12.1, label matches the host). tasks/agents/claude_hello_world_docker.yaml with --type codex --model gpt-5.4: SUCCESS, 1.0, 3/3. The echo holds all 10 contract keys with grade: true, regrade: false, and task.json records driver: docker.
  • Detached grade of that row: coder-eval evaluate <row> --allow-recorded-commands graded in a container of the task's image, SUCCESS → SUCCESS, 3/3, exit 0. The echo shows regrade: true, started_at is unchanged (the agent did not run again), task.execute.json was kept, run.json was refreshed, and no graded_on_host stamp was set.

Seen during that check, not caused by this PR:

  • With --type codex and no --model, the task's default claude-sonnet-4-6 went to the Azure Codex endpoint. The turn ended with 0 messages, 0 tokens and error: null, and the row was recorded as FAILURE. The Codex adapter should record this as an error.
  • Without --run-dir, evaluate writes grade.log and grade.docker.log to its own new runs/<timestamp>/, not beside the graded row.

Stacking

This PR is based on #177 (docs/slim-source-prose). Merge #177 first. If its branch is deleted on merge, GitHub moves this PR to main; if not, change the base to main by hand.

🤖 Generated with Claude Code

uipreliga and others added 23 commits September 14, 2026 17:34
Adds `tests/lint/prose_budget.py`: a stdlib-only measurement of the
essay-shaped prose in `src/coder_eval` — docstrings over 150 words (Typer
commands exempt) plus comment runs of 3+ lines — gated against a module
baseline of 79,754 words. It also resolves every `Rationale: <path> §
<heading>` pointer, and under `--assert-code-unchanged <ref>` proves a
commit moved prose only, by comparing the docstring-stripped AST and the
multiset of functional directive comments.

The gate is wired in both seams: `make docs-budget` / `make verify`, and
the `quality-gate` job in pr-checks.yml — CI restates every step and never
invokes `make`, so the Makefile line alone would gate nothing on a PR.

Relocates the 12 sections of `.claude/architecture-notes.md` verbatim into
`.claude/notes/<subsystem>.md` and deletes it, so rationale has exactly one
home and the tree has exactly one index.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JcdMjPKFc2wdg4J6Ezg4E2
Cuts the four densest files in the tree from 6,874 essay words to 533,
leaving the caller-facing contract and the couplings a future editor
would break, and moving the design rationale to
`.claude/notes/{timing,permissions,persistence}.md`.

Kept in the source: the `timing.ts` parity claim and its shared fixture,
`TurnClock`'s not-for-deadlines hazard, the raw-total ordering hazard and
its division guard, the stacked-window contract, and every CE reference
whose text stayed. Deleted: history git already holds, and `TurnClock`'s
per-harness roster, which `docs/agents/HARNESS_PARITY.md` owns.

Also corrects a stale relocated claim: the notes said the task directory
was not shielded, but `Orchestrator._communicate_with_retry` chmods it
alongside the reference.

No executable statement changed — proved per file by
`prose_budget --assert-code-unchanged`, which compares the
docstring-stripped AST and the functional directive comments.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JcdMjPKFc2wdg4J6Ezg4E2
Cuts the five harness adapters and their eight helpers from 17,227 essay
words to 4,456, and gives `.claude/notes/agents.md` the shared story the
adapters kept telling five times over: the turn lifecycle, first-window
seeding, per-harness generation marks, token accounting and cost
resolution, why a clean exit can still be a crash, and how each CLI is
reaped.

Every per-harness parity claim either stays in the adapter or is already
a row in docs/agents/HARNESS_PARITY.md, which the taxonomy makes the SSOT
for cross-harness facts. Both SDK#24168 FIXMEs stay FIXMEs.

No executable statement changed — proved per file by
`prose_budget --assert-code-unchanged`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JcdMjPKFc2wdg4J6Ezg4E2
Cuts orchestrator.py and orchestration/ from 17,724 essay words to
3,980, extending the four sections Phase 1 relocated into
orchestration.md rather than opening rival headings for the same topics:
the terminal-status chain and the four grading sites under
execute-vs-run, the fired-only gate and the ceiling/floor bounds under
early stop.

New sections cover what the orchestrator alone owns — recording the task
as authored, the three separately-resolved routes, interrupt-proof
teardown, restoring a PATH from an untrusted run directory, the dialog
loop, embedded commands and experiment resolution. regrade's container
rationale went to isolation.md, where detached grading already lives.

`prose_budget` now resolves a pointer against `##` or `###`, so appending
to an existing section does not force the pointer up to its parent.

No executable statement changed — proved by
`prose_budget --assert-code-unchanged`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
13,289 words of essay-shaped prose across 11 files down to 2,960, with no
code change (AST + directive multiset verified against the phase-start SHA).

docker_runner.py 4,993 -> ~1,100 and sandbox.py 3,259 -> ~590 are the bulk.
The repo's largest comment block, 34 lines on --cap-drop, is now the four
COUNTERPART lines that state the coupling plus a pointer; the capability
argument it carried is rationale and moved.

isolation.md gains Capability drops and the anti-cheat window, What crosses
into the container, Trusting what the container sends back, and The sandbox
the criteria run in. Detached grading from the CLI joins the existing
detached-grading section rather than rivalling it, and the in-container
driver rewrite stays in orchestration.md, which already owned it.

The Phase-1 relocated bullet covered nine of these topics already, so the
single-home rule needed excision as well as writing: seven spans moved out
of it, each left as a cross-reference to the section that now owns them.

The --cap-drop/FOWNER known gap stays in docs/DOCKER_ISOLATION.md; the
notes link it and say so.

All eight --help outputs are byte-identical.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…/notes

14,406 words across 34 files down to about 3,300, with no code change and no
generated surface touched.

Two constraints made this phase different from the others, and both were
verified by parsing rather than by trusting a downstream check: the first
non-blank line of every criterion class docstring, which CE033 copies into the
plugin reference, is byte-identical; and no Field(description=...) string under
models/ changed, which is what CE030 ripples into the task guide.

The LiveVerdict determinism and monotonicity properties stay at the definition
site in criteria/base.py, because they are the contract an author has to
satisfy. What moved is the derivation: why CE036 replays trajectories, and what
CE025 cannot see.

contracts.md gains the live_verdict contract, the checker base class, route
resolution, judge context and untrusted text, sub-agent judging, and recording
a CLI invocation. persistence.md gains judge persistence; agents.md the
sdk_options pass-through; orchestration.md the rates, the early-stop guardrail
placement, and the armed gate.

Docstrings that restated a Field description, or the task guide's own YAML
examples, were deleted rather than moved — the guide is the source of truth.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…de/notes

10,234 words across 30 files down to about 2,900, finishing the plan. The tree
goes from 79,754 essay-shaped words to 18,163, and from 126 docstrings over 150
words to one.

That one is agent.py::communicate, the plugin SPI contract every third-party
agent author reads: the Args/Returns/Raises, the rule that a mid-turn failure
sets pending_turn before raising and the caller rolls the counter back, and the
streaming event protocol. It is the exemption the plan predicted, and the only
one taken, so the 150-word bar held for the whole tree.

reporting.md gains the Agent ABC contract, telemetry emission, cost joining,
report rollups and the HTML twin, Harbor export, and the ATIF trajectory
bridge.

Two hazards the plan named had to be ADDED rather than kept: neither the
pricing.py/pricing.ts mirror nor the "evalboard's static twin" parity claim was
ever stated in its own source file — both lived only in CLAUDE.md. Editing one
side of either pair without the other is exactly what they exist to prevent, so
they now say so where the editor will be.

errors/categories.py is untouched: its per-member retryability notes are the
contract errors/categorization.py dispatches on, and they are one-line comments
the plan puts out of scope.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
check_pointers proves a pointer RESOLVES. It cannot see the defect that actually
shipped: a block replacement anchored on the wrong line leaves the tail of the
replaced prose stranded after the pointer, where the $-anchored regex stops
looking. The file parses, the pointer resolves, and the comment carries a severed
half-sentence.

Three phases of the prose-reduction work shipped that shape to review before this
check existed; a fourth ran clean because the check was running from the first
edit. It also reports an orphaned docstring terminator, which is worse than it
looks: the file stops parsing, and the measurement silently reports it as zero
words rather than failing.

A docstring may still follow its pointer with Args:/Returns:/Raises: — that is the
house shape, and the one case this must not flag.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Self-contained (inline CSS/JS, no external fonts or images)" is caller-facing —
it is why the file can be opened offline and uploaded as a CI artifact — and the
7/7 rewrite dropped it while adding the static-twin claim. Both belong.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fifth was promoted (b8b1dcf). These four are deferred with the reason, so
the next person does not rediscover them: the general severed-fragment shape
needs an allowlist to be usable, the single-home rule needs a similarity
measure, a pointer landing unhelpfully is probably not mechanizable at all, and
the generated-surface guard needs a commit-scoped diff the lint harness cannot
see.

The single-home one is the highest-value unbuilt guard in the notes design —
it bit every phase of this run, once against a file the phase never opened.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An audit of the finished refactor found nine places where compression either
dropped a contract or, worse, replaced it with a plausible wrong reason.

The bad one is `_resolve_backend_route`. Its docstring said the function raises
"so it must survive -O" — true of a different function, transplanted here —
while the actual contract was gone from the source AND from the notes: an
explicit backend override that cannot be honored must fail loudly, not degrade
to a backend the task author never asked for. A reader told the raise is only
about -O can reasonably "improve" it into a fallback, which is exactly the
regression the deleted sentence forbade.

The rest:

- write_text_atomic claimed mode 0o644 as if guaranteed. It is a ceiling; the
  umask narrows it, and 0o077 yields 0600 — which breaks the docker driver, the
  failure the directive exists to prevent.
- A NOTE in sandbox.py said three methods do not validate path traversal. Two
  of them go through resolve_files (and so through _within_sandbox and
  _reject_escaped); only list_files still does not. Someone reasoning about
  criterion containment from that comment reached the wrong conclusion.
- enforces_permission_windows kept the clause making the window SAFE and lost
  the one making it REAL: without the container's DAC cap drops a mode-000
  directory is still readable by root.
- capture_to's copytree flags lost the reason one of them is load-bearing.
- adopt no longer said criteria can still write to the adopted tree, under a
  docstring headlined "materializing nothing".
- The docker log constants lost why there are two of them, and had no pointer.
- armed_criteria_passed said "weighted score", which names a different quantity
  in the same class; the binarisation that makes gate_threshold=1.0 an exact
  equivalence was gone.
- judge_context said "these tokens" with no list after it.
- _build_run_command_env counted eight env facets and sets nine.

The baseline rises 18,163 -> 18,549. That is the ratchet working as intended:
restoring real contract is a legitimate reason to raise it, and the reason
belongs here rather than in a silent constant bump.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review B found one shape repeated six times: compression kept a categorical
claim and dropped the words that made it true. Each is the only fact its
comment now carries, sitting above the code someone would edit.

- claude_code_agent: the task-notification guard is checked SECOND, not first.
  What matters is that it precedes _is_sdk_result_message.
- opencode_agent: "the condition is the TELEMETRY, not the event vocabulary" —
  but one of the two arms IS the vocabulary, and a later comment says so. The
  dropped word was "alone". Deleting the arm as redundant re-opens scoring
  SUCCESS 1.0 on zero turns.
- batch: the predicate is NOT_GRADED **or** executed. "Executed is a required
  half" reads as `and`, which would leave zero-iteration execute rows ungraded
  forever.
- reference_comparison: "every failure below raises" — three branches below
  return a gating 0.0 on purpose, one of them carrying a CE039 noqa.
- docker_runner: the stdout limit does not mirror _POST_RUN_STREAM_LIMIT; it is
  256x larger, deliberately. Unifying them downward reinstates the mid-stream
  teardown that lost whole paid tasks.
- cli_called: "harness fault, not agent behaviour" contradicted the block 50
  lines down explaining that all five paths are agent-reachable.

Also restores the judge ignore floor's MECHANISM: it is a copy-time control,
the same list passed as copytree's `ignore`, not only an SDK setting. Without
that, dropping .claude / .mcp.json from it looks redundant and lets an
agent-planted settings file into the judge's own working directory.

_resolve_backend_route's contract now has a home in the notes, and the
docstring points at it rather than at the LiteLLM section.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`--tmpfs` is never emitted. `_build_argv` does not construct one,
`test_reference_inside_task_dir_needs_no_tmpfs_mask` asserts `not _tmpfs(argv)`,
and docker_runner.py's own `_reference_mount_args` docstring opens "No tmpfs
mask any more". Three of its neighbours said the opposite, two of them lines
this refactor wrote.

The claim predates the branch, so this is not a regression — but the refactor
read every one of those lines and compressed them instead of questioning them,
and it authored two new canonical homes for the falsehood. Re-ratifying it is
worse than never having looked.

What is actually true: the task dir is a shielded read-write COPY, so a
reference embedded in it is covered by the same mode-000 window rather than
hidden under a layered filesystem. `orchestration/evaluation.py`'s hard-fail
was justified by the mask and by an EROFS on a `:ro` bind — both false now, and
the real reason (the fallback resolves to a tree the window was not opened
over) is the one that survives.

Also restructures `permissions.md`'s Phase-1 blob. Its single 4,450-char line
duplicated two of its own sibling sections, restated the FOWNER/CHOWN rationale
that isolation.md explicitly delegates to docs/ saying "they are not restated
here", and carried the tmpfs claim. All four inbound source pointers still
resolve.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_ESSAY_BASELINE_WORDS was the weakest part of the gate. It was a single
tree-wide number, hand-edited eleven times in this work alone, that a reviewer
had to take on trust; it let growth in one file hide behind shrinkage in
another; and it said nothing at all about a file that did not exist yet.

It is gone. Two rules replace it, and neither has a number anyone maintains:

  own-line comments per file <= MAX(20, 0.15 * file length)
  no docstring over 150 words of PROSE

The comment budget is proportional, so deleting code takes its budget with it
and a new file is governed from its first commit. Own-line only: a trailing
`# noqa` is a directive and a per-member annotation on an enum is the contract
a dispatcher reads — counting either would push against documenting them. The
floor is what protects a constants module at one comment per constant, which is
where the tree's natural maximum sits.

The essay rule now counts prose, not structure: an Args/Returns/Raises block is
interface documentation, and counting it pushed exactly the docstrings that
document their contract best over the line. Exemptions are categorical rather
than numeric — an @AbstractMethod docstring IS the contract implementers read,
so the plugin SPI is covered by kind, and a new abstract method is covered
automatically. The old "at most two, by fiat" allowance is unnecessary: the
tree now has ZERO essays.

Getting there took 44 comment lines out of 7 files. Most came out by reflowing
two lines into one or cutting a section divider; the one real compression was a
22-line block in early_stop.py restating the floor bound that
orchestration.md already owns, and the distractor-exclusion rule it uniquely
held moved there rather than being dropped.

One reflow silently merged `# pyright: reportImportCycles=false` into the prose
line above it, which would have stopped pyright honouring it.
--assert-code-unchanged caught it. That is the second time the directive
multiset has earned its place.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nd clarifications

Co-Authored-By: Claude <noreply@anthropic.com>
Replace the json.loads + dict.get + isinstance ladder at the host→container
boundary with a Pydantic model: every field required, extra="forbid",
StrictBool for grade/regrade, StrictInt for replicate_index. The host builds
the model in _stage_inputs and keeps it on DockerRunner._staged_context.

BREAKING CHANGE: a container image and host that disagree about the
context.json keys now fail at parse time (exit 2, naming the field) instead
of falling back to defaults (grade=True, host_task_file=None, the staged
source_yaml).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oed its contract

The image version preflight now refuses instead of warning, for dockerfile_path
images too, and is the single reader of the org.coder-eval.version label. The
container echoes the ContainerContext it parsed into
environment_info["container_contract"], written after the regrade seed so the
prior row cannot overwrite it. One host guard, _assert_contract_echoed,
replaces _assert_grade_honored, _assert_regrade_honored and
_warn_on_version_mismatch, and quarantines a refused record to
task.json.unhonored. A host grade drops a prior row's stale echo, and the echo
is kept out of the rendered Environment table.

BREAKING CHANGE: a driver: docker run is refused before the container starts
when the image's org.coder-eval.version label is missing or differs from the
host's installed coder-eval. Set ALLOW_IMAGE_SKEW=1 (not an empty string) to
run a deliberately different image; a missing label is never excused. An image
that predates the host→container contract is refused after the run, and its
record is moved to task.json.unhonored instead of task.json.graded /
task.json.rerun.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t-side

DockerRunner._stage_inputs now stages task.yaml as the execution copy with
driver: tempdir and sends the authored sandbox block in the contract as
ContainerContext.authored_sandbox. The in-container entry point no longer
rewrites the driver; it records authored_task from ctx.authored_sandbox, so a
docker run's task.json still says driver: docker. The CE051 exemption moves
with the rewrite: the two sites are now host staging and opt-in host grading.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Extract the run-summary rebuild into a printer-free
orchestration/run_summary_rebuild.py (rebuild_run_summary, find_run_root) and
expose it as `coder-eval report <run_dir> --rebuild`, which prints the same
counts line and note. --rebuild refuses --format, --output, a task directory
and a directory inside another run. The rebuilt run id now comes from the
resolved directory, so `runs/latest --rebuild` records the real run id.

BREAKING CHANGE: `coder-eval aggregate` is removed; use
`coder-eval report <run_dir> --rebuild`. Its `-o/--output <dir>` option has no
replacement: --rebuild writes in place, so gather task directories into the
target directory first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… grade

`coder-eval evaluate <task_run_dir>` now rebuilds the owning run's run.json
and run.md itself after writing the verdict back, instead of telling the user
to run a second command. The refresh is best-effort: any failure only warns,
and the exit code is always the verdict's.

find_run_root accepts only a run.json that is a coder-eval run summary (a JSON
object with run_id and task_results), so a row copied into another tool's tree
never overwrites that tool's run.json. rebuild_run_summary refuses to write
through a symlinked run.json or run.md; both evaluate and report --rebuild
inherit that, since it is their one shared write path. The refresh is skipped
when the grading --run-dir sits inside the owning run, where its own task.json
would count as a second row. write_run_summary now writes atomically, and the
new console lines escape Rich markup in paths.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mmand exemption list

Two review findings from the container-contract work, closed as tests:
- the host compares the container's echo with its own dump, so a validator
  under authored_sandbox that is not idempotent across a JSON round trip would
  refuse every docker run; the round trip now runs over a maximal SandboxConfig;
- prose_budget's _TYPER_COMMANDS kept a dead entry for the deleted aggregate
  command; it must now equal the commands the CLI registers.

Two more candidates that need type information or are conventions are recorded
in .claude/harness-candidates.md.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cally

Phase 6 planned to declare the flags `run` and `execute` share once, through
option factories. The factory module was built and produced byte-identical
--help output, but review found it saved no lines and turned each new shared
flag into a five-place edit, so it was dropped and cli/ is unchanged.

What remains is the drift guard: a test that compares every click attribute a
shared flag can show or validate between the two commands, and asserts that
--resume and --format stay command-specific.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Strip the prior row's container_contract from the staged prior.json. An image
  that honors regrade but predates the echo keeps the prior environment_info,
  so a matching echo from an earlier identical dispatch passed as its own.
- A result refused by the contract echo now leaves a synthetic ERROR task.json
  in place of the quarantined record, like every other container failure, so
  the row no longer vanishes from the next run.json rebuild.
- On a detached grade, the refused task.json.unhonored is folded back from the
  scratch directory beside the graded row, and the error no longer suggests
  --allow-host-grading when the image itself was refused.
- Docstrings, the stale "version checks only warn" wording, the dead None branch
  in _quarantine_record, the run.json environment note, and a cwd-independent
  task path for the evaluate refresh tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown

Claude finished @uipreliga's task in 1m 37s —— View job


Code Review

Todo List

  • Read .github/code_review.md for review guidelines
  • Read CLAUDE.md for project conventions
  • Review the full diff (git diff origin/main...HEAD)
  • Analyze key architectural changes
  • Check cross-file consistency
  • Identify what's missing
  • Provide structured feedback

Starting code review...

@uipreliga
uipreliga changed the base branch from main to docs/slim-source-prose September 15, 2026 15:05
@akshaylive

Copy link
Copy Markdown
Collaborator

One thin I'm unsure about it it's behavior on custom docker images. Skills repo uses a custom docker image based on the coder-eval docker image. In this scenario, do we force consumers to set the env var? If not this PR LGTM.

@uipreliga
uipreliga force-pushed the docs/slim-source-prose branch from af5e07c to 4941681 Compare September 16, 2026 02:39
Base automatically changed from docs/slim-source-prose to main September 16, 2026 02:46
@uipreliga

Copy link
Copy Markdown
Collaborator Author

Good question — no, consumers don't need to set anything.

A custom image built FROM coder-eval-agent:<ver> inherits the org.coder-eval.version label from the base (Docker propagates Config.Labels), so the preflight passes with no LABEL line of its own. The skills image is exactly this shape: tests/docker/Dockerfile is FROM ${CODER_EVAL_IMAGE}, which run-coder-eval.yml sets to ghcr.io/uipath/coder-eval-agent:${CE_IMAGE_TAG}, and that tag defaults to the pinned wheel version from tests/.coder-eval-version. Image and host agree, so the check is silent. The overlay adds only npm/dotnet/SDK layers; it never reinstalls coder-eval. The one skills path where the two differ on purpose is a coder_eval_image_tag override, and UiPath/skills#3318 sets the escape hatch there.

A consumer only hits the refusal when their base is pinned to an older coder-eval than the host wheel (or a stale latest) — which is the skew the check exists for. The fix is to rebuild the derived image against a matching base, not the escape hatch; the error text names both the rebuild command and the variable.

Two things ALLOW_IMAGE_SKEW deliberately does not cover:

  • A missing label. An image that doesn't descend from the framework image — for example one built with the runtime kit — must apply LABEL org.coder-eval.version itself.
  • The contract echo. An overlay that reinstalls coder-eval keeps the inherited label but runs different code, so the result is refused.

One nit for anyone wiring it up: the value must be 1 or 0, never an empty string — ALLOW_IMAGE_SKEW= fails the boolean parse at CLI startup.

All of this is in docs/DOCKER_ISOLATION.md § Using a pre-built custom image, plus the Troubleshooting custom images table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants