SF-30 add runtime shared packages#84
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:
📝 WalkthroughWalkthroughAdds six new packages— Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Deliver as deliverWebhook
participant FetchAPI as fetch
participant Remote as RemoteServer
Caller->>Deliver: deliverWebhook(input, options)
Deliver->>Deliver: serialize payload, build headers (incl. signature)
Deliver->>FetchAPI: fetch(request, { signal })
FetchAPI->>Remote: HTTP request
Remote-->>FetchAPI: HTTP response / error / timeout
FetchAPI-->>Deliver: response or error
alt response.ok (2xx)
Deliver->>Caller: return { ok: true, attempts: [...] }
else retryable (429 or 5xx or transient error)
Deliver->>Deliver: wait exponential backoff, retry up to maxRetries
Deliver->>Caller: return aggregated failure attempts after retries
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Reviewer's GuideIntroduces a set of shared runtime packages (config, envelope, errors, metrics, queue, webhooks) into the monorepo, each as its own TypeScript package with basic implementation, build/test configuration, and package metadata, intended for reuse by downstream functions. Sequence diagram for webhook delivery with signing and retriessequenceDiagram
actor Service
participant WebhooksModule
participant Fetch as FetchAPI
participant Endpoint as WebhookEndpoint
Service->>WebhooksModule: deliverWebhook(input, options)
WebhooksModule->>WebhooksModule: JSON.stringify(input.payload)
alt secret provided
WebhooksModule->>WebhooksModule: signWebhookPayload(payloadString, secret, options)
WebhooksModule->>Fetch: fetch(url, signedRequest)
else no secret
WebhooksModule->>Fetch: fetch(url, unsignedRequest)
end
loop up to maxRetries
Fetch-->>WebhooksModule: Response
alt response.ok
WebhooksModule-->>Service: WebhookDeliveryResult(ok=true, attempts, responseStatus)
else non ok or error
WebhooksModule->>WebhooksModule: record failed attempt
alt more retries remaining
WebhooksModule->>WebhooksModule: sleep(backoffDelay)
WebhooksModule->>Fetch: fetch(url, request)
else no retries left
WebhooksModule-->>Service: WebhookDeliveryResult(ok=false, attempts, responseStatus)
end
end
end
Fetch->>Endpoint: HTTP request with JSON body
Class diagram for shared envelope packageclassDiagram
class EnvelopeMeta {
+string timestamp
}
class CanonicalEnvelopeError {
+string code
+string message
+number status
+boolean retryable
+Record~string, unknown~ details
}
class SuccessEnvelope~T~ {
+true ok
+T data
+EnvelopeMeta meta
}
class ErrorEnvelope {
+false ok
+CanonicalEnvelopeError error
+EnvelopeMeta meta
}
class Envelope~T~ {
}
class LegacySuccessEnvelope~T~ {
}
class LegacyErrorEnvelope {
}
class EnvelopeModule {
+ok~T~(data T, options timestampOptions) SuccessEnvelope~T~
+err(input CanonicalLikeErrorInput, options timestampOptions) ErrorEnvelope
+normalizeLegacyEnvelope~T~(envelope LegacySuccessEnvelope~T~ | LegacyErrorEnvelope, options normalizeOptions) Envelope~T~
+toLegacyDataEnvelope~T~(envelope Envelope~T~) LegacyDataEnvelope~T~
+toLegacyResultEnvelope~T~(envelope Envelope~T~) LegacyResultEnvelope~T~
+isValidErrorShape(input CanonicalLikeErrorInput) boolean
}
class CanonicalLikeErrorInput {
+string code
+string message
+number status
+boolean retryable
+Record~string, unknown~ details
}
class timestampOptions {
+string timestamp
}
class normalizeOptions {
+string timestamp
+number defaultStatus
+boolean defaultRetryable
}
class LegacyDataEnvelope~T~ {
+boolean ok
+T data
}
class LegacyResultEnvelope~T~ {
+boolean ok
+T result
}
Envelope~T~ <|-- SuccessEnvelope~T~
Envelope~T~ <|-- ErrorEnvelope
ErrorEnvelope --> CanonicalEnvelopeError
SuccessEnvelope~T~ --> EnvelopeMeta
ErrorEnvelope --> EnvelopeMeta
EnvelopeModule --> EnvelopeMeta
EnvelopeModule --> CanonicalEnvelopeError
EnvelopeModule --> SuccessEnvelope~T~
EnvelopeModule --> ErrorEnvelope
EnvelopeModule --> LegacySuccessEnvelope~T~
EnvelopeModule --> LegacyErrorEnvelope
EnvelopeModule --> LegacyDataEnvelope~T~
EnvelopeModule --> LegacyResultEnvelope~T~
EnvelopeModule --> CanonicalLikeErrorInput
EnvelopeModule --> normalizeOptions
EnvelopeModule --> timestampOptions
Class diagram for shared errors packageclassDiagram
class ErrorDefinition {
+string code
+number httpStatus
+boolean retryable
+string defaultMessage
}
class SuperfunctionError {
+string code
+number status
+boolean retryable
+Record~string, unknown~ details
+constructor(input SuperfunctionErrorInput)
}
class SuperfunctionErrorInput {
+string code
+string message
+number status
+boolean retryable
+Record~string, unknown~ details
}
class ErrorRegistry {
<<interface>>
+register(definition ErrorDefinition) void
+registerMany(definitions ErrorDefinition[]) void
+has(code string) boolean
+resolve(code string) ErrorDefinition
+resolveOrDefault(code string, fallback ErrorDefinitionWithoutCode) ErrorDefinition
+list() ErrorDefinition[]
}
class ErrorRegistryImpl {
-Map~string, ErrorDefinition~ definitions
+register(definition ErrorDefinition) void
+registerMany(definitions ErrorDefinition[]) void
+has(code string) boolean
+resolve(code string) ErrorDefinition
+resolveOrDefault(code string, fallback ErrorDefinitionWithoutCode) ErrorDefinition
+list() ErrorDefinition[]
}
class ErrorDefinitionWithoutCode {
+number httpStatus
+boolean retryable
+string defaultMessage
}
class ErrorModule {
+createErrorRegistry(initialDefinitions ErrorDefinition[]) ErrorRegistry
+createTypedError(input TypedErrorInput) SuperfunctionError
}
class TypedErrorInput {
+string code
+string message
+Record~string, unknown~ details
+ErrorRegistry registry
}
ErrorRegistry <|.. ErrorRegistryImpl
ErrorRegistryImpl --> ErrorDefinition
ErrorRegistryImpl --> SuperfunctionError
SuperfunctionError --> SuperfunctionErrorInput
ErrorModule --> ErrorRegistry
ErrorModule --> ErrorRegistryImpl
ErrorModule --> SuperfunctionError
ErrorModule --> ErrorDefinition
ErrorModule --> TypedErrorInput
Class diagram for shared metrics packageclassDiagram
class MetricsEmitter {
<<interface>>
+track(event string, properties Record~string, unknown~) void
}
class MetricsEmitterImpl {
-trackFn(event string, properties Record~string, unknown~) void
+track(event string, properties Record~string, unknown~) void
}
class NoopMetricsEmitter {
+track(event string, properties Record~string, unknown~) void
}
class MetricsModule {
+createMetricsEmitter(trackFn function) MetricsEmitter
+createNamespacedEmitter(namespace string, emitter MetricsEmitter) MetricsEmitter
}
class NamespacedEmitter {
-string namespace
-MetricsEmitter innerEmitter
+track(event string, properties Record~string, unknown~) void
}
MetricsEmitter <|.. MetricsEmitterImpl
MetricsEmitter <|.. NoopMetricsEmitter
MetricsEmitter <|.. NamespacedEmitter
MetricsModule --> MetricsEmitter
MetricsModule --> MetricsEmitterImpl
MetricsModule --> NoopMetricsEmitter
MetricsModule --> NamespacedEmitter
Class diagram for shared config packageclassDiagram
class ConfigError {
+"ENV_VALUE_INVALID" | "ENV_VALUE_OUT_OF_RANGE" code
+constructor(code "ENV_VALUE_INVALID" | "ENV_VALUE_OUT_OF_RANGE", message string)
}
class ConfigModule {
+readIntEnv(key string, options IntEnvOptions) number
+readIntEnvOrDefault(key string, options IntEnvOptions) number
+readStringEnv(key string, options StringEnvOptions) string
+readBooleanEnv(key string, options BooleanEnvOptions) boolean
}
class IntEnvOptions {
+number defaultValue
+number min
+number max
+NodeJS.ProcessEnv env
}
class StringEnvOptions {
+string defaultValue
+boolean allowEmpty
+NodeJS.ProcessEnv env
}
class BooleanEnvOptions {
+boolean defaultValue
+NodeJS.ProcessEnv env
}
ConfigModule --> ConfigError
ConfigModule --> IntEnvOptions
ConfigModule --> StringEnvOptions
ConfigModule --> BooleanEnvOptions
Class diagram for shared queue packageclassDiagram
class QueueAdapter~TPayload~ {
<<interface>>
+enqueue(queueName string, payload TPayload) Promise~void~
+dequeue(queueName string) Promise~TPayload | null~
+dequeueMatching(queueName string, matcher function) Promise~TPayload | null~
+peek(queueName string) TPayload[]
+size(queueName string) number
}
class MemoryQueueAdapter~TPayload~ {
-Map~string, TPayload[]~ queues
+enqueue(queueName string, payload TPayload) Promise~void~
+dequeue(queueName string) Promise~TPayload | null~
+dequeueMatching(queueName string, matcher function) Promise~TPayload | null~
+peek(queueName string) TPayload[]
+size(queueName string) number
}
class FlowFnQueueAdapterConfig {
+string baseUrl
+string apiKey
+fetch function
}
class FlowFnQueueAdapter~TPayload~ {
-FlowFnQueueAdapterConfig config
-fetcher function
+constructor(config FlowFnQueueAdapterConfig)
+enqueue(queueName string, payload TPayload) Promise~void~
+dequeue(queueName string) Promise~TPayload | null~
+dequeueMatching(queueName string, matcher function) Promise~TPayload | null~
+peek(queueName string) TPayload[]
+size(queueName string) number
}
QueueAdapter~TPayload~ <|.. MemoryQueueAdapter~TPayload~
QueueAdapter~TPayload~ <|.. FlowFnQueueAdapter~TPayload~
FlowFnQueueAdapter~TPayload~ --> FlowFnQueueAdapterConfig
Class diagram for shared webhooks packageclassDiagram
class WebhookSignatureOptions {
+"sha256" | "sha1" | "sha512" algorithm
+"hex" | "base64" encoding
+string prefix
}
class WebhookDeliveryInput {
+string url
+unknown payload
+string secret
+string signatureHeader
+Record~string, string~ headers
+"POST" | "PUT" method
}
class WebhookDeliveryAttempt {
+number attempt
+boolean ok
+number status
+string error
}
class WebhookDeliveryResult {
+boolean ok
+WebhookDeliveryAttempt[] attempts
+number responseStatus
}
class WebhookDeliveryOptions {
+fetch function
+number maxRetries
+number initialDelayMs
+number maxDelayMs
}
class WebhooksModule {
+signWebhookPayload(payload string, secret string, options WebhookSignatureOptions) string
+verifyWebhookSignature(payload string, providedSignature string, secret string, options WebhookSignatureOptions) boolean
+deliverWebhook(input WebhookDeliveryInput, options WebhookDeliveryOptions) Promise~WebhookDeliveryResult~
}
WebhooksModule --> WebhookSignatureOptions
WebhooksModule --> WebhookDeliveryInput
WebhooksModule --> WebhookDeliveryResult
WebhookDeliveryResult --> WebhookDeliveryAttempt
WebhooksModule --> WebhookDeliveryOptions
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Review Summary by QodoAdd runtime shared packages (config, envelope, errors, metrics, queue, webhooks)
WalkthroughsDescription• Add six shared runtime packages to monorepo foundation layer • Implement config package with typed environment variable readers • Implement envelope package with canonical response structures • Implement errors package with typed error registry system • Implement metrics package with emitter abstraction • Implement queue package with memory and FlowFn adapters • Implement webhooks package with signing, verification, and delivery Diagramflowchart LR
A["Shared Packages"] --> B["config"]
A --> C["envelope"]
A --> D["errors"]
A --> E["metrics"]
A --> F["queue"]
A --> G["webhooks"]
B --> B1["Env Readers"]
C --> C1["Response Envelopes"]
D --> D1["Error Registry"]
E --> E1["Metrics Emitter"]
F --> F1["Memory & FlowFn Adapters"]
G --> G1["HMAC Signing & Delivery"]
File Changes1. packages/config/src/index.ts
|
Code Review by Qodo
1.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
FlowFnQueueAdapter,peekandsizeare currently hard-coded to return empty/zero, which makesdequeueMatchingeffectively non-functional; consider either implementing these against the FlowFn API or explicitly throwingUnsupportederrors so callers don’t assume matching dequeue semantics that aren’t actually provided.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `FlowFnQueueAdapter`, `peek` and `size` are currently hard-coded to return empty/zero, which makes `dequeueMatching` effectively non-functional; consider either implementing these against the FlowFn API or explicitly throwing `Unsupported` errors so callers don’t assume matching dequeue semantics that aren’t actually provided.
## Individual Comments
### Comment 1
<location path="packages/queue/src/index.ts" line_range="27-21" />
<code_context>
+ return queue.shift() ?? null;
+ }
+
+ async dequeueMatching(queueName: string, matcher: (payload: TPayload) => boolean): Promise<TPayload | null> {
+ const queue = this.queues.get(queueName);
+ if (!queue || queue.length === 0) {
+ return null;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** FlowFnQueueAdapter.dequeueMatching never matches because peek() always returns an empty array.
Since this behavior effectively disables `dequeueMatching`, consider either:
- Throwing an explicit "not supported" error from `dequeueMatching` for this adapter, or
- Implementing a minimal supported version (e.g., a single `dequeue` followed by applying the matcher) instead of iterating over an empty list.
This makes the limitation/behavior clear to callers and avoids a silent no-op.
</issue_to_address>
### Comment 2
<location path="packages/queue/src/index.ts" line_range="113-114" />
<code_context>
+ return null;
+ }
+
+ peek(_queueName: string): TPayload[] {
+ return [];
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** FlowFnQueueAdapter.peek/size are effectively unimplemented and may surprise callers expecting the QueueAdapter contract.
For this adapter, having `peek` always return `[]` (and `size` always `0`) is likely to cause subtle bugs if callers rely on these methods for control flow or monitoring, assuming they reflect remote state.
If these operations aren’t truly supported by FlowFn, consider either throwing an explicit `Unsupported operation for FlowFnQueueAdapter` error from `peek`/`size`, or updating the adapter contract so these methods are optional and clearly documented as such.
Suggested implementation:
```typescript
peek(_queueName: string): TPayload[] {
throw new Error('Unsupported operation for FlowFnQueueAdapter: peek');
}
```
```typescript
size(_queueName: string): number {
throw new Error('Unsupported operation for FlowFnQueueAdapter: size');
}
```
</issue_to_address>
### Comment 3
<location path="packages/config/src/index.ts" line_range="21-30" />
<code_context>
+ return options.defaultValue;
+ }
+
+ const parsed = Number.parseInt(raw, 10);
+ if (!Number.isFinite(parsed)) {
+ throw new ConfigError('ENV_VALUE_INVALID', `${key} must be an integer`);
+ }
+
+ if (options.min !== undefined && parsed < options.min) {
+ throw new ConfigError('ENV_VALUE_OUT_OF_RANGE', `${key} must be >= ${options.min}`);
+ }
+
+ if (options.max !== undefined && parsed > options.max) {
+ throw new ConfigError('ENV_VALUE_OUT_OF_RANGE', `${key} must be <= ${options.max}`);
+ }
+
+ return parsed;
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using parseInt allows partially numeric strings (e.g. '10px') to be treated as valid integers.
Because `readIntEnv` uses `Number.parseInt(raw, 10)` and only checks `Number.isFinite(parsed)`, inputs like `'10px'` or `'5.5'` are accepted as `10` and `5`. For stricter behavior, validate the full string (e.g. `/^-?\d+$/`) before parsing, or use `Number(raw)` and then `Number.isInteger(value)`, so non-integer or partially numeric values correctly trigger `ENV_VALUE_INVALID` instead of being coerced.
Suggested implementation:
```typescript
if (raw === undefined || raw === '') {
return options.defaultValue;
}
const parsed = Number(raw);
if (!Number.isInteger(parsed)) {
throw new ConfigError('ENV_VALUE_INVALID', `${key} must be an integer`);
}
if (options.min !== undefined && parsed < options.min) {
throw new ConfigError('ENV_VALUE_OUT_OF_RANGE', `${key} must be >= ${options.min}`);
}
if (options.max !== undefined && parsed > options.max) {
throw new ConfigError('ENV_VALUE_OUT_OF_RANGE', `${key} must be <= ${options.max}`);
}
return parsed;
}
constructor(code: 'ENV_VALUE_INVALID' | 'ENV_VALUE_OUT_OF_RANGE', message: string) {
```
I inferred the surrounding context from the snippet. If the original implementation of `readIntEnv` appears elsewhere in the file (e.g. a previous version using `Number.parseInt`), you should also replace that occurrence in the same way: use `const parsed = Number(raw);` followed by `if (!Number.isInteger(parsed))` and keep the existing range checks. Ensure there is only a single `readIntEnv` definition exported from this file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4981acee16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| peek(_queueName: string): TPayload[] { | ||
| return []; |
There was a problem hiding this comment.
dequeueMatching is effectively non-functional in FlowFnQueueAdapter because it relies on peek, and this implementation hardcodes peek to return an empty array, so dequeueMatching always returns null even when matching jobs exist remotely. Callers that depend on predicate-based dequeue will silently skip work; this should either be implemented against the backend API or explicitly throw unsupported instead of returning null.
Useful? React with 👍 / 👎.
| const parsed = Number.parseInt(raw, 10); | ||
| if (!Number.isFinite(parsed)) { |
There was a problem hiding this comment.
Using Number.parseInt here accepts malformed values with trailing text (for example, "2000ms" parses as 2000), so invalid environment values can be treated as valid and alter runtime behavior instead of throwing ENV_VALUE_INVALID. Since this helper claims to read integer env values, it should reject non-integer strings before parsing.
Useful? React with 👍 / 👎.
| const decoded = Buffer.from(value, 'base64'); | ||
| return decoded.length > 0 ? decoded : null; |
There was a problem hiding this comment.
In the base64 branch, Buffer.from(value, 'base64') is permissive and ignores invalid trailing characters, so a tampered signature like <validDigest>!! can decode to the same bytes and still pass timingSafeEqual when encoding: 'base64' is used. This weakens webhook signature validation in that mode; enforce strict base64 format (including padding) prior to decoding.
Useful? React with 👍 / 👎.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| async dequeueMatching(queueName: string, matcher: (payload: TPayload) => boolean): Promise<TPayload | null> { | ||
| const queued = this.peek(queueName); | ||
| for (const payload of queued) { | ||
| if (matcher(payload)) { | ||
| const claimed = await this.dequeue(queueName); | ||
| if (claimed) { | ||
| return claimed; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| peek(_queueName: string): TPayload[] { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
The dequeueMatching method in FlowFnQueueAdapter will never work because it calls this.peek(queueName) which always returns an empty array [] (line 113-115). This means the for loop will never execute and the method will always return null, even if matching items exist in the queue.
This is a critical bug that breaks the functionality of dequeueMatching for the FlowFn adapter. Either:
- Implement
peekto make an API call to retrieve queued items, or - Document that
dequeueMatchingis not supported for FlowFn and throw an error:
async dequeueMatching(queueName: string, matcher: (payload: TPayload) => boolean): Promise<TPayload | null> {
throw new Error('dequeueMatching is not supported for FlowFnQueueAdapter');
}| async dequeueMatching(queueName: string, matcher: (payload: TPayload) => boolean): Promise<TPayload | null> { | |
| const queued = this.peek(queueName); | |
| for (const payload of queued) { | |
| if (matcher(payload)) { | |
| const claimed = await this.dequeue(queueName); | |
| if (claimed) { | |
| return claimed; | |
| } | |
| } | |
| } | |
| return null; | |
| } | |
| peek(_queueName: string): TPayload[] { | |
| return []; | |
| } | |
| async dequeueMatching(queueName: string, matcher: (payload: TPayload) => boolean): Promise<TPayload | null> { | |
| throw new Error('dequeueMatching is not supported for FlowFnQueueAdapter'); | |
| } | |
| peek(_queueName: string): TPayload[] { | |
| return []; | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/webhooks/src/index.ts">
<violation number="1" location="packages/webhooks/src/index.ts:92">
P2: Handle `JSON.stringify` failures before signing or sending the webhook body. As written, some valid `unknown` inputs make `deliverWebhook` throw before it can return a `WebhookDeliveryResult`.</violation>
<violation number="2" location="packages/webhooks/src/index.ts:163">
P2: `Buffer.from(value, 'base64')` silently strips non-base64 characters before decoding, so a tampered signature like `<validDigest>!!` decodes to the same bytes as the clean digest and passes `timingSafeEqual`. When `encoding: 'base64'` is used, validate that the input is strictly well-formed base64 (e.g. `/^[A-Za-z0-9+/]*={0,2}$/`) before decoding to prevent signature bypass.</violation>
</file>
<file name="packages/envelope/src/index.ts">
<violation number="1" location="packages/envelope/src/index.ts:39">
P1: `err()` advertises `status` and `retryable` as optional, but omitting either one replaces the caller's error with `INVALID_ENVELOPE_ERROR_SHAPE`. Make the parameters required here, or default them before validation, so valid calls don't silently change error semantics.</violation>
<violation number="2" location="packages/envelope/src/index.ts:108">
P2: Preserve `meta.timestamp` when normalizing an already-canonical envelope. Rebuilding it through `ok()`/`err()` restamps the envelope and changes the original event time on pass-through inputs.</violation>
</file>
<file name="packages/queue/src/index.ts">
<violation number="1" location="packages/queue/src/index.ts:114">
P1: `peek()` always returns an empty array, so `dequeueMatching()` can never match any queued item in the FlowFn adapter.</violation>
</file>
<file name="packages/metrics/src/index.ts">
<violation number="1" location="packages/metrics/src/index.ts:23">
P2: Reject namespaces that start or end with `.`. The current guard still allows malformed names like `recfn..event` through.</violation>
</file>
<file name="packages/config/src/index.ts">
<violation number="1" location="packages/config/src/index.ts:21">
P1: `parseInt()` accepts malformed values like `"1.5"` or `"123abc"`, so this can silently use the wrong configuration instead of rejecting a non-integer env value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryThis PR introduces 6 new shared runtime packages ( Key findings:
Confidence Score: 2/5Not safe to merge — FlowFnQueueAdapter.dequeueMatching is completely non-functional and the metrics namespace validation has a silent correctness gap Two confirmed logic bugs that cause silent incorrect runtime behavior bring the score down significantly: (1) the queue adapter's dequeueMatching always returning null due to a peek() stub, and (2) the metrics namespace validation allowing trailing/leading dots that produce malformed double-dot event names. The envelope err() API surprise (optional-typed fields that silently replace the caller's error code) adds further risk. The config and errors packages are solid and would score 5/5 independently. packages/queue/src/index.ts and packages/metrics/src/index.ts require fixes before merge; packages/envelope/src/index.ts needs API clarification or signature hardening Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Downstream Functions] --> B[@superfunctions/config]
A --> C[@superfunctions/envelope]
A --> D[@superfunctions/errors]
A --> E[@superfunctions/metrics]
A --> F[@superfunctions/queue]
A --> G[@superfunctions/webhooks]
D -->|createTypedError uses| C
F -->|MemoryQueueAdapter| H[In-Process Memory]
F -->|FlowFnQueueAdapter| I[FlowFn Remote API]
I -.->|dequeueMatching broken: peek always returns empty| X[❌ Always returns null]
E -->|createNamespacedEmitter| Y[Event Name Builder]
Y -.->|trailing dot namespace: foo. + event → foo..event| Z[⚠️ Malformed event names]
G -->|signWebhookPayload / verifyWebhookSignature| J[node:crypto HMAC]
G -->|deliverWebhook| K[HTTP POST + Linear Retry]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/queue/src/index.ts
Line: 99-111
Comment:
**`dequeueMatching` always returns `null` for `FlowFnQueueAdapter`**
`this.peek(queueName)` is called on line 100, but `FlowFnQueueAdapter.peek()` is a stub that unconditionally returns `[]` (lines 113–115). This means the `for` loop body is never entered and `dequeueMatching` silently returns `null` for every call on the remote adapter — regardless of what is actually in the remote queue. There is no test covering this code path.
To confirm: `MemoryQueueAdapter.peek()` correctly returns a copy of the internal queue, so only the remote adapter is broken.
Options to fix:
- **Best-effort approach:** repeatedly call `dequeue`, re-enqueue non-matching items, and return the first match (though not atomic).
- **Server-side filter endpoint:** expose a `/queues/{name}/dequeue/matching` endpoint on FlowFn that keeps the match-and-claim operation atomic.
As-is, any consumer of `QueueAdapter.dequeueMatching` that receives a `FlowFnQueueAdapter` will silently get `null` with no error.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/metrics/src/index.ts
Line: 22-24
Comment:
**Namespace validation misses trailing and leading dots**
The guard `normalized.includes('..')` only rejects strings that already contain a consecutive double-dot, but a namespace with a **trailing dot** (e.g. `"foo."`) passes the check. When that namespace is used to build an event name, the template `` `${normalized}.${event}` `` produces `"foo..bar"` — the exact double-dot form the validation is meant to prevent.
The same applies to a **leading dot**: `".foo"` produces event names like `".foo.bar"`.
Suggested fix:
```suggestion
const normalized = namespace.trim();
if (!normalized || normalized.includes('..') || normalized.startsWith('.') || normalized.endsWith('.')) {
throw new Error('METRIC_NAMESPACE_INVALID');
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/envelope/src/index.ts
Line: 55-85
Comment:
**`err()` silently substitutes the caller's error when `status`/`retryable` are omitted**
The TypeScript signature marks `status` and `retryable` as *optional*:
```ts
input: { code: string; message: string; status?: number; retryable?: boolean; ... }
```
However, if either field is absent, `isValidErrorShape` returns `false` and the function silently discards the caller's error, returning a *different* envelope with `code: 'INVALID_ENVELOPE_ERROR_SHAPE'`. A caller who writes:
```ts
err({ code: 'RATE_LIMIT_EXCEEDED', message: 'Too many requests' })
```
gets back an envelope whose `error.code` is `'INVALID_ENVELOPE_ERROR_SHAPE'`, not `'RATE_LIMIT_EXCEEDED'`. This silent substitution is confirmed by the test at `envelope.test.ts:21-26` and is very easy to miss in production.
Recommended options:
- Make `status` and `retryable` **required** fields (compile-time safety, minimal change).
- Apply safe defaults (e.g. `status ?? 500`, `retryable ?? false`) similar to what `normalizeLegacyEnvelope` already does on lines 118–119.
- Throw explicitly so failures are never silent.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/webhooks/src/index.ts
Line: 141
Comment:
**Linear backoff instead of exponential backoff**
`initialDelayMs * attempt` produces **linear** growth (1 s → 2 s → 3 s…). Standard exponential backoff (`initialDelayMs * 2 ** (attempt - 1)`) grows 1 s → 2 s → 4 s…, providing much stronger back-pressure against a struggling upstream. The existence of `maxDelayMs` as a cap strongly implies exponential growth was the intent.
```suggestion
const delay = Math.min(maxDelayMs, initialDelayMs * 2 ** (attempt - 1));
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "SF-30 add runtime shared packages" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async dequeueMatching(queueName: string, matcher: (payload: TPayload) => boolean): Promise<TPayload | null> { | ||
| const queued = this.peek(queueName); | ||
| for (const payload of queued) { | ||
| if (matcher(payload)) { | ||
| const claimed = await this.dequeue(queueName); | ||
| if (claimed) { | ||
| return claimed; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
dequeueMatching always returns null for FlowFnQueueAdapter
this.peek(queueName) is called on line 100, but FlowFnQueueAdapter.peek() is a stub that unconditionally returns [] (lines 113–115). This means the for loop body is never entered and dequeueMatching silently returns null for every call on the remote adapter — regardless of what is actually in the remote queue. There is no test covering this code path.
To confirm: MemoryQueueAdapter.peek() correctly returns a copy of the internal queue, so only the remote adapter is broken.
Options to fix:
- Best-effort approach: repeatedly call
dequeue, re-enqueue non-matching items, and return the first match (though not atomic). - Server-side filter endpoint: expose a
/queues/{name}/dequeue/matchingendpoint on FlowFn that keeps the match-and-claim operation atomic.
As-is, any consumer of QueueAdapter.dequeueMatching that receives a FlowFnQueueAdapter will silently get null with no error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/queue/src/index.ts
Line: 99-111
Comment:
**`dequeueMatching` always returns `null` for `FlowFnQueueAdapter`**
`this.peek(queueName)` is called on line 100, but `FlowFnQueueAdapter.peek()` is a stub that unconditionally returns `[]` (lines 113–115). This means the `for` loop body is never entered and `dequeueMatching` silently returns `null` for every call on the remote adapter — regardless of what is actually in the remote queue. There is no test covering this code path.
To confirm: `MemoryQueueAdapter.peek()` correctly returns a copy of the internal queue, so only the remote adapter is broken.
Options to fix:
- **Best-effort approach:** repeatedly call `dequeue`, re-enqueue non-matching items, and return the first match (though not atomic).
- **Server-side filter endpoint:** expose a `/queues/{name}/dequeue/matching` endpoint on FlowFn that keeps the match-and-claim operation atomic.
As-is, any consumer of `QueueAdapter.dequeueMatching` that receives a `FlowFnQueueAdapter` will silently get `null` with no error.
How can I resolve this? If you propose a fix, please make it concise.| const normalized = namespace.trim(); | ||
| if (!normalized || normalized.includes('..')) { | ||
| throw new Error('METRIC_NAMESPACE_INVALID'); |
There was a problem hiding this comment.
The guard normalized.includes('..') only rejects strings that already contain a consecutive double-dot, but a namespace with a trailing dot (e.g. "foo.") passes the check. When that namespace is used to build an event name, the template `${normalized}.${event}` produces "foo..bar" — the exact double-dot form the validation is meant to prevent.
The same applies to a leading dot: ".foo" produces event names like ".foo.bar".
Suggested fix:
| const normalized = namespace.trim(); | |
| if (!normalized || normalized.includes('..')) { | |
| throw new Error('METRIC_NAMESPACE_INVALID'); | |
| const normalized = namespace.trim(); | |
| if (!normalized || normalized.includes('..') || normalized.startsWith('.') || normalized.endsWith('.')) { | |
| throw new Error('METRIC_NAMESPACE_INVALID'); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/metrics/src/index.ts
Line: 22-24
Comment:
**Namespace validation misses trailing and leading dots**
The guard `normalized.includes('..')` only rejects strings that already contain a consecutive double-dot, but a namespace with a **trailing dot** (e.g. `"foo."`) passes the check. When that namespace is used to build an event name, the template `` `${normalized}.${event}` `` produces `"foo..bar"` — the exact double-dot form the validation is meant to prevent.
The same applies to a **leading dot**: `".foo"` produces event names like `".foo.bar"`.
Suggested fix:
```suggestion
const normalized = namespace.trim();
if (!normalized || normalized.includes('..') || normalized.startsWith('.') || normalized.endsWith('.')) {
throw new Error('METRIC_NAMESPACE_INVALID');
}
```
How can I resolve this? If you propose a fix, please make it concise.| export function err( | ||
| input: { | ||
| code: string; | ||
| message: string; | ||
| status?: number; | ||
| retryable?: boolean; | ||
| details?: Record<string, unknown>; | ||
| }, | ||
| options?: { timestamp?: string } | ||
| ): ErrorEnvelope { | ||
| if (!isValidErrorShape(input)) { | ||
| return { | ||
| ok: false, | ||
| error: { | ||
| code: 'INVALID_ENVELOPE_ERROR_SHAPE', | ||
| message: 'Envelope error shape is invalid; status and retryable are required.', | ||
| status: 500, | ||
| retryable: false, | ||
| details: { | ||
| received: { | ||
| code: input.code, | ||
| hasStatus: typeof input.status === 'number', | ||
| hasRetryable: typeof input.retryable === 'boolean', | ||
| }, | ||
| }, | ||
| }, | ||
| meta: { | ||
| timestamp: options?.timestamp ?? new Date().toISOString(), | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
err() silently substitutes the caller's error when status/retryable are omitted
The TypeScript signature marks status and retryable as optional:
input: { code: string; message: string; status?: number; retryable?: boolean; ... }However, if either field is absent, isValidErrorShape returns false and the function silently discards the caller's error, returning a different envelope with code: 'INVALID_ENVELOPE_ERROR_SHAPE'. A caller who writes:
err({ code: 'RATE_LIMIT_EXCEEDED', message: 'Too many requests' })gets back an envelope whose error.code is 'INVALID_ENVELOPE_ERROR_SHAPE', not 'RATE_LIMIT_EXCEEDED'. This silent substitution is confirmed by the test at envelope.test.ts:21-26 and is very easy to miss in production.
Recommended options:
- Make
statusandretryablerequired fields (compile-time safety, minimal change). - Apply safe defaults (e.g.
status ?? 500,retryable ?? false) similar to whatnormalizeLegacyEnvelopealready does on lines 118–119. - Throw explicitly so failures are never silent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/envelope/src/index.ts
Line: 55-85
Comment:
**`err()` silently substitutes the caller's error when `status`/`retryable` are omitted**
The TypeScript signature marks `status` and `retryable` as *optional*:
```ts
input: { code: string; message: string; status?: number; retryable?: boolean; ... }
```
However, if either field is absent, `isValidErrorShape` returns `false` and the function silently discards the caller's error, returning a *different* envelope with `code: 'INVALID_ENVELOPE_ERROR_SHAPE'`. A caller who writes:
```ts
err({ code: 'RATE_LIMIT_EXCEEDED', message: 'Too many requests' })
```
gets back an envelope whose `error.code` is `'INVALID_ENVELOPE_ERROR_SHAPE'`, not `'RATE_LIMIT_EXCEEDED'`. This silent substitution is confirmed by the test at `envelope.test.ts:21-26` and is very easy to miss in production.
Recommended options:
- Make `status` and `retryable` **required** fields (compile-time safety, minimal change).
- Apply safe defaults (e.g. `status ?? 500`, `retryable ?? false`) similar to what `normalizeLegacyEnvelope` already does on lines 118–119.
- Throw explicitly so failures are never silent.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| if (attempt < maxRetries) { | ||
| const delay = Math.min(maxDelayMs, initialDelayMs * attempt); |
There was a problem hiding this comment.
initialDelayMs * attempt produces linear growth (1 s → 2 s → 3 s…). Standard exponential backoff (initialDelayMs * 2 ** (attempt - 1)) grows 1 s → 2 s → 4 s…, providing much stronger back-pressure against a struggling upstream. The existence of maxDelayMs as a cap strongly implies exponential growth was the intent.
| const delay = Math.min(maxDelayMs, initialDelayMs * attempt); | |
| const delay = Math.min(maxDelayMs, initialDelayMs * 2 ** (attempt - 1)); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/webhooks/src/index.ts
Line: 141
Comment:
**Linear backoff instead of exponential backoff**
`initialDelayMs * attempt` produces **linear** growth (1 s → 2 s → 3 s…). Standard exponential backoff (`initialDelayMs * 2 ** (attempt - 1)`) grows 1 s → 2 s → 4 s…, providing much stronger back-pressure against a struggling upstream. The existence of `maxDelayMs` as a cap strongly implies exponential growth was the intent.
```suggestion
const delay = Math.min(maxDelayMs, initialDelayMs * 2 ** (attempt - 1));
```
How can I resolve this? If you propose a fix, please make it concise.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
4 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/envelope/src/index.ts">
<violation number="1" location="packages/envelope/src/index.ts:59">
P2: This type-only change removes `err()`'s runtime invalid-shape handling. JS or other untyped callers can now emit canonical error envelopes with `status`/`retryable` left undefined instead of getting `INVALID_ENVELOPE_ERROR_SHAPE`.</violation>
</file>
<file name="packages/queue/src/index.ts">
<violation number="1" location="packages/queue/src/index.ts:112">
P2: FlowFnQueueAdapter still implements `QueueAdapter`, but these methods now always throw. That breaks the shared adapter contract and turns generic `QueueAdapter` calls into runtime failures.</violation>
</file>
<file name="packages/webhooks/src/index.ts">
<violation number="1" location="packages/webhooks/src/index.ts:94">
P2: Handle `JSON.stringify` returning `undefined`; those payloads bypass this new failure path and can still be sent as a non-JSON body.</violation>
<violation number="2" location="packages/webhooks/src/index.ts:178">
P2: This canonicalization rejects valid byte-equivalent base64 signatures, so verification can now fail for callers that omit padding.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| async dequeueMatching(queueName: string, matcher: (payload: TPayload) => boolean): Promise<TPayload | null> { | ||
| void matcher; | ||
| encodeQueueName(queueName); | ||
| throw new Error('FLOWFN_QUEUE_MATCHING_UNSUPPORTED'); |
There was a problem hiding this comment.
P2: FlowFnQueueAdapter still implements QueueAdapter, but these methods now always throw. That breaks the shared adapter contract and turns generic QueueAdapter calls into runtime failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/queue/src/index.ts, line 112:
<comment>FlowFnQueueAdapter still implements `QueueAdapter`, but these methods now always throw. That breaks the shared adapter contract and turns generic `QueueAdapter` calls into runtime failures.</comment>
<file context>
@@ -97,24 +107,18 @@ export class FlowFnQueueAdapter<TPayload> implements QueueAdapter<TPayload> {
- return null;
+ void matcher;
+ encodeQueueName(queueName);
+ throw new Error('FLOWFN_QUEUE_MATCHING_UNSUPPORTED');
}
</file context>
| export function readStringEnv( | ||
| key: string, | ||
| options: { defaultValue: string; allowEmpty?: boolean; env?: NodeJS.ProcessEnv } | ||
| ): string { | ||
| const env = options.env ?? process.env; | ||
| const raw = env[key]; | ||
| if (raw === undefined) { | ||
| return options.defaultValue; | ||
| } | ||
|
|
||
| if (!options.allowEmpty && raw.trim().length === 0) { | ||
| throw new ConfigError('ENV_VALUE_INVALID', `${key} must not be empty`); | ||
| } | ||
|
|
||
| return raw; | ||
| } |
There was a problem hiding this comment.
Inconsistent behavior: readStringEnv doesn't treat empty string the same as undefined, while readIntEnv and readBooleanEnv both return the default value when raw === ''. This inconsistency will cause unexpected errors in production.
For example:
readIntEnv('KEY', { env: { KEY: '' }, defaultValue: 100 })returns100readBooleanEnv('KEY', { env: { KEY: '' }, defaultValue: true })returnstruereadStringEnv('KEY', { env: { KEY: '' }, defaultValue: 'x' })throwsConfigError
Fix: Change line 66 to match the other functions:
if (raw === undefined || raw === '') {
return options.defaultValue;
}| export function readStringEnv( | |
| key: string, | |
| options: { defaultValue: string; allowEmpty?: boolean; env?: NodeJS.ProcessEnv } | |
| ): string { | |
| const env = options.env ?? process.env; | |
| const raw = env[key]; | |
| if (raw === undefined) { | |
| return options.defaultValue; | |
| } | |
| if (!options.allowEmpty && raw.trim().length === 0) { | |
| throw new ConfigError('ENV_VALUE_INVALID', `${key} must not be empty`); | |
| } | |
| return raw; | |
| } | |
| export function readStringEnv( | |
| key: string, | |
| options: { defaultValue: string; allowEmpty?: boolean; env?: NodeJS.ProcessEnv } | |
| ): string { | |
| const env = options.env ?? process.env; | |
| const raw = env[key]; | |
| if (raw === undefined || raw === '') { | |
| return options.defaultValue; | |
| } | |
| if (!options.allowEmpty && raw.trim().length === 0) { | |
| throw new ConfigError('ENV_VALUE_INVALID', `${key} must not be empty`); | |
| } | |
| return raw; | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/config/src/__tests__/config.test.ts (1)
5-33: Add max-bound coverage for integer env validation.Current tests only exercise
min; please addmax(and optionally min+max together) so upper-bound regressions are caught.💡 Suggested test additions
describe('config', () => { @@ it('can fallback deterministically using readIntEnvOrDefault', () => { @@ }); + + it('throws ENV_VALUE_OUT_OF_RANGE for value above max', () => { + expect(() => + readIntEnv('RECFN_RATE_LIMIT_WINDOW_MS', { + env: { RECFN_RATE_LIMIT_WINDOW_MS: '6000' }, + defaultValue: 1000, + min: 100, + max: 5000, + }) + ).toThrowError(ConfigError); + }); + + it('falls back to default when value is above max', () => { + expect( + readIntEnvOrDefault('RECFN_RATE_LIMIT_WINDOW_MS', { + env: { RECFN_RATE_LIMIT_WINDOW_MS: '6000' }, + defaultValue: 1000, + min: 100, + max: 5000, + }) + ).toBe(1000); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/__tests__/config.test.ts` around lines 5 - 33, Tests only assert min-bound behavior for integer env validation; add cases covering the max bound so upper-limit regressions are caught. Update the test file to include at least one case calling readIntEnv('RECFN_RATE_LIMIT_WINDOW_MS', { env: {...}, defaultValue: X, max: Y }) that returns a value within the max, another case that passes a value greater than max and expects a ConfigError (like the existing min out-of-range test), and mirror those for readIntEnvOrDefault to ensure fallback respects max; reference the existing test patterns around readIntEnv, readIntEnvOrDefault and ConfigError to implement these additions.packages/errors/src/__tests__/errors.test.ts (1)
20-27: Avoid double-invokingresolvein the unknown-code test.You can assert type and code from one thrown instance to keep this test tighter.
♻️ Simplified assertion pattern
it('throws ERROR_CODE_UNREGISTERED for unknown codes', () => { const registry = createErrorRegistry(); - - expect(() => registry.resolve('UNKNOWN_CODE')).toThrowError(SuperfunctionError); - try { - registry.resolve('UNKNOWN_CODE'); - } catch (error) { - const typed = error as SuperfunctionError; - expect(typed.code).toBe('ERROR_CODE_UNREGISTERED'); - } + let caught: unknown; + try { + registry.resolve('UNKNOWN_CODE'); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(SuperfunctionError); + expect((caught as SuperfunctionError).code).toBe('ERROR_CODE_UNREGISTERED'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/errors/src/__tests__/errors.test.ts` around lines 20 - 27, The test currently calls registry.resolve('UNKNOWN_CODE') twice; change it to invoke resolve only once and assert on that single thrown instance: remove the expect(() => registry.resolve('UNKNOWN_CODE')).toThrowError call and keep a single try/catch around registry.resolve('UNKNOWN_CODE'), cast the caught error to SuperfunctionError and assert its .code equals 'ERROR_CODE_UNREGISTERED' (and optionally fail the test if no error is thrown) so you both verify the throw and inspect the thrown object's properties in one invocation.packages/envelope/src/__tests__/envelope.test.ts (1)
28-34: Consider explicit assertions for better test clarity.The
&&pattern in the assertions works (returnsdataifokis truthy) but is subtle. Separate assertions would be clearer and provide better failure messages.✨ Clearer assertion pattern
it('normalizes legacy {ok,result} and {ok,data} envelopes', () => { const resultEnvelope = normalizeLegacyEnvelope({ ok: true, result: { a: 1 } }, { timestamp: 't1' }); const dataEnvelope = normalizeLegacyEnvelope({ ok: true, data: { a: 2 } }, { timestamp: 't2' }); - expect(resultEnvelope.ok && resultEnvelope.data).toEqual({ a: 1 }); - expect(dataEnvelope.ok && dataEnvelope.data).toEqual({ a: 2 }); + expect(resultEnvelope.ok).toBe(true); + expect(resultEnvelope.ok && resultEnvelope.data).toEqual({ a: 1 }); + expect(dataEnvelope.ok).toBe(true); + expect(dataEnvelope.ok && dataEnvelope.data).toEqual({ a: 2 }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/envelope/src/__tests__/envelope.test.ts` around lines 28 - 34, The test uses a subtle `ok && data` expression; update the assertions in the test for normalizeLegacyEnvelope so they explicitly assert the boolean and the payload separately: for resultEnvelope and dataEnvelope first assert resultEnvelope.ok/dataEnvelope.ok (e.g., toBe(true) or truthy) and then assert resultEnvelope.data and dataEnvelope.data equal the expected objects; locate the assertions referencing normalizeLegacyEnvelope, resultEnvelope and dataEnvelope in the envelope.test.ts and replace the combined `ok && data` checks with two explicit expect statements each.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/config/src/index.ts`:
- Around line 49-57: readIntEnvOrDefault currently swallows all errors and
returns options.defaultValue without validating it; update the catch to rethrow
non-ConfigError exceptions and to validate options.defaultValue against the same
constraints as readIntEnv (min/max and integer) before returning it. Concretely:
in readIntEnvOrDefault catch the error as e, if !(e instanceof ConfigError)
throw e; then validate options.defaultValue (check it's an integer and within
options.min/options.max) and return it if valid, otherwise throw a new
ConfigError describing the invalid default; keep references to readIntEnv,
readIntEnvOrDefault, ConfigError and options.defaultValue.
In `@packages/envelope/src/index.ts`:
- Around line 11-17: Update the CanonicalEnvelopeError type to widen details
from Record<string, unknown> to unknown so it accepts producers that set
details?: unknown; also change the conversion/normalization helpers that
currently check envelope.error.details using truthiness (the checks around
envelope.error.details in the conversion helpers) to use explicit undefined
checks (details !== undefined) so falsy-but-intentional values like false, 0, or
"" are preserved. Locate the CanonicalEnvelopeError type and the functions that
read envelope.error.details in this file and make these two targeted changes.
In `@packages/metrics/package.json`:
- Around line 1-37: The release-packages.json is missing the new package names
so CI won't pick them up; add the new package identifiers
"@superfunctions/metrics", "@superfunctions/envelope", "@superfunctions/queue",
"@superfunctions/config", "@superfunctions/errors", and
"@superfunctions/webhooks" to the release-packages.json package list (the
array/object that enumerates publishable/workspace packages) so they are
included in CI/CD builds, release tagging, and workspace dependency resolution.
In `@packages/queue/src/index.ts`:
- Around line 9-49: MemoryQueueAdapter uses raw queueName strings so names like
" jobs " or "../jobs" differ from FlowFnQueueAdapter; update MemoryQueueAdapter
(methods enqueue, dequeue, dequeueMatching, peek, size) to call the same
normalization/validation function encodeQueueName on the incoming queueName
before using it as a Map key, import or reference encodeQueueName as used by
FlowFnQueueAdapter, and ensure any validation errors from encodeQueueName are
propagated so behavior matches the FlowFn adapter.
- Around line 51-55: FlowFnQueueAdapterConfig and the fetch calls in enqueue and
dequeue need abort/timeout support: add an optional timeoutMs?: number to
FlowFnQueueAdapterConfig, create an AbortController inside enqueue and dequeue,
start a timer that calls controller.abort() after config.timeoutMs (or a
sensible default) and pass controller.signal to fetch, and clear the timer when
fetch resolves; ensure any per-request override can be accepted by adding an
optional { timeoutMs } param to enqueue/dequeue signatures if needed and
propagate it to the AbortController so requests can be cancelled on timeout.
In `@packages/webhooks/src/index.ts`:
- Around line 76-80: Add a per-attempt timeout signal to WebhookDeliveryOptions
and only retry on transport errors or transient HTTP statuses: extend
WebhookDeliveryOptions with a perAttemptTimeoutMs?: number (and keep existing
fetch, maxRetries, initialDelayMs, maxDelayMs), then in the fetch retry loop
create a fresh AbortController for each attempt, derive a timeout setTimeout
that calls controller.abort() after perAttemptTimeoutMs, pass controller.signal
to the fetch call, and clear the timeout after response; when deciding to retry,
treat thrown errors during fetch as retryable, and treat only HTTP 429 and 5xx
responses as retryable—do not retry on 4xx permanent statuses like 400, 403,
404—respect maxRetries and backoff (initialDelayMs/maxDelayMs) as before.
---
Nitpick comments:
In `@packages/config/src/__tests__/config.test.ts`:
- Around line 5-33: Tests only assert min-bound behavior for integer env
validation; add cases covering the max bound so upper-limit regressions are
caught. Update the test file to include at least one case calling
readIntEnv('RECFN_RATE_LIMIT_WINDOW_MS', { env: {...}, defaultValue: X, max: Y
}) that returns a value within the max, another case that passes a value greater
than max and expects a ConfigError (like the existing min out-of-range test),
and mirror those for readIntEnvOrDefault to ensure fallback respects max;
reference the existing test patterns around readIntEnv, readIntEnvOrDefault and
ConfigError to implement these additions.
In `@packages/envelope/src/__tests__/envelope.test.ts`:
- Around line 28-34: The test uses a subtle `ok && data` expression; update the
assertions in the test for normalizeLegacyEnvelope so they explicitly assert the
boolean and the payload separately: for resultEnvelope and dataEnvelope first
assert resultEnvelope.ok/dataEnvelope.ok (e.g., toBe(true) or truthy) and then
assert resultEnvelope.data and dataEnvelope.data equal the expected objects;
locate the assertions referencing normalizeLegacyEnvelope, resultEnvelope and
dataEnvelope in the envelope.test.ts and replace the combined `ok && data`
checks with two explicit expect statements each.
In `@packages/errors/src/__tests__/errors.test.ts`:
- Around line 20-27: The test currently calls registry.resolve('UNKNOWN_CODE')
twice; change it to invoke resolve only once and assert on that single thrown
instance: remove the expect(() => registry.resolve('UNKNOWN_CODE')).toThrowError
call and keep a single try/catch around registry.resolve('UNKNOWN_CODE'), cast
the caught error to SuperfunctionError and assert its .code equals
'ERROR_CODE_UNREGISTERED' (and optionally fail the test if no error is thrown)
so you both verify the throw and inspect the thrown object's properties in one
invocation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86fbd538-7367-4ef3-8fd2-c492e0355f4c
📒 Files selected for processing (24)
packages/config/package.jsonpackages/config/src/__tests__/config.test.tspackages/config/src/index.tspackages/config/tsconfig.jsonpackages/envelope/package.jsonpackages/envelope/src/__tests__/envelope.test.tspackages/envelope/src/index.tspackages/envelope/tsconfig.jsonpackages/errors/package.jsonpackages/errors/src/__tests__/errors.test.tspackages/errors/src/index.tspackages/errors/tsconfig.jsonpackages/metrics/package.jsonpackages/metrics/src/__tests__/metrics.test.tspackages/metrics/src/index.tspackages/metrics/tsconfig.jsonpackages/queue/package.jsonpackages/queue/src/__tests__/queue.test.tspackages/queue/src/index.tspackages/queue/tsconfig.jsonpackages/webhooks/package.jsonpackages/webhooks/src/__tests__/webhooks.test.tspackages/webhooks/src/index.tspackages/webhooks/tsconfig.json
| export type CanonicalEnvelopeError = { | ||
| code: string; | ||
| message: string; | ||
| status: number; | ||
| retryable: boolean; | ||
| details?: Record<string, unknown>; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the envelope index.ts file at the specified lines
cat -n packages/envelope/src/index.ts | head -160Repository: 21nCo/super-functions
Length of output: 4976
🏁 Script executed:
# Check if the producer files exist and examine their details types
git ls-files | grep -E "(datafn|searchfn).*errors\.ts"Repository: 21nCo/super-functions
Length of output: 446
🏁 Script executed:
# Search for CanonicalEnvelopeError and related types to understand the full context
rg -n "CanonicalEnvelopeError|LegacyEnvelopeError" packages/envelope/src/Repository: 21nCo/super-functions
Length of output: 204
🏁 Script executed:
# Search for the conversion helpers mentioned in the comment
rg -n "toLegacyDataEnvelope|toLegacyResultEnvelope|normalizeLegacyEnvelope" packages/envelope/src/Repository: 21nCo/super-functions
Length of output: 1156
🏁 Script executed:
# Check datafn/server/src/http/errors.ts
cat -n datafn/server/src/http/errors.ts | head -100Repository: 21nCo/super-functions
Length of output: 1239
🏁 Script executed:
# Check searchfn/server/src/http/errors.ts
cat -n searchfn/server/src/http/errors.ts | head -100Repository: 21nCo/super-functions
Length of output: 3364
🏁 Script executed:
# Look for details type usage in these files
rg -n "details" datafn/server/src/http/errors.ts searchfn/server/src/http/errors.ts -A 2 -B 2Repository: 21nCo/super-functions
Length of output: 1852
Widen details from Record<string, unknown> to unknown for compatibility with existing producers.
The types in this file require details?: Record<string, unknown>, but datafn/server/src/http/errors.ts and searchfn/server/src/http/errors.ts both produce envelopes with details?: unknown. This type mismatch forces consuming code to cast when normalizing these envelopes. Additionally, the conversion helpers at lines 136 and 153 use truthiness checks (envelope.error.details ?), which will discard falsy-but-defined values like false, 0, or empty strings. Switch these to details !== undefined to preserve all intentional values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/envelope/src/index.ts` around lines 11 - 17, Update the
CanonicalEnvelopeError type to widen details from Record<string, unknown> to
unknown so it accepts producers that set details?: unknown; also change the
conversion/normalization helpers that currently check envelope.error.details
using truthiness (the checks around envelope.error.details in the conversion
helpers) to use explicit undefined checks (details !== undefined) so
falsy-but-intentional values like false, 0, or "" are preserved. Locate the
CanonicalEnvelopeError type and the functions that read envelope.error.details
in this file and make these two targeted changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/queue/src/index.ts (1)
64-68:⚠️ Potential issue | 🔴 CriticalAdd per-request timeout/abort for FlowFn HTTP calls.
enqueueanddequeue(Line 83 and Line 99) still awaitfetchwithout cancellation. A stalled endpoint can block function execution indefinitely.⏱️ Proposed patch (timeout + AbortController)
export interface FlowFnQueueAdapterConfig { baseUrl: string; apiKey: string; fetch?: typeof globalThis.fetch; + timeoutMs?: number; } function encodeQueueName(queueName: string): string { return encodeURIComponent(normalizeQueueName(queueName)); } export class FlowFnQueueAdapter<TPayload> implements QueueAdapter<TPayload> { private readonly fetcher: typeof globalThis.fetch; + private static readonly DEFAULT_TIMEOUT_MS = 10_000; constructor(private readonly config: FlowFnQueueAdapterConfig) { this.fetcher = config.fetch ?? globalThis.fetch; } + + private async fetchWithTimeout(input: string, init: RequestInit): Promise<Response> { + const timeoutMs = this.config.timeoutMs ?? FlowFnQueueAdapter.DEFAULT_TIMEOUT_MS; + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + try { + return await this.fetcher(input, { ...init, signal: controller.signal }); + } finally { + clearTimeout(timer); + } + } async enqueue(queueName: string, payload: TPayload): Promise<void> { const encodedQueueName = encodeQueueName(queueName); - const response = await this.fetcher(`${this.config.baseUrl}/queues/${encodedQueueName}/enqueue`, { + const response = await this.fetchWithTimeout(`${this.config.baseUrl}/queues/${encodedQueueName}/enqueue`, { method: 'POST', headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${this.config.apiKey}`, }, body: JSON.stringify(payload), }); @@ async dequeue(queueName: string): Promise<TPayload | null> { const encodedQueueName = encodeQueueName(queueName); - const response = await this.fetcher(`${this.config.baseUrl}/queues/${encodedQueueName}/dequeue`, { + const response = await this.fetchWithTimeout(`${this.config.baseUrl}/queues/${encodedQueueName}/dequeue`, { method: 'POST', headers: { Authorization: `Bearer ${this.config.apiKey}`, }, });#!/bin/bash set -euo pipefail echo "Inspect FlowFn queue config and fetch call sites:" cat -n packages/queue/src/index.ts | sed -n '64,115p' echo echo "Verify timeout/abort symbols are absent/present:" rg -n -C2 'timeoutMs|AbortController|signal\s*:' packages/queue/src/index.tsAlso applies to: 81-90, 97-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/queue/src/index.ts` around lines 64 - 68, The enqueue and dequeue implementations are calling the injected fetch without any per-request timeout/abort, so add an optional timeoutMs (number) to FlowFnQueueAdapterConfig and use AbortController when calling fetch in enqueue and dequeue (pass controller.signal to fetch), starting a setTimeout to call controller.abort() after timeoutMs (or a sensible default) and clear the timer on completion; ensure errors from an aborted fetch are handled/propagated consistently in enqueue and dequeue.
🧹 Nitpick comments (1)
packages/queue/src/index.ts (1)
34-35: Delete empty queues after removal to avoid stale Map growth.After
shift/splice, empty arrays remain keyed inqueues. Cleaning them up keeps memory bounded when many transient queue names are used.🧹 Proposed cleanup
async dequeue(queueName: string): Promise<TPayload | null> { @@ - return queue.shift() ?? null; + const payload = queue.shift() ?? null; + if (queue.length === 0) { + this.queues.delete(normalizedQueueName); + } + return payload; } @@ async dequeueMatching(queueName: string, matcher: (payload: TPayload) => boolean): Promise<TPayload | null> { @@ - const [payload] = queue.splice(index, 1); - return payload ?? null; + const [payload] = queue.splice(index, 1); + if (queue.length === 0) { + this.queues.delete(normalizedQueueName); + } + return payload ?? null; }Also applies to: 49-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/queue/src/index.ts` around lines 34 - 35, When removing items from a per-name queue, clean up the backing Map entry if the array becomes empty to avoid unbounded growth: after the queue.shift() call in the function that returns queue.shift() ?? null (and similarly after any queue.splice(...) removal around the other case at lines 49-50), check if queue.length === 0 and call queues.delete(name) to remove the empty array from the queues Map; update the code paths that use queue.shift() and queue.splice(...) to perform this post-removal cleanup using the existing queues Map and the same name/key used to look up the array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/config/src/index.ts`:
- Around line 63-77: readStringEnv currently returns options.defaultValue when
the env key is missing or when raw is '' without validating the default; this
allows an empty or whitespace-only default to bypass allowEmpty: false. Update
readStringEnv (the function named readStringEnv) to validate
options.defaultValue when returning it: if options.allowEmpty is false ensure
the defaultValue is neither empty string nor only whitespace and throw
ConfigError('ENV_VALUE_INVALID', `${key} must not be empty`) if it is; keep the
existing validation for raw values (the existing raw.trim() check) and apply the
same rule to the default path so empty defaults cannot slip through.
---
Duplicate comments:
In `@packages/queue/src/index.ts`:
- Around line 64-68: The enqueue and dequeue implementations are calling the
injected fetch without any per-request timeout/abort, so add an optional
timeoutMs (number) to FlowFnQueueAdapterConfig and use AbortController when
calling fetch in enqueue and dequeue (pass controller.signal to fetch), starting
a setTimeout to call controller.abort() after timeoutMs (or a sensible default)
and clear the timer on completion; ensure errors from an aborted fetch are
handled/propagated consistently in enqueue and dequeue.
---
Nitpick comments:
In `@packages/queue/src/index.ts`:
- Around line 34-35: When removing items from a per-name queue, clean up the
backing Map entry if the array becomes empty to avoid unbounded growth: after
the queue.shift() call in the function that returns queue.shift() ?? null (and
similarly after any queue.splice(...) removal around the other case at lines
49-50), check if queue.length === 0 and call queues.delete(name) to remove the
empty array from the queues Map; update the code paths that use queue.shift()
and queue.splice(...) to perform this post-removal cleanup using the existing
queues Map and the same name/key used to look up the array.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e7df2c6-95a9-4f5a-8894-bf989a400f52
📒 Files selected for processing (4)
packages/config/src/__tests__/config.test.tspackages/config/src/index.tspackages/queue/src/__tests__/queue.test.tspackages/queue/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- packages/config/src/tests/config.test.ts
- packages/queue/src/tests/queue.test.ts
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/config/src/index.ts">
<violation number="1" location="packages/config/src/index.ts:69">
P2: `readStringEnv` returns `options.defaultValue` without checking it against the `allowEmpty` constraint. If a caller passes `{ defaultValue: '', allowEmpty: false }` and the env var is missing or empty, the function silently returns `''`—contradicting the intent of `allowEmpty: false`. This is inconsistent with the analogous `readIntEnvOrDefault` fix in this PR, which now validates the default through `validateIntValue`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/queue/src/index.ts (2)
137-152: Timeout abort throws generic error.When
fetchWithTimeoutaborts due to timeout, the thrown error is a genericAbortError(orDOMExceptionwith nameAbortError). Callers cannot distinguish between a timeout and other abort reasons without inspecting error properties.Consider wrapping the abort error with a more descriptive message for easier debugging:
♻️ Optional: Wrap timeout errors with context
private async fetchWithTimeout(url: string, init: RequestInit): Promise<Response> { const controller = typeof AbortController === 'function' ? new AbortController() : undefined; const timeoutId = controller && this.timeoutMs > 0 ? setTimeout(() => controller.abort(), this.timeoutMs) : undefined; try { return await this.fetcher(url, { ...init, signal: controller?.signal, }); + } catch (error) { + if (error instanceof Error && error.name === 'AbortError' && timeoutId !== undefined) { + throw new Error(`FlowFn request timed out after ${this.timeoutMs}ms`); + } + throw error; } finally { if (timeoutId) { clearTimeout(timeoutId); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/queue/src/index.ts` around lines 137 - 152, fetchWithTimeout currently lets an AbortError bubble up on timeout which is indistinguishable from other aborts; modify fetchWithTimeout to catch errors from this.fetcher, detect an abort-related error (e.g., error.name === 'AbortError' or instanceof DOMException) and rethrow a new, descriptive timeout error (e.g., "Fetch to <url> timed out after <this.timeoutMs>ms" or a custom TimeoutError) while preserving the original error as the cause; for non-abort errors rethrow them unchanged. Reference fetchWithTimeout, controller, timeoutId, this.timeoutMs and this.fetcher when making the change.
117-118: Consider adding runtime validation of response shape.The type assertion
as { payload?: TPayload }assumes the server response conforms to the expected structure. If the server returns an unexpected shape (e.g.,{ data: { payload: ... } }or an error object), this could cause subtle bugs downstream.For a more defensive approach, consider validating the response structure before accessing
data.payload, or documenting the expected server contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/queue/src/index.ts` around lines 117 - 118, The current code casts the JSON result from response.json() to { payload?: TPayload } and directly returns data.payload, which can hide malformed responses; change this to validate the parsed JSON at runtime before accessing data.payload: after calling response.json() check that the result is an object (not null) and that it either has a 'payload' property or matches the expected payload shape (e.g., typeof payload checks or a small shape-guard for TPayload), then return the payload or null/throw a descriptive error; update the logic around response.json(), the local variable data, and the return path so callers of data.payload get either a validated TPayload or a clear failure.packages/webhooks/src/index.ts (1)
126-128: Consider exposing signature options for delivery.When
input.secretis provided,signWebhookPayloadis called with default options (sha256, hex, no prefix). Users cannot customize the algorithm, encoding, or prefix during delivery without manually signing and passing the header.This is fine for an MVP, but consider adding an optional
signatureOptions?: WebhookSignatureOptionsfield toWebhookDeliveryInputif consumers need to integrate with services requiring different signature formats (e.g.,sha1=prefix for legacy GitHub webhooks).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webhooks/src/index.ts` around lines 126 - 128, The delivery currently always calls signWebhookPayload(payloadString, input.secret) when input.secret exists, preventing callers from customizing algorithm/encoding/prefix; add an optional signatureOptions?: WebhookSignatureOptions property to the WebhookDeliveryInput type and pass it through to signWebhookPayload (use input.signatureOptions) and still honor input.signatureHeader for the header name; update the call site in packages/webhooks/src/index.ts (where signWebhookPayload is invoked) to supply the new options parameter and ensure WebhookSignatureOptions describes algorithm, encoding, and prefix so consumers can request e.g. a "sha1=" prefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/queue/src/index.ts`:
- Around line 137-152: fetchWithTimeout currently lets an AbortError bubble up
on timeout which is indistinguishable from other aborts; modify fetchWithTimeout
to catch errors from this.fetcher, detect an abort-related error (e.g.,
error.name === 'AbortError' or instanceof DOMException) and rethrow a new,
descriptive timeout error (e.g., "Fetch to <url> timed out after
<this.timeoutMs>ms" or a custom TimeoutError) while preserving the original
error as the cause; for non-abort errors rethrow them unchanged. Reference
fetchWithTimeout, controller, timeoutId, this.timeoutMs and this.fetcher when
making the change.
- Around line 117-118: The current code casts the JSON result from
response.json() to { payload?: TPayload } and directly returns data.payload,
which can hide malformed responses; change this to validate the parsed JSON at
runtime before accessing data.payload: after calling response.json() check that
the result is an object (not null) and that it either has a 'payload' property
or matches the expected payload shape (e.g., typeof payload checks or a small
shape-guard for TPayload), then return the payload or null/throw a descriptive
error; update the logic around response.json(), the local variable data, and the
return path so callers of data.payload get either a validated TPayload or a
clear failure.
In `@packages/webhooks/src/index.ts`:
- Around line 126-128: The delivery currently always calls
signWebhookPayload(payloadString, input.secret) when input.secret exists,
preventing callers from customizing algorithm/encoding/prefix; add an optional
signatureOptions?: WebhookSignatureOptions property to the WebhookDeliveryInput
type and pass it through to signWebhookPayload (use input.signatureOptions) and
still honor input.signatureHeader for the header name; update the call site in
packages/webhooks/src/index.ts (where signWebhookPayload is invoked) to supply
the new options parameter and ensure WebhookSignatureOptions describes
algorithm, encoding, and prefix so consumers can request e.g. a "sha1=" prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62a3d931-d449-43bb-b429-8b2b6676eac7
📒 Files selected for processing (6)
packages/config/src/__tests__/config.test.tspackages/config/src/index.tspackages/queue/src/__tests__/queue.test.tspackages/queue/src/index.tspackages/webhooks/src/__tests__/webhooks.test.tspackages/webhooks/src/index.ts
✅ Files skipped from review due to trivial changes (3)
- packages/queue/src/tests/queue.test.ts
- packages/config/src/tests/config.test.ts
- packages/config/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/webhooks/src/tests/webhooks.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 378b67868b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "url": "git+https://github.com/21nCo/super-functions.git", | ||
| "directory": "packages/config" | ||
| }, | ||
| "publishConfig": { "access": "public" }, |
There was a problem hiding this comment.
This package is declared as publicly publishable (publishConfig.access = "public"), but no matching entry was added to release-packages.json for it (or the other newly added runtime packages). The publish workflow (.github/workflows/publish-tag.yml) resolves tags through scripts/resolve-release-tag.mjs, which only accepts slugs present in that file, so release tags for these new packages will fail with No publishable workspace found and cannot be published to npm until release targets are registered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 821727de37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| async enqueue(queueName: string, payload: TPayload): Promise<void> { | ||
| const encodedQueueName = encodeQueueName(queueName); | ||
| const response = await this.fetchWithTimeout(`${this.config.baseUrl}/queues/${encodedQueueName}/enqueue`, { |
There was a problem hiding this comment.
The adapter builds request URLs by directly concatenating baseUrl with "/queues/...", so a common configuration like baseUrl: "https://flowfn.example.com/" produces https://flowfn.example.com//queues/.... On routers that do not normalize double slashes, enqueue/dequeue calls can return 404 and make the queue adapter appear broken. Normalizing trailing slashes once in the constructor (or using URL) avoids this config-sensitive failure mode.
Useful? React with 👍 / 👎.
| const headers: Record<string, string> = { | ||
| 'Content-Type': 'application/json', | ||
| ...(input.headers ?? {}), | ||
| }; |
There was a problem hiding this comment.
headers is assembled with object spread, which is case-sensitive for keys. If callers pass headers like content-type or x-webhook-signature, this object can contain both that key and the package’s Content-Type/X-Webhook-Signature; Fetch then normalizes and combines them into comma-separated values, yielding invalid Content-Type or a signature header that fails verification. This causes delivery failures for otherwise valid inputs when caller header casing differs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e27c87e99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const existingKey = findHeaderKey(headers, headerName); | ||
| if (existingKey && existingKey !== headerName) { | ||
| delete headers[existingKey]; | ||
| } |
There was a problem hiding this comment.
setCanonicalHeader only deletes the first case-insensitive match, so when headers already contains both the canonical key and another case variant (for example after merging multiple header maps), the extra variant remains and fetch/Headers coalesces them into comma-separated values. This can still produce invalid Content-Type or signature headers during webhook delivery even after canonicalization; remove all case-insensitive duplicates except the canonical key before assigning the final value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/webhooks/src/index.ts">
<violation number="1" location="packages/webhooks/src/index.ts:90">
P2: `setCanonicalHeader()` only removes one matching casing variant, so duplicate header names can still be sent and get merged into an invalid value by `fetch()`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/webhooks/src/index.ts (2)
97-105:⚠️ Potential issue | 🟠 MajorFail fast on invalid delivery inputs before retrying.
If
fetchis unavailable or the URL is malformed, the function only discovers that inside the retry loop and then classifies it as retryable. Those are permanent local failures, so the extra backoff just adds latency with no chance of recovery.Run this to verify the malformed-URL case. Expected result:
hasStatusisfalse, which means the currentshouldRetryWebhookError()path will retry it.#!/bin/bash set -e sed -n '97,105p;263,269p' packages/webhooks/src/index.ts node - <<'NODE' (async () => { if (typeof fetch !== 'function') { console.log('global fetch is unavailable in this Node runtime'); process.exit(0); } try { await fetch('not a url'); } catch (error) { console.log({ name: error?.name, hasStatus: Boolean(error && typeof error === 'object' && 'status' in error), message: error?.message, }); } })(); NODEAlso applies to: 263-268
142-148:⚠️ Potential issue | 🟠 MajorDon't let an empty secret silently disable signing.
if (input.secret)conflates “no secret provided” with “misconfigured empty secret”. In the latter case this sends an unsigned webhook instead of failing fast, which is a risky default for an auth path.💡 Safer guard
- if (input.secret) { + if (input.secret !== undefined) { + if (input.secret.length === 0) { + return { + ok: false, + attempts: [{ attempt: 1, ok: false, error: 'Webhook secret cannot be empty' }], + }; + } setCanonicalHeader( headers, input.signatureHeader ?? 'X-Webhook-Signature', signWebhookPayload(payloadString, input.secret) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webhooks/src/index.ts` around lines 142 - 148, The current guard if (input.secret) lets an empty string silently skip signing; update the logic around input.secret used with signWebhookPayload and setCanonicalHeader to explicitly distinguish "no secret provided" (undefined/null) from a misconfigured empty secret: if input.secret is undefined/null continue without signing, but if it's an empty or all-whitespace string throw an error (fail fast) so signing is not silently disabled; ensure you still use input.signatureHeader, payloadString and headers when calling signWebhookPayload/setCanonicalHeader.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/webhooks/src/index.ts`:
- Around line 136-140: The current logic preserves any caller-supplied
Content-Type even though the body is always JSON.stringify(input.payload);
change the code so the request always sends a JSON content type: remove the
branch that preserves a non-JSON type and instead always call
setCanonicalHeader(headers, 'Content-Type', 'application/json') (or set
headers['Content-Type']='application/json' when
findHeaderKey(headers,'Content-Type') is absent) so that the Content-Type header
for the JSON body is unambiguously 'application/json'; update usages of
findHeaderKey and setCanonicalHeader accordingly.
---
Duplicate comments:
In `@packages/webhooks/src/index.ts`:
- Around line 142-148: The current guard if (input.secret) lets an empty string
silently skip signing; update the logic around input.secret used with
signWebhookPayload and setCanonicalHeader to explicitly distinguish "no secret
provided" (undefined/null) from a misconfigured empty secret: if input.secret is
undefined/null continue without signing, but if it's an empty or all-whitespace
string throw an error (fail fast) so signing is not silently disabled; ensure
you still use input.signatureHeader, payloadString and headers when calling
signWebhookPayload/setCanonicalHeader.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31ab3cba-4c4f-42db-93c0-aeb3e8567bfb
📒 Files selected for processing (4)
packages/queue/src/__tests__/queue.test.tspackages/queue/src/index.tspackages/webhooks/src/__tests__/webhooks.test.tspackages/webhooks/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/webhooks/src/tests/webhooks.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1423f5225
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const normalizedInput = value.replace(/=+$/, ''); | ||
| const normalizedDecoded = decoded.toString('base64').replace(/=+$/, ''); |
There was a problem hiding this comment.
The base64 decoding path still accepts non-canonical over-padding because normalizedInput removes all trailing = before equality checking, so a valid digest can be altered and still verify. Fresh evidence on this commit: verifyWebhookSignature(payload, signature + '=', secret, { encoding: 'base64' }) returns true for a valid SHA-256 signature that was tampered by adding one extra =. This means modified signature headers can pass verification instead of being rejected as malformed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e49c4d815c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return null; | ||
| } | ||
|
|
||
| return queue.shift() ?? null; |
There was a problem hiding this comment.
dequeue uses queue.shift() ?? null, which collapses a legitimate queued payload of null (and undefined when TPayload allows it) into the same return value used for an empty queue. In that case the job is removed but callers interpret it as “no message,” so work can be silently skipped; the same nullish-coalescing pattern is also present in dequeueMatching and FlowFnQueueAdapter.dequeue.
Useful? React with 👍 / 👎.
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe2f30355d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| options: WebhookDeliveryOptions = {} | ||
| ): Promise<WebhookDeliveryResult> { | ||
| const fetchFn = options.fetch ?? globalThis.fetch; | ||
| const maxRetries = Math.max(1, options.maxRetries ?? 3); |
There was a problem hiding this comment.
deliverWebhook computes maxRetries with Math.max(1, options.maxRetries ?? 3), but Math.max returns NaN when callers pass NaN (for example from Number(process.env.WEBHOOK_MAX_RETRIES)). With maxRetries = NaN, the for (attempt <= maxRetries) loop never runs, so the function exits with attempts: [] and sends no webhook at all; this is a silent delivery failure that should be prevented by validating maxRetries as finite before clamping.
Useful? React with 👍 / 👎.
| const existingContentType = headers[findHeaderKey(headers, 'Content-Type') ?? ''] ?? undefined; | ||
| const normalizedContentType = |
There was a problem hiding this comment.
Content type normalization takes the first case-insensitive header match, so when merged headers contain multiple Content-Type case variants, an earlier stale value can override a later intended value. For example, content-type: text/plain followed by Content-Type: application/cloudevents+json will be normalized from the first entry and then canonicalized, dropping the later override; this can send the wrong media type to webhook consumers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/queue/src/index.ts">
<violation number="1" location="packages/queue/src/index.ts:124">
P2: This branch does not actually preserve `undefined` payloads for the FlowFn adapter, because the transport is still JSON and JSON cannot carry `undefined` values.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0045d8de4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| value: number, | ||
| options: { min?: number; max?: number } | ||
| ): number { | ||
| if (!Number.isInteger(value)) { |
There was a problem hiding this comment.
readIntEnv currently parses with Number.parseInt and then validates with Number.isInteger, which accepts rounded values beyond Number.MAX_SAFE_INTEGER. For example, an env value like "9007199254740993" is coerced to 9007199254740992 and treated as valid, silently changing configuration instead of raising ENV_VALUE_INVALID. This can produce incorrect runtime behavior when large numeric envs are used; the validator should require safe integers (or use BigInt parsing with explicit bounds).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f36d4116be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| constructor(private readonly config: FlowFnQueueAdapterConfig) { | ||
| this.fetcher = config.fetch ?? globalThis.fetch; | ||
| this.timeoutMs = Math.max(0, config.timeoutMs ?? 15_000); |
There was a problem hiding this comment.
FlowFnQueueAdapter clamps timeoutMs with Math.max, but Math.max(0, NaN) yields NaN, so misconfigured values (for example Number(process.env.FLOWFN_TIMEOUT_MS) when unset/invalid) silently disable request timeouts because this.timeoutMs > 0 becomes false in fetchWithTimeout. In that case stalled enqueue/dequeue calls can hang indefinitely instead of aborting, which is a production reliability regression for bad env input.
Useful? React with 👍 / 👎.
| const initialDelayMs = Math.max(0, options.initialDelayMs ?? 1000); | ||
| const maxDelayMs = Math.max(initialDelayMs, options.maxDelayMs ?? 30_000); | ||
| const perAttemptTimeoutMs = Math.max(0, options.perAttemptTimeoutMs ?? 15_000); |
There was a problem hiding this comment.
initialDelayMs, maxDelayMs, and perAttemptTimeoutMs are also normalized with Math.max but never checked for finiteness, so NaN propagates (Math.max(0, NaN) => NaN). This makes per-attempt abort timeouts never start (NaN > 0 is false) and can collapse retry backoff to effectively immediate retries, causing hangs or retry storms under env/config parse errors instead of failing fast with a clear validation error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e598b7db6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : 'application/json'; | ||
| setCanonicalHeader(headers, 'Content-Type', normalizedContentType); | ||
|
|
||
| if (input.secret) { |
There was a problem hiding this comment.
The signature header is only added when input.secret is truthy, so secret: "" is treated the same as no secret and the webhook is sent unsigned. This is a silent security/compatibility failure when secrets come from env/config and end up empty, because callers who intended signed delivery will instead emit unverifiable requests. Treat an empty secret as invalid (throw) or sign whenever the field is present (input.secret !== undefined).
Useful? React with 👍 / 👎.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9510eeec7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ('data' in envelope) { | ||
| return envelope; |
There was a problem hiding this comment.
ok before treating envelopes as success
normalizeLegacyEnvelope currently classifies an input as a success envelope based only on the presence of data, without checking the ok discriminator. If upstream code sends an error payload that also includes a data field (for example { ok: false, error: {...}, data: null }), this path returns it as success and silently masks the error. This can flip failure handling logic in callers; gate success normalization on ok === true (and error normalization on ok === false) instead of key presence alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23299201cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| code: envelope.error.code, | ||
| message: envelope.error.message, |
There was a problem hiding this comment.
normalizeLegacyEnvelope assumes every non-success input has envelope.error.code/message, so malformed legacy responses like { ok: false } (or { ok: false, error: null }) throw a TypeError instead of returning a normalized error envelope. Because this helper is typically used on external payloads, one bad upstream response can crash normalization paths; add an explicit runtime check for a valid error object before dereferencing its fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8688f563e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| code: envelope.error.code, | ||
| message: envelope.error.message, |
There was a problem hiding this comment.
normalizeLegacyEnvelope forwards envelope.error.code and envelope.error.message without runtime validation, and err/isValidErrorShape only checks status and retryable. If a malformed legacy payload like { ok:false, error:{ status:500, retryable:false } } is received, this path emits a “canonical” envelope with code/message as undefined, breaking the package’s own error contract for downstream consumers that expect strings.
Useful? React with 👍 / 👎.
| } | ||
| setCanonicalHeader( | ||
| headers, | ||
| input.signatureHeader ?? 'X-Webhook-Signature', |
There was a problem hiding this comment.
Using input.signatureHeader ?? 'X-Webhook-Signature' treats an empty string as a valid header name, so misconfigurations like signatureHeader: '' produce headers with an invalid key and cause fetch/Headers to throw on every attempt. This leads to guaranteed delivery failures (and unnecessary retries) instead of a clear configuration error or fallback to the default signature header.
Useful? React with 👍 / 👎.
Merge activity
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ec8281e73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (envelope.ok === false && 'error' in envelope && isValidErrorShape(envelope.error)) { | ||
| return envelope; |
There was a problem hiding this comment.
In normalizeLegacyEnvelope, the canonical shortcut returns envelope whenever isValidErrorShape(envelope.error) is true, but that helper only checks status and retryable. A payload like { ok:false, meta:{timestamp:"..."}, error:{ status:500, retryable:false } } is therefore treated as canonical and returned with missing error.code/error.message, violating the declared ErrorEnvelope contract and causing downstream consumers that assume string fields to break.
Useful? React with 👍 / 👎.
## Summary - add the missing shared runtime packages to `origin/dev` - include config, envelope, errors, metrics, queue, and webhooks foundations - keep the PR limited to the shared package layer used by multiple downstream functions ## Validation - inspected the copied package set in a clean worktree created from `origin/dev` - did not run a reliable monorepo validation pass in this worktree ## Notes - no `.conduct` changes included - opened directly against `origin/dev` with `GH_CONFIG_DIR=~/.config/gh-other` ## Summary by Sourcery Introduce shared runtime utility packages for envelopes, errors, config, metrics, queues, and webhooks to be reused across Superfunctions services. New Features: - Add a canonical response envelope package with helpers for constructing, normalizing, and converting success/error envelopes. - Add a shared typed error package providing an error registry and strongly-typed SuperfunctionError constructor. - Add a shared config package exposing typed environment variable readers for ints, strings, and booleans. - Add a shared metrics package with pluggable and namespaced metrics emitters. - Add a shared queue package with an in-memory adapter and a FlowFn-backed queue adapter interface. - Add a shared webhooks package for HMAC signing/verification and retrying webhook delivery. Build: - Configure TypeScript build settings and package metadata for the new config, envelope, errors, metrics, queue, and webhooks packages. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Adds shared runtime packages for config, envelope, errors, metrics, queue, and webhooks, and hardens envelope normalization and webhook signing/delivery edge cases. Registers these packages for release (SF-30). - **New Features** - `@superfunctions/config`: typed int/string/boolean readers; safe-integer bounds and min/max; validates defaults; empty strings off by default (`allowEmpty` opt‑in); booleans accept true/false, 1/0, yes/no; throws `ENV_VALUE_INVALID`/`ENV_VALUE_OUT_OF_RANGE`. - `@superfunctions/envelope`: canonical ok/err with meta timestamp; legacy normalize/convert; fills missing legacy `status`/`retryable` from defaults; guards malformed legacy errors and `ok:false`+`data`; `err()` requires numeric `status` and boolean `retryable` or returns `INVALID_ENVELOPE_ERROR_SHAPE`. - `@superfunctions/errors`: registry; `SuperfunctionError` and `createTypedError`; unknown codes throw `ERROR_CODE_UNREGISTERED`. - `@superfunctions/metrics`: no‑op emitter; namespaced emitter with strict validation (`METRIC_NAMESPACE_INVALID`). - `@superfunctions/queue`: `MemoryQueueAdapter` with name normalization/validation; `FlowFnQueueAdapter` with Bearer auth, per‑request timeouts (`AbortController`), invalid timeout → `FLOWFN_QUEUE_TIMEOUT_INVALID`, trailing‑slash normalization; 404→null on dequeue; preserves explicit `undefined`; `peek`/`size`/`dequeueMatching` unsupported. - `@superfunctions/webhooks`: HMAC sign/verify (hex/base64) with strict decoding (accepts missing padding; rejects malformed/non‑canonical) and optional prefix; delivery with retries/backoff, per‑attempt timeout, and validated timing opts (`WEBHOOK_MAX_RETRIES_INVALID`/`WEBHOOK_INITIAL_DELAY_INVALID`/`WEBHOOK_MAX_DELAY_INVALID`/`WEBHOOK_PER_ATTEMPT_TIMEOUT_INVALID`); rejects empty secrets (`WEBHOOK_SECRET_INVALID`) and blank signature header names; no retries on non‑transient 4xx; canonicalizes headers (case‑insensitive de‑dup, prefers latest variant, ensures single `Content-Type` and signature header, forces `application/json` for non‑JSON); validates JSON‑serializable payload; returns structured attempts with final `responseStatus`. <sup>Written for commit 6ec8281. Summary will update on new commits.</sup> <!-- End of auto-generated description by cubic. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Configuration helpers for reading/validating env values (ints, strings, booleans). * Canonical envelope format with converters for legacy shapes. * Error registry and typed error construction utilities. * Metrics emitter with optional namespacing. * Queue abstraction with in-memory and remote adapters. * Webhook signing, verification, and delivery with retries/backoff and timeouts. * **Tests** * Added comprehensive test suites covering the new packages and behaviors. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
graphite-app
bot
force-pushed
the
SF-30/runtime-shared-foundations
branch
from
6ec8281 to
8ff8f78
Compare
|
2 participants
Summary
origin/devValidation
origin/devNotes
.conductchanges includedorigin/devwithGH_CONFIG_DIR=~/.config/gh-otherSummary by Sourcery
Introduce shared runtime utility packages for envelopes, errors, config, metrics, queues, and webhooks to be reused across Superfunctions services.
New Features:
Build:
Summary by cubic
Adds shared runtime packages for config, envelope, errors, metrics, queue, and webhooks. Registers them for release and hardens envelope normalization and webhook signing/delivery edge cases (SF-30).
@superfunctions/config: int/string/boolean readers; safe-integer checks; min/max; validates defaults; empty strings off by default (allowEmptyopt‑in); booleans accept true/false, 1/0, yes/no; throwsENV_VALUE_INVALID/ENV_VALUE_OUT_OF_RANGE.@superfunctions/envelope: canonicalok/errwith timestamp; normalize legacy{data}/{result}; fill missingstatus/retryable; guard malformed legacy andok:false+data;err()requires numericstatusand booleanretryableor returnsINVALID_ENVELOPE_ERROR_SHAPE.@superfunctions/errors: registry;SuperfunctionError;createTypedError; unknown codes throwERROR_CODE_UNREGISTERED.@superfunctions/metrics: no‑op emitter; namespaced emitter with strict validation (METRIC_NAMESPACE_INVALID).@superfunctions/queue:MemoryQueueAdapterwith name normalization/validation (FLOWFN_QUEUE_NAME_INVALID);FlowFnQueueAdapterwith Bearer auth, per‑request timeouts (AbortController), invalid timeout →FLOWFN_QUEUE_TIMEOUT_INVALID, trailing‑slash normalization; 404→null on dequeue; preserves explicitundefined;peek/size/dequeueMatchingunsupported.@superfunctions/webhooks: HMAC sign/verify (hex/base64) with strict decoding (accepts missing padding; rejects malformed) and optional prefix; delivery with retries/backoff, per‑attempt timeout, and validated timing opts (WEBHOOK_MAX_RETRIES_INVALID/WEBHOOK_INITIAL_DELAY_INVALID/WEBHOOK_MAX_DELAY_INVALID/WEBHOOK_PER_ATTEMPT_TIMEOUT_INVALID); rejects empty secrets (WEBHOOK_SECRET_INVALID) and blank signature header names; no retries on non‑transient 4xx; canonicalizes headers (case‑insensitive de‑dup, prefers latest, singleContent-Typeand signature header, forceapplication/jsonfor non‑JSON); validates JSON‑serializable payload; returns structured attempts with finalresponseStatus.Written for commit 8ff8f78. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests