SIG-743: add storage layout diff CI tooling for UUPS upgrade safety

[worjs]

Context

SIG-743. v1-contract has no automated CI gate for UUPS storage layout drift. forge inspect storageLayout is available but baseline diffing is manual, and the OZ Hardhat upgrade plugin does not catch struct-internal field changes inside mappings. Upcoming work needs this gate active before merge:

• SIG-746: SignalsPositionStorage zombie mapping rename (label change with @custom:oz-renamed-from)
• SIG-740: multi-asset Market struct extension (append-only field additions)

A layout violation on a UUPS proxy corrupts every market and position in storage, so this is a prerequisite for the audit-prep cleanup epic (SIG-742).

Decisions

• Tracked surface limited to the three UUPS contracts (SignalsCore, SignalsPosition, SignalsLPShare). Modules, router, fee policies, libraries, and testonly variants are excluded by design. A separate AST-driven UUPS drift check fails CI if a new non-testonly UUPS contract is added without being registered.
• Baselines under storage-snapshots/ store raw forge inspect <FQN> storageLayout --json output as the human-readable review artifact. Comparison happens on a canonical semantic form (drops astId, uses types[*].label for type identity, recursively walks reachable struct members through mappings/arrays).
• yarn storage:check runs two distinct checks rather than reusing one comparator:
  • Safety check (ground truth = base branch snapshot via git show origin/main:storage-snapshots/<C>.json) allows storage-safe changes only (top-level append, struct member append, annotated rename).
  • Stale check (vs PR-committed snapshot) enforces strict canonical equality, so an intentional layout change must update storage-snapshots/ in the same PR.
• Rename allow rule reads @custom:oz-renamed-from <old> from the AST of the variable's declaration source via build-info lookup. Build-info candidates are filtered first by exact storageLayout equality, then by input.sources[<file>].content equality with the working tree, to avoid resolving against a stale build-info that differs only in NatSpec.
• Top-level additions reject packing into an existing slot (conservative) but struct member appends after the terminal member are allowed. __gap shrink plus new variable is intentionally out of scope.
• No contract source files were modified. The first real layout change lands with SIG-746 and that PR will exercise the rename path end-to-end.

Impact

No public ABI, event, or SDK surface changes. The CI pipeline gains two new steps (Test storage comparator, Validate storage snapshots) between Forge build and Forge test, and actions/checkout@v4 now uses fetch-depth: 0 so git show origin/main:... can resolve the base snapshot.

Downstream effect on tooling-only PRs in this repo: any future PR that changes a tracked UUPS contract's storage must run yarn storage:snapshot and commit the updated baseline JSON, otherwise the stale check fails.

Risk Areas

• scripts/storage-layout/comparator.ts canonical reduction and recursive struct member comparison. False negatives here silently allow unsafe layout changes; false positives block legitimate appends. Comparator unit tests live in scripts/storage-layout/__tests__/storage-layout.test.ts.
• scripts/storage-layout/build-info.ts AST disambiguation. The two-stage filter (storageLayout equality, then source-content equality) is the only thing distinguishing a fresh build-info from a stale one; a wrong choice resolves astId against the wrong AST and could miss or hallucinate annotations.
• First-merge edge case: this PR's storage:check against origin/main warns and skips the safety diff because storage-snapshots/ does not yet exist on main. The stale check still runs. After this PR merges, the safety gate becomes active for subsequent PRs automatically.
• CI checkout fetch-depth: 0 change increases checkout time and history footprint on every CI run.
• storage-snapshots/SignalsCore.json is a 1300-line generated baseline. Reviewers should confirm it matches a clean forge inspect contracts/core/SignalsCore.sol:SignalsCore storageLayout --json against current main rather than reading it line by line.

Out of Scope

• ERC-7201 namespaced storage validation (OZ upgradeable bases). forge inspect storageLayout does not surface namespaced slots, so OZ library version bumps are not covered by this gate.
• Module own-storage invariant guard (TradeModule etc. relying on inheriting SignalsCoreStorage with zero own state). Not enforced by this tool. Recommended as a separate issue.
• OZ Foundry upgrades plugin adoption. Plugin lib is present but unused; integration with the existing _authorizeUpgrade pattern needs separate evaluation.

[signals-reviewer]

Summary

Adds storage layout diff CI tooling for the three UUPS contracts (SignalsCore, SignalsPosition, SignalsLPShare). Two new CI steps run between forge build and forge test: comparator unit tests, and yarn storage:check which validates AST-discovered UUPS coverage, runs a base-branch safety diff (canonical semantic comparison ignoring astId and type-id AST numbers, allowing top-level append, struct-member append, and @custom:oz-renamed-from-annotated renames), and enforces stale-snapshot detection via strict canonical equality against the PR-committed baseline. Build-info files are filtered first by storageLayout equality and then by input.sources[*].content equality with the working tree to avoid resolving astId against a stale build. CI checkout becomes fetch-depth: 0 so git show origin/main:storage-snapshots/... can resolve. No contract source files are modified, the first real layout change lands with SIG-746.

Cross-PR Context

Sibling PRs (SIG-743)

• Only this PR. Tooling-only change with no ABI / event / SDK surface impact, so no companion PRs in v1-subgraph, v1-sdk, v1-server, or signals-app are required. Confirmed against references/impact-map.md.

Issues

• [Performance]: loadBuildInfos is called once by discoverUupsContracts and again by every BuildInfoAnnotationResolver constructor (scripts/storage-layout/build-info.ts:74-79, scripts/storage-layout/index.ts:134). For 3 tracked contracts that is 4 full reads + JSON.parse of every file in out/build-info/.

  Scenario: A fresh forge build in this repo currently produces ~272MB across 3 build-info files. yarn storage:check re-reads and re-parses every one of them 4 times per CI run, adding noticeable wall-clock and memory overhead (peak heap with parsed-JSON copies of all build-info files held simultaneously across the per-contract resolvers). As more contracts are tracked or build-info files accumulate, the cost grows linearly with each added contract.
  Evidence: Verified du -sh out/build-info/ = 272M with 3 .json files. loadBuildInfos (build-info.ts:74-79) loads every file unconditionally; matchingBuildInfos calls it directly; the resolver runs matchingBuildInfos in its constructor; the constructor runs once per tracked contract in safetyCheck (index.ts:134); discoverUupsContracts calls loadBuildInfos again (build-info.ts:258). Caching loadBuildInfos (a module-level memoized loader shared between validateTrackedUupsContracts and per-contract resolvers) would cut this to a single pass.

• [Coverage gap]: No unit test exercises discoverUupsContracts / validateTrackedUupsContracts (scripts/storage-layout/build-info.ts:255-314). This is the gate that prevents silently dropping a new UUPS contract from the tracked list, so a regression here disables storage protection for that contract entirely.

  Scenario: A future refactor changes the testonly/ exclusion (build-info.ts:277) or the linearizedBaseContracts lookup (build-info.ts:279-280) and the bug is benign for all three currently tracked contracts. Someone later adds a fourth UUPS contract; the AST drift check fails to detect it, no [FAIL] is emitted, and an unsafe layout change in that new contract is not gated. The repo-level STORAGE_STRICT_BASE=1 yarn storage:check does not surface the regression because none of the existing tracked contracts trigger the broken branch.
  Evidence: Read scripts/storage-layout/__tests__/storage-layout.test.ts end to end, only compareStorageSafety, canonicalStorageLayout, areCanonicalLayoutsEqual, and BuildInfoAnnotationResolver have coverage. Suggested minimum: a fixture-based test that constructs a fake build-info with (a) a UUPS contract under contracts/, (b) a UUPS contract under contracts/testonly/, and (c) a non-UUPS contract under contracts/, then asserts discoverUupsContracts returns exactly the first FQN, and validateTrackedUupsContracts reports the expected [FAIL] when the tracked list omits it.

Suggestions

• [Robustness]: runForgeInspect uses stdio: ['ignore', 'pipe', 'inherit'] (scripts/storage-layout/index.ts:28). Forge prints solc warnings and progress to stderr; under inherit they are interleaved with the script's own console output in the CI log, making it harder to grep for [storage:check] lines. Capturing stderr ('pipe') and only printing it on non-zero exit would keep happy-path logs clean while still surfacing real failures.

• [Documentation]: The __gap-shrink + new variable pattern is intentionally rejected. Add a one-line comment in compareStorageSafety (or a README.md next to storage-snapshots/) recording that limitation, so the next person working on SIG-746 / SIG-740 does not have to re-derive it from this PR's description.

Verdict

REQUEST_CHANGES: the two issues above are blocking, the build-info caching change is a concrete reliability/cost concern for every CI run, and the missing UUPS-discovery unit test leaves the most safety-critical part of the gate unverified. The two suggestions are also blocking per the review process; either fix or push back with a concrete technical reason.

[signals-reviewer]

Summary

Adds storage layout diff CI tooling for the three UUPS contracts (SignalsCore, SignalsPosition, SignalsLPShare). Two new CI steps run between forge build and forge test: comparator unit tests, and yarn storage:check which validates AST-discovered UUPS coverage, runs a base-branch safety diff (canonical semantic comparison ignoring astId and type-id AST numbers, allowing top-level append, struct-member append, and @custom:oz-renamed-from-annotated renames), and enforces stale-snapshot detection via strict canonical equality against the PR-committed baseline. Build-info files are filtered first by storageLayout equality and then by input.sources[*].content equality with the working tree to avoid resolving astId against a stale build. CI checkout becomes fetch-depth: 0 so git show origin/main:storage-snapshots/... can resolve. No contract source files are modified, the first real layout change lands with SIG-746.

Cross-PR Context

Sibling PRs (SIG-743)

• Only this PR. Tooling-only change with no ABI / event / SDK surface impact, so no companion PRs in v1-subgraph, v1-sdk, v1-server, or signals-app are required. Confirmed against references/impact-map.md.

Issues

• [Performance]: loadBuildInfos is called once by discoverUupsContracts and again by every BuildInfoAnnotationResolver constructor (scripts/storage-layout/build-info.ts:74-79, scripts/storage-layout/index.ts:134). For 3 tracked contracts that is 4 full reads + JSON.parse of every file in out/build-info/.

  Scenario: A fresh forge build in this repo currently produces ~272MB across 3 build-info files. yarn storage:check re-reads and re-parses every one of them 4 times per CI run, adding noticeable wall-clock and memory overhead (peak heap with parsed-JSON copies of all build-info files held simultaneously across the per-contract resolvers). As more contracts are tracked or build-info files accumulate, the cost grows linearly with each added contract.
  Evidence: Verified du -sh out/build-info/ = 272M with 3 .json files. loadBuildInfos (build-info.ts:74-79) loads every file unconditionally; matchingBuildInfos calls it directly; the resolver runs matchingBuildInfos in its constructor; the constructor runs once per tracked contract in safetyCheck (index.ts:134); discoverUupsContracts calls loadBuildInfos again (build-info.ts:258). Caching loadBuildInfos (a module-level memoized loader shared between validateTrackedUupsContracts and per-contract resolvers) would cut this to a single pass.

• [Coverage gap]: No unit test exercises discoverUupsContracts / validateTrackedUupsContracts (scripts/storage-layout/build-info.ts:255-314). This is the gate that prevents silently dropping a new UUPS contract from the tracked list, so a regression here disables storage protection for that contract entirely.

  Scenario: A future refactor changes the testonly/ exclusion (build-info.ts:277) or the linearizedBaseContracts lookup (build-info.ts:279-280) and the bug is benign for all three currently tracked contracts. Someone later adds a fourth UUPS contract; the AST drift check fails to detect it, no [FAIL] is emitted, and an unsafe layout change in that new contract is not gated. The repo-level STORAGE_STRICT_BASE=1 yarn storage:check does not surface the regression because none of the existing tracked contracts trigger the broken branch.
  Evidence: Read scripts/storage-layout/__tests__/storage-layout.test.ts end to end, only compareStorageSafety, canonicalStorageLayout, areCanonicalLayoutsEqual, and BuildInfoAnnotationResolver have coverage. Suggested minimum: a fixture-based test that constructs a fake build-info with (a) a UUPS contract under contracts/, (b) a UUPS contract under contracts/testonly/, and (c) a non-UUPS contract under contracts/, then asserts discoverUupsContracts returns exactly the first FQN, and validateTrackedUupsContracts reports the expected [FAIL] when the tracked list omits it.

Suggestions

