Skip to content

Stamp Each Host With What It Carries, and Reject the Flags the Installer Ignored - #665

Merged
ptr727 merged 12 commits into
developfrom
feature/host-stamp
Aug 11, 2026
Merged

Stamp Each Host With What It Carries, and Reject the Flags the Installer Ignored#665
ptr727 merged 12 commits into
developfrom
feature/host-stamp

Conversation

@ptr727

Copy link
Copy Markdown
Owner

Rolling the safety kit across a fleet needs an answer to "is this machine current", and there was none. #365 tracks machines by tick, a tick records that someone ran something once, and the kit has changed three times since the earliest one. Its own body admits this: "a machine ticked below is not necessarily current." Verifying meant typing three grep commands per host and reading them by eye.

The stamp

Every install writes ~/.claude/agent-safety-stamp.json and prints one pasteable line:

server-2 | Debian GNU/Linux 13 (trixie) | hub 31880f0 | payload a1ea65e4e604308f | agent-safety v1, fleet-bootstrap v1 | 2026-08-11T00:48:58Z

Host name, host type, the hub commit it came from, a digest of the bytes installed, the marker versions actually found in CLAUDE.md, and when.

--report

Read-only. Run it from a fresh hub checkout and it compares that machine against that checkout:

  • CURRENT — matches, exit 0
  • STALE — with the specific reasons, exit 1
  • NOT INSTALLED — no stamp, exit 2

It returns before creating anything, so reporting on a clean machine leaves it clean.

Three judgement calls worth stating

Compared on the payload digest, not the commit. A machine installed from an older commit whose kit bytes never changed is current, and calling it stale sends someone to re-run an installer that would write the same file byte for byte.

The digest covers the installed bytes, not the tree state. A clean commit and a dirty checkout install different content under the same SHA. A dirty install is recorded as dirty, because a stamp that claims a commit identifies the bytes when it does not is the thing this exists to prevent.

Blocks are read back off disk. Not assumed from what the installer meant to write. A block edited or deleted by hand since the install reports stale, and a start marker without its end does not count as present — the half-written case a presence check reads as success.

A defect fixed in passing

main() took no arguments while both wrappers pass "$@" through, so every flag was silently discarded. install.py --help performed a full install instead of printing usage. It now parses, and an unknown flag exits 2 having changed nothing — asserted by comparing the stamp before and after.

Verification

test_install.py, 13 cases, each proving a verdict by reintroducing the state it reports:

  • Report on a never-installed machine says so and creates nothing
  • A changed payload reports stale; a hand-deleted block reports stale
  • The printed remedy actually clears it — re-running returns CURRENT, so the verdict is not a dead end
  • A half-written block is not counted as present
  • --bogus is rejected with the stamp unchanged; --help prints usage and installs nothing
  • Every file the kit deploys is in the digest, checked in both directions: changing each listed file moves the digest, and every file install.py reads is in the list

All cases run against a throwaway CLAUDE_HOME, never the invoking user's.

Wired into the self-test step, because a test that runs nowhere is the defect #664 just found in another gate.

Merge order

This touches the same self-test block as #664. Whichever merges second needs a one-line rebase in that list — I will handle it.

Follow-up not in scope here: the issue body still carries the manual grep instructions. I will rewrite it to the one-command-per-platform form once this lands, so the instructions describe a flag that exists.

…ler Ignored
Rolling the safety kit to a fleet needs an answer to "is this machine
current", and there was none. The checklist in #365 tracked machines by
tick, a tick records that someone ran something once, and the kit has
changed three times since the earliest one. Verification meant typing
three grep commands per host and reading them by eye.
Each run now writes ~/.claude/agent-safety-stamp.json naming the host, the
hub commit it installed from, a digest of the bytes it installed, and the
marker versions actually found in CLAUDE.md afterwards. It prints the same
thing as one pasteable line.
`--report` answers the question read-only, from a fresh hub checkout,
comparing that machine against that checkout: CURRENT, STALE with the
reasons, or NOT INSTALLED. It returns before creating anything, so a
report on a clean machine leaves it clean.
The comparison is on the payload digest rather than the commit. A machine
installed from an older commit whose kit bytes never changed is current,
and calling it stale sends someone to re-run an installer that would write
the same file. The digest is taken over the installed bytes for the same
reason: a clean commit and a dirty checkout install different content
under the same SHA, and a dirty install is recorded as such.
Blocks are read back off disk rather than assumed from what was written,
so a block edited or deleted by hand since the install reports stale. A
start marker without its end does not count as present, which is the
half-written case a presence check reads as success.
`main()` also took no arguments while both wrappers passed `"$@"` through,
so every flag was silently discarded and `install.py --help` performed a
full install instead of printing usage. It now parses, and an unknown flag
exits 2 having changed nothing.
test_install.py proves each verdict by reintroducing the state it reports,
including that the printed remedy clears it, and that every file the kit
deploys is covered by the digest. Wired into the self-test step, since a
test that runs nowhere is the defect this repo just found in another gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI lite review requested due to automatic review settings August 11, 2026 00:53
`test_install.py` carries a shebang, and `.gitattributes` pins each
shebang-executable `.py` by path rather than by glob, so a new one is
unspecified to git until it is listed. The repo gate's shebang floor
caught it.
`.editorconfig` already covered it, matching `host-setup/agent-safety/*.py`
as a glob. The two files disagreeing that way is why the gate exists: the
editor would have written LF while git enforced nothing.
The comment above the block enumerates what the pins cover and said "the
agent-safety hook and its installer", which stopped being the whole list
the moment the installer gained tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an install-time “stamp” and a read-only --report mode to the host safety kit so operators can quickly determine whether a machine matches a given hub checkout, and fixes argument parsing so installer flags are no longer silently ignored.

Changes:

  • Add stamp generation (~/.claude/agent-safety-stamp.json) and a pasteable one-line summary, plus --report verdicts and exit codes.
  • Add a standard-library self-test suite (test_install.py) covering report verdicts, argument handling, and stamp/digest invariants.
  • Run the new installer self-tests in the validation workflow.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

FileDescription
host-setup/agent-safety/test_install.pyAdds self-tests that exercise --report, stamping, digest coverage, and CLI flag behavior.
host-setup/agent-safety/install.pyImplements stamping, --report, payload digesting, and proper CLI parsing.
.github/workflows/validate-task.ymlRuns the new installer self-test in CI.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadhost-setup/agent-safety/install.py Outdated
Comment threadhost-setup/agent-safety/install.py Outdated
CopilotAI review requested due to automatic review settings August 11, 2026 00:56

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

host-setup/agent-safety/install.py:120

  • source_ref() assumes the git executable is available; if it is missing (common on minimal hosts or extracted tarball installs), subprocess.run(["git", ...]) raises FileNotFoundError and the installer/report crashes. Handle the missing-binary case and treat it as vcs: none so the stamp/report still work.
 def git(*args):
r = subprocess.run(["git", "-C", str(HERE), *args], capture_output=True, text=True)
return r.stdout.strip() if r.returncode == 0 else None

host-setup/agent-safety/install.py:208

  • report() calls stamp_line(stamp) immediately after parsing JSON. A stamp file can be valid JSON but missing required keys (manual edit, partial write, old/experimental format), which currently triggers a KeyError and crashes instead of returning a controlled "stamp unreadable" verdict. Validate required fields (or catch KeyError/TypeError) and return exit 2 with a clear message.
 print(f"This machine: {stamp_line(stamp)}")

Four review findings, all real.
`--report` compared marker versions and the source payload, and never what
is actually on the machine. A block edited between its own markers left the
version untouched and reported CURRENT, which is most of what "has someone
weakened this by hand" means. The deployed hook is not marker-delimited at
all, so a modified or deleted one was invisible the same way.
The installed bytes are now digested and compared against what this
checkout would write: the hook, and each block as it appears in CLAUDE.md.
Line endings are normalized first, so a machine holding identical text with
CRLF is current rather than drifted.
`blocks_present` accepted any equal number of start and end markers, so a
duplicated block reported present and named the first version while the
second silently governed. It now requires exactly one pair.
`source_ref` let FileNotFoundError escape when git is absent, crashing both
the install and the read-only report on exactly the minimal host a tarball
install targets. It records `vcs: none` instead.
`report` read a parsed stamp straight into `stamp_line`, so a hand-edited
or partially written file raised KeyError rather than returning a verdict.
Required keys are checked first.
Ten cases added. Nine fail against the previous code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings August 11, 2026 01:04
The comment above the self-test step read "Each gate in scripts/", which
stopped being the whole list when the write-guard self-test joined it and
is wronger again with the installer's tests.
The claim worth keeping is that each gate is proven by a case that
reintroduces the fault it catches, which holds wherever the gate lives.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ptr727

