[codex] Improve extension SDK integration#35
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (37)
📝 WalkthroughWalkthroughThis PR enforces extension SDK/version and host capability validation, splits server vs client extension manifests (client manifests strip filesystem paths), installs a typed extension UI runtime and widens slot/context typings, tightens RBAC/authorization with stream and agency scopes, migrates GC Forms credentials to a database table with secret storage, extends tests and fixtures to validate boundaries, and updates workspace/CI scripts and submodule pins. ChangesExtension SDK and manifests
Extension UI runtime and component typing
Authorization and RBAC for extension routes
GC Forms credential management refactor
Test infrastructure and extension boundaries
Build, test, and quality automation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/gemini |
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the GCS Extension SDK, focusing on UI runtime isolation, capability-based validation, and improved RBAC. Key changes include a new UI runtime plugin to bridge host components to extensions, the implementation of SDK versioning requirements, and refined manifest serialization to ensure only browser-safe data is sent to the client. Additionally, the PR updates CI scripts and workspace setup to support comprehensive testing and coverage across submodules. Feedback was provided regarding a potential runtime crash in ExtensionSlotHost.vue due to a missing null check on an optional prop.
|
|
||
| const applicantRecipientId = computed(() => { | ||
| const value = context.applicantRecipientId | ||
| const value = 'applicantRecipientId' in context ? context.applicantRecipientId : undefined |
There was a problem hiding this comment.
The in operator will throw a TypeError if context is undefined. Since context is an optional prop, it can be undefined if not provided by the parent component. You should check if context exists before using the in operator to avoid a runtime crash.
const value = context && 'applicantRecipientId' in context ? context.applicantRecipientId : undefined
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/unit/extensions-core.test.ts (1)
415-425: ⚡ Quick winStrengthen capability detection to avoid false positives/negatives.
hasCapabilitycurrently relies on a plainincludesfor single-quoted literals, so valid double-quoted configs can fail, and unrelated string/comment content can pass. Prefer a boundary-aware match tied torequiredHostCapabilities.Proposed hardening
- const hasCapability = (capability: string): boolean => configSource.includes(`'${capability}'`) + const hasCapability = (capability: string): boolean => { + const escaped = capability.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + const pattern = new RegExp( + String.raw`requiredHostCapabilities\s*:\s*\[[\s\S]*?['"]${escaped}['"]`, + 'm' + ) + return pattern.test(configSource) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/extensions-core.test.ts` around lines 415 - 425, The current hasCapability function is fragile (uses configSource.includes with single quotes) and causes false positives/negatives; update hasCapability to perform a boundary-aware regex check that matches either single or double quoted literals and avoids matching inside larger words or comments (e.g. new RegExp(`(?<![\\w-])['"]${escapeRegExp(capability)}['"](?![\\w-])`)), and ensure detection is driven by the requiredHostCapabilities list when validating joinedSource in the blocks that check useExtensionApi/Fetch, useHostApi, and defineGcsExtensionNitroPlugin (referencing hasCapability, configSource, joinedSource, extensionName, violations).shared/types/database.d.ts (1)
568-571: ⚡ Quick winBroaden bigint ID fields to accept
string | number.
idandagency_idare currently typed as string-only, which is too narrow for bigint-backed IDs in this repo’s DB typing contract.Proposed fix
export interface ExtensionsGcsGcFormsCredentialTable { - id: Generated<string> - agency_id: string + id: Generated<string | number> + agency_id: string | number name_en: string name_fr: string key_id: stringAs per coding guidelines "Bigint IDs may arrive as string or number; schemas should accept both while enforcing requiredness."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/types/database.d.ts` around lines 568 - 571, The interface ExtensionsGcsGcFormsCredentialTable declares bigint-backed ID fields too narrowly; update the type of id from Generated<string> to Generated<string | number> and change agency_id from string to string | number so both fields accept string or number per the bigint-ID typing contract; locate these symbols (ExtensionsGcsGcFormsCredentialTable, id, agency_id, Generated) in the file and make the type adjustments consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/gcs-extensions.ts`:
- Around line 98-102: The caret-compatibility branch for requiredMajor === 0
should treat ^0.0.x as exact-patch only: update the logic using the variables
requiredMajor, requiredMinor, requiredPatch, hostMajor, hostMinor, hostPatch so
that when requiredMajor === 0 && requiredMinor === 0 you return hostMajor === 0
&& hostMinor === 0 && hostPatch === requiredPatch; keep the existing behavior
(hostMajor === 0 && hostMinor === requiredMinor && hostPatch >= requiredPatch)
for the case requiredMajor === 0 && requiredMinor > 0.
In `@package.json`:
- Line 38: The aggregate script "test:coverage:extensions" currently just chains
coverage runs and doesn't fail on low coverage; update it to enforce per-package
thresholds by either (A) adding Vitest coverage thresholds in each extension's
test config (e.g., set thresholds in vitest.config for
packages/gcs-ssc-extensions, extensions/gcs-automated-payments,
extensions/gcs-gcforms-integration, extensions/gcs-narrative-quality,
extensions/gcs-narrative-tags, extensions/gcs-outcome-cost-allocation) so their
tests fail when below policy, or (B) append a short post-check step to the
script that runs a Node/Bun check script (e.g., a new check-extensions-coverage
script) which reads each package's coverage/coverage-summary.json and exits
non-zero if any package's statement/branch/line coverage is under the required
threshold; ensure the package script name "test:coverage:extensions" invokes
that check after running all bun run ... test:coverage commands.
In `@server/database/migrations/9999_seed.ts`:
- Around line 1780-1793: The migration creates the table
extensions.gcs_gcforms_credentials only when stream.id === '31' but down()
always references it, causing nondeterministic rollbacks; move the CREATE TABLE
DDL for extensions.gcs_gcforms_credentials out of the stream.id === '31'
conditional (place it at top-level of up()) so the table is always created when
the migration runs, and ensure down() continues to DROP or reference the same
table name; update the up() function around the existing CREATE TABLE block and
any related logic so the table creation is deterministic across all execution
paths.
In `@tests/fixtures/nuxt-components.ts`:
- Around line 14-36: The module is missing an export for
AssessmentSchemaAccordionSection which causes imports (from
app/plugins/extension-ui-runtime.ts) to fail; add an export line using the
existing passthroughComponent helper — i.e. create export const
AssessmentSchemaAccordionSection =
passthroughComponent('AssessmentSchemaAccordionSection') alongside the other
passthroughComponent exports (e.g., UAccordion, UAlert) so the symbol is
available to tests and the plugin runtime.
In `@tests/unit/seed-advance-assessment.test.ts`:
- Around line 355-360: The credential lookup in the test is nondeterministic
because executeTakeFirstOrThrow() without ordering can return any non-deleted
credential; modify the query that builds "credential" (the
db.selectFrom('extensions.gcs_gcforms_credentials').selectAll().where(...).where(...).executeTakeFirstOrThrow()
chain) to enforce a deterministic selection—e.g. add a stable .orderBy(...)
(such as id or created_at) and/or an explicit filter that matches the known
seeded credential, and keep the executeTakeFirstOrThrow() call so the test
consistently picks the same record.
---
Nitpick comments:
In `@shared/types/database.d.ts`:
- Around line 568-571: The interface ExtensionsGcsGcFormsCredentialTable
declares bigint-backed ID fields too narrowly; update the type of id from
Generated<string> to Generated<string | number> and change agency_id from string
to string | number so both fields accept string or number per the bigint-ID
typing contract; locate these symbols (ExtensionsGcsGcFormsCredentialTable, id,
agency_id, Generated) in the file and make the type adjustments consistently.
In `@tests/unit/extensions-core.test.ts`:
- Around line 415-425: The current hasCapability function is fragile (uses
configSource.includes with single quotes) and causes false positives/negatives;
update hasCapability to perform a boundary-aware regex check that matches either
single or double quoted literals and avoids matching inside larger words or
comments (e.g. new
RegExp(`(?<![\\w-])['"]${escapeRegExp(capability)}['"](?![\\w-])`)), and ensure
detection is driven by the requiredHostCapabilities list when validating
joinedSource in the blocks that check useExtensionApi/Fetch, useHostApi, and
defineGcsExtensionNitroPlugin (referencing hasCapability, configSource,
joinedSource, extensionName, violations).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b553390a-2d19-4f0f-8081-4003f4fb412f
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
.gitmodulesapp/components/Common/Textarea.vueapp/components/Extension/ExtensionSlotHost.vueapp/plugins/extension-ui-runtime.tsextensions/gcs-automated-paymentsextensions/gcs-gcforms-integrationextensions/gcs-narrative-qualityextensions/gcs-narrative-tagsextensions/gcs-outcome-cost-allocationmodules/gcs-extensions.tspackage.jsonpackages/gcs-ssc-extensionsscripts/agent/quality-pr.shscripts/agent/quality-whole.shscripts/setup-workspaces.tsserver/api/extensions/[extensionKey]/[...route].tsserver/api/extensions/agency/[agencyId]/index.get.tsserver/api/extensions/streams/[streamId]/index.get.tsserver/database/migrations/9999_seed.tsserver/utils/extension-dispatch-route.tsserver/utils/extensions.tsshared/types/database.d.tsshared/types/schemas/extensions.tsshared/utils/extension-sdk-aliases.tsshared/utils/extensions.tsspecs/extensions-sdk.mdtests/fixtures/nuxt-components.tstests/fixtures/nuxt-imports.tstests/unit/extensions-core.test.tstests/unit/extensions-dispatch-route.test.tstests/unit/extensions-routes-behavior.test.tstests/unit/outcome-cost-allocation-extension-ui.test.tstests/unit/seed-advance-assessment.test.tstests/unit/setup.tstests/unit/stream-extensions-route.test.tsvitest.config.ts
97bc383 to
83709b1
Compare
|
@coderabbitai review |
|
/gemini |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/utils/extension-dispatch-route.ts (1)
52-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winManual stream routes still bypass stream-level disablement.
assertExtensionStreamEnabled()only checks that the stream exists and the agency is enabled. After the validation changes inmodules/gcs-extensions.ts, handlers can legally useauth: "manual"without RBAC, and those routes never reach the latergetExtensionStreamConfiguration(...).enabledcheck inprepareExtensionRbacContext(). A manual stream route can therefore dispatch even when the extension is disabled for that stream.Proposed fix
const assertExtensionStreamEnabled = async ( event: H3Event, extensionKey: string, streamId: string | undefined ): Promise<void> => { if (!streamId) { return } const streamContext = await resolveExtensionStreamContext(event.context.$db, streamId) if (!streamContext) { throwExtensionDispatchError(404, 'TRANSFER_PAYMENT_STREAM_NOT_FOUND', 'Transfer payment stream not found.') } const resolvedStreamContext = streamContext as NonNullable<typeof streamContext> + const streamConfiguration = await getExtensionStreamConfiguration(event.context.$db, extensionKey, streamId) + if (!streamConfiguration.enabled) { + throwExtensionDispatchError(403, 'EXTENSION_STREAM_DISABLED', 'Extension is disabled for this stream.') + } + const isAgencyEnabled = await isExtensionEnabledForAgency(event.context.$db, extensionKey, resolvedStreamContext.agencyId) if (!isAgencyEnabled) { throwExtensionDispatchError(403, 'EXTENSION_AGENCY_DISABLED', 'Extension is disabled for this agency.') } }Also applies to: 238-241
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/utils/extension-dispatch-route.ts` around lines 52 - 71, assertExtensionStreamEnabled currently only verifies the stream exists and the agency-level extension is enabled, so manual auth routes can bypass stream-level disablement; after resolving streamContext in assertExtensionStreamEnabled (and in the other similar occurrence), fetch the stream configuration via getExtensionStreamConfiguration(event.context.$db, extensionKey, streamId) and check its enabled flag, and if disabled throwExtensionDispatchError(403, 'EXTENSION_STREAM_DISABLED', 'Extension is disabled for this stream.'); ensure you import/use getExtensionStreamConfiguration and apply the same change where the same stream-level guard logic appears so manual routes cannot dispatch when a stream is disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/agent/quality-whole.sh`:
- Around line 62-63: The gate for "test:coverage:extensions" is a false-green
because run_gate only checks exit code; wrap the bun run
test:coverage:extensions invocation with an explicit threshold check: run the
command (as currently invoked by run_gate "test:coverage:extensions" bun run
test:coverage:extensions), capture its coverage summary output (or generate the
JSON/LCOV summary), parse the per-package statement coverage for extension
packages (e.g., packages/gcs-ssc-extensions and
extensions/gcs-outcome-cost-allocation), and if any package's coverage is below
a configurable threshold (add COVERAGE_EXTENSIONS_THRESHOLD or hardcode a
sensible percent) exit non-zero so run_gate fails; update the script to call
run_gate only after this check or make the wrapper return non-zero when
thresholds are violated.
In `@server/database/migrations/9999_seed.ts`:
- Around line 1510-1523: The seed migration creates a persistent schema object
extensions.gcs_gcforms_credentials in up() but down() only deletes rows, making
the migration irreversible; update this by either moving the CREATE TABLE DDL
into the dedicated GC Forms extension/schema migration (so schema changes live
with that subject area) or make the current migration reversible by adding a
DROP TABLE IF EXISTS extensions.gcs_gcforms_credentials in down(); target the
up()/down() functions in 9999_seed.ts and ensure the table name
extensions.gcs_gcforms_credentials is handled symmetrically.
In `@tests/unit/extension-slot-host.test.ts`:
- Around line 66-94: The test currently calls vi.unstubAllGlobals() only on the
success path which allows mocked globals (computed, useFetch) to leak if
mount(ExtensionSlotHost) or the assertion throws; fix by moving cleanup to a
guaranteed teardown (add an afterEach that calls vi.unstubAllGlobals()) or wrap
the test body in try/finally and call vi.unstubAllGlobals() in finally; ensure
you reference the same mocks (getGcsExtensionComponentMock, computed, useFetch)
so they are unstubbed after each run and leave the rest of the test (mount,
assertions) unchanged.
---
Outside diff comments:
In `@server/utils/extension-dispatch-route.ts`:
- Around line 52-71: assertExtensionStreamEnabled currently only verifies the
stream exists and the agency-level extension is enabled, so manual auth routes
can bypass stream-level disablement; after resolving streamContext in
assertExtensionStreamEnabled (and in the other similar occurrence), fetch the
stream configuration via getExtensionStreamConfiguration(event.context.$db,
extensionKey, streamId) and check its enabled flag, and if disabled
throwExtensionDispatchError(403, 'EXTENSION_STREAM_DISABLED', 'Extension is
disabled for this stream.'); ensure you import/use
getExtensionStreamConfiguration and apply the same change where the same
stream-level guard logic appears so manual routes cannot dispatch when a stream
is disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83196301-78be-452b-bf6b-cd572be495ed
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.gitmodulesapp/components/Common/Textarea.vueapp/components/Extension/ExtensionSlotHost.vueapp/plugins/extension-ui-runtime.tsextensions/gcs-automated-paymentsextensions/gcs-gcforms-integrationextensions/gcs-narrative-qualityextensions/gcs-narrative-tagsextensions/gcs-outcome-cost-allocationmodules/gcs-extensions.tspackage.jsonpackages/gcs-ssc-extensionsscripts/agent/quality-pr.shscripts/agent/quality-whole.shscripts/setup-workspaces.tsserver/api/extensions/[extensionKey]/[...route].tsserver/api/extensions/agency/[agencyId]/index.get.tsserver/api/extensions/streams/[streamId]/index.get.tsserver/database/migrations/9999_seed.tsserver/utils/extension-dispatch-route.tsserver/utils/extensions.tsshared/types/database.d.tsshared/types/schemas/extensions.tsshared/utils/extension-sdk-aliases.tsshared/utils/extensions.tsspecs/extensions-sdk.mdtests/fixtures/nuxt-components.tstests/fixtures/nuxt-imports.tstests/unit/extension-slot-host.test.tstests/unit/extensions-core.test.tstests/unit/extensions-dispatch-route.test.tstests/unit/extensions-routes-behavior.test.tstests/unit/outcome-cost-allocation-extension-ui.test.tstests/unit/seed-advance-assessment.test.tstests/unit/setup.tstests/unit/stream-extensions-route.test.tsvitest.config.ts
✅ Files skipped from review due to trivial changes (4)
- .gitmodules
- extensions/gcs-automated-payments
- tests/unit/extensions-routes-behavior.test.ts
- extensions/gcs-narrative-tags
🚧 Files skipped from review as they are similar to previous changes (26)
- tests/unit/outcome-cost-allocation-extension-ui.test.ts
- app/components/Common/Textarea.vue
- extensions/gcs-narrative-quality
- server/api/extensions/streams/[streamId]/index.get.ts
- extensions/gcs-outcome-cost-allocation
- shared/utils/extension-sdk-aliases.ts
- server/api/extensions/[extensionKey]/[...route].ts
- package.json
- extensions/gcs-gcforms-integration
- scripts/agent/quality-pr.sh
- tests/fixtures/nuxt-components.ts
- tests/unit/extensions-dispatch-route.test.ts
- tests/unit/seed-advance-assessment.test.ts
- server/api/extensions/agency/[agencyId]/index.get.ts
- shared/types/schemas/extensions.ts
- tests/unit/setup.ts
- app/plugins/extension-ui-runtime.ts
- packages/gcs-ssc-extensions
- app/components/Extension/ExtensionSlotHost.vue
- scripts/setup-workspaces.ts
- shared/utils/extensions.ts
- shared/types/database.d.ts
- vitest.config.ts
- specs/extensions-sdk.md
- tests/fixtures/nuxt-imports.ts
- tests/unit/extensions-core.test.ts
| run_gate "test:coverage" bun run test:coverage | ||
| run_gate "test:coverage:extensions" bun run test:coverage:extensions |
There was a problem hiding this comment.
test:coverage:extensions is still a false-green gate.
run_gate treats exit code 0 as a pass, but the PR note already shows bun run test:coverage:extensions exits 0 with extension packages below policy (packages/gcs-ssc-extensions at 42.17% statements and extensions/gcs-outcome-cost-allocation at 76.36%). quality-whole will therefore report this gate as passing even when extension coverage regresses. Add an explicit threshold check here until the underlying command fails correctly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/agent/quality-whole.sh` around lines 62 - 63, The gate for
"test:coverage:extensions" is a false-green because run_gate only checks exit
code; wrap the bun run test:coverage:extensions invocation with an explicit
threshold check: run the command (as currently invoked by run_gate
"test:coverage:extensions" bun run test:coverage:extensions), capture its
coverage summary output (or generate the JSON/LCOV summary), parse the
per-package statement coverage for extension packages (e.g.,
packages/gcs-ssc-extensions and extensions/gcs-outcome-cost-allocation), and if
any package's coverage is below a configurable threshold (add
COVERAGE_EXTENSIONS_THRESHOLD or hardcode a sensible percent) exit non-zero so
run_gate fails; update the script to call run_gate only after this check or make
the wrapper return non-zero when thresholds are violated.
There was a problem hiding this comment.
Same call as the package-level coverage thread: I agree this is a policy gap, but I am not adding a hard threshold in quality-whole while the existing aggregate command reports below-policy coverage and exits 0. That would make this PR fail for inherited coverage debt. The current PR keeps the extension coverage command visible in both quality workflows and documents the gap in the PR body; enforcing it should follow after raising the affected packages to the threshold.
There was a problem hiding this comment.
@omarmir, understood — the phased approach makes sense: surface the gap now and enforce thresholds only after the inherited coverage debt in packages/gcs-ssc-extensions and extensions/gcs-outcome-cost-allocation has been addressed. Appreciated the clear reasoning.
Would you like me to open a follow-up GitHub issue to track the enforcement work (raising those packages to threshold and then adding the hard check to quality-whole.sh)?
✏️ Learnings added
Learnt from: omarmir
Repo: GCS-SSC/gcs-ssc PR: 35
File: scripts/agent/quality-whole.sh:62-63
Timestamp: 2026-05-19T06:06:40.931Z
Learning: In `scripts/agent/quality-whole.sh`, the `test:coverage:extensions` gate intentionally exits 0 even when extension packages are below coverage policy (e.g., `packages/gcs-ssc-extensions` at ~42% and `extensions/gcs-outcome-cost-allocation` at ~76%). Hard threshold enforcement is deferred until those packages reach the policy level; the gap is documented in the PR body rather than enforced in the script to avoid failing PRs on inherited coverage debt.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
83709b1 to
b255a4b
Compare
Summary
Funding_Case_Agreement_Profileand adds regression coverage in the SDK packageReview Notes
main.packages/gcs-ssc-extensionsreports 42.17% statements andextensions/gcs-outcome-cost-allocationreports 76.36% statements underbun run test:coverage:extensions, although the command currently exits 0 because thresholds are not enforced there.Validation
bun run lintbun run typecheckbun run test:unitbun run test:coveragebun run test:coverage:extensionsbun run test:e2e:fast/en/agreements/51, confirmed the agreement tabs no longer include GC Forms and the page renders without console errors.Summary by CodeRabbit
New Features
Improvements
Tests
Chores