-
Notifications
You must be signed in to change notification settings - Fork 3
feat: expose oomol token #392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary by CodeRabbit发布说明
Walkthrough在 Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as 调用方
participant ContextImpl as ContextImpl
participant Env as 环境 (process.env)
Caller->>ContextImpl: await getOomolToken()
ContextImpl->>Env: read OOMOL_TOKEN
alt token 存在
Env-->>ContextImpl: 返回 token
ContextImpl-->>Caller: 返回 token
else token 为空
ContextImpl->>ContextImpl: 检查 `#tokenWarningShown`
alt 未显示过警告
ContextImpl->>ContextImpl: 调用 warning()
ContextImpl-->>Caller: 返回 ""
else 已显示警告
ContextImpl-->>Caller: 返回 ""
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/oocana-sdk/src/context.ts (2)
115-117: 建议添加 JSDoc 文档。为了与代码库中其他公共方法保持一致,建议为
getOomolToken()方法添加 JSDoc 注释,说明其用途、返回值以及使用场景。参考现有方法的文档风格,可以添加如下注释:
+ /** + * Get OOMOL authentication token from environment. + * @returns OOMOL_TOKEN environment variable value, or empty string if not set. + */ async getOomolToken(): Promise<string> { return process.env["OOMOL_TOKEN"] || ""; }
115-117: 考虑与 OOMOL_LLM_ENV 模式保持一致(可选)。
OOMOL_LLM_ENV在环境变量为空时会显示警告(第 129-138 行)。您可以考虑为getOomolToken()添加类似的警告机制,以便在令牌未设置时通知用户,这可能会影响某些功能的正常运行。如果需要添加警告,可以参考以下实现:
async getOomolToken(): Promise<string> { - return process.env["OOMOL_TOKEN"] || ""; + const token = process.env["OOMOL_TOKEN"] || ""; + if (!token && !this.#tokenWarningShown) { + await this.warning( + "OOMOL_TOKEN environment variable is not set, authentication features may not work properly." + ); + this.#tokenWarningShown = true; + } + return token; }注意:这需要在类中添加
#tokenWarningShown字段。packages/oocana-types/src/external/context.ts (1)
147-147: 建议为公共 API 添加文档。Context 接口中的其他方法都有详细的 JSDoc 文档和使用示例(如
queryAuth、runBlock等)。为了保持 API 文档的完整性和一致性,建议为getOomolToken()添加说明文档。可以添加如下文档:
+ /** + * Get OOMOL authentication token from environment. + * @returns Promise that resolves to the OOMOL_TOKEN environment variable value, or empty string if not set. + * + * @example + * ```ts + * async function main(inputs, context) { + * const token = await context.getOomolToken(); + * if (token) { + * // Use token for authentication + * } + * } + * ``` + */ getOomolToken: () => Promise<string>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/oocana-sdk/src/context.ts(1 hunks)packages/oocana-types/src/external/context.ts(1 hunks)
🔇 Additional comments (1)
packages/oocana-sdk/src/context.ts (1)
115-117: 令牌处理实现是安全的验证结果显示
getOomolToken()方法的实现是安全的:
- 方法实现不包含任何日志记录、错误暴露或调试输出
OOMOL_TOKEN环境变量仅在此方法中访问,直接返回值,无额外处理- 整个代码库中未发现此方法被调用的位置,因此无法通过现有代码路径暴露令牌
- 方法中无
console.log、logger、throw Error等会暴露令牌的语句代码符合安全要求,无需修改。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/oocana-types/src/external/context.ts (1)
146-159: 考虑增强文档说明以明确空字符串的语义。当前文档说明了方法返回空字符串(如果未设置),但可以更明确地说明空字符串的含义以及调用者应该如何处理这种情况。示例代码中的
if (token)检查很好,但可以在文档中更明确地说明这一点。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/oocana-sdk/src/context.ts(2 hunks)packages/oocana-types/src/external/context.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/oocana-sdk/src/context.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check
🔇 Additional comments (1)
packages/oocana-types/src/external/context.ts (1)
146-160: 验证令牌的安全使用。此方法暴露了认证令牌,需要确保:
- 令牌不会被意外记录到日志中
- 令牌仅用于预期的认证场景
- 使用此令牌的代码遵循安全最佳实践
根据实现,缺失令牌时会发出警告,这是好的做法。请确认令牌的使用场景都经过了安全审查。
| * } | ||
| * ``` | ||
| */ | ||
| getOomolToken: () => Promise<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
添加 readonly 关键字以保持接口一致性。
此接口中的其他方法(如 queryAuth、queryDownstream、outputs 等)都使用了 readonly 关键字。为保持一致性,建议为此方法也添加该关键字。
应用此差异以添加 readonly 关键字:
- getOomolToken: () => Promise<string>;
+ readonly getOomolToken: () => Promise<string>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getOomolToken: () => Promise<string>; | |
| readonly getOomolToken: () => Promise<string>; |
🤖 Prompt for AI Agents
In packages/oocana-types/src/external/context.ts around line 160, the interface
method declaration for getOomolToken is missing the readonly modifier which
other methods use; update the method signature to include the readonly keyword
(i.e., make it readonly getOomolToken: () => Promise<string>) so the interface
is consistent, preserve surrounding formatting/commas, and run TypeScript type
checks to ensure no downstream changes are required.
No description provided.