feat: add core_failure telemetry with PII-safe input signatures#245
Merged
suryaiyer95 merged 14 commits intomainfrom Mar 19, 2026
Merged
feat: add core_failure telemetry with PII-safe input signatures#245suryaiyer95 merged 14 commits intomainfrom
suryaiyer95 merged 14 commits intomainfrom
Conversation
Claude Code ReviewThis repository is configured for manual code reviews. Comment |
There was a problem hiding this comment.
Pull request overview
This PR expands Opencode’s telemetry by adding a new core_failure event (with privacy-preserving input signatures and coarse error classification) and emits additional telemetry from the tool execution wrapper, while updating tests and docs to reflect the updated event surface.
Changes:
- Add
core_failuretoTelemetry.Event, plusclassifyError()andcomputeInputSignature()helpers. - Emit
tool_calltelemetry (success/error) fromTool.define()and emitcore_failuretelemetry on thrown errors. - Update telemetry tests for the expanded event type set and add unit tests for the new helpers and privacy guarantees.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/opencode/test/telemetry/telemetry.test.ts | Updates event type lists and adds tests for classifyError, computeInputSignature, and core_failure privacy constraints. |
| packages/opencode/src/tool/tool.ts | Emits tool_call telemetry for tool execution and emits core_failure on exceptions, including truncated error text and input signatures. |
| packages/opencode/src/altimate/telemetry/index.ts | Adds the core_failure event type and implements error classification + input signature computation. |
| docs/docs/configure/telemetry.md | Updates telemetry documentation and adds a new “Core Engine Telemetry” section (currently inconsistent with the implemented events/fields). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
suryaiyer95
force-pushed
the
feat/core-failure-telemetry
branch
3 times, most recently
from
1e86cf0 to
a16fb16
Compare
suryaiyer95
force-pushed
the
feat/core-failure-telemetry
branch
2 times, most recently
from
8099906 to
687494e
Compare
Comment on lines
+358
to
+368
| }> = [ | ||
| { class: "parse_error", keywords: ["parse", "syntax", "binder", "unexpected token", "sqlglot"] }, | ||
| { | ||
| class: "connection", | ||
| keywords: ["econnrefused", "connection", "socket", "enotfound", "econnreset"], | ||
| }, | ||
| { class: "timeout", keywords: ["timeout", "etimedout", "bridge timeout", "timed out"] }, | ||
| { class: "validation", keywords: ["invalid params", "invalid", "missing", "required"] }, | ||
| { class: "permission", keywords: ["permission", "denied", "unauthorized", "forbidden"] }, | ||
| { class: "internal", keywords: ["internal", "assertion"] }, | ||
| ] |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add a new `core_failure` event emitted on both soft failures (`metadata.success === false`) and uncaught tool exceptions, with privacy-preserving context for debugging: - `classifyError()` — keyword-based error classification (parse, connection, timeout, validation, permission, internal, unknown) - `computeInputSignature()` — records key names + value types/lengths, never actual values; truncates by dropping keys to preserve valid JSON - `maskArgs()` — PII masking aligned to Rust SDK: 19 sensitive keys redacted, string literals in SQL replaced with `?`, recursive object traversal Telemetry is fully isolated from tool execution — all tracking calls are wrapped in `try/catch` so telemetry failures never break tools. `Truncate.output()` runs outside the telemetry error boundary so I/O errors aren't misattributed as tool failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
suryaiyer95
force-pushed
the
feat/core-failure-telemetry
branch
from
687494e to
38fb986
Compare
Tracks which skill is loaded and where it came from (`builtin`, `global`, or `project`) with duration. Wrapped in try/catch — cannot break skill loading. Docs table updated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
suryaiyer95
force-pushed
the
feat/core-failure-telemetry
branch
from
38fb986 to
2e73f2e
Compare
\`core_failure\` is for internal tool failures. SQL execution via the dispatcher is a separate concern — soft errors are returned as results (not thrown), so \`core_failure\` never fires for them. New \`sql_execute_failure\` event captures: warehouse type, query type, error message (truncated to 500 chars), and PII-masked SQL. Fires from the \`sql.execute\` handler catch path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
suryaiyer95
force-pushed
the
feat/core-failure-telemetry
branch
from
4e47382 to
ef1d1a7
Compare
Generated once as a random UUID and stored at \`~/.altimate/machine-id\` (alongside \`altimate.json\`, \`connections.json\`, etc.). Sent as \`machine_id\` in \`customDimensions\` on every App Insights event. No PII — pure random UUID, never tied to user identity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
suryaiyer95
force-pushed
the
feat/core-failure-telemetry
branch
from
fbc2375 to
625f3f4
Compare
| warehouse_type: warehouseType, | ||
| query_type: detectQueryType(params.sql), | ||
| error_message: errorMsg.slice(0, 500), | ||
| masked_sql: Telemetry.maskArgs({ sql: params.sql }), |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
Author
Code reviewFound 4 issues:
altimate-code/packages/opencode/src/altimate/native/connections/register.ts Lines 251 to 256 in 625f3f4
altimate-code/packages/opencode/src/altimate/telemetry/index.ts Lines 381 to 400 in 625f3f4
altimate-code/packages/opencode/src/tool/tool.ts Lines 85 to 100 in 625f3f4
altimate-code/packages/opencode/src/altimate/telemetry/index.ts Lines 290 to 305 in 625f3f4 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…metry
- `sql_execute_failure`: use `Telemetry.maskString(params.sql)` instead of
`Telemetry.maskArgs({ sql: params.sql })` — the latter serializes a JSON
object string `{"sql":"..."}` rather than the raw masked SQL
- `ERROR_PATTERNS`: move `permission` before `validation` so errors like
"Invalid permission denied" are not misclassified as `validation_error`
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Read/write/edit/glob/grep/bash succeed constantly in normal operation — tracking every success is high-volume noise with no actionable signal. Failures (hard throws and soft failures) are still fully captured via \`tool_call\` (status=error) and \`core_failure\`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Error messages from SQL engines can embed data values (e.g. "Value 'john@email.com' does not match type INTEGER"). Apply maskString() to all error_message fields before transmission, consistent with how args are already masked. Affects: core_failure (tool.ts), sql_execute_failure (register.ts) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds telemetry event types and privacy utilities, instruments tool/skill execution and SQL error paths with masked reporting, persists a machine ID to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as Tool/Skill Runner
participant Telemetry as Telemetry
participant FS as Filesystem (~/.altimate)
participant DB as Database / SQL Driver
participant App as AppInsights
Runner->>Telemetry: init -> read/write machine-id (UUID)
Runner->>Telemetry: start telemetry (timestamp, session_id)
Runner->>DB: execute query or invoke tool/skill
DB-->>Runner: result or error
alt success
Runner->>Telemetry: compute input_signature, maskArgs -> track tool_call (success)
Telemetry->>App: send envelope (includes machine_id)
else error or soft-failure
Runner->>Telemetry: classifyError, maskString(masked_sql) -> track core_failure / sql_execute_failure / tool_call(error)
Telemetry->>App: send envelope (masked fields)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opencode/src/altimate/telemetry/index.ts (1)
444-449:isSensitiveKey()over-masksauth_method(and similar enum-valued fields).The prefix check
lower.startsWith(${k}_)matches keys likeauth_methodagainst theauthsensitive key. However,auth_methodtypically holds non-sensitive enum values (e.g.,"keypair","password","oauth") that are useful for debugging warehouse connection issues.Consider explicitly excluding known safe keys:
🛠️ Suggested fix
+const SAFE_KEYS = new Set(["auth_method", "auth_type"]) + function isSensitiveKey(key: string): boolean { const lower = key.toLowerCase() + if (SAFE_KEYS.has(lower)) return false return SENSITIVE_KEYS.some( (k) => lower === k || lower.endsWith(`_${k}`) || lower.startsWith(`${k}_`), ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/telemetry/index.ts` around lines 444 - 449, The isSensitiveKey function currently over-masks keys like auth_method because lower.startsWith(`${k}_`) matches "auth_" prefixes; update isSensitiveKey to explicitly whitelist known safe enum-like fields (e.g., "auth_method", maybe "auth_type", or other domain-specific non-sensitive keys) before applying the generic SENSITIVE_KEYS checks so those keys are not treated as sensitive. Locate and modify the isSensitiveKey function and the SENSITIVE_KEYS usage to first check a SAFE_KEYS set (or explicit exclusions) and return false for those names, then fall back to the existing equality/endsWith/startsWith logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/tool/tool.ts`:
- Around line 137-155: The soft-failure telemetry currently falls back to
result.output which may contain unmasked PII; update the error selection in the
isSoftFailure block (where result.metadata?.error, result.output,
Telemetry.maskString and Telemetry.track are used) so it does NOT directly use
result.output — either (A) only use result.metadata.error when it's a string and
otherwise use a safe constant like "unknown error", or (B) if you must include
output context, pass result.output through a stronger sanitizer before masking
(implement or call a Telemetry.sanitizePII/Telemetry.redactPII helper that
strips emails/IPs/IDs, then call Telemetry.maskString on the sanitized text) and
use that sanitized-and-masked value as error_message in Telemetry.track.
---
Nitpick comments:
In `@packages/opencode/src/altimate/telemetry/index.ts`:
- Around line 444-449: The isSensitiveKey function currently over-masks keys
like auth_method because lower.startsWith(`${k}_`) matches "auth_" prefixes;
update isSensitiveKey to explicitly whitelist known safe enum-like fields (e.g.,
"auth_method", maybe "auth_type", or other domain-specific non-sensitive keys)
before applying the generic SENSITIVE_KEYS checks so those keys are not treated
as sensitive. Locate and modify the isSensitiveKey function and the
SENSITIVE_KEYS usage to first check a SAFE_KEYS set (or explicit exclusions) and
return false for those names, then fall back to the existing
equality/endsWith/startsWith logic.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ed820b8c-eb56-4650-acd6-3cb8af05b32e
📒 Files selected for processing (7)
docs/docs/configure/telemetry.mdpackages/drivers/src/sqlserver.tspackages/opencode/src/altimate/native/connections/register.tspackages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/tool/skill.tspackages/opencode/src/tool/tool.tspackages/opencode/test/telemetry/telemetry.test.ts
💤 Files with no reviewable changes (1)
- packages/drivers/src/sqlserver.ts
- Mask error messages in `native_call` (dispatcher.ts) and `warehouse_connect` (registry.ts) — these were sending raw error strings that could embed credentials or query fragments - Fix soft-failure `error_message` fallback: drop `result.output` as a source (raw tool output could contain file contents or secrets); fall back to `"unknown error"` instead - Strip `_retried` internal flag from App Insights payload — was leaking into `properties` on retried events - Add camelCase variants to `SENSITIVE_KEYS` (`authToken`, `bearerToken`, `jwtSecret`, etc.) — underscore prefix/suffix matching missed these - Document `machine_id` in telemetry privacy docs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/altimate/telemetry/index.ts`:
- Around line 447-452: The isSensitiveKey function is over-masking keys like
"auth_method" because the startsWith(`${k}_`) rule matches "auth_..." entries;
update isSensitiveKey (and its use of SENSITIVE_KEYS and the lower variable) to
explicitly exclude "auth_method" (or check for exact "auth_method" and return
false) before applying the generic startsWith/endsWith/equality rules so that
non-sensitive enum values (e.g., "keypair", "password", "sso") are not redacted
in telemetry.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5588fd2e-fa95-4ce9-9ddf-460f65b22665
📒 Files selected for processing (5)
docs/docs/configure/telemetry.mdpackages/opencode/src/altimate/native/connections/registry.tspackages/opencode/src/altimate/native/dispatcher.tspackages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/tool/tool.ts
…ence/telemetry.md Main renamed the file; keep `native_call` (replaces deprecated `bridge_call`) and correct contributor file path (`packages/opencode/src/altimate/telemetry/index.ts`). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs/reference/telemetry.md (1)
81-81:⚠️ Potential issue | 🟡 MinorFix contradiction: masked SQL is collected, but raw SQL is not.
Line 81 states that "SQL queries" are never collected, but line 37 documents that
sql_execute_failureevents include "PII-masked SQL." This is contradictory.According to the PR objectives, the privacy claim is "no raw SQL" (not "no SQL at all"). Masked SQL with literals replaced by
?is intentionally collected for diagnostics.Update line 81 to clarify:
-- SQL queries or query results +- Raw SQL queries or query results (masked SQL with literals replaced by `?` may be included in error events)This maintains transparency about what is and isn't collected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/reference/telemetry.md` at line 81, Update the sentence that currently reads "SQL queries or query results" to clarify that raw SQL is not collected but PII-masked SQL (with literals replaced by ?), such as the SQL included in sql_execute_failure events, is collected for diagnostics; explicitly state "raw/unmasked SQL is never collected; PII-masked SQL (literals replaced by '?') may be collected (e.g. in sql_execute_failure events)" so the doc aligns with the recorded event behavior.
🧹 Nitpick comments (1)
docs/docs/reference/telemetry.md (1)
15-15: Consider documentinginput_signatureontool_callfailures.According to the PR objectives,
tool_callevents now include aninput_signaturefield when failures occur. This is a key privacy-enhancing feature that records only key names, value types, and lengths—not raw values. Consider updating the description to mention this field is added on failures, e.g.:| `tool_call` | A tool is invoked (tool name and category — no arguments or output; on failures, includes a PII-safe `input_signature`) |This helps users understand the privacy guarantees around failure diagnostics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/reference/telemetry.md` at line 15, Update the `tool_call` table entry to mention the new `input_signature` field added on failures: change the description for `tool_call` to note that when a tool invocation fails it includes a PII-safe `input_signature` (records only key names, value types, and lengths — not raw values) so readers know failures include this privacy-preserving diagnostic info; reference `tool_call` and `input_signature` in the updated sentence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/reference/telemetry.md`:
- Line 38: Update the `core_failure` event description to match the actual event
fields: replace "function name" with "tool name" and include "tool_category",
"error_class", a truncated/shortened "error_message", and the required
privacy-safe "input_signature" field; also note that arguments may be included
but must be masked (no raw values or credentials). Ensure the wording mirrors
the suggested phrasing so the description lists tool_name/category, error_class,
truncated error_message, PII-safe input_signature, and optionally masked
arguments.
---
Outside diff comments:
In `@docs/docs/reference/telemetry.md`:
- Line 81: Update the sentence that currently reads "SQL queries or query
results" to clarify that raw SQL is not collected but PII-masked SQL (with
literals replaced by ?), such as the SQL included in sql_execute_failure events,
is collected for diagnostics; explicitly state "raw/unmasked SQL is never
collected; PII-masked SQL (literals replaced by '?') may be collected (e.g. in
sql_execute_failure events)" so the doc aligns with the recorded event behavior.
---
Nitpick comments:
In `@docs/docs/reference/telemetry.md`:
- Line 15: Update the `tool_call` table entry to mention the new
`input_signature` field added on failures: change the description for
`tool_call` to note that when a tool invocation fails it includes a PII-safe
`input_signature` (records only key names, value types, and lengths — not raw
values) so readers know failures include this privacy-preserving diagnostic
info; reference `tool_call` and `input_signature` in the updated sentence.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d5b50d4-19cb-43fe-8b7d-c36efed548ff
📒 Files selected for processing (1)
docs/docs/reference/telemetry.md
mdesmet
approved these changes
Mar 19, 2026
Collaborator
Multi-Model Code Review (7 AI models)Verdict: APPROVE with 3 required fixes before merge Reviewed by Claude, Codex (GPT-5.4), Grok, Kimi, MiniMax, GLM-5, Qwen. 1 convergence round. MAJOR — Fix Before Merge1.
return s
.replace(/'(?:[^'\\]|\\.)*'/g, "?")
.replace(/"(?:[^"\\]|\\.)*"/g, "?")
.replace(/\s+/g, " ")
.trim()2.
3.
MINOR — Follow-up PRs4.
5.
6.
7. Validation failures never emit telemetry (Codex unique)
8. Every soft failure emits
9. Missing tests for tool.ts telemetry hooks, Positive Observations
🤖 Multi-model review by 7 AI models with structured convergence. Session artifacts |
- Extend `maskString` to also mask double-quoted strings (`"John"`, `$$secret$$`-adjacent) — single-quoted-only regex was flagged as PII leak - Keep `connection` in `ERROR_PATTERNS` keywords (broad but intentional) - Truncate `masked_sql` to 2000 chars before sending — was unbounded unlike `error_message` (500) and `masked_args` (2000) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opencode/src/altimate/native/connections/register.ts (1)
258-258: Remove redundant type assertion on return value.The object already conforms to
SqlExecuteResult(erroris optional there), so the cast can be dropped to keep type safety strict.Small cleanup
- return { columns: [], rows: [], row_count: 0, truncated: false, error: errorMsg } as SqlExecuteResult & { error: string } + return { columns: [], rows: [], row_count: 0, truncated: false, error: errorMsg }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/native/connections/register.ts` at line 258, The return expression currently uses a redundant type assertion "as SqlExecuteResult & { error: string }" when returning { columns: [], rows: [], row_count: 0, truncated: false, error: errorMsg } — remove the cast so the function returns the plain object (which already satisfies SqlExecuteResult with optional error) and keep the rest of the logic unchanged; look for the return in register.ts (the failing return expression) and simply drop the "as ..." assertion to restore strict typing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/altimate/native/connections/register.ts`:
- Line 232: maskedErrorMsg and masked_sql rely on Telemetry.maskString which
currently doesn't redact PostgreSQL dollar-quoted literals, so update
Telemetry.maskString to recognize and redact dollar-quoted literal syntax
($tag$...$tag$ and $$...$$) in addition to single/double-quoted strings, then
re-run masking where maskedErrorMsg and masked_sql are produced (references:
maskedErrorMsg in register.ts and masked_sql in the same file) to ensure those
values never contain raw dollar-quoted content; implement the change inside
Telemetry.maskString (or a helper it calls) to detect dollar-quote boundaries,
replace contents with a fixed mask (respecting the existing max length
truncation), and add unit tests covering tagged and untagged dollar-quoted
cases.
---
Nitpick comments:
In `@packages/opencode/src/altimate/native/connections/register.ts`:
- Line 258: The return expression currently uses a redundant type assertion "as
SqlExecuteResult & { error: string }" when returning { columns: [], rows: [],
row_count: 0, truncated: false, error: errorMsg } — remove the cast so the
function returns the plain object (which already satisfies SqlExecuteResult with
optional error) and keep the rest of the logic unchanged; look for the return in
register.ts (the failing return expression) and simply drop the "as ..."
assertion to restore strict typing.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 68594730-a813-4b58-af5d-c9af336b8ceb
📒 Files selected for processing (2)
packages/opencode/src/altimate/native/connections/register.tspackages/opencode/src/altimate/telemetry/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/altimate/telemetry/index.ts
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Wrap all telemetry additions in `packages/opencode/src/tool/tool.ts`
and `packages/opencode/src/tool/skill.ts` with `// altimate_change
start/end` markers so the upstream marker-guard CI passes.
- `tool.ts`: markers around `import { Telemetry }` and the full
telemetry instrumentation block (startTime through soft-failure
core_failure emission)
- `skill.ts`: markers around `classifySkillSource` helper, `startTime`
declaration, and the `Telemetry.track` try-catch for `skill_used`
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Tests — All PassedTypeScript — passedcc @suryaiyer95 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/tool/tool.ts`:
- Around line 73-74: The telemetry wrapper that records startTime and emits
"tool_call" / "core_failure" is introduced too late (startTime is set at line
73) so exceptions thrown earlier by the schema/argument validation bypass
telemetry; move the telemetry wrapper (the code that records startTime and wraps
execution to emit "tool_call" and on-error "core_failure") to wrap the
validation step as well—i.e., initialize startTime and enter the try/catch
before the validation runs—or alternatively, inside the validation's catch block
emit the same "tool_call" and "core_failure" events (including error details and
elapsed time using startTime) before rethrowing so schema validation failures
are reported consistently.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3310371d-74b5-4566-bd96-4ce9a09e6e38
📒 Files selected for processing (3)
docs/docs/reference/telemetry.mdpackages/opencode/src/tool/skill.tspackages/opencode/src/tool/tool.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opencode/src/tool/skill.ts
- docs/docs/reference/telemetry.md
Comment on lines
+73
to
+74
| // altimate_change start — telemetry instrumentation for tool execution | ||
| const startTime = Date.now() |
There was a problem hiding this comment.
Schema validation failures still bypass failure telemetry.
Line 63 can throw before this new wrapper starts, so invalid-argument calls skip both tool_call and core_failure. That leaves model/schema mismatches out of the new failure signal. Please move the telemetry wrapper above validation, or emit the same failure events from the validation catch before rethrowing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/tool/tool.ts` around lines 73 - 74, The telemetry
wrapper that records startTime and emits "tool_call" / "core_failure" is
introduced too late (startTime is set at line 73) so exceptions thrown earlier
by the schema/argument validation bypass telemetry; move the telemetry wrapper
(the code that records startTime and wraps execution to emit "tool_call" and
on-error "core_failure") to wrap the validation step as well—i.e., initialize
startTime and enter the try/catch before the validation runs—or alternatively,
inside the validation's catch block emit the same "tool_call" and "core_failure"
events (including error details and elapsed time using startTime) before
rethrowing so schema validation failures are reported consistently.
This was referenced Mar 20, 2026
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
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
core_failuretelemetry event with PII-masked arguments aligned to Rust SDK (altimate-sdk)core_failureon both soft failures (metadata.success === false) and uncaught exceptions — captures 99% of real tool failuresmaskArgs()mirroring Rust SDK masking: 19 sensitive keys with prefix/suffix matching, SQL string literals replaced with?, whitespace collapsedclassifyError(): keyword-based error classification into 7 categoriescomputeInputSignature(): records key names + value types/lengths onlyinput_signaturetotool_callevents on failuresCompanion PR: https://github.com/AltimateAI/altimate-core-internal/pull/109
Privacy
masked_args: SQL string literals →?, sensitive keys →"****", structure preservedinput_signature: key names + value types/lengths only — no actual valueserror_message: truncated to 500 charsVerified in Azure App Insights
tool_callevents withinput_signatureon failures confirmedcore_failureevents withmasked_argsready (needs interactive session)Test plan
classifyError()— 7 categories + case insensitivity (8 tests)computeInputSignature()— all value types + truncation (8 tests)maskArgs()— SQL literals, sensitive keys, structure preservation, escaped quotes, truncation (7 tests)core_failureprivacy validation (3 tests)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Privacy
Documentation
Tests