docs: correct REVIEW.md inaccuracies flagged by automated review#984
Conversation
- Marker Guard is enforced by a dedicated CI job (`marker-guard` in `ci.yml`), not "at compile time" — fixed wording that overstated when marker failures actually surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdated a single line in REVIEW.md's Marker Guard section, changing the enforcement mechanism description from "compile time" to a dedicated CI job named "marker-guard", while preserving existing guidance on marker handling. ChangesDocumentation update
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Reviewed by glm-5.2 · Input: 24.7K · Output: 2.4K · Cached: 134.1K |
Merge the 5 commits on `main` since this branch was cut (bearer-auth remote MCP #793, session_start `source` telemetry #968, aireceipts CI #992, REVIEW.md docs #983/#984) into the upstream OpenCode v1.17.9 bridge branch. 9 conflicts resolved: - `altimate/telemetry/index.ts` — keep upstream `InstallationVersion`; re-add #968's `Flag`/`source` (per-session client label). - `session/index.ts` — keep upstream `permission` readonly cast AND #968's `metadata: row.metadata`. Session `metadata` already exists upstream (core `SessionTable`, upstream PR #23068), so #968's persistence layer is redundant. - `session/session.sql.ts` — accept upstream deletion (persistence moved to `@opencode-ai/core`); dropped #968's duplicate migration `20260626041744_elite_malcolm_colcord` (would crash "duplicate column: metadata" against upstream's `20260511173437_session-metadata`). - `test/session/session.test.ts` — keep upstream Effect harness (main's metadata cases were the redundant old-harness variant). - `config/config.ts` — take upstream (schema relocated to core Effect Schema); re-apply #793's `headersCommand`/`oauth` normalize passthrough. - `mcp/index.ts` — re-port #793 (bearer-auth) onto upstream's Effect-based MCP rewrite: `resolveHeadersCommand` (execFile, no shell), case-insensitive `mergeHeaders`, lenient `listTools` retry, OAuth auto-disable folded into upstream's `supportsOAuth`, `maskString` on failures. Adapted `Either`→`Result` (effect 4.0.0-beta.74). - `core/v1/config/mcp.ts` — add `headersCommand` to the Effect Schema `Remote` struct as a mutable non-empty string-array record (`Schema.mutable(...).check (Schema.isNonEmpty())`), so `DeepMutable`-wrapped consumers type-check. - `test/mcp/headers.test.ts`, `test/mcp/mcp-bearer-auth.test.ts` — re-expressed #793's schema/integration tests against the Effect Schema and `it.instance` harness (Zod `safeParse` → `Schema.decodeUnknownSync`). - SDK artifacts kept at the upstream-HEAD baseline (regenerated by CI/release). Verification: all 13 packages typecheck clean (`bun turbo typecheck`); MCP + session + telemetry suites 194/194; bearer-auth + headers 32/32. Zero conflict markers. Marker guard non-strict pass (CI mode for upstream-merge PRs). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Follow-up to #983 — corrects a wording inaccuracy in
REVIEW.mdflagged by automated review (CodeRabbit).REVIEW.mdstated theMarker Guard/altimate_changemarker check is "CI-enforced at compile time." It isn't — it's a dedicatedmarker-guardjob inci.yml(bun run script/upstream/analyze.ts --markers), separate from the typecheck step. The old wording overstated the guarantee and could mislead reviewers about when marker failures actually surface. Fixed to describe it as a CI job, not a compile-time check.Test plan
.github/workflows/ci.yml(marker-guardjob, line 448) that the marker check runs as its own CI step, not during typecheck/compile.wc -m REVIEW.md→ 9603 (under the 10,000 char budget).🤖 Generated with Claude Code
Summary by cubic
Corrected REVIEW.md to fix an inaccuracy about Marker Guard enforcement. Clarifies the check is handled by a dedicated CI job (
marker-guardinci.yml), not at compile time.Written for commit 5199576. Summary will update on new commits.
Summary by CodeRabbit