issue: update bb46 - refined implementation plan based on MVP learnings

2026-10-05 23:18:24 +00:00 · 2026-01-23 09:46:01 +00:00
parent c00403957e
commit 90d29f09e9
1 changed files with 179 additions and 5 deletions
@@ -15,6 +15,8 @@ Current testing gaps:

 ## Plan

+### Completed Phases
+
 - [x] Phase 1: Research existing test suites and specifications
  - Find git project's own http-backend tests
  - Survey cgit, gitea, gitlab test approaches
@@ -29,21 +31,20 @@ Current testing gaps:
  - Repository states (empty, large, many refs, shallow)
  - Client configurations and versions
  - Error conditions (malformed requests, timeouts, auth failures)
-  - **Output:** `docs/reference/git-http-test-matrix.md`
+  - **Output:** Research Report section (test matrix doc not committed)

 - [x] Phase 3: Implementation strategy
  - Integration test approach using TestRelay fixture
  - Real git client testing framework
  - Conformance test suite structure
  - CI/CD integration plan
-  - **Output:** `docs/explanation/git-integration-test-framework.md`
+  - **Output:** Architecture documented in Vision section

- [ ] Phase 4: Create test implementation in grasp-audit
+- [x] Phase 4: Create test implementation in grasp-audit
  - Build git protocol utilities in `grasp-audit/src/git/`
  - Implement priority test cases in `grasp-audit/src/specs/grasp01/`
  - Add to existing grasp-audit test suite
-  - Document test coverage
-  - **Output:** `grasp-audit/src/git/`, `grasp-audit/src/specs/grasp01/git_protocol.rs`
+  - **Output:** `grasp-audit/src/git/client.rs`, `grasp-audit/src/specs/grasp01/git_http_protocol.rs`

 - [x] Phase 5: MVP - Single test following existing pattern
  - Pick minimal test requiring minimal GitClient wrapper
@@ -52,6 +53,161 @@ Current testing gaps:
  - **Output:** Working MVP test demonstrating the pattern
  - **Completed:** `grasp-audit/src/git/client.rs`, `grasp-audit/src/specs/grasp01/git_http_protocol.rs`, `tests/git_http_protocol.rs`

+### Refined Implementation Plan (Post-MVP)
+
+**MVP Status: ✅ COMPLETE**
+- `GitClient` wrapper: `grasp-audit/src/git/client.rs` (238 lines)
+- Test function: `grasp-audit/src/specs/grasp01/git_http_protocol.rs` (187 lines)
+- Integration test: `tests/git_http_protocol.rs` using `isolated_test!` macro
+- Test passes in ~1.5 seconds
+
+**Key Learnings:**
+1. Pattern works well - grasp-audit provides test functions, ngit-grasp calls them with TestRelay
+2. Using real `git fetch` validates actual client behavior (not synthetic requests)
+3. `OwnerStateDataPushed` fixture provides repo with git data ready for testing
+4. The `isolated_test!` macro pattern is clean and reusable
+
+### Refined Test Matrix (65 scenarios, reduced from 95)
+
+#### P0 - Critical (15 tests)
+| ID | Test | Status | Rationale |
+|----|------|--------|-----------|
+| P0-01 | `test_gzip_encoded_upload_pack` | ✅ DONE | Caught production bug |
+| P0-02 | `test_gzip_encoded_receive_pack` | TODO | Push path validation |
+| P0-03 | `test_x_gzip_encoding_variant` | TODO | Git supports "x-gzip" |
+| P0-04 | `test_identity_encoding_explicit` | TODO | Explicit no-compression |
+| P0-05 | `test_no_encoding_header` | TODO | No header (implicit) |
+| P0-06 | `test_truncated_gzip_stream` | TODO | Error handling |
+| P0-07 | `test_protocol_v2_negotiation` | TODO | Modern git default |
+| P0-08 | `test_protocol_v0_fallback` | TODO | Legacy support |
+| P0-09 | `test_clone_basic` | TODO | Core operation |
+| P0-10 | `test_fetch_incremental` | TODO | Most common op |
+| P0-11 | `test_push_basic` | TODO | Write path |
+| P0-12 | `test_ls_remote` | TODO | Ref discovery |
+| P0-13 | `test_404_nonexistent_repo` | TODO | Must not return 200 |
+| P0-14 | `test_403_unauthorized_push` | TODO | Auth enforcement |
+| P0-15 | `test_correct_content_types` | TODO | Protocol compliance |
+
+#### P1 - Important (25 tests)
+- Protocol: v1 negotiation, version header parsing
+- Operations: shallow clone, partial clone, atomic push
+- Repo States: empty repo, many refs (>100), large objects
+- HTTP: chunked transfer, large Content-Length, keep-alive
+- Errors: malformed pkt-line, invalid Content-Type, timeout
+- Concurrent: parallel clones, parallel fetches
+
+#### P2 - Nice to Have (25 tests)
+- Advanced: HTTP/2, redirects, cookies
+- Security: path traversal, oversized requests
+- Performance: large packs, many small fetches
+- Edge Cases: peeled refs, symref, capabilities
+
+**Removed from original 95:**
+- HTTP Basic auth tests (ngit-grasp uses Nostr auth)
+- Half-auth scenarios (not applicable to GRASP)
+- Credential helper tests (not applicable)
+- Submodule tests (out of scope)
+
+### Fixture Optimization: `GitRepoFixture`
+
+**Problem:** Each test creates new repo (~1-2 seconds). 65 tests = 100+ seconds.
+
+**Solution:** Shared fixture struct:
+```rust
+pub struct GitRepoFixture {
+    pub npub: String,           // Owner's bech32 pubkey
+    pub identifier: String,     // Repo d-tag
+    pub relay_domain: String,   // HTTP endpoint
+    pub commit_hash: String,    // Existing commit
+    pub clone_url: String,      // Full clone URL
+    pub announcement: Event,    // Repo announcement
+    pub state_event: Event,     // State event
+}
+```
+
+**Performance Impact:**
+| Approach | Time for 65 tests |
+|----------|-------------------|
+| Per-test fixture | ~100 seconds |
+| Shared fixture | ~15 seconds |
+| Parallel + shared | ~8 seconds |
+
+### Remaining Implementation Phases
+
+- [ ] Phase 6: P0 Content-Encoding Tests
+  - `test_gzip_encoded_receive_pack` - Push with gzip
+  - `test_x_gzip_encoding_variant` - Alternative gzip header
+  - `test_identity_encoding_explicit` - Explicit identity
+  - `test_no_encoding_header` - No header (implicit)
+  - `test_truncated_gzip_stream` - Error handling
+  - Add `GitClient::with_encoding()` method
+
+- [ ] Phase 7: Protocol Version Tests
+  - Create `grasp-audit/src/git/pkt_line.rs` module
+  - `test_protocol_v2_negotiation`
+  - `test_protocol_v0_fallback`
+  - `test_protocol_v1_negotiation`
+  - Add `GitClient::with_protocol_version()` method
+
+- [ ] Phase 8: Core Operations Tests
+  - Implement `GitRepoFixture` struct
+  - `test_clone_basic`
+  - `test_fetch_incremental`
+  - `test_push_basic`
+  - `test_ls_remote`
+  - `test_shallow_clone`
+
+- [ ] Phase 9: Error Handling Tests
+  - `test_404_nonexistent_repo`
+  - `test_403_unauthorized_push`
+  - `test_correct_content_types`
+  - `test_malformed_pkt_line`
+  - `test_empty_repo_handling`
+
+- [ ] Phase 10: P1 Tests & Polish
+  - Implement shared fixture pattern with OnceCell
+  - Complete remaining P1 tests (20)
+  - Performance optimization
+  - Create `docs/reference/git-http-test-matrix.md`
+
+### Infrastructure Needed
+
+**GitClient Enhancements:**
+```rust
+impl GitClient {
+    pub fn builder(relay_domain: &str) -> GitClientBuilder;
+    pub fn with_protocol_version(self, version: u8) -> Self;
+    pub fn with_encoding(self, encoding: Option<&str>) -> Self;
+    pub async fn raw_request(&self, method: Method, path: &str,
+                             body: Option<&[u8]>, headers: HeaderMap) -> Result<Response>;
+}
+```
+
+**New Modules:**
+| Module | Purpose |
+|--------|---------|
+| `grasp-audit/src/git/pkt_line.rs` | Parse pkt-line format |
+| `grasp-audit/src/git/protocol.rs` | Protocol version handling |
+| `grasp-audit/src/git/fixture.rs` | GitRepoFixture struct |
+
+### Success Metrics
+
+| Metric | Target |
+|--------|--------|
+| P0 tests passing | 15/15 (100%) |
+| P1 tests passing | 25/25 (100%) |
+| Single test time | < 2 seconds |
+| Full suite time | < 2 minutes |
+| CI integration | < 5 minutes |
+
+### Definition of Done
+1. All P0 tests passing
+2. All P1 tests passing
+3. Test matrix documented
+4. Tests run in CI
+5. Full suite < 2 minutes
+6. grasp-audit git module reusable
+
 ## Vision

 Build comprehensive git HTTP smart protocol compliance tests in grasp-audit that:
@@ -591,3 +747,21 @@ test_expect_success 'git upload-pack --advertise-refs: v2' '
 6. **Empty repos:** Must handle repos with no refs (capabilities^{} line)
 7. **Peeled refs:** Annotated tags must show both tag and peeled object
 8. **Symref:** HEAD symref must be communicated in capabilities
+
+### 2026-01-23 [Session 10:00] - Plan Refinement
+- **Reviewed:** MVP implementation in bb46 worktree
+- **Validated:** Test passes (`cargo test --test git_http_protocol` ~1.5s)
+- **Refined:** Test matrix reduced from 95 to 65 scenarios
+  - Removed: HTTP Basic auth, half-auth, credential helpers, submodules (not applicable to GRASP)
+  - Kept: All content-encoding, protocol version, core operations, error handling tests
+- **Designed:** `GitRepoFixture` struct for fixture optimization
+  - Estimated performance improvement: 100s → 15s for full suite
+- **Created:** Detailed implementation phases 6-10
+  - Phase 6: P0 Content-Encoding tests
+  - Phase 7: Protocol version tests (requires pkt-line parser)
+  - Phase 8: Core operations tests (requires GitRepoFixture)
+  - Phase 9: Error handling tests
+  - Phase 10: P1 tests and polish
+- **Defined:** Success metrics and definition of done
+- **Next:** Merge MVP to main, then implement Phase 6
+