fix(arenabench): emit the launch stub with a quoted heredoc, so shellcheck and the gate are green again (#2355) - #2363
Merged
Conversation
…check and the gate are green again The stub generator echoed its lines from single quotes — correct for the intent (the $1, $STUB_LOG and \n must reach the generated file literally), but shellcheck's default severity fails the gate on the resulting SC2016/SC2028 info findings, and because shellcheck is a GATE_STEPS entry and a required check, main went red and reddened every open PR. A quoted heredoc says the same thing in the form the linter (and a reader) can see is deliberate: quoted-delimiter content is literal by construction. The emitted stub is byte-identical under bash (verified with cmp against the old generator's output), and the change also removes an interpreter dependence the old form carried: under sh, XSI echo expands the \n and splits the printf format string across two lines — exactly the hazard SC2028 exists to name. Verified: make shellcheck green; bash scripts/test-arena-scripts.sh passes 20/20, including the stub-consuming classification checks. Closes #2355
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's GuideThis PR changes how the launcher stub script is generated in scripts/test-arena-scripts.sh, switching from echoing single-quoted lines to using a quoted heredoc so that shellcheck no longer flags false positives and the generated stub remains byte-identical and interpreter-safe. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
9 tasks
This was referenced Aug 8, 2026
macanderson
added a commit
that referenced
this pull request
Aug 8, 2026
…ate one (#2365) ## What & why **`main` is red, and every open PR is red with it.** One line fixes it. `WorkspaceProbe::diff` is public and its doc comment linked `[`Self::ignores`]`, which is private. `rustdoc::private_intra_doc_links` is denied under `-D warnings`, so `cargo doc` fails the required `fmt + clippy + test` job: ``` error: public documentation for `diff` links to private item `Self::ignores` --> crates/stella-tools/src/shell_touch.rs:349:39 error: could not document `stella-tools` ``` Reproduced on main's own runs at `ad92643b` and `298b2705`, not just on a PR head. ## How it got in #2344 introduced the link. Its branch predated #2354, so the `cargo doc` gate that ran against it was the older command, and the failure it *did* report was read as the pre-existing shellcheck breakage that #2363 was already fixing. ## One thing worth a follow-up thought on #2336 / #2354 `--document-private-items` does **not** silence this lint for a public → private link. rustdoc's own note says "this link will resolve properly if you pass `--document-private-items`" — and both `make doc-warnings` and `ci.yml` already pass it, at the exact commits that failed. So the hint is misleading for this direction. That does not weaken #2354; the gate caught a real defect. It just means the working rule is narrower than the hint suggests: **a public item cites a private helper in prose, never as an intra-doc link.** `pub(crate) → pub(crate)` links, which #2354 was about, are unaffected. ## The fix The link becomes prose. No API change, no behavior change — deliberately the smallest possible diff, because an unbreak PR that also does something else is how a red `main` stays red longer. ## Witness - [ ] This PR includes a witness test None, and none is possible: the failing check *is* the witness. `cargo doc -D warnings` fails on `main` at this commit's parent and passes here — a witness test cannot assert about a rustdoc lint, and the gate already does. ## Gate Not run locally — a Terminal-Bench match is executing on this machine and a workspace build would contend for CPU, which is how a trial acquires a false timeout. `make guards-fast` is green; the compile tiers are CI's. ## Ground-rule check - [x] No I/O added to `stella-core`; no new deps - [x] No new outbound network calls - [x] No new cross-boundary types ## Summary by Sourcery Bug Fixes: - Resolve rustdoc private_intra_doc_links failure by replacing a link to the private ignores helper with plain prose in the diff method documentation.
macanderson
added a commit
that referenced
this pull request
Aug 8, 2026
…tion (b), honest about what the corpus cannot supply (#2304) (#2367) Refs #2304. Implements **PR 5** of `doc:trace-replay-learning-harness` §9 — the Claude Code transcript adapter — and completes spec §13 with the adapter's half (§13.3), which #2350 left open. **Based on `main`**, which now carries the harness — #2350 merged while this was in flight. *(Replaces #2357, which GitHub auto-closed when #2350 merged and its base branch `feat/2304-trace-replay-harness` was auto-deleted. Same content, rebased onto the merged harness.)* ## The adapter's honest limit is the design Claude Code transcripts contain **no Stella reflection JSON**. There is nothing in them to script a lessons array from, so §7.2 puts the choice plainly: derive lessons from the transcript, or decline to. Deriving them means the harness measures the adapter's lesson-invention heuristic instead of Stella's learning — it fabricates the exact signal under test. **This implements option (b), which the spec recommends**: shell history, session and turn boundaries, and timing only. It lights up the tool foundry against thousands of real commands and launders nothing. ### That needed a fourth `ScriptedReflection` arm Expressing "the source carried no reflection" with the three existing arms is impossible without asserting something the source does not support: | Spelling | The claim it makes | |---|---| | `Lessons { lessons: [] }` | the model had nothing to say | | `Unreadable { .. }` | the model said something unparseable | | `ModelError { .. }` | the call failed | The second is the dangerous one: it would **fabricate starvation**, and assertion 7 (`an_unreadable_corpus_builds_nothing_and_says_why`) counts exactly those turns. An adapter that emitted `Unreadable` for 16,000 turns would corrupt the metric its sibling test depends on. So `NotRecorded` is its own arm. The replayer skips the model boundary entirely for it, and the summary counts `turns_not_recorded` separately from empty reflections — a metric that folded them together would report the learner as idle when it was never asked. `Provenance::Derived` already exists on the `Lessons` arm from #2350, and `every_lesson_is_labelled` asserts nothing reaches a trace unlabelled — so if option (a) is ever built, it cannot ship un-stamped quietly. ## The privacy gate (§7.1), applied where the risk actually is The spec asks for "a secret-shaped string is quarantined rather than stored". Under option (b) the adapter derives **no statements**, so there is no proposal to run through `gate_proposal`/`quarantine_for`. The highest-risk field it does touch is the **shell command** — `export ANTHROPIC_API_KEY=sk-ant-…` and `curl -H "Authorization: Bearer …"` are ordinary things to have typed. So the gate runs there, and it is **stricter than quarantine**: `redact_secrets` runs on every command, and a command whose redaction *fired* is **dropped**, not kept with a `[redacted]` hole in it. Two reasons: - The redactor's prefix list is a good filter, not a complete one. A token shape it has not seen would survive in a command it had partly redacted, and the partial redaction is what would make that look safe. - The foundry loses nothing. A command carrying a credential is a one-off by nature, so it was never going to be a recurring shape worth minting a tool from. Beyond that: **local-only and opt-in** (`STELLA_REPLAY_CC_CORPUS`, reachable only from a test — no shipped command, and a plain `cargo test` never touches the corpus); **nothing derived is committed** (the CI corpus stays synthetic, permanently, and `no-scratch` fails the gate on a committed derivative on its own); and the transcript stub is left **empty**, because nothing reads it under option (b) and carrying user text into a trace we did not need would be gratuitous retention. ## Measured against the real corpus ``` corpus: 496 project director(ies) adapted 20 project(s): 16664 turn(s), 17343 command(s) ``` Every adapted trace was round-tripped through `Trace::parse` — the loader's own contract, applied to a real uncontrolled source rather than to a synthetic fixture the adapter was written against. That is the assertion that catches a transcript shape the adapter mishandles. **A correction to §7's measurement, recorded in §13.3.** The spec measured 485 project directories on 2026-08-08; it is 496 today. Consistent with the rolling window the spec already flags, and the reason a derived trace can never be a committed fixture. ## The spec now records what was built (§13) The document said `status: proposed`. Rather than leave a plan describing something that now exists, §13 records the divergences and — more usefully — **the four things replaying the real loop measured**, none of which is visible from reading a single module: the shared dedup/clustering threshold, the foundry's value-like-argument rule, the workspace-derived lineage id, and lexical `starts_with`. A future reader planning fixtures needs all four. ## Verification - `cargo test -p stella-cli` — **1531 passed, 0 failed** (12 new), on top of the merged harness - `STELLA_REPLAY_CC_CORPUS=1 cargo test … the_real_corpus_adapts` — output above - `cargo clippy -p stella-cli --all-targets -- -D warnings` — clean - `make guards-fast` — clean (#2355's shellcheck failure is fixed on `main` by #2363) **No tests deleted.** ### One guard caught a real mistake, on the first run `paths::tests::nothing_else_in_this_crate_reads_a_home_out_of_the_environment` failed on my `std::env::var_os("HOME")`. Fixed to `crate::paths::home()`, which is what lets a test redirect the anchor without mutating process-global state (#1139). Worth naming because it is exactly the guard working as designed. ## Not in this PR - **Option (a)** — synthesizing lessons. The labelling machinery is in place if it is ever wanted; the spec's recommendation against it stands. - **Committing any derived trace.** Permanently out of scope. ## Summary by Sourcery Add a local-only, opt-in Claude Code transcript adapter that feeds real shell history into the trace replay harness without inventing reflections, and extend the harness to account for turns where the source provided no reflection. New Features: - Introduce a Claude Code transcript adapter that converts local transcripts into trace sessions for the replay harness under an explicit environment-based opt-in. - Add support for a `ScriptedReflection::NotRecorded` arm and corresponding replay summary metric to represent turns where the source carried no reflection JSON. Enhancements: - Update the replayer to skip the reflection model boundary entirely for turns marked as not recorded while still driving the tool foundry and timeline. - Refine the trace replay harness documentation to describe the built Claude Code adapter, its privacy guarantees, and its measured corpus characteristics. Documentation: - Document the Claude Code adapter design, its option (b) behavior, privacy gate, and real corpus measurements in the trace replay harness spec. Tests: - Add adapter and privacy-gate tests over synthetic transcripts, including opt-in behavior and timestamp handling, and a guarded test that exercises the real local corpus when enabled.
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
1 participant
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.
What & why
mainis red: theshellcheckgate step fails onscripts/test-arena-scripts.sh(arrived with #2328, extended by #2351) with nine
SC2016/SC2028info-level findings,and since
shellcheckis aGATE_STEPSentry and a required check, every open PR is redregardless of its diff (observed on #2354, which touches no shell script).
The findings are false positives about intent — the generator
echos single-quoted linesso that
$1,$STUB_LOGand\nland literally in the generated stub. This PR takesissue #2355's preferred fix (option 1): emit the stub with a quoted heredoc
(
cat <<'STUB'), whose content is literal by construction, so both codes stop applyingand the intent is self-evident to reader and linter alike. No disables, no gate widening,
step name untouched (
check-gate-parity.shunaffected).Bonus the heredoc buys for free: the old form was interpreter-dependent — under
sh(XSI
echo), the\nin theprintfline expands and splits the format string acrosstwo lines, exactly the hazard
SC2028names. Benign in practice (the script runs underbash, and
printf "%s<newline>"behaves the same), but now it cannot happen at all.Closes #2355
The witness
make shellcheckfails onmain(exit 1,nine findings) and passes here (verified locally). The generated stub is proven
byte-identical under bash — both generator forms were run side by side and
compared with
cmp— andbash scripts/test-arena-scripts.shpasses 20/20,including the stub-consuming crash/handoff classification checks.
The gate
make shellcheckgreen locally;bash scripts/test-arena-scripts.sh20/20Closes #2355appears both above and as a commit trailerNothing left behind
interpreter-dependence observation above is fixed by the same change. Related
pre-existing alerts remain tracked in deps: two open Dependabot alerts — js-yaml (high, website) and h2 (medium, terminal_bench_analysis) #2121.
Anything reviewers should know?
Landed on its own from a fresh
mainper the issue's constraint (a red shared gate mustnot be repaired inside unrelated PRs — the #2004 merge-skew lesson). Once this merges,
open PRs (e.g. #2354) need only a re-run / branch update to go green.
Summary by Sourcery
Emit the arena launch stub using a quoted heredoc to restore shellcheck gate success while preserving the generated script’s behavior.
Bug Fixes:
Enhancements: