[JavierCarnelli-ConductorOne]

Description

• Bug fix
• New feature

Adds incremental sync support through the baton event feed (ListEvents) and targeted resource sync (Get).

• Resource changes: a resource type's event_feed query emits ResourceChangeEvents, and C1 re-syncs those resources through the resource type's get query.
• Grants and revokes: a grants block's event_feed (changes_query, plus an optional revokes_query) emits CreateGrantEvent/CreateRevokeEvent. Grants keep expandable annotations.
• Progress tracking: each source tracks its own timestamp cursor. Each poll re-reads an overlap window (default 60s) so rows with tied or late-committed timestamps are not missed; re-sent events keep the same ID, so C1 skips them.
• Pagination: event-feed queries must set pagination with strategy: offset and use ?<Limit>/?<Offset>. They must ORDER BY <cursor column>, <primary key>.
• Get returns Unimplemented when a resource type has no get query.

See the "Event Feed" section in the README and examples/ for configuration.

Breaking changes vs. earlier revisions of this PR

• The incremental_sync config keys were renamed to event_feed.
• The ?<id> placeholder in get queries was renamed to ?<resource_id>.
• Event-feed queries now require pagination (offset only) and ?<Limit>.

Notes

• C1 only processes revoke events when the tenant has the incremental revocations feature flag enabled.
• Hard-deleted rows cannot be detected. Sources need soft-delete or audit columns to emit revokes.
• Follow-up: move event-feed queries into a standalone section so a single feed can cover several resource types.

Useful links:

• Baton SDK coding guidelines
• New contributor guide

🤖 Generated with Claude Code

[linear]

CXP-481 Incremental Sync for Baton SQL

[github-actions]

Claude finished @JavierCarnelli-ConductorOne's task in 4m 19s —— View job

---

PR Review: Incremental Sync Support

• Gather context and understand changes
• Review core implementation (event_feed.go, event_pagination.go)
• Review config and validation changes
• Review connector integration
• Review example configurations
• Post detailed review

---

Overview

This PR adds incremental sync (event feed) support to baton-sql, enabling efficient change detection via timestamp-based queries instead of full re-syncs. The implementation supports three event types:

• Resource changes — detects new/modified resources via updated_at-style columns
• Grant changes — detects new grants
• Grant revokes — detects soft-deleted grants (optional)

The architecture is well-designed: each "source" (resource type or grant query) maintains its own independent cursor, and a round-robin scheduler processes one page per source per ListEvents call. The cursor design correctly separates committed state (SourceCursors) from in-flight state (CurrentSince, CurrentMaxSeen), which ensures crash safety.

What works well

• Cursor design — The two-tier cursor (SourceCursors for committed state, CurrentSince/CurrentMaxSeen for in-flight) is clean. The since timestamp stays constant across all pages of a scan cycle, preventing missed or duplicated events.
• Deterministic source ordering — getSources sorts resource type IDs lexicographically, ensuring stable cursor semantics across restarts.
• Validation is thorough — Required placeholders (?<since>, ?<id>) are checked at startup, reserved var names are blocked, and get is required when incremental_sync is configured on a resource type.
• Reuse of existing runQuery — The incremental sync queries reuse the same query execution and pagination infrastructure as full sync, avoiding code duplication.
• grantRowKey for event ID uniqueness — Good addition in the fix commit to handle cases where multiple grant rows share the same (resource, principal, timestamp) triple.
• SDK interface compliance — SQLEventFeed correctly implements connectorbuilder.EventFeed, and userSyncer inherits Get() via embedding.
• Comprehensive examples — All database-specific example files were updated with working incremental sync configurations.

Issues found

Must-fix

1. No tests for new code — event_feed.go (429 lines) and event_pagination.go (127 lines) have zero test coverage. The cursor state machine (commitAndAdvance), source enumeration (getSources), and time parsing (toTime) are all testable in isolation without a database. The validation additions in validate.go also lack test cases. Given the complexity of the multi-source pagination state machine, unit tests are essential. Add tests →

Should-fix

2. %v in grantRowKey risks scientific notation (event_feed.go:405) — Per CLAUDE.md, %v with large numeric IDs can produce 1.23e+15. Use a type switch for numeric types. See inline comment.

