Repository navigation
Add a pinned Harbor adapter for S-Code - #130
Conversation
Benchmark harnesses such as Harbor run S-Code with no one present, often against self-hosted models. Add settings for those runs. Every default keeps today's behavior. - Turn limits move into configuration: turn.max_model_calls, max_tool_calls, max_elapsed_seconds, model_stream_idle_seconds, max_cost_micros and max_total_tokens, with S_CODE_TURN_* overrides. The defaults are the previous built-in values. - turn.disabled_tools (S_CODE_TURN_DISABLED_TOOLS) removes tools from the model's tool list and rejects them if they are called anyway, including from Code Mode. - model.connect_timeout_seconds, read_timeout_seconds and request_timeout_seconds (S_CODE_MODEL_*_TIMEOUT_SECONDS) bound requests to endpoints set in the configuration file or environment, between 1 and 3600 seconds. The defaults stay 10, 45 and 180 seconds. - model.merge_system_messages (S_CODE_MODEL_MERGE_SYSTEM_MESSAGES) sends an OpenAI-compatible request's system messages, in order, as one leading message, for chat templates such as Qwen3.6's that reject later ones. It is off by default and, like the timeouts, applies to endpoints set in the configuration file or environment. - A new --raw-events-jsonl output mode emits every raw event as versioned JSON Lines. In that mode a question for the user ends the run instead of waiting for --timeout. Other output modes are unchanged. Refs #129 Signed-off-by: Wei Deng <wdeng@wustl.edu>
Add integrations/harbor/build-bundle.sh. It refuses a checkout with any uncommitted or untracked file and builds the Linux x86-64 s-code, s-code-cli, s-code-daemon and s-code-pdf-worker binaries from HEAD. It then runs their self-tests and writes a gzip tar archive with a SHA-256 sidecar. The archive's manifest records the source commit and tree, the lockfile digests, the tool versions, and each file's digest, size and mode. integrations/harbor/runtime.py validates an archive against its SHA-256 digest and manifest without extracting or running anything. It uses only the Python standard library. Refs #129 Signed-off-by: Wei Deng <wdeng@wustl.edu>
integrations.harbor.agent:SCodeAgent runs one non-interactive S-Code turn per Harbor trial against an OpenAI-compatible endpoint. It is pinned to harbor==0.17.0. - Install: validates the bundle, uploads it, checks its digest inside the task container, and runs each binary's self-test with fresh S-Code state. - Run: uses Full permission mode and --raw-events-jsonl. It disables the browser, PDF and interactive-input tools and merges system messages. It keeps S-Code's limits inside the task's Harbor deadline. - Results: keeps events.jsonl, stderr.log, final.txt, provenance.json and daemon.log in the trial's agent logs. A run that never gets a model response is raised as Harbor's NetworkConnectionError or UnknownApiError, so it counts as an infrastructure error and the verifier still runs. Refs #129 Signed-off-by: Wei Deng <wdeng@wustl.edu>
Add integrations/harbor/README.md: requirements, building the bundle, running a Harbor job, the agent kwargs and the files kept per trial. It also explains that the task container must reach the model endpoint itself. Route integrations/harbor and its tests to the benchmarks CI job, and run the adapter and bundle tests there. Refs #129 Signed-off-by: Wei Deng <wdeng@wustl.edu>
Benchmarks compare agents at fixed sampling settings. S-Code always samples at temperature 0. It starts with 8,192 output tokens per model call and doubles that after a cut-off response, up to 32,768. Add model.temperature (S_CODE_MODEL_TEMPERATURE, 0 to 2) and model.max_output_tokens (S_CODE_MODEL_MAX_OUTPUT_TOKENS, 1 to 1000000). When they are unset, behavior is unchanged. An explicit output cap stays fixed instead of doubling. The Harbor adapter passes both through as the temperature and max_output_tokens kwargs and records them in provenance. Refs #129 Signed-off-by: Wei Deng <wdeng@wustl.edu>
The existing tests cover the sampling settings' validation and the agent's fixed output cap, but none followed a configured value to the request the model receives. Add one: it runs a turn without the settings and checks the defaults (temperature 0, 8,192 output tokens), then with temperature 0.6 and max_output_tokens 4,096 and checks those values arrive. Refs #129 Signed-off-by: Wei Deng <wdeng@wustl.edu>
al2wang
left a comment
There was a problem hiding this comment.
I reviewed the whole delta at 0f98212, against base eac5c1a (current main). That covers the adapter, the bundle builder and runtime manifest, the config, gateway, daemon and CLI changes, and the tests. I also read #131 and #132 for context.
The adapter is careful work:
- The bundle is deterministic, and its manifest is checked without extracting it.
- Every timeout is derived from Harbor's resolved deadline.
- Cleanup is bounded, and cancellation survives it.
- Event validation fails closed.
- Disabled tools are filtered on every path I could find: the tool list, catalog search, executor preparation, Code Mode children and Chat
start_work. - The new
TurnConfigdefaults equal the previous built-in limits.
Testing
CI is green at this head. Locally:
cargo fmt --checkandclippy -D warningsare clean.- Tests pass for config (26), model-gateway (37) and cli (173).
- s-code-daemon passes 239 of 240. The one failure is
code_mode_worker::pdf_worker_runs_through_the_networkless_os_sandbox, which needsbwrap. That machine has none, and the test fails the same way onmainthere. It passes in CI. - The adapter (17) and bundle (9) tests pass on Python 3.12, and
test-ci-plan.py(19) passes. - An import smoke test against the real
harbor==0.17.0wheel passes.
I did not run Harbor end-to-end with a model. The items below come from code reading, small container probes, and repro scripts that drive the PR's runner and adapter with fake binaries.
There are three themes, all about whether a Harbor score measures S-Code rather than the container or the harness. The first two mostly come from existing runtime behaviour, but this adapter is where they start to change results.
1. Programs that agent commands start don't see the task image's PATH or HOME
The program the model names in run_command is found on the daemon's own PATH (program_search_path, crates/tool-runtime/src/lib.rs:163 and :828-839). Under Harbor, that is the container's PATH. The command itself, though, runs with a cleared environment:
PATHbecomes<staged program dir>:/usr/bin:/bin:/usr/sbin:/sbin(crates/platform-runtime/src/lib.rs:118, applied at:821-822);- besides it, only a per-command
TMPDIR,HOMEandXDG_CACHE_HOMEare set; - the one exception is the managed-runtime shim path, which already lists
/usr/local/bin(crates/tool-runtime/src/lib.rs:1852-1856).
So program: "python3" works, but anything a command then starts by name does not when it lives under /usr/local/bin or in a directory added by the Dockerfile's ENV PATH. Examples are sh -c "python3 …", a Makefile calling go, pip's console scripts, and npm scripts calling node.
That covers a lot of task images:
- The official language images install their toolchains under
/usr/local:python:*,node:*,golang:*andrust:*. - Many Dockerfiles add paths with
ENV PATH=..., such as/opt/venv/binor/root/.cargo/bin.
In python:3.12-slim, here is what a shell started this way sees:
$ env -i PATH=/bin:/usr/bin:/usr/sbin:/sbin /bin/sh -c 'command -v python3'
(exit 127)
$ env -i PATH=/bin:/usr/bin:/usr/sbin:/sbin /bin/bash -lc 'command -v python3'
/usr/local/bin/python3
So sh -c "python3 …" exits 127 unless the model happens to use a login shell. A login shell restores /usr/local/bin from /etc/profile, but it still drops paths that only exist in the Dockerfile's ENV. No run_command path defaults to a login shell. We saw exactly this in internal Terminal-Bench runs on python:*-slim images: shell-invoked python3 exited 127, including in the agent's own check scripts.
HOME is a fresh, empty scratch directory for each command (crates/tool-runtime/src/lib.rs:1831-1838), not the image user's home. So ~/.local (from pip install --user), ~/.config and git config --global don't persist from one command to the next.
The result is a silent shortfall that depends on the image and on the model's command style.
The smallest fix is to put /usr/local/bin (and /usr/local/sbin) ahead of /usr/bin for sandboxed workspace commands, matching what the shim path already does. For Harbor specifically, an adapter-managed opt-in might be better: give commands the agent user's container PATH (and HOME) as captured at install, and keep clearing everything else. A test that runs sh -c 'python3 --version' through run_command in python:3.12-slim would lock in either fix.
2. Files the agent creates are owner-only (0600/0700)
The runner sets umask 077 (integrations/harbor/agent.py:94) before s-code exec. The launcher sets it again in the subshell that starts the daemon (scripts/s-code:175). Nothing in the Rust crates resets it for child processes, so files that shell commands create are 0600 and directories are 0700.
In the same image:
- With the image default (
022), a newindex.htmlis-rw-r--r--. - After
umask 077, it is-rw-------in adrwx------directory. - The result is the same when a child process with a cleared environment creates the file.
By default Harbor's verifier runs as the image's default user, usually root, so default runs won't show this. Some setups will fail where agents that keep the container's umask would not:
- tasks whose verifier runs as a different non-root user (Harbor 0.17 has separate
[agent].userand[verifier].user); - services running as another user (
www-data,postgres); - tests that check file modes.
Separately, and already true on main, the edit tool does this regardless of umask. On Unix, apply_patch creates a new file through a temporary file opened with an explicit 0600 (crates/tool-runtime/src/lib.rs:1322-1328). Only files that already existed keep their mode (:1302-1310, :1334-1335). So a new script or config file the model creates through the edit tool is 0600 even under 022.
Suggestion: keep the private state protected with explicit modes, as the runner already does with mkdir -m 700. Then:
- start tool processes with the container's original umask. For example, the runner could record
$(umask)first, and platform-runtime could apply that value inpre_exec; - create new workspace files with
0666 & ~umaskinstead of a fixed0600.
3. How outcomes reach Harbor (retries and the #132 audit)
- S-Code's own outcomes look retryable. A tool-call or model-call limit stop, or an ordinary failed turn, ends with CLI exit 1.
run()then re-raises Harbor's genericNonZeroAgentExitCodeError(agent.py:1107-1108). Under Harbor's default retry filter, that type is not excluded (harbor/models/job/config.py:288-296).- With
--max-retries > 0, Harbor reruns the trial and deletes the failed trial directory first (harbor/trial/queue.py:200-222). That happens whatever the reward was. - The score quietly becomes "keep the last attempt" while provenance shows one attempt.
- Baselines such as Terminus-2 return normally when their budget runs out.
- #132 fixes this for timeouts by raising
TimeoutError. The limit stops and ordinary failures are still affected. - Suggestion: return normally for S-Code-owned terminal outcomes (the verifier still scores them) and raise only for infrastructure. Or at least have the README name
--retry-include NetworkConnectionError UnknownApiError.
- With
- A run that never starts is scored as an agent failure. If the daemon isn't ready within the launcher's 5 s window (
scripts/s-code:185-194), the CLI exits 1 with no events and zero model requests. A repro with the real launcher and a daemon that never becomes ready exits 1 after 5.2 s. It also happens if the CLI refuses the workspace (see the/note below).- The classifier marks a run as an endpoint problem only when at least one request failed (
agent.py:489-495). So this case becomesNonZeroAgentExitCodeErrorwithfailure_kind: null. - The task is untouched, so it scores 0. #132's
audit_trialreturnsflagged: falsefor it. - Suggestion: treat "no turn started" (empty or absent event stream) as infrastructure.
- The classifier marks a run as an endpoint problem only when at least one request failed (
- Deterministic rejections are treated as endpoint faults.
model_rejectedcovers every non-2xx response, including deterministic 400s such as a request over the model's context length.- It is raised as
UnknownApiError, retried, and then excluded by #132's audit. - An S-Code prompt that a model cannot accept therefore disappears from the denominator instead of counting as a failure.
- Suggestion: separate 400/413/422 from 401/403/429/5xx.
- It is raised as
- Token- and cost-budget stops look like crashes. These stops (
TurnError::LimitExceeded, for example fromrecord_usageatcrates/agent-core/src/lib.rs:854) take the execution-error path. That path publishesturn.failedwith only{"status":"failed","error_code":"agent_error"}(crates/daemon/src/lib.rs:15768-15772) and noturn.usage. The adapter records a generic failure with nolimit_kindand no token counts.
Smaller notes
- System-message merging changes behaviour for every model.
S_CODE_MODEL_MERGE_SYSTEM_MESSAGESis managed and alwaystrue, so it applies to every model, not only to those whose chat templates reject later system messages. agent-core appends a budget-convergence system reminder at 80% ofmax_model_calls(crates/agent-core/src/lib.rs:662-667). With merging on, it moves into the leading system message, away from the latest turns, and the system prefix changes mid-turn. A kwarg defaulting to on would let other endpoints opt out. - Long instructions can't reach S-Code. The instruction is passed as one argv element (
agent.py:155-167). At 128 KiB (MAX_ARG_STRLEN) the runner exits 126 with "Argument list too long" before S-Code starts. 131,071 bytes runs; 131,072 bytes does not. Terminal-Bench instructions are far smaller, but other Harbor datasets may not be. - A task with no workdir can't start. The runner doesn't set a working directory. A task with no
[environment].workdirand no imageWORKDIRruns in/.overlaps()incrates/cli/src/commands/workspace.rstreats/as overlapping/installed-agent/s-code-private/*, so the non-interactive CLI refuses the workspace. I found this by code reading; I did not run it. - Provenance doesn't identify the adapter. It pins the bundle but records the adapter only as the constant
0.1.0. The adapter decides timeouts, disabled tools, merging and classification, so a hash ofintegrations/harbor/*.pywould make runs from different adapter code distinguishable. - Unmanaged settings go unrecorded. Unmanaged
S_CODE_*variables in the agent env (S_CODE_CONFIG,S_CODE_MCP_ENABLED, and with #131S_CODE_MODEL_TEMPERATURE) reach S-Code but not provenance. With #131, setting the temperature through the env recordstemperature: null. Rejecting unmanagedS_CODE_*names, or recording them, would keep provenance faithful. - The model key stays readable through
/proc. The model credential stays in the daemon's environment for the whole run. Clearing the environment keeps it out of command environments, but a command running as the same user can still read/proc/<daemon pid>/environ. I checked this with a fixture value inpython:3.12-slim. This is the usual trade-off for installed agents. A README note recommending a scoped, budget-limited key would cover it. - The adapter tests can't catch drift. They stub every Harbor module and hand-write raw events. A small CI step that imports the adapter against the pinned
harbor==0.17.0wheel would catch drift in the Harbor API. A golden events file captured froms-code exec --raw-events-jsonlwould do the same for event names and reason strings. - #132's stack base. #132's parent is
c0097f3, the first commit of #131, not #131's head71e08c8. This will matter when the stack is retargeted.
Make agent sampling configurable
|
Thanks for the careful review. Our plan for issue 1: Under Harbor, S-Code runs in Full mode: Fix: a Harbor-only "container environment" mode.
We pass the whole baseline environment rather than just Tests cover normal builds, git config via |
al2wang
left a comment
There was a problem hiding this comment.
Thanks, Wei. This is a follow-up on the new head a0722df; my earlier review was on 0f98212. It replies to the issue-1 plan and covers what changed since then: the #131 merge (c0097f3, 71e08c8; +296/−3 across 10 files).
A correction first
You're right about HOME, and my review was wrong.
- The adapter runs
s-code exec --permission-mode full, so commands go throughrun_host. It callsexecute_hostwith an emptyenvironment_handles(crates/tool-runtime/src/lib.rs:840-847). - After
env_clear(), a command gets onlyPATH, a per-commandTMPDIR,OPENSSL_CONFfornode, and those empty handles (crates/platform-runtime/src/lib.rs:821-833).HOMEis unset. - The per-command scratch
HOMEI cited (:1831-1838) is built only on the sandboxed path (run_with_compatibility).
The effect is the same either way: ~/.local, ~/.config and git config --global don't persist.
The issue-1 plan
I agree with the approach. It is the adapter-managed opt-in I suggested, and passing the whole baseline environment is the right call given the six tasks that need more than PATH. Keeping the global PATH change out of normal builds is fair. The adapter-code hash covers my provenance note.
A few things would make it robust:
- Fail closed. If the feature is on and the capture file is missing, unreadable or malformed, stop the run with a clear reason. A silent fallback to the cleared
PATHwould bring back the original shortfall with no signal. A missing-file test would lock this in. - Protect the capture file.
- Write it into the private directory at
0600, as the agent user. - Before loading it, have S-Code check that it is a regular file (not a link) with the expected owner and mode.
- Delete it before the first command runs.
- Write it into the private directory at
- Keep the key out by name and by value. Besides subtracting the
--aenames, drop the adapter'scredential_envname, and any variable whose value equals the credential, in case the task environment carries it under another name. The canary test can check both the capture file and a command's environment. - Decide name collisions. If an
--aename also exists in the image's ownENV(sayPYTHONPATH), subtracting by name drops the image's value too. Either behaviour is defensible, but it should be chosen and tested. - Name the volatile fields. List the exclusions explicitly (for example
PWD,OLDPWD,SHLVL,_) and test that list, so a change in how Harbor runs commands can't quietly add entries. - Make the switch visible. The variable that turns the mode on should be managed, so the agent env can't override it. Provenance should record whether the mode was on, and with which bundle features, next to the names and hash.
- Capture the umask in the same step. Recording the container's
$(umask)at install is the missing piece for theme 2. platform-runtime could apply it to commands, and the edit tool could create new files with0666 & ~umaskinstead of a fixed0600. - Test the original symptom directly. Run
sh -c 'python3 --version'throughrun_commandinpython:3.12-slim, next to theHOME/git-config test.
Are themes 2 (file modes) and 3 (how outcomes reach Harbor, beyond #132's timeouts) planned for this PR or for follow-ups? Either works; it would just help to know.
The #131 delta
This part looks good, and each layer has its own test:
- Config bounds
model.temperatureto 0–2 andmodel.max_output_tokensto 1–1,000,000. The env parsing rejects non-finite values. - agent-core:
fixed_max_output_tokens(serde defaultfalse) turns off output-token doubling at both growth sites, so a configured cap really is the cap. - The daemon applies the settings to agent turns only (
execute_turn). Title generation keeps its fixed 0. - The adapter's kwargs are validated, sent only when set, and recorded in provenance.
Three small notes:
1. Temperatures go out as widened f32 values. (executed)
- The daemon narrows the configured
f64tof32(crates/daemon/src/lib.rs:1359). - The gateway puts the
f32into the body withjson!(crates/model-gateway/src/lib.rs:433,:485,:558). - In serde_json 1.0.151 (the
Cargo.lockversion, withoutarbitrary_precisionanywhere in the tree),Number::from_f32storesf as f64before formatting.
A small program built against that version prints:
0.25 -> {"temperature":0.25}
0.6 -> {"temperature":0.6000000238418579}
0.7 -> {"temperature":0.699999988079071}
Endpoints will treat these as practically the same. But the wire value no longer matches the configured one, or the 0.6 the adapter records. The gateway's f32 field predates #131; it only shows now that the value is configurable. The new gateway test uses 0.25, which f32 represents exactly (crates/model-gateway/src/lib.rs:2140), so it can't catch this. Carrying f64 through to the request, since config already holds one, plus a test with 0.6 would fix it.
2. An env-set temperature still reads null in provenance. (code reading)
S_CODE_MODEL_TEMPERATUREandS_CODE_MODEL_MAX_OUTPUT_TOKENSare managed only when their kwarg is set (integrations/harbor/agent.py:720-723). Otherwise a value from the agent env passes through (:727), and provenance recordsnull(:879-880).- Your plan to record unmanaged
S_CODE_*names would show that the variable was set, but not its value. For these two the value is the setting. - I'd refuse the two names in the agent env now that the kwargs exist, or record the value actually used.
3. A fixed cap and reasoning models. (code reading)
- With a fixed cap, a cut-off or empty response is retried at the same cap, at most twice. Then the turn fails with "model returned repeated empty responses" or the incomplete-response reason (
crates/agent-core/src/lib.rs:945-967,:982-1014). - That is the intended "fixed" meaning. But a reasoning model whose hidden reasoning uses a low cap will end turns this way, so a sentence in the docs and README would help benchmark users choose the cap.
Testing at this head
CI is red at this head; the previous head was green.
- linux:
tests::fast_durable_question_resume_keeps_tool_limit_fail_closedtimed out with "the question must be answerable before its durable worker settles: Elapsed(())" (213 passed, 1 failed). - macOS: the CLI end-to-end check failed with "CLI did not exit after Ctrl-C".
- source-gate fails as their aggregate.
Neither looks caused by this delta:
- The durable-question test comes from main (
c8ee6a4). It busy-polls for up to 5 s for the moment when the turn isAwaitingInputwhile the durable task is stillRunning, before the worker pauses itself (crates/daemon/src/lib.rs:33673-33692). A loaded runner can miss that window. - The delta doesn't touch the CLI or the interrupt path.
- With the new settings unset, the agent-turn request is unchanged (temperature 0, 8192 tokens, cap not fixed).
Rerunning the failed jobs should show whether both are flakes.
Locally at a0722df (offline, --locked):
- config: 27 passed;
- model-gateway: 37 passed;
- agent-core: 61 passed;
s-code-daemon --lib: 214 passed. That is the same target where CI had one failure, and it includesconfigured_sampling_settings_reach_the_model_request.- The durable-question test alone passed 20 of 20 runs.
- The Harbor adapter (18) and bundle (9) tests pass on Python 3.12.
I did not run the macOS Ctrl-C check.
Under Harbor, S-Code runs in Full mode, where run_command clears the environment and sets a minimal PATH and no HOME. Tools in /usr/local/bin or in Dockerfile ENV PATH directories, PYTHONPATH and JAVA_HOME were invisible to anything a command started, so scores could reflect the harness rather than S-Code. Add a Harbor-only container environment mode: - A default-off Cargo feature, harbor-container-env, that only integrations/harbor/build-bundle.sh enables. The bundle manifest records it and the adapter refuses bundles without it, so normal builds cannot activate the mode. - Before it sets anything of its own, the runner saves its environment as the agent user, minus every variable Harbor passed to the agent. The daemon loads it once at startup, only if it is a private regular file this user owns; it drops S_CODE_*, volatile shell fields and any variable holding the model credential, deletes the file and writes a summary of names and a hash. - Full-mode commands then run in that environment, with PWD set to their directory, and are found on its PATH. S-Code adds no PATH, TMPDIR or OPENSSL_CONF. Sandboxed commands, extensions and normal builds are unchanged. - credential_env must start with S_CODE_HARBOR_, and the adapter no longer sets TERM and NO_COLOR, which would otherwise reach every command. - Provenance records the verified mode with the variable names and their hash, a hash of the adapter sources in its version, and the names of unmanaged S_CODE_* variables. Refs #129 Signed-off-by: Wei Deng <wdeng@wustl.edu>
A configured temperature was narrowed to f32 on its way to the model, so 0.6 went out as 0.6000000238418579 while provenance recorded 0.6. Carry it as f64 through the daemon, agent-core and the gateway, and check 0.6 in the gateway test, which f32 cannot represent exactly. The Harbor adapter now refuses S_CODE_MODEL_TEMPERATURE and S_CODE_MODEL_MAX_OUTPUT_TOKENS in the agent env, so provenance records the values actually used, and the docs explain that a fixed output cap also bounds a reasoning model's hidden reasoning. Refs #129 Signed-off-by: Wei Deng <wdeng@wustl.edu>
|
Thanks. Issue 1 is in a2509ff and your #131 notes in b6232f3. Issue 1, against your list:
Themes 2 and 3 will be follow-up PRs; the umask capture goes with theme 2. #131: temperature is f64 end to end (gateway test at 0.6), the adapter refuses |
|
Also done, as promised (a2509ff): each run's record now shows which adapter code ran and any extra S-Code settings passed in. We're leaving your other small notes out of this PR. |
al2wang
left a comment
There was a problem hiding this comment.
Thanks, Wei. This re-review covers a0722df..b6232f3: a2509ff (issue 1) and b6232f3 (the #131 notes).
The #131 notes (b6232f3)
All three are addressed:
- Temperature is
f64from config to the request body, and nothing in those crates narrows it any more. The gateway test now checks 0.6, so it fails if anything narrows the value again. - The adapter refuses
S_CODE_MODEL_TEMPERATUREandS_CODE_MODEL_MAX_OUTPUT_TOKENSunless a kwarg manages them, and provenance names any other unmanagedS_CODE_*variable. - The docs and README say the cap also bounds hidden reasoning.
Issue 1 (a2509ff)
This follows the plan and covers my list. What I checked:
- The loader (
crates/platform-runtime/src/container_environment.rs:110-229):- it opens with
O_NOFOLLOW; - it requires a regular file owned by the effective uid, with no group or other bits;
- it reads at most 1 MiB + 1 byte and deletes the file before any later check can fail;
- it drops
S_CODE_*, the agent names, the four volatile names and exact credential values; - it writes the summary with
create_newat0600, after removing a stale one.
- it opens with
- Where the environment goes. Only
run_hostcallsexecute_host(crates/tool-runtime/src/lib.rs:850), so only Full-moderun_commandgets the environment.- Code Mode has no
run_commandof its own (crates/daemon/src/code_mode.rs:850). - Background terminals and hooks are separate, user-confirmed paths. They keep the minimal environment.
- Code Mode has no
- The runner's capture. I ran its snippet under
dashwith a made-up environment. The file was0600and the--aenames were gone.PWDwas captured, and the daemon drops it.
A few small points, none blocking:
1. Where the fail-closed lives. (code reading, plus an existing test)
- In a feature build, the daemon loads nothing when
S_CODE_HARBOR_COMMAND_ENV_FILEis unset (crates/daemon/src/main.rs:1043-1045; your test asserts this at:2584). Its commands then get the minimal environment. The comment above it (:1030-1032) says a missing setting stops the daemon. - In the Harbor flow the runner covers this case. It refuses unless the capture succeeded, and
_assert_runner_refuses_without_snapshottests that with no variable and with a missing file. So I don't see a gap in the flow as written. - The result can't tell the two cases apart, though. A completed run whose summary is missing still scores as
completed, withhost_command_env: "unverified"(test_successful_run_is_one_shot_and_records_usage,tests/test-harbor-adapter.py:1253-1280). - Having a feature build refuse to serve without the variable would make the comment true.
--self-testalready returns before the load (main.rs:1089-1094). Thenunverifiedcould only mean the summary was lost. Treatingunverifiedas an infrastructure failure would be the stricter option.
2. A snapshot without PATH loads. (code reading)
loadaccepts an empty snapshot, or one with noPATH. It also skips records without=, while its doc comment says any failure is an error (container_environment.rs:106-108).- With no
PATH, commands run with noPATHat all, while the program lookup falls back to the daemon's search path (host_search_path,crates/tool-runtime/src/lib.rs:2321-2329). env -0in a real task container always printsPATH, so a missing one means the capture went wrong. Refusing such a snapshot would match the rest of the design. Otherwise, narrow the comment.
3. The README's credential sentence. (docs) "Commands never receive --agent-env variables" (integrations/harbor/README.md:154) is accurate for what commands inherit. The /proc/<daemon pid>/environ point from my first review still applies, so adding "by inheritance" and the scoped-key line there would keep the sentence exact. I know you're leaving the other notes out; this one is just wording in a paragraph this PR adds.
4. The python:3.13-slim check. I couldn't find this run in the diff, and neither of us ran a container here. If it was a manual Harbor run, please paste a short transcript into the PR: the command, and what the run_command printed as root and as the non-root user. Committing it as an opt-in test would keep it.
Themes 2 and 3 as follow-ups sounds good.
Testing at this head
CI is green at b6232f3, including the new harbor-container-env steps.
Locally at b6232f3 (--locked):
- Python 3.12: the Harbor adapter tests (25) and bundle tests (11) pass.
- fmt and clippy: fmt is clean. Clippy with the feature on platform-runtime, tool-runtime and the daemon (
--all-targets -D warnings) is clean. evals builds. - With the feature, these pass:
- all 7
container_environmenttests; host_lookup_uses_the_container_path_before_the_daemon_path;tests/harbor_container_env.rs;- the daemon binary's 20 tests,
harbor_command_environment_needs_every_setting_and_drops_agent_namesamong them.
- all 7
- Without the feature, these pass: config 27, model-gateway 37, agent-core 61,
s-code-daemon --lib214, andnormal_builds_never_have_a_container_environment. - Expected failures on this host: 13 platform-runtime sandbox tests and 1 tool-runtime sandbox test (
commands_cannot_read_sensitive_workspace_files) fail with ENOENT, because this host has no bubblewrap.- The 13 fail the same way at
a0722df. The tool-runtime test is unchanged in this delta. - CI, which has bubblewrap, passes them.
- The 13 fail the same way at
Signed-off-by: Wei Deng <wdeng@wustl.edu>
b835451 to
c304734
Compare
|
Thanks for the re-review. I addressed the two small follow-ups in
The requested
Both runs passed verification with reward 1 and no exception. Stricter malformed/missing- These were the last PR-scoped items. I consider #130 ready to merge once CI finishes—could you please approve it? |
al2wang
left a comment
There was a problem hiding this comment.
Thanks, Wei. This re-review covers b6232f3..c304734 (9 lines in crates/daemon/src/main.rs and integrations/harbor/README.md).
The two follow-ups
Both are done.
- The daemon refuses without the snapshot.
- In a feature build, an unset
S_CODE_HARBOR_COMMAND_ENV_FILEis now an error (crates/daemon/src/main.rs:1043-1044). Startup stops with "cannot load the Harbor command environment: S_CODE_HARBOR_COMMAND_ENV_FILE must be set" (:1337-1338), so the doc comment above the function (:1030-1032) is now accurate. - The test asserts the exact message (
:2583-2586). - The callers that start a feature daemon without the variable still return before the load (
main.rs:1088-1092). These are the--self-testruns in the adapter's install step (integrations/harbor/agent.py:991) and inbuild-bundle.sh:166, and they don't reach it. The CLI's--self-testdoesn't start a daemon (crates/cli/src/args.rs:261-265). - The runner sets the variable for the real run (
agent.py:837), so the Harbor flow is unchanged. - With this,
host_command_env: "unverified"can only mean the summary was lost after a successful load. Classifying that as a follow-up is fine with me.
- In a feature build, an unset
- The README. "never inherit" plus the scoped, budget-limited credential and
/procsentence (integrations/harbor/README.md:154,:161-162) make the paragraph exact.
One optional tidy-up, for this PR or later: harbor_command_environment can no longer return Ok(None), so its Option and the if let Some at its call site (:1337) could go.
python:3.13-slim
Thanks for the transcript. That covers it: root and non-root both get /usr/local/bin/python3, the expected HOME, and no TMPDIR or credential, and both runs pass with reward 1. Those runs were at b6232f3. This delta only adds the refusal when the variable is unset, and the runner always sets it, so the result carries over.
The PATH validation and the missing-summary classification as follow-ups, and keeping the Docker fixture out of this PR, both sound good to me.
Testing at this head
CI is green at c304734 (run 39461326985): linux, macos, source-gate and the rest pass, and windows is skipped. The linux job runs the feature's clippy and tests, including cargo test -p s-code-daemon --features harbor-container-env --bin s-code-daemon, which holds the updated test.
Locally at c304734 (offline, --locked):
- Python 3.12: the Harbor adapter tests (25) and bundle tests (11) pass.
- fmt is clean, and clippy with the feature on the daemon (
--all-targets -D warnings) is clean. - The daemon binary's 20 tests pass with the feature, the updated one among them.
- The built feature daemon, started with no
S_CODE_HARBOR_*variables:--self-testexits 0 with "s-code web self-test ok";- a plain start exits 1 with
Error: "cannot load the Harbor command environment: S_CODE_HARBOR_COMMAND_ENV_FILE must be set".
- The CLI end-to-end scripts build the daemon without the feature (
tests/test-cli-e2e.sh:56,tests/test-first-run.sh:66), so they never pick up the feature binary.
Nothing blocking remains from my reviews.
Closes #129.
Adds a pinned Harbor 0.17 adapter so S-Code can run as an agent on Harbor benchmarks such as TBLite and Terminal-Bench, plus the S-Code settings that unattended runs need. Phase 0 results are in this comment.
Commits
turn.*,S_CODE_TURN_*). New settings:turn.disabled_tools, model request timeouts (model.*_timeout_seconds), an opt-inmodel.merge_system_messages, and a--raw-events-jsonloutput mode.integrations/harbor/build-bundle.shbuilds checksummed Linux x86-64 binaries from a clean commit, andruntime.pyvalidates them.integrations.harbor.agent:SCodeAgentruns one S-Code turn per trial within the task's Harbor deadline, and reports runs that never reach the model as infrastructure errors.Backward compatibility
Nothing changes for existing users unless they opt in.
model.merge_system_messagesis set.--jsonland--stream-jsonoutput modes behave as before.Testing
cargo fmtandcargo clippy -D warningsare clean. Tests pass for the CLI, config, model gateway and daemon.0f98212) passes the deterministic Harbor fixture as root and as a non-root agent user.Follow-ups