From 2ce83e41499a23552d97b1774298b6ec5b60590f Mon Sep 17 00:00:00 2001 From: Dylan Jubb Date: Mon, 7 Sep 2026 13:57:01 +0100 Subject: [PATCH] feat(book-app): add unread book listing and course updates Add unread book filtering and CLI support with tests and documentation. Include the other current agent, skill, error-handling, utility, and test updates in the same change set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/agents/data-validator.agent.md | 18 ++ .github/agents/doc-writer.md | 19 +++ .github/agents/error-handler.agent.md | 18 ++ .github/skills/book-summary/SKILL.md | 17 ++ .github/skills/security-audit/SKILL.md | 38 +++++ ERROR-HANDLING.md | 159 ++++++++++++++++++ samples/book-app-project/README.md | 5 +- samples/book-app-project/book_app.py | 31 ++-- samples/book-app-project/books.py | 26 ++- .../book-app-project/tests/test_book_app.py | 79 +++++++++ samples/book-app-project/tests/test_books.py | 83 +++++++++ samples/book-app-project/tests/test_utils.py | 54 ++++++ samples/book-app-project/utils.py | 32 +++- samples/skills/security-audit/SKILL.md | 38 +++++ 14 files changed, 593 insertions(+), 24 deletions(-) create mode 100644 .github/agents/data-validator.agent.md create mode 100644 .github/agents/doc-writer.md create mode 100644 .github/agents/error-handler.agent.md create mode 100644 .github/skills/book-summary/SKILL.md create mode 100644 .github/skills/security-audit/SKILL.md create mode 100644 ERROR-HANDLING.md create mode 100644 samples/book-app-project/tests/test_book_app.py create mode 100644 samples/book-app-project/tests/test_utils.py create mode 100644 samples/skills/security-audit/SKILL.md diff --git a/.github/agents/data-validator.agent.md b/.github/agents/data-validator.agent.md new file mode 100644 index 00000000..547b0ef3 --- /dev/null +++ b/.github/agents/data-validator.agent.md @@ -0,0 +1,18 @@ +--- +name: data-validator +description: A data validation specialist for json data. +--- + +# Data Validator + +You are a data expert focused on validating book Json data. + +## Validation Priorities +1. Checking for missing data. +2. Checking for unrealistic book titles. +3. Checking for unrealistic book authors. +4. Checking for unrealistic book names. +5. Check for other unrealistic values. + +## Output Format +Categorises issues as either: [MISSING DATA], [MALFORMED DATA], [UNREALISTIC DATA] \ No newline at end of file diff --git a/.github/agents/doc-writer.md b/.github/agents/doc-writer.md new file mode 100644 index 00000000..25d9f0a0 --- /dev/null +++ b/.github/agents/doc-writer.md @@ -0,0 +1,19 @@ +--- +name: doc-writer +description: A technical writer for clear documentation and commentary +--- + +# Doc Writer + +You are a technical writer focused on creating/updating Markdown documentation and Python docstring commentary. + +## Documentation Standards +1. Begin with a one sentence summary. +2. Clearly separate different topics. +3. Include usage examples. +4. Note any gotchas. + +## Commentary Standards +1. Begin with a one sentence summary +2. Include any input and output variables with a description. +3. Describe the type of any variables. diff --git a/.github/agents/error-handler.agent.md b/.github/agents/error-handler.agent.md new file mode 100644 index 00000000..16f81f42 --- /dev/null +++ b/.github/agents/error-handler.agent.md @@ -0,0 +1,18 @@ +--- +name: error-handler +description: A python reviewer to identify inconsistent error handling. +--- + +# Error Handler + +You are a python error handling expertfocused on identifying error handling inconsistencies. + +## Review Priorities +1. Checking for inconsistent error handling. +2. Check for bad error handling. +3. Check for missing error handling. +4. Check for error handling formatitng + +## Output Format +Suggests a unified approach for inconsistencies. +Returns other issues categorised as either: [BAD HANDLING], [MISSING], [FORMATTING] \ No newline at end of file diff --git a/.github/skills/book-summary/SKILL.md b/.github/skills/book-summary/SKILL.md new file mode 100644 index 00000000..c5a9d96f --- /dev/null +++ b/.github/skills/book-summary/SKILL.md @@ -0,0 +1,17 @@ +--- +name: book-summary +description: Generate a summary of a collection of books +--- + +# Collection Summary + +Generate a summary of a book collection in a markdown table. + +## Format + +A markdown table containing book title, book author, book year, book read status for each book in the collection. + +## Conventions + +- Use Y/N to indicate read status +- Return book author in the format: last name, first name \ No newline at end of file diff --git a/.github/skills/security-audit/SKILL.md b/.github/skills/security-audit/SKILL.md new file mode 100644 index 00000000..10c1d308 --- /dev/null +++ b/.github/skills/security-audit/SKILL.md @@ -0,0 +1,38 @@ +--- +name: security-audit +description: Security-focused code review checking OWASP (Open Web Application Security Project) Top 10 vulnerabilities. +--- + +# Security Audit + +Perform a security audit checking for: + +## Injection Vulnerabilities +- SQL injection (string concatenation in queries) +- Command injection (unsanitized shell commands) +- LDAP injection +- XPath injection + +## Authentication Issues +- Hardcoded credentials +- Weak password requirements +- Missing rate limiting +- Session management flaws + +## Sensitive Data +- Plaintext passwords +- API keys in code +- Logging sensitive information +- Missing encryption + +## Access Control +- Missing authorization checks +- Insecure direct object references +- Path traversal vulnerabilities + +## Output +For each issue found, provide: +1. File and line number +2. Vulnerability type +3. Severity (CRITICAL/HIGH/MEDIUM/LOW) +4. Recommended fix diff --git a/ERROR-HANDLING.md b/ERROR-HANDLING.md new file mode 100644 index 00000000..3461776b --- /dev/null +++ b/ERROR-HANDLING.md @@ -0,0 +1,159 @@ +# Error Handling in the Book App + +Good error handling helps the book app reject invalid input, protect saved data, and show useful messages without mixing user-interface code with book-management logic. + +## Responsibilities + +Keep responsibilities separate: + +- `books.py` validates and manages book data. +- `utils.py` validates interactive input. +- `book_app.py` displays user-facing messages. + +This makes the collection class easier to test and reuse from another interface. + +## Validate Input Consistently + +Validate values at public method boundaries, even when a command-line helper already validates them. Callers can also be tests or other Python modules. + +Important checks include: + +- Titles and authors must be non-empty strings after trimming whitespace. +- Publication years must be integers in the accepted range. +- Boolean values must not be accepted as publication years, because `bool` is a subclass of `int` in Python. +- Search and removal titles should be normalized consistently, for example with `strip()` and `casefold()`. + +Do not silently convert invalid input to a placeholder such as `0`. Raise a clear `ValueError` instead: + +```python +if not isinstance(year, int) or isinstance(year, bool): + raise ValueError("Publication year must be an integer.") +``` + +## Use Clear Error Contracts + +Use exceptions when an operation cannot complete normally: + +```python +try: + collection.add_book(title, author, year) +except ValueError as error: + print(f"Error: {error}") +``` + +Use return values for expected results, such as whether a matching book exists: + +```python +if collection.remove_book(title): + print("Book removed successfully.") +else: + print("No matching book was found.") +``` + +Keep this contract consistent. A recommended approach is: + +- `find_book_by_title`: return `Book | None`. +- `add_book`: return the new `Book` or raise `ValueError`. +- `remove_book`: return `True` when removed and `False` when not found. +- Persistence and data-integrity failures: raise a clear exception. + +## Keep User-Facing Messages in the CLI + +The data layer should report errors through exceptions or return values, not print directly. Otherwise, a library caller cannot choose how to display or log the problem. + +```python +def handle_remove(): + title = input("Enter the title of the book to remove: ").strip() + + try: + removed = collection.remove_book(title) + except ValueError as error: + print(f"\nError: {error}\n") + return + + if removed: + print("\nBook removed successfully.\n") + else: + print("\nNo matching book was found.\n") +``` + +Only one layer should own the final user-facing result. Avoid printing `Book not found` from `books.py` while `book_app.py` also prints a generic removal message. + +## Protect Corrupted Data + +Malformed JSON should not silently become an empty collection. If the user later saves a new book, the original corrupted file could be overwritten and recoverable data lost. + +A safer flow is: + +1. Detect the parsing error. +2. Preserve the original file. +3. Report which configured file is invalid. +4. Let the application decide whether recovery is appropriate. + +Include the configured path in diagnostics rather than hard-coding `data.json`: + +```python +raise ValueError( + f"{DATA_FILE} contains invalid JSON and was not changed." +) from error +``` + +## Validate Loaded Records + +Valid JSON is not necessarily valid book data. Before creating `Book` instances, check that: + +- The top-level value is a list. +- Each item is an object. +- Required fields are present. +- `title` and `author` are non-empty strings. +- `year` is an integer. +- `read` is a Boolean when present. + +Build a temporary list and replace `self.books` only after every record validates. This prevents partially loaded collections. + +## Handle Save Failures Safely + +Methods such as `add_book()` and `mark_as_read()` should not leave memory changed when saving fails. Build and save the proposed state first, then update `self.books` only after persistence succeeds. + +Wrap expected file-system failures with useful context while preserving the original cause: + +```python +try: + with open(DATA_FILE, "w", encoding="utf-8") as file: + json.dump(data, file, indent=2) +except OSError as error: + raise OSError(f"Could not save books to {DATA_FILE}.") from error +``` + +Opening a file with `"w"` truncates it before writing. For stronger protection, write to a temporary file and replace the original only after the complete write succeeds. + +## Consistent Diagnostics + +Error messages should explain what failed and whether data changed: + +```text +Error: Book title cannot be empty. +Error: Publication year must be an integer. +Error: data.json contains invalid JSON and was not changed. +Error: Could not save books to data.json. +No matching book was found. +``` + +Use consistent wording, punctuation, and routing. When wrapping an exception, use `raise ... from error` so debugging can still inspect the original cause. + +## Tests to Add + +Cover both successful operations and failure paths: + +- Empty or whitespace-only titles and authors. +- Non-string titles or authors. +- Invalid, negative, or unrealistic years. +- Case-insensitive and whitespace-tolerant title searches. +- Missing books. +- Corrupted JSON. +- JSON with the wrong top-level type. +- Records with missing fields or incorrect types. +- Permission or other save failures. +- Confirmation that failed saves do not change the in-memory collection. + +The main gotcha is that a valid JSON document can still contain invalid application data. Validate syntax and meaning separately. diff --git a/samples/book-app-project/README.md b/samples/book-app-project/README.md index d3dd580a..ca4b4c5f 100644 --- a/samples/book-app-project/README.md +++ b/samples/book-app-project/README.md @@ -3,13 +3,15 @@ *(This README is intentionally rough so you can improve it with GitHub Copilot CLI)* A Python app for managing books you have or want to read. -It can add, remove, and list books. Also mark them as read. +It can add, remove, list books, and show unread books. Books can also be +marked as read. --- ## Current Features * Reads books from a JSON file (our database) +* Lists unread books with the `unread` command * Input checking is weak in some areas * Some tests exist but probably not enough @@ -29,6 +31,7 @@ It can add, remove, and list books. Also mark them as read. ```bash python book_app.py list +python book_app.py unread python book_app.py add python book_app.py find python book_app.py remove diff --git a/samples/book-app-project/book_app.py b/samples/book-app-project/book_app.py index f0100c2d..53e09c85 100644 --- a/samples/book-app-project/book_app.py +++ b/samples/book-app-project/book_app.py @@ -1,29 +1,23 @@ import sys from books import BookCollection +from utils import print_books # Global collection instance collection = BookCollection() -def show_books(books): - """Display books in a user-friendly format.""" - if not books: - print("No books found.") - return - - print("\nYour Book Collection:\n") - - for index, book in enumerate(books, start=1): - status = "āœ“" if book.read else " " - print(f"{index}. [{status}] {book.title} by {book.author} ({book.year})") - - print() - - def handle_list(): books = collection.list_books() - show_books(books) + print_books(books) + + +def handle_unread() -> None: + books = collection.list_unread_books() + if books: + print_books(books) + else: + print("No unread books in your collection.") def handle_add(): @@ -56,7 +50,7 @@ def handle_find(): author = input("Author name: ").strip() books = collection.find_by_author(author) - show_books(books) + print_books(books) def show_help(): @@ -65,6 +59,7 @@ def show_help(): Commands: list - Show all books + unread - Show unread books add - Add a new book remove - Remove a book by title find - Find books by author @@ -81,6 +76,8 @@ def main(): if command == "list": handle_list() + elif command == "unread": + handle_unread() elif command == "add": handle_add() elif command == "remove": diff --git a/samples/book-app-project/books.py b/samples/book-app-project/books.py index 2110689f..12f03eab 100644 --- a/samples/book-app-project/books.py +++ b/samples/book-app-project/books.py @@ -36,6 +36,9 @@ def save_books(self): json.dump([asdict(b) for b in self.books], f, indent=2) def add_book(self, title: str, author: str, year: int) -> Book: + if not title.strip(): + raise ValueError("Book title cannot be empty.") + book = Book(title=title, author=author, year=year) self.books.append(book) self.save_books() @@ -44,9 +47,17 @@ def add_book(self, title: str, author: str, year: int) -> Book: def list_books(self) -> List[Book]: return self.books + def list_unread_books(self) -> List[Book]: + """Return unread books in their existing collection order.""" + return [book for book in self.books if not book.read] + def find_book_by_title(self, title: str) -> Optional[Book]: + if not isinstance(title, str): + return None + + normalized_title = title.strip().casefold() for book in self.books: - if book.title.lower() == title.lower(): + if book.title.strip().casefold() == normalized_title: return book return None @@ -60,11 +71,22 @@ def mark_as_read(self, title: str) -> bool: def remove_book(self, title: str) -> bool: """Remove a book by title.""" - book = self.find_book_by_title(title) + if not isinstance(title, str): + print("Book title must be a string.") + return False + + normalized_title = title.strip() + if not normalized_title: + print("Book title cannot be empty.") + return False + + book = self.find_book_by_title(normalized_title) if book: self.books.remove(book) self.save_books() return True + + print(f'Book not found: "{normalized_title}"') return False def find_by_author(self, author: str) -> List[Book]: diff --git a/samples/book-app-project/tests/test_book_app.py b/samples/book-app-project/tests/test_book_app.py new file mode 100644 index 00000000..70da9010 --- /dev/null +++ b/samples/book-app-project/tests/test_book_app.py @@ -0,0 +1,79 @@ +import sys +from pathlib import Path +from unittest.mock import Mock + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +import book_app +from books import Book + + +def test_handle_list_uses_shared_book_display(monkeypatch): + books = [Book("1984", "George Orwell", 1949, read=True)] + collection = Mock() + collection.list_books.return_value = books + display = Mock() + monkeypatch.setattr(book_app, "collection", collection) + monkeypatch.setattr(book_app, "print_books", display) + + book_app.handle_list() + + display.assert_called_once_with(books) + + +def test_handle_find_uses_shared_book_display(monkeypatch): + books = [Book("Dune", "Frank Herbert", 1965)] + collection = Mock() + collection.find_by_author.return_value = books + display = Mock() + monkeypatch.setattr(book_app, "collection", collection) + monkeypatch.setattr(book_app, "print_books", display) + monkeypatch.setattr("builtins.input", lambda _: "Frank Herbert") + + book_app.handle_find() + + collection.find_by_author.assert_called_once_with("Frank Herbert") + display.assert_called_once_with(books) + + +def test_handle_unread_uses_shared_book_display(monkeypatch): + books = [Book("Dune", "Frank Herbert", 1965)] + collection = Mock() + collection.list_unread_books.return_value = books + display = Mock() + monkeypatch.setattr(book_app, "collection", collection) + monkeypatch.setattr(book_app, "print_books", display) + + book_app.handle_unread() + + collection.list_unread_books.assert_called_once_with() + display.assert_called_once_with(books) + + +def test_handle_unread_reports_when_no_books_are_unread(monkeypatch, capsys): + collection = Mock() + collection.list_unread_books.return_value = [] + display = Mock() + monkeypatch.setattr(book_app, "collection", collection) + monkeypatch.setattr(book_app, "print_books", display) + + book_app.handle_unread() + + assert capsys.readouterr().out.strip() == "No unread books in your collection." + display.assert_not_called() + + +def test_main_routes_unread_command(monkeypatch): + handler = Mock() + monkeypatch.setattr(book_app, "handle_unread", handler) + monkeypatch.setattr(book_app.sys, "argv", ["book_app.py", "unread"]) + + book_app.main() + + handler.assert_called_once_with() + + +def test_show_help_includes_unread_command(capsys): + book_app.show_help() + + assert "unread - Show unread books" in capsys.readouterr().out diff --git a/samples/book-app-project/tests/test_books.py b/samples/book-app-project/tests/test_books.py index 061149c5..cbf44263 100644 --- a/samples/book-app-project/tests/test_books.py +++ b/samples/book-app-project/tests/test_books.py @@ -26,6 +26,14 @@ def test_add_book(): assert book.year == 1949 assert book.read is False +def test_add_book_rejects_empty_title(): + collection = BookCollection() + + with pytest.raises(ValueError, match="Book title cannot be empty."): + collection.add_book(" ", "George Orwell", 1949) + + assert collection.books == [] + def test_mark_book_as_read(): collection = BookCollection() collection.add_book("Dune", "Frank Herbert", 1965) @@ -39,6 +47,46 @@ def test_mark_book_as_read_invalid(): result = collection.mark_as_read("Nonexistent Book") assert result is False + +def test_list_unread_books_returns_unread_books_in_collection_order(): + collection = BookCollection() + first = collection.add_book("The Hobbit", "J.R.R. Tolkien", 1937) + collection.add_book("1984", "George Orwell", 1949) + third = collection.add_book("Dune", "Frank Herbert", 1965) + collection.mark_as_read("1984") + + unread_books = collection.list_unread_books() + + assert unread_books == [first, third] + assert unread_books is not collection.books + + +def test_list_unread_books_does_not_change_collection_state_or_data(): + collection = BookCollection() + collection.add_book("Dune", "Frank Herbert", 1965) + collection.mark_as_read("Dune") + books_before = list(collection.books) + data_before = collection.books[0].read + + assert collection.list_unread_books() == [] + assert collection.books == books_before + assert collection.books[0].read is data_before + + +def test_list_unread_books_returns_empty_list_when_all_books_are_read(): + collection = BookCollection() + collection.add_book("1984", "George Orwell", 1949) + collection.mark_as_read("1984") + + assert collection.list_unread_books() == [] + + +def test_list_unread_books_returns_empty_list_for_empty_collection(): + collection = BookCollection() + + assert collection.list_unread_books() == [] + + def test_remove_book(): collection = BookCollection() collection.add_book("The Hobbit", "J.R.R. Tolkien", 1937) @@ -51,3 +99,38 @@ def test_remove_book_invalid(): collection = BookCollection() result = collection.remove_book("Nonexistent Book") assert result is False + + +def test_remove_book_matches_case_insensitively_and_ignores_whitespace(): + collection = BookCollection() + collection.add_book("Dune", "Frank Herbert", 1965) + + result = collection.remove_book(" dUnE ") + + assert result is True + assert collection.books == [] + + +def test_remove_book_reports_missing_title(capsys): + collection = BookCollection() + + result = collection.remove_book("Nonexistent Book") + + assert result is False + assert 'Book not found: "Nonexistent Book"' in capsys.readouterr().out + + +@pytest.mark.parametrize( + ("title", "message"), + [ + (" ", "Book title cannot be empty."), + (None, "Book title must be a string."), + ], +) +def test_remove_book_rejects_invalid_title(title, message, capsys): + collection = BookCollection() + + result = collection.remove_book(title) + + assert result is False + assert message in capsys.readouterr().out diff --git a/samples/book-app-project/tests/test_utils.py b/samples/book-app-project/tests/test_utils.py new file mode 100644 index 00000000..cd0a2cfa --- /dev/null +++ b/samples/book-app-project/tests/test_utils.py @@ -0,0 +1,54 @@ +import pytest + +from books import Book +from utils import get_user_choice, print_books + + +def test_get_book_details_retries_empty_title(monkeypatch, capsys): + responses = iter(["", "The Hobbit", "J.R.R. Tolkien", "1937"]) + monkeypatch.setattr("builtins.input", lambda _: next(responses)) + + from utils import get_book_details + + assert get_book_details() == ("The Hobbit", "J.R.R. Tolkien", 1937) + assert "Book title cannot be empty." in capsys.readouterr().out + + +def test_get_user_choice_returns_valid_choice(monkeypatch): + monkeypatch.setattr("builtins.input", lambda _: "3") + + assert get_user_choice() == "3" + + +@pytest.mark.parametrize( + "invalid_choice, expected_message", + [ + ("", "Please enter a choice from 1 to 5."), + ("abc", "Please enter a numeric choice from 1 to 5."), + ], +) +def test_get_user_choice_retries_invalid_input( + monkeypatch, capsys, invalid_choice, expected_message +): + choices = iter([invalid_choice, "2"]) + monkeypatch.setattr("builtins.input", lambda _: next(choices)) + + assert get_user_choice() == "2" + assert expected_message in capsys.readouterr().out + + +def test_print_books_displays_read_status(capsys): + books = [Book("1984", "George Orwell", 1949, read=True)] + + print_books(books) + + output = capsys.readouterr().out + assert "1. 1984 by George Orwell (1949) - Read" in output + assert "āœ…" not in output + assert "šŸ“–" not in output + + +def test_print_books_handles_empty_collection(capsys): + print_books([]) + + assert capsys.readouterr().out == "No books in your collection.\n" diff --git a/samples/book-app-project/utils.py b/samples/book-app-project/utils.py index 4151dcda..f577e2f3 100644 --- a/samples/book-app-project/utils.py +++ b/samples/book-app-project/utils.py @@ -1,3 +1,6 @@ +from books import Book + + def print_menu(): print("\nšŸ“š Book Collection App") print("1. Add a book") @@ -8,11 +11,32 @@ def print_menu(): def get_user_choice() -> str: - return input("Choose an option (1-5): ").strip() + """Read and validate a menu choice from the user.""" + while True: + choice = input("Choose an option (1-5): ").strip() + + if not choice: + print("Please enter a choice from 1 to 5.") + continue + + if not choice.isdigit(): + print("Please enter a numeric choice from 1 to 5.") + continue + + if not 1 <= int(choice) <= 5: + print("Please enter a choice from 1 to 5.") + continue + + return choice def get_book_details(): - title = input("Enter book title: ").strip() + while True: + title = input("Enter book title: ").strip() + if title: + break + print("Book title cannot be empty.") + author = input("Enter author: ").strip() year_input = input("Enter publication year: ").strip() @@ -25,12 +49,12 @@ def get_book_details(): return title, author, year -def print_books(books): +def print_books(books: list[Book]) -> None: if not books: print("No books in your collection.") return print("\nYour Books:") for index, book in enumerate(books, start=1): - status = "āœ… Read" if book.read else "šŸ“– Unread" + status = "Read" if book.read else "Unread" print(f"{index}. {book.title} by {book.author} ({book.year}) - {status}") diff --git a/samples/skills/security-audit/SKILL.md b/samples/skills/security-audit/SKILL.md new file mode 100644 index 00000000..10c1d308 --- /dev/null +++ b/samples/skills/security-audit/SKILL.md @@ -0,0 +1,38 @@ +--- +name: security-audit +description: Security-focused code review checking OWASP (Open Web Application Security Project) Top 10 vulnerabilities. +--- + +# Security Audit + +Perform a security audit checking for: + +## Injection Vulnerabilities +- SQL injection (string concatenation in queries) +- Command injection (unsanitized shell commands) +- LDAP injection +- XPath injection + +## Authentication Issues +- Hardcoded credentials +- Weak password requirements +- Missing rate limiting +- Session management flaws + +## Sensitive Data +- Plaintext passwords +- API keys in code +- Logging sensitive information +- Missing encryption + +## Access Control +- Missing authorization checks +- Insecure direct object references +- Path traversal vulnerabilities + +## Output +For each issue found, provide: +1. File and line number +2. Vulnerability type +3. Severity (CRITICAL/HIGH/MEDIUM/LOW) +4. Recommended fix