diff --git a/.claude/settings.json b/.claude/settings.json index fd1d312..a5bb9ed 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -5,7 +5,7 @@ "hooks": [ { "type": "command", - "command": "n=$(ls \"${CLAUDE_PROJECT_DIR:-.}\"/.review/notes/*.json 2>/dev/null | wc -l); if [ \"$n\" -gt 0 ]; then echo \"$n open review notes in .review/notes/ (skill handling-review-notes)\"; fi; true" + "command": "p=\"${CLAUDE_PROJECT_DIR:-.}\"; g=$(git -C \"$p\" rev-parse --path-format=absolute --git-common-dir 2>/dev/null); d=\"$(dirname \"$g\")/.papercuts/papercuts.db\"; if [ -n \"$g\" ] && [ -f \"$d\" ]; then n=$(cd \"$p\" && PAPERCUTS_STORE=local PAPERCUTS_DB=\"$d\" papercuts list 2>/dev/null | sed -n 's/^count: \\([0-9]*\\) of.*/\\1/p'); if [ \"${n:-0}\" -gt 0 ]; then echo \"$n open papercuts of the review site (skill handling-review-papercuts)\"; fi; fi; exit 0" } ] } diff --git a/.claude/skills/handling-review-notes/SKILL.md b/.claude/skills/handling-review-notes/SKILL.md deleted file mode 100644 index 41e339e..0000000 --- a/.claude/skills/handling-review-notes/SKILL.md +++ /dev/null @@ -1,20 +0,0 @@ ---- -name: handling-review-notes -description: Use when Tim mentions notes or feedback on the review site (the tab »Note«), when the session start says there are open review notes, or when you change scripts/review/ or docker-compose.dev.yml. ---- - -# Handling review notes - -Tim writes notes on the review site: the tab »Note« at the right edge. The review site is this clone, served by `scripts/review/devserver.py` in the container `t7d-review` (`docker-compose.dev.yml`, host name in `.env` as `REVIEW_HOST`). It shows the checked-out branch at once and loads the page again when a file changes. A note is a JSON file in `.review/notes/`. - -1. The SessionStart hook (`.claude/settings.json`) prints the number of open notes in `.review/notes/`. Tell Tim how many are open, and list them with `ls .review/notes/*.json`. -2. Read the note: `note`, `path`, `viewport`, `theme`, `scroll`, `target` (`selector`, `text`, `rect`), `selection`, `branch`, `commit`. -3. Run `uv run --no-project --with playwright python3 scripts/review/show.py ` (more ids in one call). Look at `.review/notes/.png`. The red frame is the part that Tim pointed at. -4. Do the work with the rules of this file: a branch, `DESIGN.md`, the look at three widths. -5. After the fix, run `show.py --suffix after` and compare the two pictures. -6. Add an `"answer"` field to the note: what changed, and the commit. Move the note to `.review/notes/done/`. Tim sees the answer under »My notes« in the tab. - -- Keep notes and their pictures out of git. `.review/` and `.env` are ignored. -- A change to `devserver.py` needs `docker restart t7d-review`. A change to `review.js` or `review.css` needs only a page load. -- The server adds only `connect-src 'self'` to the CSP of a page. Keep it that way, so that the review site keeps the rules of the real site. -- Tests: `python3 -m unittest scripts/review/test_devserver.py`. diff --git a/.claude/skills/handling-review-papercuts/SKILL.md b/.claude/skills/handling-review-papercuts/SKILL.md new file mode 100644 index 0000000..c941793 --- /dev/null +++ b/.claude/skills/handling-review-papercuts/SKILL.md @@ -0,0 +1,30 @@ +--- +name: handling-review-papercuts +description: Use when Tim mentions papercuts or feedback on the review site, when the session start says there are open papercuts, or when you change scripts/review/ or docker-compose.dev.yml. +--- + +# Handling review papercuts + +Tim writes papercuts on the review site with the papercuts widget. The review site is this clone, served by `scripts/review/devserver.py` in the container `t7d-review` (`docker-compose.dev.yml`, host name in `.env` as `REVIEW_HOST`). A second container, `t7d-review-papercuts`, is the relay. It writes the local store `.papercuts/papercuts.db` in this clone. Each papercut keeps the page, the branch, the commit and a picture. + +`.github/papercuts.json` sets `"store": "local"`, so `papercuts add`, `list`, `show`, `comment` and `done` use the local store in every worktree. A worktree uses the `.papercuts/` folder of the main clone. No export is needed. Never run these commands with `PAPERCUTS_STORE=github`: that files a GitHub issue on a public repo. + +1. The SessionStart hook (`.claude/settings.json`) prints the number of open papercuts. Tell Tim how many are open, and run `papercuts list`. +2. Run `papercuts show ` to read one papercut: the text, the page, the branch, the commit and the picture. +3. Do the work in a worktree for the branch of the papercut, in `~/code/worktrees/t7d/`. Follow the rules of this file: a branch, `DESIGN.md`, the look at three widths. +4. A fix is one commit on that branch. One commit is one round on the review site. Tim sees the round in the version picker. +5. Run `papercuts comment ""` if Tim needs an explanation. +6. Run `papercuts done --fix ""` after the commit. + +## Approve + +Do not run `python3 scripts/review/approve.py ` unless Tim tells you to, in this conversation or in a papercut. The command merges the branch and publishes it to t7d.dev. + +- If Tim tells you to approve, run `python3 scripts/review/approve.py ` in your worktree. It writes the request to the main clone. +- The command prints each step of the host. It stops with exit 0 when the round is published. +- A finished text is not an approval. Tell Tim that the round is ready, and let him press Approve. + +- Keep the store and its pictures out of git. `.papercuts/`, `.review/` and `.env` are ignored. +- A change to `devserver.py` needs `docker restart t7d-review`. A change to `review.js` or `review.css` needs only a page load. +- The server adds only `connect-src 'self'` to the CSP of a page, and the origin of the relay when `PAPERCUTS_URL` is set. Keep it that way, so that the review site keeps the rules of the real site. +- Tests: `python3 -m unittest discover -s scripts/review -p 'test_*.py'` and `node --test 'scripts/review/test_*.mjs'`. diff --git a/.github/papercuts.json b/.github/papercuts.json new file mode 100644 index 0000000..6fef006 --- /dev/null +++ b/.github/papercuts.json @@ -0,0 +1,26 @@ +{ + "$schema": "https://raw.githubusercontent.com/Tim-Conrad/papercuts/main/schema/papercuts.schema.json", + "fields": [ + { + "id": "expected", + "label": "What did you expect?", + "required": false + } + ], + "presets": [ + { + "kind": "bug", + "text": "This does not work" + }, + { + "kind": "ux", + "text": "This is hard to use on the phone" + }, + { + "kind": "copy", + "text": "This text is wrong or unclear" + } + ], + "labels": [], + "store": "local" +} diff --git a/.gitignore b/.gitignore index 53916dc..f0768c8 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,7 @@ __pycache__/ /thoughts/feed.xml /blog/feed.xml .review/ +.papercuts/ .env .preview/ diff --git a/CLAUDE.md b/CLAUDE.md index 27b526d..1ee056d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -22,11 +22,15 @@ t7d.dev: the personal site of Tim Conrad. Plain HTML, CSS and a little JavaScrip - `blog/` and `CVE/` only redirect old addresses (the thoughts were a blog until November 2026). Do not add pages there. -Show a changed page as a Claude artifact, not a local server: `python3 scripts/preview-artifact.py ` builds the bundle; publish `/index.html` with the Artifact tool and pass the other printed files in `files`. Republish the same `file_path` after each change, so the link stays. The review site (skill `handling-review-notes`) is where Tim leaves notes on his own. +Show a changed page as a Claude artifact, not a local server: `python3 scripts/preview-artifact.py ` builds the bundle; publish `/index.html` with the Artifact tool and pass the other printed files in `files`. Republish the same `file_path` after each change, so the link stays. The review site (skill `handling-review-papercuts`) is where Tim leaves papercuts on his own. - -Use the skill `handling-review-notes`. A hook prints the number of open notes at the start of a session. + +Use the skill `handling-review-papercuts`. A hook prints the number of open papercuts at the start of a session. + + + +Do not run `python3 scripts/review/approve.py ` unless Tim tells you to, in this conversation or in a papercut. The command merges the branch and publishes it to t7d.dev. If you think that a text is ready, tell Tim. He presses Approve on the review site. diff --git a/README.md b/README.md index 088f814..f7d13f5 100644 --- a/README.md +++ b/README.md @@ -43,16 +43,16 @@ It is plain HTML, CSS and a little JavaScript. There is no build step and no dep | `scripts/ship.sh` | Merges a PR, waits for the deploy and checks t7d.dev | | `scripts/preview-artifact.py` | Builds a page as a Claude artifact bundle, for a review before it is published | | `scripts/reels/` | Scripts that film the two games and cut the reels | -| `scripts/review/`, `docker-compose.dev.yml` | The review site: live reload and a note tab. Never published | +| `scripts/review/`, `docker-compose.dev.yml` | The review site: live reload, rounds of a branch, the papercuts widget and the Approve button. Never published | | `.github/workflows/deploy.yml` | Runs the deploy on every push to `master` | | `DESIGN.md` | How the site looks and sounds: colours, type, layout, components and the wording rules | | `CLAUDE.md` | Instructions for Claude Code in this repository | | `.claude/skills/updating-product-reels/` | Working guide for Claude Code: how to take the reels and pictures again | | `.claude/skills/adding-a-thought/` | Working guide for Claude Code: how to turn a link into a new thought, and how to revise one | | `.claude/skills/updating-game-timeline/` | Working guide for Claude Code: how to film the timeline clips of a game page | -| `.claude/skills/handling-review-notes/` | Working guide for Claude Code: how to handle Tim's notes from the review site | +| `.claude/skills/handling-review-papercuts/` | Working guide for Claude Code: how to handle Tim's papercuts from the review site | | `.claude/rules/pages.md` | Rules for a new page; Claude Code loads them only when it works on an HTML page, the sitemap or the deploy script | -| `.claude/settings.json` | A hook that prints the number of open review notes at the start of a session | +| `.claude/settings.json` | A hook that prints the number of open papercuts at the start of a session | | `scripts/check.py` | Checks the CSP rules and the sitemap of every page; CI runs it | ## Preview locally @@ -63,19 +63,35 @@ python3 -m http.server 8000 Then open . All paths are absolute, so serve from the repository root. -For live reload and the note tab, run `python3 scripts/review/devserver.py` instead (port 8000). +For live reload, run `python3 scripts/review/devserver.py` instead (port 8000). ## Review site -The review site serves this clone with live reload and a tab »Note« at the right edge. A note keeps the page, the width, the theme and the part that you pointed at. It is a JSON file in `.review/notes/`, and git ignores that folder. The site files on `gh-pages` do not change: `scripts/deploy.sh` never publishes `scripts/`. +The review site serves this clone and its site worktrees with live reload. The site files on `gh-pages` do not change: `scripts/deploy.sh` never publishes `scripts/`. + +Each change is a branch with its own worktree in `~/code/worktrees/t7d/`. Each commit on the branch is one round. Open a page with `?branch=` to see that branch. The page then keeps the branch in a cookie. + +The bar at the bottom of each page shows the branch and two versions: master, a round, or "Working" for changes that are not committed. By default it compares the newest round with the round before it. "Show changes" marks the changed text in the page: removed words are struck out, new words are underlined. On a phone the bar is one row. A tap on the comparison opens the selects. + +Each page also has the papercuts widget. A papercut keeps the page, the branch, the commit and a picture. A relay container writes it to the local store `.papercuts/papercuts.db` of this clone. `.github/papercuts.json` sets `"store": "local"`, so `papercuts list`, `show` and `done` use that store in every worktree, with no env. Git ignores `.papercuts/`. + +"Approve round " in the bar publishes the newest round of the branch. It writes a request to `.review/approve/`. On the host, `scripts/review/approve-host.sh` checks the head and the worktree and runs `scripts/check.py`. Then it pushes the branch, opens the PR, runs `scripts/ship.sh `, and removes the worktree and the branch. The bar shows each step from `.review/approve-status/`. An agent can do the same with `python3 scripts/review/approve.py `, also from its worktree. An agent starts Approve only when Tim tells it to, in that conversation or in a papercut. To run it in a container behind a login proxy: -1. Make the notes folder: `mkdir -p .review/notes`. -2. Write the host name into `.env`: `REVIEW_HOST=`. -3. Start the stack: `docker-compose -p t7d-review -f docker-compose.dev.yml up -d`. +1. Install the host side of the Approve button once: `scripts/review/approve-install.sh`. It needs `gh` on PATH. It makes `.review/approve/`, `.review/approve-status/` and a systemd user unit that starts `approve-host.sh` for each request. To keep the button working after a reboot, run `loginctl enable-linger developer` once. +2. Make the store folder and the worktrees folder: `mkdir -p .papercuts ~/code/worktrees/t7d`. + + Run steps 1 and 2 before step 4. If the folders do not exist, Docker makes them as root. +3. Write the settings into `.env`: + - `REVIEW_HOST`: the host name of the review site. + - `REVIEW_PAPERCUTS_HOST`: the host name of the relay. + - `REVIEW_MAIN`: the host path of this clone. + - `REVIEW_WORKTREES`: the host path of the folder with the site worktrees (`~/code/worktrees/t7d`, as a full path). + - `PAPERCUTS_SRC`: the folder of the relay source with its Dockerfile. +4. Start the stack: `docker-compose -p t7d-review -f docker-compose.dev.yml up -d --build`. -`scripts/review/show.py ` opens the page of a note and frames its part in a picture. A handled note goes to `.review/notes/done/` with an `"answer"` field, which the tab shows under »My notes«. Tests: `python3 -m unittest scripts/review/test_devserver.py`. +Tests: `python3 -m unittest discover -s scripts/review -p 'test_*.py'` and `node --test 'scripts/review/test_*.mjs'`. ## Deploy diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index bb7a199..e0ea1d1 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -1,22 +1,32 @@ -# The review site: this clone with live reload and the note tab (scripts/review/devserver.py), in the lab behind its +# The review site: this clone with live reload and the papercuts widget (scripts/review/devserver.py), in the lab behind its # login. GitHub Pages still serves t7d.dev from gh-pages. This stack never touches it. # -# mkdir -p .review/notes # first: else Docker makes the folder as root -# echo REVIEW_HOST= > .env # the host name of the review site (.env and .review/ are gitignored) -# docker-compose -p t7d-review -f docker-compose.dev.yml up -d +# # first, else Docker makes the folders as root: +# mkdir -p .review/approve .review/approve-status .papercuts ~/code/worktrees/t7d +# scripts/review/approve-install.sh # once: the host side of the Approve button (a systemd user unit) +# # .env (gitignored): REVIEW_HOST=, REVIEW_MAIN=, +# # REVIEW_WORKTREES= +# # REVIEW_PAPERCUTS_HOST=, PAPERCUTS_SRC= +# docker-compose -p t7d-review -f docker-compose.dev.yml up -d --build # docker-compose -p t7d-review -f docker-compose.dev.yml down # -# The clone is read-only in the container. Only .review/notes is writable: one JSON file per note. -# A checkout of another branch in this folder shows that branch at once, with its name in the note tab. +# The clone and the worktrees are read-only in the container, at their host paths (git needs the same paths). +# The review server writes only the approve request to .review/approve. It reads the status of the host from .review/approve-status. +# The relay container writes only to .papercuts (the local store of papercuts). +# The page shows the branch of ?branch=, else of the cookie, else of the clone. The image adds only the package git. services: t7d-review: - image: python:3.12-slim + build: + context: scripts/review + dockerfile: Dockerfile + image: t7d-review:local container_name: t7d-review restart: unless-stopped user: "1001:1001" - command: [python3, /repo/scripts/review/devserver.py, --root, /repo, --notes, /repo/.review/notes, --host, 0.0.0.0, --port, "8000"] + command: [python3, "${REVIEW_MAIN:?set REVIEW_MAIN in .env}/scripts/review/devserver.py", --root, "${REVIEW_MAIN}", --worktrees, "${REVIEW_WORKTREES}", --host, 0.0.0.0, --port, "8000"] environment: PYTHONDONTWRITEBYTECODE: "1" + PAPERCUTS_URL: https://${REVIEW_PAPERCUTS_HOST:?set REVIEW_PAPERCUTS_HOST in .env} read_only: true cap_drop: [ALL] security_opt: [no-new-privileges:true] @@ -25,8 +35,10 @@ services: - /tmp:size=8m,mode=1777 logging: { driver: json-file, options: { max-size: "10m", max-file: "3" } } volumes: - - .:/repo:ro - - ./.review/notes:/repo/.review/notes:rw + - ${REVIEW_MAIN}:${REVIEW_MAIN}:ro + - ${REVIEW_WORKTREES:?set REVIEW_WORKTREES in .env}:${REVIEW_WORKTREES}:ro + - ${REVIEW_MAIN}/.review/approve:${REVIEW_MAIN}/.review/approve:rw + - ${REVIEW_MAIN}/.review/approve-status:${REVIEW_MAIN}/.review/approve-status:ro networks: [proxy] # no ports: the lab proxy routes to it labels: - traefik.enable=true @@ -37,9 +49,39 @@ services: - traefik.http.services.t7d-review.loadbalancer.server.port=8000 - homepage.group=Apps - homepage.name=t7d.dev (Review) - - homepage.description=Working copy with live reload and notes + - homepage.description=Working copy with live reload and papercuts - homepage.href=https://${REVIEW_HOST} + t7d-review-papercuts: + build: ${PAPERCUTS_SRC:?set PAPERCUTS_SRC in .env} + container_name: t7d-review-papercuts + restart: unless-stopped + user: "1001:1001" + environment: + PAPERCUTS_STORE: local + PAPERCUTS_DB: /data/papercuts.db + PAPERCUTS_HOST: 0.0.0.0 + PAPERCUTS_ORIGINS: https://${REVIEW_HOST} + PAPERCUTS_REPOS: Tim-Conrad/Tim-Conrad.github.io + PAPERCUTS_PUBLIC_URL: https://${REVIEW_PAPERCUTS_HOST} + read_only: true + cap_drop: [ALL] + security_opt: [no-new-privileges:true] + mem_limit: 128m + tmpfs: + - /tmp + logging: { driver: json-file, options: { max-size: "10m", max-file: "3" } } + volumes: + - ${REVIEW_MAIN}/.papercuts:/data:rw + networks: [proxy] # no ports: the lab proxy routes to it + labels: + - traefik.enable=true + - traefik.http.routers.t7d-review-papercuts.rule=Host(`${REVIEW_PAPERCUTS_HOST}`) + - traefik.http.routers.t7d-review-papercuts.entrypoints=websecure + - traefik.http.routers.t7d-review-papercuts.tls=true + - traefik.http.routers.t7d-review-papercuts.middlewares=authelia@file,security-headers@file + - traefik.http.services.t7d-review-papercuts.loadbalancer.server.port=8000 + networks: proxy: external: true diff --git a/scripts/review/Dockerfile b/scripts/review/Dockerfile new file mode 100644 index 0000000..56ff572 --- /dev/null +++ b/scripts/review/Dockerfile @@ -0,0 +1,4 @@ +FROM python:3.12-slim +RUN apt-get update \ + && apt-get install -y --no-install-recommends git \ + && rm -rf /var/lib/apt/lists/* diff --git a/scripts/review/approve-host.sh b/scripts/review/approve-host.sh new file mode 100755 index 0000000..289c2da --- /dev/null +++ b/scripts/review/approve-host.sh @@ -0,0 +1,199 @@ +#!/usr/bin/env bash +# The host side of the Approve button and of approve.py: publish the approved round of a branch to t7d.dev. +# The systemd user unit t7d-review-approve.path (scripts/review/approve-install.sh) starts it when +# .review/approve/request.json exists in the main clone. It runs as the host user, with the GitHub login of that user. +# +# The steps, in this order. Each step goes to .review/approve-status/status.json before it runs: +# 1. Move the request into a private path in the status folder (the container cannot write there), then read it. +# A crash then cannot start the path unit again and again. Only a regular file counts, and only its first +# 4096 bytes are read: a folder, a symlink or a FIFO from the container cannot block or fill the host. +# A request that is not valid becomes state "failed" and is removed. +# 2. The sha of the request must be the head of the branch in the main clone. +# 3. The worktree of the branch must be clean. +# 4. python3 scripts/check.py in that worktree. Then steps 2 and 3 again: the check saw the approved commit only if +# nothing changed while it ran. +# 5. If a merged PR to master has the head , an earlier run failed after the merge: skip 6 and 7. +# 6. git push origin :refs/heads/, with no force. A rejected push stops. +# 7. gh pr create, if no open PR to master has this head (title: the subject of round 1, body: the list of rounds). +# 8. scripts/ship.sh in the main clone. It merges only if the head of the PR is still . +# 9. git worktree remove (no --force) and git branch -d. +# A failed step writes state "failed" with the step and the last line of its output, and removes nothing. +# SIGTERM or SIGINT (systemd stops the unit) writes state "failed" with the step and the line "killed". +# Each status has the branch, the sha and the time of its request ("request"), so a client can find its own. +# +# One run at a time: a second run waits for the lock (flock) of the first run, then reads its own request. +# It does not exit and leave the request, because the path unit then starts it again at once, in a loop. +# +# Env: REVIEW_MAIN (the main clone, default: the clone of this script), REVIEW_GH (default: gh), +# REVIEW_SHIP (default: $REVIEW_MAIN/scripts/ship.sh). The tests set them, and a dry run can too. +set -euo pipefail +here=$(cd "$(dirname "$(readlink -f "$0")")" && pwd) +main=${REVIEW_MAIN:-$(cd "$here/../.." && pwd)} +gh=${REVIEW_GH:-gh} +ship=${REVIEW_SHIP:-$main/scripts/ship.sh} +request=$main/.review/approve/request.json +status_dir=$main/.review/approve-status +log=$(mktemp) +trap 'rm -f "$log"' EXIT +branch="" sha="" asked="" pr="" wt="" now_step="" + +# status []: write status.json atomically (a temporary file, then a rename) +status() { + python3 -c ' +import datetime, json, os, sys +d, state, step, line, pr, branch, sha, asked = sys.argv[1:] +st = {"state": state, "step": step, "line": line, "pr": pr or None, "branch": branch, "sha": sha, "request": asked, + "time": datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ")} +tmp = os.path.join(d, ".status.%d.tmp" % os.getpid()) +with open(tmp, "w") as f: + json.dump(st, f) +os.replace(tmp, os.path.join(d, "status.json")) +' "$status_dir" "$1" "$2" "${3:-}" "$pr" "$branch" "$sha" "$asked" + echo "== $1: $2${3:+ ($3)}" +} + +# step : write the step, run the command, stop with state "failed" if it fails. +# The command runs in an "if", where set -e does not apply: each function returns on its own errors. +step() { + local name=$1 + shift + now_step=$name + status running "$name" + if ! "$@" >"$log" 2>&1; then + cat "$log" + status failed "$name" "$(grep -v '^[[:space:]]*$' "$log" | tail -n 1 || true)" + exit 1 + fi + cat "$log" +} + +read_request() { + local taken=$status_dir/.request.$$ req out + mv -T -- "$request" "$taken" || return 1 + if [ -L "$taken" ] || [ ! -f "$taken" ]; then + rm -rf -- "$taken" + echo "the request is not a regular file" + return 1 + fi + req=$(head -c 4096 -- "$taken") || true + rm -f -- "$taken" + out=$(python3 -c ' +import json, sys +sys.path.insert(0, sys.argv[1]) +import approve, gitread +try: + r = json.loads(sys.argv[2]) + b, s = r["branch"], r["sha"] +except (ValueError, KeyError, TypeError): + sys.exit("the request is not JSON with a branch and a sha") +if not isinstance(b, str) or not gitread.valid_branch(b) or b == approve.BASE: + sys.exit("the request has no valid branch") +if not isinstance(s, str) or not approve.SHA.match(s): + sys.exit("the request has no full sha") +t = r.get("time") +if not isinstance(t, str) or "\n" in t: + sys.exit("the request has no time") +print(b) +print(s) +print(t) +' "$here" "$req") || return 1 + { read -r branch; read -r sha; read -r asked; } <<<"$out" +} + +check_head() { + local now + now=$(git -C "$main" rev-parse --verify -q "refs/heads/$branch^{commit}") || { echo "no branch $branch"; return 1; } + [ "$now" = "$sha" ] || { echo "$branch is at ${now:0:7} now, not at the approved ${sha:0:7}"; return 1; } +} + +check_worktree() { + wt=$(python3 -c ' +import os, sys +sys.path.insert(0, sys.argv[1]) +import gitread +main, branch = sys.argv[2], sys.argv[3] +paths = [w["path"] for w in gitread.worktrees(main) if w["branch"] == branch] +if not paths or os.path.realpath(paths[0]) == os.path.realpath(main): + sys.exit("no worktree of %s apart from the main clone" % branch) +if gitread.dirty(paths[0]): + sys.exit("the worktree %s has changes that are not committed" % paths[0]) +print(paths[0]) +' "$here" "$main" "$branch") +} + +run_check() { (cd "$wt" && python3 scripts/check.py); } + +push() { git -C "$main" push origin "$sha:refs/heads/$branch"; } + +# rounds : the PR title (subject of round 1) or the PR body (the rounds and the attribution lines) +rounds() { + python3 -c ' +import sys +sys.path.insert(0, sys.argv[1]) +import approve, gitread +what, main, branch, sha = sys.argv[2:] +rounds = gitread.rounds(main, branch, approve.BASE) +if what == "title": + print(rounds[0]["subject"]) + sys.exit() +print("The rounds of %s, approved at round %d (%s) on the review site:\n" % (branch, len(rounds), sha[:7])) +for r in rounds: + print("%d. %s %s" % (r["n"], r["sha"][:7], r["subject"])) +print("\n\U0001F916 Generated with [Claude Code](https://claude.com/claude-code)") +' "$here" "$1" "$main" "$branch" "$sha" +} + +# find_pr : the URL of the PR to master in that state (OPEN or MERGED) whose head is the approved sha +find_pr() { + (cd "$main" && "$gh" pr list --base master --head "$branch" --state all --json url,headRefOid,state \ + --jq "map(select(.headRefOid == \"$sha\" and .state == \"$1\"))[0].url // empty") +} + +# merged_pr: sets pr if an earlier run merged this sha already +merged_pr() { + pr=$(find_pr MERGED) || return 1 + echo "${pr:-no merged PR has the head ${sha:0:7}}" +} + +# open_pr: the open PR whose head is the approved sha, else a new one +open_pr() { + local title body + pr=$(find_pr OPEN) || return 1 + if [ -z "$pr" ]; then + title=$(rounds title) || return 1 + body=$(rounds body) || return 1 + pr=$(cd "$main" && "$gh" pr create --base master --head "$branch" --title "$title" --body "$body" | tail -n 1) || return 1 + fi + [[ $pr =~ ^https://github\.com/.+/pull/[0-9]+$ ]] || { echo "no PR address from gh: \"$pr\""; pr=""; return 1; } + echo "$pr" +} + +run_ship() { (cd "$main" && "$ship" "${pr##*/}" "$sha"); } + +clean_up() { + git -C "$main" worktree remove "$wt" || return 1 + if git -C "$main" show-ref --verify -q "refs/heads/$branch"; then + git -C "$main" branch -d "$branch" + fi +} + +mkdir -p "$status_dir" +exec 9>"$status_dir/.lock" +flock 9 +# -L too: a symlink to a missing file is still a request to remove +[ -e "$request" ] || [ -L "$request" ] || exit 0 # the run before this one took the request while this one waited +trap 'status failed "$now_step" killed; exit 143' TERM INT +step "read the request" read_request +step "check that ${sha:0:7} is the head of $branch" check_head +step "check that the worktree of $branch is clean" check_worktree +step "run scripts/check.py" run_check +step "check that ${sha:0:7} is still the head of $branch" check_head +step "check that the worktree of $branch is still clean" check_worktree +step "look for a merged PR" merged_pr +if [ -z "$pr" ]; then + step "push $branch" push + step "open the PR" open_pr +fi +step "ship $pr" run_ship +step "remove the worktree and the branch" clean_up +status done "published $branch at ${sha:0:7}" diff --git a/scripts/review/approve-install.sh b/scripts/review/approve-install.sh new file mode 100755 index 0000000..cd6ffa8 --- /dev/null +++ b/scripts/review/approve-install.sh @@ -0,0 +1,47 @@ +#!/usr/bin/env bash +# Install the host side of the Approve button and of approve.py as systemd user units. +# Run it once on the lab host, in the main clone that the review container mounts: scripts/review/approve-install.sh +# Then: docker-compose -p t7d-review -f docker-compose.dev.yml up -d --build +# Remove it: systemctl --user disable --now t7d-review-approve.path && rm ~/.config/systemd/user/t7d-review-approve.* +set -euo pipefail +repo=$(cd "$(dirname "$(readlink -f "$0")")/../.." && pwd) +units=${XDG_CONFIG_HOME:-$HOME/.config}/systemd/user +gh=$(command -v gh) || { echo "approve-install: gh is not on PATH" >&2; exit 1; } +# before "docker-compose up": else Docker makes the folders as root +mkdir -p "$units" "$repo/.review/approve" "$repo/.review/approve-status" + +cat >"$units/t7d-review-approve.path" <"$units/t7d-review-approve.service" </dev/null)" != yes ]; then + echo "Note: linger is off. After a reboot the button works only when $USER has a login session." + echo "To keep it on: loginctl enable-linger $USER" +fi +echo "Remove it: systemctl --user disable --now t7d-review-approve.path && rm $units/t7d-review-approve.*" diff --git a/scripts/review/approve.py b/scripts/review/approve.py new file mode 100755 index 0000000..0041280 --- /dev/null +++ b/scripts/review/approve.py @@ -0,0 +1,160 @@ +#!/usr/bin/env python3 +"""Approve the newest round of a branch: the host publishes it to t7d.dev (scripts/review/approve-host.sh). + + python3 scripts/review/approve.py [] [--root
] + +- Writes .review/approve/request.json in the main clone, also when it runs from a worktree. The sha must be the head of + the branch (default: the head). +- Then prints each step that the host writes for this request to .review/approve-status/status.json. + A status belongs to this request if its branch, sha and request time are those of the request. +- Exit 0 when the host is done. Exit 1 when it fails, when a request is waiting already, while the host runs, + or after --timeout seconds. +The review server (devserver.py) uses write_request and read_status of this module for the Approve button. +""" +import argparse +import datetime +import json +import os +import re +import sys +import time + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +import gitread # noqa: E402 + +BASE = "master" +SHA = re.compile(r"^[0-9a-f]{40}$") +REQUEST = "request.json" +STATUS = "status.json" +STALE = 1800 # seconds: a "running" status older than this is a host run that stopped without a word + + +class Refused(Exception): + def __init__(self, code, message): + super().__init__(message) + self.code, self.message = code, message + + +def now(): + return datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") + + +def write_request(main, approve_dir, branch, sha, by, *, status_dir): + """Write the request for the newest round of branch and return it. Raises Refused (400, 409 or 503). + While the status in status_dir is "running" (and not stale), a new request is refused.""" + if not isinstance(branch, str) or not gitread.valid_branch(branch) or branch == BASE: + raise Refused(400, "not a branch that can ship: %r" % (branch,)) + if not isinstance(sha, str) or not SHA.match(sha): + raise Refused(400, "not a full sha: %r" % (sha,)) + rounds = gitread.rounds(main, branch, BASE) + if not rounds or gitread.branch_head(main, branch) != sha or rounds[-1]["sha"] != sha: + raise Refused(409, "%s is not the newest round of %s" % (sha[:7], branch)) + st = read_status(status_dir) + if st["state"] == "running": + raise Refused(409, "the host is publishing %s now" % (st.get("branch") or "a request")) + req = {"branch": branch, "sha": sha, "round": len(rounds), "by": by, "time": now()} + path = os.path.join(approve_dir, REQUEST) + tmp = os.path.join(approve_dir, ".request.%d.tmp" % os.getpid()) + try: + with open(tmp, "w") as f: + json.dump(req, f) + os.link(tmp, path) # atomic, and unlike os.replace it never overwrites a waiting request + except FileExistsError: + raise Refused(409, "a request is waiting already") + except FileNotFoundError: + raise Refused(503, "approve is not set up: %s is missing" % approve_dir) + finally: + try: + os.unlink(tmp) + except FileNotFoundError: + pass + return req + + +def is_for(st, req): + """True if the status st belongs to the request req: the host copies branch, sha and the request time.""" + return (st.get("branch"), st.get("sha"), st.get("request")) == (req["branch"], req["sha"], req["time"]) + + +def read_status(status_dir): + """The status of the host, or {"state": "idle"}. A stale "running" status reads as failed.""" + try: + with open(os.path.join(status_dir, STATUS)) as f: + st = json.load(f) + except (OSError, ValueError): + return {"state": "idle"} + if not isinstance(st, dict) or st.get("state") not in ("running", "done", "failed"): + return {"state": "idle"} + if st["state"] == "running": + try: + age = time.time() - datetime.datetime.strptime(st.get("time", ""), "%Y-%m-%dT%H:%M:%SZ").replace( + tzinfo=datetime.timezone.utc).timestamp() + except ValueError: + age = STALE + 1 + if age > STALE: + st = dict(st, state="failed", line="the host wrote nothing for %d minutes" % (STALE // 60)) + return st + + +def main_clone(path): + """The main checkout of the repo at path: the parent of the common .git folder. A worktree gives its main clone.""" + res = gitread._git(path, "rev-parse", "--path-format=absolute", "--git-common-dir") + if res.returncode == 0 and res.stdout.strip(): + return os.path.dirname(res.stdout.strip()) + return os.path.normpath(os.path.join(path, "..", "..")) + + +def main(): + here = os.path.dirname(os.path.abspath(__file__)) + ap = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + ap.add_argument("branch") + ap.add_argument("sha", nargs="?", help="the head of the branch (default)") + ap.add_argument("--root", default=main_clone(here), help="the main clone (default: the main clone of this script)") + ap.add_argument("--timeout", type=float, default=3600, help="seconds to wait for the host (default 3600)") + ap.add_argument("--poll", type=float, default=2, help=argparse.SUPPRESS) + a = ap.parse_args() + approve_dir = os.path.join(a.root, ".review", "approve") + status_dir = os.path.join(a.root, ".review", "approve-status") + sha = a.sha or gitread.branch_head(a.root, a.branch) + if not sha: + print("approve: no branch %s in %s" % (a.branch, a.root), file=sys.stderr) + return 1 + try: + req = write_request(a.root, approve_dir, a.branch, sha, "agent", status_dir=status_dir) + except Refused as e: + print("approve: " + e.message, file=sys.stderr) + return 1 + print("== approve %s round %d (%s)" % (req["branch"], req["round"], req["sha"][:7]), flush=True) + start = time.time() + end, warned = start + a.timeout, False + while os.path.exists(os.path.join(approve_dir, REQUEST)): # the host removes it when it starts + if time.time() > end: + print("approve: no host took the request. It stays in %s." % approve_dir, file=sys.stderr) + return 1 + if not warned and time.time() > start + 30: + print(" waiting for the host (systemctl --user status t7d-review-approve.path)", flush=True) + warned = True + time.sleep(a.poll) + last = None + while True: + st = read_status(status_dir) + if not is_for(st, req): # the status of an earlier request, or the first step before the host read ours + st = {"state": "waiting"} + seen = (st.get("state"), st.get("step")) + if seen != last and st["state"] not in ("idle", "waiting"): + print(" %s: %s" % (st["state"], st.get("step", "")), flush=True) + last = seen + if st.get("state") == "done": + print("== done: %s" % (st.get("pr") or ""), flush=True) + return 0 + if st.get("state") == "failed": + print("approve: failed at \"%s\": %s" % (st.get("step", ""), st.get("line", "")), file=sys.stderr) + return 1 + if time.time() > end: + print("approve: the host did not finish in %d s" % a.timeout, file=sys.stderr) + return 1 + time.sleep(a.poll) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/review/check_review.py b/scripts/review/check_review.py new file mode 100644 index 0000000..a382fb2 --- /dev/null +++ b/scripts/review/check_review.py @@ -0,0 +1,470 @@ +#!/usr/bin/env python3 +"""Browser check of the review bar: the version picker and the marks of a round. + + uv run -q --no-project --with playwright==1.63.0 python3 scripts/review/check_review.py [--shots DIR] + +Makes a temporary git repo (master and the worktree t/a with two rounds), starts devserver.py on a free port and +opens the pages in Chromium at 1440, 390 and 320 px, in the light and the dark theme. Exit code 1 if a check fails. +Round 1 changes the heading. Round 2 changes one word, removes one paragraph, adds one and adds the page /new/. +The worktree t/b has one round and a change that is not committed (Working). +The Approve button: a route catches its POST, so nothing is written. A fake status file stands in for the host. +The page clock of Playwright jumps over the 60 s wait for a host that does not take the request. +""" +import argparse +import datetime +import json +import os +import re +import shutil +import socket +import subprocess +import sys +import tempfile +import time +import urllib.request + +from playwright.sync_api import sync_playwright + +HERE = os.path.dirname(os.path.abspath(__file__)) +REPO = os.path.normpath(os.path.join(HERE, "..", "..")) +CSP = ("default-src 'none'; style-src 'self'; script-src 'self'; img-src 'self'; font-src 'self'; " + "base-uri 'none'; form-action 'none'") +P1_OLD = "The first paragraph has a quick word in it." +P1_NEW = "The first paragraph has a slow word in it." +P2 = "This second paragraph goes away in round two." +P3 = "The third paragraph stays the same." +P4 = "Round two adds this closing text." +SIZES = ((1440, 900), (390, 844), (320, 640)) +WIDE = 960 # from this width the bar is one row with every select; below it the selects open on a tap +MARKS = "ins.rv-add, del.rv-del, .rv-gone, .rv-newpage" +PR = "https://github.com/Tim-Conrad/Tim-Conrad.github.io/pull/7" +GONE = "Branch t/gone is not here any more. It was published or removed." +NO_HOST = "No host took the request. On the host, run: systemctl --user status t7d-review-approve.path" + + +def page(title, paras): + body = "\n".join("

%s

" % p for p in paras) + return ('' + '' + '%s' + '' + '' + '' + '
Fixture' + '
' + '

%s

\n%s\n
') % (CSP, title, title, body) + + +def git(cwd, *args): + subprocess.run(["git", "-c", "user.name=t", "-c", "user.email=t@t", *args], cwd=cwd, check=True, + capture_output=True) + + +def write(root, rel, text): + os.makedirs(os.path.dirname(os.path.join(root, rel)), exist_ok=True) + with open(os.path.join(root, rel), "w", encoding="utf-8") as f: + f.write(text) + + +def fixture(base): + """The main clone (master) and the worktree of t/a below base/wts. Returns (main, wts).""" + main, wts = os.path.join(base, "main"), os.path.join(base, "wts") + os.makedirs(os.path.join(main, "assets")) + for f in ("site.css", "theme.js", "site.js", "favicon.svg"): + shutil.copy(os.path.join(REPO, "assets", f), os.path.join(main, "assets", f)) + shutil.copytree(os.path.join(REPO, "assets", "fonts"), os.path.join(main, "assets", "fonts")) + write(main, "index.html", page("Fixture", [P1_OLD, P2, P3])) + write(main, "long/index.html", page("Long", ["Paragraph %d of a page that scrolls." % i for i in range(60)])) + git(main, "init", "-b", "master") + git(main, "add", "-A") + git(main, "commit", "-m", "base") + wt = os.path.join(wts, "t-a") + git(main, "worktree", "add", "-b", "t/a", wt) + write(wt, "index.html", page("Fixture page", [P1_OLD, P2, P3])) + git(wt, "commit", "-am", "A longer heading") + write(wt, "index.html", page("Fixture page", [P1_NEW, P3, P4])) + write(wt, "new/index.html", page("New", ["A page that round two adds."])) + git(wt, "add", "-A") + git(wt, "commit", "-m", "Change a word, remove and add a paragraph") + wt_b = os.path.join(wts, "t-b") + git(main, "worktree", "add", "-b", "t/b", wt_b) + write(wt_b, "index.html", page("Fixture B", [P1_OLD, P2, P3])) + git(wt_b, "commit", "-am", "B heading") + write(wt_b, "index.html", page("Fixture B", [P1_NEW, P2, P3])) # Working: not committed + return main, wts + + +def free_port(): + with socket.socket() as s: + s.bind(("127.0.0.1", 0)) + return s.getsockname()[1] + + +def start(main, wts, status_dir): + port = free_port() + # --approve names a folder that does not exist: if the route misses the POST, the server answers 503 and writes nothing + proc = subprocess.Popen([sys.executable, os.path.join(HERE, "devserver.py"), "--root", main, "--worktrees", wts, + "--approve", os.path.join(status_dir, "missing"), "--approve-status", status_dir, + "--port", str(port)], stdout=subprocess.DEVNULL) + base = "http://127.0.0.1:%d" % port + for _ in range(100): + try: + urllib.request.urlopen(base + "/__review/api/state", timeout=1) + return proc, base + except OSError: + time.sleep(0.1) + proc.kill() + raise SystemExit("devserver.py did not start") + + +class Check: + def __init__(self): + self.failed = [] + + def that(self, ok, what): + print(("ok " if ok else "FAIL ") + what) + if not ok: + self.failed.append(what) + + +def texts(pg, sel): + return pg.eval_on_selector_all(sel, "ns => ns.map(n => n.textContent)") + + +def no_side_scroll(pg): + return pg.evaluate("document.documentElement.scrollWidth <= document.documentElement.clientWidth") + + +def wait_for(pg, expr): + """Poll expr in the page. wait_for_function cannot run: the page CSP has no 'unsafe-eval'.""" + for _ in range(50): + if pg.evaluate(expr): + return + pg.wait_for_timeout(100) + raise AssertionError("timeout: " + expr) + + +def bar_ready(pg): + pg.wait_for_selector(".rv-bar[data-ready]", state="attached") + + +def open_bar(pg): + """On a narrow screen the selects are in a part that opens on a tap.""" + if pg.is_visible(".rv-more-toggle") and pg.get_attribute(".rv-more-toggle", "aria-expanded") != "true": + pg.click(".rv-more-toggle") + + +def pick(pg, sel, value): + open_bar(pg) + pg.select_option(sel, value) + + +def small_targets(pg): + return pg.evaluate("[...document.querySelectorAll('.rv-bar select, .rv-bar label, .rv-bar button')]" + ".filter(n => n.offsetParent).map(n => n.getBoundingClientRect().height).filter(x => x < 40)") + + +def theme_now(pg): + return pg.evaluate("document.documentElement.dataset.theme || " + "(matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light')") + + +def now(): + return datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") + + +def fake_status(status_dir, **fields): + """The status file as approve-host.sh writes it, or none for fields == {}.""" + path = os.path.join(status_dir, "status.json") + if not fields: + if os.path.exists(path): + os.remove(path) + return + st = {"state": "running", "step": "", "line": "", "pr": None, "branch": "t/a", "sha": "", "request": "", "time": now()} + st.update(fields) + with open(path, "w") as f: + json.dump(st, f) + + +def approve_shown(pg): + open_bar(pg) + return pg.is_visible(".rv-approve") + + +def run(base, shots, status_dir, head): + c = Check() + with sync_playwright() as p: + browser = p.chromium.launch() + for w, h in SIZES: + for scheme in ("light", "dark"): + tag = "%d %s" % (w, scheme) + ctx = browser.new_context(viewport={"width": w, "height": h}, color_scheme=scheme) + ctx.set_default_timeout(5000) + pg = ctx.new_page() + errors = [] + pg.on("console", lambda m, e=errors: m.type == "error" and e.append(m.text)) + pg.on("pageerror", lambda x, e=errors: e.append(str(x))) + + # master has no rounds: only the branch select + pg.goto(base + "/?branch=master") + bar_ready(pg) + open_bar(pg) + c.that(pg.is_visible(".rv-branch-pick") and not pg.is_visible(".rv-from") + and not pg.is_visible(".rv-to") and not pg.is_visible(".rv-switch"), + tag + ": on master the bar shows only the branch select") + c.that(pg.locator(".rv-approve").count() == 1 and pg.locator(".rv-approve").is_hidden(), + tag + ": the approve button is hidden") + c.that(pg.locator(".rv-tab, .rv-panel").count() == 0, tag + ": the note tab is gone") + + # a branch that is not here any more (after a publish): the page of the main clone and one line + pg.goto(base + "/?branch=t/gone") + bar_ready(pg) + c.that(pg.text_content(".rv-status") == GONE, tag + ": a gone branch has one line (%r)" % pg.text_content(".rv-status")) + c.that(pg.eval_on_selector_all(".rv-branch-pick option", "os => os.map(o => o.value)") == ["master", "t/a", "t/b"] + and not approve_shown(pg), tag + ": a gone branch keeps the branch select and has no approve button") + c.that(no_side_scroll(pg), tag + ": no horizontal scroll with the gone line") + if shots and w in (1440, 390): + pg.screenshot(path=os.path.join(shots, "gone-%d-%s.png" % (w, scheme))) + pg.goto(base + "/?branch=master") + bar_ready(pg) + + # the branch select opens t/a, with Round 2 against Round 1 + with pg.expect_navigation(): + pick(pg, ".rv-branch-pick", "t/a") + bar_ready(pg) + c.that("branch=t%2Fa" in pg.url or "branch=t/a" in pg.url, tag + ": the branch select loads t/a") + c.that(pg.input_value(".rv-to") == pg.eval_on_selector(".rv-to", "s => s.options[s.options.length - 1].value"), + tag + ": to is the newest round") + c.that(approve_shown(pg) and pg.text_content(".rv-approve") == "Approve round 2", + tag + ": the newest round shows \"Approve round 2\"") + if pg.get_attribute(".rv-more-toggle", "aria-expanded") == "true": + pg.click(".rv-more-toggle") + c.that(pg.eval_on_selector(".rv-from", "s => s.options[s.selectedIndex].text").startswith("Round 1"), + tag + ": from is Round 1") + c.that(texts(pg, "ins.rv-add") == ["slow", P4], tag + ": ins marks the new word and the new paragraph (%r)" % texts(pg, "ins.rv-add")) + c.that(texts(pg, "del.rv-del") == ["quick"], tag + ": del marks the old word (%r)" % texts(pg, "del.rv-del")) + c.that(texts(pg, ".rv-gone") == [P2], tag + ": rv-gone holds the removed paragraph") + c.that(pg.evaluate("document.querySelector('.rv-gone').nextElementSibling.textContent") == P3, + tag + ": rv-gone is before the next kept paragraph") + c.that(pg.evaluate("[...document.querySelectorAll('[class^=rv-], [class*=\" rv-\"]')]" + ".filter(n => n !== document.documentElement && !n.closest('[data-review-ui]') && !n.matches('ins, del')).length") == 0, + tag + ": every review element outside the marks carries data-review-ui") + c.that(no_side_scroll(pg), tag + ": no horizontal scroll with marks") + small = small_targets(pg) + c.that(small == [], tag + ": touch targets are 40 px or more (%r)" % small) + if shots: + pg.screenshot(path=os.path.join(shots, "marks-%d-%s.png" % (w, scheme)), full_page=True) + + # the switch off removes every mark and keeps the text of round 2; the choice stays after a reload + pg.click(".rv-switch") + c.that(pg.locator(MARKS).count() == 0, tag + ": switch off removes every mark") + c.that(texts(pg, "main p") == [P1_NEW, P3, P4], tag + ": switch off shows the text of round 2") + pg.reload() + bar_ready(pg) + c.that(not pg.is_checked(".rv-switch input") and pg.locator(MARKS).count() == 0, + tag + ": the switch stays off after a reload") + pg.click(".rv-switch") + wait_for(pg, "document.querySelectorAll('del.rv-del').length === 1") + c.that(texts(pg, "del.rv-del") == ["quick"] and texts(pg, ".rv-gone") == [P2], + tag + ": switch on shows the marks again") + + # one failed request: the same version loads when it is selected again + round1 = pg.eval_on_selector(".rv-to", "s => [...s.options].find(o => o.text.startsWith('Round 1')).value") + pg.route(re.compile(r"/__review/api/text\?.*rev=master"), lambda route: route.abort(), times=1) + n_errors = len(errors) + pick(pg, ".rv-from", "master") + wait_for(pg, "document.querySelector('.rv-status').textContent !== ''") + c.that(pg.locator(MARKS).count() == 0, tag + ": a failed request shows no marks") + errors[n_errors:] = [e for e in errors[n_errors:] if "net::ERR_FAILED" not in e] + pick(pg, ".rv-from", round1) + wait_for(pg, "document.querySelectorAll('del.rv-del').length === 1") + pick(pg, ".rv-from", "master") + try: + wait_for(pg, "document.querySelector('main h1 ins.rv-add') !== null") + again = True + except AssertionError: + again = False + c.that(again and pg.text_content(".rv-status") == "", + tag + ": after a failed request the version loads when it is selected again") + + # to = Round 1 is an older round: the page shows round 1, marked against master + pick(pg, ".rv-to", round1) + wait_for(pg, "document.querySelector('main h1 ins.rv-add') !== null") + c.that(texts(pg, "main > p") == [P1_OLD, P2, P3], tag + ": to = Round 1 shows the text of round 1") + c.that(not approve_shown(pg), tag + ": an older round hides the approve button") + c.that(texts(pg, "ins.rv-add") == [" page"] and pg.locator("del.rv-del, .rv-gone").count() == 0, + tag + ": Round 1 against master marks only the heading (%r)" % texts(pg, "ins.rv-add")) + header = pg.evaluate("document.querySelector('header.nav') !== null") + before = theme_now(pg) + pg.evaluate("document.getElementById('theme-toggle').click()") + after = theme_now(pg) + pg.evaluate("document.getElementById('theme-toggle').click()") + c.that(header and after != before and theme_now(pg) == before, + tag + ": in an older round the theme toggle of the header still works (%s -> %s)" % (before, after)) + if pg.get_attribute(".rv-more-toggle", "aria-expanded") == "true": + pg.click(".rv-more-toggle") + if shots and w != 1440: + pg.screenshot(path=os.path.join(shots, "round1-%d-%s.png" % (w, scheme))) + pick(pg, ".rv-to", pg.eval_on_selector(".rv-to", "s => s.options[s.options.length - 1].value")) + wait_for(pg, "document.querySelectorAll('main > p').length === 3") + wait_for(pg, "document.querySelectorAll('.rv-gone').length === 1") + c.that(texts(pg, "ins.rv-add") == [" page", "slow", P4] and texts(pg, "del.rv-del") == ["quick"], + tag + ": back to Round 2 shows the page on disk, marked against master (%r)" % texts(pg, "ins.rv-add")) + c.that(approve_shown(pg), tag + ": back to Round 2 shows the approve button again") + + # a page new in "to" + pick(pg, ".rv-from", pg.eval_on_selector(".rv-from", "s => [...s.options].find(o => o.text.startsWith('Round 1')).value")) + pg.goto(base + "/new/?branch=t/a") + bar_ready(pg) + c.that(texts(pg, ".rv-newpage") == ["New page in this round"], tag + ": a new page has one line") + c.that(pg.locator("ins.rv-add, del.rv-del, .rv-gone").count() == 0, tag + ": a new page has no other mark") + c.that(no_side_scroll(pg), tag + ": no horizontal scroll on the new page") + if shots and w == 390: + pg.screenshot(path=os.path.join(shots, "newpage-%d-%s.png" % (w, scheme))) + + # a long page: the closed bar is small, sticks to the bottom and opens on a tap + pg.goto(base + "/long/?branch=t/a") + bar_ready(pg) + box = pg.evaluate("(() => { const r = document.querySelector('.rv-bar').getBoundingClientRect();" + " return {top: r.top, bottom: r.bottom, h: r.height, vh: innerHeight}; })()") + c.that(abs(box["bottom"] - box["vh"]) < 2, tag + ": the bar sticks to the bottom of a long page (%r)" % box) + pad = pg.evaluate("parseFloat(getComputedStyle(document.documentElement).scrollPaddingBottom) || 0") + c.that(pad >= box["h"], tag + ": scroll-padding-bottom (%s px) is at least the bar height (%s px)" % (pad, box["h"])) + if w < WIDE: + c.that(box["h"] <= 0.15 * h, tag + ": the closed bar is at most 15%% of the screen (%s px)" % box["h"]) + c.that(pg.get_attribute(".rv-more-toggle", "aria-expanded") == "false" and not pg.is_visible(".rv-from"), + tag + ": the selects are closed at first") + c.that(pg.text_content(".rv-more-toggle").strip() == "Round 2 vs Round 1", + tag + ": the closed bar shows the comparison (%r)" % pg.text_content(".rv-more-toggle")) + c.that(small_targets(pg) == [], tag + ": closed bar: touch targets are 40 px or more") + if shots: + pg.screenshot(path=os.path.join(shots, "long-closed-%d-%s.png" % (w, scheme))) + pg.click(".rv-more-toggle") + c.that(pg.get_attribute(".rv-more-toggle", "aria-expanded") == "true" + and pg.is_visible(".rv-branch-pick") and pg.is_visible(".rv-from") and pg.is_visible(".rv-to"), + tag + ": a tap opens the selects") + c.that(small_targets(pg) == [] and no_side_scroll(pg), + tag + ": open bar: touch targets are 40 px or more, no horizontal scroll") + if shots: + pg.screenshot(path=os.path.join(shots, "long-open-%d-%s.png" % (w, scheme))) + pg.click(".rv-more-toggle") + c.that(pg.get_attribute(".rv-more-toggle", "aria-expanded") == "false" and not pg.is_visible(".rv-from"), + tag + ": a second tap closes the selects") + else: + c.that(not pg.is_visible(".rv-more-toggle") and pg.is_visible(".rv-from"), + tag + ": a wide screen shows every select in one row") + + # Working is not a commit: no approve. With changes in the worktree the host refuses, so the newest round + # of t/b has no button either. + pg.goto(base + "/?branch=t/b") + bar_ready(pg) + c.that(pg.input_value(".rv-to") == "working" and not approve_shown(pg), + tag + ": to = Working hides the approve button") + pick(pg, ".rv-to", pg.eval_on_selector(".rv-to", "s => [...s.options].find(o => o.text.startsWith('Round 1')).value")) + c.that(not approve_shown(pg), tag + ": a worktree with changes hides the button on its newest round too") + + # the approve flow: confirm, a POST with the header and the head sha, then the status of the host + fake_status(status_dir) + pg.goto(base + "/?branch=t/a") + bar_ready(pg) + c.that(pg.text_content(".rv-status") == "" and approve_shown(pg) and pg.is_enabled(".rv-approve"), + tag + ": with no status the line is empty and the button works") + posts, dialogs, asked = [], [], now() + + def on_approve(route): + if route.request.method != "POST": + return route.continue_() + posts.append({"header": route.request.headers.get("x-t7d-review"), "body": route.request.post_data_json}) + body = {"branch": "t/a", "sha": head, "round": 2, "by": "button", "time": asked} + route.fulfill(status=200, content_type="application/json", body=json.dumps(body)) + pg.route(re.compile(r"/__review/api/approve$"), on_approve) + pg.once("dialog", lambda d, dl=dialogs: (dl.append(d.message), d.accept())) + # the status of another request does not count for this page, even if it is newer than the request + pg.click(".rv-approve") + fake_status(status_dir, state="failed", step="push t/a", line="other", sha=head, request="2026-01-01T00:00:00Z", + time="2099-01-01T00:00:00Z") + c.that(dialogs == ["Publish t/a at round 2 to t7d.dev?"], tag + ": the confirm text (%r)" % dialogs) + wait_for(pg, "document.querySelector('.rv-status').textContent !== ''") + c.that(posts == [{"header": "1", "body": {"branch": "t/a", "sha": head}}], + tag + ": the POST has the header and the head sha (%r)" % posts) + pg.wait_for_timeout(2500) # one poll of the status, which belongs to another request + c.that(pg.text_content(".rv-status") == "Publishing t/a: wait for the host." and pg.is_disabled(".rv-approve"), + tag + ": the status of another request is not shown (%r)" % pg.text_content(".rv-status")) + fake_status(status_dir, step="run scripts/check.py", sha=head, request=asked) + wait_for(pg, "document.querySelector('.rv-status').textContent.indexOf('check.py') >= 0") + c.that(pg.text_content(".rv-status") == "Publishing t/a: run scripts/check.py." and pg.is_disabled(".rv-approve"), + tag + ": while the host runs, the line shows the step and the button is off (%r)" % pg.text_content(".rv-status")) + if shots and w in (1440, 390): + pg.screenshot(path=os.path.join(shots, "approve-running-%d-%s.png" % (w, scheme))) + fake_status(status_dir, state="done", step="published t/a", sha=head, pr=PR, request=asked) + wait_for(pg, "document.querySelector('.rv-status a') !== null") + c.that(pg.text_content(".rv-status") == "Published t/a. Open the PR" + and pg.get_attribute(".rv-status a", "href") == PR, + tag + ": done shows the PR link (%r)" % pg.text_content(".rv-status")) + c.that(not approve_shown(pg), tag + ": after done the published round has no approve button") + c.that(no_side_scroll(pg) and small_targets(pg) == [], + tag + ": with the status: no horizontal scroll, touch targets 40 px or more") + if shots and w in (1440, 390): + pg.screenshot(path=os.path.join(shots, "approve-done-%d-%s.png" % (w, scheme))) + pg.unroute(re.compile(r"/__review/api/approve$")) + fake_status(status_dir) + + # no host takes the request: after 60 s the page says so and the button works again + pg.clock.install() + pg.goto(base + "/?branch=t/a") + bar_ready(pg) + asked = now() + + def on_approve_no_host(route): + if route.request.method != "POST": + return route.continue_() + body = {"branch": "t/a", "sha": head, "round": 2, "by": "button", "time": asked} + route.fulfill(status=200, content_type="application/json", body=json.dumps(body)) + pg.route(re.compile(r"/__review/api/approve$"), on_approve_no_host) + pg.once("dialog", lambda d: d.accept()) + approve_shown(pg) + pg.click(".rv-approve") + wait_for(pg, "document.querySelector('.rv-status').textContent.indexOf('wait for the host') >= 0") + pg.clock.fast_forward(59000) + pg.wait_for_timeout(300) + c.that(pg.text_content(".rv-status") == "Publishing t/a: wait for the host." and pg.is_disabled(".rv-approve"), + tag + ": before 60 s the page still waits for the host (%r)" % pg.text_content(".rv-status")) + pg.clock.fast_forward(2000) + wait_for(pg, "document.querySelector('.rv-status').textContent.indexOf('No host') >= 0") + c.that(pg.text_content(".rv-status") == NO_HOST and pg.is_enabled(".rv-approve"), + tag + ": after 60 s with no status the page says that no host took it (%r)" % pg.text_content(".rv-status")) + c.that(no_side_scroll(pg), tag + ": no horizontal scroll with the no-host line") + if shots and w in (1440, 390): + pg.screenshot(path=os.path.join(shots, "approve-no-host-%d-%s.png" % (w, scheme))) + pg.unroute(re.compile(r"/__review/api/approve$")) + + c.that(errors == [], tag + ": no console error (%r)" % errors) + ctx.close() + browser.close() + return c + + +def main(): + ap = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + ap.add_argument("--shots", help="save screenshots to this folder") + a = ap.parse_args() + if a.shots: + os.makedirs(a.shots, exist_ok=True) + with tempfile.TemporaryDirectory() as tmp: + main_clone, wts = fixture(os.path.realpath(tmp)) + status_dir = os.path.join(os.path.realpath(tmp), "approve-status") + os.makedirs(status_dir) + head = subprocess.run(["git", "rev-parse", "t/a"], cwd=main_clone, check=True, capture_output=True, + text=True).stdout.strip() + proc, base = start(main_clone, wts, status_dir) + try: + c = run(base, a.shots, status_dir, head) + finally: + proc.terminate() + proc.wait() + print("%d checks failed" % len(c.failed) if c.failed else "all checks passed") + sys.exit(1 if c.failed else 0) + + +if __name__ == "__main__": + main() diff --git a/scripts/review/devserver.py b/scripts/review/devserver.py index 239ef3b..167e0e0 100644 --- a/scripts/review/devserver.py +++ b/scripts/review/devserver.py @@ -1,37 +1,51 @@ #!/usr/bin/env python3 -"""Review server for the site: live reload and notes. Only the review site uses it. GitHub Pages serves t7d.dev. +"""Review server for the site: live reload and the rounds of a branch. Only the review site uses it. GitHub Pages serves t7d.dev. - python3 scripts/review/devserver.py [--root .] [--notes .review/notes] [--host 127.0.0.1] [--port 8000] + python3 scripts/review/devserver.py [--root .] [--host 127.0.0.1] [--port 8000] - Serves the repository root with Cache-Control: no-cache. A missing page gets /404.html with status 404. -- Every HTML page gets /__review/review.css and /__review/review.js. The page CSP gets connect-src 'self', so the +- Every HTML page gets /__review/review.css, /__review/textdiff.js and /__review/review.js. The page CSP gets connect-src 'self', so the script can reach this server. Nothing else in the CSP changes, so the review site keeps the rules of the real site. -- /__review/reload: server-sent events. The stamp changes when a file of the site changes, is added or is deleted. - The page then loads again. -- /__review/api/state: the branch and the commit of the clone, and the number of open notes. -- /__review/api/notes: GET lists the notes, open and done. POST writes one note as /.json. The POST needs - the header X-T7d-Review and a JSON body: a plain form from another site cannot send either. A handled note goes to - /done/ with an "answer" field. The page shows the answer under "My notes". +- /__review/reload: server-sent events. The stamp changes when a file of the site changes, is added or is deleted, + and when the worktree gets a new commit. The page then loads again. +- The branch comes from ?branch=, else the cookie review_branch, else the branch of the main clone. A page with a + valid ?branch= sets the cookie. An unknown branch gives the main clone. Pages come from the worktree of that branch. +- /__review/api/state: the branch and the commit of the chosen worktree. +- /__review/api/versions?branch=: the branches, the rounds of the branch since master, and if the worktree has changes. + For a branch that is not here (after a publish), the main clone, with the name in "gone". An invalid name gives 400. +- /__review/api/text?branch=&rev=&path=: the HTML of a page at that revision, or null. +- POST /__review/api/approve with the header X-T7d-Review and JSON {branch, sha}: writes the approve request of the newest + round (approve.py) to --approve, unless the host runs (409). GET /__review/api/approve: the status that the host + writes to --approve-status. + This request is the only file that the server writes. +- Env PAPERCUTS_URL (the relay of the papercuts widget): each page gets its script tag, and the origin of the URL goes into + script-src, connect-src and img-src of the CSP. Without it, the page has no widget. - /llms.txt, /thoughts/feed.xml and /blog/feed.xml are made on each request by scripts/gen-feeds.py, as the deploy makes them for gh-pages. -- /.git, /.review, /.reels and /node_modules are not served. +- /.git, /.review, /.papercuts, /.reels and /node_modules are not served. """ import argparse import importlib.util import json import os import re -import secrets +import sys import time +import urllib.parse +from html import escape as html_escape from functools import partial +from http.cookies import SimpleCookie from http.server import SimpleHTTPRequestHandler, ThreadingHTTPServer HERE = os.path.dirname(os.path.abspath(__file__)) +sys.path.insert(0, HERE) +import approve # noqa: E402 +import gitread # noqa: E402 +from gitread import HIDDEN # noqa: E402 PREFIX = "/__review/" -ASSETS = {"review.js": "text/javascript; charset=utf-8", "review.css": "text/css; charset=utf-8"} -HIDDEN = (".git", ".review", ".reels", "node_modules", "__pycache__") -MAX_BODY = 32 * 1024 -MAX_NOTE = 4000 -NOTE_ID = re.compile(r"^\d{8}-\d{6}-[0-9a-f]{6}$") +COOKIE = "review_branch" +BASE = "master" +ASSETS = {"review.js": "text/javascript; charset=utf-8", "textdiff.js": "text/javascript; charset=utf-8", + "review.css": "text/css; charset=utf-8"} GENERATED = {"/llms.txt": "text/plain; charset=utf-8", "/thoughts/feed.xml": "application/rss+xml; charset=utf-8", "/blog/feed.xml": "application/rss+xml; charset=utf-8"} @@ -53,7 +67,8 @@ def read(path): def stamp(root): - """The newest change below root, as a string: the mtime of each file and the number of files.""" + """The newest change below root, as a string: the mtime of each file, the number of files and the commit. + A commit changes only the .git folder of the main clone, so the commit of root is part of the stamp.""" newest, count = 0.0, 0 for d, dirs, files in os.walk(root): dirs[:] = [x for x in dirs if x not in HIDDEN] @@ -63,80 +78,55 @@ def stamp(root): count += 1 except OSError: pass # removed during the walk - return "%.3f-%d" % (newest, count) - - -def git_state(root): - """{branch, commit} of the clone from .git, without the git binary (the container has none).""" - g = os.path.join(root, ".git") - try: - head = read(os.path.join(g, "HEAD")).strip() - except OSError: - return {"branch": None, "commit": None} - if not head.startswith("ref: "): - return {"branch": None, "commit": head[:7]} - ref = head[5:] - commit = None - try: - commit = read(os.path.join(g, ref)).strip() - except OSError: - try: - for line in read(os.path.join(g, "packed-refs")).splitlines(): - if line.rstrip().endswith(" " + ref): - commit = line.split()[0] - except OSError: - pass - return {"branch": ref.rsplit("refs/heads/", 1)[-1], "commit": commit[:7] if commit else None} + return "%.3f-%d-%s" % (newest, count, gitread.head(root)["commit"]) + + +def git_state(path): + """{branch, commit} of the checkout at path.""" + return gitread.head(path) + + +def add_source(policy, directive, origin): + """The policy with origin in the directive: appended if the directive exists, else a new one that keeps 'self'.""" + m = re.search(r"(^|;\s*)%s\s+([^;]*)" % directive, policy) + if m: + return policy[:m.end()] + " " + origin + policy[m.end():] + return policy.rstrip("; ") + "; %s 'self' %s" % (directive, origin) -def inject(html): - """The page with the review assets, and connect-src 'self' in its CSP.""" +def inject(html, widget=None, branch="", commit=""): + """The page with the review assets, and connect-src 'self' in its CSP. + With widget (the URL of the papercuts relay), the page also gets the widget tag and the relay in the CSP.""" + origin = None + if widget: + u = urllib.parse.urlsplit(widget) + origin = "%s://%s" % (u.scheme, u.netloc) + def csp(m): policy = m.group(2) if "connect-src" not in policy: policy = policy.rstrip("; ") + "; connect-src 'self'" + if origin: + for d in ("script-src", "connect-src", "img-src"): + policy = add_source(policy, d, origin) return m.group(1) + policy + m.group(3) html = CSP.sub(csp, html, count=1) link = '' % PREFIX - script = '' % PREFIX + # textdiff.js first: deferred scripts run in order, and review.js uses window.reviewDiff. + script = '\n' % (PREFIX, PREFIX) + if widget: + script += '\n' % ( + html_escape(widget.rstrip("/")), html_escape(branch), html_escape(commit)) html = html.replace("", link + "\n" + script + "\n", 1) if "" in html else link + script + html return html -def clean_note(d): - """The stored note from a POST body, or a ValueError. Only known fields, with limits.""" - if not isinstance(d, dict): - raise ValueError("the body is not a JSON object") - note = d.get("note") - if not isinstance(note, str) or not note.strip() or len(note) > MAX_NOTE: - raise ValueError("note: 1 to %d characters" % MAX_NOTE) - - def text(k, n): - v = d.get(k) - return v[:n] if isinstance(v, str) else None - - def num(v): - return round(float(v), 1) if isinstance(v, (int, float)) and not isinstance(v, bool) else None - - out = {"note": note.strip(), "path": text("path", 500), "title": text("title", 200), "theme": text("theme", 10), - "selection": text("selection", 1000), "agent": text("agent", 300)} - for k in ("viewport", "scroll"): - v = d.get(k) - out[k] = {a: num(v.get(a)) for a in ("w", "h", "x", "y", "dpr") if a in v} if isinstance(v, dict) else None - t = d.get("target") - if isinstance(t, dict): - r = t.get("rect") if isinstance(t.get("rect"), dict) else {} - out["target"] = {"selector": t["selector"][:500] if isinstance(t.get("selector"), str) else None, - "text": t["text"][:300] if isinstance(t.get("text"), str) else None, - "rect": {a: num(r.get(a)) for a in ("x", "y", "w", "h")}} - else: - out["target"] = None - return out - - class Handler(SimpleHTTPRequestHandler): root = "." - notes = ".review/notes" + widget = None # the URL of the papercuts relay, or None for no widget + worktrees = None # the folder that holds the site worktrees, or None for any + approve_dir = None # the folder of the approve request, default /.review/approve + status_dir = None # the folder of the approve status, default /.review/approve-status def end_headers(self): self.send_header("Cache-Control", "no-cache") @@ -146,9 +136,58 @@ def log_message(self, fmt, *args): pass # quiet: the reload stream would fill the log def hidden(self): - parts = self.path.split("?", 1)[0].split("/") + parts = urllib.parse.unquote(self.path.split("?", 1)[0]).split("/") return any(p in HIDDEN for p in parts) + def servable(self, path): + """True if path is a file or folder inside the chosen worktree, with no hidden folder in its real path. + A symlink in the worktree cannot lead out of it or into .git.""" + base = os.path.realpath(self.wt) + real = os.path.realpath(path) + if not real.startswith(base + os.sep): + return real == base + return not any(p in HIDDEN for p in os.path.relpath(real, base).split(os.sep)) + + def allowed(self, path): + """A worktree is offered if it is a folder, and below self.worktrees when that is set.""" + if not os.path.isdir(path): + return False + if not self.worktrees: + return True + return os.path.realpath(path).startswith(os.path.realpath(self.worktrees) + os.sep) + + def pick(self): + """Choose the branch of this request. Sets self.branch, self.wt (its worktree path), self.names, + self.cookie_branch (a valid ?branch= to remember), self.bad_branch (True for an invalid ?branch=) and + self.gone_branch (a valid ?branch= that is not here, for example after a publish).""" + main = gitread.head(self.root)["branch"] + found = {w["branch"]: w["path"] for w in gitread.worktrees(self.root) if self.allowed(w["path"])} + if main: + found[main] = self.root + self.names = ([main] if main else []) + [b for b in found if b != main] + queried = urllib.parse.parse_qs(urllib.parse.urlsplit(self.path).query).get("branch") + self.cookie_branch, self.bad_branch, self.gone_branch = None, False, None + want = None + if queried: + if not gitread.valid_branch(queried[0]): + self.bad_branch = True + elif queried[0] in found: + want = self.cookie_branch = queried[0] + else: + self.gone_branch = queried[0] + else: + try: + value = SimpleCookie(self.headers.get("Cookie") or "")[COOKIE].value + except (KeyError, ValueError): + value = None + if value and gitread.valid_branch(value) and value in found: + want = value + self.branch = want or main + self.wt = found.get(want) or self.root + + def query(self, name): + return urllib.parse.parse_qs(urllib.parse.urlsplit(self.path).query).get(name, [None])[0] + def send_json(self, code, obj): body = json.dumps(obj).encode() self.send_response(code) @@ -158,15 +197,20 @@ def send_json(self, code, obj): self.wfile.write(body) def send_page(self, path, code=200): - body = inject(read(path)).encode() + state = git_state(self.wt) + body = inject(read(path), self.widget, state.get("branch") or "", state.get("commit") or "").encode() self.send_response(code) self.send_header("Content-Type", "text/html; charset=utf-8") self.send_header("Content-Length", str(len(body))) + if self.cookie_branch: + self.send_header("Set-Cookie", "%s=%s; Path=/; SameSite=Strict" % (COOKIE, self.cookie_branch)) self.end_headers() self.wfile.write(body) def do_GET(self): url = self.path.split("?", 1)[0] + self.pick() + self.directory = self.wt # SimpleHTTPRequestHandler serves from this folder if url.startswith(PREFIX): return self.review_get(url[len(PREFIX):]) if self.hidden(): @@ -174,6 +218,8 @@ def do_GET(self): if url in GENERATED: return self.send_generated(url) path = self.translate_path(self.path) + if not self.servable(path): + return self.not_found() if os.path.isdir(path): if not url.endswith("/"): self.send_response(301) @@ -181,6 +227,8 @@ def do_GET(self): self.end_headers() return path = os.path.join(path, "index.html") + if not self.servable(path): + return self.not_found() if path.endswith(".html"): return self.send_page(path) if os.path.isfile(path) else self.not_found() if not os.path.isfile(path): @@ -188,7 +236,7 @@ def do_GET(self): return super().do_GET() def send_generated(self, url): - body = gen_feeds().build(self.root)[url.lstrip("/")].encode() + body = gen_feeds().build(self.wt)[url.lstrip("/")].encode() self.send_response(200) self.send_header("Content-Type", GENERATED[url]) self.send_header("Content-Length", str(len(body))) @@ -196,7 +244,7 @@ def send_generated(self, url): self.wfile.write(body) def not_found(self): - page = os.path.join(self.root, "404.html") + page = os.path.join(self.wt, "404.html") if os.path.isfile(page): return self.send_page(page, 404) self.send_error(404) @@ -213,11 +261,44 @@ def review_get(self, name): if name == "reload": return self.reload() if name == "api/state": - return self.send_json(200, {**git_state(self.root), "open": len(self.list_notes("open"))}) - if name == "api/notes": - return self.send_json(200, {"notes": self.list_notes("open") + self.list_notes("done")}) + return self.send_json(200, git_state(self.wt)) + if name == "api/versions": + return self.versions() + if name == "api/text": + return self.text() + if name == "api/approve": + return self.send_json(200, approve.read_status(self.status_path())) self.send_error(404) + def versions(self): + if self.bad_branch: + return self.send_json(400, {"error": "unknown branch"}) + return self.send_json(200, {"branch": self.branch, "branches": self.names, "base": BASE, + "rounds": gitread.rounds(self.root, self.branch, BASE) if self.branch else [], + "working": gitread.dirty(self.wt), "gone": self.gone_branch}) + + def text(self): + if self.bad_branch: + return self.send_json(400, {"error": "unknown branch"}) + rev, path = self.query("rev"), self.query("path") + shas = [r["sha"] for r in gitread.rounds(self.root, self.branch, BASE)] if self.branch else [] + if rev not in ("working", BASE, *shas): + return self.send_json(400, {"error": "unknown rev"}) + rel = (path or "").lstrip("/") if (path or "").startswith("/") else None + if rel is not None and (rel == "" or rel.endswith("/")): + rel += "index.html" + if rel is None or not gitread.valid_path(rel): + return self.send_json(400, {"error": "bad path"}) + try: + if rev == "working": + full = os.path.join(self.wt, rel) + html = read(full) if self.servable(full) and os.path.isfile(full) else None + else: + html = gitread.show(self.root, rev, rel) + except UnicodeDecodeError: + html = None # a binary file + return self.send_json(200, {"html": html}) + def reload(self): self.send_response(200) self.send_header("Content-Type", "text/event-stream") @@ -225,7 +306,7 @@ def reload(self): last, beat = None, 0.0 try: while True: - now = stamp(self.root) + now = stamp(self.wt) if now != last or time.time() - beat > 15: self.wfile.write(("data: %s\n\n" % now).encode()) self.wfile.flush() @@ -234,56 +315,49 @@ def reload(self): except (BrokenPipeError, ConnectionResetError): pass # the page closed - def list_notes(self, status): - d = self.notes if status == "open" else os.path.join(self.notes, "done") - out = [] - try: - names = sorted(os.listdir(d), reverse=True) - except OSError: - return out - for f in names: - if not (f.endswith(".json") and NOTE_ID.match(f[:-5])): - continue - try: - n = json.loads(read(os.path.join(d, f))) - except (OSError, ValueError): - continue - out.append({"id": f[:-5], "status": status, "time": n.get("time"), "path": n.get("path"), - "note": n.get("note", "")[:200], "answer": n.get("answer") if status == "done" else None}) - return out + def status_path(self): + return self.status_dir or os.path.join(self.root, ".review", "approve-status") def do_POST(self): - if self.path.split("?", 1)[0] != PREFIX + "api/notes": + if self.path.split("?", 1)[0] != PREFIX + "api/approve": return self.send_error(404) - if not self.headers.get("X-T7d-Review") or "application/json" not in (self.headers.get("Content-Type") or ""): - return self.send_json(403, {"error": "the review page sends notes"}) - n = int(self.headers.get("Content-Length") or 0) - if n <= 0 or n > MAX_BODY: - return self.send_json(413, {"error": "the note is too large"}) + # A page of another site cannot send this header without a CORS preflight, which this server never allows. + if not self.headers.get("X-T7d-Review"): + return self.send_json(403, {"error": "the header X-T7d-Review is missing"}) + try: + size = int(self.headers.get("Content-Length") or 0) + if not 0 < size <= 4096: + raise ValueError + body = json.loads(self.rfile.read(size)) + branch, sha = body["branch"], body["sha"] + except (ValueError, KeyError, TypeError): + return self.send_json(400, {"error": "the body must be JSON with branch and sha"}) + offered = {w["branch"] for w in gitread.worktrees(self.root) if self.allowed(w["path"])} + if not isinstance(branch, str) or branch not in offered: + return self.send_json(400, {"error": "unknown branch"}) try: - note = clean_note(json.loads(self.rfile.read(n))) - except ValueError as e: - return self.send_json(400, {"error": str(e)}) - note.update(git_state(self.root)) - note["time"] = time.strftime("%Y-%m-%dT%H:%M:%S%z") - nid = time.strftime("%Y%m%d-%H%M%S-") + secrets.token_hex(3) - os.makedirs(self.notes, exist_ok=True) - tmp = os.path.join(self.notes, "." + nid + ".tmp") - with open(tmp, "w", encoding="utf-8") as f: - json.dump({"id": nid, **note}, f, ensure_ascii=False, indent=1) - os.replace(tmp, os.path.join(self.notes, nid + ".json")) - self.send_json(201, {"id": nid}) + req = approve.write_request(self.root, self.approve_dir or os.path.join(self.root, ".review", "approve"), + branch, sha, "button", status_dir=self.status_path()) + except approve.Refused as e: + return self.send_json(e.code, {"error": e.message}) + return self.send_json(200, req) def main(): ap = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) ap.add_argument("--root", default=os.path.normpath(os.path.join(HERE, "..", ".."))) - ap.add_argument("--notes", default=None, help="the notes folder (default: /.review/notes)") + ap.add_argument("--worktrees", default=os.environ.get("REVIEW_WORKTREES"), + help="offer only the worktrees below this folder (default: env REVIEW_WORKTREES, else any)") + ap.add_argument("--approve", help="the folder of the approve request (default: /.review/approve)") + ap.add_argument("--approve-status", help="the folder of the approve status (default: /.review/approve-status)") ap.add_argument("--host", default="127.0.0.1") ap.add_argument("--port", type=int, default=8000) a = ap.parse_args() root = os.path.abspath(a.root) - handler = partial(type("H", (Handler,), {"root": root, "notes": os.path.abspath(a.notes or os.path.join(root, ".review", "notes"))}), + handler = partial(type("H", (Handler,), {"root": root, "worktrees": a.worktrees and os.path.abspath(a.worktrees), + "widget": os.environ.get("PAPERCUTS_URL") or None, + "approve_dir": a.approve and os.path.abspath(a.approve), + "status_dir": a.approve_status and os.path.abspath(a.approve_status)}), directory=root) srv = ThreadingHTTPServer((a.host, a.port), handler) srv.daemon_threads = True diff --git a/scripts/review/gitread.py b/scripts/review/gitread.py new file mode 100644 index 0000000..648fd8f --- /dev/null +++ b/scripts/review/gitread.py @@ -0,0 +1,96 @@ +"""Read worktrees, rounds and old files from git. Never uses a shell.""" +import re +import subprocess + +HIDDEN = (".git", ".review", ".papercuts", ".reels", "node_modules", "__pycache__") + +_BRANCH_RE = re.compile(r"^[A-Za-z0-9._/-]{1,100}$") + + +def _git(cwd, *args): + # safe.directory: the container user differs from the mount owner in some setups. + try: + return subprocess.run( + ["git", "-c", "safe.directory=*", *args], + cwd=cwd, capture_output=True, text=True, timeout=5, + ) + except (subprocess.TimeoutExpired, FileNotFoundError, ValueError): + return subprocess.CompletedProcess(args, 1, "", "") + + +def worktrees(main): + out = _git(main, "worktree", "list", "--porcelain").stdout + result = [] + for block in out.split("\n\n"): + path = branch = None + for line in block.splitlines(): + if line.startswith("worktree "): + path = line[len("worktree "):] + elif line.startswith("branch refs/heads/"): + branch = line[len("branch refs/heads/"):] + if path and branch: + result.append({"branch": branch, "path": path}) + return result + + +def rounds(main, branch, base="master"): + if base.startswith("-") or branch.startswith("-"): + return [] + res = _git(main, "log", "--reverse", "--format=%H%x00%s", "--end-of-options", base + ".." + branch, "--") + if res.returncode != 0: + return [] + result = [] + for line in res.stdout.splitlines(): + sha, _, subject = line.partition("\x00") + result.append({"n": len(result) + 1, "sha": sha, "subject": subject}) + return result + + +def dirty(path): + res = _git(path, "status", "--porcelain") + if res.returncode != 0: + return True # fail closed: an unknown state counts as dirty + for line in res.stdout.splitlines(): + name = line[3:].split(" -> ")[-1].strip('"') + if line.startswith("??") and name.split("/")[0] in HIDDEN: + continue + return True + return False + + +def head(path): + """{branch, commit} of the checkout at path. A value is None if git cannot tell.""" + branch = _git(path, "symbolic-ref", "--short", "-q", "HEAD") + commit = _git(path, "rev-parse", "--short=7", "HEAD") + return {"branch": branch.stdout.strip() or None if branch.returncode == 0 else None, + "commit": commit.stdout.strip() or None if commit.returncode == 0 else None} + + +def branch_head(main, branch): + """The full sha of the newest commit of branch, or None.""" + if branch.startswith("-"): + return None + res = _git(main, "rev-parse", "--verify", "-q", "--end-of-options", "refs/heads/" + branch + "^{commit}") + return res.stdout.strip() or None if res.returncode == 0 else None + + +def show(main, rev, path): + if rev.startswith("-"): + return None + res = _git(main, "show", "--end-of-options", rev + ":" + path) + return res.stdout if res.returncode == 0 else None + + +def valid_branch(name): + if not _BRANCH_RE.match(name) or ".." in name or name.startswith("-"): + return False + return _git(None, "check-ref-format", "--branch", name).returncode == 0 + + +def valid_path(path): + if not path or path.startswith("/"): + return False + if any(ord(c) < 0x20 or ord(c) == 0x7F for c in path): + return False + parts = path.split("/") + return not any(p == ".." or p in HIDDEN for p in parts) diff --git a/scripts/review/review.css b/scripts/review/review.css index e04788a..54b410a 100644 --- a/scripts/review/review.css +++ b/scripts/review/review.css @@ -1,49 +1,64 @@ -/* The note tab of the review site (scripts/review/devserver.py). Black and white, as the site (DESIGN.md). */ -.rv-tab { - position: fixed; right: 0; top: 50%; z-index: 2147483000; - transform: translateY(-50%) rotate(180deg); writing-mode: vertical-rl; - padding: 0.9rem 0.45rem; border: var(--rule, 1.5px) solid var(--ink, #111); border-left: 0; - background: var(--bg, #fff); color: var(--ink, #111); - font: 600 0.8rem/1 var(--mono, monospace); letter-spacing: 0.08em; text-transform: uppercase; cursor: pointer; +/* The review bar and the marks of a round (scripts/review/devserver.py). Black and white, as the site (DESIGN.md). */ +.rv-bar { + position: sticky; bottom: 0; z-index: 2147483000; + display: flex; flex-wrap: wrap; align-items: center; gap: 0.5rem 0.9rem; + padding: 0.5rem var(--pad, 1rem); border-top: var(--rule, 1.5px) solid var(--ink, #111); + background: var(--bg, #fff); color: var(--ink, #111); font: 0.9rem/1.3 var(--body, sans-serif); } -.rv-tab:focus-visible, .rv-panel button:focus-visible, .rv-text:focus-visible { outline: 2px solid var(--ink, #111); outline-offset: 2px; } -.rv-badge:not(:empty) { - display: inline-block; margin-top: 0.4rem; padding: 0.2rem 0.15rem; - background: var(--ink, #111); color: var(--bg, #fff); writing-mode: horizontal-tb; transform: rotate(180deg); +.rv-bar [hidden] { display: none !important; } +.rv-field { display: flex; flex: 1 1 11rem; align-items: center; gap: 0.45rem; min-width: 0; min-height: 40px; } +.rv-label { flex: none; min-width: 3.4rem; font: 500 0.72rem/1 var(--mono, monospace); letter-spacing: 0.08em; text-transform: uppercase; color: var(--muted, #5c5c5c); } +.rv-field select { + flex: 1 1 auto; width: 100%; min-width: 0; min-height: 40px; padding: 0 0.5rem; + border: var(--rule, 1.5px) solid var(--ink, #111); border-radius: 0; + background: var(--bg, #fff); color: var(--ink, #111); font: inherit; text-overflow: ellipsis; } -.rv-panel { - position: fixed; right: 2.4rem; bottom: 1rem; z-index: 2147483001; - width: min(24rem, calc(100vw - 3.4rem)); max-height: calc(100vh - 2rem); overflow: auto; - padding: 1rem; border: var(--rule, 1.5px) solid var(--ink, #111); - background: var(--bg, #fff); color: var(--ink, #111); font: 0.9rem/1.4 var(--body, sans-serif); - box-shadow: 0.35rem 0.35rem 0 var(--ink, #111); +.rv-switch { display: inline-flex; align-items: center; gap: 0.5rem; min-height: 40px; cursor: pointer; white-space: nowrap; } +.rv-switch input { width: 1.2rem; height: 1.2rem; margin: 0; accent-color: var(--ink, #111); cursor: pointer; } +.rv-approve { + min-height: 40px; padding: 0 1rem; border: var(--rule, 1.5px) solid var(--ink, #111); + background: var(--ink, #111); color: var(--bg, #fff); font: 600 0.75rem var(--mono, monospace); text-transform: uppercase; cursor: pointer; } -.rv-panel[hidden], .rv-box[hidden], .rv-panel [hidden] { display: none; } -.rv-head { display: flex; align-items: baseline; gap: 0.6rem; margin-bottom: 0.6rem; } -.rv-head strong { font: 600 1.05rem/1.1 var(--display, sans-serif); text-transform: uppercase; } -.rv-branch { flex: 1; min-width: 0; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; font: 0.72rem var(--mono, monospace); color: var(--muted, #5c5c5c); } -.rv-text { - display: block; box-sizing: border-box; width: 100%; padding: 0.5rem; resize: vertical; - border: var(--rule, 1.5px) solid var(--ink, #111); background: var(--bg, #fff); color: var(--ink, #111); font: inherit; +.rv-approve:disabled { opacity: 0.45; cursor: default; } +/* Find-in-page and focus stop above the sticky bar. The values are the height of the bar, closed and open. */ +html.rv-has-bar { scroll-padding-bottom: 4.5rem; } +html.rv-has-bar.rv-bar-open { scroll-padding-bottom: 20rem; } + +/* narrow screens: one row with the comparison and the switch, the selects open above it on a tap */ +.rv-more-toggle { + flex: 1 1 0; min-width: 0; min-height: 40px; padding: 0 0.6rem; overflow: hidden; + border: var(--rule, 1.5px) solid var(--ink, #111); background: var(--bg, #fff); color: var(--ink, #111); + font: 600 0.85rem/1 var(--body, sans-serif); text-align: left; white-space: nowrap; text-overflow: ellipsis; cursor: pointer; } -.rv-row { display: flex; flex-wrap: wrap; align-items: center; gap: 0.5rem; margin-top: 0.6rem; } -.rv-btn, .rv-x { - min-height: 2.2rem; padding: 0.3rem 0.7rem; border: var(--rule, 1.5px) solid var(--ink, #111); - background: var(--bg, #fff); color: var(--ink, #111); font: 600 0.75rem var(--mono, monospace); text-transform: uppercase; cursor: pointer; +.rv-more-toggle::after { content: " \25B4"; } +.rv-open .rv-more-toggle::after { content: " \25BE"; } +.rv-more { order: -1; flex: 1 0 100%; display: flex; flex-direction: column; gap: 0.5rem; min-width: 0; } +.rv-more .rv-field { flex: none; } +.rv-bar:not(.rv-open) .rv-more { display: none; } +@media (max-width: 959px) { + .rv-bar { gap: 0.5rem 0.6rem; padding: 0.5rem; } + .rv-switch { gap: 0.4rem; font-size: 0.85rem; } } -.rv-send { background: var(--ink, #111); color: var(--bg, #fff); } -.rv-btn:disabled { opacity: 0.5; cursor: wait; } -.rv-link { border: 0; background: none; color: var(--ink, #111); text-decoration: underline; font: inherit; cursor: pointer; } -.rv-part, .rv-status { min-width: 0; overflow-wrap: anywhere; color: var(--muted, #5c5c5c); font-size: 0.82rem; } -.rv-list-head { margin-top: 1rem; padding-top: 0.6rem; border-top: 1px solid var(--soft, #d9d9d9); font: 600 0.72rem var(--mono, monospace); text-transform: uppercase; } -.rv-list { margin: 0.4rem 0 0; padding: 0; list-style: none; } -.rv-item, .rv-empty { padding: 0.45rem 0; border-bottom: 1px solid var(--soft, #d9d9d9); font-size: 0.82rem; } -.rv-item span { display: block; overflow-wrap: anywhere; } -.rv-meta { font: 0.7rem var(--mono, monospace); color: var(--muted, #5c5c5c); } -.rv-answer { margin-top: 0.25rem; padding-left: 0.6rem; border-left: 3px solid var(--ink, #111); } -.rv-done .rv-note { color: var(--muted, #5c5c5c); } -.rv-box { - position: fixed; z-index: 2147482999; pointer-events: none; box-sizing: border-box; - border: 3px solid #e0201b; background: rgba(224, 32, 27, 0.08); +/* wide screens: one row with every select, no toggle */ +@media (min-width: 960px) { + .rv-more-toggle { display: none; } + .rv-more, .rv-bar:not(.rv-open) .rv-more { display: contents; } + .rv-more .rv-field { flex: 1 1 11rem; } + .rv-approve { order: 1; } + .rv-status { order: 2; } +} +.rv-status:empty { display: none; } +.rv-status { min-width: 0; overflow-wrap: anywhere; color: var(--muted, #5c5c5c); font-size: 0.82rem; } +.rv-bar select:focus-visible, .rv-bar input:focus-visible, .rv-bar button:focus-visible { outline: 2px solid var(--ink, #111); outline-offset: 2px; } + +/* the marks: they change no width, so long words wrap */ +ins.rv-add { color: inherit; background: none; text-decoration: underline 2px; text-underline-offset: 0.18em; } +del.rv-del, .rv-gone { color: inherit; text-decoration: line-through; opacity: 0.55; } +del.rv-del + ins.rv-add { margin-left: 0.25em; } +/* no margin on .rv-gone: the page spaces its blocks (.prose is a grid with a gap) */ +.rv-gone, .rv-newpage, .rv-missing { max-width: 100%; overflow-wrap: anywhere; } +.rv-newpage, .rv-missing { margin: 0.75rem 0; } +.rv-newpage { + padding: 0.4rem 0; border-bottom: var(--rule, 1.5px) solid var(--ink, #111); + font: 500 0.75rem/1.3 var(--mono, monospace); letter-spacing: 0.08em; text-transform: uppercase; } -.rv-picking, .rv-picking * { cursor: crosshair !important; } diff --git a/scripts/review/review.js b/scripts/review/review.js index 426b4d9..e7717fa 100644 --- a/scripts/review/review.js +++ b/scripts/review/review.js @@ -1,133 +1,374 @@ -/* The review site (scripts/review/devserver.py): live reload and the note tab. Never on t7d.dev. */ +/* The review site (scripts/review/devserver.py): live reload, the branch picker and the changes of a round. + Never on t7d.dev. Needs window.reviewDiff from /__review/textdiff.js. */ (function () { 'use strict'; var API = '/__review/api/'; + /* if no status of the request arrives in this time, no host took it (ms) */ + var HOST_WAIT = 60000; + var diff = window.reviewDiff; + /* the branch of this page: ?branch= of the address, else the server uses the cookie */ + var asked = new URLSearchParams(location.search).get('branch'); + function q(branch) { return branch ? 'branch=' + encodeURIComponent(branch) : ''; } /* live reload: the first stamp is the page's own; a new stamp loads the page again */ - var first = null, es = new EventSource('/__review/reload'); + var first = null, es = new EventSource('/__review/reload?' + q(asked)); es.onmessage = function (e) { if (first === null) first = e.data; else if (e.data !== first) location.reload(); }; - function el(tag, cls, text) { + /* an element of the bar: blocks() of textdiff.js skips everything with data-review-ui */ + function ui(tag, cls, text) { var n = document.createElement(tag); + n.setAttribute('data-review-ui', ''); if (cls) n.className = cls; if (text) n.textContent = text; return n; } - function theme() { - var t = document.documentElement.dataset.theme; - if (t === 'light' || t === 'dark') return t; - return matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'; - } - /* a CSS selector for a part of the page: its id, else the tag path with :nth-of-type */ - function selectorOf(node) { - var parts = []; - for (var n = node; n && n.nodeType === 1 && n !== document.body && parts.length < 8; n = n.parentElement) { - if (n.id) { parts.unshift('#' + CSS.escape(n.id)); return parts.join(' > '); } - var tag = n.tagName.toLowerCase(), i = 1; - for (var s = n.previousElementSibling; s; s = s.previousElementSibling) if (s.tagName === n.tagName) i++; - parts.unshift(tag + ':nth-of-type(' + i + ')'); + function field(label, cls) { + var l = ui('label', 'rv-field ' + (cls || '')), s = ui('select', cls ? 'rv-' + cls : ''); + l.append(ui('span', 'rv-label', label), s); + return { label: l, select: s }; + } + + var bar = ui('div', 'rv-bar'); + bar.setAttribute('role', 'region'); bar.setAttribute('aria-label', 'Review'); + var branchField = field('Branch', 'branch-pick'), fromField = field('From', 'from'), toField = field('To', 'to'); + var sw = ui('label', 'rv-switch'), swBox = ui('input'); + swBox.type = 'checkbox'; sw.append(swBox, ui('span', '', 'Show changes')); + var approve = ui('button', 'rv-approve', 'Approve'); + approve.type = 'button'; approve.hidden = true; /* only on the newest round of a branch, see updateApprove */ + var status = ui('span', 'rv-status'); status.setAttribute('role', 'status'); + /* On a narrow screen the bar is one row: the comparison and the switch. A tap on the comparison opens the selects. */ + var more = ui('div', 'rv-more'), moreToggle = ui('button', 'rv-more-toggle'); + /* the approve button is in the part that opens: on a phone, a publish takes a tap more */ + more.id = 'rv-more'; more.append(branchField.label, fromField.label, toField.label, approve); + moreToggle.type = 'button'; moreToggle.setAttribute('aria-controls', more.id); moreToggle.setAttribute('aria-expanded', 'false'); + bar.append(moreToggle, more, sw, status); + var versionParts = [fromField.label, toField.label, sw]; + function setOpen(open) { + bar.classList.toggle('rv-open', open); + document.documentElement.classList.toggle('rv-bar-open', open); + moreToggle.setAttribute('aria-expanded', String(open)); + } + moreToggle.addEventListener('click', function () { setOpen(!bar.classList.contains('rv-open')); }); + bar.addEventListener('keydown', function (e) { + if (e.key === 'Escape' && bar.classList.contains('rv-open')) { setOpen(false); moreToggle.focus(); } + }); + + /* the choice per branch in localStorage; "last" is the newest version when it was saved */ + function load(branch) { + try { return JSON.parse(localStorage.getItem('review-bar:' + branch)) || {}; } catch (e) { return {}; } + } + function save(branch, s) { + try { localStorage.setItem('review-bar:' + branch, JSON.stringify(s)); } catch (e) { /* no storage: the choice is not kept */ } + } + + var texts = {}; /* rev -> promise of the HTML of this page at rev, or null */ + function text(branch, rev) { + if (!(rev in texts)) { + texts[rev] = fetch(API + 'text?' + q(branch) + '&rev=' + encodeURIComponent(rev) + '&path=' + encodeURIComponent(location.pathname)) + .then(function (r) { if (!r.ok) throw new Error(String(r.status)); return r.json(); }) + .then(function (d) { return d.html; }, function (err) { delete texts[rev]; throw err; }); /* keep no failure */ + } + return texts[rev]; + } + function parse(html) { return new DOMParser().parseFromString(html, 'text/html'); } + + /* ---- the version on screen: the page on disk, or the content of main of an older version ---- + Only main changes, so the header, the nav and the footer keep the listeners of site.js. A page without main + changes its whole body. */ + var own = null; /* {el, nodes}: the content of the page on disk while an older version is on screen */ + var shown = null; /* the rev of the older version on screen, or null */ + function contentOf(el) { + return Array.prototype.filter.call(el.childNodes, function (n) { return n !== bar; }); + } + function put(el, nodes) { + nodes.forEach(function (n) { el.insertBefore(n, el === document.body ? bar : null); }); + } + function showOwn() { + if (!own) return; + contentOf(own.el).forEach(function (n) { n.remove(); }); + put(own.el, own.nodes); + own = null; shown = null; + } + function showOther(rev, html) { + if (shown === rev) return; + if (!own) { var el = document.querySelector('main') || document.body; own = { el: el, nodes: contentOf(el) }; } + contentOf(own.el).forEach(function (n) { n.remove(); }); + var nodes; + if (html === null) { + nodes = [ui('p', 'rv-missing wrap', 'This page is not in this version.')]; + } else { + var doc = parse(html), from = own.el === document.body ? doc.body : (doc.querySelector('main') || doc.body); + /* the scripts of the page ran once already */ + Array.prototype.forEach.call(from.querySelectorAll('script'), function (s) { s.remove(); }); + /* a copy first: adoptNode takes each node out of the live childNodes list */ + nodes = Array.prototype.slice.call(from.childNodes).map(function (n) { return document.adoptNode(n); }); } - return 'body > ' + parts.join(' > '); + put(own.el, nodes); + shown = rev; } - var tab = el('button', 'rv-tab', 'Note'), badge = el('span', 'rv-badge'); - tab.type = 'button'; tab.setAttribute('aria-haspopup', 'dialog'); tab.appendChild(badge); - var panel = el('div', 'rv-panel'); panel.setAttribute('role', 'dialog'); panel.setAttribute('aria-label', 'Note on this page'); panel.hidden = true; - var head = el('div', 'rv-head'), title = el('strong', '', 'Note on this page'), branch = el('span', 'rv-branch'), close = el('button', 'rv-x', 'Close'); - close.type = 'button'; head.append(title, branch, close); - var text = el('textarea', 'rv-text'); text.rows = 5; text.maxLength = 4000; text.placeholder = 'What must change? The note keeps the page, the width and the theme.'; - var partRow = el('div', 'rv-row'), pick = el('button', 'rv-btn', 'Point at a part'), part = el('span', 'rv-part'), unpick = el('button', 'rv-link', 'Remove'); - pick.type = unpick.type = 'button'; unpick.hidden = true; partRow.append(pick, part, unpick); - var sendRow = el('div', 'rv-row'), send = el('button', 'rv-btn rv-send', 'Save note'), status = el('span', 'rv-status'); - send.type = 'button'; status.setAttribute('role', 'status'); sendRow.append(send, status); - var listHead = el('div', 'rv-list-head', 'My notes'), list = el('ol', 'rv-list'); - panel.append(head, text, partRow, sendRow, listHead, list); - var box = el('div', 'rv-box'); box.hidden = true; + /* ---- the marks ---- */ + var fallbacks = []; /* {el, nodes}: blocks with inline markup that got plain marked text */ + function unmark() { + var parents = []; + Array.prototype.forEach.call(document.querySelectorAll('ins.rv-add'), function (n) { + if (parents.indexOf(n.parentNode) < 0) parents.push(n.parentNode); + n.replaceWith.apply(n, Array.prototype.slice.call(n.childNodes)); + }); + Array.prototype.forEach.call(document.querySelectorAll('del.rv-del, .rv-gone, .rv-newpage'), function (n) { + if (n.parentNode && parents.indexOf(n.parentNode) < 0) parents.push(n.parentNode); + n.remove(); + }); + fallbacks.forEach(function (f) { f.el.replaceChildren.apply(f.el, f.nodes); }); + fallbacks = []; + parents.forEach(function (p) { p.normalize(); }); + } - var target = null, selection = ''; + /* Every text node of el, and for each character of the block text (blank runs as one space, trimmed) its node and + offset. The block text is the text that blocks() of textdiff.js gives. */ + function textMap(el) { + var nodes = [], at = [], text = '', blank = false; + var walk = document.createTreeWalker(el, NodeFilter.SHOW_TEXT); + for (var t = walk.nextNode(); t; t = walk.nextNode()) { + nodes.push(t); + for (var i = 0; i < t.data.length; i++) { + if (/\s/.test(t.data[i])) { if (!blank && text) { text += ' '; at.push({ node: t, off: i }); } blank = true; } + else { text += t.data[i]; at.push({ node: t, off: i }); blank = false; } + } + } + if (text.slice(-1) === ' ') { text = text.slice(0, -1); at.pop(); } + return { nodes: nodes, at: at, text: text }; + } + function del(textOf) { var d = document.createElement('del'); d.className = 'rv-del'; d.textContent = textOf; return d; } + function ins() { var n = document.createElement('ins'); n.className = 'rv-add'; return n; } - function state() { - fetch(API + 'state').then(function (r) { return r.json(); }).then(function (s) { - branch.textContent = (s.branch || 'detached') + (s.commit ? ' · ' + s.commit : ''); - badge.textContent = s.open ? String(s.open) : ''; - }).catch(function () { branch.textContent = 'no server'; }); - } - function showList() { - fetch(API + 'notes').then(function (r) { return r.json(); }).then(function (d) { - list.textContent = ''; - if (!d.notes.length) { list.appendChild(el('li', 'rv-empty', 'No notes yet.')); return; } - d.notes.slice(0, 20).forEach(function (n) { - var li = el('li', 'rv-item rv-' + n.status); - li.append(el('span', 'rv-meta', (n.status === 'done' ? 'Done' : 'Open') + ' · ' + (n.path || '') + ' · ' + (n.time || '').slice(0, 16).replace('T', ' ')), - el('span', 'rv-note', n.note)); - if (n.answer) li.appendChild(el('span', 'rv-answer', n.answer)); - list.appendChild(li); - }); - }).catch(function () { list.textContent = ''; list.appendChild(el('li', 'rv-empty', 'The notes cannot load.')); }); - } - function open() { - panel.hidden = false; tab.setAttribute('aria-expanded', 'true'); state(); showList(); - setTimeout(function () { text.focus(); }, 0); - } - function shut() { panel.hidden = true; tab.setAttribute('aria-expanded', 'false'); stopPick(); tab.focus(); } + /* Put the word marks into the text nodes of el, so links and inline markup stay. */ + function markWords(el, words) { + var map = textMap(el); + var live = words.filter(function (w) { return w.op !== 'del'; }).map(function (w) { return w.text; }).join(''); + if (live !== map.text || !map.at.length) return markPlain(el, words); + var edits = [], pos = 0; + words.forEach(function (w) { + if (w.op === 'eq') pos += w.text.length; + else if (w.op === 'add') { edits.push({ from: pos, to: pos + w.text.length }); pos += w.text.length; } + else edits.push({ from: pos, removed: w.text }); + }); + /* from the end to the start: a split keeps the head of a text node in that node, so earlier offsets stay true */ + for (var k = edits.length - 1; k >= 0; k--) { + var e = edits[k]; + if (e.removed !== undefined) { + var p = e.from < map.at.length ? map.at[e.from] : { node: map.at[map.at.length - 1].node, off: map.at[map.at.length - 1].off + 1 }; + var rest = p.node.splitText(p.off); + rest.parentNode.insertBefore(del(e.removed), rest); + } else { + var s = map.at[e.from], last = map.at[e.to - 1]; + var i0 = map.nodes.indexOf(s.node), i1 = map.nodes.indexOf(last.node); + for (var i = i1; i >= i0; i--) { + var node = map.nodes[i]; + var a = i === i0 ? s.off : 0, b = i === i1 ? last.off + 1 : node.data.length; + if (a >= b) continue; + var mid = node.splitText(a); + if (b - a < mid.data.length) mid.splitText(b - a); + var wrap = ins(); + mid.parentNode.insertBefore(wrap, mid); + wrap.appendChild(mid); + } + } + } + } + /* The text of el does not match the block text: replace it with the marked text, and keep the old nodes for unmark. */ + function markPlain(el, words) { + fallbacks.push({ el: el, nodes: Array.prototype.slice.call(el.childNodes) }); + el.replaceChildren(); + words.forEach(function (w) { + if (w.op === 'eq') el.append(w.text); + else if (w.op === 'del') el.append(del(w.text)); + else { var n = ins(); n.textContent = w.text; el.append(n); } + }); + } - /* the text that is selected on the page, read before the tap on the tab can clear it */ - tab.addEventListener('pointerdown', function () { selection = String(getSelection() || '').trim().slice(0, 1000); }); - tab.addEventListener('click', function () { if (panel.hidden) open(); else shut(); }); - close.addEventListener('click', shut); - document.addEventListener('keydown', function (e) { if (e.key === 'Escape' && !panel.hidden) { if (picking) stopPick(); else shut(); } }); + function root() { return document.querySelector('main') || document.body; } + function mark(fromHtml) { + if (fromHtml === null) { + var line = ui('p', 'rv-newpage', 'New page in this round'); + root().insertBefore(line, root().firstChild); + return; + } + var live = diff.blockNodes(document), byKey = {}; + live.forEach(function (b) { byKey[b.key] = b.el; }); + var ops = diff.diffBlocks(diff.blocks(parse(fromHtml)), live.map(function (b) { return { key: b.key, text: b.text }; })); + var gone = [], lastEl = null; + function gonesBefore(el) { + gone.forEach(function (t) { var g = ui('div', 'rv-gone', t); el.parentNode.insertBefore(g, el); }); + gone = []; + } + ops.forEach(function (o) { + if (o.op === 'del') { gone.push(o.a.text); return; } + var el = byKey[o.b.key]; + gonesBefore(el); + if (o.op === 'chg') markWords(el, o.words); + else if (o.op === 'add') markWords(el, [{ op: 'add', text: o.b.text }]); + lastEl = el; + }); + gone.forEach(function (t) { + var g = ui('div', 'rv-gone', t); + if (lastEl) { lastEl.after(g); lastEl = g; } else root().appendChild(g); + }); + } - /* point at a part: the outline follows the pointer, a click takes that part */ - var picking = false; - function ours(n) { return n === tab || tab.contains(n) || panel.contains(n) || n === box; } - function frame(n) { - var r = n.getBoundingClientRect(); - box.style.left = r.left + 'px'; box.style.top = r.top + 'px'; box.style.width = r.width + 'px'; box.style.height = r.height + 'px'; - box.hidden = false; - } - function onMove(e) { var n = document.elementFromPoint(e.clientX, e.clientY); if (n && !ours(n)) frame(n); } - function onPick(e) { - var n = document.elementFromPoint(e.clientX, e.clientY); - if (!n || ours(n)) return; - e.preventDefault(); e.stopPropagation(); - var r = n.getBoundingClientRect(); - target = { selector: selectorOf(n), text: (n.innerText || n.alt || '').trim().replace(/\s+/g, ' ').slice(0, 300), - rect: { x: Math.round(r.left + scrollX), y: Math.round(r.top + scrollY), w: Math.round(r.width), h: Math.round(r.height) } }; - part.textContent = target.text ? '“' + target.text.slice(0, 60) + '”' : target.selector; - unpick.hidden = false; stopPick(); frame(n); panel.hidden = false; text.focus(); - } - function stopPick() { - if (!picking) return; - picking = false; document.documentElement.classList.remove('rv-picking'); pick.textContent = 'Point at a part'; - document.removeEventListener('pointermove', onMove, true); document.removeEventListener('click', onPick, true); - if (!target) box.hidden = true; - } - pick.addEventListener('click', function () { - if (picking) return stopPick(); - picking = true; document.documentElement.classList.add('rv-picking'); pick.textContent = 'Click a part (Esc stops)'; - panel.hidden = true; - document.addEventListener('pointermove', onMove, true); document.addEventListener('click', onPick, true); + /* ---- approve: the host checks, merges and publishes the newest round (approve-host.sh) ---- */ + var job = { state: 'idle' }; /* the status of the host, from GET /api/approve */ + var mine = null; /* the request that this page sent: then only a status of that request counts */ + var sent = 0; /* Date.now() when the request of this page was sent, 0 when a status of it arrived */ + var polling = false, diffNote = '', approveNote = '', goneNote = ''; + function newest() { return v && v.rounds.length ? v.rounds[v.rounds.length - 1] : null; } + function updateApprove() { + var last = newest(); + /* only a commit can ship, not Working. The host refuses a worktree with changes, so then no button either. + A round that is published already has no button. */ + var published = last && job.state === 'done' && job.branch === v.branch && job.sha === last.sha; + approve.hidden = !(last && v.branch !== v.base && choice.to === last.sha && !v.working && !published); + if (last) approve.textContent = 'Approve round ' + last.n; + approve.disabled = job.state === 'running'; + } + function showStatus() { + var parts = [], fresh = Date.now() - Date.parse(job.time || '') < 3600 * 1000; + if (goneNote) parts.push(goneNote); + if (diffNote) parts.push(diffNote); + if (approveNote) parts.push(approveNote); + else if (job.state === 'running') parts.push('Publishing' + (job.branch ? ' ' + job.branch : '') + ': ' + job.step + '.'); + else if (job.state === 'failed' && (fresh || (v && job.branch === v.branch))) { + parts.push('Publishing ' + job.branch + ' failed at "' + job.step + '": ' + job.line); + } else if (job.state === 'done' && (fresh || (v && job.branch === v.branch))) { + parts.push('Published ' + job.branch + '.'); + if (job.pr) { var a = ui('a', '', 'Open the PR'); a.href = job.pr; parts.push(a); } + } + status.replaceChildren(); + parts.forEach(function (p, i) { if (i) status.append(' '); status.append(p); }); + } + /* GET /api/approve now, then every 2 s while the host runs */ + function poll() { + if (polling) return; + polling = true; + (function tick() { + fetch(API + 'approve') + .then(function (r) { if (!r.ok) throw new Error(String(r.status)); return r.json(); }) + .then(function (s) { + /* the host copies branch, sha and time of the request into each status */ + if (!mine || (s.branch === mine.branch && s.sha === mine.sha && s.request === mine.time)) { + if (mine) sent = 0; + /* after a publish the branch is gone: load the versions again, so the bar shows what is here now */ + if (job.state === 'running' && s.state === 'done') loadVersions(); + job = s; + } + }) + .catch(function () { /* the next tick asks again */ }) + .then(function () { + if (sent && Date.now() - sent >= HOST_WAIT) { + sent = 0; + job = { state: 'idle' }; + approveNote = 'No host took the request. On the host, run: systemctl --user status t7d-review-approve.path'; + } + updateApprove(); showStatus(); + if (job.state === 'running') setTimeout(tick, 2000); else polling = false; + }); + })(); + } + approve.addEventListener('click', function () { + var last = newest(); + if (!last || !window.confirm('Publish ' + v.branch + ' at round ' + last.n + ' to t7d.dev?')) return; + approveNote = ''; + approve.disabled = true; + fetch(API + 'approve', { + method: 'POST', headers: { 'Content-Type': 'application/json', 'X-T7d-Review': '1' }, + body: JSON.stringify({ branch: v.branch, sha: last.sha }) + }).then(function (r) { + return r.json().then(function (d) { if (!r.ok) throw new Error(d.error || String(r.status)); return d; }); + }).then(function (req) { + mine = req; + sent = Date.now(); + job = { state: 'running', step: 'wait for the host', branch: v.branch, time: req.time }; + updateApprove(); showStatus(); poll(); + }).catch(function (err) { + approveNote = 'The approve was refused: ' + err.message + '.'; + updateApprove(); showStatus(); + }); }); - unpick.addEventListener('click', function () { target = null; part.textContent = ''; unpick.hidden = true; box.hidden = true; }); - send.addEventListener('click', function () { - var note = text.value.trim(); - if (!note) { status.textContent = 'Write the note first.'; text.focus(); return; } - send.disabled = true; status.textContent = 'Saving …'; - var body = { note: note, path: location.pathname + location.hash, title: document.title, theme: theme(), selection: selection, - viewport: { w: innerWidth, h: innerHeight, dpr: devicePixelRatio }, scroll: { x: Math.round(scrollX), y: Math.round(scrollY) }, - target: target, agent: navigator.userAgent }; - fetch(API + 'notes', { method: 'POST', headers: { 'Content-Type': 'application/json', 'X-T7d-Review': '1' }, body: JSON.stringify(body) }) - .then(function (r) { return r.json().then(function (d) { if (!r.ok) throw new Error(d.error || r.status); return d; }); }) - .then(function (d) { - status.textContent = 'Saved (' + d.id + ').'; - text.value = ''; selection = ''; target = null; part.textContent = ''; unpick.hidden = true; box.hidden = true; - state(); showList(); - }) - .catch(function (err) { status.textContent = 'Not saved: ' + err.message + '. The text stays here.'; }) - .then(function () { send.disabled = false; }); + /* ---- the bar ---- */ + var v = null, versions = [], choice = {}, run = 0; + function onDisk() { return versions[versions.length - 1].value; } + function nameOf(value) { + for (var i = 0; i < versions.length; i++) if (versions[i].value === value) return versions[i].short; + return value; + } + function render() { + var mine = ++run; + moreToggle.textContent = nameOf(choice.to) + ' vs ' + nameOf(choice.from); + unmark(); + diffNote = ''; + updateApprove(); showStatus(); + var to = choice.to, from = choice.from; + var shownReady = to === onDisk() ? Promise.resolve(showOwn()) : text(v.branch, to).then(function (html) { + if (mine === run) showOther(to, html); + }); + return shownReady.then(function () { + if (mine !== run || !choice.show || from === to) return; + return text(v.branch, from).then(function (html) { if (mine === run) mark(html); }); + }).catch(function () { if (mine === run) { diffNote = 'The changes cannot load.'; showStatus(); } }); + } + function remember() { save(v.branch, { show: choice.show, from: choice.from, to: choice.to, last: onDisk() }); } + function fill(select, list, value) { + select.replaceChildren(); + list.forEach(function (o) { var n = ui('option', '', o.label); n.value = o.value; select.appendChild(n); }); + select.value = value; + } + + function start(data) { + v = data; + goneNote = v.gone ? 'Branch ' + v.gone + ' is not here any more. It was published or removed.' : ''; + showStatus(); + fill(branchField.select, v.branches.map(function (b) { return { value: b, label: b }; }), v.branch); + versions = [{ value: v.base, label: v.base, short: v.base }].concat(v.rounds.map(function (r) { + return { value: r.sha, label: 'Round ' + r.n + ': ' + r.subject, short: 'Round ' + r.n }; + })); + if (v.working) versions.push({ value: 'working', label: 'Working', short: 'Working' }); + if (versions.length < 2) { + unmark(); showOwn(); /* after a publish the bar loads again: no marks of the old branch stay */ + moreToggle.textContent = v.branch; + versionParts.forEach(function (n) { n.hidden = true; }); + return Promise.resolve(); + } + var values = versions.map(function (o) { return o.value; }); + var saved = load(v.branch), same = saved.last === onDisk(); + choice.show = saved.show !== false; + choice.to = same && values.indexOf(saved.to) >= 0 ? saved.to : onDisk(); + choice.from = same && values.indexOf(saved.from) >= 0 ? saved.from : values[Math.max(0, values.indexOf(choice.to) - 1)]; + fill(fromField.select, versions, choice.from); + fill(toField.select, versions, choice.to); + swBox.checked = choice.show; + return render(); + } + + branchField.select.addEventListener('change', function () { + location.href = location.pathname + '?branch=' + encodeURIComponent(branchField.select.value) + location.hash; }); + fromField.select.addEventListener('change', function () { choice.from = fromField.select.value; remember(); render(); }); + toField.select.addEventListener('change', function () { choice.to = toField.select.value; remember(); render(); }); + swBox.addEventListener('change', function () { choice.show = swBox.checked; remember(); render(); }); - function mount() { document.body.append(tab, panel, box); state(); } + function loadVersions() { + return fetch(API + 'versions?' + q(asked)) + .then(function (r) { if (!r.ok) throw new Error(String(r.status)); return r.json(); }) + .then(function (data) { versionParts.forEach(function (n) { n.hidden = false; }); texts = {}; return start(data); }) + .catch(function () { diffNote = 'The review server does not answer.'; showStatus(); }); + } + function mount() { + document.body.appendChild(bar); + document.documentElement.classList.add('rv-has-bar'); + moreToggle.textContent = 'Review'; + versionParts.forEach(function (n) { n.hidden = true; }); + loadVersions().then(function () { bar.setAttribute('data-ready', ''); poll(); }); + } if (document.body) mount(); else document.addEventListener('DOMContentLoaded', mount); })(); diff --git a/scripts/review/show.py b/scripts/review/show.py deleted file mode 100644 index a8db480..0000000 --- a/scripts/review/show.py +++ /dev/null @@ -1,78 +0,0 @@ -#!/usr/bin/env python3 -"""Show where a review note points: open the page at the width and in the theme of the note, frame the part in red, -and save /.png next to the note (it stays out of git). - - uv run --no-project --with playwright python3 scripts/review/show.py [ ...] [--suffix after] - ---suffix after writes .after.png: run it after the fix and compare the two pictures. The script serves the -working tree itself (devserver.py on a free port), so it shows the files as they are now. -""" -import argparse -import json -import os -import sys -import threading -from functools import partial -from http.server import ThreadingHTTPServer - -HERE = os.path.dirname(os.path.abspath(__file__)) -sys.path.insert(0, HERE) -import devserver # noqa: E402 - -FRAME = """([sel, rect, scroll]) => { - let r = null; const n = sel && document.querySelector(sel); - if (n) { n.scrollIntoView({ block: 'center', behavior: 'instant' }); r = n.getBoundingClientRect(); } - else { scrollTo({ left: scroll.x || 0, top: scroll.y || 0, behavior: 'instant' }); - if (rect && rect.w) r = { left: rect.x - scrollX, top: rect.y - scrollY, width: rect.w, height: rect.h }; } - for (const x of document.querySelectorAll('.rv-tab, .rv-panel, .rv-box')) x.remove(); - if (!r) return n ? 'found' : 'no part'; - const b = document.createElement('div'); document.body.appendChild(b); - Object.assign(b.style, { position: 'fixed', zIndex: 2147483647, pointerEvents: 'none', border: '4px solid #e0201b', - left: (r.left - 6) + 'px', top: (r.top - 6) + 'px', width: (r.width + 12) + 'px', height: (r.height + 12) + 'px' }); - return n ? 'found' : 'by position'; -}""" - - -def main(): - ap = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) - ap.add_argument("ids", nargs="+") - ap.add_argument("--suffix", default="") - ap.add_argument("--notes", default=os.path.join(HERE, "..", "..", ".review", "notes")) - a = ap.parse_args() - from playwright.sync_api import sync_playwright - - root = os.path.normpath(os.path.join(HERE, "..", "..")) - notes = os.path.abspath(a.notes) - h = partial(type("H", (devserver.Handler,), {"root": root, "notes": notes}), directory=root) - srv = ThreadingHTTPServer(("127.0.0.1", 0), h) - srv.daemon_threads = True - threading.Thread(target=srv.serve_forever, daemon=True).start() - base = "http://127.0.0.1:%d" % srv.server_port - - with sync_playwright() as pw: - chrome = "/usr/bin/google-chrome" # the system Chrome fits every Playwright version, a cached build only one - browser = pw.chromium.launch(executable_path=chrome) if os.path.exists(chrome) else pw.chromium.launch() - for nid in a.ids: - path = next((p for p in (os.path.join(notes, nid + ".json"), os.path.join(notes, "done", nid + ".json")) if os.path.isfile(p)), None) - if not path: - print("%s: no such note" % nid) - continue - n = json.loads(devserver.read(path)) - vp, theme = n.get("viewport") or {}, n.get("theme") if n.get("theme") in ("light", "dark") else "light" - ctx = browser.new_context(viewport={"width": int(vp.get("w") or 1440), "height": int(vp.get("h") or 900)}, - device_scale_factor=min(float(vp.get("dpr") or 1), 2), color_scheme=theme) - ctx.add_init_script("try { localStorage.setItem('theme', %s) } catch (e) {}" % json.dumps(theme)) - page = ctx.new_page() - page.goto(base + (n.get("path") or "/"), wait_until="networkidle") - t = n.get("target") or {} - how = page.evaluate(FRAME, [t.get("selector"), t.get("rect"), n.get("scroll") or {}]) - out = os.path.join(os.path.dirname(path), nid + ("." + a.suffix if a.suffix else "") + ".png") - page.screenshot(path=out) - ctx.close() - print("%s: %s (%s, %s px, %s) -> %s" % (nid, n.get("note", "")[:60], n.get("path"), vp.get("w"), how, out)) - browser.close() - srv.shutdown() - - -if __name__ == "__main__": - main() diff --git a/scripts/review/test_approve.py b/scripts/review/test_approve.py new file mode 100644 index 0000000..1ae2d06 --- /dev/null +++ b/scripts/review/test_approve.py @@ -0,0 +1,490 @@ +"""Tests of the approve flow: approve.py, approve-host.sh, the approve API of devserver.py and ship.sh . +Run: python3 -m unittest scripts/review/test_approve.py + +Each test makes a bare repo as origin, a main clone on master and the worktree of t/a with two rounds. +Fake gh and ship scripts log their arguments. Nothing goes to the network. +""" +import json +import os +import shutil +import signal +import subprocess +import sys +import tempfile +import threading +import unittest +import urllib.error +import urllib.request +from functools import partial +from http.server import ThreadingHTTPServer + +HERE = os.path.dirname(os.path.abspath(__file__)) +sys.path.insert(0, HERE) +import approve # noqa: E402 +import devserver # noqa: E402 + +HOST = os.path.join(HERE, "approve-host.sh") +SHIP = os.path.join(HERE, "..", "ship.sh") +PR_URL = "https://github.com/Tim-Conrad/Tim-Conrad.github.io/pull/7" + +# gh: logs each call, one per line, as JSON. "pr list" finds no PR (FAKE_PR_MERGED and FAKE_PR_LIST set the answer +# for the merged and the open lookup), "pr create" prints the URL of the new PR. +FAKE_GH = """#!/usr/bin/env bash +python3 -c 'import json, sys; print(json.dumps(sys.argv[1:]))' "$@" >>"$LOG_DIR/gh.log" +case "$1 $2" in + "pr list") if [[ "$*" == *MERGED* ]]; then echo "${FAKE_PR_MERGED:-}"; else echo "${FAKE_PR_LIST:-}"; fi ;; + "pr create") echo "%s" ;; +esac +""" % PR_URL + +# ship: logs its arguments, then merges the sha into master of this clone, as the real merge and pull would. +FAKE_SHIP = """#!/usr/bin/env bash +echo "$*" >>"$LOG_DIR/ship.log" +git -c user.name=t -c user.email=t@t merge -q --no-edit "$2" +""" + + +def git(cwd, *args): + return subprocess.run(["git", "-c", "user.name=t", "-c", "user.email=t@t", *args], cwd=cwd, check=True, + capture_output=True, text=True).stdout.strip() + + +def write(path, text, mode=None): + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w") as f: + f.write(text) + if mode: + os.chmod(path, mode) + + +class Fixture(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + base = os.path.realpath(self.tmp.name) + self.origin, self.main = os.path.join(base, "origin.git"), os.path.join(base, "main") + self.wt, self.logs = os.path.join(base, "wts", "t-a"), os.path.join(base, "logs") + os.makedirs(self.logs) + git(base, "init", "-q", "--bare", "-b", "master", self.origin) + os.mkdir(self.main) + git(self.main, "init", "-q", "-b", "master") + write(os.path.join(self.main, "index.html"), "base\n") + write(os.path.join(self.main, "scripts", "check.py"), "print('check ok')\n") + git(self.main, "add", "-A") + git(self.main, "commit", "-m", "base") + git(self.main, "remote", "add", "origin", self.origin) + git(self.main, "push", "-q", "origin", "master") + git(self.main, "worktree", "add", "-b", "t/a", self.wt) + for text in ("Round one", "Round two"): + write(os.path.join(self.wt, "index.html"), text + "\n") + git(self.wt, "commit", "-am", text) + self.head = git(self.main, "rev-parse", "t/a") + self.approve_dir = os.path.join(self.main, ".review", "approve") + self.status_dir = os.path.join(self.main, ".review", "approve-status") + os.makedirs(self.approve_dir) + os.makedirs(self.status_dir) + write(os.path.join(base, "bin", "gh"), FAKE_GH, 0o755) + write(os.path.join(base, "bin", "ship"), FAKE_SHIP, 0o755) + self.env = dict(os.environ, LOG_DIR=self.logs, REVIEW_MAIN=self.main, + REVIEW_GH=os.path.join(base, "bin", "gh"), REVIEW_SHIP=os.path.join(base, "bin", "ship")) + + def tearDown(self): + self.tmp.cleanup() + + def request(self, sha=None): + return approve.write_request(self.main, self.approve_dir, "t/a", sha or self.head, "agent", + status_dir=self.status_dir) + + def host(self, timeout=60): + return subprocess.run(["bash", HOST], env=self.env, capture_output=True, text=True, timeout=timeout) + + def new_round(self, check_py): + """Round three changes only scripts/check.py of the worktree. It becomes the head.""" + write(os.path.join(self.wt, "scripts", "check.py"), check_py) + git(self.wt, "commit", "-qam", "Round three") + self.head = git(self.main, "rev-parse", "t/a") + + def gh_calls(self, verb): + return [json.loads(line) for line in self.log("gh.log") if json.loads(line)[:2] == ["pr", verb]] + + def status(self): + with open(os.path.join(self.status_dir, "status.json")) as f: + return json.load(f) + + def log(self, name): + path = os.path.join(self.logs, name) + if not os.path.exists(path): + return [] + with open(path) as f: + return f.read().splitlines() + + def remote_branch(self): + return subprocess.run(["git", "rev-parse", "--verify", "-q", "refs/heads/t/a"], cwd=self.origin, + capture_output=True, text=True).stdout.strip() + + +class Host(Fixture): + def test_ship_gets_the_approved_sha(self): + round1 = git(self.main, "rev-parse", "t/a~1") + req = self.request() + res = self.host() + self.assertEqual(res.returncode, 0, res.stdout + res.stderr) + self.assertEqual(self.log("ship.log"), ["7 " + self.head]) + self.assertEqual(self.remote_branch(), self.head) + st = self.status() + self.assertEqual((st["state"], st["pr"], st["branch"], st["sha"]), ("done", PR_URL, "t/a", self.head)) + self.assertEqual(st["request"], req["time"]) + lists = self.gh_calls("list") + self.assertTrue(lists) + for args in lists + self.gh_calls("create"): + self.assertEqual(args[args.index("--base") + 1], "master", args) + create = [json.loads(line) for line in self.log("gh.log") if '"create"' in line] + self.assertEqual(len(create), 1) + args = create[0] + self.assertEqual(args[args.index("--title") + 1], "Round one") + body = args[args.index("--body") + 1] + self.assertIn("1. %s Round one" % round1[:7], body) + self.assertIn("2. %s Round two" % self.head[:7], body) + self.assertEqual(body.rstrip("\n").splitlines()[-1], + "\U0001F916 Generated with [Claude Code](https://claude.com/claude-code)") + self.assertNotIn("claude.ai/code/session", body) + + def test_an_open_pr_is_not_created_again(self): + self.env["FAKE_PR_LIST"] = PR_URL + self.request() + self.assertEqual(self.host().returncode, 0) + self.assertFalse([line for line in self.log("gh.log") if '"create"' in line]) + self.assertEqual(self.log("ship.log"), ["7 " + self.head]) + + def test_moved_branch_stops_before_push(self): + self.request() + write(os.path.join(self.wt, "index.html"), "Round three\n") + git(self.wt, "commit", "-am", "Round three") + res = self.host() + self.assertNotEqual(res.returncode, 0) + st = self.status() + self.assertEqual(st["state"], "failed") + self.assertIn("head", st["step"]) + self.assertEqual(self.remote_branch(), "") + self.assertEqual(self.log("ship.log"), []) + self.assertTrue(os.path.isdir(self.wt)) + + def test_dirty_worktree_stops(self): + self.request() + write(os.path.join(self.wt, "index.html"), "edit that is not committed\n") + self.assertNotEqual(self.host().returncode, 0) + st = self.status() + self.assertEqual(st["state"], "failed") + self.assertIn("clean", st["step"]) + self.assertEqual(self.remote_branch(), "") + self.assertEqual(self.log("ship.log"), []) + + def test_failed_check_stops(self): + self.new_round("raise SystemExit('check: bad CSP')\n") + self.request() + self.assertNotEqual(self.host().returncode, 0) + st = self.status() + self.assertEqual((st["state"], st["line"]), ("failed", "check: bad CSP")) + self.assertEqual(self.remote_branch(), "") + + def test_rejected_push_stops(self): + other = os.path.join(os.path.dirname(self.main), "other") + git(os.path.dirname(self.main), "clone", "-q", self.origin, other) + git(other, "checkout", "-q", "-b", "t/a") + write(os.path.join(other, "x.txt"), "remote only\n") + git(other, "add", "x.txt") + git(other, "commit", "-m", "remote only") + git(other, "push", "-q", "origin", "t/a") + remote = self.remote_branch() + self.request() + self.assertNotEqual(self.host().returncode, 0) + st = self.status() + self.assertEqual(st["state"], "failed") + self.assertIn("push", st["step"]) + self.assertEqual(self.remote_branch(), remote) + self.assertEqual(self.log("ship.log"), []) + self.assertFalse([line for line in self.log("gh.log") if '"create"' in line]) + + def test_worktree_is_removed_after_done(self): + self.request() + self.assertEqual(self.host().returncode, 0) + self.assertFalse(os.path.exists(self.wt)) + self.assertEqual(subprocess.run(["git", "rev-parse", "--verify", "-q", "refs/heads/t/a"], cwd=self.main, + capture_output=True).returncode, 1) + self.assertFalse(os.path.exists(os.path.join(self.approve_dir, "request.json"))) + + def test_commit_during_check_stops_before_push(self): + self.new_round("import subprocess\nsubprocess.run(['git', '-c', 'user.name=t', '-c', 'user.email=t@t', 'commit', " + "'-q', '--allow-empty', '-m', 'during the check'], check=True)\n") + self.request() + self.assertNotEqual(self.host().returncode, 0) + st = self.status() + self.assertEqual(st["state"], "failed") + self.assertIn("head", st["step"]) + self.assertEqual(self.remote_branch(), "") + self.assertEqual(self.log("ship.log"), []) + + def test_edit_during_check_stops_before_push(self): + self.new_round("open('index.html', 'a').write('edit during the check')\n") + self.request() + self.assertNotEqual(self.host().returncode, 0) + st = self.status() + self.assertEqual(st["state"], "failed") + self.assertIn("clean", st["step"]) + self.assertEqual(self.remote_branch(), "") + + def test_merged_pr_skips_push_and_create(self): + """A run after a failure that came after the merge: no new push and no new PR, ship.sh does the rest.""" + self.env["FAKE_PR_MERGED"] = PR_URL + self.request() + res = self.host() + self.assertEqual(res.returncode, 0, res.stdout + res.stderr) + self.assertEqual(self.remote_branch(), "") + self.assertEqual(self.gh_calls("create"), []) + self.assertEqual(self.log("ship.log"), ["7 " + self.head]) + self.assertEqual(self.status()["state"], "done") + + def test_term_marks_the_run_failed(self): + """systemd stops the unit with SIGTERM to its cgroup: the status says killed, not running.""" + started = os.path.join(self.logs, "check-started") + self.new_round("import time\nopen(%r, 'w').close()\ntime.sleep(30)\n" % started) + self.request() + proc = subprocess.Popen(["bash", HOST], env=self.env, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + start_new_session=True) + for _ in range(200): + if os.path.exists(started): + break + threading.Event().wait(0.05) + self.assertTrue(os.path.exists(started)) + os.killpg(proc.pid, signal.SIGTERM) + proc.wait(timeout=15) + st = self.status() + self.assertEqual((st["state"], st["step"], st["line"]), ("failed", "run scripts/check.py", "killed")) + self.assertEqual(self.remote_branch(), "") + + def odd_request(self, make): + path = os.path.join(self.approve_dir, "request.json") + make(path) + res = self.host(timeout=20) + self.assertNotEqual(res.returncode, 0) + st = self.status() + self.assertEqual((st["state"], st["step"]), ("failed", "read the request")) + self.assertFalse(os.path.lexists(path)) + self.assertEqual(sorted(os.listdir(self.status_dir)), [".lock", "status.json"]) + self.assertEqual(self.log("gh.log"), []) + + def test_folder_request_fails_and_is_removed(self): + def make(path): + os.mkdir(path) + write(os.path.join(path, "x"), "{}") + self.odd_request(make) + + def test_symlink_request_fails_and_is_removed(self): + self.odd_request(lambda path: os.symlink("/dev/urandom", path)) + + def test_fifo_request_fails_and_is_removed(self): + self.odd_request(os.mkfifo) + + def test_big_request_is_read_only_in_part(self): + self.odd_request(lambda path: write(path, '{"branch": "t/a", "sha": "%s", "pad": "%s"}' % (self.head, "x" * 5000))) + + def test_invalid_request_fails_and_is_removed(self): + write(os.path.join(self.approve_dir, "request.json"), '{"branch": "--force", "sha": "x"}') + self.assertNotEqual(self.host().returncode, 0) + self.assertEqual(self.status()["state"], "failed") + self.assertFalse(os.path.exists(os.path.join(self.approve_dir, "request.json"))) + self.assertEqual(self.log("gh.log"), []) + + +class Request(Fixture): + def test_request_has_the_round_and_is_atomic(self): + req = self.request() + with open(os.path.join(self.approve_dir, "request.json")) as f: + self.assertEqual(json.load(f), req) + self.assertEqual((req["branch"], req["sha"], req["round"], req["by"]), ("t/a", self.head, 2, "agent")) + self.assertEqual(os.listdir(self.approve_dir), ["request.json"]) + + def test_second_request_is_refused(self): + self.request() + with self.assertRaises(approve.Refused) as e: + self.request() + self.assertEqual(e.exception.code, 409) + cli = subprocess.run([sys.executable, os.path.join(HERE, "approve.py"), "t/a", "--root", self.main, + "--timeout", "1"], capture_output=True, text=True) + self.assertEqual(cli.returncode, 1) + self.assertIn("already", cli.stderr) + + def test_cli_in_a_worktree_writes_to_the_main_clone(self): + """An agent runs approve.py of its own worktree with no --root: the request goes to the main clone.""" + here = os.path.join(self.wt, "scripts", "review") + os.makedirs(here) + for name in ("approve.py", "gitread.py"): + shutil.copy(os.path.join(HERE, name), here) + cli = subprocess.run([sys.executable, os.path.join(here, "approve.py"), "t/a", "--timeout", "0.2", "--poll", "0.05"], + cwd=self.wt, capture_output=True, text=True) + self.assertIn("no host took the request", cli.stderr) + with open(os.path.join(self.approve_dir, "request.json")) as f: + self.assertEqual(json.load(f)["sha"], self.head) + + def test_request_is_refused_while_a_run_is_going(self): + st = {"state": "running", "step": "push t/a", "line": "", "pr": None, "branch": "t/b", "sha": "b" * 40, + "request": "2026-11-12T00:00:00Z", "time": approve.now()} + write(os.path.join(self.status_dir, "status.json"), json.dumps(st)) + with self.assertRaises(approve.Refused) as e: + self.request() + self.assertEqual(e.exception.code, 409) + self.assertEqual(os.listdir(self.approve_dir), []) + st["time"] = "2026-11-12T00:00:00Z" # stale: a run that stopped without a word blocks nothing + write(os.path.join(self.status_dir, "status.json"), json.dumps(st)) + self.request() + + def test_cli_ignores_the_status_of_another_request(self): + proc = subprocess.Popen([sys.executable, os.path.join(HERE, "approve.py"), "t/a", "--root", self.main, + "--poll", "0.05", "--timeout", "30"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, + text=True) + path = os.path.join(self.approve_dir, "request.json") + for _ in range(200): + if os.path.exists(path): + break + threading.Event().wait(0.05) + with open(path) as f: + req = json.load(f) + os.remove(path) + st = {"state": "done", "step": "published t/a", "line": "", "pr": PR_URL, "branch": "t/a", "sha": self.head, + "request": "2026-01-01T00:00:00Z", "time": approve.now()} + write(os.path.join(self.status_dir, "status.json"), json.dumps(st)) + threading.Event().wait(0.5) + self.assertIsNone(proc.poll(), "approve.py took the status of another request") + st.update(state="failed", step="push t/a", line="rejected", pr=None, request=req["time"]) + write(os.path.join(self.status_dir, "status.json"), json.dumps(st)) + out, err = proc.communicate(timeout=10) + self.assertEqual(proc.returncode, 1) + self.assertIn("rejected", err) + + def test_old_round_and_master_are_refused(self): + with self.assertRaises(approve.Refused) as e: + self.request(self.head_of_round_one()) + self.assertEqual(e.exception.code, 409) + with self.assertRaises(approve.Refused): + approve.write_request(self.main, self.approve_dir, "master", git(self.main, "rev-parse", "master"), "agent", + status_dir=self.status_dir) + self.assertEqual(os.listdir(self.approve_dir), []) + + def head_of_round_one(self): + return git(self.main, "rev-parse", "t/a~1") + + def test_cli_follows_the_status_to_done(self): + """approve.py writes the request, then prints each step that the host writes and exits 0 on done.""" + proc = subprocess.Popen([sys.executable, os.path.join(HERE, "approve.py"), "t/a", "--root", self.main, + "--poll", "0.1", "--timeout", "60"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, + text=True) + req = os.path.join(self.approve_dir, "request.json") + for _ in range(100): + if os.path.exists(req): + break + threading.Event().wait(0.05) + self.assertEqual(self.host().returncode, 0) + out, err = proc.communicate(timeout=30) + self.assertEqual(proc.returncode, 0, err) + self.assertIn(PR_URL, out) + + +class Api(Fixture): + def setUp(self): + super().setUp() + h = partial(type("H", (devserver.Handler,), {"root": self.main, "approve_dir": self.approve_dir, + "status_dir": self.status_dir}), directory=self.main) + self.srv = ThreadingHTTPServer(("127.0.0.1", 0), h) + self.srv.daemon_threads = True + threading.Thread(target=self.srv.serve_forever, daemon=True).start() + self.base = "http://127.0.0.1:%d/__review/api/approve" % self.srv.server_port + + def tearDown(self): + self.srv.shutdown() + self.srv.server_close() + super().tearDown() + + def call(self, body=None, header=True): + headers = {"Content-Type": "application/json"} + if header: + headers["X-T7d-Review"] = "1" + data = None if body is None else json.dumps(body).encode() + req = urllib.request.Request(self.base, data=data, headers=headers, method="GET" if body is None else "POST") + try: + r = urllib.request.urlopen(req) + return r.status, json.loads(r.read()) + except urllib.error.HTTPError as e: + return e.code, json.loads(e.read() or b"{}") + + def test_api_approve_needs_header_and_head_sha(self): + req = os.path.join(self.approve_dir, "request.json") + self.assertEqual(self.call({"branch": "t/a", "sha": self.head}, header=False)[0], 403) + self.assertEqual(self.call({"branch": "t/a", "sha": git(self.main, "rev-parse", "t/a~1")})[0], 409) + self.assertEqual(self.call({"branch": "t/a", "sha": "nope"})[0], 400) + self.assertEqual(self.call({"branch": "../x", "sha": self.head})[0], 400) + self.assertFalse(os.path.exists(req)) + code, body = self.call({"branch": "t/a", "sha": self.head}) + self.assertEqual(code, 200) + self.assertEqual((body["branch"], body["sha"], body["round"], body["by"]), ("t/a", self.head, 2, "button")) + with open(req) as f: + self.assertEqual(json.load(f)["sha"], self.head) + self.assertEqual(self.call({"branch": "t/a", "sha": self.head})[0], 409) + + def test_api_status_is_idle_then_the_status_file(self): + self.assertEqual(self.call(), (200, {"state": "idle"})) + st = {"state": "running", "step": "push t/a", "line": "", "pr": None, "branch": "t/a", "sha": self.head, + "time": approve.now()} + write(os.path.join(self.status_dir, "status.json"), json.dumps(st)) + self.assertEqual(self.call(), (200, st)) + + def test_api_refuses_while_a_run_is_going(self): + st = {"state": "running", "step": "push t/b", "line": "", "pr": None, "branch": "t/b", "sha": "b" * 40, + "request": approve.now(), "time": approve.now()} + write(os.path.join(self.status_dir, "status.json"), json.dumps(st)) + self.assertEqual(self.call({"branch": "t/a", "sha": self.head})[0], 409) + self.assertFalse(os.path.exists(os.path.join(self.approve_dir, "request.json"))) + + def test_api_without_the_approve_folder_is_503(self): + shutil.rmtree(self.approve_dir) + self.assertEqual(self.call({"branch": "t/a", "sha": self.head})[0], 503) + + +class Ship(unittest.TestCase): + """ship.sh with a fake gh first in PATH: the merge call is logged, the next query fails and ends the run.""" + GH = """#!/usr/bin/env bash +echo "$*" >>"$LOG" +case "$*" in + *"--json state"*) echo OPEN ;; + *"--json title"*) echo "A title" ;; + "pr merge"*) exit 0 ;; + *) exit 1 ;; +esac +""" + + def run_ship(self, *args): + with tempfile.TemporaryDirectory() as tmp: + write(os.path.join(tmp, "gh"), self.GH, 0o755) + log = os.path.join(tmp, "gh.log") + write(log, "") + env = dict(os.environ, PATH=tmp + os.pathsep + os.environ["PATH"], LOG=log) + res = subprocess.run(["bash", SHIP, *args], env=env, capture_output=True, text=True, timeout=30) + with open(log) as f: + return res, [line for line in f.read().splitlines() if line.startswith("pr merge")] + + def test_sha_goes_to_match_head_commit(self): + sha = "a" * 40 + res, merges = self.run_ship("5", sha) + self.assertEqual(merges, ["pr merge 5 -R Tim-Conrad/Tim-Conrad.github.io --merge --delete-branch --match-head-commit " + sha], + res.stderr) + + def test_without_sha_no_flag(self): + res, merges = self.run_ship("5") + self.assertEqual(merges, ["pr merge 5 -R Tim-Conrad/Tim-Conrad.github.io --merge --delete-branch"], res.stderr) + + def test_a_bad_sha_merges_nothing(self): + res, merges = self.run_ship("5", "--admin") + self.assertNotEqual(res.returncode, 0) + self.assertEqual(merges, []) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/review/test_devserver.py b/scripts/review/test_devserver.py index 2103617..c7c0d06 100644 --- a/scripts/review/test_devserver.py +++ b/scripts/review/test_devserver.py @@ -1,6 +1,9 @@ """Tests of the review server. Run: python3 -m unittest scripts/review/test_devserver.py""" import json import os +import re +import shutil +import subprocess import sys import tempfile import threading @@ -13,6 +16,11 @@ sys.path.insert(0, os.path.dirname(__file__)) import devserver # noqa: E402 +def git(cwd, *args): + return subprocess.run(["git", "-c", "user.name=t", "-c", "user.email=t@t", *args], cwd=cwd, check=True, + capture_output=True, text=True).stdout.strip() + + PAGE = ('T

