feat(safe): record proposal provenance (commit, branch, proposer, PR) (EXSC-692) - #2129
feat(safe): record proposal provenance (commit, branch, proposer, PR) (EXSC-692)#21290xDEnYO wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughChangesSafe proposal provenance
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
script/deploy/safe/safe-utils.ts (1)
1339-1373: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOverride path returns the caller's object by reference.
Unlike
captureGitProvenance, which clones,buildProposalProvenancehands backoptions.overrideitself, so the stored document aliases the caller's block (a test fixture reused across cases can be mutated downstream). A shallow copy would keep the seam side-effect free.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/safe-utils.ts` around lines 1339 - 1373, Update the override branch in buildProposalProvenance to return a shallow copy of options.override rather than the caller’s object directly, while preserving the existing reason-merging behavior and avoiding mutation of the supplied override.script/deploy/shared/git-provenance.ts (1)
509-551: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMemo is keyed on nothing, so
resolvePrUrland injected context only apply to the first call.A later
captureGitProvenance({ resolvePrUrl: false })still returns the cachedprUrl(and vice versa: a first PR-less capture permanently hides it), and on a cache hitoptions.errorsis never repopulated. Harmless for the single production caller, but worth documenting on the export so a future caller doesn't rely on per-call options.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/shared/git-provenance.ts` around lines 509 - 551, Document on the exported captureGitProvenance function that its cached result is not keyed by per-call options, so resolvePrUrl, injected context, and options.errors only affect the first invocation; clarify that subsequent calls return the existing cached provenance unchanged.script/deploy/shared/git-provenance.test.ts (1)
77-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring claims longest-prefix matching; the implementation takes the first insertion-order match.
Object.keys(handlers).find(...)returns the first registered prefix that matches, so a broad key (e.g.'git ') added before a specific one would shadow it. Either sort candidates by descending length or fix the comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/shared/git-provenance.test.ts` around lines 77 - 95, Update stubRunner’s handler selection to honor its documented longest-prefix behavior by choosing the matching key with the greatest length, rather than the first Object.keys(handlers) match. Preserve the existing command logging and unstubbed-command failure behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/deploy/safe/provenance-display.ts`:
- Around line 98-105: The provenance display must sanitize proposer-supplied
text before applying color codes: in script/deploy/safe/provenance-display.ts
lines 98-105, strip C0/C1 control characters from reason and the other stored
strings rendered by the display. In script/deploy/safe/safe-utils.ts lines
1321-1327, update normalizeProposalReason to remove control characters as well
as collapsing whitespace, ensuring stored values cannot contain escape
sequences.
---
Nitpick comments:
In `@script/deploy/safe/safe-utils.ts`:
- Around line 1339-1373: Update the override branch in buildProposalProvenance
to return a shallow copy of options.override rather than the caller’s object
directly, while preserving the existing reason-merging behavior and avoiding
mutation of the supplied override.
In `@script/deploy/shared/git-provenance.test.ts`:
- Around line 77-95: Update stubRunner’s handler selection to honor its
documented longest-prefix behavior by choosing the matching key with the
greatest length, rather than the first Object.keys(handlers) match. Preserve the
existing command logging and unstubbed-command failure behavior.
In `@script/deploy/shared/git-provenance.ts`:
- Around line 509-551: Document on the exported captureGitProvenance function
that its cached result is not keyed by per-call options, so resolvePrUrl,
injected context, and options.errors only affect the first invocation; clarify
that subsequent calls return the existing cached provenance unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85f4b525-19f0-4b06-aa6b-ec2002d86c51
📒 Files selected for processing (8)
.env.examplescript/deploy/safe/confirm-safe-tx.tsscript/deploy/safe/provenance-display.test.tsscript/deploy/safe/provenance-display.tsscript/deploy/safe/safe-utils.test.tsscript/deploy/safe/safe-utils.tsscript/deploy/shared/git-provenance.test.tsscript/deploy/shared/git-provenance.ts
| lines.push( | ||
| detailLine( | ||
| 'Reason', | ||
| provenance.reason | ||
| ? color(GREEN, provenance.reason) | ||
| : color(YELLOW, '— none given —') | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Provenance text is never stripped of control characters, so proposer-supplied text can spoof the signer prompt. Neither normalization at capture time nor rendering at confirm time removes \u001b/\r, and the rendered block is read by a different human before signing.
script/deploy/safe/provenance-display.ts#L98-L105: strip C0/C1 control characters fromreason(and the other stored strings) before wrapping them in colour codes.script/deploy/safe/safe-utils.ts#L1321-L1327: extendnormalizeProposalReasonto drop control characters in addition to collapsing whitespace, so nothing stored can carry escape sequences.
📍 Affects 2 files
script/deploy/safe/provenance-display.ts#L98-L105(this comment)script/deploy/safe/safe-utils.ts#L1321-L1327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/deploy/safe/provenance-display.ts` around lines 98 - 105, The
provenance display must sanitize proposer-supplied text before applying color
codes: in script/deploy/safe/provenance-display.ts lines 98-105, strip C0/C1
control characters from reason and the other stored strings rendered by the
display. In script/deploy/safe/safe-utils.ts lines 1321-1327, update
normalizeProposalReason to remove control characters as well as collapsing
whitespace, ensuring stored values cannot contain escape sequences.
1 participant
Which Linear task belongs to this PR?
One PR covering three tickets — they are one change, split only by concern:
script/deploy/shared/git-provenance.ts)confirm-safe-txWhy did I implement it this way?
The problem. A pending Safe proposal today says what it does but not where it came from. A signer asking "what is this, and can I trust the code behind it?" has to go and ask. Every proposal now records who created it, from which commit and branch, whether that commit is fetchable, whether the working tree was dirty, and optionally why.
Captured at the single storage funnel, not at call sites. All ten call sites — including the five bespoke task scripts and the Tron route — pass through
storeTransactionInMongoDB, so capture lives there and every caller inherits it with zero changes. The new parameter is appended last and optional, so nothing else had to be touched.Fail-soft, because this sits on the deploy path. A Mongo write failure here already aborts a deployment, so a git helper that throws would be able to take a production deploy down. Every probe swallows its own failure, returns an
'unknown'sentinel and records a one-line reason incaptureErrors; a whole-capture backstop catches anything unexpected. There are three near-duplicate git snippets in the repo today with inconsistent behaviour (one of them throws) — this module is the single fail-soft policy, but the existing call sites are deliberately not refactored here.Injectable override seam, in the same commit.
safe-utils.test.tscallsstoreTransactionInMongoDBdirectly. Without a seam that suite would start shelling out to realgitand asserting against whatever checkout CI happens to have.provenanceOptions.overrideis what keeps it deterministic — a requirement of this change, not polish.Optional field, for backward compatibility.
provenance?is optional and old rows simply have none. The signer view renders one explicit "not recorded" line for them rather than a silent gap — a gap reads as "clean and authored by nobody", which is the one impression this block must never give.Why the three parts ship together. The capture (EXSC-692) does not compile without the helper (EXSC-691), and the helper alone is dead code. The display (EXSC-693) could have been split out, but it is the only thing that makes the captured data visible, so reviewing it separately would mean reviewing a feature nobody can see. I have left "as small as possible" unticked rather than claim otherwise.
Design details worth a reviewer's attention
git status --porcelainminus the paths the deploy pipeline rewrites during its own run (deployments/**,script/deploy/_targetState.json).config/whitelist.jsonandconfig/networks.jsonare deliberately not excluded — a dirty whitelist at proposal time is exactly what a reviewer wants to know about. Capped at 20 entries with a truncation flag.ghlookup, then 0 ms for every subsequent proposal in the run.ghenvironment, skipped formain/detached/unknown branches, skippable via an option, and every failure (missinggh, unauthenticated, no PR, timeout) is swallowed without recording a capture error — otherwise every proposer withoutghwould see a spurious "capture incomplete" marker on every proposal.commitOnRemoteis honest rather than clever. It reads local remote-tracking refs (git branch --remotes --contains), so a stale checkout can reportfalsefor a commit that is in fact pushed. The CLI saysNOT PUSHED (per local refs)instead of pretending, and no network fetch is added to the hot path.safeTx. The Tron flow fabricates that object through a cast and its shape is not trustworthy, so capture reads ambient git state only. Covered by a test that stores a Tron-shaped document.process.envstubs only. This does not close an existing CI gap.SAFE_PROPOSAL_REASONis read from the environment — no CLI plumbing in this PR, that is EXSC-694 — and is optional, with no warning spam.SAFE_PROPOSAL_ACTOR=botis the opt-in a future unattended job sets. Both are in.env.example.Provenance is context, not a security control
Worth stating plainly, because the field names invite the opposite reading: this data is self-reported by the proposing machine. It makes honest mistakes visible — an unpushed commit, a dirty whitelist, a proposal nobody can explain — and it gives later checks something concrete to verify against. It is not a defence against a proposer who is deliberately lying, and a signer should not read a green "clean / pushed" line as authentication of anything.
Governance impact (rule 105)
None. This is additive metadata on a MongoDB document. No change to Safe thresholds, owner sets, timelock delays, roles, proposal authorization, signing, or execution. No Solidity is touched and no on-chain behaviour changes; the only user-visible difference is a few extra informational lines in the
confirm-safe-txprompt. Nothing was added toDEPLOYMENT_QUERY_EQ_KEYSandmongo-log-utils.tsis untouched, so deployment-record identity and upsert behaviour are unchanged too.Verification
bun test script/— 633 pass, 0 fail (35 files).script/deploy/safe/+script/deploy/shared/alone: 458 pass, 0 fail. 70 of those tests are new: 44 ingit-provenance.test.ts, 13 inprovenance-display.test.ts, 13 added tosafe-utils.test.ts.bunx eslintandbunx tsc-files --noEmiton all seven changed/added files: exit 0. Also typechecked all ten call sites of the changed signature: exit 0.git statusoutput ate the leading status column and turned.env.exampleintoenv.example. Fixed, with a regression test for an unstaged first entry.Follow-ups (not in this PR)
--reasonCLI flag plumbed throughpropose-to-safe.tsand the bash chain.gitBranch/dirtyTreeScoped/actoronIDeploymentRecord), reusing this module. Note for whoever picks it up: those fields must not go intoDEPLOYMENT_QUERY_EQ_KEYS, or an upsert becomes branch-sensitive and starts duplicating records.provenancethroughIProposalSummaryforlist-pending-proposals --json, and hashing governance config into the block, are both deliberately deferred.Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)