• [Robustness]: runForgeInspect uses stdio: ['ignore', 'pipe', 'inherit'] (scripts/storage-layout/index.ts:28). Forge prints solc warnings and progress to stderr; under inherit they are interleaved with the script's own console output in the CI log, making it harder to grep for [storage:check] lines. Capturing stderr ('pipe') and only printing it on non-zero exit would keep happy-path logs clean while still surfacing real failures.

• [Documentation]: The __gap-shrink + new variable pattern is intentionally rejected. Add a one-line comment in compareStorageSafety (or a README.md next to storage-snapshots/) recording that limitation, so the next person working on SIG-746 / SIG-740 does not have to re-derive it from this PR's description.

Verdict

REQUEST_CHANGES: the two issues above are blocking, the build-info caching change is a concrete reliability/cost concern for every CI run, and the missing UUPS-discovery unit test leaves the most safety-critical part of the gate unverified. The two suggestions are also blocking per the review process; either fix or push back with a concrete technical reason.

[signals-reviewer]

Summary

Adds storage layout diff CI tooling for the three UUPS contracts (SignalsCore, SignalsPosition, SignalsLPShare). Two new CI steps run between forge build and forge test: comparator unit tests, and yarn storage:check which validates AST-discovered UUPS coverage, runs a base-branch safety diff (canonical semantic comparison ignoring astId and type-id AST numbers, allowing top-level append, struct-member append, and @custom:oz-renamed-from-annotated renames), and enforces stale-snapshot detection via strict canonical equality against the PR-committed baseline. Build-info files are filtered first by storageLayout equality and then by input.sources[*].content equality with the working tree to avoid resolving astId against a stale build. CI checkout becomes fetch-depth: 0 so git show origin/main:storage-snapshots/... can resolve. No contract source files are modified, the first real layout change lands with SIG-746.

Cross-PR Context

Sibling PRs (SIG-743)

• Only this PR. Tooling-only change with no ABI / event / SDK surface impact, so no companion PRs in v1-subgraph, v1-sdk, v1-server, or signals-app are required. Confirmed against references/impact-map.md.

Issues

• [Performance]: loadBuildInfos is called once by discoverUupsContracts and again by every BuildInfoAnnotationResolver constructor (scripts/storage-layout/build-info.ts:74-79, scripts/storage-layout/index.ts:134). For 3 tracked contracts that is 4 full reads + JSON.parse of every file in out/build-info/.

  Scenario: A fresh forge build in this repo currently produces ~272MB across 3 build-info files. yarn storage:check re-reads and re-parses every one of them 4 times per CI run, adding noticeable wall-clock and memory overhead (peak heap with parsed-JSON copies of all build-info files held simultaneously across the per-contract resolvers). As more contracts are tracked or build-info files accumulate, the cost grows linearly with each added contract.
  Evidence: Verified du -sh out/build-info/ = 272M with 3 .json files. loadBuildInfos (build-info.ts:74-79) loads every file unconditionally; matchingBuildInfos calls it directly; the resolver runs matchingBuildInfos in its constructor; the constructor runs once per tracked contract in safetyCheck (index.ts:134); discoverUupsContracts calls loadBuildInfos again (build-info.ts:258). Caching loadBuildInfos (a module-level memoized loader shared between validateTrackedUupsContracts and per-contract resolvers) would cut this to a single pass.

• [Coverage gap]: No unit test exercises discoverUupsContracts / validateTrackedUupsContracts (scripts/storage-layout/build-info.ts:255-314). This is the gate that prevents silently dropping a new UUPS contract from the tracked list, so a regression here disables storage protection for that contract entirely.

  Scenario: A future refactor changes the testonly/ exclusion (build-info.ts:277) or the linearizedBaseContracts lookup (build-info.ts:279-280) and the bug is benign for all three currently tracked contracts. Someone later adds a fourth UUPS contract; the AST drift check fails to detect it, no [FAIL] is emitted, and an unsafe layout change in that new contract is not gated. The repo-level STORAGE_STRICT_BASE=1 yarn storage:check does not surface the regression because none of the existing tracked contracts trigger the broken branch.
  Evidence: Read scripts/storage-layout/__tests__/storage-layout.test.ts end to end, only compareStorageSafety, canonicalStorageLayout, areCanonicalLayoutsEqual, and BuildInfoAnnotationResolver have coverage. Suggested minimum: a fixture-based test that constructs a fake build-info with (a) a UUPS contract under contracts/, (b) a UUPS contract under contracts/testonly/, and (c) a non-UUPS contract under contracts/, then asserts discoverUupsContracts returns exactly the first FQN, and validateTrackedUupsContracts reports the expected [FAIL] when the tracked list omits it.

