fix: stabilize tap ingestion, keyset pagination, and OAuth/DPoP handling#8
Merged
fix: stabilize tap ingestion, keyset pagination, and OAuth/DPoP handling#8
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds local git pre-commit hook and Makefile target; updates CI action versions; introduces client env URL normalization; tightens DPoP/JWK error handling; normalizes DB timestamp expressions for keyset pagination; enhances Tap consumer auth/ack semantics; many new/updated lexicon JSON fixtures and related tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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)
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 |
Collaborator
Author
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
railway-app
bot
temporarily deployed
to
hyperindex-v2-setup / production
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.
Kzoeps
force-pushed
the
fix/tap-ack-issue
branch
from
ce258b6 to
ca01651
Compare
railway-app
bot
temporarily deployed
to
hyperindex-v2-setup / production
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/src/app/lexicons/page.tsx (1)
263-275:⚠️ Potential issue | 🟠 MajorKeep the confirmation modal open until mutation settles.
Right now, Line 484 closes the modal immediately after
mutate(), so the loading state at Line 480 is effectively skipped and errors happen after the dialog disappears. Move dialog close intoonSuccess(oronSettledif preferred).💡 Suggested fix
const deleteMutation = useMutation({ mutationFn: (nsid: string) => graphqlClient.request(DELETE_LEXICON, { nsid }), onMutate: (nsid) => setDeletingNsid(nsid), onSuccess: (_, nsid) => { + setConfirmNsid((current) => (current === nsid ? null : current)); 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), }); ... <Button type="button" variant="destructive" size="sm" loading={isConfirmDeleting} onClick={() => { if (confirmNsid) { deleteMutation.mutate(confirmNsid); - setConfirmNsid(null); } }} >Also applies to: 481-485
🤖 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 263 - 275, The confirmation modal is being closed immediately after calling mutate(), which skips the mutation loading state and hides errors; update the flow so the modal close happens inside the mutation callbacks instead: remove or move the immediate modal-close call that runs right after mutate(), and instead call the modal-close (e.g., clear deletingNsid / call setDeletingNsid(null) or call the function that hides the dialog) inside the mutation's onSettled or onSuccess handler (the mutation configured with onSuccess, onError, onSettled that currently calls setDeletingNsid(null)); ensure setDeletingNsid(null) or equivalent is used to close the modal only after the mutation settles so the loading/error UI in the modal is preserved.internal/oauth/middleware_test.go (1)
333-365:⚠️ Potential issue | 🟡 MinorFail this test on setup errors.
GenerateDPoPProof()is still ignored here. If setup fails andproofends up empty, the middleware still returns401, so this test can pass without ever exercising the key-mismatch branch.Proposed fix
- keyPair1, _ := GenerateDPoPKeyPair() - keyPair2, _ := GenerateDPoPKeyPair() + keyPair1, err := GenerateDPoPKeyPair() + if err != nil { + t.Fatalf("failed to generate keyPair1: %v", err) + } + keyPair2, err := GenerateDPoPKeyPair() + if err != nil { + t.Fatalf("failed to generate keyPair2: %v", err) + } @@ - proof, _ := keyPair2.GenerateDPoPProof("GET", "https://example.com/protected", "", "") + proof, err := keyPair2.GenerateDPoPProof("GET", "https://example.com/protected", "", "") + if err != nil { + t.Fatalf("failed to generate DPoP proof: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/oauth/middleware_test.go` around lines 333 - 365, The test TestAuthMiddleware_RequireAuth_DPoP_KeyMismatch currently ignores errors from GenerateDPoPProof (and earlier key generation) so a setup failure can produce an empty proof and falsely pass; update the test to check and fail on errors from GenerateDPoPKeyPair, CalculateJKT and GenerateDPoPProof (and assert proof is non-empty) before invoking middleware.RequireAuth so the key-mismatch branch is actually exercised when the proof is generated with keyPair2.internal/database/repositories/records.go (1)
136-164:⚠️ Potential issue | 🟠 MajorPreserve the existing CID when the incoming event omits it.
With the new skip guard, an update carrying
cid == ""now falls into the upsert path and unconditionally writes the empty CID back to the row. That drops a previously known content hash and makes later dedup/debugging worse. Keep the old CID unless the incoming one is non-empty.Suggested fix
case database.PostgreSQL: sqlStr = fmt.Sprintf(`INSERT INTO record (uri, cid, did, collection, json) VALUES (%s, %s, %s, %s, %s::jsonb) ON CONFLICT(uri) DO UPDATE SET - cid = EXCLUDED.cid, + cid = CASE + WHEN EXCLUDED.cid <> '' THEN EXCLUDED.cid + ELSE record.cid + END, json = EXCLUDED.json, indexed_at = NOW()`, p1, p2, p3, p4, p5) default: sqlStr = fmt.Sprintf(`INSERT INTO record (uri, cid, did, collection, json) VALUES (%s, %s, %s, %s, %s) ON CONFLICT(uri) DO UPDATE SET - cid = excluded.cid, + cid = CASE + WHEN excluded.cid <> '' THEN excluded.cid + ELSE record.cid + END, json = excluded.json, indexed_at = datetime('now')`, p1, p2, p3, p4, p5)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/database/repositories/records.go` around lines 136 - 164, The upsert currently overwrites an existing cid with an empty incoming cid; modify the ON CONFLICT DO UPDATE clause in the SQL strings inside records.go (where sqlStr is built for database.PostgreSQL and default) so cid is preserved when EXCLUDED/ excluded.cid is an empty string—e.g. set cid = COALESCE(NULLIF(EXCLUDED.cid, ''), record.cid) for Postgres (and cid = COALESCE(NULLIF(excluded.cid, ''), record.cid) in the default/SQLite branch) instead of unconditionally assigning EXCLUDED/ excluded.cid; leave the json and indexed_at updates as-is.
🧹 Nitpick comments (9)
Makefile (1)
1-1: Consider adding a conventionalalltarget.This would resolve the current checkmake warning and provide a standard default entrypoint.
Minimal Makefile adjustment
-.PHONY: help build run test lint clean dev db-migrate db-rollback docker hooks-install +.PHONY: all help build run test lint clean dev db-migrate db-rollback docker hooks-install + +all: help🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, Add a conventional default "all" Makefile target to eliminate the checkmake warning and provide a standard entrypoint: create an "all" phony target (add "all" to the .PHONY line) that depends on the primary default action (e.g., "build" or whichever target is intended as the default) so invoking make with no arguments runs that default; update .PHONY: to include "all" alongside the existing targets like "build", "run", "test", etc., and ensure the "all" target simply delegates to the chosen primary target.testdata/lexicons/org/hypercerts/claim/contributorInformation.json (1)
13-17: Alignidentifiervalidation with the documented contract.Line 15 documents DID-or-URI semantics, but Line 14 still accepts any string. Consider modeling this as a typed union to enforce the stated shape.
Proposed refactor
"identifier": { - "type": "string", - "description": "DID (did:plc:...) or URI to a social profile of the contributor.", - "maxLength": 2048 + "type": "union", + "description": "DID (did:plc:...) or URI to a social profile of the contributor.", + "refs": [ + "app.certified.defs#did", + "org.hypercerts.defs#uri" + ] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/org/hypercerts/claim/contributorInformation.json` around lines 13 - 17, The current "identifier" schema in contributorInformation.json claims to be a DID-or-URI but only permits a generic string; change the schema for the "identifier" property to a typed union (oneOf) that enforces either a URI (use format: "uri" plus maxLength) or a DID (use a regex pattern like '^did:[a-z0-9]+:.*$' and maxLength). Update the "identifier" entry (referenced as "identifier" in contributorInformation.json) to use oneOf with two sub-schemas (URI and DID) and preserve the existing description and maxLength.testdata/lexicons/pub/leaflet/blocks/button.json (1)
9-15: Add length bounds fortextandurl.Both fields are effectively unbounded right now; lightweight max lengths improve consistency and reduce oversized record risk.
Suggested patch
"text": { - "type": "string" + "type": "string", + "maxLength": 1000 }, "url": { "type": "string", - "format": "uri" + "format": "uri", + "maxLength": 2048 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/pub/leaflet/blocks/button.json` around lines 9 - 15, The JSON schema for the button block is missing length bounds on the "text" and "url" properties; add sensible constraints by setting "text" to include at least a minLength (e.g., 1) and a small maxLength (e.g., 128) and setting "url" to include a maxLength (e.g., 2048) while preserving its "format":"uri"; update the "text" and "url" property definitions (the keys "text" and "url" in the block schema) to include these "minLength"/"maxLength" entries to prevent unbounded values.testdata/lexicons/pub/leaflet/blocks/page.json (1)
9-10: Constrainidlength to avoid unbounded payloads.
idis currently unbounded; adding a limit improves schema hygiene and ingestion safety.Suggested patch
"id": { - "type": "string" + "type": "string", + "maxLength": 256 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/pub/leaflet/blocks/page.json` around lines 9 - 10, The "id" property in the JSON schema is unbounded; update the "id" schema object (the "id" property in page.json) to include a maximum length constraint, for example add "maxLength": 256 to the id definition to prevent unbounded payloads and improve ingestion safety; ensure the property remains type: "string" and pick a sensible maxLength (e.g., 256) consistent with other IDs in your lexicons.testdata/lexicons/pub/leaflet/pages/linearDocument.json (1)
12-19: Consider addingmaxLengthconstraint onblocksarray.The
blocksarray has no upper bound, which could allow very large documents. If there's a practical limit on document size, adding a constraint would provide clearer expectations for consumers.💡 Example constraint
"blocks": { "type": "array", + "maxLength": 10000, "items": { "type": "ref", "ref": "#block" } }🤖 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 - 19, The "blocks" array schema currently has no upper bound; update the "blocks" definition in linearDocument.json (the "blocks" property under the document schema) to include an appropriate maxLength constraint (e.g., "maxLength": <reasonable_integer>) so consumers know the practical limit; ensure the constraint is added alongside the existing "type": "array" and "items": {"type":"ref","ref":"#block"} entry and pick a sensible value consistent with other system limits or documentation.testdata/lexicons/pub/leaflet/richtext/facet.json (1)
50-59: Addformat: "uri"constraint tolink.urifor consistency.The
linkfacet'surifield lacks format validation, whileatMention.atURI(line 78) includes"format": "uri". This inconsistency could allow malformed URLs in link facets while rejecting them in AT mentions.♻️ 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 is missing a URL format constraint; update the "link" object schema so the "uri" property includes "format": "uri" (matching the atMention.atURI usage) to enforce proper URI validation while keeping the property as type "string" and required in the "link" definition.testdata/lexicons/pub/leaflet/blocks/unorderedList.json (1)
34-41: Consider addingmaxLengthconstraint on nestedchildrenarray.The
childrenarray for nested list items has nomaxLengthconstraint, which could allow deeply nested or very large list structures. If there's a practical limit on nesting depth or list size, adding a constraint would help prevent unbounded data.💡 Example constraint
"children": { "type": "array", "description": "Nested unordered list items. Mutually exclusive with orderedListChildren; if both are present, children takes precedence.", + "maxLength": 100, "items": { "type": "ref", "ref": "#listItem" } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/pub/leaflet/blocks/unorderedList.json` around lines 34 - 41, Add a maxLength constraint to the "children" array definition (the nested-list "children" property that references "#listItem") to prevent unbounded nesting/size; update the schema for "children" to include a sensible maxLength (or maxItems) value (e.g., a practical limit such as 10–20) so deeply nested or overly large lists are rejected by the validator.internal/oauth/dpop_test.go (1)
79-199: Add deterministic fixtures for the new normalization paths.These tests only use randomly generated keys, so they almost never exercise the leading-zero and zero-padded JWK cases introduced in
ParseDPoPKeyPair()/VerifyDPoPProof(). Please add table-driven fixtures forx/y/dwith leading zeroes, oversized zero-padded inputs, and mismatcheddvs public key.As per coding guidelines,
**/*_test.go: Use table-driven tests for test functions with a test slice containing name, input, expected output, and error expectation fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/oauth/dpop_test.go` around lines 79 - 199, The tests only use random keys and don't cover the new normalization edge cases in ParseDPoPKeyPair()/VerifyDPoPProof(); add table-driven fixtures (a slice of structs with name, jwk JSON input strings for x/y/d fields, expected error bool, and expected outputs like presence/absence of PrivateKey or matching JKT) that include: (1) leading-zeroed x/y, (2) oversized zero-padded x/y, (3) leading-zero d, (4) zero-padded d, and (5) mismatched d vs public key; write new table-driven test(s) (e.g., TestDPoPKeyPairNormalization or extend TestDPoPKeyPairJSON) that iterate the cases, call ParseDPoPKeyPair() and VerifyDPoPProof() as appropriate, and assert expected errors and key/public-match behavior using the existing functions GenerateDPoPKeyPair, ParseDPoPKeyPair, ToJSON/ToPrivateJSON, CalculateJKT, GenerateDPoPProof, and VerifyDPoPProof.internal/database/repositories/records_test.go (1)
520-605: Convert this new pagination suite to a table-driven test.The two scenarios here already share the same fixture and differ only by cursor direction and expected URIs. A table will make it much easier to extend the normalization coverage without duplicating setup and assertions.
As per coding guidelines, "
**/*_test.go: Use table-driven tests for test functions with a test slice containing name, input, expected output, and error expectation fields`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/database/repositories/records_test.go` around lines 520 - 605, The two subtests in TestRecordsRepository_KeysetPagination_NormalizesIndexedAtFormats should be converted into a single table-driven test: create a slice of cases containing fields like name, useForward (or method selector), cursor ([]string), expectedURIs ([]string) and expectedCount; keep the shared fixture/setup (env, repo, inserted records) at the top of the test, then loop over cases with t.Run(case.name, func(t *testing.T){ ... }) and inside call either repo.GetByCollectionSortedWithKeysetCursor or repo.GetByCollectionReversedWithKeysetCursor based on the case, assert err, assert len == expectedCount and assert the returned URIs match case.expectedURIs in order; ensure the existing cursor values ("2026-01-15T12:00:00Z", ...) and expected URIs ([n2,n1] and [n3,n2]) are used for the two cases so behavior is 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 @.githooks/pre-commit:
- Around line 23-25: The current hook runs gofmt -w on ${staged_go_files[@]}
then git add, which can accidental stage unstaged hunks; wrap the formatting
step with a temporary stash of unstaged changes (use git stash --keep-index
before running gofmt and git stash pop afterward) so only the staged snapshot is
formatted and re-added; update the pre-commit script around the gofmt and git
add lines (referencing staged_go_files, gofmt -w and git add --
"${staged_go_files[@]}") to perform git stash --keep-index, run gofmt and git
add, then git stash pop to restore unstaged work.
In `@client/src/app/lexicons/page.tsx`:
- Around line 446-493: The modal rendered when confirmNsid is truthy lacks
proper keyboard/focus behavior; update the dialog implementation (the element
with role="dialog"/aria-modal and the surrounding JSX that uses confirmNsid,
setConfirmNsid, deleteMutation, isConfirmDeleting) to: trap focus inside the
dialog by moving focus to the dialog (or first focusable) on open and cycling
Tab/Shift+Tab among tabbable elements, close the dialog on Escape by adding a
keydown handler that calls setConfirmNsid(null), and restore focus to the
element that opened the dialog when it closes; implement this via refs and a
useEffect to add/remove the keydown listener and to save/restore the previously
focused element so keyboard users can safely interact with the dialog.
In `@client/src/lib/env.ts`:
- Around line 16-17: The code currently uses raw publicClientUrl
(NEXT_PUBLIC_CLIENT_URL) which can lack a scheme; create a
normalizedPublicClientUrl (similar to normalizedVercelBranchUrl) by ensuring a
scheme (default to "https://") and trimming any trailing slash, then use
normalizedPublicClientUrl everywhere you currently use publicClientUrl for
building OAuth metadata (client_uri, redirect_uris, jwks_uri) so downstream URLs
are always valid; update references where normalizedVercelBranchUrl is used to
prefer normalizedPublicClientUrl when NEXT_PUBLIC_CLIENT_URL is present and
falls back to normalizedVercelBranchUrl otherwise.
In `@cmd/hypergoat/main.go`:
- Around line 329-337: The Tap sidecar health probe is using the request context
(reqCtx) which can block the /stats handler if the sidecar stalls; change the
health check to use a short, independent timeout context instead of reqCtx:
create a child context with context.WithTimeout (e.g., 200-500ms) and pass that
to bg.tapAdminClient.Health, ensure you call cancel() to release resources, and
keep the probe best-effort by falling back to marking
tapInfo["sidecar"]="unreachable" and setting tapInfo["sidecar_error"] on timeout
or error.
In `@internal/graphql/schema/builder.go`:
- Around line 642-649: The search resolver still emits second-precision cursors
causing pagination instability; update createSearchResolver() so any cursor
built for the "indexed_at" sort uses the same sub-second format as
sortFieldValueForRecord() (i.e., call
rec.IndexedAt.UTC().Format(time.RFC3339Nano) or otherwise use time.RFC3339Nano)
instead of the current second-precision format, ensuring the cursor string for
indexed_at matches the sortFieldValueForRecord() output and preserving
sub-second precision for stable search pagination.
In `@internal/oauth/dpop.go`:
- Around line 80-87: The JWK currently trims leading zeroes (via
trimLeadingZeros) when building X, Y (and similarly D elsewhere), producing
non-canonical variable-width members; change the code in internal/oauth/dpop.go
to use the full coordLen-byte slices for X and Y (i.e., remove trimLeadingZeros
and encode pubBytes[1:1+coordLen] and pubBytes[1+coordLen:] directly with
base64.RawURLEncoding.EncodeToString) and likewise ensure the private D is
encoded as a fixed 32-byte value (left-pad the private scalar to coordLen bytes
if necessary) so the JWK fields Kty/Crv X/Y/D are always 32 bytes for P-256;
references: JWK struct fields X, Y, D and the trimLeadingZeros usage.
In `@internal/server/oauth_handlers.go`:
- Around line 898-909: The current handlers convert the signing key with
h.config.SigningKey.ToJWK() and compute its thumbprint with CalculateJKT(), but
on error they log a warning and return a 200 with an empty jwks via
h.writeJSON(w, http.StatusOK, jwks); instead return an HTTP 5xx (e.g., 500) and
a clear error body when ToJWK or CalculateJKT fails so clients don't treat an
empty {"keys":[]} as a valid state—update the error branches for ToJWK and
CalculateJKT to log the full error and call h.writeJSON with an appropriate
server-error status and message instead of StatusOK.
In `@internal/tap/consumer_test.go`:
- Around line 1035-1043: The test's reconnect timing check using secondConnected
and start is too lax and can pass the normal backoff path; update the assertion
to specifically prove the write-error fast path by either measuring elapsed from
the actual disconnect time (record the time when the consumer detects the write
error / first disconnect and use that as the baseline instead of start) or by
tightening the allowed window to well under minBackoff (e.g., ensure elapsed <
minBackoff - slack, such as <900ms) when observing secondConnected; adjust the
check around secondConnected and the used baseline variable so the test fails if
the reconnect used the ordinary 1s backoff instead of the fast-path after write
error.
In `@internal/tap/consumer.go`:
- Around line 117-123: The current logic in Start() resets backoff to minBackoff
when c.runOnce(ctx) returns connected but still always sleeps before redialing,
causing ack write failures to incur the normal reconnect delay; modify
c.runOnce(ctx) to return an additional boolean (e.g., skipBackoff) or an error
sentinel indicating a broken-ack-write condition, set skipBackoff when the
failure is a write/ack failure, and update Start() to check that flag and bypass
the sleep/backoff when skipBackoff is true (while keeping the existing backoff
reset behavior when connected). Apply the same change to the other c.runOnce
callsite mentioned (the similar block around the later reconnect logic) so both
paths immediately retry on ack-write failures.
In `@testdata/lexicons/app/bsky/richtext/facet.json`:
- Around line 46-47: The schema currently only forbids negatives for
byteStart/byteEnd but allows byteEnd < byteStart; update the write-path
validation for the byteSlice in facet.json to enforce the invariant byteEnd >=
byteStart (add an object-level constraint in the JSON Schema or validator logic
that checks byteEnd is >= byteStart for the byteSlice properties byteStart and
byteEnd) and add tests that cover valid (equal and greater) and invalid (byteEnd
< byteStart) cases to prevent invalid facet ranges.
In `@testdata/lexicons/app/certified/defs.json`:
- Around line 7-17: The DID schema was accidentally changed from a scalar string
to an object, which is a breaking change; revert app.certified.defs#did back to
a simple string schema (type: "string", format: "did", description: "A
Decentralized Identifier (DID) string.", maxLength: 256) instead of an object
with a "did" property, and remove the surrounding object wrapper so all existing
references/unions/array items expecting a string continue to work.
In `@testdata/lexicons/app/certified/link/evm.json`:
- Around line 37-42: The "signature" JSON schema currently uses minLength: 130
and maxLength: 132 which permits an invalid 131-character hex string; update
validation so only exact valid lengths are allowed: either 130 (64 bytes hex +
"0x") or 132 (65 bytes hex + "0x"). Replace the loose min/max on the "signature"
field with a stricter schema (e.g., an anyOf/oneOf with two length constraints
or a regex that enforces 0x + 128 or 130 hex chars) and/or enforce exact-length
checks in ingestion-time validation code that parses the "signature" field to
reject 131-length values. Ensure references to the "signature" property remain
consistent.
In `@testdata/lexicons/org/hypercerts/claim/activity.json`:
- Around line 116-154: The schema broke backward compatibility by replacing
plain-string definitions with object wrappers for contributorIdentity,
contributorRole, and workScopeString; update those definitions
(contributorIdentity, contributorRole, workScopeString) to use a union
(oneOf/anyOf) that accepts the new object shape OR the original string form OR
the external reference types (com.atproto.repo.strongRef for identity/role and
org.hypercerts.workscope.cel for workScope) so existing records using strings or
strongRef/cel references still validate; ensure required properties remain valid
for the object branch and keep unchanged string/ref branches for backward
compatibility.
In `@testdata/lexicons/org/hypercerts/context/evaluation.json`:
- Around line 72-91: The schema for "score" currently declares "min", "max", and
"value" as string which permits non-numeric values; change the three properties
in the "score" object ("min", "max", "value") back to numeric types (set "type":
"number" for each), remove string-specific keywords like "maxLength", and keep
the descriptive text; if you cannot change the schema to number for backward
compatibility, instead add strict ingest-time validation logic that parses and
rejects non-numeric inputs for score.min/score.max/score.value and add rejection
tests that assert payloads like "N/A" fail validation.
In `@testdata/lexicons/pub/leaflet/blocks/image.json`:
- Around line 28-33: The schema's "width" and "height" integer properties
currently allow zero and negative values; update the JSON schema for the "width"
and "height" properties (the entries named "width" and "height") to enforce
positive dimensions by adding a constraint such as "minimum": 1 (or
"exclusiveMinimum": 0) so only integers > 0 are valid; ensure both properties
are updated consistently and run schema validation to confirm the change.
In `@testdata/lexicons/pub/leaflet/blocks/orderedList.json`:
- Around line 9-12: The schema for the ordered list's startIndex currently
allows any integer (including 0 or negative); update the "startIndex" property
in the orderedList JSON schema to enforce a lower bound by adding "minimum": 1
(and optionally keep the existing "description" about defaulting to 1) so that
startIndex values below 1 are rejected; locate the "startIndex" property in the
orderedList schema and add the "minimum": 1 keyword.
---
Outside diff comments:
In `@client/src/app/lexicons/page.tsx`:
- Around line 263-275: The confirmation modal is being closed immediately after
calling mutate(), which skips the mutation loading state and hides errors;
update the flow so the modal close happens inside the mutation callbacks
instead: remove or move the immediate modal-close call that runs right after
mutate(), and instead call the modal-close (e.g., clear deletingNsid / call
setDeletingNsid(null) or call the function that hides the dialog) inside the
mutation's onSettled or onSuccess handler (the mutation configured with
onSuccess, onError, onSettled that currently calls setDeletingNsid(null));
ensure setDeletingNsid(null) or equivalent is used to close the modal only after
the mutation settles so the loading/error UI in the modal is preserved.
In `@internal/database/repositories/records.go`:
- Around line 136-164: The upsert currently overwrites an existing cid with an
empty incoming cid; modify the ON CONFLICT DO UPDATE clause in the SQL strings
inside records.go (where sqlStr is built for database.PostgreSQL and default) so
cid is preserved when EXCLUDED/ excluded.cid is an empty string—e.g. set cid =
COALESCE(NULLIF(EXCLUDED.cid, ''), record.cid) for Postgres (and cid =
COALESCE(NULLIF(excluded.cid, ''), record.cid) in the default/SQLite branch)
instead of unconditionally assigning EXCLUDED/ excluded.cid; leave the json and
indexed_at updates as-is.
In `@internal/oauth/middleware_test.go`:
- Around line 333-365: The test TestAuthMiddleware_RequireAuth_DPoP_KeyMismatch
currently ignores errors from GenerateDPoPProof (and earlier key generation) so
a setup failure can produce an empty proof and falsely pass; update the test to
check and fail on errors from GenerateDPoPKeyPair, CalculateJKT and
GenerateDPoPProof (and assert proof is non-empty) before invoking
middleware.RequireAuth so the key-mismatch branch is actually exercised when the
proof is generated with keyPair2.
---
Nitpick comments:
In `@internal/database/repositories/records_test.go`:
- Around line 520-605: The two subtests in
TestRecordsRepository_KeysetPagination_NormalizesIndexedAtFormats should be
converted into a single table-driven test: create a slice of cases containing
fields like name, useForward (or method selector), cursor ([]string),
expectedURIs ([]string) and expectedCount; keep the shared fixture/setup (env,
repo, inserted records) at the top of the test, then loop over cases with
t.Run(case.name, func(t *testing.T){ ... }) and inside call either
repo.GetByCollectionSortedWithKeysetCursor or
repo.GetByCollectionReversedWithKeysetCursor based on the case, assert err,
assert len == expectedCount and assert the returned URIs match case.expectedURIs
in order; ensure the existing cursor values ("2026-01-15T12:00:00Z", ...) and
expected URIs ([n2,n1] and [n3,n2]) are used for the two cases so behavior is
unchanged.
In `@internal/oauth/dpop_test.go`:
- Around line 79-199: The tests only use random keys and don't cover the new
normalization edge cases in ParseDPoPKeyPair()/VerifyDPoPProof(); add
table-driven fixtures (a slice of structs with name, jwk JSON input strings for
x/y/d fields, expected error bool, and expected outputs like presence/absence of
PrivateKey or matching JKT) that include: (1) leading-zeroed x/y, (2) oversized
zero-padded x/y, (3) leading-zero d, (4) zero-padded d, and (5) mismatched d vs
public key; write new table-driven test(s) (e.g., TestDPoPKeyPairNormalization
or extend TestDPoPKeyPairJSON) that iterate the cases, call ParseDPoPKeyPair()
and VerifyDPoPProof() as appropriate, and assert expected errors and
key/public-match behavior using the existing functions GenerateDPoPKeyPair,
ParseDPoPKeyPair, ToJSON/ToPrivateJSON, CalculateJKT, GenerateDPoPProof, and
VerifyDPoPProof.
In `@Makefile`:
- Line 1: Add a conventional default "all" Makefile target to eliminate the
checkmake warning and provide a standard entrypoint: create an "all" phony
target (add "all" to the .PHONY line) that depends on the primary default action
(e.g., "build" or whichever target is intended as the default) so invoking make
with no arguments runs that default; update .PHONY: to include "all" alongside
the existing targets like "build", "run", "test", etc., and ensure the "all"
target simply delegates to the chosen primary target.
In `@testdata/lexicons/org/hypercerts/claim/contributorInformation.json`:
- Around line 13-17: The current "identifier" schema in
contributorInformation.json claims to be a DID-or-URI but only permits a generic
string; change the schema for the "identifier" property to a typed union (oneOf)
that enforces either a URI (use format: "uri" plus maxLength) or a DID (use a
regex pattern like '^did:[a-z0-9]+:.*$' and maxLength). Update the "identifier"
entry (referenced as "identifier" in contributorInformation.json) to use oneOf
with two sub-schemas (URI and DID) and preserve the existing description and
maxLength.
In `@testdata/lexicons/pub/leaflet/blocks/button.json`:
- Around line 9-15: The JSON schema for the button block is missing length
bounds on the "text" and "url" properties; add sensible constraints by setting
"text" to include at least a minLength (e.g., 1) and a small maxLength (e.g.,
128) and setting "url" to include a maxLength (e.g., 2048) while preserving its
"format":"uri"; update the "text" and "url" property definitions (the keys
"text" and "url" in the block schema) to include these "minLength"/"maxLength"
entries to prevent unbounded values.
In `@testdata/lexicons/pub/leaflet/blocks/page.json`:
- Around line 9-10: The "id" property in the JSON schema is unbounded; update
the "id" schema object (the "id" property in page.json) to include a maximum
length constraint, for example add "maxLength": 256 to the id definition to
prevent unbounded payloads and improve ingestion safety; ensure the property
remains type: "string" and pick a sensible maxLength (e.g., 256) consistent with
other IDs in your lexicons.
In `@testdata/lexicons/pub/leaflet/blocks/unorderedList.json`:
- Around line 34-41: Add a maxLength constraint to the "children" array
definition (the nested-list "children" property that references "#listItem") to
prevent unbounded nesting/size; update the schema for "children" to include a
sensible maxLength (or maxItems) value (e.g., a practical limit such as 10–20)
so deeply nested or overly large lists are rejected by the validator.
In `@testdata/lexicons/pub/leaflet/pages/linearDocument.json`:
- Around line 12-19: The "blocks" array schema currently has no upper bound;
update the "blocks" definition in linearDocument.json (the "blocks" property
under the document schema) to include an appropriate maxLength constraint (e.g.,
"maxLength": <reasonable_integer>) so consumers know the practical limit; ensure
the constraint is added alongside the existing "type": "array" and "items":
{"type":"ref","ref":"#block"} entry and pick a sensible value consistent with
other system limits or documentation.
In `@testdata/lexicons/pub/leaflet/richtext/facet.json`:
- Around line 50-59: The link facet's uri property is missing a URL format
constraint; update the "link" object schema so the "uri" property includes
"format": "uri" (matching the atMention.atURI usage) to enforce proper URI
validation while keeping the property as type "string" and required in the
"link" definition.
🪄 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: 73a8a51e-115b-446f-967b-25704f76ccd2
📒 Files selected for processing (69)
.githooks/pre-commit.github/workflows/ci.ymlMakefileREADME.mdclient/.env.exampleclient/src/app/api/oauth/client-metadata.json/route.tsclient/src/app/lexicons/page.tsxclient/src/lib/auth/client.tsclient/src/lib/env.tscmd/hypergoat/main.gogo.modinternal/database/repositories/records.gointernal/database/repositories/records_filter_test.gointernal/database/repositories/records_test.gointernal/graphql/schema/builder.gointernal/graphql/types/object.gointernal/oauth/bridge.gointernal/oauth/dpop.gointernal/oauth/dpop_test.gointernal/oauth/middleware_test.gointernal/server/oauth_handlers.gointernal/tap/consumer.gointernal/tap/consumer_test.gointernal/tap/event.gointernal/tap/event_test.gointernal/tap/handler.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
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
client/src/lib/env.test.ts (1)
26-32: Add a whitespace-only fallback case for client URL.Line 26 currently validates only
"". Add a" "case too, so fallback-to-branch behavior is locked even when env vars are present but whitespace-padded.Suggested test addition
describe("resolvePublicClientURL", () => { it("prefers NEXT_PUBLIC_CLIENT_URL when both are set", () => { expect(resolvePublicClientURL("custom.example.com", "branch.vercel.app")).toBe("https://custom.example.com"); }); it("falls back to branch url when client url is empty", () => { expect(resolvePublicClientURL("", "branch.vercel.app")).toBe("https://branch.vercel.app"); }); + + it("falls back to branch url when client url is whitespace", () => { + expect(resolvePublicClientURL(" ", "branch.vercel.app")).toBe("https://branch.vercel.app"); + }); it("returns empty string when both are empty", () => { expect(resolvePublicClientURL("", "")).toBe(""); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/lib/env.test.ts` around lines 26 - 32, The tests for resolvePublicClientURL need to include a whitespace-only client URL case: add a unit that calls resolvePublicClientURL(" ", "branch.vercel.app") and asserts it returns "https://branch.vercel.app" (and also add a case that resolvePublicClientURL(" ", " ") returns "" if desired), so the function treats whitespace-only client URLs the same as empty strings and falls back to the branch URL; update the test cases in the env.test.ts file around the existing empty-string assertions for resolvePublicClientURL to include these whitespace-only scenarios.Makefile (1)
1-1: Expand.PHONYto include all declared command targets.Currently,
.PHONYlists:help build run test lint clean dev db-migrate db-rollback docker hooks-installHowever, the Makefile defines additional phony targets that should be included:
test-coverage,fmt,db-status,db-create-migration,docker-run,tools,generate. Adding them prevents file-name collisions and follows Makefile best practices. Consider also adding analltarget as a default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, The .PHONY declaration is missing several declared targets; update the .PHONY line to include the additional targets `test-coverage`, `fmt`, `db-status`, `db-create-migration`, `docker-run`, `tools`, and `generate` (and optionally add `all` as a default target) so that these names are treated as phony and cannot collide with files; locate the existing .PHONY entry and append these target names to it (and add a simple `all` target if desired) to follow Makefile best practices.internal/oauth/dpop_test.go (1)
128-130: Consider usingt.Skipinstead oft.Fatalif edge-case key not found.The probability of finding a key with leading-zero coordinate or short private scalar in 10,000 iterations is high but not guaranteed. Using
t.Fatalcould cause flaky test failures in rare cases. Consider usingt.Skipwith a descriptive message to indicate the test was not run due to probabilistic conditions.Suggested change
if kp == nil { - t.Fatal("failed to find key with leading-zero coordinate or short private scalar") + t.Skip("skipping: did not find key with leading-zero coordinate or short private scalar in 10,000 iterations") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/oauth/dpop_test.go` around lines 128 - 130, The test currently calls t.Fatal when kp == nil which makes the test flaky under probabilistic conditions; change this to t.Skipf with a clear message (e.g., "skipping: no key with leading-zero coordinate or short private scalar found in N attempts") so the test is marked skipped rather than failing; update the check around kp in oauth/dpop_test.go (the kp variable and the t.Fatal call) to use t.Skipf and include the number of iterations/seed info in the skip message for debuggability.testdata/lexicons/org/hypercerts/funding/receipt.json (1)
14-29: Close these unions if the variant set is meant to be fixed.AT Protocol unions are open unless
closed: trueis set, and validators are expected to be permissive with unknown future variants. If the goal here is to tightenfrom/toto exactly#text,app.certified.defs#did, andcom.atproto.repo.strongRef, leaving them open weakens that guarantee. (atproto.com)Proposed diff
"from": { "type": "union", + "closed": true, "description": "The sender of the funds (a free-text string, an account DID, or a strong reference to a record). Optional — omit to represent anonymity.", "refs": [ "#text", "app.certified.defs#did", "com.atproto.repo.strongRef" ] }, "to": { "type": "union", + "closed": true, "description": "The recipient of the funds (a free-text string, an account DID, or a strong reference to a record).", "refs": [ "#text", "app.certified.defs#did", "com.atproto.repo.strongRef" ] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testdata/lexicons/org/hypercerts/funding/receipt.json` around lines 14 - 29, The unions for the "from" and "to" fields are currently open by default; if you intend to restrict variants to exactly "#text", "app.certified.defs#did", and "com.atproto.repo.strongRef", add "closed": true to each union definition (the "from" and "to" union nodes) so the lexicon enforces a fixed variant set rather than allowing future unknown variants.testdata/lexicons/org/hypercerts/context/attachment.json (2)
66-73: Union-baseddescriptionmodel is a strong improvementThis broadens interoperability cleanly. Please ensure fixture/tests explicitly cover all three union branches to avoid parser regressions.
🤖 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` around lines 66 - 73, Add tests/fixtures that exercise all three branches of the union-based "description" model: the inline string variant (org.hypercerts.defs#descriptionString), the Leaflet linear document variant (pub.leaflet.pages.linearDocument), and the strong-ref variant (com.atproto.repo.strongRef); update or add unit/integration tests that load the attachment JSON (where "type":"union" and "refs" include those three refs) and assert the parser/validator accepts each branch and correctly identifies its type and content so future parser regressions are caught.
11-11: Consider preventing empty attachment payloadsWith Line 11, records can now be valid with only
title+createdAt. If that’s not intended, add ingest-time validation requiring at least one ofcontent,shortDescription, ordescription.🤖 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, attachment.json currently allows objects with only "title" and "createdAt", which can permit empty attachments; update the JSON Schema in attachment.json to enforce that at least one of the payload fields ("content", "shortDescription", "description") is present by adding an anyOf (or oneOf) constraint referencing those properties (e.g., anyOf with required:["content"], required:["shortDescription"], required:["description"]) alongside the existing required:["title","createdAt"] so records must include title, createdAt, and at least one of the three payload fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/hypergoat/main.go`:
- Around line 329-333: The public /stats handler currently exposes raw sidecar
errors by storing err.Error() into tapInfo (from applyTapSidecarHealth /
tap.AdminClient.Health), which leaks transport/auth details to unauthenticated
callers; change applyTapSidecarHealth and the handler usage so the tapInfo entry
only contains a coarse status string ("ok" or "unreachable") and never the raw
error, and instead log the detailed error server-side (use the existing logger
where bg.tapAdminClient.Health is called or inside applyTapSidecarHealth to
record err). Ensure both the block around bg.tapAdminClient usage and the other
occurrence (lines around applyTapSidecarHealth at 375-377) follow the same
pattern so stats["tap"] only contains non-sensitive statuses.
In `@testdata/lexicons/org/hypercerts/funding/receipt.json`:
- Around line 79-89: The "text" schema currently reads like a free-text string
but actually defines an object with a required "value" property; update the
description for "text" (and mention "#text" usages) to explicitly state this is
a wrapper object and that callers must wrap free text as an object with a
required "value" field (e.g., use a "#text" object containing "value"), and
clarify the meaning of "value" in the "properties.value" description so
consumers know to pass the raw string inside that wrapper.
- Around line 31-35: The "amount" schema entry currently uses type "string" with
maxLength and a description claiming a numeric value; change the "amount" field
in receipt.json to use the appropriate numeric primitive (either "number" for
decimals or "integer" for whole units) instead of "string", remove
string-specific keywords like "maxLength", and add numeric constraints (e.g.,
"minimum"/"maximum" and optional "exclusiveMinimum"/"exclusiveMaximum") as
appropriate; also update the "description" for the "amount" field to reflect the
numeric type (e.g., "Amount of funding received as a number (e.g., 1000.50)") or
explicitly document if parsing will be enforced at the application layer.
---
Nitpick comments:
In `@client/src/lib/env.test.ts`:
- Around line 26-32: The tests for resolvePublicClientURL need to include a
whitespace-only client URL case: add a unit that calls resolvePublicClientURL("
", "branch.vercel.app") and asserts it returns "https://branch.vercel.app" (and
also add a case that resolvePublicClientURL(" ", " ") returns "" if
desired), so the function treats whitespace-only client URLs the same as empty
strings and falls back to the branch URL; update the test cases in the
env.test.ts file around the existing empty-string assertions for
resolvePublicClientURL to include these whitespace-only scenarios.
In `@internal/oauth/dpop_test.go`:
- Around line 128-130: The test currently calls t.Fatal when kp == nil which
makes the test flaky under probabilistic conditions; change this to t.Skipf with
a clear message (e.g., "skipping: no key with leading-zero coordinate or short
private scalar found in N attempts") so the test is marked skipped rather than
failing; update the check around kp in oauth/dpop_test.go (the kp variable and
the t.Fatal call) to use t.Skipf and include the number of iterations/seed info
in the skip message for debuggability.
In `@Makefile`:
- Line 1: The .PHONY declaration is missing several declared targets; update the
.PHONY line to include the additional targets `test-coverage`, `fmt`,
`db-status`, `db-create-migration`, `docker-run`, `tools`, and `generate` (and
optionally add `all` as a default target) so that these names are treated as
phony and cannot collide with files; locate the existing .PHONY entry and append
these target names to it (and add a simple `all` target if desired) to follow
Makefile best practices.
In `@testdata/lexicons/org/hypercerts/context/attachment.json`:
- Around line 66-73: Add tests/fixtures that exercise all three branches of the
union-based "description" model: the inline string variant
(org.hypercerts.defs#descriptionString), the Leaflet linear document variant
(pub.leaflet.pages.linearDocument), and the strong-ref variant
(com.atproto.repo.strongRef); update or add unit/integration tests that load the
attachment JSON (where "type":"union" and "refs" include those three refs) and
assert the parser/validator accepts each branch and correctly identifies its
type and content so future parser regressions are caught.
- Line 11: attachment.json currently allows objects with only "title" and
"createdAt", which can permit empty attachments; update the JSON Schema in
attachment.json to enforce that at least one of the payload fields ("content",
"shortDescription", "description") is present by adding an anyOf (or oneOf)
constraint referencing those properties (e.g., anyOf with required:["content"],
required:["shortDescription"], required:["description"]) alongside the existing
required:["title","createdAt"] so records must include title, createdAt, and at
least one of the three payload fields.
In `@testdata/lexicons/org/hypercerts/funding/receipt.json`:
- Around line 14-29: The unions for the "from" and "to" fields are currently
open by default; if you intend to restrict variants to exactly "#text",
"app.certified.defs#did", and "com.atproto.repo.strongRef", add "closed": true
to each union definition (the "from" and "to" union nodes) so the lexicon
enforces a fixed variant set rather than allowing future unknown variants.
🪄 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: 89c41861-6e61-483e-908b-a5ae101b2092
⛔ Files ignored due to path filters (1)
client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (73)
.githooks/pre-commit.github/workflows/ci.ymlMakefileREADME.mdclient/.env.exampleclient/package.jsonclient/src/app/api/oauth/client-metadata.json/route.tsclient/src/app/lexicons/page.tsxclient/src/lib/auth/client.tsclient/src/lib/env.test.tsclient/src/lib/env.tscmd/hypergoat/main.gocmd/hypergoat/main_test.gogo.modinternal/database/repositories/records.gointernal/database/repositories/records_filter_test.gointernal/database/repositories/records_test.gointernal/graphql/schema/builder.gointernal/graphql/types/object.gointernal/oauth/bridge.gointernal/oauth/dpop.gointernal/oauth/dpop_test.gointernal/oauth/middleware_test.gointernal/server/oauth_handlers.gointernal/server/oauth_handlers_test.gointernal/tap/consumer.gointernal/tap/consumer_test.gointernal/tap/event.gointernal/tap/event_test.gointernal/tap/handler.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
✅ Files skipped from review due to trivial changes (34)
- client/package.json
- railway.toml
- go.mod
- internal/graphql/types/object.go
- testdata/lexicons/pub/leaflet/blocks/horizontalRule.json
- testdata/lexicons/pub/leaflet/blocks/math.json
- README.md
- testdata/lexicons/pub/leaflet/blocks/code.json
- testdata/lexicons/pub/leaflet/blocks/website.json
- .github/workflows/ci.yml
- testdata/lexicons/pub/leaflet/blocks/blockquote.json
- testdata/lexicons/app/certified/defs.json
- testdata/lexicons/pub/leaflet/blocks/iframe.json
- testdata/lexicons/pub/leaflet/blocks/bskyPost.json
- testdata/lexicons/pub/leaflet/blocks/text.json
- testdata/lexicons/org/hypercerts/workscope/cel.json
- testdata/lexicons/pub/leaflet/blocks/page.json
- testdata/lexicons/app/certified/badge/award.json
- testdata/lexicons/app/certified/location.json
- testdata/lexicons/org/hypercerts/workscope/tag.json
- testdata/lexicons/app/certified/badge/response.json
- testdata/lexicons/app/certified/actor/organization.json
- testdata/lexicons/org/hypercerts/claim/contributorInformation.json
- testdata/lexicons/pub/leaflet/blocks/image.json
- testdata/lexicons/pub/leaflet/blocks/header.json
- testdata/lexicons/pub/leaflet/blocks/unorderedList.json
- testdata/lexicons/org/hypercerts/context/acknowledgement.json
- testdata/lexicons/app/certified/link/evm.json
- testdata/lexicons/pub/leaflet/pages/linearDocument.json
- testdata/lexicons/app/bsky/richtext/facet.json
- testdata/lexicons/org/hyperboards/board.json
- testdata/lexicons/app/certified/actor/profile.json
- testdata/lexicons/org/hyperboards/displayProfile.json
- testdata/lexicons/pub/leaflet/richtext/facet.json
🚧 Files skipped from review as they are similar to previous changes (28)
- client/src/app/api/oauth/client-metadata.json/route.ts
- internal/graphql/schema/builder.go
- client/src/lib/auth/client.ts
- internal/tap/event.go
- internal/tap/handler.go
- internal/database/repositories/records_filter_test.go
- internal/tap/event_test.go
- internal/oauth/bridge.go
- testdata/lexicons/pub/leaflet/blocks/poll.json
- client/src/app/lexicons/page.tsx
- testdata/lexicons/org/hypercerts/claim/contribution.json
- .githooks/pre-commit
- internal/tap/consumer_test.go
- testdata/lexicons/pub/leaflet/blocks/button.json
- testdata/lexicons/org/hypercerts/defs.json
- testdata/lexicons/org/hypercerts/claim/rights.json
- testdata/lexicons/org/hypercerts/context/evaluation.json
- testdata/lexicons/org/hypercerts/collection.json
- testdata/lexicons/pub/leaflet/blocks/orderedList.json
- client/src/lib/env.ts
- testdata/lexicons/app/certified/badge/definition.json
- internal/tap/consumer.go
- internal/database/repositories/records.go
- internal/database/repositories/records_test.go
- internal/oauth/middleware_test.go
- testdata/lexicons/org/hypercerts/context/measurement.json
- testdata/lexicons/org/hypercerts/claim/activity.json
- client/.env.example
Comment on lines
31
to
35
| "amount": { | ||
| "type": "string", | ||
| "description": "Amount of funding received." | ||
| "description": "Amount of funding received as a numeric string (e.g. '1000.50').", | ||
| "maxLength": 50 | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n testdata/lexicons/org/hypercerts/funding/receipt.json | head -50Repository: GainForest/hyperindex
Length of output: 2392
🏁 Script executed:
# Search for documentation about AT Protocol Lexicon numeric validation
web_searchRepository: GainForest/hyperindex
Length of output: 115
🏁 Script executed:
# Check if there are any other constraints or if this is part of a larger schema
cat testdata/lexicons/org/hypercerts/funding/receipt.jsonRepository: GainForest/hyperindex
Length of output: 3510
🌐 Web query:
AT Protocol Lexicon string type validation numeric format pattern
💡 Result:
The AT Protocol Lexicon defines string types with optional 'format' constraints for validation. There is no specific "numeric" format type. Numeric values use dedicated primitive types: 'integer' (signed 64-bit, recommended 53-bit safe for JS compatibility) and 'number' (float). String fields representing numeric-like data (e.g., CIDs, TIDs) use formats with alphanumeric patterns, but none are purely numeric. Supported string formats (from official specs and SDK implementations) include: - cid: Min 8 chars, alphanumeric + '+' only (e.g., bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi) - tid: Exactly 13 chars, lowercase letters + digits 2-7, first byte high bit 0 - handle: Domain name (alphanumeric + '-', 2+ dot-separated segments, 1-63 chars/segment, max 253 total) - nsid: 3+ dot segments (reverse domain + name, letters only in last) - record-key (rkey): 1-512 chars, alphanumeric + . _ : ~ - - datetime: RFC 3339/ISO 8601 with uppercase T, seconds, timezone (Z/±HH:MM), no -00:00 or whitespace - did: did:method:identifier - at-uri: at:// + handle/DID + optional /collection/rkey - uri: RFC 3986 compliant - at-identifier: DID or handle - language: BCP 47 tag String constraints: minLength/maxLength (UTF-8 bytes), minGraphemes/maxGraphemes (Unicode graphemes). Validation is per Lexicon schema; SDKs provide regex/pattern checks for formats. No explicit regex patterns in core specs; implement via SDK validators or custom regex matching the rules above. For numeric strings, use 'integer'/'number' primitives instead, with minimum/maximum constraints.
Citations:
- 1: https://atproto.blue/en/latest/atproto_client/string_formats.html
- 2: https://atproto.com/specs/lexicon
- 3: https://mintlify.com/bluesky-social/atproto/concepts/lexicons
- 4: https://atproto.com/specs/data-model
- 5: https://atproto.blue/en/latest/atproto/atproto_client.models.string_formats.html
amount field should use integer or number type instead of string.
The description claims "numeric string" but the schema only validates maxLength, allowing any string like "ten dollars" to pass. AT Protocol Lexicon has no numeric format constraint for strings; it supports dedicated integer and number types with minimum/maximum validation instead. Use the appropriate primitive type or explicitly document that numeric parsing is enforced at the application layer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testdata/lexicons/org/hypercerts/funding/receipt.json` around lines 31 - 35,
The "amount" schema entry currently uses type "string" with maxLength and a
description claiming a numeric value; change the "amount" field in receipt.json
to use the appropriate numeric primitive (either "number" for decimals or
"integer" for whole units) instead of "string", remove string-specific keywords
like "maxLength", and add numeric constraints (e.g., "minimum"/"maximum" and
optional "exclusiveMinimum"/"exclusiveMaximum") as appropriate; also update the
"description" for the "amount" field to reflect the numeric type (e.g., "Amount
of funding received as a number (e.g., 1000.50)") or explicitly document if
parsing will be enforced at the application layer.
Comment on lines
+79
to
+89
| "text": { | ||
| "type": "object", | ||
| "description": "A free-text string value (e.g. a display name, wallet address, or other identifier).", | ||
| "required": ["value"], | ||
| "properties": { | ||
| "value": { | ||
| "type": "string", | ||
| "description": "The string value.", | ||
| "maxLength": 2048 | ||
| } | ||
| } |
There was a problem hiding this comment.
Clarify that #text is a wrapper object, not a raw string.
This definition serializes as an object with a required value field, so describing the from/to variant as a “free-text string” is easy to read as a bare JSON string. I’d make the field descriptions explicit that callers must wrap free text in #text.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testdata/lexicons/org/hypercerts/funding/receipt.json` around lines 79 - 89,
The "text" schema currently reads like a free-text string but actually defines
an object with a required "value" property; update the description for "text"
(and mention "#text" usages) to explicitly state this is a wrapper object and
that callers must wrap free text as an object with a required "value" field
(e.g., use a "#text" object containing "value"), and clarify the meaning of
"value" in the "properties.value" description so consumers know to pass the raw
string inside that wrapper.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
indexed_atnormalization for SQLite) and add tests for cursor behavior.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests