Fix locale-dependent test, surface policy-only CLAUDE.md follow-ups, add read-only seam-drift audit

[justin808]

Fixes #281
Fixes #317
Refs #319

Three serial, independently reviewable commits against bin/push-downstream and
its test. No other path in the repo is touched.

Release-behavior surface and blast radius

bin/push-downstream is listed in .agents/agent-workflow.yml under
autonomous_merge.human_review_paths (id downstream-publishing, reason
release), so this PR is expected to require human review before merge. Blast
radius per change:

Change	Touches consumer repos?	Blast radius
#281 locale fix	No	Test-only. No production code path changed.
#317 CLAUDE.md follow-up	Reads only	Adds advisory stdout + PR-body text on the policy-only path. Writes no consumer file, changes no commit, push, or PR-gating logic.
#319 --audit mode	Read-only clone	New opt-in mode. Never writes to a consumer, never pushes a branch, never opens or updates a PR. Existing modes are unchanged.

Nothing in this PR changes when a policy or scaffold PR is created, what is
committed, or any existing push/lease/PR-gating safety check.

---

1. #281 — bin/push-downstream-test.rb was locale-dependent and aborted bin/validate

test_policy_apply_compares_validated_non_ascii_policy_content_as_bytes read the
published policy blob through git show backticks. That output is tagged with
Encoding.default_external, which is US-ASCII on a host with no locale
exported, so matching it against the UTF-8 source literal "# café policy"
raised Encoding::CompatibilityError instead of asserting.

Because bin/validate runs under set -euo pipefail, that error aborted the
entire validation run at its bin/push-downstream-test.rb step, so every later
suite and RuboCop never executed. Hosted CI never saw it because the runner
exports a UTF-8 locale.

Fix: compare as bytes, which is what the test name already promises — tag the
command output with .b and compare against .b literals. Test-only change.

The issue's optional second half (having bin/validate emit an explicit locale
diagnostic) is intentionally not included: it is outside this lane's owned
paths.

2. #317 — Policy-only sync now surfaces the CLAUDE.md consolidation follow-up

The full scaffold path already reported existing CLAUDE.md preserved; consolidate it to import @AGENTS.md. The policy-only fleet path could update a
consumer seam without carrying that follow-up into the generated PR, leaving a
host-dependent gap where Codex reads canonical AGENTS.md while Claude operates
from a separate rulebook. shakacode/shakapacker#1221 is the motivating miss.

Adds claude_consolidation_follow_up, a read-only audit that reports a follow-up
only when an existing CLAUDE.md neither matches the thin template nor imports
@AGENTS.md. Policy-only sync runs it after the base seam preflight, prints a
FOLLOW_UP <nwo> <text> line, and threads follow-ups into the generated policy
PR body. Full scaffold reconciliation delegates to the same audit so both paths
agree.

Advisory only: the consumer-owned file is never rewritten, deleted, or required
to be byte-identical to the thin template, and an absent CLAUDE.md produces no
follow-up.

Detection requires the explicit @AGENTS.md import form, because that is what
actually routes Claude to the canonical rulebook. A bare prose mention of
AGENTS.md, or an address-like ops@AGENTS.md, does not count.

3. #319 — Read-only fleet seam-drift audit (partial; see below)

Adds --audit: clones each selected registry consumer, runs the seam doctor, and
computes the desired-state diff inside the disposable clone. Emits one
machine-readable JSON report; exits non-zero when any consumer is not clean.

• Bound to the exact agent-workflows source SHA, plus whether that source
  worktree was clean.
• Per consumer: base SHA, seam doctor issues, exact changed managed paths, and
  the follow-ups the synchronizer intentionally cannot apply.
• Separates repo-local seam freshness from host-installed shared-skill freshness,
  so a drifted consumer is not misread as "every upstream commit needs a
  downstream PR". Shared skills are never vendored into consumers.
• A consumer that cannot be fetched, authenticated, or interpreted is reported
  blocked, with unestablished values left literally UNKNOWN — never clean.
• The report carries a contract block (and matching --audit help text) stating
  the statuses, the exit codes, and that seam_doctor_issues,
  changed_managed_paths, and follow_ups are each an Array or the string
  "UNKNOWN", so callers type-check before iterating.
• Refused in combination with --apply, --root, --policy-fleet, and
  --trusted-*, so the read-only mode cannot be turned into a writing one.

#319 partial coverage

Implemented here (script-side mechanism): direction items 1 (read-only
fleet-audit mode computing the desired-state diff without applying), 3 (one
machine-readable summary bound to the exact upstream source SHA), and 5
(distinguishing host-installed shared-skill freshness from repo-seam freshness).

Deliberately not implemented here, which is why this is Refs #319 and not
Fixes #319:

• Direction item 2 — CI trigger on contract-file change plus scheduled
  backstop. Requires a .github/workflows/ file, outside this lane's owned
  paths and itself a human_review_paths infrastructure surface.
• Direction item 4 — optionally opening/updating one focused sync PR per
  consumer. A live publishing behavior; deliberately not built in this batch.

Recommended follow-up: a scheduled + contract-path-triggered workflow that runs
bin/push-downstream --audit, uploads the JSON report, and fails on any
drifted/blocked consumer. The exit code and report schema added here are
designed for exactly that caller.

---

Review round 1 (commit 4)

audit_changed_paths mis-parsed rename entries — real latent bug, fixed.
git status --porcelain=v1 -z emits one XY <path> field per entry, but a
rename/copy entry is followed by a second field holding the origin path with no
XY prefix. Slicing every field at [3..] corrupted it. Verified directly:

field 0: " M keep.txt"  -> "keep.txt"
field 1: "R  new.txt"   -> "new.txt"
field 2: "old.txt"      -> ".txt"      <- corrupted, and old.txt is lost

Unreachable today (reconcile_scaffold writes in place and never stages, and git
only detects renames from the index) but one git add away from a publishing
tool emitting a corrupted path list a maintainer would act on. parse_porcelain_z_paths
now consumes the paired origin field explicitly: a rename reports both the new
path and the origin it removed, a copy reports only the new path (its origin is
intact), and any uninterpretable field returns nil so the consumer is reported
blocked rather than with a corrupted list. Add/modify/delete/untracked handling
is byte-for-byte unchanged.

Pinned by a staged-rename fixture that also contains modified, deleted, and
untracked files. The previous parser fails it:

--- expected
+++ actual
-["fresh.txt", "gone.txt", "keep.txt", "new.txt", "old.txt"]
+[".txt", "fresh.txt", "gone.txt", "keep.txt", "new.txt"]

Polymorphic report fields — documented, shape intentionally unchanged.
seam_doctor_issues, changed_managed_paths, and follow_ups are an Array for
clean/drifted and the string "UNKNOWN" for blocked. That is deliberate and
required by #319: an empty Array would read as "no issues, nothing to change"
for a repo that could not be inspected. Rather than flatten it, the types are now
documented where a caller sees them — a contract block in the report and the
--audit help text — including exit codes and an explicit instruction to
type-check before iterating.

Review round 2 (commit 5)

Unguarded AgentWorkflowSeamDoctor.check could abort the whole fleet audit — fixed.
It was the one call in audit_repo not degraded to an audit_blocked entry;
resolve_contract, the clone, reconcile_scaffold, and audit_changed_paths all
were. check reads consumer files and shells out via
Open3.capture2e("bash", "-n", path), so it can raise SystemCallError for a
single consumer under resource pressure (Errno::EMFILE across many fleet repos
back-to-back, Errno::ENOENT if bash is unresolvable).

That meant one bad consumer aborted the entire audit with a backtrace instead of
being recorded — contradicting the very criterion this mode exists to satisfy
("failures to fetch, authenticate, or interpret a consumer remain
UNKNOWN/blocked rather than being reported clean"), since interpreting the seam
is exactly what check does.

Now wrapped with the same rescue RuntimeError, SystemCallError breadth used for
reconcile_scaffold, returning audit_blocked with the base SHA already
established (values determined before the failure are kept; the rest stay
UNKNOWN).

Pinned by test_audit_records_uninterpretable_consumer_as_blocked_and_continues_the_fleet,
which forces check to raise for the first of two consumers and asserts the first
is blocked with its base SHA retained while the second is still audited
normally. Mutation-checked: without the rescue the exception escapes
audit_repo and the test errors with 1 runs, 0 assertions, 0 failures, 1 errors.

Review round 3 (commit 6)

Round 2's fix was incomplete — two more shell-outs in audit_repo were
unguarded. local_ref_head (Open3.capture2("git", …, "rev-parse", …)) and
audit_changed_paths (Open3.capture2("git", …, "status", …)). The asymmetry
that made this easy to miss:

Open3.capture2 on a missing binary -> raises Errno::ENOENT
Kernel#system   on a missing binary -> returns nil

Kernel#system (used for the clone) degrades safely; Open3.capture2 does not.
audit_changed_paths already handled a non-success exit status by returning
nil, but neither call handled the spawn itself failing — so under the same
Errno::EMFILE pressure the round-2 guard was written for, either one still
aborted the whole fleet audit.

Both are now under the same rescue RuntimeError, SystemCallError, with reasons
that name the failing step so a report reader can tell which stage gave up:

• consumer base commit could not be resolved: …
• consumer seam could not be interpreted: …
• desired state could not be computed: …
• reconciled clone could not be inspected: … (spawn failure)
• reconciled clone could not be compared with its base (unparseable status — unchanged)

Two regression tests force a spawn failure (not a non-zero exit) on each path
by making Open3.capture2 raise Errno::EMFILE for the first matching git
subcommand, asserting the consumer is blocked with values established before the
failure retained, and that a second consumer is still audited normally. Removing
either guard makes its test error with 1 runs, 0 assertions, 0 failures, 1 errors.

Review round 4 (commit 7) — audit path fully guarded

audit_source_provenance — highest consequence of the series. Its two
Open3.capture2 calls were unguarded, and it runs from audit_report, which
run_audit calls after consumers = repos.map { … } has already audited every
consumer. A SystemCallError there discarded the entire completed audit, not
one entry. Both fields already had an UNKNOWN representation, so each now
degrades independently (audit_source_sha, audit_source_worktree_clean): the
report still prints, with provenance honestly marked unknown rather than lost.

The system(...) clone — now uniformly guarded. Kernel#system returns nil
when exec() fails in the forked child, but still raises when fork() itself
fails (Errno::EAGAIN, Errno::ENOMEM) — the same pressure this hardening
targets. It is now inside the same rescue as its neighbours, with a distinct
reason separating "child never started" from "clone ran and failed", and the
comment no longer overstates the guarantee.

Every shell-out in the audit path is now guarded. Reasons distinguish the stage:

Stage	Reason
clone spawn	clone of <base> could not be started: …
clone result	clone of <base> failed
base commit	consumer base commit could not be resolved: …
seam doctor	consumer seam could not be interpreted: …
desired state	desired state could not be computed: …
status spawn	reconciled clone could not be inspected: …
status parse	reconciled clone could not be compared with its base

Two new regressions: a provenance spawn failure asserts the report still prints
with sha/worktree_clean as UNKNOWN and the consumer results and summary
intact; a clone fork() failure asserts the consumer is blocked and the next
consumer is still audited. Both mutation-checked — removing either guard makes its
test error with 1 runs, 0 assertions, 0 failures, 1 errors.

---

Codex Decision Log

Q: Should reconcile_claude (full scaffold path) also stop reporting the
follow-up for a rich CLAUDE.md that already imports @AGENTS.md?
Decision: yes — both paths now delegate to the same audit.
Why: #317's acceptance list requires that a rich file already routing to
@AGENTS.md produce no false positive, and it does not scope that to the
policy-only path. Two paths disagreeing about the same question would be a
defect. This only ever removes a spurious advisory line; it never changes file
content. Existing coverage still pins the rich-without-import case.
Review later: if maintainers want the scaffold path to keep nagging even after a
consumer adopts the import, revert this one delegation.

Q: What counts as "routes to canonical AGENTS.md"?
Decision: the explicit @AGENTS.md import token, via a regex that rejects a
preceding word character so ops@AGENTS.md does not qualify.
Why: Claude follows @ imports; prose mentions do not import anything, and
treating them as sufficient would silently reproduce the exact gap #317 reports.
Review later: if consumers adopt another routing convention, widen the pattern.

Q: create_policy_pr arity changed from 2 to 3.
Decision: change it and update the 17 affected test stubs.
Why: policy_pr_body is only reachable through create_policy_pr, so the
follow-ups have to travel that way. Hiding them in mutable shared state would be
worse. The stub churn is mechanical and confined to the owned test file.
Review later: none needed.

Q: Should --audit reconcile into the cloned consumer working tree?
Decision: yes — reconcile inside the disposable tmpdir clone, then read
git status for the exact changed managed paths.
Why: this is how the issue itself describes the replay ("applying it in a
disposable copy"). It is read-only with respect to the consumer repository: no
push, no PR, no gh call. Computing the diff any other way would duplicate
reconcile logic and risk drifting from real behavior.
Review later: none needed.

Q: A consumer that is seam-clean but carries an unappliable follow-up — clean
or drifted?
Decision: clean, with the follow-up still listed.
Why: the follow-up is advisory and cannot be applied by the synchronizer, so
grading it as drift would produce permanent, unfixable red and encourage
churn-only PRs — an explicit non-goal of #319.
Review later: if maintainers want these surfaced as a distinct state, add a
fourth status rather than folding it into drifted.

---

Validation

All commands run on the final lane head. Real output, not summarized.

Locale before/after evidence for #281

The sandbox blocks env -u, so the equivalence is shown explicitly — this
shell already has both variables unset, so a bare ruby is the
env -u LANG -u LC_ALL case:

$ ruby -e 'puts "LANG=#{ENV.fetch("LANG","<unset>")} LC_ALL=#{ENV.fetch("LC_ALL","<unset>")} default_external=#{Encoding.default_external}"'
LANG=<unset> LC_ALL=<unset> default_external=US-ASCII

BEFORE (at base 282a87a, unset locale):

$ ruby bin/push-downstream-test.rb --name test_policy_apply_compares_validated_non_ascii_policy_content_as_bytes
  1) Error:
PushDownstreamPolicyFleetTest#test_policy_apply_compares_validated_non_ascii_policy_content_as_bytes:
Encoding::CompatibilityError: incompatible character encodings: US-ASCII and UTF-8
    bin/push-downstream-test.rb:1576
1 runs, 6 assertions, 0 failures, 1 errors, 0 skips

AFTER (final head, unset locale):

$ ruby bin/push-downstream-test.rb --name test_policy_apply_compares_validated_non_ascii_policy_content_as_bytes
1 runs, 9 assertions, 0 failures, 0 errors, 0 skips

AFTER (final head, UTF-8 locale):

$ LANG=en_US.UTF-8 ruby bin/push-downstream-test.rb --name test_policy_apply_compares_validated_non_ascii_policy_content_as_bytes
1 runs, 9 assertions, 0 failures, 0 errors, 0 skips

Whole-file BEFORE at base, unset locale: 110 runs, 595 assertions, 0 failures, 1 errors, 0 skips.

ruby bin/push-downstream-test.rb — final head, both locales

$ ruby bin/push-downstream-test.rb
127 runs, 725 assertions, 0 failures, 0 errors, 0 skips

$ LANG=en_US.UTF-8 LC_ALL=en_US.UTF-8 ruby bin/push-downstream-test.rb
127 runs, 725 assertions, 0 failures, 0 errors, 0 skips

bin/lint (hosted Lint job runs exactly this)

$ bin/lint
== rubocop 1.87.0 ==
117 files inspected, no offenses detected
== shellcheck 0.11.0 ==
== markdownlint-cli2 0.23.2 ==
Summary: 0 issues in 0 files
Install yamllint 1.37.1 before running bin/lint

RuboCop (pinned 1.87.0), shellcheck, and markdownlint all pass. bin/lint cannot
run to completion on this machine because yamllint 1.37.1 is not installed;
actionlint is installed but is not reached. Neither applies to this diff, which
changes only two Ruby files:

$ git diff --name-only origin/main
bin/push-downstream
bin/push-downstream-test.rb

Note: bin/push-downstream has a #!/usr/bin/env ruby shebang, so bin/lint
selects it for RuboCop, not shellcheck.

Sample --audit report

Generated against local fixture repositories (no network, no consumer repo
contacted):

{
  "schema": "agent-workflows/downstream-seam-audit/v1",
  "source": {
    "repo": "shakacode/agent-workflows",
    "sha": "633370c4f65b0ecd8ee7c23212aa88dfa3b05a51",
    "worktree_clean": false
  },
  "contract": {
    "statuses": ["clean", "drifted", "blocked"],
    "polymorphic_fields": ["seam_doctor_issues", "changed_managed_paths", "follow_ups"],
    "polymorphic_note": "For a clean or drifted consumer, seam_doctor_issues, changed_managed_paths, follow_ups are each an Array. For a blocked consumer they are the string \"UNKNOWN\", because the value could not be established. Type-check before iterating; an empty Array is never used to represent an unknown value.",
    "exit_codes": {
      "0": "every selected consumer is clean",
      "1": "at least one consumer is drifted or blocked"
    }
  },
  "scope": {
    "audited": "repo-local agent-workflow seam and scaffold drift",
    "not_audited": "host-installed shared skills and workflows",
    "note": "Shared skills and workflows are installed per host and are intentionally not copied into consumer repos. A drifted consumer indicates repo-local seam drift only; it does not imply that every upstream commit requires a downstream PR."
  },
  "summary": { "total": 3, "clean": 1, "drifted": 1, "blocked": 1 },
  "consumers": [
    {
      "repo": "local/drifted-consumer",
      "base_sha": "86bec655c03f0f1940feabfa9644b214d06172ec",
      "status": "drifted",
      "seam_doctor_issues": ["missing AGENTS.md"],
      "changed_managed_paths": [
        ".agents/agent-workflow.yml", ".agents/bin/README.md", ".agents/bin/lint",
        ".agents/bin/test", ".agents/bin/validate", "AGENTS.md", "CLAUDE.md"
      ],
      "follow_ups": []
    },
    {
      "repo": "local/current-consumer",
      "base_sha": "61e8a9a5c9008653c0d96ce8845454683ff80411",
      "status": "clean",
      "seam_doctor_issues": [],
      "changed_managed_paths": [],
      "follow_ups": ["existing CLAUDE.md preserved; consolidate it to import @AGENTS.md"]
    },
    {
      "repo": "local/unreachable-consumer",
      "base_sha": "UNKNOWN",
      "status": "blocked",
      "seam_doctor_issues": "UNKNOWN",
      "changed_managed_paths": "UNKNOWN",
      "follow_ups": "UNKNOWN",
      "reason": "clone of main failed"
    }
  ]
}

Mutation check on the #317 negative control

Removing the @AGENTS.md import guard makes the negative control fail, proving
it is not vacuous:

$ ruby bin/push-downstream-test.rb --name test_claude_consolidation_audit_flags_only_rich_files_that_do_not_import_agents
  1) Failure:
Expected "existing CLAUDE.md preserved; consolidate it to import @AGENTS.md" to be nil.
1 runs, 3 assertions, 1 failures, 0 errors, 0 skips

Guard restored; the test passes on the final head.

bin/validate

Full local bin/validate runs were executed at 633370c. The later review-round
commits change only bin/push-downstream* and touch no other suite; that suite
went 120/679 → 127/725 and is green under both locales at the current head.
Hosted validate is authoritative and passed on 633370c, a1864d7, and
c5f51ad. The push-downstream step passes in both local runs,
and in the unset-locale run bin/validate now proceeds past it instead of
aborting there — which is the whole point of #281:

$ ruby bin/push-downstream-test.rb          # (inside bin/validate, LANG/LC_ALL unset)
== push-downstream unit tests ==
120 runs, 679 assertions, 0 failures, 0 errors, 0 skips
== agent workflow drift unit tests ==       # <-- run continues; previously aborted here

$ LANG=en_US.UTF-8 LC_ALL=en_US.UTF-8 bin/validate
== push-downstream unit tests ==
120 runs, 679 assertions, 0 failures, 0 errors, 0 skips

Neither full run reaches PASS agent-workflows validation on this machine, and
not because of this PR. Both abort later, in files this PR does not touch
(git diff --name-only origin/main lists only the two bin/push-downstream*
files). Reported honestly rather than as "clean":

1. Unset locale — aborts at skills/post-merge-audit/bin/completed-batch-audit-receipt-test.rb

Encoding::InvalidByteSequenceError: "\xE2" on US-ASCII
70 runs, 174 assertions, 0 failures, 37 errors, 0 skips

Same root-cause class as #281 in a different file: green under UTF-8
(70 runs, 572 assertions, 0 failures, 0 errors), 37 errors under US-ASCII. This
was previously masked — bin/validate aborted at bin/push-downstream-test.rb
before ever reaching it. Fixing #281 is what exposes it. Out of this lane's owned
paths; reported for separate scheduling, not fixed here.

2. UTF-8 locale — aborts at bin/check-agent-workflow-drift-test.rb#test_git_probes_fail_closed_when_they_time_out

Errno::ENOENT: No such file or directory @ rb_sysopen - .../hanging-git/git.pid
73 runs, 1160 assertions, 0 failures, 1 errors, 0 skips

A timing race: the test reads a pid file before the spawned hanging-git process
writes it. Re-run in isolation on this machine it still fails
(1 runs, 8 assertions, 0 failures, 1 errors), so on this host it is worse than
intermittent — it loses the race consistently under load. Locale-independent and
unrelated to this diff. Out of lane; not fixed here.

Because bin/validate runs under set -euo pipefail, both aborts mean its final
RuboCop step never executes. That is precisely the masking behavior #281
describes, so lint was verified independently via bin/lint above.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 19 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5cb420ea-1662-45c3-b160-5b304e4ecdc3

📥 Commits

Reviewing files that changed from the base of the PR and between 282a87a and bc2fb0a.

📒 Files selected for processing (2)

• bin/push-downstream
• bin/push-downstream-test.rb

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Review: Fix locale-dependent test, surface CLAUDE.md follow-ups, add --audit

Three well-scoped, independently reviewable changes to bin/push-downstream / bin/push-downstream-test.rb. No other paths touched, matching the PR description. Overall this is high-quality, carefully tested work — findings below are minor.

