Add architecture and threat model audit docs#161
Conversation
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.
|
@coderabbitai review Please review the Gate F architecture and threat-model docs/checker. Scope is documentation/tooling only: no Solidity behavior, ABI, bytecode, deployment addresses, or production evidence claims are intended. |
|
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 (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThis PR adds auditor-facing architecture and threat-model baseline documents for the local pre-audit phase, implements deterministic validation for their required structure and cross-links, integrates the checks into build and CI workflows, updates release artifacts to include the new documents, and propagates references throughout supporting documentation and project state tracking. ChangesArchitecture and threat-model audit package
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the main new documentation and checker files
echo "=== docs/architecture.md ==="
cat docs/architecture.md
echo ""
echo "=== docs/threat-model.md ==="
cat docs/threat-model.md
echo ""
echo "=== scripts/check_architecture_threat_model.py ==="
cat scripts/check_architecture_threat_model.py
echo ""
echo "=== scripts/test_architecture_threat_model.py ==="
cat scripts/test_architecture_threat_model.pyLength of output: 43491 🏁 Script executed: #!/bin/bash
echo "=== .github/workflows (list) ==="
fd -e yml -e yaml . .github/workflows/ | sort
echo ""
echo "=== Makefile (relevant lines) ==="
grep -n "architecture\|threat.model\|check_arch" Makefile | head -40
echo ""
echo "=== scripts/check.sh (relevant lines) ==="
grep -n "architecture\|threat.model\|check_arch" scripts/check.sh | head -20
echo ""
echo "=== scripts/check.ps1 (relevant lines) ==="
grep -n "architecture\|threat.model\|check_arch" scripts/check.ps1 | head -20
echo ""
echo "=== README.md (links to architecture/threat-model) ==="
grep -n "architecture\|threat.model\|threat-model" README.md | head -20
echo ""
echo "=== CHANGELOG (links) ==="
grep -n "architecture\|threat.model\|threat-model" CHANGELOG.md 2>/dev/null | head -20
echo ""
echo "=== Check if CONTRIBUTING.md and SECURITY.md exist ==="
ls -la CONTRIBUTING.md SECURITY.md 2>/dev/null
echo ""
echo "=== Check all REQUIRED_LINK_TARGETS exist ==="
python3 - <<'EOF'
from pathlib import Path
targets = [
"README.md", "CONTRIBUTING.md", "SECURITY.md",
"ops/ROADMAP.md", "ops/AUTONOMOUS_RUN.md", "ops/SLITHER_BASELINE.md",
"docs/architecture.md", "docs/threat-model.md", "docs/audit-package.md",
"docs/status.md", "docs/known-blockers.md", "docs/slither.md",
"docs/deployment.md", "docs/release-policy.md", "docs/release-signatures.md",
"docs/dependency-operations.md", "docs/randomizer-operations.md",
"docs/auction-custody.md", "docs/metadata.md", "docs/vendored-libraries.md",
"docs/adr/README.md",
"docs/adr/0001-drop-authorization.md", "docs/adr/0002-auction-custody.md",
"docs/adr/0003-payment-accounting.md", "docs/adr/0004-admin-governance.md",
"docs/adr/0005-randomness.md", "docs/adr/0006-metadata-freeze.md",
"docs/adr/0007-upgrade-redeployment.md",
"release-artifacts/README.md",
"release-artifacts/latest/release-manifest.json",
"release-artifacts/latest/SHA256SUMS",
"release-artifacts/latest/release-checksums.json",
"test/StreamPaymentsInvariant.t.sol",
"test/StreamSupplyReplayFreezeInvariant.t.sol",
"test/StreamAuctionInvariant.t.sol",
"test/StreamRandomizerPayments.t.sol",
"test/StreamDeploymentManifest.t.sol",
"scripts/check_audit_package.py",
]
for t in targets:
p = Path(t)
print(f"{'OK' if p.exists() else 'MISSING'}: {t}")
EOF
echo ""
echo "=== CI workflow that runs checks ==="
fd -e yml -e yaml . .github/ | xargs grep -l "check_architecture\|architecture_threat" 2>/dev/null | head -5Length of output: 6786
Overall AssessmentThe architecture and threat-model docs are clear, appropriately scoped, and honest about the local-baseline posture. The checker and tests are well-structured. All 38 docs/architecture.mdStrengths:
Minor observations:
docs/threat-model.mdStrengths:
Minor observations:
scripts/check_architecture_threat_model.pyStrengths:
One functional concern — redundant missing-file checks:
links = linked_repo_paths(repo_root, documents) # combined — validates all linked files exist
# ...
architecture_links = linked_repo_paths(repo_root, {architecture_path: ...}) # triggers missing-file check again
threat_links = linked_repo_paths(repo_root, {threat_model_path: ...}) # and againThe second and third calls re-check for missing files. This is harmless but means a broken link in only one doc will be reported with a less informative error (pointing at the wrong call site). Consider passing a # Suggested refactor (non-blocking):
arch_links = linked_repo_paths(repo_root, {architecture_path: documents[architecture_path]})
threat_links = linked_repo_paths(repo_root, {threat_model_path: documents[threat_model_path]})
links = arch_links | threat_links
# Then use arch_links / threat_links for reciprocal check directlyMissing test for reciprocal-link failure: The test suite covers heading, maturity, required-link, threat-phrase, and missing-file failures, but there is no test asserting that the checker rejects a pair of docs where Suggested addition to def test_rejects_missing_reciprocal_link_architecture_to_threat(self) -> None:
with tempfile.TemporaryDirectory() as temp_dir:
root = Path(temp_dir)
seed_required_targets(root)
# Remove the link from architecture → threat-model
text = minimal_architecture_doc().replace(
"Read the [threat model](threat-model.md).\n", ""
)
write_text(root / checker.DEFAULT_ARCHITECTURE, text)
write_text(root / checker.DEFAULT_THREAT_MODEL, minimal_threat_model_doc())
with self.assertRaisesRegex(
checker.ArchitectureThreatModelError, "must link to"
):
checker.validate_architecture_threat_model(
root,
root / checker.DEFAULT_ARCHITECTURE,
root / checker.DEFAULT_THREAT_MODEL,
)scripts/test_architecture_threat_model.pyStrengths:
One fragility — def test_accepts_committed_docs(self) -> None:
repo_root = Path.cwd()This passes only when the test is run from the repository root. If a CI runner or IDE invokes it from a different working directory, it will silently fail to find the committed docs. A more robust approach anchors to the script's own location: repo_root = Path(__file__).resolve().parents[1] # two levels up from scripts/CI / Makefile / Wrappers
Summary
The two most actionable items are:
Everything else is minor. The PR is in good shape for Gate F purposes. |
|
@coderabbitai review Follow-up commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== docs/architecture.md: StreamMinter and arRNG sections ==="
grep -n "StreamMinter\|arRNG\|ArweaveRNG\|on-chain RNG\|adr/0001" docs/architecture.md | head -30
echo ""
echo "=== docs/threat-model.md: deployment ceremony wording ==="
grep -n "deployment ceremony\|deployment" docs/threat-model.md | head -20
echo ""
echo "=== scripts/check_architecture_threat_model.py: linked_repo_paths calls ==="
grep -n "linked_repo_paths\|arch_links\|threat_links" scripts/check_architecture_threat_model.py | head -30
echo ""
echo "=== scripts/test_architecture_threat_model.py: Path anchor and reciprocal tests ==="
grep -n "Path.cwd\|__file__\|parents\|reciprocal\|must link" scripts/test_architecture_threat_model.py | head -30
echo ""
echo "=== Show test class methods list ==="
grep -n "def test_" scripts/test_architecture_threat_model.py | head -30
echo ""
echo "=== Show the full linked_repo_paths / validate function in checker ==="
grep -n "def validate_architecture_threat_model\|linked_repo_paths\|arch_links\|threat_links\|reciprocal" scripts/check_architecture_threat_model.py | head -40Length of output: 5995
Everything checks out. The PR is clean for Gate F. [approve] |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/test_architecture_threat_model.py (1)
145-273: ⚡ Quick winAdd an explicit regression test for missing reciprocal links.
The checker enforces bidirectional architecture↔threat-model linking, but there is no direct negative test asserting rejection when one side omits the reciprocal link.
Suggested test addition
class ArchitectureThreatModelTests(unittest.TestCase): + def test_rejects_missing_reciprocal_link(self) -> None: + with tempfile.TemporaryDirectory() as temp_dir: + root = Path(temp_dir) + seed_required_targets(root) + arch = minimal_architecture_doc().replace( + "Read the [threat model](threat-model.md).\n", "" + ) + write_text(root / checker.DEFAULT_ARCHITECTURE, arch) + write_text(root / checker.DEFAULT_THREAT_MODEL, minimal_threat_model_doc()) + + with self.assertRaisesRegex( + checker.ArchitectureThreatModelError, "must link to" + ): + checker.validate_architecture_threat_model( + root, + root / checker.DEFAULT_ARCHITECTURE, + root / checker.DEFAULT_THREAT_MODEL, + )🤖 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_architecture_threat_model.py` around lines 145 - 273, Add a new unit test method on ArchitectureThreatModelTests (e.g., test_rejects_missing_reciprocal_link) that seeds required targets, writes a minimal architecture doc that includes a link to the threat model but writes a threat model doc that omits the reciprocal architecture link (and vice versa in another subcase if desired), then call checker.validate_architecture_threat_model(root, root / checker.DEFAULT_ARCHITECTURE, root / checker.DEFAULT_THREAT_MODEL) inside self.assertRaisesRegex(checker.ArchitectureThreatModelError, "missing required links|reciprocal") to assert the checker rejects missing bidirectional links; reference the existing helpers seed_required_targets, minimal_architecture_doc, minimal_threat_model_doc, and constants checker.DEFAULT_ARCHITECTURE / checker.DEFAULT_THREAT_MODEL to locate where to add the test.
🤖 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 `@ops/ROADMAP.md`:
- Around line 76-81: The table's "CI run" cell still references PR `#160`; update
the "CI run" entry in the table (the row labeled CI run) to point to the actual
current PR or CI run ID for this change set (replace "TBD for PR `#160`" with "CI
run: PR #<current-number>" or the appropriate run URL/ID) so the verification
trail clearly references the correct PR; ensure you only edit the "CI run" cell
in the roadmap table text.
In `@scripts/test_architecture_threat_model.py`:
- Around line 146-151: The test uses Path.cwd() to set repo_root which breaks
when tests run outside the repo; change the setup in test_accepts_committed_docs
to derive the repo root from the test file location (use
Path(__file__).resolve().parent or appropriate ancestor) and pass that path to
checker.main(["--repo-root", str(repo_root)]) so the test always anchors to the
repository location relative to this test file rather than the process CWD.
---
Nitpick comments:
In `@scripts/test_architecture_threat_model.py`:
- Around line 145-273: Add a new unit test method on
ArchitectureThreatModelTests (e.g., test_rejects_missing_reciprocal_link) that
seeds required targets, writes a minimal architecture doc that includes a link
to the threat model but writes a threat model doc that omits the reciprocal
architecture link (and vice versa in another subcase if desired), then call
checker.validate_architecture_threat_model(root, root /
checker.DEFAULT_ARCHITECTURE, root / checker.DEFAULT_THREAT_MODEL) inside
self.assertRaisesRegex(checker.ArchitectureThreatModelError, "missing required
links|reciprocal") to assert the checker rejects missing bidirectional links;
reference the existing helpers seed_required_targets, minimal_architecture_doc,
minimal_threat_model_doc, and constants checker.DEFAULT_ARCHITECTURE /
checker.DEFAULT_THREAT_MODEL to locate where to add the test.
🪄 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: 51349d0a-4a47-4154-aabd-9acef8752505
📒 Files selected for processing (23)
.github/workflows/ci.ymlCHANGELOG.mdMakefileREADME.mddocs/architecture.mddocs/audit-package.mddocs/release-policy.mddocs/status.mddocs/threat-model.mddocs/tooling.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdrelease-artifacts/README.mdrelease-artifacts/latest/SHA256SUMSrelease-artifacts/latest/release-checksums.jsonrelease-artifacts/latest/release-manifest.jsonscripts/check.ps1scripts/check.shscripts/check_architecture_threat_model.pyscripts/check_audit_package.pyscripts/generate_release_manifest.pyscripts/test_architecture_threat_model.pyscripts/test_release_manifest.py
|
@coderabbitai review Follow-up commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== git log last 3 commits ==="
git log --oneline -5
echo ""
echo "=== diff of f0b4407 ==="
git show f0b4407 --stat
echo ""
echo "=== full diff of f0b4407 ==="
git show f0b4407
echo ""
echo "=== ops/ROADMAP.md CI row context ==="
grep -n "CI\|`#161`\|PR `#161`\|161" ops/ROADMAP.md | head -30Length of output: 7241
That is the correct fix — Combined with [approve] |
Summary
Closes #160.
Adds the Gate F architecture and threat-model audit docs, plus a deterministic checker/test pair so required headings, maturity warnings, core architecture terms, threat categories, maintenance commands, reciprocal links, evidence links, and missing linked files cannot silently regress.
This is documentation/tooling only. It does not change Solidity behavior, ABI, bytecode, deployment addresses, or production-readiness claims.
Changes
docs/architecture.mdanddocs/threat-model.mdas auditor-facing Gate F docs.scripts/check_architecture_threat_model.pyandscripts/test_architecture_threat_model.py.make check, Unix/Windows wrappers, and CI.Local Validation
python scripts\test_architecture_threat_model.pypython scripts\check_architecture_threat_model.pypython scripts\test_audit_package.pypython scripts\check_audit_package.pypython -m py_compile scripts\check_architecture_threat_model.py scripts\test_architecture_threat_model.py scripts\check_audit_package.py scripts\test_audit_package.py scripts\generate_release_manifest.py scripts\test_release_manifest.py scripts\generate_release_checksums.py scripts\test_release_checksums.py scripts\check_changelog.py scripts\test_changelog_check.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_changelog_check.pypython scripts\check_changelog.pybash -n scripts/check.shscripts\check.ps1git diff --checkmake checkpowershell -ExecutionPolicy Bypass -File scripts\check.ps1Summary by CodeRabbit
Documentation
New Features
Chores