From aa3391a93ec86e8d4fa5bca9ead22fe4b4588ce3 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Fri, 21 Aug 2026 07:34:08 +0000 Subject: [PATCH] feat(dirty-content-fingerprint): Makefile computes dev-loop fingerprint Adds an independent DIRTY_FINGERPRINT computation to src/frontend/Makefile, mirroring xce-build.sh's task 2.1 algorithm exactly, wired into VERSION_LDFLAGS as -X main.DirtyFingerprint=. GIT_COMMIT/GIT_STATUS/ BUILD_TIME (including GIT_COMMIT's own -dirty suffix) are completely untouched. This took two rejected review rounds to get the failure-detection right: round 1 found a `{ git diff HEAD; ...; } >file` grouped command whose exit status only reflected the last command, silently swallowing a git diff failure and fabricating a hash from incomplete data. Round 2 found the identical bug class one step further down (git ls-files | sort | xargs cat's pipe swallowing an ls-files failure). Fixed by pulling git ls-files out of the pipe into its own directly-redirected, individually-checked step, deliberately chosen over `set -o pipefail;` (which also worked) because this repo's own build-deployment.md documents a past incident of code silently relying on /bin/sh happening to be bash with no SHELL:=bash pin. One narrow, explicitly-disclosed residual gap remains (a sort-only failure, not cat, in the trailing pipe) and was assessed as acceptable low risk after independent verification across three review rounds. Co-Authored-By: Claude Sonnet 5 --- .../specs/dirty-content-fingerprint/tasks.md | 12 +- src/frontend/Makefile | 110 +++++++++++++++++- 2 files changed, 117 insertions(+), 5 deletions(-) diff --git a/.kiro/specs/dirty-content-fingerprint/tasks.md b/.kiro/specs/dirty-content-fingerprint/tasks.md index d3141d2f..587fcbff 100644 --- a/.kiro/specs/dirty-content-fingerprint/tasks.md +++ b/.kiro/specs/dirty-content-fingerprint/tasks.md @@ -54,10 +54,11 @@ - _Boundary: apiserver/meson.build, xcecli/meson.build_ - _Depends: 4.1_ -- [ ] 4.3 (P) Makefile dev-loop path computes the same fingerprint, independently of GIT_COMMIT - - `src/frontend/Makefile`'s existing `GIT_COMMIT`/`GIT_STATUS` computation (original feature, task 4.3, including its own `-dirty` suffix) is **not touched** - - Compute a `DIRTY_FINGERPRINT` value directly (git diff + untracked files, dev-loop path, no meson involved) using the identical logic as task 2.1 (empty when clean or on computation failure, real hash otherwise), and pass it via the identical `-ldflags -X main.DirtyFingerprint=` shape - - Observable: `go version -m` on a Makefile-built binary shows the same fingerprint format/value as a meson-built binary from the identical tree state; `GIT_COMMIT`'s own `-dirty` behavior is unaffected +- [x] 4.3 (P) Makefile dev-loop path computes the same fingerprint, independently of GIT_COMMIT + - `GIT_COMMIT`/`GIT_STATUS`/`BUILD_TIME` (and `GIT_COMMIT`'s own `-dirty` suffix) confirmed byte-for-byte untouched throughout two rounds of adversarial remediation + - `DIRTY_FINGERPRINT` computed independently, mirroring task 2.1's algorithm exactly (cross-checked byte-identical on the real dirty tree); failure-detection required two remediation rounds to get right -- round 1 fixed a `{ git diff HEAD; ...; } >file` grouped command silently swallowing a `git diff HEAD` failure (only the last command's exit status was checked); round 2 found the SAME class of bug one step further down (`git ls-files | sort | xargs cat`'s pipe also swallowing an `ls-files` failure) and fixed it by pulling `git ls-files` out of the pipe entirely into its own directly-redirected, individually-checked step -- deliberately choosing this over a `set -o pipefail;` prefix (which empirically also worked) because this repo's own `build-deployment.md` documents a past incident of code silently relying on `/bin/sh` happening to be bash with no `SHELL:=bash` pin + - One narrow, explicitly-disclosed residual gap remains and was assessed as acceptable: a `sort`-only failure (not `cat`) in the trailing `sort -z | xargs -0 -r cat --` pipe can still be masked -- independently confirmed present, low-risk, and accurately scoped, not grounds for further rejection + - Observable confirmed live (independently re-verified across 3 review rounds, real builds + two independently-reproduced failure-injection wrappers with control tests proving each wrapper genuinely triggers the old bug): `make -n build-cli`'s fingerprint matches a standalone reference computation byte-for-byte; both the `git diff`-fails and `git ls-files`-fails scenarios correctly degrade to empty, not a fabricated hash; `GIT_COMMIT`'s `-dirty` behavior unaffected throughout - _Requirements: 1.1, 1.2, 1.3, 1.4, 5.1_ - _Boundary: Makefile_ - _Depends: 4.1_ @@ -111,3 +112,6 @@ - Observable: the fingerprint values from two builds of the same commit with different tracked-file edits are different, and no `--impure` flag or `git` invocation inside the sandbox was required - _Requirements: 1.1, 1.2, 1.3, 5.1_ - _Depends: 2.2_ + +## Implementation Notes +- **Task 4.3**: a `{ cmd1; cmd2; } >file` or `cmd1 | cmd2 | cmd3` shell construct's exit status (as checked by `if ! ... ; then ...; fi`) only reflects the LAST command's exit status, not any earlier command's -- this silently swallows an earlier failure and can produce fabricated output from partial/incomplete data instead of the intended graceful-degradation path. Took two rejected review rounds to fully generalize (first fixed for `git diff HEAD`, then found again one step later for `git ls-files | sort | xargs`). When writing any multi-stage shell pipeline whose failure must be individually detectable, either enable `pipefail` explicitly (verify the actual invoking shell first -- `$(shell ...)` in Make does NOT inherit a Makefile's `SHELL`/`.SHELLFLAGS` settings) or restructure each stage to its own directly-redirected, individually-`$?`-checked step (the safer default absent a `SHELL:=bash`-style pin, per this repo's own documented `/bin/sh`-is-not-guaranteed-bash incident in `build-deployment.md`). diff --git a/src/frontend/Makefile b/src/frontend/Makefile index 29c6bf9f..0b44e3c4 100644 --- a/src/frontend/Makefile +++ b/src/frontend/Makefile @@ -31,7 +31,115 @@ else GIT_COMMIT := $(GIT_COMMIT_HASH)-dirty endif BUILD_TIME := $(shell date -u +%Y-%m-%dT%H:%M:%SZ) -VERSION_LDFLAGS := -ldflags "-X main.GitCommit=$(GIT_COMMIT) -X main.BuildTime=$(BUILD_TIME)" + +# dirty-content-fingerprint task 4.3: independent DIRTY_FINGERPRINT +# computation for this same dev-loop -ldflags path -- a NEW, sibling value +# alongside GIT_COMMIT_HASH/GIT_STATUS/GIT_COMMIT/BUILD_TIME above, which +# this block does not read or modify. Mirrors xce-build.sh's (task 2.1) +# dirty_fingerprint algorithm exactly: empty when the tree is clean, or when +# the hash computation itself fails (never fabricated -- Requirement 5.1); +# otherwise a sha256 prefix (8 hex chars, matching xce-build.sh's +# `cut -c1-8`) over `git diff HEAD` concatenated with the sorted raw content +# of every file `git ls-files -z --others --exclude-standard` lists. +# +# DIRTY_FP_SRC_DIR mirrors xce-build.sh's own SRC_DIR (its script's parent +# directory, i.e. src/ -- one level above this Makefile's src/frontend/): +# `git status --porcelain`/`git diff HEAD` are repo-wide with root-relative +# path display regardless of invocation cwd, but a plain, pathspec-less +# `git ls-files --others` is genuinely SCOPED to (and shows paths relative +# to) the invocation cwd -- confirmed empirically. Running every git call +# from DIRTY_FP_SRC_DIR (via `cd`, not just `git -C`, since the untracked +# filenames `git ls-files` prints must also resolve on disk relative to that +# same directory for `cat` to read them) reproduces xce-build.sh's exact +# untracked-file scope regardless of where `make` itself is invoked from -- +# required for this task's Observable ("same fingerprint ... as a +# meson-built binary from the identical tree state"). +# +# Uses `git ls-files -z ... | sort -z | xargs -0 -r cat --` instead of +# xce-build.sh's bash `while IFS= read -r -d '' f; do cat -- "$f"; done` +# loop -- both concatenate the same sorted files' raw bytes in the same +# order (verified byte-identical), but this form needs no embedded +# single-quote characters (bash's `-d ''`), which would otherwise be +# fragile to escape inside a Make `$(shell ...)` argument. A real mktemp +# scratch file (not command-substitution capture) collects the combined +# diff+file bytes before hashing, so no trailing-newline stripping skews +# the digest versus xce-build.sh's own temp-file-based approach. Zero +# working-tree/index mutation -- reads only. +# +# `git diff HEAD` and the ls-files/sort/xargs step are gated as SEPARATE, +# individually-checked steps (fp_ok, mirroring xce-build.sh's own fp_ok +# tracking) rather than one `{ cmd1; cmd2; } >"$$fp_tmp"` group -- a POSIX +# sequence/group's exit status reflects only its LAST command, so a grouped +# form would silently swallow a `git diff HEAD` failure whenever the +# ls-files/sort/xargs step still succeeded, hashing incomplete data into a +# fabricated non-empty fingerprint instead of degrading to empty +# (Requirement 5.1). +# +# round-2 remediation: `git ls-files -z --others --exclude-standard` is ALSO +# pulled out of any pipe and run as its own directly-redirected command into +# a second temp file (list_tmp), with its OWN individually-checked `$?` -- +# it is no longer the first stage of a 3-stage +# `ls-files | sort -z | xargs -0 -r cat --` pipe. That pipe had the exact +# same class of bug as the grouped-form one above: a POSIX pipe's exit +# status reflects only its LAST stage, and `sort -z`/`xargs -0 -r` both +# exit 0 on the empty stdin a failed `git ls-files` produces, so a +# `git ls-files` failure was silently swallowed and fp_tmp was hashed with +# only the `git diff HEAD` bytes in it -- a real but fabricated/incomplete +# fingerprint, not the required empty fallback. +# +# Two options were considered: (a) prefix this whole $(shell ...) body with +# `set -o pipefail;`, which empirically works in THIS repo's dev shell +# (GNU Make's $(shell) here runs under /bin/sh, itself a symlink into the +# nix store's bash-interactive package -- confirmed via `ls -la /bin/sh`), +# or (b) the restructuring below, which sidesteps pipe-exit propagation +# entirely instead of depending on it. (b) was chosen: this same repo's own +# `.kiro/steering/build-deployment.md` documents this exact codebase having +# been bitten before by code that silently depended on "/bin/sh happens to +# be symlinked to bash" (deploy-ha.sh's shebang was found locally altered to +# `#!/bin/sh` while still using bash-only syntax, and only kept working by +# that coincidence) -- its own fix was to stop relying on that coincidence +# rather than to keep depending on it, and this Makefile does not pin +# `SHELL := bash` to turn the coincidence into a guarantee. Since $(shell) +# here has no such pin either, (a)'s `pipefail` would rely on the identical +# unpinned coincidence for a build-portability property (dirty-fingerprint +# accuracy) rather than a dev-loop-only convenience script, so (b) is the +# more conservative choice. `git`, the highest-risk command (failure here +# is exactly the case under test), is now entirely outside any pipe. The +# residual `LC_ALL=C sort -z <"$$list_tmp" | xargs -0 -r cat --` pipe keeps +# a narrower gap -- a `sort` failure specifically could still be masked by +# `xargs`'s own (last-stage) exit status -- but `xargs -r cat --` DOES +# propagate an individual `cat` failure (e.g. an unreadable file `git +# ls-files` listed): GNU xargs exits non-zero (123) whenever any invoked +# command exits 1-125, so that sub-case is still caught without pipefail. +# This matches the reviewer's own assessment: a `sort` failure is a far +# lower-risk residual than the `git` failure this fix targets. +DIRTY_FP_SRC_DIR := $(CURDIR)/.. +DIRTY_FINGERPRINT := $(shell \ + cd "$(DIRTY_FP_SRC_DIR)" 2>/dev/null && \ + if [ -n "$$(git status --porcelain 2>/dev/null)" ]; then \ + fp_tmp=$$(mktemp 2>/dev/null); \ + if [ -n "$$fp_tmp" ]; then \ + fp_ok=1; \ + list_tmp=""; \ + if ! git diff HEAD >"$$fp_tmp" 2>/dev/null; then fp_ok=0; fi; \ + if [ "$$fp_ok" = 1 ]; then \ + list_tmp=$$(mktemp 2>/dev/null); \ + if [ -z "$$list_tmp" ]; then \ + fp_ok=0; \ + elif ! git ls-files -z --others --exclude-standard >"$$list_tmp" 2>/dev/null; then \ + fp_ok=0; \ + fi; \ + fi; \ + if [ "$$fp_ok" = 1 ] && ! LC_ALL=C sort -z <"$$list_tmp" | xargs -0 -r cat -- >>"$$fp_tmp" 2>/dev/null; then fp_ok=0; fi; \ + if [ "$$fp_ok" = 1 ]; then \ + fp=$$(sha256sum <"$$fp_tmp" 2>/dev/null | cut -c1-8); \ + if [ -n "$$fp" ]; then printf "%s" "$$fp"; fi; \ + fi; \ + rm -f "$$fp_tmp" "$$list_tmp" 2>/dev/null; \ + fi; \ + fi) + +VERSION_LDFLAGS := -ldflags "-X main.GitCommit=$(GIT_COMMIT) -X main.BuildTime=$(BUILD_TIME) -X main.DirtyFingerprint=$(DIRTY_FINGERPRINT)" # bug-1558: test-backend's CGO_CFLAGS/CGO_LDFLAGS, self-derived via # pkg-config so `make test-backend` works standalone instead of only inside