Commit
·
85be054
1
Parent(s):
ed93dd8
docs: expand technical debt audit with external review findings
Browse filesValidates and documents 5 additional findings from external senior review:
P2 Issues (3 total):
- Temp dir leak in UI path (cleanup_staging defaults to False)
- Brittle git branch dependency on third-party fork
- Silent empty dataset (existing, mitigated)
P3 Issues (6 total):
- Latent SSRF vector in staging (dead code, document for future)
- Redundant float64 cast (no-op, but float32 would save memory)
- Base64 data URL overhead (standard Gradio pattern)
- Type ignore comments (expected, 3 existing)
All claims validated from first principles. Finding 2 downgraded
from P2 to P3 after verifying nibabel already returns float64.
- docs/TECHNICAL_DEBT.md +173 -7
docs/TECHNICAL_DEBT.md
CHANGED
|
@@ -1,19 +1,19 @@
|
|
| 1 |
# Technical Debt and Known Issues
|
| 2 |
|
| 3 |
-
> **Last Audit**: December 2025
|
| 4 |
-
> **Auditor**: Claude Code
|
| 5 |
-
> **Status**:
|
| 6 |
|
| 7 |
## Summary
|
| 8 |
|
| 9 |
-
Full architectural review completed. The codebase is **production-ready** with
|
| 10 |
|
| 11 |
| Severity | Count | Description |
|
| 12 |
|----------|-------|-------------|
|
| 13 |
| P0 (Critical) | 0 | None |
|
| 14 |
| P1 (High) | 0 | None |
|
| 15 |
-
| P2 (Medium) |
|
| 16 |
-
| P3 (Low) |
|
| 17 |
|
| 18 |
---
|
| 19 |
|
|
@@ -49,6 +49,63 @@ def build_local_dataset(data_dir: Path) -> LocalDataset:
|
|
| 49 |
|
| 50 |
---
|
| 51 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 52 |
## P3: Type Ignore Comments (Expected)
|
| 53 |
|
| 54 |
These `# type: ignore` comments are **correct and expected** due to library typing limitations:
|
|
@@ -82,6 +139,104 @@ def is_hf_spaces(self) -> bool:
|
|
| 82 |
|
| 83 |
---
|
| 84 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 85 |
## Good Patterns Observed
|
| 86 |
|
| 87 |
### Error Handling
|
|
@@ -140,4 +295,15 @@ def get_demo() -> gr.Blocks:
|
|
| 140 |
|
| 141 |
## Conclusion
|
| 142 |
|
| 143 |
-
The codebase is **
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 1 |
# Technical Debt and Known Issues
|
| 2 |
|
| 3 |
+
> **Last Audit**: December 2025 (Revision 2)
|
| 4 |
+
> **Auditor**: Claude Code + External Senior Review
|
| 5 |
+
> **Status**: Production-ready with known limitations
|
| 6 |
|
| 7 |
## Summary
|
| 8 |
|
| 9 |
+
Full architectural review completed with external validation. The codebase is **production-ready** with documented limitations.
|
| 10 |
|
| 11 |
| Severity | Count | Description |
|
| 12 |
|----------|-------|-------------|
|
| 13 |
| P0 (Critical) | 0 | None |
|
| 14 |
| P1 (High) | 0 | None |
|
| 15 |
+
| P2 (Medium) | 3 | Temp dir leak, silent empty dataset, brittle git dep |
|
| 16 |
+
| P3 (Low) | 6 | Type ignores, SSRF vector, base64 overhead, float64 |
|
| 17 |
|
| 18 |
---
|
| 19 |
|
|
|
|
| 49 |
|
| 50 |
---
|
| 51 |
|
| 52 |
+
## P2: Unbounded Temporary Directory Accumulation (UI Path)
|
| 53 |
+
|
| 54 |
+
**Location**: `src/stroke_deepisles_demo/pipeline.py:61,106-113` and `src/stroke_deepisles_demo/ui/app.py:51`
|
| 55 |
+
|
| 56 |
+
**Issue**: The `run_pipeline_on_case()` function creates temporary directories using `tempfile.mkdtemp()` and defaults to `cleanup_staging=False`. The CLI correctly overrides this to `True`, but the Gradio UI does not pass this parameter.
|
| 57 |
+
|
| 58 |
+
**Evidence**:
|
| 59 |
+
```python
|
| 60 |
+
# pipeline.py:61 - default is False
|
| 61 |
+
cleanup_staging: bool = False,
|
| 62 |
+
|
| 63 |
+
# cli.py:76 - CLI correctly overrides ✅
|
| 64 |
+
cleanup_staging=True,
|
| 65 |
+
|
| 66 |
+
# app.py:51 - UI does NOT pass it ❌
|
| 67 |
+
result = run_pipeline_on_case(case_id, fast=fast_mode, compute_dice=True)
|
| 68 |
+
```
|
| 69 |
+
|
| 70 |
+
**Risk**: In a long-running Gradio app on HF Spaces, each inference request creates a new temporary directory (`deepisles_pipeline_*`) that is never deleted. This will eventually consume all available disk space, causing DoS.
|
| 71 |
+
|
| 72 |
+
**Mitigation**: HF Spaces containers are ephemeral and restart periodically, which limits accumulation. However, heavy usage could still trigger disk pressure.
|
| 73 |
+
|
| 74 |
+
**Recommended Fix**:
|
| 75 |
+
```python
|
| 76 |
+
# Option A: Fix UI to pass cleanup_staging=True
|
| 77 |
+
result = run_pipeline_on_case(case_id, fast=fast_mode, compute_dice=True, cleanup_staging=True)
|
| 78 |
+
|
| 79 |
+
# Option B: Change default to True (breaking change for programmatic users who want to keep staging)
|
| 80 |
+
cleanup_staging: bool = True,
|
| 81 |
+
```
|
| 82 |
+
|
| 83 |
+
**Status**: Should fix before production deployment
|
| 84 |
+
|
| 85 |
+
---
|
| 86 |
+
|
| 87 |
+
## P2: Brittle Git Branch Dependency
|
| 88 |
+
|
| 89 |
+
**Location**: `pyproject.toml:23`
|
| 90 |
+
|
| 91 |
+
**Issue**: The project depends on a specific branch of a third-party fork for the `datasets` library.
|
| 92 |
+
|
| 93 |
+
**Evidence**:
|
| 94 |
+
```toml
|
| 95 |
+
"datasets @ git+https://github.com/CloseChoice/datasets.git@feat/bids-loader-streaming-upload-fix",
|
| 96 |
+
```
|
| 97 |
+
|
| 98 |
+
**Risk**: If the repository owner deletes the branch `feat/bids-loader-streaming-upload-fix` or force-pushes changes, all builds will fail immediately. This endangers reproducibility and CI/CD reliability.
|
| 99 |
+
|
| 100 |
+
**Recommended Fix** (in priority order):
|
| 101 |
+
1. Get the fix merged upstream to `huggingface/datasets` and pin to a released version
|
| 102 |
+
2. Fork the repository to the project's organization for permanence
|
| 103 |
+
3. Vendor the required changes directly
|
| 104 |
+
|
| 105 |
+
**Status**: Monitor upstream; consider forking if not merged within 30 days
|
| 106 |
+
|
| 107 |
+
---
|
| 108 |
+
|
| 109 |
## P3: Type Ignore Comments (Expected)
|
| 110 |
|
| 111 |
These `# type: ignore` comments are **correct and expected** due to library typing limitations:
|
|
|
|
| 139 |
|
| 140 |
---
|
| 141 |
|
| 142 |
+
## P3: Latent SSRF Vector in Staging Utility
|
| 143 |
+
|
| 144 |
+
**Location**: `src/stroke_deepisles_demo/data/staging.py:124-133`
|
| 145 |
+
|
| 146 |
+
**Issue**: The `_materialize_nifti()` helper function contains code to download files from arbitrary HTTP/HTTPS URLs using `requests.get()`.
|
| 147 |
+
|
| 148 |
+
**Evidence**:
|
| 149 |
+
```python
|
| 150 |
+
elif isinstance(source, str):
|
| 151 |
+
if source.startswith(("http://", "https://")):
|
| 152 |
+
import requests
|
| 153 |
+
response = requests.get(source, stream=True, timeout=30)
|
| 154 |
+
response.raise_for_status()
|
| 155 |
+
# ...writes to local file
|
| 156 |
+
```
|
| 157 |
+
|
| 158 |
+
**Current State**: This code path is **currently unreachable** because:
|
| 159 |
+
- `CaseFiles` from `adapter.py` only contains local `Path` objects
|
| 160 |
+
- No user-facing interface accepts URL input
|
| 161 |
+
|
| 162 |
+
**Risk**: If a future feature allows user-supplied URLs (e.g., "Load from URL" button), this becomes a Server-Side Request Forgery (SSRF) vulnerability. An attacker could:
|
| 163 |
+
- Probe internal networks from the HF Space container
|
| 164 |
+
- Access cloud metadata services (169.254.169.254)
|
| 165 |
+
- Exfiltrate data to attacker-controlled servers
|
| 166 |
+
|
| 167 |
+
**Recommended Fix**:
|
| 168 |
+
```python
|
| 169 |
+
# Option A: Remove the HTTP code path entirely (recommended if not needed)
|
| 170 |
+
# Option B: Add domain allowlist if URL loading is required
|
| 171 |
+
ALLOWED_DOMAINS = {"huggingface.co", "github.com"}
|
| 172 |
+
parsed = urllib.parse.urlparse(source)
|
| 173 |
+
if parsed.netloc not in ALLOWED_DOMAINS:
|
| 174 |
+
raise ValueError(f"URL domain not allowed: {parsed.netloc}")
|
| 175 |
+
```
|
| 176 |
+
|
| 177 |
+
**Status**: Acceptable if no URL input features are added; document for future developers
|
| 178 |
+
|
| 179 |
+
---
|
| 180 |
+
|
| 181 |
+
## P3: Redundant float64 Cast in NIfTI Loading
|
| 182 |
+
|
| 183 |
+
**Location**: `src/stroke_deepisles_demo/metrics.py:27`
|
| 184 |
+
|
| 185 |
+
**Issue**: The code explicitly casts NIfTI data to `float64`, but nibabel's `get_fdata()` already returns `float64` by default.
|
| 186 |
+
|
| 187 |
+
**Evidence**:
|
| 188 |
+
```python
|
| 189 |
+
data = img.get_fdata().astype(np.float64) # Redundant - get_fdata() already returns float64
|
| 190 |
+
```
|
| 191 |
+
|
| 192 |
+
**Analysis**:
|
| 193 |
+
- The `.astype(np.float64)` call is a no-op in most cases
|
| 194 |
+
- It does NOT double memory as initially claimed (nibabel already loads as float64)
|
| 195 |
+
- However, `float32` would be sufficient for Dice computation and would halve memory usage
|
| 196 |
+
|
| 197 |
+
**Risk**: Increased memory usage for large volumes. A 512×512×256 volume:
|
| 198 |
+
- float64: ~512 MB
|
| 199 |
+
- float32: ~256 MB (50% savings)
|
| 200 |
+
|
| 201 |
+
**Recommended Fix** (optional optimization):
|
| 202 |
+
```python
|
| 203 |
+
data = img.get_fdata(dtype=np.float32) # Use float32 for memory efficiency
|
| 204 |
+
```
|
| 205 |
+
|
| 206 |
+
**Status**: Low priority optimization; current behavior is correct but suboptimal
|
| 207 |
+
|
| 208 |
+
---
|
| 209 |
+
|
| 210 |
+
## P3: Base64 Data URL Overhead for NiiVue Viewer
|
| 211 |
+
|
| 212 |
+
**Location**: `src/stroke_deepisles_demo/ui/viewer.py:47-51`
|
| 213 |
+
|
| 214 |
+
**Issue**: NIfTI files are embedded in HTML as base64 data URLs, incurring ~33% size overhead.
|
| 215 |
+
|
| 216 |
+
**Evidence**:
|
| 217 |
+
```python
|
| 218 |
+
with nifti_path.open("rb") as f:
|
| 219 |
+
nifti_bytes = f.read()
|
| 220 |
+
nifti_b64 = base64.b64encode(nifti_bytes).decode("utf-8")
|
| 221 |
+
return f"data:application/octet-stream;base64,{nifti_b64}"
|
| 222 |
+
```
|
| 223 |
+
|
| 224 |
+
**Impact**:
|
| 225 |
+
- A 5 MB NIfTI file becomes ~6.7 MB in the DOM
|
| 226 |
+
- Large DOM sizes can freeze browser tabs
|
| 227 |
+
- Server RAM spikes during encoding
|
| 228 |
+
|
| 229 |
+
**Why It Exists**: This is the standard pattern for Gradio HTML components. Gradio doesn't expose a straightforward static file serving API.
|
| 230 |
+
|
| 231 |
+
**Alternative** (significant refactor):
|
| 232 |
+
- Use `gr.File` output and let NiiVue load from a relative URL
|
| 233 |
+
- Add custom FastAPI route to serve files as binary streams
|
| 234 |
+
- Use Gradio's `add_static_files` (if supported in Gradio 6.x)
|
| 235 |
+
|
| 236 |
+
**Status**: Acceptable for demo; revisit if performance issues arise in production
|
| 237 |
+
|
| 238 |
+
---
|
| 239 |
+
|
| 240 |
## Good Patterns Observed
|
| 241 |
|
| 242 |
### Error Handling
|
|
|
|
| 295 |
|
| 296 |
## Conclusion
|
| 297 |
|
| 298 |
+
The codebase is **well-architected and production-ready** with documented limitations.
|
| 299 |
+
|
| 300 |
+
### Before Production Deployment
|
| 301 |
+
1. **Fix P2: Temp dir leak** - Add `cleanup_staging=True` to UI path (1 line change)
|
| 302 |
+
|
| 303 |
+
### Monitor / Low Priority
|
| 304 |
+
2. **P2: Git dependency** - Track upstream merge; fork if needed
|
| 305 |
+
3. **P2: Empty dataset** - Already mitigated downstream
|
| 306 |
+
4. **P3 issues** - Document for future developers; no immediate action
|
| 307 |
+
|
| 308 |
+
### Verdict
|
| 309 |
+
The codebase follows clean code principles with proper error handling, type safety, and fail-loud design. The identified issues are manageable and do not block deployment.
|