feat(agent-runtime): per-thread creation-time index#445
Conversation
…read
Adds a fire-and-forget sidecar object under
{prefix}index/threads-by-date/YYYY-MM-DD/{revMs}_{threadId} every time
`OSSAgentStore.createThread` succeeds, so analytics tooling that points
directly at OSS can enumerate threads by their creation day without first
GETting any meta.json. The suffix is the decimal complement of `Date.now()`
(13-digit zero-padded), which makes the standard ASC ListObjects on a date
prefix return the newest-first ordering that OSS's API can't provide
natively. The body is a compact JSON `{ threadId, createdAt, metadata }`
mirroring `threads/{id}/meta.json`'s timestamp value, so a single GET
yields the full record.
The index PUT is intentionally non-blocking: it runs on a background
promise tracked in an internal `Set`, contributes zero latency to
`createThread`'s happy path, and a failure produces one `warn` line
through an injected logger (falling back to `console.warn`) rather than
propagating. `OSSAgentStore` gains a public `awaitPendingWrites()`
method, used both by `destroy()` (to drain pending writes during
graceful shutdown) and by the test suite (to make the eventually
consistent write observable at an explicit point).
`OSSAgentStoreOptions` gains two new optional fields:
- `logger?: { warn(message, ...args): void }` — structurally compatible
with `EggLogger` and `console`, so callers can pass `app.logger`
without an explicit type dependency.
- `dateTimezone?: string` — IANA name (or the literal `'UTC'`, the
default) used for the date-bucket directory. UTC keeps cross-region
workers in sync; pass e.g. `'Asia/Shanghai'` when the analytics
calendar follows a local timezone.
`AgentStoreUtils` exports four new helpers, all available from the
package root, intended for downstream tooling that walks the index keys:
- `TS_MAX_MS` — the algebraic ceiling (`9_999_999_999_999`, ~year 2286).
- `reverseMs(ms)` — the complement-encoding function.
- `decodeReverseMs(suffix)` — its inverse, so analytics scripts can
read the millisecond timestamp straight from the file name without
opening the body.
- `dateBucket(ms, timezone?)` — the same `YYYY-MM-DD` formatter the
store itself uses, factored out so reproducing the key shape in a
backfill script is straightforward.
The `AgentStore` and `ObjectStorageClient` interfaces are unchanged.
`AgentRuntime` is unchanged. The read path (`getThread`) does not
consult the index, so it remains decoupled from the index's eventual
consistency. Threads created before this commit have no index entry;
backfilling is left to the consumer, which can iterate the existing
`{prefix}threads/*/meta.json` keyspace and write the corresponding
index objects using the exported helpers.
Tests added:
- `test/AgentStoreUtils.test.ts` — round-trip, monotonicity, range
guards for `reverseMs`/`decodeReverseMs`; UTC/case-insensitivity,
the day-boundary edge case, and an IANA timezone roundtrip
(`Asia/Shanghai` and `America/Los_Angeles`) plus a RangeError
guard for unknown zones for `dateBucket`.
- The new `describe('thread time index')` block in
`test/OSSAgentStore.test.ts` covers: key shape, JSON body shape with
a snapshot-not-reference check on `metadata`, within-day ASC list
order equalling time-DESC creation order, the UTC day boundary,
the `dateTimezone` override, the non-blocking latency property
using an artificially-delayed test client, the warn-logged
failure-tolerance path with both an injected logger and a
`console.warn` fallback, the graceful-shutdown drain via
`destroy()`, the no-op `awaitPendingWrites()` when the queue is
empty, prefix normalisation including the empty-prefix case, and
the immutability assertion that `appendMessages` / `createRun` /
`updateRun` never touch the index.
- `test/helpers.ts`'s in-memory `MapStorageClient` and its
no-append variant pick up the test-only inspection (`keys()`,
`keysWithPrefix(p)`) and fault-injection (`failPutWhenKeyMatches`,
`failPutForExactKey`, `clearPutFailures`, `delayPutWhenKeyMatches`,
`clearPutDelays`) hooks that those scenarios need.
No new runtime dependencies. `tsc -p tsconfig.pub.json` confirms the
new symbols are exposed on the published `.d.ts` surface.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 TS_MAX_MS, reverseMs(ms), and dateBucket(ms); integrates reverseMs/dateBucket into OSSAgentStore to write per-thread activity-index sidecars in background PUTs tracked by pendingIndexWrites; introduces awaitPendingWrites() and draining behavior; expands tests and adds test-client PUT failure/delay controls. ChangesThread time-index and timestamp encoding
Sequence DiagramsequenceDiagram
participant API as API Caller
participant Store as OSSAgentStore
participant Client as ObjectStorageClient
participant Pending as pendingIndexWrites
API->>Store: createThread(nowMs, metadata)
Store->>Store: compute createdAt
Store->>Client: put(meta.json) (await)
Store->>Store: build threadActivityIndexKey(createdAt, threadId)
Store->>Client: put(index entry) (background)
Store->>Pending: add in-flight promise
Store-->>API: return meta.json immediately
Client-->>Pending: settle (on failure -> logger.warn / console.warn)
API->>Store: destroy()
Store->>Store: awaitPendingWrites() -> wait for Pending settle
Store->>Client: destroy()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a time-index sidecar mechanism for the OSSAgentStore to enable newest-first listing of threads, a feature not natively supported by standard S3/OSS ListObjects APIs. It adds utility functions for reverse-timestamp encoding and timezone-aware date bucketing. The createThread method now performs a non-blocking, best-effort background write to this index, with a new awaitPendingWrites method and updated destroy logic to ensure these background tasks settle during shutdown. Review feedback suggests improving the robustness of date formatting by avoiding locale-dependent strings, using a loop in awaitPendingWrites to capture promises added during the drain, and logging full error objects rather than just messages to preserve stack traces.
| const fmt = new Intl.DateTimeFormat('en-CA', { | ||
| timeZone: timezone, | ||
| year: 'numeric', | ||
| month: '2-digit', | ||
| day: '2-digit', | ||
| }); | ||
| return fmt.format(new Date(ms)); |
There was a problem hiding this comment.
Relying on the en-CA locale to produce a YYYY-MM-DD format is fragile because Intl behavior depends on the ICU data available in the runtime environment. If the environment falls back to a different locale (like en-US), the date format could change (e.g., to MM/DD/YYYY), which would break the object storage key structure. Using formatToParts is a more robust way to construct a machine-readable date string.
const fmt = new Intl.DateTimeFormat('en-US', {
timeZone: timezone,
year: 'numeric',
month: '2-digit',
day: '2-digit',
});
const parts = fmt.formatToParts(new Date(ms));
const get = (type: string) => parts.find(p => p.type === type)!.value;
return `${get('year')}-${get('month')}-${get('day')}`;| async awaitPendingWrites(): Promise<void> { | ||
| if (this.pendingIndexWrites.size === 0) return; | ||
| await Promise.allSettled([ ...this.pendingIndexWrites ]); | ||
| } |
There was a problem hiding this comment.
The current implementation of awaitPendingWrites only waits for the promises that are in the Set at the moment the function is called. If new writes are initiated while the current ones are being awaited, they might be missed. Using a loop ensures that all pending writes, including those added during the await, are fully drained before proceeding.
| async awaitPendingWrites(): Promise<void> { | |
| if (this.pendingIndexWrites.size === 0) return; | |
| await Promise.allSettled([ ...this.pendingIndexWrites ]); | |
| } | |
| async awaitPendingWrites(): Promise<void> { | |
| while (this.pendingIndexWrites.size > 0) { | |
| await Promise.allSettled([ ...this.pendingIndexWrites ]); | |
| } | |
| } |
| (err: unknown) => { | ||
| this.pendingIndexWrites.delete(tracked); | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| this.logger.warn( | ||
| '[OSSAgentStore] failed to write thread time index threadId=%s key=%s err=%s', | ||
| threadId, | ||
| indexKey, | ||
| message, | ||
| ); | ||
| }, |
There was a problem hiding this comment.
When a background index write fails, logging only the error message (err.message) loses valuable debugging information like the stack trace. It is better to pass the entire error object to the logger, which allows the logging implementation to capture the full context.
(err: unknown) => {
this.pendingIndexWrites.delete(tracked);
this.logger.warn(
'[OSSAgentStore] failed to write thread time index threadId=%s key=%s',
threadId,
indexKey,
err,
);
},![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/agent-runtime/src/AgentStoreUtils.ts`:
- Around line 62-68: decodeReverseMs currently uses Number.parseInt which
accepts partial numeric prefixes; change it to first validate revMsString
strictly matches exactly 13 digits (e.g. /^[0-9]{13}$/) and throw a TypeError if
not, then parse to an integer, verify the parsed n is finite and within the
valid range (e.g. 0 <= n <= TS_MAX_MS), and only then return TS_MAX_MS - n;
refer to the function name decodeReverseMs and constant TS_MAX_MS when updating
the checks and error messages.
In `@core/agent-runtime/test/AgentStoreUtils.test.ts`:
- Around line 28-30: The test comment in AgentStoreUtils.test.ts includes a
local absolute path "/Users/jerry/.claude/plans/wobbly-swinging-sparkle.md"
which leaks user-specific data; update the comment to be path-agnostic by
removing or replacing the absolute path with a generic reference (e.g. "example
plan file" or "./plans/wobbly-swinging-sparkle.md") while keeping the timestamp
and expected complement calculation unchanged so the context for the test
remains clear.
🪄 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: 8bec90dc-18a5-456b-b219-29e367d82e6b
📒 Files selected for processing (5)
core/agent-runtime/src/AgentStoreUtils.tscore/agent-runtime/src/OSSAgentStore.tscore/agent-runtime/test/AgentStoreUtils.test.tscore/agent-runtime/test/OSSAgentStore.test.tscore/agent-runtime/test/helpers.ts
Six inline review comments from gemini-code-assist[bot] and CodeRabbit on PR #445 ("feat(agent-runtime): write per-thread creation-time index on createThread") plus the bot's pre-merge "Docstring Coverage 75% < 80% threshold" warning are addressed in this commit. Each item is taken on its own merits below; where a comment is declined the rationale is stated explicitly so a future reader sees the trade-off rather than wondering why the bot's suggestion wasn't applied verbatim. The original review comments live on the PR's "Files changed" tab at https://github.com/eggjs/tegg/pull/445/files . - dateBucket locale fragility (gemini-code-assist, AgentStoreUtils.ts in the bot-reviewed HEAD's dateBucket body): the previous Intl.DateTimeFormat('en-CA', { timeZone, year, month, day }).format shortcut relied on the en-CA locale's de-facto ISO-8601 short-date pattern in Node's bundled CLDR data, an empirical fact about the shipped tzdata rather than a Web-standards guarantee. Replaced with dayjs's utc + timezone plugin chain dayjs.tz(ms, timezone).format('YYYY-MM-DD'). dayjs's timezone plugin internally reaches the host's IANA tzdata through Intl.DateTimeFormat's timeZone constructor option (the ECMA-402 standard mechanism for the host's named-zone database), so the wall-clock-resolution layer consults the same Node-bundled ICU data as before; the output's character composition is now governed by dayjs's own fixed YYYY-MM-DD token template (a library-fixed string), not the en-CA locale's CLDR short-date pattern, which removes the empirical-en-CA-renders-as-ISO dependency. The default UTC case short-circuits through the ECMA-262-stdlib Date.prototype.toISOString().slice(0, 10), so the most-common code path has zero dependency on the Intl object and works on small-ICU Node builds and on runtimes shipped without the Intl namespace. New defensive guards reject non-finite, non-integer, and negative ms inputs with a domain-aware RangeError that names the bad input, replacing the previous silent NaN-Date.toISOString-throws-Invalid-time-value path. A defense-in-depth dayjs.isValid() check on the returned dayjs object also surfaces a RangeError for an unrecognised IANA zone name in the case where some future dayjs version catches and swallows the Intl exception. core/agent-runtime/package.json gains the runtime dep "dayjs": "^1.11.19". Test coverage in test/AgentStoreUtils.test.ts is extended with the case-insensitive 'utc'/'UTC'/'Utc' alias check, the cross-Shanghai-midnight Asia/Shanghai and cross-LA-evening America/Los_Angeles wall-clock conversions (bit-identical to the prior Intl-based path on a Node-official full-ICU runtime, since both implementations route through the same ICU tzdata via Intl.DateTimeFormat's named-zone resolver), the unknown-zone RangeError, the IEEE-754 signed-zero -0-collapses-to-the-epoch invariant, and the new NaN/Infinity/fractional/negative ms rejection. - lenient Number.parseInt in decodeReverseMs (CodeRabbit's inline major-priority comment on AgentStoreUtils.ts's decodeReverseMs): the bot's static analysis of the lenient-prefix-parse window is technically correct — Number.parseInt(s, 10) follows ECMA-262's "the longest prefix of s that's a valid decimal integer literal" rule, so a 13-character-plus-trailing-garbage string decodes to the leading-13-digits' integer value and a sign-prefixed 14-character string decodes by consuming the sign — both of which would, in principle, let a downstream consumer that composes the input by slicing a corrupted OSS-key suffix get a plausible-but-wrong timestamp back. The bot's suggested hardening is a /^[0-9]{13}$/ regex front-gate plus a [0, TS_MAX_MS] range check on the parsed integer. After review the hardening is declined on the contractual grounds that the only in-framework producer of inputs to decodeReverseMs is the sibling reverseMs encoder, which on its own input side enforces Number.isInteger(ms) and ms in [0, TS_MAX_MS] and on its output side produces the fixed 13-character zero-padded ASCII-decimal shape by construction (via String(TS_MAX_MS - ms).padStart(13, '0')). The round-trip decodeReverseMs(reverseMs(x)) === x is therefore closed over the entire in-framework call graph, and the lenient-parseInt-attack-surface the bot identifies is unreachable through the framework's own pipeline. The function's exported availability via the package's index.ts is intended for downstream backfill or analytics scripts that walk the OSS time-index keyspace and recover a creation-millisecond from a key's reverse-complement suffix; those consumers compose their input by string-slicing an OSS key name and are responsible for ensuring their slicing produces a 13-character ASCII-decimal string. The function's one-sentence JSDoc states this contract explicitly ("the input must be the literal 13-character output of reverseMs; behavior is undefined for any other input, since Number.parseInt's longest-valid-decimal-prefix semantics silently drop a trailing non-digit suffix and consume a leading sign character"). The function body is the bare return TS_MAX_MS - Number.parseInt(revMsString, 10);, without the regex front-gate, without the range post-check, and without the bot-reviewed HEAD's pre-existing cheap-NaN if (!Number.isFinite(n)) throw new TypeError(...) typecheck — the last of these is removed for consistency with the trust-the-contract stance, since it was a half-measure between full hardening and full input-trust. The corresponding HEAD test case 'throws TypeError when the input cannot be parsed as a decimal integer', which exercised the now-removed isFinite-branch by feeding strings whose Number.parseInt would return NaN (the empty string, 'abc', a leading-non-digit prefix), is removed alongside the runtime guard. The round-trip-correctness test 'inverts reverseMs for a representative range of inputs' and the boundary-case test 'parses the all-zero string as TS_MAX_MS and the all-nine string as zero' — neither of which depends on a rejection path — are preserved unchanged and both pass against the simplified body (Number.parseInt('0000000000000', 10) === 0 and Number.parseInt('9999999999999', 10) === 9_999_999_999_999 === TS_MAX_MS by elementary base-10 arithmetic). - awaitPendingWrites single-snapshot drain race (gemini-code-assist on OSSAgentStore.ts's previous one-shot Promise.allSettled drain): the bot identified the JavaScript-microtask-scheduling window where a still-in-flight createThread whose await this.client.put(meta.json) has not yet resolved at the moment awaitPendingWrites is entered — but whose main PUT does resolve during the surrounding Promise.allSettled([ ...this.pendingIndexWrites]) wait, then synchronously pushes its background-time-index PUT promise onto pendingIndexWrites on the way out of the createThread function body — would have its background-PUT promise land in the Set after the spread-operator-built array fed into Promise.allSettled was finalised by the engine. Promise.allSettled resolves over the pre-late-arrival snapshot's entries, the await resumes, the awaiter returns control to the surrounding OSSAgentStore.destroy caller, destroy awaits this.client.destroy() which tears down the underlying OSS HTTP-connection pool, and the late-arrival's in-flight put HTTP request is aborted mid-byte. The time-index sidecar is silently lost on the way out, even though the awaitPendingWrites contract — "drain in-flight background-index writes before client teardown" — is supposed to prevent this. The fix wraps the Promise.allSettled in a while (this.pendingIndexWrites.size > 0) { await Promise.allSettled([...this.pendingIndexWrites]); } loop, whose exit invariant is "the Set is empirically empty at function exit". That invariant is the precondition the surrounding client.destroy() needs to assert "no in-flight HTTP request routed through the client object is still pending teardown". Termination of the loop is bounded by the upstream side of the graceful-shutdown contract: the standard Egg.js beforeClose hook fires after the request-handler dispatcher has stopped accepting new requests and after the in-flight requests have all returned their HTTP responses, so by the time OSSAgentStore.destroy and therefore awaitPendingWrites are invoked, no further createThread call can be initiated and the rate of new arrivals into pendingIndexWrites goes to zero. The Set's membership is then strictly non-increasing across the per-iteration finally-handlers (which remove each settled promise from the Set), so the size hits zero in finite time and the while-condition's negation fires. The public method's JSDoc walks through this in three short paragraphs: the never-rejects guarantee, the late-arrival race window, and the cross-reference to the BackgroundTaskHelper-idiom from the project root's CLAUDE.md ("Use BackgroundTaskHelper for Async Tasks") which is the framework-wide convention for "graceful shutdown waits for the in-flight background-task set to clear". A new mocha test 'destroy() drains a write that arrives during a previous drain iteration' in test/OSSAgentStore.test.ts engineers the timing window via the in-memory test helper MapStorageClient.delayPutWhenKeyMatches(/^agent\/index\/threads- by-date\//, 80) (which inserts an 80 ms setTimeout-based artificial delay on the resolution of the underlying client's put for any key matching the time-index prefix), fires the first createThread to seed the Set, holds the destroy promise without awaiting, fires the second createThread whose synchronous tail pushes the second background-PUT promise into the Set after the destroy's first-iteration Promise.allSettled has spread-snapshot the array, then awaits the destroy promise and asserts that the in-memory map ended up with both threads' time-index entries — i.e., the late-arrival's PUT settled before the underlying client was destroyed. The test takes 82 ms wall clock, reflecting the upper bound of the second iteration's artificial 80 ms delay. - warn-on-failure carries the Error verbatim rather than the .message string (gemini-code-assist on OSSAgentStore.ts's background-PUT failure-arm): the .then(onFulfilled, onRejected) failure callback in the createThread background-PUT chain now passes the rejection reason as the trailing positional argument to the structurally-typed channel OSSAgentStoreWarnLogger.warn(message: string, ...args: unknown[]): void, rather than the previous behavior of pre-stringifying the Error to its .message property and embedding it into the format string as a %s slot. The reason the trailing-arg-Error pattern is correct is that both Node's console.warn (the default-fallback logger if no OSSAgentStoreOptions.logger is configured) and the eggjs ecosystem's egg-logger.EggLogger.warn (the natural fit for an application that wires app.logger as the warn channel) honor the util.format-style printf convention where trailing arguments beyond the format string's placeholder count are appended to the rendered output via util.inspect, and util.inspect's renderer for an Error instance includes the full stack trace and the Error.cause chain — exactly the property the bot's review asks for. Non-Error rejection reasons (JavaScript's throw statement accepts any value, so a promise's rejection reason can be a string, a plain object, or some custom error-shaped object that isn't an Error instance) are normalised through a one-line wrap-non-Errors guard, const errForLog: Error = err instanceof Error ? err : new Error(String(err));, so the trailing argument the logger receives always carries at least the wrap-site's stack frame on which an oncall-engineer can hang a "where in the OSSAgentStore code did the warn fire from" reading. The OSSAgentStoreWarnLogger interface itself gains a JSDoc that documents the util.format placeholder convention so a consumer wiring a custom structured-logger knows what shape of warn implementation the call site assumes (a printf-style format string with %s/%d/%j/%o/%O placeholders, plus variadic trailing args where a final Error is rendered with the stack via util.inspect). The corresponding existing test in test/OSSAgentStore.test.ts > 'createThread succeeds even when the index PUT throws; the failure becomes a single warn line' is updated: instead of asserting that the fourth positional warn-arg is a string matching the simulated-failure pattern, it now asserts the fourth-arg is an Error instance whose .message matches the pattern, and that the format string no longer contains an err=%s placeholder slot for the trailing Error (since the Error is the unformatted util.inspect-rendered tail-arg, not a %s-substituted string). - local absolute path in a test-comment (CodeRabbit's minor-priority inline comment on a comment line of test/AgentStoreUtils.test.ts in the bot-reviewed HEAD): the test case that pins the worked-example timestamp (UTC 2025-11-13T08:00:00.000Z, ms 1_763_020_800_000, with the expected complement 9_999_999_999_999 - 1_763_020_800_000 = 8_236_979_199_999 verifying the reverseMs encoder's arithmetic against a hand-computed reference) had a comment that named the absolute filesystem path of an author-local planning file from which the worked-example value was copy-pasted. The path leaked the author's home-directory layout under /Users/jerry and the local-tooling-storage-convention path-prefix of the agent CLI's per-task planning files, in a comment that wasn't semantically load-bearing — the surrounding magic-number arithmetic is the actual test content, the path was only a "where this magic number came from" pointer. The comment is reworked to name the PR description's own worked-example section as the source ("the worked example in the PR description's prose ... gives this timestamp"), preserving the algebra check (the explicit reduction 9_999_999_999_999 - 1_763_020_800_000 = 8_236_979_199_999 alongside the assert.strictEqual(reverseMs(knownMs), '8236979199999') line) without referencing the local filesystem. The test's it-title is also updated, from a generic "matches a known timestamp from the design document" formulation to the explicit "matches the worked example timestamp from the PR description" so the named source is the publicly-visible PR description rather than a private-to-the-author file. The bot's review comment thread on the PR's "Files changed" tab is the natural place to follow up on the specific leaked-string content if a future reader is curious about the exact wording that was scrubbed. - docstring coverage of the new public surface (CodeRabbit's pre-merge "Docstring Coverage 75.00% / 80.00% threshold" warning, a soft warning that does not block the PR's merge but flags the package's public-surface JSDoc-coverage shortfall after the bot-reviewed HEAD added the new OSSAgentStore methods without all of them having JSDoc): the new public method OSSAgentStore.awaitPendingWrites(): Promise<void> picks up a three-paragraph JSDoc covering (a) the wait-for-in-flight contract, (b) the never-rejects-resolves-on-settlement guarantee that the per-write catch handler in createThread's background chain underwrites by converting individual write failures into single warn-log lines through the OSSAgentStoreWarnLogger channel, (c) the rationale for the body's while-loop drain over fresh snapshots of pendingIndexWrites rather than a one-shot Promise.allSettled over the spread-once initial snapshot, with the late-arrival race window the C-item test exercises named explicitly, and (d) the cross-reference to the framework-wide BackgroundTaskHelper-based shutdown-drain idiom from the project root's CLAUDE.md. The private helper OSSAgentStore.threadTimeIndexKey(nowMs: number, threadId: string): string that composes the time-index sidecar's full OSS key picks up a one-paragraph JSDoc on the key-shape contract ({prefix}index/threads-by-date/{YYYY-MM-DD}/{13- digit reverse-complement of the millisecond timestamp}_{ threadId}, with the prefix's trailing-slash normalisation handled by the constructor's raw && !raw.endsWith('/') ? raw + '/' : raw line). The new structurally-typed OSSAgentStoreWarnLogger interface itself (the optional logger field on OSSAgentStoreOptions's static type) gains a JSDoc explaining the util.format-style printf-with-variadic-Error placeholder convention that the call sites in createThread's failure arm assume the consumer's logger honors (both Node's console.warn and egg-logger.EggLogger.warn do, so the contract holds for the two natural wirings without the consumer having to glue anything). With the public method covered, the private helper's contract documented, and the interface's placeholder-convention spelled out, the CodeRabbit pre-merge tally is expected to clear the 80% threshold on the next re-scan of this branch. The post-fixup local test suite for the package, invoked as npm test --workspace=core/agent-runtime from the worktree root (which expands per the package's "test": "mocha" script to mocha walking core/agent-runtime/test/**/*.test.ts), runs 156 cases to a clean 156-passing-zero-failing finish in 11 seconds wall clock on the author's macOS-on-Node-20.20.0 local environment, with the in-memory MapStorageClient test helper standing in for the ali-oss-client SDK so no network or cloud-credentials are touched. The two pre-existing flakes the original PR description called out — test/AgentRuntime.test.ts > cancelRun > 'should fail the run and leave the thread empty when cancel waits past the commit timeout' and the sibling 'should not overwrite watchdog-set Failed with Completed when executor finishes naturally without committing', both reporting an internal "AgentTimeoutError is not a constructor" from a stray watchdog-timer-fire after the test's own time bound — did not surface on this particular run, but the PR description's framing of them as "reproducing identically on the stock master@f4245c37 source without any of this change's diff, and therefore unrelated to this branch" remains the accurate description of those two cases as an upstream-master property independent of any single execution's outcome. The full-repo lint invoked as npm run lint at the workspace root (which expands per the root package.json's "lint": "eslint . --ext .ts" script to ESLint walking the entire monorepo's TypeScript source under the eslint-config-egg/typescript ruleset) finishes at 0 errors, 10 warnings, with all 10 warnings sitting in unrelated files: jsdoc/check-param-names and jsdoc/require-param-description and jsdoc/require-returns-description and jsdoc/require-returns-type warnings in core/agent-tracing/src/ClaudeAgentTracer.ts, core/common-util/src/ModuleConfig.ts, core/dal-runtime/src/DataSource.ts, and core/langchain-decorator/src/decorator/GraphEdge.ts; and import/no-duplicates warnings on duplicate imports of @eggjs/tegg-dynamic-inject-runtime and @eggjs/tegg in standalone/service-worker/src/StandaloneEggObjectFactory.ts and standalone/service-worker/src/mcp/MCPServerHelper.ts. All ten predate this branch's divergence from master and none of them touch any file modified by this commit, so the modified-files-subset that pre-commit hooks like lint-staged would lint is clean. The eslint default treats warnings as non-error so the lint command exits 0 overall, which the npm run ci chain (prepare-test && lint && test) requires. The aggregate diff vs. the bot-reviewed HEAD ee775f7, per git diff --stat ee775f7..HEAD invoked from the worktree root, is "5 files changed, 461 insertions(+), 127 deletions(-)": core/agent-runtime/package.json +1 line (the new "dayjs": "^1.11.19" direct dep), core/agent-runtime/src/ AgentStoreUtils.ts 151 lines changed net (the dayjs imports and extends at the top of the file, the dateBucket input guard and the UTC short-circuit and the dayjs.tz timezone branch and the unknown-zone defense-in-depth check, the decodeReverseMs body simplification to a one-line parseInt and the docstring compression to a single sentence, plus the cumulative wording tweaks to the adjacent reverseMs JSDoc that the working-tree fixup picked up in passing), core/agent-runtime/src/OSSAgentStore.ts 258 lines changed net (the new OSSAgentStoreWarnLogger interface and its util-format-convention JSDoc, the new constructor fields for logger and dateTimezone with their JSDoc-documented defaults, the new private threadTimeIndexKey helper and its JSDoc on the key-shape contract, the public awaitPendingWrites method with the while-loop drain and its three-paragraph JSDoc, the destroy method's drain-before-underlying-teardown sequencing with its inline comment, the createThread method's fire-and-forget background-PUT chain with the pendingIndexWrites Set tracking and the non-Error wrap on the rejection-reason and the trailing-Error-arg warn-call shape, and the OSSAgentStore-class JSDoc explaining the storage layout including the new index/threads-by-date/YYYY-MM-DD/revMs_threadId path shape and the sortable-newest-first-under-ListObjects-ASC property that the reverse-complement encoding provides), core/agent-runtime/test/AgentStoreUtils.test.ts 80 lines changed net (the path-leak-comment cleanup described in the E bullet above, the dateBucket cross-timezone and unknown-zone and ms-shape-rejection new test coverage, and the decodeReverseMs TypeError-rejection-block removal as the test-side mirror of the runtime-guard removal described in the B bullet), and core/agent-runtime/test/ OSSAgentStore.test.ts 98 lines changed net (the new 'destroy() drains a write that arrives during a previous drain iteration' test case for the C item's late-arrival drain coverage, plus the warn-on-failure test's assertion-shape update for the D item's trailing-Error-arg observability change). The .gitignore'd root-level package-lock.json that npm install regenerated to record the new dayjs direct dep on disk is intentionally not part of the commit per the project's lockfile-not-committed convention (eggjs/tegg root .gitignore line 16 matches package-lock.json exactly), which lets each downstream consumer of the @eggjs/agent-runtime package pick its own dayjs minor-version under the caret-range constraint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed the inline review feedback from @gemini-code-assist and @coderabbitai in commit The decline-with-rationale on the On the testing side, the post-fixup local suite invoked as The full-repo lint The aggregate Happy to follow up on any of the six items if the rationale isn't quite right for the project's broader stance on a specific kind of validation or observability question — the trade-off framings in the commit body and above are my best read of the bots' threads against the conventions visible in the rest of the agent-runtime package, but the eggjs/tegg maintainers are obviously the authoritative voice on those conventions. |
|
Thanks for the detailed breakdown and for addressing the review feedback, @jerryliang64. The rationale for the |
|
The ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/agent-runtime/src/AgentStoreUtils.ts (1)
76-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden
decodeReverseMsto enforce strict 13-digit input validation.The past review comment on this function remains unaddressed. While the updated JSDoc documents that "behavior is undefined" for malformed input and places the contract burden on callers,
Number.parseIntis permissive: it parses"123abc"as123and"abc"asNaN, causing the function to returnNaNsilently rather than throwing. Since this is a public export (re-exported from the package root per PR objectives), external callers can invoke it with arbitrary strings. Defensive programming for public utilities dictates strict validation and loud failures.🛡️ Recommended fix to add strict validation
export function decodeReverseMs(revMsString: string): number { + if (!/^\d{13}$/.test(revMsString)) { + throw new TypeError(`decodeReverseMs: expected 13-digit decimal string, got ${JSON.stringify(revMsString)}`); + } - return TS_MAX_MS - Number.parseInt(revMsString, 10); + const n = Number(revMsString); + if (n > TS_MAX_MS) { + throw new RangeError(`decodeReverseMs: parsed value ${n} exceeds TS_MAX_MS`); + } + return TS_MAX_MS - n; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/agent-runtime/src/AgentStoreUtils.ts` around lines 76 - 78, The decodeReverseMs export currently uses Number.parseInt which silently accepts malformed strings; change decodeReverseMs to validate revMsString strictly before computing TS_MAX_MS - value: require revMsString to match /^\d{13}$/ (exactly 13 decimal digits), otherwise throw a clear Error (e.g., "decodeReverseMs: expected 13-digit millisecond string"); after validation convert with Number(revMsString) (or parseInt with radix 10) and return TS_MAX_MS - numericValue, ensuring the function loudly fails on invalid input instead of returning NaN.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@core/agent-runtime/src/AgentStoreUtils.ts`:
- Around line 76-78: The decodeReverseMs export currently uses Number.parseInt
which silently accepts malformed strings; change decodeReverseMs to validate
revMsString strictly before computing TS_MAX_MS - value: require revMsString to
match /^\d{13}$/ (exactly 13 decimal digits), otherwise throw a clear Error
(e.g., "decodeReverseMs: expected 13-digit millisecond string"); after
validation convert with Number(revMsString) (or parseInt with radix 10) and
return TS_MAX_MS - numericValue, ensuring the function loudly fails on invalid
input instead of returning NaN.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aebb2c32-ae2c-48f3-9c48-f5343e863d31
📒 Files selected for processing (5)
core/agent-runtime/package.jsoncore/agent-runtime/src/AgentStoreUtils.tscore/agent-runtime/src/OSSAgentStore.tscore/agent-runtime/test/AgentStoreUtils.test.tscore/agent-runtime/test/OSSAgentStore.test.ts
✅ Files skipped from review due to trivial changes (1)
- core/agent-runtime/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- core/agent-runtime/test/AgentStoreUtils.test.ts
- core/agent-runtime/test/OSSAgentStore.test.ts
- core/agent-runtime/src/OSSAgentStore.ts
Two bots, six points on #445 : - dateBucket: dayjs's `utc` + `timezone` plugins replace the `Intl.DateTimeFormat('en-CA', { timeZone, year, month, day })` ISO-by-CLDR-locale-convention shortcut; UTC keeps the `Date.prototype.toISOString().slice(0, 10)` ECMA-262 short-circuit. Adds `RangeError`s on non-finite/negative `ms` and on an unknown IANA zone, and a new runtime dep `"dayjs": "^1.11.19"` in `core/agent-runtime/package.json`. [gemini-code-assist] - decodeReverseMs (declined): the lenient-`Number.parseInt` "longest valid decimal prefix" window the bot flags is unreachable inside the framework because `reverseMs` is the sole in-tegg producer of the function's input and it emits a fixed-width 13-ASCII-digit string by construction. The exported one-sentence JSDoc states the input precondition for downstream consumers that slice OSS-key suffixes. Body is the one-liner `return TS_MAX_MS - Number.parseInt(revMsString, 10);`, and the HEAD's pre-existing `if (!Number.isFinite(n)) throw new TypeError(...)` cheap-NaN typecheck plus the matching `'throws TypeError when the input cannot be parsed as a decimal integer'` mocha case are removed for consistency with the trust-the-contract stance. [@coderabbitai] - awaitPendingWrites: wrap the inner `await Promise.allSettled([...this.pendingIndexWrites])` in `while (this.pendingIndexWrites.size > 0)` so a still-in-flight `createThread` whose synchronous tail pushes its background-time-index PUT promise onto the Set during a prior iteration's `allSettled` wait — and therefore lands after the spread-snapshot feeding that `allSettled` was finalized — is captured on the next iteration's fresh snapshot. The exit invariant "the Set is observed empty" is the precondition `this.client.destroy()`'s underlying-HTTP-pool teardown needs. New mocha test `'destroy() drains a write that arrives during a previous drain iteration'` in `test/OSSAgentStore.test.ts` engineers the timing window via the in-memory `MapStorageClient.delayPutWhenKeyMatches(/^agent\/index\/threads-by-date\//, 80)` test-helper, an 80ms artificial setTimeout-based delay on any underlying-client `put` against a time-index-prefixed key. [gemini-code-assist] - warn-on-failure carries the Error: the rejection arm of the background-index-PUT's `.then(onFulfilled, onRejected)` chain passes the rejection reason verbatim as the trailing positional arg to `OSSAgentStoreWarnLogger.warn(format, ...args): void` instead of pre-stringifying it through `.message` into a `%s` slot in the format string, so the `util.format`-style trailing-arg `util.inspect` rendering that both Node's `console.warn` and the eggjs ecosystem's `egg-logger.EggLogger.warn` honor reaches for the stack and the ECMA-262 `Error.cause` chain. Non-`Error` rejection reasons (since the language's `throw` statement permits any value) are normalized through a `const errForLog: Error = err instanceof Error ? err : new Error(String(err));` one-liner so the trailing arg always has at least the wrap-site's stack frame. The matching warn-when-PUT-fails test's fourth-positional-arg assertion is updated from the previous string-pattern-match shape to the new `instanceof Error`-plus-`.message`-regex shape, plus a negative check on the format string no-longer-carrying the previous `err=%s` placeholder. [gemini-code-assist] - test-comment author-local-path scrub: the `'matches a known timestamp from the design document'` mocha case in `test/AgentStoreUtils.test.ts` had a comment naming the absolute filesystem path of a Claude Code agent's per-task planning file under the author's home directory. The path leaks the home-directory layout and the agent's plan-file random-slug naming convention. The path is removed; the new comment cites the worked-example timestamp arithmetic directly (UTC `2025-11-13T08:00:00.000Z` → ms `1_763_020_800_000` → complement `9_999_999_999_999 - 1_763_020_800_000 = 8_236_979_199_999` which equals the expected `reverseMs(1_763_020_800_000)` string `'8236979199999'`), and the test's `it` title is retitled from `'matches a known timestamp from the design document'` to `'matches the worked example timestamp from the PR description'` so the named source-of-the-example is the publicly-visible PR description prose rather than the private planning file. [@coderabbitai] - docstring coverage on the new public surface that the original `ee775f7b feat(agent-runtime): write per-thread creation-time index on createThread` commit introduced: the new public method `OSSAgentStore.awaitPendingWrites(): Promise<void>`, the new private helper `OSSAgentStore.threadTimeIndexKey(nowMs: number, threadId: string): string`, and the new structurally-typed `OSSAgentStoreWarnLogger` interface (the static type of the optional `OSSAgentStoreOptions.logger` field, satisfied by both Node's bare `console` and egg-logger's `EggLogger` since both implement the `(format: string, ...args: unknown[]) => void` shape under the `util.format` trailing-arg-`util.inspect`-on-Error convention) each gain a JSDoc block — the never-rejects drain-then-empty-Set contract, the `{prefix}index/threads-by-date/{YYYY-MM-DD}/{revMs13}_{threadId}` key-shape composition that the package's read path never consults, and the `util.format` `%s`/`%d`/`%j`/`%o`/`%O` placeholder convention with a trailing-`Error`-rendered-via-`util.inspect` clarification, respectively — so the bot's pre-merge "Docstring Coverage 75.00% / 80.00% threshold" soft warning is expected to clear on the next webhook-driven re-scan of the branch. [@coderabbitai pre-merge check] Local `npm test --workspace=core/agent-runtime` from the worktree root ran 156/156 mocha cases passing in roughly 11 seconds wall clock on Node v20.20.0 on macOS, using the in-memory `MapStorageClient` test helper in place of the `ali-oss-client` SDK so no network or cloud credentials are exercised. Full-repo `npm run lint` (`eslint . --ext .ts` against the project's `eslint-config-egg/typescript` ruleset) is 0 errors and 10 unrelated pre-existing warnings — all `jsdoc/check-param-names`, `jsdoc/require-param-description`, `jsdoc/require-returns-description`, `jsdoc/require-returns-type`, and `import/no-duplicates` reports in files this branch's diff doesn't touch (across `core/agent-tracing/`, `core/common-util/`, `core/dal-runtime/`, `core/langchain-decorator/`, and `standalone/service-worker/`), so the modified-file subset a pre-commit `lint-staged` would re-lint is clean. The two pre-existing `test/AgentRuntime.test.ts > cancelRun > 'AgentTimeoutError is not a constructor'` flakes the original PR description names — framed as reproducing identically on stock `master@f4245c3` and therefore unrelated to this branch — did not surface in this particular post-fixup run, but the upstream-master observation in the original description is independent of any single run's outcome. The repo-root `package-lock.json` matches line 16 of the project `.gitignore` per the egg-ecosystem's per-consumer-resolution convention, so the lockfile update `npm install` performed locally on disk to record the new direct dayjs dep is a worktree-only effect that does not enter this commit's tree. A separate follow-up commit on this branch will trim the per-block JSDoc and inline-comment volume the previous-session Claude-Code-assisted fixup pass introduced — the on-disk dissertations on ECMA-262 vs. CLDR locale stability of the `'en-CA'` vs. the dayjs-token-template-driven YYYY-MM-DD shape, the IEEE-754 signed-zero-collapses-to-the-epoch invariant on the new `dateBucket` ms-shape guard, the JavaScript run-to-completion microtask-ordering argument for the `createThread` background-PUT chain's `tracked` self-reference inside the `.then` callback, the `.then(onFulfilled, onRejected)` two-argument-form-as-unhandled-rejection-silencer note, the `util.format`-`%s`-`%d`-`%j`-`%o`-`%O` placeholder reference catalogue on the `OSSAgentStoreWarnLogger` interface, the `BackgroundTaskHelper`-cross-reference on the `awaitPendingWrites` drain-loop, the storage-layout ASCII-art surrounding prose on the class-level `OSSAgentStore` JSDoc, the per-iteration `.finally` set-membership-maintenance description on the `pendingIndexWrites` field, the matching test-file commentary on the dayjs-tz-vs-Intl bit-identicality argument for the cross-Asia/Shanghai-no-DST and cross-LA-PST-UTC-8-in-November cases, and the microtask-FIFO-discipline timing argument preamble on the new drain-race-engineering test — all of which exceed the surrounding `eggjs/tegg` source tree's prevailing terseness norm and the project root's `CLAUDE.md` plus the user's global `~/.claude/CLAUDE.md` together's "default to no comments unless the WHY is non-obvious; one short line max; no multi-paragraph docstrings; no references in the code to alternative implementations that aren't in the tree" guidance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jerryliang64
force-pushed
the
feat/agent-runtime-thread-time-index
branch
from
ab5bd68 to
50ac800
Compare
| `dateBucket: ms must be a nonnegative integer Unix-millisecond timestamp, got ${ms}`, | ||
| ); | ||
| } | ||
| return new Date(ms).toISOString().slice(0, 10); |
There was a problem hiding this comment.
这里的日期format是YYYY-MM-DD么？为什么不直接用dayjs 的 .format 方法？
| private readonly client: ObjectStorageClient; | ||
| private readonly prefix: string; | ||
| private readonly logger: OSSAgentStoreWarnLogger; | ||
| private readonly pendingIndexWrites = new Set<Promise<void>>(); |
There was a problem hiding this comment.
这个理论size有多大，是否有内存泄漏的可能性
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/agent-runtime/test/helpers.ts (1)
8-14: 💤 Low value
putFailureExactis unused dead state.Neither
MapStorageClientnorMapStorageClientWithoutAppendexposes a way to add toputFailureExact, soshouldFailPut'sexact.has(key)branch can never fire. Either drop the set (and theexactparameter), or expose a publicfailPutForKey(key: string)so callers can pre-register exact failures.♻️ Proposed simplification (remove the dead set)
-function shouldFailPut( - key: string, - exact: ReadonlySet<string>, - pattern: RegExp | null, -): boolean { - return exact.has(key) || (pattern !== null && pattern.test(key)); -} +function shouldFailPut(key: string, pattern: RegExp | null): boolean { + return pattern !== null && pattern.test(key); +}export class MapStorageClient implements ObjectStorageClient { private readonly store = new Map<string, string>(); - private readonly putFailureExact = new Set<string>(); private putFailurePattern: RegExp | null = null; private readonly putDelays: PutDelayRule[] = [];async put(key: string, value: string): Promise<void> { - if (shouldFailPut(key, this.putFailureExact, this.putFailurePattern)) { + if (shouldFailPut(key, this.putFailurePattern)) { throw new Error(`MapStorageClient: simulated PUT failure for ${key}`); }(Apply the same three changes to
MapStorageClientWithoutAppend.)Also applies to: 29-29, 76-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/agent-runtime/test/helpers.ts` around lines 8 - 14, The putFailureExact set is dead state because no clients add to it, so update shouldFailPut and related types: remove the exact parameter and the exact.has(key) branch from shouldFailPut (use only pattern), and delete the putFailureExact property from MapStorageClient and MapStorageClientWithoutAppend (or alternatively add a public method failPutForKey(key: string) on both classes if you want to keep exact-match failures); apply the same change points where putFailureExact is referenced (constructor/state and any tests) so callers compile and behavior remains based solely on the pattern or the new public failPutForKey API if you choose that option.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/agent-runtime/test/helpers.ts`:
- Around line 8-14: The putFailureExact set is dead state because no clients add
to it, so update shouldFailPut and related types: remove the exact parameter and
the exact.has(key) branch from shouldFailPut (use only pattern), and delete the
putFailureExact property from MapStorageClient and MapStorageClientWithoutAppend
(or alternatively add a public method failPutForKey(key: string) on both classes
if you want to keep exact-match failures); apply the same change points where
putFailureExact is referenced (constructor/state and any tests) so callers
compile and behavior remains based solely on the pattern or the new public
failPutForKey API if you choose that option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c03c9fd8-e2ba-45ac-92b3-1a8d07dab2f4
📒 Files selected for processing (5)
core/agent-runtime/src/AgentStoreUtils.tscore/agent-runtime/src/OSSAgentStore.tscore/agent-runtime/test/AgentStoreUtils.test.tscore/agent-runtime/test/OSSAgentStore.test.tscore/agent-runtime/test/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- core/agent-runtime/src/OSSAgentStore.ts
1 participant
Summary
{prefix}index/threads-by-date/YYYY-MM-DD/{revMs13}_{threadId}on every successfulOSSAgentStore.createThread, so analytics tooling that points directly at the bucket can enumerate threads by creation day without first GET'ing eachmeta.json.9_999_999_999_999 - Date.now(), zero-padded to 13 chars), so the natural ASCII-ascending order an OSS / S3ListObjectscall returns is the same as creation-time descending within a date directory — a workaround for the lack of a reverse-order list in the S3 protocol family. The body is a compact{"threadId":…, "createdAt":<Unix-seconds, same value as meta.json>, "metadata":{…}}JSON, so a downstream consumer can avoid a second GET for the canonical metadata.Setand contributes zero latency tocreateThread's happy path. A rejection becomes onewarnlog line through a structurally-typed injected logger (falling back toconsole.warn) and never propagates. A new publicOSSAgentStore.awaitPendingWrites(): Promise<void>drains the queue without ever rejecting;destroy()calls it before tearing down the underlying client so graceful shutdown doesn't drop in-flight writes.OSSAgentStoreOptionsfields, both optional with backwards-compatible defaults so existing call sites compile unchanged:logger?: { warn(message, ...args): void }(compatible withEggLoggerandconsole; passapp.loggerdirectly) anddateTimezone?: string(IANA name; defaults to'UTC'so workers in different physical regions agree on the day boundary, pass'Asia/Shanghai'etc. to follow a local-calendar analytics convention).TS_MAX_MS(the 13-digit ceiling, ≈ year 2286),reverseMs(ms),decodeReverseMs(revMs13), anddateBucket(ms, tz?). They live inAgentStoreUtilsand ship via the existingexport *re-export fromindex.ts.AgentStoreinterface, theObjectStorageClientinterface, andAgentRuntimeare intentionally not touched. The read path (getThread) does not consult the index, so the index can be eventually consistent — or entirely missing for historical threads — without affecting read semantics. Backfilling pre-existing threads is the consumer's responsibility; the exported helpers make it a one-liner over the existing{prefix}threads/*/meta.jsonkeyspace.What an OSS prefix actually looks like
Given
prefix: 'agent/'and four threads created at UTC2025-11-13 08:00:00.000,08:00:01.234,10:00:00.000, and2025-11-14 00:00:00.000, the bucket grows by the following four extra objects (alongside the existingagent/threads/{id}/meta.jsonfiles, which are unchanged):9_999_999_999_999 − 1_763_028_000_000 = 8_236_971_999_999checks the algebra. A consumer that wants raw milliseconds back from a key callsdecodeReverseMs(suffix), which isTS_MAX_MS − parseInt(suffix, 10). The body of each of the four entries is something like{"threadId":"thread_018f…","createdAt":1763028000,"metadata":{}}, with thecreatedAtUnix-second value byte-identical toagent/threads/<that-id>/meta.json'screatedAt, because both derive from a singleDate.now()reading insidecreateThread.Test plan
mochaincore/agent-runtime/— the existing 100+ cases still pass, plus 24 new cases (12 in a newtest/AgentStoreUtils.test.tsand 12 in a newdescribe('thread time index')block at the end oftest/OSSAgentStore.test.ts). The only two failures in the workspace's full suite aretest/AgentRuntime.test.ts > cancelRun > "should fail the run and leave the thread empty when cancel waits past the commit timeout"and"should not overwrite watchdog-set Failed with Completed when executor finishes naturally without committing", both reportingUncaught TypeError: AgentTimeoutError is not a constructorfrom a stray watchdog timer firing insrc/AgentRuntime.ts:565— confirmed to reproduce identically on the stockmaster@f4245c37source without any of this PR's changes, so they are pre-existing flakes unrelated to this change.eslint core/agent-runtime --ext .ts— clean. (Auto-fix touched two stylistic nits in the new test cases — alignment whitespace before// 2025-11-13…comments and[ x ]array-bracket-spacing — and they were re-reviewed by hand.)tsc -p tsconfig.pub.jsonincore/agent-runtime/— the generateddist/index.d.tsexposes the new public surface:TS_MAX_MS,reverseMs,decodeReverseMs,dateBucket,OSSAgentStore.awaitPendingWrites,OSSAgentStoreOptions.logger,OSSAgentStoreOptions.dateTimezone, and theOSSAgentStoreWarnLoggerinterface.The new test coverage explicitly exercises: the precise key shape (regex-matched), the body JSON contents and the snapshot-not-reference treatment of
metadata, the within-day ASC == time-DESC invariant across three timestamps, the UTC-midnight day boundary, thedateTimezone: 'Asia/Shanghai'override flipping the bucket name versus the same physical instant under default UTC, the non-blocking-latency property (the test client artificially delays index-key PUTs by 120 ms and assertscreateThreadreturns in well under that), the warn-logged failure path with both an explicit injected logger and theconsole.warnfallback, thedestroy()drain semantics with the same 120 ms artificial delay, prefix normalisation in both the empty-prefix and non-trailing-slash cases, the no-op behaviour ofawaitPendingWrites()on an empty queue, and the immutability invariant thatappendMessages/createRun/updateRunnever touch anyindex/…key after the initial creation. TheMapStorageClientandMapStorageClientWithoutAppendtest helpers gainedkeys(),keysWithPrefix(prefix),failPutWhenKeyMatches(re),failPutForExactKey(k),clearPutFailures(),delayPutWhenKeyMatches(re, ms), andclearPutDelays()hooks for these scenarios — all confined to the test helpers, none on the productionObjectStorageClientinterface.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests