Repository navigation
Conversation
yaoyu-33
added
area:agent
github-actions
Bot
added
the
sla:triage-overdue
|
/ok to test 93c5387 |
|
/review |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP — LGTM — no reliability concerns.
Reviewed both changed files, including batch construction and observation pairing, the regression tests, and affected response serialization and training-metadata paths. No actionable BLOCKER, RISK, or NOTE findings.
Static review only: tests and model rollouts were not executed. Harbor’s upstream trajectory producer is an external dependency absent from the mounted trees.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
github-actions
Bot
removed
the
sla:triage-overdue
|
/ok to test 94e45ea |
|
/review |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP — LGTM — no reliability concerns.
Follow-up to the prior clean review: compared the previous pairing logic with the current converter, inspected both changed files and the retained regression assertions, and traced affected response serialization and training-metadata paths. Shared-observation batching and ordinary positional pairing remain intact. No actionable findings were established.
Static review only: tests and model rollouts were not executed. Harbor’s pinned trajectory producer is external and absent from the mounted trees, so its emitted fields could not be independently confirmed. Two completed child report payloads were unavailable; their producer and downstream-integration topics were independently inspected by the parent.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a findings-only review. Request a full review with /review reset=true.
|
/ok to test 1600e4c |
|
/review |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP — LGTM — no concerns found.
Follow-up to the prior clean review: compared the previous batching logic with the current converter, inspected both changed files and the replacement and retained regression assertions, and traced affected response serialization and metadata paths. Shared-observation steps retain individual calls with matching outputs; zero-result steps and complete positional pairing remain unchanged. No actionable findings were established.
Static review only: tests and model rollouts were not executed. Harbor’s pinned trajectory producer is external to the mounted trees, so its emitted field guarantees could not be independently verified.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a findings-only review. Request a full review with /review reset=true.
|
/ok to test fd67629 |
|
/review |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP — LGTM — no concerns found.
Follow-up to the latest clean review: compared the previous pairing logic with the current converter, inspected both changed files and the updated and retained regression assertions, and traced affected response serialization, training metadata, and verifier-consumer paths. Shared-observation steps retain matching call/output IDs and place the screen on the last call; single-call, zero-result, and complete positional-pairing paths remain unchanged. No qualifying introduced regressions were established.
Static review only: tests and model rollouts were not executed. Harbor’s pinned trajectory producer is external to the mounted trees, so its emitted-field guarantees could not be independently verified. All three delegated inspections completed; two report payloads were unavailable, and their converter and producer/test topics were independently inspected by the parent.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a findings-only review. Request a full review with /review reset=true.
The pinned Harbor Terminus-2 never sets source_call_id, so the branch was dead, and in a mixed result list it could pair two outputs with one call. Signed-off-by: Christian Munley <cmunley@nvidia.com>
Terminus captures one terminal screen per step. Instead of batching the step's calls into one synthetic call, keep each call and pad the missing outputs with empty strings, so every function_call has a matching function_call_output like the other harnesses. Signed-off-by: Christian Munley <cmunley@nvidia.com>
The terminal screen is captured after all of a step's calls run, so it belongs on the last call. The earlier calls get the empty output. Signed-off-by: Christian Munley <cmunley@nvidia.com>
fd67629 to
a724f0c
Compare
|
/ok to test a724f0c |
|
/review |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP — LGTM — no concerns found.
This follows the latest clean bot review, which had no unresolved findings. I inspected both changed files, including the converter’s pairing logic and the new and retained regression assertions. For calls with explicit IDs and one shared observation, the current code emits matching outputs, leaves earlier outputs empty, and places the screen on the last call. Zero-result, single-call, and complete positional-pairing paths remain unchanged. I also traced affected response serialization, training capture, and verifier consumers; no qualifying regression was established.
The previously reviewed tree at fd676291b898c2b3d671bed4931a2c4563fbbe3d is unavailable, so I couldn't independently establish the inter-review delta. Harbor’s pinned trajectory producer is external to the mounted trees, so its field and screen-timing guarantees remain unverified. Static review only; I didn't execute tests or rollouts.
All three delegated inspections completed. Two report payloads were unavailable; I independently inspected their converter and producer/test topics.
Review generated by codex. This was a findings-only review. Request a full review with /review reset=true.
Review fixes for the Terminus 2 agent-session adapter: - A rollout stop during seed, or one that ends the runner before it saves a result, is reported as an empty wall_time run with ng_rollout_stop instead of failing the activation (which was retried). - The run's budget starts once the run has a concurrency slot, so a provider transport timeout after a long queue stays a failure. - Terminus's own tmux commands run from / when a task command removes or moves the workdir, instead of ending the run as a harness error. - Request instructions join Terminus's instruction; tools, tool_choice and parallel_tool_calls stay out of its model calls; a field the converter cannot represent is refused with 422 before the activation starts. - An owned sandbox carries the provider's default metadata and session labels, gets its workdir created at setup, and its default TTL reserves both setup steps. - A runner result that does not validate becomes an observation_capture_failed gap instead of failing the activation or close. - Setup refuses an image without bash. - Add configs/profiles/benchmark.yaml, like the other session harnesses, and register it with the shared profile tests. - README: capability level Evaluation until #3520 lands, supported pairings, request-field rules, terminal lifetime, scoped absolute IP, limits, and the relation to terminus_2_sandboxed_agent. - Tests: the real SandboxSession lifecycle over an in-memory sandbox (stops, close during a run, activation after close), /run refusals, runner max_turns, moved workdir, background process and imports without nemo_gym, and a regression test for each fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
What does this PR do?
Fixes how
terminus_2_agentturns a trajectory step with parallel tool calls into Responses items.When the model issues several commands in one step, the terminal runs them as one batch and returns a single combined observation.
trajectory_to_responsesemitted onefunction_callper command but only onefunction_call_output, paired with the first call, so the other calls had no output.Each call is now kept. The terminal screen is captured after all of a step's calls run, so it pairs with the step's last call, and the earlier calls get an empty
function_call_output. Everyfunction_callthen has a matching output, as in the other harness agents (Claude Code, Codex, OpenCode). Steps with no observation are unchanged.Validation
trajectory_to_responseschecked on 2 calls / 1 result, 1 / 1, 2 / 0 and 2 / 2. Only the first case changes.test_shared_terminal_observation_pairs_every_calladded. ruff check and format clean.Rollouts
10 seeded SWE-bench Verified rows,
anyswerunning terminus_2, a self-hosted DeepSeek V4.1 Flash policy. The two arms differ only interminus_2_agent/output.py(currentmainvs this branch):function_callitemsThree raw rollouts with 3- and 4-command steps read in order: N
bash_commandcalls, then an empty output for each earlier call id, then the screen output for the last call id, one to one. No multi-call step ended withmark_task_complete(0 of 241).Compatibility and benchmark impact
Only steps with several calls and one shared observation change: they gain empty outputs for the calls before the last. No config change. Rewards do not change, since grading reads the task's repo state. Training token capture happens on the model server and does not render
response.output.