Skip to content

Conversation

@SWAROOP323
Copy link
Contributor

@SWAROOP323 SWAROOP323 commented Dec 21, 2025

This PR adds Git integration for Cortex configuration management.

Features:

  • Initialize Git repository for configs
  • Auto-commit configuration changes
  • View configuration history
  • Roll back to specific commits
  • Cross-platform config directory handling
  • Unit tests with 88% coverage
  • Documentation for Git workflow

Example usage:

  • cortex config git init
  • cortex config git commit "Update config"
  • cortex config history
  • cortex config rollback

DEMO VIDEO :

Screen recording demonstrating Git-based configuration management:
https://drive.google.com/file/d/1b3W5-nUI7kZj9Yqw-MOr6YgSJZkqGxRJ/view?usp=sharing

Closes cortexlinux/cortex-ops#20

Summary by CodeRabbit

  • New Features

    • Added Git-backed configuration management with a top-level config command group.
    • New subcommands: config history, config rollback <commit>, and config git with init and commit <message>.
    • CLI provides user-facing feedback for init, commit, history, and rollback operations.
  • Tests

    • Added unit tests covering repo init, commit behavior, history retrieval, and rollback.

✏️ Tip: You can customize this high-level summary in your review settings.

Copilot AI review requested due to automatic review settings December 21, 2025 18:44
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 21, 2025

Warning

Rate limit exceeded

@Anshgrover23 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 6 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 8d066dd and 48e3ff2.

📒 Files selected for processing (2)
  • cortex/cli.py
  • cortex/config/git_manager.py
📝 Walkthrough

Walkthrough

Adds a Git-backed configuration management feature to the CLI: a new top-level config command group with subcommands (git init, git commit <message>, history, rollback <commit>). Introduces GitManager to manage a local git repo at ~/.cortex/configs and unit tests for its behavior.

Changes

Cohort / File(s) Summary
CLI Integration
cortex/cli.py
Added top-level config command group with subcommands: git init, git commit <message>, history, and rollback <commit>. Instantiates GitManager targeting ~/.cortex/configs, handles repo creation, commits, history output, and rollback user messaging.
Git Repository Manager
cortex/config/git_manager.py
New GitManager class: init_repo() initializes repo, commit_all(message) stages & commits (handles "nothing to commit"), history() returns one-line relative-date log (returns empty on no commits), rollback(commit_hash) checks out commit for all files. Uses Path and subprocess calls scoped to config path.
Unit Tests
tests/test_git_manager.py
New tests mocking subprocess.run to validate repo init (creates/exists), commit success/no-changes, empty history handling, and rollback invocation. Tests use tmp_path and patched subprocess behavior.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI
    participant GitManager
    participant Git

    User->>CLI: cortex config git init
    CLI->>GitManager: init_repo()
    GitManager->>Git: git init (cwd=config_path)
    Git-->>GitManager: success / already exists
    GitManager-->>CLI: True / False
    CLI-->>User: ✓ Repository initialized / exists

    User->>CLI: cortex config git commit "msg"
    CLI->>GitManager: commit_all("msg")
    GitManager->>Git: git add . (cwd=config_path)
    Git-->>GitManager: success
    GitManager->>Git: git commit -m "msg"
    Git-->>GitManager: success / "nothing to commit"
    GitManager-->>CLI: True / False
    CLI-->>User: ✓ Changes committed / ✓ No changes to commit

    User->>CLI: cortex config history
    CLI->>GitManager: history()
    GitManager->>Git: git log --oneline --relative-date
    Git-->>GitManager: log output / error
    GitManager-->>CLI: history string / ""
    CLI-->>User: Display commit history

    User->>CLI: cortex config rollback <commit>
    CLI->>GitManager: rollback("<commit>")
    GitManager->>Git: git checkout <commit> -- .
    Git-->>GitManager: success / error
    GitManager-->>CLI: None / raise
    CLI-->>User: ✓ Rolled back / error message
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped to init and made a commit,