1. Locale fix (#281)

Comparing git show output as bytes (.b) against .b literals is the correct fix — it matches what the test name already promised, and avoids Encoding::CompatibilityError on hosts without a UTF-8 locale exported. Good catch that this was silently aborting bin/validate (via set -euo pipefail) before RuboCop ever ran. Test-only, low risk.

2. CLAUDE.md consolidation follow-up (#317)

• claude_consolidation_follow_up is read-only and correctly delegated to from both the full-scaffold path (reconcile_claude) and the policy-only path (sync_policy_repo), so the two paths can no longer disagree about the same file.
• The CLAUDE_AGENTS_IMPORT regex (/(?<![\w.-])@AGENTS\.md\b/) correctly rejects ops@AGENTS.md-style false positives via the negative lookbehind, and the word boundary correctly rejects near-miss extensions. Nice negative-control test (test_claude_consolidation_audit_flags_only_rich_files_that_do_not_import_agents) plus a mutation check in the PR description proving the guard isn't vacuous.
• create_policy_pr's arity change from 2→3 required updating ~17 test stubs — mechanical but correctly done throughout; I didn't find a stub that was missed.
• follow_up_section extraction (shared between pr_body and policy_pr_body) is a reasonable small dedup; verified the heredoc interpolation still produces the same blank-line spacing as before when follow_ups is empty.

3. Read-only --audit mode (#319, partial)

• audit_repo reconciles inside a disposable Dir.mktmpdir clone and only ever reads git status afterward — never pushes, never touches gh. Matches the PR's own description of the design.
• CLI-level guards correctly refuse --audit combined with --apply, --root, --policy-fleet, and --trusted-*, with a dedicated integration test (test_audit_refuses_to_combine_with_applying_or_writing_modes).
• blocked consumers correctly leave every unestablished field as literal "UNKNOWN" rather than defaulting to clean — good fail-closed behavior, and it's exercised by a test.
• No shell-injection risk: all git/system calls pass args as arrays, consistent with the rest of the file.

Minor issues (posted inline):

• audit_changed_paths's NUL-split parsing of git status --porcelain -z would mis-parse a rename/copy record (the second field in a rename record has no XY prefix, so slicing [3..] off it corrupts the path). Currently unreachable since reconcile_scaffold never renames files and unstaged worktree changes don't get rename-detected anyway, but worth a guard or comment so it doesn't bite a future change silently.
• The report's seam_doctor_issues / changed_managed_paths / follow_ups fields are Array for clean/drifted consumers but the string "UNKNOWN" for blocked ones — intentional per the PR's decision log, but downstream JSON consumers (e.g. the CI workflow the PR recommends as a follow-up) need to type-check before iterating. Worth flagging explicitly in the schema/report docs.
• The new --audit flag isn't reflected in the OptionParser usage banner, and docs/downstream-sync.md doesn't document the new mode yet (it documents --root, --policy-fleet, etc., but nothing about --audit). Since the PR explicitly positions --audit as the mechanism a future CI workflow would call, a short doc addition would help discoverability.
• Not blocking, but noting for completeness: audit_repo clones fleet consumers sequentially (repos.map), so a large fleet audit could be slow. Fine for now given the module's existing sequential style elsewhere, but worth keeping in mind if this becomes a CI gate as recommended in the PR description.

Test coverage

Coverage is strong and the validation section in the PR description (before/after locale repro, mutation check, full suite run) is exemplary. Two audit-path branches don't appear to have direct unit tests: resolve_contract raising inside audit_repo (contract/preset errors) and reconcile_scaffold raising SystemCallError inside the cloned tmpdir — both are handled correctly in the code, just not directly exercised. Not a blocker given the equivalent RuntimeError paths are tested at the resolve_contract-outside-clone level.

Security

No concerns — no shell interpolation, no credentials handled directly, clones are read-only local clones of already-configured registry repos.

Nice work — this is a careful, safety-conscious change to a release-critical script, and the PR description's up-front risk/blast-radius framing made this much easier to review.

[claude]

Review summary

Three well-scoped, independently-reviewable changes to bin/push-downstream/bin/push-downstream-test.rb. No other paths touched. The PR description is unusually thorough (before/after locale evidence, mutation checks, decision log) which made this easy to verify against the diff.

1. #281 — locale-dependent test fix

Comparing git show output (tagged Encoding.default_external) against a UTF-8 literal correctly moves to byte comparison (.b). Test-only, low risk, clearly fixes the described Encoding::CompatibilityError that was silently aborting bin/validate (and RuboCop) under an unset locale.

2. #317 — CLAUDE.md consolidation follow-up on the policy-only path

• claude_consolidation_follow_up is a clean read-only audit shared by both reconcile_claude (full scaffold) and the new policy-only call site — good, avoids the two paths disagreeing.
• The @AGENTS.md import detection regex ((?<![\w.-])@AGENTS\.md\b) correctly rejects ops@AGENTS.md while accepting a real import; the negative/positive control tests back this up, and the "mutation check" in the PR description (removing the guard and watching the negative-control test fail) is a nice touch that proves the test isn't vacuous.
• Reading via File.binread(...).force_encoding("UTF-8").scrub instead of the old File.read(path, encoding: "UTF-8") is also a quiet fix for the same class of encoding crash as bin/push-downstream-test.rb is locale-dependent and aborts bin/validate when LANG is unset #281, and additionally avoids an Errno::EISDIR if CLAUDE.md were ever a directory (previously would've raised, now returns nil/no follow-up) — nice incidental hardening.
• create_policy_pr arity bump from 2 to 3 args is threaded correctly through every call site (open_policy_pull_request, ensure_policy_pull_request, refresh_policy_branch_from_base) and all sync_policy_repo control-flow branches that eventually create a PR. I traced the full method and didn't find a branch where follow_ups gets dropped.
• follow_up_section extraction to share the body-building logic between pr_body and policy_pr_body is correct — verified the squiggly-heredoc interpolation still renders a clean blank line when there are no follow-ups and a proper ## Follow-ups block when there are (also covered by the new tests).

3. #319 — --audit read-only fleet-drift mode

• CLI mutual-exclusion guards (--root/--policy-fleet/--apply/--trusted-*) are correctly enforced before dispatch, with tests for each combination.
• parse_porcelain_z_paths — the rename/copy origin-field handling is correct. I traced it against git status --porcelain=v1 -z semantics by hand (rename/copy emits a second NUL-terminated field with no XY prefix) and it matches; the regression test pinning the previously-corrupted old.txt → .txt case is a good catch and well isolated from the add/modify/delete/untracked cases.
• The polymorphic Array vs "UNKNOWN" field contract is a reasonable design choice (an empty array legitimately means "clean", so it can't also mean "unknown"), and it's documented both in the JSON contract block and in --audit's help text, which is a nice touch for machine callers.
• One correctness/robustness note left as an inline comment: AgentWorkflowSeamDoctor.check(clone) inside audit_repo isn't wrapped in a rescue the way reconcile_scaffold is, and it can raise SystemCallError via a shelled-out bash -n syntax check. Since run_audit builds the report via repos.map, an exception for one consumer currently aborts the entire batch (no report printed for anyone), which cuts against the audit's own "always report blocked, never crash" design intent — especially relevant since the PR's own recommended follow-up is running this unattended in CI across the whole fleet.

General

• All new system/Open3 calls use array-form arguments, so no shell-injection risk from nwo/branch/URL values.
• --audit is genuinely read-only: clone is --depth 1 into a disposable Dir.mktmpdir, reconcile_scaffold only touches the disposable clone, and there's no push/gh pr call anywhere on that path — matches the PR's stated blast-radius claims.
• Test coverage is thorough for the new surface (clean/drifted/blocked, follow-up passthrough, CLI flag conflicts, and the rename-parsing edge case), and the existing 17 create_policy_pr stub call sites were mechanically updated to match the new arity.

Nothing here blocks merge in my view; the one inline note is worth a look but is a narrow edge case (subprocess resource exhaustion) rather than a correctness bug in the common path.

[claude]

Review summary

Reviewed the diff (bin/push-downstream + bin/push-downstream-test.rb, ~621 additions) covering the three linked fixes: the locale-dependent test encoding bug (#281), the policy-only-sync CLAUDE.md follow-up surfacing (#317), and the new read-only --audit fleet seam-drift mode (#319).

Overall: this is a well-scoped, carefully reasoned change. The rename/copy fix in parse_porcelain_z_paths is correct — I verified against git-status(1)'s documented -z field-order-reversal behavior ("from -> to becomes to from"), and the new path/origin field consumption matches that exactly. The @AGENTS.md import-detection regex correctly excludes address-like ops@AGENTS.md via its negative lookbehind, and the byte-comparison fix for the locale test is the right fix for the root cause described. The create_policy_pr arity threading through follow_ups is consistent across every call site I checked, including the 17 updated test stubs.

One correctness gap found (left as an inline comment on bin/push-downstream): the "Review round 2" fix that wraps AgentWorkflowSeamDoctor.check in rescue RuntimeError, SystemCallError to keep one bad consumer from aborting the whole --audit fleet run is incomplete. Two more calls in the same audit_repo method — local_ref_head(clone, "HEAD") and audit_changed_paths(clone) — also shell out via Open3.capture2, which (unlike Kernel#system, used for the clone step) does not swallow subprocess-spawn failures like Errno::EMFILE/Errno::ENOENT. Since neither audit_repo nor its caller run_audit has a catch-all, the same resource-pressure failure this PR explicitly fixed for check can still crash the entire audit run (losing every consumer's result, since the JSON report is only printed after repos.map completes) rather than reporting that one consumer as blocked. Not covered by the existing "records uninterpretable consumer as blocked" test, which only stubs check.

No security concerns — all shell-outs use argv-array form (no shell interpolation), and the audit clone/reconcile happens in a disposable Dir.mktmpdir that's cleaned up even on early return. No other correctness, performance, or style issues stood out.

[claude]

Review

Reviewed the diff (gh pr diff 337) against bin/push-downstream / bin/push-downstream-test.rb. Overall this is a well-scoped, thoroughly-tested change (locale fix, CLAUDE.md follow-up surfacing, read-only --audit mode), and the three review rounds documented in the PR body already caught several real bugs (rename-parsing corruption, unguarded AgentWorkflowSeamDoctor.check, unguarded rev-parse/status spawns). Two more gaps in the same class appear to remain.

Potential issues

1. The git clone in audit_repo is the one shell-out in the new audit path left unguarded against spawn failure (bin/push-downstream:784-788)

Round 2/3 wrapped local_ref_head, AgentWorkflowSeamDoctor.check, reconcile_scaffold, and audit_changed_paths in rescue RuntimeError, SystemCallError specifically because Open3.capture2 can raise Errno::EMFILE under fleet-wide fd pressure. The comment justifying why the system(...) clone call above them doesn't need the same guard says:

Open3.capture2 raises when the child cannot be spawned (Errno::EMFILE under fleet-wide resource pressure, Errno::ENOENT for a missing git), unlike Kernel#system above, which returns nil.

That's true for the "command not found" case (exec() failing inside the forked child, reported back and converted to nil), but a fork()-level failure — which is exactly the EMFILE-under-resource-pressure scenario this PR is defending against — happens in the parent process before any child exists, and typically surfaces as a raised Errno::EMFILE/Errno::ENOMEM from Kernel#system too, not a swallowed nil. If that fires during the clone call, it isn't rescued, and it would abort the entire --audit fleet run (unlike every other shell-out in this same function, which was specifically hardened against this).

Notably, the round-3 tests (test_audit_records_base_commit_spawn_failure_as_blocked_and_continues_the_fleet, test_audit_records_status_spawn_failure_as_blocked_and_continues_the_fleet) force a spawn failure on rev-parse and status but not on the clone call itself, so this path isn't covered by the new spawn-failure regression tests. Worth either wrapping the clone call in the same rescue (folding it into the existing audit_blocked(repo, "clone of ... failed") path) or confirming empirically that Kernel#system truly can't raise here.

2. audit_source_provenance uses unguarded Open3.capture2 calls (bin/push-downstream:765-773)

def audit_source_provenance(root = File.expand_path("..", __dir__))
  sha, sha_status = Open3.capture2("git", "-C", root, "rev-parse", "HEAD")
  porcelain, porcelain_status = Open3.capture2("git", "-C", root, "status", "--porcelain")
  ...

This runs once, after every consumer has already been audited, to attach source-SHA provenance to the final report (audit_report → run_audit). Unlike every git shell-out inside audit_repo, neither call here is rescued. Since this PR's own commit history establishes that Open3.capture2 can raise SystemCallError under the same fleet-wide resource pressure that motivated rounds 2 and 3, a failure here would crash run_audit after all the (potentially expensive, network-touching) per-consumer work has completed, discarding the whole report instead of degrading source.sha/source.worktree_clean to "UNKNOWN" the way blocked consumers do. Given the surrounding code goes out of its way to avoid exactly this failure mode, this call seems like an oversight rather than an intentional asymmetry.

Notes (non-blocking)

• parse_porcelain_z_paths, the CLAUDE_AGENTS_IMPORT regex (including the ops@AGENTS.md negative case), and the follow_ups threading through create_policy_pr/policy_pr_body/pr_body all look correct and are well covered by the added tests — I traced through the porcelain-z field-pairing logic against git status semantics and it correctly handles the rename/copy origin-field case that was the round-1 bug.
• All new/changed shell-outs use array-style system/Open3.capture2 args (no string interpolation into a shell), so no command-injection concerns.
• follow_up_section extraction from pr_body into a shared helper is a clean, behavior-preserving refactor (verified the interpolation placement in policy_pr_body's heredoc still produces the same empty-line layout when there are no follow-ups).
• Minor style nit: pr_body's follow_up_section = follow_up_section(follow_ups) shadows the method name with a local of the same name. Valid Ruby (unambiguous since it's called with an argument), but a distinct local name would be easier to scan.
• The new run_audit/audit_repo fleet loop is sequential (repos.map), consistent with the existing sync_repo/sync_policy_repo loops elsewhere in the file, so no new performance regression — just worth noting for large fleets since each consumer does a real clone + bash -n seam check.

🤖 Generated with Claude Code

[claude]

Review summary

This PR bundles three independently-scoped, well-isolated changes to bin/push-downstream / bin/push-downstream-test.rb:

1. bin/push-downstream-test.rb is locale-dependent and aborts bin/validate when LANG is unset #281 — fixes a locale-dependent test (Encoding::CompatibilityError on hosts with no locale exported) that silently aborted bin/validate before RuboCop ever ran. Fix compares bytes (.b) instead of relying on Encoding.default_external. Correct, test-only, low risk.
2. Policy-only downstream sync should surface rich CLAUDE.md consolidation follow-ups #317 — extracts reconcile_claude's inline CLAUDE.md check into claude_consolidation_follow_up, and threads it through the policy-only fleet path so it surfaces the same "consolidate to import @AGENTS.md" advisory that the full scaffold path already reported. The @AGENTS.md import regex ((?<![\w.-])@AGENTS\.md\b) correctly rejects address-like ops@AGENTS.md while matching real imports. create_policy_pr/policy_pr_body/etc. arity bumped from 2→3 args to carry follow_ups; all callers and the ~17 test stubs were updated consistently. Advisory-only, never rewrites consumer files.
3. Continuously audit downstream consumer seam drift after contract changes #319 — adds a new read-only --audit mode that clones each consumer, runs the seam doctor, and diffs against desired state inside a disposable tmpdir clone. Notably thorough hardening here: every shell-out in the audit path (Open3.capture2 for rev-parse/status, system for clone, AgentWorkflowSeamDoctor.check) is now individually rescued for RuntimeError, SystemCallError so one unreachable/misbehaving consumer degrades to a blocked entry with UNKNOWN fields instead of aborting the whole fleet audit or losing an already-completed report. The rewritten parse_porcelain_z_paths correctly handles git status --porcelain=v1 -z rename/copy entries (the old code corrupted origin paths by blindly slicing every field at [3..]).

Code quality

• Consistent with existing file conventions: array-form system/Open3.capture2 calls (no shell injection risk), plain-string raise → RuntimeError used throughout so the new rescue RuntimeError, SystemCallError guards in the audit path actually catch everything reconcile/resolve code can throw (verified: no Psych::Exception/other non-RuntimeError types escape uncaught).
• The polymorphic Array vs "UNKNOWN" string fields in the audit report are unusual but are explicitly documented in-band (contract block + --audit help text), which is the right call given the alternative (an empty array meaning "no issues" vs "couldn't tell") is a worse footgun.
• follow_up_section extraction in pr_body/policy_pr_body is a clean, behavior-preserving dedup.
• Good defense-in-depth on the CLI flag parsing: --audit explicitly refuses --apply, --root, --policy-fleet, and any --trusted-* flags, so the read-only mode can't accidentally be turned into a writing one.

Potential issues

• Test coverage gap (flagged inline on bin/push-downstream:742): run_audit — the actual top-level function that loads the registry, maps audit_repo over every selected consumer, prints the JSON report, and computes the process exit code — has no test coverage at all. All existing --audit tests either call audit_repo/audit_report directly, or hit CLI-level flag-conflict rejections that return before run_audit is ever invoked. Given this is the exact entry point the recommended CI follow-up (scheduled bin/push-downstream --audit workflow) would call, it'd be good to add one test that runs it end-to-end against a small fixture registry.
• Minor/non-blocking: reconcile_claude checks File.exist?(path) while claude_consolidation_follow_up checks File.file?(path) — if CLAUDE.md were ever a directory (very unlikely in practice), the former would skip writing the thin template while the latter silently returns no follow-up. Not worth a fix given how implausible the scenario is.

Security

No new injection surface — all shell-outs use array-form system/Open3.capture2 args, consistent with the rest of the file. The new clone-and-inspect audit path is genuinely read-only with respect to consumers (verified by tests asserting no new branch is pushed to the remote).

Validation

The PR description includes extensive before/after evidence (locale repro, bin/lint, full test suite runs under both locales, a sample --audit JSON report, and mutation checks proving several of the regression tests aren't vacuous). This is unusually thorough documentation for a bug-fix/feature PR.

Overall: solid, carefully hardened change with good test density for the individual audit-path building blocks. The one gap worth closing before merge is end-to-end coverage of run_audit itself.

[justin808]

AW E batch closeout — ready-human-review-required

Final head bc2fb0aaaa42114a0a16d458bf17109a21c90ae8. All four gates green (Lint, validate, claude-review; CodeRabbit recorded as a rate-limit waiver). Zero unresolved review threads.

autonomous-merge-eligibility: human-approval-required, triggered gate repo-path:downstream-publishing on bin/push-downstream. merge-assurance: BLOCKED — auto merge authority lacks a qualifying exact-head autonomous verdict. That is the correct and intended outcome: this path is declared under autonomous_merge.human_review_paths with reason release. A maintainer decision is required on the current head regardless of the merge-path issue noted below.

Independent checker evidence: ruby bin/push-downstream-test.rb → 127 runs, 725 assertions, 0 failures, run against a git archive export of this head proven byte-identical to bc2fb0a by git hash-object. The #281 locale fix verified with LANG/LC_ALL unset: passes here, versus Encoding::CompatibilityError at base 282a87a.

Two corrections to this PR's own framing, for the reviewer

1. The rename-parsing bug was never a main-branch defect. Base 282a87a contains no --audit mode at all — no audit_changed_paths, no porcelain parsing, no run_audit. The record[3..] corruption was introduced by this PR's commit 633370c and fixed by its own commit 89d05af. The fix is correct, but its value is "a new feature did not ship broken," not "a live bug was repaired." The coordinator originally framed this as a pre-existing defect; that was wrong, and the independent checker caught it.

2. This PR is broader than its title suggests. Seven commits: an entire read-only --audit fleet mode (~200 new lines with its own JSON schema, source provenance, and blocked-consumer contract), the CLAUDE.md consolidation follow-up threaded through policy-only sync and generated PR bodies, and the locale fix. Only 7015be5 (locale) is a bug fix against main. Against this repository's small-focused-PR standard this is arguably three PRs; reviewers should size their review accordingly.

Review convergence

Five review rounds ran on this PR, each surfacing something real: rename parsing, then the unguarded seam-doctor call, then two sibling Open3.capture2 calls, then audit_source_provenance (the worst of them — it runs after every consumer is audited, so a raise there discarded a completed report). All seven shell-outs in the audit path are now guarded. The loop was closed deliberately at round five; the remaining reviewer finding — that run_audit itself has no test, including its exit-code aggregation — is tracked as #365 rather than taken as a sixth round.

Also note: validate failed once on this exact head and passed on re-run with no commit change, on an unrelated test in test/agent_doctor/**. Filed as #364.

Issue #319 stays open by design — this PR uses Refs and lands the audit mode only; the CI trigger, scheduled backstop, and sync-PR items are recorded on the issue. #281 and #317 do close here.

Why this is not auto-merged. The batch ran with merge_authority: auto_merge_when_gates_pass, but the portable merge path is closed in this repository: .agents/agent-workflow.yml sets merge_submission.mode: merge_queue_only, while main has no merge queue and no branch protection (live isMergeQueueEnabled: false), and there is no .agents/bin guard. pr-merge-submit fails closed on exactly that combination. Using gh pr merge instead would bypass the policy that helper exists to enforce, so it was not done. This needs a human merge, or a seam change.

Coordination: coordination: unavailable — repository seam .agents/agent-workflow.yml declares coordination_backend: n/a (single-operator run).

Completed-batch audit: not published. completed-batch-publication-preflight requires every pull-request target to be merged before a completed-batch-audit v1 receipt may be published; this batch has no merged targets, so publishing one would be incorrect. Audit conclusion recorded as follow-ups-remain.

QA: verified by an independent checker (checker-aw-e, distinct from every maker), read-only, including mutation testing. No UI surface in this batch, so no visual capture was required.

[justin808]

---

head_sha: bc2fb0a
triggered_gates:

• repo-path:downstream-publishing
  rollback_disposition: Code-only rollback and forward recovery were reviewed; the new audit path is read-only and existing consumer publishing behavior remains unchanged.
  decision: approve
  approved_by: justin808
  source: direct-user-task
  evidence: Fix locale-dependent test, surface policy-only CLAUDE.md follow-ups, add read-only seam-drift audit #337 (comment)
  ...

[justin808]

QA Evidence

• QA lane: checker-aw-e; read-only git archive export of PR Fix locale-dependent test, surface policy-only CLAUDE.md follow-ups, add read-only seam-drift audit #337 head proven byte-identical by git hash-object; coordination claim/heartbeat not applicable because the repository seam declares backend n/a
• Scope checked: bin/push-downstream and bin/push-downstream-test.rb; locale-independent policy fixture, rich and already-consolidated CLAUDE.md fixtures, clean/drifted/blocked fleet states, rename parsing, guarded failure paths, exact source/base bindings, and no-write audit behavior
• Tested at: PR Fix locale-dependent test, surface policy-only CLAUDE.md follow-ups, add read-only seam-drift audit #337 head bc2fb0aaaa42114a0a16d458bf17109a21c90ae8
• Automated checks: independent ruby bin/push-downstream-test.rb — 127 runs, 725 assertions, 0 failures, 0 errors; exact-head Validate, Lint, Claude review, and CodeRabbit checks passed
• Manual checks: mutation checks confirmed the new failure guards and rename parser are exercised; durable checker summary: Fix locale-dependent test, surface policy-only CLAUDE.md follow-ups, add read-only seam-drift audit #337 (comment)
• User-visible UI change: no
• Visual evidence: not applicable: no user-visible UI change
• Interaction change: no; CLI audit/reporting behavior is covered by deterministic fixtures
• Interaction evidence: not applicable: no user-visible interaction surface
• Visual fix: no; this PR changes CLI workflow behavior and tests
• Negative control: not applicable: no visual fix
• Performance evidence: not applicable: no rendered-page, asset, bundle, or performance-sensitive runtime surface
• Findings: no merge-blocking QA finding; push-downstream --audit: run_audit entry point and exit-code aggregation are untested #365 durably tracks direct run_audit entry-point and exit-code aggregation coverage, and orchestrator_trust_test hardlinked-delegate test flakes on hosted CI (passes on rerun, unchanged head) #364 tracks the unrelated unchanged-head hosted-CI flake
• QA required: yes
• QA required rationale: developer-workflow and downstream release-tool behavior require independent exact-head replay beyond ordinary worker validation
• QA lane status: satisfied
• Release-blocking status: clear
• Process-gap disposition: script