diff --git a/CHANGELOG.md b/CHANGELOG.md index faf1470f..c981d796 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,21 @@ ## Unreleased +- **Local verification now evaluates committed and uncommitted edits as one + effective worktree diff.** When a branch change and a review follow-up touch + the same path, `verify` compares the merge base directly with the current + worktree instead of concatenating overlapping diff records. The evaluated + change set is merge-base-relative while the overlay is HEAD-relative, so the + verification plan binds the exact HEAD-relative overlay path set separately — + a path canceled by an uncommitted edit leaves policy evaluation but stays + bound, at its real content, in the receipt. Canceled committed changes are + called out in `base_notes`; worktree diff collection honors repositories that + set `core.fileMode=false` rather than forcing Git's mode reads on, which had + turned every tracked file in such a checkout into a phantom mode change; and a + plan that predates the bound overlay path set fails with an explicit + re-prepare action. + ([#336](https://github.com/ThreeMoonsLab/agents-shipgate/issues/336)) + - **A coding agent can no longer enforce a verifier result the workspace has outgrown.** The reported failure ran forward: a worktree verify returned `human_review_required`, a human committed the reviewed change, a fresh diff --git a/docs/verification-reproducibility.md b/docs/verification-reproducibility.md index 26154ccb..2fb48ad2 100644 --- a/docs/verification-reproducibility.md +++ b/docs/verification-reproducibility.md @@ -69,6 +69,13 @@ Committed snapshots are materialized from Git objects with `git ls-tree` and `export-ignore` and `export-subst` cannot change the evaluated bytes. Symlinks and submodules fail closed for archived verification inputs. +Worktree verification evaluates one merge-base-to-effective-worktree diff, +including staged and unstaged changes, instead of concatenating a committed +range with a HEAD-relative overlay. The request separately binds the exact +HEAD-relative overlay path set and each path's presence, content hash, and Git +file mode. A path changed in both layers therefore has one policy-evaluation +record while the terminal receipt still identifies the complete overlay. + `attempt_id` is diagnostic and deliberately excluded from `receipt_id`. Changing an authoritative input, result, decision, or artifact changes the corresponding content ID. Reusing a base-scan cache does not change the public diff --git a/src/agents_shipgate/cli/verification.py b/src/agents_shipgate/cli/verification.py index 147d9b4e..0360b1fa 100644 --- a/src/agents_shipgate/cli/verification.py +++ b/src/agents_shipgate/cli/verification.py @@ -29,10 +29,12 @@ merge_base_sha, ref_exists, repository_identity, + require_merge_base_sha, resolve_source_head_identity, tree_sha, validate_source_head_identity, working_tree_context, + working_tree_paths, ) from agents_shipgate.config.loader import load_yaml_file from agents_shipgate.core.agent_handoff import build_agent_handoff @@ -54,10 +56,11 @@ build_terminal_receipt, build_unit_result, build_verification_plan, - sha256_file, + plan_worktree_overlay_paths, validate_engine_requirement, validate_plan_inputs, validate_receipt_artifacts, + worktree_overlay, ) from agents_shipgate.packet.json_packet import load_packet_json, write_packet_json from agents_shipgate.report.json_report import report_json_payload @@ -124,11 +127,23 @@ def prepare( ), repository=repository_identity(root), ) - changed, diff_text = ( - diff_context(root, base, head_ref) - if base - else working_tree_context(root, reject_index_hidden=True) - ) + worktree_overlay_paths: list[str] | None = None + if head is not None: + changed, diff_text = diff_context(root, base, head_ref) + elif base: + worktree_overlay_paths = working_tree_paths(root) + effective_base = require_merge_base_sha(root, base, head_ref) + changed, diff_text = working_tree_context( + root, + comparison_ref=effective_base, + reject_index_hidden=True, + ) + else: + worktree_overlay_paths, overlay_diff = working_tree_context( + root, + reject_index_hidden=True, + ) + changed, diff_text = worktree_overlay_paths, overlay_diff resolved_date = evaluation_date or commit_date(root, head_ref) config_relative = _under(root, config).relative_to(root) policy_paths = [_under(root, path) for path in policy_packs or []] @@ -156,6 +171,7 @@ def prepare( ci_mode=ci_mode, no_plugins=no_plugins, no_heuristics=no_heuristics, + worktree_overlay_paths=worktree_overlay_paths, ) except InputParseError as exc: typer.echo(f"Input parsing error: {exc}", err=True) @@ -237,6 +253,7 @@ def _build_plan( ci_mode: str, no_plugins: bool, no_heuristics: bool, + worktree_overlay_paths: list[str] | None = None, ) -> VerificationPlan: """Build the worktree or committed-tree plan for ``prepare``.""" @@ -251,7 +268,11 @@ def _build_plan( input_root=root, policy_pack_paths=policy_paths, plugins_enabled=False if no_plugins else None, - changed_files=changed, + # The overlay set is HEAD-relative and ``changed`` is merge-base- + # relative, so a canceled path appears only in the former. Bind the + # union: an overlay path the snapshot contains but never read is + # reported as absent, which would attest a present file as deleted. + changed_files=sorted({*changed, *(worktree_overlay_paths or [])}), plan_inputs=[ path for path in (baseline_path, diff_from_path, *policy_paths) @@ -276,6 +297,7 @@ def _build_plan( evaluation_date=resolved_date, options=options, plugins_enabled=False if no_plugins else None, + worktree_overlay_paths=worktree_overlay_paths, captured_input_paths=captured, ) source_identity = resolve_source_head_identity(root, head_ref=head_ref) @@ -816,18 +838,14 @@ def _validate_git_subject(plan: VerificationPlan, workspace: Path) -> None: raise ValueError("worktree-overlay plan cannot carry source-head authority") if commit_sha(root, "HEAD") != subject.head_commit_sha: raise ValueError("worker HEAD does not match the worktree-overlay plan") - rows: list[dict[str, Any]] = [] - for relative in plan.inputs.changed_paths: - candidate = (root / relative).resolve() - if candidate != root and root not in candidate.parents: - raise ValueError(f"plan changed path escapes workspace: {relative}") - rows.append( - { - "path": relative, - "status": "present" if candidate.is_file() else "deleted", - "sha256": sha256_file(candidate) if candidate.is_file() else None, - } - ) + overlay_paths = plan_worktree_overlay_paths(plan) + # Recompute through the same builder the plan committed to. #347 added + # `kind` and `executable` to the producer's rows and exposed + # `worktree_overlay` so "the same function builds the overlay a plan + # commits to and the overlay a later reader recomputes", but this reader + # was still hand-rolling the older row shape — so the two normalizations + # disagreed and every non-empty worktree overlay failed here. + rows = worktree_overlay(root, overlay_paths) actual_overlay = content_id(rows) if rows else None if actual_overlay != subject.worktree_overlay_sha256: raise ValueError("worker worktree overlay does not match the plan") diff --git a/src/agents_shipgate/cli/verify/git.py b/src/agents_shipgate/cli/verify/git.py index 2d4903e3..6a9b2c80 100644 --- a/src/agents_shipgate/cli/verify/git.py +++ b/src/agents_shipgate/cli/verify/git.py @@ -197,34 +197,33 @@ class _UnavailableRevisionError(ConfigError): """A revision expression that names refs this checkout does not have.""" -_SAFE_DIFF_CONFIG = [ - "-c", +_SAFE_DIFF_SETTINGS = [ "core.fsmonitor=false", - "-c", "core.autocrlf=false", - "-c", "core.safecrlf=false", - "-c", "core.eol=lf", - "-c", "core.bigFileThreshold=32m", - "-c", "core.fileMode=false", - "-c", "core.precomposeUnicode=false", - "-c", "submodule.recurse=false", - "-c", "core.quotePath=false", - "-c", f"core.attributesFile={os.devnull}", - "-c", f"diff.orderFile={os.devnull}", - "-c", "diff.suppressBlankEmpty=false", - "-c", "diff.renameLimit=32767", ] +_SAFE_DIFF_CONFIG = [ + argument for setting in _SAFE_DIFF_SETTINGS for argument in ("-c", setting) +] +# Worktree comparisons honor the repository's effective core.fileMode setting. +# Git writes it as false on filesystems that cannot preserve executable bits; +# forcing true there turns ordinary checkouts into whole-repository mode diffs. +_SAFE_WORKTREE_DIFF_CONFIG = [ + argument + for setting in _SAFE_DIFF_SETTINGS + if not setting.casefold().startswith("core.filemode=") + for argument in ("-c", setting) +] _DETERMINISTIC_DIFF_OPTIONS = [ "--no-ext-diff", "--no-textconv", @@ -516,6 +515,59 @@ def merge_base_sha(workspace: Path, base: str, head: str) -> str | None: return result.stdout.strip() or None +def require_merge_base_sha(workspace: Path, base: str, head: str) -> str: + """Resolve one merge base or raise a typed diff-input failure. + + Worktree verification needs only this ancestry identity before comparing + the merge-base tree directly with the effective worktree. Reading the + intermediate committed diff would make canceled or superseded branch + content an accidental input again. + """ + + base_commit = commit_sha(workspace, base) + head_commit = commit_sha(workspace, head) + if base_commit is None or head_commit is None: + missing = base if base_commit is None else head + raise DiffInputError( + DiffContext( + completeness="unavailable", + reason="refs_missing", + detail=f"Git ref {missing!r} is not available locally.", + ) + ) + result = _run_git( + workspace, + ["merge-base", "--", base_commit, head_commit], + check=False, + ) + resolved = result.stdout.strip() + if result.returncode == 0 and _GIT_OBJECT_RE.fullmatch(resolved): + return resolved + detail = _redact_local_paths(result.stderr.strip(), workspace) + if result.returncode == 1: + truncated = _history_is_truncated(workspace) + reason: DiffInputReason = ( + "merge_base_missing" + if truncated is True + else "unrelated_histories" + if truncated is False + else "git_failed" + ) + else: + reason, detail = _classify_diff_failure( + _BoundedGitResult(payload=None, stderr=detail), + limit_reason="metadata_limit_exceeded", + workspace=workspace, + ) + raise DiffInputError( + DiffContext( + completeness="unavailable", + reason=reason, + detail=detail or "Git could not resolve one merge base.", + ) + ) + + def commit_date(workspace: Path, ref: str) -> str: commit = commit_sha(workspace, ref) if commit is None: @@ -753,13 +805,15 @@ def _reject_binary_capability_paths( revspec: str, *, pathspec: list[str] | None = None, + diff_config: list[str] | None = None, ) -> None: """Fail closed when a source-like path is hidden behind a binary marker.""" + effective_config = _SAFE_DIFF_CONFIG if diff_config is None else diff_config result = _run_git_bounded_result( workspace, [ - *_SAFE_DIFF_CONFIG, + *effective_config, "diff", *_DETERMINISTIC_DIFF_OPTIONS, "--no-renames", @@ -1092,28 +1146,142 @@ def removes_a_yaml_file(workspace: Path, base: str | None, head: str) -> bool | def working_tree_context( workspace: Path, *, + comparison_ref: str = "HEAD", exclude: Path | None = None, reject_index_hidden: bool = False, ) -> tuple[list[str], str]: - """Return uncommitted changed paths and tracked-file diff text. + """Return effective changed paths and tracked-file diff text. + + ``git diff `` compares one committed tree with the current + index/worktree, so staged and unstaged tracked changes are represented in + one record per effective path. Untracked file paths are included for + trigger/check context, but their contents are intentionally not read into + the diff body. + + The verifier passes the merge-base commit as ``comparison_ref`` when a + worktree sits on top of committed branch changes. This is deliberately one + comparison, rather than concatenating ``base...HEAD`` and ``HEAD`` diffs: + concatenation makes a valid overlapping edit look structurally ambiguous. + """ - ``git diff HEAD`` includes staged and unstaged tracked changes. Untracked - file paths are included for trigger/check context, but their contents are - intentionally not read into the diff body. + _reject_unbound_diff_configuration(workspace) + _reject_executable_worktree_filters(workspace) + comparison_commit = commit_sha(workspace, comparison_ref) + if comparison_commit is None: + raise DiffInputError( + DiffContext( + completeness="unavailable", + reason="refs_missing", + detail=f"Git ref {comparison_ref!r} is not available locally.", + ) + ) + pathspec = _worktree_pathspec(workspace, exclude) + if reject_index_hidden: + _reject_index_hidden_capability_paths(workspace, pathspec=pathspec) + paths = _working_tree_paths( + workspace, + comparison_commit=comparison_commit, + pathspec=pathspec, + ) + body = _run_git_bounded_result( + workspace, + [ + *_SAFE_WORKTREE_DIFF_CONFIG, + "diff", + *_DETERMINISTIC_DIFF_OPTIONS, + comparison_commit, + "--", + *pathspec, + ], + max_output_bytes=_DIFF_BODY_LIMIT, + ) + if body.payload is None: + reason, detail = _classify_diff_failure( + body, limit_reason="body_limit_exceeded", workspace=workspace + ) + raise DiffInputError( + DiffContext( + changed_files=tuple(paths), + completeness="partial", + reason=reason, + detail=detail, + ) + ) + diff_text = _decode_diff_body(body.payload) + try: + _reject_binary_capability_paths( + workspace, + comparison_commit, + pathspec=pathspec, + diff_config=_SAFE_WORKTREE_DIFF_CONFIG, + ) + except BinaryCapabilityDiffError as exc: + exc.changed_paths = tuple(paths) + exc.diff_text = diff_text + raise + except DiffInputError as exc: + # The binary-hiding guard could not run, so the body is not proven to + # cover every capability path. Carry what was read: a caller that can + # act on partial evidence should not have to re-collect it. + raise DiffInputError( + DiffContext( + changed_files=tuple(paths), + diff_text=diff_text, + completeness="partial", + reason=exc.context.reason, + detail=exc.context.detail, + ) + ) from exc + return paths, diff_text + + +def working_tree_paths( + workspace: Path, + *, + comparison_ref: str = "HEAD", + exclude: Path | None = None, + reject_index_hidden: bool = False, +) -> list[str]: + """Return the bounded path inventory for one worktree comparison. + + This metadata-only form is used to bind the HEAD-relative overlay identity + independently from the merge-base-relative diff evaluated by policy. """ _reject_unbound_diff_configuration(workspace) _reject_executable_worktree_filters(workspace) + comparison_commit = commit_sha(workspace, comparison_ref) + if comparison_commit is None: + raise DiffInputError( + DiffContext( + completeness="unavailable", + reason="refs_missing", + detail=f"Git ref {comparison_ref!r} is not available locally.", + ) + ) pathspec = _worktree_pathspec(workspace, exclude) if reject_index_hidden: _reject_index_hidden_capability_paths(workspace, pathspec=pathspec) + return _working_tree_paths( + workspace, + comparison_commit=comparison_commit, + pathspec=pathspec, + ) + + +def _working_tree_paths( + workspace: Path, + *, + comparison_commit: str, + pathspec: list[str], +) -> list[str]: names = _run_git_bounded_result( workspace, [ - *_SAFE_DIFF_CONFIG, + *_SAFE_WORKTREE_DIFF_CONFIG, "diff", *_DETERMINISTIC_DIFF_OPTIONS, - "HEAD", + comparison_commit, "--name-status", "-z", "--", @@ -1156,51 +1324,7 @@ def working_tree_context( path = os.fsdecode(raw_path) if path not in paths: paths.append(path) - body = _run_git_bounded_result( - workspace, - [ - *_SAFE_DIFF_CONFIG, - "diff", - *_DETERMINISTIC_DIFF_OPTIONS, - "HEAD", - "--", - *pathspec, - ], - max_output_bytes=_DIFF_BODY_LIMIT, - ) - if body.payload is None: - reason, detail = _classify_diff_failure( - body, limit_reason="body_limit_exceeded", workspace=workspace - ) - raise DiffInputError( - DiffContext( - changed_files=tuple(paths), - completeness="partial", - reason=reason, - detail=detail, - ) - ) - diff_text = _decode_diff_body(body.payload) - try: - _reject_binary_capability_paths(workspace, "HEAD", pathspec=pathspec) - except BinaryCapabilityDiffError as exc: - exc.changed_paths = tuple(paths) - exc.diff_text = diff_text - raise - except DiffInputError as exc: - # The binary-hiding guard could not run, so the body is not proven to - # cover every capability path. Carry what was read: a caller that can - # act on partial evidence should not have to re-collect it. - raise DiffInputError( - DiffContext( - changed_files=tuple(paths), - diff_text=diff_text, - completeness="partial", - reason=exc.context.reason, - detail=exc.context.detail, - ) - ) from exc - return paths, diff_text + return sorted(paths) def _worktree_pathspec(workspace: Path, exclude: Path | None) -> list[str]: @@ -1989,6 +2113,7 @@ def staged_paths_under(workspace: Path, subdir: str) -> list[str]: "merge_base_sha", "read_file_at_ref", "repository_identity", + "require_merge_base_sha", "resolve_tree_path_identity", "resolve_git_push_endpoint", "resolve_source_head_identity", @@ -1998,4 +2123,5 @@ def staged_paths_under(workspace: Path, subdir: str) -> list[str]: "tree_sha", "validate_source_head_identity", "working_tree_context", + "working_tree_paths", ] diff --git a/src/agents_shipgate/cli/verify/orchestrator.py b/src/agents_shipgate/cli/verify/orchestrator.py index 84749ebf..ea4c5d40 100644 --- a/src/agents_shipgate/cli/verify/orchestrator.py +++ b/src/agents_shipgate/cli/verify/orchestrator.py @@ -137,10 +137,12 @@ ref_exists, removes_a_yaml_file, repository_identity, + require_merge_base_sha, resolve_source_head_identity, resolve_tree_path_identity, tree_sha, working_tree_context, + working_tree_paths, ) HEAD_FORMATS = ["markdown", "json", "sarif"] @@ -370,6 +372,7 @@ def run_verify( return verifier, None, 2 changed_files: list[str] = [] + worktree_overlay_paths: list[str] = [] diff_text = "" base_status: VerifierBaseStatus = "not_requested" base_tree: str | None = None @@ -482,24 +485,43 @@ def run_verify( ) base_exists = False + effective_worktree_ref = head + committed_diff_complete = False if base: base_exists = ref_exists(git_root, base) if base_exists: - collected = _collect_diff(git_root, base, head) - changed_files = list(collected.changed_files) - diff_text = collected.diff_text - if collected.completeness != "complete": - # The refs resolved, so the shortfall is about history depth, - # object availability, or Git itself — each of which has a - # different repair. Report which one instead of a single - # "could not be read". - diff_unavailable = True - base_status = "archive_failed" - diff_failures.append((collected, f"{base}...{head}")) - base_notes.append( - f"Could not collect the {base}...{head} diff in full. " - f"{collected.note}" - ) + if archive_head: + collected = _collect_diff(git_root, base, head) + changed_files = list(collected.changed_files) + diff_text = collected.diff_text + if collected.completeness != "complete": + # The refs resolved, so the shortfall is about history depth, + # object availability, or Git itself — each of which has a + # different repair. Report which one instead of a single + # "could not be read". + diff_unavailable = True + base_status = "archive_failed" + diff_failures.append((collected, f"{base}...{head}")) + base_notes.append( + f"Could not collect the {base}...{head} diff in full. " + f"{collected.note}" + ) + else: + try: + effective_worktree_ref = require_merge_base_sha( + git_root, + base, + head, + ) + committed_diff_complete = True + except DiffInputError as exc: + diff_unavailable = True + base_status = "archive_failed" + diff_failures.append((exc.context, f"{base}...{head}")) + base_notes.append( + "Could not resolve the merge base needed for one " + f"effective-head diff. {exc.context.note}" + ) else: diff_unavailable = True base_status = "ref_missing" @@ -522,11 +544,44 @@ def run_verify( try: worktree_paths, worktree_diff = working_tree_context( git_root, + comparison_ref=effective_worktree_ref, exclude=out_dir, reject_index_hidden=True, ) - changed_files = _dedupe_paths([*changed_files, *worktree_paths]) - diff_text = _join_diff_text(diff_text, worktree_diff) + if committed_diff_complete: + # This is the single merge-base-to-effective-worktree diff. + # Do not append the committed range: overlapping paths must be + # represented exactly once at their effective content. + changed_files = worktree_paths + diff_text = worktree_diff + else: + # The committed side was unavailable or partial. Preserve all + # evidence collected before the mandatory fail-closed exit. + changed_files = _dedupe_paths([*changed_files, *worktree_paths]) + diff_text = _join_diff_text(diff_text, worktree_diff) + head_commit = commit_sha(git_root, head) + worktree_overlay_paths = ( + worktree_paths + if head_commit == commit_sha(git_root, effective_worktree_ref) + else working_tree_paths( + git_root, + comparison_ref=head, + exclude=out_dir, + reject_index_hidden=True, + ) + ) + if committed_diff_complete: + cancelled_committed_paths = sorted( + set(worktree_overlay_paths) - set(worktree_paths) + ) + if cancelled_committed_paths: + count = len(cancelled_committed_paths) + noun = "change" if count == 1 else "changes" + verb = "is" if count == 1 else "are" + base_notes.append( + f"{count} committed {noun} {verb} canceled by uncommitted " + "worktree edits; the committed branch has not been verified." + ) changed_files = _bind_worktree_config_to_head( git_root=git_root, head=head, @@ -534,6 +589,13 @@ def run_verify( worktree_text=worktree_manifest_text, changed_files=changed_files, ) + worktree_overlay_paths = _bind_worktree_config_to_head( + git_root=git_root, + head=head, + config_relative=config_relative, + worktree_text=worktree_manifest_text, + changed_files=worktree_overlay_paths, + ) except Exception as exc: # noqa: BLE001 - local context degrades only. diff_unavailable = True worktree_failure = _as_diff_context(exc) @@ -719,10 +781,14 @@ def capture_capability_lock(lock: CapabilityLockFileV1) -> None: config_path, worktree_manifest_text.encode("utf-8"), ) + # The overlay set is HEAD-relative and the change set is merge-base- + # relative, so a canceled path appears only in the former. Bind the + # union: an overlay path the snapshot contains but never read is + # reported as absent, which would attest a present file as deleted. _bind_changed_files( static_snapshot, root=git_root, - relative_paths=changed_files, + relative_paths=_dedupe_paths([*changed_files, *worktree_overlay_paths]), ) static_snapshot_token = activate_static_input_snapshot(static_snapshot) @@ -994,6 +1060,7 @@ def capture_capability_lock(lock: CapabilityLockFileV1) -> None: os.getenv("EVENT_NAME") or os.getenv("GITHUB_EVENT_NAME") or None ), }, + worktree_overlay_paths=worktree_overlay_paths, evaluation_date=verification_date, ) except Exception: @@ -2498,6 +2565,7 @@ def _write_artifacts( diff_from_path: Path | None = None, authorization_path: Path | None = None, verification_options: dict[str, Any] | None = None, + worktree_overlay_paths: list[str] | None = None, evaluation_date: str | None = None, ) -> None: verifier_path.parent.mkdir(parents=True, exist_ok=True) @@ -2723,6 +2791,7 @@ def _finalize(snapshot: StaticInputSnapshot | None) -> None: **resolved_options, }, plugins_enabled=plugins_enabled, + worktree_overlay_paths=worktree_overlay_paths, external_input_root=external_input_root, captured_input_paths=captured_input_paths, ) diff --git a/src/agents_shipgate/core/agent_boundary.py b/src/agents_shipgate/core/agent_boundary.py index d4234819..e6a4a630 100644 --- a/src/agents_shipgate/core/agent_boundary.py +++ b/src/agents_shipgate/core/agent_boundary.py @@ -1294,6 +1294,7 @@ def _structural_diff_issues( ) -> list[BoundaryInputIssue]: issues: list[BoundaryInputIssue] = [] seen_paths: set[str] = set() + duplicate_paths_seen: set[str] = set() if diff_text.strip() and not diff_files: issues.append( BoundaryInputIssue( @@ -1309,17 +1310,7 @@ def _structural_diff_issues( duplicate_paths = sorted(record_paths.intersection(seen_paths)) seen_paths.update(record_paths) if duplicate_paths: - issues.extend( - BoundaryInputIssue( - code="boundary_diff_shape_invalid", - path=path, - message=( - "The supplied diff contains more than one file record " - "for the same path; one coherent record per path is required." - ), - ) - for path in duplicate_paths - ) + duplicate_paths_seen.update(duplicate_paths) continue shape_errors = [ *( @@ -1403,6 +1394,20 @@ def _structural_diff_issues( ), ) ) + if duplicate_paths_seen: + shown = ", ".join(sorted(duplicate_paths_seen)[:3]) + suffix = "" if len(duplicate_paths_seen) <= 3 else ", …" + issues.append( + BoundaryInputIssue( + code="boundary_diff_shape_invalid", + path="", + message=( + "The supplied diff contains multiple file records for " + f"{len(duplicate_paths_seen)} path(s) ({shown}{suffix}); " + "one coherent record per path is required." + ), + ) + ) return issues diff --git a/src/agents_shipgate/core/current_control.py b/src/agents_shipgate/core/current_control.py index bc769a41..6dba8bca 100644 --- a/src/agents_shipgate/core/current_control.py +++ b/src/agents_shipgate/core/current_control.py @@ -39,6 +39,7 @@ from agents_shipgate.core.errors import AgentsShipgateError from agents_shipgate.core.verification_identity import ( + plan_worktree_overlay_paths, read_regular_file_beneath, worktree_overlay, ) @@ -650,7 +651,10 @@ def _validate_worktree_currency( label="current control plan", ) plan = VerificationPlan.model_validate_json(data) - decided_paths = list(plan.inputs.changed_paths) + # Not `inputs.changed_paths`: since #336 that is the merge-base- + # relative evaluated set, while the overlay this pointer was + # published against is HEAD-relative and recorded separately. + decided_paths = plan_worktree_overlay_paths(plan) rows = worktree_overlay(live.root, decided_paths) except (ValueError, OSError) as exc: raise CurrentControlUnavailable( diff --git a/src/agents_shipgate/core/static_inputs.py b/src/agents_shipgate/core/static_inputs.py index 87bd8b3f..f0258611 100644 --- a/src/agents_shipgate/core/static_inputs.py +++ b/src/agents_shipgate/core/static_inputs.py @@ -191,6 +191,9 @@ def _record(self, key: Path, data: bytes) -> None: "static input snapshot exceeds the " f"{self.max_total_bytes}-byte aggregate limit" ) + metadata = key.lstat() + if not stat.S_ISREG(metadata.st_mode): + raise ValueError(f"static input is not a regular file: {key}") self._entries[key] = data self._total_bytes = next_total diff --git a/src/agents_shipgate/core/verification_identity.py b/src/agents_shipgate/core/verification_identity.py index b1b99e99..2b17e27c 100644 --- a/src/agents_shipgate/core/verification_identity.py +++ b/src/agents_shipgate/core/verification_identity.py @@ -184,6 +184,7 @@ def build_verification_plan( evaluation_date: str, options: dict[str, Any], plugins_enabled: bool | None, + worktree_overlay_paths: list[str] | None = None, diff_logical_path: str = "verification-input.diff", external_input_root: Path | None = None, captured_input_paths: list[Path] | None = None, @@ -191,7 +192,17 @@ def build_verification_plan( effective_plugins_enabled = _plugins_enabled(plugins_enabled) normalized_options = dict(options) normalized_options["plugins_enabled"] = effective_plugins_enabled - overlay = _worktree_overlay(git_root, changed_files) if not archived_head else [] + overlay_paths = sorted( + set(changed_files if worktree_overlay_paths is None else worktree_overlay_paths) + ) + if not archived_head: + # The evaluated change set is merge-base-relative, while the overlay is + # HEAD-relative. Keeping the latter path set inside the content- + # addressed inputs makes cancellations and overlapping edits + # reproducible without polluting policy evaluation with non-effective + # paths. + normalized_options["worktree_overlay_paths"] = overlay_paths + overlay = _worktree_overlay(git_root, overlay_paths) if not archived_head else [] overlay_hash = content_id(overlay) if overlay else None if not archived_head and source_head_commit_sha is not None: raise ValueError("worktree-overlay plans cannot declare a source head commit") @@ -979,6 +990,30 @@ def _existing_changed_blobs(paths: list[str], *, root: Path, source: str) -> lis return _blobs(candidates, root=root, source=source) +def plan_worktree_overlay_paths(plan: VerificationPlan) -> list[str]: + """Return the HEAD-relative overlay path set a worktree plan committed to. + + Since #336 this is *not* ``inputs.changed_paths``: the evaluated change set + is merge-base-relative while the overlay is HEAD-relative, so a path a + worktree edit cancels appears only here. Every recomputation of + ``worktree_overlay_sha256`` — the worker, the current-control reader — must + take the set from one place, or it recomputes a different overlay than the + plan committed to and reports drift that never happened. + """ + + declared = plan.inputs.options.get("worktree_overlay_paths") + if declared is None: + raise ValueError( + "worktree-overlay plan predates overlay path binding; " + "re-run `agents-shipgate verification prepare`" + ) + if not isinstance(declared, list) or not all(isinstance(path, str) for path in declared): + raise ValueError("plan worktree_overlay_paths must be a string list") + if declared != sorted(set(declared)): + raise ValueError("plan worktree_overlay_paths must be sorted and unique") + return declared + + def worktree_overlay(root: Path, paths: list[str]) -> list[dict[str, Any]]: """Return the normalized rows a worktree decision commits to. @@ -1007,6 +1042,9 @@ def _worktree_overlay(root: Path, paths: list[str]) -> list[dict[str, Any]]: candidate = lexical.resolve() if root_resolved not in candidate.parents: raise ValueError(f"worktree path escapes repository: {relative}") + # ``_overlay_entry`` (#347) supersedes this branch's ``git_mode``: it + # binds the executable bit as well, and additionally records ``kind`` + # and hashes a symlink's target rather than following it. rows.append({"path": relative, **_overlay_entry(lexical, candidate, snapshot)}) return rows @@ -1267,6 +1305,7 @@ def _normalized_distribution_name(value: str) -> str: "build_engine_requirement", "read_regular_file_beneath", "worktree_overlay", + "plan_worktree_overlay_paths", "build_executor", "build_terminal_receipt", "build_unit_result", diff --git a/tests/test_adapter_static_only.py b/tests/test_adapter_static_only.py index 1c71ded1..d8467f2e 100644 --- a/tests/test_adapter_static_only.py +++ b/tests/test_adapter_static_only.py @@ -257,7 +257,7 @@ class AllowedException: AllowedException( relative_path="cli/verify/git.py", surface="attr_call:subprocess.Popen", - line=1682, + line=1806, snippet=( "subprocess.Popen(cmd, env=env, stderr=subprocess.PIPE, " "stdin=subprocess.PIPE if input is not None else " @@ -276,7 +276,7 @@ class AllowedException: AllowedException( relative_path="cli/verify/git.py", surface="attr_call:subprocess.run", - line=1926, + line=2050, snippet=( "subprocess.run(cmd, capture_output=capture_output, check=check, " "env=env, input=input, stderr=stderr, stdin=stdin, stdout=stdout, " diff --git a/tests/test_agent_boundary.py b/tests/test_agent_boundary.py index 6de3728e..51d274dd 100644 --- a/tests/test_agent_boundary.py +++ b/tests/test_agent_boundary.py @@ -192,6 +192,37 @@ def test_contradictory_new_file_headers_fail_closed( ) +def test_duplicate_diff_records_are_aggregated_as_one_structural_root_cause( + tmp_path: Path, +) -> None: + diff = "".join( + [ + _change_diff("AGENTS.md", "base", "first"), + _change_diff("AGENTS.md", "first", "second"), + _change_diff(".codex/config.toml", "model = 'a'", "model = 'b'"), + _change_diff(".codex/config.toml", "model = 'b'", "model = 'c'"), + ] + ) + + result = _build(tmp_path, diff) + + structural = [ + item + for item in result.violated_rules + if item.evidence.get("kind") == "boundary_input_unresolved" + and item.evidence.get("code") == "boundary_diff_shape_invalid" + ] + assert len(structural) == 1 + assert structural[0].path == "" + structural_diagnostics = [ + item + for item in result.diagnostics + if item.code == "boundary_diff_shape_invalid" + ] + assert len(structural_diagnostics) == 1 + assert "2 path(s)" in structural_diagnostics[0].message + + def test_safe_untracked_boundary_file_is_read_and_evaluated(tmp_path: Path) -> None: _init_repo(tmp_path) target = tmp_path / ".claude" / "settings.json" diff --git a/tests/test_verification_git_snapshot.py b/tests/test_verification_git_snapshot.py index f676e9d1..a515baee 100644 --- a/tests/test_verification_git_snapshot.py +++ b/tests/test_verification_git_snapshot.py @@ -10,6 +10,7 @@ archive_tree, diff_revspec_context, repository_identity, + working_tree_context, ) from agents_shipgate.core.errors import ConfigError @@ -118,3 +119,125 @@ def test_repository_identity_normalizes_ssh_and_https_remotes(tmp_path: Path) -> "https://token@example.test/org/repo.git?credential=secret", ) assert repository_identity(root) == "example.test/org/repo" + + +def test_effective_worktree_diff_coalesces_committed_staged_and_unstaged_edits( + tmp_path: Path, +) -> None: + root = _repo(tmp_path) + target = root / "AGENTS.md" + target.write_text("base\n", encoding="utf-8") + _git(root, "add", "AGENTS.md") + _git(root, "commit", "-m", "base") + + target.write_text("committed\n", encoding="utf-8") + _git(root, "add", "AGENTS.md") + _git(root, "commit", "-m", "issue fix") + target.write_text("staged review\n", encoding="utf-8") + _git(root, "add", "AGENTS.md") + target.write_text("unstaged review\n", encoding="utf-8") + + changed, diff_text = working_tree_context( + root, + comparison_ref="HEAD~1", + reject_index_hidden=True, + ) + + assert changed == ["AGENTS.md"] + assert diff_text.count("diff --git a/AGENTS.md b/AGENTS.md") == 1 + assert "unstaged review" in diff_text + assert "committed" not in diff_text + assert "+staged review" not in diff_text + + +def test_effective_worktree_diff_preserves_rename_and_mode_semantics( + tmp_path: Path, +) -> None: + root = _repo(tmp_path) + source = root / "AGENTS.md" + source.write_text( + "shared rule\nshared scope\nbase instructions\n", + encoding="utf-8", + ) + _git(root, "add", "AGENTS.md") + _git(root, "commit", "-m", "base") + + destination = root / "CLAUDE.md" + source.rename(destination) + _git(root, "add", "-A") + _git(root, "commit", "-m", "rename instructions") + destination.write_text( + "shared rule\nshared scope\nreviewed instructions\n", + encoding="utf-8", + ) + destination.chmod(0o755) + + changed, diff_text = working_tree_context( + root, + comparison_ref="HEAD~1", + reject_index_hidden=True, + ) + + assert changed == ["AGENTS.md", "CLAUDE.md"] + assert diff_text.count("diff --git ") == 1 + assert "rename from AGENTS.md" in diff_text + assert "rename to CLAUDE.md" in diff_text + assert "old mode 100644" in diff_text + assert "new mode 100755" in diff_text + + +def test_effective_worktree_diff_honors_repository_filemode_false( + tmp_path: Path, +) -> None: + root = _repo(tmp_path) + target = root / "lib" / "util.py" + target.parent.mkdir() + target.write_text("base\n", encoding="utf-8") + manifest = root / "shipgate.yaml" + manifest.write_text("version: '0.1'\n", encoding="utf-8") + _git(root, "add", ".") + _git(root, "commit", "-m", "base") + + _git(root, "config", "core.fileMode", "false") + target.write_text("committed change\n", encoding="utf-8") + _git(root, "add", "lib/util.py") + _git(root, "commit", "-m", "change utility") + target.chmod(0o755) + manifest.chmod(0o755) + + changed, diff_text = working_tree_context( + root, + comparison_ref="HEAD~1", + reject_index_hidden=True, + ) + + assert changed == ["lib/util.py"] + assert "shipgate.yaml" not in diff_text + assert "old mode 100644" not in diff_text + assert "new mode 100755" not in diff_text + + +def test_effective_worktree_diff_retains_merge_base_relative_untracked_paths( + tmp_path: Path, +) -> None: + root = _repo(tmp_path) + tracked = root / "agent.py" + tracked.write_text("base\n", encoding="utf-8") + _git(root, "add", "agent.py") + _git(root, "commit", "-m", "base") + + tracked.write_text("committed\n", encoding="utf-8") + _git(root, "add", "agent.py") + _git(root, "commit", "-m", "change agent") + untracked = root / "new_agent.py" + untracked.write_text("untracked capability\n", encoding="utf-8") + + changed, diff_text = working_tree_context( + root, + comparison_ref="HEAD~1", + reject_index_hidden=True, + ) + + assert changed == ["agent.py", "new_agent.py"] + assert "diff --git a/agent.py b/agent.py" in diff_text + assert "new_agent.py" not in diff_text diff --git a/tests/test_verify_orchestrator.py b/tests/test_verify_orchestrator.py index 86cc2f01..3b278c94 100644 --- a/tests/test_verify_orchestrator.py +++ b/tests/test_verify_orchestrator.py @@ -10,7 +10,7 @@ import pytest from agents_shipgate.cli.scan import writing as scan_writing -from agents_shipgate.cli.verification import assemble, worker +from agents_shipgate.cli.verification import _validate_git_subject, assemble, worker from agents_shipgate.cli.verify import orchestrator as verify_orchestrator from agents_shipgate.cli.verify.git import commit_date from agents_shipgate.cli.verify.orchestrator import run_verify @@ -563,6 +563,252 @@ def test_verify_threads_uncommitted_worktree_files_into_head_scan(tmp_path): ) +def test_verify_normalizes_overlapping_commit_and_worktree_into_one_diff_record( + tmp_path: Path, +) -> None: + """Issue #336: review follow-ups may overlap the committed issue fix.""" + + repo = tmp_path / "repo" + sample_dst = repo / "samples" / "support_refund_agent" + sample_dst.parent.mkdir(parents=True) + shutil.copytree(REPO_ROOT / "samples" / "support_refund_agent", sample_dst) + target = repo / "AGENTS.md" + target.write_text("base instructions\n", encoding="utf-8") + + _git(repo, "init") + _git(repo, "config", "user.email", "test@example.test") + _git(repo, "config", "user.name", "Test User") + _git(repo, "add", ".") + _git(repo, "commit", "-m", "base") + + target.write_text("committed issue fix\n", encoding="utf-8") + _git(repo, "add", "AGENTS.md") + _git(repo, "commit", "-m", "issue fix") + target.write_text("uncommitted review follow-up\n", encoding="utf-8") + + out_dir = repo / "agents-shipgate-reports" + verifier, report, _exit_code = run_verify( + workspace=repo, + config=Path("samples/support_refund_agent/shipgate.yaml"), + base="HEAD~1", + head="HEAD", + archive_head=False, + out=out_dir, + ci_mode="advisory", + fail_on=None, + baseline=None, + baseline_mode="new-findings", + diff_from=None, + policy_packs=None, + plugins_enabled=False, + strict_plugins=False, + suggest_patches=False, + no_heuristics=False, + verbose=False, + ) + + assert report is not None + assert not any( + finding.check_id == "SHIP-AGENT-BOUNDARY-INPUT-INCOMPLETE" + and finding.evidence.get("code") == "boundary_diff_shape_invalid" + for finding in report.findings + ) + diff_text = (out_dir / "verification-input.diff").read_text(encoding="utf-8") + assert diff_text.count("diff --git a/AGENTS.md b/AGENTS.md") == 1 + assert "uncommitted review follow-up" in diff_text + assert "committed issue fix" not in diff_text + assert verifier.changed_files == ["AGENTS.md"] + + plan = VerificationPlan.model_validate_json( + (out_dir / "verification-plan.json").read_text(encoding="utf-8") + ) + assert plan.inputs.changed_paths == ["AGENTS.md"] + assert plan.inputs.options["worktree_overlay_paths"] == ["AGENTS.md"] + assert plan.subject.git.worktree_overlay_sha256 is not None + receipt = VerificationReceipt.model_validate_json( + (out_dir / "verification-receipt.json").read_text(encoding="utf-8") + ) + assert receipt.subject_id == plan.subject.subject_id + assert receipt.request_id == plan.request_id + + +def test_worktree_overlay_identity_preserves_an_effectively_cancelled_path( + tmp_path: Path, +) -> None: + """A worktree cancellation is absent from policy diff but present in identity.""" + + repo = tmp_path / "repo" + sample_dst = repo / "samples" / "support_refund_agent" + sample_dst.parent.mkdir(parents=True) + shutil.copytree(REPO_ROOT / "samples" / "support_refund_agent", sample_dst) + _git(repo, "init") + _git(repo, "config", "user.email", "test@example.test") + _git(repo, "config", "user.name", "Test User") + _git(repo, "add", ".") + _git(repo, "commit", "-m", "base") + + cancelled = repo / "CLAUDE.md" + cancelled.write_text("committed instructions\n", encoding="utf-8") + _git(repo, "add", "CLAUDE.md") + _git(repo, "commit", "-m", "add instructions") + cancelled.unlink() + + out_dir = repo / "agents-shipgate-reports" + verifier, report, _exit_code = run_verify( + workspace=repo, + config=Path("samples/support_refund_agent/shipgate.yaml"), + base="HEAD~1", + head="HEAD", + archive_head=False, + out=out_dir, + ci_mode="advisory", + fail_on=None, + baseline=None, + baseline_mode="new-findings", + diff_from=None, + policy_packs=None, + plugins_enabled=False, + strict_plugins=False, + suggest_patches=False, + no_heuristics=False, + verbose=False, + ) + + assert report is not None + assert verifier.changed_files == [] + assert (out_dir / "verification-input.diff").read_text(encoding="utf-8") == "" + plan = VerificationPlan.model_validate_json( + (out_dir / "verification-plan.json").read_text(encoding="utf-8") + ) + assert plan.inputs.changed_paths == [] + assert plan.inputs.options["worktree_overlay_paths"] == ["CLAUDE.md"] + assert plan.subject.git.worktree_overlay_sha256 is not None + assert any( + "1 committed change is canceled by uncommitted worktree edits" in note + for note in verifier.base_notes + ) + + legacy_options = dict(plan.inputs.options) + legacy_options.pop("worktree_overlay_paths") + legacy_plan = plan.model_copy( + update={ + "inputs": plan.inputs.model_copy(update={"options": legacy_options}) + } + ) + with pytest.raises(ValueError, match="predates overlay path binding"): + _validate_git_subject(legacy_plan, repo) + + +def test_worktree_overlay_binds_a_cancelled_path_that_is_still_present( + tmp_path: Path, +) -> None: + """A canceled path leaves the policy diff but stays bound at its real bytes. + + The overlay set is HEAD-relative while the change set is merge-base- + relative, so a canceled path exists only in the former. Plan construction + runs under the static input snapshot, which reports a path it contains but + never read as absent — binding only the change set would attest a present + file as ``deleted`` and make the worker reject the plan the producer just + wrote. The sibling test above cancels by deleting, which cannot catch this. + """ + + repo = tmp_path / "repo" + sample_dst = repo / "samples" / "support_refund_agent" + sample_dst.parent.mkdir(parents=True) + shutil.copytree(REPO_ROOT / "samples" / "support_refund_agent", sample_dst) + cancelled = repo / "KEEP.md" + cancelled.write_text("original\n", encoding="utf-8") + _git(repo, "init") + _git(repo, "config", "user.email", "test@example.test") + _git(repo, "config", "user.name", "Test User") + _git(repo, "add", ".") + _git(repo, "commit", "-m", "base") + + cancelled.write_text("committed change\n", encoding="utf-8") + _git(repo, "add", "KEEP.md") + _git(repo, "commit", "-m", "change keep") + cancelled.write_text("original\n", encoding="utf-8") + + out_dir = repo / "agents-shipgate-reports" + verifier, report, _exit_code = run_verify( + workspace=repo, + config=Path("samples/support_refund_agent/shipgate.yaml"), + base="HEAD~1", + head="HEAD", + archive_head=False, + out=out_dir, + ci_mode="advisory", + fail_on=None, + baseline=None, + baseline_mode="new-findings", + diff_from=None, + policy_packs=None, + plugins_enabled=False, + strict_plugins=False, + suggest_patches=False, + no_heuristics=False, + verbose=False, + ) + + assert report is not None + assert verifier.changed_files == [] + plan = VerificationPlan.model_validate_json( + (out_dir / "verification-plan.json").read_text(encoding="utf-8") + ) + assert plan.inputs.changed_paths == [] + assert plan.inputs.options["worktree_overlay_paths"] == ["KEEP.md"] + # Raises unless the producer recorded the still-present canceled file at its + # real content, exactly as the worker recomputes it from the filesystem. + _validate_git_subject(plan, repo) + + +def test_verify_fail_closes_on_merge_base_relative_untracked_trust_root( + tmp_path: Path, +) -> None: + repo = tmp_path / "repo" + sample_dst = repo / "samples" / "support_refund_agent" + sample_dst.parent.mkdir(parents=True) + shutil.copytree(REPO_ROOT / "samples" / "support_refund_agent", sample_dst) + _git(repo, "init") + _git(repo, "config", "user.email", "test@example.test") + _git(repo, "config", "user.name", "Test User") + _git(repo, "add", ".") + _git(repo, "commit", "-m", "base") + + (repo / "notes.txt").write_text("committed branch change\n", encoding="utf-8") + _git(repo, "add", "notes.txt") + _git(repo, "commit", "-m", "branch change") + (repo / "AGENTS.md").write_text("untracked instructions\n", encoding="utf-8") + + verifier, report, _exit_code = run_verify( + workspace=repo, + config=Path("samples/support_refund_agent/shipgate.yaml"), + base="HEAD~1", + head="HEAD", + archive_head=False, + out=repo / "agents-shipgate-reports", + ci_mode="advisory", + fail_on=None, + baseline=None, + baseline_mode="new-findings", + diff_from=None, + policy_packs=None, + plugins_enabled=False, + strict_plugins=False, + suggest_patches=False, + no_heuristics=False, + verbose=False, + ) + + assert report is not None + assert "AGENTS.md" in verifier.changed_files + assert any( + finding.check_id == "SHIP-VERIFY-TRUST-ROOT-TOUCHED" + and finding.evidence.get("changed_file") == "AGENTS.md" + for finding in report.findings + ) + + def test_verify_fails_closed_when_worktree_diff_cannot_be_collected(monkeypatch, tmp_path): repo = tmp_path / "repo" sample_dst = repo / "samples" / "support_refund_agent"