fix(security): extract tars without an unfiltered fallback; repair secret-scan

[dizhaky]

Status: everything within my control is green. The one check deliberately not cleared is Review label gate, which wants a ci-reviewed label because this PR touches .github/workflows/secret-scan.yml. That gate exists so a person reviews CI changes, and self-applying it would hollow it out — most of all here, where that workflow turned out to hold a long series of defects, one of which let it pass while scanning nothing and another of which left unredacted credentials on the runner. The remaining red checks are main's; see Verification.

What this PR does

The body is rewritten, not amended, whenever it drifts. Earlier versions described diffs that no longer exist and counts that are now wrong; keeping them would mislead the reviewer this gate exists to inform.

It began as a tarslip fix for the psutil sdist extraction, from a local CodeQL run over main. main then refactored that extraction into something structurally safer, so the psutil half is dropped. What remains:

area	change
agent/file_safety.py	assert_safe_tar_members removed; safe_extract_tar added
agent/curator_backup.py	now its only consumer
hermes_cli/main.py	_infer_stepfun_region parses the host instead of substring-matching
pyproject.toml / uv.lock	requires-python floored at 3.11.4
.github/workflows/secret-scan.yml	extensive repair (below)
scripts/ci/diff_gitleaks_findings.py	new — content-keyed finding comparison
two main bugs	swept in because they gate this PR

Thirty-three rounds of Codex review followed. The large majority of findings were defects in my own fixes, several of them in fixes from the immediately preceding round. Every one was reproduced before being fixed, and each is recorded with its evidence in docs/system-log/2026-08-08.md and in the per-round comments on this PR. I am not quoting a total defect count, because I have twice quoted one that was wrong.

---

1. The tar guard was replaced, not patched again

The original fix validated members and then called an unfiltered extractall. Review found three separate bypasses:

#	bypass	why the check missed it
1	clean name, escaping symlink target, later member written through it	it only looked at names
2	a/b/link -> ../victim as a hardlink	tarfile resolves hardlinks against the extraction root, not the link's parent
3	a -> ., then a/b, then a/b/link -> ../../outside	an earlier member changes what a later path means — lexical depth 2, real depth 1

The third ends the approach: containment depends on what earlier members created, which no check over member metadata can know. I had been fixing instances and leaving the class intact.

So safe_extract_tar uses the stdlib data filter where it exists and otherwise writes each member itself, never calling extractall.

The destination pre-pass, and why it is gone

Rounds 22–26 each produced a P1 in the same pre-pass. It was inspecting the destination to neutralize hazards — a pre-existing hardlink at a leaf, a symlink already on disk, a Windows directory junction — and it kept being wrong, because it was trying to predict where members land, the thing bypass #3 proves cannot be done from member metadata. One "fix" measured worse than the bug it replaced: an appended invalid member disabled the pass, and the hardlink was truncated anyway.

Round 26 stopped patching and read curator_backup.rollback() properly. It already moves every top-level entry aside except .hub and .curator_backups, and snapshot_skills() excludes both. So refusing members under those two names means extraction only ever writes paths that do not exist yet, and the entire destination-hazard class disappears — including the Windows reparse-point code I could not test on this platform, which is deleted rather than shipped unverified.

The validator is now entirely member-metadata-based: member type, path safety under both separator readings, preserved-name refusal, representable mtimes, no duplicate destinations, no member beneath a symlink member, and hardlink sources that must be an earlier regular-file member under every reading of the source name. No member type short-circuits any rule — three separate rounds found a bypass that was exactly one type skipping one check.

Normalization: three axes, and refusal is not identity

.HUB/x resolves onto an existing .hub on Windows and default macOS. a\link is one component to PurePosixPath and two to os.path on Windows. é composed and é decomposed are one file on macOS. Each of the three arrived as its own round.

The rule that came out of it, and the one most likely to be got wrong next:

Refusal rules ask "could this reach somewhere forbidden?" and should consider every reading, because over-refusing costs nothing. Identity rules ask "are these two members the same file?" and must use the host's real semantics.

Applying "every reading" uniformly regressed an earlier finding — it made demo/a\b and demo/a/b collide as duplicates, and on POSIX those are two legitimate, different files. _canonical_readings and _host_identity are separate named functions with that distinction in their docstrings. Both directions are pinned by tests.

The last round moved a rule across that line rather than adding one. "Could a later member have replaced my hardlink source?" is a refusal question, and it had been keyed by identity — so on macOS, where normcase is the identity function but the filesystem is case-insensitive, a symlink named demo/a could replace a regular demo/A while a hardlink kept the stale provenance. Provenance now folds; duplicate-destination still does not.

2. agent/curator_backup.py — a claim from this PR's first version, retracted

It stated curator_backup.py was "already correctly defended". That was wrong — identical shape, same gap, and it extracts into the skills directory.

---

The secret-scan workflow

This PR's own gitleaks (diff) failed, and the cause was the workflow added earlier in the same session. Every defect was found by extracting the step from the YAML and running it, never by reading it.

Things that made it pass without examining its input: an empty pathspec (git errors, gitleaks reports a partial scan of ~0 bytes and exits 0); a || true that swallowed every failure two grep patterns did not name; a $(dirname) that stripped a trailing newline back out of a NUL-safe path so the blob was silently dropped; generated pathspecs read as pathspec magic; and a whole event type — pushes to the default branch — routed around the content pass entirely.

Things that made it fail on content it did not introduce: merging main in made it re-scan main; squash-merged history made the merge base stale; a target that moved on made the tip wrong; a rename already merged had no source to baseline against; the branch's own earlier findings were re-reported on every later push; and — the most recent — a duplicate selector record with a different src truncated a baseline an earlier record had populated, because redirection empties its target before git show runs.

Things that were simply missed: merge-conflict resolutions (gitleaks never attributes them to a commit); symlink blobs; deleted files as a baseline for content that moved out of them.

Two structural results came out of it:

• The content pass needs two bases, not one. DIFF_BASE (merge base) selects which files. CONTENT_BASE (target tip) supplies the exemption baseline. One commit cannot answer both questions, and trying produced a false positive in each direction across two rounds.
• Comparison is content-keyed, not location-keyed. --baseline-path matches on location plus Entropy, a rounded float over the secret's characters, so a different secret at the same path/rule/line is suppressed whenever the two collide — which any permutation does exactly. That is precisely the merge-resolution case the pass exists to catch. diff_gitleaks_findings.py compares (RuleID, Secret) as a multiset.

Unredacted reports were written as siblings of the temp dirs the trap removed, while the comment beside them claimed otherwise. Verified: the old code left 603-byte reports containing real findings on the runner.

---

Two pre-existing main failures, swept in because they gate this PR

• tests/acp/test_session.py:291 — an unclosed list literal. It fails ruff check on main, and pytest could not collect the file, so its 15 tests were silently dead.
• plugins/memory/memgw/__init__.py — bare read_text()/write_text(), the repo's documented Windows footgun.

Type of Change

• 🔒 Security fix

How to Test

bash scripts/run_tests.sh tests/agent/test_file_safety_tar_members.py \
  tests/agent/test_curator_backup.py \
  tests/scripts/test_diff_gitleaks_findings.py \
  tests/hermes_cli/test_stepfun_region_inference.py

Verification

194 tests, 0 failed across those four files.

The extraction tests assert the property — extract for real, then look at the filesystem — rather than that a particular check fires, because the two paths legitimately differ in mechanism. Every case runs against both, since the fallback is otherwise unreachable on a modern interpreter, and that parameterization is what has caught most of the divergences.

Positive controls, each reverting exactly one thing:

reverted	result
unfiltered fallback reinstated	9 fail, incl. the chained-symlink bypass
hardlink base selection	exactly the 2 hardlink tests
Windows parsing → POSIX-only	exactly the 7 Windows tests
mode sanitization helper	exactly the 4 mode tests
int() on mtimes	exactly the 3 fractional-mtime cases
multiset → set membership	exactly the 2 multiplicity tests
Unicode fold → bare casefold()	exactly the 2 refusal cases, both paths
hardlink provenance → host-keyed identity	exactly the 2 case-alias cases, both paths
symlink creation made inline	nothing — which disproved my claim that ordering was the guarantee

The workflow is verified by execution against thirteen purpose-built repositories, re-run on every change:

scenario	expected	result
this branch, PR and push	pass	exit 0
pre-existing finding in a touched file	pass	exit 0
new credential in that same file	fail	exit 1
credential only in a merge resolution	fail	exit 1
credential in a unicode-named path	fail	exit 1
tracked file named :(literal)foo	fail	exit 1
pure rename, and a rename already squash-merged	pass	exit 0
rename carried through a merge, duplicate records	pass	exit 0
squash-merged history / behind-main / file-to-directory refactor	pass	exit 0
credential moved out of a deleted file	pass	exit 0
the same, but not present on main	fail	exit 1
merge pushed straight to the default branch	fail	exit 1
branch's own earlier finding, later unrelated push	pass	exit 0

Every workflow change is checked by running all thirteen against the unmodified step and the changed one, and confirming only the intended line moves.

Full suite against a same-machine origin/main baseline, both under scripts/run_tests.sh:

	files with failures	failing tests
origin/main, the branch's failing files re-run	46	108
this branch, full suite	46	108
unique to this branch	0	0

25,798 tests passing. The pyproject.toml floor change was checked against the two metadata tests specifically, since it could plausibly move them: their failures are byte-identical on both sides (an aiohttp and cbor2 pin drift on main).

Two corrections to this PR's own earlier verification claims, since both were published:

• A pytest tests/hermes_cli comparison was quoted as evidence. That was invalid — AGENTS.md requires scripts/run_tests.sh. Under the wrapper there were zero such failures; I had compared two invalid runs against each other.
• "ruff check . clean" rested on an exit code I had not checked. uv run ruff check . exits 2 in this sandbox — the local uv cannot parse the repo's uv.lock schema, before ruff runs at all. Invoking ruff directly with --config pyproject.toml gives the real answer, which is clean.

Open items I pushed back on, with evidence

• "Rollback breaks for symlinked skills." Real but pre-existing: filter="data" raises AbsoluteLinkError on the same snapshot, so it was already failing on main for every Python ≥3.11.4.
• "The multi-file pathspec guard always fires." It doesn't — grep is line-oriented, so [[:space:]] never sees the record separator. Adopted the recommended NUL parsing anyway, for an unrelated quoting bug.
• "Directory modes are widened unlike the filtered path." Implemented, then reverted: data does not restore them either. That conclusion was right and the generalisation I drew was wrong — I wrote the observed 0755 into the code as a constant, and it was only correct under umask 022. Now the umask decides.
• Two comparator findings are mutually unsatisfiable. They differ only in whether head's lines coincide with the parents'; any rule separating them stops reporting the moved secret that is the comparator's founding requirement. Kept the over-reporting direction; the trade-off is written into the module docstring rather than argued here.
• Probing the destination filesystem for case and Unicode sensitivity. Recommended as the fix for the macOS identity gap above; declined for the same reason the Windows reparse-point code was deleted in round 26 — it adds a write to the destination and a platform-dependent branch whose interesting behavior cannot be executed here. Refusal-side folding closes the same attack, is platform-independent, and is testable.

Known red checks that are main's, not this branch's

check	cause
web / check	hoisted react 19.2.8 vs react-dom 19.2.7; 4 files fail at import, 147 tests pass
apps/desktop / check:test:desktop:platforms	electron: 41.10.3 vs build.electronVersion: 40.10.2 — see below
ui-tui / check	TTY-dependent assertions (mouse watchdog, backpressure, resize)
Python slices	carry main's failing-file set; e.g. NameError: name 'prog_args' is not defined at hermes_cli/gateway.py:4100

The desktop one is a real repo bug, not environmental noise, and is worth someone's attention independently of this PR. apps/desktop/package.json declares the dependency as 41.10.3 while build.electronVersion still reads 40.10.2, so electron-builder would package a different Electron than npm ci installs — exactly the drift desktop-electron-pin.test.ts was written to catch. It arrived in 7e38fa5 (#155, a dependabot bump). Which way to align it is a shipping decision, so it is not fixed here.

Follow-ups not in this PR

• Extraction is not atomic. A staging tree with an atomic swap would make a partially-failed rollback impossible; it belongs in curator_backup, not in this helper. This is the last item I know of on this path.
• Snapshot/rollback does not round-trip an absolute-target symlinked skill — belongs in snapshot_skills().
• The psutil sdist download has no checksum — pinning one would be trust-on-first-use here.
• This branch should be recreated from main after each squash merge.
• CodeQL is advisory, not required. Auto-merge merged fix(secrets): recover hardening orphaned when PR #106 was closed #130 over a red CodeQL check.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aff3410e32

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Validate link targets before using the unfiltered fallback

On Python 3.11.0–3.11.3, which satisfies the project's >=3.11 requirement, a malicious archive can use a safe-named symlink such as psutil-7.2.2/link -> ../../outside followed by psutil-7.2.2/link/file; every member.name passes this check, but the unfiltered extraction writes file outside the temporary directory. The identical fallback in scripts/install_psutil_android.py has the same gap, so validate symlink/hardlink targets and member types before either fallback rather than treating the name check as equivalent to the data filter.

Useful? React with 👍 / 👎.

[dizhaky]

Correct, and fixed in 480caf4. I reproduced it before changing anything — built exactly the archive you describe and ran it through the name-only guard on the fallback path:

name-only guard: PASSED all members (did not fire)
file written OUTSIDE dest: True

The guard now resolves link targets against the link's own directory and requires containment, rather than blanket-rejecting .. (which would refuse legitimate internal links like sub/link -> ../README). It also refuses non-regular members — devices, fifos — which data does too.

Two things beyond what you flagged:

agent/curator_backup.py had the same gap, and my PR description explicitly cleared it as "already correctly defended". That was wrong — identical name-only + unfiltered-fallback shape. It also extracts into the skills directory rather than a temp dir, so the gap mattered more there than in the site CodeQL originally flagged. Now fixed.

Deduplicated. There were three copies of this check drifting apart, so the implementation now lives in agent.file_safety.assert_safe_tar_members and is shared by main.py and curator_backup.py. scripts/install_psutil_android.py keeps a standalone copy — it runs from install.sh on a fresh checkout where the package isn't importable — with a sync note on both sides.

Covered by three new tests (escaping symlink, absolute link target, and a benign internal relative link that must still be allowed). Positive control: reverting the source files fails 5 of the 21 tests.

---

Generated by Claude Code

[chatgpt-codex-connector]

Handle malformed endpoint overrides without aborting setup

