Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an AST-based GenAI boundary guard with import, export, dependency-manifest, and packaging enforcement; extensive scanner and fixture tests; CI wiring; and runtime, dependency, and procedural maintenance updates. ChangesGenAI boundary enforcement
Runtime and maintenance updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
OpenCodeReview — PR #2546
|
Contributor
LLxprt PR Review – PR #2546Issue AlignmentThe PR directly addresses #2352 by adding an AST-based enclave guard, CI wiring, documentation, and a dependency/inventory ratchet. The reviewed artifacts show the intended behavior: only Side Effects
Code QualityFrom the available diffs:
Tests and Coverage
VerdictNeeds Work |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/tests/genai-enclave-guard.test.ts (1)
41-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the shared harness from
genai-enclave-guard-helpers.tsinstead of duplicating it.
RUNTIME,bunAvailable,ScriptResult,runScript,runScriptRealRepo,withFixture, andGEMINI_IMPORTare already exported byscripts/tests/genai-enclave-guard-helpers.ts(whichgenai-published-root.test.tsconsumes). The duplicated copies here have already drifted: this file usesprocess.env.BUN_EXECUTABLE ?? 'bun'(Line 44) while the helper uses|| 'bun'(empty-string safe), and the localrunScript/runScriptRealRepodo not delegate to the sharedrunGuard. Consolidating on the helper module removes ~150 lines and prevents further behavioral divergence.🤖 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/tests/genai-enclave-guard.test.ts` around lines 41 - 218, Replace the duplicated harness definitions in genai-enclave-guard.test.ts with imports from genai-enclave-guard-helpers.ts, including RUNTIME, bunAvailable, ScriptResult, runScript, runScriptRealRepo, withFixture, and GEMINI_IMPORT. Remove the local implementations and update usages to rely on the shared helper behavior, including runGuard delegation and empty-string-safe BUN_EXECUTABLE handling.
🤖 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.
Nitpick comments:
In `@scripts/tests/genai-enclave-guard.test.ts`:
- Around line 41-218: Replace the duplicated harness definitions in
genai-enclave-guard.test.ts with imports from genai-enclave-guard-helpers.ts,
including RUNTIME, bunAvailable, ScriptResult, runScript, runScriptRealRepo,
withFixture, and GEMINI_IMPORT. Remove the local implementations and update
usages to rely on the shared helper behavior, including runGuard delegation and
empty-string-safe BUN_EXECUTABLE handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a7ada948-d33b-40fc-9cc7-6a47beb6ee90
⛔ Files ignored due to path filters (2)
dev-docs/genai-enclave-boundary-rules.mdis excluded by!dev-docs/**dev-docs/genai-import-baseline.mdis excluded by!dev-docs/**
📒 Files selected for processing (14)
.github/workflows/ci.yml.llxprt/LLXPRT.mdpackage.jsonpackages/core/src/utils/getPty.tspackages/providers/src/fake/FakeProvider.tsscripts/check-genai-enclave.tsscripts/genai-enclave/config.tsscripts/genai-enclave/scanner.tsscripts/tests/genai-enclave-guard-allowlist.test.tsscripts/tests/genai-enclave-guard-helpers.tsscripts/tests/genai-enclave-guard.test.tsscripts/tests/genai-enclave-scanner.test.tsscripts/tests/genai-published-root.test.tsscripts/tests/publish-integrity.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
scripts/genai-enclave/provenance.ts (1)
211-225: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove
clearNonShadowEntries()or wire it into the fixed-point loop. It isn’t called anywhere underscripts/genai-enclave, so the fixed-point doc comment is misleading and the method reads as dead code.🤖 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/genai-enclave/provenance.ts` around lines 211 - 225, The unused clearNonShadowEntries method is dead code, while its documentation claims fixed-point-loop usage. Remove clearNonShadowEntries() and its associated misleading comment, or invoke it at the appropriate point in the fixed-point iteration so prior non-shadow entries are cleared as described.
🤖 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 `@packages/cli/vitest.agentStream.config.ts`:
- Around line 7-20: Update the configuration built by mergeConfig in the default
export so the test.include and test.exclude arrays from baseConfig are replaced
rather than appended, ensuring only src/agentStream.test.tsx is selected while
retaining the intended node_modules and dist exclusions.
In `@scripts/genai-enclave/export-cjs.ts`:
- Around line 430-441: Update the non-EqualsToken branch to use
isCommonJSExports(ctx, lhs) instead of isModuleExports(ctx, lhs), ensuring
whole-target logical assignments on both module.exports and bare exports inspect
their RHS. Add a regression test for exports ||= { GeminiLeak: 1 } alongside the
existing module.exports logical-assignment coverage, and preserve equivalent
handling for ??= and &&=.
- Around line 456-494: Update dispatchStaticMethod, as used by
checkCommonJSCall, to recognize Reflect.set and route it to the existing
export-mutation checker so Reflect.set(exports, ...) violations are detected.
Keep the current defineProperty, defineProperties, and assign handling
unchanged, and do not add unrelated static methods.
In `@scripts/genai-enclave/export-detection.ts`:
- Around line 163-176: Update checkVariableStatement to inspect each variable
declaration’s initializer in addition to its binding names. When the initializer
is an object literal, recurse through its properties using the existing
object-literal checking helper so nested Gemini-named properties in exported
const/let/var declarations produce violations, while preserving the current
binding-name checks.
In `@scripts/genai-enclave/import-context.ts`:
- Around line 43-63: The isCreateRequireFactoryCallee helper only recognizes
dot-notation namespace access; add equivalent handling for a computed
ElementAccessExpression whose property text is “createRequire”. Reuse
elementAccessLiteralText to validate the bracket key, require the object
expression to be an identifier, and apply the same resolver.isNamespace check
used by the property-access branch.
In `@scripts/genai-enclave/manifest-enforcement.ts`:
- Around line 315-329: Update genaiAliasViolation so its diagnostic uses the raw
alias specifier in version without prepending npm:${target}@ again. Preserve the
existing violation detection and message context, ensuring the rendered alias
displays the original value exactly once before the prohibition text.
In `@scripts/genai-enclave/provenance-collection.ts`:
- Around line 457-475: Centralize the duplicated variable-range and
function-classification logic in provenance.ts: export one helper equivalent to
variableDeclarationRange/computeVarRange using moduleRange, hoistedRange, and
blockScopedRange, and one accessor-aware classifier equivalent to
isFunctionLikeDeclaration/isFunctionLikeNode that includes get/set accessors.
Update scripts/genai-enclave/provenance-collection.ts lines 457-475 and 568-584,
and scripts/genai-enclave/export-provenance.ts lines 117-135 and 60-76, to
import and use these shared helpers and remove the local implementations.
In `@scripts/tests/genai-enclave-adversarial.test.ts`:
- Around line 255-268: Add a valid package.json manifest inside externalDir
before calling discoverPackageWorkspaces in the symlink-outside-repo-root test.
Keep the existing symlink setup and assertion unchanged so packages/evil would
qualify by manifest but is still excluded by the symlink guard.
In `@scripts/tests/genai-enclave-guard-manifest.test.ts`:
- Around line 98-113: Update each listed manifest-violation test’s withFixture
setup, including the nested-package test and the F9 duplicate-declaration test,
to call writeRequiredManifests(write) before writing the manifest under test.
Keep each test’s targeted manifest and assertions unchanged so failures come
from the intended violation rather than missing required manifests.
In `@scripts/tests/genai-enclave-scanner-exports.test.ts`:
- Around line 27-32: Update the invalid-syntax test around parseSourceFile so it
no longer accesses the internal SourceFile.parseDiagnostics field. Use a public
TypeScript diagnostics API, or the scanner’s exposed diagnostics result if
available, while preserving the assertion that broken syntax produces at least
one diagnostic.
In `@scripts/tests/genai-enclave-scanner-regressions.test.ts`:
- Around line 191-204: Strengthen the assertion in the createRequire regression
test by asserting the entire violations result is empty with
expect(violations).toEqual([]), rather than filtering for fail-closed entries.
Keep the existing scanGeminiExports setup and test scenario unchanged.
In `@scripts/tests/publish-dependency-helpers.ts`:
- Around line 138-155: Update extractSemverRange to reuse the same
first-@-after-scope delimiter logic as extractNpmAliasTarget instead of using
lastIndexOf('@'). Ensure versionless scoped aliases such as npm:`@scope/pkg`
return null, while valid aliases extract only the version range; add coverage in
publish-dependency-helpers.test.ts for this case.
In `@scripts/tests/published-closure-regressions.test.ts`:
- Around line 366-416: Update the three resolver calls in the tests for
manifest-name mismatch, missing directories, and wrong workspaces to provide a
valid consumerWorkspace third argument. Use the temporary repository’s real
workspace directory so each test reaches its intended file: validation logic
instead of failing early due to a missing consumer workspace.
---
Nitpick comments:
In `@scripts/genai-enclave/provenance.ts`:
- Around line 211-225: The unused clearNonShadowEntries method is dead code,
while its documentation claims fixed-point-loop usage. Remove
clearNonShadowEntries() and its associated misleading comment, or invoke it at
the appropriate point in the fixed-point iteration so prior non-shadow entries
are cleared as described.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ffed0193-f6b1-4a31-aad9-d88f39696717
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lock,!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (40)
package.jsonpackages/agents/src/api/__tests__/app-service-boundary.spec.tspackages/agents/src/api/__tests__/helpers/eventHarness.tspackages/cli/src/launcher/cli-bin.test.tspackages/cli/src/ui/commands/test/subagentCommand.test.tspackages/cli/vitest.agentStream.config.tspackages/core/package.jsonpackages/tools/package.jsonscripts/check-genai-enclave.tsscripts/genai-enclave/ast-helpers.tsscripts/genai-enclave/config.tsscripts/genai-enclave/export-cjs.tsscripts/genai-enclave/export-detection.tsscripts/genai-enclave/export-provenance.tsscripts/genai-enclave/import-context.tsscripts/genai-enclave/import-detection.tsscripts/genai-enclave/manifest-enforcement.tsscripts/genai-enclave/provenance-collection.tsscripts/genai-enclave/provenance.tsscripts/genai-enclave/scanner.tsscripts/genai-enclave/violation-types.tsscripts/tests/genai-enclave-adversarial.test.tsscripts/tests/genai-enclave-guard-allowlist.test.tsscripts/tests/genai-enclave-guard-helpers.tsscripts/tests/genai-enclave-guard-manifest.test.tsscripts/tests/genai-enclave-guard.test.tsscripts/tests/genai-enclave-scanner-alias.test.tsscripts/tests/genai-enclave-scanner-bypass.test.tsscripts/tests/genai-enclave-scanner-exports.test.tsscripts/tests/genai-enclave-scanner-regressions.test.tsscripts/tests/genai-enclave-scanner.test.tsscripts/tests/genai-manifest-enforcement.test.tsscripts/tests/genai-published-root.test.tsscripts/tests/publish-dependency-helpers.test.tsscripts/tests/publish-dependency-helpers.tsscripts/tests/publish-dependency-protocols.test.tsscripts/tests/publish-integrity.test.tsscripts/tests/published-closure-regressions.test.tsscripts/tests/workspace-source-helpers.test.tsscripts/tests/workspace-source-helpers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/genai-enclave/config.ts
- scripts/tests/genai-enclave-guard-allowlist.test.ts
- scripts/tests/genai-enclave-guard-helpers.ts
# Conflicts: # packages/agents/src/core/__tests__/providerAgnosticNaming.test.ts
| IContent, | ||
| UsageStats, | ||
| } from '@vybestack/llxprt-code-core/services/history/IContent.js'; | ||
| import type { GeminiContent } from '@vybestack/llxprt-code-core/llm-types/index.js'; |
Contributor
There was a problem hiding this comment.
Refactoring looks clean and safe: you replaced the
@google/genaiContentimport with the internal neutralGeminiContenttype, andContentConverters.toIContentalready acceptsGeminiContentas input. No issues found in this diff.
Comment on lines
+293
to
+314
| function checkInlineSpreadNames( | ||
| ctx: ExportScanContext, | ||
| property: ts.SpreadAssignment, | ||
| exportForm: string, | ||
| violations: GeminiExportViolation[], | ||
| ): void { | ||
| if (isStaticSpreadSource(property)) { | ||
| const names = inlineSpreadPropertyNames(property); | ||
| for (const name of names) { | ||
| checkSinglePropertyName(ctx, property, name, exportForm, violations); | ||
| } | ||
| // Empty but inspectable — no fail-closed needed. | ||
| return; | ||
| } | ||
| // F2: non-literal spread source — cannot statically inspect properties. | ||
| addFailClosedViolation( | ||
| ctx, | ||
| property, | ||
| `${exportForm} with non-literal spread source`, | ||
| violations, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Security/DETECTION GAP:
checkInlineSpreadNamesonly checks top-level property names from inline spread sources, but does NOT recursively descend into nested object literals the waycheckObjectLiteralPropertiesdoes for regular properties. This meansexport = { ...{ nested: { GeminiDeep: 1 } } }would miss the deeply nestedGeminiDeepproperty, creating an exploitable detection gap.Suggestion: After collecting names from
inlineSpreadPropertyNames, also recurse into any nested object literal properties within the spread source. For example, iterateexpr.properties, and for each non-spread property, if its initializer unwraps to an object literal, callcheckObjectLiteralPropertieson it. Alternatively, factor out a helper likecollectNestedObjectLiteralBindingsand reuse the existing recursive logic.
Comment on lines
+105
to
+134
| export function isCreateRequireFactoryCallee( | ||
| ctx: ImportScanContext, | ||
| expr: ts.Expression, | ||
| ): boolean { | ||
| const candidate = unwrapTransparentExpressions(expr); | ||
| if (ts.isIdentifier(candidate)) { | ||
| return ctx.resolver.isFactoryAlias(candidate.text, candidate.getStart()); | ||
| } | ||
| if ( | ||
| ts.isPropertyAccessExpression(candidate) && | ||
| candidate.name.text === 'createRequire' | ||
| ) { | ||
| const namespace = unwrapTransparentExpressions(candidate.expression); | ||
| return ( | ||
| ts.isIdentifier(namespace) && | ||
| ctx.resolver.isNamespace(namespace.text, namespace.getStart()) | ||
| ); | ||
| } | ||
| if ( | ||
| ts.isElementAccessExpression(candidate) && | ||
| elementAccessLiteralText(candidate.argumentExpression) === 'createRequire' | ||
| ) { | ||
| const namespace = unwrapTransparentExpressions(candidate.expression); | ||
| return ( | ||
| ts.isIdentifier(namespace) && | ||
| ctx.resolver.isNamespace(namespace.text, namespace.getStart()) | ||
| ); | ||
| } | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Optional-chaining wrappers (
ChainExpression/PropertyAccessChain/ElementAccessChain) are not unwrapped byunwrapTransparentExpressionsand are not matched by thets.isPropertyAccessExpression/ts.isElementAccessExpressionchecks here. Expressions likem?.createRequire,obj?.['createRequire'], orcreateRequire?.()would bypassisCreateRequireFactoryCalleeandisCreateRequireHelperCallee, allowing a forbidden call to slip through the genai-enclave boundary guard. Consider unwrappingChainExpressioninunwrapTransparentExpressionsor adding explicit handling forChainExpressionin these callee checks so optional chaining does not create a detection bypass.
Comment on lines
+78
to
+79
| // Finding4: deriveAllEntryPaths now collects ALL leaf paths from every | ||
| // exports entry, not just the bun condition. |
Contributor
There was a problem hiding this comment.
The 'Finding4' comment appears to be an internal development note/tracking reference that should be removed or replaced with a descriptive comment explaining the expected behavior. Test comments should describe the behavior being verified, not reference internal findings.
Comment on lines
+651
to
+662
| const rootPackage = JSON.parse( | ||
| readFileSync(join(repoRoot, 'package.json'), 'utf-8'), | ||
| ) as RootPackageMetadata; | ||
| const packed = getPackedPaths(); | ||
|
|
||
| const packageNameToWorkspace = buildPackageNameToWorkspaceMap( | ||
| repoRoot, | ||
| rootPackage.workspaces ?? [], | ||
| ); | ||
|
|
||
| const shippedWorkspacePackagePaths = | ||
| collectShippedWorkspacePackagePaths(rootPackage); |
Contributor
There was a problem hiding this comment.
The setup logic for parsing the root package.json, obtaining packed paths, building the package-name-to-workspace map, and collecting shipped workspace paths is duplicated across three test cases. This violates DRY principles and creates maintenance risk where fixes to the setup logic must be applied in multiple places. Consider extracting this common setup into a shared helper function or a
beforeEachhook to ensure consistency and reduce boilerplate.
Comment on lines
+553
to
+556
| expect(cliPackage.dependencies?.['@google/genai']).toBeUndefined(); | ||
| expect(cliPackage.devDependencies?.['@google/genai']).toBeUndefined(); | ||
| expect(cliPackage.optionalDependencies?.['@google/genai']).toBeUndefined(); | ||
| expect(cliPackage.peerDependencies?.['@google/genai']).toBeUndefined(); |
Contributor
There was a problem hiding this comment.
The package name '@google/genai' is hardcoded in four separate assertions. As a business-specific dependency identifier, this should be extracted to a named constant (e.g.,
GENAI_PACKAGE_NAME) to improve maintainability and reduce the risk of inconsistent updates if the dependency changes.
|
|
||
| it( | ||
| 'ships source for every internal workspace dependency of shipped workspaces', | ||
| { timeout: 60000 }, |
Contributor
There was a problem hiding this comment.
The timeout value
60000is repeated three times as a magic number. Extract this to a named constant (e.g.,LONG_TEST_TIMEOUT) at the file or describe-block level to improve readability and make threshold adjustments easier if performance characteristics change.
Comment on lines
+641
to
+647
| let current = queue.shift(); | ||
| while (current !== undefined) { | ||
| if (visited.has(current)) { | ||
| current = queue.shift(); | ||
| continue; | ||
| } | ||
| visited.add(current); |
Contributor
There was a problem hiding this comment.
The BFS traversal uses
queue.shift()inside a loop, which is O(n) per operation. For transitive closure checks over packages with large import graphs, this makes the overall traversal O(n²) and can cause test timeouts. Replace the array-as-queue pattern with an index pointer (let head = 0; ... const current = queue[head++];) to achieve O(1) dequeue operations.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/genai-enclave/helper-return-analysis.ts`:
- Around line 40-92: Extend statementReturnFlow to explicitly analyze
TryStatement and SwitchStatement instead of treating them as unsupported
fallthroughs, preserving accurate allValueReturnsProven, canFallThrough, and
hasValueReturn results across try/catch/finally and switch clauses. Add
adversarial positive and negative tests covering proven returns, throwing
branches, fallthrough, and loader-detection provenance such as getReq.
In `@scripts/genai-enclave/import-detection.ts`:
- Around line 216-236: Extend the callee checks in the `.call`/`.apply` and
nested `.bind` handling around isCreateRequireFactoryCallee to also recognize
helper-returning loaders, using the existing helper-detection symbol and
preserving current createRequire behavior. Add a regression case in
genai-enclave-scanner-regressions.test.ts covering helperFn.call(null,
url)('`@google/genai`') and equivalent supported loader invocation.
In `@scripts/genai-enclave/scanner.ts`:
- Around line 58-68: Update the transpilation setup around ts.transpileModule to
pass the actual filePath as fileName instead of selecting synthetic
diagnostics.ts or diagnostics.tsx names, while preserving the existing compiler
options and diagnostics behavior. Ensure filePath is available through the
scanner flow, and add regressions covering .js, .cjs, .mjs, and .json inputs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bd6b01c4-501d-4679-98b5-0ad419627cdf
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lock,!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (25)
.llxprt/LLXPRT.mdpackages/cli/vitest.agentStream.config.tsscripts/check-genai-enclave.tsscripts/genai-enclave/ast-helpers.tsscripts/genai-enclave/export-cjs.tsscripts/genai-enclave/export-detection.tsscripts/genai-enclave/export-provenance.tsscripts/genai-enclave/helper-return-analysis.tsscripts/genai-enclave/import-context.tsscripts/genai-enclave/import-detection.tsscripts/genai-enclave/manifest-enforcement.tsscripts/genai-enclave/provenance-collection.tsscripts/genai-enclave/provenance.tsscripts/genai-enclave/scanner.tsscripts/tests/genai-enclave-adversarial.test.tsscripts/tests/genai-enclave-guard-allowlist.test.tsscripts/tests/genai-enclave-guard-manifest.test.tsscripts/tests/genai-enclave-review-regressions.test.tsscripts/tests/genai-enclave-scanner-exports.test.tsscripts/tests/genai-enclave-scanner-regressions.test.tsscripts/tests/publish-dependency-helpers.test.tsscripts/tests/publish-dependency-helpers.tsscripts/tests/published-closure-regressions.test.tsscripts/tests/workspace-source-helpers.tstsconfig.scripts.json
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/cli/vitest.agentStream.config.ts
- scripts/genai-enclave/export-provenance.ts
- .llxprt/LLXPRT.md
- scripts/tests/genai-enclave-guard-allowlist.test.ts
- scripts/tests/published-closure-regressions.test.ts
- scripts/genai-enclave/manifest-enforcement.ts
- scripts/tests/publish-dependency-helpers.test.ts
- scripts/tests/genai-enclave-guard-manifest.test.ts
- scripts/tests/genai-enclave-adversarial.test.ts
- scripts/genai-enclave/export-cjs.ts
- scripts/check-genai-enclave.ts
- scripts/tests/workspace-source-helpers.ts
- scripts/tests/publish-dependency-helpers.ts
Comment on lines
+40
to
+92
| function statementReturnFlow( | ||
| statement: ts.Statement, | ||
| proveExpression: ReturnExpressionProof, | ||
| ): ReturnFlow { | ||
| if (ts.isReturnStatement(statement)) { | ||
| return { | ||
| allValueReturnsProven: | ||
| statement.expression !== undefined && | ||
| proveExpression(statement.expression), | ||
| canFallThrough: false, | ||
| hasValueReturn: statement.expression !== undefined, | ||
| }; | ||
| } | ||
| if (ts.isThrowStatement(statement)) { | ||
| return { | ||
| allValueReturnsProven: true, | ||
| canFallThrough: false, | ||
| hasValueReturn: false, | ||
| }; | ||
| } | ||
| if (ts.isBlock(statement)) { | ||
| return sequenceReturnFlow(statement.statements, proveExpression); | ||
| } | ||
| if (ts.isIfStatement(statement)) { | ||
| const whenTrue = statementReturnFlow( | ||
| statement.thenStatement, | ||
| proveExpression, | ||
| ); | ||
| const whenFalse = | ||
| statement.elseStatement === undefined | ||
| ? FALLTHROUGH | ||
| : statementReturnFlow(statement.elseStatement, proveExpression); | ||
| return { | ||
| allValueReturnsProven: | ||
| whenTrue.allValueReturnsProven && whenFalse.allValueReturnsProven, | ||
| canFallThrough: whenTrue.canFallThrough || whenFalse.canFallThrough, | ||
| hasValueReturn: whenTrue.hasValueReturn || whenFalse.hasValueReturn, | ||
| }; | ||
| } | ||
| const isNeutralDeclaration = | ||
| ts.isVariableStatement(statement) || | ||
| ts.isFunctionDeclaration(statement) || | ||
| ts.isClassDeclaration(statement); | ||
| const isNeutralStatement = | ||
| ts.isExpressionStatement(statement) || | ||
| ts.isEmptyStatement(statement) || | ||
| ts.isDebuggerStatement(statement); | ||
| if (isNeutralDeclaration || isNeutralStatement) return FALLTHROUGH; | ||
| return { | ||
| allValueReturnsProven: false, | ||
| canFallThrough: true, | ||
| hasValueReturn: false, | ||
| }; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Handle try and switch return flow before relying on this proof for provenance.
Unsupported statements return an unproven fallthrough result. Consequently, helpers such as function getReq() { try { return createRequire(url); } catch { throw err; } } are never registered, and subsequent getReq()('@google/genai') calls can bypass loader detection.
Extend the flow analysis for at least TryStatement and SwitchStatement, with adversarial positive and negative tests. This is required by the PR’s fail-closed boundary objective.
🤖 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/genai-enclave/helper-return-analysis.ts` around lines 40 - 92, Extend
statementReturnFlow to explicitly analyze TryStatement and SwitchStatement
instead of treating them as unsupported fallthroughs, preserving accurate
allValueReturnsProven, canFallThrough, and hasValueReturn results across
try/catch/finally and switch clauses. Add adversarial positive and negative
tests covering proven returns, throwing branches, fallthrough, and
loader-detection provenance such as getReq.
Comment on lines
+58
to
+68
| const transpilation = ts.transpileModule(sourceText, { | ||
| compilerOptions: { | ||
| jsx: ts.JsxEmit.ReactJSX, | ||
| module: ts.ModuleKind.ESNext, | ||
| target: ts.ScriptTarget.Latest, | ||
| }, | ||
| fileName: | ||
| scriptKind === ts.ScriptKind.TSX || scriptKind === ts.ScriptKind.JSX | ||
| ? 'diagnostics.tsx' | ||
| : 'diagnostics.ts', | ||
| reportDiagnostics: true, |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 \
"getParseDiagnostics|broken\\.(js|cjs|jsx|json)|parse diagnostics" \
scripts/tests package.jsonRepository: vybestack/llxprt-code
Length of output: 1938
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## scanner.ts\n'
ast-grep outline scripts/genai-enclave/scanner.ts --view expanded || true
printf '\n## relevant slices\n'
sed -n '1,220p' scripts/genai-enclave/scanner.ts | cat -n
printf '\n## scanner tests\n'
sed -n '1,220p' scripts/tests/genai-enclave-scanner-exports.test.ts | cat -nRepository: vybestack/llxprt-code
Length of output: 13377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## guard manifest diagnostics test slice\n'
sed -n '460,540p' scripts/tests/genai-enclave-guard-manifest.test.ts | cat -nRepository: vybestack/llxprt-code
Length of output: 3953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## scanner.ts\n'
ast-grep outline scripts/genai-enclave/scanner.ts --view expanded || true
printf '\n## relevant slices\n'
sed -n '1,220p' scripts/genai-enclave/scanner.ts | cat -n
printf '\n## scanner tests\n'
sed -n '1,220p' scripts/tests/genai-enclave-scanner-exports.test.ts | cat -n
printf '\n## guard manifest diagnostics test slice\n'
sed -n '460,540p' scripts/tests/genai-enclave-guard-manifest.test.ts | cat -nRepository: vybestack/llxprt-code
Length of output: 17171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
let ts;
try {
ts = require('typescript');
} catch (e) {
console.log('typescript not available');
process.exit(0);
}
const samples = [
['js-valid', 'const x = 1;\n', 'sample.js'],
['js-invalid-ts-only', 'const x: number = 1;\n', 'sample.js'],
['json-valid', '{ "a": 1 }\n', 'sample.json'],
['json-invalid', '{ a: 1 }\n', 'sample.json'],
['jsx-valid', 'const x = <div />;\n', 'sample.jsx'],
];
for (const [label, text, fileName] of samples) {
const out = ts.transpileModule(text, {
compilerOptions: {
jsx: ts.JsxEmit.ReactJSX,
module: ts.ModuleKind.ESNext,
target: ts.ScriptTarget.Latest,
},
fileName,
reportDiagnostics: true,
});
console.log(`--- ${label} / ${fileName} ---`);
console.log('diagnostics:', out.diagnostics?.length ?? 0);
for (const d of out.diagnostics ?? []) {
console.log(ts.flattenDiagnosticMessageText(d.messageText, '\n'));
}
}
JSRepository: vybestack/llxprt-code
Length of output: 802
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 "getParseDiagnostics\\(|parse diagnostics|fail-closed|broken-syntax" scripts/genai-enclave scripts/testsRepository: vybestack/llxprt-code
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 "getParseDiagnostics\\(" scriptsRepository: vybestack/llxprt-code
Length of output: 1839
🌐 Web query:
TypeScript transpileModule fileName script kind diagnostics filename parsing behavior
💡 Result:
In TypeScript's Compiler API, the transpileModule function provides a fast, single-file transpilation path that bypasses the full type-checking process [1][2]. Its behavior regarding file parsing, diagnostics, and script identification is governed by the provided TranspileOptions and the input file name [3][4]. ### Parsing and Script Kind transpileModule does not have an explicit scriptKind parameter [3][4]. Instead, it infers the ScriptKind (e.g., JS, JSX, TS, TSX) from the fileName option provided in the TranspileOptions [3][4]. If the fileName is not provided, it defaults to module.ts, or module.tsx if the compiler options specify jsx [3][4]. Because transpileModule is designed to be a standalone, file-system-agnostic utility, it does not perform typical file system lookups (such as checking package.json for "type": "module"), which can lead to unexpected module output formats if not manually configured [5][6]. ### Diagnostics Behavior The reportDiagnostics option in TranspileOptions controls whether syntactic diagnostics are collected [3][4]. * When set to true, transpileModule retrieves and includes syntactic diagnostics in the returned TranspileOutput object [3][4]. * It does not perform type checking; therefore, it will not report type-level errors, regardless of the reportDiagnostics setting [7][1]. * If syntax is severely malformed, diagnostics regarding the parsing errors will be captured [7]. In summary, transpileModule is intended for fast, isolated transformations. For scenarios requiring accurate file-system context (like module resolution or package.json awareness) or full type checking, using the standard createProgram API is recommended [1][6].
Citations:
- 1: https://microsoft-typescript.mintlify.app/api/program
- 2: Do not check in the
transpileModuleAPI microsoft/TypeScript#50699 - 3: https://github.com/microsoft/TypeScript/blob/4fe27222ca2d012dad541b43d791ae12f1cf6985/src/services/transpile.ts
- 4: https://github.com/Microsoft/TypeScript/blob/0a041ee53641ea1916a9f15ad9cd57368d1dc1af/src/services/transpile.ts
- 5:
transpileModuledoes not account for package.jsontypewhen ran with node16/nodenext module setting microsoft/TypeScript#53022 - 6: ts.transpileModule outputs commonjs with "module": "nodenext" and package.json "type": "module" microsoft/TypeScript#62734
- 7: Compiler API
transpileModuledoes not emit diagnostics ever - empty array microsoft/TypeScript#51962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## check-genai-enclave.ts slice\n'
sed -n '470,540p' scripts/check-genai-enclave.ts | cat -n
printf '\n## parseSourceFile call sites\n'
rg -n -C2 "parseSourceFile\\(" scripts/check-genai-enclave.ts scripts/tests scripts/genai-enclaveRepository: vybestack/llxprt-code
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
let mod;
try {
mod = require('typescript');
} catch (e) {
console.log('typescript: unavailable');
process.exit(0);
}
const ts = mod.default ?? mod;
console.log('version:', ts.version ?? mod.version ?? 'unknown');
console.log('has JsxEmit:', !!ts.JsxEmit, 'has default.JsxEmit:', !!mod.default?.JsxEmit);
const samples = [
['js-valid', 'const x = 1;\n', 'sample.js'],
['js-invalid-ts-only', 'const x: number = 1;\n', 'sample.js'],
['json-valid', '{ "a": 1 }\n', 'sample.json'],
['json-invalid', '{ a: 1 }\n', 'sample.json'],
['tsx-valid', 'const x = <div />;\n', 'sample.tsx'],
['jsx-valid', 'const x = <div />;\n', 'sample.jsx'],
];
for (const [label, text, fileName] of samples) {
const opts = {
compilerOptions: {
module: ts.ModuleKind?.ESNext ?? 99,
target: ts.ScriptTarget?.Latest ?? ts.ScriptTarget?.ES2020 ?? 99,
jsx: ts.JsxEmit?.ReactJSX,
},
fileName,
reportDiagnostics: true,
};
const out = ts.transpileModule(text, opts);
console.log(`--- ${label} / ${fileName} ---`);
console.log('diagnostics:', out.diagnostics?.length ?? 0);
for (const d of out.diagnostics ?? []) {
console.log('-', ts.flattenDiagnosticMessageText(d.messageText, '\n'));
}
}
JSRepository: vybestack/llxprt-code
Length of output: 852
Use the real file name for diagnostics (scripts/genai-enclave/scanner.ts:58-68). transpileModule is still parsing synthetic .ts/.tsx files here, so .js/.cjs/.mjs/.json inputs can be checked under the wrong grammar. Pass filePath through and add regressions for those extensions.
🤖 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/genai-enclave/scanner.ts` around lines 58 - 68, Update the
transpilation setup around ts.transpileModule to pass the actual filePath as
fileName instead of selecting synthetic diagnostics.ts or diagnostics.tsx names,
while preserving the existing compiler options and diagnostics behavior. Ensure
filePath is available through the scanner flow, and add regressions covering
.js, .cjs, .mjs, and .json inputs.
Comment on lines
+93
to
+95
| const mod: Record<string, unknown> = await import( | ||
| '@vybestack/llxprt-code-agents/app-service.js' | ||
| ); |
Contributor
There was a problem hiding this comment.
The dynamic import hardcodes the app-service specifier string instead of using the already-defined
APP_SERVICE_SPECIFIERconstant. This creates a maintenance risk: if the specifier ever changes, this literal could be missed during updates. Please reuseAPP_SERVICE_SPECIFIERfor consistency.
Comment on lines
+128
to
+130
| if (entry.isDirectory()) { | ||
| if (PRUNE_DIRS.has(entry.name)) continue; | ||
| walk(full); |
Contributor
There was a problem hiding this comment.
The
walkPackagesfunction recursively traverses directories usingreaddirSyncwithout tracking visited inodes or filtering symlinks. WhenGENAI_ENCLAVE_ROOTis set (test mode), a circular symlink in the fixture tree would cause infinite recursion and stack overflow, crashing the process before it can report violations or complete the test. Consider either (a) skipping symlinked directories with!entry.isSymbolicLink(), or (b) tracking visited real paths usingfs.realpathSyncto detect cycles.
| @@ -0,0 +1,599 @@ | |||
| #!/usr/bin/env bun | |||
Contributor
There was a problem hiding this comment.
The shebang
#!/usr/bin/env bunsuggests the script requires Bun, but all module imports use Node.jsnode:prefixed specifiers (e.g.,node:fs,node:path,node:child_process). This mismatch could confuse developers about the required runtime and prevent direct execution withnodeif Bun is not installed. Change the shebang to#!/usr/bin/env nodeto match the actual dependencies, or ensure the project standard runtime is Bun if that is intentional.
Comment on lines
+379
to
+381
| } catch (e) { | ||
| const err = e as { code?: string }; | ||
| if (err.code === 'ENOENT' && dir === PACKAGES_DIR) { |
Contributor
There was a problem hiding this comment.
In
discoverManifests, the catch block assertse as { code?: string }without first checkinge instanceof Error. While accessing.codeon a non-object yieldsundefined(preventing crashes), the unsound type assertion bypasses TypeScript's type safety and could mask the root cause if a non-Error value is thrown. Use a type guard or at minimum checke instanceof Errorbefore the assertion, consistent with the safer pattern already used incheckManifest.
| * shown. The root declaration is a packaging bridge for the root artifact; | ||
| * source imports remain confined to the implementation enclaves. | ||
| */ | ||
| export const SANCTIONED_GENAI_VERSION = '1.30.0'; |
Contributor
There was a problem hiding this comment.
The version
'1.30.0'is duplicated acrosspackage.json(root, packages/core, packages/providers) and this config file. A version bump now requires a manual, four-place update, which is error-prone. Consider extracting the version to a single source of truth (e.g., a shared constant/version file) and referencing it from both package.json files and this guard config so they stay in sync automatically.
| } | ||
|
|
||
| describe('deriveAllEntryPaths', () => { | ||
| it('extracts main entry from exports bun condition', () => { |
Contributor
There was a problem hiding this comment.
Test name says 'extracts main entry from exports bun condition' but the current implementation collects ALL leaf paths from every exports entry, not just the bun condition. Rename the test to match the actual behavior being verified.
Comment on lines
+83
to
+87
| export type ProtocolTargetResolver = ( | ||
| depName: string, | ||
| specifier: string, | ||
| consumerWorkspace?: string, | ||
| ) => ProtocolTargetResolution; |
Contributor
There was a problem hiding this comment.
ProtocolTargetResolverreturnsProtocolTargetResolution, which does not include thereasonfield fromRealProtocolResolution. When a protocol specifier fails validation,checkProtocolTargetemits a generic message like 'unresolved protocol specifier (target does not match the workspace name→directory map)' and discards the specific cause (missing path, canonical mismatch, manifest name mismatch, etc.). Consider enrichingProtocolTargetResolutionwithreason?: stringand propagating it into the mismatch diagnostic so failures are actionable.
Comment on lines
+90
to
+96
| export function isProtocolSpecifier(version: string): boolean { | ||
| return ( | ||
| version.startsWith('file:') || | ||
| version.startsWith('workspace:') || | ||
| version.startsWith('link:') | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
isProtocolSpecifieronly recognizesfile:,workspace:, andlink:. Non-registry specifiers such as git URLs (git+https://...), tarball URLs (https://.../pkg.tgz), and plainhttp/httpsURLs fall through toextractSemverRangeand thenisRootSubsetOfWorkspace, wheresemver.subsetthrows and produces a misleading 'root range is not a subset of workspace range' error. Add explicit rejection for these non-registry protocols with a clear diagnostic, or normalize them into a supported category before semver checks.
Comment on lines
+315
to
+318
| // Cycle check: resolve the real path and verify it doesn't loop back | ||
| // to a path we've already resolved (defensive — realpathSync collapses | ||
| // cycles at the OS level, but a circular chain of symlinks could still | ||
| // exist in edge cases). |
Contributor
There was a problem hiding this comment.
Misleading comment: The comment on line 439 claims the function rejects symlinks that 'revisit already-seen paths' for cycle detection, but the implementation does not track previously seen paths at all. It only checks that the resolved path is within the repo root and is a directory. While
realpathSynccollapses cycles at the OS level, the comment overstates the cycle detection capability and could lead to false confidence. Either update the comment to accurately reflect the implementation, or add actual cycle tracking if stronger guarantees are needed.
Comment on lines
+369
to
+371
| function extractExportEntryPath( | ||
| entry: ExportsEntry | undefined, | ||
| ): string[] | undefined { |
Contributor
There was a problem hiding this comment.
Naming inconsistency:
extractExportEntryPath(line 369) returnsstring[] | undefined(multiple paths), but its singular name implies a single path. This violates the principle of least surprise. The companion functioncollectExportEntryPathscorrectly uses plural naming. Rename this function toextractExportEntryPathsto match its actual return type and avoid confusion for future maintainers.
Comment on lines
+63
to
+92
| if (ts.isIfStatement(statement)) { | ||
| const whenTrue = statementReturnFlow( | ||
| statement.thenStatement, | ||
| proveExpression, | ||
| ); | ||
| const whenFalse = | ||
| statement.elseStatement === undefined | ||
| ? FALLTHROUGH | ||
| : statementReturnFlow(statement.elseStatement, proveExpression); | ||
| return { | ||
| allValueReturnsProven: | ||
| whenTrue.allValueReturnsProven && whenFalse.allValueReturnsProven, | ||
| canFallThrough: whenTrue.canFallThrough || whenFalse.canFallThrough, | ||
| hasValueReturn: whenTrue.hasValueReturn || whenFalse.hasValueReturn, | ||
| }; | ||
| } | ||
| const isNeutralDeclaration = | ||
| ts.isVariableStatement(statement) || | ||
| ts.isFunctionDeclaration(statement) || | ||
| ts.isClassDeclaration(statement); | ||
| const isNeutralStatement = | ||
| ts.isExpressionStatement(statement) || | ||
| ts.isEmptyStatement(statement) || | ||
| ts.isDebuggerStatement(statement); | ||
| if (isNeutralDeclaration || isNeutralStatement) return FALLTHROUGH; | ||
| return { | ||
| allValueReturnsProven: false, | ||
| canFallThrough: true, | ||
| hasValueReturn: false, | ||
| }; |
Contributor
There was a problem hiding this comment.
The
statementReturnFlowfunction is missing handlers for common control flow statements: loops (WhileStatement,DoStatement,ForStatement,ForInStatement,ForOfStatement),SwitchStatement,TryStatement/CatchClause, and control transfer statements (BreakStatement,ContinueStatement,LabeledStatement). Any function containing these statements will be conservatively reported withcanFallThrough: trueandallValueReturnsProven: false, causing false positives. For example:
- A
switchwithreturnin every case anddefaultwould be flagged as fallthrough-able.- A
try/catchwhere both blocks return would be flagged as not proven.- A loop with an unconditional
returninside would also be flagged.This reduces the accuracy of the genai-enclave analysis and may incorrectly reject legitimate code. Consider adding recursive handlers for these statements, treating loops as fallthrough-able but analyzing their bodies for returns, handling
switch/try/catchby analyzing all branches, and treatingbreak/continue/labeledas flow-altering rather than as the current default fallback.
Comment on lines
+435
to
+436
| '@google/genai imports are only allowed in packages/providers/src/gemini/** ' + | ||
| 'and packages/core/src/code_assist/**' |
Contributor
There was a problem hiding this comment.
MAINTENABILITY: These enclave paths are hardcoded here but are also the source of truth in
scripts/genai-enclave/config.ts(GENAI_IMPORT_ENCLAVES). If the enclaves change, these messages will become stale. Reuse the configured prefixes to keep error messages in sync with the actual guard logic.
Comment on lines
+46
to
+72
| export function parseSourceFile( | ||
| filePath: string, | ||
| sourceText: string, | ||
| ): ts.SourceFile { | ||
| const scriptKind = inferScriptKind(filePath); | ||
| const sourceFile = ts.createSourceFile( | ||
| filePath, | ||
| sourceText, | ||
| ts.ScriptTarget.Latest, | ||
| true, | ||
| scriptKind, | ||
| ); | ||
| const transpilation = ts.transpileModule(sourceText, { | ||
| compilerOptions: { | ||
| jsx: ts.JsxEmit.ReactJSX, | ||
| module: ts.ModuleKind.ESNext, | ||
| target: ts.ScriptTarget.Latest, | ||
| }, | ||
| fileName: | ||
| scriptKind === ts.ScriptKind.TSX || scriptKind === ts.ScriptKind.JSX | ||
| ? 'diagnostics.tsx' | ||
| : 'diagnostics.ts', | ||
| reportDiagnostics: true, | ||
| }); | ||
| parseDiagnostics.set(sourceFile, transpilation.diagnostics ?? []); | ||
| return sourceFile; | ||
| } |
Contributor
There was a problem hiding this comment.
parseSourceFilelacks error handling aroundts.createSourceFileandts.transpileModule. If TypeScript hits a fatal parsing/transpilation error (malformed input, internal TS failure), the exception will bubble up unhandled. In a repo-wide scanner likecheck-genai-enclave.ts, a single bad file would abort the entire integrity check.Please add a
try/catchhere and either wrap the error in a customParseErroror at minimum document that this function can throw. If you keep it throwing, consider whether callers should be updated to handle it.
Comment on lines
+160
to
+191
| function checkNamedExports( | ||
| ctx: ExportScanContext, | ||
| node: ts.ExportDeclaration, | ||
| violations: GeminiExportViolation[], | ||
| ): void { | ||
| const clause = node.exportClause; | ||
| if (clause === undefined) return; | ||
| if (ts.isNamespaceExport(clause)) { | ||
| if (containsGemini(clause.name.text)) { | ||
| addExportViolation( | ||
| ctx, | ||
| node, | ||
| clause.name.text, | ||
| 'export * as name', | ||
| violations, | ||
| ); | ||
| } | ||
| return; | ||
| } | ||
| for (const element of clause.elements) { | ||
| const exportedName = element.name.text; | ||
| if (containsGemini(exportedName)) { | ||
| addExportViolation( | ||
| ctx, | ||
| node, | ||
| exportedName, | ||
| 'export { name }', | ||
| violations, | ||
| ); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Re-export aliases bypass the boundary guard:
checkNamedExportsonly inspects the exported alias (element.name.text), not the local identifier being re-exported (element.propertyName?.text). This meansexport { GeminiThing as SafeName }will NOT be flagged, allowing a locally-defined Gemini-named symbol to escape the enclave under a safe alias.In the TypeScript AST, an
ExportSpecifierforexport { Foo as Bar }has:
propertyName=Foo(the local binding)name=Bar(the exported alias)To fail-closed on this bypass, also check the local
propertyNamewhen it differs fromname:const localName = element.propertyName?.text; const exportedName = element.name.text; const checkName = localName ?? exportedName; if (containsGemini(checkName)) { addExportViolation(ctx, node, checkName, 'export { name }', violations); }(Keep the existing
exportedNamecheck as well, sinceexport { GeminiBar }must still be flagged.)
Comment on lines
+47
to
+54
| if (ts.isVariableStatement(node)) { | ||
| for (const decl of node.declarationList.declarations) { | ||
| registerVarShadows(ctx, decl); | ||
| } | ||
| } else if (isFunctionLikeNode(node)) { | ||
| registerFunctionShadows(ctx, node); | ||
| } | ||
| ts.forEachChild(node, (child) => collectExportShadows(ctx, child)); |
Contributor
There was a problem hiding this comment.
Missing
VariableDeclarationhandling infor-loop initializers andcatchclauses. TypeScript represents variable declarations infor/for-in/for-ofinitializers andcatchclauses as bareVariableDeclarationnodes (not wrapped inVariableStatement), so these shadows are never registered. Add ats.isVariableDeclaration(node)branch (and similarly forcollectObjectLiteralBindings) to cover these non-statement contexts.
Comment on lines
+348
to
+349
| describe('Finding6: createRealProtocolResolver — canonical path + target manifest', () => { | ||
| it('resolves a file: protocol to the canonical workspace directory and verifies the target manifest name', () => { |
Contributor
There was a problem hiding this comment.
Protocol resolver tests lack explicit path-traversal edge cases. Add cases like
file:../../etc/passwdand absolute paths to document that the workspace name-to-dir mapping blocks traversal to arbitrary locations outside the repo, rather than relying on implicit mismatch behavior.
Comment on lines
+169
to
+178
| export function isRootSubsetOfWorkspace( | ||
| rootRange: string, | ||
| workspaceRange: string, | ||
| ): boolean { | ||
| try { | ||
| return semver.subset(rootRange, workspaceRange); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
semver.subsetis called with two string arguments, but the standardsemverAPI expects an array of ranges as the first argument and a target range as the second. Passing two strings causes the subset check to malfunction (it will either throw or evaluate character-wise), which breaks the core invariant that root ranges must be subsets of workspace ranges. Usesemver.subset([rootRange], workspaceRange)instead, and verify the test expectations match the intended semantics (root^3.0.2is broader than workspace^3.0.1, so it should be rejected).
Comment on lines
+22
to
+24
| * - **npm aliases** (`npm:realName@version`): the version is extracted from | ||
| * after the last `@`, the alias target is compared, and the semver range | ||
| * is checked for subset. |
Contributor
There was a problem hiding this comment.
The module-level JSDoc comment states that the version is extracted 'from after the last
@', butextractNpmAliasTargetcorrectly usesindexOf('@')(first@). This documentation mismatch could mislead future maintainers into changing the correct implementation to match the incorrect comment, breaking tarball URL handling likenpm:name@https://user@host/pkg.tgz.
Comment on lines
+403
to
+411
| export function checkDependencyCoverage( | ||
| workspace: string, | ||
| depName: string, | ||
| workspaceVersion: string, | ||
| kind: DependencyKind, | ||
| root: ManifestDependencies, | ||
| internalPackages?: ReadonlySet<string>, | ||
| protocolResolver?: ProtocolTargetResolver, | ||
| ): DependencyMismatch | null { |
Contributor
There was a problem hiding this comment.
The
workspaceparameter is ambiguously named. It represents a directory path relative to repoRoot (e.g.packages/core), not a package name, but it is passed asconsumerWorkspacetoProtocolTargetResolverand used in path resolution. Consider renaming toconsumerWorkspaceDirorworkspaceDirto prevent callers from mistakenly passing a package name.
Comment on lines
+509
to
+515
| export function checkWorkspaceDependencies( | ||
| workspace: string, | ||
| workspaceManifest: ManifestDependencies, | ||
| root: ManifestDependencies, | ||
| internalPackages?: ReadonlySet<string>, | ||
| protocolResolver?: ProtocolTargetResolver, | ||
| ): DependencyMismatch[] { |
Contributor
There was a problem hiding this comment.
Same ambiguity here:
workspaceis passed through tocheckDependencyCoverageas a directory path forconsumerWorkspace, not a package name. Rename toworkspaceDirorconsumerWorkspaceDirfor clarity.
This was referenced Jul 15, 2026
acoliver
added a commit
that referenced
this pull request
Jul 15, 2026
* Make folder trust updates live (Fixes #637) * Harden live trust cleanup and review coverage * Resolve live trust review findings * Tighten trust transition race coverage * Surface live trust transition failures * Format merged MCP manager tests * Fix slash command trust event test mock * Harden trust lifecycle review findings * Complete trust lifecycle error handling * Contain live trust transition failures * Keep trust rollback and cleanup consistent * Extract live trust transition lifecycle * Close live trust authorization races * Harden final live trust lifecycle edges * Preserve complete trust failure reporting * Lower DEFAULT_TOKEN_LIMIT to 200K, add GLM catalog, consolidate resolution (Fixes #2527) (#2565) * Lower DEFAULT_TOKEN_LIMIT to 200K, add GLM catalog, consolidate resolution (Fixes #2527) Lower DEFAULT_TOKEN_LIMIT from 1,048,576 to 200,000 so unrecognized models get a fail-safe conservative context window instead of a dangerous 1M fallback that silently overruns sub-200K backends (#2270). Add GLM model family to the token-limit catalog: glm-5.x prefix -> 200K, glm-4.x prefix -> 128K, with exact entries for glm-5.2, glm-5.1, glm-5, glm-4.6, glm-4.5, and glm-4 (#2280). Add Gemini prefix entries (gemini-2.5-pro, gemini-2.5-flash, gemini-2.0-flash -> 1M) to prevent regressions from the lowered default for models that previously fell through to DEFAULT_TOKEN_LIMIT. Consolidate the two duplicated context-window resolution paths into a single shared resolveEffectiveContextLimit() function in tokenLimits.ts. Both getTokenLimitForConfiguredContext (agents layer) and ephemerals.contextLimit() (core runtime) now delegate to it, ensuring all consumers agree by construction (#2270 DRY item). * fix: replace nested ternary with if/else in test mocks (sonarjs/no-nested-conditional) * Close remaining live trust safety gaps * Raise unconfigured subagent turn fallback to 1000 (Fixes #2541) (#2566) * fix(agents): raise unconfigured subagent turn fallback * fix(agents): preserve foreground turn validation * test(agents): keep profile loader observable * Remove inherited Gemini names from neutral APIs (#2537) * Close final live trust lifecycle races * Harden live trust transition boundaries * Finish provider-neutral Gemini cleanup (#2573) (#2574) * Resolve final c90 review findings * Resolve latest a4 review findings * Resolve latest OCR review findings * Keep trust failure timers test-local * Resolve trust and MCP lifecycle review findings * Preserve provider quota errors across retry timeouts (Fixes #2559) (#2568) * Preserve provider quota errors across retry timeouts (Fixes #2559) * Use the public core error contract * Avoid idle-timeout cleanup deadlocks * Harden retry cleanup and request isolation * Strengthen retry contracts and behavioral coverage * Clarify timeout and structured error contracts * Bound load-balancer retries to request lifetimes * Stop cancelled failovers from mutating sessions * Use the public structured error contract * Close remaining retry lifecycle gaps * Expose retry orchestration through the provider API * Preserve failover and observation request boundaries * Preserve classifications at shared retry exhaustion * Harden trust and MCP review edge cases * Enforce the GenAI enclave boundary (Fixes #2352) (#2546) * Enforce the GenAI enclave boundary (Fixes #2352) * Preserve the published GenAI runtime bridge * Close remaining GenAI boundary bypasses * Tighten GenAI guard regression coverage * Harden GenAI guard diagnostics * Clarify GenAI packaging test types * Trim GenAI guard helper surface * Validate every GenAI dependency section * Close GenAI enclave scanner gaps * Keep boundary-test imports statically inspectable * Harden GenAI boundary against indirect bypasses * Resolve GenAI boundary review gaps * Close remaining GenAI boundary bypasses * Scope neutral GeminiContent naming exception * Close final GenAI scanner review gaps * Close provenance-based scanner bypasses * Preserve fake provider compatibility allowlist * Align GenAI guard with provider-neutral cleanup * Harden trust UI async lifecycle handling * Prevent stale trust commit UI updates * Close trust and MCP review races * Complete MCP cleanup after status failures * Reject unresolved directory trust decisions * Refresh genai import inventory
This was referenced Jul 19, 2026
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
1 participant
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.
Summary
Boundary behavior
Allowed SDK locations:
Allowed dependency declarations:
Everything else fails closed, including unreadable files, malformed manifests, parse diagnostics, incomplete discovery, computed module specifiers outside enclaves, and stale exact allowlist entries.
Test-first evidence
Behavioral negative coverage verifies that the guard rejects:
Positive coverage verifies both enclaves independently and validates the real repository.
Verification
Fixes #2352
Summary by CodeRabbit
mime-typesand added an optional@napi-rs/keyringdependency.