Skip to content

fix(orb): parse the AMS PR outcome from an anchored dedupKey, not a substring scan - #9754

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
shin-core:fix/ams-outcome-dedupkey-anchored-9703-v2
Jul 29, 2026
Merged

fix(orb): parse the AMS PR outcome from an anchored dedupKey, not a substring scan#9754
JSONbored merged 1 commit into
JSONbored:mainfrom
shin-core:fix/ams-outcome-dedupkey-anchored-9703-v2

Conversation

@shin-core

Copy link
Copy Markdown
Contributor

What & why

Closes #9703.

ams_pr_outcome is the only notification kind whose rendered content depends on a field that is not
on the event. buildAmsPrOutcomeNotification recovered the merged/closed decision with
event.dedupKey.includes(":merged:") — the dedupKey was the entire carrier. Three things made that
unsafe:

  1. Nothing validated the key's shapenormalizeAmsNotificationEventInput only checked non-empty
    string, so any ams_pr_outcome whose key lacked :merged: rendered as "closed without merge",
    including for a real merge.
  2. Two hand-duplicated producers wrote the format literal with nothing pinning them together.
  3. The server-side builder has no non-test callers — the miner's copy is what runs, so a reorder
    of decision/closedAt there would silently flip every merge notification and nothing would fail.

Change

  • ams-events.ts: AMS_PR_OUTCOME_DEDUP_KEY_PATTERN (anchored, decision as capture group) +
    parseAmsPrOutcomeDecision (the single reader). The builder and the pattern derive from one shared
    prefix constant — the format is spelled once, not twice.
  • normalizeAmsNotificationEventInput returns null for an ams_pr_outcome whose dedupKey doesn't
    parse (the MCP/self-host path reaches the normalizer directly, so validating only the HTTP zod schema
    isn't enough); every other AMS kind's validation is unchanged.
  • service.ts uses parseAmsPrOutcomeDecision; a null parse renders the closed copy, never an
    unproven merge — unreachable from ingest now the normalizer rejects such a key upstream.

No decision field is added to the event/schema (out of scope).

Validation

  • Pattern/parser tests (well-formed round-trips for both decisions; null for wrong-prefix,
    no-decision, unknown-decision, and the loose-:merged:-elsewhere trap).
  • Normalizer rejects a malformed ams_pr_outcome key while other AMS kinds keep their loose check.
  • buildAmsPrOutcomeNotification renders the closed copy on a null parse.
  • A producer-parity test pins the miner's buildAmsPrOutcomePayload output against the server
    pattern for both decisions (the drift guard).
  • Bug-catch verified: reverting the normalizer/service fails the new assertions.
  • 100% patch coverage on both source files.

…ubstring scan

ams_pr_outcome is the only notification kind whose rendered merged/closed copy
depends on a field that is not on the event: buildAmsPrOutcomeNotification recovered
the decision with event.dedupKey.includes(":merged:"). The dedupKey was the entire
carrier, yet nothing validated its shape -- normalizeAmsNotificationEventInput only
checked the key was a non-empty string -- so any ams_pr_outcome whose key lacked
":merged:" rendered as "closed without merge", including for a real merge. The
format also had two hand-duplicated producers with nothing pinning them together,
and the server-side builder has no non-test callers, so a reorder in the miner's
copy would silently flip every merge notification with nothing failing.

- ams-events.ts: add AMS_PR_OUTCOME_DEDUP_KEY_PATTERN (anchored, decision as a
  capture group) and parseAmsPrOutcomeDecision, the single reader. The builder and
  the pattern derive from one shared prefix constant -- the format is spelled once.
- normalizeAmsNotificationEventInput returns null for an ams_pr_outcome whose
  dedupKey does not parse (the MCP/self-host path reaches the normalizer directly);
  every other AMS kind's validation is unchanged.
- service.ts uses parseAmsPrOutcomeDecision; a null parse renders the CLOSED copy,
  never an unproven merge (unreachable from ingest now the normalizer rejects it).

Adds a producer-parity test pinning the miner's buildAmsPrOutcomePayload output
against the server pattern for both decisions, mirroring the source-pinning drift
guard convention.

Closes JSONbored#9703
@shin-core
shin-core requested a review from JSONbored as a code owner July 29, 2026 06:24
@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 29, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Important

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏳ LoopOver is waiting…

LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs.

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

@superagent-security

Copy link
Copy Markdown
Contributor

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

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

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.63%. Comparing base (5ecb771) to head (198cdb8).

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9754       +/-   ##
===========================================
- Coverage   90.27%   76.63%   -13.65%     
===========================================
  Files         904      284      -620     
  Lines      113177    59529    -53648     
  Branches    26840     8505    -18335     
===========================================
- Hits       102171    45618    -56553     
- Misses       9676    13629     +3953     
+ Partials     1330      282     -1048     
Flag Coverage Δ
backend 100.00% <100.00%> (+4.45%) ⬆️

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

Files with missing lines Coverage Δ
src/notifications/ams-events.ts 100.00% <100.00%> (ø)
src/notifications/service.ts 100.00% <100.00%> (ø)

... and 754 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 29, 2026
@JSONbored
JSONbored merged commit 0da1f4f into JSONbored:main Jul 29, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:flagged Contributor flagged for review by trust analysis. gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

orb(notifications): stop deriving the AMS PR outcome by substring-scanning a client dedupKey

2 participants