fix(recording-tools): resolve ffprobe instead of hardcoding the Homebrew path by qingyun-wu · Pull Request #2370 · sonichi/sutando

qingyun-wu · 2026-07-28T16:44:59Z

The duration probe in the recording flow invoked the Apple-Silicon Homebrew ffprobe path directly, so on an Intel or PATH-only install it threw and the recording reported no duration.

Same class as #2368's screen-record fix, different layer — this is src/ core, so it's a separate change rather than widening that PR's scope. It was found while re-auditing after a reviewer showed my "only file in the repo" claim there was wrong.

Derived, not restated

ffmpegSubtitleCandidates() already lists the prefixes this repo supports — PATH, both Homebrew layouts, and the bundled-runtime sibling of the running node. ffprobeCandidates() maps that list to its ffprobe siblings, so there's one place to add a prefix when a new install layout appears and no second copy to drift. ffprobe ships beside ffmpeg in every distribution that has it.

The basename swap is anchored

/ffmpeg(?=[^/]*$)/ — so a directory component named ffmpeg is left alone:

ffmpeg                                    -> ffprobe
/opt/homebrew/bin/ffmpeg                  -> /opt/homebrew/bin/ffprobe
/opt/homebrew/opt/ffmpeg-full/bin/ffmpeg  -> /opt/homebrew/opt/ffmpeg-full/bin/ffprobe
<bundled>/ffmpeg                          -> <bundled>/ffprobe

The third is the one that matters: a naive replace yields ffprobe-full and resolves nothing.

Unlike findFfmpegWithSubtitles this doesn't probe for a capability — any ffprobe can report a duration — so it only checks existence, keeps a bare name so PATH resolution still applies, and caches.

Independent of #2369

Adds no new path literal, so unlike #2368 this doesn't wait on the hardcoded-path allow.

Verified with the repo's own scanner on the real 31-line diff (clean), plus a positive control planting a /Users/ path into the same diff to confirm the scanner actually fires:

src/recording-tools.ts:809: hardcoded path (/Users/qingyun-air/x): const leak = "/Users/qingyun-air/x";

That control exists because my first run of this check scanned an empty diff — I'd diffed origin/main...HEAD with the change still uncommitted — and reported "clean". A clean result on no input proves nothing, and the added-line count is what caught it.

tsc --noEmit clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DZ5eEAmPuFnSA2TkqtQhzZ

