diff --git a/README.md b/README.md index 12517d7..5bb825a 100644 --- a/README.md +++ b/README.md @@ -86,8 +86,10 @@ the same name overrides it. `config/env.example` documents each setting with its - `jpdev setup DIR [--repo OWNER/NAME] [--branch NAME] [--base origin/main] [--manifest FILE]`: clones with `gh repo clone` (or reuses `DIR`), makes the branch from the base, selects the - manifest's toolchains (node by nvm version, go by its version when the manifest names one, python and its virtual environment), runs the - setup steps under the caps and prints one summary block. + manifest's toolchains (node by nvm version, go by its version when the manifest names one, + and Python by `python3.X` on PATH or the newest matching pyenv version), creates the declared + Python virtual environment, runs the setup steps in it under the caps, and prints one summary + block. `jpdev check` creates a missing declared environment before it runs checks too. - `jpdev check [--only NAME,...] [--skip NAME,...] [--all] [--manifest FILE]`: runs the checks in order, each under its toolchain's cap, and stops at the first failure unless `--all`. The block names each check, pass or fail, the lines its `summary` regex matched, and the first 40 @@ -135,8 +137,17 @@ the same name overrides it. `config/env.example` documents each setting with its share caches below `BASE/.jpdev-shared`; `--small` enforces the manifest handover paths and line bound after the run; `--resume` continues the named thread. Every detected thread id is written to `.jpdev/output/thread`. -- `jpdev codex brief-template`: prints `templates/codex-brief.md`, a brief skeleton that tells +- `jpdev codex review --brief FILE --model M --effort E --dir CLONE [--pr N | --issue N] + [--repo OWNER/NAME] [--out FILE] [--sandbox read-only|workspace-write] [--network]`: checks the brief and + runs `codex exec` with the model, effort and sandbox explicit and stdin closed. It writes the + final message to `--out` (default `.jpdev/output/review.md`), refuses the result if HEAD or a + tracked file changed, and prints the reviewed SHA. The default `workspace-write` sandbox lets + review probes use temporary files. It has no network unless the caller passes `--network`, the + prompt and post-run gate forbid Git writes, and it never posts. Use `jpdev record post` to post + the saved report. +- `jpdev codex brief-template [--review]`: prints `templates/codex-brief.md`, a brief skeleton that tells Codex to write `msg.txt` and `pr.md` and not to commit. + `--review` prints the reviewer skeleton with its read-only rules and findings-first report. - `jpdev codex check-brief FILE`: exits 1 and names the neutral phrasing for each word of [`config/codex-words.txt`](config/codex-words.txt) (and of the file `JPDEV_CODEX_WORDS` names) that the brief holds. `codex run` runs the same check first and does not start the run on a @@ -152,9 +163,13 @@ the same name overrides it. `config/env.example` documents each setting with its | `python` | `ulimit -v` and `timeout` | | `shell` | `timeout` only | -When the manifest declares a virtual environment, its `bin` is first on PATH for every step, whatever -the step's toolchain (the toolchain still decides the cap), so a node or Go step that runs a python -script uses the environment's interpreter and packages. +When the manifest declares a virtual environment, its path must be a non-root relative child of +the clone with no `..` segment, and setup and checks create it when it is missing. +They use `python3.X` on PATH first, then the newest `~/.pyenv/versions/3.X.*/bin/python3`, and +refuse before any step if the declared release is unavailable. Its `bin` is first on PATH for +every step, whatever the step's toolchain (the toolchain still decides the cap), so a node or Go +step that runs a Python script uses the environment's interpreter and packages. Summary rows for +Python steps, and for other steps that invoke `python3`, name the interpreter used. ## What is here @@ -164,7 +179,8 @@ script uses the environment's interpreter and packages. - `templates/dev.json`, `templates/dev.schema.json`: the manifest by example and its JSON Schema. - `templates/check.sh` and `lib/handover_check.sh`: the public checks a repository vendors as `scripts/dev/check.sh` and `scripts/dev/handover_check.sh`. -- `templates/codex-brief.md`: the brief skeleton `jpdev codex brief-template` prints. +- `templates/codex-brief.md`, `templates/codex-review-brief.md`: the build and review brief + skeletons `jpdev codex brief-template` prints. - `templates/AGENTS.md`, `templates/CLAUDE.md`: the agent instructions (one text, generated from the other by `scripts/sync-agent-docs.sh`). - `templates/ci-check.yml`: the CI step list. diff --git a/bin/jpdev b/bin/jpdev index 27a82db..7c3a0f4 100755 --- a/bin/jpdev +++ b/bin/jpdev @@ -39,7 +39,13 @@ usage: jpdev COMMAND [ARGS] model, effort and sandbox explicit, stdin closed and the transcript in the log; print the final message and the exit code; then commit the tracked changes (msg.txt, signed off), push, and open the pull request (pr.md) - codex brief-template + codex review --brief FILE --model M --effort low|medium|high --dir CLONE + [--pr N | --issue N] [--repo OWNER/NAME] [--out FILE] + [--sandbox read-only|workspace-write] [--network] + run a review with stdin closed, write its final message, refuse tracked + changes after the run, and print the reviewed SHA; no network unless + --network, and never commit, push or post + codex brief-template [--review] print the brief skeleton: what Codex writes (msg.txt, pr.md) and does not do codex check-brief [--build] FILE refuse a brief that holds a word of the filter list, and suggest the @@ -396,13 +402,77 @@ cmd_codex() { [ $# -eq 0 ] || shift case $sub in run) codex_run "$@" ;; + review) codex_review "$@" ;; check-brief) codex_check_brief "$@" ;; - brief-template) codex_brief_template ;; + brief-template) codex_brief_template "$@" ;; -h | --help | help) usage; exit 0 ;; - *) jpdev_die "codex: the subcommands are run, check-brief and brief-template" ;; + *) jpdev_die "codex: the subcommands are run, review, check-brief and brief-template" ;; esac } +# codex_review: run Codex from a review brief and retain only its final report. The prompt and the +# post-run git gate make the role read-only; workspace-write remains the default because shell +# probes need temporary files. +codex_review() { + local brief="" model=$JPDEV_CODEX_MODEL_DEFAULT effort="" dir="" sandbox=workspace-write + local out="" repo="" pr="" issue="" log="" network=0 network_state=disabled + while [ $# -gt 0 ]; do + case $1 in + --brief) need_value "$@"; brief=$2; shift 2 ;; + --model) need_value "$@"; model=$2; shift 2 ;; + --effort) need_value "$@"; effort=$2; shift 2 ;; + --dir) need_value "$@"; dir=$2; shift 2 ;; + --sandbox) need_value "$@"; sandbox=$2; shift 2 ;; + --out) need_value "$@"; out=$2; shift 2 ;; + --repo) need_value "$@"; repo=$2; shift 2 ;; + --pr) need_value "$@"; pr=$2; shift 2 ;; + --issue) need_value "$@"; issue=$2; shift 2 ;; + --network) network=1; network_state=enabled; shift ;; + -h | --help) usage; exit 0 ;; + *) jpdev_die "codex review: unknown argument $1" ;; + esac + done + [ -f "$brief" ] || jpdev_die "codex review: --brief must name a file" + [ -n "$model" ] || jpdev_die "codex review: --model is needed (the model is always passed; JPDEV_CODEX_MODEL_DEFAULT can name one)" + case $effort in low | medium | high) ;; *) jpdev_die "codex review: --effort is low, medium or high" ;; esac + case $sandbox in read-only | workspace-write) ;; *) jpdev_die "codex review: --sandbox is read-only or workspace-write" ;; esac + [ -n "$dir" ] || jpdev_die "codex review: --dir is needed" + [ -d "$dir" ] || jpdev_die "codex review: --dir is not a directory" + { [ -z "$pr" ] || [ -z "$issue" ]; } || jpdev_die "codex review: give --pr N or --issue N, not both" + dir=$(cd "$dir" && git rev-parse --show-toplevel) || jpdev_die "codex review: --dir is not a git clone" + [ -f "$dir/AGENTS.md" ] || jpdev_die "codex review: $dir holds no AGENTS.md, which the prompt names as the rules" + command -v codex > /dev/null || jpdev_die "codex review: codex is not on PATH" + brief=$(cd "$(dirname "$brief")" && pwd)/$(basename "$brief") + local sha + sha=$(git -C "$dir" rev-parse HEAD) || jpdev_die "codex review: cannot read HEAD" + if [ -z "$out" ]; then out=$dir/.jpdev/output/review.md; fi + mkdir -p "$(dirname "$out")" "$JPDEV_SCRATCH/logs" + out=$(cd "$(dirname "$out")" && pwd)/$(basename "$out") + log=$JPDEV_SCRATCH/logs/codex-review-$(basename "$dir")-$(date +%Y%m%dT%H%M%S).log + jpdev_log "codex review: model $model, effort $effort, sandbox $sandbox, in $dir; transcript $log; network $network_state" + codex_check_brief "$brief" + local prompt="The rules are this repository's AGENTS.md; follow them. Act as its reviewer. Do not change tracked files or write git metadata." + [ "$network" -eq 1 ] || prompt+=$'\n'"Do not access the network." + if [ -n "$pr" ]; then prompt+=$'\n'"Review pull request #$pr${repo:+ of $repo} at head $sha."; fi + if [ -n "$issue" ]; then prompt+=$'\n'"Review issue #$issue${repo:+ of $repo} at head $sha."; fi + prompt+=$'\n'"$(cat "$brief")" + local args=(exec -m "$model" -c "model_reasoning_effort=$effort" --sandbox "$sandbox") + if [ "$network" -eq 1 ]; then args+=(-c sandbox_workspace_write.network_access=true); fi + args+=(--json -C "$dir" -o "$out" "$prompt") + (cd "$dir" && codex "${args[@]}" < /dev/null > "$log" 2>&1) + local status=$? + [ "$status" -eq 0 ] || { echo "== codex review exit $status (transcript: $log) =="; return "$status"; } + [ -s "$out" ] || { echo "jpdev codex review: refused: codex wrote no final message" >&2; return 1; } + if [ "$(git -C "$dir" rev-parse HEAD)" != "$sha" ] || ! git -C "$dir" diff --quiet || ! git -C "$dir" diff --cached --quiet; then + echo "jpdev codex review: refused: the review changed tracked files or HEAD; restore the clone before continuing" >&2 + return 1 + fi + local verdict + verdict=$(grep -E '^(mergeable|not mergeable) — head ' "$out" | tail -n 1) + [ -z "$verdict" ] || printf '%s\n' "$verdict" + echo "jpdev codex review: reviewed $sha; record $out; transcript $log" +} + # codex_check_brief [--build] FILE: check the build heading and the filter-list words in FILE. codex_check_brief() { local file="" build=0 @@ -722,8 +792,14 @@ codex_publish() { exit 0 } -# codex_brief_template prints the brief skeleton a Codex build starts from. -codex_brief_template() { cat "$kit/templates/codex-brief.md"; } +# codex_brief_template prints the brief skeleton a Codex build or review starts from. +codex_brief_template() { + case ${1:-} in + "") cat "$kit/templates/codex-brief.md" ;; + --review) [ $# -eq 1 ] || jpdev_die "codex brief-template: --review takes no value"; cat "$kit/templates/codex-review-brief.md" ;; + *) jpdev_die "codex brief-template: unknown argument $1" ;; + esac +} # cmd_carry merges the current base into a pull-request branch. Conflicts are handed to the same # codex_run path as a normal --no-pr build, using a generated brief. @@ -805,7 +881,7 @@ PY } cmd_env() { - local v cfg + local v cfg manifest pyv venv chosen cfg=$(jpdev_config_file) printf 'jpdev %s\nkit %s\nconfig %s (%s)\n\n' "$JPDEV_VERSION" "$kit" "$cfg" "$([ -f "$cfg" ] && echo present || echo absent)" for v in "${JPDEV_SETTINGS[@]}"; do printf '%-22s %-48s %s\n' "$v" "${!v}" "${JPDEV_SOURCE[$v]}"; done @@ -816,6 +892,19 @@ cmd_env() { done printf '%-10s %s\n' python3.12 "$(jpdev_python_bin 3.12 || echo none)" printf '%-10s %s\n' node22 "$(jpdev_node_bin 22 || echo "none under $JPDEV_NVM_DIR")" + manifest=${JPDEV_ROOT:-$(git rev-parse --show-toplevel 2>/dev/null)}/dev.json + if [ -f "$manifest" ]; then + pyv=$(jpdev_manifest_get "$manifest" toolchains python version) + venv=$(jpdev_manifest_get "$manifest" toolchains python venv) + if [ -n "$venv" ] && [ -x "${manifest%/dev.json}/$venv/bin/python" ]; then + chosen=${manifest%/dev.json}/$venv/bin/python + elif [ -n "$pyv" ]; then + chosen=$(jpdev_python_bin "$pyv" || echo "not installed") + else + chosen=$(command -v python3 || echo "not on PATH") + fi + printf '%-10s %s\n' manifest-python "$chosen${pyv:+ (declared $pyv)}${venv:+ (venv $venv)}" + fi [ -f "$JPDEV_NAME_PATTERNS" ] || echo "name-pattern file absent: jpdev record post refuses to post without --no-name-check" } diff --git a/docs/adopting.md b/docs/adopting.md index dc1129c..83f9a0b 100644 --- a/docs/adopting.md +++ b/docs/adopting.md @@ -13,7 +13,9 @@ Start from `templates/dev.json` and keep what the repository runs. The manifest `jpdev` selects an installed Go that matches it and says when none is installed, and the workflow falls back to the `go.mod` version when the manifest names none), `python` (`{"version": "3.12", "venv": ".venv"}`; the version and the virtual environment are optional; - a declared virtual environment is first on PATH for every step). + the environment path is a non-root relative child of the clone with no `..` segment; setup and + checks create a missing declared environment with `python3.X` on PATH or the newest matching + pyenv version, then put its `bin` first on PATH for every step). - `setup`: steps `jpdev setup` and CI run once after a clone (`npm ci`, the python requirements). - `checks`: the check list, in the order it should run (cheap checks first). @@ -58,8 +60,14 @@ together, and copy both whole files again when updating them. `jpdev setup` name either file is absent. `scripts/dev/check.sh [--setup] [--only A,B] [--skip A,B] [--all]` runs the manifest with no -caps and prints the summary block. `jpdev check` uses the vendored copy when it exists and -sets `JPDEV_WRAP` so that each step runs under its toolchain's cap. +caps and prints the summary block. Before a setup or check run, it creates a missing declared +Python environment with `python3.X` on PATH, then the newest +`~/.pyenv/versions/3.X.*/bin/python3`; it refuses before any step when neither exists. Every step +runs with that environment's `bin` first on PATH, so the manifest's Python setup step installs its +requirements into the environment. A summary row names the interpreter for a Python step or any +step that invokes `python3`. `jpdev check` uses the vendored copy when it exists and sets +`JPDEV_WRAP` so that each step runs under its toolchain's cap. `jpdev env` shows the interpreter +selected for the current repository's manifest. ## 3. Add the agent instructions @@ -109,6 +117,20 @@ jpdev setup . --manifest dev.json jpdev check --all ``` +For a saved Codex review, start from the review skeleton and keep the clone's tracked tree +unchanged: + +```sh +jpdev codex brief-template --review > review-brief.md +jpdev codex review --brief review-brief.md --model MODEL --effort medium --dir . --pr NUMBER --repo OWNER/NAME +jpdev record post --pr NUMBER --repo OWNER/NAME --file .jpdev/output/review.md +``` + +The review command defaults to `workspace-write` so focused probes can create temporary files, +with no network access unless the caller passes `--network`. Its prompt forbids Git writes and it +refuses the result if HEAD or a tracked file changed. It closes stdin, writes only the final +message to the record, prints the reviewed SHA, and never posts. + ## Worked example: the agent repository `examples/judgment-pack-agent.dev.json` is the manifest of `judgment-pack-agent`. Its setup is diff --git a/docs/policy.md b/docs/policy.md index fc3e8c6..91eb4e0 100644 --- a/docs/policy.md +++ b/docs/policy.md @@ -90,6 +90,13 @@ normal no-new-pull-request Codex path; a carry with no conflicts is signed and p - One round per pull request. A finding small enough to fix is fixed in the pull request with its test; a second round is not opened for it. +`jpdev codex review` runs a checked review brief with an explicit model and effort, closes stdin, +and writes the final report to a file without committing, pushing or posting. The default sandbox +is `workspace-write` because read-only prevents shell probes from making temporary files; the brief +has no network unless the caller passes `--network`, forbids Git writes, and the command refuses +the result if HEAD or a tracked file changed. The command itself never posts. A Codex review of a +Codex build is the same vendor (OpenAI), not a cross-vendor claim. + ## Severity | Severity | What happens | diff --git a/lib/common.sh b/lib/common.sh index 9fd5c63..6414861 100644 --- a/lib/common.sh +++ b/lib/common.sh @@ -99,17 +99,23 @@ jpdev_go_bin() { printf '%s/bin' "$best" } +# jpdev_python_matches PYTHON MAJOR.MINOR succeeds when PYTHON reports that release. +jpdev_python_matches() { + [ "$("$1" -c 'import sys; print("%d.%d" % sys.version_info[:2])' 2>/dev/null)" = "$2" ] +} + # jpdev_python_bin MAJOR.MINOR prints the path of a working python of that version: python3.N on -# PATH when it runs (a pyenv shim for a version that is not selected does not), else the newest -# one under $PYENV_ROOT/versions (default ~/.pyenv/versions). +# PATH when it reports the release (a pyenv shim for a version that is not selected does not), else +# the newest matching version's bin/python3 under $PYENV_ROOT/versions (default ~/.pyenv/versions). jpdev_python_bin() { local found entry best="" - if found=$(command -v "python$1") && "$found" --version > /dev/null 2>&1; then printf '%s' "$found"; return 0; fi + if found=$(command -v "python$1") && jpdev_python_matches "$found" "$1"; then printf '%s' "$found"; return 0; fi for entry in "${PYENV_ROOT:-$HOME/.pyenv}"/versions/"$1".*; do - [ -x "$entry/bin/python$1" ] && best=$(printf '%s\n%s\n' "$best" "$entry" | sort -V | tail -n 1) + [ -x "$entry/bin/python3" ] && jpdev_python_matches "$entry/bin/python3" "$1" && \ + best=$(printf '%s\n%s\n' "$best" "$entry" | sort -V | tail -n 1) done [ -n "$best" ] || return 1 - printf '%s/bin/python%s' "$best" "$1" + printf '%s/bin/python3' "$best" } # jpdev_clone_key ROOT prints a short name for a clone, used for the per-clone Go cache. diff --git a/lib/validate_manifest.py b/lib/validate_manifest.py index 320244d..54a3bec 100644 --- a/lib/validate_manifest.py +++ b/lib/validate_manifest.py @@ -7,6 +7,7 @@ because the fallback without the jsonschema package has no schema. Standard library otherwise. Exit 0 when valid, 1 with one line per problem, 2 on an unreadable file. """ import json +import os import re import sys @@ -72,6 +73,11 @@ def main(): go_version = manifest.get("toolchains", {}).get("go", {}).get("version") if isinstance(manifest.get("toolchains"), dict) and isinstance(manifest["toolchains"].get("go"), dict) else None if go_version is not None and not (isinstance(go_version, str) and re.fullmatch(r"[0-9]+\.[0-9]+(\.[0-9]+)?", go_version)): problems.append("toolchains/go/version: %r is not a Go release number such as 1.26 or 1.26.5" % (go_version,)) + python = manifest.get("toolchains", {}).get("python", {}) if isinstance(manifest.get("toolchains"), dict) else {} + venv = python.get("venv") if isinstance(python, dict) else None + if venv is not None and (not isinstance(venv, str) or not venv or os.path.isabs(venv) + or ".." in venv.split("/") or os.path.normpath(venv) == "."): + problems.append("toolchains/python/venv: venv must be a relative child path of the clone") seen = set() for step in steps: name = step.get("name") diff --git a/scripts/dev/check.sh b/scripts/dev/check.sh index f0fe3f5..9d7fbb9 100755 --- a/scripts/dev/check.sh +++ b/scripts/dev/check.sh @@ -7,7 +7,8 @@ # failure unless the step is marked optional: true. Standard library python3 only. jpdev sets JPDEV_WRAP to add its caps around each step. root=${JPDEV_ROOT:-$(cd "$(dirname "$0")" && git rev-parse --show-toplevel)} || exit 2 exec python3 -I - "$root" "$@" <<'PY' -import json, os, re, signal, subprocess, sys, time +import json, os, re, shutil, signal, subprocess, sys, time +from pathlib import Path root, args = sys.argv[1], sys.argv[2:] opt = {'only': set(), 'skip': set(), 'all': False, 'setup': False, 'manifest': root + '/dev.json'} while args: @@ -17,6 +18,44 @@ while args: elif a in ('--all', '--setup'): opt[a[2:]] = True else: sys.exit('check.sh: unknown argument ' + a) m = json.load(open(opt['manifest'], encoding='utf-8')) +python = m.get('toolchains', {}).get('python', {}) +venv = python.get('venv') +run_env = dict(os.environ) +if venv is not None: + root_path, path = Path(root).resolve(), Path(venv) + target = (root_path / path).resolve() + if (not venv or path.is_absolute() or '..' in path.parts or target == root_path + or root_path not in target.parents): + print('check.sh: venv must be a relative child path of the clone', file=sys.stderr) + sys.exit(2) + interpreter = target / 'bin' / 'python3' + if not interpreter.is_file(): + version = python.get('version') + creator = None + if version: + candidate = shutil.which('python' + version) + def reports_version(path): + probe = subprocess.run([path, '-c', 'import sys; print("%d.%d" % sys.version_info[:2])'], + text=True, stdout=subprocess.PIPE, stderr=subprocess.DEVNULL) + return probe.returncode == 0 and probe.stdout.strip() == version + if candidate and reports_version(candidate): creator = candidate + if not creator: + def version_key(path): + return tuple(int(part) if part.isdigit() else part for part in path.parent.parent.name.split('.')) + candidates = sorted((path for path in Path.home().glob('.pyenv/versions/' + version + '.*/bin/python3') + if reports_version(str(path))), key=version_key) + if candidates: creator = str(candidates[-1]) + if not creator: + print('check.sh: python %s is not installed (python%s on PATH, or a pyenv version)' % + (version, version), file=sys.stderr) + sys.exit(2) + else: + creator = sys.executable + made = subprocess.run([creator, '-m', 'venv', str(target)]) + if made.returncode: + print('check.sh: could not make the virtual environment %s with %s' % (venv, creator), file=sys.stderr) + sys.exit(2) + run_env['PATH'] = str(target / 'bin') + os.pathsep + run_env.get('PATH', '') steps = m.get('setup', []) if opt['setup'] else m['checks'] + ([dict(name='parity', **m['parity'])] if 'parity' in m else []) unknown = (opt['only'] | opt['skip']) - set(s['name'] for s in steps) if unknown: print('check.sh: no step named ' + ', '.join(sorted(unknown)), file=sys.stderr); sys.exit(2) @@ -34,11 +73,18 @@ for c in steps: if wrap: cmd = [wrap, c.get('toolchain', 'shell'), '--'] + cmd tmo = c.get('timeout', 1200) t0 = time.time(); print('check: running ' + n, file=sys.stderr, flush=True) + step_env = dict(run_env, JPDEV_STEP_TIMEOUT=str(tmo)) p = subprocess.Popen(cmd, cwd=os.path.join(root, c.get('cwd', '.')), stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, text=True, errors='replace', start_new_session=True, env=dict(os.environ, JPDEV_STEP_TIMEOUT=str(tmo))) + stderr=subprocess.STDOUT, text=True, errors='replace', start_new_session=True, env=step_env) try: out, _ = p.communicate(timeout=None if wrap else tmo) except subprocess.TimeoutExpired: os.killpg(p.pid, signal.SIGKILL); out = p.communicate()[0] + '\ncheck: stopped for time\n' lines, took = out.splitlines(), '%ds' % (time.time() - t0) + if c.get('toolchain') == 'python' or re.search(r'(^|[^A-Za-z0-9_])python3([^A-Za-z0-9_]|$)', c['run']): + executable = str(Path(root, venv, 'bin', 'python3')) if venv else shutil.which('python3', path=step_env.get('PATH')) + if executable: + found = subprocess.run([executable, '--version'], text=True, stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, env=step_env).stdout.strip() + took += ', python %s (%s)' % (executable, found) if logdir: os.makedirs(logdir, exist_ok=True); open('%s/%s.log' % (logdir, n), 'w').write(out) hit = [l.strip() for l in lines if c.get('summary') and re.search(c['summary'], l)] note = [' ' + l for l in (hit if len(hit) <= 3 else ['%d lines matched; last: %s' % (len(hit), hit[-1])])] diff --git a/templates/check.sh b/templates/check.sh index f0fe3f5..9d7fbb9 100755 --- a/templates/check.sh +++ b/templates/check.sh @@ -7,7 +7,8 @@ # failure unless the step is marked optional: true. Standard library python3 only. jpdev sets JPDEV_WRAP to add its caps around each step. root=${JPDEV_ROOT:-$(cd "$(dirname "$0")" && git rev-parse --show-toplevel)} || exit 2 exec python3 -I - "$root" "$@" <<'PY' -import json, os, re, signal, subprocess, sys, time +import json, os, re, shutil, signal, subprocess, sys, time +from pathlib import Path root, args = sys.argv[1], sys.argv[2:] opt = {'only': set(), 'skip': set(), 'all': False, 'setup': False, 'manifest': root + '/dev.json'} while args: @@ -17,6 +18,44 @@ while args: elif a in ('--all', '--setup'): opt[a[2:]] = True else: sys.exit('check.sh: unknown argument ' + a) m = json.load(open(opt['manifest'], encoding='utf-8')) +python = m.get('toolchains', {}).get('python', {}) +venv = python.get('venv') +run_env = dict(os.environ) +if venv is not None: + root_path, path = Path(root).resolve(), Path(venv) + target = (root_path / path).resolve() + if (not venv or path.is_absolute() or '..' in path.parts or target == root_path + or root_path not in target.parents): + print('check.sh: venv must be a relative child path of the clone', file=sys.stderr) + sys.exit(2) + interpreter = target / 'bin' / 'python3' + if not interpreter.is_file(): + version = python.get('version') + creator = None + if version: + candidate = shutil.which('python' + version) + def reports_version(path): + probe = subprocess.run([path, '-c', 'import sys; print("%d.%d" % sys.version_info[:2])'], + text=True, stdout=subprocess.PIPE, stderr=subprocess.DEVNULL) + return probe.returncode == 0 and probe.stdout.strip() == version + if candidate and reports_version(candidate): creator = candidate + if not creator: + def version_key(path): + return tuple(int(part) if part.isdigit() else part for part in path.parent.parent.name.split('.')) + candidates = sorted((path for path in Path.home().glob('.pyenv/versions/' + version + '.*/bin/python3') + if reports_version(str(path))), key=version_key) + if candidates: creator = str(candidates[-1]) + if not creator: + print('check.sh: python %s is not installed (python%s on PATH, or a pyenv version)' % + (version, version), file=sys.stderr) + sys.exit(2) + else: + creator = sys.executable + made = subprocess.run([creator, '-m', 'venv', str(target)]) + if made.returncode: + print('check.sh: could not make the virtual environment %s with %s' % (venv, creator), file=sys.stderr) + sys.exit(2) + run_env['PATH'] = str(target / 'bin') + os.pathsep + run_env.get('PATH', '') steps = m.get('setup', []) if opt['setup'] else m['checks'] + ([dict(name='parity', **m['parity'])] if 'parity' in m else []) unknown = (opt['only'] | opt['skip']) - set(s['name'] for s in steps) if unknown: print('check.sh: no step named ' + ', '.join(sorted(unknown)), file=sys.stderr); sys.exit(2) @@ -34,11 +73,18 @@ for c in steps: if wrap: cmd = [wrap, c.get('toolchain', 'shell'), '--'] + cmd tmo = c.get('timeout', 1200) t0 = time.time(); print('check: running ' + n, file=sys.stderr, flush=True) + step_env = dict(run_env, JPDEV_STEP_TIMEOUT=str(tmo)) p = subprocess.Popen(cmd, cwd=os.path.join(root, c.get('cwd', '.')), stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, text=True, errors='replace', start_new_session=True, env=dict(os.environ, JPDEV_STEP_TIMEOUT=str(tmo))) + stderr=subprocess.STDOUT, text=True, errors='replace', start_new_session=True, env=step_env) try: out, _ = p.communicate(timeout=None if wrap else tmo) except subprocess.TimeoutExpired: os.killpg(p.pid, signal.SIGKILL); out = p.communicate()[0] + '\ncheck: stopped for time\n' lines, took = out.splitlines(), '%ds' % (time.time() - t0) + if c.get('toolchain') == 'python' or re.search(r'(^|[^A-Za-z0-9_])python3([^A-Za-z0-9_]|$)', c['run']): + executable = str(Path(root, venv, 'bin', 'python3')) if venv else shutil.which('python3', path=step_env.get('PATH')) + if executable: + found = subprocess.run([executable, '--version'], text=True, stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, env=step_env).stdout.strip() + took += ', python %s (%s)' % (executable, found) if logdir: os.makedirs(logdir, exist_ok=True); open('%s/%s.log' % (logdir, n), 'w').write(out) hit = [l.strip() for l in lines if c.get('summary') and re.search(c['summary'], l)] note = [' ' + l for l in (hit if len(hit) <= 3 else ['%d lines matched; last: %s' % (len(hit), hit[-1])])] diff --git a/templates/codex-review-brief.md b/templates/codex-review-brief.md new file mode 100644 index 0000000..abed376 --- /dev/null +++ b/templates/codex-review-brief.md @@ -0,0 +1,20 @@ +# Review brief: TITLE (pull request or issue #N of OWNER/NAME) + +## Role +Act as the reviewer. Read the issue, the repository rules and the complete change at the reviewed +head. Do not continue earlier builder reasoning. + +## Read-only rules +Do not change tracked files, create commits, push, post comments or open pull requests. You may +build once and run focused probes or mutants that leave the tracked tree unchanged. + +## Review scope +State the contract to review, the relevant files and the accepted design. Check the implementation, +tests and documentation against them. + +## Report +Findings first, ordered by severity, with file and line references. Then state the reviewed head, +the probes run, and any residual risk. End with exactly one of these lines: + +merge: yes +merge: after fixes diff --git a/templates/dev.schema.json b/templates/dev.schema.json index 2a9c573..4523eff 100644 --- a/templates/dev.schema.json +++ b/templates/dev.schema.json @@ -76,7 +76,7 @@ "venv": { "type": "string", "minLength": 1, - "description": "a virtual environment in the clone, put first on PATH for every step" + "description": "a non-root relative child path of the clone, with no .. segment; its bin is put first on PATH for every step" }, "version": { "type": "string", diff --git a/tests/run.sh b/tests/run.sh index ae6f8bb..9b41655 100755 --- a/tests/run.sh +++ b/tests/run.sh @@ -12,6 +12,8 @@ unset CI JPDEV_VENV JPDEV_LOCK JPDEV_NODE_RSS_MB JPDEV_NODE_TIMEOUT_S JPDEV_GO_U unset JPDEV_ROOT JPDEV_WRAP JPDEV_NODE_BIN JPDEV_GO_BIN JPDEV_LOGDIR JPDEV_STEP_TIMEOUT jpdev=$kit/bin/jpdev fixture=$kit/tests/fixture/dev.json +system_python=$(python3 -c 'import sys; print(sys.executable)') +system_git=$(command -v git) pass=0 fail=0 ok() { pass=$((pass + 1)); printf 'ok %s\n' "$1"; } @@ -168,6 +170,87 @@ JSON out=$(cd "$vclone" && "$jpdev" check 2>/dev/null) && s=0 || s=$? expect "the python step runs in the virtual environment the manifest names" 0 'from-the-venv' "$out" "$s" +# a missing declared environment is made with the declared interpreter before a check runs +madeclone=$work/madeclone +mkdir -p "$madeclone" "$work/pythonbin" +git init -q -b main "$madeclone" +python_release=$($system_python -c 'import sys; print("%d.%d" % sys.version_info[:2])') +python_calls=$work/python-calls +cat > "$work/pythonbin/python$python_release" <<'STUB' +#!/bin/sh +printf 'path\n' >> PYTHON_CALLS +exec SYSTEM_PYTHON "$@" +STUB +sed -i "s#PYTHON_CALLS#$python_calls#g; s#SYSTEM_PYTHON#$system_python#g" "$work/pythonbin/python$python_release" +chmod +x "$work/pythonbin/python$python_release" +cat > "$madeclone/dev.json" <&1) && s=0 || s=$? +expect "jpdev check makes a missing declared virtual environment" 0 'PASS +python-version' "$out" "$s" +if [ -x "$madeclone/made-venv/bin/python3" ]; then ok "the declared virtual environment holds python3"; else bad "the declared virtual environment holds python3"; fi +expect "a python step summary names its interpreter" 0 'python-version +.*python .*made-venv/bin/python' "$out" "$s" +if [ "$(sort -u "$python_calls")" = path ]; then ok "python3.X on PATH is tried before pyenv"; else bad "python3.X on PATH is tried before pyenv" "$(cat "$python_calls")"; fi +rm -rf "$madeclone/made-venv" +out=$(cd "$madeclone" && JPDEV_ROOT=$madeclone PATH=$work/pythonbin:$PATH \ + "$kit/templates/check.sh" --manifest "$madeclone/dev.json" 2>&1) && s=0 || s=$? +expect "the vendored runner makes a missing declared virtual environment" 0 'PASS +python-version' "$out" "$s" + +# With no matching python3.X on PATH, the newest matching pyenv version supplies the environment. +mkdir -p "$work/wrong-pythonbin" +cat > "$work/wrong-pythonbin/python$python_release" <<'STUB' +#!/bin/sh +printf '3.99\n' +STUB +chmod +x "$work/wrong-pythonbin/python$python_release" +for patch in 1 9; do + pyenv_python=$HOME/.pyenv/versions/$python_release.$patch/bin/python3 + mkdir -p "$(dirname "$pyenv_python")" + cat > "$pyenv_python" <<'STUB' +#!/bin/sh +printf '%s\n' "$0" >> PYTHON_CALLS +exec SYSTEM_PYTHON "$@" +STUB + sed -i "s#PYTHON_CALLS#$python_calls#g; s#SYSTEM_PYTHON#$system_python#g" "$pyenv_python" + chmod +x "$pyenv_python" +done +rm -rf "$madeclone/made-venv" +: > "$python_calls" +out=$(cd "$madeclone" && PATH=$work/wrong-pythonbin:$PATH "$jpdev" check 2>&1) && s=0 || s=$? +expect "jpdev check falls back from a wrong python3.X on PATH to pyenv" 0 'PASS +python-version' "$out" "$s" +expect "the newest matching pyenv version creates the environment" 0 "/$python_release\.9/bin/python3$" "$(tail -n 1 "$python_calls")" "$s" + +cat > "$madeclone/dev.json" <<'JSON' +{"version": 1, "toolchains": {"python": {"version": "3.99", "venv": "absent-venv"}}, "checks": [{"name": "must-not-run", "run": "touch ran", "toolchain": "python"}]} +JSON +out=$(cd "$madeclone" && PATH=$work/pythonbin:$PATH "$jpdev" check 2>&1) && s=0 || s=$? +expect "jpdev check refuses an unavailable declared Python before a step" 2 'python 3\.99 is not installed.*python3\.99 on PATH, or a pyenv version' "$out" "$s" +if [ ! -e "$madeclone/ran" ]; then ok "an unavailable declared Python runs no check"; else bad "an unavailable declared Python runs no check"; fi + +# A virtual environment is always a non-root child of the clone. +invalid_venvs=('empty|' 'dot|.' 'parent|../outside-venv' "absolute|$work/absolute-venv") +for row in "${invalid_venvs[@]}"; do + IFS='|' read -r label value <<< "$row" + for runner in template jpdev; do + invalid_clone=$work/invalid-$label-$runner + mkdir -p "$invalid_clone" + git init -q -b main "$invalid_clone" + printf '{"version":1,"toolchains":{"python":{"venv":"%s"}},"checks":[{"name":"must-not-run","run":"touch ran","toolchain":"python"}]}\n' "$value" > "$invalid_clone/dev.json" + rm -rf "$work/outside-venv" "$work/absolute-venv" + if [ "$runner" = template ]; then + out=$(cd "$invalid_clone" && JPDEV_ROOT=$invalid_clone "$kit/templates/check.sh" 2>&1) && s=0 || s=$? + else + out=$(cd "$invalid_clone" && "$jpdev" check 2>&1) && s=0 || s=$? + fi + expect "$runner refuses the $label virtual-environment path" 2 'venv must be a relative child path of the clone' "$out" "$s" + if [ ! -e "$invalid_clone/ran" ] && [ ! -e "$work/outside-venv" ] && [ ! -e "$work/absolute-venv" ]; then + ok "$runner creates nothing for the $label virtual-environment path" + else + bad "$runner creates nothing for the $label virtual-environment path" + fi + done +done + # the virtual environment is first on PATH for every step, whatever its toolchain pyclone=$work/pyclone git init -q -b main "$pyclone" @@ -716,7 +799,7 @@ out=$("$jpdev" codex run --no-commit --brief "$work/dirty-brief.md" --model m -- expect "codex run refuses a brief with a listed word" 1 'refused: .* holds words the content filter trips on' "$out" "$s" if [ -e "$STUB_CODEX_ARGS" ]; then bad "a refused brief did not start codex"; else ok "a refused brief did not start codex"; fi out=$("$jpdev" codex 2>&1) && s=0 || s=$? -expect "codex needs a subcommand" 2 'the subcommands are run, check-brief and brief-template' "$out" "$s" +expect "codex needs a subcommand" 2 'the subcommands are run, review, check-brief and brief-template' "$out" "$s" # the git part: commit, push, pull request, from the msg.txt and pr.md codex wrote export STUB_GIT_ARGS=$work/git-args mkdir -p "$work/ghbin" @@ -876,6 +959,67 @@ printf 'stale\n' > "$cxrepo/msg.txt" out=$(STUB_CODEX_EDIT=1 cx_run --no-push) && s=0 || s=$? expect "a msg.txt left by an earlier run is not used" 1 'wrote no msg.txt' "$out" "$s" +# codex review: a checked brief, explicit model/effort, a final-message record and no git writes +reviewrepo=$work/reviewrepo +mkdir -p "$reviewrepo" +git init -q -b review "$reviewrepo" +printf '# Rules\n' > "$reviewrepo/AGENTS.md" +printf 'unchanged\n' > "$reviewrepo/file.txt" +git -C "$reviewrepo" add . +git -C "$reviewrepo" commit -q -m base +printf 'Review the change and report findings first.\n' > "$work/review-brief.md" +review_sha=$(git -C "$reviewrepo" rev-parse HEAD) +mkdir -p "$work/review-audit" +cat > "$work/review-audit/git" <<'STUB' +#!/usr/bin/env bash +printf 'git' >> "$JPDEV_REVIEW_GIT_CALLS" +printf ' <%s>' "$@" >> "$JPDEV_REVIEW_GIT_CALLS" +printf '\n' >> "$JPDEV_REVIEW_GIT_CALLS" +exec "$JPDEV_SYSTEM_GIT" "$@" +STUB +cat > "$work/review-audit/gh" <<'STUB' +#!/usr/bin/env bash +printf 'gh' >> "$JPDEV_REVIEW_GH_CALLS" +printf ' <%s>' "$@" >> "$JPDEV_REVIEW_GH_CALLS" +printf '\n' >> "$JPDEV_REVIEW_GH_CALLS" +exit 99 +STUB +chmod +x "$work/review-audit/git" "$work/review-audit/gh" +export JPDEV_REVIEW_GIT_CALLS=$work/review-git-calls JPDEV_REVIEW_GH_CALLS=$work/review-gh-calls +export JPDEV_SYSTEM_GIT=$system_git +codex_args +out=$(echo junk | PATH=$work/review-audit:$PATH "$jpdev" codex review --brief "$work/review-brief.md" --model review-model --effort high \ + --dir "$reviewrepo" --pr 23 --repo Example/repo --out "$work/review.md" 2>&1) && s=0 || s=$? +expect "codex review prints the reviewed SHA" 0 "reviewed $review_sha" "$out" "$s" +expect "codex review logs disabled network access first" 0 '^jpdev: codex review: .*network disabled$' "$(head -n 1 <<< "$out")" "$s" +if [ "$(cat "$work/review.md")" = 'stub final message' ]; then ok "codex review writes the final message to --out"; else bad "codex review writes the final message to --out" "$(cat "$work/review.md" 2>/dev/null)"; fi +for a in exec -m review-model 'model_reasoning_effort=high' --sandbox workspace-write -C "$reviewrepo" -o "$work/review.md"; do + if has_arg "$a"; then ok "codex review passes $a"; else bad "codex review passes $a" "$(tr '\0' ' ' < "$STUB_CODEX_ARGS")"; fi +done +if ! has_arg 'sandbox_workspace_write.network_access=true'; then ok "codex review keeps network access off by default"; else bad "codex review keeps network access off by default" "$(tr '\0' ' ' < "$STUB_CODEX_ARGS")"; fi +if [ -f "$STUB_CODEX_STDIN" ] && [ ! -s "$STUB_CODEX_STDIN" ]; then ok "codex review closes stdin"; else bad "codex review closes stdin"; fi +expected_review_git=$(printf '%s\n' \ + 'git <--show-toplevel>' \ + "git <-C> <$reviewrepo> " \ + "git <-C> <$reviewrepo> " \ + "git <-C> <$reviewrepo> <--quiet>" \ + "git <-C> <$reviewrepo> <--cached> <--quiet>") +if [ "$(cat "$JPDEV_REVIEW_GIT_CALLS")" = "$expected_review_git" ]; then ok "codex review runs only its exact read-only git command set"; else bad "codex review runs only its exact read-only git command set" "$(cat "$JPDEV_REVIEW_GIT_CALLS")"; fi +if [ ! -e "$JPDEV_REVIEW_GH_CALLS" ]; then ok "codex review makes no gh call"; else bad "codex review makes no gh call" "$(cat "$JPDEV_REVIEW_GH_CALLS")"; fi + +codex_args +out=$(PATH=$work/review-audit:$PATH "$jpdev" codex review --brief "$work/review-brief.md" \ + --model review-model --effort low --dir "$reviewrepo" --network --out "$work/network-review.md" 2>&1) && s=0 || s=$? +expect "codex review logs enabled network access first" 0 '^jpdev: codex review: .*network enabled$' "$(head -n 1 <<< "$out")" "$s" +if has_arg 'sandbox_workspace_write.network_access=true'; then ok "codex review enables network only when requested"; else bad "codex review enables network only when requested" "$(tr '\0' ' ' < "$STUB_CODEX_ARGS")"; fi + +codex_args +out=$(STUB_CODEX_EDIT=1 STUB_CODEX_EDIT_FILE=file.txt "$jpdev" codex review --brief "$work/review-brief.md" \ + --model review-model --effort low --dir "$reviewrepo" --sandbox read-only 2>&1) && s=0 || s=$? +expect "codex review refuses tracked changes made during the review" 1 'refused: the review changed tracked files' "$out" "$s" +if has_arg read-only; then ok "codex review accepts and passes the read-only sandbox"; else bad "codex review accepts and passes the read-only sandbox"; fi +git -C "$reviewrepo" restore file.txt + # carry: a clean merge is signed and pushed; a conflict uses the generated Codex brief mkdir -p "$work/carrybin" cat > "$work/carrybin/gh" <<'STUB' @@ -949,9 +1093,15 @@ expect "the brief template tells codex to write pr.md" 0 'pr\.md' "$out" "$s" expect "the brief template tells codex not to commit" 0 'do not run .git commit.' "$out" "$s" out=$("$jpdev" codex check-brief --build "$kit/templates/codex-brief.md" 2>&1) && s=0 || s=$? expect "the brief template passes check-brief --build" 0 'no word of the filter list' "$out" "$s" +out=$("$jpdev" codex brief-template --review 2>&1) && s=0 || s=$? +expect "the review brief template names the reviewer role" 0 '^# Review brief:' "$out" "$s" +expect "the review brief template requires findings first" 0 'Findings first' "$out" "$s" +expect "the review brief template forbids repository writes" 0 'Do not change tracked files' "$out" "$s" +expect "the review brief template carries the merge yes line" 0 '^merge: yes$' "$out" "$s" +expect "the review brief template carries the merge after-fixes line" 0 '^merge: after fixes$' "$out" "$s" unset STUB_GIT_ARGS STUB_GH_CALLS STUB_GH_ARGS STUB_GH_BODY GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL PATH=$oldpath -unset STUB_CODEX_ARGS STUB_CODEX_CWD STUB_CODEX_STDIN STUB_CODEX_ENV +unset STUB_CODEX_ARGS STUB_CODEX_CWD STUB_CODEX_STDIN STUB_CODEX_ENV JPDEV_REVIEW_GIT_CALLS JPDEV_REVIEW_GH_CALLS JPDEV_SYSTEM_GIT # --- env printf 'JPDEV_NODE_RSS_MB=4321\nJPDEV_GO_TIMEOUT_S=99\n' > "$XDG_CONFIG_HOME/jpdev.env.tmp" @@ -968,7 +1118,9 @@ origin=$work/origin.git seed=$work/seed git init -q --bare -b main "$origin" git init -q -b main "$seed" -cp "$fixture" "$seed/dev.json" +cat > "$seed/dev.json" <&1) && s=0 || s=$? +: > "$python_calls" +out=$(PYENV_ROOT=$HOME/.pyenv PATH=$work/wrong-pythonbin:$PATH "$jpdev" setup "$work/clone" --repo Example/repo --branch topic --base origin/main 2>&1) && s=0 || s=$? expect "setup makes the branch from the base" 0 'branch +topic at' "$out" "$s" expect "setup runs the manifest's setup steps" 0 'PASS +prepare' "$out" "$s" expect "setup names the toolchains" 0 'python +Python' "$out" "$s" expect "setup names both scripts in its adoption guidance" 0 'vendor scripts/dev/check\.sh and scripts/dev/handover_check\.sh together' "$out" "$s" +expect "setup makes the declared virtual environment before its steps" 0 '/setup-venv/bin/python' "$(cat "$work/clone/prepared-by" 2>/dev/null)" "$s" +expect "setup chooses the newest matching pyenv version" 0 "/$python_release\.9/bin/python3$" "$(tail -n 1 "$python_calls")" "$s" if [ "$(git -C "$work/clone" rev-parse --abbrev-ref HEAD)" = topic ]; then ok "setup leaves the clone on the new branch"; else bad "setup leaves the clone on the new branch"; fi -out=$("$jpdev" setup "$work/clone" --branch topic 2>&1) && s=0 || s=$? +out=$(PYENV_ROOT=$HOME/.pyenv PATH=$work/wrong-pythonbin:$PATH "$jpdev" setup "$work/clone" --branch topic 2>&1) && s=0 || s=$? expect "setup reuses an existing clone and an existing branch" 0 'branch +topic' "$out" "$s" +cat > "$work/clone/dev.json" <<'JSON' +{"version":1,"toolchains":{"python":{"venv":"../setup-outside"}},"checks":[{"name":"must-not-run","run":"touch ran","toolchain":"python"}]} +JSON +rm -rf "$work/setup-outside" +out=$("$jpdev" setup "$work/clone" --branch topic 2>&1) && s=0 || s=$? +expect "setup refuses a virtual environment outside the clone" 2 'venv must be a relative child path of the clone' "$out" "$s" +if [ ! -e "$work/setup-outside" ] && [ ! -e "$work/clone/ran" ]; then ok "setup creates nothing for an outside virtual environment"; else bad "setup creates nothing for an outside virtual environment"; fi out=$("$jpdev" setup "$work/not-a-clone" 2>&1) && s=0 || s=$? expect "setup asks for --repo when the directory is not a clone" 2 'give --repo' "$out" "$s"