Skip to content

bench: pin ambient operator memory into the config identity - #93

Merged
bborbe merged 1 commit into
masterfrom
fix/pin-ambient-memory-in-config-hash
Aug 9, 2026
Merged

bench: pin ambient operator memory into the config identity#93
bborbe merged 1 commit into
masterfrom
fix/pin-ambient-memory-in-config-hash

Conversation

@bborbe

@bborbe bborbe commented Aug 9, 2026

Copy link
Copy Markdown
Owner

The config hash claimed to identify a configuration it did not determine.

What was found

The reviewer resolves $HOME/.claude/CLAUDE.md and obeys it. Proven by a like-for-like
re-run at the exact configuration that first exposed it — opus/xhigh/full on
quant#109. The review succeeded (8 findings, no gate failure) and still ended with
the operator's personal state-closer panel, verbatim:

📌 No task anchor — PR review (read-only)
🟢 ACTIVE · Review complete: 4 Should Fix, 4 Nice to Have, 0 blockers; precommit green
👤 You: nothing
⏰ Next: you decide whether to fix the gmail-token due_date before ...

📌 / 👤 You: / ⏰ Next: is defined only in ~/.claude/CLAUDE.md ("Async State
Closer") and nowhere in this plugin.

A green run is not evidence of a clean run. The first quant#109 run failed the
UNATTRIBUTABLE FINDING gate because a panel line was harvested as an item; the re-run
passed while carrying the same panel. The gate failure was incidental — the contamination
was present either way. Had the first run passed, nothing would have surfaced.

Why pinning rather than isolating

Isolation was attempted first and abandoned. Memory resolves through HOME — but so does
authentication. Every redirect tried returned Not logged in · Please run /login:

  • HOME = the config dir
  • HOME = a scratch dir with .claude.json symlinked
  • HOME = a scratch dir mirroring all 49 entries of ~/.claude, omitting only CLAUDE.md

claude --bare disables CLAUDE.md auto-discovery but disables OAuth, keychain reads and
plugin sync with it — and the bench depends on its verified plugin load path.
Containerising the review (the claude-yolo pattern: own HOME, credentials mounted)
would work and remains the route to cross-machine portability.

The change

ambient_memory_hash() becomes a component of config_hash, and is recorded as its own
result-row field rather than only folded into the digest — a reader comparing two rows
must be able to see which input differed, and ambient memory is the one input that
changes with no commit to this repo. An absent file hashes to the literal "none", which
is a real configuration state, not an error.

This does not make a score portable to another machine, and the docstring says so. It
restores the promise the digest actually makes: a change to operator memory now
invalidates cached rows instead of silently altering results.

Consequence

Every pre-existing ledger row carries a stale config hash. They were measured under an
unpinned condition. That is the honest outcome, not a regression.

Verification

  • make precommit green — 196 tests
  • test_config_hash_distinguishes_ambient_memory verified to fail against the
    pre-change payload
    , with the exact symptom: AssertionError: '7b9b7085…' == '7b9b7085…'
  • test_ambient_memory_hash_reports_none_when_absent covers the absent-file state

The reviewer resolves $HOME/.claude/CLAUDE.md and obeys it. Proven by a
like-for-like re-run: an opus/xhigh/full review of quant#109 ended with the
operator's personal state-closer panel, a convention defined only in that
file and nowhere in this plugin. The config hash covered rules/ + commands/
only, so it claimed to identify a configuration it did not determine.

Isolation was attempted first and abandoned. Memory resolves through HOME,
but so does auth: HOME redirected to the config dir, to a scratch dir with
.claude.json symlinked, and to a scratch dir mirroring all 49 entries of
~/.claude minus CLAUDE.md each produced 'Not logged in'. claude --bare drops
CLAUDE.md discovery but drops OAuth and plugin sync with it. Containerising
the review remains the route to cross-machine portability.

Pinning restores the promise the digest actually makes: a change to operator
memory now invalidates cached rows instead of silently altering results. It
does not make a score portable, and the docstring says so.

Recorded as its own row field, not only folded into the digest, so a reader
comparing two rows can see which input differed - ambient memory being the
one input that changes with no commit to this repo.

Every pre-existing ledger row carries a stale config hash as a result.

The discriminating test was verified to fail against the pre-change payload.

@ben-s-pull-request-reviewer ben-s-pull-request-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Step 4c-sel CLASSIFY

Changed files: CHANGELOG.md, bench/run.py, bench/test_config.py

Judgment rule candidates:

Rule Trigger Decision
changelog/conventional-prefix-required CHANGELOG.md Applicable — bullets present under ## Unreleased
changelog/unreleased-entry-required CHANGELOG.md + non-vendored changes Applicable## Unreleased section exists with valid - bullets
go-dependency-update-without-mod-tidy Go files Skipped — no .go files changed
claude-md/agent-context-not-user-docs CLAUDE.md Skipped — no CLAUDE.md changed
agent-cmd/* agents/**, commands/** Skipped — no agent/command files changed

Step 4d-sel ADJUDICATE

changelog/conventional-prefix-required — CHANGELOG bullets:

  • fix: ambient operator memory is now part of the configuration identity…fix:
  • docs: review_env records why isolation was attempted and abandoned…docs:

Both bullets use valid conventional prefixes. No violation.

Python code reviewbench/run.py and bench/test_config.py:

The ambient_memory_hash() implementation correctly hashes ~/.claude/CLAUDE.md and returns "none" on OSError. The config_hash() function properly includes it as a payload component. The two new tests are targeted and correct.

One minor observation (not a finding — no rule triggered):
build_row() calls ambient_memory_hash() directly at line 1566, and also calls config_hash() which internally calls ambient_memory_hash() again when no explicit ambient_hash is passed. The hash is computed twice in the same row construction. This is redundant work but produces correct results. The call site at line 1682 correctly lets config_hash default to the ambient memory hash.


Must Fix (Critical)

None.

Should Fix (Important)

None.

Nice to Have (Optional)

None.


Traceability Report Section (Selector Mode)

  • Candidates: 5 rules matched by Step 4b-i glob filter
  • Applicable: 2 (changelog/conventional-prefix-required, changelog/unreleased-entry-required)
  • Skipped:
    • go-dependency-update-without-mod-tidy → no .go files in diff
    • claude-md/agent-context-not-user-docs → no CLAUDE.md in diff
    • agent-cmd/agent-frontmatter → no agents/**/*.md in diff
    • agent-cmd/command-frontmatter → no commands/**/*.md in diff
    • agent-cmd/command-thin → no agent/command files in diff

Notes: The PR is well-reasoned. The CHANGELOG entry is thorough and explains the design decision clearly. The ambient_memory_hash() docstring is exemplary — it documents why the feature exists, what was tried and why it was abandoned, and what the hash does and does not promise. No violations found.

{
  "verdict": "approve",
  "summary": "Clean PR adding ambient operator memory ($HOME/.claude/CLAUDE.md) as an explicit component of config identity. Two changelog bullets use valid conventional prefixes (fix:, docs:). New ambient_memory_hash() and config_hash() overload are correctly implemented; two targeted unit tests added. No mechanical findings. Redundant ambient_memory_hash() calls within build_row() are noted as a non-blocking style observation.",
  "comments": [],
  "concerns_addressed": [
    "correctness: config_hash() ambient_hash parameter defaults to ambient_memory_hash() — call sites correctly rely on the default where no explicit value is needed",
    "correctness: ambient_memory_hash() uses pathlib.Path.home() — documented in the docstring with explanation of why isolation was attempted and abandoned; not a bug",
    "correctness: build_row() emits ambient_memory_hash as a top-level field — implemented correctly; value is consistent with what config_hash() uses internally",
    "correctness: absent file hashes to literal string 'none' — implemented with OSError catch; test confirms behavior",
    "tests: test_config_hash_distinguishes_ambient_memory and test_ambient_memory_hash_reports_none_when_absent added — both pass and correctly verify new behavior"
  ]
}

@bborbe
bborbe merged commit 462ec46 into master Aug 9, 2026
1 check passed
@bborbe
bborbe deleted the fix/pin-ambient-memory-in-config-hash branch August 9, 2026 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant