diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml index 41fbad21..20daf959 100644 --- a/.github/ISSUE_TEMPLATE/config.yml +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -1,4 +1,4 @@ -blank_issues_enabled: false +blank_issues_enabled: true contact_links: - name: Strands Agents SDK Support url: https://github.com/strands-agents/sdk-typescript/discussions diff --git a/.github/scripts/javascript/process-input.cjs b/.github/scripts/javascript/process-input.cjs index cf0965fa..562615f0 100644 --- a/.github/scripts/javascript/process-input.cjs +++ b/.github/scripts/javascript/process-input.cjs @@ -99,6 +99,7 @@ module.exports = async (context, github, core, inputs) => { core.setOutput('branch_name', branchName); core.setOutput('session_id', sessionId); + core.setOutput('session_prefix', process.env.GITHUB_REPOSITORY); core.setOutput('system_prompt', systemPrompt); core.setOutput('prompt', prompt); diff --git a/.github/scripts/python/agent_runner.py b/.github/scripts/python/agent_runner.py index 425d2607..776695e1 100644 --- a/.github/scripts/python/agent_runner.py +++ b/.github/scripts/python/agent_runner.py @@ -109,13 +109,14 @@ def run_agent(query: str): system_prompt = os.getenv("INPUT_SYSTEM_PROMPT", DEFAULT_SYSTEM_PROMPT) session_id = os.getenv("SESSION_ID") s3_bucket = os.getenv("S3_SESSION_BUCKET") + s3_prefix = os.getenv("GITHUB_REPOSITORY", "") if s3_bucket and session_id: print(f"πŸ€– Using session manager with session ID: {session_id}") session_manager = S3SessionManager( session_id=session_id, bucket=s3_bucket, - prefix="", + prefix=s3_prefix, ) else: raise ValueError("Both SESSION_ID and S3_SESSION_BUCKET must be set") @@ -160,4 +161,4 @@ def main() -> None: if __name__ == "__main__": - main() \ No newline at end of file + main() diff --git a/AGENTS.md b/AGENTS.md index 2772a65c..8814ab01 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -162,8 +162,21 @@ See [CONTRIBUTING.md - Development Environment](CONTRIBUTING.md#development-envi 3. **Run quality checks** before committing (pre-commit hooks will run automatically) 4. **Commit with conventional commits**: `feat:`, `fix:`, `refactor:`, `docs:`, etc. 5. **Push to remote**: `git push origin agent-tasks/{ISSUE_NUMBER}` +6. **Create pull request** following [PR.md](PR.md) guidelines -### 3. Quality Gates +### 3. Pull Request Guidelines + +When creating pull requests, you **MUST** follow the guidelines in [PR.md](PR.md). Key principles: + +- **Focus on WHY**: Explain motivation and user impact, not implementation details +- **Document public API changes**: Show before/after code examples +- **Be concise**: Use prose over bullet lists; avoid exhaustive checklists +- **Target senior engineers**: Assume familiarity with the SDK +- **Exclude implementation details**: Leave these to code comments and diffs + +See [PR.md](PR.md) for the complete RFC-style guidance and template. + +### 4. Quality Gates Pre-commit hooks automatically run: - Unit tests (via npm test) diff --git a/PR.md b/PR.md new file mode 100644 index 00000000..78caa3e8 --- /dev/null +++ b/PR.md @@ -0,0 +1,202 @@ +# RFC: Pull Request Description Guidelines + +## Status + +Active + +## Summary + +This document defines the format and content guidelines for pull request descriptions in the Strands TypeScript SDK repository. + +## Motivation + +Pull request descriptions serve as the primary documentation for code changes, helping reviewers understand the context and impact of modifications. Well-written descriptions enable faster reviews, better decision-making, and serve as valuable historical documentation. + +## Target Audience + +PR descriptions should be written for **senior software engineers familiar with both the Python and TypeScript Strands SDK**. Assume the reader: +- Understands the SDK's architecture and patterns +- Has context about the broader system +- Can read code diffs to understand implementation details +- Values concise, focused communication + +## Guidelines + +### Structure + +A PR description MUST include: + +1. **Motivation** - Why is this change needed? +2. **Public API Changes** - What changes to the public API (with code examples)? +3. **Use Cases** (optional) - When would developers use this feature? +4. **Breaking Changes** (if applicable) - What breaks and how to migrate? + +### Content Principles + +**Focus on WHY, not HOW:** +- βœ… "The OpenAI SDK supports dynamic API keys, but we don't expose this capability" +- ❌ "Added ApiKeySetter type import from openai/client" + +**Document public API changes with examples:** +- βœ… Show before/after code examples for API changes +- ❌ List every file or line changed + +**Be concise:** +- βœ… Use prose over bullet lists when possible +- ❌ Create exhaustive implementation checklists + +**Emphasize user impact:** +- βœ… "Enables secret manager integration for credential rotation" +- ❌ "Updated error message to mention 'string or function'" + +### What NOT to Include + +PR descriptions SHOULD NOT include: + +- **Implementation details** - Leave these to code comments and commit messages +- **Test coverage notes** - Assume tests are comprehensive (CI will catch issues) +- **Line-by-line change lists** - The diff provides this information +- **Build/lint/coverage status** - CI handles verification +- **Commit hashes** - GitHub links commits automatically + +### Anti-patterns + +❌ **Over-detailed checklists:** +```markdown +### Type Definition Updates +- Added ApiKeySetter type import from 'openai/client' +- Updated OpenAIModelOptions interface apiKey type +``` + +❌ **Implementation notes reviewers don't need:** +```markdown +## Implementation Notes +- No breaking changes - all existing string-based usage continues to work +- OpenAI SDK handles validation of function return values +``` + +❌ **Test coverage bullets:** +```markdown +### Test Coverage +- Added test: accepts function-based API key +- Added test: accepts async function-based API key +``` + +### Good Examples + +βœ… **Motivation section:** +```markdown +## Motivation + +The OpenAI SDK supports dynamic API key resolution through async functions, +enabling use cases like credential rotation and secret manager integration. +However, our SDK currently only accepts static strings for the apiKey parameter, +preventing users from leveraging these capabilities. +``` + +βœ… **Public API Changes section:** +```markdown +## Public API Changes + +The `OpenAIModelOptions.apiKey` parameter now accepts either a string or an +async function: + +\`\`\`typescript +// Before: only string supported +const model = new OpenAIModel({ + modelId: 'gpt-4o', + apiKey: 'sk-...' +}) + +// After: function also supported +const model = new OpenAIModel({ + modelId: 'gpt-4o', + apiKey: async () => await secretManager.getApiKey() +}) +\`\`\` + +The change is backward compatibleβ€”all existing string-based usage continues +to work without modification. +``` + +βœ… **Use Cases section:** +```markdown +## Use Cases + +- **API key rotation**: Rotate keys without application restart +- **Secret manager integration**: Fetch credentials from AWS Secrets Manager, Vault, etc. +- **Multi-tenant systems**: Dynamically select API keys based on context +``` + +## Template + +```markdown +## Motivation + +[Explain WHY this change is needed. What problem does it solve? What limitation +does it address? What user need does it fulfill?] + +Resolves: #[issue-number] + +## Public API Changes + +[Document changes to public APIs with before/after code examples. If no public +API changes, state "No public API changes."] + +\`\`\`typescript +// Before +[existing API usage] + +// After +[new API usage] +\`\`\` + +[Explain behavior, parameters, return values, and backward compatibility.] + +## Use Cases (optional) + +[If adding new functionality, provide 2-4 concrete use cases showing when +developers would use this feature.] + +## Breaking Changes (if applicable) + +[If this is a breaking change, explain what breaks and provide migration guidance.] + +### Migration + +\`\`\`typescript +// Before +[old code] + +// After +[new code] +\`\`\` +``` + +## Rationale + +**Why focus on WHY over HOW?** +- Code diffs show HOW changes are implemented +- Commit messages document detailed changes +- PR descriptions provide the broader context reviewers need + +**Why exclude test/lint/coverage details?** +- CI pipelines verify these automatically +- Including them adds noise without value +- Reviewers trust the CI process + +**Why assume senior engineer audience?** +- Enables more concise, technical communication +- Reduces redundant explanations +- Respects reviewer expertise + +## Enforcement + +- PR descriptions SHOULD follow this format +- Reviewers MAY request description updates for PRs that don't follow guidelines +- Agents creating PRs MUST follow these guidelines + +## References + +- [Conventional Commits](https://www.conventionalcommits.org/) +- [Google's Code Review Guidelines](https://google.github.io/eng-practices/review/) diff --git a/src/models/__tests__/openai.test.ts b/src/models/__tests__/openai.test.ts index 01614b7f..d043d78e 100644 --- a/src/models/__tests__/openai.test.ts +++ b/src/models/__tests__/openai.test.ts @@ -100,7 +100,7 @@ describe('OpenAIModel', () => { vi.stubEnv('OPENAI_API_KEY', '') } expect(() => new OpenAIModel({ modelId: 'gpt-4o' })).toThrow( - "OpenAI API key is required. Provide it via the 'apiKey' option or set the OPENAI_API_KEY environment variable." + "OpenAI API key is required. Provide it via the 'apiKey' option (string or function) or set the OPENAI_API_KEY environment variable." ) }) @@ -144,6 +144,55 @@ describe('OpenAIModel', () => { const mockClient = {} as OpenAI expect(() => new OpenAIModel({ modelId: 'gpt-4o', client: mockClient })).not.toThrow() }) + + it('accepts function-based API key', () => { + const apiKeyFn = vi.fn(async () => 'sk-dynamic') + new OpenAIModel({ + modelId: 'gpt-4o', + apiKey: apiKeyFn, + }) + expect(OpenAI).toHaveBeenCalledWith( + expect.objectContaining({ + apiKey: apiKeyFn, + }) + ) + }) + + it('accepts async function-based API key', () => { + const apiKeyFn = async (): Promise => { + await new Promise((resolve) => globalThis.setTimeout(resolve, 10)) + return 'sk-async-key' + } + + new OpenAIModel({ + modelId: 'gpt-4o', + apiKey: apiKeyFn, + }) + + expect(OpenAI).toHaveBeenCalledWith( + expect.objectContaining({ + apiKey: apiKeyFn, + }) + ) + }) + + if (isNode) { + it('function-based API key takes precedence over environment variable', () => { + vi.stubEnv('OPENAI_API_KEY', 'sk-from-env') + const apiKeyFn = async (): Promise => 'sk-from-function' + + new OpenAIModel({ + modelId: 'gpt-4o', + apiKey: apiKeyFn, + }) + + expect(OpenAI).toHaveBeenCalledWith( + expect.objectContaining({ + apiKey: apiKeyFn, + }) + ) + }) + } }) describe('updateConfig', () => { diff --git a/src/models/openai.ts b/src/models/openai.ts index 3e325a22..60172b86 100644 --- a/src/models/openai.ts +++ b/src/models/openai.ts @@ -8,6 +8,7 @@ */ import OpenAI, { type ClientOptions } from 'openai' +import type { ApiKeySetter } from 'openai/client' import { Model } from '../models/model.js' import type { BaseModelConfig, StreamOptions } from '../models/model.js' import type { Message } from '../types/messages.js' @@ -169,8 +170,12 @@ export interface OpenAIModelConfig extends BaseModelConfig { export interface OpenAIModelOptions extends OpenAIModelConfig { /** * OpenAI API key (falls back to OPENAI_API_KEY environment variable). + * + * Accepts either a static string or an async function that resolves to a string. + * When a function is provided, it is invoked before each request, allowing for + * dynamic API key rotation or runtime credential refresh. */ - apiKey?: string + apiKey?: string | ApiKeySetter /** * Pre-configured OpenAI client instance. @@ -241,6 +246,12 @@ export class OpenAIModel extends Model { * modelId: 'gpt-3.5-turbo' * }) * + * // Using function-based API key for dynamic rotation + * const provider = new OpenAIModel({ + * modelId: 'gpt-4o', + * apiKey: async () => await getRotatingApiKey() + * }) + * * // Using a pre-configured client instance * const client = new OpenAI({ apiKey: 'sk-...', timeout: 60000 }) * const provider = new OpenAIModel({ @@ -267,7 +278,7 @@ export class OpenAIModel extends Model { typeof process !== 'undefined' && typeof process.env !== 'undefined' && process.env.OPENAI_API_KEY if (!apiKey && !hasEnvKey) { throw new Error( - "OpenAI API key is required. Provide it via the 'apiKey' option or set the OPENAI_API_KEY environment variable." + "OpenAI API key is required. Provide it via the 'apiKey' option (string or function) or set the OPENAI_API_KEY environment variable." ) }