Split bot runtime and tooling#62
Conversation
![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 refactors the iCKB bot by modularizing its core logic into a private @ickb/bot package and transitioning the apps/bot package into a CLI adapter. It introduces a new production launcher and an incident collector script, while updating the rebalancing policy to use a 180-epoch ring model for pool inventory management, excess withdrawals, and reserve recovery. Feedback on the changes suggests preventing precision loss in the incident collector's time-bound parsing by validating safe integers using BigInt first, and using the in operator for cleaner type narrowing when extracting custom properties from Error objects.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const amount = Number(amountText); | ||
| if (!Number.isSafeInteger(amount)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
When validating if a string represents a safe integer, parse it as a BigInt first and compare it against Number.MAX_SAFE_INTEGER before converting to a Number to avoid precision loss for very large values.
| const amount = Number(amountText); | |
| if (!Number.isSafeInteger(amount)) { | |
| return null; | |
| } | |
| const parsed = BigInt(amountText); | |
| if (parsed > BigInt(Number.MAX_SAFE_INTEGER)) { | |
| return null; | |
| } | |
| const amount = Number(parsed); |
References
- When validating if a string represents a safe integer, parse it as a 'BigInt' first and compare it against 'Number.MAX_SAFE_INTEGER' before converting to a 'Number' to avoid precision loss for very large values.
There was a problem hiding this comment.
Implemented in 3dedb2a: relative amount parsing now uses BigInt(amountText), compares against BigInt(Number.MAX_SAFE_INTEGER), and converts to Number only after that bound check.
| function errorExtraFields(error: Error, seen: Set<unknown>): Record<string, unknown> { | ||
| const fields: Record<string, unknown> = {}; | ||
| const entries = Object.entries(error); | ||
| for (const key of ERROR_EXTRA_KEYS) { | ||
| const entry = entries.find(([entryKey]) => entryKey === key); | ||
| if (entry !== undefined) { | ||
| fields[key] = logValue(entry[1], seen); | ||
| } | ||
| } | ||
| return fields; | ||
| } |
There was a problem hiding this comment.
In TypeScript, use the in operator to narrow types when checking for optional or custom properties on objects (like Error). This avoids unnecessary type casting to any or convoluted Object.entries lookups while maintaining type safety in strict mode.
function errorExtraFields(error: Error, seen: Set<unknown>): Record<string, unknown> {
const fields: Record<string, unknown> = {};
for (const key of ERROR_EXTRA_KEYS) {
if (key in error) {
fields[key] = logValue((error as Record<string, unknown>)[key], seen);
}
}
return fields;
}References
- In TypeScript, use the 'in' operator to narrow types when checking for optional or custom properties on objects (like 'Error'). This avoids unnecessary type casting to 'any' while maintaining type safety in strict mode.
There was a problem hiding this comment.
Implemented in 3dedb2a: errorExtraFields now uses key in error and reads through error as Record<string, unknown>, so optional error evidence is kept without scanning Object.entries.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the iCKB bot by extracting its core runtime, rebalancing policy, and observability logic into a new private packages/bot package, leaving apps/bot as a CLI adapter. It also introduces a production launcher with run-slot retention, an incident collector script, and updates to packages/node-utils for better transaction validation and error handling. Feedback on the changes suggests restoring Date object serialization in the stringifyBigInt helper within packages/sdk to prevent potential runtime errors and maintain compatibility with previous behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| type JsonReplacerInput = string | number | boolean | bigint | null | object | undefined; | ||
|
|
||
| function stringifyBigInt( | ||
| _key: string, | ||
| value: JsonReplacerInput, | ||
| ): Exclude<JsonReplacerInput, bigint> | string { | ||
| return typeof value === "bigint" ? value.toString() : value; | ||
| } |
There was a problem hiding this comment.
When implementing custom JSON serialization or logging helpers for errors, explicitly handle both bigint and Date values: serialize bigint as a decimal string, valid dates as ISO strings, and invalid dates as null to prevent runtime serialization errors and maintain precision.
The custom JSON serialization helper stringifyBigInt currently only handles bigint values and omits the Date object handling that was present in the previous implementation. We should restore this handling to comply with the repository's rules.
| type JsonReplacerInput = string | number | boolean | bigint | null | object | undefined; | |
| function stringifyBigInt( | |
| _key: string, | |
| value: JsonReplacerInput, | |
| ): Exclude<JsonReplacerInput, bigint> | string { | |
| return typeof value === "bigint" ? value.toString() : value; | |
| } | |
| type JsonReplacerInput = string | number | boolean | bigint | null | object | undefined; | |
| function stringifyBigInt( | |
| this: Record<string, unknown> | unknown[], | |
| key: string, | |
| value: JsonReplacerInput, | |
| ): Exclude<JsonReplacerInput, bigint> | string | null { | |
| const original = Array.isArray(this) ? this[Number(key)] : this[key]; | |
| if (original instanceof Date) { | |
| return Number.isNaN(original.getTime()) ? null : original.toISOString(); | |
| } | |
| return typeof value === "bigint" ? value.toString() : value; | |
| } |
References
- When implementing custom JSON serialization or logging helpers for errors, explicitly handle both bigint and Date values: serialize bigint as a decimal string, valid dates as ISO strings, and invalid dates as null to prevent runtime serialization errors and maintain precision.
There was a problem hiding this comment.
No code change here. JSON.stringify calls Date#toJSON() before the replacer runs, so the current stringifyBigInt still serializes valid Dates as ISO strings and invalid Dates as null. packages/sdk/test/index.ts covers that alongside BigInt string serialization.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request restructures the repository by moving the core bot implementation into a private packages/bot package and refactoring apps/bot as a CLI adapter. It also updates documentation, introduces new launcher and incident collection scripts, and refines the rebalancing policy to use a ring-based model. The code review feedback focuses on improving robustness and efficiency: making temporary file cleanup and log pruning deletions best-effort to prevent critical failures, replacing manual timestamp scanning with a regular expression, and explicitly handling Date objects in the custom JSON serialization helper to avoid data loss.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } finally { | ||
| await closeArtifactHandle(handle); | ||
| if (tempCreated) { | ||
| await removeTemporaryArtifact(tempPath, writeSucceeded); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async function removeTemporaryArtifact( | ||
| tempPath: string, | ||
| writeSucceeded: boolean, | ||
| ): Promise<void> { | ||
| try { | ||
| await fsPromises.rm(tempPath, { force: true }); | ||
| } catch (error) { | ||
| if (writeSucceeded) { | ||
| throw error; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In writeContentAddressedArtifact, throwing an error during temporary file cleanup (when writeSucceeded is true) will cause the entire successful write operation to fail. Furthermore, if writeSucceeded is false, throwing a cleanup error will mask the original write error that caused the failure in the first place. Temporary file cleanup should be best-effort and should not throw errors to the caller.
} finally {
await closeArtifactHandle(handle);
if (tempCreated) {
await removeTemporaryArtifact(tempPath);
}
}
}
async function removeTemporaryArtifact(
tempPath: string,
): Promise<void> {
try {
await fsPromises.rm(tempPath, { force: true });
} catch {
// Ignore cleanup failures to avoid masking original errors or failing successful writes
}
}There was a problem hiding this comment.
Fixed in ba2ad88: temporary artifact cleanup is now best-effort, so cleanup failures no longer fail a successful content-addressed write or mask the original write/link error. The artifact cleanup test now covers the successful-write case.
| : summarizeLaunch(record, summary, timestamp); | ||
| } | ||
|
|
||
| function parseRecordTimestamp(timestamp: unknown): Date | null { | ||
| if (typeof timestamp !== "string") { | ||
| return null; | ||
| } | ||
| const parsed = new Date(timestamp); | ||
| return Number.isNaN(parsed.getTime()) ? null : parsed; | ||
| } | ||
|
|
||
| function parseTextTimestamp(line: string): Date | null { | ||
| for (let index = 0; index < line.length; index += 1) { |
There was a problem hiding this comment.
The character-by-character manual scanning loop in parseTextTimestamp is highly inefficient, especially for long log lines (such as stack traces or serialized JSON). Using a compiled Regular Expression is significantly faster and cleaner.
const ISO_TIMESTAMP_REGEX = /\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d{1,9})?(?:Z|[+-]\d{2}:\d{2})/;
function parseTextTimestamp(line: string): Date | null {
const match = ISO_TIMESTAMP_REGEX.exec(line);
if (match === null) {
return null;
}
const parsed = new Date(match[0]);
return Number.isNaN(parsed.getTime()) ? null : parsed;
}There was a problem hiding this comment.
Fixed in ba2ad88: parseTextTimestamp now uses a compiled ISO timestamp regexp and preserves the existing Date validity check before accepting a match.
| type JsonReplacerInput = string | number | boolean | bigint | null | object | undefined; | ||
|
|
||
| function stringifyBigInt( | ||
| _key: string, | ||
| value: JsonReplacerInput, | ||
| ): Exclude<JsonReplacerInput, bigint> | string { | ||
| return typeof value === "bigint" ? value.toString() : value; | ||
| } |
There was a problem hiding this comment.
The custom JSON serialization helper stringifyBigInt does not explicitly handle Date objects. According to the general rules, custom JSON serialization or logging helpers must explicitly handle Date objects by converting valid dates to ISO strings and invalid dates to null to prevent silent data loss and avoid throwing runtime errors during serialization.
| type JsonReplacerInput = string | number | boolean | bigint | null | object | undefined; | |
| function stringifyBigInt( | |
| _key: string, | |
| value: JsonReplacerInput, | |
| ): Exclude<JsonReplacerInput, bigint> | string { | |
| return typeof value === "bigint" ? value.toString() : value; | |
| } | |
| function stringifyBigInt( | |
| this: Record<string, unknown> | unknown[], | |
| key: string, | |
| value: unknown, | |
| ): unknown { | |
| const original = Array.isArray(this) ? this[Number(key)] : this[key]; | |
| if (original instanceof Date) { | |
| return Number.isNaN(original.getTime()) ? null : original.toISOString(); | |
| } | |
| return typeof value === "bigint" ? value.toString() : value; | |
| } |
References
- When implementing custom JSON serialization or logging helpers, explicitly handle Date objects by converting valid dates to ISO strings and invalid dates to null to prevent silent data loss and avoid throwing runtime errors during serialization.
There was a problem hiding this comment.
No code change here. This helper intentionally relies on JSON.stringify invoking Date#toJSON() before the replacer, which already serializes valid Dates as ISO strings and invalid Dates as null. packages/sdk/test/index.ts covers that exact Date behavior together with BigInt string serialization.
| } | ||
| total -= group.size; | ||
| } |
There was a problem hiding this comment.
If deleting an old log file or directory fails (e.g., due to a temporary file lock or permission issue), safeRm will throw an error and crash the entire launcher startup. Wrapping the deletion in a try-catch block makes log pruning best-effort and robust.
for (const item of group.items) {
try {
await safeRm(item.path, { force: true, recursive: item.kind === "directory" });
} catch {
// Ignore deletion failures to prevent crashing the launcher on non-critical cleanup
}
}There was a problem hiding this comment.
Fixed in ba2ad88: storage-quota pruning now treats stale log deletion as best-effort, so a failed delete does not block launcher startup.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the iCKB bot architecture by extracting the core bot runtime, rebalancing policy, and observability logic from apps/bot into a new private workspace package packages/bot. The apps/bot package is renamed to @ickb/bot-cli and updated to run directly from TypeScript source (Node 22.19+) as a CLI adapter. The rebalancing policy has been updated to replace the future inventory and non-standard cleanup logic with a ring bucket seeding model, excess withdrawals, and reserve recovery. Additionally, the launcher now manages log retention across 16 fixed run slots with best-effort quota pruning, and ring segment diagnostics are written as content-addressed artifacts. The changes are well-structured, successfully modularizing the bot's core logic into a reusable package while simplifying the CLI adapter to run directly from source. Since there are no review comments, no feedback on reviewer comments is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
LGTM Phroi %361 |
1 participant
Why
The bot slice now depends on the split SDK and protocol packages. Moving bot runtime policy, transaction planning, and observability into a private package keeps
apps/botas a small CLI adapter and makes the production launcher and incident tools match the source-run deployment model. This PR also carries forward the remaining lower-level package fixes from earlier slices so the bot work lands on the same package contracts as the live migration candidate.Changes
@ickb/botwith bot loop, state reads, rebalancing policy, transaction planning, observability, artifact output, and focused package tests.apps/botinto private@ickb/bot-clisource-run wiring around the bot package..mjsscripts with typedscripts/bot/**modules plus Node test coverage underscripts/test/bot/**.pnpm lintandpnpm bot:check, while preserving the existing root test path for legacy.mjsscript tests.--experimental-default-type=modulelaunch flag from bot scripts, docs, systemd units, and tests for Node 24 CI.