Repository navigation
Feat/opencode round - #14
Closed
joyful-ii-V-I wants to merge 6 commits into
Closed
joyful-ii-V-I wants to merge 6 commits into
joyful-ii-V-I wants to merge 6 commits into
Conversation
…an tell the wrong shape from the right one
opencode does not fit the existing emitter. Every wrapped agent so far pairs a
"mcpServers" key with a command/args pair; opencode uses a top-level "mcp" key
and an entry of {"type":"local","command":[...]} where command is a single
array. wrapMcpJson bakes the old shape into one printf literal, so this is a
sibling emitter rather than a parameterization.
The failure mode worth gating is not a crash. A recipe emitting the
familiar-but-wrong shape produces a config that parses, installs, looks right,
and is then silently ignored — so the gate parses the emitted JSON and conforms
it against opencode's own published schema, which is the first JSON-validity
check of any wrap output in this tree (the existing wrap gates are pure grep).
The schema is VENDORED and sha-pinned rather than fetched. No gate here has ever
reached the outside network, jsonschema is not installed and G3 forbids adding
it, and the published schema is unversioned with no $id while opencode ships
releases multiple times a day. Conformance is therefore read out of the pinned
McpLocalConfig definition — refreshing the pin automatically re-tightens the
assertions. test/tools/refresh-opencode-schema.sh does the refresh by hand and
is deliberately not a gate.
The recipe leads with the CLI and offers MCP second, the only entry in the table
that does. opencode ships a bash tool, and the MCP server's 30 verb schemas are
resident context every turn whether a verb is called or not, where the CLI costs
nothing until invoked; the measurement behind that ordering lands next commit.
AGENTS.md reuses the existing shared blurb verbatim — codex already points there,
and wrapverbscheck asserts the body is byte-identical across agents, so this adds
a row rather than a third rules body. wrap --all detection honours
XDG_CONFIG_HOME, since opencode resolves every path through xdg-basedir; the
fake-HOME test now clears that variable so a developer's real install cannot leak
into it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…every turn The CLI and the MCP server answer the same questions, and the MCP path costs more — but the term that dominates is the one no per-response comparison can see. The server advertises 30 verbs, and their names, descriptions and JSON schemas sit in the model's context every turn whether a verb is called or not. The CLI costs nothing until invoked. Measured: the tools/list array is 36,717 bytes, which this repository's own calibration table (serialize.h, o200k_base) converts to ~11.8K-14.7K tokens — roughly 4-5x a typical --for answer, paid per turn. It is reported as a BAND rather than a point because 55.3% of those bytes are English prose and the rest is JSON structure, so the table's measured Json rate (3.10 B/tok, from structure-heavy package.json) and its prose rate (2.56) bracket the truth. A crude bytes/4 rule reads 9,179 and is low by ~55%, which is why it is not used. Two things deliberately NOT claimed: Per-response framing is nearly free — 1.12x on the wire — so JSON-RPC escaping and round-trip overhead are noise next to the standing cost. And the two `for` paths are not the same bundle, so that 1.12x is not a content-parity ratio: on an identical corpus and query the MCP verb returned 13 ranked symbols to the CLI's 25, with no churn=/amp= quality lens and no est_tokens. The verb documents itself as a signatures-only inventory, so this is by design — but it makes the near-1:1 byte ratio a coincidence of two different products. Only the standing cost is quoted as a headline; it needs no such caveat. Where the MCP still wins is stated in the same breath: agents that cannot shell out, and a warm index across calls. The conclusion is "prefer the CLI where the agent can shell out, and here is the number", not "remove the MCP" — and it is what the previous commit's opencode recipe orders itself on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…on that says not to run the eval yet The north-star question — does giving a coding agent ripwire change whether it SOLVES the task — has never been measured. This makes the harness capable of answering it, and then shows that the current design cannot. WHY THE EVAL IS NOT WORTH FUNDING AS SCOPED (bench/agentloop/power_sim.py). analyze.py bootstraps clustered by repository, and with equal paired rows per repo that is algebraically a resample of the 6 repo-level MEAN deltas. Effective N is the CLUSTER count, not the 24 instances. Minimum detectable effect at the planned design is 18-32pp on resolution rate, and at 6 clusters a 5pp or 10pp effect is unreachable at ANY instance count once between-repo heterogeneity is non-zero — simulated out to 49,152 instances without crossing 80% power. Adding repos buys ~4x the power of adding instances inside the existing six. So a null from this design would be uninformative and must never be published as "ripwire has no effect"; EVALS §8 now says exactly that. The next step is a small SCORED pilot whose only job is to measure the ICC, or a wider repo universe. Also corrected in analyze.py: the clustered bootstrap's default alpha=0.025 was documented as a "95% one-sided lower bound". The 2.5th percentile is the edge of a TWO-sided 95% interval, i.e. 97.5% one-sided. Behaviour kept — conservative is the right direction for an acceptance gate — but a bound that claims 95% while delivering 97.5% understates the detectable effect by 1.5-4.6pp. THE ARMS ARE NOW THREE, because two could not attribute their own result. The 2026-08-04 pilot measured +80% tokens for ripwire_cli and the loss was diagnosed as skill-policy rather than retrieval — which was only knowable after the fact, because that arm ALSO symlinked the whole skills tree on the codex harness. The arm did not mean what its name said. ripwire_cli is now the shipped `ripwire wrap` recipe and nothing else; ripwire_skills is its own arm, and the difference between them IS the skills tax. Both share a byte-identical prompt. OPENCODE ISOLATION IS THE LOAD-BEARING PART. opencode reads $HOME/.claude/ CLAUDE.md into every run unless OPENCODE_DISABLE_CLAUDE_CODE is set, it has no --ignore-user-config equivalent, and OPENCODE_CONFIG merges on top of the global config rather than replacing it. This repository's developers keep a ripwire usage protocol in exactly that file, so an unguarded baseline arm is briefed on the tool it is a control for — silently, with every run still reporting ok. Isolation is done by relocating HOME and every XDG_* dir per run, and test/agentloopopencodecheck.sh asserts it, confirming live against `opencode debug paths` where opencode is installed. opencode also does not fail closed without credentials: it silently routes to its own free hosted model and completes the turn. Records now carry resolved_model read back from the transcript, and a run whose model contradicts --model is an error rather than an arm datapoint. RIPWIRE CALLS ARE COUNTED BY A PATH SHIM the arm's prompt points at, not by grepping a transcript. Codex and opencode self-log shell commands; `claude -p` does not, which is why every claude run recorded ripwire_calls=None since this file was written. The shim execs the real binary and is cross-checked against transcript counts where those exist. --resume skips completed cells (carrying forward only status=ok, so a transient timeout is re-run rather than baked in) and --concurrency runs N cells at once in separate checkout lanes; checkout_repo hard-resets one clone per repo, so shared lanes would have clobbered each other mid-agent. Two fixes to the scoring path that has still never executed: the report was written to the CALLER's cwd (144 strays per batch), and infrastructure failures — Docker down, disk full — were recorded as resolved=False, indistinguishable from a patch that genuinely did not fix the bug and reading as "ripwire did not help". They now stop the run instead of scoring it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A binary on PATH is invisible until the rules file says when to reach for it — but the blurb was an affirmative verb catalog, and an affirmative catalog competes badly with an existing habit. An agent that already knows how to open a file does not weigh a list of alternatives against it. Three prohibitions now close the block, carrying the LongCodeBench 29% -> 3% number as their justification, and they sit LAST because actionable content at the end of a long context measures up to +30% stronger — the same finding behind --order=important-last. The body lands exactly at the CEILING of wrapverbscheck.sh's 10-20 line band. That is the size contract working as intended rather than something to loosen: the next line added here has to displace one, on a block whose whole value is that a human pastes it whole. Two lines of the affirmative catalog already said what a prohibition says better, so they paid for themselves. `wrap claude` also now prints `skills/install.sh --hook` as a RECOMMENDED second line, off the same three-way-probed installer path, claude-only (it registers Claude Code PreToolUse/SessionStart entries; Codex has no equivalent). It is the only lever in the recipe that intercepts a default at the moment it is chosen — a skill fires only if the agent recognizes a moment AND spends a call to load it, whereas reaching for Read costs nothing. Kept a SEPARATE command: opt-in is the hook's design contract, and hookcheck.sh asserts a bare install never touches settings.json. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The hook's fast bail excluded Read outright ("every other tool (Edit/Write/Read/...)
exits here"), which left the LARGEST token sink in an agent loop as the one default it
could not see. That exclusion had the causality backwards: a skill description fires
only if the agent first RECOGNIZES a moment and then spends a call to load the skill,
while reaching for Read needs neither. The read nudge is therefore the one that matters
most, not the one to leave out.
Read and Glob now have their own categories and their own messages — `--expand=SYM` for
a whole-file read, `--for` for a candidate glob. They dedup SEPARATELY from grep: they
are different habits and each is worth one nudge. The read message deliberately does not
guess at a symbol name; suggesting `--expand=<wrong guess>` teaches an agent the tool is
unreliable, which costs more than the nudge gains.
New `--session-start` mode is the proactive half — the PreToolUse nudges only speak
after a default has already been chosen, and not every user has pasted the blurb into
CLAUDE.md. It EXTRACTS the text from `ripwire wrap claude` rather than restating it, so
it cannot drift from wrapUseWhenBlurbLines(); if wrap refuses (a CRITICAL skill-scan
finding), the fence is absent and it degrades to silence like every other path here.
Still advisory-only, still opt-in, still silent off its preconditions. The installer's
matcher becomes Read|Glob|Grep|Bash plus a SessionStart entry, and hookcheck.sh asserts
each tool by NAME so a future matcher edit that quietly drops Read fails loudly — plus
the injected text must contain the prohibition block, which is the anti-drift tripwire
against src/wrap.h.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ire was called ripwire_calls answers "did it reach for the tool". It cannot answer "how often did it default INSTEAD", which is the question every skill, hook and primer change is actually trying to move — and without the denominator, ripwire_calls=3 cannot tell a run that used the tool for everything from one that used it once and then read twenty files. native_read_calls supplies it, counting BOTH channels: shell reads/searches (cat/head/sed/rg/grep/find/ls, anchored at a command boundary so a path containing "less" cannot count) and opencode's native read/grep/glob tools, which spawn no subprocess and are therefore invisible to the PATH shim. A ripwire call piped through grep stays a ripwire call — otherwise the metric would penalize the usage the docs recommend. Two limits the reported number carries rather than hides. Codex has no native read tool at all (every read is a shell command) while opencode has both, so the rate is comparable WITHIN a harness and never across. And `claude -p` self-logs neither, so its runs report n/a — never 0.0, because a missing measurement rendered as "defaulted every time" would invert the conclusion. analyze.py reports it per ARM rather than as a paired delta: the baseline arm has no ripwire on PATH, so its rate is 0 by construction and a delta against it measures nothing. The self-test fixture pins 0/4 baseline against 3/4 treatment exactly, and asserts that blinding both counts yields n/a with n=0 rather than a fabricated rate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request expands agent-loop evaluation to Claude, Codex, and opencode. It separates CLI and skills treatments, adds isolated concurrent execution and measurement, introduces opencode wrapping and schema validation, extends hooks, and adds substitution and power analysis. ChangesAgent-loop evaluation
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Evaluator
participant AgentHarness
participant RipwireShim
participant ResultStore
Evaluator->>AgentHarness: start isolated task lane
AgentHarness->>RipwireShim: execute ripwire invocation
RipwireShim-->>AgentHarness: record invocation
AgentHarness-->>Evaluator: return transcript and metrics
Evaluator->>ResultStore: checkpoint completed result
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (2)
bench/agentloop/power_sim.py-111-112 (1)
111-112: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDeclare a Python 3.12-or-later requirement
random.binomialvariateis available only in Python 3.12 and later. The repository does not declare a Python runtime target. Add an explicit Python 3.12-or-later requirement, or replace this API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bench/agentloop/power_sim.py` around lines 111 - 112, Declare the project’s Python runtime requirement as 3.12 or later in the repository’s standard metadata or environment configuration, matching the existing project conventions; alternatively, replace random.binomialvariate usage in the simulation with an implementation compatible with the currently supported Python versions.bench/agentloop/run_agentloop.py-316-318 (1)
316-318: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
SHELL_READ_REmisses reads on later lines of a multi-line command.The pattern anchors on
^, but the regex is compiled withoutre.MULTILINE, so^matches only the start of the whole string. Both harnesses' bash tools accept multi-line scripts. Incd src\ncat foo.py, thecatis preceded by a newline, not by|,;, or&, so it does not count. Thenative_read_callsdenominator then under-reports, which biases the substitution rate in the treatment arm's favour.🐛 Proposed fix
SHELL_READ_RE = re.compile( - r"(?:^|[|;&]|&&|\|\|)\s*(?:sudo\s+)?" - r"(cat|head|tail|less|more|nl|sed|awk|rg|ag|ack|grep|egrep|fgrep|find|ls|tree)\b" ) + r"(?:^|[|;&\n]|&&|\|\|)\s*(?:sudo\s+)?" + r"(cat|head|tail|less|more|nl|sed|awk|rg|ag|ack|grep|egrep|fgrep|find|ls|tree)\b", + re.MULTILINE )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bench/agentloop/run_agentloop.py` around lines 316 - 318, Update SHELL_READ_RE so command starts after newline characters are recognized in multi-line shell scripts, either by compiling it with re.MULTILINE or by explicitly including newline in the command-boundary pattern. Preserve detection for commands at the beginning of the script and after existing separators.
🧹 Nitpick comments (3)
test/agentloopopencodecheck.sh (1)
51-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for an empty
--model.
build_opencode_commandis only called with a pinned model, so theif model:branch atbench/agentloop/run_agentloop.pyline 373 is never tested with an empty string. That is the state the docstring says must never reach opencode, because the harness then silently uses its own free hosted model. Add a negative case once the invariant is enforced.💚 Proposed addition
if cmd[ -1 ] == "PROMPT": ok( "prompt is the trailing positional" ) else: no( "prompt is not the last argument" ) + +# An empty model must be REFUSED for this harness: opencode does not fail closed without a +# credential, it routes to its own free hosted model and reports success. +try: + R.build_opencode_command( "PROMPT", "" ) + no( "build_opencode_command accepted an empty model — the run would silently use opencode's " + "own free hosted model with no error anywhere" ) +except SystemExit: + ok( "build_opencode_command refuses an empty model" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/agentloopopencodecheck.sh` around lines 51 - 69, Add a negative test in the command coverage around build_opencode_command to verify that passing an empty model does not produce an opencode command, or otherwise fails according to the enforced invariant. This should exercise the if model branch in build_opencode_command and confirm an empty --model value never reaches opencode.test/agentloopcodexcheck.sh (1)
90-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a bare
ripwire_bin.This test always passes an absolute path (
ripwireisstr(root/"build"/"ripwire")), so it never exercisesinstall_ripwire_shim's no-separator branch. That branch is the one that emitsexec ripwire "$@"and makes the shim re-enter itself — see the critical issue onbench/agentloop/run_agentloop.pylines 254-265. Add an assertion that the generated shim never execs a bare name.💚 Proposed addition
assert pathlib.Path( ctx_shim ).name == "ripwire" and os.access( ctx_shim, os.X_OK ) + # The shim is named `ripwire` and its dir goes first on PATH, so it must exec an ABSOLUTE + # path. A bare `exec ripwire` would resolve back to the shim and recurse without bound. + body = pathlib.Path( ctx_shim ).read_text() + exec_line = [ ln for ln in body.splitlines() if ln.startswith( "exec " ) ][ 0 ] + assert exec_line.split()[ 1 ].lstrip( "'\"" ).startswith( "/" ), exec_line🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/agentloopcodexcheck.sh` around lines 90 - 93, Extend the coverage around install_ripwire_shim to use a bare ripwire_bin value without a path separator, then inspect the generated shim and assert it does not emit an exec command targeting the bare name “ripwire”. Preserve the existing absolute-path assertions and ensure the test exercises the no-separator branch that must resolve the executable without recursively invoking the shim.bench/agentloop/analyze.py (1)
27-27: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUpdate the stale schema reference in the module header. The v2-to-v3 rejection is intentional and documented by the README and resume guard.
analyze.pystill says it consumes v2, which contradictsSCHEMA; change that reference to v3.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bench/agentloop/analyze.py` at line 27, Update the stale schema version reference in the analyze.py module header to v3 so it matches the SCHEMA constant and documented resume behavior; do not alter the intentional v2-to-v3 rejection logic.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/agentloop/analyze.py`:
- Around line 62-70: Align the bootstrap confidence labeling with the existing
alpha=0.025 behavior: update resolved_delta_bootstrap_95_lower, the print_report
output, and the README acceptance criterion to identify the bound as 97.5%
one-sided. Preserve resolved_delta_bootstrap_95_lower as a compatibility alias
if renaming the field, and keep the conservative alpha=0.025 calculation
unchanged.
- Around line 148-162: Update bench/agentloop/analyze.py lines 148-162 to pair
and summarize the ripwire_skills arm alongside baseline and ripwire_cli,
including its substitution-rate and count metrics in the analyzer output. Update
bench/agentloop/README.md lines 43-50 to retain the skills-tax claim only when
this comparison is reported; otherwise describe ripwire_skills as
collection-only.
In `@bench/agentloop/power_sim.py`:
- Around line 35-41: The simulation currently generates context and baseline
successes independently, forcing zero paired-outcome covariance. Update the
trial generation in power_sim.py to model each instance/seed pair jointly with a
configurable paired-outcome correlation, while preserving the repo-level mean
statistic consumed by analyze.py; use the existing simulation symbols and
sensitivity configuration rather than independent Binomial draws.
- Line 121: Reduce the computational cost of the bootstrap workflow around the
boots calculation by selecting a validated lower-cost estimator or lowering
N_BOOT and TRIALS to values that preserve the reported MDE precision within the
required error budget. Apply the reduction consistently across the main,
sensitivity, and sample-size calculations as appropriate, and keep the resulting
estimates valid for routine regeneration.
- Around line 108-113: Update the simulation around p1 and the reported lift so
clipping to [0, 1] is accounted for: either calibrate the latent delta to
achieve the requested population mean of p1 - p0, or consistently report the
realized mean lift from the generated probabilities. Ensure the MDE table uses
this realized/calibrated lift rather than the unclipped input delta.
- Around line 290-291: Update the output-writing block in the power simulation
entry point to resolve power_sim_results.json relative to the script’s __file__
location rather than the caller’s working directory, while preserving the
existing JSON serialization and indentation.
In `@bench/agentloop/README.md`:
- Around line 108-110: Qualify the cluster-power comparison in both documents
with its ICC and instance-allocation assumptions. In bench/agentloop/README.md
lines 108-110, revise the “at any instance count” claim to describe the specific
displayed scenario rather than a general result; apply the same qualified
wording in docs/EVALS.md lines 943-950. These are the only affected sites.
- Around line 40-41: Update the cost estimate in the README to account for all
three protocol arms—baseline, ripwire_cli, and ripwire_skills—instead of
multiplying by two. State whether the estimate is per harness; if it covers
execution of all three harnesses, include that multiplier as well, keeping the
estimate aligned with the declared matrix.
In `@bench/agentloop/run_agentloop.py`:
- Around line 766-772: The harness metrics flow must use the same shim binary
named by build_prompt: update both _harness_metrics call sites around lines 800
and 812 in bench/agentloop/run_agentloop.py to pass shim_bin, and prepare a shim
for claude-code-p so shim_log is populated. In _harness_metrics at lines
669-696, record shim-versus-transcript divergence in the returned error field as
documented; apply the same divergence handling to _codex_metrics around lines
660-664.
- Around line 818-823: Update the model-substitution guard in the metrics
validation flow to compare the returned harness model ID with the requested
model’s ID component explicitly, rather than using substring matching. Normalize
or extract the requested identifier consistently with the harness’s modelID
format, then fail via _fail when the identifiers do not match while preserving
the existing success and error-reporting behavior.
- Around line 359-376: Enforce the required model invariant in
build_opencode_command: reject an empty or otherwise unresolved model before
constructing the opencode invocation, rather than conditionally omitting
--model. Ensure the existing caller path, including run_one’s model handling,
cannot proceed with status="ok" when the model is empty, while preserving normal
command construction for valid models.
- Around line 669-696: Update _opencode_metrics to record an error when
shim_calls and ripwire_calls disagree, while retaining the shim count and
commands as the authoritative values. Add the resulting error field to the
returned metrics dict so it flows through run_one’s make_record path, and verify
the _fail call sites do not pass a duplicate error argument.
- Around line 983-995: Change the concurrent execution flow around _execute and
ThreadPoolExecutor so each worker slot owns a fixed lane for its entire
lifetime, rather than assigning lanes from pending-cell submission order. Ensure
a worker cannot start another cell on its lane until its previous cell has
completed, preserving isolated checkout roots for overlapping runs while keeping
_emit and shared-state synchronization unchanged.
- Around line 254-265: Update the shim creation logic around the `real` binary
resolution to resolve bare `ripwire_bin` values through `PATH` before the shim
directory is prepended. Use `shutil.which` for pathless names, fail loudly if
resolution returns no executable, and preserve direct path resolution for values
containing a path separator so the generated shim’s `exec` target is never
itself.
- Around line 64-74: Update the analyzer’s schema handling and arm pairing in
analyze.py to support the third ripwire_skills arm. Retain only consumed fields
such as native_read_calls, remove unused resolved_model and harness_version
handling, and extend the comparison logic beyond the baseline/ripwire_cli pair
so it reports ripwire_skills and the ripwire_skills-versus-ripwire_cli skills
tax.
In `@hooks/ripwire-nudge.sh`:
- Around line 67-69: Update the session marker logic in hooks/ripwire-nudge.sh
to sanitize or hash session before constructing the marker path, and store it
under a private state directory rather than directly in TMPDIR. Replace the
separate -e check and file truncation with atomic mkdir-based marker creation,
exiting when mkdir fails.
In `@skills/install.sh`:
- Around line 48-55: Update the installation logic around the existing
PreToolUse duplicate check to inspect both exact hook registrations before
returning, including the SessionStart command. Migrate legacy Grep|Bash
registrations by expanding or replacing their matcher, and append only missing
PreToolUse or SessionStart entries. Add a migration test beginning with the
legacy PreToolUse configuration and verify no duplicate entries are created.
In `@test/tools/refresh-opencode-schema.sh`:
- Line 40: Update the sha_of function so it checks whether shasum produced a
non-empty digest before accepting the result, then falls back to sha256sum when
unavailable or empty. Preserve the existing SHA-256 output format used by the
callers.
- Around line 77-82: Update the schema refresh flow around the EXIT trap and the
“to adopt” output so the fetched document is copied to a durable path before the
script exits. Print instructions referencing that persistent file instead of
$TMP/live.json, while preserving the existing hash and want= update guidance.
---
Minor comments:
In `@bench/agentloop/power_sim.py`:
- Around line 111-112: Declare the project’s Python runtime requirement as 3.12
or later in the repository’s standard metadata or environment configuration,
matching the existing project conventions; alternatively, replace
random.binomialvariate usage in the simulation with an implementation compatible
with the currently supported Python versions.
In `@bench/agentloop/run_agentloop.py`:
- Around line 316-318: Update SHELL_READ_RE so command starts after newline
characters are recognized in multi-line shell scripts, either by compiling it
with re.MULTILINE or by explicitly including newline in the command-boundary
pattern. Preserve detection for commands at the beginning of the script and
after existing separators.
---
Nitpick comments:
In `@bench/agentloop/analyze.py`:
- Line 27: Update the stale schema version reference in the analyze.py module
header to v3 so it matches the SCHEMA constant and documented resume behavior;
do not alter the intentional v2-to-v3 rejection logic.
In `@test/agentloopcodexcheck.sh`:
- Around line 90-93: Extend the coverage around install_ripwire_shim to use a
bare ripwire_bin value without a path separator, then inspect the generated shim
and assert it does not emit an exec command targeting the bare name “ripwire”.
Preserve the existing absolute-path assertions and ensure the test exercises the
no-separator branch that must resolve the executable without recursively
invoking the shim.
In `@test/agentloopopencodecheck.sh`:
- Around line 51-69: Add a negative test in the command coverage around
build_opencode_command to verify that passing an empty model does not produce an
opencode command, or otherwise fails according to the enforced invariant. This
should exercise the if model branch in build_opencode_command and confirm an
empty --model value never reaches opencode.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83e0ecca-b59e-4722-b22a-39b94ce0cbd5
📒 Files selected for processing (17)
bench/agentloop/README.mdbench/agentloop/analyze.pybench/agentloop/power_sim.pybench/agentloop/power_sim_results.jsonbench/agentloop/run_agentloop.pydocs/EVALS.mdhooks/ripwire-nudge.shskills/install.shsrc/wrap.htest/agentloopcodexcheck.shtest/agentloopopencodecheck.shtest/fixtures/opencode-config.schema.jsontest/hookcheck.shtest/opencodewrapcheck.shtest/regression.shtest/tools/refresh-opencode-schema.shtest/wrapverbscheck.sh
Comment on lines
+62
to
+70
| alpha-quantile. | ||
|
|
||
| THE DEFAULT IS 97.5%, NOT 95% — corrected 2026-08-10. The 2.5th percentile is the lower edge of a | ||
| TWO-sided 95% interval, which as a one-sided bound is 97.5%. A genuine 95% one-sided bound is the | ||
| 5th percentile (alpha=0.05). The behaviour is unchanged and deliberately kept: erring conservative | ||
| is the right direction for an acceptance gate. Only the label was wrong, and a bound that claims | ||
| to be 95% while actually being 97.5% understates the effect this eval can detect by 1.5-4.6pp | ||
| (measured — see power_sim.py). README.md's acceptance criterion is worded in terms of this bound, | ||
| so read it as 97.5% one-sided. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use one confidence label for the bootstrap bound.
The code uses alpha=0.025, which represents a 97.5% one-sided lower bound. However, resolved_delta_bootstrap_95_lower at Line 156, print_report at Lines 174-175, and bench/agentloop/README.md at Lines 138-149 still say 95%. Update the field, report, and documentation together, or change alpha to 0.05. Preserve a compatibility alias if consumers use the current field name.
As per path instructions, this is a material result-interpretation issue, not a prose nit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/agentloop/analyze.py` around lines 62 - 70, Align the bootstrap
confidence labeling with the existing alpha=0.025 behavior: update
resolved_delta_bootstrap_95_lower, the print_report output, and the README
acceptance criterion to identify the bound as 97.5% one-sided. Preserve
resolved_delta_bootstrap_95_lower as a compatibility alias if renaming the
field, and keep the conservative alpha=0.025 calculation unchanged.
Source: Path instructions
Comment on lines
+148
to
+162
| # Substitution rate is reported PER ARM, not as a paired delta: the baseline arm has no ripwire on | ||
| # PATH at all, so its rate is 0 by construction and a delta against it measures nothing. The number | ||
| # that matters is how close the ripwire arm gets to 1.0 — i.e. how often, when it needed to read or | ||
| # search, it actually reached for the tool instead of defaulting. | ||
| sub_base, n_sub_base = mean_substitution( [ b for *_, b, _ in paired ] ) | ||
| sub_ctx, n_sub_ctx = mean_substitution( [ c for *_, _, c in paired ] ) | ||
| out.update( | ||
| resolved_delta_mean=mean( rdeltas ) if scored else None, | ||
| resolved_delta_bootstrap_95_lower=lower, | ||
| localization_hit_delta_mean=mean( ldeltas ), | ||
| tokens_out_ratio_p50=tok_p50, tokens_out_ratio_p95=tok_p95, | ||
| wall_seconds_ratio_p50=wall_p50, wall_seconds_ratio_p95=wall_p95, | ||
| cost_usd_ratio_p50=cost_p50, cost_usd_ratio_p95=cost_p95, | ||
| substitution_rate_baseline=sub_base, n_substitution_baseline=n_sub_base, | ||
| substitution_rate_ripwire=sub_ctx, n_substitution_ripwire=n_sub_ctx, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep the three-arm contract consistent with analysis.
The producer and README define ripwire_cli and ripwire_skills, but the analyzer summarizes only baseline and ripwire_cli. This silently drops the skills arm.
bench/agentloop/analyze.py#L148-L162: add skills-arm pairing and output, including substitution metrics.bench/agentloop/README.md#L43-L50: keep the skills-tax claim only if the analyzer reports that comparison; otherwise document the arm as collection-only.
As per path instructions, this is a cross-layer correctness issue, not a documentation nit.
📍 Affects 2 files
bench/agentloop/analyze.py#L148-L162(this comment)bench/agentloop/README.md#L43-L50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/agentloop/analyze.py` around lines 148 - 162, Update
bench/agentloop/analyze.py lines 148-162 to pair and summarize the
ripwire_skills arm alongside baseline and ripwire_cli, including its
substitution-rate and count metrics in the analyzer output. Update
bench/agentloop/README.md lines 43-50 to retain the skills-tax claim only when
this comparison is reported; otherwise describe ripwire_skills as
collection-only.
Source: Path instructions
Comment on lines
+35
to
+41
| A second identity we use: for the repo-level MEAN delta m_g = mean(ctx_ijk - base_ijk) over the n | ||
| instance*seed pairs in repo g, m_g = mean(ctx) - mean(base) regardless of any correlation between a | ||
| specific pair's base and ctx outcome (linearity of the sum). So m_g's distribution needs only two | ||
| independent Binomial draws per repo per trial — Binomial(n, p1_g) for ctx successes and | ||
| Binomial(n, p0_g) for base successes — not n individual paired Bernoulli draws. This does NOT | ||
| reproduce row-level pairing correlation (irrelevant to analyze.py's statistic, which is a repo-level | ||
| MEAN) but is exact for the quantity the bootstrap actually consumes. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Model paired outcome covariance.
Line 38 draws treatment and baseline successes independently. The paired statistic in bench/agentloop/analyze.py is ctx - base. Its variance depends on Cov(ctx, base). Independent draws force this covariance to zero.
The MDE values therefore do not predict the analyzer for correlated paired outcomes. Generate each pair from a joint binary model, or include paired-outcome correlation as a sensitivity parameter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/agentloop/power_sim.py` around lines 35 - 41, The simulation currently
generates context and baseline successes independently, forcing zero
paired-outcome covariance. Update the trial generation in power_sim.py to model
each instance/seed pair jointly with a configurable paired-outcome correlation,
while preserving the repo-level mean statistic consumed by analyze.py; use the
existing simulation symbols and sensitivity configuration rather than
independent Binomial draws.
Comment on lines
+108
to
+113
| p0 = draw_p0( rng, baseline_rate, icc ) | ||
| eps = rng.gauss( 0.0, sd_eff ) if icc > 1e-9 else 0.0 | ||
| p1 = min( 1.0, max( 0.0, p0 + delta + eps ) ) | ||
| base_hits = rng.binomialvariate( n_per_repo, p0 ) | ||
| ctx_hits = rng.binomialvariate( n_per_repo, p1 ) | ||
| m.append( ( ctx_hits - base_hits ) / n_per_repo ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Calibrate the reported lift after clipping.
Line 110 clips p1 to [0, 1]. The modeled mean of p1 - p0 is then not the input delta, especially at high ICC or near a probability boundary. The MDE table labels this input as a resolution-rate lift.
Calibrate the latent shift to the realized population mean lift, or report the realized lift for every result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/agentloop/power_sim.py` around lines 108 - 113, Update the simulation
around p1 and the reported lift so clipping to [0, 1] is accounted for: either
calibrate the latent delta to achieve the requested population mean of p1 - p0,
or consistently report the realized mean lift from the generated probabilities.
Ensure the MDE table uses this realized/calibrated lift rather than the
unclipped input delta.
| proven in the module docstring: resample G repo MEANS with replacement, G draws, average; repeat; | ||
| sort; take boots[max(0,int(alpha*n_boot))].""" | ||
| G = len( m ) | ||
| boots = [ sum( rng.choices( m, k=G ) ) / G for _ in range( n_boot ) ] |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Reduce bootstrap work before routine use.
The main table executes 60 × 9 × 400 × 2000 = 432,000,000 bootstrap resamples. Each resample draws G repository means. This excludes the sensitivity and sample-size calculations.
Use a validated lower-cost estimator, or reduce N_BOOT and TRIALS to an error budget that supports the reported MDE precision. The current workload makes result regeneration impractical.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/agentloop/power_sim.py` at line 121, Reduce the computational cost of
the bootstrap workflow around the boots calculation by selecting a validated
lower-cost estimator or lowering N_BOOT and TRIALS to values that preserve the
reported MDE precision within the required error budget. Apply the reduction
consistently across the main, sensitivity, and sample-size calculations as
appropriate, and keep the resulting estimates valid for routine regeneration.
Comment on lines
+983
to
+995
| if a.concurrency > 1: | ||
| import concurrent.futures, itertools, threading | ||
| print( f"# concurrency: {a.concurrency} lanes, each with its own checkout root and agent home", | ||
| file=sys.stderr ) | ||
| lanes = itertools.cycle( f"-w{i}" for i in range( a.concurrency ) ) | ||
| guard, done_n = threading.Lock(), 0 | ||
| with concurrent.futures.ThreadPoolExecutor( max_workers=a.concurrency ) as pool: | ||
| futures = { pool.submit( _execute, cell, lane ): cell for cell, lane in zip( pending, lanes ) } | ||
| for fut in concurrent.futures.as_completed( futures ): | ||
| rec = fut.result() | ||
| with guard: # records + checkpoint file are shared state | ||
| done_n += 1 | ||
| _emit( done_n, futures[ fut ], rec ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Lane assignment does not prevent concurrent runs from sharing a checkout root.
Line 987 assigns lanes by submission index (i % concurrency), and line 990 submits every pending cell at once. ThreadPoolExecutor then dequeues in FIFO order as workers free up, but completion order is arbitrary.
Concrete failure with --concurrency 4: cells 0-3 occupy the four workers on lanes -w0..-w3. Cell 1 finishes first. The freed worker immediately dequeues cell 4, which carries lane -w0. Cell 0 is still running on lane -w0. Both now use repos-w0, and checkout_repo() hard-resets that tree for every run. Cell 0's agent has its working tree wiped mid-run.
This is precisely the clobbering the comment at lines 754-757 says lane sharding prevents. Bind the lane to the worker slot, not to the submission index.
🐛 Proposed fix
if a.concurrency > 1:
- import concurrent.futures, itertools, threading
+ import concurrent.futures, queue, threading
print( f"# concurrency: {a.concurrency} lanes, each with its own checkout root and agent home",
file=sys.stderr )
- lanes = itertools.cycle( f"-w{i}" for i in range( a.concurrency ) )
- guard, done_n = threading.Lock(), 0
+ # A lane is held for the DURATION of a run and returned on completion. Assigning by
+ # submission index instead would let a freed worker start cell N on the same lane a
+ # still-running cell 0 is using, and checkout_repo() hard-resets that tree.
+ free_lanes = queue.Queue()
+ for i in range( a.concurrency ):
+ free_lanes.put( f"-w{i}" )
+
+ def _execute_on_free_lane( cell ):
+ lane = free_lanes.get()
+ try:
+ return _execute( cell, lane )
+ finally:
+ free_lanes.put( lane )
+
+ guard, done_n = threading.Lock(), 0
with concurrent.futures.ThreadPoolExecutor( max_workers=a.concurrency ) as pool:
- futures = { pool.submit( _execute, cell, lane ): cell for cell, lane in zip( pending, lanes ) }
+ futures = { pool.submit( _execute_on_free_lane, cell ): cell for cell in pending }
for fut in concurrent.futures.as_completed( futures ):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if a.concurrency > 1: | |
| import concurrent.futures, itertools, threading | |
| print( f"# concurrency: {a.concurrency} lanes, each with its own checkout root and agent home", | |
| file=sys.stderr ) | |
| lanes = itertools.cycle( f"-w{i}" for i in range( a.concurrency ) ) | |
| guard, done_n = threading.Lock(), 0 | |
| with concurrent.futures.ThreadPoolExecutor( max_workers=a.concurrency ) as pool: | |
| futures = { pool.submit( _execute, cell, lane ): cell for cell, lane in zip( pending, lanes ) } | |
| for fut in concurrent.futures.as_completed( futures ): | |
| rec = fut.result() | |
| with guard: # records + checkpoint file are shared state | |
| done_n += 1 | |
| _emit( done_n, futures[ fut ], rec ) | |
| if a.concurrency > 1: | |
| import concurrent.futures, queue, threading | |
| print( f"# concurrency: {a.concurrency} lanes, each with its own checkout root and agent home", | |
| file=sys.stderr ) | |
| # A lane is held for the DURATION of a run and returned on completion. Assigning by | |
| # submission index instead would let a freed worker start cell N on the same lane a | |
| # still-running cell 0 is using, and checkout_repo() hard-resets that tree. | |
| free_lanes = queue.Queue() | |
| for i in range( a.concurrency ): | |
| free_lanes.put( f"-w{i}" ) | |
| def _execute_on_free_lane( cell ): | |
| lane = free_lanes.get() | |
| try: | |
| return _execute( cell, lane ) | |
| finally: | |
| free_lanes.put( lane ) | |
| guard, done_n = threading.Lock(), 0 | |
| with concurrent.futures.ThreadPoolExecutor( max_workers=a.concurrency ) as pool: | |
| futures = { pool.submit( _execute_on_free_lane, cell ): cell for cell in pending } | |
| for fut in concurrent.futures.as_completed( futures ): | |
| rec = fut.result() | |
| with guard: # records + checkpoint file are shared state | |
| done_n += 1 | |
| _emit( done_n, futures[ fut ], rec ) |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 990-990: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/agentloop/run_agentloop.py` around lines 983 - 995, Change the
concurrent execution flow around _execute and ThreadPoolExecutor so each worker
slot owns a fixed lane for its entire lifetime, rather than assigning lanes from
pending-cell submission order. Ensure a worker cannot start another cell on its
lane until its previous cell has completed, preserving isolated checkout roots
for overlapping runs while keeping _emit and shared-state synchronization
unchanged.
Comment on lines
+67
to
+69
| marker="${TMPDIR:-/tmp}/ripwire-nudge.${session}.session-start" | ||
| [ -e "$marker" ] && exit 0 | ||
| : > "$marker" 2>/dev/null || true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use private, atomic session markers.
session is inserted directly into a predictable path under ${TMPDIR:-/tmp}. A session ID with path separators can escape the intended marker name. The -e check and : > "$marker" are also a TOCTOU sequence that can follow a symlink inserted after the check.
Store sanitized or hashed session keys in a private state directory. Create the marker atomically with mkdir, and exit when that creation fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hooks/ripwire-nudge.sh` around lines 67 - 69, Update the session marker logic
in hooks/ripwire-nudge.sh to sanitize or hash session before constructing the
marker path, and store it under a private state directory rather than directly
in TMPDIR. Replace the separate -e check and file truncation with atomic
mkdir-based marker creation, exiting when mkdir fails.
Comment on lines
48
to
55
| tmp="$( mktemp )" | ||
| jq --arg cmd "$hookScript" ' | ||
| jq --arg cmd "$hookScript" --arg scmd "$hookScript --session-start" ' | ||
| .hooks //= {} | | ||
| .hooks.PreToolUse //= [] | | ||
| .hooks.PreToolUse += [{ matcher: "Grep|Bash", hooks: [{ type: "command", command: $cmd }] }] | ||
| .hooks.PreToolUse += [{ matcher: "Read|Glob|Grep|Bash", hooks: [{ type: "command", command: $cmd }] }] | | ||
| .hooks.SessionStart //= [] | | ||
| .hooks.SessionStart += [{ matcher: "startup|resume|clear", hooks: [{ type: "command", command: $scmd }] }] | ||
| ' "$settings" >"$tmp" && mv "$tmp" "$settings" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Migrate existing hook registrations.
Lines 31-35 return when any existing PreToolUse command matches $hookScript. A legacy Grep|Bash registration therefore does not gain Read|Glob or the new SessionStart entry. Conversely, a configuration with only the SessionStart command receives a duplicate SessionStart entry.
Check both exact registrations before returning. Add only the missing entry, or replace the legacy matcher. Add a migration test that starts with the old PreToolUse entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/install.sh` around lines 48 - 55, Update the installation logic around
the existing PreToolUse duplicate check to inspect both exact hook registrations
before returning, including the SessionStart command. Migrate legacy Grep|Bash
registrations by expanding or replacing their matcher, and append only missing
PreToolUse or SessionStart entries. Add a migration test beginning with the
legacy PreToolUse configuration and verify no duplicate entries are created.
| exit 1 | ||
| fi | ||
|
|
||
| sha_of(){ shasum -a 256 "$1" 2>/dev/null | awk '{print $1}' || sha256sum "$1" | awk '{print $1}'; } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
sha_of never falls back to sha256sum.
In a pipeline, || tests the exit status of the last command. Here that is awk, which exits 0 even on empty input. On a system without shasum (most Linux distributions), shasum fails, awk reads nothing, exits 0, and the fallback never runs. The function then prints an empty string.
The result: line 43 prints unchanged — pin is current (sha256 ) and line 80 prints new sha256: with no digest. The operator cannot update the want= pin in test/opencodewrapcheck.sh, which is the only purpose of this script.
test/opencodewrapcheck.sh lines 158-161 already handles this correctly by testing for an empty result.
🐛 Proposed fix
-sha_of(){ shasum -a 256 "$1" 2>/dev/null | awk '{print $1}' || sha256sum "$1" | awk '{print $1}'; }
+sha_of(){
+ local h
+ h=$( shasum -a 256 "$1" 2>/dev/null | awk '{print $1}' )
+ [ -n "$h" ] || h=$( sha256sum "$1" 2>/dev/null | awk '{print $1}' )
+ printf '%s' "$h"
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sha_of(){ shasum -a 256 "$1" 2>/dev/null | awk '{print $1}' || sha256sum "$1" | awk '{print $1}'; } | |
| sha_of(){ | |
| local h | |
| h=$( shasum -a 256 "$1" 2>/dev/null | awk '{print $1}' ) | |
| [ -n "$h" ] || h=$( sha256sum "$1" 2>/dev/null | awk '{print $1}' ) | |
| printf '%s' "$h" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/tools/refresh-opencode-schema.sh` at line 40, Update the sha_of function
so it checks whether shasum produced a non-empty digest before accepting the
result, then falls back to sha256sum when unavailable or empty. Preserve the
existing SHA-256 output format used by the callers.
Comment on lines
+77
to
+82
| echo | ||
| echo "=== to adopt ===" | ||
| echo " cp '$TMP/live.json' '$PINNED' # (copy now, before this script's temp dir is cleaned)" | ||
| echo " new sha256: $( sha_of "$TMP/live.json" )" | ||
| echo " then update want= in test/opencodewrapcheck.sh and re-run it." | ||
| exit 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The printed cp command cannot work — the temp dir is deleted on exit.
The EXIT trap at line 23 removes $TMP. The script exits at line 82 immediately after printing the instruction, so $TMP/live.json is gone before the operator can run the suggested cp. The parenthetical "copy now, before this script's temp dir is cleaned" does not help; there is no point at which the operator can act.
Write the fetched document to a durable path instead.
🐛 Proposed fix
+STAGED="$PINNED.new"
+cp "$TMP/live.json" "$STAGED"
echo
echo "=== to adopt ==="
-echo " cp '$TMP/live.json' '$PINNED' # (copy now, before this script's temp dir is cleaned)"
-echo " new sha256: $( sha_of "$TMP/live.json" )"
+echo " staged the fetched schema at: $STAGED"
+echo " mv '$STAGED' '$PINNED'"
+echo " new sha256: $( sha_of "$STAGED" )"
echo " then update want= in test/opencodewrapcheck.sh and re-run it."
exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| echo | |
| echo "=== to adopt ===" | |
| echo " cp '$TMP/live.json' '$PINNED' # (copy now, before this script's temp dir is cleaned)" | |
| echo " new sha256: $( sha_of "$TMP/live.json" )" | |
| echo " then update want= in test/opencodewrapcheck.sh and re-run it." | |
| exit 1 | |
| STAGED="$PINNED.new" | |
| cp "$TMP/live.json" "$STAGED" | |
| echo | |
| echo "=== to adopt ===" | |
| echo " staged the fetched schema at: $STAGED" | |
| echo " mv '$STAGED' '$PINNED'" | |
| echo " new sha256: $( sha_of "$STAGED" )" | |
| echo " then update want= in test/opencodewrapcheck.sh and re-run it." | |
| exit 1 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/tools/refresh-opencode-schema.sh` around lines 77 - 82, Update the
schema refresh flow around the EXIT trap and the “to adopt” output so the
fetched document is copied to a durable path before the script exits. Print
instructions referencing that persistent file instead of $TMP/live.json, while
preserving the existing hash and want= update guidance.
Collaborator
Author
|
Superseded by PR #15 (feat/opencode-round-public, merged 2026-08-10 19:58 ET as 2a7e255). Verified 2026-08-11 by test merge: after resolving the two counted-file conflicts (regression.sh gate loop — this branch's list is a strict subset of main's — and the EVALS 375-vs-380 count), the merged tree is byte-identical to origin/main. Every commit here, including ff928ee, landed on main as rebuilt commits 42a2f53 / 74ce8b5 / b089dc1; bench/agentloop (run_agentloop.py, power_sim.py, power_sim_results.json, tasks.lock) is fully present on main. Closing rather than landing; the outcome-eval track is not blocked. |
joyful-ii-V-I
added a commit
that referenced
this pull request
Sep 9, 2026
…rminism arm — name the writer, fix it, watch for the next CI run 34298150602 (macOS plain, shard 2/2): tokenbudgetcheck's `--for` determinism arm got est_tokens 3949 then 3947. That is at="<sha>" vs at="<sha>+dirty" — six bytes, two tokens at 2.5 B/tok — and the dirty bit is `git status --porcelain`, which src/gitstamp.h runs from ANY crawl root inside the checkout. Issue #71 blamed test/prbudgetcheck.sh's appends to "$ROOT/src/mod4.cpp"; that gate rebinds ROOT to its own mktemp fixture at :48, has never written outside it, and src/mod4.cpp does not exist. The writer was test/gateexitcheck.sh: its arm (A) copied fixtures to test/gateexitfix/.gateprobe.*.sh, untracked, for ~20 ms each — observed live under a 50 Hz git-status watcher — three worker slots from tokenbudgetcheck in that shard's run order. - test/pargates.py: the shared-tree tripwire, sibling of the shared-binary one. Baseline `git status --porcelain --untracked-files=all` before the run, sample it every 0.25 s (PARGATES_DIRT_POLL_SEC) with --no-optional-locks, and report every NEW line with the gates in flight when it was seen; a hit fails the run. It is a sampler and says so ("a floor, not a total"); a non-git root reports tree_writes=unwatched and is never turned red. - test/pargatescheck.sh: three functional arms on the real script — WRITER (seen red: rc!=0, path and gate named, although the gate itself passed), CONTROL (same gate, mktemp: clean), UNWATCHED (non-git corpus: disarmed, rc 0) — plus two static pins. - test/gateexitcheck.sh: (A)'s probe copies go to a mktemp dir (the fixtures never read $0); (E)'s must sit beside the real gate, so .gitignore now names `.gateprobe.*` and new arm (F) proves the hiding on the checkout's own .gitignore text seeded into a scratch repo, with an un-ignored twin as the mutation control, plus git check-ignore in situ and a per-probe status check in (E). Seen red with the ignore line removed. - test/tokenbudgetcheck.sh: #1, #14 and #15's pair crawl a private copy of src/ under mktemp, outside every repository, so no concurrent gate can dirty it. Assertion set unchanged. - test/prbudgetcheck.sh: the fixture root is spelled $FIX instead of rebinding ROOT, so the grep that produced the misdiagnosis finds nothing. Behaviour identical, 33 PASS. - CONTRIBUTING.md: the rule, where a gate author meets it. Not fixed here, disclosed: a dozen other gates run a stamped verb twice on the real checkout (adaptivecheck, cochangesurprisecheck, docanchorcheck, forcalibfactscheck, forlenscheck, forrankordercheck, packtaskcheck, attrvocabcheck, …). With the writer gone and the tripwire armed they are safe; the tripwire, not a per-gate copy, is the class fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
joyful-ii-V-I
added a commit
that referenced
this pull request
Sep 13, 2026
…nup the seam owns, and two examples contradicted the contract above them CodeRabbit review 5192490014 on 84e5da3, two threads, both valid. A THROW FROM THE EMITTER TOOK THE WHOLE DOCUMENT. rw::renderToString called emit( m ) outside any handler. A throw from it — std::bad_alloc out of the std::format fallback is the reachable one, since the point of this seam is to buffer a document whose size is not known in advance — skipped the fclose, the free, the alert and the documented empty-result fallback in ONE jump: the memstream and its buffer leaked, and the caller got an exception where its contract says it gets ok == false. Measured on this tree with the fault injected, --pr-context aborted at rc=134 with ZERO bytes on stdout and `libc++abi: terminating due to uncaught exception of type std::bad_alloc`. Not the estimate lost — the document. The fix is this tree's own answer to "a throw crosses a seam that owns a resource": catch at the seam, release what it owns once, DISCLOSE, and hand back the degraded value the caller already reads (search.h's `catch( ... ) { out.degraded = true; return out; }`, ingest_astquery.h's per-file degrade). Callers need no new case: ok == false has always meant "these are not the bytes the emitter wrote", and --pr-context already streams the floor level straight out when it sees one. Same run now: exit 0, a complete 14,627-byte well-formed document. fclose runs before free, both once, on every path. THE ALERT NAMES THE CAUSE IT HAD. degradeMsg says the BUFFER failed; on this path it did not. Reusing it would have been a wrong reason attached to a right consequence, so the catch carries its own literal (the macro takes a const char*, and __PRETTY_FUNCTION__ already names the caller's Emit lambda by file and line). ONE FAULT-SWITCH READER, NOT TWO. The first draft copied serialize.h's isChargeBufferFaultInjected shape verbatim and --quality-delta called it: `duplication ... 53 tokens, gating="1"`. It was right. The parsing rule is the part worth sharing — EXACT "1" is the only ON value, because the contract is a switch — and it is a rule this tree already got wrong ONCE and fixed (CA4 w1fix2-verifier G4: `value[0] == '1'` made "=10" and "=1000000" inject the fault). Copying the shape would have meant the next switch could get it wrong again. Both now read through rw::faultSwitchOn( envName ) in infra/emit.h, which serialize.h already includes; each keeps its own named wrapper and its own once-per-process `static`, because the name is what a reader greps for and the static is what determinism needs. gating back to 0. Verified both directions still hold: RIPWIRE_FAULT_RENDER_EMIT_THROW=10 and RIPWIRE_FAULT_CHARGE_BUFFER=0 both inject nothing. Gate: test/prcontextcheck.sh arm (F), six rows, with estchargecheck #14's observability-probe idiom so it can never pass for lack of an alert it could not have seen (the switch is non-NDEBUG only, like DEGRADED_PATH_ALERT itself). RED on the parent commit: rc=134, 0 B of stdout, no alert. (F4) asserts the direction the degrade actually has, and got it backwards first — the degrade streams kPrTrims[0], the UNTRIMMED floor, so it carries at least as many rows as the budget-trimmed control and here carries 40 against 4. "Same count" was the wrong invariant; the arm failed on its own tree and is now written as the claim that matters: content is never lost, only the estimate. TWO README EXAMPLES CONTRADICTED THE CONTRACT DIRECTLY ABOVE THEM — the one this lane's previous commit had just written out. `<t p="./test/verify_radix.cpp"/>` carried neither run= nor run_unknown="1", which is the "never neither" rule broken in the very block that states it (it is pre-M21(b) output, kept verbatim while the rule around it changed). And the <g> example said n="7" beside three paths, where the contract says p= splits on ',' into exactly n= paths. Now run_unknown="1" and n="3". Gates: prcontextcheck (ALL PASS, arm F red-first), estchargecheck, readmedriftcheck, readmeexamplecheck, packtaskcheck, flagscheck, manifestcheck, gatecount_build --check (613), limits_build --check (210 caps). ASan+LSan with the fault injected: 0 reports, 0 leaks — the leak this commit exists to fix. --quality-delta gating="0". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
csy20
pushed a commit
to csy20/ripwire
that referenced
this pull request
Sep 17, 2026
… a refusal e768898 (M8, 2026-09-04) made `--rank-by=churn --since=notadate` refuse before any verb runs, so it raises no DEGRADED_PATH_ALERT on any build. Several gates still read it as "can this binary print alerts?", and each arm below had stopped deciding anything. - estchargecheck redhat-et#14 and localscountcheck arm 4 took TWO readings (that probe and --version's build type) and skipped only when both said "no alerts". With the probe always silent it was one reading: a Release-labelled binary that printed every alert except redhat-et#14's SKIPPED redhat-et#14 instead of failing it. Both now probe with a --scip index that opens and fails to decode, the probe qualitystalecheck already used; the index lives in $TMP, and the run passes --no-cache. - localscountcheck arm 4's row also matched `corrupt` in the gate's own blob name (x_corrupt.ripwirecache), which the Release-visible stderr line prints, so it passed on every binary and never reached the alert branches: a dev binary with every alert stripped PASSED. It is two rows now: the `ripwire: cache <blob>: parser-version — not used` line on every flavour, and the `ingest: cache blob parserVer mismatch` alert decided by the two readings. - qualitystalecheck: the skip text and probe comment named --since=notadate; they name the --scip decode path the probe has used since 2026-09-04. - qoriginoraclecheck (c): "pre-Q1 sidecar emits the degrade alert" checked `--version` runs and stderr says pre-Q1, which is the Release-visible line the row above already pins (CONTRIBUTING §2 shape 7). Since a8c71a0 readBaseline refuses a pre-Q1 sidecar before computeDelta, so computeDelta's fail-closed "(pre-Q1 format)" alert cannot fire; the plain build prints no alert at all. Owner call: the row asserts that alert does NOT fire, on non-NDEBUG build types behind the --scip decode positive control; Release/RelWithDebInfo/MinSizeRel skip. A mutant whose readBaseline honors the pre-Q1 sidecar again PASSED the old row (its `grep pre-Q1` matched the alert) and FAILS the new one. - sincecheck "M8 $host: ONE message" asserted no `[math degraded]` line with no positive control, so a dev binary with every alert stripped passed all four rows. The alert half now asserts only on non-NDEBUG build types after the --scip decode control proves alerts print; Release skips that half once; the "ignoring it" half asserts on every flavour. Every new SKIP row sits on the Release branch only (pargates arm I). Red first, each gate run as its own script: wrappers that strip `^\[math degraded\]` from stderr (all alerts, or one seam's) and one whose --version says (Release, instead of (dev, — plus the pre-Q1 mutant and a binary that adds one alert to the --since refusal. gateexitcheck and manifestcheck pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
joyful-ii-V-I
pushed a commit
that referenced
this pull request
Sep 19, 2026
…empty-string refusal; legend roster row Independent review of 51f4b4b found three issues before measurement: 1. HIGH: the stub substitution only touched legoStr/composeStr on the buffered (open_memstream) path. When that pre-render fails, the fallback streamed packLego/packCompose straight to stdout unconditionally — a silent bypass of the default cut, regardless of --sections=. Fixed by computing the stub's total= without rendering: two new pure functions, legoPreCapRowCount/composePreCapRowCount (serialize.h), mirror packLego/packCompose's own selection predicates exactly (no FILE*, no buffering), so the degrade path can build the identical stub the happy path would have. Both call sites (buffered substitution, degrade fallback) now share one legoWillStub/composeWillStub decision and one restoring-spelling string. Verified with the tree's existing fault-injection seam (RIPWIRE_FAULT_CHARGE_BUFFER=1, serialize.h's openChargeStream) — red on 51f4b4b (full <iface>/<impl> leaks past the stub), green after the fix; new gate arms (9a-e) in forsectioncollapsecheck.sh, gated behind the same NDEBUG-flavour skip estchargecheck.sh's #14 family already established (the switch only exists on the non-NDEBUG build). 2. MED: the MCP `sections` argument's closed-set validation was guarded by `!sections.empty()`, so `sections:""` (present, empty) was silently read as absent instead of refusing — the same ABSENT-vs-PRESENT-BUT-EMPTY bug class `legend` already guards against (F9/F11). Fixed with the same `findRawValue(...).isPresent` pattern; new gate arm (8c). 3. MED: legendcoveragecheck's roster had no row exercising --sections=lego,compose, so the ratchet could not see the full <iface>/<m>/<field> attribute surface at all. Added `for-sections`. It surfaces six attributes with no dedicated definition (ctx@task, d@next, field@owner/rel/type, m@pure) — all six are the same pre-existing, deliberately-undefined debt this file already records for the plain --for rows (byte-tight header, P3/P7/L1 precedent), now visible on the one row that opts back into the full render; baselined with that cross-reference rather than forcing new legend text into an already byte-tight header for debt that predates this lane. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
usehoplite Bot
pushed a commit
to pt-act/ripwire
that referenced
this pull request
Oct 15, 2026
…named call timeouts estchargecheck timed out at 1200 s on the macOS plain shard. Profile (one local run): 337 ripwire calls, ~154 of them a cold --no-cache parse of src/ or the repo root (1.3-6 s each, ~270 s of a 420 s run). - rw: the arms on src/, . and the repo root share one warm cache per (root, artifact class) in the run's temp dir instead of --no-cache. A warm read serves the same stdout and stderr bytes as a cold parse. Fault-injection arms keep --no-cache, and #7 now compares a cold run with a warm one. - rwcall: every call runs under a per-call budget (ESTCHARGE_CALL_TIMEOUT, default 300 s). A killed call prints its own FAIL TIMEOUT/ERROR line and fails the gate, so it is never judged as an answer. - Each section is now a function, and sections that share state run as one chain. Chains run on a pool (ESTCHARGE_JOBS, default min(4, cpus); 1 = serial roster order), heaviest first. Section output is replayed in roster order, so the PASS/FAIL lines and their order are unchanged. FOR_TASK, forest_of, KEPT_NOTE/FLOOR_NOTE and sigs_unpaid_twin moved unchanged into a shared block so the redhat-et#11 sweep, redhat-et#14 and redhat-et#15 need not wait for redhat-et#11. No assertion, expected value or arm changed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.