Skip to content

fix(review): hash a JSON payload to prevent linked-issue-satisfaction cache-key collisions - #5976

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
jeffrey701:fix/5939-fingerprint-delimiter-collision
Jul 15, 2026
Merged

fix(review): hash a JSON payload to prevent linked-issue-satisfaction cache-key collisions#5976
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
jeffrey701:fix/5939-fingerprint-delimiter-collision

Conversation

@jeffrey701

Copy link
Copy Markdown
Contributor

Problem

linkedIssueSatisfactionCacheInputFingerprint (src/review/linked-issue-satisfaction-cache-input.ts) builds the payload it hashes into a linked-issue-satisfaction cache key by "|"-joining free-form GitHub text fields (issueText, prTitle, prBody, diff):

const payload = [
  LINKED_ISSUE_SATISFACTION_CACHE_INPUT_VERSION,
  input.byok ? "1" : "0",
  input.provider ?? "", input.model ?? "",
  input.issueText ?? "", input.prTitle ?? "", input.prBody ?? "", input.diff ?? "",
].join("|");

None of that text is escaped, so a | occurring naturally inside one field can shift the field boundary and produce the same joined string for two genuinely different inputs — e.g. {issueText: "foo|bar", prTitle: "baz"} and {issueText: "foo", prTitle: "bar|baz"} (other fields equal) serialize identically and hash to the same fingerprint/cache key, so one PR's linked-issue-satisfaction verdict can be replayed for a different PR.

Its own comment says it "mirrors ai-slop-cache-input.ts's fingerprint discipline" — but that sibling actually builds its payload via JSON.stringify({...}), which structurally escapes delimiters and can't collide this way.

Fix

Build the payload as a JSON.stringify'd object, matching the sibling aiSlopCacheInputFingerprint. JSON escapes the field values ("foo|bar" vs "foo","bar|baz" serialize distinctly), so distinct inputs always produce distinct fingerprints. Field values are unchanged; only the serialization is structural now.

Test

test/unit/linked-issue-satisfaction-cache.test.ts adds a case asserting the two previously-colliding inputs (issueText: "foo|bar" / prTitle: "baz" vs issueText: "foo" / prTitle: "bar|baz") now yield different fingerprints. The existing BYOK-toggle and cache round-trip tests are unchanged and still pass.

Local: the fingerprint verification (distinct inputs differ; identical inputs stay deterministic) passes; tsc --noEmit clean on the changed file.

Closes #5939

…Fingerprint

The fingerprint "|"-joined free-form GitHub text fields (issueText,
prTitle, prBody, diff), so an unescaped "|" inside one field could shift a
boundary and make two genuinely different inputs serialize identically and
collide on the same cache key. Build the payload with JSON.stringify,
matching the sibling aiSlopCacheInputFingerprint, so distinct inputs always
produce distinct fingerprints.

Closes JSONbored#5939
@jeffrey701
jeffrey701 requested a review from JSONbored as a code owner July 15, 2026 05:20
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.16%. Comparing base (4749ff7) to head (8f32c51).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5976      +/-   ##
==========================================
- Coverage   95.16%   95.16%   -0.01%     
==========================================
  Files         589      589              
  Lines       46790    46789       -1     
  Branches    14943    14942       -1     
==========================================
- Hits        44528    44527       -1     
  Misses       1511     1511              
  Partials      751      751              
Flag Coverage Δ
shard-1 43.50% <50.00%> (-0.42%) ⬇️
shard-2 36.36% <100.00%> (+0.15%) ⬆️
shard-3 31.98% <0.00%> (+<0.01%) ⬆️
shard-4 31.95% <0.00%> (+<0.01%) ⬆️
shard-5 32.26% <100.00%> (-0.24%) ⬇️
shard-6 44.89% <0.00%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...rc/review/linked-issue-satisfaction-cache-input.ts 100.00% <100.00%> (ø)

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 15, 2026
@loopover-orb

loopover-orb Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-15 05:27:27 UTC

2 files · 1 AI reviewer · no blockers · readiness 82/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This replaces the delimiter-vulnerable `"|"`-join in `linkedIssueSatisfactionCacheInputFingerprint` with `JSON.stringify`, matching the sibling `aiSlopCacheInputFingerprint` pattern the code already claimed to mirror. Field values are unchanged, only the serialization changed, and since object literal key order is deterministic across calls, the JSON string is stable for identical inputs. The added test directly reproduces the described collision (`issueText: "foo|bar"` + `prTitle: "baz"` vs `issueText: "foo"` + `prTitle: "bar|baz"`) and asserts distinct fingerprints, which is the right regression test for this exact bug class.

Nits — 3 non-blocking
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #5939
Related work ⚠️ 2 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 131 registered-repo PR(s), 63 merged, 33 issue(s).
Contributor context ✅ Confirmed Gittensor contributor jeffrey701; Gittensor profile; 131 PR(s), 33 issue(s).
Gate result ✅ Passing No configured blocker found.
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Partially addressed
The PR correctly switches to a JSON.stringify-based structured payload and adds a regression test proving the previously-colliding '|' inputs now yield distinct fingerprints, satisfying the core collision fix. However, the diff shows no change to LINKED_ISSUE_SATISFACTION_CACHE_INPUT_VERSION, which the issue explicitly requires bumping so existing cache rows keyed under the old collision-prone for

Review context
Contributor next steps
  • Start here: Review top overlaps.
  • Then work through the remaining 2 steps in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://gittensory.aethereal.dev/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 68fac24 into JSONbored:main Jul 15, 2026
15 checks passed
@nghetienhiep nghetienhiep mentioned this pull request Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(review): linkedIssueSatisfactionCacheInputFingerprint's delimiter-joined payload allows cache-key collisions across different prompts

1 participant