Publish only the work: leave build artifacts out and report them (#80)
CI / checks (push) Skipped
CI / checks (pull_request) Failing after 56s
CI / checks (push) Skipped
CI / checks (pull_request) Failing after 56s
The clone gets the standard build artifacts in its own exclude file, so they never show as changes, even to a model's `git add -A`. The harness's commits leave the same patterns out even when the project tracks them, as the sandbox's main did. What was left out is reported in the publish note and the PR description. Found again in bring-up run 3 (sandbox PR #208). Fixes #80. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PPQ14L3LikyefCMFBqEbKE
This commit is contained in:
1 parent
4f152af170
commit
e02bac110d
5 files changed
+164
-8
No files matched your search
@@ -217,6 +217,7 @@ def prepare_workspace(
|
|||||||
if not repo.exists():
|
if not repo.exists():
|
||||||
repo.clone(remote, credential=credential)
|
repo.clone(remote, credential=credential)
|
||||||
repo.configure_identity(account, f"{account}@users.noreply.local")
|
repo.configure_identity(account, f"{account}@users.noreply.local")
|
||||||
|
repo.install_excludes() # build artifacts are never the work (#80)
|
||||||
return repo
|
return repo
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -33,6 +33,35 @@ class GitError(RuntimeError):
|
|||||||
pass
|
pass
|
||||||
|
|
||||||
|
|
||||||
|
# Build artifacts and caches that are never the work (D4, #80). Kept out of
|
||||||
|
# every commit the harness makes, tracked or not, and excluded in the clone
|
||||||
|
# so they don't even show as changes. Conservative on purpose: a pattern here
|
||||||
|
# must never match a project's real source.
|
||||||
|
ARTIFACT_PATTERNS = (
|
||||||
|
"__pycache__/",
|
||||||
|
"*.py[cod]",
|
||||||
|
".pytest_cache/",
|
||||||
|
".mypy_cache/",
|
||||||
|
".ruff_cache/",
|
||||||
|
".tox/",
|
||||||
|
".nox/",
|
||||||
|
"*.egg-info/",
|
||||||
|
".coverage",
|
||||||
|
"htmlcov/",
|
||||||
|
".venv/",
|
||||||
|
"node_modules/",
|
||||||
|
".DS_Store",
|
||||||
|
)
|
||||||
|
_EXCLUDES_MARK = "# nisse-dev: build artifacts are never the work (D4)"
|
||||||
|
|
||||||
|
|
||||||
|
def _pathspec(pattern: str) -> str:
|
||||||
|
"""A pathspec leaving ``pattern`` out wherever it occurs in the tree."""
|
||||||
|
glob = pattern.rstrip("/")
|
||||||
|
tail = "/**" if pattern.endswith("/") else ""
|
||||||
|
return f":(exclude,glob)**/{glob}{tail}"
|
||||||
|
|
||||||
|
|
||||||
@dataclass(frozen=True)
|
@dataclass(frozen=True)
|
||||||
class HTTPSToken:
|
class HTTPSToken:
|
||||||
"""A push-only account's token, for HTTPS remotes (D4)."""
|
"""A push-only account's token, for HTTPS remotes (D4)."""
|
||||||
@@ -84,6 +113,7 @@ class GitRepo:
|
|||||||
*args: str,
|
*args: str,
|
||||||
credential: PushCredential | None = None,
|
credential: PushCredential | None = None,
|
||||||
cwd: str | None = None,
|
cwd: str | None = None,
|
||||||
|
raw: bool = False,
|
||||||
) -> str:
|
) -> str:
|
||||||
quoted = " ".join(shlex.quote(a) for a in args)
|
quoted = " ".join(shlex.quote(a) for a in args)
|
||||||
command, stdin, secrets = f"git {quoted}", None, {}
|
command, stdin, secrets = f"git {quoted}", None, {}
|
||||||
@@ -117,7 +147,8 @@ class GitRepo:
|
|||||||
f"git {args[0]} failed: "
|
f"git {args[0]} failed: "
|
||||||
f"{result.stderr.strip() or result.stdout.strip()}"
|
f"{result.stderr.strip() or result.stdout.strip()}"
|
||||||
)
|
)
|
||||||
return result.stdout.strip()
|
# Raw for output whose leading whitespace means something (porcelain).
|
||||||
|
return result.stdout if raw else result.stdout.strip()
|
||||||
|
|
||||||
# -- inspection -------------------------------------------------------
|
# -- inspection -------------------------------------------------------
|
||||||
|
|
||||||
@@ -191,15 +222,42 @@ class GitRepo:
|
|||||||
return False
|
return False
|
||||||
return True
|
return True
|
||||||
|
|
||||||
|
def install_excludes(self) -> None:
|
||||||
|
"""List the build artifacts in the clone's own exclude file, so they
|
||||||
|
never appear as changes. Local to the clone: nothing in the project
|
||||||
|
changes. Idempotent."""
|
||||||
|
path = shlex.quote(str(self.root / ".git" / "info" / "exclude"))
|
||||||
|
block = "\n".join([_EXCLUDES_MARK, *ARTIFACT_PATTERNS])
|
||||||
|
result = self.host.execute(
|
||||||
|
f'mkdir -p -- "$(dirname -- {path})" && touch -- {path} && '
|
||||||
|
f"grep -qxF {shlex.quote(_EXCLUDES_MARK)} {path} || "
|
||||||
|
f"cat >> {path}",
|
||||||
|
stdin=block,
|
||||||
|
timeout_s=TIMEOUT_S,
|
||||||
|
)
|
||||||
|
if not result.ok:
|
||||||
|
raise GitError(f"could not set excludes: {result.stderr.strip()}")
|
||||||
|
|
||||||
def commit_all(self, message: str) -> str | None:
|
def commit_all(self, message: str) -> str | None:
|
||||||
"""Returns the new commit sha, or None when there was nothing to
|
"""Commit the work: every change except build artifacts, which are
|
||||||
commit — an empty milestone is a fact to report, not an error."""
|
left out even when the project tracks them (#80). Returns the new
|
||||||
|
commit sha, or None when there was nothing to commit — an empty
|
||||||
|
milestone is a fact to report, not an error."""
|
||||||
if not self.has_changes():
|
if not self.has_changes():
|
||||||
return None
|
return None
|
||||||
self._git("add", "-A")
|
self._git("add", "-A", "--", ".", *(_pathspec(p) for p in ARTIFACT_PATTERNS))
|
||||||
|
staged = self._git("diff", "--cached", "--name-only")
|
||||||
|
if not staged:
|
||||||
|
return None
|
||||||
self._git("commit", "-q", "-m", message)
|
self._git("commit", "-q", "-m", message)
|
||||||
return self.head_sha()
|
return self.head_sha()
|
||||||
|
|
||||||
|
def left_out(self) -> list[str]:
|
||||||
|
"""Changes not in the last commit: build artifacts the harness left
|
||||||
|
out, to be reported rather than silently dropped (D4)."""
|
||||||
|
out = self._git("status", "--porcelain", "--untracked-files=all", raw=True)
|
||||||
|
return sorted(line[3:] for line in out.splitlines() if line)
|
||||||
|
|
||||||
def push(
|
def push(
|
||||||
self, remote_url: str, branch: str, *, credential: PushCredential | None = None
|
self, remote_url: str, branch: str, *, credential: PushCredential | None = None
|
||||||
) -> None:
|
) -> None:
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ PR exists rather than repeating the push-and-open — the spec's own example of
|
|||||||
an idempotent transition reconciled against external truth.
|
an idempotent transition reconciled against external truth.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
from collections.abc import Sequence
|
||||||
from dataclasses import dataclass
|
from dataclasses import dataclass
|
||||||
|
|
||||||
from nisse_dev.git import GitRepo, PushCredential
|
from nisse_dev.git import GitRepo, PushCredential
|
||||||
@@ -80,6 +81,8 @@ class MilestonePublisher:
|
|||||||
return PublishResult(None, "no changes to publish")
|
return PublishResult(None, "no changes to publish")
|
||||||
# The executor committed its work itself; publish that.
|
# The executor committed its work itself; publish that.
|
||||||
sha = self._git.head_sha()
|
sha = self._git.head_sha()
|
||||||
|
# D4: what was left out (build artifacts) is reported, not dropped.
|
||||||
|
left_out = self._git.left_out()
|
||||||
|
|
||||||
intent_id = self._store.begin_intent(
|
intent_id = self._store.begin_intent(
|
||||||
OPEN_PR_INTENT,
|
OPEN_PR_INTENT,
|
||||||
@@ -99,17 +102,19 @@ class MilestonePublisher:
|
|||||||
return PublishResult(
|
return PublishResult(
|
||||||
existing,
|
existing,
|
||||||
f"PR #{existing.number} was already open for this milestone; "
|
f"PR #{existing.number} was already open for this milestone; "
|
||||||
f"pushed to it: {existing.url}",
|
f"pushed to it: {existing.url}" + _left_out_note(left_out),
|
||||||
)
|
)
|
||||||
pr = self._client.open_pull_request(
|
pr = self._client.open_pull_request(
|
||||||
self._repo,
|
self._repo,
|
||||||
head=branch,
|
head=branch,
|
||||||
base=self._base_branch,
|
base=self._base_branch,
|
||||||
title=f"Milestone {milestone}: {project}",
|
title=f"Milestone {milestone}: {project}",
|
||||||
body=_pr_body(milestone, plan, service_url),
|
body=_pr_body(milestone, plan, service_url, left_out),
|
||||||
)
|
)
|
||||||
self._store.commit_intent(intent_id, cause=f"opened PR #{pr.number}")
|
self._store.commit_intent(intent_id, cause=f"opened PR #{pr.number}")
|
||||||
return PublishResult(pr, f"opened PR #{pr.number}: {pr.url}")
|
return PublishResult(
|
||||||
|
pr, f"opened PR #{pr.number}: {pr.url}" + _left_out_note(left_out)
|
||||||
|
)
|
||||||
|
|
||||||
def published_pull_request(
|
def published_pull_request(
|
||||||
self, task_id: str, milestone: int
|
self, task_id: str, milestone: int
|
||||||
@@ -161,7 +166,21 @@ class MilestonePublisher:
|
|||||||
return sha
|
return sha
|
||||||
|
|
||||||
|
|
||||||
def _pr_body(milestone: int, plan: str, service_url: str | None) -> str:
|
def _left_out_note(paths: list[str]) -> str:
|
||||||
|
if not paths:
|
||||||
|
return ""
|
||||||
|
shown = ", ".join(paths[:5]) + (
|
||||||
|
f" and {len(paths) - 5} more" if len(paths) > 5 else ""
|
||||||
|
)
|
||||||
|
return f" (left out {len(paths)} build artifact(s): {shown})"
|
||||||
|
|
||||||
|
|
||||||
|
def _pr_body(
|
||||||
|
milestone: int,
|
||||||
|
plan: str,
|
||||||
|
service_url: str | None,
|
||||||
|
left_out: Sequence[str] = (),
|
||||||
|
) -> str:
|
||||||
parts = [
|
parts = [
|
||||||
f"Milestone {milestone}, implemented by nisse-dev against the approved plan.",
|
f"Milestone {milestone}, implemented by nisse-dev against the approved plan.",
|
||||||
"",
|
"",
|
||||||
@@ -174,4 +193,13 @@ def _pr_body(milestone: int, plan: str, service_url: str | None) -> str:
|
|||||||
# so a reviewer can exercise it. Nothing reports one yet; the browser
|
# so a reviewer can exercise it. Nothing reports one yet; the browser
|
||||||
# tool (D7) is what will.
|
# tool (D7) is what will.
|
||||||
parts += ["", "## Running test service", "", service_url]
|
parts += ["", "## Running test service", "", service_url]
|
||||||
|
if left_out:
|
||||||
|
parts += [
|
||||||
|
"",
|
||||||
|
"## Left out",
|
||||||
|
"",
|
||||||
|
"Build artifacts in the workspace, never part of the work:",
|
||||||
|
"",
|
||||||
|
*(f"- `{path}`" for path in left_out),
|
||||||
|
]
|
||||||
return "\n".join(parts)
|
return "\n".join(parts)
|
||||||
@@ -195,3 +195,52 @@ def test_an_https_token_answers_git_through_the_helper(
|
|||||||
assert asked.stdout == "username=[redacted]\npassword=[redacted]\n"
|
assert asked.stdout == "username=[redacted]\npassword=[redacted]\n"
|
||||||
stored = ssh_host.execute(f"{body} store", secrets={"NISSE_PUSH_TOKEN": "x"})
|
stored = ssh_host.execute(f"{body} store", secrets={"NISSE_PUSH_TOKEN": "x"})
|
||||||
assert stored.stdout == ""
|
assert stored.stdout == ""
|
||||||
|
|
||||||
|
|
||||||
|
# --- build artifacts are never the work (#80, D4) ----------------------------
|
||||||
|
|
||||||
|
|
||||||
|
def test_artifacts_are_left_out_even_when_the_project_tracks_them(
|
||||||
|
repo: GitRepo,
|
||||||
|
) -> None:
|
||||||
|
# The sandbox's main tracks .pyc files (an earlier run published them).
|
||||||
|
cache = repo.root / "pkg" / "__pycache__"
|
||||||
|
cache.mkdir(parents=True)
|
||||||
|
(repo.root / "pkg" / "m.py").write_text("x = 1\n")
|
||||||
|
(cache / "m.cpython-312.pyc").write_bytes(b"old")
|
||||||
|
repo._git("add", "-A")
|
||||||
|
repo._git("commit", "-qm", "a project that tracks its caches")
|
||||||
|
|
||||||
|
# A test run: the tracked .pyc changes, new caches appear, and the work.
|
||||||
|
(cache / "m.cpython-312.pyc").write_bytes(b"new")
|
||||||
|
(cache / "m.cpython-310.pyc").write_bytes(b"x")
|
||||||
|
(repo.root / ".pytest_cache").mkdir()
|
||||||
|
(repo.root / ".pytest_cache" / "v").write_text("x")
|
||||||
|
(repo.root / "pkg" / "m.py").write_text("x = 2\n")
|
||||||
|
|
||||||
|
assert repo.commit_all("the work") is not None
|
||||||
|
committed = repo._git("show", "--name-only", "--format=", "HEAD").split()
|
||||||
|
assert committed == ["pkg/m.py"]
|
||||||
|
assert repo.left_out() == [
|
||||||
|
".pytest_cache/v",
|
||||||
|
"pkg/__pycache__/m.cpython-310.pyc",
|
||||||
|
"pkg/__pycache__/m.cpython-312.pyc",
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
|
def test_only_artifacts_changed_means_nothing_to_commit(repo: GitRepo) -> None:
|
||||||
|
(repo.root / "__pycache__").mkdir()
|
||||||
|
(repo.root / "__pycache__" / "a.pyc").write_bytes(b"x")
|
||||||
|
assert repo.commit_all("nothing real") is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_excludes_hide_new_artifacts_and_install_once(repo: GitRepo) -> None:
|
||||||
|
repo.install_excludes()
|
||||||
|
repo.install_excludes()
|
||||||
|
exclude = (repo.root / ".git" / "info" / "exclude").read_text()
|
||||||
|
assert exclude.count("__pycache__/") == 1
|
||||||
|
(repo.root / "node_modules").mkdir()
|
||||||
|
(repo.root / "node_modules" / "x.js").write_text("x")
|
||||||
|
(repo.root / "real.py").write_text("x")
|
||||||
|
# Even the model's own `git add -A` now skips them.
|
||||||
|
assert repo._git("status", "--porcelain", "--untracked-files=all") == "?? real.py"
|
||||||
@@ -90,6 +90,9 @@ class FakeGit:
|
|||||||
def commit_all(self, message: str) -> str | None:
|
def commit_all(self, message: str) -> str | None:
|
||||||
return self.repo.commit_all(message)
|
return self.repo.commit_all(message)
|
||||||
|
|
||||||
|
def left_out(self) -> list[str]:
|
||||||
|
return self.repo.left_out()
|
||||||
|
|
||||||
def push(self, remote_url: str, branch: str, *, credential: object = None) -> None:
|
def push(self, remote_url: str, branch: str, *, credential: object = None) -> None:
|
||||||
self.pushed.append(branch)
|
self.pushed.append(branch)
|
||||||
self.remote[branch] = self.repo.head_sha()
|
self.remote[branch] = self.repo.head_sha()
|
||||||
@@ -147,6 +150,23 @@ def test_a_branch_named_after_the_task_does_not_block_publishing(
|
|||||||
assert git.pushed == ["demo-abc-milestone-1"]
|
assert git.pushed == ["demo-abc-milestone-1"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_left_out_artifacts_are_reported_not_published(
|
||||||
|
store: StateStore, tmp_path: Path
|
||||||
|
) -> None:
|
||||||
|
git = FakeGit(tmp_path / "work")
|
||||||
|
(tmp_path / "work" / "__pycache__").mkdir()
|
||||||
|
(tmp_path / "work" / "__pycache__" / "work.cpython-310.pyc").write_bytes(b"x")
|
||||||
|
client = FakeGitea()
|
||||||
|
result = build(store, git, client).publish(
|
||||||
|
task_id="t", milestone=1, project="demo", plan="p"
|
||||||
|
)
|
||||||
|
assert "left out 1 build artifact(s): __pycache__/work.cpython-310.pyc" in (
|
||||||
|
result.note
|
||||||
|
)
|
||||||
|
assert "## Left out" in client.opened[0]["body"]
|
||||||
|
assert "`__pycache__/work.cpython-310.pyc`" in client.opened[0]["body"]
|
||||||
|
|
||||||
|
|
||||||
def test_intent_is_recorded_then_committed(store: StateStore, tmp_path: Path) -> None:
|
def test_intent_is_recorded_then_committed(store: StateStore, tmp_path: Path) -> None:
|
||||||
build(store, FakeGit(tmp_path / "work"), FakeGitea()).publish(
|
build(store, FakeGit(tmp_path / "work"), FakeGitea()).publish(
|
||||||
task_id="t", milestone=1, project="demo", plan="p"
|
task_id="t", milestone=1, project="demo", plan="p"
|
||||||
|
|||||||
Reference in new issue
Block a user