Skip to content

fix: downgrade russh version for compile err#1822

Merged
genedna merged 1 commit into
gitmono-dev:mainfrom
benjamin-747:main
Jan 20, 2026
Merged

fix: downgrade russh version for compile err#1822
genedna merged 1 commit into
gitmono-dev:mainfrom
benjamin-747:main

Conversation

@benjamin-747

Copy link
Copy Markdown
Collaborator

No description provided.

@benjamin-747 benjamin-747 requested a review from genedna January 20, 2026 11:01
@github-actions

github-actions Bot commented Jan 20, 2026

Copy link
Copy Markdown

Claude finished @benjamin-747's task in 1m 59s —— View job


Code Review Complete

Tasks

  • Read Cargo.toml changes
  • Analyze the version downgrade
  • Check CI/build status
  • Review for security implications
  • Provide comprehensive feedback

Summary

This PR downgrades russh from 0.56.0 to 0.55.0 and upgrades chrono from 0.4.42 to 0.4.43 to fix a compilation error. After reviewing the changes, I have several concerns and recommendations.

Issues Found

🔴 Critical: Incomplete PR Description

Issue: The PR has no description explaining what compilation error was encountered or why this specific fix was chosen.

Impact:

  • Makes it difficult for reviewers to understand the problem
  • No documentation of the error for future reference
  • Cannot verify if this is the correct fix without knowing the error

Recommendation: Please add a PR description that includes:

  1. The exact compilation error message
  2. Why russh 0.56.0 causes the error
  3. Whether this is a temporary workaround or permanent solution
  4. Any upstream issues or tracking tickets related to this

🟡 Medium: Misleading PR Title

Issue: The PR title says "downgrade russh version" but the PR also includes a chrono version upgrade (0.4.42 → 0.4.43).

Location: Cargo.toml:79

Impact:

  • The chrono change is unmentioned and unexplained
  • Reviewers might miss this change
  • Unclear if chrono upgrade is related to the fix or incidental

Recommendation: Either:

  1. Update the PR title to reflect both changes: "fix: downgrade russh and update chrono for compile error"
  2. Or explain in the PR description why chrono was also changed

🟡 Medium: Potential Security/Stability Concerns

Issue: Downgrading dependencies can reintroduce fixed bugs or security vulnerabilities.

Context:

  • russh is used for SSH server functionality (mono/src/server/ssh_server.rs:9-13)
  • SSH servers handle authentication and are security-critical
  • The project uses russh for public key authentication (mono/src/git_protocol/ssh.rs:123-148)

Recommendation:

  1. Check the russh 0.56.0 changelog for any security fixes or important bug fixes that this downgrade reverts
  2. If 0.56.0 contains important fixes, consider:
    • Fixing the compilation error in your code instead of downgrading
    • Opening an issue with russh maintainers if it's a breaking change
    • Adding a TODO comment in Cargo.toml to upgrade back once the issue is resolved

🟡 Medium: Missing Cargo.lock

Issue: No Cargo.lock file is committed, making dependency resolution non-deterministic.

Impact:

  • Different developers/CI runs might resolve to different transitive dependency versions
  • Makes it harder to reproduce builds
  • Security audit tools can't analyze exact dependency tree

Recommendation: Consider committing Cargo.lock for reproducible builds, especially for applications (not libraries).

🔵 Minor: Version Downgrade History

Observation: Looking at git history, russh has been upgraded and downgraded multiple times:

  • bfc3296: Set russh version to 0.52.0
  • 7b3f22e: Upgrade russh version
  • ed5185a: Update russh from 0.51.1 to 0.52.0
  • db01739: (implicitly upgraded to 0.56.0)
  • a0c586d: (this PR) Downgrade to 0.55.0

Recommendation: Consider documenting why certain russh versions are incompatible to prevent future upgrade attempts.

Testing

❓ Test Coverage

Questions:

  1. Have you verified that SSH authentication still works correctly with russh 0.55.0?
  2. Have you tested both git operations (upload-pack and receive-pack) over SSH?
  3. Has the build been tested on all target platforms?

Recommendation: Add a comment confirming manual testing was performed.

Documentation

📝 Missing Context

Issue: No inline comments or documentation explaining:

  • Why this specific version is required
  • What breaks in 0.56.0
  • Exit criteria for upgrading back

Recommendation: Add a comment in Cargo.toml:

# Note: russh 0.56.0 causes compilation errors
# TODO: Investigate and upgrade when compatible
russh = "0.55.0"

CI Status

The CI checks are currently running:

  • Base GitHub Action for Check, Test and Lints: ⏳ Queued
  • CodeQL Advanced: ⏳ In Progress
  • Test with Services: ⏳ In Progress

Recommendation: Wait for CI to complete before merging to ensure the downgrade actually fixes the compilation error.

Verdict

⚠️ Changes Requested - Please address:

  1. Required: Add a PR description with the compilation error details
  2. Required: Explain the chrono version change
  3. Recommended: Check russh 0.56.0 changelog for security implications
  4. Recommended: Wait for CI to pass
  5. Recommended: Add inline comment in Cargo.toml documenting the version constraint

Positive Notes

✅ The version constraints are specific (not using wildcard versions)
✅ Clean, focused change
✅ Follows conventional commit format in the title


@genedna genedna added this pull request to the merge queue Jan 20, 2026
Merged via the queue into gitmono-dev:main with commit 95a6e97 Jan 20, 2026
11 checks passed
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.

2 participants