Skip to content

fix(logseq-cli): resolve opam #main pins per-nightly via manifest#54

Merged
Bad3r merged 2 commits into
mainfrom
fix/logseq-cli-opam-pin-overrides
Jun 26, 2026
Merged

fix(logseq-cli): resolve opam #main pins per-nightly via manifest#54
Bad3r merged 2 commits into
mainfrom
fix/logseq-cli-opam-pin-overrides

Conversation

@Bad3r

@Bad3r Bad3r commented Jun 26, 2026

Copy link
Copy Markdown
Owner

Problem

The nightly publish-release "Validate flake" step fails with
[opam-nix] a git dependency without an explicit sha1 is not supported in pure evaluation mode
(failing run).

Upstream logseq/logseq@3684727952e6 (2026-06-25 "chore: update opam") repointed
cli/logseq-cli.opam pin-depends for melange-edn, melange-transit, and humanize at
the mutable #main branch. opam-nix's fetchGitURL rejects a git fragment that is not a
40-char sha1 in pure evaluation mode, so nix flake check throws. The desktop build legs
use native opam (which accepts branch refs) and pass, so every build leg is green while
only the Nix validation breaks. It does not self-heal: upstream's later eda573c re-pinned
only melange-transit, leaving the other two on #main.

Fix

opam-deps.nix resolves the closure over the unpatched upstream src FOD, so the
patches/ mechanism cannot reach it. Resolution moves to the producer and rides through
the manifest, like the other CLI hashes:

  • scripts/update-nightly.sh (Phase 3b) reads the opam at LOGSEQ_REV, git ls-remotes
    each non-sha1 git pin to its current commit, and writes cliOpamPinOverrides
    ({from,to} URL pairs) into the manifest.
  • lib/loadManifest.nix requires the field and asserts each to ends in an explicit
    40-char sha1.
  • opam-deps.nix applies the rewrites with substituteInPlace --replace-fail before
    buildOpamProject reads the file; an empty list reads src/cli in place (no-op).

Resolving at manifest time keeps the pin tracking upstream every nightly instead of
freezing a literal. The melange-edn main HEAD moved twice within ~30 minutes on
2026-06-26 (bdc79c1d -> f155433f), so a hardcoded pin would already diverge from what
the desktop legs build.

Validation

  • nixfmt, shfmt, shellcheck, deadnix, statix: clean.
  • nix eval of .#packages.x86_64-linux.logseq-cli.drvPath and
    .#packages.x86_64-linux.logseq.drvPath: identical drvs at the current empty-override
    rev (no regression).
  • Producer resolution and the substituteInPlace rewrite verified end-to-end against the
    real upstream opam at the failing-era rev (3684727952e6) and current master: fully
    sha1-pinned, zero #main remaining, --replace-fail matched every from.

logseq/logseq 3684727952e6 (2026-06-25 "chore: update opam") repointed the
cli/logseq-cli.opam pin-depends for melange-edn, melange-transit, and humanize at the
mutable #main branch. opam-nix fetchGitURL rejects a git pin whose fragment is not a
40-char sha1 in pure evaluation mode, so the nightly publish-release "Validate flake"
step (nix flake check) throws "[opam-nix] a git dependency without an explicit sha1 is
not supported in pure evaluation mode" while the desktop build legs (native opam, which
accepts branch refs) pass.

opam-deps.nix resolves the closure over the unpatched upstream src FOD, so the patches/
mechanism cannot reach it. scripts/update-nightly.sh (Phase 3b) reads the opam at
LOGSEQ_REV, git ls-remotes each non-sha1 git pin to its current commit, and records the
rewrites in the new manifest.cliOpamPinOverrides ({from,to} URL pairs); opam-deps.nix
applies them with substituteInPlace --replace-fail before buildOpamProject reads the
file (empty list reads src/cli in place). Resolving at manifest time keeps the pin
tracking upstream every nightly instead of freezing a literal: the melange-edn main HEAD
moved twice within 30 minutes on 2026-06-26 (bdc79c1d -> f155433f). lib/loadManifest.nix
requires the field and asserts each to URL ends in an explicit 40-char sha1.

Validation: nixfmt, shfmt, shellcheck, deadnix, statix; nix eval of
.#packages.x86_64-linux.logseq-cli.drvPath and .#packages.x86_64-linux.logseq.drvPath
(identical drvs at the current empty-override rev); producer resolution and the
substituteInPlace rewrite verified end-to-end against the real upstream opam at the
failing-era rev (3684727952e6) and current master.
@Bad3r Bad3r added status(security-review-approved) Required review gate for workflow or flake.lock changes. area(cli) logseq-cli package and upstream CLI build path. area(nightly) Nightly build, artifact, publish, and manifest update pipeline. origin(upstream) Root cause or required fix is in upstream Logseq or upstream tooling. type(fix) A corrective implementation change, usually used on PRs. labels Jun 26, 2026
@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

Claude finished @Bad3r's task in 3m 10s —— View job


Review

No issues found.

Walked through every hunk against the upstream opam shape and surrounding callers:

  • lib/loadManifest.nix:58builtins.match is implicitly fully anchored in Nix, so .*#[0-9a-f]{40} correctly requires to to end in #<40-hex>. Field is also wired into requiredKeys and isList is enforced.
  • modules/_packages/logseq-cli/opam-deps.nix:48-52runCommandLocal builds through stdenvNoCC.mkDerivation with the generic builder, so substituteInPlace is in scope. gemini's "substituteInPlace not sourced" claim was already refuted in PR thread; verified by re-reading the diff and the nixpkgs stdenv convention. --replace-fail is the correct strictness (fails loudly if the upstream URL drifted between manifest write and consumption).
  • scripts/update-nightly.sh:70-85resolve_ref_sha uses qualified git ls-remote patterns and exact $2 matching with explicit head→peeled-tag→tag→bare priority. Final [[ ! $sha =~ ^[0-9a-f]{40}$ ]] guard ensures only commit sha1s reach the manifest. Addresses both gemini medium and codex P2 from the prior round.
  • scripts/update-nightly.sh:94 — grep pattern git\+[a-z]+://[^"#]+#[^"]+ correctly excludes fragment-less dev-repo URLs and stops at the closing " of the opam string literal. Pipefail + || true is correct for the empty-result case. Already-pinned 40-hex fragments are skipped, so the override list self-clears once upstream restores explicit commits.
  • lib/loadManifest.nix / scripts/update-nightly.sh / modules/_packages/logseq-nightly.nix / package.nix — manifest field producer/validator/consumer wired in the same change per CLAUDE.md convention.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a mechanism to resolve mutable git branch pins in logseq-cli.opam to explicit 40-character SHA1 commits during nightly updates, ensuring compatibility with opam-nix in pure evaluation mode. The changes include adding a cliOpamPinOverrides field to the manifest, validating it in Nix, updating the nightly script to resolve these branch refs using git ls-remote, and applying the overrides in opam-deps.nix. Feedback focuses on two key issues: first, pkgs.runCommandLocal will fail when using substituteInPlace because the function is not available in its minimal environment, so pkgs.stdenv.mkDerivation should be used instead; second, the git ls-remote resolution in the update script is fragile and should be updated to match exact refs to avoid incorrect SHA resolution.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread modules/_packages/logseq-cli/opam-deps.nix
Comment thread scripts/update-nightly.sh Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9f748a43ec

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/update-nightly.sh Outdated
Phase 3b of update-nightly.sh resolved each opam pin-depends branch ref with
`git ls-remote "$remote" "$frag" | awk 'NR==1'`, which is wrong two ways:

- git ls-remote tail-matches the fragment, so a bare `main` also returns
  refs/heads/feature/main; sorted output places feature/main first and NR==1
  records the colliding branch's sha1 instead of refs/heads/main.
- an annotated-tag fragment records the tag-object sha1 (git cat-file -t
  reports `tag`), not the commit it points at. loadManifest.nix only checks
  for 40 hex chars, so the bad `to` value passes validation and opam-nix
  still throws when it needs a commit sha1.

resolve_ref_sha passes qualified patterns (refs/heads/<f>, refs/tags/<f>^{},
refs/tags/<f>) and matches column 2 exactly in awk, preferring a branch head,
then the peeled annotated tag, then a lightweight tag or already-qualified
ref. Verified against a synthetic repo (collision branch, lightweight tag,
annotated tag, missing ref) and the real humanize/logseq remotes; shfmt
-s -i 2, shellcheck, and bash -n are clean.
@Bad3r

Bad3r commented Jun 26, 2026

Copy link
Copy Markdown
Owner Author

Review triage (commit 1ce0565)

One change landed; one suggestion rejected with evidence.

Implemented

Exact ref matching + annotated-tag peeling in update-nightly.sh Phase 3b (gemini medium + codex P2, same line).

The pin resolver dropped git ls-remote "$remote" "$frag" | awk 'NR==1' in favor of a resolve_ref_sha helper that:

  • passes qualified patterns (refs/heads/<f>, refs/tags/<f>^{}, refs/tags/<f>) and matches column 2 exactly in awk, so a bare main can no longer pick up a colliding refs/heads/feature/main that sorts first;
  • prefers the peeled annotated-tag commit (refs/tags/<f>^{}) over the tag object, so opam-nix receives a commit sha1 rather than a tag-object sha1 that passes the manifest 40-hex check yet fails resolution.

Priority order: branch head, then peeled annotated tag, then lightweight tag / already-qualified ref. Verified against a synthetic repo (collision branch, lightweight tag, annotated tag, missing ref) and the real humanize/logseq remotes; shfmt -s -i 2, shellcheck, and bash -n are clean.

Rejected

Switch runCommandLocal to stdenv.mkDerivation for substituteInPlace (gemini high, opam-deps.nix).

substituteInPlace is a stdenv setup.sh function and runCommandLocal does source it: it builds through stdenv.mkDerivation with a buildCommand (the generic builder). Building pkgs.runCommandLocal "t" {} ''substituteInPlace …'' against this flake's nixpkgs applied the substitution and the build succeeded. runCommandLocal differs from runCommand only by preferLocalBuild/allowSubstitutes, not by skipping setup hooks, so no change is warranted.

Reliability

The accepted change replaces a positional NR==1 heuristic with exact ref matching under an explicit head / peeled-tag / lightweight-tag priority, and keeps the existing [[ ! $sha =~ ^[0-9a-f]{40}$ ]] guard that fails loudly on an unresolved ref. That closes a silent-miss path where a colliding branch or an annotated tag could have been written to the manifest and only surfaced later as an opam-nix pure-evaluation failure.

@Bad3r Bad3r merged commit 1a9af21 into main Jun 26, 2026
21 of 22 checks passed
@Bad3r Bad3r deleted the fix/logseq-cli-opam-pin-overrides branch June 26, 2026 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area(cli) logseq-cli package and upstream CLI build path. area(nightly) Nightly build, artifact, publish, and manifest update pipeline. origin(upstream) Root cause or required fix is in upstream Logseq or upstream tooling. status(security-review-approved) Required review gate for workflow or flake.lock changes. type(fix) A corrective implementation change, usually used on PRs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant