Repository navigation
test(discord-bridge+inline-tools): structural tests for task channel metadata (#1077) and workspace scan (#1081) - #1156
Conversation
VasiliyRad
left a comment
There was a problem hiding this comment.
Tests look solid — assertions are correct, escape handling is right (double-backslash for literal \n match), and the invariants are meaningful.
One blocker: build_log.md is in .gitignore (line 33) and should not be committed. It contains workspace-specific operational state (proactive loop passes, content drafts, push go-ahead lists). Please remove it from this PR.
Non-test change: src/health-check.py modification (adding _pid_script helper for multi-PID source-path enrichment) is clean and correct — ps -o command= extraction with .py suffix filtering and graceful ? fallback.
The 24 structural tests across both files correctly verify the #1077 and #1081 invariants. False-positive risk is inherent to grep-style tests but acceptable and documented. LGTM once build_log.md is dropped.
VasiliyRad
left a comment
There was a problem hiding this comment.
Tests pass (19/19 discord-bridge-task-channel-metadata, 5/5 health-check-bridge-multi-pid). Quality looks good — structural source-grep pattern is appropriate here.
Two bundling issues to flag:
-
src/health-check.pyis a production fix, not a test. Commitd9a384c(the multi-pid path enrichment) is the implementation;health-check-bridge-multi-pid.test.pytests it. Both are welcome, but the PR title says 'structural tests for #1077 and #1081' — this is a third change (#1084-equivalent) that isn't mentioned in the title or PR body table. Please add a '## health-check multi-pid path enrichment' section to the PR body so reviewers know they're also reviewing a source change, not just tests. -
build_log.mddoesn't belong in test PRs. The proactive loop's pass log is runtime state, not source. It can be committed to main directly or via a chore PR, but it dilutes the diff here and makes future bisects harder. Please strip it from this branch before merge.
The src/health-check.py change itself is correct and safe — the inner _pid_script helper is well-guarded with try/except and a 3s timeout.
…metadata + workspace scan 19 tests covering two merged PRs: PR sonichi#1077 — channel_name + guild_name in discord task files (12 tests): - channel_name and guild_name fields present in task_file.write_text() - channel_name appears AFTER channel_id (additive, no breaking change) - DM fallback: getattr(message.channel, "name", None) or "DM" - Guild fallback: message.guild.name if message.guild else "DM" - Newline sanitization on both fields (injection guard from qingyun review) - Both fields in same task-write block - Comment explaining human-readable purpose PR sonichi#1081 — inline-tool loader scans $SUTANDO_WORKSPACE/skills/ (7 tests): - loadSkillManifestTools() includes WORKSPACE_DIR/skills in scan paths - loadCoreDocumentedSkills() includes WORKSPACE_DIR/skills in scan paths - REPO_ROOT/skills and WORKSPACE_DIR/skills both present - Scan order: REPO_ROOT/skills < WORKSPACE_DIR/skills (repo → workspace → memory) - last-write-wins convention documented Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
f64883f to
07e1b5f
Compare
|
Branch rebased onto origin/main — bundled junk (build_log.md, health-check.py, health-check-bridge-multi-pid.test.py) stripped. Diff is now clean: only the intended test file(s). |
What
19 structural tests covering two merged PRs with no existing test coverage.
Source files tested:
src/discord-bridge.py— task-file channel/guild metadata fieldssrc/inline-tools.ts— workspace skills scan pathPR #1077 —
channel_name+guild_namein task files (12 tests)Background: Task files previously only carried numeric
channel_id. After a real misroute incident (core agent had to guess channel identity by grepping numeric IDs against a memory file), two additive human-readable fields were added alongsidechannel_id.channel_namefield present intask_file.write_text()guild_namefield present intask_file.write_text()channel_nameappears AFTERchannel_id(additive, no breaking change)getattr(message.channel, "name", None) or "DM"message.guild.name if message.guild else "DM".replace("\n", " ")(injection guard)PR #1081 — inline-tool loader scans
$SUTANDO_WORKSPACE/skills/(7 tests)Background: Before this PR, users couldn't park personal inline-tool skills in their workspace without forking the repo.
$SUTANDO_WORKSPACE/skills/was added as the second scan path in bothloadSkillManifestTools()andloadCoreDocumentedSkills(), with scan order: repo → workspace → memory-dir (last-write-wins).loadSkillManifestTools()includesWORKSPACE_DIR/skillsloadCoreDocumentedSkills()includesWORKSPACE_DIR/skillsREPO_ROOT/skillsandWORKSPACE_DIR/skillspresentREPO_ROOT<WORKSPACE_DIR(repo → workspace → memory)🤖 Generated with Claude Code