From 9d518118b211d68408216171522818052b3e0ce3 Mon Sep 17 00:00:00 2001 From: kikashy Date: Thu, 22 Oct 2026 14:26:52 -0400 Subject: [PATCH 1/2] Make the declared Python venv and add jpdev codex review setup and check create the venv dev.json declares, resolving the interpreter from the PATH or a pyenv version directory, and run every Python step with it (#23). jpdev codex review runs a read-only Codex review from a brief, writes the record to a file and prints the reviewed SHA; brief-template --review prints the review skeleton (#13). Signed-off-by: kikashy --- README.md | 28 +++++++--- bin/jpdev | 98 +++++++++++++++++++++++++++++++-- docs/adopting.md | 27 ++++++++- docs/policy.md | 6 ++ lib/common.sh | 16 ++++-- scripts/dev/check.sh | 45 ++++++++++++++- templates/check.sh | 45 ++++++++++++++- templates/codex-review-brief.md | 19 +++++++ tests/run.sh | 70 +++++++++++++++++++++-- 9 files changed, 325 insertions(+), 29 deletions(-) create mode 100644 templates/codex-review-brief.md diff --git a/README.md b/README.md index 12517d7..6bdab66 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,16 @@ 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]`: 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; the prompt and post-run gate keep the tracked tree read-only. + It never commits, pushes or 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 +162,12 @@ 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, 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 +177,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..5a611a5 100755 --- a/bin/jpdev +++ b/bin/jpdev @@ -39,7 +39,12 @@ 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] + run a review with stdin closed, write its final message, refuse tracked + changes after the run, and print the reviewed SHA; 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 +401,75 @@ 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="" + 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 ;; + -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") + codex_check_brief "$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 + 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." + 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 [ "$sandbox" = workspace-write ]; then args+=(-c sandbox_workspace_write.network_access=true); fi + args+=(--json -C "$dir" -o "$out" "$prompt") + jpdev_log "codex review: model $model, effort $effort, sandbox $sandbox, in $dir; transcript $log" + (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 +789,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 +878,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 +889,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..9056952 100644 --- a/docs/adopting.md +++ b/docs/adopting.md @@ -13,7 +13,8 @@ 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). + 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 +59,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 +116,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, +but its prompt forbids repository 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 commits, pushes or 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..d643a5e 100644 --- a/docs/policy.md +++ b/docs/policy.md @@ -90,6 +90,12 @@ 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 +forbids tracked writes and the command refuses the result if HEAD or a tracked file changed. 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/scripts/dev/check.sh b/scripts/dev/check.sh index f0fe3f5..0cf2344 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,39 @@ 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: + target = Path(root, venv) + 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 +68,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..0cf2344 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,39 @@ 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: + target = Path(root, venv) + 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 +68,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..3762546 --- /dev/null +++ b/templates/codex-review-brief.md @@ -0,0 +1,19 @@ +# 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 | after fixes diff --git a/tests/run.sh b/tests/run.sh index ae6f8bb..1d561e3 100755 --- a/tests/run.sh +++ b/tests/run.sh @@ -12,6 +12,7 @@ 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=$(command -v python3) pass=0 fail=0 ok() { pass=$((pass + 1)); printf 'ok %s\n' "$1"; } @@ -168,6 +169,30 @@ 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])') +ln -s "$system_python" "$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" +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" + +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 + # the virtual environment is first on PATH for every step, whatever its toolchain pyclone=$work/pyclone git init -q -b main "$pyclone" @@ -716,7 +741,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 +901,33 @@ 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) +codex_args +out=$(echo junk | "$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" +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 [ -f "$STUB_CODEX_STDIN" ] && [ ! -s "$STUB_CODEX_STDIN" ]; then ok "codex review closes stdin"; else bad "codex review closes stdin"; 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,6 +1001,11 @@ 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 line" 0 '^merge: yes \| 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 @@ -968,7 +1025,11 @@ 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" +mkdir -p "$work/setup-pythonbin" +ln -s "$system_python" "$work/setup-pythonbin/python$python_release" +cat > "$seed/dev.json" <&1) && s=0 || s=$? +out=$(PATH=$work/setup-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" 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=$(PATH=$work/setup-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" 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" From 12544c14ef0a7e17d142d9861b4b6b48c3989145 Mon Sep 17 00:00:00 2001 From: kikashy Date: Thu, 22 Oct 2026 14:42:49 -0400 Subject: [PATCH 2/2] Harden venv paths and Codex review controls Refuse virtual-environment paths that can escape or replace the clone root. Keep Codex review network access disabled unless the caller opts in, and record the network choice before the run. Cover pyenv ordering, review subprocesses, and the two valid review verdicts. Co-Authored-By: Codex Signed-off-by: kikashy --- README.md | 10 +-- bin/jpdev | 15 ++-- docs/adopting.md | 11 +-- docs/policy.md | 5 +- lib/validate_manifest.py | 6 ++ scripts/dev/check.sh | 9 ++- templates/check.sh | 9 ++- templates/codex-review-brief.md | 3 +- templates/dev.schema.json | 2 +- tests/run.sh | 120 +++++++++++++++++++++++++++++--- 10 files changed, 157 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index 6bdab66..5bb825a 100644 --- a/README.md +++ b/README.md @@ -138,12 +138,13 @@ the same name overrides it. `config/env.example` documents each setting with its bound after the run; `--resume` continues the named thread. Every detected thread id is written to `.jpdev/output/thread`. - `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]`: checks the brief and + [--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; the prompt and post-run gate keep the tracked tree read-only. - It never commits, pushes or posts. Use `jpdev record post` to post the saved report. + 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. @@ -162,7 +163,8 @@ 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, setup and checks create it when it is missing. +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 diff --git a/bin/jpdev b/bin/jpdev index 5a611a5..7c3a0f4 100755 --- a/bin/jpdev +++ b/bin/jpdev @@ -41,9 +41,10 @@ usage: jpdev COMMAND [ARGS] changes (msg.txt, signed off), push, and open the pull request (pr.md) 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] + [--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; never commit, push or post + 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 @@ -414,7 +415,7 @@ cmd_codex() { # probes need temporary files. codex_review() { local brief="" model=$JPDEV_CODEX_MODEL_DEFAULT effort="" dir="" sandbox=workspace-write - local out="" repo="" pr="" issue="" log="" + 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 ;; @@ -426,6 +427,7 @@ codex_review() { --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 @@ -441,21 +443,22 @@ codex_review() { [ -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") - codex_check_brief "$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 [ "$sandbox" = workspace-write ]; then args+=(-c sandbox_workspace_write.network_access=true); fi + if [ "$network" -eq 1 ]; then args+=(-c sandbox_workspace_write.network_access=true); fi args+=(--json -C "$dir" -o "$out" "$prompt") - jpdev_log "codex review: model $model, effort $effort, sandbox $sandbox, in $dir; transcript $log" (cd "$dir" && codex "${args[@]}" < /dev/null > "$log" 2>&1) local status=$? [ "$status" -eq 0 ] || { echo "== codex review exit $status (transcript: $log) =="; return "$status"; } diff --git a/docs/adopting.md b/docs/adopting.md index 9056952..83f9a0b 100644 --- a/docs/adopting.md +++ b/docs/adopting.md @@ -13,8 +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; - 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). + 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). @@ -126,9 +127,9 @@ 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, -but its prompt forbids repository 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 commits, pushes or posts. +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 diff --git a/docs/policy.md b/docs/policy.md index d643a5e..91eb4e0 100644 --- a/docs/policy.md +++ b/docs/policy.md @@ -93,8 +93,9 @@ normal no-new-pull-request Codex path; a carry with no conflicts is signed and p `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 -forbids tracked writes and the command refuses the result if HEAD or a tracked file changed. A -Codex review of a Codex build is the same vendor (OpenAI), not a cross-vendor claim. +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 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 0cf2344..9d7fbb9 100755 --- a/scripts/dev/check.sh +++ b/scripts/dev/check.sh @@ -21,8 +21,13 @@ 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: - target = Path(root, venv) +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') diff --git a/templates/check.sh b/templates/check.sh index 0cf2344..9d7fbb9 100755 --- a/templates/check.sh +++ b/templates/check.sh @@ -21,8 +21,13 @@ 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: - target = Path(root, venv) +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') diff --git a/templates/codex-review-brief.md b/templates/codex-review-brief.md index 3762546..abed376 100644 --- a/templates/codex-review-brief.md +++ b/templates/codex-review-brief.md @@ -16,4 +16,5 @@ tests and documentation against them. 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 | after fixes +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 1d561e3..9b41655 100755 --- a/tests/run.sh +++ b/tests/run.sh @@ -12,7 +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=$(command -v python3) +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"; } @@ -174,7 +175,14 @@ 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])') -ln -s "$system_python" "$work/pythonbin/python$python_release" +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 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=$? +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 @@ -193,6 +227,30 @@ out=$(cd "$madeclone" && PATH=$work/pythonbin:$PATH "$jpdev" check 2>&1) && s=0 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" @@ -911,15 +969,49 @@ 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 | "$jpdev" codex review --brief "$work/review-brief.md" --model review-model --effort high \ +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" \ @@ -1005,10 +1097,11 @@ 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 line" 0 '^merge: yes \| after fixes$' "$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" @@ -1025,8 +1118,6 @@ origin=$work/origin.git seed=$work/seed git init -q --bare -b main "$origin" git init -q -b main "$seed" -mkdir -p "$work/setup-pythonbin" -ln -s "$system_python" "$work/setup-pythonbin/python$python_release" 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=$(PATH=$work/setup-pythonbin:$PATH "$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"