[umermjd11]

Summary

This fixes the red develop on cf80216, which PR No. 254 introduced (diagnosis, Discussion 170 log).

DeployPlatformScriptTest.test_writeDeployments_writesImplAndCodehashMetadata and UpgradePlatformScriptTest.test_writeUpgradeMetadata_writesImplCodehashAndCommit both vm.setEnv the process-wide DEPLOYMENTS_NETWORK, to different values. forge runs test contracts in parallel, so the Upgrade test could switch the variable between the Deploy test's setEnv and its write. The Deploy test then read test-impl-metadata.json, a file that was never written. This failed 2 of 3 runs: develop push run 41476524829 and the merge-queue run that dropped PR No. 304.

Changes

• foundry/script/DeployPlatform.s.sol: _writeDeployments(d, deployer) now just calls a new _writeDeploymentsTo(d, deployer, outPath) with _deploymentsFile(""). The script's real output path and JSON are unchanged.
• foundry/test/DeployPlatformScript.t.sol, foundry/test/UpgradePlatformScript.t.sol: both harnesses expose _writeDeploymentsTo, and both tests pass their own file explicitly (deployments/test-impl-metadata.json, deployments/test-upgrade-metadata.json).
  • No test sets DEPLOYMENTS_NETWORK or GIT_COMMIT_HASH any more. The upgrade test already passed the commit hash as an argument, so its setEnv("GIT_COMMIT_HASH", …) was redundant.
  • This also takes DeploymentsPathTest.test_deploymentsFileUsesPrefixAndChainNetwork out of the race, because it reads DEPLOYMENTS_NETWORK twice.
  • The docstrings that said "any future test that reads the key MUST go into this function" are replaced, because that rule only serialized tests within one contract.

Verification

• forge build (real via_ir profile) and the full forge test: 518 passed, 0 failed on the fixed branch.
• forge test --match-contract 'DeployPlatformScriptTest|UpgradePlatformScriptTest|DeploymentsPathTest' run back to back 8 times: 20/20 passed every time. Before the fix the same race failed 2 of 3 CI runs.
• grep -rn 'setEnv("DEPLOYMENTS_NETWORK"\|setEnv("GIT_COMMIT_HASH"' foundry/test: no hits.
• foundry/script/** changed, so per MAINTAINING No. 4.2 the reviewer checks locally that DeployPlatform.run() still writes deployments/<network>.json (the wrapper passes _deploymentsFile("") through unchanged).

Fixes the develop regression from PR No. 254. Owner per MAINTAINING No. 4.6: @robertocarlous / @Santiagocetran, so a review from either of you unblocks the merge queue.

🤖 Generated with Claude Code

[robertocarlous]

Reviewed against develop in an isolated worktree (1 commit on top of cf802168, the current develop tip — branch applies cleanly, git merge-tree --write-tree exit 0, no file list; GitHub mergeable: MERGEABLE / mergeStateStatus: BLOCKED (blocked only on missing approval)). CI on 62fb2793: all 4 checks passed at run 41483946043 — CI OK, Solidity, Python, Docs.

Claimed: forge build and full forge test: 518 passed, 0 failed on the fixed branch.

Verified: CI Solidity check green at 62fb2793. Reproduced locally (required by MAINTAINING No. 4.2 because foundry/script/** is touched): forge test --match-contract 'DeployPlatformScriptTest|UpgradePlatformScriptTest|DeploymentsPathTest' in the review worktree — 20 passed, 0 failed in 31.05s. This covers all three contracts mentioned in the PR as previously racing.

Claimed: DeployPlatform.run() still writes deployments/<network>.json — the wrapper passes _deploymentsFile("") through unchanged.

Verified: grep -n "_writeDeployments\|_writeDeploymentsTo" foundry/script/DeployPlatform.s.sol shows:

• Line 133: run() calls _writeDeployments(d, msg.sender) — unchanged
• Line 410–411: _writeDeployments now delegates to _writeDeploymentsTo(d, deployer, _deploymentsFile("")) — same path as before, just extracted into the new two-argument wrapper

The output path _deploymentsFile("") reads DEPLOYMENTS_NETWORK from the environment at call time (in run()), not at compile time. The only caller of _writeDeployments in production is run(), and that path is unchanged.

Claimed: grep -rn 'setEnv("DEPLOYMENTS_NETWORK"\|setEnv("GIT_COMMIT_HASH"' foundry/test: no hits.

Verified: Running that exact command in the review worktree: no output. Confirmed — neither vm.setEnv("DEPLOYMENTS_NETWORK", …) nor vm.setEnv("GIT_COMMIT_HASH", …) remains anywhere in foundry/test/.

Claimed: The GIT_COMMIT_HASH setEnv in the upgrade test was redundant because the commit was already passed as an argument.

Verified: In the old test, vm.setEnv("GIT_COMMIT_HASH", commit) was followed immediately by harness.writeUpgradeMetadata(newImpl, "dinToken", path, commit) — the value was passed directly as the fourth argument, so the env var was never read by the function under test. The setEnv added env pollution with no effect on the assertion. Removed cleanly.

Claimed: DeploymentsPathTest.test_deploymentsFileUsesPrefixAndChainNetwork was also in the race.

Verified: That test reads DEPLOYMENTS_NETWORK to construct its expected path. Under the old parallel execution, if UpgradePlatformScriptTest had set DEPLOYMENTS_NETWORK=test-upgrade-metadata in between, the expected path would silently change to test-upgrade-metadata rather than the test's intended value. No change was needed to DeploymentsPathTest itself — the race source was the setEnv calls in the other two contracts, now removed.

---

No foundry/src/** changes — no second key needed. No mechanism changes. All claims hold. Fixes the red develop introduced by PR No. 254.

[robertocarlous]

Files changed (3) — as of 62fb2793 (PR head)

Diffed against merge-base cf802168 (develop tip). Branch was cut from the current develop tip — zero develop overlap. GitHub agrees: mergeable: MERGEABLE, mergeStateStatus: BLOCKED (blocked only on missing approval). Confirmed with a local git merge-tree --write-tree dry run: clean (exit 0, no file list).

foundry/script/DeployPlatform.s.sol

Field	Value
Change	Modified
Lines	+10/-1
Diff (what exactly is in this PR)	_writeDeployments(d, deployer) is now a one-line wrapper that calls the new _writeDeploymentsTo(d, deployer, _deploymentsFile("")). The writing logic (resolving impl addresses, building the JSON, calling vm.writeJson) moves into _writeDeploymentsTo, which takes an explicit outPath. run() is unchanged — it still calls _writeDeployments(d, msg.sender).
Functionality — how & why	How: The wrapper pattern preserves the production call path (run() → _writeDeployments → _deploymentsFile("")) while exposing _writeDeploymentsTo to tests with their own pre-determined path. Why: vm.setEnv changes a process-wide environment variable. Forge runs test contracts in parallel, so when DeployPlatformScriptTest set DEPLOYMENTS_NETWORK=test-impl-metadata and UpgradePlatformScriptTest set it to test-upgrade-metadata, either test could read the other contract's value between the setEnv and the subsequent _deploymentsFile("") call — making the Deploy test read a file that was never written. Extracting the path as an argument eliminates the shared-state dependency entirely.
Diff vs current develop HEAD	None — branch cut from current develop tip
Recommended merge proposal	Merged as-is — run() call path verified unchanged, 20/20 focused tests pass locally
Actual merge proposal	Soon
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

foundry/test/DeployPlatformScript.t.sol

Field	Value
Change	Modified
Lines	+19/-22
Diff (what exactly is in this PR)	DeployPlatformHarness.writeDeployments → writeDeploymentsTo(d, deployer, path) exposing _writeDeploymentsTo. test_writeDeployments_writesImplAndCodehashMetadata: removes vm.setEnv("DEPLOYMENTS_NETWORK", "test-impl-metadata"), constructs the path directly as a string literal, and passes it to harness.writeDeploymentsTo(d, deployer, path). Docstring updated to match.
Functionality — how & why	How: The test no longer touches the process environment. The output path foundry/deployments/test-impl-metadata.json is the same file as before, now named explicitly rather than derived from _deploymentsFile("") via a setEnv. Why: Removes this contract's contribution to the DEPLOYMENTS_NETWORK race.
Diff vs current develop HEAD	None — branch cut from current develop tip
Recommended merge proposal	Merged as-is
Actual merge proposal	Soon
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

foundry/test/UpgradePlatformScript.t.sol

Field	Value
Change	Modified
Lines	+14/-24
Diff (what exactly is in this PR)	DeployHarness.writeDeployments → writeDeploymentsTo(d, deployer_, outPath). test_writeUpgradeMetadata_writesImplCodehashAndCommit: removes vm.setEnv("DEPLOYMENTS_NETWORK", "test-upgrade-metadata"), removes vm.setEnv("GIT_COMMIT_HASH", commit) (commit was already passed as a direct argument — the setEnv was redundant), constructs the path explicitly, passes it to deployHarness.writeDeploymentsTo. assertEq message updated from "must match GIT_COMMIT_HASH" to "must match the commit passed in". Docstring "MUST go into this function" constraint removed.
Functionality — how & why	How: Same strategy as DeployPlatformScript.t.sol — path passed explicitly, no env writes. The GIT_COMMIT_HASH setEnv removal is a pure cleanup; the function under test takes the commit as an argument and never reads that env var. Why: This contract was the other source of the race: setting DEPLOYMENTS_NETWORK=test-upgrade-metadata could clobber DeployPlatformScriptTest's env before its write, making the Deploy test read the wrong file.
Diff vs current develop HEAD	None — branch cut from current develop tip
Recommended merge proposal	Merged as-is
Actual merge proposal	Soon
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

---

Verification

Deep verification detail in the separate verification comment. Summary: forge test --match-contract 'DeployPlatformScriptTest|UpgradePlatformScriptTest|DeploymentsPathTest' in the review worktree — 20 passed, 0 failed. run() call path verified unchanged. No setEnv("DEPLOYMENTS_NETWORK" or setEnv("GIT_COMMIT_HASH" remaining in foundry/test/. CI all green at run 41483946043.

Local vs. GitHub agree: git merge-tree --write-tree exit 0 matches GitHub mergeable: MERGEABLE.

[robertocarlous]

Verified at 62fb279: run() output path unchanged, 20/20 focused forge tests pass locally, no setEnv in test/ remaining, CI all green.

[umeradl]

Actual outcome — PR No. 311 merged as-is through the merge queue

This replaces the predictions in Robbert's pre-merge review comment above with what actually happened when the PR landed.

One commit on origin/develop:

1. 8b46e4e0 — the merge of this PR (a merge commit, not a squash), path A. @robertocarlous approved head 62fb2793 and queued it. The queue merged it at 2026-11-02 17:11 UTC with parents cf802168 (develop) and 62fb2793 (PR head). There were no conflicts, as predicted. GitHub shows the PR as MERGED.

Files unchanged from the PR (3 of 3)

foundry/script/DeployPlatform.s.sol, foundry/test/DeployPlatformScript.t.sol and foundry/test/UpgradePlatformScript.t.sol landed exactly as reviewed, with no deviation commit.

Verification

• PR CI on 62fb2793: run 41483946043, green.
• Merge-queue CI on gh-readonly-queue/develop/pr-311-cf802168…: run 41487293886, green.
• develop push CI on 8b46e4e0: run 41488785200, green. develop is green again. It had been red since cf802168 (run 41476524829).
• Before the PR, locally: the real via_ir forge test passed 518/518, and the three suites involved in the race passed 20/20 on 8 back-to-back runs.

Local vs. GitHub agree: yes. The merge was clean, as both predicted.