Hi

') @@ -22,6 +30,7 @@ def test_adds_assets_and_connect_src_only(self): out = devserver.inject(PAGE) self.assertIn('href="/__review/review.css"', out) self.assertIn('src="/__review/review.js" defer', out) + self.assertLess(out.index('src="/__review/textdiff.js" defer'), out.index('src="/__review/review.js"')) self.assertIn("default-src 'none'; script-src 'self'; base-uri 'none'; connect-src 'self'", out) self.assertNotIn("unsafe-inline", out) @@ -30,21 +39,21 @@ def test_keeps_an_existing_connect_src(self): self.assertEqual(devserver.inject(page).count("connect-src"), 1) -class CleanNote(unittest.TestCase): - def test_keeps_known_fields_with_limits(self): - n = devserver.clean_note({"note": " Fix it ", "path": "/x/", "theme": "dark", "evil": "x", - "viewport": {"w": 390, "h": 844, "dpr": 3, "q": 1}, - "target": {"selector": "#a", "text": "t" * 999, "rect": {"x": 1, "y": 2, "w": 3, "h": True}}}) - self.assertEqual(n["note"], "Fix it") - self.assertNotIn("evil", n) - self.assertEqual(n["viewport"], {"w": 390.0, "h": 844.0, "dpr": 3.0}) - self.assertEqual(len(n["target"]["text"]), 300) - self.assertIsNone(n["target"]["rect"]["h"]) +class Widget(unittest.TestCase): + PAGE2 = PAGE.replace("base-uri 'none'", "style-src 'self'; img-src 'self'; base-uri 'none'") - def test_rejects_an_empty_or_long_note(self): - for bad in ({"note": " "}, {"note": "x" * 4001}, {"note": 5}, ["note"]): - with self.assertRaises(ValueError): - devserver.clean_note(bad) + def test_widget_tag_and_csp_only_with_the_env(self): + self.assertNotIn("papercuts", devserver.inject(PAGE).lower()) + self.assertNotIn("papercuts", devserver.inject(PAGE, widget="").lower()) + out = devserver.inject(self.PAGE2, widget="https://p.example", branch="feat/x", commit="abc1234") + self.assertEqual(out.count("