Skip to content

fix: validate configured model reasoning effort - #473

Merged
andrei-hasna merged 2 commits into
mainfrom
factory/task-95b9bbb2714f-5a350587
Aug 1, 2026
Merged

fix: validate configured model reasoning effort#473
andrei-hasna merged 2 commits into
mainfrom
factory/task-95b9bbb2714f-5a350587

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Fixes #428.

Factory run: run_ef88c48d5ce8. Factory produced the two-file candidate and internal reviewer returned GO. Factory PR creation parked because install is pre-existing red at base 14ece82: npm install runs sdk/typescript postinstall and fails because pnpm is unavailable.

Local/manual landing evidence before PR:

  • Factory evidence: candidate files codex-rs/core/src/session/mod.rs and codex-rs/core/src/session/tests.rs; internal adversarial reviewer GO.
  • Staged secrets scan before commit: gitleaks positive control detected a synthetic secret, then staged scan found no leaks.
  • Push scan: gitleaks positive control detected a synthetic secret, then origin/main..HEAD scan found no leaks.
  • Commit trailer: Agent: Silvanus.

Required follow-up before merge: independent PR-head adversarial review and CI/head checks.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO 3a53bdf

Blocking finding:

  • Required CI/format gate is red at this exact head. ci / build-test failed in just fmt-check, and rust-ci / Format / etc failed in the cargo fmt step. The log shows rustfmt diffs in codex-rs/core/src/session/tests.rs around the new tests (model_reasoning_effort_rejects_ultra_alias_when_max_is_not_advertised and model_reasoning_effort_allows_custom_value_without_model_metadata). This is a concrete required merge gate failure, so this head is not mergeable as-is.

Functional review notes:

  • The validator added in codex-rs/core/src/session/mod.rs checks configured model_reasoning_effort during session startup after resolving ModelInfo; it rejects unadvertised values when supported_reasoning_levels is non-empty and preserves empty-metadata compatibility. That matches the issue's fast-fail/mapping intent without regressing provider-defined custom effort compatibility.
  • Base main already maps "ultra" to wire value "max"; this PR's tests verify that ultra/max is accepted when advertised and rejected clearly when not advertised.
  • Commit metadata is acceptable: author/committer Andrei Hasna <andrei@hasna.com>, one Agent: Silvanus trailer, no Co-Authored-By trailer observed.

Verification status:

  • I did not run local Rust/Bazel checks because the requested review notes local Rust/Bazel tooling is unavailable. Remote CI is still in progress for Bazel/Rust jobs and already has the formatter failure above; CI/head checks must be green after formatting before merge.

Remediates PR #473 review formatting blocker.

Agent: Silvanus
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO 56f7abd

Focused re-review of remediation cycle 1 only: the prior formatting blocker, the 3a53bdfd8d14dae9746e7876b56bae55a5d6c796..56f7abdab6fd092392b453feebace8640b0a67dc remediation diff, and direct regressions from that fix.

Findings:

  • The remediation diff is formatting-only in codex-rs/core/src/session/tests.rs: 4 additions / 3 deletions changing line wrapping around the two tests named in the original formatter failure. I saw no logic change or direct regression from the formatting fix.
  • The two named blocker gates now pass at this head: ci / build-test completed SUCCESS, and rust-ci / Format / etc completed SUCCESS with cargo fmt SUCCESS.
  • Trailer sanity check still passes for the branch commits: each has Agent: Silvanus; no Co-Authored-By trailer observed.

Residual status outside this focused re-review: other CI jobs remain in progress, and this GO does not waive those merge gates.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #473 @ 56f7abd — lens: correctness+security+gates, reviewer unresolved-account002 (1 of 1)

Reviewed exact candidate

  • Confirmed HEAD 56f7abdab6fd092392b453feebace8640b0a67dc and fetched origin/main 3fed112074002b0682d6152e05df1e26f21cc479.
  • git log --oneline origin/main..HEAD — exit 0; two commits read.
  • git diff origin/main...HEAD --stat — exit 0; 2 files, 110 insertions.
  • Full diff for both changed files — exit 0; surrounding session startup, reasoning parsing/serialization, model-manager resolution/fallback, turn-context, app-server error propagation, and issue reasoning_effort='ultra' causes opaque 400 on every turn (no validation, burns all retry attempts) #428 paths read.
  • The validation runs after auth/provider model resolution, compares the configured wire value with the resolved model metadata, accepts the normalized ultra -> max alias only when advertised, and preserves provider-defined efforts when no supported-effort metadata is advertised.

Commands and declared gates

  • bun install — exit 0; setup only, 521 packages installed. This is not reported as the repository test result. The generated untracked bun.lock setup artifact was removed; the worktree is clean.
  • package.json scripts read with jq — exit 0. The repository declares no typecheck script.
  • The repository declares no test script and therefore no package-level test gate. This absence is a finding in its own right; no replacement gate was invented.
  • git diff --check origin/main...HEAD — exit 0. THE LANE'S CHOSEN COMMAND, not a repository gate and not used as a blocker.
  • gh pr view 473 --repo hasna/codewith --json statusCheckRollup — exit 0; 23 SUCCESS, 0 failure, 3 SKIPPED, 2 IN_PROGRESS at verdict time.

Forge checks read by name

  • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-gnu — IN_PROGRESS; no conclusion yet.
  • cla — SUCCESS.
  • Check for spelling errors — SUCCESS.
  • Blob size policy — SUCCESS.
  • cargo-deny — SUCCESS.
  • build-test — SUCCESS.
  • Build Codewith Linux CLI — SUCCESS.
  • Detect changed areas — SUCCESS.
  • python-sdk — SUCCESS.
  • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-musl — IN_PROGRESS; no conclusion yet.
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 1/4 — SUCCESS.
  • Format / etc — SUCCESS.
  • sdks — SUCCESS.
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 2/4 — SUCCESS.
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 3/4 — SUCCESS.
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 4/4 — SUCCESS.
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm (native main) — SKIPPED.
  • cargo shear — SUCCESS.
  • Argument comment lint package — SKIPPED.
  • Bazel clippy on ubuntu-24.04 for x86_64-unknown-linux-gnu — SUCCESS.
  • Bazel clippy on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS.
  • Verify release build on ubuntu-24.04 for x86_64-unknown-linux-gnu — SUCCESS.
  • Argument comment lint - Linux — SUCCESS.
  • Verify release build on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS.
  • Argument comment lint - Windows — SUCCESS.
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS.
  • CI results (required) — SUCCESS.
  • [code]smith — SKIPPED.

The two in-progress Linux full-matrix Bazel test jobs are recorded honestly and are non-blocking under this repository's explicit policy that the full Rust/Bazel matrix is a confidence backstop rather than the immediate gate for a focused Codewith fix. The declared CI results (required) aggregate, builds, release builds, clippy, formatting, dependency checks, and completed platform tests are green.

Blocking P0/P1 findings

  • None.

Non-blocking follow-ups

  • Repository-level gap: package.json declares no test gate (and no typecheck script). No code defect in this PR is evidenced by that absence.
  • No additional code follow-up identified. The remaining Linux Bazel backstop jobs may continue post-disposition per repository policy.

@andrei-hasna
andrei-hasna merged commit cab48d5 into main Aug 1, 2026
28 checks passed
@andrei-hasna
andrei-hasna deleted the factory/task-95b9bbb2714f-5a350587 branch August 1, 2026 18:40
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 1, 2026
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW-GATES] #473 @ 56f7abd — final forge update

Both Linux Bazel backstop jobs that were still in progress in the GO verdict subsequently completed SUCCESS:

  • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-musl — SUCCESS.
  • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-gnu — SUCCESS.

Fresh gh pr view 473 --repo hasna/codewith --json statusCheckRollup — exit 0; 0 failures and 0 pending checks. The PR was squash-merged successfully as cab48d5bc70255eea0952dd0f7bfd18fefe91169, and the remote head branch was deleted.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

reasoning_effort='ultra' causes opaque 400 on every turn (no validation, burns all retry attempts)

1 participant