3. Implicit coupling between incremental sync query and list mapping (event_feed.go:215) — processResourceChangePage evaluates s.config.List.Map.Id against rows from the incremental sync query. If the user's incremental query doesn't return the columns referenced by the list mapping's id expression, they get a runtime CEL error with no startup validation. See inline comment.

4. earliestEvent parameter is silently ignored (event_feed.go:56) — The design decision is sound (per-source cursors shouldn't be overridden), but the unused parameter needs a comment explaining why. See inline comment.

Nits / suggestions

5. EventFeeds re-creates syncers on each call (connector.go:49-60) — GetSQLSyncers allocates new instances each time. The SDK calls this during init, so it's not a hot path, but caching would be cleaner. See inline comment.

6. getSources rebuilds on every ListEvents call (event_feed.go:64) — The source list is derived from static config and doesn't change. Could be precomputed in NewSQLEventFeed.

7. SAP HANA example uses unusual indentation — sap-hana-test.yml has all top-level keys indented 2 spaces under ---. This is valid YAML but inconsistent with all other example files. This was a pre-existing issue, not introduced by this PR.

Summary

The core design is solid — the multi-source cursor state machine, the separation of committed vs in-flight state, and the reuse of existing query infrastructure are all well-thought-out. The main gap is test coverage: the cursor state machine and validation logic have enough complexity to warrant dedicated unit tests before merging. The %v scientific notation issue and the implicit list-mapping coupling are worth addressing as well.

[github-actions]

Claude finished @JavierCarnelli-ConductorOne's task in 2m 8s —— View job

---

PR Review: Incremental Sync Support

• Gather context and understand changes
• Review core implementation (event_feed.go, event_pagination.go)
• Review configuration and validation
• Review connector integration and examples
• Post detailed review

---

Overview

This PR adds incremental sync (event feed) support to baton-sql. Instead of full re-syncs, the connector can now detect changes via timestamp-based SQL queries and emit ResourceChangeEvent, CreateGrantEvent, and CreateRevokeEvent. The implementation supports three source kinds per resource type — resource changes, grant changes, and grant revokes — each with its own independent cursor.

Architecture Assessment

The design is well-structured:

• Two-tier cursor model (event_pagination.go:17-25) — SourceCursors holds committed state; CurrentSince/CurrentMaxSeen hold in-flight state. This correctly prevents skipping events on crash/restart.
• Round-robin scheduling — One page per source per ListEvents call (event_feed.go:73-77), advancing to the next source after exhaustion (event_feed.go:158-170).
• Deterministic source ordering (event_pagination.go:72-77) — Sorting resource type IDs lexicographically ensures cursor semantics are stable across restarts.
• Reuse of runQuery — The incremental queries share pagination and query infrastructure with full sync, reducing code duplication.
• Thorough startup validation — ?<since> placeholder required, reserved var names blocked, get required when incremental_sync is configured, CEL expressions compiled at startup.

Issues

1. No tests for new code (must-fix)

event_feed.go (445 lines) and event_pagination.go (127 lines) have zero test coverage. The cursor state machine, source enumeration, and time parsing are testable without a database. Given the existing test files in pkg/bsql/ (validate_test.go, query_test.go, helpers_test.go), this codebase clearly values tests — these new files should follow suit.

Key areas to test:

• marshalCursor/unmarshalCursor round-trip and edge cases (empty string, nil SourceCursors)
• getSources deterministic ordering with multiple resource types
• commitAndAdvance state transitions (mid-page vs. source exhaustion vs. full cycle)
• toTime with time.Time, string, []byte, and unsupported types
• defaultLookback with valid duration, invalid duration, and nil config
• grantRowKey with int64, float64, missing key, nil pagination

The validation additions in validate.go (lines 177-270) also lack test cases — the existing validate_test.go only covers ListQuery.

Add tests →

2. Potential data loss: toTime silently falls back to since on parse failure (event_feed.go:229-237, 293-301)

When the cursor column value can't be parsed by toTime, the code silently falls back to the since timestamp as the rowTimestamp:

rowTimestamp := since
if ts, ok := rowMap[rc.CursorColumn]; ok {
    if t, parseErr := toTime(ts); parseErr == nil {
        rowTimestamp = t
        ...
    }
}

If toTime fails (e.g., unexpected format from a database driver), maxSeen never advances, so the next cycle will re-query from the same since — creating an infinite loop of duplicate events. This is subtle and hard to debug. Consider logging a warning on parse failure, or returning an error.

3. defaultLookback silently swallows parse errors (event_pagination.go:119-127)

func defaultLookback(config *IncrementalSyncConfig) time.Duration {
    if config != nil && config.DefaultLookback != "" {
        d, err := time.ParseDuration(config.DefaultLookback)
        if err == nil {
            return d
        }
    }
    return defaultLookbackDuration
}

Startup validation in IncrementalSyncConfig.staticValidate catches bad durations, so this path should only fire with valid values. But if it does fail at runtime (e.g., config modified post-validation), silently defaulting to 3h could be surprising. A debug log would help.

4. getSources rebuilds on every ListEvents call (event_feed.go:68)

The source list is derived from static config and can't change at runtime. Building it in NewSQLEventFeed and storing it on the struct would avoid repeated allocation on every call. Not a correctness issue, but it's a simple optimization.

5. Event ID format asymmetry between resource and grant events

Resource event IDs use the format resource:type:id:timestamp (event_feed.go:240) while grant/revoke event IDs include an extra rowKey disambiguator: grant:type:resourceID:principalID:timestamp:rowKey (event_feed.go:329).

Resource events rely on (resourceType, resourceID, timestamp) being unique. If a bulk update sets the same updated_at for multiple distinct resources, the event IDs would technically still be unique (different resourceID), but if two rows with different data map to the same resource ID (unlikely but possible with complex CEL expressions), they'd collide. This is more of a documentation point than a bug.

6. Connector.EventFeeds swallows syncer creation errors (connector.go:57-60)

syncers, err := c.config.GetSQLSyncers(ctx, c.db, c.dbEngine, c.celEnv)
if err != nil {
    l.Debug("baton-sql: failed to create syncers for event feed...", zap.Error(err))
    return nil
}

A debug-level log for a potentially significant failure (incremental sync silently becomes unavailable) is easy to miss. Consider l.Warn(...) so it's visible in normal operation.

Validation review

The validation changes are solid:

• GetQuery.staticValidate (validate.go:177-194) — Correctly requires ?<id> placeholder and blocks reserved var names.
• ResourceIncrementalSync.staticValidate (validate.go:196-221) — Requires cursor_column, validates ?<since> placeholder, blocks reserved vars, and compiles optional resource_id CEL expression at startup.
• GrantsIncrementalSync.staticValidate (validate.go:223-261) — Requires resource_id and changes_cursor_column, validates ?<since> in both changes and revokes queries, enforces revokes_cursor_column when revokes_query is set.
• IncrementalSyncConfig.staticValidate (validate.go:263-270) — Validates default_lookback as a Go duration.
• Cross-check in sql_syncer.go:183-185 — Correctly requires get when incremental_sync is configured.

One gap: as noted in the previous review, there's no startup validation that the incremental sync query returns the columns referenced by List.Map.Id (when ResourceId is not explicitly set). The resource_id field on ResourceIncrementalSync (config.go:495) was added to address this — users can explicitly set it to decouple the incremental query from the list mapping. The fallback to List.Map.Id (event_feed.go:217-220) is documented but could catch users off-guard.

Config and examples

• The ResourceIncrementalSync.ResourceId field (config.go:492-495) is a good addition — it cleanly solves the coupling issue between incremental queries and list mappings.
• example.yml is well-documented with inline comments explaining each section.
• All database-specific examples updated consistently.

Minor observations

• The earliestEvent parameter comment (event_feed.go:55-57) is clear and explains the design decision well. Good improvement from the earlier commit.
• grantRowKey (event_feed.go:406-423) correctly handles int64 and float64 to avoid scientific notation, addressing the CLAUDE.md guideline. The %v fallback for other types is fine since non-numeric types won't produce scientific notation.
• The sinceKey and idKey constants (query.go:29-32) with their doc comment properly establish the reservation.

Summary

The architecture is sound — the cursor state machine, the source scheduling, and the config validation are all well-designed. The main gap is test coverage: the new code has enough state machine complexity to warrant dedicated unit tests before merging. The silent toTime fallback (issue #2) is worth a second look since it could mask cursor advancement failures. The rest are low-severity improvements.

[github-actions]

Claude finished @JavierCarnelli-ConductorOne's task in 2m 59s —— View job

---

PR Review: Incremental Sync Support

• Gather context and understand changes
• Review core implementation files
• Review configuration and validation
• Review connector integration and examples
• Post detailed review

---

Overview

This PR adds incremental sync (event feed) support to baton-sql. Instead of full re-syncs, the connector can now detect changes via timestamp-based SQL queries and emit ResourceChangeEvent, CreateGrantEvent, and CreateRevokeEvent. The implementation supports three source kinds per resource type — resource changes, grant changes, and grant revokes — each with its own independent cursor.

Also adds Get() (targeted resource fetch by ID), required when incremental sync is configured.

Architecture

The design is solid:

• Two-tier cursor model (event_pagination.go:17-25) — SourceCursors holds committed state; CurrentSince/CurrentMaxSeen hold in-flight state. This prevents skipping events on crash/restart.
• Round-robin scheduling — One page per source per ListEvents call, advancing to the next source after exhaustion.
• Deterministic source ordering (event_pagination.go:72-77) — Sorts resource type IDs lexicographically, ensuring stable cursor semantics across restarts.
• Reuse of runQuery — Incremental queries share pagination and query infrastructure with full sync.
• Thorough startup validation — ?<since> placeholder required, reserved var names blocked, get required when incremental_sync is configured, optional CEL expressions compiled at startup.

The latest commit (28c9e4e) correctly addressed the toTime silent fallback — parse failures are now propagated as errors rather than silently falling back to since, which prevents the infinite-reprocessing loop flagged in earlier reviews.

Issues

1. GrantsIncrementalSync.resource_id CEL expression not compiled at startup (bug)

ResourceIncrementalSync.staticValidate compiles the optional resource_id CEL expression at startup (validate.go:215-218), catching typos early. But GrantsIncrementalSync.staticValidate (validate.go:223-261) only checks that resource_id is non-empty — it never calls s.env.Compile(). A bad CEL expression in grant incremental sync's resource_id would only fail at runtime during processGrantPage (event_feed.go:309).

// validate.go:223 — missing Compile call for GrantsIncrementalSync.ResourceId
func (l *GrantsIncrementalSync) staticValidate(ctx context.Context, s *SQLSyncer) error {
    if l.ResourceId == "" {
        return errors.New("incremental_sync.resource_id is required")
    }
    // Should add: if err := s.env.Compile(l.ResourceId); err != nil { ... }

Fix this →

2. No tests for new code (must-fix, carried over)

event_feed.go (450 lines) and event_pagination.go (127 lines) have zero test coverage. This was flagged in both previous reviews and remains unaddressed. The cursor state machine has enough complexity that unit tests are essential.

Testable in isolation (no database needed):

• marshalCursor/unmarshalCursor round-trip and edge cases
• getSources deterministic ordering with multiple resource types
• commitAndAdvance state transitions (mid-page, source exhaustion, full cycle wrap)
• toTime with time.Time, string, []byte, and unsupported types
• defaultLookback with valid/invalid/nil config
• grantRowKey with int64, float64, missing key, nil pagination

The validation additions (validate.go:177-270) also lack test cases — the existing validate_test.go only covers ListQuery.

Add tests →

3. EventFeeds error logging level too low (connector.go:59)

If GetSQLSyncers fails inside EventFeeds, incremental sync silently becomes unavailable with only a Debug-level log. This is a user-facing degradation that would be very hard to troubleshoot. Consider Warn level.

// connector.go:59 — Debug level hides a significant failure
l.Debug("baton-sql: failed to create syncers for event feed...")
// Should be:
l.Warn("baton-sql: failed to create syncers for event feed...")

4. getSources rebuilds on every ListEvents call (event_feed.go:68)

The source list is derived from static config and can't change at runtime. It could be precomputed in NewSQLEventFeed and stored on the struct. Not a correctness issue but a simple optimization that removes repeated allocation and sorting on every call.

Good changes since earlier reviews

• toTime parse failures now propagated as errors (28c9e4e) — Fixes the silent fallback that could cause infinite reprocessing loops. This was the most impactful issue from prior reviews.
• grantRowKey handles numeric types — Correctly uses strconv.FormatInt for int64/float64 to avoid scientific notation per CLAUDE.md guidelines.
• ResourceId field on ResourceIncrementalSync (config.go:492-495) — Decouples incremental queries from list mappings, addressing the implicit coupling concern.
• earliestEvent comment (event_feed.go:54-57) — Clear explanation of the intentional design decision.
• EventFeeds statelessness comment (connector.go:48-51) — Documents why re-creating syncers is safe.

Validation review

The validation is thorough and well-structured:

• GetQuery.staticValidate — Requires ?<id> placeholder, blocks reserved var names
• ResourceIncrementalSync.staticValidate — Requires cursor_column, validates ?<since>, blocks reserved vars, compiles optional resource_id CEL
• GrantsIncrementalSync.staticValidate — Requires resource_id and changes_cursor_column, validates ?<since> in both queries, enforces revokes_cursor_column when revokes_query is set
• Cross-check in sql_syncer.go:182-184 — Correctly requires get when incremental_sync is configured

The only gap is issue #1 above (missing CEL compile for grants resource_id).

Summary

The core architecture is well-designed — the cursor state machine, crash safety, deterministic ordering, and reuse of existing infrastructure are all solid. The latest commit addressed the most critical prior review issue (silent toTime fallback). Two items remain before this is merge-ready: add the missing CEL compile check for grants resource_id (issue #1) and add unit tests (issue #2). The log level and getSources caching (issues #3-4) are nice-to-haves.

[github-actions]

Superseded — see the current review report for commit 15cbd401b839

Connector PR Review: CXH-1390 CXP-481 Support for Incremental Sync

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base a934ace6a209.
Review mode: full
View review run

Review Summary

This PR adds SQLEventFeed.ListEvents and SQLSyncer.Get. ListEvents walks per-resource-type and per-grants-block sources in a fixed order. Each source keeps its own committed timestamp cursor, and each poll re-reads an overlap window. Event-feed queries require offset pagination. Get returns Unimplemented when no get query is configured and NotFound when no row matches. The PR also adds validation for these queries, extracts grantOptions so full sync and the event feed share it, rejects ?<Limit>/?<Offset>/?<Cursor> when no pagination is configured, and caches the resource syncers in the connector.

I scanned the full diff (21 files: Go sources, tests, README, examples) for security and correctness. No changes to go.mod/go.sum and no vendored paths.

I applied the trusted repo-local criteria. The S/R7 data-loss rules don't apply: no List/Entitlements/Grants path changed, the feed returns every query or CEL error instead of swallowing it, and a Get with no matching row returns NotFound. The L rules (logging): the new Error logs in connector.go are config/init failures that return nil, which is acceptable. The E rules (error wrapping): Unimplemented and NotFound are used correctly. The T rules (tracing) don't apply because the PR creates no spans, and the P rules (provisioning) don't apply because no Grant/Revoke code changed.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (medium confidence) pkg/bsql/event_feed.go:233 and :303: if a row lacks the configured cursor column, rowTimestamp silently falls back to since and maxSeen never advances. The source's cursor is never committed, so every poll re-scans only now - default_lookback, and any change older than the lookback is missed after downtime. No test covers this case.
• New (medium confidence) pkg/bsql/validate.go:284: offset pagination over a timestamp-ordered query with a fixed since can skip a row at a page boundary. This happens when a row from an earlier page is updated mid-cycle and moves to the end of the result set. If the skipped row's timestamp is older than maxSeen - overlap, the next cycle won't re-read it either, and the change is missed until the next full sync.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/bsql/event_feed.go`:
- Around line 232-243 (resource feed) and 302-313 (grant feed): when `rowMap` does not contain the configured cursor column, the code silently uses `since` as the row timestamp and never advances `maxSeen`, so the source cursor is never committed. Return an error such as `baton-sql: event feed query did not return cursor column %q` when the key is missing. Add a test that runs a feed query that omits the cursor column and asserts the error.

In `pkg/bsql/validate.go`:
- Around line 284 (validateEventFeedPagination): offset pagination over `ORDER BY <cursor_column>, <pk>` with a fixed `since` can skip a row when an earlier row is updated between pages, because every later row shifts back one position. Either implement keyset pagination on (cursor_column, primary_key), e.g. `WHERE (col > ?<last_ts>) OR (col = ?<last_ts> AND pk > ?<last_pk>)`, or document in the README Event Feed section that rows updated mid-cycle can cause misses that only the next full sync corrects.

[github-actions]

🟡 Suggestion (medium confidence): Offset pagination over ORDER BY updated_at, id with a fixed since can skip rows. If a row already returned on page k is updated before page k+1 is fetched, it moves to the end of the result set. Every later row shifts back one position, so the row at the page boundary is never returned. If that row's timestamp is older than maxSeen - overlap, the next cycle won't re-read it either, and the change is missed until the next full sync. A keyset cursor on (cursor_column, primary_key) would close this gap. Otherwise, document the limitation.

[github-actions]

Superseded — see the current review report for commit 5b6273940454

Connector PR Review: CXH-1390 CXP-481 Support for Incremental Sync

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base a934ace6a209.
Review mode: full
View review run

Review Summary

This PR adds SQLEventFeed.ListEvents and SQLSyncer.Get. ListEvents walks per-resource-type and per-grants-block sources in a fixed order. Each source keeps its own committed timestamp cursor, and each poll re-reads an overlap window. Event-feed queries require offset pagination. Get returns Unimplemented when no get query is configured and NotFound when no row matches. The PR also adds validation for these queries, extracts grantOptions so full sync and the event feed share it, rejects ?<Limit>/?<Offset>/?<Cursor> when no pagination is configured, and caches the resource syncers in the connector.

I scanned the full diff (21 files: Go sources, tests, README, examples) for security and correctness. No changes to go.mod/go.sum and no vendored paths.

I applied the trusted repo-local criteria. The S/R7 data-loss rules don't apply: no List/Entitlements/Grants path changed, the feed returns every query or CEL error instead of swallowing it, and a Get with no matching row returns NotFound. The L rules (logging): the new Error logs in connector.go are config/init failures that return nil, which is acceptable. The E rules (error wrapping): Unimplemented and NotFound are used correctly. The T rules (tracing) don't apply because the PR creates no spans, and the P rules (provisioning) don't apply because no Grant/Revoke code changed.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (medium confidence) pkg/bsql/event_feed.go:233 and :303: if a row lacks the configured cursor column, rowTimestamp silently falls back to since and maxSeen never advances. The source's cursor is never committed, so every poll re-scans only now - default_lookback, and any change older than the lookback is missed after downtime. No test covers this case.
• New (medium confidence) pkg/bsql/validate.go:284: offset pagination over a timestamp-ordered query with a fixed since can skip a row at a page boundary. This happens when a row from an earlier page is updated mid-cycle and moves to the end of the result set. If the skipped row's timestamp is older than maxSeen - overlap, the next cycle won't re-read it either, and the change is missed until the next full sync.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/bsql/event_feed.go`:
- Around line 232-243 (resource feed) and 302-313 (grant feed): when `rowMap` does not contain the configured cursor column, the code silently uses `since` as the row timestamp and never advances `maxSeen`, so the source cursor is never committed. Return an error such as `baton-sql: event feed query did not return cursor column %q` when the key is missing. Add a test that runs a feed query that omits the cursor column and asserts the error.

In `pkg/bsql/validate.go`:
- Around line 284 (validateEventFeedPagination): offset pagination over `ORDER BY <cursor_column>, <pk>` with a fixed `since` can skip a row when an earlier row is updated between pages, because every later row shifts back one position. Either implement keyset pagination on (cursor_column, primary_key), e.g. `WHERE (col > ?<last_ts>) OR (col = ?<last_ts> AND pk > ?<last_pk>)`, or document in the README Event Feed section that rows updated mid-cycle can cause misses that only the next full sync corrects.

---

Reviewed commit: 15cbd401b839

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 5b6273940454

Connector PR Review: CXH-1390 CXP-481 Support for Incremental Sync

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base a934ace6a209.
Review mode: incremental since 15cbd401
View review run

Review Summary

This commit changes how the event feed reads cursor values:

• A new cursorTimestamp helper replaces the silent fallback to since. When the cursor column is missing or NULL, it now returns a FailedPrecondition error.
• toTime now parses string and []byte values with parseTimeWithEngine, so engine-specific formats like Vertica's -03 offset are accepted.
• Integer and float values are read as Unix epochs in seconds or milliseconds, using a shared unixEpochUnit heuristic that is now also used in parseTime.
• GrantsEventFeed.staticValidate now compiles the resource_id CEL expression, so a bad expression fails at validation time.

The new behavior has tests: missing cursor columns on both the resource feed and the grant feed, epoch types and ranges, and the invalid-CEL case. I scanned the full PR diff for security and correctness issues and found no injection, nil-safety, or swallowed-error problems.

I applied the repo-local criteria to the event-feed paths:

• S1/S5 (data loss on empty results): the only per-row continue in processGrantPage (event_feed.go:328) skips a mapping that is filtered out or evaluates empty. No error is skipped, and every error is returned.
• E2 (error codes): FailedPrecondition is reasonable for a misconfigured query, and it survives %w wrapping, which the new test checks.
• Sections A and C: the commit adds no new logging and no spans.

Coverage note: earlier PR revisions were reviewed in previous runs. In this run I examined the new commit line by line and pattern-scanned the rest of the diff.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (medium confidence) pkg/bsql/validate.go:284: offset pagination over ORDER BY <cursor>, <pk> with a fixed CurrentSince (event_feed.go:86-99) can skip a row at a page boundary.
  • How: if an earlier-page row is updated mid-cycle, it moves to the end of the result set and every later row shifts back one position.
  • Why it is missed: the skipped row is not re-read when its timestamp is older than maxSeen - overlap, so the change waits for the next full sync.
  • Docs: the README Event Feed section (lines 84-90) does not mention this limitation.
• New (low confidence) pkg/bsql/event_feed.go:484-487: numeric epochs only work when the driver returns Go numeric types.
  • DECIMAL/NUMERIC epoch columns (for example 1740830400.500) come back as []byte/string from the MySQL and Postgres drivers. They go through parseTimeWithEngine/parseTime, which only accept integer epoch strings, so the feed fails with a parse error.
  • The failure is loud, not silent, but it narrows the "accept numeric epochs" claim.

Resolved prior findings

• Missing cursor column falling back to since and never advancing maxSeen: fixed. cursorTimestamp (pkg/bsql/event_feed.go:458-472) now returns FailedPrecondition for a missing or NULL column. It is called at event_feed.go:236 (resource feed) and :304 (grant feed), and tested in event_feed_test.go ("…without cursor column fails with FailedPrecondition").

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/bsql/validate.go`:
- Around line 284 (validateEventFeedPagination), together with pkg/bsql/event_feed.go lines 86-99: offset pagination over a timestamp-ordered query with a fixed `since` can skip a row when an earlier-page row is updated between pages, because every later row shifts back one position. Either switch event feeds to keyset pagination on (cursor_column, primary_key), e.g. `WHERE col > ?<last_ts> OR (col = ?<last_ts> AND pk > ?<last_pk>)`, or document in the README "Event Feed" section that rows updated mid-cycle can be missed until the next full sync.

In `pkg/bsql/event_feed.go`:
- Around line 484-487 (toTime, string/[]byte cases): DECIMAL/NUMERIC epoch columns such as "1740830400.500" come back from the MySQL and Postgres drivers as []byte or string, and parseTimeWithEngine/parseTime only accept integer epoch strings, so they fail to parse. When no time format matches, try strconv.ParseFloat and fall back to unixEpochFloatToTime. Add a test case for a decimal epoch string.

[github-actions]

Connector PR Review: CXH-1390 CXP-481 Support for Incremental Sync

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base a934ace6a209.
Review mode: incremental since 15cbd401
View review run

Review Summary

This commit changes how the event feed reads cursor values:

• A new cursorTimestamp helper replaces the silent fallback to since. When the cursor column is missing or NULL, it now returns a FailedPrecondition error.
• toTime now parses string and []byte values with parseTimeWithEngine, so engine-specific formats like Vertica's -03 offset are accepted.
• Integer and float values are read as Unix epochs in seconds or milliseconds, using a shared unixEpochUnit heuristic that is now also used in parseTime.
• GrantsEventFeed.staticValidate now compiles the resource_id CEL expression, so a bad expression fails at validation time.

The new behavior has tests: missing cursor columns on both the resource feed and the grant feed, epoch types and ranges, and the invalid-CEL case. I scanned the full PR diff for security and correctness issues and found no injection, nil-safety, or swallowed-error problems.

I applied the repo-local criteria to the event-feed paths:

• S1/S5 (data loss on empty results): the only per-row continue in processGrantPage (event_feed.go:328) skips a mapping that is filtered out or evaluates empty. No error is skipped, and every error is returned.
• E2 (error codes): FailedPrecondition is reasonable for a misconfigured query, and it survives %w wrapping, which the new test checks.
• Sections A and C: the commit adds no new logging and no spans.

Coverage note: earlier PR revisions were reviewed in previous runs. In this run I examined the new commit line by line and pattern-scanned the rest of the diff.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (medium confidence) pkg/bsql/validate.go:284: offset pagination over ORDER BY <cursor>, <pk> with a fixed CurrentSince (event_feed.go:86-99) can skip a row at a page boundary.
  • How: if an earlier-page row is updated mid-cycle, it moves to the end of the result set and every later row shifts back one position.
  • Why it is missed: the skipped row is not re-read when its timestamp is older than maxSeen - overlap, so the change waits for the next full sync.
  • Docs: the README Event Feed section (lines 84-90) does not mention this limitation.
• New (low confidence) pkg/bsql/event_feed.go:484-487: numeric epochs only work when the driver returns Go numeric types.
  • DECIMAL/NUMERIC epoch columns (for example 1740830400.500) come back as []byte/string from the MySQL and Postgres drivers. They go through parseTimeWithEngine/parseTime, which only accept integer epoch strings, so the feed fails with a parse error.
  • The failure is loud, not silent, but it narrows the "accept numeric epochs" claim.

Resolved prior findings

• Missing cursor column falling back to since and never advancing maxSeen: fixed. cursorTimestamp (pkg/bsql/event_feed.go:458-472) now returns FailedPrecondition for a missing or NULL column. It is called at event_feed.go:236 (resource feed) and :304 (grant feed), and tested in event_feed_test.go ("…without cursor column fails with FailedPrecondition").

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/bsql/validate.go`:
- Around line 284 (validateEventFeedPagination), together with pkg/bsql/event_feed.go lines 86-99: offset pagination over a timestamp-ordered query with a fixed `since` can skip a row when an earlier-page row is updated between pages, because every later row shifts back one position. Either switch event feeds to keyset pagination on (cursor_column, primary_key), e.g. `WHERE col > ?<last_ts> OR (col = ?<last_ts> AND pk > ?<last_pk>)`, or document in the README "Event Feed" section that rows updated mid-cycle can be missed until the next full sync.

In `pkg/bsql/event_feed.go`:
- Around line 484-487 (toTime, string/[]byte cases): DECIMAL/NUMERIC epoch columns such as "1740830400.500" come back from the MySQL and Postgres drivers as []byte or string, and parseTimeWithEngine/parseTime only accept integer epoch strings, so they fail to parse. When no time format matches, try strconv.ParseFloat and fall back to unixEpochFloatToTime. Add a test case for a decimal epoch string.

---

Reviewed commit: 5b6273940454

[github-actions]

No blocking issues found — see the full review report

[ggreer]

I noticed a couple of minor things, but this looks good to merge. Feel free to address them in this PR, a follow-up PR, or not at all.

[ggreer]

Could we call this in New()? Then we could return the error and avoid the mutex + bool.

[ggreer]

Nitpick: If this is the maximum time to look back, should it be called MaxLookback?