Repository navigation
Conversation
aviggiano
left a comment
There was a problem hiding this comment.
Reviewed from a fresh detached worktree at 9ebfd26 against origin/unstable 809b702. The runtime/dependency build and runtime test compilation passed, as did all 284 focused tests across generated-output, handoff, replay/supersede, preparation lifecycle, and artifact gates.
Two correctness issues remain: authored JSON can be silently omitted by the broad ancestor-name rules, and previously valid v1 handoffs no longer replay when they contain a newly excluded path. Independent synthetic probes reproduce both against the PR while the exact origin/unstable handoff implementation passes. Tracked edits/deletions, multi-generation unknown-JSON edits/deletions, unsafe paths, symlinks, sensitive paths, and bounded classifier cost passed the additional probes. Please address the two inline items and add regression tests before marking this ready.
| const scratch = segments.some((segment) => SCRATCH_DIRECTORIES.has(segment)); | ||
| const buildJson = | ||
| filename.endsWith(".json") && | ||
| (segments.some((segment) => BUILD_DIRECTORIES.has(segment) || segment.endsWith(".sol")) || |
There was a problem hiding this comment.
[P1] Preserve authored JSON in mixed directories
This treats every JSON file anywhere below an out, build-info, build-snapshot, or *.sol ancestor as compiler output. It does not establish that the individual file is generated. For example, adding test/recon/out/Harness.t.sol plus test/recon/out/config.json containing {"workers":1} captures the source but silently omits its configuration. Likewise, test/fixtures/Fixture.sol/cases.json containing authored test vectors disappears while a neighboring README.txt survives. Both are captured by current unstable. These omissions produce a valid manifest with no exclusion diagnostic, so downstream tests receive incomplete authored work. This violates #1308's requirement to preserve configuration and unknown files in mixed trees. Please narrow the classification or provide explicit capture provenance/override so these files remain eligible, and add tests for authored JSON inside these matched ancestors.
| untrackedPaths.set(workspacePathKey(entry), entry); | ||
| return; | ||
| } | ||
| if (isGeneratedWorkspaceOutput(workspaceRoot, relativePath)) return; |
There was a problem hiding this comment.
[P1] Keep already authenticated handoff entries during replay
The new exclusion also runs when applyWorkspacePatch calls captureWorkspaceTree, which always rebuilds its temporary index from HEAD. Files added by an existing dependency patch therefore look untracked again. I captured a normal v1 patch with the exact current origin/unstable implementation that adds .recon/build.json and test/Harness.t.sol; the old implementation applies and verifies it successfully, but this head accepts its manifest, applies its bytes, and then throws workspace patch result tree mismatch because the reconstructed tree drops .recon/build.json. The schema/version and digest are still valid, so a resumed/refreshed run with such a retained dependency is now blocked after modifying its worktree. Path-only determinism within new captures does not preserve existing handoffs. Please distinguish new capture candidates from paths already authenticated by the baseline/result manifest when reconstructing replay trees, and test replay of a pre-change capture containing a newly classified path.
aviggiano
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES.
Re-reviewed a2db63b from the separate reviewer worktree, including current origin/unstable 81aec3b. Both original blockers are fixed: the authored JSON examples survive, and retained v1 handoffs replay with explicit authenticated membership. I checked every production tree-capture caller. Additional probes confirmed that drift, recreated deleted paths, symlink replacements, and forged manifest entries still fail closed, while new neighboring scratch gains no authority.
The runtime/dependency build and runtime test compilation passed; all 286 focused tests and 11 independent probes passed. A separate real Forge 1.7.1 / solc 0.8.37 reproduction still captures normal nested compiler outputs and reaches the secret gate below the size limit; see the inline item.
CI is also not green: Build gates fails strict ESLint because isGeneratedWorkspaceOutput has complexity 25 against the maximum 20 (job 116403415083). Other required lanes were still running when reviewed. This head is not eligible for ready-for-review.
| // and test vectors. Require matching source/artifact names in a compiler layout | ||
| // or a hash-named build snapshot, not merely a familiar directory name. | ||
| const contractArtifact = | ||
| segments.at(-2) === "out" && parent.endsWith(".sol") && filename === `${parent.slice(0, -4)}.json`; |
There was a problem hiding this comment.
[P1] Recognize compiler artifacts whose type name differs from the source filename
Foundry names artifact JSON after the compiled type, not the full source basename. This equality therefore misses ordinary test/script output such as .cov-work/out/Harness.t.sol/Harness.json, as well as extra or differently named types such as .cov-work/out/Fixture.sol/DifferentName.json. I reproduced both with an actual forge build (Forge 1.7.1, solc 0.8.37), not handwritten output. With a synthetic credential-shaped ABI function name in the tracked fixture source, a clean baseline plus one authored test produces a 6,969-byte workspace patch containing these two generated JSON files and the test; the normal artifact secret gate rejects that patch. The source carrying the synthetic signature is already in HEAD and is not part of the patch, so the rejection comes solely from generated output. This leaves #1308's below-limit terminal failure in a common nested layout. Please distinguish real compiler artifacts from authored configuration without requiring identical source/type names, and add real-layout regressions for .t.sol/.s.sol and multiple types per source while retaining the authored-JSON preservation tests.
aviggiano
left a comment
There was a problem hiding this comment.
Verdict: APPROVE.
Re-reviewed bfa4898 from the separate reviewer worktree. All previously reported blockers are resolved. Rebuilding the exact synthetic fixture with Forge 1.7.1 / solc 0.8.37 now produces a 293-byte workspace patch containing only test/Harness.t.sol, and the unchanged artifact secret gate passes. Normal test/script and differently named compiler types are recognized without dropping the authored JSON configuration/fixture cases.
Validation passed: runtime/dependency build, runtime test compilation, all 290 focused tests, 15 independent probes, CI-mode strict lint, and git diff --check. The independent probes cover retained v1 replay, content-shape changes, tracked edits/deletions, drift and recreated deleted paths, forged manifest entries, sensitive/unsafe paths, symlink replacements, malformed/duplicate/non-UTF-8 JSON, oversized files, and bounded inspection. A 15.9 MB valid compiler artifact classified in about 89 ms locally; oversized candidates were retained without payload parsing. I found no remaining correctness or security issue in the authenticated-path propagation or bounded compiler recognizer.
Readiness is still conditional on CI completion: the current Build gates, external static analysis, and Socket checks are green, while remaining release-validation lanes and the Semgrep check were still running at review time. No draft-state change was made. This COMMENT carries the approval verdict because the available account is also the PR author.
|
CI diagnosis for
All other CI lanes are green; |
aviggiano
left a comment
There was a problem hiding this comment.
Verdict: APPROVE.
Confirmed that the only change from approved bfa4898 is in generated-workflow-verifier.test.ts. It fixes the stale captureWorkspaceTree call assertion by requiring treeAuthority and strengthens coverage for authenticated captures, dependency base trees, persisted preparation, manifest paths, and the absence of an unqualified tree-capture call. No production code changed.
The original failing test now passes locally after recompilation. CI-mode strict lint, Prettier, and git diff --check also pass. This retains the previous approval of the production implementation and its 290 focused tests, 15 independent probes, and real Forge reproduction.
Fresh CI run 38738125774 is still in progress. Ready-for-review eligibility remains pending all required checks on 7f893cb; draft status is unchanged.
Coverage and build scratch outside the existing fixed root exclusions can enter
workspace.patchbelow its size limit. Credential-shaped text in generated build JSON can then terminally reject an otherwise valid source handoff at the artifact secret gate.Classify generated files before staging: concrete compiler artifact/snapshot layouts, compiler/cache dumps, LCOV and coverage reports, and recognized corpus paths. Foundry artifact detection uses bounded, no-follow strict JSON inspection for ABI/bytecode and matching compiler metadata in
out/<source>.sol/<type>.json, including test/script files and multiple types per source. Authored JSON config and fixtures inside those mixed directories remain eligible, as do tracked edits/deletions. Unknown, malformed, or oversized compiler candidates keep the existing capture/overflow behavior.Replay and preparation preserve paths already authenticated by retained v1 handoffs and preparation trees, including across resumed dependency chains and replacement publications. New scratch does not gain that authority. Sensitive paths and symlinks still reach the normal rejection checks, and publication secret scanning is unchanged.
Fixes #1308.
Validation:
.t.sol/.s.soland multiple-type output layouts, a roughly 16.6 MB below-limit compiler dump, clean patch publication despite synthetic credential-shaped scratch, included-secret rejection, authored-source/config preservation, tracked edits/deletions, retained v1 handoff replay, and preparation/resume/replacement compatibility.git diff --checkpass.The fixtures and incident description are repository-neutral. This remains a draft for review.


Confidence Score: 5/5
The PR appears safe to merge; no actionable issue was found.Summary
The PR excludes recognized generated files before Git captures workspace changes.
Review checks were based on source and test inspection; tests were not executed.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Workspace files] --> B{Tracked or authenticated path?} B -->|Yes| E[Stage eligible changes] B -->|No| C{Recognized generated regular file?} C -->|Yes| D[Leave out of workspace patch] C -->|No| E E --> F[Check paths and protected source] F --> G[Capture patch and manifest] G --> H[Run unchanged publication secret gate]Reviews (1) · Last reviewed commit: "test(runtime): require tree authority in..." · Reviewed by Greptile