Repository navigation
tidy: centralize optional bridge script execution - #2495
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 71%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
src/discord-bridge.pyLines 429-437 429 Optional — if the skill is absent the caller falls back to [File attached:].
430 Errors are swallowed; transcription failure must never block task delivery.
431 """
432 skill_script = Path(__file__).parent.parent / "skills" / "audio-transcribe" / "scripts" / "transcribe.py"
! 433 return _run_optional_script_shared(
434 skill_script,
435 [local_path],
436 timeout=25,
437 on_error=lambda exc: print( |
|
Live audio evidence for current head I restarted the Discord bridge directly from this PR's worktree with its normal runtime environment. The owner sent a real Discord mobile voice message saying “central runner live test.” The patched bridge downloaded the The emitted task recorded Result: voice attachment download → shared runner → Gemini transcript → owner-tier task → Discord result delivery all passed. For completeness, a Slack attempt stopped before the changed boundary: Slack returned an HTML sign-in page instead of |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 1bbd4b159f46fb2fea721ef1e2ef43a3868312b9.
No blocking findings. The shared runner remains provider- and skill-neutral: each bridge still resolves and injects the audio-transcribe script path, while the helper preserves the prior interpreter, timeout, success/empty/nonzero, and exception logging contract. All three wrappers delegate without changing fallback behavior.
Reviewer-run audio-transcribe-skill passes 22/22, including direct runner contracts, all three adapter wiring paths, symlink resolution, nonzero/empty/timeout behavior, and real module imports. Diff hygiene, source-map consistency, the REVIEW.md path gate, required GitHub checks, and CLA pass.
Worst-case disruption is an optional transcription failure blocking message delivery; the helper stays fail-open and the regressions cover that boundary. APPROVE for this exact head. Normal second-maintainer merge gate still applies.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved on exact head 1bbd4b159f46fb2fea721ef1e2ef43a3868312b9.
I reviewed the shared runner extraction and the three bridge adapters. The behavior-preserving contract checks out: capability discovery remains in each adapter, src/optional_script.py only runs an injected script path, and the subprocess argv/timeout/stdout/fail-open behavior matches the prior helpers. I also checked that the shared helper does not name audio-transcribe or any concrete skill path.
Reviewer-run checks:
python3 tests/audio-transcribe-skill.test.py— 22 passedpython3 scripts/lint-hermetic-bridge-tests.py --diff— passedbash scripts/agents-md-sync.sh --check— passedpython3 scripts/gen-src-map.py --check— passedgit diff origin/main...HEAD | bash scripts/review-checks.sh— passedPYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2495-pyc python3 -m py_compile src/optional_script.py src/discord-bridge.py src/slack-bridge.py src/telegram-bridge.py tests/audio-transcribe-skill.test.py— passed
No blocking findings. This is a clean adapter-edge extraction with the relevant contracts pinned.
Reviewed by Qingyun's Personal Codex.
Cold review by execution — exact head
|
| scenario | OLD | NEW | on_error |
|---|---|---|---|
| missing script | None |
None |
match |
| exit 0 with output | 'hello world' |
'hello world' |
match |
| exit 0, whitespace-only stdout | None |
None |
match |
| exit 0, no stdout | None |
None |
match |
| non-zero exit (with partial stdout) | None |
None |
match |
| exceeds timeout | None |
None |
match |
| writes only stderr | None |
None |
match |
Identical on every outcome, error-handler invocation included.
What else I ran
- This PR's suite —
tests/audio-transcribe-skill.test.py: 22 tests, OK. - The suites that already covered the changed files (not just the one added here) —
bridge-skill-path-resolution,bridge-timeout-guards,python39-union-annotations,
lint-hermetic-bridge-tests,regression-skill-hints-missing,bridge-skill-hints-injection:
all pass. - The removed function-local
import subprocess— checked the balance rather than assuming:
discord keeps 1 import for 7 remaining uses; slack and telegram drop to 0 imports / 0 uses. Correct. - No hardcoded absolute paths in added lines.
One follow-up, explicitly NOT a blocker on this PR
The PR centralizes execution but leaves discovery divergent, and the three adapters disagree:
discord-bridge Path(__file__).parent.parent
slack-bridge Path(os.path.realpath(__file__)).parent.parent
telegram-bridge Path(os.path.realpath(__file__)).parent.parent
I verified this is identical on origin/main, so it predates this PR — I'm raising it only because
this PR is the one that establishes the principle ("adapters must inject script or capability paths").
It matters because CLAUDE.md documents this precise divergence as a live failure mode: under an
app-bundled src/ symlink, the non-realpath form resolves into the bundle. Discord would then silently
find no skill and fall back to [File attached:] while the other two transcribe fine. Worth a separate
one-line PR.
Logistics
mergeStateStatus: BEHIND, and 10 open PRs currently touch docs/src-map.md (#2495, #2492, #2478,
#2469, #2419, #2341, #2334, #2197, #2068, #1414). Each bumps the N modules indexed line, so they
mutually conflict — expect a regen after whichever lands first, through no fault of this change.
Merge-readiness: not yet — needs Update branch and 2 approvals (currently 0). Nothing in the code
is holding it up. Reviewed as a bot; merges remain the owner's call.
|
Follow-up filed as #2499 — the discovery divergence I flagged above, with the one-line It is independent of this PR and touches only |
|
Cold review (Echo Act IV Mini). Reviewed head Verdict: I find no blocker. One gap I'd class as a strong nit, below. Not casting a formal approval — that's a merge-gate signal and not mine to record. Behaviour preservation — verified empirically, not by readingThe claim that matters for a Line-level, all three adapters preserve: the The interpreter question I went in expecting to failCentralizing on REVIEW.md pass
The one gap — nit, not blockerNo live round trip. If a maintainer wants it closed anyway, the cheap version is one real voice note through one bridge post-restart; the three adapters are now provably the same code path, so exercising one exercises all three. That property is itself a benefit of this PR. Small observations, no action needed
|
|
Cross-PR note: #2495 and #2499 conflict with each other, in both merge orders. Neither PR body mentions the other, so flagging it before one lands and surprises the second. Both edit Measured against So whichever lands second needs a manual resolution — this isn't order-avoidable. Suggested order: #2499 first. It's a one-line change; #2495 is +198/-71 across nine files. Resolving a 1-line fix into the big refactor is cheaper than the reverse, and the resolution is mechanical: #2495 keeps discovery at the adapter edge, so its Also worth recording, because it's easy to assume the opposite: #2499 looks like it fixes one bridge of three, but at discord was the last one out of line. #2499 finishes the set rather than fixing a third of it, and after #2495 centralizes execution the discovery line stays per-adapter — so keeping all three spelled the same is what stops this from drifting again. (Method note: my first probe reported CONFLICT for both orders including the first merge, which contradicted GitHub calling both MERGEABLE. That was a broken probe — a stale worktree made |
…2281 (#2500) * feat(review): write the preflight both guides have referenced since #2281 `REVIEW.md:18` and `CLAUDE.md:71` both tell reviewers the guide reaches them three ways: `review-preflight.py`, `scripts/review-checks.sh` in CI, and the managed GitHub-App reviewer. Only two of those exist. The script was never written. No commit in the repo`s history touches the path and no blob by that name appears on any ref, while the prose describing it has shipped since #2281 (2026-07-25, `0a36bf19`). That matters more than a stale doc line because of which channel is missing. The CI runner scans a diff during CI; the GitHub App reviews on GitHub. Neither surfaces anything to an agent or human reading a PR at a terminal -- the exact case CLAUDE.md points at ("when you review, `review-preflight.py` reads `REVIEW.md` and prints the criteria inline"). #2281`s stated goal was that following the guide be "structurally enforced, not dependent on an agent remembering to"; for interactive review it has been dependent on exactly that. An instruction naming a tool that does not exist is worse than no instruction: it reads as a completed step. Found by trying to run it before reviewing #2495 and getting `can't open file`. Behaviour: - guide resolution mirrors review-checks.sh exactly (`--guide` wins, else `<repo>/REVIEW.md`), so the two can never disagree on what is authoritative - prints the `## Lessons` section verbatim, summarizes the `checks:` block (counted 6 flag / 4 allow against the shipped guide) rather than dumping it - names the one-liner that actually scans a PR diff, and warns that a bare `review-checks.sh` compares the WORKING TREE and reports "empty diff" - a missing/unreadable guide exits 1. A preflight that prints nothing and succeeds is the failure mode this exists to prevent, so it is pinned by test. Adds no new dependency; stdlib only. `scripts/` is outside `gen-src-map.py`s scope (verified by regenerating: no diff), so no src-map churn. Stand: Echo Act IV Mini * fix(lint): list review-preflight as repo-tooling, not a workspace resolver CI caught the new script on PATTERN_REPO_WALK: its repo-root fallback is `Path(__file__).resolve().parent.parent`, the historic anti-pattern the lint exists to stop. The lint`s own note says how to resolve this: check-python39-compat.py is "listed explicitly rather than spelled `parents[1]` to dodge the regex -- evading the lint by rewording would defeat the point of having it." So this adds the script to ALLOWED with its justification rather than renaming the expression. review-preflight.py is the same category as gen-src-map.py and check-python39-compat.py: repo tooling that needs the REPO ROOT (to read REVIEW.md) and has NO workspace to resolve. Going through the wrapper would resolve the workspace, which is the wrong directory. It already prefers `git rev-parse --show-toplevel` and only walks from __file__ when git is unavailable. Verified both directions: --diff (what CI runs) : refused before, "no new anti-patterns" after full tree : 9 offenders on this branch AND on origin/main -- unchanged, and review-preflight is not among them Stand: Echo Act IV Mini * test(review-preflight): cover the two degraded-environment branches Coverage Gate measured 91.8% on the diff, naming lines 45-47 and 120-122. Both are error paths: 45-47 `git rev-parse` fails -> fall back to the script`s own location 120-122 the guide exists but cannot be read -> report and exit 1 These are exactly the paths that run when the environment is degraded, which is when a reviewer most needs the tool to behave predictably instead of tracebacking, so they are worth real cases rather than a coverage pragma. Three tests added: - git raising FileNotFoundError still resolves a root that contains REVIEW.md (asserts the fallback is USEFUL, not merely that it returns something) - git succeeding with empty stdout does not yield Path("") -- the `if out:` guard, which a naive fallback test would skip - an unreadable guide exits 1 with "cannot read" on stderr and nothing on stdout Verified the lines are actually reached, because `coverage` collects nothing on this host (its own positive control on a trivial script reports no-data-collected, so any local percentage would be fabricated). Used sys.settrace over the module during the suite: positive control - line 43 executed : True lines 45-47 : covered, none missing lines 120-122 : covered, none missing Suite 10/10. Claiming the branches are exercised; CI remains the authority on the percentage. Stand: Echo Act IV Mini
|
@cla-assistant check |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approved on exact refreshed head 26452aa2 after independent activated-path review.
The refresh has current main f51f2dc2 as its second parent and preserves the nine-file behavior-preserving extraction. Each bridge still discovers its adapter-specific skill path and retains its existing log wording; src/optional_script.py only runs an injected path and preserves interpreter, argv, 25s timeout, stripped-success, empty/nonzero, exception callback, and fail-open behavior.
Reviewer-run exact-head checks pass: audio-transcribe-skill 22/22, hermetic bridge lint, AGENTS sync, generated src-map, Python compilation, diff hygiene, and REVIEW.md hardcoded-path scan. The author also supplied a real post-restart Discord voice round trip through the shared runner.
Worst-case disruption is a shared-runner regression silently disabling transcription in all three adapters or blocking message delivery. Capability discovery remains at each adapter edge, failures return None, and the direct runner plus all-three-wrapper and real-module regressions cover that blast radius. LGTM, non-blocking.
Not merge-ready yet: refreshed clean-install and diff coverage must finish green, and the final merge guard must confirm two distinct qualifying exact-head approvals with no later blocker.
|
Correction to something I said in my review here, now that this has merged. I wrote: "It is independent of this PR and touches only That was wrong. #2499 went Rebased and resolved by keeping your delegation and applying only my one-line path fix on top: skill_script = Path(os.path.realpath(__file__)).parent.parent / "skills" / ...
return _run_optional_script_shared(skill_script, [local_path], timeout=25, on_error=...)Verified the result, not just that the rebase applied cleanly: the regression test passes on the rebased head, still fails exactly two checks (both discord-bridge) when the one line is reverted, and No action needed from you — flagging it because I stated the opposite on your PR and someone reading that thread later would be misled. Nice refactor; the differential-equivalence check I ran on it held up. |
What changed, and why?
Centralizes the provider-neutral subprocess behavior used when Discord, Slack,
and Telegram invoke the optional
audio-transcribeskill.src/optional_script.pynow owns the shared interpreter selection, argumentconstruction, timeout, output normalization, and fail-open behavior. Each
bridge still discovers the skill at its existing adapter-specific path and
retains its existing
_transcribe_via_skillAPI and log wording.CLAUDE.md/AGENTS.mdnow keep optional capability discovery at the adapteredge, while
docs/architecture-boundaries.mdrecords the dependency direction.Review first
src/optional_script.py_transcribe_via_skillin the three bridge adaptersTestOptionalScriptRunnerand the real-module coverage cases intests/audio-transcribe-skill.test.pyCLAUDE.mdUser behavior or bug proved
N/A — this is a behavior-preserving
tidy:refactor. The tests pin theexisting missing-script, success, non-zero exit, exception, 25-second timeout,
stdout trimming, interpreter, symlink-resolution, and fail-open contracts.
Feature/documentation consistency
No user-visible capability changed. The architecture boundary is documented in
docs/architecture-boundaries.md;CLAUDE.mdwas updated andAGENTS.mdregenerated.
docs/src-map.mdwas regenerated for the new source module.Before / after evidence
Parent
a9fb5bbc7bc7bf820c76e1a34494b1a1abf9e959contained the same subprocesscommand in three bridge-private helpers:
HEAD has one runner and three thin adapter delegations:
Tests
The first sandboxed full-suite attempt stopped at the repository's loopback
HTTP fixture with
PermissionError: [Errno 1] Operation not permitted; theidentical suite was rerun with local test-server permission and completed with
exit 0. The existing optional
secret-scanner.test.pyandvault-intercept.test.pyskips reported thatdetect_secretsis not installedin this interpreter.
Live post-restart round trip
Discord was restarted directly from HEAD with the normal runtime environment.
The owner sent a real Discord mobile voice message; the patched bridge
downloaded the
.ogg, invoked the shared runner, and wrote the transcript intoan owner-tier task:
The task recorded
source: discord,interaction_type: message,content_modalities: audio,media_form: attachment,access_tier: owner,the downloaded
.oggattachment metadata, and the expected transcript. Aresult then completed the return path:
An earlier Slack attempt reached its unchanged file-download guard but the
configured Slack token returned an HTML sign-in page for
audio_message.m4a(
files:readscope/reinstall is required), so no Slack transcript was claimed.The Discord pass exercises the shared runner end to end.
Stacking
N/A — standalone PR based directly on current
main. It does not depend on#2492.
Hardcoded-path check
No production host path or fallback was added. Skill discovery remains in each
adapter; only the discovered
Pathis passed into the generic runner.Edge cases checked
sys.executable, argument order, and 25-second timeout;skills/oraudio-transcribedependency;Migration, config, permissions, rollback, and risk
No migration, config, prompt, protocol, or permission change. Rollback is a
single commit revert. The main risk is adapter wiring drift; direct runner
contracts plus source-wiring and real-module tests cover that boundary.
Known gaps / follow-up
Slack voice-note validation on this host requires adding
files:readto theSlack app and reinstalling it. That configuration issue occurs before the code
changed here; Slack's downloader is byte-identical to
main. No code follow-upis bundled into this concern.