From a0f8a32595110236d2d40ccd49031b960e16742a Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Tue, 11 Aug 2026 13:53:59 -0500 Subject: [PATCH] codex: retain topic approvals and close published reviews Topic pull requests are review-only, but plan admission currently requires an independent approval at the exact current source commit. Every subsequent push therefore forces the reviewer to approve again, even though GitHub still considers the earlier review effective. Retain the latest effective approval from an independent repository writer while continuing to freeze the exact source admitted into the plan. Keep the stricter exact-head check for legacy merge admission. After either publication path succeeds, inspect trusted plan history and close only topic pull requests whose current head matches the published source, generated output, and live repository refs. Leave source refs untouched, and make closure failures non-fatal after an otherwise successful publication. --- .github/CODEX.md | 42 +++++---- .github/workflows/codex-branch.sh | 120 ++++++++++++++++++++++-- t/t9905-codex-branch.sh | 148 +++++++++++++++++++++++++++--- 3 files changed, 276 insertions(+), 34 deletions(-) diff --git a/.github/CODEX.md b/.github/CODEX.md index aa1c6b6d614f19..7e1e2232f24618 100644 --- a/.github/CODEX.md +++ b/.github/CODEX.md @@ -12,11 +12,11 @@ two ordered desired-state files: - `codex.plan` for production; - `codex-unstable.plan` for preview. -Each plan row names a topic ref, its exact reviewed source SHA, the source +Each plan row names a topic ref, its exact admitted source SHA, the source boundary used to replay it, and one prerequisite. The controller retains each -reviewed source object at `refs/heads/codex-pins/`. A mutable topic ref +admitted source object at `refs/heads/codex-pins/`. A mutable topic ref may advance later; that does not change a published generation until another -reviewed plan transition pins the new head. +approved plan transition pins the new head. `codex.config` is the realized ledger. Version 3 records the plan blob, source pins, generated topic tips, and lane output tips produced by the last @@ -31,31 +31,41 @@ active topics belong in `codex`. | Change | Human review | Bot action | Plan effect | | --- | --- | --- | --- | -| Add | Review the topic PR against its lane | Pin the approved head and open a plan PR | Append one topic with its uniquely inferred prerequisite | -| Alter | Review the updated topic PR against its lane | Pin the approved head and open a plan PR | Replace only that topic's source SHA and source boundary | +| Add | Review the topic PR against its lane | Pin its current head and open a plan PR | Append one topic with its uniquely inferred prerequisite | +| Alter | Keep the topic PR approved against its lane | Pin its current head and open a plan PR | Replace only that topic's source SHA and source boundary | | Remove | Review the generated plan PR | Open a plan PR from the explicit workflow dispatch | Delete one plan row | | Reorder | Review the generated plan PR | Open a plan PR from the explicit workflow dispatch | Move one existing row; keep its SHA, boundary, and prerequisite | An add or alter has one human decision: the topic PR. The pull request is review-only because the output-lane rulesets reject ordinary updates. When an approval is submitted, a scheduled scanner on the trusted default branch -notices the exact approved head and runs the plan producer. Operators can -dispatch the same scan immediately when waiting five minutes is undesirable. -That trusted run: +notices the approved topic and freezes its current head before running the +plan producer. Operators can dispatch the same scan immediately when waiting +five minutes is undesirable. That trusted run: 1. rechecks that the PR is open, same-repository, non-draft, aimed at the matching lane, and overall `APPROVED`; -2. requires an effective approval at the exact current head SHA; +2. requires an effective approval from a different repository writer; 3. asks the dedicated plan App to create immutable pins and a `codex-plan/*` branch; 4. opens a one-row plan PR against `meta`; and 5. runs the trusted admission check, which rechecks the topic approval and gives the mechanical plan approval. -The plan PR auto-merges with rebase after its required check passes. If the -topic head changes before that happens, the exact-head check fails. A new -approval produces a new pin and a new plan transition. Once the plan merges, -the immutable pin remains authoritative even if the source branch moves. +An approval remains effective across later topic updates unless it is +dismissed or the reviewer supersedes it with a request for changes. The plan PR +auto-merges with rebase after its required check passes. If the topic head +changes before that happens, the pinned-head check fails; the next scan can +reuse the effective approval and create a new pin and plan transition. Once +the plan merges, the immutable pin remains authoritative even if the source +branch moves. + +After publication, the publisher closes a review-only topic PR only when its +head still matches both the approved plan and the published source pin. A +rebased topic may not appear verbatim in the generated lane, so GitHub shows +that PR as closed rather than merged. Staging alone never closes a PR, and a +closure failure cannot undo an otherwise successful publication. A later +change to the same topic needs another topic PR. Remove and reorder are policy decisions rather than projections of a reviewed topic head. Run **Actions > Refresh codex > Run workflow** with @@ -100,7 +110,7 @@ For a local diagnostic of the same projection: Meta/codex propose-plan --remote origin \ --lane codex-unstable \ --topic tb/codex/my-topic-unstable \ - --source-tip \ + --source-tip \ --review-pr \ --action auto --no-push ``` @@ -137,8 +147,8 @@ then run a fresh rebuild. For a pinned merge-shaped source, the controller uses its reviewed `source-base` as the exact old root and preserves the DAG across a moved generated base only when the two changed-path sets are disjoint. An overlapping base move or a graph with different reviewed roots -fails closed; restack and obtain a new exact-head review instead of flattening -or guessing. +fails closed; restack the approved topic and pin its new head instead of +flattening or guessing. ## Required automation topic diff --git a/.github/workflows/codex-branch.sh b/.github/workflows/codex-branch.sh index 12743516da525c..bf9c7b59b68d91 100755 --- a/.github/workflows/codex-branch.sh +++ b/.github/workflows/codex-branch.sh @@ -5905,7 +5905,9 @@ has_qualifying_current_review () { repository=$4 pull_number=$5 candidates=$6 - awk -F '\t' -v author="$author" -v head="$head" ' + require_exact_head=${7:-true} + awk -F '\t' -v author="$author" -v head="$head" \ + -v exact="$require_exact_head" ' NF == 4 && $2 ~ /^(APPROVED|CHANGES_REQUESTED|DISMISSED)$/ { state[$1] = $2 commit[$1] = $3 @@ -5915,7 +5917,7 @@ has_qualifying_current_review () { for (reviewer in state) if (reviewer != author && state[reviewer] == "APPROVED" && - commit[reviewer] == head) + (exact == "false" || commit[reviewer] == head)) print reviewer "\t" association[reviewer] } ' "$reviews" >"$candidates" || return 1 @@ -5937,9 +5939,10 @@ has_qualifying_current_review () { [(.author.login // "-"), .state, (.commit.oid // "-")] | @tsv' \ >"$candidates-writers" 2>/dev/null || return 1 - awk -F '\t' -v author="$author" -v head="$head" ' + awk -F '\t' -v author="$author" -v head="$head" \ + -v exact="$require_exact_head" ' NF == 3 && $1 != author && $2 == "APPROVED" && - $3 == head { approved++ } + (exact == "false" || $3 == head) { approved++ } END { exit !approved } ' "$candidates-writers" } @@ -6013,8 +6016,8 @@ validate_topic_review () { die "could not inspect reviews for topic pull request #$pull_number" has_qualifying_current_review "$tmp_dir/topic-reviews" "$author" \ "$source_tip" "$repository" "$pull_number" \ - "$tmp_dir/topic-review-candidates" || - die "topic pull request #$pull_number has no current approval for $source_tip" + "$tmp_dir/topic-review-candidates" false || + die "topic pull request #$pull_number has no qualifying approval" say "validated reviewed topic pull request #$pull_number at $source_tip" } @@ -9566,6 +9569,101 @@ stage_and_wait_for_ci () { fi } +close_published_topic_review () ( + controller=$1 + updates=$2 + plan_commit=$3 + seen=$4 + review=$(plan_trailer_optional "$plan_commit" Codex-Plan-Review \ + "Codex-Plan-Review") || return 1 + test -n "$review" || return 0 + case "$review" in + *[!0-9]*) return 1 ;; + esac + action=$(plan_trailer_one "$plan_commit" Codex-Plan-Action \ + "Codex-Plan-Action") || return 1 + case "$action" in + add|alter) ;; + *) return 0 ;; + esac + lane=$(plan_trailer_one "$plan_commit" Codex-Plan-Lane \ + "Codex-Plan-Lane") || return 1 + case "$lane" in + codex|codex-unstable) ;; + *) return 1 ;; + esac + topic=$(plan_trailer_one "$plan_commit" Codex-Plan-Topic \ + "Codex-Plan-Topic") || return 1 + topic=${topic#refs/heads/} + source_tip=$(plan_trailer_one "$plan_commit" Codex-Plan-Source-Tip \ + "Codex-Plan-Source-Tip") || return 1 + published_meta=$(awk -F '\t' \ + '$1 == "refs/heads/meta" { print $3 }' "$updates") || return 1 + published_output=$(awk -F '\t' -v ref="refs/heads/$lane" \ + '$1 == ref { print $3 }' "$updates") || return 1 + test -n "$published_meta" && test -n "$published_output" || return 1 + make_tmp_dir + review_state=$tmp_dir/published-topic-review + mkdir -p "$review_state" || return 1 + git show "$published_meta:$lane.plan" >"$review_state/plan" || return 1 + git show "$published_meta:$meta_config_path" \ + >"$review_state/config" || return 1 + planned_tip=$(git config --no-includes --file "$review_state/plan" \ + --get "branch.$topic.source-tip" || :) + test "$planned_tip" = "$source_tip" || return 0 + ledger_tip=$(git config --no-includes --file "$review_state/config" \ + --get "branch.$topic.source-tip" || :) + ledger_output=$(git config --no-includes --file "$review_state/config" \ + --get "$lane.output-tip" || :) + generated_tip=$(git config --no-includes --file "$review_state/config" \ + --get "branch.$topic.codex-tip" || :) + test "$ledger_tip" = "$source_tip" && + test "$ledger_output" = "$published_output" && + test -n "$generated_tip" || return 1 + git merge-base --is-ancestor "$generated_tip" \ + "$published_output" || return 1 + test "$(remote_head_oid origin refs/heads/meta)" = \ + "$published_meta" || return 1 + test "$(remote_head_oid origin "refs/heads/$lane")" = \ + "$published_output" || return 1 + test "$(remote_head_oid origin "refs/heads/$topic")" = \ + "$source_tip" || return 0 + if grep -F -x "$review" "$seen" >/dev/null + then + return 0 + fi + printf '%s\n' "$review" >>"$seen" || return 1 + gh api --hostname github.com "repos/openai/git/pulls/$review" \ + --jq '[.state, (.draft | tostring), .base.ref, + (.head.repo.full_name // "-"), .head.ref, .head.sha] | @tsv' \ + >"$review_state/pull-request" || return 1 + IFS="$tab" read -r pull_state draft base head_repository \ + head_ref head_sha <"$review_state/pull-request" || return 1 + test "$pull_state" != closed || return 0 + test "$pull_state" = open && test "$draft" = false && + test "$base" = "$lane" && test "$head_repository" = openai/git && + test "$head_ref" = "$topic" && test "$head_sha" = "$source_tip" || + return 0 + gh pr close "$review" --repo github.com/openai/git || return 1 + say "Closed reviewed topic pull request #$review after publishing $lane." +) + +close_published_topic_reviews () ( + controller=$1 + updates=$2 + make_tmp_dir + review_history=$tmp_dir/published-topic-review-history + reviewed=$tmp_dir/published-topic-review-seen + : >"$reviewed" || return 1 + git rev-list --first-parent --max-count=64 "$controller" -- \ + codex.plan codex-unstable.plan >"$review_history" || return 1 + while IFS= read -r plan_commit + do + close_published_topic_review "$controller" "$updates" \ + "$plan_commit" "$reviewed" || return 1 + done <"$review_history" +) + rebuild_codex_locally () { prepare_local_candidate verify_local_candidate "$local_candidate_dir" @@ -9580,6 +9678,11 @@ rebuild_codex_locally () { --updates "$local_candidate_dir/codex-updates" \ --require-automation say "Published codex candidate $candidate from local preparation session $session." + if ! close_published_topic_reviews "$controller_oid" \ + "$local_candidate_dir/codex-updates" + then + say "warning: publication succeeded, but its reviewed topic pull request could not be closed." + fi say "Generated commits identify $bot_name <$bot_email>; the push uses your configured origin credentials." } @@ -9699,6 +9802,11 @@ publish_run () { --inputs "$metadata/codex-inputs" \ --updates "$metadata/codex-updates" --require-automation say "Published codex candidate $artifact_candidate from Actions run $run_id." + if ! close_published_topic_reviews "$run_controller" \ + "$metadata/codex-updates" + then + say "warning: publication succeeded, but its reviewed topic pull request could not be closed." + fi say "Generated commits identify $bot_name <$bot_email>; the push uses your configured origin credentials." } diff --git a/t/t9905-codex-branch.sh b/t/t9905-codex-branch.sh index b9f9cb20c553c0..c50907f444b0a0 100755 --- a/t/t9905-codex-branch.sh +++ b/t/t9905-codex-branch.sh @@ -1549,7 +1549,7 @@ test_expect_success 'generated lanes are output-only and meta gates review in th " "$codex_root/.github/rulesets/codex-meta.json" ' -test_expect_success 'plan admission checks exact reviewed heads from trusted meta' ' +test_expect_success 'plan admission checks pinned topic heads from trusted meta' ' test_path_is_file "$codex_plan_admission_workflow" && test_grep "name: Codex plan admission" \ "$codex_plan_admission_workflow" && @@ -8935,7 +8935,7 @@ test_expect_success 'explicit reorder and remove stay plan-only policy changes' ) ' -test_expect_success 'topic review requires the exact approved head' ' +test_expect_success 'topic approval survives updates to its pinned head' ' mkdir topic-review-bin && cat >topic-review-bin/gh <<-\EOF && #!/bin/sh @@ -8951,6 +8951,12 @@ test_expect_success 'topic review requires the exact approved head' ' *"pulls/42/reviews?"*) printf "%s\t%s\t%s\t%s\n" reviewer APPROVED \ "$FAKE_REVIEW_HEAD" "${FAKE_ASSOCIATION:-MEMBER}" + if test -n "${FAKE_LATEST_REVIEW_STATE:-}" + then + printf "%s\t%s\t%s\t%s\n" reviewer \ + "$FAKE_LATEST_REVIEW_STATE" "$FAKE_REVIEW_HEAD" \ + "${FAKE_ASSOCIATION:-MEMBER}" + fi exit 0 ;; *"pulls/42 "*) @@ -8972,8 +8978,6 @@ test_expect_success 'topic review requires the exact approved head' ' source=$(git rev-parse HEAD) && test_commit review-moved && moved=$(git rev-parse HEAD) && - test_commit review-old && - old=$(git rev-parse HEAD) && test_expect_code 1 env PATH="$PWD/topic-review-bin:$PATH" \ FAKE_HEAD="$moved" FAKE_REVIEW_HEAD="$source" \ FAKE_DECISION=APPROVED sh "$codex_branch" \ @@ -8988,22 +8992,33 @@ test_expect_success 'topic review requires the exact approved head' ' --topic aa/codex/reviewed --source-tip "$source" \ >decision.out 2>decision.err && test_grep "is not approved" decision.err && - test_expect_code 1 env PATH="$PWD/topic-review-bin:$PATH" \ - FAKE_HEAD="$source" FAKE_REVIEW_HEAD="$old" \ + env PATH="$PWD/topic-review-bin:$PATH" \ + FAKE_HEAD="$moved" FAKE_REVIEW_HEAD="$source" \ FAKE_DECISION=APPROVED sh "$codex_branch" \ validate-topic-review --pull-request 42 --lane codex \ - --topic aa/codex/reviewed --source-tip "$source" \ - >stale.out 2>stale.err && - test_grep "has no current approval for $source" stale.err && + --topic aa/codex/reviewed --source-tip "$moved" \ + >updated.out && + test_grep "validated reviewed topic" updated.out && env PATH="$PWD/topic-review-bin:$PATH" \ - FAKE_HEAD="$source" FAKE_REVIEW_HEAD="$source" \ + FAKE_HEAD="$moved" FAKE_REVIEW_HEAD="$source" \ FAKE_ASSOCIATION=NONE FAKE_WRITER=yes \ FAKE_WRITER_HEAD="$source" \ FAKE_DECISION=APPROVED sh "$codex_branch" \ validate-topic-review --pull-request 42 --lane codex \ - --topic aa/codex/reviewed --source-tip "$source" \ + --topic aa/codex/reviewed --source-tip "$moved" \ >writer.out && test_grep "validated reviewed topic" writer.out && + for state in CHANGES_REQUESTED DISMISSED + do + test_expect_code 1 env PATH="$PWD/topic-review-bin:$PATH" \ + FAKE_HEAD="$moved" FAKE_REVIEW_HEAD="$source" \ + FAKE_LATEST_REVIEW_STATE="$state" FAKE_WRITER=no \ + FAKE_DECISION=APPROVED sh "$codex_branch" \ + validate-topic-review --pull-request 42 --lane codex \ + --topic aa/codex/reviewed --source-tip "$moved" \ + >"$state.out" 2>"$state.err" || return 1 + test_grep "has no qualifying approval" "$state.err" || return 1 + done && test_expect_code 1 env PATH="$PWD/topic-review-bin:$PATH" \ FAKE_HEAD="$source" FAKE_REVIEW_HEAD="$source" \ FAKE_ASSOCIATION=NONE FAKE_WRITER=no \ @@ -9011,7 +9026,116 @@ test_expect_success 'topic review requires the exact approved head' ' validate-topic-review --pull-request 42 --lane codex \ --topic aa/codex/reviewed --source-tip "$source" \ >nonwriter.out 2>nonwriter.err && - test_grep "has no current approval for $source" nonwriter.err + test_grep "has no qualifying approval" nonwriter.err +' + +test_expect_success 'publication closes only its exact integrated topic review' ' + git init --bare published-review.git && + test_create_repo published-review-source && + ( + cd published-review-source && + git remote add origin ../published-review.git && + test_commit published-review-base && + base=$(git rev-parse HEAD) && + git branch codex && + git switch -c aa/codex/reviewed-unstable && + test_commit published-review-topic && + source=$(git rev-parse HEAD) && + git switch -c codex-unstable "$base" && + test_commit published-review-generated && + generated=$(git rev-parse HEAD) && + git switch -c meta "$base" && + git config --file codex.config codex-unstable.output-tip "$base" && + git config --file codex.config \ + branch.aa/codex/reviewed-unstable.source-tip "$base" && + git config --file codex.config \ + branch.aa/codex/reviewed-unstable.codex-tip "$base" && + git add codex.config && + git commit -m "previous published preview" && + printf "%s\t%s\t%s\t%s\n" aa/codex/reviewed-unstable \ + "$source" "$generated" codex >rows && + write_pinned_plan codex-unstable codex rows \ + codex-unstable.plan && + git add codex-unstable.plan && + { + printf "Codex plan: alter reviewed preview\n\n" && + printf "Codex-Plan-Lane: codex-unstable\n" && + printf "Codex-Plan-Action: alter\n" && + printf "Codex-Plan-Topic: aa/codex/reviewed-unstable\n" && + printf "Codex-Plan-Source-Tip: %s\n" "$source" && + printf "Codex-Plan-Review: 42\n" + } >message && + git commit -F message && + write policy controller-policy && + git add controller-policy && + git commit -m "unrelated controller update" && + controller=$(git rev-parse HEAD) && + git config --file codex.config \ + codex-unstable.output-tip "$generated" && + git config --file codex.config \ + branch.aa/codex/reviewed-unstable.source-tip "$source" && + git config --file codex.config \ + branch.aa/codex/reviewed-unstable.codex-tip "$generated" && + git add codex.config && + git commit -m "record published preview" && + published_meta=$(git rev-parse HEAD) && + git push origin meta codex codex-unstable \ + aa/codex/reviewed-unstable && + { + printf "refs/heads/meta\t%s\t%s\n" \ + "$controller" "$published_meta" && + printf "refs/heads/codex\t%s\t%s\n" "$base" "$base" && + printf "refs/heads/codex-unstable\t%s\t%s\n" \ + "$base" "$generated" + } >updates && + mkdir close-bin && + cat >close-bin/gh <<-\EOF && + #!/bin/sh + if test "$1" = pr && test "$2" = close + then + printf "%s\n" "$*" >>"$FAKE_CLOSE_LOG" + test "${FAKE_CLOSE_MODE:-}" != failure || exit 93 + printf "%s\n" closed >"$FAKE_CLOSE_STATE" + exit 0 + fi + test "$1" = api && test "$2" = --hostname && + test "$3" = github.com && + test "$4" = repos/openai/git/pulls/42 || exit 92 + state=$(cat "$FAKE_CLOSE_STATE" 2>/dev/null || printf open) + head=$FAKE_HEAD + test "${FAKE_CLOSE_MODE:-}" != moved || head=$FAKE_OTHER + printf "%s\tfalse\tcodex-unstable\topenai/git\taa/codex/reviewed-unstable\t%s\n" \ + "$state" "$head" + EOF + chmod +x close-bin/gh && + cat >close-review <<-\EOF && + #!/bin/sh + set -- --help + . "$CODEX_BRANCH" >/dev/null + close_published_topic_reviews "$FAKE_CONTROLLER" "$FAKE_UPDATES" + EOF + close_review () { + env PATH="$PWD/close-bin:$PATH" CODEX_BRANCH="$codex_branch" \ + FAKE_CONTROLLER="$controller" FAKE_UPDATES="$PWD/updates" \ + FAKE_HEAD="$source" FAKE_OTHER="$base" \ + FAKE_CLOSE_LOG="$PWD/closed" \ + FAKE_CLOSE_STATE="$PWD/close-state" \ + FAKE_CLOSE_MODE="${1:-}" sh close-review + } && + : >closed && + close_review moved && + test_must_be_empty closed && + close_review >close.out && + test_grep "Closed reviewed topic pull request #42" close.out && + test_grep "pr close 42 --repo github.com/openai/git" closed && + close_review && + test_line_count = 1 closed && + rm -f close-state && + test_expect_code 1 close_review failure && + test_line_count = 2 closed && + test "$source" = "$(git --git-dir=../published-review.git \ + rev-parse refs/heads/aa/codex/reviewed-unstable)" + ) ' test_expect_success 'checked-in release recovery manifest is the bound incident' '