diff --git a/.github/workflows/reusable-codex-run.yml b/.github/workflows/reusable-codex-run.yml index 558d79c45..e309ad638 100644 --- a/.github/workflows/reusable-codex-run.yml +++ b/.github/workflows/reusable-codex-run.yml @@ -1174,6 +1174,7 @@ jobs: python - <<'PY' import json import os + import tempfile from datetime import datetime, timezone payload = { @@ -1193,9 +1194,20 @@ jobs: "pr_number": os.environ.get("PR_NUMBER", ""), "emitted_at": datetime.now(timezone.utc).isoformat().replace("+00:00", "Z"), } - with open("langsmith-fleet-worker-attempt.json", "w", encoding="utf-8") as fh: + # Write OUTSIDE the checkout. This file exists only to feed the upload-artifact + # step below, and a staging file in the working tree is a liability: the commit + # step later in this job runs `git add -A` and then subtracts a hand-curated + # exclusion list, so any artifact not on that list is committed onto whatever PR + # is open. This one was not, so it landed in six consumer repos and collided + # there with the copy the previous merge had left on main. RUNNER_TEMP is wiped + # between jobs and is never part of the repository, which removes the class + # rather than adding entry N+1 to the list. + out_dir = os.environ.get("RUNNER_TEMP") or tempfile.gettempdir() + out_path = os.path.join(out_dir, "langsmith-fleet-worker-attempt.json") + with open(out_path, "w", encoding="utf-8") as fh: json.dump(payload, fh, indent=2, sort_keys=True) fh.write("\n") + print(f"worker model attempt artifact written to {out_path}") PY - name: Upload worker model attempt artifact @@ -1203,7 +1215,7 @@ jobs: uses: actions/upload-artifact@v7 with: name: langsmith-fleet-v1-worker-attempt-${{ inputs.pr_number || github.run_id }} - path: langsmith-fleet-worker-attempt.json + path: ${{ runner.temp }}/langsmith-fleet-worker-attempt.json retention-days: 30 - name: Analyze Codex session @@ -1651,8 +1663,8 @@ jobs: git reset HEAD -- \ codex-output*.md \ codex-prompt*.md \ - codex-session-*.jsonl \ - codex-analysis-*.json \ + codex-session*.jsonl \ + codex-analysis*.json \ claude-output*.md \ claude-prompt*.md \ claude-session*.log \ diff --git a/scripts/sync_status_file_ignores.py b/scripts/sync_status_file_ignores.py index 5c5f965d9..d6f714888 100755 --- a/scripts/sync_status_file_ignores.py +++ b/scripts/sync_status_file_ignores.py @@ -68,6 +68,18 @@ "workloop-state.md", # Test/coverage artifacts "coverage.xml", + # Per-run agent execution telemetry (HIGH conflict risk). reusable-codex-run.yml rewrites + # this into the checkout root every agent round to stage its upload-artifact step; while + # tracked, codex-autofix committed the diff onto whatever PR was open and the next PR + # collided with main's copy. Patterns, not the literal name, because the file is named after + # the role recorded; bounded by extension so langsmith_*.py sources stay committable. + # ROOT-ANCHORED, and that leading slash is load-bearing. Unanchored, a gitignore pattern + # matches at EVERY depth, so `langsmith-fleet*.json` also swallowed this repo's own tracked + # docs/contracts/schemas/langsmith-fleet-v1.schema.json -- verified with check-ignore, not + # inferred. Same near-miss as the node_modules work: the debris lands in the checkout ROOT, + # so that is the only place the pattern should reach. + "/langsmith-fleet*.json", + "/langsmith-fleet*.ndjson", # Wrong package manager artifacts (defense-in-depth) "Pipfile.lock", "poetry.lock", @@ -85,7 +97,7 @@ # Sync from: stranske/Workflows templates/consumer-repo/.gitignore # Validate: python scripts/sync_status_file_ignores.py --check # ============================================================================= -# Template-Version: 5 +# Template-Version: 6 # BEGIN WORKFLOWS STATUS FILES """ diff --git a/templates/consumer-repo/.gitignore b/templates/consumer-repo/.gitignore index ea9990ab2..2e1d746cd 100644 --- a/templates/consumer-repo/.gitignore +++ b/templates/consumer-repo/.gitignore @@ -146,7 +146,7 @@ cython_debug/ # Sync from: stranske/Workflows templates/consumer-repo/.gitignore # Validate: python scripts/sync_status_file_ignores.py --check # ============================================================================= -# Template-Version: 5 +# Template-Version: 6 # BEGIN WORKFLOWS STATUS FILES # Agent working files (HIGH conflict risk) @@ -188,6 +188,22 @@ workloop-state.md # Test/coverage artifacts coverage.xml +# Per-run agent execution telemetry (HIGH conflict risk) +# reusable-codex-run.yml writes langsmith-fleet-worker-attempt.json into the checkout +# root on every agent round, only to stage the actions/upload-artifact step that +# follows it. While tracked, every round produced a diff that codex-autofix committed +# onto whatever PR was open, so the next PR collided with the copy the last merge left +# on main. A PATTERN, not the literal: the file is named after the ROLE recorded, so a +# verifier or evaluator attempt arrives as a sibling. Bounded to the two extensions the +# langsmith-fleet/v1 schema emits, so langsmith_*.py sources and langsmith-fleet-*.md +# docs stay committable. ROOT-ANCHORED: unanchored, a pattern matches at every depth, and +# `langsmith-fleet*.json` also swallowed Workflows' own tracked +# docs/contracts/schemas/langsmith-fleet-v1.schema.json (verified with check-ignore). The +# debris only ever lands in the checkout root, so that is all the pattern should reach -- +# the same anchoring lesson the node_modules entry below records. +/langsmith-fleet*.json +/langsmith-fleet*.ndjson + # Wrong package manager artifacts (defense-in-depth) Pipfile.lock poetry.lock diff --git a/tests/scripts/test_sync_status_file_ignores.py b/tests/scripts/test_sync_status_file_ignores.py index 5156aa14f..93721fc7d 100644 --- a/tests/scripts/test_sync_status_file_ignores.py +++ b/tests/scripts/test_sync_status_file_ignores.py @@ -96,6 +96,50 @@ def test_template_has_version_and_anchors() -> None: assert fallback_version == template_version +def test_langsmith_patterns_are_root_anchored_and_spare_the_tracked_schema( + tmp_path: Path, +) -> None: + """The langsmith-fleet debris rules must reach the checkout ROOT and nothing deeper. + + Unanchored, a gitignore pattern matches at every depth, so `langsmith-fleet*.json` + also matched this repo's own tracked `docs/contracts/schemas/langsmith-fleet-v1.schema.json` + -- silently making a load-bearing contract file untrackable. That is the same near-miss + the vendored-node_modules exception records, so it gets the same kind of guard: asserted + against real `git check-ignore`, not against the pattern string. Drop either leading + slash in FALLBACK_PATTERNS and the schema assertion below fails. + """ + subprocess = pytest.importorskip("subprocess") + run = subprocess.run + + run(["git", "init", "-q", str(tmp_path)], check=True) + (tmp_path / ".gitignore").write_text(_full_gitignore_content(), encoding="utf-8") + + debris = "langsmith-fleet-worker-attempt.json" + schema = "docs/contracts/schemas/langsmith-fleet-v1.schema.json" + for rel in (debris, schema): + target = tmp_path / rel + target.parent.mkdir(parents=True, exist_ok=True) + target.touch() + + def ignored(rel: str) -> bool: + return run(["git", "check-ignore", "-q", rel], cwd=tmp_path, check=False).returncode == 0 + + # The per-run artifact lands in the root, and that is the only place it lands. + assert ignored(debris), f"{debris} must be ignored -- it is per-run CI debris" + # The schema is committed source and must survive the pattern. + assert not ignored( + schema + ), f"{schema} must stay trackable; the langsmith-fleet rules lost their root anchor" + + # And the anchoring is visible in the canonical list itself, so a future edit that + # re-broadens it has to delete a leading slash on purpose. + langsmith = [ + pattern for pattern in sync_status_file_ignores.CANONICAL_PATTERNS if "langsmith" in pattern + ] + assert langsmith, "the langsmith-fleet debris patterns went missing from the canonical list" + assert all(pattern.startswith("/") for pattern in langsmith), langsmith + + def test_load_template_patterns_requires_version_marker( monkeypatch: pytest.MonkeyPatch, ) -> None: diff --git a/tests/workflows/test_runner_artifacts_stay_out_of_the_checkout.py b/tests/workflows/test_runner_artifacts_stay_out_of_the_checkout.py new file mode 100644 index 000000000..16e634cb1 --- /dev/null +++ b/tests/workflows/test_runner_artifacts_stay_out_of_the_checkout.py @@ -0,0 +1,249 @@ +"""Static guard: a runner artifact must not be committable, and this enforces the checklist. + +Background — why this test exists +================================= +`docs/WORKFLOW_ARTIFACT_CHECKLIST.md` already says all of this. Its decision tree ends at +"auto-generated → use workflow artifacts instead, add to .gitignore", and it carries a "Recovery +from Artifact Pollution" procedure. But **nothing referenced or enforced it** — a grep across +`.yml`, `.py` and `.sh` found zero callers — so it was a document, not a gate, and the gate is what +was missing. + +What went wrong without it. `reusable-codex-run.yml` gained a "Write worker model attempt artifact" +step that wrote `langsmith-fleet-worker-attempt.json` into the **checkout root**, purely to stage +the `actions/upload-artifact` step on the next line. The commit step earlier in the same job runs +`git add -A` and then subtracts a hand-curated `git reset HEAD --` list, so an artifact absent from +that list is committed onto whatever PR happens to be open. This one was absent. PR #2856 +(2026-07-31) diagnosed it exactly — "while tracked, every run that rewrote it produced a diff that +codex-autofix then committed onto whatever PR happened to be open" — and fixed **this repo only**, +never `templates/consumer-repo/.gitignore`. Six consumer repos were still carrying a tracked copy on +2026-08-23 (Travel-Plan-Permission, Counter_Risk, Pension-Data, Ready, trip-planner, Orchestrator), +and in stranske/Orchestrator it produced an add/add merge conflict on a path neither side authored. + +The mechanic that decides the fix, verified against a scratch repository: `git add -A` **skips** an +ignored *untracked* path but **stages** an ignored *tracked* one. So ignoring is not untracking, and +the `git reset HEAD --` list is only load-bearing for paths already in the index. Writing outside the +checkout beats both — it removes the class instead of adding entry N+1 to a list somebody must +remember. + +What is asserted +================ +1. The worker-attempt artifact is written to and uploaded from `RUNNER_TEMP`, never the checkout. +2. Its emitter heredoc still compiles (it is YAML text, so nothing else would catch a syntax error + until runtime — the same gap `test_reusable_run_refpack_heredoc_compiles.py` exists to close). +3. **The general rule:** every `actions/upload-artifact` path in the reusable runner workflows is + either outside the checkout, or covered by the commit step's exclusion list, or covered by the + consumer-template `.gitignore` — otherwise it must be named in `KNOWN_IN_CHECKOUT` with a + reason. That is the checklist, as a gate. A new artifact step now fails here instead of + surfacing weeks later as a merge conflict in a consumer repo. +""" + +from __future__ import annotations + +import fnmatch +import re +import textwrap +from pathlib import Path + +import pytest +import yaml + +ROOT = Path(__file__).resolve().parents[2] + +AGENT_REGISTRY = ".github/agents/registry.yml" +CODEX_RUN = ".github/workflows/reusable-codex-run.yml" +CONSUMER_TEMPLATE_GITIGNORE = "templates/consumer-repo/.gitignore" + +WORKER_ATTEMPT_BASENAME = "langsmith-fleet-worker-attempt.json" + +# Artifact paths that ARE inside the checkout and are deliberately not excluded/ignored. Each entry +# is an incident record: say why it is safe, so the next reader can tell a reviewed decision from an +# oversight. Anything not listed here must be excluded, ignored, or written outside the checkout. +KNOWN_IN_CHECKOUT: dict[str, str] = { + "error-diagnostics/": ( + "Created by the 'Create error diagnostics' step, which runs AFTER the commit step in the " + "same job, so `git add -A` never sees it. That is step ORDERING, not a safety property: " + "reorder the steps, or add a second commit step later in the job, and this becomes the " + "langsmith-fleet-worker-attempt.json defect again. Prefer moving it under RUNNER_TEMP if it " + "is ever touched." + ), +} + + +def workflow_text(rel: str) -> str: + path = ROOT / rel + assert path.is_file(), f"missing workflow file: {rel}" + return path.read_text(encoding="utf-8") + + +def registered_runner_workflows() -> tuple[str, ...]: + """Runner workflows named by the live agent registry, not a hand-maintained subset.""" + registry = yaml.safe_load((ROOT / AGENT_REGISTRY).read_text(encoding="utf-8")) + workflows = { + str(agent["runner_workflow"]) + for agent in (registry.get("agents") or {}).values() + if agent.get("runner_workflow") + } + assert workflows, f"{AGENT_REGISTRY} names no reusable agent runner workflows" + return tuple(sorted(workflows)) + + +def upload_artifact_paths(rel: str) -> list[str]: + """Every `path:` given to actions/upload-artifact, read from the PARSED yaml. + + Parsed rather than grepped on purpose: `path:` accepts a block scalar listing several globs, and + a regex over the raw text silently sees only the first line of one. A gate with a parser that + quietly under-reports is worse than no gate. + """ + doc = yaml.safe_load(workflow_text(rel)) + found: list[str] = [] + for job in (doc.get("jobs") or {}).values(): + for step in job.get("steps") or []: + uses = str(step.get("uses") or "") + if not uses.startswith("actions/upload-artifact"): + continue + raw = (step.get("with") or {}).get("path") + if raw is None: + continue + for line in str(raw).splitlines(): + entry = line.strip() + if entry: + found.append(entry) + return found + + +def commit_step_exclusions(rel: str) -> list[str]: + """The `git reset HEAD --` denylist the commit step subtracts from `git add -A`.""" + match = re.search(r"git reset HEAD -- \\\n(.*?)\n\s*2>/dev/null", workflow_text(rel), re.S) + assert match, ( + "could not locate the commit step's `git reset HEAD --` exclusion list in " + f"{rel}; this guard can no longer tell which artifacts are excluded" + ) + return [ + line.strip().rstrip("\\").strip() + for line in match.group(1).splitlines() + if line.strip() and line.strip() != "\\" + ] + + +def template_ignore_patterns() -> list[str]: + text = (ROOT / CONSUMER_TEMPLATE_GITIGNORE).read_text(encoding="utf-8") + return [ + line.strip() + for line in text.splitlines() + if line.strip() and not line.strip().startswith("#") + ] + + +def covered_by(path: str, patterns: list[str]) -> bool: + """Whether an exclusion or ignore pattern covers the whole uploaded path/glob.""" + bare = path.rstrip("/") + for pattern in patterns: + pat = pattern.lstrip("/").rstrip("/") + if not pat or pattern.startswith("!"): + continue + if fnmatch.fnmatch(bare, pat): + return True + return False + + +def outside_checkout(path: str) -> bool: + """Runner-temp and absolute paths are not in the working tree, so git can never stage them.""" + lowered = path.lower() + return ( + path.startswith("/") + or "runner.temp" in lowered + or "runner_temp" in lowered + or "${{ env.runner_temp }}" in lowered + ) + + +def test_worker_attempt_artifact_is_written_outside_the_checkout(): + text = workflow_text(CODEX_RUN) + assert f'open("{WORKER_ATTEMPT_BASENAME}"' not in text, ( + f"{CODEX_RUN} writes {WORKER_ATTEMPT_BASENAME} into the checkout root again. The commit step " + "runs `git add -A` and subtracts only a hand-curated list, so this file gets committed onto " + "whatever PR is open and then collides with the copy the previous merge left on main — the " + "PR #2856 defect, which reached six consumer repos. Write it under RUNNER_TEMP; the " + "upload-artifact step accepts any path." + ) + assert ( + "RUNNER_TEMP" in text + ), f"{CODEX_RUN} no longer stages the worker attempt under RUNNER_TEMP" + + +def test_worker_attempt_artifact_is_uploaded_from_outside_the_checkout(): + matches = [p for p in upload_artifact_paths(CODEX_RUN) if WORKER_ATTEMPT_BASENAME in p] + assert matches, f"{CODEX_RUN} no longer uploads {WORKER_ATTEMPT_BASENAME}" + for path in matches: + assert outside_checkout(path), ( + f"{CODEX_RUN} uploads {path!r} from inside the checkout. Uploading is the whole reason " + "the file exists, so it never needs to be in the working tree at all." + ) + + +def test_worker_attempt_emitter_heredoc_compiles(): + text = workflow_text(CODEX_RUN) + match = re.search( + r"- name: Write worker model attempt artifact.*?python - <<'PY'\n(.*?)\n PY\n", + text, + re.S, + ) + assert match, f"{CODEX_RUN}: could not locate the worker-attempt emitter heredoc" + body = textwrap.dedent(match.group(1)) + try: + compile(body, f"<{CODEX_RUN}:worker-attempt>", "exec") + except SyntaxError as exc: # IndentationError is a SyntaxError subclass + pytest.fail( + f"{CODEX_RUN}: worker-attempt emitter heredoc does not compile: " + f"{type(exc).__name__}: {exc}" + ) + + +@pytest.mark.parametrize("workflow_rel", registered_runner_workflows()) +def test_every_uploaded_artifact_is_uncommittable(workflow_rel: str): + """The checklist, as a gate: an artifact step may not leave a committable file behind.""" + exclusions = commit_step_exclusions(workflow_rel) + ignores = template_ignore_patterns() + + offenders = [] + for path in sorted(set(upload_artifact_paths(workflow_rel))): + if outside_checkout(path) or path in KNOWN_IN_CHECKOUT: + continue + if covered_by(path, exclusions) or covered_by(path, ignores): + continue + offenders.append(path) + + assert not offenders, ( + f"{workflow_rel} uploads artifact path(s) {offenders} that live in the checkout and are " + "neither excluded by the commit step's `git reset HEAD --` list nor ignored by " + f"{CONSUMER_TEMPLATE_GITIGNORE}. `git add -A` will commit them onto whatever PR is open, and " + "the next PR then conflicts with the copy the last merge left on main — that is exactly how " + f"{WORKER_ATTEMPT_BASENAME} reached six consumer repos. Pick one: write it under " + "RUNNER_TEMP (best — removes the class), add it to the template .gitignore (and untrack it " + "everywhere, since ignoring does not untrack), or record it in KNOWN_IN_CHECKOUT with a " + "reason saying why it is safe." + ) + + +def test_known_in_checkout_entries_state_a_reason(): + """An allowlist without reasons decays into a list of things nobody dares remove.""" + for path, reason in KNOWN_IN_CHECKOUT.items(): + assert len(reason) > 80, f"KNOWN_IN_CHECKOUT[{path!r}] needs a real reason, not a label" + + +def test_known_in_checkout_has_no_stale_entries(): + """An allowlisted path that is no longer uploaded anywhere must be deleted, not carried.""" + uploaded = { + path for rel in registered_runner_workflows() for path in upload_artifact_paths(rel) + } + stale = sorted(set(KNOWN_IN_CHECKOUT) - uploaded) + assert not stale, ( + f"KNOWN_IN_CHECKOUT names {stale}, which no upload-artifact step references any more. " + "Remove the entry — a stale allowlist entry silently widens the gate." + ) + + +def test_uploaded_glob_requires_a_covering_pattern_not_a_single_file_match(): + """A file inside an uploaded glob must not be mistaken for coverage of the whole glob.""" + assert not covered_by("foo/*.json", ["foo/report.json"]) + assert covered_by("foo/report.json", ["foo/*.json"])