Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/ISSUE_TEMPLATE/config.yml
Original file line number Diff line number Diff line change
@@ -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
Expand Down
1 change: 1 addition & 0 deletions .github/scripts/javascript/process-input.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
5 changes: 3 additions & 2 deletions .github/scripts/python/agent_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -160,4 +161,4 @@ def main() -> None:


if __name__ == "__main__":
main()
main()
15 changes: 14 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
202 changes: 202 additions & 0 deletions PR.md
Original file line number Diff line number Diff line change
@@ -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/)
51 changes: 50 additions & 1 deletion src/models/__tests__/openai.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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."
)
})

Expand Down Expand Up @@ -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<string> => {
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<string> => 'sk-from-function'

new OpenAIModel({
modelId: 'gpt-4o',
apiKey: apiKeyFn,
})

expect(OpenAI).toHaveBeenCalledWith(
expect.objectContaining({
apiKey: apiKeyFn,
})
)
})
}
})

describe('updateConfig', () => {
Expand Down
15 changes: 13 additions & 2 deletions src/models/openai.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -241,6 +246,12 @@ export class OpenAIModel extends Model<OpenAIModelConfig> {
* 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({
Expand All @@ -267,7 +278,7 @@ export class OpenAIModel extends Model<OpenAIModelConfig> {
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."
)
}

Expand Down