Fix/gl acceptance findings#236
Conversation
gl init names its remote 'gitlawb', but detect_gitlawb_remote() only ever read origin — so gl status reported 'not in a gitlawb repo' in the very repo gl init had just set up. Scan all remotes, preferring origin (clones) then gitlawb, then any other gitlawb:// URL. (#231 acceptance test finding)
- git init -b main (plain-init fallback for git <2.28) so a fresh repo can't land on master while every hint says main. - The closing hint checks HEAD: with no commits it prints the add/commit step first — a bare 'git push gitlawb main' at that point fails with 'src refspec main does not match any', the exact trap the acceptance test walked into. The current branch name is used in the hint either way.
show printed the signing payload and told the user to verify offline themselves. It now rebuilds the node's canonical payload (field order mirrors gitlawb-node cert issuance), derives the Ed25519 key from the certificate's node DID, verifies, and prints a VALID/INVALID verdict; --verify makes the exit code reflect it for scripting. Round-trip + tamper + malformed-signature unit tests included.
…ailure A localhost GITLAWB_NODE red-flagged every self-hosted and dev-harness setup even when the node was healthy; the connectivity check already fails loudly when the configured node is unreachable. The env check now passes with an explanatory note.
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
📝 WalkthroughWalkthrough
Changesgl CLI behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant cert_show
participant verify_signature
participant configured_node
cert_show->>verify_signature: Rebuild canonical payload and verify signature
verify_signature-->>cert_show: Return signature verdict
cert_show->>configured_node: Fetch current node DID
configured_node-->>cert_show: Return node identity
cert_show-->>cert_show: Print validity and identity comparison
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/release.ymlTraceback (most recent call last): Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gl/src/cert.rs (1)
142-169: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing-field fallbacks ("?"/"") get fed into signature verification, masking malformed responses as "INVALID".
cert_id,ref_name,old_sha, etc. default to"?"(display-only fallback) andrepo_iddefaults to""when absent, then those same values are used to rebuild the signed payload forverify_signature. If the API response is missing a field, this manifests as a generic "Ed25519 signature does not match" rather than a clear "certificate response is malformed" error, which could mislead a user into thinking a cert was tampered with.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/cert.rs` around lines 142 - 169, Validate that all certificate fields required by signature verification are present and strings before constructing the display values or calling verify_signature; return a clear malformed certificate response error identifying missing or invalid fields instead of substituting "?" or "" for verification inputs. Update the certificate-processing function around cert_id, ref_name, old_sha, new_sha, pusher, node_did, issued_at, signature, and repo_id, while retaining display-only fallbacks only for fields not used in the signed payload.
🧹 Nitpick comments (1)
crates/gl/src/cert.rs (1)
209-249: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWrap these fields in a struct before calling
verify_signature; 8 positional&strarguments make it easy to silently swap the DID-like values later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/cert.rs` around lines 209 - 249, Introduce a parameter struct for the signed fields and update verify_signature to accept that struct instead of eight positional &str arguments. Populate and access the struct fields by name at the call site and within verify_signature, preserving the existing payload contents and verification behavior while preventing accidental swaps of DID-like values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gl/src/cert.rs`:
- Around line 181-200: Make the node-info request and JSON parsing in the
certificate display flow non-fatal: handle errors from client.get("/") and the
“failed to fetch node info” context by printing a warning and continuing to the
existing signature verdict handling. Preserve the DID comparison output when
node info is available, and keep require_valid enforcement unchanged.
In `@crates/gl/src/doctor.rs`:
- Around line 151-155: Update the GITLAWB_NODE validation match in the doctor
check to parse the value as a URL and inspect its exact host, accepting only
localhost or 127.0.0.1. Remove the substring-based contains checks while
preserving the existing intentional-local-node pass behavior and handling
invalid URLs through the existing non-local path.
In `@crates/gl/src/init.rs`:
- Around line 189-204: Update the branch detection and push-instruction logic in
the initialization flow to preserve a detached HEAD as a missing branch instead
of defaulting to "main". When no branch is available, instruct the user to check
out or create a branch before pushing; retain the existing git push command only
when a valid branch name was detected.
In `@crates/gl/src/status.rs`:
- Around line 179-195: Update the remote URL inspection loop around
parse_gitlawb_url to query both fetch and push URLs using git remote get-url
--all and git remote get-url --push --all for each remote. Parse every returned
line, preserving the existing error and unsuccessful-command handling, and
return the first Gitlawb URL found.
---
Outside diff comments:
In `@crates/gl/src/cert.rs`:
- Around line 142-169: Validate that all certificate fields required by
signature verification are present and strings before constructing the display
values or calling verify_signature; return a clear malformed certificate
response error identifying missing or invalid fields instead of substituting "?"
or "" for verification inputs. Update the certificate-processing function around
cert_id, ref_name, old_sha, new_sha, pusher, node_did, issued_at, signature, and
repo_id, while retaining display-only fallbacks only for fields not used in the
signed payload.
---
Nitpick comments:
In `@crates/gl/src/cert.rs`:
- Around line 209-249: Introduce a parameter struct for the signed fields and
update verify_signature to accept that struct instead of eight positional &str
arguments. Populate and access the struct fields by name at the call site and
within verify_signature, preserving the existing payload contents and
verification behavior while preventing accidental swaps of DID-like values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1f0f7d4-dab5-482c-8a08-6ad2b67def6b
📒 Files selected for processing (5)
crates/gl/Cargo.tomlcrates/gl/src/cert.rscrates/gl/src/doctor.rscrates/gl/src/init.rscrates/gl/src/status.rs
oh-my-zsh's default git plugin ships gl='git pull', silently shadowing gl in every interactive zsh — the symptom is the baffling 'fatal: not a git repository'. Aliases beat PATH, so no installer can fix it; doctor now detects the setup (explicit alias gl= in ~/.zshrc, or the omz git plugin without an unalias) and prescribes the one-line fix. Best-effort heuristic, warning-level, covered by four unit tests.
install.sh detects the common shadowing setup (omz git plugin, no unalias in ~/.zshrc) and prints the fix right after installing; the generated Homebrew formula gains a caveats block saying the same, so brew install/upgrade surfaces it too. Catching this at install time beats letting the first gl invocation print git's error.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gl/src/doctor.rs`:
- Around line 331-332: Make alias cleanup detection shell-aware in
crates/gl/src/doctor.rs lines 331-332 and install.sh lines 200-202: replace raw
substring checks with parsing of actual unalias gl commands, and compare command
ordering with oh-my-zsh loading so only cleanup occurring after loading
suppresses the warning; handle comments, unalias global, and pre-load unalias gl
without suppressing detection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 752fab8a-027a-4b6f-9df8-1da5e1ffb619
📒 Files selected for processing (3)
.github/workflows/release.ymlcrates/gl/src/doctor.rsinstall.sh
…opback, detached HEAD, multi-URL remotes, command-aware unalias - cert show: the node-DID comparison is contextual; a node-info fetch error now prints a note instead of failing the command after the certificate and verdict were already shown. - doctor: loopback detection parses the URL host exactly — substring matching misread localhost.example and localhost-in-path URLs. - init: a detached HEAD gets its own hint (switch to a branch first) instead of a guessed 'git push gitlawb main'. - status: every fetch AND push URL of each remote is inspected — a gitlawb:// push URL behind an https fetch URL now counts. - doctor/install.sh: unalias detection is command-aware — comments, 'unalias global', and gl-only-in-a-trailing-comment no longer suppress the warning (BSD-grep-compatible pattern; ordering vs oh-my-zsh load documented as accepted slack). Eight new test cases.
|
@coderabbitai please review and approve |
|
✅ Action performedReview finished.
|
|
@coderabbitai please make a thorough review here |
|
✅ Action performedFull review finished. |
|
Reviewed the new cert verification path and the doctor/init/status fixes against the node's signing code. The core is sound: the Ed25519 verify fails closed (a bad DID, non-base64 signature, wrong length, or wrong key all render INVALID and One design question, then a few nits. Design question: what does
|
…oad form Settles the review's design question: a valid signature alone proves internal consistency — the payload was signed by whatever key the certificate itself names — so a hostile source could self-sign and pass. --verify now also requires the issuing node_did to match a trusted anchor: --expect-node when given, else the DID of the node being queried; unanchorable (node info unreachable, no --expect-node) fails closed. The displayed verdict wording says what it proves. Also from review: - A frozen-literal test pins gl's payload serialization to the node's canonical byte form, so serialization drift (new field, a preserve_order feature unifying across the workspace) fails a test instead of silently rendering every real certificate INVALID. - The printed unalias remediation uses the idempotent-safe 'unalias gl 2>/dev/null' form everywhere (doctor, install.sh, brew caveats), matching what doctor already suggested. - Acknowledged, deliberately left: bash-side alias detection and 127/8-range loopback matching (noted as out of scope for a warning-level heuristic).
Matches the CLI's printed advice: 'unalias gl 2>/dev/null' doesn't error when gl isn't currently aliased (review feedback on Gitlawb/node#236).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/gl/src/doctor.rs (1)
360-398: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider gating on the active shell to avoid false positives.
check_shell_alias_shadowingonly inspects the contents of~/.zshrc; it never checks whether zsh is actually the user's active shell (e.g. via$SHELL). A stale or dotfiles-synced~/.zshrcon a bash-only machine would trigger a spurious "shell alias" warning even thoughglis never actually shadowed for that user.♻️ Suggested guard
fn check_shell_alias_shadowing(home: Option<PathBuf>) -> Option<Check> { let home = home?; + // Skip when zsh isn't the active shell — a stale .zshrc from dotfiles + // sync or an old zsh trial would otherwise produce a false warning. + let uses_zsh = std::env::var("SHELL") + .map(|s| s.contains("zsh")) + .unwrap_or(false); + if !uses_zsh { + return None; + } let rc = std::fs::read_to_string(home.join(".zshrc")).ok()?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/doctor.rs` around lines 360 - 398, Update check_shell_alias_shadowing to first verify that the user’s active shell is zsh, using the existing environment or platform shell lookup mechanism. Return None before reading ~/.zshrc when the active shell is not zsh, while preserving the current alias and oh-my-zsh detection for zsh users..github/workflows/release.yml (1)
594-601: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCaveats display unconditionally, unlike the detection-based warnings elsewhere.
Unlike
doctor.rs/install.sh, which only warn when the oh-my-zsh git plugin is actually detected, thiscaveatsmethod always prints the oh-my-zsh-specific note to every Homebrew installer, including bash users or those without the git plugin. Homebrew'scaveatsruns client-side, so it can check the same conditions (presence of~/.zshrc, oh-my-zsh git plugin dir,plugins=(...git...), absence ofunalias gl) before deciding to print.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 594 - 601, The Homebrew formula’s caveats method unconditionally prints an oh-my-zsh warning. Update caveats to emit that note only when the client-side checks confirm ~/.zshrc exists, the oh-my-zsh git plugin is present and enabled in plugins=(...git...), and unalias gl is absent; otherwise return no caveat while preserving the existing warning text and remediation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 594-601: The Homebrew formula’s caveats method unconditionally
prints an oh-my-zsh warning. Update caveats to emit that note only when the
client-side checks confirm ~/.zshrc exists, the oh-my-zsh git plugin is present
and enabled in plugins=(...git...), and unalias gl is absent; otherwise return
no caveat while preserving the existing warning text and remediation.
In `@crates/gl/src/doctor.rs`:
- Around line 360-398: Update check_shell_alias_shadowing to first verify that
the user’s active shell is zsh, using the existing environment or platform shell
lookup mechanism. Return None before reading ~/.zshrc when the active shell is
not zsh, while preserving the current alias and oh-my-zsh detection for zsh
users.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1260b3e5-3f9f-4e00-b58d-ad5199c4bfa7
📒 Files selected for processing (7)
.github/workflows/release.ymlcrates/gl/Cargo.tomlcrates/gl/src/cert.rscrates/gl/src/doctor.rscrates/gl/src/init.rscrates/gl/src/status.rsinstall.sh
|
@coderabbitai make a thorough review her eplease |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR diff stat and changed files =='
git diff --stat HEAD^ HEAD
git diff --name-only HEAD^ HEAD
echo '== Recent commits =='
git log --oneline -8
echo '== Full PR-head diff =='
git diff --find-renames --find-copies HEAD^ HEAD -- \
crates/gl/Cargo.toml \
crates/gl/src/cert.rs \
crates/gl/src/doctor.rs \
crates/gl/src/init.rs \
crates/gl/src/status.rs \
.github/workflows/release.yml \
install.sh
echo '== Source outlines =='
ast-grep outline crates/gl/src/cert.rs --items all
ast-grep outline crates/gl/src/doctor.rs --items all
ast-grep outline crates/gl/src/init.rs --items all
ast-grep outline crates/gl/src/status.rs --items all
echo '== Targeted references and tests =='
rg -n -C 3 --glob '*.rs' \
'verify_signature|cmd_show|expect_node|CertCmd::Show|detect_gitlawb_remote|parse_gitlawb_url|is_loopback_url|alias.*gl|init -b|symbolic-ref' \
cratesLength of output: 64710 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR metadata =='
gh pr view 236 --repo Gitlawb/node --json baseRefName,baseRefOid,headRefName,headRefOid,commits
base="$(gh pr view 236 --repo Gitlawb/node --json baseRefOid --jq .baseRefOid)"
echo "== Complete diff from base ${base} to HEAD =="
git diff --find-renames --find-copies "$base" HEAD -- \
crates/gl/Cargo.toml crates/gl/src/cert.rs crates/gl/src/doctor.rs \
crates/gl/src/init.rs crates/gl/src/status.rs .github/workflows/release.yml install.sh
echo '== Certificate command implementation and tests =='
sed -n '1,430p' crates/gl/src/cert.rs
echo '== Node certificate issuer implementation =='
ast-grep outline crates/gitlawb-node/src/cert.rs --items all
rg -n -C 8 'issue_ref_certificate|node_did|signature|/info|node.*did' crates/gitlawb-node/src/cert.rs crates/gl/src/http.rs crates/gl/src/cert.rs
echo '== Tests covering changed status/init/doctor behavior =='
sed -n '130,285p' crates/gl/src/status.rs
sed -n '1,275p' crates/gl/src/init.rs
sed -n '120,575p' crates/gl/src/doctor.rsLength of output: 107350 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Workspace dependency and lockfile references =='
rg -n -C 2 '^\s*base64\s*=|name = "base64"' Cargo.toml Cargo.lock crates/gl/Cargo.toml
echo '== DID parsing and public-key derivation =='
ast-grep outline crates/gitlawb-core/src/did.rs --items all
rg -n -C 8 'impl .*Did|fn from_str|to_verifying_key|enum Did|did:key' crates/gitlawb-core/src/did.rs
echo '== Remaining init and cert test implementations =='
sed -n '245,430p' crates/gl/src/init.rs
sed -n '300,430p' crates/gl/src/cert.rs
echo '== Git remote command usage and status test coverage =='
rg -n -C 4 --glob '*.rs' 'remote", "get-url"|get-url", "--all"|get-url", "--push"|detect_gitlawb_remote' crates
echo '== CLI parser conventions for requires and command tests =='
rg -n -C 3 --glob '*.rs' 'requires = "|try_parse_from|parse_from' crates/gl/srcLength of output: 23371
Highlights:
I inspected the complete PR diff and relevant implementation/tests. I did not execute repository tests because this review sandbox does not run project code. ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/gl/src/init.rs (1)
178-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the push-guidance logic for direct unit testing.
This match block replaces the exact logic that a past review flagged as buggy (detached-HEAD defaulting to
main), yet none of the tests in this file exercise the(branch, has_commits)matrix. Extracting the detection + message selection into a small, pure helper (takinghas_commits: boolandbranch: Option<String>, returning the lines to print) would let each of the three cases be regression-tested without spawning real git processes for the println behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/init.rs` around lines 178 - 218, Extract the push-guidance selection currently handled by the match in the initialization flow into a small pure helper that accepts has_commits and branch and returns the output lines or equivalent message data. Keep git detection and printing at the existing call site, have the helper preserve all three branch/commit cases including detached HEAD, and add unit tests covering each case without spawning git processes.crates/gl/src/cert.rs (1)
246-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider replacing the 8 positional
&strargs with a named struct.
verify_signatureand its three test call sites (Lines 364-397) all pass the same 8 same-typed&strvalues positionally. Nothing in the type system prevents a future edit from transposing e.g.old_sha/new_shaorpusher/node_did— the compiler can't catch it since every parameter has the same type.♻️ Proposed refactor sketch
+struct CertPayloadFields<'a> { + repo_id: &'a str, + ref_name: &'a str, + old_sha: &'a str, + new_sha: &'a str, + pusher: &'a str, + node_did: &'a str, + issued_at: &'a str, + signature_b64: &'a str, +} + -fn verify_signature( - repo_id: &str, - ref_name: &str, - old_sha: &str, - new_sha: &str, - pusher: &str, - node_did: &str, - issued_at: &str, - signature_b64: &str, -) -> std::result::Result<(), String> { +fn verify_signature(fields: CertPayloadFields<'_>) -> std::result::Result<(), String> { + let CertPayloadFields { repo_id, ref_name, old_sha, new_sha, pusher, node_did, issued_at, signature_b64 } = fields; ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/cert.rs` around lines 246 - 286, Replace the positional parameters of verify_signature with a named parameter struct containing the eight signature fields, and update its three test call sites to construct and pass that struct. Use the struct fields when building the payload so values such as old_sha, new_sha, pusher, and node_did are explicitly associated with their names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gl/src/init.rs`:
- Around line 202-218: Update the detached-HEAD branch guidance in the (None, _)
match arm so it works whether a local main branch already exists: avoid always
suggesting git switch -c main, and provide a command or conditional guidance
that switches to the existing branch or creates it only when absent. Keep the
push instruction targeting the selected main branch.
In `@install.sh`:
- Around line 196-208: Update the negative unalias check in the install-time
oh-my-zsh detection block to treat `;`, `&`, and `|` immediately following `gl`
as valid command terminators, matching `doctor.rs`’s `rc_unaliases_gl` behavior.
Preserve the existing handling for whitespace, comments, and end-of-line so an
existing `unalias gl` suppresses the NOTE in all supported command-separator
forms.
---
Nitpick comments:
In `@crates/gl/src/cert.rs`:
- Around line 246-286: Replace the positional parameters of verify_signature
with a named parameter struct containing the eight signature fields, and update
its three test call sites to construct and pass that struct. Use the struct
fields when building the payload so values such as old_sha, new_sha, pusher, and
node_did are explicitly associated with their names.
In `@crates/gl/src/init.rs`:
- Around line 178-218: Extract the push-guidance selection currently handled by
the match in the initialization flow into a small pure helper that accepts
has_commits and branch and returns the output lines or equivalent message data.
Keep git detection and printing at the existing call site, have the helper
preserve all three branch/commit cases including detached HEAD, and add unit
tests covering each case without spawning git processes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f493c3a-bb5a-47cc-93e0-fef8af38a31c
📒 Files selected for processing (7)
.github/workflows/release.ymlcrates/gl/Cargo.tomlcrates/gl/src/cert.rscrates/gl/src/doctor.rscrates/gl/src/init.rscrates/gl/src/status.rsinstall.sh
| println!(); | ||
| println!("Ready! Push with:"); | ||
| println!(" git push gitlawb main"); | ||
| match (branch, has_commits) { | ||
| (Some(branch), true) => { | ||
| println!("Ready! Push with:"); | ||
| println!(" git push gitlawb {branch}"); | ||
| } | ||
| (Some(branch), false) => { | ||
| println!("Ready! Nothing is committed yet — commit, then push:"); | ||
| println!(" git add -A && git commit -m \"initial commit\""); | ||
| println!(" git push gitlawb {branch}"); | ||
| } | ||
| (None, _) => { | ||
| println!("Ready! HEAD is detached — create or switch to a branch, then push:"); | ||
| println!(" git switch -c main"); | ||
| println!(" git push gitlawb main"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
git switch -c main fails if main already exists while HEAD is detached.
This correctly stops guessing a branch for the detached-HEAD case (fixing the prior finding), but the fallback hint always suggests creating main with -c. If a local main branch already exists (e.g., user checked out a tag/commit while main still exists), git switch -c main errors with "branch already exists," leaving the user with a broken copy-paste command.
🔧 Proposed fix
(None, _) => {
println!("Ready! HEAD is detached — create or switch to a branch, then push:");
- println!(" git switch -c main");
+ let main_exists = std::process::Command::new("git")
+ .args(["show-ref", "--verify", "--quiet", "refs/heads/main"])
+ .current_dir(&cwd)
+ .status()
+ .map(|s| s.success())
+ .unwrap_or(false);
+ if main_exists {
+ println!(" git switch main");
+ } else {
+ println!(" git switch -c main");
+ }
println!(" git push gitlawb main");
}📝 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.
| println!(); | |
| println!("Ready! Push with:"); | |
| println!(" git push gitlawb main"); | |
| match (branch, has_commits) { | |
| (Some(branch), true) => { | |
| println!("Ready! Push with:"); | |
| println!(" git push gitlawb {branch}"); | |
| } | |
| (Some(branch), false) => { | |
| println!("Ready! Nothing is committed yet — commit, then push:"); | |
| println!(" git add -A && git commit -m \"initial commit\""); | |
| println!(" git push gitlawb {branch}"); | |
| } | |
| (None, _) => { | |
| println!("Ready! HEAD is detached — create or switch to a branch, then push:"); | |
| println!(" git switch -c main"); | |
| println!(" git push gitlawb main"); | |
| } | |
| } | |
| println!(); | |
| match (branch, has_commits) { | |
| (Some(branch), true) => { | |
| println!("Ready! Push with:"); | |
| println!(" git push gitlawb {branch}"); | |
| } | |
| (Some(branch), false) => { | |
| println!("Ready! Nothing is committed yet — commit, then push:"); | |
| println!(" git add -A && git commit -m \"initial commit\""); | |
| println!(" git push gitlawb {branch}"); | |
| } | |
| (None, _) => { | |
| println!("Ready! HEAD is detached — create or switch to a branch, then push:"); | |
| let main_exists = std::process::Command::new("git") | |
| .args(["show-ref", "--verify", "--quiet", "refs/heads/main"]) | |
| .current_dir(&cwd) | |
| .status() | |
| .map(|s| s.success()) | |
| .unwrap_or(false); | |
| if main_exists { | |
| println!(" git switch main"); | |
| } else { | |
| println!(" git switch -c main"); | |
| } | |
| println!(" git push gitlawb main"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/gl/src/init.rs` around lines 202 - 218, Update the detached-HEAD
branch guidance in the (None, _) match arm so it works whether a local main
branch already exists: avoid always suggesting git switch -c main, and provide a
command or conditional guidance that switches to the existing branch or creates
it only when absent. Keep the push instruction targeting the selected main
branch.
| # oh-my-zsh's default git plugin aliases gl='git pull', which silently shadows | ||
| # the gl binary in every interactive zsh (aliases beat PATH). Detect the common | ||
| # setup and say so now, at install time, instead of letting the first `gl` | ||
| # print git's baffling "fatal: not a git repository". | ||
| if [ -f "$HOME/.zshrc" ] && [ -d "$HOME/.oh-my-zsh/plugins/git" ] \ | ||
| && grep -Eq '^[[:space:]]*plugins=\(([^)]*[[:space:]])?git([[:space:]][^)]*)?\)' "$HOME/.zshrc" \ | ||
| && ! grep -Eq '^([^#]*[[:space:];&|])?unalias[[:space:]]+([^;&|#]*[[:space:]])?gl([[:space:]]|$)' "$HOME/.zshrc"; then | ||
| echo "" | ||
| echo "NOTE: oh-my-zsh's git plugin aliases gl='git pull', which will shadow" | ||
| echo "the gl command in interactive shells. To use gl by name, run:" | ||
| echo "" | ||
| echo " echo 'unalias gl 2>/dev/null' >> ~/.zshrc && source ~/.zshrc" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unalias regex doesn't accept ;/&/| immediately after gl, causing false-positive NOTE.
The negative-lookahead-style check requires gl be followed by whitespace or end-of-line. A line like unalias gl; (no space before the semicolon) won't match, so the install-time NOTE still fires even though gl was already unaliased. doctor.rs's rc_unaliases_gl avoids this by splitting the line on ;/&/| before tokenizing.
🛠️ Proposed fix: accept command separators as valid terminators after `gl`
- && ! grep -Eq '^([^#]*[[:space:];&|])?unalias[[:space:]]+([^;&|#]*[[:space:]])?gl([[:space:]]|$)' "$HOME/.zshrc"; then
+ && ! grep -Eq '^([^#]*[[:space:];&|])?unalias[[:space:]]+([^;&|#]*[[:space:]])?gl([[:space:];&|]|$)' "$HOME/.zshrc"; then📝 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.
| # oh-my-zsh's default git plugin aliases gl='git pull', which silently shadows | |
| # the gl binary in every interactive zsh (aliases beat PATH). Detect the common | |
| # setup and say so now, at install time, instead of letting the first `gl` | |
| # print git's baffling "fatal: not a git repository". | |
| if [ -f "$HOME/.zshrc" ] && [ -d "$HOME/.oh-my-zsh/plugins/git" ] \ | |
| && grep -Eq '^[[:space:]]*plugins=\(([^)]*[[:space:]])?git([[:space:]][^)]*)?\)' "$HOME/.zshrc" \ | |
| && ! grep -Eq '^([^#]*[[:space:];&|])?unalias[[:space:]]+([^;&|#]*[[:space:]])?gl([[:space:]]|$)' "$HOME/.zshrc"; then | |
| echo "" | |
| echo "NOTE: oh-my-zsh's git plugin aliases gl='git pull', which will shadow" | |
| echo "the gl command in interactive shells. To use gl by name, run:" | |
| echo "" | |
| echo " echo 'unalias gl 2>/dev/null' >> ~/.zshrc && source ~/.zshrc" | |
| fi | |
| # oh-my-zsh's default git plugin aliases gl='git pull', which silently shadows | |
| # the gl binary in every interactive zsh (aliases beat PATH). Detect the common | |
| # setup and say so now, at install time, instead of letting the first `gl` | |
| # print git's baffling "fatal: not a git repository". | |
| if [ -f "$HOME/.zshrc" ] && [ -d "$HOME/.oh-my-zsh/plugins/git" ] \ | |
| && grep -Eq '^[[:space:]]*plugins=\(([^)]*[[:space:]])?git([[:space:]][^)]*)?\)' "$HOME/.zshrc" \ | |
| && ! grep -Eq '^([^#]*[[:space:];&|])?unalias[[:space:]]+([^;&|#]*[[:space:]])?gl([[:space:];&|]|$)' "$HOME/.zshrc"; then | |
| echo "" | |
| echo "NOTE: oh-my-zsh's git plugin aliases gl='git pull', which will shadow" | |
| echo "the gl command in interactive shells. To use gl by name, run:" | |
| echo "" | |
| echo " echo 'unalias gl 2>/dev/null' >> ~/.zshrc && source ~/.zshrc" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` around lines 196 - 208, Update the negative unalias check in the
install-time oh-my-zsh detection block to treat `;`, `&`, and `|` immediately
following `gl` as valid command terminators, matching `doctor.rs`’s
`rc_unaliases_gl` behavior. Preserve the existing handling for whitespace,
comments, and end-of-line so an existing `unalias gl` suppresses the NOTE in all
supported command-separator forms.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Approving. The signature-verification path is the substantive change here and it holds up. I rebuilt the node's canonical signing payload on both sides: gl reconstructs the identical 7-field form the node signs, issued_at is stored as TEXT so it round-trips byte-exact, and the single-cert response carries every field the verifier reads. --verify fails closed on every case I threw at it (bad signature, missing or "?" node DID, an unreachable node with no --expect-node, a hostile self-signed cert, malformed base64 or DID input), none of which panic. 299 gl tests pass, and the new tamper test goes red when I stub the verifier to always succeed, so it is load-bearing.
Four P3 polish items below, none blocking, all yours to take or leave.
Findings
-
[P3] Qualify the
VALIDline so a scripted reader can't misread it.crates/gl/src/cert.rs:184— under--verify,VALIDprints to stdout before the issuer-anchor check can bail, so a valid-signature/untrusted-issuer cert showsVALIDon stdout while the command exits non-zero. The exit code is correct and authoritative, but a consumer doinggl cert show ... --verify | grep -q VALIDwould be misled. Either defer the line until after the anchor passes, or label itVALID (signature only). -
[P3] Don't suggest
git switch -c mainwhenmainalready exists.crates/gl/src/init.rs:213— the(None, _)arm always printsgit switch -c main, which fails with "a branch named 'main' already exists" on a detached HEAD in a repo that already hasmain(checkout an old commit or tag, thengl init). It is a printed hint, not an executed command, so recovery is trivial, but the instruction is wrong in a real scenario. Checkrefs/heads/mainfirst and printgit switch mainwhen it exists. This is the open CodeRabbit thread. -
[P3] Fall back to an assumed scheme in
is_loopback_url.crates/gl/src/doctor.rs:345—Url::parseneeds a scheme, so a scheme-lessGITLAWB_NODE=127.0.0.1:7545returns false and the doctor reports a bare loopback address as an ordinary remote node. The documentedhttp://127.0.0.1:7545form is detected correctly and this change fixes the reallocalhost.examplefalse positives, so it is net positive; retrying the parse ashttp://{value}closes the remaining shorthand gap. -
[P3] Match the shell unalias regex to the Rust splitter.
install.sh:202vsrc_unaliases_glincrates/gl/src/doctor.rs— the shell regex requires whitespace or end-of-line aftergl, sounalias gl;is not matched and the install NOTE prints even thoughglis already unaliased.rc_unaliases_glsplits on;&|first and handles it, so the two drift. Extending the trailing class to([[:space:];&|]|$)realigns them. This is the other open CodeRabbit thread.
2 participants
Summary
Fixes the four CLI issues surfaced when a fresh agent, given only the published docs, drove gl from zero to a signed push: status not seeing gl init's remote, init's push hint failing on uncommitted repos, cert show not actually verifying, and doctor red-flagging healthy local nodes.
Motivation & context
A docs-only acceptance test (agent + local harness) reached a verified push but hit exactly these traps; the docs currently carry caveats working around each. Closes #.
Kind of change
What changed
gl(status.rs): detect_gitlawb_remote scans all remotes — origin first (clones), thengitlawb(the name gl init adds), then any gitlawb:// URL.gl(init.rs):git init -b mainwith plain-init fallback for git <2.28; the closing hint is state-aware — with no commits it prints the add/commit step before the push, using the repo's actual branch name.gl(cert.rs):cert showrebuilds the node's canonical signing payload, derives the Ed25519 key from the certificate's node DID, and prints a VALID/INVALID verdict;--verifymakes the exit code reflect it. Round-trip, tampered-payload, and malformed-signature unit tests.gl(doctor.rs): a localhost GITLAWB_NODE passes with an explanatory note instead of failing — the connectivity check still fails loudly when the configured node is unreachable.How a reviewer can verify
Before you request review
Protocol & signing impact
Notes for reviewers
Verified functionally against a local node: all four flows exercised end-to-end, including a real VALID verdict on a live certificate (proving the payload reconstruction matches node signing exactly). After this ships in 0.6.1, the docs lose three workaround caveats and skill.md's cert section gets corrected (it documents a cert verify subcommand that has never existed).
Validation recap: four per-fix commits; full workspace gate green (fmt, clippy
-D warnings, all tests incl. DB suites against Postgres); functional verification against a live node — the init hint, main branch, status detection, doctor note, and a realVALIDEd25519 verdict with exit 0.Also notable: this is the first PR through your armed merge gate, so expect the four required checks to show as Required — worth confirming they do. After merge: release-please opens 0.6.1 → merging it runs the first fully-hands-free multi-channel release → then I do the docs cleanup pass (drop the status/branch caveats, fix the phantom
cert verify <cert-file>in skill.md, restore "verify withgl cert show --verify"). CodeRabbit findings are mine to address when they arrive.Summary by CodeRabbit
New Features
--verifytogl cert showfor offline Ed25519 signature verification withVALID/INVALID.--expect-nodetogl cert show(requires--verify) to enforce issuer anchoring.gl initnow initializes withmainwhen available and prints branch/HEAD-aware next steps.Bug Fixes
gl statusdetection ofgitlawb://remotes across all remotes/URL variants and updated the no-remote message.gl doctornow treats loopbackGITLAWB_NODEvalues as intentional and detects zsh alias shadowing forgl.Documentation
gl='git pull'alias shadowing and suggestunalias gl.Tests
gl doctoralias/loopback detection.