docs: bot-platform nextgen migration — auth spec, traffic isolation, migration runbook#334
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete bot-platform-nextgen spec suite under ChangesBot Platform NextGen Authentication & Traffic Isolation Specs
Specification Navigation and Plugin Setup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (8)
docs/specs/bot-platform-nextgen-auth.md (1)
350-355: ⚡ Quick winClarify the cache invalidation strategy for revocation (pub/sub vs. TTL).
Line 350 states revocation uses "pub/sub invalidation or short TTL" but doesn't commit to either strategy. This is ambiguous for implementation:
- Pub/sub invalidation (preferred): revoke takes effect immediately; requires inter-pod messaging.
- Short TTL only (fallback): revoke is eventual (up to 5 min per §16
SESSION_CACHE_TTL); simpler but slower.Part 2 §5.5 mentions revocation as "the immediate kill switch," implying immediate effect, but the cache section leaves it open.
Recommendation: explicitly state which strategy is chosen, and document the revocation latency SLA (e.g., "revoke must invalidate the cache within 10 seconds" or "revoke invalidation is eventual, TTL-bound at 5 minutes").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-auth.md` around lines 350 - 355, The revocation cache invalidation strategy is ambiguous in the specification, stating "pub/sub invalidation or short TTL" without committing to either approach. This creates implementation uncertainty about revocation latency and immediate vs. eventual consistency. Explicitly choose and document one strategy: either pub/sub invalidation for immediate revocation across all pods (as suggested by the "immediate kill switch" language in §5.5) or short TTL-based eventual invalidation bounded by SESSION_CACHE_TTL (§16). Additionally, clearly define the revocation latency SLA for the chosen strategy, specifying either the maximum time to propagate revocation (e.g., "10 seconds" for pub/sub) or the TTL bound (e.g., "5 minutes" for TTL-only), so implementers understand the exact consistency guarantees.docs/specs/bot-platform-nextgen-migration-runbook.md (1)
378-392: ⚖️ Poor tradeoffMigration runbook has critical external dependencies — ensure they are resolved before execution.
The end-to-end rollout (§5) includes Step 4 ("Activate live users-sync") which depends on an external infra team to extend the legacy→nextgen users-sync to carry the bcrypt hash into
credentials. This is marked as TBD (§7, last item).If the sync cannot be extended, Part 2 §6.3 specifies a fallback: "A literal read-only 'freeze' is the trivial fallback if the sync can't carry credentials." But a freeze complicates the rollout (no password changes during migration window) and increases operator toil.
Recommendation:
- Before starting the runbook, confirm with the infra team that the users-sync can be extended to carry
services.password.bcryptto thecredentialscollection, or commit to a freeze window and document its duration.- Update the runbook (§5 Step 4 pre-check) to require written confirmation of the sync capability.
- Add a contingency note: if the sync cannot be extended, escalate to the team responsible for Step 4 and activate the freeze fallback (reference Part 2 §6.3).
Similarly, §7 lists 5 open questions; the following are blockers and must be resolved before Step 1:
users.{account, siteId}compound index — confirm PR#295added it (§7, line 429).- Per-site DB topology — clarify if each nextgen site has its own
chatDB (affects migration job parameterization, §7, line 431).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-migration-runbook.md` around lines 378 - 392, Update the migration runbook to address critical external dependencies before execution. In Section 5, Step 4 ("Activate live users-sync"), add a pre-check requiring written confirmation from the infra team that the legacy→nextgen users-sync can be extended to carry the bcrypt hash to the credentials collection, or document a committed freeze window if the sync cannot be extended. Add a contingency note in Step 4 that references the freeze fallback from Part 2 §6.3 as an escalation path if the sync extension is not achievable. Additionally, in Section 7, mark as blockers the two critical open questions that must be resolved before Step 1 of the migration job: confirmation that the users.{account, siteId} compound index exists (PR `#295`) and clarification of the per-site DB topology (whether each nextgen site has its own chat DB, which affects migration job parameterization).docs/specs/bot-traffic-isolation-part1-requirements.md (1)
38-38: 💤 Low valueSimplify wordy phrasing in the Option B rationale.
Line 38: "in proportion to its share" is verbose. Consider a more direct phrase like "still adds proportional load" or "bot burst still saturates the human Deployment proportionally."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-traffic-isolation-part1-requirements.md` at line 38, In the Option B row of the requirements table, the phrase "in proportion to its share" in the rationale column is overly verbose. Replace this phrase with a more concise and direct alternative such as "still adds proportional load" or "bot burst still saturates the human Deployment proportionally" to improve clarity and readability while maintaining the same meaning.docs/specs/bot-traffic-isolation-part2-design.md (5)
174-189: ⚡ Quick winMake the Phase 1 scope of the subject namespace more prominent.
Lines 181–189 explain that the mirror table's rows 1–2 (user-scoped RPC subjects) are deferred to Phase 5+, and row 3 (canonical) ships in Phase 1. However, this crucial staging detail is buried in a parenthetical note on line 189 ("Phase 1 ships rows 3–4 only").
Suggested improvement: After the table, add a short callout like:
Phase 1 scope: Only canonical-stream subjects (row 3) and room subjects (row 4) are class-aware in Phase 1. User-scoped RPC subjects (rows 1–2
chat.{class}.{account}.request.…) defer to Phase 5+ to minimize Phase 1 blast radius. During Phase 1, bots still publish on the sharedchat.user.{account}.request.…subjects; message-gatekeeper classifies and routes downstream.This removes ambiguity about what "class-aware subject namespace" means at each phase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-traffic-isolation-part2-design.md` around lines 174 - 189, The Phase 1 scope of the subject namespace is unclear because the crucial staging detail is buried in a parenthetical note at the end of section 4.3. Add a prominent callout section immediately after the mirror table that explicitly defines which rows ship in Phase 1 versus Phase 5+, clarifying that only the canonical-stream subjects (row 3) and room subjects (row 4) are class-aware in Phase 1, while user-scoped RPC subjects (rows 1-2) are deferred to Phase 5+. Include a brief explanation of why this deferment minimizes Phase 1 blast radius and how Phase 1 bots still publish on shared subjects with the gatekeeper handling classification.
308-315: ⚡ Quick winClarify the JetStream consumer durable naming for split services.
§6.2 shows consumer config with durable
fmt.Sprintf("%s-%s", svc, class)(e.g.,message-worker-user,message-worker-bot). However, the spec doesn't explicitly state whether:
- The old durable (
message-workeron the legacyMESSAGES_CANONICAL_{siteID}stream) is deleted during the migration, or- Both durables temporarily coexist during Phase 2–3 (dual-subscribe phase).
Add a note under §6.3 or in the migration steps clarifying the durable lifecycle (e.g., "Old durables deleted after Phase 4 cutover soaks" or "Dual durables coexist during Phase 2–3 on separate stream/class pairs").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-traffic-isolation-part2-design.md` around lines 308 - 315, Add a clarification note to section 6.3 or in the migration steps section that explicitly describes the durable consumer lifecycle during the migration. Specify whether the old `message-worker` durable on the legacy MESSAGES_CANONICAL_{siteID} stream is deleted immediately before Phase 2 begins, or whether both the old durable and new class-scoped durables (message-worker-user, message-worker-bot) coexist on separate streams during the dual-subscribe phase (Phase 2–3). Include the timing of when old durables are cleaned up (e.g., after Phase 4 cutover soaks are confirmed).
460-460: 💤 Low valueFix markdown formatting — remove spaces inside code span.
Line 460:
Class string \`json:"class" bson:"class"\(same issue as line 72). Remove spaces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-traffic-isolation-part2-design.md` at line 460, Fix the markdown code span formatting on line 460 in the document where the canonical event struct field is documented. The code span for the Class field definition has spaces inside the backticks that need to be removed. Ensure the markdown syntax is properly formatted with no extra spaces between the opening and closing backticks and the content within, following the same correction pattern applied elsewhere in the document.
493-505: ⚡ Quick winAdd per-phase deployment and config verification steps to the checklist.
The verification checklist (§12) covers dashboard health, SLO stability, federation tests, and rollback drills. Consider adding:
- Phase 3 (split deployments): Verify
-userand-botDeployment config (correct image tag,WORKER_CLASSenv set, connection caps within budget, HPA min/max replicas correct).- Phase 4 (cutover): Verify old-subject subscriptions disabled in manifest before pods restart; no orphaned consumers on old durables.
- Phase 5+ (sunset): Verify
pkg/subjectdual-publish code removed; no lingering references to oldchat.msg.canonical.…subject space.These operational checks ensure the migration state machine doesn't get stuck between phases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-traffic-isolation-part2-design.md` around lines 493 - 505, The verification checklist in section 12 currently lacks phase-specific deployment and configuration verification steps. Add three new subsections after the existing checklist items: for Phase 3, add checks to verify the `-user` and `-bot` Deployment configurations including correct image tags, WORKER_CLASS environment variable settings, connection pool budgets, and HPA replica limits; for Phase 4, add checks to verify that old-subject subscriptions are disabled in the manifest before pod restarts and that no orphaned consumers remain on old durable queues; for Phase 5+, add checks to verify that the dual-publish code in pkg/subject is removed and no lingering references to the old chat.msg.canonical subject space remain in the codebase. These phase-specific checks ensure the migration progresses correctly through each stage without getting stuck between phases.
72-72: 💤 Low valueFix markdown formatting — remove spaces inside code span.
Line 72:
Class string \`json:"class" bson:"class"\has extra spaces between the backticks. Should beClass string `json:"class" bson:"class"`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-traffic-isolation-part2-design.md` at line 72, The markdown formatting for the struct field definition contains escaped backticks with extra spaces that should be corrected. In the line describing the MessageEvent field addition (around line 72 in the markdown file), replace the current backtick pattern with proper inline code formatting by removing the backslash escapes and the extra spaces, so that `Class string `json:"class" bson:"class"`` displays correctly as a code span without escaping characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/specs/bot-platform-nextgen-auth.md`:
- Around line 350-355: The revocation cache invalidation strategy is ambiguous
in the specification, stating "pub/sub invalidation or short TTL" without
committing to either approach. This creates implementation uncertainty about
revocation latency and immediate vs. eventual consistency. Explicitly choose and
document one strategy: either pub/sub invalidation for immediate revocation
across all pods (as suggested by the "immediate kill switch" language in §5.5)
or short TTL-based eventual invalidation bounded by SESSION_CACHE_TTL (§16).
Additionally, clearly define the revocation latency SLA for the chosen strategy,
specifying either the maximum time to propagate revocation (e.g., "10 seconds"
for pub/sub) or the TTL bound (e.g., "5 minutes" for TTL-only), so implementers
understand the exact consistency guarantees.
In `@docs/specs/bot-platform-nextgen-migration-runbook.md`:
- Around line 378-392: Update the migration runbook to address critical external
dependencies before execution. In Section 5, Step 4 ("Activate live
users-sync"), add a pre-check requiring written confirmation from the infra team
that the legacy→nextgen users-sync can be extended to carry the bcrypt hash to
the credentials collection, or document a committed freeze window if the sync
cannot be extended. Add a contingency note in Step 4 that references the freeze
fallback from Part 2 §6.3 as an escalation path if the sync extension is not
achievable. Additionally, in Section 7, mark as blockers the two critical open
questions that must be resolved before Step 1 of the migration job: confirmation
that the users.{account, siteId} compound index exists (PR `#295`) and
clarification of the per-site DB topology (whether each nextgen site has its own
chat DB, which affects migration job parameterization).
In `@docs/specs/bot-traffic-isolation-part1-requirements.md`:
- Line 38: In the Option B row of the requirements table, the phrase "in
proportion to its share" in the rationale column is overly verbose. Replace this
phrase with a more concise and direct alternative such as "still adds
proportional load" or "bot burst still saturates the human Deployment
proportionally" to improve clarity and readability while maintaining the same
meaning.
In `@docs/specs/bot-traffic-isolation-part2-design.md`:
- Around line 174-189: The Phase 1 scope of the subject namespace is unclear
because the crucial staging detail is buried in a parenthetical note at the end
of section 4.3. Add a prominent callout section immediately after the mirror
table that explicitly defines which rows ship in Phase 1 versus Phase 5+,
clarifying that only the canonical-stream subjects (row 3) and room subjects
(row 4) are class-aware in Phase 1, while user-scoped RPC subjects (rows 1-2)
are deferred to Phase 5+. Include a brief explanation of why this deferment
minimizes Phase 1 blast radius and how Phase 1 bots still publish on shared
subjects with the gatekeeper handling classification.
- Around line 308-315: Add a clarification note to section 6.3 or in the
migration steps section that explicitly describes the durable consumer lifecycle
during the migration. Specify whether the old `message-worker` durable on the
legacy MESSAGES_CANONICAL_{siteID} stream is deleted immediately before Phase 2
begins, or whether both the old durable and new class-scoped durables
(message-worker-user, message-worker-bot) coexist on separate streams during the
dual-subscribe phase (Phase 2–3). Include the timing of when old durables are
cleaned up (e.g., after Phase 4 cutover soaks are confirmed).
- Line 460: Fix the markdown code span formatting on line 460 in the document
where the canonical event struct field is documented. The code span for the
Class field definition has spaces inside the backticks that need to be removed.
Ensure the markdown syntax is properly formatted with no extra spaces between
the opening and closing backticks and the content within, following the same
correction pattern applied elsewhere in the document.
- Around line 493-505: The verification checklist in section 12 currently lacks
phase-specific deployment and configuration verification steps. Add three new
subsections after the existing checklist items: for Phase 3, add checks to
verify the `-user` and `-bot` Deployment configurations including correct image
tags, WORKER_CLASS environment variable settings, connection pool budgets, and
HPA replica limits; for Phase 4, add checks to verify that old-subject
subscriptions are disabled in the manifest before pod restarts and that no
orphaned consumers remain on old durable queues; for Phase 5+, add checks to
verify that the dual-publish code in pkg/subject is removed and no lingering
references to the old chat.msg.canonical subject space remain in the codebase.
These phase-specific checks ensure the migration progresses correctly through
each stage without getting stuck between phases.
- Line 72: The markdown formatting for the struct field definition contains
escaped backticks with extra spaces that should be corrected. In the line
describing the MessageEvent field addition (around line 72 in the markdown
file), replace the current backtick pattern with proper inline code formatting
by removing the backslash escapes and the extra spaces, so that `Class string
`json:"class" bson:"class"`` displays correctly as a code span without escaping
characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8664b59c-eb03-4a8e-a4b0-79dcc006f451
⛔ Files ignored due to path filters (2)
docs/specs/bot-platform-nextgen-auth-architecture.svgis excluded by!**/*.svgdocs/specs/bot-platform-nextgen-auth-deployment.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
docs/specs/bot-platform-nextgen-auth-part1-requirements.mddocs/specs/bot-platform-nextgen-auth-part3-components.mddocs/specs/bot-platform-nextgen-auth.mddocs/specs/bot-platform-nextgen-migration-runbook.mddocs/specs/bot-traffic-isolation-part1-requirements.mddocs/specs/bot-traffic-isolation-part2-design.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/specs/bot-platform-nextgen-auth.md (3)
146-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBot account token reversibility is broken if account prefix contains dots.
The
BotAccountTokenfunction (line 152–153) naively replaces all dots with underscores. The comment at line 151 claims the transformation is "round-trippable back to the account identity (replace _bot$ with .bot)," but this only works if the account prefix has no dots.Example:
account="svc.api.bot"→token="svc_api_bot"→ reversing via "replace _bot$ with .bot" yields"svc_api.bot"(not"svc.api.bot") — data is lost.The regex at line 159 (
^[A-Za-z0-9_-]+\.bot$) restricts the base token to alphanumeric/hyphen/underscore (no dots), but this validation only runs at login time (§5.1) and does not apply to credentials imported during migration (§6.1). If production contains a bot account like"svc.api.bot", the import will silently lose data.🛠️ Recommendations
- Clarify constraints: Verify whether production Mongo contains any bot accounts with dots in the prefix (e.g.,
"service.name.bot"). If yes, the current function is broken.- Enforce at migration: Add explicit validation in the migration script (§6.1) to reject any account that doesn't match
IsValidBotAccount, with a loud error listing the rejected accounts.- If needed, revise token function: If dots-in-prefix accounts exist, change the normalization to a deterministic bijection (e.g., base64-encode the account identity, then take a prefix). Document the new round-trip semantics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-auth.md` around lines 146 - 164, The BotAccountToken function's round-trip transformation is broken when the bot account prefix contains dots (e.g., "svc.api.bot" becomes "svc_api_bot" which cannot be correctly reversed). The regex validation in IsValidBotAccount prevents dots in the prefix, but this validation only applies at login time and not during credential migration. Add explicit validation in the migration script (referenced in section 6.1) to reject any bot account that does not match the IsValidBotAccount regex pattern, and output a loud error listing all rejected accounts so the data loss issue is discovered during migration rather than silently corrupting credentials.
203-204:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winJWT scope in §5.2 contradicts the bot subject namespace defined in §4.4.
Line 204 states the JWT should be scoped to
chat.user.{account}.>, but §4.4 explicitly mandates that bot subjects never live underchat.user.>and must usechat.bot.>instead (line 143). The table at line 171 shows the correct bot grant aschat.bot.xxx_bot.>.Section 5.2 must specify that bots receive
chat.bot.{botToken}.>scope while humans receivechat.user.{account}.>scope. The current wording is ambiguous and could lead to an implementation that breaks the namespace isolation contract defined in §4.4.📋 Suggested clarification for §5.2
Replace line 204 with:
mint a short-lived NATS user JWT scoped to chat.user.{account}.> (humans) or chat.bot.{botToken}.> (bots), as determined by the principal's class. Reuses existing JWT machinery; session = durable login, JWT = ephemeral capability.And verify that the JWT minting call in §9 (when
botplatform-servicerequests minting fromauth-service) explicitly passes the principal class so the correct scope is used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-auth.md` around lines 203 - 204, The JWT scope specified in section 5.2 (chat.user.{account}.>) contradicts the bot namespace isolation requirement defined in section 4.4 which mandates bots must use chat.bot.> instead. Update section 5.2 to explicitly clarify that the JWT scope depends on the principal type: humans receive chat.user.{account}.> scope while bots receive chat.bot.{botToken}.> scope as determined by the principal's class. Additionally, verify that section 9 (the JWT minting request from botplatform-service to auth-service) clearly documents that the principal class must be passed to auth-service so it can determine and apply the correct scope for JWT minting.
275-283:⚠️ Potential issue | 🟠 Major | ⚡ Quick winContradictory topology:
/api/v2/*routing ownership unclear.The service diagram at lines 275–283 shows:
bot-gateway:8080 POST /api/v1/login GET/POST /api/v2/* (REST -> NATS translation)But multiple other sections contradict this:
- Line 330 — "we never sit in the
/api/v2/*data path — no reverse proxy, no REST→NATS bridge here (that's downstream, Q13)."- Line 399 — "no cookies, no CSRF...
/api/v2/*is validated then reverse-proxied tobotplatform-server:8080"- Q13 (line 494) — Bridge lives in
Server/data-plane track, not our auth service."The diagram implies
bot-gatewayowns/api/v2/*routing, but the narrative saysServerowns it. Additionally, three different service names appear:botplatform-service(§7, §9 heading),bot-gateway(diagram, old naming), andbotplatform-server(line 399).Clarify before implementation:
- Is
botplatform-servicethe final service name, or is itbot-gateway?- Who owns
/api/v2/*routing — this auth service or theServer/ data-plane backend?- Redraw the topology diagram to match the final narrative (lines 340–348 is correct; lines 275–283 needs updating).
The authoritative source should be lines 340–348 and the detailed narrative in §9.2, which correctly show
botplatform-serviceas auth provider only, not a data-path proxy.Also applies to: 330-330, 399-399
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-auth.md` around lines 275 - 283, The service topology diagram at lines 275–283 contradicts the authoritative narrative in lines 340–348 and §9.2 regarding service ownership of the `/api/v2/*` endpoint. Update the diagram to show that the auth service (named `botplatform-service`, not `bot-gateway`) handles only authentication and JWT minting via POST /auth, while the data-plane backend owns `/api/v2/*` routing and the REST→NATS translation bridge. Additionally, replace all instances of the outdated service names `bot-gateway` and `botplatform-server` with the correct canonical name `botplatform-service` throughout the document to ensure consistency. Make the diagram visually distinct between what the auth service owns (authentication only) and what the downstream Server/data-plane owns (request routing and data translation).
🧹 Nitpick comments (8)
docs/specs/bot-traffic-isolation-part1-requirements.md (1)
39-40: 💤 Low valueOption B / QUEUE-GROUP-ONLY rationale could be more precise.
Line 44 states: "queue-group load-balancing is round-robin, not class-aware."
This is correct, but it may be clearer to add that NATS queue-group load balancing distributes subscriptions across the consumer group, not messages by class. A queue group with both human and bot subscriptions will each receive a round-robin share of all messages regardless of class, defeating the SLO isolation goal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-traffic-isolation-part1-requirements.md` around lines 39 - 40, In the Option B / QUEUE-GROUP-ONLY row, enhance the rationale section to clarify that NATS queue-group load balancing distributes subscriptions (not messages) across the consumer group. Specifically, explain that when both human and bot subscriptions are placed in the same queue group, each subscription receives a round-robin share of all messages regardless of message class, which means the bot Deployment still receives a random mix of human and bot messages. This clarification should directly connect to why this approach fails to achieve the SLO isolation goal mentioned in the trade-offs column.docs/specs/bot-platform-nextgen-auth.md (7)
253-255: 💤 Low valueWordiness: replace "by accident" with a shorter alternative.
The phrase "carried forward by accident" (line 254) is a bit wordy. Consider "carried forward in error" or "was stale" for conciseness.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-auth.md` around lines 253 - 255, The phrase "carried forward by accident" in the naming note explanation is unnecessarily wordy and should be replaced with a more concise alternative. Change "was carried forward by accident" to either "carried forward in error" or "was stale" to improve readability while maintaining the same meaning. This appears in the parenthetical explanation within the Option A (EXTEND-AUTH) rejected section heading.Source: Linters/SAST tools
623-626: ⚡ Quick winConfiguration validation rules missing for secret fields.
Section 16 lists
TOKEN_HMAC_KEY,TOKEN_HMAC_KEY_PREVIOUS, andCSRF_KEYas required secrets but does not specify:
- Format — are they hex strings, base64, or raw bytes?
- Length validation —
TOKEN_HMAC_KEYmust be 32 bytes (§4.3, line 579), but what if the parsed value is shorter?- Validation timing — is length checked at startup (fail-fast) or lazily on first use?
Underspecifying secret handling can lead to subtle production failures (e.g., a 16-byte key silently accepted, weakening the HMAC, not caught until high load exposes the latency difference).
Recommendation: Add explicit validation rules:
TOKEN_HMAC_KEY: required, must decode to exactly 32 bytes (from base64 or hex, specify which), checked at startup.TOKEN_HMAC_KEY_PREVIOUS: optional, same format/length asTOKEN_HMAC_KEY.CSRF_KEY: required, minimum length TBD (specify in Go types section).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-auth.md` around lines 623 - 626, The Configuration section (§16) for botplatform-service describes the secret fields TOKEN_HMAC_KEY, TOKEN_HMAC_KEY_PREVIOUS, and CSRF_KEY as required but omits critical validation rules. Add explicit specifications for each secret field: document the encoding format (e.g., base64 or hex), specify that TOKEN_HMAC_KEY must decode to exactly 32 bytes and TOKEN_HMAC_KEY_PREVIOUS must match the same format and length, define the minimum length requirement for CSRF_KEY, and clarify that all validation occurs at startup (fail-fast) to prevent production failures from invalid key configurations. Update the Configuration section text to include these validation rules inline with each secret's definition.
458-461: ⚖️ Poor tradeoffCanary error-rate gate (0.1%) may be unrealistically strict without staged ramps.
Section 10.2 gates the canary at each step on error rate < 0.1%. For the sustained 1M validations/min load (§9.5), this is ~1000 errors/min, or ~1 error per 100 requests.
At cutover time, this gate may be hard to meet due to:
- Cache cold-starts (first few minutes of traffic fill the Valkey cache).
- Potential migration data-quality issues discovered live (e.g., a user missing a role field).
- Schema/timezone differences between legacy and nextgen Mongo.
A typical canary ramp uses staged gates: start permissive (1–5% for first 30 min), then tighten to 0.1% once stability is proven. The current spec offers no staging.
Recommendation: Revise §10.2 to include:
- Warm-up phase (5–10 min at 1% traffic): let the cache warm, verify basic correctness.
- Staged gates: 1% error rate for 30 min, then 0.5%, then 0.1% for final 30 min before advancing.
- SLO gates (line 459 mentions latency; add it): P99 latency < 200ms (login) or < 5ms (validate cache hit).
Coordinate with the ops team to confirm thresholds based on production load-test results (§9.5, Phase 7).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-auth.md` around lines 458 - 461, Revise section 10.2 to replace the single static 0.1% error-rate gate with a staged canary approach that includes three phases: first, a warm-up phase (5-10 min at 1% traffic) to allow Valkey cache to populate and verify basic data correctness; second, staged error-rate gates that start permissive at 1% error rate for 30 minutes, then tighten to 0.5% for the next phase, before reaching the strict 0.1% threshold for the final 30 minutes; third, add explicit SLO gates for latency (P99 latency < 200ms for login operations, < 5ms for cache-hit validations) alongside the error-rate gates. Add a note to coordinate with the ops team to confirm all thresholds are validated against the production load-test results from section 9.5 Phase 7 before deployment.
684-711: ⚡ Quick winVerification checklist items are gating conditions — should be tracked separately from design decisions.
The specification correctly separates "open questions" (all decided, §12) from "verification items" (external confirmations needed, §22). However, the presentation conflates them:
- Line 487: Correctly notes "Q12/Q13 remain subject to external-team wiring confirmation."
- Lines 689–711: Checklist with [x] (checked) and [ ] (unchecked) items.
The unchecked items are gating blockers for Phase 1 implementation:
Unchecked item Owner Risk Line 691: Admin SSO-only accounts Legacy codebase review Implementation must handle this case Line 698: Identity-sync populates admin+bot Infra/identity-sync team Blocks credential import (Phase 2) Line 700: IsValidBotAccountpattern coverageCodebase audit Migration script validation (Phase 2) Line 704: Istio ingress / VirtualService ownership Infra/ops team Blocks cutover plan (Phase 8) Line 705: ApiGW/Server mTLS integration External teams (gateway/backend) Blocks login reroute (Phase 4) Line 706–707: WS/EventConsumer wiring, Server REST→NATS External teams Blocks validation cutover (Phase 3) These should be tracked as a separate external dependency milestone, not as a checkbox list. Failure to resolve any of these before Phase 1 could cascade into rework.
Recommendation: Before approving this design as an implementation plan:
- Create a Jira epic or milestone linking to each unchecked item.
- Assign owners for each external-team dependency (ops, gateway, backend, identity).
- Schedule confirmations to run in parallel (weeks 1–2 of the project, before Phase 1 scaffolding).
- Document the fallback plan if a confirmation fails (e.g., if Istio mTLS injection is unsupported, what's the mitigation?).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-auth.md` around lines 684 - 711, The verification checklist in section 22 conflates completed design decisions with unchecked external confirmations that are actual gating blockers for Phase 1 implementation. Replace or supplement the current checklist format with a separate section that explicitly tracks the unchecked items (lines 691, 698, 700, 704, 705, 706-707) as external dependencies. For each unchecked item, document the owner (legacy-codebase, infra/identity-sync, ops, gateway, backend teams), the implementation phase it blocks, and a fallback mitigation plan if the confirmation fails. Create a summary table or structured list that makes it clear which items must be resolved before each phase, and recommend that this be tracked as a separate Jira epic or milestone with parallel scheduling for weeks 1-2 of the project, not as simple checkboxes in the design spec.
113-113: 💤 Low valueAmbiguous "namespace" terminology in validation fast-path (§5.3).
The fast-path description states "look up in the v1 namespace only (HMAC hash)" but there is no separate "v1 namespace" in the
sessionscollection. The collection is unsegmented; it contains both legacy and v1 rows mixed by theschemefield.The intended optimization is: if the token starts with
bp1_, compute the HMAC hash and look it up; skip the SHA-256 attempt. But "namespace" is misleading terminology — it suggests a second collection or MongoDB namespace, neither of which exists.Clarify the language to: "Validator prefixes the token with
bp1_→ computebase64(HMAC-SHA-256(server_secret, token))→ querysessions.findOne({_id: h}); no fallback. Otherwise → computebase64(sha256(token))→ query our store, then legacy-store fallback."Also applies to: 207-209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-auth.md` at line 113, The term "v1 namespace" in the validation fast-path description (§5.3 around line 113 and also at lines 207-209) is misleading because the sessions collection is unsegmented and uses the `scheme` field to distinguish rows, not separate namespaces or collections. Replace this terminology with explicit, precise language that clarifies the actual implementation: if the token starts with `bp1_`, compute the base64(HMAC-SHA-256(server_secret, token)) hash and query the sessions collection with no fallback; otherwise compute base64(sha256(token)) and query the store first, then fall back to the legacy-store. This removes the confusion while accurately describing the fast-path optimization logic.
450-453: ⚡ Quick winOperational implication: chat gateway becomes load-bearing forever — HA requirements not specified.
Section 10.1 notes that after cutover, the chat gateway transitions from a legacy-focused hub to a permanent thin forwarder for the chat domain (line 452), with the guidance: "HA the chat-GW accordingly — its blast radius is now load-bearing forever."
This is a critical operational shift: any chat-gateway outage becomes a domain-wide incident, forever. However, the spec does not detail:
- Multi-zone/multi-region redundancy — how many chat-GW replicas, in which zones?
- Failover procedure — if one chat-GW fails, how quickly is traffic rerouted?
- Testing strategy — chaos/failure-injection plan to validate the HA setup pre-cutover?
- Rollback safety — if the chat-GW configuration is wrong post-cutover, can we quickly revert weights back to legacy (yes, per §10.2), but is chat-GW health-checked before rerouting?
Recommendation: Update the migration runbook (Part 3 or the ops-runbook track) to include:
- Pre-cutover checklist: chat-GW HA configuration validated, multi-zone/multi-replica confirmed.
- Health-check integration: chat-GW's own /healthz must be monitored; weighted routing updated only after health gates pass.
- Rollback trigger: if chat-GW latency or error rate spikes during the canary, revert weights back to legacy immediately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-auth.md` around lines 450 - 453, The spec describes chat-GW as a permanent load-bearing forwarder post-cutover but lacks operational HA specifications. Update the migration runbook (Part 3 or ops-runbook track) to add a pre-cutover checklist section that documents multi-zone/multi-replica redundancy requirements for chat-GW, integrates health-check details (chat-GW /healthz endpoint monitoring), and specifies the rollback trigger procedure (reverting weighted routing back to legacy when chat-GW latency or error rates spike during canary). Reference the cutover sections (10.1 and 10.2) to clarify that health gates must pass before weighted routing updates are applied and that rollback to legacy is the immediate response to chat-GW health degradation.
196-199: ⚡ Quick winProvisioning gate error handling conflates authorization and transient failures.
Line 198 states: "Store error → fail closed (same
403, loud server-side log)."Returning
403 Forbiddenfor a transient Mongo error is misleading —403signals an authZ decision (account not provisioned), not a transient infrastructure failure. A timeout, connection error, or quota-exceeded should return5xxso the client knows to retry and upstream monitoring can detect the outage.Returning the same error code for "account not provisioned" and "Mongo unreachable" also weakens troubleshooting and can mask real SLO breaches under normal login-denial logs.
Recommendation: Differentiate error codes:
403 account_not_provisioned— the account exists but hasn't been provisioned to this site (authZ logic).500 internal— Mongo/store error (transient, client should retry).Log both loudly but separately; expose them as distinct metrics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-auth.md` around lines 196 - 199, In the provisioning gate step (step 3), the current spec incorrectly returns `403` for both account-not-provisioned conditions and transient store/database errors. Separate these error cases: when a Miss occurs on the users collection lookup and the account is genuinely not provisioned, return `403 account_not_provisioned`; when a store error occurs (timeout, connection failure, etc.) during the lookup operation, catch and handle that separately by returning `500 internal` to signal a transient failure. Update the error handling logic to distinguish between these two scenarios, ensure each has its own logging statement marked as separate concerns, and document that clients should retry on `5xx` errors while treating `403` as a definitive authorization denial.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/specs/bot-platform-nextgen-auth.md`:
- Around line 146-164: The BotAccountToken function's round-trip transformation
is broken when the bot account prefix contains dots (e.g., "svc.api.bot" becomes
"svc_api_bot" which cannot be correctly reversed). The regex validation in
IsValidBotAccount prevents dots in the prefix, but this validation only applies
at login time and not during credential migration. Add explicit validation in
the migration script (referenced in section 6.1) to reject any bot account that
does not match the IsValidBotAccount regex pattern, and output a loud error
listing all rejected accounts so the data loss issue is discovered during
migration rather than silently corrupting credentials.
- Around line 203-204: The JWT scope specified in section 5.2
(chat.user.{account}.>) contradicts the bot namespace isolation requirement
defined in section 4.4 which mandates bots must use chat.bot.> instead. Update
section 5.2 to explicitly clarify that the JWT scope depends on the principal
type: humans receive chat.user.{account}.> scope while bots receive
chat.bot.{botToken}.> scope as determined by the principal's class.
Additionally, verify that section 9 (the JWT minting request from
botplatform-service to auth-service) clearly documents that the principal class
must be passed to auth-service so it can determine and apply the correct scope
for JWT minting.
- Around line 275-283: The service topology diagram at lines 275–283 contradicts
the authoritative narrative in lines 340–348 and §9.2 regarding service
ownership of the `/api/v2/*` endpoint. Update the diagram to show that the auth
service (named `botplatform-service`, not `bot-gateway`) handles only
authentication and JWT minting via POST /auth, while the data-plane backend owns
`/api/v2/*` routing and the REST→NATS translation bridge. Additionally, replace
all instances of the outdated service names `bot-gateway` and
`botplatform-server` with the correct canonical name `botplatform-service`
throughout the document to ensure consistency. Make the diagram visually
distinct between what the auth service owns (authentication only) and what the
downstream Server/data-plane owns (request routing and data translation).
---
Nitpick comments:
In `@docs/specs/bot-platform-nextgen-auth.md`:
- Around line 253-255: The phrase "carried forward by accident" in the naming
note explanation is unnecessarily wordy and should be replaced with a more
concise alternative. Change "was carried forward by accident" to either "carried
forward in error" or "was stale" to improve readability while maintaining the
same meaning. This appears in the parenthetical explanation within the Option A
(EXTEND-AUTH) rejected section heading.
- Around line 623-626: The Configuration section (§16) for botplatform-service
describes the secret fields TOKEN_HMAC_KEY, TOKEN_HMAC_KEY_PREVIOUS, and
CSRF_KEY as required but omits critical validation rules. Add explicit
specifications for each secret field: document the encoding format (e.g., base64
or hex), specify that TOKEN_HMAC_KEY must decode to exactly 32 bytes and
TOKEN_HMAC_KEY_PREVIOUS must match the same format and length, define the
minimum length requirement for CSRF_KEY, and clarify that all validation occurs
at startup (fail-fast) to prevent production failures from invalid key
configurations. Update the Configuration section text to include these
validation rules inline with each secret's definition.
- Around line 458-461: Revise section 10.2 to replace the single static 0.1%
error-rate gate with a staged canary approach that includes three phases: first,
a warm-up phase (5-10 min at 1% traffic) to allow Valkey cache to populate and
verify basic data correctness; second, staged error-rate gates that start
permissive at 1% error rate for 30 minutes, then tighten to 0.5% for the next
phase, before reaching the strict 0.1% threshold for the final 30 minutes;
third, add explicit SLO gates for latency (P99 latency < 200ms for login
operations, < 5ms for cache-hit validations) alongside the error-rate gates. Add
a note to coordinate with the ops team to confirm all thresholds are validated
against the production load-test results from section 9.5 Phase 7 before
deployment.
- Around line 684-711: The verification checklist in section 22 conflates
completed design decisions with unchecked external confirmations that are actual
gating blockers for Phase 1 implementation. Replace or supplement the current
checklist format with a separate section that explicitly tracks the unchecked
items (lines 691, 698, 700, 704, 705, 706-707) as external dependencies. For
each unchecked item, document the owner (legacy-codebase, infra/identity-sync,
ops, gateway, backend teams), the implementation phase it blocks, and a fallback
mitigation plan if the confirmation fails. Create a summary table or structured
list that makes it clear which items must be resolved before each phase, and
recommend that this be tracked as a separate Jira epic or milestone with
parallel scheduling for weeks 1-2 of the project, not as simple checkboxes in
the design spec.
- Line 113: The term "v1 namespace" in the validation fast-path description
(§5.3 around line 113 and also at lines 207-209) is misleading because the
sessions collection is unsegmented and uses the `scheme` field to distinguish
rows, not separate namespaces or collections. Replace this terminology with
explicit, precise language that clarifies the actual implementation: if the
token starts with `bp1_`, compute the base64(HMAC-SHA-256(server_secret, token))
hash and query the sessions collection with no fallback; otherwise compute
base64(sha256(token)) and query the store first, then fall back to the
legacy-store. This removes the confusion while accurately describing the
fast-path optimization logic.
- Around line 450-453: The spec describes chat-GW as a permanent load-bearing
forwarder post-cutover but lacks operational HA specifications. Update the
migration runbook (Part 3 or ops-runbook track) to add a pre-cutover checklist
section that documents multi-zone/multi-replica redundancy requirements for
chat-GW, integrates health-check details (chat-GW /healthz endpoint monitoring),
and specifies the rollback trigger procedure (reverting weighted routing back to
legacy when chat-GW latency or error rates spike during canary). Reference the
cutover sections (10.1 and 10.2) to clarify that health gates must pass before
weighted routing updates are applied and that rollback to legacy is the
immediate response to chat-GW health degradation.
- Around line 196-199: In the provisioning gate step (step 3), the current spec
incorrectly returns `403` for both account-not-provisioned conditions and
transient store/database errors. Separate these error cases: when a Miss occurs
on the users collection lookup and the account is genuinely not provisioned,
return `403 account_not_provisioned`; when a store error occurs (timeout,
connection failure, etc.) during the lookup operation, catch and handle that
separately by returning `500 internal` to signal a transient failure. Update the
error handling logic to distinguish between these two scenarios, ensure each has
its own logging statement marked as separate concerns, and document that clients
should retry on `5xx` errors while treating `403` as a definitive authorization
denial.
In `@docs/specs/bot-traffic-isolation-part1-requirements.md`:
- Around line 39-40: In the Option B / QUEUE-GROUP-ONLY row, enhance the
rationale section to clarify that NATS queue-group load balancing distributes
subscriptions (not messages) across the consumer group. Specifically, explain
that when both human and bot subscriptions are placed in the same queue group,
each subscription receives a round-robin share of all messages regardless of
message class, which means the bot Deployment still receives a random mix of
human and bot messages. This clarification should directly connect to why this
approach fails to achieve the SLO isolation goal mentioned in the trade-offs
column.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c91e409d-d290-4e3d-ab79-326bde1b13ab
📒 Files selected for processing (4)
docs/specs/bot-platform-nextgen-auth-part1-requirements.mddocs/specs/bot-platform-nextgen-auth-part3-components.mddocs/specs/bot-platform-nextgen-auth.mddocs/specs/bot-traffic-isolation-part1-requirements.md
✅ Files skipped from review due to trivial changes (2)
- docs/specs/bot-platform-nextgen-auth-part1-requirements.md
- docs/specs/bot-platform-nextgen-auth-part3-components.md
Joey0538
force-pushed
the
claude/bot-platform-nextgen-migration-hb6ok2
branch
2 times, most recently
from
eb5b66e to
23d9f9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/specs/bot-platform-nextgen-auth.md (1)
659-662:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
/v1/auth/validatereturn one schema everywhere.This table still lists flat
{valid, account, userId}, while Part I and §9.8 describe a{valid, principal}response. Pick one payload shape and use it consistently; downstream callers will otherwise wire against different contracts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/specs/bot-platform-nextgen-auth.md` around lines 659 - 662, The `/v1/auth/validate` endpoint response schema is inconsistent across the documentation. The table currently shows the response as JSON `{valid, account, userId}`, but Part I and §9.8 describe it as `{valid, principal}`. Update the response schema in the table row for the `/v1/auth/validate` endpoint to match the `{valid, principal}` format described elsewhere in the document to ensure downstream callers have a single consistent contract to implement against.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/settings.json:
- Around line 26-37: The extraKnownMarketplaces configuration for the 365-skills
marketplace is missing SHA pinning on the source object, which creates a
security risk since this community-maintained plugin source can be silently
updated without user consent. Add a "sha" field to the source object within the
365-skills marketplace configuration and set it to the specific commit SHA of
the version you intend to use (such as the latest release v1.5.2 from May 2026
or another specific commit hash).
In `@docs/specs/bot-platform-nextgen-auth.md`:
- Around line 1052-1058: Add blank lines around the table following the heading
"4.3 Token compatibility (migration support)" to fix the MD058 markdown linting
violation. Insert a blank line between the heading and the table start, and
another blank line after the table ends before any following content. This
ensures proper markdown rendering consistency and resolves the formatting flag.
- Around line 991-993: The description of botplatform-service incorrectly
includes "API proxy" in its responsibilities, which contradicts the rest of the
specification that states this service is not in the `/api/v2/*` data path and
does not act as a proxy for API calls. Remove "API proxy" from the
botplatform-service summary line, keeping only the core responsibilities like
Auth, web UI, and token validation that align with its actual role as an
authentication provider rather than a data path proxy.
- Around line 840-846: The markdown code blocks in the Token issuance (v1)
section under "14. Algorithms (precise)" and at lines 1059-1064 are missing
language tags on their code fences. Add an appropriate language identifier (such
as `go` or `text`) to each triple-backtick code fence opening (e.g., change ```
to ```go or ```text) to satisfy the MD040 linter requirement and ensure the
pseudocode examples render properly. Apply this fix to all unlabeled code blocks
throughout the document, including the dual-hash section referenced in the
amendment note.
- Around line 861-866: In the session validation pseudocode example showing the
gateway hot path, replace the cold path NATS request
`nats.Request(auth.session.validate, h)` with the HTTP validate flow using the
`/v1/auth/validate` endpoint, which is documented elsewhere in the spec as the
single authority. Alternatively, if this NATS pattern is meant to represent an
internal helper in a different component, add a clear comment or label in the
pseudocode indicating it is not part of the auth service but rather an
implementation detail of another internal service, to avoid contradicting the
specification that states this service exposes no NATS subjects.
- Around line 466-467: The documentation has conflicting statements about NATS
JWT minting responsibility. The section at lines 466-467 indicates the login
endpoint can mint and return a NATS user JWT when natsPublicKey is present, but
other sections describe NATS JWT minting as future work through the auth-service
using AUTH_SERVICE_URL. Clarify which service is responsible for signing NATS
tokens in the current release by deciding whether the login endpoint handles
native-bot JWT minting directly or delegates it to auth-service, then update all
references to NATS JWT minting throughout the document to reflect this decision
and establish a clear service boundary for the current implementation phase.
In `@docs/specs/bot-platform-nextgen-migration-runbook.md`:
- Around line 361-363: Delete the assertion that checks for leaked PATs using
the non-existent schemeOriginal field in the sessions model, since this field is
never defined and the assertion will always pass without detecting anything. The
earlier source-side count already provides adequate PAT exclusion coverage,
making this stale check unnecessary and redundant.
- Around line 220-224: The "No race" claim in the Conflict policy property is
incomplete because it assumes live-sync cannot run before bulk import inserts
the document, but this precondition is not explicitly stated. Clarify the
precondition by either adding an explicit requirement that step 4 (live
users-sync) must remain disabled until the bulk import completes, or switch the
sync path to use an upsert operation (changing from $set to $setOnInsert or
using $set within an upsert context) so it safely handles the case where
live-sync runs concurrently with or before bulk import. Make the chosen approach
explicit in the conflict policy documentation so readers understand the race
condition guarantee.
- Around line 412-417: The aggregate pipeline checking bot credentials doesn't
validate the siteId field directly, so it cannot prove credentials are in the
correct database. Replace the current aggregate query that performs a lookup to
users and compares accounts with a simpler check that directly matches
credentials.siteId against the expected site identifier from CONFIG.siteId. The
assertion should verify that there are no credentials documents where siteId
does not match the current site's configuration.
In `@docs/specs/bot-traffic-isolation.md`:
- Around line 657-658: The documentation contains conflicting guidance on
sourcing the is_bot value between the current statement (use
ctxutil.PrincipalClass(ctx)) and Part I §9.1 (use model.IsBotAccount(account) in
Phase 0). Clarify the phased fallback strategy by explicitly stating that during
Phase 0, when principal.class is not yet available end-to-end, implementers
should derive is_bot from model.IsBotAccount(account) as a temporary measure,
and then transition to using ctxutil.PrincipalClass(ctx) as the canonical source
in subsequent phases. Make it clear that once principal.class is available,
implementers must never re-derive is_bot from account ID at metric-emit time.
- Around line 594-638: The example manifest shows message-gatekeeper split into
separate user and bot deployments, but the specification in §5.1 and Part I §5
explicitly states that message-gatekeeper should remain unified through Phase 4.
Either clarify this example as a Phase 5 split-service configuration where the
service separation occurs, or revise the example to show a unified
message-gatekeeper deployment with traffic isolation implemented through other
mechanisms (such as separate worker pools or traffic routing rules) that aligns
with the current phase requirements described in the specification.
---
Outside diff comments:
In `@docs/specs/bot-platform-nextgen-auth.md`:
- Around line 659-662: The `/v1/auth/validate` endpoint response schema is
inconsistent across the documentation. The table currently shows the response as
JSON `{valid, account, userId}`, but Part I and §9.8 describe it as `{valid,
principal}`. Update the response schema in the table row for the
`/v1/auth/validate` endpoint to match the `{valid, principal}` format described
elsewhere in the document to ensure downstream callers have a single consistent
contract to implement against.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c84fe56a-6a8f-4438-9615-f5f3f30fbdba
⛔ Files ignored due to path filters (2)
docs/specs/diagrams/bot-login-flow.drawio.pngis excluded by!**/*.pngdocs/specs/diagrams/cross-cluster-cutover.drawio.pngis excluded by!**/*.png
📒 Files selected for processing (6)
.claude/settings.jsondocs/specs/bot-platform-nextgen-auth.mddocs/specs/bot-platform-nextgen-migration-runbook.mddocs/specs/bot-traffic-isolation.mddocs/specs/diagrams/bot-login-flow.drawiodocs/specs/diagrams/cross-cluster-cutover.drawio
✅ Files skipped from review due to trivial changes (2)
- docs/specs/diagrams/bot-login-flow.drawio
- docs/specs/diagrams/cross-cluster-cutover.drawio
Joey0538
force-pushed
the
claude/bot-platform-nextgen-migration-hb6ok2
branch
3 times, most recently
from
456d255 to
03182e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/specs/botplatform/auth.md`:
- Around line 1013-1015: Reconcile the service boundary definition for
`/api/v2/*` traffic in the authentication section to be consistent with the rest
of the specification. Currently, the section describes `botplatform-server` as
bridging REST→NATS traffic, but earlier in the spec it states that ApiGW
validates and forwards directly to `Server` under mTLS with
`botplatform-service` explicitly not on the data path. Review and clarify
whether `/api/v2/*` requests flow through `botplatform-service` or directly from
ApiGW to `botplatform-server`, ensuring the service boundaries and
responsibilities are unambiguous and consistent throughout the entire
specification to prevent implementation targeting the wrong service hop.
- Around line 678-681: The API contract documentation for the
`/v1/auth/validate` endpoint is incomplete and missing a required response
field. Update the response contract column in the token validation table row to
include `principal.class` along with the existing fields `valid`, `account`, and
`userId` so it reflects the canonical response shape specified in Part II §9.8
and matches what downstream consumers like ApiGW and WS integration actually
expect.
- Around line 1041-1047: The admin endpoint documentation uses `/v1/admin/bots…`
which contradicts the route specification in Part II §8 that defines admin
actions as role-gated HTML form posts on `/admin/...`. Update the endpoint
listed in the bullet point from `/v1/admin/bots…` to `/admin/bots…` to maintain
consistency with the web-UI contract and the earlier route table specification
throughout the document.
In `@docs/specs/botplatform/migration-runbook.md`:
- Around line 381-390: The migration runbook lacks a mechanism to handle data
changes in the legacy system during the credentials and sessions import window
(steps 2-3), creating a risk of data drift. Add a new step between step 3 and
step 4 that either implements a source freeze on the legacy system to prevent
further writes during import, or alternatively runs the users-sync continuously
during steps 2-3 with a final reconciliation pass to catch any delta changes.
Document the pre-check (confirm legacy is frozen or final sync complete),
post-check (verify no pending changes exist), and rollback procedure for this
new freeze/delta-catch-up step.
- Around line 316-339: The token filtering logic in the loop over user services
resume login tokens only explicitly skips PATs but does not enforce the
documented allow-list which requires importing only tokens with empty or absent
type fields. Any other non-empty token type would incorrectly be migrated.
Replace the current PAT-only skip condition with a check that skips any token
with a non-empty type field, so that only tokens with absent or empty type
values are processed and migrated to the nextgen_chat.sessions collection.
In `@docs/specs/botplatform/traffic-isolation.md`:
- Around line 127-129: The status column for `message-gatekeeper` says "review
after Phase 5" but section 5.1 states it stays shared through Phase 4 and is
re-evaluated in "Phase 5+", creating ambiguity about the exact cutover point.
Update the phase milestone wording in either the table entry or section 5.1 (or
both) so they use consistent language that clearly defines when
`message-gatekeeper` re-evaluation occurs, ensuring the rollout and rollback
plan has one unambiguous milestone.
- Around line 553-559: The migration steps describe both mirroring old subject
traffic into the new MESSAGES_CANONICAL_USER_{siteID} stream in step 2 and
dual-publishing to both old and new subjects during the publisher cutover in
step 3, which creates a window where the same event could be ingested twice.
Either clarify that the mirror phase and dual-publish phase must not overlap
(make them sequential, not concurrent) by adding explicit timing constraints
between steps 2 and 3, or if they do overlap, explicitly document the stable
message ID or idempotency key that consumers will use to deduplicate these
duplicate writes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16d47b9b-eaed-417c-a2f3-59c1075a5a21
⛔ Files ignored due to path filters (4)
docs/specs/botplatform/diagrams/bot-login-flow.drawio.pngis excluded by!**/*.pngdocs/specs/botplatform/diagrams/cross-cluster-cutover.drawio.pngis excluded by!**/*.pngdocs/specs/botplatform/diagrams/login-old-vs-new.drawio.pngis excluded by!**/*.pngdocs/specs/botplatform/diagrams/token-gen-validate-flow.drawio.pngis excluded by!**/*.png
📒 Files selected for processing (9)
.claude/settings.jsondocs/specs/botplatform/README.mddocs/specs/botplatform/auth.mddocs/specs/botplatform/diagrams/bot-login-flow.drawiodocs/specs/botplatform/diagrams/cross-cluster-cutover.drawiodocs/specs/botplatform/diagrams/login-old-vs-new.drawiodocs/specs/botplatform/diagrams/token-gen-validate-flow.drawiodocs/specs/botplatform/migration-runbook.mddocs/specs/botplatform/traffic-isolation.md
✅ Files skipped from review due to trivial changes (4)
- docs/specs/botplatform/diagrams/login-old-vs-new.drawio
- docs/specs/botplatform/diagrams/cross-cluster-cutover.drawio
- docs/specs/botplatform/diagrams/token-gen-validate-flow.drawio
- docs/specs/botplatform/diagrams/bot-login-flow.drawio
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/settings.json
Joey0538
force-pushed
the
claude/bot-platform-nextgen-migration-hb6ok2
branch
2 times, most recently
from
28ab02b to
335b5c7
Compare
mliu33
reviewed
Jun 23, 2026
| name: "Alice Smith", | ||
| active: true, | ||
| createdAt: 1718500000000, | ||
| schemaVersion: "v1" // NEW — forward-compat hook |
Collaborator
There was a problem hiding this comment.
I am not sure what is the use of "schemaVersion" field ? I think maybe its better to remove for now to keep things simplpe
mliu33
reviewed
Jun 23, 2026
| Indexes: unique on `account`. | ||
|
|
||
| ### 4.2 `sessions` collection | ||
| One document per live session. **No array, no cap.** |
Collaborator
There was a problem hiding this comment.
This new sessions collection design is good, but we still need to keep a configurable cap protection in login api (we can increase to 100).
Otherwise, misbehaved bots can spam and flood our db with tons of sessions if they perform login before every api call
mliu33
reviewed
Jun 23, 2026
| ### 5.6 "Resume" / session reuse | ||
| **Legacy REST bots:** no resume verb — a bot logs in once (username + password) and reuses its `X-Auth-Token` header on every call; the header *is* session reuse. | ||
|
|
||
| **Internal primitive:** §5.2 — a native client or the REST edge exchanges a still-valid session token for a fresh short-lived NATS JWT, without re-entering the password. Session token = durable resume credential; JWT = ephemeral capability. |
Collaborator
There was a problem hiding this comment.
For this bot/admin username/password login flow, we will not return NATS JWT. We will only return access token and userID
mliu33
reviewed
Jun 23, 2026
|
|
||
| **Internal primitive:** §5.2 — a native client or the REST edge exchanges a still-valid session token for a fresh short-lived NATS JWT, without re-entering the password. Session token = durable resume credential; JWT = ephemeral capability. | ||
|
|
||
| **New native bot SDK (re-architecture) — decided (Q1b):** the internal session→JWT exchange ships anyway; the **public `session.refresh` resume RPC is deferred to the native-SDK milestone** (no caller until that SDK exists). Legacy REST bots are untouched (header reuse). |
Collaborator
There was a problem hiding this comment.
Bot SDK will only interact with Restful HTTP api as existing behavior
mliu33
reviewed
Jun 23, 2026
|
|
||
| ## 8. Admin = role-gated web UI (Q15, Q18) | ||
|
|
||
| There is **no separate admin API.** `/dev-login` is the **one** web login for humans (admins *and* bot-account holders / devs); after auth the **server-rendered UI is role-aware**: |
Collaborator
There was a problem hiding this comment.
Do you think it'll be easier and better to have a separate web login path for admins like "/admin-login", which will render admin specific page with all the admin actions
Joey0538
force-pushed
the
claude/bot-platform-nextgen-migration-hb6ok2
branch
2 times, most recently
from
a2f25b5 to
2179b64
Compare
…isolation + runbook + diagrams + Claude plugin)
Initial design-record drop for the bot-platform-nextgen migration —
password auth + durable sessions for bots/admins, cross-cluster
cutover behind the chat-gateway, and a companion bot-traffic
isolation spec.
Layout (all under docs/specs/botplatform/):
* README.md — spec-set index + reading order + citation convention
* auth.md — main design spec, combined Parts I (architecture &
requirements), II (technical design), III (components &
integration). Covers the architecture decision (Option B /
DEDICATED-SERVICE), data model, hashing/verify algorithms, NATS
subjects, gateway topology, performance design, configuration,
test plan, verification checklist, open questions, and bot/admin
integration with ApiGW / WebSocket / EventConsumer.
* traffic-isolation.md — companion spec for routing bot traffic to
separate worker pools (Option A / SUBJECT-SPLIT decision). Covers
principal.class propagation, NATS subject namespaces, JetStream
stream design, per-service deployment split plan, supercluster
permissions, observability, and rollout phases.
* migration-runbook.md — operational AS-IS vs TO-BE runbook for the
Mongo collection deltas (users + sessions), per-site cutover
ordering, reconciliation queries, rollback steps.
* diagrams/ — 4 editable .drawio sources + rendered PNGs (embedded
inline in auth.md §10): login-old-vs-new, token-gen-validate-flow,
bot-login-flow, cross-cluster-cutover.
Key decisions baked in:
* Auth architecture: Option B / DEDICATED-SERVICE — new
botplatform-service (decided 2026-06-15 in design review). Bot
edge is a browser-facing web app + dual-token validation; isolating
from auth-service's JWT signing wins on blast-radius/key safety.
* Token format: native v1 = bp_<43-char base64url of 32 random bytes>,
stored as base64(HMAC-SHA-256(TOKEN_HMAC_KEY, token)). Legacy
unchanged for byte-for-byte compatibility. Validator dispatches on
the bp_ prefix.
* Bot account namespace: bots scope to chat.bot.{account}.>;
xxx.bot → xxx normalization via subject.BotSubjectName. Resolves
PR #295 strict-validator conflict + eliminates ACL escape between
human xxx and bot xxx.bot.
* Provisioning gate: REQUIRE_PROVISIONED=true default; mirrors
auth-service from PR #295. Bots can only log in at their home site.
* Cross-cluster cutover: chat-GW (fz2) is the PERMANENT front door
(DNS bound per-namespace, DNS-repoint infeasible). Weight-split
cross-cluster to wsp-GW.
* Traffic isolation: Option A / SUBJECT-SPLIT. message-worker +
broadcast-worker split per-class first; message-gatekeeper stays
shared through Phase 4.
Tooling:
* .claude/settings.json — adds the Agents365-ai/365-skills marketplace
(SHA-pinned for reproducibility) and enables the drawio plugin
team-wide. Used to generate the four spec diagrams; anyone working
in this repo gets the diagram-rendering tooling for free.
…hape (Q13/Q15)
Two clarifications that surfaced during PR review discussion. Both
are spec-additive — they document decisions already implied by the
rest of the spec but not pinned anywhere, preventing two predictable
re-litigation rounds during the internal team's build.
§15.1 (NEW subsection) — "Transport choice: why HTTP, not NATS RPC"
Reviewer FAQ. CLAUDE.md §6 says "use NATS request/reply for
synchronous operations" — so why is /v1/auth/validate HTTP? Both
transports are synchronous; the choice is operational fit. We
pick HTTP because (1) the service inherently serves a web UI
(HTML + CSRF + cookies), forcing HTTP anyway; (2) ApiGW is
Envoy-native and HTTP routing is its native skill; (3) in-mesh
HTTP hop (~1ms) is well inside §9.5's <5ms SLA. Reversible later
if production data justifies adding a NATS-RPC sidecar.
Explicitly states sync-vs-async is NOT the differentiator here
(both HTTP and NATS request/reply are synchronous request/reply).
Pub/sub and JetStream would be wrong shapes (no reply value),
but NATS request/reply would have worked — just at higher
operational cost.
Q13 (extended) — "Bridge shape: synchronous NATS request/reply,
NOT pub/sub or JetStream"
Q13 already says the bridge lives in botplatform-server / data-
plane track, but didn't pin the SHAPE. An implementer could read
it and pick pub/sub or JetStream, silently breaking the bot's
HTTP response expectation. Pins the shape:
HTTP request → nc.Request(subj, payload, timeout) → reply → HTTP response
Documents the helpers (pkg/subject, pkg/natsrouter,
pkg/natsutil, pkg/errcode + errnats.Reply) and the timeout
pyramid (bot 30s > ApiGW 25s > bridge 20s > nc.Request 5-10s)
so chained-sync-hop timeouts don't cascade into stuck 504s.
Calls out the wrong shapes explicitly (pub/sub, JetStream) so
the "wrong tool" mistake is hard to make.
Also fixed: §15's admin route reference (/v1/admin/bots… → /admin/bots…)
to match the round-2 CodeRabbit fix in Part III §3. The route table
itself was already updated in Part I §3a + Part III §3; §15 had one
straggler.
Also: added `account_not_provisioned` to §15's error reasons list —
mirrors the provisioning gate from Part I §5 + Part II §5.1.
…ollection) + admin split (admin-portal + admin-service)
Two intertwined pivots merged with the prior cap+permanent-sessions work
into one commit, both driven by user/ops feedback:
1. MIXED-STORAGE MODEL (corrects the prior "collapse everything onto
users" pivot). Passwords stay on users.services.password.bcrypt
in-place (no credentials collection — the credential side of the
2026-06-24 collapse stands). Sessions go BACK to a dedicated
`sessions` collection, one doc per session keyed by token hash:
- O(1) validate via sessions.findOne({_id: hash}) — no in-doc
array scan, no multikey index needed for the hot path
- Bounded users-doc size — sessions don't bloat the identity
doc that every nextgen service caches via pkg/userstore
- Per-session revocation primitive — admin revoke is one
deleteOne; revoke-all is one deleteMany({userId})
- Cap enforcement reverts from atomic $push+$slice (which only
works on an in-doc array) to count + delete-oldest-by-issuedAt
at login. Race tolerance: brief cap+1/cap+2 overshoot heals
on the next login; no Mongo transactions needed
- Index requirement: {userId: 1, issuedAt: 1} compound — IXSCAN
serves both the cap-eviction victim lookup AND admin's
list-sessions-by-user query
- schemaVersion field intentionally absent (PR feedback) —
skip until we actually need it
2. ADMIN SPLIT (reverses Q18). Admin becomes its own portal +
service, distinct from botplatform-service:
- botplatform-service narrows to login + validate + changepwd
(universal HTML pages serving both chat and admin portals).
No more /admin/* HTML routes here.
- admin-service (NEW) — REST JSON APIs for all admin
operations: list/create/suspend bots, rotate password,
revoke single/all sessions, future rate-limit config.
Every route requires class:"admin" verified via
/v1/auth/validate against the inbound session cookie.
- admin-portal (NEW) — static web frontend hosted at
admin-{site}.…; calls admin-service via XHR under the
admin's session cookie.
- /dev-login stays the universal login form on
botplatform-service. Post-login redirect target depends on
the calling portal (?next= / Referer) — same admin user can
land on chat frontend (via chat.xxx.com/dev-login) or admin
portal (via admin-{site}.…/dev-login).
- Q18 reversed: previous "no separate admin API" stance
was wrong for the actual ops UX needs (distinct deploy
cadence, isolated authz boundary, distinct frontend).
Carryover from the previous combined pivot (still applies):
- Permanent sessions (no expiresAt, no lastUsedAt, no TTL reap)
- Pure-read validate (Valkey GETEX refreshes in-memory TTL;
cache miss does sessions.findOne; no Mongo writes on the hot path)
- FIFO eviction by issuedAt at login (cap-evicted entries
valkey.del'd explicitly, not left to age out)
- Dual-token hash dispatch by bp1_ prefix (legacy SHA-256 vs
v1 HMAC-SHA-256)
- sessionExpired error reason removed
- SESSION_IDLE_WINDOW / SESSION_LASTUSED_THROTTLE env vars
removed
Migration impact (migration-runbook.md):
- Passwords: no extraction needed — identity-sync already
carries services.password.bcrypt end-to-end
- Sessions: ONE bulk-import job at cutover from legacy
users.services.resume.loginTokens[] → new sessions collection
(skip PATs; verbatim hashedToken → _id; scheme:"legacy"). After
cutover, legacy continues to write the legacy array; nextgen
never re-reads it (canary monotonically shifts forward; bot
re-login on miss is cheap)
- Cutover ordering simplified to 6 steps; added admin-service +
admin-portal as step 4 (dark deploy before canary)
- schemaVersion intentionally omitted from sessions row schema
(PR feedback)
Topology clarifications (auth.md §10.1):
- Gateway host claims and DNS: chat-gw and wsp-gw can both claim
*.chat.fx.abc.com in their server blocks; DNS (not Istio
config) decides routing — point *.chat.fx.abc.com only at
chat-gw's external IP. wsp-gw's claim is only for cross-cluster
forwarded traffic from chat-gw.
- Deploy nextgen pods in fz1 only during the ramp:
botplatform-service.wsp.svc has zero endpoints in fz2, so the
mesh resolves cross-cluster to fz1. If you ever deploy in
both clusters, Istio's default locality-aware LB would prefer
same-cluster pods — override via DestinationRule's
localityLbSetting or keep fz1-only.
Diagrams (re-rendered):
- bot-login-flow.drawio: step 13 reverted to
sessions.insertOne; step 13b cap-eviction is count + delete
oldest by issuedAt (not $slice).
- login-old-vs-new.drawio: NEW panel step 5 reverted to
dedicated sessions collection + count/delete-oldest cap.
Wins panel updated to reflect O(1) validate via
sessions.findOne, bounded users-doc, dedicated collection
benefits. Compat panel: passwords stay on users, sessions
are bulk-imported once at cutover.
- token-gen-validate-flow.drawio: g_persist reverted to
sessions.insertOne; v_mongo cold path is
sessions.findOne({_id:hash}) — O(1), single IXSCAN.
…h-service POST /auth kind discriminator Addresses mliu33's PR-review comments #3 (login should not return a NATS JWT — only authToken + userId) and #4 (Bot SDK is REST-only, existing behavior). Architectural pivot, not a feature change: * botplatform-service NEVER mints NATS JWTs and NEVER holds the signing key. The login endpoint (/api/v1/login, /v1/bot/login, /dev-login) returns ONLY {authToken, userId, me}. The natsPublicKey request field is gone, the optional jwt response field is gone, the §3 Key Decisions "NATS JWT" row is restated. * auth-service stays the single NATS-JWT minter for the whole system, with a unified 3-field POST /auth body: { kind, token, natsPublicKey } - kind:"sso" → token = OIDC bearer (existing humans). - kind:"bot" → token = bp_ session token; auth-service calls botplatform-service /v1/auth/validate to verify before minting. No separate userId field — the session token self-identifies; /v1/auth/validate returns the full principal. Migration: just update the existing endpoint in place — no new route, no compat shape, the endpoint is still under active development (no live external callers to coast for). * Response envelope {natsJwt, user} is identical across kinds. user.account always present; SSO populates the full HR profile (email, employeeId, engName, chineseName, deptName, deptId); bot populates account only (others omit-empty). Frontend reads user.account universally. * The §5.2 primitive previously phrased as "internal session→JWT exchange on botplatform-service" is recast as the auth-service kind:"bot" extension. It exists ONLY for flow B (chat-frontend using a bot account); flow A (pure REST bot SDK pods, the 99% case) never invokes it. Spec edits (auth.md): * NEW §3a — explicit Flow A vs Flow B table at the top of Part II so the rest of the spec reads against a clear consumer model. * §3 Key Decisions — "NATS JWT" row rewritten: minted by auth-service only, never by login, never by botplatform-service. * §5.1 step 6 — login response is {authToken, userId, me} only; removed the deferred natsPublicKey delegation step. * §5.2 — completely rewritten: auth-service POST /auth extension shape (kind + token + natsPublicKey), bot-kind dispatch flow (validate → mint chat.bot.{token}.> grant), unified response envelope with field-presence notes. Caller always knows kind because the caller knows which login it just performed — no server-side guessing. * §5.7 — "Resume / session reuse" reframed: flow B primitive lives on auth-service; Q1b session.refresh routes through the same auth-service extension if/when a native bot SDK lands. * §7 Option B — "signing key stays in auth-service; botplatform-service never mints" tightened. * §9.2 topology comment — auth-service calls back IN to botplatform-service /v1/auth/validate for kind:"bot" mints (not the other direction). * §13 Go types — drop NATSKey field from loginRequest; drop optional jwt field from loginResponse. * §15 transport note — botplatform-service has no NATS surface and no AUTH_SERVICE_URL outbound; the JWT-mint call is the OTHER direction (auth-service → botplatform-service). * §16 config — explicit NO AUTH_SERVICE_URL on botplatform-service. * §20 client-api delta — login request: drop natsPublicKey. * §12 Q1b + Q5 — restated to reflect the auth-service ownership of the bot-kind mint. Original ticket scope (§3 Part I context "Signing key stays put") sharpened to: botplatform-service has zero JWT involvement.
… kind to sso/bot/admin
Unversioned per-class prefixes — class is self-evident from
X-Auth-Token without a DB lookup:
* bp_<43-char base64url> = bot session token
* ad_<43-char base64url> = admin session token
* legacy unprefixed = imported RC SHA-256 tokens (unchanged)
No version digit (bp_ not bp1_) — login mechanism isn't planned to
rotate; YAGNI.
§3 Key Decisions table updated:
* Token wire format row: per-class prefixes, no version
* NATS JWT row: kind ∈ {sso, bot, admin}; admin → chat.> god-mode
(decided 2026-06-24), bot → chat.bot.{token}.>, sso →
chat.user.{account}.>. For kind:"bot" and kind:"admin",
auth-service calls back into botplatform-service /v1/auth/validate
before minting.
Bulk rename across the spec — every bp1_ reference updated. Followup
commits will add IsAdminAccount helper, the validator dispatch table,
and the §5.2 kind:"admin" branch.
…count} (strip .bot); add IsAdminAccount + class:admin
Cross-spec naming cleanup applied to both auth.md and traffic-isolation.md.
(1) Subject parameter rename: chat.bot.{botToken}.> → chat.bot.{account}.>
The old "botToken" name overloaded with the auth credential (bp_…); the
value was always the bot's account stripped of the .bot suffix. Rename to
{account} for clarity. Also drops the dot-normalization
("xxx.bot"→"xxx_bot") in favor of plain suffix-strip ("xxx.bot"→"xxx") —
the chat.bot.> namespace already encodes the class, so .bot becomes
redundant inside it.
Helpers (auth.md §4.7):
BotSubjectName(account) "alice.bot" → "alice"
BotAccountFromSubjectName(name) "alice" → "alice.bot"
Replaces BotAccountToken (which did dot-normalization).
(2) Admin account classification (auth.md §4.7 new subsection):
model.IsAdminAccount(account) — accepts the legacy p_xxx prefix
(e.g. "p_jeff"). Admin login path validates user with IsAdminAccount;
/v1/auth/validate returns class:"admin" when roles ∋ "admin" (roles are
authoritative, the helper is a login-side sanity gate).
Cross-spec touches in auth.md:
* §3 Key Decisions NATS JWT row: kind:"bot" mints
chat.bot.{stripped}.>, kind:"admin" mints chat.> god-mode.
* Part I §3a flow tables: stripped-account form.
* §4.7 Layer 2 rewritten with BotSubjectName, plus a new "Admin
account namespace" subsection.
* §4.7 result table: now shows alice / alice.bot disjointness, plus
the admin row (n/a / chat.>).
* §4.7 validator dispatch: three validators (human SSO / bot login /
admin login), each with its own login-path use.
* §5.2 grant-mint step: scope selected from principal.class.
Cross-spec touches in traffic-isolation.md (sync with auth.md):
* §4.2 sample helper code: BotAccountToken → BotSubjectName, dot-
normalization → suffix-strip.
* §4.3 mirror table: bot subjects use {account} stripped form;
admin row added to JWT-grant list (chat.> god-mode).
* §4.3 canonical row nudged to reflect Phase 1 shared canonical
(full pros/cons discussion lands in the follow-up commit).
* Cross-spec invariant updated to point at auth.md §4.7
(the new section number).
…s (Phase 1 = shared)
Phase 1 decision: keep MESSAGES_CANONICAL_{siteID} and ROOMS_{siteID}
shared across classes. Publisher namespace stays split per class
(chat.user vs chat.bot) — that's enforced by JWT grants and resolves
the ACL-escape problem. The downstream CANONICAL stream split is
deferred until measured noisy-neighbor pressure justifies it.
§4.4a captures the full trade-off so the team can revisit with data:
* PROS of SHARED (Phase 1): simpler infra (2 streams not 4), single
dashboards/alerts, single-Deployment workers, half the K8s objects,
trivial mixed-room handling, simpler federation, easier ordering
invariants, no canonical-subject rename needed.
* CONS of SHARED (the watch-list): no independent worker-pool scaling
per class, shared failure domain (poison-pill bot msg halts user
processing on the same pod), noisy-neighbor risk, metric/SLO
granularity is label-based rather than stream-based, cross-class
rate limiting lives in app code, HPA precision is coarser per-class.
* INVARIANTS preserved either way: publisher ACL isolation, subject
collision safety, per-class metric tagging (via class field on
payload), mixed-room correctness, federation correctness.
* SPLIT-LATER ESCAPE HATCH: documented triggers + the mechanic
(add chat.bot.canonical.… stream + rename existing to
chat.user.canonical.… via JetStream Mirror; non-breaking because
publisher namespace is already split).
* WHEN TO REVISIT: 30-day prod review; specific operational pain;
adding 6th+ canonical consumer.
Also nudges §4.3 mirror-table footnote + §4.4 bullet to point at
§4.4a as the source of truth for the canonical-split question.
Joey0538
force-pushed
the
claude/bot-platform-nextgen-migration-hb6ok2
branch
from
89f2b8e to
4694282
Compare
…-class worker split per §4.4a
Two related Phase-1 changes bundled together because they're a single
coherent stance ("Phase 1 = shared canonical stream, no per-class split
yet, bot publish stays sync-fail-fast"):
(1) §5.0 NEW — Bot publish edge = core NATS R/R
* Bots use REST end-to-end from the SDK's POV; bp-api translates to
core NATS request/reply (NOT JetStream js.Publish) to talk to
message-gatekeeper. Preserves legacy v2 REST fail-fast semantics.
* Per-RPC transport table:
- Query / lookup → core NATS R/R
- Mutation w/ single sync response → core NATS R/R
- Message publish (fan-out required) → core NATS R/R to gatekeeper;
gatekeeper synchronously js.Publish to MESSAGES_CANONICAL_{siteID}
and waits for PubAck before replying to bot.
* NO MESSAGES_BOT_{siteID} JetStream submit stream. The user-side
MESSAGES_{siteID} stream stays user-only; bots never publish into
JetStream at all.
* Failure modes spelled out (gatekeeper unreachable / validation
reject / canonical PubAck fail).
* User publish vs bot publish comparison table.
* Implementation impact on message-gatekeeper: existing JS pull-
consumer on MESSAGES_{siteID} unchanged + NEW core-NATS queue-
subscribe on chat.bot.*.room.*.{siteID}.msg.> for bot R/R path.
(2) §5.1 / §5.2 / §5.3 / §5.4 / §6 — Defer per-class worker split
* §5.1 message-gatekeeper: Phase 1 publishes to shared
MESSAGES_CANONICAL_{siteID} (single stream) instead of class-split
canonical. Per-class subject variant (chat.user.canonical /
chat.bot.canonical) deferred per §4.4a escape hatch.
* §5.2 message-worker: Phase 1 = single shared Deployment on the
shared canonical stream. Per-class -user/-bot deployment table
preserved as "Phase N+ when split" reference for the escape-hatch
implementation.
* §5.3 broadcast-worker: same treatment as §5.2.
* §5.4 worker binary changes: Phase 1 = nothing; class-aware
WORKER_CLASS env / filter code activates only when split triggers.
* §6 stream design: existing MESSAGES_CANONICAL_{siteID} kept as-is
in Phase 1; no rename, no per-class split. Original "separate
streams per class" rationale preserved as §6.1.1 future-design
reference for the escape-hatch implementation.
The Option A / SUBJECT-SPLIT architecture decision still stands at
the PUBLISHER namespace level (JWT grants enforce chat.user.> vs
chat.bot.> vs admin chat.>). Phase 1 just defers the DOWNSTREAM
stream / worker split as YAGNI until measured noisy-neighbor
pressure justifies the operational overhead — per §4.4a.
Joey0538
force-pushed
the
claude/bot-platform-nextgen-migration-hb6ok2
branch
2 times, most recently
from
7d935ca to
cf7061f
Compare
…a separate Mongo query
The §5.1 step 3 "provisioning gate" was being drawn and described as
if it were a second Mongo round-trip after the password verify. It's
not — the users doc loaded in step 2 already carries siteId, so the
gate is just:
if cfg.RequireProvisioned && u.SiteID != cfg.SiteID {
return errcode.Forbidden("account_not_provisioned")
}
Zero extra DB calls, zero extra latency on the login hot path. The
gate is a distinct LOGICAL security control (validates site
affinity, separate concern from credential check), but operationally
it's an O(1) field comparison on a struct already in memory.
Spec edits:
(1) auth.md §5.1 step 3 — rewritten to say "in-process field check,
NO extra Mongo query" explicitly. Shows the actual Go pattern.
(2) auth.md §1.5 (Critical Constraints) — earlier wording was
"the account's {userId, siteId} must exist in this site's
users collection", which read like a separate findOne. Replaced
with "the siteId field on the users doc (already loaded for the
password check) must match SITE_ID".
Diagram edits (all three login-flow diagrams):
(3) bot-login-flow.drawio — step 10/11 were drawn as a Mongo arrow
+ reply (PROVISIONING GATE — find {account, siteId} in users
+ found ✓). Replaced with a single red in-process activation box
on the botplatform-service lifeline ("PROVISIONING GATE
(in-process, no extra Mongo) — if u.siteId != SITE_ID → 403
account_not_provisioned"). Side-panel annotation and legend
updated accordingly. PNG re-rendered.
(4) login-old-vs-new.drawio — NEW panel had a dedicated orange
"Step 3 PROVISIONING GATE" box drawn as if it were a separate
operation. Removed it entirely and folded the field check into
Step 2's body ("if u.siteId != SITE_ID → 403 — field check on
the doc above, no extra Mongo query"). Steps 4/5/6 renumbered
to 3/4/5; the new-side column now has 5 steps, symmetric with
the legacy side. Token prefix also corrected bp1_ → bp_ (per
earlier rename in commit 0c47c7e; this diagram lagged). PNG
re-rendered.
(5) cross-cluster-cutover.drawio — Mongo cylinder caption said
"credentials + sessions + users / (provisioning gate reads
users)" — both stale and ambiguous (no credentials collection
after 2026-06-24 pivot; "reads users" implied a separate query).
Rewritten to "users (shared, password in-place) + sessions
(new, one doc per session) / Provisioning gate = in-process
field check on the user doc loaded for password verify (no
extra Mongo round-trip)". PNG re-rendered.
No semantic change to the gate's behavior or REQUIRE_PROVISIONED
default — just making explicit that it doesn't cost a round-trip,
correcting all three diagrams, and tightening the §1.5 wording.
Joey0538
force-pushed
the
claude/bot-platform-nextgen-migration-hb6ok2
branch
from
cf7061f to
d02d788
Compare
…ms (bp1_ → bp_; drop bp2_) Diagram catch-up for the C5 token-prefix decision (commit 0c47c7e) which renamed bp1_ → bp_ in auth.md text but the diagrams lagged. Also drops the bp2_ forward-versioning reference per C5's "unversioned prefix; we won't rotate the login mechanism, YAGNI" stance. Files touched (all PNGs re-rendered): * bot-login-flow.drawio — 4 spots: subtitle, step 12 token-issue box, step 15 response label, side-panel annotation (12). bp1_ → bp_ throughout. * token-gen-validate-flow.drawio — 5 spots: g_prefix box, g_response, g_coexist_body, va_v1 edge label, va_legacy_fb edge label. bp1_ → bp_ throughout. The g_coexist_body rationale bullet ("forward versioning bp2_…") is rewritten to reflect the per-class prefix decision instead: "class is self-evident from X-Auth-Token (bp_ = bot, ad_ = admin) — no DB lookup needed to know principal class". * login-old-vs-new.drawio — 2 stragglers (subtitle + n_wins_body bullet) the earlier bulk-rename missed because the reverted-by-linter dance landed bp1_ back into those strings. The wins-bullet "Versioned wire format (bp1_)" is also rewritten to "Per-class wire prefix (bp_ for bot, ad_ for admin)" to match the C5 rationale. No semantic change beyond what C5 already decided in the spec text; this just brings the diagrams into alignment so reviewers don't see bp1_ in pictures while the spec text says bp_.
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
3 participants
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.
Summary
Design spec set for the bot-platform-nextgen migration — password auth + durable sessions for bots/admins, an operational migration runbook, and a companion traffic-isolation spec. All under
docs/specs/botplatform/.Docs only. No code, no tests, no production behavior changes.
Reading order — start here
docs/specs/botplatform/README.mdlists every file with a 15-min / 45-min / 90-min reading path. If you're reviewing for ~15 min, that README walks you straight to:auth.mdPart I §1 — executive summaryauth.mdPart I §3 — architecture decision (Option B / DEDICATED-SERVICE)auth.mdPart I §10 — 4 diagrams (embedded inline)auth.mdPart I §9 — phased rolloutWhat's in this PR
docs/specs/botplatform/(new folder, all spec files namespaced here)README.mdauth.mdmigration-runbook.mdtraffic-isolation.mddiagrams/.drawiosources + rendered PNGs (embedded inauth.md§10)Diagrams (rendered inline in
auth.md§10)Tooling change
.claude/settings.jsonadds theAgents365-ai/365-skillsmarketplace (SHA-pinned) + enables thedrawioplugin team-wide — used to generate the diagrams; anyone working in this repo gets the tooling for free.Key design decisions baked in
botplatform-service(over EXTEND-AUTH). Bot edge is now a browser-facing web app + dual-token validation; isolating from auth-service's JWT signing wins on blast-radius/key safety.bp1_<43-char base64url of 32 random bytes>, stored asbase64(HMAC-SHA-256(TOKEN_HMAC_KEY, token)). Legacy unchanged for byte-for-byte compatibility. Validator dispatches onbp1_prefix.chat.bot.{botToken}.>;xxx.bot → xxx_botnormalization viasubject.BotAccountToken. Resolves PR #295 strict-validator conflict + eliminates ACL escape.REQUIRE_PROVISIONED=truedefault; mirrors auth-service from PR #295.Scope — original ticket vs delivered
Original ticket scope: bot login flow.
Delivered scope: auth migration spec + dependencies. Stretched because designing bot login surfaced four hard dependencies:
Out of scope (explicitly):
/admin/bots…surface here is bot-specific; a separate spec is needed for/admin/users…(writes to the shareduserscollection owned by PR feat: multi-site login via portal directory + provisioning-gated JWT minting #295 / portal-service team). Tracked inauth.mdPart I §11.auth.mdPart I §11.Companion spec (could be split out):
traffic-isolation.mdis a follow-on that consumes theclasssignal this design produces. It's genuinely separable if you'd prefer a smaller PR — let me know and I'll move it to a second PR.Commits — 5 logical units
docs(auth): bot-platform nextgen migration design spec (Parts I-III combined)auth.mddocs(traffic-isolation): bot traffic isolation design spec (Parts I-II combined)traffic-isolation.mddocs: schema + migration runbook (AS-IS vs TO-BE) + spec-set READMEmigration-runbook.md,README.mddocs: .drawio diagrams + rendered PNGs — 4 spec diagrams.drawio+ 4 ×.pngchore(claude-code): add 365-skills marketplace (SHA-pinned) + enable drawio plugin.claude/settings.jsonReview feedback addressed
18 CodeRabbit findings across two review rounds — all folded into the relevant logical commit:
Round 1 (11 findings): SHA pin on the marketplace, native JWT mint contradiction, NATS pseudocode contradicting "no NATS surface", "API proxy" in service summary, live-sync
$setrace, vacuousschemeOriginalassertion, siteId reconciliation, deployment-manifest example using a service that doesn't split,is_botsource phasing, MD040 code fences, MD058 table blank lines.Round 2 (7 findings): validate response schema in interfaces table, botplatform-server proxy ownership,
/admin/bots…route family (REST API → role-gated web handlers), token import allow-list (skip ANY non-empty type, not just PATs), source freeze / delta catch-up between bulk import and live-sync activation, gatekeeper milestone wording consistency, Mirror / dual-publish overlap withmessageIdidempotency contract.Test plan
Docs only. Manual review:
README.mdreading order is sensible and gets you oriented in 15 minauth.mdParts I/II/III are internally consistent (no decision contradicts another section)auth.md§10 diagrams render correctly in GitHub diff viewmigration-runbook.mdmigration pseudocode is idempotent + resumable; reconciliation queries are syntactically validtraffic-isolation.md§6.3 Mirror sequencing is unambiguous (no event-duplication window)auth.mdPart I §11 is clear (general user admin → separate spec)🤖 Generated with Claude Code
https://claude.ai/code/session_01ExuYDe9RCvUcDb5onMiM4K
Summary by CodeRabbit