Copy link
Copy Markdown
OwnerAuthor

Answering the two suppressed findings here, since they carry no thread to reply on. Both were correct and both are fixed in 6583586.

install.py:120source_ref() crashes when git is absent.

subprocess.run(["git", ...]) raises FileNotFoundError and the installer/report crashes. Handle the missing-binary case and treat it as vcs: none.

Correct, and it fails on exactly the host this is most likely to meet. A tarball install on a minimal box is the normal case for a bootstrap, not an exotic one — and --report, which is documented as read-only and safe, would have crashed there too.

The inner git() helper now catches OSError and returns None, so the stamp records vcs: none and both paths still work. Tested by running the installer with PATH="", which asserts the stamp is written and carries vcs: none. It fails against the previous code.

install.py:208report() reads a parsed stamp straight into stamp_line().

A stamp file can be valid JSON but missing required keys (manual edit, partial write, old/experimental format), which currently triggers a KeyError.

Also correct, and it defeated the surrounding intent: the json.JSONDecodeError guard immediately above returns a controlled exit 2, and then the very next line assumed a shape. Valid JSON is not a usable stamp.

Required keys are now checked before the stamp is read, returning exit 2 naming what is missing. Two cases added — a stamp with keys missing, and one holding a non-object at its root — each asserting exit 2 and no traceback.

Totals for this round: ten cases added across the four findings, nine of which fail against the previous code. The tenth is the CRLF equivalence check, which guards a regression rather than reproducing a defect, and I would rather say that than count it as ten.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

host-setup/agent-safety/install.py:148

  • payload_digest() hashes the raw bytes of claude-md-*.md, but the installer embeds those snippets via read_text(...).strip() (and preserves CLAUDE.md line endings). That means a change that is stripped away (e.g., trailing newlines/whitespace) will flip the payload digest and report STALE even though a reinstall would write identical on-disk content. Compute the digest over the normalized bytes that would actually be installed (same normalization used for expected_installed_digest()/install write path).
def payload_digest():
"""One digest over the bytes this kit installs, in a fixed order.
Fixed order because a set of files has none, and a digest that depends on directory listing
order reports drift on a machine where nothing changed.
"""
h = hashlib.sha256()
for name in PAYLOAD_FILES:
h.update((HERE / name).read_bytes())
return h.hexdigest()[:16]

host-setup/agent-safety/test_install.py:242

  • test_the_digest_covers_every_file_the_kit_installs mutates each payload file by appending only a newline. For the snippet files, the installer reads them with .strip() before embedding, so a trailing newline is not necessarily an installed-byte change. Prefer appending a non-whitespace sentinel so the test remains aligned with the digest representing installed bytes.
 try:
target.write_bytes(original + b"\n")
self.assertNotEqual(install.payload_digest(), baseline,
f"{name} is in PAYLOAD_FILES but changing it did not move the digest")

CopilotAI review requested due to automatic review settings August 11, 2026 01:08

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

host-setup/agent-safety/install.py:148

  • payload_digest() hashes the raw bytes of claude-md-*.md, but the installer reads those snippets via read_text(...).strip() (universal-newline normalized) before writing them. That means a source-only change that does not affect what gets installed (for example, adding/removing trailing newlines/whitespace that strip() removes) can still change payloadDigest and cause --report to print STALE even when the installed content matches what this checkout would write. Consider computing payloadDigest over the effective deployed content (same normalization/strip logic as the installer / expected_installed_digest()), so it only changes when what would be installed changes.
 h = hashlib.sha256()
for name in PAYLOAD_FILES:
h.update((HERE / name).read_bytes())
return h.hexdigest()[:16]

host-setup/agent-safety/install.py:261

  • --report aims to return a verdict rather than crash on a hand-edited / corrupted stamp, but the current validation only checks for presence of top-level keys. If the stamp is a JSON object with the required keys but unexpected types (for example, "source": null or "host": "..."), stamp_line(stamp) (and later stamp.get("source", {}).get("dirty")) can raise and turn the report into a traceback. It would be safer to validate the nested shapes/types before calling stamp_line() / accessing nested .get().
 # Valid JSON is not a usable stamp: a hand edit or an older format parses and then breaks the read.
missing = [k for k in STAMP_REQUIRED if k not in stamp] if isinstance(stamp, dict) else ["everything"]
if missing:
sys.stderr.write(f"Stamp at {path} is missing {', '.join(missing)}. "
"Re-run the installer to rewrite it.\n")

