mirror of
https://github.com/razzant/ouroboros.git
synced 2026-08-31 02:24:51 +00:00
review: fix the trusted-base handoff and scope what it guarantees
Adversarial review of the handoff, four accepted findings. Artifact paths (real bug): --output and --drive-root were forwarded verbatim while the base-side run executes with cwd set to the temporary trusted checkout, so a relative path resolved inside that checkout and was deleted with it when the review returned. Both are absolutized against the invoking cwd before the child argv is built. Argv robustness: the forwarded options move to equals form (--base-ref=/--head-ref=/--goal=/--scope=/--output=/--drive-root=). A goal, scope or title beginning with "-" was parsed as a flag by the child, whose argparse error surfaced as exit 2 — the code this script reserves for an empty diff. Test strength: the contract pin called the helper directly, so deleting the main() hook kept it green. A new end-to-end test invokes the REAL wrapper as a process from a checkout that is not the base and proves the base-side probe executed; removing the hook fails it (verified by mutation). The seeded repo gains a real (non-namespace) ouroboros package and the repository's own __pycache__ ignores, so an out-of-repo wrapper resolves its import there and importing does not dirty the tree. A second test pins the artifact-path and flag-shaped-value behavior through the same probe. Honesty: the guarantee is scoped wherever it is claimed (module and function docstrings, packet trust text, CONTRIBUTING, DEVELOPMENT, ARCHITECTURE). The handoff removes the dependency on which checkout the operator happened to stand in; the wrapper performing it is still read from the invoking checkout, so that checkout must be trusted — the same trust root as before, now stated instead of assumed. The stale D31 legend in scripts/v7_migration.py and the stale contributor-trust-boundary line in docs/ARCHITECTURE.md are synced to the new contract, and the MIGRATION_v7.md row no longer claims the replacement test covers eleven paths: it parametrizes three representative proposals. Co-authored-by: Ouroboros <311266734+ouroboros-agent@users.noreply.github.com>
This commit is contained in:
Ouroboros
parent
e53b7f95f8
commit
fca3dcee5b
7 changed files with 137 additions and 43 deletions
|
|
@ -185,8 +185,10 @@ The review always runs on the target-base code, whatever your PR touches. If the
|
|||
checkout you invoke it from is not already the target base, the command
|
||||
materializes that commit in a temporary worktree and re-runs itself there, so
|
||||
your proposal is reviewed by the base's review machinery and never by its own.
|
||||
Commit your work first: an uncommitted change is refused rather than silently
|
||||
left out of the reviewed snapshot.
|
||||
The command performing that handoff is still the one in the checkout you invoke,
|
||||
so a maintainer reproducing your packet should invoke it from a checkout they
|
||||
trust. Commit your work first: an uncommitted change is refused rather than
|
||||
silently left out of the reviewed snapshot.
|
||||
|
||||
## 6. Open the Pull Request
|
||||
|
||||
|
|
|
|||
|
|
@ -834,7 +834,7 @@ place so a normal row can carry it.
|
|||
| scripts/run_external_review.py::_REVIEW_SUBSTRATE_PATHS | retired:the contributor lane always executes the target base's review machinery, so no diff is classified | - | {"id":"D31","note":"owner decision 2026-08-19 (\"always run the review on the old version\"): the hand-list, its anchors-plus-name-rule successor and the base-flow import closure all retire with the decision they fed; _run_on_trusted_base materializes the base commit and re-runs the review from it unless this process already runs on it, so a proposal can never review itself and the packet records unconditional trusted execution instead of a per-proposal rerun demand"} | tests/test_external_review_script.py::test_contributor_review_always_runs_on_the_trusted_base | {"status":"retired","note":"retired on the v7 WIP; pending upstream transfer"} |
|
||||
| tests/test_external_review_script.py::_REVIEW_SUBSTRATE_PATHS | retired:the boundary characterization retires with the classifier it proved | - | {"id":"D31","note":"owner decision 2026-08-19: with no per-proposal classification left there is no boundary membership to characterize; the replacement pins are the unconditional-base-execution parametrization (a proposal touching nothing review-related and one rewriting the review machinery take the same path) and the in-place case when the executing tree already is the target base"} | tests/test_external_review_script.py::test_contributor_review_always_runs_on_the_trusted_base | {"status":"retired","note":"retired on the v7 WIP; pending upstream transfer"} |
|
||||
| tests/test_external_review_script.py::test_contributor_trust_boundary_covers_functional_review_dependencies | retired:no boundary classifies the functional review dependencies any more | - | {"id":"D31","note":"owner decision 2026-08-19: the proof that every functional review dependency fell inside the trust boundary retires with the boundary; the target base's machinery reviews every proposal whichever dependency it touches"} | tests/test_external_review_script.py::test_contributor_review_always_runs_on_the_trusted_base | {"status":"retired","note":"retired on the v7 WIP; pending upstream transfer"} |
|
||||
| tests/test_external_review_script.py::test_contributor_snapshot_flags_transitive_review_substrate_changes | retired:the snapshot carries no substrate flag left to characterize | - | {"id":"D31","note":"owner decision 2026-08-19: the snapshot stopped computing review_substrate_changed and the base-flow import closure behind it, so the eleven-path parametrization has no observable; the replacement pins that a proposal touching those same paths takes the unconditional trusted-base path like any other"} | tests/test_external_review_script.py::test_contributor_review_always_runs_on_the_trusted_base | {"status":"retired","note":"retired on the v7 WIP; pending upstream transfer"} |
|
||||
| tests/test_external_review_script.py::test_contributor_snapshot_flags_transitive_review_substrate_changes | retired:the snapshot carries no substrate flag left to characterize | - | {"id":"D31","note":"owner decision 2026-08-19: the snapshot stopped computing review_substrate_changed and the base-flow import closure behind it, so the eleven-path parametrization has no observable left; the replacement is a three-way parametrization of representative proposals (one touching nothing review-related, one touching a review-stack module, one rewriting the review script) all taking the same trusted-base handoff, not a per-path enumeration"} | tests/test_external_review_script.py::test_contributor_review_always_runs_on_the_trusted_base | {"status":"retired","note":"retired on the v7 WIP; pending upstream transfer"} |
|
||||
| tests/test_external_review_script.py::test_contributor_outcome_fails_closed_on_receipt_or_trust_drift | tests/test_external_review_script.py::test_contributor_outcome_fails_closed_on_receipt_drift_only | - | {"id":"D31","note":"owner decision 2026-08-19: the trust half of the characterization goes with the downgrade it asserted (finalize_contributor_outcome no longer takes the snapshot at all), and the renamed test keeps the execution-receipt fail-closed half plus a pin that a clean run is not downgraded by what the proposal touches"} | tests/test_external_review_script.py::test_contributor_outcome_fails_closed_on_receipt_drift_only | {"status":"pending","note":"implemented on the v7 WIP; pending upstream transfer"} |
|
||||
| ouroboros/config.py::ENDPOINT_AUTHORED_SETTINGS | ouroboros/settings_defaults.py::ENDPOINT_AUTHORED_SETTINGS | ouroboros/config.py::ENDPOINT_AUTHORED_SETTINGS | {"id":"none","note":"verbatim extraction into the settings-vocabulary owner (shipped values, retired keys, disk-only classification) preserves behavior, exact text, and re-exported identity; provider_models now imports this leaf instead of lazily importing config"} | tests/test_config_extraction.py::test_config_facade_reexports_every_moved_identity | {"status":"pending","note":"implemented on the v7 WIP; pending upstream transfer"} |
|
||||
| ouroboros/config.py::SETTINGS_KEYS_NOT_EXPORTED_TO_ENV | ouroboros/settings_defaults.py::SETTINGS_KEYS_NOT_EXPORTED_TO_ENV | ouroboros/config.py::SETTINGS_KEYS_NOT_EXPORTED_TO_ENV | {"id":"none","note":"verbatim extraction into the settings-vocabulary owner (shipped values, retired keys, disk-only classification) preserves behavior, exact text, and re-exported identity; provider_models now imports this leaf instead of lazily importing config"} | tests/test_config_extraction.py::test_config_facade_reexports_every_moved_identity | {"status":"pending","note":"implemented on the v7 WIP; pending upstream transfer"} |
|
||||
|
|
|
|||
|
|
@ -101,7 +101,7 @@ server.py (Starlette+uvicorn) ← HTTP + WebSocket on configurable host:port (de
|
|||
├── loop.py ← High-level LLM tool loop; one-shot no-op-attempt finalization nudge (declared expected_output + zero effects + no FINAL ANSWER); (v6.51.0) a one-shot ADVISORY red-verification finalization nudge (ordered before the receipt-absent nudge) when the latest host-attested verify receipt is unreconciled-RED (`outcomes.latest_unreconciled_failed_verification`) — re-check / explain / fix; (v6.52.2) a one-shot ADVISORY masked-verification nudge (ordered after the red nudge) when the latest PASSing verify check can launder its exit code (`outcomes.latest_unreconciled_masked_verification`) — re-ground without the masking pipe or explain; (v6.53.0) continuous explicit `FINAL ANSWER:` latching captures the latest typed candidate every round (tool-count-stamped, no prose mining) so review/nudge/forced-finalization paths do not erase a structured answer, and intrinsic no-deadline pacing asks for a salvageable current answer on long tasks; (v6.60.0) ALL marker prompting (P2 marker nudge, pacing salvage phrases, the context instruction) is gated on `task_contract.answer_protocol="final_answer_line"` via the `answer_protocol_active` SSOT — the latch/extractor stay unconditional; (v6.61.4) the protocol gate is SUFFICIENT for the P2 marker nudge — it no longer also requires a declared `expected_output` (a contract may carry the deliverable in `objective` while `expected_output` is empty; the latter therefore cannot suppress that salvage surface), and `extract_final_answer` structurally rejects the snake_case outcome-tier ledger identifiers (`best_effort`/`blocked_with_evidence`) as answers — internal enum vocabulary is never a deliverable (a reviewed run shipped `FINAL ANSWER: blocked_with_evidence` verbatim); `solved` stays extractable as an ordinary English word; (v6.90.0) a one-shot NANNY finalization nudge (ordered first): a child dispatched onto the delegated substrate (executor=harness) finalizing with ZERO `delegate_start` calls gets one structural reminder to delegate or state why not, and the forced-finalization paths carry the same fact as a NOTE inside their one final prompt instead of re-looping; (2026-08-10 amendments) the nudge reads durable custody evidence from the CANONICAL (budget) root via `delegate_custody.custody_root` — the same root the writes land on, so split-root children are no longer blind — and branches PENDING ≠ FAILED: a started-but-unsettled run gets a "still pending — delegate_wait before finalizing" reminder (never a failure accusation, which would invite a duplicate concurrent run), `NANNY_DELEGATED_RUN_FAILED` is reserved for terminal non-success settles, and the nudge is suppressed entirely when the delegate verbs are policy-hidden from the child's toolset
|
||||
├── loop_messages.py ← Owner-message text plumbing (L-B leaf; public home of _append_or_merge_user_content/_evict_stale_image_blocks after L3)
|
||||
├── loop_acceptance.py ← Task-acceptance fence and obligations (L-B leaf)
|
||||
├── loop_acceptance_review.py ← The host acceptance review run (L-B leaf; agent-side review surface, outside the contributor trust boundary)
|
||||
├── loop_acceptance_review.py ← The host acceptance review run (L-B leaf; agent-side review surface, not part of the contributor review flow)
|
||||
├── loop_round_limits.py ← Round limits, owner-stop drain, compaction, terminal handlers (L-B leaf)
|
||||
├── loop_nudges.py ← Mid-task steering nudges: self-check, budget milestones, nanny economics, finalization (L-B leaf)
|
||||
├── loop_model_call.py ← The per-round model call and context-fit machinery (L-B leaf)
|
||||
|
|
@ -413,7 +413,7 @@ scripts/build_linux_packages.sh ← Wraps the x86_64 Linux payload into dependen
|
|||
scripts/smoke_linux_packages.sh ← Release-gating `apt`/`dnf` CLI + installed-unit + desktop-launcher smoke on Ubuntu 22.04/Fedora plus a non-blocking vendor-registry lane for Astra Linux/RED OS
|
||||
build_windows.ps1 ← Windows build (PyInstaller → .zip)
|
||||
scripts/build_repo_bundle.py ← Builds `repo.bundle` + `repo_bundle_manifest.json` for packaged releases
|
||||
scripts/run_external_review.py ← dual-lane non-committing review wrapper. The default operator lane reviews the staged tree through the production advisory→triad→scope cycle with resolved production policy. `--contributor` reviews an exact committed target-base..head proposal in a detached checkout, freezes the configured `api_chat`/`agent_session` triad and scope rows with blocking enforcement, excludes advisory, rejects contributor version-carrier changes, and emits a redacted SHA-bound packet. Unprovable or contradictory configured→dispatched→observed binding makes the packet `INCOMPLETE`. The review machinery is always the target base's own (owner decision, 2026-08-19): unless the invoking checkout already is the target base, the lane materializes that commit in a detached worktree and re-runs the review from it, so no proposal is trusted to review itself and nothing about a diff's contents is classified. `READY_FOR_INTEGRATION` is evidence, never merge authority: final metadata and exact landing-tree review belong to maintainers.
|
||||
scripts/run_external_review.py ← dual-lane non-committing review wrapper. The default operator lane reviews the staged tree through the production advisory→triad→scope cycle with resolved production policy. `--contributor` reviews an exact committed target-base..head proposal in a detached checkout, freezes the configured `api_chat`/`agent_session` triad and scope rows with blocking enforcement, excludes advisory, rejects contributor version-carrier changes, and emits a redacted SHA-bound packet. Unprovable or contradictory configured→dispatched→observed binding makes the packet `INCOMPLETE`. The review machinery is always the target base's own (owner decision, 2026-08-19): unless the invoking checkout already is the target base, the lane materializes that commit in a detached worktree and re-runs the review from it, so no proposal is trusted to review itself and nothing about a diff's contents is classified. Scoped: the wrapper deciding to hand off is read from the invoking checkout, which the operator is responsible for trusting — the same trust root as before, now stated. `READY_FOR_INTEGRATION` is evidence, never merge authority: final metadata and exact landing-tree review belong to maintainers.
|
||||
scripts/contributor_review_evidence.py ← route-neutral contributor-packet binding: verifies content-addressed prompt/response projections, correlates stable slot ids, records route/model/profile/access/effort/terminal-settlement provenance without presenting requests as observed facts, fails closed on typed execution contradictions while retaining non-identity capability deltas as explicit degradation evidence, and carries full redacted agent-session transcripts into the shareable artifact.
|
||||
scripts/run_plan_review.py ← operator plan-review tool: runs the SAME plan-review engine as `plan_task` (`ouroboros.tools.plan_review`) over an operator envelope — `--goal`, `--plan` (prose file), `--spec-json`, repeatable `--evidence` locators, optional `--subject-root`/`--drive-root`/`--output` — in an isolated drive root; prints the recorded wave (every slot, validated findings, host aggregate) plus the coordinated output, untruncated. Not part of the runtime gate; review-exempt dev tool.
|
||||
scripts/cleanup_test_pollution.py ← Dry-run-first cleanup utility for local test-pollution artifacts: known test skill state dirs, stale `__extension_imports`, and accidental `MagicMock`-named repo-root files. Use `--apply` only after inspecting planned removals.
|
||||
|
|
|
|||
|
|
@ -792,7 +792,10 @@ review machinery is always the target base's own (owner decision, 2026-08-19):
|
|||
unless the invoking checkout already is the target base, the lane materializes
|
||||
that commit in a detached worktree and re-runs the review from it, so a proposal
|
||||
is never trusted to review itself and no per-proposal trust classification
|
||||
remains. The proposal stays the reviewed subject in the frozen checkout.
|
||||
remains. The proposal stays the reviewed subject in the frozen checkout. The
|
||||
guarantee is scoped: the wrapper deciding to hand off is itself read from the
|
||||
invoking checkout, so run it from a trusted one. That trust root is the same as
|
||||
before the change; it is now stated instead of assumed.
|
||||
|
||||
This evidence establishes readiness; it does not authorize commit, push, merge,
|
||||
or publication. Maintainers choose the landing parent and release version,
|
||||
|
|
|
|||
|
|
@ -17,9 +17,10 @@ semantics, performs provider-specific readiness checks where supported, and
|
|||
emits redacted base/head/tree/diff-bound evidence.
|
||||
|
||||
The contributor lane always runs the review machinery of the TARGET BASE (owner
|
||||
decision, 2026-08-19): unless this process already executes from the base
|
||||
commit, it materializes that commit in a detached worktree and re-runs itself
|
||||
there, so a proposal is never trusted to review itself.
|
||||
decision, 2026-08-19): unless already executing from the base commit, it
|
||||
materializes that commit in a detached worktree and re-runs itself there, so the
|
||||
reviewed proposal is never the reviewing code. The handoff itself is read from
|
||||
the invoking checkout: run this wrapper from a trusted one (unchanged trust root).
|
||||
|
||||
Exit codes:
|
||||
0 review passed
|
||||
|
|
@ -539,15 +540,17 @@ def _run_on_trusted_base(args) -> int | None:
|
|||
"""Run the contributor review with the TARGET BASE's own review machinery.
|
||||
|
||||
Owner decision (2026-08-19): a contributor review never runs on the
|
||||
proposal's unverified copy of the review flow, for any PR, whatever it
|
||||
touches — so there is nothing to classify. The one deciding fact is whether
|
||||
the tree this process imports its machinery from IS the target base; when it
|
||||
is not, the base commit is materialized in a detached worktree and this
|
||||
script re-runs from there. The proposal stays the reviewed subject: the
|
||||
trusted run binds the same base/head commits and applies the same patch into
|
||||
its own frozen checkout, where its tests still run as the hermetic preflight
|
||||
intends. Returns the base-side exit code, or ``None`` when this process is
|
||||
already on the trusted base and should continue in place.
|
||||
proposal's unverified copy of the review flow, whatever it touches — so
|
||||
there is nothing to classify. The one deciding fact is whether the tree this
|
||||
process imports its machinery from IS the target base; when it is not, the
|
||||
base is materialized in a detached worktree and this script re-runs there,
|
||||
binding the same base/head commits and applying the same patch into its own
|
||||
frozen checkout, where the proposal's tests still run as the preflight
|
||||
intends. Returns the base-side exit code, or ``None`` when already on base.
|
||||
|
||||
Scope: the handoff removes the dependency on WHICH checkout the operator
|
||||
stood in, not on this wrapper — these lines are read from the invoking
|
||||
checkout, so invoke it from a trusted one (an unchanged, now stated, root).
|
||||
"""
|
||||
base_ref = args.base_ref or _CONTRIBUTOR_DEFAULT_BASE_REF
|
||||
base_sha = _git_text(["rev-parse", f"{base_ref}^{{commit}}"]).strip()
|
||||
|
|
@ -557,22 +560,21 @@ def _run_on_trusted_base(args) -> int | None:
|
|||
_require_clean_worktree()
|
||||
checkout_root, trusted = _create_isolated_checkout("", base_commit=base_sha)
|
||||
try:
|
||||
# Commits, not refs: the trusted run binds the exact same proposal even
|
||||
# if a ref moves while it runs. The data root is passed because the child
|
||||
# would otherwise resolve it from its temporary checkout's parent.
|
||||
# Commits, not refs, so a moving ref cannot re-point the run. Artifact
|
||||
# paths absolutize against the INVOKING cwd and the data root is passed:
|
||||
# the child runs inside the temporary checkout and would resolve both
|
||||
# there, losing them with it. Equals-form keeps a leading "-" a value.
|
||||
command = [
|
||||
sys.executable, str(trusted / "scripts" / "run_external_review.py"),
|
||||
"--contributor", "--base-ref", base_sha, "--head-ref", head_sha,
|
||||
"--goal", args.goal, "--scope", args.scope,
|
||||
*(["--output", args.output] if args.output else []),
|
||||
*(["--drive-root", args.drive_root] if args.drive_root else []),
|
||||
"--contributor", f"--base-ref={base_sha}", f"--head-ref={head_sha}",
|
||||
f"--goal={args.goal}", f"--scope={args.scope}",
|
||||
*([f"--output={os.path.abspath(args.output)}"] if args.output else []),
|
||||
*([f"--drive-root={os.path.abspath(args.drive_root)}"] if args.drive_root else []),
|
||||
"--", args.commit_message,
|
||||
]
|
||||
print(f"Trusted review machinery: base {base_sha[:12]} at {trusted}", file=sys.stderr)
|
||||
code = subprocess.run(
|
||||
command, cwd=str(trusted),
|
||||
env={**os.environ, "OUROBOROS_DATA_DIR": str(DATA)},
|
||||
).returncode
|
||||
env = {**os.environ, "OUROBOROS_DATA_DIR": str(DATA)}
|
||||
code = subprocess.run(command, cwd=str(trusted), env=env).returncode
|
||||
# An abnormal termination is infrastructure, never a reviewer verdict.
|
||||
return code if code in (0, 1, 2, 3) else 3
|
||||
finally:
|
||||
|
|
@ -912,10 +914,12 @@ def _write_contributor_packet(
|
|||
"review_machinery": "target_base_unconditional",
|
||||
"note": (
|
||||
"Unconditional trusted execution (owner decision 2026-08-19): the "
|
||||
"review machinery is always the target base implementation, so no "
|
||||
"proposal reviews itself. The proposal is the reviewed subject and "
|
||||
"its own tests still run in the frozen checkout, as the preflight "
|
||||
"intends. Contributor evidence is not merge authorization or "
|
||||
"review machinery is the target base's, handed off to for every "
|
||||
"proposal alike, so the reviewed code is not the reviewing code. "
|
||||
"Scope: the wrapper performing that handoff is read from the "
|
||||
"invoking checkout, which the operator is responsible for trusting; "
|
||||
"the proposal is the reviewed subject and its own tests still run in "
|
||||
"the frozen checkout. This evidence is not merge authorization or "
|
||||
"cryptographic proof of execution."
|
||||
),
|
||||
},
|
||||
|
|
@ -1044,8 +1048,7 @@ def _parse_args():
|
|||
"Review the committed base-ref..head-ref proposal with the configured "
|
||||
"triad/scope slots, blocking clean semantics, no Claude advisory, "
|
||||
"and a shareable route-aware evidence packet. The review machinery "
|
||||
"always runs from the target base: unless this checkout is already "
|
||||
"on it, the base is materialized and this script re-runs there."
|
||||
"always comes from the target base, materialized when it has to be."
|
||||
),
|
||||
)
|
||||
parser.add_argument(
|
||||
|
|
|
|||
|
|
@ -85,9 +85,11 @@ MIGRATION_HEADERS = ("old path/symbol", "new owner/path", "facade/public contrac
|
|||
# D09 §4.3.2 LLM local retry (one physical attempt) · D11 §1.9/№8 FUNCTION_DEBT same-qualname
|
||||
# relocation rule · D13 §6.4 supervisor/git_ops pre-init roots follow OUROBOROS_* env
|
||||
# (hermetic-isolation incident fix; ratified by owner batch №11, spec §1.12) · D18 §1.9/№8 module-handle
|
||||
# reads of rebound supervisor globals in extracted leaves · D31 §1.14-2 review trust boundary derived
|
||||
# by name classification instead of the hand-list (owner batch №14, answer 2=A; widens the substrate
|
||||
# flag to the 20 review-stack leaves the literal had silently missed) · D33 §1.9/№8-pattern
|
||||
# reads of rebound supervisor globals in extracted leaves · D31 §1.14-2 the contributor review trust
|
||||
# boundary (owner decision 2026-08-19, superseding batch №14 answer 2=A): the per-proposal classifier
|
||||
# — hand-list, then anchors plus name rule plus base-flow import closure — retires whole, because the
|
||||
# contributor lane now hands the review off to the target base's own machinery for every proposal, so
|
||||
# there is nothing left to classify · D33 §1.9/№8-pattern
|
||||
# module-handle reads of monkeypatchable loop facade bindings in the L-B leaves (the ratified
|
||||
# supervisor mechanism applied to the loop stream with its own id per the §1.9-1 "separate delta
|
||||
# id" rule; owner-ratified, batch №17 answer 2=A; leaves hold no mutable state, the handle
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import json
|
|||
import hashlib
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
import zipfile
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
|
|
@ -106,16 +107,55 @@ def test_contributor_review_always_runs_on_the_trusted_base(
|
|||
assert Path(ran["machinery_root"]) != repo
|
||||
assert ran["cwd"] == ran["machinery_root"]
|
||||
assert ran["data_dir"]
|
||||
argv = ran["argv"]
|
||||
assert "--contributor" in argv
|
||||
assert "--contributor" in ran["argv"]
|
||||
# Commits, not refs: a moving ref cannot re-point the trusted run.
|
||||
assert argv[argv.index("--base-ref") + 1] == base_sha
|
||||
assert argv[argv.index("--head-ref") + 1] == head_sha
|
||||
assert argv[-2:] == ["--", "PR title"]
|
||||
options = _forwarded_options(ran["argv"])
|
||||
assert options["base-ref"] == base_sha
|
||||
assert options["head-ref"] == head_sha
|
||||
assert ran["argv"][-2:] == ["--", "PR title"]
|
||||
# The trusted worktree is temporary: it is removed once the review returns.
|
||||
assert not Path(ran["machinery_root"]).exists()
|
||||
|
||||
|
||||
def _forwarded_options(argv: list[str]) -> dict[str, str]:
|
||||
return dict(
|
||||
item[2:].split("=", 1) for item in argv if item.startswith("--") and "=" in item
|
||||
)
|
||||
|
||||
|
||||
def test_the_handoff_forwards_artifact_paths_the_child_can_still_reach(
|
||||
tmp_path, monkeypatch
|
||||
):
|
||||
"""Relative artifact paths must not resolve inside the temporary checkout.
|
||||
|
||||
The child runs with cwd set to the materialized base worktree, which is
|
||||
deleted when the review returns; a verbatim relative --output/--drive-root
|
||||
would put the operator's results there and lose them. They are absolutized
|
||||
against the INVOKING cwd instead. Options travel in equals form so a value
|
||||
starting with "-" reaches the child as a value, not as a broken flag.
|
||||
"""
|
||||
_init_contributor_repo(tmp_path, monkeypatch)
|
||||
probe = _probe_path(monkeypatch, tmp_path)
|
||||
monkeypatch.chdir(tmp_path)
|
||||
|
||||
_run_on_trusted_base(SimpleNamespace(
|
||||
base_ref="base", head_ref="HEAD", commit_message="-title-like-a-flag",
|
||||
goal="--goal-like-a-flag", scope="-s", output="artifacts/run",
|
||||
drive_root="drive",
|
||||
))
|
||||
|
||||
ran = json.loads(probe.read_text(encoding="utf-8"))
|
||||
options = _forwarded_options(ran["argv"])
|
||||
assert options["output"] == str(tmp_path / "artifacts" / "run")
|
||||
assert options["drive-root"] == str(tmp_path / "drive")
|
||||
for key in ("output", "drive-root"):
|
||||
assert not Path(options[key]).is_relative_to(Path(ran["machinery_root"]))
|
||||
# Values that look like flags survive as values.
|
||||
assert options["goal"] == "--goal-like-a-flag"
|
||||
assert options["scope"] == "-s"
|
||||
assert ran["argv"][-2:] == ["--", "-title-like-a-flag"]
|
||||
|
||||
|
||||
def test_contributor_review_invoked_from_the_target_base_runs_in_place(
|
||||
tmp_path, monkeypatch
|
||||
):
|
||||
|
|
@ -132,6 +172,40 @@ def test_contributor_review_invoked_from_the_target_base_runs_in_place(
|
|||
assert not probe.exists()
|
||||
|
||||
|
||||
def test_the_real_wrapper_hands_off_before_it_reviews_anything(tmp_path, monkeypatch):
|
||||
"""End-to-end pin of the main() wiring, not just the helper.
|
||||
|
||||
The REAL script is invoked as a process from a checkout that is not the
|
||||
target base, exactly as a contributor runs it. Reaching any review work
|
||||
without handing off first would leave the base-side probe unexecuted, so
|
||||
deleting the main() hook fails here even while the helper stays perfect.
|
||||
"""
|
||||
repo = _init_contributor_repo(tmp_path, monkeypatch)
|
||||
probe = _probe_path(monkeypatch, tmp_path)
|
||||
wrapper = Path(__file__).resolve().parent.parent / "scripts" / "run_external_review.py"
|
||||
# The base commit keeps the probe; the proposal carries the real wrapper.
|
||||
(repo / "scripts" / "run_external_review.py").write_text(
|
||||
wrapper.read_text(encoding="utf-8"), encoding="utf-8"
|
||||
)
|
||||
_git(repo, "add", "-A")
|
||||
_git(repo, "commit", "-m", "proposal adopts the real wrapper")
|
||||
base_sha = _git(repo, "rev-parse", "base").strip()
|
||||
|
||||
proc = subprocess.run(
|
||||
[
|
||||
sys.executable, str(repo / "scripts" / "run_external_review.py"),
|
||||
"--contributor", "--base-ref=base", "--head-ref=HEAD", "--", "PR title",
|
||||
],
|
||||
cwd=str(repo), capture_output=True, text=True, timeout=300,
|
||||
env={**os.environ, "REVIEW_PROBE_OUT": str(probe)},
|
||||
)
|
||||
|
||||
assert probe.exists(), f"the base-side run never happened: {proc.stderr[-2000:]}"
|
||||
ran = json.loads(probe.read_text(encoding="utf-8"))
|
||||
assert ran["machinery_sha"] == base_sha
|
||||
assert proc.returncode == 1 # the probe's exit code, passed through
|
||||
|
||||
|
||||
def test_contributor_review_refuses_a_dirty_authoring_worktree(tmp_path, monkeypatch):
|
||||
"""The uncommitted half of a proposal must not silently drop out.
|
||||
|
||||
|
|
@ -279,11 +353,21 @@ def _init_contributor_repo(tmp_path: Path, monkeypatch) -> Path:
|
|||
_git(repo, "config", "user.email", "test@example.com")
|
||||
_git(repo, "config", "user.name", "Test")
|
||||
_git(repo, "config", "core.autocrlf", "false")
|
||||
# Same ignores as the real repository: importing from a checkout writes
|
||||
# bytecode into it, which would otherwise read as an unclean worktree.
|
||||
(repo / ".gitignore").write_text("__pycache__/\n*.pyc\n", encoding="utf-8")
|
||||
(repo / "scripts").mkdir()
|
||||
(repo / "scripts" / "run_external_review.py").write_text(
|
||||
_BASE_SIDE_PROBE, encoding="utf-8"
|
||||
)
|
||||
_write_target_config(repo)
|
||||
# A real (not namespace) package, so a wrapper executed out of this repo
|
||||
# imports its module-level dependency from here and not from whatever
|
||||
# ouroboros the host interpreter happens to have installed.
|
||||
(repo / "ouroboros" / "__init__.py").write_text("", encoding="utf-8")
|
||||
(repo / "ouroboros" / "runtime_mode_policy.py").write_text(
|
||||
"GIT_OPS_FAMILY_PATHS = frozenset()\n", encoding="utf-8"
|
||||
)
|
||||
(repo / "ouroboros" / "review_substrate.py").write_text(
|
||||
"# trusted review substrate\n", encoding="utf-8"
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue