refactor: move GUEST_BIN_REL_PATH to constants to avoid duplication by teyrebaz33 · Pull Request #45 · logos-co/scaffold

teyrebaz33 · 2026-04-06T18:12:35Z

Closes #44

Summary

GUEST_BIN_REL_PATH was defined identically in both deploy.rs and report.rs. This PR moves it to src/constants.rs and imports it from there.

Changes

src/constants.rs: added GUEST_BIN_REL_PATH
src/commands/deploy.rs: removed local const, import from constants
src/commands/report.rs: removed local const, import from constants

Testing

cargo test

fryorcraken · 2026-04-08T04:32:34Z

    let binaries_root = project.root.join(GUEST_BIN_REL_PATH);

-    preflight_sequencer_reachability(&sequencer_addr)?;
+    preflight_sequencer_reachability(&sequencer_addr, json)?;


doesnt seem related to the changes and not documented in PR description

Good catch! That change (preflight_sequencer_reachability gaining a json parameter) was leftover from an unrelated experiment and shouldn't have been here. I've removed it in the latest commit — the PR now only moves GUEST_BIN_REL_PATH to src/constants.rs.

teyrebaz33 · 2026-04-08T10:27:23Z

Addressed the review feedback: the unrelated json parameter change in preflight_sequencer_reachability has been removed in the latest commit (e815dfe). The PR now only contains the GUEST_BIN_REL_PATH constant move.

weboko · 2026-04-10T21:27:07Z

same here, @teyrebaz33 , please, sign commits

Copilot

Pull request overview

This PR aims to eliminate duplication of GUEST_BIN_REL_PATH by moving it into src/constants.rs and reusing it from both the deploy and report commands.

Changes:

Added GUEST_BIN_REL_PATH to src/constants.rs and removed duplicate local consts.
Updated deploy and report commands to import the shared constant.
(Also included) Adjusted deploy command output behavior when --json is enabled (JSON summary + suppressed human-readable prints).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/constants.rs	Introduces shared `GUEST_BIN_REL_PATH` constant for reuse across commands.
src/commands/deploy.rs	Switches to shared constant; also modifies deploy output behavior for JSON mode and guards several prints behind `!json`.
src/commands/report.rs	Switches to shared constant for guest binary path resolution.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T21:29:07Z

+                if !json {
+                    println!("FAIL {program} deployment failed");
+                    println!("  Error: {summary}");
+                }


There is a redundant nested if !json block here (if !json { if !json { ... } }). This adds noise and makes the control flow harder to follow; it should be collapsed to a single conditional guarding the prints.

Suggested change

if !json {

println!("FAIL {program} deployment failed");

println!(" Error: {summary}");

}

println!("FAIL {program} deployment failed");

println!(" Error: {summary}");

Fixed in the latest commit — the redundant nested if-not-json block and all other unrelated deploy output changes have been removed. The PR now only contains the GUEST_BIN_REL_PATH constant move.

Copilot · 2026-04-10T21:29:07Z

+    if json {
+        let json_results: Vec<serde_json::Value> = results
+            .iter()
+            .map(|r| {
+                serde_json::json!({
+                    "program": r.program,
+                    "status": r.status.label(),
+                    "detail": r.detail,
+                    "tx": r.tx,
+                })
+            })
+            .collect();
+        println!(
+            "{}",
+            serde_json::to_string_pretty(&serde_json::json!({
+                "succeeded": success_count,
+                "failed": failed_count,
+                "results": json_results,
+            }))?
+        );


This PR is described as only moving GUEST_BIN_REL_PATH into constants.rs, but this hunk also changes deploy behavior by adding JSON output formatting and suppressing human-readable output when --json is set. Please update the PR description (or split into a separate PR) so reviewers/users can track this behavioral change explicitly.

Fixed — the unrelated deploy output behavior changes have been removed. The PR now contains exactly 3 file changes: constants.rs (add const), deploy.rs (remove local const, import from constants), report.rs (remove local const, import from constants).

Copilot · 2026-04-10T21:29:07Z

 const REPORT_WARNING: &str = "WARNING: This diagnostics bundle is sanitized on a best-effort basis and may still contain sensitive data. Inspect every file before sharing it publicly.";
-const GUEST_BIN_REL_PATH: &str = "target/riscv-guest/example_program_deployment_methods/example_program_deployment_programs/riscv32im-risc0-zkvm-elf/release";
+use crate::constants::GUEST_BIN_REL_PATH;



use crate::constants::GUEST_BIN_REL_PATH; is placed after a const item. The rest of the codebase keeps all use imports grouped at the top of the module before constants; please move this import up with the other use statements to match the established layout.

Fixed — use crate::constants::GUEST_BIN_REL_PATH; has been moved to the top of the use block in report.rs, before the const items.

teyrebaz33 · 2026-04-11T13:15:02Z

Commits have been re-signed with GPG and the PR has been cleaned up — it now contains only the GUEST_BIN_REL_PATH constant move (3 files, minimal diff).

weboko · 2026-04-13T22:18:44Z

looks good, but there is a merge conflict

teyrebaz33 · 2026-04-14T11:03:50Z

Merge conflict has been resolved.

danisharora099 · 2026-05-05T14:32:46Z

@teyrebaz33 feel free to resolve the merge conflict and merge this PR. there is already an approval so you should be able to merge this

wozos · 2026-05-12T15:00:38Z

CI / validate is red on this PR because cargo fmt --check flags one import-ordering issue in src/commands/report.rs (the new use crate::constants::GUEST_BIN_REL_PATH; is not in the expected position).

I prepared a fmt-only fix but cannot push it directly: pushing to teyrebaz33/logos-scaffold:refactor/guest-bin-path-to-constants is rejected (HTTP 403). "Allow edits by maintainers" is enabled on this PR, but it only permits the upstream logos-co/scaffold maintainers, not arbitrary GitHub users.

The fix is available at:

branch: wozos/scaffold:fix/pr-45-cargo-fmt
view: https://github.com/wozos/scaffold/tree/fix/pr-45-cargo-fmt

To unblock: either cherry-pick that commit onto refactor/guest-bin-path-to-constants, or just reorder the new use to be alphabetical (move use crate::constants::GUEST_BIN_REL_PATH; above use crate::model::{...};).

teyrebaz33 requested a review from a team April 6, 2026 18:12

fryorcraken reviewed Apr 8, 2026

View reviewed changes

weboko mentioned this pull request Apr 10, 2026

refactor: move GUEST_BIN_REL_PATH to constants.rs #48

Closed

weboko requested a review from Copilot April 10, 2026 21:26

Copilot started reviewing on behalf of weboko April 10, 2026 21:27 View session

weboko approved these changes Apr 10, 2026

View reviewed changes

Copilot AI reviewed Apr 10, 2026

View reviewed changes

teyrebaz33 force-pushed the refactor/guest-bin-path-to-constants branch 2 times, most recently from 7d49f28 to 025747c Compare April 11, 2026 12:44

teyrebaz33 force-pushed the refactor/guest-bin-path-to-constants branch 2 times, most recently from e6ee1b0 to 1f9c973 Compare April 14, 2026 10:51

refactor: move GUEST_BIN_REL_PATH to constants to avoid duplication

7185356

teyrebaz33 force-pushed the refactor/guest-bin-path-to-constants branch from 1f9c973 to 7185356 Compare April 14, 2026 10:53

Merge branch 'master' into refactor/guest-bin-path-to-constants

e231832

Conversation

teyrebaz33 commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing

Uh oh!

fryorcraken Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

teyrebaz33 Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

teyrebaz33 commented Apr 8, 2026

Uh oh!

weboko commented Apr 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

teyrebaz33 Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

teyrebaz33 Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

teyrebaz33 Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

teyrebaz33 commented Apr 11, 2026

Uh oh!

weboko commented Apr 13, 2026

Uh oh!

teyrebaz33 commented Apr 14, 2026

Uh oh!

danisharora099 commented May 5, 2026

Uh oh!

wozos commented May 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

teyrebaz33 commented Apr 6, 2026 •

edited

Loading