Repository navigation
Conversation
Resolves the conflict in vault.service.spec.ts by taking master's spec and re-applying the namespace test block on top of it. Master added three Vault call sites since the branch point (canCreateUserKey, the shared pqRequest path, kvCreate); they now go through namespaceHeaders() like the rest, and the header table covers all fourteen. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Coverage report
Test suite run success386 tests passing in 29 suites. Report generated by 🧪jest coverage report action from 31b8480 |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation consistently covers all Vault request paths with focused regression tests and aligned documentation.
Review effort: Balanced
Findings: None
What changed in this PR
Centralizes Vault namespace headers and applies them consistently across all Vault requests.
Changes:
- Adds a shared namespace-header helper.
- Expands tests for namespaced and non-namespaced requests.
- Documents namespace configuration and mount requirements.
| File | Description |
|---|---|
src/vault/vault.service.ts |
Applies namespace headers to all Vault calls. |
src/vault/vault.service.spec.ts |
Adds comprehensive namespace-header coverage. |
README.md |
Documents namespaced Vault setup. |
.env.template |
Explains VAULT_NAMESPACE configuration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bwmx
previously approved these changes
Oct 6, 2026
The PQ spec block kept its own `ok` helper, identical to the shared `okResponse`; its calls now use `okResponse`, as was already done for `configWith`.
PhearZero
approved these changes
Oct 6, 2026
Collaborator
|
We have a rule that you must sign commits @JBScaled . Once the commit is signed and pushed, it will trigger another review. Implementation looks good! Thanks for the contribution! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Closes #46.
VaultService sent the
X-Vault-Namespaceheader on most of its Vault calls but not on four of them (getTokenWithRole,checkToken,transitCreateKey,getKeys). Those four went to the root namespace, where a namespaced Vault keeps none of our mounts. Sign-in, user creation and user listing answered 403, and the AppRole login at boot failed.What changed
One private helper,
namespaceHeaders(), returns the header whenVAULT_NAMESPACEis set and an empty object otherwise. All fourteen call sites spread it into their headers, including the three that landed on master with post-quantum accounts (canCreateUserKey, the shared PQ request path,kvCreate). The ten that already sent the header send exactly what they sent before.getTokenWithRolenever passed an axios config, so it gets one.Behaviour change
AppRole login used to ignore
VAULT_NAMESPACEand log in at root. Now it logs in inside the namespace like every other call. If you run a namespaced Vault with AppRole mounted at root and the transit mounts in a child namespace, login breaks after this change. Move the AppRole mount into the namespace, alongside the transit, PQ and KV mounts..env.templateand the README both say so.Tests
pqListKeysstands in for the four PQ methods, which share one request path.getTokenWithRolehad no tests. It gets a success case and a 401 case.jest.resetAllMocks()never reset these mocks.jest-create-mock-instancebuilds them on its own ModuleMocker, so call history leaked from test to test. The spec now recreates the mocks before each test. Without this, the empty-namespace check passed on a broken helper.checkTokenfixtures returned the base URL for every config key, including the namespace. They now stub by key, using one sharedconfigWithhelper that replaces the two identical local copies master had. One describe label that named the wrong method is corrected. Nothing else in the existing tests moved.getTokenWithRolewithout its config, and an undefined-valued header all fail named tests.Verified against OpenBao 2.7.1
Run on the merged branch, with an
intermezzo/namespace holding the transit, KV and AppRole mounts and a policy granting the PQ engine paths. With master's service, boot-time AppRole login threwVaultException, andPOST /v1/wallet/userandGET /v1/wallet/usersreturned 403. With this branch, login succeeds and they return 201 and 200. Not tested on Vault Enterprise or HCP. CI runs with the namespace empty.