fix: address PR review feedback + fix flaky countdown toast test (#57)
* fix(todo-continuation-enforcer): make countdown toast test deterministic
The test at should show countdown toast updates called
fakeTimers.restore() to use real timers, then waited 2500ms of
wall-clock and asserted that at least 2 countdown toasts had been
emitted. On loaded CI runners the setInterval tick for the second
toast was missing the 2500ms window, producing a 1-of-2 outcome that
failed the toBeGreaterThanOrEqual(2) assertion and surfaced as a
flaky test on PR #54.
Replace the real-timer path with fakeTimers.advanceBy(2500, true),
matching the pattern used by every other countdown assertion in this
suite. With fake timers the interval fires deterministically and the
test passes on the same CI container that previously flaked.
Verified with 15 consecutive local runs of the specific test, a full
bun test run on src/hooks/todo-continuation-enforcer/, and a full
bun run script/run-ci-tests.ts across the whole repo.
* docs(sync-methodology): fix factual errors flagged by PR #52 reviews
Three corrections called out on the merged PR #52:
1. Latest upstream stable tag is v3.17.4, not v3.18.0. Verified via
gh api repos/code-yeongyu/oh-my-openagent/releases/latest.
2. publish.yml does not create a dev to master merge commit. It runs
git checkout master && git reset --hard v{version} && git push -f
origin master, so master is force-moved to the release tag. Updated
the step description to match the workflow.
3. Removed the guidance that told maintainers to leave
upstream-version.txt at its current value after skipping a release.
The upstream-tag-watcher cron reads that file and will keep
dispatching the analyzer every 15 minutes as long as the tracker
lags the upstream tag, which turns a single skip into an API-spend
loop. Document the two safe options instead: advance the tracker or
record the tag in .upstream-watcher-seen-tag.
Also replaced em dashes with ASCII double-hyphens throughout the two
docs per AGENTS.md line 129.
* fix(config): reject whitespace-only strings in disabled_skills + disabled_commands
Kilo Code and Devin flagged on PR #53 that z.string().min(1) accepts
whitespace-only values like ' ' or '\t\n' because length 1 is one
character of whitespace, not non-blank. Silent whitespace entries
would pass config validation and never match any real skill/command
name at the Set lookup layer, so typos or stale entries could
disappear without feedback.
Replace the inline min(1) constraint with a shared
NonBlankStringSchema that trims first and requires non-empty, and
add regression tests covering spaces, tabs, and newlines.
Also replaced em dashes with ASCII hyphens in the JSDoc blocks per
AGENTS.md line 129.
* docs(skill-loader): correct LoadedSkill.userInvocable JSDoc
Copilot called out on PR #54 that the LoadedSkill.userInvocable
docstring claimed the field held a resolved visibility value derived
by XOR with skills.hide_nested_by_default, but the actual loader at
loaded-skill-from-path.ts and async-loader.ts stores the raw
frontmatter value unchanged. The resolution happens later in
skill-definition-record.ts during command registration.
Rewrite the comment to describe what is actually stored (raw value,
undefined when frontmatter field is absent) and point at where the
effective visibility is computed.
* fix(skill-loader): recurse into child directories in async path, remove dead imports
Two related fixes to src/features/opencode-skill-loader/async-loader.ts
pointed out on PRs #54 and #55:
1. Dead imports (basename, isMarkdownFile) remained after the Branch C
plain-.md removal; delete them for consistency with the parallel
cleanup in skill-directory-loader.ts.
2. discoverSkillsInDirAsync returned early as soon as it found a
SKILL.md or {dirName}.md entrypoint, which meant nested skills
inside a directory that had its own SKILL.md were never discovered
via the blocking-worker path (discoverAllSkillsBlocking). The serial
loader at skill-directory-loader.ts:74-88 always recurses. Align
the two paths: collect the entrypoint skill when found, then still
descend into subdirectories up to maxDepth and append their skills.
Verified with bun test src/features/opencode-skill-loader/ (113 pass)
and a full bun run script/run-ci-tests.ts (0 failures).
* style(skill-loader-tests): replace em dashes with ASCII hyphens per AGENTS.md
Devin and Kilo Code flagged on PRs #55 and #54 that my test files
introduced em dashes in BDD 'given / when / then' comments and in
describe-block names. AGENTS.md line 129 bans em dashes and en dashes
in generated content.
Swap every em dash (U+2014) for an ASCII hyphen across both affected
test files. No behavior change, just punctuation conformance. V
Vacbo committed
b02b4bbe7f15f5b1488dbe7d458553ce4e931e5c
Parent: 4b4ccae
Committed by GitHub <noreply@github.com>
on 4/21/2026, 11:01:41 PM