fix: reject thread selections with the wrong or empty scope - #205
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:28 AM ET / 07:28 UTC. ClawSweeper reviewWhat this changesThe PR rejects mismatched repository references and explicitly empty thread selections before CLI operations or terminal-browser navigation. Merge readiness⛔ Blocked before merge - 4 items remain This fix remains necessary: main and v0.10.0 still discard repository scope during selection parsing. No introduced correctness defect was found, but the reported built-CLI run needs an inspectable transcript before merge. Priority: P2 Review scores
Verification
How this fits togetherGitcrawl archives GitHub threads locally for search, enrichment, and triage. Selection parsing determines which repository and thread numbers reach synchronization, provider requests, local overrides, and terminal-browser navigation. flowchart TD
A[Pasted reference or selection flag] --> B[Parse thread reference]
C[Selected repository] --> D[Validate repository and nonempty selection]
B --> D
D -->|Invalid| E[Usage error before access]
D -->|Valid| F[Scoped archive or network operation]
D -->|Flag omitted| G[Repository-wide operation]
Decision needed
Why: The rejection prevents unintended work but changes the behavior of existing command invocations; accepting that operator impact is a maintainer choice. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep centralized scope validation, document the intentional rejection, and demonstrate that valid selections and omitted-filter workflows remain intact on fresh and existing archives. Do we have a high-confidence way to reproduce the issue? Yes, from source: main discards a qualified reference's repository and converts empty selections into an unrestricted filter. No runtime reproduction was executed during this read-only review. Is this the best way to solve the issue? Yes, the shared parser and early caller guards are a narrow repair that preserves valid reference forms; landing still requires inspectable runtime evidence and acceptance of the documented input restriction. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against a6ca802a8f67. 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
A pasted link for a different repository could close or otherwise select the same-numbered thread in the repository supplied on the command line. Empty selection flags or empty comma-separated entries could also start unintended repository-wide synchronization or enrichment.
User Impact
Qualified references must match the selected repository, case-insensitively. Invalid or empty selections fail with exit code 2 before archive or network access. Omit the selection flag for repository-wide work; supported bare numbers, relative thread paths, matching URLs, and matching qualified references still work.
Why This Change Was Made
The shared parser now validates repository identity before discarding it, and each caller supplies its selected repository. Relative-path parsing no longer accepts arbitrary URL/path prefixes. List entries must contain a thread reference, and commands distinguish omitted selection flags from explicitly empty values, including
embed --numberandsummarize --number. The same scope check protects TUI jumps.Evidence
Synthetic built-CLI proof: before this change, closing
example/archivewith a link toother/repo#10exited 0 and closed local issue 10. Afterward it exits 2 and leaves the issue unchanged; a matching mixed-case reference still closes the selected thread. Both comma-only and empty-string selections previously made repository-wide HTTP requests; afterward they make zero requests and exit 2.Regression tests cover the stored local mutation, all command entry points, TUI jumps, valid reference forms, mismatches, unsupported URL prefixes, and empty selections. Full
make checkpassed, including coverage, vet, vulnerability/dead-code checks, smoke, release-script tests, docs, and snapshot packaging. Independent autoreview is clean through P2.