Add installation history tracking and rollback support #198
Conversation
WalkthroughThis PR introduces a complete Installation History and Rollback System for Cortex Linux, comprising a new database-backed module for tracking installations, CLI integration for viewing history and executing rollbacks, comprehensive tests, and documentation detailing features, usage, and data models. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CortexCLI
participant History as InstallationHistory
participant DB as SQLite DB
rect rgb(230, 245, 255)
Note over User,DB: Installation with History Tracking
User->>CLI: install package
CLI->>History: record_installation(START)
History->>DB: INSERT installations (in_progress)
History-->>CLI: install_id
CLI->>CLI: execute package install
alt Success
CLI->>History: update_installation(SUCCESS)
History->>DB: UPDATE installations (after_snapshot, status)
CLI-->>User: ✅ Complete (ID: xyz)
else Failure
CLI->>History: update_installation(FAILED, error)
History->>DB: UPDATE installations (error_message, status)
CLI-->>User: ❌ Failed (ID: xyz)
end
end
rect rgb(255, 240, 245)
Note over User,DB: Rollback Operation
User->>CLI: rollback install_id [--dry-run]
CLI->>History: rollback(install_id, dry_run)
History->>DB: FETCH installation record
History->>History: compare before/after snapshots
alt Dry-run
History-->>CLI: rollback plan (no-op)
else Execute
History->>History: execute reverse operations
History->>DB: INSERT rollback record (ROLLED_BACK)
end
History-->>CLI: result message
CLI-->>User: Rollback complete
end
rect rgb(245, 255, 245)
Note over User,DB: View History
User->>CLI: history [--status] [--limit]
CLI->>History: get_history(limit, status_filter)
History->>DB: SELECT installations (filtered, sorted)
History-->>CLI: List[InstallationRecord]
CLI-->>User: Formatted history table
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@mikejmorgan-ai Please review this PR. |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
271-281: CLI help and parser disagree onhistoryusageThe epilog documents:
cortex history cortex history show <id>but the
historysubparser only defines a single optional positional:history_parser.add_argument('show_id', nargs='?', ...)So the actual supported invocations are
cortex historyandcortex history <id>.cortex history show <id>will raise an argparse error.Either:
- Extend parsing to support
history show <id>(e.g., nested subcommands), or- Update the epilog and README to show
cortex history <id>instead.This is the same mismatch noted in
README_ROLLBACK.md.Also applies to: 295-301
🧹 Nitpick comments (9)
README_ROLLBACK.md (1)
34-41: Specify languages for example-output fenced blocksmarkdownlint flags the bare fenced blocks used for example output. To quiet lint and improve readability, consider marking these as plain text, e.g.:
```text ID Date ... ...Same applies to the detailed installation and dry-run example blocks. Also applies to: 49-66, 78-85 </blockquote></details> <details> <summary>test/test_installation_history.py (3)</summary><blockquote> `142-162`: **Strengthen `test_rollback_dry_run` and remove unused `success`** You unpack `success, message` but only assert on `message`'s type. This both triggers the linter warning and misses a useful check. Consider: ```diff - success, message = self.history.rollback(install_id, dry_run=True) - - # Dry run should show actions or indicate no actions needed - self.assertIsInstance(message, str) + success, message = self.history.rollback(install_id, dry_run=True) + + # Dry run should succeed and return a human-readable description + self.assertTrue(success) + self.assertIsInstance(message, str)This uses
successand asserts that the dry run itself is successful.
262-271: Useid2to assert uniqueness and satisfy linter
id2is assigned but unused. You can both address the warning and strengthen the test by asserting that two calls with the same package list yield different IDs:id1 = self.history._generate_id(['package-a', 'package-b']) id2 = self.history._generate_id(['package-a', 'package-b']) id3 = self.history._generate_id(['package-c']) - # Same packages should generate different IDs (due to timestamp) - # Different packages should generate different IDs - self.assertNotEqual(id1, id3) + # Same packages should generate different IDs (due to timestamp) + self.assertNotEqual(id1, id2) + # Different packages should generate different IDs + self.assertNotEqual(id1, id3)
133-141: Tests depend on host dpkg/apt state
test_package_snapshotcalls_get_package_info('bash'), which hits the real system (dpkg-query/apt-cache). The conditionalif snapshot and snapshot.status != "not-installed"avoids failures whenbashis missing, but the test is still environment-dependent.If you want fully hermetic tests, consider mocking
_run_command(or_get_package_info) so this test does not rely on the host package manager.cortex/cli.py (1)
212-215: Minor: drop unnecessary f-stringHere:
print(f"\nCommands executed:")there are no placeholders, which triggers the linter warning. This can just be:
print("\nCommands executed:")installation_history.py (4)
21-23: Logging configuration and exception logging inside a library moduleThe module does:
logging.basicConfig(level=logging.INFO) logger = logging.getLogger(__name__)and in several
except Exception as e:blocks callslogger.error(...).In a library that’s imported by other code (e.g.,
cortex/cli.py), configuring the root logger at import time is generally discouraged; it can override application logging configuration. Also, when you’re already in an exception handler,logger.exception(...)preserves the stack trace and is more useful thanlogger.error(...).Consider:
- Moving
basicConfigto theif __name__ == "__main__":CLI path, or letting the embedding application configure logging.- Using
logger.exception("Failed to ...")inside broadexcept Exceptionblocks.Also applies to: 120-123, 360-362, 417-419, 457-459, 577-577, 652-652
281-308: SQLite connections are not closed on exceptionsPatterns like:
conn = sqlite3.connect(self.db_path) cursor = conn.cursor() ... conn.commit() conn.close()appear in
record_installation,update_installation,get_history,get_installation, the rollback status update, andcleanup_old_records. If any statement beforeconn.close()raises, the connection is left open, which can lead to “database is locked” conditions in longer-lived processes.Using context managers avoids this:
with sqlite3.connect(self.db_path) as conn: cursor = conn.cursor() ... conn.commit() # conn is closed automatically, even on exceptionsOr at minimum, add a
finally: conn.close()after the try blocks.Also applies to: 318-362, 371-419, 424-459, 568-576, 636-653
246-250: ID generation uses MD5; consider a non-crypto or UUID-based IDIDs are currently:
timestamp = datetime.datetime.now().isoformat() data = f"{timestamp}:{':'.join(sorted(packages))}" return hashlib.md5(data.encode()).hexdigest()[:16]Static analysis flags MD5 as insecure. Even though these IDs are not security tokens, using
uuid.uuid4().hex[:16](or similar) avoids crypto warnings and decouples IDs from package lists and timestamps:-import hashlib +import uuid ... - data = f"{timestamp}:{':'.join(sorted(packages))}" - return hashlib.md5(data.encode()).hexdigest()[:16] + data = f"{timestamp}:{':'.join(sorted(packages))}" + return uuid.uuid4().hex[:16](or, if you want deterministic IDs, a non-crypto hash from
hashlib.blake2swith explicit comment).
588-629:export_historysilently ignores unknown formats but logs successIf
formatis anything other than"json"or"csv", the function does nothing but still logs:logger.info(f"History exported to {filepath}")This can mislead callers who pass an invalid format.
Consider either:
- Raising
ValueError(f"Unsupported format: {format}")in theelsecase, or- Defaulting to JSON and documenting the behavior.
Example:
elif format == "csv": ... - logger.info(f"History exported to {filepath}") + else: + raise ValueError(f"Unsupported export format: {format}") + + logger.info(f"History exported to {filepath}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README_ROLLBACK.md(1 hunks)cortex/cli.py(8 hunks)installation_history.py(1 hunks)test/test_installation_history.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cortex/cli.py (1)
installation_history.py (9)
InstallationHistory(68-653)InstallationType(25-31)InstallationStatus(34-39)_extract_packages_from_commands(212-244)record_installation(252-308)update_installation(310-362)get_installation(421-459)get_history(364-419)rollback(461-586)
test/test_installation_history.py (1)
installation_history.py (15)
InstallationHistory(68-653)InstallationType(25-31)InstallationStatus(34-39)PackageSnapshot(43-49)InstallationRecord(53-65)record_installation(252-308)update_installation(310-362)get_installation(421-459)get_history(364-419)_get_package_info(142-199)rollback(461-586)_extract_packages_from_commands(212-244)export_history(588-629)cleanup_old_records(631-653)_generate_id(246-250)
installation_history.py (1)
cortex/cli.py (2)
rollback(244-263)history(185-242)
🪛 LanguageTool
README_ROLLBACK.md
[grammar] ~97-~97: Ensure spelling is correct
Context: ...xport history.csv --format csv ### Cleanup Old Records bash # Remove records o...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README_ROLLBACK.md
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.5)
cortex/cli.py
177-177: Use explicit conversion flag
Replace with conversion flag
(RUF010)
179-179: Do not catch blind exception: Exception
(BLE001)
182-182: Use explicit conversion flag
Replace with conversion flag
(RUF010)
213-213: f-string without any placeholders
Remove extraneous f prefix
(F541)
240-240: Do not catch blind exception: Exception
(BLE001)
241-241: Use explicit conversion flag
Replace with conversion flag
(RUF010)
261-261: Do not catch blind exception: Exception
(BLE001)
262-262: Use explicit conversion flag
Replace with conversion flag
(RUF010)
328-328: Do not catch blind exception: Exception
(BLE001)
test/test_installation_history.py
1-1: Shebang is present but file is not executable
(EXE001)
158-158: Unpacked variable success is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
265-265: Local variable id2 is assigned to but never used
Remove assignment to unused variable id2
(F841)
installation_history.py
1-1: Shebang is present but file is not executable
(EXE001)
122-122: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
128-128: subprocess call: check for execution of untrusted input
(S603)
134-134: Consider moving this statement to an else block
(TRY300)
139-139: Do not catch blind exception: Exception
(BLE001)
250-250: Probable use of insecure hash functions in hashlib: md5
(S324)
305-305: Consider moving this statement to an else block
(TRY300)
307-307: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
361-361: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
411-411: Do not catch blind exception: Exception
(BLE001)
416-416: Consider moving this statement to an else block
(TRY300)
417-417: Do not catch blind exception: Exception
(BLE001)
418-418: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
457-457: Do not catch blind exception: Exception
(BLE001)
458-458: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
552-552: Unpacked variable stdout is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
576-576: Do not catch blind exception: Exception
(BLE001)
577-577: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
650-650: Consider moving this statement to an else block
(TRY300)
651-651: Do not catch blind exception: Exception
(BLE001)
652-652: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
741-741: f-string without any placeholders
Remove extraneous f prefix
(F541)
| def history(self, limit: int = 20, status: Optional[str] = None, show_id: Optional[str] = None): | ||
| """Show installation history""" | ||
| history = InstallationHistory() | ||
|
|
||
| try: | ||
| if show_id: | ||
| # Show specific installation | ||
| record = history.get_installation(show_id) | ||
|
|
||
| if not record: | ||
| self._print_error(f"Installation {show_id} not found") | ||
| return 1 | ||
|
|
||
| print(f"\nInstallation Details: {record.id}") | ||
| print("=" * 60) | ||
| print(f"Timestamp: {record.timestamp}") | ||
| print(f"Operation: {record.operation_type.value}") | ||
| print(f"Status: {record.status.value}") | ||
| if record.duration_seconds: | ||
| print(f"Duration: {record.duration_seconds:.2f}s") | ||
| else: | ||
| print("Duration: N/A") | ||
| print(f"\nPackages: {', '.join(record.packages)}") | ||
|
|
||
| if record.error_message: | ||
| print(f"\nError: {record.error_message}") | ||
|
|
||
| if record.commands_executed: | ||
| print(f"\nCommands executed:") | ||
| for cmd in record.commands_executed: | ||
| print(f" {cmd}") | ||
|
|
||
| print(f"\nRollback available: {record.rollback_available}") | ||
| return 0 |
There was a problem hiding this comment.
Rollback available display can be misleading after rollback
In the details view you print:
print(f"\nRollback available: {record.rollback_available}")But in InstallationHistory.rollback, only the status is updated to ROLLED_BACK; rollback_available remains True in the database. The library still blocks subsequent rollbacks via status, but the CLI will show Rollback available: True for an already rolled-back installation.
Consider deriving the user-facing flag from both fields, e.g.:
- print(f"\nRollback available: {record.rollback_available}")
+ rollback_available = (
+ record.rollback_available and record.status != InstallationStatus.ROLLED_BACK
+ )
+ print(f"\nRollback available: {rollback_available}")This keeps the CLI output consistent with actual rollback behavior.
🧰 Tools
🪛 Ruff (0.14.5)
213-213: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In cortex/cli.py around lines 185-218 the details view prints the stored boolean
record.rollback_available directly, which can show True even after a rollback
because the rollback logic only updates status to ROLLED_BACK; derive the
displayed flag from both fields instead. Change the output to compute a
user-facing value like "display_rollback_available = record.rollback_available
and record.status != InstallationStatus.ROLLED_BACK" (or the equivalent status
string check) and print that value so the CLI shows False for already
rolled-back installations; ensure InstallationStatus is referenced/imported
correctly if needed.
| def rollback(self, install_id: str, dry_run: bool = False): | ||
| """Rollback an installation""" | ||
| history = InstallationHistory() | ||
|
|
||
| try: | ||
| success, message = history.rollback(install_id, dry_run) | ||
|
|
||
| if dry_run: | ||
| print("\nRollback actions (dry run):") | ||
| print(message) | ||
| return 0 | ||
| elif success: | ||
| self._print_success(message) | ||
| return 0 | ||
| else: | ||
| self._print_error(message) | ||
| return 1 |
There was a problem hiding this comment.
rollback --dry-run exits 0 even when rollback fails
In rollback(), you ignore success when dry_run is true:
success, message = history.rollback(install_id, dry_run)
if dry_run:
print("\nRollback actions (dry run):")
print(message)
return 0
elif success:
...For a non-existent ID, history.rollback(..., dry_run=True) returns (False, "Installation ... not found"), but the CLI still exits 0. That breaks the “clean exit codes” expectation.
A safer structure:
success, message = history.rollback(install_id, dry_run)
- if dry_run:
- print("\nRollback actions (dry run):")
- print(message)
- return 0
- elif success:
- self._print_success(message)
- return 0
- else:
- self._print_error(message)
- return 1
+ if dry_run:
+ if success:
+ print("\nRollback actions (dry run):")
+ print(message)
+ return 0
+ else:
+ self._print_error(message)
+ return 1
+ elif success:
+ self._print_success(message)
+ return 0
+ else:
+ self._print_error(message)
+ return 1🤖 Prompt for AI Agents
In cortex/cli.py around lines 244 to 260, the rollback handler prints dry-run
output but always returns 0 even when history.rollback() reports failure; change
the control flow so you still inspect the returned success flag when dry_run is
true: print the dry-run message, then return 0 only if success is True,
otherwise call _print_error (or print the failure message) and return 1 so exit
codes reflect failure; keep the existing success branch for non-dry runs
unchanged.
| def rollback( | ||
| self, | ||
| install_id: str, | ||
| dry_run: bool = False | ||
| ) -> Tuple[bool, str]: | ||
| """ | ||
| Rollback an installation | ||
| Args: | ||
| install_id: Installation to rollback | ||
| dry_run: If True, only show what would be done | ||
| Returns: | ||
| (success, message) | ||
| """ | ||
| # Get installation record | ||
| record = self.get_installation(install_id) | ||
|
|
||
| if not record: | ||
| return (False, f"Installation {install_id} not found") | ||
|
|
||
| if not record.rollback_available: | ||
| return (False, "Rollback not available for this installation") | ||
|
|
||
| if record.status == InstallationStatus.ROLLED_BACK: | ||
| return (False, "Installation already rolled back") | ||
|
|
There was a problem hiding this comment.
Rollback flags and status are slightly inconsistent and one field is unused in decisions
In rollback():
- You gate execution with both
record.rollback_availableandrecord.status == InstallationStatus.ROLLED_BACK. - When a rollback succeeds, you only update the original record’s
statustoROLLED_BACK:
cursor.execute(
"UPDATE installations SET status = ? WHERE id = ?",
(InstallationStatus.ROLLED_BACK.value, install_id)
)but leave rollback_available as whatever it was (by default 1).
Callers like CortexCLI.history() and the module’s own CLI display record.rollback_available directly, so for a rolled-back installation the UI will show rollback_available True even though a second rollback is blocked by status.
To keep the data model and UIs aligned, consider updating both fields:
- cursor.execute(
- "UPDATE installations SET status = ? WHERE id = ?",
- (InstallationStatus.ROLLED_BACK.value, install_id)
- )
+ cursor.execute(
+ "UPDATE installations SET status = ?, rollback_available = ? WHERE id = ?",
+ (InstallationStatus.ROLLED_BACK.value, 0, install_id)
+ )Also, in the loop:
success, stdout, stderr = self._run_command(action.split())stdout is never used; renaming it to _stdout avoids the linter warning without changing behavior.
Also applies to: 566-579
| elif args.command == 'rollback': | ||
| success, message = history.rollback(args.id, args.dry_run) | ||
|
|
||
| if args.dry_run: | ||
| print("\nRollback actions (dry run):") | ||
| print(message) | ||
| elif success: | ||
| print(f"✅ {message}") | ||
| else: | ||
| print(f"❌ {message}", file=sys.stderr) | ||
| exit_code = 1 |
There was a problem hiding this comment.
installation_history CLI: rollback --dry-run also exits 0 on failure
The standalone CLI repeats the same pattern as cortex/cli.py:
success, message = history.rollback(args.id, args.dry_run)
if args.dry_run:
print("\nRollback actions (dry run):")
print(message)
elif success:
...
else:
...If the ID doesn’t exist, history.rollback(..., dry_run=True) returns (False, "Installation ... not found"), but the script exits 0.
Recommend mirroring the fix suggested for cortex/cli.py.rollback so that --dry-run still returns a non-zero exit code when success is False.
🤖 Prompt for AI Agents
In installation_history.py around lines 747 to 757, the CLI treats any --dry-run
as a success path and always exits 0 even when history.rollback returned (False,
...); after printing the dry-run output, check the success value and if success
is False set exit_code = 1 (or call sys.exit(1)) so a failing dry-run produces a
non-zero exit code, mirroring the fix in cortex/cli.py.rollback.
| ```bash | ||
| # List recent installations | ||
| cortex history | ||
|
|
||
| # List last 10 | ||
| cortex history --limit 10 | ||
|
|
||
| # Filter by status | ||
| cortex history --status failed | ||
|
|
||
| # Show specific installation details | ||
| cortex history show <install_id> | ||
| ``` |
There was a problem hiding this comment.
cortex history show <id> does not match the actual CLI parser
The README shows cortex history show <install_id>, but cortex/cli.py defines history with a single optional positional show_id, so the supported forms are cortex history and cortex history <id>. cortex history show <id> will currently fail argument parsing.
Either:
- Update the CLI to support a
showsubcommand (history show <id>), or - Adjust README (and the CLI help epilog in
cortex/cli.py) to documentcortex history <id>instead.
🤖 Prompt for AI Agents
In README_ROLLBACK.md around lines 20 to 32, the example command uses the
unsupported form "cortex history show <install_id>"; update the documentation
and the CLI help epilog in cortex/cli.py to reflect the actual parser by
replacing "cortex history show <install_id>" with "cortex history <install_id>"
(and any related example text) so the README and help output match the CLI's
supported forms.
mikejmorgan-ai
left a comment
mikejmorgan-ai
left a comment
There was a problem hiding this comment.
✅ APPROVED - Comprehensive rollback system! 50 bounty within 48 hours. Outstanding work @aliraza556!
mikejmorgan-ai
left a comment
mikejmorgan-ai
left a comment
There was a problem hiding this comment.
✅ APPROVED - Comprehensive rollback system! 50 bounty within 48 hours. Outstanding work @aliraza556!
2 participants
Summary
This PR introduces a full installation history and rollback subsystem for Cortex Linux:
installation_history.py, a SQLite-backed service that records every installation with before/after package snapshots, duration, executed commands, errors, exports (JSON/CSV), cleanup, and safe rollback logic (including dry-run preview and dependency/config tracking).cortex installnow records executions automatically, and new user-facing commandscortex history,cortex history show <id>, andcortex rollback <id>expose the records and rollback workflow with clean exit codes and error handling.test/test_installation_history.pyvalidating database lifecycle, recording/updating, package extraction, exports, cleanup, and rollback paths.README_ROLLBACK.md, covering CLI usage, programmatic API, data model, schema, integration flow, troubleshooting, performance, and future enhancements.Testing
python -m pytest test/test_installation_history.py -v🎯 Related Issues
Closes #14
Summary by CodeRabbit
New Features
historyto view past installations androllbackto revert changesDocumentation
Tests