fix(orb): parse the AMS PR outcome from an anchored dedupKey, not a substring scan - #9752
fix(orb): parse the AMS PR outcome from an anchored dedupKey, not a substring scan#9752shin-core wants to merge 1 commit into
Conversation
…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
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 06:19:19 UTC
Review summary Nits — 5 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk 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.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 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.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9752 +/- ##
==========================================
+ Coverage 76.58% 76.63% +0.04%
==========================================
Files 282 284 +2
Lines 59419 59529 +110
Branches 6553 6585 +32
==========================================
+ Hits 45508 45618 +110
Misses 13629 13629
Partials 282 282
Flags with carried forward coverage won't be shown. Click here to find out more.
|
JSONbored
left a comment
There was a problem hiding this comment.
Rejecting for this reason:
The anchored pattern is a real improvement, but because `.+` is greedy and repoFullName/closedAt are still attacker(-miner)-controlled free text reaching the same dedupKey string, a crafted key can still smuggle a fake trailing `#N:decision:` segment that regex backtracking will match over the true one — this narrows but does not fully close the original class of spoofing bug the PR is fixing.
What & why
Closes #9703.
ams_pr_outcomeis the only notification kind whose rendered content depends on a field that is noton the event.
buildAmsPrOutcomeNotificationrecovered the merged/closed decision withevent.dedupKey.includes(":merged:")— the dedupKey was the entire carrier. Three things made thatunsafe:
normalizeAmsNotificationEventInputonly checked non-emptystring, so any
ams_pr_outcomewhose key lacked:merged:rendered as "closed without merge",including for a real merge.
of
decision/closedAtthere 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 sharedprefix constant — the format is spelled once, not twice.
normalizeAmsNotificationEventInputreturnsnullfor anams_pr_outcomewhose dedupKey doesn'tparse (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.tsusesparseAmsPrOutcomeDecision; anullparse renders the closed copy, never anunproven merge — unreachable from ingest now the normalizer rejects such a key upstream.
No
decisionfield is added to the event/schema (out of scope).Validation
no-decision, unknown-decision, and the loose-
:merged:-elsewhere trap).ams_pr_outcomekey while other AMS kinds keep their loose check.buildAmsPrOutcomeNotificationrenders the closed copy on a null parse.buildAmsPrOutcomePayloadoutput against the serverpattern for both decisions (the drift guard).