feat: add triage package split#225
Merged
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds a new workspace package ( ChangesTriage + FS Integration
✨ Finishing Touches🧪 Generate unit tests (beta)
|
duncanleo
force-pushed
the
codex/triage-step-4
branch
from
1b0c55e to
f31a43c
Compare
![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: 91ace8bd99
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (17)
packages/triage/src/helpers/splitIntervalByServiceHours.ts (1)
21-24: 💤 Low valueConsider extracting the service hours constant.
The service start time (05:30) at line 23 is hard-coded. If service hours might vary or need configuration, consider extracting this to a named constant at the module or package level.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/helpers/splitIntervalByServiceHours.ts` around lines 21 - 24, Extract the hard-coded service start time (05:30) into a named module-level constant (e.g., SERVICE_START_TIME or SERVICE_HOURS_START) and use that constant instead of inline literals in the DateTime.set call used when computing segmentStart (the DateTime.max(start, start.set(...)) call). Define the constant as an object or DateTime-friendly structure so it can be reused anywhere in this module (and updated centrally), and replace any other hard-coded service hour literals in this file to reference the new constant.packages/triage/src/helpers/deriveCurrentState.ts (2)
65-77: 💤 Low valueConsider clarifying the impactEventIds tracking behavior.
The current implementation tracks only the last event ID per category (serviceEffect, serviceScopes, periods, causes, facilityEffect), meaning multiple events of the same type will overwrite each other. While this appears intentional, the JSDoc at line 50 ("The impact event ids that were used to derive the current state") could be more explicit about returning the latest event ID per category rather than all event IDs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/helpers/deriveCurrentState.ts` around lines 65 - 77, Update the JSDoc for deriveCurrentState to explicitly state that impactEventIds (the impactEventIds object in deriveCurrentState) records only the most recent event ID per category (serviceEffect, serviceScopes, periods, causes, facilityEffect) rather than a list of all event IDs; mention that each property holds the latest event id used to derive state so callers understand overwrite behavior of impactEventIds.
168-171: ⚡ Quick winRemove
@ts-expect-errorto preserve exhaustiveness checking.The
@ts-expect-errordirective at line 169 suppresses TypeScript's exhaustiveness checking on the entity type. If a new entity type is added toAffectedEntityin the future, TypeScript won't flag this location. Remove the directive to restore compile-time safety — TypeScript will error if the switch becomes non-exhaustive.♻️ Proposed fix
default: { - // `@ts-expect-error` fallback case - throw new Error(`Unknown entity type: ${impactEvent.entity.type}`); + // Exhaustiveness check - TypeScript will error if cases are missing + const _exhaustive: never = impactEvent.entity; + throw new Error(`Unknown entity type: ${(_exhaustive as { type: string }).type}`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/helpers/deriveCurrentState.ts` around lines 168 - 171, Remove the suppression comment on the switch default in deriveCurrentState (the line with "// `@ts-expect-error` fallback case"); specifically delete that `@ts-expect-error` so TypeScript will enforce exhaustiveness for impactEvent.entity.type in the switch and surface compile errors if new AffectedEntity variants are added; keep the existing default behavior (throw new Error(`Unknown entity type: ${impactEvent.entity.type}`)) so the runtime fallback remains while the compiler forces you to add missing cases.packages/triage/src/util/ingestContent/index.ts (2)
106-106: ⚡ Quick winExtract hardcoded model name into a constant.
The string
'@openai/gpt-5-nano'is hardcoded in two places (lines 106 and 133). This should be extracted into a module-level or package-level constant to ensure consistency and ease future model updates.♻️ Proposed refactor
+const DEFAULT_LLM_MODEL = '@openai/gpt-5-nano'; + export async function ingestContent(content: IngestContent) { // ... const issue: Issue = { id: issueId, type: triageResult.result.issueType, title: translatedTitles, titleMeta: { - source: '@openai/gpt-5-nano', + source: DEFAULT_LLM_MODEL, }, }; // ... const evidence: Evidence = { id: IdGenerator.evidenceId(contentDateTime), ts: contentDateTime.toISO({ includeOffset: true }), type: getEvidenceType(content), text: getText(content), sourceUrl: content.url, render: { text: await translate(getText(content)), - source: '@openai/gpt-5-nano', + source: DEFAULT_LLM_MODEL, }, };Also applies to: 133-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/util/ingestContent/index.ts` at line 106, The literal model string '@openai/gpt-5-nano' is duplicated; extract it to a single module-level constant (e.g., const DEFAULT_EMBEDDING_MODEL = '@openai/gpt-5-nano') and replace both hardcoded usages with that constant (the two occurrences around lines where source is set and the second usage). If this constant should be package-wide, export it so other modules can reuse it; update any references in functions that build embeddings or set the source to use DEFAULT_EMBEDDING_MODEL to ensure consistency.
39-43: 💤 Low valueDocument or resolve the timezone HACK.
The comment indicates that forcing
createdAttoAsia/Singaporeis a HACK. If this is a temporary workaround, consider adding a TODO with context explaining when/why this should be fixed. If it's a permanent design decision (e.g., all timestamps are normalized to a canonical timezone), update the comment to reflect that and remove the HACK label.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/util/ingestContent/index.ts` around lines 39 - 43, The code forcibly sets createdAt to Asia/Singapore using DateTime.fromISO(...).setZone('Asia/Singapore') and is labeled a "HACK"; replace that ambiguous comment with an explicit decision: either add a TODO explaining this is a temporary workaround with context and a ticket/owner reference (why content.createdAt must be forced to that zone and when to remove it), or, if this is intended as the canonical timezone, update the comment to state that timestamps are normalized to 'Asia/Singapore' permanently and remove "HACK"; update the code comment near createdAt / DateTime.fromISO to reflect the chosen option and include the unique symbols (createdAt, content.createdAt, DateTime.fromISO, setZone) for traceability.packages/triage/src/scripts/reExtractAndReplay.ts (1)
28-32: 💤 Low valueExtract duplicate sign-formatting logic into a helper.
The sign-formatting pattern
(value >= 0 ? '+' : '')${value}is duplicated on lines 30 and 41. Consider extracting this into a local helper function to improve maintainability.♻️ Proposed refactor
+function formatDiff(value: number): string { + return `${value >= 0 ? '+' : ''}${value}`; +} + for (const result of summary.results) { console.log( - `${result.issueId}: ${result.before} → ${result.after} events (${(result.diff ?? 0) >= 0 ? '+' : ''}${result.diff})`, + `${result.issueId}: ${result.before} → ${result.after} events (${formatDiff(result.diff ?? 0)})`, ); } console.log( - `Total impact events: ${summary.totalBefore} → ${summary.totalAfter} (${summary.totalAfter - summary.totalBefore >= 0 ? '+' : ''}${summary.totalAfter - summary.totalBefore})`, + `Total impact events: ${summary.totalBefore} → ${summary.totalAfter} (${formatDiff(summary.totalAfter - summary.totalBefore)})`, );Also applies to: 40-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/scripts/reExtractAndReplay.ts` around lines 28 - 32, The duplicate sign-formatting logic used for result.diff should be extracted into a small helper (e.g., formatSigned or formatSignedNumber) that accepts a number and returns a string with a '+' prefix for non-negative values and the raw number otherwise; replace the inline expressions `(value >= 0 ? '+' : '')${value}` (used when printing `${result.diff ?? 0}` in the loop over summary.results and the second occurrence at lines 40-42) with calls to this helper, and ensure the helper handles null/undefined by defaulting to 0.packages/triage/src/llm/client.ts (1)
3-7: ⚡ Quick winConsider validating the API key presence.
The function does not validate that
OPENAI_API_KEYis set. While the OpenAI SDK will likely error on undefined, catching this early with a clear message improves the developer experience.🛡️ Add explicit validation
export function getOpenAiClient() { + const apiKey = process.env.OPENAI_API_KEY; + if (!apiKey) { + throw new Error('OPENAI_API_KEY environment variable is required'); + } return new OpenAI({ - apiKey: process.env.OPENAI_API_KEY, + apiKey, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/llm/client.ts` around lines 3 - 7, The getOpenAiClient function doesn't check that process.env.OPENAI_API_KEY is present before constructing a new OpenAI client; add an explicit validation at the start of getOpenAiClient to verify OPENAI_API_KEY is set and throw (or return a clear error) with a descriptive message (e.g., "OPENAI_API_KEY is not set") if missing, then proceed to instantiate OpenAI with the validated key to fail fast and provide a helpful developer message.packages/triage/src/llm/common/MemoryStore.ts (1)
105-110: 💤 Low valueMinor: Redundant directory deletion on line 109.
The
dirsMap only stores normalized paths (viatoStorePath), sothis.dirs.delete(path)on line 109 attempts to delete a potentially non-normalized key that was never added. Line 108 already removes the normalized path. This is harmless but redundant.♻️ Simplify by removing the redundant operation
delete(path: string): void { const p = this.toStorePath(path); this.files.delete(p); this.dirs.delete(p); - this.dirs.delete(path); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/llm/common/MemoryStore.ts` around lines 105 - 110, The delete method in MemoryStore unnecessarily calls this.dirs.delete(path) after already deleting the normalized key; remove the redundant deletion so only the normalized path (const p = this.toStorePath(path)) is used, i.e. keep this.files.delete(p) and this.dirs.delete(p) and remove the extra this.dirs.delete(path) in the delete(path: string) method that uses toStorePath.packages/triage/src/llm/functions/translate/index.ts (1)
11-11: ⚡ Quick winAdd an explicit return type for the translation contract.
This helper is exported; returning an explicit typed result prevents accidental contract drift and improves downstream safety.
Proposed fix
+export type TranslateResult = z.infer<typeof TranslationsSchema>; + -export async function translate(text: string) { +export async function translate(text: string): Promise<TranslateResult> { @@ - return response.output_parsed; + return response.output_parsed; }Also applies to: 69-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/llm/functions/translate/index.ts` at line 11, The exported function translate currently lacks an explicit return type; add a clear return type to its signature (e.g., Promise<string> or the actual translation result type used in this module) to lock the translation contract and prevent drift—update the signature of the exported translate function and any other exported translate declaration in this file (the other occurrence around the later definition) to return the same explicit type so callers and downstream code have a stable, typed contract.packages/fs/src/write/common/FileWriteStore.ts (1)
11-23: ⚖️ Poor tradeoffNo parent directory creation for write operations.
writeTextandappendTextwill fail if parent directories don't exist. Callers must callensureDirfirst, which is easy to forget and creates boilerplate.Consider whether
writeText,writeJson, andappendTextshould auto-create parent directories (similar tomkdir -pbehavior). This would match common expectations and reduce caller burden. However, this is a design choice that affects the IWriteStore contract, so it may be intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/fs/src/write/common/FileWriteStore.ts` around lines 11 - 23, writeText and appendText currently write directly to fullPath and will throw if parent directories don't exist; update writeText, appendText (and let writeJson continue to delegate to writeText) to ensure parent directories are created first by calling mkdirSync(dirname(fullPath), { recursive: true }) (or the existing ensureDir helper if present) before calling writeFileSync/appendFileSync so writes behave like mkdir -p and callers don't need to pre-create directories; refer to writeText, writeJson, appendText and use dirname(fullPath) to compute the parent directory.packages/fs/src/repo/common/StandardRepository.ts (1)
14-15: No mechanism to invalidate or reload cached data.The
loadedflag (line 15) prevents re-reading from disk once data is loaded. If the underlying files change during the process lifecycle, those changes won't be visible without restarting.This is acceptable for CLI tools and build scripts that load data once. For long-running services, consider adding a
reload()method or TTL-based invalidation if you need to pick up file changes without restarting the process.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/fs/src/repo/common/StandardRepository.ts` around lines 14 - 15, The repository caches data indefinitely using the protected Map byId and the boolean loaded in StandardRepository, so file changes are never picked up; add a reload mechanism (e.g., a public reload() method on StandardRepository) that clears byId, sets loaded = false, and re-invokes the existing load logic or exposes a way to force reload, or alternatively implement TTL-based invalidation (store a lastLoaded timestamp and check it in getters to refresh when expired); update methods that rely on loaded (e.g., any getAll/getById methods) to call reload/refresh when needed.packages/fs/src/write/common/store.ts (2)
1-32: 💤 Low valueIncomplete error handling specification.
Similar to
IStore, this interface doesn't document error behavior for:
- Write failures (permissions, disk full, path doesn't exist)
- Whether
writeJsonandwriteTextcreate parent directories- Whether
appendTextcreates the file if it doesn't exist- Whether
deleteshould fail silently or throw if path doesn't existAdd JSDoc comments specifying the error contract so implementations remain consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/fs/src/write/common/store.ts` around lines 1 - 32, The IWriteStore interface lacks error/behavior contracts: update the JSDoc for IWriteStore and each method (writeText, writeJson, appendText, ensureDir, delete) to state which errors are thrown (e.g., permission, ENOSPC, ENOENT), whether parent directories are created by writeText/writeJson (specify that implementations MUST create parents or MUST throw), whether appendText will create the file if missing, and the expected delete behavior (throw if path missing vs no-op). Reference the interface name IWriteStore and each method name in the comments so implementations follow the same error/creation semantics.
31-31: ⚡ Quick winOptional
deletemethod may cause inconsistent behavior.The
deletemethod is optional (delete?:), meaning some implementations may not support deletion. Callers must check for existence before calling, which is error-prone.If deletion is a core requirement, make it mandatory. If it's truly optional (e.g., for immutable stores), document when and why implementations can omit it. Otherwise, callers may assume it exists and fail at runtime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/fs/src/write/common/store.ts` at line 31, The optional delete? declaration on the store interface should be made mandatory to avoid runtime errors: change the signature from delete?(path: string): void to delete(path: string): void in the store interface (the symbol to modify is the delete method on the Store interface in store.ts) so all implementations must implement deletion; if you prefer optional behavior instead, alternatively add clear docs to the Store interface explaining when implementations may omit delete and add runtime guards where callers use store.delete to check existence before calling.packages/fs/src/repo/common/FileStore.ts (1)
12-29: 💤 Low valueConsider documenting error behavior.
The methods don't handle errors from Node.js fs operations (
readFileSync,readdirSync). Callers need to know whether to expect exceptions for:
- Missing files/directories
- Permission errors
- Invalid paths
Either document that all errors propagate as-thrown from Node.js fs APIs, or wrap them with consistent error messages including the attempted path. The current behavior is functional but may surprise callers expecting null/undefined returns for missing files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/fs/src/repo/common/FileStore.ts` around lines 12 - 29, The FileStore methods (readText, readJson, listDir, exists) currently let Node fs errors bubble up without context; update each to catch filesystem exceptions and rethrow a consistent, informative error that includes the attempted full path (join(this.rootDir, path)) and the original error as the cause or message so callers can distinguish missing files, permission issues, or malformed JSON; specifically wrap errors thrown in readText (and propagated from readJson), listDir, and exists with a descriptive message like "FileStore.<method> failed for <fullPath>" while preserving the original error object.packages/fs/src/repo/common/store.ts (1)
10-22: Consider async alternatives for production use.All methods are synchronous, which will block the Node.js event loop during I/O operations. For production services handling concurrent requests, this can significantly degrade throughput and responsiveness.
If this code will be used in request-handling paths or concurrent operations, consider defining an async variant (e.g.,
IAsyncStore) with Promise-returning methods. The current synchronous interface is acceptable for CLI tools, build scripts, and single-threaded batch processing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/fs/src/repo/common/store.ts` around lines 10 - 22, The current Store interface exposes synchronous methods (readText, readJson, listDir) which block the Node.js event loop; add an asynchronous variant (e.g., IAsyncStore) that mirrors these signatures but returns Promises (Promise<string>, Promise<T>, Promise<string[]>) and update implementations to provide both sync and async methods or migrate call sites to the async interface; ensure types and any factory/constructor (store creation code) are updated to accept/return IAsyncStore where used in request-handling or concurrent contexts so production paths use non-blocking I/O.packages/fs/src/write/issue/IssueWriter.ts (1)
43-50: ⚡ Quick winThe validation in
getIssueDir()should enforce the complete issue ID format for consistency.The current regex only validates the date prefix (
YYYY-MM-DD), leaving the rest of theissueIdunchecked. While the practical risk is limited—Node.jspath.join()normalizes path separators and..segments, and all current callers pass pre-validatedIssue.idobjects—stricter validation improves defensive programming and prevents accidental misuse.Consider updating the regex to match the full pattern defined in
IssueIdSchema:/^(\d{4})-(\d{2})-(\d{2})-[a-z0-9]+(?:-[a-z0-9]+)*$/, ensuring both date validity and slug format constraints are enforced consistently at the point of use.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/fs/src/write/issue/IssueWriter.ts` around lines 43 - 50, getIssueDir currently only validates the date prefix of issueId which allows malformed IDs; update the validation in getIssueDir to use the full IssueId pattern (e.g. /^(\d{4})-(\d{2})-(\d{2})-[a-z0-9]+(?:-[a-z0-9]+)*$/) so it matches the complete YYYY-MM-DD-slug format, throw the same error when it fails, and keep extracting year/month from the capture groups as before; modify the regex used in getIssueDir to this full pattern to enforce slug constraints consistently with IssueIdSchema.packages/fs/src/repo/issue/IssueRepository.ts (1)
199-205: 💤 Low valueOptional refactor: avoid redundant get() call.
The loop retrieves bundles by calling
this.get(result.item.issue.id), butresult.itemis already the fullIssueBundlefromthis.list(). The subsequentget()call will hit the cache but is unnecessary.♻️ Simplify by using result.item directly
- const results: IssueBundle[] = []; - for (const result of searchResults) { - const bundle = this.get(result.item.issue.id); - if (bundle == null) { - continue; - } - results.push(bundle); - } - return results; + return searchResults.map((result) => result.item);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/fs/src/repo/issue/IssueRepository.ts` around lines 199 - 205, The loop in IssueRepository.ts unnecessarily calls this.get(result.item.issue.id) even though result.item already contains the IssueBundle from list()/searchResults; replace the redundant cache lookup by pushing result.item directly into results (i.e., use result.item instead of calling get()), ensuring the pushed value matches the expected IssueBundle type and preserving any existing null checks or filtering logic around result.item.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/fs/src/repo/common/FileStore.ts`:
- Around line 17-19: The readJson<T>(path: string) method currently calls
JSON.parse(this.readText(path)) which will throw a SyntaxError without file
context; modify readJson to wrap the parse in a try/catch, call
this.readText(path) first, then try JSON.parse on that string, and on failure
throw a new Error (or rethrow a custom error) that includes the path and the
original error message/stack (e.g., "Failed to parse JSON at {path}:
{originalError.message}") so callers can see which file caused the parse
failure; reference the readJson function in FileStore.ts and reuse
readText(path) as the source string.
In `@packages/fs/src/repo/common/store.ts`:
- Around line 4-29: Update the IStore interface JSDoc: for each method
(readText, readJson, listDir, exists) add clear `@returns` descriptions (e.g.,
readText returns file contents as a UTF-8 string, listDir returns non-dot entry
names array, exists returns boolean) and a clear `@throws/`@behavior clause that
defines error semantics (specify whether implementations MUST throw on missing
paths/invalid paths, MUST throw on I/O errors/JSON parse errors, or return a
sentinel like null — pick one policy and document it consistently). For
readJson<T> explicitly document that JSON parse errors will be thrown and that
callers are responsible for runtime validation of T (or change the signature to
return T | null/unknown if you prefer safe parsing) and mention which specific
error types (e.g., SyntaxError, FileNotFound) implementations should surface;
apply these docs to IStore so all implementations follow the same error-handling
contract.
In `@packages/fs/src/write/common/FileWriteStore.ts`:
- Around line 30-33: The delete method in FileWriteStore (delete(path: string))
currently calls rmSync(fullPath) which will throw for non-empty directories or
missing paths; update the call to rmSync to pass options { recursive: true,
force: true } so non-empty directories are removed and missing files are
ignored—locate the delete method, the fullPath variable (built with
join(this.rootDir, path)), and change the rmSync invocation accordingly.
In `@packages/fs/src/write/common/StandardWriter.ts`:
- Around line 17-20: The create method in StandardWriter currently builds a file
path from item.id without validation, allowing directory traversal; update
StandardWriter.create to validate or sanitize item.id (e.g., reject or normalize
values containing path separators, '.' or '..', or enforce the same ID
validation used by writeUnknownEntity) before calling
this.store.writeJson(join(this.dirPath, `${item.id}.json`), item); if the ID is
invalid throw a clear error, or use a safe basename/sanitization routine so
writes cannot escape this.dirPath.
In `@packages/triage/README.md`:
- Around line 26-28: The README and code reference non-public model IDs
(`gpt-5.4-mini`, `gpt-5-mini`, `gpt-5-nano`) used by
extractClaimsFromNewEvidence, triageNewEvidence, and the title/slug translation
routines; search the codebase for these exact strings, confirm whether they were
placeholders or internal names, and replace them with valid public GPT‑5 IDs
(e.g., `gpt-5.4`, `gpt-5.4-pro`, `gpt-5.5`, or `gpt-5.5-pro`) according to
desired capability/latency; update the README entries to match the chosen
official model names and ensure any code that constructs OpenAI requests (the
functions extractClaimsFromNewEvidence, triageNewEvidence, and the title/slug
translation functions) uses the same validated identifiers to avoid runtime
failures.
In `@packages/triage/src/helpers/computeImpactFromEvidenceClaims.ts`:
- Around line 118-142: The status-signal reconciliation inside the 'disruption'
branch is dead/unused: it computes currentStatus (using
currentServiceState.periods and ClaimStatusSignal) and compares it to
claim.statusSignal but the inner switch on claim.statusSignal has empty cases;
either implement the reconciliation logic or remove it — simplest fix is to
delete the comparison and empty switch (the currentStatus variable and the if
(currentStatus !== claim.statusSignal) { ... } block) from the
computeImpactFromEvidenceClaims flow to eliminate dead code and keep behavior
unchanged.
In `@packages/triage/src/llm/functions/extractClaimsFromNewEvidence/index.ts`:
- Around line 126-135: The tool-call cap check currently uses ">" and then still
falls through to call tool.runner, so it does not prevent extra calls; update
the conditional around toolCallCount and TOOL_CALL_LIMIT in the block that sends
the 'function_call_output' (and the similar block later) to use ">=" (or
equivalently check toolCallCount >= TOOL_CALL_LIMIT) and ensure you
short-circuit after pushing the error output by returning/continuing (i.e., do
not invoke tool.runner) so no further tool.runner calls are made for that item
(refer to toolCallCount, TOOL_CALL_LIMIT, tool.runner, context.push and call_id
to locate both affected blocks).
In
`@packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.ts`:
- Around line 74-83: The filter for relevantStationCodes in FindStationsTool
currently parses startedAt with validation but uses membership.endedAt without
checking DateTime validity and uses an inclusive boundary; update the
membership.endedAt handling to mirror FindServicesTool by: when
membership.endedAt is not null parse it with DateTime.fromISO, assert
endedAt.isValid with a clear message, and change the exclusion check to if
(endedAt <= this.evidenceTs) return false so memberships that end at or before
evidenceTs are excluded; leave the startedAt logic as-is.
In `@packages/triage/src/llm/functions/translate/index.ts`:
- Line 12: Remove the raw input log call console.log('[translate] Translating
text:', text) so full payloads (variable text) are not written to logs; instead,
either delete the line or replace it with a safe log that records only
non-sensitive metadata (e.g., indicate the translate function ran and include
text length or a stable hash) and keep references to the translate routine and
the text variable for context.
In `@packages/triage/src/llm/functions/triageNewEvidence/index.ts`:
- Around line 114-123: The tool-call limit check (comparisons using
toolCallCount and TOOL_CALL_LIMIT) currently only pushes an error output into
context but does not stop execution of the tool runner; modify the control flow
in the triageNewEvidence function so that when toolCallCount > TOOL_CALL_LIMIT
you immediately skip/return before invoking the tool runner (the block that runs
the tool and produces further outputs), ensuring no further tool execution or
token usage; apply the same early-exit or skip logic to the other occurrence
around the second block (the section referenced in lines 129-140) so both places
prevent any tool invocation after the limit is reached.
- Around line 128-131: The code currently does raw JSON.parse(item.arguments),
tool.parseParams(...) and await tool.runner(params) which can throw and abort
the triage flow; wrap these steps in a try/catch that guards JSON.parse,
tool.parseParams, and tool.runner calls (references: item.arguments,
tool.parseParams, tool.runner, params) and on any error return a recoverable
tool-call output object (e.g., an error result/placeholder) instead of throwing
so the triage flow continues; ensure the catch logs the error/context and
produces the same shape the caller expects for a tool result.
- Around line 19-31: The discriminated union for result uses
z.discriminatedUnion('type', ...) but each branch uses the discriminator field
named kind, causing runtime mismatch; change the discriminator to 'kind' in the
z.discriminatedUnion call so it matches the branch objects (the schema under
result that references IssueIdSchema and IssueTypeSchema), then run tests and
ensure downstream consumers like ingestContent still read the kind field as
expected.
In `@packages/triage/src/scripts/ingestViaWebhook.ts`:
- Around line 5-10: The code trusts the MESSAGE env JSON and asserts a static
type for content (IngestContent[]) without runtime validation; add runtime
schema validation using Zod: define IngestContentSchema (discriminated union for
'twitter'/'mastodon', 'reddit', 'news-website') and MessageSchema in types.ts,
then parse JSON from process.env.MESSAGE and validate it against MessageSchema
before calling ingestContent; on validation failure log the validation errors
and exit (or throw) to avoid passing malformed data into ingestContent. Ensure
you reference MESSAGE, IngestContent/IngestContentSchema, MessageSchema, and
ingestContent when adding the validation and error handling.
In `@packages/triage/src/scripts/reExtractAndReplay.ts`:
- Around line 9-15: The call to reExtractAndReplay is not wrapped in error
handling; wrap the await reExtractAndReplay({...}) invocation in a try-catch
block so any thrown errors are caught, log the error (e.g., console.error or
your process logger) with context including that reExtractAndReplay failed and
the input args, and call process.exit(1) in the catch to ensure the script
terminates with a non-zero status; ensure you still return or handle the summary
on success inside the try.
In `@packages/triage/src/scripts/reExtractAndReplayTargets.ts`:
- Around line 51-67: The parser currently only checks for null when reading the
next token after '--issue' and '--evidence', allowing another flag (e.g.,
'--dry-run') to be consumed as an ID; update the validation in the argument
handling for '--issue' and '--evidence' (the branches that use argv[index + 1]
and then call issueIds.add/evidenceIds.add) to also reject tokens that look like
flags (e.g., start with '-' or '--') and throw a "Missing value for
--issue"/"Missing value for --evidence" error when the next token is absent or
is a flag, leaving index unchanged in that error path.
In `@packages/triage/src/scripts/replayImpactEvents.ts`:
- Line 6: The call to replayImpactEvents({ dataDir: DATA_DIR }) should be
wrapped in error handling: wrap the invocation (and await it if
replayImpactEvents returns a Promise) in a try-catch, assign the result to
summary inside the try block, and in the catch log the error (including
error.message/stack) and exit with a non-zero code; reference the
replayImpactEvents call, the summary variable and DATA_DIR when making these
changes.
In `@packages/triage/src/util/ingestContent/index.ts`:
- Around line 149-152: Wrap the writer operations in try/catch blocks to surface
and handle disk I/O failures: enclose writer.issues.create(issue) (around line
109) and the subsequent writer.issues.appendEvidence(issueBundle.issue.id,
evidence) and looped writer.issues.appendImpact(issueBundle.issue.id, impact)
calls in a single try so partial writes are detected; on catch, log the error
with context (including issue id) and either cleanup the partial state (call any
available writer.issues.delete(issue.id) or writer rollback API) or rethrow a
wrapped error to abort ingestion so callers can handle retries/cleanup. Ensure
the catch includes the original error details in the log/exception.
- Around line 49-71: Wrap the LLM calls (triageNewEvidence and
extractClaimsFromNewEvidence, plus any other functions that invoke LLMs in this
module) in try/catch blocks: catch errors from the LLM calls, log a clear error
message including the caught error (use existing logger/console), and return a
safe value (e.g., null) or short-circuit the ingestion flow rather than letting
the exception bubble. Ensure you apply this pattern around the existing
triageNewEvidence and extractClaimsFromNewEvidence calls and replicate it for
the other LLM-invoking calls in this file so failures are handled gracefully and
the function returns a consistent result.
---
Nitpick comments:
In `@packages/fs/src/repo/common/FileStore.ts`:
- Around line 12-29: The FileStore methods (readText, readJson, listDir, exists)
currently let Node fs errors bubble up without context; update each to catch
filesystem exceptions and rethrow a consistent, informative error that includes
the attempted full path (join(this.rootDir, path)) and the original error as the
cause or message so callers can distinguish missing files, permission issues, or
malformed JSON; specifically wrap errors thrown in readText (and propagated from
readJson), listDir, and exists with a descriptive message like
"FileStore.<method> failed for <fullPath>" while preserving the original error
object.
In `@packages/fs/src/repo/common/StandardRepository.ts`:
- Around line 14-15: The repository caches data indefinitely using the protected
Map byId and the boolean loaded in StandardRepository, so file changes are never
picked up; add a reload mechanism (e.g., a public reload() method on
StandardRepository) that clears byId, sets loaded = false, and re-invokes the
existing load logic or exposes a way to force reload, or alternatively implement
TTL-based invalidation (store a lastLoaded timestamp and check it in getters to
refresh when expired); update methods that rely on loaded (e.g., any
getAll/getById methods) to call reload/refresh when needed.
In `@packages/fs/src/repo/common/store.ts`:
- Around line 10-22: The current Store interface exposes synchronous methods
(readText, readJson, listDir) which block the Node.js event loop; add an
asynchronous variant (e.g., IAsyncStore) that mirrors these signatures but
returns Promises (Promise<string>, Promise<T>, Promise<string[]>) and update
implementations to provide both sync and async methods or migrate call sites to
the async interface; ensure types and any factory/constructor (store creation
code) are updated to accept/return IAsyncStore where used in request-handling or
concurrent contexts so production paths use non-blocking I/O.
In `@packages/fs/src/repo/issue/IssueRepository.ts`:
- Around line 199-205: The loop in IssueRepository.ts unnecessarily calls
this.get(result.item.issue.id) even though result.item already contains the
IssueBundle from list()/searchResults; replace the redundant cache lookup by
pushing result.item directly into results (i.e., use result.item instead of
calling get()), ensuring the pushed value matches the expected IssueBundle type
and preserving any existing null checks or filtering logic around result.item.
In `@packages/fs/src/write/common/FileWriteStore.ts`:
- Around line 11-23: writeText and appendText currently write directly to
fullPath and will throw if parent directories don't exist; update writeText,
appendText (and let writeJson continue to delegate to writeText) to ensure
parent directories are created first by calling mkdirSync(dirname(fullPath), {
recursive: true }) (or the existing ensureDir helper if present) before calling
writeFileSync/appendFileSync so writes behave like mkdir -p and callers don't
need to pre-create directories; refer to writeText, writeJson, appendText and
use dirname(fullPath) to compute the parent directory.
In `@packages/fs/src/write/common/store.ts`:
- Around line 1-32: The IWriteStore interface lacks error/behavior contracts:
update the JSDoc for IWriteStore and each method (writeText, writeJson,
appendText, ensureDir, delete) to state which errors are thrown (e.g.,
permission, ENOSPC, ENOENT), whether parent directories are created by
writeText/writeJson (specify that implementations MUST create parents or MUST
throw), whether appendText will create the file if missing, and the expected
delete behavior (throw if path missing vs no-op). Reference the interface name
IWriteStore and each method name in the comments so implementations follow the
same error/creation semantics.
- Line 31: The optional delete? declaration on the store interface should be
made mandatory to avoid runtime errors: change the signature from delete?(path:
string): void to delete(path: string): void in the store interface (the symbol
to modify is the delete method on the Store interface in store.ts) so all
implementations must implement deletion; if you prefer optional behavior
instead, alternatively add clear docs to the Store interface explaining when
implementations may omit delete and add runtime guards where callers use
store.delete to check existence before calling.
In `@packages/fs/src/write/issue/IssueWriter.ts`:
- Around line 43-50: getIssueDir currently only validates the date prefix of
issueId which allows malformed IDs; update the validation in getIssueDir to use
the full IssueId pattern (e.g.
/^(\d{4})-(\d{2})-(\d{2})-[a-z0-9]+(?:-[a-z0-9]+)*$/) so it matches the complete
YYYY-MM-DD-slug format, throw the same error when it fails, and keep extracting
year/month from the capture groups as before; modify the regex used in
getIssueDir to this full pattern to enforce slug constraints consistently with
IssueIdSchema.
In `@packages/triage/src/helpers/deriveCurrentState.ts`:
- Around line 65-77: Update the JSDoc for deriveCurrentState to explicitly state
that impactEventIds (the impactEventIds object in deriveCurrentState) records
only the most recent event ID per category (serviceEffect, serviceScopes,
periods, causes, facilityEffect) rather than a list of all event IDs; mention
that each property holds the latest event id used to derive state so callers
understand overwrite behavior of impactEventIds.
- Around line 168-171: Remove the suppression comment on the switch default in
deriveCurrentState (the line with "// `@ts-expect-error` fallback case");
specifically delete that `@ts-expect-error` so TypeScript will enforce
exhaustiveness for impactEvent.entity.type in the switch and surface compile
errors if new AffectedEntity variants are added; keep the existing default
behavior (throw new Error(`Unknown entity type: ${impactEvent.entity.type}`)) so
the runtime fallback remains while the compiler forces you to add missing cases.
In `@packages/triage/src/helpers/splitIntervalByServiceHours.ts`:
- Around line 21-24: Extract the hard-coded service start time (05:30) into a
named module-level constant (e.g., SERVICE_START_TIME or SERVICE_HOURS_START)
and use that constant instead of inline literals in the DateTime.set call used
when computing segmentStart (the DateTime.max(start, start.set(...)) call).
Define the constant as an object or DateTime-friendly structure so it can be
reused anywhere in this module (and updated centrally), and replace any other
hard-coded service hour literals in this file to reference the new constant.
In `@packages/triage/src/llm/client.ts`:
- Around line 3-7: The getOpenAiClient function doesn't check that
process.env.OPENAI_API_KEY is present before constructing a new OpenAI client;
add an explicit validation at the start of getOpenAiClient to verify
OPENAI_API_KEY is set and throw (or return a clear error) with a descriptive
message (e.g., "OPENAI_API_KEY is not set") if missing, then proceed to
instantiate OpenAI with the validated key to fail fast and provide a helpful
developer message.
In `@packages/triage/src/llm/common/MemoryStore.ts`:
- Around line 105-110: The delete method in MemoryStore unnecessarily calls
this.dirs.delete(path) after already deleting the normalized key; remove the
redundant deletion so only the normalized path (const p =
this.toStorePath(path)) is used, i.e. keep this.files.delete(p) and
this.dirs.delete(p) and remove the extra this.dirs.delete(path) in the
delete(path: string) method that uses toStorePath.
In `@packages/triage/src/llm/functions/translate/index.ts`:
- Line 11: The exported function translate currently lacks an explicit return
type; add a clear return type to its signature (e.g., Promise<string> or the
actual translation result type used in this module) to lock the translation
contract and prevent drift—update the signature of the exported translate
function and any other exported translate declaration in this file (the other
occurrence around the later definition) to return the same explicit type so
callers and downstream code have a stable, typed contract.
In `@packages/triage/src/scripts/reExtractAndReplay.ts`:
- Around line 28-32: The duplicate sign-formatting logic used for result.diff
should be extracted into a small helper (e.g., formatSigned or
formatSignedNumber) that accepts a number and returns a string with a '+' prefix
for non-negative values and the raw number otherwise; replace the inline
expressions `(value >= 0 ? '+' : '')${value}` (used when printing `${result.diff
?? 0}` in the loop over summary.results and the second occurrence at lines
40-42) with calls to this helper, and ensure the helper handles null/undefined
by defaulting to 0.
In `@packages/triage/src/util/ingestContent/index.ts`:
- Line 106: The literal model string '@openai/gpt-5-nano' is duplicated; extract
it to a single module-level constant (e.g., const DEFAULT_EMBEDDING_MODEL =
'@openai/gpt-5-nano') and replace both hardcoded usages with that constant (the
two occurrences around lines where source is set and the second usage). If this
constant should be package-wide, export it so other modules can reuse it; update
any references in functions that build embeddings or set the source to use
DEFAULT_EMBEDDING_MODEL to ensure consistency.
- Around line 39-43: The code forcibly sets createdAt to Asia/Singapore using
DateTime.fromISO(...).setZone('Asia/Singapore') and is labeled a "HACK"; replace
that ambiguous comment with an explicit decision: either add a TODO explaining
this is a temporary workaround with context and a ticket/owner reference (why
content.createdAt must be forced to that zone and when to remove it), or, if
this is intended as the canonical timezone, update the comment to state that
timestamps are normalized to 'Asia/Singapore' permanently and remove "HACK";
update the code comment near createdAt / DateTime.fromISO to reflect the chosen
option and include the unique symbols (createdAt, content.createdAt,
DateTime.fromISO, setZone) for traceability.
🪄 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 Plus
Run ID: a6eecfff-3854-46c1-b68a-073ab86b5f2d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (84)
.github/workflows/validate.ymlAGENTS.mdREADME.mddocs/DATA_OVERHAUL_SPLIT.mdpackage.jsonpackages/fs/package.jsonpackages/fs/src/constants.tspackages/fs/src/index.tspackages/fs/src/repo/MRTDownRepository.tspackages/fs/src/repo/common/FileStore.tspackages/fs/src/repo/common/StandardRepository.tspackages/fs/src/repo/common/dirEntries.tspackages/fs/src/repo/common/store.tspackages/fs/src/repo/issue/IssueRepository.tspackages/fs/src/repo/landmark/LandmarkRepository.tspackages/fs/src/repo/line/LineRepository.tspackages/fs/src/repo/operator/OperatorRepository.tspackages/fs/src/repo/service/ServiceRepository.tspackages/fs/src/repo/station/StationRepository.tspackages/fs/src/repo/town/TownRepository.tspackages/fs/src/write/MRTDownWriter.tspackages/fs/src/write/common/FileWriteStore.tspackages/fs/src/write/common/StandardWriter.tspackages/fs/src/write/common/store.tspackages/fs/src/write/id/IdGenerator.tspackages/fs/src/write/issue/IssueWriter.tspackages/triage/CHANGELOG.mdpackages/triage/README.mdpackages/triage/biome.jsonpackages/triage/package.jsonpackages/triage/src/helpers/calculateDurationWithinServiceHours.test.tspackages/triage/src/helpers/calculateDurationWithinServiceHours.tspackages/triage/src/helpers/computeImpactFromEvidenceClaims.test.tspackages/triage/src/helpers/computeImpactFromEvidenceClaims.tspackages/triage/src/helpers/computeStartOfDaysWithinInterval.test.tspackages/triage/src/helpers/computeStartOfDaysWithinInterval.tspackages/triage/src/helpers/deriveCurrentState.test.tspackages/triage/src/helpers/deriveCurrentState.tspackages/triage/src/helpers/estimateOpenAICost.tspackages/triage/src/helpers/keyForAffectedEntity.tspackages/triage/src/helpers/reconstructClaimsFromImpactEvents.tspackages/triage/src/helpers/splitIntervalByServiceHours.test.tspackages/triage/src/helpers/splitIntervalByServiceHours.tspackages/triage/src/helpers/sumIntervalDuration.tspackages/triage/src/index.tspackages/triage/src/llm/client.tspackages/triage/src/llm/common/MemoryStore.test.tspackages/triage/src/llm/common/MemoryStore.tspackages/triage/src/llm/common/formatCurrentState.tspackages/triage/src/llm/common/tool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/degradedServiceHeuristics.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/eval.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/index.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/prompt.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindLinesTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindServicesTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.tspackages/triage/src/llm/functions/generateIssueTitleAndSlug/index.tspackages/triage/src/llm/functions/generateIssueTitleAndSlug/prompt.tspackages/triage/src/llm/functions/translate/index.tspackages/triage/src/llm/functions/triageNewEvidence/eval.test.tspackages/triage/src/llm/functions/triageNewEvidence/index.tspackages/triage/src/llm/functions/triageNewEvidence/prompt.tspackages/triage/src/llm/functions/triageNewEvidence/tools/FindIssuesTool.tspackages/triage/src/llm/functions/triageNewEvidence/tools/GetIssueTool.tspackages/triage/src/maintenance/reExtractAndReplay.tspackages/triage/src/maintenance/replayImpactEvents.tspackages/triage/src/scripts/ingestViaWebhook.tspackages/triage/src/scripts/reExtractAndReplay.tspackages/triage/src/scripts/reExtractAndReplayTargets.test.tspackages/triage/src/scripts/reExtractAndReplayTargets.tspackages/triage/src/scripts/replayImpactEvents.tspackages/triage/src/util/assert.tspackages/triage/src/util/ingestContent/helpers/getSlugDateTimeFromClaims.tspackages/triage/src/util/ingestContent/index.tspackages/triage/src/util/ingestContent/types.tspackages/triage/tsconfig.jsonpackages/triage/vitest.config.tspackages/triage/vitest.eval.config.tsvitest.config.ts
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/triage/src/helpers/estimateOpenAICost.ts (1)
99-104: 💤 Low valueConsider documenting the clamping behavior.
The defensive clamping ensures all token counts are non-negative, which is good practice. However, if
cachedInputTokensexceedsinputTokens(invalid API data), the calculation silently producesuncachedInputTokens = 0rather than detecting the inconsistency. While this scenario is unlikely with official API responses, a brief comment explaining the clamping strategy could help future maintainers understand the trade-off.📝 Optional documentation improvement
+ // Defensive clamping: ensures non-negative values and handles edge case + // where cachedInputTokens might exceed inputTokens (degrades to 0 uncached) const cachedInputTokens = Math.max(usage.cachedInputTokens, 0); const uncachedInputTokens = Math.max( usage.inputTokens - cachedInputTokens, 0, ); const outputTokens = Math.max(usage.outputTokens, 0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/helpers/estimateOpenAICost.ts` around lines 99 - 104, Document the defensive clamping around token counts: add a brief inline comment above the calculations for cachedInputTokens, uncachedInputTokens, and outputTokens in estimateOpenAICost explaining that Math.max(...) is used to force non-negative token counts, that if usage.cachedInputTokens > usage.inputTokens we intentionally clamp uncachedInputTokens to 0 (treating the API data as inconsistent but safe) rather than throwing, and note the trade-off (silently masking API inconsistencies vs. avoiding exceptions). Reference the variables cachedInputTokens, uncachedInputTokens, usage.inputTokens and usage.cachedInputTokens in the comment so future maintainers understand the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/triage/src/helpers/computeImpactFromEvidenceClaims.ts`:
- Around line 35-52: computeImpactFromEvidenceClaims builds an empty
result.newState but then derives the current bundle state via
deriveCurrentState(params.issueBundle) — replace or initialize result.newState
with that derived snapshot (or a shallow clone) so untouched
services/facilities/provenance are preserved, then ensure you append any IDs
from newImpactEvents to result.newState.impactEventIds (or merge the emitted
event ids into the existing array) before returning; update references in
computeImpactFromEvidenceClaims to use deriveCurrentState, newImpactEvents, and
result.newState to produce a true post-reconciliation snapshot rather than an
empty state.
- Around line 61-62: The loop over claimsByAffectedEntity currently uses
claims.at(-1) and thus discards earlier non-null fields; update the logic in
computeImpactFromEvidenceClaims so that for each [key, claims] you produce a
mergedClaim by reducing the array and for each property (e.g., effect, scopes,
causes, timeHints, and other claim fields) choose the most recent
non-null/non-undefined value rather than the trailing object as a whole, then
use mergedClaim in the downstream logic instead of claim; alternatively
implement a helper like mergeClaims(claims) called in the for loop to centralize
this per-field latest-non-null merge.
- Around line 442-459: mergeFixedEndAt currently closes a period if either side
has endAt = null; change it to preserve open-endedness by returning null when
either currentEndAt or nextEndAt is null, otherwise return the later (max) end
string; for fixedPeriodsOverlap, treat null endAt as "infinite" when comparing
ranges instead of using left.startAt <= right.startAt when either end is null —
compute effective ends (e.g., leftEnd = left.endAt ?? Infinity, rightEnd =
right.endAt ?? Infinity) and test overlap using the standard interval overlap
check (left.startAt <= rightEnd && right.startAt <= leftEnd) so open-ended
periods don’t spuriously merge or close.
In `@packages/triage/src/util/ingestContent/index.ts`:
- Around line 143-151: The issue: writer.issues.create(issue) is called too
early and can leave orphaned issues if later steps
(translation/impact/append/persist) fail; move the creation into the final
persistence block (the code that appends evidence/impact and persists the
completed issue) so the issue is only persisted after all downstream steps
succeed, or implement a rollback path that deletes the issue on any downstream
exception; update all early create calls (the current
writer.issues.create(issue) call and the similar create usages around the
append/persist blocks) to either be deferred until after append/persist
completes (inside the final persistence block) or wrapped in a try/catch that on
error calls writer.issues.delete(issue.id) to undo the premature creation.
---
Nitpick comments:
In `@packages/triage/src/helpers/estimateOpenAICost.ts`:
- Around line 99-104: Document the defensive clamping around token counts: add a
brief inline comment above the calculations for cachedInputTokens,
uncachedInputTokens, and outputTokens in estimateOpenAICost explaining that
Math.max(...) is used to force non-negative token counts, that if
usage.cachedInputTokens > usage.inputTokens we intentionally clamp
uncachedInputTokens to 0 (treating the API data as inconsistent but safe) rather
than throwing, and note the trade-off (silently masking API inconsistencies vs.
avoiding exceptions). Reference the variables cachedInputTokens,
uncachedInputTokens, usage.inputTokens and usage.cachedInputTokens in the
comment so future maintainers understand the rationale.
🪄 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 Plus
Run ID: 1738505a-f7fa-4952-91a4-f3c79e61dae0
📒 Files selected for processing (21)
packages/fs/src/index.test.tspackages/fs/src/repo/common/FileStore.tspackages/fs/src/repo/common/store.tspackages/fs/src/write/common/FileWriteStore.tspackages/fs/src/write/common/StandardWriter.tspackages/triage/README.mdpackages/triage/src/helpers/computeImpactFromEvidenceClaims.tspackages/triage/src/helpers/estimateOpenAICost.tspackages/triage/src/llm/common/MemoryStore.test.tspackages/triage/src/llm/common/MemoryStore.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/index.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.tspackages/triage/src/llm/functions/translate/index.tspackages/triage/src/llm/functions/triageNewEvidence/index.tspackages/triage/src/scripts/ingestViaWebhook.tspackages/triage/src/scripts/reExtractAndReplay.tspackages/triage/src/scripts/reExtractAndReplayTargets.test.tspackages/triage/src/scripts/reExtractAndReplayTargets.tspackages/triage/src/scripts/replayImpactEvents.tspackages/triage/src/util/ingestContent/index.tspackages/triage/src/util/ingestContent/types.ts
✅ Files skipped from review due to trivial changes (1)
- packages/triage/README.md
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/fs/src/write/common/StandardWriter.ts
- packages/fs/src/repo/common/FileStore.ts
- packages/triage/src/llm/functions/translate/index.ts
- packages/triage/src/scripts/ingestViaWebhook.ts
- packages/triage/src/scripts/replayImpactEvents.ts
- packages/fs/src/repo/common/store.ts
- packages/fs/src/write/common/FileWriteStore.ts
- packages/triage/src/util/ingestContent/types.ts
- packages/triage/src/scripts/reExtractAndReplay.ts
- packages/triage/src/llm/common/MemoryStore.test.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/index.ts
- packages/triage/src/llm/common/MemoryStore.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.ts
- packages/triage/src/scripts/reExtractAndReplayTargets.test.ts
- packages/triage/src/scripts/reExtractAndReplayTargets.ts
- packages/triage/src/llm/functions/triageNewEvidence/index.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/fs/src/write/issue/IssueWriter.ts`:
- Around line 47-54: The getIssueDir currently trusts the raw issueId and can be
abused for path traversal; change getIssueDir to parse the date prefix and a
separate slug (e.g. /^(\d{4})-(\d{2})-(\d{2})(?:-(.+))?$/), validate the slug
part against a safe pattern (allow only alphanumerics, dots, underscores,
hyphens and reject any '..' or path separators), and use only the
validated/sanitized slug (or the date-only id if no slug) when calling
join(DIR_ISSUE, year, month, safeIssueId); apply the same
validation/sanitization wherever getIssueDir is used (creation/deletion
routines) so inputs like "2025-01-15-../../etc" are rejected instead of being
joined into filesystem paths.
In `@packages/triage/src/util/ingestContent/index.ts`:
- Around line 191-215: The existing-issue path lacks rollback on partial
appends: when shouldCreateIssue is false and writer.issues.appendEvidence
succeeds but writer.issues.appendImpact throws, the code leaves partial state;
update the catch to undo partial changes for the existing-issue branch by
tracking what was appended and attempting best-effort removals (e.g., call
writer.issues.removeEvidence and writer.issues.removeImpact for each appended
impact) or, better, replace the two-step logic with a single transactional/batch
API (e.g., add or call writer.issues.appendEvidenceAndImpacts) so appendEvidence
+ appendImpact are atomic; reference shouldCreateIssue, writer.issues.create,
writer.issues.appendEvidence, writer.issues.appendImpact, writer.issues.delete,
writer.issues.removeEvidence/removeImpact (or new appendEvidenceAndImpacts) and
newImpactEvents/computeImpactFromEvidenceClaims to ensure symmetry with the
new-issue rollback.
- Around line 247-260: getEvidenceType currently returns 'statement.official'
for 'reddit', which misclassifies user-generated posts; update the function so
'reddit' returns 'report.public' to match 'twitter' and 'mastodon' (function
name: getEvidenceType, symbol: content.source). If the intent is to treat only
operator-run subreddits as official, instead implement a conditional in
getEvidenceType that inspects content.subreddit against an operatorSubreddits
list (or similar upstream flag on IngestContent) and return 'statement.official'
only for those matches, otherwise return 'report.public'.
🪄 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 Plus
Run ID: 262ccfe3-71ff-44d5-8ffa-7b194fc2b9a6
📒 Files selected for processing (5)
packages/fs/src/write/issue/IssueWriter.tspackages/triage/src/helpers/computeImpactFromEvidenceClaims.test.tspackages/triage/src/helpers/computeImpactFromEvidenceClaims.tspackages/triage/src/helpers/deriveCurrentState.tspackages/triage/src/util/ingestContent/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/triage/src/helpers/deriveCurrentState.ts
- packages/triage/src/helpers/computeImpactFromEvidenceClaims.ts
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f57c71b184
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/triage/src/util/ingestContent/index.ts (4)
67-100: 💤 Low value
getText(content)is recomputed 5 times.The switch in
getTextruns again on every triage / extraction / generation / translation / evidence-build call. It's cheap, but caching once at the top of the function also gives you a single canonicaltextto log/audit and prevents accidental drift ifcontentis ever normalised differently across call sites.♻️ Sketch
+ const text = getText(content); @@ - newEvidence: { ts: content.createdAt, text: getText(content) }, + newEvidence: { ts: content.createdAt, text },(apply analogously to the four other call sites)
Also applies to: 162-177
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/util/ingestContent/index.ts` around lines 67 - 100, getText(content) is being recomputed across multiple LLM steps; compute it once and reuse to ensure a single canonical text and avoid drift. At the start of the function, assign const text = getText(content) (or similar) and replace all calls to getText(content) inside triageNewEvidence, extractClaimsFromNewEvidence and the other runLlmStep/generation/translation/evidence-build call sites with that text (use the newEvidence.text field in triageNewEvidence, extractClaimsFromNewEvidence, etc.), and update logging to reference the single text variable.
53-61: ⚡ Quick winAvoid mutating the input
contentobject.
content.createdAt = createdAtrewrites the caller's object as a side effect of normalisation. Beyond the surprise factor, it makes this function unsafe to call concurrently on a shared payload and pinsIngestContent.createdAtto a non-readonly shape. Use a local for the rest of the function.♻️ Sketch
- const createdAt = DateTime.fromISO(content.createdAt) - .setZone('Asia/Singapore') - .toISO(); - assert(createdAt != null, 'Expected valid createdAt'); - - content.createdAt = createdAt; - console.log('[ingestContent]', content); + const normalizedCreatedAt = DateTime.fromISO(content.createdAt) + .setZone('Asia/Singapore') + .toISO(); + assert(normalizedCreatedAt != null, 'Expected valid createdAt'); + + const normalizedContent: IngestContent = { + ...content, + createdAt: normalizedCreatedAt, + }; + console.log('[ingestContent]', normalizedContent);…then use
normalizedContentfor the remainder of the function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/util/ingestContent/index.ts` around lines 53 - 61, The function currently mutates the caller's input by assigning content.createdAt = createdAt; instead, create a new local object (e.g., normalizedContent) that spreads the original content and sets createdAt to DateTime.fromISO(content.createdAt).setZone('Asia/Singapore').toISO(), assert it, and use normalizedContent for the rest of the function (including logging) so the original content object is not mutated; remove the direct assignment to content.createdAt and replace subsequent references to content with normalizedContent.
20-28: 🏗️ Heavy lift
DATA_DIRis hardcoded relative to the source file's directory — brittle for both build output and reuse.
resolve(import.meta.dirname, '../../../../../data')bakes in a specific repo layout walked frompackages/triage/src/util/ingestContent/. Anything that changes the on-disk depth of this file (compiled output underdist/, repo restructure, consumption from another workspace) silently shifts the resolved path. There's also no way for callers/tests to override the location.Consider taking the data directory as a parameter (on
ingestContent, or a constructor-like factory that buildsrepo/writer) or resolving it from a config/env var, with the current path as a default. This will also makeingestContenttestable without touching the realdata/tree.♻️ Sketch
-const DATA_DIR = resolve(import.meta.dirname, '../../../../../data'); - -function createRepository(): MRTDownRepository { - return new MRTDownRepository({ store: new FileStore(DATA_DIR) }); -} - -function createWriter(): MRTDownWriter { - return new MRTDownWriter({ store: new FileWriteStore(DATA_DIR) }); -} +const DEFAULT_DATA_DIR = resolve(import.meta.dirname, '../../../../../data'); + +export interface IngestContentOptions { + dataDir?: string; +} + +function createRepository(dataDir: string): MRTDownRepository { + return new MRTDownRepository({ store: new FileStore(dataDir) }); +} + +function createWriter(dataDir: string): MRTDownWriter { + return new MRTDownWriter({ store: new FileWriteStore(dataDir) }); +} @@ -export async function ingestContent(content: IngestContent) { +export async function ingestContent( + content: IngestContent, + { dataDir = DEFAULT_DATA_DIR }: IngestContentOptions = {}, +) {…and update
issueBundle.path = dataDiraccordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/util/ingestContent/index.ts` around lines 20 - 28, Replace the hardcoded DATA_DIR by making the data directory configurable: add a dataDir parameter (with fallback to the current resolve(import.meta.dirname, '../../../../../data') default) to the factory/initializer that creates the repo/writer and to ingestContent (or provide a constructor-like function), then use that dataDir when constructing MRTDownRepository and MRTDownWriter (replace new FileStore(DATA_DIR) / new FileWriteStore(DATA_DIR) with new FileStore(dataDir) / new FileWriteStore(dataDir)) and ensure any assignment like issueBundle.path = DATA_DIR is updated to issueBundle.path = dataDir so tests and callers can override the data location (optionally support reading from an env/config var as the default).
233-267: 💤 Low valueAdd explicit return types and
never-assertion defaults togetTextandgetEvidenceType.Both switch statements lack explicit return types. Although
IngestContentis a discriminated union andstrict: trueis enabled in tsconfig, TypeScript will not catch missing cases without explicit return type annotations. Ifcontent.sourceever gains a new variant, these functions will silently returnundefined, which then flows into evidence fields and produces invalid NDJSON.Adding explicit return types (
stringforgetText,EvidenceTypeforgetEvidenceType) combined with a defaultnever-assertion case ensures compile-time safety against future union variants.♻️ Sketch
-function getText(content: IngestContent) { +function getText(content: IngestContent): string { switch (content.source) { ... + default: { + const _exhaustive: never = content; + throw new Error(`Unhandled content source: ${JSON.stringify(_exhaustive)}`); + } } } -function getEvidenceType(content: IngestContent) { +function getEvidenceType(content: IngestContent): EvidenceType { switch (content.source) { ... + default: { + const _exhaustive: never = content; + throw new Error(`Unhandled content source: ${JSON.stringify(_exhaustive)}`); + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/util/ingestContent/index.ts` around lines 233 - 267, Add explicit return type annotations to getText (string) and getEvidenceType (EvidenceType) and make their switch statements exhaustive by adding a default/never branch that performs a never-assertion (e.g., use an assertNever helper or assign content.source to a never and throw) so the compiler fails if IngestContent gains a new source; update getEvidenceType to return the concrete EvidenceType union and reference the functions getText and getEvidenceType in your changes.packages/fs/src/write/common/store.ts (1)
32-37: ⚡ Quick winConsider whether
deleteshould remain optional given rollback dependencies.
IssueWriter.delete(issueId)and the rollback path inrestoreText(path, null)both callthis.store.delete?.(...). A store that doesn't implementdeletewill silently no-op in both places, leaving partial state behind on rollback or makingIssueWriter.deletea quiet success. If no real consumer is expected to omitdelete, making it required would make the contract honest; otherwise, the call sites should fail loudly when it's missing.♻️ Option A: make required
- /** - * Delete a file or directory. - * `@param` path - * `@returns` - */ - delete?(path: string): void; + /** + * Delete a file or directory. Implementations should be idempotent. + * `@param` path + */ + delete(path: string): void;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/fs/src/write/common/store.ts` around lines 32 - 37, The interface currently makes delete optional (delete?(path: string): void) which allows this.store.delete?.(...) to silently no-op in IssueWriter.delete and restoreText rollback paths; change the contract so failures are surfaced by either making delete required (remove the ? so delete(path: string): void) and update implementations to implement it, or keep it optional but replace the safe-optional calls in IssueWriter.delete and restoreText (locations calling this.store.delete?.(...)) with explicit checks that throw a clear error when delete is undefined (e.g., if (!this.store.delete) throw new Error("store.delete not implemented"); this.store.delete(path)); reference the optional signature, IssueWriter.delete, restoreText(path, null), and the this.store.delete?.(...) call sites to locate changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/fs/src/write/issue/IssueWriter.ts`:
- Around line 83-97: readOptionalText currently swallows all exceptions from
this.store.readText and returns null, which causes restoreText to delete files
on transient/read errors; change readOptionalText to only treat a "file missing"
error as null (e.g., check the thrown error.code === 'ENOENT' or the store's
missing-signal) and rethrow any other errors so rollback won't delete a valid
file; alternatively, update the store usage to call an explicit
exists/tryReadText if available and use that to decide null vs rethrow; ensure
callers like appendEvidenceAndImpacts rely on the rethrow to fail fast when
snapshot read fails.
---
Nitpick comments:
In `@packages/fs/src/write/common/store.ts`:
- Around line 32-37: The interface currently makes delete optional
(delete?(path: string): void) which allows this.store.delete?.(...) to silently
no-op in IssueWriter.delete and restoreText rollback paths; change the contract
so failures are surfaced by either making delete required (remove the ? so
delete(path: string): void) and update implementations to implement it, or keep
it optional but replace the safe-optional calls in IssueWriter.delete and
restoreText (locations calling this.store.delete?.(...)) with explicit checks
that throw a clear error when delete is undefined (e.g., if (!this.store.delete)
throw new Error("store.delete not implemented"); this.store.delete(path));
reference the optional signature, IssueWriter.delete, restoreText(path, null),
and the this.store.delete?.(...) call sites to locate changes.
In `@packages/triage/src/util/ingestContent/index.ts`:
- Around line 67-100: getText(content) is being recomputed across multiple LLM
steps; compute it once and reuse to ensure a single canonical text and avoid
drift. At the start of the function, assign const text = getText(content) (or
similar) and replace all calls to getText(content) inside triageNewEvidence,
extractClaimsFromNewEvidence and the other
runLlmStep/generation/translation/evidence-build call sites with that text (use
the newEvidence.text field in triageNewEvidence, extractClaimsFromNewEvidence,
etc.), and update logging to reference the single text variable.
- Around line 53-61: The function currently mutates the caller's input by
assigning content.createdAt = createdAt; instead, create a new local object
(e.g., normalizedContent) that spreads the original content and sets createdAt
to DateTime.fromISO(content.createdAt).setZone('Asia/Singapore').toISO(), assert
it, and use normalizedContent for the rest of the function (including logging)
so the original content object is not mutated; remove the direct assignment to
content.createdAt and replace subsequent references to content with
normalizedContent.
- Around line 20-28: Replace the hardcoded DATA_DIR by making the data directory
configurable: add a dataDir parameter (with fallback to the current
resolve(import.meta.dirname, '../../../../../data') default) to the
factory/initializer that creates the repo/writer and to ingestContent (or
provide a constructor-like function), then use that dataDir when constructing
MRTDownRepository and MRTDownWriter (replace new FileStore(DATA_DIR) / new
FileWriteStore(DATA_DIR) with new FileStore(dataDir) / new
FileWriteStore(dataDir)) and ensure any assignment like issueBundle.path =
DATA_DIR is updated to issueBundle.path = dataDir so tests and callers can
override the data location (optionally support reading from an env/config var as
the default).
- Around line 233-267: Add explicit return type annotations to getText (string)
and getEvidenceType (EvidenceType) and make their switch statements exhaustive
by adding a default/never branch that performs a never-assertion (e.g., use an
assertNever helper or assign content.source to a never and throw) so the
compiler fails if IngestContent gains a new source; update getEvidenceType to
return the concrete EvidenceType union and reference the functions getText and
getEvidenceType in your changes.
🪄 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 Plus
Run ID: cb131a43-f0d5-4ac6-8b62-b9ec996894e1
📒 Files selected for processing (6)
packages/fs/src/index.test.tspackages/fs/src/repo/common/store.tspackages/fs/src/write/common/FileWriteStore.tspackages/fs/src/write/common/store.tspackages/fs/src/write/issue/IssueWriter.tspackages/triage/src/util/ingestContent/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/fs/src/repo/common/store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/fs/src/write/common/FileWriteStore.ts
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ead3a74909
ℹ️ 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".
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ae41ef738
ℹ️ 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 ResponseSchema = z.object({ | ||
| title: z.string(), | ||
| slug: z.string(), |
There was a problem hiding this comment.
The schema accepts any string for slug, but ingestContent later interpolates it into issueId and IssueWriter rejects slugs containing unsafe characters (for example spaces, /, or ..). In those cases issue creation throws and ingestion fails for otherwise valid evidence, so this path needs schema-level validation (or sanitization/retry) before persistence.
Useful? React with 👍 / 👎.
duncanleo
force-pushed
the
codex/triage-step-4
branch
from
6ae41ef to
84d1ac6
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/fs/src/repo/common/StandardRepository.ts (1)
11-12: ⚡ Quick winMake the base repository abstract to enforce parser implementation.
The runtime
"Not implemented"path is avoidable. Marking the class andparseItemas abstract gives compile-time enforcement.♻️ Suggested refactor
-export class StandardRepository<T extends StandardRepositoryItem> { +export abstract class StandardRepository<T extends StandardRepositoryItem> { @@ - protected parseItem(_json: unknown): T { - throw new Error('Not implemented'); - } + protected abstract parseItem(json: unknown): T;Also applies to: 50-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/fs/src/repo/common/StandardRepository.ts` around lines 11 - 12, Make StandardRepository an abstract class and declare parseItem as an abstract method to enforce compile-time implementation in subclasses instead of the runtime "Not implemented" path; update the class declaration for StandardRepository<T extends StandardRepositoryItem> to be abstract and change the parseItem signature (the method currently throwing "Not implemented") to an abstract parseItem(...) declaration so all subclasses must implement it (referencing the StandardRepository class and its parseItem method).
🤖 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 `@docs/DATA_OVERHAUL_SPLIT.md`:
- Around line 7-11: Replace the accidental ATX heading token "#177" in the
sentence that references the PR with an inline form so it won't be parsed as a
heading (MD018); update the string "#177" to an inline form such as `#177` or
"PR 177" in the DATA_OVERHAUL_SPLIT paragraph where it appears so the reference
remains plain text and markdownlint no longer flags it.
In `@packages/fs/src/repo/common/StandardRepository.ts`:
- Around line 39-41: The load logic in StandardRepository is currently
overwriting entries by calling this.byId.set(item.id, item) which hides
duplicate IDs; change the logic in the method that calls parseItem (in
StandardRepository) to check for an existing id before setting (use
this.byId.has(item.id)) and throw an error (including the duplicate id and any
contextual info like the source filename or index) when a duplicate is detected
instead of replacing the earlier entry so duplicate IDs are rejected during
repository load.
In `@packages/fs/src/write/issue/IssueWriter.ts`:
- Around line 27-30: The append methods (e.g., appendEvidence) currently call
this.store.ensureDir(getIssueDir(issueId)) which can create orphan issue
folders; instead, validate the issue exists before appending by checking for the
canonical issue file (use getIssueDir(issueId) + FILE_ISSUE_JSON or the
equivalent identifier) via this.store.exists (or similar) and throw/return an
error if it does not exist; remove the ensureDir call from appendEvidence and
the other append methods referenced (the other append blocks around lines 35–38
and 48–56) so they only append when the issue.json is present.
- Around line 19-25: The create(issue: Issue) method currently clobbers existing
data by unconditionally calling this.store.writeJson and this.store.writeText
for FILE_ISSUE, FILE_ISSUE_EVIDENCE, and FILE_ISSUE_IMPACT; change it to first
detect existing state (use getIssueDir and the store's existence checks for the
issue directory or files) and if the issue already exists either throw/return an
error or skip writing, and only create the directory and write FILE_ISSUE,
FILE_ISSUE_EVIDENCE, and FILE_ISSUE_IMPACT when they do not already exist (i.e.,
use existence checks before calling this.store.ensureDir, this.store.writeJson,
and this.store.writeText so you do not truncate append-only evidence/impact
files).
In `@packages/triage/src/llm/client.ts`:
- Around line 3-6: The getOpenAiClient function must explicitly guard against a
missing OPENAI_API_KEY: before constructing new OpenAI, check
process.env.OPENAI_API_KEY and throw a clear error (or return a failure) if it's
undefined/empty so initialization fails fast; update getOpenAiClient to validate
the env var and only call new OpenAI({ apiKey: ... }) when the key is present,
referencing getOpenAiClient and the OPENAI_API_KEY env var in the change.
In
`@packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.ts`:
- Around line 42-43: Remove or gate the raw console.log calls that print tool
I/O in FindStationsTool.ts (specifically the logs referencing params at the
earlier call and the outputs around line 132) so sensitive user/evidence text is
not persisted; update the logging in the FindStationsTool call handler (the
method that currently logs "Calling tool with parameters:" and the later output
log) to either remove these statements or wrap them behind a strict debug flag
and redact sensitive fields before logging (only log non-sensitive metadata or a
hashed/redacted summary when debug is enabled).
---
Nitpick comments:
In `@packages/fs/src/repo/common/StandardRepository.ts`:
- Around line 11-12: Make StandardRepository an abstract class and declare
parseItem as an abstract method to enforce compile-time implementation in
subclasses instead of the runtime "Not implemented" path; update the class
declaration for StandardRepository<T extends StandardRepositoryItem> to be
abstract and change the parseItem signature (the method currently throwing "Not
implemented") to an abstract parseItem(...) declaration so all subclasses must
implement it (referencing the StandardRepository class and its parseItem
method).
🪄 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 Plus
Run ID: 42570ea4-bd5a-4daa-a754-63f2c3fa49a3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (85)
.github/workflows/validate.ymlAGENTS.mdREADME.mddocs/DATA_OVERHAUL_SPLIT.mdpackage.jsonpackages/fs/package.jsonpackages/fs/src/constants.tspackages/fs/src/index.test.tspackages/fs/src/index.tspackages/fs/src/repo/MRTDownRepository.tspackages/fs/src/repo/common/FileStore.tspackages/fs/src/repo/common/StandardRepository.tspackages/fs/src/repo/common/dirEntries.tspackages/fs/src/repo/common/store.tspackages/fs/src/repo/issue/IssueRepository.tspackages/fs/src/repo/landmark/LandmarkRepository.tspackages/fs/src/repo/line/LineRepository.tspackages/fs/src/repo/operator/OperatorRepository.tspackages/fs/src/repo/service/ServiceRepository.tspackages/fs/src/repo/station/StationRepository.tspackages/fs/src/repo/town/TownRepository.tspackages/fs/src/write/MRTDownWriter.tspackages/fs/src/write/common/FileWriteStore.tspackages/fs/src/write/common/StandardWriter.tspackages/fs/src/write/common/store.tspackages/fs/src/write/id/IdGenerator.tspackages/fs/src/write/issue/IssueWriter.tspackages/triage/CHANGELOG.mdpackages/triage/README.mdpackages/triage/biome.jsonpackages/triage/package.jsonpackages/triage/src/helpers/calculateDurationWithinServiceHours.test.tspackages/triage/src/helpers/calculateDurationWithinServiceHours.tspackages/triage/src/helpers/computeImpactFromEvidenceClaims.test.tspackages/triage/src/helpers/computeImpactFromEvidenceClaims.tspackages/triage/src/helpers/computeStartOfDaysWithinInterval.test.tspackages/triage/src/helpers/computeStartOfDaysWithinInterval.tspackages/triage/src/helpers/deriveCurrentState.test.tspackages/triage/src/helpers/deriveCurrentState.tspackages/triage/src/helpers/estimateOpenAICost.tspackages/triage/src/helpers/keyForAffectedEntity.tspackages/triage/src/helpers/reconstructClaimsFromImpactEvents.tspackages/triage/src/helpers/splitIntervalByServiceHours.test.tspackages/triage/src/helpers/splitIntervalByServiceHours.tspackages/triage/src/helpers/sumIntervalDuration.tspackages/triage/src/index.tspackages/triage/src/llm/client.tspackages/triage/src/llm/common/MemoryStore.test.tspackages/triage/src/llm/common/MemoryStore.tspackages/triage/src/llm/common/formatCurrentState.tspackages/triage/src/llm/common/tool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/degradedServiceHeuristics.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/eval.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/index.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/prompt.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindLinesTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindServicesTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.tspackages/triage/src/llm/functions/generateIssueTitleAndSlug/index.tspackages/triage/src/llm/functions/generateIssueTitleAndSlug/prompt.tspackages/triage/src/llm/functions/translate/index.tspackages/triage/src/llm/functions/triageNewEvidence/eval.test.tspackages/triage/src/llm/functions/triageNewEvidence/index.tspackages/triage/src/llm/functions/triageNewEvidence/prompt.tspackages/triage/src/llm/functions/triageNewEvidence/tools/FindIssuesTool.tspackages/triage/src/llm/functions/triageNewEvidence/tools/GetIssueTool.tspackages/triage/src/maintenance/reExtractAndReplay.tspackages/triage/src/maintenance/replayImpactEvents.tspackages/triage/src/scripts/ingestViaWebhook.tspackages/triage/src/scripts/reExtractAndReplay.tspackages/triage/src/scripts/reExtractAndReplayTargets.test.tspackages/triage/src/scripts/reExtractAndReplayTargets.tspackages/triage/src/scripts/replayImpactEvents.tspackages/triage/src/util/assert.tspackages/triage/src/util/ingestContent/helpers/getSlugDateTimeFromClaims.tspackages/triage/src/util/ingestContent/index.tspackages/triage/src/util/ingestContent/types.tspackages/triage/tsconfig.jsonpackages/triage/vitest.config.tspackages/triage/vitest.eval.config.tsvitest.config.ts
✅ Files skipped from review due to trivial changes (6)
- packages/triage/vitest.config.ts
- packages/triage/README.md
- packages/triage/biome.json
- vitest.config.ts
- packages/triage/src/llm/functions/triageNewEvidence/prompt.ts
- README.md
🚧 Files skipped from review as they are similar to previous changes (66)
- packages/fs/src/repo/MRTDownRepository.ts
- packages/triage/src/helpers/sumIntervalDuration.ts
- packages/triage/src/helpers/computeStartOfDaysWithinInterval.ts
- packages/triage/vitest.eval.config.ts
- packages/fs/src/repo/town/TownRepository.ts
- packages/triage/src/helpers/keyForAffectedEntity.ts
- packages/fs/src/repo/common/dirEntries.ts
- packages/fs/src/repo/operator/OperatorRepository.ts
- packages/triage/src/llm/common/tool.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.test.ts
- packages/fs/src/repo/landmark/LandmarkRepository.ts
- packages/triage/src/index.ts
- packages/triage/src/helpers/calculateDurationWithinServiceHours.test.ts
- packages/fs/src/repo/station/StationRepository.ts
- packages/triage/package.json
- packages/fs/src/write/common/FileWriteStore.ts
- packages/triage/src/llm/functions/generateIssueTitleAndSlug/index.ts
- package.json
- packages/triage/src/scripts/ingestViaWebhook.ts
- packages/fs/src/repo/common/store.ts
- packages/triage/src/llm/functions/generateIssueTitleAndSlug/prompt.ts
- packages/fs/src/write/MRTDownWriter.ts
- packages/triage/src/llm/functions/triageNewEvidence/tools/GetIssueTool.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindLinesTool.ts
- packages/triage/src/helpers/reconstructClaimsFromImpactEvents.ts
- packages/fs/src/repo/service/ServiceRepository.ts
- packages/triage/src/helpers/deriveCurrentState.ts
- packages/triage/src/scripts/reExtractAndReplay.ts
- packages/triage/tsconfig.json
- packages/triage/src/llm/functions/triageNewEvidence/index.ts
- packages/triage/src/scripts/replayImpactEvents.ts
- packages/triage/src/util/assert.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/index.ts
- packages/fs/src/repo/common/FileStore.ts
- .github/workflows/validate.yml
- packages/fs/src/index.ts
- packages/triage/src/scripts/reExtractAndReplayTargets.test.ts
- packages/triage/src/maintenance/replayImpactEvents.ts
- packages/triage/src/llm/functions/translate/index.ts
- packages/triage/src/maintenance/reExtractAndReplay.ts
- packages/triage/src/helpers/estimateOpenAICost.ts
- packages/triage/src/util/ingestContent/types.ts
- packages/triage/src/llm/common/MemoryStore.ts
- packages/triage/src/llm/functions/triageNewEvidence/tools/FindIssuesTool.ts
- packages/triage/src/helpers/splitIntervalByServiceHours.test.ts
- packages/triage/src/llm/common/formatCurrentState.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.ts
- packages/triage/src/helpers/computeStartOfDaysWithinInterval.test.ts
- packages/triage/src/helpers/splitIntervalByServiceHours.ts
- packages/triage/src/llm/functions/triageNewEvidence/eval.test.ts
- packages/fs/src/repo/issue/IssueRepository.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/eval.test.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindServicesTool.ts
- packages/fs/src/constants.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/prompt.ts
- packages/triage/src/helpers/computeImpactFromEvidenceClaims.ts
- packages/triage/src/helpers/deriveCurrentState.test.ts
- packages/triage/src/llm/common/MemoryStore.test.ts
- packages/fs/src/write/common/StandardWriter.ts
- packages/triage/src/scripts/reExtractAndReplayTargets.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/degradedServiceHeuristics.ts
- packages/triage/src/util/ingestContent/index.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.test.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.ts
- packages/triage/src/helpers/computeImpactFromEvidenceClaims.test.ts
- packages/fs/package.json
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.ts (1)
90-92: ⚡ Quick winDeduplicate
lineIdsbefore rendering output.Repeated memberships can emit duplicate line IDs, which adds noise to tool output.
Proposed change
- const lineIds = relevantStationCodes.map( - (membership) => membership.lineId, - ); + const lineIds = Array.from( + new Set(relevantStationCodes.map((membership) => membership.lineId)), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.ts` around lines 90 - 92, The code builds lineIds from relevantStationCodes and can include duplicates; deduplicate lineIds before using them in output to avoid noise. In FindStationsTool.ts, after the mapping that creates lineIds from relevantStationCodes (the const lineIds = relevantStationCodes.map((membership) => membership.lineId)), replace or follow it with a step that removes duplicates (e.g., using a Set or Array.prototype.filter) so the rest of the function uses the unique list of line IDs when rendering or returning results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/fs/src/write/issue/IssueWriter.ts`:
- Around line 67-70: The catch block in IssueWriter that calls
restoreText(evidencePath, evidenceBefore) and restoreText(impactPath,
impactBefore) can swallow the original append error if the first rollback
throws; change it so the original error is preserved and both rollback attempts
are attempted: capture the original caught error (e.g. origErr), then call
restoreText for evidence and impact in separate try/catch blocks (or otherwise
ensure each is attempted independently), log or aggregate any rollback errors
but do not replace origErr, and finally rethrow the original error; reference
IssueWriter, restoreText, evidencePath, impactPath, evidenceBefore and
impactBefore to locate and update the catch logic.
In `@packages/triage/src/helpers/reconstructClaimsFromImpactEvents.ts`:
- Around line 55-60: The code in reconstructClaimsFromImpactEvents currently
only takes the first period from a periods.set event (periodsEvent.periods[0]
assigned to newPeriod), which drops extra periods and causes incomplete
timeHints; update the handler(s) that create timeHints (the block using
periodsEvent and variable newPeriod, and the similar logic later in the same
function) to iterate over periodsEvent.periods, map each Period to a timeHint
entry, and combine them into the claim/evidence timeHints array so all periods
are preserved when replaying evidence.
---
Nitpick comments:
In
`@packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.ts`:
- Around line 90-92: The code builds lineIds from relevantStationCodes and can
include duplicates; deduplicate lineIds before using them in output to avoid
noise. In FindStationsTool.ts, after the mapping that creates lineIds from
relevantStationCodes (the const lineIds = relevantStationCodes.map((membership)
=> membership.lineId)), replace or follow it with a step that removes duplicates
(e.g., using a Set or Array.prototype.filter) so the rest of the function uses
the unique list of line IDs when rendering or returning results.
🪄 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 Plus
Run ID: 4ef9ebaa-5e2d-49f7-abee-ccfcb5d84779
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (85)
.github/workflows/validate.ymlAGENTS.mdREADME.mddocs/DATA_OVERHAUL_SPLIT.mdpackage.jsonpackages/fs/package.jsonpackages/fs/src/constants.tspackages/fs/src/index.test.tspackages/fs/src/index.tspackages/fs/src/repo/MRTDownRepository.tspackages/fs/src/repo/common/FileStore.tspackages/fs/src/repo/common/StandardRepository.tspackages/fs/src/repo/common/dirEntries.tspackages/fs/src/repo/common/store.tspackages/fs/src/repo/issue/IssueRepository.tspackages/fs/src/repo/landmark/LandmarkRepository.tspackages/fs/src/repo/line/LineRepository.tspackages/fs/src/repo/operator/OperatorRepository.tspackages/fs/src/repo/service/ServiceRepository.tspackages/fs/src/repo/station/StationRepository.tspackages/fs/src/repo/town/TownRepository.tspackages/fs/src/write/MRTDownWriter.tspackages/fs/src/write/common/FileWriteStore.tspackages/fs/src/write/common/StandardWriter.tspackages/fs/src/write/common/store.tspackages/fs/src/write/id/IdGenerator.tspackages/fs/src/write/issue/IssueWriter.tspackages/triage/CHANGELOG.mdpackages/triage/README.mdpackages/triage/biome.jsonpackages/triage/package.jsonpackages/triage/src/helpers/calculateDurationWithinServiceHours.test.tspackages/triage/src/helpers/calculateDurationWithinServiceHours.tspackages/triage/src/helpers/computeImpactFromEvidenceClaims.test.tspackages/triage/src/helpers/computeImpactFromEvidenceClaims.tspackages/triage/src/helpers/computeStartOfDaysWithinInterval.test.tspackages/triage/src/helpers/computeStartOfDaysWithinInterval.tspackages/triage/src/helpers/deriveCurrentState.test.tspackages/triage/src/helpers/deriveCurrentState.tspackages/triage/src/helpers/estimateOpenAICost.tspackages/triage/src/helpers/keyForAffectedEntity.tspackages/triage/src/helpers/reconstructClaimsFromImpactEvents.tspackages/triage/src/helpers/splitIntervalByServiceHours.test.tspackages/triage/src/helpers/splitIntervalByServiceHours.tspackages/triage/src/helpers/sumIntervalDuration.tspackages/triage/src/index.tspackages/triage/src/llm/client.tspackages/triage/src/llm/common/MemoryStore.test.tspackages/triage/src/llm/common/MemoryStore.tspackages/triage/src/llm/common/formatCurrentState.tspackages/triage/src/llm/common/tool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/degradedServiceHeuristics.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/eval.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/index.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/prompt.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindLinesTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindServicesTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.tspackages/triage/src/llm/functions/generateIssueTitleAndSlug/index.tspackages/triage/src/llm/functions/generateIssueTitleAndSlug/prompt.tspackages/triage/src/llm/functions/translate/index.tspackages/triage/src/llm/functions/triageNewEvidence/eval.test.tspackages/triage/src/llm/functions/triageNewEvidence/index.tspackages/triage/src/llm/functions/triageNewEvidence/prompt.tspackages/triage/src/llm/functions/triageNewEvidence/tools/FindIssuesTool.tspackages/triage/src/llm/functions/triageNewEvidence/tools/GetIssueTool.tspackages/triage/src/maintenance/reExtractAndReplay.tspackages/triage/src/maintenance/replayImpactEvents.tspackages/triage/src/scripts/ingestViaWebhook.tspackages/triage/src/scripts/reExtractAndReplay.tspackages/triage/src/scripts/reExtractAndReplayTargets.test.tspackages/triage/src/scripts/reExtractAndReplayTargets.tspackages/triage/src/scripts/replayImpactEvents.tspackages/triage/src/util/assert.tspackages/triage/src/util/ingestContent/helpers/getSlugDateTimeFromClaims.tspackages/triage/src/util/ingestContent/index.tspackages/triage/src/util/ingestContent/types.tspackages/triage/tsconfig.jsonpackages/triage/vitest.config.tspackages/triage/vitest.eval.config.tsvitest.config.ts
✅ Files skipped from review due to trivial changes (4)
- packages/triage/biome.json
- packages/fs/src/repo/landmark/LandmarkRepository.ts
- README.md
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (67)
- docs/DATA_OVERHAUL_SPLIT.md
- packages/triage/vitest.config.ts
- packages/triage/vitest.eval.config.ts
- packages/triage/src/llm/functions/generateIssueTitleAndSlug/prompt.ts
- packages/triage/src/llm/functions/triageNewEvidence/prompt.ts
- .github/workflows/validate.yml
- vitest.config.ts
- packages/triage/src/llm/client.ts
- packages/triage/src/util/assert.ts
- packages/fs/src/repo/town/TownRepository.ts
- package.json
- packages/triage/src/helpers/sumIntervalDuration.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.test.ts
- packages/triage/package.json
- packages/fs/src/repo/line/LineRepository.ts
- packages/triage/src/helpers/computeStartOfDaysWithinInterval.ts
- packages/fs/src/repo/station/StationRepository.ts
- packages/triage/src/llm/functions/triageNewEvidence/eval.test.ts
- packages/fs/src/repo/MRTDownRepository.ts
- packages/triage/src/helpers/computeStartOfDaysWithinInterval.test.ts
- packages/fs/package.json
- packages/triage/src/util/ingestContent/helpers/getSlugDateTimeFromClaims.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/eval.test.ts
- packages/triage/src/llm/common/tool.ts
- packages/fs/src/constants.ts
- packages/triage/src/llm/functions/translate/index.ts
- packages/fs/src/repo/common/FileStore.ts
- packages/fs/src/write/common/FileWriteStore.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindServicesTool.ts
- packages/triage/src/helpers/splitIntervalByServiceHours.test.ts
- packages/fs/src/repo/common/StandardRepository.ts
- packages/fs/src/index.ts
- packages/fs/src/repo/operator/OperatorRepository.ts
- packages/triage/src/llm/functions/triageNewEvidence/tools/GetIssueTool.ts
- packages/triage/src/llm/common/MemoryStore.test.ts
- packages/triage/src/helpers/computeImpactFromEvidenceClaims.ts
- packages/triage/src/scripts/replayImpactEvents.ts
- packages/fs/src/write/common/store.ts
- packages/triage/src/helpers/calculateDurationWithinServiceHours.ts
- packages/triage/src/scripts/reExtractAndReplay.ts
- packages/triage/src/llm/common/formatCurrentState.ts
- packages/triage/src/index.ts
- packages/triage/src/scripts/reExtractAndReplayTargets.test.ts
- packages/fs/src/write/common/StandardWriter.ts
- packages/fs/src/repo/common/dirEntries.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/prompt.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/degradedServiceHeuristics.ts
- packages/fs/src/repo/service/ServiceRepository.ts
- packages/triage/src/llm/functions/triageNewEvidence/tools/FindIssuesTool.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.test.ts
- packages/fs/src/write/MRTDownWriter.ts
- packages/triage/src/llm/common/MemoryStore.ts
- packages/triage/src/llm/functions/triageNewEvidence/index.ts
- packages/triage/src/helpers/calculateDurationWithinServiceHours.test.ts
- packages/triage/src/maintenance/replayImpactEvents.ts
- packages/triage/src/llm/functions/generateIssueTitleAndSlug/index.ts
- packages/triage/src/helpers/deriveCurrentState.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindLinesTool.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.ts
- packages/triage/src/util/ingestContent/index.ts
- packages/triage/src/scripts/ingestViaWebhook.ts
- packages/triage/src/util/ingestContent/types.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.ts
- packages/triage/src/helpers/deriveCurrentState.test.ts
- packages/triage/src/helpers/estimateOpenAICost.ts
- packages/triage/src/scripts/reExtractAndReplayTargets.ts
- packages/fs/src/repo/issue/IssueRepository.ts
duncanleo
force-pushed
the
codex/triage-step-4
branch
from
9c7bab7 to
7595963
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/fs/src/write/id/IdGenerator.ts`:
- Around line 22-33: The evidenceId and impactEventId functions call
ulid(ts?.toMillis?.() ?? undefined) without guarding for Luxon DateTime
validity, so a DateTime with toMillis() === NaN will silently cause ulid to use
the current time; update both functions (evidenceId and impactEventId) to check
ts?.isValid === true before calling ts.toMillis(), and if ts is provided but
invalid throw a clear error (or otherwise fail fast) instead of passing NaN into
ulid, while preserving the existing behavior when ts is undefined.
In `@packages/triage/src/llm/common/MemoryStore.ts`:
- Around line 20-23: The delete() logic in MemoryStore uses toStorePath('...')
which can return '' for root, causing the descendant-cleanup check
filePath.startsWith(`${p}/`) to become startsWith('/') and silently fail; update
the MemoryStore.delete method to explicitly detect the root path (p === '') and
either disallow/root-guard by throwing or returning early before attempting
prefix cleanup, or handle the empty prefix by using a different descendant check
(e.g., match everything when p === ''), and ensure to reference toStorePath and
the delete method when making the change so the special-case for root is handled
consistently.
In
`@packages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.ts`:
- Around line 390-406: getActiveServiceStationIds currently falls back to the
latest revision when no revision matches the evidence timestamp, causing false
overlaps; remove that fallback so the function returns no stations when there is
no active revision. In practice, change the revision selection in
getActiveServiceStationIds (which iterates service.revisions and computes
startedAtMs/endedAtMs against evidenceTsMs) to not use "??
service.revisions.at(-1)" and let revision be undefined if none match, so the
final return becomes an empty array via the existing revision?.path.stations
mapping. Ensure no other code assumes a non-null revision here.
🪄 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 Plus
Run ID: a4331431-3215-44b2-9b66-865efe2121d5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (86)
.github/workflows/validate.ymlAGENTS.mdREADME.mddocs/DATA_OVERHAUL_SPLIT.mdpackage.jsonpackages/fs/package.jsonpackages/fs/src/constants.tspackages/fs/src/index.test.tspackages/fs/src/index.tspackages/fs/src/repo/MRTDownRepository.tspackages/fs/src/repo/common/FileStore.tspackages/fs/src/repo/common/StandardRepository.tspackages/fs/src/repo/common/dirEntries.tspackages/fs/src/repo/common/store.tspackages/fs/src/repo/issue/IssueRepository.tspackages/fs/src/repo/landmark/LandmarkRepository.tspackages/fs/src/repo/line/LineRepository.tspackages/fs/src/repo/operator/OperatorRepository.tspackages/fs/src/repo/service/ServiceRepository.tspackages/fs/src/repo/station/StationRepository.tspackages/fs/src/repo/town/TownRepository.tspackages/fs/src/write/MRTDownWriter.tspackages/fs/src/write/common/FileWriteStore.tspackages/fs/src/write/common/StandardWriter.tspackages/fs/src/write/common/store.tspackages/fs/src/write/id/IdGenerator.tspackages/fs/src/write/issue/IssueWriter.tspackages/triage/CHANGELOG.mdpackages/triage/README.mdpackages/triage/biome.jsonpackages/triage/package.jsonpackages/triage/src/helpers/calculateDurationWithinServiceHours.test.tspackages/triage/src/helpers/calculateDurationWithinServiceHours.tspackages/triage/src/helpers/computeImpactFromEvidenceClaims.test.tspackages/triage/src/helpers/computeImpactFromEvidenceClaims.tspackages/triage/src/helpers/computeStartOfDaysWithinInterval.test.tspackages/triage/src/helpers/computeStartOfDaysWithinInterval.tspackages/triage/src/helpers/deriveCurrentState.test.tspackages/triage/src/helpers/deriveCurrentState.tspackages/triage/src/helpers/estimateOpenAICost.tspackages/triage/src/helpers/keyForAffectedEntity.tspackages/triage/src/helpers/reconstructClaimsFromImpactEvents.test.tspackages/triage/src/helpers/reconstructClaimsFromImpactEvents.tspackages/triage/src/helpers/splitIntervalByServiceHours.test.tspackages/triage/src/helpers/splitIntervalByServiceHours.tspackages/triage/src/helpers/sumIntervalDuration.tspackages/triage/src/index.tspackages/triage/src/llm/client.tspackages/triage/src/llm/common/MemoryStore.test.tspackages/triage/src/llm/common/MemoryStore.tspackages/triage/src/llm/common/formatCurrentState.tspackages/triage/src/llm/common/tool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/degradedServiceHeuristics.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/eval.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/index.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/prompt.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindLinesTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindServicesTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.tspackages/triage/src/llm/functions/generateIssueTitleAndSlug/index.tspackages/triage/src/llm/functions/generateIssueTitleAndSlug/prompt.tspackages/triage/src/llm/functions/translate/index.tspackages/triage/src/llm/functions/triageNewEvidence/eval.test.tspackages/triage/src/llm/functions/triageNewEvidence/index.tspackages/triage/src/llm/functions/triageNewEvidence/prompt.tspackages/triage/src/llm/functions/triageNewEvidence/tools/FindIssuesTool.tspackages/triage/src/llm/functions/triageNewEvidence/tools/GetIssueTool.tspackages/triage/src/maintenance/reExtractAndReplay.tspackages/triage/src/maintenance/replayImpactEvents.tspackages/triage/src/scripts/ingestViaWebhook.tspackages/triage/src/scripts/reExtractAndReplay.tspackages/triage/src/scripts/reExtractAndReplayTargets.test.tspackages/triage/src/scripts/reExtractAndReplayTargets.tspackages/triage/src/scripts/replayImpactEvents.tspackages/triage/src/util/assert.tspackages/triage/src/util/ingestContent/helpers/getSlugDateTimeFromClaims.tspackages/triage/src/util/ingestContent/index.tspackages/triage/src/util/ingestContent/types.tspackages/triage/tsconfig.jsonpackages/triage/vitest.config.tspackages/triage/vitest.eval.config.tsvitest.config.ts
✅ Files skipped from review due to trivial changes (10)
- packages/triage/vitest.config.ts
- packages/fs/src/repo/landmark/LandmarkRepository.ts
- vitest.config.ts
- packages/triage/biome.json
- docs/DATA_OVERHAUL_SPLIT.md
- .github/workflows/validate.yml
- README.md
- packages/triage/src/helpers/reconstructClaimsFromImpactEvents.test.ts
- packages/fs/src/index.ts
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (63)
- packages/triage/src/helpers/calculateDurationWithinServiceHours.ts
- packages/triage/src/helpers/computeStartOfDaysWithinInterval.test.ts
- packages/triage/vitest.eval.config.ts
- packages/fs/src/write/common/store.ts
- packages/fs/src/repo/town/TownRepository.ts
- packages/fs/src/write/MRTDownWriter.ts
- packages/triage/src/llm/client.ts
- packages/triage/src/llm/common/tool.ts
- packages/triage/src/scripts/replayImpactEvents.ts
- packages/triage/tsconfig.json
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/prompt.ts
- packages/fs/src/repo/line/LineRepository.ts
- packages/triage/src/helpers/calculateDurationWithinServiceHours.test.ts
- packages/triage/src/helpers/sumIntervalDuration.ts
- package.json
- packages/triage/src/util/ingestContent/helpers/getSlugDateTimeFromClaims.ts
- packages/triage/src/llm/functions/triageNewEvidence/tools/GetIssueTool.ts
- packages/fs/src/repo/common/dirEntries.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.test.ts
- packages/fs/src/repo/operator/OperatorRepository.ts
- packages/fs/src/constants.ts
- packages/triage/src/llm/functions/generateIssueTitleAndSlug/prompt.ts
- packages/triage/src/llm/functions/triageNewEvidence/prompt.ts
- packages/triage/src/helpers/deriveCurrentState.ts
- packages/triage/src/helpers/computeStartOfDaysWithinInterval.ts
- packages/triage/package.json
- packages/fs/src/repo/MRTDownRepository.ts
- packages/triage/src/helpers/splitIntervalByServiceHours.ts
- packages/fs/src/write/common/StandardWriter.ts
- packages/fs/src/repo/station/StationRepository.ts
- packages/fs/src/repo/service/ServiceRepository.ts
- packages/triage/src/helpers/splitIntervalByServiceHours.test.ts
- packages/fs/src/repo/common/FileStore.ts
- packages/triage/src/llm/functions/triageNewEvidence/eval.test.ts
- packages/triage/src/scripts/reExtractAndReplayTargets.test.ts
- packages/fs/src/repo/common/StandardRepository.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindLinesTool.ts
- packages/triage/src/llm/functions/generateIssueTitleAndSlug/index.ts
- packages/triage/src/llm/functions/translate/index.ts
- packages/triage/src/maintenance/reExtractAndReplay.ts
- packages/triage/src/llm/functions/triageNewEvidence/index.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.ts
- packages/triage/src/index.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.ts
- packages/triage/src/llm/functions/triageNewEvidence/tools/FindIssuesTool.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindServicesTool.ts
- packages/triage/src/scripts/reExtractAndReplay.ts
- packages/triage/src/util/ingestContent/types.ts
- packages/fs/package.json
- packages/triage/src/helpers/computeImpactFromEvidenceClaims.ts
- packages/triage/src/llm/common/MemoryStore.test.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.test.ts
- packages/fs/src/repo/issue/IssueRepository.ts
- packages/triage/src/maintenance/replayImpactEvents.ts
- packages/fs/src/write/common/FileWriteStore.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/eval.test.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/degradedServiceHeuristics.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/index.ts
- packages/triage/src/llm/common/formatCurrentState.ts
- packages/triage/src/scripts/reExtractAndReplayTargets.ts
- packages/triage/src/helpers/deriveCurrentState.test.ts
- packages/fs/src/index.test.ts
- packages/triage/src/util/ingestContent/index.ts
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
duncanleo
force-pushed
the
codex/triage-step-4
branch
2 times, most recently
from
9033bb7 to
0870ae3
Compare
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23faca3b61
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindServicesTool.ts (1)
38-39: ⚡ Quick winAvoid logging full tool output payloads.
Logging full markdown output here is high-noise and unnecessary for normal operation. Prefer concise structured metadata (e.g.,
lineId, result count).Suggested patch
- console.log('[findServices] Calling tool with parameters:', params); + console.log('[findServices] Calling tool', { lineId: params.lineId }); @@ - console.log(`[findServices] Response output:\n${output}`); + console.log('[findServices] Response ready', { + lineId: params.lineId, + servicesMatched: Math.max(table.children.length - 1, 0), + });Also applies to: 130-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindServicesTool.ts` around lines 38 - 39, The console.log in FindServicesTool that prints the full params payload (e.g., the line with "console.log('[findServices] Calling tool with parameters:', params);" inside the FindServicesTool call method) should be replaced with a concise structured log containing only essential metadata (for example log lineId and the number of results to be returned) to avoid noisy markdown output; update both occurrences of this logging (the call-site in the tool's execution method and the second occurrence around the result handling) to emit a small object like { lineId, resultCount } or a short string message instead of the whole params or full output payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/fs/src/repo/common/FileStore.ts`:
- Around line 12-15: Add a shared path resolver used by readText, listDir, and
exists that normalizes the input path, resolves it against this.rootDir, and
verifies the resolved path starts with the rootDir (preventing ../ escapes)
before performing any fs operation; implement a private method like
resolveAndValidate(path: string): string that uses path.resolve/path.normalize
to compute fullPath, throws an error if fullPath is outside this.rootDir, then
use that method in readText (replacing join(this.rootDir, path)), listDir, and
exists so all filesystem access is guarded by the containment check.
In `@packages/fs/src/write/issue/IssueWriter.ts`:
- Around line 93-113: getIssueDir currently only regex-validates the issueId so
impossible dates (e.g., 2026-99-99) slip through; update getIssueDir to parse
the captured year, month, and day (from tsMatch) into a Date object (or compute
normalized year/month/day) and verify the Date components match the original
year, month, and day before constructing safeIssueId and returning
join(DIR_ISSUE,...); if the date normalization does not match, throw the same
Invalid issue ID error. Ensure this check runs after extracting year/month/day
and before creating safeIssueId so functions/methods like getIssueDir, tsMatch,
safeIssueId and constant DIR_ISSUE are used to locate and fix the code.
In
`@packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindServicesTool.ts`:
- Around line 18-20: The tool's description string is incorrect: update the
public description on FindServicesTool (public description = ...) to reflect
that it searches by lineId rather than by name (e.g., "Find services by lineId"
or "Find services by lineId (lookup by lineId)"); ensure the new text matches
the actual lookup behavior used elsewhere in the class (lookups referencing
lineId) and keep the public name 'findServices' unchanged.
---
Nitpick comments:
In
`@packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindServicesTool.ts`:
- Around line 38-39: The console.log in FindServicesTool that prints the full
params payload (e.g., the line with "console.log('[findServices] Calling tool
with parameters:', params);" inside the FindServicesTool call method) should be
replaced with a concise structured log containing only essential metadata (for
example log lineId and the number of results to be returned) to avoid noisy
markdown output; update both occurrences of this logging (the call-site in the
tool's execution method and the second occurrence around the result handling) to
emit a small object like { lineId, resultCount } or a short string message
instead of the whole params or full output payload.
🪄 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 Plus
Run ID: f4ecdbab-935a-425d-97f7-110264c0b05e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (89)
.github/workflows/validate.ymlAGENTS.mdREADME.mddocs/DATA_OVERHAUL_SPLIT.mdpackage.jsonpackages/fs/package.jsonpackages/fs/src/constants.tspackages/fs/src/index.test.tspackages/fs/src/index.tspackages/fs/src/repo/MRTDownRepository.tspackages/fs/src/repo/common/FileStore.tspackages/fs/src/repo/common/StandardRepository.tspackages/fs/src/repo/common/dirEntries.tspackages/fs/src/repo/common/store.tspackages/fs/src/repo/issue/IssueRepository.tspackages/fs/src/repo/landmark/LandmarkRepository.tspackages/fs/src/repo/line/LineRepository.tspackages/fs/src/repo/operator/OperatorRepository.tspackages/fs/src/repo/service/ServiceRepository.tspackages/fs/src/repo/station/StationRepository.tspackages/fs/src/repo/town/TownRepository.tspackages/fs/src/write/MRTDownWriter.tspackages/fs/src/write/common/FileWriteStore.tspackages/fs/src/write/common/StandardWriter.tspackages/fs/src/write/common/store.tspackages/fs/src/write/id/IdGenerator.tspackages/fs/src/write/issue/IssueWriter.tspackages/triage/CHANGELOG.mdpackages/triage/README.mdpackages/triage/biome.jsonpackages/triage/package.jsonpackages/triage/src/helpers/calculateDurationWithinServiceHours.test.tspackages/triage/src/helpers/calculateDurationWithinServiceHours.tspackages/triage/src/helpers/computeImpactFromEvidenceClaims.test.tspackages/triage/src/helpers/computeImpactFromEvidenceClaims.tspackages/triage/src/helpers/computeStartOfDaysWithinInterval.test.tspackages/triage/src/helpers/computeStartOfDaysWithinInterval.tspackages/triage/src/helpers/deriveCurrentState.test.tspackages/triage/src/helpers/deriveCurrentState.tspackages/triage/src/helpers/estimateOpenAICost.tspackages/triage/src/helpers/keyForAffectedEntity.tspackages/triage/src/helpers/reconstructClaimsFromImpactEvents.test.tspackages/triage/src/helpers/reconstructClaimsFromImpactEvents.tspackages/triage/src/helpers/splitIntervalByServiceHours.test.tspackages/triage/src/helpers/splitIntervalByServiceHours.tspackages/triage/src/helpers/sumIntervalDuration.tspackages/triage/src/index.tspackages/triage/src/llm/client.tspackages/triage/src/llm/common/MemoryStore.test.tspackages/triage/src/llm/common/MemoryStore.tspackages/triage/src/llm/common/formatCurrentState.tspackages/triage/src/llm/common/tool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/degradedServiceHeuristics.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/eval.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/index.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/prompt.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindLinesTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindServicesTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.test.tspackages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.tspackages/triage/src/llm/functions/generateIssueTitleAndSlug/index.test.tspackages/triage/src/llm/functions/generateIssueTitleAndSlug/index.tspackages/triage/src/llm/functions/generateIssueTitleAndSlug/prompt.tspackages/triage/src/llm/functions/translate/index.tspackages/triage/src/llm/functions/triageNewEvidence/eval.test.tspackages/triage/src/llm/functions/triageNewEvidence/index.tspackages/triage/src/llm/functions/triageNewEvidence/prompt.tspackages/triage/src/llm/functions/triageNewEvidence/tools/FindIssuesTool.tspackages/triage/src/llm/functions/triageNewEvidence/tools/GetIssueTool.tspackages/triage/src/maintenance/evidenceSort.test.tspackages/triage/src/maintenance/evidenceSort.tspackages/triage/src/maintenance/reExtractAndReplay.tspackages/triage/src/maintenance/replayImpactEvents.tspackages/triage/src/scripts/ingestViaWebhook.tspackages/triage/src/scripts/reExtractAndReplay.tspackages/triage/src/scripts/reExtractAndReplayTargets.test.tspackages/triage/src/scripts/reExtractAndReplayTargets.tspackages/triage/src/scripts/replayImpactEvents.tspackages/triage/src/util/assert.tspackages/triage/src/util/ingestContent/helpers/getSlugDateTimeFromClaims.tspackages/triage/src/util/ingestContent/index.tspackages/triage/src/util/ingestContent/types.tspackages/triage/tsconfig.jsonpackages/triage/vitest.config.tspackages/triage/vitest.eval.config.tsvitest.config.ts
✅ Files skipped from review due to trivial changes (11)
- packages/triage/biome.json
- vitest.config.ts
- packages/triage/src/maintenance/evidenceSort.test.ts
- packages/triage/src/scripts/ingestViaWebhook.ts
- packages/triage/README.md
- packages/triage/CHANGELOG.md
- README.md
- docs/DATA_OVERHAUL_SPLIT.md
- AGENTS.md
- packages/fs/src/index.ts
- packages/triage/src/helpers/computeStartOfDaysWithinInterval.test.ts
🚧 Files skipped from review as they are similar to previous changes (64)
- packages/fs/src/repo/common/dirEntries.ts
- packages/triage/vitest.eval.config.ts
- packages/fs/src/repo/common/store.ts
- packages/fs/src/repo/MRTDownRepository.ts
- packages/fs/src/constants.ts
- packages/fs/src/repo/service/ServiceRepository.ts
- packages/triage/src/util/assert.ts
- packages/fs/src/repo/landmark/LandmarkRepository.ts
- packages/triage/src/helpers/computeStartOfDaysWithinInterval.ts
- packages/fs/src/repo/operator/OperatorRepository.ts
- .github/workflows/validate.yml
- packages/triage/src/helpers/calculateDurationWithinServiceHours.test.ts
- packages/triage/src/llm/functions/triageNewEvidence/tools/FindIssuesTool.ts
- packages/triage/src/helpers/reconstructClaimsFromImpactEvents.test.ts
- packages/triage/src/llm/functions/triageNewEvidence/tools/GetIssueTool.ts
- packages/triage/src/helpers/sumIntervalDuration.ts
- packages/fs/src/repo/station/StationRepository.ts
- packages/triage/vitest.config.ts
- packages/triage/src/llm/functions/generateIssueTitleAndSlug/prompt.ts
- packages/triage/src/index.ts
- packages/triage/src/scripts/reExtractAndReplayTargets.test.ts
- packages/triage/tsconfig.json
- package.json
- packages/triage/src/helpers/keyForAffectedEntity.ts
- packages/triage/src/scripts/replayImpactEvents.ts
- packages/fs/src/repo/town/TownRepository.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/prompt.ts
- packages/triage/src/llm/common/tool.ts
- packages/triage/package.json
- packages/triage/src/helpers/splitIntervalByServiceHours.test.ts
- packages/fs/src/write/common/StandardWriter.ts
- packages/triage/src/util/ingestContent/helpers/getSlugDateTimeFromClaims.ts
- packages/fs/src/write/common/FileWriteStore.ts
- packages/triage/src/helpers/splitIntervalByServiceHours.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/degradedServiceHeuristics.ts
- packages/triage/src/helpers/estimateOpenAICost.ts
- packages/fs/src/write/MRTDownWriter.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindLinesTool.ts
- packages/triage/src/llm/functions/triageNewEvidence/prompt.ts
- packages/fs/src/repo/common/StandardRepository.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/FindStationsTool.ts
- packages/triage/src/helpers/calculateDurationWithinServiceHours.ts
- packages/fs/src/repo/issue/IssueRepository.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/eval.test.ts
- packages/fs/src/write/common/store.ts
- packages/triage/src/util/ingestContent/types.ts
- packages/triage/src/scripts/reExtractAndReplayTargets.ts
- packages/triage/src/llm/functions/triageNewEvidence/eval.test.ts
- packages/triage/src/helpers/deriveCurrentState.ts
- packages/triage/src/maintenance/reExtractAndReplay.ts
- packages/triage/src/helpers/computeImpactFromEvidenceClaims.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/tools/ResolveRelativeDateTool.ts
- packages/triage/src/llm/common/MemoryStore.ts
- packages/triage/src/llm/common/formatCurrentState.ts
- packages/triage/src/llm/client.ts
- packages/triage/src/llm/functions/translate/index.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/normalizeClaimsForEvidence.ts
- packages/triage/src/llm/common/MemoryStore.test.ts
- packages/triage/src/llm/functions/extractClaimsFromNewEvidence/index.ts
- packages/triage/src/scripts/reExtractAndReplay.ts
- packages/triage/src/helpers/deriveCurrentState.test.ts
- packages/triage/src/llm/functions/triageNewEvidence/index.ts
- packages/triage/src/helpers/computeImpactFromEvidenceClaims.test.ts
- packages/triage/src/util/ingestContent/index.ts
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d18fb69b4
ℹ️ 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".
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a85bf8f54a
ℹ️ 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".
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb1d4fc1a7
ℹ️ 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".
duncanleo
changed the title
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa3c159f33
ℹ️ 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".
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the step-4
@mrtdown/triagepackage as an extracted slice from the large data-overhaul source PR (#177), with only the adaptation needed for this stacked branch.This PR includes:
packages/triagepackage layout, pure helpers, LLM functions, maintenance/replay scripts, ingestion utilities, and deterministic teststest:eval, with package docs for required environment variables, model dependency, and expected costeval.test.tsbuild:triage,test:triage, andtest:eval@mrtdown/fsrepository/writer support (MRTDownRepository,MRTDownWriter, file stores, standard repositories/writers, andIdGenerator) that the extracted triage package depends onThe
@mrtdown/fssupport technically belongs to the earlier file-backed repository/CLI slice, but it was present in the large source PR and is required by the extracted triage implementation. Keeping it as separate commits in this PR makes the backfill visible while avoiding another hand-written divergence from the source branch.This PR intentionally does not include static canonical data migration, issue dataset migration, runtime removal, deploy cleanup, Pages generation, or package publishing.
Rebase Note
This branch has been rebased onto the updated
mainafter the split-plan rework landed there. During the rebase, the package dependency/script conflicts were resolved by keepingmain's Pages/build surfaces and this branch's triage package commands.Validation
npm run checknpm run build:packagesnpm run test:packagesbuildvalidatenpm run test:evalwas not run because it is model-backed and paid.Summary by CodeRabbit
New Features
Documentation
Tests
Chores