Skip to content

fix(qa): additional skill routing gaps — video-plan, Korean NLP, prewarm, null-safety#419

Open
heodongun wants to merge 5 commits into
masterfrom
fix/qa-null-safety-20260526
Open

fix(qa): additional skill routing gaps — video-plan, Korean NLP, prewarm, null-safety#419
heodongun wants to merge 5 commits into
masterfrom
fix/qa-null-safety-20260526

Conversation

@heodongun

@heodongun heodongun commented May 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Removed unauthorized unskilled-agent fallback in resolveOperatorSkillAgent — now returns null when no agent has the requested skill allowlisted
  • Fixed test to override ALL seeded agent profiles so the no-fallback assertion isn't defeated by the Engineering Lead's pre-seeded browser-smoke allowlist
  • Fixed bufferedReader resource leak across 5 call sites — process.inputStream.bufferedReader().readText() is now wrapped with .use {} in LocalPlaywrightBrowserSkillRunner, LocalPlaywrightMarketingBrowserRunner, DesktopAppService, PipelineOrchestrator, and SyntaxValidator

Previous commits on this branch also included:

  • video-plan added to Marketing Operator skill allowlist
  • Korean social/content keywords added to looksLikeOperatorSkillRequest
  • Blank-skill guard in runOperatorSkillTool
  • prewarm() called eagerly in DesktopAppService.init
  • Null-safety fixes in DefaultResultAnalyzer and CompanyIssueReadinessResolver

Test plan

  • All 807 tests pass (./gradlew test jacocoTestCoverageVerification)
  • resolveOperatorSkillAgent test overrides all company agents to ["graphify"] then asserts browser-smoke resolves to null
  • Bufferedreader .use {} wrapping is a pure resource-hygiene fix; no behavior change under normal execution

🤖 Generated with Claude Code

heodongun and others added 5 commits May 26, 2026 15:06
…tartup, blank-skill guard

- Add video-plan to marketingOperatorSkillSettings skillAllowlist so Marketing
  Operator agents can dispatch it without manual override
- Expand looksLikeOperatorSkillRequest skillTarget with Korean social/content
  keywords (소셜, 블로그, cms, 고객, 타깃, 콘텐츠)
- Trigger browserSkillRunner.prewarm() eagerly in DesktopAppService init via
  serviceScope so the first browser-smoke execution doesn't pay the npm install cost
- Add early-return guard when inferOperatorSkillName produces a blank name, with a
  clear diagnostic message instead of a confusing agent-lookup failure
- Null-safety fixes: DefaultResultAnalyzer (!! → checkNotNull) and
  CompanyIssueReadinessResolver (!! → safe-call chain)
- Tests: 3 new SkillRuntimeTest cases covering video-plan allowlist, Korean NLP
  keywords, and prewarm-at-startup behaviour

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tartup, blank-skill guard

- Add video-plan to marketingOperatorSkillSettings skillAllowlist
- Expand looksLikeOperatorSkillRequest skillTarget with Korean social/content
  keywords (소셜, 블로그, cms, 고객, 타깃, 콘텐츠)
- Trigger browserSkillRunner.prewarm() eagerly in DesktopAppService init via
  serviceScope so the first browser-smoke run pays no npm install cost
- Add early-return guard when skillName is blank with a clear diagnostic message
- Tests: 3 new SkillRuntimeTest cases for video-plan allowlist, Korean NLP
  keywords, and prewarm-at-startup behaviour

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The final fallback `?: enabledDefinitions.firstOrNull()?.id` returned any
enabled agent even when none had the requested skill in their allowlist,
silently bypassing delegation policy.

Removing it makes the function return null when no allowlisted agent exists,
which correctly triggers the "No allowed agent" error path in runOperatorSkillTool.

Also makes resolveOperatorSkillAgent internal for direct test coverage.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gent profiles

The seeded Engineering Lead profile already includes browser-smoke in its allowlist.
The test needs to override ALL agents in the company, not just Builder, so that
browser-smoke truly has no allowed agent.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e leaks

Process inputStream was read via bufferedReader() without .use{}, so the
underlying reader was never closed if an exception was thrown before the
JVM GC could collect it. All five call sites are now wrapped with .use{}.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant