The oracle-mask overlay covered the same path review setup reads, so every historical cell died with can't-open-patch. Leave the staged copy visible for apply, then fail closed if it is still there when the model starts.
Co-authored-by: Cursor <cursoragent@cursor.com>
Seed the current gitnexus-review skill into older PR checkouts, force-add
historically gitignored skill paths, accept plugin-qualified Skill ids,
and lock host-unsafe workspaces to review-output.json so a generation can
finish and score. Sandbox cleanup restores owner write bits before delete
because a session that copytrees the locked clone otherwise leaves 0555
trees that rmtree cannot remove.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(build): build the web UI from prepack, not from every npm ci
gitnexus-web is a separate ~650-package tree (React, Vite, LangChain,
Mermaid). Because `prepare` built it, every `npm ci` in gitnexus/ also
installed and Vite-built a second product. On CI that install ran
uncached inside an execSync timeout, so a healthy-but-slow install was
SIGTERM'd mid-flight and surfaced as `spawnSync /bin/sh ETIMEDOUT` --
repeatedly killing node floor compat, a job that only import-links the
CLI dist and never needs the UI.
The UI is only needed inside the published tarball, so build it from
prepack instead. `npm run build` and `prepare` are now CLI-only; pass
--web (or npm run build:web) to include it. Jobs that pack or publish
install gitnexus-web in their own visible step, and the in-script
fallback install is untimed so a slow install can no longer be killed
halfway and reported as a build failure. The tsc/vite timeout default
goes 300s -> 600s so the remaining bounded steps have headroom.
Default build on this machine: 30s, no gitnexus-web work.
* fix(build): enforce web package artifact integrity
Co-authored-by: Cursor <cursoragent@cursor.com>
* refactor(build): clarify web packaging helpers without changing behavior
Keep the same opt-in, fail-closed, and pack/publish preserve rules while
trimming comments, sharing the test harness, and reading index.html
directly instead of probing it first.
Co-authored-by: Cursor <cursoragent@cursor.com>
* ci: skip prepare on typecheck so a cold shared install cannot cancel the job
quality/typecheck's 10-minute budget was spent on an uncached gitnexus-shared
npm install plus a full prepare tsc that tsc --noEmit does not need.
Co-authored-by: Cursor <cursoragent@cursor.com>
* ci: stop typecheck-web from canceling before the npm cache can save
Hashing gitnexus-shared into the web cache key forced a cold 650-package
install; the 10-minute job then canceled and never wrote a warm cache.
Co-authored-by: Cursor <cursoragent@cursor.com>
* ci: give format the same 10-minute budget as lint
A cold root npm ci already took 4m19s and canceled prettier at the 5-minute
cap. Lint does the same install and needed 7m41s on that run.
Co-authored-by: Cursor <cursoragent@cursor.com>
* ci: stop installing TypeScript 7 just to compile gitnexus-shared
A dedicated npm ci in gitnexus-shared took 7 minutes to add two packages
(TypeScript 7's optional per-platform binaries) and cancelled typecheck,
Windows pack, and coverage shard 1. Compile shared with gitnexus's tsc.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Address PR review feedback (#3166)
- Run tsc via execFileSync so the compiler path is never interpolated into a shell.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Address PR review feedback (#3166)
- Run tsc as node typescript/bin/tsc so Windows never has to execFile a .cmd shim.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Launch tsc via node and lib/tsc.js on every OS.
The npm .bin/tsc shim is tsc.cmd on Windows, which execFileSync cannot spawn.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(ci): lock eval containment against a dedicated shared npm ci
Co-authored-by: Cursor <cursoragent@cursor.com>
---------
Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
The proposer authors the overlay the benchmark arms are scored with, but its
clone was never sanitized: it could read eval/workflow_bench, i.e. the task
prompts and the hidden oracles it was about to be graded against. The last
diagnostic run did exactly that, reading
inv-feature-list-repos-filter.oracle.test.ts directly, so a proposal could
win the gate by encoding expected behavior into a skill instead of being a
better skill. Sanitize the proposer clone exactly as run_cell already does.
Also remove the PreToolUse tool-input normalizer. It never ran: headless
`claude -p` (2.1.247) dispatches no hooks from inline --settings, a settings
file, project/user/local --setting-sources, or a trusted ~/.claude.json
project entry. Keeping it would read as a control in review while enforcing
nothing, and it was the sole reason the proposer stopped using --bare —
which stays off on its own merits, since bare ignores --tools and would cost
the proposer Grep and Glob.
Blank optional arguments from the OpenAI adapter remain handled where the
code is ours: MCP aliases in local-backend normalizeToolParams. Built-in
Read still rejects pages:"" and the model self-corrects on the next turn.
Co-authored-by: Cursor <cursoragent@cursor.com>
The first fully-logged skill-evolution run failed for five deterministic
reasons that had nothing to do with the candidate under test. Each is fixed
at the layer that actually owns the contract:
- Strict provider adapters materialize omitted optional string arguments as
"". The MCP alias normalizer now treats a blank optional alias as absent
(a blank REQUIRED target is still rejected), and a trusted PreToolUse hook
strips blank strings before Read/GitNexus tool calls.
- MCP semantic errors rode home in a successful envelope and logged as
result=ok. SessionProgress now inspects the payload and reports them as
semantic-error.
- Claude Code's nested sandbox overlays absent root dotfiles with device
nodes, which the provenance snapshot read as unauthorized workspace
changes. Those names are excluded at the workspace root and hidden from
git via an immutable excludes file.
- The proposer could not read /evidence from Bash (missing allowRead entry)
and had no offline gitnexus runner, so it fell back to npx and hit the
network. Both are now mounted; ripgrep is installed in CI.
- selected-rows.json advertised host artifact names that do not exist in the
mount. Rows now name their staged patch_file/transcript_files, the prompt
describes the real layout, and oversized bundles compact artifacts before
dropping evidence rows so no row is silently lost.
Also replaces two benchmark scenarios that main already satisfies
(trivial-version-alias, inv-bug-pdg-note) with non-vacuous ones, verified to
fail against a pristine checkout.
Co-authored-by: Cursor <cursoragent@cursor.com>
The sweep printed error_kind=plan-evidence-invalid and nothing else, so
the reason a cell failed stayed in results.jsonl — an artifact uploaded
after the run, not something a watcher can read while it is still going.
Print the cell's error_detail next to its result line, redacted through
the same credential list as the artifact and bounded, since a
session-error detail carries stdout/stderr tails.
Co-authored-by: Cursor <cursoragent@cursor.com>
A proposer or benchmark session could run for an hour with nothing in the
log between "proposing…" and its final result, so a wedged run looked
exactly like a working one. The last CI failure spent 66 minutes silently
retrying a dead endpoint before saying so.
A session's stdout is evidence and is only written out after redaction,
so it can never be echoed. Add a stdout_observer hook to run_managed that
sees the stream without copying it anywhere, and a SessionProgress
reporter that prints only what can be derived safely: turn counts, tool
names, API retries, and a heartbeat while the session is quiet. API
retries are called out by name because that is the signature of the
gateway wedging.
Progress goes to stdout so the benchmark sweep's lines reach the log
live through the existing echo_stdout passthrough, rather than as a
bounded stderr tail after the fact.
Co-authored-by: Cursor <cursoragent@cursor.com>
The loopback LiteLLM proxy was started with stderr=PIPE, but nothing
drained that pipe after the readiness probe. Once the proxy's request
logs filled the 64 KiB pipe buffer it blocked on write, so every later
session request hung with no HTTP status. The last CI evolution run
burned 66 minutes and $3.98 before dying on ten "Request timed out"
retries with error_status=null.
Send proxy stdout+stderr to a 0600 log file in the gateway work dir
instead, and read startup failure detail from that file.
Also give both ends of the loopback hop a 30 minute budget: high
reasoning effort on a full context window can leave a request without a
first token for longer than Claude Code's default client timeout, so
sessions failed on the clock rather than on real errors.
Selected rows can exceed the 2MiB sandbox bundle even when each file is capped; shrink the seed and stage path so a fat prior artifact no longer kills the evolution job.
Co-authored-by: Cursor <cursoragent@cursor.com>
python -m litellm fails on 1.87 (no __main__); use the venv console entry and fall back through VIRTUAL_ENV under uv run.
Co-authored-by: Cursor <cursoragent@cursor.com>
Reuse the incumbent skill digest instead of walking the tree twice, and
keep the needrestart grep a literal match that actionlint accepts.
Co-authored-by: Cursor <cursoragent@cursor.com>
Prevent schema-four decisions from substituting fabricated task sets and preserve unmeasured cleanup failures in live progress.
Co-authored-by: Cursor <cursoragent@cursor.com>
* chore: release v1.6.10
* fix(eval): derive the pinned runtime version from package.json
The containment suite mounts a GitNexus runtime built from this checkout and
asserts its version equals PINNED_GITNEXUS_VERSION, a constant hardcoded to
"1.6.9" when the harness landed in #2566. The first release after that lands
1.6.10 in gitnexus/package.json, the built runtime reports 1.6.10, and
`eval / containment (ubuntu)` fails on drift the release itself created.
Read the version from gitnexus/package.json instead. The check keeps its real
job -- proving the mounted runtime came from this checkout rather than a
published package -- without a copy that only ever drifts on release day.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
PINNED_GITNEXUS_VERSION did not select a runtime — the sandbox already
mounts gitnexus/dist built from the checkout by the workflow's own
`npm run build`, so the benchmark has always run main, not a release. The
constant only asserted that the checkout's package.json said "1.6.9" and
raised SandboxError otherwise.
That makes it a tripwire aimed at the wrong thing. main is 1.6.9 today,
so it passes; the next release bumps it and every skill-evolution run
hard-fails until someone edits this line — discovered on a Saturday, on a
lane that runs unattended once a week, after the box has already been
started and the proposer session paid for.
What the constant was guarding is still guarded, and better: the sandbox
canary now checks the version reported from inside the sandbox against
the checkout's own package.json, so it still proves the mounted runtime
is the one this checkout built, without a literal that has to be
maintained in lockstep with releases. The digest bindings in
promotion.json (sandbox_dependency_content_digest, graph and oracle
digests) remain what actually pins the substrate a candidate was measured
against.
The remaining check keeps package.json readable and versioned, so a
malformed runtime still fails closed.
The live proof run showed the driver's own lines streaming correctly and
every one of the sweep's 24 cell lines sharing one timestamp
(12:05:29.96) four hours after the sweep began — the exact symptom
echo_stdout was added to remove, still present for the phase that
actually takes the fifteen hours.
`_drain` read with `pipe.read(8192)`. On a BufferedReader that blocks
until it has all 8192 bytes or the pipe closes; it does not return short
reads. A sweep emits a couple of short lines per ~45-minute cell and
never fills 8 KB, so everything sat in the buffer until the process
exited. The tail and the capture were unaffected — they only need the
bytes eventually — which is why nothing caught it before.
The existing echo test could not have: its child wrote one line and
exited immediately, so EOF made `read` return. The new test makes the
child refuse to exit until the echoed line has been observed, so an
implementation that only flushes at EOF deadlocks and fails on the
timeout instead of passing on a technicality. Verified it fails with
`read` and passes with `read1`.
`run_managed` reaps by process group, and cells only ever ran one at a
time before `--workers`. Nothing exercised what happens when a `killpg`
fires while other owned trees are alive — a leaked or shared pgid would
take the siblings down with it, and the sweep would read that as two
more excluded runs, which is exactly what the promotion gate refuses to
decide on.
Three real cells run concurrently: one times out and is force-killed
while the other two are mid-flight with descendants of their own. The
test asserts the victim's descendant never escapes and both siblings
still finish with their output intact.
This is the part of the concurrency change reachable without a real
sandbox — bubblewrap needs unprivileged user namespaces, which the
container this was written in denies, so the bwrap canaries stay skipped
here and run in the named Ubuntu CI job.
`ManagedProcessError.__str__` embeds up to 1000 raw bytes of stderr_tail
(process_control.py:72-73), and run_cell printed it verbatim. That line
was inert until this branch: nothing ever printed the sweep subprocess's
stdout, on success or failure. `echo_stdout` streams it live into the job
log, so the print became a sink — and the only one of its kind here that
skipped `redact_text`, which results.jsonl and every transcript already
apply to exactly this field, for exactly this reason.
GitHub masks the registered secret, but masking only catches that literal
value; it is not the guarantee the other sinks have.
The test drives a ManagedProcessError carrying the token in stderr_tail
and asserts it never reaches stdout — verified to fail without the fix.
Also from the same pass: bind `result_indexes[0]` once in run_claude, and
give the workflow contract test a `findStep` helper instead of five
copies of the same `steps.find` predicate.
The upload step needs a path that does not depend on the sweep step
surviving. It did not need shared state to get one: `runner.temp` is
available in a step, only not in a job-level `env:`, so each of the three
consumers can name `${RUNNER_TEMP}/wfevolve` itself. That deletes the
env var and the step that published it — the previous fix swapped one
threading channel for a sturdier one where no channel was required.
Also from the same review pass:
- `announce`/`keep` drop their default-argument capture of `task["id"]`
and `per_arm`. Late binding only bites a closure invoked after the loop
moves on; these are called synchronously inside `sweep_task_cells`,
which blocks until every wave completes. The trick was guarding against
a race that cannot happen here, while implying to the next reader that
it can.
- `_stub_cell_dependencies` returns the list its teardown appends to
rather than taking it as an out-parameter, dropping the boilerplate
from every call site.
- The workflow's `WORKERS` comment points at the `--workers` help text
instead of restating it, so the rationale has one home.
`--workers` reaches the sweep from evolve.py and from a workflow_dispatch
input. Both default to 1, so nothing about the scheduled lane changes:
the runner is sized for one cell at a time, and a cell starved of CPU
drifts toward its session timeout, which the gate counts as an excluded
run and refuses to decide on.
generation_timeout_seconds is left alone deliberately — it is a
worst-case sum-of-every-timeout bound (843h at current settings), already
far looser than any real run, and concurrency only makes it looser.
Raising the input is gated on the runner resize; the contract test pins
the default so the lane cannot start running 3-way on a 2-vCPU box by
accident.
18 cells at ~48 min each, strictly serial, is 97.4% of a generation's
14.7h. The cells are independent — the wall clock was a scheduling
choice, not a measurement requirement.
`--workers` (default 1) runs the cells of one task concurrently; tasks
stay sequential, so the sanitized graph snapshot each task already builds
before its cells stays a single-writer affair. Threads, not processes:
cells are subprocess-bound and `run_managed` keeps every piece of
ownership state local to its own call, so nothing is shared to race on.
Waves, not one fan-out. The outage breaker counts CONSECUTIVE systemic
failures, and "consecutive" means nothing in completion order — folding
as futures landed would make the trip point flaky between identical runs.
Each wave is folded in submission order once complete, and the next wave
starts only if the breaker held, so the breaker overruns by at most
`workers - 1` cells (the ones already in flight) rather than by a whole
task.
`--workers 1` calls the cell directly rather than using a pool of one.
That is not an optimisation: an async KeyboardInterrupt is delivered only
to the main thread, so a cell on a worker thread is outside the reach of
the ownership cleanup that kills its sandboxed process tree. The default
therefore stays exactly what it is today, Ctrl-C included, and above 1
the flag's help says what is given up.
All bookkeeping stays on the main thread — the results.jsonl append, the
per-arm accumulation, the progress prints, the streak fold. No lock is
needed anywhere, the progress counter cannot race, prints do not
interleave, and results.jsonl keeps its canonical order.
Every future is read. An exception a cell did not expect stays parked
inside its Future until something asks for it; unread, a harness bug
would become a silently missing run instead of a crash.
The sweep's innermost body — clone, sandbox, sessions, verify, oracle,
teardown — was ~260 lines of `main()`'s scope, reachable only by running
the whole sweep. Nothing tested it, and it could not run anywhere except
that loop.
`run_cell(ctx, run_idx, arm)` now owns one (run, arm) cell and returns
its row; `TaskCellContext` is a frozen dataclass holding the per-task
inputs a cell reads, so a cell depends on named fields rather than on
whatever `main()` happens to have in scope. The try/except/finally moves
verbatim, exception whitelist unchanged: a harness bug still escapes
rather than being recorded as an ordinary infra-error and averaged into
the evidence.
The task asset snapshot is now prepared once per task, next to the graph
snapshot, instead of lazily inside whichever cell reached it first.
`TaskAssetCache` is a plain dict (task_assets.py:222-226,340), so the
lazy build was a read-then-write race waiting for a caller that is not
strictly serial. One behavior delta: when that preparation fails, every
cell of the task now reports the same wrapped RuntimeError, where before
the first cell reported the original OSError/SandboxError/ValueError.
The loop keeps all the bookkeeping — progress counter, prints, the
results.jsonl append, per-arm accumulation, the outage streak. Behavior
is otherwise unchanged; this is the seam, not the concurrency.
Six new tests cover it, the first coverage this body has ever had: the
row's task/arm/run/digest bindings, each of the five expected failure
kinds still removing the clone, an unexpected KeyError escaping while
cleanup still runs, cleanup failure overriding the primary outcome, and a
failed per-task snapshot failing every cell closed.
An end-to-end pass over the lane — instance start, job, artifacts,
promotion — found three ways it loses work that has already been paid for.
**Evidence died with the job.** Three budgets have to nest: EventBridge
keeps the box up 24h from ~02:45, the job timeout was also 1440min, and
the sweep had no budget of its own. A job-level timeout CANCELS the job,
so the upload step never runs; and since the box stops 24h after it
starts while a scheduled run can begin well after the cron (the
2026-08-01 run was queued 65min late), the box always won that race —
the runner would simply vanish mid-step. The job now gets 21h, the sweep
step 19h, so a wedged generation fails the step, keeps the job alive, and
still uploads. The nesting is asserted in the contract test.
**The upload could be skipped.** Its path came from an output the sweep
step wrote — the same step whose death is the reason the upload matters.
OUT_ROOT is now a job-level env constant known before anything runs, and
the upload is unconditional: results.jsonl and transcripts are appended
as the sweep goes, so a killed generation still holds the evidence that
explains why it died.
**The lane was memoryless.** `--seed-results` is how a run sees what
already lost (summarize_gate feeds the prior promotion.json to the
proposer), and with the default --generations 1 there is no earlier
generation in-process to supply it — the workflow never passed it, so
every Saturday proposed from a blank slate and could re-propose the same
rejected candidate forever. The lane now seeds from the last successful
run's artifact, best-effort: a first run, an expired artifact, a missing
gh, or a failed download proceeds without it rather than costing a
generation.
Also guards the silent-promotion path: `.claude/skills/*` is gitignored
with a hand-maintained per-skill allowlist, and `git status --porcelain`
— how the workflow detects an applied promotion — is blind to ignored
paths. A candidate skill missing from that allowlist would report "No
promotion this run" after the gate said promote. A test now asserts every
CANDIDATE_SKILLS entry is visible in all three shipped trees.
- `_na` moves next to `measured_cost` in runner_sessions, the function
whose None it renders, so the proposer-progress line stops
reimplementing it inline.
- `evaluate_candidate` derives `ungated_tasks` from the `gated` flag
already on each row instead of accumulating a parallel list, and
reports the carve-out as one aggregate line rather than one per task:
`reasons` is truncated to three entries when it is fed back to the
proposer (summarize_gate), and a growing set of unsolvable tasks must
not crowd out why the candidate actually won or lost.
- run_claude cuts the event window at the result event, so "nothing after
the result is evidence" is a property of what the readers below can see
rather than an assumption that a `system` event never carries a
tool_use block.
- The teardown test builds its stream with the existing `event_stream`
fixture instead of re-joining the prefix by hand.
The gate demanded that the candidate resolve every valid run of every
selected task, regardless of how the incumbent scored. inv-feature-list-
repos-filter fails its hidden oracle on 100% of runs in both arms, so the
quality floor could never be met while it stayed in the set — and its
cost comparison (which ranks who spent more while failing the same
oracle) also fed the per-task regression cap and the median. One task
outside both arms' capability was silently vetoing every future
promotion.
A task both arms measured cleanly — at least min_runs valid runs, zero
exclusions — and that neither ever resolved carries no signal about the
candidate. It is now reported in the decision (`gated: false`, plus an
`ungated_tasks` list and a named reason) and left out of the floor, the
regression cap, and the median. It still runs, and its failures still
feed the proposer as evidence: an unsolved task is the loop's target,
not its veto.
The floor is unchanged everywhere it has signal. A candidate that goes
2/3 where the incumbent goes 1/3 is still rejected as unreliable, and a
generation where NO task resolved anywhere is now `insufficient_evidence`
rather than an efficiency verdict over runs that all failed.
promotion.json goes to schema_version 4 — task rows changed meaning, and
a stale v3 binding must not be applied under the new rule.
Run 29907431284 printed its whole 14h45m of output at one timestamp
(00:02:59.32) as the process exited: stdout is a pipe, so CPython
block-buffered it, and there was no way to tell a live run from a wedged
one. Three changes make the lane observable in the Actions log:
- PYTHONUNBUFFERED for the driver (workflow step) and for the benchmark
subprocess (its env is an explicit minimal dict and inherits nothing),
so lines reach the log when they are written.
- run_managed grows `echo_stdout`, a passthrough that streams a child's
stdout to stderr as it arrives while leaving the bounded tail intact.
evolve.py enables it for the benchmark sweep — the multi-hour phase,
whose per-run lines previously surfaced only as a tail, and only on
failure. It stays off everywhere else: a Claude session's stdout is the
evidence stream and is written out only after redaction.
- The sweep now announces each cell as it starts (`3/18, 47m elapsed`)
and reports `took=` and `error_kind=` when it finishes, so an excluded
run — the thing that actually blocks promotion — is visible live
instead of only in results.jsonl. evolve.py also reports the proposer's
duration, turns, and cost once the proposal lands.
The evolution loop has not been able to promote anything since it went
online. Run 29907431284 (the last green run) reached the gate and threw
away 5 of its 18 runs, and the gate requires zero excluded runs in both
paired arms — so the generation could never produce a verdict on merit.
Two causes, both in the session layer:
1. Claude Code drains background-task bookkeeping after the final result
event (`background_tasks_changed`, `task_updated`, `task_notification`,
all `type: "system"`). The parent-stream check required the result to
be the literal last event, so three sessions that had exited 0 with a
complete result and usage payload were recorded as session errors.
Trailing `system` events carry no tool_use/tool_result/usage payload
and cannot forge skill or cost evidence; anything else after the
result still fails closed.
2. The 3600s per-session ceiling killed two `workflow` incumbent runs on
inv-bug-pdg-note mid-verification. Successful `workflow` rows in the
same run finished in ~1600-2600s across both sessions, so the ceiling
moves to 5400s and now lives in one shared constant instead of two
argparse defaults that could drift apart.
Also marks the activation checklist against reality: the secrets, the
Environment, the runner, and the validation dispatch are all in place;
the repository variable GITNEXUS_EVOLUTION_ENABLED is the one remaining
gap, and until it is set the Saturday cron skips the job in seconds while
the EventBridge schedule still starts the runner for the day.
* test(scope-resolution): audit the consumers of file-scoped node ids (#2699 part A)
#2699 item 4 — "audit consumers that assume file-scoped ids" — after #2695/#2714
gave function-local CALLABLES position-bearing ids. Tests and findings only; no
production change. That split is deliberate: `impact` reports
`resolveDefGraphId` at CRITICAL with 23 DIRECT dependents across 7 modules
(every language MRO builder, both Spring attachers, C++ member lookup,
tryEmitEdge, emitReferencesViaLookup, buildGraphTargetIndex, emitFreeCallFallback,
emitReceiverBoundCalls, preEmitInheritanceEdges, emitDetectedInterfaceImplementations,
phpEmitUnresolvedReceiverEdges, emitRubyMixinEdges, emitRustTraitImplEdges,
emitDartHeritageEdges), so changing that key chain is its own change, not a
rider on an audit.
A2 — detect_changes: CONCERN RESOLVED, now pinned. The worry was that an id
containing `@row:col` re-keys whenever a declaration MOVES, making every edit
look like symbol churn. It cannot: `local-backend.ts` maps diff hunks to
symbols by LINE-RANGE OVERLAP (`n.startLine`/`n.endLine`) and merely REPORTS
`n.id`. Node identity never participates in the match. New structural test
asserts the WHERE clause never gains `n.id =` or `n.id IN`, keeps the one
legitimate id-shaped predicate (the `BasicBlock:` prefix exclusion, #2082 U7),
and confirms the id is returned rather than matched. Structural in the same
idiom as `detect-changes-worktree.test.ts`, and labelled as not proving runtime
behaviour.
A1 — ANSWERED, and the answer is that #2699 is NOT fully closed by items 1-3.
The fail-closed guard is gated on `isOverloadableCallable`
(Function | Method | Constructor), so a function-local VALUE never reaches it.
Measured on a fixture: a top-level `const handler` and a function-local
`const handler` still produce ONE node, `Const:v.ts:handler`. That is the
residual half of the issue's original complaint. Pinned as a KNOWN LIMIT with
its reason (widening identity to values re-keys ~14,700 build-time nodes to
change ~800 persisted ones — the decision recorded in `parse-worker.ts`), and
deliberately NOT fixed here.
A3 — id-persisting consumers, classified:
- detect_changes ................ SAFE (position-keyed; pinned by A2)
- MCP impact/context/trace ...... SAFE (resolve by name/uid at query time)
- bench fingerprints ............ SAFE (digest capture shape, not node ids)
- rust-captures golden .......... SAFE (digests captures, not ids)
- cfg pipeline-pdg snapshot ..... AT RISK by design — pins exact edge ids, so
it trips whenever attribution changes. That is the gate working; #2714
already exercised it.
- wiki / group-contract links ... NOT id-keyed on locals (locals are never
cross-file addressable, per the document-scoped contract of item 2).
Verified: tsc clean; 14/14 across the two touched files; `detect_changes`
reports 0 changed symbols (tests only).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* fix(php): a closure binding is a call SOURCE, not only a TARGET (#2699 part B, S1)
A call made inside a closure binding was attributed to the ENCLOSING scope, so
the closure was a call TARGET but never a call SOURCE: impact(handler,
direction:"downstream") reported nothing even though the closure calls out.
Root cause, probe-measured rather than inferred. Instrumenting
pickCallerCallableDef (graph-bridge/ids.ts) to log every rejection reason shows
the closure's own scope EXISTS and its range DOES contain the call site, but its
ownedDefs is EMPTY, so the ":94" owned-callable filter drops it and attribution
falls through to the ":97" enclosing-scope fallback.
The reason is one missing query rule. javascript/query.ts pairs the binding name
with the closure via @declaration.function anchored on the INNER arrow node, so
anchor.range equals the @scope.function range and pass2AttachDeclarations
attaches the declaration to the CLOSURE's scope. No other language had that
rule — PHP, Rust, Kotlin, Ruby and Dart all captured named function
declarations only. That single omission is the entire empty-ownedDefs cause.
This ports the rule to PHP with the same anchor discipline (@declaration.function
on the inner anonymous_function / arrow_function, NOT on the
assignment_expression wrapper). PHP needs nothing else: it already declares
(anonymous_function) and (arrow_function) as @scope.function, so the rule alone
completes it.
Measured on a fixture: `$handler = function ($x) { return target($x); }` inside
outer() now emits
Function:src/a.php:outer.$handler@3:2 -> Function:src/a.php:target
where it previously emitted `outer -> target`.
The pinned test in closure-binding-labels.test.ts asserted the OLD, wrong
behaviour by design ("to catch that asymmetry changing in EITHER direction"), so
it is INVERTED here rather than deleted, per its own instruction. Its block
comment is corrected to record the measured root cause, including that Kotlin
and Ruby will need BOTH this rule AND a relaxed kind gate (their lambda_literal
/ do_block is @scope.block deliberately, #1757), and that Dart has no closure
scope at all.
Verification: closure-binding-labels 50/50; PHP resolver suites 221/221
(php, php-coverage, php-response-shapes). detect_changes {staged}: 1 changed
symbol (PHP_SCOPE_QUERY), 0 affected processes, risk LOW.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* fix(rust): emit a node for a closure binding and make it a call SOURCE (#2699 part B, S3)
Rust was the one exception to #2687's "a closure bound to a name is a Function
node in every language": `let handler = || target(1);` produced NO graph node at
all, so the closure could be neither a call target nor a call source.
Needed BOTH query channels, which is the finding worth recording. Porting only
the scope-resolution rule (as S1 did for PHP) changed nothing measurable here,
because there was no node to attribute anything to:
- languages/rust/query.ts — closure-binding declaration, @declaration.function
on the INNER closure_expression so anchor.range aligns with the existing
(closure_expression) @scope.function. This is what gives the closure's own
scope a callable in ownedDefs, which is what stops pickCallerCallableDef
falling through to the enclosing fn.
- tree-sitter-queries.ts — @definition.function on the OUTER let_declaration.
This emits the Function NODE that Rust never had.
Note the deliberate anchor asymmetry between the two channels: the graph-node
channel anchors the WRAPPER (matching the existing
(lexical_declaration (variable_declarator ... (arrow_function))) rule), while
the scope-resolution channel anchors the INNER closure (to align with
@scope.function). Getting these backwards silently produces either no node or
an unattributable one, so both sites carry a comment saying so.
Measured on a fixture — `let handler = || target(1);` inside outer():
Function:src/a.rs:outer CALLS Function:src/a.rs:outer.handler@2:4
Function:src/a.rs:outer.handler@2:4 CALLS Function:src/a.rs:target
Previously the whole binding was absent and the call read as `outer -> target`.
The rule also covers `move` closures: the closure_expression node spans the
`move` keyword.
Verification: closure-binding-labels 50/50; rust.test.ts 192/192;
rust-coverage, rust-f70, rust-scope all pass; rust-captures-golden passes
UNCHANGED, so no golden regeneration was required. detect_changes {staged}:
2 changed symbols (RUST_SCOPE_QUERY, RUST_QUERIES), 0 affected processes,
risk LOW.
One caveat on the suite runs: this host times out `beforeAll` hooks at the
default 60s under load — rust.test.ts needed --hookTimeout=600000 to complete,
and a concurrent second vitest run starves worker startup entirely (every test
fails at ~5001ms). Both are host artifacts, not signal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* fix(kotlin,ruby): a closure binding is a call SOURCE, via a Block-scope callable boundary (#2699 part B, S2)
Kotlin and Ruby anchor a closure on a Block-kind scope — Kotlin lambda_literal
and Ruby do_block/block are @scope.block DELIBERATELY (#1757 smart casts), so
they must not be re-kinded. pickCallerCallableDef gated its child-scope walk on
kind === 'Function', so a closure there could never become a call SOURCE.
Both halves are required; neither alone changes anything:
1. kotlin/query.ts and ruby/query.ts gain the closure-binding declaration rule,
with @declaration.function on the INNER lambda_literal / block so its range
aligns with the @scope.block range (the anchor discipline documented in
javascript/query.ts). Without this the closure scope owns no callable def.
2. pickCallerCallableDef accepts a Block-kind child as a callable boundary when
the scope IS that callable's body. Without this the kind gate still rejects.
The alignment test in (2) is the part worth scrutiny. Relaxing the kind gate to
accept ANY Block owning a callable would be a real regression: a nested
`fun foo()` declared inside a block is owned by that block, so a call made at
BLOCK level — outside foo — would be misattributed to foo. Comparing the def's
declaration position against the scope's start position discriminates them: for
a closure the declaration and the scope sit on the SAME node, so the positions
match; for a nested function the block starts at `{` while the def starts at the
declaration, so they do not. Existing Function-kind behaviour is untouched, so
every already-working language is unaffected by construction.
The comparison is base-safe: scope-extractor.ts builds a def id as
`def:<filePath>#<startLine>:<startCol>:<type>:<name>` from the same Range a
scope carries, so both sides share one coordinate base. This is called out in
the helper's docblock because `defStartLine` nearby documents its own output as
1-based, which invites a wrong "fix" (#2377 is exactly this class of hazard).
Ruby's call forms are restricted to lambda/proc by name: an unrestricted
(call block: (block)) would match ANY method call taking a block, so
`mapped = items.map { |i| ... }` would wrongly declare `mapped` a callable.
Verified against the parser: 3 matches (->, lambda, proc), map excluded.
Separate #eq? patterns rather than one #match? alternation, which is a known
hazard on this tree-sitter line.
Measured on fixtures:
Kotlin Function:src/A.kt:outer.handler@2:4 CALLS Function:src/A.kt:target
Ruby Function:src/a.rb:outer.handler@4:2 CALLS Method:src/a.rb:target#1
previously `outer -> target` and `outer#0 -> target#1`.
The pinned Kotlin test asserted the old behaviour by design and is INVERTED, not
deleted. Ruby had NO pinned case, so a new one is added rather than inverted.
The describe title no longer claimed something false ("not yet a call SOURCE"
now holds only for Dart) and was retitled.
Verification: closure-binding-labels 51/51; kotlin.test.ts, kotlin-coverage,
ruby.test.ts, ruby-scope, ruby-namespaced all pass (478 passed / 1 expected
inversion before the test was flipped). impact on pickCallerCallableDef:
CRITICAL, 191 impacted, ONE d=1 (resolveCallerGraphId) — the return contract is
unchanged, so that dependent is unaffected. detect_changes {staged}: 5 changed
symbols, 2 affected processes (both EmitReferencesViaLookup, one of them the new
ScopeIsCallableBody step), risk medium.
Dart remains the last failing language: dart/query.ts declares no
@scope.function at all, and dart/captures.ts synthesizes one only from a
declaration WITH a body node, which an expression-bodied closure lacks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* fix(dart): give a closure binding a scope and a distinct identity (#2699 part B, S4)
Dart was the last language where a closure binding could not be a call SOURCE,
and fixing only that would have made the graph WORSE, not better. This lands
both halves together for that reason.
## The attribution half
A Dart closure had no scope at all. `dart/query.ts` declares no
@scope.function anywhere — Dart's function scopes are SYNTHESIZED in
`dart/captures.ts` from `declNode` + `findFunctionBody(declNode)`, and
`findFunctionBody` looked only at the next named SIBLING for a `function_body`.
A closure literal carries its body as a CHILD (`function_expression_body`), so
it matched nothing and no scope was produced.
`query.ts` gains the closure-binding declaration rule and `findFunctionBody`
understands the child form. Deliberately NO @scope.function is added to the
query: it would collide at identical range with the synthesized one, and
duplicate scope ids make `buildScopeTree` throw, which DROPS THE WHOLE FILE.
## The identity half, and why it is not optional
With attribution alone, two same-named closures in one file both keyed to the
bare `Function:a.dart:handler`. One node then appeared to call BOTH targets —
a CALLS edge present nowhere in the source. That is worse than the missing edge
it replaced, so S4 could not ship without this.
Root cause is not Dart-specific. `enclosingCallablePrefix` derives a SEMANTIC
relation — what encloses this callable — by SYNTACTIC ancestor walk. Dart parses
`int outer() { … }` as `function_signature` followed by `function_body` as
SIBLINGS, so the enclosing callable is never an ancestor of code inside it and
no membership set can fix that; the walk looks in the wrong direction.
This is what SCIP and real compilers avoid by construction. SCIP keeps a local
symbol opaque (`local <id>` — no name, no position, no chain) and models
containment as a SEPARATE `enclosing_symbol` field; its spec says the local/global
choice should follow ACCESSIBILITY, not the ability to name an enclosure. Dart's
own analyzer answers this from `Element.enclosingElement` in the element model,
never from AST ancestry. clang uses `name@offset` for a function-local; Kythe
uses a document-scoped VName plus a `childof` edge. Identity is positional and
opaque; enclosure is a relation.
`findSplitBodyCallableAncestor` is the narrow fix at that seam: a fallback used
ONLY when the ancestor walk finds nothing, recovering the callable from the
body's preceding sibling.
The sibling must be a BARE SIGNATURE, and that restriction is load-bearing —
"any preceding callable sibling" is WRONG and was caught regressing PHP during
this work. In `<?php function target($x) {…} $handler = function ($x) {…};` the
closure is at FILE level, so the ancestor walk correctly finds nothing, the
fallback runs, and an unrestricted version mis-qualified the file-level
`$handler` as `target.$handler`. A preceding sibling is only an ENCLOSING
callable when it cannot hold its own body.
`SPLIT_SIGNATURE_NODE_TYPES` is exactly that set and is DERIVED, not listed:
`LOCAL_SCOPE_BODY_NODE_TYPES` is already `FUNCTION_NODE_TYPES` minus the bare
signature types, so the difference between them IS the split-signature set
(`function_signature`, `method_signature` — verified at runtime). PHP's
`function_definition` carries a body and is in both, so it is excluded. No
language is named in shared code, and any future split-grammar language is
covered for free.
## Verification
Full resolver sweep — the gate that caught #2714's Rust regression — 2926
passed / 1 skipped / 0 failed across 51 files. closure-binding-labels 52/52;
dart.test.ts, dart-coverage, callable-id-lockstep, function-local-identity,
caller-identity-regression all pass (156/156 across 6 files).
impact on `enclosingCallablePrefix`: LOW, 5 impacted, 3 d=1 all inside
parse-worker. detect_changes {staged}: 5 changed symbols, 0 affected processes,
risk LOW.
Three existing Dart expectations FLIPPED rather than being deleted: Dart locals
now carry the same enclosing-callable + position identity every other language
got in #2695, so `local.dart:handler` became `local.dart:caller.handler@1:2`.
A new test pins the actual defect — two same-named closures staying DISTINCT
nodes — because the qualification assertions alone would not fail if the
fabricated edge returned.
One note for future work: an id-shape assertion here carries a call-site suffix
on indirect invocations (`…handler@3:2:5:9`) but not on direct calls. That is
the callable-value-flow pass keying its edge by invocation position, not part of
the node id.
Part B is now complete: PHP (S1), Rust (S3), Kotlin + Ruby (S2), Dart (S4).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* fix(scope-resolution): close every deferred item on #2699 (A1 values, twin-list guard, schema bumps)
Clears the limitations this PR had been carrying rather than leaving them as
follow-ups.
## A1 — function-local VALUES now carry their own identity
This was #2699's ORIGINAL complaint and the one a callable-only gate could never
reach: a top-level `const handler` and a function-local `const handler`
collapsed onto ONE `Const:v.ts:handler`. #2695 restricted position-qualified
identity to Function|Method|Constructor because the collision that produced
wrong CALLS edges was between callables, and widening churned ids for symbols
the pruner mostly deletes. The churn is real and is accepted here deliberately.
Widening needed THREE gates aligned, not one:
- id-building — `parse-worker.ts` nestedCallablePrefix
- resolution — `ids.ts` position key
- registration — `node-lookup.ts` position-key registration
Missing the third would register no position key for values, so every lookup
misses and falls through silently. That is the #2714 failure mode: the caller
attaches to a node that does not exist and the edge is DROPPED, which looks like
"zero dangling edges" from outside. All three now route through ONE predicate,
`isPositionQualifiedLocalLabel`, rather than repeating the label set a third
time.
Only LOCALS move. The prefix comes from `enclosingCallablePrefix`, which returns
undefined when nothing encloses the declaration, so top-level and class-member
ids are untouched — verified by the full resolver sweep, where a leak onto class
members would have broken assertions in every language. `Property` is included
on purpose: a class field stays unqualified because the prefix walk boundaries
on class-likes, while an object-literal property inside a function is genuinely
local and would otherwise keep the old collision.
Measured: `Const:v.ts:handler` + `Const:v.ts:run.handler@3:2`, two distinct
nodes. The KNOWN LIMIT test is FLIPPED per its own former instruction ("this
test should be updated as part of it rather than deleted").
## Schema bumps — required by Part B, not just by A1
INCREMENTAL_SCHEMA_VERSION 20 -> 21, parse-cache SCHEMA_BUMP 27 -> 29.
SCHEMA_BUMP is 29, not 28, and that is the point of re-checking it against
origin/main at MERGE time rather than branch time. This branch cut at 27 and
bumped to 28; #2415 also bumped 27 -> 28 and merged first. The automated
main-merge onto this branch surfaced the collision — leaving it at 28 would have
shipped this whole change with NO parse-cache invalidation, so every warm cache
keeps replaying the pre-fix captures and ids. This is the third instance of that
collision recorded in parse-cache.ts (#2632/#2653 hit it at v21, and
#2653/#2654 hit INCREMENTAL_SCHEMA_VERSION the same way).
Part B already changed emitted node ids AND edges on files that did not
themselves change (Dart locals re-keyed, Rust gained a node it never emitted,
five languages gained closure-source attribution). A v20 index topped up
incrementally keeps serving the old attribution, and a warm parse cache replays
the old captures and ids verbatim. Shipping S1-S4 without these would have let
every existing index silently keep the pre-fix graph.
## Twin-list drift guard — the sixth instance in this family
`IMPLICIT_RECEIVERS` (gitnexus-shared lookup-core.ts) and `THIS_RECEIVERS`
(type-env.ts) spell the same concept in two packages, and nothing enforced
agreement — `$this` was added to the shared list in #2714 only because it was
already in the other. New structural test asserts set equality plus the ONE
deliberate asymmetry (`Me`, Visual Basic spelling, absent from the shared list
because no SupportedLanguages entry uses it) in BOTH directions, so re-adding it
there or dropping it here each fail loudly.
Structural rather than value-imported: both constants are module-private, and
exporting them purely to be testable would widen two public surfaces to satisfy
a test.
## Two false comments corrected
- `lookup-core.ts` said "see the drift guard noted in #2714", implying a guard
existed when it was only a deferred follow-up. It exists now, and the
comment points at it.
- `callable-id-lockstep.test.ts` claimed its regex "fails if any site
reconstructs the id". It matches ONE template spelling; a hand-rolled
concatenation still slips past. Now stated as a tripwire for the known
shape, not a proof.
## Skill learnings
Four entries appended to eval/workflow_bench/learnings.jsonl from this run: the
v9fs safe-writer failure, backticks silently terminating a query template
literal (hit three times), a module-level TDZ const that passes tsc and then
presents as N file failures with ZERO failing assertions, and concurrent vitest
runs starving worker startup so a whole suite fails at ~5001ms.
## Verification
Full resolver sweep 2926 passed / 1 skipped / 0 failed (51 files) — identical to
pre-A1, which is the evidence that only locals moved. All EIGHT bench gates PASS
with fingerprints UNCHANGED, so no regeneration was needed. function-local-identity,
callable-id-lockstep, receiver-twin-list-drift and closure-binding-labels 71/71.
tsc --noEmit clean.
detect_changes {staged}: 9 changed symbols, 14 affected processes, risk HIGH —
expected, and the reason the sweep above is the gate rather than a targeted list.
Every affected process routes through `resolveDefGraphId`, the key chain Part A
measured at CRITICAL with 23 direct dependents.
Deliberately NOT done: the SCIP end state (opaque `local <id>` plus an explicit
enclosure EDGE instead of containment encoded in the id string). It is a design
direction, not a limitation of this work, and it is INCOMPATIBLE with A1 — A1
widens chain-encoded identity, that removes chain encoding entirely. Bundling
both would re-key every local twice. Written up in the research notes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* test: update two assertions the #2699 changes correctly invalidated
Both failed on CI at da3d8397 and are fixed here. Neither is a behaviour
regression; both pinned values that this PR deliberately changed.
1. this-boundary.test.ts — "a Kotlin lambda still sees the receiver"
The `this.m()` edge still exists and `this` still resolves to the enclosing
receiver, which is the ONLY property this test exists to guard (its own comment
said so: "what matters here is only that the `this.m()` edge still exists at
all"). Only the SOURCE moved, from `run` to the lambda:
Method:K.kt:K.run#0 -> Method:K.kt:K.run.f@2:16
Method:K.kt:K.run.f@2:16 -> Method:K.kt:K.m#0
The comment justifying the old expectation is now false and is corrected rather
than left: it said the lambda "is not its own caller anchor" because Kotlin
scopes `lambda_literal` as a BLOCK. Kotlin still scopes it as a block (#1757 is
unchanged) — what changed in S2 is that a Block-kind scope is accepted as a
caller anchor when the scope IS the callable's body.
2. call-summary-schema-version.test.ts — INCREMENTAL_SCHEMA_VERSION pin
Moves 20 -> 21 with the bump, which is the point of pinning it: a change that
alters emitted ids or edges without bumping would otherwise ship silently.
Also adds the missing reuse-gate case. `passesReuseGate(20)` now asserts FALSE —
a v20 index predates closure bindings becoming call SOURCES, the Rust node for
`let f = || …`, the Dart closure scope + enclosing-callable identity, and
position-qualified function-local values. Topping such an index up incrementally
keeps serving the old attribution, including the Dart case where two same-named
closures collapsed onto one node and asserted a CALLS edge present nowhere in
the source.
Why CI found these and local verification did not: the verification set was
`test/integration/resolvers/` plus a hand-picked list, and both failures sat
outside it — one integration test about `this` (which a caller-attribution
change obviously touches) and one unit test pinning the exact constant that was
bumped. Grepping for the changed constant, and for tests asserting closure
attribution, would have found both. All 8 suites that reference the schema
constants were then run: 177/177, no third pin.
Verification: this-boundary + call-summary-schema-version 17/17;
the 8 schema-referencing suites 177/177. detect_changes {staged}: 0 changed
symbols, 0 affected processes, risk LOW (assertion-only edits).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* fix(scope-resolution): resolve every finding from the multi-engine review of #2699 part B
The first cut of Part B shipped four P1 defects. A two-engine review (Claude
swarm + ce personas; Codex gpt-5.6-sol swarm + ce + adversarial) found all four,
three of them because an independent engine disagreed with the authoring one.
Each is fixed here and pinned in test/integration/closure-review-findings.test.ts.
## P1-1 — a multi-line closure binding fabricated a CALLS edge
The worst of the four, because it reintroduced the exact defect class #2699
exists to remove. The two query channels anchor on DIFFERENT nodes by design
(graph-node on the outer wrapper, scope-resolution on the inner closure) and the
bridge joins them on line only. Same line, the join matches. Split across lines:
$multi =
function ($x) { return target($x); };
the join missed, `resolveDefGraphId` failed closed, and `resolveCallerGraphId`
then CLIMBED to the parent scope — emitting `outer -> target` although `outer`
calls nothing, while the real `outer.$multi` node sat with zero outgoing edges.
`resolveCallerGraphId` now fails closed at the owning callable instead of
climbing. If we have identified the callable that owns a call site and cannot
name its graph node, crediting an ancestor is not graceful degradation — it
invents a relationship. A missing edge is the correct failure direction for a
graph whose consumers include `impact`.
Getting there took two attempts, worth recording: the first guard keyed on the
def's qualifiedName carrying the `@line:col` local marker, but that suffix is
added by parse-worker for GRAPH NODE ids and scope-resolution defs do not have
it, so the guard never fired. `pickCallerCallableDef` now reports whether the
callable came from a child scope, and the fail-closed applies to the owning
callable either way.
## P1-2 — TS constructor parameter properties were re-keyed as locals
A REGRESSION against the base, not merely an incomplete fix. Admitting
`Property` to the position-qualified set made the enclosing-callable walk reach
the constructor's `method_definition` THROUGH the parameter list — a
LOCAL_SCOPE_BODY hit that lands before any class boundary — so
`constructor(private readonly port: Port)` produced
`Property:svc.ts:Service.constructor.port@2:14` instead of `Service.port`. That
silently empties the slot `impact`, `rename` and FTS address while the class
still asserts HAS_PROPERTY against it, and it is the Angular/NestJS DI idiom.
A real instance exists in this repo at src/core/group/service.ts:304.
parse-worker.ts already computed the correct exemption (`isFunctionLocalProperty`,
lines 2245-2257) two lines above; the new ternary discarded it. Now reused, so
the owner-edge decision and the id decision cannot disagree.
## P1-3 — Dart top-level and `final` closures were never call sources
The rule matched only `initialized_variable_definition`, Dart's FUNCTION-LOCAL
shape. A top-level `var` is `initialized_identifier` and a top-level
`final`/`const` is `static_final_declaration`; the second declarator of
`var f = ..., g = ...` is also `initialized_identifier`. None got a declaration
capture, so `findFunctionBody` never synthesized their scope.
dart/captures.ts ALREADY listed all three in bindingNodeTypes for callable-flow
— the declaration rule simply did not mirror it. It does now.
## P1-4 — Ruby `do ... end` and `Proc.new` closures were uncovered
`do ... end` is the dominant MULTI-LINE Ruby style and produces `(do_block)`;
all three patterns matched `(block)` only. The scope channel already covered
both, so these closures got a Block scope owning nothing and their calls fell
through to the enclosing method. The PR's own Ruby test used the brace form, so
it passed.
Fixing it needed BOTH channels — tree-sitter-queries.ts had no graph-node rule
for the `(call)` forms either, exactly as Rust did. Verified: brace, do/end and
Proc.new are now all sources.
## Also from the review
- The split-signature fallback could fire on VALID TypeScript: a
`declare namespace` containing a bodyless overload made the next declaration's
`export_statement` a sibling of a `function_signature`, so `send` became
`internalHelper.send@2:9`. The fallback now requires the matched node to be
the signature's BODY (a body holds statements; a declaration wrapper holds
another signature), which separates the two without naming a grammar.
- `isCallableDef` re-spelled `Function | Method | Constructor` in the same file
that imports `isOverloadableCallable` and calls it three times — a NEW twin
list, in the PR whose headline is a twin-list drift guard. It now delegates.
- A partial edit had left a self-contradictory comment in parse-worker.ts
("Restricted to CALLABLE labels: the / Applies to VALUES as well as callables").
- eval/workflow_bench/learnings.jsonl carried a "skill": "gitnexus-plan" entry,
but that skill's SKILL.md:347 states feedback is chat-only and forbids
appending learnings during a planning task. Dropped; the three gitnexus-work
entries are sanctioned and stay.
- Ruby's lambda/proc patterns tested the method NAME only, so `MyMod.lambda { }`
was captured as a closure binding. `!receiver` now constrains them.
- Rust's closure work (S3) had ZERO test coverage anywhere — verified once by a
throwaway fixture and never pinned. Now covered.
## Verification
Full resolver sweep plus the identity/closure suites: 3017 passed / 1 skipped /
0 failed across 58 files (up from 2997 — the new tests). This is the gate that
mattered for P1-1: failing closed instead of climbing could have silently
deleted real edges in any language, and ~2900 resolver assertions say it did
not. All EIGHT bench fingerprint gates PASS with fingerprints UNCHANGED.
tsc --noEmit clean.
detect_changes {staged}: 14 changed symbols, 18 affected processes, risk
CRITICAL — expected, since P1-1 changes the fallthrough of `resolveCallerGraphId`,
the key chain Part A measured at CRITICAL with 23 direct dependents.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
---------
Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(eval): give vitest a writable .vite-temp inside read-only dependency mounts
Every task verify command and every hidden oracle ends in `npx vitest run
<test>`, and both run through run_verify with read_only_workspace=True. Vite
transpiles a TypeScript config by writing
<node_modules>/.vite-temp/<config>.timestamp-*.mjs before it loads anything, so
against a read-only dependency mount vitest dies with EROFS before a single
test executes:
EROFS ... /workspace/gitnexus/node_modules/.vite-temp/vitest.config.ts.timestamp-*.mjs
This is pre-existing and was masked: until #2627 the verify command died at
`npx: not found`, short-circuiting the `&&` chain before vitest ran. Confirmed
by reproducing it at that merge base with npx bypassed entirely
(`./node_modules/.bin/vitest`), so it is independent of the node-prefix mount.
Because it blocks the oracle as well as the authored-test verify, `resolved`
stays 0/N without this.
bwrap cannot create a mount point inside an already-read-only bind -- the same
constraint that put SANDBOX_NODE under /opt/claude -- so overlaying a tmpfs only
works if the directory already exists in the mounted bytes. It cannot be
mkdir'd into the dependency snapshot after capture either: the snapshot is
digest-bound and validate_dependency_binding fails closed on drift. So the empty
directory is captured during dependency capture, before the manifest and both
dependency digests are computed, making it part of the snapshot rather than an
untracked mutation of it. The sandbox then overlays a tmpfs on exactly that
path; everything else in the mount, and the whole workspace, stays read-only,
and the overlay never reaches the host clone the credited patch comes from.
Scoped to dependency mounts whose target basename is node_modules, so hidden
oracle and skill mounts stay wholly read-only with no writable island.
Note: this shifts sandbox_dependency_content_digest and
sandbox_dependency_manifest_digest, so promotion evidence recorded before this
change is no longer comparable. That is already true of any harness fix that
changes what the sandbox exposes.
Verified on the self-hosted runner through the real path -- TaskAssetCache
.prepare -> stage_task_assets -> prepare_sandbox -> run_verify with the actual
trivial-version-alias verify string: passed, 15/15 tests, no EROFS. Full eval
suite there with GITNEXUS_REQUIRE_BWRAP_CANARY=1: 337 passed, 4 skipped.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(eval): only overlay .vite-temp where the mount source actually carries it
The tmpfs overlay keyed purely on the mount target basename being
node_modules, which also matched the trusted GitNexus runtime mount at
/opt/gitnexus/node_modules. That mount's source is the built runtime and does
not carry a .vite-temp, and bwrap cannot create a mount point inside an
already-read-only bind, so the containment CI job failed:
bwrap: Can't mkdir /opt/gitnexus/node_modules/.vite-temp: Read-only file system
FAILED test_real_bubblewrap_runtime_mount_imports_cli_without_exposing_checkout
My runner probe only exercised the dependency-mount path, so it missed this.
Gate the overlay on the mount SOURCE actually containing the directory rather
than on the target name. task_assets.py captures .vite-temp only into
dependency-snapshot node_modules, so the overlay now fires exactly there and
never on the runtime mount -- and the gate is correct by construction, since a
tmpfs can only overlay a mount point that already exists in the bound bytes.
Adds a regression test for a node_modules mount whose source has no captured
.vite-temp (the runtime-mount shape) getting no overlay, and updates the
positive test to create the directory in its mount source.
Verified on the self-hosted runner: the exact failing test now passes, and the
full containment selection is 124 passed, 4 skipped.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Gergo Magyar <gergomagyar@icloud.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(eval): ignore Claude Code bootstrap noise nested below the workspace root
The planning-phase boundary check excluded Claude Code's own sandbox-bootstrap
paths only at the workspace root: workspace_snapshot tested relative.parts[0]
against WORKSPACE_SNAPSHOT_BOOTSTRAP_NOISE. But Claude Code bootstraps into
whatever directory it is running in, and the benchmark's task prompts cd into
gitnexus/, so the same noise landed one level down as
gitnexus/.claude/.cc-writes -- whose parts[0] is "gitnexus", so it was never
excluded.
In skill-evolution run 29861768554 that accounted for 13 of 18 sessions, each
failing with error_kind plan-evidence-invalid and the identical error_detail
"phase changed unauthorized workspace path(s): gitnexus/.claude/.cc-writes".
The same code path also guards the review phase (runner.py:499), so review arms
hit it as review-evidence-invalid.
Widening the whole set to match at any depth would be wrong: it also contains
package.json, package-lock.json, node_modules and the .env family, and both
gitnexus/package.json and gitnexus/.claude/settings.local.json are real tracked
files whose edits must still be caught. So the root-anchored rule is unchanged,
and a second narrow rule matches only the entries Claude Code itself creates
inside a .claude directory (.cc-writes, agents, commands) at any depth -- never
.claude itself.
The predicate moves into _is_bootstrap_noise so it is directly testable. It is
still evaluated before pending.append, so an excluded directory is never
descended into.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(eval): mount the node install prefix so npx and npm resolve in the sandbox
_runtime_mount_args bound only the `node` binary itself to SANDBOX_NODE. npm
and npx are not standalone binaries -- they are symlinks into
../lib/node_modules/npm/bin/*-cli.js -- so the install prefix carrying both
bin/ and lib/node_modules has to be mounted for them to resolve at all.
On GitHub-hosted images node lives in /usr/local/bin, whose prefix (/usr/local)
is already inside the wholesale /usr read-only bind, so npm and npx came along
for free and the gap stayed invisible. A self-hosted runner's actions/setup-node
installs into its own tool cache, outside /usr, so only the single node file was
bound. Every task's verify command is "cd gitnexus && npx tsc --noEmit && npx
vitest run <test>", so in skill-evolution run 29861768554 all 18 of 18 result
records carried the identical verify_output "/bin/sh: 1: npx: not found" -- no
run could resolve regardless of model output. It reached the model too: the
session transcripts show 12 "npm: not found" failures, with
gitnexus/scripts/build.js dying on `npm ci` with status 127.
Binds Path(node_bin).resolve().parent.parent read-only at /opt/claude/nodejs,
a fresh target outside the already-read-only trees (same constraint that put
SANDBOX_NODE under /opt/claude), and adds its bin/ to SANDBOX_PATH. The bind is
skipped when the prefix already sits inside /usr, /bin, /lib or /lib64, so the
already-covered case does not widen the mount surface redundantly.
SANDBOX_NODE is deliberately unchanged -- sanitized_graph.py and
runner_sessions.py invoke it directly. SANDBOX_PATH is now derived from
SANDBOX_NODE_PREFIX so the two cannot drift, and the minimal-mounts probe
asserts against the constant instead of a duplicated literal.
The real-Bubblewrap npx canary lives in test_proposer_sandbox.py deliberately:
test_workflow_bench.py pins the set of files carrying the canary marker, and it
runs in the eval-containment-linux job, where actions/setup-node also installs
into the tool cache -- so the canary exercises the real failure shape.
Combines plan steps 3-5 into one commit: the mount, SANDBOX_PATH and the pinned
probe assertion are one behavioural change, and splitting them would leave a
commit whose asserted PATH disagrees with the mounted reality.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(eval): only bind a verified node prefix, and stop excluding .claude/agents
Addresses two findings from the branch review of the two preceding commits.
1. The prefix was derived as Path(node_bin).resolve().parent.parent with no
check that the layout is really <prefix>/bin/node. Probed: /opt/bin/node
bound ALL of /opt (every tool cache on a hosted runner), /mnt/tools/node
bound /mnt, and a bare <dir>/node bound <dir>'s parent. That last shape is
not hypothetical -- the pre-existing real-Bubblewrap node canary builds
exactly it (tmp_path/toolcache/node), so eval-containment-linux would have
silently read-only mounted the whole pytest tmp_path inside a containment
test, passing while doing it. This function exists to keep the sandbox
surface minimal, so an unrecognized layout now binds nothing extra and
simply leaves npx unavailable, exactly as before the mount was added.
2. CLAUDE_BOOTSTRAP_ENTRIES also excluded "agents" and "commands" on the theory
that they might appear nested too; only .cc-writes ever was observed. Every
excluded name is a blind spot: once a .claude directory exists
(gitnexus/.claude/settings.local.json is tracked) anything written under an
excluded entry is invisible to the phase-boundary check, and Claude Code
loads .claude/agents relative to its cwd -- which these tasks point at
gitnexus/. Probed: a planning phase could plant
gitnexus/.claude/agents/planted.md with the check reporting nothing, then
the work phase reads it. Narrowed to .cc-writes alone; extend the set from
an observed failure, never pre-emptively.
Re-probed after both fixes: the over-broad mounts are gone while a genuine
tool-cache prefix carrying npm still binds; planted agents/commands content is
caught again; gitnexus/.claude/.cc-writes (the real run-29861768554 failure)
stays ignored; and edits to gitnexus/.claude/settings.local.json are still
caught.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(eval): gate the node-prefix bind on a working npx, not on an npm directory
The guard tested (prefix)/lib/node_modules/npm as a proxy for "this prefix
supplies npx". Test the property actually required instead: a working npx
sitting beside node in a real bin/ directory. .exists() follows the symlink, so
a dangling npx correctly fails the check -- it would not survive the mount
either. The "bin" name requirement stays, because it is what keeps the
parent.parent derivation honest; an npx sitting directly beside node in a flat
directory would make that derivation name the wrong prefix.
This matters because the guard can silently disable the fix it guards: if a
runner's layout failed the proxy check, the prefix would not be bound and npx
would still be missing, reproducing the original failure with no signal.
Testing npx directly means the guard can only pass when the bind will actually
achieve its purpose.
Validated against a real extracted Node distribution (the official nodejs.org
tarball layout that actions/setup-node unpacks into the tool cache) staged at a
tool-cache-shaped path: bin/node is a real file, bin/npx resolves to
../lib/node_modules/npm/bin/npx-cli.js, and the prefix binds while SANDBOX_NODE
is preserved.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Gergo Magyar <gergomagyar@icloud.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The first fully successful real workflow_dispatch run on the self-hosted
runner (https://github.com/abhigyanpatwari/GitNexus/actions/runs/29843028596)
still failed: 17/18 sessions hit error_kind plan-evidence-invalid with
"phase changed unauthorized workspace path(s): .claude/.cc-writes,
.claude/commands, .env, .env.development, ...".
Reproduced directly on the runner (SSM, matching the real sandbox settings
exactly, including enableWeakerNestedSandbox): a single trivial "say OK"
prompt -- no real task, no real API key even -- is enough to make Claude
Code create a synthetic package.json/lockfiles/node_modules, a full set
of .env variants, and .claude/agents, .claude/commands, .claude/.cc-writes
in the workspace on every single session. None of this is something the
model decided to write; it's Claude Code's own internal bootstrap for
running inside an already-sandboxed environment, and it happens
regardless of task or prompt.
enforce_phase_workspace (the planning-phase boundary check: verify the
plan session touched only its one plan doc) already excludes .git for
exactly this class of reason -- harness/tool noise, not substantive diff.
Extends the same exclusion to the empirically-observed bootstrap set.
workspace_snapshot has exactly one use (this check, confirmed via every
caller), so widening its exclusion list can't hide anything in some other
context that actually cares about these paths changing.
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
