Skip to content

Harden ListBooks() against external mutation by returning a defensive copy#1

Merged
askrinnik merged 2 commits into
mainfrom
copilot/implement-second-todo-item
May 14, 2026
Merged

Harden ListBooks() against external mutation by returning a defensive copy#1
askrinnik merged 2 commits into
mainfrom
copilot/implement-second-todo-item

Conversation

Copy link
Copy Markdown

Copilot AI commented May 14, 2026

This addresses todo github#2 from the C# BookApp review: BookCollection.ListBooks() exposed internal mutable state by returning the backing _books list directly. The change isolates internal collection state while keeping the public API shape intact.

  • Service behavior change

    • Updated BookCollection.ListBooks() to return a defensive copy rather than the internal list.
    • Prevents callers from mutating BookCollection state via operations on the returned list.
  • Coverage for regression risk

    • Added a focused test proving external mutation of the returned list does not affect BookCollection.Books.
// before
public List<Book> ListBooks() => _books;

// after
public List<Book> ListBooks() => _books.ToList();
Original prompt

Would you implement the second to do item?

Chronological Review: 1. User asked about BookApp.csproj → explained .NET project config 2. User asked for code quality review → 8-10 issues identified across bugs/design/test gaps 3. User asked to create numbered checklist → saved to markdown, inserted into SQL todos 4. User asked to implement todo #1 (HandleRemove bool) → fixed Program.cs, committed (95d9cdb) 5. User asked about test for fix → confirmed existing tests covered the service layer 6. User reported "The Hobbit" search bug → debugged: find command only searches by author, FindByAuthor uses exact match 7. User asked to implement Test Coverage Gap todos (github#8-10) → added 12 tests, all 17 pass, committed (a292527) 8. User asked to update todo checklist → marked 4 items done in markdown 9. User asked how to run tests → provided dotnet test command 10. User asked to push to master → failed (wrong branch name) 11. Discovered branch is "main" → tried push origin main → failed (403, no write access to upstream) 12. User fixed remote URL to their fork → push succeeded to askrinnik/copilot-cli-for-beginners

Intent Mapping:

  • All prior work: review, fix, test, commit
  • Final request: "Could you push changes to the master branch?" → then "I fixed the remote repo URL to my fork repository. Could you try to push again?"

Technical Inventory:

  • .NET 10, xUnit, C# BookApp sample project
  • git push operations, fork workflow
  • Two commits: 95d9cdb and a292527

Recent Commands:

  1. git push origin master → error: src refspec master does not match any
  2. git branch; git status --short → confirmed branch is "main"
  3. git push origin main → 403 permission denied (github org repo)
  4. User fixed remote to fork URL
  5. git push origin main → SUCCESS, pushed to askrinnik/copilot-cli-for-beginners, eb7e358..a292527

Progress: 4/10 todos done, 6 pending. Two commits pushed to fork.

1. Conversation Overview: - Primary Objectives: Code quality review of `samples/book-app-project-cs/` C# BookApp → create numbered issue checklist → implement fixes incrementally → commit and push to remote - Session Context: Educational repo (`github/copilot-cli-for-beginners`). Full pipeline completed: review → SQL todo tracking → markdown checklist → fix implementation → tests → commits → push to fork - User Intent Evolution: Started with passive review, progressed to active bug fixing and test writing, ended with git push workflow including fork resolution
  1. Technical Foundation:

    • .NET 10 / C#: Target framework net10.0, nullable enabled, console app
    • xUnit: Test framework in Tests/BookApp.Tests.csproj; 17 tests all passing
    • Git: Two commits on main branch; upstream is github/copilot-cli-for-beginners (no write access); fork at askrinnik/copilot-cli-for-beginners (write access confirmed)
    • SQL todos table: 10 items total; 4 done, 6 pending; used to track issue progress
    • Conventional Commits: Pattern fix(book-app-cs): / test(book-app-cs):
  2. Codebase Status:

    • samples/book-app-project-cs/Program.cs:
      • Purpose: CLI entry point; handles list, add, remove, find, help commands
      • Current State: Fixed HandleRemove (commit 95d9cdb) to store bool return from RemoveBook() and display correct success/not-found messages
      • Known Issue: HandleFind (lines 70–77) only searches by author, not title — "The Hobbit" search returns empty because it's a title
    • samples/book-app-project-cs/Services/BookCollection.cs:
    • samples/book-app-project-cs/Tests/BookCollectionTests.cs:
      • Purpose: xUnit tests for BookCollection service
      • Current State: 12 new tests added (commit a292527); 17 total, all passing
      • Coverage Added: FindByAuthor (exact, case-insensitive, multi-book, unknown), persistence round-trip via new BookCollection instance, edge cases (empty title, empty author, duplicate titles)
    • samples/book-app-project-cs/Models/Book.cs:
    • samples/book-app-project-cs/data.json:
    • bookapp-cs-issues.md (session artifact in ~/.copilot/session-state/.../files/):
      • Purpose: Numbered 10-item checklist with severity tiers
      • Current State: Items 1, 8, 9, 10 marked [x] done; 6 remaining open
  3. Problem Resolution:

    • Issues Encountered:
      • `HandleRe...

Created from Copilot CLI via the copilot delegate command.

Copilot AI changed the title [WIP] Implement second to do item for BookApp Harden ListBooks() against external mutation by returning a defensive copy May 14, 2026
Copilot AI requested a review from askrinnik May 14, 2026 10:19
@askrinnik askrinnik merged commit 01621c3 into main May 14, 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.

2 participants