chore: four minor review observations (TempDir ownership, GIT_SSH_COMMAND, report JSON embedding, pub surface)

[shunichironomura]

Warning

This content was written by an AI agent and must be verified by a human developer. After human verification, this alert may be removed.

Summary

Four small observations from a full-workspace review, none of which is a live defect. Filed together so they are not lost; each is independently closeable and none is urgent.

1. publish_checkout renames a live TempDir out from under itself

crates/graphcal-cli/src/deps.rs:957

fn publish_checkout(temporary: &tempfile::TempDir, destination: &Path) -> Result<(), DepsError> {
    let temporary_path = temporary.path().to_path_buf();
    ...
    std::fs::rename(&temporary_path, destination)   // moves the TempDir's own path away

It borrows the TempDir and renames its path away; the later Drop then tries to remove_dir_all a path that no longer exists and swallows the error. Harmless today.

Suggestion: take the TempDir by value and call .keep() before the rename, making the ownership transfer explicit and removing the ignored-error path.

2. GIT_SSH_COMMAND is ignored for SSH-transport dependencies

crates/graphcal-cli/src/deps.rs:715

fn isolated_git_open_options(transport: GitTransport) -> gix::open::Options {
    let overrides = match transport {
        GitTransport::Https => None,
        GitTransport::Ssh | GitTransport::Scp => std::env::var_os("GIT_SSH").map(|command| {
            format!("gitoxide.ssh.commandWithoutShellFallback={}", command.to_string_lossy())
        }),
    }
    ...

gix::open::Options::isolated() deliberately drops ambient environment config, and only GIT_SSH is re-admitted. Git itself also honours GIT_SSH_COMMAND, at higher precedence, and it is by far the more commonly set of the two — so a user with a custom SSH command may find it silently unused when fetching an ssh:// or scp-style dependency.

Suggestion: confirm the intended behaviour against the gix config-override documentation before changing anything; this may be a deliberate narrowing rather than an oversight.

3. Report HTML embeds plot JSON as a bare JS expression

crates/graphcal-report/src/report_html.rs:75-79 and plot_page.rs:58

let spec_json = escape_json_for_script(&card.figure.spec.to_string());
let _ = writeln!(body,
    "<script>vegaEmbed('#{div_id}', {spec_json}, {{\"actions\": false}}).catch(console.error);</script>");

escape_json_for_script replaces < with \u003c, which correctly neutralizes </script> and <!--. U+2028/U+2029 have been legal inside JS string literals since ES2019, so this is not exploitable on any current browser.

Suggestion: emitting into <script type="application/json"> and JSON.parse-ing it would move the guarantee from "one replace call is correct" to "structurally impossible". That is the stronger position for HTML meant to be shared with third parties. Pure hardening.

4. pub surface could be tightened tool-assisted

No item in the workspace is pub without a consumer outside its defining file — visibility hygiene is genuinely good. But 266 pub items in graphcal-compiler and 158 in graphcal-eval are never named by another crate; some are consumed only by in-crate integration tests under tests/.

Suggestion: a cargo public-api baseline in CI would let these be tightened to pub(crate) deliberately over time rather than audited by hand. Low value; recorded for completeness.

Provenance

Found during a full-workspace code review at 6e462341a (v0.0.1-alpha.27). Baseline at that commit: cargo clippy --workspace --all-targets clean, cargo test --workspace 2926 passed / 0 failed, cargo deny check advisories ok.