✨ feat(timing): 实现提醒规则删除 - #169
Conversation
通过 Timing Store 的原子关闭操作将规则置为 disabled,并取消未发生的 pending trigger,保留历史触发事实。\n\nHost 测试覆盖成功、NotFound、重复删除、关联错误和 Store 原子失败;SQLite/NVS、Runner 与投递仍不在本子任务范围内。\n\nRefs 1024XEngineer#142
cfb9519 to
c74b365
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@fennoai 你是资深后端/全栈架构师 + 嵌入式硬件工程师复合型专家,执行严格、客观、不留情面的代码仓库Review,请遵循下面所有评审规则,逐条输出审查结果,禁止敷衍、禁止只说空话、禁止笼统概括。 评审维度
输出要求
现在开始对提供的仓库代码/PR变更、硬件资料、结构模型进行评审 |
Review 结果🔴 [components/voicelife_timing/src/timing_task_service_delete_reminder_rule.cc:7-8] 只校验空串,不校验空白/控制字符。风险:外部协议传入脏 ID 会下沉到 store,形成“看似合法但永远 not found”的请求。修改:复用统一 ID 校验器,至少 trim 后再判空。 🟡 [components/voicelife_timing/src/timing_task_service_delete_reminder_rule.cc:11-13] 直接透传 store 错误但不附带 rule id。风险:线上定位删除失败时无法知道是哪条规则。修改:在错误消息里补上 🟢 [components/voicelife_timing/src/timing_task_service_delete_reminder_rule.cc:15-18] 返回 DTO 固定 🔴 [components/voicelife_timing/include/voicelife/timing/timing_task_store.h:45-48] 新 pure virtual 没有迁移窗口。风险:任何外部 🟡 [components/voicelife_timing/include/voicelife/timing/timing_task_store.h:45-48] 只返回受影响数量,不能回传被取消 trigger 列表。风险:调用方无法审计 side effects。修改:把结果升级为包含取消摘要的 struct。 🟢 [components/voicelife_timing/include/voicelife/timing/timing_task_store.h:45-48] 取消边界写死在注释里。风险:第二个 adapter 很容易实现出不同语义。修改:抽共享 helper 或常量定义。 🟡 [tests/host/support/timing_fakes.h:75-79] 失败注入是全局单次状态。风险:以后新增并发用例时,非目标 delete 可能先消费掉失败。修改:把失败注入挂到 rule id 或单独队列。 🟡 [tests/host/support/timing_fakes.h:81-90] 只校验 task 存在,不校验 trigger 与 rule 的 task 一致性。风险:跨 task 污染不会被测试暴露。修改:在 trigger 读取/删除时同时校验 🟡 [tests/host/support/timing_fakes.h:92-109] 每次删除都复制整张 rules/triggers map。风险:大状态下内存翻倍,删除路径退化成 O(n)。修改:只复制受影响项,或用事务式 staging 记录变更集。 🔴 [tests/host/support/timing_fakes.h:99-105] 只取消 🟢 [tests/host/support/timing_fakes.h:102-105] 被取消的 trigger 没有写 🟢 [tests/host/support/timing_fakes.h:115-118] 🟢 [tests/host/support/timing_fakes.h:121-126] 🟡 [tests/host/timing_task_service_test.cc:291-292] 🟡 [tests/host/timing_task_service_test.cc:293-306] 测试时间戳全是硬编码。风险:exact-now 边界和时钟漂移都没覆盖。修改:用 🟢 [tests/host/timing_task_service_test.cc:308-320] 只断言状态和计数。风险: 🟢 [tests/host/timing_task_service_test.cc:321-334] 重复删除 / 孤儿规则只看返回码。风险:冲突路径可能意外污染 store 状态却没人发现。修改:冲突后再回读 rule / trigger 状态。 🟡 [tests/host/timing_task_service_test.cc:336-349] 只验证“调用前失败”。风险:真实事务中间态回滚没有被覆盖。修改:在 fake 中增加“写规则后故障”钩子,验证整批回滚。 🟢 [tests/host/timing_task_service_test.cc:328-349] 通过 helper backdoor 注入 orphan state。风险:测试把非法状态当成正常路径,后续会掩盖 adapter 的约束缺陷。修改:把这类注入隔离到专门的 corruption fixture。 🟡 [tests/host/timing_task_service_test.cc:279-289] 新 delete 路径只在 host fake 上验证。风险:真实持久化适配器未覆盖时,生产行为可能偏离。修改:补一层 adapter / 端到端测试或至少接口契约测试。 整体风险这次改动的主逻辑是通的, 优先级
长期方案把“规则关闭 + 触发取消”的语义抽成共享领域约束,别让每个 adapter 各写一套边界判断;同时把测试夹具从“可随意注入脏状态”收敛成“显式 corruption fixture”,这样后续持久化适配器接进来时,测试才不会继续放过数据一致性问题。 已验证: |
删除规则时校验 trigger 与规则任务归属一致;故障注入在候选变更构造后返回,以验证规则和 trigger 均不会部分提交。\n\n补充 now 边界、关联错误和原子失败的 Host 测试,未扩展 Runner、投递或生产存储适配器。\n\nRefs 1024XEngineer#142
|
Reference: fennoai review and Codecov report. 判断依据:#142 只交付
Codecov 的 90% patch 报告已记录;其必需的覆盖率检查通过。本次补充的边界与原子失败测试覆盖了 review 中属于 #142 的新增行为。 |
HuXiaohui424
left a comment
There was a problem hiding this comment.
@fennoai 你是资深后端/全栈架构师 + 嵌入式硬件工程师复合型专家,执行严格、客观、不留情面的代码仓库Review,请遵循下面所有评审规则,逐条输出审查结果,禁止敷衍、禁止只说空话、禁止笼统概括。
硬性约束:最终输出评审条目数量不少于20条,若直观可见问题不足,主动挖掘隐性架构隐患、软硬件兼容风险、长期运行潜在缺陷补足条目,不得简化评审内容。
评审维度
- 架构与模块设计
- 模块职责是否清晰,是否存在循环依赖、职责混杂
- 分层是否合理,是否违反单一职责原则、开闭原则
- 接口抽象、依赖注入设计是否规范,有无硬编码耦合
- 软件与硬件模块边界是否清晰,硬件相关逻辑是否侵入业务层
- 代码规范与可读性
- 命名:变量、函数、类、文件命名是否语义清晰,禁止模糊命名、拼音命名
- 注释:复杂逻辑、硬件时序、特殊寄存器配置必须注释;冗余注释、无效注释、过期注释需要指出
- 代码格式、风格是否统一,是否存在大量魔法数字、魔法字符串,硬件参数无常量定义
- 性能隐患
- 循环内IO、数据库重复查询、不必要的内存占用、低效算法
- 资源是否释放(连接、句柄、定时器、文件流、硬件外设句柄)
- 嵌入式场景:阻塞轮询、中断处理耗时过长、内存频繁分配释放
- 安全性检查【重点】
- 输入校验、SQL注入、XSS、权限控制、敏感信息明文打印
- 密钥、token、数据库地址、硬件访问口令是否硬编码提交到仓库
- 外部指令下发至硬件驱动缺少权限校验,存在设备失控风险
- 健壮性 & 异常处理
- 是否缺少异常捕获、错误分支处理
- 参数判空、边界条件、失败重试逻辑是否完备
- 硬件通讯异常(I2C/SPI/UART断线、设备无应答)缺少容错、恢复逻辑
- 缺少硬件故障状态上报、故障隔离机制
- 硬件驱动 & 软硬件协同评审(新增专项)
- 驱动代码与硬件原理图引脚定义是否匹配,无硬件版本兼容逻辑
- 外设操作缺少电平保护、超时判断,存在烧毁外设芯片风险
- 运动控制逻辑(如有)缺少软限位、急停、碰撞检测保护
- 上下位机通讯协议:缺少校验和、重传、断线重连机制
- 硬件参数(电流、电压、速度阈值)直接写死代码,无统一配置入口
- 测试覆盖
- 是否缺少单元测试、边界用例
- 核心业务逻辑、硬件驱动逻辑无测试直接合并风险
- 缺少硬件异常工况、极限压力场景测试用例
- 工程规范
- 无用代码、注释掉的废弃代码是否大量遗留
- 依赖包版本风险、冗余依赖、存在已知漏洞依赖
- 配置区分环境(开发/样机/生产),环境配置是否混写
- 固件、上位机、3D模型、原理图版本号不统一,无关联管理
- 可维护性 & 扩展性
- 新增需求是否需要大面积修改原有代码
- 是否存在大量重复代码,缺少公共封装
- 硬件型号迭代时,软件需要大规模改动,缺少适配抽象层
输出要求
- 问题分级:🔴严重阻塞(必须修改才能合并/上机测试) / 🟡建议优化(不阻塞但强烈建议重构) / 🟢可选改进
- 每条问题固定格式:【文件路径+代码位置】问题描述 + 风险说明 + 可直接落地的修改方案
- 条目数量强制≥20条;问题较少时区分表层问题与隐性长期风险,拆分为独立条目逐条列出,禁止多条问题合并为一条
- 最后汇总三部分:
① 整体仓库风险总结
② 优先级整改清单(阻断项优先)
③ 长期架构、软硬件协同优化方案 - 如果存在架构层面缺陷,单独提炼顶层设计问题,不要只局限单行代码
- 不要美化结论,发现隐患直接指出,客观评估风险等级;不使用“建议考虑”这类温和模糊表述,明确写出故障后果(程序崩溃、硬件烧毁、机构失控、数据泄露等)
现在开始对提供的仓库代码/PR变更、硬件资料、结构模型进行评审
Review 结果🔴 【 🟡 【 🟡 【 🟢 【 🟡 【 🟢 【 🟡 【 🟡 【 🟢 【 🟢 【 🟢 【 🟡 【 🟡 【 🟢 【 🟢 【 🟢 【 🟡 【 🟢 【 🟢 【 🟢 【 🟢 【 整体仓库风险总结这版功能线是闭合的, 优先级整改清单
长期架构、软硬件协同优化方案把“规则关闭 + 触发取消”的一致性规则提到共享领域层,统一定义时钟边界、终态字段和回滚语义,不要让 fake、service、真实 adapter 各写各的。测试夹具也要分成“正常数据构造器”和“显式脏数据注入器”,否则后续持久化适配器一接入,回归会继续藏在测试假象里。 已验证: |
…nder-rule-main # Conflicts: # components/voicelife_timing/CMakeLists.txt # components/voicelife_timing/include/voicelife/timing/timing_task_store.h # components/voicelife_timing/src/timing_task_service_stubs.cc # tests/host/CMakeLists.txt # tests/host/support/timing_fakes.h # tests/host/timing_task_service_test.cc
结论
本 PR 完成 #142:
DeleteReminderRule通过 Timing Store 的单一原子操作关闭规则、取消未来 pending trigger,并保留历史 trigger 事实。请 Reviewer 重点确认未来时间边界、规则与 trigger 的原子性,以及重复删除和关联错误的语义。Refs #142
背景
DeleteReminderRule此前仍由 Service stub 返回kUnavailable,调用方无法关闭不再需要的提醒规则。本 PR 基于最新main,仅包含 #142 的删除行为。变更
TimingTaskStorePort::DisableReminderRule:同一原子边界内关闭规则并取消尚未发生的 pending trigger,返回受影响数量。DefaultTimingTaskService::DeleteReminderRule,校验空标识并透传 Store 的 NotFound、Conflict 与基础设施错误。明确未包含:
ListReminderTriggers、snooze、dismiss、完整 recurrence 展开或 Gateway 编排。架构与兼容
TimingTaskStorePort增加纯虚DisableReminderRule,后续真实 Adapter 必须在其事务中实现同样的全成或全败语义。公开 Service DTO、Profile、外部协议、组件依赖方向和持久化格式均未改变;当前只影响 Host 内存 fake。验证
./scripts/run_pre_submit_checks.sh:本机未完成,Ruff0.16.1与仓库固定的0.12.7不匹配。idf.py,仅完成python3 scripts/firmware.py validate。证据:
ctest --test-dir build-host --output-on-failure:14/14 通过。python3 scripts/check_public_api_docs.py、scripts/check_architecture.sh:通过。clang-format --dry-run --Werror与git diff --check:通过。TDD 记录
timing_task_service_test写入活动规则删除场景,执行后因DeleteReminderRulestub 返回kUnavailable失败。timing_task_service_stubs.cc移到独立翻译单元,保留其余未实现 Service 方法的 stub 边界。风险与回退
未来 trigger 的定义为
planned_trigger_at >= now且状态为 pending;此语义已写入 Port 文档,真实 Adapter 需要一致实现。真实持久化、并发和掉电恢复尚未覆盖,属于后续 Adapter 任务。当前无数据迁移。若需回退,回退本 PR 的单一提交即可恢复
kUnavailablestub;已物化数据不会被本 PR 的 Host fake 之外代码修改。