support for Magma AI Proxy - #67
Conversation
|
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:
📝 WalkthroughWalkthroughTempo 3.11.0 adds centralized bounded caching, license scope updates, timezone parsing changes, and lazy runtime behavior. The AI plugin adds provider modes, rate-limit handling, cache integration, public APIs, tests, documentation, and release metadata. ChangesLicense scope updates
Centralized cache and runtime
AI plugin updates
Parsing and release updates
Repository workflow updates
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant parseAI
participant Provider
participant TempoCache
Caller->>parseAI: parseAI(input, options)
parseAI->>TempoCache: read normalized cache key
parseAI->>Provider: request provider result
Provider-->>parseAI: parsed content and rate-limit headers
parseAI->>TempoCache: write successful result
parseAI-->>Caller: return Tempo with AI metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tempo/src/plugin/license/license.manager.ts`:
- Around line 132-135: Make revocation checking lazy by changing the response
contract used by the license manager from an already-created
res.revocationPromise to a factory, and invoke that factory only after
shouldSkipRevocation is false in the revocation flow. Update the license phase
test mock to provide the same lazy factory and spy on its invocation, verifying
opt-out licenses do not start the revocation request; modify both
packages/tempo/src/plugin/license/license.manager.ts:132-135 and
packages/tempo/test/plugins/license.phase1.test.ts:86-100.
🪄 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: bfaf2e82-e170-4bfe-abc3-f571285e9938
📒 Files selected for processing (8)
package.jsonpackages/library/package.jsonpackages/tempo/package.jsonpackages/tempo/src/plugin/license/license.manager.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/support.symbol.tspackages/tempo/src/tempo.class.tspackages/tempo/test/plugins/license.phase1.test.ts
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
packages/tempo/test/discrete/parse.locale.test.ts (1)
82-102: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the resolved date, not only parse validity.
These assertions do not verify that
próximo/proximomeans the next Monday or that article removal is applied. Use a fixed anchor and assert the resulting weekday/date for the accented, unaccented, and article-prefixed inputs.🤖 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 `@packages/tempo/test/discrete/parse.locale.test.ts` around lines 82 - 102, Update the “test Spanish el próximo lunes” test to use a fixed anchor date, then assert the resolved date (including Monday’s weekday/date) for “próximo lunes”, “el próximo lunes”, and “el proximo lunes”. Retain validity checks as appropriate, ensuring the assertions verify both modifier resolution and article removal rather than parse success alone.packages/plugins/ai/test/index.spec.ts (1)
57-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winForce the AI path in the natural-language test.
This test is intended to validate mocked LLM parsing, but it neither passes
force: truenor asserts a network call. If native parsing later accepts this phrase, the test can pass without exercising the provider contract.🤖 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 `@packages/plugins/ai/test/index.spec.ts` around lines 57 - 61, Update the parseAI invocation in the natural-language test to pass force: true alongside the existing anchor and timeZone options, ensuring the test always exercises the mocked AI provider path.
🤖 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/plugins/ai/CHANGELOG.md`:
- Line 6: Update the 0.1.0 fallback-routing release note to accurately describe
the shipped real provider requests and fallback routing, removing the “mocked
proxy” wording; only label it test-only scaffolding if that is the actual scope
of the implementation.
In `@packages/plugins/ai/doc/architecture.md`:
- Around line 28-34: The Azure OpenAI example and provider contract currently
assume provider.key is sent as a bearer token. Update the configuration around
the openai provider to support Azure’s api-key header for AZURE_API_KEY, or
explicitly rename/document the value as an Entra bearer token when using
Authorization; keep the authentication behavior consistent with the selected
credential type.
In `@packages/plugins/ai/doc/index.md`:
- Around line 72-75: Update the forced-evaluation example for parseAI to use a
natively handled phrase such as “Next Friday” instead of the AI-oriented query.
Correct the surrounding description so force is documented as skipping cache
lookup, not bypassing the cache entirely, since the result is still written to
cache.
In `@packages/plugins/ai/doc/rate-limits.md`:
- Line 56: Update the “Granular Time Gotcha” documentation to clarify that the
cache salt normally uses the execution date but uses the caller-provided anchor
date when options.anchor is set. Explain that a fixed anchor can keep the cache
key unchanged across midnight, so the automatic midnight miss is not guaranteed.
- Around line 62-63: Update the documentation example and its accompanying
guarantee to clarify that cache: false only disables caching and does not ensure
an LLM request. When demonstrating a required provider call, use force: true
together with cache: false in the parseAI options.
In `@packages/plugins/ai/package.json`:
- Line 21: Remove the main-branch check from the prepublishOnly script in
package.json, leaving the build step intact so publishing works from tag-based
and detached HEAD checkouts. Move branch validation to the release automation if
it is still required.
In `@packages/plugins/ai/README.md`:
- Around line 13-15: Fix MD028 blockquote formatting in
packages/plugins/ai/README.md lines 13-15 by removing or prefixing the blank
line between the security warning and disclaimer; apply the same treatment to
the blank line inside the experimental-plugin warning block in
packages/plugins/ai/doc/index.md lines 9-14.
- Around line 29-33: Validate each required provider environment variable before
constructing the provider list passed to initAI. In
packages/plugins/ai/README.md lines 29-33, guard GROQ_API_KEY before assigning
it to the groq provider key; apply the same conditional filtering to every
provider environment variable in packages/plugins/ai/doc/architecture.md lines
12-18 so AiProvider.key always receives a string.
In `@packages/plugins/ai/src/cache.ts`:
- Around line 83-101: Update the Map-compatible observation APIs in the TTL
cache class: ensure inherited forEach() and size access also invoke
evictExpired() before exposing entries or counts. Preserve the existing behavior
of keys(), values(), entries(), and Symbol.iterator(), and use the class’s
existing eviction mechanism rather than adding separate expiration logic.
- Around line 61-68: Update the eviction loop in the cache class’s set flow to
inspect the iterator result’s done flag rather than treating oldestKey ===
undefined as empty. Delete the yielded key, including an explicitly stored
undefined key, and only break when the iterator is exhausted so the cache
remains within maxSize.
In `@packages/plugins/ai/src/parseAI.type.ts`:
- Around line 44-50: Ensure the rate-limit parsing flow sanitizes provider
header values before assigning _state.limits: reject NaN, sentinel values such
as 999, and invalid Tempo resetAt values. Represent unavailable limits as null
or another established unknown form by updating AiRateLimits and its consumers,
rather than exposing malformed values as valid data.
In `@packages/tempo/src/tempo.class.ts`:
- Line 1642: Update `#resolve`() in packages/tempo/src/tempo.class.ts to check for
an existing resolved `#zdt` before accessing `#now`, and only read `#now` when
fallback or unresolved parsing requires it; retain lazy caching in `#now`. In
packages/tempo/CHANGELOG.md lines 12-14, keep the performance claim only once
the implementation and benchmark verify it; otherwise remove or revise the
claim.
---
Nitpick comments:
In `@packages/plugins/ai/test/index.spec.ts`:
- Around line 57-61: Update the parseAI invocation in the natural-language test
to pass force: true alongside the existing anchor and timeZone options, ensuring
the test always exercises the mocked AI provider path.
In `@packages/tempo/test/discrete/parse.locale.test.ts`:
- Around line 82-102: Update the “test Spanish el próximo lunes” test to use a
fixed anchor date, then assert the resolved date (including Monday’s
weekday/date) for “próximo lunes”, “el próximo lunes”, and “el proximo lunes”.
Retain validity checks as appropriate, ensuring the assertions verify both
modifier resolution and article removal rather than parse success alone.
🪄 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: fcbd7c8a-67d6-4c9a-97b0-1fa09fe4a82e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
package.jsonpackages/plugins/ai/CHANGELOG.mdpackages/plugins/ai/LICENSEpackages/plugins/ai/README.mdpackages/plugins/ai/doc/architecture.mdpackages/plugins/ai/doc/context.mdpackages/plugins/ai/doc/index.mdpackages/plugins/ai/doc/rate-limits.mdpackages/plugins/ai/package.jsonpackages/plugins/ai/src/cache.tspackages/plugins/ai/src/error.tspackages/plugins/ai/src/index.tspackages/plugins/ai/src/parseAI.type.tspackages/plugins/ai/test/index.spec.tspackages/plugins/ai/test/tsconfig.jsonpackages/plugins/ai/tsconfig.jsonpackages/plugins/ai/tsup.config.tspackages/plugins/parseAI/README.mdpackages/tempo/.vitepress/theme/data/catalog.jsonpackages/tempo/CHANGELOG.mdpackages/tempo/doc/4-advanced-reference/tempo.locale.mdpackages/tempo/src/plugin/license/license.manager.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.version.tspackages/tempo/test/discrete/parse.locale.test.tspackages/tempo/test/plugins/license.phase1.test.ts
💤 Files with no reviewable changes (1)
- packages/plugins/parseAI/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/tempo/test/plugins/license.phase1.test.ts
- packages/tempo/src/plugin/license/license.manager.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
🧹 Nitpick comments (2)
packages/tempo/test/discrete/parse.locale.test.ts (1)
82-102: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the resolved date, not only parse validity.
These assertions do not verify that
próximo/proximomeans the next Monday or that article removal is applied. Use a fixed anchor and assert the resulting weekday/date for the accented, unaccented, and article-prefixed inputs.🤖 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 `@packages/tempo/test/discrete/parse.locale.test.ts` around lines 82 - 102, Update the “test Spanish el próximo lunes” test to use a fixed anchor date, then assert the resolved date (including Monday’s weekday/date) for “próximo lunes”, “el próximo lunes”, and “el proximo lunes”. Retain validity checks as appropriate, ensuring the assertions verify both modifier resolution and article removal rather than parse success alone.packages/plugins/ai/test/index.spec.ts (1)
57-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winForce the AI path in the natural-language test.
This test is intended to validate mocked LLM parsing, but it neither passes
force: truenor asserts a network call. If native parsing later accepts this phrase, the test can pass without exercising the provider contract.🤖 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 `@packages/plugins/ai/test/index.spec.ts` around lines 57 - 61, Update the parseAI invocation in the natural-language test to pass force: true alongside the existing anchor and timeZone options, ensuring the test always exercises the mocked AI provider path.
🤖 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/plugins/ai/CHANGELOG.md`:
- Line 6: Update the 0.1.0 fallback-routing release note to accurately describe
the shipped real provider requests and fallback routing, removing the “mocked
proxy” wording; only label it test-only scaffolding if that is the actual scope
of the implementation.
In `@packages/plugins/ai/doc/architecture.md`:
- Around line 28-34: The Azure OpenAI example and provider contract currently
assume provider.key is sent as a bearer token. Update the configuration around
the openai provider to support Azure’s api-key header for AZURE_API_KEY, or
explicitly rename/document the value as an Entra bearer token when using
Authorization; keep the authentication behavior consistent with the selected
credential type.
In `@packages/plugins/ai/doc/index.md`:
- Around line 72-75: Update the forced-evaluation example for parseAI to use a
natively handled phrase such as “Next Friday” instead of the AI-oriented query.
Correct the surrounding description so force is documented as skipping cache
lookup, not bypassing the cache entirely, since the result is still written to
cache.
In `@packages/plugins/ai/doc/rate-limits.md`:
- Line 56: Update the “Granular Time Gotcha” documentation to clarify that the
cache salt normally uses the execution date but uses the caller-provided anchor
date when options.anchor is set. Explain that a fixed anchor can keep the cache
key unchanged across midnight, so the automatic midnight miss is not guaranteed.
- Around line 62-63: Update the documentation example and its accompanying
guarantee to clarify that cache: false only disables caching and does not ensure
an LLM request. When demonstrating a required provider call, use force: true
together with cache: false in the parseAI options.
In `@packages/plugins/ai/package.json`:
- Line 21: Remove the main-branch check from the prepublishOnly script in
package.json, leaving the build step intact so publishing works from tag-based
and detached HEAD checkouts. Move branch validation to the release automation if
it is still required.
In `@packages/plugins/ai/README.md`:
- Around line 13-15: Fix MD028 blockquote formatting in
packages/plugins/ai/README.md lines 13-15 by removing or prefixing the blank
line between the security warning and disclaimer; apply the same treatment to
the blank line inside the experimental-plugin warning block in
packages/plugins/ai/doc/index.md lines 9-14.
- Around line 29-33: Validate each required provider environment variable before
constructing the provider list passed to initAI. In
packages/plugins/ai/README.md lines 29-33, guard GROQ_API_KEY before assigning
it to the groq provider key; apply the same conditional filtering to every
provider environment variable in packages/plugins/ai/doc/architecture.md lines
12-18 so AiProvider.key always receives a string.
In `@packages/plugins/ai/src/cache.ts`:
- Around line 83-101: Update the Map-compatible observation APIs in the TTL
cache class: ensure inherited forEach() and size access also invoke
evictExpired() before exposing entries or counts. Preserve the existing behavior
of keys(), values(), entries(), and Symbol.iterator(), and use the class’s
existing eviction mechanism rather than adding separate expiration logic.
- Around line 61-68: Update the eviction loop in the cache class’s set flow to
inspect the iterator result’s done flag rather than treating oldestKey ===
undefined as empty. Delete the yielded key, including an explicitly stored
undefined key, and only break when the iterator is exhausted so the cache
remains within maxSize.
In `@packages/plugins/ai/src/parseAI.type.ts`:
- Around line 44-50: Ensure the rate-limit parsing flow sanitizes provider
header values before assigning _state.limits: reject NaN, sentinel values such
as 999, and invalid Tempo resetAt values. Represent unavailable limits as null
or another established unknown form by updating AiRateLimits and its consumers,
rather than exposing malformed values as valid data.
In `@packages/tempo/src/tempo.class.ts`:
- Line 1642: Update `#resolve`() in packages/tempo/src/tempo.class.ts to check for
an existing resolved `#zdt` before accessing `#now`, and only read `#now` when
fallback or unresolved parsing requires it; retain lazy caching in `#now`. In
packages/tempo/CHANGELOG.md lines 12-14, keep the performance claim only once
the implementation and benchmark verify it; otherwise remove or revise the
claim.
---
Nitpick comments:
In `@packages/plugins/ai/test/index.spec.ts`:
- Around line 57-61: Update the parseAI invocation in the natural-language test
to pass force: true alongside the existing anchor and timeZone options, ensuring
the test always exercises the mocked AI provider path.
In `@packages/tempo/test/discrete/parse.locale.test.ts`:
- Around line 82-102: Update the “test Spanish el próximo lunes” test to use a
fixed anchor date, then assert the resolved date (including Monday’s
weekday/date) for “próximo lunes”, “el próximo lunes”, and “el proximo lunes”.
Retain validity checks as appropriate, ensuring the assertions verify both
modifier resolution and article removal rather than parse success alone.
🪄 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: fcbd7c8a-67d6-4c9a-97b0-1fa09fe4a82e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
package.jsonpackages/plugins/ai/CHANGELOG.mdpackages/plugins/ai/LICENSEpackages/plugins/ai/README.mdpackages/plugins/ai/doc/architecture.mdpackages/plugins/ai/doc/context.mdpackages/plugins/ai/doc/index.mdpackages/plugins/ai/doc/rate-limits.mdpackages/plugins/ai/package.jsonpackages/plugins/ai/src/cache.tspackages/plugins/ai/src/error.tspackages/plugins/ai/src/index.tspackages/plugins/ai/src/parseAI.type.tspackages/plugins/ai/test/index.spec.tspackages/plugins/ai/test/tsconfig.jsonpackages/plugins/ai/tsconfig.jsonpackages/plugins/ai/tsup.config.tspackages/plugins/parseAI/README.mdpackages/tempo/.vitepress/theme/data/catalog.jsonpackages/tempo/CHANGELOG.mdpackages/tempo/doc/4-advanced-reference/tempo.locale.mdpackages/tempo/src/plugin/license/license.manager.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.version.tspackages/tempo/test/discrete/parse.locale.test.tspackages/tempo/test/plugins/license.phase1.test.ts
💤 Files with no reviewable changes (1)
- packages/plugins/parseAI/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/tempo/test/plugins/license.phase1.test.ts
- packages/tempo/src/plugin/license/license.manager.ts
🛑 Comments failed to post (6)
packages/plugins/ai/CHANGELOG.md (1)
6-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the fallback-routing release note.
The implementation performs real provider requests and fallback routing, so “mocked proxy” understates the behavior shipped in 0.1.0. Describe the actual provider behavior or clearly mark this as test-only scaffolding.
🤖 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 `@packages/plugins/ai/CHANGELOG.md` at line 6, Update the 0.1.0 fallback-routing release note to accurately describe the shipped real provider requests and fallback routing, removing the “mocked proxy” wording; only label it test-only scaffolding if that is the actual scope of the implementation.packages/plugins/ai/doc/architecture.md (1)
28-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Locate architecture and related provider files ==" git ls-files | rg '(^|/)architecture\.md$|ai|provider|openai|azure|auth|authorization' | head -200 echo echo "== Read target file excerpt ==" if [ -f packages/plugins/ai/doc/architecture.md ]; then nl -ba packages/plugins/ai/doc/architecture.md | sed -n '1,120p' fi echo echo "== Search provider key / Authorization usage ==" rg -n "Authorization|provider\.key|AuthorizationHeader|api_key|AZURE_API_KEY|openai\.azure|azure" -S . --glob '!node_modules' --glob '!dist' --glob '!build' | head -300Repository: magmacomputing/magma
Length of output: 1097
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Read target file excerpt ==" if [ -f packages/plugins/ai/doc/architecture.md ]; then awk '{printf "%6d\t%s\n", NR, $0}' packages/plugins/ai/doc/architecture.md | sed -n '1,140p' fi echo echo "== Search provider key / Authorization usage ==" rg -n "Authorization|provider\.key|AuthorizationHeader|api_key|AZURE_API_KEY|openai\.azure|azure|Authorization:" -S packages packages/plugins/ai --glob '!node_modules' --glob '!dist' --glob '!build' | head -400 echo echo "== Read AI plugin source candidates ==" for f in packages/plugins/ai/src/index.ts packages/plugins/ai/src/parseAI.type.ts packages/plugins/ai/src/error.ts packages/plugins/ai/src/cache.ts; do if [ -f "$f" ]; then echo "--- $f ---" wc -l "$f" awk '{printf "%6d\t%s\n", NR, $0}' "$f" | sed -n '1,220p' fi doneRepository: magmacomputing/magma
Length of output: 20738
🌐 Web query:
Azure OpenAI chat completions API authentication Authorization header api-key Bearer💡 Result:
Azure OpenAI Service supports multiple authentication methods for the chat completions API, categorized primarily into API Key-based and Token-based (Microsoft Entra ID) authentication [1][2]. API Key Authentication - api-key header: You can provide your API key directly in the api-key HTTP header [1][3][2]. - Authorization header: Alternatively, you can pass the API key via the Authorization header [1][4]. Token-based Authentication (Microsoft Entra ID) - Authorization header: This method requires an OAuth 2.0 access token [5][2]. - Format: The token must be provided in the Authorization header using the Bearer scheme: Authorization: Bearer {Azure_OpenAI_Auth_Token} [5][2]. - Implementation: When using Azure SDKs (such as the Azure OpenAI client libraries), this is often handled automatically using DefaultAzureCredential, which simplifies the process of obtaining and injecting the bearer token [6][7][8]. Microsoft recommends using token-based authentication (Microsoft Entra ID) over API keys as it is more secure and avoids the need to manage and store static keys [5][2][7]. Regardless of the method used, ensure your requests are sent to the correct resource endpoint [1][6].
Citations:
- 1: https://learn.microsoft.com/en-us/rest/api/microsoft-foundry/azureopenai/chat
- 2: https://learn.microsoft.com/en-us/azure/ai-foundry/openai/reference-preview
- 3: https://learn.microsoft.com/en-us/azure/foundry/openai/latest
- 4: https://learn.microsoft.com/en-us/azure/foundry/openai/authoring-reference-preview
- 5: https://learn.microsoft.com/en-us/azure/ai-foundry/openai/reference-preview-latest
- 6: https://learn.microsoft.com/en-us/azure/ai-foundry/openai/how-to/managed-identity
- 7: https://learn.microsoft.com/en-us/azure/developer/ai/keyless-connections
- 8: https://learn.microsoft.com/en-us/dotnet/ai/azure-ai-services-authentication
🌐 Web query:
Azure OpenAI chat completions authentication Authorization header api-key Bearer💡 Result:
Azure OpenAI supports two primary methods for authenticating requests: API Key authentication and Microsoft Entra ID (token-based) authentication [1][2]. Microsoft Entra ID authentication is recommended as the more secure approach [1][3]. API Key Authentication To authenticate using an API key, you must provide the key in the request header using the header name api-key [1][2]. Example (cURL): curl -X POST https://YOUR-RESOURCE-NAME.openai.azure.com/openai/v1/chat/completions \ -H "Content-Type: application/json" \ -H "api-key: $AZURE_OPENAI_API_KEY" \ -d '{...}' Microsoft Entra ID (Bearer Token) Authentication To authenticate using Microsoft Entra ID, you must provide an OAuth 2.0 access token in the Authorization header using the Bearer scheme [1][2]. Example (cURL): curl -X POST https://YOUR-RESOURCE-NAME.openai.azure.com/openai/v1/chat/completions \ -H "Content-Type: application/json" \ -H "Authorization: Bearer $AZURE_OPENAI_AUTH_TOKEN" \ -d '{...}' The required scope for obtaining the access token is typically https://cognitiveservices.azure.com/.default (or https://ai.azure.com/.default in certain AI Foundry contexts) [1][3][4]. When using official Azure SDKs (such as those for.NET, JavaScript/TypeScript, or Python), authentication is typically handled by providing a credential object (e.g., DefaultAzureCredential) or a token provider to the client constructor, which manages the bearer token acquisition automatically [5][6][4][7].
Citations:
- 1: https://learn.microsoft.com/en-us/azure/ai-foundry/openai/reference-preview-latest
- 2: https://learn.microsoft.com/en-us/azure/foundry/openai/api-version-lifecycle
- 3: https://learn.microsoft.com/en-us/azure/foundry/openai/reference-preview-latest
- 4: https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/openai/openai/README.md
- 5: https://learn.microsoft.com/en-us/azure/ai-foundry/openai/how-to/managed-identity
- 6: https://learn.microsoft.com/en-us/dotnet/ai/azure-ai-services-authentication
- 7: https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/openai/Azure.AI.OpenAI/README.md
Use Azure’s supported API key header or a bearer token in this example.
The plugin sends
Authorization: Bearer ${provider.key}, but Azure OpenAI API keys are passed in theapi-keyheader. IfAZURE_API_KEYis an API key, change the example/contract to support that header; if it is an Entra token, clarify that the value is an auth token, not an API key.🤖 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 `@packages/plugins/ai/doc/architecture.md` around lines 28 - 34, The Azure OpenAI example and provider contract currently assume provider.key is sent as a bearer token. Update the configuration around the openai provider to support Azure’s api-key header for AZURE_API_KEY, or explicitly rename/document the value as an Entra bearer token when using Authorization; keep the authentication behavior consistent with the selected credential type.packages/plugins/ai/package.json (1)
21-21: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== package file ==" sed -n '1,80p' packages/plugins/ai/package.json 2>/dev/null || true echo echo "== git rev-parse abbrev-ref behavior probe ==" tmp="$(mktemp -d)" cd "$tmp" git init -q echo ok > a.txt git add a.txt git commit -q -m init echo "attached main: $(git rev-parse --abbrev-ref HEAD)" git checkout -q --detach HEAD echo "detached HEAD: $(git rev-parse --abbrev-ref HEAD)" git checkout -q main echo "still attached main: $(git rev-parse --abbrev-ref HEAD)" git tag -a "v1.0.0" -m tag 2>/dev/null || true git checkout -q "v1.0.0" echo "tag checkout: $(git rev-parse --abbrev-ref HEAD)" rm -rf "$tmp"Repository: magmacomputing/magma
Length of output: 1709
🌐 Web query:
git rev-parse --abbrev-ref HEAD detached HEAD returns HEAD documentation💡 Result:
When running git rev-parse --abbrev-ref HEAD in a detached HEAD state, the command returns the literal string HEAD [1][2][3]. This occurs because the --abbrev-ref option attempts to provide a non-ambiguous short name for a reference [4][5]. In a normal state, where HEAD points to a branch (a symbolic reference), Git resolves this to the branch name [3]. However, when in a detached HEAD state, HEAD points directly to a commit object rather than a branch, and Git identifies this state by returning the name of the reference itself, HEAD [1]. This behavior differs from other commands like git symbolic-ref --short HEAD, which will exit with an error in a detached HEAD state because HEAD is no longer a symbolic reference [1][2]. If you need to distinguish between a named branch and a detached state in scripts, it is common practice to check the output of git symbolic-ref or use logic to handle the literal "HEAD" string [2][6].
Citations:
- 1: https://stackoverflow.com/questions/45028134/whats-the-difference-between-the-output-from-rev-parse-and-symbolic-ref
- 2: https://stackoverflow.com/questions/67812278/get-the-current-git-branch-name-or-commit-hash-if-detached
- 3: https://latchkey.dev/learn/command-reference/git-rev-parse
- 4: https://git-scm.com/docs/git-rev-parse.html
- 5: https://git-scm.com/docs/git-rev-parse
- 6: https://stackoverflow.com/questions/6245570/how-do-i-get-the-current-branch-name-in-git
Do not gate npm publishing on being checked out to
main.
prepublishOnlyruns from the checkout used by publish automation, including tag-based releases and detached HEAD checkouts, wheregit rev-parse --abbrev-ref HEADis notmain. This block exits beforenpm publishcan run; move this guard to release automation or explicitly support the release ref.🤖 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 `@packages/plugins/ai/package.json` at line 21, Remove the main-branch check from the prepublishOnly script in package.json, leaving the build step intact so publishing works from tag-based and detached HEAD checkouts. Move branch validation to the release automation if it is still required.packages/plugins/ai/src/cache.ts (2)
61-68: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the iterator’s
doneflag instead ofundefinedas the empty sentinel.
Kis generic andMappermitsundefinedkeys. If the oldest key isundefined, this condition breaks without deleting it, soset()can permanently exceedmaxSize.Proposed fix
- const oldestKey = super.keys().next().value; - if (oldestKey !== undefined) { - this.delete(oldestKey); + const oldest = super.keys().next(); + if (!oldest.done) { + this.delete(oldest.value);📝 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.while (this.size > this.maxSize) { const oldest = super.keys().next(); if (!oldest.done) { this.delete(oldest.value); } else { break; } }🤖 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 `@packages/plugins/ai/src/cache.ts` around lines 61 - 68, Update the eviction loop in the cache class’s set flow to inspect the iterator result’s done flag rather than treating oldestKey === undefined as empty. Delete the yielded key, including an explicitly stored undefined key, and only break when the iterator is exhausted so the cache remains within maxSize.
83-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Enforce TTL through all Map observation APIs.
Inherited
forEach()andsizedo not callevictExpired(), so expired entries can still be returned or counted when consumers use the Map-compatible interface. Override those APIs or centralize expiration before every observation.🤖 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 `@packages/plugins/ai/src/cache.ts` around lines 83 - 101, Update the Map-compatible observation APIs in the TTL cache class: ensure inherited forEach() and size access also invoke evictExpired() before exposing entries or counts. Preserve the existing behavior of keys(), values(), entries(), and Symbol.iterator(), and use the class’s existing eviction mechanism rather than adding separate expiration logic.packages/plugins/ai/src/parseAI.type.ts (1)
44-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not expose malformed rate-limit headers as valid values.
The implementation can publish
NaN, sentinel values such as999, or an invalidTempowhen provider headers are missing or malformed. Validate/sanitize these values before assigning_state.limits, or represent unknown values as nullable inAiRateLimits.🤖 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 `@packages/plugins/ai/src/parseAI.type.ts` around lines 44 - 50, Ensure the rate-limit parsing flow sanitizes provider header values before assigning _state.limits: reject NaN, sentinel values such as 999, and invalid Tempo resetAt values. Represent unavailable limits as null or another established unknown form by updating AiRateLimits and its consumers, rather than exposing malformed values as valid data.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/plugins/ai/src/index.ts (1)
153-158: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
Tempo.optionsfor anchor fallback settings.Lines 153-158 read
Tempo.config, which does not merge defaults, persisted settings, or discovery settings.Tempo.optionsperforms that merge. If the caller omitstimeZone,calendar,locale, orsphere, this code can build a cache key with unresolved values and createparsedIsowith[undefined]at Line 312.Proposed fix
- const resolvedConfig = Tempo.config; + const resolvedConfig = Tempo.options;🤖 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 `@packages/plugins/ai/src/index.ts` around lines 153 - 158, Update the anchor fallback setup in the surrounding function to read merged settings from Tempo.options instead of Tempo.config when resolving timeZone, calendar, locale, and sphere. Preserve explicit options overrides and the existing anchorStr fallback, ensuring omitted settings use resolved values rather than remaining undefined.
🧹 Nitpick comments (3)
packages/tempo/src/support/support.cache.ts (1)
5-9: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
get()does not refresh recency, so eviction is not true LRU.The class doc calls this "LRU," but only
set()reorders an entry (lines 82-89).get()(lines 66-72) never touches#timestampsor the Map's insertion order. A frequently-read "hot" key that is never re-written can still be evicted ahead of a rarely-read key that was written more recently. This weakens the cache's purpose (avoiding repeat parses of common queries) under sustained load.Touch the entry on read too, to make eviction reflect actual recency of use:
♻️ Proposed fix to refresh recency on read
override get(key: K): V | undefined { if (this.#isExpired(key)) { this.delete(key); return undefined; } - return super.get(key); + if (super.has(key) && !this.#staticKeys.has(key)) { + const value = super.get(key)!; + super.delete(key); + super.set(key, value); // move to end to reflect recent use + } + return super.get(key); }Also applies to: 66-72
🤖 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 `@packages/tempo/src/support/support.cache.ts` around lines 5 - 9, Update the BoundedCache get() method to refresh a successfully retrieved entry’s recency, matching the existing set() reordering behavior by updating its timestamp and Map insertion order. Preserve misses, static/immortal entries, and TTL handling while ensuring frequently read keys remain most recently used for eviction.packages/tempo/src/support/support.init.ts (1)
27-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate cache-resolution logic between the idempotent and non-idempotent init paths.
Both blocks implement the same three-way branch — BoundedCache instance, Map-like entries (seeded as static), or a plain
{maxSize, ttl}object — against different target variables. Extract a shared helper to avoid future divergence between the two paths:function resolveCache(existingCache: BoundedCache | undefined, cacheOption: any): BoundedCache { const isBoundedCacheObj = Boolean((cacheOption as any)?.isBoundedCache || cacheOption instanceof BoundedCache); if (isBoundedCacheObj) return cacheOption as BoundedCache; if (cacheOption instanceof Map || (cacheOption && isFunction((cacheOption as any).entries))) { const target = existingCache ?? new BoundedCache(); for (const [k, v] of (cacheOption as any).entries()) target.setStatic(String(k).trim().toLowerCase(), String(v)); return target; } if (existingCache) { if (isDefined(cacheOption?.maxSize)) existingCache.maxSize = cacheOption.maxSize; if (isDefined(cacheOption?.ttl)) existingCache.ttl = cacheOption.ttl; return existingCache; } return new BoundedCache(cacheOption?.maxSize ?? 1000, cacheOption?.ttl ?? (24 * 60 * 60 * 1000)); }Then each call site reduces to a single line, e.g.
runtime.state.cache = resolveCache(runtime.state.cache, options.cache);andstate.cache = resolveCache(baseState?.cache ?? runtime.state?.cache ?? prevCache, options.cache);.Also applies to: 70-90
🤖 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 `@packages/tempo/src/support/support.init.ts` around lines 27 - 47, Extract the duplicated cache-option branching from init into a shared resolveCache helper that handles BoundedCache values, Map-like entries, and plain maxSize/ttl options while preserving existing defaults. Replace both the idempotent runtime.state path and the non-idempotent state path with resolveCache calls using their respective existing cache values, and remove the duplicated inline logic.packages/tempo/src/tempo.type.ts (1)
16-16: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAlign
cachetype with its defensive usage.
Internal.State.cacheis typed as required (BoundedCache<string, string>), but every consumer accesses it with optional chaining (for examplestate.cache?.get(normalized)in the facade, andstate.cache?.set(cacheKey, ...)inmodule.parse.ts). Confirm whethercacheis truly guaranteed on everyStateinstance.If it is guaranteed, drop the optional chaining at call sites to match the type. If it is not always guaranteed, mark the field
cache?: BoundedCache<string, string>;so the type reflects the real contract and callers keep the guard intentionally instead of by habit.Also applies to: 385-385
🤖 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 `@packages/tempo/src/tempo.type.ts` at line 16, Align Internal.State.cache with its actual initialization contract: verify whether every State instance always provides a BoundedCache. If guaranteed, remove optional chaining from cache consumers such as the facade lookup and module.parse.ts update; otherwise mark cache optional in the State definition and retain the guarded accesses intentionally.
🤖 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/tempo/doc/2-core-concepts/tempo.cache.md`:
- Around line 83-90: The cache walkthrough in the parseAI example must use an
input that requires AI resolution rather than native parsing. Replace the date
string in both the parseAI call and subsequent Tempo lookup with an AI-required
example such as “The penultimate Tuesday before Thanksgiving in 2026,”
preserving the first-call and cached-second-call flow.
In `@packages/tempo/src/engine/engine.composer.ts`:
- Around line 54-60: Update the parsing logic around zdt so strings ending in Z
or containing numeric offsets are parsed with Temporal.Instant.from and
projected into the requested tz, preserving the exact instant. Keep bracketed
zone strings on Temporal.ZonedDateTime.from and retain PlainDateTime.from only
for wall-clock inputs.
In `@packages/tempo/src/support/support.cache.ts`:
- Around line 126-147: Update the JSDoc above SupportCache.clear to state that
omitting count clears all entries, including static entries; retain the existing
eviction behavior and implementation unchanged.
- Around line 205-216: Update the facade’s deletePrefix method to delegate to
the cache class’s existing deletePrefix implementation instead of iterating keys
and maintaining a duplicate count. Preserve the normalized prefix behavior and
return the class method’s count of matched and deleted entries, including the
existing no-cache result.
---
Outside diff comments:
In `@packages/plugins/ai/src/index.ts`:
- Around line 153-158: Update the anchor fallback setup in the surrounding
function to read merged settings from Tempo.options instead of Tempo.config when
resolving timeZone, calendar, locale, and sphere. Preserve explicit options
overrides and the existing anchorStr fallback, ensuring omitted settings use
resolved values rather than remaining undefined.
---
Nitpick comments:
In `@packages/tempo/src/support/support.cache.ts`:
- Around line 5-9: Update the BoundedCache get() method to refresh a
successfully retrieved entry’s recency, matching the existing set() reordering
behavior by updating its timestamp and Map insertion order. Preserve misses,
static/immortal entries, and TTL handling while ensuring frequently read keys
remain most recently used for eviction.
In `@packages/tempo/src/support/support.init.ts`:
- Around line 27-47: Extract the duplicated cache-option branching from init
into a shared resolveCache helper that handles BoundedCache values, Map-like
entries, and plain maxSize/ttl options while preserving existing defaults.
Replace both the idempotent runtime.state path and the non-idempotent state path
with resolveCache calls using their respective existing cache values, and remove
the duplicated inline logic.
In `@packages/tempo/src/tempo.type.ts`:
- Line 16: Align Internal.State.cache with its actual initialization contract:
verify whether every State instance always provides a BoundedCache. If
guaranteed, remove optional chaining from cache consumers such as the facade
lookup and module.parse.ts update; otherwise mark cache optional in the State
definition and retain the guarded accesses intentionally.
🪄 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: ae7ccd7d-7ad7-451a-b329-af51900adf24
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
package.jsonpackages/library/package.jsonpackages/plugins/ai/CHANGELOG.mdpackages/plugins/ai/package.jsonpackages/plugins/ai/src/index.tspackages/plugins/ai/src/parseAI.type.tspackages/plugins/ai/test/index.spec.tspackages/tempo/.vitepress/config.tspackages/tempo/.vitepress/theme/data/catalog.jsonpackages/tempo/CHANGELOG.mdpackages/tempo/doc/2-core-concepts/tempo.cache.mdpackages/tempo/package.jsonpackages/tempo/src/engine/engine.composer.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/support/support.cache.tspackages/tempo/src/support/support.enum.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/support.init.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tempo.version.tspackages/tempo/test/support/cache.test.ts
💤 Files with no reviewable changes (1)
- packages/plugins/ai/src/parseAI.type.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/tempo/src/tempo.version.ts
- packages/library/package.json
- packages/plugins/ai/package.json
- packages/tempo/src/support/support.index.ts
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)
packages/plugins/ai/package.json (1)
21-28: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore the main-branch publish guard for packages/plugins/ai.
prepublishOnlynow only builds, sonpm publishcan still run from a non-main branch. Other plugin packages requiregit rev-parse --abbrev-ref HEAD == mainbefore building; keep the same guard inpackages/plugins/ai/package.jsonor add an equivalent main-branch CI policy.🤖 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 `@packages/plugins/ai/package.json` around lines 21 - 28, Update the prepublishOnly script in packages/plugins/ai/package.json to restore the main-branch publish guard before running the build, matching the protection used by other plugin packages. Ensure npm publish is blocked when git rev-parse --abbrev-ref HEAD is not main.
🤖 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 @.github/ISSUE_TEMPLATE/bug_report_ai.yml:
- Around line 62-71: Update the “Minimal Reproduction Code” field description
and placeholder to explicitly warn reporters not to include API keys or private
endpoints, and replace the provider key example in the initAI() snippet with an
unmistakably redacted placeholder value.
In `@packages/plugins/ai/src/index.ts`:
- Around line 263-300: The provider response handling must replace, rather than
retain, prior rate-limit state. In the response logic around the rate-limit
header parsing and _state.limits assignment, build a response-local limits value
and assign it on every response, including when no usable headers are present.
Use that same local limits value for resetAt when constructing TempoAiError,
instead of reading stale _state.limits.
- Around line 271-281: The reset-token parser in the `resetTokHeader` handling
incorrectly accepts only the leading number from compound durations such as
4m12s. Replace the parseFloat-based conversion with full-duration validation and
parsing before constructing `Tempo`, or reject any string that is not fully
supported; ensure invalid or unsupported values do not update `resetAtTempo`.
---
Outside diff comments:
In `@packages/plugins/ai/package.json`:
- Around line 21-28: Update the prepublishOnly script in
packages/plugins/ai/package.json to restore the main-branch publish guard before
running the build, matching the protection used by other plugin packages. Ensure
npm publish is blocked when git rev-parse --abbrev-ref HEAD is not main.
🪄 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: e2e99f7b-dd1c-4a37-95b2-91bbb55c688a
📒 Files selected for processing (21)
.github/ISSUE_TEMPLATE/bug_report_ai.ymlpackages/plugins/ai/CHANGELOG.mdpackages/plugins/ai/README.mdpackages/plugins/ai/doc/architecture.mdpackages/plugins/ai/doc/index.mdpackages/plugins/ai/doc/rate-limits.mdpackages/plugins/ai/package.jsonpackages/plugins/ai/src/index.tspackages/plugins/ai/src/parseAI.type.tspackages/plugins/ai/test/index.spec.tspackages/plugins/vitest.shared.tspackages/tempo/CHANGELOG.mdpackages/tempo/doc/1-getting-started/tempo.cookbook.mdpackages/tempo/doc/2-core-concepts/tempo.cache.mdpackages/tempo/src/engine/engine.composer.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/support/support.cache.tspackages/tempo/src/support/support.init.tspackages/tempo/src/tempo.class.tspackages/tempo/test/discrete/parse.locale.test.tspackages/tempo/test/support/cache.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/tempo/test/support/cache.test.ts
- packages/tempo/test/discrete/parse.locale.test.ts
- packages/plugins/ai/doc/architecture.md
- packages/plugins/ai/CHANGELOG.md
- packages/tempo/src/support/support.init.ts
- packages/tempo/src/engine/engine.composer.ts
- packages/plugins/ai/README.md
- packages/plugins/ai/doc/index.md
- packages/tempo/CHANGELOG.md
- packages/tempo/src/module/module.parse.ts
- packages/tempo/doc/2-core-concepts/tempo.cache.md
- packages/plugins/ai/doc/rate-limits.md
- packages/tempo/src/tempo.class.ts
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/plugins/ai/test/index.spec.ts (1)
22-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
vi.restoreAllMocks()instead ofvi.clearAllMocks()forfetchspies.
vi.clearAllMocks()clears call history but keeps any scripted implementation, includingmockImplementation. The race-mode test at Line 252 setsfetchSpy.mockImplementation(...)onglobalThis.fetch. That implementation is not removed after the test, so a later test that spies onfetchagain and exhausts its queuedmockResolvedValueOncevalues falls through to the race test's body-parsing logic instead of a controlled response.Current tests happen to consume exactly as many queued once-values as fetch calls, so this does not fail today, but it is a fragile, unguarded state leak. Restore the spy in
afterEach.🧹 Proposed fix
afterEach(() => { - vi.clearAllMocks(); + vi.restoreAllMocks(); });🤖 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 `@packages/plugins/ai/test/index.spec.ts` around lines 22 - 24, Update the test cleanup in afterEach to call vi.restoreAllMocks() instead of vi.clearAllMocks(), ensuring the fetch spy’s mockImplementation and other altered behavior are restored between tests.
🧹 Nitpick comments (3)
packages/plugins/ai/src/core/init.ts (2)
96-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider a fallback for HTTP-date
Retry-Aftervalues.
parseResetHeaderToTempoonly matches plain numeric strings or compound durations (e.g.4m12s). Per HTTP semantics,Retry-After(also read at line 108) may instead be an HTTP-date string (e.g.Wed, 21 Oct 2026 07:28:00 GMT), which neither regex matches, so the function returnsnulland the reset time is silently dropped for providers/proxies using that format.Add a final fallback that attempts to parse the trimmed header with
Tempodirectly.♻️ Proposed fallback
} } + try { + const t = new Tempo(trimmed); + return t.isValid ? t : null; + } catch { + return null; + } - return null; }🤖 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 `@packages/plugins/ai/src/core/init.ts` around lines 96 - 101, Update parseResetHeaderToTempo to add a final fallback after the numeric and compound-duration parsing attempts: pass the trimmed Retry-After header directly to Tempo so HTTP-date values are converted into a reset time. Preserve the existing parsing behavior and return null only when all parsing approaches fail.
16-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate reserved-provider-id validation in two files. The same check (
RESERVED_PROVIDER_IDS.has(p.id.toLowerCase()), sameTempoAiErrormessage) is implemented independently incore/init.tsandfunctions/parse.ts. One shared helper keeps the validation rule and error message consistent if either changes.
packages/plugins/ai/src/core/init.ts#L16-L19: extract this check into a shared function (e.g.assertNoReservedProviderId(p.id)) incore/support.tsand call it here.packages/plugins/ai/src/functions/parse.ts#L91-L95: replace this loop's inline check with the same shared helper.🤖 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 `@packages/plugins/ai/src/core/init.ts` around lines 16 - 19, Duplicate reserved-provider-ID validation exists in two locations; centralize it in a shared helper to keep behavior and error messaging consistent. In packages/plugins/ai/src/core/init.ts lines 16-19, add and call an assertNoReservedProviderId helper from core/support.ts; in packages/plugins/ai/src/functions/parse.ts lines 91-95, replace the inline RESERVED_PROVIDER_IDS check with the same helper while preserving the existing TempoAiError behavior.packages/plugins/ai/src/functions/parse.ts (1)
107-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated JSON-cleanup logic.
rawContent.replace(/^```json\s*/i, '').replace(/\s*```$/i, '')is duplicated identically across the Fallback, Race, and Consensus branches. A shared helper keeps the markdown-fence stripping logic consistent if it needs to change later.♻️ Proposed helper
function stripJsonFence(raw: string): string { return raw.replace(/^```json\s*/i, '').replace(/\s*```$/i, ''); }Also applies to: 143-143, 156-156
🤖 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 `@packages/plugins/ai/src/functions/parse.ts` at line 107, Extract the duplicated JSON markdown-fence cleanup into a shared stripJsonFence helper in parse.ts, then replace the inline replace chains in the Fallback, Race, and Consensus branches with calls to that helper while preserving the existing behavior.
🤖 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 @.agent/rules/plan-execution.md:
- Around line 9-11: Constrain the /ok auto-execution workflow by validating
approval state, the identity of the currently approved plan, and each command’s
scope before enabling SafeToAutoRun or invoking run_command. Update
.agent/rules/plan-execution.md at lines 9-11 to require these runtime checks and
keep destructive or external-side-effect commands interactive unless explicitly
covered by the approved plan; update .agent/workflows/ok.md at lines 4-6 to bind
/ok execution to that same approved plan and validation flow.
In `@packages/plugins/ai/src/core/config.ts`:
- Around line 21-26: Canonicalize each provider ID before looking it up in
DEFAULT_PROVIDERS within initAI, reusing the lowercase form already used for
RESERVED_PROVIDER_IDS checks. Use that normalized ID for default endpoint,
model, and token parameter selection while preserving the original provider ID
where it is needed for output or registration.
- Around line 37-41: Update the Gemini provider entry in the configuration to
use the current supported default model instead of gemini-1.5-flash, using
gemini-3.6-flash or the repository’s equivalent current model. Add a
default-provider request test covering the Gemini configuration and verifying it
uses the updated model.
In `@packages/plugins/ai/src/core/init.ts`:
- Around line 131-135: Change updateRateLimitsFromResponse and the
fetchFromProvider flow so provider responses only produce rate-limit data
without immediately overwriting shared _state.limits. In the Race and Consensus
paths in parseAI, apply the limits from the response associated with the
successful or selected consensus result after that result is determined, while
preserving getAiRateLimits() behavior for the returned parse result.
- Around line 16-25: Normalize p.id to lowercase once within the
resolvedProviders mapping and reuse that normalized identifier for both
RESERVED_PROVIDER_IDS validation and DEFAULT_PROVIDERS lookup, while preserving
the existing fallback and returned provider fields.
In `@packages/plugins/ai/src/core/support.ts`:
- Around line 30-40: Update the virtual 'ai' descriptor returned by
getOwnPropertyDescriptor in attachAiMeta to use configurable: true, while
preserving its enumerable status, frozenMeta value, and non-writable setting;
leave the get and ownKeys behavior unchanged.
In `@packages/plugins/ai/src/functions/context.ts`:
- Around line 22-24: Do not publish AI entrypoints that always reject valid
calls: implement contextAI in
packages/plugins/ai/src/functions/context.ts:22-24, extractAI in
packages/plugins/ai/src/functions/extract.ts:26-28, formatAI in
packages/plugins/ai/src/functions/format.ts:20-22, recurrenceAI in
packages/plugins/ai/src/functions/recurrence.ts:25-27, and scheduleAI in
packages/plugins/ai/src/functions/schedule.ts:24-26, or remove each function’s
public export and corresponding documentation before release.
In `@packages/plugins/ai/src/functions/parse.ts`:
- Around line 60-82: Update the native fast-path in the parseAI flow to detect
parsed matches using Tempo’s documented public parse/cache API instead of
Symbol.for('$Tempo.internal') or $Internal?.(). Ensure natural-language inputs
with valid native matches are accepted, while preserving the existing ISO-date
fallback, validation, metadata attachment, and LLM fallback behavior.
In `@packages/plugins/ai/test/index.spec.ts`:
- Around line 415-495: Fix the describe nesting around the rate-limit tests by
keeping the outer AI Parsing Plugin suite open while describe('Rate Limit &
Reset Header Parsing Hardening', ...) runs. Move the existing outer-suite
closing `});` to after the rate-limit suite’s closing block so its beforeEach
and afterEach hooks apply to these tests.
---
Outside diff comments:
In `@packages/plugins/ai/test/index.spec.ts`:
- Around line 22-24: Update the test cleanup in afterEach to call
vi.restoreAllMocks() instead of vi.clearAllMocks(), ensuring the fetch spy’s
mockImplementation and other altered behavior are restored between tests.
---
Nitpick comments:
In `@packages/plugins/ai/src/core/init.ts`:
- Around line 96-101: Update parseResetHeaderToTempo to add a final fallback
after the numeric and compound-duration parsing attempts: pass the trimmed
Retry-After header directly to Tempo so HTTP-date values are converted into a
reset time. Preserve the existing parsing behavior and return null only when all
parsing approaches fail.
- Around line 16-19: Duplicate reserved-provider-ID validation exists in two
locations; centralize it in a shared helper to keep behavior and error messaging
consistent. In packages/plugins/ai/src/core/init.ts lines 16-19, add and call an
assertNoReservedProviderId helper from core/support.ts; in
packages/plugins/ai/src/functions/parse.ts lines 91-95, replace the inline
RESERVED_PROVIDER_IDS check with the same helper while preserving the existing
TempoAiError behavior.
In `@packages/plugins/ai/src/functions/parse.ts`:
- Line 107: Extract the duplicated JSON markdown-fence cleanup into a shared
stripJsonFence helper in parse.ts, then replace the inline replace chains in the
Fallback, Race, and Consensus branches with calls to that helper while
preserving the existing behavior.
🪄 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: 594c9ded-1be8-4b66-b9b4-b9e55615d44a
📒 Files selected for processing (23)
.agent/rules/plan-execution.md.agent/workflows/ok.md.github/ISSUE_TEMPLATE/bug_report_ai.ymlpackages/plugins/ai/CHANGELOG.mdpackages/plugins/ai/package.jsonpackages/plugins/ai/src/core/config.tspackages/plugins/ai/src/core/error.tspackages/plugins/ai/src/core/init.tspackages/plugins/ai/src/core/support.tspackages/plugins/ai/src/core/types.tspackages/plugins/ai/src/functions/context.tspackages/plugins/ai/src/functions/diff.tspackages/plugins/ai/src/functions/extract.tspackages/plugins/ai/src/functions/format.tspackages/plugins/ai/src/functions/parse.tspackages/plugins/ai/src/functions/recurrence.tspackages/plugins/ai/src/functions/schedule.tspackages/plugins/ai/src/index.tspackages/plugins/ai/test/benchmark.spec.tspackages/plugins/ai/test/index.spec.tspackages/plugins/ai/tsconfig.jsonpackages/plugins/ai/tsup.config.tspackages/tempo/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/plugins/ai/tsup.config.ts
- packages/plugins/ai/tsconfig.json
1 participant
Summary by CodeRabbit