[REF-1474] fix: make PTC tool call IDs Bedrock-safe#2262
Merged
Conversation
📝 WalkthroughWalkthroughUpdate introduces a Bedrock-safe PTC tool call ID generator that uses an underscore separator, switches the tool execution service to use it, and adjusts DTO detection to accept both legacy Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant PTCService as PTC Execute Service
participant IDGen as Tool Call ID Generator
participant DB as Persistence
participant Replay as Replay/History
Client->>PTCService: request execute PTC
PTCService->>IDGen: generateToolExecutionCallId('ptc')
IDGen-->>PTCService: returns "ptc_<uuid>"
PTCService->>DB: persist tool_call_result/action_message with toolCallId
DB-->>Replay: store record (legacy ":" IDs remain unchanged)
Replay->>PTCService: replay history (no normalization)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Deploying refly-branch-test with
|
| Latest commit: |
998578f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8ccbf22a.refly-branch-test.pages.dev |
| Branch Preview URL: | https://ptc-tool-call-id-pr.refly-branch-test.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/src/modules/tool/ptc/tool-call-id.ts (1)
5-6: Add JSDoc for the exported generator function.A short JSDoc block will document the Bedrock-safe format contract for callers.
📝 Proposed update
+/** + * Generates a Bedrock-safe tool execution call ID in the format `{callType}_{uuid}`. + */ export const generateToolExecutionCallId = (callType: ToolExecutionCallType): string => { return `${callType}_${randomUUID()}`; };As per coding guidelines,
**/*.{js,ts,jsx,tsx}: Use JSDoc style comments for functions and classes in JavaScript/TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/tool/ptc/tool-call-id.ts` around lines 5 - 6, Add a JSDoc block above the exported function generateToolExecutionCallId describing its purpose, parameters, return value, and the Bedrock-safe format contract; include a one-line summary, `@param` for callType (type ToolExecutionCallType) explaining allowed values, and `@returns` noting it returns a string in the format "<callType>_<UUID>" (UUID generated via randomUUID) so callers know the exact format and safety guarantees.apps/api/src/modules/action/action.dto.ts (1)
49-49: Extract PTC prefixes into a shared constant/helper.Line 49 hardcodes
'ptc:'and'ptc_'. Centralizing these avoids format drift between ID generation and detection.♻️ Proposed refactor
+const PTC_TOOL_CALL_PREFIXES = ['ptc:', 'ptc_'] as const; + export function actionMessagePO2DTO(message: ActionMessageModel): ActionMessage { - const isPtc = message.toolCallId?.startsWith('ptc:') || message.toolCallId?.startsWith('ptc_'); + const isPtc = PTC_TOOL_CALL_PREFIXES.some( + (prefix) => message.toolCallId?.startsWith(prefix) ?? false, + ); return {As per coding guidelines,
Avoid magic numbers and strings - use named constants in TypeScript/JavaScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/action/action.dto.ts` at line 49, The code in action.dto.ts computes isPtc using hardcoded strings ('ptc:' and 'ptc_')—extract these into a shared constant or helper to avoid magic strings and format drift: create a named constant (e.g., PTC_PREFIXES or PTC_PREFIX_SET) or a helper function (e.g., isPtcId(toolCallId: string)) in a common utils/constants module, replace the inline startsWith checks that reference message.toolCallId and the local isPtc variable with a call to that helper or a check against the shared constant, and update any other places that generate or validate PTC IDs to use the same constant/helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/change/20260302-bedrock-compatible-ptc-tool-call-id/spec.md`:
- Line 59: The spec currently contradicts itself about legacy replay
normalization: the "two-layer fix" phrase referencing "replay-time
normalization" conflicts with later statements that "normalization is not
implemented." Choose one consistent decision and update all mentions
accordingly—either (A) state that source-format correction is applied and
replay-time normalization for legacy IDs will be implemented (replace "not
implemented" language in the sections that reference Lines 73 and 117-118 with a
plan and brief implementation note), or (B) state that only source-format
correction is applied and explicitly remove/replace any claims of replay-time
normalization (remove "two-layer fix" wording and update the lines mentioning
normalization). Locate and edit the phrases "two-layer fix", "replay-time
normalization", and "legacy replay normalization" so the spec consistently
reflects the chosen approach across the sections that currently conflict.
---
Nitpick comments:
In `@apps/api/src/modules/action/action.dto.ts`:
- Line 49: The code in action.dto.ts computes isPtc using hardcoded strings
('ptc:' and 'ptc_')—extract these into a shared constant or helper to avoid
magic strings and format drift: create a named constant (e.g., PTC_PREFIXES or
PTC_PREFIX_SET) or a helper function (e.g., isPtcId(toolCallId: string)) in a
common utils/constants module, replace the inline startsWith checks that
reference message.toolCallId and the local isPtc variable with a call to that
helper or a check against the shared constant, and update any other places that
generate or validate PTC IDs to use the same constant/helper.
In `@apps/api/src/modules/tool/ptc/tool-call-id.ts`:
- Around line 5-6: Add a JSDoc block above the exported function
generateToolExecutionCallId describing its purpose, parameters, return value,
and the Bedrock-safe format contract; include a one-line summary, `@param` for
callType (type ToolExecutionCallType) explaining allowed values, and `@returns`
noting it returns a string in the format "<callType>_<UUID>" (UUID generated via
randomUUID) so callers know the exact format and safety guarantees.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/modules/action/action.dto.tsapps/api/src/modules/tool/ptc/tool-call-id.tsapps/api/src/modules/tool/ptc/tool-execution.service.tsspecs/change/20260302-bedrock-compatible-ptc-tool-call-id/spec.md
- change PTC call ID generation from colon format to underscore format for Bedrock compatibility\n- centralize call ID creation in a shared helper used by tool execution\n- keep action message PTC detection compatible with both legacy ptc: and new ptc_ IDs\n- add implemented spec document for REF-1474
nettee
force-pushed
the
ptc-tool-call-id-pr
branch
from
339b404 to
998578f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
specs/change/20260302-bedrock-compatible-ptc-tool-call-id/spec.md (1)
73-73: Consider making external references self-contained.Lines 73 and 117 reference external context ("per approval" and "based on implementation checkpoint decision") that isn't documented within the spec. While not incorrect, making these self-contained would improve clarity for future readers.
📝 Optional wording improvements for self-containment
-2. Keep replay behavior unchanged (per approval: no legacy ID normalization). +2. Keep replay behavior unchanged (no legacy ID normalization to avoid complexity).- - Legacy historical IDs are **not** normalized at replay time, based on implementation checkpoint decision. + - Legacy historical IDs are **not** normalized at replay time (deferred to reduce implementation scope).Also applies to: 117-117
Also applies to: 117-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/change/20260302-bedrock-compatible-ptc-tool-call-id/spec.md` at line 73, The spec references external context in the phrases "Keep replay behavior unchanged (per approval: no legacy ID normalization)" and "based on implementation checkpoint decision"; replace those parenthetical notes with self-contained statements that either summarize the decision and its rationale or link to an internal appendix section in this document (e.g., add a short explanation like "approved by the design committee on [date]: replay retains legacy IDs; no normalization" or "per implementation checkpoint: decision to retain legacy ID behavior for compatibility") so readers can understand the constraint without external context and add an internal reference section if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@specs/change/20260302-bedrock-compatible-ptc-tool-call-id/spec.md`:
- Line 73: The spec references external context in the phrases "Keep replay
behavior unchanged (per approval: no legacy ID normalization)" and "based on
implementation checkpoint decision"; replace those parenthetical notes with
self-contained statements that either summarize the decision and its rationale
or link to an internal appendix section in this document (e.g., add a short
explanation like "approved by the design committee on [date]: replay retains
legacy IDs; no normalization" or "per implementation checkpoint: decision to
retain legacy ID behavior for compatibility") so readers can understand the
constraint without external context and add an internal reference section if
needed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/modules/action/action.dto.tsapps/api/src/modules/tool/ptc/tool-call-id.tsapps/api/src/modules/tool/ptc/tool-execution.service.tsspecs/change/20260302-bedrock-compatible-ptc-tool-call-id/spec.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api/src/modules/tool/ptc/tool-call-id.ts
- apps/api/src/modules/tool/ptc/tool-execution.service.ts
- apps/api/src/modules/action/action.dto.ts
alchemistklk
approved these changes
Mar 3, 2026
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
2 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
This PR fixes Bedrock compatibility issues caused by PTC tool call IDs containing
:. New PTC IDs now use a Bedrock-safe format while preserving compatibility for legacy records.Changes
type:uuidtotype_uuidptcandstandaloneptc:and newptc_prefixesREF-1474Summary by CodeRabbit
Bug Fixes
New Features
Documentation