Unify CLI and App usage statistics activation after first disclosure - #5190
Conversation
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
huangruiteng
left a comment
huangruiteng
left a comment
There was a problem hiding this comment.
Request changes conclusion (author-owned PR; GitHub blocks formal self-review)
Exact head: 28d5b8839ceca7919795038a7eb2a5b15be84ac4; immutable base: 9eaacfbf2ff93d5386cee82ddf0847d9c739e78a.
[P1] Missing stderr is treated as a successful disclosure and contaminates JSON stdout. In begin, lines 80–88, sys.stderr=None raises AttributeError in the fileno probe, which is swallowed. Python then interprets print(..., file=None) as stdout, after which the notice is acknowledged. Through the actual cli_runtime.main(["version", "--format", "json"]), using isolated state and no collector access, base returns code 0 with parseable JSON and no state file; head returns code 0 with a disclosure prepended to stdout, unparseable JSON and a newly acknowledged statistics state. GUI/embedded Python can legitimately have no stderr. Please reject an absent disclosure stream before print/ACK, while retaining support for real captured or in-memory stderr. Add this ordinary-CLI regression test: stdout remains pure JSON, no notice state is created, and no subsequent measurement is authorized without a valid disclosure. Do not fix this by moving the notice to stdout.
动机
已有 usage policy 是告知后默认开启、随时关闭；但过去只在交互式 CLI 告知，App 与 script/Agent 的普通入口可能一直等待 notice。本 PR 试图使两个入口共享这个既有 policy，并把 App 的告知、关闭和持久化回读交付到真实 UI。这个方向有明确的现有文档/合入实现基础，不需要重新设计统计 owner。不过缺失 stderr 的反例使“告知后才启用、JSON stdout 干净”在其中一个真实入口失效，因此当前用户结果还不能判为完成。
改动思路
不应为了消除入口差异新增第二份 consent 状态或新的 collector。这里保留 TS usage_statistics 作为机器级 state/lock/generation/eligibility owner，CLI 和 HTTP 只分别提供“已展示”的入口证据。automatic_notice_required 从现有 notice、endpoint、policy 和开关派生，不是人工同步的重复 flag；App 的 attempted/choice 是短生命周期竞态 fence，不是 consent authority。维持原状最便宜但没有解决 App 告知缺口；绕过 notice 直接启用会违反现有契约。新增 UI 是合理的，修复当前问题只需要收紧 CLI stream boundary，不需要另起框架或 Python policy owner。
具体改动
12 个文件，269 行新增、26 行删除。最大生产新增是 87 行的 App notice component；配套变更在既有 API/client schema、typed runtime、CLI adapter、双语 usage 文档及测试。
关键代码讲解
begin：从 TTY-only 改成普通命令也向 stderr 告知；首次调用只告知并 ACK，不做测量。chat/serve-status非 TTY 和/dev/null继续 defer；但 absent stderr 被上述宽捕获错误放行，随后污染 stdout。automaticNoticeRequired/configure：自动 ACK 只允许首次或同 endpoint/policy 的 notice 更新；禁用、CI/DNT、consent-required、变更接收方仍不自动开启。锁内检查 expected notice，旧 ACK 不得替代用户明确选择，也不撤销 disable。UsageStatisticsNotice：读取 status、IntersectionObserver + 两帧和 visible/viewport 检查后 ACK。关闭操作递增 choice fence，避免迟到 ACK 回写覆盖关闭后的 UI；失败给详情重试入口，read-only workspace 不挂载此 component。_usage_statistics_update：只接受独立notice对象或 booleanenabled，复用同一个 TS owner。它没有扩展采集内容、Goal authority 或 Lark 权限。
正向 App 实验使用打包资产和真实本地 HTTP usage endpoint/TS state owner（其他 workspace 数据是 fixture）：中文/英文、桌面/390px 手机可展示告知，自动 ACK 后 sending=true，没有 last_sent_day 或 aggregate 测量；键盘关闭后 consent=disabled、刷新不再出现告知、只有一次 notice POST。截图已实际查看。负向 CLI 实验没有 mock begin 或 state owner，运行相同 base/head 原生入口；现有 green tests 不能排除这条反例。
对主干的风险
25 项 Python usage/API/goal 测试、35 项 TS usage policy/transport/aggregation 测试、control-plane typecheck、配置内 mypy 19 文件、CI 配置范围 Ruff、dashboard 构建及中英文打包 UI 实验通过。当前最强风险不是测试数，而是新增 stream fallback 在没有有效 stderr 时仍然记录告知；这会破坏脚本 JSON 消费者，并把未在指定告知通道展示的入口当作已告知。修复后需补上述 sys.stderr=None CLI 反例，再重跑 uv run --extra test python -m pytest -q tests/test_usage_ping.py tests/test_usage_goal.py 和 node --no-warnings --experimental-strip-types --test tests/control_plane_ts/usage_statistics*.test.ts，同时保留 App ACK/关闭竞态与 captured-stderr 场景。
语义与 CI 对齐
复用既有 usage notice vocabulary 和 state schema，不创建新的 consent authority。双语文档明确披露 script/Agent 和 App 的默认入口变化、首调用不测量、关闭与接收方限制。但当前实现违反已有“告知在 stderr、stdout 保持机器可读”契约；不能把 swallowing AttributeError 当成展示成功。没有更改统计预算或用新版预期掩盖失败。
本 review 未查询或等待远端 CI。完整 architecture suite 在 immutable base/head 均为 812 通过、2 个相同失败，两个失败均调用 registry I/O census；八条具体诊断及签名完全相同，且本 PR 不修改其 causal sites。这是单独的主干 manifest 维护问题，不是本次 request-changes 的原因。
我的整体评价
机器级 typed owner、可见 App 告知和一键关闭的组合是成比例且方向正确的改动，UI 本地结果也确实改善了入口体验；长期 state/retry/generation 路径继续沿用既有 owner，没有增加调度器或无界队列。整体交付仍被原生 CLI 反例阻塞，用户体验结论是 regression，不能因主路径通过而 approve。未来改动便利性已检查，最小修复是这个 adapter 的 stream guard 与原生入口回归覆盖；不需要扩大统计政策、权限或采集范围。本次不进行修复、合并或真实 collector 发送，修复后应在新 exact head 重做该反例和相关本地验证。
English verdict: REQUEST_CHANGES - absent stderr bypasses the disclosure channel, breaks JSON stdout, and acknowledges statistics activation.
|
Review frame for exact head |
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
An absent sys.stderr raised AttributeError in the fileno probe, which was swallowed, so print(..., file=None) wrote the notice to stdout and a statistics state was acknowledged without any disclosure being shown. Resolve the stream once, return without printing or acknowledging when no stream exists, and keep supporting captured or in-memory streams. Add the unit and ordinary-CLI regressions: stdout stays pure JSON, no notice state is created, no measurement is authorized, and a real stream still discloses before the acknowledgement. Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
huangruiteng
left a comment
huangruiteng
left a comment
There was a problem hiding this comment.
PR #5190 复审 — exact head c474d7a6e9f710df1ed053c8cf589396fefcd81b
Approval conclusion (author-owned PR; GitHub blocks formal self-approval)
Exact head: c474d7a6e9f710df1ed053c8cf589396fefcd81b；base f40f8379af4310d43e4cad0c4964d429abda5346。上一轮 Request changes 只有一条 P1：缺少 stderr 时
sys.stderr.fileno() 抛出的 AttributeError 被吞掉，随后 print(..., file=None) 把告知写进
stdout，既污染 JSON 又记录了一个并未展示过的告知。本 head 按同一评审给出的最小边界修复了它。
动机
既有 usage policy 是"告知后默认开启、随时关闭"，但过去只在交互式 CLI 告知，App 与 script/Agent
入口可能一直等待 notice。这包让两个入口共享既有 policy，并把 App 的告知、关闭与持久化回读交付到
真实 UI。唯一阻塞是缺失 stderr 的反例：嵌入式或 GUI Python 可以合法地没有 stderr，此时告知既没有
通道也没有 observer，却被当成"已展示"并 ACK。修复必须让"没有告知通道"与"有通道但被丢弃"区分开，
而不是把告知改到 stdout。
改动思路
在 notice 分支里把披露流解析一次：stream = sys.stderr，为 None 时直接返回，既不打印也不
acknowledge；随后 isatty() 判断、/dev/null 比较与 print(..., file=stream) 全部使用同一个
变量。保留内存流（如 io.StringIO）与真实捕获流：它们没有文件描述符但确实能展示告知，因此继续
走既有告知与 ACK 路径。没有新增 consent 字段、没有新的 Python policy owner，也没有改动
automatic_notice_required 的派生规则。
具体改动
相对最新 main 共 12 个文件 +269/-26；本次修复只动两个文件：loopx/usage_ping.py 增加披露流
guard，tests/test_usage_ping.py 增加两条回归。单元用例直接以 sys.stderr = None 调 begin，
断言无输出、无 state；真实入口用例通过 cli_runtime.main(["version", "--format", "json"]) 断言
stdout 仍是可解析 JSON、stderr 为空、state 未创建、sending 仍为 false，随后换成内存流再次运行，
证明只有"缺失流"被拒绝：内存流仍然展示告知并 ACK。两条用例在未修复的 head 上都会失败（告知文本
被前置到 stdout），修复后通过。
对主干的风险
修复后的行为变化只有一条：完全没有 stderr 的进程不再在本次调用中被记为已告知，也不再有 ACK；
它会保持未告知状态，直到出现真实披露流。这正是既有"告知在 stderr、stdout 保持机器可读"契约的
要求，也避免脚本 JSON 消费者被污染。其余路径（真实捕获流、内存流、/dev/null、chat/serve-status
非 TTY、CI/DNT/consent-required、禁用）行为不变。主干另有与本变更无关的红灯：control-plane
typecheck 在 sqlite_authority_store.ts 报错，以及 registry IO census 与 goal-instance binding
inventory 的 manifest 漂移，都在不含本 diff 的 origin/main 上复现。
我的整体评价
机器级 typed owner、可见 App 告知与一键关闭的组合方向正确且成比例；这次修复只收紧了 adapter 的
stream boundary，没有扩大统计政策、权限或采集范围，也不需要新的框架。修复后的验证覆盖 27 项
Python usage/Goal 用例、35 项 TS usage policy/transport/aggregation 用例、mypy 19 文件、Ruff 与
目标内 premerge 检查（质量回执 valid、blocking=false）；两条新回归在未修复 head 上确实失败，
证明它们约束的是本次缺陷而不是既有行为。结论：建议合入。
English verdict: APPROVE - exact head c474d7a. An absent stderr now returns before printing or
acknowledging, so the notice can no longer reach stdout and statistics are not activated without a
disclosure stream; captured and in-memory streams still disclose and acknowledge. Both new
regressions fail on the unfixed head and pass here, with the Python, TypeScript and goal-scoped
premerge checks green.
|
Merged as Exact-head evidence:
Repair carried in this head (the single P1 from the previous review):
Validation on this head: 27 Python usage/Goal cases; 35 TypeScript usage policy/transport/aggregation cases; Pre-existing
Admin bypass was used because the PR is authored by the maintainer and the exact head carries a published review, a valid change-quality receipt and a goal-scoped premerge pass; the remaining red checks belong to main, not to this diff. |
1 participant
CLI and App first-use statistics currently take different paths: an interactive terminal activates collection after disclosure, while a fresh Agent/script call or App requires explicit enable. This change applies notice-before-default-on to ordinary CLI calls and the live App under the existing opt-out policy.
CLI disclosure stays on stderr and leaves JSON stdout unchanged. The App paints a bilingual notice with turn-off and details controls before recording the shared acknowledgment. The TypeScript owner rejects automatic acknowledgment after opt-out, environment suppression, policy/recipient changes, or a stale notice. Background App services defer first disclosure to the frontend; null/broken stderr is not disclosure. Read-only views and hidden documents cannot acknowledge. Collection fields and transport remain unchanged; consent-required distributions still require explicit enable.
Validation: 25 Python entrypoint/API/Goal-observer tests; 35 TypeScript policy, transport and Goal-statistics tests; personal-workspace structure smoke; frontend typecheck and production builds including the packaged chat bundle. Packaged UI exercised with an isolated real settings backend: automatic activation without an enable click, no measurement during acknowledgment, turn-off and reload persistence, details navigation, keyboard focus, Chinese/English desktop and Chinese 390px mobile with no horizontal overflow. A simulated hidden-document transition verified the visibility guard. Public docs boundary scan and diff whitespace check passed. Vite reported its existing large-chunk warning.
Placement/future-facing pass: reuse the built-in usage-statistics runtime owner; centralize automatic-notice eligibility there and keep Python/React as adapters. No new capability or extension provider. Lark continues to inherit the same choice without a separate setting.
Visual review: a single first-use disclosure strip supplies collection purpose, scope, recipient and an immediate opt-out; it is dismissible and does not recur after acknowledgment. Normal returning-user and disabled states have no banner. The first-screen preview was approved before commit/push. Desktop/mobile views were checked against the shared design contract. Maintainer merge required for runtime/API changes.