mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
feat(pre-push-safety-net): preserve existing hooks during explicit integration
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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 <local-ref>` 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.
|
||||
|
||||
Executable
+25
@@ -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"
|
||||
Executable
+17
@@ -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"
|
||||
@@ -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)
|
||||
|
||||
@@ -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] = []
|
||||
|
||||
@@ -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()
|
||||
Reference in New Issue
Block a user