Skip to content

fix: obsolete comment and add test for double-escaping hostname in nix_eval_homebrew_attr - #654

Merged
Scott McMaster (scottmcmaster) merged 1 commit into
mainfrom
08-11-scott-nix_eval_homebrew_attr-comment
Aug 27, 2026
Merged

fix: obsolete comment and add test for double-escaping hostname in nix_eval_homebrew_attr#654
Scott McMaster (scottmcmaster) merged 1 commit into
mainfrom
08-11-scott-nix_eval_homebrew_attr-comment

Conversation

@scottmcmaster

@scottmcmaster Scott McMaster (scottmcmaster) commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

Saw this while reviewing #652 (which I think should be closed without merging, at least for the homebrew_adopt.rs change).

Test Plan

Enhanced unit test.

  • No test plan needed

Docs

  • Docs updated (companion PR in darkmatter/nixmac-web: #___)
  • No docs update needed

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@scottmcmaster Scott McMaster (scottmcmaster) changed the title scott-nix_eval_homebrew_attr-comment fix: obsolete comment and add test for double-escaping hostname in nix_eval_homebrew_attr Aug 11, 2026
@scottmcmaster
Scott McMaster (scottmcmaster) marked this pull request as ready for review August 11, 2026 02:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates nix_eval_homebrew_attr’s inline documentation to match the current implementation (which uses nix_string_literal), and strengthens the unit test to cover hostnames containing quote characters so regressions in quoting/escaping are caught early.

Changes:

  • Replace an obsolete comment that referenced serde_json::to_string with an accurate note about nix_string_literal producing a fully quoted Nix string literal.
  • Extend the existing unit test to assert correct escaping when the hostname input contains quotes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@darkmatter

darkmatter Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

🎨 Storybook preview

Open Storybook preview

Updated for e3c544b


⚠️ Detected UI changes (5)

These stories' HTML snapshots changed. I've added screenshots + links to the changed stories below. Review them carefully then accept the changes to regenerate baselines and include them in this PR:

Flows/Evolve › Playground

Flows/Evolve › Playground

Flows/Evolve › 1. Begin (idle)

Flows/Evolve › 1. Begin (idle)

Flows/Evolve › 2. Evolving (progress)

Flows/Evolve › 2. Evolving (progress)

Flows/Evolve › Evolving With Error Event

Flows/Evolve › Evolving With Error Event

Flows/Evolve › 3. Review (changes generated)

Flows/Evolve › 3. Review (changes generated)


Accept UI changes

  • Click here to accept these changes

Alternatively, you can run bun run test:update-snapshots locally to re-generate the baselines and then push the changes to this PR.

What does this do?

The screenshots above show UI changes detected by the Storybook
snapshot tests run on this PR. Each image is the rendered output of
a Storybook story from the code in this PR branch; the snapshot
test compared it against the committed baseline in
__snapshots__/ and flagged the difference.

Checking the box tells the darkmatter[bot] to regenerate the
baselines from this PR's current code and commit them directly to
this branch. The new baselines become the source of truth for
future runs — only accept after confirming the visual changes are
intentional.

Comparison baseline: the committed __snapshots__/ files on this
PR branch (carried forward from develop). Accept updates them in
place on this branch.

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor
Warnings
⚠️

No Linear issue ID found in this PR's title, description, or branch name (expected something like ENG-123). Add one so this work is traceable in Linear, or add #no-linear to the PR description to acknowledge it's intentionally untracked.

📋 PR Overview

Lines changed 11 (+8 / -3)
Files 0 added, 1 modified, 0 deleted
Draft / WIP no
Has Test Plan yes
Linear issue no
No Test Plan Needed no
New UI components no
New Storybook stories no
New Rust modules no
New TS source files no
New tests no
package.json touched no
Cargo.toml touched no
Infra / CI touched no

🔬 Coverage

Report Lines Statements Functions Branches
apps/native/coverage/coverage-summary.json 35.6% 35.2% 30.5% 29.5%

Generated by 🚫 dangerJS against e3c544b

@scottmcmaster
Scott McMaster (scottmcmaster) force-pushed the 08-11-scott-nix_eval_homebrew_attr-comment branch from 12e5bd6 to d994f95 Compare August 11, 2026 02:58
@scottmcmaster
Scott McMaster (scottmcmaster) force-pushed the 08-11-scott-nix_eval_homebrew_attr-comment branch from d994f95 to e3c544b Compare August 26, 2026 06:29
// serde_json::to_string already wraps the hostname in quotes and escapes
// internals — do not also embed quote chars in the format string or we
// produce malformed attrs like .#darwinConfigurations.""host"".config…
// nix_string_literal returns a fully quoted Nix string literal, so the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit on the wording: the attr path here isn't parsed as a Nix string literal. Nix's parseAttrPath (src/libexpr/attr-path.cc) understands "…" quoting but no backslash escapes:

} else if (*i == '"') {
    ++i;
    while (1) {
        if (i == s.end())
            throw ParseError("missing closing quote in selection path '%1%'", s);
        if (*i == '"')
            break;
        cur.push_back(*i++);
    }

So the \" cases in the test lock in output that nix rejects. Reproduced on nix 2.34.7 with a scratch flake that defines both attrs:

$ nix eval --json '.#darwinConfigurations."office\"mac".config.homebrew'
error: missing closing quote in selection path '...darwinConfigurations."office\"mac".config.homebrew'

$ nix eval --json '.#darwinConfigurations."\"quoted-host\"".config.homebrew'
error: flake ... does not provide attribute '...darwinConfigurations."\"quoted-host\"".config.homebrew'
       Did you mean "quoted-host"?

Nothing breaks in practice — scutil --get LocalHostName can't produce quotes or backslashes — so this is purely about the comment and test not asserting something nix can't parse. Would it work to say just "nix_string_literal already wraps the hostname in quotes, so the format string must not add another pair" and drop the two escape assertions?

Merged via the queue into main with commit c52e66c Aug 27, 2026
12 checks passed
@scottmcmaster
Scott McMaster (scottmcmaster) deleted the 08-11-scott-nix_eval_homebrew_attr-comment branch August 27, 2026 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants