fix: correct Tap ack format and reconnect immediately on write errors#3
Closed
fix: correct Tap ack format and reconnect immediately on write errors#3
Conversation
The TAP /channel WebSocket endpoint requires Basic Auth when TAP_ADMIN_PASSWORD is set. The consumer was sending no Authorization header, causing a 401 which gorilla/websocket surfaces as 'bad handshake'. Wire cfg.TapAdminPassword into ConsumerConfig.Password and build the Authorization header in runOnce() before dialing, matching the pattern already used by AdminClient for HTTP requests.
…ecord drops
Three interconnected bugs caused health check failures when acks were enabled
and records to be silently dropped:
1. Empty-body create/update events caused hard ParseEvent errors → never acked
→ Tap's un-acked queue grew → Tap's /health degraded → Railway health check
failed. Fix: allow these events to parse successfully; handler skips them
gracefully (nothing to store without a body) and returns nil so they get acked.
2. Records with empty CID (Tap omits cid field on some events) were silently
skipped by the Insert dedup check: existingCID("") == cid("") → Skipped.
New records were never written to the DB. Fix: only skip when cid != "".
Also log Skipped results at DEBUG for observability.
3. /health proxied to Tap's admin HTTP API, so any Tap restart/blip caused
Railway to fail the health check and restart hyperindex. Fix: /health now
reflects hyperindex's own health only. Tap sidecar status moves to /stats
where it is visible but non-blocking.
…errors
The Tap server expects acks as JSON objects: {"type":"ack","id":<id>}
but the consumer was sending bare integers (e.g. "12345"). When the server's
ReadJSON received a plain number it failed to unmarshal, causing its read loop
to close the disconnected channel and tear down the WebSocket. Every subsequent
ack write from the consumer hit the dead connection, producing a cascade of
'connection reset by peer' errors before the read loop eventually noticed.
Three fixes in this commit:
- Correct ack format: fmt.Sprintf(`{"type":"ack","id":%d}`, event.ID)
- Immediate reconnect on write errors: isWriteError() detects ack failures and
returns from runOnce() at once rather than looping on dead-connection reads
- Backoff resets on successful dial (connected=true) not only on clean close,
so a long session that drops with a read error reconnects at minBackoff (1s)
Tests updated to expect JSON ack format; two new tests added:
TestConsumer_AckFormat and TestConsumer_WriteErrorCausesImmediateReconnect.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…delete-x-icon-size-and-use-darker-red Karma/kar 110 increase lexicon delete x icon size and use darker red
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request refactors environment variable configuration in the client application (replacing Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Lexicons Page
participant Modal as Confirmation Modal
participant Mutation as Delete Mutation
User->>UI: Click delete button for lexicon
UI->>UI: Set confirmNsid state
UI->>Modal: Render modal (confirmNsid non-null)
User->>Modal: Click "Delete" button
Modal->>Mutation: Call deleteMutation.mutate(confirmNsid)
Mutation->>Mutation: Execute deletion
Modal->>UI: Clear confirmNsid state
UI->>UI: Hide modal
sequenceDiagram
participant Consumer as TAP Consumer
participant WebSocket as WebSocket Server
participant Event as Event Stream
Consumer->>WebSocket: Dial connection (runOnce)
WebSocket-->>Consumer: Connection established (connected=true)
Event-->>Consumer: Receive event
Consumer->>Consumer: Process event
Consumer->>WebSocket: Send ack (JSON format)
Note over Consumer: "Backoff resets only when<br/>connected=true, not on errors"
WebSocket-->>Consumer: Server closes
Consumer->>Consumer: Reset reconnect backoff
Consumer->>WebSocket: Attempt reconnect with minimal delay
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
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 |
railway-app
bot
temporarily deployed
to
hyperindex-v2-setup / production
Collaborator
Author
|
@coderabbitai please review this |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/lib/auth/client.ts (1)
119-129:⚠️ Potential issue | 🟠 MajorFail fast in production when
PUBLIC_CLIENT_URLis unset.Line 128 falls back to
127.0.0.1outside local dev. With the env rename, a missing production variable can now generate localhostclient_id/redirect/JWKS URLs and break OAuth discovery/callbacks.💡 Proposed fix
export const createClient = async () => { const publicUrl = env.PUBLIC_CLIENT_URL; // Must use 127.0.0.1 per RFC 8252 for ATProto OAuth localhost development const localhostUrl = `http://127.0.0.1:${env.PORT}`; const enc = encodeURIComponent; // Detect if we're running on localhost (dev mode) const isLocalDev = process.env.NODE_ENV !== "production"; + if (!isLocalDev && !publicUrl) { + throw new Error("PUBLIC_CLIENT_URL must be set in production"); + } // Use localhost URL in development, production URL otherwise - const url = isLocalDev ? localhostUrl : publicUrl || localhostUrl; + const url = isLocalDev ? localhostUrl : publicUrl;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/lib/auth/client.ts` around lines 119 - 129, The current logic sets url = isLocalDev ? localhostUrl : publicUrl || localhostUrl which silently falls back to localhost in production; update the logic to throw an error when running in production and env.PUBLIC_CLIENT_URL is not set. Specifically, keep publicUrl, localhostUrl and isLocalDev but change how url is computed so that if !isLocalDev and !publicUrl you throw a clear error (including mention of PUBLIC_CLIENT_URL) instead of using localhostUrl; otherwise set url to publicUrl in production or localhostUrl in dev.
🧹 Nitpick comments (14)
testdata/lexicons/org/hypercerts/claim/rights.json (1)
31-32: Validation constraints look reasonable.The
maxLength: 10000andmaxGraphemes: 1000pair enforces both storage limits and user-perceived character count. The 10:1 ratio accommodates multi-byte UTF-8 characters and complex grapheme clusters (emoji, combining marks).If you want to verify these constraints work as intended across different character sets, you could test with:
- Simple ASCII text (~1 byte per grapheme)
- Multi-byte Unicode text (~2-3 bytes per grapheme)
- Complex emoji sequences (~10+ bytes per grapheme)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/org/hypercerts/claim/rights.json` around lines 31 - 32, Add unit/integration tests that verify the schema constraints "maxLength" (10000) and "maxGraphemes" (1000) are enforced correctly across character sets: create test inputs for simple ASCII, multi-byte Unicode (e.g., accented/2-3 byte chars), and complex emoji/combining sequences, and assert that messages exceeding 10000 bytes are rejected while those exceeding 1000 graphemes are rejected (and valid inputs within both limits are accepted); update or add tests around the validation logic that reads "maxLength" and "maxGraphemes" so the schema in rights.json behaves as intended.client/src/app/lexicons/page.tsx (1)
446-457: Consider adding keyboard accessibility for the modal.The dialog would benefit from:
- Escape key handling to close the modal
- Focus trap to keep keyboard navigation within the modal while open
- Auto-focus on the Cancel button (or dialog container) when opened
Escape key handler example
// Add useEffect for Escape key useEffect(() => { if (!confirmNsid) return; const handleEscape = (e: KeyboardEvent) => { if (e.key === "Escape") setConfirmNsid(null); }; document.addEventListener("keydown", handleEscape); return () => document.removeEventListener("keydown", handleEscape); }, [confirmNsid]);For focus trapping, consider using a library like
@radix-ui/react-dialogorreact-focus-lock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/lexicons/page.tsx` around lines 446 - 457, The modal rendered when confirmNsid is truthy lacks keyboard accessibility; add an Escape key handler (useEffect watching confirmNsid that calls setConfirmNsid(null) on Escape), implement a focus trap for the dialog (use a library like react-focus-lock or switch to `@radix-ui/react-dialog` to ensure focus stays inside while open), and auto-focus the Cancel button by attaching a ref to that button and focusing it in a useEffect when confirmNsid becomes non-null; apply these changes around the JSX that renders the dialog (the block with role="dialog", aria-modal, confirmNsid and setConfirmNsid references).internal/graphql/types/object.go (1)
268-268: Avoid unconditional public union-type rename without migration handling.Appending
"Union"changes schema type names and can break downstream introspection/codegen consumers. Consider suffixing only on collision, or documenting this as a breaking schema change.Possible collision-aware naming approach
- unionName := lexicon.ToTypeName(contextLexiconID) + capitalizeFirst(fieldName) + "Union" + baseUnionName := lexicon.ToTypeName(contextLexiconID) + capitalizeFirst(fieldName) + unionName := baseUnionName + if _, exists := b.mapper.GetObjectType(baseUnionName); exists { + unionName = baseUnionName + "Union" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/graphql/types/object.go` at line 268, The current unconditional construction of unionName using lexicon.ToTypeName(contextLexiconID) + capitalizeFirst(fieldName) + "Union" can break consumers; change the logic in the union-name creation (where unionName is set) to detect name collisions and only append the "Union" suffix when a conflict exists (e.g., check the existing type registry/map or schema for an existing type with the base name), or generate a unique, collision-aware name (e.g., attempt baseName = lexicon.ToTypeName(contextLexiconID)+capitalizeFirst(fieldName) and if that exists, use baseName+"Union" or another deterministic suffix); adjust the code surrounding unionName, lexicon.ToTypeName and capitalizeFirst to perform this collision check rather than always renaming.internal/tap/consumer.go (2)
170-174: Non-idiomatic return order(error, bool)— consider documenting or restructuring.Go convention places error as the last return value. The current signature
(error, bool)may confuse readers. The comment explains the semantics well, but consider either:
- Restructuring to
(connected bool, err error), or- Returning a small struct if more context is needed later
This is a minor style concern; the current implementation works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/consumer.go` around lines 170 - 174, The runOnce method on type Consumer currently returns (error, bool) which is non-idiomatic in Go; change the signature of Consumer.runOnce from runOnce(ctx context.Context) (error, bool) to runOnce(ctx context.Context) (connected bool, err error) (or alternately return a small struct with those fields) and update all call sites that currently unpack the returns (look for usages of c.runOnce(...)) to match the new order so callers still use the connected value to decide reconnection backoff and handle err last.
306-316: String-based error detection is fragile but acceptable for this use case.The
isWriteErrorfunction relies on matching substrings in error messages. While this approach is brittle (error message text could change in future refactors), it works correctly here because you control the error messages indispatch()andwriteText().Consider documenting this coupling or using sentinel errors for more robust detection in the future.
♻️ Alternative using sentinel errors (optional)
// Define sentinel errors at package level var ( errAckWriteFailed = errors.New("ack write failed") errWriteDeadlineFailed = errors.New("write deadline failed") ) // In dispatch(): if err := c.writeText(conn, ackMsg); err != nil { return fmt.Errorf("%w: event %d: %w", errAckWriteFailed, event.ID, err) } // In writeText(): if err := conn.SetWriteDeadline(...); err != nil { return fmt.Errorf("%w: %w", errWriteDeadlineFailed, err) } // isWriteError becomes: func isWriteError(err error) bool { return errors.Is(err, errAckWriteFailed) || errors.Is(err, errWriteDeadlineFailed) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/consumer.go` around lines 306 - 316, isWriteError currently matches substrings which is fragile; replace that with sentinel errors: add package-level sentinel vars (e.g., errAckWriteFailed, errWriteDeadlineFailed), update dispatch() to wrap ack write failures with fmt.Errorf("%w: ...", errAckWriteFailed) and update writeText() to wrap SetWriteDeadline and write failures with fmt.Errorf("%w: ...", errWriteDeadlineFailed) (preserving original error for context), then change isWriteError(err error) to return errors.Is(err, errAckWriteFailed) || errors.Is(err, errWriteDeadlineFailed). This keeps behavior identical but makes detection robust and locatable via the symbols isWriteError, dispatch, writeText, errAckWriteFailed, and errWriteDeadlineFailed.testdata/lexicons/pub/leaflet/richtext/facet.json (1)
50-59: Consider adding URI format validation to thelinkfacet.The
linkdefinition'suriproperty is a plain string without format validation, whileatMention.atURIcorrectly uses"format": "uri". This inconsistency could allow invalid URLs.♻️ Proposed fix
"link": { "type": "object", "description": "Facet feature for a URL. The text URL may have been simplified or truncated, but the facet reference should be a complete URL.", "required": ["uri"], "properties": { "uri": { - "type": "string" + "type": "string", + "format": "uri" } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/pub/leaflet/richtext/facet.json` around lines 50 - 59, The link facet's uri property currently lacks schema validation; update the "link" object schema (property "uri") to include a URI format check by adding "format": "uri" (matching the approach used in atMention.atURI) so the validator enforces valid URLs; ensure the change is applied to the "link" definition in the lexicon JSON (property name "uri" under the "link" facet).railway.toml (1)
6-6: Re-check failure-detection tradeoff for 60s healthcheck timeout.Line 6 improves tolerance for slow boot, but it also delays restart on hard failures. Validate this against observed startup/health latency so MTTR doesn’t regress.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@railway.toml` at line 6, The healthcheckTimeout setting (healthcheckTimeout) was increased to 60s which may improve boot tolerance but will also delay restarts on hard failures; re-evaluate and pick a value based on measured startup and health probe latencies (e.g., median+3σ) or make it configurable per-environment, document the rationale inline as a comment, and if you need faster MTTR reduce the timeout (e.g., to a value closer to observed worst-case startup) or add an env var to override healthcheckTimeout so prod vs staging can differ.testdata/lexicons/pub/leaflet/blocks/horizontalRule.json (1)
7-8: Optional cleanup: omit emptyrequiredlist.For an empty-object schema, Line 7 (
"required": []) is redundant and can be removed for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/pub/leaflet/blocks/horizontalRule.json` around lines 7 - 8, The JSON schema for the horizontal rule block contains a redundant empty "required" array; remove the "required": [] entry from the horizontalRule.json schema (the object that currently lists "required" and "properties") so the schema is cleaner and only keeps "properties": {} for the empty-object schema.testdata/lexicons/pub/leaflet/blocks/code.json (1)
9-10: Consider boundingplaintextsize.Line 9 currently allows unbounded content. Adding a
maxLengthhelps prevent oversized block payloads and keeps document validation predictable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/pub/leaflet/blocks/code.json` around lines 9 - 10, The plaintext property in the code block schema is currently unbounded; add a maxLength to the "plaintext" schema entry (the "plaintext" string type) to limit payload size—choose an appropriate ceiling (e.g., 2000–10000 chars) and add "maxLength": <number> to the "plaintext" object so validation rejects oversized content.testdata/lexicons/pub/leaflet/blocks/math.json (1)
9-10: Consider adding a size limit totex.Line 9 accepts unbounded TeX input. A
maxLengthconstraint will help keep validation and storage behavior controlled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/pub/leaflet/blocks/math.json` around lines 9 - 10, The "tex" string property in the JSON schema currently allows unbounded input; update the "tex" property definition to include a maxLength constraint (e.g., "maxLength": 2048 or another agreed limit) so validation and storage are bounded, and adjust any tests or consumers that assume unlimited length accordingly; reference the "tex" property in this schema when making the change.testdata/lexicons/org/hyperboards/board.json (1)
44-60: Clarify background source invariants to avoid ambiguous rendering.Line 50 (
backgroundImage) and Line 55 (backgroundIframeUrl) can both be set at once, while Line 44 (backgroundType) implies a single active source. Add an explicit rule (or precedence) in schema/docs to keep render behavior deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/org/hyperboards/board.json` around lines 44 - 60, Add explicit invariant text to the schema/docs to make background source deterministic: declare that backgroundType, backgroundImage and backgroundIframeUrl are mutually related so only one source is active—when backgroundType == "image" the renderer must use backgroundImage and ignore backgroundIframeUrl (and backgroundImage SHOULD be present), when backgroundType == "iframe" the renderer must use backgroundIframeUrl and ignore backgroundImage (and backgroundIframeUrl SHOULD be present); alternatively specify a clear precedence rule (e.g., backgroundType wins over any set values) and mention which fields are required/ignored in each case so renderers can behave deterministically for backgroundType, backgroundImage and backgroundIframeUrl.testdata/lexicons/pub/leaflet/pages/linearDocument.json (1)
12-18: Consider adding array bounds to prevent oversized document payloads.
main.blocksandposition.blockare currently unbounded. Adding practical limits helps keep payload validation predictable and resilient.💡 Example hardening
"blocks": { "type": "array", + "maxLength": 1000, "items": { "type": "ref", "ref": "#block" } } @@ "block": { "type": "array", + "maxLength": 64, "items": { "type": "integer" } },Also applies to: 86-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/pub/leaflet/pages/linearDocument.json` around lines 12 - 18, The schema defines unbounded arrays for "blocks" (items ref "#block") and similar arrays referenced by "main.blocks" and "position.block"; add practical array bounds (e.g., maxItems and optionally minItems) to these array schemas to limit payload size and prevent oversized documents, updating the "blocks" definition and any other occurrences (main.blocks, position.block) that use the same ref so they inherit or explicitly declare the maxItems constraint.testdata/lexicons/org/hypercerts/context/measurement.json (1)
7-7: Alignsubjectscardinality with the “one or more records” description.Current schema allows
subjects: []. If one-or-more is required when present, add a minimum length to match the contract.💡 Proposed tweak
"subjects": { "type": "array", "description": "Strong references to the records this measurement refers to (e.g. activities, projects, or claims).", "items": { "type": "ref", "ref": "com.atproto.repo.strongRef" }, + "minLength": 1, "maxLength": 100 },Also applies to: 13-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/org/hypercerts/context/measurement.json` at line 7, The schema's "subjects" array under the "Measurement data related to one or more records (e.g. activities, projects, etc.)." description currently allows an empty array; add "minItems": 1 to the "subjects" array definition in measurement.json so that when "subjects" is present it must contain one or more entries, and apply the same minItems: 1 change to the other "subjects" occurrences referenced (lines 13-21) so the contract "one or more records" is enforced consistently.testdata/lexicons/org/hypercerts/claim/activity.json (1)
49-56: Bound thecontributorsarray.Every other top-level user-controlled list here is capped, but
contributorsis currently unbounded. Adding amaxLengthkeeps payload size predictable and avoids one record growing arbitrarily large.Suggested fix
"contributors": { "type": "array", "description": "An array of contributor objects, each containing contributor information, weight, and contribution details.", + "maxLength": 1000, "items": { "type": "ref", "ref": "#contributor" } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/org/hypercerts/claim/activity.json` around lines 49 - 56, The contributors array in the schema is unbounded which allows arbitrarily large payloads; update the "contributors" definition to add a sensible cap (e.g., "maxItems") so the array cannot grow without limit—locate the "contributors" property in activity.json and add a "maxItems" value appropriate for your domain (and optionally "minItems" if needed) to enforce the bound while leaving the existing "items" ref to "#contributor" unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/app/api/oauth/client-metadata.json/route.ts`:
- Around line 7-10: The code currently assigns url = publicUrl ||
`http://127.0.0.1:${env.PORT}` and thus can serve localhost OAuth metadata in
production; change the logic so the localhost fallback is only used in
non-production (e.g., when NODE_ENV !== 'production' or env.NODE !==
'production') and in production require env.PUBLIC_CLIENT_URL to be present
(throw or return an error if missing). Update the assignments around publicUrl,
url, and isConfidential (which references env.ATPROTO_JWK_PRIVATE) so that
publicUrl is mandatory in production before building client metadata and only
construct the 127.0.0.1:${env.PORT} fallback when running in development/local
environments.
In `@client/src/app/lexicons/page.tsx`:
- Around line 481-486: The click handler nullifies confirmNsid immediately which
closes the modal before deleteMutation completes; instead remove the immediate
setConfirmNsid(null) from the onClick that calls
deleteMutation.mutate(confirmNsid) and move the setConfirmNsid(null) into the
deleteMutation's onSettled callback (or onSuccess/onError as appropriate) so the
modal stays open while isConfirmDeleting is true and is cleared after the
mutation finishes; update the mutation definition that creates deleteMutation to
include an onSettled handler that calls setConfirmNsid(null) and any error
handling/display logic.
In `@client/src/lib/env.ts`:
- Around line 15-20: The scheme checks for vercelBranchUrl
(normalizedVercelBranchUrl) and the similar client URL code are too brittle:
they don't trim input and are case-sensitive (so " HTTPS://..." or "HTTPS://..."
bypasses detection). Fix by trimming the env strings (use
vercelBranchUrl.trim()) and perform a case-insensitive test like const hasScheme
= trimmed.toLowerCase().startsWith("http://") ||
trimmed.toLowerCase().startsWith("https://"); then set normalizedVercelBranchUrl
= hasScheme ? trimmed : `https://${trimmed}` (and apply the same trimmed +
case-insensitive logic to the matching public client URL variable). Ensure you
guard empty values so you don't prepend to an empty string.
In `@go.mod`:
- Around line 3-5: The go.mod change to "go 1.26" and "toolchain go1.26.0" may
break builds if other places pin older Go versions; update all CI/workflow
definitions (.github/workflows), Dockerfiles (runtime/build images),
devcontainer/configs, Makefile bootstrap scripts, and any toolchain/version
files (e.g., actions/setup-go usage, golangci-lint image tags) to use Go 1.26 or
make them accept the toolchain, and run CI locally (or via act) and rebuild
Docker images to confirm compatibility before merging.
In `@testdata/lexicons/app/certified/defs.json`:
- Around line 7-15: The description for the schema defs.did is incorrect (it
says "A Decentralized Identifier (DID) string." while the schema type is
"object"); update the description to accurately reflect that this is an object
with a nested string property (e.g., "An object containing a 'did' string
property representing a Decentralized Identifier (format: did)"), ensuring the
wording references the nested property and its format so it matches the "type":
"object" and the "properties": { "did": { "type": "string", "format": "did" } }
in the defs.did schema.
In `@testdata/lexicons/app/certified/link/evm.json`:
- Around line 13-18: The schema's "address" and "signature" fields only enforce
length; add application-level validation in the verifier path (e.g., in your
proof validation function such as verifyProof / verifyEvmProof) to reject
invalid formats: validate "address" matches 0x-prefixed hex of exactly 40 hex
chars (regex ^0x[0-9a-fA-F]{40}$) and validate "signature" is 0x-prefixed hex of
either 128 or 130 hex chars (exactly 130 or 132 characters total, allow only
^0x[0-9a-fA-F]{128}$ or ^0x[0-9a-fA-F]{130}$), and optionally perform EIP-55
checksum verification for addresses; return/throw a clear validation error
before accepting proofs if checks fail.
In `@testdata/lexicons/org/hypercerts/context/attachment.json`:
- Line 11: The attachment schema currently allows empty attachments because
"content" was removed from the "required" array; update the schema for the
"attachment" object to prevent empty records by adding "content" back into the
"required" array (so the required list includes "title", "createdAt", and
"content") or alternatively enforce a constraint that at least one of "content",
"shortDescription", or "description" must be present; modify the "required"
array or add a "oneOf"/"anyOf" clause accordingly on the attachment schema to
ensure attachments cannot be saved with only title and createdAt.
In `@testdata/lexicons/org/hypercerts/context/evaluation.json`:
- Around line 72-90: The score object's properties ("min", "max", "value") were
changed to strings which removes numeric validation and breaks downstream
numeric semantics; update the "score" schema so "min", "max", and "value" use
"type": "number" (not "string") and add appropriate JSON Schema numeric
constraints (e.g., "minimum"/"maximum" or "exclusiveMinimum"/"exclusiveMaximum"
as needed) so the schema enforces numeric values and preserves numeric
filter/sort behavior for the score object.
In `@testdata/lexicons/pub/leaflet/blocks/image.json`:
- Around line 24-33: The aspectRatio schema currently allows zero or negative
integers for "width" and "height"; update the "aspectRatio" object's "width" and
"height" property schemas (names: "aspectRatio", "width", "height") to enforce
positive dimensions by adding a minimum constraint (e.g., "minimum": 1 or
"exclusiveMinimum": 0) to each property so only integers >= 1 are valid.
---
Outside diff comments:
In `@client/src/lib/auth/client.ts`:
- Around line 119-129: The current logic sets url = isLocalDev ? localhostUrl :
publicUrl || localhostUrl which silently falls back to localhost in production;
update the logic to throw an error when running in production and
env.PUBLIC_CLIENT_URL is not set. Specifically, keep publicUrl, localhostUrl and
isLocalDev but change how url is computed so that if !isLocalDev and !publicUrl
you throw a clear error (including mention of PUBLIC_CLIENT_URL) instead of
using localhostUrl; otherwise set url to publicUrl in production or localhostUrl
in dev.
---
Nitpick comments:
In `@client/src/app/lexicons/page.tsx`:
- Around line 446-457: The modal rendered when confirmNsid is truthy lacks
keyboard accessibility; add an Escape key handler (useEffect watching
confirmNsid that calls setConfirmNsid(null) on Escape), implement a focus trap
for the dialog (use a library like react-focus-lock or switch to
`@radix-ui/react-dialog` to ensure focus stays inside while open), and auto-focus
the Cancel button by attaching a ref to that button and focusing it in a
useEffect when confirmNsid becomes non-null; apply these changes around the JSX
that renders the dialog (the block with role="dialog", aria-modal, confirmNsid
and setConfirmNsid references).
In `@internal/graphql/types/object.go`:
- Line 268: The current unconditional construction of unionName using
lexicon.ToTypeName(contextLexiconID) + capitalizeFirst(fieldName) + "Union" can
break consumers; change the logic in the union-name creation (where unionName is
set) to detect name collisions and only append the "Union" suffix when a
conflict exists (e.g., check the existing type registry/map or schema for an
existing type with the base name), or generate a unique, collision-aware name
(e.g., attempt baseName =
lexicon.ToTypeName(contextLexiconID)+capitalizeFirst(fieldName) and if that
exists, use baseName+"Union" or another deterministic suffix); adjust the code
surrounding unionName, lexicon.ToTypeName and capitalizeFirst to perform this
collision check rather than always renaming.
In `@internal/tap/consumer.go`:
- Around line 170-174: The runOnce method on type Consumer currently returns
(error, bool) which is non-idiomatic in Go; change the signature of
Consumer.runOnce from runOnce(ctx context.Context) (error, bool) to runOnce(ctx
context.Context) (connected bool, err error) (or alternately return a small
struct with those fields) and update all call sites that currently unpack the
returns (look for usages of c.runOnce(...)) to match the new order so callers
still use the connected value to decide reconnection backoff and handle err
last.
- Around line 306-316: isWriteError currently matches substrings which is
fragile; replace that with sentinel errors: add package-level sentinel vars
(e.g., errAckWriteFailed, errWriteDeadlineFailed), update dispatch() to wrap ack
write failures with fmt.Errorf("%w: ...", errAckWriteFailed) and update
writeText() to wrap SetWriteDeadline and write failures with fmt.Errorf("%w:
...", errWriteDeadlineFailed) (preserving original error for context), then
change isWriteError(err error) to return errors.Is(err, errAckWriteFailed) ||
errors.Is(err, errWriteDeadlineFailed). This keeps behavior identical but makes
detection robust and locatable via the symbols isWriteError, dispatch,
writeText, errAckWriteFailed, and errWriteDeadlineFailed.
In `@railway.toml`:
- Line 6: The healthcheckTimeout setting (healthcheckTimeout) was increased to
60s which may improve boot tolerance but will also delay restarts on hard
failures; re-evaluate and pick a value based on measured startup and health
probe latencies (e.g., median+3σ) or make it configurable per-environment,
document the rationale inline as a comment, and if you need faster MTTR reduce
the timeout (e.g., to a value closer to observed worst-case startup) or add an
env var to override healthcheckTimeout so prod vs staging can differ.
In `@testdata/lexicons/org/hyperboards/board.json`:
- Around line 44-60: Add explicit invariant text to the schema/docs to make
background source deterministic: declare that backgroundType, backgroundImage
and backgroundIframeUrl are mutually related so only one source is active—when
backgroundType == "image" the renderer must use backgroundImage and ignore
backgroundIframeUrl (and backgroundImage SHOULD be present), when backgroundType
== "iframe" the renderer must use backgroundIframeUrl and ignore backgroundImage
(and backgroundIframeUrl SHOULD be present); alternatively specify a clear
precedence rule (e.g., backgroundType wins over any set values) and mention
which fields are required/ignored in each case so renderers can behave
deterministically for backgroundType, backgroundImage and backgroundIframeUrl.
In `@testdata/lexicons/org/hypercerts/claim/activity.json`:
- Around line 49-56: The contributors array in the schema is unbounded which
allows arbitrarily large payloads; update the "contributors" definition to add a
sensible cap (e.g., "maxItems") so the array cannot grow without limit—locate
the "contributors" property in activity.json and add a "maxItems" value
appropriate for your domain (and optionally "minItems" if needed) to enforce the
bound while leaving the existing "items" ref to "#contributor" unchanged.
In `@testdata/lexicons/org/hypercerts/claim/rights.json`:
- Around line 31-32: Add unit/integration tests that verify the schema
constraints "maxLength" (10000) and "maxGraphemes" (1000) are enforced correctly
across character sets: create test inputs for simple ASCII, multi-byte Unicode
(e.g., accented/2-3 byte chars), and complex emoji/combining sequences, and
assert that messages exceeding 10000 bytes are rejected while those exceeding
1000 graphemes are rejected (and valid inputs within both limits are accepted);
update or add tests around the validation logic that reads "maxLength" and
"maxGraphemes" so the schema in rights.json behaves as intended.
In `@testdata/lexicons/org/hypercerts/context/measurement.json`:
- Line 7: The schema's "subjects" array under the "Measurement data related to
one or more records (e.g. activities, projects, etc.)." description currently
allows an empty array; add "minItems": 1 to the "subjects" array definition in
measurement.json so that when "subjects" is present it must contain one or more
entries, and apply the same minItems: 1 change to the other "subjects"
occurrences referenced (lines 13-21) so the contract "one or more records" is
enforced consistently.
In `@testdata/lexicons/pub/leaflet/blocks/code.json`:
- Around line 9-10: The plaintext property in the code block schema is currently
unbounded; add a maxLength to the "plaintext" schema entry (the "plaintext"
string type) to limit payload size—choose an appropriate ceiling (e.g.,
2000–10000 chars) and add "maxLength": <number> to the "plaintext" object so
validation rejects oversized content.
In `@testdata/lexicons/pub/leaflet/blocks/horizontalRule.json`:
- Around line 7-8: The JSON schema for the horizontal rule block contains a
redundant empty "required" array; remove the "required": [] entry from the
horizontalRule.json schema (the object that currently lists "required" and
"properties") so the schema is cleaner and only keeps "properties": {} for the
empty-object schema.
In `@testdata/lexicons/pub/leaflet/blocks/math.json`:
- Around line 9-10: The "tex" string property in the JSON schema currently
allows unbounded input; update the "tex" property definition to include a
maxLength constraint (e.g., "maxLength": 2048 or another agreed limit) so
validation and storage are bounded, and adjust any tests or consumers that
assume unlimited length accordingly; reference the "tex" property in this schema
when making the change.
In `@testdata/lexicons/pub/leaflet/pages/linearDocument.json`:
- Around line 12-18: The schema defines unbounded arrays for "blocks" (items ref
"#block") and similar arrays referenced by "main.blocks" and "position.block";
add practical array bounds (e.g., maxItems and optionally minItems) to these
array schemas to limit payload size and prevent oversized documents, updating
the "blocks" definition and any other occurrences (main.blocks, position.block)
that use the same ref so they inherit or explicitly declare the maxItems
constraint.
In `@testdata/lexicons/pub/leaflet/richtext/facet.json`:
- Around line 50-59: The link facet's uri property currently lacks schema
validation; update the "link" object schema (property "uri") to include a URI
format check by adding "format": "uri" (matching the approach used in
atMention.atURI) so the validator enforces valid URLs; ensure the change is
applied to the "link" definition in the lexicon JSON (property name "uri" under
the "link" facet).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddd0e6fe-037e-4653-b9ee-66ce381ec92c
📒 Files selected for processing (52)
client/.env.exampleclient/src/app/api/oauth/client-metadata.json/route.tsclient/src/app/lexicons/page.tsxclient/src/lib/auth/client.tsclient/src/lib/env.tsgo.modinternal/graphql/types/object.gointernal/tap/consumer.gointernal/tap/consumer_test.gorailway.tomltestdata/lexicons/app/bsky/richtext/facet.jsontestdata/lexicons/app/certified/actor/organization.jsontestdata/lexicons/app/certified/actor/profile.jsontestdata/lexicons/app/certified/badge/award.jsontestdata/lexicons/app/certified/badge/definition.jsontestdata/lexicons/app/certified/badge/response.jsontestdata/lexicons/app/certified/defs.jsontestdata/lexicons/app/certified/link/evm.jsontestdata/lexicons/app/certified/location.jsontestdata/lexicons/org/hyperboards/board.jsontestdata/lexicons/org/hyperboards/displayProfile.jsontestdata/lexicons/org/hypercerts/claim/activity.jsontestdata/lexicons/org/hypercerts/claim/contribution.jsontestdata/lexicons/org/hypercerts/claim/contributorInformation.jsontestdata/lexicons/org/hypercerts/claim/rights.jsontestdata/lexicons/org/hypercerts/collection.jsontestdata/lexicons/org/hypercerts/context/acknowledgement.jsontestdata/lexicons/org/hypercerts/context/attachment.jsontestdata/lexicons/org/hypercerts/context/evaluation.jsontestdata/lexicons/org/hypercerts/context/measurement.jsontestdata/lexicons/org/hypercerts/defs.jsontestdata/lexicons/org/hypercerts/funding/receipt.jsontestdata/lexicons/org/hypercerts/helper/workScopeTag.jsontestdata/lexicons/org/hypercerts/workscope/cel.jsontestdata/lexicons/org/hypercerts/workscope/tag.jsontestdata/lexicons/pub/leaflet/blocks/blockquote.jsontestdata/lexicons/pub/leaflet/blocks/bskyPost.jsontestdata/lexicons/pub/leaflet/blocks/button.jsontestdata/lexicons/pub/leaflet/blocks/code.jsontestdata/lexicons/pub/leaflet/blocks/header.jsontestdata/lexicons/pub/leaflet/blocks/horizontalRule.jsontestdata/lexicons/pub/leaflet/blocks/iframe.jsontestdata/lexicons/pub/leaflet/blocks/image.jsontestdata/lexicons/pub/leaflet/blocks/math.jsontestdata/lexicons/pub/leaflet/blocks/orderedList.jsontestdata/lexicons/pub/leaflet/blocks/page.jsontestdata/lexicons/pub/leaflet/blocks/poll.jsontestdata/lexicons/pub/leaflet/blocks/text.jsontestdata/lexicons/pub/leaflet/blocks/unorderedList.jsontestdata/lexicons/pub/leaflet/blocks/website.jsontestdata/lexicons/pub/leaflet/pages/linearDocument.jsontestdata/lexicons/pub/leaflet/richtext/facet.json
💤 Files with no reviewable changes (1)
- testdata/lexicons/org/hypercerts/helper/workScopeTag.json
Comment on lines
+7
to
10
| const publicUrl = env.PUBLIC_CLIENT_URL; | ||
| const url = publicUrl || `http://127.0.0.1:${env.PORT}`; | ||
| const isConfidential = !!publicUrl && !!env.ATPROTO_JWK_PRIVATE; | ||
|
|
There was a problem hiding this comment.
Avoid serving localhost OAuth metadata in production.
Line 8 uses localhost fallback regardless of deployment mode. If PUBLIC_CLIENT_URL is missing in production, this endpoint returns invalid client_id/redirect/JWKS values.
💡 Proposed fix
export async function GET() {
const publicUrl = env.PUBLIC_CLIENT_URL;
- const url = publicUrl || `http://127.0.0.1:${env.PORT}`;
+ const isLocalDev = process.env.NODE_ENV !== "production";
+ if (!isLocalDev && !publicUrl) {
+ return NextResponse.json(
+ { error: "PUBLIC_CLIENT_URL must be set in production" },
+ { status: 500 }
+ );
+ }
+ const url = publicUrl || `http://127.0.0.1:${env.PORT}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/app/api/oauth/client-metadata.json/route.ts` around lines 7 - 10,
The code currently assigns url = publicUrl || `http://127.0.0.1:${env.PORT}` and
thus can serve localhost OAuth metadata in production; change the logic so the
localhost fallback is only used in non-production (e.g., when NODE_ENV !==
'production' or env.NODE !== 'production') and in production require
env.PUBLIC_CLIENT_URL to be present (throw or return an error if missing).
Update the assignments around publicUrl, url, and isConfidential (which
references env.ATPROTO_JWK_PRIVATE) so that publicUrl is mandatory in production
before building client metadata and only construct the 127.0.0.1:${env.PORT}
fallback when running in development/local environments.
Comment on lines
+481
to
+486
| onClick={() => { | ||
| if (confirmNsid) { | ||
| deleteMutation.mutate(confirmNsid); | ||
| setConfirmNsid(null); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Modal closes before deletion completes.
Calling setConfirmNsid(null) immediately after mutate() closes the modal before the deletion request finishes. The isConfirmDeleting loading state never becomes visible since confirmNsid is nullified right away. If the deletion fails, the user won't understand why because the modal is already gone.
Move setConfirmNsid(null) to the mutation's onSettled callback instead.
Proposed fix
Update the mutation definition:
const deleteMutation = useMutation({
mutationFn: (nsid: string) =>
graphqlClient.request(DELETE_LEXICON, { nsid }),
onMutate: (nsid) => setDeletingNsid(nsid),
onSuccess: (_, nsid) => {
setSuccess(`Deleted ${nsid}`);
setError(null);
if (expandedId === nsid) setExpandedId(null);
queryClient.invalidateQueries({ queryKey: ["lexicons"] });
setTimeout(() => setSuccess(null), 3000);
},
onError: (err: Error) => {
setError(err.message);
setSuccess(null);
},
- onSettled: () => setDeletingNsid(null),
+ onSettled: () => {
+ setDeletingNsid(null);
+ setConfirmNsid(null);
+ },
});And remove the immediate setConfirmNsid(null) from the onClick:
onClick={() => {
if (confirmNsid) {
deleteMutation.mutate(confirmNsid);
- setConfirmNsid(null);
}
}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/app/lexicons/page.tsx` around lines 481 - 486, The click handler
nullifies confirmNsid immediately which closes the modal before deleteMutation
completes; instead remove the immediate setConfirmNsid(null) from the onClick
that calls deleteMutation.mutate(confirmNsid) and move the setConfirmNsid(null)
into the deleteMutation's onSettled callback (or onSuccess/onError as
appropriate) so the modal stays open while isConfirmDeleting is true and is
cleared after the mutation finishes; update the mutation definition that creates
deleteMutation to include an onSettled handler that calls setConfirmNsid(null)
and any error handling/display logic.
Comment on lines
+15
to
+20
| const vercelBranchUrl = process.env.NEXT_PUBLIC_VERCEL_BRANCH_URL || ""; | ||
| const publicClientUrl = process.env.NEXT_PUBLIC_CLIENT_URL || ""; | ||
| const normalizedVercelBranchUrl = | ||
| vercelBranchUrl && !vercelBranchUrl.startsWith("http://") && !vercelBranchUrl.startsWith("https://") | ||
| ? `https://${vercelBranchUrl}` | ||
| : vercelBranchUrl; |
There was a problem hiding this comment.
Normalize branch URL more defensively before fallback.
Line 18’s scheme check is case-sensitive and untrimmed. Inputs like HTTPS://... or leading spaces can become invalid after prefixing.
💡 Proposed fix
const vercelBranchUrl = process.env.NEXT_PUBLIC_VERCEL_BRANCH_URL || "";
const publicClientUrl = process.env.NEXT_PUBLIC_CLIENT_URL || "";
+const trimmedVercelBranchUrl = vercelBranchUrl.trim();
const normalizedVercelBranchUrl =
- vercelBranchUrl && !vercelBranchUrl.startsWith("http://") && !vercelBranchUrl.startsWith("https://")
- ? `https://${vercelBranchUrl}`
- : vercelBranchUrl;
+ trimmedVercelBranchUrl && !/^https?:\/\//i.test(trimmedVercelBranchUrl)
+ ? `https://${trimmedVercelBranchUrl}`
+ : trimmedVercelBranchUrl;
@@
- PUBLIC_CLIENT_URL: publicClientUrl || normalizedVercelBranchUrl,
+ PUBLIC_CLIENT_URL: publicClientUrl.trim() || normalizedVercelBranchUrl,Also applies to: 27-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/lib/env.ts` around lines 15 - 20, The scheme checks for
vercelBranchUrl (normalizedVercelBranchUrl) and the similar client URL code are
too brittle: they don't trim input and are case-sensitive (so " HTTPS://..." or
"HTTPS://..." bypasses detection). Fix by trimming the env strings (use
vercelBranchUrl.trim()) and perform a case-insensitive test like const hasScheme
= trimmed.toLowerCase().startsWith("http://") ||
trimmed.toLowerCase().startsWith("https://"); then set normalizedVercelBranchUrl
= hasScheme ? trimmed : `https://${trimmed}` (and apply the same trimmed +
case-insensitive logic to the matching public client URL variable). Ensure you
guard empty values so you don't prepend to an empty string.
Comment on lines
+3
to
+5
| go 1.26 | ||
|
|
||
| toolchain go1.26.0 |
There was a problem hiding this comment.
❓ Verification inconclusive
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod (version directives) =="
awk 'NR>=1 && NR<=12 {print NR ":" $0}' go.mod
echo
echo "== Potential Go version pins in repo config =="
rg -n --iglob '*.yml' --iglob '*.yaml' --iglob 'Dockerfile*' --iglob 'Makefile*' \
'setup-go|go-version|FROM golang:|GO_VERSION|GOTOOLCHAIN|go1\.[0-9]+'Repository: GainForest/hyperindex
Length of output: 1796
Verify Go 1.26/toolchain support across CI and runtime images before merge.
This upgrade can hard-fail builds if any workflow, Docker image, or local bootstrap still pins an older Go version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` around lines 3 - 5, The go.mod change to "go 1.26" and "toolchain
go1.26.0" may break builds if other places pin older Go versions; update all
CI/workflow definitions (.github/workflows), Dockerfiles (runtime/build images),
devcontainer/configs, Makefile bootstrap scripts, and any toolchain/version
files (e.g., actions/setup-go usage, golangci-lint image tags) to use Go 1.26 or
make them accept the toolchain, and run CI locally (or via act) and rebuild
Docker images to confirm compatibility before merging.
Comment on lines
+7
to
+15
| "type": "object", | ||
| "description": "A Decentralized Identifier (DID) string.", | ||
| "required": ["did"], | ||
| "properties": { | ||
| "did": { | ||
| "type": "string", | ||
| "format": "did", | ||
| "description": "The DID string value.", | ||
| "maxLength": 256 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== References to app.certified.defs#did =="
rg -n 'app\.certified\.defs#did'
echo
echo "== Potential scalar-DID assumptions in lexicons =="
rg -nP --iglob '*.json' '"format"\s*:\s*"did"' testdata/lexiconsRepository: GainForest/hyperindex
Length of output: 1796
🏁 Script executed:
# Attempt alternative verification of the review comment's claims
# Check if we can get more context about this change
# 1. First, check if the file exists and examine its structure
find . -name "defs.json" -path "*/app/certified/*" 2>/dev/null | head -20
# 2. Search for references to did format across json files
find . -name "*.json" -type f 2>/dev/null | head -50Repository: GainForest/hyperindex
Length of output: 2498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Content of testdata/lexicons/app/certified/defs.json ==="
cat -n testdata/lexicons/app/certified/defs.json
echo
echo "=== Search for references to app.certified.defs#did ==="
rg 'app\.certified\.defs#did' --type json -n
echo
echo "=== Search for other did format definitions ==="
rg '"format"\s*:\s*"did"' --type json -n
echo
echo "=== Git history of the defs.json file (last 5 commits) ==="
git log --oneline -5 -- testdata/lexicons/app/certified/defs.json
echo
echo "=== Check git diff for this file ==="
git diff HEAD~1 testdata/lexicons/app/certified/defs.json | head -100Repository: GainForest/hyperindex
Length of output: 2148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Examining files that reference app.certified.defs#did ==="
echo -e "\n--- testdata/lexicons/org/hypercerts/funding/receipt.json ---"
cat testdata/lexicons/org/hypercerts/funding/receipt.json | head -40
echo -e "\n--- testdata/lexicons/app/certified/badge/definition.json (relevant section) ---"
cat testdata/lexicons/app/certified/badge/definition.json | head -70
echo -e "\n--- testdata/lexicons/app/certified/badge/award.json ---"
cat testdata/lexicons/app/certified/badge/award.json | head -30
echo -e "\n--- testdata/lexicons/org/hypercerts/context/evaluation.json ---"
cat testdata/lexicons/org/hypercerts/context/evaluation.json | head -35Repository: GainForest/hyperindex
Length of output: 6400
Description inaccuracy: object type incorrectly described as "string".
Line 8 describes the defs.did definition as "A Decentralized Identifier (DID) string." but the actual type is "object" with a nested string property. This is misleading. The definition is correctly consumed by 6 referencing files that all expect and use it as an object type.
Wording fix
- "description": "A Decentralized Identifier (DID) string.",
+ "description": "Object containing a Decentralized Identifier (DID) string.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "type": "object", | |
| "description": "A Decentralized Identifier (DID) string.", | |
| "required": ["did"], | |
| "properties": { | |
| "did": { | |
| "type": "string", | |
| "format": "did", | |
| "description": "The DID string value.", | |
| "maxLength": 256 | |
| "type": "object", | |
| "description": "Object containing a Decentralized Identifier (DID) string.", | |
| "required": ["did"], | |
| "properties": { | |
| "did": { | |
| "type": "string", | |
| "format": "did", | |
| "description": "The DID string value.", | |
| "maxLength": 256 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testdata/lexicons/app/certified/defs.json` around lines 7 - 15, The
description for the schema defs.did is incorrect (it says "A Decentralized
Identifier (DID) string." while the schema type is "object"); update the
description to accurately reflect that this is an object with a nested string
property (e.g., "An object containing a 'did' string property representing a
Decentralized Identifier (format: did)"), ensuring the wording references the
nested property and its format so it matches the "type": "object" and the
"properties": { "did": { "type": "string", "format": "did" } } in the defs.did
schema.
Comment on lines
+13
to
+18
| "address": { | ||
| "type": "string", | ||
| "description": "EVM wallet address (0x-prefixed, with EIP-55 checksum recommended).", | ||
| "minLength": 42, | ||
| "maxLength": 42 | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AT Protocol Lexicon string schemas: what constraint keywords are supported for regex-like validation (e.g., hex strings with required 0x prefix), and what is the recommended way to enforce exact allowed lengths such as 130 or 132 only?
💡 Result:
AT Protocol Lexicon string schemas support the following constraint keywords for validation: minLength, maxLength (UTF-8 bytes), minGraphemes, maxGraphemes (Unicode graphemes), format (with predefined values: at-identifier, at-uri, cid, datetime, did, handle, nsid, tid, uri, language), knownValues (suggested values, non-enforcing), enum (closed set), const, default. No built-in "regex-like" constraint keyword (e.g., regex or pattern) is supported for arbitrary regex validation, including hex strings with 0x prefix. Predefined formats like "cid" validate base58-encoded CIDs (no 0x), but not hex. For custom patterns like "0x" + 64/66 hex chars (130/132 chars total), use a plain string type and enforce in application code, or combine minLength/maxLength if exact length matches. The recommended way to enforce exact allowed lengths (e.g., 130 or 132 only) is to set minLength and maxLength to the same value, e.g., "minLength": 130, "maxLength": 130. This works for byte lengths; use grapheme constraints if needed for visual consistency. Style guide advises against redundant format + length limits, but exact length via minLength=maxLength is standard. Validation implementations (e.g., SDKs) count UTF-8 bytes accurately.
Citations:
- 1: https://atproto.com/specs/lexicon
- 2: https://mintlify.com/bluesky-social/atproto/concepts/lexicons
- 3: https://www.mintlify.com/bluesky-social/atproto/concepts/lexicons
- 4: https://atproto.com/guides/lexicon-style-guide
- 5: https://atproto.blue/en/latest/atproto_client/string_formats.html
- 6: https://atproto.blue/en/latest/atproto/atproto%5Fclient.models.string%5Fformats.html
🏁 Script executed:
cat -n testdata/lexicons/app/certified/link/evm.jsonRepository: GainForest/hyperindex
Length of output: 4073
Address and signature fields lack format validation; only length constraints are enforced.
Lines 16-17, 40-41, and 64-65 enforce length only via minLength/maxLength. The signature field (lines 40-41) allows lengths 130–132, which includes 131 (invalid for 64 or 65 byte hex with 0x prefix). The address fields allow any 42-character string, not just valid hex with 0x prefix.
AT Protocol Lexicon does not support regex or pattern validation keywords. To enforce exact signature lengths (130 or 132 only, not 131) and validate hex format with 0x prefix for addresses, implement application-level validation in the verifier path before accepting proofs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testdata/lexicons/app/certified/link/evm.json` around lines 13 - 18, The
schema's "address" and "signature" fields only enforce length; add
application-level validation in the verifier path (e.g., in your proof
validation function such as verifyProof / verifyEvmProof) to reject invalid
formats: validate "address" matches 0x-prefixed hex of exactly 40 hex chars
(regex ^0x[0-9a-fA-F]{40}$) and validate "signature" is 0x-prefixed hex of
either 128 or 130 hex chars (exactly 130 or 132 characters total, allow only
^0x[0-9a-fA-F]{128}$ or ^0x[0-9a-fA-F]{130}$), and optionally perform EIP-55
checksum verification for addresses; return/throw a clear validation error
before accepting proofs if checks fail.
| "record": { | ||
| "type": "object", | ||
| "required": ["title", "content", "createdAt"], | ||
| "required": ["title", "createdAt"], |
There was a problem hiding this comment.
Don't allow empty attachment records.
With content removed from required and both shortDescription and description still optional, this schema now accepts records that contain only title and createdAt. That makes “attachment” records valid even when they attach nothing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testdata/lexicons/org/hypercerts/context/attachment.json` at line 11, The
attachment schema currently allows empty attachments because "content" was
removed from the "required" array; update the schema for the "attachment" object
to prevent empty records by adding "content" back into the "required" array (so
the required list includes "title", "createdAt", and "content") or alternatively
enforce a constraint that at least one of "content", "shortDescription", or
"description" must be present; modify the "required" array or add a
"oneOf"/"anyOf" clause accordingly on the attachment schema to ensure
attachments cannot be saved with only title and createdAt.
Comment on lines
+72
to
+90
| "score": { | ||
| "type": "object", | ||
| "description": "Overall score for an evaluation on a numeric scale.", | ||
| "required": ["min", "max", "value"], | ||
| "properties": { | ||
| "min": { | ||
| "type": "string", | ||
| "description": "Minimum value of the scale as a numeric string (e.g. '0', '1').", | ||
| "maxLength": 50 | ||
| }, | ||
| "max": { | ||
| "type": "string", | ||
| "description": "Maximum value of the scale as a numeric string (e.g. '5', '10').", | ||
| "maxLength": 50 | ||
| }, | ||
| "value": { | ||
| "type": "string", | ||
| "description": "Score within the inclusive range [min, max] as a numeric string (e.g. '3.7').", | ||
| "maxLength": 50 |
There was a problem hiding this comment.
Keep score numerically typed.
Changing min, max, and value to unconstrained strings means the schema now accepts non-numeric values, and downstream code will treat these fields as strings instead of numbers. internal/lexicon/types.go:70-123 only gives strings length validation, so this loses numeric validation/filter/sort semantics for score.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testdata/lexicons/org/hypercerts/context/evaluation.json` around lines 72 -
90, The score object's properties ("min", "max", "value") were changed to
strings which removes numeric validation and breaks downstream numeric
semantics; update the "score" schema so "min", "max", and "value" use "type":
"number" (not "string") and add appropriate JSON Schema numeric constraints
(e.g., "minimum"/"maximum" or "exclusiveMinimum"/"exclusiveMaximum" as needed)
so the schema enforces numeric values and preserves numeric filter/sort behavior
for the score object.
Comment on lines
+24
to
+33
| "aspectRatio": { | ||
| "type": "object", | ||
| "required": ["width", "height"], | ||
| "properties": { | ||
| "width": { | ||
| "type": "integer" | ||
| }, | ||
| "height": { | ||
| "type": "integer" | ||
| } |
There was a problem hiding this comment.
Enforce positive aspectRatio dimensions.
Line 28 and Line 31 currently allow 0 and negative values. That permits invalid image ratios and can cause downstream layout/division issues.
Suggested schema tightening
"aspectRatio": {
"type": "object",
"required": ["width", "height"],
"properties": {
"width": {
- "type": "integer"
+ "type": "integer",
+ "minimum": 1
},
"height": {
- "type": "integer"
+ "type": "integer",
+ "minimum": 1
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "aspectRatio": { | |
| "type": "object", | |
| "required": ["width", "height"], | |
| "properties": { | |
| "width": { | |
| "type": "integer" | |
| }, | |
| "height": { | |
| "type": "integer" | |
| } | |
| "aspectRatio": { | |
| "type": "object", | |
| "required": ["width", "height"], | |
| "properties": { | |
| "width": { | |
| "type": "integer", | |
| "minimum": 1 | |
| }, | |
| "height": { | |
| "type": "integer", | |
| "minimum": 1 | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testdata/lexicons/pub/leaflet/blocks/image.json` around lines 24 - 33, The
aspectRatio schema currently allows zero or negative integers for "width" and
"height"; update the "aspectRatio" object's "width" and "height" property
schemas (names: "aspectRatio", "width", "height") to enforce positive dimensions
by adding a minimum constraint (e.g., "minimum": 1 or "exclusiveMinimum": 0) to
each property so only integers >= 1 are valid.
fix: stabilize records keyset pagination across timestamp formats
Collaborator
Author
|
closing this pr to run ci and have a better description |
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.
Problem
The Tap consumer was sending acks as plain integers (e.g.
"12345") but the Tap server's read loop callsws.ReadJSON(&msg)expecting a JSON object:{"type":"ack","id":12345}. When the server received the malformed ack, itsReadJSONfailed, the read goroutine closed the connection, and every subsequent ack write from the consumer hit the dead socket — producing a cascade of hundreds of"connection reset by peer"warnings before the connection was finally dropped.The exponential backoff (1s → 2s → 4s → ... → 2min) then made recovery slow, and since no events were ever acked, Tap kept re-delivering the full backlog from the start on every reconnect.
Root Cause
Found by reading the Tap server source (
cmd/tap/types.goandserver.go):Changes
{"type":"ack","id":%d}matching the Tap server'sWsResponsestructisWriteError()detects ack write failures and returns fromrunOnce()immediately instead of continuing to read on a dead connection, preventing the cascade of hundreds of identical error logserr == nil); now resets whenever the dial itself succeeded (connected=true) so a long-running session that drops with a network error reconnects at 1s, not the last escalated intervalTests
TestConsumer_ReceivesAndAcksRecordEventandTestConsumer_ReceivesAndAcksIdentityEventto expect JSON ack formatTestConsumer_AckFormat— verifies the ack parses as valid JSON with correcttypeandidfieldsTestConsumer_WriteErrorCausesImmediateReconnect— verifies the consumer reconnects immediately after a write failure rather than looping through more dead-connection readsSummary by CodeRabbit
New Features
Bug Fixes
Configuration
Chores