feat: consolidate saorsa-core + saorsa-transport into workspace#66
feat: consolidate saorsa-core + saorsa-transport into workspace#66grumbach wants to merge 1 commit intoWithAutonomi:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR restructures ant-node into a Cargo workspace by vendoring saorsa-core (and referencing saorsa-transport) as workspace members, replacing fragile cross-repo dependency references with local path dependencies.
Changes:
- Introduces a workspace root in
Cargo.tomland switchesant-nodeto depend on localcrates/saorsa-core. - Adds the vendored
saorsa-corecrate with its ownCargo.toml, build script, and architecture doc. - Brings in a substantial amount of
.planning/review artifacts and repo/tooling config files undercrates/saorsa-core/.
Reviewed changes
Copilot reviewed 81 out of 621 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/saorsa-core/build.rs | Adds an (empty) build script stub for the vendored crate. |
| crates/saorsa-core/Cargo.toml | Defines saorsa-core package metadata and dependencies, including a path dep to saorsa-transport. |
| crates/saorsa-core/ARCHITECTURE.md | Adds architecture documentation for the vendored crate. |
| crates/saorsa-core/.planning/reviews/workflow-configuration.md | Adds internal workflow review notes. |
| crates/saorsa-core/.planning/reviews/type-safety-review-validation.md | Adds internal validation notes (includes local absolute paths). |
| crates/saorsa-core/.planning/reviews/test-coverage.md | Adds internal test coverage review notes. |
| crates/saorsa-core/.planning/reviews/task-spec.md | Adds internal task spec review notes. |
| crates/saorsa-core/.planning/reviews/review-iteration-1-complete.md | Adds internal iteration summary. |
| crates/saorsa-core/.planning/reviews/phase-1-review-summary.md | Adds internal phase summary. |
| crates/saorsa-core/.planning/reviews/minimax.md | Adds an external review artifact. |
| crates/saorsa-core/.planning/reviews/kimi.md | Adds an external review artifact (contains auth failure details). |
| crates/saorsa-core/.planning/reviews/iteration-2-summary.md | Adds internal iteration summary. |
| crates/saorsa-core/.planning/reviews/iteration-2-fixes-applied.md | Adds internal fixes-applied summary. |
| crates/saorsa-core/.planning/reviews/glm.md | Adds an external review artifact. |
| crates/saorsa-core/.planning/reviews/fixes-applied-phase2-task2.md | Adds internal fixes-applied summary. |
| crates/saorsa-core/.planning/reviews/fixes-applied-phase2-task1.md | Adds internal fixes-applied summary. |
| crates/saorsa-core/.planning/reviews/fixes-applied-encoding.md | Adds internal fixes-applied summary. |
| crates/saorsa-core/.planning/reviews/error-handling.md | Adds internal error-handling audit notes (includes local absolute paths). |
| crates/saorsa-core/.planning/reviews/documentation.md | Adds internal documentation audit notes. |
| crates/saorsa-core/.planning/reviews/consensus-type-safety-20260129.md | Adds consensus review artifact. |
| crates/saorsa-core/.planning/reviews/consensus-encoding-review-20260129.md | Adds consensus review artifact. |
| crates/saorsa-core/.planning/reviews/consensus-20260204-183500.md | Adds consensus review artifact. |
| crates/saorsa-core/.planning/reviews/consensus-20260202-143500.md | Adds consensus review artifact. |
| crates/saorsa-core/.planning/reviews/consensus-20260129-210600.md | Adds consensus review artifact. |
| crates/saorsa-core/.planning/reviews/consensus-20260129-200500.md | Adds consensus review artifact. |
| crates/saorsa-core/.planning/reviews/consensus-20260129-155118.md | Adds consensus review artifact. |
| crates/saorsa-core/.planning/reviews/consensus-20260129-154812.md | Adds consensus review artifact. |
| crates/saorsa-core/.planning/reviews/consensus-20260129-153828.md | Adds consensus review artifact. |
| crates/saorsa-core/.planning/reviews/build.md | Adds internal build validation notes. |
| crates/saorsa-core/.planning/reviews/build-iteration-2.md | Adds internal build validation notes. |
| crates/saorsa-core/.planning/reviews/EXTERNAL_REVIEWS_COMPLETE.md | Adds external reviews completion marker. |
| crates/saorsa-core/.planning/reviews/CODEX_REVIEW_SUMMARY.md | Adds external review summary artifact. |
| crates/saorsa-core/.planning/progress.md | Adds internal project progress log. |
| crates/saorsa-core/.planning/architecture-analysis/06-forward-secrecy.md | Adds internal architecture analysis document. |
| crates/saorsa-core/.planning/architecture-analysis/05-message-persistence.md | Adds internal architecture analysis document. |
| crates/saorsa-core/.planning/architecture-analysis/04-routing-strategy.md | Adds internal architecture analysis document. |
| crates/saorsa-core/.planning/architecture-analysis/03-offline-delivery.md | Adds internal architecture analysis document. |
| crates/saorsa-core/.planning/STATE.json | Adds internal project/review state tracking. |
| crates/saorsa-core/.planning/REVIEW_ITERATION_1_COMPLETE.md | Adds internal review cycle artifact. |
| crates/saorsa-core/.planning/REVIEW_CYCLE_COMPLETE.md | Adds internal review cycle artifact. |
| crates/saorsa-core/.planning/REVIEW_CYCLE_1_COMPLETE.md | Adds internal review cycle artifact. |
| crates/saorsa-core/.planning/PROJECT.md | Adds internal project description. |
| crates/saorsa-core/.planning/PLAN-phase-5.md | Adds internal planning document. |
| crates/saorsa-core/.planning/PLAN-phase-3.md | Adds internal planning document. |
| crates/saorsa-core/.planning/PLAN-phase-1.md | Adds internal planning document. |
| crates/saorsa-core/.planning/PHASE_2_TASK_1_REVIEW_ITERATION_1.md | Adds internal review tracking document. |
| crates/saorsa-core/.planning/PHASE_2_TASK_1_ITERATION_1_STATUS.md | Adds internal status document. |
| crates/saorsa-core/.planning/PHASE_2_TASK_1_FIXES_APPLIED.md | Adds internal fixes-applied document. |
| crates/saorsa-core/.planning/MILESTONES.md | Adds internal milestones document. |
| crates/saorsa-core/.planning/CODE_QUALITY_REVIEW_EXECUTIVE_SUMMARY.md | Adds internal executive summary document. |
| crates/saorsa-core/.gitignore | Adds crate-local ignore rules. |
| crates/saorsa-core/.cursorules | Adds editor/tool configuration pointer. |
| crates/saorsa-core/.cursorrules | Adds editor/tool configuration pointer (duplicate naming variant). |
| crates/saorsa-core/.clippy.toml | Adds crate-local clippy configuration. |
| crates/saorsa-core/.actrc | Adds local act runner configuration. |
| Cargo.toml | Declares the workspace and switches saorsa-core to a path dependency; bumps ant-node version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Copyright 2024 Saorsa Labs Limited | ||
| // | ||
| // Build script for saorsa-core | ||
| // | ||
| // Currently empty - VDF guest program no longer required. | ||
| // Signed heartbeats replaced VDF-based heartbeats in Phase 4. | ||
|
|
||
| fn main() { | ||
| // No build steps required | ||
| } |
There was a problem hiding this comment.
The crate adds a build.rs that does nothing. Even an “empty” build script still participates in Cargo’s build graph (can add compile-time overhead and complicate downstream consumers/docs builds). If there are truly no build steps required, remove build.rs entirely (and any related docs.rs/build-script commentary), and only reintroduce it when it’s needed.
| // Copyright 2024 Saorsa Labs Limited | |
| // | |
| // Build script for saorsa-core | |
| // | |
| // Currently empty - VDF guest program no longer required. | |
| // Signed heartbeats replaced VDF-based heartbeats in Phase 4. | |
| fn main() { | |
| // No build steps required | |
| } |
| [package] | ||
| name = "saorsa-core" | ||
| # 0.10.3: postcard serialization migration (PR #14) | ||
| # 0.10.4: refocus on phonebook + trust signals, remove user-facing APIs (PR #15) | ||
| # 0.11.0: remove feature gates — adaptive/trust/placement always compiled | ||
| # 0.11.1: fix Kademlia protocol violations in iterative lookups | ||
| # 0.12.0: configurable max_message_size, bump saorsa-transport to 0.22.0 | ||
| # 0.12.1: patch release | ||
| # 0.13.0: multi-channel transport, PeerId ownership, BLAKE3 migration (PR #32) | ||
| # 0.14.0: user-agent DHT gating — exclude ephemeral clients from DHT routing table | ||
| # 0.14.1: fix deterministic identity generation to use real ML-DSA keypairs | ||
| # 0.15.0: routing table single-source-of-truth fixes | ||
| # 0.16.0: delegate transport addressing to saorsa-transport's TransportAddr (PR #39) | ||
| # 0.17.0: remove configuration system, streamline network setup | ||
| # 0.17.1: keep logging macros crate-internal, strip from public API | ||
| # 0.17.2: bump saorsa-transport to 0.27 | ||
| # 0.18.0: harden send_message reconnect logic | ||
| # 0.18.1: enforce address invariants in KBucket::add_node | ||
| # 0.19.0: NAT traversal timeouts, dual-stack normalisation, connection reliability | ||
| # 0.20.0: simplify IP diversity, stale-peer fixes, cache atomicity improvements | ||
| # 0.21.0: penalty-only trust model, distance-sorted lookup candidates, stale docs cleanup | ||
| # 0.22.0: MASQUE relay data plane, upgrade saorsa-transport to 0.31.0 | ||
| version = "0.22.0" |
There was a problem hiding this comment.
This vendored crate includes a large amount of internal planning/review artifacts under .planning/ (as seen in this PR). If saorsa-core is ever packaged/published (or used as a dependency outside this repo), these files will be included by default, increasing crate size and distributing internal process notes. Consider adding publish = false (if it should never be published from this workspace) or adding an explicit exclude = [".planning/**", ...] / include = [...] to control packaging content.
| ## Review Summary | ||
|
|
||
| ### File Analyzed | ||
| - **Path**: `/Users/davidirvine/Desktop/Devel/projects/saorsa-core/src/messaging/encoding.rs` |
There was a problem hiding this comment.
This documentation embeds an absolute local filesystem path (including a username). That can leak developer environment details and isn’t portable/meaningful to other contributors. Replace absolute paths with repo-relative paths (e.g., src/messaging/encoding.rs) and avoid including local machine directories in committed artifacts.
| - **Path**: `/Users/davidirvine/Desktop/Devel/projects/saorsa-core/src/messaging/encoding.rs` | |
| - **Path**: `src/messaging/encoding.rs` |
| [workspace] | ||
| members = [ | ||
| ".", | ||
| "crates/saorsa-core", | ||
| "crates/saorsa-transport", | ||
| "crates/saorsa-transport/saorsa-transport-workspace-hack", | ||
| ] |
There was a problem hiding this comment.
Including "." as a workspace member is typically redundant when the workspace root is also a package (the root package is already part of the workspace). Consider dropping "." to reduce confusion and keep the workspace member list focused on subcrates.
| [workspace] | ||
| members = [ | ||
| ".", | ||
| "crates/saorsa-core", | ||
| "crates/saorsa-transport", | ||
| "crates/saorsa-transport/saorsa-transport-workspace-hack", | ||
| ] |
There was a problem hiding this comment.
The PR description says “No code changes. Pure repo structure.” but this diff introduces many additional vendored project artifacts (e.g., .planning/**, multiple review reports, tooling configs, and a build.rs). If these are intentional parts of the vendored sources, it’d help to explicitly call out that non-code artifacts are being imported as well; otherwise, consider splitting them into a follow-up PR or excluding them from the vendoring step to keep this PR narrowly about workspace consolidation.
c3c2a6b to
5327d19
Compare
5327d19 to
f42dbaf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 485 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
crates/saorsa-core/src/rate_limit.rs:1
- The doc comments state security-sensitive defaults (e.g., “/64 default: 1”, global default: 100), but
Defaultcurrently sets all limits to10_000, effectively disabling join rate limiting out of the box. This is security-relevant (Sybil/flood protection) and also misleading documentation. Recommendation (mandatory): either (a) setDefaultto the documented safe values, or (b) explicitly document that defaults are “effectively unlimited / development defaults” and provide a separateJoinRateLimiterConfig::production_defaults()(or similar) with the strict values.
use lru::LruCache;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [workspace] | ||
| members = [ | ||
| ".", | ||
| "crates/saorsa-core", | ||
| "crates/saorsa-transport", | ||
| "crates/saorsa-transport/saorsa-transport-workspace-hack", | ||
| ] | ||
| resolver = "2" |
There was a problem hiding this comment.
The PR description says “No code changes. Pure repo structure.” but this PR introduces a workspace and vendors in a substantial amount of new saorsa-core code/docs (not just a move/rename). Please update the PR description to reflect that the source is being imported into this repo (even if copied verbatim from upstream), and clarify the expected implications (e.g., semver/versioning, ownership of future edits, and upgrade workflow).
| [dependencies] | ||
| # Core (provides EVERYTHING: networking, DHT, security, trust, storage) | ||
| saorsa-core = "0.22.0" | ||
| saorsa-core = { path = "crates/saorsa-core" } |
There was a problem hiding this comment.
The PR description says “No code changes. Pure repo structure.” but this PR introduces a workspace and vendors in a substantial amount of new saorsa-core code/docs (not just a move/rename). Please update the PR description to reflect that the source is being imported into this repo (even if copied verbatim from upstream), and clarify the expected implications (e.g., semver/versioning, ownership of future edits, and upgrade workflow).
| pub struct Engine<K: Eq + Hash + Clone + ToString> { | ||
| cfg: EngineConfig, | ||
| global: Mutex<Bucket>, | ||
| /// LRU cache with max 100k entries to prevent memory DoS from many IPs | ||
| keyed: RwLock<LruCache<K, Bucket>>, | ||
| } | ||
|
|
||
| impl<K: Eq + Hash + Clone + ToString> Engine<K> { |
There was a problem hiding this comment.
Engine<K> requires ToString, but the implementation shown doesn’t use it. Dropping the ToString bound will make the type easier to reuse (and reduces unnecessary trait constraints) while keeping behavior identical.
| pub struct Engine<K: Eq + Hash + Clone + ToString> { | |
| cfg: EngineConfig, | |
| global: Mutex<Bucket>, | |
| /// LRU cache with max 100k entries to prevent memory DoS from many IPs | |
| keyed: RwLock<LruCache<K, Bucket>>, | |
| } | |
| impl<K: Eq + Hash + Clone + ToString> Engine<K> { | |
| pub struct Engine<K: Eq + Hash + Clone> { | |
| cfg: EngineConfig, | |
| global: Mutex<Bucket>, | |
| /// LRU cache with max 100k entries to prevent memory DoS from many IPs | |
| keyed: RwLock<LruCache<K, Bucket>>, | |
| } | |
| impl<K: Eq + Hash + Clone> Engine<K> { |
| /// Handle a DHT query request. | ||
| async fn handle_query( | ||
| &self, | ||
| remote_addr: SocketAddr, | ||
| data: Bytes, | ||
| ) -> LinkResult<Option<Bytes>> { | ||
| trace!(remote_addr = %remote_addr, size = data.len(), "Processing DHT query"); | ||
|
|
||
| let message: DhtMessage = postcard::from_bytes(&data) | ||
| .map_err(|e| LinkError::Internal(format!("Failed to deserialize query: {e}")))?; | ||
|
|
||
| let response = self.process_message(message).await?; | ||
|
|
||
| let response_bytes = postcard::to_stdvec(&response) | ||
| .map_err(|e| LinkError::Internal(format!("Failed to serialize response: {e}")))?; | ||
|
|
||
| Ok(Some(Bytes::from(response_bytes))) | ||
| } |
There was a problem hiding this comment.
This introduces non-trivial request/response serialization logic and error mapping, but the current tests in this file only cover stream type conversions. Consider adding unit tests that (1) round-trip a DhtMessage through handle_query and assert the decoded DhtResponse, and (2) verify invalid postcard input returns the expected LinkError variant/message prefix. This will guard against protocol regressions during the workspace consolidation.
| pub mod node_identity; | ||
| pub mod peer_id; | ||
|
|
||
| pub use node_identity::{IdentityData, NodeIdentity}; | ||
| pub use peer_id::{PEER_ID_BYTE_LEN, PeerId, PeerIdParseError}; |
There was a problem hiding this comment.
src/identity/node_identity_extensions.rs is added in this PR but is not declared here (or elsewhere in the shown module tree). As written, it won’t be compiled, and its impl NodeIdentity helpers won’t be available to callers/tests. Recommendation: either add pub mod node_identity_extensions; (or #[cfg(test)] mod ... if test-only), or remove/relocate the file to avoid dead, confusing code.
| Ok(()) | ||
| }) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("close group cache save task panicked: {e}"))? |
There was a problem hiding this comment.
spawn_blocking(...).await can fail due to cancellation as well as panic. The message “task panicked” is inaccurate in the cancellation case and can mislead operational debugging. Consider rewording to “close group cache save task failed” (or similar) while still including {e}.
| .map_err(|e| anyhow::anyhow!("close group cache save task panicked: {e}"))? | |
| .map_err(|e| anyhow::anyhow!("close group cache save task failed: {e}"))? |
| } | ||
|
|
||
| impl From<anyhow::Error> for AdaptiveNetworkError { | ||
| fn from(e: anyhow::Error) -> Self { | ||
| AdaptiveNetworkError::Network(std::io::Error::other(e.to_string())) | ||
| } |
There was a problem hiding this comment.
Converting anyhow::Error into std::io::Error loses structured context/classification and forces downstream code to treat non-I/O failures as I/O. Consider adding a dedicated Anyhow(#[from] anyhow::Error) (or Other(anyhow::Error)) variant instead, so callers can preserve error chains and avoid misclassification.
| } | |
| impl From<anyhow::Error> for AdaptiveNetworkError { | |
| fn from(e: anyhow::Error) -> Self { | |
| AdaptiveNetworkError::Network(std::io::Error::other(e.to_string())) | |
| } | |
| #[error("Unhandled error: {0}")] | |
| Anyhow(#[from] anyhow::Error), |
f42dbaf to
9384621
Compare
9384621 to
68835e6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 58 out of 487 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl Default for JoinRateLimiterConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| max_joins_per_64_per_hour: 10_000, | ||
| max_joins_per_48_per_hour: 10_000, | ||
| max_joins_per_24_per_hour: 10_000, | ||
| max_global_joins_per_minute: 10_000, | ||
| global_burst_size: 10_000, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
JoinRateLimiterConfig’s doc comments describe strict defaults (e.g., “/64 default: 1”, “/48 default: 5”, “/24 default: 3”, “global default: 100”), but Default currently sets all limits to 10,000, which effectively disables Sybil protection by default. Either update Default to match the intended security posture, or update the doc comments to reflect the actual defaults (and consider using an explicit JoinRateLimiterConfig::permissive_dev() for high limits).
| fn test_proof_of_work() { | ||
| // PoW removed: this test no longer applicable |
There was a problem hiding this comment.
This test is now a no-op and will always pass, which reduces signal in the test suite. Please remove it, or replace it with an assertion that validates the intended post-change invariant (e.g., that identity generation/verification works without any PoW-related fields or APIs).
| fn test_proof_of_work() { | |
| // PoW removed: this test no longer applicable | |
| fn test_identity_works_without_proof_of_work() { | |
| let identity = NodeIdentity::generate().expect("Identity generation should succeed"); | |
| // Identity should be usable immediately without any PoW-specific step. | |
| assert_eq!( | |
| identity.peer_id, | |
| peer_id_from_public_key(identity.public_key()) | |
| ); | |
| let message = b"identity works without proof of work"; | |
| let signature = identity.sign(message).expect("Signing should succeed"); | |
| assert!(identity | |
| .verify(message, &signature) | |
| .expect("Verification should succeed")); |
| // Re-export saorsa-transport PQC functions for convenience | ||
| pub use self::saorsa_transport_integration::{generate_ml_dsa_keypair, ml_dsa_sign, ml_dsa_verify}; | ||
|
|
||
| // Primary post-quantum cryptography types from saorsa-pqc 0.3.0 |
There was a problem hiding this comment.
The comment says saorsa-pqc 0.3.0, but Cargo.toml in this PR pins saorsa-pqc = "0.5". Please update the comment to avoid misleading version guidance (or remove the hard-coded version reference).
| // Primary post-quantum cryptography types from saorsa-pqc 0.3.0 | |
| // Primary post-quantum cryptography types from saorsa-pqc |
| | Node | Provider | IP Address | Region | Purpose | Status | | ||
| |------|----------|------------|--------|---------|--------| | ||
| | saorsa-1 | Hetzner | 77.42.75.115 | Helsinki | Dashboard & Website | Active | | ||
| | saorsa-2 | DigitalOcean | 142.93.199.50 | NYC1 | Bootstrap Node | Active | | ||
| | saorsa-3 | DigitalOcean | 147.182.234.192 | SFO3 | Bootstrap Node | Active | | ||
| | saorsa-4 | DigitalOcean | 206.189.7.117 | AMS3 | Test Node | Active | | ||
| | saorsa-5 | DigitalOcean | 144.126.230.161 | LON1 | Test Node | Active | | ||
| | saorsa-6 | Hetzner | 65.21.157.229 | Helsinki | Test Node | Active | | ||
| | saorsa-7 | Hetzner | 116.203.101.172 | Nuremberg | Test Node | Active | | ||
| | saorsa-8 | Vultr | 149.28.156.231 | Singapore | Test Node | Active | | ||
| | saorsa-9 | Vultr | 45.77.176.184 | Tokyo | Test Node | Active | |
There was a problem hiding this comment.
This doc publishes detailed infrastructure inventory (public IPs, roles, provisioning commands, and also lists SSH key identifiers later in the doc). Even if the IPs are public, consolidating this information in-repo can materially increase attackability (target selection, port scanning focus, social-engineering). Consider moving these details to a private ops repo/wiki, or redact/abstract sensitive fields (IPs, key IDs, exact firewall names), leaving only high-level operational guidance.
| version = "0.22.0" | ||
| edition = "2024" | ||
| authors = ["Saorsa Labs Limited <[email protected]>"] | ||
| license = "AGPL-3.0" |
There was a problem hiding this comment.
Many newly added source headers state the code is dual-licensed “AGPL-3.0-or-later” + “Commercial License”, but Cargo.toml declares license = "AGPL-3.0". To keep crate metadata consistent with the headers (and with SPDX expectations), update the manifest license expression (e.g., AGPL-3.0-or-later) and/or document the commercial licensing via license-file / LicenseRef-... per your distribution strategy.
| license = "AGPL-3.0" | |
| license = "AGPL-3.0-or-later" |
Move saorsa-core and saorsa-transport into crates/ as workspace members. All dependencies now use path references instead of fragile git URLs, branch names, or commit hashes. Cleaned up ~100 non-source files: AI planning artifacts, saved test outputs, Go legacy files, orphaned scripts. 492 ant-node tests pass.
68835e6 to
95bedc8
Compare
Why
Our dependency situation is getting out of hand. Across PRs and repos we have:
path = "../saorsa-transport"that only works on one machine (saorsa-core#75)[patch]sections pointing at personal fork branches (ant-client#30)branch = "rc-2026.4.1"git refs that break on rebase (ant-node Cargo.toml)rev = "6dfc7178..."commit pins that need manual updates (ant-client main)ant-quicstill referenced where it should besaorsa-transport(saorsa-testnet, saorsa-gossip)Something that builds on one developer's machine breaks on another's. Every NAT traversal fix requires coordinated PRs across 3 repos.
What this does
Moves saorsa-core and saorsa-transport into
crates/as workspace members. All dependencies become simple path references.What this does NOT do
Follow-up PRs (stacked on this)