fix: keep captured comments in chronological order - #204
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. Reviewed September 15, 2026, 3:22 AM ET / 07:22 UTC. ClawSweeper reviewWhat this changesThe PR sorts exported conversation comments by their actual timestamps and adds regression coverage, documentation, and a changelog entry. Merge readiness⛔ Blocked before merge - 2 items remain The fix remains necessary: main and v0.10.0 still sort timestamps lexically. No introduced correctness defect was found, but the reported CLI verification needs observable output before merge. Priority: P2 Review scores
Verification
How this fits togetherGitcrawl capture reads synced GitHub conversations from a local SQLite archive and exports deterministic JSON. Comment ordering affects the exported conversation and its semantic content hash. flowchart LR
A[Synced SQLite archive] --> B[Completed comment observation]
B --> C[Validate and normalize timestamps]
C --> D[Sort by time then kind and ID]
D --> E[Hash conversation]
E --> F[JSON capture output]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep the existing capture schema and timestamp normalization while comparing comment timestamps chronologically, preserving empty-time and kind/ID ordering. Do we have a high-confidence way to reproduce the issue? Yes, source inspection establishes the failure: comments at the whole second, .1, and .11 are sorted incorrectly by main's string comparator. No runtime reproduction was executed in this review. Is this the best way to solve the issue? Yes. Comparing already validated and normalized timestamps is a narrow repair; normalization also makes the retained string-equality check safe for equivalent-instant ties. AGENTS.md: found, but no applicable review policy affected this item. Codex review notes: model internal, reasoning medium; reviewed against efaeb1ff0c5b. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
What Problem This Solves
Conversation capture could export comments out of chronological order when their source timestamps used different fractional-second precision.
User Impact
Captured comments now follow their actual source times, including nanoseconds and timezone offsets. The capture schema and deterministic kind/ID tie order remain unchanged; comments without a source time still sort first.
Why This Change Was Made
RFC3339Nano strings do not sort chronologically as text:
.11Zsorts before.1Z, and both sort before the whole-secondZ. The comparator now uses the already validated timestamps as instants.Evidence
A locally built CLI synced a synthetic HTTP fixture and exported comments at the whole second,
.1, and.11. Before the fix, capture returned them in reverse chronological order; afterward it returns the correct order. The regression test additionally covers nanoseconds, absent timestamps, equivalent offsets and precision, and kind/ID ties.Full
make checkpassed, including coverage, vet, vulnerability/dead-code checks, smoke, release-script tests, docs, and snapshot packaging. Independent autoreview is clean through P2.