Skip to content

feat: complete SDK feature flag registry and add drift guard (#316) - #420

Merged
El-swaggerito merged 1 commit into
Axionvera:mainfrom
XxHugheadxX:feat/feature-flag-registry-completeness
Jul 28, 2026
Merged

feat: complete SDK feature flag registry and add drift guard (#316)#420
El-swaggerito merged 1 commit into
Axionvera:mainfrom
XxHugheadxX:feat/feature-flag-registry-completeness

Conversation

@XxHugheadxX

Copy link
Copy Markdown
Contributor

Related Issue

Implementation Scope

Read this first. The feature flag framework this issue asks for was
already delivered by PR #355 (@CiiscoDev), which merged on 28 July. This
PR does not re-implement it. It closes the one thing that framework had no
defence against — registry drift — and I am the contributor who caused the
first instance of it.

What #355 already delivers

Acceptance criterion Where it lives
Feature flag framework DEFAULT_FEATURE_FLAGS (src/config/index.ts:206), resolveFeatureFlags:221, isFeatureEnabled:279, assertFeatureEnabled:292
Configuration source metadata ConfigSourceMetadata, the sources map at src/config/index.ts:223-238
Experimental features disabled by default all registered flags default to false
Typed errors for disabled states DisabledFeatureError (src/errors/unsupported.ts:255)
Diagnostics carry config source info config.resolved emits sources and featureFlags (src/config/index.ts:428); src/diagnostics/report.ts includes both in the safe snapshot
Tests tests/feature-flags.test.ts
Documentation docs/feature-flags.md

That includes the specific gap I described when applying — the config.resolved
event carrying values but no provenance. #355 added it.

What this PR adds

A registered flag can drift out of the registry, silently.

experimentalVaultLocks gates the vault lock intents added in #274, but it is
passed as readiness.featureFlag — a variable, not a literal — so it never
appeared in a search of assertFeatureEnabled call sites. It was missing from
DEFAULT_FEATURE_FLAGS, from FeatureFlagKey, and from the documented table.

An unregistered flag still resolves to false, so nothing breaks. That is
exactly why it is silent: no consumer can discover the flag, and
config.resolved diagnostics never report its state — which undercuts the
config-source metadata this issue asks for.

This PR:

  1. Registers experimentalVaultLocks in DEFAULT_FEATURE_FLAGS and
    FeatureFlagKey, so it resolves, reports and documents like every other flag.
  2. Adds a canary test (tests/feature-flag-registry.test.ts) that scans
    src/ for experimental* flag keys and fails when one is unregistered or
    undocumented. Adding a flag without registering it is now a test failure.
  3. Marks two registered flags as Reserved. experimentalMultiAssetVault and
    experimentalAsyncSigner have zero references in src/ — they gate
    nothing. Removing them would break a published union, so the documented table
    now distinguishes Active from Reserved rather than implying
    capabilities the SDK does not have.
  • Modules changed: src/config/index.ts (+1 line), src/types/index.ts (+1 line), docs/feature-flags.md, tests/feature-flag-registry.test.ts (new)
  • Public API impact: one added key in DEFAULT_FEATURE_FLAGS and one added member of the FeatureFlagKey union. No behaviour change: the flag already resolved to false when unregistered and still does.

Tests Added / Changed

  • New tests added: tests/feature-flag-registry.test.ts — 7 tests
  • Failure paths covered: unregistered flag key, undocumented registered flag, a registered flag defaulting to anything other than false, an unknown flag resolving to false rather than throwing
  • Docs-only / config-only PR — not applicable

The scan matches the 'experimental…' string literal anywhere in src/,
not the call site, precisely because the flag that drifted was passed as a
variable. Written against the current tree the canary failed on
experimentalVaultLocks before the registry and docs were updated — it is a
test that has actually caught the bug it guards.

Verification

  • Ran the verify chain step by step

Run individually rather than through npm run verify, because that script
shells out to npm run per sub-step and this repository's dependency tree is
managed with a different package manager locally. test:coverage was not run;
every other step was:

tsc --noEmit                          2 errors — pre-existing, src/network/fee.ts
tsx scripts/check-circular-deps.ts    1 cycle — pre-existing, src/network/fee.ts
vitest run                            45 failed | 1031 passed | 1 skipped (1077)
tsx scripts/verify-acceptance.ts      checklist issue-316, 0 items remaining

Baseline measured on a clean git archive of upstream/main (aa17377):

main this branch
tsc errors 2 2
check:circular 1 cycle 1 cycle
Tests failed 45 45
Tests passed 1024 1031

Zero regressions, +7 tests. The 2 type errors and the cycle are both in
src/network/fee.ts, introduced by #415 and present on main; they are
unrelated to this issue and left to that module's author.

CI Status

  • Red only for a documented, pre-existing reason
Next steps: ✗ Fix failing automated checks, then re-run npm run verify:pr

verify:pr gates on the whole suite and the type check, both of which are
already failing on main for the reasons above.

Acceptance Criteria Coverage

Each criterion, with honest attribution:

Reviewer Notes

  • I applied to this issue before feat(config): implement configuration source metadata and feature fla… #355 landed. Most of what I described in my
    application has since been built by someone else, and I would rather say so
    plainly than present their work as mine. If the maintainers prefer to close
    this issue as delivered by feat(config): implement configuration source metadata and feature fla… #355, that is a reasonable call and I have no
    objection — the drift fix stands on its own and can be retargeted.
  • The drift was mine. experimentalVaultLocks was introduced in Implement SDK vault capability and action intent system #274, where
    I deliberately did not register it in DEFAULT_FEATURE_FLAGS to avoid editing
    the config module while it belonged to this issue. That was the right call for
    scope and the wrong outcome for discoverability; this is the correct place to
    fix it.
  • Reserved flags are documented, not removed. Deleting
    experimentalMultiAssetVault or experimentalAsyncSigner would be a breaking
    change to a published union for no functional gain. Labelling them follows the
    same reasoning the capability registry applies to planned entries: state the
    status rather than imply a capability.

Scope — what was deliberately left out

  • The 2 type errors and the dependency cycle in src/network/fee.ts — unrelated to this issue and another contributor's module.
  • No removal of the two Reserved flags — breaking change, no benefit.
  • No change to flag resolution, precedence or diagnostics plumbing — that is feat(config): implement configuration source metadata and feature fla… #355's work and it is correct.
  • The canary does not fail on Reserved flags. A registered-but-unused key is legitimate; it is required to be documented, not to be deleted.

@El-swaggerito
El-swaggerito merged commit 05dd11b into Axionvera:main Jul 28, 2026
1 check passed
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.

Implement SDK feature flag framework

2 participants