feat(cosh-ng): add standalone SLS self-upload - #2715
Conversation
github-actions
Bot
added
component:cosh-ng
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5c53019aa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
[P1] headless 一次性模式对遥测等待策略的说明略显含糊，建议在代码注释中区分统一通道与自上传 handle 的语义，确保一次性模式只对自上传 JoinHandle 进行 await。
[P1] ECS 元数据探测失败时统一回退到公网域名，建议在注释或文档中显式说明该行为，便于运维评估成本与可靠性。
[P2] README 新增的遥测关闭说明未提及旧 ANOLISA_TELEMETRY_DISABLED 环境变量的废弃及迁移步骤，建议补充一句迁移提示。
🤖 Generated by Qoder • View workflow run
|
PR number: #2715 评审结论Findings
代码组织合规变更仅限 附带提醒（非阻断）： Open Questions
Validation
|
kongche-jbw
left a comment
kongche-jbw
left a comment
There was a problem hiding this comment.
Review baseline: f9c82a734ea43389815828ab903962f2b82d4b95...e5c53019aace72d25138094f5a5d6fc1d213d863
[P1] Check opt-out before creating the persistent identity
At src/cosh-ng/crates/cosh-core/src/headless.rs:173, Rust evaluates
build_sls_record() before emit() checks the opt-out marker. Record construction
calls installation_id() and creates ~/.copilot-shell/installation_id, so a user
who opted out before the first turn still gets a persistent cross-session identifier.
Possible direction: gate record construction first or make it side-effect free, and
assert that an existing sentinel leaves the installation-ID file absent.
[P1] Honor the managed-host telemetry disable marker
At src/cosh-ng/crates/cosh-core/src/sls.rs:47, only the per-user marker is checked.
On a host where anolisa telemetry disable created
/etc/anolisa/.telemetry_disabled, an absent cosh.jsonl sends the record through
the new direct-upload branch despite that explicit system-wide choice. Treat either
marker as disabling collection and cover the missing-channel case in a regression test.
[P1] Handle hardened ECS metadata responses
At src/cosh-ng/crates/cosh-core/src/sls.rs:166, the probe sends a tokenless GET,
and line 172 accepts the body without checking the HTTP status. ECS instances with
HttpTokens=required return 403; a nonempty error body is then cached as the region,
while an empty body falls back to the public Hangzhou route. The primary ECS path can
therefore misroute or lose every standalone upload. Obtain and send the metadata token,
validate a 2xx response and region syntax, and test 403 plus token-required responses.
[P2] Restore the installation-ID test override
At src/cosh-ng/crates/cosh-core/src/sls.rs:364, init_test_env() sets the override
only while its OnceLock initializes. The test at line 618 replaces it, then
EnvVarGuard::drop at line 530 removes it instead of restoring the prior value.
Later tests call the now-no-op initializer and can write the real HOME; parallel tests
also race on the process environment. Restore the previous value or inject the path,
serialize every environment user, and assert the suite never creates a HOME-level ID.
e5c5301 to
3ba0ca5
Compare
|
PR number: #2715 评审结论Findings
已解决（相对上一 head e5c5301）
代码组织合规变更仅限 附带提醒（非阻断）： Open Questions
Validation
|
a6063e5 to
887305d
Compare
|
@SunnyQjm @kongche-jbw 提交意见已修复，请再review下 |
kongche-jbw
left a comment
kongche-jbw
left a comment
There was a problem hiding this comment.
Review baseline: 69929468cd9aefe642bef994f9054d899a26ee8d...887305dff455b472fea4ab37f9b9f7257822e2de
[P1] Support token-required ECS metadata
At src/cosh-ng/crates/cosh-core/src/sls.rs:190, the probe still sends only a
tokenless GET. With HttpTokens=required, ECS returns 403 and lines 197-203
fall back to the public cn-hangzhou endpoint. A hardened ECS instance in
another region therefore sends every standalone record to the wrong regional
project, so the feature can lose all telemetry on its primary target. Please
obtain a token from /latest/api/token, include it on the metadata GET, retain
the v1 fallback where allowed, and add a token-required server test.
[P2] Read the complete metadata response
At src/cosh-ng/crates/cosh-core/src/sls.rs:193, one read() assumes the HTTP
headers and body arrive together. TCP may return the headers first; the parser
then sees an empty body, rejects a valid region, and permanently caches the
public Hangzhou fallback. Please read through the complete bounded response
(or the declared body length) and test a server that writes headers and body
separately.
[P2] Keep blocking metadata I/O off Tokio workers
At src/cosh-ng/crates/cosh-core/src/sls.rs:131, the spawned async task runs
synchronous connect, write, and read calls with separate one-second timeouts.
On a single-vCPU runtime, a slow metadata endpoint blocks the only worker, so
the one-second one-shot grace timer cannot run and interactive input also
stalls. Please use async I/O or spawn_blocking with a hard overall deadline,
and cover a one-worker runtime with a server that accepts but delays a reply.
[P2] Isolate enabled-telemetry tests from host opt-out
At src/cosh-ng/crates/cosh-core/src/sls.rs:789, the persistence test overrides
only the ID path, while installation_id() still reads the real user and
system sentinels. For example, setting
COSH_SYSTEM_TELEMETRY_DISABLED_PATH=/etc/hosts makes this test fail at line
803; the field test at line 462 has the same dependency. The targeted suite
therefore fails on an opted-out managed host. Please make init_test_env()
point both sentinel paths at absent temp files, with opt-out tests overriding
them explicitly.
[P2] Keep the collection disclosure consistent with the payload
src/cosh-ng/README.md:186 and README_zh.md:172 say session duration is
collected, but build_sls_record() names the argument _duration and never
adds it to the record. Every uploaded payload therefore omits this documented
metric. Please either add the intended schema field and assert its value, or
remove the claim from both disclosures.
|
PR number: #2715 评审结论Findings
已解决（相对上一 head 3ba0ca5）
代码组织合规变更仅限 Open Questions
Validation
|
887305d to
7d82160
Compare
|
@SunnyQjm @kongche-jbw 关于共同提到这个问题说明： At src/cosh-ng/crates/cosh-core/src/sls.rs:190, the probe still sends only a 感谢指出 P1。IMDSv2 token 支持我们在实现时讨论过，但决定不在本 PR 中引入，原因如下： |
kongche-jbw
left a comment
kongche-jbw
left a comment
There was a problem hiding this comment.
Review baseline: 69929468cd9aefe642bef994f9054d899a26ee8d...7d82160ed759593cca1b8c2d1fb64d84393e5bb6
[P1] Keep test telemetry off the production endpoint
At src/cosh-ng/crates/cosh-core/src/sls.rs:87, every process whose unified
channel is missing now probes the real metadata IP and launches a POST to the
default SLS project. Existing binary fixtures such as
src/cosh-ng/crates/cosh-core/tests/session_recovery.rs:144 and
src/cosh-ng/crates/cosh-core/tests/compaction_lifecycle.rs:70 create temporary
homes but set neither an opt-out marker nor a local endpoint; together they
start dozens of one-shot processes, which also await this task. The normal
cargo test -p cosh-core gate can therefore write synthetic UUIDs and metrics
into production telemetry and depends on external network timing. Please make
binary tests opt out by default, then add one explicit self-upload E2E against
an injected local server that asserts the actual path and body without reaching
the public service.
[P2] Keep the metadata probe off Tokio workers
At src/cosh-ng/crates/cosh-core/src/sls.rs:92, emit() performs blocking
TcpStream I/O before spawning the upload. All production call sites, such as
src/cosh-ng/crates/cosh-core/src/headless.rs:192, invoke it from an async
result path. The first standalone turn on a slow or non-ECS host therefore
delays its result through the per-operation timeouts and occupies a runtime
worker. On a single-vCPU runtime, other timers and tasks cannot run during that
probe. Please use async
I/O or a blocking pool with a hard overall deadline, and add a one-worker
regression where a stalled metadata server does not prevent a timer advancing.
7d82160 to
b368e37
Compare
|
@kongche-jbw 帮忙继续check下 |
kongche-jbw
left a comment
kongche-jbw
left a comment
There was a problem hiding this comment.
Review baseline: 522b680ca53d...7bdb237fcf39
[P1] Finish isolating telemetry tests from production
src/cosh-ng/crates/cosh-core/tests/tool_approval.rs:21 still starts a real
cosh-core process without an opt-out marker or a local upload endpoint. The
same pattern remains in tests/jsonl_protocol.rs:61 and
tests/sls_integration.rs:94; the new unit regression at src/sls.rs:785
also enables self-upload without redirecting its target. Each user turn with
the unified file absent can therefore probe real metadata and spawn a POST to
the default SLS project during the normal test gate. Conversely, the explicit
upload test at tests/telemetry_upload.rs:96 inherits the host's real system
sentinel and fails on an opted-out managed host. Possible direction: make the
shared binary-test command safe by default, give the unit regression a local
target, and override both sentinels to controlled absent paths only in the
explicit upload test.
[P2] Keep region discovery off the result critical path
At src/cosh-ng/crates/cosh-core/src/headless.rs:192 and :632,
emit().await now completes before the result is written at :206 and :646.
The standalone branch waits on the metadata probe under a two-second deadline
at src/cosh-ng/crates/cosh-core/src/sls.rs:94. A metadata server that accepts
but stalls therefore delays the first success or error result by up to two
seconds; one-shot mode can then wait another second for the POST. This avoids
worker starvation but bypasses the stated one-second exit-delay budget.
Possible direction: keep discovery behind the returned upload handle, or emit
the result before awaiting it, and assert result latency under a stalled
metadata server rather than only checking that another timer advances.
[P2] Read the complete request in the upload integration test
At src/cosh-ng/crates/cosh-core/tests/telemetry_upload.rs:45, the mock server
captures one read() and immediately replies. TCP may deliver the headers and
body separately, so this test can observe only the headers, fail its body
assertions, and close the socket before the client finishes writing. Read
through the header terminator and declared Content-Length before capturing
and replying; a split-write server case would pin the behavior.
[P2] Make the validation count match this head
The PR body says the documented sls::tests command passes 11 tests, but that
exact command runs 18 tests on this head. Please update the count and coverage
summary so the validation claim is reproducible after the added regressions.
7bdb237 to
5eb85c0
Compare
|
@kongche-jbw 请帮忙继续检查下 |
kongche-jbw
left a comment
kongche-jbw
left a comment
There was a problem hiding this comment.
Review baseline: af879316482d...5eb85c0e408c
[P1] Make the metadata deadline stop the blocking probe
At src/cosh-ng/crates/cosh-core/src/sls.rs:94, timing out the JoinHandle
does not cancel a running spawn_blocking task. A metadata endpoint can send
one byte just under each one-second read timeout; the loop at line 240 can then
run for hundreds of seconds while holding REGION. Every later turn starts
another blocking task that waits on that mutex, so repeated turns can consume
the blocking pool and delay unrelated work. The one-shot process also awaits
this two-second probe before applying the one-second upload grace at
headless.rs:226, so its exit budget is not actually capped at one second.
Possible direction: enforce an absolute deadline inside the I/O operation,
share one in-flight probe, and test repeated emits against a slow-drip server
while asserting both pool availability and total process-exit latency.
[P1] Keep the opt-out boundary fail closed and usable on a fresh install
At src/cosh-ng/crates/cosh-core/src/sls.rs:59, Path::exists() treats a
dangling sentinel symlink or a non-ENOENT stat error such as EACCES as
absent, so an unprivileged process can upload despite the system-wide opt-out.
The documented commands at src/cosh-ng/README.md:194 and :201 also fail
before first launch when ~/.copilot-shell or /etc/anolisa does not exist,
which is the normal standalone-install case. Please allow telemetry only after
an explicit ENOENT, document creation of the parent directories in both
languages, and cover dangling, inaccessible, and fresh-install paths.
[P2] Preserve one installation ID across concurrent first starts
At src/cosh-ng/crates/cosh-core/src/sls.rs:389, each process writes its own
temporary UUID and rename() replaces an existing destination on Unix. Two
first-start processes can therefore both return different IDs while the last
rename wins; one process uploads an ID that is never persisted, breaking the
promised cross-session correlation. Use a create-if-absent/lock protocol and
read the winner on contention, with a concurrent first-start regression test.
[P2] Isolate the unified-channel test from the host opt-out
At src/cosh-ng/crates/cosh-core/tests/sls_integration.rs:20, the enabled
telemetry test sets a temporary HOME but leaves the system sentinel override
inherited. Running that exact test with
COSH_SYSTEM_TELEMETRY_DISABLED_PATH=/etc/hosts fails at line 52 because the
record is correctly dropped. Point both sentinel variables at controlled
absent paths so this integration target passes on opted-out managed hosts.
[P2] Parse Content-Length case-insensitively in the upload mock
At src/cosh-ng/crates/cosh-core/tests/telemetry_upload.rs:63, the mock only
recognizes Content-Length:. Reqwest emits content-length:, so this parser
gets zero and replies as soon as it sees the headers. The test passes only when
headers and body arrive in the same read; a split body is still truncated and
the server can close before the client finishes writing. Parse header names
case-insensitively and assert that the captured body length matches the header.
[P2] Update the PR's reproducibility claims for this head
The PR body's documented cargo test -p cosh-core --bins -- sls::tests
command runs 18 tests on this head, not the stated 11. Its Documentation
section also links .qoder/plans/2026-08-18-cosh-ng-standalone-telemetry-design.md,
but that file is absent from the reviewed tree. Please update the count and
coverage summary and either add the referenced document or remove the link.
0a53a7c to
15744f6
Compare
|
@kongche-jbw 帮忙再次检查下 |
15744f6 to
f3b4e58
Compare
|
@SunnyQjm review意见已修改，帮忙继续检查下 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3b4e5865c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
When cosh-ng is installed without anolisa unified telemetry, the cosh.jsonl channel may not exist, leaving metrics stranded. Add a standalone self-upload path: emit() checks sentinel opt-out first, then cosh.jsonl existence -- present -> append (unified), absent -> spawn background POST to SLS PutWebtracking (standalone). The two paths are mutually exclusive per call, preventing double-write. Key decisions: - Opt-out via sentinel files: ~/.copilot-shell/telemetry_disabled (per-user) and /etc/anolisa/.telemetry_disabled (system-wide). No env var: sentinel files enable runtime dynamic control and persist across restarts. - emit() returns Option<JoinHandle>: one-shot prompt mode awaits up to 1s before exit to avoid losing telemetry when runtime.shutdown_timeout(100ms) cancels pending tasks; interactive mode fire-and-forgets. - Region probe via tokio::net::TcpStream with an absolute tokio::time::timeout deadline. The probe is cached in a tokio::sync::OnceCell so concurrent first callers share one in-flight probe; a slow or drip-feeding metadata endpoint cannot pin a runtime worker or the blocking pool. Reads the full response in a loop because TCP may split headers and body across packets. Validates HTTP 2xx status and region format (alphanumeric + hyphen) before caching; malformed responses fall back to cn-hangzhou. - installation_id at ~/.copilot-shell/installation_id; does not create the file when telemetry is disabled. Concurrent first-start processes use create-new/open; losers poll briefly for a valid UUID and repair an empty or invalid file, so all callers converge on the same persistent UUID and a crashed creator cannot leave a permanently empty file. - SLS PutWebtracking requires all __logs__ field values to be strings; build_upload_body stringifies Number/Bool, drops Null. - HTTP timeouts: 2s for POST, 1s for metadata probe. Review fixes folded in: - Move metadata probe off the runtime worker with async I/O and an absolute deadline; share one in-flight probe across concurrent callers via tokio::sync::OnceCell. - Emit the headless success/error result before awaiting sls::emit() so a stalled probe does not delay the visible turn result. - Make binary integration tests safe by default via tests/common/mod.rs::cosh_core_command, which sets HOME and opts out of telemetry. Apply it to tool_approval, jsonl_protocol, and the missing-file case in sls_integration. - Redirect the sls.rs unit-regression upload target to a local non-routable address and override both sentinel env vars in telemetry_upload.rs so the explicit upload test is independent of the host's opt-out state. - Read the complete HTTP request in telemetry_upload.rs mock server by parsing Content-Length case-insensitively, so split TCP packets do not break body assertions and the captured body length matches the header. - Make the opt-out sentinel check fail-closed: only ENOENT means the sentinel is absent; dangling symlinks and permission errors keep telemetry disabled. - Use atomic create-new/open for installation_id; losers poll for a valid UUID and repair empty/invalid files by overwriting them, preventing a crashed first-start process from breaking cross-session correlation. - Isolate the unified-channel sls_integration test from the host's system-level opt-out by pointing both sentinel env vars at absent temp paths. Documentation: - Remove "session duration" from the data-collection disclosure in README.md/README_zh.md; build_sls_record does not include it. - Add `mkdir -p` to the opt-out sentinel commands in both READMEs so they work on a fresh install before ~/.copilot-shell or /etc/anolisa exist. - Add telemetry opt-out instructions to both English and Chinese user-guide configuration pages under docs/user-guide/, not just the component README. Known limitations: - One-shot await caps upload grace period at 1s; telemetry may be dropped on very slow networks. - No retry, no batching, no persistence -- acceptable for L1 anonymous operational metrics. - Sentinel paths not CLI-configurable (test-only env vars). - IMDSv2 token-required metadata is not supported; the probe falls back to the public cn-hangzhou endpoint on hardened ECS instances. Assisted-by: Qoder:1.22.0 Signed-off-by: Kailong Zhou <zhoukailong.zkl@alibaba-inc.com>
f3b4e58 to
c57850d
Compare
3 participants
Why
When cosh-ng is installed without anolisa unified telemetry, the
cosh.jsonlchannel may not exist, leaving operational metrics stranded — never uploaded. cosh-ng needs a standalone self-upload path so it can report anonymous L1 telemetry directly to SLS PutWebtracking without depending on the anolisa uploader service.What changed
sls.rs: Newemit()replaces directappend_sls_log()calls — checkscosh.jsonlexistence: present → append (unified channel), absent →tokio::spawna background POST to SLS PutWebtracking (standalone). The two paths are mutually exclusive per call. ReturnsOption<JoinHandle>so one-shot mode can await.sls.rs: Newbuild_upload_body()stringifies all__logs__field values (Number/Bool → String, drop Null) and injects__time__+cosh_upload_source: "cosh-ng-direct", fixing SLSPostBodyInvalid400 errors.sls.rs: New region probe viastd::net::TcpStream(notreqwest::blocking) to avoid nested tokio runtime panic; result cached inOnceLock, timeout 1s.sls.rs: New opt-out via~/.copilot-shell/telemetry_disabledsentinel file — enables runtime dynamic control (touch/rm) without restarting cosh-core.headless.rs: One-shot prompt mode awaitsJoinHandlebefore process exit to prevent data loss whenruntime.shutdown_timeout(100ms)cancels pending tasks; interactive mode stays fire-and-forget.README.md/README_zh.md: Added Data Collection section documenting telemetry scope and opt-out.Related issue
no-issue: standalone telemetry self-upload is a new feature not tracked by an existing issue
User / Agent impact
touch ~/.copilot-shell/telemetry_disabled.Risk and compatibility
CLI/config change: new opt-out mechanism (sentinel file). No migration needed — there was no prior opt-out on main. The unified-channel
append_sls_log()path is preserved unchanged insideemit().Rollback:
rm ~/.copilot-shell/telemetry_disabledre-enables telemetry. To fully revert, replaceemit()calls withappend_sls_log()inheadless.rs; unified channel behavior is unaffected.Validation
Environment: macOS arm64 (development). Linux ECS production is the primary target for standalone self-upload.
Documentation and rollback
.qoder/plans/2026-08-18-cosh-ng-standalone-telemetry-design.mdrm ~/.copilot-shell/telemetry_disabledre-enables; replaceemit()calls withappend_sls_log()to remove standalone upload path entirely.