Skip to content

fix(rees): coerce non-string values in safeCodeSpan/promptText#3441

Merged
gittensory-orb[bot] merged 1 commit into
mainfrom
fix/rees-safecodespan-typeerror
Jul 5, 2026
Merged

fix(rees): coerce non-string values in safeCodeSpan/promptText#3441
gittensory-orb[bot] merged 1 commit into
mainfrom
fix/rees-safecodespan-typeerror

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Found via Sentry (issue GITTENSORY-15, 12 occurrences, regressed): TypeError: value.replace is not a function at review-enrichment/src/render-helpers.ts:29 in safeCodeSpan, hit via POST /v1/enrich, tracing through the "terminology" analyzer descriptor's render template into renderDescriptorSectionrenderBriefbuildBriefserver.ts.
  • Root cause confirmed to be boundary fragility, not an analyzer data bug: scanTerminology/detectTerminology are provably pure (term/suggestion come only from a fixed, fully-typed lookup table — no config injection point exists today that could produce a non-string). The real gap is that safeCodeSpan/promptText are typed (value: string): string but call .replace() directly with zero guard, despite receiving values from renderDescriptorSectionrenderer(result as never, ...) — an as never/as unknown boundary that trusts loosely-typed analyzer/descriptor output. 54 call sites across render.ts/registry.ts pass values through this same unguarded path. The sibling helper bytesLabel(value: number | null) in the same file already narrows its signature for exactly this kind of less-trusted input — safeCodeSpan/promptText were the odd ones out.
  • Fix: added a small asRenderableString coercion and applied it inside both safeCodeSpan and promptText, mirroring bytesLabel's existing defensive style. No blanket try/catch was added around the render pipeline — the actual unguarded boundary is fixed directly, for all 54 call sites at once, without masking any other error class.
  • No issue filed — found via direct Sentry investigation, not a report.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run build (review-enrichment's own typecheck-equivalent — clean)
  • node --test --experimental-strip-types "test/**/*.test.ts" (run from review-enrichment/) — 993/993 passing, including the 6 new regression tests
  • node scripts/generate-analyzer-metadata.mjs --check — clean
  • npm run validate:sourcemaps (review-enrichment) — 58 source maps validated
  • Root npm run typecheck / test:workers / build:mcp / test:mcp-pack / ui:openapi:check / ui:build — not applicable; review-enrichment/ is a standalone sub-package with its own tsconfig.json and test runner, not included in the root tsconfig's include list, and this change doesn't touch the Worker/MCP/UI/OpenAPI surface.
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — 6 new tests: normal-string behavior preserved for both helpers, non-string coercion (undefined, null, number, array, object-with-toString) for both helpers, and an end-to-end renderBrief test reproducing the exact Sentry stack trace (a malformed terminology finding with term: undefined, suggestion: 42 through the real "terminology" descriptor render path) that no longer throws.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. — N/A.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — N/A, no API/OpenAPI/MCP surface changed (REES's /v1/enrich request/response shape is unchanged; this only prevents a crash on malformed internal render input).
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. — N/A, no UI change.
  • Visible UI changes include a UI Evidence section below. — N/A, no visible UI change.
  • Public docs/changelogs are updated where needed. — N/A, internal robustness fix, no documented/user-facing behavior change.

Notes

  • One of five fixes from a live stack-health pass (Sentry + Loki audit on the self-hosted deployment); see the sibling PRs for the RAG chunk-cap indexing priority, PR-publish silent-drop retry, codex hang-detection, and Sentry release-validation strict-mode fixes.

safeCodeSpan and promptText are typed (value: string): string but sit at
the analyzer-output boundary, receiving findings routed through `as never`
casts (registry.ts render()) rather than values this module controls. The
terminology descriptor's render template passes item.term/item.suggestion
directly with no template-literal coercion, so a non-string term or
suggestion reaching either helper threw `value.replace is not a function`
straight out of the brief pipeline (GITTENSORY-15).

Defensively coerce in both helpers, mirroring bytesLabel's existing
`value: number | null` guard in the same file -- the established pattern
for a render helper that must never throw into the pipeline.
@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@gittensory-orb gittensory-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier. label Jul 5, 2026
@gittensory-orb

gittensory-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-05 07:11:28 UTC

2 files · 1 AI reviewer · no blockers · readiness 93/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This change fixes the reachable render-helper crash by coercing descriptor-provided values before calling string methods, and the added tests cover both direct helper calls and the terminology render path that previously threw. The implementation is correctly scoped to the boundary helper instead of adding a broad render-pipeline catch. I do not see a correctness blocker in the provided diff.

Nits — 4 non-blocking
  • nit: review-enrichment/src/render-helpers.ts:31 still types asRenderableString as accepting string even though the whole point is accepting boundary data; change it to unknown so the type contract matches the runtime behavior.
  • nit: review-enrichment/test/render-helpers.test.ts:26 uses terms that this repository's own terminology analyzer flags, so the regression fixture should use neutral arbitrary strings while still proving array coercion.
  • review-enrichment/src/render-helpers.ts:31: make the helper contract honest with `function asRenderableString(value: unknown): string { return typeof value === "string" ? value : String(value ?? ""); }`.
  • review-enrichment/test/render-helpers.test.ts:26: keep the array coercion assertion but use neutral fixture data, for example `assert.equal(safeCodeSpan(["alpha", "beta"]), "`alpha,beta`");`.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 56 registered-repo PR(s), 46 merged, 416 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 56 PR(s), 416 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 56 PR(s), 416 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@gittensory-orb gittensory-orb Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gittensory approves — the gate is satisfied and CI is green.

@gittensory-orb gittensory-orb Bot merged commit eea9ec5 into main Jul 5, 2026
8 checks passed
@gittensory-orb gittensory-orb Bot deleted the fix/rees-safecodespan-typeerror branch July 5, 2026 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant