Skip to content

Honor global reasoning defaults#618

Merged
wesm merged 6 commits intoroborev-dev:mainfrom
cpcloud:investigate-codex-maximum-xhigh
Apr 3, 2026
Merged

Honor global reasoning defaults#618
wesm merged 6 commits intoroborev-dev:mainfrom
cpcloud:investigate-codex-maximum-xhigh

Conversation

@cpcloud
Copy link
Copy Markdown
Contributor

@cpcloud cpcloud commented Apr 2, 2026

Summary

  • add global config fields for review, refine, and fix reasoning defaults
  • resolve reasoning as explicit -> repo -> global -> default
  • add config and daemon regression coverage for global review_reasoning

Testing

  • go test ./...
  • go vet ./...

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 2, 2026

roborev: Combined Review (a32bbf1)

Verdict: Changes are mostly sound, but there is one medium-severity configuration error-handling regression that should be addressed.

Medium

  • Malformed or unreadable global config errors are silently ignored, causing reasoning resolution to fall back to hard-coded defaults instead of surfacing the configuration problem. This differs from the existing repo-level config behavior and can hide real user misconfiguration.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 3, 2026

roborev: Combined Review (2db776a)

Verdict: One medium-severity issue should be addressed before merge.

Medium

  • cmd/roborev/refine.go:350: config.LoadGlobal() still has its error ignored, but this change now relies on the returned global config for refine_reasoning. If the global config file is malformed or unreadable, roborev refine will silently fall back to defaults instead of surfacing the configuration error.
    Suggested fix: Handle the LoadGlobal() error here consistently with the other workflows and return a user-facing error before resolving reasoning.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@cpcloud
Copy link
Copy Markdown
Contributor Author

cpcloud commented Apr 3, 2026

Addressed in 3526b5f. roborev refine now propagates LoadGlobal() failures instead of silently falling back when the global config is malformed or unreadable, and there is regression coverage in cmd/roborev/main_test.go for the invalid-global-config path.

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 3, 2026

roborev: Combined Review (3526b5f)

Verdict: No Medium, High, or Critical findings identified across the provided reviews.

The reviewed change appears clean based on the available agent outputs.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 3, 2026

roborev: Combined Review (3526b5f)

Verdict: No Medium, High, or Critical issues identified in this PR.

All reviewed outputs that contained findings agreed the change is clean. No report surfaced any Medium, High, or Critical concerns.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 3, 2026

roborev: Combined Review (6584a53)

Verdict: No Medium, High, or Critical issues found.

All reviewed outputs agree the changes are clean. The reasoning-resolution updates and config error handling do not appear to introduce regressions or security issues based on the reviewed diffs.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm
Copy link
Copy Markdown
Collaborator

wesm commented Apr 3, 2026

LGTM

@wesm wesm merged commit d8b5d0d into roborev-dev:main Apr 3, 2026
8 checks passed
wesm pushed a commit that referenced this pull request Apr 3, 2026
## Why This PR Exists
PR #618 ("Honor global reasoning defaults") merged first, but the
original working branch still had a set of review-driven follow-up fixes
that landed after the squash merge. Those fixes never reached `main`.

This PR ports that missing follow-up series onto current `main` without
dragging the old pre-merge branch history forward.

## Motivation
The merge of #618 fixed the top-level global reasoning default issue,
but it left a set of related edge cases and regressions unresolved:
- malformed global and repo config could still be surfaced too late or
in the wrong places
- explicit reasoning handling could still interact incorrectly with
repo-config validation
- background fix jobs and reruns could resolve config from stale or
invalid worktree paths
- CI review fallback had been widened too far and could mask unexpected
config-load failures instead of only malformed config

The net effect was that reasoning/config behavior was still inconsistent
across refine, enqueue, fix, rerun, batch, backup-resolution, and CI
paths.

## What This Changes
- validates repo config at the intended CLI and daemon entrypoints
instead of letting explicit reasoning or shared workflow resolution
bypass those errors
- preserves explicit reasoning overrides while still surfacing invalid
repo/global config when that config must be consulted
- makes background fix jobs honor the active fix reasoning/model config
- keeps batch, CI, and backup-resolution behavior consistent when repo
config is malformed
- validates fix-job worktree paths before using them for config
resolution
- clears stale worktree metadata so reruns do not reintroduce invalid
config resolution later
- narrows CI fallback so only malformed repo config falls back to
global/default settings; unexpected config-load failures still fail fast
- carries the Windows test cleanup needed for the enqueue reasoning test

## Testing
- `go test ./...`
- `go vet ./...`

## Context
This is a direct follow-up to #618, created because the refined fixes
were still on the original branch after #618 had already merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants