feat(cli): add deterministic patch reviews - #655
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0092a8a6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const expected of reviewPublicationEntries) { | ||
| const actual = parsePatchReviewTreeEntry( |
There was a problem hiding this comment.
When a reviewed file changes after the final approval—for example, through a concurrent editor or a post-checkout hook—this comparison runs only after switch, add, and commit, so the unreviewed content is already recorded in a local commit before the error is raised. The command then leaves the user on the new patch branch with a commit that failed review-integrity validation; compare the current entries with the approved entries before creating the branch or commit, while retaining a final post-commit check for races.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
| try { | ||
| await lstat(join(current, ".git")); | ||
| return await realpath(current); |
There was a problem hiding this comment.
When an untrusted scanned directory contains a nested .git file, merely finding that node treats the directory as a repository and later runs git status there. Git explicitly permits such a file to redirect to an arbitrary Git directory (gitrepository-layout), so gitdir: /outside/target/.git causes the review snapshot to read metadata outside the selected repository; an attacker-controlled nested config can also set core.worktree to make status traverse another worktree. Resolve and confine the nested Git directory/worktree before invoking Git, or reject external redirections.
AGENTS.md reference: sdk/typescript/AGENTS.md:L8-L12
Useful? React with 👍 / 👎.
| const paths = listed.split("\0"); | ||
| if (paths.at(-1) === "") paths.pop(); |
There was a problem hiding this comment.
On POSIX, a valid Git filename may contain bytes that are not valid UTF-8, but runPatchReviewGit decodes this ls-files -z output as UTF-8 before it is split and written back as pathspecs. The invalid bytes become U+FFFD, so baseline capture aborts with pathspec ... did not match any files for such repositories. Git 2.43's git add -h describes --pathspec-from-file as reading pathspecs from a file and --pathspec-file-nul as using NUL-separated elements; keep these -z path lists as Buffers end-to-end rather than round-tripping them through strings.
Useful? React with 👍 / 👎.
| ) { | ||
| throw new Error("Repository inspection requires a confined relative path."); | ||
| } | ||
| return normalized.replace(/^\.\//u, "").replace(/\/$/u, ""); |
There was a problem hiding this comment.
When a reviewer passes the conventional root path "." to list_directory or search, treePath(..., true) leaves it as "." instead of converting it to the empty root path. list_directory consequently reports that the path is not a directory, while search silently uses :(top,literal). and returns no matches even when the repository contains the query. Git 2.43's git ls-tree -h describes the trailing operand as a <path>; normalize . to the same root representation used when the optional path is omitted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d83261daa1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const config = configBytes.toString("utf8"); | ||
| if ( | ||
| !Buffer.from(config, "utf8").equals(configBytes) || | ||
| /^\s*\[\s*include(?:if)?(?:\s|\")/imu.test(config) |
There was a problem hiding this comment.
In every supported Node runtime, the u flag makes \" an invalid identity escape in a regular-expression literal. Consequently, the compiled CLI fails during module parsing with SyntaxError: Invalid regular expression before any command can run; match the quote directly as " instead.
Useful? React with 👍 / 👎.
| await runWithTemporaryIndex(["--literal-pathspecs", "add", "--", ...files]); | ||
| const currentEntries = parsePatchReviewIndexEntries( | ||
| Buffer.from( | ||
| await runWithTemporaryIndex(["ls-files", "--stage", "-z", "--", "."]), |
There was a problem hiding this comment.
When a repository's ls-files --stage -z output exceeds Node's default execFile buffer (about 1 MiB), this new whole-index query rejects with a max-buffer error and --create-pr fails before creating the branch. This also affects unreviewed patch publication because the query runs even when reviewPublicationEntries is empty; either request only the paths that need validation or run this Git command without the default buffer limit.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| const removed: Buffer[] = []; | ||
| for (const pathBytes of paths) { | ||
| const key = patchReviewGitPathKey(pathBytes); | ||
| if (ignoredPathSet.has(key)) continue; |
There was a problem hiding this comment.
When a target uses an ignored, local-only AGENTS.md, this unconditional skip removes that instruction file from the immutable baseline. The reviewer runs from the empty temporary review directory with external tools disabled, so it has no other way to discover those applicable instructions and can approve code that violates the very local standards the style stage is supposed to check; convey resolved instruction files to the reviewer while continuing to exclude unrelated ignored content.
Useful? React with 👍 / 👎.
|
@codex review |
…ssessment' into mdangelo/codex/port-patch-risk-assessment
…/codex/refresh-deterministic-patch-reviews
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 568fd5e3e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ef05f7d to
711c48c
Compare
…/codex/refresh-deterministic-patch-reviews
|
@codex review |
8e84947 to
711c48c
Compare
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d619dab66b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ], | ||
| { environment, signal }, | ||
| ); | ||
| const normalizedCandidate = await normalizedPublicationTree(paths); |
There was a problem hiding this comment.
When a candidate path uses .gitattributes working-tree-encoding, the reviewer diff is produced from baselineTree and candidateTree, whose blobs contain raw worktree bytes, while this call separately runs Git normalization and records different objects for automatic publication. With a UTF-16 file, I confirmed that the review delta becomes an opaque GIT binary patch, while the staged publication blob is decoded UTF-8 source; an approval therefore does not cover the representation that --create-pr validates and commits. Generate the review delta from the normalized baseline and candidate trees, or explicitly expose the normalized publication diff to the reviewer.
AGENTS.md reference: sdk/typescript/AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
0fbbed0 to
711c48c
Compare
Merge main, preserve optional risk assessment, validate reviewed publication and usage receipts, and consolidate duplicate review plumbing and fixtures.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1b0270aa0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if any( | ||
| outcome != "hold_for_evidence" for outcome in item["outcomes"].values() | ||
| ) and not critical_unknown_ids <= resolved: | ||
| errors.append( | ||
| "terminal evidence outcomes must resolve every decision-critical unknown" |
There was a problem hiding this comment.
When an assessment has multiple decision-critical unknowns with separate evidence actions, this check rejects every action that has a terminal outcome unless that single action claims to resolve all critical unknowns. For example, two actions that honestly resolve one unknown each are rejected even though their combined resolvesUnknowns covers the complete set; the later aggregate coverage check already models that case. Because the schema permits multiple plan actions and the skill says each action should list the IDs it actually resolves, this makes valid multi-unknown hold_for_evidence assessments impossible to validate without falsifying those relationships.
Useful? React with 👍 / 👎.
| revision_evidence = material_failure or any( | ||
| item["status"] == "failed" for item in validation | ||
| ) |
There was a problem hiding this comment.
When an exact-head check fails for a reason explicitly classified as relevant: false—for example, an unrelated pre-existing formatting failure—this condition still treats it as affirmative evidence that the patch must be revised. The merge branch likewise rejects every failed validation, so an otherwise supported human-review merge cannot be represented, while changing the recommendation to revise is accepted despite there being no patch-related defect. Filter failed checks by relevant just as the auto-merge validation logic already does.
AGENTS.md reference: sdk/typescript/AGENTS.md:L14-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1b0270aa0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| revision_evidence = material_failure or any( | ||
| item["status"] == "failed" for item in validation | ||
| ) |
There was a problem hiding this comment.
When an unrelated check fails and is correctly classified with relevant: false, this expression still treats it as affirmative defect evidence; the merge branch likewise rejects every failed check. This contradicts the risk rubric's gate, which excludes only a “failed relevant check” (risk-rubric.md lines 69-74), and prevents an otherwise supported human_review_required merge—for example, a valid patch accompanied by a failed unrelated formatting job—from producing a valid assessment. Filter failed validation entries by relevant when deriving defect evidence and merge eligibility.
Useful? React with 👍 / 👎.
| if any( | ||
| outcome != "hold_for_evidence" for outcome in item["outcomes"].values() | ||
| ) and not critical_unknown_ids <= resolved: | ||
| errors.append( | ||
| "terminal evidence outcomes must resolve every decision-critical unknown" |
There was a problem hiding this comment.
When an assessment has multiple decision-critical unknowns, an evidence action that resolves only one may legitimately have a terminal defect outcome—for example, supported keeps the recommendation at hold_for_evidence, while contradicted establishes a defect and changes it to revise regardless of the remaining unknown. This condition rejects that plan because any terminal outcome forces the action to list every critical unknown, contrary to the skill contract that each action lists the specific unknown IDs it resolves (SKILL.md lines 29 and 61). Validate collective coverage separately, but do not require an independently decisive action to claim it resolves unrelated unknowns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af2c4fe776
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reviewRepository!.gitExecutable, | ||
| reviewRepository!.repository, | ||
| reviewRepository!.tree, | ||
| reviewRepository!.objectDirectory, |
There was a problem hiding this comment.
When an author turn leaves a subprocess watching the system temporary directory, it can rewrite a loose object beneath objectDirectory while the independent reviewer is running. This line hands that mutable store to the reviewer MCP, and replacing a loose object's compressed contents causes the MCP's git cat-file reads to return the substituted bytes under the existing object ID; the subprocess can then restore the object before the post-review candidate check. This lets an approval cover a forged baseline, so keep the reviewer object store outside the author's writable boundary or authenticate every referenced object before and after the reviewer invocation.
AGENTS.md reference: sdk/typescript/AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
2 participants
Summary
Add optional independent minimality and style reviews to patch-producing CLI workflows. Merge current main, preserve patch-risk assessment, and bind automatic publication to the reviewed candidate.
Changes
--review-minimalityand--review-styletopatchandscan --patch, both off by default. Minimality runs first. Each selected stage allows one revision by default;--max-review-revisions Nsets a shared nonnegative revision budget. Later-stage revisions restart earlier reviews.0.1.81and use the existing async child-process pattern for the failing Windows JUnit fixture. Retain the inherited patch-risk contract improvements, which overlap feat(plugin): strengthen patch-risk falsifiers #658.Testing
bun test --timeout 30000 --seed 655over CLI patch/skills/schema, patch-risk contract, runtime, and JUnit-report tests: 489 passed, 14 platform skips, zero failures.Failing-first regressions for filter handling, reviewer context, symlink representation, cost receipts, post-review publication changes, raw inspection bytes, and normalized publication diffs now pass.
Real bundled Codex cache upgrades from
0.1.37and0.1.60replace stale validator content and preserve credentials.Types/generated-model freshness, formatting, build, Ruff, Python 3.10 grammar, and
git diff --check: passed.Both built Node inspection entrypoints, 286-entry package validation, and full installed-package smoke: passed.
Native Windows/Linux, live model quality, and current-head CI were not run locally. CI is intentionally left for the second pass.
Final main refresh (
fd98a90): package 0.1.21 includes the MCP launcher-permission fix; SDK implementation and bundled payload are unchanged. Types/model generation, formatting, build, 28 focused tests, static artifact verification and full installed-package smoke passed, including MCP initialization. CI was not awaited.Risk and rollout
Reviews are opt-in and add model calls and latency. Unreviewed workflows retain their existing behavior. Reviewed publication fails when the candidate cannot be separated from pre-existing changes. The inherited filesystem and Git-state machinery remains substantial; this pass does not resolve every open review discussion or establish merge readiness. The Windows fixture change passed locally but still needs native CI verification.
Public disclosure review
The second attestation remains unchecked because historical commit contact metadata and automated restricted settings links remain in the existing PR history. New examples and fixtures are synthetic; the new commit uses GitHub noreply metadata.