History stitched in branches neat,
Rollback bounds with nimble feet,
Configs saved in tidy rows,
A rabbit guards where version grows.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR implements core features (init, commit, history, rollback) but omits two requirement areas: branch management and remote repository sync (push/pull) specified in issue cortexlinux/cortex-ops#20. Implement branch management support and remote repository sync (push/pull) functionality as required by issue cortexlinux/cortex-ops#20 acceptance criteria, or clarify if these are deferred to future PRs.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive The PR description covers features and usage examples but lacks required sections: 'Related Issue' number formatting, explicit confirmation of test pass status, and MVP label guidance per template. Restructure description to match template exactly: add '## Related Issue' with 'Closes cortexlinux/cortex-ops#20' formatted as required, include test pass confirmation checkbox, and clarify MVP label status if applicable.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add Git-based configuration management' clearly and concisely summarizes the main change—introducing Git integration for managing Cortex configurations.
Out of Scope Changes check ✅ Passed All changes (GitManager class, CLI integration, tests) are directly aligned with issue cortexlinux/cortex-ops#20 Git integration requirements; no out-of-scope modifications detected.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 9

🧹 Nitpick comments (1)
cortex/cli.py (1)

835-842: Clarify help text for rollback subcommand.

The config rollback command may be confused with cortex rollback (which rolls back package installations). Add clarifying help text to distinguish these commands.

🔎 Proposed fix
     # config rollback
     rollback_parser = config_subparsers.add_parser(
         "rollback",
-        help="Rollback configuration to a specific commit"
+        help="Restore configuration files to a specific commit (not package installations)"
     )
     rollback_parser.add_argument(
         "commit",
-        help="Git commit hash"
+        help="Git commit hash (use 'cortex config history' to see commits)"
     )
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f6bfa49 and 5260339.

📒 Files selected for processing (3)
  • cortex/cli.py (3 hunks)
  • cortex/config/git_manager.py (1 hunks)
  • tests/test_git_manager.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/config/git_manager.py
  • cortex/cli.py
  • tests/test_git_manager.py
tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Maintain >80% test coverage for pull requests

Files:

  • tests/test_git_manager.py
🧬 Code graph analysis (2)
cortex/cli.py (1)
cortex/config/git_manager.py (3)
  • GitManager (4-58)
  • init_repo (8-17)
  • commit_all (18-39)
tests/test_git_manager.py (1)
cortex/config/git_manager.py (3)
  • GitManager (4-58)
  • init_repo (8-17)
  • commit_all (18-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Agent
🔇 Additional comments (1)
cortex/cli.py (1)

819-869: Consider adding config edit command mentioned in PR objectives.

The PR objectives mention cortex config edit nginx which would auto-commit changes, but this command is not implemented. The current implementation only provides git subcommands. Consider whether this gap is intentional or if the edit workflow should be added.

The PR description shows example usage cortex config edit nginx → auto-commit with message, but I don't see this command implemented. Should this be added, or was the example aspirational for future work?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds Git-based configuration management to Cortex, enabling version control for system configurations through CLI commands. The implementation introduces a GitManager class to wrap Git operations and integrates it into the CLI with commands for initializing repositories, committing changes, viewing history, and rolling back to previous commits.

Key changes:

  • New GitManager class for Git operations (init, commit, history, rollback)
  • CLI integration with cortex config command and subcommands (git init, git commit, history, rollback)
  • Unit tests covering basic GitManager functionality with 88% coverage

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 22 comments.

File Description
cortex/config/git_manager.py New GitManager class implementing Git operations for configuration management via subprocess calls
cortex/cli.py Added config command parser and handler logic integrating GitManager into the CLI workflow
tests/test_git_manager.py Unit tests for GitManager with mocked subprocess calls covering basic success and failure scenarios
Comments suppressed due to low confidence (1)

tests/test_git_manager.py:2

  • Import of 'Path' is not used.
from pathlib import Path

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

)

