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 (docs/ruleset-main.json, 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 zeuron-inc 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 docs/ruleset-main.json, 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) — 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¶

Edit docs/ruleset-main.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 zeuron-inc 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).

Copilot's automated review¶

Copilot reviews every PR here automatically. Before merging, read it and act on it — implement each suggestion unless it is wrong or harmful, and reply in the thread when you decline one, so a declined comment is distinguishable from an unread one.

The catch is that 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 reads the diff and nothing else — not CLAUDE.md, not the REB constraints, not why a decision is locked. It is good at the things a diff shows (a stale cross-reference, an overstated claim, an unhandled branch) and blind to the things 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.

It reviews asynchronously after a push, so check after your last commit lands, not before.