fix(ci): make the docs sync open a checkable, revertible PR against main - #10662
fix(ci): make the docs sync open a checkable, revertible PR against main#10662aryanorastar wants to merge 10 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the direction is good. Moving the docs-sync PR creation onto the app token and switching the auto-merge from --squash to --merge are the right properties for making this path checkable and revertible.
I do think the new guard needs one more mutation case before merge. Right now check_workflow() only rejects an explicit token: ${{ secrets.GITHUB_TOKEN }} or the total absence of actions/create-github-app-token. That means a self-merging workflow can omit the token: input on peter-evans/create-pull-request (which defaults to the workflow token) while still having an app-token step elsewhere, and the guard returns success. I verified that shape locally against this PR's checker and it produced [] errors.
Could you tighten the guard so it verifies the create-pull-request step itself is using the app token/non-default token, and add a regression test for the omitted-token/default-GITHUB_TOKEN case? Once that hole is covered, the workflow change and the existing tests look like a strong fix for #10535.
Local validation run:
python3 .github/scripts/test_check_automated_main_write_admission.pypassed (8 tests)python3 .github/scripts/check_automated_main_write_admission.pypassed on the current tree
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Resolved on head 9ba827a: the guard now rejects omitted create-pull-request token/default-token cases and includes a regression test; current recommendation is positive signal / human workflow review, not changes requested.
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the update — the prior blocker is resolved on this head.
I re-ran the guard locally and the new omitted-token/default-GITHUB_TOKEN regression is now covered: the self-test suite passes with 9 tests, and the admission checker passes on the current tree. The workflow change also now opens the docs-sync PR with the app token before creation and uses gh pr merge --merge, which restores the two intended properties: generated-docs PRs are checkable and automated main writes stay revertible.
Because this PR changes workflow automation for an automated path into main, I’m keeping this as a positive maintainer signal rather than a formal approval; the existing workflow-review label and human maintainer review before merge are still appropriate.
Local validation:
python3 .github/scripts/test_check_automated_main_write_admission.pypassed (9 tests)python3 .github/scripts/check_automated_main_write_admission.pypassed on the current tree
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
9ba827a to
fc6416a
Compare
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for keeping this tight. The actual sync-docs.yml change is pointed in the right direction: it now mints the app token before PR creation, uses that token for create-pull-request, and switches the automated merge to --merge.
I found one remaining blocker in the new guard, though. check_automated_main_write_admission.py verifies that the create-pull-request token references an actions/create-github-app-token step id, but it does not verify that the app-token step appears before the create-pull-request step. A workflow can therefore put the app-token step after PR creation while still using token: ${{ steps.app-token.outputs.token }}, and the guard returns success. That order would not prove the app token is available when the PR is opened, which is the core checkability property this guard is meant to preserve.
Could you tighten the guard to reject a create-pull-request step that references an app-token step declared later in the workflow, and add a regression test for that ordering mutation?
Validation I ran on this head:
python3 .github/scripts/test_check_automated_main_write_admission.pypassed (11 tests)python3 .github/scripts/check_automated_main_write_admission.pypassed- additional wrong-order mutation (
Create Pull RequestbeforeGenerate Omi Bot token, while still referencingsteps.app-token.outputs.token) returned[]errors, exposing the gap
Keeping workflow-review is appropriate because this changes an automated path into main.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
79c283d to
ef37f1c
Compare
Resolved on current head ef37f1c: app-token ordering guard and regression test are present; current recommendation is positive signal, not request changes.
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the follow-up — the remaining guard blocker I called out on the previous head looks resolved here.
I verified the current head adds a regression for the app-token step being declared after create-pull-request, and the checker now compares the app-token step order against the PR-creation step. The actual sync-docs.yml path also now mints the Omi Bot app token before opening the PR, uses that token for peter-evans/create-pull-request, and uses gh pr merge --merge so the automated docs-sync write stays revertible.
I’m keeping this as a positive maintainer signal rather than a formal approval because this changes workflow automation for an automated path into main; the existing workflow-review label and human maintainer review before merge are still appropriate.
Local validation on ef37f1cb17ae3b356647b57ace87f7af72b55e57:
python3 .github/scripts/test_check_automated_main_write_admission.pypassed (12 tests)python3 .github/scripts/check_automated_main_write_admission.pypassed
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
Failure-Class: none
Failure-Class: none
The admission guard verified that create-pull-request references an actions/create-github-app-token step id, but not that the token step is declared before the PR step. `steps` is populated top to bottom, so a token step declared later is not available when the PR opens, and the checkable app-identity contract is unproven. Reject that ordering with a dedicated error, and add a mutation test that moves the app-token step below Create Pull Request while still referencing steps.app-token.outputs.token. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
ef37f1c to
d42fa77
Compare
|
@Git-on-my-level need human response — this is still blocked on code-owner review for an automated path into Your last AI pass on What’s left is the formal human / code-owner sign-off the |
|
@Git-on-my-level follow-up: PR #10662 remains fully CI-green at d42fa77. Your latest positive review verified the app-token ordering guard, 12 regression tests, and the checkable/revertible sync-docs workflow. The only remaining blocker is formal workflow code-owner approval. |
Keep the sync-docs checkable/revertible admission fix current for CO re-check.
|
@Git-on-my-level need human response — workflow code-owner APPROVE still the only merge blocker on #10662. Refreshed tip
Re-verified on this tip: Your last AI pass was positive (not CR). Please human / workflow-review APPROVE (or explicit hold) when you can. |
Pick up desktop glass tripwire updates so Desktop Swift CI is not failing on stale AgentPill/SettingsGlassChrome contracts.
|
@Git-on-my-level need human response — Desktop Swift red on prior tip was the same stale glass tripwires as #11142/#10964/#10965 (not this sync-docs admission fix). Merged current
Please human / workflow-review APPROVE (or explicit hold) when CI re-greens. |
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the continued work here. The actual sync-docs.yml repair still looks pointed in the right direction: the app token is minted before create-pull-request, that token is used for the PR, and the automated merge is now --merge so the docs-sync write remains revertible.
I found one remaining hole in the new admission guard, though. The checker tracks app-token step IDs and ordering at the whole-workflow level, not within the job that opens the PR. That means a workflow with an actions/create-github-app-token step in an earlier, different job can still have a later self-merging job use token: ${{ steps.app-token.outputs.token }} even though that step output is not in scope for the PR-creation job. The guard currently returns success for that shape, so it would not actually prove the create-pull-request step has an available app token when the PR opens.
Could you tighten the guard to validate the app-token reference within the same job/steps list as the peter-evans/create-pull-request step, and add a regression test for the cross-job/out-of-scope app-token mutation?
Validation I ran on this head:
python3 .github/scripts/test_check_automated_main_write_admission.pypassed (12 tests)python3 .github/scripts/check_automated_main_write_admission.pypassed on the current tree- additional two-job mutation (
app-tokenstep in one job, self-mergingcreate-pull-requestin another job referencingsteps.app-token.outputs.token) returned[]errors, exposing the gap
Keeping workflow-review is appropriate because this changes an automated path into main.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Keep tip current while addressing cross-job app-token scope review.
Validate create-pull-request app-token references within the PR job's steps list so a cross-job token mint cannot satisfy the checkable admission guard (BasedHardware#10535 / BasedHardware#10662). Co-authored-by: Cursor <cursoragent@cursor.com>
|
@Git-on-my-level addressed the same-job / cross-job app-token hole on @ Guard tighten: Regression: Local: Please re-review / dismiss CR when you can. need human response for workflow-review APPROVE after that. |
|
@Git-on-my-level need human response — please dismiss CR / re-review on tip Your same-job / cross-job app-token hole is addressed (was already on
CI re-running on the main merge. Please dismiss CHANGES_REQUESTED / human |
Resolved on current head: the admission guard now validates app-token references within the same job/steps list and includes a cross-job regression test.
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the update — the prior guard blocker looks resolved on this head.
Specific review notes:
.github/workflows/sync-docs.ymlnow mints the Omi Bot app token beforepeter-evans/create-pull-request, passessteps.app-token.outputs.tokeninto that action, and switches the automated merge from--squashto--merge. That addresses the checkability/revertibility failure mode for the generated-docs PR path..github/scripts/check_automated_main_write_admission.pynow groupssteps:blocks by job in_job_steps_lists()and validates the app-token output against the same job's steps list before the PR creation step. That closes the cross-job/out-of-scope hole from my previous change request..github/scripts/test_check_automated_main_write_admission.pyadds the missing regression for an app token minted in one job whilecreate-pull-requestruns in another job, plus the default-token, non-app-token, wrong-order, squash, and rebase mutations. The realsync-docs.ymlcoverage assertion is also useful so the checker cannot silently stop matching the workflow it is meant to protect..github/checks-manifest.yamlregisters both the admission checker and its fixture suite with workflow/script triggers, so future workflow changes should exercise this guard through the normal static-check manifest.
Validation I ran on this head:
python3 .github/scripts/test_check_automated_main_write_admission.py— 13 tests passedpython3 .github/scripts/check_automated_main_write_admission.py— passed and reported 1 self-merging workflow OK (.github/workflows/sync-docs.yml)
I am not seeing a remaining code blocker. Keeping this as maintainer review rather than formal approval because it intentionally changes a CI/workflow path that can write to main; workflow automation policy and the retained --admin behavior need maintainer sign-off before merge.
by AI on behalf of David — workflow automation policy needs maintainer sign-off for this sync-docs.yml main-write path before merge.
|
@Git-on-my-level need human response — formal workflow/code-owner APPROVE is the only merge blocker on #10662. Author-clear on tip
Please human / |
|
@Git-on-my-level workflow-review ping: current head is MERGEABLE; the checkable/revertible docs-sync PR has no author-side blocker. Please approve or state an explicit hold. |
|
@Git-on-my-level workflow CO ping: still MERGEABLE, author-side clear. Waiting maintainer APPROVE. |
|
@Git-on-my-level workflow CO ping (>1d): still MERGEABLE, CI green, author-clear. Please APPROVE or explicit hold. |
|
@Git-on-my-level Workflow CO APPROVE ping — tip |
|
Closing as superseded by #10959 for the same issue (#10535). Both tips move the Omi Bot token before
Keeping #10959 open for Workflow CO / maintainer merge. |
|
Hey @aryanorastar 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Before your next PR, please skim:
If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing. Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! |
What
sync-docs.ymlis the one automated path intomain, and it had lost both properties that keep such a path honest. Fixes #10535.secrets.GITHUB_TOKEN. GitHub deliberately does not firepull_requestworkflows for events authored by that token, so the PR arrived with an empty check list — it looked reviewed because it was a PR, while nothing verified the regeneratedsdks/*/README.mdandomiGlass/README.md.--squash, and a squashed automated commit onmaincannot be undone withgit revert -m 1— which is exactly what AGENTS.md:77 ("Merge, never squash") keeps merge commits for.Not theoretical: #8325 was created and merged four seconds apart, its whole check list
skipping/skipping, merge commit single-parent. #7543, #3680 and #3675 are the same shape.Fix
The workflow already minted an Omi Bot app token — it just did so after opening the PR, and used it only for the merge. So the repair is ordering, not new machinery:
Generate Omi Bot tokenahead ofCreate Pull Requestand passsteps.app-token.outputs.tokentopeter-evans/create-pull-request. An app-authored PR firespull_requestchecks.gh pr merge --squash→--merge, restoringgit revert -m 1recovery.What I deliberately did NOT change
--adminstays. Removing it decides whether a docs sync may still land when a check is red — that depends on branch-protection configuration I cannot see from the repository, and it is the maintainer policy question the issue raises ("does the auto-merge need to exist at all"). This PR makes the checks exist and the merge revertible without pre-empting that call. Happy to follow up once a maintainer decides.Guard
check_automated_main_write_admission.pypins both properties for any workflow that both opens a PR viacreate-pull-requestand merges one viagh pr merge. A workflow that merely opens a PR for a human is out of scope — the human is the check.Not a shared primitive because no existing check inspects the authorship of an automated PR against
main; the nearest neighbour (deployment-secret-boundary) reasons about secret bindings, not merge admission. It cites real merged instances (#8325, #7543, #3680, #3675) rather than a hypothetical.Verification
The mutation evidence is the load-bearing part: the guard was run against the real pre-fix workflow from
main, not a hand-written fixture, and rejected it with both errors.Not exercised live: the workflow itself only runs on a
docs/**.mdxpush tomainwith the bot secrets, so I could not fire it. The change is ordering plus one flag, verified statically and by the guard.Product invariants affected
none
Failure-Class: none