Commit
·
ec0a5c3
1
Parent(s):
bfc21c1
docs: add decision record for PR #55 evaluation
Browse files- Document CodeRabbit findings (35+ critical issues)
- Include verbatim quotes from author with analysis
- Compare contribution standards vs merged PRs
- Explain decision not to merge untested 17k LoC PR
This provides permanent documentation for future contributors
explaining why the PR was not accepted.
docs/decisions/2025-11-27-pr55-evaluation.md
CHANGED
|
@@ -23,12 +23,56 @@ CodeRabbit's automated review identified **35+ critical issues**:
|
|
| 23 |
|
| 24 |
The 3 import errors cause pytest to crash during collection, preventing any tests from running.
|
| 25 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 26 |
## Claims vs Reality
|
| 27 |
|
| 28 |
| Claim | Reality |
|
| 29 |
|-------|---------|
|
| 30 |
| "nothing is replaced, just added" | `src/orchestrator.py` renamed to `src/legacy_orchestrator.py`; `CLAUDE.md`, `AGENTS.md`, `GEMINI.md` deleted |
|
| 31 |
| "3 failing tests on a 13k LoC PR is not a major issue" | Those 3 tests crash pytest during collection - entire test suite cannot run |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 32 |
|
| 33 |
## Decision
|
| 34 |
|
|
@@ -38,12 +82,16 @@ The PR was not merged for the following reasons:
|
|
| 38 |
2. **Parallel architecture, not incremental improvement** - Introduces entirely different orchestration system rather than building on existing working code
|
| 39 |
3. **Maintenance burden** - Would require maintaining two separate orchestration systems
|
| 40 |
4. **Existing code labeled "legacy"** - Working, tested code renamed to "legacy" in favor of untested code
|
|
|
|
| 41 |
|
| 42 |
## Context
|
| 43 |
|
| 44 |
-
This project is a HuggingFace Spaces hackathon entry
|
|
|
|
|
|
|
| 45 |
|
| 46 |
## Links
|
| 47 |
|
| 48 |
- [PR #55](https://github.com/The-Obstacle-Is-The-Way/DeepCritical-HFSpace/pull/55)
|
| 49 |
- [CodeRabbit Review](https://github.com/The-Obstacle-Is-The-Way/DeepCritical-HFSpace/pull/55#issuecomment-3587631560)
|
|
|
|
|
|
| 23 |
|
| 24 |
The 3 import errors cause pytest to crash during collection, preventing any tests from running.
|
| 25 |
|
| 26 |
+
## Author's Comments (Verbatim)
|
| 27 |
+
|
| 28 |
+
### Comment 1 (2025-11-28T01:09:25Z)
|
| 29 |
+
|
| 30 |
+
> "nothing is replaced , just added , report writer, proofreaders , websearch , rag , planner , orchestrator , pydantic graphs , agent and retrival factories , etc . that's why there's a lot of code , but it's not wired into the gradio demo yet ;-)"
|
| 31 |
+
|
| 32 |
+
**Analysis**: This claim is factually incorrect. Git diff shows:
|
| 33 |
+
|
| 34 |
+
- `src/orchestrator.py` was renamed to `src/legacy_orchestrator.py`
|
| 35 |
+
- `CLAUDE.md`, `AGENTS.md`, `GEMINI.md` were deleted
|
| 36 |
+
|
| 37 |
+
### Comment 2 (2025-11-28T01:11:14Z)
|
| 38 |
+
|
| 39 |
+
> "btw 3 failing tests on a 13k LoC PR is not a major issue , but i'll circle back tomorrow morning ... on this auspicious day : i am thankful for you and this work you did 🦃🦃🦃"
|
| 40 |
+
|
| 41 |
+
**Analysis**: Minimizes the severity. Those "3 failing tests" crash pytest during collection—the entire test suite cannot run. This is not "3 out of 300 failing"; it's "0 tests can execute."
|
| 42 |
+
|
| 43 |
+
### Comment 3 (2025-11-28T01:28:06Z)
|
| 44 |
+
|
| 45 |
+
> "@The-Obstacle-Is-The-Way , as fearless leader i volunteer you as maintainer on your repo :-) btw code rabbit is absolutely siiiick"
|
| 46 |
+
|
| 47 |
+
**Analysis**: Notably absent is any commitment to fix the 35+ issues CodeRabbit identified. A professional response would be: "Thanks for the review, I'll address those issues." Instead, only commentary on how "sick" the tool is.
|
| 48 |
+
|
| 49 |
## Claims vs Reality
|
| 50 |
|
| 51 |
| Claim | Reality |
|
| 52 |
|-------|---------|
|
| 53 |
| "nothing is replaced, just added" | `src/orchestrator.py` renamed to `src/legacy_orchestrator.py`; `CLAUDE.md`, `AGENTS.md`, `GEMINI.md` deleted |
|
| 54 |
| "3 failing tests on a 13k LoC PR is not a major issue" | Those 3 tests crash pytest during collection - entire test suite cannot run |
|
| 55 |
+
| "code rabbit is absolutely siiiick" | No commitment to fix any of the 35+ issues identified |
|
| 56 |
+
|
| 57 |
+
## Comparison: Contribution Standards
|
| 58 |
+
|
| 59 |
+
For context, here are merged PRs from @The-Obstacle-Is-The-Way on [DeepCritical/DeepCritical](https://github.com/DeepCritical/DeepCritical) (the upstream project also maintained by @Josephrp):
|
| 60 |
+
|
| 61 |
+
| PR | Description | Quality |
|
| 62 |
+
|----|-------------|---------|
|
| 63 |
+
| [#217](https://github.com/DeepCritical/DeepCritical/pull/217) | feat(embeddings): Implement standalone embeddings and FAISS vector store | Merged, tests passing |
|
| 64 |
+
| [#183](https://github.com/DeepCritical/DeepCritical/pull/183) | feat: implement GATK HaplotypeCaller MCP server | Merged, tests passing |
|
| 65 |
+
| [#179](https://github.com/DeepCritical/DeepCritical/pull/179) | feat: implement GunzipServer MCP tool for genomics | Merged, tests passing |
|
| 66 |
+
| [#175](https://github.com/DeepCritical/DeepCritical/pull/175) | Ship MCP Server Tools test suite + bug fix | Merged, tests passing |
|
| 67 |
+
| [#174](https://github.com/DeepCritical/DeepCritical/pull/174) | fix: resolve all 204 type errors (100% type-safe) | Merged, tests passing |
|
| 68 |
+
| [#173](https://github.com/DeepCritical/DeepCritical/pull/173) | fix: resolve PrepareChallenge forward reference error | Merged, tests passing |
|
| 69 |
+
|
| 70 |
+
These contributions:
|
| 71 |
+
|
| 72 |
+
- Were tested locally before submission
|
| 73 |
+
- Fixed issues when requested without pushback
|
| 74 |
+
- Did not dump 17k lines of untested code
|
| 75 |
+
- Did not minimize quality issues when identified
|
| 76 |
|
| 77 |
## Decision
|
| 78 |
|
|
|
|
| 82 |
2. **Parallel architecture, not incremental improvement** - Introduces entirely different orchestration system rather than building on existing working code
|
| 83 |
3. **Maintenance burden** - Would require maintaining two separate orchestration systems
|
| 84 |
4. **Existing code labeled "legacy"** - Working, tested code renamed to "legacy" in favor of untested code
|
| 85 |
+
5. **No commitment to fix issues** - After CodeRabbit identified 35+ critical bugs, no indication of intent to address them
|
| 86 |
|
| 87 |
## Context
|
| 88 |
|
| 89 |
+
This project (DeepCritical-1) is a HuggingFace Spaces hackathon entry, separate from the upstream [DeepCritical/DeepCritical](https://github.com/DeepCritical/DeepCritical) repository.
|
| 90 |
+
|
| 91 |
+
All contributors have direct push access to the HuggingFace Space. Contributors are encouraged to push directly to production when confident in their code, rather than submitting PRs with untested code for others to review and take responsibility for.
|
| 92 |
|
| 93 |
## Links
|
| 94 |
|
| 95 |
- [PR #55](https://github.com/The-Obstacle-Is-The-Way/DeepCritical-HFSpace/pull/55)
|
| 96 |
- [CodeRabbit Review](https://github.com/The-Obstacle-Is-The-Way/DeepCritical-HFSpace/pull/55#issuecomment-3587631560)
|
| 97 |
+
- [Upstream Project](https://github.com/DeepCritical/DeepCritical)
|