Branching & pull requests¶

Status (2026-07-16): server-side protection on main is ACTIVE — ruleset main-protected (id 19043820). main takes no direct pushes from anyone, including repo admins. Verified by test, not assumed; see §3.

The repo is open to the lab: issues and PRs are welcome from anyone with access. main is the branch the acquisition PCs pull from, so it is the one thing that always has to work. Every change reaches it the same way — through a branch and a reviewed PR.

1. The workflow¶

git switch main && git pull                 # start from current main
git switch -c fix/cyton-latency-offset      # branch
# ...work, commit...
git push -u origin fix/cyton-latency-offset # push the branch
gh pr create                                # open the PR

Branch names: feat/…, fix/…, docs/…, chore/… — a short description after the slash.

Add a changelog.d/<issue-or-pr-number>-<slug>.md fragment for a user-visible change, in the same PR — never edit CHANGELOG.md itself (changelog.d/README.md has the format; scripts/assemble_changelog.py --check validates it, and CI runs that check on every PR). With several PRs open at once, everyone editing the same lines under Unreleased is a merge conflict unrelated to any of their own code; one fragment file per PR never collides with another. Skip only for a change with no user-visible effect.

⚠️ Working from a git worktree? pip install -e . points the whole ssvep conda env at whichever checkout last ran it, so pytest/python in a different worktree can silently import ssvep from the wrong tree (#184) — tests/conftest.py now fails the session loudly when that happens, and tells you to set PYTHONPATH=<this-worktree>/src. scripts/new-worktree.ps1 (#50) sets up a worktree with its own cloned env to avoid the collision in the first place.

Nobody pushes to main directly. Not for a one-line docs typo, not for a hotfix. GitHub will refuse it (GH013: Changes must be made through a pull request). Merged branches are auto-deleted.

2. The local hook (optional, but nice)¶

git config core.hooksPath .githooks

That points git at .githooks/, whose pre-push hook refuses a direct push to main and prints the recovery steps if you already committed there.

Since §3 landed this is convenience, not enforcement — it fails immediately and explains itself, instead of you waiting on a network round-trip to get a GH013 back. The server is the authority.

The same hook also runs the forbidden-literal guard (#298) whenever this machine has a site profile: it refuses a push whose new commits carry one of the profile's publish.forbidden_literals. docs/SITE_PROFILE.md §11 has what it checks, SSVEP_REQUIRE_LITERALS and SSVEP_PYTHON. --no-verify skips the hook and gets you rejected by GitHub instead.

3. What is enforced on `main`¶

Ruleset main-protected (applied 2026-07-16; required status checks added 2026-07-17; docs check added 2026-09-12), targeting the default branch:

Rule	Effect
`pull_request` (1 approval)	Changes must go through a PR; stale approvals dismissed on push
`required_status_checks`	The cheap CI jobs must pass before merge — `compliance (no protected data)` and `mkdocs build --strict` (the docs site; a broken link or a stale nav entry fails the PR rather than the published site) (§4). The two test jobs are not required while Actions minutes are limited; the gate for "ready" is the full local suite (§4). The job names are the check names: renaming a job silently drops it from the gate.
`non_fast_forward`	No force-pushing `main`
`deletion`	`main` cannot be deleted

Bypass: repo role admin (actor_id: 5) with bypass_mode: "pull_request".

⚠️ A repository transfer wipes bypass_actors. When the repo moved from its previous org to NeuroCognitiveImagingLab (2026-09-12) the ruleset came across intact except its bypass list, which arrived empty. The failure is quiet and total: gh pr merge --admin returns "At least 1 approving review is required", and since GitHub does not let you approve your own PR, a solo admin is simply stuck. Re-add it with gh api --method PUT repos/OWNER/REPO/rulesets/<id> --input <the exported ruleset>, then re-export to keep that file honest. Worth checking after any transfer or org change.

⚠️ The bypass covers the whole ruleset, status checks included. A pull_request-mode bypass lets an admin merge a PR without the approval and past a red or pending check (that is what gh pr merge --admin does). So the required checks are a hard gate for write collaborators — who have no bypass and simply cannot merge on red — and a strong default, not a wall, for admins. Two things make that acceptable rather than a hole:

tests/test_compliance_data.py is a whole-tree invariant, not a diff check. If anything ever slipped past on an admin bypass, the next PR's compliance job (and the push-to-main run) goes red on it immediately — it cannot merge, then hide.
Admins still merge on green in practice (Claude waits for it; §3.2). The bypass exists for the approval deadlock, not to skip CI.

If the lab ever wants the compliance check to be non-bypassable even for admins, put it in a separate ruleset with no bypass_actors — rulesets stack, so that check would then hold for everyone while this ruleset's approval bypass stays. Not done now; noted so the option is on record.

That mode is narrower than it sounds, and the distinction is the whole design, so it was tested rather than trusted. Against a scratch branch carrying an identical ruleset, an admin's direct push was rejected:

remote: error: GH013: Repository rule violations found for refs/heads/rule-test-tmp.
remote: - Changes must be made through a pull request.
 ! [remote rejected] (push declined due to repository rule violations)

So bypass_mode: "pull_request" means: admins still cannot push to main — they can only skip the approval requirement when merging a PR. Concretely:

Write collaborators (everyone with write access who is not an admin): branch → PR → need one approval → merge. They cannot self-merge.
Admins (the repository admins): branch → PR → may merge their own without waiting for a reviewer. Still no direct push.

This exists because GitHub does not let you approve your own PR. Without the bypass, a solo late-night fix would sit until someone else woke up — the kind of friction that gets protection switched off entirely. The cost is real: an admin PR can reach main unreviewed. If the lab gets into a steady review rhythm, delete the bypass_actors entry and admins get held to the same bar.

Using the bypass¶

gh pr merge <N> --squash --admin --delete-branch

--admin is what invokes it. Without the flag, gh refuses:

X Pull request #35 is not mergeable: the base branch policy prohibits the merge.

That refusal is the design working — the bypass must be asked for explicitly, never applied by accident. Squash is the default for this repo: a PR lands as one revertible commit on main.

Claude merges its own PRs this way when the work is done (this deployment's own agent workflow, which a published snapshot does not carry) — it does not wait for the maintainer. Leaving a finished PR open is a claim the work is unfinished, and it caused a real loss: #33 was merged while more commits were still being pushed to its branch, and they landed nowhere until they were rescued onto a new one (#35).

Changing the rules¶

Administering this repository's ruleset: it names the org, the repo and the ruleset id, and it needs docs/ruleset-main.json, which no snapshot carries. A derived repository has its own settings and its own ruleset, so this subsection is source-only.

Edit the exported ruleset JSON, docs/ruleset-main.json (source repository only) — versioned so it is reviewable rather than pasted from memory — then:

gh api -X PUT repos/NeuroCognitiveImagingLab/ssvep-toolbox/rulesets/19043820 --input docs/ruleset-main.json

Check what is actually live (this aggregates repo and org-level rulesets, so it is the honest answer to "is main protected?" — the repo settings UI shows what is available, which is not the same thing):

gh api repos/NeuroCognitiveImagingLab/ssvep-toolbox/rules/branches/main --jq '[.[].type]'

Plan note¶

This needs the owning org on GitHub Team (set up on the previous org 2026-07-16; the repo moved to NeuroCognitiveImagingLab in September 2026, which has academic Team). Free has no protected branches or rulesets on private repos, and refuses with a 403 whose text says "Upgrade to GitHub Pro" — that message is misleading for an org: Pro is the paid tier for personal accounts, Team is the one for organizations. They are different products, not a rename. If protection ever silently stops applying, check the org plan first.

4. What CI checks¶

.github/workflows/ci.yml has three jobs plus a cache seed, kept separate on purpose. Until the repository is public (when Actions becomes free), only compliance runs on every PR and every push to main; the two test suites run only on a manual dispatch (the Run workflow button), and there is no nightly. The reason is cost: Actions spending limits ran out, and the Windows suite was nearly all of the spend.

Job	Runs	PR	Push to `main`	Manual dispatch
compliance (no protected data)	ubuntu, ~30 s, stdlib + pytest only	✅	✅	✅
tests (linux, ssvep env)	ubuntu, real `ssvep` conda env, Qt offscreen + xvfb	—	—	full suite
tests (windows, ssvep env)	windows-latest, real `ssvep` conda env	—	—	full suite
seed the env cache (ubuntu, windows)	the env only, no tests; not a required check	—	—	✅

compliance — tests/test_compliance_data.py: nothing matching the protected patterns is tracked, and BIDS/participants.tsv has not been re-added to the index. Also scripts/assemble_changelog.py --check (stdlib-only, so it belongs here rather than in the slower test jobs): every changelog.d/ fragment is well-formed. No conda — a data-governance gate must not fail open because an unrelated dependency broke.
tests (linux, ssvep env) — the whole suite, on the platform that bills half as much. Manual dispatch only.
tests (windows, ssvep env) — Windows because that is what the acquisition PCs run, and where the ctypes amp drivers, junction/subst/UNC path handling, pythonw and the GL renderer live. Manual dispatch only, and then the full suite.

The gate for "ready" is the full local suite on Windows. Run pytest -q (no file list) on the exact commit you push and write its final pass line in the PR body. Nothing in CI checks this; it rests on whoever marks the PR ready, and the reviewer should look for the line. compliance and mkdocs build --strict still run on every PR and push and stay required.

A clean-environment run replaces the clean-env CI check. Before each bench trip and before the public release, build a conda env fresh from environment.yml (name it ssvep-clean, so the everyday ssvep env is not touched) and run pytest -q in it. That is what catches a dependency the long-lived env has but environment.yml does not list, which a local run in the everyday env cannot.

The Windows-subset markers stay, so the arrangement can be restored without a gap. Linux runs every test. A test that skips off Windows because it needs Windows must be marked windows, or it would run on no Windows job once the PR subset is restored. Three checks in tests/_ci_guard.py keep that gap from opening silently: a test-phase skip off Windows whose reason names a Windows construct fails on Linux, naming the fix; so does a module that skips while it is imported (pytest.skip(..., allow_module_level=True), a module-level importorskip), which never reaches a test report; and tests/test_ci_markers.py scans every test file for a skip whose condition is platform-shaped (sys.platform, os.name, platform.system()) and true on Linux, or an importorskip of a Windows-only module, in a test or module without the marker — immune to how the reason is worded. The reverse gap is reported rather than failed: a windows-marked test that skips on the Windows runner (say mklink /J or subst stops working on a new runner image) runs nowhere, so the Windows job's log ends with a list of them and a warning annotation on the run. When you add a test that exercises Windows-only behaviour (ctypes/WinDLL, junctions or mklink, subst, drive letters or UNC paths, pythonw, the vendored validator binary, an os.name == "nt" branch), or a known Windows failure mode worth a real Windows run on every PR (replace/unlink under NTFS semantics, CRLF, drive-letter paths, pipe EOF), mark it windows. A test that only fakes Windows (a monkeypatched os.name, a fake process or display) runs on Linux regardless and does not need it. The real-GL tests are not marked: the Windows runner has no GL beyond GDI's 1.1, so they skip there, and the Linux job's xvfb + Mesa is what runs them — on the Linux CI job a missing GL context fails rather than skips, so a runner regression cannot turn them off quietly. The markers are registered in pyproject.toml, and --strict-markers makes a misspelt one an error rather than a test quietly dropped from the Windows job.

Parallel tests. Both test jobs run pytest -n auto --dist worksteal (pytest-xdist): tests are shared out one by one and an idle worker takes queued ones from a busy one, so the job is not held up by whichever worker drew the heaviest file (--dist loadfile kept each file on one worker, and the few multi-minute montage tests then ran back to back on it). A module- or session-scoped fixture is set up once per worker that runs any of its tests, so each test must still stand on its own — which the suite already requires, since pytest -k runs any subset. A test that is not safe to run alongside others (a fixed path outside tmp_path, a process-wide resource) is marked serial and runs in a second, plain pass in the same job. That second pass exists only in the manual-dispatch suite jobs (since #423 nothing on a PR runs the suites), and a local pytest -n auto runs serial tests in parallel with the rest. So a serial test is isolated only in a dispatched suite job's pytest -m serial pass, not under a local -n; plain pytest -q runs it serially by construction. Run it alone (pytest -m serial) when its result matters, and write it so that it does not depend on isolation where it can (#507). The describe memory test measures in a child process, but under -n it can still be disturbed by the load of other workers. Locally, pytest -q -n auto is the fast run; plain pytest -q still works.

To restore the previous arrangement at the public cut-over, when Actions is free: the header of .github/workflows/ci.yml lists the exact edits. In short, pull_request and push are still in on:, so re-add the schedule cron (06:00 UTC) and revert the job if: lines (the Windows job ran only -m "windows or compliance" on a PR and the full suite nightly; seed the env cache ran on push to main), and set SSVEP_RENDER_FIGURES on the schedule again. Whether to also require tests (windows, ssvep env) and tests (linux, ssvep env) in the ruleset is a decision, not a restoration: the ruleset before this change required only the Windows one, and the Linux one was only ever in §3's table. The live ruleset has already been updated to require only compliance and mkdocs (the exported ruleset file, applied with the gh api -X PUT command in §3), so no PR waits on a test job that no longer reports. The conda-env caches are saved on main only (.github/actions/ssvep-env, one definition every job uses, so the saved key is the restored key); while only dispatch runs, a dispatch from main saves them straight after the env gate and before any test runs. A red dispatched run shows on the Actions tab.

One more required check lives in its own workflow, .github/workflows/docs.yml:

Job	Runs	Why
mkdocs build --strict	ubuntu, `requirements-docs.txt` only	Builds the ssvep-toolbox.org site. `--strict` turns a broken link or a nav entry pointing at a missing file into a build failure, so the PR fails instead of the published site. Deployment is Cloudflare Pages, not this job — CI's only purpose is to fail before Cloudflare does. Needs none of `mne`/`brainflow`/`PySide6`: mkdocstrings reads the source statically rather than importing it.

⚠️ A required check must never be path-filtered. docs.yml deliberately has no paths: filter. On a PR touching none of the filtered paths the job would not run, the required check would never report, and GitHub would hold the PR at "Expected — waiting for status" — unmergeable with nothing red to explain why. The job is ~15 s of pure-Python installs; always running it is far cheaper than that failure mode. Same applies to anything else added to the required list.

Why CI, when `.gitignore` already ignores the data¶

.gitignore stops the accident, and for that it is enough — a stray .xdf under sourcedata/ cannot be staged by a plain git add. It cannot do two things:

It is advisory and local. git add -f bypasses it (tested, not assumed), and once a path is tracked .gitignore is never consulted for it again. A contributor with a stale checkout has a stale guard. CI checks the tree that actually reaches main.
It cannot see inside a file, so it can only ever be a check on paths. That mattered most for BIDS/participants.tsv, which used to be deliberately un-ignored as part of the tracked scaffold — at once the file guaranteed to reach a US-hosted repo and the one that must carry no identifiers. It is no longer tracked: it grows a row per participant, so tracking it made a PR a precondition for recording one, and the row is only ever sub-XXX + n/a. CI now asserts it has not been re-added (git add -f can still put it back, and a tracked path ignores .gitignore entirely). The de-identification checks moved with the file: they now run against the working dataset in a local pytest -q, and skip in a clean checkout where it is absent.

No participant data is exposed by running CI: the runners check out only what is already on GitHub, and no participant data is there — which is what the compliance job enforces. CI must never be given NAS or compute-server credentials.

5. What a reviewer must check¶

CI now covers the mechanical half of this — a red build blocks the merge, so you are not the first line of defence on filenames. What is left needs judgement:

Is the data guard still true in spirit? CI matches known patterns and known columns. It cannot recognise participant data in a shape nobody anticipated — a .csv of trial responses, a screenshot with a name in it, an identifier inside a JSON sidecar. Git history is durable and GitHub is US-hosted; that is the failure a revert does not fix.
Was ALLOWED_PARTICIPANT_COLUMNS edited? Loosening the compliance test is how the guard dies quietly. A PR that adds a column and allowlists it is doing the thing the test exists to make someone think about — so think about it.
Docs moved with the change (docs/REQUIREMENTS.md + environment.yml for a dependency; the relevant docs/ page for a design shift).
A changelog.d/ fragment was added for a user-visible change, or the PR says why one wasn't needed.

Adversarial review before merge¶

Every PR an agent authors is reviewed by a model other than the one that wrote it, before it merges. The rule is different model, not second opinion. A model reviewing its own work carries the blind spots that produced the work, so it reliably fails to notice the thing it already failed to notice; the point is that someone else's failure modes are doing the looking.

Spawn a review agent with the model set explicitly to one other than the author's. The code-review skill is the natural channel — it reviews along two axes, whether the change follows this repo's documented standards and whether it matches what the originating issue asked for — but only use it for this if you have confirmed it lets you pin the reviewing model. A skill that quietly reviews with the author's own default satisfies the letter of the rule and none of its purpose.

Implement each finding unless it is wrong or harmful, and reply in the thread when you decline one, so a declined finding is distinguishable from an unread one. A PR is not ready to merge until every finding has been implemented or answered.

A reviewer reads the diff and little else — not AGENTS.md, not the REB constraints, not why a decision is locked. That makes it good at what a diff shows (a stale cross-reference, an overstated claim, an unhandled branch) and blind to what it does not (that a suggested refactor would let analysis infer design from data, that a "simplification" would drop a consent check). Judge each one; do not batch-accept and do not batch-dismiss.

What this rule does not do¶

Nothing enforces it. There is no CI check and no review attestation — .github/workflows/ holds ci.yml and docs.yml and nothing that could verify a review happened. The rule rests on the agent doing the merging, which is the same agent it declines to trust as its own reviewer. A real gap, named here rather than papered over; if it starts being skipped, the answer is a required attestation, not a firmer sentence in this file.

"Different model" is a proxy. What is actually wanted is independent failure modes, and two checkpoints of one lineage can share the blind spot the rule exists to defeat. Prefer the most distant model available and treat the requirement as a floor, not a guarantee.

A human-authored PR has a different actor. A lab contributor cannot run an agent skill. Their PR is not exempt — request @copilot review on it on their behalf, or review it yourself. What is forbidden is a PR merging with no reader but its author.

Copilot's review, on demand¶

Copilot used to review every PR here automatically. It no longer does, and the reason is worth knowing because anyone can walk back into it: a Copilot PR review costs 13 premium requests — about $0.52 once the monthly allowance is gone — and at this project's rate it exhausted the account's budget in September 2026. For the current rate, count rather than trusting a number written here:

gh api "repos/NeuroCognitiveImagingLab/ssvep-toolbox/pulls?state=all&per_page=100"   --jq '[.[] | select(.created_at >= "2026-09-01")] | length'

What followed was three required checks failing to start with "the job was not started because recent account payments have failed or your spending limit needs to be increased", main red for reasons nothing in the code explained, and every PR unmergeable.

Which of those two causes actually fired is not established. There was no payment method on file at all, and the AI-credit budget was spent, and the error names both without distinguishing them. The tidy story — "Copilot spending blocked Actions" — is not supported, and repeating it would be exactly the drift this project is worst at. The honest lesson is narrower and still worth keeping: a review budget and CI can fail together, and a red main is not always about the code.

These are also two billing accounts, not one: Actions for this repo bills to the organization that owns it, Copilot to the person whose subscription is charged. Diagnosing one from the other is how the wrong fix gets applied — a card added to a personal account does not pay for an org repo's runners.

Where the automatic-review switch actually lives: https://github.com/settings/copilot, in the personal Copilot settings of whoever's subscription is charged. Not in the repository's rulesets (main-protected carries no Copilot review rule and no required reviewers), and not in an organization Copilot page — this org has a Copilot configuration (gh api orgs/<org>/copilot/billing returns plan_type: business) but zero assigned seats, so it is not what pays for these reviews. The review is billed to a person, so it is configured by that person: counter-intuitive for a repository-wide behaviour, which is why it is written down.

It remains genuinely good, and worth spending on deliberately. Request it with @copilot review on:

changes to a compliance guard, the manifest spine, or the publication path — it caught a credential leak (#207), a hole in the built-site leak scan (#206), and four real defects in the versioned-docs build (#217);
a lab contributor's PR, where it is the only review that arrives without someone asking.

⚠️ This is a real trade against the compliance guardrails, not a free change. Automatic review meant every PR touching REB-relevant ground got a second reader whether or not its author recognised the sensitivity. On demand, that coverage depends on the author noticing — the same self-assessment blind spot the adversarial-review rule above exists to remove, reintroduced for compliance detection specifically. The adversarial review is the mitigation and it is weaker than "always on". Accepted knowingly; revisit if a compliance defect ever reaches main.

Its comments are easy not to see: gh pr view <N> --comments shows issue comments only, and Copilot leaves a review plus inline review comments, which live on different endpoints. Both are paginated at 30, so --paginate is not optional on a PR that has been round-tripped a few times — without it the check silently reads the first page and calls it the whole review.

REPO=NeuroCognitiveImagingLab/ssvep-toolbox

# the summary review ("Changes recommended" / "Looks good")
gh api --paginate repos/$REPO/pulls/<N>/reviews    --jq '.[] | select(.body != "") | {user: .user.login, state, body}'

# the inline comments, with the file and line each one is anchored to
gh api --paginate repos/$REPO/pulls/<N>/comments   --jq '.[] | {user: .user.login, path, line, body}'

Copilot reviews once, when asked — it does not re-review after a later push unless asked again. So read it after your last push, and never wait for a re-review that is not coming. A comment whose line is null is anchored to an outdated diff position: the code may already have been fixed by a later commit, so check the current file before acting on it, or before assuming it still stands.