[codex] x1 badge sdk lifecycle - #52
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kyalabs
marked this pull request as ready for review
April 16, 2026 01:50
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
packages/sdk/src/index.test.ts (1)
2-6: Consider assertingBADGE_VERSIONexport as part of the public surface.This test already guards helper exports; adding
BADGE_VERSIONhere would better lock the intended SDK contract described in the PR objective.Suggested test extension
import { + BADGE_VERSION, inferContextFromUrl, postDeclareVisit, postReportOutcome, } from "./index.js"; @@ it("exports the X1 lifecycle helpers", () => { + expect(typeof BADGE_VERSION).toBe("string"); expect(typeof inferContextFromUrl).toBe("function"); expect(typeof postDeclareVisit).toBe("function"); expect(typeof postReportOutcome).toBe("function"); });Also applies to: 9-13
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/src/index.test.ts` around lines 2 - 6, The test currently asserts the helper exports (inferContextFromUrl, postDeclareVisit, postReportOutcome) but omits BADGE_VERSION; update packages/sdk/src/index.test.ts to also import or reference BADGE_VERSION from "./index.js" and add an assertion that it is part of the public surface (for example assert it is defined and is a string or matches a semantic version pattern). Keep the existing helper export checks (inferContextFromUrl, postDeclareVisit, postReportOutcome) and add a single concise assertion validating BADGE_VERSION to lock the intended SDK contract.packages/sdk/src/context-inference.test.ts (1)
4-20: Add regression cases for segment-boundary matching.Given the inference logic, add cases like
/cartoon(should remainarrival) and mixed routes (e.g.,/checkout/cart) to prevent future misclassification regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/src/context-inference.test.ts` around lines 4 - 20, Add regression tests to inferContextFromUrl to cover segment-boundary cases: add a test asserting inferContextFromUrl("https://shop.test/cartoon") returns "arrival" to ensure "cart" is not matched inside another segment, and add a test asserting inferContextFromUrl("https://shop.test/checkout/cart") returns "checkout" to verify the function matches the correct leading segment in mixed routes; place these alongside the existing tests for inferContextFromUrl.packages/sdk/examples/demo.mjs (1)
16-16: Prefer cached token first to better demonstrate durable reuse.Line 16 currently attempts enrollment before cache lookup. For a lifecycle demo centered on restart durability, cache-first ordering is clearer and avoids unnecessary enrollment attempts.
Proposed refactor
async function enrollMerchant(installId, merchant) { - const token = await enrollAndCacheBadgeToken(merchant) ?? getCachedBadgeToken(merchant); + const cached = getCachedBadgeToken(merchant); + const token = cached ?? await enrollAndCacheBadgeToken(merchant); if (!token) { throw new Error(`Could not enroll or recover badge token for ${merchant}`); } return Badge.init({ installId, existingToken: token }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/examples/demo.mjs` at line 16, The current token acquisition calls enrollAndCacheBadgeToken(merchant) before checking cache; flip the order so you attempt getCachedBadgeToken(merchant) first and only call enrollAndCacheBadgeToken(merchant) if the cache returns null/undefined. Update the expression that assigns token (the variable token) to use getCachedBadgeToken(merchant) ?? enrollAndCacheBadgeToken(merchant) so cached tokens are preferred and enrollment is only performed when cache miss occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@badge-server`:
- Line 1: Remove the user-specific symlink file named "badge-server" from
version control and stop committing absolute local paths; run git rm on the
badge-server entry (so it is removed from the repo index), add "badge-server" to
.gitignore to prevent future accidental commits, and replace the local-symlink
approach with a proper dependency/workspace setup (e.g., add the external
https://github.com/kyalabs-io/badge-server as a package dependency or workspace)
so no user-specific absolute paths remain in the repository.
In `@packages/sdk/examples/demo.mjs`:
- Line 1: Add the Node environment annotation at the top of the example (/*
eslint-env node */) to satisfy no-undef for process and replace console.log
calls with an allowed console method (e.g., console.info) throughout the file
(including the occurrences around lines 44 and 56-57) so lint
no-console/no-undef errors are resolved; update every console.log in the module
to console.info and keep uses of process as-is after adding the env comment.
In `@packages/sdk/examples/reset.mjs`:
- Line 15: Replace the lint-blocking console.log call with a non-console output
(e.g., use process.stdout.write) so ESLint no-console/no-undef is satisfied:
change the statement using the variable target (currently console.log(`removed
${target}`)) to process.stdout.write(`removed ${target}\n`) or route the message
through your project's logger utility if one exists (use the same target
variable and ensure a trailing newline).
In `@packages/sdk/README.md`:
- Around line 112-140: Update the README examples for badge.declareVisit and
badge.reportOutcome to document the additional optional parameters exported by
the SDK: declareVisit accepts source?: BadgeEventSource (refer to
packages/sdk/src/badge.ts for the BadgeEventSource type) and reportOutcome
accepts detail?: string; add these optional fields to the example argument
objects and a short sentence describing their purpose and expected values so the
README matches the actual SDK surface.
In `@packages/sdk/src/badge-token.ts`:
- Around line 85-99: persistBadgeToken currently writes expiresAt to disk but
sets badgeTokenCache with only the raw token string, so in-memory lookups (fast
paths) can return stale tokens; change persistBadgeToken to store the same shape
in badgeTokenCache as on-disk (e.g., { token, expiresAt? }) using the cacheKey
result, and ensure any consumers that read badgeTokenCache (the fast-path
lookups used by loadPersistedBadgeToken and the call sites around lines 112/179)
handle the object shape and enforce expiry the same way loadPersistedBadgeToken
does.
- Around line 175-188: getCachedBadgeToken currently only keeps
lastEnrolledMerchant in memory; persist and restore it so no-arg calls work
across restarts. When you set lastEnrolledMerchant inside
getCachedBadgeToken(merchant) (after badgeTokenCache.get or
loadPersistedBadgeToken returns a token), also persist that merchant
(implement/push a helper like persistLastEnrolledMerchant or extend existing
persistence logic). In the no-arg branch of getCachedBadgeToken(), if
lastEnrolledMerchant is unset, attempt to restore it from persistence (e.g.,
loadPersistedLastEnrolledMerchant or reading the same persistence used for badge
tokens), assign it to lastEnrolledMerchant and then call
getCachedBadgeToken(lastEnrolledMerchant) to return the token. Update or add the
small persistence helpers to store/retrieve the last enrolled merchant alongside
the badge token storage.
In `@packages/sdk/src/badge.ts`:
- Around line 132-149: The code currently sets source = args.source ?? "sdk"
which mislabels visits where context is inferred from args.url; change the
source calculation so: if args.source is provided use it, else if args.context
is provided use "sdk", else if args.url is present use "inferred". Update the
variable used in the offline return branch and the postDeclareVisit payload
(referencing runId, source, this.identityType, inferContextFromUrl,
postDeclareVisit, BadgeEventSource, DeclareResult) so inferred visits are
emitted with source: "inferred".
In `@packages/sdk/src/context-inference.ts`:
- Around line 5-7: The current logic uses substring checks on pathname (const
pathname = new URL(url).pathname) which causes false positives like "/cartoon";
update the two checks that use pathname.includes("/cart") and
pathname.includes("/checkout") to perform path-segment matching instead (e.g.,
split pathname into segments or match segment boundaries) so you only return
"addtocart" when a segment equals "cart" and "checkout" when a segment equals
"checkout"; keep the same return values ("addtocart", "checkout") and update the
checks around the pathname variable.
In `@packages/sdk/src/declare-visit.ts`:
- Around line 28-31: The postDeclareVisit function currently accepts an empty
runId and can send an invalid trip_id; add a runtime guard at the start of
postDeclareVisit that validates args.runId (non-empty string) and throws or
returns a clear error immediately if invalid (mirror the validation behavior
used in postReportOutcome), and likewise add the same validation where
postDeclareVisit constructs/sends the request (see the subsequent block around
the code referenced at lines 41-45) so callers get fast, consistent feedback
instead of sending an invalid trip_id.
In `@packages/sdk/src/telemetry.ts`:
- Line 9: Add a clear doc comment in version.ts above the exported BADGE_VERSION
(and where badge_version is referenced) that defines its semantic meaning (e.g.,
"badge_version is the badge/report protocol/event-schema version used when
calling /api/badge/report, not the SDK package version"), state expected format
(semantic versioning or major.minor), and note that MCP tools intentionally use
different values (e.g., 2.4/2.5.0) to indicate different protocol/schema
versions; ensure the comment references the BADGE_VERSION symbol and the
/api/badge/report contract so consumers (and tests) understand it must remain
consistent with backend analytics expectations.
---
Nitpick comments:
In `@packages/sdk/examples/demo.mjs`:
- Line 16: The current token acquisition calls
enrollAndCacheBadgeToken(merchant) before checking cache; flip the order so you
attempt getCachedBadgeToken(merchant) first and only call
enrollAndCacheBadgeToken(merchant) if the cache returns null/undefined. Update
the expression that assigns token (the variable token) to use
getCachedBadgeToken(merchant) ?? enrollAndCacheBadgeToken(merchant) so cached
tokens are preferred and enrollment is only performed when cache miss occurs.
In `@packages/sdk/src/context-inference.test.ts`:
- Around line 4-20: Add regression tests to inferContextFromUrl to cover
segment-boundary cases: add a test asserting
inferContextFromUrl("https://shop.test/cartoon") returns "arrival" to ensure
"cart" is not matched inside another segment, and add a test asserting
inferContextFromUrl("https://shop.test/checkout/cart") returns "checkout" to
verify the function matches the correct leading segment in mixed routes; place
these alongside the existing tests for inferContextFromUrl.
In `@packages/sdk/src/index.test.ts`:
- Around line 2-6: The test currently asserts the helper exports
(inferContextFromUrl, postDeclareVisit, postReportOutcome) but omits
BADGE_VERSION; update packages/sdk/src/index.test.ts to also import or reference
BADGE_VERSION from "./index.js" and add an assertion that it is part of the
public surface (for example assert it is defined and is a string or matches a
semantic version pattern). Keep the existing helper export checks
(inferContextFromUrl, postDeclareVisit, postReportOutcome) and add a single
concise assertion validating BADGE_VERSION to lock the intended SDK contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bc9857ec-7032-41ff-af92-34df7f364798
📒 Files selected for processing (21)
badge-serverpackages/sdk/README.mdpackages/sdk/examples/demo.mjspackages/sdk/examples/reset.mjspackages/sdk/src/badge-token.test.tspackages/sdk/src/badge-token.tspackages/sdk/src/badge.test.tspackages/sdk/src/badge.tspackages/sdk/src/context-inference.test.tspackages/sdk/src/context-inference.tspackages/sdk/src/declare-visit.test.tspackages/sdk/src/declare-visit.tspackages/sdk/src/guest-pass.tspackages/sdk/src/index.test.tspackages/sdk/src/index.tspackages/sdk/src/report-outcome.test.tspackages/sdk/src/report-outcome.tspackages/sdk/src/telemetry.test.tspackages/sdk/src/telemetry.tspackages/sdk/src/types.tspackages/sdk/src/version.ts
Comment on lines
+112
to
+140
| ### `badge.declareVisit(args)` | ||
|
|
||
| Declare an agent visit at a merchant and write a `declared` badge event. | ||
|
|
||
| ```typescript | ||
| const runId = badge.startRun() | ||
|
|
||
| await badge.declareVisit({ | ||
| merchant: 'merchant.test', | ||
| runId, | ||
| url: 'https://merchant.test/cart', | ||
| // or context: 'arrival' | 'addtocart' | 'checkout' | ||
| }) | ||
| ``` | ||
|
|
||
| If `context` is omitted, the SDK infers it from the URL (`/cart` → `addtocart`, `/checkout` → `checkout`, otherwise `arrival`). | ||
|
|
||
| ### `badge.reportOutcome(args)` | ||
|
|
||
| Report the agent-observed outcome for a previously declared run. This writes `sampling_complete` through the existing anonymous report path so kyaScore recompute still triggers for the install ID. | ||
|
|
||
| ```typescript | ||
| await badge.reportOutcome({ | ||
| merchant: 'merchant.test', | ||
| runId, | ||
| outcome: 'not_denied', // or 'denied' | 'unparseable' | ||
| frictionReason: 'merchant_rejection', | ||
| }) | ||
| ``` |
There was a problem hiding this comment.
Document the shipped source and detail options.
badge.declareVisit() in packages/sdk/src/badge.ts also accepts source?: BadgeEventSource, and badge.reportOutcome() accepts detail?: string. The README currently documents a narrower public surface than the SDK actually exports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk/README.md` around lines 112 - 140, Update the README examples
for badge.declareVisit and badge.reportOutcome to document the additional
optional parameters exported by the SDK: declareVisit accepts source?:
BadgeEventSource (refer to packages/sdk/src/badge.ts for the BadgeEventSource
type) and reportOutcome accepts detail?: string; add these optional fields to
the example argument objects and a short sentence describing their purpose and
expected values so the README matches the actual SDK surface.
Comment on lines
175
to
188
| export function getCachedBadgeToken(merchant?: string): string | null { | ||
| if (merchant) { | ||
| const token = badgeTokenCache.get(merchant) ?? null; | ||
| const installId = getOrCreateInstallId(); | ||
| const normalizedMerchant = normalizeMerchant(merchant); | ||
| const token = badgeTokenCache.get(cacheKey(normalizedMerchant, installId)) | ||
| ?? loadPersistedBadgeToken(normalizedMerchant, installId); | ||
| // Keep lastEnrolledMerchant in sync so no-arg getHeaders() uses the right merchant | ||
| if (token) lastEnrolledMerchant = merchant; | ||
| if (token) lastEnrolledMerchant = normalizedMerchant; | ||
| return token; | ||
| } | ||
| // No merchant — return last enrolled | ||
| if (lastEnrolledMerchant) { | ||
| return badgeTokenCache.get(lastEnrolledMerchant) ?? null; | ||
| return getCachedBadgeToken(lastEnrolledMerchant); | ||
| } |
There was a problem hiding this comment.
Persist the “last enrolled” merchant too.
getCachedBadgeToken() without a merchant only works while lastEnrolledMerchant is still in memory. After a restart, persisted entries are readable, but this path still falls back to null because nothing reconstructs the last merchant from badge_tokens.json. That breaks the documented “most recently enrolled token” behavior across restarts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk/src/badge-token.ts` around lines 175 - 188, getCachedBadgeToken
currently only keeps lastEnrolledMerchant in memory; persist and restore it so
no-arg calls work across restarts. When you set lastEnrolledMerchant inside
getCachedBadgeToken(merchant) (after badgeTokenCache.get or
loadPersistedBadgeToken returns a token), also persist that merchant
(implement/push a helper like persistLastEnrolledMerchant or extend existing
persistence logic). In the no-arg branch of getCachedBadgeToken(), if
lastEnrolledMerchant is unset, attempt to restore it from persistence (e.g.,
loadPersistedLastEnrolledMerchant or reading the same persistence used for badge
tokens), assign it to lastEnrolledMerchant and then call
getCachedBadgeToken(lastEnrolledMerchant) to return the token. Update or add the
small persistence helpers to store/retrieve the last enrolled merchant alongside
the badge token storage.
Comment on lines
+132
to
+149
| const runId = args.runId ?? this.startRun(); | ||
| const source = args.source ?? "sdk"; | ||
| if (this.identityType === "offline") { | ||
| return { | ||
| recordedAs: "offline", | ||
| source, | ||
| merchant: args.merchant, | ||
| runId, | ||
| }; | ||
| } | ||
|
|
||
| const context = args.context ?? (args.url ? inferContextFromUrl(args.url) : undefined); | ||
| return postDeclareVisit(this.token, { | ||
| merchant: args.merchant, | ||
| runId, | ||
| ...(args.url ? { url: args.url } : {}), | ||
| ...(context ? { context } : {}), | ||
| source, |
There was a problem hiding this comment.
Mark inferred visits as source: "inferred".
When context is derived from inferContextFromUrl(), the default source still stays "sdk". That mislabels inferred visits even though BadgeEventSource explicitly includes "inferred", and DeclareResult.source will be wrong too.
Proposed fix
}): Promise<DeclareResult> {
const runId = args.runId ?? this.startRun();
- const source = args.source ?? "sdk";
+ const inferredContext =
+ args.context === undefined && args.url ? inferContextFromUrl(args.url) : undefined;
+ const source = args.source ?? (inferredContext ? "inferred" : "sdk");
if (this.identityType === "offline") {
return {
recordedAs: "offline",
source,
merchant: args.merchant,
runId,
};
}
- const context = args.context ?? (args.url ? inferContextFromUrl(args.url) : undefined);
+ const context = args.context ?? inferredContext;
return postDeclareVisit(this.token, {
merchant: args.merchant,
runId,
...(args.url ? { url: args.url } : {}),
...(context ? { context } : {}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const runId = args.runId ?? this.startRun(); | |
| const source = args.source ?? "sdk"; | |
| if (this.identityType === "offline") { | |
| return { | |
| recordedAs: "offline", | |
| source, | |
| merchant: args.merchant, | |
| runId, | |
| }; | |
| } | |
| const context = args.context ?? (args.url ? inferContextFromUrl(args.url) : undefined); | |
| return postDeclareVisit(this.token, { | |
| merchant: args.merchant, | |
| runId, | |
| ...(args.url ? { url: args.url } : {}), | |
| ...(context ? { context } : {}), | |
| source, | |
| const runId = args.runId ?? this.startRun(); | |
| const inferredContext = | |
| args.context === undefined && args.url ? inferContextFromUrl(args.url) : undefined; | |
| const source = args.source ?? (inferredContext ? "inferred" : "sdk"); | |
| if (this.identityType === "offline") { | |
| return { | |
| recordedAs: "offline", | |
| source, | |
| merchant: args.merchant, | |
| runId, | |
| }; | |
| } | |
| const context = args.context ?? inferredContext; | |
| return postDeclareVisit(this.token, { | |
| merchant: args.merchant, | |
| runId, | |
| ...(args.url ? { url: args.url } : {}), | |
| ...(context ? { context } : {}), | |
| source, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk/src/badge.ts` around lines 132 - 149, The code currently sets
source = args.source ?? "sdk" which mislabels visits where context is inferred
from args.url; change the source calculation so: if args.source is provided use
it, else if args.context is provided use "sdk", else if args.url is present use
"inferred". Update the variable used in the offline return branch and the
postDeclareVisit payload (referencing runId, source, this.identityType,
inferContextFromUrl, postDeclareVisit, BadgeEventSource, DeclareResult) so
inferred visits are emitted with source: "inferred".
- Remove committed badge-server symlink (local path in public repo) - Restore BADGE_VERSION to "2.4" (protocol version, not package version) - Fix inferContextFromUrl false positives (/cartoon matched as addtocart) - Add runId validation to postDeclareVisit for parity with reportOutcome - Fix demo.mjs to check cache before network enrollment - Store expiry in badge token memory cache to prevent stale token use - Document optional params (source, detail) in README - Clarify unused _token param in postReportOutcome Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace console.log/error with process.stdout/stderr.write in examples - Add eslint-env node annotation to demo.mjs - Export BADGE_VERSION from index.ts and assert in export test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
declareVisit,reportOutcome, andstartRuntoBadgecontext-inference,declare-visit,report-outcome, and sharedversion~/.kya/badge_tokens.jsonexamples/demo.mjsplusexamples/reset.mjsWhy
X1 needs a minimal, shippable SDK lifecycle that can declare visits, report outcomes, and survive reenrollment without extra infrastructure.
Impact
Validation
cd badge-server/packages/sdk && npm test -- --runcd badge-server/packages/sdk && npm run buildcd badge-server/packages/sdk && node --check examples/demo.mjscd badge-server/packages/sdk && node --check examples/reset.mjsRoot cause
The SDK was missing the X1 lifecycle entry points and durable token handling, so the spec could not be satisfied without either manual orchestration or repeated reenrollment.
Summary by CodeRabbit
Release Notes
New Features
Documentation