Repository navigation
refactor(issue-fix): import the reviewer-notification time shape from its owner - #6923
Conversation
e55e8b5 to
9ca0008
Compare
songoow
left a comment
songoow
left a comment
There was a problem hiding this comment.
Reviewer: model_agent; OpenAI GPT-6 Astra; provider OpenAI; self_reported.
审阅版本：9ca00080b2b5473135d3d9a20254c61d15ecd611；base：main 5853e151d97d9129bda9dce45a92a0944dcb9565。已阅读全部 3 个文件的 diff、domain-pack 定义、发送窗口/回执/记忆策略调用链和新增测试。结论是 COMMENT，有一项非阻塞的 guard 覆盖建议；本轮未执行测试或 CI。
动机
通知发送、记忆中的发送策略和 domain-pack 持久化回执都需要判断同一种 HH:MM 格式。继续各自声明正则会让这些入口在以后修改格式时发生漂移。复用已经负责通知回执规则的 domain-pack 定义，能减少这类重复判断。
改动思路
两个 capability 模块直接导入现有 REVIEWER_NOTIFICATION_LOCAL_TIME_PATTERN，其余窗口语义仍留在各自调用方：时区校验、起止时间不可相同、窗口外排队以及回执字段校验没有移交给正则。新增 architecture guard 负责检查重复定义、消费者绑定和格式边界。
具体改动
关键代码讲解
- domain-pack 中的定义及回执校验在 base 中已使用同一正则。本次两个被移除的本地正则与 owner 定义的内容和编译 flags 一致，没有扩展或收窄当前时间格式。
- 通知窗口决策保留输入去空白、
start != end、queue_without_send、时区转换和跨午夜分支；排队回执校验也只替换格式判断。原来原样校验回执字段的位置没有意外增加去空白行为。 - 记忆策略入口继续验证
ZoneInfo和time.fromisoformat，返回同样的规范化策略；后续 before-send 路径仍调用该入口。新的导入依赖没有从 domain-pack 反向指向这两个 capability，也没有在导入时执行通知或 ledger 写入。 - 新增 guard 的消费者断言同时核对 importer 清单、对象身份、调用引用和遗留本地名称；格式用例区分合法边界与越界小时/分钟、缺位和秒字段。已阅读这些断言及既有 before-send 结构化策略用例，没有把它们描述为本轮已通过。
[P3，非阻塞] 把不可折叠构造的检测接入真实仓库扫描
位置：tests/architecture/test_issue_fix_local_time_owner.py:126–136，适合 inline 的位置是第 135 行。
当前 collect() 只收集 time_rows()；该函数只记录可以折叠成完整时间正则的表达式。unfoldable_rows() 虽然存在，却只在合成 probe 测试中调用，最后仅断言例外表为空。因此，若实际源码新增该 probe 所示的 build(prefix) 构造，仓库级检查不会把它交给不可折叠检测，空的 DECLARED_UNFOLDABLE_SITES 也不会拒绝它。这是从测试调用关系得出的覆盖缺口，未执行 mutation。
建议增加真实仓库的 unfoldable_rows() 扫描并核对按文件计数的例外表。最小回归可把现有 probe 放进临时 loopx/ 目录，从仓库扫描入口检查空例外表能拒绝它，而不只直接测试检测 helper。当前生产迁移不使用这种构造，因此这是一项非阻塞建议。
对主干的风险
生产调用点使用的正则和值保持一致；发送权限、排队时间计算、幂等标识和持久化回执格式未改。仓库搜索未发现其他模块引用被删除的本地 LOCAL_TIME_PATTERN 或 _LOCAL_TIME_PATTERN。对现有运行行为，暂未建立本次重构引入的缺陷。
剩余的不确定性是本轮未执行导入、窗口场景或架构测试；不可折叠扫描的上述缺口则是已识别的测试设计问题，两者分别记录。
我的整体评价
两处生产迁移直接复用已有定义，周围的窗口规则和失败处理保留，改动与目标相符。建议补齐 guard 的仓库级不可折叠检查，使其实际覆盖范围与文件说明一致。没有发现需要阻断当前生产重构的实现问题，按 COMMENT 发表。
English verdict: COMMENT — the production changes preserve the existing local-time and delivery-window behavior. One non-blocking guard coverage gap remains: unfoldable expressions are tested in isolation but are not scanned in the actual repository. No tests or CI were executed for this review.
| source = path.read_text(encoding="utf-8") | ||
| except (OSError, UnicodeDecodeError): | ||
| continue | ||
| rows.extend(time_rows(source, path.relative_to(root).as_posix())) |
There was a problem hiding this comment.
[P3，非阻塞] 这里的仓库级 collect 只收 time_rows；unfoldable_rows 仅由末尾的合成 probe 测试调用。因此实际文件新增相同的 build(prefix) 构造时不会进入违规列表，空的 DECLARED_UNFOLDABLE_SITES 也没有被用于仓库检测。建议补一条遍历真实源码的不可折叠扫描，并把 probe 放进临时 loopx/ 目录，从仓库入口验证空例外表会拒绝它。这不影响当前两处直接导入的生产行为。
|
Thanks - the P3 is right and is fixed at 79f7dcb.
What changed in
Evidence on 79f7dcb: guard 23 passed; I updated the PR body's tested revision, criteria row and mutation row to match this head. Python Tests is running on the new head. |
… its owner Two issue-fix surfaces validated the same reviewer-notification ``allowed_local_time`` window with their own compile of the HH:MM shape the domain pack already owns, including the same start == end and outside_window rules. A later edit to one copy could accept a window the other rejects, and nothing tied the three call sites together. Observable result: the window verdict now comes from one pattern object for the domain pack, the notification sinks/before-send surface and the reward-memory delivery policy. The new guard judges folded values rather than spellings, pins every delegating module to the owner's object, and reconciles the modules that actually import the owner against the declared consumer list, so deleting an entry cannot silently shrink coverage. Validation: guard 22 passed; reviewer/reward-memory selection 343 passed; focused run 84 passed; ruff check clean; configured mypy scope 17 files clean. Mutations: local copy back in reward_memory -> 2 red; inline literal in the notification call site -> 2 red; dropping one consumer entry -> 1 red (reconciliation test). Refs loopx-project#4447 Signed-off-by: Twiink <154496350+Twiink@users.noreply.github.com>
… repo scan Review P3: `collect()` only collected folded-value rows, so `unfoldable_rows()` was reached only by a synthetic probe and the empty declaration budget never participated in a repository-level decision. A real `build(prefix)` construction added under `loopx/` would therefore not have been reported. Add `collect_unfoldable()` plus `unfoldable_offender_rows()` and assert the real tree has no offending site; the probe now writes itself into a temporary `loopx/` tree and is rejected through the same collector the repository uses. Validation: guard 23 passed; ruff check clean. Mutation: making the collector return nothing turns the repository-scan probe red. Signed-off-by: Twiink <154496350+Twiink@users.noreply.github.com>
loopx-agent
left a comment
loopx-agent
left a comment
There was a problem hiding this comment.
Reviewer: model_agent; model=gpt-6.1-sol; provider=OpenAI; reasoning_effort=max; declaration_source=runtime_reported; execution_observation_id=7e67cfc1236c30357c205fb4c8a0d2d1461c434b0992d98e561be74d50561f42
审阅 head：79f7dcb853a91e2b943311cc66f3d220d4b6569f；不可变基线：5853e151d97d9129bda9dce45a92a0944dcb9565。完整增量3文件、+374/-10；未发现阻塞问题。
动机
维护评审通知投递窗口的开发者，以及依赖通知队列和记忆策略校验的调用方。
以前修改同一个小时分钟格式，需要同时检查领域包和两个能力模块中的三份正则；本次让两个模块直接导入已有规则，避免后续规则调整只到达一部分入口。
当前三个真实校验入口使用同一来源；相同合法、非法和排队恢复输入在改动前后输出一致，模拟修改领域规则后，公开能力入口也随之采用新规则。
本片不调整通知窗口含义、时区、队列协议或发送权限，不证明全部语义词汇已经收敛，也不宣称长期收益已被测量。
这是一个实际维护入口的收敛任务。保留三份完全相同的格式，下一次修改就仍要依靠人同步；只复用已有规则即可消除这个具体问题，不需要引入统一的时间策略服务。
改动思路
格式判断继续由已有 issue-fix 领域包负责；窗口计算、记忆应用和通知副作用仍留在原调用方，测试扫描器仅约束这一个格式。
本 PR 只删除两个重复编译点、替换其调用引用，并增加一个有边界的仓库守卫；不新增运行时配置、持久状态或另一套通知决策。
窗口起止、跨午夜判断、时区换算、队列去重和外部发送各有现有调用方。本次只共享最小的格式判断，保留它们的不同职责。策略输入原来会去除两端空白，已存队列回执原来不会；直接复用 pattern 没有把这两种行为错误合并。
362行新增代码全部是架构测试：扫描声明的精确格式、检查两个消费者的来源、验证边界和反例。它没有变成产品运行时解析器。已比较现有身份与摘要守卫；本片不要求为了这两个重复点再抽取通用 AST 框架。
具体改动
依据改动前的 #4447 Track A 和现有领域 owner 评审：Track A: measured single owner：实测重复规则收敛、原语义保持是 implemented；Parent vocabulary convergence：整个仓库的词汇/F6 收敛是 out_of_scope。未把作者对更大目标的声明当作验收。
关键代码讲解
REVIEWER_NOTIFICATION_LOCAL_TIME_PATTERN的表达式未改。实际扫描从领域包加两个能力模块共3个编译声明，变成领域包1个声明；现有领域回执读取继续使用它。_delivery_window_decision只将本地 pattern 换成导入，开始包含、结束排除、跨午夜和相同端点拒绝均保留。公开 builder 的默认预览、窗口外排队、重复等待及开窗恢复均已按完整输出比较。validate_issue_fix_reviewer_notification_sinks_result保留回执的既有校验次序与错误。合法回执通过，24:00 和带空白的已存时间仍拒绝，未新增回执迁移或放宽权限。_reviewer_notification_delivery_policy仍负责策略标准化、时区和实际时间解析；现在复用相同格式来源，记忆应用、结果身份和发送前 gate 未变。
独立反事实不依赖 Python 正则缓存的对象身份：在隔离进程中先模拟修改领域 owner，再导入消费者，走真实公开 builder。原基线未采用 owner 改动，独立断言失败；当前 head 采用改动，断言通过。没有编辑被审仓库或发送通知。
对主干的风险
macOS/Python3.12 同输入比较覆盖15组窗口/记忆策略，包括普通时段、跨午夜、起止边界、空白、非法小时/分钟、秒数、Unicode数字、无效时区和无时区观察时间；完整值或异常类型与全文均一致。另有10组公开入口和回执结果一致，包含作者排除、默认行为、队列重复和恢复。提供方使用无 IO 的合成适配器，这证明本次改变的能力决策边界，不冒充实际消息已送达。
架构守卫 23 passed；相关能力和控制面筛选 409 passed、12078 deselected，无 skip。三文件 Ruff 通过；原生 premerge 的 5项直接检查与10个选中 catalog 检查全部通过，零 hold。依当前 wait_for_ci=false 未查询、轮询或等待远端 CI。
语义与 CI 对齐
本片选择 reuse_existing：没有新增枚举、持久协议、调度义务或 actor 权限。守卫针对精确格式和有界字符串折叠，实际仓库的 unfoldable 收集及其负例均执行；它不能认证任意别名、动态正则或全部语义等价写法。此限制不影响已逐个核对的三个真实消费者，也不应升级为本片之外的强制重构。
我的整体评价
APPROVE。这个明确的格式 owner 任务为 goal_achieved；更大的词汇收敛仍保持原范围。长程行为判断为 preserved：队列重放、等待恢复和失败 owner 不变，没有新增调度循环。使用体验同为 preserved：不增加配置、导航、信息重填或确认轮次，原有结果可读回。
生产代码采用最小现有 owner，兼容格式与权限无变化；边界测试的维护成本可见且留在测试内。没有把测试数量当作长期效果，也没有用一次预览证明外部提供方或安装后全部平台。结论仅对应上述未变 head，合并仍需独立 readiness 与维护者权限。
English verdict: APPROVE — 79f7dcb853a91e2b943311cc66f3d220d4b6569f removes two duplicate format definitions while preserving existing notification, queue and memory-policy behavior. Paired real capability outputs and a public-entry owner-change counterfactual passed, together with23 guard cases,409 related tests, Ruff and native premerge. The scanner is bounded; external delivery and broader vocabulary convergence are not certified. CI was not consulted.
|
Attribution for the current red on The failing jobs are
It requires That pattern is absent in both revisions I measured:
So the assertion is red on main's own dashboard source, independent of this branch. This PR touches three Python files only ( I am not fixing the dashboard contract here to keep the slice reviewable. If you would rather have it in a separate PR, say so and I will open one against the commit that changed Earlier on this branch the |
|
Narrowing the attribution above with the commit that introduced it.
title: personalGoalTitle(goal.id, goal.source_access?.label ?? goal.display_name),The contract test still requires the plain form I am not changing the assertion here: whether the registered display name should now be the source label (with |
79f7dcb to
17830d0
Compare
|
Status on the rebased head Remaining red, and why it is not this diff:
Both are the dashboard contract assertion introduced by
The rebase also moved the base from |
4 participants
Goal And Delivered Outcome
Outcome basis / optional anchor: [Task]: Semantic vocabulary convergence follow-ups after RFC PR #4433 / 语义词表收敛后续任务 #4447 Track A (semantic vocabulary convergence). The issue-fix domain pack already owns the reviewer-notification local-time shape and applies it to the same delivery window.
Goal/source and gap:
loopx/domain_packs/issue_fix.pyownsREVIEWER_NOTIFICATION_LOCAL_TIME_PATTERNand uses it to validate a reviewer-notification window. Two capability surfaces validated the sameallowed_local_timewindow with their own compile of the sameHH:MMpattern, including the samestart == endandoutside_windowrules. A later edit to one copy could accept a window the other rejects, and nothing tied the three call sites together.Observable before -> after, with the validation row that proves it: before, three modules answered "is this a window time" with three pattern objects; after, the notification surface and the reward-memory delivery policy import the domain pack's object, and the new guard reconciles the modules that import it. Mutation: restoring a local copy in
reward_memory.pyturns two guard tests red.Issue/task and intended base: Related to [Task]: Semantic vocabulary convergence follow-ups after RFC PR #4433 / 语义词表收敛后续任务 #4447. Base
mainat d95839c (rebased from 5853e15 after fix(workspace): reuse shared source namespace digest rule #6925).Author Declaration
Implemented against
loopx/domain_packs/issue_fix.pyat 5853e15 (the pack already owns the receipt and local-time shapes) and [Task]: Semantic vocabulary convergence follow-ups after RFC PR #4433 / 语义词表收敛后续任务 #4447 Track A.issue_fix/reviewer_notification.py,issue_fix/reward_memory.pytest_consumer_holds_the_owner_object_and_applies_ittest_the_scan_reports_a_restated_local_timetest_every_module_that_imports_the_owner_is_a_declared_consumercollect_unfoldable+unfoldable_offender_rowstest_no_module_hides_an_unfoldable_local_time_constructionREVIEWER_NOTIFICATION_LOCAL_TIME_PATTERNruff format --checkat this base revision.Scope And Continuation
Validation
staticpassedruff checkon the changed files; configuredmypy(17 source files) cleanunitpassedpytest -q tests/capabilities tests/control_plane -k "reviewer or reward_memory"343 passedregression_paritypassedreward_memory.py-> 2 red; inline literal at the notification call site -> 2 red; one consumer entry dropped -> 1 red (reconciliation test); repository unfoldable collector made blind -> 1 red (P3 probe)manualpassed+, imported fromre, or written at a matcher is reported. Residual:ruff format --checkalready failed on both product files at this base revision; those pre-existing spans are untouched, and the new guard file is formatted.Frontend / Visual Evidence
Type of Change
LoopX Area
Technical Direction
Shared-authority RFC fixture impact
Boundary Checklist
none.Signed-off-bytrailer (git commit -s).