From 749c7ab45dd26ce30638a7de592c8f1fc7efefe5 Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Sun, 13 Sep 2026 17:51:28 +0200 Subject: [PATCH] feat(pre-push-safety-net): preserve existing hooks during explicit integration --- .../implementation-group-7.md | 40 +++++++ .kiro/specs/pre-push-safety-net/spec.json | 7 +- .kiro/specs/pre-push-safety-net/tasks.md | 2 +- ops/README.md | 62 ++++++++++ ops/install-pre-push | 25 ++++ ops/pre-push | 17 +++ src/openultrasast/cli.py | 45 ++++++-- src/openultrasast/push/runner.py | 3 +- tests/test_push_hook.py | 107 ++++++++++++++++++ 9 files changed, 294 insertions(+), 14 deletions(-) create mode 100755 ops/install-pre-push create mode 100755 ops/pre-push create mode 100644 tests/test_push_hook.py diff --git a/.kiro/specs/pre-push-safety-net/implementation-group-7.md b/.kiro/specs/pre-push-safety-net/implementation-group-7.md index 7324ee0..16a3e06 100644 --- a/.kiro/specs/pre-push-safety-net/implementation-group-7.md +++ b/.kiro/specs/pre-push-safety-net/implementation-group-7.md @@ -35,3 +35,43 @@ deletions, no work, policy combinations and deadline ownership. - CLAIM: Task 7.1 interface and policy integration implemented. - EVIDENCE: `/tmp/group7-1-full.log`, `/tmp/group7-1-final-focused.log`; ruff, mypy and diff checks exit 0. - GAPS: Representative multi-ref performance and real capability admission remain group 8 work. + +## Task 7.2 brief + +Requirements 1.2, 5.3, 5.4 and 7.4; design: Result and Hook Adapter. +Deliver executable opt-in integration and non-overwriting installation, preserve the +configured hook path, explicitly chain a prior hook with its exact input/arguments, +retain rejection, and document guarded removal. The existing hook's runtime remains +its own behavior; input retention and Safety Net analysis share one deadline. +Verify with disposable repositories and a local bare remote, never this project's hooks. + +## Debug Report +- ROOT_CAUSE: `git rev-parse --path-format=absolute --git-path hooks/pre-push` canonicalizes the final symlink, so even O_EXCL on the returned name creates its target. Initial attribution to shell noclobber was not supported by the subsequent test. +- CATEGORY: LOGIC_ERROR +- FIX_PLAN: Resolve only `hooks`, append the literal `pre-push` basename, and create with O_EXCL/O_NOFOLLOW. +- VERIFICATION: Dangling-symlink regression passes; target remains absent. +- NEXT_ACTION: RETRY_TASK +- CONFIDENCE: HIGH +- NOTES: Git documents absolute output as canonical: https://git-scm.com/docs/git-rev-parse/2.33.0.html . Guarded removal instructions use directory-only resolution too. + +## Status Report +- STATUS: READY_FOR_REVIEW +- TASK: 7.2 +- RED_PHASE_OUTPUT: `/tmp/group7-2-red.log` (missing integration); `/tmp/group7-2-flag-off.log` (two failures with chaining OFF). ON: three passed; switch removed, final 16 hook/interface tests passed. +- TESTS_RUN: full regression 1282 passed, 9 skipped in 119.95s; final 16 focused tests include subsequently added exact-exit and symlink controls. Ruff, formatting, mypy, shell syntax, CLI help and diff checks pass. +- CONCERNS: input-retention failure stops the integration because a prior hook cannot safely receive partial input. Prior hook runtime remains separate from analysis. + +## Review Verdict +- VERDICT: APPROVED +- TASK: 7.2 +- MECHANICAL_RESULTS: actual diff and installation/removal recipes reviewed; disposable remote accepts/rejects correctly; original stdin bytes/arguments and exact nonzero exit preserved; source/index/ref state and core.hooksPath preserved; no placeholders or secrets; task boundary maintained. +- FINDINGS: final-path canonicalization issue repaired and regression tested. Installation is explicit and does not overwrite existing hooks or symlink targets. +- RED phase: VERIFIED. +- SUMMARY: Opt-in integration preserves prior hook behavior and supports guarded restoration. + +## Verification Result +- STATUS: VERIFIED +- CLAIM_TYPE: TASK +- CLAIM: Task 7.2 non-destructive hook integration delivered. +- EVIDENCE: `/tmp/group7-2-full.log`, `/tmp/group7-2-final-focused.log`; static checks and shell/CLI smoke exit 0. +- GAPS: No hook installed in this project's actual repository; rollout remains NO-GO. diff --git a/.kiro/specs/pre-push-safety-net/spec.json b/.kiro/specs/pre-push-safety-net/spec.json index c5ceda0..74cef9f 100644 --- a/.kiro/specs/pre-push-safety-net/spec.json +++ b/.kiro/specs/pre-push-safety-net/spec.json @@ -19,7 +19,7 @@ } }, "ready_for_implementation": true, - "implementation_approval_note": "Group 7 authorized as a whole; 7.1 verified, 7.2\u20137.3 in progress. Default capability registry empty; rollout NO-GO.", + "implementation_approval_note": "Group 7 authorized; 7.1\u20137.2 verified, 7.3 next. Default capability registry empty; rollout NO-GO.", "depends_on": [ "contributor-scan" ], @@ -108,9 +108,10 @@ "6.2", "6.3", "6.4", - "7.1" + "7.1", + "7.2" ], - "remaining_tasks": 6, + "remaining_tasks": 5, "verification_record": "implementation-group-7.md", "verification_records": [ "implementation-group-1.md", diff --git a/.kiro/specs/pre-push-safety-net/tasks.md b/.kiro/specs/pre-push-safety-net/tasks.md index 46e7fa5..a26f226 100644 --- a/.kiro/specs/pre-push-safety-net/tasks.md +++ b/.kiro/specs/pre-push-safety-net/tasks.md @@ -198,7 +198,7 @@ independent capability qualification. Groups 7–8 remain pending; rollout remai - _Boundary: CLI/config, Push runner and Result and Hook Adapter integration_ - _Depends: 2.1, 4.2, 4.3, 6.4_ - _Requirements: 1.1, 1.3, 1.4, 4.1, 5.1, 5.2, 5.3, 5.4, 5.5, 7.1_ -- [ ] 7.2 Provide non-destructive hook integration and removal +- [x] 7.2 Provide non-destructive hook integration and removal - Supply an executable integration snippet/installation path with explicit invocation, preserving existing hooks, hook path configuration, remote arguments and stdin for other consumers. - Refuse automatic overwrite; retain an existing hook's nonzero result even when the Safety Net runs advisory. Include concrete integration/removal instructions with the delivered hook interface. - Done when a disposable Git remote accepts/rejects pushes correctly with an existing stdin-reading hook, and removal restores the preceding behavior without touching source or refs. diff --git a/ops/README.md b/ops/README.md index c242f29..d2ad5db 100644 --- a/ops/README.md +++ b/ops/README.md @@ -282,3 +282,65 @@ is separate from alert eligibility: current admission is always applied. No hook by replay. Artifact timings record discovery, graph and query hits; identical-tip hits do not establish representative changed-code latency. Corrupt, partial or incompatible entries are misses, with remaining work constrained by the same push deadline. + +### Experimental pre-push integration + +The default capability registry is empty pending independent qualification. Installing +this interface does not make the project rollout-ready. Advisory is the default; +blocking and strict incomplete-coverage handling are separate explicit choices. + +Put the installed `ousast` executable on the Git process's PATH. From this tool's +checkout, explicitly install into the desired repository: + +```sh +ops/install-pre-push /absolute/path/to/repository +``` + +The installer resolves Git's effective hook directory (including `core.hooksPath`), +leaves that configuration untouched, and refuses to overwrite any existing hook. +For an existing hook, inspect it and explicitly move it aside **before** installation: + +```sh +hook_dir=$(git -C /absolute/path/to/repository rev-parse --path-format=absolute --git-path hooks) +hook="$hook_dir/pre-push" +test ! -e "$hook.before-ousast" && test ! -L "$hook.before-ousast" && mv -- "$hook" "$hook.before-ousast" +ops/install-pre-push /absolute/path/to/repository +``` + +The wrapper chains that sibling hook with the exact remote arguments and original +stdin bytes, even when analysis blocks. A prior rejection always remains a rejection. +Only an explicitly chained hook executes project-owned hook code; analysis executes +no project code. The existing hook retains its own runtime, outside the safety-net +analysis deadline. Integration input retention is bounded to 1 MiB and the analysis +deadline; failure to retain complete input stops the integration rather than feeding +partial input to another consumer. + +Artifacts default to `$XDG_CACHE_HOME/openultrasast/push` (or `~/.cache/openultrasast/push`). +Set `OUSAST_ARTIFACT_DIR` to an absolute directory outside the repository if needed. +`OUSAST_PUSH_DEADLINE` defaults to 30 seconds. `OUSAST_PUSH_MODE=blocking` opts into +blocking; `OUSAST_INCOMPLETE_COVERAGE=block` additionally blocks incomplete coverage +in that mode. These variables do not enable detector capabilities or model calls. +For a new branch, add an explicit `--comparison-base ` to both command +branches of the installed wrapper if a suitable local base exists. Without it, a new +branch records missing-base coverage rather than silently selecting HEAD or fetching. +Direct stdin integration is `ousast pre-push . --artifact /outside/push.json +--remote "$1" "$2"`; explicit `--base` and `--head` replay remains available. + +To remove, inspect the installed wrapper first. If it is unchanged, this guarded +recipe removes only the supplied wrapper and restores the previous hook: + +```sh +hook_dir=$(git -C /absolute/path/to/repository rev-parse --path-format=absolute --git-path hooks) +hook="$hook_dir/pre-push" +if cmp -s ops/pre-push "$hook"; then + rm -- "$hook" + if test -e "$hook.before-ousast" || test -L "$hook.before-ousast"; then + mv -n -- "$hook.before-ousast" "$hook" + fi +else + printf '%s\n' 'Wrapper differs: inspect and remove your integration changes manually.' +fi +``` + +Neither installation nor removal modifies source, the index, refs, or hook-path +configuration. Keep any prior hook backup until restoration is confirmed. diff --git a/ops/install-pre-push b/ops/install-pre-push new file mode 100755 index 0000000..031c0a0 --- /dev/null +++ b/ops/install-pre-push @@ -0,0 +1,25 @@ +#!/bin/sh +# Explicit installation only. Never edit core.hooksPath or overwrite a hook. +set -eu +repo=${1:-.} +source_dir=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) +hook_dir=$(git -C "$repo" rev-parse --path-format=absolute --git-path hooks) +hook="$hook_dir/pre-push" +mkdir -p -- "$(dirname -- "$hook")" +# Resolve only the directory: Git canonicalizes a final hook symlink to its target. +# O_EXCL refuses every pre-existing final path, including dangling symlinks. +python3 - "$source_dir/pre-push" "$hook" <<'PY' +import os +import shutil +import sys +try: + with open(sys.argv[1], 'rb') as source: + fd = os.open(sys.argv[2], os.O_WRONLY | os.O_CREAT | os.O_EXCL | os.O_NOFOLLOW, 0o755) + with os.fdopen(fd, 'wb') as target: + shutil.copyfileobj(source, target) + os.fchmod(target.fileno(), 0o755) +except OSError: + print('Installation refused: hook exists or cannot be created. See ops/README.md.', file=sys.stderr) + sys.exit(1) +PY +printf 'Installed %s\n' "$hook" diff --git a/ops/pre-push b/ops/pre-push new file mode 100755 index 0000000..098d9d2 --- /dev/null +++ b/ops/pre-push @@ -0,0 +1,17 @@ +#!/bin/sh +# Explicitly copy using the non-overwriting recipe in ops/README.md. +# A prior hook is chained only after you explicitly move it to this sibling path. +artifact_dir=${OUSAST_ARTIFACT_DIR:-${XDG_CACHE_HOME:-$HOME/.cache}/openultrasast/push} +prior="$0.before-ousast" +if [ -e "$prior" ] || [ -L "$prior" ]; then + exec ousast pre-push . --artifact "$artifact_dir/push-$$.json" \ + --deadline "${OUSAST_PUSH_DEADLINE:-30}" \ + --mode "${OUSAST_PUSH_MODE:-advisory}" \ + --incomplete-coverage "${OUSAST_INCOMPLETE_COVERAGE:-allow}" \ + --prior-hook "$prior" --remote "$1" "$2" +fi +exec ousast pre-push . --artifact "$artifact_dir/push-$$.json" \ + --deadline "${OUSAST_PUSH_DEADLINE:-30}" \ + --mode "${OUSAST_PUSH_MODE:-advisory}" \ + --incomplete-coverage "${OUSAST_INCOMPLETE_COVERAGE:-allow}" \ + --remote "$1" "$2" diff --git a/src/openultrasast/cli.py b/src/openultrasast/cli.py index 509658b..b94da3f 100644 --- a/src/openultrasast/cli.py +++ b/src/openultrasast/cli.py @@ -3,7 +3,9 @@ from __future__ import annotations import argparse import json import os +import subprocess import tempfile +import time from collections.abc import Mapping, Sequence from dataclasses import asdict, dataclass, replace from pathlib import Path @@ -117,6 +119,7 @@ def main(argv: list[str] | None = None) -> int: replay_parser.add_argument("--head") replay_parser.add_argument("--remote", nargs=2, metavar=("NAME", "URL")) replay_parser.add_argument("--comparison-base") + replay_parser.add_argument("--prior-hook", type=Path, help="explicitly chain an existing hook with the same input and arguments") replay_parser.add_argument("--artifact", type=Path, required=True) replay_parser.add_argument( "--cache-dir", type=Path, help="reuse compatible local artifacts in a private directory outside the repository" @@ -221,21 +224,45 @@ def main(argv: list[str] | None = None) -> int: incomplete_coverage_policy=args.incomplete_coverage, ) options: dict[str, Any] = dict(artifact=args.artifact, config=settings, max_regions=args.max_regions, cache_dir=args.cache_dir) + if args.base and args.prior_hook: + parser.error("--prior-hook requires Git stdin mode") + prior_data = None if args.base: delivery = replay(args.path, base=args.base, head=args.head, **options) else: # multiprocessing closes sys.stdin in its child; duplicate the Git pipe first. - with os.fdopen(os.dup(0), encoding="utf-8", errors="strict") as stream: - delivery = push( - args.path, - updates=lambda: stream.read(1024 * 1024 + 1), - remote_name=args.remote[0], - remote_url=args.remote[1], - **options, - ) + with os.fdopen(os.dup(0), "rb") as stream: + if args.prior_hook: + from .model.contracts import ExecutionBudget + from .push.runner import _prepare + + shared = ExecutionBudget(time.monotonic() + settings.deadline_seconds, settings.cancellation_allowance_seconds) + try: + prior_data = _prepare(lambda: stream.read(1024 * 1024 + 1), shared) + if len(prior_data) > 1024 * 1024: + raise ValueError("input exceeds hook integration limit") + except (ValueError, RuntimeError, TimeoutError): + print("Hook input could not be retained for all consumers. Retry with complete input and sufficient time.") + return 1 + options["execution_budget"] = shared + + def updates() -> str: + return (prior_data if prior_data is not None else stream.read(1024 * 1024 + 1)).decode("utf-8") + + delivery = push(args.path, updates=updates, remote_name=args.remote[0], remote_url=args.remote[1], **options) except ValueError as error: parser.error(str(error)) - print(delivery.text, end="") + print(delivery.text, end="", flush=True) + if args.prior_hook and prior_data is not None: + # This is the explicitly chained hook's own behavior, outside analysis. + # Never replace its rejection with our advisory success or impose our timeout on it. + try: + prior = subprocess.run([str(args.prior_hook.resolve()), *args.remote], input=prior_data, cwd=args.path) + except OSError: + print("Existing hook could not be started. Restore its executable path before retrying.") + return 1 + if prior.returncode: + return prior.returncode if prior.returncode > 0 else 128 - prior.returncode return delivery.exit_code if args.command == "scan": return _scan(args.path, args.config, args.mode, args.fail_on) diff --git a/src/openultrasast/push/runner.py b/src/openultrasast/push/runner.py index 69a01ef..58e37e5 100644 --- a/src/openultrasast/push/runner.py +++ b/src/openultrasast/push/runner.py @@ -405,6 +405,7 @@ def push( backend: Any = None, max_regions: int = 500, cache_dir: Path | None = None, + execution_budget: ExecutionBudget | None = None, ) -> ReportDelivery: """Consume one Git transaction. Resolution, every comparison and reporting share a deadline.""" config = config or PushConfig() @@ -413,7 +414,7 @@ def push( if cache_dir is not None and cache_dir.resolve().is_relative_to(repository.resolve()): raise ValueError("cache directory must be outside the analyzed repository") started = time.monotonic() - budget = ExecutionBudget(started + config.deadline_seconds, config.cancellation_allowance_seconds) + budget = execution_budget or ExecutionBudget(started + config.deadline_seconds, config.cancellation_allowance_seconds) reports: list[PushReport] = [] analyses: list[ComparisonAnalysis] = [] reasons: list[str] = [] diff --git a/tests/test_push_hook.py b/tests/test_push_hook.py new file mode 100644 index 0000000..6a05075 --- /dev/null +++ b/tests/test_push_hook.py @@ -0,0 +1,107 @@ +"""Real local Git pushes; never install in the project being developed.""" + +import os +import shutil +import subprocess +from pathlib import Path + +import pytest +from test_push_runner import history + + +@pytest.mark.parametrize("prior_exit", [0, 7]) +def test_existing_hook_stdin_remote_exit_and_removal(tmp_path, prior_exit): + root, base, head, git = history(tmp_path) + remote = tmp_path / "remote.git" + subprocess.run(["git", "init", "--bare", str(remote)], check=True, capture_output=True) + git("remote", "add", "origin", str(remote)) + hooks = root / ".custom-hooks" + hooks.mkdir() + git("config", "core.hooksPath", ".custom-hooks") + hook = hooks / "pre-push" + prior = hooks / "pre-push.before-ousast" + log = tmp_path / "prior-input" + args = tmp_path / "prior-args" + prior.write_text(f'#!/bin/sh\nprintf "%s\\n" "$@" > "{args}"\ncat > "{log}"\nexit {prior_exit}\n') + prior.chmod(0o755) + shutil.copyfile(Path(__file__).resolve().parents[1] / "ops/pre-push", hook) + hook.chmod(0o755) + before = (root / ".git/index").read_bytes(), (root / "api.js").read_bytes(), git("rev-parse", "HEAD") + env = { + **os.environ, + "PATH": str(Path(".venv/bin").resolve()) + os.pathsep + os.environ["PATH"], + "OUSAST_ARTIFACT_DIR": str(tmp_path / "artifacts"), + "OUSAST_PUSH_DEADLINE": "0.1", + } + result = subprocess.run(["git", "-C", str(root), "push", "origin", "HEAD:refs/heads/test"], env=env, capture_output=True) + assert (result.returncode == 0) == (prior_exit == 0), result.stderr + assert log.read_text() == f"HEAD {head} refs/heads/test {'0' * 40}\n" + assert args.read_text().splitlines() == ["origin", str(remote)] + assert ((root / ".git/index").read_bytes(), (root / "api.js").read_bytes(), git("rev-parse", "HEAD")) == before + assert git("config", "core.hooksPath") == ".custom-hooks" + assert list((tmp_path / "artifacts").glob("*.json")) + # Explicit removal restores the prior hook byte-for-byte, without config/source/ref edits. + original = prior.read_bytes() + hook.unlink() + prior.rename(hook) + assert hook.read_bytes() == original + result = subprocess.run(["git", "-C", str(root), "push", "origin", "HEAD:refs/heads/after-removal"], env=env, capture_output=True) + assert (result.returncode == 0) == (prior_exit == 0) + assert "refs/heads/after-removal" in log.read_text() + + +def test_install_refuses_existing_and_honors_hooks_path(tmp_path): + root, _, _, git = history(tmp_path) + git("config", "core.hooksPath", ".custom-hooks") + installer = Path(__file__).resolve().parents[1] / "ops/install-pre-push" + first = subprocess.run([str(installer), str(root)], capture_output=True) + assert first.returncode == 0, first.stderr + hook = root / ".custom-hooks/pre-push" + hook.write_text("#!/bin/sh\nexit 23\n") + before = hook.read_bytes() + second = subprocess.run([str(installer), str(root)], capture_output=True) + assert second.returncode != 0 + assert hook.read_bytes() == before + assert git("config", "core.hooksPath") == ".custom-hooks" + + +def test_chain_preserves_exact_nonzero_even_for_advisory(tmp_path): + import sys + + root, _, _, _ = history(tmp_path) + prior = tmp_path / "prior" + prior.write_text("#!/bin/sh\ncat >/dev/null\nexit 23\n") + prior.chmod(0o755) + result = subprocess.run( + [ + sys.executable, + "-m", + "openultrasast.cli", + "pre-push", + str(root), + "--artifact", + str(tmp_path / "result.json"), + "--prior-hook", + str(prior), + "--remote", + "origin", + "/tmp/remote", + ], + input=b"", + capture_output=True, + timeout=10, + ) + assert result.returncode == 23 + + +def test_install_refuses_dangling_symlink(tmp_path): + root, _, _, git = history(tmp_path) + git("config", "core.hooksPath", ".custom-hooks") + hooks = root / ".custom-hooks" + hooks.mkdir() + target = tmp_path / "must-not-create" + (hooks / "pre-push").symlink_to(target) + installer = Path(__file__).resolve().parents[1] / "ops/install-pre-push" + result = subprocess.run([str(installer), str(root)], capture_output=True) + assert result.returncode != 0 + assert not target.exists()