Suggestions

• [Robustness]: runForgeInspect uses stdio: ['ignore', 'pipe', 'inherit'] (scripts/storage-layout/index.ts:28). Forge prints solc warnings and progress to stderr; under inherit they are interleaved with the script's own console output in the CI log, making it harder to grep for [storage:check] lines. Capturing stderr ('pipe') and only printing it on non-zero exit would keep happy-path logs clean while still surfacing real failures.

• [Documentation]: The __gap-shrink + new variable pattern is intentionally rejected. Add a one-line comment in compareStorageSafety (or a README.md next to storage-snapshots/) recording that limitation, so the next person working on SIG-746 / SIG-740 does not have to re-derive it from this PR's description.

Verdict

REQUEST_CHANGES: the two issues above are blocking, the build-info caching change is a concrete reliability/cost concern for every CI run, and the missing UUPS-discovery unit test leaves the most safety-critical part of the gate unverified. The two suggestions are also blocking per the review process; either fix or push back with a concrete technical reason.

[signals-reviewer]

Summary

Adds a CI gate that prevents UUPS storage layout drift before merge. Two yarn scripts (storage:check, storage:test) plus three committed baselines under storage-snapshots/ cover SignalsCore, SignalsPosition, SignalsLPShare. The check runs two independent comparisons: (1) safety vs origin/main baseline (allows top-level append, struct member append, annotated rename) and (2) strict canonical equality vs the PR-committed snapshot to enforce snapshot freshness. AST-driven UUPS discovery via linearizedBaseContracts cross-validates that no untracked non-testonly UUPS contract exists, and rejects tracked entries that no longer inherit UUPSUpgradeable.

No Solidity changes; this PR only lights up the gate. The first real exercise of the rename path lands with SIG-746.

Cross-PR Context

No recent signals-reviewer[bot] REQUEST_CHANGES patterns in v1-contract to match against. No sibling PRs for SIG-743 in other repos; this is a tooling-only change with no ABI/event/SDK surface, so cross-repo impact is none.

Issues

No issues found.

Verifications performed

• discoverUupsContracts against the actual repo: grep -rln UUPSUpgradeable contracts/ returns exactly the three tracked contracts (SignalsCore, SignalsPosition, SignalsLPShare). contracts/testonly/ has no UUPS contracts that could create discovery noise.
• The three committed snapshots (storage-snapshots/SignalsCore.json, SignalsPosition.json, SignalsLPShare.json) reflect real forge inspect output (verified spot-check on SignalsLPShare's 2-slot layout and SignalsPosition's mapping/array entries).
• Comparator's struct-member rename path correctly resolves struct-member astIds via findNodeById's recursive AST walk, and compareEntry only consults the annotation lookup on label mismatch, so non-rename diffs stay free of build-info lookups (test struct member rename passes with oz rename annotation confirms).
• Two-stage build-info disambiguation (storageLayout equality, then input.sources[file].content byte equality) covers the stale-NatSpec case; tests build-info annotation lookup first filters by matching storage layout and build-info annotation lookup uses source-content disambiguation exercise both legs.
• Top-level append rule (comparePrefix, lastBaselineSlot block) correctly rejects packing into an existing slot via BigInt(entry.slot) <= lastBaselineSlot. Documented as conservative-by-design; __gap shrink + new variable is intentionally out of scope.
• Strict-base CI mode: workflow sets STORAGE_STRICT_BASE: "1" and fetch-depth: 0, so gitRefExists('origin/main') always succeeds in CI; the warn-and-skip path only fires locally. CI runs on branches-ignore: [main], so push-on-main self-comparison is not a concern.
• First-merge edge case: gitPathExists(baseRef, snapshotPath) returns false on first run, so safetyCheck warns and skips. staleCheck still runs against the PR-committed snapshot. After this PR merges, the safety gate becomes active automatically with no follow-up needed.
• validateTrackedUupsContracts runs in both directions (discovered-but-untracked and tracked-but-not-discovered), so removing UUPSUpgradeable from a tracked contract or adding it to a new one without registering produces a hard failure.

Suggestions

No blocking suggestions.

Optional polish for future cleanup, not required to land this PR:

• TrackedContract.name and TrackedContract.contractName are identical for all three entries in config.ts. Consider collapsing to one field or documenting the build-info-key vs display-label distinction in a future refactor.
• The reverse-direction validateTrackedUupsContracts path (tracked contract no longer UUPS, lines around 1184-1189 of build-info.ts) is not directly unit-tested, while the forward direction is. Trivial loop, low regression risk; can be added when the next storage-tooling PR touches this file.

Verdict

APPROVE: zero unresolved items. The gate is ready to land, and the audit-prep follow-ups (SIG-746 zombie-mapping rename, SIG-740 multi-asset Market struct extension) can now exercise the safe-change paths against this baseline.

[signals-reviewer]

Re-review Summary

One new commit since the last review (d2a2407, 5 files changed). The change is a pure polish pass that addresses both optional follow-ups left in the APPROVED review:

• Collapses the duplicated TrackedContract.name / contractName fields into a single contractName.
• Adds a unit test for the reverse direction of validateTrackedUupsContracts (tracked FQN no longer discovered as non-testonly UUPS).

No source contract changes; no Solidity touched.

Cross-PR Context

No new sibling PRs — gh search org:signals-protocol is:pr SIG-743 in:title returns only PR #75. Tooling-only change, no ABI / event / SDK / subgraph surface impact (references/impact-map.md lists only contract source files; this PR touches none).

Previous Items

• ✅ [Optional polish — name/contractName duplication]: Resolved by d2a2407. TrackedContract.name removed from types.ts; config.ts entries updated; all contract.name references in index.ts migrated to contract.contractName (runForgeInspect error path, safetyCheck map keys + log/error messages, staleCheck map key + error, check() map population). grep -n '\.name\b' across the three core files (index.ts, build-info.ts, comparator.ts) shows no remaining TrackedContract.name access — only AST node .name (ContractDefinition.name), which is unrelated.
• ✅ [Optional polish — reverse-direction UUPS validation test]: Resolved by d2a2407. New test tracked UUPS validation fails when a tracked FQN is no longer discovered (__tests__/storage-layout.test.ts:777-805) constructs a fixture where the discovered set contains only PlainContract and the tracked list contains a non-discoverable TrackedProxy, then asserts the expected [FAIL] ... is tracked for storage snapshots but was not discovered as a non-testonly UUPS contract message. Test passes (yarn storage:test: 23 / 23 ok).

New Issues

No new issues found.

Verifications performed

• Ran yarn storage:test end to end: 23 tests pass, including the newly added reverse-direction validation case.
• Confirmed every TrackedContract consumer migrated to contractName. currentLayouts map is now keyed by contractName consistently across safetyCheck, staleCheck, and check(). Display strings in user-facing error/log messages all use the same field.
• tempProject() (__tests__/storage-layout.test.ts:575-577) creates a unique mkdtempSync directory per test, so the new module-level buildInfoCache in build-info.ts:31 (already added in e3c773b) cannot pollute across tests — cache key is the project root path, which is fresh each run.
• New test isolation: the fixture in the new test only registers UUPSUpgradeable and PlainContract. discoverUupsContracts correctly returns an empty discovered set (no contract under contracts/ inherits UUPSUpgradeable transitively in this fixture), so the reverse-direction [FAIL] is the only error emitted, satisfying errors.length === 1 exactly.
• Confirmed runForgeInspect stderr is now 'pipe' (index.ts:38), the __gap shrink limitation is documented in comparator.ts:233-236, and the buildInfoCache is in place — no regression on the resolved items from the first review cycle.
• No signals-v0 paths touched; CI workflow unchanged in this commit (only the three resolved-already steps Test storage comparator, Validate storage snapshots, and fetch-depth: 0 remain).

Verdict

APPROVE: zero unresolved items. The polish commit cleanly delivers both follow-ups noted as optional in the previous APPROVED review, all 23 tests pass, and no new issues were introduced.