fix(harbor): resolve workdir dynamically at run time instead of guessing at export

[akshaylive]

Summary

• The Harbor exporter (coder-eval export --format harbor) used to shell out to docker image inspect at export time to guess a pre-built image's WORKDIR, falling back to a hardcoded /app when that failed (image not pulled locally, or docker unavailable). That guess got baked into task.toml and used verbatim by Harbor's docker exec -w, which — unlike docker run -w — hard-fails with exit 127 if the path doesn't exist in the image that actually runs the trial. Confirmed live: a wrong guess broke every trial before the agent ever ran.
• tests/test.sh now resolves its own work directory via $(pwd) at run time instead of a value baked in at export — docker exec always lands the shell there (whether via an explicit -w override or the image's own default WORKDIR), so this is authoritative and never stale.
• task.toml's [environment].workdir is now emitted only when the task explicitly pins one (sandbox.docker.working_dir, or an existing Dockerfile WORKDIR line); otherwise it's omitted entirely so Harbor's docker exec runs with no -w and the image's own current WORKDIR decides. The docker image inspect guess and its /app fallback are removed — nothing left to go stale.
• Regenerated the golden export fixture and updated the packager test suite (52 harbor tests, all passing) plus .claude/notes/reporting.md's Harbor export section to match.

Test plan

• uv run pytest tests/test_harbor_packager.py tests/test_harbor_experiment_packager.py tests/test_harbor_export_golden.py tests/test_evaluate_format_harbor.py -q — 52 passed
• uv run pytest -k harbor -q — 89 passed
• uv run ruff check src/coder_eval/harbor/ tests/test_harbor_packager.py tests/test_harbor_experiment_packager.py — clean
• GOLDEN_REGEN=1 uv run pytest tests/test_harbor_export_golden.py — regenerated and reviewed the diff
• Verified end-to-end against a real Harbor job: exported and ran 10 tasks against skills-image:latest (whose real WORKDIR is /work, not the old guessed /app) via harbor run -a coder_eval.harbor.agent:CoderEvalAgent, confirming the previous docker exec -w /app exit-127 failure is gone and the verifier correctly resolves the agent's actual working directory via $(pwd).

🤖 Generated with Claude Code

[uipreliga]

Review: coder_eval — pr:182 (7 files) axis:1,2,3,4,5,6,7,8 --post-comment

Scope: pr:182 (7 files) axis:1,2,3,4,5,6,7,8 --post-comment · branch akshaya/update_harborframework · 539ba00 · 2026-09-16T20:12Z · workflow variant

Change class: complex — changes how the Harbor export resolves the container workdir (drops export-time docker inspect + WORKDIR append, makes task.toml workdir optional, test.sh uses $(pwd) at run time), which alters verifier runtime behaviour

The codebase is healthy: types, security, architecture, error handling and the evaluation harness have no findings, and no finding can change a task's score or final_status for the same agent output; the real risk is in the Harbor export, where a task with working_dir: auto writes the literal workdir = "auto" to task.toml and the exported task then fails with exit 127 under Harbor; fix that one-line mapping and remove the history comments, then the change is ready to merge.

Summary

Axis	Score	🔴	🟠	🟡	🔵	Top Issue
1. Code Quality & Style	9.3 / 10	0	0	1	2	Workdir change history restated in packager comments, test docstrings and every emitted test.sh, against the CLAUDE.md comment and docstring rules
2. Type Safety	10 / 10	0	0	0	0	—
3. Test Health	9.9 / 10	0	0	0	1	The new run-time workdir contract of test.sh is checked only by a substring match. No test runs the generated script.
4. Security	10 / 10	0	0	0	0	—
5. Architecture & Design	10 / 10	0	0	0	0	—
6. Error Handling & Resilience	10 / 10	0	0	0	0	—
7. API Surface & Maintainability	9.5 / 10	0	0	1	0	The documented working_dir: auto sentinel is passed into task.toml as a literal workdir = "auto", which goes against the PR's own 'explicit override only' contract
8. Evaluation Harness Quality	10 / 10	0	0	0	0	—

Overall Score: 9.8 / 10 · Weakest Axis: Code Quality & Style at 9.3 / 10
Totals: 🔴 0 · 🟠 0 · 🟡 2 · 🔵 3 across 8 axes.

Blockers

None.

Non-blocking, but please consider before merge

1. [Axis 1] Workdir change history restated in packager comments, test docstrings and every emitted test.sh, against the CLAUDE.md comment and docstring rules (src/coder_eval/harbor/packager.py:297) — Delete the three new comment blocks. The docstring of _write_environment already states the contract (L246-249: "workdir is an EXPLICIT override only ... None otherwise"). L297-304: "# No docker image inspect guess here either (v1 used to shell out for / # one, defaulting to a fabricated path on failure -- a real bug: ..." is history of deleted code. L275-278: "# No fabricated WORKDIR appended when the Dockerfile declares none --" describes something that the code no longer does. L572-575: "# No task-controlled value is interpolated into the template anymore -- ... no / # longer an injection surface here to shlex.quote against." describes the removed shlex.quote. Keep the rationale only in .claude/notes/reporting.md, where it already exists. The code then reads workdir = docker_cfg.working_dir with no preamble.
2. [Axis 7] The documented working_dir: auto sentinel is passed into task.toml as a literal workdir = "auto", which goes against the PR's own 'explicit override only' contract (src/coder_eval/harbor/packager.py:305) — SandboxDockerConfig.working_dir (models/sandbox.py:205) documents a sentinel: "The sentinel 'auto' detects the image's WORKDIR at run time". The validator lets it through (if v is None or v == "auto": return v). The PR rewrites both assignments without handling it: line 279 workdir = docker_cfg.working_dir or _find_workdir(dest_dockerfile) and line 305 workdir = docker_cfg.working_dir. So a task with working_dir: auto exports [environment].workdir = "auto", and Harbor passes it as docker exec -w auto. That is the same exit-127 failure the PR says it fixes. The new docstring (line 246) says workdir is "an EXPLICIT override only". Under this design, 'auto' means exactly 'leave unset and let the image's WORKDIR decide'. Map it that way, for example explicit = None if docker_cfg.working_dir == "auto" else docker_cfg.working_dir, and use it at both sites. Add a test in tests/test_harbor_packager.py that asserts result.workdir is None and that task.toml has no workdir key for working_dir: auto. The pass-through already existed on main, but this PR rewrote these lines and set the contract that makes it wrong.

Nits

1. [Axis 1] Notes section says 'no longer' / 'originally', and one unchanged sentence now contradicts the new explicit -w path (.claude/notes/reporting.md:409) — L409 still says "$(pwd) is resolved by the container's shell at exec time and equals the WORKDIR because the exec is given no explicit cwd". After this PR, the exec IS given an explicit -w when working_dir or a Dockerfile WORKDIR is set (L420-422). Change it to say that $(pwd) equals the exec's cwd, whether -w or the image WORKDIR sets it. Also rephrase L470 "The generated shell script no longer interpolates a workdir value at all" and L474 "there is no longer an injection surface" as present-tense statements of the design. The before/after story belongs in the commit message.
2. [Axis 1] ExportResult.workdir is a field with no production reader; the PR widens its type instead of removing it (src/coder_eval/harbor/packager.py:100) — workdir: str | None on ExportResult ("What export_task produced, for the CLI to report.") is not read anywhere in src/: grep -rn '\.workdir' src/ finds no consumer in cli/harbor_command.py or experiment_packager.py. Only tests read it, and those tests could assert on task.toml instead, as several already do. The field has lost its meaning ('the resolved workdir') and now often holds None, so delete it, following 'Delete before you guard'. The tests should check doc['environment'].get('workdir').
3. [Axis 3] The new run-time workdir contract of test.sh is checked only by a substring match. No test runs the generated script. (tests/test_harbor_packager.py:120) — The PR makes tests/test.sh resolve the graded directory at run time. The template now contains coder-eval evaluate /tests/task.yaml "$(pwd)" --in-place --run-dir /logs/verifier || true (packager.py:58). The only check is assert 'coder-eval evaluate /tests/task.yaml "$(pwd)"' in content (test_harbor_packager.py:120), which is repeated at 331-333 and in the golden fixture. That proves the text is there. It does not prove what the change is for: that the shell passes the caller's cwd, quoted, to coder-eval evaluate, and that the || true / set -u flow still reaches the reward step. Add a hermetic test that is skipped on Windows. It puts a stub coder-eval script on PATH that logs its argv, runs the exported test.sh with /bin/sh from a tmp cwd whose path contains a space, and asserts that argv[3] equals that cwd and that the reward call runs. This is cheap and needs no Docker. It is Low because the export module is not on the scoring hot path and "$(pwd)" is standard shell.

What's Missing

Parallel paths:

• 🔵 The docstring of reward_command in src/coder_eval/cli/harbor_command.py:54 still shows the old test.sh shape as the 'typical use': coder-eval evaluate tests/task.yaml "$WORKDIR" .... The PR changed _TEST_SH_TEMPLATE to /tests/task.yaml "$(pwd)" but did not update this user-facing copy. Change it to match the template, or point to _TEST_SH_TEMPLATE, so the only published example of the shim is not a stale variant. (trigger: src/coder_eval/harbor/packager.py)

Downstream consumers:

• 🟡 The PR removed the 'declared no WORKDIR' warning and the /app fallback, and nothing replaces them. A dockerfile_path whose Dockerfile has no WORKDIR, or a pre-built image with no WORKDIR, now makes Harbor run docker exec with no -w. The cwd is then the base image default, often /. The agent phase (agent.py:92, --workspace-dir "$(pwd)") and the verifier (coder-eval evaluate ... "$(pwd)" --in-place) then both use the container root as the workspace, and export gives no signal. Before this PR the exporter warned about this case. Add an export-time warning when the resolved workdir is None and the Dockerfile has no WORKDIR, or document this failure mode in .claude/notes/reporting.md, and add a test that asserts the chosen behaviour. (trigger: src/coder_eval/harbor/packager.py)

Tests:

• 🔵 _write_environment has two rewritten branches (dockerfile_path and pre-built image), and neither has a test for sandbox.docker.working_dir: auto. Add one case per branch that asserts task.toml has no workdir key, so the sentinel cannot leak into docker exec -w auto. (trigger: src/coder_eval/harbor/packager.py) (restates: Axis 7: The documented working_dir: auto sentinel is passed into task.toml as a literal workdir = "auto")
• 🔵 No test pins that the agent phase and the verifier phase resolve the same directory. The two phases are now coupled only because both use "$(pwd)": harbor/agent.py for --workspace-dir, and _TEST_SH_TEMPLATE for the evaluate target. The export no longer writes a shared WORKDIR, so if either side moves to a different expression, grading silently runs against a different tree than the one the agent edited. Add a small parity assertion that both emitted command strings use the same cwd expression. (trigger: tests/test_harbor_packager.py)

Harness & Lint Improvements

Static checks (lint / type):

• [ce-lint] CE068 (the next free id; CE067 was used for a short time and then removed, see .claude/harness-candidates.md:958, so do not use that number again). New rule: no comment or docstring in src/coder_eval/ may tell the history of the code. Put the check in tests/lint/prose_budget.py, which make docs-budget already runs. That file already tokenizes comments and docstrings without importing them. Fail on this case-insensitive phrase list: 'no longer', 'anymore', 'used to', 'v1 used', 'originally', 'previously', 'was removed', 'before this change'. Apply the list to (a) own-line and trailing comments, (b) docstrings, and (c) '#'-comment lines inside module-level str constants that the code writes out as scripts (the harbor test.sh template). Exempt tests/lint/ (CE006/CE007 messages such as 'is no longer a field' are user-facing errors, not history) and .claude/notes/, where the history of a shipped defect is part of the contract. Known blind spot: a history sentence that uses none of these phrases, for example 'No fabricated WORKDIR appended ...'. The rule catches the other two blocks, which are the main defect. Prevents: Finding 1 (Code Quality, medium): packager.py L297-304 ('v1 used to shell out'), L573-575 ('anymore', 'no longer an injection surface'), and the same text copied into every emitted test.sh. The history-docstring members in tests/test_harbor_packager.py:431 are caught if the rule's scope includes test docstrings.
• [ce-lint] Remove the sharp edge first. Give SandboxDockerConfig (models/sandbox.py:199) a property explicit_working_dir -> str | None that returns None for the 'auto' sentinel, or remove the string sentinel completely and add a separate detect_image_workdir: bool field. Then add CE069, an AST rule in tests/lint/rules/ce069_working_dir_via_accessor.py, wired in tests/lint/runner.py. It forbids a raw .working_dir attribute read outside models/sandbox.py and isolation/docker_runner.py, which is the only site that resolves 'auto' at run time (docker_runner.py:380). Other code must read .explicit_working_dir. pyright cannot catch this: Literal['auto'] is a subtype of str, so the sentinel type-checks into any str | None sink, such as the task.toml workdir. Prevents: Finding 5 (API Surface, medium, cross-axis 7/8): packager.py:279 workdir = docker_cfg.working_dir or _find_workdir(...) and packager.py:305 workdir = docker_cfg.working_dir both write the literal 'auto' into [environment].workdir, which causes docker exec -w auto, exit 127.
• [ce-lint] Make CE031 (tests/lint/dead_config_fields.py) also cover public result dataclasses. Accept dataclasses.fields() in addition to Pydantic model_fields, and add a second registry of CLI-facing result types, starting with harbor.packager.ExportResult. A field passes only if some file under src/ reads it by attribute access other than the constructor site. A read that only a test makes does not count, because CE031 already scans src/ only. The existing floor applies: a name collision gives a false negative, never a false positive. Prevents: Finding 3 (Code Quality/API Surface, low): ExportResult.workdir (packager.py:100) has no reader in src/ (not in cli/harbor_command.py and not in experiment_packager.py), and the PR widened its type instead of deleting the field.

Harness improvements (not statically reachable):

• Add a hermetic test that runs the emitted script, in tests/test_harbor_packager.py, marked skip-on-Windows. It exports a task, puts on PATH a stub coder-eval that records its argv and a stub reward step, and runs tests/test.sh with /bin/sh from a tmp cwd whose path contains a space. It asserts that argv[3] equals that cwd exactly, and that the reward step still runs after || true under set -u. Use the same stub pattern for every script template that the packager emits. Why not static: The contract is shell run-time behavior: $(pwd) expansion, quoting of a path with spaces, and control flow with || true / set -u. A substring or golden match on the template text proves only that the text is there, not what the shell does. Prevents: Finding 4 (Test Health, low): the run-time workdir contract of test.sh is checked only by assert 'coder-eval evaluate /tests/task.yaml "$(pwd)"' in content (test_harbor_packager.py:120, 331-333, golden fixture).
• Add a sentinel parity test for export: parametrize over every documented value class of each config field that the Harbor packager copies into task.toml. For working_dir, the classes are None, an absolute path, and 'auto', each with and without a Dockerfile WORKDIR. Assert that task.toml never contains a sentinel literal, and that the key is absent when the value is not an explicit override. Get the sentinel set from the field's validator or from a module constant, so that a new sentinel makes the test fail until someone maps it. Why not static: The defect is a value flowing from a config field to a serialized file. It depends on which literal values have special meaning, and the only record of that is a docstring and a validator branch, not the type. Prevents: Finding 5 (API Surface, medium): working_dir: auto exported as workdir = "auto" at packager.py:279/305, with no test for the 'auto' case.
• Add a CI step (warn only) that checks notes when rationale changes. For each changed src file that has a Rationale: .claude/notes/<file> § <heading> pointer (prose_budget.py already resolves these pointers), warn when the pointed-to section is not in the same diff. Also add a checklist line to the code-review and implement-plan commands: 'grep the pointed notes section for the identifiers this change altered (e.g. $(pwd), -w) and confirm each sentence is still true, in present tense'. Why not static: Only semantic reading can tell whether a prose sentence ('the exec is given no explicit cwd') still describes the code. A diff heuristic can find the section that needs a check, but it cannot judge whether the section is correct. Prevents: Finding 2 (Code Quality, low): .claude/notes/reporting.md:409 now contradicts the explicit -w path at L420-422, and L470/L474 use 'no longer' framing.

Top 5 Priority Actions

1. In src/coder_eval/harbor/packager.py:279 and :305, change the working_dir: auto sentinel to None before you write [environment].workdir, so Harbor does not run docker exec -w auto and fail with exit 127, and add a test in tests/test_harbor_packager.py that checks task.toml has no workdir key for auto.
2. Add a hermetic test in tests/test_harbor_packager.py (near :120) that runs the exported test.sh with /bin/sh from a tmp cwd whose path contains a space, with a stub coder-eval on PATH, and check that argv[3] equals that cwd and that the reward step still runs.
3. Delete the history comment blocks in src/coder_eval/harbor/packager.py at L275-278, L297-304 and L573-575, and the matching history text in the test docstrings (tests/test_harbor_packager.py:431) and the emitted test.sh, because the _write_environment docstring and .claude/notes/reporting.md already state this rationale.
4. Delete the ExportResult.workdir field at src/coder_eval/harbor/packager.py:100, because no production code reads it, and change its tests to check doc['environment'].get('workdir') in task.toml.
5. In .claude/notes/reporting.md:409, correct the sentence to say that $(pwd) equals the exec's cwd, whether -w or the image WORKDIR sets it, and write L470 and L474 as present-tense design statements, not 'no longer'.

---

Stats: 0 🔴 · 0 🟠 · 2 🟡 · 3 🔵 across 8 axes reviewed.