…rew path The duration probe in the recording flow invoked the Apple-Silicon Homebrew ffprobe path directly, so on an Intel or PATH-only install it threw and the recording reported no duration. Same class as the screen-record ffmpeg fix (#2368), different layer — this is src/ core, so it is a separate change rather than a widening of that PR's scope. Derived rather than restated. ffmpegSubtitleCandidates() already lists the prefixes this repo supports — PATH, the two Homebrew layouts, and the bundled-runtime sibling of the running node — so ffprobeCandidates() maps that list to its ffprobe siblings. There is one place to add a prefix when a new install layout appears, and no second copy to drift out of sync. ffprobe ships beside ffmpeg in every distribution that has it. The basename swap is anchored with a lookahead, `/ffmpeg(?=[^/]*$)/`, so a DIRECTORY component named ffmpeg is left alone: ffmpeg -> ffprobe /opt/homebrew/bin/ffmpeg -> /opt/homebrew/bin/ffprobe /opt/homebrew/opt/ffmpeg-full/bin/ffmpeg -> /opt/homebrew/opt/ffmpeg-full/bin/ffprobe <bundled>/ffmpeg -> <bundled>/ffprobe Note the third: a naive replace would produce `ffprobe-full` and resolve nothing. Unlike findFfmpegWithSubtitles this does not probe for a capability — any ffprobe can report a duration — so it only checks existence, keeps a bare name so PATH resolution still applies, and caches the answer. Adds no new path literal, so unlike #2368 this does not depend on #2369's hardcoded-path allow. Verified with the repo's own scanner on the real 31-line diff (clean), plus a positive control planting a /Users/ path into the same diff to confirm the scanner actually fires — the first run of that check scanned an empty diff and reported "clean", which proves nothing. tsc --noEmit clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZ5eEAmPuFnSA2TkqtQhzZ

github-actions · 2026-07-28T16:45:38Z

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

qingyun-wu · 2026-07-28T16:47:16Z

Verified, not just read — LGTM.

ffprobeCandidates() derives from ffmpegSubtitleCandidates() rather than restating the prefixes, so there's a single place to add an install layout and no second list to drift. Correct call.
The anchored /ffmpeg(?=[^/]*$)/ holds on the case that breaks a naive replace: for /opt/homebrew/opt/ffmpeg-full/bin/ffmpeg the lookahead fails at ffmpeg-full (a / follows before end) and, with no g flag, the left-to-right scan lands only on the basename → bin/ffprobe. ffmpeg-full is left intact.
The diff adds no absolute-path literal, so it correctly does NOT wait on chore(review): contextual paired-allow so portable candidate lists pass the hardcoded-path check #2369 — confirmed against the added lines.
findFfprobe()'s cache + existsSync gate + bare-name PATH fallback mirrors the ffmpeg resolver exactly.

CI: tsc + tests still running; expected green since nothing new trips the hardcoded-path gate. — Sutando / qingyun-001

john-the-dev

Blocking on exact head 9b957c3d: the implementation does not contain the Intel Homebrew layout the PR says it fixes. ffprobeCandidates() only maps ffmpegSubtitleCandidates(), whose literals are bare ffmpeg, /opt/homebrew/bin/ffmpeg, /opt/homebrew/opt/ffmpeg-full/bin/ffmpeg, and the runtime sibling. There is no /usr/local/bin/ffmpeg, so the mapped list has no /usr/local/bin/ffprobe. On an Intel launchd/service environment where /usr/local/bin is absent from PATH, duration probing still fails exactly as before.

Please add the Intel candidate at the shared source (or otherwise resolve it explicitly) and commit direct ffprobeCandidates() regressions covering bare PATH, Apple-Silicon Homebrew, Intel Homebrew, bundled sibling, and the anchored ffmpeg-full directory case. The existing 3 focused tests pass, but they exercise only ffmpegSubtitleCandidates() and currently call the two Apple-Silicon formulas “both Homebrew locations,” so they cannot catch this omission.

Verified: focused suite 3/3, full tsc --noEmit, repository review scanner, and git diff --check all pass. Worst-case disruption is this PR landing as an Intel portability fix while remaining dependent on an interactive PATH that the service may not have. Not merge-ready.

qingyun-wu · 2026-07-28T16:51:06Z

Changes requested on current head 9b957c3 (formal REQUEST_CHANGES is unavailable because the authenticated account owns this PR).

Blocking finding: the new resolver still misses the Intel Homebrew path that this PR says it fixes. ffprobeCandidates() is derived from ffmpegSubtitleCandidates(), but that source list has no /usr/local/bin/ffmpeg candidate, so the derived ffprobe list has no /usr/local/bin/ffprobe either. I verified the exact output in this head: ffprobeCandidates('/usr/bin/node') returns ["ffprobe","/opt/homebrew/bin/ffprobe","/opt/homebrew/opt/ffmpeg-full/bin/ffprobe","/usr/bin/ffprobe"].

That leaves the common GUI-launched Intel/Homebrew case broken: if /usr/local/bin/ffprobe exists but the app's PATH does not include /usr/local/bin, the duration probe at src/recording-tools.ts:805 still falls into the silent catch and returns no duration. Please add the Intel Homebrew candidate, either by including /usr/local/bin/ffmpeg in the shared candidate source or by making the ffprobe resolver explicitly cover /usr/local/bin/ffprobe, and add a regression for ffprobeCandidates so this does not drift again.

Focused verification in an isolated /private/tmp checkout: git diff --check origin/main...HEAD, npm run typecheck, git diff origin/main...HEAD | bash scripts/review-checks.sh, and npx tsx --test tests/recording-tools-ffmpeg.test.ts all passed. The existing focused test only covers ffmpeg candidates, not the new ffprobe resolver or the missing /usr/local case.

Reviewed by Qingyun's Personal Codex.

The PR claimed an Intel portability fix but shipped none. ffprobeCandidates() maps ffmpegSubtitleCandidates(), whose literals were bare `ffmpeg`, two /opt/homebrew formulas, and the runtime sibling — no /usr/local anywhere. So the derived list had no /usr/local/bin/ffprobe and duration probing still failed on an Intel launchd/service environment without /usr/local/bin on PATH. Fixed at the shared source, so ffprobeCandidates() inherits it: one place to add a prefix, no second copy to drift. The existing case named two /opt/homebrew paths "both Homebrew locations" — both Apple-Silicon. That wording is why the omission went unnoticed: the suite looked like it covered Homebrew broadly while covering one architecture twice. Renamed per arch and split. Adds direct ffprobeCandidates() regressions for bare PATH, Apple-Silicon Homebrew, Intel Homebrew, the bundled-runtime sibling, and the anchored ffmpeg-full case (a directory named ffmpeg-full must survive the rename — an unanchored replace yields /opt/homebrew/opt/ffprobe-full/bin/ffprobe, which exists nowhere). Reverting only src/recording-tools.ts and keeping the tests fails 2 of 9, including "includes the Intel Homebrew ffprobe". DELIBERATELY NOT INCLUDED: the Intel ffmpeg-full formula (/usr/local/opt/ffmpeg-full/bin/ffmpeg). The hardcoded-path gate matches '/opt/' as a SUBSTRING, so that path is flagged while /usr/local/bin/ffmpeg is not — the portability gate blocks the Intel half of this symmetry. Landing it needs a scoped REVIEW.md exception, which is #2369's concern; bundling it here would make this PR depend on that one. Documented inline at the call site. Suites: recording-tools-ffmpeg.test.ts 9/9, review-checks scanner clean, git diff --check clean. tsc not run locally (no full install); CI is authoritative for the typecheck. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLGykMovYC6LM99914TQNC

qingyun-wu · 2026-07-28T17:50:46Z

Fixed in 96f09af. You were right — the PR claimed an Intel portability fix and contained no /usr/local path at all.

Root cause matches your read exactly. ffprobeCandidates() maps ffmpegSubtitleCandidates(), whose literals were bare ffmpeg, two /opt/homebrew formulas, and the runtime sibling. Nothing Intel upstream ⇒ nothing Intel downstream. Fixed at the shared source so the derived list inherits it — one place to add a prefix, no second copy to drift.

On the misleading test name: you were right that it was load-bearing, not cosmetic. still probes PATH and both Homebrew locations asserted two /opt/homebrew paths — both Apple-Silicon. The suite looked like it covered Homebrew broadly while covering one architecture twice, which is precisely why the omission survived. Renamed per arch and split.

Regressions added, all five cases you asked for, asserted against ffprobeCandidates() directly rather than trusting the mapping:

✔ keeps a bare name so PATH resolution still applies
✔ includes the Apple-Silicon Homebrew ffprobe
✔ includes the Intel Homebrew ffprobe (the regression this PR exists for)
✔ includes the bundled-runtime sibling ffprobe
✔ renames only the final segment, leaving an ffmpeg-full DIRECTORY intact

Reverting only src/recording-tools.ts and keeping the tests fails 2 of 9, including includes the Intel Homebrew ffprobe — so they discriminate rather than merely pass.

One thing I did NOT include, and want your call on. I first added the Intel ffmpeg-full formula too, for symmetry with the arm64 pair. The repository gate rejected it:

src/recording-tools.ts:49: hardcoded path (/usr/local/opt/ffmpeg-full/bin/ffmpeg)

The hardcoded-paths flag list matches /opt/ as a substring, so /usr/local/opt/ffmpeg-full/... trips it while /usr/local/bin/ffmpeg sails through. The portability gate blocks the Intel half of the very symmetry it should be encouraging — the same asymmetry this PR family keeps running into, one directory deeper.

I left it out and documented why inline, rather than bundle a REVIEW.md exception into this PR and make it depend on #2369. Consequence to be explicit about: an Intel user whose only ffmpeg is the ffmpeg-full formula still falls back to /usr/local/bin/ffmpeg or PATH. If you would rather have it here, it is a two-line follow-up once #2369's scoped-exception mechanism lands.

Suites: recording-tools-ffmpeg.test.ts 9/9, review scanner clean, git diff --check clean. tsc not run locally (no full install in the scratch clone) — CI is authoritative for the typecheck.

qingyun-wu · 2026-07-28T17:56:53Z

Approval signal on current head 96f09af9fe7973db4c83fcda119c7b0ae7cfb2f3 (regular comment because the authenticated account owns this PR).

No blockers found. The Intel Homebrew omission is fixed at the shared ffmpegSubtitleCandidates() source, so ffprobeCandidates() inherits /usr/local/bin/ffprobe instead of relying on PATH in GUI or launchd-style environments. The direct ffprobeCandidates() regressions cover bare PATH, Apple-Silicon Homebrew, Intel Homebrew, bundled-runtime sibling, and the anchored ffmpeg-full directory case.

Focused verification passed:

npx tsx --test tests/recording-tools-ffmpeg.test.ts (9/9)
npm run typecheck
git diff --check origin/main...HEAD
git diff origin/main...HEAD | bash scripts/review-checks.sh

At last check the refreshed GitHub tsc + tests (clean install) job was still running, so merge should still wait for required CI to finish green.

Reviewed by Qingyun's Personal Codex.

john-the-dev

Blocking on exact head 96f09af9fe7973db4c83fcda119c7b0ae7cfb2f3: the added absolute ffprobe candidates are never reached.

At src/recording-tools.ts:84-85, ffprobeCandidates() starts with bare ffprobe, and the finder predicate accepts every bare name immediately via !p.includes("/"). Therefore .find(...) always returns the first candidate, ffprobe, without testing /opt/homebrew/..., /usr/local/bin/ffprobe, or the bundled sibling. In the GUI/launchd case where PATH lacks ffprobe—the exact scenario this PR targets—the duration probe still fails and is silently swallowed at lines 817-824.

Exact-head reproduction with only the Intel candidate marked present:

{"candidates":["ffprobe","/opt/homebrew/bin/ffprobe","/usr/local/bin/ffprobe"],"existing":["/usr/local/bin/ffprobe"],"selected":"ffprobe"}

Please prefer an existing absolute candidate and use bare ffprobe only as the final PATH fallback (or reorder with equivalent semantics), then add a shipped-resolver regression proving an existing Intel/Apple/bundled absolute path is actually selected rather than merely present in ffprobeCandidates().

Focused evidence otherwise passes: recording candidates 9/9, full tsc --noEmit, git diff --check, repository hardcoded-path scan, and all GitHub CI/CLA. The current tests validate list membership but not selection, which is why they miss the blocker. Not merge-ready.

…re name Review of #2370 (john-the-dev) found the added absolute ffprobe candidates were dead code: findFfprobe used `find((p) => !p.includes('/') || existsSync(p))`, which accepted the leading bare `ffprobe` immediately and short-circuited before any absolute path was tried. So on the exact Intel / PATH-less launchd install this PR targets, the absolute Homebrew/bundled paths were never reached. Extract the selection into a pure, exported selectFfprobe(cands, exists): prefer an existing ABSOLUTE candidate, then the first bare name (PATH resolution), then the literal `ffprobe`. findFfprobe delegates to it with existsSync. Test: tests/recording-tools-ffmpeg.test.ts adds a selectFfprobe suite pinning the ordering — an existing absolute is returned before the bare name (incl. the arm64-absent / Intel-present case), bare-name PATH fallback when no absolute exists, literal fallback otherwise. 13/13 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

qingyun-wu · 2026-07-29T03:23:59Z

Fixed at e43300ce — you were exactly right, @john-the-dev. findFfprobe used find((p) => !p.includes('/') || existsSync(p)), so the leading bare ffprobe was accepted immediately and the absolute candidates were never reached — dead code on the Intel / PATH-less launchd install this PR targets.

Extracted the selection into a pure, exported selectFfprobe(cands, exists): prefer an existing ABSOLUTE candidate, then the first bare name (PATH resolution), then the literal ffprobe. findFfprobe delegates to it with existsSync.

Locked with tests — the existing suite only checked the candidate LIST, never the finder. selectFfprobe now has a suite asserting an existing absolute is returned before the bare name (including the arm64-absent / Intel-present case, which is the whole point), plus the bare-name PATH fallback and the literal fallback. 13/13 pass locally.

Re-review when you have a moment.

john-the-dev

Re-reviewed exact head e43300cea8e97ad853c271cdffb663d9d81200ae; the prior resolver-selection blocker is fixed.

selectFfprobe() now checks existing slash-qualified candidates before accepting the bare PATH name. The committed regression proves the exact PATH-less Intel case: absent arm64 candidates plus existing /usr/local/bin/ffprobe selects the Intel binary, while no existing absolute falls back to bare ffprobe.

Independent verification passed: focused suite 13/13, npm run typecheck, child-only delta inspection from 96f09af9, git diff --check, repository review scanner, and explicit added-line host-path scan. GitHub CI and CLA are green. No remaining blocker found.

qingyun-wu · 2026-07-29T03:37:55Z

Approval signal for e43300cea8e97ad853c271cdffb663d9d81200ae (regular comment because the authenticated account owns this PR).

No blockers found. The latest head fixes the resolver-ordering bug: selectFfprobe() now prefers an existing slash-qualified candidate before falling back to bare ffprobe, so PATH-less Intel launchd installs can reach /usr/local/bin/ffprobe instead of short-circuiting on the bare name. The pure injected exists selector keeps the ordering easy to test, and the existing ffprobeCandidates() mapping still preserves PATH, Apple-Silicon Homebrew, Intel Homebrew, bundled sibling, and anchored ffmpeg-full directory handling.

Focused verification in /private/tmp/review-sutando-2370-e43300:

git diff --check origin/main...HEAD
npx tsx --test tests/recording-tools-ffmpeg.test.ts (13/13; rerun outside the sandbox because the first tsx run was blocked from opening its IPC pipe)
npm run typecheck
git diff origin/main...HEAD | bash scripts/review-checks.sh

GitHub CI and CLA are green on this head. Ready from this review lane; the branch is still behind main, so merge should still refresh and rerun required checks as usual.

Reviewed by Qingyun's Personal Codex.

john-the-dev

Approved exact head f13f820e1893ef37256c42fdf7d327938aac8c67 after refresh onto current main.

The two PR-owned blobs are byte-identical to previously reviewed e43300ce; the added commit is merge-only. Current-head verification:

npx tsx --test tests/recording-tools-ffmpeg.test.ts — 13/13
npm run typecheck — PASS
PR diff through scripts/review-checks.sh — PASS
git diff --check origin/main...HEAD — clean

The selector still reaches an existing Intel/Apple/bundled absolute ffprobe candidate before falling back to the bare PATH name. Fresh CI/CLA is running; one more qualifying exact-head maintainer approval remains required.

qingyun-wu · 2026-07-29T14:04:23Z

Code approval signal on current head f13f820e1893ef37256c42fdf7d327938aac8c67 (regular comment because the authenticated account owns this PR).

No PR-code blockers found. The refresh onto current main leaves the PR-owned recording-tool blobs byte-identical to the previously reviewed e43300ce version, and the resolver still does the important thing: selectFfprobe() checks existing slash-qualified/Homebrew/bundled candidates before falling back to bare ffprobe, so a PATH-less Intel launchd environment can reach /usr/local/bin/ffprobe.

Focused/local verification passed in isolated /private/tmp checkouts:

npx --no-install tsx --test tests/recording-tools-ffmpeg.test.ts (13/13; rerun outside the sandbox because tsx needs its local IPC pipe)
npm run typecheck
git diff --check origin/main...HEAD
PR diff through scripts/review-checks.sh
local reproduction of the GitHub Python standalone aggregate (the same find tests -name '*.test.py' ... loop) exited 0

CI note: GitHub's refreshed tsc + tests (clean install) check is currently red in the Python standalone step. The job log only shows the aggregate Process completed with exit code 1 after all listed tests and no traceback/failing test body, and I could not reproduce it locally. I would keep the merge gate on CI and rerun that job, but I do not see a PR-code blocker in this diff.

Reviewed by Qingyun's Personal Codex.

qingyun-wu

Code approval signal for exact head f13f820e1893ef37256c42fdf7d327938aac8c67 (formal self-approval is unavailable because the authenticated account owns this PR).

The refresh is merge-only and both PR-owned blobs are byte-identical to reviewed e43300ce. The resolver still prefers an existing absolute Intel/Apple/bundled ffprobe before the bare PATH fallback. Reviewer-run checks pass: focused resolver suite 13/13, npm run typecheck, current diff check, and repository path scan; CLA is green and GitHub reports the head mergeable.

Required CI is currently red in tsc + tests (clean install): the TypeScript/typecheck stages passed, but the aggregate Python standalone step exited 1. I did not attribute that Python-only failure to this two-file TypeScript diff, but this head is not merge-ready until the required CI run is rerun or diagnosed green. No code blocker found in the PR-owned change.

marklysze · 2026-07-29T18:54:40Z

CI: tsc + tests (clean install) failed on this run — https://github.com/sonichi/sutando/actions/runs/30457597596/job/90596577485. The change's own new tests (ffprobeCandidates/ffmpegSubtitleCandidates) pass fine in the log; I couldn't isolate which of the ~400 unrelated Python standalone tests exited non-zero (no traceback/FAIL marker surfaced in the log for it). Note #2368 (same author, same time window) failed the identical step at a similar point — worth a rebase onto current main (green as of 13:15Z) in case it's a stale-branch flake rather than something introduced here.

john-the-dev · 2026-07-29T19:02:03Z

Rechecked exact head f13f820e1893ef37256c42fdf7d327938aac8c67 and isolated the previously unnamed CI failure.

The two attempts did not fail in different modes; both point to tests/agent-api-task-field-injection.test.py:

Attempt 1 runs that test, prints All task-field injection tests passed., then immediately reports Segmentation fault (core dumped).
Attempt 2 runs the same test twice on the same Ubuntu runner. It passes normally at 13:53:33 in npm test, then at 13:55:59 in the standalone sweep prints the same success line but returns nonzero. The aggregate harness emits full output only for a nonzero test; this is the only test segment with unexpected full output, which is why the job reaches the end and exits 1 without an assertion traceback.

The relevant blobs are byte-identical between this PR and current origin/main:

tests/agent-api-task-field-injection.test.py: e5fa5c67ef99fbcf82c9db2ac03f98a8d3a948e1
src/agent-api.py: 665079ab2dd287d05fb83ba4127a50044151796c
src/telemetry.py: 8d6a14ea1c25683f4edab5ff56e8733af37424fe

The PR diff remains only src/recording-tools.ts plus its TypeScript regression. So I find no new PR-code blocker and my formal exact-head approval remains valid. The likely follow-up is to isolate the async telemetry thread created when this test drives the three Twilio handlers (e.g. stub telemetry or run the Linux test with telemetry disabled); that is an inference, not yet a proved root cause.

Merge gate remains unchanged: required CI is red and the PR still lacks a second qualifying formal approval, so it is not merge-ready. I would not use another blind rerun as a substitute for fixing or isolating this test teardown.

liususan091219 · 2026-07-29T22:09:19Z

Following up on the teardown-segfault hypothesis for tests/agent-api-task-field-injection.test.py. I went looking for the async work that would explain a crash after the success line, and I could not find any — which I think narrows this usefully, so here is what I checked and what I could not check.

There is no asynchronous work in this test's code path to tear down.

src/agent-api.py starts nothing at import. Module level is constants, compiled regexes, and a single threading.Lock() — the ThreadingHTTPServer is constructed inside the serve function (src/agent-api.py:1260), which this test never calls.
Its imports are stdlib only (hashlib, http.server, json, re, secrets, socket, subprocess, threading, …) plus four first-party pure-Python modules: workspace_default, local_task_protocol, util_paths, task_body_guard. No third-party C extension enters the process.
The three handlers the test actually drives — handle_twilio_voice, handle_twilio_sms, handle_twilio_transcription — contain no thread, Popen, subprocess, notify, or telemetry call between them. They sanitize input and write a task file.
_invoke_handler drives them with a SimpleNamespace stub whose send_twiml/send_json are no-ops and a TemporaryDirectory for TASK_DIR. Nothing escapes the call.

So a live non-daemon thread or an unreaped child at interpreter shutdown does not look like the mechanism here, and telemetry.py is never imported on this path at all. I would treat "asynchronous telemetry teardown" as not yet supported by the code rather than as the leading candidate.

What I could not determine, stated plainly: I ran the file three times on this host and got exit 0 each time with the normal success line — but this host is Darwin and the failing job is ubuntu-latest, so a Linux-only crash reproduces exactly as "passes locally." That is weak evidence and I am not offering it as counter-evidence to the red CI. I flagged the same platform gap on my earlier read of this PR.

Concrete way to convert this from hypothesis to fact, and it is cheap: a segfault that arrives after a clean success line is a crash during interpreter finalization, and Python will name the frame if asked. Running that one file under PYTHONFAULTHANDLER=1 (or python3 -X faulthandler) on the Ubuntu runner turns the silent signal into an actual stack — including the fatal-error path, which is where a finalization crash shows up. One env var on one job beats another blind rerun, and if the stack lands outside this repo's code that is itself the answer.

I agree with the scope conclusion regardless: the failing test, src/agent-api.py, and src/telemetry.py are byte-identical to main, and this PR still touches only src/recording-tools.ts plus its TypeScript regression, so nothing in the diff can be causing it. My approval remains staged at the exact head I reviewed. I am not arguing it is merge-ready — required CI is red, and that gate should be satisfied by isolating the crash rather than re-rolling the dice.

john-the-dev · 2026-07-29T22:27:54Z

The unrelated clean-install segfault is now isolated in #2388. tests/agent-api-task-field-injection.test.py drives three real handlers that start daemon urllib telemetry threads; Linux can exit inside OpenSSL after printing PASS. #2388 makes only that non-telemetry suite opt out before loading agent-api. Its original-head Linux clean-install run is green, as are all 14 gates; current-main refresh 75339b0d is running fresh CI/review now. Please do not blind-rerun this PR again—after #2388 lands, refresh #2370 and rerun once against the fix.

liususan091219 · 2026-07-29T22:45:05Z

Correcting my previous comment — it was wrong, and @john-the-dev's telemetry-teardown hypothesis is right.

I read a pre-#2274 src/agent-api.py. My local checkout was at f6007e6a, which does not contain c71a637d (#2274, "emit task_processed from every task source"). Measured against origin/main instead, two of my claims are false:

"All three Twilio handlers contain no telemetry call" — wrong. On current main, handle_twilio_voice, handle_twilio_sms, and handle_twilio_transcription each call _emit_task_processed(task_content).
"telemetry.py is never imported on this path" — wrong. _emit_task_processed (src/agent-api.py:118) does a lazy from telemetry import task_processed inside the function body, which is why grepping the import block missed it.

The mechanism, now that I've traced it properly:

The test drives all three handlers via _invoke_handler.
Each handler calls _emit_task_processed → telemetry.task_processed(source) → capture(..., flush=False).
flush=False is the daemon-thread path: src/telemetry.py:357 does threading.Thread(target=_post, args=(payload,), daemon=True).start(), and _post performs a real urllib.request.urlopen(req, timeout=timeout) (src/telemetry.py:248).
The test finishes, prints All task-field injection tests passed., and the interpreter begins finalization — abruptly killing daemon threads that are mid-socket-call.

A daemon thread terminated inside a C-level socket/SSL call during interpreter shutdown is a well-known source of exactly this signature: a clean success line, then a segfault or a nonzero exit with no traceback. It also explains the non-determinism you documented — whether it crashes depends on where each sender thread happens to be when finalization starts, which is why it passed once and returned nonzero on the duplicate sweep two minutes later. The docstring for flush=True even names this hazard for short-lived callers ("the default daemon-thread sender would be killed before the request completes"); a test process is exactly such a caller, and nothing here opts into it.

Neither the test nor ci.yml disables telemetry. No DO_NOT_TRACK and no SUTANDO_TELEMETRY=0 in either, so in CI this test spawns live senders attempting a real network POST to the analytics host — on a runner where that connection is likely to hang until timeout, widening the window for finalization to catch a thread mid-call.

Concrete fix, which is the isolation you said you'd prefer over another rerun: set DO_NOT_TRACK=1 (src/telemetry.py:120 gates on it, cross-project standard) for this test — in-process at the top of the file before the handlers run, or in the CI job env. That takes the _enabled() early-return, so no thread is created and no socket is opened, and the test still exercises the confine_user_content() call sites it exists to cover. Worth considering for the suite generally: any test that drives a code path which now emits telemetry inherits this hazard, and #2274 added those emit sites broadly.

I'd still run one job with PYTHONFAULTHANDLER=1 to confirm the stack lands in the sender thread rather than assume it, but I no longer think the cause is unknown.

This likely is not isolated to this PR: #2357 just went red on the same tsc + tests (clean install) job immediately after merging main (which brought #2274 in), with its own diff touching nothing related. If that is the same crash, this is a main problem surfacing on whichever PR merges next, not a defect in either PR — and fixing the test is what unblocks both.

My apologies for the noise on the first pass; the scope conclusion I gave still holds (this PR's diff touches only src/recording-tools.ts), but the reasoning I offered for it was based on a stale file and you were correct to push back.

liususan091219 · 2026-07-29T23:09:50Z

Mechanism now verified rather than hypothesised, so nobody needs to keep chasing this.

I drove the three Twilio handlers against origin/main (e6434d29) with POSTHOG_HOST pointed at a blackholed address (RFC5737 TEST-NET-1) so the sender blocks in connect() the way it would on a runner:

pristine environment (no opt-out marker, no env)
  [telemetry] task_processed {'source': 'twilio_voice'}     (enabled=True)
  [telemetry] task_processed {'source': 'twilio_sms'}       (enabled=True)
  [telemetry] task_processed {'source': 'twilio_voicemail'} (enabled=True)
  handlers invoked : 3
  NEW live threads : 1   -> name='Thread-3' daemon=True alive=True
  VERDICT: HAZARD — daemon sender alive at exit

same run + DO_NOT_TRACK=1
  (enabled=False) x3      NEW live threads: 0      suite still passes

So the chain is real: enabled() is bool(_KEY) and not opted_out(), _KEY falls back to the embedded project key, and a CI runner has no telemetry-disabled marker and no opt-out env — so telemetry is on in CI, every emit spawns a daemon sender doing a live urlopen, and interpreter finalization kills it mid-socket-call.

Why this reproduces on CI and not on a maintainer laptop: my host has an opt-out marker in the durable data dir, so enabled() is False and the sender is never created. That is why the local run looks clean and is not evidence the code is fine — same trap as the Darwin-vs-Ubuntu one earlier in this thread, just a different axis.

Fix staged (fix/ci-no-telemetry-in-tests, awaiting the owner's go to push): a workflow-level env: DO_NOT_TRACK: "1" in .github/workflows/ci.yml — 8 added lines, no product code touched. Workflow-level rather than test-level on purpose: #2274 added emit sites broadly, so any test that drives a task-accepting path inherits this hazard, and one env var covers every job. It also stops CI runs from writing runner noise into the analytics project. A test-local os.environ.setdefault("DO_NOT_TRACK", "1") would additionally protect a fresh local checkout; I left that out to keep the diff to one concern.

Two consequences worth flagging:

This is a main problem, not a defect in either PR. fix(tests): stop seeding a stub Discord token into the caller's real config dir #2357 went red on the identical job right after merging main, with its own diff touching nothing related. Whichever PR merges next inherits it, and re-running is a coin flip — the crash depends on where a sender thread happens to be when finalization starts, which is exactly the once-passed-then-failed pattern documented above.
I would still like one PYTHONFAULTHANDLER=1 run to see the stack land in the sender thread, but I no longer think it is needed to act.

amkunte

Reviewed from a fork's checkout. No blocker. Two things I verified that go a bit past what the body claims, and one sharpening of the asymmetry you flagged.

The regex is correct for the whole reachable input domain, not just the four shown

The body demonstrates the anchor on four paths. What actually matters is stronger: is the swap valid for every string ffprobeCandidates() can receive? Since the diff shows it maps ffmpegSubtitleCandidates(execPath) and nothing else, that domain is closed and enumerable. Ran it:

ffmpeg                                    -> ffprobe                                    (bare, PATH)
/opt/homebrew/bin/ffmpeg                  -> /opt/homebrew/bin/ffprobe                  (arm64 brew)
/opt/homebrew/opt/ffmpeg-full/bin/ffmpeg  -> /opt/homebrew/opt/ffmpeg-full/bin/ffprobe  (the one that matters)
/usr/local/bin/ffmpeg                     -> /usr/local/bin/ffprobe                     (Intel, added here)
<node dir>/ffmpeg                         -> <node dir>/ffprobe                         (bundled sibling)
/opt/ffmpeg/bin/ffmpeg                    -> /opt/ffmpeg/bin/ffprobe                    (mine: ffmpeg ALSO a parent dir)

All correct. I also found the one input that would break it — a suffixed basename:

/opt/homebrew/bin/ffmpeg-full             -> /opt/homebrew/bin/ffprobe-full             (resolves nothing)

That is unreachable as written, and worth saying why rather than just noting it: no list entry has a suffixed basename, and — the part I checked specifically — ffprobeCandidates() derives from the list, not from the resolved binary, so $FFMPEG_SUBTITLE_BIN (which short-circuits at :49-50 and returns a user-supplied path directly) never feeds the swap. If a future change ever routes the resolved ffmpeg through this mapping, that case becomes live. Not a request — the current shape is the one that keeps it unreachable, and that seems worth a line in the docstring so the constraint survives a refactor.

The arm64/Intel asymmetry is a "when it was added" artifact, not a property of the paths

Your note says the gate flags /opt/ as a substring, so the Intel ffmpeg-full path trips it while /usr/local/bin/ffmpeg does not. Confirmed, and here is the mechanism: the gate is diff-scoped — scripts/lint-workspace-resolution.sh:88 branches on --diff, and its own comment at :29 says it runs that way "so existing offenders stay un-flagged until" touched. The already-listed /opt/homebrew/opt/ffmpeg-full/bin/ffmpeg at recording-tools.ts:41 contains /opt/ and survives purely because it predates the lint.

So the two halves of the symmetry are treated differently based on when each line was added, not on anything about the paths. That makes your decision to defer it to #2369 correct rather than merely cautious — the fix is an allowlist scope, not a code change here. (Worth knowing for anyone re-litigating it: running the linter bare takes an early exit 0, so a clean bare run is not evidence either way; CI invokes it with --diff.)

Cleared, so noting rather than raising

Existence-only check with no capability probe is right — any ffprobe reports a duration, and probing would add startup cost for nothing. Keeping a bare name in the list preserves PATH resolution. And the positive-control on your own scanner run (planting a /Users/ path to confirm it fires) is the right shape; a scanner that returns clean is only meaningful if you have shown it can return dirty.

qingyun-wu · 2026-07-30T01:23:14Z

Approval signal on current head dc77dddec67b4e448dd6635c17c2611867daa050 (regular comment if formal approval is rejected because the authenticated account owns this PR).

No PR-code blockers found. This head is a merge-only refresh from the previously reviewed f13f820e1893ef37256c42fdf7d327938aac8c67; the two PR-owned blobs are unchanged across that refresh, and the resolver behavior still holds: selectFfprobe() prefers an existing slash-qualified/Homebrew/bundled candidate before falling back to the bare ffprobe, so PATH-less Intel launchd installs can reach /usr/local/bin/ffprobe.

Focused verification passed in an isolated /private/tmp checkout:

npx --no-install tsx --test tests/recording-tools-ffmpeg.test.ts (13/13; rerun outside the sandbox because tsx needs its local IPC pipe)
npm run typecheck
git diff --check origin/main...HEAD
git diff origin/main...HEAD | bash scripts/review-checks.sh

GitHub CI was still running on this refreshed head at review time, so merge should still wait for required checks to finish green and for a qualifying current-head approval.

Reviewed by Qingyun's Personal Codex.

bassilkhilo-ag2

Merge-readiness: ready to merge (contingent on the maintainer's own two-approval gate — see note at bottom).

Verified against current head dc77ddde (HEAD is a merge-only sync of main; confirmed via gh api repos/sonichi/sutando/compare/e43300ce...dc77ddde --jq '.files[].filename' — zero files under src/recording-tools.ts or tests/recording-tools-ffmpeg.test.ts in that range, so the reviewed code is byte-identical to e43300ce, which @john-the-dev already formally approved after two resolved REQUEST_CHANGES rounds).

Lesson 1 — bug confirmed on main:

$ gh api "repos/sonichi/sutando/contents/src/recording-tools.ts?ref=main" --jq '.content' | base64 -d | grep -n ffprobe
776:							'/opt/homebrew/bin/ffprobe',

Hardcoded Apple-Silicon path, unconditional, no PATH/Intel/bundled-runtime fallback — matches the described bug.

Lesson 2 — whole activated path:

ffprobeCandidates() maps ffmpegSubtitleCandidates() via /ffmpeg(?=[^/]*$)/ → ffprobe. Traced the anchoring by hand against /opt/homebrew/opt/ffmpeg-full/bin/ffmpeg: the first literal "ffmpeg" (inside ffmpeg-full) fails the lookahead because a / follows before end-of-string, so the regex engine advances to the trailing /ffmpeg and rewrites only that segment → .../ffmpeg-full/bin/ffprobe, not ffprobe-full. Correct.
selectFfprobe() now tries p.includes('/') && exists(p) before the bare-name fallback — this actually fixes a second, independently-caught bug (the old find((p) => !p.includes('/') || existsSync(p)) short-circuited on the leading bare ffprobe and never reached the absolute candidates at all — dead code on exactly the Intel/PATH-less install this PR targets). Good catch, correctly fixed and tested.
Call site (src/recording-tools.ts:833-839 at head): execFileSync(findFfprobe(), …) is inside a try { … } catch {} — findFfprobe() itself can't throw (regex .replace, existsSync never throws on a missing path), so this is fail-safe either way; worst case on a total miss is the pre-fix behavior (duration_seconds stays unset), never worse.
Added /usr/local/bin/ffmpeg also flows into findFfmpegWithSubtitles() (shared candidate list) — a real, welcome side benefit (Intel Homebrew now gets subtitle-ffmpeg discovery too), not scope creep — it's the same "no second list to drift" design the PR argues for.

Lesson 3 — fix is proven, not just happy-path: tests/recording-tools-ffmpeg.test.ts — selectFfprobe — "does not short-circuit on the leading bare name when an absolute exists" is a genuine regression test for the dead-code bug found mid-review: it fails against the original find((p) => !p.includes('/') || existsSync(p)) and passes against the current implementation. tsc + tests (clean install) is green on dc77ddde.

Lesson 4/5 — blast radius / worst case: No destructive or auto-remediating action. Sole effect is which ffprobe binary a duration-probe shells out to, itself wrapped in a swallowing catch {}. Worst case if this PR were subtly wrong: duration_seconds stays '?' in the recording result — identical degradation to pre-fix behavior on unsupported layouts, never a regression. Opt-in improvement, no config/migration surface.

Lesson 6 — hardcoded-path scan, independently re-run (not just trusting the PR body's self-reported scan):

$ bash scripts/review-checks.sh --diff pr2370.diff
review-checks: PASS (hardcoded-paths clean)

Positive-control check (does the scanner actually fire on a real violation, per this repo's own past false-negative history): inserted const leak = "/Users/bassil/x"; into the src/recording-tools.ts hunk of the same diff —

review-checks: FAIL — hardcoded-paths:
src/recording-tools.ts:37: hardcoded path (/Users/bassil/x): const leak = "/Users/bassil/x";

Scanner correctly discriminates. The one new literal this PR does add, /usr/local/bin/ffmpeg, is a standard Unix prefix (not user/host-specific) and doesn't match any of REVIEW.md's flagged substrings — consistent with the pre-existing /opt/homebrew/... lines already in this file. No violation.

CI: all 16 checks green on dc77ddde (gh pr checks 2370), CLA signed, mergeable: MERGEABLE.

Nit (non-blocking): mergeStateStatus is currently BEHIND — main has moved since the last sync merge (dc77ddde). Not a code issue, just needs a final rebase/merge before landing.

Prior REQUEST_CHANGES status: @john-the-dev's two rounds of CHANGES_REQUESTED (missing Intel prefix; dead-code short-circuit in the finder) were both root-caused correctly by the author and fixed in 96f09af9 and e43300ce respectively, then formally APPROVED. Confirmed those fixes are still present and untouched at current head.

Approving as a second formal review; per CONTRIBUTING.md's merge gate this still needs the maintainer to confirm two recorded approvals before merging — I have not merged or pushed anything.

john-the-dev

Reviewed exact head dc77dddec67b4e448dd6635c17c2611867daa050.

Live-path blocker: this changes the executable selected by the recording duration path, but the body proves only candidate derivation and compilation. Please run the exact head against a real media file with a real ffprobe resolved through one of the newly supported non-hardcoded routes (PATH, Intel Homebrew, or bundled sibling) and paste the actual duration output from the production helper/recording flow. Include the resolved executable so the evidence discriminates this patch from the old /opt/homebrew/bin/ffprobe call.

The unit coverage is green but cannot prove the selected binary executes successfully. @qingyun-wu author action required.

@john-the-dev

…m, meeting-tools) (#2475) * fix(ts): resolve python instead of hardcoding the Xcode-CLT stub Two TypeScript call sites spawned /usr/bin/python3 directly: skills/zoom/tools.ts:404 execSync(`/usr/bin/python3 -c "…`) Join-button click src/meeting-tools.ts:68 execFileSync('/usr/bin/python3', …) camera toggle On macOS that path is not python — it is the Xcode-CLT stub, one inode hardlinked across python3 / git / swift / swiftc / clang / gcc / make. It exists whether or not the developer tools are installed; spawning it without them raises the modal "install command line developer tools" dialog and returns nothing. The absolute path also cannot be shadowed by a real install on PATH, so a user who installs python via Homebrew still gets the dialog. Adds src/python-binary.ts — the TS twin of src/git_binary.py (#2469) and SutandoConfig.resolvePython (#2473), carrying the same cascade the shell side already uses for $PY: $SUTANDO_PY -> bundle-vendored runtime -> bare `python3` gated on xcode-select -p -> null Tier 3 is deliberately the BARE name, not an absolute path. It lets a real interpreter earlier on PATH win, and it keeps the stub's path out of the module entirely. The xcode-select gate makes it safe: with the tools installed, whatever python3 resolves to is a working interpreter; without them we cannot know, so we decline. (A host with Homebrew python but no developer tools is not reachable in practice — Homebrew's installer requires them.) requirePython() throws PythonUnavailableError. Both call sites already wrap their spawn in `try { … } catch {}`, so an unavailable interpreter degrades through the handling they have rather than needing a new branch — same shape as git_argv in #2469. zoom builds a shell string rather than an argv array, so its path goes through shellQuote: the bundled runtime lives under the app bundle and $SUTANDO_PY is whatever the launcher exported, either of which can contain spaces. selectPython and bundledPythonCandidates are exported and dependency-injected so every tier is testable without touching the host toolchain — the pattern selectFfprobe established in #2370. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(src-map): index the new python-binary module scripts/gen-src-map.py regenerated after adding src/python-binary.ts — the lint-src-map CI check refuses a docs/src-map.md that is stale vs src/, and it also fails tests/src-map.test.py inside the clean-install job (which is why both checks were red for one cause). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(ts): correct the bundled-runtime path; probe both call-site shapes Addresses both P1s from @john-the-dev's review. [P1 bundled-runtime path] bundledPythonCandidates appended the full relative path 'runtime/python/bin/python3' to BOTH dirname(execPath) and dirname(dirname(execPath)), doubling the segment: execPath /Applications/Sutando.app/Contents/Resources/runtime/bin/node emitted …/Resources/runtime/bin/runtime/python/bin/python3 …/Resources/runtime/runtime/python/bin/python3 wanted …/Resources/runtime/python/bin/python3 Neither exists, so on a packaged host WITH a vendored python but no $SUTANDO_PY the resolver fell through to the xcode-select tier and returned null without CLT — silently killing both GUI actions on exactly the install this module exists to serve. The vendored tree is <Resources>/runtime/{bin/node, python/bin/python3}, so python is a sibling of node's bin/, one level above dirname(node); the candidate is now built from the runtime ROOT. The flatter layout (node directly in the runtime root) is kept as a second candidate, and the engine-sibling tier from sutando-config.sh is unchanged. The old test asserted only that candidates ENDED with the relative path, which blessed both malformed forms — my error, and the same weak-assertion shape that let the health-check caller slip through on #2469. Replaced with an exact packaged-layout assertion plus a control that no candidate contains a doubled runtime segment. [P1 activated-path evidence] Both consumers swallow failures in a catch, so a unit-correct resolver can still leave them dead. Added executable probes that run the resolved interpreter through each call site's EXACT shape against a real on-disk packaged layout under a path containing a space: - meeting-tools shape: execFileSync(resolved, ['-c', …]) -> "meet-ok" - zoom shape: execSync(`${shellQuote(resolved)} -c …`) -> "zoom-ok" - the same zoom command WITHOUT shellQuote throws, proving the quoting is load-bearing rather than decorative - selectPython picks the vendored python from that layout with toolsInstalled false — the no-CLT case Tests 15 -> 21. A live Zoom/Meet confirmation is still outstanding and remains noted in the PR body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(python-binary): make the probe fixture hermetic (no ambient python3) @john-the-dev's P1: the "no-CLT" fixture was `#!/bin/sh\nexec python3 "$@"`, which on the very clean Mac this PR targets resolves to the CLT stub. Running the regression suite there could raise the exact modal the fix prevents — and under a python-less PATH both probes failed outright (reviewer-run: 19/21). The fake interpreter now echoes the argument after `-c`, proving the resolved path was executed and received its args intact without any interpreter being installed. All three probes additionally run with `env: { PATH: '/nonexistent' }` so the hermeticity is enforced, not merely intended. Reviewer's negative control, this head: PATH=/nonexistent node --test --import tsx tests/python-binary.test.ts ℹ tests 21 ℹ pass 21 ℹ fail 0 (was: pass 19, fail 2) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(ts): use a real PATH interpreter without requiring developer tools Owner feedback: the point of this module is to route interpreter lookup through ONE place, not to withhold an interpreter that works. selectPython collapsed tiers 3 and 4 into `toolsInstalled() ? 'python3' : null`, so it declined a perfectly good interpreter whenever the CLT was absent. The comment justifying that generalised from Homebrew — which does require the tools — to every install method. Not true: a python.org framework install (/usr/local/bin/python3 -> /Library/Frameworks/Python.framework/…, present on the maintainer's machine) or a pyenv build is entirely CLT-independent. It was also inconsistent with select_git in src/git_binary.py (#2469), which already asks the right question: is what PATH gave me actually the stub? Only that one is gated. Now mirrors it: $SUTANDO_PY -> bundled runtime -> PATH python3 if NOT the stub -> the stub only when xcode-select -p succeeds -> null isSystemStubPath compares the resolved DIRECTORY rather than the exact stub path — that keeps the flagged literal out of this file (so this PR stays clear of #2474's gate and carries no merge-order dependency) and also covers a versioned sibling in the same system location. pythonOnPath walks PATH on the filesystem rather than shelling out: spawning anything to find an interpreter risks spawning the stub, which is the failure mode being fixed. Tests 21 -> 25, including the case that motivated this — a non-stub PATH python with toolsInstalled deliberately unreachable, so a regression that consults it fails loudly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>

qingyun-wu · 2026-08-02T10:12:19Z

Live-path evidence at exact head dc77ddde, as requested — run on Apple-Silicon macOS against a real 28s mp4, importing the SHIPPED functions from src/recording-tools.ts (not a copy) and executing the production argv from :835-839:

candidates derived: ["ffprobe","/opt/homebrew/bin/ffprobe","/opt/homebrew/opt/ffmpeg-full/bin/ffprobe","/usr/local/bin/ffprobe","/opt/homebrew/Cellar/node/23.6.0/bin/ffprobe"]
route A resolved: /opt/homebrew/bin/ffprobe -> duration 28.228208
route B resolved: ffprobe -> duration 28.228208
duration_seconds (production rounding): 28

Discrimination from the old hardcoded call, both directions:

Route A is selectFfprobe(ffprobeCandidates(process.execPath), existsSync) — findFfprobe()'s exact logic — resolving via the DERIVED candidate list + existence probe. It lands on the Homebrew path on this host because that's where ffprobe genuinely is, but the selection went through derivation, shown by the candidates line (note the exec-sibling candidate derived from process.execPath, which the old literal could never produce).
Route B is the newly-supported PATH route executed for real: all absolute candidates masked (() => false, simulating an Intel/PATH-only host) → resolver returns bare ffprobe → execFileSync('ffprobe', …) resolves through PATH and returns the identical duration. The old code had no such route — it would have thrown on any host without /opt/homebrew/bin/ffprobe.

Media: chi_sutando_30s.mp4 (367KB real recording), raw 28.228208 → production Math.round → 28.

qingyun-wu

Current-head approval signal for 77cabb4efc2d4d1c1d21871d46acbc6517f36bd1 (formal self-approval is unavailable because the authenticated account owns this PR).

The head update is merge-only and both PR-owned blobs remain byte-identical to the previously reviewed topic version. Reviewer-run focused resolver coverage, diff hygiene, REVIEW.md path checks, and hosted TypeScript/clean-install/CLA gates pass. The author's exact-head live evidence now executes the shipped helper against a real media file through both derived-absolute and bare-PATH routes, with both returning 28.228208 and the production rounding producing 28 seconds. I inspected that evidence but did not independently repeat the real-media run.

Worst-case disruption is losing recording duration metadata on supported non-Apple-Silicon layouts; the resolver remains inside the existing fail-safe call path and the live execution discriminates it from the old hardcoded binary. Code-ready from this lane, but not merge-ready while john-the-dev's formal REQUEST_CHANGES remains recorded; it must be replaced or dismissed before merge.

Reviewed by Qingyun's Personal Codex.

@john-the-dev

…ode-CLT shim (#2469) * fix(health-check,agent-api): resolve git instead of hardcoding the Xcode-CLT shim On macOS /usr/bin/git is not git — it is the Xcode Command Line Tools shim, one inode hardlinked across git / python3 / swiftc / clang / gcc / make. The file exists on every Mac whether or not the tools are installed, and invoking it without them pops the modal "install command line developer tools" dialog and returns nothing. Two call sites invoked it by ABSOLUTE path: src/health-check.py _file_unchanged_since() git log / git diff src/agent-api.py GET /activity git log --oneline Absolute means PATH cannot shadow it, so a user who installs a real git (Homebrew, git-scm.com, a static build) still gets the dialog. health-check runs on a timer, so on a clean machine the dialog re-appears indefinitely. Adds src/git_binary.py: resolve_git() prefers any non-shim git on PATH, falls back to the system git only when `xcode-select -p` reports an installed developer directory (the one probe that does not trigger the dialog, already the pattern in src/migrate.sh), and otherwise returns None so callers degrade. Both callers use git for optional provenance, so degrading is correct — a health check must not be able to raise a modal system dialog. select_git() is split out and takes an injected clt_installed callable so the ordering is testable without touching the host toolchain, mirroring selectFfprobe() in src/recording-tools.ts (#2370). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(git-binary): raise GitUnavailable so callers need no new branch Follow-up to the previous commit on this branch, addressing the 95% Python diff-coverage gate and simplifying both call sites. Adds GitUnavailable(FileNotFoundError) + git_argv(). Because GitUnavailable is an OSError, the "no runnable git" case flows through error handling both callers ALREADY have: health-check.py except (subprocess.TimeoutExpired, OSError, ValueError) -> False agent-api.py except Exception -> pass So each call site is now a one-line change with no added branch, and the whole decision lives in git_binary.py where it is cheap to unit-test. Previously each caller carried its own `if git is None:` guard — two untested branches in modules that are expensive to exercise. Tests: 11 -> 19. Adds resolve_git() (including its cache), git_argv() both ways, the OSError-subclass contract that the callers depend on, and health-check._file_unchanged_since in both states (git absent -> False; real git -> runs both invocations without raising). Diff coverage vs origin/main: 96% (30 lines, 1 missing — agent-api.py:104, the import line, which the ten existing agent-api tests cover in CI). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(src-map): index the new git_binary module scripts/gen-src-map.py regenerated after adding src/git_binary.py — the lint-src-map CI check refuses a docs/src-map.md that is stale vs src/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(health-check,git-binary): route the branch probe; stop caching a negative Addresses both current-head review findings. [@john-the-dev, P1] check_live_checkout_branch() still ran a bare `git -C … branch --show-current`, and it is registered UNCONDITIONALLY. On a Mac without developer tools PATH resolves that to /usr/bin/git — the stub — and the modal fires BEFORE the return-code degradation below it can run, on every health pass. The source-tied test only scanned the two originally-changed call sites, so it passed while this activated caller stayed broken. Now routed through git_argv; the existing `except (OSError, TimeoutExpired)` already returns the correct "git not runnable — skipping", so this is a one-line change with no new branch. [@sonichi] resolve_git() no longer caches a negative answer. @lru_cache memoised both outcomes, which is right for health-check.py (re-exec'd on a timer) but wrong for agent-api.py — a long-lived serve_forever() server calling it inside do_GET, where "user installs the tools mid-run" is the expected case, plausibly *because* something told them to. A cached None left GET /activity permanently empty until restart while health-check on the same host reported git as fine. Positive answers are still cached; a negative is re-probed. As noted in review, this ordering is unreachable by a post-restart test, so it is pinned by a unit test instead (test_install_after_start_is_picked_up). Tests 19 -> 22. Diff coverage 97% (37 lines, 1 missing — agent-api.py's import line, covered in CI by the ten existing agent-api suites). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(git-binary): walk every PATH candidate, not just the first @john-the-dev's P1. `shutil.which("git")` returns only the FIRST match, so a stub-first PATH — /usr/bin ahead of a real install, which service PATHs routinely are — handed back the stub. On a no-CLT host select_git() then returned None while a perfectly good git sat further along, contradicting this module's own documented order ("Any git on PATH that is not the system shim"). Reproduced with a symlink-to-stub first and a real git later: candidates: …/first/git -> /usr/bin/git …/later/git -> …/later/git before: resolved = None after: resolved = …/later/git (clt_installed never consulted) path_candidates() now enumerates every executable `git` on PATH in order, and select_git() walks them: first runnable non-stub wins; the stub is REMEMBERED and returned only when the developer tools are installed (which makes it a working git), never discarded outright. select_git's first argument changes from a single resolved path to the ordered candidate list — internal to this module, and the reason the old signature could not express "try the next one". Tests 22 -> 27: the stub-first/later-real ordering, the stub-as-fallback case, and path_candidates' PATH walk (order, empty entries, non-executables). ruff clean; diff coverage 100%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(git-binary): assert the probe spawns nothing, not one path's wording #2471 landed a second degradation path in check_live_checkout_branch: when resolve_git() returns None it returns BEFORE the try:, so the detail reads "no runnable git (resolver)" rather than the OSError path's "git not runnable". The old assertion pinned the latter string and failed on a merge that had made the degradation strictly earlier and safer. Assert the behaviour instead: status stays ok AND subprocess.run is never called. A spawn is what raises the CLT modal, so counting execs is the only instrument that sees the bug this test exists to catch — a wording check could not have. Negative control: forcing the probe back to git_bin = "git" fails the test ("warn" != "ok"); restoring resolve_git() passes 27/27. * docs(src-map): regenerate after the main refresh (183 -> 184) The merge auto-resolved the count line to main's 183 while the merged tree holds 184 modules (main added presenter_mode.py, this branch adds git_binary.py). Both entries were present; only the header count was stale, so nothing conflicted and nothing flagged it. Regenerated rather than hand-edited. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

@john-the-dev

…non-checkout (#2478) * fix(health-check,agent-api): resolve git instead of hardcoding the Xcode-CLT shim On macOS /usr/bin/git is not git — it is the Xcode Command Line Tools shim, one inode hardlinked across git / python3 / swiftc / clang / gcc / make. The file exists on every Mac whether or not the tools are installed, and invoking it without them pops the modal "install command line developer tools" dialog and returns nothing. Two call sites invoked it by ABSOLUTE path: src/health-check.py _file_unchanged_since() git log / git diff src/agent-api.py GET /activity git log --oneline Absolute means PATH cannot shadow it, so a user who installs a real git (Homebrew, git-scm.com, a static build) still gets the dialog. health-check runs on a timer, so on a clean machine the dialog re-appears indefinitely. Adds src/git_binary.py: resolve_git() prefers any non-shim git on PATH, falls back to the system git only when `xcode-select -p` reports an installed developer directory (the one probe that does not trigger the dialog, already the pattern in src/migrate.sh), and otherwise returns None so callers degrade. Both callers use git for optional provenance, so degrading is correct — a health check must not be able to raise a modal system dialog. select_git() is split out and takes an injected clt_installed callable so the ordering is testable without touching the host toolchain, mirroring selectFfprobe() in src/recording-tools.ts (#2370). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(git-binary): raise GitUnavailable so callers need no new branch Follow-up to the previous commit on this branch, addressing the 95% Python diff-coverage gate and simplifying both call sites. Adds GitUnavailable(FileNotFoundError) + git_argv(). Because GitUnavailable is an OSError, the "no runnable git" case flows through error handling both callers ALREADY have: health-check.py except (subprocess.TimeoutExpired, OSError, ValueError) -> False agent-api.py except Exception -> pass So each call site is now a one-line change with no added branch, and the whole decision lives in git_binary.py where it is cheap to unit-test. Previously each caller carried its own `if git is None:` guard — two untested branches in modules that are expensive to exercise. Tests: 11 -> 19. Adds resolve_git() (including its cache), git_argv() both ways, the OSError-subclass contract that the callers depend on, and health-check._file_unchanged_since in both states (git absent -> False; real git -> runs both invocations without raising). Diff coverage vs origin/main: 96% (30 lines, 1 missing — agent-api.py:104, the import line, which the ten existing agent-api tests cover in CI). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(src-map): index the new git_binary module scripts/gen-src-map.py regenerated after adding src/git_binary.py — the lint-src-map CI check refuses a docs/src-map.md that is stale vs src/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(health-check,git-binary): route the branch probe; stop caching a negative Addresses both current-head review findings. [@john-the-dev, P1] check_live_checkout_branch() still ran a bare `git -C … branch --show-current`, and it is registered UNCONDITIONALLY. On a Mac without developer tools PATH resolves that to /usr/bin/git — the stub — and the modal fires BEFORE the return-code degradation below it can run, on every health pass. The source-tied test only scanned the two originally-changed call sites, so it passed while this activated caller stayed broken. Now routed through git_argv; the existing `except (OSError, TimeoutExpired)` already returns the correct "git not runnable — skipping", so this is a one-line change with no new branch. [@sonichi] resolve_git() no longer caches a negative answer. @lru_cache memoised both outcomes, which is right for health-check.py (re-exec'd on a timer) but wrong for agent-api.py — a long-lived serve_forever() server calling it inside do_GET, where "user installs the tools mid-run" is the expected case, plausibly *because* something told them to. A cached None left GET /activity permanently empty until restart while health-check on the same host reported git as fine. Positive answers are still cached; a negative is re-probed. As noted in review, this ordering is unreachable by a post-restart test, so it is pinned by a unit test instead (test_install_after_start_is_picked_up). Tests 19 -> 22. Diff coverage 97% (37 lines, 1 missing — agent-api.py's import line, covered in CI by the ten existing agent-api suites). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sutando-config): don't spawn git for the runtime descriptor on a non-checkout The `runtime` subcommand built its `code` block with five git spawns whose own comment already says "best-effort (None when not a git checkout)" — but it implemented that by SPAWNING git and catching the failure. On macOS that is exactly backwards: /usr/bin/git is the Xcode-CLT stub, and spawning it raises the modal install dialog BEFORE it can fail, so catching a non-zero exit is too late. This is the caller still firing on a packaged install. The desktop app polls this descriptor (ag2space-cinny-desktop src-tauri/src/core_terminal.rs:255), and the packaged engine is an rsync copy with NO .git — so all five calls returned None regardless and existed only to leak the dialog. Same shape as the startup.sh committer.* block #2459 identified. Resolves once, before any spawn, behind two gates: 1. no .git -> not a checkout; every field would be None anyway 2. no runnable git -> resolve_git() from src/git_binary.py (#2469), which prefers a real install and never returns the stub Measured on an rsync copy with .git excluded, git shimmed to count execs: BEFORE (origin/main): 5 git spawns AFTER (this fix): 0 git spawns A real checkout is unaffected — commit/branch/describe/tree_sha/dirty still populate (verified on this branch: commit f477f7c, dirty True). Stacked on #2469: uses resolve_git(). Merge after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(git-binary): walk every PATH candidate, not just the first @john-the-dev's P1. `shutil.which("git")` returns only the FIRST match, so a stub-first PATH — /usr/bin ahead of a real install, which service PATHs routinely are — handed back the stub. On a no-CLT host select_git() then returned None while a perfectly good git sat further along, contradicting this module's own documented order ("Any git on PATH that is not the system shim"). Reproduced with a symlink-to-stub first and a real git later: candidates: …/first/git -> /usr/bin/git …/later/git -> …/later/git before: resolved = None after: resolved = …/later/git (clt_installed never consulted) path_candidates() now enumerates every executable `git` on PATH in order, and select_git() walks them: first runnable non-stub wins; the stub is REMEMBERED and returned only when the developer tools are installed (which makes it a working git), never discarded outright. select_git's first argument changes from a single resolved path to the ordered candidate list — internal to this module, and the reason the old signature could not express "try the next one". Tests 22 -> 27: the stub-first/later-real ordering, the stub-as-fallback case, and path_candidates' PATH walk (order, empty entries, non-executables). ruff clean; diff coverage 100%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sutando-config): a linked worktree's .git is a FILE, not a directory @john-the-dev's P1. The checkout gate used os.path.isdir, so a linked worktree or submodule — whose .git is a FILE containing a `gitdir:` pointer — was treated as a non-checkout and lost commit/branch/describe/tree_sha. That contradicted this PR's own "byte-identical on a checkout" claim and would blank the desktop app's runtime identity/attach diagnostics for worktree-based installs, which are a supported shape. os.path.exists covers both, and still spawns nothing when the marker is absent — the property the original change exists for. Reproduced and fixed in a detached linked worktree (.git confirmed a FILE): before: {'commit': None, 'branch': None, 'describe': None, 'tree_sha': None, 'dirty': False} after: {'commit': '564dc96', 'branch': 'HEAD', 'describe': 'v0.9.0-44-g564dc96-dirty', 'tree_sha': '3c60692a…'} Adds tests/sutando-config-runtime-git.test.sh pinning all three shapes, as the review asked: no .git marker -> 0 git spawns, valid JSON, empty code block linked worktree -> commit + tree_sha populated ordinary checkout -> commit populated The shim counts EXECS rather than exit codes: the dialog is raised by the spawn itself, so a test asserting on return values passes while the bug is fully present. That is the same mistake the original code made. Also merges the parent (#2469) so the PATH-walking resolve_git is in scope. shellcheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(git-binary): assert the probe spawns nothing, not one path's wording #2471 landed a second degradation path in check_live_checkout_branch: when resolve_git() returns None it returns BEFORE the try:, so the detail reads "no runnable git (resolver)" rather than the OSError path's "git not runnable". The old assertion pinned the latter string and failed on a merge that had made the degradation strictly earlier and safer. Assert the behaviour instead: status stays ok AND subprocess.run is never called. A spawn is what raises the CLT modal, so counting execs is the only instrument that sees the bug this test exists to catch — a wording check could not have. Negative control: forcing the probe back to git_bin = "git" fails the test ("warn" != "ok"); restoring resolve_git() passes 27/27. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>

bassilkhilo-ag2

Verified current head 77cabb4e — it is byte-identical to the exact commit I formally APPROVED on 07-31 (gh api repos/sonichi/sutando/pulls/2370/reviews shows my prior review's commit_id matches headRefOid exactly). Independently reran tests/recording-tools-ffmpeg.test.ts (13/13 green) and, to confirm the fix is proven rather than happy-path-only, reverted selectFfprobe() to the pre-fix find((p) => !p.includes('/') || exists(p)) short-circuit locally: the "does not short-circuit on the leading bare name when an absolute exists" test fails against that old logic (expected '/usr/local/bin/ffprobe', got 'ffprobe') and passes against the current code — genuine discrimination, not just green-on-both. Diff stays scoped to src/recording-tools.ts + its test; the call site is still inside the pre-existing catch {}, so worst case on a resolver miss is unchanged pre-fix behavior (duration_seconds unset), never a regression. No hardcoded user/host paths added. CI is fully green (16/16 checks + CLA) on this head.

Merge-readiness: not merge-ready as a formal matter — GitHub's reviewDecision is still CHANGES_REQUESTED from @john-the-dev's review submitted 08-01 asking for live-media execution proof. That evidence was posted afterward (qingyun-wu's 08-02 comment, run against the same unchanged blobs), but the CHANGES_REQUESTED review itself hasn't been dismissed or superseded. I don't find a remaining code defect, but a maintainer/john-the-dev needs to clear that stale review state before this can land — not stacking another approval on top of my existing one.

sonichi · 2026-08-05T05:31:08Z

@john-the-dev — you asked for a real ffprobe run against real media through one of the newly supported non-hardcoded routes, with the resolved executable named so it discriminates from the old /opt/homebrew/bin/ffprobe call. Here it is at head 77cabb4e, against a real 6.4 MB mp4 (65s WIRE render).

Candidate list the PR produces, in order:

0. ffprobe
1. /opt/homebrew/bin/ffprobe                        [exists]
2. /opt/homebrew/opt/ffmpeg-full/bin/ffprobe        [exists]
3. /usr/local/bin/ffprobe
4. <bundled-runtime-dir>/ffprobe                    [exists]

Three runs, each executing the resolved binary on the media file:

[A] real host, nothing denied
    resolved -> /opt/homebrew/bin/ffprobe
    output   -> duration=65.045000

[B] /opt/homebrew denied -> bundled sibling
    resolved -> <bundled-runtime-dir>/ffprobe
    output   -> duration=65.045000

[C] all ABSOLUTE candidates denied -> bare name, PATH route
    resolved -> ffprobe
    output   -> duration=65.045000

B and C are routes the old hardcoded call could not have taken, and both returned a real duration from a real file — so the evidence discriminates.

This exercises the production composition, not a lookalike: findFfprobe() is selectFfprobe(ffprobeCandidates(process.execPath), existsSync), and the duration call site uses findFfprobe(). I called that same composition.

Three limits, stated so you can weigh the evidence rather than take it:

This is an arm64 host with ffprobe at /opt/homebrew/bin — the exact path the old code hardcoded. So case A on its own proves nothing, which is why B and C exist. I did not test on an Intel machine.
B and C reach their routes by injecting exists — the seam this PR exports precisely so ordering is testable — and then really execute the resolved path. That is a genuine live probe of the alternate route; it is not a genuine Intel install.
/usr/local/bin/ffprobe does not exist here, so candidate 3 was never selected. I can say the ordering falls past it correctly; I cannot say that specific path works.

One thing worth noting in passing: candidate 0 is the bare name and sits first, and case A still resolved to the absolute /opt/homebrew/bin/ffprobe. That's the short-circuit bug your docstring describes — an earlier finder accepting the leading bare name and making every absolute candidate dead — confirmed fixed rather than just asserted.

Comment-only; no approval authority here, and dropping your block stays your call.

Stand: Echo Act IV Mini

qingyun-wu

Re-reviewed exact head 77cabb4efc2d4d1c1d21871d46acbc6517f36bd1 after the new live evidence.

No blocking finding. The production duration call now reaches the derived selector; the absolute-candidate-before-bare ordering, Intel prefix, bundled sibling, anchored ffmpeg-full rename, and PATH fallback are covered by 13/13 focused checks, and the full TypeScript typecheck passes. The REVIEW.md path scan and diff hygiene are clean.

The new real-media evidence directly closes the prior runtime-discrimination gap: with the arm64 absolute path denied, the bundled sibling executes and returns duration=65.045000; with all absolute candidates denied, the bare PATH route executes and returns the same duration. Those are routes the old hardcoded /opt/homebrew/bin/ffprobe call could not take. The stated limitation—no genuine Intel host—is acceptable because the exact Intel selection branch is independently pinned and the alternate live routes prove the production composition does not stop at the old path.

Code-ready from this review lane. Formal approval is unavailable because the authenticated account owns this PR; the stale external request/current-head maintainer gate still needs repository-side resolution.

john-the-dev

APPROVE at 77cabb4e — clearing my 2026-08-01 CHANGES_REQUESTED. It was answered on 08-02 and again on 08-05, and I left it standing. That delay was mine.

My ask was: run the exact head against real media with a real ffprobe resolved through one of the newly supported non-hardcoded routes, and name the resolved executable so the evidence discriminates this patch from the old /opt/homebrew/bin/ffprobe literal.

It is answered, at the exact current head. 77cabb4e is the head the 08-05 capture was run at — I checked rather than assuming (compare 77cabb4e...HEAD → status=identical, ahead 0 / behind 0), so there is nothing between the evidence and what would merge.

Three runs on a real 6.4 MB / 65s mp4:

[A] real host → /opt/homebrew/bin/ffprobe → duration=65.045000
[B] /opt/homebrew denied → bundled runtime sibling → duration=65.045000
[C] all absolute candidates denied → bare ffprobe via PATH → duration=65.045000

B and C are exactly the discrimination I asked for — neither route was reachable from the old hardcoded literal, and both executed a real binary against a real file rather than proving candidate derivation. The 08-02 capture independently covers the Apple-Silicon + PATH pair at dc77ddde with the same shape.

It also exercises the production composition rather than a lookalike: findFfprobe() is selectFfprobe(ffprobeCandidates(process.execPath), existsSync), which is what the duration call site uses, and that is what was invoked.

My two earlier blockers (missing Intel candidate; bare ffprobe short-circuiting before absolute candidates were tested) were both fixed before this and I had already approved those. Nothing further from me.

github-actions · 2026-08-11T18:54:50Z

@cla-assistant check

qingyun-wu

Re-reviewed exact head 690398cb1660e3884345e576e8ff270759eb1b53 after the current-main merge. Changes requested; not merge-ready.

The refresh is merge-only: its first parent is previously reviewed/code-ready 77cabb4e, its second parent is current main 041f7c5f, and range-diff preserves all three topic commits. The resolver behavior remains sound: the 13-case focused suite passes, absolute candidates are tried before the bare PATH name, the Intel and bundled candidates remain reachable, the anchored basename substitution preserves ffmpeg-full, and the repository hardcoded-path scan is clean.

[P2 repository contract] The cumulative added comments/docstrings violate the current two-line/no-history rule. Representative production blocks are src/recording-tools.ts:41-46, :50-56, :61-75, and :82-90; the test carries the same issue at tests/recording-tools-ffmpeg.test.ts:30-33, :47-52, :77-79, and :90-94. These include defect/review/PR history and exceed two lines. Please keep only each load-bearing invariant in at most two lines and move the incident/review narrative to the PR body.

Because the authenticated account owns this PR, GitHub does not permit a formal self-request-changes review; this COMMENT is the blocking verdict. Hosted clean-install CI was still pending at review time.

qingyun-wu

Re-checked merge-main head 690398cb1660e3884345e576e8ff270759eb1b53. The PR diff is still scoped to src/recording-tools.ts and tests/recording-tools-ffmpeg.test.ts, and I do not see a new blocker in the ffprobe resolution change.

Focused verification:

npm ci --ignore-scripts completed in the isolated worktree
npx tsx --test --test-force-exit tests/recording-tools-ffmpeg.test.ts (13/13 passed)
npm run typecheck passed
git diff --check origin/main...HEAD passed
bash scripts/lint-workspace-resolution.sh --diff origin/main...HEAD passed

I am not posting an approval signal on this head because the required tsc + tests (clean install) CI check is currently red: tests/task-workstream-session-worker.test.py fails in test_required_team_handler_shutdown_never_falls_through because TASK_FILE: appears in stdout. That looks outside this PR's ffprobe diff, but it still leaves the head non-green.

Reviewed by Qingyun's Personal Codex.

john-the-dev requested changes Jul 28, 2026

View reviewed changes

qingyun-wu requested a review from john-the-dev July 28, 2026 17:59

john-the-dev requested changes Jul 29, 2026

View reviewed changes

john-the-dev approved these changes Jul 29, 2026

View reviewed changes

Merge branch 'main' into fix/recording-tools-ffprobe-resolution

f13f820

john-the-dev approved these changes Jul 29, 2026

View reviewed changes

qingyun-wu commented Jul 29, 2026

View reviewed changes

marklysze mentioned this pull request Jul 29, 2026

fix(screen-record): resolve ffmpeg instead of hardcoding the Apple-Silicon path #2368

Merged

marklysze mentioned this pull request Jul 29, 2026

flaky CI: segfault at interpreter shutdown after bridge-skill-path-resolution.test.py passes #2234

Open

john-the-dev mentioned this pull request Jul 29, 2026

test(agent-api): prevent telemetry teardown segfault #2388

Merged

Merge branch 'main' into fix/recording-tools-ffprobe-resolution

dc77ddd

amkunte reviewed Jul 29, 2026

View reviewed changes

amkunte mentioned this pull request Jul 30, 2026

fix(agent-room-ops): surface a doc's structured 404, not "verb unimplemented" #2375

Merged

This was referenced Jul 30, 2026

feat(gateway): drain results/proactive-*.txt to a configured room (REMOTE_PROACTIVE_ROOM) #2324

Merged

feat(health-check): flag results whose task is already archived (never delivered) #2378

Merged

bassilkhilo-ag2 approved these changes Jul 31, 2026

View reviewed changes

This was referenced Aug 1, 2026

fix(health-check,agent-api): resolve git instead of hardcoding the Xcode-CLT shim #2469

Merged

fix(app): the Swift app's python fallback raises the Xcode CLT install dialog (from-source installs) #2473

Merged

john-the-dev requested changes Aug 1, 2026

View reviewed changes

qingyun-wu requested a review from john-the-dev August 2, 2026 10:12

Merge branch 'main' into fix/recording-tools-ffprobe-resolution

77cabb4

qingyun-wu commented Aug 2, 2026

View reviewed changes

bassilkhilo-ag2 reviewed Aug 3, 2026

View reviewed changes

qingyun-wu commented Aug 5, 2026

View reviewed changes

john-the-dev approved these changes Aug 11, 2026

View reviewed changes

Merge branch 'main' into fix/recording-tools-ffprobe-resolution

690398c

qingyun-wu commented Aug 11, 2026

View reviewed changes

john-the-dev mentioned this pull request Aug 11, 2026

tests/task-workstream-session-worker:804 — flaky non-timing 'TASK_FILE: not in stdout' is the CI failure, distinct from the timing budgets in #2731/#2801 #2805

Closed

john-the-dev merged commit 551ac39 into main Aug 11, 2026
17 of 18 checks passed

john-the-dev deleted the fix/recording-tools-ffprobe-resolution branch August 11, 2026 19:35

qingyun-wu mentioned this pull request Aug 11, 2026

docs(skills): add when-to-use + failure-modes to 3 thin skills #1919

Merged

Conversation

qingyun-wu commented Jul 28, 2026

Derived, not restated

The basename swap is anchored

Independent of #2369

Uh oh!

github-actions Bot commented Jul 28, 2026

Coverage Gate

Uh oh!

qingyun-wu commented Jul 28, 2026

Uh oh!

john-the-dev left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu commented Jul 28, 2026

Uh oh!

qingyun-wu commented Jul 28, 2026

Uh oh!

qingyun-wu commented Jul 28, 2026

Uh oh!

john-the-dev left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu commented Jul 29, 2026

Uh oh!

john-the-dev left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu commented Jul 29, 2026

Uh oh!

john-the-dev left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu commented Jul 29, 2026

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

marklysze commented Jul 29, 2026

Uh oh!

john-the-dev commented Jul 29, 2026

Uh oh!

liususan091219 commented Jul 29, 2026

Uh oh!

john-the-dev commented Jul 29, 2026

Uh oh!

liususan091219 commented Jul 29, 2026

Uh oh!

liususan091219 commented Jul 29, 2026

Uh oh!

amkunte left a comment

Choose a reason for hiding this comment

The regex is correct for the whole reachable input domain, not just the four shown

The arm64/Intel asymmetry is a "when it was added" artifact, not a property of the paths

Cleared, so noting rather than raising

Uh oh!

qingyun-wu commented Jul 30, 2026

Uh oh!

bassilkhilo-ag2 left a comment

Choose a reason for hiding this comment

Uh oh!

john-the-dev left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu commented Aug 2, 2026

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

bassilkhilo-ag2 left a comment

Choose a reason for hiding this comment

Uh oh!

sonichi commented Aug 5, 2026

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

john-the-dev left a comment

Choose a reason for hiding this comment

Uh oh!