Skip to content

fix(codex): classify get_goal read statuses as successful dynamic tool calls#98659

Merged
vincentkoc merged 1 commit into
openclaw:mainfrom
yetval:fix/codex-get-goal-status
Jul 1, 2026
Merged

fix(codex): classify get_goal read statuses as successful dynamic tool calls#98659
vincentkoc merged 1 commit into
openclaw:mainfrom
yetval:fix/codex-get-goal-status

Conversation

@yetval

@yetval yetval commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where users running the Codex harness with thread goals enabled would see every get_goal call reported to the model as a failed tool call, even though the read succeeded. On every call the tool ran and returned the current goal (or reported that no goal was set), yet the model was told the read failed. The failure was also persisted on the transcript with isError: true and drove the diagnostic terminal type to error, so it polluted session history, diagnostics, and any error-gated heuristics.

Why This Change Was Made

Root cause: isCodexToolResultError (extensions/codex/src/app-server/dynamic-tools.ts:1159) fail-closes. Any dynamic-tool result whose details.status is not in its non-error allowlist is classified as an error.

// before
  const status = details.status.trim().toLowerCase();
  return (
    status !== "" &&
    status !== "0" &&
    status !== "ok" &&
    status !== "success" &&
    status !== "completed" &&
    status !== "recorded" &&
    status !== "created" &&
    status !== "updated" &&
    status !== "accepted" &&
    status !== "pending" &&
    status !== "started" &&
    status !== "running" &&
    status !== "yielded"
  );

get_goal returns jsonResult(snapshot) (src/agents/tools/goal-tools.ts:87), and getSessionGoal sets details.status to "found" (a goal exists) or "missing" (no goal) (src/config/sessions/goals.ts:170, :189). Neither value is in the allowlist, so the classifier returns true, the bridge sends success: false to Codex, and the result is persisted with isError: true.

The fix adds the two read-side statuses to the same non-error allowlist, next to the write-side statuses their sibling added:

// after
    status !== "created" &&
    status !== "updated" &&
    status !== "accepted" &&
    status !== "found" &&
    status !== "missing" &&
    status !== "pending" &&

found/missing are the only two statuses get_goal can emit, and both represent a successful read. Genuinely failed statuses (error, forbidden, and the rest) remain fail-closed.

This is the right boundary: isCodexToolResultError is the single owner of the Codex dynamic-tool error/success classification, and this matches the boundary that merged sibling #96856 (b3ff64145e) already chose for the write-side goal statuses. That PR added created/updated (goal writes) and accepted (spawn) but missed the read-side get_goal statuses, so current main still carries the defect. The Codex contract confirms success is load-bearing: OpenClaw's DynamicToolResponse.success is consumed by Codex at codex-rs/core/src/tools/handlers/dynamic.rs:124 and mapped into FunctionToolOutput::from_content(body, Some(success)), which becomes FunctionCallOutputPayload.success (codex-rs/protocol/src/models.rs:1384) and the DynamicToolCallResponse event, so success: false marks the tool call failed to the model.

User Impact

Codex-harness users with goals enabled now get an accurate signal: reading the thread goal is reported to the model as a successful tool call instead of a failure on every call. This stops false failures from polluting session history and diagnostics and from feeding error-gated heuristics. Genuinely failed goal reads are unaffected.

Evidence

Real runtime, before and after. Drove the real Codex dynamic-tool bridge (createCodexDynamicToolBridge().handleToolCall) with the real get_goal tool (createGetGoalTool) against a real on-disk session store, on pristine main f936c6b vs the patched tree, identical inputs. The classifier, the goal tool, the session store read/write, and the persisted-result callback all stayed real; nothing external was mocked.

Steps: seed a temp sessions.json, invoke the bridge once with no goal present, create a real goal via createSessionGoal, then invoke the bridge again; print details.status, the response success, and the persisted isError for each call.

# BEFORE (pristine main f936c6b495)
missing-goal: details.status=missing success=false persisted.isError=true
found-goal: details.status=found success=false persisted.isError=true
# AFTER (this patch, identical inputs)
missing-goal: details.status=missing success=true persisted.isError=false
found-goal: details.status=found success=true persisted.isError=false

Both a present goal and an absent goal now return the read as a successful tool call to the harness instead of a failure, and the persisted result flag flips from failed to ok. Genuinely failed statuses are unchanged.

Checks:

  • node scripts/run-vitest.mjs extensions/codex/src/app-server/dynamic-tools.test.ts: 81 passed with the patch; the new get_goal case fails on pristine main and passes with the fix.
  • node scripts/run-oxlint.mjs and oxfmt --check on both changed files: clean.
  • node scripts/run-tsgo.mjs -p tsconfig.core.json and -p test/tsconfig/tsconfig.core.test.json: clean.

Not covered: no live Codex subprocess round-trip or live OpenAI provider request; the Codex-side mapping of success to the failed item status is established from the pinned codex-rs contract rather than a live run. Full build not run.

Real behavior proof

Behavior addressed: under the Codex harness with goals enabled, every successful get_goal read is reported to the model as a failed tool call (success false, persisted isError true).
Real environment tested: the real Codex dynamic-tool bridge (createCodexDynamicToolBridge().handleToolCall) driving the real get_goal tool (createGetGoalTool) against a real on-disk session store, on pristine main f936c6b and on the patched tree; the classifier, goal tool, session store, and persisted-result callback stayed real, nothing external mocked.
Exact steps or command run after this patch: seed a temp sessions.json, invoke the bridge with no goal present, create a real goal via createSessionGoal, invoke the bridge again, print details.status plus response success plus persisted isError.
Evidence after fix:

# BEFORE (pristine main f936c6b495)
missing-goal: details.status=missing success=false persisted.isError=true
found-goal: details.status=found success=false persisted.isError=true
# AFTER (this patch, identical inputs)
missing-goal: details.status=missing success=true persisted.isError=false
found-goal: details.status=found success=true persisted.isError=false

Observed result after fix: both a present goal and an absent goal now return the read as a successful tool call to the harness instead of a failure, with the persisted result flag flipped from failed to ok; genuinely failed statuses are unchanged.
What was not tested: no live Codex subprocess round-trip or live provider request; the Codex-side mapping of success to the failed item status comes from the pinned codex-rs contract rather than a live run; full build not run.

@yetval
fix(codex): classify get_goal read statuses as successful dynamic too…
63472ea 
…l calls

isCodexToolResultError fail-closes any dynamic-tool result whose
details.status is absent from its non-error allowlist. get_goal returns
details.status "found" or "missing" (a successful read of the thread
goal, or its absence), neither of which was in the allowlist, so every
get_goal call was classified as an error: reported to codex as
success: false and persisted on the transcript with isError: true.

Sibling openclaw#96856 added the write-side goal statuses (created/updated) and
the accepted spawn status but missed the read-side get_goal statuses.
This adds found/missing alongside them. Genuinely failed statuses stay
fail-closed.
@clawsweeper

clawsweeper Bot commented Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 1, 2026, 10:38 AM ET / 14:38 UTC.

Summary
The PR adds found and missing to the Codex dynamic-tool non-error status allowlist and adds a regression test for get_goal found/missing results.

PR surface: Source +2, Tests +47. Total +49 across 2 files.

Reproducibility: yes. Source inspection shows current main get_goal returns details.status as found or missing, while the Codex bridge fail-closes non-empty statuses absent from its success allowlist.

Review metrics: 1 noteworthy metric.

  • Dynamic read success statuses: 2 added. The Codex classifier is tool-name-agnostic, so maintainers should notice exactly which status strings become successful before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Next step before merge

  • No automated repair lane is needed because the reviewed PR already supplies the narrow code change and test coverage.

Security
Cleared: The diff changes only Codex dynamic-tool classification logic and a colocated test, with no dependency, workflow, auth, secret, permission, package, or migration surface touched.

Review details

Best possible solution:

Land the narrow Codex bridge classifier fix with focused regression coverage if the current head checks remain green.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows current main get_goal returns details.status as found or missing, while the Codex bridge fail-closes non-empty statuses absent from its success allowlist.

Is this the best way to solve the issue?

Yes. Updating the Codex dynamic-tool classifier is the narrow origin fix because that function directly drives the response success value and transcript isError; downstream compaction or docs changes would not correct the bridge output.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against ce4a259485f2.

Label changes

Label changes:

  • add P2: This fixes a concrete Codex runtime transcript/session-state correctness bug with limited blast radius and focused coverage.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes before/after live output from the real bridge and real get_goal tool against an on-disk session store, showing success and persisted isError flip for both missing and found goals.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes before/after live output from the real bridge and real get_goal tool against an on-disk session store, showing success and persisted isError flip for both missing and found goals.

Label justifications:

  • P2: This fixes a concrete Codex runtime transcript/session-state correctness bug with limited blast radius and focused coverage.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes before/after live output from the real bridge and real get_goal tool against an on-disk session store, showing success and persisted isError flip for both missing and found goals.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes before/after live output from the real bridge and real get_goal tool against an on-disk session store, showing success and persisted isError flip for both missing and found goals.
Evidence reviewed

PR surface:

Source +2, Tests +47. Total +49 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 2 0 +2
Tests 1 47 0 +47
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 49 0 +49

What I checked:

Likely related people:

  • steipete: GitHub commit history shows the session-goal tools and session-goal storage were introduced in the core session goals change. (role: feature introducer; confidence: high; commits: a509c48f0ea2; files: src/agents/tools/goal-tools.ts, src/config/sessions/goals.ts, src/agents/openclaw-tools.ts)
  • vincentkoc: GitHub commit history shows recent work on the Codex app-server dynamic-tool protocol and session helper refactors touching this behavior boundary. (role: recent area contributor; confidence: high; commits: ab1e5832d2fb, 0da706dbfb10; files: extensions/codex/src/app-server/dynamic-tools.ts, extensions/codex/src/app-server/protocol.ts, src/config/sessions/goals.ts)
  • nxmxbbd: The merged sibling PR added accepted/created/updated to the same Codex dynamic-tool success allowlist and added adjacent regression coverage. (role: recent sibling-fix contributor; confidence: medium; commits: b3ff64145ea5; files: extensions/codex/src/app-server/dynamic-tools.ts, extensions/codex/src/app-server/dynamic-tools.test.ts)
  • omarshahine: Recent GitHub path history shows work on Codex message-tool-only source reply completion in the same dynamic-tool bridge file. (role: recent adjacent contributor; confidence: medium; commits: 9b9a124cc520; files: extensions/codex/src/app-server/dynamic-tools.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels Jul 1, 2026
@vincentkoc
vincentkoc merged commit adcfebc into openclaw:main Jul 1, 2026
132 of 141 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 1, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 2, 2026
@yetval
fix(codex): classify get_goal read statuses as successful dynamic too…
f08a565 
…l calls (openclaw#98659)

isCodexToolResultError fail-closes any dynamic-tool result whose
details.status is absent from its non-error allowlist. get_goal returns
details.status "found" or "missing" (a successful read of the thread
goal, or its absence), neither of which was in the allowlist, so every
get_goal call was classified as an error: reported to codex as
success: false and persisted on the transcript with isError: true.

Sibling openclaw#96856 added the write-side goal statuses (created/updated) and
the accepted spawn status but missed the read-side get_goal statuses.
This adds found/missing alongside them. Genuinely failed statuses stay
fail-closed.
chenyangjun-xy pushed a commit to chenyangjun-xy/openclaw that referenced this pull request Jul 3, 2026
@yetval @chenyangjun-xy
fix(codex): classify get_goal read statuses as successful dynamic too…
0f3d308 
…l calls (openclaw#98659)

isCodexToolResultError fail-closes any dynamic-tool result whose
details.status is absent from its non-error allowlist. get_goal returns
details.status "found" or "missing" (a successful read of the thread
goal, or its absence), neither of which was in the allowlist, so every
get_goal call was classified as an error: reported to codex as
success: false and persisted on the transcript with isError: true.

Sibling openclaw#96856 added the write-side goal statuses (created/updated) and
the accepted spawn status but missed the read-side get_goal statuses.
This adds found/missing alongside them. Genuinely failed statuses stay
fail-closed.
@vincentkoc vincentkoc mentioned this pull request Jul 6, 2026
Merged
sheyanmin pushed a commit to sheyanmin/openclaw that referenced this pull request Jul 8, 2026
@yetval @sheyanmin
fix(codex): classify get_goal read statuses as successful dynamic too…
80983f4 
…l calls (openclaw#98659)

isCodexToolResultError fail-closes any dynamic-tool result whose
details.status is absent from its non-error allowlist. get_goal returns
details.status "found" or "missing" (a successful read of the thread
goal, or its absence), neither of which was in the allowlist, so every
get_goal call was classified as an error: reported to codex as
success: false and persisted on the transcript with isError: true.

Sibling openclaw#96856 added the write-side goal statuses (created/updated) and
the accepted spawn status but missed the read-side get_goal statuses.
This adds found/missing alongside them. Genuinely failed statuses stay
fail-closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

extensions: codex P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yetval @vincentkoc