…ping
Two more review findings, and the first exposed a redundancy worth removing.
`payload_digest` hashed raw file bytes while the installer embeds each
snippet with `.strip()`. A trailing newline therefore moved the digest
while the installed block stayed identical, so `--report` said STALE and
sent the operator to re-run an installer that would write the same file.
A verdict whose remedy changes nothing is worse than no verdict.
It now normalizes the way the installer writes: line endings first, then
stripping for the snippets and not for the hook, which is copied byte for
byte.
That makes it identical to `expected_installed_digest`, added an hour ago
for the installed-content comparison, so that function is gone and the one
digest serves both. The order already matched, being the hook and then each
block, which is why the two were the same function written twice.
`test_the_digest_covers_every_file_the_kit_installs` appended a bare
newline, which under the fix is correctly no change at all, so the test
would have asserted the wrong thing. It appends a non-whitespace sentinel
now.
Two cases added around the boundary: trailing whitespace on a snippet is
not drift, and an edit that does reach the installed block still is.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings August 11, 2026 01:12

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

host-setup/agent-safety/install.py:266

  • --report validates only that required keys exist, but not that nested fields have the expected types. A hand-edited or corrupted stamp like {..., "source": "git"} will pass the STAMP_REQUIRED key check and then crash in stamp_line() (src.get(...)), contradicting the goal of returning a verdict instead of a traceback for malformed stamps.
 missing = [k for k in STAMP_REQUIRED if k not in stamp] if isinstance(stamp, dict) else ["everything"]
if missing:
sys.stderr.write(f"Stamp at {path} is missing {', '.join(missing)}. "
"Re-run the installer to rewrite it.\n")
return 2
print(f"This machine: {stamp_line(stamp)}")
# The stamp says what was installed; the machine says what is there now.

The key check added last round passed `"source": "git"` and then raised
inside the line that formats it, which is exactly the traceback it was
added to prevent. Presence and shape are different questions, and a hand
edit produces the second failure rather than the first.
`stamp_problems` checks the type held under each required key and names
every fault in reading order, so one run tells the operator all of what is
wrong rather than the first thing.
`stamp_line` is now total as well. Every read carries a fallback, so a
formatter cannot raise even on a stamp the validator would have rejected.
Belt and braces on purpose: the caller validates first, and a verdict about
a broken stamp must not itself crash.
Seven cases added, five of them subtests covering each required key holding
the wrong type, plus four shapes asserting the formatter stays printable.
All fail against the previous code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings August 11, 2026 01:22

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

host-setup/agent-safety/install.py:245

  • The stamp includes a stampVersion field (and the surrounding comment says readers need to detect shape changes), but STAMP_SHAPE/stamp_problems() never validates stampVersion. That means a future format bump could be mis-read as a valid stamp instead of yielding a clear "re-run installer" verdict.
