fix: distinguish install/dual-solve/crystallize failures from success#402
Open
dripsmvcp wants to merge 1 commit into
Open
fix: distinguish install/dual-solve/crystallize failures from success#402dripsmvcp wants to merge 1 commit into
dripsmvcp wants to merge 1 commit into
Conversation
five fixes to code merged on the test branch, all variants of "a failure or not-yet-ready state reported as success": - install-mcp (codex toml_merge): a config.toml the minimal serializer can't round-trip (non-bmp string value, nan/inf float) was bucketed as skipped and printed "(already present)" with a clean "Done", so the user believed vouch was wired into codex when it wasn't. add a distinct `failed` bucket, report it, and exit non-zero. - install-mcp: reject a manifest `dst` that escapes the target tree (`..`/absolute) instead of writing outside it — defense in depth for the manifest file writer. shipped adapters use only contained relative paths. - dual-solve review-ui: `_serialize` computed the recommendation over the empty candidate list on every poll, so a running job showed "neither engine produced a usable diff" for its whole duration. omit the hint until candidates exist. - session.crystallize: retrying on a not-yet-ended session rewrote the summary page with a fresh wall-clock "Ended:" stamp each time (and re-embedded), so vouchdev#256's "idempotent retry" wasn't. render a stable marker for an open session. - capture ingest-codex: cap rollout parsing at 64 MiB so an oversized or newline-free-blob rollout can't be read whole into memory, matching the byte caps on other untrusted reads. each fix has a regression test; make check green.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
five small fixes to code that landed on
test, all the same shape — afailure or not-yet-ready state that was being reported as success. found
while auditing the branch; each has a regression test and none touches the
review gate.
install-mcp (codex
toml_merge) — aconfig.tomlthe minimal serializercan't round-trip (a non-bmp string value like an emoji, a
nan/inffloat)was bucketed as
skippedand printed(already present)with a cleanDone,so the user believes vouch is wired into codex when it isn't. serializer
failure now lands in a distinct
failedbucket, is reported as such, and thecommand exits non-zero. reproduced end-to-end: an existing config with an
emoji string value → the
[mcp_servers.vouch]block silently never installs.install-mcp path containment — a manifest
dstthat escapes the targettree (
../absolute) is now refused with anAdapterErrorinstead of writingoutside
target. defense in depth for the manifest file writer; every shippedadapter uses contained relative paths, so no behavior change for real installs.
dual-solve review-ui —
_serializecomputed the recommendation over theempty candidate list on every poll, so a running job rendered "neither engine
produced a usable diff" for its entire (multi-minute) duration. the hint is now
omitted until candidates exist.
session.crystallize — retrying on a session that hasn't been ended rewrote
the
session-<id>summary page with a fresh wall-clockEnded:stamp eachtime (and re-embedded it), so #256's "idempotent retry" wasn't idempotent. an
open session now renders a stable marker; retries produce an identical body.
capture ingest-codex — rollout parsing had no size cap and would read an
oversized (or newline-free-blob) rollout whole into memory. bounded to 64 MiB
up front, matching the byte caps on other untrusted reads.
make checkgreen — pytest (full, excl. embeddings), mypy, ruff.