Skip to content

fix: reject thread selections with the wrong or empty scope - #205

Merged
steipete merged 1 commit into
mainfrom
fix/thread-reference-scope
Sep 15, 2026
Merged

steipete merged 1 commit into
mainfrom
fix/thread-reference-scope

Conversation

@steipete

Copy link
Copy Markdown
Contributor

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 --number and summarize --number. The same scope check protects TUI jumps.

Evidence

Synthetic built-CLI proof: before this change, closing example/archive with a link to other/repo#10 exited 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 check passed, including coverage, vet, vulnerability/dead-code checks, smoke, release-script tests, docs, and snapshot packaging. Independent autoreview is clean through P2.

@clawsweeper

clawsweeper Bot commented Sep 15, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@steipete
steipete merged commit 0bb1341 into main Sep 15, 2026
14 checks passed
@steipete
steipete deleted the fix/thread-reference-scope branch September 15, 2026 07:29
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 15, 2026
@clawsweeper

clawsweeper Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 15, 2026, 3:28 AM ET / 07:28 UTC.

ClawSweeper review

What this changes

The 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
Reviewed head: a90e9ec90c4af0dfa58ff9831d0e995e36fe7646
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The implementation is focused and source-consistent, but the runtime proof remains an unauditable summary.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The body describes relevant built-CLI observations through local close and sync, but supplies only a narrative result; an inspectable transcript showing exit codes, unchanged archive state, request counts, and valid-input controls is still needed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The body describes relevant built-CLI observations through local close and sync, but supplies only a narrative result; an inspectable transcript showing exit codes, unchanged archive state, request counts, and valid-input controls is still needed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 9 items Repository identity and applicable policy: Origin identifies openclaw/gitcrawl. No root or applicable ancestor AGENTS.md exists; the only discovered AGENTS.md belongs to the unrelated autoreview skill subtree. No maintainer-notes directory was present.
Verified patch boundary: The checkout matches the pinned PR head. Its delta against the pinned main contains 15 files, 177 additions, and 28 deletions; the host also verified the test-merge parents and matching result. The working tree remained clean.
Main still loses repository scope: The main parser returns ref.Number without checking ref.FullName. Empty list members also remain accepted there. Reading references.go at v0.10.0 showed the same implementation, so neither inspected revision contains this fix.
Findings None None.
Security None None.

How this fits together

Gitcrawl 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]
Loading

Decision needed

Question Recommendation
Accept exit-code-2 rejection for previously accepted empty or mismatched selection inputs, requiring affected scripts to omit the flag or correct the reference? Accept the documented rejection: Keep the safety fix once runtime evidence confirms rejection without side effects and preservation of valid and omitted selections.

Why: The rejection prevents unintended work but changes the behavior of existing command invocations; accepting that operator impact is a maintainer choice.

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The body describes relevant built-CLI observations through local close and sync, but supplies only a narrative result; an inspectable transcript showing exit codes, unchanged archive state, request counts, and valid-input controls is still needed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - Existing scripts that pass empty selection variables or mismatched qualified references will stop with exit code 2 after upgrade; the intended rejection needs acceptance and compatibility proof for valid and omitted selections.
  • Complete next step (P2) - Provide the built-CLI proof transcript and obtain acceptance of the documented rejection behavior. Terminal screenshots or recordings are welcome when they show the observations; copied output and logs also count. Redact credentials, IP addresses, private endpoints, and other private data. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test LOC production +19 net; tests +121 net The production growth adds shared validation and caller guards, with regression coverage for the affected entrypoints.

Merge-risk options

Maintainer options:

  1. Accept rejection with compatibility evidence (recommended)
    Approve the documented input restriction after a built-CLI transcript confirms valid and omitted selections still work on an existing archive.
  2. Pause for affected integrations
    Defer landing if maintainers identify supported scripts that need migration before empty inputs become errors.

Technical review

Best 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.

Labels

Label changes:

  • add P2: This fixes bounded selection errors that can target the wrong local thread or unintentionally expand synchronization and enrichment.
  • add merge-risk: 🚨 compatibility: Previously accepted empty or mismatched selections become usage errors, potentially stopping existing scripts.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body describes relevant built-CLI observations through local close and sync, but supplies only a narrative result; an inspectable transcript showing exit codes, unchanged archive state, request counts, and valid-input controls is still needed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This fixes bounded selection errors that can target the wrong local thread or unintentionally expand synchronization and enrichment.
  • merge-risk: 🚨 compatibility: Previously accepted empty or mismatched selections become usage errors, potentially stopping existing scripts.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body describes relevant built-CLI observations through local close and sync, but supplies only a narrative result; an inspectable transcript showing exit codes, unchanged archive state, request counts, and valid-input controls is still needed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Repository identity and applicable policy: Origin identifies openclaw/gitcrawl. No root or applicable ancestor AGENTS.md exists; the only discovered AGENTS.md belongs to the unrelated autoreview skill subtree. No maintainer-notes directory was present. (a90e9ec90c4a)
  • Verified patch boundary: The checkout matches the pinned PR head. Its delta against the pinned main contains 15 files, 177 additions, and 28 deletions; the host also verified the test-merge parents and matching result. The working tree remained clean. (a90e9ec90c4a)
  • Main still loses repository scope: The main parser returns ref.Number without checking ref.FullName. Empty list members also remain accepted there. Reading references.go at v0.10.0 showed the same implementation, so neither inspected revision contains this fix. (internal/cli/references.go:109, a6ca802a8f67)
  • Validation precedes side effects: The shared parser checks repository identity before returning a number; list parsing requires every member. All ten command callers validate before opening runtime or invoking synchronization/enrichment, and terminal-browser jumps validate before navigation. (internal/cli/references.go:117, a90e9ec90c4a)
  • Empty selections can expand synchronization: The sync service removes nonpositive numbers, then selects repository listing when no numbers remain. The new CLI guards prevent empty strings and comma-only lists from reaching that branch. (internal/syncer/syncer.go:168, a90e9ec90c4a)
  • Proof claim inspected: The complete supplied body, unchanged in the live read, reports a synthetic built-CLI before/after run: wrong-repository local close changes from success to exit 2 without mutation, matching mixed-case input succeeds, and empty selections change from HTTP requests to zero requests. It contains no command/output transcript or linked artifact showing those observations. Both discussion comments are bot status messages. Captured context sourceRevision: c6729e5ebb65f950786b6e5c087a5fd4778e9fff63ab7de1bd311ac434ac65cb. (a90e9ec90c4a)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Attach the reported built-CLI run as redacted terminal output or a linked transcript, including rejected inputs, archive state, request counts, and matching-reference controls.
  • Include fresh and existing archive results showing omitted selection flags retain repository-wide behavior, and obtain acceptance of the intentional script compatibility change.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant