Skip to content

refactor(cli): filter compute logs on the subservice column - #6578

Open
johnstonmatt wants to merge 1 commit into
developfrom
FUNC-879/compute/change-log-schema
Open

johnstonmatt wants to merge 1 commit into
developfrom
FUNC-879/compute/change-log-schema

Conversation

@johnstonmatt

@johnstonmatt johnstonmatt commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

The compute logs query relied on log_attributes['source'] to identify the stream, but the writer now publishes this as a top-level subservice column rather than a log_attributes key, so filtering switches to that column directly.

  • Filter and project on the top-level subservice column instead of log_attributes['source'] in computeLogsQuery
  • Update unit and integration tests to assert on subservice and confirm log_attributes['source'] is no longer referenced
  • Update SIDE_EFFECTS.md to describe the new filter columns

The writer publishes subservice in sandbox and staging today; prod picks it up with the next workers-infra image bump, so compute logs returns no rows against prod until then.

@johnstonmatt
johnstonmatt force-pushed the FUNC-879/compute/change-log-schema branch from eac7755 to dcffa2d Compare September 16, 2026 06:13
@johnstonmatt johnstonmatt changed the title refactor(compute): filter logs on subservice column, not source attribute refactor(cli): filter compute logs on the subservice column Sep 16, 2026
@johnstonmatt
johnstonmatt marked this pull request as ready for review September 16, 2026 06:13
@johnstonmatt
johnstonmatt requested a review from a team as a code owner September 16, 2026 06:13

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 AI Review

Both independent reviews completed. Claude reported five findings and Codex reported none. After checking the changed code, trusted conventions, and actual PR body, all five Claude findings are refuted: the alleged production rollout gap is not stated in the PR body, the referenced comments and fixtures remain accurate, and the SQL assertions protect the intended API request shape. No confirmed issues remain.

Findings

No issues found.

Refuted findings (kept for transparency, not posted as review comments)
  • apps/cli/src/shared/compute/compute-logs.sql.ts:156 (compatibility): Using subservice without a legacy fallback breaks compute logs for production projects whose writers have not been upgraded.
    Refuted: The asserted rollout state is contradicted by the actual PR body: it says the writer now publishes the stream as top-level subservice and contains no claim that only sandbox or staging has been upgraded. The source comment at compute-logs.sql.ts:50-52 likewise describes subservice as the writer's current field and metadata.source as a legacy reader field. No checked source establishes an affected production population, so absence of a fallback does not substantiate the claimed breakage.
  • apps/cli/src/commands/experimental/compute/logs/logs.handler.ts:45 (documentation): The handler comment points to an explanation of filtering on log_attributes that no longer exists.
    Refuted: The query still filters log_attributes['worker'] at compute-logs.sql.ts:155, and compute-logs.sql.ts:39-43 explicitly explains that metadata.worker lands in the ClickHouse log_attributes map. The referenced explanation therefore still exists and matches the handler comment.
  • apps/cli/src/commands/experimental/compute/logs/SIDE_EFFECTS.md:52 (documentation): The behavior contract omits a rollout caveat even though subservice is populated only in some environments.
    Refuted: The premise that subservice is populated only in some environments is not present in the actual PR body or checked code. The PR body says the writer now publishes this top-level field, while compute-logs.sql.ts:48-54 documents it as the current stream column. SIDE_EFFECTS.md accurately describes the implemented request shape, so an unsupported rollout caveat is not required.
  • apps/cli/tests/helpers/compute.ts:273 (test-coverage): The response fixture's log_attributes.source field is stale because the query no longer reads it.
    Refuted: The query still projects the entire log_attributes map at compute-logs.sql.ts:153, and compute-logs.sql.ts:51-52 explicitly says metadata.source continues carrying the stream value for legacy readers. The fixture therefore realistically includes a returned attribute even though the CLI does not use it to filter.
  • apps/cli/src/shared/compute/compute-logs.sql.unit.test.ts:21 (test-coverage): The negative legacy-string assertion is an implementation detail duplicated in the integration test and obstructs a valid compatibility fallback.
    Refuted: The actual PR body explicitly makes removal of log_attributes['source'] from the query a requirement. The unit assertion protects the pure query builder, while the integration assertion protects the command's observable API request shape, which trusted/apps/cli/CLAUDE.md defines as part of the CLI contract. The proposed compatibility fallback depends on the refuted rollout premise.

Stats

Claude findings: 5 · Codex findings: 0 · Confirmed: 0 · Refuted: 5 · Uncertain: 0


Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run

This review runs once per PR. A maintainer can request another with a /ai-review comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant