feat: HC auth for and Unyt 0.62.0+ (upgraded to 0.64.0)#756
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDownloads alliance.dna at build time; adds SignWithKey request/response and handler; implements HC-auth bootstrap material creation and conductor space-override wiring; pins many Holochain/Kitsune dependencies; updates unyt install/restart to persist agent key/DNA hash; and adjusts Hosting UI and payment error mapping. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant Build as Build Script
participant Unyt as UnytService
participant HCAuth as HC_AUTH (bootstrap API)
participant HolochainSvc as HolochainService
participant Keystore as Keystore
participant Conductor as Conductor
participant DB as Database
Dev->>Build: cargo build
Build->>Build: ensure OUT_DIR/alliance.dna (download if missing)
Build-->>Unyt: alliance.dna available
Unyt->>Conductor: install_alliance_dna()
Conductor-->>Unyt: app_info
Unyt->>DB: read unyt_auth_material, unyt_dna_hash
alt auth missing
Unyt->>HCAuth: GET /now (challenge)
HCAuth-->>Unyt: base64url(challenge)
Unyt->>HolochainSvc: sign_with_key(agent_key, challenge)
HolochainSvc->>Keystore: keystore.sign(agent_key, challenge)
Keystore-->>HolochainSvc: signature
HolochainSvc-->>Unyt: signature
Unyt->>Unyt: assemble base64_auth_material, persist to DB
end
alt dna hash missing
Unyt->>Conductor: capture_dna_hash()
Conductor-->>Unyt: dna_hash
Unyt->>DB: store unyt_dna_hash
end
Unyt->>Conductor: restart with space_overrides (urls + base64_auth_material)
Conductor-->>HolochainSvc: ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
lucksus
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
rust-executor/src/unyt_service.rs (2)
524-526: Verify the Ed25519 public key byte extraction from HoloHash.The code extracts bytes 3..35 from the 39-byte HoloHash to get the 32-byte Ed25519 public key. This assumes a specific HoloHash structure where:
- Bytes 0-2: Type prefix
- Bytes 3-34: 32-byte Ed25519 public key
- Bytes 35-38: Location bytes (DHT location)
This is the correct structure for Holochain's
AgentPubKey, but consider adding a comment documenting this layout for future maintainers.📝 Suggested documentation improvement
- // Raw 32-byte Ed25519 public key is at bytes 3..35 of the 39-byte HoloHash let raw_39 = agent_key.get_raw_39(); + // HoloHash structure (39 bytes): + // [0..3] = type prefix (3 bytes) + // [3..35] = Ed25519 public key (32 bytes) + // [35..39] = DHT location bytes (4 bytes) let raw_pubkey: Vec<u8> = raw_39[3..35].to_vec();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/unyt_service.rs` around lines 524 - 526, The extraction of the 32-byte Ed25519 key from the HoloHash (raw_39[3..35]) should be documented and made safer: update the code around agent_key.get_raw_39(), raw_39, and raw_pubkey to include a brief comment describing the HoloHash layout (bytes 0-2 = type prefix, bytes 3-34 = 32-byte Ed25519 pubkey, bytes 35-38 = location) and add a length check/assert on raw_39 before slicing to ensure it is 39 bytes (or handle unexpected lengths with a clear error) so future maintainers understand the assumption and you avoid panics on malformed input.
529-538: Consider adding a timeout to the HTTP client request.The
reqwest::Client::new()uses default settings without an explicit timeout. If the hc-auth server is slow or unresponsive, this could block indefinitely.⏱️ Proposed fix to add timeout
// GET /now — returns base64url-encoded 32-byte challenge let now_url = format!("{}/now", HC_AUTH_URL); - let client = reqwest::Client::new(); + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(30)) + .build() + .map_err(|e| deno_core::anyhow::anyhow!("Failed to create HTTP client: {}", e))?; let payload_b64url = client .get(&now_url) .send()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/unyt_service.rs` around lines 529 - 538, The request to HC_AUTH_URL uses reqwest::Client::new() without a timeout, so create the client with a timeout (e.g., via reqwest::Client::builder().timeout(Duration::from_secs(X)).build() or set a per-request timeout on the RequestBuilder) and replace the current Client::new() usage; update the code that constructs now_url/get() (the variables now_url, client and payload_b64url) to use the timed client (and add use std::time::Duration) so the .send().await won't block indefinitely if the hc-auth server is slow or unresponsive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-executor/build.rs`:
- Around line 45-64: Add a hardcoded SHA-256 checksum (or public-key signature
verification) for the alliance release asset and verify it immediately after
downloading and before any include_bytes! usage: after the curl Command that
fetches ALLIANCE_DNA_URL to dest, compute the downloaded file's SHA-256 (e.g.,
via the sha2 crate or a system tool) and compare it to the pinned checksum
constant in the repo, and call panic! or assert! (failing the build) if it
doesn't match; alternatively implement signature verification using a stored
public key instead of checksum. Ensure references to ALLIANCE_DNA_URL, dest, and
the include_bytes! consumer are updated to reflect this verification step so the
build never embeds unverified bytes.
- Around line 29-38: The build script currently treats any non-empty dest
("alliance.dna") as valid; change this to cache by version/stamp instead: use
the ALLIANCE_DNA_VERSION constant to check a small companion file (e.g.,
"alliance.dna.version") in OUT_DIR and only accept the cached DNA if that file
exists and its contents equal the expected version; when downloading, write the
new DNA to a temporary file and atomically rename it to dest, then write the
version file (or checksum) after a successful download so partial/failed
downloads or version bumps won't be reused; apply the same pattern to the other
identical block referenced (lines 64-68).
---
Nitpick comments:
In `@rust-executor/src/unyt_service.rs`:
- Around line 524-526: The extraction of the 32-byte Ed25519 key from the
HoloHash (raw_39[3..35]) should be documented and made safer: update the code
around agent_key.get_raw_39(), raw_39, and raw_pubkey to include a brief comment
describing the HoloHash layout (bytes 0-2 = type prefix, bytes 3-34 = 32-byte
Ed25519 pubkey, bytes 35-38 = location) and add a length check/assert on raw_39
before slicing to ensure it is 39 bytes (or handle unexpected lengths with a
clear error) so future maintainers understand the assumption and you avoid
panics on malformed input.
- Around line 529-538: The request to HC_AUTH_URL uses reqwest::Client::new()
without a timeout, so create the client with a timeout (e.g., via
reqwest::Client::builder().timeout(Duration::from_secs(X)).build() or set a
per-request timeout on the RequestBuilder) and replace the current Client::new()
usage; update the code that constructs now_url/get() (the variables now_url,
client and payload_b64url) to use the timed client (and add use
std::time::Duration) so the .send().await won't block indefinitely if the
hc-auth server is slow or unresponsive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01a465eb-0679-431e-aadb-13bbae244a8b
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockbootstrap-languages/agent-language/hc-dna/Cargo.lockis excluded by!**/*.lockbootstrap-languages/direct-message-language/hc-dna/Cargo.lockis excluded by!**/*.lockbootstrap-languages/file-storage/hc-dna/Cargo.lockis excluded by!**/*.lockbootstrap-languages/p-diff-sync/hc-dna/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.gitignoreCargo.tomlbootstrap-languages/agent-language/hc-dna/tests/sweettest/Cargo.tomlbootstrap-languages/agent-language/hc-dna/zomes/agent_store/Cargo.tomlbootstrap-languages/agent-language/hc-dna/zomes/agent_store_integrity/Cargo.tomlbootstrap-languages/direct-message-language/hc-dna/tests/sweettest/Cargo.tomlbootstrap-languages/direct-message-language/hc-dna/zomes/direct-message-integrity/Cargo.tomlbootstrap-languages/direct-message-language/hc-dna/zomes/direct-message/Cargo.tomlbootstrap-languages/file-storage/hc-dna/tests/sweettest/Cargo.tomlbootstrap-languages/file-storage/hc-dna/zomes/file_storage/Cargo.tomlbootstrap-languages/file-storage/hc-dna/zomes/integrity/Cargo.tomlbootstrap-languages/p-diff-sync/hc-dna/tests/sweettest/Cargo.tomlbootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/Cargo.tomlbootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync_integrity/Cargo.tomlrust-executor/Cargo.tomlrust-executor/build.rsrust-executor/src/holochain_service/interface.rsrust-executor/src/holochain_service/mod.rsrust-executor/src/resources/alliance_0.61.0.dnarust-executor/src/unyt_service.rs
Comment on lines
+29
to
+38
| let out_dir = std::env::var("OUT_DIR").unwrap(); | ||
| let dest = Path::new(&out_dir).join("alliance.dna"); | ||
|
|
||
| // Skip download if the file already exists (cached across incremental builds). | ||
| if dest.exists() { | ||
| let metadata = std::fs::metadata(&dest).unwrap(); | ||
| if metadata.len() > 0 { | ||
| return; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Don't treat any non-empty cached DNA as valid.
Line 33 caches forever on file size alone. After a future ALLIANCE_DNA_VERSION bump, or after a failed curl that left partial bytes behind, the build can silently reuse stale/corrupt DNA and still compile. Cache against the expected version and only mark the artifact valid after a successful download.
💡 Example using a version stamp
let out_dir = std::env::var("OUT_DIR").unwrap();
let dest = Path::new(&out_dir).join("alliance.dna");
+ let version_stamp = Path::new(&out_dir).join("alliance.dna.version");
- // Skip download if the file already exists (cached across incremental builds).
- if dest.exists() {
- let metadata = std::fs::metadata(&dest).unwrap();
- if metadata.len() > 0 {
- return;
- }
- }
+ if dest.exists()
+ && std::fs::read_to_string(&version_stamp).ok().as_deref() == Some(ALLIANCE_DNA_VERSION)
+ {
+ return;
+ }
println!(
"cargo:warning=Downloaded alliance DNA v{} ({} bytes)",
ALLIANCE_DNA_VERSION, size
);
+ std::fs::write(&version_stamp, ALLIANCE_DNA_VERSION).unwrap();Also applies to: 64-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-executor/build.rs` around lines 29 - 38, The build script currently
treats any non-empty dest ("alliance.dna") as valid; change this to cache by
version/stamp instead: use the ALLIANCE_DNA_VERSION constant to check a small
companion file (e.g., "alliance.dna.version") in OUT_DIR and only accept the
cached DNA if that file exists and its contents equal the expected version; when
downloading, write the new DNA to a temporary file and atomically rename it to
dest, then write the version file (or checksum) after a successful download so
partial/failed downloads or version bumps won't be reused; apply the same
pattern to the other identical block referenced (lines 64-68).
Comment on lines
+45
to
+64
| let status = Command::new("curl") | ||
| .args([ | ||
| "-L", | ||
| "--fail", | ||
| "--silent", | ||
| "--show-error", | ||
| "-o", | ||
| dest.to_str().unwrap(), | ||
| ALLIANCE_DNA_URL, | ||
| ]) | ||
| .status() | ||
| .expect("failed to run curl – is it installed?"); | ||
|
|
||
| assert!( | ||
| status.success(), | ||
| "Failed to download alliance DNA from {}", | ||
| ALLIANCE_DNA_URL | ||
| ); | ||
|
|
||
| let size = std::fs::metadata(&dest).unwrap().len(); |
Contributor
There was a problem hiding this comment.
Pin and verify the release asset digest before embedding it.
This build trusts whatever bytes GitHub serves for alliance.dna. A replaced or corrupted release asset would be bundled silently, which is a supply-chain risk for a consensus/runtime artifact. Please pin a SHA-256 (or signature) in the repo and fail the build on mismatch before include_bytes! consumes the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-executor/build.rs` around lines 45 - 64, Add a hardcoded SHA-256
checksum (or public-key signature verification) for the alliance release asset
and verify it immediately after downloading and before any include_bytes! usage:
after the curl Command that fetches ALLIANCE_DNA_URL to dest, compute the
downloaded file's SHA-256 (e.g., via the sha2 crate or a system tool) and
compare it to the pinned checksum constant in the repo, and call panic! or
assert! (failing the build) if it doesn't match; alternatively implement
signature verification using a stored public key instead of checksum. Ensure
references to ALLIANCE_DNA_URL, dest, and the include_bytes! consumer are
updated to reflect this verification step so the build never embeds unverified
bytes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/src/components/Hosting.tsx (2)
716-734:⚠️ Potential issue | 🟡 MinorTLS file picker changes not persisted to backend.
The certificate and key file pickers (lines 722, 732) only call
setTlsConfigto update local state, without callinghandleTlsConfigChangeto persist the changes. This is inconsistent with the TLS enable toggle (lines 1916-1922) which persists immediately.If the user selects files but doesn't toggle the TLS switch, the paths will be lost on refresh.
Proposed fix: persist after file selection
const handleCertFilePicker = async () => { const filePath = await dialogOpen({ title: "Select TLS Certificate File", filters: [{ name: "Certificate", extensions: ["pem", "crt", "cert"] }], }); if (filePath && tlsConfig) { - setTlsConfig({ ...tlsConfig, cert_file_path: filePath.toString() }); + const newConfig = { ...tlsConfig, cert_file_path: filePath.toString() }; + handleTlsConfigChange(newConfig); } };Apply similarly to
handleKeyFilePicker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/Hosting.tsx` around lines 716 - 734, The file pickers update local state via setTlsConfig but don't persist to the backend; modify handleCertFilePicker and handleKeyFilePicker so after computing the new config (using tlsConfig and the selected filePath) you both call setTlsConfig(...) and then call handleTlsConfigChange(newTlsConfig) (or await it if it returns a promise) to persist the updated cert_file_path/key_file_path to the backend, ensuring you reference handleCertFilePicker, handleKeyFilePicker, setTlsConfig, handleTlsConfigChange and tlsConfig when making the change.
2122-2153:⚠️ Potential issue | 🟠 MajorBug: SMTP test UI disappears before user can see the result.
The
smtpTestingstate controls both the visibility of the test UI (line 2129) and the loading state of the Send button (line 2142). WhenhandleSmtpTestcompletes and setssmtpTesting(false), the entire test section disappears, hiding thesmtpTestStatusmessage from the user.Additionally, when clicking "Send Test" (line 2122), the loading spinner shows immediately before the user can even enter an email address.
Proposed fix: separate visibility and loading state
- const [smtpTesting, setSmtpTesting] = useState(false); + const [smtpTestVisible, setSmtpTestVisible] = useState(false); + const [smtpTestLoading, setSmtpTestLoading] = useState(false);Then update the handlers and JSX accordingly:
- Line 683:
setSmtpTestLoading(true)- Line 694:
setSmtpTestLoading(false)- Line 2122:
setSmtpTestVisible(true)- Line 2129:
{smtpTestVisible && ...}- Line 2142:
loading={smtpTestLoading}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/Hosting.tsx` around lines 2122 - 2153, The SMTP test UI currently uses smtpTesting for both visibility and button-loading, which causes the UI to disappear (hiding smtpTestStatus) and shows the spinner too early; introduce two separate states (e.g., smtpTestVisible and smtpTestLoading), replace all visibility checks of smtpTesting in the JSX with smtpTestVisible (the block that renders the test input and status), update the Send Test click to call setSmtpTestVisible(true) instead of setSmtpTesting(true), update handleSmtpTest to use setSmtpTestLoading(true) at start and setSmtpTestLoading(false) on completion (but leave smtpTestVisible true so smtpTestStatus remains visible), and change the Send button prop loading={...} to use smtpTestLoading; keep references to smtpTestEmail and smtpTestStatus and update any setSmtpTesting usages to the new state names accordingly.
🧹 Nitpick comments (1)
ui/src/components/Hosting.tsx (1)
26-34: PriceInput won't resync after receiving first truthy initialValue.The
initializedref prevents the component from ever updatinglocalfrominitialValueafter the first truthy value is received. If the user logs out/in or rates are refetched from the server with different values, the inputs may show stale data unless the component is unmounted and remounted.Consider resetting
initialized.current = falsewhen a key prop (like a session identifier) changes, or use a different approach to detect "user is currently editing".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/Hosting.tsx` around lines 26 - 34, The useEffect that syncs initialValue to local uses the initialized ref (initialized) which is set true on first truthy initialValue and prevents any future updates; change this by either (A) resetting initialized.current = false when a higher-level key/identifier prop (e.g., sessionId or similar prop passed into the component) changes so subsequent initialValue updates are applied, or (B) replace the initialized flag with an explicit isEditing boolean (track focus via onFocus/onBlur and set isEditing accordingly) and only call setLocal(String(initialValue)) when isEditing is false; update references to initialized, setLocal, initialValue, and the existing useEffect to implement one of these approaches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-executor/src/unyt_service.rs`:
- Around line 151-154: The post-install bootstrap (setting up unyt_auth_material
and applying the unyt_dna_hash space override) is only executed in the
ensure_installed() path via setup_bootstrap_auth(); factor that into a shared
helper (e.g., post_install_bootstrap() or run_post_install_steps()) that
performs the same actions as setup_bootstrap_auth() and any conductor
restart/space-override logic, then invoke this helper from both
ensure_installed() and reinstall() immediately after do_install() and before
returning and before toggling INSTALL_ONCE so reinstall() also populates
unyt_auth_material and applies the unyt_dna_hash override.
- Around line 520-523: The current flow calls get_or_create_agent_key() after
installing the app which can produce a different key than the one assigned to
the installed cell; change the flow to persist and reuse the actual installed
agent key instead: either pre-generate and persist the key by calling
get_or_create_agent_key() before calling install_app()/do_install() and pass
that key into install_app(), or after install_app()/do_install() read the
assigned agent key out of the returned app_info (use the app_info cell/agent
pubkey field) and store that value into unyt_agent_key and use it when building
the hc-auth material; update the code paths that construct hc-auth to reference
the persisted app_info agent key rather than calling get_or_create_agent_key()
post-install.
- Around line 585-597: The early return in setup_bootstrap_auth uses only DB
keys (has_auth and has_dna_hash for unyt_auth_material and unyt_dna_hash) to
assume the space override is active; change this so you do not skip setup purely
because those keys exist. Instead, verify the override is actually applied
(e.g., add or call an is_override_active() check against the running
conductor/space config or a DB flag like unyt_override_applied) and only return
when both the stored auth/hash exist AND the override is confirmed active; if
the override is not active, proceed to apply the override and restart, and
ensure retries after restart failures do not short-circuit due to the presence
of unyt_auth_material/unyt_dna_hash.
In `@ui/src/components/Hosting.tsx`:
- Around line 327-330: The variable `detail` is built from `data.detail` but
never used in the thrown error; either remove the unused `detail` variable or
include it in the Error message constructed where `throw new Error(...)` is
called (i.e., append the `detail` string to the chosen error text derived from
`data.error` or the fallback `Failed to get membrane proof (${res.status})`),
referencing `detail`, `data`, `res`, and the throw site in this file
(Hosting.tsx) to locate and update the code.
- Around line 1489-1491: The render is creating a blob URL each render by
calling URL.createObjectURL(profilePic) directly; update the Hosting component
to derive and manage a stable profilePicUrl via useMemo or useEffect so the blob
URL is created only when profilePic changes and revoked when no longer needed.
Specifically, create a profilePicUrl state or memo inside Hosting (e.g., const
profilePicUrl = useMemo(...) or useEffect that sets state), use
URL.createObjectURL(profilePic) only once when profilePic changes, and call
URL.revokeObjectURL(oldUrl) in the cleanup to prevent leaks; then replace the
inline URL.createObjectURL(profilePic) in the JSX img src with profilePicUrl.
---
Outside diff comments:
In `@ui/src/components/Hosting.tsx`:
- Around line 716-734: The file pickers update local state via setTlsConfig but
don't persist to the backend; modify handleCertFilePicker and
handleKeyFilePicker so after computing the new config (using tlsConfig and the
selected filePath) you both call setTlsConfig(...) and then call
handleTlsConfigChange(newTlsConfig) (or await it if it returns a promise) to
persist the updated cert_file_path/key_file_path to the backend, ensuring you
reference handleCertFilePicker, handleKeyFilePicker, setTlsConfig,
handleTlsConfigChange and tlsConfig when making the change.
- Around line 2122-2153: The SMTP test UI currently uses smtpTesting for both
visibility and button-loading, which causes the UI to disappear (hiding
smtpTestStatus) and shows the spinner too early; introduce two separate states
(e.g., smtpTestVisible and smtpTestLoading), replace all visibility checks of
smtpTesting in the JSX with smtpTestVisible (the block that renders the test
input and status), update the Send Test click to call setSmtpTestVisible(true)
instead of setSmtpTesting(true), update handleSmtpTest to use
setSmtpTestLoading(true) at start and setSmtpTestLoading(false) on completion
(but leave smtpTestVisible true so smtpTestStatus remains visible), and change
the Send button prop loading={...} to use smtpTestLoading; keep references to
smtpTestEmail and smtpTestStatus and update any setSmtpTesting usages to the new
state names accordingly.
---
Nitpick comments:
In `@ui/src/components/Hosting.tsx`:
- Around line 26-34: The useEffect that syncs initialValue to local uses the
initialized ref (initialized) which is set true on first truthy initialValue and
prevents any future updates; change this by either (A) resetting
initialized.current = false when a higher-level key/identifier prop (e.g.,
sessionId or similar prop passed into the component) changes so subsequent
initialValue updates are applied, or (B) replace the initialized flag with an
explicit isEditing boolean (track focus via onFocus/onBlur and set isEditing
accordingly) and only call setLocal(String(initialValue)) when isEditing is
false; update references to initialized, setLocal, initialValue, and the
existing useEffect to implement one of these approaches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 433d2420-5e6b-4dd5-84f3-a20847b70c8d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rust-executor/build.rsrust-executor/src/unyt_service.rsui/src/components/Hosting.tsx
Comment on lines
+585
to
+597
| async fn setup_bootstrap_auth() { | ||
| // Skip if auth material is already stored (previous run already set it up) | ||
| let has_auth = Ad4mDb::with_global_instance(|db| db.get_setting("unyt_auth_material")) | ||
| .unwrap_or(None) | ||
| .is_some(); | ||
| let has_dna_hash = Ad4mDb::with_global_instance(|db| db.get_setting("unyt_dna_hash")) | ||
| .unwrap_or(None) | ||
| .is_some(); | ||
|
|
||
| if has_auth && has_dna_hash { | ||
| info!("Bootstrap auth material and DNA hash already exist, skipping setup"); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't treat stored auth/hash as proof that the override is already active.
This early return fires whenever an existing unyt_auth_material is reused and capture_dna_hash() has already persisted unyt_dna_hash earlier in the install flow. In that case the running conductor still hasn't applied the new space override, but this function exits before the restart. The same guard also suppresses later retries after a failed restart because both DB keys remain populated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-executor/src/unyt_service.rs` around lines 585 - 597, The early return
in setup_bootstrap_auth uses only DB keys (has_auth and has_dna_hash for
unyt_auth_material and unyt_dna_hash) to assume the space override is active;
change this so you do not skip setup purely because those keys exist. Instead,
verify the override is actually applied (e.g., add or call an
is_override_active() check against the running conductor/space config or a DB
flag like unyt_override_applied) and only return when both the stored auth/hash
exist AND the override is confirmed active; if the override is not active,
proceed to apply the override and restart, and ensure retries after restart
failures do not short-circuit due to the presence of
unyt_auth_material/unyt_dna_hash.
unyt_service.rs - Agent key from app_info: do_install() now stores the actual installed agent key, preventing mismatch with create_auth_material() - reinstall() bootstrap: Now calls setup_bootstrap_auth() after do_install(), matching ensure_installed() behavior - HoloHash length assert: Added assert + layout comment before slicing raw_39[3..35] - reqwest timeout: 10s timeout on the hc-auth HTTP client Hosting.tsx - Unused detail: Now appended to the thrown error message - Blob URL leak: profilePicUrl via useMemo instead of inline createObjectURL per render - TLS file pickers: Now call handleTlsConfigChange() to persist selection to backend - SMTP test state: Split smtpTesting into smtpTestVisible (panel visibility) and smtpTestLoading (button spinner)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/components/Hosting.tsx (1)
256-264:⚠️ Potential issue | 🔴 CriticalDistinguish transient failures from token expiration in
fetchHostData.The function currently returns
"expired"for both 401 responses and any exception/non-401 error response. This causes transient failures (network errors, 5xx responses) to incorrectly clear valid saved sessions, and in thehandleLogin()flow at line 553–577, produces an incorrect "Session expired" message after a fresh session was just saved.Only a 401 status should indicate an expired token. Add an
"unavailable"return type to distinguish transient failures and handle them appropriately without clearing the session.Suggested approach
- }): Promise<"verified" | "unverified" | "expired"> => { + }): Promise<"verified" | "unverified" | "expired" | "unavailable"> => { @@ - return "expired"; + return "unavailable"; @@ - } else { + } else if (result === "expired") { + await saveHostSession(null); + setHostData(null); setHostRegStatus({ type: "error", message: "Session expired. Please log in again.", }); setRegStep("credentials"); + } else { + setHostRegStatus({ + type: "error", + message: "Could not load host profile. Please try again.", + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/Hosting.tsx` around lines 256 - 264, fetchHostData currently returns "expired" for 401 and all other failures; change it so only a 401 response returns "expired", and any exception or non-401 error returns "unavailable" instead; update the catch block and non-401 response handling in fetchHostData to return "unavailable" (include logging) and leave session-clearing logic out of this function. Then update the caller flow in handleLogin to treat "unavailable" as a transient/network failure (do not clear the saved session or show "Session expired") while keeping the existing behavior for "expired" tokens. Reference: function fetchHostData and its caller handleLogin to locate the changes.
♻️ Duplicate comments (3)
rust-executor/src/unyt_service.rs (2)
186-228:⚠️ Potential issue | 🟠 MajorPersist
app_info.agent_pub_keyon the existing-app path as well.
setup_bootstrap_auth()now runs after any successfulinstall_alliance_dna(), but these early-return branches never backfillunyt_agent_key. On an upgrade from an older install,create_auth_material()will mint a fresh keypair and sign hc-auth with the wrong agent identity.Suggested fix
if let Ok(Some(app_info)) = hc.get_app_info(UNYT_APP_ID.to_string()).await { + let installed_key_str = app_info.agent_pub_key.to_string(); + if let Err(e) = + Ad4mDb::with_global_instance(|db| db.set_setting("unyt_agent_key", &installed_key_str)) + { + warn!("Failed to store installed agent key: {}", e); + } + let installed_version = Ad4mDb::with_global_instance(|db| db.get_setting("unyt_dna_version")).unwrap_or(None);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/unyt_service.rs` around lines 186 - 228, When handling the "existing app" branch in install_alliance_dna (the block that runs after hc.get_app_info(UNYT_APP_ID...) returns Some), persist app_info.agent_pub_key into the settings so setup_bootstrap_auth/create_auth_material sees the correct agent identity; specifically, before each early return (both the re-enable success path after hc.enable_app(...) Ok and the paths after capture_dna_hash().await), write the agent key into your settings store (use the same mechanism as reading installed_version — Ad4mDb::with_global_instance(|db| db.set_setting("unyt_agent_key", ...)) or the project’s equivalent) so unyt_agent_key is backfilled for upgrades and subsequent setup_bootstrap_auth calls.
627-638:⚠️ Potential issue | 🟠 MajorDon't let persisted DB keys suppress a needed override retry.
A failed restart leaves both settings populated, so this guard prevents every later attempt from applying the space override. The skip condition needs to be based on successful activation, not just on
unyt_auth_material/unyt_dna_hashexisting in the DB.ui/src/components/Hosting.tsx (1)
135-138:⚠️ Potential issue | 🟠 MajorThe blob-preview URL still leaks.
useMemo()stops recreating the URL on every render, but each newprofilePicstill allocates a new blob URL and nothing ever revokes it on change/unmount.Possible fix
- const profilePicUrl = React.useMemo(() => { - if (profilePic) return URL.createObjectURL(profilePic); - return null; - }, [profilePic]); + const [profilePicUrl, setProfilePicUrl] = React.useState<string | null>(null); + + React.useEffect(() => { + if (!profilePic) { + setProfilePicUrl(null); + return; + } + + const objectUrl = URL.createObjectURL(profilePic); + setProfilePicUrl(objectUrl); + + return () => URL.revokeObjectURL(objectUrl); + }, [profilePic]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/Hosting.tsx` around lines 135 - 138, The blob URL created by URL.createObjectURL in the profilePicUrl useMemo leaks because it is never revoked; replace the current pattern by creating the object URL in an effect and revoking it on dependency change/unmount (or add a cleanup effect that revokes the previous URL): use profilePic as the dependency, call URL.createObjectURL(profilePic) to set profilePicUrl inside a useEffect (or keep useMemo but store the result in state), and in the effect cleanup call URL.revokeObjectURL(previousUrl) so any old blob URLs are revoked when profilePic changes or the component unmounts; reference the existing symbols profilePicUrl, profilePic, URL.createObjectURL and URL.revokeObjectURL when making the change.
🧹 Nitpick comments (1)
rust-executor/src/unyt_service.rs (1)
29-32: Keep the downloaded DNA bytes and the runtime version marker on one source of truth.This file hard-codes
ALLIANCE_DNA_VERSIONwhile the DNA artifact is loaded fromOUT_DIR/alliance.dna. Although both currently reference "0.61.0", they are separate declarations that can drift. Thebuild.rsscript should export the version via Cargo'srustc-envmechanism (e.g.,println!("cargo:rustc-env=ALLIANCE_DNA_VERSION=...");), and this file should consume it viaenv!("ALLIANCE_DNA_VERSION")instead of duplicating the constant. This ensures version checks inversion_info()and theunyt_dna_versionDB marker stay synchronized with the actually installed artifact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/unyt_service.rs` around lines 29 - 32, Replace the hard-coded ALLIANCE_DNA_VERSION constant with the value injected at compile time by build.rs (use env!("ALLIANCE_DNA_VERSION")) so the runtime version marker comes from the same source as the downloaded DNA; leave the include_bytes!(concat!(env!("OUT_DIR"), "/alliance.dna")) for ALLIANCE_DNA_BYTES as-is, update any references to ALLIANCE_DNA_VERSION (e.g., in version_info() and the unyt_dna_version DB marker) to use the env! value, and ensure build.rs emits the environment variable via println!("cargo:rustc-env=ALLIANCE_DNA_VERSION=...") so both artifact and version are a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-executor/src/holochain_service/mod.rs`:
- Around line 526-545: The unyt space override is currently setting
SpaceNetworkOverride { base64_auth_material: None } even though
setup_bootstrap_auth() persists unyt_auth_material; change the code that builds
the override in the block that reads get_setting("unyt_dna_hash") to also read
get_setting("unyt_auth_material") (using
crate::db::Ad4mDb::with_global_instance) and, if present, set
SpaceNetworkOverride.base64_auth_material = Some(the_persisted_string) instead
of None (keep the existing Url2 parse lines and only conditionally include the
auth material when the DB returns a value).
- Around line 418-424: The SignWithKey branch is calling keystore.sign()
directly which can block the serial receiver loop; wrap the
keystore.sign(agent_key, data_arc).await call in the same tokio timeout guard
used for other keystore calls (i.e., call tokio::time::timeout(<same_duration>,
keystore.sign(...)).await and map the timeout error into an anyhow error) so
that HolochainServiceRequest::SignWithKey in the receiver loop (using
service.conductor.keystore(), data_arc and response_tx) returns an error on
timeout instead of stalling the loop.
In `@ui/src/components/Hosting.tsx`:
- Around line 1983-1991: The TLS port input only updates local state via
setTlsConfig and doesn't persist or apply changes because it never calls
handleTlsConfigChange; update the onInput handler for the <j-input> (the
tls_port branch that currently uses tlsConfig and setTlsConfig) to compute the
new tlsConfig (with tls_port = parseInt(e.target.value) || 12001), call
setTlsConfig(newConfig) and then call handleTlsConfigChange(newConfig) so the
change is persisted/applied the same way as the toggle and file pickers.
- Around line 624-635: The UI marks the update as successful even if executor
sync fails: when calling client.runtime.setHostRates(hostReg.rates) after
setHostData(...), catch failures and change the final status accordingly instead
of always calling setHostRegStatus({type:"success",...}); specifically, ensure
that in the block that currently invokes client.runtime.setHostRates you treat
setHostRates errors as a failed/partial result (e.g.,
setHostRegStatus({type:"error", message: "Host updated but failed to sync rates
to executor: <error message>"})) or only set success when both res.ok and
client.runtime.setHostRates complete without throwing; use the same
hostReg.rates and include the thrown error text in the status so the UI reflects
the executor-side failure (reference functions/variables:
client.runtime.setHostRates, setHostData, setHostRegStatus, hostReg.rates).
- Around line 1765-1779: The filter step drops legacy entries like "ModelName
(per token)"; before you filter by validKeys you should detect and migrate those
legacy descriptions to the new plain model name so custom prices are preserved.
Specifically, in the block using parsed, modelNames, validKeys and has, scan
parsed for descriptions matching the legacy pattern (e.g., /^(.+)\s*\(per
token\)$/), for each match where the extracted model name exists in aiModels
transfer the entry's priceInHOT to a parsed entry with description equal to the
plain model name (creating it only if one does not already exist) and then
remove the legacy entry; only afterwards apply the validKeys filter and the code
that injects DEFAULT_TOKEN_PRICE/DEFAULT_LINK_PRICE so existing custom prices
are retained.
- Around line 237-253: The updater for host registration wrongly treats empty
strings/arrays as missing because it uses || and length > 0 checks; modify the
setHostReg update to use explicit undefined/null checks: for scalar fields
(name, description, location, hostUrl, computeSpecs) use conditional expressions
like (mine.description !== undefined ? mine.description : prev.description) so
empty strings are preserved, and for array fields (rates, aiModels) use
Array.isArray(mine.rates) ? JSON.stringify(mine.rates) : prev.rates (and same
for aiModels) so empty arrays round-trip correctly; keep session.indexUrl
assignment as-is.
- Around line 26-34: The PriceInput currently uses initialized (React.useRef)
and truthiness checks so it ignores legitimate zero values and later external
updates (from commitPrice, pricing sync, or hostReg.rates) once initialized;
replace the one-shot initialized logic with an editing-aware sync: track editing
state with an isEditing ref (set on input focus/blur or on change/start-edit vs
commit), and in the useEffect watch initialValue and when isEditing.current is
false AND initialValue !== null/undefined update local via
setLocal(String(initialValue)) (use explicit null/undefined checks so zero
becomes "0"); ensure the input focus/blur handlers toggle isEditing and that
commitPrice still writes back to hostReg.rates which will trigger the effect to
update the displayed value.
---
Outside diff comments:
In `@ui/src/components/Hosting.tsx`:
- Around line 256-264: fetchHostData currently returns "expired" for 401 and all
other failures; change it so only a 401 response returns "expired", and any
exception or non-401 error returns "unavailable" instead; update the catch block
and non-401 response handling in fetchHostData to return "unavailable" (include
logging) and leave session-clearing logic out of this function. Then update the
caller flow in handleLogin to treat "unavailable" as a transient/network failure
(do not clear the saved session or show "Session expired") while keeping the
existing behavior for "expired" tokens. Reference: function fetchHostData and
its caller handleLogin to locate the changes.
---
Duplicate comments:
In `@rust-executor/src/unyt_service.rs`:
- Around line 186-228: When handling the "existing app" branch in
install_alliance_dna (the block that runs after hc.get_app_info(UNYT_APP_ID...)
returns Some), persist app_info.agent_pub_key into the settings so
setup_bootstrap_auth/create_auth_material sees the correct agent identity;
specifically, before each early return (both the re-enable success path after
hc.enable_app(...) Ok and the paths after capture_dna_hash().await), write the
agent key into your settings store (use the same mechanism as reading
installed_version — Ad4mDb::with_global_instance(|db|
db.set_setting("unyt_agent_key", ...)) or the project’s equivalent) so
unyt_agent_key is backfilled for upgrades and subsequent setup_bootstrap_auth
calls.
In `@ui/src/components/Hosting.tsx`:
- Around line 135-138: The blob URL created by URL.createObjectURL in the
profilePicUrl useMemo leaks because it is never revoked; replace the current
pattern by creating the object URL in an effect and revoking it on dependency
change/unmount (or add a cleanup effect that revokes the previous URL): use
profilePic as the dependency, call URL.createObjectURL(profilePic) to set
profilePicUrl inside a useEffect (or keep useMemo but store the result in
state), and in the effect cleanup call URL.revokeObjectURL(previousUrl) so any
old blob URLs are revoked when profilePic changes or the component unmounts;
reference the existing symbols profilePicUrl, profilePic, URL.createObjectURL
and URL.revokeObjectURL when making the change.
---
Nitpick comments:
In `@rust-executor/src/unyt_service.rs`:
- Around line 29-32: Replace the hard-coded ALLIANCE_DNA_VERSION constant with
the value injected at compile time by build.rs (use
env!("ALLIANCE_DNA_VERSION")) so the runtime version marker comes from the same
source as the downloaded DNA; leave the include_bytes!(concat!(env!("OUT_DIR"),
"/alliance.dna")) for ALLIANCE_DNA_BYTES as-is, update any references to
ALLIANCE_DNA_VERSION (e.g., in version_info() and the unyt_dna_version DB
marker) to use the env! value, and ensure build.rs emits the environment
variable via println!("cargo:rustc-env=ALLIANCE_DNA_VERSION=...") so both
artifact and version are a single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 721ccd87-26aa-4df1-892b-2afaab416c2a
📒 Files selected for processing (5)
rust-executor/build.rsrust-executor/src/graphql/mutation_resolvers.rsrust-executor/src/holochain_service/mod.rsrust-executor/src/unyt_service.rsui/src/components/Hosting.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- rust-executor/build.rs
Comment on lines
+418
to
+424
| HolochainServiceRequest::SignWithKey(agent_key, data, response_tx) => { | ||
| let keystore = service.conductor.keystore(); | ||
| let data_arc = Arc::from(data.into_boxed_slice()); | ||
| let result = keystore.sign(agent_key, data_arc).await | ||
| .map_err(|e| anyhow!("Failed to sign with key: {}", e)); | ||
| let _ = response_tx.send(HolochainServiceResponse::SignWithKey(result)); | ||
| } |
Contributor
There was a problem hiding this comment.
Put SignWithKey under the same timeout guard as the other keystore calls.
This receiver loop is serial. If keystore.sign() stalls here, every later Holochain request — including shutdown/restart — is blocked behind it.
Suggested fix
HolochainServiceRequest::SignWithKey(agent_key, data, response_tx) => {
- let keystore = service.conductor.keystore();
- let data_arc = Arc::from(data.into_boxed_slice());
- let result = keystore.sign(agent_key, data_arc).await
- .map_err(|e| anyhow!("Failed to sign with key: {}", e));
- let _ = response_tx.send(HolochainServiceResponse::SignWithKey(result));
+ match timeout(
+ std::time::Duration::from_secs(3),
+ async {
+ let keystore = service.conductor.keystore();
+ let data_arc = Arc::from(data.into_boxed_slice());
+ keystore
+ .sign(agent_key, data_arc)
+ .await
+ .map_err(|e| anyhow!("Failed to sign with key: {}", e))
+ }
+ ).await.map_err(|_| anyhow!("Timeout error; SignWithKey")) {
+ Ok(result) => {
+ let _ = response_tx.send(HolochainServiceResponse::SignWithKey(result));
+ }
+ Err(err) => {
+ let _ = response_tx.send(HolochainServiceResponse::SignWithKey(Err(err)));
+ }
+ }
}📝 Committable suggestion
‼️ 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.
Suggested change
| HolochainServiceRequest::SignWithKey(agent_key, data, response_tx) => { | |
| let keystore = service.conductor.keystore(); | |
| let data_arc = Arc::from(data.into_boxed_slice()); | |
| let result = keystore.sign(agent_key, data_arc).await | |
| .map_err(|e| anyhow!("Failed to sign with key: {}", e)); | |
| let _ = response_tx.send(HolochainServiceResponse::SignWithKey(result)); | |
| } | |
| HolochainServiceRequest::SignWithKey(agent_key, data, response_tx) => { | |
| match timeout( | |
| std::time::Duration::from_secs(3), | |
| async { | |
| let keystore = service.conductor.keystore(); | |
| let data_arc = Arc::from(data.into_boxed_slice()); | |
| keystore | |
| .sign(agent_key, data_arc) | |
| .await | |
| .map_err(|e| anyhow!("Failed to sign with key: {}", e)) | |
| } | |
| ).await.map_err(|_| anyhow!("Timeout error; SignWithKey")) { | |
| Ok(result) => { | |
| let _ = response_tx.send(HolochainServiceResponse::SignWithKey(result)); | |
| } | |
| Err(err) => { | |
| let _ = response_tx.send(HolochainServiceResponse::SignWithKey(Err(err))); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-executor/src/holochain_service/mod.rs` around lines 418 - 424, The
SignWithKey branch is calling keystore.sign() directly which can block the
serial receiver loop; wrap the keystore.sign(agent_key, data_arc).await call in
the same tokio timeout guard used for other keystore calls (i.e., call
tokio::time::timeout(<same_duration>, keystore.sign(...)).await and map the
timeout error into an anyhow error) so that HolochainServiceRequest::SignWithKey
in the receiver loop (using service.conductor.keystore(), data_arc and
response_tx) returns an error on timeout instead of stalling the loop.
…'t initialize the DB
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
rust-executor/build.rs (2)
51-68:⚠️ Potential issue | 🟠 MajorVerify the downloaded release asset before embedding it.
rust-executor/src/unyt_service.rs:30-34pulls this file into the binary withinclude_bytes!, so a replaced or corrupted GitHub asset becomes part of the build without any integrity check. Please pin a SHA-256 (or signature) in the repo and fail here on mismatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/build.rs` around lines 51 - 68, The build step currently downloads ALLIANCE_DNA_URL via Command::new("curl") and then the file is embedded via include_bytes! in rust-executor/src/unyt_service.rs; add integrity verification by storing a pinned SHA-256 hash (or signature) in the repo and after the curl download (the dest path used in Command::new("curl") and ALLIANCE_DNA_URL), compute the downloaded file’s SHA-256 and compare it to the pinned value, failing the build with a clear error if they differ; update build.rs to perform this check (compute hash of dest, compare to pinned SHA-256 string) and ensure the error message references ALLIANCE_DNA_URL and dest so authors can diagnose a mismatch.
38-44:⚠️ Potential issue | 🟠 MajorDon't treat any non-empty cached DNA as valid.
If
ALLIANCE_DNA_VERSIONchanges, Cargo rerunsbuild.rs, but this early return still reuses whatever bytes are already inOUT_DIR/alliance.dna. The same check also accepts a partial file from a failed download, so the next build can silently embed stale or corrupt DNA. Cache against a version/checksum stamp and replace the file atomically after a successful download.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/build.rs` around lines 38 - 44, The build script currently treats any non-empty OUT_DIR/alliance.dna (dest) as valid which can reuse stale or partial downloads when ALLIANCE_DNA_VERSION changes or a previous download failed; update build.rs to cache by a version/checksum stamp instead of file size: compute the expected stamp from ALLIANCE_DNA_VERSION (or a downloaded checksum), store that stamp alongside the downloaded file (or include the version in the filename), and only early-return when the stored stamp matches the current ALLIANCE_DNA_VERSION; when downloading, write to a temporary file first and atomically rename into dest on success to avoid partial files being treated as valid (use dest.tmp -> std::fs::rename), and ensure any previous mismatched dest is replaced.
🧹 Nitpick comments (2)
ui/src/components/Hosting.tsx (1)
1819-1824: Consider: Zero price validation may prevent free service offerings.The validation
num > 0(line 1819) rejects zero values and resets to default. This means hosts cannot offer a model for free (0 HOT price). If this is intentional business logic, consider adding a brief comment; otherwise, you may want to allownum >= 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/Hosting.tsx` around lines 1819 - 1824, The current validation in Hosting.tsx uses `num > 0` which prevents zero prices and forces getDefault(desc); update the check to allow free offerings by changing the condition to `num >= 0` (i.e., push { description: desc, priceInHOT: num } when !isNaN(num) && num >= 0) or, if zero should indeed be disallowed, add an explicit comment explaining the business rule; ensure you reference the same variables (`num`, `desc`, `priceInHOT`, `getDefault`) and the `updated.push(...)` logic when making the change.rust-executor/build.rs (1)
4-6: MakeALLIANCE_DNA_VERSIONthe single source of truth.
ALLIANCE_DNA_URLduplicates the version string, so a one-sided edit can make the env/log say one version while the build downloads another asset. Compose the URL fromALLIANCE_DNA_VERSIONinstead of hardcoding both values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/build.rs` around lines 4 - 6, ALLIANCE_DNA_URL currently hardcodes the version; change it to be derived from the single source ALLIANCE_DNA_VERSION so edits don’t diverge. Replace the hardcoded ALLIANCE_DNA_URL with code that composes the URL from ALLIANCE_DNA_VERSION (e.g., a function like fn alliance_dna_url() -> String or a lazy/static string that uses format! to produce "https://github.com/unytco/unyt-sandbox/releases/download/v{ALLIANCE_DNA_VERSION}/alliance.dna"), and update any usage sites to call that function or use the composed value so the version is only defined in ALLIANCE_DNA_VERSION.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-executor/build.rs`:
- Around line 51-61: The curl invocation in build.rs (Command::new("curl") that
downloads ALLIANCE_DNA_URL to dest) lacks timeouts and retries; modify the logic
to retry the download a few times with exponential/backoff and add curl flags
like "--connect-timeout" and "--max-time" (and "--retry" / "--retry-delay" /
"--retry-connrefused") to bound connect/overall time, and wrap the
Command::new("curl") call in a short retry loop that checks the status result
and sleeps between attempts (use std::thread::sleep and std::time::Duration),
failing only after the retry budget is exhausted and returning a clear error if
all attempts fail. Ensure you still write to dest.to_str().unwrap() and preserve
the existing error handling behavior around the download.
In `@ui/src/components/Hosting.tsx`:
- Around line 569-580: In Hosting.tsx, when fetchHostData returns "expired"
after a login attempt, clear the stored session by calling saveHostSession(null)
in the same branch that sets setHostRegStatus and setRegStep; specifically
update the else branch handling result === "expired" to invoke
saveHostSession(null) before or after setRegStep("credentials") so the invalid
session isn't left in storage (use the existing saveHostSession function and
keep setHostRegStatus/setRegStep behavior).
---
Duplicate comments:
In `@rust-executor/build.rs`:
- Around line 51-68: The build step currently downloads ALLIANCE_DNA_URL via
Command::new("curl") and then the file is embedded via include_bytes! in
rust-executor/src/unyt_service.rs; add integrity verification by storing a
pinned SHA-256 hash (or signature) in the repo and after the curl download (the
dest path used in Command::new("curl") and ALLIANCE_DNA_URL), compute the
downloaded file’s SHA-256 and compare it to the pinned value, failing the build
with a clear error if they differ; update build.rs to perform this check
(compute hash of dest, compare to pinned SHA-256 string) and ensure the error
message references ALLIANCE_DNA_URL and dest so authors can diagnose a mismatch.
- Around line 38-44: The build script currently treats any non-empty
OUT_DIR/alliance.dna (dest) as valid which can reuse stale or partial downloads
when ALLIANCE_DNA_VERSION changes or a previous download failed; update build.rs
to cache by a version/checksum stamp instead of file size: compute the expected
stamp from ALLIANCE_DNA_VERSION (or a downloaded checksum), store that stamp
alongside the downloaded file (or include the version in the filename), and only
early-return when the stored stamp matches the current ALLIANCE_DNA_VERSION;
when downloading, write to a temporary file first and atomically rename into
dest on success to avoid partial files being treated as valid (use dest.tmp ->
std::fs::rename), and ensure any previous mismatched dest is replaced.
---
Nitpick comments:
In `@rust-executor/build.rs`:
- Around line 4-6: ALLIANCE_DNA_URL currently hardcodes the version; change it
to be derived from the single source ALLIANCE_DNA_VERSION so edits don’t
diverge. Replace the hardcoded ALLIANCE_DNA_URL with code that composes the URL
from ALLIANCE_DNA_VERSION (e.g., a function like fn alliance_dna_url() -> String
or a lazy/static string that uses format! to produce
"https://github.com/unytco/unyt-sandbox/releases/download/v{ALLIANCE_DNA_VERSION}/alliance.dna"),
and update any usage sites to call that function or use the composed value so
the version is only defined in ALLIANCE_DNA_VERSION.
In `@ui/src/components/Hosting.tsx`:
- Around line 1819-1824: The current validation in Hosting.tsx uses `num > 0`
which prevents zero prices and forces getDefault(desc); update the check to
allow free offerings by changing the condition to `num >= 0` (i.e., push {
description: desc, priceInHOT: num } when !isNaN(num) && num >= 0) or, if zero
should indeed be disallowed, add an explicit comment explaining the business
rule; ensure you reference the same variables (`num`, `desc`, `priceInHOT`,
`getDefault`) and the `updated.push(...)` logic when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3c576a2-b0a6-46ca-972d-39405ed9ffa3
📒 Files selected for processing (4)
rust-executor/build.rsrust-executor/src/graphql/mutation_resolvers.rsrust-executor/src/unyt_service.rsui/src/components/Hosting.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- rust-executor/src/graphql/mutation_resolvers.rs
- rust-executor/src/unyt_service.rs
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 by CodeRabbit
New Features
Improvements
Chores