반환 로직 수정#355
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
워크스루AWS Bedrock AgentRuntime의 invokeFlow 스트리밍 기반 호출 방식을 Bedrock Converse API의 동기 요청-응답 패턴으로 전환. Converse 클라이언트 및 요청/응답 처리 인프라 추가, 인터뷰/이력서 도메인별 클라이언트 추가, 기존 invokeFlow 로직 제거 및 서비스 통합. 변경 사항Bedrock Converse API 마이그레이션
시퀀스 다이어그램sequenceDiagram
participant InterviewService
participant ProceedBedrockClient
participant FeedbackBedrockClient
participant ConverseClient
participant Bedrock
InterviewService->>ProceedBedrockClient: requestToBedrock(qa)
ProceedBedrockClient->>ConverseClient: converse(system, messages, tools, maxTokens)
ConverseClient->>Bedrock: ConverseRequest
Bedrock-->>ConverseClient: ConverseResponse(stopReason=TOOL_USE, output=[toolUse])
ConverseClient->>ConverseClient: extractToolUse(response, toolName)
ConverseClient-->>ProceedBedrockClient: ConverseResponse with toolInput
ProceedBedrockClient-->>InterviewService: BedrockConverseResponse
InterviewService->>FeedbackBedrockClient: requestAnswerFeedback(qa, rank)
FeedbackBedrockClient->>ConverseClient: converse(system, messages, tools, maxTokens)
ConverseClient->>Bedrock: ConverseRequest
Bedrock-->>ConverseClient: ConverseResponse
ConverseClient-->>FeedbackBedrockClient: ConverseResponse with toolInput
FeedbackBedrockClient-->>InterviewService: feedback String
예상 코드 리뷰 노력🎯 4 (Complex) | ⏱️ ~60분 관련 PR
추천 리뷰어
시
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 기존의 Bedrock Flow API를 사용하던 면접 및 이력서 평가 로직을 Bedrock Converse API로 마이그레이션하는 작업을 포함합니다. 이를 통해 모델과의 직접적인 통신을 최적화하고, 연동 로직을 서비스별로 모듈화하여 유지보수성을 크게 향상시켰습니다. 또한, 설정 파일과 유틸리티 클래스를 추가하여 향후 모델 변경이나 설정 조정에 유연하게 대응할 수 있도록 구조를 개선했습니다. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Bedrock 흐름 떠나 Converse로 새 길 찾아 면접 더 밝게 Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request migrates the application from using AWS Bedrock Agents (Flows) to the AWS Bedrock Converse API. It introduces several new client classes, such as BedrockConverseClient, InterviewProceedBedrockClient, and ResumeEvaluationBedrockClient, to handle interactions with the Converse API and tool-use features. The build configuration was updated to remove the bedrockagentruntime dependency, and configuration properties were added for model parameters. Review feedback highlighted potential NullPointerException risks in BedrockConverseClient and InterviewProceedBedrockFlowAsyncService when accessing nested response objects, as well as an opportunity to deduplicate JSON mapping logic by moving it to a shared utility.
| "Bedrock 응답이 tool_use가 아닙니다. stopReason=" + response.stopReason() | ||
| + ", expected=" + expectedToolName); | ||
| } | ||
| return response.output().message().content().stream() |
| private static <T> T mapTo(Document document, Class<T> type, ObjectMapper objectMapper) { | ||
| try { | ||
| Object javaObject = DocumentJsonConverter.toJavaObject(document); | ||
| return objectMapper.convertValue(javaObject, type); | ||
| } catch (Exception e) { | ||
| throw new ExternalApiException("Bedrock toolUse 파싱 실패: input=" + document, e); | ||
| } | ||
| } |
| if (result.isInProgress()) { | ||
| requestAndSaveAnswerFeedbackAsync(questionAndAnswers, mdcContext, | ||
| result.getCurAnswer().getAnswerRank(), result.getCurAnswer().getId()); | ||
| } |
There was a problem hiding this comment.
result.getCurAnswer()가 null일 경우 getAnswerRank() 호출 시 NullPointerException이 발생할 수 있습니다. result.isInProgress() 체크와 함께 result.getCurAnswer()에 대한 null 체크를 추가하는 것이 안전합니다.
| if (result.isInProgress()) { | |
| requestAndSaveAnswerFeedbackAsync(questionAndAnswers, mdcContext, | |
| result.getCurAnswer().getAnswerRank(), result.getCurAnswer().getId()); | |
| } | |
| if (result.isInProgress() && result.getCurAnswer() != null) { | |
| requestAndSaveAnswerFeedbackAsync(questionAndAnswers, mdcContext, | |
| result.getCurAnswer().getAnswerRank(), result.getCurAnswer().getId()); | |
| } |
Test Results 51 files 51 suites 1m 32s ⏱️ Results for commit a8dc560. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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
`@src/main/java/com/samhap/kokomen/global/external/bedrock/BedrockConverseClient.java`:
- Around line 71-77: The catch block in parseToolInput(ToolUseBlock toolUse,
Class<T> type) currently embeds the full toolUse.input() into the
ExternalApiException message which may expose sensitive data; change the thrown
ExternalApiException to omit the raw input and instead include only a
non-sensitive identifier (e.g., toolUse.name() or toolUse.id() if available) and
the original exception as the cause (keep throw new ExternalApiException(...,
e)); ensure DocumentJsonConverter and objectMapper usage is unchanged and only
the exception message is sanitized.
In
`@src/main/java/com/samhap/kokomen/global/external/bedrock/BedrockConverseProperties.java`:
- Around line 5-14: Add bean validation to the BedrockConverseProperties record
so invalid config fails at startup: annotate the record with `@Validated` and add
jakarta.validation constraints to fields — mark modelId as `@NotBlank`, mark
proceedMaxTokens, endMaxTokens, answerFeedbackMaxTokens,
resumeQuestionMaxTokens, resumeEvaluationMaxTokens as `@NotNull` and
`@PositiveOrZero` (or `@Min`(0)), and constrain temperature with `@NotNull` and
`@DecimalMin`("0.0") `@DecimalMax`("1.0"); ensure imports use jakarta.validation.*
and that the class remains annotated with `@ConfigurationProperties`(prefix =
"aws.bedrock") so Spring validates the bound values on startup.
In
`@src/main/java/com/samhap/kokomen/global/external/bedrock/DocumentJsonConverter.java`:
- Line 35: In DocumentJsonConverter replace the plain IllegalStateException
thrown for unknown Document types with the project's custom exception (e.g.,
ExternalApiException) so exception hierarchy stays consistent; update the throw
in the method inside class DocumentJsonConverter to construct and throw
ExternalApiException (preserving the current message "알 수 없는 Document 타입입니다: " +
document and, if available, include the original cause or document details)
instead of IllegalStateException.
In
`@src/main/java/com/samhap/kokomen/interview/external/dto/response/BedrockConverseResponse.java`:
- Line 37: The throw in BedrockConverseResponse currently includes the raw
`document` (toolInput) in the ExternalApiException message which may expose
sensitive interview content; change the code to avoid embedding the full
`document` string—either log a redacted/trimmed form, its length, or a hash/ID
for diagnostics and include that in the ExternalApiException message instead
(keep the original exception `e` as the cause) and ensure any debug logging that
records `document` is likewise redacted; update the throw site that constructs
ExternalApiException to use the sanitized token rather than the raw `document`.
In
`@src/main/java/com/samhap/kokomen/interview/external/ResumeBasedQuestionBedrockService.java`:
- Line 59: The error log currently writes the full LLM response variable
jsonResponse on parse failures in ResumeBasedQuestionBedrockService; remove
sensitive content from logs by not including jsonResponse (or any raw
resume/portfolio text) in log.error calls inside the parsing error handler
(e.g., the block where log.error("GPT 질문 응답 파싱 실패: {}", jsonResponse, e) is
called). Instead log a non-sensitive message plus the exception (and if needed a
safe identifier such as a truncated hash or boolean flag) so the code in the
method that parses the LLM response no longer emits raw jsonResponse to logs.
In
`@src/main/java/com/samhap/kokomen/interview/service/infra/InterviewProceedBedrockFlowAsyncService.java`:
- Around line 206-217: The executor.execute(...) call can throw (e.g.,
RejectedExecutionException) and propagate up to processBedrockProceed causing
the whole flow to fail; wrap the executor.execute submission in a try-catch
around the call itself (not just inside the runnable) to catch submission
failures, log the rejection with curAnswerId and mdcContext, and avoid
rethrowing so Bedrock success isn't treated as overall failure—keep the existing
inner runnable handling for runtime errors (setMdcContext,
answerFeedbackBedrockClient.requestAnswerFeedback,
interviewProceedService.saveAnswerFeedback) but ensure executor.execute errors
are handled locally to prevent LLM_FAILED/GPT fallback from being triggered by
task submission failures.
- Around line 72-74: Wrap the executor.execute(...) call in a try/catch for
java.util.concurrent.RejectedExecutionException and in the catch release the
interview lock using the existing lock release mechanism (invoke the same logic
that frees lockKey with lockValue and updates interviewProceedStateKey / related
state) so the lock is always cleared if the task cannot be scheduled; reference
symbols: executor.execute, processBedrockProceed, lockKey, lockValue,
interviewProceedStateKey, RejectedExecutionException.
In
`@src/main/java/com/samhap/kokomen/interview/tool/InterviewBedrockSystemMessageConstant.java`:
- Line 68: The end/termination prompt in InterviewBedrockSystemMessageConstant
currently uses a different rank mapping ("랭크 매핑 : A(4-6점) / B(3점) / C(2점) /
D(1점) / F(0점") than the 진행 프롬프트, causing inconsistent scoring; open the
InterviewBedrockSystemMessageConstant class, find the end/termination prompt
string literal (the constant holding the 종료/종료 프롬프트 text) and change its rank
mapping to match the 진행 프롬프트 mapping exactly (copy the same A/B/C/D/F ranges
from the 진행 프롬프트 constant or variable) so both prompts use identical
score-to-rank rules.
In
`@src/main/java/com/samhap/kokomen/resume/external/dto/ResumeBedrockRequestFactory.java`:
- Around line 31-47: In createQuestionGenerationMessages (and the other similar
methods around lines 88-114) you are inserting raw user input into an XML-like
template which can break tag boundaries; fix this by escaping XML special
characters before formatting: add or use a helper (e.g., escapeXml or xmlEscape)
and call it on resumeText, portfolioText, and jobCareer (instead of passing
nullToEmpty(...) directly), ensuring the formatted userText uses the escaped
values; update any other message-creation methods in ResumeBedrockRequestFactory
to apply the same escape function to their inputs.
In
`@src/main/java/com/samhap/kokomen/resume/service/ResumeEvaluationAsyncService.java`:
- Line 289: The current log in ResumeEvaluationAsyncService (log.error("GPT 이력서
평가 응답 파싱 실패: {}", jsonResponse, e)) must not print the full GPT response; change
it to log only non-sensitive metadata (e.g., response length via
jsonResponse.length(), a deterministic short fingerprint/hash of jsonResponse
such as SHA-256 truncated to 8-12 hex chars, and/or an internal correlation id)
along with the exception e and a clear message; remove or redact the raw
jsonResponse from the error message while keeping the exception stacktrace for
troubleshooting.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97956e12-800f-426f-b3b5-0c36c81cc991
📒 Files selected for processing (26)
build.gradlegradle/gradle-daemon-jvm.propertiessrc/main/java/com/samhap/kokomen/global/config/AwsConfig.javasrc/main/java/com/samhap/kokomen/global/external/bedrock/BedrockConverseClient.javasrc/main/java/com/samhap/kokomen/global/external/bedrock/BedrockConverseProperties.javasrc/main/java/com/samhap/kokomen/global/external/bedrock/DocumentJsonConverter.javasrc/main/java/com/samhap/kokomen/interview/external/AnswerFeedbackBedrockClient.javasrc/main/java/com/samhap/kokomen/interview/external/InterviewProceedBedrockClient.javasrc/main/java/com/samhap/kokomen/interview/external/ResumeBasedQuestionBedrockService.javasrc/main/java/com/samhap/kokomen/interview/external/dto/request/InterviewBedrockRequestFactory.javasrc/main/java/com/samhap/kokomen/interview/external/dto/request/InterviewInvokeFlowRequestFactory.javasrc/main/java/com/samhap/kokomen/interview/external/dto/response/AnswerFeedbackOnlyResponse.javasrc/main/java/com/samhap/kokomen/interview/external/dto/response/BedrockConverseResponse.javasrc/main/java/com/samhap/kokomen/interview/external/dto/response/BedrockResponse.javasrc/main/java/com/samhap/kokomen/interview/service/infra/InterviewProceedBedrockFlowAsyncService.javasrc/main/java/com/samhap/kokomen/interview/tool/InterviewBedrockSystemMessageConstant.javasrc/main/java/com/samhap/kokomen/interview/tool/InterviewMessagesFactory.javasrc/main/java/com/samhap/kokomen/resume/external/ResumeBasedQuestionBedrockClient.javasrc/main/java/com/samhap/kokomen/resume/external/ResumeEvaluationBedrockClient.javasrc/main/java/com/samhap/kokomen/resume/external/ResumeInvokeFlowRequestFactory.javasrc/main/java/com/samhap/kokomen/resume/external/dto/ResumeBedrockRequestFactory.javasrc/main/java/com/samhap/kokomen/resume/external/dto/ResumeBedrockSystemMessageConstant.javasrc/main/java/com/samhap/kokomen/resume/service/ResumeEvaluationAsyncService.javasrc/main/resources/application.ymlsrc/test/java/com/samhap/kokomen/global/BaseTest.javasrc/test/java/com/samhap/kokomen/global/fixture/interview/BedrockResponseFixtureBuilder.java
💤 Files with no reviewable changes (5)
- build.gradle
- src/main/java/com/samhap/kokomen/resume/external/ResumeInvokeFlowRequestFactory.java
- src/main/java/com/samhap/kokomen/interview/external/dto/request/InterviewInvokeFlowRequestFactory.java
- src/main/java/com/samhap/kokomen/interview/external/dto/response/BedrockResponse.java
- src/main/java/com/samhap/kokomen/interview/tool/InterviewMessagesFactory.java
| @ConfigurationProperties(prefix = "aws.bedrock") | ||
| public record BedrockConverseProperties( | ||
| String modelId, | ||
| Integer proceedMaxTokens, | ||
| Integer endMaxTokens, | ||
| Integer answerFeedbackMaxTokens, | ||
| Integer resumeQuestionMaxTokens, | ||
| Integer resumeEvaluationMaxTokens, | ||
| Float temperature | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# validation 관련 의존성/사용 여부를 읽기 전용으로 확인
fd -i "build.gradle*" | xargs -I{} rg -n "spring-boot-starter-validation|jakarta.validation|hibernate-validator" {}
rg -n "`@ConfigurationProperties`|`@Validated`|jakarta.validation.constraints"Repository: samhap-soft/kokomen-backend
Length of output: 3433
🏁 Script executed:
# Search for other `@ConfigurationProperties` usage to see if they use validation
rg -A 10 "`@ConfigurationProperties`" --type java | head -60Repository: samhap-soft/kokomen-backend
Length of output: 1374
🏁 Script executed:
cat -n src/main/java/com/samhap/kokomen/global/external/bedrock/BedrockConverseProperties.javaRepository: samhap-soft/kokomen-backend
Length of output: 641
Bedrock 설정값에 바인딩 단계 검증을 추가하세요.
현재는 modelId 누락, 토큰 음수, temperature 범위 오류가 런타임 호출 시점까지 지연됩니다. 시작 단계에서 실패하도록 제약을 넣는 게 안전합니다. 프로젝트에 spring-boot-starter-validation 의존성이 있으며, 다른 DTO 클래스들에서도 jakarta.validation.constraints를 활발하게 사용하고 있습니다.
🔧 제안 수정안
import org.springframework.boot.context.properties.ConfigurationProperties;
+import org.springframework.validation.annotation.Validated;
+import jakarta.validation.constraints.Max;
+import jakarta.validation.constraints.Min;
+import jakarta.validation.constraints.NotBlank;
+import jakarta.validation.constraints.NotNull;
+import jakarta.validation.constraints.Positive;
+@Validated
`@ConfigurationProperties`(prefix = "aws.bedrock")
public record BedrockConverseProperties(
- String modelId,
- Integer proceedMaxTokens,
- Integer endMaxTokens,
- Integer answerFeedbackMaxTokens,
- Integer resumeQuestionMaxTokens,
- Integer resumeEvaluationMaxTokens,
- Float temperature
+ `@NotBlank` String modelId,
+ `@NotNull` `@Positive` Integer proceedMaxTokens,
+ `@NotNull` `@Positive` Integer endMaxTokens,
+ `@NotNull` `@Positive` Integer answerFeedbackMaxTokens,
+ `@NotNull` `@Positive` Integer resumeQuestionMaxTokens,
+ `@NotNull` `@Positive` Integer resumeEvaluationMaxTokens,
+ `@NotNull` `@Min`(0) `@Max`(1) Float temperature
) {
}🤖 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
`@src/main/java/com/samhap/kokomen/global/external/bedrock/BedrockConverseProperties.java`
around lines 5 - 14, Add bean validation to the BedrockConverseProperties record
so invalid config fails at startup: annotate the record with `@Validated` and add
jakarta.validation constraints to fields — mark modelId as `@NotBlank`, mark
proceedMaxTokens, endMaxTokens, answerFeedbackMaxTokens,
resumeQuestionMaxTokens, resumeEvaluationMaxTokens as `@NotNull` and
`@PositiveOrZero` (or `@Min`(0)), and constrain temperature with `@NotNull` and
`@DecimalMin`("0.0") `@DecimalMax`("1.0"); ensure imports use jakarta.validation.*
and that the class remains annotated with `@ConfigurationProperties`(prefix =
"aws.bedrock") so Spring validates the bound values on startup.
| public static List<Message> createQuestionGenerationMessages(String resumeText, String portfolioText, String jobCareer) { | ||
| String userText = """ | ||
| <resume> | ||
| %s | ||
| </resume> | ||
|
|
||
| <portfolio> | ||
| %s | ||
| </portfolio> | ||
|
|
||
| <job_career> | ||
| %s | ||
| </job_career> | ||
| """.formatted( | ||
| nullToEmpty(resumeText), | ||
| nullToEmpty(portfolioText), | ||
| nullToEmpty(jobCareer)); |
There was a problem hiding this comment.
사용자 입력 XML 이스케이프 처리 필요
사용자 입력을 XML 유사 포맷에 그대로 삽입하고 있어 태그 경계가 깨지거나 프롬프트 구조가 오염될 수 있습니다. 입력값은 최소한 XML 특수문자를 이스케이프한 뒤 삽입해 주세요.
수정 예시
@@
- String userText = """
+ String userText = """
<resume>
%s
</resume>
@@
- nullToEmpty(resumeText),
- nullToEmpty(portfolioText),
- nullToEmpty(jobCareer));
+ escapeXml(nullToEmpty(resumeText)),
+ escapeXml(nullToEmpty(portfolioText)),
+ escapeXml(nullToEmpty(jobCareer)));
@@
- String userText = """
+ String userText = """
<resume>
%s
</resume>
@@
- nullToEmpty(request.resume()),
- nullToEmpty(request.portfolio()),
- nullToEmpty(request.jobPosition()),
- nullToEmpty(request.jobDescription()),
- nullToEmpty(request.jobCareer()));
+ escapeXml(nullToEmpty(request.resume())),
+ escapeXml(nullToEmpty(request.portfolio())),
+ escapeXml(nullToEmpty(request.jobPosition())),
+ escapeXml(nullToEmpty(request.jobDescription())),
+ escapeXml(nullToEmpty(request.jobCareer())));
@@
private static String nullToEmpty(String value) {
return value != null ? value : "";
}
+
+ private static String escapeXml(String value) {
+ return value
+ .replace("&", "&")
+ .replace("<", "<")
+ .replace(">", ">")
+ .replace("\"", """)
+ .replace("'", "'");
+ }
}Also applies to: 88-114
🤖 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
`@src/main/java/com/samhap/kokomen/resume/external/dto/ResumeBedrockRequestFactory.java`
around lines 31 - 47, In createQuestionGenerationMessages (and the other similar
methods around lines 88-114) you are inserting raw user input into an XML-like
template which can break tag boundaries; fix this by escaping XML special
characters before formatting: add or use a helper (e.g., escapeXml or xmlEscape)
and call it on resumeText, portfolioText, and jobCareer (instead of passing
nullToEmpty(...) directly), ensuring the formatted userText uses the escaped
values; update any other message-creation methods in ResumeBedrockRequestFactory
to apply the same escape function to their inputs.
closed #
작업 내용
스크린샷
참고 사항