result = subprocess.run(
["git", "commit", "-m", message],
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

Potential command injection vulnerability. The message parameter is passed directly to subprocess without sanitization. While passing it as a list argument provides some protection, special characters or escape sequences in the commit message could still cause issues. Consider validating or sanitizing the message parameter.

Copilot uses AI. Check for mistakes.
@Suyashd999
Copy link
Collaborator

Suyashd999 commented Dec 21, 2025

Hello @SWAROOP323 Please add a screen recording of the working feature in the PR description

Make sure this is included in the video:

  1. Git repository initialization
  2. Auto-commit config changes
  3. Track change history
  4. Branch management
  5. Rollback to specific commits
  6. Remote repository push/pull

@Suyashd999 Suyashd999 requested review from Suyashd999 and removed request for mikejmorgan-ai December 21, 2025 19:33
@SWAROOP323
Copy link
Contributor Author

Hi @Suyashd999 👋
I’ve added a screen recording (Google Drive link) demonstrating:

  • Git repository initialization
  • Auto-commit config changes
  • Configuration history
  • Branch management
  • Rollback to specific commits
  • Remote repository handling

Video link: https : https://drive.google.com/file/d/1b3W5-nUI7kZj9Yqw-MOr6YgSJZkqGxRJ/view?usp=sharing
Please let me know if this works or if you’d prefer another format.
Thanks!

@SWAROOP323
Copy link
Contributor Author

@Suyashd999 can you review this pr

@github-actions
Copy link

github-actions bot commented Dec 30, 2025

CLA Verification Passed

All contributors have signed the CLA.

Contributor Signed As
@SWAROOP323 @SWAROOP323
@SWAROOP323 @SWAROOP323
@Anshgrover23 @Anshgrover23

Copy link
Collaborator

@Anshgrover23 Anshgrover23 left a comment

Choose a reason for hiding this comment

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

@SWAROOP323 Can u create a seperate PR for CLA-signature, just follow this reference PR #401 , thanks.

@Anshgrover23
Copy link
Collaborator

@SWAROOP323 Update your branch.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (1)
cortex/changelog/parser.py (1)

7-7: Consider a more descriptive variable name.

The variable lower is not very descriptive. Consider renaming it to change_lower or normalized_change to better convey its purpose.

🔎 Suggested improvement
     for change in entry["changes"]:
-        lower = change.lower()
-        if "cve" in lower or "security" in lower:
+        change_lower = change.lower()
+        if "cve" in change_lower or "security" in change_lower:
             security.append(change)
-        elif "bug" in lower or "fix" in lower:
+        elif "bug" in change_lower or "fix" in change_lower:
             bugs.append(change)
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8ec22ba and cd07900.

📒 Files selected for processing (2)
  • cortex/changelog/fetchers.py
  • cortex/changelog/parser.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/changelog/parser.py
  • cortex/changelog/fetchers.py
🔇 Additional comments (1)
cortex/changelog/fetchers.py (1)

1-21: This file appears unrelated to the PR's stated objectives.

The PR objectives describe adding Git-based configuration management (repository initialization, auto-commit, history, rollback, branch management, remote sync). However, this file implements changelog fetching functionality for Docker packages, which is not mentioned in the PR description, linked issue #100, or any comments. Please clarify how this changelog feature relates to the Git configuration management feature.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (3)
cortex/cli.py (3)

1666-1692: Refactor: Use elif for consistent conditional structure.

Lines 1666, 1674, 1682, and 1689 use independent if statements that all return, making subsequent checks redundant. This was previously flagged and should use elif for better readability and consistency with the rest of the codebase.

🔎 Proposed refactor
-            if args.subcommand == "git" and args.action == "init":
+            if args.subcommand == "git" and args.action == "init":
                 created = gm.init_repo()
                 if created:
                     print("✓ Git repository initialized")
                 else:
                     print("✓ Git repository already exists")
                 return 0
 
-            if args.subcommand == "git" and args.action == "commit":
+            elif args.subcommand == "git" and args.action == "commit":
                 committed = gm.commit_all(args.message)
                 if committed:
                     print(f"✓ Auto-committed: \"{args.message}\"")
                 else:
                     print("ℹ️ No changes to commit")
                 return 0
 
-            if args.subcommand == "history":
+            elif args.subcommand == "history":
                 output = gm.history()
                 if output:
                     print(output)
                 else:
                     print("No configuration history found")
                 return 0
                 
-            if args.subcommand == "rollback":
+            elif args.subcommand == "rollback":
                 gm.rollback(args.commit)
                 print(f"✓ Rolled back to commit {args.commit}")
                 return 0
+            else:
+                print("Unknown config command")
+                return 1
-
-            print("Unknown config command")
-            return 1

1679-1679: Inconsistent emoji usage.

Line 1679 uses "ℹ" without the variation selector, while the codebase typically uses "ℹ️". This was previously flagged for consistency.

-                    print("ℹ No changes to commit")
+                    print("ℹ️ No changes to commit")

1660-1696: Critical: Add comprehensive error handling for Git operations.

The config command handler lacks error handling for several failure scenarios:

  1. Git not installed: GitManager will raise FileNotFoundError if Git is not available, but this isn't caught
  2. Git operations fail: Subprocess failures from init_repo(), commit_all(), history(), and rollback() can raise CalledProcessError or RuntimeError
  3. Destructive operation without confirmation: Line 1690 performs rollback without asking the user to confirm

These issues were flagged in previous reviews and remain unaddressed.

🔎 Proposed fix with error handling and confirmation
         elif args.command == "config":
             config_dir = Path.home() / ".cortex" / "configs"
             config_dir.mkdir(parents=True, exist_ok=True)
 
-            gm = GitManager(str(config_dir))
+            try:
+                gm = GitManager(str(config_dir))
+            except (ValueError, FileNotFoundError) as e:
+                cli._print_error(f"Failed to initialize Git manager: {e}")
+                return 1
 
             if args.subcommand == "git" and args.action == "init":
-                created = gm.init_repo()
-                if created:
-                    print("✓ Git repository initialized")
-                else:
-                    print("✓ Git repository already exists")
-                return 0
+                try:
+                    created = gm.init_repo()
+                    if created:
+                        print("✓ Git repository initialized")
+                    else:
+                        print("✓ Git repository already exists")
+                    return 0
+                except FileNotFoundError:
+                    cli._print_error("Git is not installed")
+                    cx_print("Install git: sudo apt-get install git", "info")
+                    return 1
+                except Exception as e:
+                    cli._print_error(f"Failed to initialize repository: {e}")
+                    return 1
 
             if args.subcommand == "git" and args.action == "commit":
-                committed = gm.commit_all(args.message)
-                if committed:
-                    print(f"✓ Auto-committed: \"{args.message}\"")
-                else:
-                    print("ℹ No changes to commit")
-                return 0
+                try:
+                    committed = gm.commit_all(args.message)
+                    if committed:
+                        print(f"✓ Auto-committed: \"{args.message}\"")
+                    else:
+                        print("ℹ️ No changes to commit")
+                    return 0
+                except Exception as e:
+                    cli._print_error(f"Failed to commit: {e}")
+                    return 1
 
             if args.subcommand == "history":
-                output = gm.history()
-                if output:
-                    print(output)
-                else:
-                    print("No configuration history found")
-                return 0
+                try:
+                    output = gm.history()
+                    if output:
+                        print(output)
+                    else:
+                        print("No configuration history found")
+                    return 0
+                except Exception as e:
+                    cli._print_error(f"Failed to retrieve history: {e}")
+                    return 1
+                    
             if args.subcommand == "rollback":
-                gm.rollback(args.commit)
-                print(f"✓ Rolled back to commit {args.commit}")
-                return 0
+                # Confirm destructive action
+                cx_print(f"⚠️  This will restore configs to commit {args.commit}", "warning")
+                confirm = input("Continue? (y/n): ")
+                if confirm.lower() != "y":
+                    print("Rollback cancelled")
+                    return 0
+                
+                try:
+                    gm.rollback(args.commit)
+                    print(f"✓ Rolled back to commit {args.commit}")
+                    return 0
+                except Exception as e:
+                    cli._print_error(f"Rollback failed: {e}")
+                    return 1
 
             print("Unknown config command")
             return 1
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cd07900 and 034d699.

📒 Files selected for processing (1)
  • cortex/cli.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/config/git_manager.py (3)
  • GitManager (4-58)
  • init_repo (8-17)
  • commit_all (18-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: test (3.12)
  • GitHub Check: test (3.10)
  • GitHub Check: test (3.11)
🔇 Additional comments (2)
cortex/cli.py (2)

7-7: LGTM: Imports are appropriate.

The new imports (Path and GitManager) are necessary for the Git-based configuration management feature and are used correctly in the command handler.

Also applies to: 12-12


1401-1452: Argument parser structure is correct.

The command structure and argument definitions are well-organized and clear. Help text appropriately describes each subcommand.

Note: There's a potential UX concern with config rollback vs. cortex rollback naming that could confuse users, but this was previously flagged and is a design choice rather than a code defect.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (11)
tests/test_git_manager.py (6)

1-1: Remove unused import.

Path is imported but never used. The tmp_path fixture already provides Path objects.

-from pathlib import Path

7-7: Patch subprocess at the correct module location.

The patch should target "cortex.config.git_manager.subprocess.run" rather than "subprocess.run" to mock the subprocess module as imported in the GitManager module.

-@patch("subprocess.run")
+@patch("cortex.config.git_manager.subprocess.run")

Also applies to: 13-13, 20-20, 28-28, 36-36, 43-43


20-25: Mock both subprocess calls in commit_all.

commit_all calls subprocess.run twice (git add then git commit), but only a single return value is configured. Use side_effect to return different results for each call.

 @patch("cortex.config.git_manager.subprocess.run")
 def test_commit_all_success(mock_run, tmp_path):
-    mock_run.return_value.returncode = 0
-    mock_run.return_value.stdout = "Committed"
+    from unittest.mock import MagicMock
+    add_result = MagicMock(returncode=0)
+    commit_result = MagicMock(returncode=0, stdout="[main abc123] Test commit", stderr="")
+    mock_run.side_effect = [add_result, commit_result]
+    
     gm = GitManager(str(tmp_path))
     assert gm.commit_all("Test commit") is True
+    assert mock_run.call_count == 2

28-33: Mock both subprocess calls for no-changes scenario.

Same issue: needs to mock both git add and git commit calls with appropriate return values.

 @patch("cortex.config.git_manager.subprocess.run")
 def test_commit_all_no_changes(mock_run, tmp_path):
-    mock_run.return_value.returncode = 1
-    mock_run.return_value.stdout = "nothing to commit"
+    from unittest.mock import MagicMock
+    add_result = MagicMock(returncode=0)
+    commit_result = MagicMock(returncode=1, stdout="nothing to commit, working tree clean", stderr="")
+    mock_run.side_effect = [add_result, commit_result]
+    
     gm = GitManager(str(tmp_path))
     assert gm.commit_all("No changes") is False

43-47: Assert rollback is called with correct arguments.

The test only verifies subprocess.run was called, not that it was called with the correct git checkout command and commit hash.

 @patch("cortex.config.git_manager.subprocess.run")
 def test_rollback(mock_run, tmp_path):
     gm = GitManager(str(tmp_path))
     gm.rollback("abc123")
-    mock_run.assert_called()
+    mock_run.assert_called_once_with(
+        ["git", "checkout", "abc123", "--", "."],
+        cwd=tmp_path,
+        check=True
+    )

1-47: Add tests for error conditions and edge cases.

The test suite is missing coverage for:

  • Git not installed (FileNotFoundError)
  • RuntimeError when commit fails with actual errors
  • Invalid commit hash in rollback (CalledProcessError)
  • Successful history retrieval

Based on coding guidelines, >80% test coverage is required.

🔎 Suggested additional tests
@patch("cortex.config.git_manager.subprocess.run")
def test_commit_all_raises_on_error(mock_run, tmp_path):
    from unittest.mock import MagicMock
    add_result = MagicMock(returncode=0)
    commit_result = MagicMock(returncode=128, stdout="", stderr="fatal: bad object HEAD")
    mock_run.side_effect = [add_result, commit_result]
    
    gm = GitManager(str(tmp_path))
    with pytest.raises(RuntimeError, match="fatal: bad object HEAD"):
        gm.commit_all("Test")


@patch("cortex.config.git_manager.subprocess.run")
def test_history_success(mock_run, tmp_path):
    from unittest.mock import MagicMock
    result = MagicMock(returncode=0, stdout="abc123 Initial commit\ndef456 Add config")
    mock_run.return_value = result
    
    gm = GitManager(str(tmp_path))
    assert gm.history() == "abc123 Initial commit\ndef456 Add config"


@patch("cortex.config.git_manager.subprocess.run")
def test_init_repo_git_not_installed(mock_run, tmp_path):
    mock_run.side_effect = FileNotFoundError("git not found")
    gm = GitManager(str(tmp_path))
    with pytest.raises(FileNotFoundError):
        gm.init_repo()
cortex/config/git_manager.py (5)

5-7: Add class docstring and validate config_path.

The class lacks a docstring (required per coding guidelines) and doesn't validate that config_path exists or is a directory.

🔎 Proposed fix
 class GitManager:
+    """Manages Git operations for configuration files.
+
+    Args:
+        config_path: Path to the configuration directory to manage with Git.
+
+    Raises:
+        ValueError: If config_path doesn't exist or isn't a directory.
+    """
+
     def __init__(self, config_path: str):
         self.config_path = Path(config_path)
+        if not self.config_path.exists():
+            raise ValueError(f"Config path does not exist: {config_path}")
+        if not self.config_path.is_dir():
+            raise ValueError(f"Config path is not a directory: {config_path}")

As per coding guidelines, docstrings are required for all public APIs.


9-18: Add return type hint and docstring to init_repo.

Missing return type annotation (-> bool) and docstring. Also needs a blank line before the method definition per PEP 8.

🔎 Proposed fix
+
+    def init_repo(self) -> bool:
+        """Initialize a Git repository in the config directory.
+
+        Returns:
+            True if a new repository was created, False if one already exists.
+
+        Raises:
+            FileNotFoundError: If git is not installed.
+            subprocess.CalledProcessError: If git init fails.
+        """
-    def init_repo(self):
         if (self.config_path / ".git").exists():
             return False

As per coding guidelines, type hints and docstrings are required for all public APIs.


19-40: Add docstring, configure Git user, and fix output checking.

This method has multiple issues:

  1. Missing docstring and blank line before method
  2. Git requires user.name and user.email configured before committing—fails on fresh systems
  3. The "nothing to commit" check only inspects stdout, but some Git versions output to stderr
🔎 Proposed fix
+
     def commit_all(self, message: str) -> bool:
+        """Stage and commit all changes in the config directory.
+
+        Args:
+            message: The commit message.
+
+        Returns:
+            True if changes were committed, False if nothing to commit.
+
+        Raises:
+            RuntimeError: If the commit fails for reasons other than nothing to commit.
+        """
+        # Ensure git user is configured for this repo
+        subprocess.run(
+            ["git", "config", "user.name", "Cortex"],
+            cwd=self.config_path,
+            check=True
+        )
+        subprocess.run(
+            ["git", "config", "user.email", "cortex@localhost"],
+            cwd=self.config_path,
+            check=True
+        )
+
         subprocess.run(
             ["git", "add", "."],
             cwd=self.config_path,
             check=True
         )
 
         result = subprocess.run(
             ["git", "commit", "-m", message],
             cwd=self.config_path,
             capture_output=True,
             text=True,
             check=False
         )
 
-        if "nothing to commit" in result.stdout.lower():
+        combined_output = (result.stdout + result.stderr).lower()
+        if "nothing to commit" in combined_output:
             return False

As per coding guidelines, docstrings are required for all public APIs.


41-53: Fix invalid git flag and add docstring.

--relative-date is not a valid git log flag. The correct flag is --date=relative. Also missing docstring and blank line before method.

🔎 Proposed fix
+
     def history(self) -> str:
+        """Retrieve the commit history for the config repository.
+
+        Returns:
+            A formatted string of commit history, or empty string if no commits exist.
+        """
         result = subprocess.run(
-            ["git", "log", "--oneline", "--relative-date"],
+            ["git", "log", "--oneline", "--date=relative"],
             cwd=self.config_path,
             capture_output=True,
-            text=True
+            text=True,
+            check=False
         )

54-59: Validate commit hash and add docstring.

The rollback method performs a destructive checkout without validating the commit hash exists. Missing docstring and blank line before method.

🔎 Proposed fix
+
     def rollback(self, commit_hash: str) -> None:
+        """Roll back configuration to a specific commit.
+
+        Args:
+            commit_hash: The Git commit hash to roll back to.
+
+        Raises:
+            RuntimeError: If the commit doesn't exist.
+            subprocess.CalledProcessError: If rollback fails.
+        """
+        # Validate commit exists
+        verify = subprocess.run(
+            ["git", "cat-file", "-e", commit_hash],
+            cwd=self.config_path,
+            capture_output=True
+        )
+        if verify.returncode != 0:
+            raise RuntimeError(f"Commit {commit_hash} not found")
+
         subprocess.run(
             ["git", "checkout", commit_hash, "--", "."],
             cwd=self.config_path,
             check=True
         )

As per coding guidelines, docstrings are required for all public APIs.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 034d699 and 8d066dd.

📒 Files selected for processing (2)
  • cortex/config/git_manager.py
  • tests/test_git_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/config/git_manager.py
  • tests/test_git_manager.py
tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Maintain >80% test coverage for pull requests

Files:

  • tests/test_git_manager.py
🧠 Learnings (2)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Docstrings required for all public APIs

Applied to files:

  • cortex/config/git_manager.py
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to tests/**/*.py : Maintain >80% test coverage for pull requests

Applied to files:

  • tests/test_git_manager.py
🧬 Code graph analysis (1)
tests/test_git_manager.py (1)
cortex/config/git_manager.py (5)
  • GitManager (5-59)
  • init_repo (9-18)
  • commit_all (19-40)
  • history (41-53)
  • rollback (54-59)
🪛 GitHub Actions: CI
cortex/config/git_manager.py

[error] 1-1: Black formatting needed. 2 files would be reformatted by Black.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: test (3.10)
  • GitHub Check: test (3.11)
  • GitHub Check: test (3.12)

@SWAROOP323
Copy link
Contributor Author

@SWAROOP323 Update your branch.

@Anshgrover23 The branch has been updated.
Could you please review now?

Copy link
Collaborator

@Anshgrover23 Anshgrover23 left a comment

Choose a reason for hiding this comment

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

@SWAROOP323 Follow Contributing.md guideliness i.e add a demonstration video.

check=True
)
return True
def commit_all(self, message: str) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 7, 2026

Copy link
Collaborator

@Anshgrover23 Anshgrover23 left a comment

Choose a reason for hiding this comment

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

@SWAROOP323 PR doesn’t follow contributing.md (AI usage disclosure, docs, and demo video are missing). Please update these and convert the PR to draft until then.
Also resolve conflicts.

@Anshgrover23 Anshgrover23 marked this pull request as draft January 8, 2026 19:45
@SWAROOP323 SWAROOP323 closed this Jan 10, 2026
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