From 3b5363234599a72d4dd04847c1124d6381ca21c7 Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Fri, 23 Jan 2026 12:36:13 -0600 Subject: Add code review workflow and project cleanup tasks - Create docs/project-workflows/code-review.org with comprehensive code review checklist based on Code Review Pyramid framework - Add 14 new tasks to todo.org from senior developer code review: - Priority A: README GRUB refs, missing LICENSE, skeleton script - Priority B: Stale files, Makefile lint, documentation gaps - Priority C: Style consistency, .editorconfig, test docs - Add Makefile targets to todo.org: deps, lint, deploy - Create docs/session-context.org for session tracking --- docs/project-workflows/code-review.org | 275 +++++++++++++++++++++++++++++++++ docs/session-context.org | 51 ++++++ todo.org | 123 +++++++++++++++ 3 files changed, 449 insertions(+) create mode 100644 docs/project-workflows/code-review.org create mode 100644 docs/session-context.org diff --git a/docs/project-workflows/code-review.org b/docs/project-workflows/code-review.org new file mode 100644 index 0000000..79ef0ed --- /dev/null +++ b/docs/project-workflows/code-review.org @@ -0,0 +1,275 @@ +#+TITLE: Code Review Workflow +#+AUTHOR: Craig Jennings & Claude +#+DATE: 2026-01-23 + +* Overview + +This workflow guides code review with a focus on what matters most. It's based on the Code Review Pyramid (Gunnar Morling) which prioritizes review effort by cost of change - spend the most time on things that are hardest to fix later. + +#+begin_example + ┌─────────────────┐ + │ CODE STYLE │ ← Automate + ┌───┴─────────────────┴───┐ + │ TESTS │ ← Automate + ┌───┴─────────────────────────┴───┐ + │ DOCUMENTATION │ + ┌───┴─────────────────────────────────┴───┐ + │ IMPLEMENTATION SEMANTICS │ ← Focus + ┌───┴─────────────────────────────────────────┴───┐ + │ API SEMANTICS │ ← Most Focus + └─────────────────────────────────────────────────┘ + + ↑ HARDER TO CHANGE LATER = MORE HUMAN REVIEW TIME +#+end_example + +*Key Insight:* A formatting issue is a 2-minute fix. A breaking API change after release is a multi-sprint disaster. Prioritize accordingly. + +* When to Use This Workflow + +Use this workflow when: +- Reviewing pull requests / merge requests +- Doing self-review before submitting code +- Pair reviewing with another developer + +* Effort Scaling + +Not all changes need the same scrutiny: + +| Change Type | Review Depth | Time | +|-------------+--------------+------| +| Typo/docs fix | Quick scan for correctness | 2-5 min | +| Bug fix | Verify fix + no regressions | 10-15 min | +| New feature | Full checklist, deep dive on logic | 30-60 min | +| Architectural change | Bring in senior reviewers, verify design | 60+ min | + +* The Core Checklist + +Use this for every review. If you answer "no" or "maybe" to any item, dig deeper using the relevant Deep Dive section. + +** 1. API Semantics (Hardest to Change - Focus Most Here) + +These questions matter most because changes here break things for other people. + +- [ ] *Simple?* Is the API as small as possible but as large as needed? +- [ ] *Predictable?* Does it follow existing patterns and the principle of least surprise? +- [ ] *Clean boundaries?* Is there clear separation between public API and internal implementation? +- [ ] *Generally useful?* Is the API helpful for common cases, not just one specific scenario? +- [ ] *Backwards compatible?* Are there any breaking changes to things users already rely on? + +** 2. Implementation Semantics (Focus Here) + +Once the API looks good, check how the code actually works. + +- [ ] *Correct?* Does it actually do what the requirements/ticket says? +- [ ] *Logically sound?* Are there edge cases, off-by-one errors, or logic bugs? +- [ ] *Simple?* Could this be simpler? Is there unnecessary complexity or over-engineering? +- [ ] *Robust?* Does it handle errors gracefully? Concurrency issues? +- [ ] *Secure?* Is user input validated? No hardcoded secrets? (See Security Deep Dive) +- [ ] *Performant?* Any expensive operations, N+1 queries, or scaling concerns? (See Performance Deep Dive) +- [ ] *Observable?* Is there appropriate logging, metrics, or tracing? +- [ ] *Dependencies OK?* Do new libraries pull their weight? License acceptable? + +** 3. Documentation + +Good code still needs good explanations. + +- [ ] *New features documented?* README, API docs, or user guides updated? +- [ ] *Code comments explain "why"?* Complex logic or non-obvious decisions explained? +- [ ] *Readable?* Documentation is clear, accurate, no major errors? + +** 4. Tests (Automate the "Pass/Fail" Check) + +Don't just check if tests pass - check if they're good tests. + +- [ ] *Tests exist?* Are new features and bug fixes covered by tests? +- [ ] *Test the right things?* Do tests verify behavior, not implementation details? +- [ ] *Edge cases covered?* Are boundary conditions and error paths tested? +- [ ] *Right test type?* Unit tests where possible, integration tests where necessary? +- [ ] *Would catch regressions?* If someone breaks this later, will a test fail? + +** 5. Code Style (Automate This) + +Let linters and formatters handle these. Only check manually if tooling isn't set up. + +- [ ] *Linter passes?* No style violations flagged by automated tools? +- [ ] *Readable?* Methods reasonable length? Clear naming? +- [ ] *DRY?* No unnecessary duplication? + +** 6. Process & Collaboration + +Code review is a conversation, not an inspection. + +- [ ] *PR has context?* Does the description explain WHY, not just what? +- [ ] *Reasonable size?* Is this reviewable in one sitting? (<400 lines ideal) +- [ ] *Self-reviewed?* Has the author clearly reviewed their own code first? +- [ ] *No debug artifacts?* Console.logs, commented code, TODOs without tickets removed? + +* Deep Dives + +Use these when the Core Checklist flags a concern. Each section provides detailed checks for specific areas. + +** Security Deep Dive + +When "Is it secure?" raises concerns, check these: + +*** Input Validation +- [ ] All user input validated and sanitized +- [ ] SQL queries parameterized (no string concatenation) +- [ ] Output encoded to prevent XSS +- [ ] File uploads validated (type, size, content) +- [ ] Path traversal prevented + +*** Authentication & Authorization +- [ ] Auth checks at every access point +- [ ] Session management secure +- [ ] CSRF protections for state-changing operations +- [ ] Rate limiting where appropriate + +*** Secrets & Data +- [ ] No hardcoded credentials (use env vars/secrets manager) +- [ ] Sensitive data encrypted at rest and in transit +- [ ] Error messages don't leak system information +- [ ] Logs exclude sensitive data + +*** Common Vulnerabilities (OWASP) +- [ ] No SQL injection vectors +- [ ] No command injection vectors +- [ ] No unsafe deserialization +- [ ] Proper HTTP security headers + +** Performance Deep Dive + +When "Is it performant?" raises concerns, check these: + +*** Database +- [ ] No N+1 query problems +- [ ] Appropriate indexes exist +- [ ] No SELECT * - explicit columns only +- [ ] Pagination for large result sets +- [ ] Connection pooling used + +*** Code Efficiency +- [ ] No unnecessary loops or iterations +- [ ] Appropriate data structures (HashMap vs List lookups) +- [ ] Expensive operations cached where beneficial +- [ ] Large collections processed efficiently (streaming vs loading all) +- [ ] Resources properly closed (connections, file handles) + +*** Scaling +- [ ] Will this work with 10x/100x the data? +- [ ] Async operations used where beneficial +- [ ] No blocking operations in hot paths + +** Concurrency Deep Dive + +When reviewing multi-threaded or async code: + +- [ ] Shared mutable state properly synchronized +- [ ] No race conditions +- [ ] Lazy initialization is thread-safe +- [ ] Getters don't return mutable internal objects +- [ ] Thread-safe collections used where needed +- [ ] Consistent lock ordering (no deadlocks) +- [ ] Loop variables passed into closures/lambdas (not captured) +- [ ] Proper timeouts on blocking operations +- [ ] Tests run with race detector where available + +** Error Handling Deep Dive + +When reviewing error handling and logging: + +- [ ] Exceptions caught at appropriate levels +- [ ] System "fails safe" (deny on error, not grant) +- [ ] Transactions rolled back on failure +- [ ] Error messages help debugging but don't leak info +- [ ] Correlation IDs for tracing across services +- [ ] Log levels appropriate (DEBUG/INFO/WARN/ERROR) +- [ ] Critical failures trigger alerts + +** API Compatibility Deep Dive + +When reviewing API changes: + +*** Non-Breaking (Safe) +- Adding optional fields +- Adding new endpoints +- Making required fields optional +- Adding new enum values (if clients handle unknown) + +*** Breaking (Dangerous) +- Removing fields or endpoints +- Changing field types +- Making optional fields required +- Changing semantic meaning of fields +- Changing response status codes + +*** If Breaking Changes Required +- [ ] Version bumped appropriately +- [ ] Migration path documented +- [ ] Deprecation warnings added +- [ ] Transition period planned + +** Dependencies Deep Dive + +When new dependencies are added: + +- [ ] Actually necessary (not just convenience) +- [ ] From trusted, reputable source +- [ ] Actively maintained (recent commits, responsive maintainers) +- [ ] No known vulnerabilities (check Dependabot/Snyk) +- [ ] License compatible with project +- [ ] Doesn't bloat dependency tree excessively +- [ ] Version pinned in lock file + +* How to Give Feedback + +** Frame as Questions +Bad: "This is wrong." +Good: "I noticed we used X here - would Y be more efficient for this case?" + +** Distinguish Severity +- *Blocker:* Must fix before merge (security issue, breaking bug) +- *Should fix:* Important but could be follow-up PR +- *Suggestion:* Nice to have, author's discretion +- *Nit:* Minor style preference, totally optional + +** Call Out Good Stuff +Reviews are for learning too. If you see a clever solution or clean pattern, say so. + +** Be Timely +Don't let PRs sit. If you're stuck, ask someone to pair-review with you. + +* Anti-Patterns to Avoid + +** The Empty LGTM +Never rubber-stamp with "Looks Good To Me" without actually reviewing. Even noting one thing you liked shows you read it. + +** Nitpicking +Don't comment on spaces, tabs, or semicolons. Let automated tools handle formatting. Focus on logic and design. + +** The Stall +Don't let PRs languish for days. Review within 24 hours or communicate delays. + +** Bikeshedding +Don't spend 30 minutes debating variable names while ignoring architectural concerns. + +** Review by Line Count +400 lines of careful refactoring may need less scrutiny than 40 lines of new auth logic. Adjust effort to risk, not size. + +* Validation Checklist + +Before approving, verify: + +- [ ] Core checklist complete (all 5 pyramid levels) +- [ ] Any "maybe" answers investigated with Deep Dives +- [ ] Feedback framed constructively +- [ ] Blockers clearly marked vs suggestions +- [ ] You could explain this code to someone else + +* Sources + +- [[https://www.morling.dev/blog/the-code-review-pyramid/][The Code Review Pyramid]] - Gunnar Morling +- [[https://google.github.io/eng-practices/review/][Google Engineering Practices]] - Code Review Guidelines +- [[https://owasp.org/www-project-code-review-guide/][OWASP Code Review Guide]] - Security Checklist +- [[https://github.com/code-review-checklists/java-concurrency][Java Concurrency Checklist]] - Thread Safety +- [[https://go.dev/wiki/CodeReviewConcurrency][Go Concurrency Wiki]] - Race Conditions diff --git a/docs/session-context.org b/docs/session-context.org new file mode 100644 index 0000000..580be99 --- /dev/null +++ b/docs/session-context.org @@ -0,0 +1,51 @@ +#+TITLE: Session Context - Active Session +#+DATE: 2026-01-23 + +* Current Session: Friday 2026-01-23 @ 11:28 CST + +** Current Task +Code review of archzfs project complete. Issues documented in todo.org. + +** Completed This Session +- Merged zfsbootmenu branch to main (fast-forward, pushed to origin) +- ratio booted successfully with ZFSBootMenu +- Researched code review best practices (10+ web searches) +- Created docs/project-workflows/code-review.org + - Core Checklist: ~27 items across pyramid levels + - Deep Dives: Security, Performance, Concurrency, Error Handling, API Compatibility, Dependencies +- Conducted senior developer code review of entire archzfs codebase +- Added 14 new tasks to todo.org based on code review findings + +** Code Review Findings Summary +*** Critical (Priority A) +- README.org has outdated GRUB references (now uses ZFSBootMenu) +- Missing LICENSE file (referenced but doesn't exist) +- custom/archsetup-zfs is a non-functional skeleton +- Initial password should be configurable, not hardcoded + +*** Important (Priority B) +- Stale SESSION-CONTEXT.md should be deleted +- PLAN-zfsbootmenu-implementation.org should move to docs/ +- Makefile lint target swallows errors +- Unclear directories need documentation or gitignore +- Empty docs/someday-maybe.org + +*** Cleanup (Priority C) +- Inconsistent shebangs across scripts +- Inconsistent email addresses +- No .editorconfig for formatting +- Test scripts need documentation + +** Files Created This Session +- docs/project-workflows/code-review.org - Code review workflow + +** Files Modified This Session +- todo.org - Added Makefile targets, code review tasks (14 new items) + +** Pending User Review +- docs/project-workflows/code-review.org (task in todo.org) + +** Next Steps +- User to review code-review.org document +- Address Priority A issues from code review +- Delete SESSION-CONTEXT.md (root) after confirming this file is canonical diff --git a/todo.org b/todo.org index 9415a18..7d7b192 100644 --- a/todo.org +++ b/todo.org @@ -1,5 +1,9 @@ * Open Work +** TODO [#A] Review code review workflow document and provide feedback +Review [[file:docs/project-workflows/code-review.org][docs/project-workflows/code-review.org]] and refine based on feedback. +Created: 2026-01-23 + ** TODO [#A] Fix mkinitcpio configuration in install-archzfs (causes boot failure) After kernel updates or mkinitcpio regeneration, systems fail to boot because install-archzfs leaves incorrect mkinitcpio configuration from the live ISO environment. @@ -252,6 +256,98 @@ It provides boot environment selection, snapshot rollback from boot menu, and re *** Reference https://zfsbootmenu.org/ +** TODO [#A] Update README.org - remove all GRUB references (now uses ZFSBootMenu) +README.org contains multiple outdated references to GRUB that are now incorrect: +- Line 19: "EFI Boot Redundancy - GRUB installed on all disks" - now uses ZFSBootMenu +- Lines 417-472: Entire section on "grub-zfs-snap" and GRUB snapshot boot entries - doesn't exist +- Lines 98-100: Project structure lists grub-zfs-snap, zfs-snap-prune, 40_zfs_snapshots - files don't exist + +*** Actions +- Remove/update "EFI Boot Redundancy" line to mention ZFSBootMenu +- Delete or rewrite "ZFS Snapshot Boot Entries (grub-zfs-snap)" section +- Update project structure to reflect actual files +- Update "Post-Installation" section for ZFSBootMenu workflow + +** TODO [#A] Add LICENSE file (GPL-3.0) +README.org line 723 references [[file:LICENSE][LICENSE]] but the file doesn't exist. +Create LICENSE file with GPL-3.0 text as stated in README. + +** TODO [#A] Delete or complete custom/archsetup-zfs +The script has full function definitions but main() just prints "this is a skeleton". +A skeleton script that pretends to work is worse than nothing. + +*** Options +1. Delete it entirely - users can run archsetup from ~/code/archsetup +2. Complete the implementation +3. Replace with a simple launcher that calls archsetup with ZFS-specific flags + +** TODO [#A] Add initial user password to install-archzfs config +Currently hardcoded as "welcome" in archsetup-zfs. Should be configurable via: +- Interactive prompt during install-archzfs +- Config file option for unattended installs +- Document that password must be changed on first login + +** TODO [#B] Delete stale SESSION-CONTEXT.md +SESSION-CONTEXT.md in project root is from 2026-01-19 and references old GRUB workflow. +Superseded by docs/session-context.org. Delete to avoid confusion. + +** TODO [#B] Move PLAN-zfsbootmenu-implementation.org to docs/ +Implementation plan files should be in docs/ or archived after completion. +The plan is complete - ZFSBootMenu is now the bootloader. + +** TODO [#B] Clean up docs/ directory +- Delete docs/someday-maybe.org (empty, 0 bytes) +- Move date-specific docs from assets/ to docs/ for consistency +- Document or delete docs/scripts/ directory (unclear purpose) + +** TODO [#B] Fix Makefile lint target to fail on errors +Current lint target has `|| true` which swallows shellcheck errors: +#+BEGIN_SRC makefile +lint: + @shellcheck -x build.sh scripts/*.sh custom/install-archzfs ... || true +#+END_SRC + +Change to actually fail on lint errors so CI can catch issues. + +** TODO [#B] Document or gitignore unclear directories +These directories exist but aren't documented or gitignored: +- zfs-packages/ - unclear purpose +- reference-repos/ - unclear purpose +- test-logs/ - should probably be gitignored + +** TODO [#B] Fill in README.org #+AUTHOR field +Line 2 has empty #+AUTHOR: - looks unfinished. Add author info. + +** TODO [#C] Standardize shell script conventions +*** Shebang inconsistency +- build.sh: #!/bin/bash +- zfssnapshot: #!/bin/env bash +- archsetup-zfs: #!/bin/sh +Pick one convention (recommend #!/usr/bin/env bash for portability) + +*** Email inconsistency +- Some files: c@cjennings.net +- archsetup-zfs: craigmartinjennings@gmail.com +Standardize to one email address. + +** TODO [#C] Add .editorconfig for consistent formatting +No project-wide formatting rules. Add .editorconfig to enforce: +- Indent style (spaces vs tabs) +- Indent size +- End of line +- Trim trailing whitespace +- Final newline + +** TODO [#C] Consolidate test scripts documentation +scripts/ has multiple test files with unclear relationships: +- test-vm.sh - Manual VM testing +- sanity-test.sh - Quick automated checks +- test-install.sh - Installation testing +- full-test.sh - Comprehensive testing +- test-zfs-snap-prune.sh - Unit tests for prune script + +Document the testing strategy and when to use each script. + ** TODO [#B] Set up CI/CD pipeline for automated ISO builds *** Options to evaluate @@ -365,6 +461,33 @@ Extract into library of functions that can be sourced and tested: - Reusable functions for other projects - Clearer code organization +** TODO [#B] Create Makefile with distinct build targets +Replace or supplement build.sh with a Makefile for cleaner build orchestration. + +*** Proposed targets +- make deps - Install all dependencies (pacman + AUR) needed to build and test +- make lint - Run shellcheck on all bash scripts +- make build - Full ISO build (current build.sh behavior) +- make clean - Remove work/ and output/ directories +- make test - Run VM tests (single disk, mirror, raidz) - depends on lint +- make test-quick - Quick single-disk test only - depends on lint +- make aur - Build AUR packages only +- make iso - Build ISO only (skip AUR if already built) +- make deploy - Copy ISO to truenas.local AND Ventoy drive (if inserted) +- make all - Full build + tests + +*** Benefits +- Familiar interface for developers +- Dependency tracking (rebuild only what changed) +- Parallel execution where possible +- Self-documenting (make help) +- Easy CI/CD integration + +*** Considerations +- Keep build.sh as the underlying implementation +- Makefile calls build.sh with appropriate flags +- Or refactor build.sh logic into Makefile directly + ** TODO [#B] Add Docker/Podman container support for builds Idea from: https://github.com/stevleibelt/arch-linux-live-cd-iso-with-zfs -- cgit v1.2.3