Skip to content

feat(telemetry): emit task_processed from every task source, not just messaging bridges - #2274

Merged
john-the-dev merged 11 commits into
sonichi:mainfrom
john-the-dev:feat/task-processed-all-sources
Jul 29, 2026
Merged

john-the-dev merged 11 commits into
sonichi:mainfrom
john-the-dev:feat/task-processed-all-sources

Conversation

@john-the-dev

Copy link
Copy Markdown
Collaborator

Problem

task_processed — the activation signal that separates installed from actually does work — fired from only the discord / slack / telegram bridges. Every other task-accept surface created tasks but never counted:

  • voice / chat / context-drop (local LocalTaskBackend.submitTask)
  • phone (conversation-server.ts delegation)
  • relay-voice / API / incoming call / SMS / voicemail (agent-api.py)

So DAU/WAU under-reported anyone who doesn't route through a messaging platform — including the computer-direct path.

Fix

Emit at each task-accept point, keyed on the task body's own source: header so every surface counts under its true bucket.

  • src/telemetry.py — add a flush kwarg to task_processed + a _cli_main CLI entrypoint (python3 src/telemetry.py task_processed <source>) so non-Python creators emit through one source of truth. Flush path guarantees the short-lived subprocess delivers before exit.
  • src/task-delegation.ts — LocalTaskBackend.submitTask emits; parseTaskSource exported + unit-tested.
  • skills/phone-conversation/scripts/conversation-server.ts — phone tasks emit.
  • src/agent-api.py — relay-voice delegation, incoming call, SMS, voicemail, and API task creation emit, reading source from the body.

Fire-and-forget everywhere: never blocks, throws, or breaks task creation; no-ops when telemetry is opted out / unconfigured; carries only the coarse source bucket — never task text, ids, user, or channel.

Live test results

tests/telemetry.test.py ........ ALL PASS (23 checks)
  ok  CLI entrypoint delivers source-tagged task_processed (voice)
  ok  _cli_main dispatches task_processed/feature_used + rejects bad args

tests/task-delegation.test.ts ... tests 6  pass 6  fail 0
  ✔ parseTaskSource reads the surface bucket that task_processed telemetry is tagged with

tsc --noEmit → clean

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Jul 22, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 68%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • src/agent-api.py (100%)
  • src/cron-runner.py (100%)
  • src/github-webhook.py (100%)
  • src/telemetry.py (100%)

Summary

  • Total: 25 lines
  • Missing: 0 lines
  • Coverage: 100%

… messaging bridges

task_processed — the activation signal separating "installed" from "actually
does work" — fired from only the discord/slack/telegram bridges. Voice, local
chat, context-drop, phone, and the relay-voice / API / SMS / voicemail surfaces
created tasks but never counted, so DAU/WAU under-reported anyone who doesn't
route through a messaging platform (the owner's own computer-direct path
included).

Close the gap at each task-accept point, keyed on the task body's own `source:`
header so every surface counts under its true bucket:
- src/telemetry.py: add a `flush` kwarg to task_processed + a `_cli_main` CLI
  entrypoint (`python3 src/telemetry.py task_processed <source>`) so non-Python
  creators emit via one source of truth. Flush path guarantees the short-lived
  subprocess delivers before exit.
- src/task-delegation.ts: LocalTaskBackend.submitTask emits (voice/chat/
  context-drop); parseTaskSource exported + unit-tested.
- skills/phone-conversation/scripts/conversation-server.ts: phone tasks emit.
- src/agent-api.py: relay-voice delegation, incoming call, SMS, voicemail, and
  API task creation emit, reading source from the body.

Fire-and-forget everywhere: never blocks, throws, or breaks task creation;
no-ops when telemetry is opted out / unconfigured; carries only the coarse
source bucket — never task text, ids, user, or channel.

Live tests: tests/telemetry.test.py 23/23 pass (incl. CLI entrypoint delivers
source-tagged event + _cli_main branch coverage); tests/task-delegation.test.ts
6/6 pass (incl. parseTaskSource); tsc --noEmit clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QPBv1rU2s1VrdtZCVd2o8r
@john-the-dev
john-the-dev force-pushed the feat/task-processed-all-sources branch from 42315b3 to 8fc4283 Compare July 22, 2026 18:01

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The focused Python telemetry and agent-API suites pass, but the new API emission path does not enforce the stated anonymous coarse-bucket contract.

