Skip to content

feat: namespace-drift CI check for contracts package (#77)#82

Merged
chapati23 merged 2 commits into
mainfrom
feat/namespace-drift-check
Jul 17, 2026
Merged

feat: namespace-drift CI check for contracts package (#77)#82
chapati23 merged 2 commits into
mainfrom
feat/namespace-drift-check

Conversation

@chapati23

@chapati23 chapati23 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Closes #77.

Problem

@mento-protocol/contracts is generated from .treb/deployments.json by scripts/gen-contracts-package.ts, run manually one namespace at a time. Any published namespace nobody remembers to regenerate silently drifts: its live on-chain addresses never reach contracts.json, with no error anywhere. PR #76 revealed ~88 addresses that had been sitting unpublished since v0.8.1 — including Celo mainnet Chainlink relayers consumers may have needed for two months.

What this adds

A pnpm contracts:check-drift guard (scripts/check-namespace-drift.ts) that:

  1. Derives the published namespace set from the committed contracts.json itself — currently mainnet, monad-mainnet, testnet-v2-rc5. Ephemeral namespaces (virtual, throwaway, test, superseded RCs) are excluded by construction, and a newly-published namespace is guarded the moment it first lands in the package.
  2. Regenerates each into a throwaway temp copy using the real generator, then fails if the result adds or re-points any address vs the committed package. Reusing the generator makes the check authoritative — it can never disagree with the generator's own filtering rules (no hand-maintained ignore-list for the intentionally-unpublished SINGLETONs).

Design decisions (the issue's open questions)

  • Detection — regenerate & compare (authoritative), not a lightweight coverage check that would need a drifting ignore-list.
  • Comparison — semantic parsed-JSON compare in a standalone script (immune to formatting, no trunk needed in CI, runnable locally), not a shell git diff.
  • Direction — additive-only (treb ahead of package). The generator never removes keys, so package-ahead-of-treb is out of scope.
  • Location — a step in the existing check job in test.yml, reusing the out/ artifacts already built there (no second forge build).

Generator change

gen-contracts-package.ts honors a new CONTRACTS_OUT_DIR env var to redirect all output to a temp dir and skip the formatter in that mode. Normal contracts:update runs are unchanged.

Verification

  • pnpm test — 30 pass (5 new unit tests for the pure diff/namespace helpers).
  • pnpm contracts:check-drift — passes on main (package is in sync).
  • Simulated drift (dropped 137/mainnet/EUROP from the committed package): the check correctly flagged the missing key, mapped it to mainnet, printed the exact contracts:update remediation command, and exited 1.
  • trunk check clean on all changed files.

Ship Checklist

  • ship skill used
  • local review run
  • validation run

🤖 Generated with Claude Code


Note

Low Risk
CI and local tooling only; generator behavior for normal contracts:update is unchanged aside from optional CONTRACTS_OUT_DIR.

Overview
Adds pnpm contracts:check-drift so CI can catch when .treb/deployments.json has moved ahead of the committed @mento-protocol/contracts package without a regen.

The check discovers published namespaces from committed contracts.json, regenerates each namespace with the real gen-contracts-package.ts into a temp tree (via new CONTRACTS_OUT_DIR), and fails on additive drift—new contract keys or changed addresses (case-insensitive). It prints contracts:update remediation commands. gen-contracts-package.ts skips trunk fmt when writing to that temp dir.

The Foundry check job runs the drift step after forge build so ABIs come from existing out/ artifacts. Docs and Vitest cover the pure diff/namespace helpers.

Reviewed by Cursor Bugbot for commit e96810c. Configure here.

The @mento-protocol/contracts package is generated from
.treb/deployments.json one namespace at a time, so any published
namespace nobody remembers to regenerate silently drifts — its live
on-chain addresses never reach the package and nothing notices (issue
#77: ~88 addresses sat unpublished for two months).

Add `pnpm contracts:check-drift` (scripts/check-namespace-drift.ts): it
regenerates every already-published namespace into a temp copy using the
real generator and fails if the result adds or re-points any address vs
the committed contracts.json. Reusing the generator makes it
authoritative — it can't disagree with the generator's own filtering
rules. The published namespace set is derived from the committed
contracts.json, so ephemeral namespaces (virtual/throwaway/test/old RCs)
are excluded by construction and a new namespace is guarded the moment
it first lands in the package.

Drift is additive-only (treb ahead of package); the generator never
removes keys, so package-ahead-of-treb is out of scope.

- gen-contracts-package.ts: honor CONTRACTS_OUT_DIR to redirect all
  output to a temp dir and skip the formatter in that mode (the check
  compares parsed JSON, so formatting is irrelevant). Normal runs are
  unchanged.
- Wired into the existing `check` job in test.yml, reusing the out/
  artifacts already built there (no second forge build).
- Unit tests for the pure diff/namespace helpers; README documents the
  guard and local command.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fi9YQqbETy2YubqdeXxSq5
@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Claude finished @chapati23's task in 1m 50s —— View job


✅ Review complete — verdict: LGTM

  • Gather context (PR body, prior reviews, diff vs origin/main)
  • Verify the fix commit (74101a2) actually resolves the 3 prior P2 findings
  • Review full diff for any new issues
  • Check AGENTS.md/CLAUDE.md conventions (SHA-pinning, forge fmt, treb drift rules)
  • Post final review

This is a re-review triggered by the fix commit 74101a2, which addresses the three P2 findings raised against the initial commit e96810c. I re-read scripts/check-namespace-drift.ts in full against the fix diff.

Cross-cutting checks

  • SHA-pinning: .github/workflows/test.yml only adds a run: step (pnpm contracts:check-drift) — no new uses: actions introduced, so no supply-chain concern here.
  • CI ordering: the new check-drift step correctly runs after forge build/forge test in the check job, so it reuses out/ as documented.
  • Generated-output hygiene: contracts.json / typed exports aren't hand-edited; the check only reads the committed package and regenerates into a temp dir via the real generator — no drift risk from the tooling itself.
  • CONTRACTS_OUT_DIR: cleanly scoped in gen-contracts-package.ts (2 call sites, both gated on the env var), normal contracts:update behavior is unchanged.

Roll-up of all findings

  1. [P2] Temp directory leaks on drift-detected/regen-failure runs (process.exit(1) inside try skipped finally { rmSync }) — FIXED in 74101a2. regenerateInto now throws instead of exiting, and the drift path sets process.exitCode = 1 instead of calling process.exit() inside the try, so the finally cleanup always runs. A new top-level try/catch around main() preserves the clean exit-1 behavior when regenerateInto throws. Verified by reading the control flow: every exit path now either returns/throws before mkdtempSync, or unwinds through the finally.
  2. [P2] Possible silent hang if out/ is missing (child's interactive forge build prompt hangs under spawnSync's EOF stdin) — FIXED in 74101a2. main() now checks existsSync(outDir) && readdirSync(outDir).length > 0 before any subprocess is spawned (and before the temp dir is even created), failing fast with Run \forge build` first...`. This eliminates the hang at its root rather than papering over the symptom.
  3. [P2] (Codex) False-green risk from the same interactive-prompt path — same root cause as Migrate away from mocked USDC and USDT #2, and the preflight out/ check addresses it the same way: regeneration can no longer proceed against a missing/empty out/, so there's no path left where the child could exit 0 without actually reading real ABIs.

No new issues found in the fix commit itself. The unit tests (check-namespace-drift.test.ts) cover the pure diffContractsJson/publishedNamespaces helpers but not the new preflight/exit-code branches — that's reasonable since those are process-exit side effects, not pure logic, and the author confirmed manual verification (simulated drift → exit 1, zero leaked contracts-drift-* dirs).

Comment thread scripts/check-namespace-drift.ts Outdated
Comment thread scripts/check-namespace-drift.ts
Comment thread scripts/check-namespace-drift.ts Outdated
Addresses two P2 findings from Claude's review of PR #82:

- process.exit(1) inside the try block skipped the finally { rmSync }
  cleanup (Node does not unwind finally on process.exit), leaking the
  temp regen dir on every drift-detected or regen-failure run. Now the
  drift path sets process.exitCode and regenerateInto throws, so the
  finally always runs; a top-level catch keeps the clean exit-1 + message.
- Running the check locally before `forge build` would feed the child
  generator's interactive "Run forge build now?" prompt an EOF stdin
  under spawnSync, hanging forever. Now main() fails fast with a clear
  message when out/ is missing or empty.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fi9YQqbETy2YubqdeXxSq5

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e96810cb7a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-namespace-drift.ts
@chapati23

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews. Both findings addressed in 74101a2:

  1. Temp-dir leak (process.exit skipping finally) — the drift path now sets process.exitCode = 1 and regenerateInto throws, so finally { rmSync } always runs; a top-level catch preserves the clean exit-1 + message. Verified locally: a simulated-drift run exits 1 and leaves zero contracts-drift-* dirs.
  2. out/-missing hang / false-greenmain() now preflights out/ and fails fast with Run \forge build` first` before seeding the temp dir or spawning the generator, closing both the readline-hang and the no-op-regeneration-reports-green paths.

30 unit tests pass; the drift check is green on the current tree and correctly red on simulated drift.

(Note: the earlier Trunk Check failure on the first push was osv-scanner flagging pre-existing dev-only advisories in the root pnpm-lock.yaml via vitest — this PR changes no dependencies, and both Trunk Check runs on 74101a2 pass.)

@chapati23

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74101a2420

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-namespace-drift.ts
@chapati23

Copy link
Copy Markdown
Contributor Author

Re: Codex's ABI-drift suggestion on 74101a2 — valid but intentionally out of scope for #77 (which targets address drift). ABI-on-proxy-upgrade detection needs generator changes (per-namespace ABI output doesn't compose into a full-package diff), so it's tracked as a follow-up in #83. Replied inline and resolved the thread. This PR delivers address-drift detection, verified green in CI.

@chapati23
chapati23 merged commit 7b02f6a into main Jul 17, 2026
34 checks passed
@chapati23
chapati23 deleted the feat/namespace-drift-check branch July 17, 2026 17:31
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.

Namespace drift: @mento-protocol/contracts silently misses unpublished on-chain deployments

1 participant