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.

⚠️ 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. --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`	All three CI jobs must pass before merge — `compliance (no protected data)`, `tests (windows, ssvep env)` (§4) and `mkdocs build --strict` (the docs site; a broken link or a stale nav entry fails the PR rather than the published site)
`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 (MaxMascini, ZAnaKarenZ, LauraEar1): branch → PR → need one approval → merge. They cannot self-merge.
Admins (aaronjnewman, HamidMandi): 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 (CLAUDE.md §5 — this repository's own Claude-Code layer, which a published snapshot does not carry) — it does not wait for Aaron. 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 — 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 runs on every PR and every push to main. Two jobs, kept separate on purpose:

Job	Runs	Why
compliance	ubuntu, ~30 s, stdlib + pytest only	`tests/test_compliance_data.py`: nothing matching the protected patterns is tracked, and `BIDS/participants.tsv` has not been re-added to the index. No conda — a data-governance gate must not fail open because an unrelated dependency broke.
tests	windows-latest, real `ssvep` conda env	`pytest -q`. Windows because that is what the acquisition PCs run, and where the ctypes amp drivers and the GL renderer live.

A third 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).

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.