When STEPFUN_BASE_URL or model.base_url contains a malformed netloc, such as an unmatched IPv6 bracket (https://[foo) or an NFKC-invalid delimiter, urlparse(...) or .hostname raises ValueError. _model_flow_stepfun passes these unvalidated user-configured values directly into this function, so the provider setup now aborts before displaying the region picker, whereas the previous substring implementation always returned a region; catch parsing errors and default to international.

Useful? React with 👍 / 👎.

[dizhaky]

Correct — fixed in 480caf4. Verified the raise:

>>> urlparse("https://[foo").hostname
ValueError: Invalid IPv6 URL

You've identified the important part: my change made a total function partial. The substring version never raised, so _model_flow_stepfun could always show its region picker; mine would abort setup on a malformed STEPFUN_BASE_URL or model.base_url before the user got a chance to correct it. That's a regression I introduced while fixing a cosmetic issue — worse than the thing being fixed.

Now catches ValueError and defaults to international, with a comment explaining why the fallback has to be total rather than propagating. Three new parametrized cases (https://[foo, https://[::1, http://[), all failing before the fix.

---

Generated by Claude Code

[chatgpt-codex-connector]

Re-run verification through the required test wrapper

The recorded full-suite verification invokes pytest directly, bypassing the canonical runner's clean environment and per-file subprocess isolation; the five ordering-dependent failures described immediately below are direct evidence that this run does not match CI. Re-run the comparison through scripts/run_tests.sh tests/hermes_cli before treating these counts as verification.

AGENTS.md reference: AGENTS.md:L1127-L1131

Useful? React with 👍 / 👎.

[dizhaky]

You're right, and this is the most useful of the three — I broke a documented rule and then presented the output as verification.

Re-ran through the wrapper:

$ scripts/run_tests.sh tests/hermes_cli
=== Summary: 241 files, 5261 tests passed, 0 failed (100% complete) in 93.4s (8 workers) ===

Zero failures. The five I reported don't exist under CI parity — they were an artifact of the bypass, exactly as you inferred from their ordering-dependence. My conclusion ("not caused by this change") happened to be right, but I reached it by comparing two invalid runs against each other, which is not verification. Had those five been caused by my change, the same flawed method would have concluded the same thing.

The system-log entry in this PR has been corrected rather than quietly edited — it now records the original claim, why it was invalid, and the real numbers, so the mistake is legible to whoever reads that log next.

Worth noting the wrapper also explains a detail I'd rationalised away: I told myself CI "never sees" those flakes because run_tests_parallel.py isolates per file. True, but the right conclusion was then my local run isn't CI-equivalent and shouldn't be quoted as verification — not therefore they don't matter.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 480caf4810

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Resolve hard-link targets from the archive root

On Python 3.11.0–3.11.3, the unfiltered fallback remains exploitable because tar hard-link targets are interpreted relative to the extraction root, not the link member's parent. For example, a nested hard link such as a/b/link -> ../victim, followed by a regular member with the same name, passes this calculation as a/victim but can hard-link and overwrite a file beside the temporary directory. This is fresh evidence beyond the earlier link-target report: the new validator groups hard links with symlinks and therefore applies the wrong resolution semantics; the duplicated validator in scripts/install_psutil_android.py needs the same correction.

Useful? React with 👍 / 👎.

[dizhaky]

Correct, and it was exploitable end to end. I built the archive before changing anything:

GUARD: accepted
extractall OK
hardlinked to outside victim: True
outside victim.txt now: PWNED

The guard passed it, extractall created a link sharing an inode with a file outside the destination, and writing through the link overwrote that file.

The mechanism is exactly as you describe, and I confirmed it in the stdlib rather than taking it on trust — TarFile._extract_member sets tarinfo._link_target = os.path.join(path, tarinfo.linkname) with path the extraction root, whereas a symlink is handed to os.symlink verbatim and read by the kernel relative to the link's own directory. Fixed by choosing the base per link type. A test asserts the asymmetry directly: the same linkname is now refused as a hardlink and allowed as a symlink, which is the property the bug violated.

Positive control: reverting only that one-line base selection fails exactly the two hardlink tests and nothing else.

On the duplicated validator in scripts/install_psutil_android.py — it is gone rather than corrected. main landed a refactor while this PR was open: hermes_cli/psutil_android.prepare_patched_psutil_sdist never calls extractall, copying file members individually and refusing every non-file member, so link members cannot exist in its output. Both psutil call sites now use it and this branch no longer touches either file. That is a better fix than the one this PR proposed, since it removes the unfiltered fallback instead of guarding it. agent/curator_backup.py is the remaining consumer of the guard, and it does still need one.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 67f47903ca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve supported symlinked skills during rollback

On Python 3.11.0–3.11.3, a supported skill such as ~/.hermes/skills/demo -> /work/demo is stored by snapshot_skills() as an absolute-target symlink, but this new precheck rejects that snapshot before extraction, changing a previously successful rollback into a failure for the entire skills tree. Symlinked checked-out skills are explicitly supported in agent/skill_utils.py, so either preserve link-only entries while preventing later members from traversing them, or exclude/dereference them when creating the snapshot rather than making rollback unusable.

AGENTS.md reference: AGENTS.md:L114-L117

Useful? React with 👍 / 👎.

[dizhaky]

The mechanism is real, and I reproduced it — but the impact attribution is wrong, and I don't think this is a regression introduced here.

snapshot_skills() uses tf.add(entry, arcname=entry.name, recursive=True), which does not dereference, so a skill symlinked to an absolute path is indeed stored as an absolute-target symlink. Confirmed:

member 'demo' issym=True linkname='/…/work/demo'

The part that changes the conclusion is what the existing happy path already does with that snapshot. curator_backup extracts with filter="data" first, and data refuses absolute link targets itself:

--- filter="data" (the path taken on >=3.11.4, before this PR) ---
  AbsoluteLinkError: 'demo' is a link to an absolute path
  is TarError? True
--- this guard ---
  rejected -> refusing to extract link 'demo' -> '/…/work/demo': absolute target

AbsoluteLinkError subclasses TarError, so it lands in the same except (OSError, tarfile.TarError) recovery branch. On Python ≥3.11.4 — every version CI runs, and the overwhelming majority of installs — rolling back a snapshot containing an absolute-target symlinked skill was already failing on main before this PR. What the guard changes is 3.11.0–3.11.3, where it makes the behaviour consistent with every other interpreter instead of silently doing something different.

So the underlying bug is real and worth fixing: snapshot/rollback does not round-trip a supported symlinked skill. But the fix belongs on the snapshot side — dereference on capture, or record link entries in the manifest and restore them explicitly — not in the extraction guard, whose job is precisely to refuse absolute link targets. Loosening it to admit them would reintroduce the escape this PR exists to close, and would do so for every consumer, not just this one.

Filed as a follow-up rather than folded in here; happy to be pointed at a preferred shape for it. If the round-trip gap is worth closing before this merges rather than after, say so and I'll do it as a separate change against snapshot_skills().

One correction to your framing: this can't be "a previously successful rollback" on any interpreter that has the data filter, which is where the claim's severity came from.

---

Generated by Claude Code

[chatgpt-codex-connector]

Validate tar paths with native Windows semantics

On native Windows with Python 3.11.0–3.11.3, a crafted curator snapshot containing a member such as ..\outside or C:\outside passes this PurePosixPath check, while the fallback extractall() interprets backslashes and drive-qualified paths using Windows path rules and writes outside the skills directory. The same mismatch affects link targets normalized with posixpath; use a destination-root containment check with the host platform's path semantics before invoking the unfiltered fallback.

Useful? React with 👍 / 👎.

[dizhaky]

Correct — fixed. The two readings of a stored path really do disagree, and the guard was only doing one of them.

Names are now rejected if either POSIX or Windows parsing says they escape, and link targets are resolved with \ also treated as a separator. Seven cases added, covering ..\outside, skills\..\..\outside, C:\outside, \\server\share\outside, the drive-relative C:outside, plus a backslash link target and a drive-qualified one. Positive control: reverting to POSIX-only parsing fails exactly those seven and nothing else.

Two notes on the shape of the fix, since it isn't quite what you suggested:

It validates unconditionally rather than switching on the host platform. What decides whether a name is dangerous is the archive, not the machine reading it — and the same snapshot can be written on one platform and restored on another. Checking both flavours everywhere is also the cheaper thing to test, since the Windows cases then run in Linux CI.

The cost, stated rather than hidden: this refuses a filename that genuinely contains a backslash, which is legal on POSIX. For a security guard on snapshot restore that is the right direction, but it is a real (if remote) behaviour change rather than a free win.

I did not switch to a realpath-based containment check against the destination root. That resolves through the live filesystem, so its answer depends on what already exists at the destination — for a pre-flight check over member metadata, a pure-path check on both flavours is the more predictable primitive.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1940adb300

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Resolve symlink parents through earlier archive links

On Python 3.11.0–3.11.3, this fallback containment check remains bypassable because base is derived from the member's lexical name without resolving earlier symlink members. An archive containing a -> ., directory a/b, symlink a/b/link -> ../../outside, and file a/b/link/pwned passes the guard because the second target normalizes inside the lexical tree, but unfiltered extractall() creates the link under the actual <dest>/b and writes pwned into an existing directory beside the destination. I reproduced the guard accepting this archive and the outside file being created; validation must account for previously created links, or the unfiltered fallback must be removed.

AGENTS.md reference: AGENTS.md:L84-L87

Useful? React with 👍 / 👎.

[dizhaky]

Reproduced, and you've convinced me the whole approach was wrong. I built your archive before touching anything:

GUARD: ACCEPTED
extractall OK
file written OUTSIDE dest: True
   found at: ./outside/pwned

Taking the second half of your suggestion: the unfiltered fallback is removed. This is the third bypass of the same lexical check — escaping symlink targets, then hardlink resolution semantics, now an earlier member changing what a later path means. That is not a fourth thing to patch. Containment depends on what earlier members created, and a check over member metadata cannot know that. Each round I fixed the instance you found and left the class intact.

assert_safe_tar_members is gone. safe_extract_tar uses the stdlib data filter where it exists (3.11.4+) and otherwise writes each member itself, creating only directories and regular files — never calling extractall. Nothing that can redirect a later path is ever created, so the traversal question is moot rather than answered correctly. That is the same conclusion main reached independently for the psutil sdist in hermes_cli/psutil_android, which I should have followed when I first saw it instead of continuing to patch a validator.

One deliberate divergence, asserted by a test so it can't drift silently: data permits a symlink whose target stays inside the tree; the fallback refuses every link member. Supporting them safely means resolving through links already created — exactly the complexity that produced all three bypasses. It affects 3.11.0–3.11.3 only; on 3.11.4+ the real filter runs and internal links still work.

The tests were rewritten to assert the property rather than the mechanism — extract for real, then check the filesystem for anything outside the destination — with all three historical bypasses pinned as scenarios. Both code paths run everywhere, since the fallback is otherwise unreachable on a modern interpreter.

Positive control: reinstating the unfiltered fallback fails 9 tests, including your chained-symlink case, all on the manual-fallback variant.

---

Generated by Claude Code

[chatgpt-codex-connector]

Materialize symlink blobs before the content scan

When a merge conflict resolves a tracked path to a symlink, this pass does not scan the content Git actually stores for that path: [ -f "$f" ] follows the link and skips it when its target is dangling, while a non-dangling link is dereferenced by cp, so the target file is copied instead of the symlink blob (cp --help confirms that only -P, --no-dereference avoids following source links). Because the preceding history scan is explicitly unable to see merge-only resolutions, a credential committed as the symlink target can therefore escape both passes; materialize the HEAD blob or readlink output as a regular file for scanning.

AGENTS.md reference: AGENTS.md:L606-L609

Useful? React with 👍 / 👎.

[dizhaky]

Correct on both halves, and I verified each:

src/link_ok:       passes [ -f ] → copied
src/link_dangling: SKIPPED by [ -f ]
dst/link_ok:       regular file, content=SECRETVALUE   ← the target's content, not the link blob

So the live link got its target scanned and the dangling one got skipped entirely — and since the log pass provably can't see merge-only resolutions, that was a genuine hole between the two passes.

Fixed by taking your first suggestion: the pass now materializes git show <rev>:<path> instead of copying from the working tree. That gives the bytes git actually stores, uniformly, for every kind of tracked path — no -P vs default question, no dereference, and no dependence on whether a target happens to exist in the checkout.

Verified on a repo with a symlink whose target string is a credential (mode 120000 in the index):

secret_link blob = sk-ant-api03-96ipbNClShVP...

and the step exits 1 on it.

---

Generated by Claude Code

[chatgpt-codex-connector]

Restrict the content pass to newly introduced content

When a PR changes a file that already contains a detector hit, this pass copies the file in full and invokes a --no-git scan with no base revision or patch information, so the old finding is reported again even when the edited lines are unrelated. The workflow's own preceding comments describe this as scanning final working-tree content, and the added system log records 126 findings when pre-existing files were included; merely narrowing the set of files does not preserve the documented diff-only behavior. Compare findings against the base version or scan a generated resolution diff so existing credentials in touched files do not make otherwise unrelated PRs fail.

AGENTS.md reference: AGENTS.md:L606-L609

Useful? React with 👍 / 👎.

[dizhaky]

Fair, and the distinction you're drawing is the right one: narrowing the set of files is not the same as scanning only new content. I'd noted the re-report risk in passing and then not acted on it, which left a real way for an unrelated PR to fail on someone else's finding.

Taking your first option — compare against the base version. gitleaks has native support (--baseline-path), so the pass now builds a baseline by scanning the base revision of the same files and only fails on findings absent from it.

Two details turned out to be load-bearing, and I only found them because the first attempt silently suppressed nothing:

1. Fingerprints embed the file path. Scanning two temp dirs by absolute path produced /tmp/tmp.7n8e83Br9c/preexisting.py:anthropic-api-key:1 in the baseline and a different prefix at HEAD, so nothing matched. Both scans now run from inside their directory with --source ., giving preexisting.py:anthropic-api-key:1.
2. --redact rewrites the recorded Secret. A baseline built without it matches nothing even when the paths line up. Both scans now use identical flags.

With either wrong the check still looks fine — it just reports everything as new. Both are now called out in a comment at the call site.

Verified as a matrix, running the extracted step over purpose-built repos:

scenario	expected	got
pre-existing finding in a touched file, nothing new	pass	exit 0
a new credential added to that same file	fail	exit 1
new credential as a symlink target	fail	exit 1
credential only in a merge resolution	fail	exit 1 (log pass clean, content pass caught it)

The second and fourth rows are the ones that matter for your concern — the baseline suppresses the old finding without also suppressing a new one in the same file.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: acef811f94

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Reject pre-existing symlinks during manual extraction

On Python 3.11.0–3.11.3, the manual path still escapes when the destination already contains a symlink: rollback deliberately preserves .hub, so a snapshot member such as .hub/link/pwned follows an existing .hub/link -> /outside at this open() and writes outside the skills tree. Fresh evidence beyond the earlier archive-member-link report is that no link member is required in the archive—the redirect exists before extraction. Resolve or reject existing path components, or extract into a genuinely empty directory, before writing.

AGENTS.md reference: AGENTS.md:L84-L87

Useful? React with 👍 / 👎.

[dizhaky]

Correct, and this is the sharpest of the set — the redirect doesn't have to be in the archive at all. Reproduced with your exact scenario, an archive containing no link members whatsoever:

archive members contain NO link members: True
extraction: completed
outside/victim.txt now: PWNED

I had narrowed "what can redirect a path" to the archive and missed that the destination is equally a source of it — and .hub surviving rollback is precisely the case that makes it reachable rather than theoretical.

Fixed by walking each path component with O_NOFOLLOW relative to its parent directory fd, so an existing symlink anywhere along the path raises instead of being followed:

extraction: REFUSED — refusing to extract through 'link': the destination path
component is a symlink or not a directory (Not a directory)
outside/victim.txt: ORIGINAL

I chose the component walk over "extract into a genuinely empty directory" because the empty-dir route only relocates the problem: curator_backup has to end with the content in skills/, and the move back would traverse the same pre-existing .hub/link. Refusing to traverse a symlink is the property that actually needs to hold, so it's enforced where the write happens.

Platform caveat, stated rather than papered over: Windows has neither O_NOFOLLOW nor dir_fd support, so it keeps plain path operations. That is unchanged from before this PR rather than a new gap, and it's called out in the code — but it is a gap.

Positive control: dropping the O_NOFOLLOW walk fails exactly this test and nothing else.

---

Generated by Claude Code

[chatgpt-codex-connector]

Keep multi-file PR pathspecs enabled

When a PR changes two or more files, files contains newline separators, so this [[:space:]] test always returns early and leaves PATHSPEC empty. In the long-lived, squash-merged branch scenario this block was added to handle, the history scan therefore remains unscoped and re-reports findings from stale branch commits instead of scanning only the current diff; distinguish whitespace inside a filename from record-separator newlines, preferably with NUL-delimited parsing.

AGENTS.md reference: AGENTS.md:L606-L609

Useful? React with 👍 / 👎.

[dizhaky]

This one I don't think is right — grep is line-oriented, so [[:space:]] never sees the record separator. Tested directly:

$ printf 'a.txt\nb.txt\n' | grep -q '[[:space:]]' && echo YES || echo NO
NO                       # newline is not seen as a space

$ printf 'a.txt\nb c.txt\n' | grep -q '[[:space:]]' && echo YES || echo NO
YES                      # a real space inside a filename still is

And on this PR's own 9-file diff the guard did not fire — the earlier run logged the pathspec in full:

Scanning PR diff: 7e38fa5..HEAD -- .github/workflows/secret-scan.yml agent/curator_backup.py agent/file_safety.py …

So the multi-file case was working. That said, the NUL-delimited parsing you recommend is now in place anyway, for the reason in your other comment — the default output quotes non-ASCII paths — and the whitespace test moved to per-path (case "$f" in *[[:space:]]*)) rather than testing the joined blob. That's more obviously correct than relying on grep's line semantics, even though the old form happened to work.

The change also picked up something neither of us had flagged: set_pathspec now includes both sides of a rename. Limiting the walk to the destination hides the source, so git can't pair them and renders a rename as a wholesale addition — see the rename thread.

---

Generated by Claude Code

[chatgpt-codex-connector]

Baseline renamed files against their source paths

For a pure rename, f is the destination path and ${CONTENT_BASE}:${f} does not exist, so this command leaves an empty baseline file while the full renamed HEAD blob is scanned. Any detector hit that already existed before the rename is consequently reported as newly introduced, making an otherwise content-neutral rename fail the diff-only check. Fresh evidence beyond the earlier touched-file report is this old-path/new-path mismatch; resolve rename pairs and materialize the source blob under the destination path for baseline comparison.

AGENTS.md reference: AGENTS.md:L606-L609

Useful? React with 👍 / 👎.

[dizhaky]

Correct, and chasing it down turned up a second half you didn't mention that made the first half insufficient on its own.

Your point, confirmed: for a pure rename ${CONTENT_BASE}:${dst} doesn't exist, the baseline comes out empty, and every pre-existing finding in the moved file reads as new. Fixed by using git diff -M --name-status -z and materializing the base side from the source path, written under the destination path so the fingerprints line up.

But that alone still failed the scenario. Testing a pure rename of a file containing a credential:

### pure rename of a file with a pre-existing finding (expect PASS)
leaks found: 1     <- still failing
EXIT=1

The failure was in the log pass, ahead of the content pass. Because the walk is path-scoped to the change, limiting it to the destination hides the source, so git can't pair them and renders the rename as a wholesale addition:

BASE..HEAD -- fixture_new.py                    leaks found: 1
BASE..HEAD -M -- fixture_new.py                 leaks found: 1     # -M can't help; source out of scope
BASE..HEAD -M                                   no leaks found     # unscoped, pairs fine
BASE..HEAD -- fixture_old.py fixture_new.py     no leaks found     # both sides in scope

So set_pathspec now emits both sides of a rename. That's a case my own path-scoping introduced, and I'd have shipped it without your comment prompting the rename scenario.

Current matrix, all via the extracted step:

scenario	expected	result
pure rename of a file with a pre-existing finding	pass	exit 0
pre-existing finding in an edited file	pass	exit 0
new credential added to that same file	fail	exit 1
new credential in a unicode-named path	fail	exit 1
credential only in a merge resolution	fail	exit 1

---

Generated by Claude Code

[chatgpt-codex-connector]

Read content-scan paths with NUL delimiters

When a changed path contains non-ASCII bytes, tabs, or newlines, Git's default --name-only output quotes and escapes the pathname, so the loop passes that display representation to git show, which fails and skips the blob. I confirmed the documented -z behavior from git diff -h (NUL-terminated output) and reproduced résumé.txt being emitted as "r\303\251sum\303\251.txt" without it. A secret introduced only in a merge resolution for such a path therefore escapes both scans because the history pass cannot see merge-only content and this content pass never materializes the file; use git diff --name-only -z with a NUL-delimited read.

AGENTS.md reference: AGENTS.md:L606-L609

Useful? React with 👍 / 👎.

[dizhaky]

Confirmed and fixed. Reproduced your résumé.txt case exactly:

=== default --name-only ===
renamed.txt
"r\303\251sum\303\251.txt"

=== does git show accept that literal string? ===
  OK      renamed.txt
  FAILED  "r\303\251sum\303\251.txt"  <-- blob skipped

=== with -z ===
  OK      renamed.txt
  OK      résumé.txt

The || continue made it silent, which is the part that bothers me most — the loop's own error handling turned a coverage hole into a clean-looking run. Both reads are now -z with read -r -d ''.

Verified end to end: a new credential committed into résumé.txt now fails the step (exit 1), where before it would have been skipped by the content pass and invisible to the log pass if it arrived via a merge resolution.

---

Generated by Claude Code

[chatgpt-codex-connector]

Preserve hardlinked files in curator snapshots

On Python 3.11.0–3.11.3, an ordinary snapshot containing two hardlinked regular files cannot be rolled back: tarfile.add() automatically stores the second inode occurrence as an LNKTYPE member, and this branch rejects it even though the archive was produced by snapshot_skills() itself. Fresh evidence beyond the earlier crafted-hardlink-target report is this automatic archive behavior; the previous fallback and the modern data filter both restore a contained hardlink successfully. Materialize a safe hardlink member as a regular-file copy, or make snapshot creation store duplicate file contents, so supported interpreters can restore their own backups.

AGENTS.md reference: AGENTS.md:L1124-L1128

Useful? React with 👍 / 👎.

[dizhaky]

Correct — and this is the one that shows the cost of the "refuse every link member" simplification I made last round. I'd justified it as affecting only crafted archives; it also breaks ordinary ones.

Reproduced with a snapshot built exactly the way snapshot_skills() builds one, from a skills tree containing two hardlinked files:

  'demo'          isfile=False islnk=False linkname=''
  'demo/a.txt'    isfile=True  islnk=False linkname=''
  'demo/b.txt'    isfile=False islnk=True  linkname='demo/a.txt'   <- tarfile.add() deduped the inode

-- manual fallback --
  REFUSED: refusing to extract non-regular member 'demo/b.txt' (type b'1')

So the backup was unrestorable on precisely the interpreters that path exists to serve — a worse outcome than the bug I was guarding against, since it fails on legitimate input rather than hostile input.

Taking your first option: a safe hardlink is now materialized as a copy. Same content, and unlike a link a copy cannot be used to reach anything else, so it doesn't reopen the escape. The linkname goes through the same validation as a member name (hardlink targets are root-relative, as established earlier in this PR), and the source is read through the same O_NOFOLLOW walk — so the crafted a/b/link -> ../victim case from the earlier round is still refused:

extraction: OK
  a.txt: exists=True content=content
  b.txt: exists=True content=content
  materialized as a copy, not a link (distinct inodes): True

I chose the copy over changing snapshot creation because it keeps the fix inside the interpreter-specific fallback: on 3.11.4+ the data filter restores a genuine hardlink as a hardlink, and I'd rather not make every snapshot larger to accommodate a path most installs never take.

Positive control: reverting to refusing link members fails exactly the two hardlink tests.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd460a8369

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Detect secrets that replace baselined findings

When a merge-conflict resolution replaces an existing flagged fixture/token with a real token at the same path, rule, and starting line, the history scan cannot see the merge-only resolution, while --baseline-path suppresses the new finding using gitleaks' location-based fingerprint; redacting both reports also prevents the changed secret from distinguishing them. That lets newly introduced credentials pass this nominally diff-scoped check, so compare the base and resolved secret content before redaction or scan the actual resolution diff rather than treating the base finding as a blanket exemption.

AGENTS.md reference: AGENTS.md:L606-L609

Useful? React with 👍 / 👎.

[dizhaky]

Right conclusion; the mechanism is slightly different and worse than described. Fixed.

Redaction isn't what lets it through — a baseline entry is distinguished by Entropy, which survives --redact. My first test of your scenario actually caught the replacement, and diffing the two records showed why:

field        baseline                        head                            same?
Fingerprint  fixture.py:anthropic-api-key:1  fixture.py:anthropic-api-key:1  True
Secret       REDACTED                        REDACTED                        True
StartLine    1                               1                               True
Entropy      5.416892                        5.3360677                       False   <- the only discriminator

So the whole defence rests on a rounded float over the secret's characters. Which means any permutation of the baselined value collides exactly:

original : sk-ant-api03-FlJnZ6usuZSbFwwez...
permuted : sk-ant-api03-abBtTs5zejNazNhlm...   (different value, same multiset)

baseline entropy: 5.416892
head     entropy: 5.416892
identical: True
-> no leaks found

A different, real credential, fully suppressed. Not a theoretical collision — a one-line shuffle.

Taking your first option: --baseline-path is gone, replaced by scripts/ci/diff_gitleaks_findings.py, which compares the two reports on (RuleID, Secret). Reports are generated unredacted so the values can be compared, and are never echoed — only path/line/rule of a new finding is printed, with a test asserting the secret reaches neither stdout nor stderr.

Worth noting what dropping location from the key also buys, since it cuts the other way too: a secret that merely moved (line shift, or a rename) is no longer reported as new. Location-keying got both cases wrong — it reported every shift and missed every in-place replacement.

Verified: the entropy-colliding replacement is now detected; unchanged files, moves and removals pass; malformed or non-list reports fail closed rather than reading as "nothing found". 11 unit tests plus the end-to-end workflow matrix.

---

Generated by Claude Code

[chatgpt-codex-connector]

Restore archived directory permissions

On Python 3.11.0–3.11.3, this manual branch creates every archived directory with _walk_dirs' hard-coded 0755 and never applies member.mode. Consequently, rolling back a snapshot containing a private 0700 skill directory can make it world-readable under a standard umask, unlike the filtered extraction path and the regular-file branch; retain directory members and apply their sanitized modes after extracting their children.

AGENTS.md reference: AGENTS.md:L1124-L1128

Useful? React with 👍 / 👎.

[dizhaky]

I implemented this, then reverted it — the premise doesn't hold, and I'd rather show why than quietly keep a change built on it.

The claim rests on "unlike the filtered extraction path". filter="data" does not preserve archived directory modes:

stdlib data filter -> skills/private mode: 0o755      # member.mode was 0o700

So the manual branch's hard-coded 0755 was already matching the filtered path, not diverging from it. Restoring modes only in the fallback would have made a restored snapshot's permissions depend on the interpreter — 0700 on 3.11.0–3.11.3, 0755 everywhere else — which is exactly the divergence this function exists to eliminate. I'd briefly introduced that, and the test I wrote to prove the fix is what caught it: it failed on the stdlib-filter variant, not the fallback.

The test now asserts parity between the two paths rather than a specific mode, so it fails if either side drifts — the stdlib starting to preserve modes, or the fallback diverging.

The observation underneath your comment is still real and I don't want to bury it: a private 0700 skill directory does come back world-readable after a rollback. That's the stdlib's behaviour on every supported version, though, not something this PR introduces and not something to fix asymmetrically in a fallback. Worth raising as its own change against curator_backup if the skills tree is expected to hold private directories — happy to, separately.

(For the record, the data filter's own rationale for dropping directory modes is that a restrictive mode on the way down would block writing the directory's own children; it restores nothing rather than sequencing the chmods. My reverted version did sequence them deepest-first, so it worked — it was just the wrong thing to want.)

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0899a17fbd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Refuse redirects when dir-fd support is unavailable

On native Windows with Python 3.11.0–3.11.3, _HAVE_DIR_FD is false, so this branch falls back to ordinary pathname operations and _write_file() subsequently follows directory junctions or symlinks. A crafted curator snapshot containing .hub/link/pwned can therefore write outside the skills tree when the preserved .hub/link redirects elsewhere. Fresh evidence beyond the earlier supposedly fixed destination-symlink report is that the new Windows-specific fallback explicitly bypasses all no-follow enforcement; reject existing reparse points or extract into a genuinely empty directory on platforms without dir-fd support.

AGENTS.md reference: AGENTS.md:L84-L87

Useful? React with 👍 / 👎.

[dizhaky]

Fair hit, and the framing is the part I got wrong.

I had written that branch off as "unchanged from before rather than a new gap" and said exactly that in the docstring. But preserving prior behaviour isn't the standard for a function whose entire purpose is to guarantee the property — and a branch that silently performs no enforcement is worse than one that admits it can't, because the caller cannot tell the difference. My previous reply called it "a gap, stated rather than papered over"; stating it was not the same as closing it.

It now raises rather than falling through:

refusing to extract: this interpreter predates tarfile's 'data' filter
(added in 3.11.4) and this platform cannot open paths without following
links, so extraction cannot be made safe against a redirect already
present in the destination. Upgrade to Python 3.11.4 or newer.

I chose refusing over your second suggestion — extract into a genuinely empty directory — for the same reason as last round: curator_backup has to end with the content in skills/, so the move back traverses the same junction and the problem just relocates. And I chose refusing over a lexical reparse-point check because I cannot test Windows behaviour from here, and shipping unverifiable security code is a habit worth not starting.

The refused combination is narrow: an interpreter predating filter="data" (3.11.0–3.11.3) and a platform without dir_fd. On 3.11.4+ the filter runs and the walk is never reached. Failing to restore a backup is a poor outcome; restoring it through a junction that redirects outside the skills tree is a worse one, and the error names the fix.

One thing your comment prompted beyond the fix: the parent is None branches in _write_file and _copy_within are gone, and _walk_dirs now returns int rather than int | None. Those unreachable fallbacks are how the hole got in — the type said "this may not be enforced" and the code obliged.

Two tests: extraction is refused when _HAVE_DIR_FD is false and nothing is written; the filtered path still succeeds under the same condition, since only the fallback is affected.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e9a333bb11

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Count duplicate findings when baselining secret values

When a merge-only resolution adds a second occurrence of a credential that already exists in the same changed file, the base report contains one (rule, secret) key and the HEAD report contains two, but this set-membership check suppresses both HEAD findings. Because the workflow's history scan explicitly cannot see merge-only resolutions, that newly added credential passes the check; compare occurrence counts as a multiset so only the number present at the base is exempted. This is fresh evidence beyond the earlier in-place replacement report: switching to value-based keys fixed replacement detection but discarded finding multiplicity.

AGENTS.md reference: AGENTS.md:L606-L609

Useful? React with 👍 / 👎.

[dizhaky]

Correct, and your framing of it is the useful part: switching to value keys fixed replacement detection and discarded multiplicity in the same move. I traded one blind spot for another and only tested the one I'd set out to fix.

Occurrences are now counted rather than matched — only as many as existed at the base are exempt, and the surplus is reported:

remaining = Counter(key(f) for f in base)
for finding in head:
    k = key(finding)
    if remaining[k] > 0:
        remaining[k] -= 1
        continue
    new.append(finding)

Three tests, because the boundary matters in both directions: a second copy of an existing secret is new (exit 1); equal counts on both sides are all exempt (exit 0); and with two at the base and three at HEAD, exactly one is reported rather than three — the count has to be right, not just non-zero.

Positive control: reverting to set membership fails exactly the two multiplicity tests.

---

Generated by Claude Code

[chatgpt-codex-connector]

Apply data-filter permission sanitization to regular files

On Python 3.11.0–3.11.3, this manual path applies the archive's regular-file mode almost verbatim, whereas the promised filter="data" semantics strip group/other write bits, ensure owner read/write, and clear execute bits unless owner-execute was set. A snapshot member with mode 0777, for example, therefore restores as world-writable only on these supported interpreters, leaving executable skill content modifiable by other local users; sanitize the mode before passing it to _write_file and _copy_within. This is distinct from the rejected directory-mode report: the data filter intentionally ignores directory modes but explicitly sanitizes regular-file modes.

Useful? React with 👍 / 👎.

[dizhaky]

Correct, and your distinction from the directory-mode report is exactly the right one — I checked rather than assumed this time, having got the directory case wrong by assuming last round.

Measured across the two paths:

archive   data filter   manual (before)
   0777          0755              0777
   0666          0644              0666
   0755          0755              0755
   0600          0600              0600
   0400          0600              0400
   0444          0644              0444

Four of six disagreed, and the 0777 row is the one that matters: world-writable executable skill content, on 3.11.0–3.11.3 only.

The stdlib rule, from tarfile._get_filtered_attrs, is mode &= 0o755, then clear all execute bits if the owner had none, then mode |= 0o600. Both call sites now go through one helper implementing that, applied to hardlink copies too.

Checked exhaustively rather than on the six cases above — the rule is three lines, so there's no reason to sample. All 512 modes in 0o000–0o777 now agree with the stdlib byte for byte, and a second assertion pins the two invariants directly (no group/other write survives, owner rw always does) so the test still means something if the stdlib rule shifts.

Positive control: reverting the helper fails exactly the four file-mode cases.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cbd1f2db11

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Invalidate hardlink sources after member replacement

On Python 3.11.4+, a crafted rollback archive can write regular member a, replace a with a symlink to a preserved hardlink such as .hub/x, create hardlink b -> a, and then write a regular member at b; because a remains in seen_regular, validation accepts the hardlink and the filtered extractor follows the alias, overwriting the external inode linked through .hub/x. I reproduced this on Python 3.11.15. Fresh evidence after the earlier source-type fix is that a source can have been regular earlier but no longer be regular when the hardlink is processed, so duplicate member names must invalidate prior provenance or be rejected.

AGENTS.md reference: AGENTS.md:L84-L87

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Validate preserved-path directory members

When a rollback archive contains a directory member such as .hub/injected, this early return skips both path validation and the refuse_top_level check; filter="data" then accepts and creates the directory inside the preserved .hub tree, and rollback reports success without cleaning it up. I reproduced the directory persisting after safe_extract_tar returns, so directory members need the same preserved-name validation before this short-circuit to keep curator rollback from mutating hub-managed state.

AGENTS.md reference: AGENTS.md:L1140-L1143

Useful? React with 👍 / 👎.

[dizhaky]

Round twenty-eight — both confirmed and fixed (9ea2869)

agent/file_safety.py:757 — directory members skipped every check · P2

Confirmed:

A  dir '.hub/injected' created? True

The directory short-circuit ran before the preserved-name check, so a DIRTYPE member landed inside the tree rollback exists to leave alone, and rollback reported success.

Worth noting why none of the existing tests caught it: every redirect test asserts that the outside file is untouched, and nothing is written through a bare directory. The property being violated here is different — curator rollback must not mutate hub-managed state at all, whether or not anything escapes. The new test asserts that directly.

This is the second directory short-circuit bug in two rounds (the first was timestamps). Both had the same shape: a continue placed before checks that were not actually type-specific. The validation that applies to every member now runs before it.

agent/file_safety.py:794 — provenance invalidated by replacement · P1

Also confirmed, and it defeats last round's source-type rule exactly as you describe:

regular  a          -> a recorded as a regular-file member
symlink  a -> .hub/x   (same name)
hardlink b -> a        -> passes: `a` is in seen_regular
regular  b             -> follows the alias
outside: 'PWNED'

a was a regular file when the hardlink was validated and a symlink by the time extraction used it.

I did not fix this by invalidating provenance on replacement, because that leaves the general problem — any conclusion drawn about a path can be undone by a later member with the same name, and I would be re-deriving which conclusions survive each time a new one is added. Duplicate destinations are refused outright. tarfile.add() walks a tree once, so a real snapshot never contains one; the existing test_a_real_archive_of_repo_content builds a tarball from actual files and would fail if that rule were too strict for ordinary archives.

That is the round-26 move again: prefer the rule that removes the possibility over the one that tracks it.

---

170 tests, ruff check clean.

Where this stands

The extraction validator is now, in full: member type, path safety under both separator readings, preserved-name refusal, representable timestamps, no duplicate destinations, no member beneath a symlink member, and hardlink sources that must be an earlier regular-file member. Every one of those is decided from the members alone, before anything is written — which is the boundary round six established and the pre-pass kept violating.

The outstanding follow-up is unchanged: extraction is still not atomic, so a rollback that fails partway leaves a partial tree. That is a curator_backup concern and wants staging with an atomic swap. It is the last item I know of on this path that is not addressed here.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7f20dabded

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Deduplicate moved parent occurrences across paths

When a feature branch moves an existing credential from old to new but Git reports D old plus A new rather than a rename, secret-scan.yml places the target occurrence in the deleted-file report under old and the previous-head occurrence under new. Keying the parent union by file counts those as two exemptions, so a merge resolution that adds a second copy produces two HEAD findings, consumes both exemptions, and passes; the range scan cannot recover merge-only content. Reconcile cross-path occurrences that the branch moved instead of treating every parent path difference as a distinct inherited secret.

AGENTS.md reference: AGENTS.md:L604-L609

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Split Windows member paths before tracking symlinks

On native Windows with Python 3.11.4+, _filter_destination_parts() treats backslashes as ordinary characters, so an archive containing symlink a\link -> ..\.hub followed by regular member a\link\x passes validation: the recorded symlink tuple ('a\\link',) is not considered a prefix of ('a\\link\\x',). The stdlib extractor uses Windows separators, creates the contained redirect into preserved .hub, and then writes through it; if .hub/x is hardlinked to an outside file, that inode is overwritten. Track member ancestry using the destination platform's path components before invoking the filtered extractor.

AGENTS.md reference: AGENTS.md:L1140-L1143

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Normalize preserved names on case-insensitive filesystems

On Windows and default case-insensitive macOS filesystems, an archive member such as .HUB/x resolves to the existing preserved .hub/x, but _top_level_names() returns .HUB and the case-sensitive intersection with refuse_top_level misses it. The stdlib data filter accepts the path because it remains inside the extraction root, then overwrites hub-managed state; an existing hardlink at that leaf can also propagate the write outside the skills tree. Compare preserved top-level names using the destination filesystem's case semantics.

AGENTS.md reference: AGENTS.md:L1140-L1143

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Reject NUL-containing archive paths before extraction

When a corrupt or crafted PAX archive supplies a long member path containing an embedded NUL, PurePosixPath accepts it here and _validate_members() completes successfully, but both the stdlib filtered extractor and the fallback filesystem operations raise ValueError: embedded null byte. rollback() catches only OSError and TarError, so this exception bypasses recovery after the current skills have been moved into staging, leaving them absent from the live tree. Reject NULs during member validation or translate this extraction error into TarError.

AGENTS.md reference: AGENTS.md:L1124-L1128

Useful? React with 👍 / 👎.

[dizhaky]

Round twenty-nine — three fixed, one declined on the same grounds as round 27 (2c9fe2b)

Three of these are the same shape: the preserved-name boundary from round 26 is only as good as the set of spellings that reach it. Each spelling below reached the preserved tree by a route the check did not recognise.

agent/file_safety.py:714 — case · P1

.HUB/x resolves onto the existing .hub on Windows and default macOS; the comparison was case-sensitive. Folded now. Folding unconditionally can only over-refuse, and only for an archive containing a differently-cased .hub — not a skill name, so the cost is nil.

agent/file_safety.py:746 — separators in ancestry · P1

a\link -> ..\.hub then a\link\x. I applied "check both separator readings" to the preserved-name check in round 27 and did not apply it to the symlink-ancestry check three lines away, so the symlink was recorded as the single component a\link, which is not a prefix of the single component a\link\x. Both readings are tracked now.

agent/file_safety.py:694 — embedded NUL · P1

Confirmed, with one detail worth recording because it decides how the test has to be written: a short name truncates at the NUL, so the naive fixture proves nothing.

member name: 'demo/a'          # short name — NUL terminated the field
survives NUL: True | len 127   # long name — PAX length-prefixed record

Only the PAX long-path record carries it through, exactly as you specified. ValueError: embedded null byte is neither OSError nor TarError, so it escaped rollback()'s recovery with the tree already staged. Rejected during validation now, and the test asserts the premise ("\x00" in member.name) before asserting the behaviour.

scripts/ci/diff_gitleaks_findings.py:151 — declined, same conflict as round 27

The mechanism you describe is real: with D old + A new, the target's occurrence lands in the deleted report under old and the previous head's under new, and keying per file counts two. But the rule that fixes it collides with round 24:

Rule: exempt only from parent paths that also appear in HEAD

  r12 moved secret (want 0)        got 1  want 0  <-- FAILS
  r24 disjoint inherited (0)       got 0  want 0  OK
  r29 moved old->new + dup (1)     got 1  want 1  OK

It satisfies this finding and round 24 and breaks round 12 — a secret that merely moved being reported as introduced, which is the founding reason this comparison is content-keyed rather than location-keyed.

That is the second independent finding to land on the same wall. Round 27's was "same file, different lines"; this one is "different files, same secret". Both ask the comparator to decide whether two parent occurrences are one credential seen twice or two credentials — and the reports do not carry that information. Every rule I have tested trades one of these three requirements for another.

I have kept the direction that over-reports, for the reason already in the module docstring: a false failure costs a human one glance at a diff-scoped report naming file and rule; a miss is the thing this pass exists to prevent. If the noise becomes real in practice, the fix is upstream — the workflow knows old → new was a delete-plus-add and could reconcile there, where the rename information still exists, rather than asking the comparator to infer it from two JSON files.

---

178 tests, ruff check clean. Twelve workflow fixture classes unchanged.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dd6ccc21fc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Scan all commits after a default-branch rewrite

When a force-push to the default branch publishes multiple commits and the old BEFORE_SHA is not present in the checkout, this fallback scans only HEAD~1..HEAD; the content pass also uses HEAD~1 as its base. A credential added in an earlier pushed commit and unchanged by the final commit is therefore missed by both passes. Fresh evidence beyond the earlier new-branch report is that the feature-branch merge-base fallback is explicitly unavailable when REF_NAME == DEFAULT_BRANCH, so default-branch rewrites still take this one-commit path.

AGENTS.md reference: AGENTS.md:L604-L609

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Reject NUL bytes in symlink targets

When a PAX archive supplies a symlink linkpath containing an embedded NUL, the member name passes _validate_members, but the filtered extractor or deferred os.symlink raises ValueError: embedded null byte. rollback() catches only OSError and TarError, so this bypasses recovery after the live skills have been moved into staging. I reproduced this with pax_headers={"linkpath": "inside\0evil"} on Python 3.11.15; fresh evidence beyond the earlier NUL-member-name report is that member.linkname remains unchecked.

AGENTS.md reference: AGENTS.md:L1124-L1128

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Apply symlink-ancestor checks to directory members

When an archive contains a -> .hub followed by directory member a/injected, this early return skips the recorded symlink-ancestor check below. On Python 3.11.15, filter="data" follows the contained link, creates .hub/injected, and returns success, so rollback permanently mutates hub-managed state. Fresh evidence beyond the earlier direct-directory and regular-member cases is that directory members still bypass the symlink-prefix validation entirely.

AGENTS.md reference: AGENTS.md:L1140-L1143

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Compare symlink ancestors using filesystem case semantics

On Windows and default case-insensitive macOS filesystems, an archive containing A -> .hub followed by regular member a/x bypasses this case-sensitive prefix comparison: ('A',) is not considered an ancestor of ('a', 'x'), although both names resolve through the same filesystem entry. The stdlib data filter accepts the redirect because .hub/x remains inside the extraction root, then writes into preserved hub state; an existing hardlink there can propagate the overwrite outside the skills tree. Fresh evidence beyond the earlier direct .HUB preserved-name case is that the separately tracked symlink ancestry is still compared without case normalization.

AGENTS.md reference: AGENTS.md:L1140-L1143

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Select files changed by either merge parent

When both parents changed a file and a feature-branch merge resolution restores that file exactly to its merge-base blob, this git diff "$DIFF_BASE" HEAD produces no record, so the content pass never scans the resolution. The range pass cannot compensate because, as this workflow documents, gitleaks does not attribute merge-resolution bytes to a commit; I reproduced the selector with a two-parent merge whose resolution restored the base credential and obtained an empty merge-base-to-HEAD name-status diff. Fresh evidence beyond the earlier merge-resolution findings is the net-zero-against-merge-base case, where the newly added content pass has no HEAD file to materialize at all.

AGENTS.md reference: AGENTS.md:L604-L609

Useful? React with 👍 / 👎.

[dizhaky]

Round thirty — five findings (328ece4)

Three of the five are the same defect for the third round running: a normalization rule applied in one check and not in the one beside it.

finding	the rule that existed	where it wasn't applied
NUL in linkname	NUL rejection (r29)	member.name only
case-differing symlink ancestor	casefolding (r29)	preserved-name check only
directory under a symlink member	ancestry check (r25)	short-circuited by isdir — the third member kind to do so

Patching three more spots would have guaranteed a fourth round of it, so the shape is fixed instead. _canonical_readings() computes every reading once — both separators, casefolded — and every refusal rule compares against it. No member type short-circuits: each rule now runs for directories too.

The part worth reporting: the rewrite was initially wrong, and an existing test caught it

Applying "both readings" uniformly broke test_a_backslash_name_does_not_collide_with_a_nested_one. Judging identity across both readings made demo/a\b and demo/a/b collide as duplicates — and on POSIX those are two legitimate, different files. That is round 21's finding, which I would have regressed.

So there are two kinds of comparison here and they need opposite treatment:

• Refusal — "could this reach somewhere forbidden?" Consider every reading; over-refusing costs nothing.
• Identity — "are these two members the same file?" Use the host's real semantics only.

_host_identity() does the second, with os.path.normcase for the case half — it lowercases on Windows and is the identity on POSIX, which is exactly the required behaviour. Duplicates and hardlink sources use it; refusals use the readings. Both are now named functions with that distinction in their docstrings, because it is the thing the next change will get wrong.

Every refusal from rounds 25–30 re-verified, plus both POSIX controls:

OK  r26 .hub/x                          TarError
OK  r29 .HUB/x (case)                   TarError
OK  r29 a\link -> ..\.hub               TarError
OK  r30 dir under symlink member        TarError
OK  r30 case-differing symlink ancestor TarError
OK  r28 duplicate name laundering       TarError
OK  r25 symlink member redirect         TarError
OK  control ordinary                    extracted
OK  control backslash + nested (POSIX)  extracted

.github/workflows/secret-scan.yml:374 — net-zero merge resolution · P2

Fixed by unioning each merge parent's diff into the file set. I could not reproduce the exact case, and want to be straight about that: merging main makes main's tip the merge base, so a resolution restoring the merge-base blob also matches the target and is correctly exempt. The reachable shape needs origin/main to advance after the merge, which my fixture did not produce. The fix is cheap and strictly widens coverage, so I applied it on the mechanism rather than on a reproduction — flagging it as the one change this round not backed by a failing-then-passing fixture.

.github/workflows/secret-scan.yml:176 — default-branch rewrite · P1

Real, and I do not have a fix. On the default branch there is no other branch to take a merge base from and the old tip is gone from the checkout, so there is genuinely nothing to diff against; the alternatives are scanning one commit (a guess) or scanning all history (guaranteed red on every force-push). The one-commit fallback now emits a ::warning:: naming the gap and telling the reader to re-scan manually. That is worse than a fix and better than the silence it replaces.

---

186 tests, twelve fixture classes, ruff check clean.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 28790b3937

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Normalize Unicode before tracking symlink ancestors

On normalization-insensitive macOS filesystems, case-folding alone does not give filesystem identity: a crafted archive can contain symlink é -> .hub followed by regular member é/x using the canonically equivalent decomposed spelling. These tuples compare as different here, but the filesystem resolves the second path through the first symlink; the stdlib data filter accepts that contained redirect and writes into the preserved .hub tree, which rollback must not modify. Normalize components to a canonical Unicode form before performing ancestry and identity comparisons.

AGENTS.md reference: AGENTS.md:L1140-L1143

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Keep rollback working on supported Windows versions

On native Windows with Python 3.11.0–3.11.3, tarfile lacks the data filter and _HAVE_DIR_FD is false, so every non-empty curator rollback reaches this branch and fails before restoring its first member. The package still advertises requires-python = ">=3.11,<3.14", meaning a supported installation loses the rollback feature entirely; either provide a safe Windows extraction path or raise the declared Python floor to 3.11.4. This security mitigation currently destroys the feature it is intended to secure.

AGENTS.md reference: AGENTS.md:L114-L117

Useful? React with 👍 / 👎.

Round 31 — both findings addressed in ce2116f. One of them reverses a refusal I made in round 15.

P1: Unicode normalization (agent/file_safety.py:744)

Correct, and it is a third axis rather than a variant of case. macOS is normalization-insensitive: a symlink member named é composed (U+00E9) and a later member named é decomposed (e + U+0301) resolve through each other there while comparing as different tuples. Casefolding alone is not filesystem identity.

_caseless() now applies one NFC pass before the fold, and the order is the argument: NFC makes canonically-equivalent spellings byte-identical, so the casefold that follows is applied to the same input and cannot diverge. Folding first would not have that property — casefolding emits an unnormalized result for 26 code points, U+0390 among them.

Two things I checked rather than assumed:

• The defensive extra was dropped. My first draft emitted both NFC and NFD readings. A brute-force sweep of the whole code-point range found zero spellings where the single leading pass disagrees, so the second reading was dead weight. Reverting it alone fails no test; that is how it was caught, rather than argued about.
• NFC and not NFKC. Apple's filesystems decompose canonically, so canonical equivalence is the identity they implement. Compatibility folding would merge names the filesystem keeps apart.

Applied on the refusal side only — the third instance of last round's split. _host_identity() still leaves the two spellings distinct, because Linux stores them as two files and folding them for identity would call a legitimate archive a duplicate-destination collision. Refusal rules should consider every reading, because over-refusing costs nothing. Identity rules must not. Both directions are pinned: the refusal case and its control.

Positive control: reverting _caseless to a bare casefold() fails exactly the two new refusal cases (both extraction paths) and nothing else.

P1: requires-python floor (agent/file_safety.py:581)

This security mitigation currently destroys the feature it is intended to secure.

Right, and that framing is the reason this moved. Below 3.11.4 there is no data filter, so the manual path runs — and that path needs dir_fd to refuse traversing a symlink already in the destination, which Windows does not have. The combination refuses to extract at all, so a curator rollback there fails before its first member.

requires-python moves >=3.11,<3.14 → >=3.11.4,<3.14, with the rationale in a comment beside it. No real install is affected: CI's python-version: '3.11' and the Windows installer both resolve to the newest 3.11.x, years past .4.

I declined this in round 15 as unverifiable, because the sandbox's uv (0.8.17) could not parse this lockfile's schema and I was not willing to hand-edit a lock. That was a tooling limit, not a real one — a newer uv (0.12.3) parses it fine, the regenerated lock is one line, and uv lock --check passes either side of the change.

-requires-python = ">=3.11, <3.14"
+requires-python = ">=3.11.4, <3.14"

Verification

190 tests across the four affected files (up from 186), ruff check clean, uv lock --check clean. Round 31 is recorded in docs/system-log/2026-08-08.md.

The ci-reviewed label is still the only merge blocker, and still deliberately not self-applied — the gate exists so a person reviews the secret-scan.yml changes, which are the part of this branch most worth a human read.

---

Generated by Claude Code

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ef2240fae3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Preserve rename baselines across duplicate selector records

When a default-branch merge renames old to new while also carrying a target-side modification, the selector emits R old new against the first parent and then M new against the feature parent. The rename record correctly writes CONTENT_BASE:old to the baseline, but the later M record executes both redirections with src == dst == new; because new does not exist in CONTENT_BASE, each failed git show truncates the previously populated baseline file. Consequently, an unchanged credential inherited through the rename is reported as newly introduced, violating the workflow's diff-only contract. Avoid overwriting an already materialized destination baseline with a failed lookup.

AGENTS.md reference: AGENTS.md:L606-L609

Useful? React with 👍 / 👎.

Round 32 — the P2 is real, reproduced, and fixed in 601da1a.

Preserve rename baselines across duplicate selector records

Correct, and the mechanism is exactly as described. Redirection truncates its target before git show runs, so a failed lookup leaves an empty file rather than the previous contents. That only becomes reachable because one destination can arrive twice with a different src: the parent loop emits R old new against the parent that predates the rename and M new against one that already has it. The first record fills the baseline from old; the second looks for new in CONTENT_BASE, doesn't find it, and blanks what the first wrote.

The comment I wrote on that loop two rounds ago said duplicate records were harmless. It was right about the bytes — "the same path materializes to the same bytes twice" — and wrong about the failures, which is the half that mattered.

Reproduced before fixing, as a thirteenth workflow fixture: origin/main keeps old.py with a credential; a feature branch renames it to new.py; a sibling branch edits a different line; the merge differs from both parents, so the record stream is R old.py new.py followed by M new.py twice.

before:  content scan: 1 finding(s) in changed files, 0 already present at the base, 1 new   EXIT=1
after:   content scan: 1 finding(s) in changed files, 1 already present at the base, 0 new   EXIT=0

Fix: both base trees stage through a temp file and install with mv on success. No precedence guess is involved — any record that succeeds populates, any that fails leaves the tree alone.

Control in both directions: all thirteen fixtures re-run against the unmodified workflow and the fixed one. Exactly one line differs — the new fixture. The other twelve are byte-identical.

One thing I noticed while reproducing and chose to document rather than restructure: COUNT counts records, not distinct paths, so the fixture logs "3 file(s)" for one file. Both uses are -gt 0 gates, so it changes no behavior; the comment now says what it counts.

The red checks on this branch

Every one traced to a cause, none belonging to this branch:

Check	Cause
Review label gate	ci-reviewed absent — needs a human, by design
web / check	hoisted react 19.2.8 vs react-dom 19.2.7; 4 files fail at import, 147 tests pass
apps/desktop / check:test:desktop:platforms	electron: 41.10.3 vs build.electronVersion: 40.10.2
ui-tui / check	TTY-dependent assertions (mouse watchdog, backpressure, resize)
Python slices	NameError: name 'prog_args' is not defined at hermes_cli/gateway.py:4100

The desktop one is worth surfacing separately, because it is a real repo bug rather than environmental noise. apps/desktop/package.json declares the dependency as 41.10.3 while build.electronVersion still reads 40.10.2, so electron-builder would package a different Electron than npm ci installs — precisely the drift desktop-electron-pin.test.ts was written to catch. It landed in 7e38fa5 (#155, a dependabot bump) and is present identically on origin/main; git diff origin/main HEAD -- apps/desktop/package.json package-lock.json is empty for this branch.

I have not fixed it here. Which way to align — bump build.electronVersion to 41.10.3, or pull the dependency back to 40.x — is a desktop-shipping decision, and it does not belong as a rider on a tarfile-safety branch.

---

Generated by Claude Code

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 69543bc255

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Normalize duplicate identities on macOS filesystems

On default case-insensitive macOS filesystems, os.path.normcase() is still the identity function, so archive members demo/A and demo/a receive different identities despite naming the same entry. A crafted sequence consisting of regular demo/A, symlink demo/a -> ../.hub/x, and hardlink demo/b -> demo/A therefore bypasses claimed: extraction replaces the regular file with the symlink, then accepts the stale regular-file provenance and creates demo/b as a hardlink to preserved .hub/x. Fresh evidence beyond the earlier symlink-ancestor finding is that ancestry is now normalized, but duplicate/source identity remains host-insensitive on macOS; use filesystem-aware case and Unicode normalization for _host_identity() there.

AGENTS.md reference: AGENTS.md:L1140-L1143

Useful? React with 👍 / 👎.

Round 33 — the P1 is real and fixed in 6961a34, though not by the recommended mechanism. Reasoning below.

The finding is correct, and the rule was on the wrong side of the line

os.path.normcase is the identity function on macOS — but the default filesystem there is case-insensitive, so demo/A and demo/a are one entry that host-keyed provenance saw as two. Regular demo/A, symlink demo/a -> ../.hub/x, hardlink to demo/A: the symlink replaces the file, and the hardlink inherits a source that is no longer a file.

What makes this diagnosable rather than just another patch is that it sits exactly on the distinction the last few rounds established:

Refusal rules ask "could this reach somewhere forbidden?" and consider every reading. Identity rules ask "are these two members the same file?" and must use host semantics.

"Could a later member have replaced my hardlink source?" is a refusal question. I had it keyed by identity. So:

• seen_regular is keyed by canonical readings.
• Every non-regular member invalidates every reading it could occupy.
• A hardlink requires its source present under all of them.
• The duplicate-destination rule keeps _host_identity, because that one really is an identity question.

The invalidation runs ahead of the isdir return — a rule placed after one has been the bypass three separate times in this function.

Why not filesystem-aware probing

You recommended probing the destination for case and Unicode sensitivity. I'm not doing that, for the reason round 26 deleted the Windows reparse-point code: it adds a write to the destination and a platform-dependent branch whose interesting behavior — macOS case-insensitivity — is precisely what this sandbox cannot execute. I would be shipping security code I can only reason about, in a function that has already produced a P1 in six consecutive rounds from exactly that habit.

Refusal-side folding closes the same attack, is platform-independent, and is testable here. If the probe is wanted later it can go in as its own change, on a machine that can run it.

The cost, stated rather than buried

On Linux those three members are genuinely distinct files, so this over-refuses a legitimate archive. That is the deliberate direction — tarfile.add() cannot produce the shape, and a refused rollback fails loudly where a hardlink into .hub would not. Both directions are pinned: the attack must be refused, and a control with an unrelated symlink beside the file must still extract.

Verification

Positive control: reverting provenance to host-keyed identity fails exactly the two new attack cases, both extraction paths, and nothing else.

194 tests across the four affected files, ruff check clean.

Also re-measured the full-suite baseline on the current tree rather than carrying the old number forward — origin/main re-running this branch's failing files gives 46 files / 108 tests, the branch's full suite gives 46 files / 108 tests, zero branch-unique, 25,798 passing. The pyproject.toml floor change was checked against the two packaging-metadata tests specifically, since it could plausibly have moved them; their failures are byte-identical on both sides.

---

Generated by Claude Code