From 066deec911e8e54c5046df54f338fa73c49c53fe Mon Sep 17 00:00:00 2001 From: BangerSoul Date: Sun, 4 Oct 2026 07:23:28 +0530 Subject: [PATCH 1/8] fix(audit): the X1 lock, not the checkout witness, decides every pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit revision_audit compared the checkout against `repo manifest -r` output - the witness of itself - so disk-vs-disk could never disagree, and a tree with 190 drifted projects passed as "OK all 1175 active projects" (job 303324). The lock is now the sole yardstick: SHA pins are compared directly against it, tag pins are resolved in-project, an unresolvable pin fails closed as MISMATCH, and `repo manifest -r` is demoted to corroboration with per-class counters - its disagreements become note lines, so it can neither rescue a bad tree nor convict a good one. Scenario 9 of test-revision-audit.sh is rewritten around that contract (unresolvable pin, witness cannot rescue it, clean corroboration, the 303324 regression, a contradicting witness), and a negative control via EMBERBIRD_RECIPE confirms the old logic goes red over a drifted tree with the exact false OK line 303324 printed. ๐Ÿค– Generated with Codebuff Co-Authored-By: Codebuff --- tools/checks/test-revision-audit.sh | 137 ++++++++++++------ tools/guest-build/build-from-manifest.sh | 168 +++++++++++++++-------- 2 files changed, 210 insertions(+), 95 deletions(-) diff --git a/tools/checks/test-revision-audit.sh b/tools/checks/test-revision-audit.sh index 1ffb476..aea4a01 100644 --- a/tools/checks/test-revision-audit.sh +++ b/tools/checks/test-revision-audit.sh @@ -11,7 +11,10 @@ # 7. sanitize_worktrees() clears local modifications/untracked files that would # make the forced re-sync's checkout abort; # 8. sanitize_worktrees() survives a checkout whose HEAD never landed; -# 9. the audit judges against a stubbed `repo manifest -r` when one is supplied. +# 9. a stubbed `repo manifest -r` CORROBORATES the audit and never decides it - +# in particular, a witness that faithfully reports a drifted tree still yields +# MISMATCH (the job 303324 regression, and the reason the lock is the only +# yardstick). # WHY: job 302748 (2026-09-30) died at `lunch` because the Crave node's pre-seeded # tree held ~1000 lock paths at LOS 20 revisions while every other check in the # recipe reasoned about existence and files only. Job 302857 (2026-10-01) then died @@ -265,64 +268,120 @@ fi && { echo " [FAIL] untracked debris survived an unborn-HEAD sanitize"; fail=1; } \ || echo " [PASS] untracked debris removed even with no HEAD to reset to" -echo "-- 9. the audit judges against repo's own resolution (stubbed repo manifest -r)" -# The recipe hands the audit `repo manifest -r`, which resolves each pin the way repo -# does. Two properties matter, and NEITHER is testable without a stub: -# a) a pin the project's own checkout can no longer resolve is still decidable, -# because repo's resolved answer stands in for it - this is the whole reason -# the recipe passes the witness in; -# b) when repo's answer disagrees with HEAD, HEAD is wrong - the audit must not -# rubber-stamp a tree just because a resolved manifest was supplied. +echo "-- 9. the repo manifest -r witness CORROBORATES; it never decides" +# The recipe hands the audit a `repo manifest -r` witness, and this scenario is the +# whole correction from job 303324. That audit used a witness entry as the answer to +# compare on-disk HEAD against, under the comment "repo resolved this pin; its +# answer is the lock's answer, by construction" - which is false. `repo manifest -r` +# reports the revision each project is CHECKED OUT at, so it echoes the disk; the +# comparison agreed with itself and could not fail. It printed "OK all 1175 active +# projects ... (1175/1175 resolved via repo manifest -r)" over a tree holding 190 +# wrong commits and 8 absent projects, one screen above verify-lock.py refusing the +# same tree on the same witness file. +# +# So the witness is a corroborating second reading, never the yardstick, and all +# four properties below are about that distinction: +# a) a pin the checkout can no longer resolve is still undecidable WITH a witness +# - the witness must not rescue it; +# b) a clean tree plus a witness that agrees with the lock is clean, and the +# report must say how much the witness actually corroborated; +# c) THE REGRESSION (303324): a tree drifted away from the lock, with a witness +# that faithfully reports the drift, is a MISMATCH - not OK; +# d) a witness that contradicts a clean tree cannot fail it either - it is +# reported, and the lock still governs. RESOLVED="$T/resolved.xml" -stub_resolved() { # $1 = path whose resolved revision to corrupt (optional) - local bad="${1:-}" +stub_witness() { # args: " " pairs; a path may be repeated to override + rm -f "$RESOLVED" { printf '\n\n' - printf ' \n' "$SHA_OK" - printf ' \n' "$SHA_WRONG" - printf ' \n' "$SHA_GONE2" - printf ' \n' "$SHA_TAG" - printf ' \n' "$SHA_SHARED_A" - if [ "$bad" = "shared_b" ]; then - printf ' \n' "$(printf 'f%.0s' $(seq 40))" - else - printf ' \n' "$SHA_SHARED_B" - fi + for pair in "$@"; do + printf ' \n' "${pair%% *}" "${pair##* }" + done printf '\n' } > "$RESOLVED" } +FULL_WITNESS=( "ok $SHA_OK" "wrong $SHA_WRONG" "gone $SHA_GONE2" "tagged $SHA_TAG" \ + "shared_a $SHA_SHARED_A" "shared_b $SHA_SHARED_B" ) -# Damage the one thing only repo's resolution can still answer: shared_b's local tag. +# Damage shared_b's local tag, so the lock's refs/tags/android-12.1.0_r22 no longer +# resolves inside the project that pins it. The lock itself is undamaged. git -C "$WORKSPACE/shared_b" tag -d "android-12.1.0_r22" >/dev/null R9a="$T/r9a.txt" run 3 "$R9a" if grep -q "does not resolve it" "$R9a"; then - echo " [PASS] without the witness, an unresolvable local pin is undecidable" + echo " [PASS] (a1) without a witness, an unresolvable local pin is undecidable" else echo " [FAIL] expected 'does not resolve it', got: $(head -1 "$R9a")"; fail=1 fi -stub_resolved +# The witness names a concrete commit for exactly that project. It must NOT become +# the yardstick: a pin the checkout cannot resolve is still unproven, and letting +# the witness vouch for it would reintroduce the tautology one level down. +stub_witness "${FULL_WITNESS[@]}" R9b="$T/r9b.txt" -run 0 "$R9b" "$RESOLVED" -case "$(head -1 "$R9b")" in - "OK all 6 active projects"*) echo " [PASS] with the witness, that same project is decidable";; - *) echo " [FAIL] witness did not rescue the audit: $(head -1 "$R9b")"; fail=1;; +run 3 "$R9b" "$RESOLVED" +if grep -q "^mismatch shared_b .*does not resolve it" "$R9b"; then + echo " [PASS] (a2) a witness entry does NOT rescue a pin the checkout cannot resolve" +else + echo " [FAIL] the witness overrode the lock: $(head -1 "$R9b")"; fail=1 +fi + +# Repair the tag; now the tree is clean and the witness agrees with the lock. +git -C "$WORKSPACE/shared_b" tag "android-12.1.0_r22" "$SHA_SHARED_B" +R9c="$T/r9c.txt" +run 0 "$R9c" "$RESOLVED" +case "$(head -1 "$R9c")" in + "OK all 6 active projects"*) echo " [PASS] (b) a clean tree the witness corroborates is OK";; + *) echo " [FAIL] clean corroborated tree: $(head -1 "$R9c")"; fail=1;; esac -case "$(head -1 "$R9b")" in - *"repo manifest -r"*) echo " [PASS] the report names the yardstick it used";; - *) echo " [FAIL] report does not say it used the resolved manifest"; fail=1;; +case "$(head -1 "$R9c")" in + *"repo manifest -r corroborates 6/6"*) + echo " [PASS] (b) the report says how much the witness corroborated";; + *) + echo " [FAIL] report does not report corroboration: $(head -1 "$R9c")"; fail=1;; esac -stub_resolved shared_b -R9c="$T/r9c.txt" -run 3 "$R9c" "$RESOLVED" -if grep -q "^mismatch shared_b " "$R9c"; then - echo " [PASS] a resolved revision that disagrees with HEAD is caught" -else - echo " [FAIL] the audit accepted a tree the witness disagrees with: $(head -1 "$R9c")"; fail=1 -fi +# (c) THE REGRESSION. Drift `wrong` off its locked SHA, and let the witness report +# the drift - which is what repo genuinely sees. Pre-fix, `want` came from the +# witness, so want == head and the audit said OK. This is 303324 in miniature. +git -C "$WORKSPACE/wrong" -c user.email=t@t -c user.name=t commit -q --allow-empty -m drift2 +DRIFTED="$(git -C "$WORKSPACE/wrong" rev-parse HEAD)" +stub_witness "${FULL_WITNESS[@]}" "wrong $DRIFTED" +R9d="$T/r9d.txt" +run 3 "$R9d" "$RESOLVED" +case "$(head -1 "$R9d")" in + "MISMATCH 1 "*) echo " [PASS] (c) a tree drifted from the lock is MISMATCH even when the witness agrees with disk";; + *) echo " [FAIL] the audit trusted the witness over the lock: $(head -1 "$R9d")"; fail=1;; +esac +# The report abbreviates commits to 12 chars, so match on that prefix, not the +# full sha - an assertion that never matches would read as a code failure. +DRIFT12="${DRIFTED%${DRIFTED#????????????}}" +LOCK12="${SHA_WRONG%${SHA_WRONG#????????????}}" +grep -q "^mismatch wrong on-disk $DRIFT12, locked $LOCK12" "$R9d" \ + && echo " [PASS] (c) the mismatch names the drifted commit and the locked one" \ + || { echo " [FAIL] wrong mismatch line: $(grep '^mismatch ' "$R9d")"; fail=1; } +grep -q "^note wrong repo manifest -r says $DRIFT12 but the lock says $LOCK12" "$R9d" \ + && echo " [PASS] (c) the witness disagreeing with the lock is reported, as a note" \ + || { echo " [FAIL] the witness disagreement was swallowed: $(cat "$R9d")"; fail=1; } +git -C "$WORKSPACE/wrong" update-ref HEAD "$SHA_WRONG" # restore for later scenarios + +# (d) A witness that contradicts an otherwise clean tree must not fail it - the +# lock still governs - but the disagreement has to be visible in the report. +BADREV="$(printf 'f%.0s' $(seq 40))" +stub_witness "${FULL_WITNESS[@]}" "wrong $BADREV" +R9e="$T/r9e.txt" +run 0 "$R9e" "$RESOLVED" +case "$(head -1 "$R9e")" in + "OK all 6 active projects"*) echo " [PASS] (d) a contradicting witness cannot fail a clean tree";; + *) echo " [FAIL] a witness vetoed the lock: $(head -1 "$R9e")"; fail=1;; +esac +case "$(head -1 "$R9e")" in + *"repo manifest -r corroborates 5/6"*) + echo " [PASS] (d) and the report counts the disagreement instead of hiding it";; + *) + echo " [FAIL] corroboration count hides the conflict: $(head -1 "$R9e")"; fail=1;; +esac echo "-- 10. the recipe re-resolves the witness BEFORE the post-re-sync re-audit" # The control flow in the repair branch is not exercisable offline - it needs `repo` diff --git a/tools/guest-build/build-from-manifest.sh b/tools/guest-build/build-from-manifest.sh index 3266ae8..f0629e2 100644 --- a/tools/guest-build/build-from-manifest.sh +++ b/tools/guest-build/build-from-manifest.sh @@ -470,15 +470,17 @@ PY fi log "sync complete: all $total active manifest projects present on disk" - # Ask repo what every project resolves to, and audit against that. + # Ask repo what every project is checked out at, and audit the tree against the + # LOCK. The witness is corroboration, never the yardstick. # - # The lock pins 872 of its 1175 active projects to refs/tags/android-12.1.0_r22, and - # an AOSP release tag is a DIFFERENT commit in every repository - so a tag pin can - # only be settled by repo's own revision resolution, not by re-deriving one in a - # shell heredoc. `repo manifest -r` IS that resolution, and it is already the - # artifact the provenance record names as the build-side witness. Generate it here - # so the audit compares on-disk HEAD against repo's answer instead of guessing - # (see revision_audit's $2). + # The lock pins 872 of its 1175 active projects to refs/tags/android-12.1.0_r22, + # and an AOSP release tag is a DIFFERENT commit in every repository, so the audit + # resolves each such pin inside the project that pins it (see revision_audit). + # `repo manifest -r` is still generated, because it is the artifact the provenance + # record names as the build-side witness and the one verify-lock.py checks - but + # it reports what the tree ALREADY holds, so using it as the audit's reference + # answer would compare the disk with itself. That bug printed OK on a tree with + # 190 drifted projects in job 303324. # # This is called a THIRD time after any repair - see below, and resolve_witness for # why a witness that has been around since before a `repo sync` is worse than none. @@ -490,13 +492,12 @@ PY # must never stop the build. # # ON AN OPTIONAL FAILURE THE WITNESS IS REMOVED, not kept. A witness we could - # not refresh describes a tree that no longer exists, and both ways that goes - # wrong are worse than simply having none: the audit would judge the current - # tree against a stale yardstick, which can either invent drift on a perfectly - # good tree or wave through one that genuinely moved. Dropping it costs - # precision - every project then falls back to resolving its pin in its own - # checkout, which is the same commit repo would name - and buys certainty that - # no comparison uses a value from the wrong moment. + # not refresh describes a tree that no longer exists, and a stale corroborating + # reading is worse than none: it would either accuse a perfectly good tree of + # disagreeing with the lock, or reassure the reader about a tree it never saw. + # Dropping it costs one line of independent cross-checking - the audit still + # judges every project from the lock - and buys certainty that no reported + # comparison comes from the wrong moment. local out="$1" mode="${2:-optional}" rm -f "$out" if repo manifest -r -o "$out" && [ -s "$out" ]; then @@ -509,11 +510,11 @@ PY if [ "$mode" = "required" ]; then die "repo manifest -r failed - the synced tree is missing or cannot resolve a project the frozen lock references (see the sync-completeness step above), so the canonical X1 witness cannot be produced." fi - echo "note: repo manifest -r could not resolve every project; the audit will resolve each pin in its own checkout instead (the canonical witness check below is mandatory and runs regardless)" + echo "note: repo manifest -r could not resolve every project; the audit will judge the tree from the lock alone, without corroboration (the canonical witness check below is mandatory and runs regardless)" return 1 } - log "resolving the active manifest per project (repo manifest -r)" + log "generating the repo manifest -r witness (corroboration for the audit, and the artifact verify-lock.py checks)" resolved_manifest="$OUT_DIR/repo-manifest-r.xml" resolve_witness "$resolved_manifest" || true @@ -534,11 +535,9 @@ PY # offender), echoes the first line to stdout for the build log, and exits # nonzero when anything on disk disagrees with the lock. # - # $2 is optional: the `repo manifest -r` output. When it is present and a - # project's revision is in it, that resolved commit is the yardstick - see - # below. When it is absent (or does not cover a project) the pin is resolved - # in the project's own git dir instead, which is the same answer for every - # tree whose refs survived. + # $2 is optional: the `repo manifest -r` witness, used for CORROBORATION ONLY. + # It is never the yardstick. See the block that builds `resolved` below - and + # the post-mortem in docs/M2-CRAVE-BUILD.md for what happens when it is. LOCK_XML="$LOCK_XML" WORKSPACE="$WORKSPACE" python3 - "$1" "${2:-}" <<'PY' || return $? import os, re, subprocess, sys, xml.etree.ElementTree as ET @@ -552,7 +551,9 @@ root = ET.parse(os.environ["LOCK_XML"]).getroot() # A 40-hex pin names its own commit, so it needs no resolution at all. Anything # else (a tag) has to be resolved inside the project that pins it - see below. -SHA_RE = re.compile(r"[0-9a-f]{40}") +# Anchored, like verify-lock.py's own SHA_RE, so a revision is never judged by +# the 40 hex characters that happen to sit inside it. +SHA_RE = re.compile(r"^[0-9a-f]{40}$") rows = [] for p in root.findall("project"): @@ -576,44 +577,72 @@ def git_in(project_path, *args): return out.stdout.strip() if out.returncode == 0 else None -# The recipe hands us `repo manifest -r` when it could produce it. That is repo's -# OWN revision resolution, and it is strictly better than anything re-derived here: -# 872 of this lock's 1175 active projects are pinned to refs/tags/android-12.1.0_r22, -# and an AOSP release tag is a DIFFERENT commit in every repository, so a tag pin can -# only be settled by the resolution repo itself performed. Re-deriving it in a shell -# heredoc is how this audit used to compare 872 correct projects against one arbitrary -# repository's tag. Only entries repo actually resolved to a concrete commit are -# taken; a revision left as a moving ref is ignored so it can never be used as a -# yardstick (falling back is strictly better than comparing HEAD to refs/heads/*). +# `repo manifest -r` is repo's own reading of the tree, and it is a CORROBORATING +# witness - never the yardstick. This is the correction from job 303324, and the +# reason is worth stating precisely, because the previous version of this block +# asserted the opposite in a comment: +# +# "repo resolved this pin; its answer is the lock's answer, by construction." +# +# That is false, and it is not a subtle one. `repo manifest -r` reports the +# revision each project is CHECKED OUT at, so for the 1175/1175 case it simply +# echoed the disk back. Comparing on-disk HEAD to it therefore agreed by +# construction and could not fail, no matter what the tree held. On 303324 the +# seed was LOS 20 content (a foreign-base .repo that resync.sh never repaired), +# 190 SHA-pinned projects sat at the wrong commits and 8 were absent - and this +# audit still printed "OK all 1175 active projects on disk at their locked +# revisions (1175/1175 resolved via repo manifest -r)", one screen above +# verify-lock.py refusing the same tree on the same witness file. A verdict that +# cannot disagree is not a verdict. So: the LOCK decides, every time. +# +# What the witness is still good for, and is used for here: +# * an independent second reading of each project's revision, which is +# cross-checked against the lock and reported (see witness_* counters), and +# * coverage - which lock projects the tree never mentioned at all. +# A disagreement is surfaced loudly; it never rescues a project, because letting +# it do that would just reintroduce the tautology one level down. +# +# Only entries repo resolved to a concrete 40-hex commit are usable; a revision +# left as a moving ref is recorded as "seen, unresolved" and not used as a value. resolved = {} -resolved_arg = sys.argv[2] if len(sys.argv) > 2 else "" -if resolved_arg and os.path.exists(resolved_arg): +witness_seen = set() +witness_arg = sys.argv[2] if len(sys.argv) > 2 else "" +if witness_arg and os.path.exists(witness_arg): try: - rroot = ET.parse(resolved_arg).getroot() + rroot = ET.parse(witness_arg).getroot() except ET.ParseError as exc: - print("note: cannot parse %s (%s); resolving pins locally instead" % (resolved_arg, exc), + print("note: cannot parse %s (%s); the lock alone will judge the tree" % (witness_arg, exc), file=sys.stderr) rroot = None if rroot is not None: for el in rroot.findall("project"): rpath = el.get("path") or el.get("name") rrev = el.get("revision") or "" - if rpath and SHA_RE.fullmatch(rrev): + if not rpath: + continue + witness_seen.add(rpath) + if SHA_RE.match(rrev): resolved[rpath] = rrev missing = [] mismatch = [] -# How much of this verdict rests on repo's own resolution, and how much on the -# weaker per-project fallback. Reported on the OK line because a partial witness is -# normal on a depth-1 pre-seeded tree, and "(resolved via repo manifest -r)" on its -# own would read as "repo settled all of them" when it may have settled two. -via_witness = 0 +# The yardstick is the lock, every time, so the breakdown below describes how each +# pin in the LOCK was read - not how many projects the witness happened to cover. +# The witness's own numbers are reported separately, because "1175/1175 resolved +# via repo manifest -r" is exactly the sentence that hid a 190-project drift. sha_pins = 0 in_project = 0 +witness_ok = 0 +witness_conflict = 0 +witness_no_entry = 0 +notes = [] for path, rev in rows: if not os.path.exists(os.path.join(ws, path, ".git")): missing.append(path) + if witness_arg and path not in witness_seen: + notes.append("note %s has no checkout AND no repo manifest -r entry - the " + "tree's manifest does not carry this project at all" % path) continue head = git_in(path, "rev-parse", "HEAD") if head is None: @@ -622,19 +651,28 @@ for path, rev in rows: if not rev: mismatch.append((path, "lock entry carries no revision")) continue - if path in resolved: - # repo resolved this pin; its answer is the lock's answer, by construction. - want = resolved[path] - via_witness += 1 - elif SHA_RE.fullmatch(rev): - # A SHA names its own commit, so it is never in doubt and needs no witness. + # THE LOCK IS THE YARDSTICK. Both arms below derive `want` from the lock and + # from nothing else; the witness is never consulted to produce it. + if SHA_RE.match(rev): + # A SHA names its own commit. There is no resolution step to get wrong. want = rev sha_pins += 1 else: - # No resolved manifest for this project: resolve the pin in the project's own - # git dir, which is what repo would do for it too. + # A tag pin names a DIFFERENT commit in every repository, so it has to be + # resolved in the project that pins it. Resolving it HERE - in that + # project's own git dir, with that project's own refs - is the same thing + # repo does when it checks the revision out, and it is the resolution + # that is independent of the current on-disk HEAD. (The old bug's + # rationale, "re-deriving it in a shell heredoc", was really about + # resolving a tag ONCE in some other repository and applying that single + # answer to all 872 tag-pinned projects. Doing it per project, in the + # project itself, is not that.) want = git_in(path, "rev-parse", rev + "^{commit}") if not want: + # Fail closed: an unresolvable pin is an unproven pin. If the tag ref + # is not in the checkout (a depth-1 seed may not have it), we cannot + # show this project is at its locked revision, so we do not claim it + # is. The re-sync below is what brings the object in. mismatch.append( (path, "lock pins %s but this project's own checkout does not resolve it" % rev) ) @@ -642,6 +680,17 @@ for path, rev in rows: in_project += 1 if head != want: mismatch.append((path, "on-disk %s, locked %s (%s)" % (head[:12], want[:12], rev[:28]))) + # Corroboration only, and only after the lock has already had its say. + if witness_arg: + if path in resolved: + if resolved[path] == want: + witness_ok += 1 + else: + witness_conflict += 1 + notes.append("note %s repo manifest -r says %s but the lock says %s (%s) - " + "the lock governs" % (path, resolved[path][:12], want[:12], rev[:28])) + else: + witness_no_entry += 1 lines = [] if not missing and not mismatch: @@ -650,14 +699,14 @@ if not missing and not mismatch: # whether the tree was actually proven by repo - rather than by a local fallback # on a shallow tree - has to be able to tell the difference from one line. how = [] - if via_witness: - how.append("%d/%d resolved via repo manifest -r" % (via_witness, len(rows))) - else: - how.append("no repo manifest -r witness") if sha_pins: - how.append("%d SHA pins" % sha_pins) + how.append("%d SHA pins compared directly against the lock" % sha_pins) if in_project: - how.append("%d resolved in-project" % in_project) + how.append("%d tag pins resolved in-project" % in_project) + if not witness_arg or not witness_seen: + how.append("no repo manifest -r witness to corroborate") + else: + how.append("repo manifest -r corroborates %d/%d" % (witness_ok, witness_ok + witness_conflict + witness_no_entry)) lines.append( "OK all %d active projects on disk at their locked revisions (%s)" % (len(rows), ", ".join(how)) @@ -669,6 +718,13 @@ else: if mismatch: lines.append("MISMATCH %d (checkout content differs from the lock)" % len(mismatch)) lines.extend("mismatch %s %s" % (path, why) for path, why in mismatch) +# The caller turns lines starting with "missing "/"mismatch " into a scoped repo +# sync, so note lines must not be mistakable for offenders. They never are - and +# they go to stderr as well, because a witness that contradicts the lock is worth +# seeing in the build log even when the tree passes. +lines.extend(notes) +for n in notes: + print(n, file=sys.stderr) report = sys.argv[1] if len(sys.argv) > 1 else "-" text = "\n".join(lines) + "\n" From 51177d8693f7f9244c2d324c2ff9168b8cf2a735 Mon Sep 17 00:00:00 2001 From: BangerSoul Date: Sun, 4 Oct 2026 07:24:07 +0530 Subject: [PATCH 2/8] fix(crave): status pins the job id and fails closed on client errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit status ran an unpinned getlog, looked for a "Your jobs" header the client has never printed, and exited 0 even while the client answered with "Error: could not get matching git url at: ..." on the same stream as its data (observed live against job 303324). A status command that cannot fail converts "I could not look" into "there is nothing wrong" - the one confusion this runbook cannot afford. It now pins --projectID/--jobID from $JOB or the run record and refuses to guess when there is neither; parses the real section tables ("Your active jobs:" / "Job History:") with any other header ending the section, so a project row can never be read as a job row; distinguishes "left the queue" from "the client drew no table at all" (UNKNOWN, not a guess); and judges every probe on both its exit status and its text, exiting nonzero and naming the failing probe. Pinned offline by the new tools/checks/test-crave-remote-build.sh: a stub client on CRAVE_SHIM records the argv it was called with, and 15 assertions drive the real script through the pins, the table parsing, the refusal to guess, and both failure modes. Wired into run-offline-checks.sh; a control run against the pre-fix script goes red. ๐Ÿค– Generated with Codebuff Co-Authored-By: Codebuff --- docs/M2-CRAVE-BUILD.md | 134 ++++++++++-- tools/checks/README.md | 32 ++- tools/checks/run-offline-checks.sh | 2 + tools/checks/test-crave-remote-build.sh | 263 ++++++++++++++++++++++++ tools/crave/run-remote-build.sh | 96 ++++++++- 5 files changed, 507 insertions(+), 20 deletions(-) create mode 100644 tools/checks/test-crave-remote-build.sh diff --git a/docs/M2-CRAVE-BUILD.md b/docs/M2-CRAVE-BUILD.md index ee7f6c7..03a5b03 100644 --- a/docs/M2-CRAVE-BUILD.md +++ b/docs/M2-CRAVE-BUILD.md @@ -92,6 +92,8 @@ From a checkout of this repository: bash tools/crave/run-remote-build.sh run # watch it / read the whole remote log +# `status` EXITS NONZERO when the client could not answer, so it is safe in a script. +# JOB= pins the lookup; without it the id recorded by `run` is used. bash tools/crave/run-remote-build.sh status bash tools/crave/run-remote-build.sh log @@ -269,17 +271,65 @@ phantom drifts, and the audit could never pass its own post-re-sync re-audit. Ev project's pin is now resolved in **its own** checkout, with an explicit guard for a lock entry that carries no `revision` at all. -**2. Now it asks `repo` instead of re-deriving the answer.** A tag pin can only be -settled by the resolution `repo` itself performed, so the recipe generates -`repo manifest -r` **before** the audit and hands it in. When a project appears in that -witness, the audit compares on-disk `HEAD` against repo's resolved commit; a `revision` -repo left as a moving ref is ignored rather than used as a yardstick. With no witness the -per-project resolution is the fallback, so the pre-audit generation is non-fatal and can -only make the audit more accurate, never unavailable. The `OK` line says which yardstick -produced it and how much of it came from where โ€” `(6/6 resolved via repo manifest -r, -2 SHA pins, 4 resolved in-project)` โ€” because a partial witness is *normal* on a -depth-1 pre-seeded tree, and a bare "(resolved via repo manifest -r)" would read as -"repo settled all of them" when it may have settled two. +**2. Now it corroborates against `repo`, but the LOCK is still the only yardstick.** +The witness `repo manifest -r` is generated before the audit and handed in โ€” and it is +used to *corroborate*, never to decide. An earlier version of this section said the +opposite ("a tag pin can only be settled by the resolution `repo` itself performed", so +compare `HEAD` against repo's resolved commit). That was wrong, and job 303324 is what +proved it: see ยง2c. The `OK` line now reports the lock's own breakdown plus how much the +witness corroborated โ€” `OK all 6 active projects on disk at their locked revisions (3 SHA +pins compared directly against the lock, 3 tag pins resolved in-project, repo manifest -r +corroborates 6/6)`. A disagreement between witness and lock is printed as a `note` line +(and to stderr, so it lands in the remote build log) that names both commits and ends +"the lock governs". + +**2c. `repo manifest -r` can never be the reference answer โ€” job 303324 (2026-10-03).** +Job **303324** (`6e09b54`) FAILED in **7m58s**, before compiling anything. The Crave node +was pre-seeded with **LOS 20**: `.repo/manifests` pointed at +`https://github.com/accupara/los20.git` instead of `https://github.com/BlissRoms-x86/manifest.git`, +and `resync.sh` failed to re-point it (`remote origin does not have refs/heads/master`) +yet still reported `All repositories synchronized successfully.`, so only the 137 genuinely +absent projects were synced and **1038 of 1175** lock paths were left at LOS 20 revisions. + +The mandatory canonical gate caught it, which is the point of that gate existing: + +``` +[FAIL] 8 locked project(s) are absent from the synced tree: external/adt-infra, tools/adt/idea, + tools/base, tools/build, tools/idea, tools/motodev, tools/studio/cloud, tools/swt +[FAIL] 190 SHA-pinned project(s) are not at their locked revision: art (lock d898a1fed5f5, + tree aeeafbd45929), bionic (lock 145acec53cc3, tree e0aac7df6f58), ... cts +LOCK VERIFICATION FAILED (2 problem(s), verdict=witness-mismatch, exit=1) +FATAL: the synced tree does not match the committed X1 lock - stopping before the build +``` + +One screen **above** that, the revision audit on the *same witness file* printed: + +``` +OK all 1175 active projects on disk at their locked revisions (1175/1175 resolved via repo manifest -r) +``` + +It was agreeing with itself. `repo manifest -r` reports the revision each project is +**checked out at** โ€” it is a reading of the tree, not of the lock โ€” so comparing on-disk +`HEAD` against it compares disk with disk. The code even asserted the reason in a comment: +*"repo resolved this pin; its answer is the lock's answer, by construction."* That is +false, and it could not have failed on any tree. (`art` is the proof: the lock pins SHA +`d898a1fed5f5`, and the witness reported `aeeafbd45929` โ€” a SHA pin echoed back as +anything other than itself would mean `-r` were printing the manifest rather than the tree.) + +The audit now takes `want` from the lock and only from the lock: a 40-hex pin is compared +directly, a tag pin is resolved in the project that pins it, and a pin that resolves +nowhere is an unproven pin and fails closed. The witness is consulted only afterwards, to +count agreements, count disagreements, and report which lock paths the tree never +mentioned โ€” a disagreement is surfaced, never allowed to rescue a project, because letting +it do that would just move the tautology one level down. Scenario 9 of +`test-revision-audit.sh` is the regression test, and against the pre-fix recipe it +reproduces 303324 in miniature: a drifted tree plus a witness that faithfully reports the +drift yields `OK all 6 active projects ... (6/6 resolved via repo manifest -r)`. + +Lesson worth keeping separate from the code: **two checks that disagree are a bug in one +of them, and the one that is *structurally incapable* of failing is the suspect.** The +gate and the audit read the same witness and reached opposite verdicts, and the tie was +broken not by which was more thorough but by which could have been wrong. **2b. The witness is re-resolved after the repair, not just before the first audit.** The post-re-sync re-audit is the last gate before `lunch`/`make`, and it was still being @@ -330,7 +380,9 @@ regenerates the witness after any repair, so the gate describes the tree about t Offline coverage: 49 checks in `tools/manifest/test-verify-lock.py` (9 new ones for the witness, including all four failure modes), and 14 scenarios in -`tools/checks/test-revision-audit.sh`. Notable ones: scenario 10 pins the *order* of the +`tools/checks/test-revision-audit.sh`. Notable ones: scenario 9 is the 303324 regression +(witness corroborates, never decides โ€” proven to fail against the pre-fix recipe); +scenario 10 pins the *order* of the refresh relative to the re-audit in the recipe source (that control flow needs `repo` to run, and a future edit that moved the refresh back after the re-audit would still pass `bash -n`); scenarios 12โ€“13 drive `inspect_seed` and `manifests_clean` against synthetic @@ -400,6 +452,64 @@ Resubmitted 2026-10-01 as job **303004**, pinned to `ded56c9` โ€” this fix. Veri queued payload from `list --json` (`jobs_active[0].workspace.cmd` names the commit): the plain-text `crave list` table can show a stale payload, so JSON is the check that counts. +### `status` was three lines that could not fail (fixed 2026-10-03) + +Diagnosing 303324 meant asking the client what it thought, and the command meant to do +that had never worked: + +```bash +( cd "$TICKET_DIR" && bash "$CRAVE_SHIM" list | sed -n '/Your jobs/,$p' | head -8 + bash "$CRAVE_SHIM" getlog 2>&1 | tail -25 ) +``` + +Every part was wrong against the real client: + +- **The table was always empty.** The client prints `Your active jobs:` and + `Job History:` โ€” never `Your jobs`. So the `sed` range matched nothing, and an empty + table is indistinguishable from "nothing to report". When the client cannot resolve + the account at all it prints *no* job table whatsoever, which is the state this + machine is in right now. +- **`getlog` was not pinned.** `log`, `watch` and `pull` all pass `--jobID`; `status` + did not, so the client resolved against the current directory's workspace and + answered *"No running job found on this workspace"* โ€” which reads exactly like a + dead job. +- **It exited 0 no matter what.** Observed directly: three client errors on the wire, + `echo $?` = 0. A status command that cannot fail converts *"I could not look"* into + *"there is nothing wrong"*, which is the one confusion this runbook cannot afford. + +It now pins `--projectID`/`--jobID` (refusing to run at all without a job id rather +than issuing an unpinned call), reads both real section headers, and judges each probe +on **both** its exit status and its text โ€” necessary because the client prints +`Error: could not get matching git url at: C:/Users/` on the same stream as its +data while still exiting 0. It distinguishes three things that used to look alike: the +job is in the active table, the job has left the queue, and *the client drew no table +at all*, in which case it says the state is **UNKNOWN** rather than guessing. + +Verified live against job 303324 โ€” it now fetches that job's pinned log and **exits 1** +with `client error: could not get matching git url` and the `UNKNOWN` state, where the +old version printed an empty table and exited 0. Pinned offline by +`tools/checks/test-crave-remote-build.sh` (15 assertions against a stub client that +records the argv it was called with), which is red against the previous version. + +`run` is now the only subcommand that requires `origin/$branch` to be reachable, since +it is the only one that launches anything; refusing to report the status of a job that +already ran, because this checkout's origin is unreachable, is the wrong trade โ€” and +scoping it is also what lets the offline suite drive the read-only commands without a +network. + +## Executed 2026-10-03: job 303324 ran and FAILED (foreign-base seed, and an audit that could not fail) + +Job **303324** (`6e09b54`, the merge of PR #6) FAILED in **7m58s**, before compiling. +The fail-closed behaviour worked: the canonical gate refused a tree that is not the lock, +so no wrong artifact was produced and no build time was wasted. What it exposed is +documented in **ยง2c** above โ€” a foreign-base `.repo` that left 1038 of 1175 projects at +LOS 20 revisions, and a revision audit that compared the tree against a reading of itself +and so reported `OK all 1175 active projects ...` on the same file the gate rejected. +The audit now judges from the lock; scenario 9 in `test-revision-audit.sh` fails against +the pre-fix recipe. Still outstanding for the next submission: `inspect_seed` reports a +foreign base but does not refuse it by default, so this class of node is still discovered +by a ~8-minute job rather than by preflight. + ## Executed 2026-09-30: job 302748 ran and FAILED (the pre-seeded-tree trap, now closed) Job **302748** (`0fc8173`, the fix for the failure below) queued ~11h and FAILED in diff --git a/tools/checks/README.md b/tools/checks/README.md index 5744d19..f510b21 100644 --- a/tools/checks/README.md +++ b/tools/checks/README.md @@ -39,7 +39,8 @@ command. | X2 evidence, pulled record | `../manifest/verify-x2-provenance.py` | that `image/out/x2-provenance.json` describes the artifacts actually on disk: every recorded hash and byte count matches, no unrecorded `.iso`/`.img` is sitting beside it, and `manifest_revision` equals the committed X1 pin | | X2 evidence, verifier contract | `../manifest/test-verify-x2-provenance.py` | that the check above *fails* on a stale, truncated, unrecorded, traversing or wrong-build record, and that an empty artifact list is a failure rather than a vacuous pass | | build recipe | `test-build-recipe.py` | the recipe's two embedded Python heredocs, artifact discovery, and the completeness constants it shares with the lock | -| revision audit | `test-revision-audit.sh` | the recipe's verification guards extracted *verbatim* by `lib-audit.sh`: `revision_audit()` (per-project tag resolution, a tag one repository has and another does not, a stubbed `repo manifest -r` witness and its absence), `sanitize_worktrees()`, `resolve_witness()` (refresh ordering + a stubbed `repo`), and the preflight pair `inspect_seed()` / `manifests_clean()` against synthetic `.repo` trees | +| revision audit | `test-revision-audit.sh` | the recipe's verification guards extracted *verbatim* by `lib-audit.sh`: `revision_audit()` (per-project tag resolution, a tag one repository has and another does not, and the 303324 regression โ€” a `repo manifest -r` witness corroborates the verdict and can never produce it), `sanitize_worktrees()`, `resolve_witness()` (refresh ordering + a stubbed `repo`), and the preflight pair `inspect_seed()` / `manifests_clean()` against synthetic `.repo` trees | +| crave wrapper | `test-crave-remote-build.sh` | `run-remote-build.sh`'s `status`, driven end-to-end against a **stub** client on `CRAVE_SHIM` that records the argv it was called with: that `getlog` is pinned with `--jobID`/`--projectID`, that the real section headers (`Your active jobs:` / `Job History:`) are parsed and other tables are not, that "left the queue" is distinguished from "the client drew no table", and that a client `Error:` line or nonzero exit makes the command exit nonzero | | fixture git | `lib-fixtures.sh` + `git-shim.sh` | whether this environment's git can record a revision at all; when it cannot, the shim stands in so the audit still runs against real revisions instead of a field of unborn HEADs | | PowerShell, static | `check-powershell-static.py` | ASCII purity, delimiter pairing, no dangling refs to removed symbols, and the launcher's deliberate `-DryRun` fast path | | shellcheck | `run-offline-checks.sh` | every shell script, when shellcheck is installed (informational: it is neither pinned nor installed project-wide, so it never reds a run) | @@ -58,7 +59,10 @@ cannot be mistaken for one: * **QEMU boot and the X3-X5 evidence** need a WHPX-provisioned Windows host. * **`repo` itself** is not available here. The revision audit is exercised with a *stubbed* `repo manifest -r` (and its absence), so what is pinned is the audit's - decision logic -- not that a real `repo` emits the XML the recipe expects. + decision logic -- not that a real `repo` emits the XML the recipe expects. Note that + the stub can only tell you how the audit *treats* a witness: the finding behind job + 303324 was that `repo manifest -r` reports what the tree already holds, so it is a + corroborating reading and never the yardstick. The stub is faithful to that contract. * **A real pulled image.** Until a Crave job has actually succeeded and `image/out/x2-provenance.json` exists, `verify-x2-provenance.py` exits **2** and the suite reports a `skip`. That is not a pass: it is how "there is no image yet" stays @@ -104,10 +108,34 @@ git show HEAD~1:tools/guest-build/build-from-manifest.sh > /tmp/old.sh EMBERBIRD_RECIPE=/tmp/old.sh bash tools/checks/test-revision-audit.sh # must FAIL ``` +The Crave wrapper's test honours the same idea under a different name, +`EMBERBIRD_CRAVE_RUNNER`, because it drives the script itself rather than extracted +fragments of it: + +```bash +git show HEAD~1:tools/crave/run-remote-build.sh > /tmp/old-crave.sh +EMBERBIRD_CRAVE_RUNNER=/tmp/old-crave.sh bash tools/checks/test-crave-remote-build.sh # must FAIL +``` + +One honest limit on that control: the previous `run-remote-build.sh` had no +`CRAVE_SHIM` override, so pointing the test at it makes the script call the **real** +client. It still goes red on the substantive assertions (`--jobID` not passed, no rows +shown, the client's error text swallowed), but its exit code is then that of a real +API round-trip rather than of the stub, so treat the control as evidence about +*behaviour*, not about exit codes. + Anything the previous commit was missing must go red, for the right reason, *without aborting the run* -- a check that crashes the suite instead of reporting a failure hides the state of every scenario after it, which is the one thing this suite exists to prevent. +This is how the 303324 regression test was validated rather than merely written. The +pre-fix recipe, pointed at by `EMBERBIRD_RECIPE`, reports +`OK all 6 active projects ... (6/6 resolved via repo manifest -r)` over a tree with a +deliberately drifted project -- the same shape as the `OK all 1175 active projects ... +(1175/1175 resolved via repo manifest -r)` that job 303324 printed over a tree with 190 +drifted projects. A regression test that passes against the code it was written to fix is +decorative; this one was confirmed to fail against that code first. + ## Rules for adding a check here 1. **Read-only.** Nothing in this directory may write inside the repository. Temp diff --git a/tools/checks/run-offline-checks.sh b/tools/checks/run-offline-checks.sh index 5f57dc5..207e86b 100644 --- a/tools/checks/run-offline-checks.sh +++ b/tools/checks/run-offline-checks.sh @@ -28,6 +28,7 @@ # * tools/checks/test-build-recipe.py - the build recipe's embedded Python, # its artifact discovery, and the completeness constants it shares with the lock # * tools/checks/test-revision-audit.sh - the recipe's on-disk revision audit +# * tools/checks/test-crave-remote-build.sh - the Crave wrapper's `status` # (HEAD vs the lock) on synthetic git repos: clean, drifted, missing, moved tag # * tools/manifest/test-verify-x2-provenance.py - the post-pull X2 evidence # check's own contract: a stale, truncated, unrecorded or wrong-build record @@ -287,6 +288,7 @@ run "X1 lock: verifier exit-code contract" python3 tools/manifest/test-ver run "X1 lock: generator unit tests" python3 tools/checks/test-manifest-resolver.py run "build recipe: offline tests" python3 tools/checks/test-build-recipe.py run "build recipe: revision audit behaviour" bash tools/checks/test-revision-audit.sh +run "crave client wrapper: status (stub)" bash tools/checks/test-crave-remote-build.sh run "PowerShell: static checks" python3 tools/checks/check-powershell-static.py run "X2 evidence: pulled record vs artifacts" check_x2_provenance run "shellcheck (informational)" check_shellcheck diff --git a/tools/checks/test-crave-remote-build.sh b/tools/checks/test-crave-remote-build.sh new file mode 100644 index 0000000..2684265 --- /dev/null +++ b/tools/checks/test-crave-remote-build.sh @@ -0,0 +1,263 @@ +#!/usr/bin/env bash +# Offline exercise of tools/crave/run-remote-build.sh against a STUB client. +# +# WHY THIS EXISTS +# The `status` subcommand shipped as three lines that could not fail: +# +# ( cd "$TICKET_DIR" && bash "$CRAVE_SHIM" list | sed -n '/Your jobs/,$p' | head -8 +# bash "$CRAVE_SHIM" getlog 2>&1 | tail -25 ) +# +# and every part of that was wrong against the real client, observed 2026-10-03: +# * the client prints "Your active jobs:" and "Job History:" - never "Your jobs" - +# so the table was ALWAYS empty, and an empty table is indistinguishable from +# "nothing to report"; +# * `getlog` carried no --jobID, while `log`/`watch`/`pull` all pin one. Unpinned, +# the client resolves against the current directory's workspace and answers +# "No running job found on this workspace", which makes a healthy job look dead; +# * it exited 0 even when the client printed +# Error: could not get matching git url at: C:/Users/ +# - the client's own diagnostics arrive on the same stream as its data, and it +# does not reliably set a nonzero status. +# +# A status command that cannot fail is worse than no status command: it converts +# "I could not look" into "there is nothing wrong". These tests drive the real +# script (not a copy of its logic) with a stub on CRAVE_SHIM, so a future edit that +# drops the --jobID pin or swallows a client error goes red here. +# +# WHAT IS AND IS NOT TESTED +# The stub speaks for the client's OUTPUT contract only: which section headers it +# prints and what its exit status is. It cannot prove how the real client behaves +# under an API outage. Both are recorded in docs/M2-CRAVE-BUILD.md. +set -uo pipefail + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "$HERE/../.." && pwd)" +# Which revision of the script is under test. Same convention as EMBERBIRD_RECIPE in +# lib-audit.sh: a test written alongside a fix passes trivially against that fix, so +# point it at the previous revision and require it to go red - +# git show HEAD~1:tools/crave/run-remote-build.sh > /tmp/old.sh +# EMBERBIRD_CRAVE_RUNNER=/tmp/old.sh bash tools/checks/test-crave-remote-build.sh # must FAIL +SCRIPT="${EMBERBIRD_CRAVE_RUNNER:-$REPO_ROOT/tools/crave/run-remote-build.sh}" +# Some sandboxes refuse `git commit`, and ensure_ticket resolves the ticket's branch +# with `rev-parse --abbrev-ref HEAD` - on a repo with no commit that prints a fatal +# error to stderr and pollutes every assertion. lib-fixtures.sh probes for a git that +# records revisions and installs a plumbing-backed shim only if it must. +# shellcheck source=lib-fixtures.sh +source "$HERE/lib-fixtures.sh" + +T="$(mktemp -d)" +trap 'rm -rf "$T"' EXIT +emberbird_ensure_git_record "$T" || { + echo "FATAL: cannot obtain a git that records revisions; the ticket fixture would be broken" >&2 + exit 1 +} + +fail=0 +pass() { echo " [PASS] $1"; } +bad() { echo " [FAIL] $1"; fail=1; } + +# ---------------------------------------------------------------- the stub client -- +# One stub for every subcommand. It records the argv it was called with, so a test +# can assert THAT the --jobID pin was passed, not merely that the output looks right. +STUB="$T/crave-stub" +cat > "$STUB" <<'STUB' +#!/usr/bin/env bash +# A stand-in for tools/crave/crave.sh. Behaviour is driven by STUB_LIST_BODY, +# STUB_LIST_RC, STUB_LOG_BODY, STUB_LOG_RC. Every invocation appends its argv to +# STUB_ARGV so a test can prove which flags the script actually passed. +printf '%s\n' "$*" >> "${STUB_ARGV:?}" +sub="$1"; shift +case "$sub" in + list) + printf '%s' "${STUB_LIST_BODY:-}" + exit "${STUB_LIST_RC:-0}" ;; + getlog) + printf '%s' "${STUB_LOG_BODY:-}" + exit "${STUB_LOG_RC:-0}" ;; + *) + echo "stub: unexpected subcommand '$sub'" >&2; exit 97 ;; +esac +STUB +chmod +x "$STUB" + +# A ticket checkout with a .git and one real commit, so ensure_ticket takes the +# refresh path (never a clone), resolves its branch cleanly, and the refresh is a +# local no-op against itself - the suite needs no network. +TICKET="$T/ticket" +mkdir -p "$TICKET" +git init -q "$TICKET" +git -C "$TICKET" -c user.email=t@t -c user.name=t commit -q --allow-empty -m ticket +git -C "$TICKET" remote add origin "$TICKET" 2>/dev/null || true + +# run_status