Add signer custody readiness evidence gate#189
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
Author
|
@coderabbitai review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughThe PR adds a no-secret, machine-checkable signer custody readiness evidence system: a JSON schema, local template, comprehensive validator, unit tests, and integration into release manifest generation, checksums, CI/Make gates, and cross-referenced documentation that collectively define production signer readiness requirements without leaking sensitive credentials or signing data. ChangesSigner Custody Readiness Evidence System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
✅ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/check_incident_response.py (1)
84-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpand the signer-custody link coverage here.
This gate only enforces the guide and template links, so the incident-response runbook can drop the signer-custody schema or retained-artifact reference and still pass. That leaves this validator weaker than the other release/audit checks and misses the stated evidence-gate objective.
🤖 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 `@scripts/check_incident_response.py` around lines 84 - 105, The REQUIRED_LINK_TARGETS list in scripts/check_incident_response.py currently only includes the signer-custody guide and template but omits the signer-custody schema and any retained-artifact evidence paths, allowing the runbook to pass without those required artifacts; update the REQUIRED_LINK_TARGETS constant to also include the signer-custody schema file (e.g., the schema JSON/YAML used to validate custody evidence) and any retained-artifact paths referenced by your release process (e.g., retained-artifact manifest or evidence file names for signer custody), ensuring the incident-response validator enforces both the guide/template and the schema/retained-artifact artifacts (modify the REQUIRED_LINK_TARGETS list shown in the diff to add the missing signer-custody schema and retained-artifact entries).
🧹 Nitpick comments (4)
release-artifacts/schema/signer-custody-readiness.schema.json (1)
293-296: ⚡ Quick winConstrain
review.reviewed_atto a machine-parseable timestamp format.
reviewed_atcurrently accepts arbitrary text, which weakens the machine-checkable review trail. Consider enforcing RFC 3339/ISO-8601 (date-time) in schema and checker.Suggested schema tightening
"reviewed_at": { "type": "string", - "minLength": 1 + "format": "date-time", + "minLength": 20 }🤖 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 `@release-artifacts/schema/signer-custody-readiness.schema.json` around lines 293 - 296, The schema currently allows arbitrary text for review.reviewed_at; change its JSON Schema to require an RFC3339/ISO‑8601 timestamp by adding the "format": "date-time" constraint to the reviewed_at property in signer-custody-readiness.schema.json and update any runtime validation/checker that inspects review.reviewed_at to validate against the date-time format (or parse with an ISO-8601/RFC3339 parser) so invalid freeform strings are rejected.scripts/test_signer_custody_readiness.py (2)
117-117: ⚡ Quick winUse a real non-local chain ID for
productionfixtures.Line 117 maps every non-
testnetenvironment to31337, sovalid_evidence(root, environment="production")still builds a semantically local fixture. That makestest_rejects_production_without_required_drillsless focused, because any future environment-specific chain checks could fail this case before it reaches the production-drill branch.🤖 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 `@scripts/test_signer_custody_readiness.py` at line 117, The test fixture maps any non-"testnet" environment to the local chain id 31337; update the chain id selection used in the fixture (where "chain_id": 11155111 if environment == "testnet" else 31337) so that production environments get a real non-local chain id (e.g., 1 for mainnet or another canonical production chain id) instead of 31337, ensuring calls like valid_evidence(root, environment="production") produce a true production-like chain id and exercise the production-only branch in test_rejects_production_without_required_drills.
215-218: ⚡ Quick winPin the template path in the CLI smoke test.
This test currently inherits whatever default evidence list
check_signer_custody_readiness.pyexposes through its CLI. Passing the committed template path explicitly would keep the assertion scoped to the template contract and avoid unrelated default-CLI changes breaking this test.🤖 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 `@scripts/test_signer_custody_readiness.py` around lines 215 - 218, Update the test_accepts_committed_template to call checker.main with the committed template path explicitly instead of relying on the CLI default: add the evidence-list CLI argument when invoking checker.main (e.g. checker.main(["--repo-root", str(REPO_ROOT), "--evidence-list", str(<committed-template-path-constant>)])), using the existing committed-template test fixture/constant in this test file so the assertion is pinned to the committed template; keep the redirect_stdout/redirect_stderr wrapper unchanged.docs/signer-custody-readiness.md (1)
62-66: ⚡ Quick winKeep the ERC-1271 status path machine-checkable.
Requiring notes outside the template weakens the no-secret envelope, because that rationale is no longer validated by the schema/checker. If the status needs extra context, add a structured field to the template instead.
🤖 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 `@docs/signer-custody-readiness.md` around lines 62 - 66, The guidance asks to keep ERC-1271 status machine-checkable instead of requiring freeform notes outside the template; add a structured context field in the template (e.g., erc1271_status with enum values supported|not_applicable|pending|blocked|unsupported and an accompanying erc1271_status_detail or erc1271_evidence object for structured rationale) and update the schema/checker to validate both the enum and the optional structured detail (rather than external notes); ensure the template, README text (the paragraph describing ERC-1271), and any validation logic reference the exact field names (erc1271_status and erc1271_status_detail/erc1271_evidence) so the status plus its context remain machine-checked.
🤖 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 `@docs/public-beta-evidence.md`:
- Around line 53-59: The paragraph currently misreferences the evidence family
for "non-local drop authorization signing"; update the cross-reference in the
signer-custody section so it points to the signer custody readiness evidence
(the schema file `release-artifacts/schema/signer-custody-readiness.schema.json`
and the template
`release-artifacts/signer-custody-readiness/signer-custody-readiness-template.json`)
instead of the incorrect family; ensure the text clearly states that signer
custody readiness must pass `python scripts/check_signer_custody_readiness.py`
before any public-beta or production row relies on non-local drop authorization
signing.
In `@ops/ROADMAP.md`:
- Around line 89-95: Update the "CI run" table row that currently contains the
phrase "Queue Item 97 branch CI is pending until PR open" so it is no longer
stale: locate the "CI run" row in the table (the row starting with "CI run" in
the roadmap content) and replace that clause with the actual Queue Item 97 CI
result (e.g., pass/fail and run ID/time) or a time-bounded note such as "pending
as of <UTC timestamp> awaiting final branch CI", ensuring the new text clearly
and unambiguously reflects the current status for Queue Item 97.
In `@scripts/check_signer_custody_readiness.py`:
- Around line 232-236: The require_string function currently treats
whitespace-only strings as valid; update require_string to reject strings that
are empty or contain only whitespace by checking value.strip() and raising
SignerCustodyReadinessError when value is not an instance of str or
value.strip() == "" so callers (e.g., validations relying on require_string)
don't accept placeholder/blank input.
- Around line 587-601: When review_status == "reviewed", strengthen validation
beyond reviewer and approval_status by ensuring review.owner,
review.approval_reference, and review.reviewed_at are not placeholders and are
well-formed: inside the reviewed branch (where reviewer and approval_status are
checked) add checks that require_string(review.get("owner")/ "review.owner") is
not a placeholder like "TBD" (e.g., owner.strip().upper() != "TBD"),
require_string(review.get("approval_reference"), "review.approval_reference") is
non-placeholder and non-empty, and validate review.reviewed_at is a parseable
timestamp (e.g., attempt to parse ISO8601 or use an existing datetime validator)
and raise SignerCustodyReadinessError with a clear message if any of these
checks fail; reference the existing variables reviewer, approval_status,
review_status and the SignerCustodyReadinessError class when implementing these
checks.
---
Outside diff comments:
In `@scripts/check_incident_response.py`:
- Around line 84-105: The REQUIRED_LINK_TARGETS list in
scripts/check_incident_response.py currently only includes the signer-custody
guide and template but omits the signer-custody schema and any retained-artifact
evidence paths, allowing the runbook to pass without those required artifacts;
update the REQUIRED_LINK_TARGETS constant to also include the signer-custody
schema file (e.g., the schema JSON/YAML used to validate custody evidence) and
any retained-artifact paths referenced by your release process (e.g.,
retained-artifact manifest or evidence file names for signer custody), ensuring
the incident-response validator enforces both the guide/template and the
schema/retained-artifact artifacts (modify the REQUIRED_LINK_TARGETS list shown
in the diff to add the missing signer-custody schema and retained-artifact
entries).
---
Nitpick comments:
In `@docs/signer-custody-readiness.md`:
- Around line 62-66: The guidance asks to keep ERC-1271 status machine-checkable
instead of requiring freeform notes outside the template; add a structured
context field in the template (e.g., erc1271_status with enum values
supported|not_applicable|pending|blocked|unsupported and an accompanying
erc1271_status_detail or erc1271_evidence object for structured rationale) and
update the schema/checker to validate both the enum and the optional structured
detail (rather than external notes); ensure the template, README text (the
paragraph describing ERC-1271), and any validation logic reference the exact
field names (erc1271_status and erc1271_status_detail/erc1271_evidence) so the
status plus its context remain machine-checked.
In `@release-artifacts/schema/signer-custody-readiness.schema.json`:
- Around line 293-296: The schema currently allows arbitrary text for
review.reviewed_at; change its JSON Schema to require an RFC3339/ISO‑8601
timestamp by adding the "format": "date-time" constraint to the reviewed_at
property in signer-custody-readiness.schema.json and update any runtime
validation/checker that inspects review.reviewed_at to validate against the
date-time format (or parse with an ISO-8601/RFC3339 parser) so invalid freeform
strings are rejected.
In `@scripts/test_signer_custody_readiness.py`:
- Line 117: The test fixture maps any non-"testnet" environment to the local
chain id 31337; update the chain id selection used in the fixture (where
"chain_id": 11155111 if environment == "testnet" else 31337) so that production
environments get a real non-local chain id (e.g., 1 for mainnet or another
canonical production chain id) instead of 31337, ensuring calls like
valid_evidence(root, environment="production") produce a true production-like
chain id and exercise the production-only branch in
test_rejects_production_without_required_drills.
- Around line 215-218: Update the test_accepts_committed_template to call
checker.main with the committed template path explicitly instead of relying on
the CLI default: add the evidence-list CLI argument when invoking checker.main
(e.g. checker.main(["--repo-root", str(REPO_ROOT), "--evidence-list",
str(<committed-template-path-constant>)])), using the existing
committed-template test fixture/constant in this test file so the assertion is
pinned to the committed template; keep the redirect_stdout/redirect_stderr
wrapper 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cb01417-72ce-4775-8509-d92808943e9e
📒 Files selected for processing (34)
.github/workflows/ci.ymlCHANGELOG.mdMakefiledocs/audit-package.mddocs/drop-authorization-signing.mddocs/incident-response.mddocs/known-blockers.mddocs/public-beta-evidence.mddocs/release-policy.mddocs/release-readiness.mddocs/signer-custody-readiness.mddocs/status.mddocs/tooling.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdrelease-artifacts/README.mdrelease-artifacts/latest/SHA256SUMSrelease-artifacts/latest/release-checksums.jsonrelease-artifacts/latest/release-manifest.jsonrelease-artifacts/schema/signer-custody-readiness.schema.jsonrelease-artifacts/signer-custody-readiness/signer-custody-readiness-retained-artifact.txtrelease-artifacts/signer-custody-readiness/signer-custody-readiness-template.jsonscripts/check.ps1scripts/check.shscripts/check_audit_package.pyscripts/check_incident_response.pyscripts/check_release_readiness.pyscripts/check_signer_custody_readiness.pyscripts/generate_release_checksums.pyscripts/generate_release_manifest.pyscripts/test_release_checksums.pyscripts/test_release_manifest.pyscripts/test_release_readiness.pyscripts/test_signer_custody_readiness.py
This was referenced Jun 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
make check, shell/PowerShell check wrappers, release manifest generation, release checksum coverage, release-readiness/audit/incident docs, tooling docs, roadmap, and autonomous run state.Closes #187.
Validation
python -m py_compile scripts\check_signer_custody_readiness.py scripts\test_signer_custody_readiness.py scripts\generate_release_manifest.py scripts\test_release_manifest.py scripts\generate_release_checksums.py scripts\test_release_checksums.py scripts\check_release_readiness.py scripts\test_release_readiness.py scripts\check_audit_package.py scripts\check_incident_response.pypython scripts\test_signer_custody_readiness.pypython scripts\check_signer_custody_readiness.pypython scripts\test_release_manifest.pypython scripts\generate_release_manifest.py --checkpython scripts\test_release_checksums.pypython scripts\generate_release_checksums.py --checkpython scripts\test_release_readiness.pypython scripts\check_release_readiness.pypython scripts\test_audit_package.pypython scripts\check_audit_package.pypython scripts\test_incident_response.pypython scripts\check_incident_response.pypython scripts\test_public_beta_evidence.pypython scripts\check_public_beta_evidence.pypython scripts\test_drop_authorization_signing_evidence.pypython scripts\check_drop_authorization_signing_evidence.pypython scripts\test_changelog_check.pypython scripts\check_changelog.pygit diff --checkrg -n "^#|^##|^###" ops\AUTONOMOUS_RUN.md ops\ROADMAP.mdmake checkpowershell -ExecutionPolicy Bypass -File scripts\check.ps1Summary by CodeRabbit
New Features
Documentation
Chores
Tests