Skip to content

fix(runtime): require a ledger_ids field only where there are ledger IDs - #298

Merged
aviggiano merged 1 commit into
mainfrom
fix/fanin-conditional-ledger-field
Aug 6, 2026
Merged

aviggiano merged 1 commit into
mainfrom
fix/fanin-conditional-ledger-field

Conversation

@aviggiano

@aviggiano aviggiano commented Aug 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

The failure

R45 (18:46:01Z) and R46 (~20:05Z) both failed fan-in with a long list of:

Properties Markdown must preserve canonical property "property-1" with its description, category,
priority, and sources (missing "ledger_ids")

Both passed on a retry, so the pipeline survives — at the cost of a wasted agentic node attempt each time and a coin flip on a node every run must pass.

The measurement that settles it

Read from R46's own fan-in artifacts on the volume:

count
canonical properties in properties.json 219
of those, with a non-empty ledger_ids 78
blocks in properties.md 219
occurrences of ledger_ids in properties.md 78

The model rendered the field for exactly the properties that have one. That is what it was told to do (property-specification-fanin.md:86 — "add a ledger_ids array to every canonical property that represents one or more ledger entries") and what the schema allows (canonicalPropertySchema.ledger_ids is .optional()).

The gate never consulted property.ledger_ids:

const missingLedgerField = block !== undefined && ledgerFieldValues.length !== 1;

So all 141 properties with no ledger mapping were rejected for omitting a field the schema calls optional and the prompt does not ask for.

The fix

const expectsLedgerField = (property.ledger_ids ?? []).length > 0;
const missingLedgerField = block !== undefined && expectsLedgerField && ledgerFieldValues.length !== 1;

Nothing is weakened. Once a property does have ledger IDs, the checks below still enforce that each one is rendered, the right number of times, with no extras — and a regression pins that half explicitly so this cannot be read as dropping ledger parity.

Both my tests were green for the wrong reason first

Worth stating, because the exit status lied: the fixtures omitted the project-discovery evidence ledger, so the gate short-circuited on INVARIANT_LEDGER_MISSING before reaching any parity code, and the "does not demand" test passed against unfixed source. This is the same trap a reviewer caught in #290, and I only found it by reading the diagnostics rather than the pass count. With the ledger supplied, the red state reproduces R46's message verbatim.

Validation

Why this keeps happening

Fifth defect of the same family: a gate enforcing something stricter than the prompt asks and the schema declares. #289 (directory scan probes), #291 (prompt silent on directory probes), #293 (identifier shape), #292 (unfalsifiable probe text), and now this. Individually small; together they are why no run has yet reached report.md. Worth someone auditing the gate/prompt/schema triple systematically rather than one live failure at a time.

Greptile Summary

The PR aligns the property fan-in artifact gate with the optional ledger_ids schema contract, requiring the Markdown field only for canonical properties that contain ledger IDs.

  • Conditions the field-presence check on a non-empty canonical ledger_ids array.
  • Adds regressions covering properties both without and with ledger IDs.

Confidence Score: 5/5

The PR appears safe to merge with no actionable correctness or security issues identified.

The conditional requirement matches the optional canonical field, while existing parity checks continue rejecting missing or extra ledger mappings for properties that contain ledger IDs.

Important Files Changed

Filename Overview
packages/runtime/src/artifact-gates.ts Correctly narrows the ledger field-presence requirement while retaining existing missing, extra, and duplicate ID parity checks.
packages/runtime/test/artifact-gates.test.ts Adds focused fixtures that reach the parity gate and verify both optional-field acceptance and required-field rejection.

Reviews (1): Last reviewed commit: "fix(runtime): require a ledger_ids field..." | Re-trigger Greptile

Context used:

R45 and R46 each lost a full `property-specification-fanin` attempt to this, and
both recovered only because a retry happened to render the field differently.

The gate demanded exactly one `ledger_ids` field in EVERY canonical property's
Markdown block and never consulted `property.ledger_ids`. Two other components
disagree, and the gate was the outlier:

- `canonicalPropertySchema.ledger_ids` is `.optional()`
- the fan-in prompt asks for it on "every canonical property THAT REPRESENTS one or
  more ledger entries"

Measured from R46's own artifacts on the volume: 219 canonical properties, 78 with
a non-empty `ledger_ids`, and `properties.md` renders the field exactly 78 times.
The model did precisely what the prompt and schema allow, and the gate rejected it
with 141 diagnostics.

Nothing is weakened. The parity checks that follow still enforce everything that
matters once a property does have ledger IDs: each must appear, the right number of
times, with no extras. A regression pins that half explicitly so this cannot be read
as dropping ledger parity.

Both tests were initially green for the WRONG reason -- the fixtures omitted the
project-discovery evidence ledger, so the gate short-circuited on
`INVARIANT_LEDGER_MISSING` long before reaching the parity code. Caught by reading
the diagnostics rather than the exit status, the same trap a reviewer found in #290.
With the ledger supplied, the red state reproduces R46's message verbatim.

Verified this rule has a single implementation -- no template twin, unlike #289.

artifact-gates 48/48.

Fixes #297

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@aviggiano

Copy link
Copy Markdown
Collaborator Author

Full monorepo suite green on 052f54e: 442 tests / 428 pass / 0 fail / 14 skipped, EXIT=0.

Deliberately not relaunching a run for this one. R47 launched at 20:06Z from d78fe3a and R46 is at stateful-invariant-setup; this defect costs a wasted fan-in attempt but retries have recovered from it in both runs that hit it, so a third ~$400 run to pick it up would buy very little. It lands for whatever comes next.

@aviggiano

Copy link
Copy Markdown
Collaborator Author

Both reviews: merge, no blocking findings

Both reviewers attacked the coverage question directly and both landed in the same place: the "every ledger entry is mapped" guarantee never lived in the check this PR relaxes. It lives at artifact-gates.ts:477-488 (INVARIANT_LEDGER_REFERENCE_MISSING), a JSON-level check in the entry direction that runs before markdown is opened. The relaxed check is a markdown-level check in the property direction. Orthogonal.

The adversarial reviewer built seven fixtures and ran them against both origin/main and this head:

fixture scenario result on this head
ledger entries with zero properties referencing them, ledger_ids omitted everywhere the feared regression rejected — INVARIANT_LEDGER_REFERENCE_MISSING ×2
2 entries, only 1 mapped partial coverage rejected
property has ["ev-1","ev-2"], markdown renders only ev-1 under-render rejected — MAPPING_MISSING
property has IDs, markdown has two ledger_ids: lines arity rejected
property has no IDs, markdown invents one forged mapping rejected — MAPPING_EXTRA
"ledger_ids": [] in JSON empty list rejected — PROPERTIES_SCHEMA_INVALID

No fixture changed pass/fail between main and this head. The unconditional demand was duplicate noise layered on checks that already fire.

That also answers the [] question: minItems: 1 / .min(1) means an empty array cannot reach the code at all, so (property.ledger_ids ?? []).length > 0 is exactly equivalent to !== undefined post-validation — defensive, not wrong.

Mutation testing killed both mutants, one test each: forcing expectsLedgerField true (≡ main) fails only the "does not demand" test; forcing it false fails only the "still requires" test. Neither survives, and no other test constrains the line in either direction. One reviewer noted the companion test adds coverage that did not previously exist — before this PR nothing pinned the "exactly one ledger_ids field" rule even for properties that do have IDs.

On whether R46's 141 unmapped properties are themselves the defect: no. R46 passed fan-in on retry, which means INVARIANT_LEDGER_REFERENCE_MISSING was satisfied — every ledger entry was covered by the 78 mapped properties. The other 141 are lens-derived properties with no doc-ledger backing, which is legitimate.

The one finding, filed not bundled → #299

Both reviewers flagged that the prompt still reads unconditionally at :93-94 ("include its complete ledger_ids list in that block"), and a fixture confirms a model obeying that literally is rejected with MAPPING_EXTRA — the mirror image of this bug, still live. It is a one-clause prompt change, but changing a prompt changes model output, so it belongs between runs rather than in a gate fix on the critical path of a live investigation. Filed as #299, with a suggestion to land it together with #291 as one prompt-clarity change.

Not acting on the - ledger_ids: none variant a reviewer raised as SUSPECTED: there is no evidence any model emits it, and adding speculative spellings to the filter is the whack-a-mole this series of fixes has been trying to stop.

Validation

  • artifact-gates 48/48; monorepo 442 / 428 pass / 0 fail / 14 skipped
  • every release-gates step reproduced locally
  • Greptile, semgrep and both Socket checks green

Merging with the same recorded deviation as #284, #290, #294 and #296: hosted runners are still evicting queued jobs, so release-gates never ran on CI.

One environment note for future reviewers of this repo, found the hard way twice: in a fresh worktree without node_modules, bare npx tsc fetches a decoy package that prints "This is not the tsc command you are looking for", exits 0, and compiles nothing — so tsc && node --test yields a green run with zero tests executed. And artifact-gates.test.ts test 1 reads dist/topology.yml, a build output, so it fails in any worktree where pnpm build has not run.

@aviggiano
aviggiano merged commit a8780de into main Aug 6, 2026
4 checks passed
@aviggiano
aviggiano deleted the fix/fanin-conditional-ledger-field branch August 6, 2026 20:29
aviggiano added a commit that referenced this pull request Aug 6, 2026
The documentation half of two gate fixes that already landed (#290, #298). Each of
these prompts told the model to do something a gate then rejected, and each cost a
live Aave v4 run a full agentic node attempt before anyone noticed.

**Fan-in `ledger_ids`.** Line 86 was already conditional -- "add a `ledger_ids`
array to every canonical property THAT REPRESENTS one or more ledger entries" --
but two later sentences read unconditionally, so a model rendering the field on a
property whose JSON has none is rejected with
`INVARIANT_LEDGER_MARKDOWN_MAPPING_EXTRA`. The unconditional reading is
unsatisfiable anyway: `ledger_ids` carries `minItems: 1`, so a property with no
mapping has no list to render. Both sentences are now conditioned.

**Project-discovery scan probes.** The prompt never said what kind of path a probe
may name, while its own suggested `grep -rIlE ... test contracts` takes directory
arguments. R45 named `tests` and died on it twice. It now states the real contract:
a real directory or an absent path is fine and is not snapshotted; a symlink is
not, and neither is a path reached through one; a probe naming an existing file IS
snapshotted and must be UTF-8, tracked at the pinned commit and unmodified, so
prefer the directory searched or the absent path over generated or untracked files.

Getting that last part right took three passes. The first draft claimed "only
ledger `entries` are checked byte-for-byte", which is FALSE -- a regular-file probe
goes through the same `readInvariantSourceSnapshot` as an entry source, with
`git ls-files --error-unmatch`, `git diff --quiet HEAD` and a byte comparison
against the pinned ref. I found that by checking my own wording against the code;
two independent reviews then confirmed it as their blocking finding. It was the
exact error class this PR exists to remove, and it would have invited the model to
cite build output.

Reviews also caught that the fan-in file still contained the unconditional twin of
the sentence being fixed four lines below it, that "a directory" was over-broad
given both implementations reject symlinks, and that a pre-existing "use a line
range or symbol" sentence now contradicted the new text while `scan_probes` has no
line-range field at all. All fixed.

Anchored in `semantic-anchors.test.ts` on whitespace-normalised text, so a reflow
cannot masquerade as a semantic regression, with one anchor asserting the false
sentence stays absent. Proven red against the unmodified prompts.

Deferred until now on purpose: prompts change model output, so this had to land
between runs. R46 and R47 both have their prompts baked into immutable images.

Fixes #291
Fixes #299

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aviggiano added a commit that referenced this pull request Aug 6, 2026
… of re-probing

Both reviews landed on the same objection, and the second found the better design:
my first version added a `probeModalSandbox` call to the RECOVERY RELAUNCH path --
the code that rescues a run whose sandbox just died -- purely to improve logging.
`probeModalSandbox` rethrows anything that is not `NotFoundError`, and
`sandbox.poll()` is a real gRPC call, so an exhausted retry budget would propagate
out of the overseer tick; ten consecutive ticks abandon the job. Adding an abort
point to the rescue path in order to observe failures better is the wrong
dependency direction at any probability.

The value did not need a new probe at all. `runningRecoverySandbox` already polls
the outgoing sandbox during ownership resolution and threw the exit code away:

    const exitCode = await sandbox.poll();
    if (exitCode === null) return sandbox;      // non-null discarded

It now returns `{ sandbox }` while live and `{ exitCode }` once exited, the
observation rides out on `ResolvedModalRecoveryOwner.launchExitCode`, and
`launchModalRecoveryWorker` records it. No new network call, no new throw site.

The second probe was also strictly worse at capturing the value: between ownership
resolution and the reservation, the tick creates and terminates an inspector
sandbox and copies credentials, so the id can be reaped in between and a later
probe can only return a worse answer than the earlier one.

Also from review: the runner half was completely unpinned -- deleting it left the
suite green, the same defect class found in #286 and #298. `runningRecoverySandbox`
is now exported and tested across all four outcomes: exited with 137, exited
cleanly with 0 (which must survive rather than be coerced away, since a clean exit
at a node that was supposed to keep working is the most surprising answer of all),
still live, and vanished. Proven red by mutation.

packages/modal 311/311.

Fixes #302

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aviggiano added a commit that referenced this pull request Aug 6, 2026
…303)

Three Aave v4 runs lost their sandbox at `stateful-invariant-setup` -- R45 and R46
at 20 succeeded nodes, roughly $400 and two hours each -- and none could be
diagnosed, because the one number that distinguishes an OOM kill from an eviction
from a clean exit was polled and thrown away.

`runningRecoverySandbox` already polls the outgoing sandbox during ownership
resolution:

    const exitCode = await sandbox.poll();
    if (exitCode === null) return sandbox;      // non-null discarded