Comment thread src/agent-api.py Resolved
The API/web task path emits task_processed with a `source:` read from the
caller-supplied task body, so an arbitrary unbounded string could reach
PostHog as the `source` property — contradicting the coarse-only contract
and risking accidental identifier/secret leakage or unbounded cardinality
(CR sonichi#2274, qingyun-wu).

Fix at the single source of truth: task_processed() now collapses its
source through `_coarse_source()` against a fixed allowlist of known
surfaces, mapping anything unrecognized to "unknown". This covers every
caller — the Python in-process emit, the `python3 src/telemetry.py
task_processed <source>` CLI (TS task-delegation/phone paths), and the
agent-api emit — without duplicating validation per surface.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QPBv1rU2s1VrdtZCVd2o8r
@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Fixed in d8bab76. Rather than validate per-surface, I put the guard at the single source of truth — task_processed() now collapses its source through a new _coarse_source() helper against a fixed allowlist of known surfaces (voice/phone/chat/api/discord/slack/telegram/twilio_*/cron/health-check/…), mapping anything unrecognized to unknown. That covers every caller — the Python in-process emit, the python3 src/telemetry.py task_processed <source> CLI (TS task-delegation + phone paths), and the agent-api _emit_task_processed you flagged — with no per-path duplication.

So the caller-supplied source: header on the web/API path can no longer leak an identifier/secret or inflate cardinality. Added a regression test asserting a hostile source (s3cr3t-token-leak) collapses to unknown end-to-end plus unit coverage on the helper (case/space normalization, empty, injection-ish input). telemetry.py stays at 100% diff coverage.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update resolves the prior telemetry-boundary blocker: all task sources now pass through a fixed coarse allowlist, unknown and hostile values collapse to unknown, and focused tests cover the boundary.

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cold review (requested in #dev). Blocking — the "every task source" claim isn't fully met, and the PR's own allowlist contradicts the code:

  • src/telemetry.py adds cron and github to _KNOWN_SOURCES, but neither task writer emits task_processed:

    • src/cron-runner.py writes source: cron task files (write_text at line 228) with no telemetry emit — not touched by this PR.
    • src/github-webhook.py writes source: github task files (line 174) with no emit — also untouched.

    So DAU/WAU still under-reports cron- and webhook-driven activity, and the allowlist advertises two buckets that will never fire — the exact gap this PR sets out to close.

Resolution (author's call): either add the emit at those two sites, or drop cron/github from _KNOWN_SOURCES and scope the claim to interactive sources so the allowlist and the emitters agree.

The rest is solid: the five emit sites added (task-delegation local backend, phone, agent-api relay / twilio / generic POST /task) are correct and single-emit, fire-and-forget discipline holds throughout, and the _coarse_source allowlist sanitization + _cli_main entrypoint are genuinely well-tested in tests/telemetry.test.py.

Nit: no test asserts the emit actually fires at the two new backend sites — tests/task-delegation.test.ts only exercises the pure parseTaskSource helper, and agent-api-delegation.test.py asserts the task file is written, not the emit. A spy/stub per site would lock the behavior in against a future refactor.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed after the branch merged current main. Range-diff shows the two PR commits are unchanged; the full current telemetry diff retains the fixed coarse-source allowlist and resolved review thread. No merge-resolution regression or new blocker found.

liususan091219 cold-review CR on sonichi#2274: the PR added 'cron' and 'github'
to the telemetry _KNOWN_SOURCES allowlist, but neither task writer emitted
task_processed — so DAU/WAU still under-reported cron- and webhook-driven
activity and the allowlist advertised two buckets that could never fire.

Add a fire-and-forget task_processed emit at each write site, mirroring the
discord/slack/telegram bridges + agent-api:
- src/cron-runner.py emit_task() -> task_processed('cron')
- src/github-webhook.py do_POST -> task_processed('github')

Behavioral regressions (not source-grep): cron-runner.test.py asserts
emit_task fires task_processed('cron') once via a telemetry spy;
github-webhook-access-tier.test.py drives do_POST end-to-end (signed
request through the real handler) and asserts the 'github' emit. Full
suites green (cron ALL PASSED, github 31/31, telemetry 24/24).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BfqS1Z5rwiCqxMwQcp12dy
@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Fixed in 7d7873b — thanks, you were exactly right. The allowlist advertised cron and github, but the two writers never emitted, so both buckets could never fire.

Change: added a fire-and-forget task_processed emit at each write site, mirroring the discord/slack/telegram bridges + agent-api (import-guarded, never blocks or breaks task emission, no content/ids):

  • src/cron-runner.py emit_task() → task_processed("cron")
  • src/github-webhook.py do_POST → task_processed("github") (after the task file is written)

Behavioral regressions (not source-grep):

  • tests/cron-runner.test.py::test_emit_task_emits_cron_telemetry — spies telemetry.task_processed, calls emit_task(...), asserts it fired ("cron",) exactly once.
  • tests/github-webhook-access-tier.test.py::_test_do_post_emits_github_telemetry — drives do_POST end-to-end (signed request through the real WebhookHandler), asserts a task was written and ("github",) was emitted.

Suites green: cron-runner ALL PASSED, github-webhook 31/31, telemetry 24/24. git diff --check clean. Now every declared source in _KNOWN_SOURCES has a real emit path. Ready for re-review.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new cron and GitHub writer tests pass, but the cron event still is not reliably delivered from its short-lived process.

Comment thread src/cron-runner.py Outdated
try: # pragma: no cover — fire-and-forget glue; logic in tests/telemetry.test.py
from telemetry import task_processed # sibling module (src/ on sys.path)

task_processed("cron")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cron-runner.py is a one-shot launchd process (__main__ calls run() and then exits), while task_processed() defaults to a daemon-thread send. telemetry.py explicitly notes that this sender is killed when a short-lived process exits, so this new cron bucket can still emit nothing. Use the bounded synchronous path (task_processed("cron", flush=True)) or another completion mechanism, and add a subprocess-level regression that verifies receipt after the runner exits; the current monkeypatch only proves the function was called.

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Addressed the cron one-shot lifecycle blocker in db23aef.

  • _emit_cron_telemetry() now calls task_processed("cron", flush=True), using telemetry’s bounded synchronous path before the launchd process exits.
  • Added a subprocess regression with a stub receiver; the parent verifies the telemetry receipt exists only after the child runner process has exited.

Actual verification at db23aef:

$ python3 tests/cron-runner.test.py
...
  ok: cron telemetry subprocess exits cleanly ()
  ok: synchronous cron telemetry receipt exists after runner exits
...
ALL PASSED

$ python3 tests/telemetry.test.py
...
ALL PASS (24 checks)

$ git diff --check
# no output

@qingyun-wu please re-review the current head when CI finishes.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 2668680.

Approval signal. The new cron-runner change addresses the remaining blocker: cron-runner is a one-shot launchd process, and it now calls task_processed("cron", flush=True), so telemetry uses the bounded synchronous path before the process exits instead of relying on a daemon sender that can be killed. The new subprocess regression test proves a cron telemetry receipt exists after the runner process exits, and the merge-from-main head preserves the PR-specific telemetry diff.

Focused validation:

  • git diff --check origin/main...HEAD
  • python3 tests/cron-runner.test.py
  • python3 tests/telemetry.test.py (rerun outside the sandbox because it binds a loopback HTTP server)
  • python3 tests/github-webhook-access-tier.test.py
  • python3 tests/agent-api-delegation.test.py (rerun outside the sandbox because it binds a loopback HTTP server)

Reviewed by Qingyun's Personal Codex.

@john-the-dev
john-the-dev dismissed liususan091219’s stale review July 26, 2026 04:21

Resolved on current head 2668680: both cron and GitHub task writers now emit task_processed; cron uses flush=True so the one-shot process delivers synchronously, with a subprocess post-exit receipt regression. Current-head qingyun approval independently reverified the focused cron/telemetry/webhook suites.

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 2668680. CI green, MERGEABLE, not draft. Verified on this checkout that only slack/discord/telegram emit today (src/slack-bridge.py:1146, discord-bridge.py:3671, telegram-bridge.py:936), so the gap is real. Privacy: payload is {source: <allowlisted bucket>, $ip:"", $geoip_disable} + install UUID - no task text, ids, user, or channel; _coarse_source collapses hostile values; capture() still honors the live opt-out. No hardcoded paths in added lines.

Blocking:

  1. skills/phone-conversation/scripts/conversation-server.ts - the telemetry spawn has no 'error' listener. try/catch does not catch spawn's async 'error' event; I reproduced the unhandled-'error' crash on node v25.9. Line 1878 turns that into process.exit(1), killing a live call - the opposite of the comment's "telemetry must never break the call". The trigger is simply python3 not resolvable on PATH, on the first task delegation of a call. src/task-delegation.ts's execFile callback form is safe (verified: ENOENT reaches the callback). Fix: .on('error', () => {}).

  2. context-drop is allowlisted in src/telemetry.py and asserted in tests/task-delegation.test.ts, but its writer at src/task-bridge.ts:565-581 writes the task via raw writeFileSync and never calls LocalTaskBackend.submitTask, so the bucket can never fire - the same contradiction @liususan091219 blocked on for cron/github.

Non-blocking: opted-out hosts still spawn a python3 process per task (the gate is inside the child); the phone path's join(dirname(_phoneSkillDir),'..','src',...) mis-resolves if run from dist/conversation-server.js, which exists here - silent no-emit rather than a crash, and I did not verify whether anything runs the dist build; and no test exercises the TS-to-python3 emit end to end, so "voice/chat/phone now count" is asserted, not proven.

I did not run the suites or a live phone/voice round trip.

Not ready to merge - changes requested on items 1 and 2; both look like small fixes.

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Addressed both current-head blockers in 29842dd1:

  • phone telemetry now attaches an error listener before unref(), so an ENOENT cannot reach the process-level fatal handler and kill a live call
  • the raw context-drop writer now emits task_processed from the exact serialized task body

Added structural regressions for both activated paths. tests/task-delegation.test.ts (8/8), full TypeScript typecheck, and diff --check pass locally. Please re-review the new head.

@qingyun-wu

Copy link
Copy Markdown
Collaborator

Blocking status for current head 29842dd1a7973c8c90366ae029ebad8d21d94bc7.

The code-level blockers I rechecked look addressed: phone telemetry now attaches an error handler before unref(), and the context-drop writer emits telemetry from the exact serialized task body it writes. Focused local verification in /private/tmp/monitor-sutando-20260727-1330/pr-2274 passed:

  • git diff --check origin/main...HEAD - PASS
  • npm ci --cache /private/tmp/monitor-sutando-20260727-1330/npm-cache-2274 - PASS
  • npm run typecheck - PASS
  • npx tsx --test tests/task-delegation.test.ts - PASS
  • python3 tests/telemetry.test.py - PASS
  • python3 tests/cron-runner.test.py - PASS
  • python3 tests/github-webhook-access-tier.test.py - PASS

I am not posting an approval signal yet because the required GitHub tsc + tests (clean install) check is red on this same head. The Actions log fails with a bare Segmentation fault (core dumped) immediately after tests/bridge-skill-path-resolution.test.py reports PASS. Please get that required check back to green or document/rerun if it is an infrastructure flake; until then the head is not merge-ready.

Reviewed by Qingyun's Personal Codex.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval signal for current head 29842dd1a7973c8c90366ae029ebad8d21d94bc7.

The code-level blockers I rechecked on this head remain addressed: phone telemetry now attaches an error handler before unref(), and the context-drop writer emits telemetry from the exact serialized task body it writes. I previously held approval only because the required tsc + tests (clean install) check was red from a bare runner-level segmentation fault; the rerun is now green, along with diff coverage and the other required checks.

Focused verification already performed in an isolated /private/tmp checkout for this same head:

  • git diff --check origin/main...HEAD - PASS
  • npm ci - PASS
  • npm run typecheck - PASS
  • npx tsx --test tests/task-delegation.test.ts - PASS
  • python3 tests/telemetry.test.py - PASS
  • python3 tests/cron-runner.test.py - PASS
  • python3 tests/github-webhook-access-tier.test.py - PASS

Reviewed by Qingyun's Personal Codex.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval signal for current head 326fae93cab270252ff7c37d556c795901cffc92.

Re-reviewed the merge-refreshed telemetry/delegation head. The previously checked blockers remain addressed: phone telemetry attaches an error handler before unref(), context-drop telemetry is emitted from the exact serialized task body, delegation endpoints stay bearer-gated and path-confined, and the local/relay task-delegation boundary remains explicit.

Focused verification in an isolated /private/tmp checkout:

  • git diff --check origin/main...HEAD - PASS
  • python3 -m py_compile src/agent-api.py src/cron-runner.py src/github-webhook.py src/telemetry.py tests/agent-api-delegation.test.py tests/cron-runner.test.py tests/github-webhook-access-tier.test.py tests/telemetry.test.py - PASS
  • python3 tests/agent-api-delegation.test.py - PASS (run outside the sandbox for its local loopback server)
  • python3 tests/telemetry.test.py - PASS (run outside the sandbox for its local loopback server)
  • python3 tests/cron-runner.test.py - PASS
  • python3 tests/github-webhook-access-tier.test.py - PASS
  • npm ci - PASS
  • npm run typecheck - PASS
  • npm run typecheck:skills - PASS
  • npx tsx --test tests/task-delegation.test.ts - PASS (run outside the sandbox for tsx IPC)

GitHub CI is green on this head. No blocking findings from this pass.
Reviewed by Qingyun's Personal Codex.

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at 326fae93c — my blocking CR addressed (cron-runner + github-webhook emit sites, CR-citing spy tests) + context-drop/phone-spawn coverage; suites run green; merge-only delta since.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved exact head d14641c8754059b02b081c666421f9929ae13dc1.

This is a clean current-main refresh: git range-diff shows all five topic commits unchanged. The cumulative path still emits only the fixed coarse source allowlist (unknown/hostile values collapse to unknown), never task content or IDs; live opt-outs remain checked on every call, and short-lived cron emission uses the bounded synchronous path.

Exact-head evidence:

  • agent API delegation E2E — PASS
  • telemetry suite — 24/24
  • cron-runner suite — PASS, including subprocess delivery before exit
  • GitHub webhook access-tier — 31/31
  • task-delegation TypeScript — 8/8
  • Python compilation, git diff --check, and repository hardcoded-path scan — PASS
  • All current GitHub checks and CLA — green

Worst-case added work is best-effort telemetry process overhead; failures are swallowed and opt-out paths remain zero-send. Ready from this review lane, subject to the repository's remaining current-head approval gate.

@john-the-dev
john-the-dev dismissed bassilkhilo-ag2’s stale review July 29, 2026 13:06

Dismissed as stale after both documented blockers were fixed. Exact head d14641c has green CI/CLA plus Qingyun and Susan formal approvals; current-head evidence covers the phone spawn error handler and context-drop telemetry path.

@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@john-the-dev
john-the-dev merged commit c71a637 into sonichi:main Jul 29, 2026
16 checks passed
qingyun-wu added a commit that referenced this pull request Jul 30, 2026
`main` currently fails `tsc + tests (clean install)` for any branch that pulls it
in. The failure is a post-success interpreter crash, not a test failure:

    github-webhook-access-tier: 31/31 passed
    double free or corruption (out)
    Segmentation fault (core dumped)
    ##[error]Process completed with exit code 1.

Mechanism (diagnosed by @liususan091219 on #2324): since #2274 the task-accepting
paths emit `task_processed`, and `src/github-webhook.py:_emit_github_telemetry()`
lazily imports it. With flush=False the production emitter starts a DAEMON urllib
thread, which can still be inside OpenSSL while this short-lived interpreter
finalizes. A CI runner has no telemetry opt-out marker, so `enabled()` is True.

#2388 fixed exactly one suite this way (tests/agent-api-task-field-injection.test.py).
This is a second suite with the identical mechanism; same fix, same reasoning, so
the two stay consistent.

## What is and is not verified

NOT reproducible on macOS — the glibc double-free does not occur here, so I cannot
show the crash stopping. Saying so plainly rather than implying otherwise.

What IS verified locally is the precondition the crash requires, and that this
change removes it. Running the real suite under a probe that inspects threads at
interpreter exit:

    unpatched:  telemetry_env=None  lingering_threads=['Thread-1']
    patched:    telemetry_env=0     lingering_threads=[]

And the lever itself:

    SUTANDO_TELEMETRY unset -> enabled()=True,  task_processed spawns 1 thread
    SUTANDO_TELEMETRY=0     -> enabled()=False, task_processed spawns 0 threads

So: a live daemon thread survives to finalization without this, and does not with
it. That is the documented cause, eliminated. Suite still 31/31.

Placed after `from __future__ import annotations` (which must lead the file) and
before every regular import, so nothing can import `telemetry` first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DyfS386nrKn8FFdtRLme6x
qingyun-wu added a commit that referenced this pull request Jul 30, 2026
…do (#2372)

* fix(hooks): install hooks pointing at THIS clone, not ~/Desktop/sutando

install-claude-hooks.sh derives REPO_DIR on line 56 and then did not use it: the
three script hooks hardcoded $HOME/Desktop/sutando. The $ was escaped, so that
path landed in settings.json literally and was expanded at HOOK-RUN time,
resolving to the same directory on every host regardless of where the repo is.

On this machine the clone is at
"Library/Application Support/space.ag2.app/engine/sutando" and a sibling sits at
"Documents/github/sutando"; neither is ~/Desktop. Running the installer here
would wire four hooks whose scripts do not exist, and hooks fail silently — the
absence looks identical to "the feature is off".

Before (what main installs on this host):
  MISSING ~/Desktop/sutando/src/session-handoff.sh    <- fires, fails, silent
  MISSING ~/Desktop/sutando/src/check-pending-tasks.sh

After (expanded at install time from the derived REPO_DIR):
  bash <clone>/src/session-handoff.sh "$TRANSCRIPT_PATH"
  bash <clone>/src/check-pending-tasks.sh
  EXISTS both.

$HOME is left alone in the transcript-archive hook: ~/Desktop/sutando-conversations
is a genuine user-chosen destination, not a repo path, so deriving it would be
wrong.

Found while root-causing a dead relay pipeline: workspace/relay/ holds 124 notes
with relay/processed/ empty and no session-state.md, because session-handoff.sh
has never run here — the PreCompact/SessionEnd hooks that invoke it were never
installed. This does not by itself prove the hardcoded path caused that (no
repo-local .claude/settings.json exists in either clone, so the installer appears
never to have been run at all), but it does mean following the documented
install path could not have fixed it.

Not running the installer here — wiring hooks changes the owner's live
environment and is their call. This fixes the script so the wiring works when
they choose to.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZ5eEAmPuFnSA2TkqtQhzZ

* fix(hooks): quote the installed repo path and migrate stale hook variants

Both blockers, plus a third the reviews implied but did not name.

1. QUOTING. REPO_DIR is expanded at INSTALL time, so its literal text is
re-parsed by a shell at HOOK-RUN time. Unquoted, the clone this fix targets —
"Library/Application Support/space.ag2.app/engine/sutando" — splits on the
space and every hook dies with `bash: /Users/you/Library: No such file or
directory`, silently, because a hook's exit status is not surfaced. Paths now go
through shq(), which single-quotes with '\'' escaping: metacharacter-proof,
unlike double quotes which would still interpolate $ and backtick.

2. MIGRATION. Phase 1 compares exact command strings, so it could only ever ADD.
Hook specs now carry a stable <marker>, and a new phase 0 removes any command
for that event carrying our marker but not byte-identical to the one being
installed. That covers the legacy $HOME/Desktop/sutando/... entries called out
in review.

3. THE THIRD CASE. That same add-only comparison means anyone who ran the
installer at 355c4cf holds an UNQUOTED entry which no exact-match can find — so
the broken and the fixed hook would both fire. Phase 0 sweeps it, and the same
mechanism cleans an entry left by a different clone.

Scoping: the sweep is marker-scoped, so "src/session-handoff.sh" cannot match
the transcript-archive hook's "sutando-conversations/" — the separate archive
destination is preserved, as asked — and an operator's hand-added hook is
untouched. Phase 0 runs BEFORE phase 1 so the fresh command is never swept.

tests/install-claude-hooks.test.sh — 16 checks, every fixture path containing a
space. The load-bearing assertions EXECUTE the stored command string and assert
which script it reached, rather than asserting the JSON contains a path:

  ok  SessionEnd stored command executes the intended script
  ok  legacy Desktop session-handoff hook removed (PreCompact/SessionEnd)
  ok  legacy Desktop check-pending-tasks hook removed (Stop)
  ok  transcript-archive hook preserved (different marker)
  ok  operator-added unrelated hook preserved
  ok  exactly one SessionEnd handoff hook (no old+new double-fire)
  ok  second run adds nothing (added=0) / byte-identical settings.json
  ok  unquoted stale variant swept on re-run

Discriminates on both baselines: 8 of 16 fail on origin/main and 11 of 16 on
355c4cf, including all three "stored command executes" and all three
legacy-migration checks. (On a broken baseline the fixture-sanity check also
fails, as a downstream artifact rather than an independent signal.)

bash -n clean, hardcoded-path scanner clean, git diff --check clean.
shellcheck not run locally; CI gates at -S error and tests/ is in its scope.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLGykMovYC6LM99914TQNC

* fix(hooks): sweep only installer-generated shapes, not any same-marker command

The phase-0 sweep treated "contains our marker" as ownership and deleted every
same-marker command that was not byte-identical to the one being installed. An
operator hook invoking the same script with an extra flag matches that test, so
a re-run silently removed it — and a removed hook leaves no trace. The comment
above the loop claimed the opposite ("a hook the operator added by hand is
untouched"), which is how it survived review.

Ownership is now the SHAPE this installer writes: the current command with the
repo path replaced by a wildcard, anchored at both ends, matched with quotes
normalized away so the quoted and unquoted forms both qualify. Customized
commands carry extra text, fail the trailing anchor, and survive. A different
clone's entry still matches — that shape is installer-generated, just not by
this checkout — so this narrows ownership without disabling the migration.

Quote stripping is used for MATCHING only; nothing is rewritten from it.

Adds 6 checks. 4 of them fail on the parent commit: the three operator-hook
survival cases and the combined "sweeping a stale entry did not take the
operator hooks with it". The other two are regression guards that pass either
way, including "another clone's installer-shaped entry is still swept", which
is what proves the fix is not simply a weaker sweep.

* fix(hooks): derive the sweep shape structurally, not by stripping quotes

An apostrophe is legal in a checkout path, and `shq` escapes it as '\'' —
so deleting quote characters to "normalize" a command leaves a stray
backslash behind. On a clone at `/repo'quote` the normalized command and
the normalized repo path then disagreed, the split found nothing, and the
shape collapsed to an exact match of the whole command. Too narrow to
recognise the installer's own earlier output, so the stale unquoted entry
survived and both hooks kept firing: before_rerun=2, after_rerun=2.

Nothing crashed, which is what made it worth a regression test.

The shape is now built from structure rather than string surgery: command
word + marker (already the stable structural token) + the literal tail
after the marker, with the path region wildcarded and the closing quote
optional. Quoted and unquoted forms both match; a customized command has
extra text and fails the trailing anchor, so it still survives. The jq
predicates test the raw command — no lossy pre-processing anywhere.

Adds 3 checks on a fixture path containing BOTH a space and an
apostrophe. One is the discriminator (`stale variant IS swept on re-run`)
and fails on the parent; the other two are a fixture-sanity check and a
does-it-execute guard that pass either way.

* fix(hooks): the sweep's path region must start like a path, not a flag

The trailing anchor protected customization AFTER the script path but said
nothing about customization BEFORE it. A bare wildcard between the command
word and the marker swallowed a leading flag, so an operator hook

    bash -x <repo>/src/session-handoff.sh "$TRANSCRIPT_PATH"

was classified installer-owned and deleted on re-run. Reproduced on the
parent: before 2, after 1 — the -x hook gone, silently.

Same durability class as the two earlier false positives, approached from
the other side of the path.

The path region now requires an optional opening quote followed by a
character that is neither a space nor `-`. That admits every shape this
installer writes or has written — /abs/..., '/quoted path/...',
'/repo'\''quote/...' — plus the legacy $HOME/Desktop/... entries the
migration exists to remove. A flag or wrapper token fails immediately.

Requiring a literal `/` would have been the obvious rule and is wrong: it
spares exactly the legacy $HOME entries this sweep must sweep. There is a
committed check for that so the mistake cannot be reintroduced quietly.

Adds 4 checks. 2 fail on the parent: the bash -x survival and the
resulting count. The other 2 — fixture sanity and "legacy is still swept"
— pass either way; the second is the guard against over-narrowing.

* fix(hooks): never sweep a hook that embeds no repo path

Phase 0 migrates entries whose PATH went stale — a legacy $HOME/Desktop
clone, an unquoted form, another checkout. The transcript-archive hook
embeds no repo path at all ($HOME only), so nothing about it can go stale
and sweeping it can only destroy someone else's command.

With a wildcard shape the `.*` spanned the SOURCE ARGUMENT rather than a
path, so an operator archiving from a different variable

    cp "$CUSTOM_TRANSCRIPT_PATH" "$HOME/Desktop/sutando-conversations/…"

matched "our shape" and was deleted and replaced on re-run, silently
discarding their archival policy. Reproduced on the parent.

This is a regression I introduced: an earlier revision had an explicit
"no repo path -> exact match" branch, and rewriting the shape structurally
dropped it. Restored as an explicit skip, which states the intent instead
of encoding it as a pattern that happens to be unreachable.

The guard compares against the ESCAPED path body, not raw $REPO_DIR: shq
rewrites an internal `'` as `'\''`, so on /repo'quote a raw comparison
never matches and the guard would skip the sweep for exactly the hooks it
must run on — the same raw-vs-escaped confusion behind the apostrophe bug
this file already fixed once. Caught by the existing apostrophe test.

Adds 3 checks. 1 fails on the parent (the operator's archive command is
destroyed); the other 2 assert both of our hooks still install, and pass
either way — they are the guard against "fix it by skipping everything".

* docs(hooks): the ownership-test summary described a removed implementation

Four fixes landed on this block tonight and the header comment was never
re-read as a whole. It still claimed the sweep compares "with quotes
normalized away so the quoted and unquoted forms both match" — which is
the 479ffe0 approach that ddaa425 REMOVED for being lossy. So the summary
described an implementation that no longer exists, and specifically the
one the code was changed away from.

A comment describing work that no longer exists reads as a live
description; there is no tense marker to tell a reader otherwise. Worse
here, the next person to "restore consistency" would have reintroduced
the apostrophe bug.

Header now states the actual three conditions, each annotated with the
false positive that produced it — embeds-the-repo-path, path region must
start like a path, exact tail — and says plainly that matching is on the
RAW command with no pre-processing.

The two remaining quote-stripping mentions further down are historical by
construction ("never by stripping quotes", "was lossy and wrong") and are
correct as prohibitions.

Comment-only; 33/33 unchanged.

* test(github-webhook): disable telemetry in the access-tier suite

`main` currently fails `tsc + tests (clean install)` for any branch that pulls it
in. The failure is a post-success interpreter crash, not a test failure:

    github-webhook-access-tier: 31/31 passed
    double free or corruption (out)
    Segmentation fault (core dumped)
    ##[error]Process completed with exit code 1.

Mechanism (diagnosed by @liususan091219 on #2324): since #2274 the task-accepting
paths emit `task_processed`, and `src/github-webhook.py:_emit_github_telemetry()`
lazily imports it. With flush=False the production emitter starts a DAEMON urllib
thread, which can still be inside OpenSSL while this short-lived interpreter
finalizes. A CI runner has no telemetry opt-out marker, so `enabled()` is True.

#2388 fixed exactly one suite this way (tests/agent-api-task-field-injection.test.py).
This is a second suite with the identical mechanism; same fix, same reasoning, so
the two stay consistent.

## What is and is not verified

NOT reproducible on macOS — the glibc double-free does not occur here, so I cannot
show the crash stopping. Saying so plainly rather than implying otherwise.

What IS verified locally is the precondition the crash requires, and that this
change removes it. Running the real suite under a probe that inspects threads at
interpreter exit:

    unpatched:  telemetry_env=None  lingering_threads=['Thread-1']
    patched:    telemetry_env=0     lingering_threads=[]

And the lever itself:

    SUTANDO_TELEMETRY unset -> enabled()=True,  task_processed spawns 1 thread
    SUTANDO_TELEMETRY=0     -> enabled()=False, task_processed spawns 0 threads

So: a live daemon thread survives to finalization without this, and does not with
it. That is the documented cause, eliminated. Suite still 31/31.

Placed after `from __future__ import annotations` (which must lead the file) and
before every regular import, so nothing can import `telemetry` first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DyfS386nrKn8FFdtRLme6x

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: john-the-dev <johnv.the.dev@gmail.com>
john-the-dev added a commit that referenced this pull request Aug 6, 2026
…the #2274 gap

#2274 instrumented every messaging bridge with task_processed{source} at
task-write time but missed the gateway surface: ag2_sparrow's _write_task
emitted nothing, so task_processed{source="ag2space"} never existed and
gateway activity was invisible in PostHog (owner report 2026-07-30).

Fire the same fire-and-forget event after the atomic publish of a NEWLY
queued task — the dedup/idempotent early returns never reach it, so relay
redeliveries and re-writes aren't double-counted. The telemetry module lives
in the host repo's src/ (on sys.path via the src/remote-gateway-bridge.py
launcher); a standalone PyPI sparrow install has no such module and the
try/except silently no-ops, keeping the package dependency-free.

Tests: tests/gateway-task-telemetry.test.py — source tagging, PROVIDER
fallback, no double-count on idempotent/archived redelivery, missing-module
and raising-telemetry never break the write.
john-the-dev added a commit that referenced this pull request Aug 7, 2026
…the #2274 gap (#2432)

* feat(telemetry): emit task_processed for gateway (ag2.space) tasks — the #2274 gap

#2274 instrumented every messaging bridge with task_processed{source} at
task-write time but missed the gateway surface: ag2_sparrow's _write_task
emitted nothing, so task_processed{source="ag2space"} never existed and
gateway activity was invisible in PostHog (owner report 2026-07-30).

Fire the same fire-and-forget event after the atomic publish of a NEWLY
queued task — the dedup/idempotent early returns never reach it, so relay
redeliveries and re-writes aren't double-counted. The telemetry module lives
in the host repo's src/ (on sys.path via the src/remote-gateway-bridge.py
launcher); a standalone PyPI sparrow install has no such module and the
try/except silently no-ops, keeping the package dependency-free.

Tests: tests/gateway-task-telemetry.test.py — source tagging, PROVIDER
fallback, no double-count on idempotent/archived redelivery, missing-module
and raising-telemetry never break the write.

* fix(telemetry): admit gateway sources to _KNOWN_SOURCES + integration-test the real pipeline (CR)

qingyun-wu CR P1-1: task_processed("ag2space") was coarsened to
source="unknown" by _KNOWN_SOURCES — the metric the PR claims to add never
actually reached PostHog, and the fake-telemetry test couldn't see it. Add
"ag2space" + "events-promotion" to the bounded allowlist (junk still
collapses to unknown — cardinality/leak posture unchanged) and add
integration cases that run _write_task through the REAL src/telemetry.py
(sink stubbed at _post): ag2space kept, events-promotion kept, junk
collapsed.

qingyun-wu CR P1-2: _fresh_dirs() now rebinds TASK_ROOMS_FILE and
OWNER_ACTIVITY_FILE too, and an isolation audit asserts the task file,
task-rooms sidecar, and owner-activity stamp all land under the case's temp
root — no writes to the operator's real ~/.ag2-sparrow tree.

* fix(telemetry): instrument the REAL taskify path + make the suite hermetic vs host tierMap (CR round 2)

qingyun-wu CR P1-1: TaskifyHandler._promote() writes its task file directly
(never through _write_task), so events-promotion telemetry never fired in
production — the prior test hand-fed _write_task and proved only the
allowlist bucket. _promote() now emits task_processed("events-promotion")
after its atomic publish, behind the already-promoted early return (re-drains
aren't double-counted), same fail-open shape as the bridges. New test drives
the REAL handler (threshold=1, offer + duplicate re-offer) through the real
telemetry module: one event, tagged events-promotion.

qingyun-wu CR P1-2: the suite resolved the operator's ambient AG2 Space
tierMap. CLAUDE_CONFIG_DIR now points at a temp root with a seeded EMPTY
channels/ag2space/access.json BEFORE the bridge import; _fresh_dirs() resets
the seeded file + _TIER_MAP_CACHE per case; LOCAL_TIER pinned. Added the
hostile-tier control: a controlled tierMap down-tiering the fixture sender to
team → telemetry still emits, task carries access_tier: team, owner-activity
stamp suppressed — all under the temp root.

* fix(telemetry): expose host module to events client

* ci: re-trigger workflows on rebased head (squash-erased)

* ci: re-trigger required workflows (squash-erased)

* remove a newline

* chore: re-fire CI after GitHub Actions outage recovery
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.

4 participants