[guimard]

What and why

src/config/args.ts no longer imports any plugin option set: the core declares the options it reads. src/bin/options.ts composes them with every plugin family in the previous order, and DM accepts that template as a constructor parameter. This is the last prerequisite of the package split tracked in #397.

priority.json moves to src/config/, the options the core reads stay in args.ts (only authz_scope_source, read by the authzScope plugin, moved to auth.options.ts), and configuredSchemaFiles takes the option template it needs instead of importing it.

Reviews performed

• Self-review of the final diff
• AI review 1 — assistant: Claude. Found that --ldap-concurrency and the authz_*/auth_path_prefix options read by the core had moved into plugin option sets, so a core-only template stringified or ignored them; fixed in a2ef44f. Also flagged that DM's default argument still imports the umbrella composition, now named in the plan.
• AI review 2 — assistant: DeepSeek V4.1 Flash. Approved a2ef44f: composed template equivalent to the old one (290 entries, none added or removed), core-read options back in args.ts, 3800 passing, all gates green, no published-contract change. Two non-blocking nits.
• Human code owner, other than the author — required for the paths in .github/CODEOWNERS

Checklist

• A test covers the change, and fails without it — the options snapshot, parser and plugin-option suites pin the composed order and values; the full test:dev suite passes.
• npm run check passes
• CHANGELOG.md updated when administrators see the change — not needed: internal refactor with no administrator-visible change.
• The comments around the touched code still explain something true and non-obvious (AGENTS.md)

[guimard]

Review of 1d80165, checked out in a separate worktree: npm run check is green, and the touched suites plus the authzPerBranch/authzCombine ones give 257 passing against the Docker OpenLDAP.

The composition itself is faithful: the options snapshot is unchanged, withConfig views get optionKeys/configTemplate through Object.assign, and dist/bin/options.js is built for bin/cleanup-external-users.mjs. One move goes against the goal of the PR, though.

To fix

--ldap-concurrency moved from the core into jamesOptions (src/plugins/twake/james.options.ts:13). It sat under a "James plugin" comment in args.ts, but its only reader is the core: src/lib/ldap/state.ts:48 sizes the global LDAP pLimit with it. Before this PR it was part of the core template; now only the composed template knows it. With the core template alone — what new DM(options) now allows, and what @ldap-rest/core will ship — on this head:

new ConfigParser(coreArgs), --ldap-concurrency 5
  → config.ldap_concurrency === "5" (string, reported as unknown option)
  → pLimit("5") throws "Expected `concurrency` to be a number from 1 and up"
new ConfigParser(coreArgs), DM_LDAP_CONCURRENCY=5
  → config.ldap_concurrency === undefined (silently ignored)

Please keep it in src/config/args.ts.

Worth a decision

Options the core reads are now declared by the auth family. authzOptions (src/plugins/auth/auth.options.ts:28) carries authz_combine and authz_for, which DM itself reads at startup through src/lib/authz/composition.ts:339 and :75/:283. authz_unresolved_user, authz_transit_* and authz_filter_attached_entries (now in perBranchOptions) are read by src/lib/authz/base.ts, and auth_path_prefix by src/lib/auth/base.ts. The composed template hides it today, but after the split a core-only server, or a third-party authz plugin extending AuthzBase without @ldap-rest/plugin-auth, gets these silently ignored (same probe: DM_AUTHZ_COMBINE=true → undefined). If lib/auth and lib/authz stay in core, as the module map says, their options belong in core too; if they are meant to follow the auth family, the plan should say so.

