You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "me2seeks (via GitHub)" <gi...@apache.org> on 2026/09/24 06:54:32 UTC
[PR] fix(runtime): report the Host clock from every ScheduledTask result [maka]
me2seeks opened a new pull request, #5679:
URL: https://github.com/apache/maka/pull/5679
## Summary
#5667 made `ScheduledTask` report the Host clock, and its description now promises that "Every result reports the current Host time and time zone." That promise holds for `list` and `create` only: `pause`, `resume`, `delete`, and the missing-`id` guard return without the clock, so 2 of the 5 modes match the description.
This appends the clock line to those 7 return paths, which makes the stated invariant true instead of narrowing the wording to the subset that currently complies. The alternative — rewriting the sentence to name `list` and `create` — was rejected because it turns the contract into a mode-dependent rule a model has to track, while extending coverage costs about 70 characters on three rarely used modes.
Refs #5667
## Verification
- `node --test packages/runtime/dist/__tests__/scheduled-task-tools.test.js` — 5 pass, 0 fail.
- With the `scheduled-task-tools.ts` change reverted and the package rebuilt, the two new tests fail (2 of 5) and pass with it, so they cover the change rather than restating it.
- `node --test` over the four `@maka/runtime` suites that reference `ScheduledTask` — 226 pass, 0 fail.
- `tsc -p packages/runtime/tsconfig.json` with the build cache removed — 0 errors.
- `biome check` on the two changed files — clean.
- `node scripts/asf-license-headers.mjs check` — passes.
Not run: the full monorepo suite, and an end-to-end Desktop session. The tool output format changes by appending lines, and no test or consumer in the repository parses these strings; I checked that directly rather than inferring it.
## Review focus
The one judgement call is extend-coverage versus narrow-wording, described under Summary. Three return paths that already carried the clock before #5667 — non-empty `list`, and `create`'s missing-arguments and error branches — still have no exact assertion; covering them is a separate coverage gap, not part of this change.
## AI use
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Claude Code wrote the change and the tests. I reviewed the diff, ran the verification above, and removed a first-draft test that duplicated existing coverage. The commit carries a `Generated-by: Claude Code` trailer.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): report the Host clock from every ScheduledTask result [maka]
Posted by "me2seeks (via GitHub)" <gi...@apache.org>.
me2seeks merged PR #5679:
URL: https://github.com/apache/maka/pull/5679
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): report the Host clock from every ScheduledTask result [maka]
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on PR #5679:
URL: https://github.com/apache/maka/pull/5679#issuecomment-5810226656
**Seat: kabi-opus-review-orchestrator — post-merge close-out.**
Merged at `098dd89c5a2570492e3dcbd755b4dd75582e220e`. My approval was bound to `5632b66008798e86be8f5c383448ecd052edfea7`, so **this merged with the only approval on it not covering the merged code** — and that approval described the opposite implementation. I flagged it two minutes before the merge; recording the outcome rather than leaving the flag as the last word.
**The merged result is correct, and I checked it rather than assuming.** On `origin/main`: 12 returns in `impl`, 5 carry `clockLine()` — exactly `list` (both cases) and `create` (all three), which is precisely what the new sentence claims. The description and the code now agree, and the trailing "call `mode=list` first" advice is accurate again. The P3 I raised on #5667 is closed, by narrowing rather than extending.
**The final rationale is better than the one I reviewed.** The squash commit says the three skipped modes "have no use for a clock: none accepts a time input and none leads to computing runAt or startAt." That is a stronger argument for narrowing than the PR body's original case for extending, and it is not the argument I was weighing when I approved. Git history reads correctly.
**One leftover, cosmetic:** the pull request body still argues for extend-coverage and states narrow-wording "was rejected." That now contradicts both the merged code and the commit message. It does not affect `main` — the commit message is the one that matters and it is right — but anyone reading this page later will find the description arguing against the change it accompanies.
**Why I am spelling out the approval mismatch on a PR that turned out fine:** earlier today I reported that a substantial share of recently merged pull requests here carry approvals that do not cover the merged SHA, and asked someone with admin to check whether stale approvals are dismissed on push. This is that pattern, start to finish, with my own approval as the instance — approved 07:17, head reversed 07:53, merged 07:57. The outcome was good; the gate still did not do what its green state implied.
<details>
<summary>简体中文</summary>
合并在 `098dd89c5a2570492e3dcbd755b4dd75582e220e`。我的批准绑的是 `5632b66008798e86be8f5c383448ecd052edfea7` —— **所以本单合并时,唯一的那条批准并不覆盖被合并的代码**,而且它描述的是相反的实现。我在合并前两分钟贴了标注;这里记录结果,不让那条标注停在最后。
**合并结果是对的,这点我核过而不是假设**:`origin/main` 上 `impl` 共 12 条 return,5 条带 `clockLine()`,正好是 `list`(两种情况)与 `create`(三条分支)—— 与新那句话的声明完全一致。**描述与代码现在吻合**,结尾「call `mode=list` first」也重新准确。我在 #5667 上提的那条 P3 就此关闭,方式是收窄而非扩大。
**最终的理由比我评审时看到的那个更好。** squash 提交里写:被跳过的那三个 mode「没有用时钟的场景:它们都不接受时间输入,也都不会导向计算 runAt 或 startAt」。**这比 PR 正文当初支持「扩大覆盖」的论证更有力**,而且不是我批准时权衡的那个论证。**git 历史读起来是对的。**
**一处遗留,属于记录层面**:PR 正文至今仍在论证 extend-coverage 并写明 narrow-wording「被否决」。它现在同时与合并的代码和提交信息矛盾。**不影响 `main`**(起作用的是提交信息,而它是对的),但将来翻到这个页面的人,会看到描述在反对它所附带的改动。
**为什么一单结果没问题我还要把批准错配讲清楚**:今天早些时候我报过,本仓库近期合并的 PR 中有相当比例,其批准并不覆盖被合并的 SHA,并请有 admin 的人确认「push 后是否作废旧批准」。**这一单就是那个模式的完整一例,而且样本是我自己的批准** —— 07:17 批准,07:53 改动反转,07:57 合并。**结果是好的;但门禁并没有做到它那个绿色状态所暗示的事。**
</details>
---
> **Automated review notice:** This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): report the Host clock from every ScheduledTask result [maka]
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on PR #5679:
URL: https://github.com/apache/maka/pull/5679#issuecomment-5810136410
> **stale-approval-notice approved=5632b66008798e86be8f5c383448ecd052edfea7 head=098dd89c5a2570492e3dcbd755b4dd75582e220e seat=kabi-opus**
**My approval no longer covers this pull request, and this is a stronger case than ordinary head drift: the change was reversed, not amended.**
I approved `5632b66008798e86be8f5c383448ecd052edfea7`. The head is now `098dd89c5a2570492e3dcbd755b4dd75582e220e`. GitHub still shows this pull request as APPROVED, and that green state is currently attached to an implementation that no longer exists.
**What changed between those two commits:**
- `098dd89c` **reverts the clock from all seven paths** it had added — the missing-`id` guard and both branches of `pause`, `resume` and `remove`. `clockLine()` coverage goes from 12 of 12 returns back to 5 of 12.
- The description flips from *"Every result reports the current Host time and time zone"* to *"**Only the list and create results** report the current Host time and time zone"*.
That is the narrow-wording option instead of the extend-coverage one. **I said in my review that I would have accepted either, and I still would** — the new wording is accurate, and the trailing "call `mode=list` first" clause becomes correct again rather than obsolete, which also resolves the P3 I raised. So I have no objection to the direction.
**But nothing I verified still applies.** My approval rested on three checks, and the reversal invalidates all three:
1. "12 of 12 returns carry the clock" — now 5 of 12 again, by design.
2. The mutation check I reproduced (reverting the source made exactly 2 of 5 tests fail, and they were the two new ones) — the test file has since shrunk by roughly 60 lines, so those assertions appear to be gone. I have not re-run anything against this head.
3. Terminal green CI — `test` is pending again on the new commit.
**Two things for whoever merges this.** First, this approval should not be counted toward the merge gate; it describes the opposite change. Second, the pull request body still argues for extend-coverage and states that narrow-wording "was rejected because it turns the contract into a mode-dependent rule a model has to track" — **the description now argues against the code it accompanies**, and that reasoning was the main thing a reviewer was asked to weigh.
I am not dismissing my own approval, since revoking one moves a merge gate and that is a human's call. Re-reviewing the new head on request.
<details>
<summary>简体中文</summary>
**我的批准已不覆盖本 PR,而且这比普通的 head 漂移更严重:这次是改动被反转了,不是被修补。**
我批的是 `5632b66008798e86be8f5c383448ecd052edfea7`,当前 head 是 `098dd89c5a2570492e3dcbd755b4dd75582e220e`。**GitHub 仍然把本 PR 显示为 APPROVED,而那个绿色状态现在挂在一份已经不存在的实现上。**
**两个提交之间发生了什么**:`098dd89c` **把此前补上的七条路径的时钟又全部撤掉了**(缺 `id` 的守卫,以及 `pause`/`resume`/`remove` 各自的两个分支),`clockLine()` 覆盖从 12/12 退回 5/12;描述也从「Every result reports the current Host time and time zone」改成了「**Only the list and create results** report the current Host time and time zone」。
**这就是「收窄文案」而不是「扩大覆盖」那个选项。我在评审里说过两种我都能接受,现在依然如此** —— 新文案是准确的,而且结尾那句「call `mode=list` first」也随之重新成立,不再过时,**我提的那条 P3 因此也解决了**。所以我对方向没有异议。
**但我验过的东西没有一条还成立。** 我的批准建立在三项检查上,反转让三项全部失效:①「12 条 return 全部带时钟」—— 现在按设计退回 5/12;②我复现过的突变验证(还原源码后 5 条挂 2 条,且正是那两条新测试)—— 测试文件此后少了约 60 行,那些断言看来已被移除,**我没有对新 head 重跑任何东西**;③CI 终态绿 —— 新提交上 `test` 又在跑了。
**给合并者两点**:第一,**这条批准不应计入合并门禁**,它描述的是相反的改动。第二,**PR 正文仍在论证「扩大覆盖」,并写明「收窄文案」被否决,理由是「它会把契约变成模型必须记住的、依赖 mode 的规则」—— 现在正文是在反对它自己所附带的代码**,而那段论证恰恰是当初请评审者权衡的核心。
我不会自行撤销批准 —— 撤销会移动合并门禁,那是人类的决定。需要的话我重审新 head。
</details>
---
> **Automated review notice:** This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org