Skip to content

fix(runtime): accept the logical node directory generated tests are mandated to use - #362

Merged
aviggiano merged 1 commit into
mainfrom
claude/issue348-foundry
Aug 7, 2026
Merged

aviggiano merged 1 commit into
mainfrom
claude/issue348-foundry

Conversation

@aviggiano

@aviggiano aviggiano commented Aug 7, 2026 •

Copy link
Copy Markdown
Collaborator

Relates to #348 and #327. This fixes a real defect in the code path #348 points at, and it does NOT explain the two Foundry-only smoke failures. Both claims are load-bearing; the second is stated in the commit message too so it cannot be lost.

The defect

strategy_attempt_test_dir (packages/prompts/src/render.ts:763) renders

path.join(input.node.workspacePath, "test", "foundry", input.node.logicalId)

and every strategy prompt instructs the agent to keep its generated test there. materializeGeneratedTestCompanion (workflow.tsx) searched

<workspace>/test[s]/foundry/<workspaceRelativePath>
<workspace>/test[s]/foundry/<CONCRETE node id>/<workspaceRelativePath>

concreteNodeId is attempt.attemptId, which equals the logical id only when the topology does not expand the node. The production topology's strategies group carries loops: 3, so concreteIdsFor yields externalized-state-accounting-0/-1/-2. On those nodes the one directory the prompt named was never searched, existingCandidates was empty, and the companion failed closed with artifact-contract failure: generated test file is missing ….

The workflow already disagreed with itself: resetTaskArtifactsForRetry clears test/foundry/<logicalNodeId>. It cleared one directory and read from another.

Measured on the expanded topologies (expandTopology): 65 nodes declare ultrafuzz/generated-tests@1 in production; 57 of them have a concrete id that differs from their logical id. In the smoke topology (strategy_loops: 1) all 4 coincide.

The fix

generatedTestNodeIds(task) returns [logicalNodeId, concreteNodeId] deduplicated, logical first. The candidate list maps over it (also deduplicated, so a loops: 1 node does not gain a redundant candidate and cannot trip the existing generated test sources conflict check against itself). The retry reset iterates the same list, because clearing only one accepted directory would let the previous attempt's source survive in the other and be republished as this attempt's work.

Contract behaviour is otherwise unchanged: a test that reached none of the accepted directories still fails closed, and the byte-identity, symlink, hard-link and copy-mismatch guards are untouched.

Verified by execution

  • pnpm -r build → 0
  • pnpm -r test → 0
  • pnpm -w format:check → 0
  • pnpm -w lint → 0
  • Red/green on the template alone. With git checkout origin/main -- packages/runtime/src/templates/smithers/workflows/workflow.tsx, the new behavioural test fails with artifact-contract failure: generated test file is missing generated-tests/Esa.t.sol; restored, it passes.
  • Independently, before writing the fix, I drove the real template (placeholders substituted, JSX tail cut, smithers-orchestrator resolved) end to end through prepareArtifactMirror → materializeGeneratedTestCompanions → verifyArtifacts in a temporary run root. On origin/main a node with concrete=externalized-state-accounting-0 and logical=externalized-state-accounting fails; concrete=logical passes. After the fix both pass. That harness was scratch and is not part of this PR; the committed regression uses the repo's existing new Function extraction pattern.
  • 65/57/4 node counts above came from running expandTopology on both manifests.

Inferred, not executed

  • That fixing this makes any production run's generated tests publish end to end. I did not dispatch a benchmark run, and this repo has no offline path that exercises a real agent writing a .t.sol.
  • Which agents in practice write to strategy_attempt_test_dir versus elsewhere. The fix removes a defect that punished agents that did obey; it cannot help one that wrote somewhere else entirely.

Tests changed, and why

  • generated-workflow-verifier.test.ts — two new behavioural tests (mandated logical directory accepted; a test in the conventional test/ root still rejected) and one new source-shape test pinning the retry reset to generatedTestNodeIds. The pre-existing source-text assertions for the candidate list and the reset were updated to the new shape.
  • invariant-suite-handoff-durability.test.ts — #212 asserted exactly one reset root. Its fixture has attemptId: "handlers" and logicalNodeId: "stateful-invariant-handlers", so it now expects both, in the repository's own plural tests/ root, preserving that test's original point.
  • runtime.test.ts — the compiled-source assertions on the emitted workflow.

Not addressed

