diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..6313b56 --- /dev/null +++ b/.gitattributes @@ -0,0 +1 @@ +* text=auto eol=lf diff --git a/.githooks/pre-commit b/.githooks/pre-commit index fa0a3d4..1583be0 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -1,2 +1,2 @@ #!/bin/sh -exec python3 "$(git rev-parse --show-toplevel)/bin/check.py" +exec python3 "$(dirname "$0")/../bin/check.py" --root "$(git rev-parse --show-toplevel)" diff --git a/.githooks/pre-push b/.githooks/pre-push index d64a6be..e4e51bd 100755 --- a/.githooks/pre-push +++ b/.githooks/pre-push @@ -1,2 +1,2 @@ #!/bin/sh -exec python3 "$(git rev-parse --show-toplevel)/bin/check.py" --pre-push +exec python3 "$(dirname "$0")/../bin/check.py" --root "$(git rev-parse --show-toplevel)" --pre-push diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 38b5653..94aa9de 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -4,11 +4,18 @@ on: push: pull_request: +permissions: + contents: read + jobs: check: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + repository: ourPLCC/change-requests + path: change-requests - uses: actions/setup-python@v5 with: python-version: "3.12" @@ -22,6 +29,7 @@ jobs: python3 -m unittest discover -s tests -t . python3 -m unittest discover -s migrate/tests -t . - name: Consistency check - run: python3 bin/check.py --pre-push + run: python3 bin/check.py --pre-push --root change-requests - name: Backlog.md doctor + working-directory: change-requests run: backlog doctor diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 06791cd..29ee8c7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -16,7 +16,8 @@ Repo docs link to this guide rather than restating it, even to agree with it. reports a problem or suggests an idea by opening an issue on the relevant code repo. - **Change requests (CRs)** are the work we commit to, tracked centrally in - this repo with [Backlog.md](https://github.com/MrLesk/Backlog.md). A CR is + [ourPLCC/change-requests](https://github.com/ourPLCC/change-requests) with + [Backlog.md](https://github.com/MrLesk/Backlog.md). A CR is work in exactly one repo with testable acceptance criteria, and has an ID `CR-N`. Work spanning repos is one CR per repo, linked by dependencies. - **Drafts** are ideas we have not committed to. Never cite one. @@ -58,3 +59,100 @@ guide — filing, statuses, IDs, references, checks — is in the repo's git log. A CR's type is the commit type of the change that decides its version impact (`fix` → patch, `feat` → minor); its branch may carry other commit types too. + +## Working in a devcontainer + +Each ourPLCC code repo has a devcontainer. In it, you and any agent you run +act on GitHub as you, through one token of yours, and GitHub's settings, not +the agent's instructions, set the limits. The design and its reasons are in +[the devcontainer permissions spec](dev-docs/specs/2026-10-13-devcontainer-permissions-design.md). + +### Your token + +1. On GitHub, create a fine-grained personal access token (Settings → + Developer settings → Personal access tokens → Fine-grained tokens): + - Resource owner: `ourPLCC`. Repository access: all repositories. + - Repository permissions: Contents read and write, and Pull requests read + and write. Nothing else. + - Expiration: 90 days at most. +2. Wait for an org owner to approve it; until then it cannot reach ourPLCC. +3. In any ourPLCC container, run `gh auth login --with-token` and paste the + token. Every ourPLCC container on the machine shares the login, and it + survives rebuilds. + +Never use plain `gh auth login`: its browser flow yields a token for every +repo you can reach. When the token expires, make a new one, have it approved, +and load it again. To revoke it, delete it on GitHub; to forget it on one +machine, run `docker volume rm ourplcc-gh` on the host. + +Keep no SSH key that GitHub accepts loaded in your host's SSH agent; on the +host, use HTTPS with Git Credential Manager instead. VS Code forwards that +agent into every container, no setting turns it off, and the devcontainer can +only hide it, not block it, so a key there would let the container push +anywhere you can. + +### What the container cannot do + +From the container you can push branches, open and update pull requests, and +push tracker changes with `/workspaces/dev/bin/push.bash`. You cannot push to +any repo's `main` except `change-requests`', to `gh-pages`, or tags; push a +change to `.github/workflows/` (except from a Codespace; see below); run or +re-run workflows; or change settings, +rulesets, environments, or secrets. Do those in GitHub's web UI, or on the +host with your own credentials. + +Merging a pull request is yours alone: in a code repo, merging to `main` is +the release. Agents never merge. + +### Approving a release + +A release reaches PyPI only after you approve it. Open the release's workflow +run in the repo's Actions tab, choose **Review deployments**, select `pypi`, +and approve. The token cannot approve, so no agent can. + +### Codespaces + +A Codespace uses its own GitHub token; do not run `gh auth login` there. When +you create one, GitHub asks you to authorize access to `ourPLCC/dev` and +`ourPLCC/change-requests`: authorize it, or both stay read-only and +`bin/push.bash` fails. A full rebuild of a Codespace deletes its volumes and +any tracker commits you have not pushed, so push right away. + +If you use an agent regularly, use it in a local container, not a Codespace. +A Codespace's token can push changes to `.github/workflows/`, which your own +token cannot, so an agent misled by text it reads could push a workflow that +runs with write access to the repo. An agent for a one-off task in a Codespace +is fine; watch what it pushes. + +A Codespace's token expires about 12 hours after the Codespace starts. If a +push or `gh` command then fails to authenticate, restart the Codespace. + +### Moving from the host clone + +Before the devcontainers cloned `dev` into a volume, it lived on the host +beside the code repos, and tracker changes were pushed from there. Move once: + +1. In the host's `../dev`, check that `git status`, + `git log --branches --not --remotes`, `git worktree list`, and + `git stash list` come back clean. `dev`'s `main` no longer takes direct + pushes, so re-enter any tracker change they show by hand, in the + container, after step 3. +2. Create your token and wait for its approval. +3. Rebuild one ourPLCC container; it clones `dev` and `change-requests` into + their volumes. Run `gh auth login --with-token`, then push your next + tracker change with `/workspaces/dev/bin/push.bash`. +4. On a Windows host, once each code repo's branch has the + `* text=auto eol=lf` line in its `.gitattributes`, commit or stash your + work there, since the next command discards uncommitted changes. Then + check the repo out again (`git rm -r --cached -q . && git reset --hard`), + or clone it fresh. +5. Delete the host's `../dev`. Do not check out a branch of any repo from + before the move, or its devcontainer recreates `../dev`. +6. On the host, push over HTTPS with Git Credential Manager. It comes with + Git for Windows; on macOS, `brew install --cask git-credential-manager`. + ourPLCC admits only approved apps, and GCM is the one approved for + pushing, so a token another app stored fails with a 403. Erase it with + `printf 'protocol=https\nhost=github.com\n\n' | git credential reject`, + then push again and sign in through the browser. If your host clones use + SSH remotes, switch them to HTTPS and remove your GitHub key from the SSH + agent (`ssh-add -d `). diff --git a/ORG-AGENTS.md b/ORG-AGENTS.md index 0a22b6d..38242a8 100644 --- a/ORG-AGENTS.md +++ b/ORG-AGENTS.md @@ -19,3 +19,8 @@ because the reader is an agent. (`backlog task edit CR-N --append-notes "…"`), never as new CRs. - When closing the CR, triage those notes with the human: new draft, new CR, fold into an existing CR, or drop. +- Never merge a pull request. +- Never run `gh auth login`, `gh auth refresh`, or `gh auth token`; if `gh` + is not logged in, stop and tell the human. +- Push `change-requests` only through `dev`'s `bin/push.bash`, and only to + publish tracker changes the human approved. diff --git a/README.md b/README.md index f5c4338..b84f563 100644 --- a/README.md +++ b/README.md @@ -1,13 +1,15 @@ -# ourPLCC developer docs and change requests +# ourPLCC developer docs How we build ourPLCC, for maintainers, their agents, and new developers: the developer guide shared by every ourPLCC repository, org-wide specs, agent -rules, and the central change-request tracker for work on plcc-ng, -languages-ng, plcc-ng-demo, plcc-ng-devcontainer, and course-materials-ng. +rules, and the tooling for the change-request tracker. The tracker itself, for +work on plcc-ng, languages-ng, plcc-ng-demo, plcc-ng-devcontainer, and +course-materials-ng, is +[ourPLCC/change-requests](https://github.com/ourPLCC/change-requests). **Reporting a problem or suggesting an idea?** Open a GitHub issue on the relevant code repository instead. GitHub Issues are where humans talk to us; -this repository is where maintainers track the resulting work. +the tracker is where maintainers track the resulting work. **Start with [CONTRIBUTING.md](CONTRIBUTING.md)**, the developer guide. diff --git a/bin/check.py b/bin/check.py index 2ae4809..8866b18 100755 --- a/bin/check.py +++ b/bin/check.py @@ -1,8 +1,8 @@ #!/usr/bin/env python3 """Consistency checks for the ourPLCC change-request tracker. -Standard library only: this runs on maintainers' hosts (pre-push hook) as -well as in containers, without Node or Backlog.md. +Standard library only: this runs from the hooks and in CI without Node or +Backlog.md. --root names the tracker repo (ourPLCC/change-requests). """ import argparse import re @@ -71,6 +71,8 @@ def _as_list(value): def check(root, pre_push=False): root = Path(root) backlog = root / "backlog" + if not (backlog / "config.yml").is_file(): + return [f"{root}: no backlog/config.yml; --root must name the tracker repo"] config = load_config(backlog / "config.yml") projects = _as_list(config.get("projects")) types = _as_list(config.get("types")) @@ -132,7 +134,8 @@ def rel(path): def main(argv=None): parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) - parser.add_argument("--root", default=str(Path(__file__).resolve().parent.parent)) + parser.add_argument("--root", required=True, + help="root of the tracker repo, which holds backlog/") parser.add_argument("--pre-push", action="store_true", help="also fail on uncommitted changes under backlog/") args = parser.parse_args(argv) diff --git a/bin/push.bash b/bin/push.bash new file mode 100755 index 0000000..77fc240 --- /dev/null +++ b/bin/push.bash @@ -0,0 +1,78 @@ +#!/usr/bin/env bash +# Push the change-request tracker (ourPLCC/change-requests): fetch, rebase +# onto origin/main, pass bin/check.py --pre-push, push. Holds a lock on the +# ourplcc-dev volume's mount point, /workspaces/dev, the same lock the code +# repos' bin/install/shared-clones.bash takes. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +DEV_ROOT="$(dirname "${SCRIPT_DIR}")" +TRACKER="${OURPLCC_CHANGE_REQUESTS:-/workspaces/change-requests}" +LOCK_DIR="${OURPLCC_LOCK_DIR:-/workspaces/dev}" + +die() { + echo "push.bash: $*" >&2 + exit 1 +} + +git -C "${TRACKER}" rev-parse --verify --quiet HEAD >/dev/null 2>&1 \ + || die "${TRACKER} is not a git clone of ourPLCC/change-requests" \ + "(missing, empty, or an unfinished clone);" \ + "rerun bin/install/shared-clones.bash" + +# Re-run under the lock. -o keeps the lock's descriptor in flock alone, so a +# child that outlives this script (git maintenance, detached by fetch) does +# not hold it; -w turns a stuck holder into an error. +# OURPLCC_LOCK_DIR and OURPLCC_LOCK_WAIT are for tests. +if [ -z "${OURPLCC_PUSH_LOCKED:-}" ]; then + status=0 + OURPLCC_PUSH_LOCKED=1 flock -o -w "${OURPLCC_LOCK_WAIT:-120}" -E 75 \ + "${LOCK_DIR}" bash "${BASH_SOURCE[0]}" "$@" || status=$? + [ "${status}" -ne 75 ] \ + || die "timed out waiting for the lock on ${LOCK_DIR};" \ + "another push or clone holds it" + exit "${status}" +fi +unset OURPLCC_PUSH_LOCKED + +cd "${TRACKER}" + +rebase_in_progress() { + [ -d "$(git rev-parse --git-path rebase-merge)" ] \ + || [ -d "$(git rev-parse --git-path rebase-apply)" ] +} + +! rebase_in_progress \ + || die "a rebase is in progress in ${TRACKER}; finish it" \ + "(git rebase --continue) or run 'git rebase --abort', then run this again" +[ "$(git symbolic-ref --quiet --short HEAD || true)" = main ] \ + || die "${TRACKER} is not on main; check out main first" +[ -z "$(git status --porcelain)" ] \ + || die "${TRACKER} has uncommitted changes; commit or discard them first" + +git fetch --quiet origin +if ! git rebase --quiet origin/main; then + rebase_in_progress \ + || die "rebase onto origin/main failed (see git's message above);" \ + "nothing changed; run this again" + git rebase --abort \ + || die "rebase onto origin/main conflicted and could not be aborted;" \ + "run 'git rebase --abort' in ${TRACKER}" + die "rebase onto origin/main conflicted; aborted, local commits kept." \ + "Resolve with 'git pull --rebase' in ${TRACKER}, then run this again" +fi + +python3 "${DEV_ROOT}/bin/check.py" --pre-push --root "${TRACKER}" \ + || die "bin/check.py failed; nothing pushed. For a duplicate id," \ + "run 'backlog doctor --fix', commit, and run this again" + +if [ "$(git rev-list --count origin/main..HEAD)" -eq 0 ]; then + echo "push.bash: nothing to push" + exit 0 +fi + +git push --quiet origin main \ + || die "push failed (see git's message above). If another push landed" \ + "since the fetch, run this again; if it is an authentication error," \ + "see 'Your token' in dev's CONTRIBUTING.md" +echo "push.bash: pushed" diff --git a/dev-docs/specs/2026-10-13-devcontainer-permissions-design.md b/dev-docs/specs/2026-10-13-devcontainer-permissions-design.md index 548f69d..3fdc16a 100644 --- a/dev-docs/specs/2026-10-13-devcontainer-permissions-design.md +++ b/dev-docs/specs/2026-10-13-devcontainer-permissions-design.md @@ -89,7 +89,7 @@ permission rules, the spec says so. ## What a container may do The same in a local container and in a Codespace, except where "Codespaces" -below narrows it. +below narrows or widens it. | Operation | Human | Agent | Enforced by | | --- | --- | --- | --- | @@ -98,11 +98,12 @@ below narrows it. | Force-push or delete `change-requests`' `main` | no | no | branch ruleset | | Push to any other repo's `main` (`dev` included), or to `gh-pages` | no | no | branch rulesets | | Push, move, or delete a tag | no | no | tag rulesets | +| Create, edit, or delete a draft release | yes | yes | — (publishing one creates its tag, which the tag rulesets block) | | Open, update, comment on, or close a PR | yes | yes | — | | Merge a PR | yes | **no** | ORG-AGENTS.md and Claude Code deny rules only | | Publish a package | no | no | `pypi` environment's required reviewer | | Run code in CI by pushing a branch or opening a PR | yes | yes | — (CI tokens are read-only; see "Workflows") | -| Push a change to `.github/workflows/` | no | no | token lacks Workflows | +| Push a change to `.github/workflows/` | no (yes in a Codespace) | no (yes in a Codespace) | token lacks Workflows; see "Codespaces" | | Dispatch or re-run workflows; change settings, rulesets, environments, or secrets | no | no | token lacks Actions, Administration, Environments, Secrets | | Anything in a repo outside ourPLCC | no | no | token's resource owner | | Log `gh` in with another credential | no | no | ORG-AGENTS.md and Claude Code deny rules only | @@ -156,11 +157,17 @@ Set by an org owner, before any container gets push access. an org owner approves each one. Maximum lifetime: 90 days. - Classic personal access tokens are blocked for the org, so a classic token pasted into `gh` cannot reach ourPLCC. -- OAuth app access is left as it is. Restricting it would also cut off the - host's own Git Credential Manager, which the host needs for the work the - container cannot do. The cost: a `gh auth login` taken through the browser - default yields an unscoped token. The `--with-token`-only instruction, the - deny rules, and Verification step 9 stand in for that restriction. +- Third-party (OAuth) application access is **restricted**, as it already + was on 2026-10-27: only approved apps reach ourPLCC's data beyond what is + public. Git Credential Manager is approved, so a host pushes over HTTPS + with GCM's browser sign-in for the work the container cannot do; an org + owner grants it the first time they sign in with it. No other app is + approved for pushing, the GitHub CLI's included, so a `gh auth login` + taken through the browser default yields a token that cannot push to + ourPLCC. The `--with-token`-only instruction, the deny rules, and + Verification step 9 remain as a backstop. A token another app left in a + host's keychain (VS Code's, an old GCM's) fails the same way, and the + contributor erases it and signs in with GCM again. ### Workflows @@ -189,11 +196,11 @@ Per repo: org rulesets need GitHub Team, which ourPLCC does not have. "Apply the rulesets" is therefore a step in creating any ourPLCC repo. The release Apps are the only bypass actors; nobody else bypasses, org owners included, with one exception: course-materials-ng's `main` also lets its -maintainer Alain Kägi (`alainkaegi`) bypass, because he maintains it by -pushing to `main` directly. Its `main` still needs the ruleset for everyone -else: `deploy.yml` runs on every push to `main` with the release App's token, -which reaches every repo in the App's installation. The remaining risk is -that one contributor's token. +maintainer bypass, because they maintain it by pushing to `main` directly. +Its `main` still needs the ruleset for everyone else: `deploy.yml` runs on +every push to `main` with the release App's token, which reaches every repo +in the App's installation. The remaining risk is that one contributor's +token. - **`main`**, every repo except `change-requests` (`dev` included): require a pull request, block force-pushes, block deletion. @@ -273,9 +280,21 @@ Codespaces GPG verification for ourPLCC repos, and are unsigned otherwise. `"git.terminalAuthentication": false` and `"github.gitAuthentication": false` in `customizations.vscode.settings`. A push with no token fails instead of borrowing an unscoped one. -- Does not expose the host's SSH agent: SSH remotes would push with the host's - keys, unscoped. The likely mechanism is `"remoteEnv": {"SSH_AUTH_SOCK": ""}`; - see "Open checks". +- Hides the host's SSH agent, which SSH remotes would push through with the + host's keys, unscoped. VS Code forwards the agent into every container as + `/tmp/vscode-ssh-auth-.sock`, recreates the socket on every window + reload, and has no setting that turns this off. So the devcontainer sets + `"remoteEnv": {"SSH_AUTH_SOCK": ""}`, which stops `git` and `ssh` finding + the agent by default, and + `"postAttachCommand": "rm -f /tmp/vscode-ssh-auth-*.sock"`, which removes + the socket after each attach. That leaves the socket reachable while + `postStartCommand` runs, on every start (Dev Containers runs + `postAttachCommand` only after it), in the moment between an attach and + the removal, and to a process that survives a window reload. Hiding the + agent is therefore not the boundary. The rule is: keep no SSH key that + GitHub accepts loaded in the host's agent, and use HTTPS with Git + Credential Manager on the host. That rests on the contributor, and + `CONTRIBUTING.md` says so. - Uses one `remoteUser`, `vscode`, with UID 1000, like every other ourPLCC devcontainer, because the shared volumes keep real UIDs. @@ -309,9 +328,34 @@ same token. (for branches and PRs to `dev`); the Codespace's own repo is granted by default. GitHub asks for this grant when the Codespace is created. Choosing to continue without it leaves both read-only, so the docs say to authorize - it. A change to the grant reaches only Codespaces created afterwards. + it. A change to the grant reaches only Codespaces created afterwards. The + grant names a repo that must exist: before the split, GitHub lists + `ourPLCC/change-requests` as not available, and does not follow its + redirect to `dev`. +- The token is fixed in each process's environment when the process starts, + and it expires about 12 hours after the Codespace starts; a restart issues + a new one. Codespaces' own credential helper reads the same environment + variable, so a stock Codespace behaves the same way, and deferring to it + would gain nothing. So once a Codespace has run about 12 hours, pushes and + `gh` may fail to authenticate until it restarts. Whether a new terminal + gets a fresh token sooner is untested. A Codespace stops after its idle + timeout, and a stop ends every process, so this takes a Codespace kept + active that long. - So in a Codespace the scope is that repo plus `dev` and `change-requests`, - narrower than local. The table above still holds within that scope. + narrower than local. The table above holds within that scope, with one + exception: the Codespace's token can push a change to `.github/workflows/` + in its own repo, and no setting narrows it. The org's read-only + `GITHUB_TOKEN` is only a default, which a workflow's own `permissions:` + block can raise, so a workflow pushed on a branch can run with write access + to that repo. That reaches its unprotected branches, issues, and PRs, which + the token reaches anyway; its packages, which matters where people pull + them, as with plcc-ng-devcontainer's images; and `actions: write`, which can + start workflows on `main`, running reviewed code with the `release` + environment's secrets. The rulesets, the `release` environment's refusal of + other branches, and the `pypi` reviewer still hold. This is accepted, and + limited by advice rather than enforcement: `CONTRIBUTING.md` says to run + agents regularly in a local container, and to use one in a Codespace only + for a one-off task, watched. ## Where the shared clones live @@ -510,12 +554,20 @@ describes the volumes in place of the `../dev` clone, and drops plcc-ng's (`git rm -r --cached -q . && git reset --hard`) or clone it fresh. 5. Delete the host's `../dev`. Do not check out a pre-migration branch of any repo afterwards, or its devcontainer recreates it. +6. On the host, push over HTTPS through Git Credential Manager, erasing any + token another app stored (see "Tokens"). Switch SSH remotes to HTTPS and + remove any GitHub key from the host's SSH agent (see "Every + devcontainer"). ## Verification -Each code repo's CR passes this checklist in a local container on a Linux or -macOS host, in a local container on a Windows host, and in a newly created -Codespace. +The first repo to adopt the shared setup, plcc-ng, passes this checklist in a +local container on a Linux or macOS host, in a local container on a Windows +host, and in a newly created Codespace. The other repos' devcontainer changes +copy plcc-ng's, so each of their CRs instead passes a smoke test in a rebuilt +local container: push a branch and open a PR with `gh`, list CRs with +`backlog task list`, push a tracker change with `bin/push.bash`, and see a +direct push to `main` rejected. Must succeed: @@ -530,7 +582,8 @@ Must fail: 4. A direct push to the repo's `main`, to `dev`'s `main`, and to `gh-pages` if the repo has one. 5. A force-push to `change-requests`' `main`. -6. A push of a commit touching `.github/workflows/`. +6. A push of a commit touching `.github/workflows/`, locally. In a Codespace + it succeeds; see "Codespaces". 7. Pushing any tag; `gh release create` for a new tag. 8. `gh workflow run` on any workflow. @@ -547,17 +600,22 @@ Must hold: ## Open checks -Settled during step 1 of "Rollout" or the first repo's CR. A result that -contradicts this spec is fixed here, before the spec is frozen. - -- `remoteEnv` with an empty `SSH_AUTH_SOCK` stops git and `ssh` from reaching - the forwarded agent; if not, another way to stop VS Code forwarding it. -- The Codespace token cannot push workflow changes, create releases, or push - `gh-pages`. If it can, the table records the difference for Codespaces. -- A long-running process in a Codespace keeps working after the Codespace's - `GITHUB_TOKEN` is refreshed. If not, the helper becomes a small script that - defers to Codespaces' own helper when `$CODESPACES` is set. -- Codespaces honors `type=volume` mounts under `/workspaces`. +All settled on plcc-ng's branch before the split, on a macOS host and in a +Codespace; the sections above already reflect each result. + +- An empty `SSH_AUTH_SOCK` hides the forwarded agent but does not block it: + the socket stays in `/tmp`, and pointing `ssh-add` at it reaches the host. + Removing the socket in `postAttachCommand` closes that path except for the + gaps "Every devcontainer" lists, so the boundary is the rule about the + host's agent. +- The Codespace token can push a change to `.github/workflows/` and create a + draft release; it cannot push a tag or push `gh-pages`, which the rulesets + reject. Publishing a release was not tried; Verification step 7 covers it. +- A process in a Codespace keeps the token it started with, which expires + about 12 hours after the Codespace starts. Codespaces' own helper reads the + same variable, so the planned fallback to it is dropped. +- Codespaces honors `type=volume` mounts under `/workspaces`, and they + survive a restart. - `flock /workspaces/dev` in one local container blocks it in another container that mounts the same volume. @@ -580,7 +638,7 @@ contradicts this spec is fixed here, before the spec is frozen. | Token in a host environment variable via `${localEnv:…}` | VS Code launched from a GUI often does not see the shell's environment, and the setup differs on every host OS | | `gh auth setup-git` | Writes `~/.gitconfig`, which VS Code replaces with the host's copy | | Token with the Workflows permission | A workflow on a pushed branch runs before review; workflow edits are rare enough to do outside the container | -| Restrict OAuth app access for the org | Also blocks the host's Git Credential Manager, which the host needs for what the container cannot do | +| Lift the org's restriction on OAuth app access | Any app a contributor authorizes, the GitHub CLI's browser login included, would reach ourPLCC with an unscoped token; approving Git Credential Manager alone gives hosts what they need | | Keep tracker data in `dev`, restricting direct pushes to `backlog/` with a push ruleset | Push rulesets are not available to ourPLCC | | Keep tracker data in `dev`, open to direct pushes, guarded by `bin/push.bash` | A rule an injected agent can skip, on the path that sets every agent's instructions and every container's hooks | | Tracker data on an orphan branch of `dev`, checked out as a worktree | Same protection as a separate repo, but orphan branches and worktrees are unfamiliar to new contributors, and GitHub shows `main` by default | diff --git a/dev-docs/tracker.md b/dev-docs/tracker.md index a691a46..6736bf0 100644 --- a/dev-docs/tracker.md +++ b/dev-docs/tracker.md @@ -2,7 +2,9 @@ The central tracker for work on the ourPLCC code repositories: plcc-ng, languages-ng, plcc-ng-demo, plcc-ng-devcontainer, and course-materials-ng. It -is managed with [Backlog.md](https://github.com/MrLesk/Backlog.md). This is +is managed with [Backlog.md](https://github.com/MrLesk/Backlog.md). Its data +lives in [ourPLCC/change-requests](https://github.com/ourPLCC/change-requests); +its checks and tooling live here, in `dev`. This is the full guide to using it; the [developer guide](../CONTRIBUTING.md) gives the short version and the workflow around it. @@ -12,17 +14,21 @@ this guide wins. ## Setup -- Keep the ourPLCC repos as siblings on your host, with this repo cloned - beside them as `dev/`. Each code repo's devcontainer clones it there if - missing, bind-mounts it at `/workspaces/dev`, sets - `BACKLOG_CWD=/workspaces/dev`, and installs the pinned - `backlog` CLI. -- Commit inside the container; **push from the host**. Backlog.md commits - every change automatically (`auto_commit: true`). +- Each code repo's devcontainer clones this repo into the shared volume + `ourplcc-dev`, mounted at `/workspaces/dev`, and the tracker data, + [ourPLCC/change-requests](https://github.com/ourPLCC/change-requests), + into `ourplcc-change-requests`, mounted at `/workspaces/change-requests`. + Every ourPLCC container on a machine shares both. It sets + `BACKLOG_CWD=/workspaces/change-requests` and installs the pinned `backlog` + CLI, and on start fast-forwards `dev`'s `main` when it is checked out and + clean. +- Commit and push inside the container, with `/workspaces/dev/bin/push.bash`, + after every tracker change. Backlog.md commits every change automatically + (`auto_commit: true`); the volume is not a backup. - Hooks in `.githooks/` run `bin/check.py` (Python 3, standard library) on - commit and push; `core.hooksPath` is set by the devcontainer's - `initializeCommand`. -- Hosts: Linux or macOS. + commit and push; the devcontainer sets `change-requests`' `core.hooksPath` + to `/workspaces/dev/.githooks`, so the hooks come from reviewed code. +- Hosts: Linux, macOS, or Windows; or a Codespace. ## Vocabulary @@ -59,7 +65,8 @@ this guide wins. decision that binds more than one repo); update the CR's acceptance criteria to match and add the spec as a reference: `backlog task edit CR-N --add-ref ../../../plcc-ng/dev-docs/specs/.md` - (for this repo: `../../dev-docs/specs/.md`). Specs do not cite CRs. + (for this repo: `../../../dev/dev-docs/specs/.md`). Specs do not cite + CRs. 2. **Plan and execute:** the plan lives in `.plans/` inside the worktree (gitignored) and is deleted with it. Plans are not kept. During automated execution, record discovered work in the CR's notes — @@ -102,7 +109,7 @@ dependency as satisfied, so review the CRs that depended on it. - **Demote** (`backlog task demote CR-N`) only when nothing cites the CR: ```bash - grep -rn 'CR-N\b' /workspaces/dev/backlog /workspaces/ --include='*.md' + grep -rn 'CR-N\b' /workspaces/change-requests/backlog /workspaces/ --include='*.md' git -C /workspaces/ log --all --oneline -i --grep 'cr-N\b' ``` @@ -124,7 +131,8 @@ version impact; its branch may carry other commit types too. Every CR has exactly one `project`: `plcc-ng`, `languages-ng`, `plcc-ng-demo`, `plcc-ng-devcontainer`, `course-materials-ng`, or `dev` (this repo: the org -docs and specs, the tracker's config, checks, CI, and migration tooling). Work spanning repos is one +docs and specs, and the tracker's checks, CI, and migration tooling; the +config is `change-requests`' `backlog/config.yml`). Work spanning repos is one CR per repo linked with `--dep` (no umbrella CRs); `backlog task list --ready` hides a CR until its dependencies are done. @@ -164,15 +172,38 @@ projects before filing. ## Pushing and collisions -Push from the host. If the push is rejected, `git pull --rebase`, then -`python3 bin/check.py`. If it reports a duplicate ID (two people allocated the -same number before either pushed), run `backlog doctor --fix` in a container -to renumber the unpushed CR, commit, and push. CI runs the same check plus -`backlog doctor` on every push; it can only alert, so fix problems by revert. +Push with `/workspaces/dev/bin/push.bash`. It takes a lock that every ourPLCC +container on the machine shares, fetches, rebases your tracker commits onto +`origin/main`, runs `bin/check.py --pre-push`, and pushes. It stops without +pushing, and says why, when: + +- `change-requests` has uncommitted changes: commit or discard them. +- The rebase conflicts: it aborts the rebase and keeps your commits. Run + `git pull --rebase` in `/workspaces/change-requests`, resolve, and run it + again. +- `bin/check.py` fails. A duplicate ID means two people allocated the same + number before either pushed: run `backlog doctor --fix` to renumber the + unpushed CR, commit, and run it again. +- The push is rejected because another push landed after the fetch: run it + again. +- It waited two minutes for the lock: another push or a container start + holds it. Run it again; if it keeps timing out, look for a stuck + `bin/push.bash` or `shared-clones.bash` in your other containers. + +Do not edit the tracker from another container while a push runs. Backlog.md +does not take the lock, so its commit can land in the middle of the rebase +and be dropped. + +Locally, every container shares one tracker tree, so one person's sessions +never allocate the same ID. Two Codespaces, or a Codespace and a local +container, are two trees and can collide like two people. Nothing on GitHub +checks a direct push to `change-requests`; the next `bin/push.bash` fails on +a bad one and names the problem, and `dev`'s CI checks `change-requests` on +every push to `dev`. Fix problems by revert. ## Checks (`bin/check.py`) -Fails on: duplicate IDs (drafts included) or a filename that does not match its `id`; a +`bin/check.py --root ` fails on: duplicate IDs (drafts included) or a filename that does not match its `id`; a `DRAFT-N` cited outside its own file; a CR without exactly one configured `project` or `type`; an unconfigured label; anything in `archive/`; and, with `--pre-push`, uncommitted changes under `backlog/`. `CR-999` is exempt from @@ -187,7 +218,9 @@ the project and type rules. 3. Fold relevant changes into this guide (and, where they touch agent rules, into [ORG-AGENTS.md](../ORG-AGENTS.md)) and update the version it was checked against — in the same commit. -4. Rebuild devcontainers to pick up the new version. +4. Once the change merges, rebuild every ourPLCC container to pick up the new + version; each warns on start while its `backlog` differs from + `.backlog-version`. ## Migration tooling @@ -221,15 +254,20 @@ the references that pointed into `dev` when the tracker data moved to ## Working on the tracker tooling `bin/check.py` and `migrate/` use only the Python 3 standard library, so the -checks run on hosts as well as in containers. Run what CI runs -(`.github/workflows/check.yml`) before pushing a tooling change: +hooks run in every container without extra installs. Changes to `dev` go +through a pull request; its `main` takes no direct pushes. In a container, +run what CI runs (`.github/workflows/check.yml`) before pushing a tooling +change: ```bash python3 -m unittest discover -s tests -t . python3 -m unittest discover -s migrate/tests -t . -python3 bin/check.py --pre-push -backlog doctor +python3 bin/check.py --pre-push --root /workspaces/change-requests +(cd /workspaces/change-requests && backlog doctor) ``` A new rule in `bin/check.py` gets a test in `tests/test_check.py` and a line in [Checks](#checks-bincheckpy) above. + +`bin/push.bash`'s tests in `tests/test_push.py` build throwaway repos; change +them with the script. diff --git a/tests/test_check.py b/tests/test_check.py index fc14e70..1f6a687 100644 --- a/tests/test_check.py +++ b/tests/test_check.py @@ -142,6 +142,47 @@ def test_cli_exit_code_on_error(self): self.assertEqual(result.returncode, 1) self.assertIn("check: ", result.stderr) + def test_cli_requires_root(self): + result = subprocess.run( + [sys.executable, str(ROOT / "bin" / "check.py")], + capture_output=True, text=True) + self.assertEqual(result.returncode, 2) + self.assertIn("--root", result.stderr) + self.assertNotIn("Traceback", result.stderr) + + def test_root_without_tracker_is_a_clear_error(self): + empty = self.root / "not-a-tracker" + empty.mkdir() + result = subprocess.run( + [sys.executable, str(ROOT / "bin" / "check.py"), "--root", str(empty)], + capture_output=True, text=True) + self.assertEqual(result.returncode, 1) + self.assertIn("no backlog/config.yml", result.stderr) + self.assertNotIn("Traceback", result.stderr) + + def hook(self, name): + git = ["git", "-c", "user.name=t", "-c", "user.email=t@t", "-C", str(self.root)] + subprocess.run(git + ["init", "-q"], check=True) + subprocess.run(git + ["add", "-A"], check=True) + subprocess.run(git + ["commit", "-q", "-m", "x"], check=True) + return subprocess.run(["sh", str(ROOT / ".githooks" / name)], + cwd=self.root, capture_output=True, text=True) + + def test_pre_commit_hook_checks_the_repo_that_runs_it(self): + self.add("tasks", "cr-1000 - A.md", record("CR-1000", project="nope")) + result = self.hook("pre-commit") + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("project", result.stderr) + + def test_pre_push_hook_checks_the_repo_that_runs_it(self): + result = self.hook("pre-push") + self.assertEqual(result.returncode, 0, result.stderr) + self.add("tasks", "cr-1000 - A.md", record("CR-1000")) + result = subprocess.run(["sh", str(ROOT / ".githooks" / "pre-push")], + cwd=self.root, capture_output=True, text=True) + self.assertEqual(result.returncode, 1) + self.assertIn("uncommitted change under backlog/", result.stderr) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_push.py b/tests/test_push.py new file mode 100644 index 0000000..b226070 --- /dev/null +++ b/tests/test_push.py @@ -0,0 +1,262 @@ +import fcntl +import os +import subprocess +import tempfile +import time +import unittest +from pathlib import Path + +from tests.test_check import CONFIG, record + +ROOT = Path(__file__).resolve().parent.parent +PUSH = ROOT / "bin" / "push.bash" + + +class PushTest(unittest.TestCase): + def setUp(self): + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + self.tmp = Path(tmp.name) + self.origin = self.tmp / "origin.git" + self.tracker = self.tmp / "tracker" + self.other = self.tmp / "other" + # Isolate from the user's git config (signing, helpers, hooksPath). + self.lock_dir = self.tmp / "lock" + self.lock_dir.mkdir() + self.env = {k: v for k, v in os.environ.items() + if not k.startswith(("GIT_", "OURPLCC_"))} + self.env.update( + GIT_CONFIG_GLOBAL=os.devnull, + GIT_CONFIG_NOSYSTEM="1", + GIT_AUTHOR_NAME="Test", + GIT_AUTHOR_EMAIL="test@example.com", + GIT_COMMITTER_NAME="Test", + GIT_COMMITTER_EMAIL="test@example.com", + OURPLCC_CHANGE_REQUESTS=str(self.tracker), + OURPLCC_LOCK_DIR=str(self.lock_dir), + ) + self.git("init", "-q", "--bare", "-b", "main", str(self.origin)) + seed = self.tmp / "seed" + self.git("init", "-q", "-b", "main", str(seed)) + self.write(seed, "backlog/config.yml", CONFIG) + self.write(seed, "backlog/completed/cr-999 - Seed.md", + record("CR-999", project=None, type_=None)) + self.commit(seed, "Seed") + self.git("-C", str(seed), "push", "-q", str(self.origin), "main") + self.git("clone", "-q", str(self.origin), str(self.tracker)) + self.git("clone", "-q", str(self.origin), str(self.other)) + + def git(self, *args): + return subprocess.run(["git", *args], env=self.env, check=True, + capture_output=True, text=True).stdout + + def write(self, repo, rel, text): + path = repo / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + + def commit(self, repo, message): + self.git("-C", str(repo), "add", "-A") + self.git("-C", str(repo), "commit", "-q", "-m", message) + + def add_cr(self, repo, n, title="Title"): + self.write(repo, f"backlog/tasks/cr-{n} - {title}.md", record(f"CR-{n}")) + self.commit(repo, f"Create task CR-{n} {title}") + + def push_other(self): + self.git("-C", str(self.other), "push", "-q", "origin", "main") + + def run_push(self): + return subprocess.run([str(PUSH)], env=self.env, capture_output=True, text=True) + + def origin_log(self): + return self.git("--git-dir", str(self.origin), "log", "--format=%s", "main") + + def test_pushes_local_commits(self): + self.add_cr(self.tracker, 1000) + result = self.run_push() + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("Create task CR-1000 Title", self.origin_log()) + + def test_rebases_onto_commits_pushed_elsewhere(self): + self.add_cr(self.other, 1001) + self.push_other() + self.add_cr(self.tracker, 1000) + result = self.run_push() + self.assertEqual(result.returncode, 0, result.stderr) + log = self.origin_log() + self.assertIn("Create task CR-1000 Title", log) + self.assertIn("Create task CR-1001 Title", log) + self.assertEqual(self.git("-C", str(self.tracker), "rev-parse", "HEAD"), + self.git("--git-dir", str(self.origin), "rev-parse", "main")) + + def test_nothing_to_push_exits_zero(self): + result = self.run_push() + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("nothing to push", result.stdout) + + def test_refuses_uncommitted_changes(self): + self.add_cr(self.tracker, 1000) + self.write(self.tracker, "backlog/tasks/cr-1001 - Title.md", record("CR-1001")) + result = self.run_push() + self.assertNotEqual(result.returncode, 0) + self.assertIn("uncommitted", result.stderr) + self.assertNotIn("CR-1000", self.origin_log()) + + def test_rebase_conflict_aborts_and_keeps_local_commits(self): + self.write(self.other, "backlog/config.yml", CONFIG + "statuses: [a]\n") + self.commit(self.other, "Other config") + self.push_other() + self.write(self.tracker, "backlog/config.yml", CONFIG + "statuses: [b]\n") + self.commit(self.tracker, "Tracker config") + result = self.run_push() + self.assertNotEqual(result.returncode, 0) + self.assertIn("conflicted", result.stderr) + self.assertFalse((self.tracker / ".git" / "rebase-merge").exists()) + self.assertFalse((self.tracker / ".git" / "rebase-apply").exists()) + self.assertEqual( + self.git("-C", str(self.tracker), "log", "-1", "--format=%s").strip(), + "Tracker config") + self.assertNotIn("Tracker config", self.origin_log()) + + def test_duplicate_id_after_rebase_blocks_push(self): + self.add_cr(self.other, 1000, title="Other") + self.push_other() + self.add_cr(self.tracker, 1000) + result = self.run_push() + self.assertNotEqual(result.returncode, 0) + self.assertIn("duplicate id CR-1000", result.stderr) + self.assertIn("backlog doctor --fix", result.stderr) + self.assertNotIn("CR-1000 Title", self.origin_log()) + + def test_rejected_push_says_run_again(self): + hook = self.origin / "hooks" / "pre-receive" + hook.parent.mkdir(exist_ok=True) + hook.write_text("#!/bin/sh\nexit 1\n") + hook.chmod(0o755) + self.add_cr(self.tracker, 1000) + result = self.run_push() + self.assertNotEqual(result.returncode, 0) + self.assertIn("run this again", result.stderr) + + def test_refuses_branch_other_than_main(self): + self.git("-C", str(self.tracker), "switch", "-q", "-c", "topic") + self.add_cr(self.tracker, 1000) + result = self.run_push() + self.assertNotEqual(result.returncode, 0) + self.assertIn("not on main", result.stderr) + self.assertNotIn("CR-1000", self.origin_log()) + + def test_refuses_detached_head(self): + self.git("-C", str(self.tracker), "switch", "-q", "--detach") + result = self.run_push() + self.assertNotEqual(result.returncode, 0) + self.assertIn("not on main", result.stderr) + + def test_missing_tracker_clone_explains(self): + self.env["OURPLCC_CHANGE_REQUESTS"] = str(self.tmp / "missing") + result = self.run_push() + self.assertNotEqual(result.returncode, 0) + self.assertIn("not a git clone", result.stderr) + + def test_empty_tracker_dir_explains(self): + self.env["OURPLCC_CHANGE_REQUESTS"] = str(self.lock_dir) + result = self.run_push() + self.assertNotEqual(result.returncode, 0) + self.assertIn("not a git clone", result.stderr) + + def test_unfinished_clone_explains(self): + partial = self.tmp / "partial" + self.git("init", "-q", "-b", "main", str(partial)) + self.git("-C", str(partial), "remote", "add", "origin", str(self.origin)) + self.env["OURPLCC_CHANGE_REQUESTS"] = str(partial) + result = self.run_push() + self.assertNotEqual(result.returncode, 0) + self.assertIn("not a git clone", result.stderr) + + def test_rebase_failing_without_conflict_explains(self): + self.add_cr(self.other, 1001) + self.push_other() + self.add_cr(self.tracker, 1000) + (self.tracker / ".git" / "index.lock").write_text("") + result = self.run_push() + self.assertNotEqual(result.returncode, 0) + self.assertIn("push.bash: rebase onto origin/main failed", result.stderr) + self.assertFalse((self.tracker / ".git" / "rebase-merge").exists()) + self.assertNotIn("CR-1000", self.origin_log()) + + def test_rebase_in_progress_says_finish_it(self): + self.write(self.other, "backlog/config.yml", CONFIG + "statuses: [a]\n") + self.commit(self.other, "Other config") + self.push_other() + self.write(self.tracker, "backlog/config.yml", CONFIG + "statuses: [b]\n") + self.commit(self.tracker, "Tracker config") + self.git("-C", str(self.tracker), "fetch", "-q") + subprocess.run(["git", "-C", str(self.tracker), "rebase", "origin/main"], + env=self.env, capture_output=True) + result = self.run_push() + self.assertNotEqual(result.returncode, 0) + self.assertIn("a rebase is in progress", result.stderr) + self.assertTrue((self.tracker / ".git" / "rebase-merge").exists()) + + def test_failed_push_points_at_git_and_the_token(self): + hook = self.origin / "hooks" / "pre-receive" + hook.parent.mkdir(exist_ok=True) + hook.write_text("#!/bin/sh\nexit 1\n") + hook.chmod(0o755) + self.add_cr(self.tracker, 1000) + result = self.run_push() + self.assertIn("see git's message above", result.stderr) + self.assertIn("authentication", result.stderr) + + def test_waits_for_the_lock(self): + self.add_cr(self.tracker, 1000) + fd = os.open(self.lock_dir, os.O_RDONLY) + try: + fcntl.flock(fd, fcntl.LOCK_EX) + proc = subprocess.Popen([str(PUSH)], env=self.env, + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) + self.addCleanup(proc.kill) + time.sleep(0.5) + self.assertIsNone(proc.poll(), "push.bash ran while the lock was held") + self.assertNotIn("CR-1000", self.origin_log()) + finally: + fcntl.flock(fd, fcntl.LOCK_UN) + os.close(fd) + _, stderr = proc.communicate(timeout=30) + self.assertEqual(proc.returncode, 0, stderr) + self.assertIn("Create task CR-1000 Title", self.origin_log()) + + def test_gives_up_on_a_stuck_lock(self): + self.add_cr(self.tracker, 1000) + self.env["OURPLCC_LOCK_WAIT"] = "1" + fd = os.open(self.lock_dir, os.O_RDONLY) + try: + fcntl.flock(fd, fcntl.LOCK_EX) + result = self.run_push() + finally: + fcntl.flock(fd, fcntl.LOCK_UN) + os.close(fd) + self.assertNotEqual(result.returncode, 0) + self.assertIn("timed out waiting for the lock", result.stderr) + self.assertNotIn("CR-1000", self.origin_log()) + + def test_child_that_outlives_it_does_not_keep_the_lock(self): + # Stands in for the git maintenance a fetch detaches. + hook = self.tracker / ".git" / "hooks" / "pre-push" + hook.parent.mkdir(exist_ok=True) + hook.write_text("#!/bin/sh\n(sleep 5 >/dev/null 2>&1