Repository navigation
feat(bridges): inject skill instructions into owner task files to survive compaction - #1467
Conversation
…RIPT_DIR
When scripts/sync-memory.sh is invoked through a symlinked scripts/
directory (e.g. workspace/scripts → repo/scripts), \$0 resolves to the
symlink path and plain pwd gives the symlink dir — \$SCRIPT_PARENT then
points to the symlink's parent instead of the repo root, breaking the
sutando-config.sh helper lookup at L131.
Fix: replace \$0 with \${BASH_SOURCE[0]} and add pwd -P so the directory
symlink is followed back to the physical path. Mirrors the existing Python
pattern (Path(__file__).resolve().parent.parent) and the bridges' own fix.
Test: add case_script_dir_resolves_through_symlink to sync-memory migration
test suite — creates a symlinked scripts/ directory and asserts pwd -P
returns the real dir, not the link dir.
Closes sonichi#1436.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…vive compaction
All three bridges (Slack, Discord, Telegram) now append an
===SKILL INSTRUCTIONS=== block to every owner-tier task file at
write-time. The block contains the exact CLI commands to:
1. Notify the user first via the task-progress skill
2. Transcribe audio (if a voice note is attached) via audio-transcribe
3. Write the result to results/{task_id}.txt
Before this change, both skills' SKILL.md content was only available if
explicitly read during the session — after conversation compaction the
agent had no instructions and failed silently (no notify-before-work,
no transcription, no result). Injecting the commands directly into the
task file makes each task self-describing, independent of context state.
Audio detection per bridge:
- Slack: checks attachment_lines for known audio extensions
- Discord: checks attachment_note for .m4a/.mp3/.ogg/.opus/.wav/.webm/.aac
- Telegram: checks for .ogg/.oga (Telegram voice notes) in addition to above
Telegram uses --chat-id (not --channel-id) to match the notify.py API.
Non-owner tasks get no skill hints block (access-control preserved).
Tests: tests/bridge-skill-hints-injection.test.py — 19/19 structural
tests covering all three bridges.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Before this fix, the ===SKILL INSTRUCTIONS=== block was always injected for owner tasks, even on nodes where audio-transcribe or task-progress are not installed. That would produce confusing "file not found" errors when the agent followed the injected commands. Now each bridge checks for the existence of: ~/.claude/skills/task-progress/scripts/notify.py ~/.claude/skills/audio-transcribe/scripts/transcribe.py And only injects the corresponding instruction lines if the file is present. If neither skill is installed, no hints block is written at all. This makes the feature safe on any Sutando instance regardless of which skills are installed. Tests updated: 22/22 passing (3 new existence-guard checks added). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
LGTM — solid approach to the context compaction problem. Making each task file self-describing is the right fix; it's portable across sessions without relying on memory files being loaded. A few observations: Strengths:
Minor nit: The block header CI: only CLA + recheck (same as #1453 — this PR predates the full CI matrix). Tests in Ready for Chi's review. |
The skill-hints injection block added in feat/bridge-skill-hints-injection
grew _write_task by ~1.4k chars (existence-guarded notify + transcribe
injection). The regex budget {0,6000} was no longer sufficient to reach
the next \n\ndef boundary; the test reported `_write_task not found`.
Bumped to 10000 with an updated comment tracing the bump history.
The guard that actually matters (_write_task access control) is in the
first ~200 chars of the body and is unaffected by the limit change.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…g in _with_access_json
The test _with_access_json patched dm.ACCESS_JSON but not
discord_config.load_config, so resolve_owner_id() step 2 read the real
$SUTANDO_WORKSPACE/state/discord-config.json which has `"owner": "<real-id>"`.
That short-circuited the tierMap resolution the test was exercising, causing
test_tier_map_resolution_skips_bot_lookup to fail with:
assert open_calls[0]["body"] == {"recipient_id": "tier-owner-id"}
Fix: also patch dm.discord_config.load_config → lambda: {} inside the
context manager, restoring the original on teardown. With an empty config
the resolution chain falls through to the access_data fixture's tierMap
as intended.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…1467 thread + Lucy's Maddy report) Part 3 of the v0.3.0 docs sweep (PR #1472). Adds two substantive sections to `docs/workspace-sync.md`: **1. Migration risks + pre-flight checklist** — distilled from: - sonichi's "Migration" thread on 2026-06-05 (Mini + Pro + Sutando risk analyses) - Lucy's Maddy-host migration run report 2026-06-06 #design Covers: - Pre-migration checks per host (vault URL set, $SUTANDO_WORKSPACE removed from shell rc AND .env, large media excluded from sync, per-host runtime files excluded) - Serial migration order across hosts (avoids the D/F branch collision race pre-#1463) - Per-host verify step including the heartbeat .alive cleanliness check Mini surfaced - Known issues with workarounds (Claude memory not auto-moved, sync-workspace plain run silent-commits, $SUTANDO_WORKSPACE re-migrate loop) **2. Troubleshooting addition** — Lucy's #6 finding: gh-token macOS-Keychain-bound, fails with `-25308` over SSH / plain cron. Documents the launchd-plist-scoped-to-GUI-session workaround. The known-issues block calls out workarounds for the 3 substantive migration UX bugs Lucy hit (memory-not-moved, plain-run commits all, re-migrate loop). The actual fixes will land as follow-up code PRs (tracked separately); the doc is honest about the current state so users hitting these recognize the symptoms. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…1467 thread + Lucy's Maddy report) Part 3 of the v0.3.0 docs sweep (PR #1472). Adds two substantive sections to `docs/workspace-sync.md`: **1. Migration risks + pre-flight checklist** — distilled from: - sonichi's "Migration" thread on 2026-06-05 (Mini + Pro + Sutando risk analyses) - Lucy's Maddy-host migration run report 2026-06-06 #design Covers: - Pre-migration checks per host (vault URL set, $SUTANDO_WORKSPACE removed from shell rc AND .env, large media excluded from sync, per-host runtime files excluded) - Serial migration order across hosts (avoids the D/F branch collision race pre-#1463) - Per-host verify step including the heartbeat .alive cleanliness check Mini surfaced - Known issues with workarounds (Claude memory not auto-moved, sync-workspace plain run silent-commits, $SUTANDO_WORKSPACE re-migrate loop) **2. Troubleshooting addition** — Lucy's #6 finding: gh-token macOS-Keychain-bound, fails with `-25308` over SSH / plain cron. Documents the launchd-plist-scoped-to-GUI-session workaround. The known-issues block calls out workarounds for the 3 substantive migration UX bugs Lucy hit (memory-not-moved, plain-run commits all, re-migrate loop). The actual fixes will land as follow-up code PRs (tracked separately); the doc is honest about the current state so users hitting these recognize the symptoms. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sonichi
left a comment
There was a problem hiding this comment.
LGTM. Self-describing task files is the right fix for the compaction silent-failure mode — each task carries exactly the CLI it needs, no context dependency. Conditional on skill existence is clean. BASH_SOURCE[0] + pwd -P in sync-memory.sh is a strict improvement. Tests cover the injection logic structurally. Ready. Stand: Echo Act IV Pro
…rator checklist + workspace-sync node_modules Per Chi's PR #1472 comments (#design 2026-06-06): **CHANGELOG.md v0.3.0 additions:** - Fixed → Migration subsection: Lucy's Maddy v0.8 5-bug sweep (#1475 #1478 #1482 #1483 #1484) - Fixed → Cross-platform: #1476 (uname -s branch for preflight byte count) - Fixed → Security: #1452 #1462 #1466 (execSync → execFileSync sweep) - Added: #1467 (bridges inject skill instructions into owner task files) - Workspace contract rollup → M1: also reference #1473 (pre-flight scan + per-file progress reporter) **KNOWN_ISSUES.md additions** (from migration-thread digest 1512359988834730014): - Pre-migration operator checklist (7 items) — vault URL, remove shell-rc env var, grep .git/info/exclude, confirm wsId, stagger hosts, restart services, post-migration sanity check - Identity-collision rule — exactly one host at a time can hold a Discord token (also applies to Twilio + other per-identity creds) **docs/workspace-sync.md** pre-flight checklist: - Added `node_modules/` row per Chi's PR #1472 comment (covers vault-sync side; #1482 already handles the backup-side via `_BACKUP_DEFAULT_EXCLUDES`) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
left a comment
There was a problem hiding this comment.
Verdict: solid, well-scoped, and tested — this is the right fix for a real post-compaction failure. The premise is exactly right: once a conversation compacts, the agent loses SKILL.md context, so a voice note arriving via a bridge has no idea it should notify first or transcribe the attachment. Baking the protocol into the task file itself — the one artifact that survives into the next context window — is the correct layer. It mirrors the existing ===SUTANDO SYSTEM INSTRUCTIONS=== in-band-enforcement pattern for non-owner tiers, so the ===SKILL INSTRUCTIONS=== owner analogue fits the established grain of the codebase.
Things I like:
-
Node-local existence guard is the standout decision. Gating injection on
_notify_py.exists() or _transcribe_py.exists()(discord-bridge.py:2842,slack-bridge.py:~448,telegram-bridge.py:~545) means a node without those skills installed never emits instructions pointing at scripts that aren't there. That's the difference between a hint and a landmine — a danglingpython3 ~/.claude/skills/...would have the agent chase a non-existent path post-compaction. Correctly fail-closed. -
Owner-tier scoping on Slack/Discord.
access_tier == "owner" and (...)keeps the injected block off non-owner task files, which already carry their own sandboxed===SUTANDO SYSTEM INSTRUCTIONS===. No risk of the two instruction blocks colliding in a team/other task. The asymmetry with Telegram (no tier guard) reads as intentional — Telegram is single-owner by construction (default_priority_for_source("telegram", "owner")), so there's no non-owner case to exclude. -
Audio-aware step ordering. The
has_audiobranch (Discord/Telegram) andattachment_linesbranch (Slack) escalate the notify copy from "if task takes >60s" to the unconditional "Got your voice message, give me a moment," then add the transcribe step. That's the right UX: a voice note should always get an instant ack because the user can't see you working, whereas a text task only needs the ack when it'll be slow. -
The
sync-memory.shrobustness fix is genuinely load-bearing, not drive-by.${BASH_SOURCE[0]}+pwd -P(scripts/sync-memory.sh:40) fixes script-dir resolution when the script is invoked sourced or through a symlinkedscripts/dir (workspace→repo) — and it ships withsync-memory-migration.test.shproving the symlink case. Since sync-memory is what distributes these very skills across nodes, hardening its self-location is on-theme for the PR rather than an unrelated refactor. -
Test isolation fix in
dm-result-send-dm.test.pyis a real catch. Patchingdm.discord_config.load_config → {}stops a realstate/discord-config.jsonownerfield from bleeding intoresolve_owner_id()and overriding the fixture's tierMap/allowFrom. That's the kind of hidden cross-test coupling that produces "passes on my machine" flakes; good to see it fixed alongside.
Non-blocking observations:
- Attachment-path parsing is string-fragile.
attachment_note.split("[File attached: ")[-1].rstrip("]").split("\n")[0](Discord/Telegram) andap.replace("[File attached: ", "").rstrip("]")(Slack) will truncate a filename containing]. Vanishingly rare for real voice-note filenames, but if the attachment-note format ever gains a structured form, these three call sites would all want to read from it instead of re-parsing the rendered string. Worth a# TODO: parse from structured attachment metadataso the duplication is findable. - Slack multi-attachment loop emits the "Update notify message to:" hint once per attachment. In
_write_task, that advisory line lands inside thefor ap in attachment_linesloop, so two voice files would print it twice. Cosmetic — multi-voice-note messages are an edge case — but a single emit before the loop would read cleaner. - The tests are structural (source-grep), not behavioral. They assert the bridges contain the right strings and that
skill_hintsis threaded intowrite_text, which is a fine wiring-guard, but they won't catch a logic regression in the path-parsing or the audio-extension detection. A tiny behavioral test that feeds a fake message through_write_taskand asserts the produced task-file body would close that gap later; not a merge blocker.
Cross-PR linkage: this pairs naturally with #1453 (audio-transcription + task-progress skills) — #1467 is the bridge-side wiring that makes those skills actually fire post-compaction, so the two are a matched set. If #1453 isn't merged first, the existence guards here simply no-op on nodes without the skills, so there's no hard ordering dependency — but they're most valuable landed together.
Tests: the new bridge-skill-hints-injection.test.py (24 checks across all three bridges) + sync-memory-migration.test.sh + the dm-result/slack-bridge-access isolation fixes give good wiring coverage. Structural-only as noted, but appropriate for the change shape.
Ready to merge once you're comfortable with the structural-test scope — the feature is correct, fail-closed, and on-pattern. Nothing here blocks.
— Lucy (Mac Studio bot)
|
Cold review — Sutando (john-the-dev node). feat(bridges): inject skill instructions into owner task files — LGTM.
Two approvals (owner + Lucy). Depends on #1453 (audio-transcribe + task-progress skills) but that's flagged in the PR. Ready to merge once #1453 lands. |
john-the-dev
left a comment
There was a problem hiding this comment.
Upgrading to formal APPROVE. Reasoning stands from the inline cold review: correct layer (task-file is the compaction-safe artifact), mirrors established ===SUTANDO SYSTEM INSTRUCTIONS=== pattern, existence guard ensures hosts without skills are unaffected, audio-extension detection consistent across all three bridges. 30/30 tests pass. The soft ordering dependency on #1453 is flagged and the existence guard handles missing skills gracefully — no hard blocker. Ready.
liususan091219
left a comment
There was a problem hiding this comment.
Thanks @bassilkhilo-ag2! This reads as a fix for a real failure mode rather than a new feature — which is great, but two things are missing before it can land, per the project's issue-first rule:
- A tracking issue describing the bug / pain this addresses (no
closes #Nhere, and the PR body doesn't link one). - A repro script that reproduces the original failure (pre-fix). The current tests verify the new behavior works, but they don't demonstrate that the pain actually reproduces — which is what justifies the change.
Could you open a short issue (symptom + when it happens) and add a repro that fails on main and passes with this PR? Once the issue + repro are linked, happy to re-review. Thanks!
|
Opened tracking issue #1568 — "bug(bridges): voice-note tasks silently fail after context compaction — skill instructions lost" — to satisfy the issue-first rule Lucy flagged. The issue documents the symptom (silent failure on post-compaction voice notes), the root cause (skill context rolls off the compaction window), and the affected files. PR #1467 is the proposed fix. For Lucy's second requirement (a repro that fails on Linking: |
|
Tracking issue filed: #1568 (closes the first of Lucy's two blockers). @bassilkhilo-ag2 — to unblock the
Once those two are in, this should be reviewable again. |
|
Hi @bassilkhilo-ag2 — issue #1568 was filed today documenting this exact bug (symptom, root cause, repro steps, affected code). It covers @liususan091219's first requirement. For requirement #2 (repro script): the failure mode is inherently session-state — it only manifests after context compaction, which requires ~10+ minutes of turns in a live session. A fully automated pre-fix repro script would need to mock conversation compaction. One option is a test fixture that exercises the bridge task-write code path without the Could you add |
…action (sonichi#1568) Demonstrates the pre-fix failure mode: task files written on main contain no ===SKILL INSTRUCTIONS=== block, leaving an agent with no guidance after context compaction. The last 3 checks fail on main (injection code absent) and pass on this branch. Satisfies Lucy's two requirements for unblocking CHANGES_REQUESTED: 1. Tracking issue: Closes sonichi#1568 2. Repro: tests/regression-skill-hints-missing.test.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressed Lucy's two CHANGES_REQUESTED blockers:
9/9 pass locally. @liususan091219 please re-review. |
overlap Two conflicts, both spots sonichi#1084 (secret-vault) and this PR touched: - src/telegram-bridge.py: keep BOTH blocks — vault interception first (it sanitizes `text` before anything consumes it), then the skill-hints injection (reads attachments, not text). - tests/slack-bridge-access.test.py: _write_task body-capture budget — both sides bumped independently (8000 vault / 10000 skill-hints); merged function carries both additions (measured 8058 chars), set 12000. Full Python suite passes locally post-merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
liususan091219
left a comment
There was a problem hiding this comment.
Re-review after the 06-08 CHANGES_REQUESTED: (1) tracking issue — satisfied (#1568, linked via Closes). (2) repro — the structural regression test (tests/regression-skill-hints-missing.test.py: pre/post-fix task-file formats + source-presence check, with the manual 4-step reproduction documented in its docstring) is accepted as the reasonable form here: a behavioral repro would require triggering real context compaction in CI (live session, near-context-limit token burn, nondeterministic timing), which is disproportionate for a constructive fix whose correctness rests on the injection being present — task files are the artifact compaction never touches. Conflicts with main (post-#1084) were resolved in 918c8bf (vault interception ordered before skill-hints injection; merged test budget); CI green. Approving — leaving merge timing to the maintainers.
|
@cla-assistant check |
…_DIR (#1525-class) (#1588) PR #1467's skill-instruction injection hardcoded ~/.claude/skills/ in all three bridges (guards + injected command text, 6 sites), bypassing CLAUDE_CONFIG_DIR — the exact class #1525 cleaned up. On nodes with a relocated config dir the guards check a tree the config doesn't point to. - Per-bridge _claude_config local (same pattern as #1525 / bot2bot-post): Path(os.environ.get("CLAUDE_CONFIG_DIR", ~/.claude)) - Injected command text now carries the resolved absolute path — a self-describing task file shouldn't depend on the agent's shell env. - Paths kept as single slash-joined segments so source stays greppable (tests/bridge-skill-hints-injection.test.py passes unmodified). - Also updated the sandbox-instructions docs string in discord-bridge. Full Python suite green locally. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rim summary, server check-in as fallback (#1427) Susan 2026-06-09 (refining the check-in): "首先问core强制要求2分钟回答完, 如果不能完成后面继续做,2min需要答一个summary" — layer it: core answers first, the server prompt is only the fallback. - Task files now carry a deadline_protocol line: core MUST write a 1-3 sentence interim summary to results/<id>.partial.txt within 2 minutes (understood-task / findings-so-far / ETA), then keep working toward the full result. Per-task instruction in the file itself — the #1467 inject-into-task-file pattern, survives core compaction. - delegateTask's poll relays the partial the moment it lands (model reports it + asks keep-waiting vs Discord-later) and suppresses the fallback check-in; the server-timer prompt now fires ONLY when core stays silent past TASK_CHECKIN_MS. Final result cleans up a lingering partial. tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Problem
After conversation compaction, the agent loses SKILL.md content from context. When a Slack/Discord/Telegram voice note arrived, the agent had no instructions to:
task-progressto notify before doing workaudio-transcribeto transcribe the audioThis caused silent failures: no notification, no transcription, no result.
Closes #1568
Solution
All three bridges now append an
===SKILL INSTRUCTIONS===block to every owner-tier task file at write-time. The block embeds the exact CLI commands the agent needs, making each task file self-describing regardless of context state.Example (voice note via Slack):
Details
.m4a,.mp3,.ogg,.oga,.opus,.wav,.webm,.aac) and tailors the instructions accordingly--chat-id(not--channel-id) to matchnotify.py's APITests
tests/bridge-skill-hints-injection.test.py— structural tests covering all three bridgestests/regression-skill-hints-missing.test.py— regression test demonstrating the pre-fix failure mode: pre-fix task files have no===SKILL INSTRUCTIONS===block (agent gets no guidance), post-fix task files include it. The last 3 checks explicitly fail onmain(injection code absent) and pass on this PR.