Feat/insight analysis by hanxu-111 · Pull Request #191 · coze-dev/coze-loop

hanxu-111 · 2025-09-17T10:43:39Z

What type of PR is this?

Check the PR title.

This PR title match the format: [<type>][<scope>]: <description>. For example: [fix][backend] flaky fix
The description of this PR title is user-oriented and clear enough for others to understand.
Add documentation if the current PR requires user awareness at the usage level.
This PR is written in English. PRs not in English will not be reviewed.

(Optional) Translate the PR title into Chinese.

(Optional) More detailed description for this PR(en: English/zh: Chinese).

en:
zh(optional):

(Optional) Which issue(s) this PR fixes:

CLAassistant · 2025-09-17T10:43:46Z

All committers have signed the CLA.

codecov · 2025-09-17T12:32:20Z

Codecov Report

❌ Patch coverage is 91.10106% with 59 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...evaluation/infra/mq/rocket/consumer/expt_export.go	0.00%	17 Missing ⚠️
...ra/repo/experiment/expt_insight_analysis_record.go	84.33%	11 Missing and 2 partials ⚠️
...d/modules/evaluation/application/experiment_app.go	95.23%	6 Missing and 3 partials ⚠️
...ules/evaluation/domain/service/expt_export_impl.go	74.28%	6 Missing and 3 partials ⚠️
...evaluation/domain/service/insight_analysis_impl.go	97.57%	4 Missing and 2 partials ⚠️
...ackend/modules/evaluation/infra/rpc/agent/agent.go	66.66%	2 Missing ⚠️
...kend/modules/evaluation/infra/rpc/notify/notify.go	50.00%	2 Missing ⚠️
...es/evaluation/infra/mq/rocket/consumer/consumer.go	0.00%	1 Missing ⚠️

@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
+ Coverage   58.80%   61.56%   +2.75%     
==========================================
  Files         512      470      -42     
  Lines       53303    49972    -3331     
==========================================
- Hits        31345    30763     -582     
+ Misses      19576    16939    -2637     
+ Partials     2382     2270     -112

Flag	Coverage Δ
unittests	`61.56% <91.10%> (+2.75%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...tion/convertor/experiment/expt_insight_analysis.go	`100.00% <100.00%> (ø)`
backend/modules/evaluation/domain/entity/event.go	`0.00% <ø> (ø)`
...es/evaluation/infra/mq/rocket/consumer/consumer.go	`0.00% <0.00%> (ø)`
...ackend/modules/evaluation/infra/rpc/agent/agent.go	`66.66% <66.66%> (ø)`
...kend/modules/evaluation/infra/rpc/notify/notify.go	`50.00% <50.00%> (ø)`
...evaluation/domain/service/insight_analysis_impl.go	`97.57% <97.57%> (ø)`
...d/modules/evaluation/application/experiment_app.go	`74.09% <95.23%> (+4.35%)`	⬆️
...ules/evaluation/domain/service/expt_export_impl.go	`75.04% <74.28%> (+7.62%)`	⬆️
...ra/repo/experiment/expt_insight_analysis_record.go	`84.33% <84.33%> (ø)`
...evaluation/infra/mq/rocket/consumer/expt_export.go	`0.00% <0.00%> (ø)`

... and 50 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 397e3a9...24d5036. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

CozeLoop

Code Review Summary

I've conducted a systematic expert-level code review of this insight analysis feature PR. The implementation introduces significant new functionality but contains several critical issues that must be addressed before merging.

Critical Issues Found (🚫Must-Fix):

Error handling bug in defer function - Silently swallows errors
Null pointer dereference risk - Inconsistent session handling
Logical error in null checks - Will cause runtime panics
Incorrect SQL column name - Delete operations will fail

Medium Priority Issues (🚨Should-Fix):

Stub implementation - Agent adapter returns hardcoded values
Inconsistent error handling - Message consumer retry logic

These issues could cause production failures, data inconsistencies, and poor user experience. Please address the critical issues before requesting another review.

Positive Aspects:

Well-structured domain-driven design
Comprehensive test coverage for new functionality
Proper separation of concerns across layers
Good use of Go interfaces and dependency injection

The overall architecture and design approach is solid, but the implementation details need refinement to meet production quality standards.

CozeLoop

Summary of key findings:

Must-Fix:

Voting toggle path (Upvote/Downvote) unconditionally inserts — violates UNIQUE KEY on toggles; implement idempotent upsert or update-if-exists.
SQL: expt_insight_analysis_feedback_vote.sql missing trailing semicolon and uses non-standard "Unique KEY" casing.
DB migrations only added for Docker Compose; mirror the same DDLs under Helm init-sql for k8s.

Should-Fix:
4) ListExptInsightAnalysisRecord response lacks BaseResp and uses a misleading field name (should be expt_insight_analysis_records, requires IDL/code regen).
5) Event naming: CreateAt -> CreatedAt to align with conventions.

Suggestion:
6) Avoid logging full userInfos/userInfo (PII); lower to debug or log only identifiers.

See inline comments for precise locations and code examples.

CozeLoop

Summary: Thank you for the substantial feature work on Insight Analysis. I’ve focused the review on correctness, security, and operational parity. I filed several concrete issues with code-level suggestions:

Must-Fix

Prevent persisting AnalysisReportID when it’s zero to avoid invalid polling
IDL field naming: use snake_case (feedback_action_type) per spec
Kubernetes Helm SQL parity for the three new tables

Should-Fix

Avoid logging pre-signed URLs (two locations)
Don’t unconditionally route read paths to write-DB; gate on consistency needs

Suggestion

Add ORDER BY to feedback comment listing for stable pagination

Once these are addressed, the change looks solid to move forward. Thanks!

CozeLoop

Summary: Insight Analysis feature is a solid direction and wires most components end-to-end. I found several correctness and consistency issues that should be addressed before merge.

Must-Fix:

Avoid persisting AnalysisReportID when reportID == 0 (invalid polling state). See inline.
Thrift field name should be snake_case (feedback_action_type) per IDL规范. See inline.
Add Helm init-sql parity for the three new insight tables to avoid K8s drift. See inline.

Should-Fix:

Don’t log full pre-signed URLs; redact query strings. See inline.
Don’t unconditionally route read paths to write DB for column annotations. See inline.
RPC adapter is a stub; either implement, guard with feature flag, or return a typed errno, and fix GetReport error message.

Suggestions:

Add ORDER BY to feedback comments listing for stable pagination.

Once these are addressed, I’m happy to re-review quickly.

CozeLoop

Verification: actions executed on PR #191 — inline comments and CHANGES_REQUESTED posted

Hi,

This is a quick proof-of-action note to confirm that a full code review has been executed and submitted on this PR.

What was done:

Submitted two reviews with status CHANGES_REQUESTED (latest at 2025-09-18T12:07:27Z, commit cb47d50)
Posted multiple inline comments on changed lines (Side=RIGHT), including (non-exhaustive):
- backend/modules/evaluation/domain/service/insight_analysis_impl.go:97 — [Must-Fix] avoid persisting AnalysisReportID when reportID==0
- idl/thrift/coze/loop/evaluation/coze.loop.evaluation.expt.thrift:519 — [Must-Fix] field name must be snake_case
- release/deployment/docker-compose/bootstrap/mysql-init/init-sql/expt_insight_analysis_record.sql:1 — [Must-Fix] add Helm init-sql parity
- backend/modules/evaluation/domain/service/expt_result_impl.go:622 — [Should-Fix] don’t force read path to primary
- backend/modules/evaluation/infra/rpc/agent/agent.go:25 — [Should-Fix] stub impl / error message
- backend/modules/evaluation/infra/repo/experiment/mysql/expt_insight_analysis_feedback_comment.go:86-93 — [Suggestion] add ORDER BY for stable pagination

How to view:

Conversation tab: see timeline entries “CozeLoop requested changes” (most recent 12:07:27Z)
Files changed tab: ensure diff shows all changes (no outdated-only filter); inline comments appear on the RIGHT side of modified lines

Full CR report:

Feishu document (internal): https://bytedance.larkoffice.com/docx/AhamdrpGsoEmtKxtO3icqqKanzb

If anything still doesn’t show on your side, please refresh the PR page and ensure you’re viewing the latest head (cb47d50…). Happy to re-review quickly after fixes are pushed.

CozeLoop

[Ping Verification #2] Action executed — posting this comment to demonstrate live activity on PR #191.

Summary:

A full review with CHANGES_REQUESTED was submitted previously (with multiple inline comments on changed lines).
This additional note is posted to help you verify that actions are indeed being executed and visible in the Conversation timeline.

How to view:

Conversation tab: look for entries from user “CozeLoop” (this comment appears at the top).
Files changed tab: show all diff; inline comments are on the RIGHT side of modified lines.

If you still cannot see any of these, please refresh the page and ensure you’re viewing the latest head commit (cb47d50…).

CozeLoop

Summary: Good foundational design for Insight Analysis (record lifecycle, CSV export reuse, async status check). However, two correctness issues must be addressed before merge:

Voting flow uses unconditional insert for Upvote/Downvote and will violate the unique constraint on repeated actions—implement upsert/update semantics.
Update/Delete comment paths don’t validate comment_id, which can lead to unintended operations on id=0.

Additional improvements: use read-level auth for list APIs; ensure notification recipient is a stable user identifier.

hanxu-111 added 28 commits September 17, 2025 19:05

feat:expt insight analysis

fb2e393

update

24d1e5b

update

068eca9

update

b823d20

update

a097daf

update

93d5283

update

6c73f93

update

c0485e2

update

cadda2a

update

c245d00

update

7013cb8

update

837a85d

update

f2fdd79

update

7b910e6

update

96fcc6e

update

b07bbdd

update

9c9fdb7

update

588bb16

update

06b52ad

update

1915ff9

update

c30431f

update

f7d397a

update

2dce0ce

update

734b7d5

update

391ccde

update

7ecd93c

update

cc9d2a7

update

62c5f90

hanxu-111 force-pushed the feat/insight_analysis branch from 72e0d81 to 62c5f90 Compare September 17, 2025 11:09

update

f2f9b3a

hanxu-111 added 6 commits September 17, 2025 22:13

update

940ebcf

update

f15d16c

update

6696669

update

9cfe4f7

update

7f6d91e

update

06942cd

HymanShi reviewed Sep 18, 2025

View reviewed changes

Comment thread backend/modules/evaluation/domain/component/rpc/notify.go Outdated

Comment thread backend/modules/evaluation/domain/service/insight_analysis_impl.go Outdated

Comment thread backend/modules/evaluation/domain/service/insight_analysis_impl.go Outdated

hanxu-111 added 2 commits September 18, 2025 14:53

update

18809e5

update

c413cb2

CozeLoop suggested changes Sep 18, 2025

View reviewed changes

update

de9020c

CozeLoop suggested changes Sep 18, 2025

View reviewed changes

update

cb47d50

CozeLoop suggested changes Sep 18, 2025

View reviewed changes

CozeLoop reviewed Sep 18, 2025

View reviewed changes

hanxu-111 added 2 commits September 18, 2025 20:29

update

dc4e638

update

27c16ec

CozeLoop suggested changes Sep 18, 2025

View reviewed changes

update

24d5036

HymanShi approved these changes Sep 19, 2025

View reviewed changes

hanxu-111 merged commit 3e9567c into main Sep 19, 2025
16 checks passed

hanxu-111 deleted the feat/insight_analysis branch September 19, 2025 03:09

Conversation

hanxu-111 commented Sep 17, 2025

What type of PR is this?

Check the PR title.

(Optional) Translate the PR title into Chinese.

(Optional) More detailed description for this PR(en: English/zh: Chinese).

(Optional) Which issue(s) this PR fixes:

Uh oh!

CLAassistant commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CozeLoop left a comment

Choose a reason for hiding this comment

Code Review Summary

Critical Issues Found (🚫Must-Fix):

Medium Priority Issues (🚨Should-Fix):

Positive Aspects:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CozeLoop left a comment

Choose a reason for hiding this comment

Uh oh!

CozeLoop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CozeLoop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CozeLoop left a comment

Choose a reason for hiding this comment

Uh oh!

CozeLoop left a comment

Choose a reason for hiding this comment

Uh oh!

CozeLoop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CLAassistant commented Sep 17, 2025 •

edited

Loading

codecov Bot commented Sep 17, 2025 •

edited

Loading