fix(skill-nudge): log swallowed errors in debug mode

[stuart902]

What does this PR do?

Threads the existing global --debug flag into the skill-nudge pre-action context so the two remaining swallowed error paths in src/lib/skill-nudge.ts are diagnosable when debug output is requested.

Specifically:

• Profile lookup failures now emit a debug-only stderr diagnostic instead of disappearing silently.
• Managed-section target read failures (for example, an unreadable AGENTS.md) emit a debug-only stderr diagnostic while continuing to treat that target as absent.
• Non-debug behavior remains unchanged.
• JSON output remains unpolluted.
• Diagnostic delivery is itself best-effort, so a failing stderr/diagnostic sink cannot break the command.
• Regression tests cover both swallowed-error paths and the failure-isolation behavior.

This follows the debug-emission pattern established in #223.

Related issue

Closes #285

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that changes existing behavior)
• Documentation only
• Build / CI / chore

Checklist

• PR targets the main branch.
• Commits follow Conventional Commits
  (feat(...), fix(...), docs(...), …).
• npm run lint and npm run format:check pass.
• npm run typecheck passes.
• npm test passes and coverage stays at or above the 80% gate.
• New behavior is covered by unit tests (mock-based; no network or
  credentials required).
• No secrets, API keys, internal endpoints, or personal data are included.
• User-facing changes are reflected in README.md / DOCUMENTATION.md where
  relevant.

Notes for reviewers

The implementation is intentionally narrow and follows #223's existing pattern:
capture the swallowed error, honor the existing --debug flag, emit to stderr,
and preserve normal output and exit behavior.

The managed-target read failure occurs inside isVerifySkillInstalled(), so this
PR adds an optional best-effort diagnostic callback rather than changing the
function's return behavior. The callback is isolated with its own error guard so
diagnostic failures cannot affect command execution.

The branch changes only:

• src/index.ts
• src/lib/skill-nudge.ts
• src/lib/skill-nudge.test.ts

Full lint, format, typecheck, unit-test, and coverage verification is left to the
repository CI on this PR.

Summary by CodeRabbit

• New Features

  • Added optional debug output for skill-nudge processing, including skipped nudges and unreadable targets.
  • Added diagnostic reporting for errors encountered while checking skill installation status.
  • Skill nudges now identify specific callers missing required skills.

• Bug Fixes

  • Improved resilience when profile lookups, managed targets, warning output, or debug output fail.
  • Preserved warning messages even when diagnostic callbacks encounter errors.
  • JSON-mode output remains free of diagnostic noise, including when debug mode is enabled.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 816dbd78-b26c-4d5c-be67-84f6f2a48ed4

📥 Commits

Reviewing files that changed from the base of the PR and between c2d5656 and 86f9520.

📒 Files selected for processing (1)

• src/lib/skill-nudge.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/lib/skill-nudge.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

Walkthrough

The skill-nudge pre-action now receives the global --debug flag. Debug mode reports suppressed profile lookup and managed-target read failures to stderr. Caller-specific skill detection and required-target filtering are covered by tests.

Changes

Skill-nudge handling

Layer / File(s)	Summary
Presence and debug contracts src/lib/skill-nudge.ts	Skill presence checks report unreadable targets through optional diagnostics. The nudge context supports debug output.
Caller-aware nudge flow src/index.ts, src/lib/skill-nudge.ts	The pre-action hook forwards the debug setting. Nudge handling checks detected callers independently and emits guarded diagnostics.
Nudge behavior regression coverage src/lib/skill-nudge.test.ts	Tests cover read failures, debug behavior, JSON silence, warning preservation, byte-identical non-debug output, and failing diagnostic sinks.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Low

Sequence Diagram(s)

sequenceDiagram
  participant PreActionHook
  participant maybeEmitSkillNudge
  participant detectCallerFromEnv
  participant isVerifySkillInstalled
  participant stderr
  PreActionHook->>maybeEmitSkillNudge: pass debug setting
  maybeEmitSkillNudge->>detectCallerFromEnv: detect calling agents
  detectCallerFromEnv-->>maybeEmitSkillNudge: return callers
  maybeEmitSkillNudge->>isVerifySkillInstalled: check each caller target
  isVerifySkillInstalled-->>maybeEmitSkillNudge: return presence or report read error
  maybeEmitSkillNudge->>stderr: write warning or debug diagnostic

Loading

Merge Risk: ⚪ Minimal · up to 86f95

Debug diagnostics remain isolated from normal command output and do not interrupt execution; no merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The change also adds per-caller skill detection through detectCallerFromEnv, the requiredTargets filter, and caller-specific warning text. Issue #285 scopes this work to debug diagnostics for two …	Remove the per-caller detection, requiredTargets, and caller-specific warning changes from this pull request, or link and scope that behavior under a separate issue and pull request.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely describes the main change: adding debug-mode diagnostics for previously swallowed skill-nudge errors.
Linked Issues check	✅ Passed	Issue #285 coding requirements are covered. src/index.ts passes globals.debug into maybeEmitSkillNudge. src/lib/skill-nudge.ts emits debug diagnostics for profile lookup failures and unreadabl…
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files.

Full details: Out of Scope Changes check

Explanation

The change also adds per-caller skill detection through detectCallerFromEnv, the requiredTargets filter, and caller-specific warning text. Issue #285 scopes this work to debug diagnostics for two swallowed errors, debug-flag propagation, and regression coverage. The per-caller detection changes existing nudge behavior but does not implement those objectives. This is a separate behavior change without a demonstrated link to #285.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[zeshi-du]

Sorry — this has been open 26 days with no human reply, and on top of that your CI had never actually run. Fork PRs here sit at action_required until a maintainer releases them, and nobody did. I've released it; the results are below.

On the substance: I'm happy with this and intend to merge it. Two things I'd flagged internally when it was opened are both resolved:

1. The internal rewrite of skill-nudge.ts that this would have collided with landed on 2026-09-03, and your 09-10 rebase puts you cleanly on top of it — I re-checked today against main at v0.11.0 and there are zero conflicts.
2. I had a note about writing raw exception text to stderr instead of reusing the redaction path we use elsewhere. Having read the implementation, that concern doesn't apply here: the errors emitDebug surfaces come from existsSync/readFileSync on local agent-config paths, and the path is deliberately part of the diagnostic. The redaction path exists for presigned artifact URLs, which never reach this code.

The shape is right too — gating onReadError behind --debug, and wrapping the callback so a broken stderr sink can't turn a best-effort probe into a failure.

One thing blocks the merge, and it's cosmetic. npm run format:check fails on src/lib/skill-nudge.ts. Prettier wants the emitDebug call collapsed onto one line:

       onReadError: ctx.debug
         ? (path: string, error: unknown) =>
-            emitDebug(
-              write,
-              `skill nudge could not read ${path}; treating target as absent`,
-              error,
-            )
+            emitDebug(write, `skill nudge could not read ${path}; treating target as absent`, error)
         : undefined,

npm run format and a push will do it. Say the word if you'd rather I pushed that one commit to your branch instead — after 26 days it seems unfair to make you come back for a line break.

Everything else is green: typecheck, unit tests on Node 20/22 and Windows, build, coverage, E2E, secret scan, eslint-security. The one remaining red check, Dependency Review, is broken repo-wide for reasons unrelated to any PR — a repo setting on our side that I'm getting fixed. Ignore it.

[stuart902]

Thanks! I pushed the Prettier-only formatting fix in 86f9520.

@zeshi-du The new push appears to have put the fork workflows back into action_required, so CI may need to be released once more.

[zeshi-du]

Approved.

CI is now green on everything that reflects this change: typecheck, Node 20 / Node 22 / Windows unit tests, build, coverage, local E2E, gitleaks, ESLint security, and Lint & Format after your style(skill-nudge) commit — thank you for turning that around the same day.

The one red mark, Dependency Review, is ours, not yours. Our organisation has the dependency graph disabled, so that action fails on every pull request in this repo regardless of content. I've opened a fix internally so it skips with a warning instead of failing when the graph is unavailable. Please ignore it.

One note on timing: a release is mid-flight right now, and merging to main during the snapshot window would break our mirror's integrity check. So this sits approved for a day or so and then goes in — the delay is purely release mechanics, not a further review. I'll merge it myself and comment here when it lands.

For the record, since this PR waited far too long for a first human response: the two objections raised in the earlier internal review are both resolved. The file is no longer being concurrently rewritten, and the concern about bare stderr not reusing the redaction path doesn't apply here — emitDebug reports fs errors against local agent-config paths, and printing the path is the diagnostic content. Gating on --debug, wrapping the callback in its own try/catch so a diagnostic failure can't turn a best-effort probe into a hard failure, and the 100 lines of tests are all the right calls.