Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR reorganizes and expands the repository’s operator/contributor documentation by introducing an in-repo docs/ hub (with concepts, local dev, configuration reference, troubleshooting, dashboard docs, and runbooks) and updating the root README and contributor guidance to point to the new structure.
Changes:
- Replace the root README’s single “Specification” pointer with a richer Documentation + Contributing index, and update EVM env var docs.
- Add a comprehensive
docs/documentation hub including troubleshooting, configuration reference, dashboard docs, and operational runbooks (secrets + disaster recovery). - Refresh contributor/agent guidance to reflect the multi-surface client layering (per-account, dashboard, EVM) and associated validation paths.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds a docs/contributing index; updates EVM env var documentation and fixes a bad compose link. |
| docs/TROUBLESHOOTING.md | New troubleshooting guide organized by symptom + error code reference. |
| docs/runbooks/secrets.md | New secrets/key-management runbook (bootstrap/rotation/compromise response). |
| docs/runbooks/restore.md | New disaster recovery runbook (RDS restore, ACK key loss, state rebuild, operator allowlist restore). |
| docs/README.md | New documentation hub / index to guide readers by role and intent. |
| docs/QUICKSTART.md | New 60-second local “compose up + verify” quickstart. |
| docs/MULTISIG_SDK.md | Adds onboarding pointers and updates breaking-changes reference. |
| docs/LOCAL_DEV.md | New local development guide covering launch paths, features, and test commands. |
| docs/DASHBOARD.md | New operator dashboard trust model + session/auth + allowlist/permissions docs. |
| docs/CONFIGURATION.md | New consolidated runtime/deploy env var reference. |
| docs/CONCEPTS.md | New conceptual overview (trust model, lifecycle, recovery, provider rotation). |
| docs/architecture/services.md | New service decomposition / module map and storage-mode explanations. |
| docs/architecture/infra.md | New AWS deployment topology and Terraform resource inventory documentation. |
| CONTRIBUTING.md | New contributor workflow guide (cross-layer rules, testing, docs expectations). |
| AGENTS.md | Updates the agent-oriented repo map and contract-change workflow to include operator/EVM clients and smoke harnesses. |
Comments suppressed due to low confidence (2)
docs/runbooks/secrets.md:52
- These verification commands use
<stack>-prod/server/...secret IDs, but the deploy script/Terraform currently use fixed names (guardian-prod/server/ack-falcon-secret-keyandguardian-prod/server/ack-ecdsa-secret-key). Using<stack>-prodhere will cause operators to look up the wrong secrets.
```bash
aws secretsmanager describe-secret --secret-id <stack>-prod/server/ack-falcon-secret-key
aws secretsmanager describe-secret --secret-id <stack>-prod/server/ack-ecdsa-secret-key
**docs/runbooks/secrets.md:81**
* The rotation example still uses `<stack>-prod/server/ack-*-secret-key`, but ACK secrets are currently fixed to `guardian-prod/server/ack-{falcon,ecdsa}-secret-key` in the deploy script/Terraform and the server’s Secrets Manager provider. Please update the `update-secret` commands to use the fixed secret IDs so operators don’t rotate the wrong secrets.
aws secretsmanager update-secret
--secret-id -prod/server/ack-falcon-secret-key
--secret-string "$FALCON"
aws secretsmanager update-secret
--secret-id -prod/server/ack-ecdsa-secret-key
--secret-string "$ECDSA"
</details>
---
💡 <a href="/OpenZeppelin/guardian/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
| 2. **Filesystem paths missing.** Filesystem builds require | ||
| `GUARDIAN_STORAGE_PATH` and `GUARDIAN_METADATA_PATH`. Default fallbacks | ||
| if unset: `/var/guardian/storage`, `/var/guardian/metadata`, | ||
| `/var/guardian/keystore`. The process must be able to create files in | ||
| these directories. |
| ACK secret IDs (`GUARDIAN_ACK_FALCON_SECRET_ID`, | ||
| `GUARDIAN_ACK_ECDSA_SECRET_ID`) are passed in by Terraform/ECS env in | ||
| prod; defaults derive from `${stack_name}-prod/server/ack-*-secret-key`. | ||
| See [Secrets runbook](./runbooks/secrets.md#ack-signing-keys). |
| - [`ack/secrets_manager.rs`](../../crates/server/src/ack/secrets_manager.rs) | ||
| resolves Secrets Manager IDs (with whitespace-tolerant trimming) and pulls | ||
| secret payloads into the filesystem keystore at startup. In `dev` keys are | ||
| auto-generated; in `prod` they are loaded from the Secrets Manager entries | ||
| named in `GUARDIAN_ACK_FALCON_SECRET_ID` / `GUARDIAN_ACK_ECDSA_SECRET_ID` | ||
| (defaulting to `<stack>-prod/server/ack-*-secret-key`). |
| Naming defaults derive from `stack_name`; the trailing `-prod` on ACK secrets | ||
| is historical. Override via `TF_VAR_ack_falcon_secret_name` / | ||
| `TF_VAR_ack_ecdsa_secret_name` if needed. |
| aws secretsmanager put-secret-value \ | ||
| --secret-id <stack>-prod/server/ack-falcon-secret-key \ | ||
| --secret-string "$(jq -r '.falcon_secret_key' bootstrap-output.json)" | ||
|
|
||
| aws secretsmanager put-secret-value \ | ||
| --secret-id <stack>-prod/server/ack-ecdsa-secret-key \ | ||
| --secret-string "$(jq -r '.ecdsa_secret_key' bootstrap-output.json)" | ||
| ``` |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/CONCEPTS.md (1)
234-238: ⚡ Quick winAdd language identifier to code fence.
The security-group chain diagram should specify a language identifier for proper rendering, even if it's plain text.
Proposed fix
Security-group chain: -``` +```text internet → alb SG (80/443) → server SG (3000, 50051) server SG ─┬─→ postgres SG (5432) (dev path) └─→ rds_proxy SG (5432) → postgres SG (5432) (prod path)</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/CONCEPTS.mdaround lines 234 - 238, The code block containing the
security-group chain diagram in docs/CONCEPTS.md is missing a language
identifier, preventing proper rendering; update the fenced code block that
encloses the lines starting with "internet → alb SG (80/443) → server SG (3000,
50051)" and the following lines (the security-group chain diagram) to use a
language tag (for example ```text) on the opening fence so the diagram renders
correctly.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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/DASHBOARD.md:
- Around line 182-208: The dashboard runbook link uses a non-existent anchor
"#adding-or-removing-an-operator"; open the runbooks secrets doc, locate the
exact heading slug for the section about adding/removing operators (the heading
text for the operator secrets section), and update the anchor in DASHBOARD.md
(the link that currently reads
./runbooks/secrets.md#adding-or-removing-an-operator) to that exact slug; while
there, verify the surrounding text that references guardian_operator_public_keys
remains accurate and update the link text if needed to match the target heading.In
@docs/TROUBLESHOOTING.md:
- Around line 134-136: The code block showing the HTTP GET example is missing a
language specifier; update the block around the line containing "GET
/delta/since?account_id=...&nonce=" to use a bash language
fence by replacing the opening triple-backticks with "bash" (and keep the closing "") so the example renders with shell/HTTP highlighting.In
@README.md:
- Line 75: The quickstart example still uses the old per-chain env var
GUARDIAN_EVM_ENTRYPOINTS; update the snippet in
speckit/features/001-evm-proposal-support/quickstart.md to use the new shared
variable GUARDIAN_EVM_ENTRYPOINT_ADDRESS (e.g.,
GUARDIAN_EVM_ENTRYPOINT_ADDRESS=0x...), so it matches the server config read in
crates/server/src/evm/config.rs and the README.md usage; change the env var name
in the example command and any related explanatory text to the singular
GUARDIAN_EVM_ENTRYPOINT_ADDRESS and keep the same address format.
Nitpick comments:
In@docs/CONCEPTS.md:
- Around line 234-238: The code block containing the security-group chain
diagram in docs/CONCEPTS.md is missing a language identifier, preventing proper
rendering; update the fenced code block that encloses the lines starting with
"internet → alb SG (80/443) → server SG (3000, 50051)" and the following lines
(the security-group chain diagram) to use a language tag (for example ```text)
on the opening fence so the diagram renders correctly.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `7f410069-6db2-49bb-9d4c-7a6d0b28a501` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 769d875431884c4cc56bfa8f4cf4c67025578e70 and 415a94387e2dbf0c495e4d3bebda43bf56fa6324. </details> <details> <summary>📒 Files selected for processing (15)</summary> * `AGENTS.md` * `CONTRIBUTING.md` * `README.md` * `docs/CONCEPTS.md` * `docs/CONFIGURATION.md` * `docs/DASHBOARD.md` * `docs/LOCAL_DEV.md` * `docs/MULTISIG_SDK.md` * `docs/QUICKSTART.md` * `docs/README.md` * `docs/TROUBLESHOOTING.md` * `docs/architecture/infra.md` * `docs/architecture/services.md` * `docs/runbooks/restore.md` * `docs/runbooks/secrets.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| ## Enrolling an operator | ||
|
|
||
| End-to-end procedure for adding operator Alice to a deployed Guardian. | ||
|
|
||
| 1. **Alice generates a Falcon keypair** on a trusted device (the same | ||
| keypair format the multisig SDK and the smoke example use; the | ||
| `examples/operator-smoke-web` README has a UI for this). | ||
| 2. Alice gives the public key (hex `0x…`) to the deploying operator. | ||
| 3. **Deployer updates the allowlist**: | ||
| - Terraform-managed: append to `guardian_operator_public_keys` and | ||
| redeploy (see [Secrets runbook](./runbooks/secrets.md#adding-or-removing-an-operator)). | ||
| - Externally-managed: `aws secretsmanager update-secret` with the new | ||
| payload — no ECS restart required. | ||
| 4. **Alice logs in** — challenge → sign → session. The change takes | ||
| effect on her next request; the server refreshes the allowlist on | ||
| every challenge issuance and every authenticated `/dashboard/*` call | ||
| ([`dashboard/state.rs:103-108`](../crates/server/src/dashboard/state.rs#L103), | ||
| [`dashboard/state.rs:284-324`](../crates/server/src/dashboard/state.rs#L284)). | ||
|
|
||
| ### Removing or revoking an operator | ||
|
|
||
| Same shape, no restart: | ||
| 1. Update the secret payload to drop or change Alice's entry. | ||
| 2. Effect is immediate — the next challenge or authenticated request | ||
| from any task reloads the allowlist and rejects the removed key. | ||
| Currently-active sessions for the revoked operator are rejected at | ||
| their next authenticated call (the per-request reload catches them). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify state.rs line references and secrets runbook
# Check state.rs exists and has enough lines
state_file="crates/server/src/dashboard/state.rs"
if [ ! -f "$state_file" ]; then
echo "✗ $state_file does not exist"
exit 1
fi
line_count=$(wc -l < "$state_file")
echo "✓ $state_file exists ($line_count lines)"
max_line=324
if [ "$line_count" -lt "$max_line" ]; then
echo "⚠ File has only $line_count lines, but line $max_line is referenced"
exit 1
fi
echo "✓ Line references (103-108, 284-324) are within bounds"
# Check secrets runbook cross-reference
if [ -f "docs/runbooks/secrets.md" ]; then
echo "✓ docs/runbooks/secrets.md exists"
# Check for the anchor
if grep -q "adding-or-removing-an-operator" "docs/runbooks/secrets.md"; then
echo "✓ Found section about adding/removing operators"
else
echo "⚠ Section anchor 'adding-or-removing-an-operator' not found"
fi
else
echo "✗ docs/runbooks/secrets.md MISSING"
exit 1
fiRepository: OpenZeppelin/guardian
Length of output: 272
Fix the runbook link anchor for operator enrollment/removal
crates/server/src/dashboard/state.rs line references (103-108, 284-324) are within bounds, but docs/DASHBOARD.md’s link to docs/runbooks/secrets.md#adding-or-removing-an-operator doesn’t match any corresponding section/anchor in docs/runbooks/secrets.md; update it to the correct heading/anchor slug.
🤖 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/DASHBOARD.md` around lines 182 - 208, The dashboard runbook link uses a
non-existent anchor "`#adding-or-removing-an-operator`"; open the runbooks secrets
doc, locate the exact heading slug for the section about adding/removing
operators (the heading text for the operator secrets section), and update the
anchor in DASHBOARD.md (the link that currently reads
./runbooks/secrets.md#adding-or-removing-an-operator) to that exact slug; while
there, verify the surrounding text that references guardian_operator_public_keys
remains accurate and update the link text if needed to match the target heading.
| ``` | ||
| GET /delta/since?account_id=...&nonce=<last-known-nonce> | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to code block.
The code block is missing a language identifier. Since this is a shell command example, specify bash for proper syntax highlighting.
📝 Proposed fix
-```
+```bash
GET /delta/since?account_id=...&nonce=<last-known-nonce></details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 134-134: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/TROUBLESHOOTING.md` around lines 134 - 136, The code block showing the
HTTP GET example is missing a language specifier; update the block around the
line containing "GET /delta/since?account_id=...&nonce=<last-known-nonce>" to
use a bash language fence by replacing the opening triple-backticks with
"```bash" (and keep the closing "```") so the example renders with shell/HTTP
highlighting.
| - `GUARDIAN_MAX_PENDING_PROPOSALS_PER_ACCOUNT` - Maximum pending delta proposals per account (default: `20`) | ||
| - `GUARDIAN_EVM_RPC_URLS` - Comma-separated `chain_id=rpc_url` map for EVM proposal support | ||
| - `GUARDIAN_EVM_ENTRYPOINTS` - Comma-separated `chain_id=entrypoint_address` map for EVM proposal finality checks | ||
| - `GUARDIAN_EVM_ENTRYPOINT_ADDRESS` - Shared EntryPoint address used for EVM proposal finality checks (single address across chains; defaults to EntryPoint v0.9 `0x433709009b8330fda32311df1c2afa402ed8d009`) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for any remaining references to the old GUARDIAN_EVM_ENTRYPOINTS variable
# and verify the new GUARDIAN_EVM_ENTRYPOINT_ADDRESS is properly implemented.
echo "=== Checking for old variable name ==="
rg -n 'GUARDIAN_EVM_ENTRYPOINTS' --type=rust --type=ts
echo -e "\n=== Checking for new variable name ==="
rg -n 'GUARDIAN_EVM_ENTRYPOINT_ADDRESS' --type=rust --type=ts
echo -e "\n=== Checking .env files ==="
fd -e env -x cat {}Repository: OpenZeppelin/guardian
Length of output: 1679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== README context (around line 75) ==="
sed -n '60,100p' README.md | nl -ba | sed -n '1,60p'
echo "=== All references to old variable (GUARDIAN_EVM_ENTRYPOINTS) ==="
rg -n 'GUARDIAN_EVM_ENTRYPOINTS' .
echo "=== All references to new variable (GUARDIAN_EVM_ENTRYPOINT_ADDRESS) ==="
rg -n 'GUARDIAN_EVM_ENTRYPOINT_ADDRESS' .
echo "=== References to EntryPoint v0.9 address snippet ==="
rg -n '0x433709009b8330fda32311df1c2afa402ed8d009' .
echo "=== .env / env.* files containing either variable ==="
fd -e env -e env.example -e env.sample -e env.template . -x sh -c 'echo "--- $1"; rg -n "GUARDIAN_EVM_ENTRYPOINT(S|_ADDRESS)" "$1" || true' sh {}Repository: OpenZeppelin/guardian
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== README lines 60-100 (with line numbers) ==="
awk 'NR>=60 && NR<=100 {printf "%d:%s\n", NR, $0}' README.md
echo "=== All references to old variable (GUARDIAN_EVM_ENTRYPOINTS) ==="
rg -n 'GUARDIAN_EVM_ENTRYPOINTS' .
echo "=== All references to new variable (GUARDIAN_EVM_ENTRYPOINT_ADDRESS) ==="
rg -n 'GUARDIAN_EVM_ENTRYPOINT_ADDRESS' .
echo "=== References to EntryPoint v0.9 address snippet ==="
rg -n '0x433709009b8330fda32311df1c2afa402ed8d009' .
echo "=== .env / env.* files containing either variable ==="
fd -e env -e env.example -e env.sample -e env.template . \
-x sh -c 'echo "--- $1"; rg -n "GUARDIAN_EVM_ENTRYPOINT(S|_ADDRESS)" "$1" || true' sh {}Repository: OpenZeppelin/guardian
Length of output: 7055
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspecting old var reference ==="
sed -n '1,60p' speckit/features/001-evm-proposal-support/quickstart.md | nl -ba || true
echo "=== Inspecting the specific line region around old var ==="
awk 'NR>=10 && NR<=30 {print NR ":" $0}' speckit/features/001-evm-proposal-support/quickstart.mdRepository: OpenZeppelin/guardian
Length of output: 804
Update remaining EVM quickstart docs to use GUARDIAN_EVM_ENTRYPOINT_ADDRESS
- The server reads
GUARDIAN_EVM_ENTRYPOINT_ADDRESS(shared single EntryPoint address) incrates/server/src/evm/config.rs, andREADME.mdalready documents/uses it at the referenced lines. - The only remaining reference to the old per-chain variable
GUARDIAN_EVM_ENTRYPOINTSis inspeckit/features/001-evm-proposal-support/quickstart.md(example command at the env var line); update that snippet toGUARDIAN_EVM_ENTRYPOINT_ADDRESS=0x...to match the new semantics.
🤖 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 `@README.md` at line 75, The quickstart example still uses the old per-chain
env var GUARDIAN_EVM_ENTRYPOINTS; update the snippet in
speckit/features/001-evm-proposal-support/quickstart.md to use the new shared
variable GUARDIAN_EVM_ENTRYPOINT_ADDRESS (e.g.,
GUARDIAN_EVM_ENTRYPOINT_ADDRESS=0x...), so it matches the server config read in
crates/server/src/evm/config.rs and the README.md usage; change the env var name
in the example command and any related explanatory text to the singular
GUARDIAN_EVM_ENTRYPOINT_ADDRESS and keep the same address format.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #246 +/- ##
=======================================
Coverage 75.28% 75.28%
=======================================
Files 131 131
Lines 23314 23314
=======================================
Hits 17551 17551
Misses 5763 5763 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Release Notes
Documentation
Chores
GUARDIAN_EVM_ENTRYPOINTStoGUARDIAN_EVM_ENTRYPOINT_ADDRESS.