Conversation
Contributor
Author
|
This change is part of the following stack: Change managed by git-spice. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: redhat-et/ripwire/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: redhat-et/ripwire/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (15)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe test suite centralizes GNU/BSD ChangesPortable test gates
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
s0undt3ch
changed the title
s0undt3ch
marked this pull request as ready for review
September 19, 2026 22:29
Collaborator
|
Thanks for tracking this down — the coreutils What's good, with evidence
Before merge
Smaller items
What happens next This rides the next integration train once the two "before merge" items are addressed (or explicitly Detail for the orchestrator (not part of the posted text)Gates run (all against this PR's binary,
|
| gate | rc |
|---|---|
| cachehashcheck | 0 |
| cachesplitcheck | 0 |
| clonecachecheck | 0 |
| codexpromptroutecheck | 0 |
| evictioncheck | 0 |
| g1freshcheck | 0 (SKIP — asan/ not configured in this worktree, expected) |
| headsnapcachecheck | 0 |
| mcpeditmodecheck | 0 |
| portablecachecheck | 0 |
| prcontextcheck | 0 |
| qsnapcachecheck | 0 |
| qsnapprefetchcheck | 0 |
| statgatecheck | 0 |
| manifestcheck | 0 |
| gateexitcheck | 0 |
| gatecountcheck | 0 |
| claudeconfigdircheck (ambient CLAUDE_CONFIG_DIR probe, see finding 2) | 1 (RED, as expected — reproduces the leak; PASSES clean with no ambient var set) |
quality-delta
--quality-delta=origin/main..HEAD:gating="12" preexisting-worse="12", all 12 findings in src/
files this PR does not touch (compactlegend.h, dynamic_map.hpp, testmap.h, crossref.h, mergescout.h,
flipimpact.h, gitoracle.h, notes.h, renamemine.h, slice.h, taskroute.h) — a measurement artifact of the
PR's 100-commit-stale base, confirmed by re-running against the true merge-base.--quality-delta=92bc055ee3550ef695308c0b6ed7d48414e9b986..HEAD(the PR's actual merge-base with
origin/main):regressions="0" minor="0" preexisting-worse="0" gating="0". This is the number that
actually describes what the PR did.git status --porcelain --untracked-files=noempty;target_ref==git rev-parse HEAD==
af88bfd in both runs.
Train interaction (trains 8-11: compact-legend-default, MCP affected/rank_by, gate-list churn)
- No textual conflict:
git diff --name-only 92bc055e..HEAD(PR's own changes) touches 15 test/ files;
none overlap the lines trains 8-11 changed in the two files they share
(prcontextcheck.sh— trains added--legend=fullto several invocations for the new compact
default, in the OUT=/NTOUT=/PRC_FAULT lines; the PR's own edit is near the top, adding the
statcompat.shsource line and switchingfile_mode→mode_of-equivalent reads — different regions,
clean merge) andsidecarsymlinkcheck.sh(trains added--legend=fulltoreadArchCheck; the PR
doesn't touch this file at all — it's one of the four missed stat-trap gates, see finding 1). test/regression.sh's single-line gate-absorb loop grew by three names on main since the PR's base
(forhdrshapecheck,forsectioncollapsecheck,notesdegradecheck) — the PR doesn't edit that line,
so it's a non-issue for merge, but it's more evidence the PR is meaningfully behind main and should
rebase before landing rather than merge blind.- No semantic conflict found: the PR's changes are pure test-infrastructure refactors orthogonal to the
legend-default/MCP-tool changes trains 8-11 shipped.
Dogfood gaps
--for="GNU BSD stat compat"/--grep=stat -fwere not tried — this is a test/*.sh-only PR (shell,
not one of ripwire's indexed source languages for symbol-graph purposes), so--for/--expand/
--callers/--impactdon't apply to gate scripts the way they do to src/. Fell straight togrep -rl
andgit diff/git logfor the whole review, which is the correct tool for shell-script diffing, not
a fallback from a verb that should have worked. Logging as a genuine "N/A for this artifact type"
rather than a gap: ripwire's own graph has no notion of bash function call edges acrosstest/*.sh
andtest/lib/*.sh, so "who sourcesmode_of" had no ripwire verb to ask —grep -rlwas the only
route to the four missed gates in finding 1. That's a real product gap if this repo's own gate-script
reuse is a class of question worth answering (bash isn't in ripwire's language list per CLAUDE.md).
s0undt3ch
added a commit
to s0undt3ch/ripwire
that referenced
this pull request
Sep 20, 2026
…heck's own hermetic leak Addresses the two before-merge items from PR redhat-et#298's review: - cacheisolationcheck.sh, qsnapproducercheck.sh, sidecarsymlinkcheck.sh and tempfilesymlinkcheck.sh hand-rolled the same GNU-vs-BSD stat detect-once block this PR already extracted from twelve other gates. All four now source test/lib/statcompat.sh instead. - claudeconfigdircheck.sh's "UNSET IS UNCHANGED" arm relied on CLAUDE_CONFIG_DIR being absent from the environment rather than clearing it, so an ambiently exported CLAUDE_CONFIG_DIR made the gate write real files into it and then fail against its own contaminated baseline. It now sources test/lib/clean-env.sh first, same as skillinstallcheck.sh and hermesinstallcheck.sh, which had the identical gap and are consolidated onto the same helper instead of a narrower ad hoc unset. CHANGELOG.md folds these into the two stat/env entries covering the whole PR. Verified: all seven touched gates ALL PASS; claudeconfigdircheck.sh reproduces the review's exact leak scenario clean with CLAUDE_CONFIG_DIR exported ambiently; --quality-delta gating=0; determinism + xmllint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…in codexpromptroutecheck Twelve gates independently hand-rolled the same detect-GNU-vs-BSD-stat fix (a `stat -f ... || stat -c ...` one-liner never reaches its `-c` fallback on Linux, because GNU's `-f` is a different, valid flag — filesystem stat, not BSD's format string — so it succeeds with junk instead of failing). test/lib/statcompat.sh centralizes the flavour detection and provides ready-to-call functions (mtime_of/inode_of/ mode_of/size_of/inode_mtime_of) so sourcing it replaces each file's own copy-pasted block; two gates (evictioncheck.sh, qsnapprefetchcheck.sh) keep a one-line local wrapper for their own fallback-string convention. mcpeditmodecheck.sh's separately-named `file_mode` was the same function under a different name; renamed its call sites to `mode_of` instead of keeping a second name for one thing. codexpromptroutecheck.sh's `skills install --codex --hook` calls only override HOME/CODEX_HOME/AGENTS_HOME, not RIPWIRE_DATA_HOME — a real write into an ambient store path once the embedded-skills feature (redhat-et#225) lands, same class of leak as the CLAUDE_CONFIG_DIR incident test/lib/unset-agent-env-variables.sh documents on that branch (not yet on main). test/lib/clean-env.sh is this PR's main-side equivalent, scoped to the vars gates on main actually read. Verified: full pargates suite, alone and under added CPU contention; --quality-delta gating=0; determinism + xmllint clean. The three gates that fail under -j6 parallel load (legendcoveragecheck.sh, pargatescheck.sh, rootrelemitcheck.sh) all pass alone and are unrelated to any file this commit touches — confirmed pre-existing/ environmental, not introduced here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…heck's own hermetic leak Addresses the two before-merge items from PR redhat-et#298's review: - cacheisolationcheck.sh, qsnapproducercheck.sh, sidecarsymlinkcheck.sh and tempfilesymlinkcheck.sh hand-rolled the same GNU-vs-BSD stat detect-once block this PR already extracted from twelve other gates. All four now source test/lib/statcompat.sh instead. - claudeconfigdircheck.sh's "UNSET IS UNCHANGED" arm relied on CLAUDE_CONFIG_DIR being absent from the environment rather than clearing it, so an ambiently exported CLAUDE_CONFIG_DIR made the gate write real files into it and then fail against its own contaminated baseline. It now sources test/lib/clean-env.sh first, same as skillinstallcheck.sh and hermesinstallcheck.sh, which had the identical gap and are consolidated onto the same helper instead of a narrower ad hoc unset. CHANGELOG.md folds these into the two stat/env entries covering the whole PR. Verified: all seven touched gates ALL PASS; claudeconfigdircheck.sh reproduces the review's exact leak scenario clean with CLAUDE_CONFIG_DIR exported ambiently; --quality-delta gating=0; determinism + xmllint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
s0undt3ch
force-pushed
the
test-hermetic-fixes
branch
from
September 20, 2026 11:34
21f5e59 to
308a988
Compare
Contributor
Author
|
@joyful-ii-V-I thank you for the review. It's ready for another go. |
joyful-ii-V-I
pushed a commit
that referenced
this pull request
Sep 20, 2026
…IT_* in the shared helper
`git -C DIR` changes the working DIRECTORY and does not override the ENVIRONMENT, so
GIT_DIR/GIT_WORK_TREE/GIT_COMMON_DIR/GIT_INDEX_FILE/GIT_OBJECT_DIRECTORY/
GIT_ALTERNATE_OBJECT_DIRECTORIES/GIT_PREFIX all outrank it. A gate that builds a
throwaway repo and asks it a question answers from the CALLER's repository whenever one
of those is exported -- a git hook running the suite, a CI job, a rebase running it per
commit -- and then passes or fails on data it never selected.
Measured at the reported call shape, with GIT_DIR exported:
git -C "$REPO" init -q -> $REPO has NO .git at all; the init, the config, the add
and the COMMIT all landed in the ambient repository
HEADSHA="$( git -C "$REPO" rev-parse HEAD )" -> f4a36c95, a commit in someone
else's tree, read back as the fixture's own
and with test/lib/clean-env.sh sourced first, the same six lines give $REPO its own .git
and HEADSHA=10d21e20, the fixture's.
SWEEP: 157 gates initialise a git repository; 2 had already found this independently and
hand-rolled it at the call site (dispatchordercheck: GIT_DIR GIT_WORK_TREE
GIT_INDEX_FILE; pagingsweepcheck: those three plus GIT_COMMON_DIR), neither carrying the
object-directory names -- the #298 shape exactly. So 155 were exposed, and the clearing
goes into the helper #298 introduced rather than a 156th copy. Both hand-rolled copies
folded onto it, each keeping its own reason as a pointer.
NEW GATE test/gitenvhermeticcheck.sh, five registrations: (A) the defect is live on this
git, or the gate exits 2 rather than conclude; (B) the helper fixes the rev-parse, and
each of the seven names is cleared; (C) the helper's list is derived and compared with a
pin, both families together; (D) a DERIVED sweep -- every test/*.sh that inits a repo
must source the helper -- with a control that strips the source line from a real gate and
requires the sweep to flag the copy. Registered in regression.sh (gate count re-derived
645 -> 646 through docs/gatecount_build.py), and in binoverridecheck's EXEMPT list with
its reason: it binds no ripwire binary.
qdrefpaircheck (E) re-pinned 9 -> 8 with its reason in the RE-PIN LOG: the row that left
is exactly the one train 12 added, `curl | http_call` at test/mcpremotecheck.sh:51 -- an
all-`test/`-pathed-`.sh` clone group, which is the population fix 2's new skip exempts.
This gate greps src/quality.h and was missed by the first round's gate selection.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
Hi @s0undt3ch — this was in great shape by the time we got to it; you'd already pushed a follow-up We did extend the sweep one step further, per the task: past the gates the review named, Landed on Landed in #306 (integration train 13), now on |
pt-act
pushed a commit
to pt-act/ripwire
that referenced
this pull request
Sep 21, 2026
…heck's own hermetic leak Addresses the two before-merge items from PR redhat-et#298's review: - cacheisolationcheck.sh, qsnapproducercheck.sh, sidecarsymlinkcheck.sh and tempfilesymlinkcheck.sh hand-rolled the same GNU-vs-BSD stat detect-once block this PR already extracted from twelve other gates. All four now source test/lib/statcompat.sh instead. - claudeconfigdircheck.sh's "UNSET IS UNCHANGED" arm relied on CLAUDE_CONFIG_DIR being absent from the environment rather than clearing it, so an ambiently exported CLAUDE_CONFIG_DIR made the gate write real files into it and then fail against its own contaminated baseline. It now sources test/lib/clean-env.sh first, same as skillinstallcheck.sh and hermesinstallcheck.sh, which had the identical gap and are consolidated onto the same helper instead of a narrower ad hoc unset. CHANGELOG.md folds these into the two stat/env entries covering the whole PR. Verified: all seven touched gates ALL PASS; claudeconfigdircheck.sh reproduces the review's exact leak scenario clean with CLAUDE_CONFIG_DIR exported ambiently; --quality-delta gating=0; determinism + xmllint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pt-act
pushed a commit
to pt-act/ripwire
that referenced
this pull request
Sep 21, 2026
's own sweep PR redhat-et#298 (@s0undt3ch) centralized the GNU-vs-BSD stat trap and, after the review posted publicly on this PR, pushed a follow-up commit closing the hermetic leak in claudeconfigdircheck.sh plus skillinstallcheck.sh/hermesinstallcheck.sh (test/lib/clean-env.sh). The task asked to extend that fix past the single call site the review reproduced, so I swept the rest of test/*.sh for gates that call skills/install.sh or scripts/install.sh with only HOME= varied per invocation, leaving CODEX_HOME/AGENTS_HOME/HERMES_HOME/CLAUDE_CONFIG_DIR/ RIPWIRE_DATA_HOME free to leak in from an ambient export the same way claudeconfigdircheck.sh did. Found and closed six more: - test/hookcheck.sh, test/routehookcheck.sh — both call `install.sh --hook` (the Claude path, reads CLAUDE_CONFIG_DIR) varying only HOME=. - test/agenttablecheck.sh, test/codexinstallhonestycheck.sh, test/meterdisclosurecheck.sh — same install.sh --hook / skills-install shape, CLAUDE_CONFIG_DIR left uncleared on at least one call site each. - test/releaseinstallcheck.sh already unset CODEX_HOME/AGENTS_HOME/HERMES_HOME for scripts/install.sh, which reads the identical five-variable set (confirmed: scripts/install.sh:375/383/391 read CLAUDE_CONFIG_DIR/ CODEX_HOME-or-AGENTS_HOME/HERMES_HOME the same way skills/install.sh does) but was missing CLAUDE_CONFIG_DIR and RIPWIRE_DATA_HOME from its unset line. Five sourced test/lib/clean-env.sh (the helper PR redhat-et#298 already introduced); releaseinstallcheck.sh already had a bespoke unset line for installer-specific vars (RIPWIRE_NO_ACTIVATE etc.), so it keeps that line and gains the two missing names instead of switching shape. Red-first, reproduced the exact PR redhat-et#298 review scenario against hookcheck.sh: with CLAUDE_CONFIG_DIR exported ambiently, the pre-fix script wrote a real settings.json + skills/ into that directory and then failed its own "file created" assertion against the sandboxed path (rc=1, `install.sh --hook: .../hookhome/.claude/settings.json not created`). The fixed script leaves the ambient directory untouched and reports ALL PASS under the same export. No other test/*.sh has the same call-then-check shape against these five variables (checked hookcheck.sh's stat -f fixture-string hit separately — literal test data for a different gate, not a live stat call, matching PR redhat-et#298's own note). codexdoctorcheck.sh and similar Codex-only gates always pass --codex, which reads CODEX_HOME/AGENTS_HOME, not CLAUDE_CONFIG_DIR, and already set those explicitly per call. Verified: all 25 gates PR redhat-et#298 touches or that I touch here pass, plus manifestcheck/gateexitcheck/gatecountcheck (no new gate files). No source changes; test-only. Credit: @s0undt3ch (PR redhat-et#298, statcompat.sh/clean-env.sh + the first four migrated gates and the original claudeconfigdircheck.sh fix). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pt-act
pushed a commit
to pt-act/ripwire
that referenced
this pull request
Sep 21, 2026
…fest pin on the merged tree fix 3 re-captures a Java/Kotlin `import` as @reference.import, which removes the only reference train 12's redhat-et#60 module-scope mint had to own in a .kt file. kotlincheck's §1 counts fall back to 3/31/14, its census row to symbols="23", and §1a inverts: the arm that pinned "exactly ONE module-scope owner, owning the import line" now pins ZERO owners, the import still VISIBLE on --uses with role="import", and no <file-scope> in_id anywhere in the fixture. §7a's mutation arm asserts --callers=square falls 1 -> 0 again, keeping train 12's named-edge form, which is the better arm either way. queries/java/tags.scm's conflict resolved toward fix 3 with train 12's redhat-et#60 consequence note kept as the record of the call this answers. qschemetrip.hash re-derived with UPDATE_GOLDEN=1: kParserVer 118 -> 119 and computeDelta's two new clone-group demotions both move manifest text. NO kQSnapCacheScheme bump, measured rather than argued -- a blob written by ae6e3e7 is refused by this build cold and warm ("cache ...-lean.bin: parser-version -- not used"), and both runs answer 31/14 against the base's 32/15. printf_parity.manifest: help_all only (UPDATE_GOLDEN_EXPECT matched), from fixes 1 and 4's --help lines. docs/COMMANDS.md regenerated through docs/docs_commands_build.py. CHANGELOG: the four honesty fixes, and the three contributions credited to @pt-act (redhat-et#291), @s0undt3ch (redhat-et#298) and @llvm-x86 (redhat-et#302). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this PR exists
While chasing an unrelated CI failure on #293, I found that
stat -f 'FMT' FILE 2>/dev/null || stat -c 'FMT' FILE— a one-liner meant to be portable across GNU coreutils and BSD/macOS — silently does the wrong thing on Linux. GNU's-fis a different, valid flag (report on the filesystem, not the file), so it succeeds with a filesystem-info block instead of failing, and the-cfallback never fires. The caller ends up comparing junk (block/inode counts that legitimately drift under concurrent I/O) instead of the field it actually asked for.That exact trap had already been independently rediscovered and fixed, by hand, in twelve separate gate scripts — each with its own near-identical comment explaining the same lesson. This PR centralizes the fix instead of leaving a thirteenth copy waiting to happen.
Separately, while auditing for a related class of bug (env vars read before
HOME, capable of leaking a real write into the operator's machine — the same failure mode as a documented past incident), I foundcodexpromptroutecheck.sh'sskills install --codex --hookcalls overrideHOME/CODEX_HOME/AGENTS_HOMEbut notRIPWIRE_DATA_HOME. That's inert onmaintoday (the current shell installer doesn't read it), but becomes a real leak once the embedded-skills feature (#225) lands and starts reading it. Fixed proactively, in the same PR, since it's the same category of gap this cleanup is already touching.What changed
test/lib/statcompat.sh(new): detects GNU vs. BSD/macOSstatonce, providesmtime_of/inode_of/mode_of/size_of/inode_mtime_ofas ready-to-call functions. Callers just source it — no per-file branching on the flavour.cachehashcheck.sh,cachesplitcheck.sh,clonecachecheck.sh,evictioncheck.sh,g1freshcheck.sh,headsnapcachecheck.sh,mcpeditmodecheck.sh,portablecachecheck.sh,prcontextcheck.sh,qsnapcachecheck.sh,qsnapprefetchcheck.sh,statgatecheck.sh) source it instead of hand-rolling their own detect-once block. Two of them keep a one-line local wrapper for their own caller-specific fallback string (|| echo 0,|| echo "MISSING").mcpeditmodecheck.sh's locally-namedfile_modewas the exact same function asmode_ofunder a different name — call sites renamed rather than keeping two names for one thing.statgatecheck.shhad a dead, uncalledmtime_nshelper (a leftover from before this cleanup) — removed rather than migrated.test/lib/clean-env.sh(new): unsets the agent-home-relocating env vars (CODEX_HOME/AGENTS_HOME/HERMES_HOME/CLAUDE_CONFIG_DIR/RIPWIRE_DATA_HOME) before a gate variesHOME=per invocation.codexpromptroutecheck.shnow sources it and passesRIPWIRE_DATA_HOMEexplicitly at itsskills install --codex --hookcall sites.Verification
test/pargates.pyrun, twice, plus once more under added CPU/I-O contention (20yesprocesses + 10 concurrent gate copies) — the exact conditions that made the original stat bug reproducible in the first place.--quality-deltagating=0, determinism (diff -qon two runs) clean,xmllint --nooutclean.-j6parallel load on this machine regardless of this branch (legendcoveragecheck.sh,pargatescheck.sh,rootrelemitcheck.sh) — all pass individually, confirmed pre-existing/environmental (reproduced identically on unmodifiedmain), not touched by anything in this diff.🤖 Generated with Claude Code
Summary by CodeRabbit