On unattended runs -- which is every benchmark run -- the overseer is what observes
these deaths, and its reservation force-closes the dying attempt's lifecycle row
with every diagnostic hardcoded to "unknown" (`launch-state.ts:1147-1160`). R46's
three lifecycle rows all read `worker_exit_code: "unknown"`, including the attempt
whose sandbox died mid-node.

`runningRecoverySandbox` now returns `{ sandbox }` while live and `{ exitCode }`
once exited; the observation travels on `ResolvedModalRecoveryOwner.launchExitCode`
into `reserveModalLaunchAttempt` and onto the lifecycle row.

The first version of this PR instead added a fresh `probeModalSandbox` call to the
recovery relaunch path, and both reviewers rejected it. `probeModalSandbox`
rethrows anything that is not `NotFoundError`, `sandbox.poll()` is a real gRPC
call, and nothing between it and the overseer poll loop catches -- so ten
consecutive failing ticks would abandon a run carrying twenty succeeded nodes, in
order to log why such runs die. Threading the existing observation removes that
failure mode rather than catching it, and captures a better value: between
ownership resolution and the reservation the tick creates and terminates an
inspector sandbox and copies credentials, so a second probe can only return a worse
answer than the first.

Reviewers also found the runner half completely unpinned -- deleting it left the
suite green, the same defect class as #286 and #298. `runningRecoverySandbox` is now
exported and tested across all four outcomes: exited with 137, exited cleanly with
0, still live, and vanished. The 0 case is not just falsy-guard hygiene; a clean
exit at a node that was supposed to keep working would be the most surprising of the
four, and it is exactly the one a careless guard discards.

packages/modal 311/311. Every release-gates step reproduced locally.

Fixes #302

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aviggiano added a commit that referenced this pull request Aug 7, 2026
Integrates origin/main (fa8be64): PR #287's invariant-suite handoff deletion
channel and durable provenance, plus #282, #284, #290, #294, #296, #298, #300,
#303, #305 and #308. CHANGELOG keeps both Unreleased sections; the modal runner
test import list keeps both new symbols.

Fixes the review finding on #165 (issue #212). Compiled task specs can only name
singular `test/...` output roots, because they are built before the worktree
exists. Targets are not consistent: the Aave v4 target at 6959e321 has no `test/`
at all, only plural `tests/`. The generated workflow created the singular tree
regardless, so such a repository grew empty `test/recon`, `test/chimera` and
`test/invariants` directories, which setup.md tells the model to read as the
repository's convention, and retry cleanup then scrubbed that invented tree while
leaving the real `tests/` output behind.

Workspace output roots are now re-anchored at execution time onto the test root
the repository actually uses, restoring the semantics of main's `invariantTestRoots`
helper that this branch dropped in merge 2bbecba, at the layer this branch's retry
preparation uses.

Three review findings on the first cut of that fix are addressed here:

- Exactly ONE root is selected, never the union of those present. Anchoring under
  every existing root would have recreated the same defect from the other side: once
  an ancestor stage introduces the other root, each invariant stage would create an
  empty duplicate tree beside the real one. Preference follows
  INVARIANT_TEST_ROOT_NAMES, so a repository that has `test/` behaves exactly as it
  did before this helper existed and only a repository without it is redirected.
- The resolution is pinned per attempt at preparation time. Rediscovering per call was
  unsafe: prepareTaskWorkspaceOutputRoots finishes by replaying ancestor workspace
  patches, and an ancestor stage can introduce the other test root as a side effect, so
  a second independent discovery could select a root whose subdirectories were never
  created. taskWorkspaceOutputRoots would then lstat a missing path and throw a bare
  ENOENT instead of the typed artifact-contract failure, killing a retries:0 node on
  exactly the restart and resume paths this machinery exists to survive.
- Only ENOENT and ENOTDIR mean "this root is absent". Swallowing EACCES or ELOOP would
  silently fall back to the singular default against a root that does exist.

Test changes, also from review:

- The non-directory case was vacuous: with only a regular file named `test` present,
  "rejected it" and "fell back to the default" both read `test`. It now runs against a
  repository that also has a real `tests/`, so the outputs differ; verified by mutation
  (dropping the isDirectory() guard fails the test).
- taskWorkspaceOutputRoots' use of the helper is now pinned in the generated-source
  assertions. Reverting only that call site previously left every test green while
  every task on a `tests/`-only target died on a bare ENOENT.
- Adds coverage for the pinning behaviour, for per-attempt isolation of the pin, and
  for surfacing an unreadable candidate root.

Also repairs a merge-integration gap the new #217 harness could not have anticipated:
it lifts resetTaskArtifactsForRetry, which on this branch calls prepareAnchoredDirectory
(branch-only, absent from main), so the harness now supplies that collaborator and drops
the dead invariantTestRoots stub.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Property fan-in requires a ledger_ids field on every canonical property, but the schema and prompt both make it conditional

1 participant