The two Foundry-only smoke failures (externalized-state-accounting, lifecycle-view-boundaries) are not this. Their lane pins strategy_loops: 1. My reproduction of the smoke shape found several distinct ways the companion path fails, all data-dependent on what the agent wrote, all indistinguishable from the published bundle — which carries no message field by design. The full analysis, including what I ruled out and what evidence would settle it, is a comment on #348.

🤖 Generated with Claude Code

Greptile Summary

This PR aligns generated-test discovery and retry cleanup with the logical node directory mandated by strategy prompts while retaining compatibility with concrete node directories.

  • Adds a deduplicated logical-first node-ID helper shared by companion lookup and retry cleanup.
  • Searches both accepted node directories without weakening containment, identity, or conflict checks.
  • Adds behavioral and generated-template regression coverage for logical IDs, missing sources, and dual-directory cleanup.

Confidence Score: 5/5

The PR appears safe to merge, with generated-test lookup and retry cleanup consistently covering the prompt-mandated logical directory and the compatible concrete directory.

The shared deduplicated ID helper closes the expanded-topology lookup defect while preserving path containment, byte-identity conflict handling, and fail-closed behavior for missing tests.

Important Files Changed

Filename Overview
packages/runtime/src/templates/smithers/workflows/workflow.tsx Accepts deduplicated logical and concrete generated-test directories and clears the same directory set before retries.
packages/runtime/test/generated-workflow-verifier.test.ts Adds behavioral coverage for logical-directory acceptance, missing-source rejection, and retry-cleanup source shape.
packages/runtime/test/invariant-suite-handoff-durability.test.ts Updates the extracted retry workflow fixture and verifies cleanup of both accepted generated-test roots.
packages/runtime/test/runtime.test.ts Updates compiled-workflow assertions to require multi-ID generated-test discovery.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Strategy prompt] --> B[Agent writes generated test]
  B --> C["test[s]/foundry/logicalNodeId"]
  B -. backward-compatible path .-> D["test[s]/foundry/concreteNodeId"]
  C --> E[Generated-test companion lookup]
  D --> E
  E --> F{Multiple sources?}
  F -- No --> G[Validate and publish artifact]
  F -- Yes, identical --> G
  F -- Yes, different --> H[Fail artifact contract]
  I[Retry] --> J[Deduplicated logical and concrete IDs]
  J --> K[Clear every accepted node directory]
Loading

Reviews (1): Last reviewed commit: "fix(runtime): accept the logical node di..." | Re-trigger Greptile

Context used:

…andated to use

`strategy_attempt_test_dir` (`packages/prompts/src/render.ts:763`) renders
`<workspace>/test/foundry/<LOGICAL node id>/` and every strategy prompt tells
the agent to keep its generated test there. `materializeGeneratedTestCompanion`
searched `test[s]/foundry/<CONCRETE node id>/` instead, so on any node the
topology expands the one directory the prompt named was never visited and an
obedient agent's test failed the contract as `generated test file is missing`.

The production topology expands every `strategies` node three ways (group
`loops: 3`), giving concrete ids like `externalized-state-accounting-0`; 57 of
the 65 nodes that declare `ultrafuzz/generated-tests@1` are affected. The
retry reset one screen above already keyed on the logical id, so the workflow
cleared one directory and read from another.

Both ids are now accepted, deduplicated, logical first. The retry reset clears
every accepted directory, or the previous attempt's sources would survive in
whichever one it skipped and be republished as this attempt's work.

Scope note: the smoke topology pins `strategy_loops: 1`, so its concrete and
logical ids coincide and this does NOT explain the two Foundry-only smoke
failures on #348; that residual is analysed in a comment on the issue.

Refs #348, #327.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@aviggiano
aviggiano merged commit fcf3573 into main Aug 7, 2026
8 checks passed
@aviggiano
aviggiano deleted the claude/issue348-foundry branch August 7, 2026 21:12
aviggiano pushed a commit that referenced this pull request Aug 7, 2026
Brings B1 (#363 threat-model artifact retention) and B2 (#365 expansion
observability) onto the release branch, plus #362 and the rest of main.

workflow-sync.ts: kept #188's accumulating findingsCount AND #350's
refreshVerifiedArtifactDigest re-seal — orthogonal, and dropping the latter
would have silently reverted the #348 fix on this branch.

Co-Authored-By: Claude Opus 5 (1M context) <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.

2 participants