Skip to content

fix: prefer app bundle name over sdef dictionary title#15

Open
mickdarling wants to merge 1 commit into
developfrom
fix/sdef-app-name-priority
Open

fix: prefer app bundle name over sdef dictionary title#15
mickdarling wants to merge 1 commit into
developfrom
fix/sdef-app-name-priority

Conversation

@mickdarling
Copy link
Copy Markdown
Contributor

Summary

The sdef interrogator used the <dictionary title="Mail Terminology"> attribute as the adapter name. This propagated through the entire pipeline, producing @mcpaql/generated-Mail Terminology-adapter as the package name.

Now uses priority order: --app flag > app name from .app bundle path > dictionary title.

Closes #14

Test plan

  • --app Mail produces name: "Mail" in discovery bundle
  • --sdef /path/to/Mail.app/.../Mail.sdef (no --app) produces name: "Mail" from path
  • Fallback to dictionary title still works when path has no .app segment

🤖 Generated with Claude Code

The sdef dictionary title (e.g. 'Mail Terminology') was used as the adapter
name, propagating through the entire pipeline. Now prefers: --app flag >
bundle name from .app path > dictionary title.

Closes #14
@mickdarling
Copy link
Copy Markdown
Contributor Author

@claude review this PR — fixes sdef interrogator to prefer app bundle name over dictionary title

@claude
Copy link
Copy Markdown

claude Bot commented Apr 4, 2026

Claude finished @mickdarling's task in 1m 0s —— View job


PR Review

  • Gather context
  • Read changed file and diff
  • Provide review feedback

Overall: LGTM — clean, focused fix. The change is correct and the priority order makes intuitive sense.

What changed

src/sdef-cli.ts:71-73 introduces a derivedAppName variable that consolidates the name-resolution logic with a clear priority chain:

--app flag  >  .app bundle path  >  sdef dictionary title  >  filename

Previously the name was resolved inline as sdef.application || appName || ..., putting the dictionary title (e.g. "Mail Terminology") first — the root cause of the bad package names like @mcpaql/generated-Mail Terminology-adapter.

Correctness

  • Regex /\/([^/]+)\.app\// correctly captures the bundle name from standard macOS paths (/Applications/Mail.app/Contents/Resources/Mail.sdef"Mail"). The trailing / in the pattern means it won't false-match a bare .app directory reference, which is fine since the input is always a .sdef file path.
  • --app + --sdef together: if both flags are passed, appName wins, which is the correct and expected behavior.
  • server_url improvement: the old code could produce native-applescript://undefined if both sdef.application and appName were absent. Now derivedAppName is always a non-empty string (the path.basename fallback guarantees this), so that silent bug is also fixed.
  • No .app segment in path (e.g. --sdef /tmp/foo.sdef): bundleMatch is null, the ternary short-circuits cleanly to null, and the chain falls through to sdef.application then "foo" — matching the documented fallback behavior.

Minor observations

  1. Comment omits the final fallback — line 71 says dictionary title but doesn't mention the path.basename tail. Not a bug, just slightly incomplete documentation.

  2. Spaces in server_url — if sdef.application is used as the final name (e.g. "Mail Terminology"), the URL becomes native-applescript://Mail Terminology. This was already the case before, so not a regression introduced here, but worth noting as a future cleanup.

  3. DRY win — replacing three independent sdef.application || appName chains with a single derivedAppName is a nice readability improvement.

Test plan coverage

The three test cases in the PR description cover the happy path, the path-derived case, and the fallback — that's exactly the right set.

No issues blocking merge.

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.

fix: sdef interrogator should prefer app bundle name over dictionary title

1 participant