Skip to content

test: drop assertions that cannot fail - #416

Merged
oxwen11 merged 7 commits into
mainfrom
test/drop-low-value-tests
Oct 5, 2026
Merged

oxwen11 merged 7 commits into
mainfrom
test/drop-low-value-tests

Conversation

@oxwen11

@oxwen11 oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner

Summary

  • Delete pure self-comparisons (f(x) === f(x)) in session keys, SSH state keys, socket dirs, cron jitter, and stableHash. They cannot fail while those functions stay pure.
  • Delete isHelpOrVersion negatives. They pass if the predicate always returns false, and they never check --help.
  • Delete the ready-line prefix case. The round-trip already fails if the prefix is missing.
  • Delete the pie/ branch-name echo. Session and CLI tests already require ^pie/[0-9a-f]{8}$.

Kept security, protocol, path, package, and visual-class contracts. Those still have no stronger owner.

Proof

  • server handshake, worktree, cron: 11 passed
  • pi-loop cron: 11 passed
  • ssh target: 13 passed
  • app session-ref: 3 passed
  • verify browser: 21 passed

@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

React Doctor found no new issues. 🎉

Reviewed by React Doctor for commit 7842c37.

@pkg-pr-new

pkg-pr-new Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
npx https://pkg.pr.new/oxwen11/pie/@getpie/cli@416

commit: 7842c37

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

auto-merge: no

head 4deeef70fc04c2ee971457caa2c6e1d944da6fe5 — required CI green (Check, react-doctor, both Publish @getpie/cli preview), MERGEABLE (BEHIND).

out of .agents/rules/auto-merge-pr.md allowed groups; fail closed:

  • test coverage removal, not an allowed docs/tooling/lint/ui-presentation hunk: drops tautological/stability assertions across session-ref, cron jitter/stableHash, handshake ready-prefix, server worktree branch-name, ssh remoteStateKey, and verify browser socket-dir tests
  • deletes entire tools/verify/src/surfaces/cli-run.test.ts (isHelpOrVersion smoke)
  • matches Din preference / prior pattern (test: drop the makePiAgent construction smoke #414): test-file deletion / assertion stripping → human review, even when product code is untouched

@oxwen11
oxwen11 force-pushed the test/drop-low-value-tests branch from 4deeef7 to 4d8361c Compare September 30, 2026 13:37
@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

Current-base gate — not merged

  • Context: test: drop assertions that cannot fail. Same-repository oxwen11 PR, open, non-draft, targeting main.
  • Head: 4d8361c0d39c45d9d20a86d3c9dfa3db28e32f6b.
  • Current base and trusted rules commit: c7c4138afdafaf951349be57cb8acc990a07e0aa. The rule/protection files are unchanged from the previously trusted d7f4b7a95588e5de2aeefbe790e65111e89195f6; the intervening merge is documentation only.
  • Head is behind current main by 1 commit(s). GitHub state: MERGEABLE / BEHIND.

Required CI observed at this head

Context and acceptance

Full diff inspected for routing: this removes existing assertions and an entire CLI verification test file, rather than adding isolated tests. The CI-only exception does not apply to changed assertions. No independent test run or final correctness signoff on the removed guarantees is claimed.

Stopped at the current-base gate; not merged. No further code review or independent verification is run against a behind candidate. No branch update, conflict resolution, fix-and-merge or protection bypass was performed. Revisit only after the author supplies a current-base-containing, conflict-free candidate with all required checks present, executed and successful; then restart at CI, review and independent acceptance. Unchanged blockers need no duplicate processing.

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

auto-merge: no

head 4d8361c0d39c45d9d20a86d3c9dfa3db28e32f6b — required CI green (Check, react-doctor, both Publish @getpie/cli preview), MERGEABLE (BEHIND). Prior no was on 4deeef70fc04c2ee971457caa2c6e1d944da6fe5; re-reviewed this rebased head.

out of .agents/rules/auto-merge-pr.md allowed groups; fail closed:

  • test coverage removal, not an allowed docs/tooling/lint/ui-presentation hunk: drops tautological/stability assertions across session-ref, cron jitter/stableHash, handshake ready-prefix, worktree branch-name, ssh remoteStateKey, verify browser socket-dir; deletes tools/verify/src/surfaces/cli-run.test.ts (isHelpOrVersion smoke)
  • not ui-presentation / lint-format; docs-tooling.md does not cover stripping or deleting tests

Policy from #345 head 261780ad (.agents/rules/auto-merge-pr.md + groups; absent on current main after docs rewrite). No merge (Din review-first).

@oxwen11
oxwen11 force-pushed the test/drop-low-value-tests branch from 4d8361c to 3e03c44 Compare September 30, 2026 14:38
@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

分支更新结果

按开发者授权,使用 GitHub 原生分支更新(rebase)将 PR 更新到当前 base。

  • 更新前 head:4d8361c0d39c45d9d20a86d3c9dfa3db28e32f6b
  • 更新后 head:3e03c44acf2a9fca3e342c8e957a0b88cff5ff64
  • 更新时 base:d7f4b7a95588e5de2aeefbe790e65111e89195f6
  • 查询时 GitHub 状态:UNKNOWN / UNKNOWN

未手动修改文件或解决冲突,未进行代码审查、验收或合并。旧版本 CI 和验收结论不能用于新 head;等待新版本必需 CI 全部成功且无冲突、不落后后,再进入审查。

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

更新后的 base 复核:通过 GitHub commit compare 确认新 head 3e03c44acf2a9fca3e342c8e957a0b88cff5ff64 已包含当前 main c7c4138afdafaf951349be57cb8acc990a07e0aa,behind_by=0。上一条记录的 base 是更新前 PR 查询返回的值;本条补充当前主分支的准确集成版本。仍须等待新 head 的必需 CI,通过后再审查。

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

auto-merge: no

head 3e03c44acf2a9fca3e342c8e957a0b88cff5ff64 — required CI green (Check, react-doctor, both Publish @getpie/cli preview, Continuous Releases, React Doctor), MERGEABLE (BEHIND). Prior no was on 4d8361c0d39c45d9d20a86d3c9dfa3db28e32f6b; re-reviewed this rebased head.

out of .agents/rules/auto-merge-pr.md allowed groups; fail closed:

  • test coverage removal, not an allowed docs/tooling/lint/ui-presentation hunk: drops tautological/stability assertions across session-ref, cron jitter/stableHash, handshake ready-prefix, server worktree branch-name, ssh remoteStateKey, and verify browser socket-dir stability
  • deletes entire tools/verify/src/surfaces/cli-run.test.ts (isHelpOrVersion coverage)
  • docs-tooling.md / lint-format.md / ui-presentation.md do not allow stripping or deleting tests; uncertainty on whether every removed assert is truly vacuous → skip

exclusions.md not the primary hit (no product control-flow change), but fail-closed on weakened verification still blocks auto-merge.

@oxwen11
oxwen11 force-pushed the test/drop-low-value-tests branch from 3e03c44 to 416f6e9 Compare September 30, 2026 15:25
@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

分支跟进当前 base:旧 head 3e03c44acf2a9fca3e342c8e957a0b88cff5ff64 已 rebase 为 416f6e961194c2ed0ccc139b12a6080ffd2fc8d7。GitHub compare 确认已包含 main 8f7546548ca95b7167c2261ebe87f4425806cf65,behind_by=0。等待新版本全部 required CI;尚未审查、验收或合并。

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

CI 门禁失败:未进入审查

  • Head:416f6e961194c2ed0ccc139b12a6080ffd2fc8d7
  • 当前 base / 可信规则 commit:8f7546548ca95b7167c2261ebe87f4425806cf65
  • 使用可信 base 的 PR 流程;本次候选不是旧 head 的验收延续。

当前 required checks:Check、react-doctor、另一项 preview SUCCESS;这一项 Publish @getpie/cli preview 失败。GitHub job 记录显示依赖安装及 CLI 构建通过,失败步骤是 Publish preview to pkg.pr.new。

这不是对实现质量的结论。必需 CI 尚未全成功,停止于 CI 门禁;没有查看产品 diff、运行独立功能验证、修复或合并,也没有重跑检查或绕过保护。

重新处理条件:明确处理发布检查失败,随后对包含当前 main、无冲突的新候选重新检查全部 required CI。原因若是外部服务/网络,需要记录诊断与有依据的后续处理;不可仅反复重跑直到变绿。

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

补充上一条 CI 阻塞记录:失败日志明确出现 pkg.pr.new 的 HTTP 500,随后 fetch failed / Headers Timeout Error。安装与构建已成功;当前证据指向发布服务/网络失败,不是代码审查结论。没有重跑或绕过此必需检查。

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

发布故障已具备一次受控重试的依据:原失败是 pkg.pr.new HTTP 500/Headers Timeout;随后 #402 的同一 preview workflow(run 36738144611)中实际 Publish preview to pkg.pr.new 步骤成功,而非 push 去重跳过。按开发者要求直接推进无需新决策的修复,对当前 head 416f6e961194c2ed0ccc139b12a6080ffd2fc8d7 的失败 job 发起一次重试。

不改代码、不削弱检查;若再次失败就保留阻塞继续诊断,不循环刷绿。新结果全部成功后才进入代码审查与独立验收。

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

CI 恢复后审查发现:必须保留隔离保护的负例

Head 416f6e961194c2ed0ccc139b12a6080ffd2fc8d7,base/可信规则 8f7546548ca95b7167c2261ebe87f4425806cf65。发布服务恢复后的一次受控重试成功,全部 required CI 已成功。

完整 diff 与调用链审查发现:删除 tools/verify/src/surfaces/cli-run.test.ts 不能归为“断言无法失败”。isHelpOrVersion 是 runPie 无 current run 时是否允许调用真实 CLI 的守卫;原负例能发现把普通命令错判为 help 的回归,保护没有隔离 run 时不落到默认宿主环境。没有正例意味着覆盖不完整,不意味着负例无效。其余同值自比较删除与此不同。

当前不予通过、不合并。按开发者已授权的无新决策修复范围,后续修复将保留原有负例并补 help/version 正例;修复版本必须重新通过 CI,再交独立审查和验收,不把本次修复视作自我批准。

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

修复提交与独立审查交接

  • 新 head:34417420dc25fd3e3d0224107700dd444ebcc6f2
  • 集成的 main / 可信规则:8f7546548ca95b7167c2261ebe87f4425806cf65

保留 isHelpOrVersion 的原负例,补齐 --help/-h/--version/-v 与子命令 help 正例;其余自比较清理不变。实际运行 pnpm exec vitest run --project=verify tools/verify/src/surfaces/cli-run.test.ts tools/verify/src/runtime/browser.test.ts:2 个文件、23 项测试通过。oxfmt 与提交钩子通过。

这是实现者的修复自检记录,不是独立验收或批准。已普通推送并触发新 CI;不复用旧 head 的全绿状态。下一步:新 required CI 全部成功后,由独立审查者在干净 worktree 审查完整 PR 并验收。UI 产品路径仍需 Web/Desktop 截图与视频(适用时)。本轮没有合并。

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

独立审查后的覆盖修复

独立审查在 head 34417420dc25fd3e3d0224107700dd444ebcc6f2 / 可信 base 8f7546548ca95b7167c2261ebe87f4425806cf65 上指出两个剩余 P2:SSH state key 重复调用稳定性,以及 server/pi-loop 两份 recurring jitter 的确定性,没有被仅检查格式/范围的断言替代。审查者没有修改源码,也没有把 CI 全绿当作批准。

已在新 head a71c5e1d8b9127d00130ed19630b0fd27e51c0f7 恢复这三处原有行为约束(server cron 测试文件恢复为基线内容),保留此前已恢复的 CLI 隔离负例和新增正例;剩余清理不扩展。

实现者实际运行 4 个相关测试文件:32 项通过,无类型错误;格式、diff、提交钩子通过。未改产品代码,普通推送触发新 CI。

前次独立审查结论仍是阻塞,不能由本条自检替代。新的完整 PR 必须重新通过 CI 与独立复核后才能合并;未合并。

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

auto-merge: no

head a71c5e1d8b9127d00130ed19630b0fd27e51c0f7 — required CI green (Check, react-doctor, both Publish @getpie/cli preview, Continuous Releases, React Doctor), MERGEABLE (BEHIND). Prior no was on 3e03c44acf2a9fca3e342c8e957a0b88cff5ff64; re-reviewed this rebased head.

out of .agents/rules/auto-merge-pr.md allowed groups; fail closed:

  • test coverage removal / rewrite, not an allowed docs/tooling/lint/ui-presentation hunk: drops or reshapes tautological/stability assertions across session-ref, cron jitter/stableHash, handshake ready-prefix, server worktree branch-name, ssh remoteStateKey, and verify browser socket / CLI help-version suites
  • exclusions do not allow tests that change what is proven; stripping "cannot fail" asserts is still outside the three allowed groups

Din review-first: no squash-merge.

iamdin added 3 commits October 1, 2026 02:26
Remove pure self-comparisons, a help-flag probe that passes if the predicate is always false, and cases already owned at a stronger boundary.
@oxwen11
oxwen11 force-pushed the test/drop-low-value-tests branch from a71c5e1 to 92b025b Compare September 30, 2026 18:26
@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

After #415 and #417 passed independent acceptance and merged, GitHub-native rebase updated this PR from a71c5e1d8b9127d00130ed19630b0fd27e51c0f7 to 92b025b74f05a5c5a68a282e3b840f1cd2e6e347, targeting current main/trusted rules a9709fcc6d513f6786dd6c6ce44b13ba8a8e71c3. New CI is pending; no prior-head acceptance is carried forward. The restored CLI guard, SSH identity stability and recurring-jitter coverage still require independent full-PR re-review after all required checks succeed. No merge or deferred auto-merge armed.

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

auto-merge: no

head 92b025b74f05a5c5a68a282e3b840f1cd2e6e347 — required CI green (Check, react-doctor, Publish @getpie/cli preview, Continuous Releases, React Doctor), MERGEABLE (BEHIND). Prior no was on a71c5e1d8b9127d00130ed19630b0fd27e51c0f7; re-reviewed this updated head.

out of .agents/rules/auto-merge-pr.md allowed groups; fail closed:

  • test coverage removal / rewrite, not an allowed docs/tooling/lint/ui-presentation hunk: drops or reshapes tautological/stability assertions across session-ref, cron jitter/stableHash, handshake ready-prefix, server worktree branch-name, and verify browser socket dir (plus a small isHelpOrVersion add)

fail closed; no merge / no update-branch.

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

Independent acceptance — blocked, not merged

Head f24d02b658d1efe92c629f9616e9a77c000d9145; base/trusted rules 01f864fe30b4376714547bd6bd6576b5540e3d03. All four reported required checks passed before review. Full six-file diff and affected callers inspected: retained CLI denial/positive cases, SSH stability, recurring-jitter determinism, and worktree integration assertions address the previous coverage findings. No production-code or security-boundary change identified.

Fresh pinned reviewer worktree, locked dependencies and Turbo builds. Independent results:

  • Focused regression/integration tests: 11 files, 92 tests passed, no type errors.
  • Actual Verify CLI calls with an empty isolated root: --help, -h, --version, -v, and daemon --help succeeded; daemon status refused with no current run. Launch first.; no current run was created.
  • Chromium app E2E: 14 passed (real server/Pi process, seeded test provider; not a claim of real-provider acceptance).
  • Full Electron E2E: 9 passed, 5 failed, no retries. Failures: initial background renderer connection, renderer reload/new MessagePort, server-crash recovery, repeated-failure Retry, and terminal-failure Quit. Assertions timed out on expected UI/root elements.

Not accepted or merged. These failures have not been attributed to this test-only diff or established as baseline failures. No source fix, conflict resolution, protection bypass or deferred merge. A diagnosed/repaired E2E path with successful independent acceptance is required before proceeding; do not merely rerun until green. Local raw logs/daemon records are not uploaded because they may contain runtime credentials.

@oxwen11

oxwen11 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

auto-merge: no

head f24d02b658d1efe92c629f9616e9a77c000d9145 — required CI green (Check, react-doctor, Publish @getpie/cli preview, Continuous Releases, React Doctor), MERGEABLE (BEHIND). Prior no was on 92b025b74f05a5c5a68a282e3b840f1cd2e6e347; re-reviewed this updated head.

out of .agents/rules/auto-merge-pr.md allowed groups; fail closed:

  • test coverage removal / rewrite, not an allowed docs/tooling/lint/ui-presentation hunk: drops tautological/stability assertions across session-ref, cron jitter/stableHash, handshake READY_PREFIX, worktree branch naming, and agent-browser socket stability (apps/app/src/lib/session-ref.test.ts, packages/pi-loop/tests/cron.test.ts, packages/server/test/http/handshake.test.ts, packages/server/test/worktree.test.ts, tools/verify/src/runtime/browser.test.ts)
  • also adds a new isHelpOrVersion help/version case in tools/verify/src/surfaces/cli-run.test.ts — still not lint-format mechanical hygiene or docs-tooling-only
  • exclusions: not presentation-only; test edits that change what is asserted (even if product control flow untouched) are outside allowed groups → uncertainty / fail closed

fail closed — comment only (Din review-first; no merge).

@oxwen11

oxwen11 commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

Queue update after verified #423 merge — waiting at CI

Head a69e57eb59ec94149d89360b770ffc368cc9b397 now contains main/trusted-rules 0920277492d381b108b520b341c449bcc3e3dbd8 (behind_by=0 verified). GitHub performed a conflict-free merge update guarded by the previous exact head; no conflict resolution or source repair.

Required CI for this new head is queued/in progress at this observation, so no old-head review or acceptance is carried forward. The earlier failed Desktop acceptance remains unresolved; a main sync is not evidence that it passed.

No merge or deferred auto-merge. Continue only after the applicable blocker is resolved and the new version passes required CI → review → independent acceptance.

@oxwen11

oxwen11 commented Oct 5, 2026

Copy link
Copy Markdown
Owner Author

auto-merge: no

head a69e57eb59ec94149d89360b770ffc368cc9b397 — required CI green (Check, react-doctor, Publish @getpie/cli preview, Continuous Releases, React Doctor), MERGEABLE (BEHIND). Prior no was on f24d02b658d1efe92c629f9616e9a77c000d9145; re-reviewed this updated head.

Out of the allowed groups (docs/tooling, lint-format, ui-presentation):

  • test-suite semantics change, not docs/tooling or mechanical lint: removes assertions/cases in session-ref.test.ts, pi-loop/tests/cron.test.ts (stableHash), server/test/http/handshake.test.ts (ready-line prefix), server/test/worktree.test.ts (generateWorktreeBranchName), and tools/verify/.../browser.test.ts, and adds a new isHelpOrVersion case in cli-run.test.ts. Deciding which assertions "cannot fail" is a judgment call, so fail closed.
  • the earlier failed Desktop acceptance noted on this PR is still unresolved.

@oxwen11

oxwen11 commented Oct 5, 2026

Copy link
Copy Markdown
Owner Author

审查与验证记录:通过

  • Head 7842c37d9b84d4cb55a4600277cb5f39d9ff1eb2;base 与可信规则 commit:main d06b4aeee52a62d85124bd4999423b13e2871b92(head 已包含该 commit)
  • CI:当前 head 的必需检查全部 SUCCESS,包括 Check、react-doctor 和两项 Publish preview。
  • 不适用 CI-only 例外:本 PR 删除和修改了断言,所以完整走了审查和验证。

审查

逐项检查了被删除的断言,确认每项要保护的性质仍由其他测试覆盖:

  • stableHash 的稳定性:由 applyRecurringJitter 的 "is stable for the same task id" 测试覆盖,它对同一 id 断言 a === b。
  • sessionRefKey:实现是 JSON.stringify([environmentId, sessionId]),只依赖值;content-panel.test.ts 也有使用。
  • ready-line 前缀:由解析的往返测试覆盖。
  • pie/ 分支名:由 session 和 CLI 测试中的 ^pie/[0-9a-f]{8}$ 覆盖。
  • isHelpOrVersion:反例断言保留了,并新增了 --help、-h、--version、-v、daemon --help 的正例,比 main 更严格。
  • 不涉及产品代码、安全或持久化。

非阻塞:PR 描述仍写着"删除 isHelpOrVersion negatives",还提到 SSH state key,但当前 diff 的实际做法是保留反例并新增正例,也没有改 SSH 测试。描述已过时,以 diff 为准。

独立验证

在 reviewer 自己的干净 worktree 中检出 7842c37d9b84d4cb55a4600277cb5f39d9ff1eb2,用 pnpm install --frozen-lockfile 安装依赖,用 Turbo 构建上游,然后运行受影响的 6 个测试文件:session-ref、pi-loop cron、server handshake/worktree、verify browser、cli-run。结果:6 个文件、43 个测试全部通过。

本 PR 只改测试,没有运行时界面,所以不需要 UI 证据。

结论:通过,可以合并。如果 head 或 base 发生变化,从 CI 重新开始。

@oxwen11
oxwen11 merged commit 7011961 into main Oct 5, 2026
6 checks passed
@oxwen11
oxwen11 deleted the test/drop-low-value-tests branch October 5, 2026 16:59
@oxwen11

oxwen11 commented Oct 5, 2026

Copy link
Copy Markdown
Owner Author

已合并(squash)。merge commit 为 7011961484d80943646d994f0c61635a1a6b8f12,合并时的 head 为 7842c37d。审查与验证记录见 #416 (comment) 。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants