fix(agent): use native tools for workspace tasks#4969
Conversation
…tool-calling # Conflicts: # docker/entrypoint.sh # src/agent_loop.py # src/tool_execution.py # static/js/chat.js # static/js/chatRenderer.js # tests/test_agent_loop.py # tests/test_chat_helpers.py # tests/test_llm_core_temperature_reasoning.py
|
The workspace shell-write guard (
Flip side: it's also trivially under-inclusive as a guard — the Reliably separating redirects from comparisons/quoted text really needs tokenization (shlex) rather than a regex over the raw string. If you want to keep it lightweight, at minimum special-case (Separately, this PR is bundling several independent things — run records, ChatGPT-subscription tools, the workspace guard — which will make it hard to land; might be worth splitting the guard out on its own.) |
|
Good catch. I pushed The guard now allows read-only diagnostics like awk/jq comparisons, quoted I also added regression coverage for the examples you listed plus fd-prefixed redirects. Validation: Focused Docker runs passed: |
|
Thanks for the detailed validation and follow-up on the workspace shell-write guard. I started maintainer audit, but this is currently blocked by a real merge conflict against current
The conflict appears to be from current Expected rebase intent:
Once the branch is clean against current |
|
Resolved in 541727e by merging current dev into this branch and resolving the parser/workspace test conflicts. I kept the #3681 fenced-parser invariant intact: same-line bash/python metadata remains inert, non-dispatching fences stay visible, and only actual tool calls are stripped. This PR's zero-arg fenced get_workspace path is preserved and now has parse/strip regression coverage. I also kept the workspace confinement/shell-guard tests alongside the incoming dev glob coverage.\n\nValidation run in Docker:\n- docker compose build odysseus\n- python -m pytest tests/test_fenced_inline_args.py tests/test_fenced_invoke_no_raw_xml.py tests/test_workspace_confine.py tests/test_tool_path_confinement.py tests/test_agent_rounds_exhausted.py -q\n\nResult: 112 passed, 2 dependency deprecation warnings. |
alteixeira20
left a comment
There was a problem hiding this comment.
Thanks for rebasing this and resolving the parser conflict. The previous conflict blocker is gone, GitHub checks are green, local merge probe is clean, and the focused validation is strong.
I am requesting changes because the new durable agent-run record path appears internally inconsistent.
The new AgentRunRecord model and agent_run_records.finish() support these fields:
partial_chars
assistant_message_id
latest_for_session() also exposes partial_chars, and the unit test verifies that finish(..., partial_chars=42, assistant_message_id="assistant-msg-1") persists those values.
However, the real detached-run manager path in src/agent_runs.py currently calls:
agent_run_records.finish(
run.record_id,
status=run.status,
stop_reason=run.stop_reason,
error=run.error,
event_count=len(run.buffer),
last_event_type=_last_event_type(run.buffer[-1]) if run.buffer else "",
)
So real detached runs never pass partial_chars or assistant_message_id.
I verified this with a direct probe against the PR worktree. The actual finish payload was:
{'status': 'done', 'stop_reason': '', 'error': '', 'event_count': 4, 'last_event_type': 'done'}
and the probe reported:
MISSING_DURABLE_FIELDS: partial_chars, assistant_message_id
Please either:
- populate these fields on the real detached-run path, ideally by deriving
assistant_message_idfrom the streamedmessage_savedevent and computing a meaningfulpartial_chars, with integration coverage foragent_runs.start(...); or - remove/de-scope those fields from the durable run contract/API/tests if they are intentionally unused.
Given this PR adds a DB table and user-visible durable run/resume behavior, the stored lifecycle record should not expose fields that are only populated in unit-level direct calls but never by the production path.
|
Addressed in 1510c7f. The detached-run production path now derives durable finish fields from the actual buffered SSE stream before calling agent_run_records.finish():\n\n- assistant_message_id is captured from the streamed message_saved event.\n- partial_chars is computed from non-thinking visible delta chunks, with agent_final.text as a fallback when no deltas were emitted.\n- latest_for_session now includes assistant_message_id alongside partial_chars.\n\nI also updated the real agent_runs.start(...) regression coverage so the detached-run manager test now fails if those fields are omitted, and added coverage for the agent_final fallback path.\n\nValidation after Docker rebuild:\n\n |
|
Resolved the new Conflict files resolved:
Resolution notes:
Local validation: Focused pytest result: GitHub now reports the head as |
|
Conflict resolution update for #4969: I traced the conflict source to the
I resolved this by merging current
Resolution notes:
Validation:
GitHub now reports the PR as |
|
The shlex rewrite sorted out the redirect false-positives from before — thanks for that. Noticed one sibling gap in the same guard though: Over-blocks a few read-only commands (which is the thing the guard's own message says it won't do): And it under-blocks the opposite way, since it only anchors on You've already got |
|
Thanks, good catch. Addressed in The guard now uses Added regression coverage for the cases you called out:
Validation: Focused pytest result: |
Summary
Fixes Agent workspace file tasks for ChatGPT Subscription runs by preserving native Responses function-call/tool-output turns, enabling tool support for the subscription endpoint, and keeping process details behind the same disclosure-style UI used for thinking output.
Target branch
dev, notmain. All PRs land indev;mainis curated by the maintainer at each release. If your PR is onmainby accident, click "Edit" on this PR and change the base.Linked Issue
Fixes #4968
Type of Change
Checklist
devdocker compose uporuvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough.How to Test
http://127.0.0.1:7000.Read the README in the workspace and tell me exactly what it says.Confirm the agent calls workspace tools and returns a clean final answer with process details collapsed.Create agent_ui_native_tool_test.txt in the workspace with one line: native UI tools worked.Confirm the file is written and the final answer reports success after tool evidence.List the workspace files and confirm whether agent_ui_native_tool_test.txt exists.Confirm the file appears in the listing and the visible final answer stays separate from process details.Validation run locally after resolving conflicts:
The focused pytest runs were executed in the Odysseus Docker image with the working tree mounted and passed:
155 passed, 1 warningand6 passed, 1 warning.Manual Agent smoke tests were run with both
gpt-5.5andgpt-5.3-codex-sparkagainst the mounted workspace. The UI smoke was also run withgpt-5.3-codex-sparkon the local Docker app.Visual / UI changes - REQUIRED if you touched anything that renders
Anything that changes what the UI looks like - buttons, icons, padding, colors, fonts, spacing, layout, CSS, HTML, SVG, or any
static/js/module that draws to the DOM - needs all of the following. PRs that change rendering without these WILL be closed.--red,--fg,--bg,--card,--border, etc.) - do not introduce new color values, font sizes, or spacing units.static/index.html) or plain text.Fira Code) for primary UI text. Don't override.Screenshots / clips
Agent README read - process collapsed
Agent workspace write - process collapsed
Agent workspace list/confirm - process collapsed