STAMP_SHAPE = {
"host": dict,
"source": dict,
"payloadDigest": str,
"blocks": dict,

host-setup/agent-safety/install.py:305

  • --report decides CURRENT/STALE by comparing the deployed hook + CLAUDE.md blocks against this checkout, but it does not verify that settings.json still registers the PreToolUse hook and the managed permission rules. If a user later removes those entries, --report can still say CURRENT even though the safety kit is effectively inactive.
 live_installed = installed_digest(claude_home)
if live_installed is None:
problems.append("the deployed hook or CLAUDE.md is missing, so the kit is not fully installed")
elif live_installed != current:
problems.append("the installed content differs from what this checkout would write")

`--report` compared the deployed hook and the CLAUDE.md blocks and never
looked at settings.json, where the hook is registered and the permission
rules live. A machine with every byte correct and the PreToolUse entry
removed carries a complete, current, entirely inert kit, and every check
here called it CURRENT.
That is the worst verdict this tool can give, since the whole question it
answers is whether the guard is in force on this machine.
`registration_problems` now reads settings.json and reports an unregistered
hook, a hook registered more than once, a missing managed permission rule,
and a settings file that is absent or unreadable.
`stampVersion` was written into every stamp, described in its own comment
as the way a reader detects a format change, and never validated. A stamp
from another format version now says so rather than being read as valid.
Nine cases added, all failing against the previous code, including that
re-running clears an unregistered hook.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings August 11, 2026 01:27
@ptr727

Copy link
Copy Markdown
OwnerAuthor

Both suppressed findings on the head are correct and fixed in dc0451a. The second is the most serious finding on this PR.

--report decides CURRENT/STALE by comparing the deployed hook + CLAUDE.md blocks against this checkout, but it does not verify that settings.json still registers the PreToolUse hook and the managed permission rules. If a user later removes those entries, --report can still say CURRENT even though the safety kit is effectively inactive.

Correct, and it defeats the point of the feature. The installer does three things — deploy the hook, register it in settings.json, write the CLAUDE.md blocks — and the report checked two of them. A machine with every byte correct and the PreToolUse entry deleted carries a complete, current, entirely inert kit, and every check here called it CURRENT.

The question this tool exists to answer is whether the guard is in force on that machine. Correct bytes on disk are not a running guard.

registration_problems() now reads settings.json and reports:

  • the hook not registered at all, so the guard never runs
  • the hook registered more than once, so it runs repeatedly
  • a managed permission rule absent
  • settings.json missing, unreadable, or not an object

The stamp includes a stampVersion field (and the surrounding comment says readers need to detect shape changes), but STAMP_SHAPE/stamp_problems() never validates stampVersion.

Also correct, and it is the sharper kind of finding: the comment above the field promised exactly what the code did not do. A version carried and never read is a version telling nobody anything. It is now in STAMP_SHAPE as an int, and a value this installer does not write is reported rather than accepted — a mismatch is unreadable in either direction, since a newer stamp holds fields this code does not know and an older one lacks fields it reads.

Cases: nine added, all failing against the previous commit, including that re-running the installer clears an unregistered hook. Thirty-four in the file now.

On this PR overall. Eleven findings across six rounds, every one real, and three of them named holes in the central claim rather than edge cases: presence-without-content, source-bytes-without-normalization, and now bytes-without-wiring. The pattern in my own work is consistent — I checked the thing I had just built and not the thing the feature promises. I would rather that be said here than not.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

host-setup/agent-safety/install.py:338

  • report() can incorrectly return CURRENT if CLAUDE.md already contains duplicated/malformed marker blocks at the time of the original install. In that situation blocks_present() returns {}, and because the stamp is generated from that same blocks_present() result, live and stamp["blocks"] both remain empty on later runs; installed_digest() will still hash the first matching block, so the corruption can slip through as CURRENT. Treat any file that contains agent-safety/fleet-bootstrap markers but yields no valid block from blocks_present() as STALE (corrupted markers), regardless of what the stamp recorded.
 live = blocks_present(claude_home / "CLAUDE.md")
problems = []

… Install
An install onto an already-corrupted CLAUDE.md wrote the corruption into
the stamp. `blocks_present` returns nothing for a duplicated or
half-written block, the stamp records that same nothing, and every later
run found them equal and reported CURRENT. Two wrong answers agreeing read
as a match.
`marker_corruption` judges the file alone. Markers present that yield no
valid block are reported regardless of what the stamp recorded.
The installer also could not clear it. `re.sub` replaced every match with
the snippet, so a file arriving with two blocks kept two, and the remedy
the report prints was a dead end for the one case that most needs it. It
now keeps the first and drops the rest, and says how many it removed.
Five cases added, all failing against the previous code, including that a
stamp hand-edited to record no blocks cannot agree its way to a clean
verdict.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings August 11, 2026 01:33
@ptr727

Copy link
Copy Markdown
OwnerAuthor

Correct, and it is the sharpest finding on this PR. Fixed in 23e80ac.

report() can incorrectly return CURRENT if CLAUDE.md already contains duplicated/malformed marker blocks at the time of the original install. In that situation blocks_present() returns {}, and because the stamp is generated from that same blocks_present() result, live and stamp["blocks"] both remain empty on later runs.

Exactly right, and the reason it slipped past every check I added is that I built the comparison out of one function on both sides. The stamp records what blocks_present() returns and the report compares against what blocks_present() returns, so a case where that function reports nothing is a case where both sides agree perfectly. Two wrong answers agreeing reads as a match. Comparing a value against itself proves only that the reader is deterministic.

marker_corruption() now judges the file alone. Markers present that yield no valid block are reported no matter what the stamp says.

And the remedy was a dead end, which I found while fixing this. The installer used re.sub with no count, replacing every match with the snippet — so a file arriving with two blocks kept two. blocks_present() still returned nothing, the report still said re-run, and re-running produced the identical file. The one case where corruption most needs clearing was the one case the printed remedy could not clear.

It now keeps the first occurrence, drops the rest, and reports how many it removed:

CLAUDE.md -> ~/.claude/CLAUDE.md (agent-safety block updated, 1 duplicate(s) removed)

Cases: five added, all failing against the previous commit, including a stamp hand-edited to record no blocks — which cannot agree its way to a clean verdict any more. Thirty-nine in the file.

Counting honestly. Twelve findings across seven rounds on this PR, every one real. The three that mattered most all had the same shape: I verified the mechanism I had just written rather than the property the feature claims. Presence instead of content, source bytes instead of installed bytes, files instead of wiring, and now a self-comparison instead of an independent read. That is a habit rather than four accidents, and the review caught all four.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

host-setup/agent-safety/install.py:158

  • payload_digest() claims to normalize line endings the way the installer writes them, but it only normalizes CRLF (\r\n) and not bare CR (\r). Because the installer reads snippet files in text mode (universal newlines) and then writes them back with \n or \r\n, a snippet file that happens to contain CR-only line endings would install normalized content but compute a different payload digest, causing --report to incorrectly return STALE for otherwise identical installed bytes.
 for name in PAYLOAD_FILES:
raw = (HERE / name).read_bytes().replace(b"\r\n", b"\n")
# A snippet is embedded stripped, so trailing whitespace is not installed content.
# The hook is copied byte for byte, so nothing about it is stripped.
if name.endswith(".md"):
raw = raw.decode("utf-8").strip().encode("utf-8")
h.update(raw)

`payload_digest` reduced CRLF and left a bare CR alone, while the installer
reads a snippet in text mode, where a bare CR arrives as a newline and
installs as one. A snippet carrying CR-only endings therefore installed
normalized content and hashed to something else, and the machine was
reported STALE against its own content.
Four sites were doing this replacement by hand and two of them disagreed,
which is the arrangement that produced the defect. They all call one helper
now, so the two digests cannot normalize differently.
Two cases added. The first builds the CR-only variant from the normalized
form rather than by replacing newlines in the file, because these snippets
are CRLF in this repository and a blind replace produces a doubled CR
rather than a bare one. My first version of that test made exactly that
mistake and failed against the fixed code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings August 11, 2026 01:40
@ptr727

Copy link
Copy Markdown
OwnerAuthor

Correct, and fixed in 6894146.

payload_digest() claims to normalize line endings the way the installer writes them, but it only normalizes CRLF (\r\n) and not bare CR (\r).

Right, and the claim in the docstring is the part that made it worse: it said "normalized the way the installer writes it" while doing something narrower. The installer reads snippets in text mode, so a bare CR arrives as a newline and installs as one, and a digest that left it alone reported the machine STALE against content it had itself just written.

The underlying cause was four hand-written normalizations across the file, two of which disagreed. They now go through one normalized() helper, so the two sides of the comparison cannot diverge again:

payload_digest -> normalized(read_bytes())
installed_digest -> normalized(hook bytes), normalized(CLAUDE.md text)
the install write -> normalized(existing)

Two cases added, and the first one caught me. My initial version built the CR-only variant with original.replace(b"\n", b"\r") and it failed against the fixed code. The reason is worth recording: these snippet files are CRLF in this repository, so a blind newline replace turns every \r\n into \r\r rather than into a bare CR. The test was wrong, not the fix. It now builds the variant from the normalized form.

The second case asserts the helper directly over CRLF, bare CR, and LF in both bytes and text, since a helper two digests depend on is worth pinning rather than inferring.

Thirteen findings, eight rounds, all real. This one belongs to the same family as the rest: I wrote a docstring describing the property I wanted and then implemented a weaker one, and nothing compared the two. Forty-one cases in the file now.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

host-setup/agent-safety/install.py:135

  • source_ref() claims to report whether the checkout is dirty, but the git status call only checks PAYLOAD_FILES. If install.py itself is modified (which can change the installed bytes/behavior), dirty will incorrectly remain false, and the stamp can imply the recorded commit identifies what ran when it does not.
 status = git("status", "--porcelain", "--", *PAYLOAD_FILES)
ref["dirty"] = bool(status)

host-setup/agent-safety/install.py:358

  • report() reads the stamp with path.read_text(encoding="utf-8"), which can raise UnicodeDecodeError if the stamp file is corrupted or partially written with non-UTF-8 bytes. That exception is not caught, so --report can crash with a traceback instead of returning the intended exit code 2.
 try:
stamp = json.loads(path.read_text(encoding="utf-8"))
except (json.JSONDecodeError, OSError) as e:
sys.stderr.write(f"Stamp at {path} is unreadable ({e}). Re-run the installer to rewrite it.\n")
return 2

`report` and `registration_problems` each caught JSONDecodeError and
OSError, and a file holding bytes no decoder accepts raises
UnicodeDecodeError before the JSON parser is reached. A partially written
stamp therefore crashed the read-only report with a traceback, which is
what those handlers exist to prevent.
Both catch ValueError now, which covers JSONDecodeError and
UnicodeDecodeError alike, rather than naming the subclass and missing its
sibling.
Two cases added, one per call site, both failing against the previous code.
The settings one is included because the finding named only the stamp and
the same shape was two hundred lines above it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ptr727

Copy link
Copy Markdown
OwnerAuthor

Correct, and fixed in 3894a87.

report() reads the stamp with path.read_text(encoding="utf-8"), which can raise UnicodeDecodeError if the stamp file is corrupted or partially written with non-UTF-8 bytes. That exception is not caught.

Right. UnicodeDecodeError is a ValueError, not an OSError, and it is raised by the decode before the JSON parser is ever reached — so the handler names the subclass it thought of and misses its sibling. A partially written stamp crashed the read-only report with a traceback, which is precisely what that handler exists to prevent.

Both reads catch ValueError now, which covers JSONDecodeError and UnicodeDecodeError alike.

Two call sites, not one. You named the stamp read; registration_problems() two hundred lines above it reads settings.json with the identical except (json.JSONDecodeError, OSError), and I only added that function three commits ago. I fixed both rather than only the one reported, since a finding names a sample and not the population.

Two cases added, one per site, both failing against the previous code.

Fourteen findings, nine rounds, all real. Four of them have now been the same mistake at different depths: I write a guard for the failure I pictured and it does not cover the failure's siblings. Presence not content, source bytes not installed bytes, files not wiring, CRLF not bare CR, and now one exception subclass not the other.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

host-setup/agent-safety/install.py:586

  • The post-install guidance says to run the check "from a fresh hub checkout", but the printed command hard-codes this checkout's absolute path via HERE. That makes the pasteable line easy to reuse incorrectly later (running against an old checkout) and it doesn't match the intent that --report compares against whichever checkout you run it from.
 print(f" stamp -> {stamp_path}")

CopilotAI review requested due to automatic review settings August 11, 2026 01:47

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

host-setup/agent-safety/test_install.py:474

  • test_every_deployed_file_is_in_the_digest doesn’t actually check the deployed snippet files. The regex only matches literal HERE / "..." usages (currently just gh-write-guard.py), while the snippet filenames are referenced via the blocks = [(..., "claude-md-*.md"), ...] list. This makes the test a false-positive: adding a new deployed snippet (or forgetting to add one to PAYLOAD_FILES) could slip through.
 for name in re.findall(r'HERE / "([^"]+\.(?:py|md))"', source):
if name == "install.py":
continue
self.assertIn(name, install.PAYLOAD_FILES,
f"install.py reads {name} but PAYLOAD_FILES omits it, so the digest misses it")

…Escape the Digest
`test_every_deployed_file_is_in_the_digest` matched only literal
`HERE / "..."` reads, which is the hook and nothing else. The snippet names
came from a list inside `main`, so the test never saw them and a new
snippet would have passed while being absent from the digest. It asserted
coverage and measured one file.
The structural fix rather than a wider regex: `CLAUDE_MD_BLOCKS` is a
module constant and `PAYLOAD_FILES` derives from it, so adding a block
enters the digest with no second edit. Two lists maintained by hand are two
lists that drift.
Three readers each carried their own copy of the marker pair, which is the
same hazard one level down, and all three now read `BLOCK_MARKERS`.
Three cases: the scan covers both sources and asserts it matched more than
the hook, the derivation holds, and no reader carries its own pair.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings August 11, 2026 01:54
@ptr727

Copy link
Copy Markdown
OwnerAuthor

Correct, and this is the best finding on the PR because it is a finding about a test — the one place a false positive is invisible by construction. Fixed in 25cf480.

test_every_deployed_file_is_in_the_digest doesn't actually check the deployed snippet files. The regex only matches literal HERE / "..." usages (currently just gh-write-guard.py), while the snippet filenames are referenced via the blocks = [...] list.

Exactly right. The test asserted coverage over three files and measured one. It was green, it was named for the property, and it verified almost none of it — and I wrote it specifically to catch a file being left out of the digest.

I did not just widen the regex. A wider pattern is another thing to keep in step with the code. The gap is structural, so the fix is:

CLAUDE_MD_BLOCKS= (
("agent-safety", "claude-md-safety.md"),
("fleet-bootstrap", "claude-md-fleet.md"),
)
PAYLOAD_FILES= ("gh-write-guard.py",) +tuple(filenamefor_, filenameinCLAUDE_MD_BLOCKS)

Adding a block now enters the digest with no second edit, so the omission the test was watching for cannot be made. Verified by appending a fourth block and confirming it derives into the payload list.

Same hazard one level down, which the finding did not name. Three readers — blocks_present, installed_digest, marker_corruption — each carried their own literal ("agent-safety", "fleet-bootstrap"). A new block would have reached whichever ones I remembered to edit. All three read BLOCK_MARKERS now, and a case asserts that literal appears nowhere in the file.

Three cases: the scan reads both sources and asserts it matched more than the hook, the derivation holds, and no reader carries its own pair.

Fifteen findings, ten rounds. This one changes how I read the rest: several of my other cases in this file are the same shape as the one you caught, in that they assert a property and exercise a mechanism. The derivation is worth more than any of them, because it removes the property from the set of things a test has to watch.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@ptr727
ptr727 merged commit e2a99f1 into developAug 11, 2026
7 checks passed
@ptr727
ptr727 deleted the feature/host-stamp branch August 11, 2026 02:11
ptr727 added a commit that referenced this pull request Aug 11, 2026
…ng (#670)
Five commits, `1927e9a..56f4d7d`. Nineteen files, +4241/-136.
## What lands
- **#664** `b0d0d13` — a shellcheck gate in `validate-task.yml`, with
the file list from `git ls-files '*.sh'` so a new script is gated
without editing the step. Also wired `scripts/test_host_gate.py` into
the self-test step, which was running in no workflow at all.
- **#666** `8c6fd27` — `prose_lint.py` chose its rule set from the
working directory rather than the scanned repository, so standing in an
operational repo and scanning a release repo silently discarded
`home-path`, the rule that exists because real paths reached a public
comment.
- **#665** `e2a99f1` — a host stamp at
`~/.claude/agent-safety-stamp.json` plus `--report`, so "is this machine
current" has an answer that is not a tick in an issue. Also fixed
`install.py` taking no arguments while both wrappers passed `"$@"`,
which made `--help` perform a full install.
- **#668** `6864a9b` — the two remaining `prose_lint.py` false cleans,
fixed as a class. An absolute path argument scoped a `--diff` run to
nothing and exited 0, and an untracked file was invisible to both a
diff-scoped run and a whole-tree sweep. Every input to a verdict now
derives from the repository being scanned, and every run states the
scope it read.
- **#667** `56f4d7d` — the host bootstrap tooling under
`host-setup/linux/`, its `bootstrap.sh` loader,
`scripts/test_bootstrap.py`, and the rules the scripts run under.
## Review record
Every one of the five closed its Copilot loop on its own pull request.
#668 ran five rounds and #667 seven, and between them eighteen findings
arrived as suppressed comments rather than as inline threads, thirteen
of which were real. Two of those were defects that would otherwise have
shipped in the gate this promotion carries: a subtree of new files
taking a filesystem walk that applies no ignore rules, and a docstring
count that was wrong as well as brittle.
## Consequence worth stating
The `GOVERNANCE.md` "Hub-Hosted Tooling" paragraph #667 added makes
every carrying repository's copy a past revision once this reaches
`main`. That is the ordinary consequence of a canonical moving rather
than a defect, but a repository meeting it first as a red audit line
will read it as a surprise. HomeAutomation-Config has already
re-vendored it by content rather than by bytes, since a byte copy from a
CRLF hub into an LF repository rewrites every line to change one
paragraph.
## Verified on this head
`develop` at `56f4d7d`, in sync with `origin/develop`. Local run of the
CI invocations: 223 prose self-tests, the prose gate over 117 files,
`repo_gate` (eol, eol-coverage, sha-pin), `spec/validate.py` with 22
cataloged, markdownlint over 45 files, and editorconfig-checker, all
clean.
Merge as a **merge commit**, never a squash, and without
`--delete-branch`: this pull request's head is `develop` itself.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@ptr727