Skip to content

fix(ci): make the docs sync open a checkable, revertible PR against main - #10662

Closed
aryanorastar wants to merge 10 commits into
BasedHardware:mainfrom
aryanorastar:fix/10535-sync-docs-unchecked-main-write
Closed

fix(ci): make the docs sync open a checkable, revertible PR against main#10662
aryanorastar wants to merge 10 commits into
BasedHardware:mainfrom
aryanorastar:fix/10535-sync-docs-unchecked-main-write

Conversation

@aryanorastar

@aryanorastar aryanorastar commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

What

sync-docs.yml is the one automated path into main, and it had lost both properties that keep such a path honest. Fixes #10535.

  • It opened its PR with secrets.GITHUB_TOKEN. GitHub deliberately does not fire pull_request workflows 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 regenerated sdks/*/README.md and omiGlass/README.md.
  • It merged that PR with --squash, and a squashed automated commit on main cannot be undone with git 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:

  • Move Generate Omi Bot token ahead of Create Pull Request and pass steps.app-token.outputs.token to peter-evans/create-pull-request. An app-authored PR fires pull_request checks.
  • gh pr merge --squash--merge, restoring git revert -m 1 recovery.

What I deliberately did NOT change

--admin stays. 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.py pins both properties for any workflow that both opens a PR via create-pull-request and merges one via gh 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

check_automated_main_write_admission.py                -> exit 0 on the fixed tree
same guard vs main's PRE-FIX sync-docs.yml             -> exit 1, naming BOTH defects
test_check_automated_main_write_admission.py           -> 8 passed
test_run_checks.py (manifest contract)                 -> 26 passed
run_checks.py --lane local                             -> both new checks SELECTED and PASS

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/**.mdx push to main with 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

Review in cubic

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@Git-on-my-level Git-on-my-level added the workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior label Jul 26, 2026

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py passed (8 tests)
  • python3 .github/scripts/check_automated_main_write_admission.py passed 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.

@Git-on-my-level
Git-on-my-level dismissed their stale review July 27, 2026 20:50

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 Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py passed (9 tests)
  • python3 .github/scripts/check_automated_main_write_admission.py passed 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.

@aryanorastar
aryanorastar force-pushed the fix/10535-sync-docs-unchecked-main-write branch from 9ba827a to fc6416a Compare July 31, 2026 07:09
@Git-on-my-level Git-on-my-level added the docs-tooling Layer: Documentation, examples, dev tools label Jul 31, 2026

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py passed (11 tests)
  • python3 .github/scripts/check_automated_main_write_admission.py passed
  • additional wrong-order mutation (Create Pull Request before Generate Omi Bot token, while still referencing steps.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.

@aryanorastar
aryanorastar force-pushed the fix/10535-sync-docs-unchecked-main-write branch from 79c283d to ef37f1c Compare August 1, 2026 04:17
@Git-on-my-level
Git-on-my-level dismissed their stale review August 1, 2026 11:03

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 Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py passed (12 tests)
  • python3 .github/scripts/check_automated_main_write_admission.py passed

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

aryanorastar and others added 4 commits August 3, 2026 21:40
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
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>
@aryanorastar
aryanorastar force-pushed the fix/10535-sync-docs-unchecked-main-write branch from ef37f1c to d42fa77 Compare August 3, 2026 16:18
@aryanorastar

Copy link
Copy Markdown
Contributor Author

@Git-on-my-level need human response — this is still blocked on code-owner review for an automated path into main.

Your last AI pass on ef37f1c / current head d42fa77a was a positive signal (not CHANGES_REQUESTED): app-token ordering guard + regression covered, sync-docs.yml mints the bot token before create-pull-request, and merge is --merge (revertible). Local validation you cited: 12 self-tests + admission checker green. CI on this head is fully green.

What’s left is the formal human / code-owner sign-off the workflow-review label asks for — or an explicit “hold / change X” if something else is blocking. Happy to follow up immediately either way.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

@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.
@aryanorastar

Copy link
Copy Markdown
Contributor Author

@Git-on-my-level need human response — workflow code-owner APPROVE still the only merge blocker on #10662.

Refreshed tip 4d20e929 = merge of current main onto the checkable/revertible sync-docs fix (no conflicts). Still:

  • app token minted before create-pull-request, and used as the PR token
  • gh pr merge --merge (revertible)
  • admission guard covers omitted-token + wrong-order mutations
  • Failure-Class: none

Re-verified on this tip: test_check_automated_main_write_admission.py12 passed; live check_automated_main_write_admission.py → OK on sync-docs.yml.

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.
@aryanorastar

Copy link
Copy Markdown
Contributor Author

@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 main on tip 0b0946cf0c46. Fix intact:

  • app token minted before create-pull-request, used as PR token
  • gh pr merge --merge (revertible)
  • admission guard covers omitted-token + wrong-order mutations
  • Failure-Class: none

Please human / workflow-review APPROVE (or explicit hold) when CI re-greens.

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py passed (12 tests)
  • python3 .github/scripts/check_automated_main_write_admission.py passed on the current tree
  • additional two-job mutation (app-token step in one job, self-merging create-pull-request in another job referencing steps.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.

aryanorastar and others added 2 commits August 8, 2026 17:35
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>
@aryanorastar

Copy link
Copy Markdown
Contributor Author

@Git-on-my-level addressed the same-job / cross-job app-token hole on @37c9d7a63e5a.

Guard tighten: _job_steps_lists scopes app-token id + ordering checks to the same job as peter-evans/create-pull-request. A mint job that declares actions/create-github-app-token no longer satisfies a later self-merging job that references steps.app-token.outputs.token (out of scope).

Regression: test_rejects_an_app_token_step_from_a_different_job — the two-job mutation you verified now returns same-job errors (was []).

Local:

python3 .github/scripts/test_check_automated_main_write_admission.py → 13 passed
python3 .github/scripts/check_automated_main_write_admission.py → OK on sync-docs.yml

Please re-review / dismiss CR when you can. need human response for workflow-review APPROVE after that.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

@Git-on-my-level need human response — please dismiss CR / re-review on tip 85479939657f.

Your same-job / cross-job app-token hole is addressed (was already on 37c9d7a; tip refreshed onto current main):

  • Guard: _job_steps_lists scopes app-token id + ordering to the same job as peter-evans/create-pull-request
  • Regression: test_rejects_an_app_token_step_from_a_different_job (your two-job mutation now fails closed)
  • Local re-check after main refresh: 13 passed + live admission checker OK on sync-docs.yml
  • Workflow still: app token minted before create-PR, gh pr merge --merge, Failure-Class: none

CI re-running on the main merge. Please dismiss CHANGES_REQUESTED / human workflow-review APPROVE (or explicit hold) when you can.

@Git-on-my-level
Git-on-my-level dismissed their stale review August 9, 2026 13:50

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 Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update — the prior guard blocker looks resolved on this head.

Specific review notes:

  • .github/workflows/sync-docs.yml now mints the Omi Bot app token before peter-evans/create-pull-request, passes steps.app-token.outputs.token into that action, and switches the automated merge from --squash to --merge. That addresses the checkability/revertibility failure mode for the generated-docs PR path.
  • .github/scripts/check_automated_main_write_admission.py now groups steps: 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.py adds the missing regression for an app token minted in one job while create-pull-request runs in another job, plus the default-token, non-app-token, wrong-order, squash, and rebase mutations. The real sync-docs.yml coverage assertion is also useful so the checker cannot silently stop matching the workflow it is meant to protect.
  • .github/checks-manifest.yaml registers 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 passed
  • python3 .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.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

@Git-on-my-level need human response — formal workflow/code-owner APPROVE is the only merge blocker on #10662.

Author-clear on tip 076ed8b45d0c (refreshed onto current main):

  • Your CR dismissed 51m ago: same-job app-token guard + cross-job regression resolved; no remaining code blocker
  • Local re-check: 13 passed · admission checker OK on sync-docs.yml
  • Still: app token before create-PR · gh pr merge --merge (revertible) · Failure-Class: none
  • Sibling workflow PR: fix(ci): run checks and regular-merge the sync-docs PR (#10535) #10959 (same #10535 family; switches --admin--auto — this PR intentionally kept --admin for the separate maintainer policy call you flagged)

Please human / workflow-review APPROVE (or explicit hold on retained --admin / main-write automation policy) when you can — branch protection is waiting on code owner review from you (.github/workflows/**).

@undivisible undivisible added the human Human-authored pull request label Aug 10, 2026
@aryanorastar

Copy link
Copy Markdown
Contributor Author

@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.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

@Git-on-my-level workflow CO ping: still MERGEABLE, author-side clear. Waiting maintainer APPROVE.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

@Git-on-my-level workflow CO ping (>1d): still MERGEABLE, CI green, author-clear. Please APPROVE or explicit hold.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

@Git-on-my-level Workflow CO APPROVE ping — tip 076ed8b4 MERGEABLE; author-side clear. Please APPROVE when convenient.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #10959 for the same issue (#10535).

Both tips move the Omi Bot token before create-pull-request and switch squash → merge. The material difference:

#10662 #10959
Merge gh pr merge --merge --admin gh pr merge --auto --merge (no --admin)
Scope + admission ratchet (~500 lines) workflow-only (shrink)
David positive signal; guard kept needing holes closed CR dismissed; direction OK pending maintainer workflow policy

main still has the broken shape (GITHUB_TOKEN create + --squash --admin). Landing #10959 is the merge-quality fix for that. The admission checker in #10662 is useful as a follow-up if we want a ratchet later — not as a competing same-issue PR that still leaves --admin on the live path.

Keeping #10959 open for Workflow CO / maintainer merge.

@github-actions

Copy link
Copy Markdown
Contributor

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:

  • Project standards — Ensuring consistency across the codebase
  • User needs — Making sure changes align with what our users need
  • Code best practices — Maintaining code quality and maintainability
  • Project direction — Keeping aligned with our product principles and locked invariants

Before your next PR, please skim:

  • PRODUCT.md — product north star
  • Product invariants — locked rules (shared chat, memory tiers, agent control plane, integrations, brand)

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-tooling Layer: Documentation, examples, dev tools human Human-authored pull request workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sync-docs.yml auto-merges to main with --admin --squash on a PR that ran zero checks

3 participants