DM still imports every family at runtime. constructor(options: ConfigTemplate = configArgs) with import configArgs from './options' (src/bin/index.ts:76, :348) keeps src/bin/index.ts → src/bin/options.ts → src/plugins/**/*.options.ts. The default is a sensible transition (176 test files and three bin/*.mjs scripts call new DM()), but docs/development/modularization-plan.md:19 now says "The core no longer imports plugin code", while the plan's own rule is "runtime option imports leave the core". Please name this edge as what remains — dropping the default, with the callers passing the template, when options.ts moves to the umbrella — rather than declaring the core decoupled.

Nits

• docs/usage/plugins/auth/authz-per-route.md:17 still says src/plugins/priority.json.
• docs/architecture/adr/0003-authorization-composition.md:59 points to src/config/args.ts; the composition options now live in src/plugins/auth/auth.options.ts.
• src/bin/options.ts:144-155: the authzDynamic comment describes authz_dynamic_bypass but stays in the composition root, which moves to the umbrella. The authz comments moved next to their declarations in this PR; this one could move above authz_dynamic_bypass in auth.options.ts too.

— Claude

[guimard]

Fixed in a2ef44f:

• --ldap-concurrency, --auth-path-prefix and the authz_* options read by DM, lib/auth and lib/authz are back in src/config/args.ts. Only authz_scope_source, read by the authzScope plugin, stays in auth.options.ts.
• The plan now names the remaining edge: DM's default argument still imports src/bin/options.ts; phase 2 drops the default and lets the umbrella pass the template.
• authz-per-route.md uses the new priority.json path, ADR 0003 points to the right option files, and the authzDynamic comment sits next to authz_dynamic_bypass.

Re-checked: core-only --ldap-concurrency 5 parses as a number and DM_LDAP_CONCURRENCY/ DM_AUTHZ_COMBINE are read; npm run test:dev gives 3800 passing, and npm run check is green.

[guimard]

This line and the one further down (Import the declaration in src/bin/options.ts ..., 83 chars) pass the 80-column wrap the rest of the file keeps. Since the paragraph was reflowed anyway by replacing args.ts, re-wrapping it here would keep the diff consistent. Cosmetic only.

— Deepseek

[guimard]

Typo carried over from args.ts: should be Backchannel.

Worth noting for the record: --authz-scope-source now sits at position 229 of 290 instead of 226. That is harmless — the parser is name-based and findEntry sorts neither — and the runtime value is identical for unset, empty and set DM_AUTHZ_SCOPE_SOURCE. But it is now a defineOptions entry rather than a hand-written one, so if the composed order is meant to be stable (e.g. for the future umbrella or options.snapshot.json diffs), a short comment here stating the order is normative would make an accidental move visible.

— Deepseek

[guimard]

Review: approved

I checked out a2ef44f4 in a separate worktree and ran the checks locally (full mocha suite with Docker, check:ts, eslint, prettier, the API and options snapshots, check-openapi, hooks-doc, check-comments, scripts/contracts.ts --base <merge-base>): all green, 3800 passing / 0 failing.

Core correctness

• The composed template is equivalent to the old one. I compared src/bin/options.ts in the head with src/config/args.ts in the base on the same checkout: 290 entries on both sides, no option added or removed, no duplicate, and the only value differences are the checkout path in the --schemas-path / --group-schema / --static-path defaults. --authz-scope-source gains string, null, null, false, which changes nothing: I also checked the parsed value for DM_AUTHZ_SCOPE_SOURCE unset, empty and set, and it is identical before and after. The entry order changed only for that option (226th → 229th), which is safe since ConfigParser.findEntry matches by name; see the note on src/bin/options.ts.
• The options the core really reads are kept in args.ts, and each claim in the body holds: authz_unresolved_user and authz_identity are read by lib/authz/base.ts and lib/auth/base.ts, authz_for/authz_combine by lib/authz/composition.ts, auth_path_prefix by lib/auth/base.ts, and ldap_concurrency by lib/ldap/state.ts. Only authz_scope_source moved to auth.options.ts, which is the one read solely by the authzScope plugin.
• The new seams behave as advertised. With new DM(template) the parser only knows the injected keys (optionKeys has the injected key and not auth_token), configTemplate is what the unknown-options report diffs against, and a core-only template reports an undeclared DM_* variable the same way. withConfig copies configTemplate/optionKeys by reference/instance as intended; static and DmPlugin.options() read them from this.server, so plugin views keep working.
• The remaining importers are type-only (import type { Config }, BranchPermissions, ...), so the core's runtime dependency on plugin option sets is indeed gone. The one runtime edge still there — DM's default constructor argument (src/bin/index.ts: import configArgs from './options') — is documented in modularization-plan.md. That default is also what keeps the many new DM() callers (tests, bin/*.mjs) working, which is a good call for this step.

The priority.json move and every reference to it (code, tests, docs, ADRs, module map) are updated; scripts/buildDockerfile.ts regenerates the Dockerfile byte-for-byte, dist/bin/options.js is emitted by Rollup as expected, and the contracts job reports no published-contract change. I agree the CHANGELOG doesn't need an entry.

The only remarks are two nits, not blocking (they are posted inline): a slightly stale line wrapping in docs/plugin-development/options.md, and the Backchannel typo in a comment in src/bin/options.ts.

One optional suggestion, for a later step rather than this PR: DmPlugin.options() and configuredSchemaFiles() are exactly the authority the package split will use, and no test drives them through an injected non-default template today. A small test constructing new DM(customTemplate) and checking optionKeys and the unknown-option report would pin the behaviour the split depends on.

Note on the GitHub verdict: the API account running this review is the author of the PR (guimard), and GitHub refuses self-approval. This body is an explicit approval; please treat it as approve and merge once a human code owner from .github/CODEOWNERS has reviewed, as the PR body already requires.

Signed-off-by: Deepseek

[guimard]

Added a docs-only commit a7dde37 after the DeepSeek review: the plan now records that phase 2 must export ConfigTemplate and ConfigEntry from the umbrella, since DM's constructor already names them in the published reports but no entry point exports them yet.